Generated by pr-reviewer v0.5.0
{
"coverage_gaps": [
"The `.memory/*` additions are not worth a dedicated review track; they document the bug but do not change runtime behavior.",
"This plan does not spend a separate track on unit tests themselves; test sufficiency is reviewed inside each owning runtime track because the change is small and behavior-centric.",
"It does not broaden into unrelated package-manager semantics beyond npm/cargo/pnpm cache ownership, because the primary regression surface here is process identity and path resolution."
],
"recommended_provider_mix": "Use a strong reasoning model for the first two tracks because the bugs are in env precedence, path resolution, and cross-process behavior rather than syntax. The cargo-bootstrap track can use a cheaper code-review model because it is a narrow ownership/exec-options audit with limited blast radius.",
"risks": [
"Environment precedence is easy to get subtly wrong: one misplaced merge order can either reintroduce root-owned caches or override intentional caller-supplied cache paths.",
"Changing state-root resolution in shared code affects persisted server port files and reconnect behavior, which may not fail until restart or recovery scenarios.",
"The new cache split depends on every subprocess launcher opting into `getAgentCacheEnvironment`; any unmodified launcher elsewhere in the repo may still inherit supervisor paths."
],
"summary": "This change splits package-manager cache environment between the root sidecar supervisor and agent-owned subprocesses, centralizes sidecar state-root resolution, and forces Cargo config bootstrap writes to run as `agent` instead of `root`. Real risk is in environment precedence and process-boundary behavior: a small mistake here can reintroduce root-owned cache trees, break reconnect/state lookup, or silently change subprocess runtime behavior across local and docker drivers.",
"tracks": [
{
"evidence_targets": [
"Precedence in `resolveSidecarStateRoot`: `STORAGE_PATH` before `AGENT_WORKSPACE_ROOT` before explicit `workspaceRoot` fallback",
"Whether `getSupervisorCacheEnvironment` writes all cache dirs under `.sidecar/state/supervisor-cache` and never back into `/home/agent` directly",
"Local root-supervisor startup path in `LocalDriver.startContainer`: does the env merge preserve required subprocess identity vars while overriding only cache locations",
"Docker bootstrap env rewriting in `sidecar-bootstrap.ts`: correctness of `envArrayToObject` and `mergeEnvArray`, especially with existing duplicate keys and values containing `=`",
"Opencode adapter reconnection/state lookup: moving to `resolveSidecarStateRoot({ workspaceRoot })` must preserve port-file location across storage-path configurations"
],
"goal": "Verify that root supervisor processes now use sidecar-managed cache paths without breaking state-root resolution or bootstrap env construction.",
"scope": [
"packages/shared/src/cache-env.ts",
"packages/shared/src/index.ts",
"packages/shared/src/sidecar-bootstrap.ts",
"apps/orchestrator/src/driver/local.ts",
"packages/sdk-provider-opencode/src/adapter.ts",
"packages/shared/tests/cache-env.test.ts",
"apps/orchestrator/tests/unit/local-driver-process-user.test.ts"
],
"should_use_subagents": false,
"suggested_provider": "gpt-5",
"track_id": "supervisor-cache-and-state-root"
},
{
"evidence_targets": [
"Workspace-root selection in each launcher: `AGENT_WORKSPACE_ROOT` from explicit env, then process env, then cwd fallback",
"Env merge order: `process.env` -> agent cache env -> caller overrides; check whether explicit caller cache vars can still intentionally override defaults",
"Root-to-agent demotion path via `resolveSubprocessIdentity`: cache env should point to agent-owned paths when uid/gid are set",
"Consistency across PTY, LSP, Newt, and Opencode server so no launcher is left on supervisor cache paths",
"Interaction with `HOME`/`XDG_*`/tool-specific dirs: verify no launcher now points caches at one root while leaving home-based state elsewhere in a way that breaks npm/cargo/pnpm behavior"
],
"goal": "Verify that all agent-owned subprocess launchers restore agent cache paths correctly and do not inherit supervisor cache env by accident.",
"scope": [
"apps/sidecar/src/lsp/server-process.ts",
"apps/sidecar/src/newt/newt-manager.ts",
"apps/sidecar/src/terminal/pty-manager.ts",
"packages/sdk-provider-opencode/src/server.ts",
"apps/sidecar/tests/unit/pty-manager.test.ts",
"packages/sdk-provider-opencode/tests/server-process-user.test.ts"
],
"should_use_subagents": false,
"suggested_provider": "gpt-5",
"track_id": "agent-subprocess-cache-env-restoration"
},
{
"evidence_targets": [
"Docker exec payloads now include `User: \"agent\"` on the Cargo config write path in both call sites",
"Whether the target command still has access to `CARGO_HOME` when run as `agent`, and whether directory creation/file write semantics remain correct",
"Error-path handling if `agent` user is missing or lacks permission; the change should fail loudly rather than silently skipping config",
"Test coverage only asserts the exec option is set; inspect whether there is any missing validation around ownership, exit-code handling, or runtime route parity"
],
"goal": "Verify that Cargo proxy config bootstrap now writes agent-owned files in both docker-driver and host-agent runtime flows, and that failure behavior remains visible.",
"scope": [
"apps/orchestrator/src/driver/docker.ts",
"apps/host-agent/src/routes/runtime.ts",
"apps/orchestrator/tests/unit/docker-driver-startsidecar.test.ts"
],
"should_use_subagents": false,
"suggested_provider": "gpt-5-mini",
"track_id": "cargo-bootstrap-ownership"
}
]
}Scope: packages/shared/src/cache-env.ts, packages/shared/src/index.ts, packages/shared/src/sidecar-bootstrap.ts, apps/orchestrator/src/driver/local.ts, packages/sdk-provider-opencode/src/adapter.ts, packages/shared/tests/cache-env.test.ts, apps/orchestrator/tests/unit/local-driver-process-user.test.ts
-
🟠 [medium] getEnvValue and envArrayToObject disagree on duplicate-key semantics (
packages/shared/src/sidecar-bootstrap.ts:735)In sidecar-bootstrap.ts,
getEnvValue(line 68) returns the first matching entry for a key, whileenvArrayToObject(line 78) uses object assignment so the last entry wins. On line 735-741,getEnvValue(containerEnv, 'AGENT_WORKSPACE_ROOT')feeds theworkspaceRootparameter whileenvArrayToObject(containerEnv)feeds theenvparameter ofgetSupervisorCacheEnvironment. IfcontainerEnvever contains duplicateAGENT_WORKSPACE_ROOTentries (e.g., from Docker image layers plus container config), theenvobject would resolve to the last value whileworkspaceRootresolves to the first, causing the state root resolution to use inconsistent workspace roots. In practice Docker env arrays rarely have true duplicates, but this is a latent correctness issue.const sidecarEnv = mergeEnvArray( containerEnv, getSupervisorCacheEnvironment({ env: envArrayToObject(containerEnv), // last-wins workspaceRoot: getEnvValue(containerEnv, "AGENT_WORKSPACE_ROOT"), // first-wins }), ); -
🟠 [medium] resolveSidecarStateRoot test suite missing AGENT_WORKSPACE_ROOT and workspaceRoot-only fallback cases (
packages/shared/tests/cache-env.test.ts:18)The test at cache-env.test.ts only covers the STORAGE_PATH-first precedence and the supervisor cache path derivation. There is no test for: (1) AGENT_WORKSPACE_ROOT without STORAGE_PATH resolving to
<root>/.sidecar/state, (2) the final fallback when both env vars are absent (should useworkspaceRootor default to/home/agent), (3) the edge case where STORAGE_PATH is whitespace-only (should be treated as absent due totrimEnvValue). These are the exact precedence rules this PR centralizes — they deserve explicit coverage since multiple callers now depend on this shared function.Only one precedence test exists: it("resolves sidecar state from STORAGE_PATH first", () => { ... })
Missing: AGENT_WORKSPACE_ROOT-only, workspaceRoot-only fallback, whitespace STORAGE_PATH
- 🟡 **[low]** Adapter's resolveSidecarStateRoot call reads process.env implicitly, not the container's env (`packages/sdk-provider-opencode/src/adapter.ts:3449`)
> The adapter's `getPortFilePath` (line 3447-3453) calls `resolveSidecarStateRoot({ workspaceRoot })` without passing `env`. This defaults to `process.env` inside `resolveSidecarStateRoot`. The old private `getSidecarStateRoot` also read `process.env` directly, so this is behavior-preserving. However, the adapter runs in the sidecar supervisor process whose env now gets rewritten by `getSupervisorCacheEnvironment`. If `STORAGE_PATH` or `AGENT_WORKSPACE_ROOT` in the supervisor's `process.env` differs from what the sidecar originally resolved, the port file path could shift. This is low severity because in practice the supervisor inherits these env vars unchanged from the container, but it's worth noting as an implicit coupling.
resolveSidecarStateRoot({ workspaceRoot }) // env defaults to process.env
### codex — 0 findings
Scope: `packages/shared/src/cache-env.ts, packages/shared/src/index.ts, packages/shared/src/sidecar-bootstrap.ts, apps/orchestrator/src/driver/local.ts, packages/sdk-provider-opencode/src/adapter.ts, packages/shared/tests/cache-env.test.ts, apps/orchestrator/tests/unit/local-driver-process-user.test.ts`
No findings from this reviewer.
## 02-agent-subprocess-cache-env-restoration
### claude — 2 findings
Scope: `apps/sidecar/src/lsp/server-process.ts, apps/sidecar/src/newt/newt-manager.ts, apps/sidecar/src/terminal/pty-manager.ts, packages/sdk-provider-opencode/src/server.ts, apps/sidecar/tests/unit/pty-manager.test.ts, packages/sdk-provider-opencode/tests/server-process-user.test.ts`
- 🔴 **[high]** Newt manager env spread undoes agent cache fix (`apps/sidecar/src/newt/newt-manager.ts:217`)
> In `NewtManager.start()`, the env merge order is `...process.env, ...getAgentCacheEnvironment(workspaceRoot), ...env`. The `env` parameter defaults to `process.env` (line 168) and is `{ ...process.env }` when called from `configure()` (line 157) or restart (line 396). Since the sidecar's `process.env` contains supervisor cache vars (set by `sidecar-bootstrap.ts`), the final `...env` spread re-applies them _after_ `getAgentCacheEnvironment()` has overridden them. This means Newt subprocesses will still inherit supervisor cache paths (e.g., `NPM_CONFIG_CACHE=/home/agent/.sidecar/state/supervisor-cache/.cache/npm`), defeating the fix. The other three launchers (PTY, LSP, OpenCode) don't have this problem because their final spread is either a partial env (specific caller overrides, not a full process.env copy) or absent entirely.
Line 168: start(env: Env = process.env) — env defaults to process.env.
Lines 214-218:
const spawnEnv = {
...process.env,
...getAgentCacheEnvironment(workspaceRoot),
...env, // re-applies process.env including supervisor cache vars
};
Configure path (line 157): const nextEnv = { ...process.env } → passed to start().
Restart path (line 396): const restartEnv = this.#lastEnv ? { ...this.#lastEnv } : process.env → passed to start().
- 🟠 **[medium]** No test coverage for Newt cache env isolation (`apps/sidecar/src/newt/newt-manager.ts:214`)
> PTY and OpenCode both have tests verifying that supervisor cache vars in `process.env` are overridden with agent-owned paths in the spawn env. Newt has no equivalent test, which allowed the bug above to ship. Given that Newt is one of four subprocess launchers that all need the same fix, the missing test is a meaningful coverage gap.
Changed test files include apps/sidecar/tests/unit/pty-manager.test.ts (PTY) and packages/sdk-provider-opencode/tests/server-process-user.test.ts (OpenCode), both asserting agent cache paths override supervisor paths. No corresponding test exists for Newt.
### codex — 2 findings
Scope: `apps/sidecar/src/lsp/server-process.ts, apps/sidecar/src/newt/newt-manager.ts, apps/sidecar/src/terminal/pty-manager.ts, packages/sdk-provider-opencode/src/server.ts, apps/sidecar/tests/unit/pty-manager.test.ts, packages/sdk-provider-opencode/tests/server-process-user.test.ts`
- 🟠 **[medium]** PTY/LSP/Newt overwrite Docker’s session-scoped cache envs with `/home/agent` defaults (`apps/sidecar/src/terminal/pty-manager.ts:150`)
> These launchers no longer preserve the cache paths that Docker already computed for the session. In Docker, the container env is intentionally split: `AGENT_WORKSPACE_ROOT` stays `/home/agent`, while the actual cache envs are session-specific (for example `/home/agent/project-1/.cache/npm`). The new launcher code ignores those existing cache vars, takes `AGENT_WORKSPACE_ROOT` first, and then calls `getAgentCacheEnvironment(workspaceRoot)`, which rewrites the child env back to `/home/agent/.cache/...`. That collapses per-session caches into the base workspace and reintroduces cross-session sharing. The same pattern appears in `apps/sidecar/src/lsp/server-process.ts` and `apps/sidecar/src/newt/newt-manager.ts`, so this is not isolated to PTY.
apps/orchestrator/tests/unit/docker-driver-envs.test.ts:115-125 asserts NPM_CONFIG_CACHE=/home/agent/project-1/.cache/npm while AGENT_WORKSPACE_ROOT=/home/agent. But apps/sidecar/src/terminal/pty-manager.ts:150-157 does const workspaceRoot = options.env?.AGENT_WORKSPACE_ROOT?.trim() || process.env.AGENT_WORKSPACE_ROOT?.trim() || cwd; const env = { ...process.env, ...getAgentCacheEnvironment(workspaceRoot), ...options.env };, which rewrites the cache env from /home/agent/project-1/... back to /home/agent/.... apps/sidecar/src/lsp/server-process.ts:279-286 and apps/sidecar/src/newt/newt-manager.ts:210-218 use the same merge pattern.
- 🟠 **[medium]** Demoted PTY/LSP/Newt subprocesses still inherit the supervisor `HOME` (`apps/sidecar/src/terminal/pty-manager.ts:154`)
> The new cache restoration only updates XDG/package-manager cache vars; it never fixes `HOME` when the sidecar is running as root and the child is demoted via `uid`/`gid`. Because these launchers start from `...process.env` and `resolveSubprocessIdentity()` only changes numeric identity, the child can run as the agent user while still carrying the root supervisor’s home directory. For PTY this is immediately user-visible: shell startup, `~` expansion, history files, and tools that still consult `HOME` can target `/root` and hit `EACCES`. The same risk applies to LSP and Newt because they use the same env assembly pattern.
apps/sidecar/src/terminal/pty-manager.ts:154-159 builds env from ...process.env plus cache vars, then calls resolveSubprocessIdentity(env) and passes only uid/gid to spawn; there is no HOME override. apps/sidecar/src/lsp/server-process.ts:283-289 and apps/sidecar/src/newt/newt-manager.ts:214-220 do the same. Since resolveSubprocessIdentity() only returns { username, uid, gid }, the effective home directory remains whatever the root supervisor had in process.env.
## 03-cargo-bootstrap-ownership
### claude — 2 findings
Scope: `apps/orchestrator/src/driver/docker.ts, apps/host-agent/src/routes/runtime.ts, apps/orchestrator/tests/unit/docker-driver-startsidecar.test.ts`
- 🟠 **[medium]** Cargo config write failures are silently swallowed in both paths (`apps/orchestrator/src/driver/docker.ts:798`)
> Both `docker.ts:798-803` and `runtime.ts:705-709` catch exec failures and log a warning, then proceed to start the sidecar and return success. If the `agent` user doesn't exist in the container image (e.g., a custom image without the expected user), the exec will fail with a Docker API error. The caller has no indication that cargo proxy configuration was not applied — the container starts without registry source replacement, and Rust builds will silently pull from crates.io directly instead of the Kellnr proxy. This pre-dates the PR, but the ownership change makes it more likely to trigger: previously running as root would succeed even with wrong permissions, now running as `agent` adds a new failure mode (user not found). The repo's CLAUDE.md explicitly states 'NO FALLBACKS: Never implement silent fallback modes that fake success' — this pattern contradicts that policy.
catch (err) { logger.warn("Failed to write cargo config for Kellnr proxy", { containerId: id.substring(0, 12), error: err instanceof Error ? err.message : String(err), }); }
- 🟡 **[low]** Test does not verify exit-code or error-path behavior for agent-user exec (`apps/orchestrator/tests/unit/docker-driver-startsidecar.test.ts:94`)
> The new test at `docker-driver-startsidecar.test.ts:94-105` only asserts that `mockContainer.exec` was called with `User: "agent"`. It doesn't test what happens when the exec returns a non-zero exit code (e.g., `agent` user can't write to `$CARGO_HOME`), nor when the exec itself throws (user doesn't exist). The mock always returns `ExitCode: 0`. Given that the whole point of this change is fixing a permissions issue, a test exercising the permission-failure path would be valuable — it would catch any future regression where the warn-and-continue behavior is accidentally changed to throw (or vice versa).
it("writes cargo proxy config as the agent user", async () => { process.env.CONTAINER_CARGO_REGISTRY = "http://host.docker.internal:8000/api/v1/cratesio/";
await driver.startContainer("container-id");
expect(mockContainer.exec).toHaveBeenCalledWith(
expect.objectContaining({
User: "agent",
}),
);
});
### codex — 1 findings
Scope: `apps/orchestrator/src/driver/docker.ts, apps/host-agent/src/routes/runtime.ts, apps/orchestrator/tests/unit/docker-driver-startsidecar.test.ts`
- 🟠 **[medium]** Cargo config bootstrap still fails silently when the `agent` exec cannot run (`apps/host-agent/src/routes/runtime.ts:679`)
> The new ownership change makes the cargo config write depend on `docker exec` succeeding as `agent`, but both start paths still swallow failures. In `apps/host-agent/src/routes/runtime.ts`, any exception from `container.exec(...)` or `exec.start(...)` is caught and only logged, and even `inspect().ExitCode !== 0` only emits a warning before the route continues to `startSidecarServer()` and returns 200. `apps/orchestrator/src/driver/docker.ts:762-803` has the same behavior. This matters because the change increases the chance of bootstrap failure on images where the `agent` user is absent or lacks write access to `$CARGO_HOME`; those cases now look like a successful start while silently skipping the proxy config the PR is trying to enforce.
const exec = await container.exec({ ..., User: "agent", ... }); ... if (inspection.ExitCode !== 0) { logger.warn("Cargo config write exited with non-zero code", ...) } ... } catch (err) { logger.warn("Failed to write cargo config for Kellnr proxy", ...) }
## Validation (dedup + verification)
```json
{
"command": [],
"result": {
"accepted_findings": [
{
"body": "In sidecar-bootstrap.ts, `getEnvValue` (line 68) returns the *first* matching entry for a key, while `envArrayToObject` (line 78) uses object assignment so the *last* entry wins. On line 735-741, `getEnvValue(containerEnv, 'AGENT_WORKSPACE_ROOT')` feeds the `workspaceRoot` parameter while `envArrayToObject(containerEnv)` feeds the `env` parameter of `getSupervisorCacheEnvironment`. If `containerEnv` ever contains duplicate `AGENT_WORKSPACE_ROOT` entries (e.g., from Docker image layers plus container config), the `env` object would resolve to the last value while `workspaceRoot` resolves to the first, causing the state root resolution to use inconsistent workspace roots. In practice Docker env arrays rarely have true duplicates, but this is a latent correctness issue.",
"category": "correctness",
"confidence": "medium",
"evidence": "const sidecarEnv = mergeEnvArray(\n containerEnv,\n getSupervisorCacheEnvironment({\n env: envArrayToObject(containerEnv), // last-wins\n workspaceRoot: getEnvValue(containerEnv, \"AGENT_WORKSPACE_ROOT\"), // first-wins\n }),\n );",
"file": "packages/shared/src/sidecar-bootstrap.ts",
"line": 735,
"severity": "medium",
"title": "getEnvValue and envArrayToObject disagree on duplicate-key semantics"
},
{
"body": "The test at cache-env.test.ts only covers the STORAGE_PATH-first precedence and the supervisor cache path derivation. There is no test for: (1) AGENT_WORKSPACE_ROOT without STORAGE_PATH resolving to `<root>/.sidecar/state`, (2) the final fallback when both env vars are absent (should use `workspaceRoot` or default to `/home/agent`), (3) the edge case where STORAGE_PATH is whitespace-only (should be treated as absent due to `trimEnvValue`). These are the exact precedence rules this PR centralizes \u2014 they deserve explicit coverage since multiple callers now depend on this shared function.",
"category": "testing",
"confidence": "high",
"evidence": "Only one precedence test exists:\n it(\"resolves sidecar state from STORAGE_PATH first\", () => { ... })\n\nMissing: AGENT_WORKSPACE_ROOT-only, workspaceRoot-only fallback, whitespace STORAGE_PATH",
"file": "packages/shared/tests/cache-env.test.ts",
"line": 18,
"severity": "medium",
"title": "resolveSidecarStateRoot test suite missing AGENT_WORKSPACE_ROOT and workspaceRoot-only fallback cases"
},
{
"body": "The adapter's `getPortFilePath` (line 3447-3453) calls `resolveSidecarStateRoot({ workspaceRoot })` without passing `env`. This defaults to `process.env` inside `resolveSidecarStateRoot`. The old private `getSidecarStateRoot` also read `process.env` directly, so this is behavior-preserving. However, the adapter runs in the sidecar supervisor process whose env now gets rewritten by `getSupervisorCacheEnvironment`. If `STORAGE_PATH` or `AGENT_WORKSPACE_ROOT` in the supervisor's `process.env` differs from what the sidecar originally resolved, the port file path could shift. This is low severity because in practice the supervisor inherits these env vars unchanged from the container, but it's worth noting as an implicit coupling.",
"category": "correctness",
"confidence": "high",
"evidence": "resolveSidecarStateRoot({ workspaceRoot }) // env defaults to process.env",
"file": "packages/sdk-provider-opencode/src/adapter.ts",
"line": 3449,
"severity": "low",
"title": "Adapter's resolveSidecarStateRoot call reads process.env implicitly, not the container's env"
},
{
"body": "These launchers no longer preserve the cache paths that Docker already computed for the session. In Docker, the container env is intentionally split: `AGENT_WORKSPACE_ROOT` stays `/home/agent`, while the actual cache envs are session-specific (for example `/home/agent/project-1/.cache/npm`). The new launcher code ignores those existing cache vars, takes `AGENT_WORKSPACE_ROOT` first, and then calls `getAgentCacheEnvironment(workspaceRoot)`, which rewrites the child env back to `/home/agent/.cache/...`. That collapses per-session caches into the base workspace and reintroduces cross-session sharing. The same pattern appears in `apps/sidecar/src/lsp/server-process.ts` and `apps/sidecar/src/newt/newt-manager.ts`, so this is not isolated to PTY.",
"category": "regression",
"confidence": "high",
"evidence": "`apps/orchestrator/tests/unit/docker-driver-envs.test.ts:115-125` asserts `NPM_CONFIG_CACHE=/home/agent/project-1/.cache/npm` while `AGENT_WORKSPACE_ROOT=/home/agent`. But `apps/sidecar/src/terminal/pty-manager.ts:150-157` does `const workspaceRoot = options.env?.AGENT_WORKSPACE_ROOT?.trim() || process.env.AGENT_WORKSPACE_ROOT?.trim() || cwd; const env = { ...process.env, ...getAgentCacheEnvironment(workspaceRoot), ...options.env };`, which rewrites the cache env from `/home/agent/project-1/...` back to `/home/agent/...`. `apps/sidecar/src/lsp/server-process.ts:279-286` and `apps/sidecar/src/newt/newt-manager.ts:210-218` use the same merge pattern.",
"file": "apps/sidecar/src/terminal/pty-manager.ts",
"line": 150,
"severity": "medium",
"title": "PTY/LSP/Newt overwrite Docker\u2019s session-scoped cache envs with `/home/agent` defaults"
},
{
"body": "The new cache restoration only updates XDG/package-manager cache vars; it never fixes `HOME` when the sidecar is running as root and the child is demoted via `uid`/`gid`. Because these launchers start from `...process.env` and `resolveSubprocessIdentity()` only changes numeric identity, the child can run as the agent user while still carrying the root supervisor\u2019s home directory. For PTY this is immediately user-visible: shell startup, `~` expansion, history files, and tools that still consult `HOME` can target `/root` and hit `EACCES`. The same risk applies to LSP and Newt because they use the same env assembly pattern.",
"category": "operational",
"confidence": "medium",
"evidence": "`apps/sidecar/src/terminal/pty-manager.ts:154-159` builds `env` from `...process.env` plus cache vars, then calls `resolveSubprocessIdentity(env)` and passes only `uid`/`gid` to `spawn`; there is no `HOME` override. `apps/sidecar/src/lsp/server-process.ts:283-289` and `apps/sidecar/src/newt/newt-manager.ts:214-220` do the same. Since `resolveSubprocessIdentity()` only returns `{ username, uid, gid }`, the effective home directory remains whatever the root supervisor had in `process.env`.",
"file": "apps/sidecar/src/terminal/pty-manager.ts",
"line": 154,
"severity": "medium",
"title": "Demoted PTY/LSP/Newt subprocesses still inherit the supervisor `HOME`"
},
{
"body": "In `NewtManager.start()`, the env merge order is `...process.env, ...getAgentCacheEnvironment(workspaceRoot), ...env`. The `env` parameter defaults to `process.env` (line 168) and is `{ ...process.env }` when called from `configure()` (line 157) or restart (line 396). Since the sidecar's `process.env` contains supervisor cache vars (set by `sidecar-bootstrap.ts`), the final `...env` spread re-applies them _after_ `getAgentCacheEnvironment()` has overridden them. This means Newt subprocesses will still inherit supervisor cache paths (e.g., `NPM_CONFIG_CACHE=/home/agent/.sidecar/state/supervisor-cache/.cache/npm`), defeating the fix. The other three launchers (PTY, LSP, OpenCode) don't have this problem because their final spread is either a partial env (specific caller overrides, not a full process.env copy) or absent entirely.",
"category": "correctness",
"confidence": "high",
"evidence": "Line 168: `start(env: Env = process.env)` \u2014 env defaults to process.env.\nLines 214-218:\n```\nconst spawnEnv = {\n ...process.env,\n ...getAgentCacheEnvironment(workspaceRoot),\n ...env, // re-applies process.env including supervisor cache vars\n};\n```\nConfigure path (line 157): `const nextEnv = { ...process.env }` \u2192 passed to start().\nRestart path (line 396): `const restartEnv = this.#lastEnv ? { ...this.#lastEnv } : process.env` \u2192 passed to start().",
"file": "apps/sidecar/src/newt/newt-manager.ts",
"line": 217,
"severity": "high",
"title": "Newt manager env spread undoes agent cache fix"
},
{
"body": "PTY and OpenCode both have tests verifying that supervisor cache vars in `process.env` are overridden with agent-owned paths in the spawn env. Newt has no equivalent test, which allowed the bug above to ship. Given that Newt is one of four subprocess launchers that all need the same fix, the missing test is a meaningful coverage gap.",
"category": "testing",
"confidence": "high",
"evidence": "Changed test files include `apps/sidecar/tests/unit/pty-manager.test.ts` (PTY) and `packages/sdk-provider-opencode/tests/server-process-user.test.ts` (OpenCode), both asserting agent cache paths override supervisor paths. No corresponding test exists for Newt.",
"file": "apps/sidecar/src/newt/newt-manager.ts",
"line": 214,
"severity": "medium",
"title": "No test coverage for Newt cache env isolation"
},
{
"body": "The new ownership change makes the cargo config write depend on `docker exec` succeeding as `agent`, but both start paths still swallow failures. In `apps/host-agent/src/routes/runtime.ts`, any exception from `container.exec(...)` or `exec.start(...)` is caught and only logged, and even `inspect().ExitCode !== 0` only emits a warning before the route continues to `startSidecarServer()` and returns 200. `apps/orchestrator/src/driver/docker.ts:762-803` has the same behavior. This matters because the change increases the chance of bootstrap failure on images where the `agent` user is absent or lacks write access to `$CARGO_HOME`; those cases now look like a successful start while silently skipping the proxy config the PR is trying to enforce.",
"category": "operational",
"confidence": "high",
"evidence": "`const exec = await container.exec({ ..., User: \"agent\", ... }); ... if (inspection.ExitCode !== 0) { logger.warn(\"Cargo config write exited with non-zero code\", ...) } ... } catch (err) { logger.warn(\"Failed to write cargo config for Kellnr proxy\", ...) }`",
"file": "apps/host-agent/src/routes/runtime.ts",
"line": 679,
"severity": "medium",
"title": "Cargo config bootstrap still fails silently when the `agent` exec cannot run"
},
{
"body": "Both `docker.ts:798-803` and `runtime.ts:705-709` catch exec failures and log a warning, then proceed to start the sidecar and return success. If the `agent` user doesn't exist in the container image (e.g., a custom image without the expected user), the exec will fail with a Docker API error. The caller has no indication that cargo proxy configuration was not applied \u2014 the container starts without registry source replacement, and Rust builds will silently pull from crates.io directly instead of the Kellnr proxy. This pre-dates the PR, but the ownership change makes it more likely to trigger: previously running as root would succeed even with wrong permissions, now running as `agent` adds a new failure mode (user not found). The repo's CLAUDE.md explicitly states 'NO FALLBACKS: Never implement silent fallback modes that fake success' \u2014 this pattern contradicts that policy.",
"category": "operational",
"confidence": "medium",
"evidence": "catch (err) {\n logger.warn(\"Failed to write cargo config for Kellnr proxy\", {\n containerId: id.substring(0, 12),\n error: err instanceof Error ? err.message : String(err),\n });\n}",
"file": "apps/orchestrator/src/driver/docker.ts",
"line": 798,
"severity": "medium",
"title": "Cargo config write failures are silently swallowed in both paths"
},
{
"body": "The new test at `docker-driver-startsidecar.test.ts:94-105` only asserts that `mockContainer.exec` was called with `User: \"agent\"`. It doesn't test what happens when the exec returns a non-zero exit code (e.g., `agent` user can't write to `$CARGO_HOME`), nor when the exec itself throws (user doesn't exist). The mock always returns `ExitCode: 0`. Given that the whole point of this change is fixing a permissions issue, a test exercising the permission-failure path would be valuable \u2014 it would catch any future regression where the warn-and-continue behavior is accidentally changed to throw (or vice versa).",
"category": "testing",
"confidence": "medium",
"evidence": "it(\"writes cargo proxy config as the agent user\", async () => {\n process.env.CONTAINER_CARGO_REGISTRY =\n \"http://host.docker.internal:8000/api/v1/cratesio/\";\n\n await driver.startContainer(\"container-id\");\n\n expect(mockContainer.exec).toHaveBeenCalledWith(\n expect.objectContaining({\n User: \"agent\",\n }),\n );\n });",
"file": "apps/orchestrator/tests/unit/docker-driver-startsidecar.test.ts",
"line": 94,
"severity": "low",
"title": "Test does not verify exit-code or error-path behavior for agent-user exec"
}
],
"coverage_gaps": [],
"coverage_score": 100,
"evidence_score": 100,
"recommendation": "needs-work",
"rejected_findings": [],
"signal_score": 100,
"status": "fail"
}
}