Skip to content

fix(hooks): port the two upstream guard-main-checkout-bash fixes — escaped \" in a double-quoted word, and shell comments read as commands - #5755

Merged
hotlong merged 2 commits into
mainfrom
claude/issue-5459-guard-bash-port-two-fixes
Aug 23, 2026
Merged

fix(hooks): port the two upstream guard-main-checkout-bash fixes — escaped \" in a double-quoted word, and shell comments read as commands#5755
hotlong merged 2 commits into
mainfrom
claude/issue-5459-guard-bash-port-two-fixes

Conversation

@os-zhuang

@os-zhuangos-zhuang commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Fixes#5459
Fixes#5712

Ports the two upstream fixes that this repo's copy of guard-main-checkout-bash.sh had
drifted from. The file's own header declares the two repos' copies "deliberately kept
case-for-case identical … so the two repos' guards cannot drift"; two independent fixes had
landed upstream since, and neither was here.

One commit per member card, in upstream's own order.

Commit 1 — \" inside a double-quoted word does not end the quote (#5459, upstream objectstack PR #10406)

POSIX: inside "…" a backslash keeps its meaning only before ", \, $ and a backtick.
Both quote-aware passes (split_segments() and tokenize()) ended the string at the \",
after which the rest of a node -e "…" program was read as bare shell.

Measured against this repo's pre-fix copy, from a cwd inside the shared primary checkout —
the desync failed in both directions:

commandpre-fix verdict
node -e "const j=require(\"./a.json\"); console.log(j.x.map(st=>st.a)…)"BLOCK, target: st.a).join(" — a pure read
sed -i "s/\"a\"/\"b\"/" pkg/x.tsALLOW — a real write, missed

The second is the reflexive sed -i this guard exists to catch, so the shared checkout was
unguarded against that shape until this PR.

Commit 2 — a shell COMMENT is text, not a command (#5712, upstream objectstack PR #11129)

Both passes now apply the shell's comment rule: outside quotes and heredoc bodies, an
unquoted # that starts a WORD begins a comment running to the next newline.

Measured against this repo's pre-fix copy:

# rename foo -> bar
echo hello -> BLOCK, target: bar (a pure read)
echo hello -> allow (control: same command, no comment)

Fail-CLOSED, so nothing unguarded slipped through — but it is the false-positive direction
that trains an operator onto OS_ALLOW_MAIN_EDITS=1, which switches the guard off for the
whole command.

The word-start condition is the whole rule: foo#bar, ${x#y}, curl 'url/#frag',
sed 's/#//' and grep '#' are not comments and keep every verdict they had. {/} do
not reset word state (reserved words, not metacharacters), so the # of ${#arr[@]} cannot
swallow a real redirect behind it.

Verification

Self-test: 70 → 82 → 100 cases, matching upstream's own counts at each step (upstream
PR #11129's message records 82 -> 100), which is independent evidence the two ports were
split at the right line.

Ablation — each member's assertions are independently load-bearing. Two fixes in one PR
is exactly where one shared assertion can hide a missing half, so each fix was reverted
alone, with the mutation confirmed on disk by marker count before reading any result:

runon-disk proofresult
both present100 passed, 0 failed
escaped-quote fix ablatedescape-block markers 2 → 096 passed, 4 failed — all in the \" section; comment-rule section green
comment-rule fix ablatedword-tracking markers 3 → 096 passed, 4 failed — all in the COMMENT section; \" section green
restoredmarkers back to 2 / 3100 passed, 0 failed, tree clean

No other section failed in either ablation.

Gates run at db9d0fd0 (the final commit), derived from the actual diff against this
repo's own scripts — scripts/pm/dispatch-gates.mjs is objectstack-only and does not exist
here:

  • .claude/hooks/guard-main-checkout-bash.selftest.sh100 passed, 0 failed
  • node scripts/check-control-bytes.mjs✅ check-control-bytes: OK (scanned 4797 tracked text file(s); skipped 85 binary).
  • node scripts/check-changeset-presence.mjs✅ No source of a released package changed in this range, so no changeset is owed.
  • bash -n on both files → clean

ESLint owes nothing on this diff, and that is a measurement rather than a skip: every block
in eslint.config.js is scoped to **/*.{ts,tsx} (no .sh glob anywhere in the config),
this diff contributes 0 files to that population, and since no shell file is in any
config block's files — nor importable by TypeScript — the diff cannot move a verdict on
any untouched file.

Localisation

Per the header's own convention. The residual diff against objectstack's copy is now
localisation only — every logic line is byte-identical:

  • issue refs carry the objectstack# prefix (objectstack#10247, objectstack#10570),
    matching the existing objectstack#4890 precedent — upstream's bare #10247 / #10570
    are not copied in verbatim;
  • the non-ASCII paragraph and its self-test probe cite this repo's own AGENTS.md prose
    arrows instead of objectstack's CLI boot banner (➜ API:), which does not exist here —
    measured: 0 occurrences of that glyph in objectui source, 14 in AGENTS.md;
  • this copy's own cross-references to guard-shared-stash.sh, its packages/fields/src/x.tsx
    example path, its @object-ui/app-shell filter example and the objectui#3435 / #3430
    refs are left untouched.

One deliberate non-divergence, flagged for review

Upstream's header says "Three layers:" and then lists four — PR #11129 added layer 4
without bumping the count. It is ported verbatim, miscount included, because
case-for-case identity is this file pair's whole invariant and correcting it here would be
inventing a fix ahead of the source repo — the same principle as the exclusion list below.
Filed upstream instead: objectstack-ai/objectstack#11234. Once that lands, the correction
ports here like any other drift item.

Explicitly NOT in this PR

  • guard-main-checkout.sh — the Edit/Write guard parses no shell text;
  • guard-shared-stash.sh and its self-test — a different guard; neither card claims drift there;
  • objectstack's copy — this port runs one direction only;
  • upstream objectstack#11131 (backslash-desync) and objectstack#11133 (heredoc-in-comment) —
    the two fail-open siblings recorded in #11129's out-of-scope note, both unadjudicated
    upstream; porting them would be inventing fixes ahead of the source repo;
  • the case-for-case identity gate itself. Noted as a follow-up candidate per the triage
    decision, not built here. Both cards raise it and this PR is the third data point: a gate
    that diffs the two copies modulo the localised lines would have caught both drifts when
    they were introduced. Two things a future gate would need, learned here: the localised set
    is small and enumerable (issue refs, example paths, the package name, the sibling-hook
    cross-references, and the downstream-only "Ported from …" provenance paragraph), and the
    file pair's self-tests keep matching case counts, which is a cheaper second signal.

Related finding

CI in neither repo runs these hook self-tests — grep -rn selftest .github/workflows/
returns nothing in objectui or objectstack — so a broken guard lands green. Filed separately
as #5754; not fixed here.

Governed surface (.claude/**): left as a draft, not enqueued, no auto-merge. The
maintainer's manual merge is the review record.

⚠️The review request could not be set. Requesting os-zhuang returned
Review cannot be requested from pull request author — all agents share the os-zhuang
identity, so the reviewer the governed-surface rule names is also this PR's author, and
GitHub rejects that pairing. Not worked around: nothing was flipped, relabelled, or
reassigned to force it through. @os-zhuang — this needs your review; the request has to
come from outside the shared identity, or the convention needs a different reviewer.


Generated by Claude Code


Generated by Claude Code

zhuangjianguoand others added 2 commits August 23, 2026 04:42
…not end a double-quoted word
Ports objectstack PR #10406 (objectstack#10247) into this repo's copy of the hook, which
the header declares case-for-case identical to objectstack's and which had drifted from it.
POSIX: inside `"…"` a backslash keeps its meaning only before `"`, `\`, `$` and a backtick.
Both quote-aware passes (`split_segments()` and `tokenize()`) used to end the string at the
`\"`, after which the rest of a `node -e "…"` program was read as bare shell.
Measured against this repo's pre-fix copy, the desync failed in BOTH directions:
node -e "const j=require(\"./a.json\"); console.log(j.x.map(st=>st.a)…)"
-> BLOCKED, target: st.a).join(" (a pure read — the false positive)
sed -i "s/\"a\"/\"b\"/" pkg/x.ts
-> ALLOWED (a real write — the guard MISSED it)
The second is the reflexive `sed -i` this guard exists to catch, so the shared checkout was
unguarded against that shape until now.
Self-test extended with PR #10406's two sections (70 -> 82 cases), including the negative
twins so the allow side is widened for escapes only and not for redirection at large.
Localised per the header's convention: issue refs carry the `objectstack#` prefix, and the
non-ASCII paragraph cites this repo's own AGENTS.md prose instead of objectstack's CLI
boot banner, which does not exist here.
Fixes#5459
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0124Qg8rLvpXnQDwCmpKUmaJ
…t commands
Ports objectstack PR #11129 (objectstack#10570) into this repo's copy, the second drift
item between two files the header declares case-for-case identical.
Both quote-aware passes (`split_segments()` and `tokenize()`) now apply the shell's comment
rule: outside quotes and heredoc bodies, an unquoted `#` that starts a WORD begins a comment
running to the next newline.
Measured against this repo's pre-fix copy, from a cwd inside the shared primary checkout:
# rename foo -> bar
echo hello -> BLOCKED, target: bar (a pure read)
echo hello -> allowed (control: no comment line)
Fail-CLOSED, so nothing unguarded slipped through — but it is the false-positive direction
that trains an operator onto OS_ALLOW_MAIN_EDITS=1, which switches the guard off for the
whole command. Prose arrows and a `;` in an explanatory comment are ordinary, so the shape
recurs.
The word-start condition is the whole rule: `foo#bar`, `${x#y}`, `curl 'url/#frag'`,
`sed 's/#//'` and `grep '#'` are not comments and keep every verdict they had. In
`split_segments()` that means `{`/`}` do not reset word state (they are reserved words, not
metacharacters), so the `#` of `${#arr[@]}` cannot swallow a real redirect behind it.
Nothing changes about what the hook blocks once it has a real command; this is only about
what counts as a command. Self-test extended in both directions: 82 -> 100 cases, matching
the upstream count exactly.
Fixes#5712
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0124Qg8rLvpXnQDwCmpKUmaJ
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants

@os-zhuang@hotlong@zhuangjianguo