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 5 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 |
|---|---|---|
|
|
@@ -595,13 +595,41 @@ 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.""" | ||
| (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()) | ||
| 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: # deleted cwd | ||
| return "" | ||
|
|
||
|
|
||
| def _same_live_dir(a: Path, b: Path) -> bool: | ||
| """Directory identity that survives spelling aliases resolve() cannot normalize. On a | ||
| case-insensitive filesystem (default macOS APFS) resolve() follows symlinks but keeps the | ||
| caller's casing, so one directory spelled ``MixedCase`` and ``mixedcase`` yields two | ||
| resolved strings — and two pin keys, so the rebuild re-probes git and rewrites the | ||
| session-start snapshot mid-session. When both paths exist the filesystem itself | ||
| (``samefile``) is the authority; a missing path (deleted cwd, vanished root) falls back | ||
| to the string comparison.""" | ||
| if a == b: | ||
| return True | ||
| try: | ||
| return a.samefile(b) | ||
| 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(Path(a), Path(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.""" | ||
|
|
@@ -614,7 +642,7 @@ def _persisted_workspace_block(prompt: str, key: str) -> Optional[str]: | |
| 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: | ||
| if _same_live_dir(root, cwd) or any(_same_live_dir(root, p) for p in cwd.parents): | ||
| return block | ||
| start = prompt.find(head, start + 2) | ||
| return None | ||
|
|
@@ -647,8 +675,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(Path(stored_cwd).resolve(), Path(key).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]
Consequence at this seam: Reachability: merge base Repair (smallest complete): normalize the two arguments inside |
||
| 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 +709,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,7 @@ 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) | ||
|
|
||
|
|
@@ -300,7 +371,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.