[ConfigManager] Config Registry - #3943
Merged
Merged
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3943 +/- ##
==========================================
- Coverage 59.60% 58.63% -0.97%
==========================================
Files 2329 2233 -96
Lines 199860 188907 -10953
==========================================
- Hits 119117 110757 -8360
+ Misses 69339 67638 -1701
+ Partials 11404 10512 -892
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
bdchatham
force-pushed
the
plt-775-registry
branch
2 times, most recently
from
August 17, 2026 22:15
6824cf0 to
c8d3161
Compare
bdchatham
changed the title
A key enters here once, as a field on its owning package's section struct, registered with a baseline that may vary by node mode. The dotted key identity, the canonical environment spelling, the schema fingerprint and the read site all derive from that one registration, so nobody hand-writes a flag, an environment binding or a cast-heavy reader per key. Today those three derivations are written out per key at three separate call sites, which is why a rename can move the reader and leave the flag behind. Baselines are not state. They live in the binary, may change between releases, and never mutate a configuration file or require a migration. A written value is a commitment the system never rewrites; an absent key tracks whatever baseline the running binary carries. Resolve reduces a set of named layers to one value per declared key, in the order Precedence declares, and records which layer each value came from. That last part is the reason it exists rather than a map merge: the legacy path combines its layers inside one viper before anything observes them, so a value's origin is unrecoverable and an operator whose file says one thing and whose node does another has no way to find out why. It answers for declared keys only, so it serves a diagnostic or an authoring check rather than the boot. Nothing reads it yet. This is the declaration point on its own, with no section registered and no reader migrated, so it changes how no node runs. Registering never panics. A registration this package cannot use is recorded as a Defect and the section is not registered, because a panic during the package initialisation of something every feature imports would take down every seid invocation including --help, and would turn a compile-time-fixable mistake into a fleet-wide incident. spec_test.go asserts each rule as its own test, named for the property it holds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
doc.go states the rules for someone reading the package's documentation. This says the things a reader outside the design discussions needs and a godoc is the wrong place for: the problem the package exists for, what it deliberately does not do, and how to add a section. The problem stated once, so nobody has to reconstruct it. A key needs its reader's lookup, its flag binding and its environment spelling written down, they are written separately today, and nothing ties them together. A rename moves one and leaves the others, which is how a key an operator sets ends up reaching nothing. The four things it says the package is not are the ones a reviewer would otherwise have to infer from absence. It is not the boot, not a file format, not a validator, and not wired to anything. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Resolve's documentation says every declared key resolves, because the baseline is a layer like any other. That was false for any section with a pointer field. The type walk unwraps a pointer to derive its keys, and it must, or an optional subtree would declare nothing. The value walk skips a nil one, and it must, or a section would claim defaults it does not have. Each choice is right alone. Together they produce a declared key with no resolution. The hole reads as correct, which is what makes it worth refusing rather than documenting. Overrides iterates the resolutions and simply never reports the key, and From answers false, so a diagnostic that renders Keys and calls From prints nothing for it and looks right. Filling the hole is not honest either way. A zero value states a judgement the binary never made, which is the failure TestEveryModeHasABaseline already exists to prevent. Dropping the key from the declared set makes an operator's written value land in Unknown, where it cannot be told from a typo, and makes the declared set vary by mode, which would make the fingerprint vary by mode. So a baseline that does not state a value for every key its section declares is refused, at the one function every resolution passes through. A section states a value for each key it declares, or declares against a struct without the optional field. Both are explicit. The same guard closes the general case it is an instance of. Nothing held the two walks to one type: the prototype's type derives the keys while whatever Defaults returns supplies the values, so the two could traverse different structs and every declared key would silently carry nothing. That divergence could also panic. A baseline whose type squashes a non-struct reached reflect.NumField on a string, in a package whose stated posture is that a bad registration never panics. The value walk now carries the guard the type walk already had. Reaching it takes a valid prototype and a divergent baseline, since a bad prototype is refused at registration and the section is never registered at all. Both tests fail without their guard, checked one at a time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fingerprint hashed every registration so a key added, renamed or retyped moved the hash, and its comment said CI compares it against a recorded one and fails until the bump and its migration land together. No CI does that. Nothing in .github or the Makefile mentions a fingerprint, and its only caller anywhere was one assertion seven changes further along. So it was a primitive built for a gate nobody has written, described by a comment asserting that gate exists. It comes back with the check that uses it, where a reviewer can judge the two together instead of taking the hash on the promise of a consumer. EnvPrefix goes unexported in the same pass. It had no reader outside this package, and the one test reading it asserted EnvName starts with the constant EnvName itself uses, which holds for any value. It now asserts the namespace it actually means. Four comments claimed the fingerprint derives from a registration alongside the key identity and the environment spelling. They name the read site instead, which is what remains true. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ng the compiler helps Coverage was 83.8%, and the number was the least of it. Every test registered a flat struct of scalars, which is the one shape no upstream configuration struct is. isLeaf's true branch was never taken and the value walk's nested-struct and live-pointer recursion were both dead, so the walks were only ever exercised on the case they cannot get wrong. A section carries a nested struct for a sub-table, an embedded base it squashes so the base adds no segment, a duration that must stay one value rather than becoming a group of keys, and sometimes a pointer that is set. Each takes a different branch, and both walks have to agree on all of them or a key derives with nothing behind it. One test now drives all four and checks the resolved values, not only the key names. The refusals were the other gap. The package's stated posture is that it refuses rather than guesses, and only the untagged field was covered. A refusal nothing exercises is a refusal that can stop working, so each is now driven: a squashed field that also names a segment, an empty name, a dash name, an upper-case key, a squashed scalar, a struct declaring nothing, an empty or upper-case section name, a missing struct, a non-struct prototype, a missing baseline, a section registered twice, two layers naming one source, a baseline that is nil or a scalar or a nil pointer, and a bad tag nested one level down and inside a squashed base. 98.2%. Writing them turned up two things about the code. time.Duration is an int64, so the walk takes the leaf path on its kind alone and the "time.Duration" entry in isLeaf never matches; the test that covers the reachable half says so. And an unexported embed is skipped before its tag is read, which masked three of these refusals until the embedded types were exported. Both are recorded where a reader meets them. The README claimed a rename "moves all of them together or fails to compile". It does not. The tag is a string literal, so a rename is a rename of text; what changes is that there is one occurrence instead of three, and the test holding the derived names against the reader is what turns a disagreement into a failure. Said plainly now, because a claim of compiler enforcement is the kind a reviewer checks first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…lies it The package used two. Everything a caller or an operator touches said default: Precedence's first entry, the Source that Resolution.From reports, the Defaults field, and the test in Overrides. Everything explanatory said baseline. One comment used both in a single sentence, "Defaults returns the section's baseline for a mode", which is the clearest sign the second word was carrying nothing the first did not. Collapsed onto default, because that is the word already in the surface. An operator reading which layer won sees "default"; a section author writes a Defaults function. Teaching them a second word for the same thing in the prose only asks them to map between the two. Two identifiers moved to make room. The Source-name constant is defaultSource, since it names a Layer.Source and now says so, which frees defaultLayer for the function that renders one. Nothing about the claim changes. Defaults are still not state: they live in the binary, may change between releases, and never mutate a file or require a migration. Saying it with the word people already have the wrong intuition about is better than inventing a second word to carry the correction. Coverage holds at 98.2%, and the suite passes under -race. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bdchatham
force-pushed
the
plt-775-registry
branch
from
August 18, 2026 15:50
b9c5679 to
c161fbe
Compare
bdchatham
marked this pull request as ready for review
August 18, 2026 15:53
PR SummaryLow Risk Overview
A large Reviewed by Cursor Bugbot for commit db66208. 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 self-contained, well-documented new config/registry package with no production wiring yet, so nothing here can break a running node. The findings are gaps between what the package documents/promises and what it enforces — an unimplemented env-collision check, a walk that can derive duplicate or silently-empty key paths, a doc reference to a function that does not exist, and one test that can never fail.
Findings: 0 blocking | 8 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
Non-blocking
Precedenceis an exported mutable package-level slice (registry.go:34), so any importer can reorder or truncate the declared precedence at runtime — which is exactly the "emergent rather than declared order" the package exists to remove. Considerfunc Precedence() []stringreturning a fresh slice, matching the shape already used byModes().Reset()is exported from production code and lives inregistry.gorather than a test-only file. Since the tests are in an externalregistry_testpackage it does need to be exported, but a separateexport_test.go-style seam or aregistrytesthelper would keep "clear the global registry" out of the API every feature package imports.- 6 suggestion(s)/nit(s) flagged inline on specific lines.
bdchatham
changed the title
amir-deris
reviewed
Aug 18, 2026
| // | ||
| // It never panics. A registration this package cannot use is recorded as a Defect and the | ||
| // section is not registered. | ||
| func RegisterSection(name string, proto any, defaults func(Mode) any) { |
Contributor
There was a problem hiding this comment.
I was wondering what does proto name stands for in this context?
Contributor
Author
There was a problem hiding this comment.
I've updated this sinceproto is a bit overloaded and could read as protobuf everywhere else in this tree, and the argument is a prototype struct. Renamed to prototype, and RegisterSection's godoc now says what it is for: read for its fields and their tags, never for its values, which is why it is the reader's own struct rather than a copy. A second struct would be a second statement of the same key set, and the two would disagree the first time somebody edited one.
Leaving this thread for you to close, since it was a question rather than a defect.
The package refused a key it could not derive and admitted several it could derive but nothing could reach. Each of these registered cleanly and left a setting an operator writes into nothing: - a struct that contains itself, which overflowed the stack during package initialisation rather than recording a defect, since a stack overflow cannot be recovered into one - two fields declaring one path, where one of them is unreachable and which one is not observable - a section name or a tag carrying a dot or a space, which claims a subtree the struct does not have - a struct field that declares no key, whether an empty struct, one whose fields are all unexported, or a defined type over an opaque leaf - an unexported field carrying a tag, which silently dropped an embedded type's whole subtree through ,squash - two keys differing only between a dot and a hyphen, which answer to one environment variable, as EnvName's own documentation already claimed was checked Resolve took two registry snapshots, one to render the defaults and one for the set every layer's keys are checked against, so a section registering between them was declared with no default rendered. Both now come from one snapshot. Lookup and Sections handed out the registry's own key slice, which a caller could sort or write into from outside the mutex. Both copy, as Defects already did. Precedence was an exported mutable slice, so one importer reordering it at init would change every other importer's resolved values. It is now a function returning a copy. Removes Resolution.Key, which duplicated the map key it is stored under and nothing read, and the "time.Duration" entry in isLeaf, which the walk cannot reach because it asks isLeaf only after finding a struct. Every guard is verified by removing it and watching the covering test fail. The package is at 100% of statements with no unreached block. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
amir-deris
approved these changes
Aug 18, 2026
proto reads as protobuf in this tree, where the argument is a struct read for its fields and tags and never for its values. RegisterSection's godoc now says that, since it is what a reader has to know to pass the right struct. TestEnvSpellingCollisionsAreDetectable said a collision check was owed. It exists, so the comment points at it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
yzang2019
approved these changes
Aug 18, 2026
Comment on lines
+20
to
+23
| ModeValidator Mode = "validator" | ||
| ModeFull Mode = "full" | ||
| ModeSeed Mode = "seed" | ||
| ModeArchive Mode = "archive" |
masih
reviewed
Aug 18, 2026
| // open carries the struct types on the current path, so a self-referential one is refused rather than | ||
| // recursed into. A stack overflow cannot be recovered into a Defect, so this is the one refusal that | ||
| // has to happen before the recursion rather than after it. | ||
| func walk(t reflect.Type, prefix string, keys *[]string, open map[reflect.Type]bool) error { |
Collaborator
There was a problem hiding this comment.
Unfortunate to use reflection; can we design this such that all the config is verified at compile time?
I am curious to understand why reflection at all.
Contributor
Author
There was a problem hiding this comment.
Yeah, not ideal but it's something we can continue to consolidate complexity out of once we harmonize where we are sourcing configuration from. The downstream readers are the challenge with this, since they ask for a specific dotted config string like viper.GetBool("giga_executor.occ_enabled"). My thinking is we take these improvements in bite-sized chunks. This first step is bridging configuration to the registry pattern and essentially wrapping the app.toml and config.toml plumbing in a single sei.toml source
After this lands, I see no reason we can't go to the consumer code paths and move them off the dotted lookup onto the struct field it already decodes into, then remove the lookups all together once there are no remaining read edge cases. The key names themselves mostly stay as they are, since renaming one breaks a file an operator already wrote
The README and doc.go both described the package, so a rule could be edited in one and left in the other. doc.go now carries all of it, using the headings, lists and code blocks a Go doc comment supports, so it renders as one page and gofmt keeps it canonical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A layer named its source with a string and the order it won in came from an exported slice, so an importer could reorder the precedence at init and change every other importer's resolved values. Source is now a typed constant whose declaration order is the precedence, which Resolve reads directly. sourceNames keys each name to the constant it belongs to, so the set and its names are one statement and cannot be paired wrongly. Sources derives from it. A Source is an int, so a value no constant names is still representable and Resolve refuses it rather than skipping that layer in silence. Removes the Precedence accessor and the known lookup it fed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing needs to know which source supplied a key. Every consumer asks the narrower question, whether a key carries a value somebody chose or one the binary decided, and Resolved answers that with Overrides. Resolve now takes the sources as a struct of named fields instead of a variadic list of self-describing layers. The precedence is stated once, in the loop that applies them. Named fields rather than positional parameters because File and Flags are the same type, so passed positionally, swapping the two compiles and silently inverts the precedence for every key both supply. Three refusals go with the layer type, each unrepresentable now rather than checked: a layer naming a source that has no priority, a layer naming the reserved default, and two layers naming one source. FileLayer and EnvLayer are internal, since the normalisation a file needs and the declared-key direction the environment needs are both details of resolving rather than choices a caller makes. Removes Layer, Resolution, Source and its four constants, Sources, String and the name table. The exported surface is thirteen symbols. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Resolve derived its declared set from one snapshot and envValues read the registry again for the same thing. A section registering between the two is declared by the second read and not the first, so its environment variable is read and then reported as a key no section declares. An operator whose variable is real would be told it matches nothing. envValues now takes the declared set rather than reading it, so one Sections call answers for the whole resolution. The failure has an external symptom, unlike the earlier window, so it has a test: a key reported as undeclared that the registry declares. The trigger is deterministic rather than raced, because Resolve calls each section's Defaults between the two reads and Defaults is caller-supplied. A goroutine does not reliably land in that window, and a concurrent version of the test passed with the defect present. 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 bd827db. Configure here.
…time "Today they are written separately" and "the binder the node uses today" both date the documentation. Neither needs to: what makes the three statements drift is that a key does not register here, and what falls back to a field name is mapstructure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…th segments A section name was checked for a dot and a field tag for a dot or a space, so a section named "a b" registered cleanly and declared "a b.a", whose environment spelling is SEID_A B_A. That variable cannot be exported, and the key cannot be written in a file unquoted. The two checks were the same rule stated twice, which is how they drifted, so unaddressableChar now holds it for both. Each caller keeps its own consequence, since a dot in a section name shadows another section's subtree while a dot in a tag claims a subtree the struct does not have. Removing the space from the shared rule now fails both the section and the tag case, where before it could only fail one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bdchatham
enabled auto-merge
August 18, 2026 18:51
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 18, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Aug 18, 2026
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
4 participants
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 configuration key today needs three things written separately, the reader's lookup, a flag binding, an environment spelling, with nothing tying them together, so a rename moves one and leaves the others. Several such mismatches are already pinned in
testutil/configtest(#3816, #3837, #3851). A key now enters once, as a field on its owning package's section struct, and all three come from that field'smapstructuretag.Start with
config/registry/doc.go— the problem, the declaration contract, what the package deliberately is not, and how to add a section.Scope
Resolveanswers for declared keys only, so it serves a diagnostic or an authoring check, not the boot. A running node reads a source carrying every key whether a section declares it or not. Delivery, the file format, and per-section validation are later slices.Verified
build,vet,gofmt -s,goimports,golangci-lintclean. 34 tests, 104 assertions, 100% of statements with no unreached block, under-race.Resolvereads the registry once, so the defaults and the declared set describe one registry. That property has no test: from outside the package, a section arriving during the call cannot be told from one arriving after it.