fix: scenario-level merge for MODIFIED requirements#843
Open
phuongvm wants to merge 6 commits intoFission-AI:mainfrom
Open
fix: scenario-level merge for MODIFIED requirements#843phuongvm wants to merge 6 commits intoFission-AI:mainfrom
phuongvm wants to merge 6 commits intoFission-AI:mainfrom
Conversation
Previously, the MODIFIED handler in specs-apply.ts replaced entire requirement blocks wholesale. When a delta spec only included modified/new scenarios (the natural authoring pattern), unchanged scenarios within the same requirement were silently dropped. This fix adds scenario-level parsing and merge: - New ScenarioBlock type and parseScenarios() in requirement-blocks.ts - Scenarios tagged (MODIFIED): replace matching scenario by name - Scenarios tagged (REMOVED): remove matching scenario - New scenarios (not in main): appended - Unchanged scenarios: preserved from main spec - No tags at all: falls back to full-block replacement (backward compat) - Warning emitted when scenario count decreases unexpectedly Includes 9 new test cases covering all merge behaviors. Fixes scenario data loss during archive/apply-specs operations.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
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:
📝 WalkthroughWalkthroughAdds fenced-codeblock awareness to requirement parsing, implements scenario-level merges for requirement blocks (MODIFIED/REMOVED tags, tag stripping, matched-removed accounting), introduces first-class Purpose delta support, updates validation/schema/docs, and adds comprehensive tests for parsing, merging, and purpose handling. Changes
Sequence DiagramsequenceDiagram
participant Parser as Requirement Parser
participant Merger as Scenario Merger
participant Rebuilder as Reconstructor
rect rgba(100,150,200,0.5)
Note over Parser,Merger: Parse & Merge Flow
Parser->>Parser: splitTopLevelSections(content) with fence tracking
Parser->>Parser: extractRequirementsSection(section) (skip headers inside fences)
Parser->>Parser: parseScenarios(block) -> description + scenarios[]
Parser->>Merger: send main.scenarios[], delta.scenarios[]
Merger->>Merger: build mainByName map (normalized names)
Merger->>Merger: for each delta scenario: match → replace/remove/append
Merger->>Merger: track matchedRemovedCount, log unmatched warnings
Merger->>Rebuilder: return mergedScenarios[], matchedRemovedCount
Rebuilder->>Rebuilder: stripScenarioTag() on merged items
Rebuilder->>Rebuilder: reconstructRequirementBlock(description, scenarios, header)
Rebuilder->>Parser: return merged requirement block
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![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
🧹 Nitpick comments (4)
.serena/memories/project_overview.md (1)
20-20: Hardcoded version will become stale.The version
v1.2.0is hardcoded here. Consider removing the version number or noting it may be outdated, since this documentation won't auto-update with releases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.serena/memories/project_overview.md at line 20, The package line hardcodes a specific release ("v1.2.0") for `@fission-ai/openspec`; remove the fixed version or replace it with a non‑stale phrasing (e.g., just `@fission-ai/openspec` or `@fission-ai/openspec (version may change)`) so the documentation doesn't become outdated—update the line that currently reads "**Package**: `@fission-ai/openspec` v1.2.0 (MIT, published on npm)" accordingly..serena/project.yml (1)
93-99: Consider reordering languages to prioritize TypeScript.Per the configuration comment on line 91, "the first language is the default language and the respective language server will be used as a fallback." For a TypeScript project, having
powershellfirst may not be ideal. Consider movingtypescriptto the first position.Suggested reordering
languages: -- powershell -- terraform -- yaml -- python -- markdown - typescript +- markdown +- yaml +- python +- powershell +- terraform🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.serena/project.yml around lines 93 - 99, The languages list under the languages: key places powershell first, making it the default language server; update the YAML so typescript is the first entry (i.e., move 'typescript' to the top of the array and keep the remaining items in their current order: powershell, terraform, yaml, python, markdown — or any preferred order after typescript) so the TypeScript language server becomes the default.test/core/archive.test.ts (2)
1039-1095: Test name doesn't match behavior being tested.The test is named "should emit scenario count warning on unexpected decrease" but lines 1091-1094 verify that no warning is emitted because the count didn't decrease. Consider either:
- Renaming to something like
"should not emit warning when scenario count is preserved"- Adding an additional test case that actually triggers and verifies the warning emission
The tasks.md mentions testing "Scenario count warning emitted on unexpected decrease" but this test doesn't exercise that path.
💡 Suggested test name change or additional test
- it('should emit scenario count warning on unexpected decrease', async () => { + it('should not emit warning when scenario count is preserved via (MODIFIED) tags', async () => {Or add a separate test that actually triggers the warning by having an unexpected decrease.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/core/archive.test.ts` around lines 1039 - 1095, The test name "should emit scenario count warning on unexpected decrease" is misleading because the body asserts no warning; either rename the test to reflect preserved count (e.g., "should not emit warning when scenario count is preserved") or add a new test that uses archiveCommand.execute with a change delta that removes one or more scenarios (so the resulting main spec has fewer scenarios) and assert console.log was called with expect.stringContaining('scenario count changed'); update references to changeName/changeSpecDir and mainSpecDir in the new test to create the removing delta and ensure noValidate/yes flags are consistent with other tests.
1138-1138: Consider a more robust assertion for verifying old content was replaced.The assertion
expect(updated).not.toContain('THEN gate is checked\n')relies on a specific line ending pattern. This could be fragile if the spec reconstruction normalizes line endings differently. A more robust approach would be to check for the exact old phrase that's unique to the original content.💡 Alternative assertion
- expect(updated).not.toContain('THEN gate is checked\n'); + // Ensure old scenario body is replaced - the unique old ending was just "is checked" + // while new ending is "checked with evidence" + expect(updated).not.toMatch(/THEN gate is checked(?!\s+with evidence)/);Or simply rely on the positive assertion at line 1137 which already confirms the new content is present.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/core/archive.test.ts` at line 1138, The negative assertion is brittle due to the newline; update the test around the `updated` variable to assert the absence of the unique old phrase without relying on line endings (e.g., assert `updated` does not include "THEN gate is checked" or use a regex like /THEN gate is checked/), or simply remove the negative check and rely on the existing positive assertion that the new content is present; reference the `updated` variable and the failing `expect(updated).not.toContain('THEN gate is checked\n')` assertion when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/specs-apply.ts`:
- Around line 75-127: The merged output is retaining delta-specific tags (e.g.,
"(MODIFIED)" / "(REMOVED)") because mergeScenarios pushes delta
ScenarioBlock.raw/headerLine directly; fix by normalizing scenario blocks before
adding them to the result: in mergeScenarios (references: mergeScenarios,
ScenarioBlock.raw, ScenarioBlock.headerLine, ScenarioBlock.tag) create or call a
helper (e.g., stripScenarioTag) that removes trailing "(MODIFIED|REMOVED)" from
headerLine, rebuilds raw with the cleaned header, and clears the tag, and use
that cleaned ScenarioBlock when pushing deltaScenario (where
result.push(deltaScenario) is invoked) and when appending new delta scenarios
(where result.push(s) is invoked); this ensures reconstructRequirementBlock sees
clean headers.
In `@src/utils/change-utils.ts`:
- Line 6: The default schema constant DEFAULT_SCHEMA was changed to
"opti-spec-driven" which can cause createChange() to fail because
validateSchemaName(...) still expects the original "spec-driven"; either revert
DEFAULT_SCHEMA back to "spec-driven" to preserve the previous default path used
by createChange(), or update all default-resolution paths/tests and ensure
validateSchemaName, createChange, and any schema asset deployment accept
"opti-spec-driven"; locate DEFAULT_SCHEMA, createChange, and validateSchemaName
in change-utils.ts and make the chosen change consistently across those symbols.
---
Nitpick comments:
In @.serena/memories/project_overview.md:
- Line 20: The package line hardcodes a specific release ("v1.2.0") for
`@fission-ai/openspec`; remove the fixed version or replace it with a non‑stale
phrasing (e.g., just `@fission-ai/openspec` or `@fission-ai/openspec (version
may change)`) so the documentation doesn't become outdated—update the line that
currently reads "**Package**: `@fission-ai/openspec` v1.2.0 (MIT, published on
npm)" accordingly.
In @.serena/project.yml:
- Around line 93-99: The languages list under the languages: key places
powershell first, making it the default language server; update the YAML so
typescript is the first entry (i.e., move 'typescript' to the top of the array
and keep the remaining items in their current order: powershell, terraform,
yaml, python, markdown — or any preferred order after typescript) so the
TypeScript language server becomes the default.
In `@test/core/archive.test.ts`:
- Around line 1039-1095: The test name "should emit scenario count warning on
unexpected decrease" is misleading because the body asserts no warning; either
rename the test to reflect preserved count (e.g., "should not emit warning when
scenario count is preserved") or add a new test that uses archiveCommand.execute
with a change delta that removes one or more scenarios (so the resulting main
spec has fewer scenarios) and assert console.log was called with
expect.stringContaining('scenario count changed'); update references to
changeName/changeSpecDir and mainSpecDir in the new test to create the removing
delta and ensure noValidate/yes flags are consistent with other tests.
- Line 1138: The negative assertion is brittle due to the newline; update the
test around the `updated` variable to assert the absence of the unique old
phrase without relying on line endings (e.g., assert `updated` does not include
"THEN gate is checked" or use a regex like /THEN gate is checked/), or simply
remove the negative check and rely on the existing positive assertion that the
new content is present; reference the `updated` variable and the failing
`expect(updated).not.toContain('THEN gate is checked\n')` assertion when making
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 20d00ec9-c09e-4380-92ba-c93f69a02e09
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
.serena/.gitignore.serena/memories/project_overview.md.serena/memories/style_and_conventions.md.serena/memories/suggested_commands.md.serena/memories/task_completion_checklist.md.serena/project.ymlopenspec/changes/archive/2026-03-08-fenced-codeblock-parsing/.openspec.yamlopenspec/changes/archive/2026-03-08-fenced-codeblock-parsing/design.mdopenspec/changes/archive/2026-03-08-fenced-codeblock-parsing/proposal.mdopenspec/changes/archive/2026-03-08-fenced-codeblock-parsing/specs/cli-archive/spec.mdopenspec/changes/archive/2026-03-08-fenced-codeblock-parsing/tasks.mdopenspec/changes/archive/2026-03-15-scenario-level-merge/.openspec.yamlopenspec/changes/archive/2026-03-15-scenario-level-merge/design.mdopenspec/changes/archive/2026-03-15-scenario-level-merge/proposal.mdopenspec/changes/archive/2026-03-15-scenario-level-merge/specs/cli-archive/spec.mdopenspec/changes/archive/2026-03-15-scenario-level-merge/tasks.mdopenspec/specs/cli-archive/spec.mdsrc/core/parsers/requirement-blocks.tssrc/core/specs-apply.tssrc/utils/change-utils.tstest/core/archive.test.tstest/core/parsers/requirement-blocks-fence.test.ts
…HEMA, fix tests - Add stripScenarioTag() helper to remove delta-specific tags before merging - Revert DEFAULT_SCHEMA from 'opti-spec-driven' back to 'spec-driven' - Fix misleading test name, brittle assertion, and add warning suppression test - Remove .serena/ from tracking and add to .gitignore
Author
Review Feedback Addressed (commit 2d36d2e)Thanks for the thorough review @coderabbitai! All actionable comments have been addressed: Critical Fixes
Test Quality Fixes
Additional
Verification
|
1 similar comment
Author
Review Feedback Addressed (commit 2d36d2e)Thanks for the thorough review @coderabbitai! All actionable comments have been addressed: Critical Fixes
Test Quality Fixes
Additional
Verification
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me verify the key fixes in the updated code before responding. 🏁 Script executed: Length of output: 5268
The implementation looks solid. All critical correctness issues are resolved, test coverage is comprehensive, and the 1356-test clean run confirms no regressions. The PR is ready for final merge. 🎉 If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me verify the fixes in commit 🏁 Script executed: Length of output: 2004
The tag-pollution fix is particularly well-done — The PR looks good to me. 🎉 [approve] |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/specs-apply.ts`:
- Around line 107-142: The append loop currently lets tagged delta scenarios
that never matched a main scenario become new scenarios or be silently ignored;
change it so only untagged scenarios may be appended: in the second loop over
delta.scenarios check normalizeScenarioName(s.name) against usedDeltaNames and
if not used then if s.tag is present (e.g., 'MODIFIED' or 'REMOVED') throw or
surface an error mentioning the scenario name/tag instead of appending or
ignoring it; only call stripScenarioTag(s) and push into result when the
scenario is truly untagged. This touches the delta.scenarios loop,
usedDeltaNames, normalizeScenarioName, stripScenarioTag, and deltaByName logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 290409be-0f97-46db-9f67-b25b64d8a95b
📒 Files selected for processing (4)
.gitignoresrc/core/specs-apply.tssrc/utils/change-utils.tstest/core/archive.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/utils/change-utils.ts
Collaborator
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Review from Alfred (OpenSpec Bot)
Nice work on this PR — both the fenced code block parsing fix and the scenario-level merge are solid contributions that address real pain points.
Highlights
- Dogfooding: Love that you used OpenSpec's own change workflow (proposal → design → tasks → specs). That's how it's supposed to work.
- Test coverage: 15 new tests across both features is thorough.
- Responsive to feedback: The
stripScenarioTag()fix and DEFAULT_SCHEMA revert in commit 2d36d2e were handled quickly and cleanly.
Requested Changes
-
change-utils.tscosmetic reformatting: Serena reformatted all quotes from single → double. The project uses single quotes (check any other source file). This adds 48+/24- of noise. Please revert this file or submit the style changes separately. -
package-lock.jsonversion bump (1.1.1 → 1.2.0): Version bumps happen during release, not in feature PRs. Please revert.
Minor Observations (non-blocking)
-
MODIFIED-tagged scenario with no main match: If a delta scenario is tagged
(MODIFIED)but its name doesn't match any main scenario (typo), it gets silently appended as new. CodeRabbit flagged this too — worth considering a warning here. -
Scenario count warning suppression: Currently suppressed when ANY
(REMOVED)tag exists. If you REMOVE 1 but accidentally drop 3 others, the warning won't fire. Consider: compare againstmainCount - removedCountinstead.
Overall this is a high-quality first contribution. The core merge logic is correct and well-tested. Looking forward to seeing items 1-2 cleaned up so we can merge. 🎉
… fixes - Add MergeResult interface with matchedRemovedCount for precision suppression - Add mainByName set for accurate REMOVED scenario matching - Emit warnings for unmatched MODIFIED/REMOVED scenarios (typo detection) - Revert change-utils.ts and package-lock.json to origin/main state - Add 5 new tests for unmatched warnings and precision suppression (37 total) - Update schema.yaml specs instruction with scenario-level tag guidance - Add new schema-instruction spec with 1 requirement - Update cli-archive spec with 3 merged scenarios - Archive: fix-scenario-merge-review, schema-instruction-scenario-tags
Author
Thanks for the thorough review, Alfred! Really appreciate the detailed feedback. Here's what we've done: Requested Changes — ✅ Both Fixed
Both fixes landed in commit abd9bc9. Minor Observations — ✅ Both Implemented
(MODIFIED) (REMOVED)
(REMOVED) Both improvements have dedicated tests (37 total in archive.test.ts Additionally, we updated the spec-driven schema instruction ( schema.yaml (MODIFIED) (REMOVED) Ready for re-review when you get a chance! 🚀 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
test/core/archive.test.ts (1)
838-1485: Optional: extract a scenario-merge test fixture helper to reduce duplication.The repeated directory/file setup in this block can be centralized (e.g.,
setupScenarioMergeCase) so each test focuses only on intent + assertions.Refactor sketch
+async function setupScenarioMergeCase(args: { + tempDir: string; + changeName: string; + capability?: string; + mainSpec: string; + deltaSpec: string; +}) { + const capability = args.capability ?? 'multi'; + const changeDir = path.join(args.tempDir, 'openspec', 'changes', args.changeName); + const changeSpecDir = path.join(changeDir, 'specs', capability); + const mainSpecDir = path.join(args.tempDir, 'openspec', 'specs', capability); + await fs.mkdir(changeSpecDir, { recursive: true }); + await fs.mkdir(mainSpecDir, { recursive: true }); + await fs.writeFile(path.join(mainSpecDir, 'spec.md'), args.mainSpec); + await fs.writeFile(path.join(changeSpecDir, 'spec.md'), args.deltaSpec); + return { changeDir, mainSpecDir }; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/core/archive.test.ts` around lines 838 - 1485, Extract the repeated test setup into a helper (e.g., setupScenarioMergeCase) that creates the change and main spec directories, writes main spec content (or multiScenarioMainSpec()) and delta content, and returns paths/values needed by the tests so each it-block only calls setupScenarioMergeCase(changeName, { mainContent?, deltaContent? }) then invokes archiveCommand.execute(changeName, { yes: true, noValidate: true }) and reads the updated spec; update tests to call the helper instead of repeating fs.mkdir/fs.writeFile and path.join logic to reduce duplication while keeping existing assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@openspec/changes/archive/2026-03-18-schema-instruction-scenario-tags/tasks.md`:
- Around line 13-23: The fenced code block containing the "## MODIFIED
Requirements" snippet is missing a language identifier and triggers MD040; fix
it by adding a language after the opening fence (e.g., change the opening "```"
to "```md") for the block that includes "## MODIFIED Requirements" / "###
Requirement: User Authentication" / "#### Scenario: Login with MFA (MODIFIED)";
keep the closing "```" as-is so the block is properly fenced and markdown-lint
compliant.
In `@openspec/specs/cli-archive/spec.md`:
- Around line 114-119: Update the scenario text to match the new expected-count
logic: remove the outdated "no (REMOVED) tags" condition and change the
WHEN/THEN wording to compute expected_count as main_count - matchedRemovedCount
and emit a warning only when merged_count < expected_count; update the warning
message to `"⚠️ Warning: {specName} requirement '{name}': scenario count is
{merged_count}, expected at least {expected_count}"` (or similar phrasing using
merged_count and expected_count) so it references expected_count, main_count,
matchedRemovedCount, and merged_count.
In `@openspec/specs/schema-instruction/spec.md`:
- Line 4: Replace the placeholder "TBD - created by archiving change
schema-instruction-scenario-tags. Update Purpose after archive." on the Purpose
line with the finalized Purpose statement for this spec: locate the Purpose
header/section (the line currently containing "TBD" and the reference to
schema-instruction-scenario-tags) and update it to a clear, concise description
of the spec's intent and scope, ensuring it no longer contains "TBD" or archival
notes and accurately reflects the finalized purpose of schema-instruction.
In `@src/core/specs-apply.ts`:
- Around line 121-126: The loop over delta.scenarios increments
matchedRemovedCount per REMOVED entry which double-counts duplicates; instead,
track unique removed scenario names using the existing normalizeScenarioName and
removedNames set (or create one) and only increment matchedRemovedCount when a
normalized name is newly added to removedNames and exists in mainByName. Update
the same logic where matchedRemovedCount is adjusted around the later block
referenced (lines ~446-447) to use unique names rather than raw entries so
expectedMinCount and warning suppression reflect unique removals.
- Around line 157-167: Appended scenarios from delta can be duplicated because
the normalized name isn't added to usedDeltaNames after pushing; in the loop
over delta.scenarios (the block using normalizeScenarioName(s.name) and
result.push(stripScenarioTag(s))), add usedDeltaNames.add(name) immediately
after result.push(...) so subsequent iterations skip duplicates, preserving the
existing MODIFIED/REMOVED checks and behavior.
---
Nitpick comments:
In `@test/core/archive.test.ts`:
- Around line 838-1485: Extract the repeated test setup into a helper (e.g.,
setupScenarioMergeCase) that creates the change and main spec directories,
writes main spec content (or multiScenarioMainSpec()) and delta content, and
returns paths/values needed by the tests so each it-block only calls
setupScenarioMergeCase(changeName, { mainContent?, deltaContent? }) then invokes
archiveCommand.execute(changeName, { yes: true, noValidate: true }) and reads
the updated spec; update tests to call the helper instead of repeating
fs.mkdir/fs.writeFile and path.join logic to reduce duplication while keeping
existing assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9ea2f510-ac9d-40b4-a8cc-76c5f02c36bf
📒 Files selected for processing (15)
openspec/changes/archive/2026-03-18-fix-scenario-merge-review/.openspec.yamlopenspec/changes/archive/2026-03-18-fix-scenario-merge-review/design.mdopenspec/changes/archive/2026-03-18-fix-scenario-merge-review/proposal.mdopenspec/changes/archive/2026-03-18-fix-scenario-merge-review/specs/cli-archive/spec.mdopenspec/changes/archive/2026-03-18-fix-scenario-merge-review/tasks.mdopenspec/changes/archive/2026-03-18-schema-instruction-scenario-tags/.openspec.yamlopenspec/changes/archive/2026-03-18-schema-instruction-scenario-tags/design.mdopenspec/changes/archive/2026-03-18-schema-instruction-scenario-tags/proposal.mdopenspec/changes/archive/2026-03-18-schema-instruction-scenario-tags/specs/schema-instruction/spec.mdopenspec/changes/archive/2026-03-18-schema-instruction-scenario-tags/tasks.mdopenspec/specs/cli-archive/spec.mdopenspec/specs/schema-instruction/spec.mdschemas/spec-driven/schema.yamlsrc/core/specs-apply.tstest/core/archive.test.ts
✅ Files skipped from review due to trivial changes (5)
- openspec/changes/archive/2026-03-18-schema-instruction-scenario-tags/proposal.md
- openspec/changes/archive/2026-03-18-fix-scenario-merge-review/design.md
- openspec/changes/archive/2026-03-18-schema-instruction-scenario-tags/design.md
- openspec/changes/archive/2026-03-18-schema-instruction-scenario-tags/.openspec.yaml
- openspec/changes/archive/2026-03-18-fix-scenario-merge-review/.openspec.yaml
openspec/changes/archive/2026-03-18-schema-instruction-scenario-tags/tasks.md
Outdated
Show resolved
Hide resolved
- Add PURPOSE_MODIFIED to DeltaOperationType enum - Parser extracts ## Purpose section from delta specs - Validator accepts purpose-only delta files as valid - ChangeParser emits PURPOSE_MODIFIED in parsed output - Apply/archive replaces main spec Purpose on merge - Rewrite schema-instruction delta as true Purpose delta - Add delta specs for openspec-conventions, cli-validate, specs-sync-skill - Update docs/concepts.md and schema.yaml with Purpose delta semantics - Add 9 new tests (8 purpose-delta + 1 change-parser regression) - Fix CodeRabbit C1-C5, N1 findings (mergeScenarios bugs, spec text, lint) Closes: PR Fission-AI#843 second-round review findings Reviewed-by: Reviewer-Collab (6 rounds, APPROVED v6)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/parsers/requirement-blocks.ts (2)
42-55:⚠️ Potential issue | 🟠 MajorFence tracking still misses valid Markdown fence forms.
All of these scanners toggle
inFenceonly onstartsWith('```'). Markdown fences can be indented and can use~~~, so headings inside those blocks will still be treated as real##/###/####markers and split specs incorrectly. Please centralize fence detection behind a Markdown-compatible helper and reuse it everywhere, includingreplacePurposeSection()insrc/core/specs-apply.ts.🛠️ Suggested helper
+const FENCE_DELIMITER_REGEX = /^\s*(```|~~~)/; + +function isFenceDelimiter(line: string): boolean { + return FENCE_DELIMITER_REGEX.test(line); +} @@ - if (lines[i].startsWith('```')) { + if (isFenceDelimiter(lines[i])) { inFence = !inFence; continue; }Also applies to: 67-78, 95-104, 170-181, 210-241, 339-378
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/parsers/requirement-blocks.ts` around lines 42 - 55, The scanners toggle fence state only on lines starting with "```", which misses indented fences and "~~~" variants; add a single Markdown-compatible helper (e.g., FENCE_DELIMITER_REGEX and isFenceDelimiter(line: string)) and replace all direct startsWith('```') checks in requirement-blocks.ts (the fence logic loop using inFence/inFenceEnd) and the other locations mentioned plus replacePurposeSection() in src/core/specs-apply.ts so they call isFenceDelimiter(line) to toggle the inFence flag; ensure the helper recognizes optional leading indentation and both backtick and tilde fences so headings inside fenced blocks are ignored consistently.
121-160:⚠️ Potential issue | 🟠 MajorKeep Purpose-section presence separate from Purpose text.
purposeText: purposeText || undefinederases an empty## Purposesection, andsectionPresencehas nopurposeflag. Downstream callers then cannot distinguish “Purpose header present but prose missing” from “no Purpose section at all”, which makes the new Purpose-specific validation path impossible to implement cleanly.🛠️ Suggested shape change
export interface DeltaPlan { added: RequirementBlock[]; modified: RequirementBlock[]; removed: string[]; // requirement names renamed: Array<{ from: string; to: string }>; purposeText?: string; // delta Purpose text, if present sectionPresence: { added: boolean; modified: boolean; removed: boolean; renamed: boolean; + purpose: boolean; }; } @@ - const purposeText = purposeLookup.found ? purposeLookup.body.trim() : undefined; + const purposeText = purposeLookup.found ? purposeLookup.body.trim() : undefined; return { @@ - purposeText: purposeText || undefined, + purposeText, sectionPresence: { added: addedLookup.found, modified: modifiedLookup.found, removed: removedLookup.found, renamed: renamedLookup.found, + purpose: purposeLookup.found, }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/parsers/requirement-blocks.ts` around lines 121 - 160, The parseDeltaSpec currently loses the fact that a Purpose header existed when its body is empty because it sets purposeText to undefined; update parseDeltaSpec so sectionPresence includes a purpose boolean (e.g., sectionPresence.purpose) derived from purposeLookup.found, and set purposeText to purposeLookup.found ? purposeLookup.body.trim() : undefined (so an empty Purpose section yields an empty string rather than being treated as absent). Make sure to reference the existing symbols parseDeltaSpec, purposeLookup, purposeText, and sectionPresence when implementing the change so callers can distinguish “Purpose header present but empty” from “no Purpose section”.
🧹 Nitpick comments (2)
src/core/parsers/change-parser.ts (1)
147-156: LGTM!The PURPOSE_MODIFIED delta parsing correctly:
- Finds the
## Purposesection- Skips empty sections
- Creates a delta with truncated description (80 chars max)
💡 Optional: Minor efficiency improvement
The
trim()call is repeated 3 times. Consider caching it:// Parse Purpose delta const purposeSection = this.findSection(sections, 'Purpose'); - if (purposeSection && purposeSection.content.trim()) { + const purposeContent = purposeSection?.content.trim(); + if (purposeContent) { deltas.push({ spec: specName, operation: 'PURPOSE_MODIFIED' as DeltaOperation, - description: `Update Purpose: ${purposeSection.content.trim().substring(0, 80)}${purposeSection.content.trim().length > 80 ? '...' : ''}`, + description: `Update Purpose: ${purposeContent.substring(0, 80)}${purposeContent.length > 80 ? '...' : ''}`, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/parsers/change-parser.ts` around lines 147 - 156, The purpose parsing repeats purposeSection.content.trim() multiple times—cache the trimmed string to avoid redundant work: inside the block where purposeSection is non-null, assign const trimmed = purposeSection.content.trim(); then use trimmed for the truthy check, for building the description and for computing length/substring; update the block that pushes to deltas (referencing purposeSection, specName, deltas, and DeltaOperation) to use trimmed instead of calling trim() repeatedly.test/core/purpose-delta.test.ts (1)
171-297: Add one apply test for specs that lack## Purpose.Both apply tests only exercise replacement of an existing Purpose section. The new fallback in
replacePurposeSection()that inserts## Purposeafter the title is still untested, so a regression in the “missing Purpose” path would slip through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/core/purpose-delta.test.ts` around lines 171 - 297, The tests only cover replacing an existing "## Purpose" section; add a new test that exercises the fallback path in replacePurposeSection() by providing a target spec that has no "## Purpose" header and a source delta that contains a Purpose section, then call buildUpdatedSpec(update, 'test-change') and assert the rebuilt output now includes the injected "## Purpose" and the new purpose text while preserving requirements; reference SpecUpdate, buildUpdatedSpec, and replacePurposeSection in the test so the case where Purpose is missing is covered and counts reflect no requirement removals (and any added requirements if present).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@openspec/changes/pr843-coderabbit-review-fixes/specs/specs-sync-skill/spec.md`:
- Around line 13-16: The spec requires that an "## ADDED Requirements" delta
should overwrite an existing requirement with the same name, but the ADDED
branch in src/core/specs-apply.ts currently throws "ADDED failed ... already
exists"; change the ADDED handler so when a requirement name collision is found
it performs an update/replace of the existing requirement (reuse the existing
update logic or call the same function used for CHANGED/UPDATE flows) instead of
throwing, making sure to preserve any required identifiers and run existing
validation paths; update or add tests to cover the "ADDED requirement already
exists" scenario.
In `@src/core/specs-apply.ts`:
- Around line 103-107: When building deltaByName from delta.scenarios using
normalizeScenarioName, detect and reject duplicate normalized names instead of
overwriting: iterate delta.scenarios, compute key =
normalizeScenarioName(s.name), if deltaByName.has(key) throw a descriptive error
(or aggregate conflicts and throw) naming both conflicting scenarios and their
operations; apply the same guard to the other map-building code paths that
normalize and set scenario names (e.g., any baseByName or similar maps in the
later block) so conflicting normalized names (e.g., "Foo (MODIFIED)" vs "Foo
(REMOVED)" or repeated entries) fail fast.
---
Outside diff comments:
In `@src/core/parsers/requirement-blocks.ts`:
- Around line 42-55: The scanners toggle fence state only on lines starting with
"```", which misses indented fences and "~~~" variants; add a single
Markdown-compatible helper (e.g., FENCE_DELIMITER_REGEX and
isFenceDelimiter(line: string)) and replace all direct startsWith('```') checks
in requirement-blocks.ts (the fence logic loop using inFence/inFenceEnd) and the
other locations mentioned plus replacePurposeSection() in
src/core/specs-apply.ts so they call isFenceDelimiter(line) to toggle the
inFence flag; ensure the helper recognizes optional leading indentation and both
backtick and tilde fences so headings inside fenced blocks are ignored
consistently.
- Around line 121-160: The parseDeltaSpec currently loses the fact that a
Purpose header existed when its body is empty because it sets purposeText to
undefined; update parseDeltaSpec so sectionPresence includes a purpose boolean
(e.g., sectionPresence.purpose) derived from purposeLookup.found, and set
purposeText to purposeLookup.found ? purposeLookup.body.trim() : undefined (so
an empty Purpose section yields an empty string rather than being treated as
absent). Make sure to reference the existing symbols parseDeltaSpec,
purposeLookup, purposeText, and sectionPresence when implementing the change so
callers can distinguish “Purpose header present but empty” from “no Purpose
section”.
---
Nitpick comments:
In `@src/core/parsers/change-parser.ts`:
- Around line 147-156: The purpose parsing repeats purposeSection.content.trim()
multiple times—cache the trimmed string to avoid redundant work: inside the
block where purposeSection is non-null, assign const trimmed =
purposeSection.content.trim(); then use trimmed for the truthy check, for
building the description and for computing length/substring; update the block
that pushes to deltas (referencing purposeSection, specName, deltas, and
DeltaOperation) to use trimmed instead of calling trim() repeatedly.
In `@test/core/purpose-delta.test.ts`:
- Around line 171-297: The tests only cover replacing an existing "## Purpose"
section; add a new test that exercises the fallback path in
replacePurposeSection() by providing a target spec that has no "## Purpose"
header and a source delta that contains a Purpose section, then call
buildUpdatedSpec(update, 'test-change') and assert the rebuilt output now
includes the injected "## Purpose" and the new purpose text while preserving
requirements; reference SpecUpdate, buildUpdatedSpec, and replacePurposeSection
in the test so the case where Purpose is missing is covered and counts reflect
no requirement removals (and any added requirements if present).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d105c79a-e9c0-47f4-8d5b-f152b1580b8e
📒 Files selected for processing (22)
docs/concepts.mdopenspec/changes/archive/2026-03-18-schema-instruction-scenario-tags/tasks.mdopenspec/changes/pr843-coderabbit-review-fixes/.openspec.yamlopenspec/changes/pr843-coderabbit-review-fixes/design.mdopenspec/changes/pr843-coderabbit-review-fixes/proposal.mdopenspec/changes/pr843-coderabbit-review-fixes/specs/cli-archive/spec.mdopenspec/changes/pr843-coderabbit-review-fixes/specs/cli-validate/spec.mdopenspec/changes/pr843-coderabbit-review-fixes/specs/openspec-conventions/spec.mdopenspec/changes/pr843-coderabbit-review-fixes/specs/schema-instruction/spec.mdopenspec/changes/pr843-coderabbit-review-fixes/specs/specs-sync-skill/spec.mdopenspec/changes/pr843-coderabbit-review-fixes/tasks.mdopenspec/specs/cli-archive/spec.mdopenspec/specs/schema-instruction/spec.mdschemas/spec-driven/schema.yamlsrc/core/parsers/change-parser.tssrc/core/parsers/requirement-blocks.tssrc/core/schemas/change.schema.tssrc/core/specs-apply.tssrc/core/validation/validator.tstest/core/archive.test.tstest/core/parsers/change-parser.test.tstest/core/purpose-delta.test.ts
✅ Files skipped from review due to trivial changes (9)
- openspec/changes/pr843-coderabbit-review-fixes/.openspec.yaml
- openspec/changes/pr843-coderabbit-review-fixes/specs/schema-instruction/spec.md
- schemas/spec-driven/schema.yaml
- openspec/changes/archive/2026-03-18-schema-instruction-scenario-tags/tasks.md
- openspec/changes/pr843-coderabbit-review-fixes/tasks.md
- openspec/changes/pr843-coderabbit-review-fixes/specs/openspec-conventions/spec.md
- openspec/changes/pr843-coderabbit-review-fixes/proposal.md
- openspec/changes/pr843-coderabbit-review-fixes/design.md
- openspec/specs/schema-instruction/spec.md
openspec/changes/pr843-coderabbit-review-fixes/specs/specs-sync-skill/spec.md
Outdated
Show resolved
Hide resolved
Commit
|
Phase 4 of PR Fission-AI#843 — fixes all 5 findings from CodeRabbit review on db09c44: - F1: ADDED dup scenario → abort with guidance (spec alignment) - F2: mergeScenarios() preflight validates duplicate scenario names - F3: shared isMarkdownFenceLine() replaces all inline fence checks - F4: empty Purpose section classified correctly (no spurious CHANGE_NO_DELTAS) - N2: Purpose insertion test for target spec without ## Purpose Tests: 64/64 passing | Build: clean | openspec validate: clean Reviewer-Collab: APPROVED (v10)
- Archived change to openspec/changes/archive/2026-03-20-pr843-coderabbit-review-fixes/ - Synced 5 delta specs to main specs: cli-archive, cli-validate, openspec-conventions, schema-instruction, specs-sync-skill - Updated tasks.md: marked 8.6 (Reviewer APPROVED) and 8.7 (commit+push) complete
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.
Previously, the MODIFIED handler in specs-apply.ts replaced entire requirement blocks wholesale. When a delta spec only included modified/new scenarios (the natural authoring pattern), unchanged scenarios within the same requirement were silently dropped.
This fix adds scenario-level parsing and merge:
Includes 9 new test cases covering all merge behaviors.
Fixes scenario data loss during archive/apply-specs operations.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests