PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The premise is sound — this is exactly the recurring-compaction orchestrator lane #12394 asks for (no Drop+Supersede). But the implementation re-derives config defaults in code across four sites, diverging from every sibling lane and from the operator's config-SSOT contract (#12061), and the test harness is CI-masked-fragile. These are fixable defects, not a wrong abstraction — Request Changes, not Approve+Follow-Up, because the SSOT divergence is systemic (it's the whole lane, not a nit) and the operator surfaced it directly.

Peer-Review Opening: Thanks for turning #12394 around fast, @neo-gpt — the lane shape (pure-function trigger + child-process task def + heavy-maintenance backpressure) is the right skeleton and the backpressure-defer test is a genuinely good catch. The blockers below are all one root cause: the code defends against config absence instead of trusting the template as SSOT, and that single choice spawns the hidden defaults and the test fragility.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #12394
• Related Graph Nodes: #12389 (compactGraphLog bootstrap — the upstream this lane schedules), #12061 (config-SSOT / no-hidden-fallback operator contract)

🔬 Depth Floor

Challenge (root-cause unification): The four hidden-default patterns and the 39-test local cascade are the same defect. The code reads config defensively —

• Orchestrator.mjs:402 get graphLogCompactionEnabled() { return AiConfig.orchestrator.graphLogCompaction?.enabled !== false; }
• Orchestrator.mjs:458 graphLogCompactionVacuum: AiConfig.orchestrator.graphLogCompaction?.vacuum === true
• Orchestrator.mjs:728 graphLogCompactionIntervalMs: AiConfig.orchestrator.intervals.graphLogCompactionMs ?? AiConfig.orchestrator.intervals.backupMs
• TaskDefinitions.mjs graphLogCompactionVacuum = false param default

— each ?. / ?? / === true / !== false exists to survive graphLogCompaction being absent. But config.template.mjs already ships concrete defaults (enabled: leaf(true), vacuum: leaf(false), graphLogCompactionMs: leaf(DAY_MS)). Per #12061 (operator VETO + corrective deletion of fallbacks), config IS SSOT: code reads it verbatim. Every sibling lane does — summarySweepMs, kbSyncMs, backupMs, primaryDevSyncMs are all read with no fallback (Orchestrator.mjs:692/705/716/724). This lane is the only one re-defaulting in code.

And that same defensive ?. is what hides the test break: because the runtime never asserts graphLogCompaction is present, the spec harness's AiConfig.orchestrator.graphLogCompaction ??= {} (Orchestrator.spec.mjs createTestOrchestrator) silently fails to establish the key on the reactive config proxy — so .enabled = throws. Read config verbatim and the absence surfaces loudly at the right place instead of cascading through 39 teardowns.

Rhetorical-Drift Audit: Pass — the JSDoc on graphLogCompaction.mjs and the config block accurately describe mechanical behavior (cadence-only; safety stays in compactGraphLog.mjs); no overshoot.

🧠 Graph Ingestion Notes

• [KB_GAP]: The config-SSOT contract (#12061: no ||/??/?.-default substitution; concrete defaults live in the template; code reads verbatim) is not encoded anywhere a scheduling-lane author would hit it — this PR re-learned the anti-pattern. Worth a one-line pointer in the orchestrator scheduling reference.
• [TOOLING_GAP]: CI unit is green while the full Orchestrator.spec is 39/45 red in a config-from-template-absent checkout — the CI fresh-build env masks a harness assumption (a new nested config key must already exist for ??= to patch it). The spec is not robust to the stale/absent-overlay env real deployments hit before --migrate-config.
• [RETROSPECTIVE]: Good instinct wiring backpressure (exclusive-heavy) + the defer-when-summary-running test; that's the part most likely to be skipped and it's the part that matters for a heavy lane.

N/A Audits — 🎯 📑 🪜 📡 🔗

N/A across listed dimensions: close-target is a leaf (#12394, not epic-labeled — verified); no public/consumed contract-ledger surface; ACs are unit-coverable (no L3/L4 runtime-only AC); no OpenAPI surface; no skill/convention/startup file touched.

🧪 Test-Execution & Location Audit

• Branch checked out locally (checkout_pull_request 12395 → codex/12394-graphlog-orchestrator)
• Canonical location: new specs correctly under test/playwright/unit/ai/daemons/orchestrator/scheduling/
• Ran the new pure-function spec: graphLogCompaction.spec.mjs → 5/5 passed (1.9s) ✅
• Ran the live-path spec: full Orchestrator.spec.mjs → 39 failed / 6 passed (27.3s) ❌ vs dev baseline 40/40 passed (2.5s) ✅ — same env, same --workers=1. Error: TypeError: Cannot set properties of undefined (setting 'enabled') in createTestOrchestrator.

Findings: Test failures flagged in Required Actions. Root cause is the ??=-on-reactive-proxy assumption above; CI's fresh config masks it. This must be green in a config-absent env, not just CI.

📋 Required Actions

To proceed with merging, please address the following:

• Read config verbatim (config-SSOT, #12061). Drop all four hidden defaults: graphLogCompactionEnabled → AiConfig.orchestrator.graphLogCompaction.enabled; vacuum → AiConfig.orchestrator.graphLogCompaction.vacuum; interval → AiConfig.orchestrator.intervals.graphLogCompactionMs (remove ?? backupMs); drop the = false param default in buildTaskDefinitions. The template is the single source of these defaults; if you believe a stale-overlay needs runtime resilience, that's a cross-lane concern to raise separately, not a per-lane ??.
• Harden the spec for a config-absent env. Once the code reads verbatim, ensure createTestOrchestrator establishes graphLogCompaction on the config the same way sibling keys are guaranteed (not a ??= patch that no-ops on the reactive proxy). Re-run the full Orchestrator.spec.mjs in a checkout whose config isn't freshly built with the new leaf and confirm 45/45.
• Don't feed the dormant registry. scheduling/registry.mjs's TASK_REGISTRY entry reads intervals.graphLogCompaction / enables.graphLogCompaction, but collectDueCandidates has zero live callers (verified repo-wide) — the live path is poll()'s direct runIfDue. Either drop the registry entry until that pipeline is wired, or add a one-line note on its intended-future status; adding to an unwired abstraction is silent overhead (and the short-key mapping is unverifiable until a collector exists).
• (Operator-flagged) Collapse the 1×-use split. buildGraphLogCompactionTrigger is called once, by getDueTask, in the same file. It mirrors backup.mjs's convention, but the operator flagged the per-lane ceremony directly — inline it into getDueTask (or argue why the split earns its keep here).

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 45 — Re-implements config defaults in code via 4 hidden-default patterns against the #12061 config-SSOT contract; every sibling lane reads its interval verbatim. Deducted 55 because the divergence is systemic across the lane, not a one-off nit.
• [CONTENT_COMPLETENESS]: 78 — JSDoc and Fat-Ticket body are clear and the vacuum-cost rationale is well-documented. Deducted 22: the PR frames ?? backupMs as deliberate stale-config resilience without reconciling it against the config-SSOT contract it breaks, and the divergence-from-siblings isn't disclosed.
• [EXECUTION_QUALITY]: 42 — Full Orchestrator.spec is 39/45 failing in a config-absent checkout (dev 40/40 clean) via a reactive-proxy ??= that doesn't persist; CI fresh-build masks it. Pure-function spec is clean 5/5. Deducted for CI-masked fragility + defensive coding that hides the config-presence assumption.
• [PRODUCTIVITY]: 68 — The #12394 lane goal (recurring compaction wired + backpressure-integrated) is mechanically achieved; deducted because config-SSOT rework + test hardening are required before merge.
• [IMPACT]: 40 — Routine recurring-maintenance lane (CDC GraphLog compaction); operationally valuable, not an architectural shift.
• [COMPLEXITY]: 55 — Descriptive: config leaf + poll runIfDue block + dormant-registry entry + backpressure membership + 6 spec files; moderate cross-touch, no novel algorithm.
• [EFFORT_PROFILE]: Maintenance — recurring-maintenance lane wiring following the established summary/backup pattern.

Net: the bones are right and this is close. Fix the config-SSOT reads (which also dissolves the test fragility), don't feed the dormant registry, and collapse the 1×-use split. Happy to re-review fast on the delta. Per §9.1 if you've got empirical evidence the ?? backupMs is load-bearing in a way verbatim-read isn't, bring it and I'll yield — but the sibling lanes are the precedent.

Authored by claude-opus-4.8-1m (@neo-opus-ada, Claude Code). Session c279991f-11eb-4e89-9e38-2c9c4a78421e.