fix(cli): remove deprecated noun command groups - #1626
Conversation
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe CLI removes the deprecated ChangesLegacy command removal
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/commands/legacy-groups-removed.test.ts (1)
68-73: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert unknown-command behavior.
A nonzero exit code can also occur when a registered command group fails in its
listsubcommand. Assert that the combined output reports an unknown command. This makes the test prove that the deleted group is not still registered.Proposed test update
expect(result.exitCode).not.toBe(0); - expect(`${result.stdout}\n${result.stderr}`).not.toContain('deprecated'); + const output = `${result.stdout}\n${result.stderr}`; + expect(output).toMatch(/unknown command/i); + expect(output).not.toContain('deprecated');🤖 Prompt for 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. In `@test/commands/legacy-groups-removed.test.ts` around lines 68 - 73, Update the test loop in legacy-groups-removed.test.ts to assert that the combined stdout and stderr explicitly reports an unknown command, in addition to the existing nonzero exit-code check and deprecated-output assertion. This ensures each removed group is unregistered rather than merely failing its list subcommand.
🤖 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 `@docs/stores-beta/user-guide.md`:
- Around line 430-431: Update the command-scope statement in the user guide to
keep view listed as supporting --store <id>; identify only templates and schemas
as current-directory-only commands, while preserving the surrounding deprecated
noun-form documentation.
In `@test/commands/list-specs.test.ts`:
- Line 83: Update the root-path assertion around output.root to compare
canonical paths: canonicalize output.root.path with
FileSystemUtils.canonicalizeExistingPath() and projectDir with
fs.realpathSync.native() before applying the expectation, preserving the
existing object-shape assertion.
---
Nitpick comments:
In `@test/commands/legacy-groups-removed.test.ts`:
- Around line 68-73: Update the test loop in legacy-groups-removed.test.ts to
assert that the combined stdout and stderr explicitly reports an unknown
command, in addition to the existing nonzero exit-code check and
deprecated-output assertion. This ensures each removed group is unregistered
rather than merely failing its list subcommand.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e1a1f866-81ca-4b11-b1de-7c3f94c9c881
📒 Files selected for processing (31)
.changeset/remove-legacy-noun-command-groups.mddocs/agent-contract.mddocs/stores-beta/user-guide.mdopenspec/specs/cli-change/spec.mdopenspec/specs/cli-show/spec.mdopenspec/specs/cli-spec/spec.mdopenspec/specs/cli-validate/spec.mdopenspec/specs/openspec-conventions/spec.mdsrc/cli/index.tssrc/commands/change.tssrc/commands/show.tssrc/commands/spec.tssrc/commands/validate.tssrc/core/completions/command-registry.tssrc/core/completions/types.tssrc/core/root-selection.tssrc/core/validation/constants.tstest/cli-e2e/validate-scenario-loss.test.tstest/cli-e2e/validate-task-numbering.test.tstest/commands/change.interactive-show.test.tstest/commands/change.interactive-validate.test.tstest/commands/legacy-groups-removed.test.tstest/commands/list-specs.test.tstest/commands/show.test.tstest/commands/spec.interactive-show.test.tstest/commands/spec.interactive-validate.test.tstest/commands/spec.test.tstest/commands/validate.enriched-output.test.tstest/core/commands/change-command.list.test.tstest/core/commands/change-command.show.test.tstest/core/completions/command-registry.test.ts
💤 Files with no reviewable changes (7)
- test/commands/spec.test.ts
- test/commands/change.interactive-validate.test.ts
- test/commands/spec.interactive-show.test.ts
- test/core/commands/change-command.list.test.ts
- test/commands/spec.interactive-validate.test.ts
- src/core/completions/command-registry.ts
- test/commands/change.interactive-show.test.ts
| - **Some commands stay where they are.** `view`, `templates`, and `schemas` | ||
| act on the current directory only — no `--store`. The deprecated noun-form |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document view --store support.
view supports --store <id>. These lines state the inverse. List only templates and schemas as current-directory-only commands.
Proposed fix
- **Some commands stay where they are.** `view`, `templates`, and `schemas`
+ **Some commands stay where they are.** `templates` and `schemas`
act on the current directory only — no `--store`.Based on learnings: openspec view supports --store <id> and must remain store-capable.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **Some commands stay where they are.** `view`, `templates`, and `schemas` | |
| act on the current directory only — no `--store`. The deprecated noun-form | |
| - **Some commands stay where they are.** `templates` and `schemas` | |
| act on the current directory only — no `--store`. The deprecated noun-form |
🤖 Prompt for 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.
In `@docs/stores-beta/user-guide.md` around lines 430 - 431, Update the
command-scope statement in the user guide to keep view listed as supporting
--store <id>; identify only templates and schemas as current-directory-only
commands, while preserving the surrounding deprecated noun-form documentation.
Source: Learnings
| { id: 'auth', requirementCount: 2 }, | ||
| { id: 'payment', requirementCount: 1 }, | ||
| ]); | ||
| expect(output.root).toMatchObject({ path: projectDir }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Canonicalize the root path before comparing it.
output.root.path and projectDir identify an existing filesystem path. Compare canonical paths so this assertion remains valid through aliases and platform path normalization.
As per coding guidelines, “When asserting existing filesystem paths as identities, canonicalize both actual and expected paths first using FileSystemUtils.canonicalizeExistingPath() in project code and fs.realpathSync.native() in test-only expectations.”
Proposed test update
-import { promises as fs } from 'node:fs';
+import { promises as fs, realpathSync } from 'node:fs';
...
- expect(output.root).toMatchObject({ path: projectDir });
+ expect(realpathSync.native(output.root.path)).toBe(
+ realpathSync.native(projectDir)
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(output.root).toMatchObject({ path: projectDir }); | |
| expect(realpathSync.native(output.root.path)).toBe( | |
| realpathSync.native(projectDir) | |
| ); |
🤖 Prompt for 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.
In `@test/commands/list-specs.test.ts` at line 83, Update the root-path assertion
around output.root to compare canonical paths: canonicalize output.root.path
with FileSystemUtils.canonicalizeExistingPath() and projectDir with
fs.realpathSync.native() before applying the expectation, preserving the
existing object-shape assertion.
Source: Coding guidelines
1 participant
Summary
openspec change ...andopenspec spec ...command groups from Commander help and shell completionsopenspec show,openspec list, andopenspec validatebehavior, while removing unreachable noun-command implementationsBreaking change
Scripts using the removed noun-form commands must migrate to:
openspec show <item> --type change|specopenspec list --changes|--specsopenspec validate <item> --type change|specThe deprecated noun-form
--longand raw JSON contracts are not migrated.Validation
pnpm buildpnpm lintCI=true pnpm test— 123 files passed, 1 skipped; 3776 tests passed, 66 skipped🤖 Generated with Claude Code
Summary by CodeRabbit
Breaking Changes
openspec change ...andopenspec spec ...command groups.openspec show,openspec list, andopenspec validateinstead.--type change|specwhen the item type is ambiguous.Documentation
Tests