Skip to content

feat(workflows)!: render ctx.ui.* prompts as synthetic graph stage nodes - #1054

Merged
lavaman131 merged 9 commits into
mainfrom
issue/1046-awaiting-input-node-state
May 26, 2026
Merged

feat(workflows)!: render ctx.ui.* prompts as synthetic graph stage nodes#1054
lavaman131 merged 9 commits into
mainfrom
issue/1046-awaiting-input-node-state

Conversation

@flora131

@flora131 flora131 commented May 25, 2026

Copy link
Copy Markdown
Collaborator

Summary

Converts node-local `ctx.ui.input/confirm/select/editor` prompts from run-level overlay banners into first-class synthetic DAG stages with `awaiting_input` status. Prompt answers are submitted from the owning stage's chat panel or via `/workflow send`, and failed-run continuation replay can reuse live prompt answers without ever persisting raw response data.

Closes #1046

Key Changes

Prompt graph nodes

  • `ctx.ui.*` calls create a synthetic stage in the DAG with `awaiting_input` status when `usePromptNodesForUi: true` on `RunOpts` (used in detached-run mode)
  • Each synthetic stage is assigned a UUID and integrated into the frontier via `GraphFrontierTracker`; stage lifecycle follows `running → completed | failed | skipped`
  • Unstarted placeholder stages are filtered from the graph view while a prompt node is active (via `_graphStages` in `GraphView`)
  • `ctx.ui.select([], ...)` now throws before node creation (previously silent)

Live-only answer ledger

  • `Store` gains `getStagePromptAnswer` / `clearStagePromptAnswer` APIs backed by an in-memory-only `_stagePromptAnswers` map
  • `PromptAnswerRecord` stores `{ runId, stageId, promptId, kind, value, answeredAt }` and is intentionally never serialized, logged, or copied into snapshots
  • New optional `recordAnswer` flag on `ResolveStagePendingPromptOptions` controls ledger retention; abort/default resolutions pass `false`

Stable continuation replay

  • New `prompt-callsite.ts` module: normalizes the call stack, filters workflow-runtime frames, and extracts the author callsite
  • Each prompt stage carries a `replayKey` of the form `prompt:::` using SHA-256 (first 32 chars of hex)
  • `ContinuationReplayIndex.decide` accepts a typed `ContinuationReplayInput` with explicit `replayKey` and `kind` fields; legacy snapshots without `replayKey` fall back to display-name matching
  • `PromptAnswerReplaySafety` enum (`"allowed" | "unavailable" | "ambiguous"`) and `promptAnswerState` field on `StageSnapshot` track replay availability without exposing the answer value

Stage chat & prompt card

  • `StageChatView` extended to display, answer, skip, and detach node-local prompts
  • Prompt card gains keybinding-aware word/line navigation for `input` and `editor` prompt types via `keybindings-adapter`
  • Completed stage chat handles are retained for follow-up interactions
  • `callId` field on `AskUserQuestionToolEvent` made optional to harden edge-case handling

Breaking Changes

Change Migration
`StageSnapshot.parentIds` is now mutable (was `readonly`) Treat as immutable from consumer code; executor may replace the frozen array during late topology inference — do not cache this reference across store updates
New optional fields `replayKey` and `promptAnswerState` on `StageSnapshot` Safe to read; `promptAnswerState` never contains the raw answer value
`"awaiting_input"` now emitted by synthetic `ctx.ui.*` prompt stages Previously only emitted by `ask_user_question` tool activity
`resolveStagePendingPrompt` signature gains optional 4th param `options?: ResolveStagePendingPromptOptions` Existing callers are unaffected
`AskUserQuestionToolEvent.callId` changed from `string` to `string undefined`
`ctx.ui.select([], ...)` now throws before node creation Pass a non-empty options array

Validation

```sh
AGENT=1 bun test test/unit/prompt-callsite.test.ts test/unit/executor.test.ts --test-name-pattern "continuation"
AGENT=1 bun test test/unit/builtin-workflows.test.ts
bun run typecheck
bun run lint
bun run test:unit
```

Implement node-local workflow UI prompt stages with awaiting_input state, stage-local prompt cards, continuation replay safeguards, live-only answer replay, and hardened prompt callsite identity for packaged runtime paths.

Add regression coverage for prompt-node lifecycle, continuation topology, prompt answer privacy, TUI routing, persistence restore, and packaged prompt callsite filtering.

AI-Assisted-By: GPT-5.5
@flora131

Copy link
Copy Markdown
Collaborator Author

Implementation Notes

Task: Implement #1046

Running Notes

  • Record implementation decisions, deviations from the spec, tradeoffs, blockers, validation notes, and anything else the user should know.

Iteration 1 implementation (2026-05-25)

  • Delegated discovery, current-flow analysis, pattern finding, implementation, review, and validation to subagents; artifacts were saved under subagent-artifacts/1046-*.md.
  • Implemented normal detached workflow ctx.ui.input/confirm/select/editor prompts as executor-owned synthetic graph stages backed by StageSnapshot.pendingPrompt and awaiting_input, rather than RunSnapshot.pendingPrompt.
  • Decision: synthetic prompt node labels use the prompt kind (input, confirm, select, editor) for iteration 1, matching the spec suggestion and avoiding a new truncation/redaction policy for prompt text.
  • Decision: prompts before any prior stage are root nodes with no parents, using the natural GraphFrontierTracker.onSpawn(...) behavior.
  • Decision/tradeoff: added an internal RunOpts.usePromptNodesForUi switch and enable it from runDetached; direct executor runs with an explicit UI adapter preserve the existing seam and legacy fallback behavior.
  • Legacy run-level RunSnapshot.pendingPrompt and GraphView overlay support remain available as fallback, but the normal detached workflow path no longer records run.pendingPrompt.
  • StageChatView now renders and resolves structured stage.pendingPrompt prompt cards locally while preserving mounted StageUiBroker custom UI precedence.
  • Prompt responses are returned to the workflow promise only and are not stored in StageSnapshot.result, preserving the prior privacy shape.
  • Abort handling resolves the stage prompt with a safe default to clear store state, rejects the workflow HIL awaiter so the run can finalize as killed, and finalizes the synthetic prompt stage as skipped.
  • Documentation was updated in packages/workflows/README.md and packages/coding-agent/docs/workflows.md to describe node-local HIL prompts and the focus/Enter response flow.
  • Validation completed successfully: targeted Bun tests for background-runner-hil, stage-chat-view, overlay-graph, background-ui-adapter, store-pending-prompt, and node-card passed (192 pass, 0 fail); bun run typecheck passed; writer also reported full AGENT=1 bun run test:unit passed (1535 tests).
  • No blockers remain and no issues.md was needed.

Iteration 2 implementation (2026-05-25)

  • Delegated discovery, pattern finding, implementation, and validation to subagents; artifacts were saved under subagent-artifacts/1046-iter2-*.md.
  • Implemented continuation replay mapping for executor-owned synthetic prompt nodes by making prompt-node creation participate in ContinuationReplayIndex.decide(...) after GraphFrontierTracker.onSpawn(...); downstream stages can now translate prompt-node parent IDs during continuation.
  • Decision/tradeoff: prompt responses remain intentionally absent from StageSnapshot.result; replayed prompt nodes are recorded structurally as completed/replayed and return the existing safe fallback/default response for the prompt kind. This preserves privacy but may not exactly replay branch semantics for previously answered prompts.
  • Preserved prompt-node graph parentage by settling replayed and executed prompt stages through GraphFrontierTracker.onSettle(...) exactly once.
  • Fixed attached prompt-node keyboard routing so mounted custom UI remains highest precedence, then Ctrl+D detaches/closes before prompt-card handling; detaching does not resolve or cancel the pending prompt.
  • Fixed terminal prompt archive behavior by treating skipped no-live-handle stages as read-only archives in StageChatView.
  • Added regression coverage for continuation through before -> ctx.ui.confirm -> after, Ctrl+D prompt detach/close variants, skipped read-only archives, and killing runs with pending prompt nodes (skippedReason = "run-aborted", prompt cleared, non-attachable).
  • Validation completed successfully: focused executor/background-runner-hil/stage-chat-view tests passed (156 pass, 0 fail); overlay/background-ui/store-pending-prompt tests passed (106 pass, 0 fail); bun run typecheck passed; full bun run test:unit passed (1539 pass, 0 fail).
  • No blockers remain and no issues.md was needed for iteration 2.

Iteration 2 implementation (2026-05-25)

  • Implemented continuation replay mapping for executor-owned synthetic prompt nodes: buildPromptNodeUiContext().ask(...) now calls the same replay index path as normal stages after GraphFrontierTracker.onSpawn(...), records replay metadata for structurally replayed prompt nodes, and settles the graph frontier exactly once.
  • Decision/tradeoff: prompt responses remain private and are still not written to StageSnapshot.result; replayed prompt nodes therefore return the safe kind fallback/default while preserving topology. This can change branch semantics in a resumed run but avoids persisting potentially sensitive HIL answers.
  • Updated StageChatView input precedence so mounted custom UI remains first, then Ctrl+D detach/close runs before prompt-card input. Detaching from a prompt node does not resolve or cancel the pending prompt.
  • Updated read-only archive detection so skipped terminal nodes without live handles render as read-only archive surfaces instead of editable blank composers.
  • Note: the store normalizes attachable: false by omitting the optional attachable property; tests assert non-attachable as not true.
  • Added regression tests for prompt-node replay mapping, prompt parentage after continuation, prompt-node abort finalization, prompt Ctrl+D handling, and skipped read-only rendering.
  • Validation completed successfully: bun test test/unit/executor.test.ts test/unit/background-runner-hil.test.ts test/unit/stage-chat-view.test.ts passed (156 pass, 0 fail); bun test test/unit/overlay-graph.test.ts test/unit/background-ui-adapter.test.ts test/unit/store-pending-prompt.test.ts passed (106 pass, 0 fail); bun run typecheck passed.
  • No blockers encountered.

Iteration 3 implementation (2026-05-25)

  • Delegated discovery, implementation analysis, code/test implementation, and validation to subagents; artifacts were saved under subagent-artifacts/1046-iter3-*.md.
  • Implemented safe continuation replay for executor-owned prompt nodes: completed prompt nodes now replay the original answer when the private in-memory prompt-answer ledger has it, and otherwise re-prompt instead of returning kind defaults.
  • Added private live-only stage prompt answer storage in the workflow store, recorded by resolveStagePendingPrompt(...), looked up by source run/stage during continuation, and cleared on run removal/store clear. Raw answers are not exposed through store snapshots or persistence.
  • Added snapshot-safe stage metadata for continuation: replayKey and optional promptAnswerState; replayKey is persisted in stage start/end entries and restored, while prompt answers remain unpersisted.
  • Added prompt replay identity separate from display name. Decision/tradeoff: callsite hashing was deferred because stack traces are runtime/bundling-sensitive; iteration 3 uses descriptor hash plus deterministic ordinal to disambiguate common parallel prompts while preserving explicit ambiguity for truly indistinguishable cases.
  • Updated continuation replay matching to use stage.replayKey ?? stage.name with legacy fallback for older source stages without replay keys. Ambiguity errors mention display name/replay key but not answer values.
  • Added/updated regression coverage for preserving a prior confirm(true) answer, re-prompting when an answer ledger entry is unavailable, parallel prompt replayKey disambiguation, prompt-answer privacy in snapshots, and replayKey persistence/restore without answer persistence.
  • Validation completed successfully: focused bun test test/unit/background-runner-hil.test.ts test/unit/executor.test.ts test/unit/overlay-graph.test.ts test/unit/stage-chat-view.test.ts test/unit/store-pending-prompt.test.ts test/unit/persistence-session-entries.test.ts test/unit/persistence-restore.test.ts passed (302 pass, 0 fail); bun run typecheck passed.
  • No blockers encountered and no issues.md was needed.

Iteration 4 implementation (2026-05-25)

  • Delegated current-flow analysis, pattern discovery, implementation, and validation to subagents.
  • Implemented continuation replay topology correction for prompt nodes: replay-matched prompt stages now install translated source parentIds before store recording, persistence stage-start entries, and frontier settlement.
  • Added a narrow GraphFrontierTracker.replaceParents(...) seam so the executor can replace provisional live-frontier parents with source-run-authoritative parents without redesigning frontier inference.
  • Replaced scheduling-sensitive prompt replay ordinals with stable prompt replay keys based on descriptor hash plus a normalized callsite hash. Only hashes are persisted; raw stack frames and absolute stack text are not persisted.
  • Split continuation prompt matching from answer replay eligibility. When duplicate prompts remain indistinguishable by stable identity/topology, continuation re-prompts with promptAnswerState: "ambiguous" instead of replaying a potentially wrong private answer.
  • Extended StageSnapshot.promptAnswerState to include ambiguous; raw prompt answers remain live-only in the store ledger and are not added to snapshots, stage results, or persistence entries.
  • Added regression tests for concurrent prompt topology preservation before replay settlement and ambiguous same-callsite duplicate prompts re-prompting.
  • Validation completed successfully with Bun-only commands: AGENT=1 bun test test/unit/executor.test.ts --test-name-pattern "continuation" passed (10 passed, 82 filtered); AGENT=1 bun test test/unit/background-runner-hil.test.ts test/unit/stage-chat-view.test.ts test/unit/store-pending-prompt.test.ts passed (88 passed); bun run typecheck passed.
  • No blockers encountered and no issues.md was needed.

Iteration 5 implementation (2026-05-25)

  • Delegated discovery, pattern analysis, implementation, and validation to subagents; artifacts were saved under subagent-artifacts/1046-iter5-*.md.
  • Implemented strict-by-default continuation replay topology validation: normal ctx.stage(...) replay now rejects translated parent mismatches even when there is only one mapped source candidate.
  • Preserved a narrow prompt-node settlement-drift exception only for synthetic prompt nodes, gated by prompt parents whose answers were actually replayed in the continuation. This avoids extending parent replacement to ordinary stages or unavailable/ambiguous prompt answers.
  • Restored Ralph's project_initialization_preflight orchestrator prompt block and the instruction to perform/delegate setup preflight before decomposing implementation work.
  • Restored Ralph unit coverage for generic project initialization discovery and added regression coverage for single-candidate topology drift: A -> B becoming A -> X -> B, and parallel roots a,b becoming a -> b.
  • Prompt answer privacy remains unchanged: no raw answers were added to snapshots, persistence entries, stage results, or validation output.
  • Validation completed successfully with Bun-only commands: AGENT=1 bun test test/unit/executor.test.ts --test-name-pattern "continuation" passed (12 pass, 82 filtered); AGENT=1 bun test test/unit/builtin-workflows.test.ts passed (28 pass); AGENT=1 bun test test/unit/background-runner-hil.test.ts test/unit/stage-chat-view.test.ts test/unit/overlay-graph.test.ts passed (144 pass); bun run typecheck passed.
  • No blockers encountered and no issues.md was needed for iteration 5.

Iteration 6 implementation (2026-05-25)

  • Delegated context discovery, behavior analysis, pattern finding, implementation, and fresh-context review/validation to subagents; artifacts were saved under subagent-artifacts/1046-iter6-*.md.
  • Implemented a narrow prompt callsite helper seam in packages/workflows/src/runs/shared/prompt-callsite.ts and updated packages/workflows/src/runs/foreground/executor.ts so promptCallsiteHash() hashes the first normalized non-runtime author frame while preserving the existing prompt:<kind>:<descriptorHash>:<callsiteHash> replay key format.
  • Runtime-frame filtering now normalizes before classification, including file:// decoding, Windows backslash normalization, local relative path normalization, and narrow filtering of packages/workflows/src/, dist/builtin/workflows/src/, and node_modules/@bastani/workflows/src/ implementation roots.
  • Decision/tradeoff: used an internal shared helper module with focused unit coverage instead of exporting private executor functions or adding a public API. The classifier intentionally preserves .atomic/workflows/*, test/unit/*, packages/workflows/builtin/*, and dist/builtin/workflows/builtin/* as workflow-author callsites.
  • Added test/unit/prompt-callsite.test.ts covering source, packaged, node_modules, Windows, and file-URL runtime filtering, author-frame preservation, and packaged-runtime stack selection with distinct author callsites.
  • Deviation/tradeoff: no full packaged workflow execution repro was added because prompt replay key internals remain private and live test stacks come from source paths; the helper test simulates packaged executor frames followed by author frames, and existing executor continuation tests cover live replay behavior.
  • Packaged Atomic raw TypeScript copy was regenerated/validated by the writer with bun run --cwd packages/coding-agent copy-builtin-packages and bun run --cwd packages/coding-agent build; generated dist files are ignored in this checkout.
  • Validation completed successfully: bun test test/unit/prompt-callsite.test.ts passed; AGENT=1 bun test test/unit/executor.test.ts --test-name-pattern "continuation" passed; AGENT=1 bun test test/unit/background-runner-hil.test.ts test/unit/stage-chat-view.test.ts test/unit/store-pending-prompt.test.ts passed; bun run typecheck passed; review subagents also reran prompt-callsite/continuation/typecheck checks successfully.
  • Review outcome: fresh-context analyzer, pattern-finder, and inspect-only debugger found no blockers or fixes worth doing now. One optional caveat remains that paths containing literal spaces are not explicitly covered by stack parser tests, but this was outside the iteration 6 spec examples.
  • Prompt answer privacy and raw stack-frame privacy remain unchanged: raw answers stay live-only, and raw stack frames are only transiently normalized before hashing.

@mintlify

mintlify Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
bastani 🟢 Ready View Preview May 25, 2026, 8:29 PM

@claude

claude Bot commented May 25, 2026

Copy link
Copy Markdown

Code Review — feat(workflows): render ui prompts as graph nodes

Thanks for this — the move from run-level overlays to node-local prompt stages is a substantive UX/state-model improvement and the test coverage (replay topology, ambiguity, parallel/concurrent prompts, privacy, callsite filtering) is impressively thorough. Below are the things worth a second look before merging; most are nits, a few are worth a comment in code, and one is a real correctness concern about the replayedFromStageId lookup.

Correctness

1. getStagePromptAnswer keyed off opts.continuation?.source.id ?? \"\" is brittle (executor.ts)

const replayAnswer = replayDecision.kind === \"replay\"
  ? activeStore.getStagePromptAnswer(opts.continuation?.source.id ?? \"\", replayDecision.source.id)
  : undefined;

Reaching kind: \"replay\" requires a continuation to exist (the no-continuation branch of createContinuationReplayIndex only returns kind: \"execute\"). So the ?? \"\" fallback can never actually fire — but it makes the invariant invisible at the callsite. Suggest an explicit assertion or destructuring const sourceRunId = opts.continuation!.source.id to encode the invariant. As-is, a future refactor that returns replay from the no-continuation branch would silently swallow a bug as "answer unavailable."

2. select returns options[0] even when options is empty (executor.ts)

async select<T extends string>(message: string, options: readonly T[]): Promise<T> {
  
  return options[0]; // T | undefined cast through return type
}

With readonly T[] the type system allows []. The current fallback would return undefined typed as T. Either narrow the signature to readonly [T, ...T[]] or throw when options.length === 0. Low likelihood of being hit, but the typing lies today.

3. selectPromptCallsiteFrame blindly strips the first stack line

return stack.split(\"\\n\").slice(1).map()

This assumes the V8 "Error" header line is always present. For an Error produced by new Error() in Bun/Node this is fine, but if anyone ever swaps in a custom Error.captureStackTrace-less subclass, the first real frame would be lost. Minor; worth a defensive comment noting the assumption since this drives replay identity.

4. isWorkflowRuntimeFrame heuristic is path-substring (prompt-callsite.ts)

A user repo containing …/packages/workflows/src/… (say, a fork that vendors workflows under a different name structure) would have their authored workflow filtered out and the callsite would degrade to \"unknown\". That would collapse all prompts to the same callsite hash → false ambiguous markers on replay. Probably fine in practice, but consider documenting the heuristic's bounds and/or also requiring the path to not be under process.cwd() plus the project's .atomic/ or .agents/workflows/ dirs.

State / lifecycle

5. Render mutating promptState (stage-chat-view.ts)

_syncPromptState is now called inside render(...):

this._syncPromptState(stage?.pendingPrompt);
const promptActive = !customUiActive && this.promptState !== null;

Render mutating component state is a smell — works because the store subscription also calls it, but a stray render before the subscription fires (or during a torn-down state) could resurrect a card briefly. Either compute promptActive directly from stage?.pendingPrompt without mutating, or restrict mutation to the subscription/invalidate path.

6. replayablePromptContinuationStageIds is monotonic for the run's lifetime

Acceptable — it's per-run state — but worth a code comment explaining why entries are never removed (a later prompt may still need the drift exemption against an earlier replayed-prompt parent). Without that comment, the absence of cleanup looks like an oversight.

7. hasOnlyReplayablePromptParentDrift deserves a longer comment

The interaction between provisional parent inference, source-run translation, and "only-replayed-prompt drift is OK" is subtle. Right now the function reads as a clever predicate. Suggest a short example in the JSDoc — e.g. [P1, A] → [P1', A] where P1 is a replayed prompt whose continuation id (P1') is different — to anchor the reader.

8. Abort path stores the fallback in the answer ledger

When onAbort fires it calls resolveStagePendingPrompt(…, fallbackForPromptDescriptor(descriptor)), which writes the fallback value into _stagePromptAnswers. The stage is then marked skipped, and your decide correctly avoids returning replay for non-completed source stages, so the fallback is never replayed today. Worth a one-line comment in onAbort so a future refactor that loosens the replay condition doesn't accidentally treat the abort fallback as a real user answer.

Privacy

9. Snapshot privacy test is good — consider hardening it

store-pending-prompt.test.ts checks the JSON-stringified snapshot doesn't contain \"super-secret-value\". Consider also asserting it doesn't appear in appendStageStart/appendStageEnd persistence payloads (or anywhere _stagePromptAnswers keys leak). Today the snapshot path is clean; an additional assertion on the persistence API mock would prevent a regression where someone wires the answer into the session entry by mistake.

Style / minor

10. crypto.randomUUID() for prompt IDs

Fine in modern Bun/Node, but consistent with the rest of the file (which uses crypto.randomUUID() for stage IDs). Just flagging — no change requested.

11. stableHash is 32-bit FNV-1a

Collision risk for both descriptor hash AND callsite hash simultaneously colliding is negligible, but the function name implies determinism rather than collision safety. Rename to fnv1a32 or add a comment that it's used only for identity, not cryptographic uniqueness.

12. opts.usePromptNodesForUi === true ? buildPromptNodeUiAdapter() : opts.ui ?? makeUnavailableUIContext()

Reads cleanly but it's two ternaries deep at a contract boundary. A 1–2-line comment explaining the precedence (prompt-node mode overrides any caller-supplied ui) would help a future reader understand why the explicit-true check is intentional.

13. replayKey: \stage:${name}`` for ordinary stages

Stage replay keys aren't unique (parallel siblings share stage:foo), and the index falls back to topology disambiguation — that's the right design. A code comment near the construction noting "replayKey is intentionally non-unique for stages; topology disambiguates" would prevent confusion vs. the prompt replayKey, which is meant to be unique per callsite.

Tests

Coverage is strong. A few additional cases worth considering:

  • Process restart between source and continuation: the answer ledger is in-memory only; verify that a continuation across clear() correctly re-prompts (you've covered explicit clearStagePromptAnswer, but not the broader "store reset" scenario which is the realistic restart case).
  • Custom UI mounted while a prompt is pending: stage-chat-view.test.ts covers custom UI winning, but not the transition (custom UI mounted → unmounted → prompt re-appears). Worth one test for the unmount path.
  • recordStagePendingPrompt returning false: the executor has a defensive if (!accepted) finalizePromptStage(\"skipped\") path with skippedReason: \"prompt-unavailable\" — no test exercises it. Easy to add by injecting a store stub.

Summary

This is a solid, well-tested change. The main thing I'd want to see before merging is item #1 (eliminate the ?? \"\" source-id fallback so the invariant is explicit) and items #5/#7 (render purity + a JSDoc example for the drift predicate). The rest is polish.

Nice work on the privacy split between live answer ledger and persisted snapshot — that's the kind of detail that makes resume-after-failure usable without leaking secrets.

🤖 Generated with Claude Code

Render stage-local text prompts through the host editor, support scrollable prompt bodies, and hide unstarted placeholder stages while prompt nodes await responses.

Keep completed stage chat handles resumable until the host clears the registry, and let ctx.complete fall back to SDK sessions when no complete-adapter-specific options are used.

Assistant-model: GPT-5.5
@claude claude Bot changed the title feat(workflows): render ui prompts as graph nodes feat(workflows): promote ctx.ui prompts to node-local graph stages May 25, 2026
@claude

claude Bot commented May 25, 2026

Copy link
Copy Markdown

Review — feat(workflows): render ui prompts as graph nodes

Solid, well-tested feature. Replaying HIL prompts as first-class graph nodes (with a stage-local pendingPrompt rather than a run-level overlay) is a clean architectural win, and the private answer ledger keeps secrets out of snapshots/persistence. Below are concerns worth a second look before merge.

Bugs / correctness

  1. tracker.onSpawn called twice for the same stageIdpackages/workflows/src/runs/foreground/executor.ts:2158-2163:

    if (opts.continuation === undefined && stageSnapshot.startedAt === undefined) {
      const actualParentIds = tracker.onSpawn(stageId, name);
      if (!sameStringSet(actualParentIds, stageSnapshot.parentIds)) {
        setStageParentIds(stageSnapshot, actualParentIds);
      }
    }

    GraphFrontierTracker.onSpawn mutates stageParents/nodes and is documented as "Call when ctx.stage(name) is invoked." Calling it a second time for an already-registered stage overwrites both maps. It works today because the maps allow overwrite, but the prompt-node path correctly uses the new tracker.replaceParents(). Use replaceParents here too for consistency and to honor the documented onSpawn contract.

  2. setStageParentIds mutates a frozen array via a cast — same file:

    const setStageParentIds = (stage: StageSnapshot, parentIds: readonly string[]): void => {
      (stage as { parentIds: readonly string[] }).parentIds = Object.freeze([...parentIds]);
    };

    This writes through a readonly cast. The stage object itself isn't frozen so it's currently legal, but the cast is the kind of thing that bites later when someone tightens the type or freezes the snapshot. Consider making parentIds legitimately mutable internally, or surfacing the mutation through recordStageStart-style store APIs.

  3. _disposePromptEditor doesn't actually dispose the editorpackages/workflows/src/tui/stage-chat-view.ts:

    private _disposePromptEditor(): void {
      this.promptEditor = null;
      this.promptEditorPromptId = null;
    }

    Just nulls references. If the pi-tui Editor holds timers / listeners, they leak. Worth checking the upstream Editor lifecycle and calling editor.dispose?.() if it exists.

  4. complete() fallback silently changes contractstage-runner.ts:594-612:
    Previously ctx.complete(...) without adapters.complete threw complete adapter not configured. After this change, it falls back to promptWithFallback(...) whenever no model/maxTokens/fallbackModels is passed. The new error wording "prompt adapter not configured" is also misleading when the user called complete(). This is a behavior change that isn't called out in the PR summary — please confirm it's intentional and matches issue Show ctx.ui.input as awaiting input node state instead of graph overlay #1046, and update the error message to mention complete (e.g. "pi-workflows: ctx.complete requires either an adapters.complete or adapters.agentSession").

  5. Editor pageUp/pageDown swallowed by body scroller_handlePromptScrollInput runs before _handlePromptInput, so pageUp/pageDown always scroll the prompt body even when the user is typing in the pi-tui editor. Likely surprising in long-form editor prompts. Consider letting the editor see those keys first when this.promptEditor is active.

Smaller things

  • selectPromptCallsiteFrame uses new Error().stack on every prompt invocation (promptCallsiteHash()). Fine for HIL (interactive), but worth a comment that this is intentionally only on the slow path.
  • promptReplayKey collisions when two distinct prompts in the workflow share kind + message + callsite are flagged as ambiguous and re-asked. Good. But please add a doc line in runs/shared/prompt-callsite.ts explaining the contract — a future reader will wonder why the callsite is part of the replay key at all.
  • Dead code questionbackground-ui-adapter.ts now claims it's "for tests and non-executor fallback callers", but runner.ts dropped the import. Grep shows no production callers. If only tests use it, mark @internal and add a TODO to delete once tests migrate, or delete it now and update tests. Keeping a "legacy" adapter that nothing actually uses is an attractive nuisance.
  • Comment drift in releaseLiveHandleWhenIdle — the old polling/streaming watcher is gone (good), but the function name still says "WhenIdle" though it now fires immediately. Consider renaming to dropStageControlForCompletion or similar to match the new behavior.

Security

  • Prompt answers live in _stagePromptAnswers as raw values (passwords, tokens, etc. if a workflow asks for them). The PR correctly keeps them out of snapshot() / persistence (verified by store-pending-prompt.test.ts). Worth a short doc comment on getStagePromptAnswer warning callers that the returned value is sensitive and must not be logged or persisted.

Tests

Coverage is strong — replay-key disambiguation, ambiguous same-callsite re-prompting, parallel root preservation, topology rejection, packaged/Windows/file-URL callsite filtering, TUI scrolling, Ctrl-D detach, skipped-stage read-only render. Two suggestions:

  • Add an explicit test for removeRun purging the prompt-answer ledger for a run with multiple stage answers (the current test covers a single stage).
  • Add a test that abort during prompt resolution doesn't leave a dangling _stagePromptAnswers entry when the resolver was racing the abort (the abort path in buildPromptNodeUiAdapter calls resolveStagePendingPrompt with the fallback value, which writes to the ledger before the stage is marked skipped — verify that's the intended state).

Style / conventions

  • All new code uses bun:test + node:assert/strict
  • No dist/ introduced under packages/workflows
  • .js import extensions used consistently ✅
  • any/unknown avoided ✅ (PromptCardAction, PromptAnswerReplaySafety are nicely typed)

Overall this is a thoughtful refactor with good test coverage. The main thing I'd want addressed before merge is #1 (double onSpawn) and clarification on #4 (the complete() semantics change), since both are subtle behavioral shifts that could surprise users.

@flora131 flora131 changed the title feat(workflows): promote ctx.ui prompts to node-local graph stages feat(workflows): render UI prompts as graph nodes May 25, 2026
@flora131

Copy link
Copy Markdown
Collaborator Author

Addressed the review feedback in bb09c1f:

  • Replaced the second tracker.onSpawn() with currentParents() + replaceParents() and made StageSnapshot.parentIds internally assignable without a readonly cast.
  • Disposed host prompt editors when prompt state resets and routed pageUp/pageDown to the editor while it is active.
  • Kept ctx.complete()'s AgentSession fallback intentional, but changed missing-adapter errors/tests to use a complete-specific configuration hint.
  • Cleared prompt-answer replay state on abort and added coverage for multi-stage removeRun cleanup and aborted prompt nodes.
  • Added comments/docs for callsite hashing, sensitive answer records, the legacy background adapter, and renamed the completion control cleanup helper.

Validation:

  • AGENT=1 bun test test/unit/executor-phase-c.test.ts --test-name-pattern "complete adapters absent|stage snapshot has failed"
  • AGENT=1 bun test test/unit/stage-runner.test.ts test/unit/store-pending-prompt.test.ts test/unit/stage-chat-view.test.ts test/unit/executor.test.ts
  • AGENT=1 bun run test:unit
  • bun run typecheck
  • bun run lint
  • Commit/push hooks: bun run lint, bun run test:unit

@claude claude Bot changed the title feat(workflows): render UI prompts as graph nodes feat(workflows): render ctx.ui.* prompts as synthetic graph stage nodes May 25, 2026
@claude

claude Bot commented May 25, 2026

Copy link
Copy Markdown

Code Review

Solid PR that converts run-level HIL prompts into per-stage synthetic graph nodes. The state model, replay-key design, and live-only answer ledger are well thought out; tests are thorough. Notes below, with code quality first and a few concrete bugs after.

Architecture / code quality

  • The split between snapshot-safe promptAnswerState: "available" | "unavailable" | "ambiguous" and the live-only _stagePromptAnswers ledger is the right model. The store test that asserts JSON.stringify(snapshot()).includes(secret) === false (test/unit/store-pending-prompt.test.ts) makes that contract enforceable.
  • Continuation replay topology in createContinuationReplayIndex (packages/workflows/src/runs/foreground/executor.ts:1226-1351) handles four interlocking concerns (replayKey vs name fallback, parent translation, prompt-parent drift, ambiguous duplicates) and is unusually well-commented for code this dense.
  • stageControlRegistry.clear() now disposes leftover handles (packages/workflows/src/runs/foreground/stage-control-registry.ts:271-279), and factory() calls it on session boundaries (packages/workflows/src/extension/index.ts:3269). This is the cleanup path the per-stage unsubscribeAskUserQuestionWatcher subscription depends on — worth a one-line comment in clear() explaining that.
  • Docs (packages/coding-agent/docs/workflows.md, packages/workflows/README.md) and the legacy background-ui-adapter.ts TODO are updated coherently with the new behaviour.

Potential bugs

  1. Default __ask_user_question__ callId can prematurely clear awaiting_inputaskUserQuestionToolEvent (packages/workflows/src/runs/foreground/executor.ts:286-300) falls back to a constant callId when the event has none. If a real ask_user_question tool_execution_start arrives without a callId, the constant goes into activeAskUserQuestionCalls. Then any subsequent tool_execution_end / tool_result from any tool that also lacks a callId will produce phase: "end", callId: "__ask_user_question__", hit activeAskUserQuestionCalls.has(toolEvent.callId) (line 1955), and incorrectly delete the entry, flipping the stage out of awaiting_input while the question is still pending. Two fixes worth considering: (a) require a real callId on start (drop the fallback), or (b) skip end events whose nameMatched is false unless the callId is a real string. Probably rare in pi's real event stream, but the failure mode is silent and user-visible.

  2. isWorkflowRuntimeFrame substring match can misclassify user pathspackages/workflows/src/runs/shared/prompt-callsite.ts:53-57 uses .includes("/packages/workflows/src/"). A user with ~/code/packages/workflows/src/... (or any clone path that happens to contain that substring) would have their authentic workflow author frame dropped, falling back to "unknown" and collapsing all their prompt callsites to the same hash. Anchoring with a tighter check (e.g. requiring the workflow path to be the first matching segment relative to known package roots, or matching against process.cwd() instead of substring) would harden this. Low-impact in practice but easy to fix.

  3. buildPromptNodeUiAdapter().select can return undefined typed as Tpackages/workflows/src/runs/foreground/executor.ts:1786-1792: when the response is invalid and options is empty, options[0] is undefined but the signature is Promise<T>. Pre-existing pattern in this codebase (the legacy adapter does the same), so not a regression — but worth a runtime guard since empty options would now silently propagate undefined into the stage result instead of throwing.

Minor / nits

  • promptCallsiteHash() allocates an Error per prompt to capture a stack. Comment correctly justifies this for interactive prompts; if you ever extend the call-site mechanism to non-interactive primitives, revisit.
  • _resetPromptEditor in stage-chat-view.ts:461-481 creates a new editor on every prompt-id change. The onChange mutates promptState.rawText = text; promptState.caret = text.length; — caret is always pinned to end-of-text, so any host editor that supports mid-string editing will lose its actual caret position when the workflow re-renders. Currently fine because submission is the only consumer, but worth a comment so future editors don't get bitten.
  • RunOpts.usePromptNodesForUi is now the canonical surface for the executor (background/runner.ts:126), so extension/background-ui-adapter.ts is dead code on the hot path. The TODO is there, just calling out that the file can be deleted once tests migrate — a follow-up cleanup PR would be welcome.

Security

  • Live answers are correctly excluded from snapshot() (JSON.parse(JSON.stringify(...)) round-trip on _runs only; _stagePromptAnswers is a separate Map). removeRun and clear both purge entries.
  • Callsite frames are hashed via FNV-1a before being placed in replayKey. Stack contents never persist in raw form — confirmed by reading appendStageStart/appendStageEnd payloads.
  • stableHash is FNV-1a (non-cryptographic). That's appropriate for replay keys (collision risk only degrades replay precision, never introduces a security boundary).

Test coverage

Strong: continuation replay (test/unit/executor.test.ts:730-1100+), ambiguous prompt detection, parallel prompt topology, ledger isolation, persistence restore of replayKey, slash-dispatch answer routing, graph-overlay rendering of awaiting nodes, and stage-chat Ctrl+D variants under pending prompts. The PR adds ~1300 lines of tests for ~1300 lines of production code, which is the right ratio for state-machine code like this.

Gaps worth considering:

  • No test for the __ask_user_question__ fallback callId race noted above.
  • No test that asserts stageControlRegistry.clear() actually invokes handle.dispose() (the new wiring at stage-control-registry.ts:271-279).
  • The Windows-path normalization in prompt-callsite.test.ts covers the packaged runtime root — adding one case where a user's path happens to contain \packages\workflows\src\ would lock in the desired behaviour for issue 2 above.

Overall: looks ready to merge once the __ask_user_question__ collision is addressed.

@flora131

Copy link
Copy Markdown
Collaborator Author

Addressed this follow-up review in 144e273:

  • Removed the shared __ask_user_question__ fallback id and now track anonymous ask_user_question calls with a counter, so unrelated anonymous tool-end events cannot clear awaiting_input early.
  • Hardened prompt callsite runtime-frame filtering so user-authored paths that merely contain packages/workflows/src are preserved.
  • Added empty-option guards for prompt-node and legacy ctx.ui.select adapters.
  • Documented that stageControlRegistry.clear() disposes retained direct chat handles and added detached-handle dispose coverage.

Validation:

  • AGENT=1 bun test test/unit/executor.test.ts test/unit/prompt-callsite.test.ts test/unit/background-ui-adapter.test.ts test/unit/stage-control-registry.test.ts
  • AGENT=1 bun run test:unit
  • bun run typecheck
  • bun run lint
  • Commit/push hooks: bun run lint, bun run test:unit

@claude

claude Bot commented May 25, 2026

Copy link
Copy Markdown

Code Review — PR #1054 feat(workflows): render ctx.ui.* prompts as synthetic graph stage nodes

Reviewing as a Claude code reviewer. Overall this is a solid, well-scoped change with thoughtful test coverage (~1k lines of new tests across replay, ambiguity, abort, persistence). The architecture is sound: synthetic prompt nodes integrate cleanly with the existing frontier tracker, PromptAnswerRecord is correctly excluded from snapshots/persistence, and continuation replay handles parallel/duplicate cases explicitly. A few issues worth addressing.

Bugs / correctness

  1. Dead branch in normalizeStackPathpackages/workflows/src/runs/shared/prompt-callsite.ts:45

    } else if (normalized.startsWith(\"/\") && !win32.isAbsolute(normalized)) {

    win32.isAbsolute(\"/foo\") returns true (POSIX-style absolute paths are also Windows-absolute). The negation makes this branch unreachable for any path that already starts with /. If the intent is "relativize remote/leading-slash paths that aren't Windows drive paths," the predicate should be !posix.isAbsolute(...) or just drop the branch — non-cwd absolute paths can stay as-is. Behaviour today: harmless because such paths still get filtered by isWorkflowRuntimeFrame's substring match, but the branch is misleading and the test suite passes despite it never firing.

  2. Brief stale-state window in HIL abort handlerpackages/workflows/src/runs/foreground/executor.ts:1742-1746

    const onAbort = (): void => {
      activeStore.resolveStagePendingPrompt(runId, stageId, prompt.id, fallbackForPromptDescriptor(descriptor));
      activeStore.clearStagePromptAnswer(runId, stageId);
      reject(hilAbortError(ownController.signal));
    };

    resolveStagePendingPrompt fires notify() synchronously before clearStagePromptAnswer runs. Subscribers (e.g. StageChatView._syncPromptState) observe a transient promptAnswerState: \"available\" with the fallback in the ledger, then a second notify clears it. This isn't a security leak (fallback isn't the user answer), but observers could briefly flash "available." Consider adding a recordStageEnd overload or an internal resolveAndClear that batches the two ops behind a single notify.

  3. Stack-frame-based replay key fragilitypackages/workflows/src/runs/foreground/executor.ts:245-250 + prompt-callsite.ts:73-100
    The regex /(\\S+):(\\d+):(\\d+)\\)?$/ works for V8's at fn (path:line:col) format. The new tests only validate the parser on hand-crafted strings, not on a live new Error().stack under bun. If Bun's stack format ever shifts (esp. async stack-trace frames or anonymous arrow functions) the parser falls through to \"unknown\", collapsing every prompt of the same (kind, message) to the same replayKey — the parallel-disambiguation test (line 871 of executor.test.ts) would pass on Node and silently fail on Bun. Suggest adding a smoke test that captures a real stack from a workflow author callsite and asserts selectPromptCallsiteFrame returns a defined value under the runtime the repo actually uses (bun:test).

  4. replayContext only partially implements InternalStageContextexecutor.ts:1903

    const replayContext: StageContext & Pick<InternalStageContext, \"__modelFallbackMeta\"> = { ... };

    ctx.task (line 2360) currently uses (stage as InternalStageContext).__modelFallbackMeta?.() defensively with ?.. Any future consumer that calls __dispose, __ensureSession, __agentSession, __sessionMeta, etc. on a replayed stage will get undefined is not a function at runtime with no type safety. Either widen the Pick<...> to cover the actually-callable internal surface, or add a runtime guard / explicit narrowing helper so the gap is obvious.

Code quality / nits

  1. StageSnapshot.parentIds made mutablepackages/workflows/src/shared/store-types.ts:66 (was readonly). This was necessary for the synthetic-injection path, but it loses a useful invariant elsewhere. The executor now uses setStageParentIds(stage, parentIds) (executor.ts:1510) which re-freezes the array; could the field stay readonly with that helper acting on the snapshot object? At minimum, document the mutation contract on the type.

  2. StoreSnapshot.snapshot() deep-clones via JSON.parse(JSON.stringify(...)) every time _version bumps (store.ts:231). With prompt nodes adding many short-lived mutations (recordStagePendingPromptrecordStageAwaitingInputresolveStagePendingPromptrecordStageEnd is 4 notifies per prompt), this gets called for every subscriber on each mutation. Not new in this PR, but the prompt-node flow amplifies it. Worth profiling on workflows with many prompts before shipping to large fleets.

  3. promptDescriptorHash ignores initial when computing identity is intentional, but the comment block doesn't note this. If a workflow author calls ctx.ui.editor(\"draftA\") then later ctx.ui.editor(\"draftB\") at the same callsite, both prompts share a replay key. That's correct for replay (the initial is a default, not a user choice), but it's worth a one-line comment in promptReplayKey so a reader doesn't think it's a bug.

  4. PR size — 2682 additions, 33 files. The diff is cohesive (everything wires the same feature) but the test split + TUI work could plausibly land as two PRs (feat: synthetic prompt graph nodes + replay then feat: stage-chat prompt card integration). Not blocking — just an observation for future similar changes.

Documentation

  1. The Migration Notes list new replayKey and promptAnswerState fields on StageSnapshot, but the public docs at packages/coding-agent/docs/workflows.md only get the one-line UX update (line 275). Consumers reading StageSnapshot programmatically (e.g., extensions, custom reporters) won't learn about the \"available\" | \"unavailable\" | \"ambiguous\" semantics or the live-only PromptAnswerRecord ledger. A short section under "Human input" describing the replay model (and the security boundary on the answer ledger) would help.

Test coverage

  1. Strong coverage for the executor replay paths (continuation, parallel, ambiguous duplicate-callsite, parent-drift, abort). The new prompt-callsite.test.ts is parser-only and exercises five distinct frame shapes — see point (3) about adding a runtime smoke test.
  2. stage-chat-view.test.ts (+346 lines) appears to cover prompt rendering and answer flow, but I didn't see a test for the abort/clear race in point (2). Consider asserting that subscribers don't observe promptAnswerState: \"available\" during the abort path.

Security

  • PromptAnswerRecord correctly never enters snapshot(), never appended to persistence (verified via persistence-restore.ts — only replayKey/replayedFromStageId/replayed cross the boundary), and removeRun purges per-stage entries. The contract is clearly stated and enforced. Good.

Summary

Approve in spirit pending (1), (3), and (4); (2) is worth a follow-up. The replay infrastructure is the highest-risk piece and has the strongest test coverage — good prioritization.

🤖 Generated with Claude Code

@flora131

Copy link
Copy Markdown
Collaborator Author

Addressed this review in 0ecac9e:

  • Removed the unreachable normalizeStackPath branch and unused win32 import.
  • Added a recordAnswer: false stage-prompt resolution path for abort/default answers so subscribers do not observe a transient promptAnswerState: "available" state.
  • Added a real Bun stack smoke test for selectPromptCallsiteFrame().
  • Made replayed stages implement the full InternalStageContext surface with safe no-op/read-only internal hooks.
  • Documented the parentIds mutation contract and the prompt descriptor initial replay-key decision.
  • Expanded public workflow docs/README with promptAnswerState semantics and the live-only PromptAnswerRecord security boundary.

Validation:

  • AGENT=1 bun test test/unit/prompt-callsite.test.ts test/unit/executor.test.ts --test-name-pattern "real Bun|aborting a pending ctx.ui prompt|continuation maps replayed"
  • AGENT=1 bun test test/unit/prompt-callsite.test.ts test/unit/executor.test.ts test/unit/store-pending-prompt.test.ts
  • bun run typecheck
  • bun run lint
  • AGENT=1 bun run test:unit
  • Commit/push hooks: bun run lint, bun run test:unit

@claude claude Bot changed the title feat(workflows): render ctx.ui.* prompts as synthetic graph stage nodes feat(workflows)!: render ctx.ui.* prompts as synthetic graph stage nodes May 25, 2026
@claude

claude Bot commented May 25, 2026

Copy link
Copy Markdown

Code Review — feat(workflows): prompt nodes as synthetic graph stages

Nice, thorough piece of work. The synthetic-stage model is well-factored, the private answer ledger keeps the secret/snapshot boundary clean, and the continuation replay logic (descriptor hash + filtered author callsite) handles the hard cases — parallel duplicates, parent topology drift, ambiguous re-asks. Test coverage is excellent: +610 lines in executor.test.ts for continuation paths, a dedicated prompt-callsite.test.ts, and good coverage of the persistence/restore round-trip.

A handful of things to consider before merge — mostly correctness edge cases and a few code-quality nits.

Correctness

  1. Stack trace limit can swallow the author callsite (packages/workflows/src/runs/foreground/executor.ts:247-252, prompt-callsite.ts:92-98)
    Error.stackTraceLimit defaults to 10 in V8/Bun. With deeply nested workflow code (e.g. ctx.chain → ctx.parallel → ctx.task → stage.prompt → adapter → ... → ctx.ui.confirm), most frames are workflow runtime, and selectPromptCallsiteFrame may find no author frame and return undefined. promptCallsiteHash() then falls back to the literal string "unknown". Two independent prompts at distinct authoring sites would then collide on the same replayKey, which (per the duplicate-counting in createContinuationReplayIndex) tags both as "ambiguous" and forces a re-prompt on continuation.

    Suggest temporarily raising the limit around the capture, e.g.:

    function promptCallsiteHash(): string {
      const prev = Error.stackTraceLimit;
      Error.stackTraceLimit = Math.max(prev, 50);
      try {
        const frame = selectPromptCallsiteFrame(new Error().stack ?? "") ?? "unknown";
        return stableHash(frame);
      } finally {
        Error.stackTraceLimit = prev;
      }
    }

    …and add a regression test that runs ctx.ui.confirm from 12+ frames deep.

  2. Prompt-stage display name can collide with user stage names (executor.ts:1646-1672)
    Synthetic prompt stages get name = descriptor.kind ("input", "confirm", "select", "editor"). The replayKey discriminator (prompt:… vs stage:…) keeps them apart in the primary path, but the displayName-fallback in createContinuationReplayIndex.decide (line 1305-1307) filters source candidates by stage.replayKey === undefined, so older source snapshots that predate this PR (or any source missing replayKey) could match a user's ctx.stage(\"input\") against a synthetic prompt stage or vice versa. Probably very low risk in practice, but the prompt-stage display name (\"input\", \"confirm\") is also what users see in the graph viewer — consider a more distinctive label like \"ui:input\" so users can tell author stages apart from prompt stages and the namespacing matches the replayKey.

  3. store.recordStageEnd unconditionally overwrites replay metadata (packages/workflows/src/shared/store.ts:369-372)

    existing.replayKey = stage.replayKey;
    existing.promptAnswerState = stage.promptAnswerState;
    existing.replayedFromStageId = stage.replayedFromStageId;
    existing.replayed = stage.replayed;

    If a future caller invokes recordStageEnd(runId, { id, status, ... }) with a thin stage object that doesn't carry these fields, the live state is silently wiped. Today the executor always passes the same stageSnapshot reference so this is safe, but the asymmetry is brittle — other field updates here use if (... !== undefined)-style guards. Consider conditional assignment for consistency.

  4. Empty-select returns wrong type (executor.ts:1794, background-ui-adapter.ts:153)

    if (options.length === 0) return \"\" as T;

    If T extends \"yes\" | \"no\", \"\" is not assignable — the as T cast hides a runtime/contract violation. Functionally it falls through to the workflow body's branching logic, which may not have a case for \"\". Either reject the call (throw) or document that an empty select is a no-op that returns the empty string. Currently the new test \"empty options completes without creating a prompt node\" codifies the fallthrough behavior, so this is an API decision — but worth a docs note.

  5. promptDescriptorHash includes initial (executor.ts:232-241)
    The comment justifies including initial because changing visible context shouldn't replay a stale answer. Fair. But that also means a workflow that builds initial dynamically (e.g., ctx.ui.editor(PR #${number} description)) will never replay across continuations because the number/timestamp differs. Worth a note in the workflows docs alongside promptAnswerState.

Performance

  1. descendantsOf is O(N²) (executor.ts:1529-1530)
    runSnapshot.stages.filter(...) + per-stage BFS via hasAncestor is fine for typical workflow sizes (<100 stages) but could bite long-running workflows that do many cascade pause/resume cycles. Not blocking; flag if you start seeing many stages per run.

  2. Per-prompt stack capture (executor.ts:247-252)
    new Error().stack for every ctx.ui.* call is acceptable for human-paced HIL but would become noticeable for a workflow that prompts in a tight loop. The comment already acknowledges this — fine as-is.

Code organization

  1. executor.ts is now 2515 lines. The 170-line buildPromptNodeUiAdapter body inside run() and the also-substantial createContinuationReplayIndex factory are good extraction candidates — splitting into prompt-node-adapter.ts and continuation-replay.ts would make this file easier to navigate. Not blocking, but the trend is concerning.

  2. StageSnapshot construction in buildPromptNodeUiAdapter (executor.ts:1670-1690) — the nested ternary for the replay/answer fields is hard to scan. A small helper returning the field partial would clarify intent.

  3. stage-chat-view.ts._resolvePromptResponse (line 491-498) ignores the return value of store.resolveStagePendingPrompt. If it returns false (stale id, run already ended, etc.) we silently drop the input but the UI state is already cleared. A short comment or assertion would help future debugging.

  4. opts.usePromptNodesForUi vs opts.ui precedence (executor.ts:1815) — when both are set, usePromptNodesForUi wins silently. Worth a brief comment so future callers know they're mutually exclusive (or assert at top of run()).

Security / privacy

  • PromptAnswerRecord is correctly kept out of snapshots — separate _stagePromptAnswers map, never touched by JSON.stringify(snapshot). ✓
  • Cleared on clear() and removeRun but not on recordRunEnd. That's intentional (continuation needs it) and documented. ✓
  • One micro-nit: _stagePromptAnswers has no TTL or size cap. For a long-lived session with many sensitive prompt answers, they accumulate until session clear. Probably fine; flagging in case any user runs a session for days.

Tests

The new tests are tight and cover the interesting paths well. Suggested additions:

  • A test exercising the stack-trace-limit edge case described in (1).
  • A test that explicitly verifies a user ctx.stage(\"input\") and a ctx.ui.input(...) don't get cross-matched during continuation replay (per (2)).
  • A snapshot serialization test asserting that JSON.stringify(store.snapshot()) never contains the answer value (defensive against a future regression in (store.ts) that would smuggle the ledger into the snapshot).

Docs / migration

The README/docs prose around promptAnswerState reads well. The migration note about parentIds becoming mutable is good — confirm no consumer in packages/coding-agent or elsewhere relies on TypeScript's readonly enforcement at compile time (a quick grep should suffice).

Overall: solid PR. The stack-trace issue (1) is the only one I'd block on; the rest are polish/robustness improvements that could land in a follow-up.

@flora131

Copy link
Copy Markdown
Collaborator Author

Addressed this review in c81de74:

  • Raised Error.stackTraceLimit only around prompt callsite capture so deeply nested ctx.ui.* calls keep distinct author frames instead of falling back to unknown.
  • Added a regression test that forces a low global stack limit and verifies two same-descriptor prompt calls from deep/distinct callsites still produce distinct replay keys.
  • Made recordStageEnd preserve existing replay metadata when a future/thin caller omits those fields.
  • Added docs for empty ctx.ui.select(..., []) behavior and the dynamic initial replay-key tradeoff.
  • Added a comment for prompt-node-vs-adapter precedence and documented the stale-prompt resolution behavior in StageChatView.

Validation:

  • AGENT=1 bun test test/unit/executor.test.ts test/unit/store-pending-prompt.test.ts --test-name-pattern "deep prompt call stacks|recordStageEnd preserves|aborting a pending ctx.ui prompt"
  • AGENT=1 bun test test/unit/executor.test.ts test/unit/store-pending-prompt.test.ts test/unit/prompt-callsite.test.ts
  • bun run typecheck
  • bun run lint
  • AGENT=1 bun run test:unit
  • Commit/push hooks: bun run lint, bun run test:unit

@claude

claude Bot commented May 26, 2026

Copy link
Copy Markdown

Review — feat(workflows)!: render ctx.ui.* prompts as synthetic graph stage nodes

Thanks for the thorough write-up and tests. Overall this is a clean refactor with strong coverage. Below are findings grouped by severity.

Strengths

  • Live-only answer ledger is a good privacy boundary — _stagePromptAnswers lives outside the snapshot tree, never serialized/persisted; promptAnswerState exposes only an "available" | "unavailable" | "ambiguous" marker. Nice separation.
  • Replay-key design (prompt:<kind>:<descriptorHash>:<callsiteHash>) handles the realistic failure modes: identical-descriptor duplicates collapse to ambiguous and force re-prompt rather than silently replaying the wrong answer.
  • Test coverage is excellent — ~1.6k new test lines exercising continuation parent insertion, parallel→sequential drift, same-callsite ambiguity, packaged callsite filtering, abort cleanup, etc.
  • Packaged-runtime callsite filtering (prompt-callsite.ts:50-68) correctly handles both dev (packages/workflows/src/) and packaged (dist/builtin/..., node_modules/@bastani/...) roots.

Issues

1. Unhandled rejection in stage-control-registry.ts:272-280

for (const handle of handles) {
  void handle.dispose?.();
}

dispose() is typed void | Promise<void>; the executor.ts impl is async () => { await releaseLiveHandle(); }. If releaseLiveHandle rejects, this becomes an unhandled rejection. Suggest:

void Promise.resolve(handle.dispose?.()).catch((err) => {
  console.debug("pi-workflows: stage handle dispose failed", err);
});

2. Silent empty-options coercion in select (executor.ts:1801-1802, background-ui-adapter.ts:153)

async select<T extends string>(message: string, options: readonly T[]): Promise<T> {
  if (options.length === 0) return "" as T;

The cast lies to the type system — "" is unlikely to be a valid T. Calling select with [] is almost certainly programmer error; throw a precise error instead of silently returning a value the workflow body will never expect.

3. Defensive-but-misleading fallback in executor.ts:1670

activeStore.getStagePromptAnswer(opts.continuation?.source.id ?? "", replayDecision.source.id)

The ?? "" branch is unreachable — createContinuationReplayIndex never returns kind: "replay" when continuation is undefined. The fallback masks the invariant. Either opts.continuation!.source.id with a comment, or pull the continuation source id into a local before constructing the adapter.

4. Global Error.stackTraceLimit mutation (executor.ts:252-258)

Captures are synchronous within the try block, so this is technically safe for concurrent prompts (each new Error() resolves before its own finally). But it's still mutating a global from a hot path that can be exercised in parallel via Promise.all([ctx.ui.confirm(...), ctx.ui.confirm(...)]). If anything ever schedules between the limit bump and the new Error(), you'd silently get the wrong limit restored. Worth either:

  • a one-line comment in the function explaining why the synchronous capture makes the interleaving safe, or
  • dropping the bump and accepting that 10-frame deep helpers will collapse to "unknown" (the ambiguous re-prompt path handles that anyway).

5. Prompt answer lifetime / memory residency

_stagePromptAnswers holds raw user responses (potentially credentials/secrets entered via ctx.ui.input) for the entire run lifetime. They're only cleared on:

  • Explicit clearStagePromptAnswer call
  • removeRun / store.clear()

For a long-running workflow that prompts for a secret early and then runs for hours, the cleartext sits in heap. Consider:

  • Documenting this lifetime on getStagePromptAnswer / PromptAnswerRecord
  • Auto-clearing answers on recordRunEnd (since continuation replay only matters across run() invocations that consume a stored source snapshot)

Right now the JSDoc says "must never be logged, serialized, or copied into snapshots/persistence" — good — but doesn't mention residency.

6. Mutable parentIds subtlety

The new mutability contract is "consumer code treats as immutable; executor may swap the frozen array reference." External code that does const parents = stage.parentIds; ... use parents later ... will get stale data after late topology inference. Worth a one-liner on the field or in replaceParents doc warning against caching the reference.

Minor / style

  • stableHash(JSON.stringify(...)) in promptDescriptorHash allocates the full descriptor string up front. Fine for normal prompts, potentially wasteful for multi-MB editor prefills — acceptable on the HIL slow path.
  • A few inline comments still explain the WHAT (e.g. // a. Generate stageId, // b. tracker.onSpawn → provisional parentIds); CLAUDE.md prefers WHY-only. These look pre-existing though.
  • bun run typecheck, bun run lint, bun run test:unit are listed in the PR validation block — good adherence to the Bun-only rule.

Architecture / API

  • usePromptNodesForUi as a RunOpts flag is fine for the migration window, but the legacy background-ui-adapter.ts is now flagged @internal + TODO-for-deletion. Consider a follow-up issue to track removal so it doesn't accumulate as dead-but-loaded code.
  • replayKey derived from FNV-1a of {kind, message, choices, initial} + callsite — note that changing message text alone (e.g. tweaking copy in the workflow source) invalidates replay across continuations. The ambiguous re-prompt fallback handles this gracefully, so it's a non-issue, but worth being explicit in the workflow docs.

Nice work overall — the synthetic stage approach is a real UX win over the run-level overlay.

@flora131

Copy link
Copy Markdown
Collaborator Author

Addressed this review in d0a680e:

  • stageControlRegistry.clear() now observes async dispose() failures and logs them instead of allowing unhandled rejections.
  • Empty ctx.ui.select(..., []) now throws pi-workflows: ctx.ui.select requires at least one option before creating a prompt node; updated prompt-node and legacy adapter tests.
  • Replaced the defensive continuation source-id fallback with the explicit continuation invariant.
  • Clarified the temporary global Error.stackTraceLimit mutation around synchronous stack capture.
  • Expanded docs/JSDoc for prompt answer memory residency, replay-key invalidation inputs, and parentIds reference replacement.
  • Added comments for legacy display-name replay fallback and parent tracker/snapshot synchronization.

Validation:

  • AGENT=1 bun test test/unit/executor.test.ts test/unit/background-ui-adapter.test.ts test/unit/stage-control-registry.test.ts --test-name-pattern "empty options|clear observes|deep prompt call stacks"
  • AGENT=1 bun test test/unit/executor.test.ts test/unit/background-ui-adapter.test.ts test/unit/stage-control-registry.test.ts test/unit/store-pending-prompt.test.ts
  • bun run typecheck
  • bun run lint
  • AGENT=1 bun run test:unit
  • Commit/push hooks: bun run lint, bun run test:unit

@claude

claude Bot commented May 26, 2026

Copy link
Copy Markdown

Review — feat(workflows)!: render ctx.ui.* prompts as synthetic graph stage nodes

Reviewed against CLAUDE.md conventions (Bun-first, raw-TS @bastani/workflows, strict types, bun:test). Net assessment: thoughtful design, strong test coverage, careful handling of the snapshot/live-state split. Findings below in rough priority order.

Correctness / behavior

1. Error.stackTraceLimit mutation in promptCallsiteHash is a global side-effect. packages/workflows/src/runs/foreground/executor.ts:247-261 saves and restores Error.stackTraceLimit around stack capture. The save/restore is synchronous, so a single prompt cannot observe a mid-flight value, but the global is briefly mutated on every HIL call — any concurrent diagnostic code, profiler, or external library that samples Error.stackTraceLimit will see the temporary value. Bun's default is Infinity, which already makes Math.max(previousLimit, 50) a no-op. Consider either dropping the mutation entirely (rely on the runtime default) or capturing the trace with Error.captureStackTrace and a private container if you need a guaranteed minimum depth without flipping the global.

2. 32-bit FNV-1a hash collisions silently replay another stage's answer. stableHash (executor.ts:222-230) returns a 32-bit value; promptReplayKey combines kind plus two 32-bit hashes. Collisions are improbable but not negligible when the hash is the gate for surfacing a prior user-entered answer (which can include secrets per the PR description). Two practical options: (a) document collision behavior in the user-facing PR docs (the README/docs change today calls out what's hashed but not the collision property), or (b) move to a stronger hash (e.g. crypto.subtle.digest('SHA-256', …) truncated, or even a 64-bit FNV via two streams). Even SHA-1's bare bytes via crypto.createHash would change this from "unlikely" to "never going to happen."

3. parentIds: readonly string[] is now mutated by the executor. The store-types contract still says readonly, and consumers reading parent arrays during a tracker refresh may see stale references. The breaking-change note acknowledges this as "treat as immutable from consumer code," which is a documentation hedge that breaks the type contract. Two cleaner alternatives: keep parentIds truly readonly and have the executor swap the entire StageSnapshot reference on topology refresh, or expose a dedicated store.replaceStageParents(stageId, parentIds) mutation. Today the field is one of the structurally-typed properties that ties live snapshots to persistence; weakening it has a wide blast radius.

4. _graphStages filter is a behavioral assumption. packages/workflows/src/tui/graph-view.ts:293-304 filters out stages where status === 'pending' && startedAt === undefined && pendingPrompt === undefined && toolEvents.length === 0 whenever any stage on the run has a pending prompt. That's correct for the new node-injection flow, but the predicate is structural rather than semantic — any future code path that spawns a pending stage and lets it sit briefly will vanish from the graph while an unrelated prompt is up. A kind: \"placeholder\" flag on the snapshot would make the filter explicit and survive future executor changes.

5. _stagePromptAnswers is grow-only between explicit clears. Store entries are removed only by removeRun, clear, or explicit clearStagePromptAnswer (store.ts:601-617). Long-lived sessions with many prompts will accumulate retained raw answers in memory forever. The docstring at store.ts:144-149 says "resident in memory until explicitly cleared, the run is removed, or the store is cleared" — that's accurate, but you might want a TTL or a cap so a workflow with 10k prompts doesn't pin the answers permanently.

Style / smaller items

6. runDetached uses void _ignoredFgUi; to silence unused-destructure warnings (runs/background/runner.ts:111-118). noUnusedLocals requires the underscore prefix alone; the void statement adds runtime weight for a TypeScript-only concern. Consider const { jobs: _jobs, cancellation: _cancellation, ui: _ui, store: storeOverride, ...restOpts } = opts; and dropping the void.

7. promptDescriptorHash treats choices: undefined and choices: [] identically. This collapses two distinct legitimate inputs into one hash. Probably benign (an empty-options select throws upstream anyway), but if the descriptor ever permits empty choices in some other surface, the replay key will collide.

8. waitForExecutorStagePendingPrompt polls every 5 ms up to 1 s (executor.test.ts test helper). Works fine locally but is the kind of timing-based wait that flakes under CI load. An event-based wait via store.subscribe would be deterministic and the same length of code.

9. Legacy background-ui-adapter's deletion TODO (extension/background-ui-adapter.ts:23-24) is good — but adding a target milestone or tracking issue would help future maintainers know when it's safe to delete.

10. _syncPromptState runs inside StageChatView.render (tui/stage-chat-view.ts:578). Mutating UI state from a render pass is fragile — a render that doesn't fire (e.g. window hidden) leaves the state stale, and a render that fires twice does the work twice. The store subscription already calls _syncPromptState; the render-time call appears redundant.

Strengths worth calling out

  • The split between snapshot-safe promptAnswerState: 'available' | 'unavailable' | 'ambiguous' and the private PromptAnswerRecord is a well-considered design — it keeps secrets out of persistence while still giving continuation enough info to surface the right behavior.
  • Continuation handling of parent drift via hasOnlyReplayablePromptParentDrift (executor.ts:1291-1302) correctly distinguishes "the user's workflow grew/shrank a prompt branch" from "the topology is incompatible."
  • Stack-frame filter test cases cover both POSIX, Windows, file:// URLs, and node_modules layouts — very thorough.
  • runFailureMetadata and applyFailureToStage keep the failed-stage attribution clean across replay/cancel/fail-fast paths.
  • recordStagePendingPrompt correctly rejects on terminal stage/run state, preventing stale prompts after kill.

Nice work overall. The hash strength and Error.stackTraceLimit items are the only ones I'd push to change before merging; the rest are improvements you can take or leave.

@flora131

Copy link
Copy Markdown
Collaborator Author

Addressed the blocking items from this review in e335246:

  • Replaced the 32-bit FNV replay hash with SHA-256 truncated to 128 bits for prompt descriptor/callsite hashes.
  • Removed the temporary Error.stackTraceLimit mutation entirely and kept the deep-callsite regression test using Bun's default stack behavior.
  • Cleaned up the runDetached unused UI destructure so it no longer needs a runtime void statement.

Validation:

  • AGENT=1 bun test test/unit/executor.test.ts test/unit/background-runner.test.ts --test-name-pattern "deep prompt call stacks|runDetached|background"
  • bun run typecheck
  • bun run lint
  • AGENT=1 bun run test:unit
  • Commit/push hooks: bun run lint, bun run test:unit

@claude

claude Bot commented May 26, 2026

Copy link
Copy Markdown

Code Review — synthetic prompt graph nodes

Overall: ambitious, well-structured refactor with strong test coverage. The live-only PromptAnswerRecord ledger and promptAnswerState snapshot marker are a nice separation of secret-bearing payload from snapshot-safe metadata. A few items worth addressing or confirming below.

Correctness / potential bugs

  1. PR description vs. implementation hash mismatch. The PR body (and packages/workflows/README.md/docs/workflows.md) repeatedly says replay keys use FNV-1a, but the implementation uses truncated SHA-256:

    // packages/workflows/src/runs/foreground/executor.ts
    import { createHash } from "node:crypto";
    function stableHash(value: unknown): string {
      return createHash("sha256").update(JSON.stringify(value)).digest("hex").slice(0, 32);
    }

    SHA-256-128 is fine (and more conservative than FNV-1a), but the docs and PR description should be updated so future readers don't go looking for an FNV-1a routine that doesn't exist. If FNV-1a was the original intent, it would be much cheaper for this slow-path use; either way, please reconcile docs with code.

  2. opts.ui is silently dropped when usePromptNodesForUi: true. In run():

    ui: opts.usePromptNodesForUi === true
      ? buildPromptNodeUiAdapter()
      : opts.ui ?? makeUnavailableUIContext(),

    runDetached deliberately ignores _ui, but external callers that flip the flag and also pass ui won't get any signal that their adapter is unused. A console.warn or assertion (opts.ui === undefined) when both are set would prevent confusing test/host bring-up failures later.

  3. ctx.complete() AgentSession fallback skips finalizePromptOutput. In stage-runner.ts, the non-fallback prompt/complete paths run the SDK output through finalizePromptOutput(rawText, outputOptions, runtimeCwd). The new fallback:

    await promptWithFallback(text, undefined, \"complete\");
    lastAssistantText = lastAssistantTextFromSession(session, lastAssistantText) ?? \"\";

    bypasses output post-processing. The early check only rejects model/maxTokens/fallbackModels — if completeOpts.output exists (file write, format coercion, etc.), it's silently ignored when no CompleteAdapter is configured. Either reject output-like options here too, or thread them through finalizePromptOutput. Worth adding a test for complete(text, { output: ... }) without a CompleteAdapter.

  4. editor.onChange callback forces caret to end of text. In _resetPromptEditor:

    editor.onChange = (text: string) => {
      if (this.promptState?.prompt.id !== prompt.id) return;
      this.promptState.rawText = text;
      this.promptState.caret = text.length;   // ← always end-of-text
      this.requestRender?.();
    };

    Because the pi-tui Editor tracks its own caret internally and rendering uses editor.render(...), the visible UX is OK today. But anyone reading promptState.caret later (e.g., for cursor introspection, telemetry, future serialization, or in the applyTextEdit paths during a fallback) will get a stale/wrong position. A safer pattern: store the editor's current caret if exposed by pi-tui (e.g., editor.caret), or document that promptState.caret is only authoritative in the no-pi-tui code path.

  5. Replay-key callsite filtering depends on path strings. prompt-callsite.ts only treats a frame as runtime if it matches CURRENT_WORKFLOW_RUNTIME_ROOT (derived from import.meta.url) or one of two PACKAGED_WORKFLOW_RUNTIME_ROOTS substrings:

    const PACKAGED_WORKFLOW_RUNTIME_ROOTS = [
      \"/dist/builtin/workflows/src/\",
      \"/node_modules/@bastani/workflows/src/\",
    ] as const;

    Workflows installed at custom paths (pnpm hoisting variants, bun's node_modules/.cache/... for hot-reload, monorepo symlinks that don't resolve under node_modules/@bastani/workflows/src/, or Yarn PnP zip paths) will leak runtime frames into the replay-key hash. Two effects: (a) the replay key picks up executor.ts:LINE:COL and silently changes between deploys/installs, breaking continuation replay; (b) two prompts that should be ambiguous look distinct. Consider also matching /packages/workflows/src/ without anchoring to a leading slash, or grepping for known internal function names from the workflow runtime. A test for at least the pnpm + symlinked-monorepo cases would be valuable.

  6. Stack capture cost. new Error().stack is captured on every ctx.ui.* call. Acknowledged in the code comment, and prompts are a slow path — but if ctx.ui.input is ever used inside a tight loop (e.g., a workflow that polls for confirmation in retry logic), this can add up. Could be cached per (workflow-author-callsite via __callerHint || identity) if it becomes a problem.

Style / nits

  • id: \hil-${crypto.randomUUID()}`produces a non-canonical UUID string. Minor; consider either dropping the prefix or naming the fieldpromptToken`.
  • The doc string "Treat as immutable from consumer code" on parentIds is a real readonly → mutable contract change. Since this is a breaking change advertised in the PR body, please make sure the changelog entry under packages/workflows/CHANGELOG.md lists this under Breaking Changes with a migration line for consumers caching the reference.
  • _stagePromptAnswerKey joins ${runId}:${stageId} — fine for UUIDs but if either field ever holds a :, two distinct stages could collide. Using \\u0000 as the separator (you already do for sortedIdentity) would be more defensive.
  • setEditorBorderColor does if (candidate.borderColor !== undefined) candidate.borderColor = borderColor; — checking the existing value to decide whether to write a function looks accidental. If you're checking that the editor supports a borderColor field, prefer \"borderColor\" in candidate (or a typed capability probe).
  • console.debug(...) for async dispose failures in stage-control-registry.clear() is reasonable; just confirm that's the project convention (a quick scan of the rest of the codebase looks like console.warn is more common for failure paths).

Test coverage

Strong. The new tests cover: callsite filtering (multiple platforms + file URLs), deep-stack disambiguation, parallel prompt replay, ambiguous duplicate prompts, abort while pending (no leaked answer), topology-change rejection, prompt-parent-drift acceptance, persistence round-trip with replayKey, ledger purge on removeRun, structured-prompt rendering, pi-tui editor integration, scroll handling, Ctrl+D detach without answering, custom-UI overriding a pending prompt. Things I'd still add:

  • Repeated answering through clearStagePromptAnswer then re-resolving (already covered for the unavailable path; explicitly assert that re-resolving after clear stores a fresh ledger entry and bumps _version).
  • The hash-truncation collision risk is vanishingly small at 128 bits, but a sanity test that two structurally-different descriptors don't collide on the truncated 32-hex (and one that the descriptor key order is stable when produced by JSON.stringify) would lock in current behavior.
  • An e2e test for the documented invariant "raw answer never appears in persistence JSON" — currently store-pending-prompt.test.ts only checks s.snapshot() JSON, not actual persistence-session-entries JSON.

Security

Live-only ledger design is sound: _stagePromptAnswers lives outside RunSnapshot/StageSnapshot, is purged on removeRun/clear/explicit clear, and is not threaded into any appendStage* payload. Reviewed the recordStageEnd change (if (stage.X !== undefined) existing.X = stage.X) and the new ledger writes — no path observed where the raw answer is copied into promptAnswerState, persistence payloads, or notices. Good.

Suggested follow-ups (non-blocking)

  • Reconcile FNV-1a docs with SHA-256 implementation.
  • Plumb completeOpts.output (and any other untested fields) through the new ctx.complete() AgentSession fallback, or reject them explicitly.
  • Broaden the workflow-runtime callsite filter to handle non-default install layouts.
  • Consider a warning when both usePromptNodesForUi and opts.ui are set.

Nice work on the parent-drift detection (hasOnlyReplayablePromptParentDrift) — that's the kind of thing that's easy to get wrong, and the test coverage is reassuring.

@flora131

Copy link
Copy Markdown
Collaborator Author

Addressed this review in 18a146c:

  • Added an explicit warning when usePromptNodesForUi is enabled together with a caller-provided RunOpts.ui adapter.
  • Added changelog Breaking Changes / Added entries for synthetic prompt stages, promptAnswerState, parentIds reference replacement, and empty select behavior.
  • Hardened _stagePromptAnswers keys with JSON tuple encoding so colon-bearing run/stage ids cannot collide.
  • Switched stage handle async dispose failure logging to console.warn to match failure-path convention.
  • Fixed setEditorBorderColor capability detection to use property presence instead of the existing value.
  • Added comments for graph placeholder filtering and 128-bit SHA-256 replay hash truncation.
  • Added regression tests for prompt-node UI precedence warnings and colon-bearing prompt-answer ledger keys.

Validation:

  • AGENT=1 bun test test/unit/executor.test.ts test/unit/store-pending-prompt.test.ts test/unit/stage-control-registry.test.ts --test-name-pattern "warns when prompt-node|colon-bearing|clear observes"
  • AGENT=1 bun test test/unit/executor.test.ts test/unit/store-pending-prompt.test.ts test/unit/stage-control-registry.test.ts test/unit/background-runner.test.ts
  • bun run typecheck
  • bun run lint
  • AGENT=1 bun run test:unit
  • Commit/push hooks: bun run lint, bun run test:unit

Also confirmed ctx.complete() has no output/file options in CompleteStageOpts, so the output-postprocessing concern is not reachable through the public type surface.

@claude

claude Bot commented May 26, 2026

Copy link
Copy Markdown

Code review

I focused on the new prompt-as-graph-node path (executor.ts, store.ts, prompt-callsite.ts, stage-chat-view.ts, prompt-card.ts) and the supporting persistence/replay pieces. Overall this is a well-shaped change — the synthetic-node model is consistent with how ctx.stage() already flows through the executor, and the live-only _stagePromptAnswers ledger is the right way to keep raw responses out of snapshots/persistence. Tests are substantial (executor +681, stage-chat-view +346, callsite suite, ambiguous-prompt cases) and the documentation (CHANGELOG/README/workflows.md + the PR's breaking-changes table) makes the contract easy to follow.

A few things worth a look before merging:

Correctness / behavior

  • Tracker stays dirty when replayIndex.decide() throws. Both the new prompt path (packages/workflows/src/runs/foreground/executor.ts:1650) and the existing stage path (:1830) call tracker.onSpawn(stageId, kind) before replayIndex.decide(...). decide() can throw from failTopology(...) (:307, :321, :325), in which case the stage id is registered in GraphFrontierTracker but onSettle() never runs. The stage path predates this PR, but the prompt path widens the surface — a try { decide() } catch { tracker.onSettle(stageId); throw; } would keep the frontier clean if a future continuation hits the new prompt topology checks.
  • recordStageEnd semantics changed subtly. In packages/workflows/src/shared/store.ts:368-374 the old code unconditionally assigned replayedFromStageId/replayed from the incoming stage (clearing them to undefined if the caller omitted them); the new code only assigns when defined. This is the more correct behavior and is covered by the new "preserves existing replay metadata" test, but it's an observable change in store API semantics worth a one-liner in the Changed/Breaking section of the changelog since extensions could depend on the old clearing behavior.
  • replayKey for ctx.stage(...) is name-only (packages/workflows/src/runs/foreground/executor.ts:1833: stage:${name}). Prompt nodes get prompt:<kind>:<descriptorHash>:<callsiteHash>, but ctx.stage("foo") invoked from two different callsites collapses to the same identity. That's back-compat-safe (legacy snapshots also matched by name), but it means the stage path loses the disambiguation benefit you just built for prompts. If that's intentional for back-compat, a JSDoc note on replayKey saying "stage replay keys are name-only by design" would prevent someone "fixing" it later.
  • stage-control-registry.clear() doesn't await dispose (packages/workflows/src/runs/foreground/stage-control-registry.ts:273-280). It fan-outs Promise.resolve(handle.dispose?.()).catch(...) and returns synchronously. The doc comment says it's "used on session boundaries to release retained direct chat handles" — if the session is shutting down right after, those promises may be cut. The "observes asynchronous dispose failures" test awaits a microtask to verify the warn fires, which implicitly confirms the fire-and-forget shape; that may be fine, but consider whether host shutdown gives those disposers a chance to actually run.
  • replayKey accidentally encodes kind twiceprompt:${descriptor.kind}:${promptDescriptorHash(descriptor)}:${promptCallsiteHash()} where promptDescriptorHash already hashes kind (packages/workflows/src/runs/foreground/executor.ts:79-91). Harmless, just untidy.

Readability nits

  • caretLineUp / caretLineDown in packages/workflows/src/tui/prompt-card.ts:395 and :405 compute the visual column with visualColumnAt(raw.slice(lineStartOffset, safe), raw.slice(lineStartOffset, safe).length), which is just visibleWidth(raw.slice(lineStartOffset, safe)) written the long way. The current form takes a substring and then asks for the column at its end, which works but is harder to follow than a direct width call.
  • _syncPromptState is called from setup(), the store subscription, render(), handleInput(), and invalidate() (packages/workflows/src/tui/stage-chat-view.ts). It's idempotent so this isn't a bug, just defensive churn — most call sites could rely on the store subscription as the single source of truth.
  • The editor.onChange handler resets promptState.caret = text.length on every keystroke (packages/workflows/src/tui/stage-chat-view.ts:472). Safe today because _renderPrimitivePromptBody short-circuits the card path whenever the dedicated editor is mounted, but if the prompt-card fallback ever rendered alongside the editor, the cursor would jump to the end on every keystroke. A short comment pinning that invariant would help.

Security / privacy

  • The "raw answer never leaves the live ledger" contract is well structured: _stagePromptAnswers is a Map, removeRun/clear purge it, abort and default resolutions pass recordAnswer: false, and the JSON.stringify(snapshot()).includes("super-secret-value") === false assertion in test/unit/store-pending-prompt.test.ts:163 is a nice belt-and-braces check. Truncating SHA-256 to 128 bits in stableHash is fine for replay-key identity; nothing security-sensitive depends on the truncation.

Tests

  • Coverage of the new surface is genuinely good: replay-key collisions in prompt nodes (ambiguous case), parent-set drift heuristic, callsite normalization across file URLs / Windows paths / packaged-runtime roots, abort-while-awaiting, and the snapshot/persistence round-trip for replayKey. The one gap I noticed: I didn't see a test that exercises a ctx.ui.* prompt inside a parallel branch that completes in a different order between runs — the hasOnlyReplayablePromptParentDrift heuristic is the most subtle piece of the new replay logic and would benefit from a dedicated case.

Nothing here is blocking — the architecture and privacy model both look solid, and the breaking-changes are clearly called out.

@lavaman131
lavaman131 merged commit c58dcf8 into main May 26, 2026
10 checks passed
@lavaman131
lavaman131 deleted the issue/1046-awaiting-input-node-state branch May 26, 2026 01:15
lavaman131 pushed a commit that referenced this pull request Jun 29, 2026
…des (#1054)

* feat(workflows): render ui prompts as graph nodes

Implement node-local workflow UI prompt stages with awaiting_input state, stage-local prompt cards, continuation replay safeguards, live-only answer replay, and hardened prompt callsite identity for packaged runtime paths.

Add regression coverage for prompt-node lifecycle, continuation topology, prompt answer privacy, TUI routing, persistence restore, and packaged prompt callsite filtering.

AI-Assisted-By: GPT-5.5

* feat(workflows): enhance awaiting input stage prompts

Render stage-local text prompts through the host editor, support scrollable prompt bodies, and hide unstarted placeholder stages while prompt nodes await responses.

Keep completed stage chat handles resumable until the host clears the registry, and let ctx.complete fall back to SDK sessions when no complete-adapter-specific options are used.

Assistant-model: GPT-5.5

* fix(workflows): address prompt node review feedback

Assistant-model: GPT-5.5

* fix(workflows): harden prompt input edge cases

Assistant-model: GPT-5.5

* fix(workflows): harden prompt replay internals

Assistant-model: GPT-5.5

* fix(workflows): preserve deep prompt callsites

Assistant-model: GPT-5.5

* fix(workflows): tighten prompt edge handling

Assistant-model: GPT-5.5

* fix(workflows): strengthen prompt replay hashing

Assistant-model: GPT-5.5

* fix(workflows): document prompt replay invariants

Assistant-model: GPT-5.5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Show ctx.ui.input as awaiting input node state instead of graph overlay

2 participants