feat(editor): route the diff-view save through the guard (U8, #1375) - #1916
Open
easonLiangWorldedtech wants to merge 22 commits into
Open
easonLiangWorldedtech wants to merge 22 commits into
easonLiangWorldedtech wants to merge 22 commits into
Conversation
easonLiangWorldedtech
requested review from
JamesRobert20,
edelauna,
navedmerchant and
taltas
as code owners
October 5, 2026 12:13
Contributor
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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
Contributor
Review statusThanks for contributing. This comment tracks the review sequence and the next action. Current step: Fix the failing required CI checks; awaiting-maintainer requires CI and automated review completion. Review-state labels are managed by this workflow; do not edit them manually. |
This was referenced Oct 5, 2026
easonLiangWorldedtech
force-pushed
the
fws/u8-diffview-guarded-save
branch
from
October 5, 2026 12:35
ed27ffe to
a7df0c2
Compare
…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.
easonLiangWorldedtech
force-pushed
the
fws/u8-diffview-guarded-save
branch
from
October 5, 2026 12:55
a7df0c2 to
a6a3ce3
Compare
…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.
easonLiangWorldedtech
force-pushed
the
fws/u8-diffview-guarded-save
branch
from
October 5, 2026 13:16
a6a3ce3 to
2d6d158
Compare
added 2 commits
October 5, 2026 22:30
… 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.
easonLiangWorldedtech
force-pushed
the
fws/u8-diffview-guarded-save
branch
from
October 5, 2026 14:39
2d6d158 to
d749d72
Compare
added 6 commits
October 5, 2026 22:47
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.
easonLiangWorldedtech
force-pushed
the
fws/u8-diffview-guarded-save
branch
from
October 5, 2026 14:52
d749d72 to
5e72ea6
Compare
8 tasks done
github-actions
Bot
removed
the
has-conflicts
U6's ApplyPatchTool calls saveChanges with the writeKind argument, so the parameter must exist before U6 can build. U8 owns that signature, so U8 now lands before U6.
easonLiangWorldedtech
force-pushed
the
fws/u8-diffview-guarded-save
branch
from
October 5, 2026 15:13
5e72ea6 to
45b7912
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Same four apply_diff tests 404 on the mock server here while the identical code passes e2e at U7 and U9.
This was referenced Oct 5, 2026
added 2 commits
October 6, 2026 02:49
This unit was rebuilt from the pre-fix content source, so its copy of safeWriteText.ts still restored the backup over a write whose commit rename had already succeeded when the parent-directory fsync failed. Unit 1 (fws/u1-atomic-publish) already fixes this. Taking the file here keeps the shared code byte-identical across the units, so merging the chain in order does not overwrite unit 1's fix. Tests: 52 passed in safeWriteText.spec.ts.
Contributor
Author
|
@coderabbitai full review |
Contributor
|
added 7 commits
October 6, 2026 03:44
safeWriteJson rethrows RollbackFailureError and the telemetry callers record only error.message, so the generic wrapper message lost the filesystem errno text of the publish failure. Include the publish error message while keeping the rollback context (publishError, rollbackError, backupPath) unchanged. Tests: 52 passed in safeWriteText.spec.ts, ESLint clean with --max-warnings=0.
… rollback failure The assertion accepted any rmdir argument, so a regression that removed a different directory still passed. Read this write's own staging directory from fsSync.mkdirSync and assert that exact path. Tests: 52 passed in safeWriteText.spec.ts, ESLint clean with --max-warnings=0.
…the Actions queue, no source change
…the last green run on these files
…is branch and no source changed
The dump path was the fixed sibling <target>.acl.tmp. A pre-existing user file at that path is unlinked by the failed-save branch and by both cleanup paths, and two concurrent writes to the same target share one dump, so one write can restore or delete the other's. Use the per-write unique name like the staging file. Tests: 52 passed in safeWriteText.spec.ts, ESLint clean with --max-warnings=0.
…he other eight units of the same chain with the same guard behaviour
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split unit U8 of #1833, under the plan on this issue (5993969784 / 5994039786 / 5994053776). Base is U8 (#1918) per the merge order.
Scope (one gate scope): the interactive save path —
saveChanges()publishes through the guard, a rejected save cleans up only its own placeholder and tab, and one teardown path owns a cancelled save.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): 2542 a+d / 486 changed executable lines. 2542 a+d is above the 1000 hard cap — documented deviation: the file's 2056-line spec is a single file whose tests are interleaved across the behaviours, and splitting it would move tests away from the behaviour they prove.
Verification at this head: 130 passed; 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.