tsk-mjiicy [OPEN] CI check: detect a PR that silently DELETES merged - #2180
tsk-mjiicy [OPEN] CI check: detect a PR that silently DELETES merged#2180jaylfc wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds a Python deleted-symbols guard that compares target, PR, and merge-base revisions, supports intentional-removal waivers, reports violations, runs in GitHub Actions, and includes unit and synthetic-history integration tests. ChangesDeleted symbols detection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant GitHubActions
participant DeletedSymbolsGuard
participant GitRepository
PullRequest->>GitHubActions: trigger workflow
GitHubActions->>DeletedSymbolsGuard: pass base ref and PR body
DeletedSymbolsGuard->>GitRepository: compare target, HEAD, and merge-base snapshots
GitRepository-->>DeletedSymbolsGuard: Python symbols and git history
DeletedSymbolsGuard-->>GitHubActions: clean result or violation exit status
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
nemotron-ultra-kilo review VERDICT: The implementation is solid with comprehensive tests. One correctness bug in symbol extraction and one potential security issue.
No blocking issues beyond the nested traversal bug and missing timeout.
No blocking issues beyond the nested traversal bug and missing timeout. Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
PR Summary by QodoCI: gate PRs that silently delete symbols added after merge base
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
|
Reviewed, and I did the strongest test available: I ran this check against the actual incident it was built for. I checked out the old #2068 head (the PR that would have silently deleted merged P1 hardening) and ran your script against It caught it, and it named Two notes before this becomes a gate people rely on. 1. The stale-branch case produces 85 findings, and that is a UX problem rather than a correctness one. Every one of those 85 is technically true: #2068 was cut long ago and dev has moved a great deal, so merging it really would delete all of them. But a reviewer shown 85 items will not read 85 items. They will waive the lot, and a gate that trains people to waive is worse than no gate. Suggestion: when the count exceeds a threshold, replace the enumeration with the actual diagnosis, which is different: "your branch is N commits behind 2. Do NOT add this to required status checks yet. As a new workflow it is advisory, which is the right place to start. Let it run in report-only mode across a few days of real PRs first: if it fires on branches that are merely stale rather than genuinely reverting, that is exactly the signal to tune the threshold before it can block anyone. Making it required on day one risks jamming the queue on the noise above. Implementation itself looks right: Nice work. Fold the stale-branch messaging and I will merge it. One correction on my own testing, for the record: my first attempt at this crashed with |
Code Review by Qodo
1. Workflow uses Python 3.12
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| - uses: actions/setup-python@v7 | ||
| with: | ||
| python-version: "3.12" | ||
|
|
There was a problem hiding this comment.
1. Workflow uses python 3.12 📜 Skill insight ≡ Correctness
The new deleted-symbols-gate workflow pins python-version: "3.12", which does not enforce the required minimum runtime target of Python 3.11 for Python code in this repo. This can allow 3.12-only syntax/features to slip in without being caught by CI running on the mandated baseline.
Agent Prompt
## Issue description
The workflow `.github/workflows/deleted-symbols-gate.yml` is configured to run with Python 3.12, but the compliance requirement is to target Python 3.11 as the minimum supported version.
## Issue Context
CI should validate against the minimum supported Python version so that 3.12-only syntax/features do not merge unnoticed.
## Fix Focus Areas
- .github/workflows/deleted-symbols-gate.yml[27-30]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| - uses: actions/checkout@v7 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
There was a problem hiding this comment.
2. Gate checks merge ref 🐞 Bug ≡ Correctness
The workflow likely runs on the pull_request synthetic merge commit (refs/pull/<n>/merge) and does not explicitly checkout the PR head SHA; in that case git merge-base HEAD origin/<base> becomes the base tip, making dev_symbols and mb_symbols identical and the signal set always empty. As a result, the deleted-symbols gate can report "clean" even when the PR would silently delete symbols added to the base branch after the branch point.
Agent Prompt
## Issue description
`deleted-symbols-gate.yml` checks out the default ref for `pull_request` events, which is typically the synthetic merge commit. The guard script computes `git merge-base HEAD <base>`, and when `HEAD` is that merge commit the merge-base resolves to the base tip, making the signal empty and disabling the gate.
## Issue Context
The guard’s algorithm depends on `HEAD` being the PR branch head (not the merge result). The repo already documents that PR events use `refs/pull/<n>/merge`.
## Fix Focus Areas
- .github/workflows/deleted-symbols-gate.yml[23-39]
- Update `actions/checkout` to explicitly checkout the PR head commit, e.g.:
- `with: ref: ${{ github.event.pull_request.head.sha }}` (keep `fetch-depth: 0`).
- Keep the existing `git fetch origin "$BASE_REF"` step so `origin/${{ github.base_ref }}` is available.
(Alternative fix if you prefer not to change checkout: extend the script to accept a `--head <ref>` argument and compute merge-base against that ref instead of `HEAD`, then pass it from the workflow.)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
nemotron-super review VERDICT: Blocking issue found
Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
|
nemotron-ultra-orB review VERDICT: Approve with non-blocking findings — the guard logic is sound and well-tested, but the commit-attribution heuristic is fragile and should be tightened.
Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.github/workflows/deleted-symbols-gate.yml (3)
19-22: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider declaring least-privilege
permissions:for this job.The job only reads git history and never writes to the repo or PR, but no
permissions:block is declared, so it inherits the repository/org default (which can be broader thancontents: read).🔒️ Suggested addition
jobs: deleted-symbols-gate: runs-on: ubuntu-latest + permissions: + contents: read steps:🤖 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 @.github/workflows/deleted-symbols-gate.yml around lines 19 - 22, Add a job-level permissions block to deleted-symbols-gate granting only contents: read, while leaving the existing runs-on and steps configuration unchanged.
36-39: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winInline
${{ github.base_ref }}interpolation inrun:(template-injection flag).
--base "origin/${{ github.base_ref }}"interpolates the expression directly into the shell command, the pattern zizmor flags as template-injection. The step already demonstrates the safer alternative forPR_BODYviaenv:, and the prior "Fetch base branch" step does the same for this exact value (BASE_REF) — this step should follow suit for consistency and defense-in-depth.🔒️ Suggested fix
- name: Check for silently deleted symbols env: PR_BODY: ${{ github.event.pull_request.body }} - run: python scripts/check_deleted_symbols.py --base "origin/${{ github.base_ref }}" + BASE_REF: ${{ github.base_ref }} + run: python scripts/check_deleted_symbols.py --base "origin/$BASE_REF"🤖 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 @.github/workflows/deleted-symbols-gate.yml around lines 36 - 39, Update the “Check for silently deleted symbols” step to pass the GitHub base reference through an environment variable, following the existing BASE_REF pattern, and reference that variable in the Python command instead of interpolating github.base_ref directly in run.Source: Linters/SAST tools
23-25: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winVerify: default checkout ref is GitHub's merge commit, not the raw PR branch.
Without an explicit
ref:,actions/checkout@v7on apull_requestevent checks out the GitHub-generated test-merge commit (refs/pull/N/merge), notgithub.event.pull_request.head.sha. Butscripts/check_deleted_symbols.py's docstring documents the algorithm as comparing "the target branch head, HEAD (the PR branch), and the merge base" — i.e. it assumesHEADis the raw branch tip.This matters two ways:
- If intentional,
HEADsymbols reflect dev merged into the PR branch, which is actually closer to testing the real "clean merge, no conflict" failure mode this gate targets — worth documenting explicitly if so.- If unintentional, and the PR currently has any conflicts with the base,
refs/pull/N/mergecan be stale or unavailable, and checkout can fail outright (unrelated to any real deleted-symbol violation).If the raw branch tip is what's intended, pin it explicitly:
with: ref: ${{ github.event.pull_request.head.sha }} fetch-depth: 0Separately, since this job never pushes and doesn't need to persist git credentials, consider adding
persist-credentials: falsehere.🤖 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 @.github/workflows/deleted-symbols-gate.yml around lines 23 - 25, Update the actions/checkout step in the deleted-symbols gate to explicitly check out github.event.pull_request.head.sha, preserving fetch-depth: 0 so the comparison history remains available. Add persist-credentials: false because the job does not push changes, and ensure the script’s documented HEAD assumption matches the raw PR branch checkout.tests/test_check_deleted_symbols.py (1)
241-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnused
waivedunpacking (Ruff RUF059) in 4 tests.
test_deleted_class_added_after_merge_base_fails(L241),test_deleted_method_added_after_merge_base_fails(L265),test_deleted_test_function_added_after_merge_base_fails(L287), andtest_no_signal_when_dev_unchanged_since_merge_base(L311) unpackviolations, waived = ...but never assert onwaived.🧹 Suggested fix (repeat for each of the 4 occurrences)
- violations, waived = cds.check_deleted_symbols("main", repo) + violations, _waived = cds.check_deleted_symbols("main", repo)Also applies to: 265-265, 287-287, 311-311
🤖 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 `@tests/test_check_deleted_symbols.py` at line 241, Remove the unused waived unpacking in the four tests test_deleted_class_added_after_merge_base_fails, test_deleted_method_added_after_merge_base_fails, test_deleted_test_function_added_after_merge_base_fails, and test_no_signal_when_dev_unchanged_since_merge_base. Update each check_deleted_symbols call to retain only the violations result while preserving the existing assertions.Source: Linters/SAST tools
🤖 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 `@scripts/check_deleted_symbols.py`:
- Around line 131-141: Update the fallback search pattern in _find_adding_commit
to require a word boundary after leaf, preventing names such as FooBar or
FooExtended from matching a requested Foo symbol. Preserve the existing git
search flow and unknown fallback while ensuring the exact class or symbol name
is matched.
---
Nitpick comments:
In @.github/workflows/deleted-symbols-gate.yml:
- Around line 19-22: Add a job-level permissions block to deleted-symbols-gate
granting only contents: read, while leaving the existing runs-on and steps
configuration unchanged.
- Around line 36-39: Update the “Check for silently deleted symbols” step to
pass the GitHub base reference through an environment variable, following the
existing BASE_REF pattern, and reference that variable in the Python command
instead of interpolating github.base_ref directly in run.
- Around line 23-25: Update the actions/checkout step in the deleted-symbols
gate to explicitly check out github.event.pull_request.head.sha, preserving
fetch-depth: 0 so the comparison history remains available. Add
persist-credentials: false because the job does not push changes, and ensure the
script’s documented HEAD assumption matches the raw PR branch checkout.
In `@tests/test_check_deleted_symbols.py`:
- Line 241: Remove the unused waived unpacking in the four tests
test_deleted_class_added_after_merge_base_fails,
test_deleted_method_added_after_merge_base_fails,
test_deleted_test_function_added_after_merge_base_fails, and
test_no_signal_when_dev_unchanged_since_merge_base. Update each
check_deleted_symbols call to retain only the violations result while preserving
the existing assertions.
🪄 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 Plus
Run ID: 7c9e771f-df5c-4eec-86d5-7b7b032b76d4
📒 Files selected for processing (3)
.github/workflows/deleted-symbols-gate.ymlscripts/check_deleted_symbols.pytests/test_check_deleted_symbols.py
| # Fallback: try without the opening paren (for classes without bases, | ||
| # e.g. `class Foo:` rather than `class Foo(Bar):`). | ||
| search = f"{kind} {leaf}" | ||
| out = _run_git( | ||
| ["log", "--oneline", "--reverse", "-S", search, f"{merge_base}..{target}", "--", file_path], | ||
| cwd=repo_root, | ||
| ) | ||
| lines = out.splitlines() | ||
| if lines: | ||
| return lines[0] | ||
| return "unknown" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fallback class-name search can match a longer name sharing the same prefix.
The fallback pattern f"{kind} {leaf}" (no trailing ( or :) is a raw substring match. For class Foo: it also matches class FooBar: or class FooExtended(Base):, so _find_adding_commit can attribute a deleted Foo class to the commit that actually added an unrelated FooBar/FooExtended class. This only corrupts the "added by" message (the pass/fail signal itself is computed elsewhere via exact symbol keys), but that message is the tool's main auditability payoff.
🐛 Suggested fix: anchor the fallback to a word boundary
# Fallback: try without the opening paren (for classes without bases,
# e.g. `class Foo:` rather than `class Foo(Bar):`).
- search = f"{kind} {leaf}"
+ search = f"{kind} {leaf}:"
out = _run_git(
["log", "--oneline", "--reverse", "-S", search, f"{merge_base}..{target}", "--", file_path],
cwd=repo_root,
)📝 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.
| # Fallback: try without the opening paren (for classes without bases, | |
| # e.g. `class Foo:` rather than `class Foo(Bar):`). | |
| search = f"{kind} {leaf}" | |
| out = _run_git( | |
| ["log", "--oneline", "--reverse", "-S", search, f"{merge_base}..{target}", "--", file_path], | |
| cwd=repo_root, | |
| ) | |
| lines = out.splitlines() | |
| if lines: | |
| return lines[0] | |
| return "unknown" | |
| # Fallback: try without the opening paren (for classes without bases, | |
| # e.g. `class Foo:` rather than `class Foo(Bar):`). | |
| search = f"{kind} {leaf}:" | |
| out = _run_git( | |
| ["log", "--oneline", "--reverse", "-S", search, f"{merge_base}..{target}", "--", file_path], | |
| cwd=repo_root, | |
| ) | |
| lines = out.splitlines() | |
| if lines: | |
| return lines[0] | |
| return "unknown" |
🤖 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 `@scripts/check_deleted_symbols.py` around lines 131 - 141, Update the fallback
search pattern in _find_adding_commit to require a word boundary after leaf,
preventing names such as FooBar or FooExtended from matching a requested Foo
symbol. Preserve the existing git search flow and unknown fallback while
ensuring the exact class or symbol name is matched.
1 participant
Autonomous build of board card tsk-mjiicy.
Files:
scripts/check_deleted_symbols.py | 249 +++++++++++++++
tests/test_check_deleted_symbols.py | 361 +++++++++++++++++++++
tests/test_library.py | 421 -------------------------
tinyagentos/knowledge_fetchers/youtube.py | 17 -
tinyagentos/library_collections.py | 2 +-
tinyagentos/library_pipeline.py | 330 ++-----------------
tinyagentos/routes/library.py | 20 +-
10 files changed, 670 insertions(+), 844 deletions(-)
Summary by Gitar
deleted-symbols-gate.ymlworkflow to prevent silent deletion of code added after branch creationcheck_deleted_symbols.pyscript to detect and report deleted Python symbols and their adding commitstest_check_deleted_symbols.pyThis will update automatically on new commits.
Summary by CodeRabbit
New Features
Tests