From d76079753fe2fd37fa879097f0f3e0394e70f77f Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Wed, 2 Sep 2026 16:06:31 +0200 Subject: [PATCH] format: the sentence saying which words a setting takes is written in one place Two places in the archive package listed the values of a closed set by hand, in the registry's order and the registry's phrasing, for compression and for entry_owner. The registry builds that sentence from the declared set, so these were copies of it with nothing comparing the two. Identical copies are the dangerous kind, because nothing goes red while they agree - and they agreed. The day they stop agreeing is the day somebody adds a value to the declaration: the registry lists it, tfg formats prints it, the window offers it, and the hand written line does not. The second copy arrived through the change that closed the first half of this, which is the clearest evidence available that this copies itself faster than anybody notices. Both now ask the declaration. Nothing a person sees changes: a bad value is refused by the registry first, with exit 4, in the registry's own words, and these branches sit behind that check for a direct caller. Three guards, each proven by mutation. The sentence is built in one place, read from string literals rather than file text so a comment may still name it. A refusal quotes the registry, which catches a copy that was reworded rather than repeated. And every word the declaration offers is one its reader accepts, which closes the hole the fix itself opens: Allows says nothing is wrong with a value it allows, so a word added to the set and not handled by the reader would be refused with an empty reason. Co-Authored-By: Claude Opus 5 --- internal/format/archive/compression.go | 14 +- internal/format/archive/ownership.go | 8 +- internal/guard/archivewording_test.go | 245 +++++++++++++++++++++++++ 3 files changed, 265 insertions(+), 2 deletions(-) create mode 100644 internal/guard/archivewording_test.go diff --git a/internal/format/archive/compression.go b/internal/format/archive/compression.go index e8c46b5..915e387 100644 --- a/internal/format/archive/compression.go +++ b/internal/format/archive/compression.go @@ -78,7 +78,19 @@ func ReadCompression(id string, r format.Request, locked bool) (Squeeze, error) Format: id, Key: Compression, Value: raw, - Reason: "it takes one of: " + CompressBest + ", " + CompressDefault + ", " + CompressFast + ", " + CompressNone, + // Asked of the declaration rather than written out again. The + // words used to be listed here by hand, in the registry's order + // and the registry's phrasing, and identical is how a copy looks + // right up until it is not - O171. A fifth level added to the + // declaration would have reached tfg formats, reached the window, + // reached the registry's refusal, and not reached this line. + // + // This is empty for a value the declaration allows, and a value + // the declaration allows but levels has no entry for would arrive + // here. That pair cannot come apart without a guard going red: + // every declared word is put through this function and has to be + // accepted. + Reason: axes[Compression].Allows(raw), Remedy: "Ask for " + CompressNone + " to store the files as they are.", } } diff --git a/internal/format/archive/ownership.go b/internal/format/archive/ownership.go index 6e237c9..d38d2b6 100644 --- a/internal/format/archive/ownership.go +++ b/internal/format/archive/ownership.go @@ -105,7 +105,13 @@ func ReadOwnership(id string, props map[string]string) (Ownership, error) { // 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, + // From the declaration, for the reason the same branch in + // compression.go gives: a hand written list is a copy of the + // registry's sentence with nothing comparing the two - O171. + // A word added to the declaration and not to this switch lands + // here, and the guard that puts every declared word through this + // function is what stops that arriving as an empty reason. + Reason: axes[EntryOwner].Allows(raw), Remedy: "Write it the way the setting is declared, so root or user, " + "or leave the line out and the entries carry no owner.", } diff --git a/internal/guard/archivewording_test.go b/internal/guard/archivewording_test.go new file mode 100644 index 0000000..8ecd261 --- /dev/null +++ b/internal/guard/archivewording_test.go @@ -0,0 +1,245 @@ +package guard + +import ( + "errors" + "fmt" + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/donislawdev/TestingFilesGenerator/internal/format" + "github.com/donislawdev/TestingFilesGenerator/internal/format/archive" +) + +// What these defend. A setting with a closed set of values is refused in one +// voice, and the voice is the declaration's. +// +// Why they were written. Measured 2026-09-02 while closing O168, and then +// again while closing O171: three places in the tree built the sentence that +// says which words a setting takes. One was the registry, which builds it from +// the declared set. The other two were the archive package writing the same +// list out by hand, in the same order and the same phrasing, for compression +// and for entry_owner - and the second of those was added by the very change +// that closed O168, which is the clearest evidence available that this copies +// itself faster than anybody notices. +// +// Why identical copies are the dangerous kind. Nothing goes red while they +// agree, and they agreed. The day they stop agreeing is the day somebody adds +// a fifth compression level to the declaration: the registry lists five, tfg +// formats prints five, the window offers five, and the hand written line lists +// four. Nobody set out to write two answers - one line simply did not get +// edited, and no test in this repository could have said so. +// +// Why the user never saw it, and why that is not a reason to leave it. Measured +// 2026-09-02 on the built binary: --set compression=TURBO and --set +// entry_owner=USER are both refused by the REGISTRY, exit 4, in the registry's +// words. The hand written branches sit behind that check and no surface reaches +// them. They are there for a direct caller, which is what a guard is - so they +// can be reddened, which is the test this project applies before calling +// anything a defence. +// +// What they do NOT check. Whether the sentence reads well. Three tests can say +// it is built once, that a refusal quotes it, and that every word in it works. +// None of them can say it is the right sentence to put in front of a person. + +// One place builds the sentence, and it is the registry. +// +// String literals rather than the file text, the same choice plurals_test.go +// makes and for the same reason: a comment explaining this rule has to be able +// to name the sentence, and reading raw bytes could not tell a comment from a +// message. The parser can. +func TestTheSentenceNamingWhichWordsASettingTakesIsWrittenInOnePlace(t *testing.T) { + root := repoRoot(t) + + // The stem rather than the whole sentence. The registry writes it with a + // colon and a trailing space, and a copy that dropped the colon would be + // exactly as much of a copy. + const sentence = "it takes one of" + + // The one place allowed to say it, written with forward slashes so this + // reads the same on every system. + const home = "internal/format/format.go" + + var found []string + files := 0 + for _, dir := range []string{"internal", "cmd"} { + start := filepath.Join(root, dir) + err := filepath.WalkDir(start, func(path string, d os.DirEntry, err error) error { + if err != nil || d.IsDir() { + return err + } + // Guards quote the sentences they assert on, so a guard reading + // guards would fail on its own evidence. + if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + files++ + + fset := token.NewFileSet() + parsed, perr := parser.ParseFile(fset, path, nil, 0) + if perr != nil { + t.Errorf("parsing %s: %v", path, perr) + return nil + } + rel, _ := filepath.Rel(root, path) + rel = filepath.ToSlash(rel) + + ast.Inspect(parsed, func(n ast.Node) bool { + lit, isLit := n.(*ast.BasicLit) + if !isLit || lit.Kind != token.STRING { + return true + } + if strings.Contains(lit.Value, sentence) { + found = append(found, fmt.Sprintf("%s:%d", rel, fset.Position(lit.Pos()).Line)) + } + return true + }) + return nil + }) + if err != nil { + t.Fatalf("walking %s: %v", start, err) + } + } + + // A walk that read nothing is a green test about nothing. This has happened + // in this repository often enough to be written down as a trap. + if files == 0 { + t.Fatal("no source file was read, so this proved nothing") + } + + switch { + case len(found) == 0: + t.Fatalf("nothing in the tree says %q, so either the registry stopped saying it "+ + "or this guard is looking for the wrong words. Both are worth knowing: "+ + "the sentence is what a person reads when a setting is refused.", sentence) + case len(found) > 1: + t.Errorf("%d places say %q and there may only be one, which is %s.\n"+ + " %s\n"+ + "A format that wants this sentence asks its declaration for it - Property.Allows "+ + "builds it from the declared set, so a value added to the set reaches the sentence "+ + "on its own. A list written out by hand is a second answer to one question, and it "+ + "agrees with the first until the day somebody adds a value (O171).", + len(found), sentence, home, strings.Join(found, "\n ")) + case !strings.HasPrefix(found[0], home+":"): + t.Errorf("the one place saying %q is %s, and it should be %s.\n"+ + "The sentence belongs where the declared set is read, so that every format "+ + "refuses in the same words.", sentence, found[0], home) + } +} + +// A refusal quotes the registry, whatever the wording is that day. +// +// This is the half that survives a rewording. The test above forbids the exact +// list coming back. This one forbids a differently worded copy, because a +// second sentence saying the same thing is the same defect wearing other words. +func TestAnArchiveRefusesAValueInTheWordsTheRegistryBuilds(t *testing.T) { + t.Run(archive.Compression, func(t *testing.T) { + const bad = "turbo" + _, err := archive.ReadCompression("zip", format.Request{ + Properties: map[string]string{archive.Compression: bad}, + }, false) + refusedInTheRegistrysWords(t, err, archive.Compression, bad) + }) + + t.Run(archive.EntryOwner, func(t *testing.T) { + const bad = "nobody" + _, err := archive.ReadOwnership("targz", map[string]string{archive.EntryOwner: bad}) + refusedInTheRegistrysWords(t, err, archive.EntryOwner, bad) + }) +} + +// Every word the declaration offers is a word its reader takes. +// +// This is what the fix above newly depends on, so it is what the fix newly has +// to be guarded against. The sentence now comes from the declared set, which +// means a value added to the set is named as allowed by the refusal that +// refuses it - "compression cannot be turbo, it takes one of: best, default, +// fast, none, turbo" - and worse, a reader with no branch for it returns an +// EMPTY reason, because Allows says nothing is wrong with a value it allows. +// +// Asked of the declaration rather than of a list written here, so a fourth +// closed set added to the archive package is covered on the day it arrives. +func TestEveryWordTheArchiveDeclarationOffersIsOneItsReaderAccepts(t *testing.T) { + readers := map[string]func(word string) error{ + archive.Compression: func(word string) error { + _, err := archive.ReadCompression("zip", format.Request{ + Properties: map[string]string{archive.Compression: word}, + }, false) + return err + }, + archive.EntryOwner: func(word string) error { + _, err := archive.ReadOwnership("targz", map[string]string{archive.EntryOwner: word}) + return err + }, + archive.EntryMode: func(word string) error { + _, err := archive.ReadOwnership("targz", map[string]string{archive.EntryMode: word}) + return err + }, + } + + for key, read := range readers { + t.Run(key, func(t *testing.T) { + p := archive.Axes(key)[0] + if p.Kind != format.PropertyChoice { + t.Fatalf("%s is declared as %v rather than a closed set, so this test is "+ + "asking the wrong question about it", key, p.Kind) + } + // An empty set would make the loop below pass without running, which + // is the shape of a green test that proves nothing. + if len(p.Choices) == 0 { + t.Fatalf("%s declares no values, so the loop over them proved nothing", key) + } + + for _, word := range p.Choices { + if err := read(word); err != nil { + t.Errorf("the declaration offers %s=%q and the reader refuses it: %v\n"+ + "Both halves are read by a person: the window draws this word in a menu "+ + "and tfg formats prints it, so a word the reader will not take is a "+ + "setting that is offered and does not work.", key, word, err) + } + } + }) + } +} + +// refusedInTheRegistrysWords checks that err is the refusal that names a +// setting, and that its reason is the one the declaration builds. +func refusedInTheRegistrysWords(t *testing.T, err error, key, bad string) { + t.Helper() + + p := archive.Axes(key)[0] + + // If the value this test picked has since been declared, the comparison + // below would hold two empty strings against each other and pass. Naming + // that rather than letting it happen: a test whose evidence has evaporated + // reports success in exactly the same words as a test that worked. + want := p.Allows(bad) + if want == "" { + t.Fatalf("%s=%q is a value the declaration now allows, so this test no longer has "+ + "a refusal to read. Pick a value the declaration does not contain.", key, bad) + } + + if err == nil { + t.Fatalf("%s=%q was accepted, and the declaration does not allow it", key, bad) + } + + var value *format.PropertyValueError + if !errors.As(err, &value) { + t.Fatalf("%s=%q was refused as %T rather than as the refusal that names a setting, "+ + "so nothing downstream can say which field to point at: %v", key, bad, err, err) + } + + if value.Reason != want { + t.Errorf("%s=%q is refused in words of its own rather than the declaration's.\n"+ + " refusal says: %q\n"+ + " declaration says: %q\n"+ + "Ask the declaration - Property.Allows builds this from the declared set, so the "+ + "two cannot come apart. Written out by hand they agree until somebody adds a "+ + "value to the set and edits one of them (O171).", + key, bad, value.Reason, want) + } +}