fix: stop the reviewer re-triggering itself on its own replies [OSWE-267] - #144
Conversation
…267]
Every re-review on mastra-pilot concluded its check as failure ("Review
did not complete") ~45s in, then a replacement run succeeded without ever
re-concluding the check. First-review-per-PR always worked.
The reviewer was answering itself. This deployment posts as
`openswebot[bot]`, but the finding-reply guard hard-coded
`sender_login == "open-swe[bot]"`, so every reply the reviewer wrote to
its own finding thread came back through the webhook as a *human* reply.
That path settles the in-flight check and dispatches a run with
`multitask_strategy="interrupt"` — which killed the review still
publishing, which replied again. The whole "conclude then requeue race"
is two sequential statements in one webhook handler, which is why it
reproduced on quiet heads with zero load.
- Match every login we post as. `GITHUB_BOT_LOGINS` adds the deployment's
own login(s) to the built-in set, read from the env rather than baked
in, and matching is case-insensitive across both the bare slug and the
`[bot]` form. Applied to the finding-reply guard, the public-repo org
gate, and finding reconciliation, which shared the same assumption.
- A preemption is not a review failure. Superseded checks now conclude
`neutral` regardless of `REVIEW_CHECK_BLOCKING`, in both the
finding-reply handoff and the run-completion handler. Genuine crashes
keep failing a blocking check, so the merge gate is unchanged.
- A run that preempted a review re-concludes on the head SHA. The
handoff records the check it closed and the successor reports the real
result there, instead of leaving the head with no full-review check.
Fixes the failure half introduced by 1dea596 (shipped 2026-08-05); the
self-trigger predates it.
![openswebot[bot]](https://avatars.githubusercontent.com/in/4340220?s=60&v=4){kind=small}
…-267] Review of the first pass found three real defects, all in the check handling rather than the self-trigger fix: - Matching the bare app slug in `is_internal_bot_login` was a privilege bug. REST reports `name[bot]` for apps and the bare login for users, and GitHub lets both exist — `open-swe` is a User, `open-swe[bot]` is the App — so a human owning an app's slug was handed the PUBLIC_REPO_ORG_GATE bypass. Sender matching is now exact; the bare form is accepted only by `is_internal_bot_author`, for GraphQL comment authors, where it decides comment ownership and nothing else. - Concluding the preempted check as neutral freed the gate: `neutral` is non-blocking here, and the finding-reply path opens no replacement in-progress check, so a blocking PR was mergeable for the whole successor run — permanently if the successor never published. The check is no longer concluded at all. It stays in progress and is handed to the successor, which keeps the gate closed throughout, and the after-agent hook still settles it if that run dies. - A finding reply reassesses one thread and never reads the diff, so it must not answer for a review it interrupted. It now concludes the inherited check with the real result only when a full review of this head completed; otherwise the honest incomplete result stands. Also: the handoff marker is no longer consumed before the settle succeeds — it is matched against the check the thread tracks, so it lapses on its own once that check is settled or replaced, and a failed settle stays retryable. And `GITHUB_BOT_LOGINS` is documented in INSTALLATION.md, without which a custom app slug reintroduces the loop.
|
Reviewed all six findings. Three were real defects and are fixed in 3fccdc2; the other three follow from them. 🔴 bare-slug matching ( 🔴 neutral supersede frees the gate ( 🔴 finding reply concluding the full-review check ( 🟠 marker deleted before the settle succeeded ( 🟡 undocumented Full suite green (2511) and lint clean. |
Leaving the preempted check in progress moved the risk onto the run that inherits it, and review found two ways it could be stranded or stolen. - The after-agent hook only runs when the graph exits normally. A run killed by a platform timeout or a hard error skipped it, and the completion handler could not help because the finding-reply dispatch passed `_AGENT_VERSION_METADATA` with no check id. With blocking on, that stranded the check in progress and left the PR unmergeable until someone started another review by hand. The handoff now returns the check it handed over and the dispatch records it on the run, which is what `_settle_failed_reviewer_check` reads. - Two replies arriving in quick succession left both runs seeing the same marker, so the first could settle the check on its way out and leave the run that actually owns the handoff with nothing to report. Claiming now also requires being the thread's current reviewer run.
|
Second round reviewed. Two of the three are real and fixed in 52ee0e3; the 🔴 is a false positive against the current diff. 🟠 🟡 🔴 Full suite green (2513) and lint clean. Aside worth recording: this PR's own re-review reproduced the bug live at 13:15, which is about as direct a confirmation as the root-cause analysis could get. The reviewer posted five replies to its own finding threads at 13:15:35 (comments 3740734776-83, all |
|
Resolved the three remaining review threads: the 🟠 ( |
1 participant
The bug
Every re-triggered review on
mobilyze-llc/mastra-pilot(#23, #24) concluded its check as failure — "Review did not complete" ~45 s in, then a replacement run succeeded 30 s later and never re-concluded the check, leaving a red gate on a green PR. First-review-per-PR always worked. Reproduced ≥7 times, including on quiet heads with zero fleet load and a freshly restarted API.The reviewer was answering itself. This deployment posts as
openswebot[bot], but the finding-reply guard hard-codedsender_login == "open-swe[bot]":So every reply the reviewer wrote to its own finding thread came back through the webhook as a human reply (
"kind": "human_reply",needs_reassessment: True). That path settles the in-flight check and dispatches a run withmultitask_strategy="interrupt", which killed the review still publishing — and that run replied again.One webhook handler, from the logs:
The reported "conclude-then-requeue race" is not a race: those are two sequential statements in one handler, which is why load made no difference. Why first reviews work: a first review has no existing finding threads to reply to, so it never emits the comment that triggers itself.
What changed
1. Match every login we post as.
GITHUB_BOT_LOGINSadds the deployment's own login(s) to the built-in set, read from the env rather than baked in, matched case-insensitively across both the bare slug (GraphQL authors) and the[bot]form (REST senders). Applied to the finding-reply guard, the public-repo org gate, and finding reconciliation — all three shared the same assumption that the bot is namedopen-swe[bot].2. A preemption is not a review failure. New
superseded_review_check_result()concludesneutralregardless ofREVIEW_CHECK_BLOCKING, used by the finding-reply handoff and by the run-completion handler when a newer reviewer run already owns the thread. Genuine crashes still fail a blocking check viaincomplete_review_check_result(), so the merge gate is not weakened — only infrastructure preemption stops being reported as a failed review.3. The superseding run re-concludes the HEAD check.
settle_review_check_runclearsreview_check_run_id, so the successor previously had nothing to update andis_finding_replyblocked it anyway. The handoff now records the check it closed; the successor consumes that marker and reports the real result on the head SHA (creating the check, since the old id is completed). The marker is only written when the PATCH actually succeeded, so a failed settle still goes through the existing retry paths instead of opening a second check. A finding reply that preempted nothing keeps today's behavior, and never advanceslast_reviewed_sha.On the regression-window hypothesis in OSWE-267
The ticket pinned this on control-plane image
5bf5f962(deployed 2026-08-07 23:09 ET). That is wrong, and the evidence is in this repo:git diff 625054fc..5bf5f962touches three files, +81/−0 —canonicalize_repo_configand its test. Zero reviewer, check-run, webhook-handler, or pool changes. The GitHub webhook path is byte-identical across the window. The 23:09 deploy was a config change (addingGITHUB_REPO_ALIASESfor the mastra-pilot transfer).1dea596e(2026-08-04), first shipped in the Aug 5 01:35 ET image — ~46 h earlier. The self-trigger is older still (209132d3, 2026-06-26).openswebot[bot]with zero human replies, and the loop is plainly visible on 2026-08-06 (three parents replied to at 07:23:14, the same three again at 07:23:47). Those reviews were substantially the loop generating its own runs.The reported wedged DB pool (
requests_queued=145flat) was also a misread: it climbs monotonically 1 → 145 over 8 h and resets on restart — a cumulative counter, not a gauge — whilepool_available == pool_sizeon every sample, i.e. zero connections in use. No wedge; no pool change here. That metric is emitted bylanggraph_api, not this repo.Testing
make lintclean; full unit suite green (2509 passed). New coverage:tests/utils/test_internal_bot_logins.py— identity matching, incl. thatopenswebot[bot]matches only when configured, and that third-party bots and humans never do.test_finding_reply_ignores_replies_authored_by_our_own_bot— the loop's regression test.test_finding_reply_never_settles_failure_under_blocking,test_preempted_reviewer_settles_neutral_even_when_blocking.test_finding_reply_publish_reconcludes_check_it_superseded.test_finding_reply_ignores_pending_result_from_superseded_checkfor the neutral conclusion; its original intent (a pending result belonging to a different check can't speak for this one) is preserved.Deploy note: set
GITHUB_BOT_LOGINS=openswebot[bot]in the control-plane env alongside this image — without it the guard still misses and the loop continues.Unrelated pre-existing flake observed:
tests/api/test_logging_redaction.py::test_asgi_access_log_query_string_is_redacted_and_fields_are_preserved[code]assertsoutput.count("401") == 2against a timestamped log line, so it fails whenever the timestamp happens to contain401(~1 run in 3). Not touched here.Refs OSWE-267, OSWE-265, OSWE-264.