Skip to content

DECISIONS.md

Latest commit

 

History

History
196 lines (161 loc) · 9.94 KB

DECISIONS.md

File metadata and controls

196 lines (161 loc) · 9.94 KB
Raw

Architecture Decisions (taskflow)

Status: living document. Records the structural decisions behind the multi-host layout (taskflow-core / taskflow-mcp-core / taskflow-hosts / pi-taskflow / codex-taskflow / claude-taskflow / opencode-taskflow / grok-taskflow), the trade-offs that were considered, and the direction to take as the host count grows.

Written as the output of the PR #26 (claude + opencode hosts) architecture review. This is not a changelog — it captures why the structure is what it is and what should change next.

The invariant that must never break

The engine (taskflow-core/src/runtime.ts) is host-agnostic. It speaks only to the SubagentRunner contract (runTask → RunResult). Adding hosts (codex, claude, opencode, grok, + pi) changed zero lines of engine code. Any future restructuring MUST preserve this seam — it is the single reason a host SDK breaking change cannot force an engine release.

Consequence: the spawn/classify/idle-watchdog boilerplate that every host runner needs MUST stay funneled through the single runSubagentProcess in runner-core.ts. A new host that copy-pastes its own process lifecycle is a bug waiting to happen (the original 3-way copy-paste already diverged on contextTokens). One process lifecycle, ever.

Decision A — taskflow-hosts: a shared host-runner package (DONE)

Status

Implemented. Non-pi host runners (codex / claude / opencode / grok) live in a single taskflow-hosts package. Delivery packages (codex-taskflow / claude-taskflow / opencode-taskflow / grok-taskflow) keep their npm names, install paths, version pins, and plugin scaffolds, and import their runner from taskflow-hosts; each also re-exports the runner so its existing public surface (import ... from "codex-taskflow") is unchanged. A new host lands as one <host>-runner.ts in taskflow-hosts, not a whole new runner-owning package. Grok Build shipped this way (grok-runner.ts + grok-taskflow delivery + .grok-plugin scaffold).

Context

codex-taskflow, claude-taskflow, opencode-taskflow, and grok-taskflow are each published as a separate npm package. This was reasonable at 1 host (codex) and tolerable at 3–4 delivery adapters. The review's concern: at ~10–15 hosts this becomes the dominant maintenance cost.

The tension

There is a real reason adapters look like independent packages: each is a host ecosystem's delivery artifact (codex plugin add taskflow@taskflow, npm i -g codex-taskflow, an MCP server a user points their client at). But there is no reason their release cadence is independent — adapters almost never change except when taskflow-core's contract changes or a host CLI changes its flags. Today all nine packages are lockstep versioned at the same number, which makes a per-package semver meaningless: a codex flag fix forces a new version of the untouched core engine.

Cost projection at N hosts

5 hosts (now: pi + 4 MCP) 12 hosts (projected)
npm publishes per release 8 8 (+ thin delivery only if install brand requires it)
package.json to keep version-pinned 8 ~8
README/CHANGELOG package rows 8 ~8
npm names consumed (*-taskflow delivery) 4 4+ only when a host needs its own install brand

Decision

Keep the three existing packages for backward compatibility (their names are already in codex plugin add, npm i commands, user configs). Do NOT create a new package per future host. Instead, introduce a single taskflow-hosts package that re-exports all host runners + their MCP bins from one published unit:

taskflow-hosts
├─ codex-runner.ts        ← lives here directly
├─ claude-runner.ts       ← lives here directly
├─ opencode-runner.ts     ← lives here directly
├─ grok-runner.ts         ← lives here directly (Grok Build)
├─ <future-host-a>.ts     ← lives here directly
├─ test/                  ← all host arg-contract + parser tests
└─ index.ts               ← barrel re-exporting the host runners
  • Current publishes: core → mcp → hosts → pi/codex/claude/opencode/grok (8 publishes). Future runners ship inside taskflow-hosts; new delivery packages only when the host needs a branded install (grok plugin install, codex plugin add, …).
  • Future hosts ship in taskflow-hosts and are discovered via npx -p taskflow-hosts <host>-mcp or static import.
  • The three legacy packages can later become thin re-exports of taskflow-hosts (deprecated in their READMEs) without breaking the install commands users already have.

Trigger to act

✅ Done at 3 hosts (the moment adoption is lowest, so the migration is cheapest). taskflow-hosts now exists; Grok Build was added as the 4th non-pi runner without a second process-lifecycle copy — future hosts go here.

What we explicitly reject

  • A unified HostConfig interface / generic command-builder. Each host's argv genuinely differs (codex pastes the prompt; claude uses --append-system-prompt; opencode uses provider/model ids where codex/claude/grok use flat ids; permission models are sandbox vs --allowedTools vs --auto vs Grok's --tools + --always-approve). Forcing these into one interface produces an abstraction with a per-host parameter for every flag — more complex than the short pure builders it replaces. Instead: each host owns a pure, exported buildXxxArgs builder (extracted in this PR) that is independently unit-tested. Shared shape, not shared code.

Decision B — host CLI contracts are locked by unit tests, not e2e

Context

The executor e2e suites (e2e-codex.mts, e2e-claude.mts, e2e-opencode.mts, plus e2e-grok-mcp.mts for MCP) spawn a live host CLI and need auth + spend tokens, so they never run in CI. That left each host's argv construction (the --json / --format json / --output-format stream-json flags, the permission→flag mapping, the model-id resolution rules) completely untested in CI. A host renaming a flag would only be caught by a user at runtime.

Decision

Each host runner exposes a pure buildXxxArgs(ctx) (no process.env, no spawn) plus its already-pure permission/model helpers, and these are covered by *-args.test.ts files that run in the normal CI unit glob. The tests pin:

  • the exact leading flags (exec --json --skip-git-repo-check, -p --output-format stream-json --verbose --strict-mcp-config, run <prompt> --format json);
  • the permission mapping (read-only vs mutating whitelists → the right flag);
  • model-id resolution (flat vs provider/ path vs {{placeholder}} → pass-through vs drop);
  • bin resolution (default + PI_TASKFLOW_*_BIN override).

The live e2e suites still exist (they verify the event-stream parser against real captured fixtures and the real handshake), but the flag contract is now CI-checked. A flag rename trips a unit test, not a user.

What we explicitly reject

  • Dropping the live e2e suites. They remain the only thing that catches a host changing its event-stream JSON schema (which buildXxxArgs cannot see). Two layers: unit (flag contract) + manual e2e (stream schema). Neither replaces the other.

Decision C — structured run-log header is opt-in, stderr-only

Context

Cross-host debugging ("why did my flow fail?") previously had only taskflow_peek (phase output) and a 64KB-capped raw stderr per child. With 4 hosts, each child's stderr has a different CLI prefix (codex exec, grok -p, claude -p, opencode run), making it hard to tell which agent/bin produced a given error blob.

Decision

runSubagentProcess emits a structured header [taskflow:run] agent=<name> bin=<bin> model=<model> args=[...] to the host process's stderr, gated by PI_TASKFLOW_RUN_LOG=1 and default-off.

  • It is never written to stdout (stdout is the JSON-RPC channel for the MCP server; polluting it would break the protocol). This is pinned by a test.
  • Default-off so it adds zero noise to a normal run; an operator turns it on when debugging.
  • Lives in the one shared runSubagentProcess so all hosts get it for free — no per-host logging code to drift.

Things deliberately left as-is (recorded so they aren't re-litigated)

  • runner-core.ts lives in taskflow-core. It is host-neutral (no host SDK import — only node:child_process), so keeping it in core does not violate the "core has zero host-SDK deps" rule. Moving it out (e.g. into taskflow-mcp-core) would force every host adapter to depend on a second package for no benefit.
  • Skill generation is single-sourced (skills-src/ + build-skills.mjs + a drift-guard test). New hosts add one entry.<host>.md and extend the comma host list; the skill body is shared. Do not per-host the skill body.
  • MCP server is its own package (taskflow-mcp-core) — a pure presentation layer over core. Pi users never pull MCP code. This boundary is correct.
  • Lockstep versioning is kept for now (all nine packages share a version). It is crude but it is less work than tracking which subset of packages need a given bump, and at 9 packages the cost is still low. taskflow-hosts and taskflow-dsl exist; the next revisit is when core genuinely needs to move on its own cadence independent of host CLI flag churn.

Test-suite layering (current, acceptable)

Layer What Runs in CI?
Unit parsers, builders, permission/model helpers, verify, interpolate, cache yes (*.test.ts glob)
Shared-process runSubagentProcess spawn/idle/abort/classify yes (runner-process.test.ts)
Arg-contract each host's buildXxxArgs flag contract yes (added in this PR)
E2E (stream schema + live handshake) real host CLI over stdio manual (needs auth/tokens)

Full suite is ~1090 unit tests in ~25s on CI across node 22 + 24. Adding a host now costs ~12 arg-contract tests (a few ms), not a new e2e job. This scales fine to ~20 hosts.