feat(workflow): show the plan before a Dynamic Workflow runs - #35
Conversation
Manual mode approved every DynamicWorkflow call outright. That policy sat below auto- and yolo-approve, so it only ever fired in manual mode — the one mode whose purpose is to ask was the one mode that never saw what it was agreeing to. Removing it lets the call reach the ask, and the approval now carries the fan-out: subagent count, task list, prompt template, worker model, and the summed size of the prompts about to be sent. The token figure is the real summed prompt estimate rather than a projected total cost. A guessed multiplier in an approval dialog is worse than no number, because the operator would be approving against a figure that could be an order of magnitude out. "Approve for this session" is keyed on the workflow description, so agreeing to one small review no longer pre-approves a later 128-agent fan-out. The rule ships with its matcher: an arg-bearing approval rule with no `matchesRule` never matches, which would record the grant and then ignore it every time. Also adds `/workflow save <name>`, which writes the last run out as a skill bundle under `.pythinker-code/skills/` so a fan-out that worked can be re-run by name. Names are validated rather than merely lowercased — `normalizeSkillName` is not a sanitizer, and feeding it straight to `path.join` let `../../..` place the file anywhere on disk.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughDynamic Workflow approvals now show execution plans and use plan-specific approval matching. The TUI stores completed workflow arguments and adds ChangesDynamic Workflow lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DynamicWorkflow
participant ApprovalAdapter
participant ApprovalPanel
DynamicWorkflow->>ApprovalAdapter: provide workflow preview metadata
ApprovalAdapter->>ApprovalPanel: emit invocation and workflow_plan blocks
ApprovalPanel->>ApprovalPanel: render plan details
sequenceDiagram
participant DynamicWorkflowTool
participant TUIState
participant WorkflowCommand
participant SKILLmd
DynamicWorkflowTool->>TUIState: store completed workflow arguments
WorkflowCommand->>TUIState: read lastDynamicWorkflowArgs
WorkflowCommand->>SKILLmd: write project skill
WorkflowCommand->>WorkflowCommand: refresh autocomplete
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
commit: |
|
Caution CodeRabbit couldn't update its existing comment. The review summary may be out of date. Error details |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/pythinker-code/src/tui/commands/dynamic-workflow.ts`:
- Around line 133-182: Refactor handleSaveSubcommand so it only parses the save
command, validates the required name/session data, and dispatches a save request
to a controller or lower-level persistence utility. Move savedWorkflowSkillDir,
mkdir, writeFile, renderSavedWorkflowSkill, formatErrorMessage, and related save
error handling out of this command module; keep UI status/error reporting and
autocomplete refresh at the appropriate higher-level boundary.
- Around line 162-175: Update the workflow skill write flow around
savedWorkflowSkillName, mkdir, and writeFile to reject any symlink in the target
directory or SKILL.md path, then write atomically using no-follow semantics so
an existing symlink cannot be followed or overwritten. Preserve normal workflow
creation behavior and add a regression test proving a symlinked SKILL.md is
rejected without modifying its target.
In `@apps/pythinker-code/src/tui/components/dialogs/approval-panel.ts`:
- Around line 204-214: Strip terminal control sequences from workflow metadata
before applying styling in the approval panel: sanitize block.model,
block.prompt_template, and each block.items entry before passing them to
summary, truncateOneLine, or rendered line construction. Preserve the existing
truncation and preview limits, using the project’s existing terminal-sequence
sanitization utility if available.
In `@packages/agent-core/src/agent/dynamic-workflow/save-as-skill.ts`:
- Around line 65-67: Update quoteYamlScalar to return JSON.stringify(value),
ensuring YAML double-quoted scalars preserve newlines, carriage returns, tabs,
and other control characters. Add a round-trip test covering a multiline
description through frontmatter serialization and parsing.
In `@packages/agent-core/src/tools/builtin/collaboration/dynamic-workflow.ts`:
- Around line 363-372: Update the approval-plan construction around prompts and
items so resumed-agent entries display both their agent ID and the normalized
prompt they will receive, matching the prompts counted in prompt_tokens.
Preserve the existing item ordering and count, and update the resumed-agent
preview test to assert the new combined content.
- Around line 180-181: Update the approval rule construction in the dynamic
workflow tool so approvalRule and matchesRule derive their subject from a
canonical payload containing every execution-affecting field, rather than only
args.description. Use a stable serialization or hash consistently for both rule
creation and matching, and add coverage for each execution-affecting field to
ensure differing plans cannot reuse the same session approval.
In `@packages/agent-core/test/tools/builtin-current.test.ts`:
- Line 882: Remove the conditional error guards from the affected tests around
the execution assertions. Move the shared non-error assertion and type narrowing
into a helper, or split each result branch into separate tests, while preserving
the existing validation of successful DynamicWorkflow execution.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: df3c11f1-4430-4a20-b629-2f05af8d7315
📒 Files selected for processing (22)
.changeset/dynamic-workflow-plan-approval.mdapps/pythinker-code/src/tui/commands/dynamic-workflow.tsapps/pythinker-code/src/tui/commands/registry.tsapps/pythinker-code/src/tui/components/dialogs/approval-panel.tsapps/pythinker-code/src/tui/controllers/subagent-event-handler.tsapps/pythinker-code/src/tui/reverse-rpc/approval/adapter.tsapps/pythinker-code/src/tui/reverse-rpc/types.tsapps/pythinker-code/src/tui/tui-state.tsapps/pythinker-code/test/tui/commands/dynamic-workflow.test.tsapps/pythinker-code/test/tui/commands/registry.test.tsapps/pythinker-code/test/tui/components/dialogs/approval-panel.test.tsapps/pythinker-code/test/tui/reverse-rpc/approval-adapter.test.tspackages/agent-core/src/agent/dynamic-workflow/save-as-skill.tspackages/agent-core/src/agent/index.tspackages/agent-core/src/agent/permission/policies/dynamic-workflow-mode-approve.tspackages/agent-core/src/agent/permission/policies/index.tspackages/agent-core/src/tools/builtin/collaboration/dynamic-workflow.tspackages/agent-core/test/agent/dynamic-workflow-save.test.tspackages/agent-core/test/agent/permission.test.tspackages/agent-core/test/tools/builtin-current.test.tspackages/node-sdk/src/index.tspackages/protocol/src/display.ts
💤 Files with no reviewable changes (1)
- packages/agent-core/src/agent/permission/policies/dynamic-workflow-mode-approve.ts
…tion Review findings on the preceding commit. Keying the session approval on the description let a second call keep the description, swap in a different 128-item list, and match the earlier grant — the exact fan-out the preview exists to expose. The subject now covers every field that changes what runs, the item list included. It cannot carry that plan verbatim. The rule DSL parses `Tool(subject)` by splitting on the first paren and then glob-matches the subject, so JSON punctuation breaks parsing and survives neither escaping nor picomatch: an approval would be recorded and then never match again. The subject is a trimmed description plus a digest of the plan, and a new test drives the real `matchPermissionRule` rather than the `matchesRule` callback, which is what made the encoding failure visible in the first place. Also: capture the workflow arguments for `/workflow save` when the tool call completes rather than inside the shared helper the streaming path also calls, so a half-parsed argument set cannot be saved; move the persistence into `writeSavedWorkflowSkill` in agent-core so surfaces other than the TUI can keep a workflow; and encode YAML scalars with `JSON.stringify`, which escapes the newlines and control characters the hand-rolled version emitted raw.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/agent-core/test/tools/builtin-current.test.ts (1)
883-892: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining digest fields.
dynamicWorkflowApprovalSubjecthasheseffortandoutput_schemain addition to the fields asserted here. Neither has a negative case. A regression that drops either field from the plan object would keep this test green.Add two cases in the same pattern.
♻️ Proposed additional assertions
expect( execution.matchesRule?.(subjectOf({ ...base, prompt_template: 'Rewrite {{item}}' })), ).toBe(false); + expect(execution.matchesRule?.(subjectOf({ ...base, effort: 'high' }))).toBe(false); + expect( + execution.matchesRule?.(subjectOf({ ...base, output_schema: { type: 'object' } })), + ).toBe(false); });As per path instructions: "New behavior should come with vitest coverage."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/test/tools/builtin-current.test.ts` around lines 883 - 892, Extend the test cases for dynamicWorkflowApprovalSubject by adding negative matches for changed effort and output_schema values, following the existing execution.matchesRule assertions. Ensure each case confirms the rule does not match when that digest field differs, while preserving the current test pattern.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/agent-core/src/agent/dynamic-workflow/save-as-skill.ts`:
- Around line 143-144: Update the save-as-skill write flow around fs.mkdir and
fs.writeFile to reject symlinked skill-directory components and an existing
symlink at SKILL.md before writing, ensuring writes remain within the configured
workDir skills root. Use no-follow or root-constrained filesystem checks, and
add regression coverage for both directory and file symlink cases.
In `@packages/agent-core/src/tools/builtin/collaboration/dynamic-workflow.ts`:
- Around line 407-424: Update dynamicWorkflowApprovalSubject to include the
resume prompt mapping or prompt values used by dynamicWorkflowPreview/execution
in the hashed approval payload, not just workflow.items containing resume agent
IDs. Ensure changed resume prompts produce a different digest, and add a
regression test covering approval reuse prevention when only a resume prompt
changes.
---
Nitpick comments:
In `@packages/agent-core/test/tools/builtin-current.test.ts`:
- Around line 883-892: Extend the test cases for dynamicWorkflowApprovalSubject
by adding negative matches for changed effort and output_schema values,
following the existing execution.matchesRule assertions. Ensure each case
confirms the rule does not match when that digest field differs, while
preserving the current test pattern.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e3b6bd46-0e65-4c48-bfa2-5c1ec57f5212
📒 Files selected for processing (8)
apps/pythinker-code/src/tui/commands/dynamic-workflow.tsapps/pythinker-code/src/tui/controllers/subagent-event-handler.tsapps/pythinker-code/test/tui/commands/dynamic-workflow.test.tspackages/agent-core/src/agent/dynamic-workflow/save-as-skill.tspackages/agent-core/src/agent/index.tspackages/agent-core/src/tools/builtin/collaboration/dynamic-workflow.tspackages/agent-core/test/tools/builtin-current.test.tspackages/node-sdk/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/agent-core/src/agent/index.ts
- apps/pythinker-code/src/tui/controllers/subagent-event-handler.ts
- packages/node-sdk/src/index.ts
- apps/pythinker-code/src/tui/commands/dynamic-workflow.ts
- apps/pythinker-code/test/tui/commands/dynamic-workflow.test.ts
Resume prompts were missing from both the preview and the approval digest. `items` carried only `resume <agentId>`, so the panel hid the instruction a resumed subagent would receive, and a second call could keep the same agent ids, change what it told them to do, and match the first call's session approval. The prompt now travels with the id, which fixes the display and the digest together. Saving a workflow followed symlinks. Agents work in repositories they did not write, and a checked-out tree can already contain `.pythinker-code/skills/<name>/SKILL.md` as a link pointing anywhere on the machine. The resolved path is now checked against the scope root before the directory is created and again before the write, and the write itself refuses to follow a final symlink. Checking after `mkdir` was not enough on its own: a symlinked `skills/` still had a directory created through it and left outside the project even when the write was refused. The approval panel styled workflow metadata without stripping terminal control sequences. That text comes from the model, and the panel is the thing the operator is judging, so a sequence could blank a line, redraw the buttons, or reverse the reading order of the plan being approved. Also lifts the narrowing out of the DynamicWorkflow tests into a helper, which is what `vitest(no-conditional-in-test)` was pointing at.
…-phase1-remainder
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/agent-core/src/agent/dynamic-workflow/save-as-skill.ts`:
- Around line 156-161: The save flow around assertResolvesInsideRoot and
writeFileNoFollow is vulnerable to parent-directory and final-component
replacement races; replace path-based validation/opening with
descriptor-relative, no-follow traversal for every directory and the SKILL.md
component, or fail the save when the platform lacks the required enforcement.
Preserve root containment and recursive directory creation, and add regression
coverage for both parent replacement and final-path replacement races.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e364c917-7504-405c-b665-2c809dfc0d33
📒 Files selected for processing (7)
apps/pythinker-code/src/tui/components/dialogs/approval-panel.tsapps/pythinker-code/test/tui/components/dialogs/approval-panel.test.tspackages/agent-core/src/agent/dynamic-workflow/save-as-skill.tspackages/agent-core/src/tools/builtin/collaboration/dynamic-workflow.tspackages/agent-core/test/agent/dynamic-workflow-save.test.tspackages/agent-core/test/tools/builtin-current.test.tspackages/node-sdk/src/index.ts
💤 Files with no reviewable changes (1)
- packages/node-sdk/src/index.ts
runHook has three outcomes for the PowerShell case: pwsh ran, pwsh is not installed, or pwsh was still starting when the hook budget expired. The assertion covered only the first two, so a saturated CI runner failed here with a bare "expected false to be true" that named nothing. The timeout path returns allow with empty stdout and stderr, so neither branch matched. All three outcomes confirm the PowerShell spawn path was taken, which is what the test is for.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/agent-core/test/hooks/runner.test.ts`:
- Around line 156-159: Update the assertion in the shell-selection test around
result.stdout, result.stderr, and result.timedOut so it passes only on
deterministic PowerShell evidence: the expected “ok” output or a
PowerShell-specific ENOENT result. Remove the generic result.timedOut branch and
avoid the conditional OR assertion by using an explicit deterministic mock or
separate assertions; move startup-timeout behavior into a dedicated test that
can independently fail.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a95d03f9-dc08-4ee3-9b2f-9051fd28aff9
📒 Files selected for processing (1)
packages/agent-core/test/hooks/runner.test.ts
Asserting on pwsh stdout tied this test to PowerShell being installed and to its cold start fitting inside the hook budget. On a loaded CI runner the budget expired, runHook returned allow with empty streams, and the assertion failed with a bare "expected false to be true". Accepting the timeout would have made the test vacuous: a runner that ignored options.shell entirely would still pass. Assert the spawn instead, which is the contract the test names and is deterministic. The mock delegates to the real spawn so the rest of the file keeps running actual processes.
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @pythoughts/pythinker-code@0.12.0 ### Minor Changes - [#35](#35) [`2ce6b5e`](2ce6b5e) - Show the plan before a Dynamic Workflow runs, and let a good one be saved as a command Manual mode used to approve every `DynamicWorkflow` call outright. That approval only ever fired in manual mode — auto and yolo approve earlier in the chain — so the one mode whose purpose is to ask was the one mode that never saw what it was agreeing to. A `DynamicWorkflow` call in manual mode now asks, and the approval carries the fan-out: how many subagents, the task list, the prompt template, the worker model, and the summed size of the prompts about to be sent. "Approve for this session" is keyed to that workflow's description rather than granting every future `DynamicWorkflow` call. `/workflow save <name>` writes the last run back out as a skill under `.pythinker-code/skills/`, so a fan-out that worked can be re-run by name. - [#38](#38) [`44efbc7`](44efbc7) - Let a release declare a minimum supported version, so a client below it is offered the update without waiting for its staged rollout batch. - [#38](#38) [`44efbc7`](44efbc7) - Show update availability and live download progress in the status row under the prompt, replacing the startup banner chip that was computed once and never refreshed. ### Patch Changes - [#37](#37) [`12069a8`](12069a8) - Stop offering updates to versions that were never published: the update channel now advertises only the release that is actually available for download. - [#38](#38) [`44efbc7`](44efbc7) - Stop offering an update with no build for the running platform, give every installer network call a timeout, expire a stale install lease instead of blocking updates forever, and say which version is installing and why a failed one stopped retrying. ## @pythoughts/pythinker-code-sdk@0.13.0 ### Minor Changes - [#35](#35) [`2ce6b5e`](2ce6b5e) - Show the plan before a Dynamic Workflow runs, and let a good one be saved as a command Manual mode used to approve every `DynamicWorkflow` call outright. That approval only ever fired in manual mode — auto and yolo approve earlier in the chain — so the one mode whose purpose is to ask was the one mode that never saw what it was agreeing to. A `DynamicWorkflow` call in manual mode now asks, and the approval carries the fan-out: how many subagents, the task list, the prompt template, the worker model, and the summed size of the prompts about to be sent. "Approve for this session" is keyed to that workflow's description rather than granting every future `DynamicWorkflow` call. `/workflow save <name>` writes the last run back out as a skill under `.pythinker-code/skills/`, so a fan-out that worked can be re-run by name. ## pythinker-code@0.8.6 ### Patch Changes - Updated dependencies [[`2ce6b5e`](2ce6b5e)]: - @pythoughts/pythinker-code-sdk@0.13.0 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added approval previews for Dynamic Workflow calls in manual mode, including fan-out details. - Added session-scoped workflow approvals. - Added `/workflow save <name>` to save the latest workflow run as a reusable skill. - Added live update availability and download progress indicators. - **Bug Fixes** - Improved update reliability with build checks, network timeouts, stale-install recovery, and clearer retry messages. - **Release Updates** - Released PyThinker Code 0.12.0, VS Code extension 0.8.6, and SDK 0.13.0. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Related Issue
No issue. Closes items 4 and 8 of Phase 1 in
blackbox/Dynamic-Workflow/pythinker_dynamic_workflow_enhancement_plan.md, whosescorecard has both marked Not started. See the note on item 5 below.
Problem
A
DynamicWorkflowcall can launch up to 128 subagents. In manual mode —the mode whose entire purpose is to ask before acting — it launched them
without asking.
DynamicWorkflowModeApprovePermissionPolicysat at position 15 of the policychain and approved every
DynamicWorkflowcall whenever workflow mode wasactive. Auto mode approves at position 6 and yolo at 14, so that policy could
only ever fire in manual mode. The one mode that asks was the one mode that
never saw what it was agreeing to.
Two smaller problems came with it:
approvalRulewas the bare tool name, so "approve for this session" grantedevery future
DynamicWorkflowcall. Agreeing to a 3-file review silentlypre-approved a later 128-agent fan-out.
What changed
The plan is shown before the fan-out runs. The blanket policy is deleted, so
a manual-mode call reaches the ask, and the approval carries the plan: subagent
count, task list, prompt template, worker model, and prompt size.
The token figure is the summed prompt estimate, not a projected total cost.
The plan text asks for projected tokens estimated from prior
usage.recordentries, but
resolveExecutionis synchronous and receives only the toolarguments — no usage history is reachable there. Rather than invent a
per-subagent multiplier, the preview reports a number that is actually true:
how much prompt is about to be sent. A guessed figure in an approval dialog is
worse than none, because the operator approves against it.
Session approval is scoped to the workflow.
approvalRuleis keyed on thedescription. The matcher ships with it — an arg-bearing rule with no
matchesRulenever matches, so the grant would have been recorded and thenignored on every later call. An existing test asserted the old bare-name
behaviour and caught this; it is rewritten to assert the new contract in both
directions.
/workflow save <name>writes the last run out as a skill bundle under.pythinker-code/skills/, reusing the existing skill scope model rather thanadding a parallel
.pythinker/workflows/loader.Note on Phase 1 item 5 (origin gating) — not implemented, deliberately
The plan asks for implicit and keyword triggers to require a human origin. No
such trigger exists.
DynamicWorkflowMode.enterhas two live call sites (theagent RPC and the tool) plus
restoreEnteron replay, andapplyAgentState—the only route from a request body into workflow mode — has exactly two callers,
both REST endpoints driven by human clients. There is no cron, webhook, or
keyword path to gate. Building the gate would add a threading burden across four
files to protect a surface that does not exist. Flagging rather than shipping it.
Verification
pnpm vitest runpnpm run typecheckpnpm lintapps/vscodeboth tsconfigsThe path-traversal rejection and the approval-panel rendering are covered by
tests that fail against the pre-fix behaviour.
Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.Summary by CodeRabbit
New Features
/workflow save <name>to save the latest workflow as a reusable project skill.Bug Fixes