fix(cli): let --change find change names that exist on disk - #1375
Conversation
The --change flag on status and instructions validated names with the creation-time kebab-case rule, so digit-leading names (e.g. the date-prefixed convention 2026-07-04-voice-copilot-v1) were rejected at parse time even though list, validate, and archive all handle them. Lookup now only guards against unsafe directory names (path separators, relative segments, null bytes, hidden entries) and otherwise accepts whatever getAvailableChanges could return. Creating a change still enforces kebab-case via validateChangeName. Fixes #1308 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe ChangesChange Lookup
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant WorkflowCommand
participant validateChangeExists
participant ChangeDirectory
WorkflowCommand->>validateChangeExists: provide --change name
validateChangeExists->>validateChangeExists: validate lookup safety
validateChangeExists->>ChangeDirectory: resolve existing change directory
ChangeDirectory-->>validateChangeExists: return directory result
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
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/commands/workflow/shared.ts`:
- Around line 136-160: Update validateChangeLookupName to reject the reserved
change name "archive", matching the exclusion applied by getAvailableChanges
before returning undefined for otherwise safe names.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5486705e-83a4-4323-8ac1-a95959652629
📒 Files selected for processing (3)
.changeset/change-lookup-accepts-existing-names.mdsrc/commands/workflow/shared.tstest/commands/artifact-workflow.test.ts
Matches the getAvailableChanges filter so --change archive can't address the archive directory as if it were a change (CodeRabbit review). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
alfred-openspec
left a comment
There was a problem hiding this comment.
Reviewed the diff, linked issue, surrounding workflow/apply paths, CI, and focused local tests. Looks good to merge.
Fixes #1308.
What was wrong
Any command taking
--change(status,instructions <artifact>,instructions apply) rejected digit-leading change names at parse time:Meanwhile
list,validate, andarchiveall handled the same change fine, so a change could live its whole lifecycle while being unaddressable by--change. Repos with a date-prefixed naming convention (which the archive flow itself produces) couldn't use the status/instructions workflow at all.The cause:
validateChangeExists()reusedvalidateChangeName()— the creation-time kebab-case rule — as its path-traversal guard, so the lookup path enforced a naming convention on directories that already exist.How it was fixed
Lookup and creation are now separate rules, matching how the rest of the CLI behaves:
--change) accepts any directory name thatopenspec listcould return, and only rejects unsafe names: path separators,./.., null bytes, and hidden (dot-leading) entries.openspec new change) still enforces kebab-case viavalidateChangeName()— this PR takes no position on Support numbered change names (NNNNN-slug) for sequencing #1169 (numbered names at creation).One small function in
src/commands/workflow/shared.ts; no other call sites touched.Replication / proof
Before (repro from the issue, real output):
After:
Tests: new cases for digit-leading names on
statusandinstructions, plus a hidden-name rejection; the existing traversal tests (../foo,/etc/passwd,foo/bar) pass unchanged. Full suite: 1,861 passed (the 17 zsh-installer failures are the known local-environment ones).Notes
list/archivealready resolve those directories.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--changeoption now accepts any safe existing change name on disk, including date- or digit-prefixed names.Bug Fixes
./.., and null characters) are still rejected.archiveis rejected when provided via--change.Tests
statusandinstructionschange-name validation and acceptance.Documentation
--changeoption accordingly.