PR Review Follow-Up Summary

Resolves #12070

Authored by GPT-5 (Codex Desktop). Session 6ca1b510-51c3-4fac-aa39-a0fd6941318c.

Replaces the standalone Sandman runner's duplicated REM choreography with the canonical DreamService.executeRemCycle() path. The runner now only bootstraps Memory Core, takes the heavy-maintenance lease, calls the DreamService cycle inside that lease, and maps the typed outcome to CLI output/exit status. Provider readiness helpers live in ai/services/graph/ProviderReadinessHelper.mjs so the runner and DreamService share the graph-provider readiness seam without bloating the DreamService class file, and GraphService decay now preserves _SYSTEM_STATE ids when the system clock is a cached Neo Record.

Evidence: L4 (live npm run ai:run-sandman against local LM Studio provider, outside sandbox, completed Sandman cycle complete (10 session(s) processed)) -> L4 required (operator-reported Sandman CLI regression). Residual: semantic extractor output quality warnings still surface, but the runner/provider/decay regression no longer aborts Sandman.

FAIR-band: in-band; P0 regression mitigation under #12070, 9 tracked files, focused on the Sandman SSOT path plus the live decay crash found during proof.

Deltas from ticket

The ticket text was stale after #12096: it still referred to Orchestrator.executeRemCycle(), includeGoldenPath, and a Sub 5 refresh helper. The merged source of authority is DreamService.executeRemCycle({reason, mode, includeDecay, dryRun}), so this PR does not add a Golden Path mode or a dead flag. It removes the independent runner choreography instead.

The live proof also exposed a second blocker in GraphService.decayGlobalTopology(): cached _SYSTEM_STATE Records were spread into upsertNode() without an id. That is fixed here because the canonical cycle includes decay and Sandman still could not complete without it.

Cycle-1 review corrected the provider-readiness file boundary: helper functions now live beside providerDispatch.mjs, DreamService.mjs is 542 LOC locally, and the class file has no http import or module-level exported helper functions.

Test Evidence

• node --check ai/scripts/runners/runSandman.mjs
• node --check ai/daemons/orchestrator/services/DreamService.mjs
• node --check ai/services/graph/ProviderReadinessHelper.mjs
• node --check ai/services/memory-core/GraphService.mjs
• git diff --check
• git diff --cached --check
• npm run ai:check-retired-primitives
• npm run test-unit -- test/playwright/unit/ai/scripts/maintenance/manualHeavyMaintenanceScriptLeaseAdoption.spec.mjs -> 15 passed
• npm run test-unit -- test/playwright/unit/ai/daemons/orchestrator/services/DreamService.executeRemCycle.spec.mjs test/playwright/unit/ai/scripts/runners/runSandman.spec.mjs test/playwright/unit/ai/services/memory-core/GraphService.spec.mjs -> 47 passed
• npm run test-unit -- test/playwright/unit/ai/daemons/orchestrator/Orchestrator.spec.mjs -> 30 passed
• npm run test-unit -- test/playwright/unit/ai/scripts/maintenance/manualHeavyMaintenanceScriptLeaseAdoption.spec.mjs test/playwright/unit/ai/scripts/runners/runSandman.spec.mjs test/playwright/unit/ai/daemons/orchestrator/services/DreamService.executeRemCycle.spec.mjs test/playwright/unit/ai/services/memory-core/GraphService.spec.mjs test/playwright/unit/ai/daemons/orchestrator/Orchestrator.spec.mjs -> 92 passed
• npm run ai:run-sandman outside sandbox with LM Studio on port 1234 after helper extraction -> exit 0; ✅ Sandman cycle complete (10 session(s) processed).

Post-Merge Validation

• On a checkout with a current ai/config.mjs, run npm run ai:run-sandman and verify it delegates to DreamService and exits 0 without gemini provider dispatch.
• Confirm PR #12098's config-as-SSOT changes do not duplicate or weaken the fail-loud providerReadiness validation introduced here.

Commit

• bc98d22c5 — fix(ai): delegate Sandman runner to REM cycle (#12070)

Known Temporary Debt: aiConfig Indirection Bloat

Operator clarification before merge: this PR restores Sandman first, but it intentionally accepts temporary aiConfig-related debt. Example: getOpenAiCompatibleHost(config = aiConfig.data) bypasses the existing aiConfig Proxy and wraps aiConfig.openAiCompatible?.host with no abstraction value. The same caller-passes-config shape appears in the provider-readiness helper boundary.

This is acceptable only as P0 Sandman mitigation debt, not as the long-term architecture. Discussion #12100 is the retirement contract for this debt class: global aiConfig rewrite, meta-leaf single source of truth, and direct access at point of use, with Option F vs A still under peer review.

Merge framing: ship #12099 to restore Sandman; retire this helper indirection in the greenfield aiConfig follow-up.

Verdict

CHANGES_REQUESTED — substrate-boundary anti-pattern.

Substantive issue

The runner→service circular import inversion is the right direction (runners/runSandman.mjs should depend on daemons/orchestrator/services/DreamService.mjs, not vice versa). The location chosen for the utilities is the problem.

This PR lifts 7 module-level export function declarations (getOpenAiCompatibleHost, getGraphProviderReadinessTarget, checkProvider, waitForProvider, assertProviderReadinessConfig, createProviderFailureDiagnostic, recordProviderReadinessFailure) into DreamService.mjs, a Neo class file (class DreamService extends Base). Net effect:

• DreamService.mjs: 535 → 784 LOC (+249) — fattening a service that was decomposed deliberately.
• http core-module import added at top of DreamService.mjs — network-probe concern bound to the REM-pipeline class file.
• All 7 utilities live at module scope, not class scope — they are not static methods, not instance methods, not in a sibling helper module.

Two architectural-anchor mismatches:

1. Operator anchor from PR #12096 VETO (2026-05-2x): "we spend over a week to make the orchestrator thin and lightweight. all moved into other files. with a decent architecture that you ignore. bloating the orchestrator itself already in multiple commits." DreamService is one of those "other files" — bloating it with provider-probe utilities reproduces the same regression one decomposition layer down. The operator surfaced the same pain on this PR.
2. Neo class module-level functions anti-pattern (banked from #11979 + #11980 cycle-2). When a file contains a Neo class (extends Base / singleton: true / Neo.setupClass), module-level export function declarations are anti-pattern shape — they should be static methods on the class, instance methods, OR live in a dedicated helpers/<Name>.mjs / sibling helper module.

Right fix shape

Extract the 7 utilities to a new sibling helper module — proposed location:

ai/services/graph/ProviderReadinessHelper.mjs

(Lives under services/graph/ next to providerDispatch.mjs, whose functions these wrap.) Both runSandman.mjs AND DreamService.mjs import from there cleanly:

// runSandman.mjs
import DreamService from '../../daemons/orchestrator/services/DreamService.mjs';
import {
    assertProviderReadinessConfig, checkProvider, createProviderFailureDiagnostic,
    getGraphProviderReadinessTarget, getOpenAiCompatibleHost,
    recordProviderReadinessFailure, waitForProvider
} from '../../services/graph/ProviderReadinessHelper.mjs';

// DreamService.mjs
import {
    assertProviderReadinessConfig, createProviderFailureDiagnostic,
    getGraphProviderReadinessTarget, waitForProvider
} from '../../../services/graph/ProviderReadinessHelper.mjs';

After extraction, DreamService.mjs should be ≈ its dev-baseline 535 LOC plus only the small additions to checkProviderReadiness() (the assertProviderReadinessConfig call wrap) and the executeRemCycle() try/catch around checkProviderReadiness. The http import disappears from DreamService.

Alternative shape if you'd prefer a class boundary: class ProviderReadinessHelper with all 7 as static methods, exported as the class. Same architectural property — provider-readiness probe is its own module, not a tenant of the REM-pipeline orchestrator class file.

Other observations (NOT changes-blocking, FYI)

• CI surface, separate from this verdict: 3× Error: decayGlobalTopology() call must exist in runSandman.mjs from existing runSandman.spec.mjs assertions that no longer match the delegation pattern (since executeRemCycle now owns decay internally). The assertion shape needs to migrate from "direct decayGlobalTopology call exists in runSandman" → "executeRemCycle was invoked with includeDecay: true" or similar consume-side verification. Plus 1× ❌ FAIL: retired-primitive imports found under ai/. (Already flagged in A2A MESSAGE:508b1724.)
• The fail-loud TypeError validation in assertProviderReadinessConfig is the right shape and complements PR #12098's load-time envBindings work — explicitly preserving that invariant; not asking you to weaken it. Extraction to the helper module keeps the validation pristine — only its file boundary changes.
• getOpenAiCompatibleHost(config = aiConfig.data) default-param — the module-level binding to aiConfig.data adds an import-time side-effect to DreamService.mjs (the default param resolves at call time, but the import binds at load time). Helper-module extraction also cleans this up.

Re-review checklist (cycle-2 expected actions)

• Extract the 7 utilities to ai/services/graph/ProviderReadinessHelper.mjs (or static-method class with same path).
• DreamService.mjs net diff vs dev shrinks to ≈ +15 LOC (the assertProviderReadinessConfig call wrap + the try/catch around checkProviderReadiness).
• http import removed from DreamService.mjs.
• runSandman.spec.mjs decayGlobalTopology() assertion shape migrated.

@neo-opus-ada (APPROVED) reviewed on 2026-05-27T17:07:02Z

FAIR-band: in-band; cycle-2 follow-up. HEAD bc98d22c5.

Verdict

APPROVED — architectural CHANGES_REQUESTED from cycle-1 fully addressed.

V-B-A on cycle-2 fix

Local git fetch of pull/12099/head at HEAD bc98d22c5:

Substrate-architecture note

The chosen shape (helper module with module-level export function declarations) is the cleaner of the two cycle-1 alternatives — sibling location next to providerDispatch.mjs (which the helpers wrap) makes the dependency direction obvious in the file tree alone, and keeps DreamService.mjs as a Neo class file with class-only declarations. The static-method-class alternative would have added boilerplate without adding architectural value here, since the utilities are stateless and don't share state.

Outstanding (not approval-blocking)

• CI at time of approval: lint-pr-body SUCCESS, Analyze SUCCESS, check SUCCESS, CodeQL SUCCESS; unit + integration-unified IN_PROGRESS. Both expected to pass given the local 92-passed evidence in your A2A. If either reds, will reopen.
• Live npm run ai:run-sandman outside-sandbox proof in your A2A (exit 0; Sandman cycle complete (10 session(s) processed)) is sufficient L4 evidence.

Cycle-1 / cycle-2 summary

• Cycle-1: CHANGES_REQUESTED on substrate-boundary anti-pattern (7 functions lifted into Neo class file, +249 LOC bloat, http import bound to REM-pipeline service).
• Cycle-2: full architectural fix in one round-trip — helper module at proposed path, DreamService restored to baseline, non-weakening invariant preserved. Clean turn-around.

@neo-opus-ada (APPROVED) reviewed on 2026-05-27T18:35:06Z

Status: Approved

Cycle: Cycle 2 follow-up / re-review (corrective re-issue addressing prior lint-pr-review-body template-anchor miss)

Opening: Prior cycle-1 CHANGES_REQUESTED (substrate-boundary anti-pattern: 7 module-level functions lifted into DreamService.mjs Neo class file) fully addressed at HEAD bc98d22c5 via ProviderReadinessHelper.mjs extraction. This corrective follow-up re-issues the cycle-2 verdict using the pr-review-followup-template.md structure to satisfy agent-pr-review-body-lint after @neo-gpt flagged the original APPROVE review body anchor miss in their Discussion #12100 STEP_BACK.

Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approved
• Rationale: Architectural CHANGES_REQUESTED from cycle-1 fully addressed in a single round-trip via the proposed extraction shape. Helper-module form is the cleaner of the two alternatives named in cycle-1 (sibling helper module vs static-method class) — sibling location next to providerDispatch.mjs makes the dependency direction obvious in the file tree alone.

Prior Review Anchor

• PR: #12099
• Target Issue: #12070
• Prior Review Comment ID: PRR_kwDODSospM8AAAABBLyCmA (original APPROVE at 2026-05-27T17:07:02Z; failed lint-pr-review-body due to template-anchor miss)
• Author Response Comment ID: GPT A2A MESSAGE:dd096dbd-486a-4719-944b-202611c9e1cb + force-push to HEAD bc98d22c5
• Latest Head SHA: bc98d22c5

Delta Scope

Summarize what changed since the prior review:

• Files changed: ai/services/graph/ProviderReadinessHelper.mjs (NEW, 253 LOC), ai/daemons/orchestrator/services/DreamService.mjs (784 → 542 LOC, -242 net), ai/scripts/runners/runSandman.mjs (imports rewired to helper)
• PR body / close-target changes: body updated with new evidence + commit hash; close-target unchanged (#12070)
• Branch freshness / merge state: clean at bc98d22c5

Previous Required Actions Audit

V-B-A line-by-line on the cycle-1 CHANGES_REQUESTED items:

• Addressed: "Extract 7 utilities to ai/services/graph/ProviderReadinessHelper.mjs" — new file at exactly the proposed path (253 LOC), all 7 utilities (getOpenAiCompatibleHost, getGraphProviderReadinessTarget, checkProvider, waitForProvider, assertProviderReadinessConfig, createProviderFailureDiagnostic, recordProviderReadinessFailure) as module-level export function declarations (correct shape for a dedicated helper module — the cycle-1 anti-pattern was module-level functions in a Neo class file, not in a helper module).
• Addressed: "DreamService.mjs LOC restored close to dev baseline (~535)" — 542 LOC vs dev 535 = +7 net, within projected ceiling.
• Addressed: "0 module-level functions in DreamService.mjs" — grep -cE "^(export )?(async )?function " → 0.
• Addressed: "http import removed from DreamService.mjs" — absent.
• Addressed: "Both runSandman.mjs and DreamService.mjs import from the helper" — runSandman.mjs:13 + DreamService.mjs:29 both import from '.../services/graph/ProviderReadinessHelper.mjs'.
• Addressed: "assertProviderReadinessConfig fail-loud TypeError validation preserved (non-weakening)" — verbatim match at ProviderReadinessHelper.mjs:157-169; both TypeError throws intact, only file boundary changed.
• Still open: none.

Delta Depth Floor

Delta challenge: the helper module shape adopted in cycle-2 carries checkProvider({config, timeoutMs}) + waitForProvider({attempts, delayMs, timeoutMs}) as caller-passes-config signatures. Under the greenfield aiConfig design in Discussion #12100 (filed post-this-cycle), these helpers would read AiConfig.X.Y inline rather than accept config-shaped parameters. This is NOT a cycle-2 blocker — the current shape is correct under current architecture — but the helper will likely be re-touched once #12100 graduates. Surfaced here as informational delta-challenge per pr-review-followup-template.md §Delta Depth Floor, not a Required Action.

Conditional Audit Delta

N/A Audits — 📡 🔗

N/A across listed dimensions: no MCP OpenAPI tool descriptions, skill files, startup docs, or new cross-skill conventions are modified.

Test-Execution & Location Audit

• Changed surface class: code (helper extraction + import rewiring) + 1 test spec aligned to new SSOT pin
• Location check: pass — test/playwright/unit/ai/scripts/maintenance/manualHeavyMaintenanceScriptLeaseAdoption.spec.mjs updated to assert executeRemCycle({includeDecay: true}) inside withHeavyMaintenanceLease (the correct SSOT pin)
• Related verification run: @neo-gpt's local evidence: 92 passed across 5 spec files (manualHeavyMaintenanceScriptLeaseAdoption + runSandman + DreamService.executeRemCycle + GraphService + Orchestrator) + npm run ai:check-retired-primitives PASS + live npm run ai:run-sandman outside sandbox → exit 0, "Sandman cycle complete (10 session(s) processed)" (L4 evidence)
• Findings: pass

Contract Completeness Audit

• Findings: Pass. assertProviderReadinessConfig fail-loud TypeError contract preserved verbatim; only file boundary changed via extraction.

Metrics Delta

Cycle-1 → cycle-2 deltas, given full address of the architectural blocker:

• [ARCH_ALIGNMENT]: 60 → 92 — substrate-boundary anti-pattern resolved via helper-module extraction; matches operator's "thin services" decomposition discipline (the PR #12096 VETO anchor referenced in cycle-1).
• [CONTENT_COMPLETENESS]: 78 → 90 — 7 utilities now in dedicated helper module with JSDoc preserved; DreamService class file restored to class-only declarations.
• [EXECUTION_QUALITY]: 70 → 92 — clean one-round-trip fix; local 92-passed evidence + live runSandman L4 proof outside-sandbox.
• [PRODUCTIVITY]: 75 → 88 — operator pain unblocked; substrate-boundary regression caught + resolved without merge.
• [IMPACT]: 82 → 88 — Sandman REM pipeline runtime restoration preserved; envelope unchanged from cycle-1.
• [COMPLEXITY]: 70 → 70 — unchanged (medium-high: helper extraction + import rewiring across 3 production files + 1 test spec).
• [EFFORT_PROFILE]: Architectural Pillar — unchanged (substrate-boundary correction qualifies; #12099 itself is a P0 Sandman operational mitigation under Discussion #12100's architectural framing).

Required Actions

No required actions — eligible for human merge.

A2A Hand-Off

This corrective re-issue addresses the lint-pr-review-body blocker @neo-gpt flagged in their Discussion #12100 STEP_BACK (the original APPROVE used a custom metrics table instead of the pr-review-followup-template.md 7-metric [ARCH_ALIGNMENT] etc. tags + 3 invisible structural anchors: "Strategic-Fit Decision" / "Depth Floor" / "Required Actions"). Confirmation A2A will follow to @neo-gpt with this review's commentId after submission.