Repository navigation
feat(read-file): record read scope and report clipping separately (U4, #1375) - #1913
easonLiangWorldedtech wants to merge 26 commits into
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 28 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (4)
📝 SummarySummary by CodeRabbit
WalkthroughThe change adds task-local file observations and read-completeness reporting. It adds staged text publication with backup handling, then updates ChangesFile read observations
Safe text and JSON writes
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant ReadFileTool
participant FileSystem
participant ObservationRegistry
ReadFileTool->>FileSystem: stat before read
ReadFileTool->>FileSystem: read file
ReadFileTool->>FileSystem: stat after read
ReadFileTool->>ObservationRegistry: record matching version token and completeness
sequenceDiagram
participant safeWriteJson
participant proper-lockfile
participant safeWriteText
participant FileSystem
safeWriteJson->>FileSystem: resolve lock key and publish target
safeWriteJson->>proper-lockfile: acquire lock
safeWriteJson->>safeWriteText: publish staged JSON with backup enabled
safeWriteText->>FileSystem: write, sync, and rename staged content
safeWriteJson->>proper-lockfile: release lock
Suggested reviewers: Merge Risk: 🔵 Low · up to The remaining concerns are a narrow write-failure edge case and a read-notice test gap. They do not establish a broad failure that blocks merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Read authorization remains intact, but JSON writes now follow file symlinks. A project configuration update can therefore modify configuration outside that project, including global tool-approval settings, when a workspace-controlled link points there. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (5 passed)
Full details: Regression EvidenceExplanation The new confinement check has an uncovered boundary case. Resolution Add a focused Full details: Security BoundariesExplanation
Resolution Pass the canonical workspace directory as Full details: Lifecycle Resource CleanupExplanation The new backup lifecycle can leave persistent orphan files. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
has-conflicts
Review statusThanks for contributing. This comment tracks the review sequence and the next action. Current step: Required CI passed. Waiting for automated review of the latest commit. If automated review does not start, a maintainer must restart it. Review-state labels are managed by this workflow; do not edit them manually. |
9774b0a to
60b9221
Compare
github-actions
Bot
removed
the
has-conflicts
…ve (U1, issue 1375) Split unit U1 of PR 1833. Three changes, each with a test that fails without it: - a caller-supplied staging path is checked for location and file type before anything is written, so an arbitrary path or a symlink cannot be published onto the target; - a failed parent-directory fsync on POSIX is reported as PostCommitDurabilityError instead of being swallowed, so a successful return never claims durability the filesystem did not grant; - the staged file and this write's own staging directory are released before RollbackFailureError is thrown. Focused coverage for resolveLockKey added: canonical parent directory, a dangling-link chain, and termination at the bounded depth on a two-link cycle.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
60b9221 to
bb64d87
Compare
|
@coderabbitai full review |
|
…ishTarget (U1, issue 1375) The resolver may fall back to the given path only when lstat also reports the path as absent. An EACCES or EIO failure says nothing about whether the path is a link, so falling back would publish through a link we were not allowed to inspect. Focused tests added for both branches.
bb64d87 to
6dd95ce
Compare
… type-sound
compile failed at the unit head on three points:
- RollbackFailureError needs a string backupPath, but the throw now happens after cleanup, so the
`string | null` narrowing was lost. The failure is now held as { error, backupPath }.
- The async lstat stand-in is built on the Stats prototype so it satisfies fsSync.Stats.
- The realpath/readlink mocks are typed to the real signatures; the readlink mock answers once
because only the link path is read.
tsc clean, 50 tests pass, ESLint --max-warnings=0 clean, no suppression change.
6dd95ce to
5bdf5b0
Compare
The any usage this entry covered is gone in the rewritten file, so the count drops 4 -> 3. eslint --prune-suppressions --max-warnings=0 confirms it.
The read tools record the observed on-disk version through task.observationRegistry, but the field was only declared in a later unit, so at this head the call dereferences undefined and the mocked e2e run fails on the read_file smoke tests. The registry is introduced by this unit, so the field belongs here. tsc clean on this unit, 11 observationRegistry tests pass, ESLint --max-warnings=0 clean.
The two any usages this entry covered are gone in the rewritten spec, so the count drops 98 -> 96. eslint --prune-suppressions --max-warnings=0 confirms it.
5bdf5b0 to
d7eab3d
Compare
The staging-identity guard compared the caller-staged file with the target through fs.lstat(targetPath).catch(() => null). Any error other than ENOENT - EACCES, ENOTDIR, ELOOP - was therefore indistinguishable from "there is no target", so a staging path that is a hard link of the target would pass the identity check, reach the commit rename, and let the failure handler unlink the only copy of the content. Only ENOENT now yields "no target"; anything else fails closed with a StagingPathError before anything is opened, staged or renamed. Test: the staging path resolves to a hard link of the target (same ino/dev) and the target lstat rejects with EACCES; the write is rejected with the comparison error and neither openSync nor rename is called.
|
Series alignment in 467cfda: the staging-identity guard no longer reads a failed target Regression test in Local: 85 passed / 4 skipped across @coderabbitai full review |
|
github-actions
Bot
added
coderabbit-review-active
The staging-identity guard read fs.Stats.ino/dev as JS numbers. On NTFS and ReFS those identifiers can exceed
Number.MAX_SAFE_INTEGER, and the rounding makes two different files look identical - a valid caller-staged file is
then rejected with StagingPathError (and a real alias could be missed). Both lstats now request { bigint: true } and
the comparison checks for bigint values before comparing them.
The spec stand-in carries bigint ino/dev, matching what lstat({ bigint: true }) returns at runtime (fs.BigIntStats is
a type-only export, so the double is documented at its single assertion).
github-actions
Bot
removed
coderabbit-review-active
The identity stand-in returns bigint identifiers regardless of the options, so the same-file tests could still pass if
either lstat dropped { bigint: true } - which is exactly the case that matters on NTFS/ReFS. Both tests now assert
that every identity lstat requested bigint stats.
The calls are filtered by their options rather than by path spelling: path.resolve prefixes a drive letter to
/tmp/... on Windows, so a path filter would only see one of the two reads on that platform.
…served On win32 the DACL of an existing target is saved before the commit rename so it can be reapplied afterwards. Two paths silently skipped that step and still published: - icacls /save failed (saved === false): the dump is cleaned up and the rename proceeds, so the new file inherits different access rights; - fs.access(targetPath) failed with something other than ENOENT (EACCES, ...): the catch treated "cannot check" as "target absent" and skipped DACL handling entirely. Both now report through a new onWarning sink (default console.warn): the write still proceeds - a missing or failing icacls must not leave the user unable to save, which is the documented fallback - but the caller is told the replacement may inherit different access rights instead of discovering it later. Tests: icacls save failure still commits the write and yields exactly one access-rights warning; an EACCES on the target yields the could-not-check warning and no icacls call. Verified as real regression tests - neutralizing the two warn calls makes both fail. 272 passed / 4 skipped locally; tsc and eslint clean.
|
Series alignment in 1331a91: a win32 replacement whose DACL could not be saved (failed |
|
✏️ Learnings added
|
github-actions
Bot
added
coderabbit-review-active
onWarning is documented as the sink for non-fatal safety notices, but delivery was not isolated from the write: an onWarning callback that threw propagated to the outer failure handler before fs.rename, turning a non-fatal notice into a failed save. The warn binding now catches callback failures and logs them. The restore-failure notice is routed through the same binding so a caller supplying onWarning receives it. Same fix as the u6 unit, kept aligned across the series. Local: safeWriteText 58/58 green; eslint clean, no suppression growth.
|
Series alignment with the u6 unit (#1915): Pushed as e412fce. Local: @coderabbitai full review |
|
github-actions
Bot
removed
coderabbit-review-active
proper-lockfile creates ${lockKey}.lock beside the lock key, and the key is the
symlink referent. A repository that plants .roo/mcp.json -> ~/.ssh/config
therefore made a confined write create a lock directory OUTSIDE the declared
scope, and when that directory was not writable the caller got a
lock-acquisition error after up to five retries instead of
ConfinedPathEscapeError. The confinement check now runs on the lock key before
acquireFileLock and is repeated on the resolved publish target inside the lock
(a peer writer may have moved the referent in between); both checks share
_assertWithinScope so they canonicalize identically.
Ported across the file-safety unit series so every unit that declares a scope
carries the same guarantee (same fix as fws/u3-observation-completeness).
|
Series alignment: the confinement check now runs before the advisory lock ( One ordering test added (a lock mock that throws if reached, no symlinks so it runs on every lane); |
github-actions
Bot
added
coderabbit-review-active
Series alignment for the two review findings fixed on fws/u6-apply-patch-wiring: 1. safeWriteText's warn wrapper could not catch a rejection from an async onWarning sink - TypeScript accepts a value-returning callback where a void one is expected - so the rejected promise was left unhandled, which under Node's default mode can end the process after a write that already succeeded. The wrapper now attaches a catch handler without awaiting (awaiting would let warning delivery delay a committed write, or stall it on a hung sink) and reports the rejection through the fallback sink. 2. safeWriteJson created the target's parent directory BEFORE the preflight confinement check, so a confined write to an out-of-scope path with a missing parent still created a directory outside confineTo. resolveLockKey and the check need no directory to exist, so the order is now lock key, confinement, mkdir; the in-lock check on the resolved publish target stays.
|
Series alignment with #1915: two review findings fixed here in
Tests ported; local suites green ( |
github-actions
Bot
added
coderabbit-review-active
This branch has not been deployed
1 participant
Split unit U4 of #1833, under the plan on this issue (5993969784 / 5994039786 / 5994053776). Base is U2 (#1912) per the merge order.
Scope (one gate scope): the read side — what a read records about its own scope, and reporting truncation and clipping as two separate notices.
Content source of record:
kind: commit, base7c291bb08→ head6768ccfaf, replayed on the current main tip9af61f87eso this branch carries nothing that main already has.Budget (own delta, not the stacked view): 932 a+d / 105 changed executable lines. Inside both caps.
Verification at this head: 94 passed (readFileTool) and 48 passed (indentation-reader); ESLint
--max-warnings=0clean on every file in the unit; Prettier clean;src/eslint-suppressions.jsonnever increased.The GitHub view also carries the unmerged base, so the numbers above are this unit's own delta.