fix(archive): make the scenario-drift check fence-aware, plus release-audit follow-ups - #1475
Merged
Conversation
parseScenarioBlocks matched #### Scenario: headers on raw lines while the validator's countScenarios masks fenced code blocks (#1151). The drift check (#1391) inherited the raw scan, so a fenced scenario example in the current spec aborted an archive that validate had passed, and a fenced name in the MODIFIED block counted as keeping a scenario the block had actually dropped. Build the shared code-fence mask and skip masked lines in both the header scan and the block-end scan. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The overall request budget was armed inside the first send() and its callback closed over that hop's request. After a redirect the timer destroyed the already-dead first request, so a redirect target that trickled bytes kept resetting its idle timeout and held the socket open until the body-size cap. Track the in-flight request and have the budget timer destroy whichever one is open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he 1.7.0 notes 18 feat/fix commits merged since v1.6.0 without a changeset, so the pending Version Packages PR would have released them silently: five tool integrations (ZCode, Hermes, CodeArts, Kimi Code rename, Codex skills-only), skills.sh distribution, symlinked schema dirs, nested spec discovery, drift multiplicity, checkbox markers, Windows welcome input, npx avoidance, doctor store drift, local dates, missing-core-workflows warning, store-aware main specs, open-questions guidance, and spec content guidance. Plus changesets for this branch's two fixes. 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:
📝 WalkthroughWalkthroughThe PR hardens archive scenario handling, parsing, validation, version checks, CLI/UI behavior, command generation, completion paths, and release metadata for ChangesOpenSpec behavior and release updates
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
The gemini adapter interpolated the description into a TOML basic string and the body into a multiline basic string with no escaping. Every current template value happens to be safe; the first description with a double quote or backslash would silently produce invalid TOML for all Gemini command files. Escape both contexts (#1447 fixed the same class for the YAML adapters but scoped itself to YAML). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three follow-ups from the release audit: - A path segment literally named volta (a user or project directory) classified the install as volta-managed and swallowed the upgrade offer. The undotted spelling now requires volta's own tools/image layout, matching how pnpm and yarn already demand corroboration. - The Windows npm-ownership fallback checked that the npm prefix exists, which is true of any X\node_modules\pkg tree, hand-copied ones included. Corroborate with the openspec.cmd shim npm actually writes. - A https registry redirecting to plain http was followed; a MITM on that reply controls the newer-version answer. Refuse the downgrade. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… description zcode was registered but missing from the adapters barrel (its test imported the module directly), and the completion registry still carried the pre-#1062 description for the instructions command. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A BOM-prefixed delta spec (Windows editors, PowerShell Out-File) failed validate and archive with 'No delta sections found' because the first line never matched '## ADDED Requirements'. Strip the BOM in both normalizers, the same way tool detection already does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A 300-character change name surfaced two raw ENAMETOOLONG errno dumps from stat and mkdir. Bound the name at 200 characters in validateChangeName so the failure is a normal validation error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two asymmetries left over from the #1376/#1386/#1437 no-op work: - MODIFIED counted every delta as applied even when the block was byte-equal to the main spec, so a fully early-synced change rewrote the file (normalization churn), printed '~ N modified', and reported specsUpdated: true where its ADDED/REMOVED/RENAMED twins print 'Specs already in sync; no files changed.' Count only real replacements. - RENAMED's already-synced skip (source gone, target present) had no near-miss guard: a case/whitespace variant of the source still in the spec means a typo'd header, and REMOVED already hard-aborts on that signal. Apply the same guard, excluding the target itself so a case-only rename still no-ops. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The delta-validation loop swallowed every error as 'if no specs dir, treat as no deltas', so an EACCES capability folder produced the misleading 'Change must have at least one delta' while archive let the same error propagate. Tolerate only ENOENT and ENOTDIR (a stray specs file); anything else stays loud, matching discoverSpecFiles' documented fail-loud contract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Under delivery: commands, update removed the skills of adapterless skills-only tools (Hermes, Kimi Code, Vibe, CodeArts, ForgeCode) without a word — leaving zero OpenSpec artifacts while the tool's detection dir kept re-suggesting an init that would also generate nothing. Print the same per-tool configuration correction init already prints, pointing at 'openspec config set delivery both'. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The installer used a set $ZSH only as an is-installed signal and then wrote to ~/.oh-my-zsh regardless, so a custom OMZ location got a freshly created ~/.oh-my-zsh tree that no shell ever loads — and isInstalled/uninstall looked in the same wrong place. Route every path through the $ZSH/$ZSH_CUSTOM-aware helpers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The static branch printed 'Press Enter to select tools...' and returned immediately, so the Enter landed in the tool picker and submitted the pre-selected set sight-unseen. #1462 routed reduced-motion, OPENSPEC_NO_ANIMATION, --no-animation, NO_COLOR, and narrow-terminal users onto this path. Wait in a TTY; drop the prompt line when there is no TTY to wait on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Only missing-gh and unauthenticated flows showed the formatted feedback and pre-filled submission URL; issues-disabled, network, or rate-limit failures printed gh's stderr and discarded the path to submit what the user had already typed. Route those through the same manual fallback, preserving gh's exit code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/command-generation/adapters/gemini.ts`:
- Around line 35-36: Update escapeTomlMultilineBasicString to escape
TOML-invalid C0 control characters using TOML_CONTROL_CHARS or the appropriate
TOML basic-string variant, while preserving intentional tabs, newlines, and
carriage returns. Add regression coverage for values containing NUL and vertical
tab to ensure generated multiline strings remain valid.
In `@src/core/completions/installers/zsh-installer.ts`:
- Around line 57-58: Update generateOhMyZshFpathGuidance() to derive its fpath
check from completionsDir, so custom ohMyZshCustomDir() installations reference
the actual <custom>/completions path instead of the literal default path. Add
cross-platform regression coverage with a custom ZSH_CUSTOM value, using
path.join(...) for filesystem expectations.
In `@src/core/update.ts`:
- Around line 292-298: Update the skills removal flow in the update logic around
shouldGenerateCommandsForTool and the later skillsInvocableCommandSkips handling
so tools recorded in zeroArtifactTools for commands-only delivery are not also
added to the generic “Commands skipped ... (uses skills)” message. Preserve the
existing skip reporting for tools whose skills remain, and add or update a test
asserting the contradictory message is absent.
In `@src/core/version-check.ts`:
- Around line 452-455: Update the undotted Volta detection condition in the
version-check logic to require both `tools` and `image` segments independently,
while preserving the existing `.volta` check. Add regression coverage for paths
containing `volta` with only `tools` and only `image`, ensuring neither is
classified as Volta.
🪄 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: 16a6f9bd-ff3f-4e32-81e8-586a6d6e608d
📒 Files selected for processing (31)
.changeset/bom-delta-parsing.md.changeset/change-name-length.md.changeset/feedback-manual-fallback.md.changeset/gemini-toml-escaping.md.changeset/modified-noop-counting.md.changeset/renamed-near-miss-guard.md.changeset/static-welcome-waits.md.changeset/update-check-detection-hardening.md.changeset/update-zero-artifact-notice.md.changeset/validator-unreadable-specs.md.changeset/zsh-completions-custom-omz.mdsrc/commands/feedback.tssrc/core/command-generation/adapters/gemini.tssrc/core/command-generation/adapters/index.tssrc/core/completions/command-registry.tssrc/core/completions/installers/zsh-installer.tssrc/core/parsers/markdown-parser.tssrc/core/parsers/requirement-blocks.tssrc/core/specs-apply.tssrc/core/update.tssrc/core/validation/validator.tssrc/core/version-check.tssrc/ui/welcome-screen.tssrc/utils/change-utils.tstest/core/archive.test.tstest/core/command-generation/adapters.test.tstest/core/parsers/requirement-blocks.test.tstest/core/update.test.tstest/core/version-check.test.tstest/ui/welcome-screen.test.tstest/utils/change-utils.test.ts
Comment on lines
+292
to
+298
| // A tool with no command adapter now has zero OpenSpec artifacts; | ||
| // say so like init does, rather than deleting its skills silently | ||
| // and letting tool detection re-suggest an init that would also | ||
| // generate nothing under this delivery setting. | ||
| if (!shouldGenerateCommandsForTool(tool.value, delivery)) { | ||
| zeroArtifactTools.push(tool.name); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Suppress the contradictory “uses skills” message.
For a skills-only tool in commands delivery, these lines remove its skills and record it as zero-artifact. The later skillsInvocableCommandSkips path at Line [318] still reports “Commands skipped ... (uses skills),” so users receive both contradictory messages. Avoid adding that generic skip entry when commands-only delivery removes the skills, and cover the absence of the misleading message in the test.
Also applies to: 359-368
🤖 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 `@src/core/update.ts` around lines 292 - 298, Update the skills removal flow in
the update logic around shouldGenerateCommandsForTool and the later
skillsInvocableCommandSkips handling so tools recorded in zeroArtifactTools for
commands-only delivery are not also added to the generic “Commands skipped ...
(uses skills)” message. Preserve the existing skip reporting for tools whose
skills remain, and add or update a test asserting the contradictory message is
absent.
The ownership corroboration now checks for the openspec.cmd shim npm writes beside node_modules; the Homebrew-prefix fixture built the layout without it, so the test failed on windows-pwsh. Write the shim in the fixture and pin the inverse: the same shape with nothing npm wrote (a hand-copied portable tree) is not an npm install. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… spelling
The corroboration used has('tools', 'image'), which is some() — volta
AND (tools OR image) — so /srv/volta/tools/apps/... still classified as
a Volta install and swallowed the upgrade offer. Require both segments,
matching the real %LOCALAPPDATA%\Volta\tools\image layout.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
escapeTomlMultilineBasicString handled backslashes and quote-triples but not the C0 controls that are as invalid in a multiline basic string as in a single-line one. Reuse TOML_CONTROL_CHARS, applied last so the escapes it introduces are not re-doubled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fpath verification advice still grepped the literal custom/completions, which a relocated $ZSH_CUSTOM need never contain — grep the actual directory instead. The installer tests cleared only $ZSH, so on a machine exporting $ZSH_CUSTOM they would have written into (and deleted from) the developer's real OMZ custom dir — the same leakage class #1400 fixed for $ZSH. Clear/restore both, and pin the custom-location paths with two new tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Hermes is skills-only (no command adapter), and zcode's namespaced commands register /opsx:<id>, not /opsx-* — the release notes must not reintroduce the invocation-spelling confusion #1471 removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The new reportGhFailure output (formatted feedback + pre-filled URL) had no coverage; the network-failure test now asserts it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/commands/feedback.test.ts (1)
347-356: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the typed feedback is preserved.
These checks only prove that generic fallback text and a URL are printed. A regression that drops the submitted title/body would still pass. Assert the formatted title (for example,
Title: Feedback: Test) and the encoded title in the manual URL; also prefer the fullhttps://github.com/...prefix.This directly covers the PR objective that feedback failures retain manual submission data.
🤖 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/commands/feedback.test.ts` around lines 347 - 356, Strengthen the assertions in the manual-submission fallback test so they verify the submitted feedback is preserved, not just that generic instructions appear. Alongside the existing fallback check, assert the formatted title such as “Title: Feedback: Test” and the encoded title in the manual URL, using the full “https://github.com/Fission-AI/OpenSpec/issues/new” prefix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/completions/installers/zsh-installer.ts`:
- Around line 346-348: Update the fpath check in the zsh installer to treat
completionsDir as a literal value: emit fpath entries via printf '%s\n' with
"$fpath", use grep -F, and shell-escape the completionsDir before embedding it
in the displayed command so quotes, backslashes, and regex characters remain
safe.
---
Nitpick comments:
In `@test/commands/feedback.test.ts`:
- Around line 347-356: Strengthen the assertions in the manual-submission
fallback test so they verify the submitted feedback is preserved, not just that
generic instructions appear. Alongside the existing fallback check, assert the
formatted title such as “Title: Feedback: Test” and the encoded title in the
manual URL, using the full “https://github.com/Fission-AI/OpenSpec/issues/new”
prefix.
🪄 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: fe3dc428-381b-450a-8c1f-7f23a461a68f
📒 Files selected for processing (9)
.changeset/add-hermes-tool.md.changeset/add-zcode-tool.mdsrc/core/command-generation/adapters/gemini.tssrc/core/completions/installers/zsh-installer.tssrc/core/version-check.tstest/commands/feedback.test.tstest/core/command-generation/adapters.test.tstest/core/completions/installers/zsh-installer.test.tstest/core/version-check.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- .changeset/add-zcode-tool.md
- .changeset/add-hermes-tool.md
- src/core/command-generation/adapters/gemini.ts
- test/core/version-check.test.ts
- src/core/version-check.ts
alfred-openspec
requested changes
Jul 28, 2026
alfred-openspec
left a comment
alfred-openspec
left a comment
Collaborator
There was a problem hiding this comment.
The multiline TOML escaper still emits a bare carriage return: geminiAdapter.formatFile({ ...content, body: "a\rb" }) is rejected by Python 3.13 tomllib with Illegal character '\r'. Please normalize CRLF and escape a lone CR, then add a parser-backed regression so the new “control characters cannot produce invalid TOML” guarantee holds.
…idance The verification advice interpolated the completions dir into grep "<dir>" where regex metacharacters make the check unreliable and quotes could break the displayed command. Print one fpath entry per line and match with grep -F on a shell-quoted literal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
alfred-openspec
requested changes
Jul 28, 2026
alfred-openspec
left a comment
alfred-openspec
left a comment
Collaborator
There was a problem hiding this comment.
Rechecked exact head 3de75e0: the zsh follow-up is sound, but the previously requested Gemini fix is not in this head and the same a\rb output still fails Python 3.13 tomllib. Keeping this changes-requested until CRLF is normalized, a lone CR is escaped, and parser-backed regression coverage is added.
A lone CR is illegal in a multiline basic string — Python 3.13 tomllib rejects the file — and the control-char pass deliberately skipped it on the assumption it only appears as CRLF. Normalize CRLF to LF and escape any remaining CR as \r. The escaping guarantee is now parser-backed: smol-toml (new devDependency) round-trips every hostile body in the regression matrix (lone CR, CRLF, CR before a quote run, NUL/VT/FF, trailing backslash, four- and five-quote runs), and the same outputs were verified against Python tomllib. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The lockfile changed, so the fixed-output derivation hash moved; value taken from the CI mismatch report. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
alfred-openspec
approved these changes
Jul 28, 2026
alfred-openspec
left a comment
alfred-openspec
left a comment
Collaborator
There was a problem hiding this comment.
Verified the requested Gemini fix at exact head 6049821: CRLF is normalized, lone CR is escaped, and parser-backed hostile-body coverage round-trips correctly. Local build, lint, 1,121 focused tests, an additional 185-document Python tomllib sweep, and the hosted CI/security/Nix matrix all pass.
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
2 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 — pre-1.7.0 audit follow-ups from three full audit passes; every fix has a failing-then-passing test or a direct E2E repro. Full suite: 3,378 passing; CI green on macOS/Linux/Windows.
Three rounds over v1.6.0..main (88 commits): (1) clean-build + full suite, E2E, security review, per-commit review of everything since 2026-07-23; (2) adversarial re-review of parser/archive/validation and stores/init/update/completions plus E2E torture tests; (3) an adversarial review of THIS PR's own diff, a coverage-gap sweep (release mechanics, all 78 changesets parsed, docs-vs-CLI accuracy, dep-major compatibility, workflows), and triage of CodeRabbit's findings. No blockers or majors exist on main; this PR fixes everything real that any round found.
What was wrong / how it was fixed (one commit per fix; changesets included)
Round one
parseScenarioBlocksdidn't mask code fences while validate'scountScenariosdoes: a fenced#### Scenario:example false-aborted archives, and a fenced name masked a genuinely dropped scenario. Now uses the shared fence mask.voltastole the upgrade hint (now requires volta's fulltools+imagelayout); the Windows npm-ownership check was trivially true (now corroborates theopenspec.cmdshim); https→http registry redirect downgrades are refused.zcodeAdapterbarrel export; staleinstructionscompletion description.Round two
7. BOM broke delta parsing —
## ADDED Requirementsreported "No delta sections found" (Windows editors,Out-File). Both normalizers now strip the BOM.8. MODIFIED never no-op-aware — a fully early-synced change with a MODIFIED delta rewrote the file and claimed
~ N modifiedwhere its ADDED/REMOVED/RENAMED twins print "Specs already in sync". Now counts only real replacements.9. RENAMED near-miss guard — an already-synced rename silently skipped even when a case variant of the source (a typo) still existed; now aborts like REMOVED does, while case-only renames still no-op.
10. Validator masked unreadable specs dirs as "no deltas found"; only ENOENT/ENOTDIR are tolerated now.
11. Silent skills wipe —
delivery: commandsdeleted the skills of adapterless tools (Hermes, Kimi Code, Vibe, CodeArts, ForgeCode) with no message, then tool detection re-suggested an init that would also generate nothing. Update now prints init's configuration correction. (Init's generate-nothing-plus-correction design is deliberate and tested; only update's silence was the bug.)12. zsh completions ignored
$ZSH— custom OMZ locations got a~/.oh-my-zshtree nothing loads;$ZSH/$ZSH_CUSTOMare honored everywhere, the fpath advice greps the real directory, and the installer tests now isolate both variables (the #1400 leakage class).13. Static welcome screen printed "Press Enter to select tools..." and returned immediately, letting the Enter submit the tool picker sight-unseen (#1462 widened exposure). It now waits in a TTY and drops the line otherwise.
14. Feedback discarded text on issues-disabled/network/rate-limit gh failures; the manual fallback (formatted text + pre-filled URL) now shows on every gh failure, with test coverage.
15. ENAMETOOLONG errno dump for 300-char change names → clean "too long (200 characters max)" validation error.
Round three (review of this PR's own diff + CodeRabbit triage)
16. Volta corroboration was OR where the comment said AND (
/srv/volta/tools/...still misclassified) — fixed with both-segments requirement + test.17. Gemini multiline prompts didn't escape C0 control characters — fixed + test.
18. Hermes/zcode changeset texts misdescribed shipped behavior (hermes is skills-only; zcode registers
/opsx:*, not/opsx-*) — corrected.19. CodeRabbit's "contradictory update messages" finding was verified false (the two message lists are capability-disjoint) — no change.
Replication / proof
tomllibparses the generated TOML; BOM change validates;/srv/volta/tools/...now → npm).Notes / follow-ups deliberately not in this PR (design calls or pre-existing behavior too risky right before a release)
#### Scenario:) is narrower than the validator's (####= any h4); aligning changes abort behavior — own PR.## ADDED Requirementssections silently keep only the last — own PR.store doctorexits 0 on a broken store; archive Purpose spacing/alphabetical ordering; config.yaml warn-vs-silent inconsistency; pwsh-on-POSIX detection; uncancelable DNS lookup can outlive the update-check budget promise; bare/og/docswebsite route asymmetry.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
User Experience