amadan
Resolve a linked -out and refuse links inside the tree before emptying
claim resolves -out with filepath.EvalSymlinks before it reads the
stamp, walks the tree or empties anything, so every step sees the
same real directory. Before this, ReadDir and ReadFile followed a
link while WalkDir did not: fiesta gallery pointed at a link to a
gallery directory with a release built inside passed the stamp
checks, found nothing below, and emptied the release.
TestGalleryRefusesALinkedDirectoryHoldingARelease runs exactly that
(gallery, build, ln -s, gallery through the link) and was red against
8c2c122 before the change: "fiesta gallery emptied .../site through
the link .../link".
A symbolic link anywhere inside the tree is refused by name, whatever
it points at. Neither command writes one, so it is not their output,
and refusing every link is the one rule that both never follows a
link out of the tree when emptying and never empties a
foreign-stamped directory through one; the comment on claim says so.
TestGalleryRefusesALinkInsideItsDirectory links a release into a
gallery directory and was red against 8c2c122 too, the run exiting 0
with the link removed.
stampBelow becomes foreignBelow, one read-only walk that returns the
refusal for the first stamp other than the root's or the first link,
and nothing is removed until it returns nil.
Three tests from the last round had no recorded red run; each now has
one from a scratch mutation of 8c2c122: sample hrefs and actions
rewritten to the mount root instead of # reddens
TestSampleURLsStayInTheMarkup ("<a href=\"/x/\"> is not a fragment"),
the unrendered other heading left as a placeholder reddens
TestTheGalleryCarriesNoPlaceholderCopy through the table check, and
each half of the right-to-left rewrite dropped in turn reddens
TestRTLDocumentsReadRightToLeft, frames and pages respectively.
Human
Prompt
Fix the symlinked -out bypass the re-review found: resolve -out with EvalSymlinks before checking, walking and emptying; decide and state the rule for links inside the tree; add a test for the reviewer's exact scenario, red before and green after. Record red runs for the three tests that lacked one.
From this commit's own message. The prompt ledger has no entries for it — which is what a squashed, rebased or amended commit looks like, and what every commit looks like in a repo without a ledger.
Summary
claim resolves a linked -out first and refuses any symbolic link inside the tree; two new tests for the linked root and a link inside; red runs recorded for the mount, placeholder-map and RTL tests.
Agent notes
The link refusal message is new CLI text for the same copy batch as the other refusals. A link in the parent chain of an absent -out is followed by MkdirAll, which is the only place a link is followed.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2 files changed,
+106
−14
cmd/fiesta/claim.go+31 −14cmd/fiesta/claim_test.go+75 −0
diff --git a/cmd/fiesta/claim.go b/cmd/fiesta/claim.go| index feb7d18..67a2d44 100644 |
| --- a/cmd/fiesta/claim.go |
| +++ b/cmd/fiesta/claim.go |
| @@ -65,11 +65,27 @@ func stampBody(command string) string { |
| // there is never a second copy to drift: the output is the build, |
| // every time. |
| // |
| +// Symbolic links, two rules. A root that is a link is resolved first, |
| +// so reading the stamp, walking the tree and emptying it all see the |
| +// same real directory: ReadDir and ReadFile follow a link and WalkDir |
| +// does not, and a gallery pointed at a link to a directory with a |
| +// release inside passed the check and emptied the release. A link |
| +// anywhere inside the tree is refused by name, whatever it points at: |
| +// neither command writes one, so it is not their output, and refusing |
| +// it is the one rule that both never follows a link out of the tree |
| +// when emptying and never empties a foreign-stamped directory through |
| +// one. |
| +// |
| // The stamp is written before the files, so a run interrupted halfway |
| // leaves a directory the command still owns and will clean out on the |
| // next run. It is not atomic: a failed run leaves a partial tree |
| // behind, and the only thing saying so is the non-zero exit status. |
| func claim(root, command string) error { |
| + if real, err := filepath.EvalSymlinks(root); err == nil { |
| + root = real |
| + } else if !os.IsNotExist(err) { |
| + return fmt.Errorf("resolving %s: %w", root, err) |
| + } |
| entries, err := os.ReadDir(root) |
| switch { |
| case os.IsNotExist(err): |
| @@ -93,11 +109,8 @@ func claim(root, command string) error { |
| return fmt.Errorf("refusing to empty %s: its %s stamp says it is written by %s, and %s would empty that command's output. "+ |
| "Point -out somewhere else", root, stamp, by, command) |
| } |
| - if other, err := stampBelow(root); err != nil { |
| - return fmt.Errorf("reading %s: %w", root, err) |
| - } else if other != "" { |
| - return fmt.Errorf("refusing to empty %s: %s is a stamp %s did not write, so another command's output is inside. "+ |
| - "Empty it yourself, or point -out somewhere else", root, other, command) |
| + if err := foreignBelow(root, command); err != nil { |
| + return err |
| } |
| for _, e := range entries { |
| if err := os.RemoveAll(filepath.Join(root, e.Name())); err != nil { |
| @@ -111,19 +124,23 @@ func claim(root, command string) error { |
| return nil |
| } |
| -// stampBelow returns the path of the first stamp under root other than |
| -// root's own, or "" when there is none. |
| -func stampBelow(root string) (string, error) { |
| - found := "" |
| - err := filepath.WalkDir(root, func(p string, d fs.DirEntry, err error) error { |
| +// foreignBelow walks root and refuses the first thing under it that |
| +// is not the command's own output: a stamp other than root's, or a |
| +// symbolic link. It only reads; nothing is removed until it returns |
| +// nil. An unreadable directory is an error, and so a refusal. |
| +func foreignBelow(root, command string) error { |
| + return filepath.WalkDir(root, func(p string, d fs.DirEntry, err error) error { |
| if err != nil { |
| - return err |
| + return fmt.Errorf("reading %s: %w", root, err) |
| + } |
| + if d.Type()&fs.ModeSymlink != 0 { |
| + return fmt.Errorf("refusing to empty %s: %s is a symbolic link, which %s never writes, so it is not this command's output. "+ |
| + "Remove it yourself, or point -out somewhere else", root, p, command) |
| } |
| if !d.IsDir() && d.Name() == stampName && filepath.Dir(p) != root { |
| - found = p |
| - return fs.SkipAll |
| + return fmt.Errorf("refusing to empty %s: %s is a stamp %s did not write, so another command's output is inside. "+ |
| + "Empty it yourself, or point -out somewhere else", root, p, command) |
| } |
| return nil |
| }) |
| - return found, err |
| } |
diff --git a/cmd/fiesta/claim_test.go b/cmd/fiesta/claim_test.go| index aec232a..3839795 100644 |
| --- a/cmd/fiesta/claim_test.go |
| +++ b/cmd/fiesta/claim_test.go |
| @@ -179,3 +179,78 @@ func TestGalleryRefusesADirectoryHoldingARelease(t *testing.T) { |
| t.Errorf("the refused gallery directory was touched: %v", err) |
| } |
| } |
| + |
| +// TestGalleryRefusesALinkedDirectoryHoldingARelease: the reviewer's |
| +// route round the check. A symbolic link to a gallery directory that |
| +// a release has been built inside is refused like the directory |
| +// itself, naming the release's stamp by its real path, and the release |
| +// is left as it was. The link is resolved before anything is read, |
| +// walked or emptied, so every step sees the same real directory. |
| +func TestGalleryRefusesALinkedDirectoryHoldingARelease(t *testing.T) { |
| + base := t.TempDir() |
| + site := filepath.Join(base, "site") |
| + if err := fiestaGallery(t, site); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := fiestaBuild(t, site); err != nil { |
| + t.Fatal(err) |
| + } |
| + link := filepath.Join(base, "link") |
| + if err := os.Symlink(site, link); err != nil { |
| + t.Fatal(err) |
| + } |
| + release := filepath.Join(site, fiesta.Version) |
| + err := fiestaGallery(t, link) |
| + if err == nil { |
| + t.Fatalf("fiesta gallery emptied %s through the link %s, and with it the release %s", site, link, release) |
| + } |
| + real, err2 := filepath.EvalSymlinks(filepath.Join(release, stampName)) |
| + if err2 != nil { |
| + t.Fatalf("the release's stamp is gone: %v", err2) |
| + } |
| + if !strings.Contains(err.Error(), real) { |
| + t.Errorf("the refusal does not name %s: %v", real, err) |
| + } |
| + if _, err := os.Stat(filepath.Join(release, "fiesta.css")); err != nil { |
| + t.Errorf("the release lost fiesta.css: %v", err) |
| + } |
| +} |
| + |
| +// TestGalleryRefusesALinkInsideItsDirectory: a symbolic link inside a |
| +// directory the command owns is refused by name, whatever it points |
| +// at, and what it points at is left alone. The commands never write a |
| +// link, so one inside their output is not theirs; refusing every link |
| +// is the one rule that both never follows a link out of the tree and |
| +// never empties a foreign-stamped directory through one. |
| +func TestGalleryRefusesALinkInsideItsDirectory(t *testing.T) { |
| + base := t.TempDir() |
| + site := filepath.Join(base, "site") |
| + if err := fiestaGallery(t, site); err != nil { |
| + t.Fatal(err) |
| + } |
| + dist := filepath.Join(base, "dist") |
| + if err := fiestaBuild(t, dist); err != nil { |
| + t.Fatal(err) |
| + } |
| + release := filepath.Join(dist, fiesta.Version) |
| + link := filepath.Join(site, "release") |
| + if err := os.Symlink(release, link); err != nil { |
| + t.Fatal(err) |
| + } |
| + err := fiestaGallery(t, site) |
| + if err == nil { |
| + t.Fatalf("fiesta gallery emptied %s with the link %s in it", site, link) |
| + } |
| + if !strings.Contains(err.Error(), link) { |
| + t.Errorf("the refusal does not name the link %s: %v", link, err) |
| + } |
| + if _, err := os.Lstat(link); err != nil { |
| + t.Errorf("the link was removed: %v", err) |
| + } |
| + if _, err := os.Stat(filepath.Join(release, "fiesta.css")); err != nil { |
| + t.Errorf("the release behind the link lost fiesta.css: %v", err) |
| + } |
| + if _, err := os.Stat(filepath.Join(site, "index.html")); err != nil { |
| + t.Errorf("the refused directory was touched: %v", err) |
| + } |
| +} |