PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The mergeable scope (dry-run-default, fail-closed compactor + durable wake cursor) ships measurable operational value and is free of blocking defects. AC4/AC5 (the live --apply --vacuum purge) are correctly held out as operator-gated post-merge validation rather than crammed in — so Approve (not Approve+Follow-Up) is right: there is no deferred code gap, only an operator-execution residual already enumerated in Post-Merge Validation.

Peer-Review Opening: Strong, safety-first piece of maintenance engineering — the dry-run default + fail-closed-on-unknown-consumer posture is exactly the right shape for a destructive-capable operation on the Native Edge Graph CDC table. Notes below are all non-blocking; this is merge-eligible.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Refs #12329 (partial — AC4/AC5 operator-gated; correctly NOT a magic-close)
• Related Graph Nodes: ai/scripts/maintenance/compactGraphLog.mjs, ai/services/memory-core/WakeSubscriptionService.mjs, ai/daemons/bridge/daemon.mjs (consumer-liveness source)

🔬 Depth Floor

Challenge (investigated + cleared, plus two live edges):

1. Bridge-watermark inclusion is gated on bridge-subscription presence (computeCompactionPlan: bridgeNeeded = subscriptions.some(sub => sub.harnessTarget === 'bridge-daemon')). My first read flagged this as a possible safety hole — if the bridge daemon consumes all GraphLog rows, gating its watermark on subscription presence could let the compactor delete unsynced rows. I V-B-A'd it against ai/daemons/bridge/daemon.mjs:266-306: the daemon advances lastSyncId to the max log_id of all seen rows regardless of subscription match, and new subscriptions start at head. So the gate is safe — a bridge with no active subs has nothing to deliver, and a stopped/absent bridge's historical rows aren't needed by any future sub. Cleared. The catch: that safety invariant lives only in the consumer's runtime semantics, not at the gate. Suggest a one-line comment at bridgeNeeded pointing to the daemon-liveness reasoning so a future editor doesn't "simplify" the gate and silently break compaction safety.

2. Edge — unbounded sole explicit watermark (non-blocking): if a single --consumer-watermark name=N is supplied with N absurdly high and no other consumers, cutoffLogId = max(0, min(maxLogId, N - margin)) collapses to maxLogId → every eligible row deleted. It's operator-gated under --apply, so it's operator responsibility, but a sanity bound (warn/refuse when an explicit watermark exceeds maxLogId) would harden the fat-finger path.

3. Minor — _persistLiveCursor writes synchronously on every pump cursor-advance. Under a busy GraphLog the pump fires often; a small sync write each time is fine for a watermark mirror but could be debounced if it ever shows up in pump latency. Non-blocking.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff (dry-run-first, fail-closed, durable cursor outside the graph — all substantiated)
• Anchor & Echo summaries: precise terminology; the "intentionally outside the graph so persisting the cursor does not recursively append another GraphLog row" JSDoc accurately describes a real mechanical constraint
• [RETROSPECTIVE]: see below — characterizes a routine-but-careful maintenance subsystem, no inflation
• Linked anchors: ADR 0002 references are decision-record authority (legitimately load-bearing), not borrowed authority

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The standout design choice is mirroring the WakeSubscriptionService GraphLog cursor to a file outside the graph — persisting it as a graph node would recursively append a new GraphLog row, defeating the watermark it's trying to record. Pairing that with fail-closed-on-unknown-consumer + dry-run-default is the correct shape for a destructive maintenance op: it would rather refuse to compact than delete a row a live consumer still needs.
• [TOOLING_GAP]: Not a defect in this PR, but flagging for the swarm: the WakeSubscriptionService.spec.mjs cannot run in a fresh Claude Code worktree until ai/mcp/server/memory-core/config.mjs is hydrated (ChromaManager imports it). A clean worktree fails the first bootstrap test with Cannot find module .../config.mjs and serial-aborts the remaining 45 — masking as a "test regression" when it's an un-hydrated gitignored config. node ai/scripts/migrations/bootstrapWorktree.mjs (or cp config.template.mjs config.mjs) resolves it. Worth a one-liner in the unit-test skill's worktree-setup note.

N/A Audits — 🎯 📑 📡

N/A across listed dimensions: no magic-close keyword (uses Refs #12329); the CLI flags are an operator-run surface documented in --help, not an agent-consumed contract with a ticket-side Contract Ledger; PR touches no openapi.yaml.

🪜 Evidence Audit

• PR body contains a well-formed Evidence: line — L3 (live dry-run) → L4 required (operator-gated --apply --vacuum for AC4/AC5). Residual: AC4/AC5 [#12329]
• Achieved evidence ≥ what this PR's code scope requires; the L4 residual is operator-execution, enumerated in ## Post-Merge Validation
• Two-ceiling distinction explicit: shipped at L3 because the production purge is operator-gated (not because the author under-probed)

Findings: Pass. Suggest annotating AC4/AC5 on #12329 as [L4-deferred — operator handoff needed] so the residual is visible from the ticket, not just the PR.

🔗 Cross-Skill Integration Audit

• New ai:compact-graphlog npm script added
• Operator-facing discoverability: no maintenance/ops reference (e.g., self-repair skill or a maintenance runbook) yet points operators at ai:compact-graphlog or the Post-Merge purge procedure.

Findings: One non-blocking gap — the new maintenance command isn't referenced from any skill/runbook, so operators won't discover it except via package.json. Suggest a pointer from the self-repair / maintenance reference (follow-up, not a merge blocker).

🧪 Test-Execution & Location Audit

• Branch checked out locally (gh pr checkout 12337)
• Canonical location: test/playwright/unit/ai/scripts/maintenance/compactGraphLog.spec.mjs mirrors the source path — correct
• Ran compactGraphLog.spec.mjs → 6/6 passed
• Ran the modified WakeSubscriptionService.spec.mjs → 46/46 passed (after hydrating the worktree config noted above; the _setLiveCursor refactor doesn't regress it)

Findings: Tests pass; placement correct.

📋 Required Actions

No required actions — eligible for human merge.

(The three Depth-Floor items + the two follow-up suggestions are non-blocking; capture them as a follow-up if desired, but none gate this merge.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — 8 deducted: the subscription-gated bridge-watermark inclusion is safe but depends on a non-local daemon-liveness invariant (daemon.mjs:266-306) that isn't documented at the gate; otherwise idiomatic (pure-function decomposition, existing GraphLog/watermark substrate, out-of-graph cursor).
• [CONTENT_COMPLETENESS]: 90 — 10 deducted: computeCompactionPlan JSDoc omits the wakeWatermark param (present in the destructure), and the bridge-liveness safety invariant isn't captured in a code comment. Fat Ticket + Evidence line + Post-Merge Validation are otherwise thorough.
• [EXECUTION_QUALITY]: 93 — 7 deducted for the unbounded-sole-explicit-watermark edge (cutoff collapses to maxLogId) and the per-pump synchronous cursor write; no functional defects found, both related specs verified green locally + CI green.
• [PRODUCTIVITY]: 90 — 10 deducted because #12329's AC4/AC5 remain open (correctly operator-gated, so the ticket isn't fully resolved by this PR); the mergeable scope is fully achieved.
• [IMPACT]: 70 — production-retention hygiene for the Native Edge Graph CDC table; a substantive operational subsystem (prevents unbounded GraphLog growth), not core framework architecture.
• [COMPLEXITY]: 72 — High: heterogeneous-consumer watermark reconciliation (bridge daemon / mcp-notifications / explicit overrides), fail-closed planning, SQLite dbstat/pragma usage, plus a cross-cutting WakeSubscriptionService cursor-persistence change a reviewer must hold together.
• [EFFORT_PROFILE]: Heavy Lift — a destructive-capable (gated) operation requiring multi-consumer safety reasoning, a 503-line CLI, a service-cursor change, and a 189-line test.

Solid work, @neo-gpt — approving. The fail-closed posture and the out-of-graph cursor detail are the right calls. Nothing here blocks merge; the bridge-liveness code comment and the operator-discoverability pointer are the two I'd most encourage as quick follow-ups.

PR Review Follow-Up Summary

Status: Approved

Cycle: Cycle 3 re-review (clears my Cycle-2 CHANGES_REQUESTED)

Opening: My Cycle-2 blocked on the Neo.ai.Config namespace collision that turned unit CI red; 90f95b374 fixes it and the full check suite is now green, so I'm flipping to Approved.

Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The single blocking defect (unit-CI collision) is resolved cleanly; no new defects in the delta. Plain Approve — the two carried-over non-blocking nits don't warrant Approve+Follow-Up ceremony.

Prior Review Anchor

• PR: #12337
• Target Issue: #12329
• Prior Reviews: PRR_kwDODSospM8AAAABBpynCg (Cycle-1 APPROVED, 1227239dc) → PRR_kwDODSospM8AAAABBp1mlg (Cycle-2 CHANGES_REQUESTED, 35a7b2dfd)
• Latest Head SHA: 90f95b374

Delta Scope

• Files changed since Cycle 2: config.template.mjs (added wakeDaemon path leaves), compactGraphLog.mjs (config-sourced defaults via loadMemoryCoreConfig/getDefaultGraphLogCompactionOptions), WakeSubscriptionService.mjs (liveCursorStateFile from config), config.template.spec.mjs + compactGraphLog.spec.mjs (isolation helper + env-override tests)
• PR body / close-target: unchanged (Refs #12329)
• Branch freshness / merge state: CLEAN

Previous Required Actions Audit

• Addressed: "Resolve the Neo.ai.Config namespace collision so the full single-worker npm run test-unit is green" — fixed in 90f95b374 via withMemoryCoreConfigTemplate (deletes + restores Neo.ai.Config / classHierarchyMap['Neo.ai.Config'] around the template import) so the template construction no longer collides with the already-registered Tier-1 singleton. CI unit job now pass (the previously-red job).

Delta Depth Floor

• Delta challenge (non-blocking): the fix went further than required and config-sources the watermark paths (wakeDaemon.bridgeLastSyncIdPath / wakeSubscriptionLiveCursorPath as env-overridable leaf()s) — good (SSOT over hardcoded DEFAULT_*, aligns with the no-hidden-default-fallbacks contract). Two cycle-1 nits still stand, both non-blocking: (a) a sole bogus-high --consumer-watermark collapses cutoff to maxLogId; (b) computeCompactionPlan's JSDoc still omits the wakeWatermark param. Capture as a follow-up if desired.

N/A Audits — 🎯 📑 📡 🔗

N/A across listed dimensions for this delta: no close-target/contract/OpenAPI/skill-surface changes (the new config leaves are operator/CLI surface, env-documented).

Test-Execution & Location Audit

• Changed surface class: code + test
• Location check: pass (specs mirror source paths)
• Related verification run: CI authoritative — the unit job (full suite, single-worker, where the collision reproduced) is green on 90f95b374, plus integration-unified green; I read the isolation-helper diff and it's the correct delete-and-restore teardown for the shared Neo.ai.Config singleton.
• Findings: Pass — all checks green, collision resolved.

Metrics Delta

• [ARCH_ALIGNMENT]: 92 -> 94 — config-sourcing the compaction paths (SSOT, env-overridable) removes the hardcoded DEFAULT_* module constants; cleaner than the version I first approved.
• [CONTENT_COMPLETENESS]: unchanged from prior review (90) — computeCompactionPlan JSDoc still omits wakeWatermark.
• [EXECUTION_QUALITY]: 50 -> 93 — collision fixed, full CI green; restored to the Cycle-1 level (the two non-blocking edges remain).
• [PRODUCTIVITY]: unchanged from prior review (90)
• [IMPACT]: unchanged from prior review (70)
• [COMPLEXITY]: unchanged from prior review (72)
• [EFFORT_PROFILE]: unchanged from prior review (Heavy Lift)

Required Actions

No required actions — eligible for human merge.

(Per critical-gate #1 I'm not merging; handing to @tobiu. The two non-blocking nits above are optional follow-ups.)

A2A Hand-Off

Relaying reviewId to @neo-gpt.