feat(mcp): add physical file read evidence - #554
Conversation
zzet
left a comment
There was a problem hiding this comment.
CI is red, and it reproduces locally.
TestReadFilePhysicalEvidenceHashesFullDiskBuffer fails on macOS:
expected: "/var/folders/.../001/evidence.txt"
actual : "/private/var/folders/.../001/evidence.txt"
The test asserts resolved_path == filepath.Join(dir, ...) while the implementation returns the symlink-resolved path. The Ubuntu job shows fail in gh pr checks, but its Test step was cancelled by matrix fail-fast — macOS is the only genuine failure. Everything else is clean: only that one test fails in the whole internal/mcp package, and golangci-lint is green.
The one-line test fix (EvalSymlinks the expectation) is trivial, but it surfaces a real design bug:
- symlink_resolved doesn't mean what it says. It's EvalSymlinks(abs) != abs, so it's true whenever any ancestor component is a link — every file under /tmp or /var on macOS, any repo under a symlinked home. The issue asked to "report link vs target"; this reports "something in the path was a link", which a caller can't use to decide whether the file itself is a symlink. os.Lstat(absPath).Mode()&os.ModeSymlink is the fact being asked for.
The most subtle new code has zero coverage. samePhysicalFileVersion plus the double-read / re-stat / re-EvalSymlinks drift detector is the safety argument for the feature, and nothing exercises it. Issue #545 explicitly lists "same-size content drift, symlinks/junctions, and files changing during observation" as acceptance criteria. The six tests cover happy path, window, binary, empty, ETag stability, arg validation, and facade publication — all the easy parts.
It reads the file twice, against an explicit constraint in the issue ("Hash from one stable buffer/file handle where possible so the bytes are not read twice"). readPhysicalFileEvidence does ReadAll → seek → ReadAll and holds both buffers: 2× peak memory and 2× I/O per evidence read, uncapped. The stat-triple already covers the ordinary case, and the byte-compare only closes a window that reopens the moment the function returns.
content_truncated gets its meaning overwritten. In the base path it means "capped at max_chars", paired with a max_chars field and a matching omission note. Under physical_evidence it's reassigned to "returned bytes differ from hashed bytes in any way" — windowing, binary, redaction, and elision all set it, with no max_chars companion (the PR's own binary test asserts this). same_buffer_as_content already carries that meaning; the existing field shouldn't be repurposed.
|
Addressed the requested redesign in f8eaa39:
Local verification:
All passed. |
zzet
left a comment
There was a problem hiding this comment.
Request changes. The revision resolves the earlier one-buffer, truncation, macOS path, and direct-symlink concerns; all GitHub checks are green, the full internal/mcp package and focused race tests pass, and the branch merges cleanly with current main. However, adversarial tests on this exact head reproduce two failures in the core physical-evidence guarantee: repository confinement can be bypassed by retargeting a symlink around the read, and same-size content drift is accepted when mtime is restored. A repository-local FIFO also blocks before rejection, redacted secret content still exposes its raw digest, and invalid UTF-8 is incorrectly labeled as the same returned buffer. These must fail closed before merge. Keep #545 open; mutation receipts, Windows junction coverage, and contract documentation remain follow-up scope.
| return mcp.NewToolResultError(readErr.Error()), nil | ||
| } | ||
| // Close the symlink-retargeting window around the stable physical read. | ||
| if guardErr := s.guardSymlinkWithinRepo(absPath); guardErr != nil { |
There was a problem hiding this comment.
High — bind confinement to the target actually hashed. This post-read guard re-resolves absPath, not physicalEvidence.resolvedPath. I reproduced the pre-guard seeing an inside target, the helper reading and hashing a stable outside target, then the post-guard succeeding after the link was restored inside. Validate the observed resolved target against the resolved repository roots before returning evidence, ideally using handle-relative/no-follow primitives, and add an out-and-back retarget regression.
|
|
||
| func samePhysicalFileVersion(a, b os.FileInfo) bool { | ||
| return a != nil && b != nil && a.Mode().IsRegular() && b.Mode().IsRegular() && | ||
| os.SameFile(a, b) && a.Size() == b.Size() && a.ModTime().Equal(b.ModTime()) |
There was a problem hiding this comment.
High — this version check can certify stale bytes. Rewriting before to same-length after! in the existing afterRead hook and restoring the original mtime makes inode, size, and mtime all compare equal, so the helper succeeds and reports disk_verified=true although the path now contains different bytes. Use a stronger change signal where available or a verification strategy that cannot be bypassed by preserved/coarse timestamps, and add the restored-mtime regression.
| if err != nil { | ||
| return nil, physicalReadEvidence{}, fmt.Errorf("could not resolve physical file: %w", err) | ||
| } | ||
| f, err := os.Open(absPath) |
There was a problem hiding this comment.
Fail closed before opening special files. os.Open on a FIFO blocks waiting for a writer; the regular-file check occurs only afterward. My bounded regression hung until it supplied a writer, then received the intended error. Reject non-regular targets before a blocking open and use a race-safe/nonblocking open plus fstat so FIFOs, devices, and sockets cannot stall or cause side effects.
| result["resolved_path"] = physicalEvidence.resolvedPath | ||
| result["file_kind"] = "regular" | ||
| result["byte_count"] = physicalEvidence.byteCount | ||
| result["content_sha256"] = physicalEvidence.contentSHA256 |
There was a problem hiding this comment.
Do not bypass the secret-read boundary with a raw digest. I verified that a .env password is replaced with the redaction marker while this field still returns the SHA-256 of the unredacted disk bytes and folds it into the ETag. That is an offline guess-verification oracle for low-entropy secrets. Require explicit allow_secrets intent for the physical digest, or omit/refuse raw-derived evidence whenever redaction fires.
| result["hash_source"] = "disk" | ||
| result["content_source"] = contentSource | ||
| result["disk_verified"] = true | ||
| result["same_buffer_as_content"] = !contentAltered |
There was a problem hiding this comment.
same_buffer_as_content must reflect wire bytes. For a file containing invalid UTF-8 bytes ff fe and no NUL, looksBinary is false and this reports true, but JSON serialization replaces those bytes. Base this claim on actual lossless representability/byte equality, or encode non-text content losslessly, and add an invalid-UTF-8-without-NUL regression.
Summary
physical_evidenceanddigestparameters toread_fileand theread.filefacade schemaresolved_path,byte_count,content_sha256, hash scope/source, content source, symlink state, observation time, and same-buffer status)Scope
This is the read-side delivery slice of #545. Mutation receipts remain a separate follow-up, so this PR references rather than closes the issue.
Refs #545
Verification
go test ./internal/mcp -run '^TestReadFilePhysicalEvidence' -count=1go test -race ./internal/mcp -run '^TestReadFilePhysicalEvidence' -count=1go vet ./internal/mcpgo build ./cmd/gortexgit diff --cached --checkgo test ./internal/mcp -count=1was also attempted on Windows but exceeded the 5-minute local timeout without emitting a test failure. The focused, race, vet, and build checks above completed successfully.