Repository navigation
fix(agent): normalize the workspace pin key to the resolved path #124787
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 6 commits
1088868
e1b2ed3
9fef6b1
f6871b3
6eb71ac
a52ee4c
4a90a34
dad6ca1
80deae4
35ab806
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -593,15 +593,58 @@ def _alibaba_identity_part(agent: Any) -> List[str]: | |
|
|
||
|
|
||
| def _workspace_pin_key() -> str: | ||
| """The directory the workspace probe inspects, which is also the prompt's ``Current working | ||
| directory``: a build with no cwd bound (launch dir) and a later one binding that same dir | ||
| (TUI ``/compress``) are one workspace, not two.""" | ||
| """The directory the workspace probe inspects, in resolved form — the prompt's | ||
| ``Current working directory`` line carries the session's own spelling of it. Under that | ||
| normalization a build with no cwd bound (launch dir) and a later one binding that same | ||
| dir (TUI ``/compress``) are one workspace, not two. | ||
|
|
||
| Normalized to the resolved real path: a launch-dir key comes from ``os.getcwd()`` (which | ||
| reports the physical path — ``/private/var/...`` on macOS) while a bound cwd arrives as the | ||
| session spelled it (``/var/...`` through the symlink). Comparing those spellings misses the | ||
| pin, the rebuild re-probes git live, and the session-start snapshot is rewritten mid-session | ||
| — invalidating the cached prompt prefix for a workspace that never changed.""" | ||
| try: | ||
| return str(resolve_context_cwd() or resolve_agent_cwd()) | ||
| except OSError: # deleted 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [P2] Compare existing directories by filesystem identity. I reproduced this on the exact head through the real Please compare live paths with |
||
| except (OSError, RuntimeError): # deleted cwd, or a symlink loop in a bound spelling | ||
| return "" | ||
|
|
||
|
|
||
| def _same_live_dir(a: str, b: str) -> bool: | ||
| """Directory identity that survives spelling aliases a plain string comparison misses. | ||
|
|
||
| *Symlink* spellings: a launch-dir key comes from ``os.getcwd()`` (the physical path — | ||
| ``/private/var/...`` on macOS) while a bound cwd or a persisted ``- Root:`` line carries | ||
| the session's own spelling (``/var/...`` through the symlink); comparing those raw | ||
| misses the pin, the rebuild re-probes git live, and the session-start snapshot is | ||
| rewritten mid-session — invalidating the cached prompt prefix for a workspace that | ||
| never changed. | ||
| *Case* spellings: resolve() follows symlinks but keeps the caller's casing, so on a | ||
| case-insensitive filesystem (default macOS APFS) one directory spelled ``MixedCase`` | ||
| and ``mixedcase`` still yields two strings. | ||
|
|
||
| When both paths exist the filesystem itself (``samefile``) is the authority; a missing | ||
| path (deleted cwd, vanished root) falls back to the string comparison. Both inputs may | ||
| be persisted session bytes, so resolve() runs here under (OSError, RuntimeError): a | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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 The consequence follows through the changed caller: The persisted sibling has the same cause. 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 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. |
||
| except (OSError, RuntimeError): | ||
| return False | ||
| if pa == pb: | ||
| return True | ||
| try: | ||
| return pa.samefile(pb) | ||
| except OSError: | ||
| return False | ||
|
|
||
|
|
||
| def _same_pin_key(a: str, b: str) -> bool: | ||
| """Pin-key equality; "" is a real pinned value (no workspace) and only equals itself.""" | ||
| return a == b or (bool(a) and bool(b) and _same_live_dir(a, b)) | ||
|
|
||
|
|
||
| def _persisted_workspace_block(prompt: str, key: str) -> Optional[str]: | ||
| """The workspace snapshot inside ``prompt`` taken for ``key`` (its ``- Root:`` is ``key`` or an | ||
| ancestor); "" when the prompt has none; None when it has one for another root.""" | ||
|
|
@@ -613,8 +656,11 @@ def _persisted_workspace_block(prompt: str, key: str) -> Optional[str]: | |
| cwd = Path(key).resolve() | ||
| while start >= 0: | ||
| block = prompt[start + 2:].split("\n\n", 1)[0] | ||
| root = Path(block.split("\n", 2)[1][len("- Root: "):]).resolve() | ||
| if root == cwd or root in cwd.parents: | ||
| # The Root line is persisted session bytes: resolve it inside _same_live_dir so a | ||
| # symlink loop in it cannot escape into the caller's blanket handler. | ||
| root_line = block.split("\n", 2)[1][len("- Root: "):] | ||
| if _same_live_dir(root_line, str(cwd)) or any( | ||
| _same_live_dir(root_line, str(p)) for p in cwd.parents): | ||
| return block | ||
| start = prompt.find(head, start + 2) | ||
| return None | ||
|
|
@@ -647,8 +693,12 @@ def _seed_workspace_pin(agent: Any, key: str) -> None: | |
| if not prompt: | ||
| return | ||
| stored_cwd = runtime_host_value(prompt, "Current working directory") | ||
| if stored_cwd and stored_cwd != key: | ||
| return | ||
| if stored_cwd: | ||
| # 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(stored_cwd, key): | ||
| return | ||
| block = _persisted_workspace_block(prompt, key) | ||
| # Only a real snapshot is adopted: a prompt without one (built on a surface without the | ||
| # coding posture, or with tools off) leaves the pin open so this build captures one. | ||
|
|
@@ -677,7 +727,7 @@ def _coding_parts(agent: Any) -> Tuple[List[str], List[str], List[str]]: | |
| _seed_workspace_pin(agent, cwd_key) | ||
| pinned = getattr(agent, "_frozen_workspace_snapshot", None) | ||
| # "" is a real pinned value (no workspace here) — only a cwd mismatch re-probes. | ||
| replay = pinned[1] if pinned is not None and pinned[0] == cwd_key else None | ||
| replay = pinned[1] if pinned is not None and _same_pin_key(pinned[0], cwd_key) else None | ||
| parts = coding_system_prompt_parts(platform=agent.platform, cwd=cwd, model=agent.model, | ||
| valid_tool_names=agent.valid_tool_names, workspace_block=replay) | ||
| if replay is None: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| lihongyuan99 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,9 @@ | |
| back to its last good bytes (fail-open), never silently vanishes | ||
| """ | ||
| import unittest | ||
|
|
||
| import pytest | ||
|
|
||
| from types import SimpleNamespace | ||
| from unittest.mock import patch | ||
|
|
||
|
|
@@ -252,6 +255,74 @@ def test_binding_the_launch_dir_explicitly_replays_the_pin(self): | |
| os.chdir(old_cwd) | ||
| shutil.rmtree(tmp, ignore_errors=True) | ||
|
|
||
| def test_symlink_spelling_of_the_launch_dir_replays_the_pin(self): | ||
| """The macOS shape on any POSIX host: os.getcwd() reports the physical dir while the bound | ||
| cwd arrives through a symlink (/var -> /private/var). Same workspace, so the pin must hit.""" | ||
| import os, sys, tempfile, shutil | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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 |
||
| self.skipTest("directory symlinks need privileges on Windows") | ||
| tmp = Path(tempfile.mkdtemp(prefix="test-pinned-symlink-")) | ||
| old_cwd = os.getcwd() | ||
| try: | ||
| repo = _init_repo(tmp / "proj", "init commit") | ||
| link = tmp / "link" | ||
| link.symlink_to(repo, target_is_directory=True) | ||
| os.chdir(repo) # os.getcwd() is the physical spelling | ||
| agent = self._pin_agent() | ||
| with patch("agent.prompt_builder.load_soul_md", return_value=""), \ | ||
| patch("agent.prompt_builder.build_environment_hints", return_value="ENV HINTS"): | ||
| with patch("agent.system_prompt.resolve_context_cwd", return_value=None): | ||
| p1 = build_system_prompt(agent) | ||
| self.assertIn("Status: clean", p1) | ||
| (repo / "untracked.txt").write_text("wip\n") | ||
| invalidate_system_prompt(agent) | ||
| with patch("agent.system_prompt.resolve_context_cwd", return_value=link): | ||
| self.assertEqual(build_system_prompt(agent), p1) | ||
| finally: | ||
| os.chdir(old_cwd) | ||
| shutil.rmtree(tmp, ignore_errors=True) | ||
|
|
||
| @pytest.mark.platforms("macos") | ||
| def test_case_alias_spelling_of_the_launch_dir_replays_the_pin(self): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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:
Consequence: the regression added to guard the macOS finding stays green on every lane without ever running — if 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 Regression check: |
||
| """macOS default APFS is case-insensitive and case-preserving: resolve() follows the | ||
| symlink yet keeps the caller's casing, so a directory bound under a differently cased | ||
| spelling is the same workspace and the pin must hit. The platforms marker brings the | ||
| file into the macOS lane (an unmarked file runs on no OS lane at all); the samefile | ||
| self-guard below still skips on a case-sensitive volume, where the alias genuinely | ||
| is a second directory.""" | ||
| import os, tempfile, shutil | ||
| from pathlib import Path | ||
| from agent.system_prompt import build_system_prompt, invalidate_system_prompt | ||
|
|
||
| tmp = Path(tempfile.mkdtemp(prefix="test-pinned-case-alias-")) | ||
| old_cwd = os.getcwd() | ||
| try: | ||
| repo = _init_repo(tmp / "Repo", "init commit") | ||
| alias = repo.with_name(repo.name.lower()) | ||
| try: | ||
| alias_same = alias.samefile(repo) | ||
| except OSError: | ||
| alias_same = False # case-sensitive filesystem: the alias does not exist | ||
| if not alias_same: | ||
| self.skipTest("case-sensitive filesystem: the alias is a different directory") | ||
| os.chdir(repo) # os.getcwd() reports the on-disk casing | ||
| agent = self._pin_agent() | ||
| with patch("agent.prompt_builder.load_soul_md", return_value=""), \ | ||
| patch("agent.prompt_builder.build_environment_hints", return_value="ENV HINTS"): | ||
| with patch("agent.system_prompt.resolve_context_cwd", return_value=None): | ||
| p1 = build_system_prompt(agent) | ||
| self.assertIn("Status: clean", p1) | ||
| (repo / "untracked.txt").write_text("wip\n") | ||
| invalidate_system_prompt(agent) | ||
| with patch("agent.system_prompt.resolve_context_cwd", return_value=alias): | ||
| self.assertEqual(build_system_prompt(agent), p1) | ||
| finally: | ||
| os.chdir(old_cwd) | ||
| shutil.rmtree(tmp, ignore_errors=True) | ||
|
|
||
| def test_agent_that_did_not_build_the_prompt_replays_the_persisted_snapshot(self): | ||
| """Resume / gateway / TUI shape: a fresh agent rebuilds (compaction, a first /compress) after | ||
| the repo moved and replays the snapshot its session row already holds — unless that prompt | ||
|
|
@@ -279,7 +350,45 @@ def env(cwd): | |
| with patch("agent.prompt_builder.load_soul_md", return_value=""), env(other), \ | ||
| patch("agent.system_prompt.resolve_context_cwd", return_value=other): | ||
| moved = self._pin_agent(_cached_system_prompt=None, _session_db=db) | ||
| self.assertIn(f"- Root: {other}", build_system_prompt(moved)) | ||
| self.assertIn(f"- Root: {other.resolve()}", build_system_prompt(moved)) | ||
| finally: | ||
| shutil.rmtree(tmp, ignore_errors=True) | ||
|
|
||
| def test_loop_cwd_spelling_in_persisted_bytes_cannot_drop_the_coding_block(self): | ||
| """A symlink loop in persisted session bytes (the Current working directory hint or a | ||
| ``- Root:`` line) makes Path.resolve raise RuntimeError on Python <= 3.12; the pin | ||
| seams must contain it instead of letting it reach the blanket handler that would | ||
| silently drop the whole coding block for that build.""" | ||
| import tempfile, shutil | ||
| from pathlib import Path | ||
| from agent.system_prompt import _same_live_dir, build_system_prompt | ||
|
|
||
| tmp = Path(tempfile.mkdtemp(prefix="test-pinned-loop-")) | ||
| try: | ||
| loop = tmp / "loop" | ||
| loop.symlink_to(loop) # symlink pointing at itself | ||
| # Contained at the comparison seam on every supported interpreter. | ||
| self.assertIs(_same_live_dir(str(loop), str(tmp)), False) | ||
|
|
||
| repo, other = _init_repo(tmp / "proj", "init commit"), _init_repo(tmp / "other", "init other") | ||
|
|
||
| def env(cwd): | ||
| return patch("agent.prompt_builder.build_environment_hints", | ||
| return_value=f"Host: x\nUser home directory: /h\nCurrent working directory: {cwd}") | ||
|
|
||
| with patch("agent.prompt_builder.load_soul_md", return_value=""), env(repo), \ | ||
| patch("agent.system_prompt.resolve_context_cwd", return_value=repo): | ||
| stored = build_system_prompt(self._pin_agent()) | ||
| self.assertIn("Status: clean", stored) | ||
| # A persisted prompt whose cwd hint spells a symlink loop: seeding must neither | ||
| # adopt nor raise — the rebuilt prompt keeps its coding block. | ||
| poisoned = stored.replace(f"Current working directory: {repo}", | ||
| f"Current working directory: {loop}") | ||
| db = SimpleNamespace(get_session=lambda sid: {"system_prompt": poisoned}) | ||
| with patch("agent.prompt_builder.load_soul_md", return_value=""), env(other), \ | ||
| patch("agent.system_prompt.resolve_context_cwd", return_value=other): | ||
| self.assertIn("- Root: ", build_system_prompt( | ||
| self._pin_agent(_cached_system_prompt=None, _session_db=db))) | ||
| finally: | ||
| shutil.rmtree(tmp, ignore_errors=True) | ||
|
|
||
|
|
@@ -300,7 +409,7 @@ def test_persisted_prompt_without_a_snapshot_does_not_pin_an_empty_one(self): | |
| return_value=f"Host: x\nUser home directory: /h\nCurrent working directory: {repo}"), \ | ||
| patch("agent.system_prompt.resolve_context_cwd", return_value=repo): | ||
| resumed = self._pin_agent(_cached_system_prompt=None, _session_db=db) | ||
| self.assertIn(f"- Root: {repo}", build_system_prompt(resumed)) | ||
| self.assertIn(f"- Root: {repo.resolve()}", build_system_prompt(resumed)) | ||
| finally: | ||
| shutil.rmtree(tmp, ignore_errors=True) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nonblocking] The same spelling comparison remains at the runtime-cwd sink.
agent/conversation_loop.py:877-878(_stored_prompt_matches_runtime) still compares the storedCurrent working directorytostr(resolve_agent_cwd())raw, andagent/prompt_builder.py:1001renders 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 hostCurrent 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.