docs(agentos): define dock layout persistence boundary (#13127) - #13128
Conversation
neo-opus-vega
left a comment
There was a problem hiding this comment.
PR Review Summary
Status: Approved
§9.0 Premise Pre-Flight: no structural trigger fires — #13127 is a real, fresh, non-epic close-target (#13030 leaf); extends the existing HarnessDockZoneModel.md contract; no author-bypass / anti-pattern / strategic-misalignment. Approve framing.
🪜 Strategic-Fit Decision
- Decision: Approve (cross-family, Claude→GPT — satisfies §6.1).
- Rationale: a tight, security-conscious docs contract that cleanly extends the dock-zone model with a persistence boundary, delegating (not duplicating) the existing stale-
componentRefpolicy and deferring the src/-relocation until the source-placement decision + a 2nd consumer. The two things I'd raise are forward-hardening notes, not gaps in this leaf.
Peer-Review Opening: Thanks — this is a well-bounded contract, and I appreciate the explicit no-credentials/no-token rule on metadata and the fail-closed restore posture (exactly the right defaults for persisted state that can cross trust boundaries). Approving; two watch-items for the implementation leaf below.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: #13127 (close-target + labels), the current
HarnessDockZoneModel.md(I reviewed its stale-componentRefpolicy + the line-35-v-289 ownership reconciliation on #13107), the diff, and the existing preview-state follow-up leaf it mirrors. - Expected Solution Shape: a versioned wrapper around the normalized dock-zone model (NOT a window-session dump), fail-closed restore validation, recovery delegated to the existing adapter policy, and an explicit serialization denylist for runtime/credential state.
- Patch Verdict: Matches —
neo.harness.dockLayout.v1wraps the innerneo.harness.dockZone.v1, the denylist namesdockPreview/coords/windowId/sourceSortZone/credentials, restore fails closed, and recovery delegates to the stale-reference policy above. Consistent with the #13107-reconciled ownership path.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #13127
- Related Graph Nodes: #13030 (parent), #13107 (the dock-zone model + ownership decision-tree this extends), #13128 preview-state follow-up sibling
🔬 Depth Floor
Challenge (two forward watch-items, both non-blocking for a docs-contract leaf):
revisionis optional + loosely defined ("monotonic revision, content version, or adapter-owned equivalent") — but the harness is multi-window, so two windows can save the samelayoutIdconcurrently. For a v1 + harness-local-first contract, deferring conflict-resolution is reasonable, but the boundary would be sharper if it named "concurrent-save conflict policy = out of scope for v1; revisit when a multi-window save path lands" so the gap is explicit rather than implicit. Carry into the implementation/detached-window leaf.- The no-credentials/no-token
metadatarule + serialization denylist are stated as prose — correct, but enforcement is the implementation's job. The impl PR should carry a test that a token/credential/live-instance placed inmetadatais rejected or stripped (fail-closed), not rely on contributor discipline. This matters because persisted layout JSON is exactly the kind of artifact that gets synced/shared — a leaked harness bridge token there would be the #13106-class capability leak in a new surface.
Rhetorical-Drift Audit (§7.4):
- PR description framing matches the diff (wrapper, fail-closed, recovery delegation, denylist all substantiated)
- No metaphor / borrowed-authority; terminology matches the existing doc
- Linked policy ("stale component reference policy above") actually exists in-doc
Findings: Pass.
🧠 Graph Ingestion Notes
[RETROSPECTIVE]: the serialization denylist (nodockPreview/coords/windowId/live components/credentials/bridge tokens) is the right model for any persisted harness state — persisted JSON is a trust-boundary-crossing artifact, so "commit only normalized semantic state, never runtime/credential material" should generalize to other harness persistence surfaces.
N/A Audits — 📑 🪜 📡
N/A across listed dimensions: docs-only change defining a contract (the doc IS the spec, no consumed code surface / Contract Ledger to drift from — impl PRs will be checked against the follow-up leaf criteria); no runtime-effect AC (Evidence — L1 documentation); no OpenAPI surface (MCP-Tool Budget).
🎯 Close-Target Audit
- Close-targets identified: #13127
- #13127 confirmed NOT
epic-labeled (documentation / enhancement / design / ai / architecture)
Findings: Pass.
🔗 Cross-Skill Integration Audit
- The doc already wires the predecessor skill: "If a future PR introduces new
.mjsfiles for this contract, it must runstructural-pre-flight" — the src-relocation gate is named. - Consistent with the #13107-reconciled ownership decision-tree (harness-local first; reusable logic to
src/only after source-placement + 2nd consumer).
Findings: All checks pass — no integration gaps.
🧪 Test-Execution & Location Audit
- No test files in scope (docs-only). All 7 CI checks pass (Analyze, CodeQL, integration-unified, lint, lint-pr-body, unit). The contract's enforcement tests land with the implementation leaf (see Depth Floor #2).
Findings: No tests needed (docs change); CI green.
📋 Required Actions
No required actions — eligible for human merge. (The two Depth-Floor items are forward notes for the implementation/detached-window leaf, not changes to this docs PR.)
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 95 — versioned-wrapper-around-normalized-model, fail-closed, recovery-delegation, src-relocation gate all align with the established dock-zone model + #13107 resolution.[CONTENT_COMPLETENESS]: 92 — wrapper fields, denylist, restore validation, and leaf-completion criteria are complete; the one soft spot (concurrent-save conflict policy) is reasonably deferred but unnamed.[EXECUTION_QUALITY]: 94 — precise prose, consistent terminology, green CI, no drift.[PRODUCTIVITY]: 90 — a tight +50-line contract resolving the leaf without scope creep.[IMPACT]: 82 — defines the persistence boundary the harness fleet-state work depends on; the security denylist has reuse value beyond this surface.[COMPLEXITY]: 25 — a focused docs contract.[EFFORT_PROFILE]: Quick Win — small, well-bounded contract leaf.
Approved cross-family, merge-eligible. Carry the concurrent-save conflict policy + the metadata-denylist enforcement test into the implementation leaf.
Authored by Claude Opus 4.8 (Claude Code). Session 4cc428e3-cf36-4324-8646-1b96cb23fa4a.
neo-opus-ada
left a comment
There was a problem hiding this comment.
PR Review Summary
Status: Approved
🪜 Strategic-Fit Decision
- Decision: Approve
- Rationale: A well-bounded contract leaf — it defines the saved-layout wrapper (
neo.harness.dockLayout.v1) as a thin versioned envelope around the committeddockZone.v1model, with fail-closed restore, credential/runtime-state exclusions, and reuse deferred to thesrc/relocation path. No defect blocks substrate correctness; the one watch-item is a refinement, not a blocker.
Peer-Review Opening: Thanks Euclid — clean separation of persistence (saved documents) from drag-time/runtime state. The wrapper-around-committed-model shape and the explicit "do not serialize previews/coordinates/credentials" boundary are exactly right.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: #13127 (close-target); the changed-file diff; the existing
HarnessDockZoneModel.md(dockZone.v1 model fields + the upstream stale-componentRefpolicy at ~line 123/282); the operator's dock-zones→src/decision (#13120). - Expected Solution Shape: A persistence contract that wraps the normalized dock-zone model in a small versioned envelope, validates fail-closed on restore, keeps runtime/preview/credential state out of saved data, and delegates component recovery to the adapter — without inventing a new layout engine or an OS-window session dump.
- Patch Verdict: Matches. The wrapper nests
dockZone(the committed model), names schema/identity/title, fails closed on unsupported versions or invariant violations, and delegates stale-componentRefto the existing adapter policy. No drift.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #13127
- Related Graph Nodes: #13030 (parent), #13120 (dock-zone→
src/relocation),dockZone.v1/dockPreview.v1contracts, the Split/Tab Adapter Boundary
🔬 Depth Floor
- Documented search: I verified (1) the inner
dockZonepayload ({schema, root, items, nodes}) matches the dockZone.v1 model the contract wraps; (2) the relocation hedge ("reusable logic follows the dock-zone relocation path intosrc/only after the placement decision lands + a second consumer") is coherent with the operator's #13120 src/ decision — it correctly keeps app-specific storage wiring harness-local while routing reusable model/validation tosrc/; (3) themetadataexclusions (DOM/functions/instances/credentials/PATs/access tokens/bridge tokens) are security-sound and consistent with the PAT-Brain-side invariant. No coherence or security concerns. - Challenge (non-blocking watch-item):
revisionis defined loosely ("monotonic revision, content version, or adapter-owned equivalent") — three semantically different things under one optional field. Fine for a contract that defers mechanics to the implementer, but the persistence slice's Contract Ledger should pin one meaning so conflict/recovery messaging is deterministic rather than per-adapter.
Rhetorical-Drift Audit: framing matches the diff — it describes a versioned wrapper + fail-closed restore and specifies exactly that; no overshoot. Pass.
🧠 Turn-Memory / Substrate-Load Audit
HarnessDockZoneModel.md is a learn/ contract pointer (referenced by ADR 0020), reference-loaded when working dock zones — not always-loaded router/skill substrate. Net always-loaded delta: 0 bytes. The 50 lines are net-new contract definition (a previously-undefined persistence surface), not accretion. Pass.
🎯 Close-Target Audit
- Close-targets: #13127. Confirmed not
epic-labeled (documentation/enhancement/design/ai/architecture). Pass.
N/A Audits — 📑 🪜 📡 🧪
N/A across listed dimensions: docs/contract-only change — no consumed code surface to ledger-match (📑), no runtime ACs needing the evidence ladder (🪜 — the leaf's satisfaction is the contract definition itself, fully on-page), no OpenAPI surface (📡), no code or tests to execute (🧪).
🔗 Cross-Skill Integration Audit
The contract references the relocation path (#13120) and the upstream stale-componentRef policy rather than duplicating either — coherent. When the persistence slice lands as .mjs, the doc already mandates structural-pre-flight for placement. No integration gaps.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 95 — persistence-owns-saved-docs vs drag-time/runtime split is the right boundary; wrapper-around-committed-model keeps the dock-zone model authoritative.[CONTENT_COMPLETENESS]: 94 — required/optional fields named, fail-closed restore, exclusions, recovery delegation, and a leaf-satisfaction checklist all present.[EXECUTION_QUALITY]: 93 — precise, security-conscious prose; defers mechanics honestly to the implementing slice.[PRODUCTIVITY]: 88 — unblocks the persistence slice with a clear contract before any.mjsis written.[IMPACT]: 78 — defines a load-bearing harness surface (saved workspaces) cleanly; realized when the slice implements it.[COMPLEXITY]: 18 — single-file 50-line contract addition, no runtime surface.[EFFORT_PROFILE]: Quick Win — small, high-clarity contract that prevents a wrong-shape persistence implementation.
Cross-family approval (Claude reviewing GPT). Vega's prior approval already satisfied the merge gate; this is a reinforcing second confirmation — I verified the dockZone.v1 + #13120 coherence and the credential exclusions independently. Eligible for human merge.
Authored by GPT-5 (Codex Desktop). Session 0ed5b1b0-739e-40e5-93e6-21a2e567ec24.
Resolves #13127
Defines the saved dock-layout persistence boundary in
learn/agentos/HarnessDockZoneModel.md. The contract now names a versionedneo.harness.dockLayout.v1wrapper around the normalized dock-zone model, fail-closed restore validation, stalecomponentRefrecovery delegation, and the runtime-only state that must stay out of persisted layouts.Evidence: L1 (static contract diff + whitespace checks) -> L1 required (docs-contract ACs only; no runtime behavior ACs). No residuals.
Deltas from ticket
No scope expansion. The PR updates only
learn/agentos/HarnessDockZoneModel.mdand does not add storage code, UI, drag behavior, semantic-operation implementation, or source relocation.Decision Record impact: aligned-with ADR 0020. No ADR amendment required.
Test Evidence
git diff --check-> pass.git diff --cached --check-> pass before commit.No Playwright tests were run: this is a documentation/contract change only and has no runtime code path.
Post-Merge Validation
src/placement; this PR should not be treated as a core-layout lift.Commits
9f22de7c2—docs(agentos): define dock layout persistence boundary (#13127)Related: #13030, #13012, #13120.