Salvage the plugin-owned static prompt idea from PR #51589 into the constrained #64167 contract: stable IDs, deterministic placement, bounded fail-open rendering, and full-prompt resume recovery without new session columns.
Co-authored-by: Topher Ross <biz@topherross.com>
Wire an optional `task=<key>` kwarg through the PluginLlm facade so a
plugin can route an LLM call through an auxiliary model slot it
registered via `ctx.register_auxiliary_task`. Registration already
existed; this adds the missing consumption half. Closes#44673.
Sub-issue 08/14 of the plugin-interface expansion tracking issue #64182.
- New optional `task:` kwarg on complete/acomplete/complete_structured/
acomplete_structured. Unset or "auto" keeps today's main-model path
byte-for-byte (task=None reaches call_llm exactly as before), so no
prompt-cache or default-behavior change.
- A set task resolves provider/model through `auxiliary.<task>` via the
existing auxiliary_client path, identical to built-in aux tasks.
- Trust gate (per the round-2 design correction): a plugin may only pass
a key it registered itself; a built-in key additionally requires
`plugins.entries.<id>.llm.allow_task_override: true`. A foreign or
unknown key is rejected with a PluginLlmTrustError and a logged warning
naming the offending plugin and key -- fail loud, NOT a silent fallback
to auto (which would mask misconfiguration and could route to the main
model the user steered elsewhere).
- The plugin_llm audit dict and audit-log line gain a `task` field.
- register_auxiliary_task now stores the plugin's canonical id
(`key or name`, the same id ctx.llm is bound to) as the slot owner, so
the trust gate matches ownership even when a manifest sets a distinct
key. For the common no-key case this equals the name (unchanged).
Tests (tests/agent/test_plugin_llm_task_routing.py, 24): _check_task
resolution incl. own/foreign/unknown/built-in-gated keys and loud
rejection; end-to-end routing sync+async+structured; production-path
forwarding into call_llm/async_call_llm (covers the task=None->task line);
and ownership resolution against the real plugin registry incl. the
name/key reconciliation.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013b1XyXitAxV7phGmKWigJX
Address teknium1 review on #64189:
- Re-pull gate now delegates to each source's is_enabled(cfg) via the
registry contract, so a plugin source with custom activation logic is
honored (previously only secrets.<name>.enabled was checked).
- Add BUILTIN_SOURCE_NAMES to the registry so plugin-vs-bundled is a
single source of truth instead of a hard-coded set at the call site.
- Reconcile docs: rewrite the timing :::note to describe both the
post-discovery re-pull and the remaining import-time limitation, and
cross-link the first-process bootstrap section.
- Tests: real SecretSource subclasses, custom is_enabled activation
(positive + negative), is_enabled-raises skip, builtin-only no-op,
and a discovery-registration end-to-end re-pull check.
Follow-up to #84632:
- Guard _llama_cpp_grammar_hit inside status_code == 400 to restore
short-circuit behavior on non-400 errors (minor efficiency)
- Extract _NO_USER_QUERY_SIGNAL constant for the duplicated string
between _INVALID_MESSAGE_BODY_PATTERNS and the llama.cpp exclusion
guard, preventing silent drift if the phrase is ever changed
Local engines wrap Qwen template raise_exception("No user query found…") as
applyPromptTemplate / "Unable to generate parser for this template". That used
to match llama_cpp_grammar_pattern, strip tool schema keywords, and retry while
the real cause was a poisoned/oversized transcript after failed compression.
Classify as format_error so recovery fails fast toward /new instead.
* fix(models_dev): map meta-ai provider to models.dev 'meta' id
Muse Spark models (muse-spark-1.1/1.2/-contributor) are served via the Meta
Model API and reverse-map from api.meta.ai to the Hermes provider id 'meta-ai'.
models.dev keys the same models under the provider id 'meta'.
lookup_models_dev_context() / _get_provider_models() resolve the models.dev id
strictly via PROVIDER_TO_MODELS_DEV.get(provider) (no raw-id fallback, unlike
get_model_info()), so an unmapped 'meta-ai' missed entirely and context fell
back to the generic 256K default instead of the true ~1M window. Add the
meta-ai->meta mapping (plus a defensive meta->meta) so context and pricing
resolve from models.dev: 1.1=1,000,000; 1.2 & -contributor=1,048,576.
* add contributor email
---------
Co-authored-by: Beto de Paola <betodepaola@meta.com>
The write_file / patch file tools hard-denied ~/.ssh/config as a
"protected system/credential file", while the terminal tool only
*asked* for approval on ~/.ssh writes. That inconsistency meant a write
to ~/.ssh/config was refused via write_file but succeeded via terminal
after an approval prompt -- the same operation flip-flopping between
denied and OK depending on which tool ran it.
The SSH client config carries no private-key material, and editing it
(host aliases, ProxyJump, VS Code Remote-SSH targets) is a routine,
user-initiated task. It CAN carry ProxyCommand / Match exec directives
that run commands, so a free write is still inappropriate -- approval,
not a flat refusal, is the right policy, matching what the terminal tool
already does.
Changes:
- agent/file_safety.py: remove ~/.ssh/config from the flat credential
deny; add build_write_approval_paths() + is_write_approval_required(),
and short-circuit it out of the ~/.ssh/ prefix deny so the file is
allowed at the classifier layer. Private keys, authorized_keys, and
everything else under ~/.ssh/ stay hard-denied.
- tools/file_tools.py: _check_approval_required_write() routes ssh config
writes through the shared _run_approval_gate (once/session/always,
honors --yolo, fail-closed with no human), wired into write_file_tool
and patch_tool right after the protected-instruction gate.
- Non-interactive consumers fail closed: the ACP file bridge
(copilot_acp_client) rejects approval-required paths outright, and the
TTS output-path picker refuses them as before.
- Docs + tests updated (security.md exception note;
TestSshConfigApprovalGate covers config approval-gated, keys still
hard-denied).
Two follow-ups from live Windows sessions:
1. agent/prompt_builder.py: extend the Windows shell hint with the
native-binary path rule. Hermes disables MSYS path conversion for its
bash, so agents passing /c/Users/... or /tmp/... to NATIVE programs
(git -C, node, python, rg) hit 'cannot change to' / 'not found' while
the same path works in bash builtins — observed repeatedly in a live
session (git -C failures, git apply /tmp/x.patch failures). The hint
now says: forward-slash native form (C:/Users/x) for native tools,
$LOCALAPPDATA/Temp over /tmp for scratch files native tools read.
(/tmp is pure model habit from Linux training data — nothing
instructs it — so the hint is the right layer.)
2. tests: pin LF/CRLF preservation through write_file and patch_replace.
A live session saw a repo-LF file come back full-CRLF after an edit
(4699-line diff churn); not reproducible through current tool APIs,
so pin the correct behavior — LF files stay LF, CRLF files stay CRLF,
no mixed endings — to catch any regression on the Windows write path.
Sweep of open Windows issues affecting day-to-day agent operation
(explicitly excluding install/setup and locale classes):
- hermes_cli/_subprocess_compat.py: new split_command_line() — Windows-
safe command-line tokenizer (posix=False + quote stripping) so
backslash paths survive. POSIX behavior unchanged (plain shlex.split).
- hermes_cli/console_engine.py (#83934): console commands like
'sessions export C:\Users\me\out.jsonl' no longer silently mangle the
path into a relative filename in the cwd.
- agent/shell_hooks.py (#78293): hook commands with backslash paths now
spawn, resolve their script path, and pass hooks doctor instead of
reporting 'not executable'. All three shlex sites routed through the
shared splitter.
- agent/prompt_builder.py (#51755): system prompt now reports
Windows (11) on Windows 11 — platform.release() returns 10 for both;
distinguish via sys.getwindowsversion().build >= 22000.
- hermes_cli/commands.py (#42016): @ autocomplete no longer crashes the
prompt_toolkit event loop when rg emits a path on a different mount
(device paths \.\nul, other drive letters) — relpath ValueError is
skipped per-entry.
- tools/browser_use_cli.py (#83884): screenshot-path detection now
matches Windows drive-letter paths (C:\... and C:/...) in addition to
POSIX; Browser Use screenshots attach on Windows.
- tools/skills_hub.py + tools/skills_guard.py (#62310): the two 'MUST
stay symmetric' skill content hashes actually agree on Windows now.
Bundle keys are normalized to POSIX separators before hashing, and the
disk digest sorts by rel-posix STRING (case-sensitive) instead of Path
objects (case-insensitive on Windows). Fixes permanent false-positive
update_available for every installed skill.
Tests: tests/tools/test_windows_agent_loop_papercuts.py — 16 cases
covering each fix, including a disk-vs-bundle hash symmetry check built
with native Windows separators and a mixed-case filename.
Follow-up fixes on top of the salvaged #83678 commit:
1. Hoist the MiniMax-M3 marker exclusion ABOVE the native-Anthropic
early return. provider="anthropic" pointed at a MiniMax /anthropic
proxy is a supported override (_anthropic_base_url_override_ok), and
the is_native_anthropic branch matched on provider alone — returning
(True, True) before the M3 exclusion was reached. Two regression
tests pin the proxy route (M3 off, M2.7 still on).
2. Reuse the existing _model_name_suggests_minimax_m3() helper from
agent/model_metadata.py instead of a second inline substring copy.
3. Drop the debug kwarg on normalize_usage() — it had zero production
callers and duplicated standard logging level gating. The
cache-observability line is now a plain logger.debug scoped to
MiniMax providers on the Anthropic wire only, so the "+128 floor"
note can no longer appear for native Anthropic where it is false.
Tests updated accordingly (MiniMax logs, native Anthropic does not).
MiniMax-M3 ships server-side automatic prefix caching on the
Anthropic-compatible endpoint (content-keyed, no marker needed —
see platform.minimax.io/docs/api-reference/text-prompt-caching).
cache_control markers are NOT on its explicit-cache support list
(which covers only M2.7/M2.5/M2.1/M2).
Emitting markers on M3:
- wasted serialization overhead
- risked perturbing the server-side prefix hash
- gave users a false sense of explicit-cache savings (the
cache_read_input_tokens field carries a +128 constant floor
and cache_creation_input_tokens is always 0 for M3)
Also add an opt-in debug=True parameter to normalize_usage() that
emits a debug-level log line carrying the observable cache fields.
This is the only reliable cache signal for M3 — off by default,
debug-level, scoped to the anthropic_messages wire, so production
callers see no impact.
Pin both changes with 8 new tests:
- 4 M3 tests covering provider, host, and custom-provider paths
- 1 regression guard ensuring M2.x caching is unaffected
- 3 observability tests (off-by-default, on-with-M3, on-with-Claude)
Verified end-to-end against api.minimaxi.com/anthropic/v1/messages
with MiniMax-M3[1m]: identical system prompt hit-rate with and
without markers; cache_read field is unreliable (128 floor),
input_tokens drop (8467 -> 1) is the real hit signal.
* fix: make verify_on_stop opt-in everywhere (default False, not auto)
The verify-on-stop nudge was already judged more noise than signal: the
v31 migration flips existing installs off, the v32 migration catches the
baked-in literal-true population, and the docs tell users to 'treat off
as the effective default and opt in explicitly'. But DEFAULT_CONFIG still
shipped the "auto" sentinel, so exactly one population kept getting the
nudges: fresh installs (and any config missing the key), where "auto"
resolves ON for CLI/TUI/desktop surfaces. Live symptom: repeated
'[System: You edited code ... run verification]' interruptions the user
never asked for and had to hunt down in source to disable.
- DEFAULT_CONFIG: agent.verify_on_stop "auto" -> False (opt-in).
- verify_on_stop_enabled(): missing/unrecognized value now falls back
OFF instead of surface-aware; explicit "auto" still selects the
legacy surface-aware behavior, explicit bools unchanged, and the
HERMES_VERIFY_ON_STOP env override is untouched.
- No migration needed: v31/v32 already normalized existing installs,
and this only changes the merged default for configs without the key.
- Docs updated; default-path E2E test now asserts OFF, plus a new
missing-value regression test. Also added the standard win32 skip
marker to the symlink-based temp-dir test (pre-existing Windows
failure, same class as tests/cron/test_cron_script.py).
* test: update config goldens — verify_on_stop=False is now stripped as default
With the DEFAULT_CONFIG flip to False, the migration-write invariant
(_persist_migration / save_config strip_defaults) no longer materialises
verify_on_stop: false to disk unless the user explicitly set the key:
- V20 floor fixture (agent: {} on disk): v31's write is stripped —
agent stays {} and load_config() supplies False at read time.
- V12 floor fixture (explicit verify_on_stop: true on disk): the key is
a user-set path, so the v32 flip stays materialised as false.
- Partial-write and _persist_migration regressions now assert the key is
absent from disk and (for the merge case) that the merged view still
resolves False.
Behavior verified with a one-shot migrate_config run against both
fixture shapes.
* fix: warn agents off driving interactive console TUIs via pty on Windows
Driving 'gh auth login' (and other survey-style console TUIs) through a
pty background process on Windows silently hangs: these programs read
Win32 console key events via ReadConsoleInput, not the stdin byte
stream, so Enter keypresses submitted over process stdin never register.
The agent-visible symptom is a prompt frozen at 'Press Enter to open
browser...' while the user sees nothing, and a turn interrupt then kills
the process, invalidating any device code the user already entered on
github.com.
Two guidance fixes, both proven in a live session on Windows 10:
- agent/prompt_builder.py: extend _WINDOWS_BASH_SHELL_HINT to steer
agents toward non-interactive paths (flags, --with-token, config
files, curl-polled OAuth device flow) instead of answering console
prompts programmatically.
- skills/github/github-auth: document the pitfall and add the manual
OAuth device-flow procedure (curl against gh's public client_id,
poll for the token, finish with 'gh auth login --with-token'), which
succeeded first try after two interactive attempts hung.
* fix: send CRLF for Enter on Windows PTY submit; correct root cause in guidance
Review feedback (helix4u) was right on both counts:
1. Root cause correction. gh's 'Press Enter to open browser' prompt is
waitForEnter -> bufio.Scanner reading stdin, not a survey/console-API
prompt. The real bug is ours: submit_stdin appended a bare \n, and
through pywinpty/ConPTY a lone \n is not delivered as a line
terminator, so the child's blocking line read never returns. Verified
empirically against pywinpty 2.0.15 with a readline() child:
\n -> hang, \r -> line delivered, \r\n -> line delivered.
Fix: submit_stdin now appends \r\n for Windows PTY sessions (POSIX
PTYs and Popen pipes keep \n). Windows-only regression tests cover
the PTY and pipe branches.
2. Prompt hint rewritten: instead of claiming Windows console TUIs
cannot be driven, it now says to use process(submit) rather than raw
writes with bare \n, and to prefer non-interactive paths when a CLI
offers one.
3. Skill device flow rewritten as an executable script: parses the
device-code response, polls per the returned interval, handles
authorization_pending / slow_down (+5s per GitHub docs) /
expired_token / access_denied / unexpected responses, pipes the token
straight into gh without echoing it, and drops the undocumented
workflow scope (repo,read:org,gist is the documented minimum for
gh auth login --with-token). The pitfall note is narrowed to the
reproduced condition.
The PR added APIConnectionError handling to the main request and
iteration try blocks but missed the finalization drain loop (line ~1492).
That site catches httpx transport errors to preserve an already-completed,
already-billed response when the drain iterator fails. Without the
APIConnectionError handler, an SDK-wrapped transport error during drain
would propagate uncaught and discard the completed response.
Also strengthens the test's no-payload-leak assertion to check the full
request body and URL are absent from the log message, not just the
literal string 'payload'.
The Kimi team noticed that traffic from Hermes Coding Plan users
identifies itself as Claude (User-Agent: claude-code/0.1.0) rather
than the actual client. They asked us to update the UA so they can
properly attribute traffic and understand how their services are
accessed — especially important as they open up to more third-party
agents.
Three code paths were sending wrong/attribution-less headers to Kimi:
1. run_agent.py — _apply_client_headers_for_base_url sent
{"User-Agent": "claude-code/0.1.0"} for api.kimi.com. Now sends
the same _AI_GATEWAY_HEADERS set used for Vercel AI Gateway:
HTTP-Referer + X-Title + HermesAgent/{version} User-Agent.
2. agent/anthropic_adapter.py — the Anthropic Messages path for
api.kimi.com/coding sent 'claude-code/0.1.0'. Now sends the same
three-header attribution set.
3. plugins/model-providers/kimi-coding/__init__.py — both kimi and
kimi_cn profiles sent a static 'hermes-agent/1.0' with no
HTTP-Referer or X-Title. Now sends the full three-header set with
a dynamic version, matching the pattern used by the gmi, fireworks,
xai, and ai-gateway provider profiles.
The attribution header set (HTTP-Referer + X-Title + User-Agent) is
the canonical Hermes pattern used for OpenRouter, Vercel AI Gateway,
Fireworks, and other providers that read these headers for traffic
attribution.
Adds the comment-based hotspot convention (no new primitives) across three
guidance surfaces:
- KANBAN_GUIDANCE worker lifecycle: new step 7 — when a file keeps colliding
with siblings or appears in other cards' recent comments, leave a
'hotspot: <path> — <reason>' kanban_comment and repeat it in completion
metadata so the orchestrator can decompose the file first.
- kanban.md (en + zh-Hans): 'Collision hotspots in parallel campaigns'
subsection — the convention, the orchestrator response (2+ flags on one
path => dedicated decomposition card before queuing more work touching
it), and the cross-link to merge-reconciler for conflicts that already
happened.
- merge-reconciler SKILL.md Pitfalls: repeated conflicts on the same file
across rounds are a hotspot signal — flag for decomposition rather than
serially reconciling.
Live-verified: guidance renders once via real import (6152 chars); hotspot
comment round-trips through add_comment -> list_comments -> worker context
on an isolated HERMES_KANBAN_DB; kanban tools, review-surfaces, and
merge-reconciler skill tests green (45 passed).
Design decisions belong to the orchestrator: decide naming schemes,
schemas, file formats, and API shapes before fanning out; never let two
subtree cards decide the same question; stamp every decision into each
dependent card body since workers cannot see sibling context. Mirrored
in the kanban docs (en + zh-Hans) with an exporter/importer worked
example, and bounded KANBAN_GUIDANCE size with an invariant test.
Add a non-terminal "review" status so a worker that finished implementation
can hand off for human review without abusing kanban_block. The old
kanban_block(reason="review-required: ...") convention routed the handoff
through the unblock-loop breaker, so a normal review -> changes -> review
cycle was falsely escalated to triage.
- kanban_db: request_review (running/ready -> review, non-block, emits
review_requested), reopen_review_task (review -> ready/todo, review_reopened),
complete_task accepts review -> done, and a review_dispatch gate (default off,
shared by the dispatcher loop and the gateway health probe).
- kanban_request_review worker tool + `request-review` / `reopen-review` CLI
verbs; tool wired through toolsets, EXPOSED_TOOLS, _POLISHED_TOOLS.
- Gateway notifier wakes the origin subscriber on review_requested and
block_loop_detected; the subscription survives until done/archived, so every
review cycle re-notifies.
- Dashboard PATCH + bulk route the review transitions (request_review /
reopen_review_task) and render the review column.
- goals.py goal-loop and KANBAN_GUIDANCE recognize review as a terminator.
- Docs (reference tables, user guide, AGENTS.md, zh-Hans mirrors) + tests.
needs_input / failed are unchanged: they still route through kanban_block,
still count toward block_recurrences, and still escalate to triage.
Cold CLI time-to-banner was ~1.8s (hermes) / ~2.8s (hermes -w). The banner
path was paying for work the session doesn't need before first input:
- aux availability probes built REAL OpenAI/httpx clients (openai import
~0.3s + SSL context) just to answer check_fns. New aux_probe_mode()
returns a cache-excluded stub; resolution policy unchanged.
- tools/mcp_tool imported the mcp SDK (~260ms, mcp.types pydantic model
construction) at module import even with zero MCP servers configured.
SDK import is now lazy behind _ensure_mcp_sdk(); _MCP_AVAILABLE is a
find_spec probe so every existing gate/test keeps its semantics.
- banner blocked 500ms on the update-check prefetch; now waits 50ms and
defers the warning line to a daemon thread (prints above the prompt).
- banner recomputed get_tool_definitions + skills scan + git state every
launch; now snapshotted to ~/.hermes/cache/banner_snapshot.json keyed on
(config.yaml, .env, checkout rev, toolsets) and replayed on warm launches
with a background refresh. Agent tool list is still computed fresh.
- _resolve_active_context_length probed the Nous portal /models (~200ms
network) per launch; the tool-search gate now prefers the on-disk
context cache when present.
- schema reconciliation re-executed SCHEMA_SQL in a scratch SQLite DB
(~85ms) per SessionDB(); the reference parse is now disk-memoized by
DDL hash (live-DB diffing still runs every startup).
- bundled-skills sync (~120-170ms rglob/hash) moved off the startup path
to a daemon thread; plugin discovery starts in the background and every
synchronous consumer joins via discover_plugins().
- hermes_cli.auth imported httpx eagerly (~30ms); now a lazy proxy that
test monkeypatching still reaches (setattr forwards to the real module).
- fast chat launch: unambiguous 'hermes'/'hermes chat' invocations skip
building all ~40 subcommand parsers (bails to full dispatch on anything
else, incl. container mode).
- -w path: git worktree add runs with checkout.workers=8 (0.6s→0.2s) and
overlaps HermesCLI construction; --skills preload runs in the background
and is folded in at agent init (finalize_preloaded_skills, same
fail-loud contract for fully-unknown skill lists); stale-worktree prune
moved off the banner path.
Warm results (PTY time-to-banner, 5-run): hermes 1.80s → 0.38-0.40s;
hermes -w -s hermes-agent-dev --yolo 2.82s → 0.57-0.69s.
After a partial update (stash restore overwriting providers/base.py with
an older version), the NousProfile singleton was instantiated from a
ProviderProfile class that predates the supports_prompt_cache_key field
(added in f4fb23f3d). Accessing profile.supports_prompt_cache_key raised
AttributeError, crashing every API call with:
'NousProfile' object has no attribute 'supports_prompt_cache_key'
Use getattr(profile, 'supports_prompt_cache_key', False) so a stale
profile degrades to 'no prompt cache key' instead of crashing.
Trim verbose comments in conversation_loop.py and run_agent.py to 2 lines
each. Fix the same bug class in the compression summary path at
chat_completion_helpers.py: remove _thinking_prefill from the explicit
pop tuple and move the generic underscore-key sweep to after
_drop_thinking_only_and_merge_users, so the drop pass can recognize
prefill stubs there too.
- warn (not debug) on final text-turn flush failure: a failure here
reopens the exact #81641 data-loss window with _persist_session as
the only remaining retry, unlike the verify siblings which retry
in-loop; include session id for triage
- trim the flush-site comment to sibling proportion, pointing to the
test module for the full incident narrative
- test: assert _persist_session presence before indexing, so a wiring
change fails with a clean assertion instead of ValueError from max()
A pure-text assistant turn (finish_reason=stop) had no durable write of
its own. Its answer reached the user through the streaming / interim
display path, which is display-only and never touches state.db, and the
first durable write was finalize_turn's _persist_session — after the
loop exits and behind post-turn work that can include micro-compaction's
aux-LLM call.
Anything that ended the process or tore the session down inside that
window lost a reply the user had already been shown. On a remote
(non-loopback) backend the window is easy to hit: WS 1006 closures drive
ws_orphan_reap teardown, and affected sessions ended up with user rows
and zero assistant rows in state.db.
The neighbouring exits of the same loop already close this gap:
* the tool-call exit flushes the assistant(tool_calls) block before
handing control to _execute_tool_calls (#49045)
* the verify-on-stop and pre_verify exits flush final_msg before
appending their nudge (#65919 §7)
Apply that same idiom to the ordinary text exit rather than adding a new
persistence mechanism. The intrinsic _DB_PERSISTED_MARKER dedup makes the
later _persist_session a no-op for this row, so no duplicate rows and no
extra write — the same write, just earlier.
Unlike the tool-call exit, a failed flush must not abort the turn: no
side effect runs after this point and the answer is already produced, so
the failure is logged and _persist_session remains the retry.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Move the legacy ghost-row filter from inside the api_messages loop to
BEFORE repair_message_sequence_with_cursor. Dropping a ghost assistant
row between two user messages creates user→user which the repair can
now fix (previously the repair ran first and missed it).
Promote '[This response was interrupted by a user correction.]' to
module-level _INTERRUPT_SCAFFOLD_MARKER constant — used in both
_apply_active_turn_redirect (checkpoint_parts) and the ghost filter,
so they can never drift.
Update ghost-row test: the two consecutive user messages are now
merged by repair, so check for content as substring.
Sessions already poisoned by the incomplete #73146 else branch still replay
hidden assistant rows whose content is the raw interrupt scaffold. Skip those
rows when building provider messages so old state.db history cannot keep
seeding the echo loop.
The incomplete #73146 else branch still wrote the interrupt checkpoint into
the placeholder assistant row. Mid-tool steers then replayed that scaffold as
the model's own prior reply, which it echoed into a self-replicating ghost
loop. Carry the scaffold only on the user correction's api_content, matching
the assistant-tail branch.
Fix: _LENGTH_CONTINUATION_DROPPED_TOOLS_PREFIX ended with '(' but
_get_continuation_prompt still had f'({tool_list})', producing
'((write_file)' instead of '(write_file)'. Removed the '(' from
the prefix constant — the parenthesis belongs in the interpolation.
Widened: promoted the empty-response nudge (line 6993,
'You just executed tool calls but returned an empty response...')
to _EMPTY_TOOL_RESPONSE_NUDGE constant and added it to the
classifier's recognition set. Same bug class — its
_empty_recovery_synthetic metadata flag doesn't survive SessionDB
projection either.
Test: added parametrize case for the empty-response nudge (7→8 cases).
E2E: verified byte-for-byte string equivalence for all nudge constants.
aed114a69 taught _is_synthetic_compression_user_turn to recognize the
max-iteration nudge as ephemeral runtime scaffolding rather than a human
turn, since its role="user" metadata flag doesn't survive SessionDB
projection and a crash/interrupt mid-turn can persist it durably — becoming
the compaction anchor / auto-focus topic in place of the real task.
conversation_loop.py's retry loop appends several more role="user" rows
with the exact same "ephemeral, metadata-tag-only" shape, none of them
recognized by the classifier:
- The three _get_continuation_prompt variants (length-continuation nudge,
tagged _length_continuation_nudge) — two fixed strings plus a third that
interpolates the dropped-tool-call list.
- _CODEX_INCOMPLETE_NUDGE (codex/responses reasoning-only retry).
- The codex ack-continuation nudge (acknowledgment-only reply re-prompt).
- The dropped-tool-call nudge (tagged _dropped_toolcall_nudge) — persisted
across up to 3 consecutive retries before the finalization pop-loop
strips it; an interrupt/crash before that pop can persist it same as the
max-iteration case.
Promote the previously-inline nudge strings to named module-level constants
in conversation_loop.py (single source of truth for both construction and
recognition), then extend the classifier to recognize all of them — exact
match for the five fixed-content nudges, a stable-prefix check for the
dropped-tool-call continuation variant (its tool list is interpolated so it
can't be exact-matched, same treatment TODO_INJECTION_HEADER already gets).
Imported lazily inside the classifier to avoid a module-load-order cycle —
conversation_loop.py already imports FROM context_compressor.py at call
time for the same reason.
Simplify registration/unregistration to match delegate_tool.py's
hasattr+getattr pattern instead of over-defensive try/except Exception
blocks. Delete inspect.getsource() change-detector test (breaks on
rename, proves nothing the behavioral test doesn't cover).
Net: -73 lines, +35 lines = -38 lines.
A background memory/skill review (agent/background_review.py) forks a
second, complete AIAgent in a daemon thread that deliberately shares the
live agent's own session_id for prompt-cache warmth. Nothing previously
stopped a user's next live turn from starting while that fork was still
mid-conversation, letting both stream against the same session_id and
credentials concurrently. That produced two observable failures:
- Doubled prompt-token accounting on the live turn's own calls (the two
concurrent request/response streams under one session_id confuse the
token-usage bookkeeping), triggering premature context compression.
- A lockup that a normal interrupt could not clear: the review fork is a
fully independent AIAgent with its own _interrupt_requested flag, and
was never added to the parent's _active_children list -- the only list
AIAgent.interrupt() actually walks for cross-agent cancellation -- so a
live-turn Ctrl+C had no propagation path to it at all.
Fix, three files:
1. agent/agent_init.py -- add _background_review_agent /
_background_review_lock tracking state to every AIAgent, mirroring the
existing _active_children pattern.
2. agent/background_review.py -- the review fork now registers itself on
the parent's _active_children right after construction (reusing the
same list/lock interrupt() already fans out to for real subagent
delegation), and unregisters on every exit path (success, the
tool-whitelist finally, and the outer exception safety-net). All
registration is defensive (getattr/try-except) so an AIAgent built
without going through agent_init.py's setup degrades to "no
cross-turn cancellation" instead of aborting the whole review.
3. agent/conversation_loop.py -- at the very start of every
run_conversation() turn, if a prior background review is still
in-flight, it is now proactively cancelled via interrupt() before the
live turn proceeds -- fire-and-forget, non-blocking, adds no latency.
Adds 3 regression tests to tests/run_agent/test_background_review.py,
confirmed to fail against the pre-fix code via a scripted revert.
Verified: ruff clean on all touched files; 66/66 background-review and
interrupt-propagation tests pass; 256/256 across turn_finalizer +
run_agent regression suites; no fork-only symbols in the diff.
Follow-ups from review of #82049:
- extract append_user_instruction() into agent/skill_commands so the
stable-prefix construction cannot drift between the skill and cron
builders (the registered prefix must stay a byte-prefix of the built
message); cron no longer imports the private _SINGLE_SKILL_INSTRUCTION
- add the startswith guard to the skill builder registration site,
matching the stronger cron guard
- rename _MAX_BYTES to _MAX_CHARS (sum(map(len, ...)) counts characters,
not bytes) and correct the comment
- collapse find_stable_prefix's two-lock dance into a single critical
section (scan is <=32 short-circuiting startswith calls, measured
2-4us; drops the snapshot copy and the TOCTOU re-check)
- document the split-shape lifetime (marked-endpoint window) in the
module docstring
- add a contract test for the helper's byte-prefix invariant
(mutation-checked)
Follow-up review of the builder-declared cache boundary (#81867) found three
ways the split could silently stop paying off, or keep paying more than it
should, on a long-lived gateway process.
Flattening no longer consults the registry. `strip_anthropic_cache_control`
matched the decorated split by looking the first block up in the prefix
registry, so a mid-turn failover that re-decorates a request built many
messages earlier (#72626) would fail to flatten once _MAX_ENTRIES newer
scaffolds had been registered in between, and would hand the next provider
the two-part shape instead of the canonical string. The split is now matched
by its shape: a marker on the *first* part of a user message is something no
other decoration produces (list content otherwise gets its marker on the last
part, and the two-part [static, volatile] split is role-gated to system), so
the ""-join stays provably byte-exact without any process state. This drops
`is_registered_stable_prefix` and one lock acquisition per stripped message.
Lookups now refresh LRU position. A scaffold fired every minute by cron could
be evicted by a burst of one-off skill invocations while still being the
hottest prefix in the process, silently reverting it to whole-message caching.
Registration now also evicts by total retained bytes (4 MiB). Entries hold
whole expanded skill bodies, so a 32-entry cap alone does not bound memory.
The newest entry is always kept, so a single oversized scaffold still gets a
boundary instead of disabling the split.
Tests: eviction-then-failover round-trip, LRU refresh on hit, byte-cap
eviction, and oversized-single-entry survival.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Webhook/cron skill invocations concatenate a large static scaffold
(activation note + expanded skill body) with a small volatile tail
(ticket payload, timestamps) into one user string, and the Anthropic
cache planner marked that whole string as a single atomic block — so a
few changed tail bytes forced a full cache rewrite on every invocation.
Instead of re-parsing scaffold marker strings out of the message at
request time (fragile when a payload or skill body quotes the marker),
the builders now register the exact stable-prefix bytes in a small
process-local LRU registry at construction time. The cache planner
splits a registered user string into [marked stable prefix, unmarked
volatile tail] request-locally; canonical session history stays a plain
string, and the failover stripper flattens the split back byte-exactly
via an O(1) registry lookup. Unregistered messages keep the existing
whole-message policy.
Covers the single-skill builder (webhook + slash command + TUI) and the
cron job prompt assembler (multi-skill, bundles, skipped-skill notice),
with registration guarded against injection-scanner sanitization.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Folds the model-switch fix in with the untitled retry. They answer
different halves and each is wrong alone: counting alone left a session
that merely opened with machinery nameless forever, because nothing
reconsidered it, and the stored title alone would never title at all on a
store too old to report one. Skip only when both agree — past the opening
turn, and already named.
Counting a turn now judges a multimodal one on its text, so "here's a
screenshot, fix the login" counts as the question it is rather than
reading as machinery and undercounting the conversation.
Co-authored-by: yy28 <yy28@vip.sina.com>
Switching models before sending the first real message titled the session
"[System: The active model for this chat has…" instead of the user's actual
question.
`_append_model_switch_marker` persists its notice with `role="user"` because
strict OpenAI-compatible providers reject a system message that is not first
(#48338). Titling had no way to tell that apart from a genuine opening turn,
which caused two distinct failures:
1. `_MACHINE_PREFIXES` did not cover the marker. Its `[System: ` prefix
matches none of `[CONTEXT COMPACTION`, `[Runtime note:`, or `[SYSTEM]`
(different case, no closing bracket), so `is_titleable_user_message()`
returned True and the marker was formatted into the title.
2. `maybe_auto_title()` counted the marker as a user message. With the marker
present, the first real question arrived at `user_msg_count == 2` and the
`> 1` guard returned early, so the session was never titled at all and its
`title` column stayed NULL. Fixing only (1) would therefore have traded a
wrong title for a permanently missing one.
Add the marker prefix to `_MACHINE_PREFIXES` (kept in sync with
`tui_gateway.server._MODEL_SWITCH_MARKER_PREFIX`) and count only titleable
user messages when detecting the opening turn.
The guard stays narrow: ordinary user text that happens to start with
"[System:" still titles normally.
Adds 6 regression tests, verified to fail without the fix.
Two lookalike gaps found auditing the titler.
_MACHINE_PREFIXES missed the compressor's legacy summary opener and the
"[System note:" injections, so a compacted or resumed session could be
named after the note that carried it. Take the summary prefix from the
compressor that emits it rather than keeping a fourth local copy.
The fast-model exclude list covered embedders but not the other non-chat
siblings a provider names after its chat model — "gpt-4o-mini-tts"
satisfies the "-mini" rung and cannot answer a prompt.
The fast-model picker reads /v1/models to find the small model a provider
currently serves, and it asked anonymously. Most of those endpoints need a
key, so the fetch 401'd and the empty result read as "this provider has no
small model" — the picker fell back to its curated list and never noticed.
Worse, a failed fetch cached its empty result forever, so one bad moment
during startup disabled live model discovery for the life of the process,
and the processes that read this run for weeks. Give the failure an expiry
and pass the provider's credentials.
The bare family rungs (-mini, -flash, haiku) also picked whichever id
sorted first, which is the oldest generation a provider still serves:
gpt-3.5-mini over gpt-5.4-mini, claude-3-haiku over claude-haiku-4.5.
Compare the digit runs as numbers so the rung meant to keep us current
does.
SeoYeonKim