Skip to content

fix(cli): resolve store pointer for view command - #1455

Merged
clay-good merged 6 commits into
Fission-AI:mainfrom
c4patino:fix/openspec-view-use-store
Jul 27, 2026
Merged

fix(cli): resolve store pointer for view command#1455
clay-good merged 6 commits into
Fission-AI:mainfrom
c4patino:fix/openspec-view-use-store

Conversation

@c4patino

@c4patino c4patino commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Description

The openspec view command was hardcoding '.' as the target path and
ignoring the shared root resolution used by all other commands (list,
validate, etc.). When the project uses a store: pointer in
openspec/config.yaml (external store), the command would look for specs
and changes in the local openspec/ directory instead of the external
store's root.

Now view accepts --store <id> and properly resolves the configured
store pointer via resolveRootForCommand, matching the behavior of other
CLI commands.

Changes

  • src/cli/index.ts — Added --store <id> option and
    hiddenStorePathOption() to the view command; use
    resolveRootForCommand before executing.

Summary by CodeRabbit

  • New Features
    • Added --store <id> support to the openspec view command (including shell completion).
    • Added a hidden --store-path option for advanced workflows.
  • Behavior Changes / Bug Fixes
    • openspec view now runs against the resolved OpenSpec root when available, rather than defaulting to the current directory.
  • Documentation
    • Updated store-selection guidance across OpenSpec skills to include openspec view in the list of commands that accept --store <id>.
  • Tests
    • Updated CLI/skill template parity and completion expectations for the new view store-flag behavior.

@c4patino
c4patino requested a review from a team as a code owner July 27, 2026 15:08
@c4patino
c4patino requested review from clay-good and removed request for a team July 27, 2026 15:08
@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The openspec view command now accepts store-related options, resolves the OpenSpec root, exits when no root is found, and runs ViewCommand against the resolved root path. Store-selection guidance and completion coverage now include view.

Changes

View store support

Layer / File(s) Summary
View command options and root resolution
src/cli/index.ts
Registers --store and hidden --store-path options, resolves the OpenSpec root, and passes root.path to ViewCommand.
Store flag registration and validation
src/core/completions/command-registry.ts, test/core/completions/command-registry.test.ts
Adds the shared store flag to view and updates completion expectations for the command and flag surfaces.
Store-selection guidance and parity records
src/core/templates/workflows/store-selection.ts, skills/openspec-*/SKILL.md, test/core/templates/skill-templates-parity.test.ts
Adds view to store-selection instructions and updates generated-content parity hashes.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: clay-good, alfred-openspec, tabishb

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
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main fix: resolving the store pointer for the view command.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between c33fcb3 and 8fd77a5.

📒 Files selected for processing (1)
  • src/cli/index.ts

Comment thread src/cli/index.ts
@c4patino
c4patino force-pushed the fix/openspec-view-use-store branch from 278d065 to 35db0cf Compare July 27, 2026 15:27

@alfred-openspec alfred-openspec left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@clay-good

Copy link
Copy Markdown
Collaborator

Code review — verdict: needs rework (CI is red), but the fix itself is correct and worth landing

The diagnosis and the fix are both right. The problem is that adding --store to a command in this repo pulls a chain of three other files with it, and CI is currently failing because of that. Details and the exact recipe below.

1. Real need — yes, confirmed with a reproduction

view really was the odd one out. I built a pointer-only project (openspec/config.yaml containing just store: mystore, no local specs/changes) against a registered store that holds one billing spec:

########## MAIN: view
  ● Specifications: 0 specs, 0 requirements          <-- wrong

########## MAIN: list --specs
Using OpenSpec root: mystore (/…/store1455)
  billing     requirements 1                          <-- correct

########## PR:   view
Using OpenSpec root: mystore (/…/store1455)
  ● Specifications: 1 specs, 1 requirements
Specifications
  ▪ billing                        1 requirement      <-- fixed

So on main, list follows the store pointer and view silently doesn't — it reports an empty dashboard rather than erroring, which is the worst kind of wrong. Genuine bug, genuine inconsistency with every sibling command.

2. Does the fix work — yes, on every path I tested

  • Normal local project (no store): output is byte-identical to main. root.path is the project root, so ViewCommand.execute()'s path.join(targetPath, 'openspec') still resolves correctly. I checked this specifically because passing the wrong level here would have broken view outright.
  • Pointer-only project: now follows the store (above).
  • Explicit --store mystore: works, with the standard Using OpenSpec root: banner.
  • Directory with no OpenSpec root at all: exit code stays 1, and the message actually improves:
    MAIN: No openspec directory found
    PR:   ✖ Error: No OpenSpec root found in the current directory or its ancestors.
          Registered stores: mystore. Pass --store <id> to use one, or run openspec init…
    
    allowImplicitRoot: false is the right choice for a read-only diagnostic command, matching doctor and context.

3. Breaking changes — none found

Local-project behaviour is unchanged; the only new output is the store banner, and only when a store is actually in play.

4. Scope — correctly surgical

Ten 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

Test (linux-bash), Test (macos-bash), Test (windows-pwsh) and Test are all red. Reproduced locally:

FAIL test/core/completions/command-registry.test.ts
  × matches visible Commander command flags and aliases
    → view flags: expected [] to deeply equal [ { name: 'store', takesValue: true } ]
  × uses one --store description on every lifecycle command
    → expected [ Array(10) ] to deeply equal [ Array(9) ]

This repo pins the CLI flag surface in three places that must move together. Adding --store to view requires all of:

  1. src/core/completions/command-registry.ts — add COMMON_FLAGS.store to the view entry (around line 64). Without this, shell completion won't offer --store on view even though the CLI accepts it.

  2. test/core/completions/command-registry.test.ts — add 'view' to the expected command list around line 167.

  3. src/core/templates/workflows/store-selection.ts — this is the one that's easy to miss. STORE_SELECTION_GUIDANCE currently reads:

    …pass --store <id> on the commands that read or write specs and changes (new change, status, instructions, list, show, validate, archive, doctor, context). Other commands do not take the flag.

    The test enforces that every --store-bearing command appears in that string, with this comment explaining why:

    The store-selection guidance interpolated into every generated skill enumerates exactly these commands; drift here means agents are taught a stale flag surface.

    So view has to be added to the guidance text too — otherwise agents are explicitly told view doesn't take --store.

Important consequence: STORE_SELECTION_GUIDANCE is interpolated into every generated SKILL.md, so editing it invalidates the golden hashes. After making the three edits above you'll need:

pnpm build && pnpm generate:skills && pnpm regen:parity-hashes

then re-run the parity tests (test/core/templates/skill-templates-parity.test.ts, test/core/templates/skillssh-parity.test.ts). That will legitimately touch the committed skills/*/SKILL.md files and the hash table — expected for this change, not scope creep.

Also worth addressing while you're in there

  • Dead branch. resolveRootForCommand only returns null inside its output.json error path. view passes no json flag, so in practice it either returns a root or throws (into your catchfailWithError). The if (!root) return; guard is therefore unreachable here. Harmless, but it reads as if there's a silent-exit path when there isn't. Fine to keep for symmetry with doctor/context — just be aware it isn't doing anything.
  • No test for the new behaviour. Given this is a "command X ignored the store pointer" bug, a small regression test asserting view resolves through a store pointer would stop it from silently regressing. The completion-registry test will catch the flag going missing, but not the resolution.
  • No changeset. Validate Release Tracking is passing, so it isn't mandatory, but this is a user-visible fix and probably deserves a patch note.

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.

@c4patino
c4patino force-pushed the fix/openspec-view-use-store branch 2 times, most recently from df1a162 to 5215936 Compare July 27, 2026 18:59
@c4patino

Copy link
Copy Markdown
Contributor Author

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 openspec view to actually pop out the right thing. Didn't even notice that it was used in other places.

Should be good now!

@c4patino
c4patino force-pushed the fix/openspec-view-use-store branch from 5215936 to 08dae9c Compare July 27, 2026 22:57

@alfred-openspec alfred-openspec left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

clay-good and others added 3 commits July 27, 2026 18:16
# 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>
@clay-good

clay-good commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

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 866bb97. Details so nothing is a surprise:

1. Merge conflict (0338f21). #1464 landed on main and touched the same generated skills, so the golden-hash table in skill-templates-parity.test.ts collided. Nothing to hand-resolve — those hashes and every skills/*/SKILL.md are generated. I took main's side and re-ran pnpm build && pnpm regen:parity-hashes && pnpm generate:skills.

Worth knowing for next time, since it's documented in a gitignored file you can't see: never hand-merge skills/<name>/SKILL.md — take either side and regenerate. I checked before resolving that both sides had identical it() titles and hash keys, so no test bodies were at risk.

2. Changeset (4c96f78). Added .changeset/view-resolves-store-pointer.md. view gaining --store is user-facing and would otherwise have been missing from the release notes. CI doesn't catch this — the job only validates changesets when a PR adds one.

3. One behavior fix (4c4b4cb) — the part worth your review.

I reverted the allowImplicitRoot: false that CodeRabbit suggested. It looked right but caused a real regression:

# openspec/ with project.md but no config.yaml (a pre-#565 project), no stores registered
main:  openspec view  -> exit 0, dashboard
PR:    openspec view  -> exit 1, "No OpenSpec root found from the current directory."
       openspec list  -> exit 0   # same directory, still works

That made view the only command refusing a directory its siblings accept. openspec update doesn't backfill config.yaml, so those users would have stayed broken until re-running init. Dropping the flag restores exact parity with list/status and preserves the old error text, while keeping everything your fix added.

Verified against a main build across the matrix — pointer project 0 specs -> 1 specs (your fix, confirmed), --store <id> works, nested subdirectory now works where main failed, and no-root output is unchanged. Also confirmed the Using OpenSpec root: banner goes to stderr, so nothing parsing view's stdout is affected.

4. Tests. Added test/cli-e2e/view-store-resolution.test.ts (the store-pointer regression Alfred asked for): config.yaml pointer, explicit --store, the legacy-directory fallback, and a case pinning view's exit code to list's. I confirmed it fails with allowImplicitRoot: false and passes without, so it's a real guard rather than decoration.

Full suite locally: 113 files / 2295 tests passing.

Nice catch on the underlying bug — a store-pointer project silently rendering an empty dashboard while list read it correctly is exactly the kind of wrong that's hard to notice. And no need to apologize for the skill fan-out; that STORE_SELECTION_GUIDANCE string reaching 12 generated files isn't discoverable until it bites you.

@c4patino c4patino left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've taken a look at it, i'm good with the changes that you made!

@alfred-openspec alfred-openspec left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@clay-good
clay-good added this pull request to the merge queue Jul 27, 2026
Merged via the queue into Fission-AI:main with commit 6b3623a Jul 27, 2026
13 checks passed
@c4patino
c4patino deleted the fix/openspec-view-use-store branch July 28, 2026 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants