Skip to content

fix: remove duplicate attempt field in OnRetryInput dataclass#2898

Open
praisonai-triage-agent[bot] wants to merge 1 commit into
mainfrom
claude/issue-2896-20260710-1713
Open

fix: remove duplicate attempt field in OnRetryInput dataclass#2898
praisonai-triage-agent[bot] wants to merge 1 commit into
mainfrom
claude/issue-2896-20260710-1713

Conversation

@praisonai-triage-agent

@praisonai-triage-agent praisonai-triage-agent Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Fixes #2896

Summary

The OnRetryInput hook dataclass in praisonaiagents/hooks/events.py declared the attempt field twice with conflicting defaults (1 then 0). Python keeps only the last annotation, so the intended 1-based default was silently shadowed by 0. The to_dict() method also wrote the "attempt" key twice (a no-op).

Changes

  • Removed the shadowing second attempt: int = 0 declaration, keeping a single attempt: int = 1 field with the intended 1-based default and a clarifying comment.
  • Removed the duplicate "attempt" key in to_dict().

Capability preserved

  • All four production constructors (agent/tool_execution.py, agent/execution_mixin.py, agent/chat_mixin.py:4254 and :4335) pass attempt= explicitly, so runtime behaviour is unchanged.
  • to_dict() output is identical (the duplicate key produced a single entry).
  • All legacy back-compat fields and hook firing behaviour untouched.

Validation

  • Verified default is now 1 and to_dict()["attempt"] returns 1.
  • tests/unit/hooks/ β€” 65 passed.
  • tests/unit/tools/test_retry.py + tests/unit/test_tool_retry_integration.py β€” 38 passed.

Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Retry attempt numbering now starts at 1, providing clearer and more intuitive retry tracking.
    • Retry event details continue to include delay information in the serialized output.

…2896)

The OnRetryInput hook dataclass declared the `attempt` field twice with
conflicting defaults (1 then 0), so the intended 1-based default was
silently shadowed by 0. The to_dict() also wrote the "attempt" key twice.

Removed the shadowing declaration and the duplicate dict key, keeping a
single `attempt` field with the intended 1-based default. All production
constructors pass attempt= explicitly, so runtime behaviour is unchanged.

Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
@MervinPraison

Copy link
Copy Markdown
Owner

@coderabbitai review

@MervinPraison

Copy link
Copy Markdown
Owner

/review

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more β†’

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account β†’

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us β†’

@MervinPraison MervinPraison added pipeline/blocked:ci Blocked: CI not green on HEAD pipeline/blocked:manual-review Blocked: requires manual review pipeline/blocked:no-final Blocked: no FINAL @claude trigger yet pipeline/final-claude-pending Reviews done; waiting for FINAL @claude labels Jul 10, 2026
@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor
βœ… Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. πŸŽ‰

ℹ️ Recent review info
βš™οΈ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4c7460e9-0241-4ee7-a7fa-66543a25fb87

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between ca6489a and f3f5463.

πŸ“’ Files selected for processing (1)
  • src/praisonai-agents/praisonaiagents/hooks/events.py

πŸ“ Walkthrough

Walkthrough

OnRetryInput now defaults attempt to 1-based indexing, and its serialized delay_seconds field is emitted later in the payload.

Changes

Retry input contract

Layer / File(s) Summary
Retry input default and serialization
src/praisonai-agents/praisonaiagents/hooks/events.py
Updates the default attempt value and changes the position of delay_seconds in to_dict() output.

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

πŸš₯ Pre-merge checks | βœ… 5
βœ… Passed checks (5 passed)
Check name Status Explanation
Description Check βœ… Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check βœ… Passed The title clearly matches the main OnRetryInput cleanup and is specific enough for history scanning.
Linked Issues check βœ… Passed The PR removes the duplicate attempt field and duplicate to_dict key requested in #2896 and keeps the intended default of 1.
Out of Scope Changes check βœ… Passed No unrelated changes are evident beyond the requested OnRetryInput cleanup and a minor dict field ordering adjustment.
Docstring Coverage βœ… Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
πŸ“ Generate docstrings
  • Create stacked PR
  • Commit on current branch
πŸ§ͺ Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/issue-2896-20260710-1713

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

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

@greptile-apps

greptile-apps Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR cleans up the retry hook input shape. The main changes are:

  • Keeps a single OnRetryInput.attempt field with the 1-based default.
  • Removes the shadowing duplicate attempt declaration.
  • Removes the duplicate attempt entry from to_dict().

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.
  • Current production retry hook constructors pass attempt explicitly.
  • Serialized retry hook output stays the same for those paths.

Important Files Changed

Filename Overview
src/praisonai-agents/praisonaiagents/hooks/events.py Removes duplicate attempt declarations and serialization keys from OnRetryInput without changing current explicit constructor paths.

Reviews (1): Last reviewed commit: "fix: remove duplicate attempt field in O..." | Re-trigger Greptile

@MervinPraison

Copy link
Copy Markdown
Owner

@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

  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/

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. Push all code fixes directly to THIS branch (do NOT create a new PR)
9. Comment a summary of exact files modified and what you skipped

Phase 3: Final Verdict
10. If all issues are resolved, approve the PR / close the Issue
11. If blocking issues remain, request changes / leave clear action items

@MervinPraison MervinPraison added pipeline/awaiting-merge-gate FINAL done; waiting for merge gate / CI pipeline/blocked:cooldown Blocked: post-push or @claude cooldown and removed pipeline/final-claude-pending Reviews done; waiting for FINAL @claude pipeline/blocked:no-final Blocked: no FINAL @claude trigger yet labels Jul 10, 2026
@praisonai-triage-agent

praisonai-triage-agent Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Claude finished @MervinPraison's task β€”β€” View job


Final Architecture Review β€” PR #2898 βœ… Approved

  • Read AGENTS.md + changed file
  • Validate the fix (correctness, backward compat)
  • Review reviewer feedback (Greptile, CodeRabbit, Qodo)
  • Verify runtime behaviour
  • Final verdict

Phase 1 β€” Architecture Review

Criterion Verdict
Protocol-driven βœ… Change is confined to a hook input dataclass in core praisonaiagents/hooks/events.py β€” correct location, no heavy impl.
Backward compatible βœ… All 4 production constructors pass attempt= explicitly (execution_mixin.py:1490, chat_mixin.py:4254 & :4335, tool_execution.py:2001), so runtime behaviour is unchanged. Legacy fields untouched.
Performance βœ… Pure dataclass cleanup, zero hot-path impact.
SDK value βœ… Genuine correctness fix β€” a duplicate attempt annotation silently shadowed the intended 1‑based default with 0, and to_dict() wrote the key twice. Removing it makes the default correct and the shape unambiguous. No scope creep.
Agent bloat βœ… No Agent params added.
Repo routing βœ… Hook lifecycle type lives correctly in core (it is a protocol/data type, not a heavy plugin implementation).

Phase 2 β€” Reviewer Feedback

  • Greptile: 5/5, safe to merge, no blocking issues. βœ… Concur.
  • CodeRabbit: still processing; nothing actionable flagged. Diff is 2 lines net.
  • Qodo: reviews paused (billing) β€” nothing to action.

No valid bugs or architectural flaws raised by any reviewer. No additional fixes needed.

Verification (runtime)

default attempt = 1
to_dict attempt = 1
delay_seconds in dict = True   # not accidentally dropped
attempt key count = 1          # duplicate key removed
OK

Note: I was unable to run the full pytest suite here (the pytest invocation was outside my approved command allowlist). Correctness was validated via direct construction/serialization above; the PR author reports 65 + 38 tests passing. If you'd like me to run the suites, add Bash(python -m pytest:*) / the relevant path to --allowedTools.

Phase 3 β€” Final Verdict

βœ… Approved. Minimal, focused, backward-compatible correctness fix. No files modified by me β€” the committed change is already complete and correct. Safe to merge.
β€’ Branch: claude/issue-2896-20260710-1713

@praisonai-triage-agent praisonai-triage-agent Bot removed pipeline/blocked:ci Blocked: CI not green on HEAD pipeline/blocked:cooldown Blocked: post-push or @claude cooldown labels Jul 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pipeline/awaiting-merge-gate FINAL done; waiting for merge gate / CI pipeline/blocked:manual-review Blocked: requires manual review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Core SDK: duplicate attempt field in OnRetryInput dataclass silently discards intended default

1 participant