feat(workflows): add first-class workflow composition via imports - #1135
Conversation
Implementation NotesTask: Implement GitHub issue #1071 in this repository. Work in the dedicated git worktree provided via git_worktree_dir. First inspect the issue details (use gh/GitHub tooling as needed), then make the necessary code/test/doc changes following the repository AGENTS.md guidance (Bun commands, no npm/yarn/pnpm). Run relevant Bun tests/typechecks where practical and leave a concise implementation summary with modified files and verification results. Do not create a release or publish anything. Running Notes
Iteration 2 Notes
Iteration 3 Notes
|
Code Review — feat(workflows): add workflow importsNice piece of work — clean API surface ( Correctness / potential bugs
API / design observations
Test coverage gapsThe new
Smaller nits
SecurityNothing new — path imports execute arbitrary local TS through jiti, which is the same trust boundary discovery already has. Worth flagging in the README that local path imports run the imported file's top-level code at validation time (not just at run time), so a missing or pathological local module surfaces during discovery rather than at first dispatch. Overall: ship-ready after addressing (1), (3), and the (4) duplicate-validation question. The rest is polish that can land follow-up. |
|
Review WIP — see follow-up comment with details. |
Code Review — PR 1135 (workflow imports)Solid, well-tested feature. The design (boundary stage + replay-safe child snapshot) is clean and the diagnostics surface from (Apologies for the WIP placeholder above — replaced here.) Correctness1. Required-output enforcement is asymmetric ( 2. 3. 4. Replay snapshot is shallow-cloned on restore ( Performance / hot-path concerns5. Workflow modules are re-executed on every resolve ( 6. Top-of- API / ergonomics7. Continuation does not partially resume children — by design, a replayed boundary stage skips re-running the child entirely. That is the right default, but worth mentioning in 8. Security note9. Path-import code execution is documented, but worth defense-in-depth: TestsCoverage is good. Suggestions for gaps:
Style10. Overall: the boundary-stage + replay-snapshot approach is the right shape, the diagnostics are well-typed, and tests cover the core paths. The items above are mostly polish — items 2, 3, 4, and 6 are the ones I would want addressed before merge. |
Code review — workflow importsReviewed against the project conventions in Bugs / behaviour concerns
Style / conventions
Security note
Test coverageStrong, with one gap:
Nits
Nothing here is blocking. Items (1), (3), and (6) are the ones I'd most like resolved (or explicitly documented) before merge. |
Code Review — Workflow Imports (#1071)Nice piece of work — the public API ( A few items worth a second look before merge: 🐛 Bugs / correctness
🔐 Security
⚡ Performance
🧪 Tests / coverageThe new unit suites are good. A few gaps that would strengthen coverage:
🧹 Minor / nits
✅ Things I particularly liked
Overall this looks close to mergeable; the most important items above are #1 (misleading diagnostic), #2 (non-exhaustive switch), #3 (callback inheritance contract), and #6 (security framing of path imports). |
claude
Bot
changed the title
Review: feat(workflows): add workflow importsThanks for this — the API split ( Bugs / correctness1.
2.
3. Error throw new Error(`pi-workflows: workflow import "${alias}" ...`, {
cause: { ...(failedChildStage?.failureKind ...), ...(failedChildStage?.failureMessage ...) },
});
4.
Performance5.
6. Import-graph validation runs three times for a single dispatched run
For a typical Style / cleanup7. 8. SecurityNo new attack surface beyond what discovery already accepts: workflow path imports are loaded with jiti and can reference any file the process can read, same as the existing user-global / project-local sources. Worth noting in the README that Test coverageCovers the high-value cases well. Two gaps worth adding:
Overall this is a substantial, well-structured addition. The boundary-stage replay design (persisting 🤖 Generated with Claude Code |
Code Review — PR #1135 (workflow imports)Reviewed against Higher-priority
Path-import / security
API / behavior
Minor
TestingCoverage is strong: child input default propagation, explicit/implicit output selection, required-output enforcement, input validation before child start, continuation replay (single + concurrent + legacy without metadata), and discovery cycles. Two gaps:
Style / repo conventions
SummarySolid, well-tested feature. Main asks before merging: (1) split the |
Code Review — Workflow Imports (#1071)Reviewed against Strengths
Concerns / suggestions
Minor / nitpicks
CLAUDE.md compliance
Nice work overall — the persistence round-trip story (boundary snapshot, restore via 🤖 Generated with Claude Code |
Code Review — workflow importsOverall the PR is well-structured. The resolver/executor split is clean, the import graph validator is symmetric across discovery / dispatch / A few issues / questions follow. Bugs / correctness
Sharp edges worth documenting / surfacing
Drive-by change
Minor / style
Things that look great
Nice work overall. |
Code review — workflow imports (#1071)Reviewed against Bugs / correctness
Design / API
PR description mismatch
Style nits
What is good
Nothing blocking; the items above are minor and the design is sound. |
|
Tested this with very basic/simple workflow composition, and the parent/child import flow worked. One UX note: when a workflow stage internally has completed, the UI can show an empty completed stage, which may be a bit confusing for the end user because they see an empty stage instead of the actual child workflow that ran / workflow that was imported. This may be more of a user-experience thing than a bug.
Workflows used to repro: Child workflow: import { defineWorkflow } from "@bastani/workflows";
export default defineWorkflow("pr1135-import-child")
.description("PR 1135 child workflow: emits typed outputs for parent import/output projection verification.")
.input("subject", {
type: "text",
required: true,
description: "Subject echoed into the child workflow output.",
})
.input("count", {
type: "number",
default: 2,
description: "Number used to build a deterministic child summary.",
})
.output("summary", {
type: "text",
required: true,
description: "Human-readable child summary.",
})
.output("score", {
type: "number",
required: true,
description: "Deterministic numeric score derived from count.",
})
.output("extra", {
type: "text",
description: "Optional output that can be omitted by parent output projection.",
})
.run(async (ctx) => {
const subject = String(ctx.inputs.subject ?? "");
const count = typeof ctx.inputs.count === "number" ? ctx.inputs.count : 2;
await ctx.stage("child-boundary-marker", { noTools: "all" }).complete("PR1135 child workflow completed deterministically.");
return {
summary: `child:${subject}:${count}`,
score: count * 10,
extra: `extra:${subject}`,
};
})
.compile();Parent workflow: import { defineWorkflow } from "@bastani/workflows";
export default defineWorkflow("pr1135-import-parent")
.description("PR 1135 parent workflow: exercises .import(), .output(), ctx.workflow(), output selection, and rename mapping.")
.input("subject", {
type: "text",
default: "workflow-imports",
description: "Subject forwarded to the child workflow.",
})
.input("count", {
type: "number",
default: 3,
description: "Count forwarded to the child workflow.",
})
.import("childByPath", { path: "./pr1135-import-child.ts" })
.import("childByName", { workflow: "pr1135-import-child" })
.output("pathSummary", {
type: "text",
required: true,
description: "Summary selected from the path-imported child workflow.",
})
.output("namedScore", {
type: "number",
required: true,
description: "Score renamed from the name-imported child workflow.",
})
.run(async (ctx) => {
const subject = String(ctx.inputs.subject ?? "workflow-imports");
const count = typeof ctx.inputs.count === "number" ? ctx.inputs.count : 3;
const pathChild = await ctx.workflow("childByPath", {
stageName: "run-child-by-path-select-summary",
inputs: { subject: `${subject}:path`, count },
outputs: ["summary"],
});
const namedChild = await ctx.workflow("childByName", {
stageName: "run-child-by-name-rename-score",
inputs: { subject: `${subject}:name`, count: count + 1 },
outputs: { score: "namedScore" },
});
return {
verification: "PR1135_IMPORTS_SUCCESS",
pathSummary: String(pathChild.outputs.summary),
namedScore: Number(namedChild.outputs.namedScore),
childRunIds: [pathChild.runId, namedChild.runId],
boundaryStages: ["run-child-by-path-select-summary", "run-child-by-name-rename-score"],
replayExpectation: "Re-running from restored persistence should replay completed child boundary stages instead of executing children again.",
};
})
.compile(); |
|
Code review for PR 1135 — workflow imports Reviewed the workflow imports feature. Overall this is a thorough, well-tested addition: 24 test cases, clean module decomposition (import-resolver.ts, workflow-module-loader.ts), and the boundary-stage-as-replay-checkpoint design fits the existing replay machinery cleanly. CONCERNS
SMALLER THINGS
WHAT IS WORKING WELL
TEST COVERAGE Excellent. Specifically the following cases all have explicit coverage: input validation before child spawn, missing required outputs, undeclared output rejection, type mismatch, parent-key collisions, continuation replay with deep-clone verification, replay of repeated boundaries, legacy snapshot fallback, non-serializable selected output (fails) vs non-serializable raw output (succeeds with rawOutput:undefined). VERDICT Approve with minor follow-ups — primarily the duplicated graph validation cost on path imports (concern 1) and the unannounced inputBindings freeze change (3). Neither blocks merge. |
Review — feat(workflows): first-class workflow composition via importsThorough, well-tested change. The API shape ( A few items worth a look before merging. Correctness / behavior
Quality / API ergonomics
Minor / nits
Test coverageStrong. Suggested additions:
Nothing here is blocking — the core machinery looks correct and the replay/serialization design is careful. The |
# Conflicts: # packages/workflows/CHANGELOG.md # packages/workflows/src/runs/foreground/executor.ts # test/unit/node-card.test.ts
Review: Workflow imports (
|
) * feat(workflows): add workflow imports * fix(workflows): address import review feedback * fix(workflows): tighten child import semantics * docs(workflows): clarify import replay semantics * fix(workflows): disambiguate workflow import replay * fix(subagents): shorten optional npm discovery * fix(subagents): lazy-load optional intercom package root * fix(workflows): handle uncloneable child raw outputs * fix(workflows): harden child output replay * fix(workflows): avoid wc in deep research line count * fix(workflows): clarify imported boundary nodes
2 participants
Summary
Introduces first-class workflow composition to
@bastani/workflows: workflows declare typed import dependencies with.import(), expose output contracts with.output(), and invoke child workflows at runtime viactx.workflow(). The import graph is validated before any run starts, and completed child runs are persisted in a replay-safe boundary stage so continuation replays skip already-finished child executions.Closes #1071
Key Changes
API surface (
define-workflow.ts,types.ts)defineWorkflow().import(alias, source, options?)— declares a child workflow dependency by registered name ({ workflow: "id" }) or local file path ({ path: "./file.ts", export?: "namedExport" })defineWorkflow().output(key, schema?)— declares a typed output contract (type,description,required) consumed by parent workflows when selecting or mapping child outputsctx.workflow(alias, options)— executes a declared import as a nested workflow run; acceptsinputs,outputs(select list or rename map), and an optionalstageNamefor the parent boundary stageWorkflowImportSource,WorkflowImportDeclaration,WorkflowOutputSchema,WorkflowRunChildOptions,WorkflowChildResultImport resolver (
src/workflows/import-resolver.ts) — new fileresolveWorkflowImport— resolves a single alias against the registry or file systemvalidateWorkflowImportGraph— depth-first traversal that detectsIMPORT_UNRESOLVED,IMPORT_CIRCULAR, andIMPORT_INVALIDdiagnostics across all registered roots, deduplicating by content keyformatWorkflowImportDiagnostics— human-readable diagnostic formatterShared module loader (
src/extension/workflow-module-loader.ts) — new filediscovery.tsinto a shared helper used by both discovery and the import resolver, ensuring consistent TypeScript/ESM/CJS semantics and@bastani/workflowsvirtual SDK alias across all load pathsDiscovery (
src/extension/discovery.ts)workflow-module-loader.tsinstead of an inline loadervalidateWorkflowImportGraphafter all workflows register and emits import diagnostics as structuredIMPORT_UNRESOLVED,IMPORT_CIRCULAR, andIMPORT_INVALIDerror codesExecutor (
src/runs/foreground/executor.ts)ctx.workflow()runs the child via the existingrunWorkflowinfrastructure, records the parent boundary stage through the normal stage start/end lifecycle, writes aworkflowChildpayload to that boundary stage, and returns a typedWorkflowChildResultctx.workflow()as a valid stage creatorPersistence & replay (
store-types.ts,persistence-restore.ts,persistence-session-entries.ts)WorkflowChildReplaySnapshotcarries child results across session restoresworkflowChildMetadataextractor validates and clones child payloads during restoreWorkflowChildResultwithout re-running the childRuntime & dispatcher (
runtime.ts,dispatcher.ts)ExtensionRuntimeOptsgainsworkflowSourcesandcwd, forwarded throughrunOptions()anddispatch()for relative-path import resolutiondispatch()validates the import graph before starting a run, short-circuiting with a structuredIMPORT_*error on failureTUI (
src/tui/node-card.ts)Bug fixes
builtin/deep-research-codebase.ts): replaced POSIXwcsubprocess with in-processcountNewlineBytesto fix failures on non-POSIX (e.g. Windows) hostspackages/subagents): defer global npm root discovery until actually needed, avoiding unnecessary startup overheadTests
test/unit/workflow-imports.test.tstest/unit/executor.test.tsctx.workflow()execution, output selection/mapping, input validation, type checking, replaytest/unit/persistence-restore.test.tstest/unit/persistence-session-entries.test.tstest/unit/node-card.test.tsdefine-workflow.test.ts,workflow-runner.test.ts,builtin-workflows.test.ts.import()/.output()API additionsTest Plan
bun run test:unitpasses with all new and extended tests greenbun run typecheckreports no errorsIMPORT_UNRESOLVEDandIMPORT_INVALIDdiagnostics are emitted correctly for bad imports🤖 Generated with Claude Code