[Shellghostt]

Closes #359

Summary

Adds direct unit tests for parseDefaultActiveSkills and parseSubscriptionUsageProviders in packages/coding-agent/src/core/settings-list-values.ts, which previously had no dedicated test file. Tests only; no source changes.

Coverage

• parseDefaultActiveSkills: undefined/empty array, valid kebab-case names kept in order, duplicates dropped first-seen, invalid names rejected (Foo, foo_bar, -foo, foo-, foo--bar, empty), 64-character name accepted / 65 rejected, 64 entries accepted / 65 rejected, non-array and null rejected — with the exact error messages from the implementation.
• parseSubscriptionUsageProviders: undefined passthrough, empty array, IDs kept in order, padded IDs kept as-is, empty/whitespace-only and non-array inputs rejected, and the returned array is a copy of the input.

Tests

• node ../../node_modules/vitest/dist/cli.js --run test/settings-list-values.test.ts (from packages/coding-agent) — 19 passed.
• biome check test/settings-list-values.test.ts — clean after formatting.

[dmae97]

OMK Community

Thanks for opening this pull request. Mention @omk-community with reproduction steps, a question, or the review help you need.

Automated response. No code changes or merges are performed.

[dmae97]

Reviewed the single test-file diff at 30604045 against main (4896be96). One coverage issue needs correction: the empty/whitespace provider-ID cases currently exercise non-array rejection instead of invalid array entries. Details are inline.

This is a source review; I have not independently run Vitest or Biome. CI run 42437057490 is action_required, with zero jobs and zero check runs for this head. Successful repository CI is still needed before merge.

[dmae97]

it.each spreads each array row into callback arguments, so these rows pass "" and " " to value, not [""] and [" "]. parseSubscriptionUsageProviders therefore throws at !Array.isArray(value), and the assertions would still pass if the value.some(...) entry validation were removed. That leaves the empty/whitespace cases requested by #359 untested. Please pass arrays to the parser, for example use it.each(["", " "]) and call parseSubscriptionUsageProviders([value]), or wrap each array in another row. Then rerun the targeted test; removing the entry-validation clause temporarily should make these cases fail.

[Shellghostt]

Fixed in 7c55712 with the exact shape you suggested. Mutation check confirmed: removing the entry-validation clause now fails precisely these two cases. Details in the PR comment above.

[dmae97]

Rechecked 7c557127 against current main (4896be96). Both cases now pass [entry] to parseSubscriptionUsageProviders, so they reach the array-entry validation rather than the non-array guard. This source-level finding is addressed, and the full PR diff still contains only the test file.

I have not independently rerun Vitest, Biome, or the mutation check reported above. CI run 42439423299 for this head is still action_required, with zero jobs and zero check runs. Successful repository validation is still needed before merge; this follow-up is not merge approval.

[Shellghostt]

Fixed in 7c55712 — thank you, that was a real coverage hole. The cases now use it.each(["", " "]) and pass [entry] to parseSubscriptionUsageProviders, so they exercise the entry validation rather than the non-array path.

Verified as you suggested:

• Targeted test: 19 passed (node ../../node_modules/vitest/dist/cli.js --run test/settings-list-values.test.ts), biome check on the file is clean.
• Mutation check: temporarily removing the value.some(...) entry-validation clause makes exactly those two cases fail; restored afterwards, and the source file is unchanged in this PR.

CI for this head still shows as requiring approval on your side — please let me know if there's anything else you'd like adjusted.

[Shellghostt]

Follow-up on the validation gap: since the upstream CI run for this head is action_required, I enabled Actions on my fork and dispatched the repo's own CI workflow against this exact branch (test-settings-list-values at 7c55712):

• build-check-test: success — https://github.com/Shellghostt/omk/actions/runs/42441951792 (ubuntu-22.04; the workflow's npm run check + test suite, which includes the targeted settings-list-values tests).
• The fork run's secret-scan job reports 17 gitleaks findings, but that job scanned the fork's full history (28,762 commits) under workflow_dispatch; every finding is in pre-existing upstream files (e.g. packages/coding-agent/test/evidence-command.test.ts at the base commit), and none are in the only file this PR changes.

That doesn't replace the upstream run — it still needs an approval click on your side — but the branch itself now has a green full CI pass at the exact head SHA. Happy to adjust anything else you'd like changed.