fix(runner): narrow the runner environment and drop subscription auto-upload#5298
Conversation
dosubot
Bot
added
the
size:L
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
hosting/kubernetes/helm/templates/runner-deployment.yaml (1)
110-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove
optional: truefor consistency and fail-fast behavior.In
_helpers.tpl(which generatesservicesEnv),AGENTA_RUNNER_TOKENis correctly injected withoutoptional: true. Keeping it optional here introduces an inconsistency and allows the container to attempt startup when the secret is missing. Since the runner application validates this token asnonEmptydownstream, removingoptional: trueensures Kubernetes catches the missing secret early (via aCreateContainerConfigError), rather than letting the application crash at runtime.♻️ Proposed refactor
{{- if $auth.tokenSecretRef }} - name: AGENTA_RUNNER_TOKEN valueFrom: secretKeyRef: name: {{ $auth.tokenSecretRef.name | quote }} key: {{ $auth.tokenSecretRef.key | quote }} - optional: true {{- end }}services/runner/tests/unit/sandbox-agent-run-plan.test.ts (1)
1149-1174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing
createLocalCwdstub causes real filesystem side effects.Both new success-path tests reach
ok: trueand therefore exercise the realdefaultLocalCwd, unlike every other passing-path test in this file which injects acreateLocalCwdstub (e.g.createLocalCwd: () => "/tmp/local-cwd"). This creates unmocked filesystem side effects during the unit test run.🧪 Suggested fix
it("accepts a local Pi runtime_provided run when PI_CODING_AGENT_DIR names a mount", () => { withEnv({ PI_CODING_AGENT_DIR: "/agenta/harness/pi" }, () => { - const result = buildRunPlan({ - harness: "pi_core", - sandbox: "local", - messages: [{ role: "user", content: "hello" }], - credentialMode: "runtime_provided", - }); + const result = buildRunPlan( + { + harness: "pi_core", + sandbox: "local", + messages: [{ role: "user", content: "hello" }], + credentialMode: "runtime_provided", + }, + { createLocalCwd: () => "/tmp/local-cwd" }, + ); assert.equal(result.ok, true);Apply the same pattern to the
env-mode test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5452c811-3729-4093-bee0-3daf07653596
📒 Files selected for processing (17)
.github/workflows/12-check-unit-tests.ymlhosting/docker-compose/ee/docker-compose.gh.ymlhosting/docker-compose/oss/docker-compose.gh.ymlhosting/kubernetes/helm/templates/_helpers.tplhosting/kubernetes/helm/templates/runner-deployment.yamlhosting/kubernetes/helm/tests/test_runner_secret_absence.pyhosting/kubernetes/helm/values.schema.jsonhosting/kubernetes/helm/values.yamlservices/runner/src/engines/sandbox_agent.tsservices/runner/src/engines/sandbox_agent/daytona.tsservices/runner/src/engines/sandbox_agent/mount.tsservices/runner/src/engines/sandbox_agent/pi-assets.tsservices/runner/src/engines/sandbox_agent/run-plan.tsservices/runner/src/tracing/otel.tsservices/runner/tests/unit/sandbox-agent-daytona.test.tsservices/runner/tests/unit/sandbox-agent-orchestration.test.tsservices/runner/tests/unit/sandbox-agent-run-plan.test.ts
| # === SUBSCRIPTION MOUNTS (opt-in) ========================= # | ||
| # Use your own harness subscription for LOCAL runs instead of a managed API | ||
| # key. Mount the login read-only; the runner copies it into a per-run dir, so | ||
| # the harness's own writes never touch your source. Single-operator convenience | ||
| # only — this state is never shipped to a Daytona sandbox. A runtime_provided | ||
| # local run without the matching mount fails with a clear error. | ||
| # volumes: | ||
| # - ~/.pi/agent:/agenta/harness/pi:ro # Pi / Codex login | ||
| # - ~/.claude:/agenta/harness/claude:ro # Claude login | ||
| # Then point the harness config var at the mount (override the defaults above): | ||
| # PI_CODING_AGENT_DIR: /agenta/harness/pi | ||
| # CLAUDE_CONFIG_DIR: /agenta/harness/claude |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add CLAUDE_CONFIG_DIR to the environment block.
The comment instructs users to "override the defaults above" for both PI_CODING_AGENT_DIR and CLAUDE_CONFIG_DIR. However, unlike PI_CODING_AGENT_DIR, CLAUDE_CONFIG_DIR is missing from the environment section above. Since the runner service lacks an env_file directive, Docker Compose will drop this variable if set on the host environment or a global .env file, breaking the documented feature.
💻 Proposed fix
Declare CLAUDE_CONFIG_DIR in the environment section of the runner service (around line 293):
AGENTA_RUNNER_DEFAULT_SANDBOX_PROVIDER: ${AGENTA_RUNNER_DEFAULT_SANDBOX_PROVIDER:-local}
PI_CODING_AGENT_DIR: ${PI_CODING_AGENT_DIR:-/pi-agent}
+ CLAUDE_CONFIG_DIR: ${CLAUDE_CONFIG_DIR:-}
AGENTA_RUNNER_DAYTONA_API_KEY: ${AGENTA_RUNNER_DAYTONA_API_KEY:-}| // Local Claude subscription run: copy the mounted CLAUDE_CONFIG_DIR into a per-run dir and point | ||
| // the daemon at the copy, so the harness's own writes stay off the read-only source mount | ||
| // (interface.md section 6). buildRunPlan already rejected a runtime_provided Claude run with no | ||
| // configured CLAUDE_CONFIG_DIR, so the source here is the mount. | ||
| if ( | ||
| !plan.isDaytona && | ||
| plan.acpAgent === "claude" && | ||
| plan.credentialMode === "runtime_provided" | ||
| ) { | ||
| const runClaudeConfigDir = prepareLocalClaudeConfigDir( | ||
| process.env.CLAUDE_CONFIG_DIR, | ||
| logger, | ||
| ); | ||
| if (runClaudeConfigDir) env.CLAUDE_CONFIG_DIR = runClaudeConfigDir; | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Prevent temporary directory and credential leak.
The temporary directory created by prepareLocalClaudeConfigDir is stored in a block-scoped const (runClaudeConfigDir). Because it is block-scoped and never attached to the environment object, it is completely inaccessible to the environment.destroy cleanup function. This results in permanently leaking a temporary directory (which contains sensitive Claude configuration/credentials) in the runner's /tmp on every local runtime_provided Claude run.
Lift the variable declaration so it can be captured by the destroy closure, and clean it up during teardown.
🔒️ Proposed fix to ensure cleanup
Declare the variable before the block (around line 799):
const binaryPath = (deps.resolveDaemonBinary ?? resolveDaemonBinary)();
let runAgentDir = prepareLocalPiAssets({ plan, env, log: logger });
+ let runClaudeConfigDir: string | undefined;
// Local Claude subscription run: copy the mounted CLAUDE_CONFIG_DIR into a per-run dir and point
// the daemon at the copy, so the harness's own writes stay off the read-only source mount
// (interface.md section 6). buildRunPlan already rejected a runtime_provided Claude run with no
// configured CLAUDE_CONFIG_DIR, so the source here is the mount.
if (
!plan.isDaytona &&
plan.acpAgent === "claude" &&
plan.credentialMode === "runtime_provided"
) {
- const runClaudeConfigDir = prepareLocalClaudeConfigDir(
+ runClaudeConfigDir = prepareLocalClaudeConfigDir(
process.env.CLAUDE_CONFIG_DIR,
logger,
);
if (runClaudeConfigDir) env.CLAUDE_CONFIG_DIR = runClaudeConfigDir;
}Then, add the cleanup step inside the environment.destroy function (around line 964):
// The per-run Agenta agent dir (skills isolation) is throwaway; remove it too.
if (environment.runAgentDir)
rmSync(environment.runAgentDir, { recursive: true, force: true });
+ if (runClaudeConfigDir)
+ rmSync(runClaudeConfigDir, { recursive: true, force: true });
1404fcc to
66b0f17
Compare
|
🌙 Overnight — reviewed + backend-QA'd (21/21 on 8280), but the merge is blocked by a real 2-file code conflict with the already-merged pi-skill work: |
…-upload The runner discovered its own Pi login (auth.json + settings) and uploaded it into a third-party Daytona sandbox for any self-managed run. A personal subscription credential must never leave the runner container, and a local harness that shares the runner container must not be able to read a static platform API key from the runner's process environment. Phases 2+3 of runner-selfhosting-cleanup: - Delete shouldUploadOwnLogin + uploadPiAuthToSandbox and their tests. Daytona never receives a personal login; managed Daytona runs authenticate from the vault keys instead. - buildRunPlan rejects Daytona + runtime_provided, and rejects a local runtime_provided run whose subscription mount (PI_CODING_AGENT_DIR / CLAUDE_CONFIG_DIR) is unset, before any sandbox side effect. - Local runtime_provided Pi and Claude always copy the read-only mounted login into a per-run dir so harness writes never touch the operator's source. - Remove the static AGENTA_API_KEY OTLP exporter fallback; export auth rides the per-run caller credential. - One structured "mount degraded kind=<k> cause=<c>" warning at each durable-null site (session cwd, agent mount, harness transcript). Failure behavior unchanged. - Helm: wire AGENTA_RUNNER_TOKEN via agentRunner.auth.tokenSecretRef on both the runner (verifier) and Services (sender), guarded so it renders only when set, plus a rendered-chart secret-absence test wired into CI. - Compose: commented opt-in read-only subscription mount examples (OSS + EE). Deferred (post-release): the credentialMode strictness flip is NOT free on this base. A model-less agent template in the SDK handler threads no resolved_connection and omits the field (see status.md). This slice keeps the existing inference branch; the flip lands after the caller fix. AGENTA_RUNNER_INHERIT_ALL_PROVIDER_KEYS, commonEnv, and AGENTA_SESSION_HARNESS_MOUNTS are already no-ops on this base. Claude-Session: https://claude.ai/code/session_01XhENr63WL9npkKrJGnzDc1
… a per-run copy A local runtime_provided run authenticated from the operator's mounted harness login, but the runner copied that login into a throwaway per-run directory and pointed the harness at the copy. Subscription logins are OAuth logins: the harness refreshes its access token mid-run and writes the new one back to its config dir. The copy discarded that refresh, so as soon as the provider rotated the refresh token the next run failed and the operator had to log in by hand. Mount the login read-write and let the harness run directly out of it, exactly as it does on a normal local install, so a refreshed token persists to the source. Drops prepareLocalClaudeConfigDir and the Pi credential copy; the dev Compose stacks stop cp -a'ing the login into a container-local path. The subscription path deliberately reports NO throwaway dir, so teardown's rmSync can never delete the operator's real login. Pinned by a test. Tradeoff, now documented: concurrent local subscription runs share the harness config dir, the same way two local harness sessions do. This path is single-trusted-operator only. The missing-mount guard is unchanged: a local runtime_provided run with no configured PI_CODING_AGENT_DIR / CLAUDE_CONFIG_DIR still fails up front.
325ef16 to
07cffc7
Compare
1 participant
The problem
For any self-managed run, the runner discovered its own personal Pi login (
auth.json+ settings) and uploaded it into a third-party Daytona sandbox. A personal subscription credential must never leave the runner container. Separately, a local harness that shares the runner container could read a static platform API key straight out of the runner's process environment.What changed
Phases 2 + 3 of the runner-selfhosting-cleanup:
shouldUploadOwnLogin+uploadPiAuthToSandboxand their tests. Daytona never receives a personal login; managed Daytona runs authenticate from the vault keys instead.buildRunPlannow rejects Daytona +runtime_provided, and rejects a localruntime_providedrun whose subscription mount (PI_CODING_AGENT_DIR/CLAUDE_CONFIG_DIR) is unset — before any sandbox side effect.runtime_providedPi and Claude runs always copy the read-only mounted login into a per-run dir, so harness writes never touch the operator's source mount.AGENTA_API_KEYOTLP exporter fallback; export auth now rides the per-run caller credential.mount degraded kind=<k> cause=<c>warning at each durable-null site (session cwd, agent mount, harness transcript). Failure behavior is unchanged.AGENTA_RUNNER_TOKENviaagentRunner.auth.tokenSecretRefon both the runner (verifier) and Services (sender), guarded so it renders only when set, plus a rendered-chart secret-absence test wired into CI.Deferred (post-release)
The
credentialModestrictness flip is not free on this base. A model-less agent template in the SDK handler threads noresolved_connectionand omits the field, so this slice keeps the existing inference branch; the flip lands after the caller fix.AGENTA_RUNNER_INHERIT_ALL_PROVIDER_KEYS,commonEnv, andAGENTA_SESSION_HARNESS_MOUNTSare already no-ops on this base.Tests
helm lintclean + the new rendered-chart secret-absence test passes.Note on
status.mdThis PR carries the 17 code + hosting files. The project's
status.mdupdate (Phase 2+3 landed marker + the deferred-flip note) could not be included here: its base version lives only in thedocs/runner-selfhosting-cleanup-planlane, and this lane is anchored onrefactor/runner-config-names(which does not contain the file), so GitButler attributes the file's creation to the docs lane and cannot land the delta here without unsafe surgery. The status update should ride the docs-plan lane.https://claude.ai/code/session_01XhENr63WL9npkKrJGnzDc1