fix(tooling): marker guard false-positives on changes deep inside a pre-existing block#904
Conversation
…inside a pre-existing block The push-to-main Marker Guard went red on `89699f6919` (the #895 merge), flagging `worker.ts:183` as "new code without altimate_change markers" even though that line sits inside a correctly-marked block. CI was broken on main. Root cause: `parseDiffForMarkerWarnings` derived marker state purely from the lines visible in each diff hunk (`git diff -U5` → 5 context lines). When a line is *modified* deep inside a large pre-existing `altimate_change` block, the block's `start` marker is further than the context window, so it never appears in the hunk; `inMarkerBlock` stays false and the change is mis-flagged. The per-hunk marker-state reset compounded it. Fix: compute marker coverage from the FULL post-change file (`computeMarkedLines`, depth-counted so nested start/end pairs are handled) and pass it into the parser. A changed line is covered if the in-hunk tracker saw its `start` OR the full-file map says it's inside a marked block. The map can only *suppress* false positives, never add new ones, so all existing warning behavior is preserved. - `computeMarkedLines(content)` — full-file, nesting-aware coverage set. - `parseDiffForMarkerWarnings(file, diff, markedLines?)` — optional coverage map. - `checkFileForMarkers` reads `git show HEAD:<file>` (or the working tree) and feeds the map in. - 11 new unit tests: nesting, unbalanced markers, and the exact context-window false positive (with/without coverage, plus an end-to-end reconstruction). Verified: with the fix, `analyze.ts --markers --base 7e909a9 --strict` (the exact base that failed) passes; reverting the fix reproduces the worker.ts:183 failure. Closes #876
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
Too many files changed? Review this PR in Change Stack to see how the pieces fit before you dive in. Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Disabled knowledge base sources:
📝 WalkthroughWalkthroughThe PR extends marker-based change detection to fix false positives when ChangesFull-file marker coverage and false-positive suppression
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
What does this PR do?
Fixes the Marker Guard CI job, which went red on
main(commit89699f6919, the #895 merge) by false-positiving onpackages/opencode/src/cli/cmd/tui/worker.ts:183— a line that is correctly wrapped in analtimate_changeblock.Root cause:
parseDiffForMarkerWarningsderived marker state only from the lines visible in each diff hunk (git diff -U5→ ±5 context lines). When a line is modified deep inside a large pre-existingaltimate_changeblock, the block'sstartmarker sits outside the context window and never appears in the hunk, soinMarkerBlockstaysfalseand the change is mis-flagged as unmarked. The per-hunk marker-state reset compounded it.Fix: compute marker coverage from the full post-change file and pass it into the parser:
computeMarkedLines(content)— full-file, depth-counted coverage set (handles nestedstart/endpairs, which a singleopenBlockwould miss).parseDiffForMarkerWarnings(file, diff, markedLines?)— a changed line is covered if the in-hunk tracker saw itsstartor the full-file map says it's inside a marked block. The map can only suppress false positives, never add new ones — so all existing warning behavior is preserved.checkFileForMarkersreadsgit show HEAD:<file>(or the working tree for base-less runs) and feeds the map in.Type of change
Issue for this PR
Closes #876
How did you verify your code works?
script/upstream/analyze.test.ts: nesting, unbalanced markers, and the exact context-window false positive (with/without coverage map, plus an end-to-end reconstruction). Full file: 29/29 pass.analyze.ts --markers --base 7e909a9777 --strict(the exact base the CI job used) flagsworker.ts:183with the original code and passes with the fix. Reverting the fix reproduces the failure.Checklist
Summary by cubic
Fix Marker Guard false positives when editing lines deep inside a pre-existing
altimate_changeblock. CI no longer misflags valid changes, and genuine warnings are unchanged.computeMarkedLinesfor full-file, nesting-aware marker coverage.parseDiffForMarkerWarningsto take a coverage map; a line is covered if seen in-hunk or in the map.checkFileForMarkersnow reads the post-change file to supply coverage; added tests for the context-window case and nested markers. Closes fix(tooling): marker-guard false-positive across tags — carry marker state across diff hunks #876.Written for commit d54778f. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests