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.
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.Posted to the PR's comment threads when you submit.
runCommand only wraps a single argument that looks like shell source. TestRunCommand includes the find . -name x -exec rm {} ; and expr 3 > 2 rows I asked for, and both assert argv is preserved. The blackbox test also runs expr 3 > 2 against a real cluster and expects 1.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.Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1263,
"verdict": "ready",
"event": "approve",
"summary": "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.\n\n**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 \u003e 2`, `grep -F '|'` and `echo hi \u003e /tmp/result`, and each one asserts that argv is preserved. `TestRunPreservesArgumentBoundaries` in the blackbox suite checks `expr 3 \u003e 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.\n\n**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.\n\nI also traced the wrapped form through the server. `resolveCommand`/`shellQuote` turn it into `'/bin/sh' '-c' '\u003csrc\u003e'`, and `appspec` puts the config entrypoint in front of that. So on CNB apps it runs as `launcher /bin/sh -c \u003csrc\u003e` and still gets the buildpack environment.\n\nI 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.\n\nI'm resolving my open thread. I'm leaving phinze's thread for them to close, since they opened it.",
"comments": [
{
"path": "blackbox/tasks_test.go",
"line": 275,
"side": "RIGHT",
"body": "`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."
}
],
"replies": [
{
"thread_id": "PRRT_kwDONZ__ss6lbgQE",
"body": "This is addressed in the current code. The multi-argument operator branch is gone, and `runCommand` only wraps a single argument that looks like shell source. `TestRunCommand` includes the `find . -name x -exec rm {} ;` and `expr 3 \u003e 2` rows I asked for, and both assert argv is preserved. The blackbox test also runs `expr 3 \u003e 2` against a real cluster and expects `1`.",
"resolve": true,
"resolve_reason": "addressed"
},
{
"thread_id": "PRRT_kwDONZ__ss6mCESk",
"body": "I checked the current head: the multi-argument branch has been removed as you suggested. `find … -exec … ;` and `expr 3 '\u003e' 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."
}
],
"posted_to_pr": true,
"head_sha": "f9e365bf7e651cac0eb6112efa552f629c6fb256"
}This can move forward once you've made a decision about one regression. Detection now relies on two signals the user has to choose on purpose: a single argument that is shell source, or an argument that is exactly an operator token. That fixes the problems from my last review. python -c "print('hi')", psql -c "SELECT * FROM users;", the & URL, grep -E 'foo|bar', sh -o pipefail -c … and /usr/bin/env bash -c … all keep their argv unchanged. The new TestRunCommand rows assert this, and TestLegacyRunCommandCheck covers the older-cluster side. I'm resolving all three of my earlier threads.
; in the operator list breaks find -exec. In miren app run -- find /tmp -name '*.log' -exec rm {} \;, your local shell passes ; as its own argument. runCommand (app_run.go:117) treats it as an operator and builds 'find' … '{}' ;, so find fails with "missing argument to -exec". That command works on main today, because resolveCommand/shellQuote in servers/app/runs.go preserve argv. A lone quoted operator used as data, like grep -F '|' file, hits the same path. That case is rare enough that I'd accept it. \; after -exec is not rare. The inline comment has options.
Older clusters now refuse a form that used to work. legacyRunCommandCheck rejects any single argument that contains a space. On pre-runs clusters, miren app run -- 'bin/rails console' now gets a hard error. Your own comment at app_run.go:131 says those servers join argv into shell source when the app has an entrypoint, so on buildpack apps with the CNB launcher, that command used to run. The error message is clear, and I understand why you'd rather refuse than guess. But this takes away something that works for users still on older clusters during the compatibility window, so please accept that on purpose rather than by side effect.
If you drop ; (or special-case -exec) and you're fine with the legacy trade-off, I'm happy with this.
; here breaks find … -exec cmd {} \;. The local shell passes the escaped ; as its own argv element, so this turns it into a real statement separator and find fails with "missing argument to -exec". That command works on main today. I'd take ; out of this list, since the single-string form already covers command sequencing and the docs point people there. The other option is to leave a ; or + that closes a -exec/-execdir/-ok alone. Either way, please add a TestRunCommand case for find . -name x -exec rm {} ; that asserts argv is preserved.Posted to the PR's comment threads when you submit.
it's $HOME, all with argv preserved. I have one remaining note about ; in the operator list, and I've left it as a new inline comment.sh -o pipefail -c …, /usr/bin/env bash -c … and bash -e -c … have no bare operator tokens, so they pass through unchanged. The new table cases and the real-shell exit tests cover them.python -c "print(1)" and psql -c "SELECT * FROM t" pass the legacy check now, and TestLegacyRunCommandCheck asserts both.Verdict: caveats
I don't think this is ready to merge. The detection heuristic breaks the most common thing people do with app run: pass a quoted code or SQL string to an interpreter.
The core problem. By the time runCommand gets args, the user's local shell has already removed the quotes. So the CLI can't tell a | the user meant as a pipe from a | inside an argument they deliberately quoted. The PR treats any argument that contains a character from shellSyntax as raw shell source and pastes it in unquoted (app_run.go:125-131). That silently changes or breaks commands that work correctly today, since the server's shellQuote in servers/app/runs.go preserves argv exactly:
miren app run -- python -c "print('hi')" becomes 'python' '-c' print('hi'), which is a sh syntax error.miren app run -- psql -c "SELECT * FROM users;" gets a glob expansion, a word split and a statement break. The shellQuote comment names psql -c "SELECT 1" as exactly the case argv preservation exists to protect.miren app run -- curl "https://x/?a=1&b=2" backgrounds curl at the &.miren app run -- grep -E 'foo|bar' log pipes grep into a command called bar.rails runner "User.find(1)", node -e "..." and ruby -e 'puts [1,2].sum' all hit the same problem.None of the new tests put a metacharacter inside a quoted argument. TestRunCommandPreservesLiteralArgumentsInPipeline only uses literals without metacharacters, so this regression isn't covered.
Explicit shells. The explicit-shell check (app_run.go:112-121) misses real invocations: sh -o pipefail -c '...', bash -O extglob -c '...' and /usr/bin/env bash -c '...'. Those scripts then get spliced in unquoted, so the command the user wrote as a proper shell invocation breaks too. Details are in the inline comment.
Legacy clusters. The same misdetection flows into legacyRunCommandCheck. On older clusters, python -c "print(1)" is now refused outright with "shell expressions … require a newer cluster", even though it isn't a shell expression.
Suggested direction. Pick signals that can't be confused with quoted data:
-- 'echo $HOME | wc -c'), which the docs already recommend, and/or--shell / -s.Treating only arguments that are exactly an operator token (|, &&, ;, >, <, >>) as shell structure would be much safer than a substring match, but $VAR expansion still can't be inferred reliably. The tests that run the rewritten argv through a real sh/bash and check the exit code are a good way to cover this; I'd extend them to the quoted-metacharacter and -o pipefail cases above.
$|&;<>()*?[]{}~\ can't separate an operator the user typed from data they quoted. python -c "print('hi')", psql -c "SELECT * FROM t;", curl "https://x/?a=1&b=2" and grep -E 'foo|bar' f all get their argument inserted unquoted and change meaning or fail. An argument containing a stray ', like echo "it's $HOME", becomes an unterminated quote. Please base this on something unambiguous: the single-argument form, an explicit --shell flag, or at minimum arguments that are exactly an operator token. Also add table cases with metacharacters inside a quoted argument that assert argv is preserved.-, so a shell option that takes a value hides a later -c. For sh -o pipefail -c 'a | b', the scan sees -o, then stops at pipefail, and never reaches -c. The script then gets spliced in unquoted as 'sh' '-o' 'pipefail' '-c' a | b, and the outer sh runs sh -o pipefail -c a piped into b. bash -O extglob -c ... breaks the same way. So does /usr/bin/env bash -c ..., because filepath.Base(args[0]) is env. Narrowing the metacharacter detection would make these harmless. If this block stays, it should skip values for -o/-O/+o and look through env.runCommand's detection, a plain python -c "print(1)" or psql -c "SELECT * FROM t" against an older cluster is now rejected as a "shell expression", even though the user didn't ask for any shell behaviour. Narrowing the detection fixes this too, but it's worth a test case here.Verdict: not_ready