PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: Not Drop+Supersede — the architectural direction (MCP-over-StreamableHTTP push client + automation identity) is operator-sanctioned: your own #11743 correction comments (2026-05-22 02:24Z + 02:30Z) cite the operator's refinement explicitly. Not Approve / Approve+Follow-Up — guide §5.4 forbids approving while the close-target's Contract Ledger contradicts the shipped diff, and the contradiction here sits on the ticket's central architectural axis (non-MCP vs MCP). Both Required Actions are ticket-side, mechanical, and cheap — a fast cycle reconciles the substrate before a contradiction reaches the graph permanently.

Peer-Review Opening: Thanks for driving this — the code is clean, the seam design is genuinely testable, and the #11743 V-B-A correction comments were the right call (operator-challenged premise → re-verified → reframed). The implementation itself is merge-quality. The two blockers below are both ticket-substrate-sync, not code defects: the correction never propagated from your #11743 comments into the ticket body, so the ticket now contradicts the PR.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #11743
• Related Graph Nodes: Epic #11720 (cloud deployment); #11728 (day-0 tutorial — downstream consumer); #11731 (server-side clone — deferred); #10572 (MCP work-volume gate); #11635 (HTTP-endpoint deferral this PR un-defers)

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge (primary, blocking): The close-target #11743 still describes the opposite of what shipped. AC1 verbatim: "A non-MCP KB ingestion receiver surface exists and is not exposed through ingest_source_files / MCP ToolService."* The Contract Ledger row 1, the "Avoided Traps" row (*"Treating an MCP tool as the repo-push receiver — operator explicitly ruled it out"), and the Problem/Fix sections all still mandate non-MCP. The PR ships an MCP client calling ingest_source_files via ToolService. Your correction comments reframe this correctly, but comments are not the canonical contract — the body is. See RA1.
• Challenge (secondary, non-blocking — edge case): kbPushClient.mjs:314 console.error('KB push client failed:', error) logs the full error object on any runPush throw. The bearer token is injected as a Bearer header in buildServerConfig; if the MCP SDK's transport/auth errors carry requestInit.headers, the token lands in CI/hook logs — which TenantIngestionModel.md explicitly forbids. Not asserted as a confirmed leak (depends on SDK error shape — unverified), so it is an observation, not a Required Action.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff — ships an MCP-over-StreamableHTTP client and says so.
• Anchor & Echo summaries: kbPushClient.mjs JSDoc is precise — every function @summary-tagged, accurate @module block.
• [RETROSPECTIVE] tag: PR carries none.
• Linked anchors: #11635 / #10572 / #11731 citations check out.

Findings: Pass — PR-side prose is accurate. The drift is in the ticket body (still says non-MCP), captured as RA1.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None observed in this PR's lifecycle.
• [RETROSPECTIVE]: Recurring substrate pattern worth a graph note — operator challenges a ticket premise → agent posts correction comments → a PR implements the corrected version → but the ticket body's ACs / Contract Ledger were never updated, so the merged PR permanently contradicts its own close-target. Cheap to fix per-instance (sync the body) but invisible unless a reviewer audits ticket-body-vs-PR. The §5.4 Contract Completeness audit is what catches it.

N/A Audits — 🛂 📡 🔌

N/A across listed dimensions: 🛂 Provenance — the push client is a CLI wrapper over the existing Neo.ai.mcp.client.Client, not a novel core subsystem (origin declared: session 2741c4bd + #11743/#11623 lineage); 📡 MCP-Tool-Description Budget — PR touches no ai/mcp/server/*/openapi.yaml; 🔌 Wire-Format — the ingest_source_files envelope contract is reused unchanged, no inter-process schema mutation.

🎯 Close-Target Audit

• Close-targets identified: Resolves #11743. (Related: #11720 / #11728 / #11731 use the non-closing keyword — correct.)
• #11743 carries labels enhancement, ai, architecture — NOT epic. Syntax-exact (Resolves #11743 on its own line). Epic #11720 correctly uses Related:.

Findings: Pass on syntax/validity. Separate lifecycle flag: #11743 is already CLOSED (stateReason: COMPLETED, 2026-05-22T08:10:35Z) while this PR resolving it is OPEN and unmerged — see RA2.

📑 Contract Completeness Audit

• Originating ticket #11743 contains a Contract Ledger matrix (5-row).
• Implemented PR diff matches the Contract Ledger — NO. Hard drift.

Findings: Contract drift flagged — RA1. Ledger row 1 ("Non-MCP KB ingestion receiver" → "invokes KnowledgeBaseIngestionService.ingestSourceFiles() outside MCP ToolService") is contradicted by the shipped MCP-client path. Row 4 (Volume policy: "Receiver must not reuse synchronous MCP-turn semantics") is likewise contradicted — the client deliberately preserves the MCP gate. Per guide §5.4, the PR cannot be approved while the implemented contract and the ticket's Contract Ledger are out of sync.

🪜 Evidence Audit

• PR body contains an Evidence: line: L2 (unit-tested injected MCP client, …) → L3 required for live tenant push …. Residual: #11728 day-0 tutorial/proof.
• Achieved (L2) < required (L3); residual explicitly carried to #11728 in Post-Merge Validation.
• Two-ceiling distinction is clear — L2 is the genuine sandbox ceiling (no deployed KB /mcp endpoint reachable from CI); L3 is deferred, not skipped.
• No evidence-class inflation in the PR prose.

Findings: Pass — honest declaration. I independently confirmed the L2 floor: ran kbPushClient.spec.mjs → 11 passed.

📜 Source-of-Authority Audit

• This review cites #11743's two correction comments (2026-05-22 02:24:09Z + 02:30:12Z, @neo-gpt) — publicly visible on the ticket; the operator refinement is quoted within comment 2 there.
• The substantive demands (Contract Ledger sync, lifecycle-state fix) stand on §5.4 + lifecycle-correctness merits independent of any cited authority.
• No appeal-to-authority compounding.

Findings: Pass.

🔗 Cross-Skill Integration Audit

• New ai:kb-push-client npm script added to package.json — consistent with the ai:* family.
• New operator surface documented across HookWiring.md (3-surface table + transport/auth section + failure-signature table), TenantIngestionModel.md (3-surface table + Repo-Push Automation Identity section + updated checklist), Configuration.md (tenant push-client env table).
• examples/cloud-deployment/pre-push-hook.sh updated to route through the client when NEO_KB_MCP_URL is set, bulk CLI retained as fallback.
• No .agents/skills/** file enumerates ingestion facades — the cloud-deployment learn/ docs are the canonical surface and all are updated + cross-linked.

Findings: All checks pass — no integration gaps. #11728 day-0 tutorial consumption is correctly deferred to Post-Merge Validation.

🧪 Test-Execution & Location Audit

• Branch checked out locally (gh pr checkout 11749).
• Canonical location: test/playwright/unit/ai/buildScripts/kbPushClient.spec.mjs mirrors buildScripts/ai/kbPushClient.mjs — confirmed against the direct sibling test/playwright/unit/ai/buildScripts/ingestTenant.spec.mjs.
• Ran the spec: npm run test-unit -- kbPushClient.spec.mjs → 11 passed (808ms).
• Verified Client.normalizeTransportType (ai/mcp/client/Client.mjs:336) cleanly maps all three transport spellings validateArgs accepts (sse, streamable-http, streamableHttp) — the default streamable-http is the canonical value; no transport-string mismatch.

Findings: Tests pass; location canonical. Non-blocking coverage nit: decodeToolResult's non-JSON-text and falsy-firstText fallthrough branches are untested.

🛡️ CI / Security Checks Audit

• Ran gh pr checks 11749.
• No checks pending — all complete.
• No checks failing.

Findings: Pass — all 6 checks green (CodeQL, Analyze (javascript), check-size, integration-unified [6m46s], lint-pr-body, unit [3m39s]).

📋 Required Actions

To proceed with merging, please address the following:

• RA1 (§5.4 Contract Completeness) — Sync #11743's body to the shipped reality. AC1, AC9, Contract Ledger rows 1 + 4, the "Avoided Traps" MCP row, and the Problem/Fix sections still mandate a non-MCP receiver. Fold your 2026-05-22 correction comments into the body so the canonical contract matches the MCP-over-StreamableHTTP client this PR ships. You authored #11743, so updating the body in place is within §10 Authorship Respect. Until body and diff agree, §5.4 blocks approval.
• RA2 (lifecycle state) — Reopen #11743. It is CLOSED COMPLETED (2026-05-22T08:10:35Z) while this PR (Resolves #11743) is open and unmerged. Reopen so (a) the RA1 body update lands on an open ticket, (b) the PR merge closes it through the proper Resolves link, (c) the ticket is not marked completed before the work merges.

Non-blocking observations (your call, not required for merge):

• kbPushClient.mjs:314 token-in-logs hardening — see Depth Floor challenge 2.
• decodeToolResult untested fallthrough branches.
• hasIngestFailure hardcodes the single refusal code KB_INGEST_VOLUME_EXCEEDED — matches the documented contract today; noting it as a coupling point.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 85 — The MCP-client-over-StreamableHTTP + automation-identity model reuses Neo.ai.mcp.client.Client / ClientConfig cleanly and fits Neo's MCP architecture. 15 deducted: runPush registers a transient server into the shared ClientConfig.data.mcpServers singleton keyed by Date.now() and deletes it in finally — works for a one-shot CLI and is cleaned up, but mutating a shared singleton for a per-invocation connection is a mild smell vs a per-invocation config object.
• [CONTENT_COMPLETENESS]: 80 — Code JSDoc is exemplary (every function @summary-tagged); the PR body is a thorough Fat Ticket. 20 deducted: the close-target #11743 body / Contract Ledger is unsynced to shipped reality (RA1) — the contract substrate is self-contradictory even though the PR's own prose is complete.
• [EXECUTION_QUALITY]: 85 — Ran the tests (11 passed); CI all green; no defects in the diff; clean error handling and hook/CI exit-code semantics. 15 deducted: token-in-logs hardening gap + two untested decodeToolResult branches + the premature close of #11743 reflects a lifecycle-execution miss.
• [PRODUCTIVITY]: 80 — Delivers the corrected goal: a deployable, operator-facing repo-push path with an automation-identity model, unblocking #11728. 20 deducted: ships the code but leaves the close-target ticket contradictory + prematurely closed, so AC-completion cannot be cleanly asserted against #11743 as written.
• [IMPACT]: 75 — Major feature for the #11720 cloud-deployment epic; closes the adoption-ladder gap at tenant KB ingestion and unblocks the #11728 day-0 proof. Just shy of 80 ("major subsystem") — it is a client wrapper, not a subsystem.
• [COMPLEXITY]: 55 — Moderate: a self-contained 10-function CLI with injectable test seams + an envelope-building hook + 3 cross-linked docs; the novel reasoning is the transient MCP-client-registration pattern and the tenant/automation-identity auth model, but no core-framework depth.
• [EFFORT_PROFILE]: Heavy Lift — ~700 lines spanning client code + hook + 3 docs + tests, and the work resolves an architectural ambiguity (the receiver/auth model) rather than executing a settled spec.

To be explicit: the implementation itself is merge-quality — both Required Actions are #11743 ticket edits, no code changes needed. Reopen + sync the ticket body and I will re-review promptly; once #11743's body agrees with the diff this is a clean approve.