fix: staging CI triage — consolidate retry parsing, fix flaky tests, add docs - #1427
Conversation
…1288, #1280) - Extract shared `parse_retry_after()` into `src/llm/retry.rs` supporting both delay-seconds and RFC2822 formats, replacing duplicated inline parsing in anthropic_oauth.rs, nearai_chat.rs, and embeddings.rs - Fix flaky `bind_rejects_wildcard_*` tests in oauth_helpers.rs by adding `tokio::sync::Mutex` to serialize env var access (matching the ENV_MUTEX pattern in oauth_defaults.rs) - Add regression tests for parse_retry_after edge cases Closes #1288, #1280 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! 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 addresses several issues identified during a recent staging CI review triage. It focuses on improving code robustness and maintainability by centralizing Highlights
Using Gemini Code AssistThe 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
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 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the Retry-After header parsing logic into a new shared utility function, parse_retry_after, in src/llm/retry.rs, which is then used across anthropic_oauth.rs, nearai_chat.rs, and workspace/embeddings.rs to reduce code duplication. It also improves test stability in oauth_helpers.rs by serializing environment-mutating tests with a tokio::sync::Mutex and ensuring proper environment variable restoration. Additionally, #[cfg(test)] attributes were added to cap_retry_after imports to restrict them to test builds, and new unit tests were added for the parse_retry_after function. A new documentation file for CI triage was added, and comments were updated in src/service.rs to clarify daemon mode configuration. Review comments suggest improving readability by moving #[cfg(test)] attributes above use statements and simplifying code by leveraging the parse_retry_after function's built-in handling of missing or unparseable headers.
There was a problem hiding this comment.
Pull request overview
This PR addresses several staging CI triage items by consolidating duplicated Retry-After parsing logic, hardening env-var-sensitive OAuth tests against parallel execution races, and documenting why the interactive CLI is disabled in daemonized service templates.
Changes:
- Extract shared
parse_retry_after()intosrc/llm/retry.rsand switch multiple providers to use it. - Serialize env-mutating OAuth callback tests to reduce CI flakiness.
- Add inline comments in launchd/systemd templates explaining
CLI_ENABLED=falsein daemon mode.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/llm/retry.rs |
Adds shared Retry-After parsing helper and unit tests. |
src/workspace/embeddings.rs |
Uses shared retry-after parsing for embedding providers. |
src/llm/nearai_chat.rs |
Replaces inline retry-after parsing with shared helper. |
src/llm/anthropic_oauth.rs |
Replaces inline retry-after parsing with shared helper. |
src/llm/oauth_helpers.rs |
Adds a mutex to serialize env-mutating wildcard bind tests. |
src/service.rs |
Documents why CLI_ENABLED=false is set in daemon service definitions. |
docs/plans/2026-03-18-staging-ci-triage.md |
Adds triage plan/notes documenting the staging CI review cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Change parse_retry_after() return type from Option<Duration> to Duration (it never returns None due to the 60s fallback) - Fix doc comment: reference RFC 7231 §7.1.1 for HTTP-date, not RFC 2822 - Add parse_retry_after_http_date test for the RFC 2822 date parsing branch - Remove stale per-file test helpers (parse_retry_after_*_for_test) that duplicated old inline logic instead of testing the shared function - Remove unnecessary comments above #[cfg(test)] imports - Use crate-wide ENV_MUTEX instead of local tokio::sync::Mutex in oauth_helpers tests to prevent cross-module env-var races Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Drop runtime-flavor assumption; justify by short-lived awaited operation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…add docs (#1427) * fix: consolidate retry-after parsing and fix flaky OAuth env tests (#1288, #1280) - Extract shared `parse_retry_after()` into `src/llm/retry.rs` supporting both delay-seconds and RFC2822 formats, replacing duplicated inline parsing in anthropic_oauth.rs, nearai_chat.rs, and embeddings.rs - Fix flaky `bind_rejects_wildcard_*` tests in oauth_helpers.rs by adding `tokio::sync::Mutex` to serialize env var access (matching the ENV_MUTEX pattern in oauth_defaults.rs) - Add regression tests for parse_retry_after edge cases Closes #1288, #1280 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: add comments explaining CLI_ENABLED=false in service templates (#990) Clarify that CLI_ENABLED=false is needed in daemon mode (launchd/systemd) to prevent blocking on stdin when running as a background service. Closes #990 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address review comments on retry-after consolidation - Change parse_retry_after() return type from Option<Duration> to Duration (it never returns None due to the 60s fallback) - Fix doc comment: reference RFC 7231 §7.1.1 for HTTP-date, not RFC 2822 - Add parse_retry_after_http_date test for the RFC 2822 date parsing branch - Remove stale per-file test helpers (parse_retry_after_*_for_test) that duplicated old inline logic instead of testing the shared function - Remove unnecessary comments above #[cfg(test)] imports - Use crate-wide ENV_MUTEX instead of local tokio::sync::Mutex in oauth_helpers tests to prevent cross-module env-var races Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: reword await_holding_lock safety comment Drop runtime-flavor assumption; justify by short-lived awaited operation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…add docs (#1427) * fix: consolidate retry-after parsing and fix flaky OAuth env tests (#1288, #1280) - Extract shared `parse_retry_after()` into `src/llm/retry.rs` supporting both delay-seconds and RFC2822 formats, replacing duplicated inline parsing in anthropic_oauth.rs, nearai_chat.rs, and embeddings.rs - Fix flaky `bind_rejects_wildcard_*` tests in oauth_helpers.rs by adding `tokio::sync::Mutex` to serialize env var access (matching the ENV_MUTEX pattern in oauth_defaults.rs) - Add regression tests for parse_retry_after edge cases Closes #1288, #1280 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: add comments explaining CLI_ENABLED=false in service templates (#990) Clarify that CLI_ENABLED=false is needed in daemon mode (launchd/systemd) to prevent blocking on stdin when running as a background service. Closes #990 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address review comments on retry-after consolidation - Change parse_retry_after() return type from Option<Duration> to Duration (it never returns None due to the 60s fallback) - Fix doc comment: reference RFC 7231 §7.1.1 for HTTP-date, not RFC 2822 - Add parse_retry_after_http_date test for the RFC 2822 date parsing branch - Remove stale per-file test helpers (parse_retry_after_*_for_test) that duplicated old inline logic instead of testing the shared function - Remove unnecessary comments above #[cfg(test)] imports - Use crate-wide ENV_MUTEX instead of local tokio::sync::Mutex in oauth_helpers tests to prevent cross-module env-var races Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: reword await_holding_lock safety comment Drop runtime-flavor assumption; justify by short-lived awaited operation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…add docs (nearai#1427) * fix: consolidate retry-after parsing and fix flaky OAuth env tests (nearai#1288, nearai#1280) - Extract shared `parse_retry_after()` into `src/llm/retry.rs` supporting both delay-seconds and RFC2822 formats, replacing duplicated inline parsing in anthropic_oauth.rs, nearai_chat.rs, and embeddings.rs - Fix flaky `bind_rejects_wildcard_*` tests in oauth_helpers.rs by adding `tokio::sync::Mutex` to serialize env var access (matching the ENV_MUTEX pattern in oauth_defaults.rs) - Add regression tests for parse_retry_after edge cases Closes nearai#1288, nearai#1280 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: add comments explaining CLI_ENABLED=false in service templates (nearai#990) Clarify that CLI_ENABLED=false is needed in daemon mode (launchd/systemd) to prevent blocking on stdin when running as a background service. Closes nearai#990 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address review comments on retry-after consolidation - Change parse_retry_after() return type from Option<Duration> to Duration (it never returns None due to the 60s fallback) - Fix doc comment: reference RFC 7231 §7.1.1 for HTTP-date, not RFC 2822 - Add parse_retry_after_http_date test for the RFC 2822 date parsing branch - Remove stale per-file test helpers (parse_retry_after_*_for_test) that duplicated old inline logic instead of testing the shared function - Remove unnecessary comments above #[cfg(test)] imports - Use crate-wide ENV_MUTEX instead of local tokio::sync::Mutex in oauth_helpers tests to prevent cross-module env-var races Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: reword await_holding_lock safety comment Drop runtime-flavor assumption; justify by short-lived awaited operation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…add docs (nearai#1427) * fix: consolidate retry-after parsing and fix flaky OAuth env tests (nearai#1288, nearai#1280) - Extract shared `parse_retry_after()` into `src/llm/retry.rs` supporting both delay-seconds and RFC2822 formats, replacing duplicated inline parsing in anthropic_oauth.rs, nearai_chat.rs, and embeddings.rs - Fix flaky `bind_rejects_wildcard_*` tests in oauth_helpers.rs by adding `tokio::sync::Mutex` to serialize env var access (matching the ENV_MUTEX pattern in oauth_defaults.rs) - Add regression tests for parse_retry_after edge cases Closes nearai#1288, nearai#1280 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: add comments explaining CLI_ENABLED=false in service templates (nearai#990) Clarify that CLI_ENABLED=false is needed in daemon mode (launchd/systemd) to prevent blocking on stdin when running as a background service. Closes nearai#990 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: address review comments on retry-after consolidation - Change parse_retry_after() return type from Option<Duration> to Duration (it never returns None due to the 60s fallback) - Fix doc comment: reference RFC 7231 §7.1.1 for HTTP-date, not RFC 2822 - Add parse_retry_after_http_date test for the RFC 2822 date parsing branch - Remove stale per-file test helpers (parse_retry_after_*_for_test) that duplicated old inline logic instead of testing the shared function - Remove unnecessary comments above #[cfg(test)] imports - Use crate-wide ENV_MUTEX instead of local tokio::sync::Mutex in oauth_helpers tests to prevent cross-module env-var races Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: reword await_holding_lock safety comment Drop runtime-flavor assumption; justify by short-lived awaited operation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Fixes from staging CI review triage (batch processing of 50 open
staging-ci-reviewissues):parse_retry_after()intosrc/llm/retry.rssupporting both delay-seconds and RFC2822 formats, replacing duplicated inline parsing inanthropic_oauth.rs,nearai_chat.rs, andembeddings.rstokio::sync::Mutextobind_rejects_wildcard_*tests inoauth_helpers.rsto serialize env var access, matching theENV_MUTEXpattern inoauth_defaults.rsTriage summary
Of the 50 open staging-ci-review issues:
Test plan
cargo checkpassescargo clippy --all --all-features— zero warningsparse_retry_after_*unit tests passbind_rejects_wildcard_*tests pass with mutex serialization🤖 Generated with Claude Code