fix(agent): classify provider memory-ceiling 400s as overloaded, not context_overflow - #52289
briandevans wants to merge 12 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a misclassification in the agent’s API error classifier where local-inference “memory ceiling / memory guard” HTTP 400s (and similar no-status streaming errors) were being treated as context_overflow, incorrectly triggering the compress → shrink-context → retry loop and potentially leading to compression exhaustion and session reset.
Changes:
- Add
_MEMORY_CEILING_PATTERNSand classify matching errors asFailoverReason.overloadedbefore context-overflow checks (both in 400-status and no-status paths). - Add targeted tests covering multiple real-world memory-ceiling wordings plus a negative/invariant test ensuring genuine context-window overflows still classify as
context_overflowand trigger compression.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
agent/error_classifier.py |
Adds memory-ceiling pattern detection and prioritizes it ahead of context-overflow classification for 400 and no-status message paths. |
tests/agent/test_error_classifier.py |
Adds regression tests verifying memory-ceiling errors route to overloaded (no compression) and that real context overflows still compress. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alt-glitch
added
type/bug
|
Thanks for jumping on this, @briandevans — this matches the core of #52261 and I'm glad to see it The guard-before-context_overflow shape and the 1. Consider 2. Make sure the guard beats 3. oMLX also returns a structured code worth matching. I captured the raw provider body (before 3. oMLX also returns a structured code worth matching. I captured the raw provider body (before {"error": {"message": "oMLX prefill memory guard rejected this prompt: ... dynamic ceiling is 13.50 GB. ... or reduce context length.",
"type": "invalid_request_error", "code": "prefill_memory_exceeded", "omlx_code": "prefill_memory_exceeded",
"estimated_bytes": 15978071248, "limit_bytes": 14495514624}, "type": "error"}So it's unambiguously memory at the source ( I have sanitized real oMLX logs + a few extra fixtures (the 400, the streaming abort, the raw body Hope this helps! |
tonydwb
left a comment
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment (prior COMMENT review exists)
Well-tested fix for memory ceiling pattern classification. The fix correctly identifies local-inference memory/resource-ceiling rejections and routes them to overloaded instead of context_overflow. Good test coverage with 3 positive cases across 400-status and no-status streaming paths.
Changes:
agent/error_classifier.py: New_MEMORY_CEILING_PATTERNScheck before context_overflowtests/agent/test_error_classifier.py: 3 new test cases
Note: This PR has a prior COMMENT review. This review adds confirmation that the fix is clean and well-tested.
Reviewed by Hermes Agent
|
Followed up by checking out your branch and running it directly, @briandevans — here's what I found
The streaming one is the notable miss: Three small, structure-preserving changes fix all of it — verified, your suite stays green (165 → 169
The streaming one is the notable miss: Three small, structure-preserving changes fix all of it — verified, your suite stays green (165 → 169
Happy to open this as a PR against your branch (it's a +112/−12 diff with the 4 tests, ready to go), |
|
Thanks @jp-cruz — checking out the branch and running the real wordings is exactly the kind of verification that makes this solid. I took two of the three, and want to be straight with you about the third. Applied in d2a75c12b:
Four regression tests added (streaming→billing collision, 400+code-reworded, no-status+code-only, plus a billing-invariant negative guard); verified fail-before/pass-after, suite 165→169 green, adjacent failover/recovery suites green. On point 2 ( Sanitized fixtures still welcome if you want to widen coverage — appreciate the careful pass. |
|
Thank you @jp-cruz — checking out the branch and running it against real oMLX wordings is exactly the kind of verification that makes this solid, and your table nailed it. Both misclassifications you caught are now fixed on the branch (commit
On your If you can share those sanitized oMLX logs + raw body JSON, they would make great fixtures for a follow-up. Thanks again for the thorough pass. |
|
Assembling logs and syncing repos now. Thank you @briandevans for taking a look at this. Took me a bit of time running through the issue/proposed fix manually (slow) as well as running it by both claude/chatGPT to make sure I wasn't the one hallucinating. Here are the sanitized fixtures you asked about, @briandevans — drop them in wherever they help, no I bundled them as a small fixtures module covering the three transport shapes the same root cause
Plus two negative controls — genuine The raw oMLX 400 body (before any proxy re-wrap), verbatim: {"error": {
"message": "oMLX prefill memory guard rejected this prompt: Prefill would require ~14.88 GB peak (current 13.56 GB + KV+SDPA 1.32 GB) but dynamic ceiling is 13.50 GB. Raise custom_ceiling_bytes in admin Memory settings (currently pinned at 13.50 GB), or reduce context length. ...",
"type": "invalid_request_error", "param": null,
"code": "prefill_memory_exceeded", "omlx_code": "prefill_memory_exceeded",
"estimated_bytes": 15978071248, "limit_bytes": 14495514624
}, "type": "error"}Full fixtures module (a single self-contained file, Thanks again for taking all of this on so cleanly — the three commits read exactly right, and After posting, I had to go back and do a few sanity checks and rerun the logic a few times as I thought I was second-guessing myself after running through the code a few times. Hope this helps. Thanks for letting me contribute!
|
|
Thank you @jp-cruz — those sanitized fixtures and the per-shape coverage table are genuinely useful, and I went through all five transport shapes against the branch before deciding what to take. Wanted to be precise about each rather than bulk-importing. I checked out clean
Added ( Held off on the other four — they're already pinned, so adding them would be duplicate coverage of the same code paths:
Both your negative controls are also already guarded ( On the On Really appreciate the careful pass — the proxy-flattened capture in particular was the one gap worth closing. |
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused classifier coverage. Current main still has both reported priority collisions: 400 messages reach the context-overflow branch at agent/error_classifier.py:1187-1192, while status-less "limit exceeded" messages reach usage-limit/billing handling at agent/error_classifier.py:1351-1366.
Problems
- Current main added 5xx context-overflow routing after this PR's base (
a04b7024f):agent/error_classifier.py:1019-1030and:1033-1044compress any matching context text. This PR only adds memory-ceiling precedence to_classify_400and_classify_by_message, so a 500/502/503/529 memory-ceiling response containing"reduce context length"still enters compression. - The structured-code branch at
agent/error_classifier.py:1171-1175returnsoverloadedwithoutshould_fallback=True, unlike the new 400 and message-pattern branches. Its code-only regression test attests/agent/test_error_classifier.py:728-747does not cover that field.
Suggested changes
- Guard the current 5xx context-overflow branches with the same narrow memory code/message detection, with 5xx regression cases and a real-overflow control.
- Make the structured-code recovery annotation consistent with the selected memory-ceiling contract and assert it.
This is an automated hermes-sweeper review.
| # server with each compression call, and ends in "Cannot compress further" → | ||
| # destructive session reset. These tokens reference memory/allocation/ceiling/ | ||
| # guard wording exclusively (never a token or window count), so they are | ||
| # disjoint from genuine context-window-overflow language. Must be checked |
There was a problem hiding this comment.
Current main also has explicit context-overflow branches for 500/502 and 503/529 at agent/error_classifier.py:1019-1044 (added in a04b7024f after this PR's base). This PR only installs the guard for 400 and no-status paths, so a 5xx memory-ceiling message containing reduce context length still enters compression. Please cover those status branches before claiming this applies at every classification site.
There was a problem hiding this comment.
Addressed in 620b19bb — the guard is now installed on both 5xx routes you named, ahead of the existing overflow checks rather than after them:
_classify_by_status, the 500/502 branch:if _is_memory_ceiling(error_msg, error_code): return _memory_ceiling_result(result_fn), placed before the overflow-as-500 check for llama.cpp/llama-server.- the
{503, 529}branch: the same guard, ahead of the model-load-OOM overflow check — which was the likeliest 5xx home for a memory abort, since the bareoverloadedfallthrough was only reachable when the body happened to omit context wording.
The 400 path (_classify_400), the structured-code path (_classify_by_error_code) and the no-status message path (_classify_by_message) route through the same _memory_ceiling_result helper, so all five paths share one annotation contract instead of five hand-written results.
Coverage for the two branches you flagged: test_5xx_memory_guard_is_overloaded_not_context_overflow and test_5xx_memory_code_reworded_message_is_overloaded are parametrized across the status codes, test_5xx_genuine_context_overflow_still_compresses pins that a real overflow still compresses, and test_503_generic_overload_unaffected_by_memory_guard / test_500_generic_server_error_unaffected_by_memory_guard are negative controls so the guard cannot swallow an ordinary 5xx.
There was a problem hiding this comment.
Correction to the SHA above, covering this thread and the _classify_by_error_code should_fallback thread alongside it: 620b19bb no longer exists. The branch was 6,030 commits behind its old base and has been rebased onto current main; both answers now live in fc34396, and the branch head is 8207128.
Nothing in either answer changed, and both are re-anchored to symbols rather than to a SHA this time. The 5xx coverage is _is_memory_ceiling at agent/error_classifier.py:1346 in the 500/502 branch and :1376 in the {503, 529} branch, each ahead of that branch's overflow check — plus :1581 in _classify_400 and :1770 in _classify_by_message, where current main's ordering also required it to sit ahead of _USAGE_LIMIT_PATTERNS, _OVERLOADED_PATTERNS, _BILLING_PATTERNS and _RATE_LIMIT_PATTERNS. The should_fallback point is _memory_ceiling_result at :497: the structured-code route at :1682 returns that shared helper rather than building its own result, so all five routes annotate identically. Regression coverage for both is in tests/agent/test_error_classifier.py under test_5xx_memory_guard_is_overloaded_not_context_overflow and test_no_status_prefill_memory_code_is_overloaded, which assert on test names that survive the next rebase.
There was a problem hiding this comment.
Since the reply above, the request in this thread grew by two more status branches. Three commits (e347abdab78, dd5f376a89c, 0330df0a050) carried the guard past the {500, 502} / {503, 529} pair you named, to the remaining routes in _classify_by_status (agent/error_classifier.py:1201) that a local-inference memory wall can actually arrive on:
:1452—if status_code == 507 and _is_memory_ceiling(error_msg, error_code). oMLX mapsModelTooLargeErrorandInsufficientMemoryErrorto 507 on the model-load path, a different guard from the prefill path and one that never surfaces as a 400 or as either 5xx branch you flagged. It previously fell through to the generic "other 5xx" rule — retryable, butshould_fallbackunset — so once the retries were spent the turn died on a host whose memory ceiling had not moved.:1482—if status_code == 409 and _is_memory_ceiling(error_msg, error_code).ModelLoadingError("load aborted: process memory limit exceeded") maps to 409, which reached the generic "other 4xx" bucket and was reported asformat_error,retryable=False— a transient memory abort called a malformed request.
Both are gated on the same _is_memory_ceiling predicate rather than on the bare status, so neither branch claims a whole status code: test_507_without_memory_wording_is_generic_server_error and test_409_without_memory_wording_is_still_format_error are the controls for that.
This also moves the should_fallback thread sitting alongside this one, because both new routes return the shared _memory_ceiling_result (:537) instead of building their own result. On 507 that flag is the regression; on 409 it was already set and what changed is retryable. The structured-code route at :1775 gained a second code in the same series — prefill_memory_aborted joined _MEMORY_CEILING_ERROR_CODES (:514), the sibling that oMLX's prefill-memory body builder selects by exception type, so "admitted then killed mid-prefill" now annotates identically to "turned away at admission" instead of diverging once the message is reworded.
The evidence grades behind those three are deliberately not equal, and the code says so where each one lands: the 507 shape rests on a captured response body, whereas the 409 route and the prefill_memory_aborted pairing are read from the engine's source with no reporter body and are marked NOT CAPTURED in the fixtures that construct them.
The line anchors in my reply above have all moved — the branch was rebased again — so, corrected against the current head: _memory_ceiling_result is :537 (not :497), the 500/502 guard :1386 (not :1346), the {503, 529} guard :1416 (not :1376), _classify_400 :1659 (not :1581), the structured-code route :1775 (not :1682), _classify_by_message :1862 (not :1770). fc34396 and 8207128 cited here, and c2c430d890d cited on the should_fallback thread, were rewritten by that rebase and resolve to nothing; head is f17f3af7b7b as of 2026-08-15T07:29Z.
The names are the part that survives all of this. test_5xx_memory_guard_is_overloaded_not_context_overflow (tests/agent/test_error_classifier.py:939) and test_no_status_prefill_memory_code_is_overloaded (:835) still pin the two branches you asked for, and test_507_omlx_model_load_ceiling_is_overloaded_with_fallback, test_409_memory_abort_is_overloaded_not_format_error and test_prefill_memory_aborted_code_is_overloaded pin the three additions.
There was a problem hiding this comment.
Correction to the SHA above: the current head is 2976b5fbfdd160675ef44b65d8337cf75e769315; f17f3af7b7b was rewritten and is a non-ancestor of that head.
The status branches requested in this thread remain routed through the shared _memory_ceiling_result recovery contract. The durable regression anchors are test_5xx_memory_guard_is_overloaded_not_context_overflow (tests/agent/test_error_classifier.py:939) for 500/502/503/529 and test_507_omlx_model_load_ceiling_is_overloaded_with_fallback (tests/agent/test_error_classifier.py:1029) for the 507 model-load path.
| if code_lower in _MEMORY_CEILING_ERROR_CODES: | ||
| return result_fn( | ||
| FailoverReason.overloaded, | ||
| retryable=True, |
There was a problem hiding this comment.
This structured-code path leaves should_fallback at its default False, unlike the new 400 and message-pattern memory paths. If the intended contract is that memory-ceiling results are failover-eligible, set the flag here too and assert it in the code-only regression test.
There was a problem hiding this comment.
Addressed in 620b19bb. _classify_by_error_code no longer constructs its own result for this case — the memory-ceiling code returns _memory_ceiling_result(result_fn), and that shared helper sets should_fallback=True explicitly, with the rationale in its docstring: a local-inference memory wall stays a wall on retry, so failover is the correct recovery rather than compression.
Routing it through the shared helper also removes the per-route drift this comment was guarding against — the 400, 500/502, 503/529, code-only and no-status paths now all annotate identically instead of each setting the flags by hand.
The code-only regression test asserts the flag directly: test_no_status_prefill_memory_code_is_overloaded drives a structured code with no HTTP status and ends with assert result.should_fallback is True.
There was a problem hiding this comment.
Correction to the SHA above: 620b19bb is in the ancestry of neither this branch nor main — it was rebased away after that reply, so the citation points at nothing. Current head is c2c430d890d.
The mechanism is unchanged, and these anchors survive a rebase: _classify_by_error_code no longer builds its own result for the memory-ceiling codes, it returns the shared helper _memory_ceiling_result(result_fn) (agent/error_classifier.py:1683), defined at :497, which sets should_fallback=True as one recovery contract for every detection route — so the structured-code path can no longer be silently weaker than the 400 and message paths. test_no_status_prefill_memory_code_is_overloaded in tests/agent/test_error_classifier.py pins exactly that and closes on assert result.should_fallback is True.
There was a problem hiding this comment.
Both halves of this are in place. My previous two replies pinned them to commit SHAs, and this branch has been rebased twice since — c2c430d890d, which the last reply called the current head, is no longer an ancestor of it. Re-anchoring on names instead, since those survive a rebase and SHAs on this branch clearly do not.
Verified against the current head f17f3af7b7b:
The flag is set on the structured-code path. _classify_by_error_code (agent/error_classifier.py:1748) no longer builds its own result for this case — the memory-code guard at :1775 returns the shared _memory_ceiling_result helper (:537), which sets should_fallback=True. That helper is now the single construction site for this classification, reached from all four detection routes (400, 5xx, structured code, and the status-less streaming message), which is what stops the annotation drifting between them — the exact asymmetry you pointed at.
It is asserted in the code-only regression test. test_no_status_prefill_memory_code_is_overloaded, tests/agent/test_error_classifier.py:835 — a status-less body carrying only prefill_memory_exceeded with the message fully reworded, so the structured code is the only signal. It asserts should_fallback is True alongside reason == overloaded, retryable is True, and should_compress is False, so the code-only route is pinned to the same recovery contract as the others rather than a weaker subset.
On the contract question you raised: memory-ceiling results are intended to be failover-eligible, and the reasoning is recorded in the helper's docstring rather than left implicit — a local-inference memory wall stays wedged until the server restarts, so retry-with-backoff alone does not recover it and failing over to a roomier provider is the durable path. Compression is deliberately still off, since it cannot relieve a prefill memory peak.
Apologies for the churn on this thread. Going forward I will cite symbols and test names here rather than SHAs.
There was a problem hiding this comment.
Correction to the SHA above: f17f3af7b7b was rewritten by the successful rebase; the current head is 2976b5fbfdd. The mechanism is unchanged: _classify_by_error_code routes memory-ceiling codes through _memory_ceiling_result at agent/error_classifier.py:1918, and test_no_status_prefill_memory_code_is_overloaded at tests/agent/test_error_classifier.py:835 still asserts should_fallback is True at :859.
teknium1
added
sweeper:risk-session-state
29ed405 to
59e4002
Compare
|
Both findings were correct and are addressed in 1. 5xx memory-ceiling collision — confirmed, fixedReproduced before fixing: a 503 memory-guard body classified as The collision is the same one the PR already documents: New coverage, parametrized over
2. Inconsistent recovery annotation — confirmed, fixed at the rootYou are right that the structured-code branch returned Rather than patch the one branch, detection and the recovery contract are now factored into One correction: the line refs had drifted — the code-only test is at Verification
|
6ed6b30 to
620b19b
Compare
teknium1
added
the
area/memory
|
@jp-cruz — correction to the commit ids cited above, consolidated into one comment rather than four. The 2026-07-15 rebase-and-squash collapsed this branch to a single commit, so every SHA I cited in earlier replies is orphaned: Nothing was dropped in the squash. Per claim, verified against
Test names and file paths survive a rebase where commit ids do not, so those are the durable references from here on. Most of the rows above exist because you checked out the branch and ran real oMLX wordings rather than reading the diff — the proxy-flattened capture in particular would not have been pinned without your fixtures. |
|
Independent confirmation from a second local-inference setup, with a captured incident. Hermes 0.20.0 ( What happenedoMLX rejected a request with its prefill memory guard: 7 ms later Hermes started a compression. It ran for 8 minutes and reduced the session from 174 messages to 9: The conversation was never the problem: 63,337 tokens against a 262,144-token window. The memory was held by a second, idle model the engine had kept resident (28.0 GB + 35.6 GB against a 77.76 GB cap). Evicting that model fixes the request; compressing the conversation cannot. Setting a Why it misclassifiesOn clean >>> [p for p in _CONTEXT_OVERFLOW_PATTERNS if p in error_msg.lower()]
['context length']This branch fixes itI applied only the
The second row is the one I would highlight: that shape carries no structured error code at all, and is caught by the Happy to supply the full sanitized log excerpt if that helps. |
alt-glitch
added
area/compression
|
Thank you for this, @tkaufmann — a second engine build on completely different hardware, with a captured timeline, is worth far more than another synthetic fixture. The 174 → 9 line is the part that makes the cost concrete in a way the original issue never quite did: 63,337 tokens against a 262,144-token window, and the recovery mechanism spent eight minutes destroying 165 messages that were never the problem. Your diagnosis of the actual cause (a second idle resident model holding 28.0 + 35.6 GB) is also the thing that makes this a misclassification rather than a tuning issue — no amount of shrinking the conversation reaches memory held by another process. The premise is still live on current Row 2 is the row I'd highlight too, and the branch is built that way deliberately. Detection has two independent halves, and either alone is sufficient: One detail from re-running your 400 body against the pattern list, because it cuts in your favour: it matches on two patterns, Your code is covered explicitly. All four routes converge on one contract. 400, 5xx, structured code, and status-less message all return through a single On the drift — you're right, and it is worse than "the test file has drifted". The branch sits on a base ~5993 commits behind current Yes please on the sanitized log excerpt, and specifically the second shape — the streaming |
|
Here is the sanitized excerpt for the code-less streaming shape, and the answer to your question about the HTTP status: there is none, and there is no The capture (host, paths and session id redacted; message text verbatim): That last line is the misclassification landing two minutes later: a memory rejection read as context overflow, compression attempted, and the conversation declared uncompressible at 37,629 tokens. Why there is no status. The abort lands after the response has begun; the stream opened normally and then carried an error event. The SDK raises the base Why there is no code, and how that is provable from the message text. The engine has two ways to build a streaming error body. One is prefill-aware and does carry the code; the other is generic and does not: # prefill-aware branch
if isinstance(e, PrefillMemoryExceededError):
error_data = _prefill_memory_openai_error_body(e) # sets code + omlx_code
else:
error_data = {"error": {"message": str(e), "type": "server_error"}}
# the chat streaming generator's own handler, no isinstance check
except Exception as e:
error_data = {"error": {"message": str(e), "type": "server_error"}}The prefill-aware builder never emits the bare exception text: it wraps it as Worth noting for the fixture: the guard does raise For contrast, the same guard rejecting before the stream opens: Counts across the whole log, so the ratio isn't overstated: the prefill guard fired four times — twice as the code-less streaming On the wording, confirming your reading: the two variants differ in exactly the phrase the pattern was written against. The 400 says "predicted peak would require ~78.57 GB"; the streaming one says "predicted peak would exceed prefill safety cap 77.8GB". Neither matches One token on each side. The One refinement to the 262,144 figure from my original report. The number is the model's own window ( For completeness on that event, since it belongs to the 400 shape rather than this one: compression started at |
|
Filed as #86097, with the stack, the fall-through gates and the line numbers against current Impact turned out to be display-only as far as I can trace it: the agent's own path resolves the provider and gets the right window, and I could not find a place where this route's value feeds a decision. So it is a wrong "Auto-detected" figure in the settings UI for every custom-provider install, nothing worse. |
|
Thanks @tkaufmann. Before touching anything I ran your three shapes against All three gaps are fixed, in five commits. I have kept your evidence grading in the commit bodies and in the code comments rather than flattening it, because the difference between the 507 and the 409 is the whole reason this was worth acting on quickly. Captured
Read from source, not captured — labelled that way in the commit body and in the code comment
Corrections —
|
de86bb0 to
f17f3af
Compare
|
Correction to the SHAs in my comment above, @tkaufmann. The branch has been rebased onto current
Branch head is now The test names in that comment are the anchors that survive this, and none of them moved: Two things from the rebase itself, since both land in the classifier you have been reading:
Re-verified while I was in there: the premise is still unfixed upstream — |
fix(agent): classify provider memory-ceiling 400s as overloaded, not context_overflow Deeply documented and exhaustively tested — the captured oMLX bodies, the 0.5.6→0.5.7 rewording analysis, and the "must be checked BEFORE overflow/usage-limit at every site" ordering are the right rigor for a classifier bug. The centralized
|
…context_overflow Local-inference memory/resource-ceiling rejections (oMLX/MLX memory guard, OOM, structured prefill codes, "prefill context too large for available memory") were being classified as context_overflow. That routes them into a compress-and-shrink loop which cannot relieve a prefill memory peak and wedges the session into a "Cannot compress further" reset loop — the prompt is often tiny, so shrinking history does nothing. Classify them as `overloaded` instead: retry with backoff, no compression, no session reset (mirrors 503/529 recovery). The guard runs before the context_overflow check in both the 400 and no-status APIError paths, and is ordered after the empty-provider-response guard. See NousResearch#52261.
A second field report on issue NousResearch#52261 supplies the shape the suite covered most thinly: the oMLX prefill memory guard rejecting a request *after* the stream has opened. That exit is structurally different from the pre-stream 400 already covered here. The chat streaming generator catches bare ``Exception`` and builds ``{"error": {"message": str(e), "type": "server_error"}}`` without the ``isinstance(e, PrefillMemoryExceededError)`` discrimination the pre-stream builder uses, so the structured ``prefill_memory_exceeded`` code is dropped; and the OpenAI SDK raises the base ``APIError`` from ``_streaming.py:95``, which has no ``status_code`` attribute at all. The same structured error loses its structure at one exit. Neither the 400 route nor the error-code route can fire, so the classification rests entirely on the message text — and the only context token in that text is "context length", from the trailing "or reduce context length" remediation hint. Against an unguarded classifier this fixture reproduces the reported failure exactly: reason=context_overflow, should_compress=True, on a session of 37,629 tokens against a 256,000-token window. The reporter's log shows the consequence two minutes later — "Context length exceeded: 37,629 tokens. Cannot compress further." The window figures in the test are the reporter's own corrected ones: the model's window is 262,144, but Hermes could not read it because the value is nested under ``text_config`` in config.json, so it fell back to 256,000 and took the compression decision against that. Message text is pinned verbatim as a module constant; the classifier matches substrings, so reflowing it would silently change what is asserted.
Companion to the mid-stream capture: the same prefill memory guard, the same engine build, rejecting before the stream opens. Here the prefill-aware body builder runs, so the wrapper prefix and the structured ``code``/``omlx_code`` both survive — which is exactly what isolates the streaming exit as the only thing that strips the structure. Two things this fixture pins that the existing 400 cases did not: - ``str(error)`` is the OpenAI SDK's full body repr, so the literal ``'type': 'invalid_request_error'`` is inside the matched text. That is the reason _REQUEST_VALIDATION_PATTERNS excludes that one entry from its own match; without the exclusion this 400 would be answered as a non-retryable format_error before any memory or overflow check ran, and the exclusion had no test standing on it from this direction. - The body carries no context-overflow token at all (the reporter elided the remediation sentence), so against an unguarded classifier it does not reach the overflow branch — it falls through 400 handling to the format_error default, reason=format_error with retryable=False. The same rejection is therefore wrong in two different directions depending on which exit it takes, which is why both routes need the guard rather than only the overflow-adjacent one.
…tterns The reporter on issue NousResearch#52261 ran both captures against the pattern lists and the result exposes a stale entry in this list's own evidence base: >>> [p for p in _CONTEXT_OVERFLOW_PATTERNS if p in msg.lower()] ['context length'] >>> [p for p in _MEMORY_CEILING_PATTERNS if p in msg.lower()] ['available memory'] "prefill would require" was written against the oMLX build that reports "Prefill would require ~13.87 GB peak". oMLX 0.5.7 reworded that same sentence to "predicted peak would require ~78.57 GB" on the pre-stream path and "predicted peak would exceed prefill safety cap 77.8GB" mid-stream, so the entry stopped matching the engine it was written for and nothing failed — the shapes kept classifying correctly, but on one unrelated token each. That leaves the mid-stream shape decided by a single substring on either side: "available memory" for the memory route, "context length" for the overflow route, and the latter comes from the appended remediation hint rather than from the rejection itself. One more copy-edit in either sentence flips a memory rejection back into the compression loop. So generalize on the wording that survived the verb change and appears in both 0.5.7 exits: the verb-independent "predicted peak would", and the two cap names "prefill safety cap" and "metal_cap". "prefill would require" is kept — the older wording is still in the field, and it is the only match on the 13.5 GB build alongside "dynamic ceiling". All three additions name an allocation ceiling in bytes, never a token or window count, so they stay disjoint from _CONTEXT_OVERFLOW_PATTERNS; a test now asserts that disjointness across both lists rather than leaving it as a comment, because widening this list is exactly how genuine window overflows would get diverted out of compression.
The pre-stream fixture was transcribed from a report that elided the
remediation sentence with a trailing "...", and a second elision dropped
``limit_bytes`` and the outer ``"type": "error"`` wrapper. The reporter has
since supplied the complete body; all three are on the wire.
This matters beyond fixture hygiene. The elided sentence is
Raise kernel iogpu.wired_limit_mb in Terminal (currently caps Metal at
86.40 GB), or reduce context length. To continue, set Memory Guard to
aggressive, raise the custom memory guard ceiling, free system memory,
or compact/reduce context.
which contains "context length", so the real body DOES reach the overflow
branch — the opposite of what the elided form showed. Classified against an
unguarded classifier, the real capture is context_overflow with
should_compress=True, i.e. wrong in the SAME direction as the mid-stream
shape, not a different one. format_error is reachable only from the
truncated string, which the engine never emits.
The body pinned here is the 13 Aug 14:11 firing (kv_len=83168), which is the
one that goes with the approx_tokens=63337 this test passes; the previously
transcribed 12 Aug 16:37 firing is equally real but pairs with different
accounting numbers. All four existing assertions hold unchanged either way.
The truncated string is kept under a name that says it is constructed, with
its own test. No real oMLX body omits the remediation hint, so the guard is
never asked to work without one — but pinning that it could keeps a future
engine copy-edit from silently reopening this bug.
Both claims were ours, both are comment-only, and neither ever matched the
code — the assertions they sit above pass unchanged before and after.
1. test_400_omlx_057_prefill_memory_exceeded_is_overloaded said the body
"does not even reach the overflow branch" and was "wrong in a different
direction from the streaming shape". That described the elided
transcription the fixture used to carry. The real capture ends "or reduce
context length", so an unguarded classifier reads it as context_overflow
with should_compress=True — the same direction as the mid-stream shape.
This claim is also in the message of commit 4db5a59. That commit is
pushed and amending it would rewrite published history, so the correction
rides here instead.
2. test_streaming_omlx_057_prefill_abort_without_status_or_code said the
model's window could not be read because it was nested under
``text_config`` in config.json, and that the 256,000 fallback was "the
number the compression decision was actually taken against". oMLX does
report the window as ``{"max_model_len": 262144}`` on /v1/models, which
agent/model_metadata.py already parses; the nested
``max_position_embeddings`` is a red herring. The 256,000 is a display
value produced by a different route.
The reporter's stack shows that route is /api/model/info, and that the
agent's own client had a working base_url one second later. No log line
records what the compressor itself resolved, so that remains a strong
inference rather than a measurement, and the corrected comment does not
assert it either way. ``context_length=256000`` stays as a parameter; it
only has to be a plausible window well above 37,629.
A third rejection shape from the same engine, and a different guard from the
two already covered: the prefill guard refuses a prompt before admitting it,
while this is the process memory enforcer aborting a request already in
flight because resident usage crossed a watermark.
Request aborted: process memory limit exceeded (usage 49.8 GB, abort
threshold (hard watermark) 49.2 GB, ceiling 51.8 GB). Reduce context
length, free system memory, or loosen memory_guard_tier
(safe -> balanced -> aggressive).
No production change is needed — "memory limit exceeded" and
"memory_guard_tier" are both already in _MEMORY_CEILING_PATTERNS, so the
guard covers this route today. What was missing was the pin.
It is worth pinning because its unguarded failure mode is the worst of the
three and is not the overflow misroute the other two suffer. "process memory
limit exceeded" contains "limit exceeded", a _USAGE_LIMIT_PATTERNS entry
checked ahead of the overflow branch, and the body carries none of the
transient signals that disambiguate a usage limit toward rate_limit. So
without the guard a local Metal watermark abort classifies as billing:
non-retryable, surfaced to the user as an account problem, turn stranded.
That is a third distinct wrong answer, which is the argument for a memory
guard sitting ahead of these branches rather than beside them.
The tier separator is transcribed as "→" but could not be confirmed against
the raw log, so both spellings are pinned and asserted to classify
identically. None of the matching tokens involve the separator.
…ract
_classify_by_status had no 507 branch, so a local-inference model-LOAD
rejection fell to the generic "other 5xx" rule: server_error,
retryable=True, should_fallback=False.
507 is a distinct guard from the prefill path this PR already covers, and
it is reachable from ordinary traffic: oMLX maps both ModelTooLargeError
and InsufficientMemoryError to 507, and /v1/chat/completions,
/v1/completions and /v1/messages all reach them through
get_engine_for_model -> get_engine. A captured body reads "Model 'X'
(33.95GB) does not fit under the dynamic memory ceiling (25.22GB) ...
raise memory_guard_tier", i.e. it carries both "memory ceiling" and
"memory_guard_tier" but never reaches a memory check.
The bit that differs from the contract is should_fallback. Retrying is
not wrong — the capture's own retry succeeded 2.7s later once the aborted
request released its memory — but a model that does not fit under the
host's ceiling does not begin to fit within the retry budget, so when the
wall does not clear the turn dies on the wedged host with no failover to a
roomier provider. That is precisely the case _memory_ceiling_result was
written for.
Routed through the existing _is_memory_ceiling predicate rather than a
bespoke branch, so the annotation cannot drift from the 400 / 5xx / code /
status-less routes. Gated on the predicate, so a 507 in its literal
Insufficient Storage sense keeps the generic 5xx treatment.
Evidence: captured against this branch's head by tkaufmann on two Apple
Silicon hosts running oMLX 0.5.7.
Regression tests (tests/agent/test_error_classifier.py):
- test_507_omlx_model_load_ceiling_is_overloaded_with_fallback
(red before this change: server_error / should_fallback=False)
- test_507_without_memory_wording_is_generic_server_error (control)
Refs NousResearch#52261
_MEMORY_CEILING_ERROR_CODES held prefill_memory_exceeded but not its
sibling. oMLX's prefill-memory body builder selects between the two by
exception type:
code = ("prefill_memory_aborted"
if isinstance(exc, PrefillMemoryAbortedError)
else "prefill_memory_exceeded")
so "exceeded" is the prompt turned away at admission and "aborted" is the
prompt admitted and then killed mid-prefill. Same guard, same memory
wall, same recovery — retry with backoff, no compression, fall back once
the retries are spent.
Scope, stated precisely: this is NOT a live misclassification today. The
aborted body still says "memory guard" and "available memory", so
_MEMORY_CEILING_PATTERNS catches it. What fails is the CODE layer, which
exists for the case where the message has been reworded by the engine or
flattened by a proxy — and that case is not hypothetical here, since the
0.5.6 -> 0.5.7 rewording of the prefill sentence already silently broke a
pattern written against the release it was written for. With the memory
wording gone the two codes diverge: exceeded -> overloaded/retryable,
aborted -> format_error/not-retryable on the 400 path and the retryable
unknown bucket on the status-less one.
The code pairing is read from the engine's body builder, not captured:
neither reporter's logs contain prefill_memory_aborted.
Regression test (tests/agent/test_error_classifier.py):
- test_prefill_memory_aborted_code_is_overloaded[400] and [None]
(red before this change: format_error / unknown), including the
assertion that no memory pattern is present in the fixture message,
so the test cannot pass through the message layer.
Refs NousResearch#52261
…ract
CODE READING, NOT A CAPTURE — stated up front because it is the one part
of this series with no observed body behind it.
oMLX raises ModelLoadingError carrying "Model 'X' load aborted: process
memory limit exceeded" and server.py maps it to 409. _classify_by_status
has no 409 branch, so it reaches the generic "other 4xx" bucket and is
reported as format_error, retryable=False: a transient memory abort
described as a malformed request. should_fallback is already set there,
so the turn is not stranded — this is the mildest of the three gaps — but
the request is never retried against the primary either, even though the
abort clears as soon as the host reclaims memory.
No reporter has produced a 409 body; this is read from the engine's source
alone. It is included because it is the same guard and the same wall as
the shapes that ARE captured, and because gating it on the shared
_is_memory_ceiling predicate makes it inert otherwise: a 409 in its
ordinary sense — a model swap already in flight, a duplicate request id —
keeps the existing 4xx treatment, which the control test pins.
Regression tests (tests/agent/test_error_classifier.py):
- test_409_memory_abort_is_overloaded_not_format_error
(red before this change: format_error / retryable=False)
- test_409_without_memory_wording_is_still_format_error (control)
Refs NousResearch#52261
Comments only — no behaviour change, verified by the diff carrying no non-comment added line. Three claims in this PR's own notes were either overstated or stale. 1. The tier-ladder separator is settled as U+2192, and this branch already used it. The test fixture's comment still said the spelling "has not been established from the raw log"; it has been since — twelve occurrences in agent.log and twelve in errors.log, every one the bytes e2 86 92, zero occurrences of the ASCII "safe -> balanced" in either, and the engine holds the ladder in one module-level constant with no ASCII sibling. No fixture bytes change. The ASCII variant is re-labelled CONSTRUCTED — the same status the file already gives _SYNTHETIC_PREFILL_400_NO_OVERFLOW_TOKEN — and kept as a lower bound that no matching token lives in the separator, not as a second real spelling. 2. "memory_guard_tier" and "dynamic ceiling" are not load-bearing and the comment should not imply they are. oMLX builds the remediation tail from describe_ceiling_binding(), whose branches vary both: the noun before "ceiling" is ceiling / static ceiling / dynamic ceiling / metal_cap ceiling depending on which cap binds (ties slash-join, so "static/metal_cap ceiling" is reachable), and memory_guard_tier is named in only some branches — the dynamic+custom branch points at custom_ceiling_bytes and the metal_cap branch at iogpu.wired_limit_mb instead. Nothing is left unguarded, because "memory limit exceeded" is in the message prefix and survives every branch; the note now says so, so a later pruning pass cannot mistake the widening entries for the guarantee. 3. omlx_prefill_memory_exceeded has no known producer. omlx_code mirrors code rather than being a prefixed variant, which the captured bodies in the tests show directly, so that set member has never matched anything observed. Retained rather than dropped — it cannot false-positive, and a namespacing proxy is the obvious way it would start appearing — but now documented as defensive rather than left reading as a shape someone had seen. Refs NousResearch#52261
The process-memory abort has now been captured twice with different remediation tails: 2 Aug, which this PR already pinned, and 11 Aug on a different host. Only the earlier half was on the record. Not a host difference. engine_core.py on the first host carries an mtime one day after its own 2 Aug capture, and the tail now comes from describe_ceiling_binding(), which emits neither "loosen" nor "free system memory" in any branch — so that host cannot reproduce its own earlier wording today. "loosen memory_guard_tier ... free system memory" became "Close other apps to free RAM ... raise memory_guard_tier", and the bound cap moved from a bare "ceiling" to a "dynamic ceiling" with the static cap reported alongside. This is the same drift the memory-accounting entries in _MEMORY_CEILING_PATTERNS exist for, caught a second time on a different sentence. The first time it happened nothing failed: the pattern written against "Prefill would require ~13.87 GB peak" silently stopped covering the release it was written for. A second captured wording of the same shape is the cheapest guard against a third copy-edit doing it again. Honest about what this is: a characterisation test, not a regression test for any commit in this series — the reworded body already classifies correctly on this branch, so there is no red-before against it. It IS red against clean origin/main (c896c09), where it classifies as billing / retryable=False, which is the worst of the unguarded failure modes this file documents. A companion test asserts the two captures genuinely differ, so a later tidy-up cannot collapse them into one string and quietly delete the drift they record. Refs NousResearch#52261
f17f3af to
2976b5f
Compare
…rloaded
oMLX/MLX prefill memory-guard rejections name an allocation peak in BYTES but
close with "Reduce context length", so _CONTEXT_OVERFLOW_PATTERNS claims them
and the turn enters the compress-and-shrink loop. Compression cannot lower a
prefill peak — the prompt is usually far below the window — so it burns the
compression budget, re-hits the wedged server on every attempt and ends in
"Cannot compress further" plus a destructive session reset.
Classify them as `overloaded` instead: retry with backoff, no compression, no
session reset (mirrors 503/529 recovery).
The guard runs before the overflow check AND before the usage-limit
disambiguation. The second ordering matters more than it looks: "memory limit
exceeded" contains "limit exceeded", so a status-less rejection — a proxy that
flattened the body — is currently classified as `billing` and rotates a
healthy credential.
Sites covered:
- _OVERFLOW_AS_5XX_RULES, which _400_TAIL_RULES extends → 400, 500, 502,
503, 529
- _MESSAGE_HEAD_RULES for the status-less path (ahead of usage-limit)
- _ERROR_CODE_VERDICTS for the structured oMLX codes
- _classify_400, because _by_status runs before _by_error_code, so a body
whose wording a proxy stripped would otherwise fall through to
format_error
Every pattern names memory/allocation in bytes, never a token or window count,
so the list stays disjoint from _CONTEXT_OVERFLOW_PATTERNS. Both oMLX wordings
are kept: 0.5.6 says "Prefill would require ~13.87 GB peak", 0.5.7 reworded it
to "predicted peak would require/exceed" and both are in the field. A test
pins that a genuine window overflow still compresses.
Refs NousResearch#52261. Supersedes NousResearch#52289, which predates the classifier rewrite and can
no longer be merged.
|
Heads-up: this no longer merges. The branch is over 5000 commits behind The bug is still there on |
alt-glitch
added
P1
…rloaded
oMLX/MLX prefill memory-guard rejections name an allocation peak in BYTES but
close with "Reduce context length", so _CONTEXT_OVERFLOW_PATTERNS claims them
and the turn enters the compress-and-shrink loop. Compression cannot lower a
prefill peak — the prompt is usually far below the window — so it burns the
compression budget, re-hits the wedged server on every attempt and ends in
"Cannot compress further" plus a destructive session reset.
Classify them as `overloaded` instead: retry with backoff, no compression, no
session reset (mirrors 503/529 recovery).
The guard runs before the overflow check AND before the usage-limit
disambiguation. The second ordering matters more than it looks: "memory limit
exceeded" contains "limit exceeded", so a status-less rejection — a proxy that
flattened the body — is currently classified as `billing` and rotates a
healthy credential.
Sites covered:
- _OVERFLOW_AS_5XX_RULES, which _400_TAIL_RULES extends → 400, 500, 502,
503, 529
- _MESSAGE_HEAD_RULES for the status-less path (ahead of usage-limit)
- _ERROR_CODE_VERDICTS for the structured oMLX codes
- _classify_400, because _by_status runs before _by_error_code, so a body
whose wording a proxy stripped would otherwise fall through to
format_error
Every pattern names memory/allocation in bytes, never a token or window count,
so the list stays disjoint from _CONTEXT_OVERFLOW_PATTERNS. Both oMLX wordings
are kept: 0.5.6 says "Prefill would require ~13.87 GB peak", 0.5.7 reworded it
to "predicted peak would require/exceed" and both are in the field. A test
pins that a genuine window overflow still compresses.
Refs #52261. Supersedes #52289, which predates the classifier rewrite and can
no longer be merged.
|
Superseded: the memory-ceiling → |
* fix: keep Bot Mode pet selection rings inside the gallery
* fix(prompt): keep memory guidance within available tools
* fix(compression): keep lean tails lean after auxiliary feasibility
Lowering the session trigger must not replace the window-relative lean
selection budget with threshold times target_ratio. Invalidate the lean
cache through the existing property while preserving explicit legacy and
external-engine fallback behavior.
Narrow adaptation of the aux-sync diagnosis and invariants in #93576,
without adding a required recalibration method to context engines.
Related: #95681, #93576
Co-authored-by: Turgut Kural <58116817+TurgutKural@users.noreply.github.com>
* feat(desktop): let users order Group Chat rooms
Add Move up/down controls for actual rooms without changing bot or folder
ordering. Preserve default pin/activity ordering until an explicit move,
retain hidden room slots, and persist Desktop-local order through room
updates, mirror merges, and hydration. No membership or routing writes.
Adapted narrowly from the group ordering idea in archived
NousResearch/Hermes-Bot-Mode#105 by @onuraycicek; rename already exists.
Co-authored-by: Onur Aycicek <onur.m.aycicek@gmail.com>
* fix: hide inactive grouping options from delegation schema
* fix(desktop): show the focused bot's working think pose
Port Adolanium's focused-turn pose from Hermes-Bot-Mode#101 and
hermes-agent#88134 to the current typed Bot Mode implementation.
Match the busy signal's connection-qualified focused owner rather than
the gateway socket, retain worker activity, and ease transitions in
elapsed time on the existing shared face clock.
Includes owner-isolation and animated-pose invariants, both proven red
on origin/main, and native Electron before/after verification against
a real temporary Hermes backend with held loopback inference.
Co-authored-by: Teknium <127238744+teknium1@users.noreply.github.com>
* fix(desktop): load property card guidance only on demand
* fix(cli): keep monitor repaints safe during prompt handoff
* fmt(js): `npm run fix` on merge (#106039)
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* fmt(js): `npm run fix` on merge (#106065)
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* test(desktop): retire model catalog fixture jobs at teardown
* fix(desktop): let subagent header collapse roster and details
* feat: add GPT Image 2.5 generation and editing to OpenAI provider
* feat: add FAL GPT Image 2.5 generation and editing selections
* chore(deps): bump httpx2 in the uv group across 1 directory
Bumps the uv group with 1 update in the / directory: [httpx2](https://github.com/pydantic/httpx2).
Updates `httpx2` from 2.7.0 to 2.12.0
- [Release notes](https://github.com/pydantic/httpx2/releases)
- [Changelog](https://github.com/pydantic/httpx2/blob/main/src/httpx2/CHANGELOG.md)
- [Commits](https://github.com/pydantic/httpx2/compare/v2.7.0...v2.12.0)
---
updated-dependencies:
- dependency-name: httpx2
dependency-version: 2.12.0
dependency-type: direct:production
dependency-group: uv
...
Signed-off-by: dependabot[bot] <support@github.com>
* feat(desktop): default glass to 29% tint on the sidebar
* docs(desktop): document sidebar glass defaults
* fix(desktop): keep background reports behind bounded disclosures
* docs(desktop): explain background report disclosures
* fix: trim computer use tool schema guidance
* fix(desktop): pin Windows update handoff cwd
* test(desktop): exercise production cwd setup in Windows self-test
* fix(mcp-oauth): keep refresh_token when a refresh response omits it (#62333)
HermesProviderMixin._handle_refresh_response overrides the SDK's handler (to
accept any 2xx and keep token bodies out of logs) but dropped the SDK's RFC 6749
section 6 carry-forward. An authorization server that does not rotate refresh
tokens (TinyFish, Google, Zoho, Asana, Futu) answers the refresh grant without a
refresh_token; we then stored the response verbatim, erasing the only refresh
token we had, so the next expiry had nothing to refresh with and forced a
browser re-auth roughly one TTL after every login.
Carry the prior refresh_token (and scope, per section 5.1) forward on the
OAuthToken before _store_tokens, so both the live provider and the on-disk
token file keep it. A rotating AS still wins: only None fields are filled.
Tests: two invariants on the real HermesMCPOAuthProvider + HermesTokenStorage
(omitted -> preserved in memory and on disk; provided -> rotated). The
carry-forward test is red on main.
* fix(desktop): allow project creation while browsing all profiles
* test(desktop): cover project creation scope and reconnect routing
* fix(observability): attribute ACP and batch execution surfaces
Fleet telemetry showed "unknown" as the single largest execution_surface
bucket. Two construction paths were mis-attributed, both silently:
1. ACP editor sessions (VS Code / Zed / JetBrains) declare platform="acp",
but "acp" was absent from EXECUTION_SURFACES, so the contract's
closed-schema fallback folded every editor session into "other" --
the bucket meant for genuinely unclassifiable traffic.
2. batch_runner built agents from _AGENT_PASSTHROUGH, which omitted
"platform" entirely, so every batch task run reported "unknown"
despite "batch" already being a first-class surface.
Neither is a reporting bug in the exporter: both are declaration gaps at
the construction site. "unknown" must mean "this run genuinely could not
be attributed", not "a construction site forgot to say who it was".
Changes:
- add "acp" to EXECUTION_SURFACES and map it to the "interactive"
entrypoint alongside cli/desktop/tui
- add "acp" to the v2 wire schema enum (kept in sync by an existing test)
- pass platform through batch_runner: added to _AGENT_PASSTHROUGH, set
self.platform = "batch" on the runner, and defaulted at the worker call
site so callers that build a config without it stay attributable
Wire compatibility: the ingest service validates the envelope only and
stores metric bodies verbatim, so packages carrying the new value are
accepted by the already-deployed server. No coordinated deploy needed.
Tests: 12 new behavioural tests. Verified red before the fix (4 failed),
green after. Three fix-mutants confirmed killed:
M1 revert acp from EXECUTION_SURFACES -> 3 failed
M2 revert acp entrypoint mapping only -> 1 failed
M3 revert batch passthrough -> 1 failed
No source-text assertions; every test is a contract between the surfaces
the schema accepts and the surface each path declares. A guard test pins
that a genuinely undeclared run still reports "unknown", so attribution
cannot be "fixed" by inventing a default that hides real gaps.
* fix(desktop): keep visible renderer animations running on blur
* fix(desktop): keep pets and starmap animated without focus
* fix(desktop): keep macOS HUD visible when inactive (#102573)
* fmt(js): `npm run fix` on merge (#106231)
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* feat(desktop): show messages below the thread viewport
* test(desktop): cover scrolling message counts and pane isolation
* fmt(js): `npm run fix` on merge (#106237)
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* fix(agent): a surface switch must not re-prefill the whole request (#104414)
`_stored_prompt_matches_runtime` treated `Platform` as a runtime-identity field, so
answering a live session from another surface — desktop -> TUI, or a resume after a
dashboard restart whose chat is a PTY TUI child — declared the stored prompt stale and
rebuilt it. The system prompt is the first thing in the request, so changing any byte of
it moves the first divergent byte to the head of a 220K-token request and the entire
conversation behind it re-prefills: a session that was hitting 240000/240287 came back
at 1536/219861.
The guard was not wrong about correctness — a desktop-built prompt on a terminal session
advertises inline widgets and a MEDIA: channel the TUI does not have — but the surface is
advisory metadata about the renderer, not a cache domain. Model/provider and cwd drift
change what the prompt should SAY; the surface changes only one paragraph.
Reuse the stored bytes across a surface switch and correct the paragraph where it costs
nothing to cache: `_stage_surface_switch_note` stages a one-shot note carrying the CURRENT
surface's guidance on the same per-turn user-message channel the gateway's must-deliver
notes use. It lands after the cached prefix and is stamped into the byte-stable
`api_content` sidecar, so later turns replay it instead of re-prefilling, and the prompt
converges at the next compaction — a boundary that already breaks the cache.
The saved tool_names prefix is not pinned across a switch: the tool registry is
process-global, so `_merge_preserving_prefix` would carry a saved-but-unloaded tool
forward (under `coding_context: focus` desktop gets a desktop_ui toolset the TUI cannot
run). On the same surface the tools freeze is untouched.
* fix(agent): skip the tools freeze once on a surface switch, not for the session
The first cut gated the saved tool_names pin on "the surface drifted", which stays true
for as long as the stored prompt names the old surface — i.e. until the next compaction.
On the gateway path, where a fresh AIAgent is built per turn, that left the tools freeze
off for every remaining turn, so a check_fn that flaps could reorder `tools[]` and break
the tool cache block on its own.
Gate it on the turn that actually ANNOUNCES the switch instead, and persist the fresh,
toolset-correct names there. The next turn's row already holds this surface's tools, so
the pin resumes immediately: skipped once, not disabled.
* fix(agent): the surface note must not outlive its own truth
Two holes the first cut left open, both created by the note itself.
Switching BACK to the surface the prompt was built for (desktop -> tui -> desktop) left the
`Platform:` trailer agreeing with the runtime, so nothing was staged — while the newest note
in the transcript still told the model it was on tui. And a rebuild for an unrelated reason
(a model switch) refreshed the prompt but not that note, leaving the same contradiction from
the other side.
Compare the runtime surface against what the model was last TOLD — the newest surface note
when one exists, else the prompt's own trailer — and stage from the rebuild path too. The
full surface guidance rides along only when the prompt itself is out of date; when the prompt
already describes the current surface the note just retires the stale one and points at it.
* fix(agent): hold the tools pin through a surface switch, name what it carried
The announcing turn used to skip the tools freeze and re-persist the array the new
surface had just built. That is the one mutation this fix cannot afford: tools[] is
serialized ahead of the system prompt, so rebuilding it moves the request at token 0
and re-prefills everything behind it — the exact cost #104414 measured (1% cache hit
on a 220K session), spent on the very turn the fix exists to make cheap. On a
`desktop -> tui` switch with a configured toolset selection (`_gui_surface_toolsets`
gives desktop `desktop_ui`, the TUI nothing), skipping the pin dropped ~a dozen tools
and bought back the whole miss.
The pin now holds. `_merge_preserving_prefix` still appends what the new surface
brought, so a `tui -> desktop` switch pays a break no freeze could have avoided, and
the tools it carries FORWARD are named at the end of the surface note instead of being
silently advertised: a `focus_pane` a terminal turn can only answer with
`tool_error("desktop only")` now reads as unavailable rather than as live capability.
The toolset converges at the next real rebuild boundary, where the break is already
paid.
Credit to @StanleyStetson, who caught that the tool array is evaluated ahead of the
system prompt and that the bypass reintroduced the miss this PR is about.
* fix(agent): retire stale surface notes on bot-chat refresh, isolate platform from decoys
When Bot Chat capability refresh rebuilds the system prompt for the current
surface, call _stage_surface_switch_note() so any earlier switch note sitting in
the transcript is retired instead of overriding the rebuilt prompt.
Also isolate _stored_prompt_platform() to parse only the authoritative identity
portion before '# Hermes runtime environment' (with legacy fallback for prompts
without the boundary), preventing embedder prose or HERMES_ENVIRONMENT_HINT decoys
from shadowing the real platform and falsely suppressing surface switch announcements.
Credit to @ehz0ah, who identified both correctness gaps on current main and
verified the regression scenarios.
* refactor(agent): surface-switch note lives in its own sibling; skip it where no sidecar exists
Move the six surface-switch helpers out of the conversation_loop facade
into agent/surface_switch.py (AGENTS.md: new behaviour goes in a topical
sibling), and fold the review findings on #104494:
- MoA and codex_app_server turns never stamp the api_content sidecar, so
the staged note could not be read back from the transcript and was
re-sent on every turn after a switch. Those modes now skip the note
(stored prompt still reused).
- The announced surface was parsed with split(".") — a plugin platform
with a dot in its name would never compare equal and re-stage the note
every turn. The note now closes the name with a fixed terminator.
- One identity-line parser (identity_line_value) shared by
_stored_prompt_matches_runtime and the switch detector instead of two
copies of the runtime-boundary/rpartition logic; tool names via the
existing tools.mcp_tool_agent._def_name; the transcript scan is bounded
to the last 200 rows (it ran every turn over the whole history).
- consume_surface_switch_note reduced to a plain pop; developer-guide
prompt-assembly.md updated (Platform is no longer an identity field);
17 new tests trimmed to 10 (same-shape pin/retire variants folded).
Restoring Platform as an identity field still turns 5 tests red.
* simplify(agent): surface switch — reuse flatten_message_text / agent_tool_names / one runtime-boundary split
- _transcript_row_texts re-implemented agent.message_content.flatten_message_text
and the api_content sidecar rule; the note can only land on a user row,
so the transcript scan now skips assistant/tool rows (the bulk of the bytes).
- Three sites computed "names of agent.tools"; tools.mcp_tool_agent gains
agent_tool_names() used by the switch note and conversation_loop, which
also stops importing the private _def_name across modules. The name list
is only captured when a switch was announced.
- split_runtime_boundary() is the single owner of the runtime-block
rpartition/END check for both identity_line_value and
_stored_prompt_matches_runtime.
- platform_surface_hint was a public alias of _platform_hint; the function is
now platform_hint (its docstring pointed at the pre-move module).
- consume_gateway_turn_context_notes and consume_surface_switch_note share
_pop_turn_note so the two one-shot channels have identical semantics.
- platform check hoisted above the transcript scan.
* fix(agent): row-addressed api_content backfill for pre-persisted user turns (#102194)
The api_content sidecar ('persist what you send') preserves prompt-cache
stability across turn boundaries by persisting the exact API-bound bytes
(including memory-manager prefetch, plugin injections, and API-only notes)
and substituting them on replay.
When a user turn was already materialized in the database before the
sidecar could be composed (in-place preflight compaction or a close/early
flush racing the prologue on the CLI path), the turn-start crash persist
marker-skips that message. Previously, the backfill was gated strictly on
in-place compaction (_preflight_compressed and _last_compaction_in_place),
so racing CLI flushes left api_content = NULL in SQLite and broke prompt
caching on subsequent turns (#102194).
Positional approaches (such as #102239 and #102286) using LIMIT 1 on the
newest active user row are unsafe: repeated common inputs ('ok', 'yes',
'continue') cause the backfill to match and overwrite the PREVIOUS turn's
row with the new turn's sidecar, corrupting history and breaking cache parity.
Resolve all landing blockers and review feedback from #102411:
1. Bounded state owner (Sahilvishnaliya):
Add SessionDB.set_message_api_content(session_id, row_id, content, api_content)
to SessionMessagesMixin in hermes_state_messages.py instead of growing
hermes_state.py. Update set_latest_user_api_content docstring with durable
warning on the positional hazard.
2. API-only turns & durable content selection (ehz0ah):
When a pre-flushed clean input has an API-only difference (e.g. voice
prefix or model-switch note):
- Retain the differing API-facing bytes as api_content even when no
new memory or plugin context was injected.
- Derive the durable content guard using _override_replaces_content so
the SQL 'content IS ?' guard matches the clean override text stored
in the DB row rather than the restored wire text.
3. Turn prologue gating (_row_id) & fail-closed store duck-typing (ehz0ah):
In agent/turn_context.py::_stamp_api_content_sidecar: check _row_id on
the live user dict (stamped by _insert_message_rows and synced by
sync_flushed_message_markers). If valid (positive int, not bool), address
by exact ID. Do NOT fall back to positional matching when a row ID is
present: if an external or custom wrapper lacks set_message_api_content,
fail closed and skip rather than corrupting a neighbouring row. If absent
but in-place compacted, fall back to positional update. On normal turns,
skip the backfill entirely (single atomic INSERT).
4. Real lifecycle test coverage (salch-cred, ehz0ah):
Comprehensive tests in tests/agent/test_api_content_row_addressed_backfill.py
covering store guards, surrogate scrubbing, gate non-arming, older identical
row protection, real close-flush row_id synchronization, API-only clean
override preservation with exact wire replay, and duck-typed store fail-closed
verification when set_message_api_content is absent.
Fixes #102194.
Closes #102411.
* fix(agent): read the sidecar row id under the session persist lock
_stamp_api_content_sidecar read _row_id without holding
_session_persist_lock. A close/early flush holds that lock while it
commits the row and only afterwards writes _row_id back onto the live
dict; a stamp that ran in between saw no id and skipped the backfill,
the flush finished with api_content = NULL and marked the message
persisted, and the turn-start persist skipped it — the row kept the
wrong bytes with no writer left to fix it.
Run the _row_id read and the DB backfill under the (re-entrant) lock,
re-checking _row_id after acquiring it. Race reported by @ehz0ah on
Co-authored-by: sal <141555468+salch-cred@users.noreply.github.com>
#102411; same fix shape as @salch-cred's follow-up on #103721.
* refactor(agent): one durable-row rule for the flush and the sidecar stamp; trim tests
The turn-start stamp had grown its own copy of the "what does the current
user row hold" rule (persist override = clean transcript, live content =
wire bytes = sidecar when they differ) that _db_flush_row already
implements. Two copies drift; extract durable_user_row_content() in
session_persistence and call it from both.
Also: reuse _persist_lock() instead of a third open-coded lock/nullcontext
ladder; drop the hasattr guard on set_latest_user_api_content (it predates
this fix and exists on every SessionDB); cut the comment to the WHY;
trim the new test file from 18 cases to the 7 invariants (real close
flush E2E, repeated-"ok" positional protection, API-only pre-flushed
turn, normal path writes nothing, compaction keeps positional, store
guards). Still 3 red / 4 green when agent/turn_context.py is swapped
for main's copy.
* simplify(agent): sidecar backfill — drop the hasattr guard and the duplicated row-id predicate; tests 7→6
_session_db is always a SessionDB (agent_init / delegate_tool), so the
"fail closed on a store wrapper" hasattr was defense around code that
cannot fail; the store's own guard binds the value into SQL, so the
prologue only needs the sibling idiom isinstance(_row_id, int) that
session_persistence and transcript_repair already use. The positional
hazard is explained once, on set_latest_user_api_content. The in-place
compaction test duplicated test_api_content_sidecar's
test_inplace_compaction_backfills_sidecar_into_db verbatim (its row_id
parameter was never varied); dropped, as was the positional-helper tail
of test_older_identical_row_is_untouched already covered there.
* chore: map contributor email for @0xalydev (#103581 salvage)
* fix(agent): inherit parent's full tool surface on review fork for cache parity (#103579)
Ensure unrouted background_review forks inherit the parent's full advertised
tools[] surface. Without this, skip_memory=True caused memory-provider tools
(e.g. fact_store/fact_feedback) and dynamically injected plugin/late MCP tools
to be omitted from the fork's tools array, breaking byte-exact prefix-cache parity
and incurring full cold-read costs on providers where tools are part of the cache key.
Inheriting the full parent tools array preserves complete prefix cache parity
while execution dispatch remains strictly bounded by the thread tool whitelist.
* fix(background-review): freeze review fork tool snapshot generation against compaction refresh
Freezes review_agent._tool_snapshot_generation to _FROZEN_TOOL_SNAPSHOT_GENERATION
(2_147_483_647) when inheriting the parent tool surface for same-model cache parity.
When in-place compaction boundaries trigger refresh_agent_mcp_tools(content_aware=True),
the staleness guard in _publish_tool_snapshot refuses the rebuild (snapshot_generation < published_gen),
preventing agent.tools from being reconstructed from the raw registry and preserving
inherited memory-provider and late tools across compaction boundaries (#103579).
Adds unit regression test verifying tool preservation across content_aware refresh.
Co-authored-by: joaomarcos <joaomarcosdias444@gmail.com>
* fix(background-review): inherit and freeze empty parent tools list for cache parity (#103579)
Copy and freeze review_agent._tool_snapshot_generation even when parent.tools is an empty list ([]). Previously, the truthiness check (\
ot parent_tools\) caused an empty parent tool surface to be skipped, allowing newly available late MCP or plugin tools to be retained on the review fork and leaving its snapshot generation unfrozen. This broke the byte-parity contract when no tools were active on the parent.
Returning early only when parent_tools is not an instance of list or tuple guarantees that an empty tool snapshot is faithfully inherited and frozen. Adds dedicated regression test test_unrouted_review_fork_inherits_empty_tool_surface.
* refactor(background-review): collapse the tool-surface copy to the agent_init shape; 2 tests
agent.tools is always a list (agent_init assigns it from
get_tool_definitions) and every entry is a well-formed function schema,
so the isinstance ladder over parent/entry/function/name guarded shapes
that cannot reach this helper. Use the same two lines agent_init uses;
`or []` keeps the empty-surface contract from the previous commit.
Docstring cut to the WHY (the between-turn refresh note described the
other guard). Tests trimmed to the two invariants: inherited tools
survive the compaction-boundary refresh (deep-copy isolation folded in),
and an empty parent surface is copied and frozen. Literal sentinel
asserts replaced with the constant.
* test(background-review): assert the behaviour, not the sentinel
The compaction-refresh and empty-surface tests pinned
_tool_snapshot_generation == _FROZEN_TOOL_SNAPSHOT_GENERATION next to
the behavioural assertion (refresh returns set(), tools unchanged). The
behaviour is the contract; the constant is the mechanism.
* chore(contributors): map sgarrand@gmail.com -> sgarrand
Scott Garrand (@sgarrand) identified the NixOS /bin/true systemd-probe bug
first in #102587; the salvage of #105436 credits him with a Co-authored-by
trailer, so the release script needs his mapping.
* fix(process-registry): use portable /bin/sh probe for systemd-run scope availability (#105365)
* test(process-registry): mark systemd probe tests linux_only
* test(process-registry): exercise the portable probe payload
Execute the selected no-op rather than freeze its spelling, while rejecting
/bin/true to model the NixOS failure. Mark the regression Linux-only and
retain the current user-bus environment handling.
Consolidates the earlier NixOS scope-probe report and fix in #102587 with
the PATH-independent payload from #105436. The fallback resolver is not
needed when /bin/sh is used directly.
Co-authored-by: Scott Garrand <sgarrand@gmail.com>
* fix(gateway): guard display config reads against present-but-null values
A profile config with a bare `display:` key (present-but-null) made
`user_config.get("display", {})` return None — the {} default only
applies when the key is missing — so the chained
`.get("memory_notifications")` in _wire_turn_agent_callbacks raised
AttributeError on every real gateway turn (Discord / cron). Oneshot
turns bypass this wiring, which masked the crash during smoke tests.
Use the same `or {}` guard the other gateway display readers
(display_config.py, runtime_footer.py) already apply, and fall back to
the documented default "on".
Fixes #105674
* test(gateway): fold the null/missing display cases into one parametrized test
* chore: map philmossman's contributor email (#105704 salvage)
* fix(cron): don't stamp the next occurrence on an off-tick manual run
claim_job_for_fire() derives the occurrence identity from next_run_at
before the same function advances it. On a scheduler tick next_run_at is
the occurrence being run, which is correct; on an off-tick manual run it
is the NEXT occurrence, so the execution is stamped with the identity of
a slot that has not happened yet. _job_is_due() then finds a completed
execution carrying that identity and skips the real slot, returning
before the last_dispatch write — no error, no log line, no dispatch
record.
The manual flag already guards this and both _job_is_due() and
claim_job_for_fire() honour it; the agent-facing run-now path never
declared itself. Add a keyword-only manual= parameter and pass it from
_claim_for_manual_run(). Deliberately not force=True: force also calls
_activate_job_record(), which would resume a paused or disabled job, and
the run-now tool depends on continuing to refuse those.
The local flag is renamed to manual_fire so the new parameter is not
shadowed inside the apply closure, which would raise UnboundLocalError.
Three existing tests in tests/tools/ pinned the old call signature via
assert_called_once_with; they now pin manual=True, so dropping the flag
again fails loudly rather than silently reintroducing the skip.
Restores the intent stated in #104790 — the column records the scheduled
instant an execution was claimed for, and an off-tick manual run was
claimed for none.
Fixes #105690
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(cron): the dashboard "Trigger" run-now no longer stamps the next occurrence either
Second entry of the same bug class: POST /api/cron/jobs/{id}/trigger →
_fire_cron_job_for_profile → CronScheduler.fire_due → claim_fire built its claim
without `manual`, so an off-tick run from the web UI stamped the future slot exactly
like the tools path #105704 fixes. fire_due/claim_fire gain `manual` (forwarded only
when set, mirroring `force`, so third-party providers keep working) and the dashboard
trigger passes it when the provider's signature accepts it. Webhook and misfire
catch-up fires run the slot that is due and keep the stamp.
Also drops the base-green tick-stamp test (the same contract is pinned by
tests/cron/test_scheduled_occurrence.py) and documents `manual` vs `force`.
* chore: map tkaufmann's contributor email (#105463 salvage)
* fix(agent): classify local-inference memory-ceiling rejections as overloaded
oMLX/MLX prefill memory-guard rejections name an allocation peak in BYTES but
close with "Reduce context length", so _CONTEXT_OVERFLOW_PATTERNS claims them
and the turn enters the compress-and-shrink loop. Compression cannot lower a
prefill peak — the prompt is usually far below the window — so it burns the
compression budget, re-hits the wedged server on every attempt and ends in
"Cannot compress further" plus a destructive session reset.
Classify them as `overloaded` instead: retry with backoff, no compression, no
session reset (mirrors 503/529 recovery).
The guard runs before the overflow check AND before the usage-limit
disambiguation. The second ordering matters more than it looks: "memory limit
exceeded" contains "limit exceeded", so a status-less rejection — a proxy that
flattened the body — is currently classified as `billing` and rotates a
healthy credential.
Sites covered:
- _OVERFLOW_AS_5XX_RULES, which _400_TAIL_RULES extends → 400, 500, 502,
503, 529
- _MESSAGE_HEAD_RULES for the status-less path (ahead of usage-limit)
- _ERROR_CODE_VERDICTS for the structured oMLX codes
- _classify_400, because _by_status runs before _by_error_code, so a body
whose wording a proxy stripped would otherwise fall through to
format_error
Every pattern names memory/allocation in bytes, never a token or window count,
so the list stays disjoint from _CONTEXT_OVERFLOW_PATTERNS. Both oMLX wordings
are kept: 0.5.6 says "Prefill would require ~13.87 GB peak", 0.5.7 reworded it
to "predicted peak would require/exceed" and both are in the field. A test
pins that a genuine window overflow still compresses.
Refs #52261. Supersedes #52289, which predates the classifier rewrite and can
no longer be merged.
* test(agent): fold the five memory-ceiling cases into one parametrized invariant
Same coverage (5 red on main, 1 guard green), one contract test instead of five.
* fix(agent): isolate periodic scheduler callbacks from blocking siblings (#102574)
* refactor(agent): one _requeue for the three heappush sites; timing test asserts ordering, not a 0.3 s bound
Fold the identical heappush(...) into PeriodicScheduler._requeue; notify() instead of
notify_all() now that the scheduler thread is the only condition waiter; drop the
PR-history paragraph from the module docstring (the commit carries it).
Tests: the blocked-sibling test asserted `sibling_ran.wait(0.30)` — a wall-clock bound
under the repo's ≥2 s flake floor; it now asserts the sibling fired while the blocker
still held its worker. The worker-start-failure fake keys on this scheduler's own
_run_callback rather than the global thread-name prefix so a leaked handle on _DEFAULT
cannot consume the single injected failure. The base-green no-overlap test is dropped
(it does not prove the fix).
* fix(agent): scope background review memory access to its trigger (#105921)
The review fork's tool whitelist granted the whole memory toolset
whenever the profile had memory enabled, regardless of which nudge
fired, so a skill-nudge fork held remove/replace on MEMORY.md it was
never asked to use; combined with the memory tool's near-limit
'consolidate now' hint, an unattended fork deleted standing rules with
no user in the loop.
- Pass review_memory from spawn_background_review_thread through
_run_review_in_thread/_run_review_fork into _review_tool_whitelist;
a skill-only review no longer gets the memory tool at all.
- Fail-closed operation gate in memory_tool: a background-review fork
may add, never replace/remove (single or in a batch) — consolidation
decisions reach a human via the review summary instead.
- Keep the deny/prompt wording in sync with the whitelist so a
memory-less review doesn't advertise memory.
* fix(review): distinguish explicit /refine from unattended reviews and surface staged consolidations
Review follow-up on #105944 (#105921):
- explicit /refine forks now run under the refine_review write origin
(explicit flows from the CLI/gateway handlers through
_spawn_background_review_now and spawn_background_review_thread down
to build_cache_parity_fork), so a user-requested review keeps the
full memory operation set; only automatic reviews stay behind the
unattended delete gate.
- the unattended delete gate now stages the denied replace/remove (or
whole batch) into the pending store instead of dropping it: the
fork's own review summary is never published, so a plain denial lost
the consolidation request with no surfacing path. The staged proposal
carries a proposal_staged marker that summarize surfaces as an action
line, and a staging failure still fails closed to a plain denial.
- regression tests: explicit-path origin pass-through, refine_review
keeping replace working, near-limit denial end to end (add rejected
by budget -> replace staged -> proposal surfaces, store unchanged).
* fix(review): keep /refine under the background_review origin; attendedness is its own flag
The salvaged commit forked an explicit /refine under a new "refine_review" origin so
the memory delete gate would not treat it as unattended. But is_background_review()
is the key for every other review guard — skill_manager_guards (curator-owned-only,
read-before-write), skill_manager_tool (archive instead of rmtree), skill_ledger
actor, write_approval staging, the [auto] tag — so a /refine fork silently escaped
all of them.
Carry attendedness separately: the fork keeps origin "background_review" and sets
_review_attended; turn_context binds it beside the origin ContextVar; the memory
gate keys on the new is_unattended_review(). Also run the gate AFTER
_validate_single_op / the operations list check, as memory_tool's own docstring
requires, so an invalid replace is rejected now rather than staged and failed at
approve time.
* fix(sessions): serialize fresh FTS bootstrap
* fix(sessions): restore trigram after deferred bootstrap
* refactor(state): drop the table-exists probe made dead by the early return above it
* chore: map portavales's contributor email (#105694 salvage)
* fix(loop): re-anchor current_turn_user_idx after the alternation repair merges rows
prepare_iteration() runs repair_message_sequence_with_cursor() before each API
call; the repair merges adjacent user rows in place (after a compaction, the
role=user summary sits next to the protected first user message). The loop's
current_turn_user_idx was recorded at turn start, so after a merge it points
past the current user row: the per-turn context injection (prefetch/plugin
context) silently misses it, and hosts that settle the transcript by this index
(hermes-webui) write the current user turn to the FRONT of the context —
rewriting the prompt's leading messages every turn (0% prefix-cache hits at
200K+ tokens, ~100 s re-prefill per turn) and duplicating the user's question.
The in-loop compression restart path already re-anchors; do the same after a
repair that changed the list: reanchor_current_turn_user_idx (last user row
carrying this turn's text), return the index through the IterationPrep verdict
so the loop state picks it up, and mirror it into agent._persist_user_message_idx,
which hosts read when the result carries no index. The new phase parameters
default to None so direct callers keep their signature.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013gp366ijf39n4UUtJhZuMh
* feat(loop): export {turn_id, current_turn_user_idx} on every result envelope
Hosts that settle their own transcript by index (hermes-webui) cannot prove which
row of result["messages"] is the current user turn once this loop rewrote history
(alternation repair, compaction, post-turn micro-compaction): the instance-side
_persist_user_message_idx predates those rewrites, and a text match relabels an
identical historical prompt and claims its old answer. Only the producer can
assert the coordinate against the exact list it returns.
run_conversation now wraps the turn (_run_conversation_turn) and stamps the pair
through export_current_turn_boundary on every envelope that leaves the loop
(success, partial/error, interrupt, retry-exhausted, tool-limit, preflight
timeout, codex runtime), computed on the final messages after finalize_turn and
micro-compaction. The pair is exported only when the addressed row is this turn's
user message verbatim (reanchor's last-match rule); a rewritten row exports
nothing so hosts fail closed. The final index is mirrored into
_persist_user_message_idx for the persist override.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013gp366ijf39n4UUtJhZuMh
* test(agent): prove the re-anchor through prepare_iteration; reuse the compaction _reanchor
The salvaged regression test exercised only repair_message_sequence and
reanchor_current_turn_user_idx — pre-existing helpers — so reverting the fix left it
green. It now drives prepare_iteration on a real AIAgent with adjacent user rows and
asserts the returned index addresses this turn's row and mirrors into
_persist_user_message_idx (red without the re-anchor: IndexError).
Both re-anchor sites (repair and compression restart) call
turn_context_compaction._reanchor instead of inlining "reanchor + mirror", so they
cannot drift. The export tests fold into one parametrized invariant plus the
run_conversation envelope test; the WHAT-restating comment shrinks to the WHY.
* fix(gateway): preserve shared MCP visibility across profile reloads
* fix(gateway): register shared MCP tools per profile
* fix(mcp): a profile only adopts a shared connection whose credentials match its own config
_same_server_route compared config_fingerprint alone, which by design excludes
env/headers/auth (so the schema cache survives a token rotation). Profile B with the
same URL but different headers/env therefore adopted profile A's live connection and
called tools as A. _connection_identity = route fingerprint + env + headers + auth mode,
used by both the adopt and the stale-removal checks.
Also collapses the three writers of _server_tool_scopes to two: the adoption loop
re-implemented in mcp_tool_discovery._select_new_servers is dropped —
register_connected_into_current_scope (which runs first in register_mcp_servers) is
the single adopter, and _register_candidates records scope for freshly registered tools.
* fix(gateway): ledger-bracket the queued-lane final send
When a follow-up is queued behind a turn, the first response is delivered by
the queued lane before the follow-up runs. That lane called adapter.send bare
and discarded the result: no delivery-ledger obligation was recorded, so a
final refused there (flood control, a transport that had just died) was lost
for good. Neither the boot sweep nor the runtime redelivery could see it and
the follow-up ran as if the answer had landed.
Route the queued lane's text send through the adapter's _send_final_text, the
same ledger-bracketed method the normal lane uses: the obligation is recorded
before the send under the id the normal lane would compute for the same turn,
the result finalizes it, and the reply is marked notify-worthy like every
other final. The reconcile-by-edit path is unchanged; adapters without the
base contract and sends without a session key keep the plain send.
* fix(gateway): key the queued-lane obligation on the raw inbound id
The queued lane's ledger bracket used the reply anchor as the obligation's
message reference. The anchor is None wherever replies are not used (Telegram
forum topics, Slack reaction handoffs), so two turns in one topic answering
with the same text shared an obligation id and the second record overwrote
the first's outstanding row; the id also differed from the normal lane's.
The lane now runs the adapter's record / send-with-retry / finalize sequence
itself, keyed on turn_ctx.inbound_message_id like the normal lane, while the
anchor stays the reply target. Tests cover the forum-topic identity, the row
being `attempting` while the send is in flight, and the call site passing
both ids.
* fix(gateway): carry the raw inbound id through a chained queued turn
Round-2 review. _run_agent_deliver_first_response passed the turn's inbound id to
the queued lane, but the recursive _run_agent for a chained follow-up did not, so
the chained turn ran with inbound_message_id=None. In a Telegram forum topic (no
reply anchor) two chained follow-ups answering with the same text would then key
their queued-final obligations on None and collide. The recursive call now carries
pending_event's raw message id, with a test on the chained path.
* test(gateway): let the queued-native-image fake accept the persist kwargs
Now that a queued follow-up carries its raw inbound id, the gateway passes
persist_user_platform_id to run_conversation for that turn (run_turn_runner.py
only adds it when inbound_message_id is set). The real AIAgent accepts it; the
test's fake did not. Accept **kwargs, matching the real signature.
* fix(gateway): ledger a queued chain's terminal reply under its own inbound id
The outer final send is bracketed by the adapter against the event that OPENED
the chain, so a terminal reply was recorded under the first message's id. When
two turns of one chain answered with the same text, the terminal reply computed
the earlier row's obligation id, replaced its outstanding row and marked it
delivered, so a first reply the platform had refused was never redelivered.
MessageEvent gains a documented ledger_message_id that the obligation hash
prefers, the queued follow-up returns the terminal turn's inbound id (innermost
wins on nested chains), and the handler sets it before the adapter brackets the
send. Reply routing is untouched: the anchor still comes from the event.
Three of the four new tests fail without this change; the whole tests/gateway
suite shows the same failure set before and after.
* refactor(gateway): one send_final_ledgered bracket for the normal and queued final lanes
The queued lane re-implemented _send_final_text's record / send-with-retry / finalize
sequence by duck-typing four private adapter members from gateway/. Lift the bracket
into a public BasePlatformAdapter.send_final_ledgered(event, session_key, text,
metadata, *, reply_to, is_ephemeral_response); _send_final_text keeps only the
ephemeral-delete tail on top, the queued lane calls it with the inbound-id ledger event.
ledger_message_id is a real dataclass field now, read directly.
Tests trimmed from 18 to 8 invariants (bracket recorded+delivered; flood refusal stays a
failed ledger row; forum-topic identity; plain adapter keeps plain send; normal-lane
parity; chained/terminal/deeper-chain inbound ids). Still 6 red with main's
run_notifications.py swapped in.
* fix(state): guard close-time checkpoint for replaced/deleted-generation handles (#105670)
- close() and _try_wal_checkpoint() now skip when _db_replaced or _db_wal_generation_lost
(previously only _db_corrupt was checked) — prevents checkpointing stale-generation frames
into the main DB, which is the shutdown-time damage reported in #105670
- _halt_if_db_generation_changed() calls _disable_close_time_checkpoint() alongside the flag
set (3.12+: disables SQLite internal last-connection checkpoint too)
- Regression tests: halted handle must not run explicit PRAGMA checkpoint on close(),
halt must call setconfig(NO_CKPT_ON_CLOSE), periodic _try_wal_checkpoint() must skip
* refactor(state): drop the ruff-format reflow from the checkpoint guard, keep the ~20 semantic lines
The cherry-picked commit re-wrapped hermes_state.py wholesale (+527/-131 for a
fix of about twenty lines). Restore main's layout and re-apply only the fix:
disable the close-time checkpoint on both generation-loss halts, gate the
periodic checkpoint on the sticky generation flags, and name the quarantine
reason at close.
* test(state): mark the checkpoint-guard tests linux_only instead of a bare skipif
AGENTS.md: a bare skipif(sys.platform != linux) is never listed by
scripts/ci/list_os_marked_tests.py, so the tests would run nowhere on the
OS lanes. The marker is the contract.
* fix(state): the deferred FTS rebuild retry is quarantined by the same rule as the checkpoints
retry_deferred_fts_recovery gated only on _db_corrupt ("mirrors _try_wal_checkpoint /
close") — after this PR it no longer mirrored them: on a replaced/lost-generation handle
the periodic housekeeping tick still ran FTS DDL/DML + commit, the same split-brain write
class as the #105670 checkpoint. One SessionDB._quarantine_reason() now decides for the
periodic checkpoint, close(), and the FTS retry, with the halt path's precedence
(replaced before generation loss) and the operator wording in one place.
Test: the periodic-checkpoint case folds into the close test (same setup), which now
also proves the FTS retry returns False without touching the file; the mutation with
main's schema sibling swapped in returns True (a rebuild ran).
* chore: map albert748's contributor email (#104444 salvage)
* fix(agent): persist /steer as a standalone user message
`apply_pending_steer_to_tool_results` used to smear the steer text onto
the last `role:tool` message's content. That tool row had already been
flushed to the session store and carries `_DB_PERSISTED_MARKER`; the
append-only persistence never rewrites it, so the replayable transcript
diverged from the live request bytes at the injection point — resumed
sessions (surface switch / process restart / background-review close)
missed the provider prompt cache (75-85% hit) and the user's mid-run
instructions were never part of the durable history.
The steer is now emitted as a standalone `role:user` message (marker
text preserved):
- role alternation stays legal: assistant(tool_calls) -> tool -> user is
the documented 'user jumped in mid-run' pattern that
`repair_message_sequence` deliberately keeps;
- the appended dict carries no `_DB_PERSISTED_MARKER`, so the next
`_flush_messages_to_session_db` writes it to the session store —
transcript bytes and replayed history finally agree, and the steer
becomes searchable/retrievable like any other user message;
- the no-tool-result fallback (interrupt) still requeues the steer, which
the caller then delivers as a normal next-turn user message.
Tests: TestSteerInjection updated for the new shape plus a persistability
assertion (no marker => flushable); tool-batch-segmentation malformed
scenario updated. steer + segmentation suites: 67 passed, 1 skipped.
* test: keep the steer suite on the canonical patch targets, not PLUGIN-COMPAT pointers
The cherry-picked commit carried an unrelated hunk repointing three patch()
targets back to run_agent.* — those are PLUGIN-COMPAT re-exports, off limits
in-tree (scripts/check_compat_pointers.py; removed 2026-09-14). Keep main's
model_tools.* / agent.process_bootstrap.OpenAI targets.
* fix(agent): the pre-API-call /steer drain also stops smearing the persisted tool row
Second site of the same bug class #104444 fixes in apply_pending_steer_to_tool_results:
_inject_steer_into_newest_tool_result (the drain that runs when a /steer lands during an
API call) mutated the newest role:tool row in place. That row was already flushed
append-only, so the replayed history diverged from the live request bytes at the
injection point and broke the prompt cache exactly like the post-batch path.
Deliver it the same way: a standalone user row inserted right after the newest tool
result (not yet persisted, so the next flush writes it to the transcript). Restash when
there is no tool row yet, unchanged. Stale comments claiming steer lands "in the newest
tool result" and agent/AGENTS.md's alternation rule now describe the real shape.
* fix(agent): a persisted /steer row survives the next prompt's alternation repair; typed for history
Both steer sites now build the row through one helper, prompt_builder.steer_user_row:
a role:user row with display_kind="steer" and no leading blank lines. The alternation
repair (_merge_consecutive_users) skips a steer-typed prev row, so a run that ended
right after a steered batch (Ctrl-C, interrupt) does not get the next real prompt
merged INTO the already-persisted steer row — which would have rewritten it in place
and re-broken live≠replay parity, the exact class this PR fixes.
TUI/desktop history projects the steer row as the user's own words instead of the
model-facing marker wrapper; 'steer' joins the display_kind union. The compression
anchor scan keeps its tool-row branch for transcripts persisted before this change and
its docstring says so.
* fix(tui): resolve default profile session names
* fix(tui): preserve names for custom profile homes
* fix(tui): fail closed on unavailable profile targets matching custom root basenames
* test(tui): add coverage for custom default roots, real session db stamping, and sibling isolation
* fix(tui): a real named profile "hermes" is not swallowed by the legacy-basename alias
"hermes" matches the profile-id regex, so canonicalising it unconditionally at the RPC
boundary misrouted a genuine <root>/profiles/hermes to the default profile. Alias only
when no such named profile exists; ".hermes" can never be a real id and stays aliased.
Also: profile_name_for_home collapses its duplicated pre/post-resolve block into one
loop over (path, resolved path) and drops the bare "parent named profiles" fallback
that bypassed named_profile_home's root check; _profile_home goes back to main's
single resolve() comparison; the symlink-loop assertion in the target-unavailable
test is no longer wrapped in a try/except that could silently skip it.
* fix(profiles): a stored <root>/profiles/<name> home names its profile even when the root carries no markers
CI: tests/test_tui_gateway_server.py::test_ensure_session_db_row_stamps_profile_name used a bare tmp
root; profile_name_for_home fell through to None and the row was stamped default. The stored home
is authoritative (its owner resolved it), so the profiles/<name> shape is sufficient.
* fix(cli): honor --resume in one-shot mode (#105892)
The -z exit path accepted --resume/-c in the parser but never forwarded
args.resume: every resumed one-shot turn silently started a fresh session,
so each wire request carried only [system, current user] and the model
lost all prior context (reported against Ollama/custom OpenAI-compatible
endpoints, but provider-independent).
Normalize session args (latest/title/--continue/--in + cwd restore) via
the chat path's _resolve_chat_session_args before the oneshot exit path
takes over, then load the resumed transcript in _run_agent through the
same contract the interactive CLI uses (compression-chain redirect,
safe-resume guard, session_meta filtering) and continue the existing
session id instead of creating a new one. An explicit --resume of an
unknown session now fails loudly instead of starting fresh.
* fix(cli): keep the resolved session id when a resumed oneshot session is empty
Review finding on #105957: `_load_resume_target` returned None for a
resolved session with no stored messages, so `hermes -z "hello" -c <title>
--create-if-missing` recorded the turn under a freshly minted session id and
the just-created titled session stayed empty. Preserve `resolved` unconditionally — the interactive /resume path keeps the selected id for an
empty session too; only the history replay is empty. Regression tests pin the
durable id for both a plain empty session and an empty compression-chain head.
* fix(cli): restore stored session runtime and reopen ended rows on oneshot resume
Review fixes (#105957):
- A resumed one-shot ignored the session's stored model/provider runtime:
_resolve_model_and_provider()/resolve_runtime_provider() ran before
_load_resume_target(), which only loaded the session id + transcript, so an
ambient config (e.g. openrouter/ambient-model) served the resumed transcript
instead of the stored route (custom:stored/stored-model). The stored runtime
is now applied before runtime resolution, with the same contract as the
interactive _restore_session_model(): stored model/provider/base_url/api_mode
replace the ambient choice unless --model was passed explicitly, and a
changed provider drops the ambient api_key so resolution re-fetches
credentials for the restored endpoint.
- Passing the resumed id to AIAgent did not reopen the already-ended session
row: end_session() only writes rows whose ended_at is null and the
existing-row upsert never clears the end fields, so the resumed turn was
recorded under a session that stayed closed and its new lifecycle boundary
was lost. _load_resume_target() now reopens the row (best effort), same as
the interactive resume does before continuing.
* refactor(cli): one stored_session_route for interactive and one-shot resume
_apply_stored_session_runtime was a line-for-line copy of the first half of
_restore_session_model (stored-model guard, session_gateway_runtime, bare-custom heal,
model/provider-changed check). Extract that pure decision into
cli_model_switch_mixin.stored_session_route and have both resume paths call it; the
one-shot keeps only the _ModelChoice mapping and the drop-ambient-key rule.
main.py stops re-normalising `resume` — _resolve_chat_session_args already did.
Tests trimmed from 20 to 13: near-duplicate unit tests of the private helpers go, the
end-to-end _run_agent contracts (stored runtime + reopen; explicit --model wins) and the
empty-session-keeps-id case stay.
* fix(cli): keep the no-stored-model early return ahead of the route read
CI: tests/cli/test_cli_resume_command.py builds bare HermesCLI objects without .model; the
refactor read self.model before the stored-model check the contributor's code made first.
* test(agent): the worker-start-failure test intercepts the callback worker again
`kwargs.get("target") is sched._run_callback` is always False (a bound method is a fresh object
per access), so the fake never returned Boom and the _dispatch failure branch went untested;
the test passed on the normal worker. Compare with == and assert the interception happened
(mutation: retiring the handle on start failure now fails the test).
The thread-count assertions sampled while per-fire workers were still live; quiesce every
handle with cancel(wait=) before sampling so the count is deterministic (AGENTS.md: timing tests
must not assume a quiet runner).
Follow-up to #106308.
* fix(loop): the turn-boundary export skips preflight-timeout envelopes and stops re-anchoring the persist index
Follow-up to #106312. _preflight_timeout_result carries the prior history without this turn's
user row (#7100); with a repeated prompt ("continue") the verbatim scan resolved to the
historical copy and exported it as this turn's proven boundary — the exact relabeling the export
exists to prevent. Nothing is exported for that envelope now.
The trailing `agent._persist_user_message_idx = idx` ran after finalize_turn had already flushed
the transcript, so it never influenced a persist and the next turn reset it: dead state, removed.
* fix(gateway): the ephemeral delete goes to the adapter that sent the final
Follow-up to #106316. send_final_ledgered resolved the live adapter internally and
_send_final_text resolved it a second time for _schedule_ephemeral_delete; a reconnect between
the two sent the delete to a transport that never owned result.message_id (the ownership rule
_final_delivery_adapter documents). The bracket now returns (result, adapter).
The queued lane carried the ledger identity through MessageEvent.message_id while the PR added
ledger_message_id for exactly that; it now uses the typed field, and the ledger read is
getattr-tolerant of duck-typed events (a missing attribute was swallowed as "ledger skipped").
* fix(state): VACUUM is gated by the same quarantine rule as the checkpoints
Follow-up to #106315. vacuum() ran PRAGMA wal_checkpoint + VACUUM + wal_checkpoint(TRUNCATE) on
self._conn with no quarantine check; the only guard it inherited (optimize_fts raising
DeletedWalGenerationError) was swallowed by its own try/except and the rewrite proceeded on the
split-brain handle. Mutation on main: vacuum() returned 2 and rewrote pages after the write stop.
* fix(agent): a /steer row is human input for every user-turn predicate
Follow-up to #106317. Typing the steer row (display_kind="steer") for the renderer and the
alternation-repair guard collided with the convention that any display_kind on a user row means
scaffolding: is_user_originated_turn / _is_actionable_user_turn / split_user_originated_turn
returned False for it (tail anchoring, auto-focus, dispatcher views, resume counts) while
_is_real_user_message returned True (anchor restoration) — the two predicate families disagreed
on the same row, and list_recent_user_messages (/undo, /rewind) skipped it in SQL. A steer
carries full user authority; the steer kind is now whitelisted in all four.
Also: the pre-API drain's requeue tail reuses _requeue_pending_steer instead of a copy; the TUI
history projection compares against STEER_DISPLAY_KIND; the steer() docstring describes the row.
* fix(state): guard vacuum() and optimize_fts() against quarantined SessionDB handles
A quarantined/replaced/split-generation handle must never run a full-file rewrite or an FTS5
'optimize': both read damaged or foreign pages and commit the result back, turning contained,
diagnosable corruption into an amplified one. Same guard _execute_write applies to every write.
Salvaged from #102092 onto current main: the _try_wal_checkpoint half landed via #106315's
_quarantine_reason(), so only the two rewrite sites remain.
* fix(auth): preserve independent same-account OAuth grants
* fix(auth): carry pool-row lineage into the provider-block heal
With account-identity matching gone, the providers.<id> block consolidation
only fired on shared token material. A historical fork (same copied pool-row
id, profile rotated, both pairs diverged) then healed the pool row into root
but left root's providers.openai-codex block on the spent pair; root's next
load_pool() re-seeds its device_code row FROM that block and undid the heal.
_HealPass now records that a profile pool row matched root by copied id or
shared tokens and passes that verdict to _heal_forked_provider_block, which
accepts it as lineage proof. No account-identity guessing is restored; an
independent same-account grant (no id/token match) is still left alone.
Follow-up to simpolism's #106177.
* fix: address 6 P1 findings from merged PR review threads
- repair_controller.py: build the retirement completion command through
_governed_command_prefix() (adds -P) instead of a bare `python -m`
invocation, matching the sibling identity command; an untrusted
exact-head PR worktree could otherwise get prepended to sys.path.
- cli.py doctor probe: report worker_completion_policy failed whenever
HERMES_SAFE_MODE is active, since dispatched workers inherit it and
PluginManager skips all plugin discovery under it regardless of what
the profile config declares.
- worker_contract.py: default a manifest's missing `name` to its
directory name before comparing, matching parse_manifest_file()'s
actual runtime behavior, so a name-less override plugin.yaml is no
longer treated as absent.
- worker_contract.py: fail closed when a profile's plugins.enabled/
disabled list still contains an unexpanded ${VAR} reference, since
expanding it against doctor's own environment doesn't guarantee the
dispatched worker's .env resolves it the same way.
- methods_profiles.py: catch SystemExit (not just Exception) around
_write_raw_config_values(), which raises SystemExit for managed-scope
keys; the shared TUI/Desktop/dashboard RPC backend must not exit on a
refused profiles.configure write.
- config.py _preserve_env_ref_templates(): match a modified, reordered,
unnamed list entry to the loaded item it most structurally resembles
instead of the raw item at its new output position, so a sibling's
unchanged ${VAR} template isn't dropped into plaintext on save.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix(tui-gateway): /review shows its reviewer in the Desktop subagent stack
`slash.exec` runs on the RPC pool, outside any turn, so `/review` dispatched the
reviewer with no HERMES_UI_SESSION_ID and no steer authority bound. delegate_task
registered the child with `owner_session_id=None`, `subagent.list` (owner-scoped)
returned nothing for the parent session, and the Desktop status stack's 5s
snapshot poll reconciled the live `subagent.start` row away — the user saw only
"Review started. Results will return here." with no subagent card.
Bind the same session identity a turn binds (`_set_session_context(...,
ui_session_id=sid)` + `_current_runtime_session_record`) around `start_review`,
and clear it after. The reviewer now registers under the parent sid with the
request transport as authority, so `subagent.list`, steer, stop and the Desktop
roster all see it.
Live repro (tui_gateway stdio, real OpenRouter reviewer):
before: registry owner_session_id=None, owner_transport=NoneType;
subagent.list -> {"subagents": []}
after: owner_session_id=<parent sid>, owner_transport=StdioTransport;
subagent.list -> [{"goal": "Review recent work", "status": "running", ...}]
* fix: address remaining P1 findings (dispatch generation, completion guard, context compressor)
- feedback_retirement.py: extend governed retirement to pr_local_ci
receipts too -- audit-pr rejects a non-OPEN PR identity outright, so a
card whose PR closes mid-audit had no other path to clear its pending
ledger row and stayed stuck forever.
- controller.py: reintroduce _dispatch_generation() (lost track of
ClaimLease.reopened during an earlier merge -- version > 1 is the same
signal) and wrap all 3 create_or_get_task() call sites, so a reclaimed
dispatch gets a fresh Kanban identity instead of returning the
pre-closure done card.
- controller.py _is_staged_auto_dispatch_task(): also require no real
"blocked" lifecycle event, so a repair worker's legitimate kanban_block
call (same status/idempotency-prefix/evidence shape as a never-run
staged card) isn't misclassified as a failed staging promotion and
bounced back to ready.
- kanban_completion_policy.py: load the bundled github_pr_feedback
package by file path instead of a bare import, so the control-plane
completion-guard fallback works even in a dispatched worker profile
that doesn't itself enable the plugin (previously ModuleNotFoundError,
uncaught).
- context_compressor.py: scan the actual handoff-expanded window
(scan.tail_start) for the current-task assignment summary instead of
the initial compression window, so a newer assignment carried by a
later-consumed handoff isn't shadowed by a stale in-window match (or
missed entirely).
- test_run_agent.py: fix a NameError from an earlier merge -- an
undefined mock_record_failure reference where the test actually needs
hermes_cli.kanban_db.block_task patched.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix(desktop): SSH remote backend stops following the host's sticky active_profile
A Desktop-owned `hermes serve --isolated --ssh-session-token-file ...` child
is spawned with an explicit `--profile <name>` when the connection names a
remote profile, and with no flag for the remote root home. Without the flag,
`_apply_profile_override` read the remote host's sticky `active_profile`
file and re-homed the backend into whatever profile the user last selected
on that machine's CLI. Settings then read one config.yaml while the remote
gateway wrote another, so model picks and toggles "didn't stick".
Treat the SSH token flag as a fixed-identity marker, the same way
supervisor-launched gateway children are (#74872): a Desktop backend's
profile is chosen by the client, never by the host.
Live repro (before/after, temp HERMES_HOME with active_profile=foo):
serve --isolated --ssh-session-token-file ... hermes_home=<root>/profiles/foo -> <root>
same + --profile foo hermes_home=<root>/profiles/foo (unchanged)
serve (no token file, user CLI) hermes_home=<root>/profiles/foo (unchanged)
* fix: verification evidence ledger is inert while verify_on_stop is off
The ledger in verification_evidence.db exists only to feed the verify-on-stop
guard, but the recorder kept running on every foreground terminal command and
every file edit after #53552 turned the guard off by default. Users who never
opted in still accumulated a multi-MB database (7 MB / 4.6k rows on one install).
Every ledger entry point (record_terminal_result, record_verify_run,
mark_workspace_edited, verification_status) now checks…
What does this PR do?
Local-inference providers (oMLX / MLX with a memory guard, and similar
Metal/CUDA setups) abort a request when the prefill memory peak exceeds a
GPU/unified-memory ceiling. Their rejection text often suggests "reduce context
length" / "reduce context size", which collides with the context-overflow
patterns — so Hermes classifies a memory-ceiling 400 as
context_overflowand routes it into the compress → shrink-context → retry loop. Compression
cannot lower a prefill memory peak (the conversation is tiny — ~5.7k tokens in
the report), so it exhausts
max_compression_attempts, the compression callitself re-hits the wedged server, and the loop ends in "Cannot compress
further" → destructive session reset.
This PR adds a
_MEMORY_CEILING_PATTERNScheck that runs before thecontext-overflow check at every classification site, classifying these as
FailoverReason.overloaded(transient, retry-with-backoff, no compression, noreset) — the same "checked BEFORE context_overflow" guard pattern already used
for multimodal / image-too-large / request-validation 400s.
overloadedmirrors the existing 503/529 recovery:
retryable=True,should_compressdefaults to
False, and it is in the retryable set so the loop never enters theclient-error abort/reset path.
The same wall reaches the classifier on five different routes, and all five
now share one predicate (
_is_memory_ceiling) and one recovery annotation(
_memory_ceiling_result), so the treatment cannot drift between them:context_overflow→ compress → resetoverloaded, retryable, fallbackAPIError)context_overflow/billingoverloaded, retryable, fallbackcontext_overflow/ bareoverloadedoverloaded, retryable, fallbackserver_error, retryable,should_fallback=Falseoverloaded, retryable, fallbackModelLoadingError)format_error,retryable=Falseoverloaded, retryable, fallbackcode(proxy-stripped message)format_error/unknownoverloaded, retryable, fallbackThe 507 and 409 routes and the
prefill_memory_abortedcode were found by@tkaufmann against this branch's head. Evidence grades are recorded per
commit and per code comment and are not flattened: the 507 gap and both
process-abort wordings are captured bodies from his own hosts; the 409 mapping
and the
prefill_memory_abortedcode pairing are read from the engine's sourceand are labelled as code readings, not as evidence.
Related Issue
Fixes #52261
Type of Change
Changes Made
agent/error_classifier.py:_MEMORY_CEILING_PATTERNS+_MEMORY_CEILING_ERROR_CODES, consumedthrough the single
_is_memory_ceilingpredicate and the single_memory_ceiling_resultrecovery contract._classify_400, the500/502 and 503/529 branches,
_classify_by_error_code, and the no-status_classify_by_messagestreaming path.507branch (local-inference model-load guard: oMLX maps bothModelTooLargeErrorandInsufficientMemoryErrorhere, reachable from/v1/chat/completions,/v1/completionsand/v1/messages). Previouslyfell to generic "other 5xx": retryable, but
should_fallback=False.409branch (ModelLoadingError, "load aborted: process memory limitexceeded"). Previously fell to generic "other 4xx":
format_error,retryable=False.prefill_memory_abortedadded to_MEMORY_CEILING_ERROR_CODES— theadmitted-then-killed sibling of
prefill_memory_exceeded, selected byexception type in the engine's body builder.
literal Insufficient Storage sense and an ordinary 409 conflict keep their
existing treatment.
tests/agent/test_error_classifier.py: captured provider wordings pinned asmodule constants across the 400 / no-status / 5xx / 507 / 409 / structured-code
paths, each with a control proving the guard is narrow, plus invariant tests
that a genuine context-window overflow still compresses and a genuine billing
exhaustion is still billing.
How to Test
uv run --with pytest --with pytest-asyncio python3 -m pytest tests/agent/test_error_classifier.py -vrestoring the pre-change blob of
agent/error_classifier.pyand re-running:test_507_omlx_model_load_ceiling_is_overloaded_with_fallback→ red asserver_error/should_fallback=Falsetest_prefill_memory_aborted_code_is_overloaded[400]/[None]→ red asformat_error/unknowntest_409_memory_abort_is_overloaded_not_format_error→ red asformat_error/retryable=FalseChecklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — for this update:tests/agent/(the suite covering the only production file touched). 155 failures, and the failing set is byte-identical to cleanorigin/main(c896c09), which fails the same 155. Zero of them are intest_error_classifier.py, which is 115/115 green.Documentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AContract Protected
Invariant: a provider memory/resource-ceiling rejection never enters the
compress-and-shrink-context path and is never reported as a client error,
regardless of which route it arrives on — HTTP 400, 409, 500/502, 503/529, 507,
a structured error code, or a no-status streaming
APIError.prefill memory guard rejected … dynamic ceiling is 13.50 GB … reduce context
length"; "process memory limit exceeded … loosen memory_guard_tier" and its
reworded 11 Aug sibling "… dynamic ceiling 60.4 GB. Close other apps to free
RAM … raise memory_guard_tier"; the no-status "Prefill context too large for
available memory"; and the 507 "Model 'X' (33.95GB) does not fit under the
dynamic memory ceiling (25.22GB)".
captures): the 409 "Model 'X' load aborted: process memory limit exceeded",
and the
prefill_memory_abortedstructured code._MEMORY_CEILING_PATTERNSkeys onmemory/allocation/ceiling/guard wording (OOM, llama.cpp/vLLM, Metal/CUDA),
disjoint from token/window-count language;
_MEMORY_CEILING_ERROR_CODEScovers the reworded/proxy-stripped case where no memory substring survives.
Two captured wordings of the same shape, nine days apart, are pinned against
each other so a third engine copy-edit fails loudly instead of silently.
maximum context length … reduce the length400still routes to
context_overflow+ compression; a genuine billingexhaustion is still
billing; a plain 500/503 is untouched; a 507 with nomemory wording stays a generic
server_error; an ordinary 409 conflict staysa
format_error.