Implement workflow orchestration engine (P0 + P1)

[fwdslsh-dev]

Summary

Implements the orchestration plan P0 (Workflow Plan Graph compiler) and P1 (native executor) for engine-driven workflow execution. Workflows can now declare per-step orchestration subsections (### Runner, ### Model, ### Timeout, ### Fan-out, ### Schema, ### Env, ### Depends On) and be executed via akm workflow run with automatic unit dispatch, fan-out concurrency, schema-validated structured output with retry, and per-unit persistence.

Key changes:

Parser & Schema

• Extended workflow Markdown grammar with orchestration subsections (parser-orchestration.ts)
• Added WorkflowStepOrchestration type to schema with runner, model, timeout, fan-out, schema, env, and dependency declarations
• Semantic validation for Depends On references

IR Compilation (P0)

• ir/schema.ts: Backend-agnostic Workflow Plan Graph with IrStepPlan, IrAgentNode, IrMapNode, and IrGateNode
• ir/compile.ts: Deterministic compiler from parsed workflows to plan graphs; linear workflows compile to agent chains (no behavior change), orchestrated steps compile fan-out into map nodes

Native Executor (P1)

• native-executor.ts: Executes one step's IR subgraph with fan-out through the scheduler, schema-validated structured output via runStructured, per-unit persistence, and workflow_unit_* events
• scheduler.ts: Concurrency policy layer with engine-wide limits (16 cores max, lifetime unit cap per run)
• unit-writer.ts: Serialized writer queue for workflow_run_units to prevent SQLite contention
• run-workflow.ts: Engine loop that walks the plan graph and advances steps through completeWorkflowStep gate

Storage & Persistence

• Migration 004: workflow_run_units table for per-unit state (runner, model, input hash, result, tokens, failure reason, timestamps)
• WorkflowRunUnitRow and related types in workflow-runs-repository.ts

Supporting Infrastructure

• json-schema.ts: Bounded JSON Schema subset validator for structured output normalization
• env-binding.ts: Extracted env-ref resolution shared between akm env run and workflow engine
• Unit preamble template with run/step/unit context and ground rules
• Check-in surfacing fixes (C2, M1) for stalled-run detection in plain-text output

Documentation & Tests

• Comprehensive test suites for parser, IR compilation, native executor, and unit persistence
• Updated workflow feature docs with orchestration examples
• Added fable model alias (Mythos-class tier)
• STABILITY.md notes experimental status

The implementation maintains the gated step spine invariant: every step advances strictly through completeWorkflowStep, never by direct row writes. Durable-row resume re-dispatches only steps that never completed, using input hashes to avoid double-issuing side-effecting work.

Test plan

• Added 552 lines of tests in native-executor.test.ts covering fan-out dispatch, item interpolation, evidence collection, schema validation with retry, and reducer logic
• Added 231 lines in parser-orchestration.test.ts for extended grammar parsing
• Added 150 lines in ir-compile.test.ts for plan graph compilation
• Added 182 lines in run-units.test.ts for unit persistence and writer queue
• Added 79 lines in json-schema-subset.test.ts for schema validation
• Added 102 lines in checkin-surfacing.test.ts for check-in rendering fixes
• Updated migration characterization snapshot to include migration 004
• All existing workflow tests pass; no behavior change for linear workflows

Checklist

• Tests pass (bun test)
• Lint passes (bun run lint)
• Docs updated (workflows.md, CHANGELOG.md, STABILITY.md)

https://claude.ai/code/session_018DptAcDcuW2TF2RfM3a4ok

[Copilot]

Pull request overview

Adds an engine-driven workflow orchestration layer to AKM: workflows can now declare per-step orchestration metadata (runner/model/timeout/fan-out/schema/env/depends-on), compile to a deterministic IR plan graph, and execute via a new akm workflow run path with per-unit persistence, concurrency controls, and structured-output validation.

Changes:

• Extend workflow Markdown parsing + schema to support orchestration subsections and validate Depends On references.
• Introduce a Workflow Plan Graph IR compiler (P0) and a native executor + scheduler + unit journal/writer queue (P1), plus a new CLI subcommand workflow run.
• Add migration 004 and repository support for workflow_run_units, plus substantial tests and docs for the new engine and related fixes (check-in surfacing, model alias).

Reviewed changes

Copilot reviewed 39 out of 39 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/workflows/run-units.test.ts	New tests for workflow_run_units persistence + serialized writer queue behavior.
tests/workflows/parser-orchestration.test.ts	New tests for orchestration subsection parsing and error cases.
tests/workflows/native-executor.test.ts	New tests for fan-out dispatch, schema validation/retry, durable-row reuse, and engine loop behavior.
tests/workflows/migrations.test.ts	Updates migration ledger expectations to include migration 004.
tests/workflows/ir-compile.test.ts	New tests for deterministic IR compilation and fan-out → map node lowering.
tests/workflows/checkin-surfacing.test.ts	New tests ensuring stalled-run check-in directives surface in status and plain text.
tests/storage/sqlite-migrations.characterization.test.ts	Updates workflow DB schema characterization to include workflow_run_units.
tests/storage/snapshots/sqlite-migrations.characterization.test.ts.snap	Snapshot update reflecting migration 004 schema additions.
tests/json-schema-subset.test.ts	New tests for the bounded JSON-Schema-subset validator.
tests/agent/agent-builders.test.ts	Adds tests for the new built-in fable model alias resolution.
STABILITY.md	Marks akm workflow run + orchestrated steps as experimental/stability caveat.
src/workflows/validator.ts	Adds semantic validation for ### Depends On references; fixes frontmatter allowed-keys messaging.
src/workflows/schema.ts	Adds orchestration types (runner, model, timeout, fanOut, schema, env, dependsOn).
src/workflows/runtime/workflow-asset-loader.ts	Retains full parsed workflow document for IR compilation while keeping the existing step projection.
src/workflows/runtime/runs.ts	Includes check-in evaluation in run-detail shaping (checkin surfaced when stalled).
src/workflows/parser.ts	Integrates orchestration parsing and improves unknown-subsection error messaging.
src/workflows/parser-orchestration.ts	New orchestration subsection parser (Runner/Model/Timeout/Fan-out/Schema/Env/Depends On).
src/workflows/ir/schema.ts	New backend-agnostic plan-graph IR schema for workflow execution nodes + gated step spine.
src/workflows/ir/compile.ts	New deterministic compiler from parsed workflows → plan graph IR (linear and fan-out).
src/workflows/exec/unit-writer.ts	New serialized write queue to avoid concurrent SQLite writer contention.
src/workflows/exec/scheduler.ts	New scheduler policy layer (global concurrency cap + lifetime unit cap + abort support).
src/workflows/exec/run-workflow.ts	New engine loop implementing akm workflow run, advancing steps strictly via completeWorkflowStep.
src/workflows/exec/native-executor.ts	New native executor for one step plan: dispatch fan-out units, validate structured output, persist unit rows.
src/workflows/db.ts	Adds migration 004 creating workflow_run_units table + index.
src/workflows/cli.ts	Registers run as a workflow subcommand.
src/storage/repositories/workflow-runs-repository.ts	Adds workflow_run_units row types and repo methods to insert/finish/query units.
src/output/text/workflow.ts	Registers a plain-text formatter for workflow-run output.
src/output/text/helpers.ts	Adds workflow run plain formatter; restores check-in directive in plain status output.
src/output/shapes/passthrough.ts	Adds workflow-run to passthrough output command list.
src/integrations/agent/model-aliases.ts	Adds built-in fable alias mapping for supported platforms.
src/core/json-schema.ts	New bounded JSON-Schema-subset validator used for structured output verification.
src/core/events.ts	Adds workflow_unit_started / workflow_unit_finished event types.
src/commands/workflow-cli.ts	Adds akm workflow run JSON command wiring and flag parsing (--params, --max-steps).
src/commands/env/env-cli.ts	Refactors env injection path to reuse extracted resolveEnvBinding.
src/commands/env/env-binding.ts	New shared env-ref resolution + safety enforcement used by both env run and workflow units.
src/assets/prompts/workflow-unit-preamble.md	New standardized per-unit preamble template injected into dispatched prompts.
docs/features/workflows.md	Documents orchestrated steps and the new akm workflow run behavior and semantics.
docs/features/agent-integration.md	Documents fable as a built-in model alias.
CHANGELOG.md	Adds release notes for orchestration engine, check-in surfacing fixes, and fable alias.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[itlackey]

I would not merge this PR as-is. It is large — 107 files, 15,948 additions, 268 deletions — and the riskiest changes are in execution, persistence, replay, and validation paths.

I did a static review from the GitHub PR/source. I did not execute the test suite locally.

Blocking / high-risk correctness issues

1. Run lease is explicitly not enforced, so two workflow run processes can double-dispatch work

The PR adds lease columns, but both docs and migration comments say enforcement is deferred. The runtime still loads the next step and frozen plan, then dispatches without acquiring any cross-process lease.

The in-process unit writer queue does not solve this. It serializes writes only inside one Bun process; it does not prevent a second CLI process from dispatching the same unit.

Impact: duplicated agent/LLM spend, duplicated side effects, corrupt run evidence, nondeterministic branch execution.

Fix: implement a DB-backed lease before executing any step. Use an atomic UPDATE ... WHERE engine_lease_until IS NULL OR engine_lease_until < now pattern, store holder id, refresh while running, and release on completion/failure.

---

2. Durable-row replay hash is incomplete and can reuse stale work under changed execution conditions

The input hash only includes prompt, runner, model, and schema. It excludes resolved profile, resolved runner kind, resolved model after alias lookup, environment bindings, timeout, isolation, config defaults, tool policy, and harness adapter behavior.

Worse, environment bindings are resolved before checking completed unit rows, so a fully reusable completed step can fail just because a now-unused env asset was deleted or changed.

The default dispatcher also resolves live config at execution time, not from a frozen execution profile.

Impact: the engine can silently skip work that should be re-run, or fail to replay completed work because live config/env changed.

Fix: either freeze the resolved execution context into the plan/run, or include every execution-affecting input in the unit hash. Move env resolution into the dispatch path after durable-row reuse, or persist env binding identity/version in the hash.

---

3. Completion gates judge a synthetic summary, not the real artifact/output

The native executor returns summaries like “Executed N units…” rather than a task-specific summary of the actual work. That synthetic string is then passed into completeWorkflowStep as the summary for completion criteria validation.

The docs admit artifact-judging gates are not implemented yet.

Impact: gates can falsely reject good work or falsely pass bad work. on_error: continue makes this worse because the gate receives a generic count of failed units rather than structured evidence.

Fix: do not market this as gated artifact execution yet. Either pass actual artifact/evidence into the judge, or disable criteria validation for native engine execution until R2 artifact gates exist.

---

4. Scheduler swallows thrown exceptions and converts them into generic “aborted” unit failures

concurrentMap catches every thrown worker error and leaves the slot undefined. The native executor then maps that to an "aborted" failure with a generic message.

Under on_error: continue, those swallowed infrastructure errors can still allow the step to pass.

Impact: DB write failures, programming errors, and unexpected dispatcher bugs can be hidden as ordinary unit failures.

Fix: distinguish expected unit failures from infrastructure exceptions. concurrentMap should return { ok:false, error } or rethrow fatal errors, not erase them.

---

5. Empty-string text outputs are persisted as NULL

resultJson uses outcome.text ? JSON.stringify(outcome.text) : null, so a successful text output of "" becomes NULL. On replay, NULL becomes undefined, and the successful output is lost.

Impact: downstream expressions can see null/missing output even though the original unit succeeded with an empty string.

Fix: use outcome.text !== undefined instead of truthiness.

---

6. Parameter JSON schemas are documented but not enforced

Docs say params are JSON-Schema declarations. But the compiler only keeps parameter names in the frozen plan, dropping the schemas.

startWorkflowRun accepts and stores raw params without validating them against the declared schemas.

Impact: invalid params fail later as expression/runtime errors, not as authoring/start validation errors. The declared schema is mostly documentation.

Fix: freeze param schemas into the plan and validate --params at start/auto-start.

---

7. Docs claim lint catches unknown params and bad output paths, but compiler does not

The docs say akm workflow validate checks “unknown step, unknown param, bad path” at lint time.

The compiler checks whether a referenced step exists and comes earlier, but it does not validate the output path against the producer’s declared output schema. It explicitly does not check param presence.

Impact: users will trust validation results that do not actually prove what the docs say they prove.

Fix: either implement schema-aware path validation and declared-param checks, or weaken the docs.

---

8. Step id grammar is internally inconsistent

The program schema allows step ids beginning with digits and containing dots.

The expression parser’s requires a letter/underscore first and allows letters, digits, underscores, and hyphens — but not dots.

So a step id like 1.review can be valid as a step id but impossible to reference as ${{ steps.1.review.output... }}.

Impact: valid workflow programs can contain step ids that cannot be addressed by the expression language.

Fix: make step ids expression-addressable, or tighten the step id pattern to the expression identifier grammar.

---

9. JSON Schema subset validator has object/property correctness bugs

For required fields, the validator uses key in record, so inherited prototype properties can satisfy required. It also uses key in record for properties.

additionalProperties: false is enforced only when properties exists. A schema like { type: "object", additionalProperties: false } should only allow {}, but this implementation will allow arbitrary properties.

Impact: structured output validation can pass objects that violate the declared schema.

Fix: use Object.hasOwn(record, key) and enforce additionalProperties: false even when properties is absent.

---

10. Retry taxonomy does not include several failure reasons the executor actually emits

The allowed retry.on vocabulary is the PROGRAM_RETRY_REASONS set.

But the executor can emit reasons outside that set, including validation_error, expression_error, unit_cap_exceeded, env_unsupported, and dispatch_error.

Impact: workflow authors cannot write retry policies for actual engine failures. Some retryable conditions become unretryable by schema.

Fix: define one canonical workflow failure taxonomy and make parser, executor, docs, and tests all use it.

---

11. CommonJS require is used inside ESM output paths

The package is "type": "module" and TypeScript emits module: "ESNext".

Yet runtime code uses bare require(...) in native-executor.ts and runs.ts.

The repo has Node compatibility scripts, so this is not just a Bun-only concern.

Impact: Node ESM execution can throw ReferenceError: require is not defined on these paths.

Fix: use dynamic await import(...), or explicitly create require via createRequire(import.meta.url) in every ESM module that needs it.

---

12. Completed route replay trusts journaled selected without validating it is a legal target

seedJournaledRouteDecisions reads evidence.route.selected and applies it directly.

journaledRouteSelection only checks that it is a non-empty string.

applyRouteDecision then adds that arbitrary selected step to routeSelected and marks all declared targets as unselected.

Impact: corrupted/manual/tampered evidence can cause all real branches to be skipped.

Fix: validate journaled selected against Object.values(route.when) plus defaultStepId; fail loudly if it is not a legal branch target.

---

13. A completed run still loads/verifies the plan before returning no-op state

runWorkflowSteps calls getNextWorkflowStep, then seeds units and loads the frozen plan before the main loop. That happens even when next.done is already true.

loadFrozenPlan can throw for corrupt plan JSON/hash or fall back to loading the live asset for legacy runs.

Impact: akm workflow run can fail because a completed run’s plan is corrupt or legacy asset is missing, even though no execution is needed.

Fix: return final state before loading the plan when next.done is true.

---

14. Unit row replacement can hide repeated dispatch attempts and weaken the lifetime cap

The repository uses INSERT OR REPLACE for unit starts.

The lifetime cap is seeded from the number of unit rows.

Impact: repeated crash/retry of the same base unit id can replace the row instead of increasing the durable dispatch count. That makes the “lifetime” cap less durable than advertised.

Fix: append attempts instead of replacing, or keep a separate monotonic dispatch-attempt counter.

---

15. finishUnit does not verify that it updated a row

finishUnit runs an UPDATE ... WHERE run_id = ? AND unit_id = ? but ignores the affected row count.

Impact: if the insert failed, row was deleted, or unit id mismatched, the executor can proceed as though persistence succeeded.

Fix: check changes; throw a fatal persistence error if no row was updated.

---

Major design / architecture issues

16. The native engine is built around current live config, not a fully frozen run contract

The plan is frozen, but execution substrate is not. Runner resolution, default profile, default LLM, model aliases, and harness resolution are read from live config at execution time.

Impact: the same frozen plan can execute differently across invocations if config changes.

Fix: freeze resolved execution settings or record config fingerprints into the run/unit hash.

---

17. Summary gates fail open when no LLM judge is configured

completeWorkflowStep builds a judge only when a default LLM exists; otherwise judge is null and validation effectively does not block completion.

Impact: “completion gates” are optional in a way that may surprise users, especially because the native engine claims to advance through gates.

Fix: make fail-open explicit in docs/CLI output, or provide a config option to fail closed for workflows that require gates.

---

18. Structured output claims “JSON Schema” but implements a permissive subset

The parser only verifies that output is an object.

The validator ignores $ref, combinators, pattern, format, and every unknown keyword.

Impact: users can write schemas that look strict but are silently weak.

Fix: either use a real JSON Schema validator or clearly name this as akmSchemaSubset and reject unsupported keywords instead of ignoring them.

---

19. The workflow format exposes declarations the engine does not enforce

Docs list typed step-output validation, artifact gates, gate.max_loops, run leases, budget ceilings, watch, and worktree isolation as not enforced.

The compiler still carries some of those fields into the IR.

Impact: users can author workflows that look safer/more controlled than they are.

Fix: hide or reject not-yet-enforced fields unless an explicit experimental flag is enabled.

---

20. workflow_step_completed event is emitted for skipped/failed steps and leaks raw notes

completeWorkflowStep always appends workflow_step_completed, regardless of whether the status is completed, failed, blocked, or skipped. It also includes notes in metadata.

The engine uses completeWorkflowStep for skipped route targets and failed execution.

This conflicts with the nearby event hygiene rule that raw workflow-authored content should stay out of events.

Impact: consumers of events can misinterpret failed/skipped steps as completed, and raw notes may become prompt-injection surface.

Fix: emit status-specific events or include status in metadata; remove raw notes from event metadata.

---

21. Route failure evidence drops the actual route failure detail

On route evaluation failure, the code writes notes with the route error, but persists only result.evidence, not an evidence field describing the route failure.

Impact: status/evidence consumers do not get structured route failure diagnostics.

Fix: persist evidence.routeError or equivalent.

---

22. Active-run creation guard is not atomic across processes

startWorkflowRun checks for an existing active run and then inserts a new run in the same callback, but there is no unique partial index or lock shown here.

Impact: two processes can race and both create active runs for the same workflow/scope.

Fix: enforce this at the database level, not only in application code.

---

23. workflow validate mishandles origin-qualified workflow refs

Runtime code supports origin-qualified refs by parsing origin//workflow:name.

But resolveWorkflowFilePath only treats values starting with workflow: as refs; anything else is returned as a filesystem path.

Impact: akm workflow validate some-origin//workflow:foo will be treated as a path, while other workflow commands can treat it as a ref.

Fix: parse refs with parseAssetRef instead of startsWith("workflow:").

---

24. YAML workflow authoring is only partially integrated into workflow create

workflow template --yaml can print a YAML program.

But createWorkflowAsset always parses imported content as markdown and writes through the markdown workflow creation path.

Impact: users can validate/run YAML workflows, but the create command does not provide a clean YAML creation path.

Fix: add akm workflow create --yaml or a separate workflow program create.

---

25. Timeout docs and parser disagree

Docs say timeout values are strings: "ms" | "s" | "m" | "none".

Parser also accepts bare numbers as milliseconds.

Impact: docs and schema expectations drift; tests may miss numeric timeout behavior.

Fix: either document bare numeric milliseconds or reject them.

---

26. The parser’s expression checker is fragile and silently disables itself

loadExpressionChecker uses createRequire(import.meta.url) with "./expressions" and returns null on any failure.

It also catches parser/checker exceptions and returns null, meaning malformed expressions can pass this parser pass if the checker import/API fails.

Impact: duplicated parser/compiler validation gives false confidence and hides broken imports.

Fix: directly import the expression parser. The “parallel task may not exist” TODO should be removed now that the file exists.

---

27. Environment merging silently overwrites duplicate keys

resolveEnvBindings merges each env ref with Object.assign. Later refs silently overwrite earlier keys.

Impact: a workflow author may think multiple env assets are additive when they are order-dependent.

Fix: detect duplicate keys and fail unless an explicit override policy is declared.

---

Documentation / release-quality issues

28. Changelog contradicts itself about markdown orchestration

The “Added” section says workflow orchestration is declared with markdown subsections like ### Runner, ### Fan-out, ### Schema, etc.

The “Changed” section then says that same markdown grammar was removed before release and replaced with YAML programs.

Impact: release notes are confusing and imply a surface that no longer exists.

Fix: remove the obsolete markdown-orchestration “Added” language and describe only the final YAML surface.

---

29. Codex adapter comments are stale

The Codex builder says it is “NOT registered anywhere yet.”

But the Codex harness imports the builder and exposes it on the registered harness descriptor.

The result extractor has the same stale comment.

Fix: remove stale integration-task comments before merge.

---

30. Built-in model alias docs look brittle

The code hardcodes fable to claude-fable-5 / opencode/claude-fable-5.

Docs present fable as a built-in tier and recommend it for deep work.

Impact: hardcoded provider model ids age badly. If these ids are not valid in every supported environment, default workflows fail.

Fix: keep semantic aliases in docs/config examples, but avoid hardcoding speculative or provider-specific defaults unless covered by integration tests or official compatibility checks.

---

31. There is a stray duplicate block comment

runs.ts has a duplicate /** before buildDefaultSummaryJudge.

Fix: remove the extra opener.

---

32. Compiler comment says retry is future work even though retry is implemented

compileProgramUnit says retry dispatch is “engine-rework scope,” but the executor implements bounded retry and tests it.

Fix: update the comment; stale architecture comments are dangerous in this PR because the surface is already complex.

---

Missing tests I would require before merge

The added tests cover a lot of happy-path and peer-review scenarios, but the native executor tests intentionally use injected fake dispatchers and avoid real agent binaries/LLMs.

I would add tests for:

1. Two-process run lease race: launch two workflow run invocations against the same active run and prove only one dispatches.

2. Replay hash correctness: change env refs, profile, model alias config, default agent, and timeout between invocations; prove reuse does or does not happen intentionally.

3. Env-after-reuse: complete all units, delete the env asset, rerun/resume, and prove completed units can still replay.

4. Empty string output persistence: successful unit returns ""; verify row stores JSON string and downstream ${{ steps.x.output }} resolves to "".

5. Parameter schema validation: invalid --params fails at start/auto-start, not during execution.

6. Bad path lint: ${{ steps.discover.output.missing.deep }} should fail validation when producer output schema makes that impossible.

7. Step id grammar: ids with dots/digit starts should either fail parsing or be referenceable.

8. JSON schema edge cases: required: ["toString"], additionalProperties: false with no properties, inherited/prototype keys, unsupported keywords.

9. Retry taxonomy closure: every executor-emitted failureReason is either accepted by retry.on or explicitly documented as non-retryable.

10. Origin-qualified validate: akm workflow validate origin//workflow:name.

11. Node ESM execution: exercise workflow run, default LLM gate, and default unit dispatcher under Node compatibility mode.

12. Tampered route evidence: completed route with evidence.route.selected = "not-a-target" must fail loudly.

13. Event semantics: failed/skipped steps must not emit misleading workflow_step_completed without status.

14. No-op completed run: completed run with corrupt/missing legacy plan should return done status without trying to execute/load.

Merge recommendation

Block this PR until at least these are fixed: run lease enforcement, replay hash completeness, param/schema validation alignment, expression id grammar mismatch, JSON Schema validator bugs, ESM require usage, and misleading gate semantics. The rest can be follow-up only if the feature remains clearly experimental and the docs stop overstating what is enforced.

[Copilot]

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

[itlackey]

@codex do an exhaustive code review

[chatgpt-codex-connector]

Codex Review: Something went wrong. Try again later by commenting “@codex review”.

Unknown error

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[itlackey]

@codex do an exhaustive code review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e78cb16797

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Keep run leases alive during long unit dispatches

With the lease expiring after 90s and only being renewed between steps, any unit that runs longer than the TTL (the default unit timeout is 10 minutes) leaves the run claimable while the first engine is still dispatching. A second akm workflow run after the expiry can acquire the lease and dispatch the same pending/running units, and insertUnit uses the same unit ids, so the two invocations can clobber each other's journal rows and double-run side-effecting work. The lease needs a heartbeat during long step execution or a TTL that cannot expire while dispatch is in flight.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Reject YAML suffixes when creating markdown workflows

Because explicit workflow extensions now win here, akm workflow create foo.yaml routes the create path to workflows/foo.yaml, but createWorkflowAsset still writes and validates the markdown template with parseWorkflow. The resulting asset is immediately unusable: show/start/validate choose the YAML program parser by the .yaml extension and reject the markdown body. Either the create path should reject .yaml/.yml names or write/validate a YAML program for those suffixes.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Align step IDs with the expression grammar

This pattern accepts step ids that start with digits or contain dots, but step-output references are parsed with readIdent, which requires a letter/underscore first character and stops before dots. In workflows that later need to consume such a step's output, ids like 1build or build.js are valid in the parser/schema but cannot be addressed as ${{ steps.<id>.output }}, so map inputs, routes, and later unit templates become impossible to write without renaming the step.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Preserve empty-string unit outputs in the journal

Successful text units that legitimately return an empty string are journaled as NULL here because the check uses truthiness instead of outcome.text !== undefined. On a later resume, reuseCompletedUnit rehydrates that completed row with no text or result, so the promoted step artifact changes from "" on the live run to null on replay, which can break downstream ${{ steps.*.output }} consumers and schema gates.

Useful? React with 👍 / 👎.

[fwdslsh-dev]

The live/replay divergence you identified was real and is fixed, but in the opposite direction (37e504c): an empty successful free-text output now normalizes to absent on every surface — live dispatch, journal rehydration on resume, and the R3 workflow report ingest all produce result_json = NULL and the same promoted artifact. Preserving "" was the other consistent option, but "no output" is the more honest reading of an empty result, and it keeps the failed-branch semantics (which already mapped ""→NULL) uniform. The semantics are documented in the executor module doc, and the EMPTY_OUTPUT driver-parity golden plus a chaos test pin all three surfaces to the identical unit graph. Schema-bearing units are unaffected: an empty output there fails schema validation rather than passing a null through.

---

Generated by Claude Code

[fwdslsh-dev]

Codex round-3 findings and the remaining plan items are all landed (head 1eb9a23, CI green).

Round-3 fixes — all 10 findings verified true: engine resume now seeds gate-loop state from the journal via the same shared helpers brief/report use (crash-after-rejection resumes at loop 2 with stored feedback, pinned by a parity golden); report --rerun carries prior attempts' tokens forward so max_tokens holds run-lifetime; a reported failure under on_error: fail finalizes the step immediately (retry-eligible failures excepted); report/brief rehydrate ~r retry rows so a successful engine retry survives a driver handoff; a required gate whose judge throws now blocks instead of failing open; status --units surfaces stale claims; undeclared-param references warn at validate (see the inline thread for why warning rather than rejection); create rejects cross-extension canonical-name collisions; and report --settle (emitted by brief, guarded by lease/CAS/--expect-step) makes route-first workflows drivable externally.

One disposition note on the Claude structured-output finding (review body, no inline thread): fixed in substance — the builder now emits --output-format json plus the shared schema directive for schema-bearing units, with a new result extractor unwrapping the {"type":"result",...} envelope — but the registry claim was corrected from native-schema to native-json: the headless claude -p CLI has no output-schema flag; the JSON envelope is its documented structured path, and the engine's schema validation still runs on top.

Plan completion: the addendum's last outstanding promises are in — workflow.maxConcurrency config knob (CPU-derived default min(16, max(1, cores−2)), explicit values clamped to [1, 64]; drivers unaffected) and the non-fatal validator warnings channel (untyped step artifacts + undeclared param references, surfaced in validate JSON/text and as stderr warnings at start). A ~45-promise audit of the plan and feature docs against the code found everything else holding; the plan doc, workflows.md, STABILITY.md, and the CHANGELOG were reconciled to describe the shipped reality. One non-workflow item recorded for owner triage rather than changed: STABILITY.md's "11 supported asset types" list predates env/secret/session/fact (code has 14) and still names deprecated vault — classifying those is an owner call.

---

Generated by Claude Code

[itlackey]

Manual validation pass on the new workflow feature. I reviewed the docs, workflow-related stash memories/context, the new CLI/exec/runtime code paths, and then ran break-oriented CLI scenarios in an isolated sandbox.

Sandbox/context used:

• branch: claude/workflow-orchestration-improvements-k39vjo
• sandbox root: /tmp/opencode/workflow-pr714-manual
• docs reviewed: docs/features/workflows.md, docs/technical/akm-workflows-orchestration-plan.md
• targeted tests re-run:
  • bun test tests/commands/workflow-driver-cli.test.ts
  • bun test tests/integration/worktree-isolation.test.ts

Findings

1. workflow list --active includes blocked runs, despite the flag/docs saying “active only”.

• src/commands/workflow-cli.ts:218-223 passes activeOnly, but src/storage/repositories/workflow-runs-repository.ts:257-259 filters with status IN ('active', 'blocked').
• Manual repro: after forcing a required gate to block, bun src/cli.ts workflow list --active returned the blocked run.
• Impact: scripts using --active can treat blocked runs as still-executable work.

2. The external-driver claim flow appears to misrender claimed units in workflow brief.

• In my run, workflow report ... --status running succeeded and created a live claim; finishing without --session-id was correctly refused, proving the claim was enforced.
• But the next workflow brief still showed the same unit as action: "pending" with a normal completion command, instead of claimed.
• Intended behavior seems to be in src/workflows/exec/brief.ts:560-561, with unit/journal association built in src/workflows/exec/brief.ts:410-459.
• Impact: two external drivers can be misled into thinking work is free when it is actually claimed.

3. Resuming a required-gate-blocked run leaves brief in a weak state for external drivers.

• After workflow resume, the run became active, but workflow brief showed the only unit as action: "done" with no report command, while the step was still current and the message still said “Execute them, then report each result.”
• Relevant behavior:
  • src/workflows/exec/brief.ts:334-345
  • src/workflows/exec/brief.ts:598-603
  • src/workflows/exec/brief.ts:611-626
• Re-reporting the completed unit idempotently re-blocked the gate, so there is a recovery path, but brief does not make it obvious.
• Impact: external-driver recovery after gate blocks is awkward and easy to strand.

4. Real workflow run + isolation: worktree completed functionally, but the CLI invocation did not exit cleanly in this environment.

• I ran a live agent-backed workflow in a sandbox git repo.
• The run reported completed in about 7 seconds, preserved a dirty isolation worktree correctly, and left the main repo clean.
• But the shell call stayed alive until the 10-minute tool timeout.
• I did not root-cause this to one file yet, but it points at process-lifecycle cleanup around live agent dispatch.
• Impact: high for real usage if reproducible outside this environment, because a successful workflow can still hang the caller.

Validated

1. External-driver happy path worked end to end.

• I drove basic-check through brief/report across fan-out review, aggregate, route, and final ship.
• Routing selected ship from aggregate.output.verdict = "pass" and advanced automatically.

2. Required-gate blocking worked.

• A workflow with gate.required: true and no LLM judge blocked exactly as expected on the report surface.

3. Budget enforcement worked.

• A max_tokens: 100 workflow failed hard when cumulative reported tokens hit 110, even with on_error: continue.

4. Lease protection worked.

• I simulated a live engine lease in workflow.db; both workflow complete and workflow report refused with clear holder/expiry messaging.

5. Worktree isolation worked functionally.

• The real agent run created worktree-sentinel.txt in the retained isolation worktree under /tmp/akm-worktrees/.../mutate-solo.
• The main repo under /tmp/opencode/workflow-pr714-manual/project stayed clean.

6. Watch output looked good.

• workflow watch emitted the expected workflow_* NDJSON backlog and terminal envelope.

Open questions

1. Is --active intended to include blocked, or is the current implementation drifting from the CLI/docs contract?
2. For external-driver claims, should brief ever show a claimed unit as pending, or is that definitely a bug?
3. After resume on a gate-blocked run, what is the intended external-driver UX when all units are already done but the gate still needs reevaluation?
4. Have others seen workflow run hang after success with real agent dispatch, or should I treat this as a likely branch regression?

If useful, I can break these into follow-up issues with minimal repro commands per item.

[fwdslsh-dev]

Thanks for the break-oriented pass — all four findings reproduced, root-caused, and fixed (head 16562fb). Answers to your open questions:

1. Is --active intended to include blocked? No — that was drift. listRuns was unioning ('active','blocked'); it now filters exactly status='active', and blocked runs stay visible (with their status) in the plain workflow list. The union semantics you might worry about elsewhere were already a separate repository method (findActiveOrBlockedRunForScope, used by the show scope guard), and the start guard was already active-only — both are now pinned by tests so neither can silently drift again.

2. Should brief ever show a claimed unit as pending? Definitely a bug, though a subtler one than it looked: the action mapping itself was rendering claimed correctly in our repro — what made claimed work look free was that the emitted report command omitted --session-id, so any driver copy-pasting it would attempt (and be refused) completion, and nothing signaled the claim in the command itself. The command for a live-claimed unit now carries the claim holder; stale (expired claim) keeps the plain reclaimable form. If you can still reproduce action: "pending" on a live claim after this, that's a repro I'd want — our CLI-level test (start → report --status running → brief) shows claimed with claimedBy/claimExpiresAt.

3. Intended UX after resume on a gate-blocked run? Your instinct was right that the state was underserved. "All units terminal but the step still needs finalization" is now first-class: brief detects it, drops the misleading "Execute them" message, explains what will happen (a required gate with no judge re-blocks; a passing judge completes), and emits exactly one actionable command — workflow report <run> --settle, which now finalizes a fully-terminal step through the same shared completion path under the finalize CAS. Settle still refuses steps with genuinely pending/in-flight/retry-eligible work.

4. The post-success hang — real, root-caused, fixed. The opencode SDK runner caches opencode serve child processes in a per-env registry for reuse, and its only teardown was process.once('exit') — but the live children themselves kept the event loop open, so on a successful run the exit event could never fire: a deadlock until your tool timeout. The engine now drains dispatch resources (closing cached SDK servers) in its run finally on every exit path, with the exit hook kept as backstop. Regression-tested end-to-end through the real dispatch path with a fake server factory (a real opencode serve child isn't spawnable in our sandbox — flagging that honestly; your environment is the true test). Fixing the disposal also surfaced and fixed a latent module-init cycle (TDZ crash when the SDK barrel loads first), now guarded by a subprocess load-order test.

No follow-up issues needed unless #4 still reproduces for you on this head — if it does, that repro would be gold.

---

Generated by Claude Code

[itlackey]

Follow-up retest after the fixes landed.

Pulled latest claude/workflow-orchestration-improvements-k39vjo and reran the workflow-focused validation set.

Automated retest

• Passed:
  • bun test tests/commands/workflow-driver-cli.test.ts tests/workflows/brief.test.ts tests/workflows/report.test.ts tests/workflows/conformance/driver-parity.test.ts tests/workflows/dispatch-disposal.test.ts tests/workflows/validate-summary.test.ts tests/integration/worktree-isolation.test.ts
  • bun test tests/workflow-cli.test.ts tests/workflows/status-units.test.ts tests/workflows/program-warnings.test.ts tests/workflows/scheduler.test.ts
• Result: 203 pass, 0 fail

Manual retest in a fresh sandbox

• Sandbox root: /tmp/opencode/workflow-pr714-repro
• Revalidated the originally reported areas:
  • claimed units now render as action: "claimed" and inject --session-id
  • workflow list --active excludes blocked runs
  • blocked required-gate recovery now advertises and supports workflow report --settle
  • budget ceilings still fail hard as intended
  • worktree isolation still keeps the base repo clean and journals worktree_path

Additional real-world manual scenarios I added and ran

• stale claim reclaim:
  • manually aged a claimed running unit in workflow.db
  • workflow brief surfaced it as action: "stale" with staleUnits.length === 1
  • a new driver could reclaim/finalize it
• scope isolation across directories:
  • started the same workflow from two different working directories under the same sandbox
  • each directory saw only its own active run in workflow list --active
  • scope keys differed as expected
• watch stream on blocked run:
  • workflow watch <run> --stream --interval-ms 5 exited cleanly
  • trailing envelope reported status: "blocked", streamed: true

Remaining manual caveat

• I can still reproduce one live-harness issue:
  • bun src/cli.ts workflow run workflow:worktree-proof --max-steps 1
  • functional behavior is correct: run completes, retained worktree is created, base repo stays clean
  • but with the real opencode harness the parent process still stays alive until caller timeout, even though the command already printed a successful completed payload
• So the review findings I raised earlier look addressed, but I would still treat the live-harness disposal/exit behavior as not fully closed.

Runbook updated

• Added/expanded: docs/technical/pr-714-workflow-validation-repro.md
• It now includes:
  • prerequisites
  • exact sandbox setup
  • exact config/workflow asset creation steps
  • automated suites to run
  • manual scenarios with shell variables instead of hardcoded run ids
  • explicit cleanup steps
  • notes on the remaining live-harness hang caveat

If helpful, I can split the remaining live-harness hang into a dedicated follow-up issue with the minimal repro from the runbook.

[fwdslsh-dev]

Thanks for the retest and the runbook — and especially for confirming the live-harness hang survived the registry drain. That repro pinpointed the real mechanism, now fixed in 8eae5dc:

Why the drain wasn't enough: disposeDispatchResources() was firing correctly, but the SDK's own createOpencodeServer close() only sends SIGTERM — and it never unref()s the child process or its stdio pipes. In Bun/Node, a spawned child's handle keeps the parent's event loop alive until the child actually exits. A real opencode serve (a live HTTP server, often with provider children) can outlive SIGTERM long enough that akm sits pinned on its handle indefinitely — exactly your symptom: successful payload printed, process alive until caller timeout. The fake-factory regression test couldn't see this because a fake has no OS handle.

The fix: the runner now owns the spawn (createManagedOpencode — the SDK package is used only for createOpencodeClient). After the URL handshake, the child and its pipes are unref()ed/destroyed so the handle can never hold akm open; close() sends SIGTERM and arms an unref'ed 2-second grace timer that escalates to SIGKILL. akm's exit is now fully decoupled from how the server child feels about dying. The spawn stays in the factory call's synchronous prefix, so the env-binding overlay contract is unchanged.

Proven against real children this time (tests/integration/opencode-sdk-managed-server.test.ts): a real spawned process that deliberately ignores SIGTERM — handshake resolves, closeServer() returns in <500ms without waiting, the stubborn child is SIGKILLed within the grace window, and the test process itself exits promptly (which it wouldn't with an un-unref'ed live child).

Your runbook's workflow run workflow:worktree-proof --max-steps 1 repro should now exit cleanly with the real binary — that's the one thing our sandbox genuinely can't verify (no opencode here), so a quick re-run on your side would close it out. If it still hangs after this, a dedicated issue with the runbook repro would be the right next step, but I'd bet on this one.

---

Generated by Claude Code

[itlackey]

Follow-up: I implemented the shared spawn-layer fix, reran the regressions, and then reran the manual verification end to end.

What changed

• Fixed the shared CLI harness spawn cleanup in src/integrations/agent/spawn.ts
  • readStream() now captures, clears, and unref()s its drain timeout
  • the timeout-path SIGKILL grace timer is now unref()'d too
• Added regression coverage in:
  • tests/architecture/agent-spawn-seam.test.ts
  • tests/agent/agent-spawn.test.ts
• Updated the manual verification runbook:
  • docs/technical/pr-714-workflow-validation-repro.md

Regression results

• Passed:
  • bun test tests/architecture/agent-spawn-seam.test.ts tests/agent/agent-spawn.test.ts tests/workflows/dispatch-disposal.test.ts tests/integration/opencode-sdk-managed-server.test.ts
  • bun test tests/workflows/brief.test.ts tests/workflows/report.test.ts tests/integration/worktree-isolation.test.ts
• Result: 116 targeted tests passing, 0 failures

Manual verification rerun

• External-driver happy path: passed
• Required-gate block / resume / settle: passed
• Budget ceiling: passed
• Stale claim reclaim: passed
• Scope isolation across directories: passed
• workflow watch --stream on blocked run: passed

Live harness matrix after the fix

• opencode
  • workflow completed
  • retained worktree still journaled
  • command now exits normally
• codex
  • workflow failed terminally due to local auth (401 Unauthorized)
  • command now exits normally
• claude
  • workflow failed terminally due to local account state (Credit balance is too low)
  • command now exits normally

So the cleanup hang no longer reproduced in the live harness matrix I reran.

Production-style examples added to the manual workflow
To keep the manual verification grounded in real live usage, the runbook now also includes safe verification of production task/workflow assets from the current stash:

1. Prompt-backed live task example

• /home/founder3/akm/tasks/curate-agent-learning.yml
• drives workflow:curate-to-wiki

2. Command-backed live task example

• /home/founder3/akm/tasks/akm-health-report.yml
• env-injected hourly Discord health report

3. Converted production orchestration example

• source markdown workflow: /home/founder3/akm/workflows/web-ux-validation-gate.md
• converted YAML workflow-program: /home/founder3/akm/workflows/web-ux-validation-gate-program.yaml
• the new YAML validates cleanly as a workflow program with zero warnings and is documented in the runbook as a production-style orchestration example

Pushed commit

• 265ee555 — fix(agent): release workflow-run spawn timers cleanly

One note: the converted web-ux-validation-gate-program.yaml lives in the working stash (/home/founder3/akm/workflows/), not in this repo, so it was used/documented for manual verification but is not part of the git push for this PR.

[fwdslsh-dev]

Great to see the live-harness matrix (opencode/codex/claude) all exit cleanly now — that closes finding 4. Your spawn-timer commit 265ee55 is integrated on the branch.

It did trip one CI casualty (test-unit (1)): reflect-propose-cli.test.ts's "short --timeout-ms is honoured" hung to 30s. Root cause was in the test, not your fix — that test's fake setTimeoutFn stored a single shared timerCb, but now that readStream routes the two stream-drain watchdogs through the same injected seam, the later drain-timer registrations clobbered the wrapper-timeout callback so it never fired and the hanging spawn wedged. Fixed in ee99d42 by firing each callback independently (matching the array-based fakes your new agent-spawn tests use). No production change; your spawn-seam / agent-spawn / dispatch-disposal / managed-server suites all pass together, and I swept for any other single-timerCb fakes — this was the only one.

The two spawn fixes are complementary and both needed: yours releases the CLI-path drain/SIGKILL timers, mine (8eae5dc) owns the opencode serve spawn so the server child is unref'd and force-killed on close. CI should be green once the new head finishes.

---

Generated by Claude Code

[fwdslsh-dev]

Heads-up on the CI red on 01598ab (test-unit shards 2/3/4) — I dug in but am holding off pushing since it's your active rework. Diagnostic findings:

The failures are all the same class and look environmental, not a hard bug:

• Failing tests: tests/secret-indexing.test.ts ("secret values never appear in the FTS index…") and tests/parameter-metadata.test.ts ("indexed command entries include parameters in search text") — both 30s timeouts, not assertion failures.
• Both drive indexing via runCliCapture, which needs an embedder — and embedder downloads are blocked in CI (the known pre-existing flake family: e2e/semantic/zero-config/FTS-ranking).
• They pass in isolation on 01598ab (2–3s each) and pass running the two files together locally (23 pass, 10s). They only hang in the full 58-file CI shard, and they were green on the prior head ee99d42 — so this is shard-load-dependent, not deterministic.

Most likely mechanism: these blocked-embedding indexing tests already run near the 30s wall under shard resource pressure, and your readStream rewrite (Bun-native new Response(stream).text() → a manual getReader() loop with per-chunk decoder.decode() + Promise.race per chunk) adds a bit of per-spawn drain latency. readStream is on the hot path for every captured subprocess, so a small per-call cost compounds across a shard and can tip borderline-slow indexing spawns over the timeout. The hardening itself (structured StreamReadResult, timeout/error surfacing) is good — it's just the extra per-spawn cost interacting with the already-flaky blocked-embedding tests.

I didn't push anything since you're iterating on these exact files and there's no confident one-line fix — a CI re-run may well go green (flaky), or if you want the throughput back, restoring the Bun-native drain for the non-timeout/non-error path while keeping the structured result would remove the added latency. Happy to take either on if you'd like — just say the word. My two spawn-adjacent commits (8eae5dc managed serve-spawn, ee99d42 reflect test-fake) are unaffected by this.

---

Generated by Claude Code

[fwdslsh-dev]

Quick note on the test-unit (3) red on 6a93892: it's a flaky pre-existing test, not your codex-builder change. The failure is tests/proposal-storage-sqlite.test.ts › concurrent reject + gate-decision mutation cannot revive a pending row — an assertion failure (143ms, not a hang) in a real-SQLite-WAL concurrency test, an unrelated subsystem to 6a93892 (which only touched codex/agent-builder.ts + its test). I reproduced against your exact head: it passes 8/8 locally (5× isolated + 3× full-file), so it's a WAL timing flake under CI shard load, not a regression.

Safe to re-run or ignore — your next push will re-trigger CI and it'll almost certainly go green (it was green on 19c66c71). I tried to kick a failed-jobs re-run myself but the integration lacks the workflow-rerun permission (403). I didn't touch the flaky test — it's out of this PR's scope and you're actively iterating — but if you'd like it de-flaked (the concurrent reject/gate-decision timing window looks tightenable), say the word and I'll take it on separately.

---

Generated by Claude Code

[itlackey]

Post-merge triage note (0.9.0): PR #714 merged and closed PR-linked issue #559. Remaining open 0.9.0 milestone items are #711, #692, #672 (all still open after sync/main). I did not find any other open 0.9.0 milestone issues. Work split: #711 tasks multi-stash execution, #692 search salience ranking config/outcome gate, #672 improve prep accumulator + salience/outcome repo/widened X4 boundary; none were addressed by #714.