Skip to content

fix(relay): hold GitHub authorization and run reports across a CP reconnect - #1067

Merged
zfy0701 merged 2 commits into
mainfrom
fix/relay-authz-across-cp-reconnect
Aug 16, 2026
Merged

fix(relay): hold GitHub authorization and run reports across a CP reconnect#1067
zfy0701 merged 2 commits into
mainfrom
fix/relay-authz-across-cp-reconnect

Conversation

@zfy0701

@zfy0701 zfy0701 commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Summary

A GitHub PR-lifecycle delivery that lands while the relay's CP link is down disappears without a trace: no HookRun row, no Check, nothing on the PR — and GitHub never retries a delivery it considers served.

Two independent paths fail on the same dead link:

  • pull_request:opened / synchronize are gated on a live maintainer-permission lookup (author_association is deliberately not trusted, fix(github): require maintainer authorization for triggers #319), and authorizeGithubComment throws the moment the link is not READY. The ingress fails closed → pullRequestNeedsMaintainer → the review is skipped, for the repository owner's own PR as much as anyone's.
  • The compensating "review request required" informational Check is reported through emitRunReport on that same link, which dropped it.

Observed in practice: a rolling restart terminated the CP a few seconds before a PR was opened, and that PR simply never got a review — while the deliveries ~20s either side of it were processed normally.

Fix

  • authorizationRequest() — one seam for both GitHub authorization RPCs (rc/github-comment-authz, rc/github-rerequest). It waits up to 30s for the link to register, and re-issues the request exactly once when the wire failure was retryable (i.e. the link itself died mid-flight). A settled error REP stays terminal, so a refusal or an old CP still fails closed on the first answer, and no request is ever retransmitted on the same connection. Both call sites are already off GitHub's HTTP request (void dispatch…), so the wait costs nothing but a pending promise. The post-await currentAuthorizedRule re-read already fences a rule that changed during the wait.
  • emitRunReport queues instead of dropping, and the queue replays in order on READY (bounded at 200, oldest dropped first). A delivery refused pre-dispatch never reaches a daemon, so no late hook/report converges its row — that report is the only trace it will ever get. firedAt travels with the report, so a replayed row keeps its real ingest time.

Test plan

  • relay-cp-client.test.ts (new): authorization rides out a restart instead of failing closed; a request the dying link swallowed is re-issued exactly once on the new link; it still fails closed when the link stays down for the whole window; run reports replay oldest-first with firedAt intact; the queue bound drops the oldest.
  • Existing single-shot semantics preserved — the "old CP error" case still rejects on the first answer with one frame sent.
  • pnpm --filter @agentconnect.md/relay test (494 passed), typecheck, eslint, prettier.

🤖 Generated with Claude Code

…onnect

A GitHub PR-lifecycle delivery that lands while the relay's CP link is down
disappears without a trace. The live maintainer-permission lookup is the gate
for `pull_request:opened`/`synchronize`, it fails closed the moment the link
is not READY, and the compensating "review request required" report is emitted
on that same dead link and dropped. No HookRun row, no Check, nothing on the
PR — and GitHub never retries a delivery it considers served.

- `authorizationRequest()` is the one seam both GitHub authorization RPCs go
  through: it waits up to 30s for the link to register, and re-issues a request
  exactly once when the wire failure was retryable (the link itself). A settled
  error REP stays terminal, so a refusal or an old CP still fails closed on the
  first answer. Both callers are already off GitHub's HTTP request, so the wait
  costs nothing but a pending promise.
- `emitRunReport` queues instead of dropping, and the queue replays in order on
  READY (bounded at 200, oldest dropped first). A delivery refused pre-dispatch
  never reaches a daemon, so no late `hook/report` converges its row: that
  report is the only trace it will ever get. `firedAt` rides along, so a
  replayed row keeps its real ingest time.

Tests: authorization rides out a restart, re-issues a swallowed request exactly
once, still fails closed when the link stays down for the whole window; run
reports replay oldest-first and the queue bound drops the oldest.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@agentconnect-md-test agentconnect-md-test Bot 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.

Found one blocking correctness issue in the new authorization retry seam. The retry decision currently treats every retryable WireError as proof that the relay↔Control Plane link died, but retryable errors also arise while that same connection remains READY. This can duplicate the upstream GitHub authorization lookup and breaks the stated single-shot-per-link / settled-error-terminal behavior.

I verified the local checkout is the trusted synthetic merge of base 52e7754a2e672a4c464709e27d86af10f8b8bcf0 and head c0a63a58c43d5f3bab923cace56c6baf8a4ee45f, and verified both changed files match that merge tree. I could not rerun the Vitest suite because this isolated checkout has no installed dependencies; Git-based checks are also unavailable because the sandbox denies /dev/null.

sent by review-bot (Codex · gpt-5.6-sol) · open in session

Comment thread packages/relay/src/relay-cp-client.ts
…ot a flag

`err.retryable` is not proof the link died: `ReqRep` marks its single-shot ack
timeout retryable, and the CP answers retryable `error` REPs for its own
GitHub/DB blips while the connection stays READY. Both would have reached the
second attempt over that same live link and duplicated the upstream permission
lookup — the opposite of the single-shot-per-link contract.

A registration counter now identifies which connection a request rode. The
retry waits for a replacement and gives up on the original error when the
generation is unchanged, so only a request the CP demonstrably never answered
on a now-dead connection is re-issued.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@agentconnect-md-test agentconnect-md-test Bot 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.

The previous blocking finding is resolved on 19dfdb3c2056d1842bebc765a4734cd8b828e669. Authorization requests now record the registration generation they used and permit the second attempt only after a replacement connection reaches READY. A retryable Control Plane error and a same-link ACK timeout therefore remain terminal after one request, while an in-flight request rejected by an observed disconnect is retried once on the newly registered link.

I also rechecked the complete base-to-head report-queue change: replay remains FIFO, retains firedAt, and is bounded at 200 entries. I found no blocking correctness issues.

Verification: the checkout is exactly the trusted head, and both changed files match its Git tree. I reviewed the complete PR patch and the fix delta and checked the changed files for trailing whitespace. I could not rerun Vitest/typecheck because this isolated checkout has no installed dependencies; Git-based checks are unavailable because the sandbox denies /dev/null.

sent by review-bot (Codex · gpt-5.6-sol) · open in session

@zfy0701
zfy0701 merged commit 42cd61c into main Aug 16, 2026
18 of 19 checks passed
@zfy0701
zfy0701 deleted the fix/relay-authz-across-cp-reconnect branch August 16, 2026 03:32
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