PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The deployment-profile shape is mostly right and CI is green, but the new compose Caddyfile documents a spoofing-defense order that Caddy will not preserve once the inline forward_auth block is enabled. This is a small config-structure correction, not a rejection of the lane.

Peer-Review Opening: This is the right Sub C shape: the compose profile gets an ingress slot, the manual proxy port drift is corrected, and backup persistence lands in the same deployment profile where the cloud-safe scheduler writes it.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #11724
• Related Graph Nodes: #11720, #11721, #11723, #11725, #10803, #10801, ADR 0014, learn/agentos/DeploymentCookbook.md, learn/agentos/SharedDeployment.md

🔬 Depth Floor

Challenge: The new ai/deploy/Caddyfile says the proxy strips client-supplied identity headers before anything trusts them, then shows an operator-uncommentable forward_auth block. Caddy's Caddyfile adapter does not execute differently named directives in typed order by default: the official directive order lists forward_auth before request_header, and the route directive is the documented way to preserve literal order. The existing manual Caddy proxy already uses route { ... } for this reason. So if an operator enables the provided auth block as written, the auth middleware can run before the spoofed identity headers are stripped, contradicting the file's own security model. See Caddy's route docs and directive-order docs: https://caddyserver.com/docs/caddyfile/directives/route and https://caddyserver.com/docs/caddyfile/directives#directive-order.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: the ingress / backup / port-fix framing matches the diff.
• Anchor & Echo summaries: N/A; no code JSDoc or class summaries added.
• [RETROSPECTIVE] tag: N/A; none added.
• Linked anchors: the ai/deploy/Caddyfile security prose borrows the SharedDeployment.md trust-proxy model, but the Caddy directive structure does not yet mechanically enforce the documented order when auth is enabled.

Findings: One security-order drift flagged as the Required Action.

🧠 Graph Ingestion Notes

• [KB_GAP]: None on Neo deployment concepts; the profile, shared SQLite mount, backup path, and proxy port mapping line up with the current docs.
• [TOOLING_GAP]: Docker and Caddy are unavailable in this agent sandbox, so I could not run docker compose config or caddy adapt. I used YAML parsing plus source/docs checks, while CI covers repository tests.
• [RETROSPECTIVE]: For Caddy proxy examples that mix identity-header stripping, forward_auth, and path routing, keep using an explicit route { ... } block. The security primitive is ordered middleware, not comments describing the intended order.

🛂 Provenance Audit

N/A — this is an implementation of the already-filed deployment-profile sub, not a new architectural abstraction.

🎯 Close-Target Audit

• Close-targets identified: Resolves #11724 in the PR body.
• #11724 checked live: labels enhancement, ai, build; not epic-labeled.
• Branch commit messages checked via git log origin/dev..HEAD --format='%h%x09%s%n%b'; no stray Closes / Fixes / Resolves keyword appears in commit bodies.

Findings: Pass.

📑 Contract Completeness Audit

• Originating ticket #11724 defines ACs for production compose profile, reverse proxy + TLS, redeploy-safe persistence, and copy-paste runtime handoff.
• Implemented diff covers the intended surfaces: ingress profile, Caddy config, backup bind mount, and manual proxy port correction.
• One contract drift remains: the reverse-proxy spoofing-defense order is documented but not guaranteed by the Caddyfile structure once forward_auth is enabled.

Findings: Contract drift flagged in the Required Action.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence is L1 static verification with an explicit L2 residual for AC4 runtime stand-up in #11725.
• The two-ceiling distinction is honest: Docker/Caddy runtime proof is outside this sandbox and explicitly left to Sub D #11725.
• Review language does not promote the static checks to runtime proof.

Findings: Pass.

📜 Source-of-Authority Audit

N/A — this review demand is grounded in Caddy directive-order docs and repo-local proxy precedent, not in operator or peer authority.

📡 MCP-Tool-Description Budget Audit

N/A — no ai/mcp/server/*/openapi.yaml surface is touched.

🔌 Wire-Format Compatibility Audit

N/A — no JSON-RPC, A2A, or MCP payload schema changes.

🔗 Cross-Skill Integration Audit

• Existing deployment docs already document NEO_PUBLIC_URL and NEO_AUTH_TRUST_PROXY_IDENTITY; this PR correctly leaves operator-owned auth as an extension point rather than pretending it can be zero-config.
• Manual proxy references were synced to the compose port values.
• Compose Caddyfile should mirror the manual Caddyfile's route { ... } ordering discipline for auth/header/routing sections.

Findings: One integration gap, same Required Action.

🧪 Test-Execution & Location Audit

• Branch checked out locally via checkout_pull_request.
• Ran git diff --check origin/dev...HEAD: passed.
• Parsed ai/deploy/docker-compose.yml with Ruby YAML: 5 services (chroma, kb-server, mc-server, orchestrator, ingress) and 4 volumes (chroma-data, shared-sqlite-data, caddy-data, caddy-config).
• Could not run docker compose -f ai/deploy/docker-compose.yml config: docker is not installed in this sandbox.

Findings: Static checks pass; runtime compose validation remains #11725 / operator environment scope.

🛡️ CI / Security Checks Audit

• Ran gh pr checks 11747.
• Confirmed no checks are pending/in-progress.
• Confirmed no checks are failing.

Findings: Pass - all checks green: lint-pr-body, CodeQL/Analyze, unit, and integration-unified.

📋 Required Actions

To proceed with merging, please address the following:

• Wrap the ordered security/auth/routing body in ai/deploy/Caddyfile in route { ... }, matching the existing ai/mcp/deploy/proxy/Caddyfile pattern. The required mechanical property is: strip client-supplied identity headers first, then optional forward_auth, then route/proxy to /kb/* and /mc/*. Without route, Caddy's default directive order places forward_auth before request_header, so enabling the documented auth block can violate the spoofing-defense order the file claims.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 82 - 18 points deducted because the deployment topology matches the intended Sub C shape, but the Caddy auth/header ordering discipline diverges from the existing manual proxy precedent.
• [CONTENT_COMPLETENESS]: 88 - 12 points deducted because the PR body, deltas, evidence, and residuals are clear, but the compose Caddyfile comments overstate the security order until the structure preserves it.
• [EXECUTION_QUALITY]: 78 - 22 points deducted for one config-order defect in a security-sensitive path; CI and static checks are otherwise green, and runtime proof is properly deferred.
• [PRODUCTIVITY]: 86 - 14 points deducted because the main deployment-profile work is present, but merge should wait on the small Caddy order fix.
• [IMPACT]: 68 - This is MVP-critical deployment substrate for #11720, but it is a reference-profile increment rather than the full runtime proof.
• [COMPLEXITY]: 52 - Moderate: four config/docs-adjacent files, compose service topology, TLS/proxy routing, and auth boundary semantics.
• [EFFORT_PROFILE]: Quick Win - The required fix is mechanically small and preserves the otherwise correct high-value deployment profile.

After the Caddyfile is wrapped in route {}, I expect this to be approval-ready without another broad concern.