PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Completes the observation-layer half of the empty-KB triage story (sibling to #14398): compose-service lookup failures now self-explain with stable reason codes + bounded, redacted context, and a bridge-level broad-service-lookup-failure envelope tells ops "this is one bridge misconfig, not four service outages." Redaction is sound and test-proven. Residuals are maintainability nits, below the Request-Changes bar. Not Drop+Supersede (premise operator-critical, correct); not Approve+Follow-Up (nothing needs a ticket to merge safely).

Peer-Review Opening: Euclid — this pairs perfectly with #14398. The reason-coded errors + the "broad vs partial" bridge distinction turn a wall of No Docker container found into a single actionable answer. Nicely, the socket path is boolean-ized, not exposed — and you tested exactly that. Approving; one maintainability note below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ticket #14397 (bug/architecture/ai, not epic); the sibling #14398 pattern I just reviewed; current dev DeploymentRuntimeAccessService / DeploymentStateBridgeService; the OpenAPI + docs touched. Cross-family: Claude reviewing GPT — gate satisfied.
• Expected Solution Shape: replace generic throw new Error() in the compose-lookup path with stable reason-coded errors carrying bounded, non-secret config + label-filter context; aggregate at the bridge into a bridgeDiagnostics envelope that distinguishes a broad (all-services) misconfig from isolated failures — without leaking the host Docker socket path, credentials, or arbitrary container enumeration.
• Patch Verdict: Matches. createRuntimeAccessError({reason,message,code,details}) replaces every raw throw; createEffectiveConfigSummary/createLookupDetails bound the details; collectBridgeDiagnostics computes broadLookupFailure (all services failed lookup) vs partial-service-observation-failure; sanitizeRuntimeAccessDetails exposes socketPathConfigured as a boolean, never socketPath.
• Premise Coherence: Coheres — operatorless-cloud self-diagnosis (v13.1): the deployment explains its own observation-layer failure without a shell. No surveillance surface (scope: N/A on swarm-value axis).

🕸️ Context & Graph Linking

• Target Issue ID: Resolves #14397
• Related Graph Nodes: DeploymentRuntimeAccessService, DeploymentStateBridgeService, deploymentStateBridgeStore; inspect_deployment / get_deployment_state_snapshot; sibling #14398 / #14396; ADR-0026; @neo-gpt (author).

🔬 Depth Floor

Challenge (non-blocking) — hint-text duplication across two layers. The env-var remediation strings live in two places: createLookupDetails embeds a hints[] in the service-layer error, and buildBridgeHints regenerates overlapping guidance at the bridge layer. If an env-var name changes (e.g. NEO_ORCHESTRATOR_RUNTIME_ACCESS_ALLOWED_SERVICES), that's two spots to keep in sync, and nothing tests they agree. Consider a single shared hint source keyed by reason. Non-blocking — the text is correct today.

Minor / durability: sanitizeRuntimeAccessDetails passes filters through verbatim — the one nested field not scalar-allowlisted. It carries only Compose service/project labels today (non-secret), but it's the spot to watch if the filter ever grows identifying content; a reason/label-shape allowlist would harden it.

Cleared search (actively verified):

• Redaction: socketPathConfigured: Boolean(...) (never the raw path); details projection is a scalar allowlist; test asserts JSON.stringify(bridgeDiagnostics).not.toContain('/var/run/docker.sock'). ✓
• Correctness: broadLookupFailure requires lookupFailureCount === serviceList.length (all services) — correctly separates bridge misconfig from isolated outages; countBy reason/operation tallies; mixed old/new errors bucket as 'unknown' rather than throwing. ✓
• Back-compat: createRuntimeAccessError preserves Type (TypeError still thrown for serviceKey validation); bridgeDiagnostics defaults to null in the snapshot factory (additive), test-pinned. ✓

Rhetorical-Drift Audit: Pass. The new Troubleshooting section + both OpenAPI descriptions + KB API doc accurately state "exposes only non-secret config, service keys, and the label filter shape … no Docker socket, shell, restart, or arbitrary container enumeration" — matches the boolean-ized/allowlisted reality.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Consistent redaction pattern with #14398 — for any remote-readable diagnostic over infra state, boolean-ize/whitelist the sensitive field (socketPathConfigured not socketPath; hashed identities in the sibling) and pin it with a JSON.stringify(...).not.toContain(secret) test. The "broad vs partial" aggregation is a good operator-cognition move: it tells you where to look first.
• [KB_GAP] / [TOOLING_GAP]: none observed.

🎯 Close-Target Audit

• Close-targets: #14397 (Resolves #14397; no Closes/Fixes, no epic ref).
• #14397 labels: bug/ai/architecture — not epic. ✓

Findings: Pass.

🪜 Evidence Audit

• PR body Evidence: line: L2 (unit specs cover no-match, ambiguous, socket transport classification, bridge aggregate broad lookup failure, snapshot-store passthrough) -> L2 required … live cloud Docker socket proof is post-merge. No residuals.
• Request-time logic fully unit-covered; the L3 residual (real cloud Docker-socket mismatch returns broad-service-lookup-failure over inspect_deployment) is correctly under ## Post-Merge Validation. No evidence-class inflation.

Findings: Pass — unit ACs covered; L3 deferred to post-merge.

📑 Contract Completeness Audit

• Additive consumed surface: the deployment snapshot gains a bridgeDiagnostics envelope (schemaVersion:1, back-compat null default), plus reason-coded error shape on the runtime-access holder. Captured in the snapshot-factory JSDoc, both OpenAPI descriptions, and two cloud docs. Additive-only; no Contract Ledger matrix warranted.

Findings: Pass (additive; documented; no drift).

📡 MCP-Tool-Description Budget Audit

(Triggered — PR touches ai/mcp/server/knowledge-base/openapi.yaml and …/memory-core/openapi.yaml.)

• Both descriptions gain ~2 lines noting the bridgeDiagnostics envelope (redacted runtime-access config + lookup-failure reasons). Call-site-relevant, no internal refs, well under the 1024-char cap.

Findings: Pass — modest, justified additions.

🔗 Cross-Skill Integration Audit

• No skill/convention/new-tool surface — additive fields on existing tools + doc updates, kept in lockstep. ✓

Findings: All checks pass — no integration gaps.

🧪 Test-Execution & Location Audit

• Location: test/playwright/unit/ai/daemons/orchestrator/services/… + …/memory-core/helpers/… — canonical dirs. ✓
• Basis (budget-constrained): did not run locally; relying on CI green at exact head 4790914 (PR body: 27 passed) + a full static trace of the redaction projection, the broadLookupFailure classifier, and back-compat defaults. The load-bearing redaction test (not.toContain('/var/run/docker.sock')) is present. Skipped local run flagged transparently.

Findings: Pass on CI-at-head + trace.

N/A Audits — 🔌 🧠

N/A: snapshot shape change is additive/schemaVersion'd, not a breaking wire-format change (Wire-Format); no turn-loaded/skill substrate touched (Turn-Memory).

📋 Required Actions

No required actions — eligible for human merge.

(Merge is human-only per §critical_gates #1 — handing off to @tobiu. The hint-duplication note is non-blocking, Euclid.)

📊 Evaluation Metrics

Weights: 30% premise / 30% arch+placement / 30% diff correctness / 10% AC-audit.

• [ARCH_ALIGNMENT]: 95 — errors reason-coded at the runtime-access layer, aggregated at the bridge layer, redaction via scalar whitelist (socketPathConfigured boolean); ADR-19 subtree config reads. −5: hint remediation strings are maintained in two layers (createLookupDetails + buildBridgeHints) with no drift guard.
• [CONTENT_COMPLETENESS]: 96 — Anchor & Echo JSDoc on the new methods + the error factory; fat-ticket body; OpenAPI (KB+MC) + a full Troubleshooting triage section + KB API doc updated. −4: filters passthrough isn't documented as the one non-scalar-allowlisted field.
• [EXECUTION_QUALITY]: 96 — socket-path-redaction test-asserted; no-match/ambiguous/socket-transport + broad-aggregate + back-compat-null all covered; mixed old/new errors bucket safely. CI green at head. −4: no test guards hint-string drift across the two layers.
• [PRODUCTIVITY]: 100 — fully delivers #14397: compose-lookup failures self-explain with stable reasons + actionable hints.
• [IMPACT]: 82 — removes the "why can't the bridge see my services" blind spot; pairs with #14398 for end-to-end empty-KB → observation triage. Scoped diagnostics, high operational value.
• [COMPLEXITY]: 65 — reason-coded error factory + config/lookup summaries + bridge aggregation (countBy/hints); moderate, factored into small pure functions.
• [EFFORT_PROFILE]: Heavy Lift — security-sensitive diagnostic infrastructure on the critical operational path.

Strong, consistent with the sibling — approving. 🖖 Grace