docs(agent-workflows): plan runner self-hosting cleanup#5288
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ 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 |
|
|
||
| | Variable | Role | Default | | ||
| |---|---|---| | ||
| | `AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS` | deployment capability configuration | `local` | |
There was a problem hiding this comment.
🤖 The AI agent says: Decision: provider naming and safe defaults. I prefer ENABLED_SANDBOX_PROVIDERS over an “allowlist” because this describes deployment capability, not an authorization exception. Unset means local only, so adding Docker or E2B to a future binary cannot silently enable it in existing installations. Please use this thread for alternate names or default semantics.
|
|
||
| ## 6. Bootstrap manifest | ||
|
|
||
| Bootstrap assets describe files or directories the operator intentionally makes available to a run. The manifest is mounted read-only and selected through `AGENTA_RUNNER_BOOTSTRAP_CONFIG`. |
There was a problem hiding this comment.
🤖 The AI agent says: Decision: bootstrap version 1 is declarative data, not executable hooks. A mounted manifest gives local and Daytona the same typed copy contract, keeps operator sources read-only, and lets us validate paths, modes, size, and redaction. If scripts or VPN hooks are required for the first version, we should define their execution and secret boundary here before implementation.
There was a problem hiding this comment.
The idea is not bad. We decided not to go with an extension or something. At the end of the day, it's kind of the runners, as in the local hosters, who are going to add these, so it's their responsibility. I'm not sure if these are as extensible and as easy to use and to implement as we want them to be. It looks overly complicated for something I'm not sure we need even now.
Even if we are not talking about hooks, I don't know what other solution. What's the problem that you're trying to solve here? Really, let's be clear about that problem. Otherwise, let's keep it just simple. I don't know, but I'd like to revisit this. Simplicity is very important in the implementation for this PR.
There was a problem hiding this comment.
🤖 Review agent says: The problem this manifest solves is real but small: (a) stop the runner from auto-discovering its own Pi login and uploading it to Daytona, and (b) give a local operator a declared way to use their own subscription. Neither needs a manifest engine. Version 1 proposal: delete shouldUploadOwnLogin and uploadPiAuthToSandbox, and for local subscriptions document a plain read-only volume mount in Compose (mount the credential directory, point PI_CODING_AGENT_DIR / CLAUDE_CONFIG_DIR at it). No YAML schema, no validation engine, no AGENTA_RUNNER_BOOTSTRAP_CONFIG variable. The company-CA / runtime-config use case has no user demand yet; it belongs with RSH-4. This removes section 6 here and most of phase 4 in plan.md, and it matches how operators already customize the rest of the stack: by editing the Compose file they own.
|
|
||
| - The local adapter creates a unique per-run harness configuration root, copies applicable assets, sets `PI_CODING_AGENT_DIR` or `CLAUDE_CONFIG_DIR`, and deletes the copy at teardown. | ||
| - The Daytona adapter uploads applicable non-auth assets before daemon start and applies the same logical destination mapping. | ||
| - Version 1 rejects `purpose: harness-auth` for Daytona. |
There was a problem hiding this comment.
🤖 The AI agent says: Decision: no remote subscription authentication in version 1. This removes the current automatic Pi login upload. Daytona bootstraps still support non-auth runtime files, but a harness-auth asset is local-only until OpenAI or Anthropic confirms the third-party remote execution model and we design it explicitly.
| 5. transport-disconnected recovery gets one bounded remount attempt, then fails; | ||
| 6. no plain directory is substituted at the durable path. | ||
|
|
||
| Errors name the failed mount kind and remediation category without exposing credentials. There is no public environment switch to disable transcript mounts. |
There was a problem hiding this comment.
🤖 The AI agent says: Decision: persistence assertions fail loudly. A session id, workflow artifact, or resumable harness session promises durable state, so a signing or mount failure should fail the run. Only a truly sessionless invocation may degrade to an ephemeral cwd. This intentionally replaces the current best-effort fallback.
There was a problem hiding this comment.
Yeah, I don't agree. I don't think we should have it so hard on one hand, and I'm not sure if it's in scope. Obviously, if there's something really problematic, like a signing failure, yes, I agree GEC if it's not working. I would really not have it that hard.
Otherwise, we cannot release, right? The goal is obviously to clean up things and clean up the documentation so people can self-host it, release, and then iteratively improve, not add conditions that might break our implementation for the moment because of fighting millisecond issues or whatever. For example, had I think something with the GFC, if this mount or whatever, and then at some point there was some small race condition that broke everything.
Please think about that and review it, but I would add only what is needed to scope here. That was not the goal of the cleanup, right? Here, you're thinking really about the logic.
There was a problem hiding this comment.
🤖 Review agent says: Agree with the owner comment here: this is a behavior rewrite, not cleanup, and it is out of scope for the release. Rewriting mount semantics the night before a code freeze risks exactly the race-condition class that best-effort currently papers over. Version 1 proposal: keep the current best-effort behavior, add one structured warning log when a durable mount degrades to an ephemeral directory (so we can measure how often it actually happens), and move the fail-loud contract to a new open-issues entry. Keep only the cheap config-time startup validation from section 3, which fails before anything runs and cannot break a live run. This removes phase 5 from the release path.
|
|
||
| ### No Helm common environment | ||
|
|
||
| The PR includes `agenta.commonEnv` in the runner deployment. That block contains application license, Postgres password and URIs, Agenta auth and cryptographic keys, Redis credentials, store master credentials, and unrelated service configuration. |
There was a problem hiding this comment.
🤖 The AI agent says: Decision on PR #5286: keep the runner token and provider-key narrowing, but do not inject agenta.commonEnv. A local harness can inspect the runner process through /proc, so that Helm include exposes database, auth, cryptographic, Redis, and store-master secrets. The proposal also removes the static runner AGENTA_API_KEY; callbacks and traces use the credential already scoped to each request.
|
|
||
| No credential value, bootstrap content, or local source path is logged. | ||
|
|
||
| ## 4. Provider capability discovery |
There was a problem hiding this comment.
This one looks very tricky to implement because now you're expecting the runner to provide capabilities, the service to fetch those capabilities, and then to show them to the frontend. Yes, it's the right way, probably, to do it, but it feels like it adds a lot of complexity into a codebase that still needs a lot of refactoring. I'm not sure about this. Would you go into either a redundant environment variable? Are we using that environment variable for some of us?
There was a problem hiding this comment.
unless i am missing something about complexity
There was a problem hiding this comment.
🤖 Review agent says: Agree, cut /capabilities from version 1. To answer the question in this thread: yes, the redundant environment variable already exists. The API gates sandbox availability today through AGENTA_SANDBOX_LOCAL_ALLOWED in api/oss/src/utils/env.py, and the SDK and services read it too. So the pragmatic v1 shape is: keep that pattern but rename it to the canonical value (AGENTA_RUNNER_ENABLED_SANDBOX_PROVIDERS), and have Compose and Helm feed both the API and the runner from one operator-facing entry in the env file. Drift then requires deliberate effort. The runner stays the final authority and rejects a disabled provider with a clear error, which is honest enough for an MVP. Park the /capabilities endpoint in RSH-7 for when multi-runner or heterogeneous deployments make one shared value insufficient. This deletes phase 2 from the release path.
🤖 Review agent: inline commentsPosted as one PR-level comment because a pending review on this PR blocks inline review comments via the API. Intended anchors noted per item. Reply to this thread 🤖 Review agent says: Agree, cut
🤖 Review agent says: The problem this manifest solves is real but small: (a) stop the runner from auto-discovering its own Pi login and uploading it to Daytona, and (b) give a local operator a declared way to use their own subscription. Neither needs a manifest engine. Version 1 proposal: delete
🤖 Review agent says: Alignment check on customization: the operator owns it through scripts and images, not through runner environment machinery. Daytona: we ship and document the snapshot build scripts that already exist at
🤖 Review agent says: Agree with the owner comment here: this is a behavior rewrite, not cleanup, and it is out of scope for the release. Rewriting mount semantics the night before a code freeze risks exactly the race-condition class that best-effort currently papers over. Version 1 proposal: keep the current best-effort behavior, add one structured warning log when a durable mount degrades to an ephemeral directory (so we can measure how often it actually happens), and move the fail-loud contract to a new open-issues entry. Keep only the cheap config-time startup validation from section 3, which fails before anything runs and cannot break a live run. This removes phase 5 from the release path.
🤖 Review agent says: Release-risk flag on making
🤖 Review agent says: Proposed re-scope for the MVP release. Ship: phase 0 (QA repair), phase 1 (canonical names, typed runner config, startup validation; this is the open interface we cannot change after release), a slim phase 3 (keep the runner token and key narrowing, drop
🤖 Review agent says: Two gaps in this phase. First, the plan has no workstream for making the hosting tree itself self-explanatory, and that is a release requirement. Add a docs-only slice: a
🤖 Review agent says: The Diátaxis split is right but the plan is incomplete against the release goals. (1) It never reconciles with the existing |
🤖 Review agent: overall assessmentThis design gets the important thing right: one runner-owned namespace ( But the plan wraps that cleanup in three engineering subsystems that are not needed to release, and each one is new logic with new failure modes the night before a code freeze:
Two things the plan is missing that the release does need:
Proposed release scope: ship phase 0, phase 1, slim phase 3 (token + narrowing, minus the credentialMode strictness flip unless a caller audit proves it free), docs + hosting README. Defer phases 2, 4, 5 as described in the inline threads. Roughly 4 PRs, largely parallelizable. Inline comments on the specific sections carry the details. |
|
|
||
| No credential value, bootstrap content, or local source path is logged. | ||
|
|
||
| ## 4. Provider capability discovery |
There was a problem hiding this comment.
unless i am missing something about complexity
|
|
||
| ## 6. Bootstrap manifest | ||
|
|
||
| Bootstrap assets describe files or directories the operator intentionally makes available to a run. The manifest is mounted read-only and selected through `AGENTA_RUNNER_BOOTSTRAP_CONFIG`. |
There was a problem hiding this comment.
The idea is not bad. We decided not to go with an extension or something. At the end of the day, it's kind of the runners, as in the local hosters, who are going to add these, so it's their responsibility. I'm not sure if these are as extensible and as easy to use and to implement as we want them to be. It looks overly complicated for something I'm not sure we need even now.
Even if we are not talking about hooks, I don't know what other solution. What's the problem that you're trying to solve here? Really, let's be clear about that problem. Otherwise, let's keep it just simple. I don't know, but I'd like to revisit this. Simplicity is very important in the implementation for this PR.
| - Asset content and credential filenames are redacted from traces and normal logs. | ||
| - Version 1 does not execute scripts. Hooks, plugins, and VPN setup remain future work. | ||
|
|
||
| ## 7. Harness installation |
There was a problem hiding this comment.
Yeah, I'm sure you mean this. The way I see it is that, when we're talking about the daytona snapshot or whatever, we provide the scripts that allow you to create these images, upload them, and do whatever you need to do with them to kind of set yourself up for success.
For the runner, we have the docker compose. You can update it if you want to add something. I don't know, maybe you want to add other dependencies. It's your thing for the runner, so I'm not sure we need an environment variable at all. We just need scripts to kind of allow this, which I think is what you're saying, right? Just to make sure we're aligned
There was a problem hiding this comment.
🤖 Review agent says: Alignment check on customization: the operator owns it through scripts and images, not through runner environment machinery. Daytona: we ship and document the snapshot build scripts that already exist at services/runner/sandbox-images/daytona/ (create the snapshot, upload it, set AGENTA_RUNNER_DAYTONA_SNAPSHOT). Local: the operator edits their Compose file to mount extra folders, or builds a custom image from services/runner/docker/Dockerfile.gh when they need extra dependencies (chromium, gh, certificates). The only environment variables involved are the image and snapshot references that already exist. Everything else is documentation, not contract.
| 5. transport-disconnected recovery gets one bounded remount attempt, then fails; | ||
| 6. no plain directory is substituted at the durable path. | ||
|
|
||
| Errors name the failed mount kind and remediation category without exposing credentials. There is no public environment switch to disable transcript mounts. |
There was a problem hiding this comment.
Yeah, I don't agree. I don't think we should have it so hard on one hand, and I'm not sure if it's in scope. Obviously, if there's something really problematic, like a signing failure, yes, I agree GEC if it's not working. I would really not have it that hard.
Otherwise, we cannot release, right? The goal is obviously to clean up things and clean up the documentation so people can self-host it, release, and then iteratively improve, not add conditions that might break our implementation for the moment because of fighting millisecond issues or whatever. For example, had I think something with the GFC, if this mount or whatever, and then at some point there was some small race condition that broke everything.
Please think about that and review it, but I would add only what is needed to scope here. That was not the goal of the cleanup, right? Here, you're thinking really about the logic.
| | `runtime_provided` | the harness authenticates from explicitly prepared runtime state | local bootstrap asset | | ||
| | `none` | the run intentionally has no model credential | no credential | | ||
|
|
||
| Provider and deployment metadata required for model-key narrowing are also required. Missing or unknown values fail contract validation. There is no legacy "infer from hasApiKey" branch and no inherit-all escape hatch. |
There was a problem hiding this comment.
🤖 Review agent says: Release-risk flag on making credentialMode, provider, and deployment hard-required with no inference branch: any caller that does not send them yet starts failing contract validation. Before adopting this, audit what the API and SDK actually send today on every run path (playground, workflows, evaluations, SDK local). If every in-repo caller already sends them, the flip is free and correct. If any caller omits them, fix that caller in the same PR or keep a narrow default for the missing field, and do the strictness flip after the release. Same risk category as the mount rework: correctness hardening that can turn a working demo into a failing one.
| - The support matrix matches automated QA. | ||
| - The docs do not imply local isolation or remote subscription support. | ||
|
|
||
| ## Proposed PR stack |
There was a problem hiding this comment.
🤖 Review agent says: Proposed re-scope for the MVP release. Ship: phase 0 (QA repair), phase 1 (canonical names, typed runner config, startup validation; this is the open interface we cannot change after release), a slim phase 3 (keep the runner token and key narrowing, drop commonEnv, drop the inherit-all flag, but see the credentialMode strictness flag on interface.md section 5), and the docs plus hosting layout work. Defer: phase 2 (capability endpoint; shared env var instead, see the section 4 thread), phase 4 (bootstrap manifest; documented volume mounts instead, see the section 6 thread), phase 5 (fail-loud mounts; warning log instead, see the section 8 thread). That collapses the 7-PR stack to roughly 4 PRs: (1) QA repair, (2) rename plus typed config plus validation across code, Compose, Helm, Railway, (3) runner environment narrowing, (4) docs plus hosting README. Slices 2, 3, and 4 touch mostly disjoint files and can be built in parallel.
| - Store and FUSE failures surface a remediation-oriented error. | ||
| - Recovery tests prove one remount attempt and then failure. | ||
|
|
||
| ## Phase 6: public documentation |
There was a problem hiding this comment.
🤖 Review agent says: Two gaps in this phase. First, the plan has no workstream for making the hosting tree itself self-explanatory, and that is a release requirement. Add a docs-only slice: a hosting/README.md that maps the layout (Compose variants per edition and per image mode, Helm, Railway, the runner image built from services/runner/docker/, the Daytona snapshot built from services/runner/sandbox-images/daytona/), plus a commented entry in every env.*.example for each canonical runner variable, plus a final grep check that no removed name survives anywhere under hosting/ or docs/. Second, sequencing: 'docs land after all phases' does not survive a release tomorrow. The only real prerequisite for the docs is the phase 1 rename. Land the docs with or immediately after that PR. Only the troubleshooting error strings genuinely depend on later implementation; add those entries as the code lands.
| @@ -0,0 +1,169 @@ | |||
| # Documentation plan | |||
|
|
|||
| Public documentation lands after the runtime and hosting contract is stable. The pages follow Diátaxis so readers do not have to extract a tutorial from a configuration reference. | |||
There was a problem hiding this comment.
🤖 Review agent says: The Diátaxis split is right but the plan is incomplete against the release goals. (1) It never reconciles with the existing docs/docs/self-host/ tree: quick-start, configuration, guides 07 (deploy the runner), 08 (custom images), and 09 (Daytona) already exist. Say explicitly which existing pages are replaced, which are rewritten, and where their URLs redirect; otherwise we ship two parallel doc sets. (2) Missing a security and trust concept page: local runs share the runner container and are not an isolation boundary; use Daytona when you need one. The content already exists in context.md and needs a public home, because self-hosters on Reddit will ask this first. (3) Missing how-tos people will actually search for: mount extra folders into local agent runs; add dependencies (chromium, gh CLI, certificates) via a custom runner image; use your own Claude or ChatGPT subscription locally (make Codex/ChatGPT explicit, not just Pi); and a short extension-points page saying another sandbox provider or harness is a feature request with a link to open an issue. (4) The runner reference page should note it is generated from or checked against the typed config so it cannot drift.
🤖 Review agent: v2 pushed, ready for second-round reviewCommit 1472d38 adopts every round-1 decision. What changed per file:
Round-2 questions for the owner are in status.md: approve the re-scope as a whole, the docs dispositions, and the conditional strictness flip. |
mmabrouk
force-pushed
the
docs/runner-selfhosting-cleanup-plan
branch
2 times, most recently
from
e593343 to
50eaf55
Compare
Adopt owner review round 1: defer the capability endpoint, the bootstrap manifest, and fail-loud mounts; add the hosting-layout workstream and per-page docs dispositions. Claude-Session: https://claude.ai/code/session_01XhENr63WL9npkKrJGnzDc1
mmabrouk
force-pushed
the
docs/runner-selfhosting-cleanup-plan
branch
from
33b762e to
f061c2b
Compare
1 participant
Context
Runner self-hosting currently mixes runner routing, sandbox selection, Daytona configuration, runtime facts, subscription credentials, and mount behavior across overlapping environment variables. The local 8280 failure made the result concrete: Services reached a subscription sidecar without Daytona credentials, so a valid Daytona request failed before provisioning.
PR #5274 exposed immediate snapshot and mount follow-ups. PR #5285 proposes a provider allowlist and future Docker provider. PR #5286 adds useful runner authentication and provider-key narrowing, but also injects the broad Helm application environment into the runner.
Proposal
This docs-only PR defines a cleanup that:
commonEnv, static runner API credentials, and inherit-all fallbacks;Review guide
The most consequential choices are in:
interface.md: canonical names, ownership, capability discovery, bootstrap schema, and failure contracts.plan.md: implementation slices and reconciliation with [chore] Agent mounts followup and clean-up #5274, docs(design): plan docker sandbox provider + sandboxes allowlist #5285, and [fix] Land the v7 audit findings that need no product decision #5286.qa.md: supported and rejected sandbox, harness, and authentication combinations.documentation-plan.md: explanation, tutorial, how-to, reference, and troubleshooting pages.status.md: the complete owner decision checklist.I will add inline threads to the key decision lines so review can happen in place.
Scope
This PR changes design documentation only. It does not change runtime behavior, hosting configuration, or public docs.
Verification
runner-selfhosting-cleanup/.Related: #5274, #5285, #5286