fix(status): exit gracefully when no changes exist#759
fix(status): exit gracefully when no changes exist#759fsilvaortiz wants to merge 2 commits intoFission-AI:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds graceful handling for Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Client (openspec)
participant Status as statusCommand
participant Shared as getAvailableChanges
participant FS as FileSystem
CLI->>Status: run "openspec status" [no --change]
Status->>Shared: getAvailableChanges(projectRoot)
Shared->>FS: read openspec/changes directory
FS-->>Shared: list of change dirs (empty / non-empty)
alt no changes found
Status->>Status: stop spinner
Status-->>CLI: output text "No active changes..." or JSON {"changes":[],"message":"No active changes."}
Status-->>CLI: exit 0
else changes exist
Status->>Status: proceed to validateChangeExists (existing behavior)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Greptile SummaryThis PR fixes issue #714 by making Key changes:
The implementation is clean, well-tested, and maintains backward compatibility. No issues found. Confidence Score: 5/5
Important Files Changed
Last reviewed commit: eca0298 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/workflow/shared.ts (1)
89-109:⚠️ Potential issue | 🟡 MinorOrphaned JSDoc comment belongs on
validateChangeExists, not abovegetAvailableChanges.The block at lines 89-92 was the original JSDoc for
validateChangeExists. InsertinggetAvailableChanges(with its own JSDoc at lines 93-96) immediately after it leaves the old comment orphaned — TypeScript tooling will silently discard it, andvalidateChangeExistsnow has no JSDoc at all.♻️ Proposed fix
-/** - * Validates that a change exists and returns available changes if not. - * Checks directory existence directly to support scaffolded changes (without proposal.md). - */ -/** +/** * Returns the list of available change directory names under openspec/changes/. * Excludes the archive directory and hidden directories. */ export async function getAvailableChanges(projectRoot: string): Promise<string[]> { ... } +/** + * Validates that a change exists and returns available changes if not. + * Checks directory existence directly to support scaffolded changes (without proposal.md). + */ export async function validateChangeExists(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/workflow/shared.ts` around lines 89 - 109, Move the orphaned JSDoc so it documents the correct function: remove the stray comment block currently sitting above getAvailableChanges and place it immediately above the validateChangeExists function declaration; ensure getAvailableChanges retains its own JSDoc (the comment that currently documents it should remain directly above getAvailableChanges) so both getAvailableChanges and validateChangeExists have appropriate, non-duplicate JSDoc blocks.
🧹 Nitpick comments (1)
src/commands/workflow/status.ts (1)
44-57: DoublegetAvailableChangescall when--changeis omitted and changes exist.When
options.changeis undefined andavailable.length > 0,getAvailableChanges(projectRoot)is called at line 45, then called again insidevalidateChangeExistsat line 57 (which always calls it whenchangeNameis undefined). Tworeaddirround-trips for the same path in the same request.♻️ Proposed fix — pass the already-resolved list to `validateChangeExists`
One approach: short-circuit entirely once you have
available, skipping the secondvalidateChangeExistscall in the no---changebranch.- if (!options.change) { - const available = await getAvailableChanges(projectRoot); - if (available.length === 0) { - spinner.stop(); - if (options.json) { - console.log(JSON.stringify({ changes: [], message: 'No active changes.' }, null, 2)); - return; - } - console.log('No active changes. Create one with: openspec new change <name>'); - return; - } - } - - const changeName = await validateChangeExists(options.change, projectRoot); + let changeName: string; + if (!options.change) { + const available = await getAvailableChanges(projectRoot); + if (available.length === 0) { + spinner.stop(); + if (options.json) { + console.log(JSON.stringify({ changes: [], message: 'No active changes.' }, null, 2)); + return; + } + console.log('No active changes. Create one with: openspec new change <name>'); + return; + } + // Changes exist but --change not provided — replicate the validateChangeExists error path + spinner.stop(); + throw new Error( + `Missing required option --change. Available changes:\n ${available.join('\n ')}` + ); + } else { + changeName = await validateChangeExists(options.change, projectRoot); + }Alternatively, this is low-impact enough that a comment acknowledging the duplicate is sufficient if you prefer to keep
validateChangeExistsas the single source of truth for that error message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/workflow/status.ts` around lines 44 - 57, When options.change is undefined you call getAvailableChanges(projectRoot) and then validateChangeExists(...) will call getAvailableChanges again; avoid the duplicate readdir by extending validateChangeExists to accept an optional precomputed list (e.g., available: string[] | undefined) and use that list if provided instead of calling getAvailableChanges internally, then call validateChangeExists(options.change, projectRoot, available) from status.ts when you already have the available array; update the validateChangeExists signature and its callers accordingly so the second disk lookup is skipped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openspec/changes/graceful-status-no-changes/design.md`:
- Line 37: The risk description in the design text is self-contradictory; update
the sentence referencing getAvailableChanges, validateChangeExists and
statusCommand so it correctly states that the extra filesystem read occurs when
changes do exist (i.e., getAvailableChanges returns results and statusCommand
proceeds to call validateChangeExists), and that when no changes exist
statusCommand returns early and only one read happens; reference
getAvailableChanges, validateChangeExists, and statusCommand in the corrected
sentence for clarity.
---
Outside diff comments:
In `@src/commands/workflow/shared.ts`:
- Around line 89-109: Move the orphaned JSDoc so it documents the correct
function: remove the stray comment block currently sitting above
getAvailableChanges and place it immediately above the validateChangeExists
function declaration; ensure getAvailableChanges retains its own JSDoc (the
comment that currently documents it should remain directly above
getAvailableChanges) so both getAvailableChanges and validateChangeExists have
appropriate, non-duplicate JSDoc blocks.
---
Nitpick comments:
In `@src/commands/workflow/status.ts`:
- Around line 44-57: When options.change is undefined you call
getAvailableChanges(projectRoot) and then validateChangeExists(...) will call
getAvailableChanges again; avoid the duplicate readdir by extending
validateChangeExists to accept an optional precomputed list (e.g., available:
string[] | undefined) and use that list if provided instead of calling
getAvailableChanges internally, then call validateChangeExists(options.change,
projectRoot, available) from status.ts when you already have the available
array; update the validateChangeExists signature and its callers accordingly so
the second disk lookup is skipped.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changeset/graceful-status-no-changes.mdopenspec/changes/graceful-status-no-changes/.openspec.yamlopenspec/changes/graceful-status-no-changes/design.mdopenspec/changes/graceful-status-no-changes/proposal.mdopenspec/changes/graceful-status-no-changes/specs/graceful-status-empty/spec.mdopenspec/changes/graceful-status-no-changes/tasks.mdsrc/commands/workflow/shared.tssrc/commands/workflow/status.tstest/commands/artifact-workflow.test.ts
Extract `getAvailableChanges` as a public function from `validateChangeExists` and use it in `statusCommand` to detect the no-changes case early. Returns a friendly message (text and JSON modes) with exit code 0 instead of a fatal error. Generated with Claude Code using claude-opus-4-6. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
fsilvaortiz
force-pushed
the
fix/714-graceful-status-no-changes
branch
from
eca0298 to
738d6a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
openspec/changes/graceful-status-no-changes/design.md (1)
37-37:⚠️ Potential issue | 🟡 MinorSelf-contradictory risk/mitigation sentence — previous review comment not yet addressed.
The risk clause states the extra filesystem read occurs "when no changes exist", while the mitigation in the same sentence says "the extra read only happens when changes do exist." These two halves directly contradict each other and are still present in the current code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/changes/graceful-status-no-changes/design.md` at line 37, The sentence describing the risk/mitigation is self-contradictory: clarify whether the extra filesystem read happens when no changes exist or only when changes do exist by updating the sentence in design.md; reference the behavior of getAvailableChanges, validateChangeExists and statusCommand to state precisely that statusCommand returns early when no --change is provided and no changes exist (avoiding validateChangeExists and the second read), so the extra read only occurs when changes exist (because validateChangeExists will call getAvailableChanges again); rewrite the line to remove the contradiction and make the condition for the extra read explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openspec/changes/graceful-status-no-changes/proposal.md`:
- Around line 17-19: Update the "Modified Capabilities" note to mention that
while behavior is unchanged, the internal implementation of validateChangeExists
was refactored to delegate to the newly exported getAvailableChanges; explicitly
reference statusCommand (unchanged), validateChangeExists (refactored), and
getAvailableChanges so readers know the function was re-implemented not left
untouched.
In `@src/commands/workflow/shared.ts`:
- Around line 100-102: The catch in getAvailableChanges currently swallows all
errors and returns []; change it to only suppress ENOENT (directory not present)
and re-throw any other errors so permission or IO errors surface: inside
getAvailableChanges catch the error, if (err.code === 'ENOENT') return [];
otherwise throw err (or rethrow) so callers like statusCommand see real
filesystem errors.
---
Duplicate comments:
In `@openspec/changes/graceful-status-no-changes/design.md`:
- Line 37: The sentence describing the risk/mitigation is self-contradictory:
clarify whether the extra filesystem read happens when no changes exist or only
when changes do exist by updating the sentence in design.md; reference the
behavior of getAvailableChanges, validateChangeExists and statusCommand to state
precisely that statusCommand returns early when no --change is provided and no
changes exist (avoiding validateChangeExists and the second read), so the extra
read only occurs when changes exist (because validateChangeExists will call
getAvailableChanges again); rewrite the line to remove the contradiction and
make the condition for the extra read explicit.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changeset/graceful-status-no-changes.mdopenspec/changes/graceful-status-no-changes/.openspec.yamlopenspec/changes/graceful-status-no-changes/design.mdopenspec/changes/graceful-status-no-changes/proposal.mdopenspec/changes/graceful-status-no-changes/specs/graceful-status-empty/spec.mdopenspec/changes/graceful-status-no-changes/tasks.mdsrc/commands/workflow/shared.tssrc/commands/workflow/status.tstest/commands/artifact-workflow.test.ts
✅ Files skipped from review due to trivial changes (1)
- openspec/changes/graceful-status-no-changes/.openspec.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- src/commands/workflow/status.ts
- openspec/changes/graceful-status-no-changes/tasks.md
- test/commands/artifact-workflow.test.ts
- .changeset/graceful-status-no-changes.md
| } catch { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
Silent catch {} masks real filesystem errors (e.g., EACCES).
The catch block absorbs every error, including permission-denied and corrupted-path cases. When the openspec/changes directory exists but is unreadable, getAvailableChanges silently returns [], causing statusCommand to print No active changes. instead of surfacing the real error — a misleading signal.
Consider narrowing the catch to only ENOENT (directory not yet created), or at minimum re-throw unexpected errors:
🛡️ Proposed fix
- } catch {
- return [];
- }
+ } catch (err: unknown) {
+ const code = (err as NodeJS.ErrnoException).code;
+ if (code === 'ENOENT') return [];
+ throw err;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/workflow/shared.ts` around lines 100 - 102, The catch in
getAvailableChanges currently swallows all errors and returns []; change it to
only suppress ENOENT (directory not present) and re-throw any other errors so
permission or IO errors surface: inside getAvailableChanges catch the error, if
(err.code === 'ENOENT') return []; otherwise throw err (or rethrow) so callers
like statusCommand see real filesystem errors.
There was a problem hiding this comment.
This catch {} block is pre-existing behavior — it was the private closure inside validateChangeExists before this PR. We only extracted it as a public function without changing its internals. Narrowing the catch to ENOENT is a valid improvement but outside the scope of this fix. Happy to open a separate issue/PR for it if the maintainers agree.
There was a problem hiding this comment.
@fsilvaortiz, thanks for the clarification! You're absolutely right — keeping behavioral changes separate from refactoring/extraction work is good practice for scope management and makes PRs easier to review.
Since you've confirmed the improvement is valid, would you like me to create a follow-up issue to track narrowing the catch block to only suppress ENOENT errors? I can draft one that references this discussion and includes the suggested fix for visibility.
Address CodeRabbit review feedback: - Fix contradictory risk description in design.md (double-read happens when changes exist, not when they don't) - Clarify in proposal.md that validateChangeExists was internally refactored to delegate to getAvailableChanges Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
openspec statusnow exits gracefully (code 0) when no changes exist, instead of throwing a fatal errorgetAvailableChangesas a public function fromvalidateChangeExistsinshared.tsstatusCommandchecks for available changes before callingvalidateChangeExists, avoiding string-coupled error matchingapply,show,instructions) are unaffected —validateChangeExistsremains strictChanges
src/commands/workflow/shared.tsgetAvailableChangesas public exportsrc/commands/workflow/status.tstest/commands/artifact-workflow.test.ts.changeset/graceful-status-no-changes.mdopenspec/changes/graceful-status-no-changes/Test plan
openspec statuswith no changes → friendly message, exit 0openspec status --jsonwith no changes → valid JSON, exit 0openspec statuswith changes but no--change→ error listing changes (preserved)openspec status --change non-existent→ error "not found" (preserved)init→status→new change→status --changeworks end-to-end🤖 Generated with Claude Code using claude-opus-4-6.
Summary by CodeRabbit
Bug Fixes
Tests