Proposal: safer defaults for local runs and cloud handoffs #4

Activity

1. TheOnlyFusionCube commented on Oct 23, 2026

   

   TheOnlyFusionCube

   on Oct 23, 2026 – with ChatGPT Codex Connector

   Owner

   More actions

   Thanks @hessabi for the careful proposal, and sorry this sat unanswered. I reviewed the current main implementation. These are meaningful safety issues, but I'd like to separate clearly scoped fixes from decisions that change how this plugin is meant to work.

   Green light for separate PRs now:

   1. P0 — Exit non-zero when a Devin task or review fails #7 API endpoint validation: ACCEPT. cloud.mjs currently uses DEVIN_API_URL for bearer-authenticated create/get/message calls. Validate before any authenticated request. Production should use the exact https://api.devin.ai origin; if tests need loopback HTTP, make it explicitly test-only/opt-in, not a silent alternate destination for a live key. Reject userinfo, paths, query strings, fragments, and unexpected ports; disallow redirects on authenticated requests. Please test malicious URL variants and verify no credential-bearing request is made.
   2. P0 — Stop cloud sessions on cancel and read v3 pr_url #3 sandbox failure: ACCEPT. runDevinJob silently retries a failed sandboxed review in unsandboxed normal mode, using a broad regex over error output. Remove that retry and report a clear, actionable error instead. Devin's own sandbox documentation says an unavailable sandbox fails closed; Windows and missing Linux prerequisites should be covered in the diagnostics/tests. No implicit downgrade.
   3. P2 (small compatibility win) — Treat the CLI's first-run welcome screen as a failed run #8 Desktop CLI lookup: ACCEPT. Preserve explicit DEVIN_COMPANION_DEVIN_BINARY selection, then try devin on PATH, then the proposed macOS app-bundle path if executable; resolve/validate the fallback. Use the same resolved binary for setup, auth, session listing, and execution. Include a PATH-precedence test and a simulated Desktop-only test.

   I favor the security direction, but please hold implementation of these defaults until we settle the policy/UX:

   4. P1 — Use DEVIN_BINARY in setup so the binary override is honored #2 permission modes: MODIFY. I'd favor rejecting dangerous, yolo, and bypass in the agent-facing companion, including aliases and conflicting --read-only combinations. An environment-variable unlock is not a reliable human-approval boundary if an agent can launch shell commands and set that variable. Whether to provide a separate human-only escape hatch is a maintainer decision; please don't add an env-var bypass as part of a PR.
   5. P1 — Proposal: safer defaults for local runs and cloud handoffs #4 workspace trust: MODIFY. Unconditionally adding --respect-workspace-trust false is the wrong default for untrusted checkouts. But Devin CLI documents that noninteractive -p fails on an untrusted workspace when trust is respected. We should remove the unconditional bypass and give users useful trust-setup diagnostics; any explicit bypass for unattended/CI workflows needs a deliberate opt-in design, not another silent fallback.
   6. P1 — Allow overriding the default model and explain Pro-only failures #6 cloud handoff: MODIFY. Adding disable-model-invocation: true to the Claude slash command is a good defense in that surface, but does not restrict direct devin-companion.mjs handoff calls from other agents. I'd favor explicit approval before a billable handoff and separate approval/opt-in before sending the tracked, uncommitted diff (the current command attaches it automatically and it may contain secrets). Both the v1 and v3 session-create schemas support max_acu_limit; please validate a positive integer and include it on create. The default ACU budget, whether/how users can raise it, and the portable-CLI confirmation contract are not decided yet.
   7. P2 — Don't crash reading stdin when it is not a pipe or file #5 subprocess environment: MODIFY. Inheriting process.env is broader than necessary; the CLI documents DEVIN_MODEL, DEVIN_PERMISSION_MODE, and DEVIN_SANDBOX as configuration inputs. Start with blocking unintended mode/model/sandbox overrides across all subprocess calls (not just runDevinPrint). A strict general allowlist needs cross-platform testing so it doesn't break PATH, home/config directories, authentication, proxies, or certs. I would not merge an untested blanket allowlist.
   8. P3 — Fix auth status check treating 'Not logged in' as logged in #1 isolated worktrees: DEFER THE DEFAULT / REDESIGN FIRST. I like review-before-apply for write tasks, but mandatory worktrees plus /devin:apply are a substantial behavioral/API change. We need a design for dirty and untracked starting trees, branch naming/lifecycle, session resume and job state, reviewing and applying binary/conflicting changes, cancellation/cleanup, and safe --in-place semantics. Also, worktree separation is not an OS security sandbox. An opt-in prototype/design PR is more appropriate than changing /devin:rescue's default now.

   Suggested sequence: small independent PRs for #7, #3, and #8 first, each with focused regression tests and any README/AGENTS updates. Please hold policy-changing PRs for #2/#4/#6/#1 while their user-visible defaults are decided; for #5, a narrowly scoped env-precedence/test PR would be a useful first step. This is an initial scope review, not blanket approval of the unresolved defaults. Keeping this issue open for those decisions.
2. TheOnlyFusionCube commented on Oct 23, 2026

   

   TheOnlyFusionCube

   on Oct 23, 2026 – with ChatGPT Codex Connector

   Owner

   More actions

   @hessabi Follow-up: the maintainer has now resolved the three product defaults I left open in my earlier review. Please treat this as the updated direction for proposals 1 and 6 (the previously approved independent PRs for proposals 3, 7, and 8 remain welcome).

   Proposal 1 — isolated worktrees: ACCEPT the worktree-first default, in phases

   For new, write-capable local tasks (including /devin:rescue), use a plugin-managed Git worktree/branch by default, keep the caller's checkout unchanged, and provide an explicit review-and-apply step such as /devin:apply <job-id>. An explicit --in-place override may preserve the old behavior, but it must be clearly identified as less isolated. Read-only reviews should not require worktree creation.

   Please start with a short design PR or narrowly scoped foundation PR before the full default switch, covering:

   • Dirty/staged/untracked starting trees: how task context is carried into the worktree without silently moving or discarding local changes, or else a clear fail-with-guidance policy.
   • Worktree/branch location and safe naming; state persistence (job ID -> worktree, branch, base commit); resuming the right session and job; cancellation, cleanup, and recovery.
   • An apply command that previews the delta, requires explicit user action, handles conflicts/binary files safely, and never overwrites existing changes without consent. Keep the worktree available on apply failure.
   • Graceful handling of non-Git directories, compatibility for Codex/portable CLI users, plus regression tests and docs.

   A worktree protects the active checkout from accidental edits; it is not a security sandbox and should not weaken sandbox or permission controls. This is an approval of the default direction, not an instruction to merge an unreviewed large rewrite.

   Proposal 6 — cloud handoff: ACCEPT explicit approval + configurable ACU cap

   User approval is required before creating a billable cloud session. For Claude, add disable-model-invocation: true on /devin:handoff, but don't rely on that metadata alone: the shared CLI is also invoked by Codex and other agents. Propose a portable CLI interaction that requires an affirmative, informed user decision, behaves safely in noninteractive invocations (no silent auto-approval), and clearly surfaces the target task, destination, and cost cap. A command-line flag alone is not strong proof of a human decision if an agent can supply it; document the threat model and avoid claiming stronger guarantees than provided.

   Require separate explicit consent to attach the uncommitted tracked diff. Do not automatically include it with every handoff. A user must be able to launch without it. Show the scope/size and note that local changes can contain secrets; preserve the existing diff-size limit.

   ACU cap: configurable, with no hard-coded numeric default authorized. Read a documented user-level configuration or explicit per-invocation value, validate that max_acu_limit is a positive integer, and send it in the session-create payload for the applicable API version. If no cap is configured or supplied, stop with actionable setup guidance rather than starting an uncapped session. Show the cap before approval; require fresh approval if the cap changes. Include tests for missing/invalid caps, cancellation/denial, omitted diff, approved diff, and both v1/personal-key and v3/service-key payloads.

   PR sequencing: please continue with small security fixes first (proposals 7 and 3), then Desktop CLI discovery (8). The worktree foundation/design and cloud approval/budget work are now welcome as separate PRs with the above contracts; don't combine them with unrelated security fixes. The earlier qualifications for permission modes, workspace trust, and environment handling (proposals 2, 4, 5) still stand. Thanks again for offering to split these up.
3. hessabi commented on Oct 23, 2026

   

   hessabi

   on Oct 23, 2026

   ContributorAuthor

   More actions

   Thanks for the careful review. Splitting the scoped fixes from the policy calls makes sense, and the contracts for proposals 1 and 6 are clear enough to build against.

   Following your sequence, these are up as separate PRs off the current main:

   • fix: validate DEVIN_API_URL and disallow redirects on authenticated requests #11, proposal 7: DEVIN_API_URL is validated before any authenticated request, and only the exact https://api.devin.ai origin is accepted. Loopback is allowed only behind an explicit test opt-in, and redirects are refused. Tests cover 58 malicious variants and assert that no request carrying the key is made.
   • Fail closed when the Devin sandbox is unavailable #12, proposal 3: the unsandboxed retry is removed, and failures get platform-specific guidance (WSL2, bwrap and socat, Seatbelt). One call for you: only sandbox failures exit non-zero. Ordinary failed runs still exit 0, as they do on main today. If you'd rather every failed job exits non-zero, I can send that as its own small PR.
   • Detect Devin Desktop's bundled CLI on macOS #13, proposal 8: the binary is resolved in this order: override, then PATH, then the macOS app bundle. The same resolved binary is used for setup, auth, listing and runs, and there are PATH-precedence and Desktop-only tests.

   The three merge cleanly together if you take all of them.

   On proposals 2, 4 and 5: agreed that an env-var unlock isn't a real human boundary when an agent can set it. I'll leave those alone until you decide. The narrow env-precedence PR for proposal 5 is easy to send whenever it's useful.

   For proposal 1, I'll start with the short design PR you described before touching the /devin:rescue default. For proposal 6, the hard part is the portable confirmation contract, so I'll put that in writing for you to push back on before building.

   One housekeeping note: my earlier bug-fix PRs (#1 to #3 and #5 to #8) were all closed on Sept 24, in the same second, around when main was rewritten, so I assume that was a side effect rather than a review. #2 is already covered on main. The other six are still present on current main. I re-ran the auth, EAGAIN and exit-code ones today; the rest I checked in the code:

   • the auth check treats "Not logged in." as logged in
   • /devin:rescue fails with EAGAIN when stdin is empty and non-blocking
   • cancel only messages the cloud session instead of stopping it, and v3 pr_url isn't read
   • failed runs exit 0
   • the first-run welcome screen is stored as a completed result
   • there's no way to change the Pro-only default model on Free accounts

   Want me to rebase those onto current main and reopen them, or would you rather handle them differently?
4. TheOnlyFusionCube commented on Oct 23, 2026

   

   TheOnlyFusionCube

   on Oct 23, 2026

   Owner

   More actions

   Thanks @hessabi for the quick and clean implementation across #11, #12, and #13.

   To answer your questions:

   1. Exit code policy: Keeping ordinary failed runs exiting 0 for now in Fail closed when the Devin sandbox is unavailable #12 is the right call to preserve existing consumer contracts while landing sandbox protections. We will revisit broader exit code semantics in a dedicated discussion after this sequence lands.

   2. Rebasing closed bug-fix PRs: Yes, please rebase and reopen the following 5 focused bug fixes as individual PRs against current main:

      • Auth status check treating "Not logged in" as logged in (former Fix auth status check treating 'Not logged in' as logged in #1)
      • Cloud cancel session termination and reading v3 pr_url (former Stop cloud sessions on cancel and read v3 pr_url #3)
      • Non-blocking empty stdin / EAGAIN handling (former Don't crash reading stdin when it is not a pipe or file #5)
      • Default model override and Free-tier guidance (former Allow overriding the default model and explain Pro-only failures #6)
      • Treating CLI first-run welcome screen as failed/needs-re-run (former Treat the CLI's first-run welcome screen as a failed run #8)

      (Please hold off on former Exit non-zero when a Devin task or review fails #7 [non-zero exits on failed tasks/reviews] for now so it can be evaluated alongside the exit code discussion in question 1; Use DEVIN_BINARY in setup so the binary override is honored #2 is already handled on main).

   I am reviewing and landing #11, #12, and #13 now.
5. TheOnlyFusionCube

   added a commit that references this issue on Oct 23, 2026

   Fail closed when the Devin sandbox is unavailable (#12)

   ...

   6891fbe