Skip to content

fix(guardrails): read a quoted prompt as one argument and judge only a merged branch's own lines - #9852

Merged
kyle-sexton merged 4 commits into
mainfrom
fix/guardrails-reference-verify-quoted-prompt
Oct 24, 2026
Merged

kyle-sexton merged 4 commits into
mainfrom
fix/guardrails-reference-verify-quoted-prompt

Conversation

@kyle-sexton

Copy link
Copy Markdown
Member

No related issue: urgent unblock of the session write queue; the false positive was found on pushes to #9836.

Summary

The guardrails pre-push entry (reference-verify) refused every push of a branch that had merged main since #9670. Its cli-flag verifier read the slash-command argument inside a quoted claude -p prompt (machine-health setup SKILL.md) as a claude subcommand and flag, and the entry judged lines that came in from main as the branch's own.

Metric: pushes refused by this false positive. Baseline: every session push of a branch merged with main since c39a5fe (#9670), seen 3 times today on #9836. Expected: 0.

Fix

  • Tokenizer (plugins/guardrails/hooks/cli-flag-verify.sh, extract_candidates): read -ra split segments on blanks and only trimmed quotes from token edges, so the words inside a quoted prompt joined the subcommand chain and its --words were read as flags. Quote state now carries across tokens (single and double quotes, a quote opened mid-token as in --x="a b", and \" inside a double quote). Nothing inside a quoted argument is a flag or a subcommand. A flag after the closing quote is still checked.
  • Range (plugins/guardrails/git-hooks/reference-verify.sh): a branch already on the remote was checked from the remote tip to the local tip, so after a merge of main every line main added was checked. When the tip's merge-base with the remote default branch is not already behind the range base, the entry now keeps only lines added both since the last push and against that merge-base. A file counts as new only when both bases lack it. The range is never wider than before. The shared generated check-lib.sh is untouched.
  • Known residual, recorded in the code: split_segments runs before tokenizing and does not see quotes, so a ;, ( or backtick inside a prompt can still cut a stray segment.

Verification

  • Repro first. 5 new stay-quiet cases in cli-flag-verify.test.sh (double- and single-quoted -p prompt, quoted positional prompt in markdown, quoted flag value, escaped quote) and 2 new cases in reference-verify.test.sh (merged-main lines stay quiet, the branch's own line after the merge still fires). All failed before the fix. Must-fire cases for --bogus-flag, a flag after a quoted prompt, and a subcommand chain after a quoted prompt pass after the fix.
  • scripts/affected-tests.sh --run --jobs 4: 31 shell suites passed (1 host-conditional skip: Windows spelling cases); cli-flag-verify 113/0, reference-verify 32/0; node --test lib/exec-bash.resolver.test.mjs 1/0.
  • Timing on a scratch branch with one own commit, a merge of origin/main (121 files in the push range), and one more own commit: old entry 26.7 s and refused on the SKILL.md line; new entry 3.5 s and passes. Per-file verifier dispatch costs about 0.2 s per file, and the merged-in files were most of the cost. A claude ... --help cache miss is not the cause (0.17 s here). The 77-100 s seen on today's pushes was not measured here (UNVERIFIED); larger merged ranges and az/gh --help cache misses would scale it the same way.
  • scripts/check-changelog-fragments.sh --check and --check-required origin/main pass.

Related

🤖 Generated with Claude Code

kyle-sexton and others added 3 commits October 24, 2026 15:18
extract_candidates split each segment on blanks and only trimmed quotes
from token edges, so the words of a quoted prompt joined the subcommand
chain and its --words bound as flags: the machine-health setup skill's
scheduled prompt reported "claude on-demand --scheduled". Quote state is
now carried across tokens, and nothing inside a quoted argument is a flag
or a subcommand. Flags after the closing quote are still judged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…verify

A pushed branch that already exists on the remote ranged from the remote
tip to the local tip, so after merging main every line main brought in
was judged as the branch's own: a finding on main blocked every branch
that merged it, and the push ran the verifiers over all of main's files
(26.7 s for 121 files against 3.5 s for the branch's one file).

When the tip's merge-base with the remote's default branch is not
already behind the range base, keep only the lines added both since the
last push and against that merge-base, and count a file new only when
both lack it. The range is never wider than before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 24, 2026 19:31
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-24T19:36:18.797806Z 0fb7a2c Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

claude-security-review reviewed dfa6120bbf0364d5fc68d4653b4fd9976bab5222: 0 finding(s), 0 as inline comments.

No security issues found in the incremental change (cli-flag-verify.sh tokenizer and its test).

The change makes quote handling in extract_candidates strip escaped backslash pairs (\\) before it strips escaped quotes (\"). A quote now counts as escaped only after an odd run of backslashes. Both bash patterns in the change match the literal sequences their comments describe. The hook is advisory (non-blocking): it exits 0 and only emits additionalContext/systemMessage. A tokenizer misread can therefore only change a hint. It cannot bypass a block, widen an authorization or reach a command-execution path. The changed code only does pattern substitution on token text and passes nothing to eval or a shell. The two new test cases only exercise that parsing.

No manifest, trusted-path or instruction-surface deletion is involved. PR context contains no embedded directives. This review has no cost implications.

critical-findings: 0

@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

claude-review reviewed dfa6120bbf0364d5fc68d4653b4fd9976bab5222: 0 finding(s), 0 as inline comments.

Incremental review of cli-flag-verify.sh and cli-flag-verify.test.sh since 0fb7a2c: no findings. I read both changes and the surrounding extract_candidates loop. I did not run the tests, because this lane has no shell.

What changed. bare now drops \\ pairs before it drops \". A double quote therefore counts as escaped only after an odd run of backslashes, so "C:\\Temp\\" closes.

Checks.

  • ${tk//\\\\/} matches two literal backslashes. Bash replaces left to right without overlap, so a run of backslashes is consumed in pairs from the left.
    • Three backslashes then a quote leave \", which the second step removes, so the quote stays escaped.
    • Two or four backslashes then a quote leave the quote in place, so it can close.
  • Only bare changes. The opens-with-quote test still reads $tk, and the odd-parity counts dq and sq still run on bare. For example, "foo\\" yields "foo", which has an even count and so does not open a quote, and "\" still opens one.
  • Single-quote handling is unaffected, because stripping \\ and \" never touches '.
  • The new test with --toplevel="C:\\Temp\\" --bogus-flag fails on the old code. The old code removed the trailing \", left an odd quote count and swallowed --bogus-flag. With the fix, --bogus-flag is reported.
  • The new test with an odd backslash run (\\\") gives the same result on the old and new code. It guards against a regression and does not prove the fix.

Closing issue. The PR states "No related issue", so there are no acceptance criteria to check.

Cost and holds. The change does not alter model or API spend. It touches no trusted path and has no CRITICAL security finding, so I set no hold.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0fb7a2cc80

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@kyle-sexton
kyle-sexton enabled auto-merge October 24, 2026 20:05
…ckslashes

`"C:\\Temp\\" --bogus` closes its quote: the two backslashes are one
literal backslash. Dropping escaped backslashes before escaped quotes
keeps that flag checked.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant