PR Review Provenance — tangle-network/agent-dev-container#386

Generated by pr-reviewer v0.5.0

Plan

{
  "coverage_gaps": [
    "No separate track for test quality; the test surface is small enough to review inside the two behavior tracks.",
    "No integration/runtime track for Docker or actual sidecar startup because the diff is narrow and the main review risk is semantic regression, not orchestration plumbing volume.",
    "No style/refactor track because the extracted helper itself is low risk unless it changes semantics, which the first track already owns."
  ],
  "recommended_provider_mix": "Use a strongest-reasoning reviewer on the capability policy track because the main bug surface is subtle security semantics around undefined, empty, and invalid env values. Use a fast code-accurate reviewer on the driver-defaults track to trace helper behavior through constructor paths and confirm caller override semantics. Two tracks are sufficient; more would create overlap without improving defect discovery.",
  "risks": [
    "Silent security posture change: containers created through driver helpers may now gain `SETUID` and `SETGID` where they previously had no added capabilities.",
    "Config defaults and driver defaults can drift again because both now depend on a shared helper but still apply defaults in separate places.",
    "Unit tests validate object construction, but not whether the downstream container runtime interprets these capabilities safely or as intended."
  ],
  "summary": "This change extracts container capability resolution into a shared helper and, more importantly, changes orchestrator driver defaults from `add: []` to `add: [\"SYS_PTRACE\", \"SETUID\", \"SETGID\"]` unless `DEFAULT_CAP_ADD` overrides them. The real risk is not the refactor itself; it is the behavioral change in default security posture across driver-created containers and whether existing override/disable paths still work exactly as intended.",
  "tracks": [
    {
      "evidence_targets": [
        "resolveDefaultContainerCapabilities() parsing of undefined vs empty string vs comma-separated values",
        "ALLOWED_CONTAINER_CAPABILITIES and whether the new default set is intentionally broader than before",
        "InvalidConfigError path and whether bad values fail at config load consistently",
        "config.defaults.container.security.capabilities.add import-time behavior after vi.resetModules()",
        "tests covering disable-by-empty-string and rejection of unsupported capabilities"
      ],
      "goal": "Verify the shared capability resolver preserves the intended policy: allowed list enforcement, correct defaults, and explicit hardening via empty `DEFAULT_CAP_ADD` without accidental fallback.",
      "scope": [
        "apps/orchestrator/src/config.ts",
        "apps/orchestrator/src/config/container-capabilities.ts",
        "apps/orchestrator/tests/unit/config.test.ts"
      ],
      "should_use_subagents": false,
      "suggested_provider": "gpt-5",
      "track_id": "capability-policy-and-env-semantics"
    },
    {
      "evidence_targets": [
        "createDriverConfig() default security.capabilities behavior before vs after this PR",
        "createSecurityOptions() behavior when callers omit capabilities entirely",
        "behavior when callers pass partial security options or explicit empty capability arrays/objects",
        "all call sites that rely on driver defaults rather than config.defaults.container.security",
        "tests proving helpers honor env overrides and do not overwrite explicit caller-provided capabilities"
      ],
      "goal": "Check whether changing driver/security helper defaults from no added capabilities to subprocess-safe capabilities introduces privilege expansion or breaks caller override expectations.",
      "scope": [
        "apps/orchestrator/src/driver/interface/index.ts",
        "apps/orchestrator/tests/unit/config.test.ts"
      ],
      "should_use_subagents": false,
      "suggested_provider": "claude-sonnet",
      "track_id": "driver-defaults-and-override-regressions"
    }
  ]
}

01-capability-policy-and-env-semantics

claude — 1 findings

Scope: apps/orchestrator/src/config.ts, apps/orchestrator/src/config/container-capabilities.ts, apps/orchestrator/tests/unit/config.test.ts

🟠 [medium] resolveDefaultContainerCapabilities() evaluated lazily in driver interface but eagerly in config.ts (apps/orchestrator/src/driver/interface/index.ts:112)

In config.ts, resolveDefaultContainerCapabilities() is called at module load time (it's inside a module-level const config = { ... } initializer). In driver/interface/index.ts, it's called inside createDriverConfig() (line 112) and createSecurityOptions() (line 147) — meaning it runs on every invocation when no capabilities are provided.

This creates two issues:

If the driver interface is used without importing config.ts (e.g., in tests, or a future entry point), an invalid DEFAULT_CAP_ADD value won't fail at startup — it'll throw InvalidConfigError at container creation time, potentially mid-operation.
Each call re-parses and re-validates the env var. While negligible for performance, it means the resolved capabilities aren't guaranteed to be consistent across calls if process.env is mutated between invocations.

Neither is likely to cause production issues today since the orchestrator imports config.ts at startup (catching bad values early), but it's a latent inconsistency worth noting.

config.ts line 410: `add: resolveDefaultContainerCapabilities()` — called once at import.
driver/interface/index.ts line 112: `add: resolveDefaultContainerCapabilities()` — called per `createDriverConfig()` invocation.
driver/interface/index.ts line 147: `add: resolveDefaultContainerCapabilities()` — called per `createSecurityOptions()` invocation.

codex — 0 findings

Scope: apps/orchestrator/src/config.ts, apps/orchestrator/src/config/container-capabilities.ts, apps/orchestrator/tests/unit/config.test.ts

No findings from this reviewer.

02-driver-defaults-and-override-regressions

claude — 2 findings

Scope: apps/orchestrator/src/driver/interface/index.ts, apps/orchestrator/tests/unit/config.test.ts

🟠 [medium] No test verifies explicit caller-provided capabilities are preserved by createSecurityOptions or createDriverConfig (apps/orchestrator/tests/unit/config.test.ts:112)

Every test of createSecurityOptions passes an empty object ({}), and every test of createDriverConfig omits the security field entirely. This means there is no test asserting that when a caller provides explicit capabilities — e.g. createSecurityOptions({ capabilities: { drop: ['ALL'], add: ['NET_BIND_SERVICE'] } }) — those capabilities are returned unchanged rather than being replaced by the new subprocess-safe defaults. The only thing preventing the defaults from stomping caller-provided caps is the || operator on line 145 of index.ts (and line 106 for createDriverConfig). If someone refactors this to use nullish coalescing, deep-merge, or spread semantics, there is no test to catch the regression. Given that this PR specifically widens the default privilege set, a test proving caller overrides are respected is important to prevent unintentional privilege expansion for callers who deliberately restrict capabilities.
```
All createSecurityOptions test calls use createSecurityOptions({}) — grep for 'createSecurityOptions({' in tests returns only empty-object invocations. No createDriverConfig test passes an explicit security field.
```
🟡 [low] resolveDefaultContainerCapabilities() is evaluated at call time, not module load — capabilities can drift between calls (apps/orchestrator/src/driver/interface/index.ts:112)

In createDriverConfig (line 112) and createSecurityOptions (line 147), resolveDefaultContainerCapabilities() is called on every invocation rather than being captured once at module load. This means if process.env.DEFAULT_CAP_ADD is mutated at runtime between two calls, the two containers would get different capability sets. In the config.ts path this is fine (evaluated once at module load), but in the driver helpers it's evaluated per-call. This is likely intentional for testability (the tests rely on vi.resetModules + env mutation), but it's worth noting that the config.ts and driver paths have subtly different evaluation semantics for the same function.
```
config.ts calls resolveDefaultContainerCapabilities() once during module evaluation. createDriverConfig and createSecurityOptions call it on every invocation. If process.env.DEFAULT_CAP_ADD changes between calls, containers get different caps.
```

codex — 1 findings