Repository navigation
Harden Container Mount Assertions With A Platform-Aware shellWord (BF-2201) - #1679
Conversation
Export shellWord for setup command display strings and use the allowlist quoting rule so Windows bind-mount words are single-quoted in previews. Update container-computer mount expectations to call shellWord instead of hard-coding unquoted mount specs. Co-authored-by: Jay Wedgeworth <jaywedgeworth22@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
|
Closing as stale: this visibility draft has had no commit in 25 days and its CI is red. Nothing is deleted: the branch stays on the remote, and the PR can be reopened if the work resumes. Fleet draft sweep: AFC-679. |
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
|
Automatic approval held Kody's assessment:
|
|
@kody start-review |
|
Reopening: the staleness this close cited is cured on the current head. The branch was updated onto today's main during the OPS-1054 sweep (merge commit minutes old), the failing checks were from the pre-update head, and the touched-area gates pass locally. Driving it through the normal path: CI on the fresh head, Kody review, threads resolved, then merge. |
Kody: cmd.exe ignores single quotes, so win32 gets the double-quote form with doubled quotes; POSIX keeps the escaped single-quote form. Mount assertions now pin literal strings, and direct shellWord unit tests cover plain, quoted, and win32 paths so quoting regressions cannot pass by construction. Refs BF-2201.
The merge had silently replaced main's refined local shellWord — quote only shell metacharacters, leave a backslash or tilde bare because Windows paths have always printed as-is — with the branch's wider Oct 15 rule. One coherent version now: main's metachar class and bare-path rule, plus the win32 double-quote form from the review. Refs BF-2201.
jaywedgeworth22
changed the title
This comment has been minimized.
This comment has been minimized.
Pull request was closed
Main landed the BF-2201 alignment independently, which restored the wide Oct 15 quoting rule and the tautological mount assertions. Keep one coherent version: main's metachar policy from the earlier local helper, the win32 double-quote form from review, and the literal assertions plus the direct unit tests. Refs BF-2201.
Round-3 review: an empty argument joined as nothing and silently shifted the pasted command, bracket pairs glob in POSIX shells, and cmd.exe expands %VAR% inside double quotes and treats ^ as its escape. A backslash or tilde still prints bare, per the long standing Windows-path rule the comment documents. Refs BF-2201.
This comment has been minimized.
This comment has been minimized.
…read) A word ending in a backslash, e.g. C:\Program Files\, closed as \" and the Windows argv parser treated the closing quote as escaped, merging the next argument. The run is now doubled before the quote. Test added.
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
Automatic approval held: Changes platform-aware shell quoting for container setup commands with subtle POSIX/cmd escaping rules; compatibility assumptions need human verification despite new unit tests. |
Aligns the setupCommands mount assertions with the shellWord quoting the commands actually use, per BF-2201, and fixes what the alignment uncovered.
Verification: pnpm typecheck and vitest server/container-computer.test.ts (106 passing) in an isolated lane. Fixes found by Kody in rounds 1 and 2; every thread answered and resolved. Refs BF-2201 (Done via #945); this hardens the tests it left behind.
Summary
Reworks display quoting for generated container runtime commands so pasted text behaves correctly in POSIX shells and cmd.exe. POSIX words with whitespace or metacharacters are single-quoted, while win32 words are double-quoted with embedded quotes doubled.
Changes
shellWordand threads the target platform throughsetupCommandscommand formatting. Leaves words without whitespace or listed metacharacters bare, preserving existing bare rendering of backslashes and tildes in Windows-style paths. Stops quoting--mount type=bind,...workspace strings.Tests
shellWordunit coverage for bare tokens, backslash/tilde handling, POSIX single-quote escaping, and win32 double-quote handling. No test-run results, approvals, or runtime verification are evidenced in the provided diff.Unresolved risks
server/container-computer.ts: brackets/backslash/tilde left unquoted,%and^left unquoted on win32, and empty arguments dropped rather than quoted. Impact of these edge cases is unverified.Summary
Hardens platform-aware
shellWord()used for container mount assertions (BF-2201). Empty words can no longer vanish from pasted commands, and POSIX bracket globs plus cmd.exe%/^now trigger quoting.Changes
server/container-computer.ts: returns""on win32 and''otherwise for empty input. Extends win32 triggers with[,],^,%and POSIX triggers with[,]. Retains existing single-quote vs double-quote escaping per platform.server/container-computer.test.ts: adds coverage for empty words and for bracket, percent, and caret quoting.Tests
Tests added for the new quoting cases; no run results, deployment, or runtime verification reported.
Open risk
Automated review reports one high finding in
server/container-computer.ts:2013: win32 quoting may lose the closing quote on a trailing backslash. Behavior remains unverified.Summary
Addresses BF-2201 by hardening Windows shell quoting in
shellWord. Doubles a trailing backslash run before wrapping awin32word in double quotes so the closing quote is not escaped under the Windows argv parser.Changes
shellWordinserver/container-computer.tsforwin32to duplicate trailing backslashes after escaping embedded quotes. POSIX quoting behavior is unchanged.server/container-computer.test.tscoveringC:\Program Files\onwin32. Run status and runtime verification remain unverified.Impact
Prevents malformed quoted Windows paths ending with a backslash. No POSIX behavior change is shown by the patch.
Automated code review reported no issues in the reviewed changes.