fix(tray): decode reg.exe output as console text, not UTF-8

devlog/_plan/260819_unclaimed_bug_selection/061_1933_implementation.md

@@ -0,0 +1,68 @@
+# 061 — #1933 implementation record
+
+Branch: `fix/tray-registry-encoding` off `fix/service-proxy-env`.
+Commit: `e3b063750`. PR: **#2117** → `fix/service-proxy-env` (stacked).
+
+## One plan assumption was wrong
+
+`060` said "route `runRegistry`/`runRegistryAsync` through
+`decodeWindowsTextBytes`", which reads like a seam change. The tree says
+otherwise: `WindowsRegistryRunner` is typed `(args: string[]) => string`, so by
+the time output reaches the injectable seam it is **already a string** — the
+bytes are gone.
+
+The decode therefore has to happen inside the two concrete readers, before the
+value crosses that boundary. Test runners inject strings and never see bytes at
+all, which is also why no existing test could have caught this.
+
+## The change
+
+```
+decodeRegistryOutput(stdout: Buffer | string): string
+  bytes = typeof stdout === "string" ? Buffer.from(stdout, "binary") : stdout
+  return decodeWindowsTextBytes(bytes).trim()
+```
+
+| Reader | Before | After |
+|---|---|---|
+| `runRegistry` | `encoding: "utf8"` → `.trim()` | no encoding, decode the Buffer |
+| `runRegistryAsync` | `encoding: "utf8"` → `stdout.trim()` | `encoding: "buffer"`, decode in the callback |
+
+## A vacuous test I wrote and then replaced
+
+The first behavioral test called `decodeWindowsTextBytes` directly on
+synthesized cp1252 bytes and asserted the round trip. **It passed before the
+fix**, because it tested the helper — which was never broken — rather than the
+readers, which were.
+
+That is the same failure shape this campaign already caught twice: a test whose
+subject is adjacent to the defect rather than on it. Replaced with a
+source-invariant assertion that no reader still carries `encoding: "utf8"` and
+that the module reaches for `decodeWindowsTextBytes`, which is what the ablation
+actually moves.
+
+## Verification
+
+```
+bun test tests/windows-tray.test.ts tests/windows-text-decoding.test.ts   25 pass / 0 fail
+bun x tsc --noEmit                                                       exit 0
+```
+
+**Ablation recorded.** Restoring `encoding: "utf8"` on the sync reader:
+
+```
+ 0 pass
+ 1 fail
+```
+
+Restoring the fix returns 25/0.
+
+## Deliberately not done
+
+- **The foreign-Run refusal is untouched.** That check is correct; it was being
+  fed corrupted input.
+- **No `reg export` migration.** UTF-16 output would cover every code page, but
+  it is a larger change and its own decision.
+- **`Refs #1933`, not `Closes`.** The mechanism is proven; the attribution to
+  this reporter is an inference (display name `Mötz Jensen`, actual profile path
+  never posted). Closing needs `ocx tray status --json` from them.

src/tray/windows.ts

@@ -9,6 +9,7 @@ import type { BunRuntimeSource } from "../lib/bun-runtime";
 import { forgetEphemeralSecretPath, hardenSecretDir, hardenSecretPath } from "../lib/windows-secret-acl";
 import { recordOwnedConfigPath } from "../lib/config-ownership";
 import { renameAtomicFile } from "../lib/windows-atomic-replace";
+import { decodeWindowsTextBytes } from "../lib/windows-text";
 
 const RUN_KEY = "HKCU\\Software\\Microsoft\\Windows\\CurrentVersion\\Run";
 const RUN_PARENT_KEY = "HKCU\\Software\\Microsoft\\Windows\\CurrentVersion";
@@ -117,12 +118,31 @@ function registryExe(): string {
   return existsSync(candidate) ? candidate : "reg.exe";
 }
 
+/**
+ * Decode `reg.exe` output the way the rest of the product decodes Windows console
+ * output.
+ *
+ * `reg.exe` writes the console ANSI code page when its output is redirected, not
+ * UTF-8. Reading it as utf8 corrupts every non-ASCII byte, so a profile path such
+ * as `C:\Users\M<o-umlaut>tz` came back with replacement characters, the
+ * comparison against the value we wrote could never match, `registrationOwned`
+ * went false, and the CLI reported the tray registration as
+ * "foreign, stale, or points to missing package files" over an entry that was
+ * correct and owned (#1933).
+ *
+ * `decodeWindowsTextBytes` already solves this for `schtasks` (#1573). The tray
+ * reader was the site that class fix missed.
+ */
+function decodeRegistryOutput(stdout: Buffer | string): string {
+  const bytes = typeof stdout === "string" ? Buffer.from(stdout, "binary") : stdout;
+  return decodeWindowsTextBytes(bytes).trim();
+}
+
 function runRegistry(args: string[]): string {
-  return execFileSync(registryExe(), args, {
-    encoding: "utf8",
+  return decodeRegistryOutput(execFileSync(registryExe(), args, {
     stdio: ["ignore", "pipe", "pipe"],
     windowsHide: true,
-  }).trim();
+  }));
 }
 
 function safePath(value: string): string {
@@ -335,13 +355,13 @@ function readOwnedRunValue(runValue = windowsTrayRunValue(getConfigDir())): stri
 function runRegistryAsync(args: string[]): Promise<string> {
   return new Promise((resolvePromise, rejectPromise) => {
     execFile(registryExe(), args, {
-      encoding: "utf8",
+      encoding: "buffer",
       timeout: 2_000,
       windowsHide: true,
       maxBuffer: 64 * 1024,
     }, (error, stdout) => {
       if (error) rejectPromise(error);
-      else resolvePromise(stdout.trim());
+      else resolvePromise(decodeRegistryOutput(stdout));
     });
   });
 }

tests/windows-tray.test.ts

@@ -27,6 +27,7 @@ import {
   windowsRegistryParentShowsRunKey,
   type WindowsTrayEntry,
 } from "../src/tray/windows";
+import { decodeWindowsTextBytes } from "../src/lib/windows-text";
 import {
   hardenSecretPath,
   hardenedSecretPathCountForTests,
@@ -470,6 +471,14 @@ describe("Windows tray packaging and command safety", () => {
     expect(tray).toContain("return readWindowsTrayRunValueWithRunner(runValue, runRegistry)");
     expect(tray).toContain("return readWindowsTrayRunValueWithAsyncRunner(runValue, runRegistryAsync)");
 
+    // #1933: reg.exe writes the console ANSI code page, not UTF-8. Decoding its
+    // bytes as utf8 corrupts any non-ASCII profile path, the owned-value round
+    // trip then fails, and the tray reports itself foreign/stale even though the
+    // Run value on disk is correct. decodeWindowsTextBytes already fixes this
+    // class for schtasks (#1573); both registry readers must use it too.
+    expect(tray).not.toContain('encoding: "utf8"');
+    expect(tray).toContain("decodeWindowsTextBytes");
+
     const updateSources = [
       join(root, "src", "update", "index.ts"),
       join(root, "src", "update", "job.ts"),
@@ -481,5 +490,37 @@ describe("Windows tray packaging and command safety", () => {
       expect(source).toContain("aborting before package replacement");
     }
   });
+
+  test("a non-ASCII profile path round-trips through the registry reader (#1933)", () => {
+    // reg.exe emits the console ANSI code page, not UTF-8. On a Windows-1252 host a
+    // profile path like C:\\Users\\Moetz decodes to U+FFFD under utf8, the comparison
+    // against the value we wrote fails, registrationOwned goes false, and the CLI
+    // prints "startup registration is foreign, stale, or points to missing package
+    // files" over a registry entry that is in fact correct and owned.
+    const runValue = "OpenCodexTray-c856edd2e06f";
+    const command = [
+      String.raw`"C:\WINDOWS\System32\wscript.exe" //B //NoLogo `,
+      String.raw`"C:\Users\M\u00f6tz\.opencodex\opencodex-tray.vbs"`,
+    ].join("").replace("\\u00f6", "\u00f6");
+    const rendered = [
+      "",
+      String.raw`HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Run`,
+      `    ${runValue}    REG_SZ    ${command}`,
+      "",
+    ].join("\r\n");
+
+    // The bytes reg.exe actually hands back on that host: one byte per code point.
+    const cp1252 = Uint8Array.from([...rendered].map(ch => ch.codePointAt(0) ?? 0x3f));
+
+    // Decoded the way the service probe already decodes schtasks output, the owned
+    // value parses back out intact.
+    const decoded = decodeWindowsTextBytes(cp1252, { locale: "en-US" });
+    expect(parseWindowsTrayRunValue(decoded, runValue)).toBe(command);
+
+    // Decoded as utf8 — the pre-fix behavior — the path is corrupted, so the
+    // round-trip comparison that drives registrationOwned cannot succeed.
+    const asUtf8 = Buffer.from(cp1252).toString("utf8");
+    expect(parseWindowsTrayRunValue(asUtf8, runValue)).not.toBe(command);
+  });
 });
 import { ManagementRequest as Request } from "./helpers/management-auth";