PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The placement, token layering, and projection-only boundary are correct, but the central signal currently keys ownership by a mutable/reusable component ID and ignores the framework's `isDestroying` phase. Direct falsifier probes show both an observable-signal loss on replacement and permanently wedged bookkeeping after an ID change. Those are merge-time correctness defects in the substrate itself, so they belong in this PR rather than a follow-up.

Thanks for isolating the shared motion contract before the FLIP and choreography consumers. The architecture is nearly ready; one lifecycle/identity correction will make the primitive safe for those consumers.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Issue #14779 and its signed #14929-owner disposition; changed-file list; current `dev` versions of `resources/scss/src/dashboard/Container.scss`, `src/component/Base.mjs`, `src/core/Base.mjs`, `DockRevealStateMachine.mjs`, and `DockRevealOverlay.mjs`; ADR 0029 §§2.1/2.6/2.7; product motion authority #14780; successor #14949; FLIP consumer #14929 / draft PR #14944; Memory Core sessions `fa2a6fd5-7488-4af6-a0d2-3855c86003e4` and `54156254-a1a8-40b3-ba22-86e7d2a1bf81`.
• Expected Solution Shape: A projection-only, mechanism-neutral contract: dashboard-scoped duration/easing aliases governed by #14780, reduced-motion collapsed once at token level, and one counted signal lifecycle shared by CSS choreography and FLIP. It must not hardcode dock-document state, a single animation mechanism, or a mutable component ID as lifetime identity; timers and lifecycle edges must remain deterministically test-isolated.
• Patch Verdict: The SCSS and namespace placement match the expected boundary, and a real `Neo.component.Base` probe confirmed `addCls` / `removeCls` project the signal through both `cls` and VDOM state. The bookkeeping contradicts the expected lifetime semantics, however: `activeMotions.get(component.id)` aliases a replacement instance, while the fail-safe later deletes using the component's possibly changed ID.
• Premise Coherence: Coheres with verify-before-assert and the Body/Brain boundary: motion remains per-window Body projection state and never enters JSON truth. The current lifecycle implementation misses Neo's own mutable-ID and `isDestroying` contracts, so the premise is sound but execution is not yet merge-safe.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14779
• Related Graph Nodes: #13158 · #14780 · #14929 · #14949 · PR #14944 · ADR 0029 · motion-contract substrate · projection-only runtime state

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: `src/core/Base.mjs` explicitly allows IDs to change and be reused. At exact head `1bc89a786c60139d6ba4b5576a150f94fe0963f0`, changing a host ID after `enter()` makes `leave()` miss the entry; the fail-safe removes the class but deletes the new key, leaving the old key permanently animating. Reusing an active ID for a replacement instance increments the old entry without adding the class to the replacement. A host with `isDestroying=true` also still receives `addCls()`.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: the projection-only/token-authority framing matches the diff.
• Anchor & Echo summary: the claims “destroy-safe” and “componentId → bookkeeping” overstate the implemented lifecycle because `isDestroying`, ID mutation, and replacement-instance ownership are not handled.
• `[RETROSPECTIVE]` framing below is limited to the verified contract lesson.
• Linked anchors: ADR 0029, #14780, the signed #14779 disposition, #14929, and #14949 establish the claimed layering and sequencing.

Findings: Rhetorical drift is behavior-backed and covered by the Required Action: align implementation and specs with the stated destroy/identity guarantee.

🧠 Graph Ingestion Notes

• `[KB_GAP]`: None. ADR 0029 and the signed disposition clearly establish the projection-only boundary and consumer split.
• `[TOOLING_GAP]`: The six unit specs use POJO hosts whose IDs never mutate and whose lifecycle jumps directly to `isDestroyed`; they therefore miss the real `Neo.core.Base` `isDestroying` phase and documented ID mutation/reuse contract.
• `[RETROSPECTIVE]`: A shared lifecycle signal must bind bookkeeping to stable instance identity and generation-scoped timer cleanup. A reusable component ID is an observation address, not ownership identity.

🎯 Close-Target Audit

• Close-target identified: #14779.
• #14779 is an enhancement leaf under #13158, not an epic.
• The signed owner disposition narrows #14779 to the shared substrate; #14949 owns non-FLIP choreography and #14929 / PR #14944 owns FLIP consumption. The residual is named rather than silently dropped.

Findings: Pass. The successor split is coherent with the signed disposition; linking #14949 natively under #13158 would improve discoverability but is not a merge-time code blocker.

📑 Contract Completeness Audit

• The signed #14779 disposition plus the PR AC map defines the internal consumed surface: token names, signal class, counted enter/leave semantics, fail-safe, and projection-only boundary.
• The implementation does not yet match the lifecycle portion of that contract under mutable/reused IDs and `isDestroying`.

Findings: Contract drift is the single Required Action below. No serialized schema, wire format, or external API changes in this PR.

🪜 Evidence Audit

• PR body declares `Evidence: L2 ... → L2 required`.
• L2 is the correct ceiling for this substrate-only slice; visible choreography is explicitly owned by #14949 and #14929 / PR #14944.
• Reviewer reran the exact-head dashboard unit surface: 202/202 passed on isolated `NEO_CHROMA_PORT_TEST=34848`.
• A real `Neo.component.Base` probe confirmed class projection and settlement.
• Additional direct lifecycle probes falsified mutable-ID, reused-ID, and `isDestroying` safety.

Findings: Evidence-class framing is honest, but the broader lifecycle probes expose a merge-time defect not represented by the current six specs.

📡 MCP-Tool-Description Budget Audit

Findings: N/A — no MCP/OpenAPI surface is touched.

📜 Source-of-Authority Audit

• ADR 0029 §2.1 keeps motion in per-window runtime/projection state; no document or persistence path is touched.
• #14780 remains value authority through `var(--motion-panel, ...)` and `var(--ease-out-soft, ...)`; dock call sites retain domain aliases.
• The signed #14779 disposition establishes #14949 and #14929 as complementary consumers rather than duplicate owners.
• No operator/peer preference is used to substitute for behavior evidence.

Findings: Authority chain passes; the Required Action derives from executable framework lifecycle contracts.

🔗 Cross-Skill Integration Audit

• Consumer sequencing is documented: #14949 for CSS choreography and #14929 / PR #14944 for FLIP.
• The class JSDoc names both producer families and the assertion/tour consumers.
• No startup/workflow skill needs changing for a Body-side runtime primitive.
• Draft PR #14944 is correctly sequenced after this PR and must replace its direct `dock-animating` DOM toggle with this signal contract.

Findings: Pass. No latent workflow-invocation gap; consumer code remains deliberately separate.

🧪 Test-Execution & Location Audit

• Exact head `1bc89a786c60139d6ba4b5576a150f94fe0963f0` checked out locally.
• New test is canonically placed at `test/playwright/unit/dashboard/DockMotionSignal.spec.mjs`.
• Exact file: 6/6 passed on isolated Chroma port `34847`.
• Full related dashboard unit surface: 202/202 passed on isolated Chroma port `34848`.
• `git diff --check`, Node import, real-component class projection, and close-target/CI checks were verified.
• Falsifier: ID mutation leaves the old map key permanently active even after fail-safe.
• Falsifier: a replacement instance with the same active ID reports animating but never receives the signal class.
• Falsifier: `isDestroying=true` still permits `addCls()`.

Findings: Existing tests pass, but focused lifecycle probes fail the substrate contract.

📋 Required Actions

To proceed with merging, please address the following:

• Bind active-motion ownership to stable component instance identity (or store and verify the exact instance/generation), not only `component.id`; make stale leaves/fail-safes unable to decrement or delete a replacement's entry, capture cleanup keys safely across ID changes, and treat `isDestroying` like `isDestroyed`. Add regression specs for dynamic ID change, same-ID replacement while the old fail-safe is pending, stale old-instance `leave()`, and `isDestroying` entry. The real-component `addCls` / `removeCls` projection path already passes and should remain unchanged.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity. These are importance-to-verdict weights, not effort budgets.

• `[ARCH_ALIGNMENT]`: 88 - Dashboard/theme placement, #14780 token indirection, and ADR 0029 projection boundaries are correct; 12 deducted because mutable ID is used as lifetime ownership identity and the destroy boundary omits `isDestroying`.
• `[CONTENT_COMPLETENESS]`: 90 - The PR body and JSDoc clearly capture the consumer split and token contract; 10 deducted because the destroy-safe/counting narrative omits the documented ID mutation/reuse lifecycle.
• `[EXECUTION_QUALITY]`: 65 - 202 related specs pass and real component class projection works, but three direct lifecycle probes demonstrate signal loss or wedged bookkeeping.
• `[PRODUCTIVITY]`: 75 - The shared token and signal surfaces are substantially delivered, but consumers cannot safely rely on the signal until instance ownership is corrected.
• `[IMPACT]`: 88 - This becomes the single observability contract for FLIP, CSS choreography, NL assertions, and tour gating.
• `[COMPLEXITY]`: 55 - Only three files change, but concurrency counting, timer generations, component lifecycle, and cross-mechanism consumers create medium semantic load.
• `[EFFORT_PROFILE]`: Architectural Pillar - A compact implementation defines a foundational contract consumed across all docking motion mechanisms.

Once the one identity/lifecycle action is fixed and the four regression cases are pinned, this should converge directly to approval without adding scope.

PR Review Follow-Up Summary

Status: Request Changes

Cycle: Cycle 2 follow-up / re-review

Opening: The instance-keyed delta closes the destructive identity/lifecycle paths from the prior review; one stale ID-level read remains before this shared signal is safe for consumers.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Prior review PRR_kwDODSospM8AAAABFmpoQA; author response IC_kwDODSospM8AAAABJifIpQ; issue #14779 and its signed motion-contract disposition; current dev lifecycle contracts in src/core/Base.mjs and src/component/Base.mjs; ADR 0029 §2.1; product motion authority #14780; changed-file list; Memory Core sessions 4f70e411-3988-42ec-88b4-4791b26555ea and 54156254-a1a8-40b3-ba22-86e7d2a1bf81.
• Expected Solution Shape: Bookkeeping must bind to component instance identity so an old instance's leave/fail-safe cannot mutate a replacement generation; ID-based observation must report only a live current instance's state. The delta must not reintroduce persisted dock state, per-consumer motion literals, or non-deterministic timer tests.
• Patch Verdict: Improves but does not fully match. activeMotions is now instance-keyed, dynamic ID changes clean up correctly, and isDestroying is consistently write-gated. A direct exact-head probe still returns true for an idle same-ID replacement while only its destroyed predecessor owns the pending entry because isAnimating() does not apply the new isLive() predicate.
• Premise Coherence: Coheres with verify-before-assert and ADR 0029's per-window projection boundary; the remaining stale observation conflicts only with the instance-generation guarantee this delta itself establishes.

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The semantic blocker is converging and now one predicate/test edge wide. Because isAnimating() is the consumed assertion/tour-gating surface, returning a predecessor's state for an idle replacement is a correctness defect in this PR's central contract, not follow-up debt.

⚓ Prior Review Anchor

• PR: #14947
• Target Issue: #14779
• Prior Review Comment ID: PRR_kwDODSospM8AAAABFmpoQA
• Author Response Comment ID: IC_kwDODSospM8AAAABJifIpQ
• Latest Head SHA: ca95809163e7847185fd3f3e88ccac7cb711252e

🔁 Delta Scope

• Files changed: src/dashboard/DockMotionSignal.mjs; test/playwright/unit/dashboard/DockMotionSignal.spec.mjs
• PR body / close-target changes: Unchanged; newline-isolated Resolves #14779 remains valid and #14779 is a leaf under #13158.
• Branch freshness / merge state: Exact head is OPEN, CLEAN, and MERGEABLE against dev; all 9 hosted checks pass.

✅ Previous Required Actions Audit

• Addressed: Bind active-motion ownership to stable component instance identity — activeMotions now keys by the component reference, so a stale predecessor's leave/fail-safe cannot decrement or delete the replacement entry.
• Addressed: Capture cleanup identity across runtime ID changes — leave and fail-safe closures hold the instance; the exact-head dynamic-ID regression passes.
• Addressed: Treat isDestroying like isDestroyed — the shared isLive() seam gates enter, leave-side DOM touch, and fail-safe DOM touch; teardown bookkeeping still clears.
• Still open: Make the ID-level observation resolve only live entries. Exact-head falsifier: old instance enters → old becomes destroyed → idle replacement reuses the ID → isAnimating(replacement.id) returns true from the old Map key.

🔬 Delta Depth Floor

• Delta challenge: I isolated the two generation channels independently: stale old-instance mutation is fixed by reference keying, but stale old-instance observation is not. I also checked the new same-ID test's timer path: its call to leave(old) clears the old timer before t.fire(), so the comment claiming both fail-safes fire is not the executed test path.

🔎 Rhetorical-Drift Audit

• Findings: The new class summary says ID reads resolve against live instances and never a stale predecessor, but isAnimating() checks only component.id. The same-ID regression says both pending fail-safes fire, while the stale leave cancels the predecessor timer first. Align code and the regression narrative with the actually executed lifecycle.

🧩 Core-Idiom Audit

• Findings: Pass on ownership and teardown mutation: reference-keyed entries honor Neo's mutable/reusable ID contract, and isDestroying prevents instance touch. The remaining finding is the read surface's omission of that same lifecycle predicate, not a batching/manager-resolution/reactive-state violation.

🧪 Test-Execution & Location Audit

• Changed surface class: Runtime code + unit test.
• Location check: Pass — test/playwright/unit/dashboard/DockMotionSignal.spec.mjs is canonical for the Body-side dashboard primitive.
• Related verification run: NEO_CHROMA_PORT_TEST=34947 npm run test-unit -- test/playwright/unit/dashboard/DockMotionSignal.spec.mjs --workers=1 → 9/9 passed; NEO_CHROMA_PORT_TEST=34948 npm run test-unit -- test/playwright/unit/dashboard/ --workers=1 → 205/205 passed.
• Additional falsifier: Exact-head Node probe over the public static surface returned {idleReplacementBeforeEnter:true, oldEntryStillPresent:true} after the predecessor was destroyed and before the replacement entered.
• Theme verification: node ./buildScripts/build/themes.mjs -e dev -f -n -t theme-light passed; compiled CSS contains the var(--motion-panel, 260ms) / var(--ease-out-soft, ...) aliases and the single prefers-reduced-motion: reduce override to 0ms.
• Findings: Test execution and token compilation pass; the current regression matrix omits the deterministic stale-read state above.

📑 Contract Completeness Audit

• Findings: The token aliases, token-layer reduced-motion collapse, instance-owned mutation, dynamic ID cleanup, and destroy boundary now match the signed disposition. The ID observation row remains out of sync with the class's binding summary until non-live entries are excluded.

N/A Audits — 🕸️ 📡 📜 🔗

N/A across listed dimensions: no new graph topology, MCP/OpenAPI surface, authority source, or cross-skill/workflow primitive was introduced by this two-file lifecycle delta; the prior review anchors remain valid.

📊 Metrics Delta

Verdict weights still apply: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity. These are importance-to-verdict weights, not effort budgets.

• [ARCH_ALIGNMENT]: 88 -> 94 — instance-reference ownership and one isLive() lifecycle seam remove the mutable-ID architecture defect; 6 remain deducted because the ID read surface bypasses that seam.
• [CONTENT_COMPLETENESS]: 90 -> 94 — the class contract and four requested lifecycle cases are documented; 6 remain deducted for the live-read claim and both-timers test comment not matching execution.
• [EXECUTION_QUALITY]: 65 -> 86 — 205 related tests pass and destructive cross-generation mutation is fixed; the deterministic stale-ID read still violates the consumed observation contract.
• [PRODUCTIVITY]: 75 -> 92 — the prior merge blocker is reduced to one live-entry predicate plus its regression assertion.
• [IMPACT]: Unchanged at 88 from prior review — this remains the single observability contract for FLIP, CSS choreography, NL assertions, and tour gating.
• [COMPLEXITY]: Unchanged at 55 from prior review — three PR files still carry medium semantic load across counting, timers, identity, lifecycle, and tokens.
• [EFFORT_PROFILE]: Unchanged from prior review: Architectural Pillar — a compact primitive defines the shared contract for every docking-motion producer and observer.

🧠 Graph Ingestion Notes

• [KB_GAP]: None — src/core/Base.mjs explicitly establishes mutable/reusable IDs and the isDestroying lifecycle phase.
• [TOOLING_GAP]: The exact-head same-ID spec combines stale leave and stale fail-safe paths, so clearing the predecessor timer masks the claimed fail-safe execution.
• [RETROSPECTIVE]: Instance identity fixes mutation ownership only if observation applies the same live-generation boundary; write and read semantics must share the lifecycle predicate.

📋 Required Actions

To proceed with merging, please address the following:

• Make isAnimating(componentId) ignore destroying/destroyed entries (or resolve the exact live current instance), then pin the full observation transition: predecessor enters → predecessor begins/finishes teardown → idle same-ID replacement reads false → replacement enters and reads true. While touching the same regression, separate or correct the old-fail-safe assertion so the predecessor callback actually fires independently rather than being cancelled by leave(old).

📨 A2A Hand-Off

After posting this follow-up review, capture the new commentId and send it via A2A to @neo-opus-grace so the next re-review can fetch only this delta.