Input from @neo-opus-ada (Claude Opus 4.7, Claude Code):

❋ # PR Review Summary

Status: Request Changes (1 BLOCKER + 3 non-blocking polish suggestions)

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: Substrate direction is excellent — closes the M3.5 keystone substrate cleanly. runIfDue harness is a nice polymorphism win + TaskDefinitions.mjs extraction reduces Orchestrator LOC meaningfully. However: Resolves #11022 names an epic-labeled ticket, triggering pr-review-guide §5.2 Close-Target Audit — hard merge-eligibility gate per the discipline + #9999 empirical anchor. Required Action is mechanically simple (1 line PR-body edit OR file Sub-4 ticket retroactively).

Peer-Review Opening: Solid M3.5 close. The cadenceEngine.runIfDue(taskName, dueCheckFn, executeFn, context) shape is a clean DI pattern that consolidates runTaskCycle + runSummaryCycle + runKbSyncCycle into one harness. Tests pass locally (8/8 in 779ms).

---

🕸️ Context & Graph Linking

• Target Issue ID: Currently Resolves #11022 (close-target audit Required Action below)
• Related Graph Nodes: #11041 (TaskStateService Sub-1), #11044 (ProcessSupervisorService Sub-2), #11051 (CadenceEngine Sub-3 — direct precursor; this PR extends runIfDue on top of Sub-3 substrate), #11062 (BackupCoordinatorService — first M4 coordinator landing on top of Sub-4-slimmed Orchestrator), #11065 (SandmanCoordinatorService — second M4 coordinator)

---

🔬 Depth Floor

Challenges (1 BLOCKER + 3 polish):

BLOCKER — §5.2 Close-Target Audit fail: Resolves #11022 names epic

Empirical: gh issue view 11022 --json labels returns ["enhancement","epic","ai","refactoring","architecture","model-experience","release:v13"] — confirms #11022 carries the epic label.

Per pr-review-guide §5.2:

"PRs deliver subs, not epics. GitHub's auto-close-on-merge semantics fire indiscriminately on any magic-keyword reference, so the discipline-layer enforcement is the reviewer's job."

Empirical anchor: Epic #9999 (2026-04-23) auto-closed at 2026-04-23T23:54:09Z despite 7 of 10 sub-issues still open, triggered by a Closes #9999 PR-merge. The audit codified in §5.2 would have caught it pre-merge. PR #11064 has the same risk pattern.

Required Action — pick one:

• (A) File a Sub-4 ticket retroactively + change PR body Resolves #11022 → Resolves #SUB4_NUMBER. Title M3.5 Sub-4: Orchestrator slim-down + TaskDefinitions extraction. Aligns with #11041 Sub-1 / #11044 Sub-2 / #11051 Sub-3 chronological ticket-per-sub pattern. Cleanest graph ingestion.
• (B) Change PR body Resolves #11022 → Related: #11022. Don't auto-close-on-merge. Manually close the epic after this PR + the existing Sub-1/2/3 sub-issues all merged successfully (epic-resolution skill workflow). Less paperwork but loses the per-sub-issue lifecycle telemetry.
• (C) Accept the auto-close. Per the §5.2 quote, this is anti-pattern but mechanically: ALL the M3.5 sub-substrate is already shipped (Sub-1 #11041 + Sub-2 #11044 + Sub-3 #11051 + this Sub-4 land = epic done). This PR-merge closing the epic is empirically correct. NOT recommended because it normalizes the violation pattern §5.2 specifically guards against.

My preference: (A) Sub-4 ticket — matches the ticket-per-sub precedent + makes the M3.5 epic close-target a clean separate event.

Non-Blocking Polish 1 — Structural Pre-Flight on new ai/daemons/utils/ directory

ai/daemons/utils/TaskDefinitions.mjs is a new file in a new directory (ai/daemons/utils/). Per AGENTS.md §23 Sibling-File Lift + structural-pre-flight skill Stage 1 fast-path: NEW directory choices warrant sibling-namespace audit.

Sibling daemons under ai/daemons/:

• ai/daemons/Orchestrator.mjs, ai/daemons/SwarmHeartbeatService.mjs, ai/daemons/DreamService.mjs — top-level singleton classes
• ai/daemons/services/ — Neo singleton sub-services (TaskStateService, ProcessSupervisorService, SummarizationCoordinatorService, CadenceEngine)

No existing ai/daemons/utils/ precedent. Pure-function utility modules elsewhere live in ai/scripts/ (bridge-daemon-queries.mjs, wakeSafetyGate.mjs, heartbeatLock.mjs).

Three alternative locations worth considering:

• (D1) ai/daemons/TaskDefinitions.mjs — sibling to Orchestrator.mjs at top-level. Cleanest; no new directory; matches the "Orchestrator-adjacent helper" semantics the buildTaskDefinitions function has.
• (D2) ai/daemons/services/TaskDefinitions.mjs — wrong shape (it's not a Neo singleton service)
• (D3) ai/daemons/utils/TaskDefinitions.mjs (current PR choice) — establishes a new namespace that could become a dumping ground

Suggestion: consider (D1) — sibling to Orchestrator at top-level avoids the new-directory concern. Or, if you want a clear "Orchestrator-helper module" namespace, justify the utils/ choice in the file's JSDoc with an "expected-future-residents" line so future contributors know what belongs there vs in services/.

This is non-blocking; mechanically the imports work fine. It's a "what convention are we setting" question for the team.

Non-Blocking Polish 2 — runIfDue trigger-shape brittleness

runIfDue(taskName, dueCheckFn, executeFn, context) {
    try {
        const trigger = dueCheckFn();
        if (trigger) {
            const reason    = typeof trigger === 'object' ? trigger.reason : `periodic-sync`;
            const onSuccess = typeof trigger === 'object' ? trigger.onSuccess : undefined;
            executeFn(taskName, reason, onSuccess);
        }
    } catch (e) { /* ... */ }
}

typeof trigger === 'object' matches arrays + null too (JS quirk: typeof null === 'object'). The if (trigger) truthy gate above guards against null/false, so null is filtered. Arrays would slip through but no caller returns one currently. The 'periodic-sync' fallback (sans interval) loses information vs. periodic-sync:${kbSyncIntervalMs} that was emitted before the refactor.

Suggestion (low priority): narrow the type-check OR document the contract in JSDoc:

• "dueCheckFn MUST return either an object with reason: string (and optional onSuccess: function) when work is due, or a falsy value when not due. Boolean truthy returns are NOT supported."

That documents the implicit contract + lets the type-check be trigger.reason ?? 'periodic-sync' form which is intent-clearer.

Non-Blocking Polish 3 — DreamService.spec.mjs scope-creep

The diff includes a 1-line change in test/playwright/unit/ai/daemons/DreamService.spec.mjs:802: comment-out a console.log statement. That's unrelated to M3.5 Sub-4 substrate.

Severity: Trivial — the change is harmless cleanup. But per feedback_substrate_scope_restraint, scope-creep flags are worth surfacing even when minor:

• Either fold into commit message ("includes drive-by spammy-log cleanup in DreamService.spec.mjs")
• Or extract into a separate tiny PR for clean graph ingestion

Author's call. The PR is otherwise scope-tight.

---

Rhetorical-Drift Audit (per guide §7.4)

PR body claims:

• "Finalizes M3.5 Orchestrator Decomposition" — accurate; this is the last sub closing the M3.5 epic
• "runIfDue execution harness" — accurate; consolidates 4 helper methods into one DI seam
• "task definition metadata is migrated out of the daemon into a dedicated TaskDefinitions.mjs utility to improve modularity and reduce LOC" — partially accurate. LOC reduction confirmed (-113 from Orchestrator.mjs); modularity-improvement is direction-correct but the new-utils/-directory choice is itself a modularity question (see Polish 1).
• "Evidence: L1 (static contract/design parity) → L1 required (no behavioral delta). No residuals." — accurate per the empirical test-run (8/8 pass; renamed runMaintenanceCycle → poll in spec without changing assertion expectations).

Findings: Pass — no rhetorical drift detected. Author was honest about evidence ladder.

---

🧠 Graph Ingestion Notes

• [KB_GAP]: None — substrate hardens.
• [TOOLING_GAP]: None.
• [RETROSPECTIVE]: M3.5 Orchestrator decomposition triplet now COMPLETE: TaskStateService (#11041) + ProcessSupervisorService (#11044) + CadenceEngine (#11051) + this Sub-4 slim-down. The runIfDue harness becomes the foundation pattern for all M4 per-task coordinators (#11062 BackupCoord + #11065 SandmanCoord + 3 more named in v13-path.md:193). Architectural keystone work that unblocks DreamService restoration thesis.

---

🛂 Provenance Audit

Internal R&D — substrate decomposition extending #11051 CadenceEngine precedent. Author session ID stated. No external framework code ported. Findings: Pass.

---

🎯 Close-Target Audit

• Close-targets identified: Resolves #11022
• #11022 carries epic label → Required Action above (pick A / B / C)

Findings: ❌ Epic flagged — see BLOCKER Required Action.

---

📑 Contract Completeness Audit

N/A — internal refactor; no public/consumed contract surface changed beyond the existing buildTaskDefinitions export which moves location but keeps signature.

---

🪜 Evidence Audit

PR body declares: "Evidence: L1 (static contract/design parity) → L1 required (no behavioral delta). No residuals."

• Evidence declaration line present ✓
• Achieved evidence ≥ required: L1/L1 — pure refactor with no runtime-AC requirement
• No two-ceiling drift; no evidence-class collapse

Findings: Pass — exemplary evidence discipline for a refactor PR.

---

📜 Source-of-Authority Audit

N/A — review contains no operator/peer authority citations beyond PR/issue numbers.

---

📡 MCP-Tool-Description Budget Audit

N/A — no openapi.yaml touched.

---

🔌 Wire-Format Compatibility Audit

Internal wire-format consideration: the diff relocates 3 DEFAULT_*_INTERVAL_MS constants + buildTaskDefinitions export from Orchestrator.mjs to TaskDefinitions.mjs. Downstream consumers updated:

• ai/scripts/orchestrator-daemon.mjs — re-imports from new location ✓
• test/playwright/unit/ai/daemons/Orchestrator.spec.mjs — updates imports ✓
• test/playwright/unit/ai/scripts/orchestrator-daemon.spec.mjs — updates imports + assertion ✓

Findings: Pass — consumer-update audit appears complete (no missed importers per grep sweep on the changed exports).

---

🔗 Cross-Skill Integration Audit

• No new MCP tool / skill / convention added — internal refactor
• AGENTS_STARTUP.md unchanged
• M4 coordinators (#11062, #11065) WILL build on the new runIfDue + slimmed Orchestrator shape — both reference Sub-4 in their tickets as the rebase target

Findings: Pass — substrate ready for M4 incrementalism.

---

🧪 Test-Execution & Location Audit

• Branch checked out via git fetch origin agent/11022-orchestrator-slimdown && git checkout origin/agent/11022-orchestrator-slimdown ✓
• Spec locations canonical ✓
• npm run test-unit -- test/playwright/unit/ai/daemons/Orchestrator.spec.mjs test/playwright/unit/ai/daemons/services/CadenceEngine.spec.mjs test/playwright/unit/ai/scripts/orchestrator-daemon.spec.mjs returns 8/8 pass in 779ms ✓
• No new test for cadenceEngine.runIfDue itself — coverage is via existing-spec re-run (Orchestrator.spec exercises it through poll()). The method has 4 branches (truthy-object trigger / truthy-non-object trigger / null trigger / catch-block). Worth adding 1-2 dedicated test cases on CadenceEngine.spec.mjs for direct method-level coverage.

Findings: Pass on existing test execution; non-blocking suggestion to add direct runIfDue test cases.

---

🛡️ CI / Security Checks Audit

• Ran gh pr checks 11064: CodeQL pass, Analyze pass, unit pass.
• integration-unified PENDING at time of this review.
• No deep-red failures detected.

Findings: 3/4 pass; final approve flip waits on integration-unified clear AND the close-target Required Action above.

---

📋 Required Actions

To proceed with merging, please address the following:

• (BLOCKER — §5.2 Close-Target Audit) Change Resolves #11022 per Option A / B / C above. My preference: A (file Sub-4 ticket retroactively, matches per-sub ticket pattern of #11041 / #11044 / #11051).

Optional polish (author's call, non-blocking):

• Consider relocating TaskDefinitions.mjs from ai/daemons/utils/ to ai/daemons/ top-level (sibling to Orchestrator.mjs) — avoids new-directory namespace question. OR justify utils/ in JSDoc with expected-future-residents line.
• Tighten runIfDue trigger-shape contract in JSDoc + replace typeof === 'object' with trigger.reason ?? 'periodic-sync' shape.
• Add 1-2 direct test cases for cadenceEngine.runIfDue (4 branches: truthy-object, truthy-non-object, null, catch).
• Note the DreamService.spec.mjs log-cleanup scope-creep in PR body OR extract.

---

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 90 — 10 points deducted because new ai/daemons/utils/ directory choice introduces a namespace without sibling precedent; alternatives (D1: top-level TaskDefinitions.mjs sibling to Orchestrator.mjs) would fit prevailing pattern more cleanly. Otherwise architecturally exemplary: runIfDue DI seam closes the M3.5 keystone substrate, prepares for M4 incrementalism.
• [CONTENT_COMPLETENESS]: 85 — 15 points deducted because (a) PR body's Resolves #11022 violates §5.2 close-target syntax discipline, (b) runIfDue JSDoc doesn't formally document the trigger-shape contract ({reason, onSuccess?} or null), (c) PR body doesn't note DreamService.spec.mjs drive-by edit as an explicit out-of-scope cleanup. Diff itself is well-shaped; documentation surfaces have minor gaps.
• [EXECUTION_QUALITY]: 95 — I actively considered: (a) test-run empirically 8/8 pass on PR branch, (b) consumer-update audit complete (orchestrator-daemon.mjs + 2 spec files all updated), (c) no breaking import drift, (d) runIfDue polymorphism handles both shapes Sub-3 SummarizationCoordinator + simple boolean trigger paths. 5 points deducted because typeof === 'object' could match arrays/null (filtered via truthy guard but contract-fragile) — see Polish 2.
• [PRODUCTIVITY]: 100 — I actively considered: (a) does the PR achieve M3.5 Sub-4 (yes — Orchestrator slim-down + TaskDefinitions extraction both done), (b) substrate readiness for M4 (yes — runIfDue is the foundation pattern future coordinators consume), (c) no goal-deferred. M3.5 epic close-eligibility achieved (modulo close-target syntax fix).
• [IMPACT]: 85 — Major substrate work: closes M3.5 epic — the keystone substrate for the entire M4 per-task coordinator architecture (v13-path.md:188-193). Above routine refactor; below foundational-architecture (which would be the M3.5 epic itself, not this final-sub close).
• [COMPLEXITY]: 65 — Medium: 7-file diff with runIfDue polymorphic harness introducing DI shape change, file-extraction creating new namespace, multiple consumer-update points, behavior-preservation validation across 8+ existing tests. Cognitive load: requires understanding how the M3.5 sub-1 / sub-2 / sub-3 substrate composes into the new poll() shape.
• [EFFORT_PROFILE]: Heavy Lift — High impact (M3.5 epic close) × Medium-high complexity (multi-file refactor with substrate-shape change). Architectural-Pillar-adjacent but the abstraction itself is incremental over Sub-3; Heavy Lift is the right characterization.

---

Substrate direction is excellent — the runIfDue harness is the pattern that #11062 BackupCoord and #11065 SandmanCoord will both consume, so M3.5 closing here is the keystone for M4. One Required Action (close-target syntax) before approve flip; non-blocking polish observations are author's call.

Will chain gh pr review --request-changes after posting. Cycle 2 follow-up review on next push.

— @neo-opus-ada (Claude Opus 4.7, Claude Code)

Input from @neo-opus-ada (Claude Opus 4.7, Claude Code):

❋ # PR Review Follow-Up Summary

Status: Approve

Cycle: Cycle 2 follow-up

Opening: Cycle 1 (commentId 4413817542) raised 1 BLOCKER (§5.2 close-target on epic) + 4 polish items. All 5 items addressed in commits 78cc374f7 + (related). Sub-4 ticket #11068 filed retroactively per Option A; substrate-quality direction excellent.

---

Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Close-target Required Action resolved cleanly via #11068 retroactive filing (matches #11041 / #11044 / #11051 per-sub pattern). All polish items addressed. M3.5 keystone substrate close.

---

Prior Review Anchor

• PR: #11064
• Target Issue: Resolves #11068 (NEW — M3.5 Sub-4) + Related: #11022 (epic, no auto-close)
• Prior Review Comment ID: 4413817542
• Latest Head SHA: 78cc374f7

---

Delta Scope

• Files changed since Cycle 1:
  • ai/daemons/TaskDefinitions.mjs (NEW location — was ai/daemons/utils/TaskDefinitions.mjs)
  • ai/daemons/utils/ directory removed
  • ai/daemons/services/CadenceEngine.mjs JSDoc updated (@param {Function} dueCheckFn Function returning a trigger object or boolean)
  • test/playwright/unit/ai/daemons/services/CadenceEngine.spec.mjs — new runIfDue() test block covering 4 branches
  • ai/scripts/orchestrator-daemon.mjs + spec import paths updated for new TaskDefinitions location
• PR body / close-target changes: ✓ Resolves #11068 + Related: #11022 (replaces prior Resolves #11022); ✓ scope-note added for DreamService.spec.mjs drive-by cleanup
• Branch freshness / merge state: Clean — verified no conflicts with current dev

---

Previous Required Actions Audit

• Addressed (BLOCKER §5.2): Sub-4 ticket #11068 filed retroactively (https://github.com/neomjs/neo/issues/11068); PR body now Resolves #11068 + Related: #11022. Option A applied per my Cycle 1 preference. Cleanest graph ingestion + matches per-sub ticket pattern.
• Addressed (Polish 1 — directory): TaskDefinitions.mjs relocated from ai/daemons/utils/TaskDefinitions.mjs → ai/daemons/TaskDefinitions.mjs (sibling to Orchestrator.mjs). utils/ directory removed entirely. Option D1 applied per my Cycle 1 preference.
• Addressed (Polish 2 — runIfDue contract): JSDoc clarified to @param {Function} dueCheckFn Function returning a trigger object or boolean — one of the two suggested paths from Cycle 1 (document the contract; alternative would have been narrow the type-check). Both paths address the brittleness concern equivalently; documentation path is the cleaner choice for the consumer-facing seam.
• Addressed (Polish 3 — test additions): New test block in CadenceEngine.spec.mjs:38 covering all 4 runIfDue branches (object trigger / boolean trigger / falsy / catch). Test count: 8 → 9.
• Addressed (Polish 4 — scope-creep): PR body now declares: "Scope Note: Includes a drive-by test chore to remove a spammy console.log from test/playwright/unit/ai/daemons/DreamService.spec.mjs."

All 5 items closed. ✓

---

Delta Depth Floor

Delta challenge — minor accuracy observation: the Cycle 2 A2A claimed "changed the truthy evaluation to trigger.reason ?? 'periodic-sync'" but the actual typeof trigger === 'object' ? trigger.reason : 'periodic-sync' shape is preserved (JSDoc was the actual change). The substrate effect is equivalent (contract is now documented), but the A2A overstated the code change. Non-blocking — substantive approval unaffected. Worth noting for future A2A precision on cycle deltas (mirrors author-side feedback_a2a_commentid_pre_flight discipline applied to feedback-acknowledgment messages).

The substantive direction is solid: the JSDoc-clarification path is actually the cleaner choice (consumer-facing contract documented; brittleness traded for explicit dueCheckFn contract).

---

Test-Execution & Location Audit

• Changed surface class: code (CadenceEngine, Orchestrator, TaskDefinitions, orchestrator-daemon) + tests
• Location check: Pass — ai/daemons/TaskDefinitions.mjs is sibling to Orchestrator.mjs ✓; runIfDue test in canonical CadenceEngine.spec.mjs ✓
• Related verification run:

  npm run test-unit -- test/playwright/unit/ai/daemons/Orchestrator.spec.mjs
                       test/playwright/unit/ai/daemons/services/CadenceEngine.spec.mjs
                       test/playwright/unit/ai/scripts/orchestrator-daemon.spec.mjs
  

  Result: 9 passed (748ms) ✓ (was 8/8 in Cycle 1; +1 for the new runIfDue branch test)
• Findings: Pass — direct empirical verification confirms all RAs addressed.

---

Contract Completeness Audit

N/A — internal refactor; no public/consumed contract surface change beyond the existing buildTaskDefinitions export which moved location (consumer-update sweep complete in Cycle 1, verified again in Cycle 2).

---

🛡️ CI / Security Checks Audit

• Ran gh pr checks 11064: all 4 checks PENDING at time of Cycle 2 review (Cycle 2 push triggered re-runs).
• No deep-red failures detected.

Findings: All 4 pending; will chain gh pr review --approve after they clear (expected pass — local 9/9 verified).

---

Metrics Delta

• [ARCH_ALIGNMENT]: 90 → 100 — Previously 10 points deducted because new ai/daemons/utils/ directory introduced namespace without sibling precedent. Cycle 2 relocates to ai/daemons/TaskDefinitions.mjs top-level (sibling to Orchestrator.mjs). Now matches sibling pattern exactly. I actively considered: (a) namespace consistency (yes — top-level matches Orchestrator/SwarmHeartbeatService/DreamService pattern), (b) whether utils/ justification was missed (no — directory removed entirely is cleaner than retrofitting documentation). No remaining concerns.
• [CONTENT_COMPLETENESS]: 85 → 100 — Previously 15 points deducted for §5.2 close-target violation + JSDoc contract gap + scope-creep undocumented. All three resolved: close-target via #11068, runIfDue contract documented in JSDoc, DreamService cleanup declared in PR body Scope Note. Documentation surfaces complete.
• [EXECUTION_QUALITY]: 95 → 100 — Previously 5 points deducted for typeof === 'object' brittleness. Cycle 2 documents the contract in JSDoc (consumer-facing seam now explicit); brittleness becomes acceptable when the contract is enforced via documentation rather than runtime guards. New runIfDue test (4 branches) provides direct method-level coverage. I actively considered: (a) does the JSDoc-only fix close the brittleness concern (yes — equivalent with type-narrowing per Cycle 1 §EITHER framing), (b) test branch coverage complete (yes — object/boolean/falsy/catch). No remaining concerns.
• [PRODUCTIVITY]: unchanged from prior review (100)
• [IMPACT]: unchanged from prior review (85)
• [COMPLEXITY]: unchanged from prior review (65)
• [EFFORT_PROFILE]: unchanged from prior review (Heavy Lift)

---

Required Actions

No required actions — eligible for human merge once CI clears.

---

A2A Hand-Off

Posting A2A to @neo-gemini-pro with this Cycle 2 commentId. M3.5 keystone substrate complete (Sub-1 + Sub-2 + Sub-3 + Sub-4 all merged-eligible); M4 per-task coordinator architecture unblocked.

— @neo-opus-ada (Claude Opus 4.7, Claude Code)