[guoqunabc]

Summary

Expand the rate_limit_cascade test fixture from 2 to 11 consecutive stub_rate_limit tool calls, enough to exceed the 10-call threshold referenced in the stuck-detection logic. This addresses the gap identified in PR #593 review (comment IDs 2893751216, 2893861186).

Changes

Fixture (rate_limit_cascade.json)

• Expanded from 2 → 11 consecutive stub_rate_limit tool call steps
• Final step remains a text response so the agent can produce a closing message

Test (e2e_worker_coverage.rs)

• Set max_tool_iterations(14) so all 11 tool calls can execute before the iteration limit forces a text response
• Assert >= 10 rate-limited tool calls occurred (previously only asserted non-empty)
• Increased timeout from 15s → 30s to accommodate the longer trace
• Added assertion that the agent still produces a response after the cascade
• Added doc comments referencing test: rate_limit_cascade should hit consecutive-rate-limit threshold #605 and PR test: add 29 E2E trace tests for issues #571-575 #593

What this exercises

The test now verifies that the agent can handle a sustained cascade of rate-limited tool failures (11 consecutive) without panicking or losing the ability to produce a final response. Previously only 2 calls were made, which did not stress the cascade/stuck-detection code path.

Closes #605

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the robustness of the agent's rate-limiting stuck-detection mechanism by significantly expanding an existing end-to-end test. It ensures that the agent can gracefully handle a prolonged sequence of rate-limited tool failures without crashing or failing to provide a final user response, thereby validating its ability to recover from such scenarios.

Highlights

• Test Fixture Expansion: The rate_limit_cascade.json fixture was expanded from 2 to 11 consecutive stub_rate_limit tool calls to exceed the 10-call stuck-detection threshold.
• Test Configuration Update: The e2e_worker_coverage.rs test now sets max_tool_iterations to 14 and increases the timeout to 30 seconds to accommodate the extended trace.
• Improved Assertions: New assertions were added to verify that at least 10 rate-limited tool calls occurred and that the agent still produces a final response after the cascade.
• Documentation: Doc comments were added to the test, referencing the relevant issue and previous PR review comments.

Changelog

• tests/e2e_worker_coverage.rs
  • Added detailed documentation comments explaining the test's purpose and context.
  • Increased the maximum tool iterations to 14 to allow for more tool call attempts.
  • Extended the test timeout duration from 15 to 30 seconds.
  • Modified the assertion for rate-limited calls to check for at least 10 occurrences.
  • Introduced an assertion to confirm the agent still provides a response after the rate-limit cascade.
• tests/fixtures/llm_traces/worker/rate_limit_cascade.json
  • Expanded the sequence of stub_rate_limit tool calls from 2 to 11.
  • Updated the final text response to reflect the extended rate-limiting scenario.

Activity

• Addressed feedback from PR test: add 29 E2E trace tests for issues #571-575 #593 review comments regarding the inadequacy of the previous rate-limit cascade test.
• Closed issue test: rate_limit_cascade should hit consecutive-rate-limit threshold #605, which likely tracked the need for this expanded test coverage.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request expands the rate_limit_cascade test to better stress the agent's ability to handle consecutive tool failures. The changes increase the number of simulated rate-limited tool calls from 2 to 11 and adjust the test assertions and configuration accordingly. The changes are a good improvement to the test suite. I have one point of feedback regarding the "10-call threshold" mentioned in the comments and PR description, as it seems the test may not be exercising the intended logic in worker.rs.

[gemini-code-assist]

The comment states this test exceeds a '10-call threshold' for stuck-detection. However, the MAX_CONSECUTIVE_RATE_LIMITS constant in src/agent/worker.rs applies to consecutive LLM rate-limit errors (LlmError::RateLimited), not tool errors (ToolError::RateLimited) as produced in this test. The current implementation does not seem to have a counter for consecutive tool failures.

While this test effectively verifies agent resilience against a cascade of tool failures, it does not appear to test the specific 10-call stuck-detection threshold mentioned. Please clarify if there's another threshold being tested, or adjust the comment to reflect what is being tested (resilience to repeated tool failures up to the iteration limit).

[zmanian]

Review: test: expand rate_limit_cascade to hit consecutive-rate-limit threshold (#605)

Overall

The test changes are well-structured and the fixture expansion is clean. The increased timeout and iteration limit are appropriate. However, there is a significant conceptual mismatch between what the PR claims to test and what it actually exercises.

Issue: Wrong rate-limit code path

The PR description and comments say this test "exercises the stuck-detection path" by "exceeding the 10-call threshold" from MAX_CONSECUTIVE_RATE_LIMITS in worker.rs:307. However, that threshold applies to LLM-level rate limits (LlmError::RateLimited from select_tools() / respond_with_tools()), not tool-level rate limits.

StubRateLimitTool returns ToolError::RateLimited, which is a tool execution failure. The worker handles this as a failed tool call result fed back to the LLM, which then decides what to do next (in this case, the fixture provides the next trace entry). The consecutive rate-limit counter in worker.rs:431 and worker.rs:461 is never incremented because the replayed LLM never returns LlmError::RateLimited.

The mark_stuck("Persistent rate limiting") path at worker.rs:440 is not exercised by this test.

What this test actually verifies: The agent can handle 11 consecutive tool-level ToolError::RateLimited failures without panicking and still produce a final text response. That is still useful coverage, but the PR description should be corrected to reflect reality.

Minor: Redundant assertion

assert!(
    !responses.is_empty(),
    "Agent should produce a response even after rate-limit cascade"
);

This is redundant. wait_for_responses(1, Duration::from_secs(30)) already blocks until at least 1 response is captured -- if none arrived, the test would have timed out and panicked before reaching this assertion. Not a blocker, but it adds noise.

Suggestion

To actually test the MAX_CONSECUTIVE_RATE_LIMITS stuck-detection path, the test rig would need an InstrumentedLlm that returns LlmError::RateLimited from its chat/tool-selection methods (simulating provider-level throttling), rather than a tool that fails with ToolError::RateLimited. That is a different (and likely more involved) test -- but it would match what the PR claims.

Verdict

The code itself is correct and non-flaky. The gap is in accuracy of documentation/description vs actual behavior. I'd suggest either:

1. Updating the PR description and doc comments to accurately describe what is being tested (tool-level rate-limit resilience, not LLM-level stuck detection), or
2. Adding a separate test that actually exercises the MAX_CONSECUTIVE_RATE_LIMITS / mark_stuck path.

[zmanian]

Review: test/605-rate-limit-cascade-threshold

The test changes are well-structured and follow existing patterns, but there is a significant accuracy issue with what this test actually exercises.

Issue: Test does not hit the claimed code path

The PR title and description claim this test hits the "consecutive-rate-limit threshold" from JobDelegate::MAX_CONSECUTIVE_RATE_LIMITS (the = 10 constant at src/worker/job.rs:1008). However, that counter tracks LLM-level rate limits -- it increments when the LLM provider itself returns LlmError::RateLimited during select_tools() or respond_with_tools().

The StubRateLimitTool returns ToolError::RateLimited, which is a tool-level error. This error gets formatted as a regular error message fed back to the LLM via process_tool_result(). It never touches the consecutive_rate_limits atomic counter in JobDelegate. Since the LLM is a trace-replay mock that always succeeds, the consecutive_rate_limits counter stays at 0 throughout the test.

What the test actually exercises: the agent's resilience to many consecutive tool-execution failures (all returning errors), verifying it still produces a final text response. This is useful, but it is not the stuck-detection / cascade-threshold path described in the PR.

To fix

Either:

1. Update the PR description and test comments to accurately describe what is being tested (consecutive tool failures, not LLM rate-limit cascade detection), and remove the reference to the 10-call threshold since it's not exercised here, OR
2. Write a test that actually exercises MAX_CONSECUTIVE_RATE_LIMITS by using a mock LLM that returns LlmError::RateLimited for 10+ consecutive calls, which would trigger the handle_rate_limit -> mark_failed path.

Other observations (minor, non-blocking)

• The test mechanics themselves are fine: with_max_tool_iterations(14), the 30s timeout, the >= 10 assertion, and the fixture expansion all make sense for what the test actually does.
• .unwrap() usage is appropriate for tests.
• No flaky timing concerns -- the trace replay is deterministic.
• Follows existing test patterns correctly.