refactor(#591): fail-closed decoders for persisted domain records (phase 3)

[BorisTyshkevich]

What & why

Part of #593 (phase 3 of 8). Implements #591: gives the five persisted-domain reads in
src/state.ts that previously trusted localStorage verbatim (varValues,
filterActive, varRecent, varRecentDisabled, history) a fail-closed decoder each,
following the pattern decodeStoredSavedQueries already establishes in
core/library-codec.ts — drop malformed data to the field's documented default, never
throw, never let a corrupt or hand-edited value reach the UI with the wrong shape.

Also fixes an item inherited from #586 (phase 1): firstValidPx's validator was
parseInt-lenient ('1e3' → 1, '0x10' → 0, '420px' → 420), so a genuinely
corrupt canonical rightInspectorPx could still beat a valid legacy fallback. It now
requires a complete, trimmed, optionally-signed decimal integer before accepting a
candidate, preserving the documented rightInspectorPx → docPanePx → cellDrawerPx → 480
precedence among fully-valid candidates only.

This PR also updates this repo's /ship skill (skills/ship/references/per-issue-cycle.md):
Medium-risk plan review now defaults to a chatgpt-review plan-mode pass (previously
"none by default"), matching what this phase's plan should have gotten before
implementation — the plan turned out sound, but going forward Medium-risk plans get the
same second opinion High-risk plans already do, with the same escape hatch if ChatGPT is
unreachable. Bundled into this PR at the owner's explicit request.

Contract coverage

All 4 acceptance criteria + both inherited-item checkboxes claimed and met, nothing
deferred:

1. All five fields read through a fail-closed decoder (src/core/state-codec.ts), not a
   raw as cast.
2. Each decoder falls back to the field's documented default ({} / {} /
   emptyRecentMap() / false / []) on malformed input and never throws.
3. Malformed individual entries in a collection-shaped field (varRecent's per-name
   lists, history) are dropped without discarding the rest of the collection.
4. npm test (coverage gate), tsc --noEmit, check:arch, check:schemas,
   check:examples, and npm run build all pass.
5. firstValidPx rejects trailing junk, exponent form, hex form, and whitespace-only
   values instead of coercing them.
6. Regression cases cover a corrupt canonical value with a valid legacy fallback in each
   position (docPanePx, cellDrawerPx, and the all-corrupt→480 case).

Non-goals respected: no change to the four numeric geometry preferences' clamp
NaN-safety (that was #570's scope, closed Not Planned); no behavior change for
well-formed persisted data (verified — the existing 'uses defaults' and 'reads + clamps persisted prefs' tests pass unmodified); decodeStoredSavedQueries itself
untouched.

Invariant verification

Invariant	Enforcement	Sabotage case (executed, confirmed, reverted)
Each decoder never throws / never propagates a wrongly-typed top-level value	Total type-guard functions over unknown	Removed a guard → targeted tests failed as expected
Each decoder's fallback matches the documented default, returned fresh	Literal default per decoder; fresh allocation per call	Hoisted a shared fallback instance → freshness test failed
Collection-shaped decoders drop malformed entries without discarding the rest	Per-entry filter + projection, no whole-value bailout	Whole-collection discard on first bad entry → "rest kept" test failed
createState reads all five fields through decoders, not raw casts	Five call-site edits	Reverted one call site to a raw cast → independently re-verified by the coordinator: exactly the matching wiring test failed, all others stayed green
history's decode cap and pushHistory's write cap can't drift apart	Both reference HISTORY_MAX_ENTRIES	Dropped the decode-side .slice() → cap test failed
firstValidPx accepts only a complete trimmed optionally-signed decimal integer	Full-string regex match before parseInt	Reverted to bare parseInt + Number.isFinite → lenient-form regression tests failed
firstValidPx preserves documented precedence among valid candidates only	Ordered loop, first full match wins	Reversed iteration order → existing precedence test failed
Well-formed persisted data decodes identically to today	Decoders are identity-modulo-rebuild on valid input	Coerced with !! instead of requiring the exact type → entry-filter test failed

All 8 sabotage cases were executed by the implementation worker, each reverted, and the
full gate re-confirmed green. The coordinator independently re-verified the wiring
invariant with its own sabotage/revert cycle (see below) rather than trusting the
worker's self-report.

Tests

• tests/unit/state-codec.test.ts (new, 19 tests) — one describe per decoder,
  covering wrong-top-level-type, entry-level drop-and-keep, fresh-fallback, and
  (for history) the size cap.
• tests/unit/state.test.ts (+11 tests) — 5 wiring tests proving createState actually
  calls each decoder end-to-end, plus 6 firstValidPx regression cases for the
  inherited item.
• Existing 'uses defaults' and 'reads + clamps persisted prefs' tests pass
  unmodified, proving no behavior change for well-formed data.

Build & gate

check:types, check:arch (no boundary violations), check:schemas, check:examples,
npm test (207 files / 6810+ tests, 0 coverage-threshold errors — state-codec.ts
100/100/100/100 confirmed via lcov, state.ts 100/96.23/100/100, both clear the
100/95/90/100 floor), npm run build — all run explicitly and independently
re-verified by the coordinator (.npmrc sets ignore-scripts=true here, so a green
npm test alone would not prove tsc/arch/schema checks ran).

e2e

npx playwright test --project=chromium --project=webkit: 418 tests, 414 passed, 4
skipped (touch-only, feature-gated), 0 failed. This phase has no UI-visible surface, but
state.ts is widely depended upon so the full e2e suite was run as a regression check.

Checklist

• npm test passes (the per-file coverage gate is non-negotiable)
• Tests added/updated in the same change as the code
• npm run build succeeds (single-file dist/sql.html)
• Layers kept honest: pure logic in src/core/, network in src/net/ (injected
  fetch), DOM in src/ui/
• No new runtime dependency
• README / CHANGELOG.md ([Unreleased]) updated
• Reconciled affected tracked work — Umbrella: V2 architecture refactor — shell primitives, composition root, state reactivity, transport adapter #593's ## Phases checklist and ship-log
  comment will be updated once this PR is open (phase 3 row); no other tracked work
  is reshaped by this change

🤖 Generated with Claude Code

https://claude.ai/code/session_01R32bb4VZGPgNo3tB9iVSKF

[BorisTyshkevich]

ChatGPT review pass 1

Reviewed head: 509ca2024511ace552562523daf4a3d8cb971c96

Major findings

1. decodeStoredRecentMap is prototype-sensitive for a stored "__proto__" variable name.

   • File: src/core/state-codec.ts:65-69
   • The decoder rebuilds byName as {} and writes each untrusted persisted name with byName[name] = filtered. For name === "__proto__", that invokes Object.prototype.__proto__ instead of creating an own map entry. A value parsed from {"byName":{"__proto__":[{"value":"x","seq":1}]}} therefore returns a byName whose prototype is the attacker-controlled array, with no own __proto__ property. for...in consumers then see inherited array indices, so the decoder has surfaced a wrongly-shaped map rather than failing closed.
   • Fix: create the property with defineJsonField, Object.defineProperty, or an Object.fromEntries rebuild; do not use bracket assignment on {} for persisted keys. Add a regression test asserting Object.getPrototypeOf(result.byName) === Object.prototype, Object.hasOwn(result.byName, "__proto__"), and that the entry round-trips. The same keyed-write pattern in enforceTotalCap should be made safe so a correctly decoded own key is not re-corrupted later.

2. firstValidPx accepts syntactically valid decimal strings whose conversion is not finite, so a corrupt canonical value still beats valid legacy fallbacks.

   • File: src/state.ts:655-659
   • The regex rejects junk but the code immediately returns parseInt(t, 10) without restoring the previous Number.isFinite check. For example, "9".repeat(400) matches the regex and parses to Infinity; "-" + "9".repeat(400) parses to -Infinity. With a valid docPanePx of 420, the former leaves state.rightInspectorPx === Infinity, while the latter is clamped to 320; neither falls through to 420. This violates the inherited requirement to select the first fully-valid finite candidate and the documented valid-candidate precedence.
   • Fix: parse after the full-string match, then return only when Number.isFinite(n); otherwise continue to the next candidate. Add overflow regressions for a corrupt canonical value followed by a valid docPanePx, and for corrupt canonical + docPanePx followed by a valid cellDrawerPx.

Verification

The canonical PR still pointed at the SHA above when this review was submitted. GitHub's ci workflow is green for that head. Direct git clone was unavailable in the review runtime, so I inspected the complete six-file canonical diff and surrounding files through GitHub and ran focused Node reproductions for both cases; both reproduce as described.

[BorisTyshkevich]

ChatGPT review pass 2

Previously reviewed head: 509ca2024511ace552562523daf4a3d8cb971c96

Reviewed head: 8cd85afd46edf13fa94c952cd571c682cc10197d

Reassessment of pass-1 findings

• firstValidPx overflow: resolved. The new post-parseInt Number.isFinite check rejects both positive and negative overflow and preserves valid fallback precedence.
• Direct decodeStoredRecentMap prototype mutation: resolved at the decoder site. Object.fromEntries preserves a stored "__proto__" name as an ordinary own property without changing the result object's prototype.

Major finding

1. The pass-1 downstream warning is still unfixed: enforceTotalCap re-corrupts the safely decoded "__proto__" entry on the first over-cap recording.
   • File: src/core/recent-values.ts:77-81
   • Pass 1 explicitly called out the equivalent keyed write in enforceTotalCap. The updated decoder now correctly returns a normal object with an own enumerable "__proto__" entry, but recordRecent calls enforceTotalCap whenever the total grows past 100. That function rebuilds into {} and still executes byName[name] = list. When the surviving name is "__proto__", the assignment invokes the inherited setter, changes the rebuilt map's prototype to the entry array, drops the own property, and the next persistence write loses that entry.
   • Focused reproduction: start with a safely decoded map containing "__proto__" plus 99 one-entry names (100 total), give "__proto__" a high sequence so it should survive eviction, then record one new name. Before recording, the key is own and the prototype is Object.prototype; afterwards, the key is no longer own, the prototype is an array, and JSON.stringify/parse no longer contains "__proto__".
   • Fix: rebuild the capped map with Object.fromEntries, defineJsonField, or Object.defineProperty, not bracket assignment onto {}. Add an end-to-end regression that runs decodeStoredRecentMap(...) followed by recordRecent(...) at the 100→101 boundary and asserts the normal prototype, own "__proto__" property, retained value, and persistence round-trip.

Verification

The new head is exactly one commit ahead of the pass-1 SHA and changes only src/core/state-codec.ts, src/state.ts, and their two unit-test files. I re-inspected the complete updated six-file PR and relevant recent-values.ts path. GitHub's ci workflow is green for the reviewed head. Direct cloning remains unavailable in this runtime due DNS resolution, so focused Node reproductions were used for the repaired sites and the remaining failure.

[BorisTyshkevich]

ChatGPT review pass 3

Previously reviewed head: 8cd85afd46edf13fa94c952cd571c682cc10197d

Reviewed head: 812eaa43b3721d59a6aedef1fa1c898f22f3e4f2

Reassessment of earlier findings

• firstValidPx accepts overflowing integer strings: resolved. The full-string validation is followed by a finite-result check, so positive and negative overflow no longer beat valid legacy fallbacks.
• decodeStoredRecentMap mutates the rebuilt map prototype for a stored "__proto__" key: resolved. The decoder uses Object.fromEntries, preserving the key as an own enumerable data property on a normal object.
• enforceTotalCap re-corrupts that key during the 100→101 eviction path: resolved in this head. The cap rebuild now also uses Object.fromEntries.
• Related inherited-key read behavior: the new Object.hasOwn checks in recordRecent, clearRecent, and visibleRecents correctly prevent inherited __proto__/prototype-member reads from being treated as recorded variable histories.

Findings

No actionable findings in this pass.

The new head is exactly one commit ahead of the pass-2 SHA and changes only src/core/recent-values.ts and tests/unit/recent-values.test.ts. I re-inspected the complete updated eight-file PR and the affected persistence/mutation path. A focused reproduction of the exact pass-2 scenario—an own "__proto__" entry at the 100-entry cap followed by recording another name—kept Object.prototype, retained the own entry, evicted the true lowest-sequence entry, and preserved "__proto__" through JSON serialization and parsing. The empty-map recordRecent, clearRecent, and read behavior for that name also behaved as intended.

GitHub's ci workflow is green for the reviewed head. Direct repository cloning remains unavailable in this runtime, so verification used the canonical GitHub diff/files and focused Node reproductions.