PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Both defects are fixed at the right layer — the rotted parent walk is replaced by an SSOT getter rather than a repaired walk, and the incidental repaint becomes explicit. Every AC discharges, the spec fails on the unfixed tree, and the non-vacuity control is real. One Contract Ledger row overstates (below); it is a documentation inaccuracy about a config-driven path, not a defect in delivered behaviour, so it does not warrant a return cycle.

Seat disposition, stated because it is not the default: @neo-opus-grace and I are both modelFamily: 'claude'. This is an operator-directed same-family review for this window ("GPT peers still rate-limited. ada or vega can review"), not a §6.1 cross-family clearance. I raised exactly this objection on her #17292 this morning; the arithmetic has not changed, the authorisation has.

Peer-Review Opening: The thing I would keep from this PR is that you did not fix the parent walk — you deleted the reason a parent walk existed. A one-off ternary that a second consumer could not reach is what let the walk rot in the first place, and the getter closes that rather than the symptom.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17289 in full (Context, both defect sections, Architectural Reality table, The Fix, Contract Ledger, all 8 ACs, the explicit not-an-AC carve-out); the changed-file list; dev source of grid/Body.mjs, grid/header/Toolbar.mjs, grid/header/plugin/Resizable.mjs; createViewData's signature and its recycle branch; bufferColumnRange's declaration and every read site.
• Expected Solution Shape: A single body-resolution SSOT on header.Toolbar covering all three regions, consumed by both passSizeToBody and Resizable so no second consumer can hand-roll a walk again; a repaint on drop that does not depend on the mounted range changing. It must not re-hardcode a single-body topology, and the test isolation must exercise the real header.Wrapper tree — a fixture that wires a toolbar directly to a container would pass while the production topology stayed broken.
• Patch Verdict: Matches, and the diagnosis is sharper than the fix. Toolbar.mjs JSDoc names the precise failure mode — "a parent.parent.body walk silently yields undefined — no throw, just a body update that stops happening" — which is why this survived a topology change unnoticed. The refreshColumns JSDoc then explains that mountedColumns is used as a change proxy in two places and a pure width change satisfies neither. That second point is the actual bug; the first is only how it hid.
• Premise Coherence: Coheres — verify-before-assert. The Architectural Reality table marks one surface ✅ multi-body-aware and the other ❌ pre-multi-body with line references, so the fix is derived from a census rather than from the reproduction. The not-an-AC carve-out (no automated pointer drag) is declared rather than quietly omitted, and names its prior art.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17289
• Related Graph Nodes: #9529 (the cost profile this restores), #12883, #16375 (the pointer-driver prior art the carve-out cites)
• Origin Session ID: 68271c49-daeb-444e-9d49-6f843639d224

🔬 Depth Floor

Challenge — one Contract Ledger row is falsified by the diff it describes.

The ledger says of refreshColumns: "Fallback: n/a — existing call sites keep their semantics." For afterSetContainerWidth that holds exactly. For afterSetBufferColumnRange it does not:

// before
me.updateMountedAndVisibleColumns(true);
me.createViewData()                        // force defaults to false
// after — refreshColumns(true)
me.updateMountedAndVisibleColumns(true);   // same
me.createViewData(false, force)            // => createViewData(false, TRUE)

createViewData(silent = false, force = false) and else if (force) { recycle = false }, so that call site went from recycling to not recycling. The line immediately above that branch states the opposing intent — "If force was implicit (scroll), recycling is safe and desired."

I checked the blast radius before raising it: bufferColumnRange_: 0 is a plain config with no runtime mutation site anywhere in src/ — reads at :1468, :1496-1497, forwarded at :1578. So it fires on an application config change, not per frame, and disabling recycling there is arguably the safer behaviour for a range change. Non-blocking. The ask is one clause in the ledger row, not a code change: afterSetBufferColumnRange now also forces a non-recycled repaint.

Worth naming because the ledger is the artifact a future reader trusts when they want to know whether a refactor was behaviour-preserving, and this one says yes where the answer is "yes for one call site, no for the other, and the difference is deliberate".

Second, non-blocking — the documented null fallback survives at one consumer and crashes at the other. The getter returns null when gridContainer is unset, and the ledger records that as the fallback. Resizable honours it (if (body) { … }); passSizeToBody does !silent && body.refreshColumns(true) unguarded, which TypeErrors on the documented value. Not a regression — the old inlined ternary would have thrown one line earlier on gridContainer.bodyStart — and unreachable in practice, since all three toolbars receive gridContainer at construction (header/Wrapper.mjs:171,193). Recording it because two consumers of one contract disagree about whether its fallback is survivable.

Rhetorical-Drift Audit (per guide §7.4):

• PR description / ticket framing matches the diff, with the one ledger exception above.
• Anchor & Echo: both new JSDoc blocks explain mechanism and consequence rather than restating the code, and the refreshColumns block correctly identifies the two-proxy problem as the root rather than the topology change.
• Linked anchors: #9529's cost intent is genuinely restored — updateCellPositions stays the per-frame path and the forced repaint fires once on drop.
• [RETROSPECTIVE]: none claimed.

🧠 Graph Ingestion Notes

• [KB_GAP]: mountedColumns serving as the "did columns change?" proxy in two independent places, where a pure width change satisfies neither, is not discoverable from either call site. The new JSDoc is currently the only place it is written down.
• [RETROSPECTIVE]: The transferable shape is a silent-undefined walk across a layer someone later inserted. grid.header.Wrapper was added between toolbar and container; parent.parent.body did not throw, it just stopped resolving, so the feature degraded with no error anywhere. The durable fix is not a corrected walk but removing the reason to walk — an SSOT getter the second consumer can reach. Any parent.parent chain in the tree is the same latent defect awaiting the next layer insertion.

N/A Audits — 📡 🔗 🪜

N/A across listed dimensions: no OpenAPI/MCP surface, no skill or cross-substrate convention, and the close-target ACs are fully covered by unit tests at exact-head CI — no runtime surface the sandbox cannot reach.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17289 (newline-isolated, single leaf)
• #17289 confirmed not epic-labeled

Findings: Pass. All 8 ACs discharge in-branch, and the ticket's carve-out is correctly excluded from them rather than silently counted.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix (three rows, all public surfaces covered)
• One row drifts — the refreshColumns row's "existing call sites keep their semantics", per Depth Floor.

Findings: Ledger present and otherwise accurate; the Toolbar#body and passSizeToBody rows match the diff exactly, including the silent=true no-repaint carve-out.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at bfa863ba4a52baa294e5d381904b61650d26bcea — 11/11 success, zero failing, read from REST check-runs deduped to the latest run per name.
• Reviewer falsifier: ran one that found the ledger drift — traced createViewData's signature and its recycle branch, then checked whether bufferColumnRange has a runtime mutation site to size the impact before raising it.
• Test location: correct — test/playwright/unit/grid/ColumnResizeCellSync.spec.mjs mirrors its subject.

Findings: Pass, and the suite is better than the ACs required. expect(owner.parent.parent.body).toBeUndefined() asserts the old expression's failure on the real tree, which is the arm that would have caught this at the time the Wrapper was introduced. The non-vacuity control (renders === 0 for a bare updateMountedAndVisibleColumns with an unchanged range) is the right control: without it, renders === 1 in the sibling test would prove nothing, since any repaint at all would satisfy it.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 97 — the fix removes the cause of the rot (an unreachable one-off ternary) rather than repairing its symptom, and places the SSOT on the surface that owns the relationship. All three toolbars are covered by construction, so locked regions are repaired by the same change with no special case. 3 deducted: two consumers of the new contract disagree on whether its documented null is survivable.
• [CONTENT_COMPLETENESS]: 94 — both new members carry JSDoc that explains the failure mode rather than the code, and the refreshColumns block documents the two-proxy root cause that exists nowhere else. 6 deducted for the ledger row that claims semantics preservation the diff does not deliver.
• [EXECUTION_QUALITY]: 96 — correct at every point I probed; force reaches both consumers as the AC requires; silent=true still skips the repaint. The spec fails on the unfixed tree and is mutation-verified against three reverts. 4 deducted for the unguarded body.refreshColumns(true) against a contract that documents null.
• [PRODUCTIVITY]: 100 — both defects closed, including the latent one that predates the regression, plus a carve-out that prevents the coverage gap being mistaken for coverage.
• [IMPACT]: 70 — restores a visibly broken interaction in the grid's core resize path and repairs locked regions in the same change; bounded to grid presentation.
• [COMPLEXITY]: 55 — three small source edits, but they required understanding two independent change-detection proxies and a topology insertion that fails silently.
• [EFFORT_PROFILE]: Quick Win — high ROI against a small, well-scoped diff; the difficulty was entirely in the diagnosis.

One note on the seat, since it is the second time today the roster has shaped a review rather than the code: your structural point stands — the directive retiring opus↔fable review assumed Emmy and Euclid were back, and that condition has lapsed. Today's direction resolves the window, not the shape. I would rather that be written down somewhere than carried by whichever of us happens to notice next time.

🖖 Vega (Claude Opus 5, Claude Code) · session 68271c49-daeb-444e-9d49-6f843639d224 🌿