Skip to content

Commit 1b5f726

Browse files
donislawdevclaude
andcommitted
core: close the small findings the audit listed last
Ten of them, none individually large, and three worth naming because they could have destroyed something. recipe fmt -w truncated the file first and filled it afterwards, so a process ending in between left a recipe at half its length and no copy of what it was. Every generated file has gone through a temporary name and a rename since the beginning, and the manifest since earlier today. This is the same thing for the one file in the tool that is not ours. A run refused to write over a file under the name it ends up with, and never looked at the name it writes through. That one is created in a way that truncates, so a file already sitting there lost its contents in silence. It is an unlikely name to meet by accident and an easy one to leave behind - a run killed outright leaves exactly that shape - so the next run into the same directory would have eaten it. --size 1e5 quietly meant 100000 bytes while a recipe refuses 1_000 and 0x10 outright, on the grounds that guessing at a spelling is the behaviour that reader was written to remove. The difference was nobody's decision: it fell out of ParseFloat being permissive. Sizes now take digits and at most one decimal point, so 1.5gib is untouched and 1e5 is refused in the same words the recipe uses. The rest, in one line each: - The command in the manifest was joined with spaces, so an argument holding one arrived as two and the recorded command described a different run. - tfg generate --json dropped the error from writing to standard output, so a closed pipe was reported as a success with nothing delivered. - Progress counted the bytes of files that failed, so the bar claimed bytes nobody could find. - classify had no answer for a deadline running out, which falls through to the ending that means this program broke. Unreachable from the command line today and reachable from the window that is coming. - percent multiplied before dividing, which leaves the range of an int64 above about 92 PB. - AppendFiller divided by the length of its vocabulary without checking it had one, and FillRecords would spin without end on a record that added no bytes. Neither is reachable today and neither is forbidden by the interface, so both say which mistake it is rather than hanging or panicking anonymously. - Every CI job now has a ceiling on its time, and the window binary is built. Nothing built it, so it could stop compiling without a red run. Not fixed, written down instead: "go test ./internal/<package>/" is green without running a single test, on seven packages out of seven, because every guard lives in one package on purpose. Adding empty test files to each would trade one misleading answer for a wall of noise. See O50. Seven guards, each seen red first, and fourteen mutations across this batch. Two mutations were caught wrong: one counted a commented out ceiling as a real one, and one demanded a ceiling for the triggers under "on:" as though they were jobs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent c643851 commit 1b5f726

11 files changed

Lines changed: 424 additions & 7 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,11 @@ env:
2121
jobs:
2222
test:
2323
name: test on ${{ matrix.os }}
24+
# A hung job otherwise holds a runner until the GitHub default of six
25+
# hours. Every number here is well above what the job takes today: the
26+
# matrix runs in about a minute, the race detector took 148 s when it was
27+
# measured, and fuzzing is given 5 minutes a target by its own loop.
28+
timeout-minutes: 20
2429
strategy:
2530
fail-fast: false
2631
matrix:
@@ -73,9 +78,22 @@ jobs:
7378
- name: build the command line binary
7479
run: go build ./cmd/tfg
7580

81+
- name: build the window binary
82+
# Nothing built this, so it could stop compiling and no run would say
83+
# so. The package is a stub today and that is exactly why it is cheap
84+
# to keep honest - it costs a second now and a bisect later.
85+
#
86+
# CGO_ENABLED is 0 for this whole job, which is the point: the window
87+
# binary has to keep building without a C toolchain for as long as the
88+
# toolkit is not wired in. The day it is, this step needs a compiler
89+
# and moves to a job that has one, and finding that out here is better
90+
# than finding it out when the release does not build.
91+
run: go build ./cmd/tfg-gui
92+
7693
govulncheck:
7794
name: known vulnerabilities
7895
runs-on: ubuntu-latest
96+
timeout-minutes: 15
7997
steps:
8098
- uses: actions/checkout@v7
8199

@@ -99,6 +117,7 @@ jobs:
99117
staticcheck:
100118
name: staticcheck
101119
runs-on: ubuntu-latest
120+
timeout-minutes: 15
102121
steps:
103122
- uses: actions/checkout@v7
104123

@@ -126,6 +145,7 @@ jobs:
126145
race:
127146
name: race detector
128147
runs-on: ubuntu-latest
148+
timeout-minutes: 30
129149
env:
130150
# The one thing in this project that needs a C toolchain. Linux runners
131151
# ship one, so this job carries the cost and the matrix above stays on
@@ -156,6 +176,7 @@ jobs:
156176
coverage:
157177
name: coverage gate
158178
runs-on: ubuntu-latest
179+
timeout-minutes: 20
159180
env:
160181
CGO_ENABLED: "0"
161182
steps:
@@ -205,6 +226,8 @@ jobs:
205226
# searching.
206227
if: github.event_name == 'schedule' || github.event_name == 'workflow_dispatch'
207228
runs-on: ubuntu-latest
229+
# Four targets at five minutes each, plus the build and the baseline pass.
230+
timeout-minutes: 45
208231
env:
209232
CGO_ENABLED: "0"
210233
steps:

‎CHANGELOG.md‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,23 @@ because it turns other people's test suites red.
8888

8989
### Changed
9090

91+
- **A size is written the way a person writes it.** `--size 1e5` quietly meant
92+
100000 bytes, while a recipe refuses `1_000` and `0x10` outright on the
93+
grounds that a spelling is never guessed at. The flag now applies the same
94+
rule. `1.5gib` and every other decimal are untouched.
95+
- **`tfg recipe fmt -w` replaces a file in one step**, through a temporary name,
96+
so a process ending mid write no longer leaves the recipe at half its length.
97+
Generated files and the manifest already worked this way.
98+
- **The command recorded in the manifest can be run again.** It was built by
99+
joining the arguments with spaces, so `--name "my file.txt"` came back as two
100+
arguments and described a different run.
101+
- **A run refuses to write through a name something else already occupies.**
102+
Only the final name was checked, and the temporary one is created in a way
103+
that truncates - so a file already sitting under it lost its contents without
104+
a word. A run killed outright leaves exactly that shape behind.
105+
- **`tfg generate --json` reports a manifest that could not be delivered.**
106+
Piping into a command that closes early left the run reporting success with
107+
nothing on standard output.
91108
- **`tfg recipe fmt` says what it does not check.** It settles the layout of a
92109
file and never claimed otherwise in `--help`, but the text implied more than
93110
it did. A recipe with a key nobody recognises still has a settled shape and

‎internal/cli/cli.go‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,28 @@ func (p propertyFlag) Set(v string) error {
230230
return nil
231231
}
232232

233+
// args2 rebuilds the command as it would have to be typed to run again.
234+
//
235+
// It goes into the manifest, where its whole job is to be re-runnable, and it
236+
// was assembled by joining the arguments with spaces. An argument holding a
237+
// space then arrived as two - "--name my file.txt" reads as a name of "my" and
238+
// a stray word - so the recorded command produced a different run, or none.
239+
//
240+
// Quoted only where it is needed, so the common line stays readable. Single
241+
// quotes are avoided because the shells this tool is aimed at disagree about
242+
// them, and double quotes with escaping work in all of them.
233243
func args2(args []string) []string {
234-
out := []string{"generate"}
235-
return append(out, args...)
244+
out := make([]string, 0, len(args)+1)
245+
out = append(out, "generate")
246+
for _, a := range args {
247+
out = append(out, quoteArg(a))
248+
}
249+
return out
250+
}
251+
252+
func quoteArg(a string) string {
253+
if a != "" && !strings.ContainsAny(a, " \t\"\\") {
254+
return a
255+
}
256+
return `"` + strings.NewReplacer(`\`, `\\`, `"`, `\"`).Replace(a) + `"`
236257
}

‎internal/cli/errors.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,15 @@ func classify(err error) int {
151151
if errors.Is(err, context.Canceled) {
152152
return ExitInterrupted
153153
}
154+
// A deadline running out is the same ending as a signal that says time is
155+
// up, and the frozen table tells that apart from somebody cancelling.
156+
// Nothing in the command line sets a deadline today - it is reachable only
157+
// through a caller that does, which the window will be - so this is here
158+
// before it is needed rather than after it has been reported as the tool
159+
// crashing. Without it the ending falls through to RUNTIME.
160+
if errors.Is(err, context.DeadlineExceeded) {
161+
return ExitTerminated
162+
}
154163
var pathErr *os.PathError
155164
if errors.As(err, &pathErr) {
156165
return ExitIO

‎internal/cli/generate.go‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -395,7 +395,14 @@ func produce(ctx context.Context, targets []engine.Target, opt engine.Options, g
395395
fmt.Fprintf(errOut, "tfg: cannot render the manifest: %s\n", describeError(err))
396396
return ExitRuntime
397397
}
398-
out.Write(buf.Bytes())
398+
// The error is read rather than dropped. A closed pipe is the ordinary
399+
// way this fails - "tfg generate --json | head" - and a run that could
400+
// not deliver what was asked for has not succeeded, whatever happened
401+
// on the disk.
402+
if _, err := out.Write(buf.Bytes()); err != nil {
403+
fmt.Fprintf(errOut, "tfg: the files were written and the manifest could not be sent to standard output: %s\n", describeError(err))
404+
return ExitIO
405+
}
399406
}
400407

401408
if res.Failures > 0 {

‎internal/cli/progress.go‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ package cli
44
import (
55
"fmt"
66
"io"
7+
"math"
78
"os"
89
"strings"
910
"time"
@@ -92,9 +93,19 @@ func (p *progressBar) remaining(pr engine.Progress) string {
9293
return " " + roughly(left) + " left"
9394
}
9495

96+
// percent divides before multiplying where it has to, so a very large run does
97+
// not wrap on the way to a number between nought and a hundred.
98+
//
99+
// done*100 leaves the range of an int64 above about 92 PB. No disk holds that
100+
// today, and the arithmetic that produces it is free to be right anyway.
95101
func percent(done, total int64) int {
96-
if total <= 0 {
102+
switch {
103+
case total <= 0:
104+
return 100
105+
case done >= total:
97106
return 100
107+
case done > math.MaxInt64/100:
108+
return int(done / (total / 100))
98109
}
99110
return int(done * 100 / total)
100111
}

‎internal/cli/recipecmd.go‎

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,32 @@ func loadRecipe(path string, errOut io.Writer) (*recipe.Recipe, string, int) {
5252
return rec, hash, ExitOK
5353
}
5454

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+
5581
// validate runs the checks a run would run and writes nothing at all, so it
5682
// suits a pre commit hook.
5783
func validate(args []string, out, errOut io.Writer) int {
@@ -272,7 +298,7 @@ Flags:
272298
fmt.Fprintf(errOut, "%s was already settled and was not touched.\n", path)
273299
return ExitOK
274300
}
275-
if err := os.WriteFile(path, canon, 0o644); err != nil {
301+
if err := replaceFile(path, canon); err != nil {
276302
fmt.Fprintf(errOut, "tfg: cannot write %s: %s\n", path, describeError(err))
277303
return ExitIO
278304
}

‎internal/core/records.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,15 @@ func FillRecords(ctx context.Context, w io.Writer, rng *rand.Rand, remaining int
7979

8080
mark := len(buf)
8181
buf = rec.Append(buf, rng)
82+
// A record that adds nothing would leave remaining untouched and the
83+
// buffer never reaching the chunk size, so this loop would spin without
84+
// end and without growing - the one failure shape a size guard cannot
85+
// see, because no file is ever produced to measure. No builder does it
86+
// today and the interface does not forbid it, so it is named here
87+
// rather than left to a hang somebody has to interrupt.
88+
if len(buf) == mark {
89+
return fmt.Errorf("core: a record of this format added no bytes, so the file could never be filled")
90+
}
8291
// Stop while what is left still fits a whole closing record. That is
8392
// what keeps the last one from being a stub.
8493
if remaining-int64(len(buf)-mark) < shortest {
@@ -133,6 +142,13 @@ func AppendFiller(dst []byte, words []string, n int64, separator func(i int) str
133142
// FillRecords turns that into an error rather than a wrong size.
134143
return dst
135144
}
145+
if len(words) == 0 {
146+
// The loop below indexes words modulo its length, so an empty
147+
// vocabulary divides by zero. No caller passes one today and none
148+
// should: a format with nothing to say cannot pad to a length. Saying
149+
// which mistake it is beats a runtime panic with no name on it.
150+
panic("core: AppendFiller was given no words to pad with")
151+
}
136152

137153
start := len(dst)
138154
for i := 0; int64(len(dst)-start) < n; i++ {

‎internal/core/size.go‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,22 @@ func ParseSize(s string) (int64, error) {
6767
return n * multiplier, nil
6868
}
6969

70+
// Only the spellings a person writes. ParseFloat also takes exponents, hex
71+
// floats and the words for infinity, and none of those describes a file.
72+
//
73+
// Measured on 2026-08-03: "--size 1e5" quietly meant 100000 bytes, while
74+
// the recipe refuses "1_000" and "0x10" outright, with the reasoning that
75+
// guessing at a spelling is the behaviour that type was written to remove.
76+
// Two doors into one idea applying opposite rules, and the difference was
77+
// nobody's decision - it fell out of ParseFloat being permissive. In a size
78+
// field, "1e5" is far likelier to be a typo than an intention.
79+
//
80+
// The decimal point stays, because 1.5gib is a real thing people write.
81+
if !plainDecimal(digits) {
82+
return 0, fmt.Errorf(
83+
"size %q is not a plain number: write the digits out, as 10mb, 1.5gib or 1048576. Spellings such as 1e5, 0x10 or 1_000 are refused rather than guessed at, the same as in a recipe", s)
84+
}
85+
7086
f, err := strconv.ParseFloat(digits, 64)
7187
if err != nil {
7288
return 0, fmt.Errorf("size %q is not a number: write something like 10mb, 1.5gib or 1048576", s)
@@ -96,6 +112,30 @@ func ParseSize(s string) (int64, error) {
96112
return n, nil
97113
}
98114

115+
// plainDecimal reports whether the text is digits, with at most one decimal
116+
// point and an optional leading sign. Nothing else counts as a number here.
117+
//
118+
// Leading zeros are allowed and mean nothing, which matches how the recipe
119+
// reads a number: 010 is ten because that is what it looks like.
120+
func plainDecimal(s string) bool {
121+
s = strings.TrimPrefix(strings.TrimPrefix(s, "+"), "-")
122+
if s == "" {
123+
return false
124+
}
125+
dot, digits := false, 0
126+
for _, r := range s {
127+
switch {
128+
case r >= '0' && r <= '9':
129+
digits++
130+
case r == '.' && !dot:
131+
dot = true
132+
default:
133+
return false
134+
}
135+
}
136+
return digits > 0
137+
}
138+
99139
func unitBytes(unit string) (int64, bool) {
100140
const (
101141
ki = 1024

‎internal/engine/engine.go‎

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -425,14 +425,32 @@ func preflight(files []PlannedFile, opt Options) error {
425425
// Nothing else is written over either. This tool runs in directories that
426426
// belong to the user, so destroying their work is the one failure that
427427
// cannot be undone by running again.
428+
//
429+
// The temporary name is checked as well as the final one. It is created
430+
// with os.Create, which truncates, so a file already sitting under that
431+
// name lost its contents without a word - and the collision check only
432+
// ever looked at the name the file ends up with. It is an unlikely name to
433+
// meet by accident and an easy one to leave behind: a run killed outright
434+
// leaves exactly this, and the next run into the same directory would eat
435+
// it silently.
428436
for _, f := range files {
429437
if path := filepath.Join(opt.OutDir, f.Name); exists(path) {
430438
return &CollisionError{Path: path}
431439
}
440+
if path := tempPathFor(opt.OutDir, f.Name); exists(path) {
441+
return &CollisionError{Path: path}
442+
}
432443
}
433444
return nil
434445
}
435446

447+
// tempPathFor is the name a file is written under before it is renamed into
448+
// place. One definition, used by the check above and by the write below, so
449+
// the two cannot disagree about what is being protected.
450+
func tempPathFor(outDir, name string) string {
451+
return fmt.Sprintf("%s%s%d", filepath.Join(outDir, name), core.PartialMarker, os.Getpid())
452+
}
453+
436454
func exists(path string) bool {
437455
_, err := os.Stat(path)
438456
return err == nil
@@ -499,7 +517,12 @@ func Run(ctx context.Context, files []PlannedFile, opt Options) (*Result, error)
499517
}
500518

501519
sum, err := writeOne(ctx, f, opt.OutDir, report)
502-
bytesDone += f.Plan.Bytes
520+
if err == nil {
521+
// Only what reached the disk. Counting a file that failed would
522+
// have the bar claim bytes nobody can find, and on a run where
523+
// several fail the total would arrive before the files do.
524+
bytesDone += f.Plan.Bytes
525+
}
503526
if opt.OnProgress != nil {
504527
opt.OnProgress(Progress{
505528
FilesDone: i + 1, FilesTotal: len(files),
@@ -532,7 +555,7 @@ func writeOne(ctx context.Context, f PlannedFile, outDir string, report func(int
532555
// not produce, and the bytes of the other had already gone through the same
533556
// handle. The name never survives the run, so nothing about it has to be
534557
// repeatable - and the file it becomes is settled by the plan, not by this.
535-
tmp := fmt.Sprintf("%s%s%d", final, core.PartialMarker, os.Getpid())
558+
tmp := tempPathFor(outDir, f.Name)
536559

537560
fh, err := os.Create(tmp)
538561
if err != nil {

0 commit comments

Comments
 (0)