fix(trajectory): keep trajectory JSONL out of git-tracked working directories - #78408
fix(trajectory): keep trajectory JSONL out of git-tracked working directories#78408ZHJay wants to merge 4 commits into
Conversation
…ectories save_trajectory() opens a bare relative filename, so it resolves against the CWD the agent was launched in. Called from finalize_turn, an agent run started inside a source checkout appended a full verbatim transcript — message text, tool results and tool-call arguments — next to the user's code as an untracked trajectory_samples.jsonl, one `git add -A` away from being published. Resolve a relative filename through resolve_trajectory_path() first: when the resolved target sits inside a git work tree, write under <HERMES_HOME>/trajectories/ instead and warn once with the destination. Nothing is dropped or truncated — trajectories are training data and stay full-fidelity; only the destination changes, to the private directory the rest of Hermes' state already uses. CWD placement stays supported three ways: an absolute filename is honoured as-is, agent.trajectory_allow_git_cwd: true restores the old behaviour globally, and a CWD outside any work tree is untouched (the common datagen shape). `.git` is probed with exists() so a linked worktree or submodule, where it is a file, is covered too. run_agent.py's --save_sample builds the same payload from _convert_to_trajectory_format under a relative sample_<uuid>.json, so it goes through the same helper. Refs NousResearch#77472
alt-glitch
added
type/security
…rect Review of the original guard found six should-fix gaps. All confirmed with reproductions on the previous head and each fixed with a behaviour-contract test that fails without the change. 1. The guard failed OPEN into the repo. `_find_git_root`'s `try` covered only `start.resolve()`, not the `.exists()` walk. `Path.exists()` swallows ENOENT/ENOTDIR but not EACCES, and `resolve()` raises `RuntimeError` (not `OSError`) on a symlink loop, so both escaped into the broad handler, which returned the original filename — writing the transcript into the checkout, logged only at debug. Measured: an unreadable ancestor left `?? locked/`; a read-only HERMES_HOME left `?? trajectory_samples.jsonl`. `resolve_trajectory_path` now returns `Optional[str]`, where `None` means "do not write", and every unknown-containment path takes it. `os.getcwd()` is included because it raises EACCES under an unreadable ancestor while a relative `open()` still succeeds against the CWD file descriptor — so the write really would land in the repo. A skip is a returned decision, never a raise: `save_trajectory` runs during turn finalization. 2. The destination had no post-condition. With HERMES_HOME inside a work tree (or `git init ~`), `git add -A` would stage `.hermes/trajectories/trajectory_samples.jsonl` — the warning's promise was false. `trajectories/` now carries a self-ignoring `.gitignore` (`*`, which ignores itself, so no tracked file is added), and the notice names the containing checkout and the remedy. Refusing to write there instead would have broken trajectory saving for every dotfiles user. 3. The notice never reached a terminal. `setup_logging` installs no stderr StreamHandler unless `--verbose`, so `logger.warning` only reached errors.log. Each distinct notice now also prints one line to stderr, once per process, deduped as before. `AIAgent(save_trajectories=True)` — the path datagen uses — now names the destination instead of printing a bare "Trajectory saving enabled". 4. An existing pipeline went stale in silence. A pre-existing ./trajectory_samples.jsonl kept its plausible content and simply stopped growing. A distinct notice now names both paths and all three remedies. The user's file is not moved or modified and nothing is created in their checkout — leaving files in checkouts is what this guard is for — so the signal is out-of-band: terminal, errors.log, and the docs. 5. Two repos merged into one dataset, unrecoverably: the entry schema carries no repo/session field, so after the merge only `model` distinguishes them. The path is now keyed per work tree (`<basename>-<sha256[:8]>`, following `agent/moa_trace.py`'s per-session naming), restoring the pre-fix one-dataset-per-repo shape. Keying the path rather than adding a provenance field keeps a documented, pipeline-consumed format unchanged and keeps repo paths out of the dataset. The old comment claiming subdirectory preservation prevented collisions was false and is gone. 6. Docs contradicted shipped behaviour. Updated developer-guide/trajectory-format.md and guides/python-library.md plus both zh-Hans mirrors, including the `load_trajectories(...)` / `data_files=...` snippets a datagen user copies. Also: honour `GIT_DIR`/`GIT_WORK_TREE` (verified with git — `GIT_DIR` alone makes the CWD the work tree, so a CI runner or wrapper got no protection); normalize interior `..` so `a/../b/keep.jsonl` keeps `b/` instead of silently flattening to the basename; stop claiming "the git work tree at X" when a stray `.git` is not a real repo; drop a change-detector `inspect.getsource` assert; and correct the config-default rationale — `get_missing_config_fields()` calls `load_config()`, which deep-merges `DEFAULT_CONFIG` first, so a declared key is never reported missing (measured: 0). Refs NousResearch#77472
|
An independent adversarial review of this PR found six should-fix problems, each Point by point. 1. The guard failed open into the repo.
Two things I found while fixing it that the reproduction made visible:
2. The redirect target was never checked. With I did not make this refuse to write: that would break trajectory saving for Worth flagging precisely, because the reviewer's script still prints "STILL 3. The notice never reached a terminal. Correct, and this was the one I was Each distinct notice now also prints one line to stderr, once per process 4. An existing pipeline going stale — agreed this was the worst of the six. On the tension you flagged: the most discoverable place for a pointer is next I also considered dropping a marker inside 5. Cross-repo merge — you were right that the comment was false. Two repos Reasoning, since you asked for it explicitly: the JSONL is a documented format Basename plus digest because basename alone would silently merge 6. Docs. Updated 7. Nits — all taken:
Windows. Called out in the PR body now rather than implied away: the Re-verified after the changes — everything the review confirmed as already
Numbers. 54 tests in the file, all passing. Teeth check against the previous Thanks for the depth here — items 1 and 4 in particular were the difference |
…silently The resolver fails closed and tells the user where the transcript went, but the append it authorizes had no such treatment: `open(..., "a")` failures went to `logger.warning` only, and `hermes_logging.setup_logging` installs no stderr handler without `--verbose` (root handlers are a queue handler feeding rotating files). Measured on a real setup_logging: a chmod 0400 destination lost the turn with stdout and stderr both empty — the same silent drop the guard refuses to allow one line earlier. Route the failure through the same notice mechanism, naming the path and the reason. Split the print out of `_notify_once` so this path logs EVERY occurrence — errors.log is how a user learns how many turns were lost — while the terminal shows one line per destination, so a datagen run saving every turn is not flooded. Tests drive the real save path: a lost turn appears on stderr with its path, three failures produce three log records and one terminal line, and a successful save prints no failure notice.
…n prose
The doc guards were change-detectors, which AGENTS.md rejects outright, and
the worst of them asserted the *absence* of two literal sentences:
assert "Trajectories are written to files ... directory:" not in text
assert "轨迹写入当前工作目录下的文件：" not in text
Two things were wrong with that. A faithful reword of either page failed the
test with zero behavior change — verified: renaming the heading to "Migrating
a pipeline you already run" and rephrasing the destination paragraph, tokens
intact, fails the old form on the upgrade-path assertion and passes the new
one. And the guard could not run on the diff that would break it:
$ printf 'website/docs/developer-guide/trajectory-format.md\n' \
| python3 scripts/ci/classify_changes.py
python=false
``website/`` is in ``_PY_SKIP``, so a docs-only PR skips the whole Python
lane, goes green, and lands red on main (where the classifier fails open).
Freezing prose bought nothing in the one direction it claimed to cover.
Replaced with one assertion derived from the code at runtime: write a real
trajectory in a real git work tree, read the destination directory and the
default dataset filename *off the resolved path*, and require all four pages
(EN + zh-Hans) to name them. It can now only fail when code and docs actually
disagree — rename ``trajectories/`` or the default filename and the pages
fail until updated; reword the surrounding prose and nothing fails. Derived
via a HERMES_HOME-wide glob rather than the ``_landed`` helper, which knows
the literal name ``trajectories`` and would have made the derivation
circular. Matching is delimiter-aware, not substring: mutating the default to
``samples.jsonl`` still matched the documented ``trajectory_samples.jsonl``
under a bare ``in``, so that assertion had been passing against code that no
longer wrote the file it named.
``test_reference_docs_name_the_opt_out`` is kept as-is — a config key is an
identifier the reader looks up verbatim, not prose.
Also collapsed the duplicated default-config literal:
assert DEFAULT_CONFIG["agent"]["trajectory_allow_git_cwd"] is False
assert trajectory._allow_git_cwd() is False
into the invariant plus one direction statement — the reader must resolve to
whatever DEFAULT_CONFIG declares (pins the two readers to each other; ``is``
so a truthy non-bool fails), and separately, the secure default is False with
a message saying why. The security default genuinely is the contract here, so
the meaning is preserved while the literal is stated once. Added the
``hermes_home`` fixture so the assertion reads a config-less home rather than
whatever the ambient one holds, and asserted that premise.
Tests only; no production change. 57 -> 55 tests (two deleted parametrizations
minus the replacement's four), all passing.
What does this PR do?
agent/trajectory.py'ssave_trajectory()opens a bare relative filename, soit resolves against whatever CWD the agent was launched in:
It is called from
finalize_turn(agent/turn_finalizer.py:250→run_agent.py_save_trajectory→agent/trajectory.py), so an agent run started inside asource checkout appends a full verbatim transcript — message text, tool
results and tool-call arguments — next to the user's code as an untracked
trajectory_samples.jsonl, onegit add -Aaway from being published.This PR resolves a relative filename through a new
resolve_trajectory_path()first. When the resolved target sits inside a git work tree, the write goes
under
<HERMES_HOME>/trajectories/<work-tree>/instead, and a one-time noticenames the destination on the terminal.
Redirect, not refusal — deliberately. Trajectories are training data and
exist to be full-fidelity, so refusing to write them by default would silently
break legitimate datagen runs inside a checkout. Nothing here is dropped,
truncated or redacted; only the destination changes, to the private directory
the rest of Hermes' state already uses.
agent/moa_trace.py— the siblingwriter in the same issue cluster, carrying the same class of data — already
defaults under
<HERMES_HOME>/moa-traces/, so this makes trajectory theconsistent case rather than the outlier.
A user who wants trajectories in the CWD keeps three supported paths:
filename=agent.trajectory_allow_git_cwd: true/tmp, a datagen box — already the common datagen shape)The knob is a
config.yamlbehavioural setting with a documented default inhermes_cli/config_defaults.py, perAGENTS.md. No newHERMES_*env var.One dataset per work tree — not one flat shared file
The pre-fix path was CWD-relative, so
projA/andprojB/each accumulatedtheir own dataset. A single flat file under
trajectories/would have mergedthem irreversibly: the entry schema carries no
cwd/repo/session field, soafter the merge the only discriminator is
model, which is usually identical.The relocated path is therefore keyed per work tree:
Basename so the directory is recognisable, digest so
~/a/projand~/b/projcannot land in the same place. This follows
agent/moa_trace.py:128, which keysits trace file per session.
Why key the path instead of adding a provenance field to the entry. The
JSONL is a documented format consumed by training pipelines
(
website/docs/developer-guide/trajectory-format.md), so widening the schemahas a much larger blast radius than changing a directory name — and a
cwd/repo field would write local filesystem paths into the dataset, which isa privacy regression in a PR whose whole purpose is not leaking local context.
Path keying also restores exactly the shape users already had. The entry schema
is unchanged and asserted so by a test.
Failing closed
If containment cannot be established — unreadable ancestor, symlink loop,
unwritable or full
HERMES_HOME—resolve_trajectory_path()returnsNoneand the save is skipped, reported on the terminal and in
errors.log. Itnever writes into the checkout as a fallback, which was the pre-fix behaviour.
Skipping is chosen over falling back to a scratch location because a transcript
in
/tmpis data the OS deletes and no pipeline reads — a silent loss dressedup as a save. A skip is a returned decision, never an exception:
save_trajectoryis a side effect of turn finalization, and turning a failedtrace into a raise would trade a leak for a broken turn.
Scope boundary vs our other open PRs
This PR is about where the file is written. It does not change file modes:
trajectory_samples.jsonl/failed_trajectories.jsonl0600and adds both to.gitignore. That protects this repo's checkout andthe file's permissions; it does nothing for a user's own project where
Hermes actually runs. Both PRs edit
agent/trajectory.py: fix(security): create plaintext transcript artifacts owner-only #77520 rewrites theopen()call inside thetry, this one assignsfilenameabove it.resolve_trajectory_pathruns beforeopen_private_append, so therelocated file still lands
0600when both are in — composition verified.Adjacent, not conflicting — whichever lands second may need a trivial rebase.
batch_runner.py. Disjoint files from this PR.cli-config.yaml.exampleandhermes_cli/config_defaults.py. Checked: no textual collision. In theexample its hunk is
@@ -727,6 +727,23 @@ session_reset:, mine is@@ -823,0 +824,15 @@ agent:; inconfig_defaults.pyit appends inside thesessionsblock, mine insideagent. Either merge order applies cleanly.Directory mode, for the record:
mkdir(parents=True, exist_ok=True)createstrajectories/at0755inside a0700HERMES_HOME, so the parent stillgates access and (with #77520) the file itself is
0600.agent/moa_trace.py:127does the same, so this is consistent with existingcode. Deliberately not tightened here — it would collide with
#77520/#77655's directory-mode work.
Upgrading an existing pipeline (please read if you collect trajectories)
If you already have a
./trajectory_samples.jsonlin a checkout, it is leftexactly as it is and stops receiving new entries. That is the nastiest
possible failure mode — worse than an empty dataset, because the file still
exists with plausible content and simply stops growing while a pipeline keeps
reading it. So Hermes now prints a distinct one-time notice naming both paths
and the three ways out (repoint the pipeline, concatenate, or set the opt-out),
logs it to
errors.log, and documents it in the trajectory-format guide.Your file is not moved or modified — it is your data — and nothing new is
created in your checkout, because not leaving files in checkouts is the entire
point of this change. The signal is deliberately out-of-band for that reason.
Bug class: what I included and what I left out
Included
run_agent.py --save_sample. It builds its payload from the same_convert_to_trajectory_formatunder a relativesample_<uuid>.jsonandopen(...,"w")s it into the CWD — the same leak, so it routes through the samehelper.
Deliberately left out
batch_runner.py'sPath("data") / run_name. It isCWD-relative too, but it is not the same bug: it is the primary declared
output of an explicitly-invoked CLI tool, the path is printed on startup
(
Output directory: …), and--resumere-derives it to globbatch_*.jsonland rebuild the completed-prompt set (
batch_runner.py:745). It also passessave_trajectories=False(batch_runner.py:331), so it never routes throughsave_trajectoryat all. Relocating it would silently orphan every in-progressrun — a "fix" that breaks the feature it protects. Happy to revisit batch output
placement as its own change if maintainers want it.
Related Issue
Refs #77472
Type of Change
Changes Made
agent/trajectory.py—resolve_trajectory_path()(returnsOptional[str];None= do not write),_find_git_root()(mirrorsagent/prompt_builder.py'shelper;
.exists()so a linked worktree/submodule.gitfile counts;raises
_GitRootUndeterminedwhen the walk can't complete),_explicit_work_tree()(GIT_DIR/GIT_WORK_TREE),_work_tree_key(),_contained_relative(),_shield_trajectories_dir(),_warn_pre_existing_dataset(),_notify_once(),describe_trajectory_destination(), and_allow_git_cwd()(lazy read-onlyconfig read, per the
moa_traceprecedent).agent/agent_init.py— thesave_trajectoriesstatus line now names thedestination instead of a bare "Trajectory saving enabled".
run_agent.py—--save_samplerouted through the same helper and handles askipped destination;
--save_trajectoriesbanner updated.hermes_cli/config_defaults.py—agent.trajectory_allow_git_cwd: False.cli-config.yaml.example— documents the key and the upgrade path.website/docs/developer-guide/trajectory-format.md,website/docs/guides/python-library.mdand bothzh-Hansmirrors — thedocs said trajectories are written to the CWD and the copy-paste snippets
(
load_trajectories(...),data_files=...) pointed at that path.tests/agent/test_trajectory_git_cwd_guard.py— 54 tests.How to Test
Measured before → after with a frozen timestamp so the comparison is exact,
same throwaway git repo, real
save_trajectory()in a subprocess fromrepo/srcwithHERMES_HOMEpointed at a temp dir:Identical sha256 on both sides is the point: the transcript is unchanged, it
just isn't in your repo any more. (Payload includes CJK, escaped quotes, a
backslash, a tab and a 5 KB tool result.)
Reproduce by hand:
Automated:
./scripts/run_tests.sh tests/agent/test_trajectory_git_cwd_guard.py # 54 passedTests exercise the real
save_trajectory()→open()path against a realgit initrepo and a tempHERMES_HOME— nothing on the write path is mocked.Exposure is asserted with
git status --porcelainandgit add -An, since afile physically inside a work tree but git-ignored is not actually committable.
Teeth check — the same tests against the previous head of this branch
(
4cf34daf3): 31 failed, 21 passed, so every behaviour added in this roundis pinned. Against unmodified
upstream/mainthe module doesn't import at all.Per finding: fail-closed 6, destination post-condition 4, terminal notice 4,
staleness 1, per-work-tree keying 6 (+2 shared), docs 8.
Mutation testing — 12 mutants, all caught: identity resolver (32 failed),
_find_git_root→None(31), inverted opt-out (35), restore fail-open (4),swallow EACCES in the walk (1), skip the destination
.gitignore(2), log-onlynotice (2), drop the staleness notice (1), flat shared path (5), basename-only
key (1), ignore
GIT_DIR/GIT_WORK_TREE(2), re-flatten interior..(1).Blast radius — the same 86 test files as before: 2220 passed, 2 failed
(was 2183/2; the +37 is this round's new tests). Both failures
(
tests/tools/test_wake_word.py::test_openwakeword_ensures_base_models_for_custom_path,tests/tools/test_web_providers.py::…test_web_extract_tool_runs_discovery_before_registry_lookup)reproduce identically on a clean
upstream/mainworktree — pre-existing,unrelated. Plus all 26 test files that reference trajectory saving: 496 passed,
0 failed.
Concurrency — two processes appending simultaneously to one dataset (same
repo), 40 entries × 40 KB and 15 × 1 MB, plus the two-repo case: zero corrupt
lines, zero lost entries, exact per-model counts in every configuration.
Checklist
Code
./scripts/run_tests.sh, CI parity) and they passDocumentation & Housekeeping
guides and both
zh-Hansmirrors, docstrings, banners)cli-config.yaml.examplefor the new config keyCross-platform
pathlibonly (Path.resolve(),.parents,.exists(),.parts) plusos.getcwd()/os.path.normpath— no POSIX-only calls, nochmod, no shell outon the write path (
gitis only invoked inside the tests, which follow 12existing test files that do the same).
get_hermes_home()already resolves%LOCALAPPDATA%\hermeson Windows, so the redirect target is correct there. Thework-tree key is restricted to
[A-Za-z0-9._-]and stripped of leading/trailingdots and spaces, so it is a valid directory name on Windows too.
scripts/check-windows-footguns.pyis clean on all changed files. The.git-as-a-file case matters on every platform (linked worktrees, submodules).Windows append atomicity — untested, stated honestly. The no-corruption
result above rests on POSIX
O_APPEND, where eachwrite()to a file opened"a"is atomic with respect to the file offset. Windows emulates append withseek-then-write, so concurrent appends to a shared file are a materially
different risk profile there, and I have not tested it — I'm on macOS and cannot.
Two notes on scope:
save_trajectoryhas alwaysappended to a single shared path; the pre-fix path was shared per CWD, this
one is shared per work tree.
different projects no longer contend for one file at all. What remains is two
processes in the same repo, which was already the pre-existing shape.
If a maintainer wants belt-and-braces on Windows, an
msvcrt.locking()/portalockerwrapper around the append would be the fix, but that is a separatechange to the write call (which #77520 is already rewriting) and I'd rather not
land untested platform-specific locking inside a placement fix.
Behaviour verified on macOS 15 arm64 / Python 3.11; the logic is
platform-independent and should hold on Linux and WSL2 — I have not run the
suite on Windows or WSL2.