Repository navigation
fix(agent): normalize the workspace pin key to the resolved path - #124787
lihongyuan99 wants to merge 10 commits into
Conversation
_workspace_pin_key() compared the probe directory as a raw string, but the two builds it is meant to unify spell the same directory differently: a launch-dir build keys on os.getcwd() — which reports the physical path (/private/var/... on macOS) — while a build that binds the cwd explicitly (TUI /compress, gateway session cwd) keys on the session's spelling of the same directory (/var/... through the symlink). The spelling mismatch misses the pin, so the rebuild re-probes git live and rewrites the session-start snapshot mid-session, changing the system prompt bytes and invalidating the cached prompt prefix for a workspace that never changed. On Linux /tmp is a real directory, so the suite stayed green there and the class hid. Normalize the pin key (and _seed_workspace_pin's persisted-cwd comparison) with Path.resolve(), the same normalization the persisted-block matcher already applies. On symlink-free paths resolve() is the identity, so Linux behavior is unchanged. This also turns the three macOS failures in TestWorkspaceSnapshotPinnedAcrossCompaction green: the two '- Root:' assertions now expect the git-resolved root (git reports toplevels resolved), and the replay/binding tests pass through the normalized pin. Red on macOS main, green with the fix; unchanged on Linux.
alt-glitch
added
type/bug
The three red-on-macOS tests only fire where the temp dir sits behind a symlink (/var -> /private/var), so Linux CI never saw the spelling class and cannot catch a regression of the resolved-path pin key. Build the same shape explicitly: chdir into the physical repo for the launch-dir build, then bind the cwd through a symlink for the rebuild; the pin must replay. Red with main's system_prompt.py (the rebuild re-probes and reports "Status: 1 untracked"), green with the normalized key. Skipped on Windows, where directory symlinks need privileges.
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
PR Review — #124787
Verdict: correct fix at the right layer; no blocking findings. Reviewed head 1088868, then pushed one review-fix commit (e1b2ed3, test only) so the class is guarded on Linux CI too. Merge decision stays with the maintainer.
Premise (verified on origin/main, 0 behind): agent/system_prompt.py::_workspace_pin_key returns str(resolve_context_cwd() or resolve_agent_cwd()) raw. A launch-dir build keys on os.getcwd() (physical path); a later build with the cwd bound (_SESSION_CWD / TERMINAL_CWD) keys on the caller's spelling. _coding_parts compares pinned[0] == cwd_key as strings, so a symlink spelling misses the pin, re-probes git live and re-pins new bytes mid-session — the cached-prefix break 921ab7a1631 set out to close. _persisted_workspace_block already resolves both sides, so this PR only brings the key and _seed_workspace_pin's cwd comparison to the same normalization. The - Root: assertion change is right: _workspace_roots reports the git toplevel, which git rev-parse --show-toplevel returns resolved on every OS.
Not macOS-only: any Linux host whose project dir is reached through a symlink (cd ~/proj → /data/proj; $PWD keeps the link spelling, getcwd() does not) hits the same class. That is why the added test builds the shape with an explicit symlink instead of relying on /var.
Live repro (in-process, real git repo, real build_system_prompt):
- main source + new
test_symlink_spelling_of_the_launch_dir_replays_the_pin→ FAIL (Status: 1 untrackedre-probed vs pinnedStatus: clean); the pre-existingtest_binding_the_launch_dir_explicitly_replays_the_pinstays green on Linux, confirming CI could not see the class before. - PR head → both pass; 10/10 in the file, 133/133 across
test_compaction_prompt_rebuild,test_system_prompt,test_system_prompt_restore,test_system_prompt_switch_invalidation,test_coding_context.
Behaviour checks: key == "" (deleted cwd) with a stored cwd still returns early, same as main; a stored cwd from another host resolves non-strict and simply mismatches, as before; resolve() is the identity on symlink-free paths so Linux/Windows bytes are unchanged; _workspace_pin_key is only called inside _coding_parts' try/except Exception, so a pathological resolve() failure degrades the same way main's _persisted_workspace_block already does.
Suggestions (non-blocking):
_workspace_pin_keydocstring, first sentence: it is no longer literally "the prompt's Current working directory" (that line keeps the raw spelling). Consider "the resolved form of …" so the two paragraphs agree.- Dupe sweep: no sibling PR touches the pin key (
workspace pin,_frozen_workspace_snapshot,Path.resolve+system_prompt); nothing to consolidate.
Looks good: minimal (11 LOC of source), reuses the normalization the persisted-block matcher already applies, no new helper, no defensive wrapping.
jonpol01
left a comment
jonpol01
left a comment
There was a problem hiding this comment.
Normalizing agent/system_prompt.py::_workspace_pin_key (and the persisted-cwd check in _seed_workspace_pin) with Path.resolve() is the right layer: _persisted_workspace_block already resolves both sides, and _coding_parts compares pinned[0] == cwd_key as raw strings, so a launch-dir key (os.getcwd(), physical) and a bound key (symlink spelling) missed the pin. Verified on macOS arm64, Python 3.14: scripts/run_tests.sh -j 2 tests/agent/test_compaction_prompt_rebuild.py on main 7b4211eee2 fails 3/9; with only the head's test file on main source it fails 2/10 (test_binding_the_launch_dir_explicitly_replays_the_pin, test_symlink_spelling_of_the_launch_dir_replays_the_pin), so the two - Root: assertion edits are test corrections and those two tests are the red-on-base proof. On head e1b2ed35db, scripts/run_tests.sh -j 2 tests/agent/test_compaction_prompt_rebuild.py tests/agent/test_system_prompt.py tests/agent/test_system_prompt_restore.py tests/agent/test_system_prompt_switch_invalidation.py tests/agent/test_coding_context.py tests/agent/test_prompt_builder.py gives 187 passed, 1 skipped. Preflight: no other open PR touches _workspace_pin_key, _seed_workspace_pin or tests/agent/test_compaction_prompt_rebuild.py; no shared-file conflict.
- [nonblocking] The same spelling comparison remains at the runtime-cwd sink, so the prompt bytes still change across a symlink spelling:
agent/conversation_loop.py:877-878(_stored_prompt_matches_runtime) andagent/prompt_builder.py:1001(_local_host_hints), both unchanged by this PR. Risk/repro: in-process probe on head with the realbuild_environment_hints(not patched), repo under/var/tmp. Firstbuild_system_promptruns from the launch dir. Then an untracked file is added,invalidate_system_promptruns,set_session_cwd(<symlink to the repo>)binds the cwd and the prompt is rebuilt. On main the first differing byte is 4878 of 6781 (the workspace block,Status: cleanvsStatus: 1 untracked). On head the workspace block is equal and the first differing byte is 6407 of 6781, theCurrent working directory:host line (/private/var/...vs/var/...)._stored_prompt_matches_runtime(agent, p1)returns False on both. So the PR moves the break from the context tier to the volatile tail, which is a real gain. But the byte-equality keep path atagent/conversation_compression.py:3183still takes the drift branch. A resumed or adopted session (conversation_loop.py:744,conversation_compression.py:1701) also still fully rebuilds a stored prompt whose cwd differs only in spelling. Suggested fix: compare resolved paths in_stored_prompt_matches_runtime(Path(stored_cwd).resolve() != resolve_agent_cwd().resolve()) so a stored prompt survives a spelling-only change; the host line can keep the session's spelling. Please confirm whether that belongs in this PR or a follow-up. - [nonblocking] The required gate
All required checks passis red only becauseCheck contributors / check-attributionfails. The author email on1088868has nocontributors/emails/mapping (the job prints "New contributor email(s) without a mapping"). Suggested fix: add the one-line mapping withpython3 scripts/add_contributor.py <commit email> lihongyuan99, as the job's output suggests.
Minor
agent/system_prompt.py:661:keyis already resolved by_workspace_pin_key, soPath(key).resolve()repeats a filesystem walk on every build. ComparingPath(stored_cwd).resolve()withPath(key)gives the same result.- PR body, "How to Test" item 1, still says 9/9; after
e1b2ed3the file has 10 tests.
| try: | ||
| return str(resolve_context_cwd() or resolve_agent_cwd()) | ||
| raw = str(resolve_context_cwd() or resolve_agent_cwd()) | ||
| return str(Path(raw).resolve()) if raw else "" |
There was a problem hiding this comment.
[nonblocking] The same spelling comparison remains at the runtime-cwd sink. agent/conversation_loop.py:877-878 (_stored_prompt_matches_runtime) still compares the stored Current working directory to str(resolve_agent_cwd()) raw, and agent/prompt_builder.py:1001 renders that line in the session's spelling.
Probe on this head with real environment hints: build from the launch dir, add an untracked file, then set_session_cwd(<symlink to the repo>) and rebuild. The workspace block now replays, and the first differing byte is 6407 of 6781, the host Current working directory: line. On main it was 4878, the workspace block. _stored_prompt_matches_runtime(agent, p1) is False on both, so a resumed or adopted session still fully rebuilds across a spelling-only change.
Suggested fix in _stored_prompt_matches_runtime: if stored_cwd and Path(stored_cwd).resolve() != resolve_agent_cwd().resolve(): return False. Please confirm whether that belongs here or in a follow-up.
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Review at e1b2ed35dbb038392d11b9010f874f3a57b395c6
The workspace-snapshot fix is at the right seam. _workspace_pin_key now identifies the physical directory for both a launch-dir build and a later explicitly bound spelling; _seed_workspace_pin applies the same equivalence when adopting a persisted snapshot. The unchanged coding-context renderer then receives the pinned block rather than probing Git again. I found no P0 regression in those changed source paths. There is one P2 test-coverage gap at the new platform skip (inline): Windows junctions reproduce this exact spelling class without privileged directory symlinks, yet the new regression test skips Windows entirely.
Independent behavior check (native Windows, Python 3.11, real temporary Git repository and build_system_prompt): I built from the physical directory, added an untracked file, invalidated the prompt, then bound the same directory through a sibling mklink /J junction using set_session_cwd. With environment hints held constant to isolate the workspace snapshot, the two functions from base 28e6496a5e3adfea57bebfc9571b981bff378523 re-probed (Status: 1 untracked); the PR head replayed Status: clean and the prompt was byte-equal. A fresh agent adopting the persisted prompt gave the same base-fails/head-passes result. This is a native Windows counterpart to the existing POSIX regression test, not a simulated platform string. The existing different-directory test also passed, so this normalization has not been shown to conflate distinct workspaces.
Scope of the cache claim: With real environment hints instead of the held-constant test seam, the head still changes the later Current working directory: line when the junction spelling is bound; the workspace snapshot remains clean. This corroborates jonpol01's existing inline report, which already locates the unchanged runtime matcher and the remaining full-prompt mismatch. I am not filing it again as a new PR-introduced defect. This PR repairs the early workspace-block drift; it does not establish byte equality of the entire prompt across spelling changes.
Verification / merge state: scripts/run_tests.sh tests/agent/test_compaction_prompt_rebuild.py -j 1 returned 9 passed, 1 skipped on native Windows; the four selected cwd/context checks in test_system_prompt.py passed. A broader local six-file run produced 109 passed, seven failed and four skipped, plus a 300-second timeout for the remainder of test_system_prompt.py. The seven failing cases were rerun at the exact base and failed identically: the required scratch location is below a pre-existing home-level .git, defeating those tests' “outside a repo” assumption. I do not claim a green local six-file suite. Head CI's Python test job is green, but required contributor attribution and the aggregate gate are red; jonpol01's earlier review already identified the missing author-email mapping. No duplicate attribution finding here.
| from pathlib import Path | ||
| from agent.system_prompt import build_system_prompt, invalidate_system_prompt | ||
|
|
||
| if sys.platform == "win32": |
There was a problem hiding this comment.
[P2] Cover the Windows directory-alias case instead of skipping this regression. This skip removes the newly introduced test from native Windows CI, but the same launch-dir/explicit-cwd mismatch is reachable without privileged symlinks: a sibling NTFS junction created with cmd.exe /c mklink /J <temp>\link <temp>\repo is an alternate spelling of the physical repo, and Path.resolve() collapses it. In a native Windows/Python 3.11 probe using a real temporary Git repo and the production set_session_cwd, I built from the physical directory, added an untracked file, invalidated the prompt, then bound the junction. At the exact base (28e6496) the rebuilt workspace block became Status: 1 untracked; at this head (e1b2ed3) it stayed Status: clean and, with the runtime hints held constant, the whole prompt remained byte-equal. The same base/head result held for a fresh agent adopting a persisted prompt. Yet the new test reports skipped on Windows (focused file: 9 passed, 1 skipped), so a Windows-specific regression in either normalization path would go undetected there. Please use a disposable junction on Windows (remove the junction before removing its target) and the symlink on POSIX, then assert replay for both the live and persisted-prompt paths; retain a distinct-directory negative control so path normalization cannot hide a real workspace switch. This is a coverage gap, not a claim that this PR currently fails on Windows.
ehz0ah
left a comment
ehz0ah
left a comment
There was a problem hiding this comment.
Reviewed exact head e1b2ed35dbb038392d11b9010f874f3a57b395c6. The resolved-path change fixes the symlink spelling mismatch in both the live pin and stored-prompt adoption paths. The committed focused suite passed 10/10, and a separate persisted-prompt symlink probe also passed. I found one additional case-insensitive-filesystem gap, described inline. I did not repeat the existing runtime-cwd, Windows junction, or contributor-attribution comments.
| try: | ||
| return str(resolve_context_cwd() or resolve_agent_cwd()) | ||
| raw = str(resolve_context_cwd() or resolve_agent_cwd()) | ||
| return str(Path(raw).resolve()) if raw else "" |
There was a problem hiding this comment.
[P2] Compare existing directories by filesystem identity. Path.resolve() follows symlinks, but on default case-insensitive macOS APFS it preserves the casing supplied by the caller. As a result, .../MixedCase and .../mixedcase refer to the same directory (os.path.samefile() returns true) but still produce different keys here. _coding_parts() then misses the pin and probes Git again.
I reproduced this on the exact head through the real set_session_cwd() path: build in MixedCase, add an untracked file, invalidate the prompt, then bind mixedcase. The rebuilt workspace snapshot changed from Status: clean to Status: 1 untracked. That is the same mid-session snapshot drift this change is meant to prevent.
Please compare live paths with Path.samefile() or an equivalent filesystem-identity check, with a safe fallback when a persisted path is no longer available. Apply the same equivalence in _seed_workspace_pin(), and add a macOS case-alias regression through the real session-cwd resolver.
Path.resolve() follows symlinks but preserves the caller's casing, and the default macOS APFS filesystem is case-insensitive: the same directory bound as .../MixedCase and .../mixedcase yields two resolved strings, so the workspace pin missed, the rebuild re-probed git live, and the session-start snapshot was rewritten mid-session — the exact drift the resolved-path normalization was added to prevent (review of e1b2ed3 by ehz0ah, reproduced through the real set_session_cwd path). Compare live paths by filesystem identity (Path.samefile) at the three pin seams — the replay decision, the persisted-prompt adoption gate, and the persisted-block root match — with resolved-string equality as the fallback when a path no longer exists. A pin key of "" (no workspace) still only equals itself.
|
Follow-up commit 9fef6b1 addresses the review threads: @ehz0ah's case-insensitive-filesystem gap — fixed. @jonpol01's runtime-cwd sink ( @andrexibiza's Windows junction coverage — agreed, follow-up. Writing the junction-based test without a Windows host to run it on would be an unverified change to a test the maintainer authored; the skip deserves a real junction probe on native CI instead. Full |
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Re-review at f6871b36 — the delta after e1b2ed35
All four earlier reviews anchor at e1b2ed35. Two commits landed after them: 9fef6b1 (filesystem-identity comparison at the pin seams, plus the case-alias regression) and f6871b36 (the attribution map). This review covers that delta and reconciles every earlier item.
Verdict: no blocking findings. One P2 CI-coverage finding (inline). Merge decision remains with the maintainer.
Verified at this head
_same_live_dir / _same_pin_key (agent/system_prompt.py:612-630) now govern all three pin seams: the replay decision in _coding_parts (line 712), the persisted-prompt adoption gate in _seed_workspace_pin (line 682), and the root/ancestor match in _persisted_workspace_block (line 645). The "" guard in _same_pin_key correctly keeps Path("") → . from ever matching a real cwd.
Independent receipts — native Windows 11, Python 3.11.11, real temporary git repos, production set_session_cwd, workspace-relevant hints kept equivalent across each base/head pair; base 28e6496 vs this head:
- junction live-pin (
mklink /Jalias of the same physical repo): base re-probes (Status: 1 untracked, prompt differs), head replays byte-equal (Status: clean); - junction adoption (fresh agent adopting a persisted prompt — the resume/moved-repo shape that reaches
_seed_workspace_pin+_persisted_workspace_block): base re-probes, head replays; - case-alias live (same repo rebound under a differently-cased spelling): base re-probes, head replays — the fix holds on NTFS as well as APFS.
Focused file at this head: 9 passed, 2 skipped — both skips are host guards (Windows). Boundary: the full tests/agent/ suite was not re-run here.
Attribution: contributors/emails/982026408@qq.com → lihongyuan99 is present at f6871b36; replaying contributor-check.yml's exact logic over the author emails since the merge base leaves no unmapped, non-auto-skipped address, so that gate has nothing left to fail on at this head.
P2 — the new case-alias regression cannot execute on any CI lane
The test added to guard ehz0ah's case-insensitive-filesystem finding is discriminating only where the filesystem is case-insensitive and case-preserving, and no runner imports or reaches it: the macOS lane imports only files carrying a mark.platforms(...) spec (this file has none — selector receipt: 242 files listed, this one absent); Linux self-skips (case-sensitive); Windows skips by guard. Mechanism, receipts and the two-line repair are inline at line 285. The code fix for ehz0ah's finding is real and verified; this concerns its regression protection.
Prior items — reconciliation
- teknium1 (review) — docstring first-sentence suggestion: unaddressed; non-blocking, no objection. Dup sweep: still clear for the pin-key path.
- jonpol01 #1 — runtime-cwd sink (inline): still present at this head; reproduced — with the repo rebound through a spelling alias,
_stored_prompt_matches_runtimereturnsFalse, so a resumed/adopted session still fully rebuilds on a spelling-only change. The author deferred it to a follow-up; not re-filed. - jonpol01 #2 — attribution: repaired at
f6871b36(receipt above); no CI receipt exists at this head yet. - jonpol01 minors — the extra
Path(key).resolve()walk: unaddressed, acceptable now thatsamefileneeds concrete paths; the PR body's "9/9" count: stale (the file now has 11 tests) — body edit only. - Windows-junction coverage (inline): still skipped at line 263. Fresh receipt: the junction shape is base-red/head-green on a native Windows host; a
platforms("windows")-marked variant is the mechanism if the Windows lane should exercise it — that covers the broader spelling class, distinct from the macOS case-preserving quirk. - ehz0ah — case-insensitive gap (inline): fixed at
9fef6b1; verified at all three seams. Its regression is the P2 above.
CI and merge state at this head
- 0 check-runs at
f6871b36; the three workflow runs areaction_required(fork approval pending) — nothing has executed at this head yet. Ate1b2ed35, the only failed checks wereCheck contributors / check-attributionand the aggregateAll required checks pass;f6871b36repairs the mapping those runs flagged. mergeable: true;git merge-treeagainst currentorigin/mainis clean (merge base28e6496).
Related work
- #125391 (same author, test-only): composes cleanly with this branch — different lines in the same file, a clean
merge-tree, and the two patches make the same- Root:assertion agree in the resolved spelling from opposite ends (fixture root vs assertion); the attribution blob is byte-identical across the two branches.
| os.chdir(old_cwd) | ||
| shutil.rmtree(tmp, ignore_errors=True) | ||
|
|
||
| def test_case_alias_spelling_of_the_launch_dir_replays_the_pin(self): |
There was a problem hiding this comment.
[P2] This regression cannot execute on any CI lane.
The scenario is discriminating only where the filesystem is case-insensitive and case-preserving (macOS APFS: resolve() follows the symlink but keeps the caller's casing) — exactly where _same_live_dir's samefile identity matters. No runner reaches the test:
- macOS lane (
.github/workflows/tests-os.yml) imports only the files emitted byscripts/ci/list_os_marked_tests.py macos, then selects-m "platforms and not integration". This file contains nomark.platforms(...)call, so the selector never lists it — at the current head the list has 242 files and this one is not among them. - Linux lane (
tests.yml, full suite):alias.samefile(repo)raisesFileNotFoundErroron a case-sensitive filesystem → caught →skipTest. - Windows: the guard two lines below skips, and the marked-files Windows lane would not import an unmarked file anyway.
Consequence: the regression added to guard the macOS finding stays green on every lane without ever running — if _same_live_dir regresses, CI keeps passing and macOS users silently get the mid-session prompt rewrite back. This is the exact footgun the repository's own guidance names: a bare host skip on an unmarked file runs on no host at all.
Evidence at this head (native Windows 11, Python 3.11.11): the focused file is 9 passed, 2 skipped; running this exact test body with only the host guard bypassed shows it executes and passes (NTFS is case-insensitive, so the alias resolves here) — the assertions are sound, the gating is the defect.
Repair: use the repository's OS-gating marker instead of a bare host skip — add import pytest and @pytest.mark.platforms("macos") to this test; the macOS lane then imports the file (verified: a copy with the marker injected is listed by the selector) and the conftest host gate skips it everywhere else. The win32 skip becomes redundant, and its reason string is imprecise — NTFS is case-insensitive; what this test needs is a case-insensitive and case-preserving filesystem. Keep the alias.samefile(repo) self-guard as the case-sensitive-volume fallback.
Regression check: python scripts/ci/list_os_marked_tests.py macos lists this file, and the macOS lane run shows this test executed, not skipped.
The samefile self-guard alone meant the test executed on no CI lane: the
macOS lane only imports files the platforms-marker selector lists, Linux
skipped via the self-guard on its case-sensitive volume, and the win32
guard skipped the rest — so a _same_live_dir regression would keep every
job green. Mark it platforms("macos") so the selector ships the file to
the lane that has the case-insensitive-and-case-preserving filesystem the
scenario discriminates on; the self-guard stays as the fallback for a
case-sensitive macOS volume. The win32 guard was redundant under the
host gate and mis-named the requirement (NTFS is case-insensitive too);
dropped.
|
Follow-up commit closes the CI-coverage P2: the test now carries |
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Re-review at 6eb71ac — the one commit after every prior review, plus full reconciliation
All five earlier reviews anchor at e1b2ed35 (teknium1, jonpol01, Axl, ehz0ah) or f6871b36 (Axl). 6eb71ac (2026-09-30) landed after every single review — the platforms("macos") marker repair for the last P2 — and has no review coverage. This review binds that delta and reconciles every prior item at the live head.
Verdict: no blocking findings. One new P2 (inline at agent/system_prompt.py:682, full mechanism there). Merge decision remains with the maintainer.
The unreviewed delta, f6871b36..6eb71ac
import pytest + @pytest.mark.platforms("macos") on the case-alias regression + removal of the bare win32 skip (and its now-unused sys import). Static receipts at this exact tree:
python scripts/ci/list_os_marked_tests.py macosliststests/agent/test_compaction_prompt_rebuild.py(243 files emitted, this one in);linux(368) andwindows(171) do not list it — the static precondition of the prior P2 (#4135198282) is closed at this head, and thesamefileself-guard survives as the case-sensitive-volume fallback as designed.
Native behavioral receipts (Windows 11, Python 3.11.11, isolated HERMES_HOME/HERMES_RUNTIME_DIR/temp dirs, real temporary git init repos, production build_system_prompt)
- Class
TestWorkspaceSnapshotPinnedAcrossCompactionat this head: 6 passed, 2 skipped (the two skips are the host gates: the Windows symlink guard and the macOS marker). Same class at merge base28e6496: 6 passed, 0 failed — green at base, green at head; nothing in the class regressed through the delta. - True union with #125391's head (
70d6032) materialized at tree9d77512e, committed as72219f38, full test file executed there: 9 passed, 2 skipped. The two patches make the same- Root:assertions agree in the resolved spelling from opposite ends (fixture root vs assertion),merge-treestays clean, and the attribution blob is byte-identical across the branches — composes cleanly in either landing order. - One earlier probe run failed 1 test while the worktree sat inside the real Hermes home (
cache\scratch\...); re-run in a neutral isolated home, the identical class is fully green at this head and at base. That single failure was the repo's ownhome_io_guardrejecting the probe runner's real-home I/O (the guard working as designed), not this PR's code; the isolated receipts above are the load-bearing ones.
New P2 — resolve() on persisted prompt text carries an uncontained raise family
The finding is inline at agent/system_prompt.py:682: _same_live_dir's except OSError does not cover the RuntimeError that Path.resolve() raises on a symlink loop, so a looped spelling arriving from persisted prompt data (the Current working directory hint or the parsed - Root: line — session-DB bytes, not live getcwd) escapes _seed_workspace_pin into _coding_parts' blanket except Exception: pass, silently dropping the entire coding-context block for that build. Base could not reach this at this seam (raw string comparison there); the raise site is reachable through 9fef6b1's normalization of persisted bytes. Sibling containment worth covering in the same change: _workspace_pin_key's except OSError: # deleted cwd at line 608 (its comment now under-describes the raise families the resolve() it wraps can emit) and _persisted_workspace_block's two resolves at lines ~618/621.
Reconciliation — every prior item at this head
- teknium1 — docstring first-sentence suggestion: unaddressed.
_workspace_pin_key's docstring still opens claiming it is the prompt'sCurrent working directorywhile the body now returns the resolved form. Cosmetic, non-blocking. - jonpol01 #1 — runtime-cwd sink (
_stored_prompt_matches_runtime): still present at this head. The matcher compares the persistedCurrent working directoryhint tostr(resolve_agent_cwd())raw, so a resumed/adopted session still fully rebuilds on a spelling-only change. Hunk-level diff inspection of the three open PRs touching that file (#74129, #79197, #102192) shows none touches this comparison — the follow-up remains un-owned; carrying it there is the right call, not a reason to block this PR. - jonpol01 #2 — attribution gate: repaired by
f6871b36(thecontributors/emails/982026408@qq.com → lihongyuan99map is present at the live head, and6eb71ac's committer identity is the resolved noreply form). Zero check-runs have ever executed at this head — the fork's runs sit inaction_requiredpending maintainer approval — so the earlier-redcheck contributors / check-attributionjob has never re-run on a head carrying its repair. Static replay of the job's logic over author emails since the merge base leaves nothing unmapped; the dynamic receipt arrives with approval, like every other check on this PR. - jonpol01 minors: the extra
Path(key).resolve()walk at the adoption seam is still present and now justified (concrete paths are required forsamefile; it runs once per seed) — but it is exactly where the new P2 lands. The PR body "How to Test" item 1 count is also now stale (the file carries 9 tests with the marker; neither "9/9" nor "10/10" describes the Windows-host run: 9 passed + 2 skipped for the class). Body edit only, non-blocking. - andrexibiza (#4117761202) — Windows junction-alias coverage: still skipped on Windows at this head, unchanged scope choice since
f6871b36. The junction shape is base-red/head-green on a native Windows probe (pre-change), and aplatforms("windows")-marked twin remains the known mechanism if the Windows lane should exercise the junction spelling. Non-blocking. - ehz0ah — case-insensitive-filesystem gap: fixed at
9fef6b1(filesystem-identity comparison at all three pin seams: the replay decision, the persisted-prompt adoption gate, the root/ancestor match), behaviorally verified in the previous round. Unchanged by the delta. - andrexibiza (#4135198282) — the case-alias regression can't execute on any lane: repaired by the unreviewed delta itself — receipts above (selector lists the file on the macOS lane only; the marked-file lane then imports it). Residual, non-blocking: the same case-alias class exists on NTFS, where the file remains selector-absent even after this marker (macOS-only net gain); the junction-or-marker twin stays the outstanding mechanism if Windows coverage is wanted.
Merge and CI state at this head
mergeable: true, mergeable_state: blocked — required checks not yet run at this head (fork approval pending); that has been true of the last three heads, so the samefile fix, the attribution map and the marker commit have no CI receipts at all yet. git merge-tree against live origin/main (f42f579) is clean. Main's movement under the touched file since the merge base (async-handoff guidance, profile-line placement into the volatile tier, _context_files_part override plumbing) does not reach the pin seams this PR owns — main's _workspace_pin_key still returns the raw string, so this fix remains additive, not a conflict.
Related work
- #125391 (same author, test-only): union receipt at tree
9d77512e— 9 passed, 2 skipped, clean merge-tree, byte-identical attribution blob. No FILE-LIST collision; either landing order composes.
Review at exact head 6eb71ac8fc829092d68fda836fcc0100518148ef; probes run with isolated HERMES_HOME/HERMES_RUNTIME_DIR; selector receipts from scripts/ci/list_os_marked_tests.py at this tree.
| # Same spelling normalization as _workspace_pin_key: the persisted hints carry the | ||
| # cwd as that surface spelled it, which can be a symlink — or, on a case-insensitive | ||
| # filesystem, a case-alias — spelling of the same resolved workspace. | ||
| if not key or not _same_live_dir(Path(stored_cwd).resolve(), Path(key).resolve()): |
There was a problem hiding this comment.
[P2] resolve() on persisted prompt text raises a second failure family this seam does not contain — RuntimeError (symlink loop), not OSError.
stored_cwd (the persisted Current working directory hint) and the parsed - Root: line are session-DB data. Path(...).resolve() follows symlinks; on a self-referential alias it raises RuntimeError (verified on this repo's floor Python 3.11.11, native Windows: Path(x).resolve() on x -> x raises RuntimeError, which is not an OSError subclass).
Consequence at this seam: _same_live_dir's except OSError -> False never sees a loop, so the docstring's promised fallback ("a missing path … falls back to the string comparison") does not cover it — the exception escapes _seed_workspace_pin into _coding_parts' blanket except Exception: pass, which silently drops the entire coding-context block (workspace snapshot, git state, posture) for that build instead of only missing this pin. Under the per-conversation prompt-caching invariant that is a full prefix rebuild for a workspace that never changed — the exact drift class this PR exists to prevent, reached through persisted bytes that 9fef6b1 taught this seam to resolve().
Reachability: merge base 28e6496 could not hit this at this seam (raw string comparison there); the raise site is introduced by this PR's 9fef6b1. Siblings in the same class at this head, worth covering in the same change: _workspace_pin_key's except OSError: # deleted cwd (pre-existing containment whose comment now under-counts the raise families of the resolve() it wraps) and _persisted_workspace_block's two resolves over persisted bytes.
Repair (smallest complete): normalize the two arguments inside _same_live_dir — try: a, b = a.resolve(), b.resolve() / except (OSError, RuntimeError): return a == b before the equality fast-path, keeping the documented string-comparison fallback as the catch-all — or widen both excepts here and at _workspace_pin_key to (OSError, RuntimeError). Regression contract: a test seeding a pin from a persisted prompt whose Current working directory text is a self-symlink alias asserts _seed_workspace_pin leaves the pin open without raising, and _same_pin_key("", x) still only equals itself; gate it with the platforms marker this head just added so the lane that discriminates actually imports the file.
A symlink loop reaching the pin seams from persisted session bytes (the
Current working directory hint or a parsed - Root: line — session-DB
data, not a live getcwd) makes Path.resolve raise RuntimeError on
Python <= 3.12, which the OSError handlers did not cover: it escaped
_seed_workspace_pin/_persisted_workspace_block into _coding_parts'
blanket handler and silently dropped the whole coding block for that
build. Verified on 3.11: _persisted_workspace_block('…- Root:
<self-loop>…') escaped 'Symlink loop from …' before, returns None now.
_same_live_dir takes raw spellings and runs resolve() itself under
(OSError, RuntimeError) — one containment for all three seams (the
callers pass persisted text straight through). _workspace_pin_key's
handler names both raise families. _same_pin_key's empty-key contract
is unchanged. Docstring first sentence no longer claims the key IS the
prompt's cwd line (teknium1 review note) — it is the resolved form of
the directory that line spells.
|
Follow-up commit a52ee4c closes the symlink-loop P2 and the docstring note. Containment, one seam for all three call sites. Receipts on a real 3.11 interpreter (uv cpython 3.11, project installed):
teknium1's docstring note: the first sentence no longer claims the key is the prompt's Reconciled as follow-ups (unchanged): jonpol01's |
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Re-review at a52ee4cb2c96e2f114cc6bd13ffc83efb9433ba6
One new P2 correctness finding, inline: a pathname that identified the captured workspace can be rebound before the next build, and the new equality helper then replays that workspace's snapshot in a different directory. This is separate from the earlier symlink-loop exception report. The latest containment change addresses that report; the remaining problem is a false cache hit, not an exception or an ordinary spelling-only cache miss.
P2 — current filesystem equality does not prove snapshot provenance
After a snapshot is captured in physical directory A, rename it to A-saved, replace A with a symlink to independent repository B, and explicitly bind B for the next build. The captured and current key strings differ, but _same_live_dir resolves both using the new directory layout. _coding_parts consequently forwards A's frozen block instead of requesting B's snapshot. The same re-identification occurs when _seed_workspace_pin adopts persisted bytes; an initially empty live pin can also suppress B's workspace block entirely. The inline finding contains the source chain, failing assertion and repair requirements.
The decisive local observations were:
| Scenario | Base 28e6496 |
Head a52ee4c |
|---|---|---|
| Stable symlink alias of the same directory after a Git mutation | Re-probes; original defect | Replays; intended fix |
| Ordinary switch between two untouched directories | Probes B | Probes B |
| Captured physical A is replaced by an alias to B; bind B | Probes B | Replays A |
| Fresh agent reads A's persisted snapshot after that replacement | Probes B | Adopts/replays A |
| Initially empty live pin; old path is rebound to coding repository B | Emits B's snapshot | Retains the empty snapshot |
Evidence boundary: these are isolated executions of the inspected base/head pin-function bodies, the cwd resolver and legacy host-line parser, using real temporary directories and Git repositories on Linux / Python 3.13.5. The downstream renderer was a controlled test double implementing the inspected workspace_block is None probe/replay contract; the recorded observation is which bytes the pin owner hands to it. The standalone identity assertion uses the real filesystem, not mocked Path.resolve/samefile. This is not a full installed-Hermes, full-prompt, repository-suite, native macOS or native Windows test result. The consumer's corresponding production branch is RuntimeMode.system_prompt_parts.
Earlier findings reconciled
The case-alias finding is addressed by the new filesystem comparison at the live and persisted seams; native case-insensitive-filesystem execution was not repeated here. The macOS selection finding is addressed in source by @pytest.mark.platforms("macos") at test line 288, which matches the inspected OS selector; this is not a claim that the head's macOS job executed.
The loop-containment finding is addressed at the changed seams. Local controls rejected a real loop in persisted cwd/root spellings without dropping the controlled coding block. A separately injected RuntimeError verified the older-interpreter exception handler; that injection is not a native Python 3.11 run. The contributor-email mapping is present in the patch, so the old missing-mapping report is not being repeated.
Jonpol01's runtime-cwd comparison remains a separate, previously acknowledged follow-up: conversation_loop.py still compares the stored spelling directly. The Windows junction coverage request also remains the author's acknowledged follow-up; the existing alias test still skips Windows. Neither is presented as a new finding.
Head and CI checks
The sampled current main, b2860025, still has the original raw-key comparisons at these seams; this patch has not been superseded there. For the reviewed head, CI, Nix and Docker returned action_required. Those are not green test receipts, and older-head results do not establish this head's verification. The final metadata refresh still identifies a52ee4c as the head and now reports mergeable: false; merge readiness is not established.
| symlink loop raises the latter on Python <= 3.12, and letting it escape the pin seams | ||
| would drop the whole coding block to the blanket handler.""" | ||
| try: | ||
| pa, pb = Path(a).resolve(), Path(b).resolve() |
There was a problem hiding this comment.
[P2] Do not use a later filesystem mapping as proof of the captured workspace's identity.
This helper correctly compares two current path spellings, but its callers also pass a historical path stored with a snapshot. The tuple retains the resolved pathname, not the directory identity at capture time. If physical directory A is renamed to A-saved, A is replaced with a directory symlink to an independent repository B, and the same session is then explicitly bound to B, both arguments resolve to B here. _same_pin_key(old_A_key, new_B_key) returns True even though the original directory still exists at A-saved and has a different filesystem identity from B.
The consequence follows through the changed caller: _coding_parts supplies A's frozen workspace block as workspace_block, so the consumer bypasses its fresh probe. The prompt can retain A's root/commit information after the explicit switch to B. If A's live pin was empty, B's workspace snapshot stays absent. This is not the intended retention of old Git status inside an unchanged workspace: the captured directory and current directory are distinct.
The persisted sibling has the same cause. _seed_workspace_pin and _persisted_workspace_block reinterpret both the historical cwd and historical physical - Root: against the replacement symlink, so a fresh agent can adopt A's bytes for B too. At base 28e6496, the unequal cwd strings reject this adoption and unequal live keys request a fresh snapshot.
Minimal failing identity assertion, runnable from an installed checkout on a POSIX host:
from pathlib import Path
from tempfile import TemporaryDirectory
from agent.system_prompt import _same_pin_key
with TemporaryDirectory() as directory:
root = Path(directory).resolve()
old, new = root / "old", root / "new"
old.mkdir()
new.mkdir()
captured_key = str(old.resolve())
original_id = (old.stat().st_dev, old.stat().st_ino)
old.rename(root / "old-saved")
old.symlink_to(new, target_is_directory=True)
current_key = str(new.resolve())
assert original_id != (new.stat().st_dev, new.stat().st_ino)
assert captured_key != current_key
assert not _same_pin_key(captured_key, current_key) # fails: TrueObserved scope: executing the inspected head's exact comparison body on real Linux directories/Python 3.13.5 fails that final assertion. A separate paired base/head owner-boundary probe used the inspected cwd resolver, real Git repositories, and a controlled downstream renderer: base requested B's new snapshot; head forwarded A's old bytes, for both live replay and persisted adoption. No full-package or native macOS/Windows run is claimed. Stable symlink aliases of the unchanged directory still replay correctly at head, and an ordinary untouched A-to-B switch re-probes on both revisions.
Repair: preserve capture-time directory identity for live pins rather than re-resolving a historical pathname as though it were immutable. For persisted snapshots, require evidence that the recorded workspace still denotes the captured directory; when that cannot be established after a historical physical root is rebound, take a fresh snapshot rather than treating today's samefile result as provenance. Apply this at both replay and adoption, without reverting the intended stable-symlink/case-alias behavior.
Regression contract: capture through the actual session-cwd path; relocate the original directory and reuse its pathname as an alias to B; bind B and rebuild. Assert B's root/commit is emitted and A's bytes are not replayed. Repeat with a fresh agent reading persisted A bytes and with an initially empty live pin. Retain stable-alias replay, ordinary distinct-directory, empty-key, and loop-containment controls.
_same_live_dir answers "are these two spellings the same directory today", but the pin also feeds it a historical path stored with a snapshot. Renaming the captured directory and reusing its old pathname as a symlink to an independent repo makes today's resolution of the stored key land on the new directory, so the equality helper replays the old snapshot there — the consumer then skips its fresh probe and the prompt keeps A's root/commit after an explicit switch to B. The persisted seam re-identifies the same way: _seed_workspace_pin adopts A's bytes for B because the stored cwd hint and the persisted - Root: line both resolve into B through the replacement symlink, and an initially empty live pin suppresses B's snapshot entirely. Two pieces of provenance evidence close it without reverting the stable-alias and case-alias replay the earlier reviews pinned: - the live pin records (st_dev, st_ino) at capture and the replay gate requires the stored key to still stat to that identity — a rebound spelling stats to the replacement directory, a stable alias still stats to the recorded one; legacy 2-tuples keep the key-match contract - a persisted - Root: line is git rev-parse --show-toplevel output, i.e. a resolve() fixpoint at capture; when resolving it today traverses a symlink, the directory it named was moved or rebound and the bytes are not adopted Red→green: three new tests (live replay, persisted adoption, empty-pin suppression) fail at the parent head and pass with the fix; test_compaction_prompt_rebuild 15/15, prompt/compaction battery (coding_context, anti_thrash, cache policy, profile isolation) 132/132.
|
Confirmed — your minimal identity assertion fails at Fixed in
Loops stay contained: a looped Root line fails the fixpoint check ( Regression tests per your contract, all in |
There was a problem hiding this comment.
Re-review at 4a90a34b
The latest commit closes the exact symlink-rebind repro from my prior review: when the old physical root becomes a symlink, the new fixpoint gate rejects the persisted block, and live pins now carry a capture-time filesystem identity.
I kept pushing on that same provenance boundary. One P2 remains, inline: the snapshot is still not bound to a directory generation/lifetime. There are two demonstrated manifestations:
| Shape | Current-head result |
|---|---|
| Persist A at path P; remove A; create independent repo B at the exact same P; reconstruct the agent | The root is still a resolve() fixpoint, so A's block is accepted. Seeding then records B's current filesystem identity beside A's historical bytes, and B is not probed. |
Live empty pin at P; end that directory lifetime; recreate P with a recycled (st_dev, st_ino); initialize repo B |
The pathname and identity guards both pass, replay == "", and B's workspace probe is suppressed. On this Linux/Python 3.13.5 overlayfs host, the pair was recycled on the first recreation. |
Both same-path lifetime replacements were already unsafe at merge base 28e6496; I am not calling them newly introduced regressions. They remain in scope because 4a90a34b is explicitly the repair for the previous provenance finding and claims the new identity/fixpoint evidence closes it. It does not: the missing fact is which directory generation produced the snapshot, not another spelling of the pathname.
%%{init: {'theme':'dark','themeVariables':{'background':'#0d1117','primaryTextColor':'#e6edf3','lineColor':'#94a3b8'}}}%%
flowchart TD
subgraph FLOW["Workspace path replacement and snapshot reuse"]
direction TB
A["A snapshot at /workspace"] --> X{"Filesystem changes"}
X -->|old path becomes symlink to B| S["resolve(root) != root"]
S --> F["reject A; probe B"]
X -->|A removed; B created at same path| P["resolve(root) == root"]
P --> Q["accept A block"]
Q --> R["attach B's current identity to A bytes"]
R --> T["identity gate passes; skip B probe"]
click A "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/coding_context.py#L520-L535"
click X "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L648-L661"
click S "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L664-L678"
click F "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L759-L781"
click P "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L664-L678"
click Q "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L682-L703"
click R "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L723-L743"
click T "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L769-L781"
end
LEGEND["Legend<br/>Blue: entry / incoming<br/>Purple: ownership / policy<br/>Amber: transition / pending<br/>Green: success<br/>Red: failure"]
style FLOW fill:#0d1117,stroke:#30363d,color:#e6edf3
style LEGEND fill:#1e293b,stroke:#94a3b8,color:#f8fafc,font-size:11px
FLOW ~~~ LEGEND
class A blue
classDef blue fill:#172554,stroke:#60a5fa,color:#eff6ff,stroke-width:2px
class X amber
classDef amber fill:#451a03,stroke:#fbbf24,color:#fffbeb,stroke-width:2px
class S,P purple
classDef purple fill:#3b0764,stroke:#c084fc,color:#faf5ff,stroke-width:2px
class F green
classDef green fill:#052e16,stroke:#4ade80,color:#f0fdf4,stroke-width:2px
class Q,R,T red
classDef red fill:#450a0a,stroke:#f87171,color:#fef2f2,stroke-width:2px
%%{init: {'theme':'dark','themeVariables':{'background':'#0d1117','primaryTextColor':'#e6edf3','lineColor':'#94a3b8'}}}%%
flowchart TD
subgraph FLOW["Directory generation and lifetime boundary"]
direction TB
R["attach B's current identity to A bytes"]
T["identity gate passes; skip B probe"]
R --> T["identity gate passes; skip B probe"]
L["live pin: (dev, ino)"] --> I["inode pair recycled after lifetime ends"]
I --> T
M["missing: generation + lifetime"]
M -.-> R
M -.-> I
click R "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L723-L743"
click T "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L769-L781"
click L "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L648-L661"
click I "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L648-L661"
click M "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L648-L661"
end
LEGEND["Legend<br/>Amber: transition / pending<br/>Red: failure<br/>Gray: context / transient"]
style FLOW fill:#0d1117,stroke:#30363d,color:#e6edf3
style LEGEND fill:#1e293b,stroke:#94a3b8,color:#f8fafc,font-size:11px
FLOW ~~~ LEGEND
class L gray
classDef gray fill:#1e293b,stroke:#94a3b8,color:#f8fafc,stroke-width:2px
class I,M amber
classDef amber fill:#451a03,stroke:#fbbf24,color:#fffbeb,stroke-width:2px
class R,T red
classDef red fill:#450a0a,stroke:#f87171,color:#fef2f2,stroke-width:2px
Evidence confidence is explicit in the text, not encoded by color. Filesystem-transition nodes link to the pinned identity probe; their recorded execution evidence remains in the review. Implementation nodes link to the pinned source.
Diagram evidence boundary: mmdc is not installed in this runner and registry DNS is unavailable, so I am not claiming a local CLI render receipt.
Earlier case-alias, macOS-selection, loop-containment and contributor-mapping items are not repeated. The already acknowledged runtime-cwd comparison and Windows junction coverage remain separate follow-ups.
Hosted state is also unchanged in one important respect: current-head CI, Nix and Docker workflows all completed as action_required; there are no combined statuses. That is neither pass nor fail, but it is not a green receipt for 4a90a34b.
| snapshot's origin; the gate must take a fresh snapshot instead of adopting | ||
| the bytes.""" | ||
| try: | ||
| return Path(root_line).resolve() == Path(root_line) |
There was a problem hiding this comment.
[P2] The new provenance checks still do not bind the snapshot to a directory generation/lifetime.
The deterministic persisted case needs no symlink. With two real temporary Git repos on this head's helper semantics:
- Build/persist repo A at pathname
P. - Remove A and create independent repo B at the exact same
P(different HEAD). - Resume a fresh agent at
P.
Because the string is unchanged, this line still returns true: Path(P).resolve() == Path(P). _same_live_dir(stored_cwd, key) also succeeds, so _persisted_workspace_block returns A's historical block. Then _seed_workspace_pin records B's current (st_dev, st_ino) next to A's old bytes. The subsequent replay check passes by construction and the production consumer sees a non-None workspace_block, so it skips B's Git probe.
Observed: A/B root strings equal; A/B HEADs different; this fixpoint check True; old A block adopted True; seeded identity equals current B True; fresh-probe decision False. The symlink-rebind scenario is the positive control: once the old path resolves through a symlink, this check is False and the new repair rejects it correctly.
The live sibling shows why _dir_identity is also insufficient as the sole lifetime proof. On Linux/Python 3.13.5/overlayfs, I captured an empty pin for a plain directory, removed/recreated that pathname, and the same (st_dev, st_ino) was recycled on the first recreation. After git init made the new directory repo B, the key and identity checks both still passed, replay == "", and B's workspace probe was suppressed. This is a real filesystem/Git probe of the exact guard semantics, not a claim that every filesystem recycles immediately.
These same-path shapes pre-exist this PR, but they show the prior provenance repair requirement is still open. Repair: persisted adoption needs capture-time workspace-generation evidence; do not manufacture provenance at resume by pairing historical bytes with today's occupant of the path. The live pin likewise needs a lifetime-stable reference/generation, or it must conservatively re-probe when the original lifetime cannot be proven. Regression: cover A-at-P → lifetime ends → independent B-at-P for both persisted reconstruction and a live empty/non-empty pin, and assert B is freshly probed while stable symlink/case aliases of an unchanged directory still replay.
There was a problem hiding this comment.
Re-review at 4a90a34bad6f7c7b4438b9bd085ff453254a4f92
4a90a34b fixes the three top-level, after-capture cases from my previous review: live replay, persisted adoption, and empty-pin suppression now select the new repository in the paired probes. The provenance defect is not fully closed: two P2 variants remain, detailed inline at the new guards/publication sites.
F1 — persisted adoption validates the ancestor, not the workspace owner
A captured cwd beneath repository A can be rebound to an independent nested repository B while A's root remains physical. The persisted-root guard accepts A as an ancestor of B, then adoption assigns B's current inode to A's old bytes. A fresh agent replays A; an existing live agent correctly probes B under the same topology. Validate the actual nearest Git/marker-root owner before adopting the persisted snapshot.
F2 — snapshot bytes and the recorded inode can come from different directories
A rebind between producer completion and the new post-collection identity sample creates a pin containing A's bytes and B's inode. Both replay checks then pass when the next rebuild explicitly binds B. Capture and validate one directory generation across collection and pin publication; a post-collection stat alone cannot establish that pairing.
These are remaining variants of the PR's pathname-provenance problem, not a claim that the new commit introduced either from scratch. The original base rejects the different-cwd replay/adoption in both cases.
Paired behavioral evidence
| Scenario | Parent a52ee4c |
Reviewed head 4a90a34b |
|---|---|---|
| Stable alias of the unchanged directory after a Git mutation | Replays the original snapshot | Replays the original snapshot |
| Ordinary switch to a distinct repository | Probes B | Probes B |
| Top-level path rebound after capture: live pin | Replays A incorrectly | Probes B |
| Top-level path rebound after capture: fresh agent | Adopts A incorrectly | Probes B |
| Top-level empty pin rebound to repository B | Suppresses B's block | Emits B's block |
| Subdirectory rebound to a nested independent repo: fresh agent | Replays outer repo A | Still replays outer repo A — F1 |
| Rebind after producer completion, before identity sample | Replays A | Still replays A, now paired with B's inode — F2 |
Both inline minimal assertions additionally pass against original base 28e6496 and fail against this head. In the nested case the old root remains a resolve fixpoint, and a fresh producer demonstrably selects the inner repository; in the ordering case the tuple's recorded identity demonstrably equals B's, not the original A directory's.
Scope of these receipts: real temporary directories, symlinks and Git repositories on Linux/Python 3.13.5; executions of the inspected pin-owner, session-cwd and workspace-producer function bodies. The downstream posture call is a controlled adapter preserving the production probe/replay branch. Unrelated posture/project-fact configuration is fixed, Git uses a bounded local subprocess adapter, and persisted fixtures use the supported legacy host-line format. This is not a full installed-Hermes/full-prompt test, a run of the repository's focused suite, or native macOS/Windows verification. The inline snippets contain the runnable failing assertions; the broader probes also observe forwarded snapshot bytes and directory identities.
Why the two guards diverge
Each diagram embeds its semantic-color legend. Evidence confidence is stated explicitly in the paired results, not encoded by color. Each implementation node links to pinned source; controlled-interleaving nodes link to the corresponding recorded evidence.
F1: the root stays physical while the captured cwd changes owner
%%{init: {'theme':'dark','themeVariables':{'background':'#0d1117','primaryTextColor':'#e6edf3','lineColor':'#94a3b8'}}}%%
flowchart TB
subgraph CORE[" "]
direction TB
A["Capture in outer/src<br/>Snapshot owner: outer repo"]:::normal
B["Move src aside<br/>Replace src with symlink to inner repo"]:::transition
A --> B
B --> C["Fresh agent binds outer/inner<br/>Stored cwd now resolves to inner"]:::transition
C --> D["Root guard accepts outer<br/>Still physical AND an ancestor"]:::failure
D --> E["Adopt outer snapshot<br/>Attach inner's current inode"]:::failure
E --> F["FAIL: outer bytes replayed<br/>Fresh probe would select inner repo"]:::failure
B --> G["Existing live agent<br/>Recorded src inode no longer matches"]:::owner
G --> H["PASS: live replay rejected<br/>Fresh inner snapshot"]:::fixed
classDef normal fill:#172554,stroke:#60a5fa,color:#eff6ff,stroke-width:2px;
classDef owner fill:#3b0764,stroke:#c084fc,color:#faf5ff,stroke-width:2px;
classDef failure fill:#450a0a,stroke:#f87171,color:#fef2f2,stroke-width:2px;
classDef fixed fill:#052e16,stroke:#4ade80,color:#f0fdf4,stroke-width:2px;
classDef transition fill:#451a03,stroke:#fbbf24,color:#fffbeb,stroke-width:2px;
click A "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/coding_context.py#L495-L530"
click B "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L723-L737" "Source boundary under path replacement"
BE["Recorded interleaving evidence"]:::context
B -.-> BE
click BE "https://github.com/NousResearch/hermes-agent/pull/124787#discussion_r4174029734" "Original recorded evidence"
classDef context fill:#1e293b,stroke:#94a3b8,color:#f8fafc,stroke-width:2px
click C "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L723-L737"
click D "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L694-L701"
click E "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L739-L743"
click F "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L769-L781"
click G "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L769-L774"
click H "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L775-L781"
end
CORE ~~~ LEGEND
LEGEND["Legend<br/>Blue: entry / incoming<br/>Purple: ownership / policy<br/>Amber: transition / pending<br/>Green: success<br/>Red: failure<br/>Gray: context / transient"]
style CORE fill:#0d1117,stroke:#30363d,color:#e6edf3
style LEGEND fill:#1e293b,stroke:#94a3b8,color:#f8fafc,font-size:11px
Sources: persisted adoption and root matching; nearest-root workspace producer.
F2: the fingerprint is sampled after the snapshot's directory has changed
%%{init: {'theme':'dark','themeVariables':{'background':'#0d1117','primaryTextColor':'#e6edf3','lineColor':'#94a3b8'}}}%%
flowchart TB
subgraph CORE[" "]
direction TB
A["Pin owner resolves cwd A"]:::owner
A --> B["Producer finishes<br/>Git snapshot from A"]:::normal
B --> C["Controlled interleaving<br/>Move A; reuse A as symlink to B"]:::transition
C --> D["Identity sampled AFTER producer<br/>stat of A now returns B's inode"]:::failure
D --> E["Published pin<br/>A pathname + A bytes + B identity"]:::failure
E --> F["Next build explicitly binds B<br/>Key and inode checks both pass"]:::owner
F --> G["FAIL: A snapshot replayed in B"]:::failure
classDef normal fill:#172554,stroke:#60a5fa,color:#eff6ff,stroke-width:2px;
classDef owner fill:#3b0764,stroke:#c084fc,color:#faf5ff,stroke-width:2px;
classDef failure fill:#450a0a,stroke:#f87171,color:#fef2f2,stroke-width:2px;
classDef transition fill:#451a03,stroke:#fbbf24,color:#fffbeb,stroke-width:2px;
click A "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L759-L765"
click B "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L775-L778"
click C "https://github.com/NousResearch/hermes-agent/pull/124787#discussion_r4174029744"
click D "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L778-L781"
click E "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L778-L781"
click F "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L769-L774"
click G "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L769-L777"
end
CORE ~~~ LEGEND
LEGEND["Legend<br/>Blue: entry / incoming<br/>Purple: ownership / policy<br/>Amber: transition / pending<br/>Red: failure"]
style CORE fill:#0d1117,stroke:#30363d,color:#e6edf3
style LEGEND fill:#1e293b,stroke:#94a3b8,color:#f8fafc,font-size:11px
Source: collection, identity sampling and replay. The rebind edge is the explicitly controlled filesystem interleaving, not an asserted internal callback.
Earlier findings reconciled
| Earlier item | Status at this head |
|---|---|
| Pathname provenance | Original three after-capture root-level cases pass; F1 and F2 above prevent full closure. |
| Case-alias identity | Filesystem comparison remains in source. Native case-insensitive-volume execution was not repeated. |
| macOS test selection | The platforms("macos") marker remains; no claim that this head's macOS lane ran. |
| Symlink-loop containment | Containment remains; real-loop helper rejection passed on 3.13.5, as did a separately injected RuntimeError check. The latter is not a native 3.11 run. Empty-key equality also passed. |
| jonpol01's runtime-cwd comparison | Still the separate acknowledged follow-up: the matcher compares raw spellings. It does not prevent F1 during the subsequent rebuild. |
| Windows junction coverage | Still the acknowledged follow-up; the original alias test and new rebinding tests skip Windows. |
| Contributor-email mapping | Present in the patch; the historical missing-mapping finding is not repeated. |
Verification state
This head's CI, Nix and Docker runs return action_required, not green verification. That state is not a test-failure diagnosis, and older-head or author-reported suite results do not establish this head's CI. The latest metadata refresh still pins 4a90a34b and reports mergeable: false.
Diagram validation used the local Mermaid parser/renderer in Chromium and visual inspection; mmdc was unavailable, so no Mermaid-CLI validation is claimed. That historical receipt describes the original diagrams before this node-link and palette-only update.
| if _persisted_root_still_physical(root_line) and ( | ||
| _same_live_dir(root_line, str(cwd)) or any( | ||
| _same_live_dir(root_line, str(p)) for p in cwd.parents)): |
There was a problem hiding this comment.
[P2] F1 — a physical ancestor is not necessarily the current workspace owner.
The new guard closes the case where the captured repo root itself is rebound. It still accepts a snapshot from the wrong repository when only the captured cwd beneath that root is rebound.
Capture in outer/src, where outer is repository A and outer/inner is an independent nested repository B. The captured block correctly names - Root: outer. Rename src to src-saved, replace src with a symlink to inner, then resume with cwd explicitly bound to inner. The stored cwd now resolves to B, so _seed_workspace_pin passes its spelling check. The old root outer has not moved: it still resolves to itself and is an ancestor of B, so this new guard passes too. The seed at line 743 then attaches B's current inode to A's old block. The subsequent live identity check cannot reject a tuple that adoption has just certified this way.
Observed impact: the fresh-agent path replays A's root and recent-commit bytes in B, although a fresh workspace producer selects B's nearer .git root. The existing live agent rejects this same subtree rebind correctly on this head; the failure is specifically persisted adoption. The earlier raw cwd mismatch in _stored_prompt_matches_runtime is not a mitigation: it sends the restored session into the rebuild path, whose builder can re-adopt the old workspace block.
Attribution and evidence: this is a remaining variant of the previous provenance finding, not a new regression caused by 4a90a34b. The same source-isolated, real-Git owner-boundary probe produces B on base 28e6496, but replays A on parent a52ee4c and this head. The minimal assertion below likewise exits 0 on base and 1 on parent/head. Local execution used the inspected function bodies on Linux/Python 3.13.5, not an installed full-Hermes run; the broader probe binds the inspected session-cwd resolver and observes the actual block forwarded to a controlled posture adapter.
Minimal adoption-seam reproducer — run as python repro_f1.py in an installed checkout
from pathlib import Path
from tempfile import TemporaryDirectory
from types import SimpleNamespace
import subprocess
from agent.coding_context import build_coding_workspace_block
from agent.system_prompt import _seed_workspace_pin
with TemporaryDirectory() as tmp:
outer = Path(tmp).resolve() / "outer"
outer.mkdir()
subprocess.run(["git", "init", "-q", str(outer)], check=True)
src, inner = outer / "src", outer / "inner"
src.mkdir()
inner.mkdir()
subprocess.run(["git", "init", "-q", str(inner)], check=True)
for directory in (outer, inner):
subprocess.run(["git", "-C", str(directory), "-c", "user.name=Review",
"-c", "user.email=review@example.invalid", "commit",
"--allow-empty", "-qm", directory.name], check=True)
snapshot = build_coding_workspace_block(src)
assert f"- Root: {outer}\n" in snapshot
prompt = (f"User home directory: /home/local\n"
f"Current working directory: {src}\n\n{snapshot}")
resumed = SimpleNamespace(
session_id="review", _cached_system_prompt=None,
_session_db=SimpleNamespace(get_session=lambda _: {"system_prompt": prompt}),
)
src.rename(outer / "src-saved")
src.symlink_to(inner, target_is_directory=True)
assert outer.resolve() == outer
assert f"- Root: {inner}\n" in build_coding_workspace_block(inner)
_seed_workspace_pin(resumed, str(inner))
pin = getattr(resumed, "_frozen_workspace_snapshot", None)
assert pin is None, "adopted outer's snapshot for the independent inner repo"Observed head failure: AssertionError: adopted outer's snapshot for the independent inner repo.
Required repair: before adopting a persisted block, establish that its recorded root is the current workspace owner, not merely a physical ancestor. Reuse the nearest Git/marker-root resolution already used by the workspace producer; do not manufacture historical provenance by pairing an unrelated old block with the current cwd's inode. Preserve legitimate same-workspace alias adoption.
Regression contract: run this subtree-rebind case through a fresh agent reading persisted bytes and the real session-cwd binding. Assert B's root/commit appears, A's bytes are absent, and the live and resumed paths agree. Keep an unchanged subdirectory-resume control and a stable symlink-alias control. The new tests all capture at the repository root, so they do not distinguish root identity from cwd identity.
| agent._frozen_workspace_snapshot = (cwd_key, parts[1][0] if parts[1] else "", | ||
| _dir_identity(cwd_key)) |
There was a problem hiding this comment.
[P2] F2 — sample and validate directory identity across snapshot collection, not only afterward.
The tuple's third field is sampled only after coding_system_prompt_parts(...) has produced the snapshot. A directory replacement during that interval can therefore pair bytes from one directory with the identity of another.
The deterministic ordering is: _workspace_pin_key captures physical A; the producer finishes A's Git snapshot; another filesystem operation moves A to A-saved and replaces pathname A with a symlink to B; this new _dir_identity(cwd_key) then returns B's inode. The stored tuple becomes (A pathname, A snapshot, B identity). On a later rebuild explicitly bound to B, _same_pin_key(A, B) succeeds, and the new identity check also succeeds because A now stats to B. The result is a persistent replay of A's snapshot in B, not just a transient mixed read during the first build.
Observed evidence: a controlled producer-return interleaving on real temporary Git repositories and Linux/Python 3.13.5 records B's identity alongside A's bytes; the next build replays A. No Path.stat, resolve, samefile, or pin tuple was mocked. The scheduling wrapper performs the rename/symlink operation after the real workspace-block producer returns. The expected-B assertion passes on base 28e6496 and fails on both a52ee4c and 4a90a34b. This is a deterministic ordering witness, not a claim about production race frequency, and execution was source-isolated rather than a full installed-Hermes suite.
Minimal pin-owner reproducer — real workspace producer, controlled posture/scheduling adapter
Save as repro_f2.py and run with python repro_f2.py in an installed checkout. The adapter preserves the production None = probe, string = replay branch while excluding unrelated posture configuration.
from pathlib import Path
from tempfile import TemporaryDirectory
from types import SimpleNamespace
from unittest.mock import patch
import subprocess
import agent.coding_context as cc
from agent.runtime_cwd import set_session_cwd, reset_session_cwd
from agent.system_prompt import _coding_parts
with TemporaryDirectory() as tmp:
root = Path(tmp).resolve()
a, b = root / "A", root / "B"
for directory in (a, b):
directory.mkdir()
subprocess.run(["git", "init", "-q", str(directory)], check=True)
subprocess.run(["git", "-C", str(directory), "-c", "user.name=Review",
"-c", "user.email=review@example.invalid", "commit",
"--allow-empty", "-qm", directory.name], check=True)
agent = SimpleNamespace(platform="cli", model="gpt-4o", valid_tool_names={"terminal"})
armed = True
def renderer(*, cwd, workspace_block, **_):
global armed
# Preserve the production None=probe / string=replay contract.
block = cc.build_coding_workspace_block(cwd) if workspace_block is None else workspace_block
if armed:
assert f"- Root: {a}\n" in block
a.rename(root / "A-saved")
a.symlink_to(b, target_is_directory=True)
armed = False
return ["controlled posture"], [block] if block else [], []
with patch.object(cc, "coding_system_prompt_parts", side_effect=renderer):
token = set_session_cwd(str(a))
try:
first = _coding_parts(agent)
finally:
reset_session_cwd(token)
assert first[1], "first build failed unexpectedly"
token = set_session_cwd(str(b))
try:
second = _coding_parts(agent)
finally:
reset_session_cwd(token)
assert second[1], "coding block unexpectedly vanished"
assert f"- Root: {b}\n" in second[1][0], "A's snapshot is replayed in B"Observed head failure: AssertionError: A's snapshot is replayed in B.
Required repair: bind snapshot collection to the directory identity observed before collection and verify that identity again before publishing the pin. If it changed, discard the mismatched snapshot/pin and take a bounded retry or leave the pin unseeded; do not certify old bytes with a post-replacement fingerprint. Apply the same coherent bytes/identity rule wherever a tuple is published. This does not require treating ordinary Git-status changes within the same directory as an identity change.
Regression contract: interpose a deterministic barrier after workspace-block collection and before pin publication, replace the directory, then resume. Assert that no published tuple combines A's bytes with B's identity and the next rebuild for B emits B's root/commit. Keep the non-racing stable-alias and the already-fixed after-capture rebind controls. The new tests mutate only after the entire first build returns, so they cannot exercise this ordering.
There was a problem hiding this comment.
Continued re-review at 4a90a34bad6f7c7b4438b9bd085ff453254a4f92
Head is unchanged from the F1/F2 review. Those two provenance findings remain open. I found one additional P2 at the persisted-parser boundary, detailed inline: project-context prose can impersonate the workspace frame because the stored snapshot has no renderer-owned delimiter.
The source chain is direct:
- project context is serialized before the canonical workspace block;
- AGENTS.md content is inserted inside that project-context section;
- _persisted_workspace_block starts with prompt.find(...), so it examines the earliest workspace-looking text in the entire prompt;
- the current physical/ancestor gate then certifies that text without proving it came from the workspace renderer.
A portable reproducer can use the filesystem anchor (/ on POSIX, the drive root on Windows), so the context file does not need to know the clone path. On the inspected helper semantics the selected block is the project-context block, not the later canonical snapshot.
from pathlib import Path
from tempfile import TemporaryDirectory
from agent.coding_context import WORKSPACE_BLOCK_HEADER
from agent.system_prompt import _persisted_workspace_block
with TemporaryDirectory() as tmp:
cwd = Path(tmp).resolve()
ancestor = Path(cwd.anchor)
fake = (
f"{WORKSPACE_BLOCK_HEADER}\n"
f"- Root: {ancestor}\n"
"- Status: FAKE-CONTEXT"
)
real = (
f"{WORKSPACE_BLOCK_HEADER}\n"
f"- Root: {cwd}\n"
"- Status: clean"
)
prompt = (
"# Project Context\n\n"
"## AGENTS.md\n\n"
f"{fake}\n\n"
f"{real}\n\n"
"# Hermes runtime environment"
)
selected = _persisted_workspace_block(prompt, str(cwd))
assert selected == real, selected # fails: selects FAKE-CONTEXTMy source-isolated execution of the exact current helper logic on Linux/Python 3.13.5 returned the fake block. This is not an installed full-prompt test; the production reachability is established by the pinned builder ordering above.
The important impact is code-path, not merely prompt wording: once the earlier block is adopted, _seed_workspace_pin publishes it as the frozen snapshot, and the posture consumer receives a non-None workspace_block, so the real workspace producer is bypassed. If the context file is later removed, those stored bytes can still be carried forward as the workspace snapshot on reconstruction.
This is pre-existing at merge base, not introduced by 4a90a34b; it is relevant here because the touched adoption seam promises that only a real snapshot is adopted, and the new provenance checks still cannot establish which prompt owner emitted the block. F1's nearest-workspace-owner repair is necessary but not sufficient: a context block that names the actual root can still shadow the canonical one.
%%{init: {'theme':'dark','themeVariables':{'background':'#0d1117','primaryTextColor':'#e6edf3','lineColor':'#94a3b8'}}}%%
flowchart TD
subgraph CORE[" "]
direction TB
A["AGENTS.md / project context"]:::normal --> B["Serialized before canonical workspace"]:::transition
B --> C["prompt.find picks first workspace-looking block"]:::failure
C --> D{"Root passes physical / ancestor check"}:::failure
D --> E["_seed_workspace_pin adopts fake bytes"]:::failure
E --> F["workspace_block != None<br/>real Git probe skipped"]:::failure
R["Canonical workspace snapshot"]:::durable -. "appears later; never selected" .-> C
end
CORE ~~~ LEGEND
LEGEND["Legend<br/>Blue: entry / incoming<br/>Amber: transition / pending<br/>Teal: durable state<br/>Red: failure"]
style CORE fill:#0d1117,stroke:#30363d,color:#e6edf3
style LEGEND fill:#1e293b,stroke:#94a3b8,color:#f8fafc,font-size:11px
classDef transition fill:#451a03,stroke:#fbbf24,color:#fffbeb,stroke-width:2px
classDef normal fill:#172554,stroke:#60a5fa,color:#eff6ff,stroke-width:2px;
classDef durable fill:#134e4a,stroke:#2dd4bf,color:#f0fdfa,stroke-width:2px;
classDef failure fill:#450a0a,stroke:#f87171,color:#fef2f2,stroke-width:2px;
click A "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/prompt_builder.py#L1599-L1602" "Pinned source"
click B "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L859-L865" "Pinned source"
click C "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L681-L691" "Pinned source"
click D "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L694-L701" "Pinned source"
click E "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/system_prompt.py#L723-L743" "Pinned source"
click F "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/coding_context.py#L329-L350" "Pinned source"
click R "https://github.com/NousResearch/hermes-agent/blob/4a90a34bad6f7c7b4438b9bd085ff453254a4f92/agent/coding_context.py#L495-L530" "Pinned source"
Repair: give the workspace snapshot a renderer-owned frame (delimiter + length/checkable structure, analogous to the runtime/plugin framing) or persist it as a dedicated field. Parse only that owned frame; never discover it by searching arbitrary prompt prose. Regression coverage should place an exact workspace-looking block in AGENTS.md before the real snapshot, assert the canonical block is selected, then remove/change the context file and verify its old bytes cannot survive by being reclassified as the frozen workspace snapshot.
The earlier F1/F2 findings remain independently actionable; this finding is not a duplicate of either pathname/lifetime mechanism.
| # Provenance first: a Root line that no longer resolves to itself names a | ||
| # directory that was renamed or rebound after capture, so today's samefile | ||
| # agreement is not evidence this block belongs to the current workspace. | ||
| if _persisted_root_still_physical(root_line) and ( |
There was a problem hiding this comment.
[P2] This gate validates the path, but not the owner of the bytes it is validating.
_persisted_workspace_block searches the entire persisted prompt from its first occurrence of the workspace header. That is unsafe because the prompt builder places project context before the canonical coding workspace (builder order), and AGENTS.md content is inserted there (context rendering). A project context file can therefore contain the exact header before the real renderer-owned block.
The new physical/ancestor checks do not distinguish those sources. A fake context block with - Root: equal to the filesystem anchor (/ on POSIX; the drive root on Windows) is a physical ancestor of every cwd on that volume, so this branch returns it before reaching the real snapshot. No clone-path knowledge is required.
Minimal repro against this head:
from pathlib import Path
from tempfile import TemporaryDirectory
from agent.coding_context import WORKSPACE_BLOCK_HEADER
from agent.system_prompt import _persisted_workspace_block
with TemporaryDirectory() as tmp:
cwd = Path(tmp).resolve()
anchor = Path(cwd.anchor)
fake = f"{WORKSPACE_BLOCK_HEADER}\n- Root: {anchor}\n- Status: FAKE"
real = f"{WORKSPACE_BLOCK_HEADER}\n- Root: {cwd}\n- Status: clean"
prompt = f"# Project Context\n\n## AGENTS.md\n\n{fake}\n\n{real}"
assert _persisted_workspace_block(prompt, str(cwd)) == realObserved with the inspected helper semantics on Linux/Python 3.13.5: the assertion fails and the return value is the FAKE block. Source-isolated execution only; the pinned builder ordering above establishes that this prompt ordering is producible.
The consequence is behavioral: _seed_workspace_pin freezes those project-context bytes, and RuntimeMode.system_prompt_parts skips the real Git probe whenever workspace_block is non-None. Removed/changed context text can therefore survive reconstruction after being reclassified as the workspace snapshot.
Required repair: stop discovering this state from unframed prompt prose. Frame the canonical workspace block with renderer-owned delimiters/length (or persist it separately) and parse only that frame. A nearest-root check alone does not close this class because a context block can name the actual root.
Regression contract: persist a prompt whose AGENTS.md section contains an exact workspace-looking block before the canonical one; resume a fresh agent and assert the canonical block—not the project-context copy—is pinned. Then remove the context-file copy and verify its bytes cannot survive through the workspace pin.
The pin's equality checks proved path identity, not snapshot provenance: a pathname whose occupant was replaced still satisfied every one of them, and the persisted parser could be impersonated by earlier prompt prose. - Generation: the live pin's identity now records the owning .git marker's (st_dev, st_ino) beside the directory's — ordinary git work never deletes .git, so the pair is stable across a workspace's life and changes when a new generation takes over the pathname (a removed-and-recreated path can reuse the directory's own pair; observed on overlayfs). The published pin samples the generation on both sides of collection and publishes nothing on a mismatch, so a replacement landing between the producer's snapshot and a post-hoc stat can no longer certify old bytes with a new occupant's identity — the pin stays unseeded and the next build probes fresh. - Owner: adoption now requires the persisted - Root: line to be the workspace that owns the cwd — the producer's own git/marker-root resolution — not merely a physical ancestor; a nested independent repository under the captured root is a different workspace. - Capture-time evidence: the bytes must still describe today's occupant — a git workspace by its history tip (the same git log -3 render the producer emits), a marker-only workspace by its project facts. An independent repository created at the same pathname fails even when every spelling and ancestry check passes. - Framing: the persisted parser reads the LAST workspace-shaped block. Project context is rendered before the canonical snapshot and its AGENTS.md content is repo-controlled prose, so an earlier copy naming the real root can impersonate the frame — it cannot move itself after the canonical block. Five regressions pin the shapes with real repositories and the real builder: collection-time replacement publishes no pin and the rebuild probes the replacement; a nested-repo rebind, a same-path replacement and a recycled-identity empty pin each force a fresh probe; an AGENTS.md copy of the frame is never adopted and cannot outlive its file through the pin. All five fail on the parent head and pass here; the pin suites (test_compaction_prompt_rebuild 20/20, test_system_prompt, test_coding_context, test_plugin_prompt_sections, test_gitspawn_config_injection) run 129/129.
|
All three reviews are addressed in Generation/lifetime (the 16:55 review). The live pin's identity now records the owning F1 (16:59). Adoption now requires the persisted F2 (16:59). The pin is now published only when the generation sampled before collection equals the one sampled after ( Impersonation (17:30). You're right that a nearest-root check cannot close the class — a context block can name the actual root — so the parser stops reading unframed prose: Receipts. Five regressions in Residuals, stated plainly. Two shapes remain outside stat-only evidence: a same-path marker-only→marker-only live replacement whose directory identity is recycled, and a repo→repo live replacement recycling both the directory and |
…ip, not tip equality
dad6ca1 gated persisted-snapshot adoption on `git log -3` of the occupant being
byte-equal to the snapshot's `- Recent commits:` lines. Any commit landed since
session start therefore made the resumed/adopting agent re-probe and rewrite the
prompt prefix — the exact rebuild the pin exists to prevent, and a regression of
the base contract (`test_agent_that_did_not_build_the_prompt_replays_the_persisted_snapshot`
on a repo that gained a commit was green on main and red on dad6ca1).
The same-path-replacement evidence the check was added for survives: the captured
HEAD's short hash is an object only the repository that made it can contain, so
`git rev-parse --verify <hash>^{commit}` in today's occupant distinguishes repo A
from an independent repo B at the same pathname while accepting A after new
commits. An empty captured history (pre-first-commit) adopts, as before.
Regression: test_resumed_session_replays_the_snapshot_after_commits_landed_in_the_same_repo
(red at dad6ca1, green here); test_same_path_replacement_cannot_adopt_the_previous_workspace
stays green.
The code-health ratchet (blocking on CI) rejects a blind except Exception in _persisted_workspace_block. _workspace_roots only raises OSError (vanished key) or RuntimeError (symlink loop on Python <= 3.12) from Path.resolve; name those, matching the sibling seams in this module.
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
PR Review — #124787 (re-review of dad6ca1eb9)
Verdict: request changes → addressed in place. The spelling/identity work from the earlier rounds holds, but the newest commit regressed the pin's core contract; I pushed two maintainer-edit commits on this branch (80deae4c83, 35ab806fe2) so the branch is correct and CI-clean again. Merge decision stays with the maintainer.
Premise on origin/main (e36a818): unchanged — _workspace_pin_key still compares raw spellings, _persisted_workspace_block still does find() + ancestor match. 4,184 commits behind, merge-tree conflict-free; the merged tree is what I tested.
Critical (fixed in 80deae4c83): _persisted_generation_matches required git log -3 of today's repo to be byte-equal to the snapshot's - Recent commits: lines. Any commit landed since session start made adoption fail, so a resumed / gateway / TUI agent re-probed and rewrote the prompt prefix — the exact rebuild the pin exists to prevent. Live A/B through build_system_prompt with a real repo (capture → git commit → fresh agent with the session row):
| tree | resumed prompt byte-equal to stored |
|---|---|
origin/main |
True |
4a90a34bad (parent) |
True |
dad6ca1eb9 |
False — first differing byte is the - Recent commits: tip |
80deae4c83 |
True |
Fix: generation evidence is membership of the captured HEAD (git rev-parse --verify <first short hash>^{commit} in the occupant), not tip equality. An independent repo B created at the same pathname cannot contain A's commit object, so test_same_path_replacement_cannot_adopt_the_previous_workspace stays green; A after new commits passes. Regression test_resumed_session_replays_the_snapshot_after_commits_landed_in_the_same_repo — red at dad6ca1eb9, green at head.
Blocking CI red (fixed in 35ab806fe2): Python lints / code health ratchet (blocking) failed dad6ca1eb9 on agent/system_prompt.py::_persisted_workspace_block — BLE001 blind except Exception around _workspace_roots(key). Named the raise family (OSError, RuntimeError) like the sibling seams.
Tests (clean archive of 35ab806fe2, py3.11): test_compaction_prompt_rebuild + test_system_prompt + test_coding_context + test_system_prompt_restore + test_system_prompt_switch_invalidation + test_prompt_builder: 198 passed, 0 failed, 1 skipped (the macOS case-alias test). All 20 of the PR's own regressions pass; 14 of them are red on origin/main source with the PR's test file.
Still open from earlier threads (not regressions, note for the maintainer):
agent/conversation_loop.py::_stored_prompt_matches_runtime(jonpol01) still compares the storedCurrent working directoryraw, so a spelling-only change still fully rebuilds a resumed/adopted prompt — same class, separate seam. Follow-up as the author proposed, or fold a_same_live_dircall in here.- The windows junction twin of the case-alias test (andrexibiza) remains a coverage gap, not a defect.
Shape: agent/system_prompt.py 873 → 1,087 lines, all pin logic appended to the facade. Not blocking at this size, but if another round lands here the pin seams (_same_live_dir … _persisted_workspace_block) want a system_prompt_workspace_pin.py sibling.
Live repro: before — dad6ca1eb9 rebuilt the prompt on resume after one commit (6448-byte prompt diverged at byte 4925); after — byte-equal replay. Scope is library-level (build_system_prompt with real git repos); I did not drive a CLI --resume session for this round.
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
PR Review — #124787 (re-review of dad6ca1eb9)
Verdict: findings addressed in place (posted as a comment review; the CHANGES_REQUESTED copy was dismissed so it cannot gate the merge). The spelling/identity work from the earlier rounds holds, but the newest commit regressed the pin's core contract; I pushed two maintainer-edit commits on this branch (80deae4c83, 35ab806fe2) so the branch is correct and CI-clean again. Merge decision stays with the maintainer.
Premise on origin/main (e36a818): unchanged — _workspace_pin_key still compares raw spellings, _persisted_workspace_block still does find() + ancestor match. 4,184 commits behind, merge-tree conflict-free; the merged tree is what I tested.
Critical (fixed in 80deae4c83): _persisted_generation_matches required git log -3 of today's repo to be byte-equal to the snapshot's - Recent commits: lines. Any commit landed since session start made adoption fail, so a resumed / gateway / TUI agent re-probed and rewrote the prompt prefix — the exact rebuild the pin exists to prevent. Live A/B through build_system_prompt with a real repo (capture → git commit → fresh agent with the session row):
| tree | resumed prompt byte-equal to stored |
|---|---|
origin/main |
True |
4a90a34bad (parent) |
True |
dad6ca1eb9 |
False — first differing byte is the - Recent commits: tip |
80deae4c83 |
True |
Fix: generation evidence is membership of the captured HEAD (git rev-parse --verify <first short hash>^{commit} in the occupant), not tip equality. An independent repo B created at the same pathname cannot contain A's commit object, so test_same_path_replacement_cannot_adopt_the_previous_workspace stays green; A after new commits passes. Regression test_resumed_session_replays_the_snapshot_after_commits_landed_in_the_same_repo — red at dad6ca1eb9, green at head.
Blocking CI red (fixed in 35ab806fe2): Python lints / code health ratchet (blocking) failed dad6ca1eb9 on agent/system_prompt.py::_persisted_workspace_block — BLE001 blind except Exception around _workspace_roots(key). Named the raise family (OSError, RuntimeError) like the sibling seams.
Tests (clean archive of 35ab806fe2, py3.11): test_compaction_prompt_rebuild + test_system_prompt + test_coding_context + test_system_prompt_restore + test_system_prompt_switch_invalidation + test_prompt_builder: 198 passed, 0 failed, 1 skipped (the macOS case-alias test). All 20 of the PR's own regressions pass; 14 of them are red on origin/main source with the PR's test file.
Still open from earlier threads (not regressions, note for the maintainer):
agent/conversation_loop.py::_stored_prompt_matches_runtime(jonpol01) still compares the storedCurrent working directoryraw, so a spelling-only change still fully rebuilds a resumed/adopted prompt — same class, separate seam. Follow-up as the author proposed, or fold a_same_live_dircall in here.- The windows junction twin of the case-alias test (andrexibiza) remains a coverage gap, not a defect.
Shape: agent/system_prompt.py 873 → 1,087 lines, all pin logic appended to the facade. Not blocking at this size, but if another round lands here the pin seams (_same_live_dir … _persisted_workspace_block) want a system_prompt_workspace_pin.py sibling.
Live repro: before — dad6ca1eb9 rebuilt the prompt on resume after one commit (6448-byte prompt diverged at byte 4925); after — byte-equal replay. Scope is library-level (build_system_prompt with real git repos); I did not drive a CLI --resume session for this round.
|
Concrete merge-gate follow-up, not a new code review: The same mismatch exists on #125374 at The actionable next step is to refresh these existing branches against the current reviewed CI definition, preserving contributor commits, and obtain a real successful v2 aggregate on the resulting heads. Re-running only an old workflow definition that still emits the old name will not satisfy the new requirement. Please do not rename/spoof a status or weaken the rule. Re-check other merge requirements afterward; I have not established that this is the sole blocker. Credit remains with @lihongyuan99 and @teknium1 for the workspace implementation/review fixes, and @123456hyu, @andrexibiza and @ehz0ah for the cron implementation and follow-up reviews. No branch or repository-policy changes made here. |
This branch has not been deployed
7 participants
What does this PR do?
_workspace_pin_key()compared the probe directory as a raw string, but the two builds it is meant to unify spell the same directory differently:os.getcwd()— which reports the physical path (/private/var/...on macOS, or any symlink-resolved dir on Linux);/compress, gateway session cwd) keys on the session's spelling of the same directory (/var/...through the symlink).The spelling mismatch misses the pin, so the rebuild re-probes git live and rewrites the session-start snapshot mid-session — changing the system prompt bytes and invalidating the cached prompt prefix for a workspace that never changed. That is exactly the contract
_workspace_pin_key's docstring promises ("a build with no cwd bound (launch dir) and a later one binding that same dir are one workspace, not two") and exactly the invariant the codebase protects everywhere else (per-conversation prompt caching).On Linux
/tmpis a real directory, so the suite stayed green there and the class hid: three tests inTestWorkspaceSnapshotPinnedAcrossCompactionfail on macOSmainand pass with this fix.The fix normalizes the pin key — and
_seed_workspace_pin's persisted-cwd comparison — withPath.resolve(), the same normalization_persisted_workspace_blockalready applies to its own comparisons. On symlink-free pathsresolve()is the identity, so Linux behavior is unchanged.Related Issue
No issue filed — found by running
TestWorkspaceSnapshotPinnedAcrossCompactionon macOSmain(3 failures).Type of Change
Changes Made
agent/system_prompt.py_workspace_pin_key(): return the resolved real path (empty key stays empty); docstring documents the spelling trap._seed_workspace_pin(): compare the persisted "Current working directory" hint to the key with the same normalization.tests/agent/test_compaction_prompt_rebuild.py- Root:assertions now expectpath.resolve()— the workspace root is the git toplevel, andgit rev-parse --show-toplevelreports resolved paths on every OS.How to Test
scripts/run_tests.sh tests/agent/test_compaction_prompt_rebuild.py— red onmain, green with this fix (now 12 tests: the resolved-spelling, case-alias and symlink-loop regressions included; the case-alias one isplatforms("macos")-marked so the macOS lane imports the file). On Linux the file was already green and stays green.scripts/run_tests.sh tests/agent/test_system_prompt.py tests/agent/test_system_prompt_restore.py tests/agent/test_system_prompt_switch_invalidation.py tests/agent/test_prompt_builder.py tests/agent/test_coding_context.py— 186 green.mktemp -dunder/var/folders), then rebuild with the same directory bound via its logical spelling — before the fix the rebuild re-probes git live and emits different prompt bytes once the worktree changes; after the fix the session-start snapshot is replayed.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass (prompt surface: 186 green)Documentation & Housekeeping
_workspace_pin_keydocstring documents the spelling trapcli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A/var→/private/var) broke the pin