Skip to content

fix(cli): decode ast-grep stderr with a StringDecoder - #126

Merged
thecodedrift merged 1 commit into
mainfrom
fix/scan-stderr-decoder
Aug 20, 2026
Merged

fix(cli): decode ast-grep stderr with a StringDecoder#126
thecodedrift merged 1 commit into
mainfrom
fix/scan-stderr-decoder

Conversation

@thecodedrift

Copy link
Copy Markdown
Member

ast-grep's stderr was decoded one chunk at a time with chunk.toString(), so a multi-byte UTF-8 sequence split across a chunk boundary had each half independently replaced with U+FFFD — unrecoverable by the time the pieces were joined. Each stream now uses a single StringDecoder("utf8"), written per data event and flushed with .end() on close. stdout needs nothing: it is read through node:readline, which decodes across boundaries itself.

No scan result was ever wrong — the corrupted text only reaches an error message. But it is the message a user reads when ast-grep rejects a rule file, naming a rule id or a path, which is exactly where a non-ASCII character turns up.

A fourth copy, beyond the one reported. The issue names scan.ts as the third instance after vale/run.ts and verify.ts. runtime/narrow.ts carried a fourth, identical in pattern and consequence, and is fixed here too — the issue's own reasoning for closing it ("leaving one copy of a defect after fixing its siblings is how it comes back") applies to it unchanged. Happy to split it out if you would rather review it separately.

That one had a wrinkle worth a look during review. Its stderr suffix was gated on stderrChunks.length > 0, and the decoder's final flush pushes an empty string on a stream that ended cleanly — so adding the flush alone would have made the condition unconditionally true and appended a bare : to every ast-grep narrow failed (…) message. The condition now tests the joined text instead:

const stderr = stderrChunks.join("").trim();
`ast-grep narrow failed (${cause})${stderr === "" ? "" : `: ${stderr}`}`;

scan.ts was already safe there — it tested the joined string rather than the chunk count.

No regression test. Reproducing this requires ast-grep to emit a multi-byte stderr message split at a chosen byte offset, and both spawn sites resolve their binary internally, so there is no seam to inject a fake process without a refactor larger than the fix. The two earlier fixes shipped the same way. If you want this class of bug made unrepresentable rather than fixed per site, that is #99, which was measured and rejected on bundle size.

Fixes #124
Refs #99

`runAstGrepScan` decoded stderr one chunk at a time with
`chunk.toString()`. A multi-byte UTF-8 sequence split across a chunk
boundary is decoded as two invalid sequences, both halves become U+FFFD,
and the original bytes are unrecoverable by the time the pieces are
joined. Each stream now uses a single StringDecoder, flushed with
`.end()` on close — the remedy `vale/run.ts` and `verify.ts` already
carry. stdout needs nothing: readline decodes across boundaries itself.

`runtime/narrow.ts` carried a fourth copy of the same pattern, with the
same consequence, and is fixed alongside. Its stderr suffix was gated on
`stderrChunks.length > 0`, which the decoder's final flush would have
made unconditionally true — an empty string is still a chunk — so the
condition now tests the joined text and no message gains a bare `: `.

The corrupted text only ever reached an error message, so no scan result
was ever wrong. It is the message a user reads when ast-grep rejects a
rule file, naming a rule id or a path, which is where a non-ASCII
character turns up.

No regression test: reproducing this needs stderr split at a chosen byte
offset, and both spawn sites resolve their binary internally, leaving no
seam to inject a fake process. The two earlier fixes shipped the same way.

Fixes #124
Refs #99

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@thecodedrift
thecodedrift marked this pull request as ready for review August 20, 2026 23:09
Copilot AI lite review requested due to automatic review settings August 20, 2026 23:09
@thecodedrift

Copy link
Copy Markdown
Member Author

@claude /review

@claude

claude Bot commented Aug 20, 2026

Copy link
Copy Markdown

Claude finished @thecodedrift's task in 1m 10s —— View job


Review: fix/scan-stderr-decoder

  • Read the diff (git diff origin/main...HEAD)
  • Reviewed packages/cli/src/rules/scan.ts
  • Reviewed packages/cli/src/rules/runtime/narrow.ts
  • Reviewed .changeset/decode-sg-stderr.md
  • Compared against the established pattern in vale/run.ts / verify.ts
  • Checked the stderrChunks.length > 0 → joined-string condition fix in narrow.ts
  • Confirmed no test seam exists for a regression test (matches the PR description's claim)

Overall assessment: looks good, no correctness issues found.

This is a clean, well-scoped fix that mirrors the StringDecoder pattern already established in vale/run.ts and verify.ts.

Details checked:

  • scan.ts: stderrDecoder.write() is called per data chunk and stderrDecoder.end() is called once in the close handler before the existing stderr ? ... : "" check. That check already tested the joined/trimmed string (not chunk count), so it was already safe against the flush's empty-string push — as the PR description notes.
  • narrow.ts: the stderrChunks.length > 0 gate is correctly replaced with a check on the joined+trimmed text (stderr === "" ? "" : ...). This was the fix that was actually needed here, since the old length-based gate would otherwise become permanently true once .end() unconditionally pushes a (possibly empty) string on every clean close. Verified this produces the same "no suffix" behavior on a clean exit with empty stderr as before.
  • stdout in both files continues to go through node:readline, which decodes multi-byte sequences across chunk boundaries itself, so no decoder is needed there — correctly left alone.
  • The .changeset/decode-sg-stderr.md entry is present, correctly scoped as a patch, and matches the existing changeset file conventions in this repo.
  • No behavior change for ASCII-only stderr output — StringDecoder is a strict superset of Buffer#toString() behavior for valid UTF-8 that isn't split across chunks.
  • Test coverage: confirmed there's no existing seam to inject a fake sg/ast-grep process (both findSgBinary() call sites resolve the binary internally), consistent with the two prior fixes (vale/run.ts, verify.ts) also shipping without a regression test. Not a blocker given the precedent, though #99 (mentioned in the PR body) would make this whole class of bug structurally unrepresentable if revisited later.

No inline comments — nothing in the diff rises to a concrete, actionable issue.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request fixes UTF-8 decoding of ast-grep’s stderr in the CLI so multi-byte characters split across chunk boundaries are preserved, improving the reliability of user-facing error messages (without affecting scan correctness).

Changes:

  • Use a single StringDecoder("utf8") for ast-grep stderr in runAstGrepScan, writing per data event and flushing with .end() on close.
  • Apply the same stderr decoding approach to the runtime “narrow” subprocess, and adjust the error-message suffix logic to avoid appending : when stderr is empty.
  • Add a changeset documenting the patch release impact.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
packages/cli/src/rules/scan.ts Decode ast-grep stderr with a shared StringDecoder and flush on close to avoid UTF-8 boundary corruption.
packages/cli/src/rules/runtime/narrow.ts Decode ast-grep stderr with StringDecoder, flush on close, and avoid empty-suffix error formatting.
.changeset/decode-sg-stderr.md Patch changeset describing the stderr decoding fix and user-visible impact.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@thecodedrift
thecodedrift merged commit f13d501 into main Aug 20, 2026
9 checks passed
@thecodedrift
thecodedrift deleted the fix/scan-stderr-decoder branch August 20, 2026 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

runAstGrepScan decodes stderr per chunk, corrupting multi-byte characters

2 participants