tidy-confops: one transaction for the per-field server setters - #244
Merged
Conversation
SetServerEnabled, SetServerTrace and SetServerTools each opened the same transaction: look the id up in tx.Servers, refuse if it is absent, assign one field, write the doc back, report the spec. Eighteen lines apiece, differing in the assignment. The refusal is the part worth making structural. Every reference to a server id is checked so a typo becomes an error rather than a ghost entry — and a fourth setter written by copying one of these three is exactly how that check goes missing, silently, because assigning a field of a doc that came back zero-valued reads as having worked. mutateServer is that transaction once, taking the assignment as a func that may itself refuse (SetServerTools keeps its allowList step inside it). No behaviour change. make ci green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Nightly tidy over
internal/confops+internal/registry.confops:mutateServer— one transaction for the three per-field server setters, so the id refusal cannot be dropped by a fourthChecked and deliberately not done:
SetCallsEnabled/SetAuditKeyIDshare a before/afterResolvedCalls()pair whose ordering is load-bearing, but there are only two of them and collapsing them would rely on "an identical write does not bump the generation" being true. Correctness that has to be reasoned about is not a tidy-night change.internal/registry: zero duplicated blocks, zero dead functions.omitzerorule (AGENTS.md) verified across every persisted selector:ServerEntry.Tools,ToolSelector.Allow,Profile.Servers, and theapi/ctlapiwire twins all carryomitzero.Profile.Toolsis a map where absence of a key already means "no rule", soomitemptythere is correct.