state: preserve private reflected field access during migration

[MeteorsLiu]

No description provided.

[codecov]

Codecov Report

❌ Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/state/decode.go	80.00%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[fennoai]

Review: serialize reflect.Value read-only (private) fields

This extends reflect.Value serialization to preserve private-field access restrictions by capturing the reflect-internal flag read-only bits (flagStickyRO bit 5, flagEmbedRO bit 6) into a new ReadOnly uint64, clearing them on encode so the graph can be traversed, and restoring them on decode.

Correctness looks solid. The mask 1<<5 | 1<<6 is exactly flagRO and excludes the dangerous adjacent bits (flagIndir, flagAddr, flagMethod, flagKindMask). Untrusted input is bounded: reflectedValue.load rejects any bits outside the mask (object.go) before they reach the unsafe SetUint in decodeObject, and decode only adds read-only restrictions — it cannot escalate access. Test coverage is good, including the negative flag-validation test and end-to-end round-trips through a child process.

No blocking issues. The inline notes below are maintainability/consistency nits — take or leave them.

Summary of suggestions

• Extract the thrice-repeated FieldByName("flag") + SetUint idiom into one helper, and prefer named bit constants over bare 1<<5/1<<6/1<<8. This confines the reflect-internals dependency to a single place (and Field(2) avoids FieldByName's string scan, though that's negligible here).
• A one-line comment on the x.ReadOnly != 0 guard and on the ReadOnly field would help future readers.

[fennoai]

The two read-only bits are documented as "distinct" but the mask uses bare magic numbers. Consider named constants composed into the mask, e.g.:

const (
	flagStickyRO = uint64(1 << 5)
	flagEmbedRO  = uint64(1 << 6)
)
const reflectValueReadOnlyMask = flagStickyRO | flagEmbedRO

This makes the "keep both bits" intent self-evident and pairs naturally with the bare 1 << 8 (flagAddr) already used in reflectValueUnaddressable. Nit — the whole change rests on undocumented reflect internals, so naming the bits reduces the risk of a future "simplification".

[fennoai]

This reflect.ValueOf(&value).Elem().FieldByName("flag") + reflectValueRWAddr(...).SetUint(...) idiom now appears here, in decodeObject (decode.go), and in reflectValueUnaddressable (decode_unsafe.go). Consider a single helper (e.g. setValueFlagBits(v, set, clear uint64) uint64 returning the prior masked bits) so the reflect-internals dependency — field name, uintptr width, addressability — lives in exactly one place. If the runtime's flag field ever changes, there'd be one site to update instead of three. Nit.

[fennoai]

The added || x.ReadOnly != 0 clause is correct — a zero reflect.Value can't carry read-only bits, so a nonzero ReadOnly here signals a corrupt/malicious stream — but bundled with x.Addressable it reads as unexplained. A short inline comment noting why would help. Nit.

[fennoai]

Minor consistency: the rest of the codebase pins ABI-dependent comments to the exact toolchain ("Go 1.26.6", matching go.mod and state.go). This new comment says "Go 1.26". The referenced bit values are stable across releases so it's not wrong, just less precise than the established convention. Nit.