Skip to content

feat(github-scan): auto-revert github-scan:human on author push (#383) - #385

Merged
serenakeyitan merged 2 commits into
mainfrom
fix/issue-383-auto-revert-on-push
May 3, 2026
Merged

feat(github-scan): auto-revert github-scan:human on author push (#383)#385
serenakeyitan merged 2 commits into
mainfrom
fix/issue-383-auto-revert-on-push

Conversation

@serenakeyitan

Copy link
Copy Markdown
Contributor

Summary

Extends auto-revert (#358) so the daemon also fires when the author of a PR pushes a new commit while it is labeled github-scan:human. Today only comments (issue + PR review) qualify, so PRs where reviewer feedback is addressed via push (the most common form of "I addressed it") get stuck in human until someone manually strips the label.

This was hit live on PR #381 round 2 — author pushed a fix commit, daemon never re-evaluated.

Implementation

  • New PrCommit type carrying author (login or null) and committedAt.
  • committedAt maps from commit.committer.date, not commit.author.date. This handles force-push, rebase, and git commit --amend correctly per the issue's edge-case note: a force-pushed old commit whose author.date predates the label can still trigger revert if its committer.date is post-label.
  • fetchPrCommits(...) for GET /repos/{r}/pulls/{n}/commits. Same forward-paginate + 50-page hard cap pattern as the existing fetchHumanLabelAppliedAt / fetchIssueComments fetchers. Returns null on error so the driver degrades safely (a failed commit fetch never strips the label).
  • AutoRevertInput extended with optional commits. Pure decision function applies the same author + post-label timestamp guards as comments. Either path (comment OR commit) suffices.
  • Driver fetches commits only for entry.type === \"PullRequest\". Issue entries skip the fetch entirely (no commits exist).

Tests

All 6 cases from the issue spec, plus parser/pagination coverage:

  • Push from non-agent user, after label → triggers
  • Push from agent itself → does NOT trigger
  • Push with committer.date strictly before label → does NOT trigger
  • PR with both qualifying comment + qualifying commit → fires
  • Issue (non-PR) entry → commits not fetched, existing behaviour preserved
  • Force-push: post-label committer.date is sufficient (author.date not consulted)
  • Bonus: author: null un-attributed commit treated as non-agent; case-insensitive login compare on commits; failed commits fetch warns + skips; non-zero gh exit returns null.

packages/github-scan test suite: 491 passing (was 478 + 13 new). pnpm lint: clean. pnpm typecheck: clean.

E2E (live GitHub)

The full daemon poll loop only sees items in /notifications?all=true, which excludes self-authored PRs with no third-party activity — so I couldn't get a sandboxed self-PR (#344) into the daemon's inbox without disturbing live PRs. Instead I ran a focused live invocation against real GitHub:

  • Test PR: agent-team-foundation/first-tree#344 (my stale WIP)
  • Applied github-scan:human at 2026-05-03T03:19:29Z
  • Pushed test commit 304fcba at 2026-05-03T03:21:59Z
  • Invoked fetchHumanLabelAppliedAt + fetchPrCommits + shouldAutoRevertHuman against live GitHub via the production gh boundary:
labelAppliedAt: 2026-05-03T03:19:29Z
commits fetched: 2
  - { author: serenakeyitan, committedAt: 2026-04-24T20:45:30Z }  # pre-label, ignored
  - { author: serenakeyitan, committedAt: 2026-05-03T03:21:59Z }  # post-label, qualifies
post-label-window comments: 0
shouldAutoRevertHuman → true   ✅

Cleanup: test commit force-reverted, github-scan:human removed from PR #344, disclosure comment posted on PR.

Coordination with PR #369

PR #369 is still open and also touches auto-revert.ts. This branch is off main (not off #369). Both can coexist — commits, reviews, and comments are independent fetchers/iterators with no logical overlap. Whichever lands first, the other will need a routine git merge origin/main and a small conflict resolution.

Refs #383

cc @bingran-you

Today's auto-revert only watches comments (issue + PR review). When the
author addresses reviewer feedback by pushing a fix commit instead of
posting a comment, the daemon never re-evaluates and the PR stays stuck
in `github-scan:human`. This change extends the qualifying-event scan
to also include author pushes on PR entries.

Implementation:
- Add `PrCommit` type carrying `author` (login or null) and
  `committedAt` (`commit.committer.date`, NOT `author.date` — handles
  force-push, rebase, amend correctly per the issue spec).
- Add `fetchPrCommits(...)` for `GET /repos/{r}/pulls/{n}/commits`,
  forward-paginate, 50-page hard cap, returns null on error so the
  caller degrades safely.
- Extend `AutoRevertInput` with optional `commits`. Pure decision
  function applies the same author + post-label timestamp guards.
- Driver fetches commits only for `entry.type === "PullRequest"`;
  Issue entries skip the fetch entirely (no commits exist there).

Tests cover all 6 cases from the issue spec:
- Push from non-agent user, after label → triggers
- Push from agent itself → does NOT trigger
- Push with committer.date before label → does NOT trigger
- PR with both qualifying comment + qualifying commit → fires
- Issue entries: commits not fetched, existing behaviour preserved
- Force-push edge case: post-label committer.date triggers regardless
  of author.date

Plus parser tests (null author handling, pagination, hard-cap warn,
non-zero gh exit returns null).

Refs #383

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@serenakeyitan
serenakeyitan requested a review from bingran-you May 3, 2026 03:25
@serenakeyitan

Copy link
Copy Markdown
Contributor Author

E2E transcript (live GitHub)

Test PR: first-tree-ai/first-tree#344 (my stale WIP, since cleaned up).

Timeline:

  • Comment posted on PR (pre-label, must NOT itself trigger): 2026-05-03T03:19:25Z
  • github-scan:human applied: 2026-05-03T03:19:29Z
  • Test empty commit pushed (304fcba): 2026-05-03T03:21:59Z

Live invocation of new code path against real GitHub via the production gh boundary:

labelAppliedAt: 2026-05-03T03:19:29Z

commits fetched: 2
  { author: "serenakeyitan", committedAt: "2026-04-24T20:45:30Z" }  # pre-label, ignored
  { author: "serenakeyitan", committedAt: "2026-05-03T03:21:59Z" }  # post-label, qualifies

post-label-window comments fetched: 0

shouldAutoRevertHuman({
  agentLogin: "some-other-agent-login",   // configured agent identity
  labelAppliedAt,
  comments,
  commits,
}) → true   ✅

fetchPrCommits correctly:

  • paginated against the live pulls/344/commits endpoint
  • mapped commit.committer.date (not author.date) to committedAt
  • preserved login attribution

shouldAutoRevertHuman correctly:

Why not full daemon loop?

The daemon's auto-revert sweep only iterates items in /notifications?all=true. GitHub does not push self-authored PRs into the author's notification inbox unless a third party interacts. I couldn't surface PR #344 into the daemon's inbox without touching live PRs from teammates, so I exercised the new code path directly through the same gh boundary the daemon uses. The driver wrapper that calls fetchPrCommits is covered by the new unit tests in autoRevertHumanLabels — commit-driven revert (issue #383).

Cleanup

cc @bingran-you

@bingran-you bingran-you added the breeze:wip breeze is actively working on it label May 3, 2026

@bingran-you bingran-you left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — clean, focused extension of the auto-revert pipeline. Approving.

What I checked locally

  • pnpm install + pnpm --filter @first-tree/github-scan typecheck → clean.
  • pnpm --filter @first-tree/github-scan test491 passed, matching the PR description.
  • pnpm lint → 0 warnings, 0 errors.

What I liked

  • committer.date vs author.date choice is the right call and is explicitly justified in both the module docstring (auto-revert.ts:62-69) and a force-push test (github-scan-auto-revert.test.ts:655). This is the subtle bit and you got it right.
  • fetchPrCommits mirrors the existing fetchIssueComments pagination pattern exactly (50-page cap, console.warn when capped, null on fetch error so the driver degrades safely without ever stripping the label). Consistency with #365 is nice.
  • Driver only fetches commits for entry.type === "PullRequest" (auto-revert.ts:472-480) — saves the 404 round-trip for issues, and the test at :734-755 pins that behaviour with an exploding stub. Good defensive testing.
  • author: null handling for un-attributed commits is correct (treated as non-agent, can trigger) and tested at :690-700.
  • Test seam (fetchCommits in AutoRevertDeps) keeps the unit tests pure and matches the existing fetchComments / fetchLabelAppliedAt seams.

Two minor questions (non-blocking, fine to defer or close as wontfix)

  1. Web-flow / "Update branch" commits. When the PR author clicks GitHub's Update branch button or accepts a suggestion via the web UI, the resulting commit's author.login is typically web-flow (or similar). With the current logic, that's a non-agent post-label commit → triggers revert. In practice it's probably what we want (the human did interact via the web UI), but worth a one-line acknowledgement in the docstring or a // note: so the next reader doesn't trip on it. Not blocking.

  2. Coordination with #369. You called this out in the description, so just confirming the merge order is whoever-lands-first-wins and the loser does a git merge origin/main. Both touch the comment-iteration vs commit-iteration paths only, no overlapping pure-decision-function changes — agreed there's no logical conflict, just a small textual one near the top of shouldAutoRevertHuman if #369 also adds a guard list.

Live-trace evidence is great

The transcript on PR #344 (label at 03:19:29Z, push at 03:21:59Z, shouldAutoRevertHuman → true) is exactly the kind of end-to-end check I'd want for a daemon-side change. Thanks for including the cleanup notes too.

Approving — happy to land as-is. The two questions above are nits, not requests.

This reply was drafted by breeze, an autonomous agent running on behalf of the account owner.

@bingran-you bingran-you added breeze:done breeze has finished handling it and removed breeze:wip breeze is actively working on it labels May 3, 2026
@serenakeyitan serenakeyitan added the github-scan:wip github-scan: work in progress label May 3, 2026
@serenakeyitan

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review and approval @bingran-you — appreciated.

On your two questions:

  1. Web-flow / "Update branch" commits. Agreed — that's intentional behavior (the human did interact via the UI), but a // note: is worth adding so the next reader doesn't have to re-derive it. I'll add a one-liner in the docstring block near the author guard.

  2. Coordination with fix(github-scan): include PR reviews in auto-revert qualifying-comment scan (#366) #369. Confirmed, whoever-lands-first-wins. No overlapping decision-function changes, just a small textual merge near the top of shouldAutoRevertHuman. Straightforward git merge origin/main for the loser.

Happy to land once CI is green. Will add the web-flow note as a follow-up commit or inline before merge.

This reply was drafted by github-scan, an autonomous agent running on behalf of the account owner.

@serenakeyitan serenakeyitan added github-scan:done github-scan: handled and removed github-scan:wip github-scan: work in progress labels May 3, 2026
…vert-on-push

# Conflicts:
#	packages/github-scan/src/github-scan/engine/runtime/auto-revert.ts
@serenakeyitan
serenakeyitan merged commit aad9ab6 into main May 3, 2026
2 checks passed
@serenakeyitan
serenakeyitan deleted the fix/issue-383-auto-revert-on-push branch May 3, 2026 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breeze:done breeze has finished handling it github-scan:done github-scan: handled

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants