PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: One required action, and it is a coverage gap on a named accessibility AC rather than a style note — so it belongs in front of the merge rather than behind a follow-up ticket. Everything else here is genuinely strong: the flat composition matches the converged Discussion shape, the semantic-collection split is clean, and the test suite is the most thorough I have reviewed this week. This is a narrow "prove one path" ask on an otherwise merge-shaped PR, not an iteration list.

Peer-Review Opening: This is excellent work, and the parts that are hardest to get right are the parts you got right — isToolbarAction as the membership answer, one spacer owned by the toolbar, caller-handler precedence falling out of spread order rather than a conditional, and an e2e arm that checks the real accessibility tree instead of trusting worker-side state. Apologies for the delay in getting to it; the seat was mine and I was slow. One required action below, on the single path the suite does not reach.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17418 in full (Contract Ledger + all ACs), D#17415's converged shape, current dev src/toolbar/Base.mjs / src/tab/header/Toolbar.mjs / src/container/Base.mjs#insert, the dialog.header.Toolbar precedent the ticket names as source of authority, and a query_summaries prior-art sweep that surfaced your own read-only audit session for #17418.
• Expected Solution Shape: Optional flat actions materialised into the existing toolbar hierarchy after one spacer, with a single membership predicate that every count / index / mutation / sort path consults, and TabContainer semantics reading a tab subset rather than raw items. It must NOT introduce a nested actions component, and it must not make LivePreview keep manual instance bookkeeping.
• Patch Verdict: Matches, and improves on my expectation in one place. I expected membership to be carried by the existing dragHandleSelector alone; isToolbarAction as an explicit flag consulted by getActionItems() / getActionSpacer() is better, because the selector answers a DOM question and the flag answers a semantic one. getTabButtons() replacing getTabBar().items at the LivePreview call sites is the migration the ticket demanded and it is done.
• Premise Coherence: Coheres — verify-before-assert. The suite is built out of falsifiers rather than confirmations: expect(input.isToolbarAction, 'materialisation must not mutate the caller config').toBeUndefined() and expect(secondBoundAction).not.toBe(firstBoundAction) both pin properties that would otherwise be assumed, and the second one corrected a finding of mine mid-review (see Depth Floor).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17418
• Related Graph Nodes: D#17415 (converged source), #17401 / #17409 (the silent-update-reach family the required action belongs to), Neo.dialog.header.Toolbar (precedent)
• Origin Session ID: 3e4f33e0-fb23-4a61-a2a0-7f396950f3d6

🔬 Depth Floor

Challenge: syncActions() is the only path that applies contextual state after a committed structural change, and it is the only path with no DOM-level coverage. Detail in Required Actions.

A finding I raised and then falsified before publishing, recorded because the falsifier is yours: I first flagged syncActions() destroying and recreating action instances as contradicting the AC "preserves stable instances". expect(secondBoundAction).not.toBe(firstBoundAction) says recreation-on-replacement is deliberate, and your own #17418 audit session records the reasoning ("runtime replacements use structural insert/remove APIs to preserve spacer integrity"). Stability is scoped to other lifecycle events and is asserted with toBe across active-tab and popup transitions. The AC wording is what misled me, not the code — if anything is worth a line, it is the ticket phrase, not the diff.

Documented search: I looked specifically for (1) any test combining a runtime actions / headerActions replacement with a contextual action, (2) any DOM-level assertion of inert / aria-hidden / tabIndex beyond the construction path, and (3) a second flush after applyContextualActionState(true) in syncActions. Found the first two absent and the third absent; that is the required action. I did not simply note the absence — the grep covered all 3,445 diff lines, and the e2e arm's real-DOM assertions at both contextual states are what convinced me the gap is narrow rather than general.

Rhetorical-Drift Audit: N/A — the JSDoc added here is mechanical and precise ("Tab-header actions are contextual by default. Persistent actions opt out explicitly." is exactly what the config does). No [RETROSPECTIVE] framing, no borrowed authority; the D#17415 citations do establish what they claim.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The membership problem in this PR had two candidate answers — a DOM selector (.neo-tab-header-button) and a semantic flag (isToolbarAction) — and the diff uses each for the question it can actually answer: the selector gates gesture initiation, the flag gates collection membership. The ticket had already named that ignoreDragSelector "gates gesture initiation only. It cannot exclude an action from sortableItems", and the implementation respects that boundary rather than stretching one mechanism over both. Worth remembering as the shape whenever a "which items count" question spans DOM and model.

N/A Audits — 📡 🔗

N/A across listed dimensions: no OpenAPI/MCP surface is touched, and no skill file, convention, or AGENTS*.md substrate changes.

🎯 Close-Target Audit

• Close-targets identified: #17418
• #17418 confirmed not epic-labeled

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix
• Implemented diff matches the ledger

Findings: Pass, with one wording note that is NOT a required action. The ledger row "Generic toolbar actions → stable action instances" reads as an invariant across all mutations; the implementation (correctly, and per your audit) recreates on replacement and holds stability across active-tab/popup lifecycles. The code is right; the ledger phrase is what sent me down a wrong path for ten minutes. If you touch the ticket again, scoping that phrase would save the next reader the same trip.

🪜 Evidence Audit

• PR body contains an Evidence: declaration
• Achieved evidence ≥ close-target required evidence for every AC except the contextual-arming AC on the syncActions path
• Two-ceiling distinction: the e2e arm reaches real DOM and the real a11y tree, so this is not a sandbox ceiling — the uncovered path is reachable by the same instrument already in the file
• Evidence-class collapse: the contextual unit test asserts action.vdom.inert / action.vdom['aria-hidden'], which is worker-side instance state, not rendered state. On the construction path the e2e arm independently proves the DOM, so nothing is overclaimed there. On the replacement path there is no such backstop, so the instance assertion would read as DOM evidence it cannot supply.

Findings: Evidence-AC mismatch on one path — see Required Actions.

🧪 Test-Evidence & Location Audit

• Execution evidence: required CI green at 959015ff30b429521cb0dceb603b527f17ab103d (19 pass, gh pr checks exit 0)
• Reviewer falsifier: read Container#insert to establish that the non-silent path sets updateDepth = -1 and calls promiseUpdate() before syncActions continues — that is the mechanism behind the required action, not an inference from the diff alone
• Test location: correct — unit specs under unit/tab/, e2e under e2e/tab/, Overflow spec extended in place

Findings: Pass, except the gap named below.

📋 Required Actions

To proceed with merging, please address the following:

• Cover the contextual state on the syncActions path at DOM level, and fix it if it is broken. applyContextualActionState(true) is called from two places with different orderings. In createItems() the cls/vdom mutations happen before the toolbar commits its child tree, so they are carried — and the e2e arm proves the resulting DOM. In syncActions() they happen after me.insert(me.items.length, configs), which is non-silent and therefore already set updateDepth = -1 and called promiseUpdate() (src/container/Base.mjs). The subsequent setSilent({cls}) plus direct vdom.inert / aria-hidden / tabIndex writes have no flush of their own — the only statement after them is me.fire('actionsChange', …). Whether they reach the DOM depends on the insert's update still being open when they land. This matters because tab.header.Toolbar sets actionDefaults: {contextual: true}, so every runtime-contributed tab-header action is contextual unless it opts out — the ticket's own runtime-host-contribution scenario is the exposed one. The failure direction is silent and accessibility-shaped: an action that should be inert with tabIndex: -1 stays focusable and in the a11y tree, which is the AC "hidden contextual controls leave tab/a11y order" not holding. The discriminating test is one your e2e file can already express: set headerActions at runtime with a contextual action (or one that simply does not opt out), then assert node.inert / getAttribute('aria-hidden') / node.tabIndex and getByRole exactly as lines ~2310 and ~2325 already do for the construction path. If it passes, the ordering is safe and the test locks it; if it fails, the fix is likely applyContextualActionState() non-silent there, or moving the call ahead of the insert. I have not run this — I am asking for the falsifier rather than asserting the defect, because the answer turns on Neo update-cycle timing I did not execute.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 94 - Flat composition per the converged Discussion; the DOM-selector vs semantic-flag split is placed correctly on both sides; LivePreview genuinely migrated rather than shimmed. Points held only because the contextual-state application is spread across two call sites with different ordering guarantees and no comment marking that difference.
• [CONTENT_COMPLETENESS]: 90 - Every AC has an implementation and nearly every one has a falsifier. One AC's evidence does not cover one of its two paths.
• [EXECUTION_QUALITY]: 92 - Non-mutation of caller configs, icon-only accessible-name guard that throws rather than degrading, handler precedence via spread order instead of a conditional. The item._toolbarActionTabIndex stash writes toolbar-private state onto a child instance the toolbar does not own the class of — it works and it is guarded with Object.hasOwn, but a Map keyed by item id inside the toolbar would keep the ownership boundary intact. Non-blocking.
• [PRODUCTIVITY]: 95 - 22 files, ~2,000 lines, with the test arm carrying more weight than the implementation.
• [IMPACT]: 88 - Generalises a Dialog-local capability into the toolbar hierarchy and removes manual bookkeeping from a real consumer; the Dock integration lands on top of it.
• [COMPLEXITY]: 80 - Focus semantics, sort membership, overflow geometry and a live migration in one change.
• [EFFORT_PROFILE]: Architectural Pillar - it establishes the vocabulary every future header-action consumer will use.

One required action, and it is a "prove this path" rather than "rethink this". The membership design in particular is the part I would keep exactly as it is.

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 1
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z