Fix archive exit code on validation failure#1311
Open
danilopopeye wants to merge 2 commits into
Open
Conversation
In human (non-JSON) mode, openspec archive returned exit code 0 when validation failed and nothing was archived. The three blocking paths in ArchiveCommand.run() printed an error message but returned null silently, leaving process.exitCode at 0. Scripts and CI could not distinguish a blocked archive from a successful one. The --json path was already correct (it throws ArchiveBlockedError, caught by printJsonFailure which sets exitCode = 1). This was an asymmetry between the two modes for the same failure. Set process.exitCode = 1 at the three human-mode abort points before returning null: - delta-spec validation failure - spec rebuild failure - rebuilt-spec validation failure Legitimate user cancellations (selecting no change, declining a confirmation prompt) remain exit 0 by design. Aligns archive with the same exit-code guarantee already approved for apply instructions in Fission-AI#1250. References Fission-AI#498.
Contributor
|
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 (1)
📝 WalkthroughWalkthroughThis change updates ChangesArchive exit code fix
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/core/archive.test.ts (1)
837-921: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing regression test for the third abort path.
This new describe block covers the delta-validation-failure path (archive.ts Line 309) and the spec-rebuild-failure path (archive.ts Line 432), but not the rebuilt-spec validation-failure path (archive.ts Line 456, reached when
buildUpdatedSpecsucceeds butValidator.validateSpecContenton the rebuilt content reports invalid). Since this is one of the three paths the PR explicitly fixes, add a third test to prevent silent regression on that branch.🤖 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/core/archive.test.ts` around lines 837 - 921, Add a third regression test in the existing archive exit-code describe block to cover the rebuilt-spec validation-failure path in archiveCommand.execute. Create a change that reaches buildUpdatedSpec successfully, then fails Validator.validateSpecContent on the rebuilt content, and assert process.exitCode becomes 1, the failure is logged, and no archive is created. Use the existing archiveCommand, Validator.validateSpecContent, and buildUpdatedSpec flow as the locating symbols.
🤖 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.
Nitpick comments:
In `@test/core/archive.test.ts`:
- Around line 837-921: Add a third regression test in the existing archive
exit-code describe block to cover the rebuilt-spec validation-failure path in
archiveCommand.execute. Create a change that reaches buildUpdatedSpec
successfully, then fails Validator.validateSpecContent on the rebuilt content,
and assert process.exitCode becomes 1, the failure is logged, and no archive is
created. Use the existing archiveCommand, Validator.validateSpecContent, and
buildUpdatedSpec flow as the locating symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 03ab610b-2755-4cba-9169-bfcbf32eba27
📒 Files selected for processing (3)
.changeset/fix-archive-exit-code.mdsrc/core/archive.tstest/core/archive.test.ts
Cover the third archive blocking path (spot 3): buildUpdatedSpec succeeds but Validator.validateSpecContent rejects the rebuilt content. Spy on validateSpecContent (same pattern as the existing --no-validate test) to force the rebuilt spec invalid while the rest of the flow runs for real, since this branch is otherwise defensive and nearly unreachable — spot 1 already enforces the same SHALL/MUST/scenario rules on the delta. Asserts process.exitCode === 1, the failure is logged, the main spec is left unchanged, and no archive is created.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
In human (non-
--json) mode,openspec archive <change> -yreturned exit code 0 when validation failed and nothing was archived. The command printed "Validation failed" (or "Aborted. No files were changed.") and exited cleanly, so scripts and CI could not distinguish a blocked archive from a successful one.The
--jsonpath was already correct — it throwsArchiveBlockedError, caught byprintJsonFailure, which setsprocess.exitCode = 1. This was an asymmetry between the two modes for the same failure class.Reproduced in a clean sandbox against 1.5.0:
openspec archive bad-change -ywith a delta spec missing the SHALL/MUST keyword → printed "Validation failed", exited 0,changes/archive/was never created.Changes
Set
process.exitCode = 1at the three human-mode abort points inArchiveCommand.run()beforereturn null:buildUpdatedSpecthrew)This mirrors what
printJsonFailurealready does in the same file for the JSON path. Legitimate user cancellations (no change selected, declining a confirmation prompt) remain exit 0 by design.Related
archivewith the same exit-code guarantee already approved forapplyinstructions in fix(apply): fail with exit 1 when apply instructions are blocked #1250 — same class of silent-exit-0 bug, same fix philosophy.Testing
test/core/archive.test.tscovering both blocking paths reachable in human mode (delta validation failure, spec rebuild failure) plus a success path asserting exit code stays unset.process.exitCodeto prevent leakage between tests.pnpm exec vitest run— 99 files / 1811 tests passing.pnpm run lint— 0 errors.Notes
The third abort path (rebuilt-spec validation failure) got the same one-line fix but is defensively hard to trigger in isolation — the delta-spec validator catches most malformed deltas first. I kept the dedicated test for the two reachable paths rather than contriving validator internals to hit the third.
Summary by CodeRabbit