fix(deleted-symbols): replace predicate with base_tip - HEAD, drop merge-base, rewrite tests for merge-result model - #2226
Conversation
…rge-base, rewrite tests for merge-result model - compute signal = symbols(base_tip) - symbols(HEAD); remove _merge_base and mb_symbols entirely - keep the merge-ref checkout and Removes-Intentionally waiver - rewrite integration tests to model the merge result (merge PR into base, run there) instead of checking out the PR head - invert test_no_signal_when_dev_unchanged_since_merge_base to expect violations when base code is lost in the merge result - add probe-3 test: PR deletes a long-merged test file, gate must FAIL - fix _find_adding_commit to search base history via git log -S instead of the removed mb..base range - add permissions contents:read on the job and move github.base_ref into env in the workflow
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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 |
|
|
PR Summary by QodoFix deleted-symbols gate to compare base tip vs merge result
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
|
Lead verification, per the gate rule: I ran this script from the branch in a constructed scratch repo, all three directions, at the merge-result ref CI checks out. |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| violations: list[Violation] = [] | ||
| waived_in_signal: set[str] = set() | ||
| for symbol, kind in signal.items(): | ||
| if symbol in waived_set: | ||
| waived_in_signal.add(symbol) | ||
| continue | ||
| file_path, name = symbol.rsplit(":", 1) | ||
| added_by = _find_adding_commit(file_path, name, kind, base_ref, repo_root) | ||
| violations.append(Violation(symbol=symbol, added_by=added_by)) |
There was a problem hiding this comment.
3. Per-symbol git log spawning 🐞 Bug ➹ Performance
For every missing symbol, check_deleted_symbols() runs a separate git log -S search (and sometimes a second fallback search), which can scale poorly when the merge result drops many symbols (e.g., large refactors or file deletions).
Agent Prompt
### Issue description
`check_deleted_symbols()` shells out to git once per violation to find the introducing commit. `_find_adding_commit()` itself can run up to two `git log -S` calls (fallback path), so a large number of violations can result in many git processes.
### Issue Context
This runs in CI on PRs; the number of missing symbols can spike during broad refactors or when a whole Python file is deleted.
### Fix Focus Areas
- scripts/check_deleted_symbols.py[185-193]
- scripts/check_deleted_symbols.py[106-133]
### Suggested fix
- Add memoization in `_find_adding_commit` or around it (e.g., cache by `(base_ref, file_path, kind, leaf)`), so repeated lookups don’t respawn git.
- Add `--max-count=1` to the `git log` calls since only the first line is used.
- Optionally, when `len(violations)` exceeds a threshold, skip commit attribution or attribute only the first N symbols and clearly state the truncation in output (to keep runtime bounded).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
nemotron-ultra-kilo review VERDICT: BLOCKING - Fundamental algorithmic flaw causes false positives; missing merge-base comparison defeats the tool's stated purpose.
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
|
nemotron-ultra-orB review VERDICT: Strong implementation with good test coverage; one correctness concern and minor style nits.
No blocking issues beyond the async function detection bug and the checkout version. Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
|
nemotron-super review VERDICT: Pass Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
…scenario, base-branch wording, deterministic violation order
|
Qodo adjudication + folds pushed: (1) REAL - test_deletes_own_newly_added_code_passes committed its PR-branch work on main (_branch creates without checkout; every sibling test has the _checkout call, this one missed it) - fixed, the test now models its scenario and still passes for the right reason. (2) REAL-minor - failure text said symbols landed on dev after the branch cut, which is not what the predicate measures - reworded to base-branch-vs-merge-result. (3) Performance note on per-symbol git log -S - real but deletions are rare; noted, not folded. (4) REAL-trivial - violation order now sorted for stable CI output. 23/23 tests green locally. Kilo check is the rate-limited fake-red (no review content) - this push retriggers it; merging on a real kilo result + green CI. |
Thanks for the update. The changes address finding 1, finding 2, and finding 4; finding 3 remains a documented performance consideration. The reported 23/23 green tests are consistent with the revised diff. |
1 participant
Autonomous build of board card tsk-ct2c3z.
and mb_symbols entirely
base, run there) instead of checking out the PR head
violations when base code is lost in the merge result
of the removed mb..base range
env in the workflow
Files:
.github/workflows/deleted-symbols-gate.yml | 41 +++
scripts/check_deleted_symbols.py | 236 +++++++++++++++
tests/test_check_deleted_symbols.py | 442 +++++++++++++++++++++++++++++
3 files changed, 719 insertions(+)