fix(agent): reword SKILLS_GUIDANCE trigger and stop mislabelling its 400 as billing - #82177
fix(agent): reword SKILLS_GUIDANCE trigger and stop mislabelling its 400 as billing#82177jackulau wants to merge 2 commits into
Conversation
|
Not a duplicate of #78025 — complementary, different failure mode:
Both can land independently. Rebased onto current |
1ffe2d2 to
fb6d4f2
Compare
|
suggesting changes The exact Anthropic 400 is still classified, persisted, and surfaced as billing; this patch only adds a caveat after that decision. I reproduced the exact 400 on current main and on this head replayed onto current main: both returned Please carry the ambiguity through the classifier, persisted credential state, fallback/terminal labels, and structured result, and add a regression for the returned terminal response. Otherwise a content-filter rejection still tells every downstream surface that billing is exhausted and benches the credential as such. Security evidence:
Not checked:
Signed: GPT-5.6-sol-xhigh in Codex |
…400 as billing On an Anthropic subscription OAuth credential, every request failed with HTTP 400 "You're out of extra usage. Add more at claude.ai/settings/usage". That is not a billing condition: Anthropic's server-side content filter rejects the first sentence of Hermes' own built-in SKILLS_GUIDANCE prompt, and the rejection is surfaced with a billing-shaped message. Because the message points at the usage settings page, it reliably sends people to buy quota they do not need — the reporter lost three debugging sessions to it. Bisected against the live API with the real 71,721-char assembled prompt: the first SKILLS_GUIDANCE sentence alone reproduces the 400 and removing it alone clears it. Size was ruled out (20 KB of unrelated filler returns 200) and so was the system[0] identity gate (that returns 429, a different failure). Three changes, all serving the same outcome — a subscription user can no longer be misdirected by this 400: - agent/prompt_builder.py: reword the triggering sentence to the phrasing the reporter verified returns 200. Meaning, the skill_manage reference, and the ## Skill Safety Rule block are all preserved. The reword is empirically validated rather than understood, so a comment records the bisect and warns that any rewrite must be re-verified against an OAuth token, not an API key. - agent/conversation_loop.py: the Anthropic branch of the billing guidance no longer asserts exhaustion as fact. It hedges the opening line, names the content-filter alternative, and gives the operator a way to tell the two apart (if the usage page still shows quota, suspect a content rejection). It also points at `hermes auth reset anthropic`, because the credential exhaustion latch replays the stored error for ~60 min without issuing a request — which makes a real fix look like it did not work. - hermes_cli/auth.py: document that CLAUDE_CODE_OAUTH_TOKEN is an OAuth token, not an API key, despite auth_type="api_key". It stays in api_key_env_vars because that tuple doubles as the credential-discovery list; removing it would stop Hermes finding a `claude setup-token` credential at all. Docs updated to match the reworded prompt. Fixes NousResearch#82154
…00 through classification, cooldown, and terminal surfaces
Review follow-up (egilewski): the previous commit only hedged the guidance
text; the exact Anthropic 400 was still classified, persisted, and surfaced
as confirmed billing exhaustion. Carry the ambiguity all the way through:
- agent/error_classifier.py: 'out of extra usage' matches on the 400 and
status-less paths now attach error_context {billing_unverified,
possible_content_filter}. Reason stays FailoverReason.billing (rotation +
fallback remain the right recovery either way); ClassifiedError grows a
billing_unverified property.
- agent/credential_pool.py: new FAILURE_REASON_BILLING_UNVERIFIED. An
unverified billing exhaustion gets the short transient cooldown instead of
the one-hour bench, regardless of pool size: a content-filter rejection
leaves the credential healthy and fails identically on every key, and the
hour-long sole-credential latch is what replayed the stored error and made
real fixes look ineffective. A true 402 keeps the full bench. The marker
persists with the entry so a restart cannot upgrade it back to a bench.
- agent/agent_runtime_helpers.py + run_agent.py: recover_with_credential_pool
threads billing_unverified and hands the pool 'billing_unverified' as the
persisted failure_reason.
- agent/conversation_loop.py: the fallback-switch status, max-retries status,
terminal label, and both structured terminal results hedge when the verdict
is unverified. New _billing_terminal_label + _billing_failure_result build
the returned terminal response in one place; the result dict now carries
billing_unverified and the billing_block gains 'unverified': true. The
confirmed-billing path (a real 402 or an API-key credit depletion) keeps
the original assertive wording, so the caveat no longer dilutes it.
Regression tests: classifier marking (400 + status-less + unambiguous-body
negative), pool cooldown TTLs + persistence round-trip, pool failure_reason
plumbing, and the returned terminal response for both unverified and
confirmed verdicts.
Note: tests/agent/test_credential_pool_routing.py::TestFailureAttribution::
test_unmatched_key_does_not_retry_only_pool_entry fails identically on
current main without this change (pre-existing, unrelated).
fb6d4f2 to
42ca5eb
Compare
|
Addressed in 42ca5eb (rebased onto current main). The ambiguity is now carried through every layer the review named, not just the guidance caveat: Classifier ( Persisted credential state ( Fallback and terminal labels ( Structured result: both terminal result dicts now carry Regression for the returned terminal response: terminal-response construction was factored into Verification: 110 targeted tests green (classifier, guidance, carry-through, sole-cooldown, skills-guidance), and the changed-file Windows footgun check passes. One note: |
|
Merged via PR #86677. Both commits were cherry-picked onto current main with your authorship preserved in git log — thank you, especially for carrying the billing-ambiguity flag through every surface rather than just the guidance text. |
What does this PR do?
On an Anthropic subscription OAuth credential, every request failed with
HTTP 400 "You're out of extra usage. Add more at claude.ai/settings/usage and keep going."That is not a billing condition — Anthropic's server-side content filter rejects the first sentence of Hermes' own built-inSKILLS_GUIDANCEprompt, and the rejection is surfaced with a billing-shaped message. Because the message points at the usage settings page, it reliably sends people to buy quota they don't need; the reporter lost three debugging sessions to it before capturing the actual request.The issue's bisect (live API, real 71,721-char assembled prompt,
claude-opus-5, stable across repeats): the firstSKILLS_GUIDANCEsentence alone reproduces the 400 and removing it alone clears it. Size was ruled out (20 KB of unrelated filler → 200) and so was thesystem[0]identity gate (that returns 429, a different failure).Three changes, all serving one outcome — a subscription user can no longer be misdirected by this 400:
1. Reword the trigger (
agent/prompt_builder.py) — to the phrasing the reporter verified returns 200. Meaning, theskill_managereference, and the## Skill Safety Ruleblock are preserved. The reword is empirically validated rather than understood, so a comment records the bisect and warns that any rewrite must be re-verified against an OAuth token, not ansk-ant-api…key (which doesn't hit the filter).2. Stop asserting exhaustion as fact (
agent/conversation_loop.py) — this is the durable half. The reword chases a moving server-side target; the misleading message is what cost the debugging sessions, and it will keep costing them the next time some other prose trips the classifier. The Anthropic branch of_billing_or_entitlement_messagenow hedges its opening line, names the content-filter alternative, and gives the operator a discriminator (if the usage page still shows quota, suspect a content rejection). It also points athermes auth reset anthropic, because of the reporter's observation 1 below.3. Document the setup-token confusion (
hermes_cli/auth.py) — the issue's observation 2.On the issue's two related observations
Observation 1 (cached exhaustion latch replays errors). Addressed as guidance, not as a mechanism change. After a failure the credential is latched exhausted for ~60 min and the stored error is replayed without issuing a request, so a real fix looks like it didn't work — exactly the trap that made this bug so expensive to diagnose. Actually suppressing or labelling the replay at its source means touching the credential-pool cooldown machinery (
_exhausted_until/STATUS_EXHAUSTEDinagent/credential_pool.py), which is shared across every provider and well outside this fix's blast radius. I deliberately left that alone and instead surfaced the reset command at the moment the user reads the confusing error. If maintainers want the replayed error tagged as cached at the source, that's a clean follow-up and I'm happy to open it.Observation 2 (
claude setup-tokenis not an API key). The issue suggests "a docstring note, or dropping that var from the tuple." Dropping it would be a regression —api_key_env_varsdoubles as the credential-discovery list (agent/credential_pool.pybuilds its env scan frompconfig.api_key_env_vars), so removingCLAUDE_CODE_OAUTH_TOKENwould stop Hermes finding a setup-token credential at all. Took the docstring option and wrote down why the entry has to stay.Relationship to open PR #78025 — please read before merging
#78025 (open,
needs-decision) already contains this sameSKILLS_GUIDANCEreword as its "layer 2", alongside a much larger wire-level tool-name aliasing change (session_search→chat_history_lookup). It targets a different issue (#65365), whose bisect found a three-sentence AND-condition rather than the single-sentence trigger reported here.I am not trying to race it. This PR is deliberately the small, surgical version of #82154: the prompt reword plus the diagnosability fixes that no open PR covers. If #78025 lands first, the
prompt_builder.pyhunk here becomes redundant and I'll happily rebase it out — items 2 and 3 stand on their own either way. If maintainers would rather fold everything into #78025, close this and I'll port items 2 and 3 across.Related open PRs in the same area, none of which touch the message or the registry note: #76807, #80854, #72173, #80618.
Related Issue
Fixes #82154
Type of Change
Changes Made
agent/prompt_builder.py— reword theSKILLS_GUIDANCEfirst sentence; comment records the bisect and the re-verification requirement.agent/conversation_loop.py—_billing_or_entitlement_messageAnthropic branch: hedgeis exhausted→may be exhausted, add the content-filter alternative with a discriminator, add thehermes auth reset anthropicnote for the cached-replay trap. Additive — every prior line is retained.hermes_cli/auth.py— comment on theanthropicProviderConfigexplaining thatCLAUDE_CODE_OAUTH_TOKENis an OAuth token (sk-ant-oat01…: 401s asx-api-key, 429s as bare Bearer), that onlyANTHROPIC_API_KEY/ANTHROPIC_TOKENare literal API keys, and why the entry must stay for discovery.tests/agent/test_skills_guidance_content_filter.py— new, 11 tests: rejected fragments absent (parametrized), the verified reword pinned, behaviour preserved, Safety Rule intact, newline count held above the boundtest_ghost_skill_pruning.pyasserts, and the constant still wired into the system prompt.tests/agent/test_anthropic_billing_guidance.py— 5 tests: content-filter alternative named, exhaustion not asserted as fact, cached-replay warning present, original billing remedies retained, and the Anthropic-specific lore not leaking to other providers.website/docs/user-guide/features/skills.md— "When the Agent Creates Skills" quoted the old prompt verbatim (After completing a complex task (5+ tool calls)); updated so the docs don't drift from the reworded prompt.The tests deliberately assert on the trigger substrings rather than only on an exact replacement string, so a future reword is free to change the prose as long as it doesn't reintroduce the rejected phrasing.
How to Test
The root cause only reproduces against the live API with a subscription OAuth credential:
Automated:
pytest tests/agent/test_skills_guidance_content_filter.py -q pytest tests/agent/test_anthropic_billing_guidance.py tests/agent/test_ghost_skill_pruning.py \ tests/agent/test_error_classifier.py -q python scripts/check-windows-footguns.py agent/prompt_builder.py agent/conversation_loop.py hermes_cli/auth.pyTo see the new guidance text without an Anthropic credential:
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — see note belowNote on the test checkbox, stated rather than ticked. I developed this on Windows and cannot reproduce the root cause here — it needs a live Anthropic subscription OAuth token, so the end-to-end fix is verified by the issue reporter's bisect, not by me. What I did verify locally:
test_anthropic_billing_guidance.py,test_ghost_skill_pruning.py,test_error_classifier.pypass: 87 tests onupstream/mainin a clean worktree, and 103 with this branch — exactly 87 + the 16 new ones. No pre-existing test changed outcome.tests/agent/as a whole has ~143 Windows-environment failures on both this branch andupstream/main(test_title_generator,test_model_metadata,test_models_dev,test_verification_*, path-separator assertions intest_system_prompt/test_prompt_builder, …). None are in the subsystems this PR touches. I could not get a full clean before/after diff of that suite inside my time budget, which is why I scoped the comparison to the three suites that actually cover the changed code and ran those against a pristineupstream/maincheckout.I also grepped the tree for any other assertion on the old sentence or the old billing string — the only hits were the docs file (updated here) and a user-story quote in
website/src/data/userStories.json, which is quoted testimony and deliberately left alone.CI on Linux/macOS is the real check here, and a reviewer with a subscription token running the reproduction above would be worth more than any of my local runs.
Documentation & Housekeeping
website/docs/user-guide/features/skills.mdquoted the old prompt verbatim and would otherwise have driftedscripts/check-windows-footguns.pyclean on the diffScreenshots / Logs
The guidance a subscription user now sees on that 400:
The first three lines are unchanged apart from
is→may be; the last two are new.