fix(agent-guard): scope Segment.raw to its own tokens, not the whole command - #1151
Conversation
…command split_segments() passed the entire compound command as `raw` for every segment it produced, so guard_commit_trailer's Co-Authored-By scan could fire on a segment whose own text never mentioned it. raw is now shlex.join(current), a reconstruction of that segment's own tokens. Fixes apache#1150 Generated-by: Claude Code (Sonnet 5)
ac3e993 to
be37147
Compare
potiuk
left a comment
There was a problem hiding this comment.
Good catch, and the diagnosis is exactly right: passing the whole command as every segment's raw meant a substring scan could not tell which segment the text came from, so any mention of the phrase anywhere on the line poisoned every other segment.
Since this is a security guard, I tested the false-negative direction specifically — narrowing raw could plausibly have weakened detection rather than just removing the over-match. It does not:
| Command | Result |
|---|---|
git commit -m "fix\nCo-Authored-By: ..." |
denied |
cd /tmp && git commit -m "x\nCo-Authored-By: a" |
denied |
git commit --amend -m "Co-Authored-By: x" |
denied |
| heredoc commit containing the trailer | denied |
| heredoc commit, clean message | allowed |
echo "...Co-Authored-By..." && git commit -m "clean fix" |
allowed (the fix) |
The heredoc row is the one that mattered: the docstring you updated claims raw exists for "substring scans that survive heredocs", so shlex.join had to preserve that property — and it does, because the heredoc body is already in the segment's own token list.
I also confirmed the bug on main before the fix: that last case is denied there today, so this is a real false positive being removed, not a hypothetical.
Docstring updated to match the new semantics, which is the part these changes usually miss.
🤖 This review was drafted by an AI-assisted tool and may contain mistakes. It has been reviewed and confirmed by an Apache Magpie maintainer before submission. See CONTRIBUTING.md for what this project considers a maintainer review.
|
Good call testing the false-negative direction specifically, the heredoc row is exactly the one I'd have worried about if I'd only checked that the over-match went away. |
The marker sat at a1cff44, 17 commits behind main. Bumping it alone would claim those commits are described by the specs, so the drift is closed first. Specs updated for what actually shipped: - meta-and-quality-tooling: skill-evals errors, rather than passing, when a case's CLI produced no gradeable output (#1161). - security-reporting: the tracker dashboard projects the current partial bucket to its end-of-bucket value, splitting RATE series (accumulate from zero) from LEVEL series (carry over), and deliberately not projecting mean-based signals (#1158). - project-agnosticism: <PROJECT> and <project> are two placeholders holding different values, and the lint carries both spellings plus spaced variants (#1154). - adapters: the forwarder relay's contact_handle defaults to an org-level shared inbox rather than a named individual (#1135). The multi-hop coordinator case is designed in RFC-AI-0008 and unimplemented. - issue-management-family: the family's eval suites, and the note that --cli runs belong outside a credential-denying sandbox (#1145). Commits needing no spec change: #1152, #1143 and #1156 updated their own specs in-commit; #1149, #1147, #1151 are behaviour-preserving bug fixes; #1155 and #1141 are CI and dependency chores; #1159's spec edits landed with it; #1144 removes hardcoded literals that no spec asserted. One genuine gap recorded rather than papered over: no spec covers marketplace distribution or the dev-version stamping rule from #1160, which is load-bearing because `claude plugin update` compares version strings, not commit SHAs. Logged in adoption-and-setup as wanting its own spec. Generated-by: Claude Code (Opus 5)
split_segments()builds everySegmentin a compound command asSegment(current, command), passing the whole input string asrawregardless of which segment it is.guard_commit_trailersearchesseg.rawforco-authored-by:, so it was really searching the entire line, not the commit's own message. Agit commitwith a clean message gets denied if an unrelated earlier segment (a comment, a grep, an echo) happens to contain that phrase.Fixed by giving each segment
raw = shlex.join(current), a reconstruction of that segment's own tokens only. It still supports the heredoc-survival case the docstring describes:shlex.splitdoesn't parse<<specially, so a heredoc's delimiter and body just become ordinary tokens attached to the segment they trail, andshlex.joinputs them back as one searchable string. Checked the field's only other reader,GuardContext.raw: none of the fourguards.d/skill-contributed guards use it, so the fix's blast radius is exactly the one guard this issue is about.Added
test_compound_command_other_segment_not_denied, the new test breaks without the change and passes with it. Fulltools/agent-guardsuite (92 tests), ruff, ruff format and mypy all come back clean in apython:3.11-slimcontainer.Fixes #1150