PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Sprint-aligned (40h cloud-deployment trial readiness). #11938 is the API adapter my #11790 directly depends on; reviewing it cross-family unblocks the next step in the deployment chain (#11788 GitMirror → #11789 envelope builder → #11790 scheduler lane → #11791 docs/telemetry). 562 insertions / 2 deletions; full CI green; the rename-aware GitMirror diff handling is a meaningful Δ from ticket prescription that catches a real edge case (renamed source = new live path + old path tombstone).

Peer-Review Opening: Clean separation of concerns — envelope-shape contract lives at the public buildIngestEnvelope() boundary; full-vs-incremental decision tree is a simple cascade (no baseline → full; non-linear history → full; linear → incremental); error codes are stable (KB_INGEST_ENVELOPE_*) and namespace-disjoint from GitMirror's (KB_GITMIRROR_*). The rename-aware delta + manifestSnapshot for bootstrap/non-linear paths is the kind of substrate-correctness that the trial-window operator will quietly appreciate.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #11789; Related: #11731
• Related Graph Nodes: GitMirror (#11788 — consumed; rename-aware diff added here), TenantRepoAccessContract (consumed for normalizeRepoSlug + deriveTenantRepoMirrorPath + redactTenantRepoSecrets), KnowledgeBaseIngestionService.ingestSourceFiles (downstream consumer), TenantIngestionModel.md (operator-facing guide updated).

🔬 Depth Floor

Challenge / V-B-A surfaces worth attention:

1. headRevision null-safety in incremental path: resolveRevision({ref: newHead}) (default 'HEAD') without fallbackToFull: true. If the mirror has no HEAD (corrupt mirror state), this throws KB_INGEST_ENVELOPE_REF_RESOLVE_FAILED. That's correct behavior, but worth confirming the consuming scheduler (#11790) propagates rather than swallows — otherwise the operator gets a silent skip with no traceable cause.

2. Sequential file-content reads in buildFilePayloads: each readRevisionFile is awaited individually. For a tenant repo with thousands of changed paths in a single push, this could be slow vs. a git cat-file --batch stream. Acceptable for MVP (per ticket "post-MVP cloud deployment" residuals at #11735) — flagging as known future-perf consideration, not a blocker.

3. manifestSnapshot.pathsAfterPush in buildFullEnvelope: sorted + filtered ls-tree output. If the downstream KnowledgeBaseIngestionService doesn't currently consume this field, it's defensive substrate (claim the path set so the KB can reconcile). Worth a sentence in the JSDoc confirming the consumer-side contract; otherwise a future reader might think it's vestigial.

4. GIT_MAX_BUFFER = 50 * 1024 * 1024 (50MB): reasonable for ls-tree output but could overflow on monorepos with hundreds of thousands of paths. Empirical-anchor case probably fine; substrate-edge case worth noting in JSDoc or a follow-up "very large tenant repo" ticket.

None of the above are blockers — the substrate-shape is correct.

Rhetorical-Drift Audit: N/A — slot-rationale in PR body is precise; no overshoot.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A.
• [TOOLING_GAP]: N/A.
• [RETROSPECTIVE]: Stable error-code namespacing (KB_INGEST_ENVELOPE_* vs KB_GITMIRROR_*) + injectable gitMirror parameter (default GitMirror, replaceable for tests) is a model worth lifting as a substrate convention for future adapter-shape modules.

🎯 Close-Target Audit

• Close-targets identified: Resolves #11789 (leaf sub-ticket; not epic-labeled).
• #11789 confirmed not epic-labeled.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket (#11789) + parent epic (#11731) contain the prescription matrix (envelope-shape contract documented inline + slot-rationale in PR body).
• Implemented diff matches: buildIngestEnvelope signature exactly mirrors the ticket-prescribed envelope shape (files, deleted, manifestSnapshot, baseRevision, headRevision).

Findings: Pass.

🪜 Evidence Audit

• PR body declares Evidence: L2 (local Git fixture unit tests for mirror diff and ingest envelope contracts) → L2 required.
• Achieved evidence (L2) ≥ close-target required evidence (L2) per the PR body's "server-side envelope construction is fully unit-verifiable without external tenant credentials" justification.
• No [L<N>-deferred] residual claims; PR is L2-complete.
• Two-ceiling distinction not applicable (server-side substrate is fully sandbox-reachable for unit testing; no L3+ residual on this surface).

Findings: Pass.

N/A Audits — 📡 🔗

N/A across listed dimensions: no ai/mcp/server/*/openapi.yaml touched → MCP-Tool-Description Budget Audit N/A; no skill/AGENTS files touched → Cross-Skill Integration Audit N/A.

🧪 Test-Execution & Location Audit

• Branch fetched locally (git fetch origin codex/11789-ingest-envelope-builder).
• Canonical Location: tests placed at test/playwright/unit/ai/services/knowledge-base/ (canonical mirror of source location) per unit-test.md.
• CI: all 6 checks PASS (Analyze, CodeQL, check, integration-unified, lint-pr-body, unit) on head cdd96cf094d2b1af807b86d0bc6d3b53657f6f5a.
• No tests skipped or marked .only.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

(The four V-B-A flags above are non-blocking observations worth considering for follow-up if they prove load-bearing in real-tenant deployments — none gate this PR.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — Clean adapter shape between #11788 GitMirror primitive and KnowledgeBaseIngestionService.ingestSourceFiles() consumer; preserves the no-secret persistence boundary documented in TenantIngestionModel.md; full/incremental decision cascade is the canonical shape for diff-aware sync. Considered: pre-MVP parsed-chunk-v1 vs raw-file split (both supported via rootKind), credential-handling boundary (correctly delegated to GitMirror), envelope idempotency (full+manifestSnapshot path covers it) — all correctly addressed.
• [CONTENT_COMPLETENESS]: 95 — buildIngestEnvelope has full Anchor & Echo JSDoc; helper functions have summary blocks. 5 points deducted for manifestSnapshot.pathsAfterPush lacking explicit consumer-contract documentation (V-B-A #3 above) — a future reader might not know whether KB currently uses it or it's defensive-only.
• [EXECUTION_QUALITY]: 100 — execFileAsync (no shell interpolation), stable error codes, redactTenantRepoSecrets on every error message, sequential for loops to avoid promise.all explosion on per-file reads, sort-deduplicate on path lists. Considered: race conditions in concurrent envelope builds (each call constructs its own paths via local mirror — no shared state), credential leakage paths (redact at error site), buffer overflow on huge repos (GIT_MAX_BUFFER 50MB is reasonable for unit-of-work) — all handled.
• [PRODUCTIVITY]: 100 — Closes #11789 + adds rename-aware GitMirror Δ + updates TenantIngestionModel.md anchor (3 deliverables in one PR; all stated goals achieved).
• [IMPACT]: 70 — Major substrate for the cloud-deployment trial path; bridges the GitMirror primitive to the KB ingestion service; enables #11790 scheduler lane work. Trial-window-critical.
• [COMPLEXITY]: 60 — Medium: 562-line diff including 2 substantial test files; decision-tree logic is bounded (full/incremental cascade); cognitive load is the envelope-shape contract + the rename-aware GitMirror addition.
• [EFFORT_PROFILE]: Heavy Lift — Cloud-deployment-trial-critical adapter substrate; one of the load-bearing PRs in the 40h sprint chain.

APPROVED — eligible for human merge. Ready for my #11790 scheduler-lane pickup against this API.