test(harness): add Phase 2 replay and gateway coverage - #2896
Conversation
First fixture-driven Layer 1 (replay) coverage of the full v1 approval cycle: pause -> user resolution -> resume. Companion to the existing no_done_emitted_while_awaiting_approval test in e2e_response_order.rs, which covers the pause but not the resume. Three scenarios: - approval_yes: user approves -> tool runs once -> final LLM response - approval_no: user denies -> tool does NOT run -> agent surfaces a built-in rejection message (no follow-up LLM call, by design) - approval_always: allow-always on first call -> second call runs without re-prompting, exactly one ApprovalNeeded total Uses a test-only NeedsApprovalProbe tool with ApprovalRequirement::UnlessAutoApproved registered via TestRig::with_extra_tools, with auto_approve_tools(false) so the agent actually pauses for resolution. The deny-path discovery (no LLM follow-up on rejection) is documented in the test so future readers don't reintroduce the trailing text step. Updates tests/fixtures/llm_traces/README.md to list the new fixtures. Bumps approvals coverage in the harness-testing matrix from ~ to (closer to) full at Layer 1.
Adds the four approval scenarios that the original three-test set omitted, completing the state-space matrix across ApprovalRequirement variants, the master kill-switch config, and submission-routing edge cases. New tests (all in tests/e2e_approval_traces.rs): - always_requirement_ignores_allow_always_persistence ApprovalRequirement::Always is the unbypassable hard floor — even an 'allow-always' resolution must NOT skip the pause on subsequent calls of an Always-tool. Two pauses for two calls. - slash_approve_routes_as_approval_response '/approve' is parsed as Submission::ApprovalResponse even though bare 'yes' downgrades to UserInput when nothing is pending. Pins the divergent routing in submission.rs. - bare_yes_with_no_pending_approval_is_user_input Bare 'yes' with no pending approval must downgrade to UserInput and reach the LLM as a normal user message. Asserts the routing layer in agent_loop.rs performs the downgrade (parser is stateless). - config_auto_approve_bypasses_unless_auto_approved Agent-config auto_approve_tools=true is the master kill-switch — no ApprovalNeeded is ever emitted, even for UnlessAutoApproved tools. Also adds AlwaysApprovalProbe (mirrors NeedsApprovalProbe but returns ApprovalRequirement::Always) and three fixtures: - approval_always_floor.json - approval_slash.json - approval_bare_yes_no_pending.json README updated to list the new fixtures. Phase 2 of #2828.
Five replay fixtures covering the engine v2 auth-gate state space: - auth_credential_provided: happy path (CredentialProvided -> resume) - auth_cancelled: user rejects (Cancelled -> resume) - auth_retry_invalid_then_valid: invalid credential, retry path - auth_external_callback: ExternalCallback submission path - auth_gate_request_id: AuthRequired populates request_id (v2 only) Probe tool: MockActivateTool (name "tool_activate") with scriptable output queue, installed via TestRegistry::replace_for_test to bypass PROTECTED_TOOL_NAMES. Planted minimal SKILL.md provides the credential spec needed by AuthManager's submit_auth_token path (otherwise the auth flow short-circuits with "Extension not installed"). Rig additions: - send_gate_auth_resolution(request_id, AuthGateResolution) - send_external_callback(request_id) - with_test_tool_override(tool) builder - TestChannel::channel_name / user_id accessors Serialization: all auth-gate tests share engine_v2_test_lock() (per-file static Mutex) because engine v2 uses a process-global OnceLock<RwLock<Option<EngineState>>>. Fixtures omit tools_used / all_tools_succeeded because engine v2 suppresses ToolStarted/ToolCompleted events when a tool output becomes a gate pause; verification uses the mock's internal execution counter instead.
…2828) Introduces Trace/TraceOperation/TraceExpectation types and TraceRunner that replays an ordered sequence of tool invocations against a libSQL test DB. The runner creates ActionRecords via the same save_action path gateway handlers use and matches outcomes against declared expectations. This is the inverse of the agentic TraceLlm harness: where TraceLlm replays an LLM stream and asserts the agent re-produces tool calls, TraceRunner replays caller-dispatched tool calls and asserts the Tool -> ActionRecord -> save_action pipeline matches expectations. Deliverables: - tests/support/trace_runner.rs: Trace, TraceOperation, TraceExpectation (Success { assertions } / Failure { error_contains }), TraceResult (with job_id for DB cross-checks), TraceFailure, TraceRunner with replay(). Assertion DSL supports eq / contains_text / fields (dot-path). - tests/e2e_gateway_trace_harness.rs: 7 integration tests covering echo roundtrip, idempotency, unknown-tool failure, mix assertions, forced mismatch detection, DB persistence via get_job_actions, and cross-run determinism. - tests/fixtures/gateway_traces/: 4 JSON fixtures + README documenting the wire format and the deferred settings_* / extension_* roadmap (blocked on #640 and network-stub work respectively). Pitfalls addressed: - Parent agent_jobs row is created via save_job before the first save_action; job_actions.job_id has a FK to agent_jobs(id) ON DELETE CASCADE that would otherwise fail. - Deterministic-field check in the determinism test excludes id / executed_at / duration (intentionally variable across replays). - ToolError has no NotFound variant; missing-tool lookups are reported via ExecutionFailed("tool not registered: {name}") so Failure expectations can substring-match on "not registered".
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive end-to-end testing for approval workflows and authentication gates. Key additions include a TraceRunner harness for replaying tool sequences, numerous test fixtures for LLM and gateway traces, and integration tests covering various auth and approval scenarios. The ToolRegistry was updated with a test-only method to override protected tools, and the test rig was enhanced with helpers for auth gate resolution. I have no feedback to provide.
henrypark133
left a comment
There was a problem hiding this comment.
What looks good:
- The new replay coverage is mostly caller-level and fixture-driven rather than helper-only, which is the right shape for approval/auth regressions.
- The gateway trace harness exercises the real
Tool -> ActionRecord -> save_actionpath against a migrated libSQL test database. - The new text-auth-fallback router tests cover both the positive registered-credential branch and the negative trust-check branches.
No verified findings.
Summary:
- Recommended verdict: Approve
- Prior feedback status: no prior review
- Review coverage: checked-out worktree plus harness/router/testing pass
- Residual risk: I did not run the full replay fixture matrix locally in this pass
zmanian
left a comment
There was a problem hiding this comment.
Reviewed the full diff. Strong PR overall — approving.
What works well:
- Router fallback tests test through the caller (
handle_with_engine_inner), not justparse_credential_name. Positive, unregistered-credential, and invalid-name branches are all covered, matching the IronClaw rule that gating predicates need call-site regression coverage. Thewith_installed_engine_statepanic-safe guard correctly restoresENGINE_STATEon unwind. - Approval replay suite exercises actual side effects via
AtomicUsizeexecution counters, not tautologies.always_requirement_ignores_allow_always_persistenceis a load-bearing test — it pins theApprovalRequirement::Alwayshard floor against a real foot-gun.bare_yes_with_no_pending_approval_is_user_inputcorrectly inspects captured LLM requests to prove the routing downgrade. - Auth-gate tests use typed
Submission::GateAuthResolution/ExternalCallbackrather than string parsing, which is the right abstraction level. TraceRunnercorrectly creates the parentagent_jobsrow to satisfy thejob_actionsFK, separatesTraceResult.failures(assertion-level) fromActionRecord.success(tool-level), and the determinism test strips only the documented variable fields (id,executed_at,duration).- Fixture README clearly differentiates
llm_traces/vsgateway_traces/intent and documents deferred work (#640 settings CRUD, extension lifecycle).
Minor notes (non-blocking):
wait_for_approval_neededpolls every 50ms up to 15s. Under a loaded CI runner this should be fine, but if these ever get flaky bump the deadline rather than the poll interval.unknown_tool_fails.jsonexpects substring\"tool not registered\"whileassertion_mix.jsonuses\"not registered\". Both match the runner's actual message (\"tool not registered: {name}\"), but pick one convention for consistency.TraceExpectation::Success.assertionssilently accepts unknown top-level keys as failures (good) but thefieldsdot-path walker can't express keys that contain a literal dot. Not a problem for current fixtures; worth a comment if you extend the DSL.CompletedTextLlmandNeedsApprovalProbe/AlwaysApprovalProbehave near-identical shapes; a small shared helper could de-duplicate, but duplication in tests is fine.
No blocking issues. Scope is tight, coverage is meaningful, no prod-side unwraps added.
* test(replay): add approval round-trip fixtures (Phase 2 of nearai#2828) First fixture-driven Layer 1 (replay) coverage of the full v1 approval cycle: pause -> user resolution -> resume. Companion to the existing no_done_emitted_while_awaiting_approval test in e2e_response_order.rs, which covers the pause but not the resume. Three scenarios: - approval_yes: user approves -> tool runs once -> final LLM response - approval_no: user denies -> tool does NOT run -> agent surfaces a built-in rejection message (no follow-up LLM call, by design) - approval_always: allow-always on first call -> second call runs without re-prompting, exactly one ApprovalNeeded total Uses a test-only NeedsApprovalProbe tool with ApprovalRequirement::UnlessAutoApproved registered via TestRig::with_extra_tools, with auto_approve_tools(false) so the agent actually pauses for resolution. The deny-path discovery (no LLM follow-up on rejection) is documented in the test so future readers don't reintroduce the trailing text step. Updates tests/fixtures/llm_traces/README.md to list the new fixtures. Bumps approvals coverage in the harness-testing matrix from ~ to (closer to) full at Layer 1. * test(replay): expand approval coverage with 4 missing scenarios Adds the four approval scenarios that the original three-test set omitted, completing the state-space matrix across ApprovalRequirement variants, the master kill-switch config, and submission-routing edge cases. New tests (all in tests/e2e_approval_traces.rs): - always_requirement_ignores_allow_always_persistence ApprovalRequirement::Always is the unbypassable hard floor — even an 'allow-always' resolution must NOT skip the pause on subsequent calls of an Always-tool. Two pauses for two calls. - slash_approve_routes_as_approval_response '/approve' is parsed as Submission::ApprovalResponse even though bare 'yes' downgrades to UserInput when nothing is pending. Pins the divergent routing in submission.rs. - bare_yes_with_no_pending_approval_is_user_input Bare 'yes' with no pending approval must downgrade to UserInput and reach the LLM as a normal user message. Asserts the routing layer in agent_loop.rs performs the downgrade (parser is stateless). - config_auto_approve_bypasses_unless_auto_approved Agent-config auto_approve_tools=true is the master kill-switch — no ApprovalNeeded is ever emitted, even for UnlessAutoApproved tools. Also adds AlwaysApprovalProbe (mirrors NeedsApprovalProbe but returns ApprovalRequirement::Always) and three fixtures: - approval_always_floor.json - approval_slash.json - approval_bare_yes_no_pending.json README updated to list the new fixtures. Phase 2 of nearai#2828. * test(replay): add auth-gate round-trip fixtures (Phase 2 of nearai#2828) Five replay fixtures covering the engine v2 auth-gate state space: - auth_credential_provided: happy path (CredentialProvided -> resume) - auth_cancelled: user rejects (Cancelled -> resume) - auth_retry_invalid_then_valid: invalid credential, retry path - auth_external_callback: ExternalCallback submission path - auth_gate_request_id: AuthRequired populates request_id (v2 only) Probe tool: MockActivateTool (name "tool_activate") with scriptable output queue, installed via TestRegistry::replace_for_test to bypass PROTECTED_TOOL_NAMES. Planted minimal SKILL.md provides the credential spec needed by AuthManager's submit_auth_token path (otherwise the auth flow short-circuits with "Extension not installed"). Rig additions: - send_gate_auth_resolution(request_id, AuthGateResolution) - send_external_callback(request_id) - with_test_tool_override(tool) builder - TestChannel::channel_name / user_id accessors Serialization: all auth-gate tests share engine_v2_test_lock() (per-file static Mutex) because engine v2 uses a process-global OnceLock<RwLock<Option<EngineState>>>. Fixtures omit tools_used / all_tools_succeeded because engine v2 suppresses ToolStarted/ToolCompleted events when a tool output becomes a gate pause; verification uses the mock's internal execution counter instead. * test(router): cover auth fallback caller path (Phase 2 of nearai#2828) * test(harness): add gateway-ops trace replay runner (nearai#643, Phase 2 of nearai#2828) Introduces Trace/TraceOperation/TraceExpectation types and TraceRunner that replays an ordered sequence of tool invocations against a libSQL test DB. The runner creates ActionRecords via the same save_action path gateway handlers use and matches outcomes against declared expectations. This is the inverse of the agentic TraceLlm harness: where TraceLlm replays an LLM stream and asserts the agent re-produces tool calls, TraceRunner replays caller-dispatched tool calls and asserts the Tool -> ActionRecord -> save_action pipeline matches expectations. Deliverables: - tests/support/trace_runner.rs: Trace, TraceOperation, TraceExpectation (Success { assertions } / Failure { error_contains }), TraceResult (with job_id for DB cross-checks), TraceFailure, TraceRunner with replay(). Assertion DSL supports eq / contains_text / fields (dot-path). - tests/e2e_gateway_trace_harness.rs: 7 integration tests covering echo roundtrip, idempotency, unknown-tool failure, mix assertions, forced mismatch detection, DB persistence via get_job_actions, and cross-run determinism. - tests/fixtures/gateway_traces/: 4 JSON fixtures + README documenting the wire format and the deferred settings_* / extension_* roadmap (blocked on nearai#640 and network-stub work respectively). Pitfalls addressed: - Parent agent_jobs row is created via save_job before the first save_action; job_actions.job_id has a FK to agent_jobs(id) ON DELETE CASCADE that would otherwise fail. - Deterministic-field check in the determinism test excludes id / executed_at / duration (intentionally variable across replays). - ToolError has no NotFound variant; missing-tool lookups are reported via ExecutionFailed("tool not registered: {name}") so Failure expectations can substring-match on "not registered". * fix: address review findings (iteration 1)
Summary
Included in this PR
Approval replay fixtures
Auth-gate replay fixtures
Gateway trace replay harness
tests/support/trace_runner.rstests/e2e_gateway_trace_harness.rstests/fixtures/gateway_traces/Engine-v2 caller-level runtime-boundary coverage
handle_with_engine_inner()text-based auth fallbackBridgeOutcome::Pending+StatusUpdate::AuthRequiredRefs #2828
Refs #643
Testing
cargo test --features libsql --no-default-features --test e2e_approval_tracescargo test --features libsql --no-default-features --test e2e_auth_gate_tracescargo test --features libsql --no-default-features --test e2e_gateway_trace_harnesscargo test --features libsql --no-default-features --lib handle_with_engine_text_auth_fallback_cargo fmt --checkcargo clippy --features libsql --no-default-features --lib --test e2e_approval_traces --test e2e_auth_gate_traces --test e2e_gateway_trace_harness