[dajiaohuang]

Problem

A workflow task whose predecessor failed can still dispatch through next_tasks, despite the default stop-on-dependency-failure policy. The async scheduler checks failed dependencies only inside one batch-flush branch, and the synchronous scheduler does not check them before workflow dispatch.

This surfaced in review of #5933, but is independently reproducible on unchanged main: returning None from the first task marks it failed and still invokes the second agent. It is not introduced by the guardrail rejection marker, so this fix is separate from that PR.

Minimal repair

Reuse the existing _deps_failed and _should_continue_on_dep_failure checks at the actual AgentTeam dispatch boundary, after necessary async flushes and before either sync or async task invocation. The synchronous workflow dispatcher uses the same checks. No new router, failure policy, API, or Process-generator rewrite.

Explicit dependent on_error="continue" and skip_on_failure=True keep their existing degraded-mode behavior. Successful dependencies retain context propagation.

Validation

• Original public regression matrix: 4 default-stop failures / 8 continuation controls pass on main.
• Final 24 start/astart × predecessor/successor sync/async × stop/continue/skip cases pass; 8 existing dependency-context cases also pass (32 total).
• New test Ruff and diff-check pass.
• Exact committed-head actual Agent.start, Agent.achat and a successful AgentTeam workflow produced local pretrained model text; owned loopback server stopped. Two preceding workflow-smoke attempts exceeded the tiny model's available context after reusing conversation history; clearing fixture history before the independent workflow run fixed that setup problem. Offline failure-policy tests are the proof for the repaired branch; this is not remote-provider or full-suite acceptance.

Base 1aeee2f39eae8ef79b0d5283b0e40448967b835d.

Summary by CodeRabbit

• Bug Fixes

  • Workflow tasks now consistently stop when a prerequisite task fails.
  • Dependent tasks are skipped by default in both synchronous and asynchronous workflows, preventing execution against failed prerequisites.
  • Workflows can still continue to dependent tasks when configured with on_error="continue" or skip_on_failure=True.

• Tests

  • Added coverage for failed prerequisites across synchronous and asynchronous workflow execution.

[qodo-code-review]

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: d8b5a069-482f-4cfc-b0f0-aae127cb594c

📥 Commits

Reviewing files that changed from the base of the PR and between 1aeee2f and bd360e8.

📒 Files selected for processing: 2

• src/praisonai-agents/praisonaiagents/agents/agents.py
• src/praisonai-agents/tests/unit/agents/test_workflow_failed_predecessor.py

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review.

📝 Walkthrough

Asynchronous and synchronous workflow execution now skip dependent tasks after a predecessor fails, unless the dependent task allows continuation. Tests cover both entry points, task execution modes, and failure policies.

Changes: Workflow failure handling

Layer / File(s)	Summary
Dependency checks and policy tests src/praisonai-agents/praisonaiagents/agents/agents.py, src/praisonai-agents/tests/unit/agents/test_workflow_failed_predecessor.py	Both execution paths mark and skip tasks whose dependencies failed, unless skip_on_failure or on_error="continue" allows continuation. Parametrized tests cover the entry points, task modes, and failure policies.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: mervinpraison

Merge Risk: ⚪ Minimal · up to bd360

The changed dispatch paths and tests cover the reported dependency-failure behavior. No merge-blocking issue is established beyond normal validation.

Pre-merge checks: 5

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: preventing task dispatch when workflow dependencies have failed.
Docstring Coverage		Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches: 💡 1

🧪 Generate unit tests (beta)

• Create a new PR

🛠️ Fix failing CI checks: 💡

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge.

Summary

The PR checks failed dependencies before workflow tasks run through either entry point.

• Workflow successors check failed dependencies before their agents run.

No actionable issues were found. Tests were reviewed but not run.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Workflow yields task] --> B[Wait for pending dependencies if needed]
    B --> C{Dependency failed?}
    C -- No --> D[Run task]
    C -- Yes --> E{Task allows continuation?}
    E -- Yes --> D
    E -- No --> F[Mark task failed without dispatch]

Loading

_{Reviews (1) · Last reviewed commit: "fix: check failed workflow dependencies ..." · Reviewed by Greptile}

[MervinPraison]

@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Review changes in this PR. Python SDK: praisonaiagents, praisonai. TypeScript SDK: src/praisonai-ts/. Do NOT modify src/praisonai-rust. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding.

MANDATORY READ (before reviewing):

• Always read src/praisonai-agents/AGENTS.md
• If this PR touches src/praisonai-ts/, also read src/praisonai-ts/AGENTS.md §2.1.2 (TS triage + PR review checklist)

Phase 1: Review per AGENTS.md

1. Protocol-driven: check heavy implementations vs core SDK
2. Backward compatible: ensure zero feature regressions
3. Performance: no hot-path regressions
4. SDK value: review in depth whether the change genuinely adds value to the SDK — never add features for the sake of adding them. It must strengthen the SDK (simpler, more user-friendly, robust, world-class, secure). If it does not clearly add value, request changes or recommend rejecting/closing rather than merging scope creep
5. Do not bloat the Agent class with additional params — only if absolutely required; we already support many params.
6. Repo routing: agent-callable tools → PraisonAI-Tools; lifecycle plugins → PraisonAI-Plugins; optional sandbox backends → PraisonAI-Plugins (praisonai.sandbox entry point) — request changes if wrongly added to praisonaiagents/
   6b. External tool examples (examples/tools/external/**): If the diff is confined to that path, mark SDK value ❌ for monorepo merge — redirect the contributor to PraisonAI-Tools; do not Approve for merge unless the PR has the maintainer-accept-example label.
   6c. New vendor *_tools.py in core (src/praisonai-agents/praisonaiagents/tools/): If the PR adds a new *_tools.py for a third-party API/SaaS/dataset (e.g. FXMacroData), mark Repo routing ❌ and SDK value ❌ — redirect to MervinPraison/PraisonAI-Tools. Do not Approve because legacy bundled tools (Tavily/Exa/You.com/etc.) already in core are not precedent for new vendor modules. Request changes or recommend closing unless the PR has maintainer-accept-core-tools.

MANDATORY COMMENT FORMAT — include this Phase 1 table in your review comment:

Phase 1 — AGENTS.md review

Check	Result
Protocol-driven / no heavy impl in core	✅ or ❌ + one-line rationale
Backward compatible	✅ or ❌ + one-line rationale
Performance (hot path)	✅ or ❌ + one-line rationale
SDK value	✅ or ❌ + one-line rationale (explicitly judge whether the change strengthens the SDK)
No Agent param bloat	✅ or ❌ + one-line rationale
Repo routing	✅ or ❌ + one-line rationale

For TypeScript PRs (src/praisonai-ts/), also add:
| TS types / parity / tests | ✅ or ❌ + one-line rationale (npm run build && npm test) |

Phase 2: FIX Valid Issues
7. For any VALID bugs or architectural flaws found by Gemini, CodeRabbit, Qodo, Copilot, or any other reviewer: implement the fix
8. Also independently identify and fix any gaps or issues you find in the changed code — do not rely only on prior reviewer feedback
9. Push all code fixes directly to THIS branch (do NOT create a new PR)
10. Comment a summary of exact files modified and what you skipped

Phase 3: Final Verdict
11. Never Approve or write “Approve for merge” if any Phase 1 row is ❌, if rule 6b/6c applies, or if the change belongs in PraisonAI-Tools/PraisonAI-Plugins. Use Request changes, redirect to the external repo, and recommend closing the monorepo PR when routing is wrong.
12. Approve only when every Phase 1 row is ✅ (including Repo routing and SDK value) and blocking issues are resolved; otherwise request changes with clear action items

[praisonai-triage-agent]

Security review: PASS

(Advisory only. This is a fork PR from an outsider, so a maintainer must merge it. I reviewed it from the PR data alone; I did not check out or run any of the code.)

#	Check	Result
1	Scope	OK. The PR touches 2 files: agents/agents.py (+11/−6) and one new unit test. Every change is about the dependency-failure check. There are no unrelated edits.
2	Exfiltration / network	None. There are no new URLs, hosts, sockets, telemetry or webhooks. The test sets llm="gpt-4o-mini", but chat/achat are overridden, so it should make no provider call.
3	Code execution	None. No eval/exec, subprocess, dynamic imports, pickle or yaml.
4	Secrets	None. No env vars, credential files or new logging are touched.
5	Weakened controls	None. The change makes the default stop-on-dependency-failure policy stricter. The existing opt-outs (on_error="continue", skip_on_failure=True) are unchanged. No bypass or debug switches are added.
6	Supply chain	None. No manifests, lockfiles, CI, conftest or entry points are changed.
7	Obfuscation	None. Both files are plain, short and readable, with no blobs or unusual unicode.
8	Tests	Nothing is removed or loosened. The new test asserts the outcome in both directions: 0 calls under stop, 1 call under continue/skip. It would therefore fail if the fix were missing or if it over-blocked.
9	Backdoors	None. Nothing is keyed on specific users, dates, hosts or magic values.
10	Injection artifacts	None found in code, comments, docstrings or the PR body.

No deterministic risk flags were included in the request, and the checklist above found none.

---

Code review

What it does

• arun_all_tasks, async branch: the existing _deps_failed / _should_continue_on_dep_failure check used to run only right after a batch flush. It now runs for every async task, so a predecessor that ran synchronously or that has already settled is also caught.
• arun_all_tasks, sync-in-async branch: adds the same check after pending async work has been flushed.
• run_all_tasks, workflow process: adds the same check before run_task.

The fix is minimal and reuses existing helpers. There are no new params, exports or modules, so it fits AGENTS.md's lightweight rule. Changing self.tasks[task_id].status to task.status is equivalent here.

Points for the maintainer to verify (I could not read the surrounding code)

1. Is the async change limited to workflow? If the loop in arun_all_tasks is shared with sequential and hierarchical, this change also alters async behaviour for those processes. A task with a failed context dependency would now be marked failed and skipped instead of running. That is probably the correct policy, but it goes beyond the PR's stated "workflow" scope. The sync path changes only the workflow branch, so if the async loop is shared, sync and async would now behave differently for sequential.
2. Do skipped dependents fire hooks? A dependent that is skipped is set to "failed" without going through run_task. Callbacks, hooks and trace events for that task will not fire. This matches the earlier flush branch, but downstream consumers such as status reporting should be OK with it.
3. How does the generator react? After a dependent is marked failed, process.workflow() / aworkflow() may route differently based on that status, for example through a next_tasks chain or a loop. Please check that a chain A→B→C with A failing ends cleanly and does not spin or re-yield.
4. CI: merge state is UNSTABLE. Please confirm that any failing checks are unrelated before merging.

Tests: good parametrized coverage, 24 cases (2 entry points × 2 × 2 sync/async combinations × 3 policies). Suggestion: add a 3-task chain (A fails → B skipped → C) to cover the failure propagating more than one step.

Verdict: this is a minimal, safe fix that makes failure handling stricter. Once points 1 and 3 are confirmed, it is OK to merge after manual review.