PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: A retry added to a fail-closed security guard is the shape I came in most prepared to reject — §5.1 names "an unnecessary retry loop" as the canonical suspect pattern. It survives on the merits: the retry is confined to the fetch helper, the fail-closed contract and exit code are byte-identical, and the loop is structurally incapable of falling through to a certify path. All five ACs are met as written. My one finding is a retryable-set omission that fails in the safe direction (false red, never false green) and is a one-element predicate change — polish, not a return cycle.

Peer-Review Opening: Emmy, the thing that makes this approvable is what you did not touch. The Promise.all structure, the throw-on-unread contract, main's catch and exit 2, and the operator-facing message prefix are all unchanged; the retry lives entirely inside one leg's fetch. And the seams (fetchImpl, sleep, retryDelays) are what let the fixtures prove retry classification without real timers — that is the design decision the whole test suite rests on.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #16075 in full (including its explicit "That contract is correct and must not be weakened… This ticket is not 'make it lenient'"); the changed-file list; origin/dev's check-codeql-extraction.mjs verdict/exit paths; and the repo's existing GitHub-retry authority ai/services/github-workflow/GraphqlService.mjs:80-83. Also my own prior exposure to this exact race — PR #16070 was the observed instance, and I resolved it then by re-running the failed leg, which is the reflex this PR exists to remove.
• Expected Solution Shape: Bounded retry on availability-class statuses only, inside the log fetch, with the terminal contract untouched — and critically, no reachable path where retry exhaustion reaches the certify branch. The boundary this must not hardcode: the retryable status set and the delay schedule should be data, not literals buried in a conditional. Test isolation: injected fetch/sleep seams so retry classification is provable without wall-clock, plus an assertion that terminal statuses never reach the retry (absence of side effect, not just presence of error).
• Patch Verdict: Matches. The evidence that settled it is structural rather than textual: for (let attempt = 1; ; attempt++) has no exit condition — only return on logRes.ok and throw on terminal-or-exhausted. There is no fallthrough, so the retry cannot degrade into a silent pass no matter how the delays are configured. main's catch still emits the identical could not read an Analyze-leg log prefix and process.exit(2) (:177-178). The exhaustion message appends after N attempts over Mms only when the final status was retryable, which is exactly the ticket's "we waited and it never appeared" versus "we asked once at a bad moment" distinction, implemented rather than paraphrased.
• Premise Coherence: Coheres with verify-before-assert in an unusually literal way — the guard exists to refuse certification from an unread surface, and this change makes it better at reading without making it worse at refusing. Also friction→gold with a sharp edge: the ticket names the real cost as behavioural, not mechanical ("trains readers to re-run red checks reflexively, which is exactly how a real drop would get waved through"). Fixing a false red to protect the credibility of true reds is the correct framing, and it is why this is worth more than its diff size suggests.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16075
• Related Graph Nodes: #16070 (observed instance), #16073 (the same guard passing in the same window — the contrast that proved it transient), GraphqlService.retryableHttpStatuses

🔬 Depth Floor

Challenge: The retryable set omits 429, and the repo already has an authority saying it should not.

isRetryable = status => status === 404 || status >= 500 (:121). ai/services/github-workflow/GraphqlService.mjs:83 declares retryableHttpStatuses: [429, 502, 503, 504] for GitHub API traffic. So the repo's own established set treats 429 as retryable, and this guard — same host, same transient class — treats it as terminal.

This is not a reviewer hypothesis about what might happen; it is a divergence from in-repo precedent, which is why I am raising it rather than tagging it as needing V-B-A. Consequence: a rate-limited log fetch fails closed immediately, producing a red check on a healthy PR — precisely the false red this ticket exists to eliminate, reached by a different status code. The guard job sits in a needs: chain behind Analyze in a workflow that makes many other API calls, so rate-limit pressure is plausible rather than exotic.

Two honest qualifiers. First, the sets differ in both directions and yours is deliberately tailored: you include 404 (log-not-yet-available, the whole point here) which GraphqlService omits, so this is not simply "you used the wrong list." Second, the failure direction is safe — 429-as-terminal produces a false red, never a false green, so nothing about the security contract is at risk. That is why this is a recommendation and not a Required Action.

Your call on landing it: adding 429 to :121 is a one-element change and squarely Maintainer Polish, or it becomes a named follow-up. If you keep the sets divergent, a one-line note saying why would help the next reader — two retryable sets for the same host with no recorded reason is the kind of thing that reads as an oversight in six months even when it was a decision.

(Non-blocking observation, not a second finding: 403 is correctly terminal per AC2, but GitHub also signals secondary rate limits as 403 in some cases. Distinguishing permission-403 from rate-limit-403 needs a header check (retry-after / x-ratelimit-remaining: 0) rather than a status check, which is a genuinely bigger change than the 429 one and I am not asking for it here.)

Rhetorical-Drift Audit (per guide §7.4):

• PR description and ticket: framing matches the diff — the ticket promises the fail-closed contract is unchanged and the diff leaves main's catch, message prefix, and exit code byte-identical.
• Anchor & Echo summaries: the new JSDoc on retryDelays states the bound mechanically ("bounding attempts to retryDelays.length + 1 and total wait to the delay sum") rather than describing it loosely. Verified against the loop: 4 attempts, 7000ms ceiling.
• [RETROSPECTIVE] tag: N/A — none introduced.
• Linked anchors: #16070 and #16073 establish exactly what they are cited for (one instance, one same-window contrast).

Findings: Pass. No drift — the prose is more precise than it needed to be.

🧠 Graph Ingestion Notes

• [KB_GAP]: There is no single authority for "which GitHub HTTP statuses are retryable." GraphqlService.retryableHttpStatuses is the closest thing and it is service-local, so a new caller re-derives the set from scratch and can silently diverge — as here. Worth a shared constant the next caller can find, rather than each site reasoning from first principles.
• [RETROSPECTIVE]: The transferable move is that the retry was added without weakening the terminal contract, and the ticket said so explicitly before the code existed. A retry bolted onto a fail-closed guard usually erodes it, because the easiest implementation is "keep going until something works." The shape that avoided it: retry only availability classes, keep the throw on everything else, and let the loop have no exit other than success-or-throw so exhaustion cannot reach the certify branch. The last part is what makes it safe by construction rather than by care.

N/A Audits — 📑 📡 🔗 🪜

N/A across listed dimensions: no public/consumed surface, config leaf, or wire format changes (an internal build-script helper gains injectable seams); no ai/mcp/server/*/openapi.yaml; no skill file, workflow convention, or MCP tool surface; and the close-target ACs are fully provable at unit level with no runtime surface CI cannot reach.

🎯 Close-Target Audit

• Close-targets identified: #16075
• #16075 confirmed not epic-labeled (bug, ai, model-experience)

Findings: Pass. Single newline-isolated leaf target, no Closes / Fixes variants.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 1757fe55f4 — no failing or pending checks at review time, re-verified rather than taken from the PR body.
• Reviewer falsifier: named concern — can retry exhaustion reach the certify branch? Traced the loop's control flow at head: for (;;) with return only on logRes.ok and throw on !isRetryable || retryDelay == null, plus main's unchanged catch → exit 2. Result: no reachable fallthrough; the fail-open I was hunting does not exist. Second probe: repo-wide search for an existing retryable-status authority, which surfaced GraphqlService.mjs:83 and produced the finding above.
• Test location: test/playwright/unit/ai/buildScripts/util/ is canonical for the module path.

Findings: Pass, and the fixture forms are the strong ones. a terminal 403 fails immediately without sleeping, an incomplete Analyze leg is terminal and never fetches its log, and a malformed jobs payload … never enters log-fetch retry all assert absence of the side effect rather than merely presence of an error — which is the only form that proves the terminal path did not quietly retry first. retry exhaustion stays fail-closed and reports attempts plus elapsed window pins the exact operator-facing string (after 3 attempts over 15ms: 503 Service Unavailable), so AC4 cannot regress silently. 429 appears in no fixture, consistent with its absence from the predicate.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 88 - Retry sits in the fetch helper where the transient actually occurs; delays and seams are injected parameters rather than buried literals; the terminal contract, main, and the exit code are untouched. 12 deducted for diverging from the repo's existing retryableHttpStatuses authority without a recorded reason.
• [CONTENT_COMPLETENESS]: 92 - Ticket carries Context / Problem / Architectural Reality / Fix / ACs and pre-empts the weakening risk in its own body. New JSDoc states the attempt and wall-clock bound mechanically. 8 deducted for the unexplained retryable-set divergence, which is a documentation gap more than a code one.
• [EXECUTION_QUALITY]: 88 - Exhaustion cannot reach certify by construction, not by care; the exhaustion message distinguishes waited-and-never-appeared from asked-once-at-a-bad-moment; terminal fixtures assert absence of side effect. 12 deducted for 429-as-terminal reintroducing a false red in the class the ticket set out to remove.
• [PRODUCTIVITY]: 95 - All five ACs met as written, including the two that are easy to skip (the fail-once-then-succeed certify case, and the exhaustion-still-fails-closed guard).
• [IMPACT]: 78 - Modest surface, but the cost it removes is behavioural: a guard that cries wolf trains reflexive re-runs of red checks, and this guard is the one standing between a silently dropped source file and a clean alert gate.
• [COMPLEXITY]: 40 - One bounded loop and a status predicate; five added fixtures. The reader holds one classification rule and one bound.
• [EFFORT_PROFILE]: Quick Win - Small, self-contained, high ROI: it converts a recurring manual re-run into a bounded automatic wait without touching the safety contract.

The part I want to name last is the part that made this easy to approve: exhaustion cannot reach the certify branch because the loop has no exit other than success or throw. That is safety by construction, and it is the difference between a retry that protects a guard and one that hollows it out.

🌿 A guard that cries wolf teaches people to ignore wolves. This one now waits before it cries.