From ba7677e1a6446c7f575b1bb37fe0fddd08c0b8bf Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Mon, 7 Sep 2026 06:44:19 +0200 Subject: [PATCH] fix: a refusal about a size names the key the recipe actually carries The format is handed a count of bytes and answers about bytes, so every size refusal it produces says "size" - the only size key it knows. A target that draws its sizes from a range has no "size" key at all. Measured on 2026-09-06: a recipe carrying "size-range: 143-200" was refused at "targets[1].size", which sent a script reading validate --json, and a person reading the window, to a box that was neither on their screen nor in their file. It now reads "targets[1].size-range", and ordinary targets are unchanged. The substitution happens in atTarget, where the target is in hand. The format cannot make it, because it never learns which key held the number. Nothing else moved, and that was measured rather than reasoned about: refusalcorpus reports 96 identical and 0 differing across 48 recipes and two commands, and bytesweep reports all 23 formats byte for byte identical. The comment on drawSizes is corrected in the same commit, because it promised an invariant the code does not provide. It said a range "either works for every file or for none". That holds only if a format's reachable sizes are one unbroken interval from its minimum, and for four of them they are not - PNG has an unreachable band of eleven byte counts above every picture's encoded size, and the OPC three declare the same shape. So a drawn size can land in a band and be refused later, and whether that happens depends on the count: one 64x64 PNG recipe at one seed with size-range 143-200 is accepted at counts 1 and 2 and refused at 3, 5, 8, 12, 20 and 40. Closing that needs the format to declare its bands so the whole interval can be judged before anything is drawn, which changes format.Descriptor and is the owner's call. This commit does not do it, and the comment no longer claims otherwise. Two mutations in opposite directions, both caught. The pair matters more than either half: a build answering "size-range" for every size refusal would satisfy the first case and misaddress every ordinary target in the tree. Checked rather than assumed that this was not already guarded - the table in TestEveryRecipeRefusalSaysWhichSettingItIsAbout has a size-range case, but that refusal comes from the recipe reader, which knows which key it read. Both cases were tried there first and came back valid, because nothing on that path plans a file. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 9 ++++ internal/core/setting.go | 14 +++++ internal/engine/engine.go | 29 ++++++++-- internal/engine/errors.go | 11 +++- internal/engine/plantarget.go | 4 +- internal/guard/sizerange_test.go | 91 ++++++++++++++++++++++++++++++++ internal/recipe/compose.go | 4 +- 7 files changed, 154 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 88474a9..79069de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -299,6 +299,15 @@ because it turns other people's test suites red. ### Fixed +- **A refusal about a size now names the setting you actually wrote.** A target + using `size-range` was refused at `targets[1].size`, a key that recipe does + not have, so `validate --json` sent a script - and the window sent a person - + to a box that was not there. It now reads `targets[1].size-range`. Targets + using `size` are unchanged. + + The wording of every refusal is byte for byte what it was. Only the address + moved. + - **`verify` no longer calls a half-written manifest a file it knows nothing about.** A run killed outright can leave `.tfg-writing` behind. `verify` reported it as `extra`, the word it uses for a file somebody else put diff --git a/internal/core/setting.go b/internal/core/setting.go index de310e4..e9b64d1 100644 --- a/internal/core/setting.go +++ b/internal/core/setting.go @@ -84,6 +84,20 @@ func articleFor(name string) string { // than either surface's own word for the same thing. const SettingSize = "size" +// SettingSizeRange is the recipe key a refusal about a RANGE of sizes is about. +// +// A target that draws its sizes from a range has no "size" key at all, so an +// address naming one sends somebody to a box that is not on their screen and +// not in their file. Measured on 2026-09-06: a recipe carrying "size-range: +// 143-200" was refused with "at": "targets[1].size", naming a setting the +// person had not written. +// +// The refusal itself comes from the format, which knows only that a number of +// bytes is out of reach and cannot know which key carried that number. So the +// substitution happens where the target is in hand - see atTarget in the +// engine. +const SettingSizeRange = "size-range" + // SettingErrorf is a refusal that names the setting it is about through a slot, // so each surface reads it in its own words. // diff --git a/internal/engine/engine.go b/internal/engine/engine.go index 2a02fb7..c0cf03c 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -78,8 +78,31 @@ type Target struct { // container was told to hold - would otherwise fail on some runs and not // others, depending on what came out of the seed. A tool whose whole promise // is that the same seed gives the same run cannot have an error that appears -// and disappears. So the low end is planned first and the range either works -// for every file or for none. +// and disappears. +// +// THE LOW END IS NOT THE WHOLE ANSWER, and this comment claimed it was +// until 2026-09-06. It said the range "either works for every file or for +// none", and the code does not provide that. The check here is sufficient only +// if a format's reachable sizes are one unbroken interval starting at its +// minimum, and for four of them they are not: PNG has an unreachable band of +// eleven byte counts immediately above every picture's encoded size, because +// the smallest padding chunk costs twelve bytes, and the OPC three declare the +// same shape between the comment capacity and the smallest extra part. +// +// So a size DRAWN into such a band is refused later, by the per file plan, and +// whether that happens depends on the count. Measured on 2026-09-06, one 64x64 +// PNG recipe at one seed with size-range 143-200: counts 1 and 2 are accepted, +// counts 3, 5, 8, 12, 20 and 40 are refused. The low end moves with the seed +// too - 144, 143, 144 B at seeds 1, 2 and 3 - so judging file 0's band says +// nothing about file 2's. +// +// The bytes are stable under a raised count and that was verified, so rule 2 +// holds for CONTENT. What is not stable is whether the run happens at all. +// Closing that needs the format to declare its unreachable bands so the whole +// interval can be judged before anything is drawn, which is a change to +// format.Descriptor and the owner's call. Until then the refusal at least +// names the key the recipe carries - see atTarget - rather than pointing at a +// "size" setting a range target does not have. // // The judge is the generator itself rather than a second copy of its rules // here. A copy would be a place for the two to disagree, and the disagreement @@ -392,7 +415,7 @@ func PlanContext(ctx context.Context, targets []Target, opt Options) ([]PlannedF // deliberately leaves alone. desc, err := settleTarget(t, opt, seen) if err != nil { - return nil, atTarget(i+1, err) + return nil, atTarget(i+1, t, err) } targetSeed := core.TargetSeed(opt.Seed, t.ID) diff --git a/internal/engine/errors.go b/internal/engine/errors.go index 6ec192b..08917b4 100644 --- a/internal/engine/errors.go +++ b/internal/engine/errors.go @@ -152,7 +152,7 @@ const ( // Inventing a position for it would put a message about the whole run under // one batch of twenty, which is worse than leaving it at the foot of the form // where a message about the run belongs. -func atTarget(position int, err error) error { +func atTarget(position int, t *Target, err error) error { var about interface{ AboutSetting() string } if !errors.As(err, &about) || about.AboutSetting() == "" { return err @@ -163,6 +163,15 @@ func atTarget(position int, err error) error { if core.AddressNamesATarget(setting) { return err } + // A refusal about a size belongs to the key that carried the number. The + // format cannot know which one that was - it is handed a count of bytes and + // answers about bytes - so the substitution happens here, where the target + // is in hand. Without it a recipe written with "size-range" is refused at + // "targets[1].size", which is a box that is not on the screen and not in + // the file. Measured on 2026-09-06. + if setting == core.SettingSize && t != nil && t.SizeIsRange { + setting = core.SettingSizeRange + } return &addressedError{err: err, at: core.TargetAddress(position, setting)} } diff --git a/internal/engine/plantarget.go b/internal/engine/plantarget.go index 3b3bd73..f47162b 100644 --- a/internal/engine/plantarget.go +++ b/internal/engine/plantarget.go @@ -44,12 +44,12 @@ func (pl *planning) files(ctx context.Context, t *Target, desc format.Descriptor Properties: t.Properties, }) if err != nil { - return atTarget(position, err) + return atTarget(position, t, err) } name, err := renderName(t, desc, idx) if err != nil { - return atTarget(position, err) + return atTarget(position, t, err) } // Two files heading for one name means one of them would be // destroyed by the other, and the manifest would still describe diff --git a/internal/guard/sizerange_test.go b/internal/guard/sizerange_test.go index fc9a203..b50da46 100644 --- a/internal/guard/sizerange_test.go +++ b/internal/guard/sizerange_test.go @@ -1,8 +1,11 @@ package guard import ( + "bytes" + "context" "crypto/sha256" "encoding/hex" + "encoding/json" "os" "path/filepath" "strings" @@ -321,3 +324,91 @@ targets: t.Errorf("all six archives came out the same size, so nothing is being drawn") } } + +// A refusal about a size names the key the recipe actually carries. +// +// The format is handed a count of bytes and answers about bytes, so every size +// refusal it produces says "size" - the only size key it knows. A target that +// draws from a range has no "size" key at all. Measured on 2026-09-06: a recipe +// carrying "size-range: 143-200" was refused at "targets[1].size", sending +// somebody to a box that was neither on their screen nor in their file. +// +// NOT COVERED BY TestEveryRecipeRefusalSaysWhichSettingItIsAbout, and that was +// checked rather than assumed. Its table has a "size-range" case already, but +// that refusal comes from the recipe READER, which knows which key it was +// reading. This one comes from underneath, and the table's runner never reaches +// it - the two cases were tried there first and the recipe came back valid, +// because nothing in that path plans a file. +// +// The pair matters more than either half. A build that answered "size-range" +// for every size refusal would pass the first case and misaddress every +// ordinary target in the tree. +func TestASizeRefusalIsAddressedToTheKeyTheRecipeCarries(t *testing.T) { + cases := []struct { + name string + src string + want string + }{ + { + name: "a range the format cannot deliver", + src: `version: 1 +targets: + - id: a + format: pdf + count: 40 + size-range: 10-8kb +`, + want: "targets[1].size-range", + }, + { + name: "a plain size the format cannot deliver", + src: `version: 1 +targets: + - id: a + format: pdf + count: 1 + size: 10 +`, + want: "targets[1].size", + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + dir := t.TempDir() + path := writeRecipe(t, dir, c.src) + + var out, errOut bytes.Buffer + if code := cli.Run(context.Background(), + []string{"validate", path, "--json"}, &out, &errOut); code == cli.ExitOK { + t.Fatalf("this recipe was meant to be refused and validate was happy with it:\n%s", + out.String()) + } + + var report struct { + Problems []struct { + What string `json:"what"` + At string `json:"at"` + } `json:"problems"` + } + if err := json.Unmarshal(errOut.Bytes(), &report); err != nil { + t.Fatalf("the report is not readable as JSON: %v\n%s", err, errOut.String()) + } + // Asserted rather than assumed. A report with no problems in it + // would pass the loop below by never entering it. + if len(report.Problems) == 0 { + t.Fatalf("the report carries no problems at all:\n%s", errOut.String()) + } + + for _, pr := range report.Problems { + if pr.At == c.want { + continue + } + t.Errorf("the refusal %q is addressed to %q and belongs at %q.\n"+ + "What to do: a refusal about a size carries the key that held the number, "+ + "so a window can mark the box somebody can actually change.", + pr.What, pr.At, c.want) + } + }) + } +} diff --git a/internal/recipe/compose.go b/internal/recipe/compose.go index 59a3786..0dd7bfe 100644 --- a/internal/recipe/compose.go +++ b/internal/recipe/compose.go @@ -322,8 +322,8 @@ const ( KeyID = "id" KeyFormat = "format" KeyCount = "count" - KeySize = "size" - KeySizeRange = "size-range" + KeySize = core.SettingSize + KeySizeRange = core.SettingSizeRange KeyBoundary = "boundary" KeyName = "name" KeyGroup = "group"