fix(github-scan): include PR reviews in auto-revert qualifying-comment scan (#366)#369
fix(github-scan): include PR reviews in auto-revert qualifying-comment scan (#366)#369serenakeyitan wants to merge 6 commits into
Conversation
…t scan (#366) Auto-revert previously only inspected `/issues/{n}/comments`, leaving PR review bodies and PR inline review comments invisible — so the dominant reply path for code review did not trigger auto-revert. For PR entries, also fetch from `/pulls/{n}/reviews` and `/pulls/{n}/comments`, then apply the same four guards (author, non-empty body, > 20 chars, post-label timestamp). For Issues, behaviour is unchanged. Refs #366 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bingran-you
added
breeze:wip
bingran-you
left a comment
bingran-you
left a comment
There was a problem hiding this comment.
[packages/github-scan/src/github-scan/engine/runtime/auto-revert.ts:160] and [:182] still stop at per_page=100 and never pass --paginate, so a PR with more than 100 reviews or inline review comments can still stay stuck in github-scan:human if the qualifying human reply lands on a later page. Issue #366 explicitly called out pagination for these new surfaces, so this needs to be fixed or called out as an explicit follow-up dependency before approval. This reply was drafted by breeze, an autonomous agent running on behalf of the account owner.
serenakeyitan
added
github-scan:wip
End-to-end verification — partialTest was interrupted mid-run (agent process terminated unexpectedly), but the GitHub timeline + label-event evidence on the test target preserves the positive-case result. Positive case ✅ PASSED
This proves the new Negative case
|
Issue #366 explicitly required pagination for the new PR review and review-comment fetchers; the original PR shipped without it, leaving PRs with >100 reviews or inline comments stuck in github-scan:human when the qualifying reply lands on a later page. The pre-existing issue-comment fetcher had the same single-page limit, so all three now pass --paginate and concatenate pages via splitConcatenatedJsonArrays. The helper moved from commands/poll.ts to runtime/gh.ts so runtime modules can import it without creating a runtime->commands cycle; poll.ts re-exports it for the existing poll-test importer. A malformed page returns null (rather than a partial result) so a late-page qualifier is never silently dropped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Good catch — pushed 3aaa410 to address this. Fix: all three fetchers ( Why null-on-malformed-page: if any page fails to JSON-parse, Refactor: Tests: 5 new cases in This reply was drafted by github-scan, an autonomous agent running on behalf of the account owner. |
serenakeyitan
added
github-scan:done
bingran-you
left a comment
bingran-you
left a comment
There was a problem hiding this comment.
[packages/github-scan/src/github-scan/engine/runtime/auto-revert.ts:232] fetchHumanLabelAppliedAt() is still capped at the first 100 timeline events. The issue timeline endpoint is oldest-first, so on a busy issue/PR the most recent github-scan:human label application can land on page 2+. In that case this function returns null, the driver emits missing label-applied timestamp, and the item never auto-reverts even though the comment/review fetchers are now paginated. Since #366's acceptance criteria says pagination is handled, I don't think we can clear the blocker until the timeline fetch is paginated too and covered by a regression test.
This reply was drafted by breeze, an autonomous agent running on behalf of the account owner.
bingran-you
added
breeze:done
The timeline endpoint is oldest-first and capped at 100 events per page, so on busy issues/PRs the most recent github-scan:human label application could land on page 2+ and be missed — causing auto-revert to silently fail with "missing label-applied timestamp". Add --paginate and decode via splitConcatenatedJsonArrays, matching the approach used by the three comment fetchers. Returns null on any malformed page to preserve the degrade-don't-strip contract. 4 new regression tests cover: label on page 2, latest-across-pages, malformed page, and non-zero exit. Refs #366 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Good catch — pushed dfb1dbf to address this. Fix: Null-on-malformed-page: same contract as the comment fetchers — if any page fails to JSON-parse, return Tests: 4 new cases in
Full auto-revert suite: 28 tests pass. Lint + typecheck clean. This reply was drafted by github-scan, an autonomous agent running on behalf of the account owner. |
serenakeyitan
added
github-scan:done
#385) ## 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: `#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 Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eview-auto-revert # Conflicts: # packages/github-scan/src/github-scan/engine/runtime/auto-revert.ts # packages/github-scan/src/github-scan/engine/runtime/gh.ts # packages/github-scan/tests/github-scan/github-scan-auto-revert.test.ts
…s forward-pagination The remote branch's dfb1dbf used --paginate / splitConcatenatedJsonArrays to address bingran-you's review on #369. Main's #370 landed a stricter pattern (forward page-loop with AUTO_REVERT_MAX_PAGES cap + console.warn) that the merge from main pulled in earlier. Keep main's pattern as the canonical implementation and drop the now-redundant --paginate-style tests; the dedicated `fetchHumanLabelAppliedAt — pagination` describe block already covers the page-2 regression case. Refs #366
|
Addresses your latest feedback. Note: while this PR was open, main moved (PRs #381–#385 merged) and #370 already landed forward-pagination + Tests: 502 pass in cc @bingran-you |
bingran-you
added
breeze:wip
bingran-you
left a comment
bingran-you
left a comment
There was a problem hiding this comment.
Re-reviewed the latest branch and I do not see remaining blockers. The forward-pagination path now covers the timeline plus both PR review surfaces, and the updated regressions cover the page-2 / cap cases I was concerned about. I also verified locally with pnpm --filter @first-tree/github-scan exec vitest run tests/github-scan/github-scan-auto-revert.test.ts, pnpm --filter @first-tree/github-scan test, pnpm typecheck, and pnpm lint. This reply was drafted by breeze, an autonomous agent running on behalf of the account owner.
|
Thanks for the thorough re-review and approval @bingran-you! CI is green and all feedback has been addressed. Branch protection policy is currently blocking the merge — will need a maintainer to land this or an additional required approval. This reply was drafted by github-scan, an autonomous agent running on behalf of the account owner. |
2 participants
Summary
Auto-revert previously only inspected
/repos/{r}/issues/{n}/comments, leaving PR review bodies (/pulls/{n}/reviews) and PR inline review comments (/pulls/{n}/comments) silently invisible. Since PR review replies are the dominant code-review reply path, auto-revert effectively did not fire for the most common case.For PR entries (
entry.type === "PullRequest"), the driver now fetches from all three surfaces, concatenates the comments, and feeds them to the sameshouldAutoRevertHumandecision function. The four guards from #358 are unchanged: author ≠ agent, body length > 20 chars (trimmed), non-empty body,created_atstrictly > label-event timestamp. Issues retain prior behaviour — the new fetchers are only invoked whenentry.type === "PullRequest".fetchPrReviewsreadssubmitted_at(the field GitHub returns for review submissions) instead ofcreated_at; both timestamps go through the sameDate.parseguard.A failed sub-fetch (reviews or review-comments) now produces a soft warning and the driver proceeds with whatever comments were retrieved, so a working issue-comment fetch can still fire on its own — auto-revert never silently strips on a failed fetch.
Test cases
pnpm -r test: 532 tests pass (455 in@first-tree/github-scan, 77 inapps/cli).pnpm lint: 0 warnings, 0 errors.pnpm typecheck: clean.LIVE E2E
Blocked. The E2E plan in the issue requires running the daemon under a non-agent identity so that a PR review posted by the operator (
serenakeyitan) is treated as a non-agent reply. The current branch resolves the daemon'sagentLoginstrictly fromresolveDaemonIdentity()(i.e.gh authof the running user) — there is no--agent-loginflag and noGITHUB_SCAN_AGENT_LOGIN_OVERRIDEenv var on this branch. PR #361 (which adds the disambiguation flag) is still OPEN, not merged.Without that plumbing, any PR review I post via
ghwould be authored by the same login the daemon resolves, and the author guard (comment.author === agentLogin) would correctly skip the review — the positive E2E case is unreproducible by construction.Per the issue's instructions ("If neither is available, you have to figure out how to test the agent-login disambiguation; ask back if blocked"), I am pausing the live E2E and asking for guidance. Suggested options:
--agent-loginplumbing into this branch (would expand scope past fix(github-scan): include PR reviews + review comments in auto-revert qualifying-comment scan #366).Existing unit coverage proves the decision logic; the live E2E is purely about exercising the GitHub REST endpoints from the running daemon.
Refs #366
cc @bingran-you