refactor(agent-core): one Retry-After parser, reset-grammar table, token estimator, head/tail truncator and NO_PROXY matcher; LLM traffic honours CIDR/wildcard NO_PROXY - #109539
Conversation
૮ >ﻌ< ა ci reviewran on 5d29daa — fix(agent): explicit "retry after N s" wins over "resets in
|
gaoanze888
left a comment
gaoanze888
left a comment
There was a problem hiding this comment.
The consolidation is mostly coherent, but two behavior regressions remain at exact head 082ccf1f164513f155c799da18bed90cf0f6291e:
skills_hub_clawhubusesparse_retry_after_seconds(...) or 5, so validRetry-After: 0becomes a five-second delay. DistinguishNonefrom0.0and add a call-site zero-value test.- The NO_PROXY target parser can throw on malformed provider URLs: accessing
urlsplit(...).portraises forhttp://example.com:bad/v1, and malformed IPv6 raises during split.process_bootstrapdoes not catch this, changing the old fail-safe behavior from “keep the proxy” to client-construction crash. Treat unparsable target/entries as non-matches and test malformed port/IPv6.
Please also lock intentional behavior for wildcard apex matching: *.example.com now excludes only subdomains whereas the old helper included the apex. The current parity test can pass port cases merely because the host string contains :, so add real IPv6/port mismatch/case/malformed coverage. truncate_head_tail(max_chars=0) also returns full text due to text[-0:]; reject or handle non-positive limits.
Diff and compile checks are clean.
alt-glitch
added
type/refactor
…eed every retry wait
Seven sites hand-rolled `float(headers.get("Retry-After"))` (anon_auth,
shared_metrics_sender, gemini_native_adapter, extract_api_error_context,
nous_rate_guard, skills_hub_github, skills_hub_clawhub x2) and silently
dropped RFC 7231 HTTP-date values that the conversation loop already honours
via agent/retry_utils.py::parse_retry_after_seconds. They now call it; per-site
caps/floors stay at the call site.
The free-text "resets in / quotaResetDelay / retry after N s" regexes lived in
two tables (agent_runtime_helpers vs credential_pool) whose "resets in"
grammars diverged: the pool accepted only integer `Nhr Nmin` while the error
context accepted h/hr/hours + m/min/minutes + s/seconds with decimals. One table
(agent/retry_utils.py::RETRY_DELAY_PATTERNS / reset_delay_from_message) using
the wider grammar, so a pooled credential's cooldown and the UI's reset time
now agree.
…se the canonical token estimator context_breakdown._chars_to_tokens and native_compaction._approx_tokens did raw chars//4, under-counting CJK/Cyrillic by 2-4x next to the conversation slice that already used estimate_tokens_rough — the /context pie chart mixed two estimators. Both now call the canonical. The four private `= 4` ratio constants import one CHARS_PER_TOKEN from agent/model_metadata.py. Estimates only feed UI and budgets; no prompt or message bytes change.
…r truncate through one head/tail helper Four copies of the 40/60 head/tail algorithm with a near-identical notice (terminal_tool_result, mcp_tool_content, code_execution_tool, environments/base_output) collapse into tools/tool_output_truncate.py, so the ratio and the `... [<LABEL> TRUNCATED - N <unit> omitted out of T total] ...` marker are defined once. execute_code keeps byte mode + spill path and only shares the notice/split. Visible change: the terminal notice now uses thousands separators like the other three (`9,000 chars` not `9000 chars`). kanban_specify._truncate: comment claimed escape stripping the body never did; comment now says what the plain clamp is for.
…ke the platform adapters do Three answers to "is this host in NO_PROXY": process_bootstrap used the stdlib proxy_bypass_environment (no CIDR, no `*.`), gateway/platforms/base.py had a full matcher (should_bypass_proxy) and a second suffix-only one (is_host_excluded_by_no_proxy, used by Slack). Live-verified: with NO_PROXY=10.0.0.0/8 Telegram bypassed the proxy while the LLM call to a 10.x endpoint went through it. The full matcher moves to the leaf module agent/proxy_bypass.py (stdlib only, importable at early boot); both base.py functions are one-line forwarders and process_bootstrap._get_proxy_for_base_url uses it (passing host:port so port-qualified entries match). The six-key proxy env scan is also shared.
082ccf1 to
72932a1
Compare
The rebase resolution used 'parsed or 5', which turned a legitimate 0 s (negative headers clamp to 0) into the 5 s default; the pre-refactor code kept it. Test None explicitly.
gaoanze888
left a comment
gaoanze888
left a comment
There was a problem hiding this comment.
The Retry-After: 0 call-site bug is fixed at 55e0cbd9fdb984071e9698ef045703c7b7d60a58, and the focused shared suites pass 20/20. The other two concrete regressions from my review remain unchanged and are production-reachable:
split_host_port("http://example.com:bad/v1") -> ValueError: Port could not be cast...
split_host_port("http://[::1/v1") -> ValueError: Invalid IPv6 URL
truncate_head_tail("abc", 0) -> notice + "abc" (keeps the full text)
truncate_head_tail("abc", -1) -> impossible "4 chars omitted out of 3" + "bc"
process_bootstrap._get_proxy_for_base_url() does not catch the URL exceptions, so malformed configured provider URLs now crash proxy setup instead of retaining the proxy/failing safely at normal endpoint validation. Please make parsing total (invalid target/entry => non-match), test malformed port/brackets through the actual LLM call site, and reject or define non-positive truncation budgets. Also add explicit wildcard-apex and real IPv6/port mismatch assertions; the current or ":" in host assertion does not verify adapter parity for port entries.
The shared matcher took the gateway adapter's `*.` branch, which only matched subdomains. The adapter's own `is_host_excluded_by_no_proxy` docstring promised "leading-dot and `*.` entries match the apex domain and subdomains" (the curl/requests convention), so `NO_PROXY=*.slack.com` silently stopped covering `slack.com`. `*.` and `.` entries now share one apex+subdomain rule. Review follow-up on #109539.
…e transport `_get_proxy_for_base_url` lost its guard when it moved onto the shared matcher: `split_host_port` read `urlsplit(...).port`, which raises ValueError for `http://host:notaport/v1` or `:99999`, and `build_keepalive_http_client`'s outer except then returned None -- the client silently lost the shared pool instead of merely skipping the bypass check. The port parse now yields `(host, None)` on ValueError only; the host still matches NO_PROXY entries. Review follow-up on #109539.
…he shared reset table Unifying the credential pool's `_RETRY_DELAY_PATTERNS` into `RETRY_DELAY_PATTERNS` flipped the pool's precedence: "retry after 30s; resets in 4hr" cooled the credential for 14400 s where the pool used to take 30. A body carrying both describes a short throttle inside a long quota window; the explicit retry-after is the wait the provider actually asks for, so it is tried before "resets in". Review follow-up on #109539.
gaoanze888
left a comment
gaoanze888
left a comment
There was a problem hiding this comment.
Incremental recheck at 5d29daa1a7d77ba4d457962fb1d5592f6f58cae3: malformed/non-numeric and out-of-range URL ports are now total, wildcard apex semantics are pinned, and the shared retry parser precedence is clearer. One URL parser case and the truncation contract from the prior review remain:
split_host_port("http://[::1/v1") -> ValueError: Invalid IPv6 URL
truncate_head_tail("abc", 0) -> truncation notice + full "abc"
truncate_head_tail("abc", -1) -> "4 chars omitted out of 3" + "bc"
The try begins after urlsplit(raw), so malformed bracketed IPv6 still escapes before .port is read and remains reachable through _get_proxy_for_base_url(). Wrap URL splitting and hostname/port extraction as one total parse; invalid target/NO_PROXY token should be a non-match that retains the proxy. Add the malformed-bracket case through the process-bootstrap call site, plus a bracketed valid IPv6 port-match/mismatch case.
Please also reject non-positive max_chars or define it as an empty bounded result before slicing; the current output violates both size and metadata invariants. Existing focused proxy/retry changes otherwise look good.
The shared matcher took the gateway adapter's `*.` branch, which only matched subdomains. The adapter's own `is_host_excluded_by_no_proxy` docstring promised "leading-dot and `*.` entries match the apex domain and subdomains" (the curl/requests convention), so `NO_PROXY=*.slack.com` silently stopped covering `slack.com`. `*.` and `.` entries now share one apex+subdomain rule. Review follow-up on #109539.
…e transport `_get_proxy_for_base_url` lost its guard when it moved onto the shared matcher: `split_host_port` read `urlsplit(...).port`, which raises ValueError for `http://host:notaport/v1` or `:99999`, and `build_keepalive_http_client`'s outer except then returned None -- the client silently lost the shared pool instead of merely skipping the bypass check. The port parse now yields `(host, None)` on ValueError only; the host still matches NO_PROXY entries. Review follow-up on #109539.
3 participants
Retry-After parsing, free-text reset grammars, rough token estimation, head/tail tool-output truncation and NO_PROXY matching each have exactly one implementation now, and the LLM transport bypasses the proxy for CIDR /
*.NO_PROXY entries the way the platform adapters already did.Changes
float(headers.get("Retry-After"))sites callagent/retry_utils.py::parse_retry_after_seconds; HTTP-date headers (Cloudflare 5xx, OAuth endpoints) are honoured everywhere the conversation loop already honoured them. Per-site caps/floors (metrics 1–86400 s, GitHub 60 s, ClawHub 15 s) stay at the call site.agent_runtime_helpersvscredential_pool._RETRY_DELAY_PATTERNS) becomeagent/retry_utils.py::RETRY_DELAY_PATTERNS/reset_delay_from_message. The widerresets ingrammar wins (h/hr/hrs/hour/hours + m/min/minutes + s/seconds, decimals) — it parses every form the narrowerNhr Nmintable did plus"resets in 2 hours 5 minutes"/"resets in 45s".context_breakdown._chars_to_tokensandnative_compaction._approx_tokenscallestimate_tokens_rough; the four private= 4constants import oneCHARS_PER_TOKENfromagent/model_metadata.py. Estimates feed UI/budgets only — no prompt or message bytes change.tools/tool_output_truncate.py(40/60 split + one notice formatter); terminal, MCP, execute_code (byte mode + spill kept) and_BoundedOutputCollector.renderuse it. Web/browser/file_tools truncators are different products and untouched.hermes_cli/kanban_specify.py::_truncatecomment no longer claims escape stripping the body never did.agent/proxy_bypass.pyholds the full matcher (exact,.suffix,*.wildcard, IP literal, CIDR,host:port,*) and the six-key proxy env scan.gateway/platforms/base.py::should_bypass_proxy/is_host_excluded_by_no_proxyare one-line forwarders (only that region of base.py touched);agent/process_bootstrap._get_proxy_for_base_urldrops the stdlibproxy_bypass_environment.Sites
hermes_cli/anon_auth.py::_retry_after_secondsagent/retry_utils.py::parse_retry_after_secondshermes_cli/observability/shared_metrics_sender.py::_retry_after_secondsagent/gemini_native_adapter.py::gemini_http_error(inline)agent/agent_runtime_helpers.py::_set_reset_from_retry_afteragent/nous_rate_guard.py::_parse_reset_seconds(retry-after leg)tools/skills_hub_github.py::GitHubClient._github_get(inline)tools/skills_hub_clawhub.py(owner lookup + download, inline ×2)agent/agent_runtime_helpers.py::_reset_delay_from_message+ 3 regexesagent/retry_utils.py::reset_delay_from_message(deleted)agent/credential_pool.py::_RETRY_DELAY_PATTERNS/_extract_retry_delay_secondsagent/context_breakdown.py::_chars_to_tokens/_bytes_to_tokensagent/model_metadata.py::estimate_tokens_rough/CHARS_PER_TOKENagent/native_compaction.py::_approx_tokensestimate_tokens_roughtools/budget_config.py::_CHARS_PER_TOKEN,agent/prompt_builder.py::_CONTEXT_FILE_CHARS_PER_TOKEN(deleted),agent/context_compressor.py::_CHARS_PER_TOKEN,tools/transcription_command.py::_PROMPT_CHARS_PER_TOKENagent/model_metadata.py::CHARS_PER_TOKENtools/terminal_tool_result.py::_truncate_head_tailtools/tool_output_truncate.py::truncate_head_tailtools/mcp_tool_content.py::_truncate_mcp_text_resultlabel="MCP RESULT")tools/code_execution_tool.py::_truncate_stdout_texthead_tail_split+truncation_notice(unit="bytes")tools/environments/base_output.py::_BoundedOutputCollector.renderhead_tail_split+truncation_noticegateway/platforms/base.py::_split_host_port/_no_proxy_entries/_ip_or_none/_no_proxy_entry_matchesagent/proxy_bypass.py(moved)gateway/platforms/base.py::should_bypass_proxy,is_host_excluded_by_no_proxyagent/proxy_bypass.py::should_bypass_proxyagent/process_bootstrap.py::_get_proxy_for_base_url(stdlibproxy_bypass_environment)agent/proxy_bypass.py::should_bypass_proxyagent/process_bootstrap.py::_get_proxy_from_env+base.py::resolve_proxy_urlenv-key tuplesagent/proxy_bypass.py::first_proxy_env_valueBehavior change
10.0.0.0/8),*.internalwildcard andhost:portNO_PROXY entries; before, only exact/.suffixentries bypassed on that path (live-verified gap, adapters already bypassed).Retry-Afteris honoured at the 7 listed sites (previously fell back to each site's default wait)."resets in N hours M minutes"/"resets in Ns"(previously default cooldown)./contextbreakdown static categories and native-compaction retention count CJK/Cyrillic text with the byte/codepoint-corrected estimator (higher, matches the conversation slice).9,000 chars omitted out of 10,000 total) to match MCP/execute_code/collector; label/wording otherwise unchanged.Validation
NO_PROXY=10.0.0.0/8,build_keepalive_http_client("http://10.1.2.3:8000/v1")HTTPProxypoolConnectionPool(E2E, fresh interpreter, temp HERMES_HOME)extract_api_error_contextwithRetry-After: <HTTP-date +90s>reset_atreset_at ≈ now+90_normalize_error_context({"message": "resets in 2 hours 5 minutes"})reset_atreset_at ≈ now+7500_chars_to_tokens("Привет мир…"×50)vsestimate_tokens_roughTests (invariants, each sabotage-verified red by reverting one site's fix):
tests/agent/test_retry_delay_parsers_shared.py— HTTP-date header parsed identically at anon_auth / error-context / nous_rate_guard; pool and error-context agree on 5 reset grammars.tests/agent/test_token_estimator_shared.py— breakdown + retention == canonical on Cyrillic/CJK/ASCII; all ratio constants are the one object.tests/agent/test_proxy_bypass_shared.py— CIDR /*./.suffix/host:portbypass on LLM and adapter paths; non-matching host keeps the proxy on both.tests/tools/test_tool_output_truncate.py— 10k input at terminal / MCP / execute_code / collector: one notice, exact 40/60 head and tail, correct omitted/total/unit.scripts/run_tests.shovertests/agent tests/tools tests/test_retry_utils.py+ proxy/slack/commands/anon_auth/observability files: see report. ruff, windows-footguns, compat-pointers,git diff --checkclean.Not done:
hermes_cli/commands_platforms.py::_clamp_command_names—hermes_cli/commands.py's copy sits inside the PLUGIN-COMPAT block, andscripts/check_compat_pointers.pyfails CI on any in-tree import from it; the dedupe has to wait for the compat block's scheduled removal (or the helper's move to a non-compat module). Discord/Slack/Photon Retry-After sites are the adapters lane.Infographic
Review follow-ups
Fixes for the SHOULD-FIX findings of the independent review (each its own commit, each test proven red with the fix reverted):
*.example.comno longer matched the apexexample.com(the adapter docstring promised apex + subdomains)agent/proxy_bypass.py::no_proxy_entry_matches:*.and.entries share one apex+subdomain rule (curl/requests convention)tests/agent/test_proxy_bypass_shared.pyrows*.slack.com/.slack.com→slack.com;notslack.comstays proxied_get_proxy_for_base_urllost its guard:http://host:notaport/v1raisedValueErrorout ofurlsplit().portand the outerexceptinbuild_keepalive_http_clientsilently dropped the shared keepalive transportagent/proxy_bypass.py::split_host_portcatchesValueErrorfrom the port parse only →(host, None); the host still matches NO_PROXYtest_malformed_port_in_base_url_keeps_the_proxy_instead_of_raising(:notaport,:99999keep the proxy;NO_PROXY=hoststill bypasses)RETRY_DELAY_PATTERNSorder flipped the credential pool's precedence:retry after 30s; resets in 4hrcooled 14400 s instead of 30agent/retry_utils.py::RETRY_DELAY_PATTERNS: quotaResetDelay → explicit retry-after → resets-in (pool precedence restored; the shorter explicit wait is what the provider asks for)tests/agent/test_retry_delay_parsers_shared.pyrow"Rate limited. Retry after 30s; resets in 4hr"→ 30Additional declared behavior (unchanged by the follow-ups, was undeclared):
NO_PROXYandno_proxyare unioned rather than first-non-empty; trailing-dot hosts (example.com.) match;*./.alone match nothing (not everything);"resets in 1 hour and 30 minutes"parses as 3600 s (partial match) where the pool previously returned None.Tests:
scripts/run_tests.sh tests/agent/test_proxy_bypass_shared.py tests/agent/test_retry_delay_parsers_shared.py tests/run_agent/test_create_openai_client_proxy_env.py tests/gateway/test_proxy_mode.py tests/test_retry_utils.py tests/agent/test_credential_pool*.py tests/gateway/test_slack.py→ 22 files, 450 passed, 0 failed.