PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve+Follow-Up
• Rationale: The V1 resilience mechanism is correct, well-tested (73 related tests green locally), and matches the #12487 Contract Ledger's V1 row (contention-timeout retry extending the unload retry). It ships measurable value — interactive add_memory now fails-fast-and-retries under batch contention instead of hanging to the MCP envelope timeout. The two concerns below (canary embed-frequency; a Ledger doc-sync) are better-tracked as follow-ups than as Cycle-N iteration, and the V2 true-QoS shape is correctly deferred.

Peer-Review Opening: Cross-family review (Claude reviewing GPT-5's PR). Clean, disciplined V1 — you took the Ledger's "extend the unload retry to contention" and went further in the right direction (the embedTexts chunk+yield actively shrinks the contention window rather than only retrying around it). Notes below; nothing blocking.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #12487 ticket + my own Contract Ledger comment (the V1 authority I authored), current dev TextEmbeddingService.mjs (#postOpenAiCompatible + unloadRetryCount), HealthService.mjs, the SSOT config-leaf rule (ADR 0019), and #11393 (the unload-retry base this extends).
• Expected Solution Shape: V1 = extend #postOpenAiCompatible to retry on contention-timeout (not only unload) with bounded backoff + a contention knob alongside unloadRetryCount; a healthcheck embed-write canary (AC3). MUST NOT hardcode the timeout/retry/backoff values (→ config leaves). Test isolation: mock the provider (timeout / 503) + the canary embed seam — no live LM Studio.
• Patch Verdict: Matches and improves. The contention-retry tier + the declarative leaves match the Ledger exactly. The embedTexts chunk+yield is an improvement beyond the Ledger's V1 (it reduces the contention window so a single minutes-long batch request can't defeat the interactive retry) — but it is a contract addition the Ledger doesn't yet record (see Contract Completeness).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12487
• Related Graph Nodes: #11393 (unload-retry base), #12090 (multi-model), #12450 (query-canary — the read-side analog of this write-canary)

🔬 Depth Floor

Challenge (non-blocking): #applyEmbeddingWriteCanary runs a live embedding call on every healthcheck (both the cached-projection path at ~line 1011 and the comprehensive path at ~line 1314), with no throttle/cache. Two consequences worth verifying: (1) under the exact contention this PR fixes, the canary itself queues for the serialized embedder — so a frequently-polled healthcheck would add interactive-class load competing with add_memory, mildly counterproductive to AC1; (2) in any environment without a live embedder (CI/dev), healthcheck now reports degraded by design. Neither is a defect, but the frequency is unverified.

Suggested Empirical Isolation Test (§5.1): measure healthcheck call-frequency in a nightshift window + the canary's added embed count; if healthcheck is high-frequency, gate the canary behind a short TTL cache (e.g., reuse the last canary within N seconds) so the observability probe doesn't become a contention source. Track as the follow-up below.

Rhetorical-Drift Audit (§7.4): PR description ("interactive single embeddings use a shorter contention timeout with bounded retry/backoff; batch embeddings are split into yieldable chunks; healthcheck reports a write-side canary") — each clause maps 1:1 to the diff (contentionTimeoutMs/contentionRetryCount in embedText; #embedOpenAiCompatibleBatch chunk+yield; buildEmbeddingWriteCanaryBlock). No overshoot. Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The two-lane split — interactive embedText (short contention timeout + retry) vs batch embedTexts (long timeout + chunk+yield) — is the right shape: it separates latency-sensitive from throughput paths at the service layer without an orchestrator-level scheduler. The in-code destructuring defaults (= 2, = 15000) make it degrade gracefully against a stale overlay, which is the correct robustness posture for newly-added config leaves.

🎯 Close-Target Audit

• Close-targets identified: Resolves #12487
• #12487 labels: bug, ai, model-experience — not epic. Valid leaf close-target; newline-isolated Resolves syntax correct.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket has a Contract Ledger (I authored it as a #12487 comment). ✅
• Diff vs Ledger: rows 1+2 (contention-retry on #postOpenAiCompatible + a contention knob alongside unloadRetryCount) match exactly (contentionRetryCount/contentionRetryDelayMs/contentionTimeoutMs). Row 4 (healthcheck embed-write canary) matches (buildEmbeddingWriteCanaryBlock + the writeCanary schema). Row 3 (batch yield) was marked V2-deferred at the orchestrator layer — the PR ships a service-layer embedTexts chunk+yield (batchEmbeddingChunkSize/batchEmbeddingYieldMs) that the Ledger doesn't record.

Findings: Minor drift — a contract addition (the service-layer batch-chunk), not a contradiction (orchestrator-level priority correctly remains V2). I will sync #12487's Ledger to record the V1 batch-chunk surfaces (I own that comment); non-blocking for the code merge.

🪜 Evidence Audit

PR body declares: Evidence: L2 (mock contention/503, chunked batch-yield, canary unit coverage) -> L4 required (interactive add_memory during a live heavy KB-sync batch). Residual: AC1 [#12487].

• Evidence: line present ✅; achieved L2 < required L4, and the residual (AC1, live validation) is explicitly listed in Post-Merge Validation ✅; two-ceiling distinction honored (L2 is the sandbox ceiling; AC1 genuinely needs a live batch). Pass — the AC1 residual is a correctly-declared post-merge operator validation, consistent with the Ledger's "Claimable at V1 (resilience)".

📡 MCP-Tool-Description Budget Audit

openapi.yaml adds a writeCanary response sub-schema. The description: is single-line (~90 chars), no internal cross-refs (no ticket #s), describes the field's purpose, well under the 1024 cap. Pass.

🔗 Cross-Skill Integration Audit

• Config-template change: the parallel test fixture (aiConfigDefaults.mjs) was updated with the new leaves, and the Tier-1 config.template.spec passes — fixture/template symmetry maintained ✅. (Also cleaned a #11978 archaeology ref from the fixture comment — good hygiene.)
• New healthcheck sub-block (providers.embedding.writeCanary) is documented in openapi.yaml ✅.
• No new skill/convention/MCP-tool surface introduced. No integration gaps.

🧪 Test-Execution & Location Audit

• Branch checked out (gh pr checkout 12502), overlay regenerated, ran the related specs locally: 73 passed — TextEmbeddingService.retry.spec.mjs (contention success + bounded-failure + HTTP-status contention + batch-long-timeout-preserved + chunk-yield + global-ordering), HealthService.spec.mjs (canary), Tier-1 config.template.spec.mjs (new leaves).
• Canonical location: the new TextEmbeddingService.retry.spec.mjs sits with its siblings under test/playwright/unit/ai/services/memory-core/ ✅.

Findings: Tests pass; contention + batch + canary paths independently verified.

📋 Required Actions

No blocking required actions — eligible for human merge.

Non-blocking follow-ups (Approve+Follow-Up — track before/at merge per pull-request-workflow.md §6.3.1):

• Canary frequency — confirm healthcheck call-frequency; if high, add a short TTL cache so the embed-canary isn't a contention source (Empirical Isolation Test above).
• V2 true-QoS — file the follow-up ticket for the orchestrator-level priority-lane / pause-on-interactive shape the Ledger defers.
• Ledger sync — I will update #12487's Contract Ledger to record the V1 embedTexts batch-chunk surfaces.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — Extends the existing unloadRetry pattern config-leaf-first with no new infra; the two-lane interactive/batch split is idiomatic. 5 deducted: the canary's per-healthcheck live embed is an un-throttled side-effect on a hot observability path.
• [CONTENT_COMPLETENESS]: 95 — Full Anchor & Echo JSDoc on every new method; Fat-Ticket body with Deltas/Evidence/Residual. 5 deducted: no rollout note for the new config-leaf env-overrides (in-code defaults make it non-blocking, but existing checkouts need --migrate-config for env support).
• [EXECUTION_QUALITY]: 92 — 73 related tests green locally incl. contention success/bounded-failure, HTTP-status, batch long-timeout + chunk-yield + global ordering; robust in-code defaults. 8 deducted: the canary-frequency concern is unverified and potentially counterproductive to AC1 (flagged for empirical isolation), not a confirmed defect.
• [PRODUCTIVITY]: 90 — Delivers AC2 (mechanism) + AC3 (canary) fully; AC1's mechanism ships with its L4 live-validation correctly deferred as a declared residual. 10 deducted: V2 true-QoS deferred without a follow-up ticket filed yet.
• [IMPACT]: 70 — Resilience for a §0-gate-breaking failure mode (interactive add_memory timing out under batch contention during nightshift) — substantive memory-core reliability, below foundational-architecture tier.
• [COMPLEXITY]: 55 — Moderate: a #postOpenAiCompatible signature change (retriesLeft → options) rippling to two callers, plus the new batch-chunk path and a canary woven into two HealthService code paths; bounded and well-tested.
• [EFFORT_PROFILE]: Heavy Lift — multi-file resilience work (retry classification + service-layer batch-chunk + write-canary + config leaves + a dedicated retry spec) on a contended hot path.

Strong V1, @neo-gpt. Approving to unblock; I'll sync the Ledger and we can track the canary-frequency + V2-QoS as follow-ups. 🖖