fix(google_chat): restore nested format_message placeholders in reverse insertion order (#24567)#24582
Closed
briandevans wants to merge 1 commit into
Closed
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a Google Chat outbound formatting bug where nested Markdown conversions could leak placeholder tokens (GC<n>) into the final rendered message by restoring placeholders in the wrong order.
Changes:
- Restore
format_messageplaceholders in reverse insertion order to correctly resolve nested placeholder values. - Add regression tests covering multiple nesting shapes (bold/italic/link/header around inline code/bold) and a realistic multi-line sales update scenario.
- Add targeted assertions that neither NUL sentinels nor bare
GC<n>tokens survive in formatted output.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
plugins/platforms/google_chat/adapter.py |
Reverse placeholder restoration order in format_message to prevent nested placeholder leaks. |
tests/gateway/test_google_chat.py |
Add parameterized + scenario regression tests that reproduce and prevent GC<n> placeholder leakage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This was referenced May 12, 2026
3b5dcd8 to
2f0cb36
Compare
Closed
19 tasks
Contributor
Author
|
CI audit — all 8 failing checks are pre-existing baselines on clean
Nothing in |
2f0cb36 to
274e109
Compare
briandevans
added a commit
to briandevans/hermes-agent
that referenced
this pull request
May 19, 2026
…'t false-positive GitHub re-resolves `pull_request.base.sha` to the current upstream HEAD at workflow-trigger time, not at branch-off time. The supply-chain scanner's `git diff "$BASE".."$HEAD"` (two-dot) therefore includes every commit upstream merged after the PR contributor's last push. Concrete failure mode the scanner is currently hitting: if upstream modifies `hermes_cli/setup.py` (which happens often — ~20 commits in the last 14 days alone), every open PR re-scanned after that point will be flagged with `### 🚨 CRITICAL: Install-hook file added or modified`, even though the PR itself doesn't touch `setup.py`. The "Fail on critical findings" step then blocks the PR until the next scanner run. Switching to three-dot (`A...B`, which git rewrites to `git diff $(merge-base A B)..B`) restricts the diff to changes the PR branch actually introduced. The two file-list scans (`.pth` and install-hook) and the line-level DIFF all get the same fix, so B64_EXEC_HITS / PROC_HITS see consistent input. Evidence on a live PR (NousResearch#24582): 2-dot: hermes_cli/setup.py (false positive — touched by upstream) 3-dot: (empty) (correct — PR doesn't touch any install-hook) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4d9260d to
cc7972b
Compare
…se insertion order (NousResearch#24567) Real-world agent output mixes Markdown constructs — bold spans around inline code, links that wrap bold or inline code, headers containing inline code — and Google Chat users started seeing the raw sentinel token (`GC0`, `GC6`, …) in their rendered messages, e.g.: > * Reactivated Copper Opportunity XXXX (Status moved from Abandoned to GC6). Root cause is in `GoogleChatAdapter.format_message`. Each `_ph` callback runs left-to-right inside a `re.sub` pass and stores a placeholder whose value can embed the keys of previously-inserted placeholders (e.g. a header whose body contained inline code that was already protected). The restore loop iterates `placeholders.items()` in forward insertion order, so when the outer key is substituted into `text`, it re-introduces the inner `\x00GC<n>\x00` marker *after* the inner-key iteration already passed — leaving the sentinel in the returned string. JSON / Google Chat API stripping of the NUL boundary bytes downstream then surfaces the bare `GC<n>` token to the user. Iterating in reverse insertion order is sufficient: each `_ph` callback can only embed keys with *lower* counter numbers (later regex passes see text that already contains earlier placeholder keys, never vice versa). Substituting outer (highest counter) first re-introduces each inner key into `text` while the inner key is still pending in the loop. Reproduces with: - `**Use \`active\` mode**` → leaked `*Use \x00GC0\x00 mode*` - `***Use \`mode\` here***` → leaked `*_Use \x00GC0\x00 here_*` - `[See **important** docs](url)` → leaked `<url|See \x00GC0\x00 docs>` - `[Run \`cmd\`](url)` → leaked `<url|Run \x00GC0\x00>` - `## Status: \`active\`` → leaked `*Status: \x00GC0\x00*` With the fix, every one of those plus the three-deep `[See **\`cmd\` mode**](url)` case round-trip cleanly. Tests: parametrized regression coverage for each nesting shape plus a realistic sales-update message body that mirrors the reporter's symptom, both asserting (a) the expected Chat-dialect output and (b) that neither `\x00` nor a bare `GC<digit>` token survives in the rendered output. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cc7972b to
054ecf9
Compare
Contributor
Author
|
Closing this to keep the contribution queue manageable while it awaits maintainer review. The fix still applies cleanly and I'm happy to reopen if a maintainer wants to pick it up — just give it a nudge and I'll refresh it against main. |
|
is it going to get found that way? I'll keep my subscribe on it, but, i doubt any maintainer is looking at closed issues. |
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
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.
What does this PR do?
Reverses the
format_messageplaceholder-restore iteration so outer placeholders whose values embed earlier (inner) placeholder keys are substituted first, re-introducing the inner key intotextwhile the inner key is still pending in the loop.GoogleChatAdapter.format_messageprotects Markdown constructs from cross-conversion by substituting them with\x00GC<n>\x00sentinels via the_phhelper, then restoring values at the end. Each_phcallback runs left-to-right inside are.subpass, so a later placeholder's value can contain earlier keys — e.g.**Use \active` mode**first becomes\x00GC0\x00 = `active`(inline-code pass) then\x00GC1\x00 = Use \x00GC0\x00 mode(bold pass). The restore loop iteratedplaceholders.items()in forward insertion order, so when the loop reached\x00GC0\x00that marker was not yet intext— the iteration was a no-op. Then\x00GC1\x00was substituted, writingUse \x00GC0\x00 modeback intotext— but inner key's iteration already passed. JSON serialization / the Google Chat REST API silently strips NUL bytes downstream, surfacing the bareGC` token in the rendered message (direct quote from #24567 reporter's session: "Status moved from Abandoned to GC6").One-line semantic change: iterate
reversed(placeholders.items()). The_phcallbacks can only produce values that embed keys with lower counter numbers, so substituting outermost first re-introduces inner keys while every inner key is still pending in the loop.Related Issue
Fixes #24567
Type of Change
Changes Made
plugins/platforms/google_chat/adapter.py—for key, value in reversed(placeholders.items()):informat_message's restore loop.tests/gateway/test_google_chat.py— 8 new cases inTestFormatMessagecovering bold-around-inline-code, bold-italic-around-inline-code, link-around-bold, link-around-inline-code, header-with-inline-code, three-deep link→bold→inline, and a realistic sales-pipeline body. Each asserts both expected Chat-dialect output and that neither a raw\x00nor a bareGC<digit>token survives.How to Test
uv run --with pytest --with pytest-xdist --with pytest-asyncio python3 -m pytest tests/gateway/test_google_chat.py::TestFormatMessage -v**Use \active` mode**→'Use \x00GC0\x00 mode'(sentinel leak). After fix:'Use `active` mode'`. Reproduced for all 6 nesting patterns and the realistic scenario.Checklist
Code
fix(scope):,feat(scope):, etc.)Documentation & Housekeeping
docs/, docstrings) — N/Acli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/Areversed(dict.items())supported since Python 3.8 (project requires ≥3.11)Related / Positioning
tests/gateway/test_google_chat.py(TestPlatformRegistration::test_enum_value,TestEnvConfigLoading::*,TestAuthorizationEmailMatch::*) reproduce on cleanorigin/mainunder the local.[all,dev]install matrix and are unrelated to this change.