fix(desktop): keep quoted literals in agent-activity send previews - #6841
fix(desktop): keep quoted literals in agent-activity send previews#6841baxen wants to merge 2 commits into
Conversation
An agent posting ordinary markdown lost the text of its own message in the activity view. `--content 'Fixed `labelForStatus` in the rail'` renders with no message in it, while the same send in plain prose renders fine. The cause is that the guard added by #2201 tests characters, not meaning. It rejects any `--content` containing `$` or a backtick so that `--content "$(cat file)"` and `"$MESSAGE"` can never be displayed as if the unexpanded shell expression were the sent text. That intent is right and is kept. But `tokenizeShellCommand` discards quoting, so by the time the guard runs a single-quoted literal backtick -- ordinary markdown the shell never touches -- is indistinguishable from a real substitution, and both are dropped. Since agents write markdown by default, the rejected case is the common one. Quote context is therefore preserved through tokenization. Each token now carries `hasSubstitution`, set only where the shell could actually expand something: `$` and a backtick unquoted or inside double quotes. Inside single quotes, or after a backslash, they are literal text and the preview survives. The guard reads that flag instead of scanning characters, so it still rejects every substitution #2201 was about -- `$(cat file)`, `$MESSAGE`, bare and prefixed, and a token that concatenates a quoted literal with an unquoted variable -- and those four tests continue to pin it. The `$` rule stays deliberately conservative: any unescaped `$` outside single quotes flags the token, including places bash would leave it literal. Showing a shell expression as the sent message is the failure being prevented, so it errs towards dropping the preview. Verified against bash rather than against a reading of it: for nine command lines, `bash` was asked for the real argv entry it passes as `--content`. Every preserved preview is byte-identical to that argv, and every case where the shell changed the text yields `null`. Three mutants, each producing a distinct failure set: restoring the character test fails the four new quoted-literal tests; making substitution tracking never fire fails eight, four of them #2201's own; and making single quotes stop protecting fails four. `tokenizeShellCommand` is kept as a string-returning wrapper so #2201's tokenizer test still exercises the same path. Not addressed here, and still open in #6834: the rail's missing event-fetch fallback (part b, gated on a design call) and the latent dot/underscore `isBuzzMessageSend` mismatch, which has no user-visible path on main. Desktop suite 5516 passing / 0 failing; typecheck, check and the file-size ratchet clean. Refs #6834 Signed-off-by: ss-dev-01 <11939edb7df583f855dbef923f2358f1184538f88ca452e19e7e35e42ad6d796@buzz.block.builderlab.xyz> Co-authored-by: Bradley Axen <baxen@squareup.com> Signed-off-by: Bradley Axen <baxen@squareup.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60642ca333
ℹ️ 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".
Codex found a hole in the previous commit. Backslash-escaping earned literal
credit -- `--content "literal \$MESSAGE"` was displayed as sent text -- on the
assumption that a backslash escape is portable. It is not, and `BUZZ_SHELL`
explicitly supports the shells where it is not (`buzz-dev-mcp/src/shell.rs`
dispatches `-Command` for powershell/pwsh and `/C` for cmd).
Verified by running PowerShell 7 rather than reasoning about it:
'Fixed `labelForStatus` in the rail' -> Fixed `labelForStatus` in the rail
'Costs $5 per run, not $MESSAGE' -> Costs $5 per run, not $MESSAGE
"literal \$MESSAGE" -> literal \ (variable EXPANDED)
So single quoting is literal in PowerShell too, and the fix this PR exists for
is portable. Backslash is not an escape character there at all: the shell keeps
the backslash and still expands the variable, which is exactly the #2201 failure
the guard prevents, and the old character test returned null for it.
Escaped `$` and backtick therefore stay flagged as substitutions even though
bash reads them as literal. Only single quoting is trusted, because only single
quoting means the same thing in every shell an operator can select. The affected
test now asserts null and says why.
The bash-argv probe still agrees on all nine command lines, with the escape case
marked as intentionally more conservative than bash. Mutating the new guard away
fails exactly one test -- the new one -- so it is not vacuous.
Desktop suite 5516 passing / 0 failing; typecheck, check and the file-size
ratchet clean.
Refs #6834
Signed-off-by: ss-dev-01 <11939edb7df583f855dbef923f2358f1184538f88ca452e19e7e35e42ad6d796@buzz.block.builderlab.xyz>
Co-authored-by: Bradley Axen <baxen@squareup.com>
Signed-off-by: Bradley Axen <baxen@squareup.com>
Chessing234
left a comment
Chessing234
left a comment
There was a problem hiding this comment.
this is the right diagnosis. the guard was written against characters because tokenizeShellCommand had already thrown away the only thing that could answer the question, and carrying hasSubstitution out of the tokenizer puts the decision where the information is. the test that pins the mixed case — 'a literal '"$MESSAGE" — is the one that convinced me, because that's where a naive per-token flag would have leaked.
rejecting the backslash-escaped form as non-portable is also the right call, and the comment explaining it (bash literal, powershell not) is worth having.
one gap i'd want acknowledged, because the pr reframes what the guard means:
the old code said "reject $ or backtick", which was obviously a heuristic. the new comment says "quoting decides this, not the characters: a single-quoted or escaped $ / backtick is literal text the shell never touched". that reads as a general guarantee, and hasSubstitution still only tracks $ and backticks. the shell rewrites unquoted tokens in several other ways:
--content ~/notes -> preview "~/notes", sent "/Users/me/notes"
--content report_*.md -> preview "report_*.md", sent whatever globbed
--content {a,b} -> preview "{a,b}", sent "a b"
none of those are new — main has the same holes — and they're much rarer in agent output than a markdown backtick, so i don't think they block. but the comment now invites the next reader to trust the flag as "the shell couldn't have changed this", and it doesn't mean that. either narrow the wording to the two constructs it actually tracks, or set the flag for an unquoted leading ~, */?/[, and {, } too — the tokenizer already knows the quote state, so it's cheap.
smaller: const group = tokens[range.groupIndex].value drops the optional access that the line below keeps (tokens[range.verbIndex]?.value ?? "run"). findBuzzCommand presumably guarantees groupIndex is in range, and main indexed it unguarded too, so this isn't a regression — but the two adjacent lines now disagree about whether the range is trustworthy, and one of them will be wrong if findBuzzCommand ever changes.
and worth exporting tokenizeShellCommandTokens with a short doc comment on hasSubstitution saying what it does and does not cover — it's a public API now, and it's the kind of flag someone will reach for from another call site expecting the general guarantee.
2 participants
The bug
An agent posting ordinary markdown loses the text of its own message in the agent activity view. These two sends differ only in the body:
Agents write markdown by default, so the broken case is the common one. This is part (a) of #6834.
Why
The guard is right in intent, but it tests characters instead of meaning. #2201 (
c5d00a358) added it so--content "$(cat file)"and"$MESSAGE"could never be displayed as if the unexpanded shell expression were the sent text — that intent is kept, and its tests stay green.The problem is upstream:
tokenizeShellCommanddiscards quoting. By the time the guard runs, a single-quoted literal backtick — text the shell never touches — is indistinguishable from a real substitution, so both get dropped.The fix
Preserve quote context through tokenization. Each token now carries
hasSubstitution, set only where the shell could actually expand something —$or a backtick unquoted or inside double quotes. Inside single quotes, or after a backslash, they are literal and the preview survives. The guard reads that flag instead of scanning characters.Only single quoting is trusted, because only single quoting is literal in every shell
BUZZ_SHELLcan select. A backslash escape is not portable — verified by running PowerShell 7, where"literal \$MESSAGE"keeps the backslash and expands the variable — so escaped$/backtick stay flagged even though bash reads them as literal. (Codex caught this; see the resolved thread. First push credited\$as a literal, which would have reintroduced the #2201 failure underBUZZ_SHELL=pwsh.)The
$rule is deliberately conservative in the same direction: any$outside single quotes flags the token, including places bash would leave it literal (a trailing$,$before a space). Showing a shell expression as the sent message is the failure being prevented, so it errs towards dropping the preview.Going the other way — teaching the classifier the selected shell dialect — is not available: it sees only the command string, with no record of which shell ran it.
tokenizeShellCommandis retained as a string-returning wrapper over the newtokenizeShellCommandTokens, so #2201's tokenizer test still exercises the same code path. It has no other callers indesktop/src.Verification
Checked against bash, not against a reading of bash. For nine command lines, bash was asked for the real argv entry it passes as
--content. Contract: if the shell changed the text, the preview must benull; if it did not, the preview must equal that argv byte for byte.*= intentionally more conservative than bash, per the cross-shell finding above.An earlier version of this probe read the wrong argv position and failed loudly on 4/9 rather than passing falsely — noting that because a probe that silently agrees is the one to distrust.
Mutants — three, each with a distinct failure set (mutated line printed before each run, so a silently-failed revert can't contaminate the next):
includes("$") || includes("")`isSubstitutionCharalways returnsfalse(guard disarmed)Gates at
9dc2ec80d, read in the same shell asgit rev-parse HEAD, tree clean:tsc --noEmit,pnpm check,pnpm check:file-sizesall clean. The 2 biome warnings are pre-existing insidebar/onboardingfiles this PR does not touch.desktop-check,desktop-typecheck,desktop-test(98.76s at head).Scope — deliberately not fixed here
Both are recorded in #6834 and neither is blocked by this change:
main.isBuzzMessageSenddot/underscore mismatch. Real, but I could not find a path onmainthat shows it to a user, so fixing it here would be an unverifiable change.Refs #6834