Skip to content

Commit 98153b9

Browse files
donislawdevclaude
andcommitted
core: keep a replaced file's mode, and refuse a read only one everywhere
"tfg recipe fmt -w" is the one command that writes over a file somebody wrote by hand. Measured on Windows and on Linux with a new probe, tools/probes/atomic-replace, on the code that was shipping: - it did not keep the file's mode. A rename moves the file it renames, mode and all, and the temporary copy was created with 0644 - so a recipe somebody had made private at 0600 came back readable by everyone on the machine. - read only meant two different things. A rename asks for permission on the directory rather than on the file, so a read only recipe was protected on Windows and replaced anyway on Linux. - the half written copy was cleaned up after a failed rename and not after a failed write, and that copy sits in somebody's repository because a rename across volumes is not one operation. So the replacement keeps the mode it found, refuses a read only file on every system - the same rule this project applies to file names that only work on one - and removes its copy whatever failed. The refusal says what to do about it rather than passing the system's sentence through: on Windows "Access is denied" is what you get when another program is holding the file open, which is exactly the state a recipe is in while somebody edits it. It moved to internal/core because it is a different operation from writing one of our own files, not because two callers needed it. The manifest keeps its own write: it claims a name nobody else can hold and streams into it, so preserving a mode and refusing read only are questions that do not arise there. Five guards, five mutations. The one about the mode is proven by probe instead, because it skips on Windows - that is where the mutation runner lives, and a substitution there would be reported as not caught for a guard that works. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 7cdde53 commit 98153b9

4 files changed

Lines changed: 340 additions & 27 deletions

File tree

internal/cli/recipecmd.go

Lines changed: 2 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"io"
1010
"os"
1111

12+
"github.com/donislawdev/TestingFilesGenerator/internal/core"
1213
"github.com/donislawdev/TestingFilesGenerator/internal/engine"
1314
"github.com/donislawdev/TestingFilesGenerator/internal/recipe"
1415
)
@@ -52,32 +53,6 @@ func loadRecipe(path string, errOut io.Writer) (*recipe.Recipe, string, int) {
5253
return rec, hash, ExitOK
5354
}
5455

55-
// replaceFile puts new content in place of a file in one step.
56-
//
57-
// "recipe fmt -w" is the only command here that writes over a file somebody
58-
// wrote by hand, and it was the least careful write in the tool: os.WriteFile
59-
// truncates first and fills afterwards, so a process ending in between leaves
60-
// the recipe half its length and no copy of what it was.
61-
//
62-
// Every generated file has gone through a temporary name and a rename since
63-
// the beginning, for exactly this reason, and so does the manifest since
64-
// earlier today. This is the same thing for the one file that is not ours.
65-
//
66-
// The temporary file sits beside the target rather than in the system
67-
// temporary directory, because a rename across volumes is not one operation
68-
// and this whole function exists to have one.
69-
func replaceFile(path string, content []byte) error {
70-
tmp := path + ".tfg-writing"
71-
if err := os.WriteFile(tmp, content, 0o644); err != nil {
72-
return err
73-
}
74-
if err := os.Rename(tmp, path); err != nil {
75-
os.Remove(tmp)
76-
return err
77-
}
78-
return nil
79-
}
80-
8156
// validate runs the checks a run would run and writes nothing at all, so it
8257
// suits a pre commit hook.
8358
func validate(args []string, out, errOut io.Writer) int {
@@ -292,7 +267,7 @@ Flags:
292267
fmt.Fprintf(errOut, "%s was already settled and was not touched.\n", path)
293268
return ExitOK
294269
}
295-
if err := replaceFile(path, canon); err != nil {
270+
if err := core.ReplaceFile(path, canon); err != nil {
296271
fmt.Fprintf(errOut, "tfg: cannot write %s: %s\n", path, describeError(err))
297272
return ExitIO
298273
}

internal/core/replace.go

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
package core
2+
3+
import "os"
4+
5+
// writingSuffix marks the half written copy while it is being filled.
6+
//
7+
// Beside the target rather than in the system temporary directory, because a
8+
// rename across volumes is not one operation and the whole point of this is to
9+
// have one.
10+
const writingSuffix = ".tfg-writing"
11+
12+
// ReplaceFile puts new content in place of a file somebody else owns.
13+
//
14+
// This is a different operation from writing one of our own files, and the
15+
// difference is the whole reason it exists separately. Everything else this
16+
// tool writes goes into the output directory under a name it just claimed -
17+
// nobody had it before and nobody else can be holding it. This lands on a file
18+
// that already exists, belongs to a person, and lives in their repository.
19+
//
20+
// Three properties, and all three came from measuring rather than from care.
21+
// The probe is tools/probes/atomic-replace and it was run on Windows and on
22+
// Linux, because the two disagree about two of them.
23+
//
24+
// IT KEEPS THE MODE. A rename moves the file it renames, mode and all, so
25+
// the mode that survives is the temporary file's. Measured on Linux: a
26+
// recipe somebody had made private at 0600 came back at 0644, readable by
27+
// everyone on the machine. That is the version this tool has shipped.
28+
//
29+
// IT REFUSES A READ ONLY FILE, ON EVERY SYSTEM. A rename asks for permission
30+
// on the DIRECTORY, not on the file, so read only protects a recipe on
31+
// Windows and does not on Linux. Refusing everywhere is the same rule this
32+
// project applies to file names that only work on one system: a recipe that
33+
// behaves differently on a colleague's machine is worse than one refused on
34+
// all of them. Measured: the owner write bit is off in both places, so one
35+
// question answers it.
36+
//
37+
// IT LEAVES NOTHING BEHIND. Whatever fails, the half written copy goes.
38+
//
39+
// What it does not do is protect against the file being changed underneath it
40+
// between a read and this call. That belongs to the caller, which is the only
41+
// one that knows when it read.
42+
func ReplaceFile(path string, content []byte) error {
43+
mode, err := modeToKeep(path)
44+
if err != nil {
45+
return err
46+
}
47+
48+
tmp := path + writingSuffix
49+
if err := writeWhole(tmp, content, mode); err != nil {
50+
os.Remove(tmp)
51+
return err
52+
}
53+
if err := os.Rename(tmp, path); err != nil {
54+
os.Remove(tmp)
55+
return &ReplaceError{Path: path, Err: err}
56+
}
57+
return nil
58+
}
59+
60+
// modeToKeep is the mode the replacement has to come back with.
61+
//
62+
// A file that is not there yet is not a replacement at all, so it gets the
63+
// ordinary mode for a new file. A file that is there and cannot be written to
64+
// is refused before anything is created.
65+
func modeToKeep(path string) (os.FileMode, error) {
66+
info, err := os.Stat(path)
67+
switch {
68+
case os.IsNotExist(err):
69+
return 0o644, nil
70+
case err != nil:
71+
return 0, err
72+
}
73+
74+
mode := info.Mode().Perm()
75+
// The owner write bit, which is the one question that means the same thing
76+
// on both systems. Windows has no permission bits and an attribute
77+
// instead, and Go turns that attribute into exactly this bit.
78+
if mode&0o200 == 0 {
79+
return 0, &ReadOnlyError{Path: path, Mode: mode}
80+
}
81+
return mode, nil
82+
}
83+
84+
// writeWhole fills the copy and makes sure it carries the mode it was given.
85+
//
86+
// The mode is set explicitly rather than left to the create call, for two
87+
// reasons that both bite quietly: a create only applies its mode when the file
88+
// is new, so a leftover copy from an interrupted run would keep whatever it
89+
// had, and the process umask takes bits away from a create and not from a
90+
// chmod.
91+
func writeWhole(path string, content []byte, mode os.FileMode) error {
92+
f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, mode)
93+
if err != nil {
94+
return err
95+
}
96+
if _, err := f.Write(content); err != nil {
97+
f.Close()
98+
return err
99+
}
100+
if err := f.Close(); err != nil {
101+
return err
102+
}
103+
return os.Chmod(path, mode)
104+
}
105+
106+
// ReadOnlyError is refusing to write over a file marked read only.
107+
type ReadOnlyError struct {
108+
Path string
109+
Mode os.FileMode
110+
}
111+
112+
func (e *ReadOnlyError) Error() string {
113+
return "the file is read only, so it was left as it was and nothing was written. " +
114+
"A file has to be writable to be replaced. " +
115+
"Make it writable and try again, or write to a different path"
116+
}
117+
118+
// ReplaceError is the rename failing, which is where the interesting failures
119+
// land.
120+
//
121+
// The wording names the likely cause rather than repeating the system's, and
122+
// that is measured rather than polite: on Windows a file that any other
123+
// process has open cannot be renamed onto, and the sentence the system gives
124+
// for it is "Access is denied" - which is true about the call and says nothing
125+
// about what happened or what to do. A recipe is exactly the file somebody has
126+
// open in their editor while they work on it.
127+
type ReplaceError struct {
128+
Path string
129+
Err error
130+
}
131+
132+
func (e *ReplaceError) Error() string {
133+
return "the file could not be replaced and was left as it was, so nothing was written. " +
134+
"On Windows this is what happens when another program is holding it open. " +
135+
"Close it there and try again, or write to a different path"
136+
}
137+
138+
func (e *ReplaceError) Unwrap() error { return e.Err }

internal/guard/mutationcoverage_test.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,13 @@ var notProvenByMutation = map[string]bool{
8787
// "proven another way" are different states and lumping them together would
8888
// send a later session to re-prove what is already proven.
8989
var provenByProbe = map[string]string{
90+
"TestReplacingAFileKeepsTheModeItHad": "checked 2026-08-12 with tools/probes/atomic-replace, run on Windows and on Linux through WSL2. " +
91+
"The version this replaced wrote the temporary copy with 0644 and renamed it over the original, and a rename moves the file it renames - " +
92+
"so a recipe somebody had made private at 0600 came back at 0644, readable by everyone on the machine. Measured on Linux, on the code that was shipping. " +
93+
"It cannot be a mutation entry because the guard SKIPS on Windows, which is where the mutation runner lives: Windows has no permission bits, " +
94+
"so the question cannot be put there at all and a substitution would be reported NOT CAUGHT for a guard that is working. " +
95+
"The probe is the honest instrument here, because it is the one that can be run where the answer exists.",
96+
9097
"TestNoNumberATestPrintsIsCopiedIntoTheProse": "checked 2026-08-05 by planting a document in docs/ carrying one bad number per rule - " +
9198
"a D1 parity pair, an identifier count, a link count, a mutation breakdown in three spellings, a guard count and a coverage figure. " +
9299
"All eight were named, and deleting the file put the run back to green. " +

internal/guard/replacefile_test.go

Lines changed: 193 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,193 @@
1+
package guard
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"strings"
7+
"testing"
8+
9+
"github.com/donislawdev/TestingFilesGenerator/internal/core"
10+
)
11+
12+
// Replacing somebody's file keeps the mode it had.
13+
//
14+
// The defect this closes was in the tool before the window existed, and it is
15+
// the reason the whole write path got looked at. "tfg recipe fmt -w" replaced
16+
// a recipe through a rename, and a rename moves the file it renames - mode and
17+
// all - so what survived was the temporary file's mode rather than the
18+
// original's. Measured on Linux with tools/probes/atomic-replace: a recipe
19+
// somebody had made private at 0600 came back at 0644, readable by everyone
20+
// on the machine.
21+
//
22+
// It skips on Windows and says so, rather than passing there. Windows has no
23+
// permission bits, so the question this asks cannot be put to it - and a guard
24+
// that reports success for a question it did not ask is worse than one that
25+
// stays away.
26+
func TestReplacingAFileKeepsTheModeItHad(t *testing.T) {
27+
if os.Getenv("GOOS") == "windows" || filepath.Separator == '\\' {
28+
t.Skip("Windows has no permission bits, so there is no mode to keep - measured on Linux instead")
29+
}
30+
31+
path := filepath.Join(t.TempDir(), "recipe.yaml")
32+
if err := os.WriteFile(path, []byte("version: 1\n"), 0o600); err != nil {
33+
t.Fatal(err)
34+
}
35+
36+
if err := core.ReplaceFile(path, []byte("version: 2\n")); err != nil {
37+
t.Fatalf("replacing a writable file failed: %v", err)
38+
}
39+
40+
info, err := os.Stat(path)
41+
if err != nil {
42+
t.Fatal(err)
43+
}
44+
if got := info.Mode().Perm(); got != 0o600 {
45+
t.Errorf("the file was %v before and is %v after, so a private recipe came back readable by others",
46+
os.FileMode(0o600), got)
47+
}
48+
}
49+
50+
// A file marked read only is left alone, on every system.
51+
//
52+
// Measured on both, because they disagree: a rename asks for permission on the
53+
// DIRECTORY rather than on the file, so read only stops the replacement on
54+
// Windows and does not stop it on Linux. Refusing everywhere is the rule this
55+
// project already applies to file names - a recipe that behaves differently on
56+
// a colleague's machine is worse than one refused on all of them.
57+
//
58+
// Both halves are checked, because refusing while having already written is
59+
// the failure that looks like success: the error is reported, and the file is
60+
// gone anyway.
61+
func TestAReadOnlyFileIsLeftAlone(t *testing.T) {
62+
dir := t.TempDir()
63+
path := filepath.Join(dir, "recipe.yaml")
64+
const was = "version: 1\n"
65+
if err := os.WriteFile(path, []byte(was), 0o444); err != nil {
66+
t.Fatal(err)
67+
}
68+
defer os.Chmod(path, 0o644)
69+
70+
err := core.ReplaceFile(path, []byte("version: 2\n"))
71+
if err == nil {
72+
t.Fatal("a read only file was replaced without complaint")
73+
}
74+
75+
// The refusal says what to do about it, which is D6 rather than politeness.
76+
for _, want := range []string{"read only", "writable"} {
77+
if !strings.Contains(err.Error(), want) {
78+
t.Errorf("the refusal does not mention %q. It says: %s", want, err)
79+
}
80+
}
81+
82+
now, readErr := os.ReadFile(path)
83+
if readErr != nil {
84+
t.Fatal(readErr)
85+
}
86+
if string(now) != was {
87+
t.Errorf("the file was refused and changed anyway: %q", string(now))
88+
}
89+
}
90+
91+
// Nothing is left beside the file, whatever happened.
92+
//
93+
// The half written copy sits next to the target rather than in the system
94+
// temporary directory, because a rename across volumes is not one operation.
95+
// That puts it in somebody's repository, so it has to go - on the way out of
96+
// every path, not only the happy one. The version this replaced cleaned up
97+
// after a failed rename and not after a failed write.
98+
func TestReplacingLeavesNoHalfWrittenCopyBehind(t *testing.T) {
99+
for _, c := range []struct {
100+
what string
101+
build func(t *testing.T, dir string) string
102+
}{
103+
{"a replacement that worked", func(t *testing.T, dir string) string {
104+
path := filepath.Join(dir, "fine.yaml")
105+
if err := os.WriteFile(path, []byte("version: 1\n"), 0o644); err != nil {
106+
t.Fatal(err)
107+
}
108+
return path
109+
}},
110+
{"one refused before anything was created", func(t *testing.T, dir string) string {
111+
path := filepath.Join(dir, "locked.yaml")
112+
if err := os.WriteFile(path, []byte("version: 1\n"), 0o444); err != nil {
113+
t.Fatal(err)
114+
}
115+
t.Cleanup(func() { os.Chmod(path, 0o644) })
116+
return path
117+
}},
118+
// The one that matters, and the one the first version of this guard
119+
// did not have: the copy is written and THEN the rename fails. Without
120+
// a case that gets that far, this test passed on a writer that never
121+
// cleaned up, because the only failure it tried refuses before there
122+
// is anything to clean.
123+
//
124+
// A directory with something in it is the portable way to make a
125+
// rename fail. Holding the target open does it on Windows and not on
126+
// Linux, and a full disk cannot be arranged in a test.
127+
{"one where the rename failed after the copy was written", func(t *testing.T, dir string) string {
128+
path := filepath.Join(dir, "occupied")
129+
if err := os.MkdirAll(filepath.Join(path, "child"), 0o755); err != nil {
130+
t.Fatal(err)
131+
}
132+
return path
133+
}},
134+
} {
135+
t.Run(c.what, func(t *testing.T) {
136+
dir := t.TempDir()
137+
path := c.build(t, dir)
138+
139+
_ = core.ReplaceFile(path, []byte("version: 2\n"))
140+
141+
leftovers, err := filepath.Glob(filepath.Join(dir, "*"+".tfg-writing"))
142+
if err != nil {
143+
t.Fatal(err)
144+
}
145+
if len(leftovers) != 0 {
146+
t.Errorf("after %s there is still %v beside the file", c.what, leftovers)
147+
}
148+
})
149+
}
150+
}
151+
152+
// The case above is only worth having if the middle of it is really reached.
153+
//
154+
// A test that arranges a failure it never triggers reports a clean directory
155+
// because nothing ever happened in it - which is the same shape as a guard
156+
// that passes without reaching the code. So this asks the arrangement itself:
157+
// does replacing onto a non empty directory actually fail.
158+
func TestTheRenameFailureThatGuardIsBuiltOnReallyHappens(t *testing.T) {
159+
dir := t.TempDir()
160+
path := filepath.Join(dir, "occupied")
161+
if err := os.MkdirAll(filepath.Join(path, "child"), 0o755); err != nil {
162+
t.Fatal(err)
163+
}
164+
165+
if err := core.ReplaceFile(path, []byte("version: 2\n")); err == nil {
166+
t.Fatal("replacing onto a non empty directory succeeded, so the guard above never reaches the cleanup it exists for")
167+
}
168+
}
169+
170+
// The content that arrives is the content that was asked for.
171+
//
172+
// The plain case, and it is here because everything else in this file is about
173+
// a refusal - a writer that refuses correctly and writes the wrong bytes would
174+
// pass all of them.
175+
func TestReplacingPutsTheContentThatWasAskedFor(t *testing.T) {
176+
path := filepath.Join(t.TempDir(), "recipe.yaml")
177+
if err := os.WriteFile(path, []byte("version: 1\n"), 0o644); err != nil {
178+
t.Fatal(err)
179+
}
180+
181+
const want = "version: 2\ntargets: []\n"
182+
if err := core.ReplaceFile(path, []byte(want)); err != nil {
183+
t.Fatalf("replacing failed: %v", err)
184+
}
185+
186+
got, err := os.ReadFile(path)
187+
if err != nil {
188+
t.Fatal(err)
189+
}
190+
if string(got) != want {
191+
t.Errorf("the file holds %q and should hold %q", string(got), want)
192+
}
193+
}

0 commit comments

Comments
 (0)