Skip to content

fix: pass bash -c "..." commands directly to Docker and display with correct quoting (issue #91)#95

Merged
konard merged 4 commits intomainfrom
issue-91-8e243739c7af
Mar 13, 2026
Merged

fix: pass bash -c "..." commands directly to Docker and display with correct quoting (issue #91)#95
konard merged 4 commits intomainfrom
issue-91-8e243739c7af

Conversation

@konard
Copy link
Member

@konard konard commented Mar 13, 2026

Fixes #91

Root Cause

Bug 1 — Quote stripping / wrong script argument: `args-parser.js` calls `commandArgs.join(' ')` to build a command string. When the user ran `--isolated docker --image konard/sandbox -- bash -i -c "nvm --version"`, the shell stripped quotes so `process.argv` contained `['bash', '-i', '-c', 'nvm --version']`. But `join(' ')` produced `bash -i -c nvm --version` — a flat string where the boundary between `nvm` and `--version` is lost.

Bug 2 — Double-wrapping: `isInteractiveShellCommand()` (added for issue #84) returns `false` for any command including `-c`. So the else-branch fired and wrapped it again: `['/bin/bash', '-i', '-c', 'bash -i -c nvm --version']` → Docker ran `/bin/bash -i -c "bash -i -c nvm --version"`.

Bug 3 — Misleading display: Even after the execution was fixed, the command was still displayed as `$ bash -i -c nvm --version` (without quotes), giving the user inaccurate feedback on how their command was interpreted.

Fix (in `js/src/lib/isolation.js`)

Three helpers added:

The branch logic in `runInDocker()` attached mode:
```js
if (isBareShell) {
// issue #84 fix (bare shell, no -c)
} else if (isShellInvocationWithArgs(command)) {
attachedCmdArgs = buildShellWithArgsCmdArgs(command); // issue #91 fix
} else {
attachedCmdArgs = [...shellCmdArgs, '-c', command]; // non-shell commands unchanged
}
```

The display line now shows:

$ bash -i -c "nvm --version"

instead of the misleading:

$ bash -i -c nvm --version

A full regression test suite is in `js/test/regression-91.test.js` (37 tests, all pass).

Adding .gitkeep for PR creation (default mode).
This file will be removed when the task is complete.

Issue: #91
@konard konard self-assigned this Mar 13, 2026
@konard konard changed the title [WIP] bash -i -c "nvm --version" was interpreted as bash -i -c nvm --version, and executed inside bash, instead of directly fix: pass bash -c "..." commands directly to Docker without double-wrapping (issue #91) Mar 13, 2026
@konard konard marked this pull request as ready for review March 13, 2026 15:15
@konard
Copy link
Member Author

konard commented Mar 13, 2026

🤖 Solution Draft Log

This log file contains the complete execution trace of the AI solution draft process.

💰 Cost estimation:

  • Public pricing estimate: $0.384494
  • Calculated by Anthropic: $0.592856 USD
  • Difference: $0.208362 (+54.19%)
    📎 Log file uploaded as Gist (601KB)
    🔗 View complete solution draft log

Now working session is ended, feel free to review and add any feedback on the solution draft.

@konard
Copy link
Member Author

konard commented Mar 13, 2026

The issue is not resolved, check my feedback at: #91 (comment)

We need to ensure all changes are correct, consistent, validated, tested, logged and fully meet all discussed requirements (check issue description and all comments in issue and in pull request). Ensure all CI/CD checks pass.

@konard konard marked this pull request as draft March 13, 2026 15:42
@konard
Copy link
Member Author

konard commented Mar 13, 2026

🤖 AI Work Session Started

Starting automated work session at 2026-03-13T15:42:52.178Z

The PR has been converted to draft mode while work is in progress.

This comment marks the beginning of an AI work session. Please wait for the session to finish, and provide your feedback.

…t (issue #91)

Add buildDisplayCommand() helper that quotes -c script arguments containing
spaces so the displayed command accurately reflects how it was interpreted.
Also export the helper and add 6 regression tests for the display behavior.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@konard konard changed the title fix: pass bash -c "..." commands directly to Docker without double-wrapping (issue #91) fix: pass bash -c "..." commands directly to Docker and display with correct quoting (issue #91) Mar 13, 2026
@konard konard marked this pull request as ready for review March 13, 2026 15:48
@konard
Copy link
Member Author

konard commented Mar 13, 2026

Fixed the remaining display issue from your feedback.

What was wrong: Even after the execution fix in v0.24.6, the command was still displayed as:

$ bash -i -c nvm --version

Root cause: createCommandLine(command) receives the flat joined string from args-parser.js and displays it as-is — no quoting is preserved.

Fix: Added buildDisplayCommand(command) in isolation.js. When the command is a shell-with--c invocation, it uses buildShellWithArgsCmdArgs() to split the argv correctly and then re-joins with the script argument quoted if it contains spaces.

Result: The output now correctly shows:

$ bash -i -c "nvm --version"

6 new regression tests added for the display behavior, all 37 tests pass.

@konard
Copy link
Member Author

konard commented Mar 13, 2026

🤖 Solution Draft Log

This log file contains the complete execution trace of the AI solution draft process.

💰 Cost estimation:

  • Public pricing estimate: $0.820006
  • Calculated by Anthropic: $0.815738 USD
  • Difference: $-0.004268 (-0.52%)
    📎 Log file uploaded as Gist (972KB)
    🔗 View complete solution draft log

Now working session is ended, feel free to review and add any feedback on the solution draft.

@konard
Copy link
Member Author

konard commented Mar 13, 2026

🔄 Auto-restart triggered (attempt 1)

Reason: CI failures detected

Starting new session to address the issues.


Auto-restart-until-mergeable mode is active. Will continue until PR becomes mergeable.

…issue #91)

Extracted SHELL_NAMES, isInteractiveShellCommand, isShellInvocationWithArgs,
buildShellWithArgsCmdArgs, and buildDisplayCommand from isolation.js into a
new shell-utils.js module to keep isolation.js under the 1000-line CI limit
(984 lines after extraction, down from 1009).

Added changeset file for the patch release.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@konard
Copy link
Member Author

konard commented Mar 13, 2026

🔄 Auto-restart-until-mergeable Log (iteration 1)

This log file contains the complete execution trace of the AI solution draft process.

💰 Cost estimation:

  • Public pricing estimate: $1.870354
  • Calculated by Anthropic: $1.179726 USD
  • Difference: $-0.690627 (-36.92%)
    📎 Log file uploaded as Gist (1960KB)
    🔗 View complete solution draft log

Now working session is ended, feel free to review and add any feedback on the solution draft.

@konard
Copy link
Member Author

konard commented Mar 13, 2026

✅ Ready to merge

This pull request is now ready to be merged:

  • All CI checks have passed
  • No merge conflicts
  • No pending changes

Monitored by hive-mind with --auto-restart-until-mergeable flag

@konard konard merged commit b017a91 into main Mar 13, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bash -i -c "nvm --version" was interpreted as bash -i -c nvm --version, and executed inside bash, instead of directly

1 participant