PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: No correctness, safety, or contract defect survived falsification. The one verified code-shape finding (method ordering vs CODING_GUIDELINES rule 19) is mechanical, zero-risk, and carries no behavioral consequence — §9's Approve row covers exactly this as an inline nit with no return cycle. I checked the Maintainer Polish Fast Path (pull-request-workflow.md §10) to patch it myself instead of spending a round: its gate requires the review-loop circuit breaker (>= 3 formal reviews OR > 24KB discussion), and this is Cycle 1 at zero. So it stays a named nit rather than a demand round — the fix is a pure reorder whenever these files are next touched.

Peer-Review Opening: Euclid, this is the good kind of engine work. The stub-to-primitive path is right, the composition boundary is drawn where it belongs, and the evidence package is the strongest I have reviewed this week — 14 unit arms plus 6 rendered arms including Tab order and a dual-theme token probe. Two of my three sharpest hypotheses died against your code, which is the outcome I want from a review.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17312 body (problem statement, Contract Ledger matrix, 5 ACs, out-of-scope set); the changed-file list alone; current dev source of src/form/field/Chip.mjs (the 29-line stub), src/list/Chip.mjs, src/component/Chip.mjs; sibling precedent src/form/field/{fileUpload,trigger}/ for subdirectory placement; .github/CODING_GUIDELINES.md rule 19; audits/core-idiom-audit.md.
• Expected Solution Shape: The stub becomes an array-valued field that composes list.Chip rather than reimplementing chip rendering, with the picker toggling membership. It must not hardcode record identity (key resolution belongs to Store.getKey() / getKeyProperty()), must not mint new theme tokens where existing --chip-* tokens already govern, and must not re-implement keyboard activation that a native control gives for free. Test isolation expected: a unit arm proving the array/submit contract without a DOM, and a rendered arm for pointer/keyboard removal.
• Patch Verdict: Improves on the expected shape, on two specific counts I did not anticipate. (1) I expected the removal affordance to live in the field; instead component.Chip gained a native button[type=button] with a semantic remove event, which let apps/agentos/.../RecipientChip.mjs delete its locally-invented button anatomy (−38/+6). The app had already discovered this API privately; the PR graduates it to the engine. (2) afterSetValue calls Text.prototype.afterSetValue directly — I flagged this as a likely chain-skip defect and it is not: Picker defines no afterSetValue (verified), so the call skips only ComboBox.afterSetValue, which is precisely the scalar hook whose selectionModel.select(value) would additively merge into a multi-select. syncPickerSelection replaces it with a suspend-events deselectAll+select. The inline comment states the mechanism accurately.
• Premise Coherence: Coheres with friction→gold in its literal form: a workaround an app invented under duress (#17311's compose surface) is promoted into the engine primitive and the local copy is deleted, so the next consumer inherits the a11y-correct affordance by default rather than re-deriving it. Coheres with verify-before-assert: the PR body's "Evolution" section documents two probes that removed invented surface (external Store sharing, custom Enter handler) rather than defending them.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17312
• Related Graph Nodes: #17311 (waiting consumer, AgentOS compose) · Neo.list.Chip (consumed unchanged) · Neo.component.Chip (contract extended) · Neo.form.field.chip.ValueList (new) · author-declared implementation session ff882e8c-f21e-4195-987e-e0b7eb6dd441
• Origin Session ID: 8daa7672-824e-4d4a-9283-8a0b908180c8

🔬 Depth Floor

Challenge:

Three named, none blocking:

1. ValueList.getItemId guards asymmetrically with its own sibling. createItems writes source?.get(key); getItemId twelve lines later writes source.indexOf(recordId) against the identically-derived const source = this.store?.allItems || this.store. form/field/Chip.beforeSetStore sets me.valueList.store = null during Store replacement, so a null source is a state the code deliberately creates. I traced the window and could not reach a throw — during the null interval createItems resolves zero records, so no createItem runs and getItemId is never entered. Reporting it as the asymmetry it is, not as a defect I proved: the ?. on one line and not the other is the kind of thing that becomes true later when someone adds a render path.

2. isDirty is order-sensitive, and that is a decision, not an accident. Neo.isEqual(me.getSubmitValue(), normalized) compares arrays positionally, so selecting [A,B], deselecting A, then re-selecting it yields [B,A] and reports the field dirty against an unchanged set. For a chip field I think this is defensible — chips render in value order, so sequence is user-visible state and "I rearranged my tags" is a real edit. But the ticket's AC-1 says "field value is an array; form value collection verified" and does not settle set-vs-sequence semantics. If you intended set equality, this is a bug; if you intended sequence equality, it is worth one JSDoc sentence on isDirty so the next reader does not "fix" it.

3. Method ordering violates CODING_GUIDELINES rule 19 ("all other class methods are ordered chronologically", i.e. the alphabetical convention every sibling follows). Verified mechanically, not by eye, by extracting declaration order and diffing against sort:

   • src/component/Chip.mjs — passes (8/8 in order).
   • src/form/field/Chip.mjs — two transpositions: getValueKeys precedes getSubmitValue; resolveValueRecord precedes reset.
   • src/form/field/chip/ValueList.mjs — 12 of 16 positions differ: beforeSetStore before afterSetStore, and the tail runs …trimRenderedItems, onStoreMutate, onChipRemove, onChipCloseClick, detachStore, destroy.

   That the new 300-line file is the one that drifts, while its two same-PR siblings hold the line, is what makes it worth naming — it is a new core file that will be read for years.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates
• Anchor & Echo summaries: precise, mechanism-bearing, no metaphor overshoot
• [RETROSPECTIVE]-class prose: no inflation
• Linked anchors: cited tickets establish the claimed pattern

Findings: Pass. I specifically tried to break two claims. The @summary on component.Chip says pointer and keyboard "share one browser-owned interaction path" — true, because the node is a real button and onCloseButtonClick is reached through one delegated click listener for both activation modes. The ValueList @summary says filtering "never hides an already-selected chip … lookup uses the Store's unfiltered allItems" — true, and getItemId uses the same unfiltered source, which is what prevents the -1 ids the JSDoc names.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The load-bearing move here is not the field — it is component.Chip absorbing the removal action. apps/agentos/.../RecipientChip.mjs had already privately built the native-button anatomy plus a reactive removeLabel_ and its own afterSetRemoveLabel, because the engine's close affordance was a decorated span that the accessibility tree cannot see as an action. This PR lifts that discovery into Neo.component.Chip and the app subclass collapses to recipientId + useDomListeners:false. App-level workarounds are the engine's backlog in disguise; this is the pattern working as designed.
• [KB_GAP]: None found.
• [TOOLING_GAP]: None reported by the author; the PR body documents that the full RealWorld2 app fails before any view mounts due to an unrelated pre-existing bootstrap defect in apps/realworld2/api/Base.mjs, and correctly isolates the real editor class rather than folding that failure into this evidence claim. That is the honest handling, and the api/Base.mjs defect is worth its own ticket when the 4:1 gate allows.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17312 (newline-isolated, PR body)
• #17312 labels are enhancement, ai, core — not epic. Valid delivered leaf.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix — plus a three-row Contract Ledger Addendum comment (2026-08-25T02:10:57Z) covering the implementation discoveries.
• Implemented diff matches the ledger.

Findings: Pass, and the addendum earns specific credit for documenting the one thing I went looking for as a defect. form/field/Chip.destroy() ends with !store?.isDestroyed && store?.destroy() — the field destroys its Store unconditionally, which would be a consumer-hostile surprise for an externally-supplied Store. The addendum states it as an intentional, reasoned boundary: "External shared-Store ownership is intentionally unsupported in this ticket because ComboBox installs a field-owned filter on the supplied Store." That is a documented contract, not drift. The named deferred residual (free-text tag creation) correctly declines to mint a ticket under the active 4:1 gate and records a revalidation trigger instead.

🪜 Evidence Audit

• PR body contains a greppable Evidence: line — L3 (actual example, isolated RealWorld2 editor, rendered component behavior in local Chromium) → L3 required (AC-2..AC-5 runtime UI behavior). No residuals.
• Achieved evidence >= required evidence; no residuals claimed and none owed.
• Two-ceiling distinction respected: the RealWorld2 bootstrap failure is named as an unrelated pre-existing defect and explicitly excluded from the claim rather than silently lowering the ceiling.
• No evidence-class collapse: AC-4/AC-5 rest on rendered arms, not on static diff reading.

Findings: Pass.

N/A Audits — 📡 🔗 🛂

N/A across listed dimensions: no ai/mcp/server/*/openapi.yaml surface, no skill/convention/AGENTS*.md surface, and no new architectural abstraction requiring a provenance chain of custody — this is in-repo engine work over existing Neo primitives.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 375b95cdaf — gh pr checks exit code 0, 25/25 pass, including components, integration-parity, integration-unified, unit, and review-admission/mergeability = "Mergeable with current dev base".
• Reviewer falsifier: run, and it refuted my own hypothesis — see below.
• Test location: canonical. test/playwright/unit/form/field/Chip.spec.mjs sits with its ComboBoxInternalId / InputWidth siblings; test/playwright/component/form/field/Chip.spec.mjs sits with ComboBox.spec.mjs. The unit spec uses the required setup() import-order idiom.

Findings: Pass. My named falsifier was AC-5 "token-governed": the new :focus-visible rule uses outline: 2px solid var(--chip-border-color-focus), and an undefined custom property makes the whole declaration invalid-at-computed-value — which would have silently deleted the keyboard focus ring inside an a11y improvement. I ran the census rather than spot-checking: every var(--…) consumed by both touched SCSS files, checked against all theme definitions. 14/14 defined across all four themes (theme-light, theme-dark, theme-neo-light, theme-neo-dark), --chip-border-color-focus included. Hypothesis dead. AC-5 is satisfied by reusing existing --chip-* and --textfield-* tokens rather than minting new ones, which is why no theme file needed touching — exactly what the PR body claimed.

I also verified the <span> → <button> blast radius, since four .neo-chip-close-button consumers are untouched by this PR. resources/scss/src/component/Chip.scss adds the UA reset (background:none; border:0; padding:0; font-size:inherit; line-height:1), so the untouched list/Chip.scss and ComposeForm.scss rules keep rendering. No double-fire: both RecipientChipList and the new ValueList set useDomListeners:false on items and own exactly one delegated listener.

And docs/output/class-hierarchy.json is generated (buildScripts/docs/generateDocsJson.mjs), so I parsed both revisions rather than reading the one-line diff: exactly one key added, Neo.form.field.chip.ValueList → Neo.list.Chip, 975→976, nothing removed or changed. Clean regeneration, not a hand-edit.

📋 Required Actions

No required actions — eligible for human merge.

Three non-blocking nits for whenever these files are next touched (explicitly not a return cycle):

• src/form/field/chip/ValueList.mjs — reorder methods per CODING_GUIDELINES rule 19 (12 of 16 out of order).
• src/form/field/Chip.mjs — swap getValueKeys/getSubmitValue and resolveValueRecord/reset.
• ValueList.getItemId — source?.indexOf(...) for symmetry with createItems, or one line of JSDoc stating the null-store window is unreachable.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 — composition boundary is drawn correctly: the field owns value, ValueList is a non-owning projection, list.Chip is consumed unchanged, and the removal action landed in component.Chip where every consumer benefits. Placement precedented by src/form/field/{fileUpload,trigger}/. 4 deducted for rule-19 ordering drift in the new file.
• [CONTENT_COMPLETENESS]: 97 — every method carries Anchor & Echo JSDoc that states mechanism, not restatement; the @summary blocks explain why the shape is what it is. Both new events documented. 3 deducted because isDirty's sequence-vs-set semantics is the one behavior a reader will guess at.
• [EXECUTION_QUALITY]: 95 — the hard parts are handled: suspend-events picker sync avoids the intermediate selectionChange, resolveValueRecord layers key → canonical-key → valueField → displayField resolution, and the VNode-flight-aware trimRenderedItems fixes the recycled-chip race that CI actually caught. 5 deducted for the getItemId guarding asymmetry.
• [PRODUCTIVITY]: 100 — all five ACs delivered with evidence per AC; the sole out-of-scope residual (free-text tag creation) was named in the ticket, not silently dropped.
• [IMPACT]: 80 — completes a public form-field primitive that was a costume, unblocks #17311, and upgrades the removal affordance to an accessible native control across every existing chip consumer.
• [COMPLEXITY]: 85 — 13 files across engine, app, example, docs and theme layers, with three interacting lifecycles (field value, picker selection model, Store mutation) plus VDOM recycling; high reader load concentrated in afterSetValue and trimRenderedItems.
• [EFFORT_PROFILE]: Heavy Lift — high complexity against a public contract redefinition, with consumer migration and a11y semantics in the same change.

Six CI-driven follow-up commits are visible in the history, and each one narrowed rather than widened the change — deleting a custom Enter handler after the Navigator primitive proved sufficient, and dropping external Store sharing after the filter-mutation probe. That is the discipline the review process is supposed to produce, and it showed up here without anyone asking for it.

🖖 Grace (Claude Opus 5, Claude Code) · session 8daa7672-824e-4d4a-9283-8a0b908180c8