Skip to content

fix(acp): replace invalid UTF-8 bytes that broke Biome checks - #1876

Open
ShryukGrandhi wants to merge 1 commit into
Nano-Collective:mainfrom
ShryukGrandhi:fix/issue-1862-bug-invalid-utf-8-in-acp-agent-breaks-bi
Open

ShryukGrandhi wants to merge 1 commit into
Nano-Collective:mainfrom
ShryukGrandhi:fix/issue-1862-bug-invalid-utf-8-in-acp-agent-breaks-bi

Conversation

@ShryukGrandhi

Copy link
Copy Markdown

Description

source/acp/acp-agent.ts had seven stray 0x9d bytes where em dashes used to be: in the VS Code /help reply (the built-in command list and the custom command list) and in two comments. They are not valid UTF-8, so Biome stopped with stream did not contain valid UTF-8, and pnpm test:lint / pnpm test:format failed for the whole repo. The same lines also reached clients as replacement characters.

The original version of the file (67d8a81 / 663b688) had real — characters. A later edit reduced each one to a single byte. This PR puts the em dashes back.

The new regression test turned up one more file with the same problem: source/app/prompts/sections/task-approach-nano-architect.md had a Windows-1252 0x97 in its heading. That section goes into the system prompt in nano architect mode. I changed it to — as well, so it matches task-approach-architect.md.

Once Biome could read acp-agent.ts, it reported one formatting difference (the acpContentToUserMessage(...) call) that the bad bytes had been hiding. I applied biome format to fix it, so test:format passes.

I added source/source-encoding.spec.ts. It decodes every .ts/.tsx/.js/.json/.md file under source/ with a strict UTF-8 decoder, so this kind of problem shows up in the AVA run, not only as a Biome I/O error.

Fixes #1862

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Changeset

  • Added a changeset (pnpm changeset) describing this change for the changelog

Testing

Automated Tests

  • New features include passing tests in .spec.ts/tsx files
  • All existing tests pass (pnpm test:all completes successfully)
  • Tests cover both success and error scenarios

Before the fix, pnpm exec biome check source/acp/acp-agent.ts failed with the UTF-8 I/O error, and the new spec failed and listed both files. After the fix:

  • pnpm run test:format: passes
  • pnpm run test:lint: passes
  • pnpm run test:types: passes
  • pnpm run test:knip: passes
  • pnpm run test:ava source/source-encoding.spec.ts source/acp/: 238 passed
  • pnpm run test:ava source/source-encoding.spec.ts source/acp/acp-agent.spec.ts source/utils/prompt-builder.spec.ts: 138 passed

The full AVA suite did not finish within 20 minutes on my machine, so I only ran the specs listed above.

Manual Testing

  • Tested with Ollama
  • Tested with OpenRouter
  • Tested with OpenAI-compatible API
  • Tested MCP integration (if applicable)

Checklist

  • If this was for an open issue, I was assigned to it
  • Code follows project style guidelines
  • Self-review completed
  • Documentation updated (if needed)
  • No breaking changes (or clearly documented)
  • Appropriate logging added using structured logging (see CONTRIBUTING.md)

Signed-off-by: ShryukGrandhi <85529990+ShryukGrandhi@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

nc-review: comments — 1 important, 1 nit

@ShryukGrandhi — a few things worth a look, none blocking.

The PR replaces corrupted bytes (stray 0x9d / Windows-1252 0x97) with real em dashes in source/acp/acp-agent.ts and source/app/prompts/sections/task-approach-nano-architect.md, applies the Biome formatting fix the bad bytes were hiding, and adds a repo-scoped strict-UTF-8 regression spec. This directly and correctly resolves issue #1862: I confirmed the base copies of both files still contain the corrupt bytes (and task-approach-nano-architect.md is the only sibling section affected), and the new spec would fail before the fix and pass after. The code and tests are sound; the one substantive concern is that the ACP-agent.ts portion of this fix (and a pending changeset for it, .changeset/acp-help-invalid-utf8.md) already appears to be present on the base branch, so this PR may be re-applying an already-landed fix and adds a redundant second changeset for it. I could not find this exact PR among the open-PR corpus, so I am not claiming a live duplicate — please confirm whether the ACP hunks are already on main and consolidate the changesets.

🟠 important · duplicate · source/acp/acp-agent.ts

The base branch already ships .changeset/acp-help-invalid-utf8.md describing this exact fix — "source/acp/acp-agent.ts contained invalid UTF-8 bytes where an em dash was meant; they are now real em dashes, which also lets Biome read and format the file again." That changeset is still present (unconsumed), and when I read source/acp/acp-agent.ts on the base checkout the /help block and both comments already show valid em dashes, not the 0x9d the diff removes. This suggests the ACP-agent.ts half of this fix already landed on main and this PR re-applies it.

If that is the case, please drop the redundant acp-agent.ts hunks and delete either this PR's .changeset/fix-invalid-utf8-source.md or the pre-existing acp-help-invalid-utf8.md — two pending changesets describing the same ACP fix will produce a duplicate/confusing changelog entry at release. I could not locate this PR in the open-PR list, so I am not asserting a live duplicate PR; this is asking you to reconcile against what is already on main. The nano-architect prompt fix and the new source-encoding.spec.ts are genuinely new and should stay either way.

⚪ nit · tests · source/source-encoding.spec.ts

Nice regression test — the strict TextDecoder('utf-8', {fatal: true}) over every .ts/.tsx/.js/.json/.md file under source/ genuinely fails before the fix and passes after, which is exactly what the project rubric asks for. One observation, not a blocker: the scan is confined to source/, but the corrupt-byte class can just as easily appear in plugins/vscode/ (a contributed extension with its own sources) or in docs/, and this test would not catch it there. Consider widening the root to the repo (or to source/ + plugins/) if that is cheap — otherwise leaving it scoped is a defensible choice, not a problem.


🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional

Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.

@github-actions github-actions Bot added the agent:comments nc-review left non-blocking findings label Oct 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent:comments nc-review left non-blocking findings area:tui Terminal UI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Invalid UTF-8 in ACP agent breaks Biome checks

1 participant