Repository navigation
feat(tools): publish apply_patch through the guard (U6, #1375) - #1915
Open
easonLiangWorldedtech wants to merge 98 commits into
Open
easonLiangWorldedtech wants to merge 98 commits into
easonLiangWorldedtech wants to merge 98 commits into
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR routes the
apply_patchtool through the guard, so a patch hunk can no longer "write" a file the model never fully read: an unobserved overwrite and a stale version token are refused with the standard remediation. A move carries the source's completeness to its destination instead of claiming completeness for lines the model never read, and a partial-source move onto an already-observed destination is rejected before any state changes. It also fixes a resource leak: when the guard refuses a publish,saveDirectlynow rolls back the parent directories it created.It is the sixth of nine PRs for epic #1375 (chain tracked on #1991).
Related GitHub Issue
Closes: #2021 (this unit's tracking issue).
Refs: #1991 (chain tracking), #1375 (epic).
Description
What this PR changes:
apply_patchpublishes viaguardedWrite, so an unobserved overwrite and a stale version token are rejected with the read-first / re-read-then-retry remediation instead of clobbering the file.completeflag, so a slice, range, truncated, or indentation-block source never authorizes a full-file replacement at the new path.saveDirectlycaptures the listcreateDirectoriesForFilereturns and, if the guard rejects, removes those directories innermost-first withrmdir(which refuses a directory another writer populated, so the loop stops at the first failure) and rethrows the original write error.AncestorReplacedErrorif an ancestor was swapped underneath it.Why the diff looks bigger than the change: the chain is non-cumulative - this branch is based on
mainand carries its own copy of the earlier units' content, which the GitHub diff shows as context.Review focus:
src/core/tools/ApplyPatchTool.ts, the guard wiring insrc/core/tools/guardedWrite.ts, the rollback path insrc/integrations/editor/DiffViewProvider.ts, and the specs named below.Test Procedure
From the repository root, with dependencies installed:
Environment: Node 22+, Linux/macOS/Windows CI runners (the guard's platform-specific branches are exercised through the injected
platformoption, not a real Windows host).Pre-Submission Checklist
.changesetor CHANGELOG changes (AGENTS.md).src/eslint-suppressions.jsonunchanged - no suppression count increased.--max-warnings=0) rather than relying on suppressions.Visual Snapshots
Not applicable - no UI change.
Videos (interaction / animation only)
Not applicable.
Documentation Updates
None. No user-facing setting, command, or documented behavior string changes; the guard's remediation text is already introduced in earlier units. No
.changesetand no CHANGELOG edit (AGENTS.md).Additional Notes
DiffViewProvider.tschange stashed it fails.Verification at this head
integrations+core/tools+activate- 1340 passed / 17 skipped across 60 files;tsc --noEmitclean; eslint clean on the changed files with no suppression-count increase.Updated this round
Get in Touch
Reviewer and maintainer questions are welcome as comments on this PR or on #1991.
Stryker directive removal (equivalence notes)
Per AGENTS.md the two Stryker suppression comments in the guarded save
path were removed instead of kept; their equivalence claims:
"update"->""): an empty kinddispatches exactly like
"update"inside guardedWrite - only"edit"and
"create"branch distinctly - and the default-parameter line isnot part of this unit's changed-line selector, so the mutant is
equivalent / out of scope.
GuardRejectedErroris only thrown by guardedWrite, which is reachedonly after the
!saveTaskcheck above has already thrown. Wheneverthe operand is evaluated
saveTaskis provably non-null, so droppingthe operand cannot change which branch is taken.
mutation-diff remains advisory for stacked units; maintainers decide any
narrow gate-config exclusion.
Re-review round (1e8b458)
.local/state/gh/device-id, gitignored,swept in by an add -A during a refresh). No other
.local/strays on anyunit branch or main.
as anycleanup inDiffViewProvider.spec.ts: bracketaccess for private members + documented unknown-based structural casts for
VS Code doubles; suppression 310 -> 285.
console.errormust carry[Original Error Caught]with the commit-failureobject as its second argument, and no call may carry the cleanup-error text.
Re-review responses (error-fix re-review wave)
answered by completing the source unlink and reporting unlink failures as
PartialMoveError- since SUPERSEDED by the crash-availability reversalbelow; see the next section for the shipped semantics.
Crash-availability reversal (durability-error move)
A follow-up review (Major Data-Integrity) reversed the previous round's
guidance for the same branch, and the new semantics shipped: when the
destination's commit rename landed but its directory entry is not known to
be durable (
PostCommitDurabilityError), the source is KEPT and thedurability error propagates - unlinking it could leave NEITHER path
available after a crash. The duplicate is visible to the model and
recoverable; a file lost to the crash is neither, so crash availability
outranks not leaving the content at both paths. The try/catch is gone (the
error propagates naturally) and the durability test now asserts the source
unlink was NOT called; the unlink-failure test from the superseded round was
removed with its unreachable scenario. Negative control: restoring the
unlink reddens exactly the kept-source test.
Move deletion bound to the recorded observation (440dca4)
The Persistence Integrity row demanded the source deletion prove the
source is still the file the observation authorized. The move now
deletes under the shared advisory lock (
withFileLockon the canonicallock key): the source must still be present in the observation registry
and its recorded version must equal a fresh bigint stat of the source,
otherwise the move raises
PartialMoveErrorbefore any unlink. A statfailure is likewise a
PartialMoveError- an unreadable source is not adeletable source. Tests: the never-observed case now asserts no unlink
and no destination tracking; three new tests pin the changed-source
rejection, the vanished-source rejection, and that the deletion runs
under the source's lock key. 26/26 green; negative controls (faithful
mutants of each check) each redden exactly one test.
Durability note: on the durability-error path the source is kept
outright -
PostCommitDurabilityErrorpropagates before the deletion isreached - the strongest form of the protected-deletion contract this
unit's issue asks for.
Security Boundaries disposition (warn-only DACL). The row asks for
the DaCl restore failure to fail closed. That change belongs to the
publish-primitive owner (PR 1916, fixed there in 3624e4f), which
merges before this unit in the declared order; this unit carries the
same carrier copy and reconciles at rebase (record 6106959976 on issue
1408; tracking 1991). Implementing it twice here would fork the fix.
Walkthrough carry-over evidence (04:34 walkthrough vs head 440dca4)
The 04:34 walkthrough still carries the Persistence Integrity and
Security Boundaries ERROR rows whose claims predate this head (pushed
2026-10-16T03:55Z). The PI row's raw-unlink line numbers belong to the
superseded head
993fc6f; at this head the deletion is lock-serializedand observation-bound exactly as the Resolution demands (missing,
changed, vanished, or unobserved sources are kept;
PartialMoveErrornames the committed destination and remaining source; the durability
path keeps the source outright; tests and negative controls above). The
SB row re-flags the carrier copy's warn-only DACL restore, whose
fail-closed fix lands at the publish-primitive owner (PR 1916,
3624e4fea) that merges before this unit; copies reconcile at rebase(record 6106959976 on issue 1408). Evidence comment posted on the pull
request; no code change this round.