"The replace/remove ops of a staged payload, single or batch" was spelled
three ways across apply_memory_pending, the CLI pending list and the pin
step, one of them re-typing _BG_DELETE_ACTIONS as a literal tuple. One
destructive_ops() helper beside the constant replaces them, and the CLI
drops isinstance guards on records this code itself wrote (the apply path
never checked them either).
Tests: the legacy/unpinned case had its own outcome and an early return
inside the parametrised "names the removed entry" test; it is its own
test now, so each name states what it asserts.
A destructive pending record staged before entry pinning carries only
its old_text search string. Replaying that search at approve time is
exactly the #120841 hazard: the live agent may have rewritten the entry
in place, and a newer entry that still contains old_text gets deleted or
overwritten. Naming the removed entry afterwards does not undo it.
So apply_memory_pending now fails closed: a replace/remove (single or any
batch op) without matched_entry is refused with nothing applied, the
record stays pending, and the message tells the approver to reject it and
recreate the change. /memory pending flags such records as an unpinned
legacy target so this is visible before approving.
The legacy case of test_approve_names_the_entry_a_remove_deleted now
asserts refusal + preserved record + intact entry; two existing tests that
hand-build replay payloads pin them with matched_entry, as real staging
does since the previous commit.
Co-authored-by: JoaoMarcos44 <joaomarcosdias444@gmail.com>
A staged replace/remove recorded only its old_text search string, and
/memory approve re-ran that search against the file as it was at
approval time. The background review stages every replace/remove it
wants (#105921), so when the live agent updated the targeted entry in
place and the new text still contained old_text, approving deleted or
overwrote the newer entry the approver never saw, and the output said
only "Approved 1 memory write(s)."
- Both staging gates resolve each replace/remove, under the store lock,
to the full entry it matches and record it as matched_entry. No match
or an ambiguous match is refused at staging, as the direct write is.
- Approval matches that entry exactly. If it changed since staging, the
write is refused and the pending record is kept, as #96059 does for
skills.
- /memory approve lists removed entries next to overwritten ones, so a
record staged before this change (no matched_entry, still replayed by
old_text) never removes an entry silently.
- /memory pending shows the full entry each staged replace/remove
targets, not just its search string.
(cherry picked from commit 28700fa750c9321ded48be77a76531b5809f5fea)
The store now returns replaced_entry / replaced_entries, but the write-approval
replay (the exact path #117952 was lost on) went through
hermes_cli.write_approval_commands._apply_one, which kept only (success, error)
and printed "Approved N memory write(s)." -- the overwritten text never reached
the approver. Return the full applier result and list every overwritten entry
under the approve output (CLI and gateway share this handler).
Also re-word the staged preview: "- replace: old -> new" reads as a span patch,
which is exactly the misreading that made the approver wave the batch through.
It now says "replace entry matching '<old>' -> whole entry becomes: <new>".
One invariant test, red on the PR head (approve output lacked the entry text).
Quality fold on the #117952 salvage stack: replaced_entries keys now match
the 1-based "Operation N" numbering the model sees for failed ops in the
same batch response (0-based dict keys JSON-serialialize to "0" and would
misalign with "Operation 1"); _apply_batch_op docstring states the actual
None/None-success semantics; the missing-old_text replace hint is hoisted
out of the f-string concat.
90/90 across the memory, schema, import-fallback, write-approval,
background-review-scope and honho-write suites.
A partial-entry replace (old_text = a span inside an entry, content = its
replacement) silently overwrote the WHOLE entry: every clause outside the
match was destroyed with success=true, most damagingly through the
write-approval replay path (/memory approve -> apply_memory_pending ->
apply_batch) that exists precisely so background agents can stage writes for
human review (#117952, dup of #59184).
The cluster's repeated fix attempt (#66321, #61357, four closed predecessors)
flipped replace to naive span-splicing (entry.replace(old_text, new, 1)).
That corrupts the canonical identifier-style call (replace(old_text="3.11",
content="Python 3.12 project") -> "Python Python 3.12 project project"),
breaks the background-review refine consumer, and split-brains external
memory mirrors, which receive the raw op content, not a merged entry.
The defect is the CONTRACT, not the mechanism: the schema invited patch-style
calls ("mirrors old_text (patch-tool shape)") while the store commits
whole entries. This pins whole-entry semantics end to end and makes the
overwrite visible instead of silent:
- schema + recovery error text + memory_tool docstring now say content is the
COMPLETE new entry and old_text only locates it (website docs updated in the
same PR)
- a successful replace surfaces replaced_entry / replaced_entries (full text
that was overwritten) on single, batch, and approval-replay paths, so the
caller can re-add lost clauses instead of discovering them gone later
- _mutate gains an optional extra payload field; _apply_batch_op returns the
replaced entry text alongside the error
Live repro on origin/main dec236b21 before: approve-apply of a staged
partial-entry replace truncated a 3-clause entry to the replacement span
alone. After: same final entry (whole-entry contract, unchanged semantics)
plus replaced_entries in the response. Grafted naive span-splice on main:
2 existing tests red (test_replace_entry,
test_attended_review_keeps_full_operation_set) — the regression this PR
avoids. Mutation checks: reverting the visibility fields or the exact-match
priority turns the new invariant tests red.
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.
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).
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.
Every PLUGIN-COMPAT __getattr__ now calls hermes_cli.plugin_compat.warn_once(facade, name, target) before
resolving, emitting a HermesPluginCompatWarning (FutureWarning) once per process per name: old path, new
path, removal target. Importing a facade for its live API stays silent; only resolving a moved name warns.
COMPAT_MANIFEST.md documents the warning and how to silence it during migration.
Verified the runtime never routes through a pointer: every entry point (run_agent, cli, hermes_cli.main,
gateway.run, tui_gateway.server, web_server, model_tools + tool discovery, hermes_state, cron.scheduler,
browser_tool, mcp_tool, kanban, auth) imports clean and `hermes doctor` runs end to end with the warning
promoted to an error.
Also restores the check_compat_pointers CI step to .github/workflows/lint.yml, which a0be177aac dropped
when the compat layer was regenerated (the lint script itself was present; the workflow step was not).
hermes_cli/plugin_compat.py, tests/test_plugin_compat_warning.py and the two-line insert per facade are
part of the compat layer and go away with it.
The Sep 2026 decomposition (PR #102117) makes internal import paths a non-API: names now live in
the focused modules that define them. This commit is the ONLY thing keeping the old paths alive,
so external plugins have time to update. It is deliberately a single, unsquashed commit:
git revert <this sha>
removes every shim, stub and manifest at once on the announced date. Nothing in-tree may depend on
these pointers: scripts/check_compat_pointers.py (wired into lint.yml) fails CI if it does.
What it adds (see COMPAT_MANIFEST.md, compat_manifest.json):
- 332 facade modules get one delimited `PLUGIN-COMPAT` block appended at the end of the file
- 1,172 moved names resolved lazily via a module `__getattr__` (PEP 562) — never a top-level import,
so no import cycles; facades that already had `__getattr__` get a chained one
- 592 third-party/stdlib names the old modules used to expose, with their original import statements
- 266 public definitions that had been deleted as unused, restored byte-for-byte from the pre-decomposition
tree (+40 private helpers and 16 imports pulled in only because a restored definition needs them)
- 3 deleted modules recreated as re-export stubs (gateway/startup_watchdog, hermes_cli/observability/
relay_runtime, tools/environments/modal_utils)
- private names (`_x`) get no pointer: they were never API (3,792 skipped)
Verified: all 335 touched modules import under a fresh HERMES_HOME and every manifest name resolves;
the lint reports zero in-tree uses; ruff clean; targeted suites unchanged.
The memory guidance led with 'Save proactively' and the memory tool schema
ranked 'user preferences & corrections' as top priority, while the skills
nudge was a conditional 'offer to save'. In practice that asymmetry made
the agent end sessions writing memory entries (fighting a 2,200-char
budget) and skip updating the skill it had just used, even though the
procedure was the reusable artifact. Both surfaces now state the same
rule with skills first: what you learn doing a task, including the user's
preferences and corrections for that kind of work, goes in the task's
skill; memory is only for facts that apply to every session.
memory_tool_store: _error() helper for every failure dict, reload folded into
_mutate, load_from_disk loops over targets with an inline snapshot sanitizer,
_locate inlined into _edit, batch-op splice, drift check compacted; on-disk
format and every result string unchanged (golden corpus).
memory_tool: single _apply_write_gate handles op + batch, _batch_op_line helper,
gate/validation returns folded, on-disk store built directly.
session_search_tool: _get_session_meta unifies 5 get_session lookups, rebuild
note folded into _discover_payload via _ok, lineage dedupe inlined into
_discover, scroll rebind inlined, title-match shaping via one closure, browse
comprehension, unreachable scroll guard dropped, single hermes_state import.
microsoft_graph_client/auth: signatures hugged, bodiless-response handling
inlined, header dict built in one expression, dead from_env removed.
process_registry_notifications: _preamble merges header/task-source/role lines,
_notice_lines shared, table-driven completion status, comprehension headers.
Schemas byte-identical (SCHEMA-OK); golden corpus old-vs-new identical.
Route the model-supplied target through _bound_error_text so a huge
bogus target can't bloat context, and restore the "Use 'memory' or
'user'" hint. Follow-up to HexLab98's review note on the salvage.
Normalize malformed memory config during initialization and bind per-target write permissions to the session MemoryStore so direct and staged writes cannot update a disabled built-in store.
Reuse the built-in store predicate during agent initialization and evaluate the config-backed memory tool check immediately after edits instead of applying the generic external-probe TTL.
With memory.memory_enabled and memory.user_profile_enabled both false,
agent_init never builds a MemoryStore -- but check_memory_requirements()
returned True unconditionally and MEMORY_GUIDANCE was gated only on the
tool being present in valid_tool_names. So the tool shipped in every
request's schema while answering "Memory is not available" on every call,
and the system prompt still told the model to save durable facts there.
Gate both on the config flags, using the store predicate for the tool and
the already-resolved agent state for the guidance (config is not re-read
mid-conversation, so the prompt stays byte-stable). Either flag alone
still backs the tool, so only turning both off removes it.
This lets a user running a third-party provider (Hindsight, Mem0, ...)
turn the built-in files off without paying for the dead surface on every
API call. The provider's own tools are unaffected: hiding the built-in
tool moves the decision onto the toolset gate, and listing memory under
agent.disabled_toolsets remains the only switch that takes those down.
memory replace/remove target an entry by old_text but supply the replacement
via content — an asymmetric pairing. Callers naturally reach for new_text to
mirror old_text (it's exactly the patch tool's old_string/new_string shape),
which left content empty and errored 'content is required'. The failure also
rendered tersely, making the cause easy to miss and costing a retry.
Accept new_text as an alias for content on both shapes:
- single-op: coalesce content = content or new_text in memory_tool() + a
new_text param wired through the registry handler.
- batch ops: content = op.content or op.new_text (and in the approval-gate
preview builder).
- schema: document the alias on content and add a new_text property to the
single-op params and batch item props so strict validators accept it.
content wins if both are set.
Tests: new_text alias on single add/replace, batch add/replace, and
content-wins-over-new_text. 43/43 across memory tool + schema suites.
Co-authored-by: Teknium <teknium1@users.noreply.github.com>
Complements the cherry-picked contributor fixes and closes out the
remaining sites of the 'missing explicit encoding' bug class, which is
now permanently gated by ruff PLW1514 (enabled repo-wide in
pyproject.toml and enforced by the blocking `ruff check .` step in
.github/workflows/lint.yml):
- tools/memory_tool.py: read MEMORY.md/USER.md via utf-8-sig so a
Notepad BOM never glues U+FEFF onto the first entry (issue #10878,
PR #10888 by @easyvibecoding — strict-decode contract of
_read_raw_checked preserved rather than errors="replace", so
undecodable files still refuse read-modify-write instead of being
lossily rewritten). Regression tests included.
- tools/skills_tool.py: SKILL.md and skill file reads pinned to
utf-8-sig + errors="replace" — deterministic across platforms instead
of the locale fallback proposed in PR #51701 (superseded: falling back
to cp1252/GBK makes the same skill render differently per host); .env
reader aligned with the canonical utf-8-sig dialect in hermes_cli/config.py.
- agent/shell_hooks.py, hermes_cli/main.py, gateway/slash_commands.py:
explicit utf-8 on the remaining fdopen/open text-mode sites flagged by
the AlexFucuson9 sweep series (#56033#56940#65565#66782#66791).
Co-authored-by: easyvibecoding <easyvibecoding@users.noreply.github.com>
Co-authored-by: AlexFucuson9 <AlexFucuson9@users.noreply.github.com>
Co-authored-by: flyingdoubleg <wangzhe00zju@gmail.com>
Co-authored-by: LeonSGP43 <cine.dreamer.one@gmail.com>
Deduplicates the mkstemp→fsync→atomic_replace pattern that existed in
three places: agent_import.py (added by #72983), MemoryStore._write_file,
and skill_manager_tool._atomic_write_text. All three now call a single
utils.atomic_write_text helper.
Also wraps the atomic_write_text call in _merge_memory_entries with
try/except OSError so a write failure records a per-item error instead
of propagating uncaught and aborting the entire import with no record.
Follow-up to #72983.
Follow-up hardening on top of the salvaged #69745 guard, addressing both
review findings:
- Drift detection no longer re-reads the file. _reload_target performs ONE
checked read and derives both the drift check and the entry parse from that
same raw snapshot (_detect_external_drift now takes the raw text). The old
second read swallowed OSError as 'no drift', so a read failure between the
two reads let replace/remove/apply_batch rewrite the file from a stale view,
discarding externally added entries.
- Invalid UTF-8 now counts as unreadable: the checked read catches
UnicodeDecodeError and mutations return the preservation refusal instead of
raising (or worse, rewriting bytes we can't round-trip).
- USER.md is covered by the same guard (shared _reload_target path) and now
pinned by an explicit test.
Tests: read-once structural invariant, invalid-UTF-8 refusal with
byte-identical file, user-store refusal.
`_read_file` degraded any read failure to `[]`, conflating "file exists but
couldn't be read" with "empty store". That is a silent, total data-loss bug on
the `add` path.
`add` re-reads the file under lock, appends the new entry, and rewrites the
WHOLE file from the parsed entries. It deliberately skips the drift guard
(#42874: "appending never clobbers existing content") — but that reasoning only
holds when the reload actually saw the file. When `read_text` raises
(an external editor momentarily holding the file on Windows, a permission
change, a filesystem/EINTR blip), `_read_file` returns `[]`, so `add` treats
the store as empty and rewrites the file down to just the new entry — every
prior memory gone — while returning `success: True`.
Reproduced with a transient read failure during `add`:
entries on disk before : 3 (dark-mode pref, deadline, deploy target)
add("A brand new fact") : success=True
entries on disk after : 1 ("A brand new fact") <-- the other 2 wiped
replace/remove/apply_batch were shielded only incidentally — an empty view
means `old_text` never matches, so they abort before writing — but they still
returned a misleading "no entry matched" instead of naming the real problem.
Fix: distinguish unreadable from empty. `_read_entries_checked` returns
`(entries, read_ok)`, with `read_ok=False` only when the file exists but can't
be read; absent/empty stays a clean `([], True)`. `_reload_target` returns a
`_READ_FAILED` sentinel in that case without touching in-memory state, and all
four mutation paths (add, replace, remove, apply_batch) refuse the write with a
clear "retry in a moment" error. This is the same posture as the drift guard
and the pairing/checkpoint fixes: never rewrite a file from a view that isn't
the real one. `_read_file` keeps its `[]`-on-error contract for the read-only
`load_from_disk` caller, which never persists.
tests/tools/test_memory_tool.py: new TestUnreadableFileDoesNotWipeMemory —
add/replace/remove/apply_batch all refuse and leave the file byte-identical on
a transient read failure, plus controls that an absent file is still a clean
empty store and the happy path is undisturbed. The four refusal tests fail on
main. Full suite: 90 passed, 1 pre-existing failure (`test_deduplication_on_load`,
a UnicodeDecodeError unrelated to this change, identical on clean main).
The salvaged retention check compared the built-in memory snapshot
before vs after the disk reload. That holds for a long-lived CLI agent,
but on fresh-agent surfaces (gateway per-turn agents, TUI) the cached
prompt is restored from the session DB and can predate mid-session
memory writes that the fresh MemoryStore already absorbed at init: the
snapshot is then identical on both sides of the reload while the prompt
itself is stale, so compression would retain (and re-persist via
update_system_prompt) a prompt missing the new memory for the life of
the session.
Replace the equality check with a containment check
(_cached_prompt_reflects_builtin_memory): retain the cached prompt only
when the freshly-reloaded rendered blocks appear verbatim inside it,
and rebuild when a leftover block header remains for a target whose
entries have since been emptied or disabled. Block headers are shared
via MEMORY_BLOCK_HEADERS in tools/memory_tool.py so the check stays in
lockstep with MemoryStore._render_block.
Adds regression guards for the gateway stale-restore path and the
emptied-memory leftover-block path; verified with a real-MemoryStore
E2E matrix (9 scenarios) against a temp HERMES_HOME.
Port from nearai/ironclaw#4547: treat a JSON null memory target as omitted so strict providers that fill optional fields with null use the documented default target instead of failing validation.
Builds on the zero-match feedback fix (previous commit) to close the silent-hang
symptom: when memory is at capacity, a failed `add`/`replace`/`remove`
consolidation could loop the whole turn to iteration-budget exhaustion and
deliver no user-facing reply.
#41755 turned the at-capacity overflow error into a *commanded* in-turn retry
("...then retry this add — all in this turn"); combined with the fragile
substring-only `replace`/`remove` matching (LLMs can't reliably re-quote a long
entry verbatim), the model loops add↔replace on inexact guesses until the turn
dies. The existing tool_guardrails halt would catch this, but hard_stop_enabled
is opt-in (off by default), so a default install still hangs.
This fixes it at the memory layer without changing global guardrail behavior:
- MemoryStore tracks per-turn consolidation failures; after a cap (3) it drops
the "retry in this turn" instruction and returns a terminal "leave memory
unchanged, continue your reply" result, so a failed memory side effect can
never block the turn's reply.
- The counter resets on any successful write (progress) and at each turn
boundary (turn_context.reset_consolidation_failures, guarded via getattr so
plugin memory stores without the method are a no-op).
Co-authored-by: liuhao1024 <sunsky.lau@gmail.com>
- replace() and remove() now return entry previews and current_entries
when no entry matches old_text, matching the multi-match and add-limit
error behavior
- add() limit error also now returns previews for consistency
- Agent can self-correct after a failed replace/remove instead of looping
blindly until turn budget is exhausted with no user response
The drift guard (introduced for #26045) correctly protects replace/remove
from clobbering un-roundtrippable content, but it also fires on the add
path. Since add only appends and never overwrites, the guard is
unnecessary and causes false positives when prior add() calls in the same
session shift the byte count of the on-disk file.
Add skip_drift parameter to _reload_target() and pass True from add().
Replace/remove continue to use the drift guard unchanged.
Salvaged from #42880 by @liuhao1024.
Closes#42874
Follow-up to the /memory approve fresh-store fix. Both the CLI fallback and
the messaging-gateway handler built a bare MemoryStore() with the hardcoded
default char limits (2200/1375), ignoring the user's configured
memory.memory_char_limit / user_char_limit. A live agent honors those
overrides (agent/agent_init.py), so an approval applied without a live agent
could accept a write the user's lower cap would reject, or vice versa.
Extract a shared tools.memory_tool.load_on_disk_store() factory that reads
the configured limits (falling back to defaults if config can't load) and
wire both the CLI and gateway handlers to it, closing the gap on both
surfaces and de-duplicating the construction block.
Single-op replace/remove failed with a dead-end 'old_text is required'
error when a structured-output client omitted the optional old_text field
(it can't be schema-required without a top-level if/then combinator that
OpenAI's Codex backend 400s on). The model couldn't recover.
Now a missing old_text returns the current entry inventory plus a retry
instruction (mirroring the batch path's _batch_error), so the model can
reissue the call with old_text set. Also sharpens the old_text schema
description to state it's required for replace/remove.
Fixes#49466, #43412.
* docs: clean up three stale comments from the #32848 audit
- tools/memory_tool.py:20 — 'read' action was intentionally removed
but the docstring still listed it. Now matches the schema.
- tools/fuzzy_match.py:9 — unicode_normalized was added but the
chain-count docstring still said '8-strategy'. Now says '9'.
- run_agent.py:1485 — 'See #<TBD>.' placeholder was never filled in.
Replaced with a backfill note.
Fixes#32848 (parts 3, 4, and 12)
* docs(memory): also remove stray memory(action=read) references in lines 144 and 201
The original #32848 audit fix (in 6fd661d6) only addressed line 20
(the action list in the module docstring), but the action was
referenced in two other places:
- tools/memory_tool.py:144 — in a class docstring, claimed
'memory(action=read)' was a way to SEE poisoned entries
- tools/memory_tool.py:201 — in a user-facing warning message,
told the user to 'use memory(action=read) to inspect'
Since the schema on line 683 only allows add/replace/remove, both
references were misleading: the first claimed a way to inspect
poisoned entries that doesn't exist, the second would error out
when the user followed the warning.
This commit removes both references:
- Line 144: '...keep the original text so the user can still SEE
poisoned entries by inspecting the source files directly, and
remove them — silently dropping them would hide the attack
from the user.'
- Line 201: '...use memory(action=remove) to delete the
original. (drop the read-action reference)'
Followup to the previous commit on this branch.
---------
Co-authored-by: KeyArgo <keyargo@argobox.com>
The memory tool was strictly one-op-per-call. With the store running near
its char limit by design, a new add that would overflow gets rejected with
'consolidate now, then retry' -- but the model could not consolidate and add
in one call. It had to remove/replace across several turns, then retry the
add, each turn re-sending the whole conversation context. Expensive thrash.
Add an 'operations' array: a list of add/replace/remove ops applied
atomically against the FINAL char budget. The model frees space and adds new
entries in ONE call, even when an add alone would overflow. All-or-nothing:
any bad op aborts the whole batch, nothing written.
Root-cause note: the two agent-level memory interception sites
(agent_runtime_helpers.py, tool_executor.py) silently dropped any param not
in their explicit kwarg list, so 'operations' never reached the handler and
batch calls failed with 'Unknown action None'. Both now pass it through and
bridge each add/replace op to external memory providers.
Also: success response is now terminal (done=true + 'do not repeat' note,
no full-entries echo that invited re-edits); schema rewritten to lead with
the batch mechanism and an explicit one-shot stop rule (2138 -> 1476 chars).
Live-verified: near-full consolidate-and-add went 7 calls -> 1 call,
stable across 3 reps. 103 memory/approval tests + 398 background-review/
run_agent tests green; 6 new batch tests added.
Follow-ups to #38199/#43354 found in post-merge review:
- Inline CLI memory approval never worked: the per-thread approval callback
was not passed to prompt_dangerous_approval, so the prompt_toolkit
fail-closed guard (#15216) denied every gated foreground write without
showing a prompt. Now invokes the registered callback directly; a crashed
prompt falls back to staging instead of a silent deny.
- Gateway sessions claimed inline support but prompt_dangerous_approval has
no gateway round-trip (that lives in the pending-approval queue), so gated
gateway memory writes hit the input() fallback and denied. Gateway
contexts now stage for /memory pending review.
- /skills pending|approve|reject|diff|approval now works on the gateway
(gateway_config_gate on skills.write_approval), so skills staged from a
messaging session can be reviewed there. Diff output truncated for chat.
- memory_tool validates required params before the gate so invalid writes
are rejected immediately instead of staged and failing at approve time.
- Stale tri-state write_mode docstrings updated to the boolean gate; docs
table corrected (inline prompt is interactive-CLI-only).
- 6 new tests covering the interactive approve/deny/error paths, gateway
staging, skills never-prompt invariant, and pre-gate validation.
The shipped tri-state write_mode (on|off|approve) conflated two concepts —
whether writes are enabled and whether they're gated — so 'on' (writes flow
freely, gate inactive) read like 'gating is on'. Replace it with a single
clear boolean gate that defaults off.
memory.write_approval / skills.write_approval:
false (default) — write freely; the approval gate is off (pre-gate behaviour)
true — require approval: memory foreground prompts inline, memory
background-review + all skill writes stage for review
The old 'off = block all writes' mode is dropped; memory_enabled: false already
disables memory entirely, so a third 'block' state was redundant.
- tools/write_approval.py: get_write_mode/MODE_* → write_approval_enabled() bool;
evaluate_gate() loses the config-driven 'blocked' path (blocked now only comes
from an interactive user denial).
- tools/memory_tool.py, tools/skill_manager_tool.py: comment + behaviour follow.
- hermes_cli/config.py: memory/skills write_mode → write_approval (False);
_config_version 28→29 with a 28→29 migration that renames any persisted
write_mode (approve→true, on/off/unset→false) and drops the old key.
- slash commands: '/memory|/skills mode <on|off|approve>' → 'approval <on|off>'
('mode' kept as a back-compat alias); set_mode_fn callback now takes a bool.
- write_approval_commands.py, cli_commands_mixin.py, gateway/slash_commands.py,
commands.py: handlers + registry args/subcommands updated.
- docs + tests rewritten for the boolean model; added migration tests.
Adds memory.write_mode and skills.write_mode (on|off|approve), applied to
both foreground turns and the background self-improvement review fork — the
source of the unprompted 'wrong assumption' saves users reported.
- on (default): write freely, unchanged behaviour
- off: never write; the tool returns a clean disabled result
- approve: don't commit. Memory foreground writes prompt inline (small,
reviewable in a chat bubble); background memory writes and ALL skill writes
stage to a pending store instead (a SKILL.md is too large to review inline,
and a daemon thread can't block on a prompt)
Review staged writes from CLI or any messaging platform:
/memory pending|approve|reject|mode
/skills pending|approve|reject|diff|mode
Skill review respects the size asymmetry: inline you see a one-line gist;
the full unified diff stays out-of-band (/skills diff, dashboard, or the
staged JSON file).
New: tools/write_approval.py (gate + pending store), hermes_cli/
write_approval_commands.py (shared CLI+gateway handlers). Gates wired at the
single entry points memory_tool() and skill_manage(), using the existing
write-origin ContextVar to distinguish foreground from background_review.
* fix(memory): make overflow errors instruct in-turn consolidation + retry
When bounded memory is full, the add/replace overflow errors now explicitly
tell the model to consolidate (merge/remove/shorten) and retry the write in
the same turn, matching the documented behavior. The replace-overflow path
now also echoes current_entries + usage for parity with add-overflow, so the
model has the same context to act on.
Closes#23378 (working-as-documented; this sharpens runtime to match docs).
* fix(memory): broaden overflow remediation hint beyond 'stale'
Say 'stale or less important' — entries don't have to be stale to be the
right ones to drop when making room.
Remove unused imports (F401) and duplicate/shadowed import
redefinitions (F811) across the codebase using ruff's safe
autofixes. No behavioral changes -- imports only.
- ~1400 safe autofixes applied across 644 files (net -1072 lines)
- __init__.py re-exports preserved (excluded from F401 removal so
public re-export surfaces stay intact)
- Re-exports that are imported or monkeypatched by tests but look
unused in their defining module are kept with explicit # noqa:
F401 (gateway/run.py load_dotenv; run_agent re-exports from
agent.message_sanitization, agent.context_compressor,
agent.retry_utils, agent.prompt_builder, agent.process_bootstrap,
agent.codex_responses_adapter)
- Unsafe F841 (unused-variable) fixes deliberately skipped -- those
can change behavior when the RHS has side effects
- ruff lints remain disabled in pyproject.toml (only PLW1514 is
selected); this is a one-time cleanup, not a config change
Verification:
- python -m compileall: clean
- pytest --collect-only: all 27161 tests collect (zero import errors)
- core entry points import clean (run_agent, model_tools, cli,
toolsets, hermes_state, batch_runner, gateway)
- static scan: every name any test imports directly from an edited
module still resolves
Hardens the context window against Brainworm-class promptware attacks
(see #496). Three changes:
1. tools/threat_patterns.py — single source of truth for injection/promptware
patterns. Replaces the duplicated pattern lists in prompt_builder.py and
memory_tool.py. Adds ~15 new Brainworm/C2 patterns (node registration,
heartbeat/beacon, pull tasking, anti-forensic disk avoidance, identity
override, known framework names). Three scopes — 'all' (narrow, classic
injection), 'context' (adds promptware/role-play, broader detection),
'strict' (adds persistence/SSH-backdoor patterns for user-mediated writes).
2. MemoryStore.load_from_disk() now scans entries at snapshot-build time.
Poisoned entries are replaced with [BLOCKED: ...] placeholders in the
frozen system-prompt snapshot. Live state keeps the original so the
user can still inspect + remove via memory(action=read/remove). Scan is
deterministic from disk bytes — prefix-cache invariant holds.
3. make_tool_result_message() wraps results from high-risk tools
(web_extract, web_search, browser_*, mcp_*) in
<untrusted_tool_result source="...">...</untrusted_tool_result>
delimiters with framing prose telling the model the content is data,
not instructions. Architectural defense against indirect injection
from poisoned web pages, GitHub issues, MCP responses — does NOT
regex-scan tool results (pattern arms race + per-iteration latency).
Multimodal content lists pass through unwrapped to preserve adapter
compatibility.
Pattern philosophy: anchor on C2-specific vocabulary or unambiguous attack
behavior, NOT on bossy English. Dropped patterns suggested in #496 that
would have tripped legitimate content: standalone 'you are obligated to',
'do not respond immediately', 'you must X' without a C2-verb anchor.
Validation:
- 257/257 targeted tests pass (test_threat_patterns + test_memory_tool +
test_tool_dispatch_helpers + test_prompt_builder)
- E2E run with real Brainworm payload: blocked from AGENTS.md context-file
path, blocked from MEMORY.md snapshot, wrapped in delimiters when
arriving via web_extract. Legitimate 'you must follow conventions'
phrasing not flagged.
Explicitly NOT in this PR (per #496 discussion):
- Per-tool-result regex scanning (pattern arms race)
- SessionBehaviorMonitor / polling-loop detection (wrong layer)
- Outbound network gating (Docker backend already covers this)
- security.context_scanning warn|block knob (current behavior is always
block-with-placeholder — there's no warn mode that makes sense)
Closes#496 for Phase 1 + the architectural delimiter piece of Phase 2.
Phase 3 stays in tracking issue territory.
Expand _MEMORY_THREAT_PATTERNS from 13 to 24 regex patterns and align
_INVISIBLE_CHARS with skills_guard.py (10 → 17 characters).
Key changes:
- Add multi-word bypass prevention (?:\w+\s+)* to injection patterns
- Add missing injection patterns: role_pretend, leak_system_prompt,
remove_filters, fake_update, translate_execute, html_comment_injection,
hidden_div
- Add exfiltration patterns: send_to_url, context_exfil
- Add persistence patterns: agent_config_mod, hermes_config_mod
(both require modification-verb prefix to avoid false positives on
mere mentions of config filenames)
- Add hardcoded secret detection pattern
- Add role_hijack precision fix: require article after "now" to avoid
blocking "you are now ready/connected/set up" etc.
- Expand invisible unicode set with directional isolates (U+2066-2069)
and invisible math operators (U+2062-2064)
Test coverage expanded from ~8 to ~30 scan tests including dedicated
false-positive regression tests for all precision-sensitive patterns.
Known limitations (deferred to follow-up PRs):
- prompt_builder.py and cronjob_tools.py still use older pattern sets
- No semantic/LLM-based scanning (regex-only approach)
- No cross-entry or cross-store analysis
Reproduction (production, 2026-05-14): two concurrent sessions on the
same agent. Session A patches MEMORY.md directly via the patch tool,
appending ~8KB of structured content (Vendor Master, Standing Orders,
Pin Board) — none of it through the memory tool, so no § delimiters.
Session B starts later with stale in-memory state (1 entry, ~331
chars). Session B calls memory(action=replace) on its one known
entry. The tool's _read_file parses A's content as a single 8KB
'entry' (no § splits), then replace truncates that entry to B's new
333-byte content. ~8KB of structured content silently destroyed.
The atomic-rename write path is fine in isolation. The bug is the
implicit contract: the tool assumes MEMORY.md is exclusively a
§-delimited list of small entries it wrote, but the v0.13 install
runbook itself uses 'cat >> MEMORY.md' for onboarding, the patch tool
edits the file directly, and operators do too.
Fix: a drift guard in MemoryStore._detect_external_drift that fires
on either signal:
1. Re-parse + re-serialize doesn't produce identical bytes
(catches oddly-encoded delimiters / partial writes).
2. Any single parsed entry exceeds the store's whole-file char
limit. The tool budgets the ENTIRE store against that limit
(2200 chars for memory, 1375 for user), so no tool-written
entry can legitimately be larger. An entry bigger than the
store limit means an external writer dropped free-form content
into what the tool will treat as one entry.
When drift fires, _reload_target writes a .bak.<ts> snapshot of the
on-disk file, then add/replace/remove refuse to flush. The original
file stays untouched. The error dict surfaces the .bak path AND a
remediation string ('integrate missing entries via memory(add=...)
one at a time, then rewrite the file clean') so the model can act on
it without escalating to the operator.
Tests:
- test_replace_refuses_on_drift, test_add_refuses_on_drift,
test_remove_refuses_on_drift — all three mutators refuse
- test_clean_file_does_not_trigger_drift — false-positive check
- test_error_message_points_at_remediation — error string shape
- test_drift_guard_also_protects_user_target — USER.md too
- test_drift_backup_filename_is_unique_per_invocation — bak.<ts>
naming pin
144 memory tests passing (was 137; +7).
Fixes#26045
1. trajectory_compressor.py: yaml.safe_load() returns None on empty
files, crashing with TypeError on `if 'tokenizer' in data`. Fix by
adding `or {}` fallback. (HIGH — blocks startup with empty config)
2. 6 files with fcntl.flock(LOCK_UN) in finally blocks without
try/except: cron/scheduler.py, hermes_cli/auth.py,
agent/shell_hooks.py, tools/skill_usage.py,
tools/environments/file_sync.py, tools/memory_tool.py. If unlock
raises OSError, fd.close() is skipped and the lock is held forever.
The msvcrt branches already had try/except; the fcntl branches did
not. Fix by wrapping in try/except (OSError, IOError): pass.
3. agent/copilot_acp_client.py line 639: TOCTOU race — path.exists()
followed by path.read_text() with no try/except. If file is deleted
between the check and the read, FileNotFoundError propagates. Fix
by using try/except FileNotFoundError.
4. gateway/sticker_cache.py: non-atomic write via Path.write_text()
can leave truncated JSON on crash, causing JSONDecodeError on next
load. Fix by writing to tempfile + fsync + os.replace (atomic).
On Windows (msvcrt path), _file_lock() first checked if the lock file
existed and wrote it with write_text(), then opened it with open('r+').
Between these two calls, another process could delete the file causing
open('r+') to raise FileNotFoundError — uncaught, leaving memory writes
to proceed without holding the lock, risking data corruption.
Replace the three-line sequence with a single open('a+', ...) call which
atomically creates the file if missing or opens it if it exists, closing
the TOCTOU window entirely. The existing fd.seek(0) before msvcrt.locking()
is preserved and sufficient for correct lock byte positioning.
Root cause: TOCTOU between lock_path.write_text() and open('r+')
Impact: concurrent memory writes on Windows could corrupt MEMORY.md
Replace with for all literal-tuple
membership tests. Set lookup is O(1) vs O(n) for tuple — consistent
micro-optimization across the codebase.
608 instances fixed via `ruff --fix --unsafe-fixes`, 0 remaining.
133 files, +626/-626 (net zero).
## Why
Hermes supports Linux, macOS, and native Windows, but the codebase grew up
POSIX-first and has accumulated patterns that silently break (or worse,
silently kill!) on Windows:
- `os.kill(pid, 0)` as a liveness probe — on Windows this maps to
CTRL_C_EVENT and broadcasts Ctrl+C to the target's entire console
process group (bpo-14484, open since 2012).
- `os.killpg` — doesn't exist on Windows at all (AttributeError).
- `os.setsid` / `os.getuid` / `os.geteuid` — same.
- `signal.SIGKILL` / `signal.SIGHUP` / `signal.SIGUSR1` — module-attr
errors at runtime on Windows.
- `open(path)` / `open(path, "r")` without explicit encoding= — inherits
the platform default, which is cp1252/mbcs on Windows (UTF-8 on POSIX),
causing mojibake round-tripping between hosts.
- `wmic` — removed from Windows 10 21H1+.
This commit does three things:
1. Makes `psutil` a core dependency and migrates critical callsites to it.
2. Adds a grep-based CI gate (`scripts/check-windows-footguns.py`) that
blocks new instances of any of the above patterns.
3. Fixes every existing instance in the codebase so the baseline is clean.
## What changed
### 1. psutil as a core dependency (pyproject.toml)
Added `psutil>=5.9.0,<8` to core deps. psutil is the canonical
cross-platform answer for "is this PID alive" and "kill this process
tree" — its `pid_exists()` uses `OpenProcess + GetExitCodeProcess` on
Windows (NOT a signal call), and its `Process.children(recursive=True)`
+ `.kill()` combo replaces `os.killpg()` portably.
### 2. `gateway/status.py::_pid_exists`
Rewrote to call `psutil.pid_exists()` first, falling back to the
hand-rolled ctypes `OpenProcess + WaitForSingleObject` dance on Windows
(and `os.kill(pid, 0)` on POSIX) only if psutil is somehow missing —
e.g. during the scaffold phase of a fresh install before pip finishes.
### 3. `os.killpg` migration to psutil (7 callsites, 5 files)
- `tools/code_execution_tool.py`
- `tools/process_registry.py`
- `tools/tts_tool.py`
- `tools/environments/local.py` (3 sites kept as-is, suppressed with
`# windows-footgun: ok` — the pgid semantics psutil can't replicate,
and the calls are already Windows-guarded at the outer branch)
- `gateway/platforms/whatsapp.py`
### 4. `scripts/check-windows-footguns.py` (NEW, 500 lines)
Grep-based checker with 11 rules covering every Windows cross-platform
footgun we've hit so far:
1. `os.kill(pid, 0)` — the silent killer
2. `os.setsid` without guard
3. `os.killpg` (recommends psutil)
4. `os.getuid` / `os.geteuid` / `os.getgid`
5. `os.fork`
6. `signal.SIGKILL`
7. `signal.SIGHUP/SIGUSR1/SIGUSR2/SIGALRM/SIGCHLD/SIGPIPE/SIGQUIT`
8. `subprocess` shebang script invocation
9. `wmic` without `shutil.which` guard
10. Hardcoded `~/Desktop` (OneDrive trap)
11. `asyncio.add_signal_handler` without try/except
12. `open()` without `encoding=` on text mode
Features:
- Triple-quoted-docstring aware (won't flag prose inside docstrings)
- Trailing-comment aware (won't flag mentions in `# os.kill(pid, 0)` comments)
- Guard-hint aware (skips lines with `hasattr(os, ...)`,
`shutil.which(...)`, `if platform.system() != 'Windows'`, etc.)
- Inline suppression with `# windows-footgun: ok — <reason>`
- `--list` to print all rules with fixes
- `--all` / `--diff <ref>` / staged-files (default) modes
- Scans 380 files in under 2 seconds
### 5. CI integration
A GitHub Actions workflow that runs the checker on every PR and push is
staged at `/tmp/hermes-stash/windows-footguns.yml` — not included in this
commit because the GH token on the push machine lacks `workflow` scope.
A maintainer with `workflow` permissions should add it as
`.github/workflows/windows-footguns.yml` in a follow-up. Content:
```yaml
name: Windows footgun check
on:
push:
branches: [main]
pull_request:
branches: [main]
jobs:
check:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: actions/setup-python@v5
with: {python-version: "3.11"}
- run: python scripts/check-windows-footguns.py --all
```
### 6. CONTRIBUTING.md — "Cross-Platform Compatibility" expansion
Expanded from 5 to 16 rules, each with message, example, and fix.
Recommends psutil as the preferred API for PID / process-tree operations.
### 7. Baseline cleanup (91 → 0 findings)
- 14 `open()` sites → added `encoding='utf-8'` (internal logs/caches) or
`encoding='utf-8-sig'` (user-editable files that Notepad may BOM)
- 23 POSIX-only callsites in systemd helpers, pty_bridge, and plugin
tool subprocess management → annotated with
`# windows-footgun: ok — <reason>`
- 7 `os.killpg` sites → migrated to psutil (see §3 above)
## Verification
```
$ python scripts/check-windows-footguns.py --all
✓ No Windows footguns found (380 file(s) scanned).
$ python -c "from gateway.status import _pid_exists; import os
> print('self:', _pid_exists(os.getpid())); print('bogus:', _pid_exists(999999))"
self: True
bogus: False
```
Proof-of-repro that `os.kill(pid, 0)` was actually killing processes
before this fix — see commit `1cbe39914` and bpo-14484. This commit
removes the last hand-rolled ctypes path from the hot liveness-check
path and defers to the best-maintained cross-platform answer.
PR #21238 introduced top-level `allOf: [{if/then/required}]` blocks in the
built-in memory tool's parameters schema as conditional-required hints.
Two problems:
1. OpenAI's Codex backend (chatgpt.com/backend-api/codex, gpt-5.x) rejects
top-level `allOf`/`anyOf`/`oneOf`/`enum`/`not` outright with a
non-retryable 400 — affected every user on openai-codex/gpt-5.x.
2. The `if/then` hints were silently ignored by every other provider
(Chat Completions doesn't honour them on function schemas), so they
never actually enforced anything anywhere.
The runtime handler in `memory_tool()` already validates the per-action
required fields and returns actionable error messages, so removing the
block changes nothing behaviourally.
Paired with the defense-in-depth sanitizer in the previous commit, this
closes the bug both at the source (schema no longer emits the forbidden
form) and at the wire boundary (sanitizer strips it if anything else
re-introduces it).
- Rewrites `tests/tools/test_memory_tool_schema.py` to guard against
regressing the forbidden-combinator shape instead of asserting it.
- Adds AUTHOR_MAP entry for @hrkzogw (author of the sanitizer fix).
