v8 spec architecture review

major | task path has no freeze stage today, and the spec never says whether it gets one | runPromptTask/runCommandTask in src/tasks/runner.ts resolve engine/model/cascade against live loadConfig() synchronously inline (no compile/resolve/freeze staging, no persisted plan), while §5.4's two-stage freeze is specified purely in workflow terms (compileResolveFreezeWorkflow) — the spec asserts cascade.ts is "consumed at freeze (workflows) and dispatch (tasks)" but never states whether task dispatch runs compile→resolve→freeze in-memory-only or calls cascade.ts directly, skipping the asset-loader injection seam that persona/uses: resolution depends on. | spec §4.3, §5.4; src/tasks/runner.ts:461-567; src/workflows/ir/freeze.ts:43

major | AgentUnitExecutor seam glosses over two structurally different dispatch surfaces | Workflow dispatch already flows through UnitDispatchRequest (frozen FrozenEngineSnapshot, IrInvocation, env: Record<string,string>, journaled via workflow_run_units) in src/workflows/exec/unit-dispatch.ts, but the task-prompt path (runPromptTask) builds a RunnerSpec from live config via resolveEngine/resolveLlmEngineUse and calls executeRunner() directly, then writes to task_history/logs.db — a completely different type and persistence shape; "one AgentUnitExecutor, two callers" needs an adapter layer the spec doesn't name, not just a shared strategy interface. | src/workflows/exec/unit-dispatch.ts:9-31; src/tasks/runner.ts:479-539

major | ShellUnitExecutor claim ignores that the task runner's shell path is a distinct, already-hardened subsystem | runCommandTask already owns process-group kill-ladders, AKM_EVENT_SOURCE stamping, resolveNestedAkmCommand, and its own log/history sinks (src/tasks/runner.ts:250-341); folding this into one ShellUnitExecutor shared with the orchestrator's Bun.spawn() path is plausible but the spec treats it as "just" a strategy swap (§5.1) when it actually requires reconciling two independent timeout/kill/logging contracts — not costed anywhere in §9. | src/tasks/runner.ts:273-296; spec §5.1, §9

major | cascade module's two consumption times are not obviously bitemporally safe for uses: tasks/<ref> composition | A workflow step referencing tasks/<id> resolves that task's cascade layers at the workflow's freeze time (baked into the frozen plan), but the same task run standalone via akm task run <id> re-resolves cascade against config as of the actual run — the spec never states which config snapshot a step's uses: tasks/<ref> composition should read (config at workflow-freeze time vs. config at task-file's own authoring), so two runs of "the same" referenced task can diverge without either being wrong per the spec's own text. | spec §4 layers table, §5.4; src/workflows/ir/freeze.ts:41 ("only source-to-runtime boundary")

major | persona snapshot / "show layer" reuse is directionally sound but the show layer isn't a resolver today, it's a CLI-output formatter | The provenance ceiling the spec cites is real (src/commands/read/show.ts:433-445, isPrimaryStash gate deleting toolPolicy for non-writable stashes) but it lives inside akm show's response-shaping code, not a reusable resolution function; extracting a {systemPrompt, toolPolicy, model, …} snapshot for freeze means carving a pure resolver out of a command handler that currently also does display formatting — a real but unscoped refactor, understated as "consumes the show layer's verdict." | src/commands/read/show.ts:433-445; spec §4 item 2

minor | IrInvocation as currently typed (engine: string required, model, timeoutMs, llm?) is agent/llm-shaped only — extending it to a {kind:"agent",…}|{kind:"shell",…} discriminated union is the right shape given IrRuntimeKind already exists as "llm"\|"agent"\|"sdk", but the spec should say explicitly whether IrRuntimeKind gets a "shell" member or whether shell units carry no runner field at all — decodeWorkflowPlanV3's validateInvocation/assertUnitEngineCompatibility (src/workflows/ir/schema.ts:481-527) hard-requires invocation.engine on every unit today, so the decoder rewrite is nontrivial, not a narrow "instructions become optional" patch. | src/workflows/ir/schema.ts:78-84, 383-452, 517-527; spec §5.2

minor | hashVersion 5 preimage table is a clean, additive extension of the existing pattern | step-work.ts's computeStepWorkList already documents its hash preimage exhaustively (hashVersion 4, lines 375-392) and explicitly walks through why each field is in/out; the v5 additions (target kind + ref + content hash, shell text, env literal values) fit the same discipline and the existing code's own doc comments make clear this is a well-understood seam to extend, not a risky one. | src/workflows/exec/step-work.ts:333-392; spec §5.3

minor | "one cascade module" name is right, but engine/model resolution logic that would feed it is currently duplicated three ways with subtly different fallback rules (exactModel/effectiveTimeout in freeze.ts, resolveEngine+inline overrides in runner.ts, resolveLlmEngineUse in engine-resolution.ts) — each has bespoke opencode-sdk-fallback handling; cascade.ts absorbing all three without losing the sdk-fallback edge cases (freeze.ts:174-186, 199-208) is the real engineering cost, understated by calling it "one cascade module." | src/workflows/ir/freeze.ts:163-209; src/tasks/runner.ts:479-514; spec §4 item 3

minor | env literal/ref provenance split matches an idiom the codebase already uses elsewhere (names-only hashing, resolved-value redaction sets) | step-work.ts already hashes env: template.env ?? null as ref names only and documents exactly the "hashing a resolved secret would leak it" rationale the spec restates in §5.5 — this is not a new pattern, it's naming an existing invariant, which is the correct minimal move. | src/workflows/exec/step-work.ts:353-358; spec §5.5

minor | type: task adapter-ordering claim is correct and lint-checkable today | BUILTIN_ADAPTERS in src/core/adapter/adapters/index.ts:73-90 does put akmWorkflowAdapter before akmTaskAdapter, both before the loose akmAdapter, confirming the spec's M8 claim that "workflow adapter is ordered first and claims .md files" is grounded, not asserted. | src/core/adapter/adapters/index.ts:73-90; spec §3

minor | GHA-alignment naming (uses:/run:/with:) is clean and the mutual-exclusion rule maps directly onto a single discriminated Target union — no simpler alternative stands out; the one soft spot is overloading uses: tasks/<n> to mean "compose this task's fields into me" while uses: workflows/<r> means "invoke and return," two different composition semantics under one keyword, which the spec acknowledges (no nesting rule) but doesn't name as an asymmetry worth flagging to authors beyond a doc note. | spec §2.2, §5.1

minor | over-engineered: threading llm overrides validation (assertUnitEngineCompatibility, validateLlmOverrides) through a shell invocation union member that never carries them is dead branch surface — the discriminated union (§5.1 {kind:"shell",...}) should structurally exclude llm/engine fields rather than relying on decoder-time cross-checks the way today's single-shape IrInvocation must; the spec doesn't say the shell variant drops the whole invocation wrapper, so decoder complexity may grow rather than shrink under v4. | src/workflows/ir/schema.ts:481-498, 517-527; spec §5.2

minor | under-specified: scheduler-id mapping (/ → -- with collision lint) and per-engine capability-declaration shape are both explicitly deferred to the implementer (§11 "Open items") — reasonable to defer, but subdir task ids interacting with validateTaskId's current rejection of / (cited in §9) means the migration step and the new id grammar must land atomically, which isn't sequenced anywhere in §8's migration steps. | spec §8, §11; src/tasks/task-id.ts (not read in depth but referenced by spec)