Repository navigation
fix(chat): the workflow planner returned a fixed skeleton and auto-started it without approval (#13809) - #17947
Conversation
…e every generated command, default to approval (#13809)
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChat-driven workflow creation now uses the orchestrator planner with a shell-command contract. The manager converts tasks with commands into steps that require confirmation. The route defaults to plan approval and returns ChangesChat Workflow Planning
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🔵 Low · up to Malformed commands can appear in chat-created workflows. The current execution path limits the impact, but the conversion should be corrected. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Approval is safer by default, but generated plans can lose prerequisite relationships during conversion. Real command execution is not demonstrated by the available code, which limits confirmed impact while leaving execution-safety guarantees unresolved. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❓ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Full details: Out of Scope Changes checkExplanation The planner changes, approval defaults and related tests support Resolution Reviewable evidence of the exact edits in ✨ Finishing Touches 💡 1📝 Generate docstrings
🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
15d5f80c-7d66-4a43-b2d9-bcdae6e59a49
⛔ Files ignored due to path filters (2)
repo_tests/python_file_size_ratchet_baseline.pyis excluded by!repo_tests/python_file_size_ratchet_baseline.pyscripts/python_file_size_known_large.pyis excluded by!scripts/python_file_size_known_large.py
📒 Files selected for processing (4)
autobot-backend/orchestration/orchestrator_prompts.pyautobot-backend/orchestrator.pyautobot-backend/services/workflow_automation/manager.pyautobot-backend/takeover_manager_e2e_test.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
autobot-backend/services/workflow_automation/manager.py (2)
118-118: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAccept only non-empty string commands.
Line 118 keeps any task whose
commandis truthy. A list or dict passes this filter, andWorkflowStep.commanddoes not check its type at runtime. Accept a command only when it is a string with non-blank content after stripping.
130-130: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPlanner dependency references can be lost.
build_workflow_planassigns its own task IDs. LLM dependency references such astask_0therefore do not matchstep_ids, and line 130 drops them without a warning. A dependent command can then run without its prerequisite. Before conversion, resolve each dependency reference to a generated task ID. If a reference cannot be resolved, log it.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
8392976c-beae-490f-9170-8a322cd83492
⛔ Files ignored due to path filters (2)
repo_tests/python_file_size_ratchet_baseline.pyis excluded by!repo_tests/python_file_size_ratchet_baseline.pyscripts/python_file_size_known_large.pyis excluded by!scripts/python_file_size_known_large.py
📒 Files selected for processing (4)
autobot-backend/orchestration/orchestrator_prompts.pyautobot-backend/orchestrator.pyautobot-backend/services/workflow_automation/manager.pyautobot-backend/takeover_manager_e2e_test.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
… in silence (#13809) Both review threads on #17947, and they are the same shape: a filter that was right about what to exclude and wrong about doing it quietly. COMMANDS. `inputs.get("command")` was filtered on truthiness alone, so a non-empty list, dict or int passed it and travelled into `WorkflowStep.command` -- declared `str`, not validated at runtime -- and from there into workflow status and snapshots. A non-dict `inputs` raised `AttributeError` into the broad handler above and became a plain `return None`, which is the #13730 shape of an error vanishing into a "no workflow" result. `_planned_command` now requires a non-empty `str` and tolerates a malformed `inputs`. Measured against the old filter rather than assumed: of the eight rejected shapes, truthiness admitted five (list, dict, int, whitespace-only, and non-dict inputs by raising). DEPENDENCIES. The planner copies the model's `dependencies` through unchanged, and the model has no id to reference -- the schema asks for `["task_ids"]` while giving it no id field to assign, and ids are generated server-side after it answers. So every reference it writes is unresolvable by construction, and filtering them left steps that look independent: a plan that said "B waits for A" became indistinguishable from one that stated no ordering. The filter stays, because an id that resolves to nothing cannot be ordered against, but the unresolvable ids are now named in the log. Root cause is in the prompt shared by every planner caller, so it is filed as #17979 rather than fixed here; this PR makes the symptom visible and points at it. VERIFICATION, stated rather than overstated. This test module cannot run on this machine: it imports `StrEnum`, which needs 3.11+, and no interpreter here that has it also has pytest. CI (3.14) is the first place the new tests execute. What I did verify locally, by lifting the logic out of the module and exercising it directly: `_planned_command` is correct on all nine input shapes, and the dependency resolution reports exactly the unresolvable id, keeps the resolvable one, and stays silent when everything resolves. Refs #13809, #17979
#13809) `verify-generated-types` is a required check and it was red: this PR changed the `create_from_chat` route docstring, FastAPI renders a route docstring into the OpenAPI description, and the generated client types therefore drifted by exactly that one line. Applied from the `generated-types-patch` artifact that the drift job publishes rather than regenerating locally. Regenerating needs the backend's OpenAPI schema built, which means importing the application -- and the project rule is not to run codebase code outside CI or the deployment. The artifact is the sanctioned route and the job exists to provide it (#15362). The whole diff is the added description line; no type or shape changed. Refs #13809, #15362
1 participant
Closes #13809
Thinking Path
/api/workflow-automation/create_from_chatreturned the same three placeholder steps for every request:plan_workflow_stepsis a fixed skeleton, and every step fell through toecho 'Executing: <action>'. Before #13730 the path failed with a 500; afterwards it succeeded silently, doing nothing.The issue recommended routing to
create_workflow_plan, the canonical LLM planner. Reading that planner's prompt showed the recommendation alone would still have left the endpoint hollow: the prompt asks only for agent tasks{agent, action, inputs: {}}, never a shell command. Rerouting would have produced request-shaped actions whose steps still all echoed. Both points went to the owner, whose ruling is recorded on #13809 before any code:create_workflow_plan, the one canonical planner (AC4).require_approval=True,auto_start=False(AC3). The frontend's only caller (useWorkflowBuilder.ts) already sendsrequireApproval=true, so this removes a server/client divergence rather than tightening a default anyone relies on.requires_confirmation=True, a property of the step, whatever the route defaults are.A plan with no executable command, including the planner's own fallback when the LLM fails, now returns no workflow (a visible failure) rather than an echo-only one.
What Changed
orchestration/orchestrator_prompts.pybuild_planning_prompt(shell_commands=False): appends_SHELL_COMMAND_CONTRACTonly when askedorchestrator.py_build_planning_prompt(goal, planning_ctx);create_workflow_planreadscontext["shell_commands"]. The file is 6 lines shorter (1114 → 1108).repo_tests/python_file_size_ratchet_baseline.py,scripts/python_file_size_known_large.pyorchestrator.pyceiling lowered to 1108 in bothservices/workflow_automation/manager.pycreate_workflow_planwith the contract on;_steps_from_plankeeps only commanded tasks, allrequires_confirmation=True; no command →Noneservices/workflow_automation/routes.pycreate_from_chatdefaultsauto_start=False,require_approval=True, reasoning recorded at the defaulttests/services/test_chat_workflow_is_request_shaped.pytests/orchestration/test_planning_caller_wiring.py,takeover_manager_e2e_test.pyKnowingly left behind, filed as #17910:
_extract_command_from_step's keyword fallbacks (e.g. any "install" action →sudo apt install -y git curl wget) andclassify_request_complexity_verdictnow have no live caller. Neither is deleted.Verification
main(deployed backend interpreter, python 3.14.6; CI is the authority):386 passed across
orchestration/,services/workflow_automation/, the chat-workflow file andtest_planning_caller_wiring;11 passed in
takeover_manager_e2e_test.py; 14 passed in the two prompt suites(
test_learned_prompt_template_wiring,test_ledger_vs_executor); 31 passed inpython_file_size_ratchet_test.py, which is what proves the two baselines carry both thisbranch's entry and the one that landed meanwhile.
requires_confirmation=Falsetest_a_generated_command_requires_confirmation_on_its_stepshell_commands: False)test_the_chat_path_asks_the_planner_for_commandstest_the_route_starts_nothing_unless_askedtest_a_plan_with_no_command_creates_no_workflowpre-commit run --from-ref origin/main: 0 failed, as recorded pre-rebase. Not re-run after therebase: the 7 code files are byte-identical and the only merged region is the two baseline dicts,
whose own guard test passes.
git log 53803bddca..origin/main -- <each changed file>returns 0 commits for all 7 code files, but 1 for each of the two ratchet baselines:
the
schemas_chat.pyceiling moved732 -> 702. The pre-rebase tip still carried732,so merging it unrebased would have reversed that. Resolved in favour of the newer value:
the rebased baselines hold
schemas_chat.py: 702andorchestrator.py: 1108, andpython_file_size_ratchet_test.pypasses (31) against the real tree.Every one of the 9 changed files other than those two baselines is byte-identical to the
pre-rebase commit (blob-hash compared), so nothing else moved.
Model Used
Claude Opus 5.5
🤖 Generated with Claude Code
Summary by CodeRabbit