Follow-up to the salvaged #79323 commits. The three hand-rolled
stat -> atomic_write_text -> chmod blocks (xai migration, uninstaller
shell-rc rewrite, dashboard SOUL.md editor) collapse into an opt-in
preserve_mode=True kwarg on utils.atomic_write_text, plus create_mode=
on both atomic_write_text and atomic_yaml_write for first-create paths
(SOUL.md first save, write_manifest's allowlist create path).
Beyond deduplication this closes two gaps the hand-rolled copies had:
- Owner preservation: the old in-place writes kept the inode, so file
ownership survived root-run rewrites for free. atomic_write_text
swaps in a new inode owned by the writing user, and the hand-rolled
blocks restored only the mode -- a root-run 'hermes migrate xai' or
sudo uninstall on a user-owned Docker/NAS volume would flip
config.yaml / ~/.zshrc ownership to root. preserve_mode now routes
through the same _preserve_file_owner/_restore_file_owner helpers
atomic_yaml_write and atomic_json_write already use.
- chmod-after-replace window: the mode is applied to the temp fd via
fchmod BEFORE the replace (mirroring atomic_json_write's mode= param),
so the target never transits through mkstemp's 0600.
Also removes write_manifest's caller-side existed/chmod block (and its
small TOCTOU) in favor of atomic_yaml_write(create_mode=0o644), and
corrects the SOUL.md mode comment (the default profile's runtime seeder
does run it through _secure_file; named profiles do not).
preserve_mode defaults to False so the existing callers (memory store,
skill manager, cron, agent importer) keep their current semantics.
New tests in tests/test_atomic_write_text_metadata.py cover mode
preservation, owner restore through symlinks, fchmod-before-replace,
create_mode on both writers, and no-behavior-change without opt-in;
all mutation-checked.
Both files were routed through the shared atomic writers earlier in this
branch. tempfile.mkstemp creates the temp file 0600 and the atomic swap
carries that mode onto the target, so the *create* paths silently tightened
two files that previously landed at the umask default:
- web_routers/profiles.py: the dashboard persona editor's first-ever Save has
no prior SOUL.md to copy permissions from, so the existing guard skipped the
chmod entirely -- contradicting the comment directly below it, which states
profile SOUL.md is created 0644 and is not run through _secure_file.
- profile_distribution.py: atomic_yaml_write only restores a mode it captured
from a file that already existed. _materialize() calls write_manifest() with
no manifest on disk whenever a distribution declares an explicit
distribution_owned allowlist that omits distribution.yaml, so the staged
copy is never placed in the profile.
Both are fixed with a local chmod at the two sites this branch regressed;
utils.py's public mode semantics are left alone. profiles.py now also
distinguishes "no file yet" (FileNotFoundError -> 0644) from "stat failed for
some other reason" (-> leave the mode alone rather than guess at it).
uninstall.py and xai_retirement.py have no create path and are unchanged: the
former captures prior_mode unconditionally after a successful read_text(), and
the latter runs require_readable_config_before_write() first.
`utils.atomic_write_text`'s docstring states the invariant: it exists "so that
every destructive file rewrite in the codebase shares one implementation."
Four full-file rewrites of *existing user-authored files* still bypass it and
use a bare truncating `open(path, "w")` / `Path.write_text()`, which truncates
the target before the new content is produced. A crash, SIGINT, or ENOSPC
mid-write therefore leaves the file empty or half-written.
In all four cases the read half degrades silently to a default rather than
erroring, so the damage is invisible and the next write cements it:
* `xai_retirement.apply_migration()` rewrites the user's config.yaml. Merged
commit beaa1a08e added a readability guard here and noted the writer "lives
outside the atomic_yaml_write path, so the chokepoint didn't cover it"; this
closes the durability half it left open. `--no-backup` is a documented flag,
so on that path the truncated file is the only copy that exists, and the
loader returns early on `doc is None` — the next run reports nothing to
migrate rather than surfacing the damage.
* `uninstall.remove_path_from_shell_configs()` rewrites the user's shell rc
(~/.bashrc, ~/.zshrc, ...). Hermes does not own these files and this function
takes no backup; the enclosing `except Exception` downgrades a partial write
to a warning, so the next login just starts a bare shell.
* `web_routers.profiles.update_profile_soul()` replaces SOUL.md from the
dashboard editor. The paired GET reports an unreadable file as
`{"content": "", "exists": False}`, so an interrupted save presents as "your
persona was never set" and the editor's next Save persists the empty document.
* `profile_distribution.write_manifest()` rewrites distribution.yaml on every
install/update. `read_manifest` treats an unparseable manifest as "not a
distribution", silently dropping update tracking and env_requires.
The xAI migration keeps its ruamel round-trip dumper (comments, key order and
quoting must survive) and now serializes to a string before handing the bytes
to the shared writer. `write_manifest` moves to `atomic_yaml_write`, whose
SafeDumper output the manifest already round-trips through, retiring the local
`_dump_yaml` helper.
`atomic_write_text` recreates the target from a 0600 temp file, so each of its
call sites re-applies the file's previous permission bits: `_secure_file`
deliberately leaves config.yaml alone under managed (NixOS 0640) and container
installs, shell rc files are normally 0644, and profile SOUL.md is created 0644
and never secured. `atomic_yaml_write` already preserves mode and owner itself.
Routing through `atomic_replace` also keeps a symlinked config.yaml or ~/.zshrc
(dotfiles repo, managed deployment) pointing at the real file.
Tests: one regression test per site fails on clean main (the interrupted write
completes there and destroys the file) and passes here; the remaining cases are
behaviour guards covering symlink survival, permission preservation, comment
round-tripping, and the existing happy paths.
export_profile() accepts extra_files (root-relative filename -> text) so a
caller can stage companion files into the archive; the desktop uses it for
desktop.json, its appearance/interface overlay, now part of the default
profile's export allow-list.
New routes wrapping the existing hermes profile export/import machinery:
- POST /api/profiles/{name}/export (extra_files + optional output path)
- POST /api/profiles/import (returns the bundled desktop overlay)
- GET /api/profiles/{name}/desktop-overlay
Paths cross the API, not bytes - the desktop's native dialogs and its
local/pooled backends share a filesystem.
le=100 would 422 real desktop callers: sessions-settings fetches
archived at limit=200, the command palette lists at 200, and the
electron remote-merge over-fetches limit+offset (exceeds 100 at
offset>=81, and its .catch(()=>null) silently drops remote sessions).
Clamp must sit above real client maxima. New test pins limit=200 w/
offset.
Re-derivation of aydnOktay's twin clamp PRs onto current main (the
session-list endpoints moved into web_routers/; the analytics endpoints
gained asyncio.to_thread wrappers since the originals):
- limit le=100 on /api/sessions, /api/sessions/search and the
/api/profiles/sessions fan-out (one unbounded request could drag every
session row + correlated-subquery preview work out of SQLite, times
every profile's state.db on the fan-out).
- days ge=1 le=365 on /api/analytics/usage + /api/analytics/models
(huge or non-positive values force full-history InsightsEngine work or
inverted windows; the UI only offers 7/30/90 presets).
FastAPI Query bounds reject at the validation layer (422). 8 new tests;
both clamp classes mutation-checked (clamp removed -> its tests fail).
Ports the negative limit/offset fix onto the current router modules
(hermes_cli/web_routers/sessions.py, profiles.py) since the handlers
moved out of web_server.py in 011ec4513e after this PR was opened.
Per review feedback: only add Query(..., ge=0) — no le=500. The
messages route already clamps oversized requests with min(limit, 500)
and must keep that behavior (succeed + cap) rather than reject them;
the two session-list routes never had a public 500 cap and shouldn't
gain a new rejecting one as a side effect of this fix.
kshitij
briandevans
Brooklyn Nicholson
kshitij
aydnOktay
joaomarcos
teknium1