[ConfigManager] Source Configuration from sei.toml 1/2 - #4043
Open
bdchatham wants to merge 35 commits into
Open
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4043 +/- ##
==========================================
- Coverage 61.30% 60.22% -1.09%
==========================================
Files 2178 2074 -104
Lines 190788 178785 -12003
==========================================
- Hits 116968 107674 -9294
+ Misses 62796 61093 -1703
+ Partials 11024 10018 -1006
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
Author
|
@seidroid review |
…oks up A node's app.toml settings are read one key at a time, when the thing that wants them asks, and that happens after the boot has built the source they are read from. So a value can be put into that source and the later lookup finds it. This installs the declared keys an operator's sei.toml supplied, and nothing else. Only what a source supplied, not the whole resolution. Resolve answers for every declared key, so installing all of it would write this binary's own defaults over an operator's file for every key they did not mention. The keys of a section whose reader decodes its file whole are skipped, because putting a value into the source is no delivery at all for those: their file is read into a struct before this runs and nothing consults the source for them afterwards. They are marked here as needing that second delivery, which is a separate change. Nothing here can stop a node starting, and the guard that makes that true is new. What follows walks the node's own configuration types by reflection and decodes through two libraries, so a panic is a shape nobody predicted rather than a value an operator wrote, and letting it escape would refuse a boot for the one reason this path promises never to refuse one. An unreadable sei.toml is no longer reported as an absent one. A node with no such file is every node today, so that stays quiet; a node whose file will not parse, or records a schema this binary does not know, or names no node kind, is a node where somebody wrote the file and it is doing nothing. Collapsing the two meant the only signal an operator had for their mistake was the one that got collapsed. Verified by mutation: collapsing every read failure back to absent fails the new distinction, and removing the guard lets a panic escape the install path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ile can cost An operator was told their sei.toml wrote keys it does not contain. Every unmatched flag name reached the resolution under its own name, and the resolution merged the file's undeclared keys with the flags' before reporting them, so `--home` and `--trace` were reported as the file's mistakes on every boot. That is the only signal there is for a mistyped key, and it fired whether or not one was typed. Resolved now carries UnknownInFile and UnknownFromFlags apart, and the install reports only the file's own. The environment needs no set of its own: it is asked only for names derived from declared keys, so it cannot carry one that is not declared. Ignored carries its reason. The resolver already built a reason per key and threw it away, so the one warning an operator gets for an ignored variable told them nothing about why. The reason now travels with the key, and a parallel mechanism for recording the same fact per section is deleted: it had no callers, nothing read what it recorded, and the package already states that the reason belongs to the channel rather than to any section. A misspelled delivery declaration is reported. A section states its keys and how they are delivered from two calls side by side, and only the first was checked, so `memool` beside a section registered as `mempool` left the real section's keys installed into a source its reader never asks. Derived at every read, because nothing fixes the order of the two calls. One acquisition where there were two. The sections and the delivery declarations are halves of one answer, and read separately a section arriving between them is described by one half and absent from the other. Reset clears the declarations too, so a fresh registry cannot hold a declaration naming a section it does not have. A file is read within a bound. A 200 KB sei.toml of one deep heading cost 7 GB and a 400 KB one killed the process, on every restart, and a recover cannot catch that. Size, key depth and array nesting are now bounded before the bytes are parsed, and the refusal that names an over-deep key no longer renders the whole key: that message was 400 KB for a 200 KB key. The unbounded reports are bounded, the install names the keys a node reads here for the first time, and the package documentation describes the delivery that exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bdchatham
force-pushed
the
plt-775-install-app
branch
from
August 31, 2026 14:29
0becc6b to
05f98d1
Compare
bdchatham
marked this pull request as ready for review
August 31, 2026 14:29
PR SummaryHigh Risk Overview Precedence and safety behaviors added: typed flags are snapshotted before the legacy handler so Registry support: sections can declare decode-vs-lookup delivery ( Extensive integration and unit tests cover channel precedence, both delivery paths, logging floor, and refusal paths. Reviewed by Cursor Bugbot for commit d00141c. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The install path is well-structured and the reporting fixes (separating flag names from file keys, carrying the ignore reason, capping rendered lists, bounding sei.toml before it is parsed) are solid. One blocking gap remains: keys belonging to decoded sections are dropped from the install with no report at all, and the boot then logs that the file supplied no declared value.
Findings: 1 blocking | 6 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion]
PersistentPreRunEruns for every seid subcommand exceptinit, soinstallResolvednow runs — and emits its"configuration installed"Info line — onseid tx,seid q,seid keysand so on, at a levelkeepOwnReportingVisibledeliberately holds above the operator'slog_level. A node withlog_level = "error"and a sei.toml gets a config line on every CLI invocation. Consider restricting the install (or at least the Info report) to the commands that actually boot a node. - [suggestion]
TestAFileWhoseCostOutgrowsItsSizeIsRefusedBeforeItIsReadwrites files a few segments past each bound, so it proves the refusal message but not the property the bounds exist for: that the refusal happens before the expensive step. The key-depth guard is load-bearing only becausekeyIsAddressableruns earlier inrefuseUnsupportedShapesthanf.decodable(), where the quadraticflattencost actually lives — and nothing in the test suite pins that ordering. A case at realistic scale (a ~200 KB single heading, asserting Load returns quickly / within an allocation bound) would fail if the depth check were ever moved after the decode. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…is binary refused A file whose every key belongs to a section read by a decode supplied plenty and was told it supplied nothing. Those keys are filtered out of the install on purpose, because putting a value into the source reaches nothing for them, and then they were invisible: absent from what was installed, and declared, so absent from the undeclared keys too. An operator who wrote `[mempool] size` and `log-level` read that their file supplied no declared value while the node ran the old values. They are now named, and the supplied-nothing line is scoped to a file that really supplies nothing. A refused registration is reported. It is not the operator's mistake and not something they can fix, but it reaches them: a refused section leaves its keys out of the declared set, so a valid value they wrote for one of those keys was reported as a key nothing declares, pointing at their file for a defect in this binary's own source. The cause is now reported before the symptom. The routine installed line is scoped to the command that runs a node. Every subcommand passes through the same pre-run, so `seid keys list` on a node with a sei.toml printed a configuration line, at a level deliberately held above the operator's own log-level. What was refused or held back still reports on any command, because those are problems rather than routine. Two reads of the registry became one at the last place that had them, which is the hazard the widened snapshot exists to remove. The reporting floor now has a test. It was the one invariant with no coverage, and it is what keeps a quiet fleet from running a manager that changes what a node does and says nothing. Breaking the derived logger name fails it. The read bounds now pin their own ordering. The depth bound only helps because the check runs before the decode, and the existing test would have passed with it moved after. A deep heading is compared against the same bytes of ordinary keys, as a ratio, so the race detector's overhead applies to both sides. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@seidroid review |
… quiet the sibling line The report of keys this install holds back said sei.toml supplied them. The set it names comes from the overrides, which a file, an environment variable and a flag all reach alike, so a variable's value was reported as the file's. That is the same misattribution the undeclared-key report was split apart to end, reintroduced two functions away. No source is named now, because the resolution does not record which one answered. The supplies-no-declared-value line is routine for the same reason the installed line is, and sits at the same floor, so it also fired on every subcommand. Both now follow the one gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
The install path is well-constructed and the five findings from the previous review are genuinely addressed. One new blocker remains in the same "report names the wrong thing" family: the held-back report attributes every decoded-section override to sei.toml, so a common invocation like seid start --log_level=info produces a warning about a key the operator's file never contains.
Findings: 1 blocking | 4 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] The held-back warning fires on every subcommand, not just the ones that boot a node.
runsANodequiets the routine "configuration installed" line forseid keys list/seid qbecause a line above the operator's own log_level on every CLI invocation is noise — but any sei.toml holding a[p2p],[mempool],[consensus],[rpc]or root key (i.e. most real files) makesreportWhatThisInstallHoldsBackemit a Warn on every singleseidinvocation, and it will keep doing so until delivery 2/3 lands. That is the "a report that fires on every boot is one nobody reads" case doc.go names. Consider giving it the samerunsANodetreatment, or emitting it once per boot only. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
… happens The keys reported as held back came from the whole supplied set, which every channel fills. `seid start --log_level=info` on any node with a sei.toml therefore warned that the file writes log-level, which it does not contain, and that the value reads as it always has, when the flag does apply. It also inverted the line beside it: a typed flag filling that set suppressed the supplies-no-declared-value report for a file that genuinely supplied nothing. The set is now narrowed to what the file itself wrote. One read of the registry answers both halves of the decoded-delivery question. Two calls left a section arriving between them absent from what is reported and present in what is dropped, which is undelivered and unreported at once. The read bound now applies to the bytes that arrive. It was checked against a size reported by a separate stat and then the path was re-opened unbounded, so the guard described a different read. Worse, the kind of thing on the path was never checked: a FIFO reports a size of zero, and opening one blocks until something writes, so a node would hang on start with nothing to say. The kind is checked before the open, because a check made on the open file never runs, and the size is enforced through a limited read. The refusal report stated the registration case for both kinds of defect it can carry. A refused declaration leaves the key space whole, so telling an operator their key space has a hole was wrong for half of them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@seidroid review |
There was a problem hiding this comment.
The install path is well-structured and the nine findings from the two previous reviews are genuinely fixed (the held-back set is now narrowed to the file's own keys, the reads go through one snapshot(), the size bound applies to the bytes actually read). One blocker remains in the new read guard: os.Lstat refuses any symlinked sei.toml, which is the standard Kubernetes ConfigMap mount shape, and the rationale recorded beside it describes os.Stat rather than os.Lstat.
Findings: 1 blocking | 4 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] Several godocs added here are multi-paragraph explanations of rationale, mechanism and design history, which
AGENTS.mdrules out explicitly ("Godocs say what a thing is, not why it came to be or how it works inside", "Multi-paragraph godocs are rare", "Never record design history").DeclareDecodedNotLookedUp,keepOwnReportingVisible,SuppliedAndOwnedByDecodedSections,TypedFlagsandflagValuesare the clearest cases: each opens with a paragraph of what it is and then spends three or four more on why it exists and what breaks without it. The load-bearing rationale belongs at the line that needs it; the surrounding package already reads this way, so this is worth settling as a package-wide convention rather than per-symbol. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
…nnot act on The kind check refused every symlinked sei.toml, including a symlink to an ordinary file. A Kubernetes ConfigMap volume mounts each entry as one, and so does any layout that keeps the real file elsewhere and links it in, so those nodes got their file reported as unreadable and silently not delivered. Relative to the base branch that is a behaviour change: the read it replaced followed symlinks. Stat rather than Lstat, which is what the comment beside it already described. A symlink to a FIFO is still refused, because Stat reports the FIFO, so the open that would never return still cannot be reached. Both directions are tested. The report of keys held back drops a level off the boot with the other lines that describe an ordinary one. It is a problem, but not one an operator can act on, and its trigger is any file carrying a [p2p], [mempool] or root key, which is nearly every file somebody would write. Firing it on `seid keys list` buried the two reports beside it that are actionable. It keeps its own level on the boot, which is the one place holding those keys back changes what the node runs. The warning that this manager's own level could not be held goes to stderr. It was reported through the logger that had just been left at a level this could not raise, so on the fleet the floor exists for it was silenced too. An accessor that lost its last caller when one read replaced two is removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Stat answers a dangling symlink the same way it answers a path with nothing there, and the one caller acts on that answer by staying quiet. Somebody who placed the link, or a ConfigMap caught mid-update, got no signal that the file they wrote does nothing. An absent file still answers as absent, which is the ordinary state and stays quiet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@seidroid review |
There was a problem hiding this comment.
The install path is well-constructed and all thirteen findings from the three previous reviews are genuinely fixed (the held-back set is narrowed to the file's own keys, os.Stat restores symlink following while still refusing a FIFO, the reads go through one snapshot(), the held-back warning takes the runsANode gate, the floor warning goes to stderr, KeysADecodeDelivers is gone). No blockers remain; three non-blocking points, one of which is a repeat of the still-unaddressed godoc-style finding.
Findings: 0 blocking | 4 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The godocs added here are still mostly multi-paragraph rationale, which
AGENTS.mdrules out ("Godocs say what a thing is, not why it came to be or how it works inside", "Multi-paragraph godocs are rare"). This was raised last round and the current tree still hasSuppliedByDecodedSection(four paragraphs of why),keepOwnReportingVisible,TypedFlags,flagValues,DeclareDecodedNotLookedUpandinstallResolvedin that shape. The load-bearing rationale belongs at the line that needs it. Worth settling as a package-wide convention rather than per-symbol. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
seidroid
Bot
dismissed stale reviews from themself
August 31, 2026 19:18
Superseded: latest AI review found no blocking issues.
… a file that may not be involved The reporting floor was assigned unconditionally, so on a node an operator had turned up it lowered this manager's verbosity instead of raising it. The lines below the floor are exactly the ones somebody raises the level to see: that there is no file, and what an ordinary invocation held back or installed. It reads the current level now and leaves anything already lower alone. The test drove only the raising direction, so the lowering one is covered too. The warning for an ignored environment variable said the file's value applies. Nothing in that set says the file wrote the key, and the realistic case is an operator who reached for the variable because they had written it nowhere else, so no value applies at all. It says what was written elsewhere applies, which is what the godoc already said. An exported projection with no production caller is removed; the accessor the boot uses answers it, and the test that wanted the narrower shape asks that one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rries one in The redaction only handled the userinfo of a URL. PostgreSQL accepts a password three other ways, and all three reached the log: postgres://seid@host/idx?password=... postgres://seid@host/idx?sslpassword=... host=... user=seid password=... dbname=idx Four of five forms leaked. A named password field is now removed wherever it appears, with the prefix kept so sslpassword still reads as itself, and the value running only to the next separator so nothing after it is swallowed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…his delivery makes untrue The doc comment on the decode guard still opened with the name it had before the rename, so it did not name the function it documents. Swept the rest of the package: every other doc comment names its own subject. The install's report of keys it holds back goes away here, because this is where they stop being held back. The delivery it describes now exists, so the report that said those values do not arrive would be the false one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A value can decode cleanly, mean exactly what it says, and still be one the node refuses. A transaction-size ceiling of minus one is a valid int, so none of the four shapes the decode guard covers applies to it. It decoded to -1 and published, and the node then measured every transaction against it and found all of them larger, so it would have accepted none. Around thirty checks of that kind live in the node's own validation and nothing above this could see any of them. Checked on the rehearsal copy, which is the one place a copy exists to check, so it inherits the section-scoped refusal: a bad value costs its own section rather than the file. This is parity rather than a regression, since an existing config.toml is not validated either, but the copy makes it cheap to close for the channel added here. A quoted password leaked its tail. PostgreSQL accepts a keyword value in quotes and a password may hold spaces, so a run that stopped at the first one redacted the first word and left the rest in the log line. The test could not see it either, because it compared the whole secret and the first word was genuinely gone; it now asserts on every word. The production file no longer imports testing. The helper that needed it takes the two methods it uses as an interface instead, which keeps the property that mattered: it can still end a test, so it cannot answer without having measured. The type walk beside it moved into the test that is its only caller. Five other dependencies already pull testing into this binary, so that part of the concern was not new, but a production file importing it is worth not adding to. Three doc comments named the mechanism this delivery deliberately does not use, and a test variable carried a number that meant nothing to it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two tests here called an accessor the install PR removed once a single read replaced it. They go through the same one read now, so a test cannot describe a registry the boot would not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r leaf holds The redaction failed open on three inputs. A backslash escape inside a quoted keyword value ended the match at the escaped quote, so the tail of the password stayed in the line, and the same held for an escaped space in an unquoted value. Separately, a password holding a byte a URL parser refuses in userinfo made the parse fail, and a connection string in that form carries no named field either, so the whole credential was logged. Escapes are consumed with what follows them now, and a value that does not parse as a URL has its userinfo removed by pattern. The test could not see any of it, so it grew the three cases. Removing either half fails it on two and three leaked words. A pointer to anything other than a struct is left as a pointer by the decoder, so rendering one gives an address, and publishing assigns a fresh pointer for a leaf that is not a struct. The report would have named such a key as moved on every boot and printed two addresses. Nothing reaches it today because every pointer leaf is left undeclared, but the walk around it is driven from the type so that a field added later is covered, and this keeps that true for one more shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…wn rules The node's rules answer for the whole configuration, and a boot never applies them to an existing config.toml, so a node can already hold a value they reject. Refusing on that blamed the section being delivered for a failure it did not cause and left every later change unable to land on that node. The copy is now compared against what the node already holds: a failure that was already there is reported and the section is applied, and only a failure this section introduced refuses it. The keys neither side could be read for are dropped before the comparison. Naming them and then comparing them anyway let the report say the section matches the node's own file, which is the statement naming them exists to withhold. The filter is its own step so it can be measured, because it had no test when it was inline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…a source wrote A section read by a decode holds what its own file said, and config.toml is not consulted for a declared key under this manager. So every key the resolution answered is handed to the decode, including the ones that took their declared value. A key sei.toml leaves out would otherwise keep whatever config.toml said, which makes the file a patch on the configuration rather than the configuration. That replaces what an operator's config.toml said for a key their sei.toml does not mention, and it is meant to. A path rendering sei.toml from a node's existing files is what makes it safe, and it has to land before this is switched on anywhere. The accessor is named for what it now answers, the delivery no longer returns whether it did anything, and two tests that asserted the old model now assert this one. The one whose failure message said it plainest read "the node's own file turned the metrics listener on, sei.toml said nothing about it, and the node runs with it off" — which is now the correct outcome. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…port a value as held The two deliveries ran in the wrong order. The decode published its sections one at a time, and the install refused its whole set afterwards on a single bad key, under a line saying nothing was applied. A node in that state read sei.toml for the settings its own file decodes and its own files for the rest, which is described nowhere. The install now runs first, so a refusal costs what its message says. An empty value reached no check. A decode is weakly typed, so it turns an empty value into the zero of a numeric field, and nothing objected: not the decoder, and not the node's own rules. A line written with nothing after the equals sign turned a setting off rather than leaving it alone, and that is what an unfilled template variable renders as. Measured on a real decode, mempool.max-tx-bytes went from 1048576 to 0 and p2p.max-connections from 100 to 0. The four value checks were also inert for two of the three sources. Only a file carries a typed number; an environment variable and a flag both arrive as text, and the numeric test matched Go numeric types alone. It now parses a numeric string, so all three reach the same checks. A password in a connection string is reported as the node holds it. The redaction covered one declared key, cost two regular expressions and their reasoning, and a value an operator wrote in plain text is theirs to write. Comments state reasons plainly. A sentence carrying two clauses joined by "so" becomes two sentences, and the abstract verbs are gone: "answers for the state of" is "records", "the file wins over the command line" is "the flag is dropped". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bdchatham
changed the title
Contributor
Author
|
@seidroid review |
There was a problem hiding this comment.
The install/delivery path is carefully built and every finding from the four previous rounds is genuinely fixed, but two problems remain in the new code: reportWhatMoved logs raw before/after values (leaking tx-index.psql-conn credentials) despite a godoc that claims redaction, and the "values a reader turns into something else" guard has no case for boolean fields, so the api.enable = "yes please" example the PR description says is refused still silently arrives as false.
Findings: 2 blocking | 3 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] The godocs added here are still predominantly multi-paragraph rationale, which
AGENTS.mdrules out ("Godocs say what a thing is, not why it came to be or how it works inside", "Multi-paragraph godocs are rare").reportWhatMoved(5 paragraphs),whatDecodesToSomethingElse(7),deliverOneSection,copyNodeConfig,detachReferences,publishNodeConfig,ResolvedAndOwnedByDecodedSectionsandtheFileNamesTheKindThisNodeRunsare the clearest cases. This was raised in each of the last two rounds and is still open; the load-bearing rationale belongs at the line that needs it. Worth settling as a package-wide convention rather than per-symbol. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
…ries to one reporting level The report's doc comment described taking a password out of a value in every form a connection string writes one. Nothing does that, on purpose: a value an operator writes in plain text is theirs to write, and the regular expressions that used to do it were removed. The paragraph describing them was not. The same comment said a key that did not move is not reported, while the body reported the section at a level a quiet node cannot turn down. Every subcommand installs, so a section that matched printed a line on `seid keys list` and on every query. It is now a debug line, and the report naming what moved goes out at the level the command warrants, the same one the install already used. Only the install honoured that before, so the decode's lines reached every invocation. Both routine lines are now covered by the test that holds them to the boot. It wrote a key on the install side alone, which is why the decode's lines were free to leak. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… with the struct The value check covered four numeric shapes, so no boolean was ever examined. api.enable = "yes please" parses as a string, reaches the source as written, and a reader turns it into false, so an interface an operator wrote in order to turn on arrives off with nothing naming the key. An empty value reached the same place, because that guard asked whether the field holds a number. Every declared boolean was reachable this way, and the description of this change named the case as refused while it was not. The log level was applied only when a source supplied it, while the decode publishes whatever the resolution answered into the struct either way. On a node whose file omits the key, the field moved, the process did not, and the report named a move that never reached the logger. It is now applied whenever the resolution answered, which is what makes the three agree. It also ran before every gate that reports nothing was applied, so those reports were false for this one setting. It now runs after both deliveries. The refusals stay visible: this package holds its own logger at a floor and each of them is a warning or an error, which is what the earlier ordering was for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@seidroid review |
There was a problem hiding this comment.
A large, well-tested addition that makes sei.toml the source of truth under SEI_CONFIG_MANAGER=v2, split into a viper install and a per-section decode into the tendermint config struct. No blockers found; three non-fatal gaps in the fail-open guards and one doc overstatement are worth addressing before the switch is used anywhere.
Findings: 0 blocking | 4 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
config/seitoml/file.go: themaxArrayDepthguard's comment says a file whose cost grows faster than its size is "refused before it is parsed rather than after it has taken the memory", butvalueIsAddressableWithinruns fromrefuseUnsupportedShapes()afterParsehas already built the whole document. OnlymaxFileBytesis enforced pre-parse. Worth correcting the comment so a later reader does not rely on a pre-parse guarantee that isn't there. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
seidroid
Bot
dismissed
their stale review
September 2, 2026 17:31
Superseded: latest AI review found no blocking issues.
… what was published The rules a section was held to were the whole configuration's, and those stop at the first failing section. So on a node already failing anywhere, a value written here failed the same check and was published under a line saying the node was already broken. Delivery is in order, so the failure has to be standing in a later section for it to show: an invalid statesync in the node's own file let a rejected rpc value land. Each section is now held to its own rules, which are the ones attributable to it, and the escape hatch is gone. The log level was read from the resolution rather than from what the delivery published. The section carrying that key is refused whole when any of its values is wrong, and the resolution still holds a level, so the process moved while the struct kept what it had. It now reads the level the node's configuration holds, so the two agree whether the section landed or not. The package doc said app.toml and config.toml are not consulted for a declared key, and that nothing here stops a node starting. Both overstate. The boot's own handler still parses those files to build the source and still refuses to start on one it cannot read, so a node with a complete sei.toml does not start on a malformed legacy file. What this manager replaces is their values for a declared key. 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 845c698. Configure here.
The boot's handler applies a level from the command line and never writes it into the node's configuration struct: the flag is spelled log_level and the struct's key is log-level, so the two never meet. Reading the level off the struct afterwards therefore put the file's level back over the command line, and only when the section carrying that key was refused, since a section that lands brings the flag's value with it. The level the handler already applied is now read from the same source the handler read it from, and a non-empty answer means there is nothing left to apply. On a refused section the struct keeps the file's level while the process runs the flag's. Nothing in the node's run path reads that field afterwards, and the legacy path already leaves the same pair disagreeing when it declines an override. One test could no longer fail. It matched a message that was renamed, so it passed whatever the code did; the mutation it was written for compiles and passes with the old string. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
masih
reviewed
Sep 2, 2026
masih
left a comment
masih
left a comment
Collaborator
There was a problem hiding this comment.
One blocker on leaking creds in tendermint; left some comments for non-blockers.
Please let me know if I missed something on the creds since it was highlighted by AI already and marked as resolved; I unresolved it with a rationale comment.
Thanks @bdchatham 🙌
| // source hands the value out as it was written, and a reader asking for a number gets a zero from a | ||
| // word, so a setting an operator meant to change ends up off with nothing naming it. Dropped one key | ||
| // at a time, because a lookup delivers each key on its own and one wrong value need not cost the rest. | ||
| if bad := whatDecodesToSomethingElse(whatEachDeclaredKeyHolds(registry.Mode(mode)), |
Collaborator
There was a problem hiding this comment.
OK so here i think that rejected env values could remain effective. Because it only covers viper overrides. Meaning, this leaves the "winning" env var visible to whatever the eventual reader would be. Example: SEID_API_MAX_OPEN_CONNECTIONS=1
To fix this we would need to keep track of provenance and when winner is invalid install the next valid etc. etc.
IMHO most of this complexity is accidentally caused by viper. I point this out as feedback to the machine in case while we traverse the forest of solutions we can reduce the potency of this edgecase. If you decide not to fix it, please make sure to comment about the footgun and make it clear that env vars may slip through.
Contributor
Author
There was a problem hiding this comment.
Confirmed by measurement, and documented rather than fixed, per your ask.
serverCtx.Viper.AutomaticEnv() runs at sei-cosmos/server/util.go:124 with this binary's name as the prefix and dots and hyphens mapped to underscores. So a reader asks for a key and gets SEID_THAT_KEY whether anything was installed for it or not. Driven:
SEID_API_MAX_OPEN_CONNECTIONS=-1
guard refused the key: true
reader sees api.max-open-connections = -1 (GetUint=0)
The comment now sits at the line that drops the key and says exactly that, with the measured example.
Left as it is on the reasoning that the variable was equally effective before this manager existed, so the key does read the way it always has, which is what the report claims. There is a cheap fix if we want the refusal to bite: install the declared value instead of dropping the key, since the install sits at override precedence and would beat the variable. That is one branch, not provenance tracking. The cost is putting this binary's default over a variable an operator deliberately set, which is why it is not the default choice.
Collaborator
There was a problem hiding this comment.
We can proceed with merge here @bdchatham but I want to point out the issue again since I worry that I might have not been clear enough:
The above only works by falling directly to the declared default. It cannot preserve precedence correctly. Example:
sei.toml: api.max-open-connections = 100
environment: SEID_API_MAX_OPEN_CONNECTIONS=-1
In this case, the correct fallback is 100, not the binary default IIUC.
If that tracks then, I think this is a footgun we need to address in future PRs. Please feel free to capture an issue for this etc.
| func valueIsAddressableWithin(key parser.Key, v parser.Value, depth int) error { | ||
| if depth > maxArrayDepth { | ||
| return fmt.Errorf("%s nests arrays %d deep and this file is read to %d. No setting here is a "+ | ||
| "list of lists, so nothing legitimate reaches that depth", key, depth, maxArrayDepth) |
Collaborator
There was a problem hiding this comment.
This might give a false sense of security: a deeply nested file below 1MiB can exhaust the resource before the check.
Contributor
Author
There was a problem hiding this comment.
This one is already guarded, separately from the byte bound.
Key depth and array nesting are both bounded at 8 and refused during the parse, before the expensive step, so a deeply nested file under 1 MiB does not get to spend the resource. TestADeepHeadingIsRefusedBeforeTheExpensiveStep drives a 200 KB file with 100,000 key segments, requires it to be refused, and holds the cost of refusing it to a small factor of the cost of simply reading the same bytes. Decoding first put that ratio in the thousands, which is what the guard exists for.
TestAFileWhoseCostOutgrowsItsSizeIsRefusedBeforeItIsRead covers the byte bound beside it. Both pass.
Collaborator
There was a problem hiding this comment.
The 100,000-segment test covers deep keys, not deeply nested arrays. The array test uses roughly 12 levels, so it proves eventual rejection but not pre-parse resource safety.
The decoded delivery read every key of a section before and after it published. It then reported each value that changed. A declared key can hold a secret. tx-index.psql-conn is a PostgreSQL connection string. A node that holds one in config.toml, and no entry in sei.toml, gets the declared empty value delivered over it. The two values differ, so the report printed the secret. Validator operators share logs with third parties, so the value has to leave the log. Removed reportWhatMoved, whatBothSidesCouldBeReadFor and asSet. Removed the two describe calls in deliverOneSection, and the unread-key reports that existed only for them. Removed the said parameter the delivery passed down. Redaction was the alternative. It needs a hand-maintained list of secret keys, and the first forgotten key is a password in a log. Every refusal stays. A value that decodes to something else, a decode error, a section that breaks its own rules and a node-kind disagreement each still report. Each of them names keys and error text rather than a value. describe and DescribeForTest stay, because they read values rather than log them. Two validation tests depend on them and neither one changed. TestTheDivergencesFromAGeneratedFileAreTheRecordedOnes pins the declared values against the binary's own generator. TestNoDeliveryCarriesADeclaredDefault proves no declared default lands over a setting nobody wrote. Both pass. TestAFileHoldingOnlyDecodedKeysIsNotReportedAsSupplyingNothing goes with the line it measured. It held that an operator who writes only decoded keys sees those keys named. The removed line was the only place that named them. TestOnlyTheBootReportsTheRoutineLine now covers the one routine line left. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rules of a section are found by matching the node's own type against the section's prefix, and both callers derived that prefix themselves. The registry names the root of the file node_base and the node's type carries no tag of that name, so a caller handing over the name instead gets no rules and reports nothing. The function now takes the section's keys and derives the prefix itself, which makes it an invariant rather than something each caller has to remember. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The decoded delivery reads every key of a section before and after it publishes, and reports which of them the delivery changed. It reports key names only. A declared key can hold a secret: tx-index.psql-conn is a PostgreSQL connection string, and validator operators hand their logs to third parties. Removing the report went too far. Without it the one routine line is constant. Measured on three files, including an empty one, it read count=112 and read_here_first_count=112 every time. Every declared lookup key is installed whether the file mentions it or not, so that count is a property of the binary rather than of the operator's file. Nothing named a key an operator wrote. Restored describe on both sides of the publish, whatBothSidesCouldBeReadFor, asSet, and the said parameter the delivery passes down. reportWhatMoved appends the key rather than a rendered before-and-after pair. A key neither side could be read for stays out of the comparison, because it compares equal and would report as a key that did not move. TestNoReportPrintsAValueANodeHolds drives a node that holds postgresql://indexer:hunter2@db/tx in tx-index.psql-conn and leaves the key out of sei.toml. The declared empty value lands over it, so the key moves and the report names it. The test then asserts that the connection string and its password appear in no line the install wrote. The value-shape refusals in values.go still embed the written value. They fire for numeric shapes and for unparseable text on a bool field. A secret lives in a string field, which always decodes, so none of them reaches one. TestAFileHoldingOnlyDecodedKeysIsNotReportedAsSupplyingNothing measures its property again: a file holding only decoded-section keys now names mempool.size. TestOnlyTheBootReportsTheRoutineLine holds both deliveries to the command again, with a file writing one key each side. installWithSeiToml calls installOnCommand rather than repeating the home and command scaffolding, which was the same code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…able The boot's source reads the environment itself, under this binary's name with dots and hyphens as underscores. So a reader asks for a key and gets the variable of that name whether anything was installed for it or not, and a value refused here because a variable supplied it still reaches the reader. Measured: SEID_API_MAX_OPEN_CONNECTIONS=-1 is refused, the key is dropped, and the reader still answers -1. Left as it is, and said so where the key is dropped. The variable was equally effective before this manager existed, so the key does read the way it always has. Installing the declared value instead would make the refusal bite, at the cost of putting this binary's default over a variable an operator set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
masih
approved these changes
Sep 3, 2026
masih
left a comment
masih
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for resolving the blocker issue. I unresolved some of my previous comments that I think still hold along with rationale
@bdchatham can I ask you to make sure PR description and title is up to date please since those will end up in commit history. I would personally avoid "1/2" etc in the title (which will end up being commit title) and try and word it as something that represents a shippable unit of work being done.
Thank you for reflecting on all my comments 🙌
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
3 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.
Under
SEI_CONFIG_MANAGER=v2, a node'ssei.tomlanswers every declared key. A key the file states takesthe written value; a key it leaves out takes the value this binary declares for that kind of node, and
app.tomlandconfig.tomlare not consulted for a declared key. Unset by default.First of two;
seid config checkfollows.Two deliveries, because a node reads a setting two ways
Most settings are looked up by name, after the boot builds the source they are read from, so a resolved
value is installed into it. The node's own configuration file is decoded into a struct before any lookup, so
a value installed afterwards reaches nothing; those are decoded into a copy and published into it.
The split is exact: of 274 declared keys, 159 are looked up and 115 are decoded, and the two sets share no
key. A decode is all or nothing for what it is handed, so one section is delivered at a time.
Values a reader turns into something else
Refused before delivery, because nothing downstream objects to them:
An empty value is what an unfilled template variable renders as. Both deliveries ask the same check, and it
reads all three sources; only the file carries a typed number.
Nothing stops a node starting
Every failure leaves each key reading as it always did, and the node on its own files.
One case costs the whole file rather than one key: when
sei.tomland the node's own file disagree aboutwhat kind of node this is, nothing is delivered. Every resolved value is the answer for one kind, so a
disagreement is the whole configuration answering for a node this is not.
Verified
Formatters,
go vet,golangci-lint, and-race -count=2 -shuffleclean. Each guard and refusal ismutation-verified.