From 4d66011843a339307123171192b59478f67df1bb Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 2 Sep 2026 12:53:06 +0200 Subject: [PATCH] format: a setting value is spelled the way the format declares it The registry compared a closed set with EqualFold and a boolean with ToLower, so a value the declaration does not contain walked past the one check meant to stop it and arrived at the generator. What happened next was each generator's own decision, and measured across the eight formats that declare a closed set there were four of them: refuse in its own words csv, ico, log, wav fold and understand it pdf page_size, directory_entries in archives swallow it and build the targz entry_owner - USER and ROOT produced a DEFAULT file, exit 0 file byte for byte identical to unset read it as another value zip encryption=NONE, which then asked for a password to lock an archive "with NONE" The third is a file that is not what was ordered, reported as success. Allows now compares exactly, which is one place rather than twenty one settings, and no generator is handed a value its own declaration does not spell. ReadOwnership gains the default branch it never had, because that function is callable directly and silence there would be the same wrong file through another door. And targz records entry_mode and entry_owner in the manifest - neither reached it before, which is why a swallowed owner left nothing to disagree with. Three guards walk the whole registry rather than a list copied beside them: 63 values in a case the declaration does not use, plus the 14 that carry no letters, counted and printed so a day when this guard walks nothing is a day it says so. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 21 +++ internal/format/archive/ownership.go | 42 +++++- internal/format/csvfile/dialect.go | 12 +- internal/format/format.go | 18 ++- internal/format/targz/targz.go | 9 ++ internal/guard/closedsets_test.go | 185 +++++++++++++++++++++++++++ 6 files changed, 279 insertions(+), 8 deletions(-) create mode 100644 internal/guard/closedsets_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 53e5191..f8f92ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -102,8 +102,29 @@ because it turns other people's test suites red. That takes the plain encoder and writes the same bytes this tool wrote before, to the byte - there is a pinned hash proving it. +- **A setting value is spelled the way the format declares it.** `--set + page_size=A4` and `--set directory_entries=TRUE` used to be accepted and now + refuse with exit 4, naming the setting and the value. Write `a4` and `true`. + + Nothing else changes: the bytes of every file are what they were, and a + recipe writing `header: true` is unaffected, because a YAML boolean arrives + as `true` either way. Only a value quoted into another case is refused. + + It is a breaking change for a fourth of a reason and a fix for the rest. A + value the declaration did not contain used to pass the check and land on the + format, which then did one of four things with it: refuse in its own words, + understand it anyway, **quietly ignore it and produce the default file**, or + read it as something else entirely. `--set entry_owner=USER` on a `targz` + wrote an archive owned by nobody and reported success. Now nothing gets that + far. + ### Added +- **A `targz` manifest says what its entries claim about themselves.** Two new + keys on the file entry, `entry_mode` and `entry_owner`, written every run + rather than only when you ask for them, so a harness never has to read a + missing key as "nobody owns this". The manifest schema version is unchanged. + - **A CSV can be written in the dialect you were handed.** `--set delimiter=semicolon`, `--set line_ending=crlf` and `--set header=false` on a `csv`, separately or together. Separators are named rather than typed, so diff --git a/internal/format/archive/ownership.go b/internal/format/archive/ownership.go index e78b8d7..6e237c9 100644 --- a/internal/format/archive/ownership.go +++ b/internal/format/archive/ownership.go @@ -34,6 +34,23 @@ type Ownership struct { // All four are left at nought and empty for the unset owner. Uid, Gid int Uname, Gname string + // Stated is what the settings said, in the words they were written in, for + // the manifest to record. + // + // Carried rather than worked out again at the far end. A mode of 000 turns + // back into "0" rather than "000" through the obvious arithmetic, and an + // owner would have to be recovered from Uname, which is empty for two + // different reasons. A fact the writer already has is cheaper to pass along + // than to reconstruct - ARCHITECTURE.md 5. + Stated Stated +} + +// Stated is the pair of settings as a person wrote them. +type Stated struct { + // Mode is the three octal digits, so 644 stays 644. + Mode string + // Owner is the declared word: root, user or unset. + Owner string } // ReadOwnership reads the two settings that say what an entry records about @@ -45,7 +62,7 @@ type Ownership struct { // a setting where the number a person types is not the number the file gets is // the kind of difference nobody predicts. func ReadOwnership(id string, props map[string]string) (Ownership, error) { - own := Ownership{Mode: 0o644} + own := Ownership{Mode: 0o644, Stated: Stated{Mode: "644", Owner: OwnerUnset}} if raw := props[EntryMode]; raw != "" { mode, err := strconv.ParseInt(raw, 8, 32) @@ -61,16 +78,37 @@ func ReadOwnership(id string, props map[string]string) (Ownership, error) { } } own.Mode = mode + own.Stated.Mode = raw } - switch props[EntryOwner] { + switch raw := props[EntryOwner]; raw { case OwnerRoot: + own.Stated.Owner = OwnerRoot own.Uname, own.Gname = "root", "root" case OwnerUser: // The first ordinary account on a Linux system, which is what somebody // unpacking a fixture on their own machine most likely is. + own.Stated.Owner = OwnerUser own.Uid, own.Gid = 1000, 1000 own.Uname, own.Gname = "user", "user" + case "", OwnerUnset: + // Nothing to record, which is what this format wrote before the setting + // existed. An absent key and the declared word for absent mean the same + // archive. + default: + // There was no default branch here, and the value fell through to an + // archive owned by nobody - the same bytes as unset, reported as + // success. Measured 2026-09-02: entry_owner=USER and entry_owner=ROOT + // each produced a file byte for byte identical to the default one, with + // exit 0 and not a word about it, which is rule 6 broken outright. The + // registry now stops a misspelling before it arrives, so this branch is + // for the other door: this function is callable directly. + return Ownership{}, &format.PropertyValueError{ + Format: id, Key: EntryOwner, Value: raw, + Reason: "it takes one of: " + OwnerRoot + ", " + OwnerUnset + ", " + OwnerUser, + Remedy: "Write it the way the setting is declared, so root or user, " + + "or leave the line out and the entries carry no owner.", + } } return own, nil } diff --git a/internal/format/csvfile/dialect.go b/internal/format/csvfile/dialect.go index fa6bfa8..259517e 100644 --- a/internal/format/csvfile/dialect.go +++ b/internal/format/csvfile/dialect.go @@ -80,10 +80,14 @@ func defaultDialect() dialect { // // A value that is not in the declared set has already been refused by the // registry, which checks it against the declaration for every format at once. -// The refusals here catch what that check lets through: it compares without -// regard for case, so REALISTIC style spellings arrive here rather than being -// stopped there. Written up as O168 - the branches below are reachable through -// that door and only through it, so they are not dead code. +// +// Until 2026-09-02 that check compared without regard for case, so a REALISTIC +// style spelling walked past it and was refused here instead, in different +// words - O168. The registry compares exactly now, so nothing arriving through +// a recipe or a flag can reach these branches. They stay because this function +// is callable directly and a guard calling it is such a caller, and because a +// generator that trusts its input is one registry change away from writing a +// file nobody ordered. func parseDialect(props map[string]string) (dialect, error) { d := defaultDialect() diff --git a/internal/format/format.go b/internal/format/format.go index d551548..29540a8 100644 --- a/internal/format/format.go +++ b/internal/format/format.go @@ -385,14 +385,28 @@ func (p Property) Allows(raw string) (bad string) { } } case PropertyChoice: + // Spelled the way it is declared, not merely close to it. This used to + // fold with EqualFold, and the folding was invisible here and decisive + // three layers down: a value the declaration does not contain reached + // the generator, and each generator did something different with it. + // Measured 2026-09-02 across the eight formats with a closed set, four + // answers - a refusal in the generator's own words, a fold that + // understood it, a swallow that made the DEFAULT file and reported + // success, and a misread that demanded a password to lock an archive + // with "NONE". O168. One place to say no is the whole point of + // declaring the set here. for _, c := range p.Choices { - if strings.EqualFold(raw, c) { + if raw == c { return "" } } return "it takes one of: " + strings.Join(p.Choices, ", ") case PropertyBool: - switch strings.ToLower(raw) { + // Exact for the same reason, and it costs a user nothing: a recipe + // writing `header: True` arrives as "true" already, because the reader + // puts a YAML boolean through FormatBool. Only a hand quoted "TRUE" + // changes, and it changes into a refusal that names the setting. + switch raw { case "true", "false": default: return "it takes true or false" diff --git a/internal/format/targz/targz.go b/internal/format/targz/targz.go index 07e425c..6c07861 100644 --- a/internal/format/targz/targz.go +++ b/internal/format/targz/targz.go @@ -334,6 +334,15 @@ func describe(target int64, label string, m memo, groups []format.Content) forma // never has to read a missing key as flat. archive.Depth: m.layout.Depth, archive.DirectoryEntries: m.layout.DirEntries, + // What every entry says about itself. Neither of these reached the + // manifest until 2026-09-02, and the gap is what made a swallowed + // owner invisible: a run asking for entry_owner=USER produced an + // archive owned by nobody and a manifest identical to the one that + // asked for user, so there was nothing to disagree with. Written + // every time rather than only when stated, like depth and + // compression beside them. + archive.EntryMode: m.own.Stated.Mode, + archive.EntryOwner: m.own.Stated.Owner, // How hard the stream was squeezed. This key used to be the // constant "none", written when the format could only store - and // the comment beside it said so, which is why it is worth saying diff --git a/internal/guard/closedsets_test.go b/internal/guard/closedsets_test.go new file mode 100644 index 0000000..b95351e --- /dev/null +++ b/internal/guard/closedsets_test.go @@ -0,0 +1,185 @@ +package guard + +import ( + "errors" + "strings" + "testing" + + "github.com/donislawdev/TestingFilesGenerator/internal/format" + _ "github.com/donislawdev/TestingFilesGenerator/internal/format/all" + "github.com/donislawdev/TestingFilesGenerator/internal/format/archive" +) + +// A closed set is closed, and the registry is what says so. +// +// It did not. Property.Allows compared a choice with EqualFold and a boolean +// with ToLower, so a value spelled in a case the declaration does not use went +// straight past the registry and arrived at the generator - and what happened +// next was decided by whichever generator caught it. Measured on 2026-09-02 +// across all eight formats that declare a closed set, twenty one settings, +// there were FOUR different answers and not one of them was declared: +// +// refuses in its own words csv, ico, log, wav - the two voices of O168 +// folds and understands it pdf page_size, directory_entries in both archives +// swallows it and makes the targz entry_owner - USER and ROOT come out byte +// DEFAULT file, exit 0 for byte identical to unset, with nothing said +// reads it as another value zip encryption=NONE, which then demands a +// password "to lock with NONE" +// +// The third one is rule 6 broken outright: a file that is not what was ordered, +// reported as success. The fourth quotes a value the format does not have. +// +// So the fix is one place rather than twenty one: the registry stops folding, +// and no generator is ever handed a value its declaration does not contain. +// These guards are the reason that stays true for the format added tomorrow. +func TestAValueTheDeclarationDoesNotSpellIsRefusedByTheRegistry(t *testing.T) { + asked, skipped := 0, 0 + + for _, d := range format.All() { + for _, p := range d.Properties { + values := p.Choices + if p.Kind == format.PropertyBool { + values = []string{"true", "false"} + } + if p.Kind != format.PropertyChoice && p.Kind != format.PropertyBool { + continue + } + for _, v := range values { + shouted := strings.ToUpper(v) + if shouted == v { + // A value with no letters in it - a permission like 644, a + // bit depth like 16 - cannot be spelled in another case at + // all. Counted rather than passed over silently, because a + // day when every value is a number is a day this guard + // walks nothing and says so. + skipped++ + continue + } + asked++ + t.Run(d.ID+" "+p.Name+"="+shouted, func(t *testing.T) { + err := d.CheckProperties(map[string]string{p.Name: shouted}) + if err == nil { + t.Fatalf("%s took %s=%s, which its declaration spells %q. "+ + "Whatever the generator does with it next is its own "+ + "decision, and there are four of those in this tree", + d.ID, p.Name, shouted, v) + } + + // Refused is not enough. It has to be refused HERE, in the + // declaration's words, or the two voices are back with one + // of them further away. + var bad *format.PropertyValueError + if !errors.As(err, &bad) { + t.Fatalf("%s refused %s=%s with %T, not with the "+ + "registry's own refusal: %v", d.ID, p.Name, shouted, err, err) + } + if bad.Key != p.Name { + t.Errorf("the refusal names %q, and the setting is %q", bad.Key, p.Name) + } + if bad.Value != shouted { + t.Errorf("the refusal quotes %q, and the value was %q", bad.Value, shouted) + } + // The declared spelling still works, or this guard would be + // green on a registry that refuses everything. + if err := d.CheckProperties(map[string]string{p.Name: v}); err != nil { + t.Errorf("%s refuses %s=%s, which is its own declared value: %v", + d.ID, p.Name, v, err) + } + }) + } + } + } + + if asked == 0 { + t.Fatalf("no closed set carried a letter, so this guard asked nothing "+ + "(%d values had no letters to shout)", skipped) + } + t.Logf("%d values asked in a case the declaration does not use, %d had no letters", asked, skipped) +} + +// An owner the archive does not know is refused, rather than quietly becoming +// no owner at all. +// +// ReadOwnership was a switch with no default: anything that was not root or +// user fell through to the zero value, which is the same archive as unset. The +// registry now stops a misspelling before it gets here, so this is about the +// other door - the function is callable directly, and a guard calling it is +// exactly such a caller. Silence there would be a file that is not what was +// asked for, which is rule 6 whichever door it came through. +func TestAnOwnerTheArchiveDoesNotKnowIsRefusedRatherThanDropped(t *testing.T) { + for _, raw := range []string{"USER", "nobody", "root "} { + t.Run(raw, func(t *testing.T) { + own, err := archive.ReadOwnership("targz", map[string]string{archive.EntryOwner: raw}) + if err == nil { + t.Fatalf("an owner of %q was taken and turned into %+v, so the archive "+ + "is not the one that was ordered and nothing said so", raw, own) + } + var bad *format.PropertyValueError + if !errors.As(err, &bad) { + t.Fatalf("refused %q with %T, not with the refusal every other "+ + "setting uses: %v", raw, err, err) + } + if bad.Key != archive.EntryOwner { + t.Errorf("the refusal names %q, and the setting is %q", bad.Key, archive.EntryOwner) + } + }) + } + + // Every declared owner still passes, or the guard above would be satisfied + // by a function that refuses everything. + for _, owner := range []string{archive.OwnerRoot, archive.OwnerUser, archive.OwnerUnset} { + if _, err := archive.ReadOwnership("targz", map[string]string{archive.EntryOwner: owner}); err != nil { + t.Errorf("%q is a declared owner and was refused: %v", owner, err) + } + } + // And an absent setting is not a misspelling. It is the common case. + if _, err := archive.ReadOwnership("targz", map[string]string{}); err != nil { + t.Errorf("an archive with no owner setting was refused: %v", err) + } +} + +// The manifest says who owns the entries and what mode they carry. +// +// It said neither, and that is why the swallowed owner above was invisible: the +// file was wrong, the run said success, and the one document a test suite reads +// had nothing to disagree with. Measured on 2026-09-02 - a run with +// entry_owner=user and one with entry_owner=USER produced manifests that were +// identical, because both were silent. +// +// Written every time rather than only when set, like depth and compression +// beside them, so a harness never has to read a missing key as "nobody owns +// this". +func TestTheArchiveManifestSaysWhoOwnsTheEntries(t *testing.T) { + d, err := format.Get("targz") + if err != nil { + t.Fatal(err) + } + + for _, tc := range []struct { + props map[string]string + mode, wner string + }{ + {map[string]string{}, "644", archive.OwnerUnset}, + {map[string]string{archive.EntryOwner: archive.OwnerRoot}, "644", archive.OwnerRoot}, + {map[string]string{archive.EntryMode: "755", archive.EntryOwner: archive.OwnerUser}, + "755", archive.OwnerUser}, + } { + name := tc.mode + "/" + tc.wner + t.Run(name, func(t *testing.T) { + p, err := d.Generator.Plan(format.Request{ + Bytes: 20480, Seed: 3, Label: true, Properties: tc.props, + }) + if err != nil { + t.Fatalf("planning %v: %v", tc.props, err) + } + if got := p.Properties[archive.EntryMode]; got != tc.mode { + t.Errorf("the manifest says entry_mode %v, and the archive was built with %s", + got, tc.mode) + } + if got := p.Properties[archive.EntryOwner]; got != tc.wner { + t.Errorf("the manifest says entry_owner %v, and the archive was built with %s", + got, tc.wner) + } + }) + } +}