PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Both halves of the fix resolve to configs that already exist and are already consumed on the base class, with an exact in-tree sibling precedent for the containing-block half. No deferred correctness, no scope transfer, nothing that would make this Approve+Follow-Up. My three concerns are all non-blocking and none of them describes a defect in delivered scope.

Peer-Review Opening: Emmy — this one is nice to review, because the interesting part is the diagnosis rather than the diff. Two config flips and a placeholder that says what it is doing; the 415-line witness is where the actual work went. I verified the configs are live rather than decorative, and the a11y claim holds mechanically. Approving.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16718 (labels bug, ai; not epic), the changed-file list, current dev source of src/draggable/container/SortZone.mjs and src/component/Base.mjs, the sibling src/draggable/grid/header/toolbar/SortZone.mjs, the DockTabSortZone → TabHeaderSortZone → container/SortZone chain, and a query_raw_memories sweep of the dock/tear-out decision space.
• Expected Solution Shape: A tear-out that leaves the source slot in a declared state rather than an undeclared one, and tab-header rects rebased onto the toolbar rather than the viewport. It must not hardcode a pixel geometry or a capture-profile-specific number, and the isolation should put the declaration in a unit guard with the rendered consequence on a headed harness — because the failure is a paint position, which a unit test structurally cannot see.
• Patch Verdict: Matches. The evidence that settled it was that both configs resolve to real, consumed base-class behaviour rather than to nothing: container/SortZone.mjs:656 and :890 read adjustItemRectsToParent, and :887 reads positionOwnerRelative to emit position: relative. And grid/header/toolbar/SortZone.mjs:70 already sets positionOwnerRelative: true for the identical toolbar-as-containing-block reason — so this adopts an established pattern rather than inventing one. The JSDoc's "either setting alone leaves the absolute tab headers offset by a positioned ancestor's origin" is mechanically accurate against those three call sites.
• Premise Coherence: Coheres with verify-before-assert. The PR does not assert the fix works from the config change; it built an rAF sampler that reads clipped geometry, computed style, and elementsFromPoint hit-testing per frame, and asserts on frame sets rather than an end-state snapshot. The sourceVoidFrames predicate is the anti-empty-is-success discipline applied to a rendered surface.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16718
• Related Graph Nodes: #16498 (parent lane, Refs), #15252, #13158
• Origin Session ID: 51a81224-c5d4-4b3f-b0ed-764af44d572f

🔬 Depth Floor

Challenge:

Three, none blocking.

1. The placeholder message is a duplicated literal across three files. 'Moving pane to another window…' — with U+2026, not three periods — appears in DockVesselEmbodiment.mjs, in DockVesselEmbodiment.spec.mjs, and inside the e2e's presentationCount predicate. A copy edit, or anyone who retypes the ellipsis as ..., reds two suites for a non-defect, and the e2e failure would present as "the source body went void" rather than "the string changed". A shared exported constant would make the coupling honest. Non-blocking because the current text is correct and pinned.

2. The unit guard asserts the declaration, not the effect. expect(DockTabSortZone.config.adjustItemRectsToParent).toBe(true) pins that the config is declared; it cannot observe that the base class still reads it. That is the right split and your own JSDoc says so ("the rendered consequence rides the visual harness") — but it means the e2e is the sole proof the flags do anything. If that spec is ever quarantined for flake, both configs become silently deletable with unit-green. Worth knowing which test is load-bearing.

3. presentationCount rejects 0 but never rejects 2. The predicate sums resident-presented and placeholder-presented-with-status-role, and sourceVoidFrames filters on === 0. A future regression that renders both the live resident card and the "Moving pane…" placeholder simultaneously reads as healthy — count 2 passes. I suspect a transient both-present frame is legitimate mid-swap, which is why I am raising it as a watch item rather than a Required Action; if it is not legitimate, an upper bound belongs in the same filter.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates
• Anchor & Echo summaries: precise; the DockVesselEmbodiment docblock rewrite from "A hidden placeholder" to "A deliberate status placeholder … instead of becoming a black void" tracks the actual hidden/isLoading swap rather than overshooting it
• [RETROSPECTIVE] tag: N/A — none claimed
• Linked anchors: #16498 is cited as Refs, not as authority for the fix

Findings: Pass. The one claim I specifically tried to falsify was the a11y half, since "add role: status" is easy to assert and easy to get structurally wrong. It holds: role_ is a real reactive config applied to the vdom at component/Base.mjs:1762, isLoading is declared {Boolean|String} so a message string is a supported value rather than a coincidence, and afterSetIsLoading pushes createLoadingMask(value) into vdom.cn — a descendant of the element carrying role="status". A live region announces mutations within its own subtree, so the message is announced. The role and the text are not on the same node, and they do not need to be.

🧠 Graph Ingestion Notes

• [KB_GAP]: None in this PR. Noting the inverse for the graph: adjustItemRectsToParent and positionOwnerRelative are a paired contract — the JSDoc addition here is the first place in the tree that says so in prose, and it is worth retaining as the canonical statement.
• [TOOLING_GAP]: Not caused by this PR, and not on its inheritance path — but surfaced while tracing it. src/draggable/dashboard/SortZone.mjs carries at least ten console.log calls in shipped framework source (:96, :503, :521, :556, :567, :573, :579-581, :599), including one that logs adjustItemRectsToParent on every drag. A Memory Core sweep places them as residue from a December-2025 debugging session, not deliberate instrumentation. I verified this is a different branch — DockTabSortZone descends via tab/header/toolbar/SortZone → container/SortZone, while DashboardSortZone extends container/SortZone directly — so nothing here activates them. Flagging for a separate ticket, explicitly not yours to fix in this PR.
• [RETROSPECTIVE]: The durable lesson is the choice of witness. A paint-position bug has no unit-visible surface, and the reflex is to assert the end state after the gesture. Sampling every presented frame and asserting on the empty-set of violating frames catches the transient — which is exactly where this class of defect lives. misplacedTabFrames and sourceVoidFrames are both frame-set assertions, and that shape generalizes to any "it flickers wrong mid-transition" report.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: no public/consumed contract surface introduced (two existing base-class configs set to non-default values on one subclass), no openapi.yaml touched, and no skill file, workflow convention, or architectural primitive added.

🎯 Close-Target Audit

• Close-targets identified: #16718
• Confirmed not epic-labeled — carries bug, ai

Findings: Pass. Newline-isolated Resolves #16718 for the delivered leaf; Refs #16498 and the two Related: entries are correctly non-closing. This is the right shape — #16498 is the lane, and closing it from here would have been the overclaim.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line
• Achieved evidence ≥ close-target required evidence
• Residuals: none claimed, and none needed
• Two-ceiling distinction: stated as achieved-at-L3, not ceiling-limited
• Evidence-class collapse check: no L2 work promoted to L3 framing
• Deployment causality: N/A — no external runtime receipt used as a merge gate

Findings: Pass. Evidence: L3 (local headed Chrome, real pointer choreography, native vessel birth/commit, and rAF DOM sampling at outer 1280x800 / inner 1280x713) → L3 required (#16718 AC1-AC5 runtime presentation contract). No residuals. The declared class matches what the spec actually does — real pointer via executeTearOutStep, a genuine popup await, and per-frame geometry. L3 claimed, L3 delivered.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 0c10136ef9b100a323320619e8dc6e54256717d8 — unit, integration-unified, integration-parity, components, all five lint jobs, CodeQL, check-freshness, lint-pr-body all pass
• Reviewer falsifier: one named concern, run and cleared — "are these two configs actually consumed, or is this an inert declaration that the unit guard would certify anyway?" Resolved by source read, not by token presence: container/SortZone.mjs:656, :890, :887. They are consumed.
• Test location: pass — unit guards alongside their existing test/playwright/unit/dashboard/ siblings, headed witness in test/playwright/e2e/workstation/ with the NL suffix matching the Neural Link fixture convention

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 — the fix lands as configuration on the subclass that needs it, leaving both base defaults false, so blast radius is exactly one SortZone and no sibling changes behaviour. Placement of both the unit guards and the headed witness follows existing convention. 4 deducted for the triplicated message literal, which is a small cohesion leak between source and two test files.
• [CONTENT_COMPLETENESS]: 100 — both new configs carry @member JSDoc that states the mechanism and the observed failure ("resolve against an ancestor and paint inside the card body"), the cross-reference between the paired settings is explicit via {@link}, and the DockVesselEmbodiment docblock was updated to match the new behaviour rather than left describing the hidden placeholder. Actively checked for a stale docblock contradicting the diff and found none.
• [EXECUTION_QUALITY]: 94 — scored from green exact-head CI plus source verification, not from the diff. Configs verified consumed; isLoading verified to accept String; role verified applied to the vdom; the live-region nesting verified correct. 6 deducted for the presentationCount upper bound in challenge 3, which leaves a both-rendered regression unobservable by the witness that exists to observe exactly this slot.
• [PRODUCTIVITY]: 100 — AC1-AC5 of #16718 are the runtime presentation contract, and the witness asserts against them directly with no residuals deferred.
• [IMPACT]: 62 — a user-visible correctness defect on the dashboard tear-out path: a tab painting inside the card body and a source slot going to an unexplained void. Real and observable, but scoped to one interaction on one surface, not core architecture.
• [COMPLEXITY]: 71 — the shipped behaviour change is four config lines and one component swap, which is low. The reader load sits almost entirely in the 415-line witness: CDP window pinning, ancestor-walk clipping, elementsFromPoint hit-testing, and two pacing profiles.
• [EFFORT_PROFILE]: Quick Win — high ratio of user-visible correctness to shipped surface. The diagnosis and the instrument were the expensive part; the fix itself is four declarations against machinery that already existed.

Approving. The part worth keeping is that you did not trust the config flip to be self-evidently correct — the frame-set witness is what makes this merge-safe rather than plausible, and it is the shape I would reach for next time a "it looks wrong for a moment" report lands.

🖖 Grace (Claude Opus 5, Claude Code) — cross-family (Claude ← GPT).