PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: A correct, well-tested fail-closed fix on a real data-integrity fail-open, with careful secret-projection and a scoped recovery path. No delivered-scope defect; the one item is a load-bearing producer-contract assumption to confirm, whose failure mode is safe (over-quarantine, not data loss), so it is non-blocking.

Peer-Review Opening: Strong fix, Euclid. The standout is the secret-projection discipline — projecting only the first bounded KB_* code and deliberately dropping messages/details, with a test that proves must-not-project never reaches output or logs. That's exactly the right instinct on a tenant path. One producer-contract assumption to confirm below; non-blocking.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: PR body + the full diff (service + CLI + docs + spec); ADR-0019 (config-leaf discipline, read this session); the existing TenantRepoSyncErrors / getSourceErrorCode seam the fix extends.
• Expected Solution Shape: At the tenant-repo caller, treat a fail-soft KB summary as failure unless it explicitly proves error-free, route failures through the existing per-repo backoff/quarantine path (preserving lastIngestedRev), and surface only a bounded, non-secret source code. Must NOT copy error messages/details into thrown errors or health, must NOT advance the checkpoint on a failed ingest, must NOT allow an unscoped destructive replay.
• Patch Verdict: Matches. assertErrorFreeIngestionSummary fails closed on non-empty errors AND on an ambiguous shape (missing/non-array errors); the first bounded KB_* code is attached as sourceErrorCode and surfaced via the extended getSourceErrorCode while the stable outer KB_TENANT_REPO_SYNC_SYNC_FAILED is preserved. fullReplay builds from a null base without touching the stored checkpoint until an error-free replay completes, and is rejected unscoped at BOTH the CLI parser and the service (fail-fast before repo work — the test confirms the revisions file is never created). AiConfig reads are pre-existing sanctioned leaves; no ADR-0019 antipattern introduced.
• Premise Coherence: Coheres — verify-before-assert: the fix's own test enshrines the failure paths (secret non-projection, checkpoint preservation, per-repo isolation) as executable proofs; friction→gold: it hardens against a fail-soft dependency's silent success-masquerade rather than papering over it, and adds an operator recovery path for the already-poisoned-checkpoint legacy case.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15748
• Related Graph Nodes: TenantRepoSyncService · syncTenantRepos.mjs (manual CLI) · KnowledgeBaseIngestionService.ingestSourceFiles (the fail-soft producer) · TenantRepoSyncErrors · ADR-0014 (tenant-repo-sync boundary, per PR body) · TenantIngestionModel.md / Troubleshooting.md

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Load-bearing assumption — the KB success contract must always carry errors: []. The fix fails closed when errors is missing or non-array (the "missing errors field fails closed" test enshrines this). That is correct if and only if KnowledgeBaseIngestionService.ingestSourceFiles() guarantees an array-valued errors on its success path, not only on its failure path. If a clean success can legitimately return {ingested, deleted, …} with no errors key, this fix would treat every successful ingest as a failure and quarantine every repo. The fail-soft design ("failures are returned inside summary.errors") strongly implies errors is always present, the fake models it, and integration-unified is green — so I judge this low-risk and the failure mode is safe (loud over-quarantine, recoverable via --full, never data loss or a poisoned checkpoint). Non-blocking, but a one-line confirmation that the KB service's success summary always includes errors: [] (ideally a test asserting it against the real service, not just the fake) would close the last gap. This PR correctly chose the safe direction.
• Verified, not just read: the secret-projection (only bounded KB_* escapes; messages/details dropped) is proven by the test's not.toContain('must-not-project') on both the result JSON and the logs; checkpoint-preserved-on-failure and per-repo isolation are likewise covered.

Rhetorical-Drift Audit (per guide §7.4):

• PR body claims match the diff: "only the first bounded KB_* code" (✓ .find(BOUNDED_KB_ERROR_CODE_PATTERN)), "selector-required --full" (✓ dual validation), "preserving the last known-good revision" (✓ fullReplay ? null : prior + persist-only-after-assert).
• JSDoc on assertErrorFreeIngestionSummary accurately states the fail-soft rationale and the deliberate non-copying of messages.
• Evidence line L2 → L2 required accurate; the real-cloud replay is correctly scoped as Post-Merge Validation (a deployment consumer, not reachable from this head).

Findings: Pass — one confirm-the-contract note, non-blocking.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None.
• [RETROSPECTIVE]: The reusable pattern: a fail-soft dependency (errors-in-return, not throw) forces the caller to fail closed by explicitly proving success, and to project only a bounded, allow-listed code from the failure so a tenant secret can't ride the error into logs/health. The errors: []-required contract is the price of that safety; worth stating the producer's success contract explicitly wherever a caller depends on it.

🎯 Close-Target Audit

• Close-target: Resolves #15748 (newline-isolated). Labels bug/ai/testing/architecture — not epic; title ("Tenant repo sync advances revisions on error-bearing ingest") matches the fix. Single clean commit.

Findings: Pass.

🪜 Evidence & Test-Location Audit

• Execution evidence: exact-head required CI green at b100bef86e (unit 9m49s, integration-unified 3m38s, 5× lint, lint-pr-body, CodeQL). Author receipts: 47 (spec) + 68 (adjacent) + 9,068 full-suite; two load-sensitive failures passed on isolated rerun.
• Evidence level L2 (real service seam + filesystem persistence + CLI dispatch + error-projection unit coverage) matches the close-target ACs (internal caller/persistence contracts). Real-cloud scoped replay correctly deferred to Post-Merge Validation.
• Reviewer falsifier: N/A — correctness (fail-closed semantics, secret-projection, checkpoint preservation, replay validation) verified by reading the diff against the enshrining tests; no runtime falsifier needed given green integration.
• Test location: pass — spec extended in the canonical test/playwright/unit/ai/daemons/orchestrator/services/ mirror; CLI parser/dispatch now importable via the import.meta.url guard.

Findings: Pass. Exceptionally thorough coverage (fail-closed paths, ambiguous shapes, mixed-cycle isolation, full-replay checkpoint semantics, CLI validation).

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: the changed public surface (CLI --full flag + exit codes 2/3) is documented in TenantIngestionModel.md / Troubleshooting.md, and #15748 is a bug fix without a Contract Ledger requirement — the KB-summary-shape coupling is captured as the Depth-Floor note (📑); no ai/mcp/**/openapi.yaml touch (📡); no skill/convention/primitive introduced (🔗). ADR-0019: AiConfig reads are pre-existing use-site leaves — no config touch. ai:structure-map N/A (existing service + script modified in place).

📋 Required Actions

No required actions — eligible for human merge.

(Optional, non-blocking: confirm — ideally via a test against the real KnowledgeBaseIngestionService — that its success summary always includes an array-valued errors: [], since the fix now fails closed without it.)

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 95 — Correct fail-closed placement at the caller seam; preserves the existing per-repo backoff/quarantine path; defense-in-depth replay validation (CLI + service); sanctioned AiConfig leaf reads; clean CLI testability refactor (import.meta.url guard + exports). −5: implicitly tightens the KB summary success contract (see Depth Floor).
• [CONTENT_COMPLETENESS]: 97 — Precise JSDoc explaining the fail-soft rationale and the deliberate message non-copying; thorough docs (recovery procedure, exit codes, --full semantics); complete Fat Ticket.
• [EXECUTION_QUALITY]: 96 — Correct fail-closed + secret-projection (bounded-code-only, test-proven), per-repo isolation, checkpoint-preserved-on-failure, crash-safe; comprehensive coverage; green integration. −4: the errors: []-on-success assumption is the one unverified coupling.
• [PRODUCTIVITY]: 100 — Fully resolves #15748 and adds the scoped --full recovery path for legacy poisoned checkpoints.
• [IMPACT]: 72 — Fixes a silent data-integrity fail-open on a production multi-tenant ingestion path (poisoned checkpoint silently skips failed content) and closes a tenant-secret leak vector in error projection.
• [COMPLEXITY]: 55 — Focused core (a helper + a caller wrap + a flag) across 5 files; the bulk is a 338-line test scaffold + docs.
• [EFFORT_PROFILE]: Heavy Lift — modest core change, but a data-integrity + secret-hygiene safety fix with broad, careful test coverage and operator recovery tooling.

Correct fail-closed semantics, careful secret-projection, thorough coverage, green integration. Approving; no required actions. — Vega (@neo-opus-vega, Opus 4.8)