Unlink the generated path until it holds nothing, so a live symlink is replaced too

[thedavidmeister]

buildFileForContract writes through a live symlink at the generated path, while
LibFs.sol claims it does not. This finishes the unlink and makes the claim true.

What defect this fixes, and why there is no issue

This PR closes no issue, so here is the defect it is for.

#110 fixed the
dangling symlink half of the unlink and named the other half as what it was
leaving behind: "the write's behaviour for a symlink that does resolve
(covered here only for isPresent's own answer, not through
buildFileForContract)". The issue that owned the removal block's coverage,
#66, was closed with
#110. This is that remainder, and it is a behaviour defect rather than a
coverage gap: the live case is not merely untested, it is wrong, and the NatSpec
asserts the opposite.

What is wrong

Foundry's fs cheatcodes resolve a path before they act on it. vm.removeFile on
a live symlink therefore removes the link's TARGET and leaves the link in
place; only on a dangling symlink, where resolution fails and the unresolved
path is used, does it remove the link itself.

A single if (isPresent(vm, path)) removal is enough for the dangling case and
not for the live one. It removes the target, which leaves the link dangling, and
then vm.writeFile follows the link and re-creates the target. The generated
source lands at the link's target and the generated path is still a symlink.

Measured on main at c816251, with the tree read back through the shell rather
than through the cheatcodes under test. A live link
src/generated/ProbeELive.sol -> ProbeELiveTarget.txt whose target held
SENTINEL, then one buildFileForContract:

$ ls -l src/generated/
lrwxrwxrwx 20  ProbeELive.sol -> ProbeELiveTarget.txt
-rw-r--r-- 379 ProbeELiveTarget.txt

$ readlink src/generated/ProbeELive.sol
ProbeELiveTarget.txt

$ head -c 120 src/generated/ProbeELiveTarget.txt
// SPDX-License-Identifier: LicenseRef-DCL-1.0
// SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd
pragma solidity ^0.8.25;

The path is still a symlink, SENTINEL is gone, and the 379 byte generated file
is at the link's target. The write left the path it was given, which is exactly
what the docstring said could not happen.

How far it reaches

Bounded by fs_permissions, not by GENERATED_DIR. A link whose target resolves
outside the grant reverts inside isPresent, at vm.exists, before anything is
removed or written:

[FAIL: vm.exists: the path probe-outside-target.txt is not allowed to be
accessed for read operations] testProbeOutsideGrant()

with the outside file's contents intact afterwards. So nothing escapes the grant.

meta is granted read-write alongside src/generated, though, and that is where
the write does land on main. Same probe, link pointed at meta instead:

$ ls -l src/generated/ meta/
src/generated/: lrwxrwxrwx 32  ProbeMeta.sol -> ../../meta/probe-meta-target.txt
meta/:          -rw-r--r-- 378 probe-meta-target.txt

$ head -c 60 meta/probe-meta-target.txt
// SPDX-License-Identifier: LicenseRef-DCL-1.0

So the generated source leaves src/generated entirely. With the loop, the same
probe leaves meta/ empty and src/generated/ProbeMeta.sol a regular 378 byte
file.

The change

if becomes while. The first pass removes what the link resolves to, which
leaves the link dangling; the second removes the link. Every pass removes
something the next one no longer finds, so it terminates, and the path holds
nothing when the write happens.

The docstring is corrected to state the two consequences that are true today
rather than the one that was not: taking a live symlink off the path takes what
it resolves to with it, and a directory, or a symlink onto a directory, cannot be
unlinked at all and reverts.

Termination, measured rather than argued

Turning a bounded if into an unbounded while has exactly one new failure
mode, so every shape that could spin was run on the merge commit. Probes are
throwaway and are not part of the diff; each was run on its own under a shell
timeout so a non-terminating loop would show as the timeout rather than as a
hung suite.

shape at the generated path	result	tree afterwards
X.sol -> X.sol (self loop)	PASS	path is a regular generated file
A.sol -> B.txt -> A.sol (2-cycle)	PASS	path is a regular generated file, B.txt left pointing at it
path -> mid -> file (chain)	PASS	path is a regular generated file, file gone, mid left dangling beside it
symlink onto a directory	reverts	untouched
a directory at the path	reverts	untouched
link resolving outside the grant	reverts	untouched

Both reverts are vm.removeFile: failed to remove file "…": Is a directory (os error 21). Neither writes anywhere, so nothing escapes the path. Both predate
this change and #110, and are out of scope here; they are stated so they are on
the record, and the docstring now names them.

QA

Merged current main in; the change is re-sited into the five-arg overload
#112 introduced, which is
where the unlink and the write now live. The docstring correction stays on the
four-arg NatSpec, which is where the unlink paragraph lives and which the
overload defers to.

#100 has since landed in
the same block and hoists the file's whole content into a local before any disk
mutation, so a build that reverts cannot destroy the file that is already there.
The loop is resolved onto that ordering rather than over it: content is
computed first, then vm.createDir, then the loop, then the write. #100's
testBuildFileForContractFailedBuildKeepsExistingFile passes on the merge
commit, which is what says the ordering survived.

• Discriminating test: testBuildFileForContractReplacesLiveSymlink. Run against
  current main's if with nothing else changed, where it fails
  [FAIL: the path is still a symlink], Suite result: FAILED. 0 passed; 1 failed, and passes only with the loop.
• Oracle: the filesystem read back through ln/readlink/rm over ffi, never
  through the cheatcodes under test, plus a control contract generated at a path
  that held nothing — the claim is that the symlink case produces the same file
  as the empty-path case, not that particular bytes appear.
• Category check: the finding is that a live symlink at the generated path is
  written through while LibFs.sol claims it is not. Covered: the write
  behaviour (test above) and the claim itself (docstring corrected to what is
  measured, including the directory cases it cannot unlink).
• Full suite on the merge commit: nix develop -c forge test →
  Ran 23 test suites: 166 tests passed, 0 failed, 0 skipped (166 total tests),
  against 156 on this branch immediately before the merge, with no existing test
  changed. The +10 is entirely main's own: main went from 155 test functions
  at 89cb0a2 to 165 at ff69c10 (+1 in LibFs.buildFileForContract.t.sol, +9
  in LibHexString.bytesToHex.t.sol). This PR's one added test,
  testBuildFileForContractReplacesLiveSymlink, is the +1 on top of each.
  main moved five times during this: c816251 → 37a8dcf (version bump and
  a .soldeerignore edit) → 89cb0a2 (chore: bump forge-std 1.16.1 -> 1.16.2, including the prefix rewrite the pin forces #126's forge-std 1.16.2 prefix rewrite,
  which touches both of this PR's files) → e31d902 (LibFs: build the generated file content before touching disk #100, which reorders the
  block this PR changes) → ff69c10 (docs: state the fixed-point bound and how a non-converging build reports #136 and a version bump). It was merged in
  and the suite, formatter, coverage and mutation matrix were re-run after each.
• nix develop -c forge fmt --check → exit 0.
• Coverage still 100% on all four columns for every src/lib file: LibFs.sol
  100% lines (22/22), statements (19/19), branches (3/3), funcs (5/5). The line
  and statement denominators rose from 20 and 17 because LibFs: build the generated file content before touching disk #100's content hoist is
  now in the function; branches and funcs are unchanged.
• Mutation matrix on the merge commit, run with mutation-probe, which proves
  the suite ran from its own tally rather than from an exit code. Baseline green
  at 166, then 3/3 KILLED, 0 survived, 0 no-run, 0 harness errors:
  • while (isPresent(vm, path)) → if (isPresent(vm, path)), i.e. the
    behaviour on main — KILLED by
    testBuildFileForContractReplacesLiveSymlink.
  • while (isPresent(vm, path)) → while (vm.exists(path)), i.e. looping on
    the resolved path instead of the path — KILLED by
    testBuildFileForContractReplacesLiveSymlink and
    testBuildFileForContractReplacesDanglingSymlink.
  • while (isPresent(vm, path)) → while (false), i.e. no unlink at all —
    KILLED by the same two.

[coderabbitai]

Warning

Review limit reached

@thedavidmeister, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 42 minutes

Limit details: You’ve used all 1 included review currently available under your plan.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 6758ee81-e2da-4357-97a3-bc91d78580d3

📥 Commits

Reviewing files that changed from the base of the PR and between ff69c10 and 8593160.

📒 Files selected for processing (2)

• src/lib/LibFs.sol
• test/src/lib/LibFs.isPresent.t.sol

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}