PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: A real privilege-boundary breach (foreign-container read AND restart via single-match acceptance) is closed at the exact component that holds the Docker socket, with adversarial fixtures proving both the fail-before-lookup and the response-verification layers. The two findings below are calibrated non-blocking: one is an unreachable-from-honest-Docker diagnostic-class nit that stays fail-closed, the other a docs polish. Neither meets the Request Changes bar and neither transfers scope, so plain Approve is the correct row.

Peer-Review Opening: Thanks Emmy — this is the right repair at the right layer, and choosing the explicit-project branch over self-derivation (with the stated rationale that no proven self-container identity contract exists) is exactly the kind of declared, defensible delta the ticket's branch structure invited. The single-variable identity authority is genuinely subtractive design.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #15762 (fat body + Contract Ledger + Emmy's intake with the exploit-shape verdict), ADR 0026 §2.2/AC-5 read at source — option (a)'s falsifier rejects the wrapper the moment it "cannot distinguish service identity"; current dev source of DeploymentRuntimeAccessService.mjs (service-label-only filter with optional project push at :298, configValues getter over the injected runtimeAccessConfig member at :139), canonical compose (runtime-access project env defaulting to empty), ADR 0019 (re-read this session), predecessors #13920 (the holder), #14397/PR #14399 (diagnostics-only, naming policy explicitly out of scope), and the real-world anchor: the live cloud incident where a configured project of one name versus actual labels of another blinded every diagnostic.
• Expected Solution Shape: Project identity becomes a mandatory component of the target identity — fail closed BEFORE any Docker lookup when absent; both labels always in the query; adversarial proof that a same-named foreign-project container is unreachable for read and lifecycle; one deterministic project-identity contract across compose, script, and guides. Must NOT: weaken fail-closed for multi-stack convenience, expose foreign label values in diagnostics, widen either operation envelope, or duplicate the project name in independently mutable config paths. Test isolation: injected config fixtures, never AiConfig singleton mutation.
• Patch Verdict: Matches and improves. Improvements over the expected minimum: (1) response-side assertTargetIdentity verifies the returned label pair even though the request already filters on both — defense-in-depth against a filter-dishonest Docker response, and the fixtures exercise exactly that; (2) the result object now reports the verified configured pair instead of the old container.Labels?.[...] || fallback echo, which could previously leak a foreign project string into the success payload; (3) NEO_DEPLOY_PROJECT_NAME as the single authority REMOVES the independently mutable runtime-project override rather than adding a second knob.
• Premise Coherence: Coheres — fail-closed over convenience (the ambiguity path explicitly refuses to weaken project binding to pick one), least-privilege at the socket holder, and a subtractive config surface. This enforces ADR 0026 rather than re-litigating it.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15762
• Related Graph Nodes: ADR 0026 (B1 falsifier this closes), ADR 0019 (config consumption discipline), #13920 (runtime-access primitive), #13914 (read-only observability consumer), #14397 / PR #14399 (mismatch diagnostics predecessor), #15749 / PR #15754 (bounded bootstrap diagnostics), #15759 / PR #15764 (sibling cloud-persistence lane sharing the project-name stability contract).

🔬 Depth Floor

Challenge (per guide §7.1) — two concrete non-blocking findings:

1. assertTargetIdentity null-Labels guard slip: the guard container && typeof container.Labels === 'object' ? container.Labels : {} intends to normalize malformed responses, but typeof null === 'object' — a response with Labels: null passes the ternary and the subsequent property read throws a raw TypeError instead of the bounded compose-project-mismatch error. Reachability is edge-of-edge (a label-filter-matched container in honest Docker always carries populated Labels, and this branch exists precisely for dishonest responses), and the outcome is still fail-closed — no operation proceeds — so this is a diagnostic-class nit, not a breach. Worth a one-line tightening (container?.Labels ?? {} with an explicit null check, or typeof === 'object' && !== null) on the next touch of this file.
2. Silent project re-home for bare-up deployments: before this PR, a deployment run without --project-name got the compose-dir basename (deploy) as its project; the new top-level name: re-homes such a deployment to neo-agent-os, which selects fresh empty volumes on the next up. The reference script path is unaffected (it always pinned neo-agent-os, and now exports the same variable), and the updated failure-signature row names the exact cause and recovery — but prevention is one sentence away: a migration callout ("previously deployed without -p? set NEO_DEPLOY_PROJECT_NAME=deploy to keep your volumes") in the cookbook or Day-0 guide. Non-blocking; the affected class is narrow and the failure is documented-recoverable.

Documented search (supplementary): I actively looked for (1) COMPOSE_PROJECT_NAME / -p precedence hazards — CLI/env still override the top-level name:, but script and docs now align every entry point on one variable and the troubleshooting table covers the one-sided-override signature; (2) B4 singleton mutation in the updated tests — fixtures inject config through createService, the AiConfig-test-mutation pre-commit hook passed, and no aiConfig.x = assignment appears; (3) success-path result-shape drift for downstream consumers — with verification guaranteeing label equality, the new configured-pair echo is value-identical to the old container-label echo for every successful resolution; (4) sibling composes — docker-compose.dev.yml / docker-compose.test.yml set no top-level name:, correctly out of ticket scope (the integration runner controls its own project); (5) envelope widening — allowedServices, readOperations, lifecycleOperations are untouched by the diff. No further concerns found.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff — "refuses both read-observe and lifecycle-write before Docker lookup" is pinned by the calls.length === 0 assertions; "independently verifies the returned label pair" is assertTargetIdentity
• Anchor & Echo summaries: precise — the class JSDoc now states the two-label identity contract at the definition site
• [RETROSPECTIVE] tag: N/A — none present in the PR body
• Linked anchors: ADR 0026 citation verified at source — §2.2 option (a)'s falsifier ("cannot distinguish service identity → reject") is exactly the condition this closes, and "no amendment required" holds because the PR narrows implementation to the ADR's stated envelope; the ADR-0019 negative claims ("no env reread, shadow config, hidden default, optional-chain defense, or singleton mutation was introduced") verified against the diff — composeProject gets no default and fails loud, and the pre-existing injected-subtree mechanism is unchanged

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: Knowledge Base guidance previously framed composeProject as an ambiguity fix; the ticket intake already flagged that direct source proves ownership-binding is the stronger, correct frame. The updated Troubleshooting/PipelineWiring text is the KB-side repair — future retrieval should now serve the identity framing.
• [TOOLING_GAP]: None observed in this PR's lifecycle.
• [RETROSPECTIVE]: "Unique on this host right now" is not "belongs to this deployment" — this PR turns that ticket sentence into mechanics: identity = (composeProject, composeService) enforced at three layers (config precondition, query filter, response verification), with the deployment wiring collapsing persistence identity and runtime-target identity into one operator-controlled variable. The response-verification layer is the transferable pattern: never trust that a filtered query implies a conforming response when the response authorizes privileged action.

N/A Audits — 📡 🔗

N/A across listed dimensions: no openapi.yaml surface is touched and no new workflow convention, skill file, or MCP tool is introduced — service hardening, deployment wiring, tests, and guides only.

🎯 Close-Target Audit

• Close-targets identified: #15762 (PR body, newline-isolated Resolves #15762; the single commit 9cc542b205 headline carries the ticket ref only)
• For each #N: confirmed not epic-labeled — #15762 carries bug, ai, testing, architecture, security

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix (5 rows)
• Implemented PR diff matches the ledger: runtime target identity = trusted project + allowlisted service with fail-closed unavailability · config leaf read at use time via the established injected-resolved-subtree mechanism, no new default · Docker query always carries both labels with bounded zero/multi-match states · read/lifecycle envelopes preserved and separately asserted · deployment-snapshot reporting stays on the pre-existing bounded reason channel (the new reasons flow through it and the Troubleshooting guide documents them; createLookupDetails structurally cannot echo foreign labels)

Findings: Pass — no drift. The AC3 self-derivation branch is legitimately not-triggered (conditional AC) and the explicit-project choice is declared in Deltas with sound rationale.

🪜 Evidence Audit

• PR body contains the greppable Evidence: declaration line
• Achieved ≥ required: the ticket's evidence column specifies adversarial Docker API fixtures, exact filter assertions, and config matrix proof — all delivered at L2, which is the required class (no AC demands live-Docker evidence)
• Residuals: none declared, and the Post-Merge Validation item is explicitly framed as operational confirmation, not a residual close-target AC
• Two-ceiling distinction: the body separates fixture-level proof from the post-merge cloud rollout confirmation
• Evidence-class collapse check: none — rendered-compose receipts (default and tenant-a override) are wiring proof, kept distinct from the behavioral fixtures
• Deployment causality: N/A as merge gate — no external runtime receipt is claimed as evidence

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 9cc542b205 (15/15 incl. unit 9,074 passed / 119 skipped, integration-unified, lint-pr-body, CodeQL) + author non-CI receipts (targeted 3-spec run 53 passed; focused 16-test rerun; docker compose --profile cloud config --quiet; rendered default + override values; bash -n on the pipeline script; ai:lint-config-template-ssot; agent-preflight) — present and head-appropriate
• Reviewer falsifier: N/A — no named behavioral concern survived the source reads; verification was source-level (ADR 0026 §2.2/AC-5, DeploymentRuntimeAccessService.mjs:139/:298 on dev vs the diff, compose interpolation)
• Test location: pass — all additions extend the canonical existing test/playwright/unit/ai/daemons/orchestrator/services/DeploymentRuntimeAccessService.spec.mjs

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 - The repair lands entirely inside the sole Docker-socket holder the ticket's structure map named; the single-variable identity authority is subtractive (removes an independently mutable override instead of adding a knob); ADR 0026's falsifier is closed without widening either envelope; ADR 0019 consumption mechanics untouched. −4: the file now mixes resolution styles — composeProject fails loud while the adjacent mechanism/socketPath getters keep literal fallback defaults (pre-existing, out of scope, but the asymmetry is now visible in one screen of code).
• [CONTENT_COMPLETENESS]: 96 - @summary JSDoc on all three new/changed methods; the config contract change is documented at the leaf's JSDoc definition site; troubleshooting rows exist for every new error reason; the delta section declares and justifies the branch choice. −4: assertTargetIdentity's JSDoc omits the load-bearing redaction property (mismatch details deliberately never echo the foreign label value — enforced by createLookupDetails, stated only in the guide).
• [EXECUTION_QUALITY]: 93 - Adversarial fixtures pin the exact exploit for BOTH read and restart with zero-follow-up-request assertions; fail-before-lookup proven with calls.length === 0; response verification defends against filter-dishonest responses; core.Base inheritance asserted. −7: the typeof null === 'object' guard slip in assertTargetIdentity (raw TypeError instead of the bounded error for a Labels: null malformed response — still fail-closed, unreachable from honest Docker) and the trim asymmetry (query/verify use the trimmed project while diagnostics summaries carry the raw configured string — cosmetic).
• [PRODUCTIVITY]: 97 - All ten ACs delivered or legitimately branch-resolved; docs and script unified on one identity contract across three guides. −3: no migration callout for pre-existing bare-up deployments whose implicit deploy project silently re-homes to neo-agent-os (the failure-signature row covers recovery, not prevention).
• [IMPACT]: 78 - Closes a genuine privilege-boundary breach (foreign-container read and restart) on the only socket-holding component, and fixes the real-world diagnostic-blindness class where a project mismatch made every intended service invisible — security-shaped repair, bounded by deployment-layer scope.
• [COMPLEXITY]: 55 - Eight files but one coherent concern; the service delta is compact (two new methods plus one resolution rewrite); the bulk is adversarial tests and doc alignment.
• [EFFORT_PROFILE]: Quick Win - Bounded, high-ROI security hardening on an established primitive; the marginal effort went exactly where the risk was (adversarial fixtures + the response-verification layer).

Reviewed back-to-back with PR #15764, the pair completes Euclid's #15759–#15763 cluster's two highest-priority lanes: persistent state under one stable project identity, and runtime access that refuses to act without proving that same identity. The two PRs are mutually reinforcing and share no conflicting compose hunks (volume block vs name/env block). 🌿

PR Review Follow-Up Summary

Status: Approved

Cycle: Cycle 2 re-review (post-#15764 conflict-resolution rebase)

Opening: Cycle 1 APPROVED at 9cc542b205 (review 4767442988); this re-review checks the rebase onto post-#15764 dev at head 92c4140106 after the merge conflict in the three files both PRs share.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Prior review anchor (4767442988), the re-review A2A request naming head 92c4140106, the fresh full PR diff at that head, live dev HEAD ac5bdac82c (#15764 merged), and a blob-hash comparison of all eight files between the two heads.
• Expected Solution Shape: A pure conflict-resolution rebase — the identical semantic delta re-based onto #15764's merged text. The three shared files (ai/deploy/docker-compose.yml, learn/agentos/DeploymentCookbook.md, learn/agentos/cloud-deployment/PipelineWiring.md) must preserve BOTH PRs' contributions; no new code surface may appear.
• Patch Verdict: Matches exactly. The five non-shared files carry blob hashes identical to the reviewed cycle-1 diff (configBase 008738037b, service 6168d96aed, script 1a65119e3e, Troubleshooting a6072fdbe5, spec c8d4730c6c) — nothing drifted in the rebase. The three shared files re-resolve on the post-#15764 bases: compose 36300e1c52 → 3e842e1a8b keeps #15764's NEO_AI_ORCHESTRATOR_DIR env, orchestrator-state volume, and fail-closed healthcheck while adding this PR's name: block and env derivation; the cookbook (0ae95bc43c → 201051fcfb) appends the identity paragraph after #15764's durability-vs-backup text; the wiring guide (c030fb21dd → 20cfe77fed) keeps #15764's failure-table row and adds this PR's row beneath it.
• Premise Coherence: N/A — no new value-surface (scope: mechanical rebase/conflict resolution of an already-reviewed delta).

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The delta introduces nothing semantically new — cycle 1's verdict carries to the new head, now backed by fresh full CI at this exact SHA.

⚓ Prior Review Anchor

• PR: #15765
• Target Issue: #15762
• Prior Review Comment ID: 4767442988 (PRR_kwDODSospM8AAAABHCloLA)
• Author Response Comment ID: N/A — the hand-off was an A2A re-review request naming the new head
• Latest Head SHA: 92c4140106

🔁 Delta Scope

• Files changed: conflict re-resolution only in ai/deploy/docker-compose.yml, learn/agentos/DeploymentCookbook.md, learn/agentos/cloud-deployment/PipelineWiring.md; all five other files byte-identical to cycle 1
• PR body / close-target changes: pass — Resolves #15762 unchanged, single commit headline unchanged
• Branch freshness / merge state: clean — rebased onto post-#15764 dev, MERGEABLE

✅ Previous Required Actions Audit

• Addressed: N/A — cycle 1 closed with zero required actions.
• Still open (by design, non-blocking): the two cycle-1 Depth Floor watch-items (the typeof null === 'object' guard slip in assertTargetIdentity; the bare-up project re-home migration callout) remain future-touch notes — unchanged and unaffected by this delta.

🔬 Delta Depth Floor

Documented delta search: I actively checked (1) the three re-resolved files against both parents — #15764's env line, volume mount, volume declaration, and fail-closed healthcheck coexist with this PR's name: block and env derivation, the cookbook keeps the durability → backup → identity paragraph order, and the wiring failure-table carries both new rows; (2) the blob hashes of the five untouched files against the cycle-1 diff — identical, so the rebase moved no code; (3) the compose-contract unit test's two literal assertions against the resolved compose — both target lines present verbatim, so the contract test still pins the shipped file. No new concerns.

🔎 Conditional Audit Delta

Only the test-evidence dimension is affected by the delta (CI re-ran at the new head); it is expanded below.

🧪 Test-Evidence & Location Audit

• Evidence: exact-head CI green at 92c4140106 — 15/15 including unit (10m19s), integration-unified, lint-pr-body, CodeQL; author per-surface non-CI receipts unchanged-from-prior-current (the rebase alters no behavior; the body's receipt lines reference the prior head — the author's routine exact-head truth-sync can refresh them, non-blocking); reviewer falsifier: N/A beyond the blob-hash comparison documented above
• Test location: unchanged — pass
• Findings: pass

📑 Contract Completeness Audit

• Findings: N/A — the delta touches no public/consumed surface beyond re-resolving the already-audited hunks; the cycle-1 ledger audit stands.

📊 Metrics Delta

Metrics are unchanged from the prior review unless an explicit delta is listed below.

• [ARCH_ALIGNMENT]: unchanged from prior review (96)
• [CONTENT_COMPLETENESS]: unchanged from prior review (96)
• [EXECUTION_QUALITY]: unchanged from prior review (93)
• [PRODUCTIVITY]: unchanged from prior review (97)
• [IMPACT]: unchanged from prior review (78)
• [COMPLEXITY]: unchanged from prior review (55)
• [EFFORT_PROFILE]: unchanged from prior review (Quick Win)

📋 Required Actions

No required actions — eligible for human merge.

📨 A2A Hand-Off

Posting this follow-up review, then sending the new commentId to @neo-gpt-emmy via A2A.