fix(archive): tell the caller which flag to pass when archive can't ask its questions - #1483
Merged
Merged
Conversation
An AI agent runs the CLI with stdin closed, so every confirmation `openspec archive` asks rejects with @InQuirer's "User force closed the prompt with 0 null" - true, and useless: it names neither the question nor the flag that answers it, so agents abort and guess (#1479). Each confirmation now reports the same guidance JSON mode has always given for that decision point, with a pasteable command. The change picker got the opposite treatment: it swallowed the same failure, printed "No change selected. Aborting." and exited 0, reporting success for a run that archived nothing. It now exits 1 asking for a change name, matching `openspec show` and `openspec validate`. The detection is reactive - a prompt that already failed, at a stdin that is not a terminal - so piped answers, --yes, --json and Ctrl-C at a real terminal are untouched. Closes #1479 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
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:
📝 WalkthroughWalkthrough
ChangesArchive non-interactive behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ArchiveCommand
participant InquirerPrompts
participant InteractiveUtils
participant ArchiveBlockedError
ArchiveCommand->>InquirerPrompts: request archive confirmation or change selection
InquirerPrompts-->>ArchiveCommand: closed prompt error
ArchiveCommand->>InteractiveUtils: classify prompt error
InteractiveUtils-->>ArchiveCommand: non-interactive result
ArchiveCommand->>ArchiveBlockedError: create diagnostic with rerun command
ArchiveBlockedError-->>ArchiveCommand: return blocked archive result
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
alfred-openspec
previously approved these changes
Jul 29, 2026
alfred-openspec
left a comment
alfred-openspec
left a comment
Collaborator
There was a problem hiding this comment.
The reactive prompt handling preserves piped input and real-terminal cancellation while giving non-interactive callers an actionable command, and the missing-name path now exits nonzero. The exact head passed the hosted CI/security matrix plus an isolated build and 130 focused archive, interactive, and CLI tests.
…honor every non-interactive signal Adversarial review of the first commit found four defects in it: - The suggested rerun dropped the flags the caller had passed. For `archive x --skip-specs` it suggested a bare `--yes` rerun, and following it merged deltas into the main specs - the exact thing --skip-specs was passed to prevent. - The change name went into that command unquoted, so a change named `my change` produced an unrunnable paste and one named `a;touch x` produced a paste that runs a second command. - The predicate keyed on stdin.isTTY alone, so a CI runner that allocates a pty still got the raw @InQuirer failure - #1479 unfixed under the very signals `isInteractive()` already treats as authoritative. - A genuine Ctrl-C reaches a process whose stdin is a pipe, and that was reported as "this terminal is not interactive", telling a user who deliberately quit to rerun with --yes. The signal is now `!isInteractive()` with SIGINT excluded, so the terminal proves capability and the signal proves intent. Messages say what happened ("no answer could be read from stdin") rather than asserting a property of the terminal, which was false under MinTTY. Mutation testing found four more gaps in the tests: an unconditional `throw blocked()`, a stripped `withStoreFlag`, and either half of the predicate's `||` all left the suite green. Each now has a test, along with the flag carry-forward, the quoting, the pty-CI case, and the two prompts that had no end-to-end coverage. docs/cli.md documents the behavior without a terminal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Deploying openspec-docs with
|
| Latest commit: |
663205c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://cd5a0d5d.openspec-docs.pages.dev |
| Branch Preview URL: | https://fix-non-interactive-cli-guid.openspec-docs.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/cli-e2e/basic.test.ts`:
- Around line 243-244: Update the filesystem expectations in the affected
end-to-end test to construct every nested path from separate segments via
path.join, including the openspec, changes/specs, and filename components.
Preserve the existing assertions and expected existence values.
In `@test/core/templates/skill-templates-parity.test.ts`:
- Around line 595-599: Update the archive-invocation regex in the parity test to
match both bare “openspec archive” lines and commands with arguments, while
preserving the existing --yes assertion for every matched invocation.
🪄 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: 2fa69a20-a863-4832-bab1-b931d48f51fa
📒 Files selected for processing (10)
.changeset/archive-non-interactive-guidance.mddocs/cli.mdskills/openspec-onboard/SKILL.mdsrc/core/archive.tssrc/core/templates/workflows/onboard.tssrc/utils/interactive.tstest/cli-e2e/basic.test.tstest/core/archive.test.tstest/core/templates/skill-templates-parity.test.tstest/utils/interactive.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/core/templates/workflows/onboard.ts
- .changeset/archive-non-interactive-guidance.md
- test/utils/interactive.test.ts
- src/core/archive.ts
The template guard required a space after `openspec archive`, so a regression to a bare `openspec archive` line - which blocks agents exactly as #1479 describes - would have passed it. Verified by mutation: the widened pattern fails on that edit. Expected filesystem paths in the new e2e assertions are built from path segments, per the repo's testing guideline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…res, and Windows shells A second adversarial pass, scoped to the previous two commits, found three defects in the fix itself: - A change named `--force` was emitted bare, and commander reads it as an option however it is quoted, so the suggested command failed with `unknown option`. Such changes do archive, so the case is reachable: the name now goes behind a `--`, with the store flag kept in front of it where it is still read as an option. - The change-name-required path was the one blocked site left hard-coded, so `archive --skip-specs` with nothing to answer the picker suggested a rerun without `--skip-specs` - the same merge the previous commit set out to prevent. - Quoting was POSIX-only: cmd.exe does not treat `'` as quoting at all, and PowerShell escapes an embedded quote by doubling it, so the emitted command was wrong on Windows. Names now use double quotes, which bash, zsh, PowerShell and cmd.exe all read the same way, and a name containing something with no portable spelling (a quote, backslash, `$`, backtick, newline) names the placeholder rather than emitting a command that could expand. Two tests were pinning less than they claimed. The real-terminal cancellation test had become a duplicate of the piped one, since the SIGINT check short-circuits before the terminal is consulted; it now covers the terminal leg with a non-SIGINT failure, which is the leg nothing else guarded. The template guard iterated two identical strings and could not see an indented invocation; it now sweeps every rendered skill and command template, and both mutations were confirmed to fail it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
alfred-openspec
requested changes
Jul 29, 2026
alfred-openspec
left a comment
alfred-openspec
left a comment
Collaborator
There was a problem hiding this comment.
The new portable quoting path still emits unsafe cmd.exe reruns for valid change names containing percent expansion. For example, a change directory named %USERNAME% is suggested as openspec archive "%USERNAME%" --yes, but cmd.exe expands that even inside double quotes, so the pasted command targets a different name; exclamation marks have the analogous problem under delayed expansion/history expansion. Please fall back to the named placeholder for these characters too and add focused cases. Exact head 9886038 otherwise passed the isolated build and all 161 focused tests.
`%USERNAME%` is a legal change directory name, and cmd.exe expands it inside double quotes, so the suggested rerun `openspec archive "%USERNAME%" --yes` targets a different change than the one that was blocked. `!` has the same problem under cmd.exe's delayed expansion and bash's interactive history expansion. Both characters now fall back to the `<change-name>` placeholder, the same path a `$`/backtick name already took: a rerun the reader has to fill in beats one that silently archives something else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
alfred-openspec
previously approved these changes
Jul 29, 2026
alfred-openspec
left a comment
alfred-openspec
left a comment
Collaborator
There was a problem hiding this comment.
The cross-shell blocker is fixed: percent and exclamation names now fall back to the explicit placeholder instead of emitting commands that cmd.exe or shell history can expand. Exact head 1f05044 passed the hosted CI/security matrix plus an isolated build and all 161 focused archive, interactive, template, and real-CLI tests.
…li-guidance # Conflicts: # test/core/templates/skill-templates-parity.test.ts
Four adversarial reviews of this branch turned up one real defect and two guards that were not actually pinned. The human-mode message for an unanswerable incomplete-task confirmation interpolated the change name raw, and archive resolves a change by stat-ing its directory, so the name is attacker-influenceable. A newline in it added a second, forged `Fix:` line - and because `quoteChangeName` degrades the real fix to `<change-name>` for exactly those names, the forged line was the only pasteable command on screen. Control characters are now collapsed. Also pinned two mutations that passed the whole suite green: dropping `withStoreFlag` from only the dash-leading branch of `rerunCommand`, and dropping the `validate === false` leg of the `--no-validate` test - the one leg Commander actually produces. The --yes parity guard only saw invocations that opened a line, so a `$ ` prompt, a list marker or `openspec --store x archive` slipped past it. It now matches those and names the onboarding floor instead of trusting `total > 0`. Docs and spec catch up: a troubleshooting entry under the message people actually search for, and cli-archive scenarios for the unanswerable-prompt paths, including that Ctrl-C stays a cancellation.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Accepting a global flag between `openspec` and `archive` needed nested quantifiers, and CodeQL was right to call that a ReDoS shape (js/redos, high) even in a test over our own templates. Splitting the line into tokens decides the same question in linear time - a 20k-flag line now costs ~2ms - and reads more plainly than the pattern did. Same classifications as before, plus it correctly ignores `openspec list archive`, where `archive` is an argument rather than the subcommand.
Windows rejects control characters in a filename, so the change directory the test needs cannot be created there - which is also why the hole it covers is POSIX-only. Matches the existing `it.skipIf(process.platform === 'win32')` idiom in the suite.
alfred-openspec
approved these changes
Jul 30, 2026
alfred-openspec
left a comment
alfred-openspec
left a comment
Collaborator
There was a problem hiding this comment.
The CodeQL ReDoS finding is resolved: the nested-quantifier archive matcher was replaced with linear tokenization, and the current alert instance is fixed. Exact head 863f1e7 passed the full hosted CI/security matrix plus an isolated build and 166 focused archive, interactive, template, and real-CLI tests.
clay-good
added a commit
to clay-good/OpenSpec
that referenced
this pull request
Jul 30, 2026
Fission-AI#1483 landed while this branch was in review. Three conflicts: - `archive.ts`: one import line, both sides' imports kept. - `skill-templates-parity.test.ts`: hash constants, resolved by key-union and then regenerated from the merged source, which is the only authority once two branches have edited the same template. - `archive.test.ts`: the trap this repo documents. Both branches appended a DIFFERENT describe block at the same place - `capability retirement (Fission-AI#1302)` here, `non-interactive prompts (Fission-AI#1479)` on main - so taking either side would have dropped 16 or 133 tests with a green suite. Both are kept. The conflict boundary also cut the retirement describe's last two closing braces, which `tsc --noEmit` accepted and only esbuild caught as "Unexpected end of file". Restored by brace-balance against both parents. Verified after: every one of main's 91 archive titles and 19 parity titles is present, Fission-AI#1483's describe still holds its 16 tests, and its own non-interactive repro still behaves as it does on main.
clay-good
added a commit
to clay-good/OpenSpec
that referenced
this pull request
Jul 30, 2026
Both blockers from the last review. The recovery line offered `git checkout HEAD -- <path>` for every retirement, including ones where the file never lived under the directory archive was run from: a selected store, or a symlinked capability directory. Git rejects an absolute path from a different worktree however it is quoted, and an unquoted path containing a space splits when pasted - a real store path reproduced both. Those cases now say where the file was and leave recovery to the reader, rather than handing them a command that cannot work. The ordinary case still gets the command, quoted when the path needs it, via the portable quoting Fission-AI#1483 already established for change names. And `openspec/specs/specs-sync-skill/spec.md` still authorised deletion from the four original conditions, with no mention of the tail-heading veto the CLI gained - so the living spec permitted something the code refuses. It now carries that condition, and a parity test pins it in the generated guidance so the two cannot drift apart again. Both fixes are mutation-verified: restoring the unconditional command fails the escaped-path regression, and rewording the veto out of the template fails the guidance test.
clay-good
added a commit
to clay-good/OpenSpec
that referenced
this pull request
Aug 4, 2026
…I#1484) * fix(archive): retire a capability when a change removes its last requirement A delta whose REMOVED entries cover every requirement rebuilt the main spec empty, and an empty spec fails validation ("Spec must have at least one requirement"), so the archive aborted with no way forward. Pre-deleting the main spec did not help: the delta was then treated as a create and landed on the same empty spec. Archive now treats an emptied capability as retired. It deletes the capability's spec.md and any directory the deletion leaves empty, stopping short of the specs root, and reports the removals in the totals. Nothing is deleted unless this run actually removed a requirement, so a re-applied or already-synced delta still leaves the file alone. Closes Fission-AI#1302 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(archive): decide retirement from the validator and contain the deletion Adversarial review found the original rule unsound. It retired whenever no canonical `### Requirement:` blocks were left, but the validator counts requirements differently: MarkdownParser accepts any `###` heading under `## Requirements`, while the delta block parser indexes only canonical headers and sweeps the rest into the preamble, which survives into the rebuilt spec. A strict-valid spec could therefore be deleted on an archive that previously succeeded. Retirement is now decided by putting the rebuilt spec to the validator and retiring only when its sole error is that it has no requirements, which makes "this spec could not have been written anyway" true by construction. Also fixed: - The directory prune walked string prefixes, but path.resolve does not resolve symlinks and readdir/rmdir both follow them, so a symlinked capability directory let it delete directories outside the repository. Pruning is now bounded by real paths and refuses to descend through a symlink. - A spec that was already requirement-less and lost nothing this run is no longer skipped past validation; it aborts exactly as it did before. - Deletions are deferred until every spec write has succeeded, so a later failure cannot leave a spec already deleted. - Retirement is recorded in `warnings`, naming any other sections the deleted file held, so JSON consumers and humans can both see what went. - Totals carry every applied operation; a rename applied on the way to the removal was being dropped. - bulk-archive guidance, the sync/archive skill specs, and the docs that described archive as never deleting a spec. Closes Fission-AI#1302 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(archive): close the retirement gaps a second review round found Five adversarial reviews, mutation testing and CodeRabbit went at the reworked retirement. The findings, all verified by repro before fixing: - The archive-name collision check ran AFTER the spec merge, so archiving twice in one day deleted the capability's spec and then failed, leaving the change unarchived and the file gone. The destination depends only on the change name, so it is now settled before any spec is written or deleted - which also closes the same, older window for ordinary writes. - `--no-validate` retired too, but the whole safety argument is the validator's verdict, and that path produces none. It now writes the spec exactly as it did before this feature existed, leaving no exception to the claim that nothing previously working changes. - The validator can be talked out of seeing a requirement: a stray `### Requirements` under Purpose captures its section lookup, so a spec still holding a real requirement reported "no requirements" and was deleted. Any `###` heading left under `## Requirements` now vetoes retirement outright - a reader is not fooled by the stray heading even when the parser is. - A dangling symlink made `update.exists` false (`fs.access` follows links, `unlink` does not), skipping the "removed something this run" guard: a run that removed nothing deleted an entry and reported a removal. The no-target case is now an explicit branch that never deletes, instead of an ENOENT probe. - `findOtherSections` reported `## ` headings that were inside HTML comments and listed duplicates; it now masks comments like every other structural scan here and dedupes. The warning also names the `## Purpose`, which the deletion always takes, and the resolved path when a symlink puts the file outside the repo. - A failed `unlink` surfaced a bare errno; it now says what was being attempted and what to do. Tests grew from 19 to 33, killing every surviving mutant the review found: deferral proven against a failing write (not just a failing validation), the warnings payload, the already-gone path's output, multi-level pruning, the `+ path.sep` boundary, a symlinked specs root, two retirements in one archive, and `isRetirableSpec` unit-tested directly - including the two-error shape that proves `every` rather than `some`. Agent guidance, the three living specs and the docs now state the same conditions the CLI applies, so a sync agent cannot delete a spec archive keeps. Closes Fission-AI#1302 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(archive): make the write-failure test platform-neutral and the path note meaningful Windows CI and CodeRabbit each caught one: - `chmod 0o555` is not a write barrier on Windows, so the test that proves deletions are deferred until every write succeeds never failed a write there: the archive completed, the spec was retired, and the assertion blew up. It now puts a directory where the second spec's file belongs, which fails the write on every platform. Verified it still kills the reordering mutant. - The "resolved to" note compared a canonicalized path against a merely resolved one, so any symlinked ancestor - the platform's own /var -> /private/var is enough - decorated an ordinary retirement with a path that says nothing. It now fires only when the spec really lived outside the specs tree, which is the fact the nominal path hides. Both directions are pinned by tests. Closes Fission-AI#1302 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(archive): make the residual-heading veto position-independent A third review round, scoped to the code the earlier rounds never saw. The veto that is supposed to stop a retirement deleting hand-written content only worked when that content sat ABOVE the first requirement. `parts.preamble` is by definition the text before the first `### Requirement:` header; anything after the last one belongs to that block's raw and is discarded with it, so the rebuilt-body scan never saw it. Identical content, different position: one aborted, the other was deleted silently. The veto now reads the original Requirements section - preamble plus every block - so position does not matter. Also: - `realpath` follows a symlinked `spec.md` but `unlink` removes the link, so the warning declared it had deleted a file outside the repo that was still there. The note is now skipped when the target is itself a symlink. - `findHeadings` masked HTML comments before code fences, so an unterminated `<!--` inside a fenced example blanked the rest of the document and truncated the very list of sections the deletion was reporting. Fence first, then comments. - Moving the collision check before the merge widened the window between it and the move, where a claimed destination surfaced as a raw ENOTEMPTY and degraded to `archive_error`. `moveDirectory` now reports that as `archive_target_exists`, the same diagnostic the pre-flight check gives. And a simplification the review asked for: the overlapping `retirable` / `deletes` / `retired` booleans are now one `decideSpecOutcome()` returning 'write' | 'delete' | 'skip'. Behavior is identical - same clauses, same order - but the fourth state that existed only as a comment is now a visible return. Both guards were kept: the review constructed inputs where each is the sole thing preventing a data-losing delete. Two tests the review found wanting are gone or rewritten: one killed no unique mutant, and one assertion straddled two editable message fragments and could have gone vacuously true. Closes Fission-AI#1302 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(archive): canonicalize both negative path assertions CodeRabbit caught that `expect(warnings).not.toContain(shared)` passed vacuously: on macOS the temp root lives under /var, whose realpath is /private/var, so the warning would print a form the assertion never compared against. The sibling assertion on `tempDir` had the same flaw. Both now canonicalize first, and both were confirmed to fail against a mutant - dropping the lstat guard, and forcing the resolved-path note on - which neither did before. Closes Fission-AI#1302 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(archive): move a retired capability's spec into the archive instead of deleting it Retiring a capability was the first case where archiving deleted a file under `openspec/specs/`. Nothing in the repo had ever removed spec content before, so the blast radius of a wrong verdict was a lost file with only the reflog to recover it. The spec now moves instead. It is staged into the change directory, which the archive step renames onto the archive path moments later, so it comes to rest at `<archive>/retired-specs/<capability>/spec.md` beside the proposal and tasks that retired it. `git` records a rename, and bringing a capability back is a `git mv` from the archive. Staged into the change rather than written to the archive path after the move, because the archive path must not exist yet and the ordering is safer: if a later step fails, the spec sits in a change that is still active and a rerun carries it through, versus stranding the live specs tree without a spec it still needs. A symlinked `spec.md` is copied by content and its link removed, rather than moved: relocating the link itself would archive a relative path that no longer resolves from where it landed. A spec already staged by an earlier aborted run is never overwritten - it is the only copy once the live one moves. The retirement verdict, its guards, and the deferral until every write has succeeded are all unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(archive): clean up staging directories when a retirement move fails The staging directories are created before the move, so any failure left an empty `retired-specs/<capability>/` behind. That folder then rode into the archive with the change, where it reads as a retirement that never happened - a spec was supposedly retired here, and there is nothing to show for it. The failure path now prunes back up to the change directory. Only empty directories go, so a capability the same run already staged next to the failing one is untouched, and the guard that refuses to overwrite a staged spec still stops at a non-empty destination. Both cases are covered by tests that fail without the prune: a dangling symlink is the reproducible post-staging failure, since lstat sees a file and the copy then follows the link and finds nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(archive): say "moved" where the retirement path still said "deleted" Three leftovers from the deletion version: the `residualRequirementHeadings` comment, `pruneEmptyDirs`'s `mainSpecsDir` parameter - now a boundary that is the change directory on the cleanup path, not the specs root - and a sentence in writing-specs.md that used "deleted" for the requirement and then again for the file, two lines apart. No behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(archive): roll back a staged copy when the live spec cannot be removed Both non-atomic retirement routes - a symlinked main spec, and the EXDEV/EPERM rename fallback - copy the spec into staging first and remove the original second. A copy that landed before an `unlink` that failed left the spec in TWO places, and the staged one then tripped the "already staged" guard on every rerun. The error told the caller to rerun the archive, and the rerun could never work. Reproduced at the previous head with a symlinked `spec.md` in a read-only capability directory: `copyFile` succeeded, `unlink` returned EACCES, and both copies remained. The failure path now deletes the destination this attempt created, so the capability is left exactly as the attempt found it and the rerun works. The rollback is gated on a flag set only after the destination is proven free, so a spec staged by an EARLIER run is never the thing removed - the overwrite guard still fires ahead of it and rolls nothing back. A partially written copy is cleaned by the same call. The message no longer promises more than it delivers: it reports that the spec is still in place, or names the leftover copy when the rollback itself failed. Regression tests cover both routes and assert the rerun succeeds, not just that the copy is gone. Both fail without the rollback. The cross-device route injects EXDEV, which cannot be provoked inside one temp directory. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(archive): run the rename-fallback rollback case on Windows too The two post-copy rollback cases shared one `skipIf(win32)`, inherited from the symlink case, which needs privileges Windows does not grant by default. The rename-fallback case uses regular files and spies only, and the sibling errno it stands in for - EPERM - is the Windows case, so skipping it there left that route untested on the platform that produces it. Skipping is now per-case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(archive): claim the retirement destination atomically `fs.access` followed by a write is not an ownership claim. Two concurrent retirements both saw the destination free and both set `destIsOurs`; one moved the spec into staging, and the other - equally convinced the file was its own - rolled it back out. The source and the staged copy both ended up gone. Reproduced at the previous head in 36 of 40 iterations. The claim and the content now arrive in one syscall: `copyFile` with `COPYFILE_EXCL` fails with EEXIST rather than overwriting, so exactly one caller can ever own the path. That is also the check that refuses to clobber a spec an earlier aborted run staged, now decided atomically rather than by a separate look beforehand. The losing caller fails two ways, and both used to destroy the winner's file. EEXIST is the obvious one. ENOENT is not: `copyFile` opens the source first, so a loser that arrives after the winner removed the source fails before creating anything - and treating that as "a partial copy of mine" unlinked the winner's file. Neither errno now claims ownership. Fixing only EEXIST left 4 of 40 iterations still losing both copies. Copying rather than renaming is what makes the claim possible: `rename` overwrites silently on every platform, so it cannot tell "I created this" from "I destroyed someone else's". It also crosses filesystems, which retires the EXDEV/EPERM fallback, and reads a symlink's content rather than moving the link - so the two routes collapse into one shape. Regression asserts the invariant over 25 rounds: exactly one caller retires, the spec survives once and intact, and the source is gone. It fails against the old access-then-write shape. Not crash-safe, which is a weaker promise and now documented: a process killed between the copy and the unlink leaves the spec in both places, and the next run refuses rather than guessing which to keep. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(archive): take retirement ownership from an exclusive create, not an errno Claiming the destination with `copyFile(..., COPYFILE_EXCL)` closed the concurrent race but kept reading ownership out of a failure code, and that cannot be made correct however the errnos are partitioned. An errno says what went wrong, not what was created: a source-side EACCES is indistinguishable from a partial copy of our own, so the cleanup deleted a recovery copy an earlier run had staged - the last remaining copy of a spec whose live file could not even be read. Reproduced at the previous head with an unreadable `spec.md` and a pre-existing `retired-specs/legacy/spec.md`: the staged file was destroyed. Ownership now comes from `open(dest, 'wx')`. O_CREAT|O_EXCL returns a handle exactly when it created the file, so the question is answered by the syscall instead of inferred afterwards, and every failure path leaves the flag false. EEXIST remains the refusal that protects an earlier run's copy, now decided by the same operation. Content is written through the claimed handle, as bytes, and the handle is closed before any rollback so Windows can unlink it. The regression uses real mode bits, skipped on Windows and under root: the defect was a source-side errno being read as proof about the destination, and stubbing a JS-level read cannot reproduce it, because the copy it has to fool never went through one. Verified it fails against the errno- inference version. All three findings on this path now hold together: the pre-existing copy survives, 0 of 120 racing iterations lose a spec, and a post-copy unlink failure still rolls back and reruns cleanly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(archive): keep the staged copy when the source is already gone The rollback exists for a copy that landed while the source survived - the two-places state that blocks every rerun. It must not fire once the source is gone: at that point the staged copy holds the only remaining content, and the end state the retirement was reaching for is already reached. An external delete landing between the read and the unlink produced exactly that, and the rollback destroyed the spec outright - `retired: false`, no live file, no staged copy, content gone. `unlink` returning ENOENT is now a success rather than a failure to roll back. Every other errno still throws: the source is still sitting there, and leaving the staged copy beside it is the state that blocks a rerun. Found reviewing the finished path rather than reported - the same class as the three review findings before it, all of them the rollback reaching a copy it should not have. Regression verified against the unconditional unlink. Also corrects a doc line that still credited the copy with claiming the destination; the claim is the exclusive create. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(archive): gate retirement on a declared marker, drop retired-specs/ Reworks Fission-AI#1302 to follow the design that already exists instead of adding one. The move-into-the-archive approach introduced two things OpenSpec did not have: capability retirement as a lifecycle state, and `retired-specs/` as an on-disk convention no schema declares - which a future unarchive command would have to know about. Its whole justification was preserving content that two existing mechanisms already preserve: the archived change carries the delta naming every REMOVED requirement with its Reason and Migration, and git carries the file. The approach even conceded the point by advertising `git mv` as the recovery path. The issue itself proposed neither. It asked for a delete, or an explicit retirement marker. This does both: archive deletes the emptied spec, and only when the change declares `retire_capabilities: true` in its `.openspec.yaml`. `skip_specs` is the precedent. The marker reader is the same function, parameterised by key, so the two can never drift apart on what counts as honorable metadata - a marker in unparseable YAML, or one whose schema does not load, is not a marker in either case. An explicit `false` is not an unhonorable marker, it is simply undeclared. Without the marker nothing changes: the unwritable spec aborts the archive exactly as before, except the abort now names the marker as the way out - and says nothing about it when retiring would not have made the spec writable anyway, so it never sends an author after the wrong fix. Applying REMOVED already deletes requirement content from a main spec, so deleting the spec once nothing is left is that same operation carried to its end. Every guard survives: the validator's verdict, the residual-heading veto, something-removed-this-run, and never under --no-validate. What goes is the exclusive claim, the rollback, the staging directories, and the four data-loss windows they created across four review rounds. Net 307 lines smaller than the move. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: regenerate parity hashes over the merged sync-specs template Fission-AI#1482 and this branch both edit the sync-specs template, so the merged template needs its own hash - neither side's committed value describes it. * docs(archive): correct claims the redesign left false, and bump to minor Review findings, all verified before fixing: - `pruneEmptyDirs`'s doc claimed "two callers, two boundaries", naming the change directory as the second. That was the staging walk from the move design; there is one caller. The boundary stays a parameter, and the comment now says why. - Three comments still described the retirement as moving the file somewhere. It deletes it. - The sync skill told agents the retirement condition includes "no other `###` headings or prose" and then claimed "openspec archive draws exactly these lines". It does not draw the prose line: a main spec with loose prose under `## Requirements` retires and is deleted, and the prose is not named in the warning, which reports `## ` sections only. Verified against the built CLI. The condition now states what the CLI enforces, and the template tells the agent to read that prose back to the user, since the CLI cannot see it for the agent. - `docs/concepts.md`'s `.openspec.yaml` field list omitted the new marker - the one place a user goes to learn what that file may hold. - `docs/cli.md`'s `--no-validate` row did not mention that it disables retirement, though the row two lines down documents retirement. - Bumped patch -> minor. `skip_specs`, the marker this one mirrors, shipped as a minor change in 1.7.0 (Fission-AI#1399); this adds a metadata field and an archive outcome on the same footing. No behavior change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(archive): refuse to retire a spec with a second Requirements section Four review agents ran against this branch. Two data-loss findings, both reproduced before fixing. 1. A spec with a SECOND `## Requirements` section was deleted even though it passed `validate --strict` with zero issues, and the report named only `Purpose`. `extractRequirementsSection` binds to the FIRST `## Requirements`, so everything after it rides through the merge untouched: the residual-heading veto never sees it, `findOtherSections` filters it out by title, and the validator's own section lookup stops there too - which is why a second section holding a `SHALL` with a scenario reads as valid and then died with the file. The earlier round made that veto position-independent WITHIN the section; this is the same evasion one level up. Retirement is now refused outright for such a spec, so the archive aborts as it did before Fission-AI#1302. The abort's marker hint takes the same conjunct, so it never advises a marker that would not have helped. 2. The recovery line promised `git checkout HEAD -- <path>` unconditionally, and the path was wrong twice over. Verified failures: an UNTRACKED spec - the ordinary case, since an earlier `openspec archive` creates the main spec and nobody has committed it yet - is deleted and the printed command errors, so the file is gone for good; under a store-selected root the nominal `openspec/specs/...` path does not exist in the caller's repo; and a symlinked capability directory puts the file somewhere else entirely. The line now names the path the file actually lived at, and is phrased as the condition it really is rather than a promise archive cannot keep. Regressions for both, plus the three fail-closed branches on the deletion authorisation path that no test observed: a marker in unparseable YAML, and a failing unlink. Each verified against a mutation - removing the veto, restoring the unconditional promise, swallowing the unlink error, and honouring a marker in broken YAML each fail their test. Also pins the sync skill's retirement guidance by content rather than by golden hash, since a hash proves only that it matches its source. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(archive): note that retiring a capability strands an in-flight MODIFIED A capability's main spec is the base Fission-AI#1482's scenario-loss check compares a MODIFIED block against. Retire the capability and that check goes silent by design (a missing main spec is the sister-change-in-flight case), so a change that modifies the retired capability keeps validating clean and then refuses to archive with "target spec does not exist". Nothing is lost - there are no scenarios left to drop - but nothing connects the refusal back to the retirement either, so the changeset says it up front. Found by testing this PR against the three that merged into main today. * fix(archive): veto retirement on any heading past the merged section A sixth data-loss defect, from a second round of review agents. Reproduced before fixing: a `validate --strict`-clean spec was deleted with a live SHALL requirement in it, and the report named only "Purpose". The cause is a mask disagreement. `extractRequirementsSection` - the function that decides where the Requirements section ENDS - masks fenced blocks only. `findHeadings`, which both retirement vetoes were built on, masks HTML comments as well. So a multi-line comment holding a `## ` line terminates the section for the merge while being invisible to the scan that had to notice it: everything below became a tail no guard could see. The round-five guard counted `## Requirements` headings, which the same trick skins straight past. The veto is now asked of the tail itself - does anything `###`-shaped sit past the boundary the merge actually chose - read with the fence-only mask, so it answers the question whatever produced that boundary. That subsumes the multiple-Requirements-sections case it replaces and every comment variant. Also from this round: - The recovery command is derived from the path that was unlinked, not rebuilt from the capability id. On a case-insensitive filesystem the id and the real directory differ in case, git is case-sensitive, and the printed command was one git rejects. - An absolute recovery path now says which checkout to run it in - for a selected store, the file is not under the directory archive was run from. - A declared marker refused by the tail veto says why, instead of dropping the author who did what the docs asked back into the bare Fission-AI#1302 abort. - Corrected "draws exactly these four lines" in the sync skill, a claim added two commits ago that was false when written: the CLI checks two more. Both regressions are mutation-verified. Reverting the veto to the narrow multi-section count fails the comment-boundary test. One reported finding was NOT actioned, because its premise does not hold: a residual `###` heading INSIDE the section still counts as a requirement to the validator, so that spec is valid and simply gets written - there is no silent dead end there to explain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(archive): say the marker needs the schema key beside it `.openspec.yaml` requires `schema:`, so a file holding only `retire_capabilities: true` is not honorable metadata and the marker does nothing. The docs and the abort hint both described adding one line, which sends anyone creating that file from scratch into a dead end. The message did explain itself once you were there ("schema: Invalid input: expected string, received undefined"), but it should not need to. Pre-existing shared behavior - `skip_specs` has the same requirement - so this is wording, not a behavior change. * chore: merge main (Fission-AI#1483) and keep both archive test suites Fission-AI#1483 landed while this branch was in review. Three conflicts: - `archive.ts`: one import line, both sides' imports kept. - `skill-templates-parity.test.ts`: hash constants, resolved by key-union and then regenerated from the merged source, which is the only authority once two branches have edited the same template. - `archive.test.ts`: the trap this repo documents. Both branches appended a DIFFERENT describe block at the same place - `capability retirement (Fission-AI#1302)` here, `non-interactive prompts (Fission-AI#1479)` on main - so taking either side would have dropped 16 or 133 tests with a green suite. Both are kept. The conflict boundary also cut the retirement describe's last two closing braces, which `tsc --noEmit` accepted and only esbuild caught as "Unexpected end of file". Restored by brace-balance against both parents. Verified after: every one of main's 91 archive titles and 19 parity titles is present, Fission-AI#1483's describe still holds its 16 tests, and its own non-interactive repro still behaves as it does on main. * fix(archive): only print a recovery command that would actually run Both blockers from the last review. The recovery line offered `git checkout HEAD -- <path>` for every retirement, including ones where the file never lived under the directory archive was run from: a selected store, or a symlinked capability directory. Git rejects an absolute path from a different worktree however it is quoted, and an unquoted path containing a space splits when pasted - a real store path reproduced both. Those cases now say where the file was and leave recovery to the reader, rather than handing them a command that cannot work. The ordinary case still gets the command, quoted when the path needs it, via the portable quoting Fission-AI#1483 already established for change names. And `openspec/specs/specs-sync-skill/spec.md` still authorised deletion from the four original conditions, with no mention of the tail-heading veto the CLI gained - so the living spec permitted something the code refuses. It now carries that condition, and a parity test pins it in the generated guidance so the two cannot drift apart again. Both fixes are mutation-verified: restoring the unconditional command fails the escaped-path regression, and rewording the veto out of the template fails the guidance test. * fix(archive): retire only what the merge can account for Replaces the tail-heading veto with a rule that does not read Markdown at all. Six review rounds each found a different way to dress content so a heading scan would miss it: a second `## Requirements` section, a `##` inside an HTML comment ending the section early, a three-space indent, a setext underline. Every fix was another regex approximating a parser, and every round found the next skin. `extractRequirementsSection` has already split the file into the parts this merge understands. So instead of asking "does anything here look like a requirement" - a question a regex and a renderer answer differently - the guard now asks where content ended up: anything non-blank between the `## Requirements` header and the first requirement, or after the section ends, is content the merge carried through without understanding, and a retirement that would delete the file is refused. There is no second opinion to disagree with the first, because there is no second parse. The in-block heading guard stays, and its comment now says why: a `###` heading that is not a requirement header is absorbed into the block above it, so it never reaches the preamble or the tail. Folding that into the rule above needs a parser that ends a block at any `###` heading, which belongs in the parser. This narrows the feature: a spec carrying an authored section beyond Purpose can no longer be retired automatically. That is deliberate. The abort names the lines that stood in the way, and deleting a file whose contents this merge cannot enumerate is exactly the case a person should decide. Depends on Fission-AI#1490 for indented requirement headers, which are swallowed by the block parser before any of this runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(archive): account for the whole spec, not two slices of it Defect eight, same class as the seven before it. The guard asked where content landed, which was the right question, but it only read two of the five slices `extractRequirementsSection` produces: the preamble and the tail. Content simply moved somewhere nobody looked. Reproduced: a hand-written migration runbook and a table written below a requirement's scenarios live inside that requirement's `raw` - the block runs to the next header the parser RECOGNISES - so removing the requirement deleted them, and the report said "Its section(s) went with it: Purpose". Not silence: a false statement the reader can act on. The same hole covered anything written above the `## Requirements` section. And because the abort hint is gated on the same checks, an unmarked run RECOMMENDED adding the marker that destroys it. The audit now covers the whole file. Expected: the title, the `## Purpose` section, the `## Requirements` header, and inside each block a requirement's own parts - its header, its statement, its scenarios' bullets. Every other non-blank line is reported and refuses the retirement. That folds in the `###`-heading guard, which was a patch on this same leak using the technique the rewrite was meant to abandon. One reported shape is deliberately not a case: prose between `## Purpose` and `## Requirements` IS the Purpose body, since the section runs to the next `##`, and the warning already names Purpose as going with the file. The test says so. Both regressions fail against the two-slice version. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(specs): keep content absorbed into a removed requirement A requirement block's `raw` runs to the next header the parser RECOGNISES, so a heading it does not - one indented by the 0-3 spaces CommonMark allows, or a plain `### Notes` - is absorbed into the requirement above it. Removing that requirement deleted the absorbed content with it. Silently: nothing counted it, so nothing warned, and the spec left behind still validated. Reproducible on main with no marker and no capability retirement involved. Anything from the first `#`/`##`/`###` heading after a removed block's own header is now kept in place. `####` is excluded deliberately - a requirement's `#### Scenario:` headings are its own and go with it. This replaces an earlier attempt on this branch that widened every heading pattern in both parsers to accept indentation. That was wrong twice over. It reclassified content, so a spec that was valid became invalid - commented-out and indented examples started parsing as real requirements, taking `list` from 1 requirement to 3. And it did not even fix the bug: moving the line out of the block only meant the reconstruction dropped it at a different step, since `rebuilt` is assembled from `before + header + kept blocks + after` and anything skipped is simply gone. So nothing is reclassified now. An indented heading is still not a requirement, exactly as before; it just survives its neighbour's removal, which is all this ever needed to do. The repo's own corpus produces byte-identical `list`, `validate --specs --strict` and `validate --changes --strict` output. Four regressions, each mutation-verified: removing the salvage fails the three absorbed-content cases, and counting `####` as a boundary fails the scenario case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(specs): keep notes absorbed into a modified or removed requirement A slow audit of the previous commit found the fix covered one of three paths. A requirement block absorbs anything below it that the parser does not read as a new header - a note indented by the 0-3 spaces CommonMark allows, say - so that content rides inside the block. The previous commit salvaged it when the requirement was REMOVED and missed MODIFIED entirely: that path rebuilds the block from the delta, which never carried the note, so it was dropped exactly as before. Verified against the real CLI: main loses it on both paths. RENAMED was the opposite trap. It rewrites the original block's header line in place, so the note is already there - but it also deletes the original key from the block map, which made the requirement look REMOVED to the salvage and produced a duplicate. Tracking which operation applied is therefore not reliable at this point in the merge, so the salvage now asks the assembled result instead: re-insert a note only when nothing else in the rebuilt section already carries it. That is correct for all three paths by construction. Salvaged content also keeps its position now, next to the requirement it was written beside, rather than being appended at the end of the section. Six regressions, three of them mutation-verified against this logic: never re-inserting fails four, always re-inserting duplicates on rename, and appending at the end loses the position. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(specs): decide salvage by identity, not by matching text Another audit pass, another defect in my own fix. Deciding whether a note survived by searching the rebuilt section for its text is wrong when two requirements carry the same note: the first copy is found, and the second is dropped. Reproduced - two removed requirements each followed by an identical `### Notes`, one note destroyed. Survival is a question about the block, not about text. An untouched block is the same object the parser produced and still carries its note; a replaced one is a different object and does not. The RENAMED path previously blurred that by copying the whole raw, so it now carries only the requirement's own lines and the salvage puts the note back like every other path. With every replacement uniformly lacking the tail, `replacement !== block` decides it exactly, and no text is compared at all. Four properties, each mutation-verified: matching text instead of identity loses the duplicate note, always re-inserting doubles an untouched block's note, letting RENAMED keep the tail doubles it on rename, and counting `####` as a boundary severs a requirement from its scenarios. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(specs): warn when a note absorbed into a requirement will be deleted An adversarial review found the previous approach was worse than the bug. Salvaging the "foreign tail" out of a requirement block relied on a positional rule: everything after the first heading-shaped line is not the requirement's. That is not true. A `# comment` inside a scenario bullet, or a markdown example, matches the same shape - and on MODIFIED the old text was then spliced back in after the new, so the spec asserted both. The validator called the result valid, and re-applying the same delta grew the file every time. Reproduced end to end. It also turned a working archive into a hard abort: preserving an unindented `### Notes` made the rebuilt spec fail validation as a scenario-less requirement, so changes that archived cleanly on main stopped archiving, with an error that never mentioned the note. Measured before choosing: 3 of 742 requirement blocks in this repo contain a heading-shaped line, and the repro shows those are false positives. Trading a rare silent deletion for silent corruption on the most common operation is a bad trade. So the merge is left exactly as it was - byte-identical output, verified against main - and the loss is reported instead. That fixes the part of the bug that actually hurt: it was silent. A wrong warning costs a line of output; acting on a wrong answer rewrites the spec. Eight tests. Dropping the warning fails three; ignoring the fence mask fails one - the fence case the previous version left unpinned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(archive): scope a scenario's bullets, and stop refusing ordinary prose Defect nine, plus the over-refusal it exposed. Every bullet counted as a scenario's own, anywhere in the block. So an operational note bulleted below the last scenario - "IMPORTANT: escrow keys live in the legacy vault" - was deleted with the file, on a spec that passes `validate --strict`, and the report named only "Purpose". A scenario's bullets run unbroken beneath its header; a blank line after them ends the run, and bullets past that point are the author's own note. Measuring the guard against this repo's 36 specs then showed the opposite failure was already there: 7 of them could never be retired, almost entirely because every fenced line inside a requirement was treated as foreign. A code example inside a scenario is that requirement's own content - a `### Requirement:` inside a fence is not a heading to any reader - so fenced lines are now accounted for, as are numbered lists and a statement that opens with inline code. One ambiguity is left deliberately unresolved: a scenario whose bullets are split by a blank line reads exactly like a note bulleted below it, and no line-based rule separates them. Those specs are REFUSED, never deleted. The abort quotes the lines, and the author moves them or removes the file by hand. Refusing costs a message; the alternative costs the file. Two regressions: the bulleted note must refuse, and a requirement using a numbered list, a fenced example and an inline-code statement must still retire. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(archive): a section is not only an ATX heading Defect nine, from a deep adversarial pass, and it is the same species as the eight before it: the guard decided what a section IS by one syntax while a reader recognises three. Once `## Purpose` was seen, every later line in the pre-requirements slice was accepted as its body until the next ATX `##`. But a setext underline turns the line above it into a heading, and raw HTML says so outright - a reader sees a sibling of `## Purpose`, not more of it. So a whole authored section could sit between Purpose and Requirements, pass `validate --specs --strict`, and be deleted with the file while the report said only "Purpose". On main the same archive aborts and loses nothing. Reproduced with a `Data Migration Notes` section underlined with dashes: the capability retired, the notes gone, unnamed. Now refused, with the lines quoted. Two path defects from the same review, one fix: the reported path was rebuilt from the capability id, so on a case-insensitive filesystem it differed in case from the file actually unlinked and git rejected the printed command; and a capability directory symlinked to a sibling deleted one spec while naming another. `retireSpec` now always returns the path it unlinked, and archive reports that. Whether to print a command at all is decided against the REAL repo root, so a symlink that stays inside the repo still gets a working command and only a path that genuinely leaves it falls back to prose. Also pins `!skipValidation` in isolation. The existing --no-validate test passed for the wrong reason - its fixture was blocked by the content guard - so the conjunct itself was unpinned. Four regressions, all mutation-verified. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(archive): close remaining capability retirement gaps * fix(archive): close final transaction safety gaps * fix(archive): close retirement race windows * fix(archive): preserve retirement authorization * fix(archive): verify complete fallback copies * fix(archive): preserve transactional safety Reject structurally ambiguous or symlinked inputs before mutation, serialize archive claims safely, and preserve permissions during verified fallback moves. Keep retired specs as inode-preserving backups until the archive commits, restore them on rollback, and retain any backup changed concurrently instead of deleting user data. * fix(archive): preserve replaced claims on Windows Add a per-claim nonce and verify stable claim contents before unlinking because Windows file IDs may not distinguish a replacement lock entry. * test(archive): respect Windows deferred deletion Skip the POSIX unlink-and-recreate claim simulation on Windows, where deletion of an open file remains pending until the original handle closes. * test(archive): align symlink fixtures with path boundaries --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
3 participants
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.
Status: LGTM — ready to merge. Closes #1479.
What was wrong
An AI agent (or any script) runs the CLI with stdin closed.
openspec archiveasks up to three yes/no questions, so on the first run without--yesthe prompt fails and the command dies with:That message names neither the question that was asked nor the flag that answers it. The reporter's agent aborted, guessed, and only then found
--yes.The change picker had the mirror-image problem:
openspec archivewith no change name swallowed the same failure, printedNo change selected. Aborting.and exited 0 — success for a run that archived nothing.How it was fixed
When a prompt fails and nothing could have answered it, archive reports what it needed plus a pasteable rerun:
User force closed the prompt with 0 nullUpdating 1 spec(s) requires confirmation, and no answer could be read from stdin.Fix: openspec archive add-thing --yes1 incomplete task(s) found for change 'add-thing'…Fix: Complete the tasks or rerun with openspec archive add-thing --yes--no-validateFix: openspec archive add-thing --no-validate --yesNo change selected. Aborting.+ exit 0A change name is required: no answer could be read from stdin.+ exit 1The suggested rerun carries the flags you already passed —
openspec archive x --skip-specssuggestsopenspec archive x --skip-specs --yes, so following the advice can never merge specs you opted out of merging — and shell-quotes a change name that needs it, since archive resolves a change by stat-ing its directory and will happily acceptmy change. Quoting stops where portability does: double quotes are the one form bash, zsh, PowerShell and cmd.exe read alike, but cmd.exe expands%USERNAME%inside them (and!name!under delayed expansion), so a name containing$, a backtick,%or!becomes a<change-name>placeholder instead of a command that would archive something else.The detection is deliberately reactive: it inspects a prompt that already failed, then asks whether anything could have answered it. A pre-emptive "refuse to prompt without a TTY" check would have broken
printf 'y\n' | openspec archive …— including the change picker, whichprintf '\n' |drives today. Beyond the terminal it defers to the existingisInteractive(), soCI,OPEN_SPEC_INTERACTIVE=0and--no-interactivecount even when a runner allocated a pty. SIGINT is excluded: Ctrl-C reaches a piped process too, so the signal proves someone was there and quit.The onboarding walkthrough — the only generated guidance that tells an agent to run
openspec archive— now shows--yes, pinned by a semantic test rather than only a golden hash.Nothing else changes
Verified against the built CLI, not just mocks:
printf 'y\n' | openspec archive add-thingprintf 'n\n' | openspec archive add-thingopenspec archive add-thing --yesy(viaexpect)User force closed the prompt with SIGINT, exit 1 (unchanged)No change selected. Aborting., exit 0 (unchanged)--jsonmodeThe one behavior change: bare
openspec archivewith nothing able to answer now exits 1 instead of 0. It archived nothing either way; exiting 0 said otherwise.openspec showandopenspec validatealready exit non-zero in this situation (by a pre-emptive check rather than this reactive one), so archive now agrees with them on the outcome.Proof
runCLI, which closes the child's stdin — the agent's exact situation: all three confirmations, the change picker, flag carry-forward, and the--storesuffix on a store-rooted change.CIcase, Ctrl-C over a pipe, and a non-prompt error (anEACCES) surfacing as itself instead of being relabelled "rerun with--yes".throw blocked(), strippedwithStoreFlag, and either half of the predicate's||).pnpm lintclean, parity hashes andskills/regenerated by their own scripts.Review round three (post-merge with main)
Five independent reviews — breaking-change audit, shell-quoting attack, regression hunt, mutation testing, conventions/doc sync — ran against the branch after merging
main. Verdict: not breaking. One real defect and two unpinned guards were found and fixed in663205c3:Fix:line. The incomplete-task message interpolated the name raw, and a newline in it printed a second, attacker-chosenFix:line — the only pasteable one, sincequoteChangeNamecorrectly degrades the real fix to<change-name>for exactly those names. Control characters are now collapsed.withStoreFlagfrom only the dash-leading branch ofrerunCommand, and dropping thevalidate === falseleg — the one Commander actually produces — of--no-validate.--yesparity guard was defeated. It only matched invocations that opened a line, so$ openspec archive,- openspec archiveandopenspec --store x archivewere invisible. It now matches those and names the onboarding floor instead of trustingtotal > 0.openspec/specs/cli-archive/spec.mdgained scenarios for the unanswerable-prompt paths (including that Ctrl-C stays a cancellation), anddocs/troubleshooting.mdan entry under the message people actually search for.The injection surface was attacked with 36 adversarial change names executed through real bash and zsh, and commander's
--handling was verified in a harness: no input reaches a shell as anything but the literal name.One open question I could not settle locally (no
pwshavailable), noted for the maintainer: PowerShell parses a bare numeric argument as a number, so a change literally named1.10may reach the CLI as1.1. Names like that became legal in #1435. The fix would be one character — require a leading letter for the unquoted fast path inquoteChangeName, sending everything else down the always-safe quoted branch — but I did not want to ship an unverified change.Notes
src/utils/interactive.ts, beside the existingisInteractive().docs/cli.mdnow documents the no-terminal behavior.@inquirer/promptsand still surface the raw error; and three near-identical prompt-cancellation predicates now exist (shared-output.ts,version-check.ts, and this one) and could be consolidated.Summary by CodeRabbit
Bug Fixes
openspec archivenon-interactive handling by stopping before confirmation, returning structured diagnostics with pasteable rerun guidance (including--yes) while preserving existing flags.1) and avoids treating real cancellations as missing stdin answers.Documentation
openspec archive "<name>" --yesfor agent/CI runs without a terminal.Tests
--yesbehavior.