fix(#4410): read workspace diff content uncapped + symlink-safe (Codex gate)
Codex regression gate found that routing workspace reads through read_file_content() introduced a 400KB MAX_FILE_BYTES cap not present on master (Path.read_text), so a large but legitimate workspace file raised ValueError -> caught -> None -> the rollback diff falsely reported it as DELETED. Replace with an uncapped anchored O_NOFOLLOW read (safe_resolve_ws + open_anchored_fd + S_ISREG fstat check) that keeps the symlink-escape protection but has no size cap. Adds a >400KB regression test (RED on the capped path, GREEN with the fix). Co-authored-by: Hinotoi-agent <Hinotoi-agent@users.noreply.github.com>
This commit is contained in:
1 parent
35f040d19f
commit
94909590ad
2 files changed
+52
-7
No files matched your search
+27
-7
@@ -153,17 +153,37 @@ def _read_checkpoint_text(git: str, ckpt_dir: Path, modes: dict[str, int], rel_p
|
||||
|
||||
|
||||
def _read_workspace_text(workspace_root: Path, rel_path: str) -> str | None:
|
||||
"""Read workspace text through the anchored workspace file API.
|
||||
"""Read workspace text symlink-safely without the size cap of read_file_content.
|
||||
|
||||
The normal workspace helpers reject traversal and symlink escapes. Treat
|
||||
invalid or unreadable files as absent so the diff does not imply that the
|
||||
workspace contains an empty file.
|
||||
Resolves the path under the workspace (rejecting traversal/symlink escapes)
|
||||
and opens it through an anchored, O_NOFOLLOW file descriptor so a symlink
|
||||
component cannot redirect the read outside the workspace boundary. Unlike
|
||||
``read_file_content``, this has no MAX_FILE_BYTES cap — a large but legitimate
|
||||
regular file must render as *modified*, not be silently dropped (which would
|
||||
make the rollback diff falsely report it as *deleted*). Treat missing /
|
||||
invalid / escape / non-regular paths as absent (None).
|
||||
"""
|
||||
from api.workspace import read_file_content
|
||||
from api.workspace import open_anchored_fd, safe_resolve_ws
|
||||
|
||||
try:
|
||||
return read_file_content(workspace_root, rel_path)["content"]
|
||||
except (FileNotFoundError, OSError, ValueError):
|
||||
target = safe_resolve_ws(workspace_root, rel_path)
|
||||
except (ValueError, OSError):
|
||||
return None
|
||||
try:
|
||||
fd = open_anchored_fd(workspace_root, target, want_dir=False)
|
||||
except (FileNotFoundError, ValueError, OSError):
|
||||
return None
|
||||
try:
|
||||
st = os.fstat(fd)
|
||||
if not stat.S_ISREG(st.st_mode):
|
||||
return None
|
||||
with os.fdopen(fd, "rb") as fh:
|
||||
return fh.read().decode(errors="replace")
|
||||
except OSError:
|
||||
try:
|
||||
os.close(fd)
|
||||
except OSError:
|
||||
pass
|
||||
return None
|
||||
|
||||
|
||||
|
||||
@@ -100,3 +100,28 @@ def test_restore_checkpoint_reads_git_blob_after_worktree_symlink_swap(tmp_path,
|
||||
assert "POST_COMMIT_SECRET_MARKER_SHOULD_NOT_COPY" not in (workspace / "file.txt").read_text(
|
||||
encoding="utf-8"
|
||||
)
|
||||
|
||||
|
||||
def test_checkpoint_diff_renders_large_workspace_file_as_modified_not_deleted(tmp_path, monkeypatch):
|
||||
"""Regression (Codex gate): a large but legitimate workspace file (> the
|
||||
read_file_content MAX_FILE_BYTES 400KB cap) that differs from the checkpoint
|
||||
must render as MODIFIED, not be silently dropped to None and reported as
|
||||
DELETED. The symlink-safe workspace reader must have no size cap.
|
||||
"""
|
||||
workspace, ckpt_dir, checkpoint = _init_checkpoint(tmp_path, monkeypatch)
|
||||
# Checkpoint has a small version of the file.
|
||||
_commit_checkpoint_file(ckpt_dir, "big.txt", "old small checkpoint content\n")
|
||||
# Workspace has a LARGE (> 400KB) modified version of the same file.
|
||||
big_content = "X" * (500 * 1024) + "\nMODIFIED_LARGE_MARKER\n"
|
||||
(workspace / "big.txt").write_text(big_content, encoding="utf-8")
|
||||
|
||||
result = rollback.get_checkpoint_diff(str(workspace), checkpoint)
|
||||
|
||||
# The file must appear in the diff as a change, NOT be treated as deleted.
|
||||
assert "big.txt" in result["diff"]
|
||||
statuses = {f["file"]: f.get("status") for f in result["files_changed"]}
|
||||
assert statuses.get("big.txt") == "modified", (
|
||||
"a large modified workspace file must render as modified, not deleted "
|
||||
"(the capped read_file_content path regressed this): got "
|
||||
f"{statuses.get('big.txt')!r}"
|
||||
)
|
||||
Reference in new issue
Block a user