fix(agent): critical UnboundLocalError on Agent() without max_budget#1643
fix(agent): critical UnboundLocalError on Agent() without max_budget#1643MervinPraison wants to merge 1 commit into
Conversation
PR #1635 introduced a local 'from ... import ExecutionConfig' inside __init__ (line 743 inside 'if max_budget is not None:' branch). Per Python scoping rules, a name assigned anywhere in a function is treated as local throughout the entire function. When max_budget is None (the common case), the local branch never executes but line 777 still hits 'ExecutionConfig' and raises UnboundLocalError, breaking every Agent() instantiation without max_budget. ExecutionConfig is already imported at module level (line 162-166). Only resolve_execution needs the local lazy import. Test: tests/unit/agent/test_scientific_writer_agent.py surfaced this; now passes along with 311 other agent unit tests.
โ You've reached your Qodo monthly free-tier limit. Reviews pause until next month โ upgrade your plan to continue now, or link your paid account if you already have one. |
|
Caution Review failedThe pull request is closed. โน๏ธ Recent review infoโ๏ธ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ๐ Files selected for processing (1)
๐ WalkthroughWalkthroughAgent.init's ChangesExecutionConfig Import Scope Fix
Estimated code review effort๐ฏ 2 (Simple) | โฑ๏ธ ~8 minutes Possibly related issues
Possibly related PRs
Poem
โจ Finishing Touches๐ Generate docstrings
๐งช Generate unit tests (beta)
โ๏ธ Resolve 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. 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:
|
![gemini-code-assist[bot]](https://avatars.gh.lumenfield.work/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an UnboundLocalError in the Agent class by removing a redundant local import of ExecutionConfig within the init method. Feedback suggests also moving the local import of resolve_execution to the module level to prevent potential scope issues and ensure consistency with the fix applied to ExecutionConfig.
| # make Python treat ExecutionConfig as a local name throughout this | ||
| # entire __init__ function and break the branch below that uses it | ||
| # when max_budget is None (UnboundLocalError). | ||
| from ..config.feature_configs import resolve_execution |
There was a problem hiding this comment.
The local import of resolve_execution is redundant and carries the same risk of UnboundLocalError that this PR is fixing for ExecutionConfig. If resolve_execution were ever used outside this if block later in the __init__ function, it would trigger another UnboundLocalError when max_budget is None because Python treats names assigned via imports as local to the entire function scope.
Since ExecutionConfig is already imported at the module level (lines 162-166) from the same ..config.feature_configs module, the module is already loaded. There is no performance benefit to lazy-loading resolve_execution here. It is recommended to move resolve_execution to the module-level imports for safety and consistency.
Greptile SummaryThis PR fixes a Python scoping bug introduced by PR #1635: adding a
Confidence Score: 5/5Safe to merge โ the change is a minimal, well-targeted one-line fix that restores the pre-regression behavior for all standard The change removes exactly one symbol ( No files require special attention. The single changed file has a straightforward, isolated fix. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Agent.__init__(max_budget=...)"] --> B{max_budget is not None?}
B -- "Yes (budget provided)" --> C["lazy import: resolve_execution only\n(ExecutionConfig from module level)"]
C --> D{execution is None?}
D -- Yes --> E["execution = ExecutionConfig(max_budget=max_budget)"]
D -- No --> F["resolve_execution(execution)\nthen merge max_budget"]
F --> G[line 782: apply_config_defaults uses module-level ExecutionConfig โ
]
E --> G
B -- "No (common case)" --> H["skip branch entirely"]
H --> G
style G fill:#d4edda,stroke:#28a745
style C fill:#fff3cd,stroke:#ffc107
Reviews (1): Last reviewed commit: "fix(agent): critical UnboundLocalError o..." | Re-trigger Greptile |
|
@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: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task โโ View job Final Architecture Review - PR #1643 โ ANALYSIS COMPLETETask: Conduct final architecture review per AGENTS.md guidelines and implement fixes for valid issues found by previous reviewers. Todo Checklist:
๐ฏ Final Verdict: PR IS OBSOLETE - CLOSE WITHOUT MERGINGKey Finding: The original issue NO LONGER EXISTSโ
Verified: PR #1635 was completely reverted via #1642 Architecture Review per AGENTS.mdPhase 1: Architecture Compliance โ
Phase 2: Reviewer Analysis
Gemini's concern: The local import of Phase 3: Implementation Decision โ NO FIXES IMPLEMENTED - Reason: The entire PR addresses a problem that no longer exists post-revert of #1635. Technical SummaryOriginal Problem (PR #1635): # BROKEN: Local import inside conditional
if max_budget is not None:
from ..config.feature_configs import ExecutionConfig, resolve_execution
# ... uses ExecutionConfig later outside this block = UnboundLocalErrorCurrent State (main branch):
Recommendation:CLOSE PR #1643 - The regression it fixes was already resolved by reverting the problematic PR #1635 via #1642. |
1 participant
Critical Regression Fix
Root cause: PR #1635 introduced a local
from ..config.feature_configs import ExecutionConfiginsideAgent.__init__at line 743, inside theif max_budget is not None:branch.Per Python scoping rules, a name assigned anywhere in a function is treated as local throughout the entire function. When
max_budgetis None (the common case), the local branch never executes but line 777 still referencesExecutionConfigand raises:This broke every single
Agent()instantiation withoutmax_budgetโ effectively the entire library for most users.Fix
ExecutionConfigis already imported at module level (line 162-166). Onlyresolve_executionneeds the lazy local import. Removed the redundantExecutionConfigfrom the local import statement and added a comment explaining why it must stay at module-level.Validation
Agent(name='test', instructions='Be helpful')โ now works โAgent(name='test', max_budget=1.0)โ still works โtests/unit/agent/test_scientific_writer_agent.py::TestScientificWriterAgentIntegration::test_real_agentic_testsurfaced this bugDiscovery
Found while doing thorough post-merge validation of PRs #1635, #1637, #1626, #1639, #1611, #1600 against AGENTS.md standards.
Summary by CodeRabbit
Note: This release contains internal optimizations with no user-facing changes.