Repository navigation
fix(resolver): treat \, as a literal comma so values are not fanned out - #356
Conversation
Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
kubestellar-prow
Bot
added
the
dco-signoff: yes
|
[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 |
kubestellar-prow
Bot
added
the
size/M
hivecommons-hive
Bot
added
author/hive
|
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 |
|
Hive filed review findings that do not block this PR as backlog items:
Cap: 10 backlog item(s) per PR per day. |
![hivecommons-hive[bot]](https://avatars.githubusercontent.com/in/3568013?s=60&v=4){kind=small}
There was a problem hiding this comment.
Read src/resolver.ts at the PR head and the base, plus the ui.html command generator. Looks correct to me.
splitUnescaped(src/resolver.ts:33-39): the(?<!\\),lookbehind split then\,→,replace is applied beforeexpandPiece, so a value with an escaped comma stays one iteration and a mixeda\,b,cyields['a,b','c']— both covered by the new tests.@filepath (src/resolver.ts:45-51): base code only ever split on\n, never on commas, so the issue's claim that file lines were comma-split (#355 "resolver.ts:40-44") was inaccurate; the PR's unescape-only treatment there is the right call and keeps existing files unchanged.- Builder round-trip (
src/ui.html:898-911): a value containing\,failsbareWordand is single-quoted, where the backslash is literal, so the generated command reaches the resolver with\,intact; glob-source values go throughdquote, which doubles the backslash and also survives the shell.
One known limitation, not a defect: there is no \\ escape, so a value whose intended separator comma is immediately preceded by a literal backslash cannot be expressed. Fine for the prose-blank use case in #355; worth a line in the README table only if it ever comes up.
CI green on all six build-and-test matrices (dist/ in sync).
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read src/resolver.ts at 988512b and the callers; this looks correct to me.
- Both
expandArrayValuecall sites (src/resolver.ts:174flag path,:204interactive answer) go through the newsplitUnescaped, so the escape covers exactly the two paths #355 names. @filelines were only ever split on\n(src/resolver.ts:46-51), so adding the\,→,unescape there without a comma split is the right call — issue #355's "@file lines are split too" was not accurate against the tree, and the PR body says so.src/ui.html:910emitsrow.valuesraw into the generated command, so\,survives to the CLI and is unescaped exactly once;splitValues(:818) is preview-only. No double-unescape.a\,b,c→['a,b','c']andreturns 200\, not 500→ single value, both asserted insrc/resolver.test.ts;dist/is regenerated and the dist-sync CI gate is green on Node 22/24/26.
One note, not a defect: template {{var=default}} defaults (src/resolver.ts:195-198) never pass through expandArrayValue, so a \, in a default stays literal \,. Defaults were never comma-split, so nothing regresses; just mentioning it in case anyone expects the escape to be universal.
— 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 |
hivecommons-hive
Bot
removed
role/trusted
splitUnescaped only looked one character back, so a value ending in a backslash (C:\out\,D:\out) could no longer be split after hivecommons#356, and there was no escape to recover it. A run of backslashes right before a comma is now read in pairs: each \\ is a literal backslash and an odd leftover makes the comma literal. Backslashes elsewhere are untouched, so plain Windows paths and existing \, values behave as before. The builder preview (ui.html) and @file lines follow the same rule. Closes hivecommons#357 Hive-Run: hivecommons#357 Hive-Plan: review-backlog/correctness Hive-Spec: promptargs-comma-escape#357 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HRH6kne36mF3hG2NYw72Ef Signed-off-by: Karibhou <Karibhou@users.noreply.github.com>
…ma (#358) splitUnescaped only looked one character back, so a value ending in a backslash (C:\out\,D:\out) could no longer be split after #356, and there was no escape to recover it. A run of backslashes right before a comma is now read in pairs: each \\ is a literal backslash and an odd leftover makes the comma literal. Backslashes elsewhere are untouched, so plain Windows paths and existing \, values behave as before. The builder preview (ui.html) and @file lines follow the same rule. Closes #357 Hive-Run: #357 Hive-Plan: review-backlog/correctness Hive-Spec: promptargs-comma-escape#357 Claude-Session: https://claude.ai/code/session_01HRH6kne36mF3hG2NYw72Ef Signed-off-by: Karibhou <Karibhou@users.noreply.github.com> Co-authored-by: Karibhou <Karibhou@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Adds a
\,escape: values are split only on unescaped commas, then\,is unescaped to,. Applied to--flagvalues, interactive answers (sharedexpandArrayValue), and@filelines (unescape only; file lines were never comma-split, so that behaviour is unchanged). Values without\,behave as before. The builder UI preview (ui.html) accepts the same escape; its generated command already passes the raw value through. Docs updated in README and SKILL.md; tests, changelog fragment and rebuiltdist/included.Closes #355
— hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88