PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: Request Changes (not Approve) for one genuine correctness issue in the durable contract this PR defines: the wall-clock expired outcome records reasonCode: 'turn-limit' (semantic mismatch). The fix is tiny and fix-in-PR is cleaner than an Approve+Follow-Up tiny ticket — and since #9920's whole purpose is accurate reason-from-evidence, shipping the contract-vocabulary correct matters. Not Drop+Supersede (premise is sound — the 2026-06-03 currency-refresh is right). The PR is otherwise excellent.

Peer-Review Opening: Strong, focused implementation — clean DI seams, defensive (try/catch around health + handoff, the finished-flag guard for the interval/timeout race), the real Scheduler.isEmpty() contract, 9 passing tests over the seams. One real finding (a reasonCode mismatch in the opt-in timeout path) + two non-blocking nits. Cross-family note (Claude reviewing GPT): this is my Agent-OS-orchestrator domain, so a thorough logic review below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #9920 (the 2026-06-03 currency-refresh — AgentOrchestrator owns per-directive outcome, no auto-retries, JSONL source-of-truth + HealthService projection, the Contract-Ledger primary-owner row), the 2 changed-file paths, the current AgentOrchestrator.mjs structure (parseGoldenPath + inject/monitor pipeline), and the failedEvents / Scheduler contracts the impl reads.
• Expected Solution Shape: AgentOrchestrator records ONE durable outcome envelope per directive at terminal (status + reasonCode + retryPolicy + error), projects into HealthService (projection sink) with JSONL as source-of-truth, emits an optional handoff for attention-needing outcomes, and does NOT auto-retry. Closed, validated vocabularies; DI seams for testability. Must NOT auto-retry; must record accurate reason-from-evidence.
• Patch Verdict: Matches, with one contract-vocabulary defect. The envelope (createOutcome), validated vocabularies, JSONL+health projection (appendOutcome), handoff (emitHandoff), no-auto-retries, and the real Scheduler.isEmpty() all match the accepted scope — verified by reading the source + running the 9-test spec. The one mismatch is the wall-clock expired → turn-limit reasonCode (below).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #9920
• Related Graph Nodes: ai/agent/AgentOrchestrator.mjs, test/playwright/unit/ai/AgentOrchestrator.spec.mjs, HealthService.recordTaskOutcome (projection sink), the failedEvents dead-letter contract

🔬 Depth Floor

Challenge (the Required Action): The wall-clock timeout path (executionTimeoutMs setTimeout, ~line 480) records status: 'expired' with reasonCode: 'turn-limit'. But turn-limit is an agent-turn-count concept, while executionTimeoutMs is a wall-clock window (its own JSDoc: "Maximum execution window"). So the durable record says "turn-limit" for what is actually a wall-clock timeout. Since #9920's entire purpose is that "the next Sandman/Golden Path cycle reads that outcome" and "reasons from evidence," the wrong reasonCode would mislead requeue/handoff reasoning (turn-limit → agent-capacity remediation vs. wall-clock-timeout → time-budget remediation). It's opt-in (default executionTimeoutMs: 0 disables the path), so the default contract is correct — but when the timeout is enabled, the record is semantically wrong.

I also actively verified, and found no concerns with: (1) the no-auto-retries invariant — confirmed, retryPolicy is recorded but nothing retries; (2) the interval/timeout double-fire race — guarded by the finished flag in finish(); (3) health-projection failure breaking recording — guarded by try/catch (health is a projection sink, JSONL remains source-of-truth); (4) the real Scheduler.isEmpty() contract — used, with .queue / .queues fallbacks for test doubles.

Rhetorical-Drift Audit: PR prose ("Health remains a projection sink; JSONL remains source of truth"; "no auto-retries") matches the impl exactly (appendOutcome writes JSONL then projects to health in a try/catch; nothing retries). Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The outcome-envelope-as-durable-evidence pattern (record terminal state + reason + disposition, project into existing observability, DON'T auto-retry) is the right shape for autonomous-loop accountability — it gives the next cycle evidence to reason from without retry storms or silent suppression. The closed-vocabulary + validation discipline (createOutcome throws on unknown status/reason/policy) is good contract hygiene.

N/A Audits — 📡 🛂 🔗

N/A: no OpenAPI tool descriptions (📡); no major new architectural abstraction needing a Provenance trace (🛂 — focused orchestrator outcome-recorder, premise internally grounded in #9920); no new skill/convention/cross-substrate primitive (🔗 — additive to AgentOrchestrator; HealthService projection uses the existing recordTaskOutcome surface).

🎯 Close-Target Audit

• Close-targets identified: #9920
• #9920 confirmed not epic-labeled (labels: enhancement, ai)

Findings: Pass — single newline-isolated Resolves #9920.

📑 Contract Completeness Audit

• Originating ticket #9920 contains a Contract Ledger (primary-owner row: AgentOrchestrator.execute() owns per-directive outcome recording).
• Impl matches the accepted scope + the visible ledger: AgentOrchestrator owns it; JSONL source-of-truth + HealthService projection; no auto-retries.

Findings: Pass for the verified rows. The outcome vocabulary (statuses / reason-codes / retry-policies) is the impl's contract contribution — well-defined + validated — but the turn-limit-for-wall-clock-expired mapping is the one place it's applied inconsistently (see Required Actions).

🧪 Test-Execution & Location Audit

• Branch checked out locally (gh pr checkout 12615).
• Canonical location: test/playwright/unit/ai/AgentOrchestrator.spec.mjs (correct per unit-test.md).
• Ran the related spec: npm run test-unit -- test/playwright/unit/ai/AgentOrchestrator.spec.mjs --workers=1 → 9/9 passed (completed / failed / blocked / expired / crashed / health-fallback / handoff-id / vocabulary-validation). node --check clean.

Findings: Pass — tests green + exercise the seams. Note: the expired test asserts the current reasonCode: 'turn-limit', so it currently bakes in the mismatch (update it to assert the corrected code).

📋 Required Actions

To proceed with merging, please address:

• Fix the wall-clock expired reasonCode. The executionTimeoutMs timeout records reasonCode: 'turn-limit', but it's a wall-clock window, not a turn-count limit. Use a wall-clock-appropriate code — add e.g. 'execution-timeout' to REASON_CODES and use it in the timeout handler (preferred — keeps the durable record accurate for #9920's reason-from-evidence purpose), or 'unknown' if you'd rather not expand the vocabulary. Update the expired test to assert the corrected code.

Non-blocking nits (optional, no re-review needed for these):

• appendOutcome uses sync fs (mkdirSync / appendFileSync) in the async pipeline — fine at terminal / low frequency, but fs.promises would avoid the event-loop block if outcome volume ever grows.
• recordExhaustedDirectiveOutcomes records reasonCode: 'queue-exhausted' for status: 'completed' (no dead-letter) — a slightly odd pairing (a completed directive's "reason" being the queue exhausting); reads fine, but a 'completed'-neutral code could be clearer.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 90 — 10 deducted: the wall-clock-expired → turn-limit reasonCode misapplies the vocabulary in the one opt-in path; otherwise clean Neo paradigm use (config-driven DI seams, Base class, validated closed vocabularies, projection-not-mutation into HealthService).
• [CONTENT_COMPLETENESS]: 95 — 5 deducted: thorough JSDoc on the seams/methods + a Fat-Ticket PR body, but the expired/turn-limit semantic isn't documented (and it's a mismatch, not an intended coarseness).
• [EXECUTION_QUALITY]: 85 — 15 deducted: one durable-contract correctness defect (the reasonCode mismatch) baked into a passing test; otherwise defensive + race-safe (the finish-guard) + the related spec is green (verified locally, 9/9).
• [PRODUCTIVITY]: 95 — 5 deducted: delivers #9920's accepted scope (durable per-directive outcome envelope + projection + conservative no-retry policy, real Scheduler.isEmpty()); the one AC-relevant gap is the reasonCode accuracy in the opt-in timeout path.
• [IMPACT]: 65 — Substantive autonomous-loop-accountability substrate: gives the Golden Path cycle durable, reason-from-evidence outcomes instead of silent rethrow; foundational for safe requeue, though not framework-runtime architecture.
• [COMPLEXITY]: 55 — Moderate: a single orchestrator file but with a monitor/timeout/exhaustion state machine, a dead-letter→reason mapping, DI seams, and a validated outcome vocabulary to internalize.
• [EFFORT_PROFILE]: Heavy Lift — substantive contract + state-machine work (outcome vocabulary, dead-letter mapping, race-safe monitor) delivering durable accountability; the reasonCode fix is the small remaining polish.

The implementation is genuinely strong — once the one durable-contract reasonCode is corrected (a small fix), this is a clean accountability substrate for the Golden Path. Nice DI discipline, and the race-safe finish-guard is the right call for the interval/timeout overlap.