refactor(monorepo)!: restructure workspace with @bastani/workflows

[lavaman131]

Summary

Restructures the Atomic monorepo from two packages (packages/atomic + packages/atomic-sdk) into a focused six-package workspace and migrates the runtime requirement from Node.js ≥22 to Bun ≥1.3.14. Introduces @bastani/workflows (packages/workflows) as a pi extension delivering DAG-driven, multi-stage workflow authoring and execution — shipped as raw TypeScript with no compile step.

Key Changes

Package & Repository Structure

• Root package renamed from @bastani/atomic-monorepo (private, Node ≥22) to atomic-monorepo (private, Bun ≥1.3.14)
• Workspace simplified to packages/* — examples and test fixtures no longer treated as workspaces
• Replaced packages/atomic + packages/atomic-sdk with six focused workspace packages:
  • packages/coding-agent → @bastani/atomic — Atomic-branded fork of pi's coding-agent CLI
  • packages/workflows → @bastani/workflows — pi extension for multi-stage workflow execution
  • packages/intercom, packages/mcp, packages/subagents, packages/web-access — supporting packages
• @bastani/workflows ships raw .ts sources loaded by pi directly — no dist/ build artifacts

Workflow Authoring & Runtime API (packages/workflows)

• Added defineWorkflow, runTask, runParallel, runChain, resolveInputs authoring primitives
• Implemented model fallback handling with pre-run validation (WorkflowModelValidationError), input resolution, worktree support, and runtime config tunables (maxDepth, concurrency, statusFile)
• ConcurrencyLimiter — semaphore controlling per-run stage parallelism
• GraphFrontierTracker — infers DAG parent edges from JavaScript execution order for automatic stage dependency detection
• Structured persistence layer: createStore, store, compaction policy (re-emits in-flight runs across auto-compaction), and restore utilities
• CancellationRegistry — tracks and manages active background run AbortControllers (children aborted before primary)

Pi Extension Entrypoint (packages/workflows/src/extension/)

• Implemented /workflow and /workflows-doctor slash commands with argument completion
• Workflow tool with discovery, config loading, persistence, and MCP scoping
• Intercom routing (structural — no hard pi-intercom import), subagent delegation, and async/background run controls (kill, pause, resume, inspect)
• Cascade pause support, structured doctor card output, and inline HIL input forms
• ask_user_question HIL tool vendored from upstream

TUI Surfaces (packages/workflows/src/tui/)

• Status widgets, DAG graph overlays with sparse GraphCanvas renderer, stage chat, inline HIL input forms
• Workflow/session pickers, doctor cards, background UI adapter
• UI mockups for attach, dispatch, HIL, and stage-chat surfaces (ui/)

Built-in Workflows (packages/workflows/builtin/)

• deep-research-codebase — multi-wave parallel specialist pipeline (scout + research-history chain, locator/pattern-finder/analyzer/online-researcher waves, aggregator)
• open-claude-design — design-doc generation workflow
• ralph — autonomous task runner workflow

Dependencies

• Replaced OpenTUI + React + Claude Agent SDK with @earendil-works/pi-coding-agent and @earendil-works/pi-tui as peer dependencies
• Removed oxlint, ajv, bun-pty, yaml, and React-related dev deps
• Updated prek ^0.3.13 → ^0.4.0

CI / DX

• Migrated pre-commit configuration to prek; added push/PR prek hook checks
• Consolidated test scripts: test:unit, test:integration, test:all
• Updated GitHub Actions workflows for new workspace structure and Bun runtime
• Added Catppuccin theme variants (frappe, latte, macchiato, mocha)
• 86 new test files covering unit and integration suites (test/unit/, test/integration/)

Breaking Changes

Area	Before	After
Workspace packages	packages/atomic, packages/atomic-sdk	packages/coding-agent, packages/workflows (+ 4 supporting)
Workflows package name	@bastani/atomic-sdk	@bastani/workflows
Runtime	Node.js ≥22	Bun ≥1.3.14
Pi requirement	—	pi ≥ 0.74
Distribution	Built dist/ artifacts	Raw TypeScript via pi
Workflow API	Legacy CLI/SDK dispatch	defineWorkflow + pi extension runtime

Migration: Consumers of @bastani/atomic or @bastani/atomic-sdk must migrate to @bastani/workflows imports and install via pi package management.

Validation

• bun run typecheck ✅
• bun run test:unit ✅ — 1197 passing tests across 76 files
• bun run test:integration ✅ — 260 passing tests across 8 files
• bun run test:all ✅

Known Issues / Follow-ups

• packages/workflows/package.json has "private": true — remove before publishing
• GraphFrontierTracker.onSettle() produces incomplete parent edges for diamond-merge DAG patterns (e.g. A→B→D, A→C→D: D gets [C] instead of [B, C])
• extension/index.ts (2143 lines) and executor.ts (1506 lines) should be split in a follow-up
• tsconfig.json dropped noUncheckedIndexedAccess — consider restoring

[claude]

Code Review

Thanks for the substantial restructure. The new package layout, executor/runtime separation, and per-package test split feel coherent, and the test coverage (1197 unit + 260 integration) is very solid. The notes below mostly target rough edges introduced during the move rather than new architectural concerns.

Highest-impact items

• CLAUDE.md is stale and contradicts this PR. It still describes the project as "TUI built on OpenTUI" powered by "OpenCode SDK, Claude Agent SDK, and Copilot SDK," references .impeccable.md (deleted), packages/atomic/script/bump-version.ts (removed — actual path is scripts/bump-version.ts), and docs/opencode/, docs/copilot-cli/, docs/claude-code/ (all gone). Since CLAUDE.md is the entry-point doc for agents working in this repo, leaving it stale will misdirect every future contribution. Either update to describe the pi extension model, or delete the dead sections.

• engines / packageManager mismatch in root package.json. packageManager pins bun@1.3.13 but engines.bun requires >=1.3.14. A fresh bun install on a machine without 1.3.14 will warn, and any tooling that honours packageManager will install a version that fails the engine check. Pick one.

• packages/coding-agent/package.json still declares engines.node >= 20.6.0. Given the rest of the monorepo is Bun-only and CLAUDE.md says "do NOT use node," it's worth checking whether this is intentional (the fork still ships as a Node-runnable CLI?) or a leftover from the prior structure.

• typescript: "^6.0.3" as a root devDependency. TS 6 is unreleased at the time of this PR's draft; if it's a typo for ~5.6.x or similar this will surface only at first bun install post-merge.

Bugs / correctness

• packages/workflows/src/runs/shared/concurrency.ts:46-54 — release() has no guard against over-release. If a caller pairs an unbalanced release() (e.g. a bug in error handling) with no queued waiter, _running decrements unbounded and below zero, silently inflating subsequent capacity. The executor's finally blocks pair acquire/release correctly today, but this is a footgun worth defending against with an if (this._running > 0) this._running--; plus an assert in debug builds.

• packages/workflows/src/runs/foreground/executor.ts:893-912 — maxDepth rejection allocates a runId but never records anything in the store/persistence. Callers polling the store by runId will see no record. The doc says "reject before any store/persistence side effects" — fine, but you may want to at least omit the runId (return empty string) so consumers don't try to look it up. As-is, the runId is essentially a dangling reference.

• packages/workflows/src/runs/foreground/executor.ts:348-363 — truncateByBytes can cut UTF-8 mid-codepoint via text.slice(0, mid) (UTF-16 code units). Buffer.byteLength re-encodes the JS string to UTF-8, but slice cuts at a UTF-16 boundary, so a surrogate pair can be split, leaving a lone surrogate that re-encodes as U+FFFD. Either step mid to a valid codepoint boundary, or use a TextEncoder + byte-level slice + TextDecoder({ fatal:false }) round-trip to guarantee a clean cut.

• packages/workflows/src/runs/shared/worktree.ts:155-156 — worktree paths are placed in os.tmpdir() keyed only by runId and index. Functionally fine (UUIDs collide negligibly), but if two host processes share runId (e.g. resumed run replaying), git worktree add will fail because the branch already exists. The cleanupWorktrees path is best-effort, so half-cleaned state from a crash will block re-runs until manual cleanup. Worth documenting or detecting on startup.

• packages/workflows/src/runs/shared/worktree.ts:147 — safePatchAgentName strips characters to _ but does not enforce length bounds. Very long agent names produce very long filenames; on some filesystems (eCryptfs, certain FAT variants) the 255-byte path limit gets hit silently.

• packages/workflows/src/extension/discovery.ts:261-294 — importWorkflowFile collects every named export as a candidate. If a workflow file legitimately exports helpers/types alongside its default, every helper is validated as a WorkflowDefinition and generates INVALID_DEFINITION diagnostics. Consider filtering on the __piWorkflow: true sentinel before passing into applyBatch, so the diagnostic stream isn't polluted by every non-workflow named export.

Style / consistency

• Stale cross-ref comments. Many files contain cross-ref: v0.x packages/atomic-sdk/... comments pointing at code deleted in this PR (e.g. define-workflow.ts:8, registry.ts:7, several tui/*.ts files). These rot fast; either rewrite them to point at the new locations or remove them entirely — per CLAUDE.md, identifiers should document themselves.

• packages/workflows/src/runs/shared/worktree.ts uses tabs while the rest of packages/workflows/src/** uses spaces. Minor but visible in mixed editors.

• packages/workflows/src/extension/index.ts is 2163 lines. The other extension files (dispatcher, runtime, discovery, config-loader) are appropriately sized. Carving out tool registration, surface wiring, and lifecycle hooks into separate files would make future review markedly easier — this file is currently the long-pole for understanding the extension entrypoint.

• run (executor.ts) is ~600 lines after // 5. Build WorkflowRunContext. The closure-heavy style (release barriers, cascade pause/resume, stage handle registration, runTrackedStageCall) makes each individual flow readable but the file as a whole hard to step through. The ctx.stage(...) factory in particular is large enough to warrant its own module.

Security

• packages/workflows/src/runs/shared/worktree.ts:270-313 — runWorktreeSetupHook spawns a user-configured executable with the input piped on stdin. The path checks (must be absolute or contain a /) and the existence check are good. Two follow-ups:

  1. The hook receives the run's baseCommit and full repo paths via stdin JSON — this is fine for a trusted hook, but worth documenting that any setup hook can read/write the worktree, the original repo (via the node_modules symlink), and os.tmpdir().
  2. parseWorktreeSetupHookOutput doesn't bound stdout length; a runaway hook could OOM the parent. Adding a maxBuffer-equivalent guard would be cheap.

• .mcp.json ships <your-org> placeholder for @azure-devops/mcp. Not security-critical but it will fail at startup if the user runs bunx -y against the literal <your-org> value. Either remove or note in docs.

Test coverage

• 82 test files for ~8.7k lines of workflow source is genuinely strong, and the split between test/unit and test/integration reads well. Two gaps worth filling:
  1. No explicit test for the maxDepth rejection path returning a usable shape (current depth-enforcement.test.ts exercises depth but the no-store-recording side effect noted above isn't asserted on).
  2. truncateTaskOutput has no test covering multi-byte UTF-8 characters straddling the byte boundary.

Nits

• prek.toml runs bun run test:unit as a pre-commit hook for every commit. With ~1200 unit tests this could be slow on large PRs; consider making it a pre-push hook instead and keeping pre-commit light (lint only).
• packages/workflows/package.json peerDependencies.@bastani/atomic: "*" with peerDependenciesMeta.optional: true is unusual — most consumers can't satisfy * without explicit installs. Pin to the workspace version (or use workspace:* semantics if Bun supports it).

Overall this is a clean, well-tested refactor. The functional surface (workflow authoring + DAG executor + extension wiring) is in good shape; most of the items above are housekeeping around the move rather than blockers.

[claude]

PR Review — refactor(monorepo)!: restructure workspace with @bastani/workflows

Massive but well-scoped refactor. The core workflow runtime (packages/workflows) is the strongest part: tight types (essentially zero any/as any in source — the lone match is inside a string literal), immutable builder, abort-aware release barriers, and spawnSync with shell: false plus path-escape validation in worktree code. The test suite is real (1197 unit / 260 integration) and the executor is unusually well-commented for code this dense. Comments below in rough order of impact.

---

Workspace / tooling

1. Root packageManager vs engines contradicts itself. package.json sets "packageManager": "bun@1.3.13" while every package (including root) requires "bun": ">=1.3.14". Corepack-aware tooling will reject the declared pm against the engine constraint. Bump one or the other.

2. Two TypeScript versions in the same workspace. Root pins "typescript": "^6.0.3", packages/coding-agent/package.json pins "^5.7.3". Workspaces resolve TS per package, so bun run typecheck at root and cd packages/coding-agent && tsc will use different compilers — different diagnostics, different lib types. Either align both or document that coding-agent is intentionally pinned to upstream's TS.

3. bun run typecheck covers far less than the PR claims. Root tsconfig.json explicitly excludes packages/coding-agent, packages/subagents, packages/mcp, packages/web-access, and packages/intercom, leaving only packages/workflows, test/, and examples/ in the include set. The validation section's "✅" emoji glosses over this — five of the six workspace packages aren't being checked by the top-level script.

4. bun run lint isn't a linter. Both lint and typecheck in root and packages/workflows run tsc --noEmit -p ../../tsconfig.json. The PR description says oxlint was removed; the script wasn't. The prek bun-lint hook on every commit (prek.toml:19) and every push therefore runs tsc twice (bun-lint + bun-test-unit both transitively check). Either drop the alias or wire an actual linter back in.

5. Mixed indentation across packages/workflows/src. 35 of 114 .ts files use tabs (vendored ask-user-question/* and runs/shared/worktree.ts); the rest use 2-space. Split correlates with provenance, but the inconsistency reaches across sibling modules in runs/shared/. Running prettier once on the package would be cheap and keeps diffs clean going forward.

6. packages/coding-agent uses Vitest, contradicting CLAUDE.md. CLAUDE.md mandates bun test over Vitest/Jest, but packages/coding-agent/package.json ships "test": "vitest --run" with "vitest": "^3.2.4" in devDeps. If the fork intentionally keeps its upstream test runner, call that out in CLAUDE.md so contributors don't trip on it. Same goes for engines.node: >=20.6.0 only on coding-agent — the rest of the repo is Bun-only.

7. README TypeScript badge. README badges "TypeScript 6.x" but the published @bastani/atomic package is built against TS 5.7. Pick a story or two badges.

---

Runtime correctness

8. CancellationRegistry.register silently replaces controllers. packages/workflows/src/runs/background/cancellation-registry.ts:43-51: re-registering with the same runId swaps the controller in place without aborting the previous one. Anyone holding the original AbortController then loses the wire — its signal will never fire from registry.abort(runId). The executor at packages/workflows/src/runs/foreground/executor.ts:948-950 already routes around this via a comment ("avoid overwriting it"), which is the tell that this API is sharp. Either throw on duplicate runId, or expose replaceController and make register strict.

9. Caller-signal listener leak on the happy path. executor.ts:927: callerSignal.addEventListener("abort", ...) runs once-only, but the listener is never removed when the run completes normally (it's only { once: true }, which removes after firing). If a top-level workflow holds one AbortSignal and spawns many nested runs over its lifetime, each nested run accretes a listener that lives until either the parent aborts or is GC'd. Add a signal.removeEventListener(...) in the finally of run().

10. resolveInputs validates presence, not types. executor.ts:125-144 honors required and default, but never inspects the declared type ("text" / "number" / etc.). A workflow declaring .input("max_partitions", { type: "number", default: 4 }) will happily accept "4" as a string and crash later inside the stage body. If type-validation is deferred by design, document it on WorkflowInputSchema; otherwise add a guard.

11. Worktrees live under os.tmpdir(). worktree.ts:155-157: pi-worktree-<runId>-<index> goes in /tmp. On Linux that's typically tmpfs (RAM-backed, often half of system RAM) and crosses filesystems from the repo, so the node_modules symlink (linkNodeModulesIfPresent, line 177) will work but cross-filesystem git worktree operations can be flaky on some setups. Also, if the process is kill -9'd before cleanupWorktrees runs, the parent repo accumulates orphan worktree records. Consider git worktree prune on startup, or at minimum document recovery in the doctor card.

12. prepareDirectWorktrees and runChain chain-step worktrees per step. Each chain step creates and tears down its own worktree (executor.ts:756, :779), so a 5-step chain with worktree isolation pays 5× the git worktree add/rm cost and forfeits any continuity between steps. If that's intentional ("steps are independent units"), worth a doc line — otherwise consider one worktree per chain.

---

Smaller things worth a look

13. bun-test-unit runs the full 1197-test suite as a pre-commit hook. That will get slow fast and contributors will reach for --no-verify. Move it to pre-push, scope it to changed packages, or split out a "fast" subset.

14. builtin/ralph.ts hard-codes model names (openai/gpt-5.5, github-copilot/claude-opus-4.7, etc.). These are likely to drift faster than the workflow file; consider pulling from WorkflowModelCatalogPort so the catalog is the single source of truth for available models.

15. reviewApproved in builtin/ralph.ts uses substring matching on the LLM output ("patch is correct", "approved", "no actionable findings"). Easy to false-positive on phrases like "this is not yet approved." Either tighten the regex (anchored, with negative lookbehind) or have the reviewer emit a structured { status: "approve" | "request-changes" } payload.

16. @bastani/workflows ships raw TypeScript and is published privately ("private": true) with peer deps marked optional. Worth a quick callout in the README about how downstream consumers are expected to invoke the loader — discoverability is currently low.

---

Net

The core authoring API and executor are solid. The biggest blocker is #3 (typecheck doesn't actually check most packages): the PR's "✅ typecheck" claim is technically true for one package, but consumers reading the Validation section will assume it covers the whole workspace. I'd ship after items 1–4 are addressed and would treat 8–11 as fast-follow.

[claude]

Code Review — refactor(monorepo)!: restructure workspace with @bastani/workflows

Massive PR (+265k/-224k) — I focused on the new packages/workflows runtime, the persistence/store layer, and the Pi extension entrypoint, since the bulk of new behavior lives there. Skimmed the rest. Overall the architecture is well-factored: clean separation between authoring API (defineWorkflow), executor, store, and Pi-facing extension; ports for persistence/MCP/cancellation; tests cover the core paths. Below is a punch list, ordered roughly by severity.

🔴 Bugs / correctness

1. runOptionsWithAdapters silently overrides caller-provided store — packages/workflows/src/runs/shared/workflow-runner.ts:48-53. The spread happens before store: createStore(), so any runOptions.store passed by an external caller of runWorkflow() is discarded and replaced by a fresh in-memory one. Either drop the override (and let ...options.runOptions win) or store: options.runOptions?.store ?? createStore(). As-is, the public SDK contract leaks.
2. inspectRun ambiguous-prefix doc/code mismatch — packages/workflows/src/runs/background/status.ts:316-353. The JSDoc claims "ambiguous" is returned when a prefix matches multiple runs, and RunDetail's contract reads that way, but the implementation just find()s the first match silently. Either return { ok: false, reason: "ambiguous", matches: [...] } (mirroring resolveRunIdPrefix in extension/index.ts:755) or update the doc — the current state means a user typing a short prefix may attach to the wrong run with no warning.
3. packageManager ↔ engines.bun skew — root package.json pins bun@1.3.13 via packageManager while engines.bun requires >=1.3.14. Corepack-style tools will install 1.3.13 and then fail the engines gate. Bump one.
4. pauseRun marks the run paused before pause() settles — status.ts:284,303,307. void handle.pause() is fire-and-forget, then the run is recorded as paused synchronously. If pause() rejects on one stage, the run snapshot says paused but the stage is still running. At minimum await Promise.allSettled(...) and skip recordRunPaused if no handle actually transitioned (mirror what resume already does via the recordRunResumed guard).
5. Lint script is just typecheck — bun lint runs the same tsc --noEmit as bun typecheck. CLAUDE.md tells contributors to run both. After dropping oxlint there's no actual linter; either rewire lint to a real one (oxlint, biome) or drop the script and update CLAUDE.md to stop promising a separate lint stage.

🟡 Performance / scalability

6. Store snapshot is deep-cloned on every state change — shared/store.ts:169-173. notify() calls snapshot() which does JSON.parse(JSON.stringify(...)) over the entire run set. With one tool event per stage tick, this is O(M·N) for a long run with many tool events, and snapshots are pushed to every subscriber. For a multi-stage workflow with many tool events this becomes the dominant CPU cost. Consider:
   • lazy snapshot computation (compute on demand inside the listener)
   • structural sharing / immer-style updates
   • or version-bumped notifications where listeners pull on-demand.
     status.ts also uses JSON.parse(JSON.stringify(...)) 5× as a defensive copy boundary — same concern at lower frequency, but worth a structuredClone pass at minimum (faster than JSON, supports more types).
7. unsubscribe on _listeners: Set<> mutated during iteration — if a listener inside notify() unsubscribes itself, the for (const fn of _listeners) loop is fine for Set (iteration tolerates deletion of the current element), but inserting during iteration would yield the new listener immediately. Worth a brief guard or doc note.

🟠 API / ergonomics

8. runTask overload disambiguation is brittle — executor.ts:533-554 isRunOpts enumerates 17 keys to tell WorkflowDirectOptions from RunOpts. A new RunOpts field requires editing this list; missing the edit causes silent misrouting of the second arg. Either:
   • require explicit calling style (drop the overload, take a single options object), or
   • use a sentinel/branded type so the discriminator can't drift.
9. directRunId allocates UUIDs before validation can fail — when validateDirectModels rejects, failedDirectDetails returns a runId that was never recorded in the store. Anything that tries to look up that runId later (e.g. tracker UIs) will see a phantom. Either validate before allocating, or skip the runId in the failed case.
10. makeUnavailableUIContext returns rejected promises whose handlers depend on caller await — executor.ts:150-159. If a user accidentally calls ctx.ui.input(...) without await, the unhandled rejection crashes Bun's default unhandled-rejection mode. Consider returning a plain throw (or attaching a .catch(() => {})) and a clearer error type.
11. @bastani/workflows ships raw .ts and depends on Bun-only loaders — fine for the documented pi runtime, but the engines.bun: ">=1.3.14" in packages/workflows/package.json doesn't gate the actual host. Worth a peerDependencies or runtime check that fails fast with a useful message if someone tries to load it under Node.

🛡️ Security

12. discoverWorkflows dynamically imports any .ts/.js/.mjs/.cjs from <cwd>/.atomic/workflows/ and <homeDir>/.atomic/agent/workflows/ — discovery.ts:241-295. This is the documented design (it's how user workflows load), but worth being explicit in user-facing docs that opening a project in atomic executes any workflow file in that directory at startup. If you want defense in depth, consider:

• a one-time trust prompt per project (similar to VS Code's workspace trust)
• or restricting user-global discovery to files owned by the current user.

13. parseWorkflowArgs JSON-parses user input — safe (no eval), just confirming this is JSON.parse not vm.runInNewContext. ✅
14. No secret-scrubbing in persisted inputs — appendRunStart writes the resolved inputs map into the session entry stream. If a workflow input ever contains an API key (e.g. a "token" field), it lands in the persistent transcript and any compaction snapshot. Consider redacting fields whose schema marks them sensitive, or document that workflow inputs MUST NOT contain secrets.

🧪 Tests

15. Unit/integration coverage is solid (86 unit + 7 integration files for the workflows package alone) — good. Two gaps I'd suggest closing:
    • No reentrancy/deadlock test for ConcurrencyLimiter under nested workflow runs. Today nested run() calls each get their own limiter (createRunLimiter is per-run), which is correct, but a test asserting that nested runs at maxDepth - 1 complete without starvation would lock that behavior in.
    • No test for runOptionsWithAdapters honoring an externally-supplied store. Add one and watch it fail against bug add agent instructions #1 above.
16. Manual tests in test/manual/ aren't wired into any CI script. If they're expected to be run by humans, document that in CLAUDE.md; otherwise consider deleting to avoid bitrot.

🧹 Style / nits

17. Cross-ref comments to v0.x packages/atomic-sdk/... (define-workflow.ts:8) point at deleted code. Either remove or rewrite to point at the relevant test/spec.
18. status.ts:330 runs.length > 0 ? runs.find(...) : undefined is redundant — Array.find on empty arrays already returns undefined.
19. appendRunStart/appendStageStart calls in executor.ts are sync (no await) even though WorkflowPersistencePort.appendEntry returns synchronously today. If that ever becomes async, the executor will silently lose ordering. Worth a type-level guarantee that the port is sync-only or wiring await now.

Validation

PR description claims bun typecheck/test:unit/test:integration/test:all all pass. I trust the run; not re-running locally given the size.

---

Nice work — the executor's DAG/cascade-pause logic, the GraphFrontierTracker, and the per-run AbortController plumbing are particularly clean. The deepest concerns are #1 (silent store override), #2 (silent ambiguous-prefix attach), and #6 (JSON.parse(JSON.stringify(...)) in the hot path); the rest is polish.

[claude]

Code Review — PR #936 (part 1 of 4) — Massive refactor (2108 files, +266k/-225k). I focused review on the new @bastani/workflows package since it is the core of the change. Builtin workflows, TUI surfaces, and coding-agent were spot-checked. Summary: Architecture and public authoring API (defineWorkflow, runTask/runParallel/runChain, createStore, GraphFrontierTracker, CancellationRegistry) read well. Test coverage in packages/workflows is strong (executor 65 tests, discovery 47, cancellation 24). Findings cluster around concurrency lifecycle and type-safety violations of CLAUDE.md. BLOCKING/HIGH-PRIORITY: 1) Cancellation listeners and aborted controllers leak — packages/workflows/src/runs/background/cancellation-registry.ts:41-50 — register() silently replaces a prior entry without aborting/cleaning the old controller. The executor (runs/foreground/executor.ts:927) adds a permanent abort listener to callerSignal with no symmetric detachment when opts.signal is long-lived. abort() does not unregister, so the map keeps already-aborted entries. abortAll (:79-85) iterates _runs.keys() while handlers may call unregister() — concurrent mutation during iteration; snapshot the keys first. 2) Orphan stage handles when ctx.stage(name) returns without a tracked call — packages/workflows/src/runs/foreground/executor.ts:1078-1196 — ctx.stage() synchronously creates an inner context, registers a stage-control handle, subscribes for input. Disposal (disposeInnerContext, unregisterStageHandle, tracker.onSettle) only fires inside runTrackedStageCall. If the body calls ctx.stage("x") then throws (or never invokes prompt/complete/subagent), all those resources leak for the entire run. Track spawned stage cleanups and dispose them in the outer finally. 3) loadFromPaths will import() arbitrary specifiers — packages/workflows/src/extension/discovery.ts:328-355 — scanWorkflowDir filters .ts/.js/.mjs/.cjs (:244-246), but importWorkflowFile does a bare await import(filePath). Bun import() accepts URL specifiers, so an http:// or data: URI in globalWorkflows executes arbitrary code at extension boot. isAbsolute(rawPath) also allows escape from projectRoot via ../. Add an extension allowlist + reject URL-like specifiers + verify the path is under an expected root. 4) validateDefinition is too permissive — packages/workflows/src/extension/discovery.ts:146-165 — checks __piWorkflow and typeof run but not the inputs schema shape, so dispatcher.ts:137 resolveInputs() crashes at dispatch time on malformed definitions. Tighten the check so failures surface at discovery.

[claude]

Code Review — PR 936 (part 2 of 4) — Concurrency/correctness. 5) snapshot() deep-JSON-clones on every store mutation — packages/workflows/src/shared/store.ts:162-173 — every recordStageStart/recordToolStart|End/recordStageNotice calls notify() which deep-clones the entire store via JSON.parse(JSON.stringify(...)). For long runs with streaming tool events this is quadratic and produces noticeable GC pressure. It also silently strips functions/Date/Map. Consider lazy snapshots or structural-share/shallow-immutable snapshots. 6) GraphFrontierTracker is timing-dependent — packages/workflows/src/runs/shared/graph-inference.ts:26-51 — onSpawn/onSettle mutate frontier without locking and assume a synchronous spawn->await->settle ordering. If a workflow spawns a sibling stage inside a .then(), the inferred parent depends on event-loop order rather than the true DAG. Document the supported authoring patterns or capture the frontier at promise-construction time. 7) runDirectAsync fire-and-forget is unkillable — packages/workflows/src/extension/runtime.ts:289-335 — void background.then(...) (:311) never registers the run with cancellationRegistry. killAllRuns (index.ts:2064) only kills registered runs, so async direct runs leak past session shutdown and continue burning subagent tokens. Wire runId into the registry before returning. 8) awaitPendingPrompt has a TOCTOU window that can hang forever — packages/workflows/src/shared/store.ts:354-372 — the resolver is registered after recordPendingPrompt returns; if resolvePendingPrompt is called between the two, the response is dropped and the await hangs. Atomically construct the promise inside recordPendingPrompt. 9) setMcpScope has no try/finally guarantee — packages/workflows/src/extension/mcp.ts:73-83 — the comment claims "always clear scope" but the contract lives in the caller. If a stage between setMcpScope and clearMcpScope crashes, the scope leaks across stages. Move the cleanup into a finally or wrap with a disposable.

[claude]

Code Review — PR 936 (part 3 of 4) — Type safety, violates CLAUDE.md ("Avoid any and unknown"): (a) packages/workflows/src/extension/workflow-schema.ts:37,39,41,60 — Type.Any() on DirectTaskSchema.model, tools, noTools, WorkflowParametersSchema.inputs. LLM tool calls accept arbitrary payloads. At minimum inputs should be Type.Record(Type.String(), Type.Union([Type.String(), Type.Number(), Type.Boolean(), Type.Null()])). (b) packages/workflows/src/runs/foreground/executor.ts:178-192,611-614,696,740,829 — repeated (x as Record<string, unknown>) and as WorkflowArtifact[] / as WorkflowTaskResult[] casts. The WorkflowDefinition result type could be generic. (c) packages/workflows/src/runs/foreground/executor.ts:533-554 — isRunOpts structural sniff is fragile. Use a tagged discriminator. (d) packages/workflows/src/runs/foreground/stage-runner.ts:79-83 — Object.create(null) as ... casts; createStubAgentSession in workflow-runner.ts:55-98 returns an empty object cast to agent — first method call throws. Replace with a Proxy that throws a friendly error. (e) packages/workflows/src/shared/types.ts:283,327 — Record<string, unknown> on WorkflowPersistencePort.appendEntry and SubagentStageOpts.config. (f) packages/workflows/src/extension/subagents.ts:97,107 — payload as unknown as Record<string, unknown> is any-laundering; fix PiEventBus.emit signature instead. SMALLER FINDINGS: packages/workflows/src/extension/index.ts:730-739 — installInputInterceptor catch handler calls commandCtx.ui.notify unconditionally; if ctx.ui is undefined the catch itself throws into pi input pipeline. Guard ctx?.ui?.notify. Same file: commandCtx = ctx as PiCommandContext is an unchecked cast. packages/workflows/src/extension/config-loader.ts:115-123 — tryReadFile only swallows ENOENT; an EACCES on the global config (read-protected home dir) crashes extension boot. Convert to a diagnostic. packages/workflows/src/extension/subagents.ts:51 — injectWorkflowEnv does not sanitize runId/stageId before placing them in env vars. If those ever come from user input, newlines or shell metacharacters could flow to child processes. packages/workflows/src/runs/foreground/executor.ts:1067-1071 — rejectReleaseBarriers runs once on abort; barriers added after abort do not reject. packages/workflows/src/workflows/registry.ts:77 — remove returns this when key not present but register/merge always allocate a new wrapper. Inconsistent; add a comment or normalize.

[claude]

Code Review — PR 936 (part 4 of 4) — TEST COVERAGE: test/unit has 75 files (PR body says 76) covering executor, discovery, cancellation, store, dispatcher, persistence, TUI surfaces — strong for packages/workflows. test/integration has 7 files (PR body says 8): mock-extension-api, custom-registry, overlay-entrypoints, runtime-wiring, runtime-tunables, input-interceptor, mcp-entrypoint. packages/coding-agent has 123 in-package tests but uses vitest.config.ts, and the root bun test scripts only run test/unit and test/integration — CI gap: confirm coding-agent tests are actually executed in CI under the new scripts; bun run test:all from the root will not touch them. Untested areas: packages/web-access (25 files, no tests), most of packages/mcp OAuth (mcp-auth*.ts, mcp-oauth-provider.ts, mcp-callback-server.ts), runs/shared/worktree.ts, runs/background/job-tracker.ts, extension/render-call.ts/render-result.ts/renderers.ts, tui/graph-canvas.ts, tui/inline-form-editor.ts. The PR body count mismatch (76->75, 8->7) suggests files moved late in the PR; worth a final reconciliation. STYLE NOTES (non-blocking): package.json:13 — "lint": "tsc --noEmit" is identical to typecheck. With oxlint removed there is no actual linter wired up; document if intentional. CLAUDE.md is unchanged in this PR but its claims about the tech stack (OpenTUI / figlet / @clack/prompts) no longer match the new world (pi-coding-agent + pi-tui). Update before merge. OVERALL: the abstractions are sound and the test coverage on the core orchestration layer is impressive. Before merge I would want at least items 1-4 addressed and CI verified to actually run the in-package coding-agent suite.

[claude]

Code review

This PR is a structural rewrite — 2,094 files, 263k insertions, 222k deletions, six new packages and a runtime migration. A coherent end-to-end human review at this size isn't really feasible; if you can split it into landing PRs (e.g. workspace skeleton → `@bastani/workflows` core → extension surface → moved tests), each one becomes reviewable on its own and bisecting future regressions stops being a nightmare.

Below is what I caught from spot reads of the runtime, extension, and packaging.

Packaging / versioning inconsistencies

1. Bun version mismatch. Root `package.json` declares `"packageManager": "bun@1.3.13"` but `"engines": { "bun": ">=1.3.14" }`. The pinned package manager is below the declared minimum.
2. Workflows version vs. changelog. `packages/workflows/package.json` is at `0.8.0`, but `packages/workflows/CHANGELOG.md` lists only `[0.0.1] — 2026-05-15` as the initial release. Pick one.
3. `private: true` on a "publishable" package. `@bastani/workflows` is marked `private: true` yet the PR description and README call it a publishable pi extension. If publish is the goal, drop `private` (and add a `publishConfig` if needed); otherwise update the description.
4. `@bastani/atomic` peer dependency is optional + `"*"`. `peerDependenciesMeta` marks it optional, but `packages/workflows/src/runs/foreground/executor.ts:7`, `extension/discovery.ts`, and `extension/config-loader.ts` all do `import { CONFIG_DIR_NAME } from "@bastani/atomic"` on the hot path. If a consumer installs `@bastani/workflows` without `@bastani/atomic`, those imports fail at load time. Either inline `CONFIG_DIR_NAME` (it's a string), or make the peer mandatory and pinned to a real version range, not `"*"`.
5. TypeScript v6 in root, v5.7 in coding-agent. Root devDependency is `"typescript": "^6.0.3"`, but `packages/coding-agent/package.json` pins `"typescript": "^5.7.3"`. Two major TS versions in one workspace is asking for cross-package type drift; please align unless there's a concrete reason.
6. vitest in coding-agent vs. "use bun test" in CLAUDE.md. `packages/coding-agent/package.json` keeps `"test": "vitest --run"` and `vitest: ^3.2.4`. Top-level `CLAUDE.md` says: "Use `bun test` instead of `jest` or `vitest`". Either migrate or carve an explicit exception in `CLAUDE.md` so the rule isn't half-true.

Workspace / tooling

7. `typecheck` doesn't typecheck most of the repo. Root `tsconfig.json` `include` is `["packages/workflows//*", "test//", "examples/**/"]` and `exclude` lists every other workspace package (`coding-agent`, `subagents`, `mcp`, `web-access`, `intercom`). The root `"lint": "tsc --noEmit"` and `"typecheck": "tsc --noEmit"` scripts therefore only cover workflows and tests. Either include the other packages, or document that CI runs per-package typechecks separately.
8. Workspace test script reaches outside the package. `packages/workflows/package.json` runs `"test:unit": "bun test ../../test/unit"`. The workspace's tests live at the repo root, not in the package. That's surprising for anyone running `bun run --filter @bastani/workflows test` and makes it harder to publish the package with its own tests. Consider moving tests under `packages/workflows/test/`.

Runtime correctness / behavior

9. `store.ts:snapshot()` deep-clones via `JSON.parse(JSON.stringify(...))` on every notify (`packages/workflows/src/shared/store.ts:165`). Every `recordToolStart`/`recordToolEnd` fires `notify()` → full snapshot clone, even when no subscriber is listening. For streaming tool events on a long run this is wasteful and will show up on profiles. Cheap wins: lazy snapshot computed on demand, and/or version-keyed memoization so multiple subscribers in the same tick share one snapshot.
10. `discovery.ts:importWorkflowFile` calls `await import(absPath)` with a raw filesystem path (`packages/workflows/src/extension/discovery.ts:267`). On Windows, ESM dynamic import requires a `file://` URL — passing a plain `C:...` path throws `ERR_INVALID_URL`. Use `pathToFileURL(absPath).href`.
11. `ConcurrencyLimiter.release()` can drive `_running` negative if release is called more times than acquire (`packages/workflows/src/runs/shared/concurrency.ts:46`). Probably won't happen via the `run()` helper, but `acquire()`/`release()` is also exposed publicly and the executor uses the raw form. One stray `release()` in an error path and the counter is off forever. Either assert `_running > 0` before decrementing, or guard against the case.
12. `cancellation-registry.ts:unregister()` doesn't clear children (`packages/workflows/src/runs/background/cancellation-registry.ts:90`). If a future caller unregisters a run that still has live child controllers, those become unreachable — no `abort(runId)` will hit them. Today the only call site is `executor.run()`'s `finally`, so it's safe, but defensive cleanup makes the API harder to misuse.
13. `validateConfig` allows `maxDepth: 0` (`packages/workflows/src/extension/config-loader.ts:144`). The executor enforces `depth >= maxDepth` (`executor.ts:902`), so `maxDepth: 0` immediately rejects every top-level run. Either require `>= 1` in the validator, or document the semantic.
14. `config-loader.ts:tryReadFile` dynamically imports `node:fs/promises` per call (`packages/workflows/src/extension/config-loader.ts:122`). The module is always loaded; the dynamic import adds latency on every read with no benefit. Hoist it to a top-level static import.
15. `worktree.ts:linkNodeModulesIfPresent` symlinks the host `node_modules` into each parallel worktree (`packages/workflows/src/runs/shared/worktree.ts:177`). If two parallel tasks run a tool that mutates `node_modules` (a `bun install`, a postinstall script), they race on the shared tree. At minimum, document that workflows touching dependency installation must disable the symlink — or skip linking when worktree count > 1 and emit a notice.
16. Large single-file modules. `runs/foreground/executor.ts` is 1506 lines; `extension/index.ts` is 2143 lines. Both bundle disparate responsibilities. Splitting by responsibility now will pay back during incident reviews.
17. `executor.ts:descendantsOf` / `hasAncestor` are O(n²) over `runSnapshot.stages` (`executor.ts:983-1006`). Fine for small DAGs, but the executor calls `descendantsOf` per pause/resume cascade; a memoized children-adjacency map costs nothing and removes a foot-gun for large workflows.

Security

The git operations in `packages/workflows/src/runs/shared/worktree.ts` use `spawnSync(..., { shell: false })` consistently and `safePatchAgentName` sanitises the agent name before joining it into a path — no shell injection or path injection found there. One thing worth calling out in docs (not a bug): `runWorktreeSetupHook` happily executes any path the config points at — the threat model rightly assumes the config author is trusted, but readers of the README should be reminded that enabling setup hooks for an untrusted repo runs that repo's code.

Tests

144 test files moved/added under repo-root `test/`. Coverage looks comparable to what was removed from `tests/sdk/` and `tests/services/`, but a few cases I'd like to see explicitly:

• `ConcurrencyLimiter`: an extra `release()` does not corrupt state.
• `CancellationRegistry`: `unregister(runId)` after `registerChild` does not orphan the child controller (or document that it does).
• `awaitPendingPrompt` race: a run is killed between `recordPendingPrompt` and `awaitPendingPrompt`; the awaiter rejects cleanly.
• Windows: `discoverWorkflows` importing from an absolute path on Win32 (likely fails today because of (10)).

Nits

• README and package metadata use `@bastani/workflows`, but several internal error messages still say `"pi-workflows: ..."` (e.g. `executor.ts:925`, several spots in `validate-inputs.ts`). Prefer one identifier in user-facing strings.
• `tsconfig.base.json` is present (Node16, `types: ["node"]`) but doesn't appear to be `extends`-ed anywhere I can see. If unused, delete; if it's for a package's tsconfig, wire the `extends` in.

Happy to dive into any of these in a follow-up.

[claude]

Review — refactor(monorepo)!: restructure workspace with @bastani/workflows

Massive restructure — this is a great direction (focused @bastani/workflows extension, raw-TS distribution, Bun-only runtime). Type discipline is consistent (no any/unknown floating around, structural pi surfaces). Below are concrete items worth addressing before/after merge.

Bugs / correctness

1. GraphFrontierTracker.onSettle() drops diamond-merge parents (PR self-flagged; no test exists).
   packages/workflows/src/runs/shared/graph-inference.ts:43-51 — onSettle removes parents of the settled stage from the frontier. For A→B→D, A→C→D the order spawn(A), settle(A), spawn(B), spawn(C), settle(B), settle(C), spawn(D) ends with the frontier containing only the last settler. test/unit/graph-frontier-tracker.test.ts covers sequential / parallel / fan-in but never builds a diamond that re-uses an ancestor across two branches that settle non-atomically. Please add a failing test now so the bug is captured in the suite, then fix in a follow-up. A simple fix is to track per-frontier-entry refcounts (number of unsettled descendants) or rebuild the frontier from nodes + stageParents at each onSpawn.

2. GraphFrontierTracker.getParents typing lies.
   graph-inference.ts:33,59 — Object.freeze(parents) freezes the same array stored in stageParents, but getParents(): string[] advertises a mutable result. Either return readonly string[] or clone on read. Right now an inadvertent getParents(id).push(...) would silently no-op in strict mode and crash in non-strict.

3. CancellationRegistry.register silently replaces the primary controller.
   runs/background/cancellation-registry.ts:41-50 — the old controller is detached without abort. Combined with executor.ts:927 which registers an abort listener on the caller signal, a re-registration leaks the old listener for the lifetime of the caller signal. Either reject re-registration (throw) or abort the old controller before replacing. Comment on line 42 acknowledges this but doesn't guard.

4. store.recordStageAwaitingInput filter ignores awaiting=false for awaiting_input stage.
   shared/store.ts:438-447 — the if (stage.status === "completed" ...) return false guard rejects awaiting_input → running transition that is supposed to be the inverse case. Trace: recordStageAwaitingInput(awaiting=false) after the prompt resolves, against a stage in awaiting_input — falls through guard fine, but the symmetric guard for the upward transition (awaiting=true on a paused stage) silently returns false. Worth a unit test that exercises both directions across running ↔ awaiting_input ↔ paused.

5. store.snapshot() deep-clones via JSON.parse(JSON.stringify(...)) on every notify().
   shared/store.ts:169-172 — fired from every record* method. For a typical run with ~50 stages and many ToolEvents, this is N² work per second. Switch to structuredClone() (Bun supports it natively, faster, handles Date/Map-shaped values if they ever appear) — or notify listeners with the mutable _runs/_notices/_version and let consumers clone when they need a frozen copy. Same pattern in runs/background/status.ts:230-337 is fine since it's per-call, not per-mutation.

Performance

6. hasAncestor / descendantsOf / blockingAncestorFor are O(V+E) per call.
   executor.ts:984-1012, 1035-1057 — cascadeResumeFrom iterates descendantsOf and for each descendant calls blockingAncestorFor. For a wide DAG this is O(V²). Probably fine for typical workflow sizes (< 50 stages) but worth a cached descendants(stageId) memo keyed by tracker.version if the deep-research workflow keeps growing.

Code quality / maintainability

7. packages/workflows/package.json is "private": true.
   PR self-flagged. Confirm the publish gate happens before the first cut of the new package (CI/release docs say private: true will silently no-op npm publish).

8. extension/index.ts at 2143 lines and executor.ts at 1505 lines.
   PR self-flagged. The executor's main run() function is ~600 lines with a deeply nested ctx.stage closure — splitting runTrackedStageCall into its own file (taking tracker, limiter, releaseBarriers, etc. as args) would also let it be tested in isolation. Right now the only coverage is via the public run().

9. tsconfig.json dropped noUncheckedIndexedAccess.
   PR self-flagged. There's heavy use of array[i]! non-null assertions across executor.ts (e.g. chain[index]!, prepared.tasks[0]!, results[index - 1]) — restoring the flag would catch the next regression. The current code is correct, but the safety net is off.

10. Mixed indentation: runs/shared/worktree.ts uses tabs, the rest uses spaces.
    Worth running prek run --all-files (or whatever the project's prettier/biome config is) to normalize before the next release branch.

11. Test asserts on void return.
    test/unit/cascade-pause.test.ts:43 — assert.equal(store.recordStageEnd(...), undefined). recordStageEnd is typed void in the Store interface; asserting undefined is asserting on JS-runtime behavior rather than the API contract. Drop the assertion or change recordStageEnd to return boolean like the other record* methods for consistency.

Security

12. importWorkflowFile does dynamic import(filePath) from config-supplied paths.
    extension/discovery.ts:268 — by design (user is loading their own workflows). Worth a one-line note in the README / loadWorkflowConfig docstring that workflows from config.paths execute with full process privileges. Right now a misconfigured shared config file could load arbitrary code.

13. Worktree setup hook execution is safely shelled.
    runs/shared/worktree.ts:274-280 — spawnSync(hookPath, [], { shell: false, ... }), hook path resolved against repoRoot and existence-checked. No injection vector. The syntheticPaths normalization in normalizeSyntheticPath correctly rejects .. escapes. Looks good.

Test coverage

• 1197+260 tests is great. Targeted gaps:
  • graph-frontier-tracker.test.ts — add a diamond-merge case (issue add agent instructions #1 above) and a chained-fanin case (A→B, A→C, B→D, C→D, D→E) to catch any future frontier bug.
  • cancellation-registry.test.ts — add a "re-register same runId" case that asserts on the old controller's signal.
  • store.test.ts — round-trip awaiting_input ↔ running ↔ paused against recordStageAwaitingInput to lock the precondition guard.

Nits

• executor.ts:69 — trailing import without newline (} from "../shared/types.js";import { buildRuntimeAdapters } — formatter slip in extension/index.ts:69).
• executor.ts:60 — export interface ResolvedInputs extends Record<string, unknown> {} — empty extension. Just export type ResolvedInputs = Record<string, unknown>.

---

Overall: ship-worthy with the diamond-merge bug captured as a failing test before merge. Items 1–4 are correctness; the rest can land in follow-ups.

[claude]

Code Review — refactor(monorepo)!: restructure workspace with @bastani/workflows

Reviewed at a depth proportional to the parts of the diff most likely to bite in production. Skipped surface-level commentary on the 2k+ deleted skill files. The new @bastani/workflows runtime, extension entrypoint, TUI surfaces, tests, and workspace plumbing were each analyzed in detail.

Overall this is a large, well-structured rewrite with strong test discipline (1457 passing tests, real-implementation fakes, deterministic race tests). The findings below are issues worth resolving before/shortly after merge — none invalidate the architectural direction.

---

Security — High Priority

1. Path traversal -> arbitrary code execution via workflow discovery config

packages/workflows/src/extension/discovery.ts:329 resolves user-config-supplied paths via resolve(baseCwd, rawPath) and importWorkflowFile (discovery.ts:261-295) calls import(filePath) after only a stat check — no containment check that the path stays inside an expected directory. Combined with validateDefinition (discovery.ts:146-165) only gating on the __piWorkflow === true sentinel, anyone who can write .atomic/extensions/workflow/config.json (or the global equivalent) can inject a config entry pointing to ../../evil.ts and the extension will import() and execute it in-process with full tool/skill surface.

Mitigations to consider: normalize paths and require them to be under the project root or a per-user allowed-roots list; refuse paths containing .. after normalization; require workflow files to be inside a configured workflows/ directory.

2. Stage subagent privilege model

wiring.ts:255-287 documents that each stage session inherits the same themes/extensions/tools/skills as the orchestrator (wiring.ts:269-273). For an orchestrator framework this is a deliberate choice, but it deserves a SECURITY.md note: a workflow that imports a malicious subagent gets the same blast radius as the user.

3. Persistence may capture secrets

runs/foreground/executor.ts:958-961 persists resolvedInputs verbatim. If users pass secrets through workflow inputs (API keys etc.) they land in the session entries on disk. Worth either documenting (inputs are persisted, do not pass secrets) or adding a scrub layer keyed on input schema.

(continued in replies — correctness, code quality, performance, tests, CI, and strengths)

[claude]

Correctness — Concrete Bugs

4. AbortController registration is skipped for foreground runs that pass a signal — executor.ts:948-949

The guard if (!opts.signal) { opts.cancellation?.register(runId, ownController); } is intended to avoid overwriting a pre-registration from the detached runner. The condition is wrong: any foreground caller that passes opts.signal ends up unregistered, so cancellation.abort(runId) becomes a no-op for those runs. Fix: detect pre-registration explicitly (e.g. if (!cancellation.has(runId))) rather than gating on opts.signal.

5. SDK errors silently swallowed by pause/resume retry loop — stage-runner.ts:481-484

The catch block reads if (pauseRequest) { ...continue; } throw err;. If a non-pause SDK error (e.g. transient network failure) throws while pauseRequest is non-null, the catch discards the error, awaits the resume deferred, and retries with the resume message. The original error is gone. Distinguish pause-triggered aborts from real errors before consuming them.

6. Worktree cleanup leak on early abort — executor.ts:651-831 (runTask / runParallel / runChain)

createWorktrees runs synchronously before the workflow body executes; cleanup lives in try/finally inside the body. If ownController is already aborted when def.run(ctx) is invoked, the body never runs and worktrees + branches are leaked. Move cleanup outside the body or wrap creation in an outer try/finally.

7. Concurrency limiter waiters never reject on abort — concurrency.ts:38-41 and executor.ts:1222

limiter.acquire() is awaited unconditionally with no race against the run's abort signal. A stage queued for a concurrency slot at the moment of abort hangs until prior stages release — which only happens after their finally blocks. Race acquire against ownController.signal so aborted runs cancel queued stages immediately.

8. Extension lifecycle subscription leaks — extension/index.ts:2074, 2115

installStoreWidget(pi, store) (line 2074) and subscribeIntercomControl (line 2115) run unconditionally at factory load and assign to storeWidgetUnsubscribe / intercomControlUnsubscribe. The session_start handler (lines 2039-2040) reinstalls the widget with session-scoped UI and overwrites the previous unsubscribe ref — the original subscription leaks. Same shape for the intercom subscription on re-factory scenarios. Either do not pre-install before session_start, or unsubscribe before re-assigning.

9. HIL form required check bypassed for non-string typed inputs — index.ts:1739-1745

The empty-required guard only checks value === "". If a user passes count=0 via CLI and parseWorkflowArgs coerces it to the number 0, the picker is skipped — the workflow receives 0 for what was a required field. Use the input schema's required declaration plus a per-type emptiness check, not a string-only literal.

10. Slash command single-quote values are not unwrapped — index.ts:800-877

tokenizeWorkflowArgs keeps quote characters in the buffer; for prompt='hello world', JSON.parse("'hello world'") throws and the fallback path delivers the literal 'hello world' (including quotes) to the workflow. Add single-quote unwrapping or document the requirement to use double quotes.

11. delivery === "result" is dropped by shouldEmitControl — runtime.ts:264 and intercom/result-intercom.ts:125-127

shouldEmitControl only returns true for "notify" and "control-and-result". Passing delivery: "result" to emitWorkflowControlIntercom silently emits nothing. Either widen shouldEmitControl to include "result" or strip the call site so the dead path is not reached.

12. pauseRun snapshot timing — runs/background/status.ts:284-307

void handle.pause() is fire-and-forget; the returned snapshot reflects pre-cascade state. TUI consumers can briefly show a stage as paused that has not yet received its abort. Worth awaiting the handle.

13. disposeCurrentSession race window — stage-runner.ts:458-465

Sets session = undefined and sessionPromise = undefined before awaiting current.dispose(). External callers (steer, followUp, compact) reaching ensureSession() during this window will construct a new session before the old one finishes disposing. Either await dispose first or guard ensureSession with the in-flight dispose.

14. Minor — known DAG diamond-merge bug not pinned by a failing/skipped test

The PR description acknowledges GraphFrontierTracker.onSettle() produces incomplete parent edges for diamond merges, but there is no test.skip("documents diamond-merge bug", ...) in graph-frontier-tracker.test.ts. Add one to prevent regression bookkeeping loss.

[claude]

Code Quality / CLAUDE.md Compliance

CLAUDE.md forbids any and unknown. Most of the new code adheres; the violations worth fixing:

• packages/workflows/src/shared/store.ts:69 — resolvePendingPrompt(response: unknown) and awaitPendingPrompt(): Promise<unknown> in the public Store interface, propagated to ResolverEntry at line 163. The widget store is a structural surface — type the prompt response payloads.
• packages/workflows/src/extension/wiring.ts:423-424 — export type PiTheme = unknown; export type PiKeybindings = unknown. These are then cast at every callsite (e.g. stage-chat-view.ts:318) via as ConstructorParameters<typeof CustomEditor>[2]. Import the real types from @earendil-works/pi-tui or define structural interfaces.
• packages/workflows/src/extension/workflow-schema.ts:37-60 — TypeBox schema uses Type.Any() for model, tools, noTools, inputs. Constrain these.
• packages/workflows/src/extension/index.ts:91, 229 — stages?: unknown[] and [key: string]: unknown index signatures on the Pi*Context types.
• tui/stage-chat-view.ts:378-490 — event as { field?: unknown } casts throughout _appendEvent. The SDK event union should already discriminate by type; eliminate the structural re-cast.
• tui/graph-view.ts:648-649 — canvas as unknown as { rows: ... } reaches into GraphCanvas's private field. Expose a getCell(row, col) query instead.

Duplicated truncation helpers — normalizeMaxOutput, truncateByLines, truncateByBytes are duplicated identically in executor.ts:328-363 and stage-runner.ts:207-236 (~80 lines). Lift to a shared util.

Unbounded form-state map — inline-form-store.ts:46 retains every /workflow form state for the life of the module (acknowledged at line 14). Add a finalize hook that evicts after submission.

@bastani/atomic import in wiring — wiring.ts:140 does await import("@bastani/atomic") but JSDoc throughout (wiring.ts:86-91, 253) names @earendil-works/pi-coding-agent as the canonical module. Reconcile to avoid future confusion.

Error swallowing in diffWorktrees — runs/shared/worktree.ts:539-543 writes an empty patch on failure with no log/warning surface. Callers cannot distinguish 'genuinely no diff' from 'capture failed.'

---

Performance — TUI Hot Paths

• stage-chat-view.ts:863-870 — _statusPill calls blendBg/lerpColor (six per render, each does parseInt x6) at the 80 ms animation cadence. Theme-derived values are constant; cache them in the constructor when this.theme is set.
• stage-chat-view.ts:1069-1098 — A fresh ToolExecutionComponent is constructed inside _renderEntry on every frame; any internal state (subscriptions, image caches) is discarded each tick. Memoize per-toolCallId.
• stage-chat-view.ts:971-983 — _renderComponentTail renders every component from the tail and only breaks after the render call; for long transcripts with a tight height budget, many off-screen components are rendered then discarded. Move the break above the .render(width) call.
• graph-view.ts:487-490 and 800-801 — nodeByStageId/byId Maps are reconstructed every render. The layout itself is cached; cache the index alongside it.
• stage-chat-view.ts:767-768 — render() calls _currentStage() and _isBlocked() (which internally calls _currentStage() again), so store.snapshot() is traversed twice per frame. Pass the first snapshot down.

[claude]

Test Coverage

Generally strong — 1197 unit + 260 integration tests, real createStore not mocks, hand-rolled typed fakes rather than framework mocking. Specific gaps worth addressing:

• extension.test.ts is 12 lines for a 2143-line entrypoint. It only asserts the factory is callable and does not throw. The dispatch logic at index.ts:408-660 and the orchestration inside makeExecuteWorkflowTool/registerWorkflowCommand are not directly exercised. Slash parsing is well-covered by slash-dispatch.test.ts but the wiring is not.
• runs/shared/worktree.ts has no test file and is not imported in any test. Critical for the parallel workflows feature given it manages git branch + filesystem state outside the per-test temp dirs.
• tui/stage-chat-view.ts tool-card transcript and compaction UI paths are untested in stage-chat-view.test.ts (which covers keyboard routing well).
• overlay-graph.test.ts:47-78 stubs every store method as a no-op and asserts on _focusedIndex / _switcherOpen internals. Brittle; assert on rendered output where possible.
• as never escape hatches in stage-chat-view.test.ts:139,188, inline-form.test.ts:445,497, runtime-wiring.test.ts:137 weaken the type-level contract on what those host interfaces require.

---

Workspace / CI / Config

• Engine/packageManager mismatch — root package.json:5 pins packageManager: "bun@1.3.13" while engines.bun: ">=1.3.14" (line 7). Bump packageManager.
• packages/coding-agent declares engines.node: ">=20.6.0" with no bun engine (line 103). Repo CLAUDE.md is unambiguously 'Bun >= 1.3.14'; either add the bun engine here or document why the coding-agent fork keeps Node.
• vitest still present in packages/coding-agent/package.json (line 41 test script, line 85 devDep). Root CI uses bun test, so this is unused but adds install weight. Either drop or document why both runners coexist.
• publish.yml npm publish --provenance — per .claude-pr/CLAUDE.md this is the documented exception (OIDC provenance lives in the npm CLI). No NPM_TOKEN / NODE_AUTH_TOKEN present, which is correct. Just verified, no action needed.
• TypeScript major split — root uses typescript ^6.0.3, coding-agent uses typescript ^5.7.3 plus @typescript/native-preview 7.0.0-dev.20260511.1. Two majors across the monorepo will create subtle type-inference differences. Worth aligning, or documenting why coding-agent needs the older toolchain.
• Root tsconfig.json include (lines 32-39) only typechecks packages/workflows, test, examples. The other four packages are not covered by bun run typecheck. Consider expanding or document the divided typecheck strategy.
• noUncheckedIndexedAccess dropped (PR-acknowledged) — combined with the index signatures noted in the Code Quality section, this loses meaningful safety on the Pi*Context types. Restoring it would force fixes for cases the index signatures currently hide.
• packages/workflows private: true is actively guarded by publish.yml:94-98, so the cleanup mentioned in Known Issues needs the guard updated too — otherwise the publish workflow will error when the flag is removed.

---

Notable Strengths Worth Highlighting

• releaseBarriers / rejectReleaseBarriers (executor.ts:971-1071) — cascade-pause gating wired to the run's abort signal so no pending stage can hang on abort. Solid design.
• recordRunEnd terminal-status guard (store.ts:276-278) — prevents completed runs from being overwritten by late finalizers and signals to callers via boolean return.
• Worktree paths and branch names use unguessable UUIDs (worktree.ts:155-157) — concurrent runs cannot collide; runWorktreeSetupHook uses shell:false and JSON stdin (worktree.ts:274) — correctly hardened against shell injection.
• Post-stage-settle abort race test (executor.test.ts:1361-1431) uses a holdWorkflow gate to make the race deterministic — a precise regression test rather than a setTimeout approximation.
• runChain parallel-group handoff tests (executor.test.ts:792-831) exercise template interpolation end-to-end across mixed sequential/parallel steps.
• Mutable-ref + stable proxy pattern in extension/index.ts:1012-1037 cleanly handles async registry swap-in without closure updates.

---

Happy to dive deeper on any individual finding. The path-traversal in workflow discovery (Security #1), the AbortController guard bug (Correctness #4), the SDK error swallow (Correctness #5), and the worktree leak on early abort (Correctness #6) are the four items I would most want fixed before this lands as the new default.

[claude]

Review — refactor(monorepo)!: restructure workspace with @bastani/workflows

Thanks for the huge effort here. The new package split, defineWorkflow builder, frontier-based DAG inference, and pi extension entrypoints are clean and the test surface (1,457 tests across 81 files) is substantial. A few issues / suggestions, ordered roughly by importance.

Blockers (please address before merge)

1. CI typecheck only validates packages/workflows/. Root tsconfig.json explicitly excludes packages/coding-agent, packages/subagents, packages/mcp, packages/web-access, packages/intercom, and four of those packages (intercom, mcp, subagents, web-access) define no scripts.typecheck of their own. The PR description's bun run typecheck ✅ is therefore narrower than it reads. Either add per-package typecheck scripts wired into CI, or expand the root include / drop the exclusions so tsc --noEmit covers the whole workspace. Otherwise changes to those four packages can silently break types.

2. packageManager vs engines.bun mismatch in root package.json:

   "packageManager": "bun@1.3.13",
   "engines": { "bun": ">=1.3.14" }

   The pinned package manager fails its own engines constraint. Bump packageManager to bun@1.3.14 (or relax engines).

3. packages/workflows/package.json is still "private": true. Already called out in the PR description as a follow-up, but worth flagging as a release-gate item since this package is the headline of the rename.

Code quality

4. No tripwire for the documented diamond-DAG bug. GraphFrontierTracker.onSettle() is acknowledged to drop parent edges in A→B→D, A→C→D (D ends up with [C] not [B, C]), but test/unit/graph-frontier-tracker.test.ts and test/unit/graph-inference.test.ts have no test.todo / test.skip pinning this. Adding a skipped test that asserts the desired behavior would prevent silent regressions and act as an executable spec for whoever picks up the fix. (See packages/workflows/src/runs/shared/graph-inference.ts:43.)

5. noUncheckedIndexedAccess dropped from tsconfig.json. Given how much new indexing-heavy code lands (executor.ts, worktree.ts, frontier tracker), restoring this flag would catch a real class of bugs. CLAUDE.md also says "avoid any/unknown" — noUncheckedIndexedAccess is the natural complement.

6. Mega-files flagged for follow-up are large enough that splitting before merge would help review and git blame:

   • packages/workflows/src/extension/index.ts — 2,143 lines, mixes ExtensionAPI types, command handlers, tool registration, lifecycle, and HIL plumbing.
   • packages/workflows/src/runs/foreground/executor.ts — 1,505 lines.

7. Minor formatting nit at packages/workflows/src/extension/index.ts:69 — two import statements joined on the same line: } from "../shared/types.js";import { buildRuntimeAdapters } from "./wiring.js";. Lint/format would catch it; worth tightening since the file is heavily-imported.

Performance

8. Store.snapshot() deep-clones via JSON.parse(JSON.stringify(...)) on every read (packages/workflows/src/shared/store.ts:170). notify() calls it once, and every TUI consumer (graph-view, stage-chat-view, workflow-attach-pane, store-widget-installer, …) calls it again per re-render. With long-running multi-stage runs this becomes the hot path. Two cheap wins:

   • Replace with structuredClone() — native in Bun/Node, considerably faster than JSON round-trips and handles undefined/Date correctly.
   • Cache the snapshot per-version (the store already increments _version) so multiple subscribers in the same tick share one clone.

   Same pattern repeats in runs/background/status.ts at lines 230, 231, 286, 305, 337.

Correctness / sharp edges

9. Singleton cancellationRegistry (packages/workflows/src/runs/background/cancellation-registry.ts:113) has no clear() and no test-time reset hook. Unit tests that import the singleton instead of calling createCancellationRegistry() will leak AbortControllers across cases. Worth either exporting a reset() method or adding a comment pointing tests at the factory.

10. Windows browser open in packages/mcp/utils.ts:14 shells out via cmd /c start "" <browser> <target>. Even with proper arg quoting from pi.exec, cmd.exe's start builtin re-parses metacharacters (&, ^, |) in its arguments. URLs containing those characters can misbehave. Not new to this PR if the code is lifted from elsewhere, but worth confirming.

Test coverage

• Strong unit coverage across executor, store, frontier tracker, persistence, MCP scoping, inline forms, slash dispatch.
• Integration suite is thin relative to the surface area (7 files). Specifically missing: end-to-end coverage of kill / pause / resume cascading across nested runs, and worktree setup-hook timeout / cleanup behavior.
• Examples (examples/parallel-fan-out.ts) reach into packages/workflows/src/shared/types.js directly rather than through @bastani/workflows — fine for the in-repo example, but a published consumer would have to expose those types from the public index.ts. Worth confirming the public type surface.

Nice things

• defineWorkflow builder's immutable chain with WorkflowBuilder / CompletedWorkflowBuilder split (.compile() only available after .run()) is a tidy use of the type system.
• worktree.ts uses spawnSync(..., { shell: false }) everywhere — no injection surface.
• Atomic temp-file-then-rename status writer with deduped error notices avoids both torn reads and notification loops.
• validateInputs deliberately refuses to coerce — surfacing user mistakes early is the right call.

Happy to dig deeper on any of these.