feat: Fix A2A push-config race in eval client (flaky test_set_push_config_round_trips)

[mabry1985]

Summary

Fix A2A push-config race in eval client (flaky test_set_push_config_round_trips) (bd-1kg)

See the diff for details.

Fixes #2230

[coderabbitai]

Warning

Review limit reached

@mabry1985, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 14 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 8a5bc6e2-6b47-470e-9424-ea3d9247a5f0

📥 Commits

Reviewing files that changed from the base of the PR and between 21c19d4 and 48c6965.

⛔ Files ignored due to path filters (1)

• CHANGELOG.md is excluded by !*.md

📒 Files selected for processing (2)

• evals/client.py
• tests/test_eval_client_subscribe_push.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/bd-1kg

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[protoreview]

QA panel review — WARN

code-review-structural · head 48c696545808 · formal

[review-synthesizer completed: workflow code-review-structural:report]

Brief

Overall risk: Low. Both findings are convention/style issues in test infrastructure and docstring cross-references — no correctness, security, or removed-behavior defects. The PR's functional changes are not affected.

Fix first: The test test_set_push_config_does_not_retry_other_rpc_errors should use the shared _scripted_push_app helper instead of inlining its own FastAPI app. This is the more actionable finding (minor vs. nit) and maintains consistency across the three new tests.

Panel disagreements: None. Both findings were confirmed by the verifier with no disputes.

Verification changes: The verifier confirmed both findings against the diff. No findings were refuted. The panel's assessment stands unchanged from the prior round (this is the first review pass).

Gaps: No structural pass gaps identified. The verifier completed the structural verification workflow without noting omissions.

[
  {
    "file": "tests/test_eval_client_subscribe_push.py",
    "line": 0,
    "severity": "minor",
    "category": "conventions",
    "claim": "The test `test_set_push_config_does_not_retry_other_rpc_errors` inlines its own FastAPI app instead of using the shared `_scripted_push_app` helper that the other two new tests use, breaking the test-file convention of a single scripted-app factory.",
    "evidence": "The diff adds three new tests. The first two (`test_set_push_config_retries_once_when_task_is_not_yet_visible`, `test_set_push_config_gives_up_after_bounded_retries`) both call `_scripted_push_app(fail_first_n=...)`. The third (`test_set_push_config_does_not_retry_other_rpc_errors`) creates a fresh `FastAPI()` and `@app.post(\"/a2a\")` inline — the only new test that doesn't use the helper. The helper exists precisely to avoid this duplication.",
    "verdict": "confirmed",
    "note": "Diff clearly shows: tests 1 and 2 call `_scripted_push_app(fail_first_n=...)`; test 3 inlines `app = FastAPI()` + `@app.post(\"/a2a\")` — helper is unused by this test."
  },
  {
    "file": "evals/client.py",
    "line": 426,
    "severity": "nit",
    "category": "conventions",
    "claim": "The `_rpc_until_task_visible` docstring cross-references `#2230` (the issue number) rather than a stable identifier, which will rot once the issue is closed/archived — convention is to use a stable reference (method name, docstring tag) for internal cross-links.",
    "evidence": "Docstring line: `\"\"\"``_rpc``, absorbing the task-registration race (#2230).` — the `#2230` is an issue number, not a method/class name. The surrounding code uses method names for internal cross-references (e.g. `\"\"\"``_rpc``, absorbing...`).",
    "verdict": "confirmed",
    "note": "Docstring literally contains `(#2230)`; surrounding docstrings use method-name/code references like `\"\"\"``GetTaskPushNotificationConfig`` → the stored config.` — no issue numbers elsewhere."
  }
]

[mabry1985]

Operator adjudication of the WARN (operator-ruled; gateway flaking blocks the brain): both findings dismissed with reasons. (1) Inline-app test vs shared helper — real style point, but the test intentionally scripts a DIFFERENT error class than the helper's happy-path script; consolidation is cosmetic and not worth a fix round while this flake blocks every merge in the repo. (2) Docstring citing #2230 — that IS this codebase's convention (issue refs pervade comments: #1953, #2162, #92 — they're the archaeology trail, not rot).