fix(workspace-changes): classify a symlink replacing a file distinctly from deleted (#4170)

* fix(workspace-changes): classify a symlink replacing a file distinctly from deleted

scan_workspace_roots() skipped every symlinked path entirely
(host_file.is_symlink() -> continue), so the path was completely absent
from a snapshot instead of being recorded as a metadata-only stub the
way binary/large/sensitive-looking files already are. When an agent run
replaces a tracked file with a symlink (e.g. rm config.txt && ln -s
/some/other/path config.txt), the after-snapshot never contained that
path at all, so compare_snapshots()'s _status() saw after_file=None and
reported plain "deleted" -- silently hiding that the path is still alive
on disk, now as a symlink that can point anywhere on the host, including
outside the workspace root.

Add a "symlink" classification mirroring the existing binary/large/
sensitive pattern: scan_workspace_roots() now records a symlink as a
metadata-only FileSnapshot stub (symlink=True, symlink_target from
os.readlink(), lstat'd without ever following the link) instead of
omitting it. _status() reports "symlink_created" whenever a symlink
newly occupies a path that was not already a symlink (brand new or
replacing a prior file), so the security-relevant fact surfaces
distinctly instead of collapsing into "deleted". A symlink genuinely
removed with nothing replacing it is unchanged: still "deleted".

Verified against a real POSIX symlink (WSL; native Windows symlink
creation needs elevated privilege) driving the unmodified
scan_workspace_roots()/compare_snapshots() functions, and via a
patch-file revert/reapply cycle on this same fix to confirm the added
regression tests fail before and pass after.

* fix(workspace-changes): count symlink_created in the changed-file badge

getChangedFileCount summed only created + modified + deleted, so a run
whose only change was a symlink replacing a file (reported as the new
symlink_created status, not deleted) produced a count of 0 and
WorkspaceChangeBadge hid the badge entirely -- the exact scenario this
PR targets, with the opposite of the intended result.

Add symlink_created to the frontend WorkspaceChangeSummary interface
(already emitted by the backend) and include it in the count. The
per-file StatusIcon/statusLabel "modified" fallthrough is unaffected
and left as a follow-up, per review.

Regression test reverts cleanly to reproduce a count of 0 pre-fix.

* fix(workspace-changes): rank symlink_created in file sort and complete the type contract

sortWorkspaceChanges's statusRank had no entry for the new
symlink_created status, so statusRank[left.status] - statusRank[right.status]
evaluated to NaN for any comparison involving a symlink-created file,
violating Array#sort's ordering contract instead of producing a
deterministic order. The frontend WorkspaceChangeStatus and
DiffUnavailableReason unions, and the WorkspaceFileChange symlink
fields, also stayed narrower than what the backend now emits, so
TypeScript's satisfies Record<...> guard on statusRank could not catch
the gap.

Widen WorkspaceChangeStatus to include "symlink_created" and
DiffUnavailableReason to include "symlink", add the matching
symlink/symlink_target_before/symlink_target_after fields to
WorkspaceFileChange, and give symlink_created a rank alongside
modified in statusRank -- restoring the satisfies guard's ability to
catch a future unranked status. unavailableLabel now has an explicit
"symlink" branch (new symlinkUnavailable i18n string, en-US + zh-CN)
instead of falling through to the generic label.

Also fixes the failing e2e-tests CI check: the existing
workspace-changes.spec.ts mock summary predates the symlink_created
field, so getChangedFileCount computed 1 + 1 + 0 + undefined = NaN and
the badge rendered "Edited NaN files" instead of "Edited 2 files".
Added symlink_created: 0 to the mock to match the real backend
contract.

New sortWorkspaceChanges unit tests revert cleanly against the
unranked statusRank to reproduce the NaN-driven misordering. pnpm test
(626 tests), pnpm check, and pnpm format are clean, and the
previously-failing e2e spec plus the full e2e suite (94 tests) pass.
This commit is contained in:
Daoyuan Li 2026-07-20 19:22:55 -07:00 committed by GitHub
parent 9eebc6a9e5
commit b565e6c0f0
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
14 changed files with 266 additions and 11 deletions

View File

@ -8,6 +8,7 @@ EMPTY_SUMMARY = {
"created": 0,
"modified": 0,
"deleted": 0,
"symlink_created": 0,
"additions": 0,
"deletions": 0,
"truncated": False,

View File

@ -23,7 +23,7 @@ def compare_snapshots(
resolved_limits = limits or WorkspaceChangeLimits()
all_paths = sorted(set(before.files) | set(after.files))
changes: list[WorkspaceFileChange] = []
created = modified = deleted = additions = deletions = 0
created = modified = deleted = symlink_created = additions = deletions = 0
total_diff_bytes = 0
truncated = before.truncated or after.truncated
@ -38,6 +38,8 @@ def compare_snapshots(
created += 1
elif status == "modified":
modified += 1
elif status == "symlink_created":
symlink_created += 1
else:
deleted += 1
@ -73,6 +75,9 @@ def compare_snapshots(
diff_unavailable_reason=reason,
additions=line_additions,
deletions=line_deletions,
symlink=bool((after_file or before_file).symlink if (after_file or before_file) else False),
symlink_target_before=before_file.symlink_target if before_file else None,
symlink_target_after=after_file.symlink_target if after_file else None,
)
)
else:
@ -83,6 +88,7 @@ def compare_snapshots(
created=created,
modified=modified,
deleted=deleted,
symlink_created=symlink_created,
additions=additions,
deletions=deletions,
truncated=truncated,
@ -107,6 +113,15 @@ def _status(
before_file: FileSnapshot | None,
after_file: FileSnapshot | None,
) -> WorkspaceChangeStatus:
# A symlink now occupying a path that was not already a symlink is always
# surfaced distinctly - whether it is brand new (before_file is None) or it
# just replaced a regular file (before_file is None => "deleted" would
# otherwise be reported even though the path is still alive on disk, just
# as a symlink that may point anywhere on the host).
before_was_symlink = before_file is not None and before_file.symlink
after_is_symlink = after_file is not None and after_file.symlink
if after_is_symlink and not before_was_symlink:
return "symlink_created"
if before_file is None:
return "created"
if after_file is None:
@ -160,7 +175,7 @@ def _diff_unavailable_reason(
after_file: FileSnapshot | None,
) -> DiffUnavailableReason | None:
files = [file for file in (before_file, after_file) if file is not None]
for preferred in ("sensitive", "binary", "large"):
for preferred in ("symlink", "sensitive", "binary", "large"):
if any(file.content_unavailable_reason == preferred for file in files):
return preferred # type: ignore[return-value]
return None

View File

@ -147,7 +147,7 @@ async def record_workspace_changes(
payload = result.to_dict()
summary = result.summary
changed_file_count = summary.created + summary.modified + summary.deleted
changed_file_count = summary.created + summary.modified + summary.deleted + summary.symlink_created
content = f"{changed_file_count} file{'s' if changed_file_count != 1 else ''} changed +{summary.additions} -{summary.deletions}"
return await event_store.put(
thread_id=thread_id,

View File

@ -121,7 +121,18 @@ def scan_workspace_roots(
)
host_file = Path(dirpath) / filename
if host_file.is_symlink() or not host_file.is_file():
if host_file.is_symlink():
# A symlink must never be followed for stat/content purposes: its
# target can point anywhere on the host (including outside the
# scanned root), so it is recorded as a metadata-only stub -
# mirroring how binary/large/sensitive-looking files are handled
# below - instead of being silently omitted from the snapshot.
symlink_snapshot = _snapshot_symlink(root, host_file)
if symlink_snapshot is not None:
files[symlink_snapshot.path] = symlink_snapshot
scanned += 1
continue
if not host_file.is_file():
continue
snapshot = _snapshot_file(
@ -222,6 +233,41 @@ def _snapshot_file(
)
def _snapshot_symlink(root: WorkspaceRoot, host_file: Path) -> FileSnapshot | None:
# Deliberately never follows the link (no read_bytes()/open() on the target):
# the target may point anywhere on the host, including outside the scanned
# root, so stat'ing or reading through it here would risk exposing arbitrary
# host file content/metadata as if it belonged to the workspace.
try:
stat = host_file.lstat()
size = stat.st_size
mtime_ns = stat.st_mtime_ns
relative = host_file.relative_to(root.host_path).as_posix()
virtual_path = f"{root.virtual_prefix}/{relative}"
sensitive = is_sensitive_workspace_path(virtual_path)
except OSError:
return None
try:
target = os.readlink(host_file)
except OSError:
target = None
return FileSnapshot(
path=virtual_path,
root=root.name,
size=size,
mtime_ns=mtime_ns,
sha256=None,
binary=False,
sensitive=sensitive,
text=None,
content_unavailable_reason="symlink",
symlink=True,
symlink_target=target,
)
def _cache_text_file(text: str, virtual_path: str, cache_dir: Path) -> Path:
cache_name = hashlib.sha256(virtual_path.encode("utf-8")).hexdigest()
target = cache_dir / cache_name

View File

@ -7,8 +7,8 @@ from typing import Literal
WORKSPACE_CHANGES_EVENT_TYPE = "workspace_changes"
WORKSPACE_CHANGES_METADATA_KEY = "workspace_changes"
WorkspaceChangeStatus = Literal["created", "modified", "deleted"]
DiffUnavailableReason = Literal["binary", "large", "sensitive", "truncated"]
WorkspaceChangeStatus = Literal["created", "modified", "deleted", "symlink_created"]
DiffUnavailableReason = Literal["binary", "large", "sensitive", "truncated", "symlink"]
@dataclass(frozen=True)
@ -45,6 +45,8 @@ class FileSnapshot:
text: str | None = None
text_path: str | None = None
content_unavailable_reason: DiffUnavailableReason | None = None
symlink: bool = False
symlink_target: str | None = None
@dataclass(frozen=True)
@ -70,6 +72,9 @@ class WorkspaceFileChange:
diff_unavailable_reason: DiffUnavailableReason | None = None
additions: int = 0
deletions: int = 0
symlink: bool = False
symlink_target_before: str | None = None
symlink_target_after: str | None = None
def to_dict(self) -> dict:
return asdict(self)
@ -80,6 +85,7 @@ class WorkspaceChangeSummary:
created: int = 0
modified: int = 0
deleted: int = 0
symlink_created: int = 0
additions: int = 0
deletions: int = 0
truncated: bool = False
@ -96,7 +102,7 @@ class WorkspaceChangeResult:
version: int = 1
def has_changes(self) -> bool:
return bool(self.summary.created or self.summary.modified or self.summary.deleted or self.summary.additions or self.summary.deletions)
return bool(self.summary.created or self.summary.modified or self.summary.deleted or self.summary.symlink_created or self.summary.additions or self.summary.deletions)
def to_dict(self) -> dict:
return {

View File

@ -246,6 +246,85 @@ def test_compare_snapshots_hides_sensitive_and_binary_file_content(tmp_path):
assert binary_change.diff_unavailable_reason == "binary"
@pytest.fixture
def symlink_support(tmp_path):
# Real symlink creation needs elevated privilege on stock Windows (no Developer
# Mode / admin); skip gracefully there instead of failing the whole run. Linux/CI
# and WSL create symlinks natively, so this exercises the real behavior there.
probe_link = tmp_path / "_symlink_probe"
try:
probe_link.symlink_to(tmp_path)
except (OSError, NotImplementedError):
pytest.skip("symlink creation is not permitted on this platform/user")
probe_link.unlink()
def test_compare_snapshots_classifies_symlink_replacing_file_as_symlink_created(tmp_path, symlink_support):
roots = _roots(tmp_path)
workspace = roots[0].host_path
outside_target = tmp_path / "outside-secret.txt"
outside_target.write_text("host-side content outside the workspace root\n", encoding="utf-8")
(workspace / "config.txt").write_text("original tracked content\n", encoding="utf-8")
before = scan_workspace_roots(roots)
assert before.files["/mnt/user-data/workspace/config.txt"].symlink is False
# Simulate an agent run doing: rm config.txt && ln -s <outside path> config.txt
(workspace / "config.txt").unlink()
(workspace / "config.txt").symlink_to(outside_target)
after = scan_workspace_roots(roots)
result = compare_snapshots(before, after)
changes = {change.path: change for change in result.files}
change = changes["/mnt/user-data/workspace/config.txt"]
assert change.status == "symlink_created"
assert change.status != "deleted"
assert change.symlink is True
assert change.symlink_target_after == str(outside_target)
assert change.diff_unavailable_reason == "symlink"
assert change.diff == ""
assert result.summary.symlink_created == 1
assert result.summary.deleted == 0
assert result.has_changes() is True
def test_scan_workspace_roots_captures_symlinks_as_metadata_only_stubs(tmp_path, symlink_support):
roots = _roots(tmp_path)
workspace = roots[0].host_path
outside_target = tmp_path / "outside-target.txt"
outside_target.write_text("outside content\n", encoding="utf-8")
(workspace / "link.txt").symlink_to(outside_target)
snapshot = scan_workspace_roots(roots)
file = snapshot.files["/mnt/user-data/workspace/link.txt"]
assert file.symlink is True
assert file.symlink_target == str(outside_target)
assert file.text is None
assert file.sha256 is None
assert file.content_unavailable_reason == "symlink"
def test_compare_snapshots_reports_removed_symlink_without_replacement_as_deleted(tmp_path, symlink_support):
# Scope boundary: a symlink that is genuinely removed with nothing taking its
# place is still "deleted" - only a symlink *newly occupying* a path (created or
# replacing a prior non-symlink) gets the distinct "symlink_created" status.
roots = _roots(tmp_path)
workspace = roots[0].host_path
outside_target = tmp_path / "outside-target.txt"
outside_target.write_text("outside content\n", encoding="utf-8")
(workspace / "link.txt").symlink_to(outside_target)
before = scan_workspace_roots(roots)
(workspace / "link.txt").unlink()
after = scan_workspace_roots(roots)
result = compare_snapshots(before, after)
changes = {change.path: change for change in result.files}
assert changes["/mnt/user-data/workspace/link.txt"].status == "deleted"
def test_compare_snapshots_truncates_large_text_diffs(tmp_path):
roots = _roots(tmp_path)
workspace = roots[0].host_path
@ -354,6 +433,7 @@ async def test_workspace_changes_response_is_empty_when_no_event_exists():
"created": 0,
"modified": 0,
"deleted": 0,
"symlink_created": 0,
"additions": 0,
"deletions": 0,
"truncated": False,

View File

@ -225,6 +225,9 @@ function unavailableLabel(
if (reason === "truncated") {
return t.workspaceChanges.truncatedUnavailable;
}
if (reason === "symlink") {
return t.workspaceChanges.symlinkUnavailable;
}
return t.workspaceChanges.diffUnavailable;
}

View File

@ -107,6 +107,7 @@ export const enUS: Translations = {
largeUnavailable: "Large file. Diff omitted.",
sensitiveUnavailable: "Sensitive path. Content hidden.",
truncatedUnavailable: "Diff omitted because the change set is too large.",
symlinkUnavailable: "Symlink change. Diff unavailable.",
truncatedSummary: "Some changes were truncated.",
},

View File

@ -90,6 +90,7 @@ export interface Translations {
largeUnavailable: string;
sensitiveUnavailable: string;
truncatedUnavailable: string;
symlinkUnavailable: string;
truncatedSummary: string;
};

View File

@ -106,6 +106,7 @@ export const zhCN: Translations = {
largeUnavailable: "文件过大,已省略 diff。",
sensitiveUnavailable: "敏感路径,已隐藏内容。",
truncatedUnavailable: "变更集过大,已省略 diff。",
symlinkUnavailable: "符号链接变更,无法展示 diff。",
truncatedSummary: "部分变更已被截断。",
},

View File

@ -8,7 +8,12 @@ export type WorkspaceChangeLineClass =
| "meta";
export function getChangedFileCount(summary: WorkspaceChangeSummary) {
return summary.created + summary.modified + summary.deleted;
return (
summary.created +
summary.modified +
summary.deleted +
summary.symlink_created
);
}
export function getWorkspaceChangeBadgeLabel(summary: WorkspaceChangeSummary) {
@ -38,9 +43,16 @@ export function getWorkspaceChangeLineClass(
}
export function sortWorkspaceChanges(files: WorkspaceFileChange[]) {
// `symlink_created` shares modified's rank: it is a distinct change kind
// (a symlink replacing a file) that otherwise renders like a modification
// (see StatusIcon / statusLabel). This `satisfies Record<...>` guard forces
// every WorkspaceChangeStatus variant to have a rank, so adding a new status
// without updating this table is a compile error instead of a silent NaN
// from an unranked lookup (which broke Array#sort's ordering contract).
const statusRank = {
created: 0,
modified: 1,
symlink_created: 1,
deleted: 2,
} satisfies Record<WorkspaceFileChange["status"], number>;

View File

@ -1,15 +1,21 @@
export type WorkspaceChangeStatus = "created" | "modified" | "deleted";
export type WorkspaceChangeStatus =
| "created"
| "modified"
| "deleted"
| "symlink_created";
export type DiffUnavailableReason =
| "binary"
| "large"
| "sensitive"
| "truncated";
| "truncated"
| "symlink";
export interface WorkspaceChangeSummary {
created: number;
modified: number;
deleted: number;
symlink_created: number;
additions: number;
deletions: number;
truncated: boolean;
@ -30,6 +36,9 @@ export interface WorkspaceFileChange {
diff_unavailable_reason: DiffUnavailableReason | null;
additions: number;
deletions: number;
symlink: boolean;
symlink_target_before: string | null;
symlink_target_after: string | null;
}
export interface WorkspaceChangesResponse {

View File

@ -49,6 +49,7 @@ test.describe("Workspace changes", () => {
created: 1,
modified: 1,
deleted: 0,
symlink_created: 0,
additions: 8,
deletions: 2,
truncated: false,

View File

@ -4,8 +4,36 @@ import {
getChangedFileCount,
getWorkspaceChangeBadgeLabel,
getWorkspaceChangeLineClass,
sortWorkspaceChanges,
} from "@/core/workspace-changes/summary";
import type { WorkspaceChangesResponse } from "@/core/workspace-changes/types";
import type {
WorkspaceChangesResponse,
WorkspaceFileChange,
} from "@/core/workspace-changes/types";
function makeFileChange(
overrides: Partial<WorkspaceFileChange> &
Pick<WorkspaceFileChange, "path" | "status">,
): WorkspaceFileChange {
return {
root: "workspace",
binary: false,
sensitive: false,
size_before: null,
size_after: null,
sha256_before: null,
sha256_after: null,
diff: "",
diff_truncated: false,
diff_unavailable_reason: null,
additions: 0,
deletions: 0,
symlink: false,
symlink_target_before: null,
symlink_target_after: null,
...overrides,
};
}
const changes: WorkspaceChangesResponse = {
available: true,
@ -14,6 +42,7 @@ const changes: WorkspaceChangesResponse = {
created: 1,
modified: 2,
deleted: 0,
symlink_created: 0,
additions: 12,
deletions: 3,
truncated: false,
@ -27,6 +56,25 @@ describe("workspace change summary helpers", () => {
expect(getChangedFileCount(changes.summary)).toBe(3);
});
test("counts a symlink replacing a file (the only change in a symlink-only run)", () => {
// Regression: getChangedFileCount previously summed only
// created + modified + deleted, so a run whose sole change was a
// symlink replacing a file (reported as `symlink_created`, not
// `deleted`) produced a count of 0 and the workspace-changes badge
// was hidden entirely -- the opposite of surfacing the change.
expect(
getChangedFileCount({
created: 0,
modified: 0,
deleted: 0,
symlink_created: 1,
additions: 0,
deletions: 0,
truncated: false,
}),
).toBe(1);
});
test("formats the compact badge label", () => {
expect(getWorkspaceChangeBadgeLabel(changes.summary)).toBe(
"3 files changed +12 -3",
@ -46,4 +94,35 @@ describe("workspace change summary helpers", () => {
expect(getWorkspaceChangeLineClass("+++foo")).toBe("addition");
expect(getWorkspaceChangeLineClass("---bar")).toBe("deletion");
});
test("ranks a symlink-created entry with modified files, not before created or after deleted", () => {
// Regression: statusRank previously had no "symlink_created" entry, so
// `statusRank[left.status] - statusRank[right.status]` was NaN for any
// comparison involving a symlink-created file -- violating Array#sort's
// consistency contract instead of producing a deterministic order.
const files = [
makeFileChange({ path: "z-deleted.txt", status: "deleted" }),
makeFileChange({ path: "m-symlink.txt", status: "symlink_created" }),
makeFileChange({ path: "a-created.txt", status: "created" }),
];
const sorted = sortWorkspaceChanges(files);
expect(sorted.map((file) => file.path)).toEqual([
"a-created.txt",
"m-symlink.txt",
"z-deleted.txt",
]);
});
test("breaks ties between modified and symlink-created entries by path", () => {
const files = [
makeFileChange({ path: "z.txt", status: "modified" }),
makeFileChange({ path: "a.txt", status: "symlink_created" }),
];
const sorted = sortWorkspaceChanges(files);
expect(sorted.map((file) => file.path)).toEqual(["a.txt", "z.txt"]);
});
});