feat: retain approvals across formatting-only hunks - #186
feat: retain approvals across formatting-only hunks#186asyncawaitpromise wants to merge 1 commit into
Conversation
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
Confidence Score: 2/5This PR should not merge until formatting retention stops masking control-flow scope changes and punctuation changes inside string values. The new normalizer can classify substantive hunks as trivial, causing ChangesSince to omit them and preserve approvals that should be dismissed; one path can bypass renewed review of authorization-sensitive scope changes. Files Needing Attention: internal/git/normalize_formatting.go, internal/git/normalize.go, internal/git/diff.go
|
| Filename | Overview |
|---|---|
| internal/git/normalize_formatting.go | Introduces punctuation normalization that can erase control-flow scope and semantic punctuation inside string literals. |
| internal/git/normalize.go | Adds whole-hunk block normalization and triviality classification; its lexical and structural fidelity depends on each normalization step. |
| internal/git/diff.go | Wires normalization into ChangesSince so misclassified hunks directly affect stale-approval detection. |
| internal/app/app.go | Correctly passes protected base-branch approval-retention configuration into diff construction. |
| internal/git/normalize_formatting_test.go | Covers intended formatting transformations but omits scope-changing brace edits and punctuation-only string changes. |
| README.md | Documents block comparison and the intentionally broad formatting behavior, including scope-sensitive transformations. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Base-branch approval retention config] --> B[GitDiff.ChangesSince]
B --> C[Split hunk into added and removed blocks]
C --> D[collapseFormatting]
D --> E{Normalized blocks equal?}
E -->|Yes| F[Omit hunk from approval diff]
E -->|No| G[Report hunk as changed]
F --> H[Existing approval retained]
G --> I[Approval may be dismissed]
Reviews (1): Last reviewed commit: "feat: retain approvals across formatting..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if unicode.IsSpace(r) || strings.ContainsRune(structuralPunctuation, r) { | ||
| pendingSpace = b.Len() > 0 | ||
| continue | ||
| } |
There was a problem hiding this comment.
When formatting retention is enabled, removing non-empty braces lets a contiguous edit move a statement into or out of a conditional or authorization block while producing the same normalized token sequence. ChangesSince then omits the hunk, causing an existing approval to remain valid after a control-flow or authorization-scope change.
How this was verified: Tracing a brace-scope edit through whole-hunk normalization shows that discarded braces leave identical token sequences, which are omitted from the stale-approval diff.
| r, size := utf8.DecodeRuneInString(block[i:]) | ||
| i += size | ||
| if unicode.IsSpace(r) || strings.ContainsRune(structuralPunctuation, r) { | ||
| pendingSpace = b.Len() > 0 | ||
| continue | ||
| } | ||
| writeToken(string(r), r, r) | ||
| } | ||
| return dropTrailingCommas(b.String()) |
There was a problem hiding this comment.
When formatting retention is enabled, collapseFormatting removes braces, semicolons, whitespace, and trailing-comma patterns without recognizing string boundaries. A punctuation-only change to a URL, query, template, command, or other string value can therefore normalize away, causing the existing approval to remain valid even though string_literals is disabled.
Carries the shared normalizer the other content flags are steps on, plus the
first of those flags.
The design decision that matters is block-wise comparison. Everything a hunk
adds is joined into one string and everything it removes into another, and the
two are normalized and compared whole. Line-wise comparison cannot pair up a
statement rewrapped across several lines, because one removed line becomes
three added ones:
- if (!isOpen) return
+ if (!isOpen) {
+ return
+ }
Block-wise, both sides normalize to the same text.
formatting collapses whitespace, braces, semicolons and trailing commas.
Parentheses are deliberately kept: their placement separates a call from a
reference and decides precedence, so dropping them would hide real changes.
A pair of braces closing over nothing is kept for the same reason, since an
empty argument or body says something the code does not say without it.
The engine hooks into the existing hunk loop in changesSince, reading the same
bytes hunkHash already reads, so there are no new git calls.
One file per flag: the engine and its lexical helpers here, each later flag in
its own file, so the flags can be reviewed and enabled independently.
b313dc9 to
86cc504
Compare
2c2af88 to
bfa8b53
Compare
1 participant
Related PR(s)
Depends on #183. Carries the shared normalizer the other content flags are steps on, so it lands first among them. These build on it:
whitespacecommentsstring_literalsSummary / Background
The first content flag, plus the engine the rest plug into.
The design decision that matters: compare blocks, not lines
Everything a hunk adds is joined into one string and everything it removes into another, then both are normalized and compared whole. A line-wise comparison can never pair up a statement rewrapped across several lines, because one removed line becomes three added ones:
Block-wise, both sides normalize to the same text.
What
formattingignoresWhitespace, braces, semicolons and trailing commas.
Parentheses are deliberately kept. Their placement separates a call from a reference (
handlervshandler()) and decides precedence ((a+b)*cvsa+(b*c)), so dropping them would hide real changes behind a formatting flag.An empty pair of braces is kept for the same reason: an empty argument, body or literal says something the code does not say without it.
Where it hooks in
changesSinceininternal/git/diff.go. The hunk loop already filters on!oldHunkHashes[hunkHash(hunk)]; the triviality check is a second condition on the same loop, reading the same byteshunkHashalready reads. No new git calls.File layout
One file per flag — the engine and its lexical helpers here, each later flag in its own file. The flags can then be reviewed and enabled independently, and the only shared edit point is the ordered if-chain in
newNormalizer.That chain is deliberately explicit rather than a registry: step order is load-bearing (comments must come off before the punctuation steps), and an explicit chain states the constraint where a priority number would hide it.