fix(security): create plaintext transcript artifacts owner-only - #77520
fix(security): create plaintext transcript artifacts owner-only#77520ZHJay wants to merge 3 commits into
Conversation
/save snapshots, agent trajectories, and MoA traces each persist verbatim conversation content — message text, tool results, tool-call arguments — through a bare open()/json.dump(), so permissions came from the process umask (0o644 on a default install). Trajectories are the sharpest case: they append to the CWD, not to the 0o700 HERMES_HOME, so a run inside a checkout drops a world-readable full transcript next to the source. - utils: add open_private_append() for append-mode artifacts. Only the creating open applies the mode; a file the user deliberately relaxed to share or feed a training pipeline keeps its permissions. - cli /save: write the snapshot via atomic_json_write(mode=0o600). Also removes a truncated-file window — the old json.dump wrote in place, so a serialization failure mid-write left a partial snapshot behind. - trajectory + moa_trace: create the JSONL owner-only; create moa-traces/ as 0o700. - gitignore: trajectory_samples.jsonl / failed_trajectories.jsonl. Content is deliberately NOT redacted. These artifacts export the same history the session DB replays, and masking a credential in a replayed path poisons the replay (NousResearch#43083); trajectories and traces exist to be full-fidelity (training data, offline audit). Trace redaction is already available opt-in via moa.privacy_filter. This change is scoped to file modes only. Refs NousResearch#77472 (partial — file-mode items only)
There was a problem hiding this comment.
Pull request overview
Hardens plaintext transcript artifacts (CLI /save snapshots, trajectory JSONL, and MoA trace JSONL) to be created with owner-only permissions to avoid umask-derived world-readable transcripts, aligning with Hermes’ full-fidelity persistence/replay requirements.
Changes:
- Added
utils.open_private_append()to create new append-mode transcript artifacts as owner-only while preserving permissions on existing files. - Updated
/save, trajectory export, and MoA trace persistence paths to create files/dirs with private modes (0o600files,0o700trace dir) and to write snapshots atomically. - Added a POSIX-only test suite asserting the “no group/other bits on newly created artifacts” contract and ignored trajectory artifacts in
.gitignore.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| utils.py | Adds open_private_append() helper for private-by-default append artifacts. |
| cli.py | Writes /save snapshots via atomic_json_write(..., mode=0o600) to enforce private perms + atomicity. |
| agent/trajectory.py | Uses open_private_append() for trajectory JSONL creation (owner-only on first create). |
| agent/moa_trace.py | Creates moa-traces/ as 0o700 and uses open_private_append() for trace JSONL. |
| .gitignore | Ignores trajectory JSONL artifacts that are written into the CWD. |
| tests/test_transcript_artifact_file_modes.py | Adds contract tests for artifact permissions (POSIX). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if hasattr(hermes_constants, "_hermes_home_cache"): | ||
| hermes_constants._hermes_home_cache = None | ||
| for mod in [m for m in sys.modules if m.startswith("cli") or m == "hermes_constants"]: | ||
| sys.modules.pop(mod, None) |
There was a problem hiding this comment.
Fixed in 3d831557d — the purge is gone entirely rather than narrowed.
The mechanism you flagged was real, and worse than order-dependence. Measured in this repo's venv:
modules matching startswith('cli'): 12
['click', 'click._compat', 'click._utils', 'click.core', 'click.decorators',
'click.exceptions', 'click.formatting', 'click.globals', 'click.parser',
'click.termui', 'click.types', 'click.utils']
identity preserved after purge: False
All 12 matches were click; zero were cli. The loop evicted only unrelated modules and never did the thing it was there for. Popping them mid-suite breaks class identity for anything already holding a reference — a reimported click.exceptions.UsageError is a different object, so a live except UsageError no longer catches it.
On the suggested narrowing (clear the home cache, pop only cli): also not needed, because nothing is resolved at import time and there is no cache.
cli.py:8312—saved_dir = get_hermes_home() / "sessions" / "saved"sits insidesave_conversation, so it resolves per call, not at import.hermes_constants.py:114get_hermes_home()→hermes_constants.py:62_hermes_home_from_env()→hermes_constants.py:71val = os.environ.get("HERMES_HOME", "").strip(). Nolru_cache, no module-level memo. The old_hermes_home_cachereset was dead code — no such attribute exists onhermes_constants.tests/conftest.py:402autouse_hermetic_environmentalready redirectsHERMES_HOMEper test (conftest.py:441).
So the local monkeypatch.setenv is sufficient however early cli was imported. The Path.home patch was redundant with it and went in the same commit.
Current state, tests/test_transcript_artifact_file_modes.py:198-203:
# No sys.modules purge and no Path.home patch here: save_conversation
# resolves the directory through get_hermes_home() at call time, and that
# reads HERMES_HOME from the environment on every call with no caching, so
# the setenv above is sufficient no matter how early cli was imported.
# Purging modules by ``startswith("cli")`` would also evict click and its
# submodules, breaking module identity for the rest of the suite.
import cliGreen at branch head 3f34df1d5:
$ ./scripts/run_tests.sh tests/test_transcript_artifact_file_modes.py
=== Summary: 1 files, 8 tests passed, 0 failed (100% complete) in 0.4s (24 workers) ===
3d831557d also touches cli.py, but only the comment block above the atomic_json_write call — the four file-mode fixes are untouched, so there is no production behavior change in that commit.
The /save case reset module state before importing cli:
for mod in [m for m in sys.modules
if m.startswith("cli") or m == "hermes_constants"]:
sys.modules.pop(mod, None)
``startswith("cli")`` also matches ``click`` and its 11 submodules, all
imported by the time this test runs. Popping them mid-suite breaks class
identity for anything already holding a reference — a reimported
``click.exceptions.UsageError`` is a different object, so a live
``except UsageError`` no longer catches it. Verified in the venv: 12
modules matched, and identity is False after the purge.
The purge was also unnecessary. ``tests/conftest.py:441`` (autouse
``_hermetic_environment``) already redirects HERMES_HOME per test,
``get_hermes_home()`` re-reads the env var on every call with no caching,
and ``save_conversation`` resolves it at call time — so the local
``monkeypatch.setenv`` is sufficient however early ``cli`` was imported.
The ``Path.home`` patch was redundant with it, and the
``_hermes_home_cache`` reset was dead code: no such attribute exists on
``hermes_constants``.
Also rewords the ``save_conversation`` comment, which asserted "the
enclosing HERMES_HOME is 0700" as a given. It is not a portable
invariant: ``ensure_hermes_home()`` chmods it 0700 via ``_secure_dir()``,
but that is skipped in managed mode (the NixOS module sets 2770 on
HERMES_HOME and sessions/ so the hermes group can share state) and is
overridable with HERMES_HOME_MODE=0701 for web-server traversal. The
snapshot's own parent, ``sessions/saved``, is created by a bare mkdir at
umask default (0755) in every case. That makes the file's own mode the
part that holds across deployments, not a weaker argument for it.
No production behavior change: the four file-mode fixes are untouched.
…e= wiring
Three defects a full-suite verification pass found in this PR's own work, none
of which the suite could catch.
1. `moa-traces/` revoked NixOS group access. `mode=0o700` was passed to
`base.mkdir` unconditionally, with no managed-mode carve-out.
`nix/nixosModules.nix` pre-creates only stateDir, .hermes, cron, sessions,
logs, memories, plugins (2770 — setgid, group-rwx) via systemd.tmpfiles;
`moa-traces` is NOT among them, so on a managed host it is created lazily
here and that `mode` was the only thing setting it. The module runs the
gateway with `UMask = "0007"` specifically so "files created by the gateway
should be group-writable so interactive users in the hermes group can
read/write them", and avoids `chown -R` to keep setgid alive "for group
access by hostUsers" — who get a `~/.hermes` symlink to that same stateDir.
Gateway and interactive CLI share one $HERMES_HOME, so a 0700 dir created by
whichever ran first locked the other out with EACCES: the tracing feature
itself, not just its permissions. `ensure_hermes_home` already branches on
`is_managed()` at its own creation site, and its `logs/curator` lazy mkdir is
the direct precedent. Measured on the real path, fresh dir, parent 2770,
umask 0007:
merge base 0o770 (group-rwx) before this 0o700 after this 0o770
Non-managed behavior is unchanged: umask 022 and umask 077 both stay 0700,
and a pre-existing managed 0750 dir is still left alone. `HERMES_HOME_MODE`
is deliberately not honored — that hatch exists so a web server can
*traverse* HERMES_HOME to reach a served subdir, and nothing is served from
`moa-traces`.
2. `mode=0o600` in `save_conversation` was untested. Stripping it left 2514
files / 23624 tests green. `mkstemp` creates at 0600 and passing `mode`
forces `original_mode = None` so `_restore_file_mode` returns early — on a
fresh file the argument does nothing, and `/save`'s only test wrote a fresh
file. What it buys is the *overwrite* path, where `atomic_json_write`
otherwise preserves the existing mode. That path is reachable: the snapshot
name has second resolution, so two `/save` calls in the same second collide,
and a 0644 snapshot from pre-fix Hermes would keep 0644 while receiving a
fresh full transcript. Tested at both levels — the helper (covering every
caller that passes a mode, including a 0640 case `mkstemp` cannot produce,
so only a real chmod reaches it) and the `/save` wiring itself with a frozen
timestamp. Also names `failed_trajectories.jsonl`, hardened by the same line
as `trajectory_samples.jsonl` but previously asserted only by implication.
3. Two comments failed measurement. "mode applies to dirs this call creates" is
false for intermediates: `pathlib.mkdir(parents=True, mode=…)` applies the
mode to the final component only, so a nested `moa.trace_dir` override
leaves intermediates at 0755 (measured). No security impact — the leaf stays
0700 and the JSONL 0600 — but the guarantee was overstated. And in `cli.py`,
2770 is HERMES_HOME's managed mode (0750 is `stateDir/home`, a different
path), `HERMES_HOME_MODE` does not apply in managed mode at all since
`_secure_dir` returns early, and `saved_dir` is not 0755 "either way": it
measures 0755 by default and 0770 under the managed unit's UMask.
Content stays deliberately unredacted (NousResearch#43083): masking a credential in a
replayed path poisons the replay.
8 -> 14 tests. Every mechanism proven non-vacuous by reverting it and
confirming a diagnostic failure: unconditional `mode=0o700` fails the managed
test, dropping `mode=` entirely fails the unmanaged one, removing
`mode=0o600` from `/save` fails only the new overwrite test (the pre-existing
fresh-file test stays green, which is the reported gap), replacing
`open_private_append` with a bare `open` fails the trajectory, failed-trajectory
and moa-trace tests, and disabling both chmod calls in `atomic_json_write`
fails the 0640 assertion.
Verified: ruff 0.16.1 clean on all four files, check-windows-footguns and
check_subprocess_stdin clean, 9-file helper regression set 67/67, and a 68-file
sweep of moa/save_conversation/managed-mode tests at 1397 passed / 1 failed —
that one (Linux-only systemd restart) reproducing identically at the untouched
merge base.
What does this PR do?
/savesnapshots, agent trajectories, and MoA traces each persist verbatim conversation content — message text, tool results, tool-call arguments — through a bareopen()/json.dump(). Permissions therefore came from the process umask, i.e.0o644on a default install.Trajectories are the sharpest case:
save_trajectory()appends to the CWD, not to the0o700HERMES_HOME. A run inside a checkout drops a world-readable full transcript next to the source, and neither filename was gitignored.Scoped to file modes only. Content is deliberately not redacted:
tests/agent/test_tool_call_arg_no_redaction.py.moa.privacy_filter: display.Related Issue
Refs #77472 (partial — the file-mode items only; the "exact-value redaction on every persistence path" ask is deliberately not implemented, for the reasons above)
Type of Change
Changes Made
utils.py— newopen_private_append()for append-mode artifacts. Only the creating open applies the mode: a file the user deliberately relaxed (to share, or to feed a training pipeline) keeps its permissions. Mode is advisory on Windows, where at-rest protection is ACL-based.cli.py(save_conversation) — write the snapshot viaatomic_json_write(..., mode=0o600). This also closes a truncated-file window: the oldjson.dumpwrote in place, so a serialization failure mid-write left a partial snapshot behind. Now the temp file is discarded and the same(x_x) Failed to save:message is printed.agent/trajectory.py— createtrajectory_samples.jsonl/failed_trajectories.jsonlowner-only.agent/moa_trace.py— create the trace JSONL owner-only andmoa-traces/as0o700, except on a managed (NixOS) install, where the mode is omitted so the module's configured setgid + umask decide. See "Managed-mode carve-out" below..gitignore— ignore both trajectory filenames.tests/test_transcript_artifact_file_modes.py— new.How to Test
scripts/run_tests.sh tests/test_transcript_artifact_file_modes.py tests/hermes_cli/test_atomic_json_write.py— 14 tests (was 8: 5 + 3). They assert the contract (no group/other bits on a freshly created artifact) rather than freezing an octal, run under a deliberately permissiveumask 0o022, and exercise the real write paths so a regression to umask-derived modes fails here.Regression check on the paths touched:
scripts/run_tests.sh tests/cli/test_save_conversation_location.py tests/agent/test_moa_trace_streamed_capture.py tests/test_trajectory_compressor.py tests/test_trajectory_compressor_async.py— 33 passed.Shared-helper blast radius:
scripts/run_tests.sh tests/hermes_cli/test_atomic_json_write.py tests/test_atomic_replace_symlinks.py tests/hermes_cli/test_atomic_yaml_write.py tests/test_stale_utils_module_import.py tests/test_utils_truthy_values.py— 22 passed. Plusscripts/run_tests.sh -k moa(2999 files) — exit 0.Manual, real config gate (not monkeypatched) — temp
HERMES_HOME,moa.save_traces: true,umask 022:Manual
/save: snapshot lands at0o600; with non-serializable content it prints(x_x) Failed to save: Object of type Unserializable is not JSON serializableand leaves no partial or.tmpfile behind.Review follow-up (second commit): the
/savetest no longer purgessys.modulesbefore importingcli. The filter wasm.startswith("cli"),which also matched
clickand its 11 submodules and broke class identity forthe rest of the suite. It was unnecessary — the autouse
_hermetic_environmentfixture (
tests/conftest.py:441) already redirectsHERMES_HOME,get_hermes_home()re-reads the env var on every call with no caching, andsave_conversationresolves it at call time. Teeth check after the edit:restoring the pre-fix
open(path, "w") + json.dumpmakestest_save_conversation_snapshot_owner_onlyfail withhermes_conversation_*.json is group/other-accessible (0o644).Pre-existing unrelated failure, present on clean
mainbefore this branch and unchanged by it:tests/agent/test_compression_concurrent_fork.py::test_fence_cancelled_compression_leaves_lock_reacquirable.Managed-mode carve-out, and the
mode=wiring gap (third commit)A full-suite verification pass (2514 files / 23624 passed on this branch at
merge base
3572d4bca, 2026-08-01; 39 failures, all 39 reproducingidentically at that same merge base) found three defects in this PR's own
work. None of them were catchable by the suite as it stood.
upstream/mainhas since advanced past that base — 2551 raw
tests/files at3572d4bcaversus 2630 today — so absolute counts from a re-run will not match these.
1.
moa-traces/was revoking NixOS group access.mode=0o700was passedto
base.mkdirunconditionally, with no managed-mode carve-out.nix/nixosModules.nixpre-creates only stateDir,.hermes, cron, sessions,logs, memories, plugins (2770 — setgid, group-rwx) via
systemd.tmpfiles;moa-tracesis not among them, so on a managed host it is created lazilyhere and that
modewas the only thing setting it. The same module runs thegateway with
UMask = "0007"specifically so "files created by the gatewayshould be group-writable so interactive users in the hermes group can
read/write them", and avoids
chown -Rto keep the setgid bit alive "for groupaccess by hostUsers" — who get a
~/.hermessymlink to that same stateDir.Gateway and interactive CLI therefore share one
$HERMES_HOME, and a0700dir created by whichever ran first locked the other out with
EACCES— thetracing feature itself, not just its permissions.
ensure_hermes_homealready branches onis_managed()at its own creationsite, and its
logs/curatorlazy mkdir deliberately lets the configured umaskdecide inside an already-secured parent. This follows that precedent at the
creation site rather than only at reconciliation.
Measured on the real
save_moa_turnpath under a tempHERMES_HOME:umask 0220o7550o7000o700umask 0770o7000o7000o700umask 0007, parent 2770, dir absent0o7700o7000o77007500o7500o7500o750HERMES_HOME_MODE=0701(unmanaged)0o7550o7000o700The trace JSONL is
0o600in every row after the fix.HERMES_HOME_MODEisdeliberately not honored: that hatch exists so a web server can traverse
HERMES_HOMEto reach a served subdirectory, and nothing is served out ofmoa-traces.2.
mode=0o600insave_conversationwas untested. Stripping it left2514 files / 23624 tests green.
mkstempcreates at0600and passingmodeforces
original_mode = Noneso_restore_file_modereturns early — so on afresh file the argument does nothing, and
/save's only test wrote a freshfile:
mode=mode=0o6000o6000o600(identical — no-op)0o6440o6440o600(the only case it changes)What the argument buys is the overwrite path, where
atomic_json_writeotherwise preserves the existing mode. That path is reachable: the snapshot
name has second resolution (
hermes_conversation_%Y%m%d_%H%M%S.json), so two/savecalls in the same second land on the same file, and a0644snapshotleft by a pre-fix Hermes would keep
0644while receiving a fresh fulltranscript. Now tested at both levels — on the helper (covering every caller
that passes a mode, including a
0o640casemkstempcannot produce, so onlya real chmod reaches it) and on the
/savewiring itself with a frozentimestamp. Also names
failed_trajectories.jsonl(thecompleted=Falsebranch), hardened by the same line as
trajectory_samples.jsonlbut previouslyasserted only by implication.
3. Two comments failed measurement. "mode applies to dirs this call
creates" is false for intermediates:
pathlib.mkdir(parents=True, mode=…)applies the mode to the final component only, so a nested
moa.trace_diroverride leaves intermediates at
0o755(measured onshared/audit/moa: leaf0o700,audit/andshared/0o755). No security impact — the leaf stays0o700and the JSONL0o600, so traversal into the traces is still blocked —but the guarantee was overstated. In
cli.py,2770is HERMES_HOME's managedmode (
0750applies to${stateDir}/home, a different path),HERMES_HOME_MODEdoes not apply in managed mode at all since
_secure_dirreturns early, andsaved_diris not0755"either way" — it measures0o755by default and0o770under the managed unit'sUMask. Both load-bearing conclusions stand;only the parenthetical octals were wrong.
sessions/savedis still0o755and is deliberately left for a follow-up —it is a directory-mode issue, not a file mode, and out of this PR's scope.
Teeth, per mechanism. Each was reverted individually and confirmed to fail
with a diagnostic naming the cause, then restored byte-identically (checksums
verified):
mode=0o700restoredmode=dropped entirelymode=0o600removed from/save0o644); the pre-existing fresh-file test stays green — the reported gap, reproducedopen_private_append→ bareopenatomic_json_write0o640assertion fails ("just mkstemp's 0o600 default surviving")The managed test reproduces all three real conditions together
(
HERMES_MANAGED=nixos, parent2770,umask 0007, dir absent) and stubsnothing: a real
config.yamlopens the gate, realload_config()reads it, andreal
_traces_enabled_and_dir()derives the path. Setting only the env varwould pass for the wrong reason, since a default
umask 022yields0755andsatisfies "group access survived" by ambient umask rather than by the carve-out.
_managed_install()is a deliberate local helper:hermes_cli.config'ssecure_mkdir(#77655) centralizes exactly this branch, but it is not onmainyet, so importing it would couple this PR's merge to that one. Consolidate when
it lands.
Verified: ruff 0.16.1 clean on all four changed files,
check-windows-footguns.py --diff upstream/mainandcheck_subprocess_stdin.pyclean, 9-file helper regression set 67/67, and a 68-file sweep of
moa /
save_conversation/ managed-mode tests at 1397 passed / 1 failed — thatone (Linux-only systemd restart) reproducing identically at the untouched merge
base.
Precedent for this exact pattern
This isn't a new convention —
gateway/shutdown_flush.pyalready writes asecret-bearing artifact the same way:
flush_dir.mkdir(parents=True, exist_ok=True, mode=0o700)(72024950c, "fix(gateway): harden shutdown message flush")atomic_json_write(final_path, payload, mode=0o600, default=str)(720cdd1d1, "refactor: use atomic_json_write instead of hand-rolled _write_payload")Same two primitives, same modes, same reasoning: pending gateway messages are
verbatim user content, so the directory is created
0o700and each payload0o600.atomic_json_write'smode=parameter exists for precisely this("avoiding chmod-after-write TOCTOU exposure for secret-bearing files" — its
own docstring). This PR applies the established pattern to three artifact
paths that were missed, plus one append-mode helper for the JSONL cases
atomic_json_writecan't cover.Why this isn't a reversal of #74897
#74897 moved
write_file's new files off a hardcoded0600and ontoumask-derived
0644(then #74918 hardened the arithmetic intochmod "=rw").Both are already in this branch's base, so the direction looks opposite. It
isn't the same class of file:
write_file)HERMES_HOME)0600write_fileis a general-purpose file writer; forcing0600there brokecross-process readers, which is exactly why #74897 was right. These four
paths are agent-generated secret-bearing artifacts —
save_trajectory()appends a full transcript to the CWD under a fixed filename the user never
named. No cross-process reader is implied by that, and there was no interop
contract to break.
The two changes also agree on the invariant #74897 actually protected:
never override permissions the user set. #74897's regression guard is
"overwrite 0755 file → preserved".
open_private_append()applies its modeonly on the creating
os.open, so an existing file is never re-tightened —asserted directly by
test_trajectory_existing_relaxed_file_is_not_retightened(writes the file,chmods it
0644, appends, asserts still0644). A user who widens atrajectory to feed a training pipeline keeps that. The
/savesnapshot is afresh timestamped filename every time, so it has no pre-existing mode to
preserve.
Narrow reading: this PR only changes what mode a file is born with on paths
where the agent picked the name.
Checklist
Code
scripts/run_tests.shand they pass (see above)Documentation & Housekeeping
cli-config.yaml.example— N/A (no config keys added; this is not env/config surface)CONTRIBUTING.md/AGENTS.md— N/Aos.openhonours only the read-only bit), so the tests areskipif(os.name != "posix")and the helper's docstring says so. Windows at-rest protection is ACL-based and is deliberately left to the sibling PR fix(security): enforce owner-only ACLs on Windows in _secure_file #77527 (enforce owner-only ACLs on Windows in _secure_file) rather than duplicated here — one Windows ACL implementation, in the shared helper, not two. No behavior change on any platform beyond the permission bits.