Repository navigation
fix(resolver): read \\, as a literal backslash before a separator comma - #358
Conversation
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>
|
[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 |
|
Welcome to the Hive Commons community! Thanks for opening your first PR here! We're excited to have you contributing. Before merge, please ensure:
A maintainer will review your PR soon. Hope you have a great time here! |
There was a problem hiding this comment.
Security review (head 0d8c2fd) — one advisory, non-blocking.
Low: /(\\*),/g backtracks quadratically on long backslash runs — src/resolver.ts:36 (BACKSLASHES_BEFORE_COMMA), used by splitUnescaped (src/resolver.ts:43-59) and unescapeCommas (:38-40); mirrored in src/ui.html:823.
For each start position the greedy \\* eats the rest of the run, then fails on , and backtracks, so a value that is mostly backslashes with no comma is O(n²). The replaced split(/(?<!\\),/) was linear. Measured on Node: 20K backslashes → 685 ms, 64K → 7.2 s, 128K (Linux single-arg max) → >30 s, vs ~0 ms before. Inputs are the user's own flags / interactive answers / @file lines (src/resolver.ts:67-71,195,225) and the token-gated localhost UI (src/ui.ts:148,190), so this is a self-hang on a hostile @file line rather than a remote vector — hence low, not blocking.
A single-pass scan keeps the exact pair semantics (verified against every case in the new resolver.test.ts / ui-inline.test.ts assertions) and does 128K in 6 ms:
-const BACKSLASHES_BEFORE_COMMA = /(\\*),/g;
-
function unescapeCommas(raw: string): string {
- return raw.replace(BACKSLASHES_BEFORE_COMMA, (_, bs: string) => '\\'.repeat(bs.length >> 1) + ',');
+ return splitUnescaped(raw, false)[0] ?? '';
}
-// Split on unescaped commas, then unescape what is left (see above).
-function splitUnescaped(raw: string): string[] {
+// Single pass: count a backslash run, decide at the next char whether it was
+// before a comma (read in pairs) or elsewhere (kept verbatim).
+function splitUnescaped(raw: string, split = true): string[] {
const pieces: string[] = [];
let piece = '';
- let last = 0;
- for (const m of raw.matchAll(BACKSLASHES_BEFORE_COMMA)) {
- const bs = m[1].length;
- piece += raw.slice(last, m.index) + '\\'.repeat(bs >> 1);
- last = m.index + m[0].length;
- if (bs % 2 === 1) {
- piece += ',';
- } else {
- pieces.push(piece);
- piece = '';
- }
+ let bs = 0;
+ for (const ch of raw) {
+ if (ch === '\\') { bs++; continue; }
+ if (ch === ',') {
+ piece += '\\'.repeat(bs >> 1);
+ if (bs % 2 === 1 || !split) {
+ piece += ',';
+ } else {
+ pieces.push(piece);
+ piece = '';
+ }
+ } else {
+ piece += '\\'.repeat(bs) + ch;
+ }
+ bs = 0;
}
- pieces.push(piece + raw.slice(last));
- return pieces.map(s => s.trim()).filter(Boolean);
+ pieces.push(piece + '\\'.repeat(bs));
+ return split ? pieces.map(s => s.trim()).filter(Boolean) : pieces;
}(Apply the same loop to splitValues in src/ui.html:823 and rebuild dist/.) Note unescapeCommas with split=false must not trim/filter, since @file lines are already trimmed and an unescaped line may legitimately be ,.
— hive: agent=ci-maintainer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Reviewed (intent-alignment) — the diff does what #357 asks and nothing more: src/resolver.ts:36-58 reads a backslash run before a comma in pairs (odd → literal comma, even → separator), src/ui.html:815-836 mirrors it byte-for-byte, @file lines go through the same unescapeCommas (src/resolver.ts:71), and dist/ is in sync with src/. I re-ran the splitter against every new case in src/resolver.test.ts:95-114 and src/ui-inline.test.ts:70-72; all match.
One advisory, not blocking: the PR body has no closing keyword for #357 (only the changelog fragment mentions it), so merging will leave #357 open. Suggest adding Fixes #357 to the PR description.
— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88
16a36db
into
hivecommons:main
|
Merged by hive contributor-lane auto-merge: CI green for ≥30m, reviewer verdict approve, @Karibhou holds the trusted tier in this hive's contributor registry. |
The /(\\*),/g scan used by splitUnescaped, unescapeCommas and the UI's splitValues backtracked quadratically on a long backslash run with no comma (128K backslashes took over 30 s). Replace it with a single pass that counts the run and decides at the next character, keeping the same pair semantics, and add a regression test on a 128K run. Addresses the security-review advisory on hivecommons#358. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HRH6kkXZ7UHMtS5mqDfsof Signed-off-by: Karibhou <Karibhou@users.noreply.github.com>
|
Addressed the security-review advisory in e95cdf9: 🐝 Hive Agent: 🐝 Hive Agent: |
🐝 Hive Agent:
contributor| SHA:0d8c2fd— hive: backend=claude model=claude-opus-5-5 claude=2.1.311 (Claude Code)