Skip to content

fix(parser): classify delta operation by earliest keyword, hyphen-safe#1331

Open
seattled23 wants to merge 1 commit into
Fission-AI:mainfrom
seattled23:fix/delta-op-classification
Open

fix(parser): classify delta operation by earliest keyword, hyphen-safe#1331
seattled23 wants to merge 1 commit into
Fission-AI:mainfrom
seattled23:fix/delta-op-classification

Conversation

@seattled23

@seattled23 seattled23 commented Jul 8, 2026

Copy link
Copy Markdown

Problem

parseDeltas classifies each delta bullet's operation (ADDED / MODIFIED / REMOVED / RENAMED) by scanning the description against a fixed keyword priority using \b word boundaries. Two issues compound:

  1. \b matches inside hyphenated wordsadd matches inside add-ons, new inside new-user.
  2. Fixed type-priority lets that false match win over the real leading verb.

So "Remove the add-ons page" classifies as ADDED instead of REMOVED.

Fix

Select the operation by earliest keyword position in the description, and use hyphen-safe boundaries so add-ons / new-user no longer trigger a false ADDED. The leading verb wins by position.

This also corrects a latent case: "Remove the rename button" previously classified as RENAMED (priority) and now correctly resolves to REMOVED (earliest).

Test

Adds coverage for the reported case plus ADDED / REMOVED controls, asserting the parsed operation on real markdown deltas.

Co-authored-by: Sōren Vale soren@tessara.us

Summary by CodeRabbit

  • Bug Fixes

    • Improved change-detection so the app classifies delta actions based on the first meaningful keyword in a description.
    • Reduced false matches from partial words, making ambiguous change text more reliable.
  • Tests

    • Added coverage for descriptions with overlapping or misleading keywords to verify the correct action is selected.

`parseDeltas` classified operations by scanning the description against a
fixed keyword priority using `\b` boundaries. That mislabeled bullets like
"Remove the add-ons page" as ADDED — `\b` matched "add" inside "add-ons", and
type-priority let it win over the real leading verb "Remove".

Fix selects the operation by earliest keyword position and uses hyphen-safe
boundaries so "add-ons" / "new-user" no longer trigger a false ADDED. Adds
tests covering the reported case plus ADDED/REMOVED controls.

Co-authored-by: Sōren Vale <soren@tessara.us>
@seattled23 seattled23 requested a review from TabishB as a code owner July 8, 2026 17:21
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The delta operation classification logic in parseDeltas was changed from a fixed priority if/else chain to selecting the operation whose keyword regex matches earliest in the description text. A corresponding test was added to verify correct classification for ambiguous descriptions.

Changes

Delta Classification Fix

Layer / File(s) Summary
Earliest-match operation classification
src/core/parsers/markdown-parser.ts
Replaced prioritized if/else regex chain with logic that scans ordered {op, re} patterns and picks the operation matching at the lowest index in the lowercased description, using stricter boundary regexes.
Classification test coverage
test/core/parsers/markdown-parser.test.ts
Added a test verifying delta operations are classified by leading keyword, ensuring substrings like "add-ons" or "rename"/"new-user" don't override the correct leading verb.

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

Suggested reviewers: alfred-openspec, TabishB

🚥 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: earliest-keyword delta classification with hyphen-safe matching.
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

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/core/parsers/markdown-parser.ts`:
- Around line 207-209: The regexes in markdown-parser.ts for the op mapping are
building invalid gerund forms, so `renaming`, `removing`, and `creating` are not
matched and fall through to `MODIFIED`. Update the word-pattern logic in the
parser’s operation detection table so the `RENAMED`, `REMOVED`, and `ADDED`
cases match the correct gerund spellings, and verify the matching behavior in
the relevant parser function/class that scans description text.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e663b23f-fe73-4921-af57-d1c16ee07952

📥 Commits

Reviewing files that changed from the base of the PR and between 93e27a7 and a42bbff.

📒 Files selected for processing (2)
  • src/core/parsers/markdown-parser.ts
  • test/core/parsers/markdown-parser.test.ts

Comment on lines +207 to +209
{ op: 'RENAMED', re: /(?<![\w-])rename(s|d|ing)?(?![\w-])/ },
{ op: 'REMOVED', re: /(?<![\w-])(remove(s|d|ing)?|delete(s|d|ing)?)(?![\w-])/ },
{ op: 'ADDED', re: /(?<![\w-])(add(s|ed|ing)?|create(s|d|ing)?|new)(?![\w-])/ },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Fix gerund matching for renaming, removing, and creating.

The current suffix pattern builds renameing, removeing, and createing, so descriptions like “Removing old flow” or “Creating new spec” fall back to MODIFIED.

Proposed fix
-          { op: 'RENAMED', re: /(?<![\w-])rename(s|d|ing)?(?![\w-])/ },
-          { op: 'REMOVED', re: /(?<![\w-])(remove(s|d|ing)?|delete(s|d|ing)?)(?![\w-])/ },
-          { op: 'ADDED', re: /(?<![\w-])(add(s|ed|ing)?|create(s|d|ing)?|new)(?![\w-])/ },
+          { op: 'RENAMED', re: /(?<![\w-])(?:rename[sd]?|renaming)(?![\w-])/ },
+          { op: 'REMOVED', re: /(?<![\w-])(?:remove[sd]?|removing|delete[sd]?|deleting)(?![\w-])/ },
+          { op: 'ADDED', re: /(?<![\w-])(?:add(?:s|ed|ing)?|create[sd]?|creating|new)(?![\w-])/ },
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{ op: 'RENAMED', re: /(?<![\w-])rename(s|d|ing)?(?![\w-])/ },
{ op: 'REMOVED', re: /(?<![\w-])(remove(s|d|ing)?|delete(s|d|ing)?)(?![\w-])/ },
{ op: 'ADDED', re: /(?<![\w-])(add(s|ed|ing)?|create(s|d|ing)?|new)(?![\w-])/ },
{ op: 'RENAMED', re: /(?<![\w-])(?:rename[sd]?|renaming)(?![\w-])/ },
{ op: 'REMOVED', re: /(?<![\w-])(?:remove[sd]?|removing|delete[sd]?|deleting)(?![\w-])/ },
{ op: 'ADDED', re: /(?<![\w-])(?:add(?:s|ed|ing)?|create[sd]?|creating|new)(?![\w-])/ },
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/core/parsers/markdown-parser.ts` around lines 207 - 209, The regexes in
markdown-parser.ts for the op mapping are building invalid gerund forms, so
`renaming`, `removing`, and `creating` are not matched and fall through to
`MODIFIED`. Update the word-pattern logic in the parser’s operation detection
table so the `RENAMED`, `REMOVED`, and `ADDED` cases match the correct gerund
spellings, and verify the matching behavior in the relevant parser
function/class that scans description text.

@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.

This is the right direction, but the keyword patterns still miss common gerunds before merge. The current regexes append ing to stems that already include a silent e, so they match renameing / removeing / deleteing / createing, not renaming / removing / deleting / creating.

That means descriptions like Removing the legacy flow, Renaming the capability, or Creating a new spec still fall back to MODIFIED or miss the intended operation. Please switch those to explicit alternatives such as renam(es|ed|ing)?, remov(es|ed|ing)?, delet(es|ed|ing)?, and creat(es|ed|ing)?, with regression tests for the gerunds plus the existing hyphenated cases.

Focused parser tests pass locally: npm test -- --run test/core/parsers/markdown-parser.test.ts.

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.

2 participants