feat: fetch a force-pushed approval commit instead of dismissing blind - #185
feat: fetch a force-pushed approval commit instead of dismissing blind#185asyncawaitpromise wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces an 'Approval Retention' feature, allowing configuration to retain existing approvals across specific types of changes (e.g., whitespace, comments, formatting, string literals, renames) and enabling the fetching of orphaned approval commits from the remote repository. The implementation includes updates to the configuration structure, logic to handle git diff options, and comprehensive testing for both the configuration and the new git fetching capabilities. My feedback addresses an issue in the error handling of the fetchRef function, where discarding the command output makes debugging git failures difficult; I have provided a suggestion to include the command output in the returned error.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| internal/git/diff.go | Adds guarded, timeout-bounded orphaned-commit fetching and historical-diff retry behavior while preserving original errors. |
| internal/app/app.go | Enables orphaned-approval recovery only when the repository configuration explicitly requests it. |
| internal/config/config.go | Adds the opt-in fetch_orphaned_approval TOML setting with the zero-value default remaining disabled. |
| internal/git/diff_test.go | Covers successful recovery, disabled behavior, unrelated diff failures, fetch and retry errors, argument ordering, and timeout use. |
| internal/app/orphaned_approval_test.go | Exercises approval dismissal and recovery against a real repository where the approval commit exists only on the remote. |
| README.md | Documents the new opt-in network behavior, default, purpose, and timeout. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Compute changes since approval] --> B{Historical diff succeeds?}
B -->|Yes| C[Evaluate later owned changes]
B -->|No| D{Recovery enabled?}
D -->|No| E[Dismiss approval as unsafe]
D -->|Yes| F{Approval commit resolves locally?}
F -->|Yes| E
F -->|No| G[Fetch commit from origin with timeout]
G -->|Failure| E
G -->|Success| H[Retry historical diff]
H -->|Failure| E
H -->|Success| C
Reviews (2): Last reviewed commit: "feat: recover an approval whose commit n..." | Re-trigger Greptile
b313dc9 to
86cc504
Compare
626ea8c to
56807e0
Compare
56807e0 to
eeee60e
Compare
21d8f62 to
5e49d56
Compare
When a branch is rebased or force-pushed, the commit an approval points at stops being reachable from any local ref. ChangesSince then fails on `git diff base...<approvalSHA>`, and the approval lands in badApprovals with no ownership check at all. That dismissal is not "your files changed", it is "I could not tell, so I reset you". GitHub still serves the orphaned object. With `fetch_orphaned_approval` on, a ref which cannot be resolved locally is fetched from origin once and the diff retried, so the approval is judged on its diff rather than lost to a rewritten branch. Fail-safe: if the fetch or the retry fails, the original diff error stays the cause and the approval is dismissed exactly as before. Hardening: - The ref is probed with `cat-file` first, so a diff which failed for some other reason does not spend a remote round trip that cannot help. - The fetch is bounded at 60s. It is the only git call here that waits on a remote; every other one is local and returns promptly. - The ref is passed after a `--` so a ref beginning with a dash cannot be read as an option. `git fetch` accepts --upload-pack, which names a command to run. - The original diff error is preserved as the wrapped cause, with any fetch or retry failure appended. Opt-in only, and default off so that enabling it is always a deliberate choice to add network calls to a run. Coverage badge regenerated.
5e49d56 to
5a26683
Compare
|
Codeowners approval required for this PR: |
Review catch: fetchRef discarded CombinedOutput and returned only the error, so a failed fetch reached the caller as a bare "exit status 128" with nothing about why. getGitDiff already wraps its output, so this was inconsistent with the rest of the file. The scripted test executor had the same bug, returning nil output alongside an error, which is why nothing caught this. It now returns both, the way CombinedOutput does.
Review catch, and the same defect the hunk-filter runner already guards against. exec.CommandContext SIGKILLs git, but `git fetch` spawns git-remote-https, which inherits the CombinedOutput pipe, and Wait blocks until every writer closes. So against an unresponsive remote the call sat there long past the deadline while the error claimed it had timed out. cmd.WaitDelay bounds the wait for the helper to go away. The ref is also now required to look like an object name before it reaches the network. `--` already stopped a dash-leading ref being read as an option, but it does not stop refspec parsing, and `refs/heads/main:refs/heads/injected` created a local ref. The only caller passes a GitHub-issued SHA, so this is defence in depth, and it closes the empty-ref case for free: git reads an empty ref as HEAD, which would have diffed base...HEAD and let an approval pass ownership on the wrong comparison. Test refs are object names now, since that is what production passes, plus rows for a refspec-shaped ref and an empty one. README: `fetch_orphaned_approval` was described as the only setting that adds a network call, which is wrong because self_approval_via_teams fetches team members, and the 60s bound was not true until the WaitDelay fix above.
Summary / Background
When a branch is rebased or force-pushed, the commit an approval points at stops
being reachable from any local ref.
ChangesSincethen fails ongit diff base...<approvalSHA>, and the approval lands inbadApprovalswithno ownership check at all.
That dismissal is not "your files changed". It is "I could not tell, so I reset
you".
The fix
GitHub still serves the orphaned object. With
fetch_orphaned_approvalon, a refwhich cannot be resolved locally is fetched from
originonce and the diffretried, so the approval is judged on its diff rather than lost to a rewritten
branch.
Fail-safe
If the fetch or the retry fails, the original diff error stays the cause and the
approval is dismissed exactly as before. Nothing gets less safe.
Hardening
remote; every other one is local and returns promptly.
--so a ref beginning with a dash cannot be read asan option.
git fetchaccepts--upload-pack, which names a command to run.retry failure appended.
Flag
Opt-in, default off. It is the only setting in
codeowners.tomlwhich adds anetwork call to a run, so enabling it should always be a deliberate choice rather
than something inherited.
Verification
An end-to-end test builds a repository whose approved commit exists only on the
remote, asserts the commit genuinely does not resolve locally (otherwise both
cases would pass for the wrong reason), and checks that the approval is dismissed
with the flag off and recovered with it on.