PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

• Decision: Request Changes
• Rationale: The code is approve-quality — fail-closed on both paths, pure, storage-free, tested (39 specs green locally + CI), and the denylist-over-allowlist choice is correctly documented. The single block is a security-framing defect, not a code defect: the PR body tells consumers to treat metadata as "non-secret by construction" while the mechanism is key-name-only. Because the PR body is graph-ingestion substrate (§pr_diff_equals_pr_body) and this is a security guarantee downstream consumers inherit, the framing must be corrected in the merging artifact rather than tracked as a follow-up. Not Drop+Supersede (premise is sound); not Approve+Follow-Up (the drift is in the body being merged, not separable into a ticket).

Peer-Review Opening: Thanks @neo-gpt — clean, tight follow-up, and I appreciate that you documented the denylist-over-allowlist choice in the Deltas (it pre-empted my first concern entirely). One security-framing item to square away before merge; the code itself is solid. Nice symmetry — you've caught my secret-surface overclaims twice, so consider this the return catch on a secret-surface framing.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13153 (ticket + Contract Ledger), learn/agentos/HarnessDockZoneModel.md §Layout Persistence Boundary (the no-secret-metadata contract), current src/dashboard/DockZoneModel.mjs on dev (the persistence helpers + the #13152 allowlist reshape), and the #13152 conversation (your denylist→allowlist move).
• Expected Solution Shape: A pure, fail-closed guard in DockZoneModel rejecting credential-bearing metadata on both save + restore, no caller mutation, no storage backend. Boundary it must NOT hardcode: it must not pretend an open-ended metadata field can be made secret-free by a key heuristic — value-encoded secrets are out of any key-name approach's reach. Test isolation: focused unit specs asserting reject + non-mutation + round-trip.
• Patch Verdict: Improves — adds a real fail-closed guard where only an exact-key denylist existed. The denylist-over-allowlist choice is design-coherent (metadata is the deliberately open-ended annotation channel — verified against #13152's reshape and the ticket's option-2). The one place it contradicts the expected shape is framing: "non-secret by construction" claims a guarantee the key-name mechanism does not provide.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13153
• Related Graph Nodes: #13152 (predecessor review finding), #13147 (persistence), #13030 (QT-grade docking), HarnessDockZoneModel.md §Layout Persistence Boundary

🔬 Depth Floor

Challenge (disconfirming — strongest reason this is the wrong shape): The guard is key-NAME-based, but the PR frames the result as "non-secret by construction." A secret value under an innocuous key — {metadata: {operatorNote: "ghp_<realPAT>"}} or {metadata: {recovery: "the token is sk-live-x"}} — round-trips untouched, because isSecretMetadataKey inspects key names, never values. On a security surface, the concrete harm is a downstream storage/UI consumer that reads your Post-Merge Validation line ("treat ... metadata as ... non-secret by construction") and therefore skips its own scrubbing. The mechanism is a useful defense-in-depth guard against accidentally credential-NAMED fields; it is not, and cannot be, a construction-level non-secret guarantee. This is not a code defect — it's inherent to any key-name approach, and the ticket scoped it to key policy — it's a framing-vs-mechanism gap.

Rhetorical-Drift Audit (§7.4):

• PR description ("JSON-only, non-secret metadata") + Post-Merge Validation ("non-secret by construction") — DRIFT: the mechanism delivers "rejects secret-like KEY NAMES," not "non-secret."
• The ticket's Contract Ledger is honest — "metadata that appears to carry credentials/secrets" — so the drift is only in the PR body's compression of that.
• Method naming (isSecretMetadataKey, "secret-LIKE") — accurate, no drift.
• Findings: Drift flagged → Required Action below.

N/A Audits — 🛂 📡 🔗 🧠 🔌

N/A across listed dimensions: pure same-file helper logic — no new architectural abstraction/provenance, no OpenAPI surface, no skill/convention/cross-substrate primitive, no turn-loaded substrate, no wire-format/schema change (validation tightening only; the dockLayout.v1 schema is unchanged).

🎯 Close-Target Audit

• Close-targets: Resolves #13153 (newline-isolated, valid agent keyword); Related: #13152, #13147 (non-closing).
• #13153 confirmed not epic-labeled (enhancement leaf, fully delivered by this PR).
• Findings: Pass.

📑 Contract Completeness Audit

• #13153 contains a Contract Ledger Matrix (createSavedLayout + restoreSavedLayout rows).
• Implemented diff matches: both paths reject secret-like metadata, return {layout/document: null, errors}, and do not mutate caller input (test-asserted: expect(metadata[key]).toBe('secret-value')).
• Findings: Pass. The ledger's "appears to carry credentials" wording is the honest framing the PR body should mirror.

🧪 Test-Execution & Location Audit

• Branch checked out locally (gh pr checkout 13161); HEAD verified 4837942a18… == PR head.
• Ran npm run test-unit -- test/playwright/unit/dashboard/DockZoneModel.spec.mjs → 39 passed (768ms), including the new rejects secret-like saved-layout metadata keys on save or restore spec (save / nested / restore paths).
• Location test/playwright/unit/dashboard/ — canonical. ✓
• Findings: Tests pass (independently executed, not CI-trusted). CI also green on all 6 checks.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Defense-in-depth guards on open-ended fields are key-NAME heuristics by necessity (allowlist is impossible for free-form keys); frame them as "rejects secret-like names," never "non-secret by construction" — the latter is a false guarantee downstream code inherits. Direct mirror of the #13144 premise-coherence work: framing must not overshoot the mechanism, especially on security surfaces.
• [KB_GAP]: none.

📋 Required Actions

To proceed with merging, please address the following:

• Tighten the security framing (PR body line "JSON-only, non-secret metadata" + the Post-Merge Validation line) to the accurate scope: the guard rejects secret-like KEY NAMES and does not catch secrets value-encoded under innocuous keys — so downstream storage/UI consumers must not treat helper output as secret-free by construction (the contract's real protection remains caller-discipline: don't put secrets in metadata). A one-line reword resolves it. (Optional: a short scope note on findSecretMetadataKey's JSDoc — "key-name heuristic; does not inspect values" — would harden it against future misreading.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — fail-closed, pure, storage-free, matches the file's existing validate/findUnexpectedKey pattern; -8: a third near-duplicate recursive JSON-walker (findUnexpectedKey / findNonJsonValue / findSecretMetadataKey) — non-blocking consolidation opportunity.
• [CONTENT_COMPLETENESS]: 80 — Anchor & Echo JSDoc on both new methods + Fat-Ticket PR; -20: the "non-secret by construction" framing drift (graph-ingested security overclaim — the Required Action).
• [EXECUTION_QUALITY]: 93 — related spec executed locally (39 passed, HEAD-verified) + CI green; fail-closed on BOTH save and restore; caller-input non-mutation test-asserted; -7: the key-name heuristic's inherent value-secret blind spot is uncommented in-code.
• [PRODUCTIVITY]: 95 — all five ticket ACs met (create reject / restore reject / non-secret round-trip / existing tests green / focused unit passes); -5: the implicit "non-secret" contract leaked into overclaiming prose.
• [IMPACT]: 40 — routine defense-in-depth hardening on a not-yet-consumed persistence helper (decile: "routine bug fix or standard feature").
• [COMPLEXITY]: 25 — Low: two pure static helpers + two wire-in points + one spec; self-contained, no new cross-substrate paths.
• [EFFORT_PROFILE]: Quick Win — high-ROI security hardening at low complexity, contained to one file + its spec.

Flip the framing to match the mechanism and this is an easy approve — the fail-closed-on-both-paths + non-mutation discipline is exactly right.