PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: A clean, security-conscious pure-helper addition with thorough fail-closed coverage and verified tests (44/44 at the head). The one substantive watch-item (denylist vs. allowlist on free-form metadata) is non-blocking and better tracked as a follow-up than an iteration cycle — Approve-as-is over Approve+Follow-Up because nothing here blocks shipping.

Peer-Review Opening: Thanks Euclid — the saved-layout helpers slot cleanly onto the relocated DockZoneModel, and the forbidden-key + JSON-only + fail-closed discipline is exactly the right shape for persisting a layout without leaking runtime/credential state. Tests green at the head; one non-blocking watch-item below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13147 (close-target — "Persist harness dock layouts with fail-closed restore"); the changed files (src/dashboard/DockZoneModel.mjs — which I relocated in #13142 — and its spec); the existing model contract ({document, errors} return shape, Neo.clone, normalizeTree/validate, the JSDoc rule "runtime pixels, DOMRects and preview state never enter it"); the sibling DockLayoutAdapter discipline (rejects preview-only fields, recoverable placeholders) from my #13107 review.
• Expected Solution Shape: a neo.harness.dockLayout.v1 JSON-only wrapper over a validated dock-zone document — create (validate → reject forbidden runtime/preview/credential fields → normalize → envelope) + restore (fail-closed validate → normalize) — returning the {x, errors} shape, deep-clone-isolated, app-agnostic (the model is reusable src/dashboard/ now), tested via pure static-method specs. Must NOT hardcode app-specific layout nor let runtime state leak into the persisted form.
• Patch Verdict: Matches. createSavedLayout validates → findForbiddenSavedLayoutKey → normalizeTree → envelope {schema, layoutId, title, dockZone, metadata, [revision]} → re-checks forbidden + findNonJsonValue on the full layout → cloneJson; restoreSavedLayout fail-closes on schema/identity/dockZone/forbidden/non-JSON/validate before normalize + re-validate. The {x:null, errors} fail-closed shape mirrors the existing model. (An initial concern that cloneJson/findNonJsonValue were unused was V-B-A'd false — both are consumed in createSavedLayout's final assembly.)

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13147
• Related Graph Nodes: #13142 (the relocation this builds on), #13107 (DockLayoutAdapter sibling), #13030, #13012

🔬 Depth Floor

Challenge (non-blocking): The forbidden-field rejection is a denylist (forbiddenSavedLayoutKeys), and findNonJsonValue only rejects non-serializable values (functions / symbols / cyclic / non-finite). A secret stored as a plain JSON string under a key not in the denylist — most reachably inside the free-form metadata field (passed through as metadata.metadata, key-unconstrained) — round-trips into the persisted layout. The denylist has token / accessToken / bridgeToken / secret / pat / password (exact-key, case-insensitive), but apiKey / sessionKey / authKey are not in it, so createSavedLayout(doc, {metadata:{apiKey:'sk-live-…'}}) persists the secret. validate() constrains the dockZone document shape, but metadata is deliberately free-form, so the denylist is the only guard there.

Non-blocking (metadata is operator-authored annotation, not an adversarial surface, and the denylist is comprehensive for the runtime/preview leakage it targets). Worth a follow-up: either allowlist the metadata key surface, or document a "metadata is non-secret operator annotation — MUST NOT carry credentials" contract on the helper + ticket. Empirical isolation to make the gap concrete (§5.1): a one-line test asserting createSavedLayout(doc(), {metadata:{apiKey:'sk-live-x'}}) currently returns a layout (not an error) would pin whether this is in-scope-acceptable or worth closing. Happy to file the follow-up or fold it into the storage/UI leaf.

Rhetorical-Drift Audit: The PR description ("JSON-only wrapper creation, forbidden runtime-field rejection, fail-closed restore") and the new JSDoc ("wrap the same committed model … after rejecting runtime-only fields and non-JSON values … deliberately do not choose a storage backend") match the mechanical reality. The only slight overshoot is that "forbidden runtime-field rejection" reads as a broader secrets guarantee than the key-denylist delivers — folded into the challenge above rather than flagged separately. Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: denylist-of-runtime-keys + JSON-only round-trip validation + fail-closed {x:null, errors} + deep-clone-on-output is a clean, reusable "persistable projection of a live model" pattern — structurally excluding runtime/preview/credential state from any committed Neo document destined for storage.

🎯 Close-Target Audit

• Close-targets identified: #13147.
• Confirmed not epic-labeled (enhancement / ai / architecture) — valid leaf close-target.

Findings: Pass.

🪜 Evidence Audit

PR declares Evidence: L2 (focused Playwright unit coverage of pure helper behavior) → L2 required (pure helper-contract ACs). No residuals. The ACs are pure helper-contract, fully covered by the unit specs; the storage backend (which would carry runtime ACs) is explicitly a later leaf. Achieved L2 = required L2, no residuals. Findings: Pass.

🧪 Test-Execution & Location Audit

• Branch checked out locally — git fetch origin pull/13152/head into a --detach worktree; verified rev-parse HEAD == 0ee51e0cc (== headRefOid).
• Canonical Location: test/playwright/unit/dashboard/DockZoneModel.spec.mjs — correct mirror of src/dashboard/ (the location established by the #13142 relocation).
• Ran the related specs:

UNIT_TEST_MODE=true npx playwright test -c test/playwright/playwright.config.unit.mjs \
  test/playwright/unit/dashboard/DockZoneModel.spec.mjs \
  test/playwright/unit/dashboard/DockLayoutAdapter.spec.mjs
→ 44 passed (1.3s)

The 7 new saved layout persistence cases (round-trip, schema-mismatch, malformed identity, invalid dock-zone, forbidden-field, non-JSON, no-caller-mutation) + 37 existing all green. CI also fully green (unit 5m26s, integration-unified 6m46s, CodeQL, lint-pr-body, Analyze). Findings: Tests pass.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: the new methods are framework-internal model helpers consumed by a future sibling leaf — not an external/agent-facing contract surface (no MCP tool / openapi.yaml / config / CLI), so no formal Contract Ledger gate fires (consistent with sibling pure-helper leaves); no OpenAPI surface touched; no skill / workflow-convention / AGENTS*.md surface, so no cross-skill predecessor to fire. (One soft doc note below, not a gate.)

Doc note (non-blocking): the new neo.harness.dockLayout.v1 schema isn't reflected in learn/agentos/HarnessDockZoneModel.md, which documents the sibling dockZone.v1 / dockPreview.v1 contracts. Worth capturing the persisted-form contract there when the storage/UI leaf lands.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — follows the existing DockZoneModel {document, errors} fail-closed pattern, Neo.clone/normalizeTree/validate reuse, and static-method discipline; 8 deducted because a denylist is a less-robust shape than an allowlist for a security-sensitive persisted surface (the metadata gap).
• [CONTENT_COMPLETENESS]: 90 — full @summary JSDoc on every new method + a Fat-Ticket PR body; 10 deducted because the new neo.harness.dockLayout.v1 schema is not yet documented in the HarnessDockZoneModel.md contract doc alongside the existing dockZone.v1 / dockPreview.v1 contracts.
• [EXECUTION_QUALITY]: 90 — 44/44 green at the verified head; cycle-guarded recursion, deep-clone isolation, and no-caller-mutation are all tested; 10 deducted for the denylist value-gap (a sensitive string under a non-denylisted metadata key round-trips) — a real if narrow security edge that is untested and uncovered.
• [PRODUCTIVITY]: 90 — fully delivers #13147's persist-with-fail-closed-restore ACs; only optional polish (allowlist / contract-doc) deferred.
• [IMPACT]: 72 — a reusable persistence projection for the dock-zone model enabling operator layout persistence; below "major subsystem" (one model's helper pair), above "substantive refactor," realized once a storage/UI leaf consumes it.
• [COMPLEXITY]: 45 — moderate: two cycle-guarded recursive validators plus the create/restore envelope logic and 7 new tests, all within one self-contained model file; no cross-substrate integration.
• [EFFORT_PROFILE]: Quick Win — high ROI (unblocks layout persistence) at contained complexity (one model file + its spec, pure helpers).

Cross-family approval (Claude reviewing GPT) — this clears the §6.1 merge gate. I checked out the head (0ee51e0cc), ran the related specs green (44/44), and validated the close-target, evidence, and CI. The denylist-vs-allowlist metadata gap and the contract-doc note are non-blocking follow-ups. Eligible for human merge.

PR Review Follow-Up Summary

Status: Approved

Cycle: Cycle 2 re-review

Opening: My cycle-1 review (corrected) requested changes on two blocking findings — the cloneJson JSON-round-trip anti-pattern and the brittle forbiddenSavedLayoutKeys denylist. The cycle-2 reshape (head 3b4f589e) addresses both: a finite per-level schema allowlist (reject-by-default) and Neo.clone at every clone site. Verified mechanically (45/45 at the head, CI green) and against the prio-0 "does this make sense?" — it now does.

Patch-Blind Premise Snapshot

• Inputs Read Before Patch: my cycle-1 Required Actions; the FULL DockZoneModel.mjs at the head (the allowlist Sets, findUnexpectedKey / findUnexpectedDockZoneKey, every clone() return site, the metadata/blueprint opaque-extension JSDoc); the file's own clone() method.
• Expected Solution Shape: field-safety inverted from a name-denylist to a finite known-schema allowlist (reject-by-default); the clone routed through the canonical Neo.clone the file already uses; free-form metadata/blueprint opaque-by-contract with JSON value-shape enforced.
• Patch Verdict: Matches — the reshape is exactly that shape.

Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Both blocking mechanism flaws are reshaped to the correct shapes; the feature + envelope structure (already sound) are unchanged; no new blocking concern.

Prior Review Anchor

• PR: #13152
• Target Issue: #13147
• Prior Review: my CHANGES_REQUESTED review 4492192546 (2026-06-14T00:06Z)
• Latest Head SHA: 3b4f589e

Delta Scope

• Files changed (delta): src/dashboard/DockZoneModel.mjs (cloneJson deleted; forbiddenSavedLayoutKeys + findForbiddenSavedLayoutKey replaced by the allowlist Sets + findUnexpectedKey / findUnexpectedDockZoneKey; clone() at the return sites) + its spec (updated for the allowlist; 44 → 45 cases).
• PR body / close-target: Resolves #13147, unchanged; not epic-labeled.
• Branch freshness / merge state: MERGEABLE; CI green on 3b4f589e.

Previous Required Actions Audit

• Addressed: "delete cloneJson; clone via the existing clone() / Neo.clone." — cloneJson removed; DockZoneModel.clone(...) at the return sites (:580 create, :646 restore); no JSON.parse/JSON.stringify remains in code.
• Addressed: "invert the denylist → an allowlist of the known schema; metadata opaque-by-contract." — replaced by per-level allowlist Sets (savedLayoutKeys / dockZoneDocumentKeys / dockZoneItemKeys / type-keyed dockZoneNodeKeys / dockZoneEdgeKeys) + findUnexpectedKey (reject-by-default) + recursive findUnexpectedDockZoneKey (document → items → nodes-by-type → edge-zones), applied at create (:567) and restore (:621); metadata/blueprint documented as opaque-JSON-only-no-secrets, findNonJsonValue enforcing value shape. Subsumes #13153.
• Addressed (consequent): scaffolding re-evaluated — findForbiddenSavedLayoutKey removed; findNonJsonValue retained for value-shape (a sensible keys-vs-values division).

Delta Depth Floor

• Delta challenge (non-blocking): the allowlist must track the dock-zone schema as it evolves — a field added to the model but not the allowlist would be false-rejected. This is the correct trade: false-reject is fail-closed (a layout loudly fails to persist) vs the denylist's fail-open (a runtime/secret field silently persists). I verified the Sets match the current grammar (split / tabs / edge-zone node types; the item + document field sets). A one-line comment near the Sets noting they mirror the live schema would help future maintenance, but is not blocking.

N/A Audits — 📑 🔗

N/A: framework-internal model helpers, no external / MCP / config contract surface; no skill / workflow-convention surface.

Test-Execution & Location Audit

• Branch checked out: detached worktree at 3b4f589e (== headRefOid).
• Location: test/playwright/unit/dashboard/DockZoneModel.spec.mjs — correct.
• Ran: DockZoneModel.spec.mjs + DockLayoutAdapter.spec.mjs → 45 passed (the reshape added a case). CI also green (unit 5m30s, integration-unified 6m31s, CodeQL, lint-pr-body, Analyze).
• Findings: pass.

Metrics Delta (revised up from my cycle-1 REQUEST_CHANGES)

• [ARCH_ALIGNMENT]: 45 → 90 — the anti-pattern is gone (canonical Neo.clone) and field-safety is a reject-by-default allowlist; −10 only for the allowlist↔schema maintenance coupling.
• [CONTENT_COMPLETENESS]: 78 → 90 — JSDoc now describes the allowlist + opaque-extension contract accurately; no rationalized anti-pattern.
• [EXECUTION_QUALITY]: 50 → 90 — both load-bearing mechanisms are correct-shaped; 45/45 green at the head + CI green.
• [PRODUCTIVITY]: 70 → 90 — delivers #13147 with the corrected mechanisms.
• [IMPACT]: 72 — unchanged; the feature's value is real.
• [COMPLEXITY]: 45 — unchanged (allowlist Sets + recursive validator ≈ the prior scaffolding's load).
• [EFFORT_PROFILE]: Quick Win — the reshape was small and lands the feature correctly.

Required Actions

No required actions — eligible for human merge.

A2A Hand-Off

Cross-family approval (Claude → GPT) — clears the §6.1 gate over 3b4f589e. Both prior blocking findings reshaped + verified (allowlist reject-by-default + Neo.clone); the lone watch-item (allowlist↔schema coupling) is non-blocking and fail-safe. Eligible for human merge.