feat(validate): let a MODIFIED block declare a renamed scenario - #1843
Draft
brettheap wants to merge 1 commit into
Draft
feat(validate): let a MODIFIED block declare a renamed scenario#1843brettheap wants to merge 1 commit into
brettheap wants to merge 1 commit into
Conversation
Per CONTRIBUTING's format-change process: a proposal only, ahead of implementation. A MODIFIED requirement block's scenario-currency check (validate and archive) compares scenario titles, so a deliberate rename reads the same as an accidental drop and the only edit that satisfies the check is reverting the rename. This proposes a declaration the block can carry to say a scenario was renamed rather than dropped, answering Fission-AI#1697's design-review question with a concrete spelling and design rationale (see design.md for the alternatives considered and why each was rejected). An implementation is written and tested; it is on a separate branch, linked from the PR, pending this proposal's review. Refs Fission-AI#1793, Fission-AI#1697
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
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
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.
Refs #1793, #1697 — this is a proposal, not a fix, so it does not close either issue itself (see "Process note").
This answers #1697's Q1 ("is an explicit scenario-level operator the
direction?") with a concrete spelling and design rationale. Its Q2 (should
it also cover
REMOVED Scenario?) is deliberately left open — see "Deliberatelyout of scope" in
design.md— so that issue should stay open for it.Process note, read this first
Per CONTRIBUTING §2 this is a format change ("anything that changes OpenSpec's
architecture needs an OpenSpec change proposal first... Open it as a PR
containing only
openspec/changes/<name>/and wait for it to be approvedbefore you write the code"), so this PR contains only
openspec/changes/add-scenario-rename-declaration/—proposal.md,design.md,tasks.md, and the two delta specs.The implementation is written and tested —
tasks.mdis checked off — but itis deliberately not in this PR. It sits on a separate branch in my fork,
ready to open as its own PR the moment the format is agreed:
main...brettheap:OpenSpec:fix/scenario-rename-declaration-impl
That link is a read-only diff for reviewers who want to see the shape of the
code before ruling on the format — not a request to review code in this PR.
What
A
MODIFIEDrequirement block may declare that a scenario the main spec stillcarries was renamed, not dropped:
That is #1697's own "Suggested fix" option 1, unchanged.
Why
The loss guard compares scenario TITLES, so a deliberate rename is
indistinguishable from an accidental drop and the only edit that satisfies it is
restoring the old title — reverting the change where the rename was the point.
#1793 is the sharper form: there the narrowed successor contradicts its
predecessor, so "just keep both scenarios" puts two conflicting statements in one
requirement block.
The precedent for the fix is inside this very check: a
MODIFIEDwhoserequirement is renamed away by
## RENAMED Requirementsin the same delta isalready skipped (
renamedAway). This is that affordance one level down.Detection is not weakened, and that was the design constraint. @johnmcarbajal's
replay over 75 archived changes (quoted in full in
proposal.md) found the guardcatching real losses — 4 caught by hand at archive time, 2 shipped and repaired
by a later change — so this adds no inference and no suppression. An UNDECLARED
omission is reported exactly as today, at the same level, with the same message
and exit code.
Shape
A declaration is a claim, not a suppression. The scenario named by
TO:mustreally be in the block. Where it is not, no credit is granted, the omission is
still reported as the loss it is, and the unbacked declaration is reported too.
So a declaration can only ever say "this old title is now that new one", and the
new one has to be there to say it.
declaration, so two instances of a title and one declaration still leaves one
unaccounted for. A duplicate cannot hide a loss behind a single declaration
(Archiving two changes that MODIFY the same requirement silently drops scenarios (distinct from #1112) #1246 / fix(archive): make scenario-drift check multiplicity-aware (#1246) #1391).
example documenting the syntax declares nothing.
current spec; one that survived into canon would otherwise be a standing
exemption for that title.
scenarios superseded by a single narrower one), which is the shape both
reporters described.
parseRenamedPairs, where a dropped half meant a requested rename neverhappened while archive reported success.
validateandarchiveshare the comparison AND the sentence, so adeclaration accepted at authoring time cannot be refused at archive time
(the parity
findMissingCurrentScenariosexists for, validate: omitted scenarios in a MODIFIED requirement are only rejected by archive #1477).merge INTO and carries no delta operation markers — the sync guidance says so
in those words. It is stripped before the "already in sync" comparison as well
as before the write, so re-archiving a change whose only edit was a declared
rename is a no-op rather than a whitespace rewrite.
findMissingCurrentScenarioskeeps its signature and its return type; thedeclaration enters as extra credit seeded into the tally it already builds. That
is deliberate — #1809 is open on the same function, and this keeps the overlap
textual rather than semantic.
Why NOT
#### RENAMED ScenarioThe follow-up comment on #1697 suggested a
#### RENAMED Scenarioheading withFROM:/TO:bullets under it. That spelling cannot work, and the reason isworth stating because it looks fine right up until the guard's arithmetic is
wrong:
parseScenarioBlockscounts EVERY non-fenced level-4 header as ascenario, on purpose, because the spec path's
countScenariosdoes and the twomust agree (#1521 —
SCENARIO_HEADER's own comment warns against breaking thatparity). A
#### RENAMED Scenarioheading would therefore enter the comparisonas a scenario in its own right: the block gets credited with a scenario nobody
wrote. A bullet cannot collide with it.
Alternatives considered
## RENAMED Scenariossection (#1793's first ask)DeltaPlaninto both commands, and a caller that missed the new argument would silently lose the declaration — reopening the validate/archive divergence #1477 closed. Inside the block, both commands read it from the one function they already share.openspec/config.yaml(#1793's second ask)REMOVED Scenarioin the same change (#1697 Q2)Scope and safety
declaration, so a corpus that writes none behaves identically.
RENAMED SCENARIO FROM:/TO:inside aMODIFIEDblock is now read as adeclaration rather than prose.
reconciling against the current base, not declaring an intent the author never
had. Cannot rename a scenario —
MODIFIEDreads a rename as a dropped scenario and blocks archive #1697's replay separates that class for exactly this reason.any input that validates today.
Tests (already written, on the linked branch)
test/core/parsers/requirement-blocks.test.ts— 18 added: pairing, the threeaccepted name forms, every CommonMark bullet marker, fence masking, unpaired
halves, absent successor, multiplicity, merge, a declaration in the CURRENT
spec ignored, and the strip (including byte-identity for a block with none).
test/core/validation.scenario-rename.test.ts— 7 added, both reportedreproductions driven through both commands: accepted and applied, the
declaration absent from the written spec, already-in-sync on a second archive,
and refused by both for an absent successor and for an unpaired
FROM:.test/core/templates/skill-templates-parity.test.tsare updated, because thatguard pins template payloads byte-for-byte and the guidance edit (documenting
that the declaration must not be copied into a main spec) is a deliberate
content change — no assertion logic touched.
I ran the full toolchain myself on the implementation branch before opening
this proposal, in two passes:
pnpm build,pnpm exec tsc --noEmit, andpnpm lintare clean. All 68tests in the three files this change touches or adds pass, both inside the
full run and in isolation:
test/core/parsers/requirement-blocks.test.ts(35),test/core/validation.scenario-rename.test.ts(7),
test/core/templates/skill-templates-parity.test.ts(26).Tests 4 failed | 4584 passed (4588), all fourpre-existing and unrelated to this change (two
completion-tiptimingassertions, one
file-statelock-contention timeout, one completion-cacheTTL test that only fails under full-suite parallel load) — I reproduced the
identical four on unmodified
origin/main.additional failures — every one a hard
testTimeout/hookTimeout(never anassertion) scattered across files this change does not touch (
init,update,store,workset,doctor, and others). Rather than wave thataway, I re-ran the exact same file list against a pristine
origin/mainworktree on the same machine and got the same class of failures at a
comparable rate (e.g.
test/core/archive.test.tsalone: 4 failed of 241 onboth trees, different specific tests each run — consistent with scheduler
contention, not a regression). The one failing file this change actually
touches,
test/core/specs-apply.security.test.ts, passes 6/6 in isolation.Net: build/typecheck/lint are clean, every test this proposal is responsible
for is green under any load, and nothing failing is new.
openspec/changes/*entries already trip the very check this proposalextends (a maintainer call on whether to declare those renames, not mine to
make).
Notes
.changeset/README.md's default of the normal releasecadence. Say the word and I will add one on the implementation PR — this is
user-facing enough that you may well want it release-tracked.
skills/openspec-sync-specs/SKILL.mdmoves withsrc/core/templates/workflows/sync-specs.tsviapnpm generate:skillson theimplementation branch, so the parity test stays green. The guidance change
matters beyond discoverability: without it an agent following the sync
workflow would copy the declaration lines into the main spec, which is the one
thing that guidance says never to do.
MODIFIEDfrom whole-block replacement to ascenario-level merge, which is a different model for a different problem.
This proposal assumes today's replacement semantics and would need
re-reading, not reverting, if fix: scenario-level merge for MODIFIED requirements #843 lands.
guard fires. That says which file to open; this lets an author state an
intent. Neither needs the other, and the implementation leaves
findMissingCurrentScenarios's signature alone so the two do not conflictsemantically.
Provenance
Drafted with Claude Code (Anthropic); I reviewed the design and every line of
the implementation, and ran every command above myself before opening this.