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
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>
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.
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.
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.
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.
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>
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.
_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.
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.
_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.
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.
- _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.
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.
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.
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.
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.
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.
`_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.
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.
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.
kshitijk4poor