Skip to content

fix(vcard): stop fold loop on invalid UTF-8 - #14

Merged
steipete merged 2 commits into
openclaw:mainfrom
SebTardif:fix/vcard-fold-invalid-utf8
Sep 5, 2026
Merged

fix(vcard): stop fold loop on invalid UTF-8#14
steipete merged 2 commits into
openclaw:mainfrom
SebTardif:fix/vcard-fold-invalid-utf8

Conversation

@SebTardif

Copy link
Copy Markdown
Contributor

What Problem This Solves

clawdex export vcard --all can hang forever when People() salvages a
person.md whose display name is longer than 75 bytes and starts with an
invalid UTF-8 byte.

folded in internal/vcard/vcard.go wraps RFC 6350 lines at 75 octets and
walks cut backward until utf8.ValidString(line[:cut]) so it does not
split a rune. A leading invalid byte makes every nonempty prefix invalid.
cut becomes 0, ValidString("") is true, the function writes a blank
continuation (\r\n ) and does not advance line, then repeats.

The loop has been in place since the CLI bootstrap
(fc837601e1,
2026-05-08). Open PRs #11
and #12 cover avatar reads
and Google HTTP timeouts. They do not touch vCard fold.

Evidence

Built clawdex from this branch and from unpatched origin/main. Initialized
a contacts repo, wrote people/bad-name/person.md with broken YAML so
People() salvages, and set name to byte 0xff plus 80 ASCII a bytes
(81 bytes, over the 75-octet fold limit). Then ran the public export.

Unpatched binary (same fixture, 1 second deadline):

$ clawdex --config config.toml --repo contacts --plain export vcard --all -o out.vcf
TIMEOUT after 1000ms (export did not return)

Patched binary, same fixture and command:

$ clawdex --config config.toml --repo contacts --plain export vcard --all -o out.vcf
exported: 1
out: C:\Users\sebta\AppData\Local\Temp\clawdex-f006-export\out-fixed.vcf

Elapsed 29ms. Output is 288 bytes and starts BEGIN:VCARD / FN: with the
name folded across continuation lines. Export returns instead of spinning.

A standalone go program that calls vcard.Write with
Name: "\xff" + 80*'a' matches that: unfixed fold hits the 400ms deadline;
patched Write returns in 0s with 267 bytes and contains_FN=true.

Real behavior proof

  • Behavior or issue addressed: vCard export no longer hangs when a salvaged person name is longer than 75 bytes and starts with an invalid UTF-8 byte. Fold advances one invalid byte and finishes the address book.

  • Real environment tested: Windows amd64, Go 1.26.6, clawdex built from this branch to bin/clawdex.exe, isolated config under %TEMP%\clawdex-f006-export with auto_repair = false so People() salvages the broken person.md.

  • Exact steps or command run after this patch:

    clawdex --config config.toml init contacts --remote ""
    write people/bad-name/person.md (broken YAML, name = 0xff + 80 a bytes)
    clawdex --config config.toml --repo contacts --plain export vcard --all -o out.vcf
  • Evidence after fix: terminal output from the patched binary:

    $ clawdex --config config.toml --repo contacts --plain export vcard --all -o out.vcf
    exported: 1
    out: C:\Users\sebta\AppData\Local\Temp\clawdex-f006-export\out-fixed.vcf

    Same fixture against the unpatched binary never printed a result (killed after 1s). After the patch the command returns in 29ms and writes a 288-byte .vcf.

  • Observed result after fix: Export completes and writes one vCard. The long FN is folded on 75-octet lines. The rest of the book is not aborted.

  • What was not tested: Google or Apple import of a long invalid-UTF-8 FN (those adapters emit UTF-8). Live Contacts import of the folded .vcf on a phone.

folded walked cut down to 0 when every nonempty prefix was invalid
UTF-8, wrote a blank continuation, and never advanced the line.

Advance one invalid byte so export can finish. People() already
salvages broken person.md names; aborting the whole address book
would drop every later contact.

Signed-off-by: Sebastien Tardif <SebTardif@ncf.ca>
@clawsweeper

clawsweeper Bot commented Sep 5, 2026

Copy link
Copy Markdown

🦞👀
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 Sep 5, 2026
@clawsweeper

clawsweeper Bot commented Sep 5, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed September 5, 2026, 4:10 PM ET / 20:10 UTC.

ClawSweeper review

What this changes

Makes vCard export advance past invalid UTF-8 bytes in damaged contact names, with regression tests and a changelog entry.

Merge readiness

Ready for maintainer review

This remains a useful, focused fix: current main still permits the export loop to stop advancing. The supplied real CLI proof supports completion after the patch, and no blocking defect was found.

Priority: P2
Reviewed head: 9e586b4d18a277b4352d0bf99a341ca78852f7cc

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, source-supported repair with real CLI before-and-after evidence and targeted regression coverage.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The supplied Windows CLI transcript exercises damaged Markdown salvage through actual vCard file export: the baseline times out, while the patched exporter completes in 29 ms. The current head changes only the changelog since that implementation; phone import compatibility is outside the demonstrated termination fix.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The supplied Windows CLI transcript exercises damaged Markdown salvage through actual vCard file export: the baseline times out, while the patched exporter completes in 29 ms. The current head changes only the changelog since that implementation; phone import compatibility is outside the demonstrated termination fix.
Evidence reviewed 7 items Current main still contains the defect: The fold loop can reach cut == 0 on an invalid leading byte, emit a continuation, and repeat without consuming input.
Introduced change and regression coverage: The complete pinned diff adds a one-byte fallback when no valid prefix exists. Three tests cover malformed-input completion, byte preservation, valid Unicode boundaries, and the public writer.
Reachable production path: ExportVCardCmd loads People(), which uses ReadPerson and scalar salvage; damaged name bytes reach the vCard writer without UTF-8 normalization. The CLI, store, and Markdown implementation are unchanged by this PR.
Findings None None.
Security None None.

How this fits together

Clawdex reads contacts from local Markdown files, salvaging fields when their metadata is damaged. Its vCard exporter formats those contacts into folded text lines and writes them to a file or stdout.

flowchart LR
  A[Contact Markdown files] --> B[Read and salvage fields]
  B --> C[Export selected contacts]
  C --> D[Format vCard fields]
  D --> E[Fold lines with forward progress]
  E --> F[File or stdout]
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test growth production +4/-1, tests +71/-0 The production growth is limited to ensuring progress, supported by three focused regression tests.

Technical review

Best possible solution:

Keep folding byte-preserving and guarantee forward progress on malformed input while retaining valid Unicode boundaries.

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

Yes: current-main source establishes the zero-progress loop, and the contributor supplies a concrete malformed-contact CLI reproduction. This read-only review did not execute it.

Is this the best way to solve the issue?

Yes: the fallback narrowly repairs termination without introducing configuration, discarding contact bytes, or changing valid-input folding.

AGENTS.md: not found in the target repository.

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

Labels

Label justifications:

  • P2: The fix addresses an export hang triggered by malformed local contact metadata, with a limited 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 Windows CLI transcript exercises damaged Markdown salvage through actual vCard file export: the baseline times out, while the patched exporter completes in 29 ms. The current head changes only the changelog since that implementation; phone import compatibility is outside the demonstrated termination fix.
  • proof: sufficient: Contributor real behavior proof is sufficient. The supplied Windows CLI transcript exercises damaged Markdown salvage through actual vCard file export: the baseline times out, while the patched exporter completes in 29 ms. The current head changes only the changelog since that implementation; phone import compatibility is outside the demonstrated termination fix.

Evidence

What I checked:

  • Current main still contains the defect: The fold loop can reach cut == 0 on an invalid leading byte, emit a continuation, and repeat without consuming input. (internal/vcard/vcard.go:184, baa945d86da0)
  • Introduced change and regression coverage: The complete pinned diff adds a one-byte fallback when no valid prefix exists. Three tests cover malformed-input completion, byte preservation, valid Unicode boundaries, and the public writer. (internal/vcard/vcard.go:188, 9e586b4d18a2)
  • Reachable production path: ExportVCardCmd loads People(), which uses ReadPerson and scalar salvage; damaged name bytes reach the vCard writer without UTF-8 normalization. The CLI, store, and Markdown implementation are unchanged by this PR. (internal/cli/cli.go:657, 9e586b4d18a2)
  • Real CLI before-and-after evidence: The supplied body snapshot, sourceRevision 6dceaa4a8a45f9231fa884b8e97dd5e7fb8482e3039844fc2831dcda2a615efe, records Windows amd64 with Go 1.26.6, an isolated damaged contact with auto_repair=false, and the public export command. The unpatched binary timed out after 1 second; the patched binary reported exported: 1 in 29 ms and wrote a 288-byte file. This exercises the changed fold owner through actual contact loading and file export.
  • Re-review continuity: Only CHANGELOG.md changed since the previously reviewed head. The prior structured review retained no findings; the production fix and tests are unchanged. (CHANGELOG.md:5, 9e586b4d18a2)
  • Area history and inspection limits: Available main history identifies Peter Steinberger on the exporter bootstrap, avatar support, and later file-safety work. Deeper blame and historical release-source reads failed because required objects were unavailable and the remote could not resolve; no exact introducing-line or shipped-fix claim is made. (internal/vcard/vcard.go, 826b0a608822)

Likely related people:

  • Peter Steinberger: 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.

History

Review history (2 earlier review cycles)
  • reviewed 2026-09-05T04:36:05.777Z sha bb1c48e :: needs maintainer review before merge. :: none
  • reviewed 2026-09-05T19:58:06.206Z sha bb1c48e :: needs maintainer review before merge. :: none

@steipete

steipete commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Maintainer proof for head 9e586b4:

  • Built and ran the real CLI on macOS arm64 with Go 1.26.6, using an isolated synthetic contacts repo and automatic repair disabled. A damaged Markdown name containing 0xff followed by 80 ASCII bytes made current main exceed a one-second export deadline. This head exported one complete vCard in 0.070 seconds and preserved the name bytes after unfolding.
  • Full all-package tests and race tests passed; total statement coverage was 90.3%. The valid-Unicode regression also passes.
  • Codex branch autoreview against origin/main completed with no actionable P0–P2 findings.
  • Exact-head CI passed: https://github.com/openclaw/clawdex/actions/runs/33989145086

The Unreleased entry credits @SebTardif. Prepared for a maintainer squash merge; no merge performed.

@steipete
steipete merged commit 9041310 into openclaw:main Sep 5, 2026
8 checks passed
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.

2 participants