[ConfigManager] Sei Config File - #3952
Open
bdchatham wants to merge 17 commits into
Open
Conversation
The file holds only what an operator decided, and editing it preserves the document, so the comments they wrote to explain a choice survive a later set or unset. Every save is atomic, since a node cannot boot from a file a crash truncated mid-write. Three top-level keys describe the file rather than configure the node, and Values leaves all three out so a check comparing written keys against a declared set never reports them as keys no section owns. Four defects found while covering the value decoder, each verified by reverting the fix and watching the covering test fail: - a multi-line basic string could not be decoded at all. Unquote reads Go's single-line syntax, so the literal newlines that make the form multi-line have to be escaped before it sees them. Left raw, a value an operator wrote was refused for having more than one line in it. - an integral float was written as "1", which reads back as an integer. TOML tells a float from an integer by the fractional part, so a key declared as a float resolved as one type from a node's own files and another from its sei.toml, and which of the two an operator got depended on the value they chose: 0.5 survived and 1.0 did not. - an infinity or a NaN could be read but not written. TOML spells them as words and ParseFloat accepts them, so a file could hold one, and any later edit of any other key failed on a value this package had handed back. Both directions now refuse. - writing back a list read from a file failed, because reading an array produces a list of any and only a list of string could be written. Narrower integer widths are refused rather than rendered. The cases are the widths configuration structs in this tree declare, so int8 through uint16 and float32 are a named refusal until a field needs one. Coverage is 94.2% of statements. The sixteen uncovered statements are all error propagations from operations that cannot fail on valid input: a mapping insert the caller already proved absent, a document render, six syscalls on a temporary file just created, and three decodings the parser already validated. Reaching the syscalls means an injectable filesystem, and the atomicity those lines serve is held by three tests that drive it end to end. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3952 +/- ##
==========================================
- Coverage 59.69% 58.63% -1.06%
==========================================
Files 2331 2234 -97
Lines 200163 189050 -11113
==========================================
- Hits 119489 110858 -8631
+ Misses 69278 67659 -1619
+ Partials 11396 10533 -863
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
…cannot hold Two independent reviews of this package found a class of defect this commit closes at its cause. Every fix is verified by reverting it and watching the covering test fail. The decoder used Go's string grammar where TOML's was needed. strconv.Unquote and strconv.Quote implement a different language from the one the file is written in, and the same dependency exports scanner.Unescape and scanner.Escape for the right one. Four defects followed from that single substitution: - a multi-line basic string containing an escaped quote made Values return nothing for the entire file, because the decoder escaped a quote the operator had already escaped - a backslash ending a line decoded to the characters backslash and n, where TOML folds the line break away - a file saved on Windows carried its carriage returns into every multi-line value, so a value differed from the default it matched and a diff reported a change nobody could see - a control character could be read and not written, because Go writes one as \x07 and TOML defines no such escape TOML also permits more shapes than a node's configuration uses, and each was accepted and then lost or corrupted further in. Parse now refuses an inline table, an array of tables, a repeated table heading, a key or heading that is not lower case, and a quoted key carrying a dot or a space. The inline-table case was the worst of them: Set on a new leaf defined the table a second time and produced a file BurntSushi/toml, a direct dependency of this repo, will not load, while Set returned nil and Values reported the file healthy. Refusing an inline table makes the reader unable to produce a map, which closes the last gap between what it reads and what the writer accepts. The flattening machinery those values needed is gone. Version now refuses a file whose schema counter is ahead of this binary. A release migrates the file on the node's own disk, so rolling the binary back does not roll the file back with it, and the older binary would otherwise apply only the keys it still recognises. Save no longer reports a failure after the new file is installed. Past the rename the new values are what the node reads, so a directory entry that has not been flushed is reported as ErrNotDurable rather than as a failed write. It also refuses a symbolic link, which it used to replace with a regular file while the link's target kept the old values. Three tests could not fail: - the mode-preservation test asserted 0600 stays 0600, and 0600 is the default for a new file, so it passed with the whole inheritance deleted. It now drives 0600, 0640 and 0644. - the atomicity test made the temporary file fail to create, so it never reached the install step its name and comment described. It now drives two failures that each leave a previous file to compare, and says plainly that a rename failing after the write is held by the ordering in Save rather than by a test. - one loop skipped its assertions behind a comment claiming an inline leaf was unreachable through Get. It was reachable. Removes setVersion and the error returns from insert and insertGlobal. transform.InsertMapping reports false only for a collision it was told not to replace, and both call sites tell it to replace, so four functions plumbed a failure that cannot occur. Coverage is 95.7% of statements. The fourteen uncovered statements are error propagations that need a filesystem fault or an input the parser rejects. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comments called the value a binary carries for an absent key a baseline. It is a default, which is the word the registry uses for the same thing, and one word for one thing is what keeps the two packages readable together. floatValue's godoc used "by default" in the other sense, so with the rename the word would have carried two meanings in one file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three callers wrote it and nothing read it. Its own documentation said no answer
may depend on it and a test enforced that none did, so it was write-only by
design.
The release is knowable from the deployment more reliably than from the file. A
static configuration moves in lock-step with the container it is mounted into,
and a node that writes its own file is running the release in question. A copy in
the file is a second source that can disagree with the one the platform already
knows.
Removing it makes New take one argument. It took two adjacent strings, so
New("v6.7.0", "validator") compiled, passed, and wrote a node mode no mode
matches, which Mode then returned without complaint. The swap is now
unrepresentable rather than caught by a type.
Absence was already permanent, since a build outside the release process omitted
the key, so a file written before this returns is indistinguishable from one the
design already tolerated. That is what makes adding it back cost nothing.
Coverage is 95.8% of statements.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The editing parser locates lines and preserves comments, which is why it is here, and it stops short of interpreting a literal. Deciding what 1_000 or a multi-line string means was a second implementation of the TOML specification, and two reviews found five defects in it. BurntSushi/toml, already a direct dependency of this repo, is that implementation maintained by somebody else. Compared against the old decoder on every value shape, twelve of fifteen keys were already byte-identical, including every shape the hand-written one got wrong. Removes goValue, tokenValue, unquote, unescape, unixNewlines, trimOpeningNewline, foldContinuations, continuationEnd and arrayValue, and the four in-package tests that existed only because those functions were ours. values.go goes from 238 lines to 151, and guards_test.go from 121 to 41. Three consequences of adopting it, each carried rather than absorbed: - the decoder reads an infinity and a NaN, which the old one refused, so the refusal moved to the decoded values. It recurses into a list, because that is where the old check missed one. - a date decodes to a time this package has no way to write back. Nothing configures a node with a date, so Parse refuses one rather than adding a writer for a type no field uses. - a carriage return inside a multi-line string is kept, as the specification says. The old decoder normalised it, which is a deviation this no longer makes; a multi-line string in a node's configuration is a shape no field has. Parse also refuses a key written twice in one table. That is the one shape the editing parser accepts and a conforming decoder rejects, so without it a file parsed and then every read of it failed. Coverage is 96.0% of statements. Each new refusal is verified by reverting it and watching the covering test fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
creachadair/atomicfile is already in this module and sei-tendermint's confix uses it for this exact job, so the next reader of writeAndSync will reasonably ask why it is not used here. It renames on Close and never syncs, and its temporary file is unexported, so the flush cannot be added from outside. Recorded at the function rather than left to be rediscovered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bdchatham
marked this pull request as ready for review
August 18, 2026 21:31
PR SummaryMedium Risk Overview Load / parse / edit / save go through
Unsupported TOML shapes (inline tables, array-of-tables, bad keys, non-finite numbers, etc.) are rejected at parse or before persist. Extensive tests cover round-trips, comment preservation, and save failure behavior. Reviewed by Cursor Bugbot for commit 169f961. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
A well-documented, heavily tested new config/seitoml package for reading/editing sei.toml. The design intent — refuse at Parse every shape a later verb cannot write back — is not fully upheld on the write side: Set can produce documents that this package's own Load/reader refuses, in two independent ways.
Findings: 2 blocking | 6 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
Savereturns a non-nil error wrappingErrNotDurablefor an outcome the doc says is a success. Every idiomatic caller writesif err := f.Save(p); err != nil { … }and will therefore report exactly the "your change did not land" misreading the doc warns against — the test itself has to special-case it (seitoml_test.go:1286). There are no callers in the tree yet, so this is the cheapest moment to make the signal shape something a caller cannot get wrong (e.g. a separate return value, or logging the sync failure and returning nil).decoded()re-renders the whole document and re-decodes it with BurntSushi on everyGet,Values,ModeandVersion. A resolver that walks the declared key set callingGetper key pays a full render+parse per key. Config-scale so not urgent, but caching the decode against an edit counter would be cheap.- 4 suggestion(s)/nit(s) flagged inline on specific lines.
…name
Two gaps of the same kind, both found by review: the writer accepted keys the
reader refuses, so a file could be saved and then never read.
keyOf, which Set, Unset and Get share, rejected an empty segment and nothing
else, while Parse also rejects a segment carrying a space. Set("foo bar", 1)
therefore succeeded and the next Parse of the saved file failed. keyOf now
applies keyIsAddressable, so the rule is stated once.
TOML gives a name to a value or to a table, never both, and neither Parse nor
Set refused a file using one name for each. Three shapes reached it:
- a file already holding a scalar flatkv beside a [state-commit.flatkv] heading
parsed, and every read of it then failed
- Set writing state-commit.flatkv.enable where flatkv was already a value
succeeded, and what it wrote re-parsed cleanly while no read of it could
succeed, so nothing on the way in or out reported the damage
- Set writing state-commit.flatkv where that heading already existed did the
same in the other direction
keysDoNotShadowEachOther holds the rule for both, over the document's own paths
at Parse and over those plus the candidate at Set. It sorts and compares
neighbours, so a key that merely shares a prefix, such as flatkvx beside flatkv,
still writes; a test drives that, because refusing it would be the easy mistake.
Coverage is 96.2% of statements. Each guard is verified by removing it and
watching the covering test fail, including the prefix comparison, which is
loosened rather than removed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A quoted key is spelled one way by the decoder and another by a lookup, so Values reported a key Get answered absent for. Reproduced with "a#b" = 1, which parsed, appeared as p.a#b, and could not be fetched. keyIsAddressable now admits only a bare TOML key, meaning lower-case letters, digits, underscores and hyphens, and refuses an empty segment, which "" = 1 previously slipped through. Every mapstructure tag in this tree is already a bare key; the only two that are not are refusal fixtures in config/registry's own tests. Two more values could be written and not read back: - an unsigned integer above int64, which renders and then decodes as an error, so every later read of the file failed including the two keys that describe it - schema_version 0 or below, which Version returned as-is while its own documentation says an absent or unreadable counter is an error rather than a zero, leaving a caller unable to tell that zero from the error's Rewords the editing promise. It said set and unset leave the rest byte for byte, which overstates what tomledit.Format does: it re-renders and normalises vertical spacing once. The doc now says what the tests actually hold, that every other line of content is untouched and spacing settles on the first save. Adds the coverage two review findings pointed at without either failure reproducing. A dotted key inside a table renders beside a later nested heading and BurntSushi accepts the result, so the reported unreadable file did not occur, but the fixture claiming to cover that shape used a heading instead and now uses a dotted key. A preamble replaces its predecessor across a save and reload, and leaves an operator's own top-of-file comment in place, so both halves of the reported failure are absent, but the round trip is the flow that runs and was only exercised in memory. Coverage is 96.1% of statements. Each guard is verified by removing it and watching the covering test fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n with A systems review found that Parse and Set both accepted files viper refuses, so a save could produce a sei.toml no node can boot from, having atomically replaced the good one. I had reported the first of those as a non-issue because BurntSushi accepts it; BurntSushi is the only decoder in this module graph that does, and the TOML specification gives that exact shape as an invalid example. viper decodes TOML with pelletier/go-toml/v2, so this package now does too. "This file parses here" and "the node can boot from it" become one statement rather than two that drift. That closes the class rather than three instances of it. The guard added last commit was written against leaf value paths when the thing that collides is the table namespace, so it could not see a table an ancestor's dotted key created, could not see a section holding nothing, and stepped over any collision a hyphenated sibling sorted between, since a hyphen orders before a dot and is the word separator every key in this tree uses. That guard is deleted. The decoder answers instead, at Parse and again after every edit: Set writes, renders, offers the result to the decoder, and undoes the write if the document no longer reads. insert also stops appending a heading for a table an ancestor's dotted key already created, and extends the dotted name instead, so that edit now succeeds and viper reads the result. Two more from the same review: - SetPreamble deleted a comment an operator wrote at the top of their file. It had no way to tell that block from one it wrote, so it now writes a mark and looks for it, and leaves an unrecognised block alone. - Set silently changed a string that was not valid UTF-8, because the escaper substitutes a replacement rune. Refused now, in both the scalar and list paths. Three test fixtures were shaped to agree with a conclusion rather than to attack it, and each is now the variant that can fail: flatkv-mode rather than flatkvx, since only a hyphen hides the ordering bug; a blank line after the operator's comment, since without one the parser attaches it to the next key and the branch under test is unreachable; and a#b in the verb loop, since every key there was caught by the rule the commit replaced. That last one left the previous commit's headline untested. Coverage is 94.8% of statements. Seven guards verified by removing each and watching a named test fail, including the three reshaped fixtures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eated A table can exist without a heading anywhere in a file, including at the top level: giga.enabled = true creates the table giga, and a.b.c = 1 creates both a and a.b. insert only looked for an ancestor among the named sections, and the global section carries no heading, so no prefix could find it. Set then wrote a heading, the decoder refused the second definition, and the write was undone — correctly, but the write should have succeeded, and the form it would have written is one viper reads without complaint. ancestorOf now falls back to the global section when a top-level dotted key has already created the table. globalCreated is what decides, and it checks every proper prefix of every top-level key, because each one names a table. The other direction matters as much and is tested: a table nothing has created is new, and gets a heading, because that is the form an operator expects to read. Treating the global section as everything's ancestor would turn every new section into top-level dotted keys. Both mutations fail their test. Also copies the key paths where they are stored or extended. They are slices of one another, so appending to a shorter one wrote into the longer one's storage; it happened to write the same byte it read, which is luck rather than a property. Coverage is 94.5% of statements. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@seidroid please re-review. Context is in the comment above: two commits since your last verdict on |
…an edit invalidates A re-review cleared the previous blockers and found that the preamble mark moved the failure rather than closing it, plus four smaller things and answers to two questions. All of it is here. SetPreamble owned a block, and a block's extent is chosen by the parser's blank-line grouping, which an operator controls by how they type. So the header still stacked when anything was written above it, since the search was anchored at the first item, and it still deleted an operator's line when the parser grouped that line into the same block. Reproduced both. The region is now delimited at both ends. One mark cannot tell a line before the region from a line inside it; two can, so everything between them belongs to this package and everything outside them belongs to whoever wrote it. Comments are also taken from wherever the parser put them: a block with a blank line after it is an item of its own, and without one it is carried by the item below, and the earlier fix only looked at the former. Every generated region is dropped rather than the first, so a stale header cannot survive, and an unpaired delimiter leaves the lines alone rather than guessing where a region ended. The three fixtures that mattered are now the shapes that fail: an operator block above the region, and an operator line immediately below and immediately above it with no blank line. A fourth pins the contract the delimiters state, that a note written between them is inside the part a regenerate replaces. Also from the re-review: - ancestorOf extended a dotted name whenever any prefix section existed, so a table was spelled heading-or-dotted depending on the order its keys were set. It now joins a dotted name only where one already created the table, and a table nothing created gets a heading, which is the form an operator reads. - appendItem asked InsertMapping to replace while its undo removed by identity, so a replacement would have left the undo deleting an entry that predated the edit. Unreachable, because Set looks the key up first, and now an assertion instead of an assumption. - Save checks the rendering it is about to write. Unreachable today for the same reason, and it is the one function every write to disk passes through, so a verb added later cannot forget it. - The duplicate-key and duplicate-heading refusals are subsumed by the decoder. They stay because they name the key and say what an edit would reach, and their comments now say that rather than claiming the decoder would miss them. Landed(err) answers the question Save's error cannot: one outcome it reports is not a failure, so the plain err != nil check reads a landed save as a failed one. The correct check now ships beside the sentinel that needs it. Reading no longer renders and decodes every time. A caller walking the declared key set paid that per key, and building a file was quadratic in its size because every edit checks the result. The cache is dropped by every edit, including an undone one, which is the half the test drives. Coverage is 94.5% of statements over 137 cases. Nine guards verified by removing each and watching a named test fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3078f55. Configure here.
Values filtered the two keys describing the file out of the map decoded returns, and that map is now the cache. So one Values call left the cache without them, and every later Version, Mode and Get read a file that holds them as missing them. Reproduced: Version returns "sei.toml has no schema_version" on a file whose first line is schema_version = 1. The same aliasing runs outward and was not reported: Values handed the cache to its caller, so a caller writing into or deleting from the map they were given changed what the next read of the file answered. Values now builds its own map. decoded's contract says the map is the cache and a caller needing to hand one outward or change it builds its own, which is the rule Values broke. The cache was mine, added in the commit before this one, and the test that drove it only checked that an edit invalidates. It now also reads every way twice in an order that exposes a read leaking, and writes into a returned map to check the next read is unaffected. Restoring the in-place delete fails it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ests able to fail Four rounds of review found a hole in SetPreamble each time, and each fix moved the boundary rather than closing it: items[0], then the leading run of comment items plus items[0].Block, and each time the shape one step outside failed. The last round reproduced a stale generated region left in a file forever when a key sits above it. The cause is the design, not the fixtures. SetPreamble located its region by walking the parser's item and block structure, and an operator chooses that structure by where they put blank lines. Delimiting both ends made the region's extent well defined and did nothing for finding it. It has no caller in this PR; the two that exist are in the CLI slice. So it goes, and comes back with them, written to search every comment surface a document has rather than a position. That removes four findings outright. Three findings from the same round stand on their own and are fixed: - The longest-prefix comparison in ancestorOf was dead. At most one section can qualify, because two would name one table twice and the decoder refuses that at the door, so the code claimed to resolve an ambiguity that cannot arise. - appendItem returned a no-op undo when the key was already present, so a write that inserted nothing returned success. insert now reports whether it inserted and Set says so. Still unreachable; no longer silent if it ever is not. - createsTable's empty-key guard was unreachable, since insert only asks about a table FindTable missed. Two claims in the previous commit message were not true of the suite, which is the part worth recording: - It said the test drove the invalidation after an undone edit. Removing that call killed nothing, and the reason is that it was redundant: a decode that fails leaves no cache behind, so there is nothing to drop after the undo. The call is gone and the comment says why none is needed. - It said the quadratic build was fixed. It is not. Every insert still renders and decodes the whole document to check it, measured at 700us per insert at 1600 keys, so building a file stays quadratic in its size. The cache helps consecutive reads, which is the half that was true. The cache test could not fail: deleting the cache passed it. The properties it names are only visible inside the package, so they moved there, and they are the two that matter rather than the one I asserted. A read with no edit before it reuses the last decode, driven by writing a sentinel into the first result. And no edit leaves a decode describing another document, driven by comparing what is held against a fresh decode after each kind of edit, including a refused one. Both mutations now fail. The external test keeps the half that belongs outside: every edit is visible to the next read. The ordering test never varied the order it was named for. It now sets four keys spanning both branches in all 24 orders and compares the resulting shape, and making the spelling depend on a section merely existing fails it. Coverage is 94.2% of statements over 131 cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A read caches its decode, and Values built its own map so a caller's writes could not reach it. The map was shallow, so a list stayed shared: a caller sorting the slice they were handed sorted the cache, and every later read reported the new order. Get returned the cached value with no copy at all. Both now hand values out through one function that copies a list, recursively for a list of lists. The map already had a comment saying it is the cache; the comment now says the lists are too, and where a caller-facing copy happens. The test that holds the reads against each other carried no list, which is why it agreed. Its fixture now has one flat list and one nested, and it mutates what each read handed back. Reverting either call site, or the recursion, fails a named assertion. Two claims elsewhere were stronger than the code. The package doc said every edit is asked the decoder's question; two of the three paths change no namespace and are covered by the check in Save instead, which the paragraph did not mention. Save's own doc did not mention that check either. Both now say what happens. Unset returned "the file carried no such key" when a removal found the key and did nothing. That is the shape corrected in Set, and it is corrected here for the same reason: a caller told nothing was there cannot find out otherwise. Unreachable, so uncovered, and it is the shape the next verb's author copies. 131 cases, 94.1% of statements, race clean, 0 lint issues.
bdchatham
dismissed
seidroid[bot]’s stale review
August 19, 2026 01:08
Dismissing a stale verdict: this review was submitted on cc0f2c0 and the branch is now 8 commits further on. Every finding it raised has been fixed and mutation-verified. The re-review at 1dcef6f reported 0 blocking findings; its one would-fix-before-merge item (Values and Get sharing the cache list) is fixed in 9b80106. Requesting a fresh verdict.
Contributor
Author
|
@seidroid review |
There was a problem hiding this comment.
A new config/seitoml package for reading/editing sei.toml, with a thorough test suite; the two blockers and four suggestions from the previous review are all genuinely fixed (the implicit-table insert, the shared bare-key rule at keyOf, the schema-version floor, the unsigned bound, and the doc wording), and the SetPreamble surface is gone entirely. No blocking problems found — the remaining notes are a documented-but-untested comment-preservation claim, a key-rule divergence against config/registry, and two documentation gaps.
Findings: 0 blocking | 7 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
decoded()is now cached againstchanged(), andguards_test.go'sTestAReadReusesItsDecodeAndNeverAStaleOnedrives both halves of that (reuse and no-stale-decode) including the refused-Setrollback path — that closes the previous review's performance note properly rather than by assertion.Landedaddresses the previous review'sErrNotDurablenote, but the invariant "this error is not a failure" is still enforced at every call site by remembering to callLandedrather than at the choke point.AGENTS.mdcalls that out directly ("a guard repeated at every call site is a convention the next caller can forget"). With no callers in the tree yet,Savereturning(durable bool, err error)— or logging the sync failure and returningnil— is still the cheaper shape; ifLandedstays, it needs to be named inSave's godoc (see inline).- 5 suggestion(s)/nit(s) flagged inline on specific lines.
Set replaced the whole parsed value on the existing line. A comment above the
key hangs off the key and survived; a comment beside the value hangs off the
value and was dropped. So an operator who wrote their reason at the end of the
line lost it the first time anything changed that key, which is the loss this
package exists to prevent.
The fixture said it held "a reason beside a value" and both its comments were
on their own lines, so the property was stated and never driven. The comment on
one key moves to the end of its line, both keys are now edited, and reverting
the one-line carry fails the test by name. The fixture keeps its two keys rather
than gaining a third, so the tests that count them are untouched.
The two value assertions were anchored to a line start: "enabled = " is a suffix
of "occ_enabled = ", so the unanchored form found the other key's line and
stopped discriminating.
Three documentation gaps, all found by reading a claim against the code:
- File did not say it is for one goroutine at a time. Reading is not pure
since every read decodes and holds the result, so two concurrent reads of a
shared File race.
- Save did not say a non-nil error can mean the values are on disk. It now
names ErrNotDurable and Landed at the call a caller is looking at.
- A test comment listed a date and an inline table among the shapes its
fixture drives. Both are refused when the file is read, so the fixture
cannot hold either.
131 cases, 94.1% of statements, race clean, 0 lint issues.
Save returned an error that sometimes meant the values were on disk. A caller
writing the check every other Go call wants read a landed save as a failed one,
and the package answered that with an exported sentinel plus an exported helper
to test for it. That put the invariant in every caller's memory rather than in
the code, and a caller who forgets writes something that looks correct.
The distinction is gone rather than moved. Past the rename the new file is what
a node reads, so whether the directory entry has been flushed does not change
whether the save landed, and no caller has an action either way. Retrying is
actively wrong: Linux reports a writeback error once per descriptor and does not
write the pages again, so a second flush can succeed over data that never
reached the device. syncDir returns nothing, and there is no value left to get
wrong. Two exported names go with it.
The test that pinned the old behavior drove it with a directory mode of 0300,
which fails opening the directory rather than flushing it. So the sentinel's
text named a state the code had not established, in the only case the suite
produced. The test now asserts the save succeeds and is renamed for what it
proves.
modeToWrite mapped every inspect failure onto "no file there yet" and accepted
any destination that was not a symbolic link:
- A path whose parent is a file is neither present nor absent. Reading it as a
first save chose the default mode on a guess, then failed further down on
the temporary file and named that instead of the path given.
- A pipe, a socket or a device node is replaced by the rename, not written
through. The destination was destroyed and the configuration took whatever
permission it carried, which for a device node is world-writable on a file
naming key paths.
Both are refused where the mode is decided, before anything is written. The
refused save leaves the destination in place.
132 cases, 94.0% of statements, race clean, 0 lint issues. Reverting any of the
three guards fails a named test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A node's configuration is spread across
app.tomlandconfig.toml, neither of which records what an operator chose as distinct from what a release defaulted. This is the file that does:sei.tomlholds only what an operator decided, and a key absent from it resolves to the running binary's default for the node's mode.Start with
config/seitoml/doc.go— what the file is, the two keys that describe it rather than configure the node, the shapes it deliberately does not carry, and why editing preserves the document.Properties
SetandUnsetchange the one line they name and leave every other line of content untouched. Vertical spacing normalises once, on the first save of a file nothing has saved before.ErrNotDurablerather than a failed write.sei.toml. Infinities and NaN have no form in this format and are refused in both directions.\xescape, Go has no line-ending continuation, and Go's decoder rejects the literal newline that makes a multi-line string multi-line. A file saved on Windows holds the same values as the same file saved anywhere else.Parse. An inline table, an array of tables, a repeated heading, a key or heading that is not lower case, a quoted key carrying a dot or a space. Each was otherwise read into something a later verb could not write back, and the refusal is what lets every verb below assume the document holds only shapes it can round-trip.Versionnames both counters rather than letting an older binary apply only the keys it still recognises.Scope
Nothing here migrates a file. This package reads and writes the schema counter; the chain that acts on it arrives with the migrations. Nothing here resolves a value or knows what keys exist.
Verified
build,vet,gofmt -s,goimports,golangci-lintclean. 131 cases under-race, 94.2% of statements.Every refusal and every fix is held by a test that fails when the code is reverted. The fourteen uncovered statements are error propagations that need either a filesystem fault or an input the parser rejects before this package sees it.