Repository navigation
Bind edit_file writes to verified inode across symlink swaps - #501
Conversation
sambitcreate
changed the title
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The native write closes the reported symlink-swap path, but its new helper lookup breaks edit_file for CLI Bots launched outside the repository.
Reviewed changes Reviewed the parent edit path, native descriptor-relative commit, helper distribution, tests, and documented inode-confinement boundary through e4655822d.
- Verified-inode writes: Parent edits pass exact root/file identities and a content hash to a no-follow native walk, preserving in-place writes and stable in-root symlinks while rejecting multiply linked or oversized files.
- Helper distribution and CI: Refreshed macOS/Linux prebuilts and manifests, plus native-build prerequisites for coding-tool test lanes.
- Behavioral validation: Coding-tool/router tests passed 69/69, native mutator tests passed 30 with three platform/environment skips, and manifest/CI-registry tests passed 20/20; an external-CWD reproduction confirmed the inline CLI regression.
- Security boundary: Reviewed the explicit limitation that a concurrently moved original inode may still be modified through its verified descriptor.
gpt-6.1-sol ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Reviewed the CLI helper-resolution fix since the prior review at e4655822d.
- Passed the installed helper through Bot tools: Derived the native mutator path from
AIDEN_CLI_ENTRYand threaded it through the router and pinned coding-tool factory, preserving the verified-inode write path independently of the project directory. - Added packaged regression coverage: Exercised a successful Bot edit from an external directory containing an executable decoy at the old fallback path, and verified that the decoy was not executed.
Validation passed: fresh CLI build, all 99 CLI tests, all 69 coding-tool/router tests, and root and CLI type-checks. The prior helper-lookup finding is addressed; hosted CI on the exact head remains a separate merge gate.
gpt-6.1-sol ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes Reviewed the merge integration and verification updates since the prior review at a4535dfc.
- Integrated literal replacement: Combined main's
indexOf/slice replacement with the verified-inode native commit, preserving literal JavaScript replacement markers without weakening the symlink-swap protection. - Updated verification records: Documented the merged behavior and local results, including the independently observed glob comparison failure.
Validation passed: six focused edit tests, eight Bot router tests, twenty helper-manifest/CI-inventory tests, and root type-check. The prior finding remains resolved. Full CLI and coding-tools suites were not repeated in this incremental review; exact-head hosted CI remains a separate merge gate.
gpt-6.1-sol ｜ 𝕏
Hermes Review BotConfidence: 5 Engine: SummaryResolves issue #423 by binding Confidence Score: 5/5Full end-to-end trace completed across the TypeScript caller in 📁 Important Files Changed
FindingsNo findings. Sequence DiagramsequenceDiagram
autonumber
participant Tool as coding-tools (TS)
participant FS as Node fs (Handle)
participant Helper as native mutator (edit-file)
participant VFS as OS VFS / Kernel
Tool->>FS: fs.open(canonicalPath, O_RDONLY | O_NOFOLLOW | O_NONBLOCK)
FS->>Tool: FileHandle
Tool->>FS: handle.stat({ bigint: true }) & handle.readFile()
Note over Tool: Verify nlink == 1 & calculate SHA-256
Tool->>FS: handle.close()
Tool->>Helper: spawn(edit-file, root, dev/ino, relPath, file dev/ino, sha256, len) via stdin
Helper->>VFS: open(root, O_RDONLY | O_DIRECTORY | O_NOFOLLOW)
VFS-->>Helper: root_fd (verify root dev/ino)
loop For each path component
Helper->>VFS: openat(current_dir_fd, component, O_DIRECTORY | O_NOFOLLOW)
end
Helper->>VFS: openat(parent_dir_fd, leaf, O_RDWR | O_NOFOLLOW | O_NONBLOCK)
VFS-->>Helper: file_fd
Note over Helper: Verify dev/ino, nlink == 1, pread SHA-256 == expected, and fstatat(leaf) == before
Helper->>VFS: ftruncate(file_fd, 0)
Helper->>VFS: write_all(file_fd, stdin_bytes)
Helper->>Tool: stdout "edited\n" (exit 0)
Tool-->>Tool: Return fileMutationResult
[]
|
Fixes #423.
Change
edit_filecommits.edit_fileworks from external project directories and never executes a project-providedbuild/nativebinary.Security boundary
This addresses #423's symlink/path swap to a different outside inode. Once the helper has opened and verified the original file, a local actor can still rename that same inode or its parent outside the root before the descriptor write. The helper would modify the moved original inode. It therefore does not guarantee that every file modified remains inside the workspace for the entire operation. A broader guarantee needs control over concurrent namespace mutation; another pathname recheck or a held parent descriptor cannot provide it.
Local verification
edit_fileafter changing to an external project directory that contains an executable decoy at the old fallback path, and verifies the decoy was not run. Bot file router suite passed 8/8.The complete
coding-tools.test.tsrun after #492 merged passed 61 of 62 tests; the second merge did not touch coding-tools code or tests, and its focused edit/literal tests passed 5/5. The independentforeground glob agrees with native glob on normal patterns, hidden paths and safe linkstest failed for**/*: the foreground result descended intolink/, while Node's native glob did not. An earlier focused rerun afternpm cireproduced the same mismatch. The edit and literal-replacement tests passed on both merged heads. This is a local test result; hosted checks on the exact new PR head are pending.PR #492 merged to
mainas c68462b, followed by #491 at dd2c0d0, #497 at 17d51d4, #498 at 6bc994a, and the shared E2E baseline fix #506 at 896f31a. This branch merged eachorigin/mainhead without rebasing. The resultingedit_fileuses #492's literalindexOf/slice replacement and #423's descriptor-bound native commit; focused tests cover both. PR #504 remains stacked on this branch.Exact-head E2E result
Electron E2E shard 3, run 38825795960 failed on head
b5a6ba37e5937f47c1ad893795cb088700e40612:tests/e2e/draft-agent-chats.spec.ts:205failed on both attempts. After the test clicks the Second workspace header, it moves the pointer away and expects its “New chat” button to have opacity0; it remained1. The header can retain keyboard focus, and the button intentionally usesgroup-focus-within/workspace:opacity-100. Both the test and component matched the then-currentmainand originated in Hybrid new chat: workspace choice, progressive composer context, worktrees on send #491. The same opacity failure repeated on Wait for managed-worktree terminals before deletion #500 and cleanmain, confirming a shared baseline mismatch rather than a Bind edit_file writes to verified inode across symlink swaps #501 edit-path integration change.tests/e2e/chat-top-bar.spec.ts:236and:327initially failed because[data-worktree-send-failure]was absent within 20 seconds. Both passed on the single built-in retry and are recorded as flaky, as required by the repository's flaky-test policy. These tests and their UI came from Hybrid new chat: workspace choice, progressive composer context, worktrees on send #491 and are unchanged by Bind edit_file writes to verified inode across symlink swaps #501.The run's uploaded safe receipt reports
electron-e2e/test-failedand deliberately omits screenshots and traces. No timeouts, retries, or assertions were weakened. That workflow was still active whenmainadvanced through #497; the failed job was not rerun on the stale head. After merging #497, focused edit/literal tests passed 5/5, CLI Bot parity passed 1/1, and root type-check passed. The separate #506 change now moves focus out of the row before asserting the hidden state. After merging it, the corrected sidebar test and both previously flaky worktree-send specs passed twice each (6/6) locally with--fail-on-flaky-tests. Production build, root and E2E type checks, focused edit tests, and CLI Bot parity also passed. The new head requires its own CI.Hosted CI note
Model catalog contracts, run 38812053863 failed during
npm ciwithECONNRESET; its verification step was skipped. This is an install/network failure, not a model catalog test failure. That run was on an earlier PR head. The latest pushed head isa616fc4bda6ea409ca4c5f991e1bbbe098772d87; its checks are the merge gate.