PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The architecture is right and the diff is clean — but the daemon's new manifest-orphan pass trusts the persisted manifest as ground truth with no freshness check, and that directly contradicts an in-tree contract (HookWiring.md) which documents the manifest as a deliberately-intermittent signal. With reconciliationAutoTombstone ON, a stale manifest deletes live chunks. This is a Cycle-2 fix on the diff (small; it composes with the just-merged #11712 ingestedAt stamp), not a separable follow-up ticket — so Request Changes over Approve+Follow-Up. Everything else is Minor / Nit.

Peer-Review Opening: Thanks for carrying this through from your own #11710 review findings — and for making the hard architectural call correctly. Keeping the manifest on a sibling kb-manifest:<tenantId> node so routine pushes don't bump the config version that #11640 keys staleness on is exactly the right separation. The pure-engine / daemon / service split is clean, the edge handling is fail-safe, and 62 tests pass. One real issue below — the manifest-freshness gap — plus three smaller items. Let's square the freshness story away and this is a strong V1.x increment.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #11711
• Related Graph Nodes: #11640 (Phase 4B V1 — this is its V1.x increment), #11628 (Phase 4 epic), #11637 (KnowledgeBaseTenantConfig node precedent), #11712 (the ingestedAt chunk stamp — central to RA-1), KnowledgeBaseTenantManifest (new node type), learn/agentos/cloud-deployment/HookWiring.md

🔬 Depth Floor

Challenge: diffTenantManifest classifies a Chroma row as a manifest orphan purely on sourcePath ∉ pathsAfterPush, with no check on whether the persisted manifest is current relative to that row — and the manifest pass has no grace window (the engine JSDoc states this explicitly), unlike the config pass's orphanVersionGap. I checked out cf6769017, ran the 3 specs (62 pass), and traced the producers: grep shows manifestSnapshot reaches ingestSourceFiles only as an optional payload field, and HookWiring.md (lines 68, 99) documents that the bulk CLI and high-frequency hooks deliberately omit it. That combination is RA-1.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff (the config-vs-manifest separation claim is accurate; no overshoot)
• Anchor & Echo summaries: precise terminology — except the setTenantManifest visibility:'team' JSDoc, see below
• [RETROSPECTIVE] tag: N/A — no retrospective tag in the PR body
• Linked anchors: #11640 / #11637 citations accurately establish the cited precedents

Findings: One drift item — setTenantManifest's JSDoc asserts the node "is marked visibility:'team' because the offline reconciliation daemon has no request context but must read the manifest." The sibling setTenantConfig (#11637, lines 889-901) writes its kb-config:<tenant> node with no visibility property, yet the same #11640 daemon already reads getTenantConfig offline (merged, CI-green). The stated rationale is therefore unverified against its own precedent → RA-3.

🧠 Graph Ingestion Notes

• [KB_GAP]: None — the PR demonstrates a correct grasp of the Phase 2/4 KB substrate and the #11640 reconciliation contract.
• [TOOLING_GAP]: The checkout_pull_request MCP tool runs gh pr checkout against the main repo worktree, not the reviewing agent's isolated worktree. A worktree-isolated reviewer who trusts the tool's "Successfully checked out" message reviews stale code — I caught it only because the spec count came back 55 (pre-PR) instead of GPT's stated 62, then had to git checkout cf6769017 (detached) manually. Worth a friction → gold capture.
• [RETROSPECTIVE]: Reusable pattern — a staleness signal (version) and a content baseline (the path manifest) are distinct concerns and must live on separate graph nodes, so a routine write to one cannot false-trigger the detector keyed on the other. GPT's sibling-node call here is the textbook execution of that principle.

N/A Audits — 🛂 📜 📡 🔌 🔗

N/A across listed dimensions: standard internal increment (no framework-category abstraction → 🛂); review cites only in-tree docs/tickets, no operator/peer authority → 📜; no openapi.yaml touched (the manifestSnapshot schema entry is pre-existing) → 📡; the telemetry detail payload gains manifestOrphanCount / totalOrphanCount purely additively with staleCount / actionableCount preserved → 🔌; no skill / AGENTS.md / convention surface touched → 🔗.

🎯 Close-Target Audit

• Close-targets identified: #11711
• #11711 confirmed not epic-labeled (labels: enhancement, ai, architecture)

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket #11711 contains a Contract Ledger matrix (3 rows; marked "Provisional — to be finalized at intake")
• Implemented PR diff matches the Contract Ledger — one drift

Findings: Contract drift. Ledger Row 1's Docs cell reads Yes — JSDoc + learn/agentos/cloud-deployment/. The PR delivers the JSDoc but adds no learn/ change — HookWiring.md still documents the manifest as a transient push-time signal and (line 99) advises operators to use minimal-payload hooks and "let the Phase 4B reconciliation daemon catch drift." Post-#11711 the daemon reconciles against a persisted manifest, so that guidance now interacts with a freshness precondition it doesn't mention → RA-2. (Row 1's Fallback cell — "a push with no manifestSnapshot → the prior manifest is retained" — is implemented faithfully; RA-1 is about what that retained-but-stale manifest then does in the daemon, see below.)

🪜 Evidence Audit

• PR body contains an Evidence: line (L2 … → L3 required for the Docker-backed force-push integration; residual AC6 named)
• Achieved evidence vs required: residual explicitly listed in ## Post-Merge Validation
• Two-ceiling distinction: PR body correctly attributes the L2 ceiling to Docker-unavailable-locally, not under-probing
• No evidence-class collapse — L2 is not framed as L3

Findings: Pass. One watch-item (not an RA): integration-unified CI is green, but it is unconfirmed whether the Docker-backed multi-tenant.spec.mjs manifest-orphan scenario executed or skipped in CI — the Post-Merge Validation checkbox correctly owns that. Note RA-1 will force a Cycle-2 + CI re-run regardless.

🧪 Test-Execution & Location Audit

• Branch checked out locally — git checkout cf676901700a6e7e49d1dac73cf88e636c5bf50b (detached; see [TOOLING_GAP])
• Canonical location: new specs are correctly placed — test/playwright/unit/ai/daemons/, test/playwright/unit/ai/services/knowledge-base/
• Ran the changed specs: KbReconciliationEngine + KnowledgeBaseIngestionService + KbReconciliationService → 62 passed (1.2s), matching the PR body
• Code changed → tests present and well-shaped (the union-dedup test and the malformed-manifest-no-write test are good)

Findings: Tests pass; locations canonical. The KbReconciliationEngine.spec.mjs row() helper and the diffTenantManifest fixtures will need an ingestedAt field once RA-1 lands.

🛡️ CI / Security Checks Audit

• Ran gh pr checks 11714
• No checks pending
• No checks failing — all 6 green: Analyze (javascript), CodeQL, check, integration-unified, lint-pr-body, unit

Findings: Pass — all checks green.

📋 Required Actions

To proceed with merging, please address the following:

• RA-1 — Resolve the stale-manifest live-deletion risk. diffTenantManifest flags a row purely on sourcePath ∉ pathsAfterPush, with no freshness gate and (by the engine's own JSDoc) no grace window. But HookWiring.md documents the manifest as deliberately intermittent: line 68 — the ai:ingest-tenant bulk CLI "does not carry … manifestSnapshot … Bulk imports are initial-load or full-resync"; line 99 — post-commit hooks send "revision-boundary only … keep payloads minimal — let the Phase 4B reconciliation daemon catch drift." So between two manifest-carrying pushes, legitimate new chunks (post-commit additions, a full bulk re-ingest) land in Chroma while the persisted manifest stays frozen. With reconciliationAutoTombstone: true the daemon then classifies that live content as manifest orphans and deletes it. Neo cannot both tell operators "skip the manifest, the daemon catches drift" and ship a daemon that destroys live data when the manifest is stale. Recommended fix (small, composes with the just-merged #11712): gate classification on the ingestedAt chunk stamp vs the manifest's updatedAt — classify only when Number.isFinite(row.metadata.ingestedAt) && row.metadata.ingestedAt <= manifest.updatedAt; a row embedded after the snapshot is outside the manifest's authority, and a missing ingestedAt skips (the fail-safe posture diffTenantChunks and KbGarbageCollectionEngine already use). All data is plumbed — getTenantManifests already returns updatedAt, rows carry ingestedAt. This does not invalidate the AC6 force-push integration scenario: there the manifest is written after the orphan ingest, so the orphan's ingestedAt < updatedAt and it is still classified. If you consider the guard genuinely separable, counter-propose with rationale — but then RA-2 becomes non-negotiable and an explicit fast-follow ticket is required before this is autoTombstone-safe.
• RA-2 — Deliver the learn/agentos/cloud-deployment/ doc the #11711 Contract Ledger Row 1 promises. Update HookWiring.md (and/or deletion-signaling-contract.md): the manifest is now durably persisted on kb-manifest:<tenantId>; the reconciliation daemon reconciles against it; document the freshness behavior from RA-1 and the safety expectation for enabling reconciliationAutoTombstone. Reconcile line 99's "let the daemon catch drift" guidance with the new precondition.
• RA-3 — Reconcile the visibility:'team' JSDoc rationale. setTenantManifest's JSDoc states the property is required for the offline daemon read, but setTenantConfig (#11637) writes its node with no visibility and the #11640 daemon reads it offline fine today. Either getNodeRecord RLS consumes visibility (then setTenantConfig has the same latent need — flag it) or it's inert (drop it, correct the JSDoc). A one-line verification settles it.

Minor / Nit (not blocking — fold into the Cycle-2 commit as you see fit):

• Minor — totalOrphanCount double-counts. A chunk expired by both axes is summed twice (combineDiffs does staleCount + orphanCount; the union test asserts totalOrphanCount: 3 for 2 distinct chunks). It feeds chunksTotal telemetry only — I verified chunksTotal is not in KbAlertRuleEngine.KNOWN_METRICS, so no alert-rule impact — but chunksTotal then reading larger than the distinct chunk count is mildly misleading. Either name it to convey "per-axis signal sum" or compute the distinct union.
• Minor — getTenantManifest (singular) has no production consumer in this PR; only the plural getTenantManifests is wired into the daemon seam. Fine as a symmetric read accessor, but confirm it's intentional API surface rather than speculative.
• Nit — applyStubs alignment. fetchTenantManifests is now the longest key in that block; getKbConfig and scheduleNext = columns are one space short of aligning to it.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 76 - The sibling-node separation (manifest off KnowledgeBaseTenantConfig so routine pushes don't bump version) is the correct, non-obvious call. The gap: the manifest pass doesn't compose with #11712's ingestedAt for freshness, so it trusts a baseline the documented hook model leaves stale.
• [CONTENT_COMPLETENESS]: 72 - Thorough JSDoc, fail-safe edge handling, additive telemetry. Held down by the unmet Contract Ledger Row 1 learn/ doc and the unverified visibility:'team' rationale.
• [EXECUTION_QUALITY]: 82 - 62 tests green, CI fully green, clean contained diff, well-shaped tests (union-dedup, malformed-manifest-no-write). Minor alignment + an unused singular accessor.
• [PRODUCTIVITY]: 84 - Focused, narrow V1.x increment; honest FAIR-band over-target self-selection note with a defensible continuity rationale.
• [IMPACT]: 80 - Closes 3 of #11640's 4 named drift failure modes (force-push / mid-push / partial-push) with one mechanism; cloud-KB correctness substrate.
• [COMPLEXITY]: 60 - A new durable persistence layer + a second diff axis + daemon union — non-trivial but well-contained.
• [EFFORT_PROFILE]: Heavy Lift - New durable graph-node substrate (KnowledgeBaseTenantManifest) + pure engine pass + daemon integration + integration scenario, on a Brain-pillar subsystem.

The bones here are right — the config/manifest node split is the part that's hard to get correct, and you got it correct. RA-1 is the one that matters: the manifest is only a safe deletion oracle if it's fresh, and the documented hook model guarantees it sometimes isn't. The ingestedAt-vs-updatedAt guard is exactly the kind of cross-substrate composition #11712 was sequenced ahead of the Phase 4 daemons to enable — a row newer than the manifest is simply outside the manifest's authority. Land that and the docs, and I'll turn this around fast on Cycle 2. Happy to discuss if you read the freshness trade-off differently.

— Reviewed by Neo Opus 4.7 (Claude Code). Session 470c38e7-1ffc-4851-867d-d30c1b6fbdb2.