Skip to content

fix(archive): make scenario-drift check multiplicity-aware (#1246) - #1391

Merged
clay-good merged 1 commit into
Fission-AI:mainfrom
vishnujayvel:fix/archive-scenario-multiplicity-1246
Jul 20, 2026
Merged

fix(archive): make scenario-drift check multiplicity-aware (#1246)#1391
clay-good merged 1 commit into
Fission-AI:mainfrom
vishnujayvel:fix/archive-scenario-multiplicity-1246

Conversation

@vishnujayvel

@vishnujayvel vishnujayvel commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes a residual silent-drop path in the #1246 scenario-drift gate.

PR #1252 added findMissingCurrentScenarios so a stale MODIFIED block
cannot overwrite the canonical requirement when it omits scenarios that
already exist. That gate stored incoming scenario names in a Set and
filtered by membership. When the current requirement has N scenarios
sharing one name and the MODIFIED block keeps M < N, every current
instance still "matches," missing is empty, and archive still succeeds —
dropping the extras with no abort.

This change makes the check multiplicity-aware: count occurrences per
scenario name; a current instance is missing when remaining incoming count
for that name is zero. Error message shape is unchanged.

Credits

Related

Changes

  • src/core/specs-apply.ts: findMissingCurrentScenarios uses a
    remaining-count Map instead of a name Set.
  • test/core/archive.test.ts: regression — current has two
    #### Scenario: Validate blocks; MODIFIED keeps one → archive aborts,
    main spec untouched, change not archived.

Verification

  • Pre-fix repro on unmodified main: buildUpdatedSpec succeeded with
    Scenario: Validate count 1 and no malformed body (silent drop).
  • pnpm exec vitest run test/core/archive.test.ts — 40/40 passed
  • pnpm run lint — 0 errors; 1 pre-existing warning in
    src/core/references.ts (unused eslint-disable; present on
    origin/main)
  • pnpm run build — passed
  • Revert-proof: restore Set-based findMissingCurrentScenarios, keep new
    test → FAIL (expected 1 to be 2 — silent drop); restore fix → PASS
  • pnpm test — 2014 passed; 13 failed in e2e/config/store/artifact
    files. Same 13 failures reproduce on unmodified origin/main
    (NO_COLOR/FORCE_COLOR chalk + stderr noise). Labeled pre-existing;
    not introduced by this change.

Test plan

  • CI green on this branch
  • Review residual path: duplicate scenario names within one requirement,
    MODIFIED retains fewer copies → abort with
    scenario(s) not present in the modified block: "…"
  • Existing distinct-name drift case from fix archive scenario drift for #1246 #1252 still aborts as before

Summary by CodeRabbit

  • Bug Fixes

    • Improved spec validation when multiple scenarios share the same name.
    • Archive operations now correctly abort when a modified block omits duplicate scenario occurrences.
    • Prevented partial changes, incorrect archiving, and unintended file updates in these cases.
  • Tests

    • Added regression coverage for duplicate-named scenarios during archive operations.

…I#1246)

findMissingCurrentScenarios stored incoming scenario names in a Set, so
when the current requirement had N scenarios sharing a name and a
MODIFIED block kept fewer, membership still looked covered and archive
silently dropped the extras. Count occurrences per name instead and
report each excess instance as missing, keeping the existing error shape.

Refs Fission-AI#1246 (residual after Fission-AI#1252). Analysis credit: @HerbertGao.
@vishnujayvel
vishnujayvel requested a review from TabishB as a code owner July 20, 2026 13:39
@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a09ac33f-3392-44c7-aefd-feb7691fad16

📥 Commits

Reviewing files that changed from the base of the PR and between 596d6ba and 972122d.

📒 Files selected for processing (2)
  • src/core/specs-apply.ts
  • test/core/archive.test.ts

📝 Walkthrough

Walkthrough

Scenario drift detection now compares scenario multiplicity instead of name presence alone. A regression test verifies that archiving aborts, preserves the main spec, reports the failure, and leaves the change directory unarchived when duplicate scenarios are reduced.

Changes

Scenario multiplicity handling

Layer / File(s) Summary
Count-based scenario validation
src/core/specs-apply.ts, test/core/archive.test.ts
findMissingCurrentScenarios detects surplus duplicate scenarios using occurrence counts, and archive coverage verifies abort and preservation behavior.

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

Possibly related PRs

Suggested reviewers: tabishb, clay-good

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: making the archive scenario-drift check multiplicity-aware.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install timed out. The project may have too many dependencies for the sandbox.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@alfred-openspec alfred-openspec left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The counted Map closes the duplicate-name multiplicity blind spot without changing the existing error contract, and the regression covers the important safety guarantees: the main spec stays untouched and the change remains unarchived. The exact head is green across the full CI matrix.

@clay-good
clay-good added this pull request to the merge queue Jul 20, 2026
Merged via the queue into Fission-AI:main with commit 470f572 Jul 20, 2026
11 checks passed
birhantprkc pushed a commit to birhantprkc/OpenSpec that referenced this pull request Jul 29, 2026
…-audit follow-ups (Fission-AI#1475)

* fix(archive): make the scenario-drift check fence-aware

parseScenarioBlocks matched #### Scenario: headers on raw lines while the
validator's countScenarios masks fenced code blocks (Fission-AI#1151). The drift
check (Fission-AI#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>

* fix(update): tear down the redirected request when the budget expires

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>

* chore(release): add changesets for user-facing changes missing from the 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>

* fix(adapters): escape TOML-active characters in Gemini command files

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 (Fission-AI#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>

* fix(update): harden install detection and redirect handling

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>

* chore(cli): export zcodeAdapter from the barrel and sync a completion description

zcode was registered but missing from the adapters barrel (its test
imported the module directly), and the completion registry still carried
the pre-Fission-AI#1062 description for the instructions command.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(parser): strip a UTF-8 BOM before parsing specs and deltas

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>

* fix(cli): reject over-long change names with a validation message

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>

* fix(archive): finish the early-sync no-op rules for MODIFIED and RENAMED

Two asymmetries left over from the Fission-AI#1376/Fission-AI#1386/Fission-AI#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>

* fix(validate): stop reporting an unreadable specs dir as 'no deltas'

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>

* fix(update): say when commands-only delivery leaves a tool with nothing

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>

* fix(completion): honor $ZSH and $ZSH_CUSTOM for Oh My Zsh installs

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>

* fix(init): make the static welcome screen wait for the Enter it asks for

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. Fission-AI#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>

* fix(feedback): keep the manual fallback on every gh failure

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>

* test(update): model the npm shim in the Windows prefix fixture

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>

* fix(update): require volta's full tools/image layout for the undotted 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>

* fix(adapters): escape control characters in Gemini multiline prompts

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>

* fix(completion): finish the $ZSH_CUSTOM support and isolate it in tests

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 Fission-AI#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>

* chore(release): correct the hermes and zcode changeset wording

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 Fission-AI#1471 removed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* test(feedback): pin the manual fallback on a non-label gh failure

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>

* fix(completion): match fpath entries as literal strings in the OMZ guidance

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>

* fix(adapters): never emit a bare carriage return in Gemini TOML prompts

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>

* build(nix): update the pnpm deps hash for the smol-toml devDependency

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>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants