U9 re cut, rung 2 of 4: guarded write core and its recovery readers (416 counted lines, 57 selector ranges, headroom 84) - #1917

Summary by CodeRabbit

• New Features
  • File updates support atomic publishing, optional backups, and preservation of existing file permissions.
  • File-reading results can indicate when returned lines have been clipped.
  • File changes can be checked against the version previously read, with successful updates refreshing that version.
• Bug Fixes
  • Writes resolve symlink targets and enforce configured workspace boundaries to help prevent unintended changes outside allowed areas.
  • Writes based on outdated file contents are rejected, and failed writes are less likely to leave temporary files behind.

The PR adds observation-based guarded writes, atomic text and JSON publication through resolved paths, and clipped-line reporting in indentation reads. It also updates recovery locking and lock-key tests.

Changes: Observed versions and guarded writes

Layer / File(s)	Summary
Manage task observations src/core/task/observationRegistry.ts, src/core/task/__tests__/*	The observation registry adds forget. Tests cover removal, preservation of other entries, and registry isolation between tasks.
Authorize and bind write targets src/core/tools/guardedWrite.ts, src/core/tools/__tests__/guardedWrite.spec.ts	Guarded writes add lexical and canonical workspace checks. Approved outside-workspace writes require a pre-approval canonical target identity.
Check versions and publish writes src/core/tools/guardedWrite.ts, src/core/tools/__tests__/guardedWrite.spec.ts, knip.json	Create, update, and edit writes use observation and version checks, per-path FIFO ordering, cancellation checks, and post-write observation refresh.

Atomic text and JSON publication

Layer / File(s)	Summary
Resolve targets and stage text writes src/services/file-safety/safeWriteText.ts, src/services/file-safety/__tests__/safeWriteText.spec.ts	The text writer resolves publication targets and lock keys, validates staging paths, and writes staged content while preserving target modes. Tests cover path resolution, staging, and byte handling.
Commit text writes and handle durability src/services/file-safety/safeWriteText.ts, src/services/file-safety/__tests__/*	The text writer adds optional backups, atomic publication, directory syncing, Windows DACL handling, warnings, and cleanup. Tests cover commit and durability outcomes, including real-filesystem cases.
Resolve, confine, and publish JSON targets src/utils/safeWriteJson.ts, src/utils/__tests__/safeWriteJson*	The JSON writer checks canonical scope confinement and uses resolved targets for locking, merging, staging, and publishing. It preserves target modes and best-effort syncs the parent directory after publication.
Align recovery and lock-key handling src/utils/jsonFileRecovery.ts, src/utils/__tests__/safeWriteJson*, src/core/task-persistence/__tests__/TaskHistoryStore.deleteSemantics.spec.ts	Recovery and deletion helpers lock by resolved keys while continuing to operate on caller-supplied paths. Tests cover lock-key resolution, interruption matching, and history deletion lock identity.

Clipped-line reporting

Layer / File(s)	Summary
Add clipped-line result state src/integrations/misc/indentation-reader.ts, src/integrations/misc/__tests__/indentation-reader*.spec.ts	Indentation reads report whether returned lines were clipped. Invalid reads and empty results report no clipped lines.

Estimated code review effort: 4 (Complex) | ~60 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant guardedWrite
  participant TaskWorkspace
  Caller->>guardedWrite: submit path and write options
  guardedWrite->>TaskWorkspace: check workspace containment and target identity
  TaskWorkspace->>guardedWrite: return canonical target identity
  guardedWrite->>Caller: reject or continue with bound target

Loading

sequenceDiagram
  participant Caller
  participant safeWriteText
  participant Filesystem
  Caller->>safeWriteText: provide target and content
  safeWriteText->>Filesystem: write and sync staged file
  safeWriteText->>Filesystem: optionally back up existing target
  safeWriteText->>Filesystem: rename staged file to target
  safeWriteText->>Filesystem: sync parent directory on non-Windows platforms
  safeWriteText->>Caller: return success or publication error

Loading

sequenceDiagram
  participant Caller
  participant safeWriteJson
  participant FileLock
  participant Filesystem
  Caller->>safeWriteJson: submit JSON path and options
  safeWriteJson->>Filesystem: resolve target and check confinement
  safeWriteJson->>FileLock: acquire lock for resolved key
  safeWriteJson->>Filesystem: merge, stage, and publish at resolved target
  safeWriteJson->>Caller: return write result

Loading

Merge Risk: Moderate · up to 88927

Guarded atomic writes are added, but several path-race, staging-permission and recovery concerns raised earlier remain open or deferred to follow-up work. Resolve or explicitly accept them before merging.

Security architecture risk: 🟡 Moderate · up to e787e

Version checks and canonical locking improve write safety, but atomic replacement can weaken existing Windows file permissions. Permissions are restored only after publication, and restoration failures are ignored. In directories accessible to other accounts, a previously restricted file could become readable or writable by those accounts.

Retained concerns

• Medium · security · inferred: New atomic text publication does not preserve Windows access restrictions throughout the transition. The replacement file is renamed into place before the original DACL is restored. Failed DACL capture skips restoration, and failed restoration is swallowed. If staging inherits broader permissions than the original target, other local or shared-directory principals can gain read or write access during this window; interruption or restoration failure can leave that exposure persistent despite a successful return. The merge-base direct-save path wrote the existing file without replacing its security descriptor.

Security review details

Security Blast Radius

• inferred — The permission-drift concern reaches existing Windows files published through the shared text primitive, including approved direct tool edits. Its independently attackable scope is the affected files accessible to another principal under the replacement DACL; no remote, cross-tenant, or privilege-escalation reachability was established.

Security Findings and Attack Paths

• inferred — For a target whose explicit DACL is narrower than its directory's inherited permissions, replacement can expose content before restoration. A principal newly permitted by that DACL can read or modify the file without controlling the tool request. Failed restoration can leave the broader access in place; the tests explicitly expect publication to succeed despite capture or restoration failure.

Trust Boundaries and Controls

• observed — The owning task's observation registry supplies publication authority. The guard rejects absent edit authority, checks cancellation before publication, and compares versions under a canonical advisory lock. Its documented atomicity guarantee applies to writers participating in that lock protocol, not arbitrary external filesystem writers.

Resilience and Maintainability Implications

• inferred — Without a prior task observation, ApplyDiff can derive content from one read while the preview records a newer token. The guard can then accept earlier-derived content against that newer token. Existing observations prevent this substitution, and unobserved direct edits reject. The same inter-read overwrite exposure existed in the merge-base unguarded save flow, so it is a remaining limitation rather than an introduced concern.

Hardening Proposals

• proposed — Make Windows permission preservation a pre-commit requirement: restrict staging before writing sensitive bytes, apply and verify the intended DACL before publication, and abort without replacing the original when that guarantee cannot be established.
• proposed — Bind edit publication to the stat-matched read used to derive its content, rather than permitting a later preview read to supply that identity.

Passed checks: 5 passed

Check name	Status	Explanation
Title check		The title names a rung of a split PR series, the guarded write core, and its recovery readers. The changeset adds guardedWrite.ts, jsonFileRecovery.ts changes, indentation-reader hasClippedLines, and …
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Security Boundaries		PR #1917 (author easonLiangWorldedtech) adds guarded-write, safe-write, and recovery code. I checked the changed paths against the criterion: a concrete changed path must leak secrets or PII, trust or…
Lifecycle Resource Cleanup		The changed lifecycle paths in this PR release their resources. I checked the PR head (refs/coderabbit/pre-merge-check/head) for each changed file that creates a resource. - Locks (`guardedWrite.t…

Full details: Title check

Explanation

The title names a rung of a split PR series, the guarded write core, and its recovery readers. The changeset adds guardedWrite.ts, jsonFileRecovery.ts changes, indentation-reader hasClippedLines, and observationRegistry.forget, so the title refers to real parts of the change. However, the title is dominated by process metrics (line counts, selector ranges, headroom) that do not describe the change to a reader scanning history, and it runs 22 words, longer than a short title should be. The core subject is clear, so this is a partial match rather than a vague or unrelated title.

Full details: Security Boundaries

Explanation

PR #1917 (author easonLiangWorldedtech) adds guarded-write, safe-write, and recovery code. I checked the changed paths against the criterion: a concrete changed path must leak secrets or PII, trust or execute unvalidated input, or bypass approval or allowlist controls. Evidence from the review threads and the code they quote: - Workspace containment is enforced. guardedWrite.ts checks lexical containment before queuing and canonical containment before publish. Unapproved outside-workspace writes are rejected. Approved outside-workspace writes must match the canonical target captured before approval. - The argument-order defect in WriteToFileTool.ts, ApplyPatchTool.ts, EditTool.ts, EditFileTool.ts, and SearchReplaceTool.ts passes isOutsideWorkspace as completeOverride. This does not open a bypass. approvedOutsideWorkspace stays undefined, so guardedWrite rejects the outside-workspace write. The defect fails closed, but it breaks valid approved writes. - ApplyDiffTool does not pass the approval flag. This also fails closed. - The preview observation in DiffViewProvider.open() can let an edit publish to a file the model did not read. This weakens the read-before-edit guard. It is a staleness and data-integrity defect. The user still approves the diff before the save, so no approval step is skipped. - The TOCTOU and symlink-swap findings (ancestor replacement, lock-key retargeting) need a local attacker who races the write. They are hardening issues. I found no changed path where they bypass an approval or allowlist control. - A review finding noted an absolute ancestor path in a guard error. That was addressed in the latest update. It is not a secret or PII leak under the criterion. - No changed path logs secrets or PII, or takes unvalidated input into execution. No changed path matches an explicit failure condition of this check. The findings above are correctness, data-loss, or robustness issues. The check does not cover them.

Full details: Lifecycle Resource Cleanup

Explanation

The changed lifecycle paths in this PR release their resources. I checked the PR head (refs/coderabbit/pre-merge-check/head) for each changed file that creates a resource. - Locks (guardedWrite.ts, safeWriteJson.ts, jsonFileRecovery.ts): each acquisition uses withFileLock or acquireFileLock. safeWriteJson.ts releases the lock in a finally block, including when the merge callback throws. - Temp files, staging, and DACL dumps (safeWriteText.ts): the catch path unlinks tempPath, daclDumpPath, and the per-write staging directory. Descriptors opened with openSync are closed in finally blocks. - Per-path FIFO chain (guardedWrite.ts, enqueue): each settled link removes its own map entry, whether it resolves or rejects, so the map does not grow per written path. resetChain is test-only. - Backup file (safeWriteText.ts): a pre-commit failure unlinks the backup (line 614–621). A partial backup is removed when the copy fails (line 525). Step 6 unlinks the backup on success. The only retained backup is the post-commit durability case. That retention is deliberate. PostCommitDurabilityError reports backupPath to the caller, and the code comments state this contract. - Observation registry (observationRegistry.ts): forget only deletes a map entry. The file has no timers, listeners, or watchers. No changed path adds a listener, watcher, timer, or provider. Work is not duplicated after cancellation: guardedWrite re-checks cancellation before publish. Several review threads concern DiffViewProvider.ts, TaskHistoryStore.ts, and ClineProvider.ts. Those files are not in this PR's changed-file list (git diff --name-status for the review range), so I did not count them against this PR. The residual risk is the intentional post-commit backup. It becomes a leak only if a caller drops PostCommitDurabilityError. No production caller in this PR does that. Wiring to production callers is planned for later rungs.

Explanation

The changed JSON write path in src/utils/safeWriteJson.ts can leave a symlinked JSON file unrecoverable after a crash. The standing prior failure still holds at head 88927cb1. Changed path (diff base..head, src/utils/safeWriteJson.ts is modified): - Head resolves the publish target first. The EPERM fallback runs at lines 322–325: rename(resolvedTargetPath, bak), then rename(temp, resolvedTargetPath). The base used absoluteFilePath for these renames. - Lines 232–241 pick the recovery scan target. They call resolvePublishTarget(absoluteFilePath) and fall back to the link path on any error. - resolvePublishTarget (src/services/file-safety/safeWriteText.ts, lines 168–183) throws for a dangling symlink. The fallback therefore scans dirname(link), not the referent directory. - recoverInterruptedReplacement (src/utils/jsonFileRecovery.ts, line 177) scans dirname(absoluteFilePath) only. The .bak_ and .new_ leftovers sit beside the referent, so the scan misses them. - recoverMissingJsonFile (jsonFileRecovery.ts, lines 33–63) uses the same path.dirname(absoluteFilePath) readdir at line 40. A reader therefore misses them too. Triggering scenario: on Windows, a reader holds the file open. The EPERM fallback runs. The process exits after rename(referent, bak) and before rename(temp, referent). The referent is now absent and the link dangles. The old content sits in the backup, and the new content sits in the staged file. Every later read and write through the link fails. Recovery finds nothing and never restores the backup. The state needs manual repair. The base revision had a different symlink defect, but it recovered this crash window. This head does not. Test gap: safeWriteJson.interruption.test.ts has no symlink or dangling-referent case (grep found none). No test covers this state. Secondary gap: safeWriteJson does not handle PostCommitDurabilityError. The error only appears in comments. The retained backup is not released or reported by the caller. This does not lose data but leaves an unmanaged backup copy. Prior-failure disposition: the claims are factually accurate at this head. The committed-flag fix in safeWriteText.ts (lines 354, 537, 614) addresses only the rollback-after-commit issue. It does not address the symlink recovery path.

Resolution

1. In src/utils/safeWriteJson.ts, when resolvePublishTarget fails on a dangling link, call readlink on the link. Scan the referent's directory for .bak_ and .new_ leftovers. Restore the backup under the held lock before the strict check at line 255. 2. In src/utils/jsonFileRecovery.ts, make recoverMissingJsonFile and the reader path scan the resolved referent directory and path, as well as the link directory. Do not scan path.dirname(absoluteFilePath) alone. 3. Add an interruption test that uses a symlink target. Simulate a crash after rename(target, backup) and before rename(temp, target). Assert that the next read and the next write restore the committed content and leave no backup. 4. Define and document the policy for a backup retained after PostCommitDurabilityError. Handle that error in safeWriteJson, either by releasing the backup or by reporting it to the caller.

Explanation

The description follows the template's main sections (Related GitHub Issue, Description, Test Procedure, Pre-Submission Checklist) but does not link an approved issue. The PR links no issue number. It cites #1991 as the parent re-cut plan and #2443 as a dependency. The author also states the title and head ref are stale and does not claim checklist items. The description covers scope, depends-on order, and gate results in detail. It lacks a Closes: # issue reference, the Simplified Technical English style is not followed in several places (long, multi-clause sentences and dense identifiers), and the Documentation Updates, Visual Snapshots, and Get in Touch sections are absent. Dense blob hashes and metrics make the Description section hard to review. The description also states it is one unit of a four-part sequence, so the scope is partially explained.

Resolution

1. Add a Related GitHub Issue section with an approved issue number in the form 'Closes: #NNN'. If the approved issue is #1991, state that explicitly and confirm it is approved. 2. Rewrite the Description to explain the how: the guarded-write compare-and-swap design, the observation refresh, the workspace containment and canonical-target binding, and the reason for the rung-1 dependency. Identify the reviewer checks needed for the TOCTOU check-to-rename race the comments mention. 3. State the Test Procedure as repeatable steps, including the commands and the environment, and note that the root typecheck was not completed. 4. Complete the Pre-Submission Checklist by marking each applicable item. 5. Fill in Documentation Updates (state 'No documentation updates are required' if true). Mark the Visual Snapshots and Videos sections as not applicable. 6. Add a Get in Touch Discord username. 7. Rewrite the text in short active sentences that follow the requested ASD-STE100-inspired style. Remove the stale-branch-name explanation or move it to Additional Notes.

Explanation

Two changed behaviors in PR #1917 still lack focused regression coverage at the head reviewed. The standing failure's claims hold. Primary PR author: easonLiangWorldedtech. 1. Lock-key change in src/utils/jsonFileRecovery.ts (changed in the PR diff). recoverMissingJsonFile and deleteJsonFile now call resolveLockKey(...) and lock the returned key. Before, they locked the caller's path. src/utils/__tests__/jsonFileRecovery.test.ts is not in the PR's changed-file list. A search of that file finds no resolveLockKey, no symlink, and no withFileLock reference. Its vi.mock targets only fs/promises. Its it/describe cases cover ties, restore order, and traces on plain paths. The changed TaskHistoryStore.deleteSemantics.spec.ts checks lock identity on a regular path, not a symlink alias whose referent is in another directory. safeWriteJson.lockKey.spec.ts tests resolveLockKey alone, not the two helpers' lock acquisition. No test would fail if the helpers reverted to the lexical path. 2. Chmod failure branch in _copyTargetMode (src/utils/safeWriteJson.ts, around lines 447–479). When fs.chmod rejects, the code re-stats the staged file. It throws on a mode mismatch. It logs and continues on a match. The only chmod call in src/utils/__tests__ that touches safeWriteJson is fsSyncActual.chmodSync(..., 0o600) at safeWriteJson.test.ts:1118, which is the POSIX mode-preservation setup. No test mocks fs.chmod to reject. The mismatch rejection branch and the match fallback branch are both untested. The mismatch branch prevents widening a 0o600 target to 0o644, so omitting it is an affected error branch. Both gaps match the check: changed behavior lacks focused coverage at the unit layer, and an affected error branch is omitted. Other specs in the PR (guardedWrite, safeWriteText, TaskHistoryStore) cover adjacent behavior but not these two paths. The prior failure's required resolution has not been applied.

Resolution

Add these focused unit tests before merge: 1. In src/utils/__tests__/jsonFileRecovery.test.ts, create a symlink whose referent is in another directory. Assert that deleteJsonFile and recoverMissingJsonFile acquire the referent's lock key. Spy on withFileLock or resolveLockKey and confirm the assertion fails if the alias path is used as the key. 2. In the same file, force resolveLockKey to reject with a non-ENOENT error. Assert that deleteJsonFile rejects, and that the alias, the referent, and any leftover files remain. 3. In src/utils/__tests__/safeWriteJson.test.ts, mock fs.chmod to reject while the staged file's mode differs from the target's mode. Assert that safeWriteJson rejects, the original target content is unchanged, and no staging files remain. 4. In the same file, mock fs.chmod to reject while the staged mode already equals the target mode. Assert that the write succeeds.

Generate unit tests (beta)

• Create a new PR

• X
• Mastodon
• Reddit
• LinkedIn

Changed-code mutation testing

Focused tests

• extension (cwd src): The mutation run was incomplete; selected test file names were not recorded. See Advisory findings for the failure.

Reproduce locally

From a full checkout containing both commits:

pnpm install --frozen-lockfile
node scripts/stryker-diff.mjs ci --base 5f42a918b9458b0383b7a3a0c86b901a5b311664 --head c38cdfbea4c4e6292ca5955ef7706713b5963ee1

Mutation reports

• extension: reports/mutation/extension/mutation.html

The workflow uploads generated reports in the changed-code-mutation-report artifact. A direct artifact link appears below after upload.

Advisory findings

• extension Stryker preflight failed: 03:26:54 (2730) INFO ProjectReader Found 4 of 1385 file(s) to be mutated. 03:26:54 (2730) INFO Instrumenter Instrumented 4 source file(s) with 267 mutant(s) 03:26:55 (2730) INFO ConcurrencyTokenProvider Creating 2 test runner process(es). 03:26:56 (2730) INFO DryRunExecutor Note: running the dry-run only. No mutations will be tested. 03:26:56 (2730) INFO DryRunExecutor Starting initial test run (vitest test runner with "perTest" coverage analysis). This may take a while. 03:27:28 (2730) ERROR DryRunExecutor One or more tests failed in the initial test run: extension.ts shares the extension-owned focus tracker with the sidebar and code actions and disposes its listeners Cannot find module '../services/file-safety/safeWriteText' imported from /home/runner/work/Zoo-Code/Zoo-Code/.stryker-tmp/sandbox-Ii1AAV/utils/jsonFileRecovery.ts 03:27:28 (2730) ERROR Stryker There were failed tests in the initial test run. file:///home/runner/work/Zoo-Code/Zoo-Code/node_modules/.pnpm/@[email protected]_@[email protected]/node_modules/@stryker-mutator/core/dist/src/process/3-dry-run-executor.js:65 throw new ConfigError('There were failed tests in the initial test run.'); ^ ConfigError: There were failed tests in the initial test run. at DryRunExecutor.validateResultCompleted (file:///home/runner/work/Zoo-Code/Zoo-Code/node_modules/.pnpm/@[email protected]_@[email protected]/node_modules/@stryker-mutator/core/dist/src/process/3-dry-run-executor.js:65:27) at DryRunExecutor.executeDryRun (file:///home/runner/work/Zoo-Code/Zoo-Code/node_modules/.pnpm/@[email protected]_@[email protected]/node_modules/@stryker-mutator/core/dist/src/process/3-dry-run-executor.js:100:14) at process.processTicksAndRejections (node:internal/process/task_queues:103:5) at async WorkItem.execute (file:///home/runner/work/Zoo-Code/Zoo-Code/node_modules/.pnpm/@[email protected]_@[email protected]/node_modules/@stryker-mutator/core/dist/src/concurrent/pool.js:32:28) at async file:///home/runner/work/Zoo-Code/Zoo-Code/node_modules/.pnpm/@[email protected]_@[email protected]/node_modules/@stryker-mutator/core/dist/src/concurrent/pool.js:69:13 { innerError: undefined } Node.js v22.23.4