Repository navigation
🐛 fix(attach): keep literal backslashes inside double quotes in splitShellWords - #389
Conversation
…ellWords Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
hive/awaiting-ci: CI pending — sweep will merge when green. 🐝 Hive Agent: — hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
There was a problem hiding this comment.
Read splitShellWords in full at 8721552 (src/attach.ts:67-116) and its caller buildCliCommand (src/attach.ts:118). Looks correct to me.
- The escape set
'$"\\n'(src/attach.ts:76) is exactly POSIX 2.2.3's: before those characters the backslash is still dropped, before anything else it is kept. Outside quotes the branch is skipped byquote === '"', so existing behaviour is unchanged; inside single quotes` never reaches this branch (src/attach.ts:81). - Trailing
\inside an open"still appends the backslash and then throws the existingUnterminated quoteerror (src/attach.ts:109-113), so no new silent path. - Built and ran
test/attach-orchestration.test.jslocally: 44/44 pass.
Only note (pre-existing, not from this change): \+newline inside double quotes yields a literal newline where POSIX would join lines; the unquoted path already behaves that way, so nothing to do here.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Hive filed review findings that do not block this PR as backlog items:
Cap: 10 backlog item(s) per PR per day. |
|
stage 060-reviewed → 050-review-pending: pending: head pushed 1m29s ago (< min_head_age) |
There was a problem hiding this comment.
Looks correct to me.
Read splitShellWords in full at fafa17c (src/attach.ts:67-112) plus the callers (src/attach.ts:46,117,130,226,357,445, src/cli.ts:249). The one-line change at src/attach.ts:76 only fires on the escaping branch when quote === '"', so unquoted (a\ b) and single-quoted paths are untouched, and \", \\, \`, \$ inside double quotes still collapse as before — matching the POSIX rule the issue (#387) spells out. The trailing-backslash handling at :103-106 is unaffected (a dangling \ inside "…" still ends in the unterminated-quote throw).
Ran node --test test/attach-orchestration.test.js on the PR head: 44/44 pass, including the five new cases. verify, test (22/24/26) and DCO are green; the only pending check is tide.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read splitShellWords in full at fafa17c plus all nine callers (src/attach.ts:46,117,130,226,357,380,445, src/cli.ts:249). Looks correct to me.
src/attach.ts:76only fires whenquote === '"', so unquoted\/\"handling (test/attach-orchestration.test.js:541) is untouched — confirmed by the existing test still passing.- Inside
"…",\"still reaches the escaping branch before thechar === quotecheck at:93, so the quote is not closed early;\\,\`and\$collapse to one char as POSIX requires. - The
rgBincaller at:46regex-escapes each word after splitting, so a now-preserved backslash in a quoted path is escaped rather than passed raw.
Ran node --test test/attach-orchestration.test.js at the PR head: both splitShellWords tests pass, npm run build clean. Matches the behaviour #387 asked for (per #387: keep the backslash unless the next char is one of $ " `).
Not blocking, pre-existing: \<newline> inside double quotes yields a literal newline rather than being removed as a line continuation; nothing in the issue scope depends on it.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
splitShellWords dropped every backslash outside single quotes, including inside double quotes where POSIX treats it as literal unless followed by $, backtick, ", \ or newline. Inside double quotes the backslash is now kept for ordinary characters; behaviour outside quotes is unchanged. Added tests next to the existing escaping test and a changelog fragment.
Closes #387
— hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88