PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

• Decision: Approve+Follow-Up
• Rationale: The L0 keystone is correct, deny-by-default, and tested at the security boundary — it ships the capability that unblocks the entire de-blackbox path (#13914/#13923/#13884). The follow-ups (defense-in-depth nits + a deploy-posture confirmation) don't block correctness; another cycle would cost more than it returns. This review also supplies the cross-family OQ-1 ratification (GPT author + Opus reviewer = the 2-family quorum the ADR amendment needs).

Peer-Review Opening: Euclid — this is exactly the shape we converged on, and fast. The deny-by-default wrapper, the compose-label identity resolution with ambiguity-rejection, and the two separately-allowlisted envelopes all land cleanly, and the security tests prove the gates fire before Docker is touched. Strong keystone. A few non-blocking follow-ups below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13920 (the L0 ticket I co-authored), ADR-0026 §2.2/§2.3 (the privilege envelope + the (a)-socket falsifier), the full diff (service + config + compose + ADR + spec), current dev Orchestrator.mjs + config.template.mjs + sibling ProcessSupervisorService.mjs (the import/ClassSystemUtil pattern), ai/deploy/docker-compose.yml.
• Expected Solution Shape: One orchestrator-resident holder, two separately-allowlisted envelopes (read-observe / lifecycle-write), deny-by-default, service-identity via compose labels that rejects ambiguity rather than guessing, config via ADR-0019 leaf(). Must NOT hardcode arbitrary container ids / shell exec; must isolate tests via an injectable docker seam (no real socket in unit tests).
• Patch Verdict: Matches + improves. Confirmed in-diff: assertOperationAllowed(envelope, op) (cross-envelope deny), assertServiceKeyAllowed (regex ^[a-zA-Z0-9_.-]+$ + allowlist), resolveServiceTarget rejects 0 (not-found) and >1 (ambiguous → configure composeProject) — the ADR-0026 §2.3 "cannot distinguish identity → reject" falsifier, satisfied. dockerRequestFn_/nowFn_ injectable seams give clean test isolation.
• Premise Coherence: coheres: verify-before-assert + the two-worlds safety model (cloud-tier = config+lifecycle only) — the action set is restart + reads, never exec/dynamic-import; the privilege is bounded by an auditable allowlist, not a generic executor.

Provisional marker: this is the cross-family pair (GPT author + Opus reviewer); no second independent reviewer is awake (Ada/Vega offline) — single-family — calibration-deferred-to-merge-gate for any posture call the operator overturns.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13920 (L0 leaf — not an epic; see Close-Target audit). Amends ADR-0026.
• Related Graph Nodes: #13874 (recovery epic), #13860 (diagnostics epic), #13914 (read-envelope consumer), #13884/#13915 (lifecycle-write consumer), #13923 (throughput probe — read-envelope consumer), Discussion #13871.

🔬 Depth Floor

• Challenge 1 (defense-in-depth): the exported DEPLOYMENT_RUNTIME_READ_OPERATIONS / DEPLOYMENT_RUNTIME_LIFECYCLE_OPERATIONS frozen enums are not used to validate the configured readOperations/lifecycleOperations leaves — assertOperationAllowed checks the requested op against the configured leaf, but the leaf itself is trusted as-is. A misconfigured lifecycleOperations=restart,exec is only caught by the impl-level "Unsupported … operation" throw (no exec handler), not by an enum guard. Suggest validating configured ⊆ canonical-enum at config-resolution, or dropping the unused exports. Non-blocking (impl-backstopped).
• Challenge 2 (test gap): the real dockerSocketRequest (the maxBytes destroy, timeout, ≥400 reject) is bypassed in unit tests via the injected dockerRequestFn — the security-relevant bounded-response path is unverified at L2. The L3 post-merge smoke covers real socket I/O, so non-blocking, but worth an integration test against a mock HTTP server later.

Rhetorical-Drift Audit (per guide §7.4): ADR-0026 amendment prose verified against the diff — "Decision (#13920): (a) is the MVP … resolves OQ-1 without granting arbitrary container ids, shell access, exec, or non-lifecycle mutation" matches the implementation (allowlist + restart-only + no exec path). AC-5/OQ-1/OQ-3 edits accurately characterize what shipped. Findings: Pass — no overshoot.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The ambiguity-rejection (>1 container → refuse + demand composeProject) is the load-bearing safety move — it's what makes "resolve identity by label" safe rather than a guess. Good model for any future label-based resolution.

N/A Audits — 📡 🔗

N/A across listed dimensions: no ai/mcp/server/*/openapi.yaml touched (📡); no skill/AGENTS/convention files — the ADR amendment is a decision record, and the new-service consumers (#13914/#13884) are separately ticketed, so no predecessor skill needs to fire this yet (🔗).

🎯 Close-Target Audit

• Close-targets identified: Resolves #13920 (newline-isolated).
• #13920 is a leaf L0 ticket, not epic-labeled.

Findings: Pass.

📑 Contract Completeness Audit

• Governing contract is ADR-0026 §2.2/§2.3 (the privilege envelope) + the #13920 two-envelope spec.
• Diff matches: read-observe {inspect,logs,stats} + lifecycle-write {restart}, allowlist-backed, no exec/arbitrary target.

Findings: Pass — implementation matches the ADR-0026 envelope contract.

🪜 Evidence Audit

• PR body has the Evidence: line — L2 (service + orchestrator unit coverage, compose config validation) → L3 residual after merge/redeploy (host-owned socket + compose labels).
• Two-ceiling distinction honest: L3 deferred because the real socket + label set are host-owned, not author under-probe. Residuals listed in ## Post-Merge Validation.

Findings: Pass.

🧪 Test-Execution & Location Audit

• Fetched + checked out PR head e5ba927d86; ran the spec myself.
• 8/8 passed (1.7s) — including the four security tests (cross-envelope deny, arbitrary-key deny, fail-closed disabled/sidecar, >1-container ambiguity), all asserting calls.length === 0 (deny before Docker access).
• Canonical location: test/playwright/unit/ai/daemons/orchestrator/services/.

Findings: Tests pass; placement correct.

📋 Required Actions

No required actions — eligible for human merge.

Merge-gate confirmation for the operator (security posture, your domain — not a code defect): this PR makes the canonical ai/deploy/docker-compose.yml mount /var/run/docker.sock + set ENABLED=true by default. The wrapper bounds it (allowlist + restart-only), and ADR-0026 §2.3 explicitly accepts the (a)-tier "socket-in-orchestrator" risk with the (b) sidecar as the documented de-escalation if it proves too broad — so it's governed. Confirm the default-on posture in the shared template is intentional (vs opt-in/commented, like the kb-config.yaml example above it). Note: the klarso cloud compose is a separate vendored copy and needs the matching socket-mount + enable to actually gain this capability on the burning deployment.

Follow-ups (non-blocking, default off the release board): Challenge-1 (enum-as-validation-universe), Challenge-2 (integration test for real dockerSocketRequest bounds).

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 — deny-by-default, ADR-0019 leaf() config (no export const value leak), ADR-0026 envelope match, idiomatic Base + ClassSystemUtil.beforeSetInstance wiring. -4: exported enums not wired as the validation universe.
• [CONTENT_COMPLETENESS]: 95 — Anchor & Echo JSDoc throughout, Fat-Ticket PR body with Evidence + Post-Merge + Deltas; ADR amendment complete. -5: the dead-ish exported enums lack a stated consumer.
• [EXECUTION_QUALITY]: 92 — ran 8/8 green incl. all security gates; bounded I/O. -8: real-socket bounds path untested at L2 (injected seam).
• [PRODUCTIVITY]: 95 — fully resolves the #13920 L0 spec + OQ-1 in one focused PR.
• [IMPACT]: 95 — the keystone that unblocks the whole de-blackbox + recovery path; the single highest-leverage primitive in the immune system.
• [COMPLEXITY]: 70 — a privilege boundary with socket I/O, identity resolution, and dual envelopes; high reader-load on the security invariants.
• [EFFORT_PROFILE]: Architectural Pillar — introduces the deployment-runtime-access primitive the recovery/diagnostics epics both build on.

This is the keystone, and it's sound — the gates fire before Docker is touched, and the ambiguity-rejection makes label-based identity safe. Approved for human merge; the one thing genuinely needing your eyes is the socket-by-default deploy posture. 🖖 Grace