[codex] Fix ENGINE_V2 auto-approve tool behavior - #2013
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
ilblackdragon
left a comment
There was a problem hiding this comment.
Automated review — LGTM
TL;DR: Root-cause fix with two-tier test coverage (unit + E2E). Targeted and focused — no unrelated cleanup bundled in. Ready to merge pending the clippy confirmation below.
Why it's solid
Restores `AGENT_AUTO_APPROVE_TOOLS=true` behavior for engine v2's `UnlessAutoApproved` tools while preserving `Always` gates. Regression was introduced in commit `4c9a985b` (#1557) when `EffectBridgeAdapter` was created — it only checked per-session "always" approvals and never received the global config flag. Fix correctly:
- Adds `auto_approve_tools: bool` field to `EffectBridgeAdapter` (not a bandaid)
- Chains `.with_global_auto_approve()` in router init (proper wiring)
- Exposes `Agent::config()` with `pub(crate)` visibility (correctly scoped)
- Short-circuits in `effect_adapter.rs:472-473` with `self.auto_approve_tools || ...` (correct OR logic)
Test coverage
Two-tier regression coverage:
- Unit (`bridge/effect_adapter.rs`): `global_auto_approve_skips_unless_auto_approved_gates` + `global_auto_approve_does_not_bypass_always_gates`
- E2E (`e2e_engine_v2.rs`): `v2_honors_global_auto_approve_for_unless_auto_approved_tools` with new `ApprovalProbeTool` that reproduces the exact issue #2010 scenario
Critical check: the separate test at `effect_adapter.rs:478-489` confirms `Always` gates still block even when the global flag is true. Destructive tools remain protected.
Open questions
- Confirm `cargo clippy --all --tests --examples --all-features` passes on this branch (not mentioned in PR description).
- E2E test is `#[cfg(feature = "libsql")]`-gated — intentional? Should the integration-feature PostgreSQL path also run it?
- Consider adding an E2E test for `Always` gates blocking in the full engine v2 pipeline (currently only at bridge level).
Summary
agent.auto_approve_toolsforUnlessAutoApprovedtoolsAlways-gated tools blocked even when global auto-approve is enabledRoot Cause
ENGINE_V2=truerouted tool execution through the bridge adapter, but that adapter only tracked per-toolalways approvedecisions and never received the globalAGENT_AUTO_APPROVE_TOOLSsetting. As a result, the v1 dispatcher respected the flag while the v2 path still paused on standard approval-gated tools.Impact
This restores parity between the legacy dispatcher and engine v2 for tool approval behavior. Deployments using
AGENT_AUTO_APPROVE_TOOLS=truewill no longer get unexpected approval pauses in v2 for standard tools, while destructiveAlwaysapprovals remain protected.Validation
cargo test global_auto_approve_ --lib --features libsql -- --nocapturecargo test need_approval_preserves_current_call_id --lib --features libsql -- --nocapturecargo test --test e2e_engine_v2 v2_honors_global_auto_approve_for_unless_auto_approved_tools --features libsql -- --nocapturecargo fmt --all --checkCloses #2010