PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: Clean extraction that separates the ingest_source_files MCP-facade policy concerns (transport visibility, batch-volume gate, stdio fail-closed) from the generic toolService.mjs mapping module. Co-locates the policy into one cohesive module — next agent reading "how is ingest_source_files transport-gated?" has one file to look at. Runtime behavior preserved (no Evidence-class collapse risk). The structural pre-flight choice (server-local sibling vs new tools/ subdir) is defensible — a tools/ directory at this point would carry one entry; extract first, generalize later if siblings accumulate.

Peer-Review Opening: Nice substrate work — toolService.mjs was carrying too many policy responsibilities (mapping + transport-gating + work-volume-gate). Approving.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12033
• Related Graph Nodes: KB_IngestionService, ingestSourceFilesViaMcp MCP facade, KB transport-visibility policy

🔬 Depth Floor

Challenge: The preserved error-message string still references Phase 2C ("a tenant-scoped bulk ingestion facade is planned (Phase 2C)"). The phrase is a 1:1 lift from the pre-extraction version (not new archaeology), but it's a decay-prone phase-marker per feedback_jsdoc_archaeology_self_audit — if the Phase 2C plan reshapes or ships, the error message goes stale silently. Non-blocking for this PR (it's a no-behavior-change refactor, not a comment-rewrite), but worth a follow-up cleanup once the bulk-path actually lands so the message describes the surface in capability terms rather than phase terms.

Rhetorical-Drift Audit: Pass — the PR body claims a "no-behavior-change MCP facade refactor"; the diff substantiates that. Same functions, same logic, same exports, just relocated. The toolService.mjs re-export for compat is noted in the body and confirmed by the spec being able to switch from toolService.ingestSourceFilesViaMcp to ingestTool.ingestSourceFilesViaMcp without changing behavior (only import source).

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: This PR partially answers the architectural sub-question raised in PR #12037 — "is in-process invocation the intended orchestrator→ingestion path, or should the orchestrator route through kb-server's MCP ingestSourceFiles tool?" The facade is now a first-class alternative path with policy gates (transport visibility, batch-volume refusal, stdio fail-closed). Follow-up question worth filing as a ticket: when should the orchestrator's viaMcp: false hardcode at TenantRepoSyncService.mjs:442 migrate to invoking this facade via SSE? Likely answer: once the orchestrator container reliably reaches the kb-server's SSE transport in cloud-deployment topology (ADR 0014).
• [KB_GAP]: The Phase 2C phrase in the error message is an internal phase-marker that won't be in the published KB; if an external client (tenant push agent) hits the gate and surfaces the error verbatim, "Phase 2C" is meaningless to them. Surface-area-aware: the error message is consumer-facing for remote tenant-push clients, not internal-only.

N/A Audits — 🎯 📑 📡 🔗

N/A across listed dimensions: no close-target audit (single Resolves #12033, not an epic), no public contract surface introduced (refactor preserves runtime + same exports), no OpenAPI description changes (only internal module split), no cross-skill integration (the policy substrate it touches already existed — extraction doesn't introduce new conventions).

🪜 Evidence Audit

PR body declares Evidence: L2 targeted unit coverage → L2 required for a no-behavior-change MCP facade refactor. Residual: no live deployed StreamableHTTP smoke in this PR.

• PR body contains Evidence: declaration line.
• Achieved evidence ≥ close-target required (L2 = L2).
• Residual explicitly named (no live SSE smoke). Reasonable for a pure-extraction refactor — a live SSE smoke would over-specify the scope.
• Two-ceiling distinction: shipped at L2 because the close-target's runtime is preserved, not because sandbox-ceiling — explicit in the residual statement.
• No evidence-class collapse — body doesn't inflate L2 to L3/L4.

Findings: Pass.

🧪 Test-Execution & Location Audit

• Canonical Location: spec at test/playwright/unit/ai/mcp/server/knowledge-base/IngestSourceFilesTool.spec.mjs matches the unit-test placement convention.
• Spec correctly imports ingestSourceFilesViaMcp from the new module path (ingestSourceFilesTool.mjs) while keeping callTool + listTools imports against toolService.mjs — locks the extraction without disrupting the rest of the surface.
• JSDoc inside the spec updated to reference the new module path ("tested directly from ingestSourceFilesTool.mjs"). No stale archaeology on the test side.
• Did NOT locally check out the branch; trusting PR body claim of 6/6 passing.

Findings: Pass on canonical placement + spec wiring; local test-run verification deferred to CI.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — single-module-one-responsibility extraction; transport-policy localized to the facade.
• [CONTENT_COMPLETENESS]: 90 — gate logic + visibility helper + fail-closed assertion all extracted together; spec import path locks the extraction.
• [EXECUTION_QUALITY]: 92 — structural pre-flight performed; JSDoc updated symmetrically; compat re-export keeps non-spec callers undisturbed.
• [PRODUCTIVITY]: 90 — narrow refactor scope, low review cost, contained blast radius.
• [IMPACT]: 65 — improves separation of concerns + readability for future agents; not a behavior change, so user-facing impact is indirect.
• [COMPLEXITY]: 22 — mostly file relocation + import wiring; one decision point (sibling vs new tools/ subdir) documented.
• [EFFORT_PROFILE]: Quick Win — small substrate cleanup with positive future-readability return.

Approving — ready for human-gate merge.