fix: scenario-level merge for MODIFIED requirements#843
fix: scenario-level merge for MODIFIED requirements#843phuongvm wants to merge 2 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.
📝 WalkthroughWalkthroughTwo major features are introduced: fenced code block awareness in markdown parsing to prevent misinterpreting headers inside code blocks as document structure, and scenario-level merging for MODIFIED requirement blocks enabling granular delta spec application at the scenario level instead of full-block replacement. Changes
Sequence DiagramsequenceDiagram
participant User
participant Parser as Requirement Parser
participant Merger as Scenario Merger
participant Reconstructor as Block Reconstructor
participant Archive as Archive State
User->>Parser: parseScenarios(delta block)
Parser->>Parser: extract description & scenarios
Parser->>Parser: detect MODIFIED/REMOVED tags
Parser-->>Merger: RequirementBlockWithScenarios
Merger->>Merger: hasScenarioTags(delta)
alt Has Scenario Tags
Merger->>Parser: parseScenarios(main block)
Parser-->>Merger: main scenarios
Merger->>Merger: mergeScenarios(main, delta)
Merger->>Merger: match by normalized name
Merger->>Merger: apply MODIFIED/REMOVED/ADDED rules
Merger-->>Reconstructor: merged scenario array
Reconstructor->>Reconstructor: reconstructRequirementBlock()
Reconstructor-->>Archive: merged requirement block
else No Scenario Tags
Merger-->>Archive: full-block replacement
end
Merger->>Merger: check scenario count decrease
Merger-->>User: emit warning if unexpected decrease
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
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 Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
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
Review Feedback Addressed (commit 2d36d2e)Thanks for the thorough review @coderabbitai! All actionable comments have been addressed: Critical Fixes
Test Quality Fixes
Additional
Verification
|
|
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! |
|
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] |
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
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. 🎉
2 participants
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
Release Notes
Bug Fixes
New Features
Tests