Skip to content

fork-ci: fail clippy only on lines the PR changes - #16

Closed
thedancingdeveloper wants to merge 2 commits into
fork/mainfrom
fork/ci-lines
Closed

thedancingdeveloper wants to merge 2 commits into
fork/mainfrom
fork/ci-lines

Conversation

@thedancingdeveloper

Copy link
Copy Markdown

Summary

  • The file-level gate still inherits a file's whole upstream lint debt. Add a config-gated Claude Code global-instructions provider #12 edited `message_processor.rs` and was failed for a `let_and_return` at line 374, a line the PR never touched.
  • The gate now reads the changed line ranges from `git diff -U0 $base...HEAD` and fails only when a warning or error's primary span overlaps one of them. A pure deletion overlaps nothing. jq's stderr is still captured synchronously, so a broken filter fails the step.

Test plan

  • `fork/ci-lines-fail` adds a `let_and_return` as its only change, so the lint sits on a changed line. Its run must fail at `cargo clippy` naming `cli/src/main.rs`.
  • `fork/ci-lines-pass` adds the same lint and then a harmless function after it, so the lint is in a changed file but not on a changed line. Its run must pass the clippy step.

@thedancingdeveloper thedancingdeveloper left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Review: blocker (head be98bad)

The design is right: one file start count per hunk from git diff -U0, and a diagnostic fails only if its primary span overlaps a changed hunk with count > 0. Pure deletions are ignored, and jq stderr is still checked synchronously.

  1. The awk hunk parser isn't portable. sub("^+","",a[1]) is an invalid ERE (+ with nothing to repeat). I tested the exact program under mawk: regular expression compile failed (syntax error ^* or ^+). Ubuntu images commonly default awk to mawk; I haven't confirmed which awk the GitHub runner uses. Under set -o pipefail that fails the step on every PR. Use regex literals: sub(/^\+/, "", a[1]), and sub(/^b\/codex-rs\//, "", file). Verified with mawk: cli/src/main.rs 2 1 and cli/src/main.rs 6 1 for a one-line change plus a one-line append. Or call gawk explicitly after installing it.

  2. Probe evidence, required as with #11:

    • (a) a lint on a changed line fails at cargo clippy, naming that line;
    • (b) a lint elsewhere in the same changed file but outside the hunk passes the clippy step.

    Link both runs. (b) is the case #11 couldn't handle.

Non-blocking: file names with spaces would split $4. There are none in the tree today, so a comment is enough.

@thedancingdeveloper thedancingdeveloper left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Review (head 15b0b77): blocker 1 is fixed. The awk now uses regex literals sub(/^b\/codex-rs\//, "", file) and sub(/^\+/, "", a[1]), which is the form I verified under mawk. Still open: blocker 2, the probe evidence. Link (a) a lint on a changed line failing at cargo clippy and naming that line, and (b) a lint in the same changed file but outside every changed hunk passing clippy. Once both are linked and green or red as expected, this has no blockers.

A file-level gate still inherits a file's whole upstream lint debt: a PR
editing one line of message_processor.rs was failed for a let_and_return
elsewhere in that file. The gate now reads the changed line ranges from
git diff -U0 and fails only when a warning or error's primary span overlaps
one of them. A pure deletion (count 0) overlaps nothing.
@thedancingdeveloper

Copy link
Copy Markdown
Author

Blockers (head e874aec)

Epic lead. The code is unchanged and correct: git range-diff shows 27ca8d4 and e874aec identical to the reviewed be98bad and 15b0b77 (line-based overlap filter, mawk-safe regex literals). The green fork-ci run on this PR (38041022712) doesn't exercise the gate, though, and neither probe proves it:

  1. The fail-probe didn't fail at clippy. Run 37953310081 (fork/ci-lines-fail, 90e65b2) failed at cargo test. The "clippy findings in lines this PR changes" line in its log is only the echoed script source. So the clippy step passed with a let_and_return on a changed line. The run is also on the old base 219664f with the pre-nextest workflow, so it doesn't use fork-ci: fail clippy only on lines the PR changes #16's step at all.
  2. The pass-probe can't pass by design. fork/ci-lines-pass (0e220d8) adds _fork_ci_lint_elsewhere, so its lint is on changed lines, and the gate is right to flag it. It also failed at cargo test (run 37953315010). The case this PR exists for, a lint in a changed file but outside every changed hunk, is unproven.

Required, both on probe branches cut from this head (e874aec on fork/main b8011f5) so they run #16's workflow:

  • (a) fail-probe: add fn _p() -> i32 { let value = 1; value } to any file. The run must fail at the cargo clippy step, printing that file:line and the let_and_return message.
  • (b) pass-probe: edit one line of codex-rs/app-server/src/message_processor.rs far from upstream's existing let time_provider = … binding at line 370 (e.g. a comment near the top). The cargo clippy step must pass, even though clippy reports let_and_return at :370–373 in that changed file.

Link both runs and name the step each ended at. With those, this has no blockers.

@thedancingdeveloper

Copy link
Copy Markdown
Author

Superseded: these commits were cherry-picked into #22 (fork features, merged to fork/main as c2d74d2) and ship in release fork-c2d74d2d9. Closing so the work isn't merged twice. The branch is kept.

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.

1 participant