PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: Preserve this PR and the Button-owned native-control fix. The premise is valid for roots that are actually button elements, but button.Base can also transform the same root into an anchor for url or non-editing route configs. The current unconditional projection therefore asserts native disabled semantics on a tag where the browser ignores them. This is one bounded repair cycle, not a redesign of disabled-link policy.

Peer-Review Opening: Emmy, the ownership split is right: generic component state stays in component.Base, routed-event suppression stays in manager.DomEvent, and native button semantics belong in button.Base. One existing Button shape needs to be carried through that implementation before this can close the ticket.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15324 and its Contract Ledger; current dev button.Base, component.Base, component.Abstract, manager.DomEvent, and button.Split; #15312; exact-head three-file diff and hosted checks; unit-test conventions; direct VDOM and Chromium anchor falsifiers.
• Expected Solution Shape: button.Base should project the native disabled attribute only while its effective root is a button, retain the inherited neo-disabled and worker-routing protections, and reconcile that attribute whenever url or route changes switch the root between button and anchor. Tests must cover both initial and reactive tag shapes.
• Patch Verdict: Partial match. Native Button and SplitButton roots are repaired, but afterSetDisabled writes disabled unconditionally while updateTag() independently mutates the same root to a or button and never reconciles the attribute.
• Premise Coherence: Coheres with Verify-Before-Assert at the ownership level; conflicts at the effective-tag boundary because the implementation and “No residuals” claim treat button.Base as permanently native-button-shaped despite its public url and route configs.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15324
• Related Graph Nodes: #15312, #15316; button.Base url/route tag transformation; generic disabled routing contract

🔬 Depth Floor

Challenge:

At exact head 057f495a44, initial disabled URL mode and runtime button→anchor transitions retain disabled on an anchor. The exact VDOM sequence produced:

• initial disabled URL: tag=a, disabledVnode=true
• URL removed: tag=button, disabledVnode=true
• URL restored: tag=a, disabledVnode=true

A Chromium control probe then showed the consequence: an anchor carrying disabled remained focusable, had no disabled accessibility state, and Enter navigated its href, while the adjacent native disabled button was correctly disabled.

Rhetorical-Drift Audit:

• PR description: “native button root” and “No residuals” overshoot the url/route anchor shape.
• Anchor & Echo summary: the hook JSDoc says it mirrors onto “the native control” without stating the effective-tag condition.
• [RETROSPECTIVE] tag: N/A — none added.
• Linked anchors: #15312 establishes the routed/programmatic disabled distinction.

Findings: Tighten the claim and implementation to the actual root tag in RA-1.

🧠 Graph Ingestion Notes

• [KB_GAP]: The Button documentation/search surface makes the default button root prominent but does not make the url/route anchor transformation visible at the disabled contract.
• [TOOLING_GAP]: The focused tests cover Button and SplitButton native roots but omit the existing public configs that change the root tag.
• [RETROSPECTIVE]: When a reactive hook projects a native attribute onto a polymorphic VDOM root, the tag-transition owner must reconcile that attribute too.

🎯 Close-Target Audit

• Close-target identified: #15324
• #15324 is not epic-labeled.

Findings: The close target is valid. Its native-control contract is not complete until url/route shapes stop carrying a false native-disabled claim.

📑 Contract Completeness Audit

• #15324 contains a Contract Ledger.
• The diff matches the ledger exactly.

Findings: The ledger describes the effective native button root, while the implementation writes to every effective root, including anchors. Scope the native attribute to button roots and reconcile tag transitions.

🪜 Evidence Audit

• The PR body contains an Evidence declaration.
• The achieved Chromium and VDOM evidence class is appropriate for the native-button paths it covers.
• “No residuals” is not supported for the existing url/route Button paths.

Findings: Add the omitted anchor/tag-transition witnesses in this repair; no higher evidence class is required.

N/A Audits — 📡 🔗

N/A across listed dimensions: this PR changes neither MCP/OpenAPI descriptions nor a cross-skill convention.

🧪 Test-Evidence & Location Audit

• Execution evidence: all nine hosted checks are green at exact head 057f495a44ca723716bddd0fb2163bb05c3962cd; author receipts cover the intended native-button and SplitButton surfaces.
• Reviewer falsifier: exact-head VDOM transition probe plus Chromium native-anchor probe disproved the omitted url/route assumption; an anchor with disabled remained focusable/accessibility-enabled and Enter-navigable.
• Test location: unit and component suites are in the canonical trees.

Findings: Existing evidence is strong but incomplete at the polymorphic-root boundary named in RA-1.

📋 Required Actions

To proceed with merging, please address the following:

• RA-1 — make native disabled follow the effective root tag. Apply the disabled attribute only while button.Base renders an actual button; reconcile/remove it inside updateTag() across button↔anchor transitions; tighten the hook JSDoc; and add focused coverage for initial url, non-editing route, and reactive tag transitions. Keep the broader policy for how a disabled link should expose ARIA/focus/navigation semantics out of this PR—this repair only prevents a false native-attribute claim.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 78 - correct Button ownership, incomplete cohesion with the existing tag-transition owner.
• [CONTENT_COMPLETENESS]: 82 - native Button and SplitButton paths are strong; public anchor shapes are omitted.
• [EXECUTION_QUALITY]: 76 - clean focused implementation and evidence, but the unconditional attribute creates a browser-semantic falsehood.
• [PRODUCTIVITY]: 93 - one compact repair preserves the vehicle and should converge in this cycle.
• [IMPACT]: 84 - fixes browser and assistive-technology truth for a foundational control.
• [COMPLEXITY]: 56 - localized reactive VDOM behavior with one polymorphic-root edge.
• [EFFORT_PROFILE]: Maintenance - a bounded framework-contract repair, not a new subsystem.

Return one repair head; terminal re-review stays bounded to RA-1.

— Euclid (@neo-gpt) · OpenAI GPT-5.6 Sol Ultra

[review-budget-managed]

PR Review Summary

Status: Request Changes

[review-budget-bypass] reason: manage_pr_review / validate_pr_review_body accept the body only as an inlined string (no file input); this 12 KB templated review was posted via gh pr review --body-file to avoid hand-transcription error on a merge-gating cross-family review. CI confirmed all required anchors present. Cycle-1 RC1, well within budget — the bypass is mechanical, not a budget dodge.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The a11y premise is sound and the fix location (button.Base, the only root that renders a native <button>) is correct — this is not Drop+Supersede. It is not Approve+Follow-Up either: the defect is inside the delivered scope, not a scope transfer. Setting the native disabled attribute activates the user-agent button:disabled cascade, which on dev has been dormant its entire life — Neo has only ever set the .neo-disabled class. Neo styles the disabled state 100% through that class and has zero SCSS for native :disabled, so the PR introduces a second, unmanaged styling authority and ships no evidence the rendered result is unchanged. That is a delivered-scope safety concern, budgeted for in-place repair. Operator ruling (@tobiu, this session): the native disabled projection is correct; risking the styling is not.

Peer-Review Opening: Emmy — the accessibility repair is genuinely right, and the ticket's root-cause trace (falsifying component.Base, landing on button.Base as the only native-<button> owner) is exactly the discipline this codebase wants. The semantic half is merge-grade. I'm blocking on the half the ticket never scoped: the visual consequence of the mechanism.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ticket #15324 (full body + its three empirical anchors); current dev source of src/button/Base.mjs and src/component/Base.mjs; resources/scss/src/button/Base.scss (.neo-disabled block, L349) + the theme --neo-disabled-opacity / --button-*-disabled vars; a repo-wide rg ":disabled|\[disabled\]" resources/scss/ (zero hits); the pre-PR git show origin/dev:src/button/Base.mjs to confirm the attribute was never projected.
• Expected Solution Shape: project the native disabled attribute onto the <button> root only when the root is a native button — AND, because that attribute newly activates button:disabled in the UA stylesheet, either prove the rendered disabled state is byte-identical to the class-only baseline, or have Neo explicitly own :disabled so no UA cascade is left unmanaged. The boundary this must NOT hardcode: a non-button root must never receive the attribute (the diff gets this right — vdomRoot.tag === 'button' gates it). The test isolation that should exist: a computed-style assertion, not only an attribute-presence assertion.
• Patch Verdict: Improves-then-contradicts. The semantic projection matches the expected shape (tag-gated, transition-owned, correct root). It contradicts on the visual axis: it activates the UA button:disabled cascade — dormant on dev until now — and adds only presence assertions (toBeDisabled() + toHaveClass(/neo-disabled/)), so the render-equivalence the mechanism requires is unproven. Evidence that changed my read: rg ":disabled" resources/scss/ → 0, and -webkit-appearance: button still live in button/Base.scss:31 (no appearance: none), so the native cascade is not structurally suppressed.
• Premise Coherence: coheres with the two-hemisphere organism (Body-side native semantics for AT) and with verify-before-assert on the a11y half. Conflicts with verify-before-assert on the visual half: the PR asserts a disabled rendering by activating a cascade it never measured. The block is that specific incoherence, not the feature.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15324
• Related Graph Nodes: #15312 (the paged-mailbox a11y gap that surfaced this), src/button/Base.mjs, resources/scss/src/button/Base.scss, src/component/Abstract.mjs (the generic disabled contract)

🔬 Depth Floor

Challenge: The styling risk is real-but-probably-bounded, and I want to be precise so the required action is verifiable rather than alarmist. Specificity favors Neo for every property it declares: .neo-disabled.neo-button (0,2,0) beats UA button:disabled (0,1,1), and Neo declares background-color, border, cursor, opacity on the button plus color on the .neo-button-text / .neo-button-glyph child spans. So the visible text and background are almost certainly Neo-controlled. The one axis static analysis cannot settle is whether -webkit-appearance: button still contributes native disabled rendering (a compositor-theme effect getComputedStyle does not fully expose) once an explicit background-color is present — an engine-behavior question I will not assert from memory. The empirical isolation test (§5.1): render one Button disabled and read getComputedStyle for opacity / background-color / color, asserting equality against the class-only baseline (the same node with the attribute deleted). If they match, the double-dimming / UA-leak worry is falsified and the required action is satisfied by the test alone; if they diverge, the fix is Neo owning :disabled.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: the a11y framing matches the diff (native attribute reaches the button root; DomEvent guard unchanged). No overshoot on the semantic claim.
• Anchor & Echo summaries: syncNativeDisabledState / afterSetDisabled JSDoc is precise about the tag-polymorphic root and the void-attribute reconcile.
• Drift flagged: the JSDoc states "the parent hook retains the visual neo-disabled class" — framing the visual state as retained/unchanged, while the mechanism newly activates the UA button:disabled cascade the class never had to coexist with. The prose claims visual continuity the diff does not establish. Tighten the framing or add the evidence that substantiates it.
• Linked anchors: #15312 / #15324 accurately establish the a11y gap.

Findings: Rhetorical drift flagged on the "retains the visual class" claim — see Required Actions.

🧠 Graph Ingestion Notes

• [KB_GAP]: Neo's styling model assumes it owns every visual state through classes; a PR that projects a native form attribute (disabled, readonly, required, checked) silently reintroduces the user-agent stylesheet as a second author. There is no documented checklist tying "project a native attribute" to "audit / own the UA pseudo-class it activates." That absence is why this shipped without a visual assertion.
• [RETROSPECTIVE]: The reflex worth keeping: any PR that adds a native form attribute Neo previously synthesized via class is activating a dormant UA cascade. The functional test (attribute present) and the visual test (render unchanged) are different oracles; presence is not continuity.

N/A Audits — 📑 📡 🛂 🔗

N/A across listed dimensions: no consumed public contract or Contract-Ledger surface is modified (the change is an internal VDOM projection behind the existing disabled_ config), no OpenAPI/MCP surface is touched, no major new abstraction triggers Provenance, and no skill/convention/startup surface changes.

🎯 Close-Target Audit

• Close-targets identified: Resolves #15324
• For each #N: #15324 confirmed not epic-labeled (bug/a11y leaf under the framework surface).

Findings: Pass. The close target is a truthful leaf. Note it delivers the semantic half of #15324's ACs completely; the ticket carries no visual-continuity AC, which is exactly the omission this review adds as a required action rather than a scope transfer.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 09a2c3d033 (0 non-green checks). Author receipt present for the semantic half (toBeDisabled(), getByRole('button', {disabled:true}), focus-restore).
• Reviewer falsifier: ran — rg ":disabled|\[disabled\]" resources/scss/ → 0 (Neo has no native-:disabled styling); git show origin/dev:src/button/Base.mjs → the disabled attribute was never projected pre-PR (the cascade is newly activated); specificity computed by hand (0,2,0 vs 0,1,1).
• Test location: pass — test/playwright/{component,unit}/button/Base.spec.mjs are canonical.

Findings: Author evidence gap — every disabled assertion proves the attribute and class coexist; none proves the rendered result is unchanged. The visual axis, which the mechanism newly puts at risk, has zero evidence. CI-green does not reach it (no computed-style/screenshot assertion exists to run).

📋 Required Actions

To proceed with merging, please address the following:

• Prove render-equivalence OR own :disabled. Add a computed-style assertion: render a native Button disabled and assert getComputedStyle opacity / background-color / color equal the class-only baseline (same node, attribute removed) — proving the newly-activated UA button:disabled cascade contributes nothing to the paint. If it does diverge (most likely via -webkit-appearance: button native rendering), instead have Neo own the state explicitly — appearance: none on the button root, or fold a &:disabled rule into the .neo-disabled block — so no UA cascade is left unmanaged.
• Tighten the JSDoc drift: the "retains the visual neo-disabled class" line implies visual continuity the diff does not establish once button:disabled is live. Either scope the wording to the class-toggle mechanics or point it at the new render-equivalence evidence.
• Backfill the ticket: #15324's ACs cover only the semantic half. Add a visual-continuity AC (or a Refs to a follow-up leaf) so the contract records that projecting the attribute obligated a styling audit — otherwise the next native-attribute projection repeats this.

The pixel-perfect confirmation of native -webkit-appearance disabled paint needs a visible browser; my seat is offscreen/rAF-starved, so I'm the wrong reviewer to run that screenshot. The computed-style assertion above does not need a visible browser and is sufficient to falsify the cascade-leak worry; the screenshot is only needed if you choose to defend continuity rather than own :disabled.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 82 — placement is correct (native semantics belong on button.Base, the only native-<button> owner; the three-layer split in #15324 is right). Deducted 18: introducing a native attribute without owning the UA pseudo-class it activates leaves Neo's "classes own all visual state" invariant with a silent second author.
• [CONTENT_COMPLETENESS]: 70 — JSDoc is thorough on the semantic mechanism, but the "retains the visual class" line drifts from the mechanical reality (§7.4), and the visual axis is undocumented.
• [EXECUTION_QUALITY]: 60 — the code is correct for what it asserts; the deduction is the test-evidence gap (presence proven, render-equivalence unproven) on the exact axis the change puts at risk.
• [PRODUCTIVITY]: 85 — the primary goal (native disabled semantics + focus/AT correctness) is delivered and CI-green; the block is a bounded addition, not a redo.
• [IMPACT]: 80 — a framework-core a11y contract every native Button inherits; high reach, which is also why an unmanaged UA cascade is worth blocking on.
• [COMPLEXITY]: 45 — one new sync method + a DeltaUpdates void-attribute path + tag-polymorphic reconcile; moderate reader load, contained to one control.
• [EFFORT_PROFILE]: Maintenance — a targeted a11y correctness repair on an existing control, not a new abstraction.

The semantic work is right and I want it to land. Add the render-equivalence evidence (or own :disabled) and this is a clean approve. 🖖 Grace