Sign up
View stargazersSign up

fix(server): PR watch wakes no longer split an emoji in a long comment#29633

merged1
main
GitHub fork: fix/surrogate-hunt
opened9d agobySsaphid
2 files changed
View on GitHubTwo-way sync enabledPR last imported 7d ago

Problem

When an agent watches a pull request, T3 Code wakes it with a message listing new comments, each shortened to 200 characters. The cut counts UTF-16 code units, so when an emoji (common in review-bot comments) straddles the cut, the message keeps only the first half of its surrogate pair.

That message is sent into the agent's thread as a turn. JSON encoding turns the half emoji into a lone \ud83d escape, which strict parsers reject: serde_json (used by the Codex app-server) fails with unexpected end of hex escape, and the Anthropic API rejects requests with no low surrogate in string. The wake meant to get the agent working on review feedback fails instead.

Change

snippet() in apps/server/src/orchestration-v2/pullRequestWatch.ts steps back one code unit when the last kept unit is a high surrogate. It uses the same step-back as OrchestratorMcpService.ts paging, ContextHandoffService.ts and sanitizeThreadTitle.

Scope and approval

Very small, focused fix for an obvious bug, so no prior issue: one truncation site, 5 non-test lines, one regression test. Overlap search: the local PR index and GitHub search found no PR touching pullRequestWatch.ts truncation. Related surrogate fixes (#29158, #29163, #29168, #29563, #29572, #29600, open #24611, #28831) cover other call sites, not this one.

Verification

  • Red on main: the new case in pullRequestWatch.test.ts fails with AssertionError: expected false to be true (text.isWellFormed()).
  • Green: vp test run src/orchestration-v2/pullRequestWatch.test.ts passes 21/21.
  • vp run check:changed: format, lint, knip, typecheck (3 packages), tests (22/22) all pass.
  • Runtime: the main-branch cut, encoded as a turn/start JSON-RPC line and parsed with serde_json::from_str, gives serde_json error: unexpected end of hex escape at line 1 column 284.
  • Not checked: a live Codex or Claude turn woken by a real PR comment.

Reviewed with sol-loop: 1 round with GPT-6.1 Sol on high. Sol verdict: approved.

Created with Claude Opus 5.5 in Claude Code (T3 Code).

🤖 Generated with Claude Code

Checks

24 successful · 14 skipped
All checks passed
58486bb

Added 1 commit

  • fix(server): PR watch wakes no longer split an emoji in a long comment58486bb
MmacroscopeappBot9d ago
Synced from GitHub

Approvability

Verdict: Approved at 58486bb

Macroscope's review found this PR approvable — PR-watch comment excerpts now avoid splitting an emoji at the truncation boundary. The change is confined to message formatting and leaves watch behavior and stored state unchanged.

You can add or adjust custom eligibility rules. Learn more.

CcoderabbitaiBot9d ago
Synced from GitHub

Review in Change Stack →

No actionable comments were generated in the recent review.

External image Recent review info
External image Run configuration
External image Commits

Reviewing files that changed from the base of the PR and between 60aec28a9c6fdb7417a7382ec094ccc4984d0738 and 58486bb58b95fb6210180a8f31ac50bdf80045ce.

External image Files selected for processing: 2
  • apps/server/src/orchestration-v2/pullRequestWatch.test.ts
  • apps/server/src/orchestration-v2/pullRequestWatch.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

Walkthrough

The PR updates PR-watch snippet truncation to avoid splitting a surrogate pair. It adds a test that checks the generated wake message when an emoji falls at the truncation boundary.

Changes: PR-watch snippet truncation

Layer / File(s)Summary
Surrogate-safe truncation and test
apps/server/src/orchestration-v2/pullRequestWatch.ts, apps/server/src/orchestration-v2/pullRequestWatch.test.ts		snippet shortens the truncated text by one character when the boundary would split a surrogate pair. The new test checks the generated message for an emoji at the boundary.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Bug fix

Possibly related issues

  • Issue 27842 describes a similar truncation problem with emoji surrogate pairs. This PR addresses the same type of boundary issue in PR-watch comment snippets.

Suggested reviewers: juliusmarminge

Merge Risk: External image Minimal · up to 58486

No actionable merge-blocking risk remains in the reviewed change.

Pre-merge checks: Passed 4
Check nameStatusExplanation
Title checkPassedThe title clearly and concisely describes the main fix: preventing PR-watch messages from splitting an emoji during truncation.
Description checkPassedThe description includes all required sections: Problem, Change, Scope and approval, and Verification. It explains the failure mode, implementation, rationale for the focused scope, test results, runt…
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description includes all required sections: Problem, Change, Scope and approval, and Verification. It explains the failure mode, implementation, rationale for the focused scope, test results, runtime verification, and the unchecked live-turn scenario.

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Mmaria8d ago

Note

Written by gpt-6.1-sol on behalf of Maria

Verified in a real dev environment

Verified the formatter-to-provider path on Linux at 58486bb58b95fb6210180a8f31ac50bdf80045ce, with the limitation below.

  • Installed and ran github/main (71646e031a) in a separate worktree, then installed and ran the exact PR head. One dev server at a time.
  • Created projects and threads in the real web app using repository Playwright and /usr/bin/google-chrome. Used real Codex gpt-6.1-sol turns through the local provider proxy.
  • Supplied controlled boundary input (196 ASCII characters followed by 🚀 and more) directly to the production pullRequestWatchMessage formatter. Sent its unchanged output through the running server's authenticated WebSocket orchestration.dispatchCommand, then inspected the actual Codex turn/start protocol log and web client.
  • On main, the output contained a lone \ud83d; Codex never acknowledged that turn/start, and the client remained working for at least 95 seconds before teardown. Ordinary turns completed. On this head, the output was well-formed and Codex completed with TRANSPORT_OK.
  • A subsequent ordinary composer turn preserved 🚀 café in the provider response and client. Pasting the malformed main payload through the composer repaired it, so the boundary reproduction used the WebSocket API to preserve the exact string.
  • Blacksmith: cd apps/server && vp test run src/orchestration-v2/pullRequestWatch.test.ts, 21/21 passed. Existing CI reports 31 successful checks. No source changes or pushes were needed.

Sanitized provider transcript

Real Codex transport completion and intact emoji follow-up

Running app accepts the formatted boundary message and completes an ordinary emoji follow-up

Not verified

Automatic wake delivery from a newly posted external PR comment was not exercised. The boundary content was controlled formatter input, not a comment fetched by the watch reactor. Watches ignore the signed-in user's own comments, and the existing external comments do not hit this boundary. This proves the changed formatter and real downstream Codex path, not full external-comment ingestion.

Claude, native iOS/Android, and macOS/Windows desktop clients were not exercised on this Linux VM.