fix: replace the pointer on override instead of writing through it - #52
Open
blindchaser wants to merge 1 commit into
Open
fix: replace the pointer on override instead of writing through it#52blindchaser wants to merge 1 commit into
blindchaser wants to merge 1 commit into
Conversation
Both defects come from resolving a pointer field by reusing whatever pointee was already there. ResolveIncrementalIntent shallow-copies the caller's config, so the copy and the original share every pointer. Overriding a non-nil pointer field wrote through the shared pointee and silently changed the caller's config, which later diff, retry, and rollback logic reads as the pre-override state. Allocating before parsing also left a rejected value behind as a pointer to the zero value. For storage.state_commit.write_mode_enable_auto that zero is false, which renders sc-write-mode-enable-auto = false — a pin. ResolveEnv only warns on a bad value and keeps going, so a typo in an env var could take a node off a governance-driven migration with no error surfaced. The value now parses into a fresh pointee and the pointer is replaced only on success, so nothing else aliases the write and a failure leaves the field as it was. Reported by Codex and Bugbot on #51. Both regression tests fail against the previous resolve.go. Co-authored-by: Cursor <cursoragent@cursor.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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.
Fixes both review findings on #51 — Codex's aliasing report and Bugbot's failed-set report. Worth landing before the rollout, because the second one silently produces the exact state the new field exists to make deliberate.
Both come from the same mistake: resolving a pointer field by reusing whatever pointee was already there.
Aliasing
ResolveIncrementalIntentshallow-copies the caller's config:Every pointer field is shared between the two. Overriding a non-nil pointer field wrote through the shared pointee, so
currentchanged too — andcurrentis what later diff, retry, and rollback logic reads as the pre-override state.Failed set leaves a pin
Allocating before parsing meant a rejected value stayed behind as a pointer to the zero value. For
storage.state_commit.write_mode_enable_autothat zero isfalse, which renderssc-write-mode-enable-auto = false— a pin.ResolveEnvonly warns on a bad value and continues, so a typo in an env var could take a node off a governance-driven migration with nothing in the logs to say so.That is the failure mode the field was added to prevent being accidental. A reserve node is pinned on purpose; a node pinned by a typo diverges from the chain and nobody finds out until a digest disagrees.
The fix
Parse into a fresh pointee, replace the pointer only once parsing succeeds. Nothing else aliases the write, and a failure leaves the field exactly as it was.
Tests
TestApplyOverrides_PointerDoesNotAliasCallerdrives the realResolveIncrementalIntentpath and asserts the caller's config is untouched.TestApplyOverrides_PointerRejectedValueLeavesFieldUnsetasserts a rejected value leaves the field nil.Both fail against the previous
resolve.go:The rest of the suite passes unchanged.
Made with Cursor