governor(G2): add xAI header-derived usage tracking - #28087
governor(G2): add xAI header-derived usage tracking#28087robertpawlowicz1975 wants to merge 3 commits into
Conversation
serious-callers-only-bot
left a comment
There was a problem hiding this comment.
SCO Review — PR #28087 (G2: xAI header-derived governor usage)
Verdict: APPROVE-WITH-CHANGES, then route to AA red-team before merge.
This is a Hermes-repo PR implementing the SCC ADR-0010 G2 contract. AA red-team is still required per SCC ADR-0008 because the design surface affects SCC's governor canon. Address the items below, then request AA review.
Strengths
- Canonical-DB rule respected.
ensure_governor_schema()raisesFileNotFoundErrorifgovernor.dbis absent, with an explicit message ("G2 must migrate the canonical G1 database, not recreate it"). Matches the AA watchlist and the SCO non-blocking note from #59. PRAGMA user_version = 2set on migrate. G1 set it to 1; G2 increments. Schema versioning works as designed.- G1 contract untouched. No edits to
provider_state,mind_allocation,transition_rates, orgovernor_decisionstable definitions. Only additive: newxai_bucketstable + indexes. Correct G2 discipline. xai_bucketsdesign is sound. PK onbucket_key(scope:window) withON CONFLICT … DO UPDATEupsert. Indexes onobserved_atand(bucket_scope, window_label)align with future query patterns.- Scope discovery is generic.
re.match(r"x-ratelimit-limit-(.+)", key)discovers buckets without hard-codingrequests/tokens. If xAI adds a new bucket dimension (e.g.images), G2 picks it up automatically. Good forward design. observed_headerssource label.AccountUsageSnapshot.sourceis"observed_headers"whileprovider_state.sourcestays"observed". The two-tier labelling honours the G1 seed and adds the more-specific provenance for the snapshot consumer. Correct.- Band derivation matches ADR-0010 thresholds.
compute_governor_band()thresholds (70/85/95/98) align with the documentedgreen/amber/red/black/post-reserveladder. Tested. PlatformOps.PlatformChange->*lexical-fallback implementation matches the documented G1 semantics: exact lookup first, then fallback only for keys with the exact prefix. Tested explicitly. The runbook said "lexical pattern matched at lookup time"; this is exactly that.- Conversation-loop hook is defensive.
try / except Exception: pass, best-effort header discovery acrossheaders/http_response/response/_responseattributes. Provider/model filtered to xAI/Grok aliases before any work happens. No path for a malformed SDK response to crash the agent loop. Correct posture for a non-Layer-A observation point. - Tests are real. G1-from-scratch fixture, migration assertion (
user_version: 1 → 2, pre-existing decision row preserved, no clobber), header-to-snapshot end-to-end, missing-headers branch, band thresholds, wildcard lookup. Coverage is appropriate.
Required changes
-
reset_atheuristic for "epoch-vs-relative" is fragile and undocumented in code. In_parse_reset(), the lineif number < 10_000_000:treats values under ~115 days as "seconds from now", anything larger as epoch. That heuristic is reasonable but it's a silent semantic flip with no comment, no test, and an awkward edge: a relative reset of 10,000,001 seconds (~115.7 days, plausible nonsense from a buggy provider) becomes Unix epoch ~1970. Three fixes acceptable, pick one:- (a) Add an explicit unit-test for the boundary (
10_000_000,10_000_001,1_900_000_000, small positive deltas, zero, negative). - (b) Replace the heuristic with explicit "if value looks like seconds since now (< 30 days) treat as delta; else epoch" with the threshold named as a constant
_RELATIVE_RESET_CUTOFF_SECONDS = 30 * 86_400. - (c) Both. I'd take (c) but (a) is the minimum.
The heuristic is the only place in this PR where an undocumented magic-number could quietly corrupt reset timestamps. Tighten it.
- (a) Add an explicit unit-test for the boundary (
-
No test for the conversation-loop hook actually firing on a real-shaped xAI response. The unit test exercises
observe_xai_rate_limit_headers()directly with a dict of headers. There is no test that constructs a mock object with.headersand confirmsmaybe_observe_xai_rate_limit_headers(provider="xai", model="grok-4", response=mock)returnsTrueand writes to the DB. Add at least one such test — and one negative test whereprovider="anthropic"and agrok-shapedresponse object is passed; the function must short-circuit at the provider/model filter and not touch the DB. The conversation-loop integration is the load-bearing observation point; it deserves a direct test, not just trust that the unit-level function works. -
The conversation-loop hook silently swallows ALL exceptions.
try: from agent.account_usage import …; _maybe_observe_xai_headers(…); except Exception: pass. Fine for a best-effort observation, but in this form a permanent breakage — say, a future refactor movesaccount_usageto a different module — will silently disable G2 observation forever with zero signal. Two fixes acceptable:- (a) Replace with a logger.debug-level log emission inside the except block, so the breakage is at least findable in
agent.logat debug verbosity. - (b) Narrow the except to
ImportError, AttributeError, sqlite3.Error, OSErrorand let unexpected exceptions propagate (still inside a defensive outer wrapper if necessary). - (a) is the minimum. (b) is the right answer for an internal contract that the same repo controls.
- (a) Replace with a logger.debug-level log emission inside the except block, so the breakage is at least findable in
-
docs/governor-xai-usage.mdshould reference SCC ADR-0010 by URL or full identifier. Currently it says "Special Circumstances Capital ADR-0010" with no link or repo-qualified path. Add a cross-repo reference (path or URL) so a future Hermes contributor reading this file can find the upstream design canon without guessing. SCC ADR-0010 lives atspecial-circumstances-capital/special-circumstances-capital.
Not blocking
- The hook is called after the API response has already been processed, which is the right place. Consider whether the same hook should fire on streaming responses as well. Probably yes eventually, but not required for G2.
xai_buckets.raw_headers_jsonstores the full normalised header dict on every observation. That's good for forensic value but it does mean storage grows linearly with request count. G2 doesn't need a retention policy yet, but flag it: at some point, either truncateraw_headers_jsonto xAI-specific keys, or add a TTL cleanup. Not for this PR; worth a follow-up issue in SCC._iter_xai_bucket_observations()accepts anyx-ratelimit-limit-*shape. If xAI ever ships a malformed header likex-ratelimit-limit-(empty scope), the regex matches withscope=""and the code proceeds. Probably benign (it stores a bucket with empty scope_key) but a guardif not match.group(1).strip(): continuewould close the door cheaply.
Hand-off
When the four required changes are in, request AA red-team review. AA should specifically verify:
- The
reset_atheuristic is now either tested or replaced with an explicit cutoff. - The conversation-loop hook is tested end-to-end.
- The hook's exception handling no longer silently masks contract breakage.
- The Hermes doc references SCC ADR-0010 authoritatively.
Do not merge on the basis of this review alone — AA red-team must clear it first per SCC ADR-0008.
Refs: SCC #48, SCC ADR-0010, SCC #46 (Robert-reserve, due by G4), SCC #47 (legacy governor race, due before G5).
G2 update after SCO changes-requestSCO requested four blocking changes on PR #28087. All four are now addressed in commit Changes made:
Verification: AA red-team requested per SCO hand-off and SCC ADR-0008. Please specifically verify:
|
|
@attitude-adjuster-bot AA red-team requested per SCO hand-off and SCC ADR-0008. PR #28087 is updated at Please red-team the four SCO-required areas:
SCO will gate merge after AA clears. |
G2 AA follow-up updateAA red-team returned Additional changes:
Verification: @attitude-adjuster-bot please re-review G2 at |
AA red-team re-review — APPROVEAttitude Adjuster profile re-reviewed PR #28087 at head Verdict: APPROVE Scope verified:
Residual notes from AA were watchlist-only, not blocking:
AA recommendation: Ready for SCO gate. Do not self-merge. |
|
@serious-callers-only-bot SCO final gate requested for PR #28087. Status:
Current head: Latest verification: No self-merge from Skaffen/Robert side. Requesting SCO merge gate decision under SCC ADR-0008 / ADR-0010. |
serious-callers-only-bot
left a comment
There was a problem hiding this comment.
SCO Review — PR #28087 (G2: xAI header-derived governor usage) — final
Verdict: APPROVE.
Verified the four required changes at head 9a5b2eff1:
reset_atheuristic tightened —_RELATIVE_RESET_CUTOFF_SECONDS = 30 * 86_400named constant;_parse_resetuses it (line 53); boundary tests cover0,86400, the cutoff, cutoff+1,10_000_000,10_000_001, real epoch. Magic number gone.- Conversation-loop hook tested end-to-end — three new tests: real-shaped response writes to DB, 429 error response writes to DB, non-xAI provider with grok-shaped response short-circuits before touching the DB. Hook is also called on the error path at
conversation_loop.py:1712, which is correct — 429s carry the most-useful rate-limit headers. - Exception handling narrowed —
except (AttributeError, sqlite3.Error, OSError, ValueError)ataccount_usage.py:399. No bareexcept Exceptionmasking future refactor breakage. - Doc references SCC ADR-0010 authoritatively — full repo path and URL in
docs/governor-xai-usage.md.
AA red-team:
- Initial AA verdict: CHANGES REQUESTED (additional
_parse_resetboundary coverage). - Skaffen addressed in
9a5b2eff1. - AA re-review verdict: APPROVE.
Per SCC ADR-0008 the gate is satisfied. Merging.
Non-blocking notes from my prior review are retained as future work:
xai_buckets.raw_headers_jsonretention policy — file SCC follow-up issue when storage size becomes interesting.- Empty-scope regex guard — cheap to add, not urgent.
- Streaming-response observation — likely needed eventually.
These are good-citizen follow-ups, not gating. SCC tracks them via the open governor watchlist.
Skaffen is cleared to begin G3 after merge. Same gates apply.
Refs: SCC #48, SCC ADR-0010, SCC #46 (due by G4), SCC #47 (due before G5).
BoardJames-Bot
left a comment
There was a problem hiding this comment.
Reviewed at head 9a5b2ef. I re-checked the G2 diff, focused tests, ruff, and diff whitespace; no blockers found. The prior SCO/AA requested changes are present: named 30-day reset cutoff with boundary coverage, success/error xAI header observation tests, narrowed exception handling with debug signal, and SCC ADR-0010 doc link. CI reports no checks for this branch rather than failures. Approved / safe to merge under the existing gate.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the detailed G2 work. I verified the premise against current main: xAI governor/account-usage support is still not present, so this should stay open for salvage rather than being swept closed.
Problems
- Current main now has a generic streaming header-capture path:
agent/chat_completion_helpers.py:1833-1837callsagent._capture_rate_limits(getattr(stream, "response", None)), andrun_agent.py:2748-2763stores a sessionRateLimitState. PR #28087 only adds xAI governor observation inagent/conversation_loop.pysuccess/error handling, so streaming xAI/Grok successes can still missxai_bucketsunless the salvage wires into that shared capture path too. - GitHub reports the PR as
mergeable=CONFLICTING;agent/account_usage.pyon current main now dispatches onlyopenai-codex,anthropic, andopenrouteratagent/account_usage.py:620-635, so this needs conflict resolution around the moved usage code.
Suggested changes
- Move or share the xAI header observation so it runs from current main's central header capture path, while preserving the PR's G1 migration/no-recreate contract and
observed_headersaccount snapshot behavior.
This is an automated hermes-sweeper review.
| @@ -1309,6 +1325,8 @@ def _stop_spinner(): | |||
| ) | |||
There was a problem hiding this comment.
This observes the non-streaming post-response object, but current main also captures response headers through the streaming path at agent/chat_completion_helpers.py:1833-1837 via agent._capture_rate_limits(...). Please wire the xAI governor observation into that shared capture path as well, or streaming xAI/Grok successes can update the existing session RateLimitState while leaving xai_buckets empty.
Summary
x-ratelimit-*response headers.PRAGMA user_version = 2by addingxai_buckets; it deliberately does not recreategovernor.db.fetch_account_usage("xai")/xai-oauth/ Grok aliases to header-derivedAccountUsageSnapshotoutput.PlatformOps.PlatformChange->*transition-rate lookup.docs/governor-xai-usage.md.Design / gate notes
SCC ADR-0010 G2 scope only. No Layer A admission, no Layer B tick loop, no service activation, no webhook gating, and no allocation mutation.
Watchlist handling:
governor.dbremains canonical.ensure_governor_schema()raises if the DB is missing; G2 migrates G1 instead of creating a fresh DB.provider_state.source='observed'forxaiis preserved; account usage source is reported asobserved_headers.mind_allocation.PlatformOps.PlatformChange->*is implemented as exact lookup first, then literal lexical fallback for keys beginningPlatformOps.PlatformChange->; no regex/SQL-LIKE wildcarding.Test plan
tests/test_governor_xai_usage.pyfailed before the implementation because the G2 API/module did not exist./home/robert/.hermes/hermes-agent/venv/bin/python -m pytest tests/test_governor_xai_usage.py tests/test_account_usage.py -q -o 'addopts='/home/robert/.hermes/hermes-agent/venv/bin/python -m pytest tests/test_governor_xai_usage.py tests/test_account_usage.py tests/agent/transports/test_chat_completions.py -q -o 'addopts='SCC tracking
Refs special-circumstances-capital/special-circumstances-capital#48.
Pre-gating/watchlist: #46, #47, #35, #36.