PR Review Summary

Status: Approved

Cycle-1 reviewers: run §9.0 Premise Pre-Flight BEFORE composing Required Actions. If any structural trigger fires (premise-invalid / upstream-not-graduated / author-bypassed / anti-pattern / strategic-misalignment / better-existing-substrate / source-ticket-stale/currency-risk), default to Drop+Supersede framing — single-item close-recommendation, NOT multi-item iteration list.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Premise Pre-Flight passes cleanly. Operator-surfaced regression (ai:sync-github-workflow failing with 504 Gateway Timeout in PullRequestSyncer.syncPullRequests → GraphqlService.query) maps directly to the bounded-retry shape this PR introduces. KB precedent #9063 (older 502/504 mitigation via backoff + chunking) aligns with the implementation direction. Live GitHub probe (96 pages / 2,879 PRs successful) empirically confirms the symptom is transient — not deterministic — so retry at the query() boundary is the substrate-correct primitive (vs. caller-side or sync-loop-level retry which would compound complexity). CI fully SUCCESS on head a373103d4.

Peer-Review Opening: Thanks for the rapid pivot off the #11577 cleanup lane to ship this. The retry shape is well-bounded, the Retry-After header support + jitter pattern is solid, and the transport-error detection list reads as production-tested. Reactive-config exposure (maxRetryAttempts, retryBaseDelayMs, etc.) gives downstream tunability without re-deploys. Cross-family pairing converged smoothly — A2A handoff at MESSAGE:d2bdffaa was the right read of the orthogonal P0.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #11585
• Related Graph Nodes: #11583 (preceding husky/sync_all P0), KB #9063 (older GitHub GraphQL 502/504 backoff precedent), GraphqlService consumers (PullRequestSyncer, DiscussionSyncer, IssueSyncer, PrComment*Service)

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

Two substantive observations as non-blocking follow-up surfaces:

• Challenge 1 — 5xx coverage scope: retryableHttpStatuses defaults to [429, 502, 503, 504]. GitHub's edge also occasionally returns 500 (transient internal error) on the GraphQL endpoint, which would fall through to fail-fast under the current shape. Defensible scope choice — the empirical anchor is 504 + adjacent gateway statuses — but worth noting for future tuning if 500-class transients appear. The reactive-config exposure makes this trivially adjustable post-merge without code change.

• Challenge 2 — cumulative retry timeout under storm conditions: With maxRetryAttempts=3, retryBaseDelayMs=1000, retryMaxDelayMs=10000, and 2x exponential, worst-case per-call delay is ~15s (1+2+4+8). For a 96-page sync, a sustained-transient-storm could compound to ~24min added latency. Not blocking — sustained transients are improbable and the user-facing alternative (sync abort) is worse — but worth recording for graph-ingestion as a known operational ceiling.

• Documented search: I actively looked for (1) Neo reactive-config convention violations (none — apiUrl / new configs follow the same _export shape), (2) auth-token-override security surface (safe; class-config access requires direct module import, not user-input plumbing), (3) authTokenOverride interaction with token caching (#authToken cache is bypassed when override is set — correct test ergonomics), (4) Retry-After header parsing edge cases (numeric form checked before date form — HTTP spec order preserved), (5) fail-fast on semantic GraphQL errors (preserved unchanged — json.errors check downstream of the retry loop), and (6) live-CI agreement with PR body claims (matches: lint-pr-body / Analyze / check / integration-unified / unit / CodeQL all SUCCESS).

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing accurate. "Bounded retry/backoff for transient HTTP 429/502/503/504 and fetch transport failures" exactly matches the implementation.
• Anchor & Echo: JSDoc precise on each new config + private method.
• [RETROSPECTIVE] tag: N/A — none in PR body.
• Linked anchors: #11585 + KB #9063 citations both validly establish the pattern.

Findings: Pass. Two non-blocking observations recorded above.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A.
• [TOOLING_GAP]: N/A — authTokenOverride cleanly resolves the prior gap where GraphqlService could only be exercised via real gh auth token shell-out.
• [RETROSPECTIVE]: Bounded-retry at the query() boundary is the right substrate level for GitHub GraphQL transient resilience. Caller-side retry (in syncers) would multiply per-method without benefit; sync-loop-level retry would lose granularity on which queries failed. The single-method-with-config-tunability pattern (reactive configs exposing retry policy) makes ops-tuning trivial without code change.

N/A Audits — 🛂 📑 📜 📡 🔌 🔗

N/A across listed dimensions: this is a service-internal bug fix with no new architectural abstraction, no public/consumed runtime contract change (the query() signature is unchanged; retry is opaque to callers), no operator/peer authority citation requiring source-of-authority verification, no MCP OpenAPI surface, no wire-format change (GraphQL request payload is unchanged), no skill/AGENTS surface touched.

🎯 Close-Target Audit

• Close-targets identified: #11585
• For each #N: verified #11585 not epic-labeled (labels: bug, regression, ai, architecture, model-experience, build — no epic).

Findings: Pass.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line — Evidence: L3 (same-shape live GitHub GraphQL pagination probe completed 96 pages / 2,879 PRs after an operator-observed 504; focused unit contracts cover retry/fail-fast behavior) → L3 required. No residuals.
• Achieved evidence ≥ close-target required evidence. The live-probe + focused unit coverage covers both empirical anchor + behavioral invariants. L3 is appropriate (runtime behavioral surface against external GitHub edge).
• Two-ceiling distinction: shipped at L3 because L3 is the substrate-correct ceiling for transient-failure-recovery semantics (not because sandbox couldn't reach further). Post-Merge Validation declares operational verification at L3+: next ai:sync-github-workflow run + scheduled data-sync pipeline pass-through.
• Evidence-class collapse check: review language does not promote L3 to L4 framing.

Findings: Pass.

🧪 Test-Execution & Location Audit

• Branch inspected via gh pr diff 11587 (no checkout needed; diff is contained to 2 files with clean spec).
• Canonical Location: test/playwright/unit/ai/services/github-workflow/GraphqlService.spec.mjs — correct placement per feedback_mcp_test_location (canonical test root for MCP / service tests).
• Test file: NEW spec covering 504 retry success, fetch failure retry success, exhausted retry budget, non-transient 400 fail-fast, semantic GraphQL error fail-fast. AC mapping 1:1 to #11585's retry ACs.
• Test isolation: beforeEach snapshots all retry configs + authTokenOverride + globalThis.fetch; afterEach restores. Cross-test contamination avoided. Per feedback_symmetric_spec_cleanup: symmetric setup/teardown — correct.
• PR body transparency on sandbox failures: 3 PullRequestService.spec failures attributed to sandboxed gh pr view/diff network gap — outside this PR's surface, defensibly documented.

Findings: Pass — tests at canonical location, AC-test mapping complete, isolation symmetric, CI green.

🛡️ CI / Security Checks Audit

• Ran gh pr view 11587 --json statusCheckRollup to empirically verify CI status.
• Confirmed all 6 checks SUCCESS: lint-pr-body, Analyze (javascript), check, integration-unified, unit, CodeQL.
• No checks pending/in-progress. mergeStateStatus: CLEAN per author's pre-routing V-B-A.

Findings: Pass — all checks green.

📋 Required Actions

No required actions — eligible for human merge.

Optional follow-ups (post-merge, file separately if pursued):

• Extend retryableHttpStatuses to include 500 if 500-class transients are observed against GitHub GraphQL (currently defensible to start scope-bounded on the empirical 504-class anchor).
• Surface a metric for cumulative-retry-time on the sync pipeline so the ~24min worst-case storm latency is observable rather than silent.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 - Bounded-retry-at-query-boundary is the substrate-correct level (vs caller-side or sync-loop-level). Reactive-config exposure follows Neo conventions exactly. 5-point reservation for the 5xx-coverage scope observation in Depth Floor.
• [CONTENT_COMPLETENESS]: 100 - PR body has Evidence declaration, Deltas-from-ticket, Test Evidence with sandbox-failure transparency, Post-Merge Validation. Maps 1:1 to #11585 ACs.
• [EXECUTION_QUALITY]: 100 - 6 CI checks SUCCESS; 5 focused tests at canonical location with symmetric setup/teardown; live-probe L3 evidence; AC-test mapping complete.
• [PRODUCTIVITY]: 100 - Single-cycle approval-eligible fix. Cross-family pivot off #11577 cleanup lane absorbed cleanly via A2A handoff at MESSAGE:d2bdffaa.
• [IMPACT]: 80 - Restores GitHub workflow sync resilience; foundational infrastructure for swarm ticket/PR corpus refresh. High operational value but bounded to one service surface.
• [COMPLEXITY]: 50 - Moderate: 318 additions across one service + new test spec; retry-loop logic is well-bounded but introduces 5 new private methods + 6 reactive configs.
• [EFFORT_PROFILE]: Quick Win - Substrate-resilience fix with clear behavioral surface + immediate sync-pipeline unblock value.

Ship it. The two non-blocking observations (5xx coverage, cumulative-retry telemetry) are tickets-for-later, not gates on this merge.