fix: wrapper layer gaps — sync/async convergence, kanban FD leak, exception handling#1908
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthroughAgentsGenerator: extract YAML loading and workflow detection into _load_config and _is_workflow_yaml, early-return when missing, and route workflow-mode configs to _run_yaml_workflow/_arun_yaml_workflow; otherwise continue to the shared prepare/run path. KanbanDispatcher: log release_claim failures with traceback, fix subprocess log-file handle leak, and narrow shutdown exception handling (TimeoutExpired/OSError). ChangesAgentsGenerator config & kickoff routing
Error Handling and File-Handle Leak Fixes in KanbanDispatcher
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Greptile SummaryThis PR centralises config loading and workflow-type detection into
Confidence Score: 5/5Safe to merge; the kanban dispatcher fixes are straightforward and correct, and the config-loading refactor properly restores CLI-override merging for both YAML branches. The two concrete bugs addressed (FD leak and CLI-merge regression) are correctly fixed. The narrowed exception handling in the dispatcher is a clean improvement. The only item worth a second look is the undocumented addition of type: job / type: hybrid routing in _is_workflow_yaml, which changes dispatch behaviour for any config using those keys — but this is likely intentional and does not break existing configs that relied solely on process: workflow or steps+workflow sections.
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["generate_crew_and_kickoff / agenerate_crew_and_kickoff"] --> B["_load_config()"]
B --> C{agent_yaml set?}
C -- Yes --> D["yaml.safe_load(agent_yaml)"]
C -- No --> E["open(agent_file) yaml.safe_load"]
E -- FileNotFoundError --> F["print error, return None"]
D --> G["cli_config? _merge_cli_config"]
E --> G
G --> H{config is None?}
H -- Yes --> I["return silent"]
H -- No --> J["_is_workflow_yaml(config)"]
J -- process==workflow OR steps+workflow OR type job/hybrid --> K["_run_yaml_workflow / _arun_yaml_workflow"]
J -- otherwise --> L["_prepare_for_run(config)"]
L --> M["adapter.run / adapter.arun"]
Reviews (3): Last reviewed commit: "fix: wrapper layer gaps - CLI config reg..." | Re-trigger Greptile |
|
@claude You are the Lead Engineer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL analysis and reviews above carefully (Gemini, CodeRabbit, Qodo, Copilot, etc). Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Claude finished @MervinPraison's task —— View job Phase 1: Architecture Review & Analysis ✅Todo List:
✅ All Valid Issues FixedIssue 1: CLI Config Regression (Priority 1) - FIXEDProblem: Lines 474-486 - CLI config only merged when loading from file, not when # Before: CLI config only merged for file path
if self.cli_config:
self._merge_cli_config(config, self.cli_config) # Only inside file branch
# After: CLI config merged for both paths
if self.cli_config:
self._merge_cli_config(config, self.cli_config) # Outside conditionalIssue 2: Docstring Mismatch (Priority 2) - FIXEDProblem: Line 562 docstring claimed 5-tuple return, line 633 returned 4-tuple Issue 3: OSError in process.kill() (Priority 2) - FIXEDProblem: Lines 350, 357 - unguarded 🎯 Architecture Compliance Analysis✅ AGENTS.md Compliance:
✅ Refactoring Quality:
✅ Security Review:
🚀 Final Verdict: APPROVESummary of Changes:
What was NOT changed:
Test Status: Syntax validation passed. Full integration testing deferred to CI pipeline. Recommendation: ✅ Merge this PR 📁 Files Modified: |
![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
🧹 Nitpick comments (1)
src/praisonai/praisonai/gateway/kanban_dispatcher.py (1)
154-165: 📐 Maintainability & Code Quality | 💤 Low valueThe FD leak fix is correct, but the temp file creation pattern has a minor inefficiency.
The two-step approach (create temp file for path, then reopen) works but creates an unnecessary race window. A cleaner pattern would be to pass the file handle directly to Popen:
with tempfile.NamedTemporaryFile(mode='w', delete=False, suffix='.log') as temp_log: temp_log_path = temp_log.name process = subprocess.Popen( cmd, env=env, stdout=temp_log, stderr=subprocess.STDOUT, text=True )This avoids reopening the file and eliminates the TOCTOU window. The current implementation is functional and fixes the leak as intended, so this is optional.
🤖 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 `@src/praisonai/praisonai/gateway/kanban_dispatcher.py` around lines 154 - 165, Replace the two-step temp file creation and reopen with using the NamedTemporaryFile handle directly when launching the subprocess: keep the tempfile.NamedTemporaryFile(...) as temp_log, capture temp_log.name into temp_log_path if still needed, and pass temp_log as stdout to subprocess.Popen(cmd, env=env, stdout=temp_log, stderr=subprocess.STDOUT, text=True) in the kanban_dispatcher.py block (use the existing temp_log, temp_log_path, cmd and env symbols) to avoid reopening the file and the TOCTOU window while preserving the FD-leak fix.
🤖 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 `@src/praisonai/praisonai/agents_generator.py`:
- Around line 488-493: The _is_workflow_yaml predicate misses top-level
type-based workflows so YAMLs with type: "job" or "hybrid" are treated as
adapter mode; update the _is_workflow_yaml(config) function to also return True
when config.get('type') is 'job' or 'hybrid' (in addition to the existing
checks) so generate_crew_and_kickoff and agenerate_crew_and_kickoff route them
to the workflow executor; preserve the current process/type/steps/workflow logic
and add the extra type check to the boolean expression.
- Around line 474-485: The method currently returns
yaml.safe_load(self.agent_yaml) early, skipping CLI overrides; change the flow
in the agents_generator.py load routine (the code handling self.agent_yaml and
self.agent_file) to parse self.agent_yaml into a local config variable instead
of returning immediately, then call self._merge_cli_config(config,
self.cli_config) when self.cli_config is present, and finally return the merged
config; keep the existing file-path fallback logic (self.agent_file) unchanged.
---
Nitpick comments:
In `@src/praisonai/praisonai/gateway/kanban_dispatcher.py`:
- Around line 154-165: Replace the two-step temp file creation and reopen with
using the NamedTemporaryFile handle directly when launching the subprocess: keep
the tempfile.NamedTemporaryFile(...) as temp_log, capture temp_log.name into
temp_log_path if still needed, and pass temp_log as stdout to
subprocess.Popen(cmd, env=env, stdout=temp_log, stderr=subprocess.STDOUT,
text=True) in the kanban_dispatcher.py block (use the existing temp_log,
temp_log_path, cmd and env symbols) to avoid reopening the file and the TOCTOU
window while preserving the FD-leak fix.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b125980d-16a3-45f3-b28f-8d8e8af1ba91
📒 Files selected for processing (2)
src/praisonai/praisonai/agents_generator.pysrc/praisonai/praisonai/gateway/kanban_dispatcher.py
| def _is_workflow_yaml(self, config): | ||
| """Check if configuration is workflow mode YAML.""" | ||
| process_type = config.get('process', 'sequential') | ||
| has_steps = 'steps' in config | ||
| has_workflow_config = 'workflow' in config | ||
|
|
||
| if process_type == 'workflow' or (has_steps and has_workflow_config): | ||
| # Route to YAMLWorkflowParser for advanced workflow patterns | ||
| return self._run_yaml_workflow(config) | ||
|
|
||
| # Canonical format conversion: 'agents' -> 'roles', 'instructions' -> 'backstory' | ||
| # This ensures backward compatibility while supporting the new canonical format | ||
| if 'agents' in config and 'roles' not in config: | ||
| config['roles'] = {} | ||
| for agent_name, agent_config in config['agents'].items(): | ||
| role_config = dict(agent_config) if agent_config else {} | ||
| # Convert 'instructions' to 'backstory' if present | ||
| # Note: preserve 'instructions' key for adapters that pass it to PraisonAgent | ||
| if 'instructions' in role_config and 'backstory' not in role_config: | ||
| role_config['backstory'] = role_config['instructions'] | ||
| # Ensure required fields have defaults | ||
| if 'role' not in role_config: | ||
| role_config['role'] = agent_name.replace('_', ' ').title() | ||
| if 'goal' not in role_config: | ||
| role_config['goal'] = role_config.get('backstory', 'Complete the assigned task') | ||
| if 'backstory' not in role_config: | ||
| role_config['backstory'] = f'You are a {role_config["role"]}' | ||
| config['roles'][agent_name] = role_config | ||
| return process_type == 'workflow' or (has_steps and has_workflow_config) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Recognize type: job / type: hybrid as workflow YAML.
This predicate is now the gate for both generate_crew_and_kickoff() and agenerate_crew_and_kickoff(), but it misses the workflow shapes that the CLI routes by top-level type. A job or hybrid YAML without process: workflow will drop into the adapter path instead of the workflow executor.
Suggested fix
def _is_workflow_yaml(self, config):
"""Check if configuration is workflow mode YAML."""
process_type = config.get('process', 'sequential')
has_steps = 'steps' in config
has_workflow_config = 'workflow' in config
- return process_type == 'workflow' or (has_steps and has_workflow_config)
+ workflow_type = config.get('type')
+ return (
+ process_type == 'workflow'
+ or (has_steps and has_workflow_config)
+ or workflow_type in {'job', 'hybrid'}
+ )🤖 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 `@src/praisonai/praisonai/agents_generator.py` around lines 488 - 493, The
_is_workflow_yaml predicate misses top-level type-based workflows so YAMLs with
type: "job" or "hybrid" are treated as adapter mode; update the
_is_workflow_yaml(config) function to also return True when config.get('type')
is 'job' or 'hybrid' (in addition to the existing checks) so
generate_crew_and_kickoff and agenerate_crew_and_kickoff route them to the
workflow executor; preserve the current process/type/steps/workflow logic and
add the extra type check to the boolean expression.
|
@claude Rebase conflict ping (fresh) Please rebase onto Conflicts (merge-tree vs
Do not merge; reply here when rebased and checks pass. |
|
Claude finished @MervinPraison's task —— View job ✅ Rebase onto origin/main completed successfullyTodo List:
✅ All Tasks Completed SuccessfullyRebase Summary:
Key Fixes Applied During Rebase:
Sync/Async Convergence Status: Ready for merge - All conflicts resolved, CI green ✅ 📁 Branch: claude/issue-1863-20260607-0813 |
…on, cli_backend support (fixes #1863) * Fix sync/async path divergence in agents_generator.py - Extract shared _prepare_run() method consolidating ~150 lines of duplicate logic - Ensure both sync and async paths call adapter.resolve(), init_observability(), adapter.setup() - Add AgentOps initialization and AutoGen version selection to both paths - Remove behavioral drift between run() and arun() * Fix file handle leak in kanban_dispatcher.py - Replace inline open() with proper context manager in _spawn_worker() - Parent FD now closes immediately after Popen(), preventing slow leaks * Fix bare except clauses in kanban_dispatcher.py - Replace bare except: with specific exception handling - Add comprehensive error logging with task context - Allow KeyboardInterrupt/SystemExit/CancelledError to propagate for clean shutdown Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
…OSError handling - Fix CLI config regression: _merge_cli_config now applies to both agent_yaml and agent_file paths (fixes #1863) - Fix docstring mismatch: _prepare_run returns 4-tuple not 5-tuple - Fix OSError handling: guard process.kill() calls in kanban dispatcher shutdown 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Claude <noreply@anthropic.com>
praisonai-triage-agent
Bot
force-pushed
the
claude/issue-1863-20260607-0813
branch
from
05ce83e to
a90d96d
Compare
1 participant
Fixes #1863
Addresses wrapper-layer gaps: shared run preparation for sync/async paths, kanban dispatcher file-handle leak, and narrower exception handling.
Generated with Claude Code; PR opened from triage branch
claude/issue-1863-20260607-0813.Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Refactor