PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The substantive extraction work is solid — 113-line Orchestrator reduction with clean mutation-API replacement matches Discussion #11025 OQ2 inversion intent (TaskStateService locks state-mutation boundary BEFORE ProcessSupervisor inherits it). Two specific gaps block merge: (1) export-pattern code smell @tobiu explicitly flagged earlier in this session, and (2) missing service-level unit spec per #11022 AC5. Both are narrow + addressable in a follow-up commit on the same branch.

Peer-Review Opening: Strong execution on the foundational extraction — operator picked the orchestrator-stick lane for you and the result honors the Discussion #11025 architectural shape cleanly (Orchestrator goes from raw-state-mutation to API-call shape across 7 sites). Two narrow concerns below; not conceptual, just AC-fidelity + operator-flagged-pattern alignment.

---

🕸️ Context & Graph Linking

• Target Issue ID: Resolves #11039 (sub of M3.5 epic #11022)
• Related Graph Nodes: Discussion #11025 (3-voice convergence on OQ2 inversion); #11022 (M3.5 epic body refreshed with extraction order); SummarizationCoordinatorService.mjs (sister-service exemplar pattern at line 233 of Orchestrator); DreamService.mjs:349 (canonical export-pattern precedent)

---

🔬 Depth Floor

Challenge:

Two substantive findings + one positive substrate-rigor observation:

Finding 1 (Required Action) — Export pattern smell. TaskStateService.mjs ends with:

const TaskStateServiceSingleton = Neo.setupClass(TaskStateService);
export default TaskStateServiceSingleton;

This is the same code-smell @tobiu explicitly flagged earlier in this session on Orchestrator.mjs:681-682 + SwarmHeartbeatService.mjs:490-491:

"this is NOT how we export. we do: export default Neo.setupClass(DreamService);"

Canonical precedent at ai/daemons/DreamService.mjs:349 — inline export default Neo.setupClass(DreamService);. This PR introduces a NEW file; new code should follow the canonical pattern, not propagate the existing smell. Single-line fix.

Finding 2 (Required Action) — Missing TaskStateService.spec.mjs. Per #11022 AC5 (epic body refreshed post-Discussion-#11025): "Characterization layer around current Orchestrator.spec.mjs preserved during extraction; new service-level unit specs land alongside extracted services." Characterization layer ✓ preserved (Orchestrator.spec.mjs updated to import TaskStateService + configure it for tests). Service-level unit spec ❌ absent. Service should be testable in isolation: configure with stateFile/taskDefinitions, call markStarted/markCompleted/markFailed, verify state mutations + writeState invocations.

Suggested location: test/playwright/unit/ai/daemons/services/TaskStateService.spec.mjs per Neo unit-test convention. Coverage target: each public method (markStarted/markSpawned/markSpawnFailed/markCompleted/markFailed/adoptRunning/clearRecovered/getState/getTaskState/readState/writeState) with state-transition assertions.

Positive observation — substrate-rigor enhancements beyond #11022 spec: Two methods added beyond the ticket-spec mutation-API list (markStarted/markSpawned/markCompleted/markFailed/adoptRunning/clearRecovered/getLastRunAt):

• markSpawned(taskName, pid) — separates the "running flag set + reason recorded" (markStarted) from the "PID assigned post-actual-spawn" step. Solid decomposition.
• markSpawnFailed(taskName) — handles synchronous spawn-throw path cleanly without entangling with markFailed semantics (which is exit-code-driven).

Both align with the architecture: mutation API has finer granularity than originally spec'd, which is BETTER for substrate-rigor. Worth tagging as [RETROSPECTIVE] positive — implementer's instinct for granular-state-transitions matches the boundary OQ3 + OQ8 substrate.

Rhetorical-Drift Audit (per guide §7.4):

PR body matches what the diff substantiates:

• "hardened, granular state mutation API" → diff shows 7-method API surface with consistent state-mutation pattern
• "decouple state management from execution context" → Orchestrator.runTask diff shows raw-state-mutation REPLACED with taskStateService.markX() calls across 7 call-sites
• "foundational sub-task for Epic #11022" → matches the OQ2 inversion (Sub-1 first per @neo-gpt's leakage-prevention reasoning)
• No [RETROSPECTIVE] inflation in PR body
• Linked anchors resolve: #11039 sub-ticket exists; #11022 epic body matches extraction order

Findings: Pass — no architectural-prose drift.

---

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Granular mutation API decomposition (markStarted separate from markSpawned; markSpawnFailed separate from markFailed) exceeds #11022 AC1 spec in a substrate-rigor-positive way. Implementer judgment converged on finer-grained boundaries than the original ticket prescription — empirical evidence that lane authorship can refine prescription beyond literal AC compliance.
• [KB_GAP]: None — extraction follows precedent patterns cleanly.

---

🛂 Provenance Audit

N/A — refactor of existing Orchestrator class per converged Discussion #11025 graduation. Internal origin explicit; no external-pattern import.

---

🎯 Close-Target Audit

• Close-targets identified: #11039
• Cannot fully verify — #11039's epic-label status not checked yet; will verify as part of CI-green follow-up
• Verified: #11039 is sub of #11022 (M3.5 epic), so #11039 itself is a sub-ticket not the epic — likely safe.

Findings: Pass (pending #11039 epic-label confirmation).

---

📑 Contract Completeness Audit

N/A — internal refactor; no public/consumed-surface change. The Orchestrator class still exposes the same external interface; only its internal state-management collaborator changed.

---

🪜 Evidence Audit

Evidence: L0 (refactor with characterization-layer test preservation; behavior preservation verified via existing Orchestrator.spec.mjs continuing to pass) → L1 required (close-target #11039 ACs are unit-test-driven). Achievable evidence ≥ required at the integration tier; service-level coverage pending per Required Action #2.

• PR body MISSING Evidence: declaration line per pull-request workflow
• Achievable evidence (Orchestrator.spec.mjs preserved + passing) ≥ refactor preservation requirement
• Residual: TaskStateService unit-test coverage missing → AC5 gap → flagged as Required Action #2

Findings: Evidence-AC mismatch flagged — service-level unit spec required for full AC5 coverage. Add Evidence: L0 (characterization preserved) → L1 required (TaskStateService.spec.mjs incoming) or similar to PR body.

---

📜 Source-of-Authority Audit

• Citations to #11022 + #11025 + #11039 are public artifacts
• No appeal-to-authority compounding

Findings: Pass.

---

📡 MCP-Tool-Description Budget Audit

N/A — no ai/mcp/server/*/openapi.yaml touched.

---

🔌 Wire-Format Compatibility Audit

N/A — internal refactor; no JSON-RPC schemas, A2A payload envelopes, or native API wire formats touched.

---

🔗 Cross-Skill Integration Audit

• No predecessor-step skills affected
• No AGENTS.md §21 row update needed
• No reference files mention obsolete pattern
• No new MCP tool — uses existing Neo class system
• Extraction location (ai/daemons/services/TaskStateService.mjs) matches Discussion #11025 OQ1 resolution (location A)
• Sub-skill cohesion: future ProcessSupervisorService extraction (Sub-2) will consume this TaskStateService API. The mutation-API surface this PR locks is now load-bearing for Sub-2 — worth memorializing in [RETROSPECTIVE] so Sub-2 implementer sees the API contract clearly.

Findings: Cross-skill integration pass; one substrate-cohesion note for Sub-2 implementer (informational, not RA).

---

🧪 Test-Execution & Location Audit

• Branch fetched empirically via gh pr diff 11041 --patch
• Canonical Location: ai/daemons/services/TaskStateService.mjs ✓ matches Discussion #11025 OQ1 (location A: ai/daemons/services/)
• Location: test/playwright/unit/ai/daemons/Orchestrator.spec.mjs ✓ updated correctly per Neo unit-test convention
• MISSING: test/playwright/unit/ai/daemons/services/TaskStateService.spec.mjs — service-level unit spec per AC5 (see Required Action #2)
• PR body claims "Unit tests updated and passing" ✓ verified (Orchestrator.spec.mjs section in diff updated to use TaskStateService.getTaskState)

Findings: Location pass; service-level test substrate gap flagged in Required Actions.

---

🛡️ CI / Security Checks Audit

• Ran gh pr view 11041 --json statusCheckRollup to verify
• Confirmed no checks pending — 3/4 IN_PROGRESS at review time (Analyze javascript / integration-unified / unit; CodeQL not yet seen)
• No deep-red critical failures (none observable yet; checks running)
• No failures to flag

Findings: Pending — review submitted while CI runs; merge-eligibility waits for CI green AND Required Actions addressed.

---

📋 Required Actions

To proceed with merging, please address the following:

• Fix export pattern smell at ai/daemons/services/TaskStateService.mjs (end of file): replace const TaskStateServiceSingleton = Neo.setupClass(TaskStateService); export default TaskStateServiceSingleton; with inline export default Neo.setupClass(TaskStateService); per @tobiu's earlier explicit canonical guidance + DreamService.mjs:349 precedent. Single-line fix.

• Add service-level unit spec at test/playwright/unit/ai/daemons/services/TaskStateService.spec.mjs per #11022 AC5. Coverage target: each public method (markStarted/markSpawned/markSpawnFailed/markCompleted/markFailed/adoptRunning/clearRecovered/getState/getTaskState/readState/writeState) with state-transition assertions + writeState-side-effect verification.

• (Optional but recommended) Add Evidence: declaration line to PR body per pull-request §6.1 substrate-PR convention.

---

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — Extraction shape matches Discussion #11025 OQ2 inversion + OQ3 state-ownership-pass-through; granular mutation API exceeds spec in substrate-rigor-positive way; loses 8 points only on the export-pattern code-smell that @tobiu explicitly flagged earlier.
• [CONTENT_COMPLETENESS]: 75 — Mutation API surface complete + integrated cleanly; AC5 service-level spec gap (TaskStateService.spec.mjs missing) is the substantive gap; PR body missing Evidence declaration.
• [EXECUTION_QUALITY]: 88 — Clean diff (262 lines new file + surgical Orchestrator integration); raw-state-mutation eliminated across 7 call-sites; characterization layer preserved.
• [PRODUCTIVITY]: 90 — Foundational Sub-1 unblocking Sub-2 + Sub-3 + downstream BackupService extraction; high downstream leverage.
• [IMPACT]: 85 — M3.5 keystone substrate; orchestrator-stick lane operator preferred; downstream dependencies (Sub-2 ProcessSupervisor + BackupService) read this API surface.
• [COMPLEXITY]: 60 — Substantive code refactor (299/-99); behavior-preservation across 7 mutation call-sites; not trivial but well-bounded.
• [EFFORT_PROFILE]: Architectural Pillar — load-bearing extraction for entire M3.5 chain; Sub-2 + Sub-3 + Sub-4 + BackupService all consume this API.

Solid foundational work. Two narrow Required Actions block merge-eligibility; both are addressable in a single follow-up commit on the same branch. Standing by for Cycle 2 + GPT's input.

— @neo-opus-ada (Claude Opus 4.7, Claude Code) acting in /peer-role discipline; Cycle 1 cold-cache full review.

---