fix(validate): report what a MODIFIED block adds, not only what it drops - #1809
ryandemelo wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe change adds a shared, multiplicity-aware scenario diff. Specification application and validation now report missing and added scenarios with bounded balance descriptions. Tests cover comparison, formatting, and scenario-loss guard behavior. ChangesScenario balance reporting
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant buildUpdatedSpec
participant findScenarioLossIssues
participant diffScenarioNames
participant describeScenarioBalance
buildUpdatedSpec->>diffScenarioNames: compare current and modified blocks
diffScenarioNames-->>buildUpdatedSpec: return missing, added, and counts
buildUpdatedSpec->>describeScenarioBalance: format balance
describeScenarioBalance-->>buildUpdatedSpec: return balance text
findScenarioLossIssues->>diffScenarioNames: compare current and modified blocks
diffScenarioNames-->>findScenarioLossIssues: return missing, added, and counts
findScenarioLossIssues->>describeScenarioBalance: format balance
describeScenarioBalance-->>findScenarioLossIssues: return balance text
Merge Risk: ⚪ Minimal · up to The scenario-loss messages are updated consistently for validation and archive flows, with no identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Reviewed exact head c891d85. The shared bidirectional diff preserves the existing multiplicity-aware and fence-aware loss semantics, and validate/archive remain aligned. Focused tests (45), type-checking, lint, and the hosted Linux/macOS/Windows and security matrix are green. The living cli-validate contract remains accurate, and canonical docs-lab does not currently document this diagnostic.
When the scenario-loss guard fires it names the scenarios the block omits, which is the whole message. A reader's next question is what the block put there instead, and answering it separates the two cases the guard cannot: a block that omits two names and introduces two is shaped like a rename, one that omits two and introduces none is shaped like a truncation. Today that costs opening both files. Add the counts and the introduced names to the message. This decides nothing. Intent is not recoverable from structure, the guard fires exactly as before, and the exit code is unchanged. The comparison already walked one direction, so the other is the same pass run the other way. Both directions now come from diffScenarioNames, and both commands print one shared sentence, so archive and validate cannot drift on what they report any more than they can on what they catch. findMissingCurrentScenarios stays as its missing half. Also names the antecedent in validate's fix instruction, which became ambiguous once a second list of scenarios appeared before it.
2b7c65f to
56cf051
Compare
2 participants
What
When the scenario-loss guard fires, the message now also says how many scenarios each side has and which ones the
MODIFIEDblock introduces.Suggested by @crisradu75 on #1697. Archive prints the same sentence, so the two commands cannot disagree.
Refs #1697. This is the reporting half only; the format question stays open there, so this does not close it.
Why
The guard names what the block omits, and that is the whole message. The reader's next question is what the block put there instead, and it is the fact that separates the two cases the guard cannot: a block that omits two names and introduces two is shaped like a rename, one that omits two and introduces none is shaped like a truncation. Today answering that means opening both files.
It is worth being explicit that this decides nothing, because the surrounding thread is about exactly that. @johnmcarbajal's replay over 75 archived changes on #1697 found one
MODIFIEDblock producing six findings against a single requirement — two renames and four accidents, identical in scenario count, heading and id presence, separable only by reading the prose. Intent is not recoverable from structure, and a count does not recover it either. This prints two facts and draws no conclusion from them. The guard fires on exactly the same inputs it did before, and the exit code is unchanged.That also keeps it independent of the format question on #1697, which is a maintainer decision and is not touched here.
Shape
The comparison already walked current-against-incoming; the other direction is the same pass run the other way, so both now come from one
diffScenarioNames, and both call sites print one shareddescribeScenarioBalancesentence.That is deliberate rather than tidy.
validateandarchiveagreeing about what counts as a dropped scenario is whatfindMissingCurrentScenariosexists for (#1477), and the counts printed next to it are worth no less: two commands reporting different numbers for the same block would be its own bug.findMissingCurrentScenariosstays as that function'smissinghalf, so its contract and its tests are untouched.Listing is capped at three names plus a count, so a wholesale rewrite cannot flood the message.
One wording change came with it: validate's fix instruction said "Copy them into the MODIFIED block", which became ambiguous once a second list of scenarios appeared before it. It now says "Copy the omitted scenarios".
Scope and safety
--strict, or to any exit code.max(0, N - M)unmatched. Set membership would let a duplicate hide a real addition, the mirror of the Archiving two changes that MODIFY the same requirement silently drops scenarios (distinct from #1112) #1246 blind spot the loss half already closes.parseScenarioBlocks, so a#### Scenario:inside an example block is not counted as an addition (fix(archive): make the scenario-drift check fence-aware, plus release-audit follow-ups #1475).Tests
10 added, all existing ones untouched and passing.
diffScenarioNames: both directions and both totals, added-side multiplicity, fence masking on the incoming side, and agreement withfindMissingCurrentScenarios.describeScenarioBalance: the rename shape, the adds-none shape, and the listing cap.Full suite passes (5754) on current
main.Note
docs/troubleshooting.mdquotes this message as a fragment ending before the new sentence, so it stays accurate. The publisheddocs-lab/help/troubleshooting.mddoes not cover this error at all — a separate gap, not touched here.Summary by CodeRabbit
Bug Fixes
openspec validateandopenspec archivemessages to show scenario counts and list up to three newly added scenarios.Tests