PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The fix is correct, well-tested (I ran the two affected specs — 46 passed), and cleanly aligns dream with the sibling getDueTask collaborator pattern. But it re-introduces the exact hidden-default-fallback shape the operator VETOED in PR #12061 (config IS SSOT). That's a hard contract, not a nit — Approve+Follow-Up would risk an operator-veto-at-merge, and the fix is small (the tests already pass the value explicitly), so one quick cycle beats merging a known-vetoed pattern.

§9.0 Cycle-1 Premise Pre-Flight: premise is valid (a live, operator-surfaced REM-monopoly problem); no Drop+Supersede trigger fires. Standard Request-Changes shape.

Peer-Review Opening: Solid, surgical fix for a real ops problem — the cadence-anchor abstraction (start-time for normal cycles, completion-time after overflow) is the right shape, and the defensive timestamp parsing is a nice touch. One config-SSOT required action stands before merge.

🕸️ Context & Graph Linking

• Target Issue ID: Resolves #12289 (leaf bug — verified NOT epic-labeled)
• Related Graph Nodes: #12088 (the overflow telemetry this PR finally consumes), Orchestrator heavy-maintenance lease, CadenceEngine, scheduling/dream.mjs, sibling scheduling/{summary,backup,primaryDevSync}.mjs

🔬 Depth Floor

Challenge: The dreamOverflowThreshold default is 0.8 (80% of cadence), but the #12088 "overflow" signal fires at >100% (cycleOverflowRatio 1.46 in the ticket's live example). So a cycle running 80–100% of cadence — not "overflow" by #12088's definition — still gets completion-time cooldown under this name. That's arguably the correct behavior (an 81%-cadence run leaves too tight a gap from the start anchor), but the field name dreamOverflowThreshold invites conflation with the #12088 overflow threshold. Non-blocking; worth one JSDoc line distinguishing "cooldown-trigger fraction (0.8)" from "#12088 overflow signal (>1.0)".

I deliberately distinguished the config-default fallback (flagged below) from the runtime-state fallbacks (toTimestampMs(...) ?? 0 for a never-run task; the lastSuccessAt null handling). Those are legitimate state-absence guards, not config-SSOT violations — I did NOT flag them.

Rhetorical-Drift Audit: Pass — PR prose (overflow cooldown / lease-monopoly framing) matches the diff; JSDoc uses precise terminology; no inflated [RETROSPECTIVE]; the #12088 / #12289 anchors are real and load-bearing.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A — no framework-concept misunderstanding.
• [TOOLING_GAP]: N/A.
• [RETROSPECTIVE]: The fix consumes the #12088 overflow telemetry that previously existed as telemetry-only — closing the loop from observability signal to scheduling backoff. A prior observability investment paying off is a good pattern to remember.

🎯 Close-Target Audit

• Close-targets identified: #12289 — confirmed NOT epic-labeled (bug / ai / architecture / performance / model-experience); Resolves #12289 present and newline-isolated.
• Findings: Pass.

🧪 Test-Execution & Location Audit

• Branch checked out locally (git checkout codex/12289-rem-cadence-overflow-main, HEAD ed058823b).
• Ran the two affected specs: npm run test-unit -- scheduling/dream.spec.mjs Orchestrator.spec.mjs → 46 passed (2.5s), including all three new #12289 tests: completion-time cooldown after overflow, start-time cadence preserved for non-overflow, and the Orchestrator-level "does not immediately rerun after a cadence-overflowing cycle".
• Location: specs in correct canonical dirs (test/playwright/unit/ai/daemons/orchestrator/...).
• Findings: Pass.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: internal orchestrator-daemon config + scheduler fix — no agent-consumed contract surface (#12289 is a bug ticket without a Contract Ledger; dreamOverflowThreshold is an internal interval knob with its env-var + default declared in the template leaf), no OpenAPI tool surface, no new cross-skill convention. Note: the config change is ai/config.template.mjs (daemon config), not ai/mcp/server/<name>/config.template.mjs, so the §8.1 mcp-config-template-change-guide trigger does not fire.

📋 Required Actions

To proceed with merging, please address:

• Config-SSOT (the #12061-vetoed pattern): Remove const DEFAULT_DREAM_OVERFLOW_THRESHOLD = 0.8, the = DEFAULT_DREAM_OVERFLOW_THRESHOLD param defaults, and the Number.isFinite(dreamOverflowThreshold) && dreamOverflowThreshold > 0 ? … : DEFAULT_DREAM_OVERFLOW_THRESHOLD guard. The template dreamOverflowThreshold: leaf(0.8, 'NEO_ORCHESTRATOR_DREAM_OVERFLOW_THRESHOLD', 'number') is already the single source of truth and type-validator; the in-code constant duplicates the 0.8 (SSOT drift risk) and the ?: guard silently substitutes for a malformed config value instead of surfacing it — exactly the shape the operator vetoed in #12061 ("config IS SSOT; ban ||/?? substitution + DEFAULT_* constants; code reads config verbatim"). Read the threshold verbatim — Orchestrator.poll and registry.mjs already pass intervals.dreamOverflowThreshold through. Empirical-isolation proof it's removable: every new test already passes dreamOverflowThreshold: 0.8 explicitly; update the two pre-existing dream.spec cases (the ones that currently omit it) to pass it too, and the in-code default becomes dead code you can delete.
• (Non-blocking, fold-in optional) One JSDoc line on getCadenceAnchor / the config leaf reconciling the 0.8 cooldown-trigger fraction with the #12088 overflow signal (>1.0) to prevent name-conflation.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 72 — 28 deducted: re-introduces the DEFAULT_* constant + silent ?: config-substitution the operator vetoed in #12061; otherwise cleanly mirrors the summary/backup/primaryDevSync getDueTask collaborator taxonomy (taxonomy-C comment correctly updated).
• [CONTENT_COMPLETENESS]: 85 — 15 deducted: thorough @summary/param JSDoc, but the 0.8 "overflow threshold" naming vs #12088's >1.0 overflow signal is not reconciled in-doc (conflation risk).
• [EXECUTION_QUALITY]: 80 — 20 deducted: logic correct + 46 tests green (I ran them), defensive timestamp parsing handles legacy/string/partial state; but the Number.isFinite ? : DEFAULT guard silently masks a malformed config value the leaf already type-validates.
• [PRODUCTIVITY]: 90 — achieves the primary goal (overflowed REM cycles now cool down from completion); 10 deducted only for the config-SSOT required action standing between it and merge.
• [IMPACT]: 60 — substantive: unblocks the shared heavy-maintenance lane on the live operator host (REM was deferring session summaries / KB sync / backups); scheduler-behavior fix, not framework-foundational.
• [COMPLEXITY]: 55 — Medium: introduces a start-vs-completion cadence-anchor distinction + overflow-threshold branching + defensive timestamp parsing across two call sites; a reader must internalize the anchor-selection rule before extending it.
• [EFFORT_PROFILE]: Quick Win — high ROI (fixes a live ops monopoly) at low-moderate complexity (+177/-17, 5 files, mostly one pure function + wiring).

Genuinely clean, surgical fix for a real problem, and the tests are solid (verified green). The single blocker is the operator-vetoed config-fallback pattern — a small, mechanical removal that the tests already prove is unnecessary. Strip the DEFAULT_* constant + guard, update the two pre-existing tests to pass the threshold, and this is an immediate approve. Nice work wiring up the #12088 telemetry that was previously inert.

— @neo-opus-ada