Skip to content

feat: fetch a force-pushed approval commit instead of dismissing blind - #185

Open
asyncawaitpromise wants to merge 6 commits into
mainfrom
feat/fetch-orphaned-approval
Open

feat: fetch a force-pushed approval commit instead of dismissing blind#185
asyncawaitpromise wants to merge 6 commits into
mainfrom
feat/fetch-orphaned-approval

Conversation

@asyncawaitpromise

@asyncawaitpromise asyncawaitpromise commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Summary / Background

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".

The fix

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. Nothing gets less safe.

Hardening

  • 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.

Flag

Opt-in, default off. It is the only setting in codeowners.toml which adds a
network 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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment thread internal/git/diff.go
@asyncawaitpromise asyncawaitpromise changed the title feat/fetch orphaned approval feat: fetch a force-pushed approval commit instead of dismissing blind Aug 19, 2026
@asyncawaitpromise
asyncawaitpromise changed the base branch from main to feat/approval-retention-config August 19, 2026 08:11
@asyncawaitpromise
asyncawaitpromise marked this pull request as ready for review August 20, 2026 09:19
@greptile-apps

greptile-apps Bot commented Aug 20, 2026

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

The PR appears safe to merge.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Compute approval diff] --> B{Diff succeeds?}
    B -- Yes --> C[Evaluate changed-file ownership]
    B -- No --> D{Recovery enabled and commit-shaped ref absent locally?}
    D -- No --> E[Return original diff error]
    D -- Yes --> F[Fetch commit from origin with timeout]
    F --> G{Fetch succeeds?}
    G -- No --> E
    G -- Yes --> H[Retry approval diff]
    H --> I{Retry succeeds?}
    I -- No --> E
    I -- Yes --> C
Loading

Comment thread internal/git/diff.go Outdated
@asyncawaitpromise
asyncawaitpromise marked this pull request as draft August 21, 2026 21:33
@asyncawaitpromise
asyncawaitpromise force-pushed the feat/approval-retention-config branch from b313dc9 to 86cc504 Compare August 24, 2026 20:59
@asyncawaitpromise
asyncawaitpromise force-pushed the feat/fetch-orphaned-approval branch from 626ea8c to 56807e0 Compare August 24, 2026 20:59
@asyncawaitpromise
asyncawaitpromise force-pushed the feat/fetch-orphaned-approval branch from 56807e0 to eeee60e Compare September 1, 2026 05:11
@asyncawaitpromise
asyncawaitpromise changed the base branch from feat/approval-retention-config to main September 1, 2026 05:11
@asyncawaitpromise
asyncawaitpromise force-pushed the feat/fetch-orphaned-approval branch from 21d8f62 to 5e49d56 Compare September 1, 2026 06:49
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.
@asyncawaitpromise
asyncawaitpromise force-pushed the feat/fetch-orphaned-approval branch from 5e49d56 to 5a26683 Compare September 1, 2026 07:25
@asyncawaitpromise
asyncawaitpromise marked this pull request as ready for review September 1, 2026 07:28
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

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.
main gained GitHub Enterprise support (#199) and the v1.11.0 release (#201)
while this branch was open.

Resolved:
- README coverage badge: regenerated from the merged tree; covbadge.yml fails
  CI on a stale number, so neither side's value would have done.
- orphaned_approval_test.go: gh.NewClient took a fourth argument in #199. This
  branch adds a caller main never saw, so it merged clean and broke the build.
main gained the hunk filters (#200), the urfave/cli bump (#193) and the
codeowners.toml parse fix (#184) while this branch was open.

Resolved:
- README coverage badge: regenerated from the merged tree; covbadge.yml fails
  CI on a stale number, so neither side's value would have done.
- config.go, app.go: this branch and #200 each added a config field and a
  GitDiff option in the same place. Both are kept.
- diff.go: both branches independently added the same functional-options
  pattern and declared DiffOption identically. One copy is kept, in the
  position #200 put it, so the file matches what later branches fork from.
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codeowners approval required for this PR:

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