Skip to content

refactor: delegate _apply_tool_truncation_async to sync impl#2346

Open
praisonai-triage-agent[bot] wants to merge 2 commits into
mainfrom
claude/issue-2345-20260626-1711
Open

refactor: delegate _apply_tool_truncation_async to sync impl#2346
praisonai-triage-agent[bot] wants to merge 2 commits into
mainfrom
claude/issue-2345-20260626-1711

Conversation

@praisonai-triage-agent

@praisonai-triage-agent praisonai-triage-agent Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Fixes #2345

Summary

_apply_tool_truncation_async in agent/chat_mixin.py was a byte-identical copy of the sync _apply_tool_truncation (differing only in docstring and the -async log tag) and contained zero await statements. It is now a thin delegate to the sync implementation, eliminating ~28 duplicated lines and removing the sync/async drift risk.

Changes

  • _apply_tool_truncation_async now calls self._apply_tool_truncation(messages, compactor, policy).
  • Net: 2 insertions, 30 deletions.

Capability preserved

  • Same threshold (1000 chars), same head/tail slicing, same truncation message, same (CompactionRoute.TRUNCATE_TOOLS, truncated_msgs) return.
  • Async call sites still await the method β€” unaffected.

Validation

  • Functional parity check: sync and async paths produce identical output for a 2000-char tool message.
  • Module imports cleanly.

Generated with Claude Code

Summary by CodeRabbit

  • Refactor
    • Simplified tool-output truncation so async handling now uses the same shared logic as the synchronous path.
    • Centralized truncation behavior to keep results more consistent across chat processing.

…2345)

The async tool-truncation method was a byte-identical copy of the sync
version with no await points. Make it a thin delegate to eliminate ~28
duplicated lines and remove sync/async drift risk.

Co-authored-by: Mervin Praison <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 β†’

@coderabbitai

coderabbitai Bot commented Jun 26, 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 Jun 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

βš™οΈ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: eb583258-fdd0-420b-ae6d-2edfab67a151

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • πŸ” Trigger review
πŸ“ Walkthrough

Walkthrough

The async tool-output truncation helper now delegates to the shared synchronous truncation implementation. The async-specific copy of truncation, token counting, and logging logic was removed.

Changes

Tool truncation delegation

Layer / File(s) Summary
Async wrapper delegation
src/praisonai-agents/praisonaiagents/agent/chat_mixin.py
_apply_tool_truncation_async now forwards to _apply_tool_truncation, and the async-specific truncation body was removed.

Sequence Diagram(s)

None

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Possibly related PRs

Suggested labels

Review effort 2/5

Suggested reviewers

  • MervinPraison

Poem

A bunny hopped through truncation light,
πŸ‡ snippets trimmed and tucked just right.
The async path gave sync a call,
one shared trail now serves them all.
Flop, hop, done β€” neat as a night.

πŸš₯ 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 summarizes the refactor to delegate _apply_tool_truncation_async to the sync implementation.
Linked Issues check βœ… Passed The change removes duplicated async truncation logic by delegating to the sync helper, matching issue #2345's no-behavior-change goal.
Out of Scope Changes check βœ… Passed The PR only refactors the async truncation path in chat_mixin.py and does not introduce unrelated changes.
Docstring Coverage βœ… Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
πŸ§ͺ Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/issue-2345-20260626-1711

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.

@MervinPraison

Copy link
Copy Markdown
Owner

@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:

  1. Bloat check: Are changes minimal and focused? Any unnecessary code or scope creep?
  2. Security: Any hardcoded secrets, unsafe eval/exec, missing input validation?
  3. Performance: Any module-level heavy imports? Hot-path regressions?
  4. Tests: Are tests included? Do they cover the changes adequately?
  5. Backward compat: Any public API changes without deprecation?
  6. Code quality: DRY violations, naming conventions, error handling?
  7. Address reviewer feedback: If Qodo, Coderabbit, or Gemini flagged valid issues, include them in your review
  8. Suggest specific improvements with code examples where possible

@greptile-apps

greptile-apps Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR centralizes tool-output truncation so the async path reuses the shared helper.

  • Adds a configurable log tag to the sync truncation helper.
  • Keeps the sync path on the tool-truncation log tag.
  • Delegates async truncation to the shared helper with the tool-truncation-async log tag.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.

Important Files Changed

Filename Overview
src/praisonai-agents/praisonaiagents/agent/chat_mixin.py The async truncation wrapper now delegates to the shared helper while preserving its async log tag.

Reviews (2): Last reviewed commit: "fix: preserve [tool-truncation-async] lo..." | Re-trigger Greptile

Comment thread src/praisonai-agents/praisonaiagents/agent/chat_mixin.py Outdated
@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.

Phase 2: FIX Valid Issues
5. For any VALID bugs or architectural flaws found by Gemini, CodeRabbit, Qodo, Copilot, or any other reviewer: implement the fix
6. Push all code fixes directly to THIS branch (do NOT create a new PR)
7. Comment a summary of exact files modified and what you skipped

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

@praisonai-triage-agent

praisonai-triage-agent Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

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


Final Architecture Review β€” PR #2346 βœ…

  • Read AGENTS.md guidelines
  • Read changed file chat_mixin.py
  • Assess Greptile P2 log-tag concern
  • Apply fix
  • Run tests (functional parity verified)
  • Final verdict + push

Phase 1 β€” Review per AGENTS.md

Criterion Verdict
Protocol-driven βœ… Pure refactor in the wrapper layer; no new heavy impl in core.
Backward compatible βœ… Same threshold (1000), same head/tail slicing, same return tuple (TRUNCATE_TOOLS, msgs). Async call sites still await. Zero behavior regression.
Performance βœ… No hot-path regression β€” removes a duplicated loop; delegation is a single sync call (the async body had zero await, so nothing is lost).
SDK value βœ… Genuinely strengthens the SDK: eliminates ~28 byte-identical duplicated lines and removes a real sync/async drift risk. Not scope creep.
No Agent bloat βœ… No new Agent params. The fix below adds only a private-method kwarg, not a public surface.

This is a clean, valuable refactor that matches issue #2345 exactly.

Phase 2 β€” Fixed valid issue

Greptile P2 (VALID): Async truncation log misclassified. After delegation, the async path emitted the sync [tool-truncation] tag, dropping the prior [tool-truncation-async] marker. Any log filter/dashboard keying on the async tag would silently lose those events.

Fix (src/praisonai-agents/praisonaiagents/agent/chat_mixin.py):

  • Added optional log_tag="tool-truncation" kwarg to _apply_tool_truncation (line 789).
  • Log line now uses f"[{log_tag}] …" (line 815).
  • _apply_tool_truncation_async delegates with log_tag="tool-truncation-async" (line 910).

This restores the diagnostic distinction without re-introducing the duplicated body β€” the refactor's core goal is preserved (net +3/-3, still a thin delegate).

Skipped (not actionable): Qodo (reviews paused β€” no findings), CodeRabbit (πŸŽ‰ no actionable comments), Copilot/Gemini (no concrete blocking items surfaced beyond the log tag already handled).

Validation

import OK
functional parity OK   # sync & async produce identical output for a 2000-char tool msg

Confirmed: async path now logs [tool-truncation-async], sync logs [tool-truncation]. No tests/external code depend on the tag string (grep-verified), so this is a safe, additive change.

Phase 3 β€” Verdict: βœ… Approve (changes applied)

All reviewer feedback resolved. The PR is a net-positive de-duplication with backward-compatible behavior and now correct diagnostics. Pushed c23b7338 to this branch.

Note: I can't formally approve via GitHub review (security restriction), but from an architecture standpoint this is ready to merge.
β€’ Branch: claude/issue-2345-20260626-1711

Addresses Greptile P2: async truncation was emitting the sync
[tool-truncation] log tag after delegation. Add an optional
log_tag param to the sync helper so the async wrapper can pass
"tool-truncation-async", restoring the diagnostic distinction
without re-introducing sync/async drift.

Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
@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.

Phase 2: FIX Valid Issues
5. For any VALID bugs or architectural flaws found by Gemini, CodeRabbit, Qodo, Copilot, or any other reviewer: implement the fix
6. Push all code fixes directly to THIS branch (do NOT create a new PR)
7. Comment a summary of exact files modified and what you skipped

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

@praisonai-triage-agent

praisonai-triage-agent Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

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


Final Architecture Review β€” PR #2346 βœ…

  • Read AGENTS.md guidelines
  • Read changed file chat_mixin.py (current state)
  • Verify Greptile P2 log-tag fix is in place
  • Run validation
  • Final verdict

Phase 1 β€” Review per AGENTS.md

Criterion Verdict
Protocol-driven βœ… Pure refactor in the wrapper-method layer; no new heavy impl in core.
Backward compatible βœ… Same threshold (1000 chars), same head/tail slicing ([:300] / [-200:]), same return tuple (TRUNCATE_TOOLS, msgs). Async call sites still await (lines 855, 861). Zero behavior regression.
Performance βœ… No hot-path regression β€” the deleted async body had zero await, so delegation to the sync impl loses nothing and removes a duplicated loop.
SDK value βœ… Genuinely strengthens the SDK: removes ~28 byte-identical duplicated lines and eliminates real sync/async drift risk. Not scope creep.
No Agent bloat βœ… No new Agent params. Only a private-method kwarg (log_tag) was added β€” not public surface.

Phase 2 β€” Valid issues already fixed

Greptile P2 (VALID, resolved in commit c23b7338): After delegation the async path would have emitted the sync [tool-truncation] tag, dropping the prior [tool-truncation-async] marker. Confirmed fixed:

  • _apply_tool_truncation(... log_tag="tool-truncation") β€” chat_mixin.py:789
  • Log line uses f"[{log_tag}] …" β€” chat_mixin.py:814-816
  • Async wrapper delegates with log_tag="tool-truncation-async" β€” chat_mixin.py:910

This restores the diagnostic distinction without re-introducing the duplicated body.

Skipped (not actionable): Qodo (reviews paused β€” no findings), CodeRabbit (no actionable comments), Copilot/Gemini (no concrete blocking items beyond the log tag, already handled).

Validation

import OK
async delegates to sync: True
async log_tag = "tool-truncation-async": True

(pytest not installed in this runner env; verified via static + runtime introspection. grep confirms no tests/external code depend on the tag string, so the change is safe and additive.)

Phase 3 β€” Verdict: βœ… Approve (no further changes required)

All reviewer feedback is resolved on the current HEAD (c23b7338). The PR is a net-positive de-duplication with backward-compatible behavior and correct diagnostics. From an architecture standpoint this is ready to merge.

Note: I can't formally approve via the GitHub review API (security restriction), but this is my architectural sign-off. No new commits were needed this round β€” the branch already contains the complete fix.
β€’ Branch: claude/issue-2345-20260626-1711

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Duplicate logic: _apply_tool_truncation_async is byte-identical to the sync version (agent/chat_mixin.py)

1 participant