feat: hunk filters (externally decided already-reviewed hunks) - #200
feat: hunk filters (externally decided already-reviewed hunks)#200asyncawaitpromise wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces 'Hunk Filters' to Codeowners Plus, allowing external tooling to determine which post-approval changes do not require re-review. It adds configuration options, updates the diff logic to apply the filter, and implements the hook execution package with comprehensive tests. The reviewer feedback highlights two key improvements: addressing non-deterministic map iteration in the hook response processing to prevent flaky behavior, and optimizing the hunk filter application by only parsing older diffs for files present in the survivors list.
de9007c to
24847d9
Compare
When an approval is checked, the diff is taken twice from the same merge base, once to the head and once to the approved commit, and hunks appearing in both are subtracted. That subtraction is exact but literal: a hunk which has been reformatted, or had a comment edited, or is a generated file being regenerated, is not the same text and so counts as unreviewed. Whether that is right depends on the codebase. In one repository a reformat is noise; in another the formatter changing its mind is the thing somebody needs to look at. This action should not pick one on a repository's behalf, and every consumer would inherit whichever it picked. Add a seam instead. The new `hunk-filter` input names a program which receives the hunks still outstanding against an approval, alongside the hunks that approval introduced, and answers which of them the reviewer has effectively already reviewed. The policy lives with whoever wants it. Design properties: - Subtractive only: a filter can narrow what an approval is held responsible for and nothing else. It cannot add hunks, name a file it was not sent, touch ownership, or grant an approval. Resolution and the staleness check downstream run unaltered on what survives. - Fail-open on the safe side: a missing or non-executable program, a non-zero exit, a timeout, output that is not a single known-shaped JSON object, or an index outside what was sent all leave the diff exactly as computed, with a warning. A broken filter costs the optimisation, never the gate. - Bounded: 60s per call, 8MB of output, executed directly rather than through a shell, once per distinct approved commit. The README is explicit that this is a trust decision, and that a path inside the checkout is writable by the pull request author and so is the wrong thing to name. Code changes: - `pkg/hook`: wire contract, subprocess runner, response validation - `internal/git`: `DiffOption`, `WithHunkFilter`, filtering in `changesSince` - `internal/app`: hook wiring and the fail-safe policy; `main.go`/`action.yml`: `hunk-filter` input - README: "Hunk Filters" section - Tests: contract validation, every runner failure mode, and that each way a filter can misbehave leaves the diff untouched Coverage badge regenerated.
24847d9 to
c84c48f
Compare
|
Codeowners approval required for this PR: |
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| pkg/hook/hook.go | Defines the versioned hook contract, bounded subprocess runner, strict single-object JSON decoding, and response validation. |
| internal/git/diff.go | Adds optional hunk filtering to approval-time diff processing while preserving unfiltered behavior. |
| internal/app/app.go | Wires the action input into diff construction and treats hook failures as filtering no hunks. |
| action.yml | Exposes the optional hunk-filter path and forwards it to the executable. |
| main.go | Parses the hunk-filter input and passes it into application configuration. |
Sequence Diagram
sequenceDiagram
participant App
participant Diff as Git diff processing
participant Hook as Hunk filter
participant Review as Approval evaluation
App->>Diff: Compare merge base to head and approved commit
Diff->>Diff: Subtract textually matching hunks
alt Filter configured and outstanding hunks remain
Diff->>Hook: Send outstanding and approval-time hunks
Hook-->>Diff: Return reviewed hunk indexes
Diff->>Diff: Validate response and remove named hunks
end
Diff-->>Review: Remaining changes since approval
Review->>Review: Keep or dismiss approval
Reviews (2): Last reviewed commit: "fix: close four ways a filter could reac..." | Re-trigger Greptile
Review catch: json.Decoder.More() reports false when the next byte is a closing
delimiter, so it cannot tell a clean end of stream from trailing junk. A response
like {"reviewed":[...]}} passed validation and its indexes were applied.
Accepting a malformed response breaks the guarantee that anything unexpected
leaves the diff whole, and it fails in the dismiss-less direction. Require a
second Decode to return io.EOF instead, and cover a trailing brace, bracket and
bare token.
Review catches, in order of how much they mattered. A path can appear twice in one diff. git emits a delete and an add for the same name when its type changes, file to symlink among others, and the answer is keyed by name, so a filter naming that path spoke for both entries. Answering one index for one file dropped two hunks, and the path left the approval diff entirely, so an unreviewed deletion went unnoticed. The protocol has no way to say which entry is meant, so a duplicate-named path is no longer offered to the filter at all, and an answer naming one is ignored. stderr was unbounded while stdout was capped, which made the documented "60s and 8MB per call" untrue: a hook can write gigabytes inside the deadline and all of it was kept for the length of the run. Capped at 256KB into the log. The filter inherited the whole environment, INPUT_GITHUB-TOKEN included, so a compiled hook could read the token the action runs with. That is a bigger risk than the one the README warned about, and it is now removed from the child environment along with the rest of the action's inputs. A relative path resolved inside the checkout and a bare name went through PATH, which is exactly the case the README told people to avoid. The path must now be absolute, checked once at setup so a bad one is reported rather than quietly producing nothing. Also fixed a test that passed for the wrong reason: a lone negative index is an inert map key, so it could not tell "answer voided" from "index ignored". It is paired with a valid index now.
Summary / Background
When an approval is checked, the diff is taken twice from the same merge base, once to the head and once to the approved commit, and the hunks appearing in both are subtracted. What is left is what the reviewer has not seen, and if any of it is theirs, the approval is dismissed.
That subtraction compares text, so a hunk which changed for a reason a particular codebase does not care about still counts as unreviewed. Whether that is right depends on the codebase: in one repository a reformat is noise, in another the formatter changing its mind is what somebody needs to look at. Picking one on a repository's behalf means every consumer inherits it, along with the language-awareness it takes to stay correct.
This adds the seam and no policy. The new
hunk-filterinput names a program which receives the hunks still outstanding against an approval, alongside the hunks that approval introduced, and answers which of them the reviewer has effectively already reviewed.Behavior
hunk-filter(default empty, so unset means no change at all): path to an executable. It reads one JSON object on stdin and writes one on stdout, naming already-reviewed hunks by index. Format documented in the new README section.Implementation notes
NewDiffandNewDiffWithExecutorgain a variadic...DiffOptionparameter, so existing call sites keep compiling. Both live ininternal/, so no external consumer can reach them either way.pkg/only gains a new package (pkg/hook); nothing already exported changes.changesSince, which is the only place both the head hunks and the approval-time hunks still exist as text.GITHUB_WORKSPACEis writable by the PR author and is the wrong thing to point at.Code Changes
pkg/hook: wire contract, subprocess runner, response validationinternal/git:DiffOption,WithHunkFilter, filtering inchangesSinceinternal/app: hook wiring and the fail-safe policymain.go/action.yml: thehunk-filterinputREADME.md: "Hunk Filters" sectionTesting
TestChangesSinceWithoutFilterIsUnchanged: with no filter configured the diff is byte-for-byte what it is todayTestHunkFilterDropsOnlyWhatItNames,TestHunkFilterDroppingEveryHunkRemovesTheFile: the narrowing itselfTestHunkFilterFailuresChangeNothing: error, index past the end, negative index, unsent file, empty answerTestHunkFilterOnlySeesSurvivingHunks: already-subtracted hunks are never offered to the filterTestIndexesAcceptsWhatWasSent,TestIndexesRejectsAnswersOutsideTheRequest: response validation, including that index order is deterministicTestRunReadsTheResponse,TestRunFailsLoudly,TestRunMissingHookIsAnError,TestRunHonoursTheDeadline: every runner failure mode, including a hook which outlives its own deadlineTestAppHunkFilterAppliesAValidAnswer,TestAppHunkFilterAnswersNoneOnFailure,TestAppHunkFilterMissingHookAnswersNone: the fail-safe wiring end to endgo test ./...,go vet ./...,golangci-lint run ./...clean