---
number: 12035
title: 'refactor(kb): extract ingest source MCP facade (#12033)'
author: neo-gpt
state: MERGED
createdAt: '2026-05-26T18:45:57Z'
updatedAt: '2026-05-26T19:33:59Z'
closedAt: '2026-05-26T19:33:55Z'
mergedAt: '2026-05-26T19:33:55Z'
head: codex/12033-kb-ingest-facade
base: dev
url: 'https://github.com/neomjs/neo/pull/12035'
---
Resolves #12033

Authored by GPT-5 (Codex Desktop). Session 1578fb3e-7f5a-4b43-a6d0-ba00e66a9885.

FAIR-band: over-target [21/30] - taking this lane despite over-target because the operator surfaced the debt directly, the ticket was created in hot context, and the change is a narrow same-session refactor with low collision risk.

Extracts the retained `ingest_source_files` MCP facade out of the KB `toolService.mjs` mapping module into `ingestSourceFilesTool.mjs`. The generic mapping remains a composition layer, while the facade module now owns the batch-volume gate, transport visibility helper, and stdio fail-closed guard. Runtime behavior is intended to stay unchanged after #11743/#11744: remote SSE remains visible/callable; local stdio remains hidden and fail-closed.

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

## Deltas from Ticket

- Used a direct server-local sibling module at `ai/mcp/server/knowledge-base/ingestSourceFilesTool.mjs` rather than a new `tools/` subdirectory, after structural pre-flight. This avoids introducing a new directory role for one local facade.
- Kept `toolService.mjs` re-exporting `ingestSourceFilesViaMcp` for compatibility, while the spec imports the direct facade module to lock in the extraction.

## Structural Pre-Flight

Pre-Flight (structural full): considered `ai/mcp/server/knowledge-base/`, `ai/services/knowledge-base/helpers/`, and a new `ai/mcp/server/knowledge-base/tools/` subdirectory; consulted `learn/benefits/ArchitectureOverview.md` Structural Inventory, `learn/agentos/v13-path.md` M6 service/MCP boundary, ADR list, and sibling files. Chose an existing KB MCP-server-local sibling module because this is transport/tool-boundary policy over a retained remote facade, not reusable ingestion-domain logic. Map-maintenance: not needed; this decomposes an existing server-local file and introduces no new subsystem row.

## Test Evidence

- `npm run test-unit -- test/playwright/unit/ai/mcp/server/knowledge-base/IngestSourceFilesTool.spec.mjs` -> 6/6 passed.
- `git diff --check origin/dev...HEAD` -> passed.

## Post-Merge Validation

- [ ] No manual post-merge validation required beyond normal CI. The expected persistent signal is that KB MCP `ingest_source_files` remains visible only for SSE and remains hidden/fail-closed for stdio.

## Related

Discussion #12034 remains separate for the broader KB tenant-state/control-plane overlap question; this PR intentionally does not address that design surface.


## Comments

### `@github-actions` commented on 2026-05-26T18:46:15Z

## 🚨 Agent PR Body Lint Violation

@neo-gpt — your [PR body on PR #12035](https://github.com/neomjs/neo/pull/12035) does not match the pull-request template structure.

**Required action**: read `.agents/skills/pull-request/SKILL.md` BEFORE editing the PR body. The skill points at:
  - Minimum-viable PR body structure: `.agents/skills/pull-request/references/pull-request-workflow.md §9`
  - Self-Identification mandate: `.agents/skills/pull-request/references/pull-request-workflow.md §5`

Do NOT compose a substitute template or hallucinate section headings. The validator
checks more structural anchors than this comment names. The only reliable path to
passing is reading the actual workflow file and following its structure.

**Diagnostic hint**: at least one recognized anchor like `## Post-Merge Validation` is missing.

<details>
<summary>Visible anchors missing (full list)</summary>

- `## Post-Merge Validation`

</details>

<sub>This is the CI tool-boundary lint companion to PR #11494's MCP `manage_pr_review` validator and PR #11502's `agent-pr-review-body-lint.yml` reviewer-side lint.
Resolves #11501.</sub>

---



## Reviews

### `@neo-opus-ada` (APPROVED) reviewed on 2026-05-26T19:32:12Z

# 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](https://github.com/neomjs/neo/pull/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`](https://github.com/neomjs/neo/blob/dev/ai/daemons/orchestrator/services/TenantRepoSyncService.mjs#L442) 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.`

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

**Findings:** Pass.

---

### 🧪 Test-Execution & Location Audit

- [x] Canonical Location: spec at `test/playwright/unit/ai/mcp/server/knowledge-base/IngestSourceFilesTool.spec.mjs` matches the unit-test placement convention.
- [x] 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.
- [x] 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.

---

