MIR-871: Let app run execute shell expressions

[evanphx]

Why

miren app run treated shell expressions supplied as a single command string as literal argv. Operators need expansion, pipelines, and redirection in one-off runs, without changing commands that pass arguments to interpreters or tools.

Assumptions and compatibility

• A single argument containing whitespace or shell syntax is a shell command string. This enables miren app run -- 'echo $HOME | wc -c' on current clusters.
• Multiple arguments are argv, including standalone operator characters. The caller's local shell consumed unescaped operators before Miren starts; an operator that reaches the CLI was quoted or escaped and must remain literal data (e.g. find -exec ... \; or expr 3 '>' 2).
• The legacy-server fallback keeps its original argv behavior, including its existing limitations for single-string commands on different image entrypoints. This change does not alter that compatibility path.
• Quote a shell expression locally so expansion happens in the sandbox, not before the CLI receives it.

Verification

• make lint — 0 issues
• go test -p 1 ./cli/commands -count=1 — passed
• go test -tags blackbox -timeout 15m -v -count=1 -p 1 -run '^TestRunPreservesArgumentBoundaries$' ./blackbox — passed against the local dev cluster (literal operator argv, pipeline, expansion, redirect, exit code)
• make generate-check — passed

Linear: https://linear.app/miren/issue/MIR-871

[coderabbitai]

Warning

Review paused — included plan limit reached

Keep your review moving with free on-demand reviews.

• Run this review for free

On-demand reviews are free for the next 16 days.

• Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Promotion and pricing details

On-demand reviews are free for the next 16 days. After that, they cost $0.25 per reviewed file.

Review limit details

Or wait 9 minutes for your next included review.

Check out review usage here.

Limit details: You’ve used all 6 included reviews currently available. Your 40 included PR review attempts over the past 7 days set your current allowance at 6 reviews per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 0cc1e48f-df0d-459b-8896-2959de123d9a

📥 Commits

Reviewing files that changed from the base of the PR and between bf57528 and 0bd89ba.

📒 Files selected for processing (5)

• blackbox/tasks_test.go
• cli/commands/app_run.go
• cli/commands/app_run_doc.go
• cli/commands/app_run_test.go
• docs/docs/command/app-run.md

---

_{Comment @coderabbitai help to get the list of available commands.}

[phinze]

We figured this might have been handled by the durable-runs rework of app run, but it wasn't. miren app run -- 'echo $HOME | wc -c' still exits 127 on main, because shellQuote deliberately mutes the server's sh -c. The single-string form is a clean fix. The one thing I'd change before merging is the inline note about the multi-argument operator branch.

One design question, not blocking: since the server already runs every override through sh -c, would it be cleaner for CreateRun to take a "this is shell source" flag and skip shellQuote, instead of sending a client-built /bin/sh -c that gets quoted and wrapped in a second shell? That would also make app runs list show the command as typed.

--p+🤖

[phinze]

🤖 This branch only ever sees a standalone operator when the user escaped it on purpose. Unescaped, miren app run -- echo hi | wc has the local shell eat the | before we run. And people escape operators because they want the literal character. Both of these work on main and break here:

find . -name '*.log' -exec rm {} \;   ->  find: missing argument to `-exec'
expr 3 '>' 2                           ->  prints nothing, creates a file named "2"

Dropping ; fixes find, but expr, test/[, and awk programs take >, <, |, and & as literal arguments too. I'd drop the multi-argument branch altogether and let the single-string form carry shell syntax. The docs already steer people there, and it's the only form you can type without hand-escaping operators.

[miren-code-agent]

I agree, and I should have followed my own reasoning further. In my last review I called grep -F '|' rare. But you're right that the local shell consumes any unescaped operator before runCommand sees it. So every lone |, >, ; or & that reaches line 117 was quoted or escaped on purpose, and that points to literal intent, not shell structure. I checked the code: expr 3 '>' 2 goes to the > case, gets joined as 'expr' '3' > '2', and redirects into a file named 2. That's a silent write, not just a failed command, which makes it worse than the find case. The same goes for test/[ and awk programs. Dropping ; alone wouldn't fix any of this. I'd also remove the whole multi-argument branch and keep only the len(args) == 1 shell-source path. A nice side effect is that legacyRunCommandCheck then only rejects the single-string form, so older clusters lose less.

[miren-code-agent]

I checked the current head: the multi-argument branch has been removed as you suggested. find … -exec … ; and expr 3 '>' 2 now pass through as argv, and there are unit and blackbox tests for both. Removing it also let legacyRunCommandCheck go, so older clusters are back to how main behaves. I'll leave this one for you to close.

[evanphx]

🤖 Agreed. Operators that reach the CLI were quoted or escaped locally, so treating them as shell syntax was unsafe. The merged change only interprets a single command string as shell source; the affected blackbox test passed.

[miren-code-agent]

🍪 biscuit: ✅ ready to merge — auto-review, non-blocking

I'm happy for this to merge. Both caveats from my last review are gone, and the fixes are the ones phinze and I asked for.

The multi-argument operator branch has been removed. runCommand (app_run.go:96-102) now switches to /bin/sh -c only for a single argument that contains shell syntax or whitespace. Anything with more than one argument keeps its argv unchanged. The comment above it gives the right reason: once the local shell has removed the quoting, a standalone operator is data the user escaped on purpose. TestRunCommand has rows for find … -exec rm {} ;, expr 3 > 2, grep -F '|' and echo hi > /tmp/result, and each one asserts that argv is preserved. TestRunPreservesArgumentBoundaries in the blackbox suite checks expr 3 > 2 end to end, so the silent-redirect-into-a-file-named-2 case now has a test at the level where it would actually cause harm.

The older-cluster regression is gone too. legacyRunCommandCheck has been removed, and appRunLegacy gets the raw opts.Args again. That means miren app run -- 'bin/rails console' on a pre-runs cluster behaves exactly as it does on main.

I also traced the wrapped form through the server. resolveCommand/shellQuote turn it into '/bin/sh' '-c' '<src>', and appspec puts the config entrypoint in front of that. So on CNB apps it runs as launcher /bin/sh -c <src> and still gets the buildpack environment.

I have one small inline note: a new blackbox assertion depends on an env var alias that is deprecated and scheduled for removal. It doesn't block the merge, but it's a one-word fix.

I'm resolving my open thread. I'm leaving phinze's thread for them to close, since they opened it.

---

🍪 full review note · reviewed at f9e365b · comment /biscuit review to run biscuit again.

[miren-code-agent]

MIREN_APP is the deprecated alias that api/app/runtimeenv.go keeps only for a deprecation window. The comment there says the aliases "will be removed in a future release". Once that happens, this assertion fails because $MIREN_APP expands to nothing, and that has nothing to do with shell handling. Please use echo $MIREN_RUNTIME_APP instead, since that's the canonical name appspec injects. The test checks the same thing and doesn't depend on the alias.

[evanphx]

Responding to the biscuit review and human review: the merged change preserves every multi-argument invocation as argv and retains the legacy fallback unchanged, rather than rejecting single-string commands. I kept the existing run protocol instead of adding a shell-source flag, which would widen this compatibility change. make lint, CLI tests, the affected blackbox test, and make generate-check passed before merge. A subsequent test-only suggestion to use the canonical MIREN_RUNTIME_APP variable was verified locally but arrived after PR #1263 merged; it is not in the merged PR. — e + 🤖