PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The macro shape is right: one dashboard-owned store over the landed dockLayoutCollection.v1 schema, with injected persistence and DockZoneModel validation. Three direct probes break its binding atomicity/CRUD contracts, so this should converge in place before the switcher consumes it.

Grace, keep the class and location. The required pass is behavioral: isolate public reads, preserve a valid active record on delete, and enforce one reachable name/id namespace.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14669; HarnessDockZoneModel.md; the landed DockZoneModel collection validators/migration helpers; Neo config getter semantics; sibling dashboard stores; and the changed spec.
• Expected Solution Shape: A dashboard-owned CRUD/list facade over the existing saved-layout collection, with no new persistence schema, plain-data boundaries, whole-candidate validation, explicit collision decisions, and caller-injected storage.
• Patch Verdict: Correct ownership and reuse, but the public getter currently escapes the atomic seam, active deletion fails in a normal multi-record collection, and cross-namespace collisions create unreachable records.
• Premise Coherence: The PR says every read/write crosses as a clone and every mutation funnels through commit(). Exact-head probes falsify both assertions through the generated collection getter.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14669 under #13158
• Related Graph Nodes: #14668 · #14670 · DockZoneModel · dockLayoutCollection.v1 · HarnessDockZoneModel.md

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: Mutating store.collection.layouts directly changes internal state because object-valued Neo config getters clone only when cloneOnGet is explicitly configured. That bypasses beforeSetCollection() and commit(). A subsequent persist() reports success and writes a collection that DockZoneModel.validateSavedLayoutCollection() rejects.

Rhetorical-Drift Audit (per guide §7.4):

• No new persisted schema is introduced.
• Hydration and authored mutations validate whole candidates.
• “Every read or write crosses as plain JSON clones” is false for store.collection.
• “Every mutation funnels through commit()” is false for nested public-getter mutation.
• renamePerspective does not accept the promised explicit replace:true decision.

🧠 Graph Ingestion Notes

• [KB_GAP]: None; #14669 and HarnessDockZoneModel.md define the store boundary.
• [TOOLING_GAP]: The authored five-case spec lacks adversarial getter-mutation, active-delete-with-sibling, and cross-name/id collision probes.
• [RETROSPECTIVE]: Whole-candidate validation is only atomic when public reads cannot mutate the held candidate behind the setter's back.

🎯 Close-Target Audit

• #14669 is the correct non-epic leaf.
• CRUD/list/persistence belong in one coherent store lane.
• Delete does not work for the active record when any sibling remains.
• Collision semantics do not keep both technical ids and product names reachable.

Findings: Converge here; splitting the store's core invariants into follow-ups would leave the next UI slice consuming an unsafe contract.

📑 Contract Completeness Audit

• The class documents storage, migration, events, and collision intent.
• Existing events fire after authored commits with plain payloads.
• Public collection reads are not clone-isolated despite the binding class contract.
• renamePerspective claims save-equivalent replacement semantics but has no options parameter.
• The new consumed class/events API has no Contract Ledger matrix in the PR evidence.

Findings: The first two are binding runtime/JSDoc mismatches. The ledger omission is evidence debt, not a separate architectural blocker.

🪜 Evidence Audit

• Exact-head CI is 9/9 green.
• Authored focused spec passed 5/5; syntax and git diff --check passed.
• Macro reuse of DockZoneModel validation/migration was verified.
• Direct getter mutation produced internally invalid state and persist() still returned {persisted:true}.
• Active A plus inactive B: removing A returned removed:false because null activeLayoutId is invalid while B remains.
• Saving id/name pairs first-id/shadow then shadow/Second succeeded twice, but lookup shadow returned first-id; the second record's technical id became unreachable.

📡 MCP-Tool-Description Budget Audit

Findings: N/A — no MCP tool surface changed.

🔗 Cross-Skill Integration Audit

• The store stays dashboard-owned and storage-technology agnostic.
• It reuses the landed collection authority instead of defining parallel validators.
• removePerspective() reimplements active-record removal differently from DockZoneModel.removeSavedLayout() and loses its explicit replacement invariant.
• resolveEntry() falls through to inherited layouts[name] rather than an own-property lookup.

🧪 Test-Execution & Location Audit

• Exact head 5e808be9052e8d5a36622b17fb564436da1ec957 audited.
• Store and spec are correctly placed under the dashboard substrate/unit suite.
• Current-head CI is 9/9 green and GitHub reports the branch CLEAN.
• No test attempts nested mutation through the public collection getter.
• No test deletes the active record from a collection with a remaining sibling.
• No test crosses one record's product name with another record's technical id or uses a prototype-shaped name.

Findings: The existing happy paths are useful but do not exercise the three invariants most likely to corrupt or strand a collection.

📋 Required Actions

To proceed with merging, please address the following:

• [P1] Restore the atomic read/write boundary. Make collection reads deep-cloned/isolated, keep all state changes behind commit(), and revalidate the held collection before persistence so a corrupted runtime value cannot be written with {persisted:true}. Add a getter-mutation falsifier that proves internal bytes and events remain unchanged.
• [P1] Make active-record deletion valid in multi-record collections. Require or derive a replacement activeLayoutId when siblings remain, ideally by reusing the landed DockZoneModel.removeSavedLayout() invariant. Test active A + inactive B → delete A → B remains valid and active.
• [P1] Enforce one collision-safe, fully reachable name/id namespace. Detect incoming layoutId against every existing perspectiveName, use own-property lookup for technical ids, and cover cross-namespace plus prototype-shaped names. A successful save must remain addressable by both documented resolution paths.
• [P2] Align rename's public contract. Either implement atomic replace:true parity or narrow the binding class/JSDoc claim and demonstrate the explicit caller-decision flow; do not advertise an ignored option.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 80 — correct store ownership, schema reuse, and injected persistence.
• [CONTENT_COMPLETENESS]: 74 — strong narrative; getter and rename contracts drift from runtime.
• [EXECUTION_QUALITY]: 52 — green happy paths, three reproduced state-contract failures.
• [PRODUCTIVITY]: 70 — coherent lane with a bounded hardening pass.
• [IMPACT]: 88 — this becomes the switcher's state authority.
• [COMPLEXITY]: 68 — localized store invariants, no schema migration.
• [EFFORT_PROFILE]: Medium Lift — retain the architecture; harden four public operations and their falsifiers.

The next exact head should be terminal if these runtime contracts and probes are green.