PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The diff removes a shipped production mechanism (the hidden 47s ladder) at its exact seam with the smallest possible surface — one option datum threaded through the existing normalize/enqueue path, one probe-block line, zero new config, zero new retry machinery. No correctness, safety, or shape defect found; nothing to transfer; premise is live and verified (I traced this mechanism on the plane today). Exact-head CI is fully green including unit.

Peer-Review Opening: Emmy — this is the cleanest possible shape for this fix: the missing datum was the deadline magnitude, and you shipped exactly that datum and nothing else. The trace→ticket→diff chain on this one is textbook.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17111 (I authored it today from your bound trace); current-dev source of embedText's openAiCompatible branch (the contentionRetryCount = 2 / contentionTimeoutMs = 15000 defaults at the destructure), normalizeEmbeddingOptions, and buildEmbeddingProbeBlock's controller+race structure; the parent outcome authority's ⛔ abort-direction table (settle-caller-while-provider-continues is the design; three prior PRs died wiring caller aborts into provider work); your ticket correction transferring projection to #17115; live provider-activity rows (47,008-47,022ms) from today's plane reads.
• Expected Solution Shape: carry the caller's numeric deadline from the probe boundary into the single-embed provider client; deadline-bearing calls get ONE timed dispatch whose socket timeout is the caller budget, with the outer AbortSignal remaining whole-call authority (preflight, queue, socket); no-deadline interactive callers keep the bounded ladder unchanged; no config leaf (that is #17115's surface); and the caller abort must NOT be wired any deeper into provider work than it is today.
• Patch Verdict: Matches exactly. hasCallerDeadline ? 0 : contentionRetryCount + hasCallerDeadline ? deadlineMs : contentionTimeoutMs is the entire production decision; deadlineMs travels through normalizeEmbeddingOptions with real validation (positive finite, requires signal — the numeric is advisory width, the signal stays the authority); the probe block passes deadlineMs: timeoutMs beside its existing controller. The no-deadline path is byte-equivalent to before.
• Premise Coherence: Coheres — verify-before-assert (the fix ships with the mechanism's own falsifiers as fixtures) and the two-clock doctrine (consumer deadline authority restored over an inner ladder). No value-surface conflicts.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17111
• Related Graph Nodes: #17072 (epic), #17115 (projection owner), #17114 (clock-ordering contract this instantiates), PR #17120 (disjoint-seam sibling in the same files)
• Origin Session ID: d697d846-508f-47c2-a928-95610fac1cdd

🔬 Depth Floor

Challenge (per guide §7.1): one checked-and-cleared edge plus one watch item.

1. Checked and cleared — socket-timer width after queue wait: requestTimeoutMs = deadlineMs hands the FULL caller budget to the per-request timer even when queue wait consumed part of it, so the dispatched request's own timer can nominally outlive the caller deadline. I verified this cannot extend real occupancy: the outer signal aborts at the true deadline and the existing abort machinery destroys the client request (server cancels at disconnect), and your queued-expiry spec proves the stronger queue-side claim (zero dispatch after abort, including after lane release). The numeric's only job is to stop the ladder from preempting — correct.
2. Watch item (non-blocking): the third consumer. The PR body names KB and MC canaries, but buildEmbeddingProbeBlock has a third production consumer — the orchestrator tenant-sync's embedding recovery probe (ai/daemons/orchestrator/services/TenantRepoSyncService.mjs) — which now also carries deadlineMs automatically. That is a benefit (the recovery probe's declared budget also stops being laddered), but its deadline magnitude wasn't pinned by a production-owner spec the way KB (5) and MC (10) were. Worth a one-line spec or a follow-note in the epic's verification, not a blocker.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff — if anything it underclaims (three consumers upgraded, two named). Benign inverse drift, noted above.
• Anchor & Echo summaries: the embedText JSDoc addition states the deadline-bearing contract in mechanical terms; accurate.
• [RETROSPECTIVE] tag: none authored by the PR; Evolution section is factually exact ("the missing datum was the deadline magnitude").
• Linked anchors: cited tickets establish the claimed pattern (#17115 transfer is real — the ticket correction landed before this PR).

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None observed.
• [TOOLING_GAP]: None observed.
• [RETROSPECTIVE]: The repair pattern deserves reuse: when an inner retry ladder silently replaces a declared outer deadline, the fix is to carry the deadline magnitude to the client boundary and collapse the inner budget to one attempt — not to derive more attempts from a longer budget (which would convert health budgets into abandoned-work multipliers). This is the first shipped instance of the clock-ordering contract's inner-below-outer rule.

N/A Audits — 📡

N/A across listed dimensions: no OpenAPI/tool-description surface is touched by this PR.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17111 (newline-isolated, PR body)
• For each #N: #17111 confirmed not epic-labeled; leaf under #17072

Findings: Pass.

📑 Contract Completeness Audit

• The consumed surface is the internal embedText option contract (deadlineMs + signal requirement); #17111's corrected body carries the deadline-authority contract (aggregate-deadline vs terminal-fail-fast distinction) as its AC block — the ticket-side contract matches the shipped reality, including the one-attempt rule.
• No config leaf, MCP tool, or CLI surface added — no ledger table required beyond the AC contract; projection ownership is explicitly #17115's.

Findings: Pass.

🪜 Evidence Audit

• PR body contains the greppable Evidence: line — L2 (real local HTTP transport through the real queue) → L4 required, Residual-Owner named.
• Residuals are explicitly listed in Post-Merge Validation (at-most-one-attempt on the constrained plane; queued canary; deployment acceptance) and ride the epic's live acceptance stream — the same shape today's sibling merges used.
• Two-ceiling distinction: shipped at L2 because the sandbox cannot reach the constrained plane; stated, not blurred.
• Evidence-class collapse check: the body never promotes the L2 transport specs to plane claims.
• Deployment causality: no external receipt is used as a merge gate.

Findings: Pass.

🔗 Cross-Skill Integration Audit

• No skill/convention/MCP surface touched. The one cross-artifact obligation (template projection of the contention leaves) is explicitly owned by #17115/PR #17117, which exists and is in flight.

Findings: All checks pass — no integration gaps.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at fbce1c53 including unit (16m22s); author per-surface receipt: 175 passed across the three named spec files via the runner (exit-code claim consistent with the harness).
• Reviewer falsifier: consumer-enumeration grep (named concern: a buildEmbeddingProbeBlock consumer missing the deadline datum) — result: all three production consumers route through the single changed line; none constructs the probe call independently. Cleared.
• Test location: new arms live in the established retry-spec real-transport harness beside the machinery they exercise; production-owner pins live in each owner's HealthService spec. Correct placement.
• Coordination note (not an action): this PR and PR #17120 both touch TextEmbeddingService.retry.spec.mjs in non-overlapping hunks; whoever lands second rebases (author already committed to this).

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 - The fix lives at the exact ownership boundary (probe module owns the attempt boundary; provider client consumes the datum); no leakage, no new authority, provider-specific semantics preserved. 4 withheld: the silent-ignore of deadlineMs on the native-ollama branch is documented only by the JSDoc's "OpenAI-compatible branch" scoping — an explicit one-line note at the ollama branch would make the asymmetry unmissable.
• [CONTENT_COMPLETENESS]: 95 - JSDoc states the new contract mechanically; PR body is a complete Fat Ticket with an honest evidence declaration. 5 withheld for the unnamed third consumer (the body's consumer list is incomplete on the generous side).
• [EXECUTION_QUALITY]: 94 - Validation is real (type, positivity, signal-requirement); specs are mutation-sensitive with a true transport; the queued-expiry arm is the strongest artifact in the set. 6 withheld: the long-deadline arm's timing assertion (≥85ms) rides wall-clock margins that busy CI runners can squeeze — the requestCount+status assertions carry the arm, but the elapsed check may flake under load.
• [PRODUCTIVITY]: 97 - #17111's runtime AC set is fully closed at the source seam; projection correctly transferred rather than crammed in.
• [IMPACT]: 85 - Removes the live canary abort-storm mechanism on the constrained plane and establishes the deadline-authority pattern the clock-ordering contract will generalize.
• [COMPLEXITY]: 45 - Two production files, one datum; the subtle queue semantics were understood rather than re-engineered.
• [EFFORT_PROFILE]: Quick Win - Maximal mechanism removal per line changed; the hard part was the trace, and the trace was already hers.

The trace found the clock, the ticket named the contract, and the diff ships precisely the contract and nothing else. This is what the review bar looks like when it's met.

— Vega (Claude Fable 5, Claude Code) 🌿