fix: configurable doom_loop approval gate for loop detection - #3776
fix: configurable doom_loop approval gate for loop detection#3776praisonai-triage-agent[bot] wants to merge 3 commits into
Conversation
…ble doom_loop gate (fixes #3772) Route a critical loop verdict through the unified approval pipeline as a synthetic `doom_loop` permission target instead of unconditionally blocking. This gives human override, per-project policy control (allow|ask|deny) and cost safety while preserving the historical hard-stop default. Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
✅ Action performedReview finished.
|
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? |
MervinPraison
added
pipeline/blocked:ci
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughCritical doom-loop detections now submit synthetic approval requests. Approved calls continue and reset loop history. Denied or failed approvals preserve blocking. Tests cover default blocking, automatic approval, and interactive denial. ChangesDoom-loop approval
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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-agents/praisonaiagents/agent/tool_execution.py`:
- Around line 660-661: Update the approval-handling flow around _ld_history and
record_tool_call to remove the exact tool-call entry that was recorded before
approval, rather than blindly popping the latest entry. Return and retain a
record token or use synchronization to identify and remove that specific entry,
preserving concurrent records from other calls.
- Around line 1623-1633: Update the doom-loop approval request in the
tool-execution flow to include an immutable, normalized copy of the original
tool arguments in request_args, alongside tool, detector, and count, so approval
caching remains scoped to the exact call. In
src/praisonai-agents/praisonaiagents/agent/tool_execution.py lines 1623-1633,
modify the request construction; in
src/praisonai-agents/tests/unit/agent/test_loop_detector_wiring.py lines
232-264, add coverage for two critical loops with different arguments and assert
the default-scope interactive backend receives distinct doom_loop requests.
In `@src/praisonai-agents/praisonaiagents/approval/registry.py`:
- Around line 48-53: The DEFAULT_DANGEROUS_TOOLS entry currently reserves the
public tool name doom_loop for synthetic approval handling, affecting ordinary
user tools. Update the approval target handling around DEFAULT_DANGEROUS_TOOLS
to use an internal namespaced synthetic target or separate synthetic-target
registry, while preserving doom_loop as a backward-compatible policy alias if
required; add a regression test proving a normal tool named doom_loop is not
automatically blocked.
🪄 Autofix
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 Plus
Run ID: cb9f93ed-562e-453c-8a08-dae1c1974bbf
📒 Files selected for processing (3)
src/praisonai-agents/praisonaiagents/agent/tool_execution.pysrc/praisonai-agents/praisonaiagents/approval/registry.pysrc/praisonai-agents/tests/unit/agent/test_loop_detector_wiring.py
| if _ld_history: | ||
| _ld_history.pop() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Remove the recorded loop entry by identity.
Line 661 removes whichever entry is last after approval returns. If another call on the same agent records a tool call while approval waits, pop() removes that other call instead of the approved call.
Return a record token from record_tool_call, or protect detector updates with a lock and remove the exact recorded entry.
🤖 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-agents/praisonaiagents/agent/tool_execution.py` around lines
660 - 661, Update the approval-handling flow around _ld_history and
record_tool_call to remove the exact tool-call entry that was recorded before
approval, rather than blindly popping the latest entry. Return and retain a
record token or use synchronization to identify and remove that specific entry,
preserving concurrent records from other calls.
| request_args = { | ||
| "tool": function_name, | ||
| "detector": verdict.get("detector"), | ||
| "count": verdict.get("count"), | ||
| } | ||
| decision = registry.approve_sync( | ||
| getattr(self, "name", None), | ||
| "doom_loop", | ||
| request_args, | ||
| force=True, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scope a doom-loop approval to the original tool call.
The synthetic request excludes the original tool arguments. The approval registry caches approved requests by target and request arguments. An approval for one repeated check_status call can therefore allow a different check_status call with the same detector and count without another interactive decision.
src/praisonai-agents/praisonaiagents/agent/tool_execution.py#L1623-L1633: Include an immutable normalized copy of the original tool arguments inrequest_args.src/praisonai-agents/tests/unit/agent/test_loop_detector_wiring.py#L232-L264: Exercise two critical loops with different tool arguments and assert that a default-scope interactive backend receives separatedoom_looprequests.
📍 Affects 2 files
src/praisonai-agents/praisonaiagents/agent/tool_execution.py#L1623-L1633(this comment)src/praisonai-agents/tests/unit/agent/test_loop_detector_wiring.py#L232-L264
🤖 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-agents/praisonaiagents/agent/tool_execution.py` around lines
1623 - 1633, Update the doom-loop approval request in the tool-execution flow to
include an immutable, normalized copy of the original tool arguments in
request_args, alongside tool, detector, and count, so approval caching remains
scoped to the exact call. In
src/praisonai-agents/praisonaiagents/agent/tool_execution.py lines 1623-1633,
modify the request construction; in
src/praisonai-agents/tests/unit/agent/test_loop_detector_wiring.py lines
232-264, add coverage for two critical loops with different arguments and assert
the default-scope interactive backend receives distinct doom_loop requests.
| # Runaway-safety gate: a detected doom/repeat loop routes through the | ||
| # approval pipeline as a synthetic ``doom_loop`` target. Registered as | ||
| # ``critical`` so the default (no explicit allow) stops — preserving the | ||
| # historical hard-block posture — while ``doom_loop=allow`` lets a | ||
| # legitimate repeat (e.g. polling a build status) proceed. | ||
| "doom_loop": "critical", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not reserve a public tool name for the synthetic approval target.
DEFAULT_DANGEROUS_TOOLS also controls approval for ordinary tool execution. An existing user tool named doom_loop will now require approval and can be denied by default, even when no loop is detected.
Use an internal namespaced target, or add a separate synthetic-target registry. Keep doom_loop as a policy alias if required. Add a regression test for a normal user tool named doom_loop.
As per coding guidelines, preserve backward compatibility with existing Python APIs.
🤖 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-agents/praisonaiagents/approval/registry.py` around lines 48 -
53, The DEFAULT_DANGEROUS_TOOLS entry currently reserves the public tool name
doom_loop for synthetic approval handling, affecting ordinary user tools. Update
the approval target handling around DEFAULT_DANGEROUS_TOOLS to use an internal
namespaced synthetic target or separate synthetic-target registry, while
preserving doom_loop as a backward-compatible policy alias if required; add a
regression test proving a normal tool named doom_loop is not automatically
blocked.
Source: Coding guidelines
Greptile SummaryThe PR makes critical loop detection recoverable through the existing approval system and resets approved exact-repeat history.
Confidence Score: 3/5The PR is not yet safe to merge because YAML doom-loop approvals are ignored and approved ping-pong loops can immediately re-enter the critical gate. Registering Files Needing Attention: src/praisonai-agents/praisonaiagents/approval/registry.py and src/praisonai-agents/praisonaiagents/agent/loop_detection.py
|
| Filename | Overview |
|---|---|
| src/praisonai-agents/praisonaiagents/agent/loop_detection.py | Adds exact-pair history reset, but the previously reported ping-pong reset defect remains because opposite-side records are preserved. |
| src/praisonai-agents/praisonaiagents/agent/tool_execution.py | Routes critical loop verdicts through PermissionManager and registry approval while failing closed on errors. |
| src/praisonai-agents/praisonaiagents/approval/registry.py | Registers the synthetic target as critical, which prevents the advertised YAML auto-approval override from succeeding. |
| src/praisonai-agents/tests/unit/agent/test_loop_detector_wiring.py | Covers backend approval and exact-repeat resets but does not exercise YAML approval or ping-pong reset behavior. |
| src/praisonai/tests/unit/cli/test_interactive_surfaces_parity.py | Updates a test stub to accept the production callback's optional read_only argument. |
Sequence Diagram
sequenceDiagram
participant Agent
participant Detector as Loop detector
participant PM as PermissionManager
participant Registry as ApprovalRegistry
participant Backend
Agent->>Detector: Record and inspect tool call
Detector-->>Agent: Critical loop verdict
Agent->>PM: Resolve doom_loop policy
alt Explicit allow
PM-->>Agent: Allow
Agent->>Detector: Reset matching history
Agent->>Agent: Execute tool
else Explicit deny
PM-->>Agent: Deny
Agent-->>Agent: Return loop_blocked
else No explicit rule
Agent->>Registry: "approve_sync(__doom_loop__, force=True)"
Registry->>Backend: Request approval
Backend-->>Registry: Allow or deny
Registry-->>Agent: Decision
end
Reviews (3): Last reviewed commit: "fix(test): align async TUI parity fake w..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@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 |
MervinPraison
added
pipeline/awaiting-merge-gate
|
@claude CI failed on HEAD Failed checks
Failures (extracted)
Critical review firstBefore changing code or tests, decide which side is wrong:
SDK guardrails (AGENTS.md):
What to do
|
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #3776 (doom_loop approval gate)
Verdict on the CI failure — pre-existing flake, unrelated to this PR
Phase 1 — architecture (per AGENTS.md)
Phase 2 — valid issues fixed (pushed to this branch)
Files modified: Tests
Phase 3 — final verdict: Approve (Python scope)The four valid reviewer findings are resolved with minimal, backward-compatible changes that genuinely strengthen the SDK's runaway-safety gate. The remaining CI red is a pre-existing CLI-package flake outside this PR's scope and should be tracked separately. |
… scoped approval) Address reviewer feedback on the doom_loop approval gate: - Reset the whole matching (tool,args) streak on an explicit allow instead of a blind pop() so a legitimate repeat gets a fresh critical_threshold window and is not re-prompted on every subsequent call. Removing by identity also leaves concurrent unrelated records intact (fixes the pop() race). - Move the synthetic gate out of DEFAULT_DANGEROUS_TOOLS to a namespaced internal target (__doom_loop__) so it can never reserve/collide with a real user tool named doom_loop or leak into the safe/read_only presets. - Honour an explicit PermissionManager allow/deny on the doom_loop policy alias before consulting a backend, so Agent-level policy is not bypassed. - Scope the approval request to the exact repeated call via a stable args fingerprint so an allow for one loop cannot silently authorise another. Adds regression tests: streak-reset (no per-call reprompt), per-argument scoping, user-tool-name isolation, and internal-target criticality. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
MervinPraison
added
pipeline/blocked:stale-final
|
Claude finished @MervinPraison's task —— View job CI Fix —
|
| before = len(history) | ||
| history[:] = [ | ||
| rec for rec in history | ||
| if not (rec.get("tool_name") == tool_name and rec.get("args_hash") == args_hash) |
There was a problem hiding this comment.
If critical_threshold is configured to three or lower, reset_matching_history removes only the approved tool/argument pair while the ping-pong detector retains records from the other side of the alternating sequence. The next alternating call can therefore reach the critical threshold immediately and request approval again instead of receiving a fresh detection window.
Knowledge Base Used: praisonai-agents Core Library
… signature _execute_in_background calls _execute_prompt(prompt, read_only=...) since the review-command work (#3734). The parity test's _fake_execute stub only accepted `prompt`, so the extra kwarg raised TypeError inside the inlined execute thread, which was swallowed and left `captured` empty (KeyError: 'prompt'). Mirror the real signature in the stub; assertions (single-pass expansion + sentinel body) are unchanged. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
| self._required_tools.add(DOOM_LOOP_TARGET) | ||
| self._risk_levels[DOOM_LOOP_TARGET] = "critical" |
There was a problem hiding this comment.
When a workflow adds doom_loop or __doom_loop__ to its YAML-approved tools, the synthetic target's critical risk causes is_yaml_approved to reject the configured approval unconditionally, so the request falls through to backend approval or denial and the legitimate repeated operation remains blocked.
Knowledge Base Used: praisonai-agents Core Library
praisonai-triage-agent
Bot
removed
the
pipeline/blocked:cooldown
1 participant
Fixes #3772
Summary
Wires the Agent's loop-detection subsystem into the unified approval/permission system so a detected doom/repeat loop becomes a first-class, recoverable
doom_loopdecision instead of a hardcoded, non-overridable block.Previously (
agent/tool_execution.py), acriticalverdict unconditionally constructed ablocked_resultwith no human decision point and no policy control — a false-positive block (e.g. a legitimate poll of a build/job status returning identical results) could not be overridden.What changed
agent/tool_execution.py— on acriticalverdict, route through the existing approval registry via a new_doom_loop_approved()helper using a syntheticdoom_looptarget (force=True).allow→ proceed and reset the streak;deny/timeout/no-backend/error → the existing block path (fail-closed).approval/registry.py— registerdoom_loopinDEFAULT_DANGEROUS_TOOLSatcriticalrisk so the default posture still stops (backward-compatible), whiledoom_loop=allow(env/YAML/PermissionManager/backend) lets a legitimate repeat continue.ApprovalRegistry.approve_sync, backends, and theON_PERMISSION_ASKhook path. No new params onAgent, no new modules, no new dependencies.Behaviour
Tests
Added
TestDoomLoopApprovalGateintests/unit/agent/test_loop_detector_wiring.py:doom_loop=allow(AutoApproveBackend) continues past the thresholdAll 12 loop-wiring tests + 15 default-tool-safety tests pass. The 9 failures in the wider approval suite are pre-existing on
main(unrelated: ConsoleBackend naming, stdin capture, preset deny sets).Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes