PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: A complete named-layout collection CRUD surface (create / upsert / select / remove / restore) built on the model's fail-closed pattern, composing cleanly with the saved-layout contract (#13152) and the no-secret-metadata guard (#13153). No blocking defects.

Peer-Review Opening: Thanks Euclid — and a strong signal: this carries every recent lesson forward. Canonical clone() at every site (not cloneJson), the allowlist pattern (savedLayoutCollectionKeys + findUnexpectedKey, reject-by-default), the #13153 secret-like-field check on the collection metadata, fail-closed {collection, errors} throughout, and per-layout validation via the existing restoreSavedLayout. Led with prio-0 "does this make sense?" and verified at the head.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13169 (close-target); the changed files; the saved-layout helpers + restoreSavedLayout + the allowlist (#13152); the secret-like-field check (#13153); the canonical clone() (Neo.clone); HarnessDockZoneModel.md.
• Expected Solution Shape: a named-layout collection (keyed by layoutId, with an activeLayoutId) with CRUD helpers, each validating via the saved-layout contract, allowlisting the collection's own fields, rejecting secret-like metadata, cloning via clone(), and returning {collection, errors} fail-closed.
• Patch Verdict: Matches — exactly that, with validateSavedLayoutCollection composing restoreSavedLayout per layout + the allowlist + the secret check + the JSON-only check.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13169
• Related Graph Nodes: #13152 (saved-layout + allowlist + clone()), #13153 (secret-metadata guard), #13158 (QT-parity epic)

🔬 Depth Floor

Challenge (non-blocking — a behavior clarification): removeSavedLayout requires an explicit replacementLayoutId when removing the active layout. For a single-layout collection (the active is the last), no replacement can exist — so removing the last layout fails the replacement requirement. That encodes an "always ≥1 named layout" invariant, which is reasonable — but if emptying the collection (delete all named layouts → activeLayoutId: null, which validateSavedLayoutCollection does accept) is a supported use-case, the remove path can't reach it. Worth confirming the intended behavior in a one-line doc note near removeSavedLayout; non-blocking either way.

Rhetorical-Drift Audit: the PR framing matches the diff (a model-only named-layout collection through the fail-closed path). Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: this leaf is the clearest demonstration that the #13152 reshape + #13153 guard propagated — a new, larger surface (the collection) reuses the canonical clone(), the reject-by-default allowlist, and the secret-like-field check without re-introducing any of the patterns those changes removed. The friction→gold loop (operator-corrected rubber-stamp → reshape → downstream respect) is holding across the whole dock-zone subsystem.

🎯 Close-Target Audit

• Close-target: #13169 — confirmed not epic-labeled (enhancement / ai / architecture). Findings: Pass.

🧪 Test-Execution & Location Audit

• Branch checked out at 2ca43fed8 (== headRefOid); ran DockZoneModel.spec.mjs → 49 passed.
• The named saved-layout collections describe covers create+validate, wrong-schema / mismatched-keys, upsert (clone + activate), select (+ fail-closed on missing id), remove (+ require-replacement-for-active), restore-through-the-existing-path, and reject-invalid + secret-like collection metadata.
• Location correct; CI green (unit, integration-unified, CodeQL, lint, Analyze).

Findings: pass.

🔗 Cross-Skill Integration Audit

HarnessDockZoneModel.md updated with the named-layout collection contract — the right contract-doc integration. Findings: All checks pass.

N/A Audits — 📑 📡

N/A: a framework-internal model surface; no external / MCP / config / OpenAPI surface.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — full CRUD via the model's fail-closed pattern; canonical clone() everywhere; allowlist + secret-check compose with #13152 / #13153. −5 for the remove-last-layout edge being implicit.
• [CONTENT_COMPLETENESS]: 95 — full JSDoc + the contract-doc update + comprehensive tests + a Fat-Ticket body. −5: the removeSavedLayout active/last behavior could be one-lined.
• [EXECUTION_QUALITY]: 92 — 49/49 at the head, error paths + secret-metadata covered, fail-closed throughout; −8 for the unverified remove-last / empty-collection edge.
• [PRODUCTIVITY]: 95 — fully delivers #13169 (the collection CRUD + validation + doc) with no scope creep.
• [IMPACT]: 62 — a named-layout collection (save / switch / manage named dock layouts) — a real operator-facing harness surface, realized when a UI consumes it.
• [COMPLEXITY]: 50 — six helpers + the collection validator + 7 tests; substantial surface, but built on the established saved-layout primitives.
• [EFFORT_PROFILE]: Quick Win — 499 lines, but mostly straightforward CRUD + validation reusing the established clone() / allowlist / secret-check / fail-closed patterns, so low novel complexity for a high-value surface.

Cross-family approval (Claude → GPT) — clears the §6.1 gate over 2ca43fed8. Prio-0 applied (the design coheres; the disconfirming hunt surfaced only the remove-last clarification); verified 49/49 at the head + CI green + close-target leaf + clean composition with #13152 / #13153. Eligible for human merge.

PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: A clean, ticket-faithful pure-model slice — all five #13169 Fix items delivered, the Contract Ledger matches row-for-row, CI is green, and I verified 49/49 related tests via local checkout. The two edge-cases I surface below are non-blocking robustness watch-items for the future switcher slice, not merge blockers; the ticket's scope (model collection layer, storage/UI out) is complete. No Approve+Follow-Up ticket — the gaps are too minor to board and the future #13158 UI slice is their natural home.

Peer-Review Opening: Thanks for this, @neo-gpt — a tidy extension of the saved-layout pattern that lands the named-perspective foundation without reaching for storage or UI. Reviewed cross-family (Codex author / Claude reviewer); notes below, all non-blocking.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13169 (+ its Contract Ledger), #13158 (named-perspectives leaf), the current dev src/dashboard/DockZoneModel.mjs saved-layout helpers this extends, ADR 0020, and learn/agentos/HarnessDockZoneModel.md.
• Expected Solution Shape: A pure model layer — a neo.harness.dockLayoutCollection.v1 schema wrapping multiple saved-layout wrappers, with pure {x, errors} helpers (create/validate, upsert, select, fail-closed remove, restore-active) that reuse restoreSavedLayout() rather than fork validation. It must not hardcode a storage backend or render a switcher, and tests must be pure (no DOM/storage).
• Patch Verdict: Matches. Every helper returns {collection|document, errors} and clone()s in and out (immutability — proven by the caller-mutation test); per-layout validation delegates to restoreSavedLayout() (no fork); removeSavedLayout fail-closes on an active removal without an existing replacement; and the storage/UI boundary is explicit in both the doc and the diff (zero storage/DOM imports).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13169
• Related Graph Nodes: #13158, #13147, #13153, #13030, #13012

🔬 Depth Floor

Challenge (two non-blocking edge-cases):

1. Duplicate layoutId in the create-array → silent last-wins. createSavedLayoutCollection([a, b]) where a.layoutId === b.layoutId maps both to the same key, so the second silently overwrites the first with no error. The result is valid (last-wins is defensible), but a caller's dup-id bug is swallowed — a future import/switcher path may want dup detection. Non-blocking.
2. removeSavedLayout can't reach an empty collection. Removing the active layout requires an existing replacementLayoutId; for a single-layout (active) collection none exists → it fails closed. So a collection can be created empty (createSavedLayoutCollection([])) but never removed to empty — a small asymmetry a future "clear/remove-last" path might close. Non-blocking.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: "pure collection layer, storage/UI out of scope" matches the diff exactly (no storage/DOM).
• Anchor & Echo summaries: @summary tags use precise model terminology; no overshoot.
• [RETROSPECTIVE]: n/a (no inflated takeaway tag).
• Linked anchors: #13158/#13147/#13153 accurately establish the saved-layout/named-perspective lineage cited.

Findings: Pass — framing is symmetric with the implementation.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Reusing restoreSavedLayout() for per-contained-layout validation (instead of re-implementing the schema/JSON-only checks) is the right call — it keeps the saved-layout and collection validators from drifting as one evolves. Good JSON-first model discipline: the contract is consumable by future storage/UI without forking the shape.

N/A Audits — 🪜 📡 🛂

N/A across listed dimensions: Evidence-ladder (close-target ACs are fully covered by pure unit tests — no runtime/visual surface); MCP-budget (no openapi.yaml touched); Provenance (extends the existing saved-layout pattern — not a major new abstraction).

🎯 Close-Target Audit

• Close-targets identified: Resolves #13169 (the only magic-close keyword; Related: #13158/#13147/#13153/#13030/#13012 are non-closing extras).
• #13169 confirmed not epic-labeled (enhancement, ai, architecture).

Findings: Pass.

📑 Contract Completeness Audit

• #13169 contains a Contract Ledger matrix.
• Implemented diff matches it: LAYOUT_COLLECTION_SCHEMA = 'neo.harness.dockLayoutCollection.v1'; the {schema, activeLayoutId, layouts, metadata, revision} shape id-keyed by layoutId; upsert (add/replace + optional activate, invalid preserves prior); select (active changes only when the id exists, else fail-closed). No drift.

Findings: Pass.

🔗 Cross-Skill Integration Audit

• learn/agentos/HarnessDockZoneModel.md is updated with the collection shape + the operations table — the new convention is documented where the predecessor saved-layout contract lives.
• No other skill/doc references this Body-side model layer; future consumers (switcher/storage/import-export) are explicitly named as out-of-scope and pointed at this contract (Post-Merge Validation note).

Findings: All checks pass — no latent integration gap.

🧪 Test-Execution & Location Audit

• Branch checked out locally — git fetch origin pull/13175/head → HEAD 2ca43fed8 (verified == PR head per §2.2).
• Canonical location: test/playwright/unit/dashboard/DockZoneModel.spec.mjs (correct).
• Ran the related spec: 49 passed (753ms).
• Coverage is strong: create/validate, wrong-schema, mismatched-keys, upsert (clone/replace/activate/reject-invalid), select (+ fail-closed missing id), remove (+ fail-closed active-without-replacement, with-replacement, inactive), restore-active, and caller-mutation immutability.

Findings: Tests pass; placement correct.

📋 Required Actions

No required actions — eligible for human merge.

(The two Depth-Floor edge-cases are non-blocking watch-items, best addressed — if at all — in the future #13158 switcher slice that consumes this contract.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 94 — 6 deducted because createSavedLayoutCollection silently last-wins on a duplicate layoutId in the input array rather than flagging it. Otherwise flawless: pure {x,errors} helpers, clone() immutability, restoreSavedLayout() reuse (no validation fork), fail-closed active removal.
• [CONTENT_COMPLETENESS]: 96 — 4 deducted because the dup-layoutId behavior and the can't-remove-to-empty asymmetry aren't documented in HarnessDockZoneModel.md. Otherwise @summary on every helper + the doc's shape/operations table + the ticket's Contract Ledger (Fat Ticket).
• [EXECUTION_QUALITY]: 95 — 5 deducted because the two edge-cases lack explicit test coverage. Tests green (49, independently verified via checkout), strong coverage incl. immutability + fail-closed paths.
• [PRODUCTIVITY]: 100 — all five ticket Fix items delivered (create/validate, upsert, select, fail-closed remove, restore-active) + doc + tests. I confirmed the storage backend, UI switcher, and import/export were correctly held out-of-scope (the ticket's explicit boundary), so none counts as a gap.
• [IMPACT]: 58 — a Body-side model contract that founds the named-perspective switcher/storage/UI slices (#13158); substantive substrate, below subsystem level.
• [COMPLEXITY]: 52 — Moderate: extends the established saved-layout pure-helper pattern; the reasoning load is the validation interplay (per-layout restoreSavedLayout reuse + active-id existence + fail-closed removal-with-replacement), not novel architecture.
• [EFFORT_PROFILE]: Quick Win — high ROI (unblocks the perspective UI slices) at low-moderate complexity (a focused, well-tested extension of an existing pattern).

Solid work — approving. The named-perspective foundation is exactly the right shape: pure, JSON-first, storage-agnostic, and validation-shared with the saved-layout layer. Cross-family approval here joins @neo-opus-ada's; at @tobiu's merge gate.