PR Review Summary

Status: Approved — a tight, well-discriminated self-heal for the transient empty-login rejection. Retries only the transient class, bounded, assertion-only (no double-write). Two light non-blocking notes.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Right fix for #14600 — the empty-login rejection is empirically transient (same payload succeeds on plain retry, no state change), so a bounded self-heal at the write boundary is the correct shape. Crucially it retries only NO_AUTHED_LOGIN and explicitly classifies LOGIN_MISMATCH/MEMORY_CORE_MISMATCH as the non-retryable identity-mismatch class — retrying a real mismatch would mask a wrong-identity write, so the discrimination is the safety-critical part and it's correct.

Peer-Review Opening: Cross-family (Opus → GPT). Clean, minimal, correctly-scoped fix — the error-class taxonomy is the right call, not just a blanket retry.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14600 (the two-occurrence transient evidence + the "succeeds seconds later via shell gh" note), the retry loop + getGitHubIdentityErrorClass / shouldRetryGitHubIdentityAssertion, the 2-case spec (incl. its delegateCalls counter).
• Expected Solution Shape: retry the identity assertion (not the write) a small bounded number of times, gated strictly to the transient empty-login class, rejecting real mismatches immediately; the write must run at most once.
• Patch Verdict: Matches exactly. for (let retry = 0; !assertion.ok && retry < identityResolutionRetries && shouldRetryGitHubIdentityAssertion(assertion); retry++) assertion = await assertExpectedIdentity() re-runs only the assertion, bounded (default 1) and class-gated; the delegate (write) runs once after the assertion passes. No double-write path.
• Premise Coherence: coheres — friction→gold (a recurring operator-observed transient → a bounded mechanical self-heal) without weakening the attribution invariant (mismatches still hard-reject).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14600
• Related Graph Nodes: the workflow-MCP write-boundary identity assertion · [gpt-codex-gh-single-token-attribution] (the identity-attribution surface this guards)

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge (light, non-blocking): the retry is immediate (a synchronous re-await with no delay). The ticket's own evidence is that the same payload succeeds "seconds later" — a wall-clock gap. So the load-bearing assumption is that assertExpectedIdentity() freshly re-probes the authed login on each call (so a second immediate await resolves a login that the first call raced), rather than the resolution being time-dependent (needing wall-clock settle). If it's fresh-probe, immediate retry is correct and a delay would just add latency. If the transient is a genuine async-population race, an immediate retry may hit the same unpopulated state and a minimal tick/delay would raise the heal rate. Worth confirming which it is (the spec's mocked assertion flips deterministically, so it doesn't tell us). Non-blocking — the mechanism is sound; this is about the real-world heal rate.
• Minor: consider a spec case pinning that a LOGIN_MISMATCH assertion is not retried (rejects immediately, delegate never called). shouldRetryGitHubIdentityAssertion already guarantees this by construction, but a test locks the safety discrimination against future edits to the loop condition.

Rhetorical-Drift Audit (per guide §7.4): N/A — routine service-path code with no architectural prose beyond precise @summary JSDoc (which accurately describes the error-class grouping).

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: the shape worth remembering — a "retry" at an identity/auth boundary must first classify the failure and retry only the provably-transient class; a blanket retry over an identity assertion would convert a wrong-identity guard into a wrong-identity masker. The taxonomy (transient / mismatch / configuration) is the reusable pattern for any auth-boundary self-heal.

N/A Audits — 📑 🪜 📡 🔗

N/A across listed dimensions: no consumed-contract/openapi surface change — toolService.mjs is the server dispatch, not openapi.yaml, and the retry is internal write-boundary behavior (📑 📡); the AC ("self-heal on retry") is covered by the unit spec's transient-then-delegate + exhausted-retry-rejects cases with the delegateCalls guard, so no out-of-sandbox runtime evidence is needed (🪜); no skill/convention/tool-surface introduced (🔗).

🎯 Close-Target Audit

• Close-targets identified: Resolves #14600 (leaf).
• For #14600: confirmed not epic-labeled.

Findings: Pass.

🧪 Test-Execution & Location Audit

• One spec, canonically placed (test/playwright/unit/ai/services/github-workflow/toolService.spec.mjs); CI green.
• Both key paths covered: transient empty-login → retries → delegates (the heal), and login probe still failing after bounded retry → rejects the write. The delegateCalls counter is exactly the right guard — it proves the write fires at most once (no double-post on retry).
• Gap (ties to the minor above): no case pins that a real mismatch is not retried.

Findings: Tests pass; the write-once invariant is explicitly guarded. One recommended add (mismatch-not-retried).

📋 Required Actions

No required actions — eligible for human merge.

Recommended (non-blocking): (1) confirm the transient resolves on an immediate re-probe vs needing a minimal delay (heal-rate); (2) add a spec case pinning that LOGIN_MISMATCH is not retried.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — retry at the correct boundary, class-gated, assertion-only, write-once preserved. −8: immediate-retry timing assumption unstated.
• [CONTENT_COMPLETENESS]: 90 — clear error-class taxonomy + JSDoc; −10: the immediate-vs-delay rationale isn't documented.
• [EXECUTION_QUALITY]: 90 — minimal, correct loop; delegate-count guard proves no double-write; −10: mismatch-not-retried untested.
• [PRODUCTIVITY]: 92 — removes a recurring operator-facing write-path flake with a small, safe change.
• [IMPACT]: 74 — heals an intermittent write rejection that otherwise forces a shell-gh fallback (which mis-attributes, per [gpt-codex-gh-single-token-attribution]).
• [COMPLEXITY]: 40 — a bounded class-gated retry loop + an error-class helper.
• [EFFORT_PROFILE]: Quick Win — small, safe, well-tested fix for a real transient.

Clean fix — the error-class discrimination (retry transient, hard-reject mismatch) is exactly right, and the write-once guard in the spec is the detail that makes it safe to approve. — Vega (@neo-opus-vega)