Skip to content

feat: add storage.state_commit.write_mode_enable_auto - #51

Merged
blindchaser merged 1 commit into
mainfrom
feat/sc-write-mode-enable-auto
Aug 28, 2026
Merged

feat: add storage.state_commit.write_mode_enable_auto#51
blindchaser merged 1 commit into
mainfrom
feat/sc-write-mode-enable-auto

Conversation

@blindchaser

Copy link
Copy Markdown
Contributor

Adds the one unified-schema field a node needs in order to be pinned to an explicit write mode. Needed by the FlatKV v6.7 gate re-certification (STO-713), which cannot build its reserve cohort without it.

The gap

The binary reads sc-write-mode-enable-auto under [state-commit] to decide whether to derive its write mode from committed migration state or to honor the configured write_mode. It defaults to on, so a node follows a governance-driven migration regardless of what write_mode says.

The unified schema has no field for that key. BuildRegistry discovers keys by walking SeiConfig's struct tags, so the key was never registered, and ApplyOverrides rejects an unregistered key as unknown — correctly, since the alternative is silently dropping it. The result is that a node cannot be pinned through the controller at all: spec.overrides refuses the key, and editing app.toml on the volume holds only until the next start, when the sidecar renders the file again from the intent.

Why the gate run needs it

A migration gate compares migrated nodes against a reserve cohort that stays on memiavl_only while the chain migrates around it. Without a pin the reserve nodes migrate too, and there is nothing left to compare against.

Why *bool

All three states are meaningful and distinct:

Value Rendered Meaning
nil key absent binary's own default applies
false sc-write-mode-enable-auto = false pinned to the configured write_mode
true sc-write-mode-enable-auto = true explicitly following migration state

A bare bool cannot express the pin. With omitempty an explicit false is indistinguishable from unset and gets dropped; without omitempty every node in the fleet starts emitting the key, which changes the rendered app.toml of nodes nobody is migrating. The pointer form follows consensus.unsafe_bypass_commit_timeout_override, which is already *bool for the same reason.

Nil is the default, so no existing node's rendered config changes.

Incidental fix

setReflectValue switched on Kind() with no pointer case, so a pointer field reached the default arm and was rejected as an unsupported type. A registered pointer key could therefore be discovered but never set. It now allocates and recurses, which also makes consensus.unsafe_bypass_commit_timeout_override settable through an override for the first time.

Tests

TestWriteModeEnableAuto_Tristate asserts each of the three states down to the rendered app.toml text, including that unset emits no key. TestWriteModeEnableAuto_RoundTrip asserts a render-and-read-back preserves the pin, so a later re-render cannot silently unpin a node. The existing suite passes unchanged.

After merge

Tag a release and confirm the harbor sidecar picks it up, so storage.state_commit.write_mode_enable_auto: "false" in a SeiNode's spec.overrides survives a restart.

Made with Cursor

The binary reads sc-write-mode-enable-auto under [state-commit] to decide
whether to derive its write mode from committed migration state or honor the
configured write_mode. The unified schema had no field for it, so BuildRegistry
never produced the key and ApplyOverrides rejected it as unknown. A node could
not be pinned to an explicit write mode through the controller at all: editing
app.toml on the volume works until the next start, when the sidecar renders the
file again from the intent.

The field is *bool rather than bool because all three states are meaningful and
distinct. Nil renders no key, leaving the binary's own default in force, which
is what every existing node needs. A bare bool cannot express the pin: with
omitempty an explicit false is dropped, and without it every node in the fleet
starts emitting the key.

setReflectValue rejected pointer fields, so a registered pointer key could be
discovered but never set. It now allocates and recurses, which also makes
consensus.unsafe_bypass_commit_timeout_override reachable through an override
for the first time.

Co-authored-by: Cursor <cursoragent@cursor.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a0a6fcfea9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread resolve.go
Comment on lines +126 to +130
if v.Kind() == reflect.Ptr {
if v.IsNil() {
v.Set(reflect.New(v.Type().Elem()))
}
return setReflectValue(v.Elem(), s)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clone pointer values before applying incremental overrides

When ResolveIncrementalIntent receives a config read from an app.toml that explicitly sets this flag, its shallow copy shares the non-nil *bool with current. Recursing directly into that existing pointee means an incremental override such as true to false also mutates the caller's supposedly existing configuration, potentially corrupting later diff, retry, or rollback logic. Allocate a new pointee before setting an already non-nil pointer, or deep-copy the configuration before applying overrides.

Useful? React with 👍 / 👎.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit a0a6fcf. Configure here.

Comment thread resolve.go
if v.IsNil() {
v.Set(reflect.New(v.Type().Elem()))
}
return setReflectValue(v.Elem(), s)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failed pointer set pins to false

Low Severity

setReflectValue allocates a nil pointer before parsing the override string. A rejected value therefore leaves the field as a pointer to the zero value rather than nil. For write_mode_enable_auto, that zero is false, which renders a pin. ResolveEnv only warns and keeps going, so a bad env value can silently pin a node off the migration path.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit a0a6fcf. Configure here.

@blindchaser
blindchaser merged commit cce2bed into main Aug 28, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants