Terminate approval patterns at multi-line args and summarize them in display text#1407
Merged
Aaronontheweb merged 3 commits intoJun 15, 2026
Merged
Conversation
…display text (netclaw-dev#1402) Multi-line quoted strings (message bodies, inline scripts) broke the shell approval flow at two layers: - Stored patterns: ReconstructClauseText appended arg.Raw verbatim, so a command like `freshdesk ticket reply --message "Hi,\n..."` persisted the full multi-line blob as the approval pattern — every unique body became a new pattern, defeating pattern-based auto-approval. The blob is now a termination condition, same mechanism as the digit-bearing value rule: the pattern stops at the preceding flag. The same rule guards redirect targets, whose quoted form survives normalization (`>> "$LOGDIR\nfile"` previously persisted a newline-bearing pattern). - Display text: FormatForDisplay returned the raw command including embedded newlines, which Slack/Discord/Mattermost dump into single-line code fences. Multi-line commands are now rebuilt from the parse tree — statement separators render as explicit operators and multi-line args/redirect targets become "(N lines, M chars)" size summaries, with a flatten fallback on Windows or parser failure. Line-break detection covers lone CR as well as LF: a bare carriage return corrupts patterns the same way and enables cursor-repositioning spoofing in terminal-rendered prompts. Two command shapes skip display reconstruction and fall back to the flattened raw command, because a rebuild would misrepresent what runs: heredocs (the parser drops the body from the tree — a reconstruction would hide the executable payload from the approver) and subshells (grouping does not survive the flat clause list — a reconstruction would misstate which statements a pipe or && guard applies to). The clause after a closed subshell also carries CompoundOperator.None, which the display walk now renders as a statement separator instead of throwing into its own catch. Parse-or-fail-safe handling is centralized in TryParseCommand. Note: the digit-bearing rule already kept patterns clean when an ID preceded the quoted arg (e.g. `... reply 605 --message "..."`); the pattern bug only manifested without one. Spec amended accordingly.
e0a6c35 to
9fc6b66
Compare
This was referenced Jun 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1402
Problem
Multi-line quoted strings (message bodies, inline scripts) broke the shell approval flow at two layers:
ReconstructClauseTextappendedarg.Rawverbatim, sofreshdesk ticket reply --message "Hi,\n..."persisted the full multi-line blob as the approval pattern. Every unique body became a new pattern, defeating pattern-based auto-approval and bloating the store.FormatForDisplayreturned the raw command including embedded newlines, which Slack/Discord/Mattermost dump into single-line code fences, corrupting the prompt layout.Note: for #1402's exact command the stored pattern was already clean — the digit-bearing
605terminates the walk before--message. The pattern bug only manifests when no digit-bearing token precedes the quoted arg.Changes
All in
ShellApprovalMatcher(src/Netclaw.Security/IToolApprovalMatcher.cs); no channel renderer changes needed sinceFormatForDisplayfeeds all three plus the TUI.Pattern path:
freshdesk ticket reply --message).>> "$LOGDIR\nfile"previously persisted a newline-bearing pattern).Display path:
(N lines, M chars)size summaries.&&scope).CompoundOperator.Noneon a non-first clause (emitted after a closed subshell) renders as a statement separator instead of throwing into the method's own catch.Robustness:
ContainsLineBreak): a bare carriage return corrupts patterns identically and enables cursor-repositioning spoofing in terminal-rendered prompts.TryParseCommand.Example
Command:
freshdesk ticket reply 605 --message (2 lines, 42 chars)freshdesk ticket reply --messageTesting
ShellApprovalMatcherTestscovering pattern termination (flag/digit-ID/redirect/CR variants), display summarization, heredoc and subshell fallbacks, and statement-separator rendering — 592/592 passingdotnet slopwatch analyzeclean, copyright headers verifiedSpec
openspec/specs/tool-approval-gates/spec.mdamended in the same PR: line-break termination rule (LF or CR, args and redirect targets), single-line display reconstruction requirement, heredoc/subshell fallback rule, plus scenarios for each.Deferred
Single-line quoted free-text args still inflate patterns (same class, no line break) — filed as #1406 since the fix changes grant semantics for things like
git commit -m "fix"and deserves its own design discussion.