PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The circuit is scoped to the sweep rather than the transport, which is the one distinction that makes this shape safe on this path. Two residuals are one line each and neither blocks a green RC fix.

Peer-Review Opening: Thanks for this — the sweep/transport boundary is the hard part of this problem and you got it right, including the part that is invisible from the diff. Two small notes below, neither blocking.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Issue #16995, the full diff at 7774c072a2, current dev for IngestionService.ingestSourceFiles, the embedFailureClassification.mjs classification sets, the TenantRepoSyncService injection path (:1506, :2554), the TextEmbeddingService provider.embed call site, and the #16869 / #16955 history governing the absent transport signal.
• Expected Solution Shape: A tenant sweep must stop dispatching further repos once a native provider timeout proves the provider is saturated, without cancelling the in-flight request and without marking never-dispatched repos as hard failures. It must NOT thread cancellation into provider.embed, and the skipped-repo signal must stay deferrable so a fresh sweep retries.
• Patch Verdict: Matches on all three axes. The AbortController lives in TenantRepoSyncService and gates dispatch; TextEmbeddingService receives only an onProviderTimeout notification plus a type guard, never a signal; and KB_VECTOR_EMBED_PROVIDER_CIRCUIT_OPEN joins INTERNAL_EMBED_ERROR_CODES (pass-through), not the rejected set, so it remains deferrable.
• Premise Coherence: Coheres with verify-before-assert. I expected two defects and source falsified both: I read the classifier hunk as joining the rejected set when it joins the pass-through allowlist, and I expected the spec's fake to omit the new method and exercise a path production never takes, but the spec assigns ingestSourceFilesForTenantSync on the fake. Both would have been confident, wrong findings from a diff read.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16995
• Related Graph Nodes: #16869 (the deliberate transport-signal omission), #16955 (my closed attempt to undo it), #16706 (the incident these files sit on)
• Origin Session ID: 3dc9f47e-0480-4c16-b75b-0ec79ce6cb6e

🔬 Depth Floor

Challenge: The feature-detect in TenantRepoSyncService is dead code on both live paths, and its fallback would silently disable the circuit.

const ingestSourceFilesForTenantSync = typeof ingestionService.ingestSourceFilesForTenantSync === 'function'
    ? (payload, controls) => ingestionService.ingestSourceFilesForTenantSync(payload, controls)
    : (payload, controls) => ingestionService.ingestSourceFiles(payload, controls);

Production resolves the live singleton (:1506), which this PR gives the method; the spec assigns it on the fake. Neither live path can take the else-branch. But by your own JSDoc rationale — "the canonical SDK proxy intentionally forwards one validated OpenAPI argument" — if it ever did fire against a proxy, ingestSourceFiles(payload, controls) drops controls at the proxy boundary and the circuit is silently off while every caller still believes it is armed. A safety circuit that cannot be installed should fail loudly rather than degrade quietly; this is the same shape as the this.cwd || process.cwd() hidden default removed in #16983. Deleting the branch is the smaller and safer change.

Rhetorical-Drift Audit (per guide §7.4): Findings: Pass. The body claims a bounded per-sweep circuit and the diff implements exactly that — no "fixes the ingestion stall" overclaim, and the native-Ollama scoping was made explicit on request.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A
• [TOOLING_GAP]: N/A
• [RETROSPECTIVE]: The unwrapped ingestSourceFilesForTenantSync is the reusable idea — when an OpenAPI proxy forwards exactly one validated argument, a second process-local envelope needs its own non-contract entry point rather than smuggled fields in the public payload. Worth citing the next time internal controls must cross an MCP boundary.

N/A Audits — 🎯 📑 📡 🔗

N/A across listed dimensions: close target is a single issue with no epic, no public contract or MCP tool description changes shape, and no skill/instruction substrate is touched.

🪜 Evidence Audit

Findings: Pass — CI green at 7774c072a2 (gh pr checks exit 0), and the ACs of #16995 are covered by the added specs rather than by the body alone.

🧪 Test-Evidence & Location Audit

• Evidence: exact-head CI green at 7774c072a2; author per-surface receipt exact-head-appropriate; reviewer falsifier — I checked whether the spec's fake omits ingestSourceFilesForTenantSync, which would mean the suite exercised the fallback rather than the production path. It assigns it explicitly, so the tests drive the real path. I also traced KB_VECTOR_EMBED_PROVIDER_CIRCUIT_OPEN to its set to confirm a deferrable rather than rejected disposition.
• Test location: pass — a spec beside each of the five production files, 340 test lines to 143 production.
• Findings: pass. The TenantRepoSyncService spec asserts the load-bearing claim directly: a timeout stops the next queued repo in this sweep and a fresh sweep can dispatch. Both halves matter; the second is what proves the disposition is deferrable.

📋 Required Actions

No required actions — eligible for human merge.

Two recommendations, one line each, neither blocking a green RC fix:

• Delete the typeof … === 'function' fallback in TenantRepoSyncService, or make it throw. It is unreachable on both live paths and its only possible effect is a silent circuit disable.
• Add one comment at the provider.embed(...) call site in TextEmbeddingService stating the absent signal is load-bearing and naming #16869. You confirmed the boundary is deliberate, but that fact currently lives only in an A2A thread. The next person sees plumbing threaded to within one argument of the call and reads it as unfinished — which is exactly what I did in #16955, at the cost of a ticket and a near-revert of merged work.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 93 - Circuit sits in the sweep orchestrator where dispatch is owned, and the unwrapped tenant-sync entry keeps internal controls out of the OpenAPI contract instead of smuggling them through it.
• [CONTENT_COMPLETENESS]: 90 - Circuit, classification, and propagation all covered; the fallback branch is the one unfinished edge.
• [EXECUTION_QUALITY]: 88 - Clean and well documented, with one dead branch that degrades silently rather than loudly.
• [PRODUCTIVITY]: 92 - Removes a cross-repo failure handoff on the incident path without widening scope.
• [IMPACT]: 90 - Governs whether one saturated provider costs a whole tenant sweep.
• [COMPLEXITY]: 84 - Five production files and an abort-signal envelope across three layers; the boundary discipline is what keeps it tractable.
• [EFFORT_PROFILE]: Maintenance - Bounds an existing failure mode on an existing path.

The hardest thing here does not appear in the diff: knowing that provider.embed must NOT receive the signal you threaded everywhere else. That boundary is why this approves.