fix(cli): resolve store pointer for view command - #1455
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe ChangesView store support
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CLI
participant RootResolver
participant ViewCommand
CLI->>RootResolver: resolve store or store path
RootResolver-->>CLI: return resolved root
CLI->>ViewCommand: execute with root.path
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
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/index.ts`:
- Line 258: Update the root resolution call in the view command to pass
allowImplicitRoot: false to resolveRootForCommand, ensuring openspec view does
not fall back to the current directory when no configured or discovered root
exists.
🪄 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 Plus
Run ID: ad4022df-95b3-49a9-9988-fd38bd6df6ed
📒 Files selected for processing (1)
src/cli/index.ts
278d065 to
35db0cf
Compare
alfred-openspec
left a comment
There was a problem hiding this comment.
The root-resolution direction is right, but adding --store to view without updating the completion registry leaves two tests failing on Linux, macOS, and Windows. Please update the registry/parity expectations and add a focused selected-store or store-pointer view regression.
Code review — verdict: needs rework (CI is red), but the fix itself is correct and worth landingThe diagnosis and the fix are both right. The problem is that adding 1. Real need — yes, confirmed with a reproduction
So on 2. Does the fix work — yes, on every path I tested
3. Breaking changes — none foundLocal-project behaviour is unchanged; the only new output is the store banner, and only when a store is actually in play. 4. Scope — correctly surgicalTen lines, aligning one command with the shared root-resolution helper every other command already uses. Exactly the right shape. What's blocking: CI is failing, and the fix is bigger than one line
This repo pins the CLI flag surface in three places that must move together. Adding
Important consequence: pnpm build && pnpm generate:skills && pnpm regen:parity-hashesthen re-run the parity tests ( Also worth addressing while you're in there
Once CI is green with the three files above in sync, I'd be happy with this — the underlying change is a good catch and clearly the right direction. |
df1a162 to
5215936
Compare
|
Okay, I've gone through and fixed those. there were a lot of skills that needed to be updated. Sorry about that! I was just doing some practical testing where the fix was the minimum i needed to do to get Should be good now! |
5215936 to
08dae9c
Compare
alfred-openspec
left a comment
There was a problem hiding this comment.
The implementation now works and the completion/skill surfaces are in sync, but the focused selected-store or store-pointer view regression requested in the prior review is still missing. Please add view to the store root-selection CLI test so this exact wiring bug cannot silently return.
# Conflicts: # test/core/templates/skill-templates-parity.test.ts
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Dropping the implicit-root fallback made view reject a pre-config.yaml openspec/ directory that list and status still accept, so projects initialized before config.yaml existed lost the dashboard entirely. view now resolves the root the same way its siblings do. Adds the store-pointer, --store, and fallback regression coverage the review asked for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thank you @c4patino ❤️ Pushed three commits to your branch to unblock this — conflicts are resolved and CI should go green. Nothing of yours was rewritten; it's all additive on top of 1. Merge conflict ( Worth knowing for next time, since it's documented in a gitignored file you can't see: never hand-merge 2. Changeset ( 3. One behavior fix ( I reverted the That made Verified against a 4. Tests. Added Full suite locally: 113 files / 2295 tests passing. Nice catch on the underlying bug — a store-pointer project silently rendering an empty dashboard while |
c4patino
left a comment
There was a problem hiding this comment.
i've taken a look at it, i'm good with the changes that you made!
alfred-openspec
left a comment
There was a problem hiding this comment.
Reviewed at 4c4b4cb. The new CLI regressions cover config store pointers, explicit --store selection, legacy pre-config roots, ambiguous registered-store resolution, and missing roots; 97 focused tests, build, lint, the cross-platform suite, audit, dependency review, and release tracking all pass.
Description
The
openspec viewcommand was hardcoding'.'as the target path andignoring the shared root resolution used by all other commands (list,
validate, etc.). When the project uses a
store:pointer inopenspec/config.yaml(external store), the command would look for specsand changes in the local
openspec/directory instead of the externalstore's root.
Now
viewaccepts--store <id>and properly resolves the configuredstore pointer via
resolveRootForCommand, matching the behavior of otherCLI commands.
Changes
src/cli/index.ts— Added--store <id>option andhiddenStorePathOption()to theviewcommand; useresolveRootForCommandbefore executing.Summary by CodeRabbit
--store <id>support to theopenspec viewcommand (including shell completion).--store-pathoption for advanced workflows.openspec viewnow runs against the resolved OpenSpec root when available, rather than defaulting to the current directory.openspec viewin the list of commands that accept--store <id>.viewstore-flag behavior.