Skip to content

fix(cli): preserve shaping joiners in human-readable results - #76

Open
rudycelekli wants to merge 1 commit into
openclaw:mainfrom
rudycelekli:fix/goplaces-cli-unicode-shaping-20261006
Open

rudycelekli wants to merge 1 commit into
openclaw:mainfrom
rudycelekli:fix/goplaces-cli-unicode-shaping-20261006

Conversation

@rudycelekli

Copy link
Copy Markdown

Problem

Human result rendering removes every Unicode Cf character. That changes emoji sequences such as 👩‍🔬 into 👩🔬 and drops ZWNJ used in Persian shaping from names and addresses. The JSON output retains the original text, so human and machine consumers receive different names.

Change and security boundary

Preserve only ZWNJ U+200C and ZWJ U+200D in human-readable result text. The Unicode Standard's Arabic/Persian discussion describes the shaping role of these characters; its implementation guidance also describes ZWJ's role in emoji sequences.

Keep parser/error diagnostics on the existing strict sanitizer, removing all Cf including joiners. ESC, C0/C1 controls, bidi controls, and every other Cf remain removed in human results as well. JSON output is unchanged. This preserves the diagnostic security boundary introduced in #53 and the bidi protection introduced in 47e9e25, while avoiding legitimate result-text corruption. This is display text handling, not a general anti-confusable or identifier-validation guarantee.

Verification

  • Focused regressions failed on baseline 9883e1044f504a19ab440e9c909dbf1c5d9c1a1f before the fix.
  • Exact signed head 09b976240cfac03dc867133619d4c141d4b2b285 passed go build ./..., go test ./..., go test -race ./..., make coverage (92.4%, required90%), and make lint-check with the pinned golangci-lint v2.13.2.
  • The compiled CLI reached an owned HTTP fixture through real request/response mapping. Human output preserves the two shaping code points after the fix; JSON is byte-identical; error output is byte-identical and still strips all Cf. No authenticated Google calls.

AI assistance was used for implementation and verification. The source patch received separate review before publication.

Inspectable compiled CLI transcript

Compiled with go build -o goplaces-after ./cmd/goplaces from 09b976240cfac03dc867133619d4c141d4b2b285. Updated binary SHA-256: 5bd935dc38ccad65f44c5c3412b5a54cecb5cf82ac91769a225949b2c766ac47. <owned-local-base> replaces the process-owned ephemeral loopback endpoint; the key owned-local-test is synthetic. The fixture returns displayName.text and formattedAddress containing 👩‍🔬 می‌خواهم, plus adversarial RLO/LRI/ZWSP/BOM/ESC/BEL controls in the name. Error-control requests return HTTP400 with those format controls in their JSON error body. This proves exact code-point preservation, not rendering support in every terminal font.

$ ./goplaces-before search coffee --api-key=owned-local-test --base-url=<owned-local-base> --no-color
fixture_case=human
exit=0 server_requests=1
stdout="Results (1)\n1. \ud83d\udc69\ud83d\udd2c \u0645\u06cc\u062e\u0648\u0627\u0647\u0645[31m \u2014 \ud83d\udc69\ud83d\udd2c \u0645\u06cc\u062e\u0648\u0627\u0647\u0645\nID: owned\n\n"
stderr=""
contains_U+200D=false contains_U+200C=false
$ ./goplaces-before search coffee --api-key=owned-local-test --base-url=<owned-local-base> --json
fixture_case=json
exit=0 server_requests=1
stdout="{\n  \"results\": [\n    {\n      \"place_id\": \"owned\",\n      \"name\": \"\ud83d\udc69\u200d\ud83d\udd2c \u0645\u06cc\u200c\u062e\u0648\u0627\u0647\u0645\u202e\u2066\u200b\ufeff\\u001b[31m\\u0007\",\n      \"address\": \"\ud83d\udc69\u200d\ud83d\udd2c \u0645\u06cc\u200c\u062e\u0648\u0627\u0647\u0645\"\n    }\n  ]\n}\n"
stderr=""
contains_U+200D=true contains_U+200C=true
$ ./goplaces-before search coffee --api-key=owned-local-test --base-url=<owned-local-base> --no-color
fixture_case=error
exit=1 server_requests=1
stdout=""
stderr="goplaces: api error (400): {\"error\": {\"message\": \"safe\\u001b[31m\\u0007\"}}\n"
contains_U+200D=false contains_U+200C=false
$ ./goplaces-after search coffee --api-key=owned-local-test --base-url=<owned-local-base> --no-color
fixture_case=human
exit=0 server_requests=1
stdout="Results (1)\n1. \ud83d\udc69\u200d\ud83d\udd2c \u0645\u06cc\u200c\u062e\u0648\u0627\u0647\u0645[31m \u2014 \ud83d\udc69\u200d\ud83d\udd2c \u0645\u06cc\u200c\u062e\u0648\u0627\u0647\u0645\nID: owned\n\n"
stderr=""
contains_U+200D=true contains_U+200C=true
$ ./goplaces-after search coffee --api-key=owned-local-test --base-url=<owned-local-base> --json
fixture_case=json
exit=0 server_requests=1
stdout="{\n  \"results\": [\n    {\n      \"place_id\": \"owned\",\n      \"name\": \"\ud83d\udc69\u200d\ud83d\udd2c \u0645\u06cc\u200c\u062e\u0648\u0627\u0647\u0645\u202e\u2066\u200b\ufeff\\u001b[31m\\u0007\",\n      \"address\": \"\ud83d\udc69\u200d\ud83d\udd2c \u0645\u06cc\u200c\u062e\u0648\u0627\u0647\u0645\"\n    }\n  ]\n}\n"
stderr=""
contains_U+200D=true contains_U+200C=true
$ ./goplaces-after search coffee --api-key=owned-local-test --base-url=<owned-local-base> --no-color
fixture_case=error
exit=1 server_requests=1
stdout=""
stderr="goplaces: api error (400): {\"error\": {\"message\": \"safe\\u001b[31m\\u0007\"}}\n"
contains_U+200D=false contains_U+200C=false

Signed-off-by: Rudy Celekli <rudy@gradiahq.com>
@clawsweeper

clawsweeper Bot commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Oct 6, 2026
@clawsweeper

clawsweeper Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed October 6, 2026, 3:35 AM ET / 07:35 UTC.

ClawSweeper review

What this changes

The CLI preserves Unicode shaping joiners in human-readable results while continuing to strip them from diagnostics, with regression tests and a changelog entry.

Merge readiness

✅ Ready for maintainer review

This PR remains necessary: current main and v0.4.11 still remove shaping joiners. The focused patch preserves the diagnostic boundary, and the supplied compiled-CLI transcript demonstrates the corrected behavior. No blocking defect was found.

Priority: P2
Reviewed head: 09b976240cfac03dc867133619d4c141d4b2b285

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused implementation with meaningful before/after CLI proof and no identified blocking defect.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The supplied pinned-head compiled-CLI transcript exercises search through real HTTP response mapping and shows preserved joiners after the fix, continued control removal, and unchanged JSON/error output. No stored-data contract changes.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The supplied pinned-head compiled-CLI transcript exercises search through real HTTP response mapping and shows preserved joiners after the fix, continued control removal, and unchanged JSON/error output. No stored-data contract changes.
Evidence reviewed 7 items Introduced patch: The pinned main-to-head diff changes only the changelog, three regression tests, the shared display sanitizer, and the diagnostic sanitizer call. Production delta is +12/-2 lines; tests add 40 lines.
Still needed on main: Main removes every Unicode Cf character in human output. The live GitHub main SHA matches the pinned base, so the joiner exception has not landed.
Latest release retains the defect: GitHub identifies v0.4.11 as the latest release; its source still removes all Cf characters. This PR's behavior is therefore absent from both the latest release and current main.
Findings None None.
Security None None.

How this fits together

goplaces maps Google Places responses into names, addresses, and other result fields. The CLI renders those fields as human-readable terminal text or JSON and separately sanitizes errors.

flowchart TD
  A[Places HTTP response] --> B[Typed result mapping]
  B --> C[CLI output choice]
  C --> D[Human text sanitizer]
  C --> E[JSON output]
  D --> F[Terminal results]
  G[Parser and API errors] --> H[Strict diagnostic sanitizer]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test LOC production +12/-2, tests +40/-0 The small production increase separates display filtering from strict diagnostics and has focused regression coverage.

Technical review

Best possible solution:

Preserve legitimate shaping characters in displayed results while retaining strict diagnostic filtering and unchanged JSON contracts.

Do we have a high-confidence way to reproduce the issue?

Yes: main's shared human sanitizer unconditionally removes Cf characters, including both joiners. The supplied baseline CLI transcript confirms the resulting corruption; this reviewer did not execute it.

Is this the best way to solve the issue?

Yes: a two-character exception for human output plus a separate strict diagnostic path is a narrow repair that preserves existing security and JSON behavior.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against 9883e1044f50.

Labels

Label changes:

  • add P2: This repairs corrupted human-readable names and emoji with a limited CLI-output blast radius.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The supplied pinned-head compiled-CLI transcript exercises search through real HTTP response mapping and shows preserved joiners after the fix, continued control removal, and unchanged JSON/error output. No stored-data contract changes.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The supplied pinned-head compiled-CLI transcript exercises search through real HTTP response mapping and shows preserved joiners after the fix, continued control removal, and unchanged JSON/error output. No stored-data contract changes.

Label justifications:

  • P2: This repairs corrupted human-readable names and emoji with a limited CLI-output blast radius.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The supplied pinned-head compiled-CLI transcript exercises search through real HTTP response mapping and shows preserved joiners after the fix, continued control removal, and unchanged JSON/error output. No stored-data contract changes.
  • proof: sufficient: Contributor real behavior proof is sufficient. The supplied pinned-head compiled-CLI transcript exercises search through real HTTP response mapping and shows preserved joiners after the fix, continued control removal, and unchanged JSON/error output. No stored-data contract changes.

Evidence

What I checked:

  • Introduced patch: The pinned main-to-head diff changes only the changelog, three regression tests, the shared display sanitizer, and the diagnostic sanitizer call. Production delta is +12/-2 lines; tests add 40 lines. (internal/cli/render.go:453, 09b976240cfa)
  • Still needed on main: Main removes every Unicode Cf character in human output. The live GitHub main SHA matches the pinned base, so the joiner exception has not landed. (internal/cli/render.go:466, 9883e1044f50)
  • Latest release retains the defect: GitHub identifies v0.4.11 as the latest release; its source still removes all Cf characters. This PR's behavior is therefore absent from both the latest release and current main. (internal/cli/render.go:466, c0cdace7197b)
  • Diagnostic security boundary: All parser and application error calls still pass through writeError, which now selects strict filtering. Human filtering permits only U+200C and U+200D; controls and other Cf characters remain removed. The related merged security work is fix(security): scope API redirects and sanitize diagnostics #53. (internal/cli/run.go:147, 09b976240cfa)
  • Real compiled-CLI proof: The complete supplied PR body records before/after search commands against an owned HTTP fixture using the compiled pinned head. After the fix, human names and addresses contain U+200C/U+200D, adversarial controls remain stripped, and JSON and API-error output remain identical. The real request path maps displayName.text and formattedAddress into the fields consumed by renderSearch. This proves code-point preservation without claiming universal terminal-font support. (internal/cli/search.go:62, 09b976240cfa)
  • Feature history and routing: The feature-history log associates terminal hardening, the CLI refactor, and diagnostic security work with Peter Steinberger; GitHub commit metadata maps the terminal-hardening author to steipete. Local historical patch/blame inspection encountered unavailable promisor objects, so no source-line introduction pointer is asserted. (internal/cli/render.go, 47e9e25d1fa9)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant