Skip to content

fix(run_agent): gate concurrent checkpoint preflight on block_result (fixes #34827)#34856

Closed
beardthelion wants to merge 1 commit into
NousResearch:mainfrom
beardthelion:fix/concurrent-checkpoint-preflight-order
Closed

fix(run_agent): gate concurrent checkpoint preflight on block_result (fixes #34827)#34856
beardthelion wants to merge 1 commit into
NousResearch:mainfrom
beardthelion:fix/concurrent-checkpoint-preflight-order

Conversation

@beardthelion

@beardthelion beardthelion commented May 29, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

In the concurrent tool-execution path, checkpoint preflight (write_file, patch, destructive terminal) fired BEFORE plugin guardrail block_result was computed. A blocked write_file could still dirty checkpoint state (doc_modified_this_turn, _last_write_file_call_id, turn_counter).

This PR moves checkpoint preflight to AFTER block_result computation, gated on if block_result is None: — matching the invariant the sequential path already enforces.

Related Issue

Fixes #34827

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • agent/tool_executor.py: Reorder concurrent path so block evaluation happens before checkpoint preflight. Checkpoint preflight now only runs when block_result is None.
  • tests/run_agent/test_run_agent.py: 4 new tests verifying blocked tools (write_file, patch, terminal) don't trigger checkpoint, and blocked write doesn't consume the dedup slot.

How to Test

  1. Run pytest tests/run_agent/test_run_agent.py -v -k concurrent — all 26 tests should pass
  2. The new tests specifically verify blocked tools skip checkpoint preflight in the concurrent path

Checklist

  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run tests and all pass
  • I've added tests for my changes

@beardthelion
fix(run_agent): gate concurrent checkpoint preflight on block_result (f…
e275f25 
…ixes NousResearch#34827)

In the concurrent tool-execution path, checkpoint preflight (write_file,
patch, destructive terminal) fired BEFORE plugin guardrail block_result
was computed. A blocked write_file could still dirty checkpoint state
(doc_modified_this_turn, _last_write_file_call_id, turn_counter).

Move checkpoint preflight to AFTER block_result computation, gated on
`if block_result is None:` — matching the invariant the sequential path
already enforces.
@beardthelion beardthelion force-pushed the fix/concurrent-checkpoint-preflight-order branch from ec7666a to e275f25 Compare May 29, 2026 20:48
@liuhao1024

liuhao1024 commented May 29, 2026

Copy link
Copy Markdown
Contributor

Verified: Correct fix for checkpoint-before-block evaluation ordering.

agent/tool_executor.py -- checkpoint preflight moved below block evaluation

The original code creates checkpoints for write_file, patch, and destructive terminal commands before evaluating whether the tool is blocked by guardrails. This means blocked tools still trigger ensure_checkpoint(), consuming dedup slots and creating unnecessary snapshots.

The fix reorders the code so:

  1. Block evaluation runs first (block_result is determined)
  2. Checkpoint preflight runs only when block_result is None (tool will actually execute)

This is correct -- function_name, function_args, and agent._checkpoint_mgr are all available at the insertion point (same loop iteration, after alias resolution).

Test coverage is thorough: 4 new tests covering blocked write_file, blocked patch, blocked terminal, and the edge case where a blocked write_file must not consume the dedup slot for a subsequent allowed write on the same path. All tests pass (confirmed in PR body: 140 passed).

Fixes #34827.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent loop, run_agent.py, prompt builder labels May 29, 2026
This was referenced May 30, 2026
Closed
Merged
@alt-glitch alt-glitch mentioned this pull request May 30, 2026
Merged
@teknium1

teknium1 commented May 30, 2026

Copy link
Copy Markdown
Contributor

Merged via PR #35255 (commit 6baf001). Your commit was cherry-picked onto current main with your authorship preserved in git log. Thanks for the clean fix and the test coverage.

@teknium1 teknium1 closed this May 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

comp/agent Core agent loop, run_agent.py, prompt builder P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Concurrent checkpoint preflight side effects in tool_executor

4 participants

@beardthelion @liuhao1024 @teknium1 @alt-glitch