[CI] Add CI check for unnecessarily deleted comments #570
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| name: Check deleted comments | ||
| on: | ||
| workflow_dispatch: | ||
| pull_request: | ||
| types: | ||
| - opened | ||
| - reopened | ||
| - synchronize | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.head_ref || github.ref }} | ||
| cancel-in-progress: true | ||
|
Comment on lines
+1
to
+11
There was a problem hiding this comment. 🟣 Pre-existing defense-in-depth nit. This workflow declares no top-level or job-level Extended reasoning...What the bug isThe new Why this matters in contextThe PR introduces two known compromise surfaces in this same workflow that have already been raised on the PR:
Independently, Addressing the refutationThe refutation makes two arguments worth addressing:
The refutation is right that this is not a sole-cause vulnerability — it's a defense-in-depth concern that interacts with the other already-flagged issues. That is exactly why the recommended severity is pre_existing, not a merge-blocker. Step-by-step proof
How to fixAdd at the top of permissions:
contents: readTwo lines, no behavior change for the legitimate path ( |
||
| jobs: | ||
| check-deleted-comments: | ||
| name: Check deleted comments | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Wait using dead-reckoning for builds to finish, before running, to save AI cost, if someone pushes many commits in a row | ||
| run: sleep 1800 | ||
|
|
||
| - name: Get diff of changed files | ||
| id: changed | ||
| env: | ||
| BASE_REF: ${{ github.base_ref }} | ||
| run: | | ||
| git diff "origin/$BASE_REF...HEAD" \ | ||
| -- '*.py' '*.cpp' '*.h' '*.hpp' '*.c' '*.cc' \ | ||
|
claude[bot] marked this conversation as resolved.
claude[bot] marked this conversation as resolved.
|
||
| > /tmp/pr_diff.patch | ||
|
|
||
| if [ ! -s /tmp/pr_diff.patch ]; then | ||
| echo "skip=true" >> "$GITHUB_OUTPUT" | ||
| echo "No relevant files changed." | ||
| else | ||
| echo "skip=false" >> "$GITHUB_OUTPUT" | ||
| echo "Diff written ($(wc -l < /tmp/pr_diff.patch) lines)" | ||
| fi | ||
|
|
||
| - name: Install Cursor CLI | ||
| if: steps.changed.outputs.skip != 'true' | ||
| run: | | ||
| curl https://cursor.com/install -fsS | bash | ||
| echo "$HOME/.cursor/bin" >> $GITHUB_PATH | ||
|
|
||
| - name: Check for unnecessarily deleted comments | ||
| if: steps.changed.outputs.skip != 'true' | ||
| env: | ||
| CURSOR_API_KEY: ${{ secrets.CURSOR_KEY_HUGH }} | ||
|
Comment on lines
+47
to
+50
There was a problem hiding this comment. 🟣 Fork PR runs always crash because GitHub Actions does not pass repository secrets to Extended reasoning...What the bug isThe new workflow triggers on The Cursor CLI requires a valid API key for authentication and exits non-zero on auth failure. GitHub Actions runs each Why existing safeguards do not prevent itThe workflow has no Step-by-step proof
ImpactEvery fork PR sees this CI check fail with an unrelated auth error, regardless of whether the contributor deleted any comments. If How to fixApply the same one-line guard to both workflows. Any of these works: - name: Check for unnecessarily deleted comments
if: steps.changed.outputs.skip != 'true' && github.event.pull_request.head.repo.full_name == github.repository
env:
CURSOR_API_KEY: ${{ secrets.CURSOR_KEY_HUGH }}
...Alternatively: skip when Severity
|
||
| run: | | ||
| RESULT=$(agent -p "$(cat <<'PROMPT' | ||
| The file /tmp/pr_diff.patch contains a unified diff of changes in this PR. Check for comments or docstrings that have been deleted (lines starting with -) without good reason. | ||
|
|
||
| A deletion is UNNECESSARY if: | ||
| - A comment explaining non-obvious logic, a trade-off, or a constraint was removed, and the code it described is still present | ||
| - A docstring was removed or significantly reduced without the function being deleted | ||
|
|
||
| A deletion is FINE if: | ||
| - The code the comment described was also deleted | ||
| - The comment is no longer correct | ||
|
|
||
| Use the @@ hunk headers and --- file headers to determine file paths and line numbers. | ||
| Do NOT modify any files. | ||
| Be precise: only flag clear violations, not borderline cases. | ||
| Stop after finding 10 violations. | ||
|
|
||
| If there are NO violations, your final output must start with the word PASS. | ||
| If there ARE violations, your final output must start with the word FAIL, followed by a list of violations (up to 10) in the format: <filepath>:<line_number>: <brief description> | ||
| PROMPT | ||
| )" --model claude-4.6-opus-high-thinking --mode ask --output-format text --trust) | ||
|
Comment on lines
+52
to
+71
There was a problem hiding this comment. 🟣 The agent at lines 50-69 reads Extended reasoning...What the bug isThe "Check for unnecessarily deleted comments" step (lines 50-69) constructs a prompt that includes This is the classic LLM prompt-injection anti-pattern: untrusted input is fed to a model whose output is then trusted by downstream code. The downstream code here is Why existing safeguards do not prevent itThe prompt's only constraint is "Be precise: only flag clear violations, not borderline cases." There is no instruction to disregard directives embedded in the diff body, and no syntactic delimiter (XML tags, code fence, etc.) separating the file path reference from the rest. Models — even high-thinking ones — are not robustly resistant to in-band instructions, especially when those instructions come framed as authoritative "ignore previous" overrides or appear to come from a trusted source ("this deletion was reviewed by a maintainer"). Step-by-step proof
The same vector applies to the existing Impact and mitigating factors
How to fixAdd a one-line guardrail to the prompt. Smallest change: Optionally, drop |
||
|
|
||
| echo "$RESULT" | ||
| if echo "$RESULT" | grep -qE "^FAIL([[:space:]]|$)"; then | ||
|
There was a problem hiding this comment. 🟡 The regex Extended reasoning...What the bug isCommit How it manifestsThe prompt instructs the agent to output verdicts in the form
I verified empirically: only bare Why this is worse than the alternativeThe prior review comment explicitly listed three options: a word boundary Addressing the refutationThe refutation argues the prompt directs the model to emit bare
ImpactDirection is silent failure: a real FAIL verdict silently turns into a green CI, defeating the check on exactly the cases it exists to catch. Probability is modest (depends on the model emitting punctuation after Step-by-step proof$ printf "FAIL: foo.py:42: comment removed without code deletion\n" \
| grep -qE "^FAIL([[:space:]]|\$)" && echo MATCH || echo NO_MATCH
NO_MATCH
$ printf "FAIL: foo.py:42: comment removed without code deletion\n" \
| grep -qE "^FAIL\b" && echo MATCH || echo NO_MATCH
MATCH
$ printf "FAILED to determine line numbers\n" \
| grep -qE "^FAIL\b" && echo MATCH || echo NO_MATCH
NO_MATCH # \b still rejects FAILED — best of both worldsFixif echo "$RESULT" | grep -qE "^FAIL\b"; then exit 1; fiOr, more robustly, parse just the last non-empty line of |
||
| exit 1 | ||
|
claude[bot] marked this conversation as resolved.
|
||
| fi | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟣 The workflow declares
workflow_dispatchas a trigger (line 3), butgithub.base_refis only populated forpull_request/pull_request_targetevents — on a manual run BASE_REF is empty and line 29 expands togit diff "origin/...HEAD", which git rejects as an ambiguous ref, so the step fails underset -e. Manual invocations from the Actions tab will therefore always crash. Pre-existing —check_wrapping.ymlline 27 has the identical broken pattern; cheap fix is e.g.BASE_REF: ${{ github.base_ref || 'main' }}or droppingworkflow_dispatchfrom the triggers in both files.Extended reasoning...
What the bug is
The workflow lists
workflow_dispatchas a trigger (line 3) and reuses thepull_requestdiff logic for it. The "Get diff of changed files" step (lines 25–37) has:Per GitHub Actions context docs,
github.base_refis only populated forpull_requestandpull_request_targetevents. Forworkflow_dispatch, it is the empty string. So when an operator triggers the workflow manually from the Actions UI,BASE_REFis empty, the substituted command becomesgit diff "origin/...HEAD", and git treats that as a malformed three-dot range with an empty left-hand ref.Why existing safeguards do not prevent it
There is no fallback expression (
github.base_ref || 'main'), no input on theworkflow_dispatchblock, and noif:guard on the step. GitHub Actions runs bash steps underbash --noprofile --norc -eo pipefail, so the momentgit diffexits non-zero, the step exits and the job fails.Step-by-step proof
Reproduced locally:
In the GitHub Actions runtime:
on.workflow_dispatchfires.${{ github.base_ref }}to''(empty for non-PR events).git diff "origin/...HEAD" -- '*.py' ....git diffexits 128 with the "ambiguous argument" error above.set -e, the step fails and the job is marked red. The Cursor agent never runs.Impact
The
workflow_dispatchtrigger is effectively dead — every manual invocation crashes loudly with an opaque git error. Anyone who tries to re-run the check from the Actions UI (e.g. after a transient agent failure) or to test the workflow againstmainwill hit it. The failure mode is loud, not silent, so it will not mask correctness bugs, but it does mean the trigger as declared is non-functional.How to fix
Three equivalent options, any one of which works:
workflow_dispatchfrom the triggers if it isn't intended to be supported.workflow_dispatch.inputs.base_refinput and resolve it from there.The same issue exists in
.github/workflows/check_wrapping.yml(workflow_dispatchlisted in triggers,origin/${{ github.base_ref }}...HEADinlined directly into therun:block on line 27). Since this PR is the right moment to harden the pattern, ideally fix both files together.Severity
pre_existing— the broken pattern was first introduced incheck_wrapping.ymland copied into this new workflow. The PR doubles the surface area but doesn't introduce the underlying bug, and the failure mode is loud (operator sees a clear git error in the Actions log), so this is a UX/polish issue rather than a correctness regression. Worth a one-line fix while the file is being added.