Repository navigation
chore: single-source CLI normalization helpers - #389
Conversation
Share create/update legacy none collection normalization and runtime unset field resolution so the SDK-backed command primitives have one canonical implementation. Export ConfigKey from the positional config router and reuse it in config command handling to remove duplicated config surface metadata. PM evidence: pm-92if records GitHub/dependency checks, temp-dir smoke, pm-changelog verification, Sentry/telemetry, static quality, and full 100/100/100/100 coverage.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
@codex review |
📝 WalkthroughWalkthroughExtracts two shared helpers— ChangesShared CLI helpers and command consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
To use Codex here, create a Codex account and connect to github. |
|
✅ Action performedFull review finished. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors several CLI commands to eliminate duplicate code and types. It consolidates legacy none/null collection normalization and runtime unset field resolution into shared utility functions, which are now reused across the create and update commands. Additionally, it exports the canonical ConfigKey type from the positional config router to remove a duplicate type definition in config.ts. There are no review comments on these changes, and I have no additional feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Code Review
This pull request refactors and single-sources several shared utilities across CLI commands to reduce duplication. Specifically, it consolidates legacy none/null collection normalization into legacy-none-tokens.ts and runtime --unset field resolution into shared-unset-fields.ts, reusing these helpers in both create.ts and update.ts. Additionally, it exports ConfigKey from positional-value.ts to eliminate duplicate type definitions in config.ts. There are no review comments, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 8 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR centralizes two previously duplicated CLI helpers: the legacy
Confidence Score: 5/5Safe to merge — pure extraction of duplicate logic into shared helpers with no behavioral regressions in the happy path and improved defensive handling of malformed registry data. Both extracted helpers faithfully replicate the old per-command behavior: the legacy-none normalizer is functionally identical to the removed loops (with an added non-string entry guard), and the runtime unset resolver correctly scopes to per-command field lists and normalizes tokens to lowercase. Test-only aliases preserve the existing test surface, fixtures were updated to supply command_to_fields, and new regression cases cover the malformed-registry edge cases. The ConfigKey rename is a rename-and-export with no logic change. No files require special attention. Important Files Changed
Reviews (4): Last reviewed commit: "fix: normalize runtime unset candidates" | Re-trigger Greptile |
Address current-head Cubic review feedback on PR #389 by resolving runtime --unset fields only from the active command_to_fields registry and by rejecting malformed non-string legacy none/null collection payloads with usage errors. Update command helper fixtures and legacy none-token tests so the new guard branch remains covered under the repo-wide 100/100/100/100 coverage gate. Verification: pm test pm-92if --run --progress passed 8/8; node scripts/run-tests.mjs coverage passed 256 files, 5138 tests, 100/100/100/100; pnpm lint, typecheck, quality:static, and git diff --check passed.
|
Review fix pushed in f1252cb. Re-running review agents on the current head. @codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Code Review
This pull request refactors the CLI commands to eliminate duplicate logic by introducing shared helpers. Specifically, it extracts legacy none/null collection token normalization and runtime schema unset field resolution into shared modules, and single-sources the config key type definition. Unit tests have been updated to align with these changes. The feedback suggests adding defensive checks to the shared unset field resolution helper to prevent potential runtime TypeErrors if dynamically loaded registry fields are missing expected properties.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const candidates = new Set<string>([ | ||
| definition.key, | ||
| definition.metadata_key, | ||
| definition.cli_flag.replaceAll("-", "_"), | ||
| definition.cli_flag, | ||
| ...definition.cli_aliases.map((alias) => alias.replaceAll("-", "_")), | ||
| ...definition.cli_aliases, | ||
| ]); |
There was a problem hiding this comment.
Defensive checks should be added when resolving runtime unset field definitions. If cli_flag or cli_aliases are missing or undefined in the registry (which is loaded dynamically), calling replaceAll or map directly on them will throw a runtime TypeError. Adding safe guards ensures robustness at dynamic boundaries.
const candidates = new Set<string>();
if (definition.key) candidates.add(definition.key);
if (definition.metadata_key) candidates.add(definition.metadata_key);
if (definition.cli_flag) {
candidates.add(definition.cli_flag.replaceAll("-", "_"));
candidates.add(definition.cli_flag);
}
if (Array.isArray(definition.cli_aliases)) {
for (const alias of definition.cli_aliases) {
candidates.add(alias.replaceAll("-", "_"));
candidates.add(alias);
}
}References
- Do not remove runtime type checks or defensive guards at dynamic import boundaries (such as loading an SDK or module from a configured path), even if the expected module is statically typed. Statically typed shapes do not guarantee actual runtime exports for dynamically imported modules, and runtime validation prevents failures further down the execution path.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/commands/shared-unset-fields.ts`:
- Around line 160-168: The candidate matching in shared-unset-fields.ts is
inconsistent because `definition.key` is compared without the same normalization
applied to `parseCreateUnsetTargets()` and `parseUpdateUnsetTargets()`. Update
the candidate set construction in the helper that builds `candidates` so
`definition.key` is normalized the same way as the lower-cased `--unset` token
(for example, matching the existing lowercase/underscore handling used for
`metadata_key`, `cli_flag`, and `cli_aliases`), ensuring runtime keys like
`githubUrl` still match their `--unset` input.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 66d83cf4-0b19-4793-a6d4-2b640cdfb4a3
📒 Files selected for processing (11)
.agents/pm/epics/pm-92if.toon.agents/pm/history/pm-92if.jsonlsrc/cli/commands/config.tssrc/cli/commands/create.tssrc/cli/commands/legacy-none-tokens.tssrc/cli/commands/shared-unset-fields.tssrc/cli/commands/update.tssrc/core/config/positional-value.tstests/unit/commands/create-command.spec.tstests/unit/commands/legacy-none-tokens.spec.tstests/unit/commands/update-command.spec.ts
There was a problem hiding this comment.
1 issue found across 9 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Harden shared runtime unset field resolution against malformed dynamic registry metadata before matching create/update unset tokens. Document Gemini review evidence on pm-92if and add malformed alias coverage to preserve full branch coverage.
|
Gemini review fix pushed in 86899ee. Re-running review agents on current head. @codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Code Review
This pull request refactors and centralizes shared CLI command helpers to improve code quality and reduce duplication. Specifically, it exports the canonical ConfigKey type from positional-value.ts to eliminate duplicate definitions in config.ts. It also extracts and centralizes legacy none/null collection normalization into legacy-none-tokens.ts and runtime unset field resolution into shared-unset-fields.ts, reusing these helpers across both the create and update commands. Unit tests have been updated and expanded to cover these centralized helpers, including handling of malformed payloads. There are no review comments to address.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Normalize runtime unset lookup tokens and candidate forms so camelCase dynamic field keys still match lowercased --unset input. Record current-head review evidence on pm-92if and cover the lowercased key lookup path.
|
Runtime unset normalization fix pushed in a800b3f. Re-running review agents on current head. @codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Code Review
This pull request refactors and consolidates duplicate code across the create and update commands by introducing shared helpers for normalizing legacy none/null collection tokens and resolving runtime --unset field definitions. Additionally, it single-sources the canonical ConfigKey type definition from positional-value.ts to be reused in config.ts, and updates unit tests to cover these shared behaviors. There are no review comments, so no further feedback is provided.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
PM context: pm-92if. Historical linkage verified during the 2026-08-04 ecosystem review. This comment records canonical context only; it does not reopen or change this artifact. |
Summary
none/nullcollection normalization inlegacy-none-tokens.--unsetfield resolution inshared-unset-fieldswhile preserving existing test-only aliases.ConfigKeyunion from the positional config router and reuse it inconfig.ts.pm-92if.Verification
node scripts/run-tests.mjs coverage-> 256 files, 5137 tests, 100/100/100/100 coverage.pm test pm-92if --run --progress-> 7/7 linked checks passed, including Sentry/telemetry best-effort.pnpm lintpnpm typecheckpnpm quality:static-> duplicate_chunks=0, docstring coverage 100%.pnpm contracts:checkpnpm security:scanpnpm changelog:pm:check-> pm-changelog 2026.6.27, CHANGELOG.md unchanged.pnpm audit --audit-level moderatepm health --check-only --summary --jsonpm validate --check-resolution --check-history-drift --json-> history drift ok; existing closed-item resolution metadata warning backlog remains.node dist/cli.js package doctor --project --detail deep --jsonPM_PATH/PM_GLOBAL_PATHpassed init/config/create/update/files/docs/test/comments/close/release/health/validate.GitHub sweep
pnpm audit --audit-level moderateclean.RELEASE_PATconfiguration, not by code in this branch.Summary by cubic
Centralized CLI helpers for legacy "none"/"null" collection handling and runtime
--unsetfield resolution, shared bycreateandupdate. Exported canonicalConfigKey, normalized unset matching, and hardened guards to reduce duplicates while keeping behavior strict.Refactors
--unsetresolver;create/updatereuse it and test-only aliases still work.ConfigKeyfrom the positional router and reused it inconfig.Bug Fixes
--unsettocommand_to_fields.Written for commit a800b3f. Summary will update on new commits.