From 4867a68586e07c04ae46caf0dfaf81262f6a3845 Mon Sep 17 00:00:00 2001 From: Gordon Beeming Date: Fri, 14 Aug 2026 01:41:21 +1000 Subject: [PATCH 1/5] fix(shell): stop the profile block baking in a snapshot of PATH (#128) __copilot_update_profile wrote the marker block with an unquoted heredoc, so $HOME and $PATH expanded while the file was being written. Profiles ended up holding the value of PATH from install time, and every shell start replaced the live PATH with that copy instead of prepending ~/.local/bin to it. On WSL that drops the Windows interop entries, so `code .` stops resolving. Quoting the delimiter and writing the markers and script path literally makes the shell-written block byte-identical to the one ShellIntegration writes. EnsureBlock returned as soon as it saw the start marker, so --install-shells could never repair a block an older release had corrupted. It now compares the marked region against the block it wants and rewrites on drift, leaving the surrounding config alone and leaving a marker with no matching end untouched. Also fixes cleanup() in the CLI integration tests, which built paths from $BASH_SOURCE after the tests had changed directory and so never removed publish/cli-test. --- app/Infrastructure/ShellIntegration.cs | 27 +++++++- copilot_here.sh | 20 +++--- .../ShellIntegrationTests.cs | 64 +++++++++++++++++++ tests/integration/test_cli.sh | 55 ++++++++++++++-- 4 files changed, 152 insertions(+), 14 deletions(-) diff --git a/app/Infrastructure/ShellIntegration.cs b/app/Infrastructure/ShellIntegration.cs index f02dcb6..7743e8b 100644 --- a/app/Infrastructure/ShellIntegration.cs +++ b/app/Infrastructure/ShellIntegration.cs @@ -597,7 +597,8 @@ private static void EnsureDownloadedIfMissing(string url, string destinationPath File.WriteAllText(destinationPath, content); } - private static void EnsureBlock(string filePath, string markerStart, string markerEnd, string block) + /// Internal for testing via InternalsVisibleTo. + internal static void EnsureBlock(string filePath, string markerStart, string markerEnd, string block) { var dir = Path.GetDirectoryName(filePath); if (!string.IsNullOrWhiteSpace(dir)) @@ -607,8 +608,30 @@ private static void EnsureBlock(string filePath, string markerStart, string mark var existing = File.Exists(filePath) ? File.ReadAllText(filePath) : string.Empty; - if (existing.Contains(markerStart, StringComparison.Ordinal)) + var startIndex = existing.IndexOf(markerStart, StringComparison.Ordinal); + if (startIndex >= 0) { + // A block written by an older release can be wrong (one baked the literal value + // of PATH into the profile instead of the variable), so reconcile what is on disk + // against what we want rather than trusting the marker's presence. Only the marked + // region is rewritten; the user's own config above and below it is untouched. + var endMarkerIndex = existing.IndexOf(markerEnd, startIndex, StringComparison.Ordinal); + if (endMarkerIndex < 0) + { + // Start marker with no matching end: the block's extent is unknowable, so + // rewriting could swallow the user's config. Leave it for the uninstaller. + return; + } + + var endIndex = endMarkerIndex + markerEnd.Length; + var current = existing[startIndex..endIndex]; + var desired = block.TrimEnd('\r', '\n'); + + if (!string.Equals(current, desired, StringComparison.Ordinal)) + { + File.WriteAllText(filePath, string.Concat(existing.AsSpan(0, startIndex), desired, existing.AsSpan(endIndex))); + } + return; } diff --git a/copilot_here.sh b/copilot_here.sh index eeecf61..8f2c049 100755 --- a/copilot_here.sh +++ b/copilot_here.sh @@ -205,8 +205,7 @@ __copilot_update() { __copilot_update_profile() { local profile_path="$1" local profile_name="$2" - local script_path="$HOME/.copilot_here.sh" - + # Create profile if it doesn't exist if [ ! -f "$profile_path" ]; then touch "$profile_path" @@ -231,10 +230,15 @@ __copilot_update_profile() { grep -v "copilot_here.sh" "$profile_path" > "$temp_file" 2>/dev/null || cat "$profile_path" > "$temp_file" 2>/dev/null || true fi - # Add fresh marker block - cat >> "$temp_file" << EOF + # Add fresh marker block. + # The quoted 'EOF' is load-bearing: $HOME and $PATH must land in the profile as + # variables, not as their values at install time. An unquoted delimiter bakes in + # a snapshot of PATH, and the block then replaces the user's live PATH on every + # shell start instead of prepending to it. That is why the markers and the script + # path are written out literally here rather than interpolated. + cat >> "$temp_file" << 'EOF' -$marker_start +# >>> copilot_here >>> # Ensure user bin directory is on PATH if [ -d "$HOME/.local/bin" ]; then case ":$PATH:" in @@ -242,10 +246,10 @@ if [ -d "$HOME/.local/bin" ]; then *) export PATH="$HOME/.local/bin:$PATH" ;; esac fi -if [ -f "$script_path" ]; then - source "$script_path" +if [ -f "$HOME/.copilot_here.sh" ]; then + source "$HOME/.copilot_here.sh" fi -$marker_end +# <<< copilot_here <<< EOF # Preserve symlinks: if target is a symlink, mv into the resolved path atomically; diff --git a/tests/CopilotHere.UnitTests/ShellIntegrationTests.cs b/tests/CopilotHere.UnitTests/ShellIntegrationTests.cs index ef0e726..df5eb55 100644 --- a/tests/CopilotHere.UnitTests/ShellIntegrationTests.cs +++ b/tests/CopilotHere.UnitTests/ShellIntegrationTests.cs @@ -117,6 +117,70 @@ public async Task RemoveBlock_MissingFile_ReturnsFalse() await Assert.That(changed).IsFalse(); } + [Test] + public async Task EnsureBlock_StaleBlock_IsRewritten_PreservingSurroundingContent() + { + // An older release wrote profiles with the value of PATH baked in rather than the + // variable, so a present marker is not proof the block on disk is the right one. + var profile = Path.Combine(_tempDir, ".bashrc"); + File.WriteAllText(profile, + "export EDITOR=vim\n" + + $"{MarkerStart}\n" + + "export PATH=\"/home/someone/.local/bin:/usr/bin:/bin\"\n" + + $"{MarkerEnd}\n" + + "alias g=git\n"); + + var block = $"{MarkerStart}\nexport PATH=\"$HOME/.local/bin:$PATH\"\n{MarkerEnd}\n"; + ShellIntegration.EnsureBlock(profile, MarkerStart, MarkerEnd, block); + var result = File.ReadAllText(profile); + + await Assert.That(result).Contains("export PATH=\"$HOME/.local/bin:$PATH\""); + await Assert.That(result).DoesNotContain("/home/someone/.local/bin"); + await Assert.That(result).Contains("export EDITOR=vim"); + await Assert.That(result).Contains("alias g=git"); + } + + [Test] + public async Task EnsureBlock_CurrentBlock_LeavesFileUnchanged() + { + var profile = Path.Combine(_tempDir, ".bashrc"); + var block = $"{MarkerStart}\nexport PATH=\"$HOME/.local/bin:$PATH\"\n{MarkerEnd}\n"; + var original = $"export EDITOR=vim\n{block}alias g=git\n"; + File.WriteAllText(profile, original); + + ShellIntegration.EnsureBlock(profile, MarkerStart, MarkerEnd, block); + + await Assert.That(File.ReadAllText(profile)).IsEqualTo(original); + } + + [Test] + public async Task EnsureBlock_MissingEndMarker_LeavesFileUnchanged() + { + // Without an end marker the block's extent is unknowable, so rewriting could + // swallow whatever the user has below the start marker. + var profile = Path.Combine(_tempDir, ".bashrc"); + var original = $"export EDITOR=vim\n{MarkerStart}\nexport PATH=\"/frozen:/usr/bin\"\nalias g=git\n"; + File.WriteAllText(profile, original); + + ShellIntegration.EnsureBlock(profile, MarkerStart, MarkerEnd, $"{MarkerStart}\nfresh\n{MarkerEnd}\n"); + + await Assert.That(File.ReadAllText(profile)).IsEqualTo(original); + } + + [Test] + public async Task EnsureBlock_NoMarker_AppendsBlock() + { + var profile = Path.Combine(_tempDir, ".bashrc"); + File.WriteAllText(profile, "export EDITOR=vim\n"); + + var block = $"{MarkerStart}\nexport PATH=\"$HOME/.local/bin:$PATH\"\n{MarkerEnd}\n"; + ShellIntegration.EnsureBlock(profile, MarkerStart, MarkerEnd, block); + var result = File.ReadAllText(profile); + + await Assert.That(result).Contains("export EDITOR=vim"); + await Assert.That(result).Contains(block); + } + [Test] public async Task BuildCmdWrapper_UsesArgsSplatForForwarding() { diff --git a/tests/integration/test_cli.sh b/tests/integration/test_cli.sh index d8a4a58..dda89fa 100755 --- a/tests/integration/test_cli.sh +++ b/tests/integration/test_cli.sh @@ -8,6 +8,12 @@ set -e +# Resolved once at load time: tests change the working directory as they run, so +# deriving these from $BASH_SOURCE later (including in the EXIT trap) resolves +# against the wrong directory. +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO_ROOT="$(cd "$SCRIPT_DIR/../.." && pwd)" + # Parse arguments CLI_PATH="" while [[ $# -gt 0 ]]; do @@ -122,9 +128,7 @@ setup_cli() { } cleanup() { - local script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" - local repo_root="$(cd "$script_dir/../.." && pwd)" - rm -rf "$repo_root/publish/cli-test" 2>/dev/null || true + rm -rf "$REPO_ROOT/publish/cli-test" 2>/dev/null || true rm -rf "$TEST_DIR" 2>/dev/null || true } @@ -311,6 +315,48 @@ test_passthrough_help() { fi } +test_profile_block_keeps_path_variable() { + test_start "Profile block references \$PATH instead of a snapshot of it" + + local fake_home profile result + + fake_home="$TEST_DIR/profile-home" + mkdir -p "$fake_home/.local/bin" + profile="$fake_home/.bashrc" + printf 'export USER_CONFIG_KEPT=1\n' > "$profile" + + # Sentinel entries stand in for whatever PATH happens to hold at install time. + # If any of them reach the profile, the block has baked in a snapshot and will + # wipe the user's live PATH on every shell start. + env HOME="$fake_home" PATH="/sentinel/alpha:/sentinel/beta:/usr/bin:/bin" \ + bash -c "source '$REPO_ROOT/copilot_here.sh' >/dev/null 2>&1; __copilot_update_profile \"\$HOME/.bashrc\" 'test profile'" >/dev/null 2>&1 + + if grep -qF '/sentinel/alpha' "$profile" || grep -qF '/sentinel/beta' "$profile"; then + test_fail "Profile block baked in the install-time PATH: $(grep -F '/sentinel/' "$profile" | head -n 1)" + return + fi + + if ! grep -qF ':$PATH:' "$profile" || ! grep -qF '$HOME/.local/bin' "$profile"; then + test_fail "Profile block is missing the literal \$PATH / \$HOME references" + return + fi + + if ! grep -qF 'USER_CONFIG_KEPT' "$profile"; then + test_fail "Profile update discarded the user's own config" + return + fi + + # Sourcing the block from an unrelated PATH must prepend, not replace. + result=$(env -i HOME="$fake_home" PATH="/keep/me:/usr/bin:/bin" \ + bash -c 'source "$HOME/.bashrc" >/dev/null 2>&1; echo "$PATH"') + + if [[ "$result" == "$fake_home/.local/bin:/keep/me:/usr/bin:/bin" ]]; then + test_pass "Block prepends ~/.local/bin and preserves the existing PATH" + else + test_fail "Unexpected PATH after sourcing profile: $result" + fi +} + # ============================================================================ # MAIN # ============================================================================ @@ -339,7 +385,8 @@ main() { test_dotnet_alias test_yolo_flag test_passthrough_help - + test_profile_block_keeps_path_variable + print_summary exit $? } From d3f9ef1f98b27ebcf5ffbcbf63490d45cdc162d3 Mon Sep 17 00:00:00 2001 From: Gordon Beeming Date: Fri, 14 Aug 2026 01:50:58 +1000 Subject: [PATCH 2/5] fix(shell): reload before writing profiles; preserve profile BOM on rewrite Address Codex review feedback on PR #131 (round 1): - __copilot_update downloaded the fixed copilot_here.sh but still called the in-memory (pre-update) __copilot_update_profile before sourcing the new file, so a user upgrading from a release with the PATH-snapshot bug would have it re-applied by the update that was supposed to fix it. Reload the freshly downloaded script first so the repaired function is the one that runs. - ShellIntegration.EnsureBlock rewrote a stale marked region with File.WriteAllText's default (BOM-less UTF-8) overload. install.ps1 writes the PowerShell profile as UTF-8 with a BOM so Windows PowerShell 5.1 recognizes the encoding; losing the BOM on rewrite would make a profile with non-ASCII content reinterpreted via the legacy code page and garbled. Detect the original BOM before reading and carry it through the write. --- app/Infrastructure/ShellIntegration.cs | 22 ++++++++- copilot_here.sh | 17 ++++--- .../ShellIntegrationTests.cs | 49 +++++++++++++++++++ 3 files changed, 80 insertions(+), 8 deletions(-) diff --git a/app/Infrastructure/ShellIntegration.cs b/app/Infrastructure/ShellIntegration.cs index 7743e8b..4b3d65c 100644 --- a/app/Infrastructure/ShellIntegration.cs +++ b/app/Infrastructure/ShellIntegration.cs @@ -1,4 +1,5 @@ using System.Net.Http; +using System.Text; namespace CopilotHere.Infrastructure; @@ -606,7 +607,15 @@ internal static void EnsureBlock(string filePath, string markerStart, string mar Directory.CreateDirectory(dir); } - var existing = File.Exists(filePath) ? File.ReadAllText(filePath) : string.Empty; + var fileExists = File.Exists(filePath); + // install.ps1 writes the PowerShell profile as UTF-8 with a BOM (Windows PowerShell 5.1 + // needs the BOM to recognize UTF-8; without it, a profile with non-ASCII content is + // reinterpreted using the legacy code page and comes out garbled). File.ReadAllText + // strips the BOM on decode without recording that it was there, and the WriteAllText + // overload below defaults to BOM-less UTF-8, so a rewrite would silently drop it unless + // the original encoding is detected up front and carried through the write. + var hasBom = fileExists && FileStartsWithUtf8Bom(filePath); + var existing = fileExists ? File.ReadAllText(filePath) : string.Empty; var startIndex = existing.IndexOf(markerStart, StringComparison.Ordinal); if (startIndex >= 0) @@ -629,7 +638,8 @@ internal static void EnsureBlock(string filePath, string markerStart, string mar if (!string.Equals(current, desired, StringComparison.Ordinal)) { - File.WriteAllText(filePath, string.Concat(existing.AsSpan(0, startIndex), desired, existing.AsSpan(endIndex))); + var rewritten = string.Concat(existing.AsSpan(0, startIndex), desired, existing.AsSpan(endIndex)); + File.WriteAllText(filePath, rewritten, new UTF8Encoding(encoderShouldEmitUTF8Identifier: hasBom)); } return; @@ -653,4 +663,12 @@ private static bool FileContains(string path, string value) return false; } } + + private static bool FileStartsWithUtf8Bom(string filePath) + { + using var stream = File.OpenRead(filePath); + Span preamble = stackalloc byte[3]; + var read = stream.ReadAtLeast(preamble, preamble.Length, throwOnEndOfStream: false); + return read == 3 && preamble[0] == 0xEF && preamble[1] == 0xBB && preamble[2] == 0xBF; + } } diff --git a/copilot_here.sh b/copilot_here.sh index 8f2c049..118b7c9 100755 --- a/copilot_here.sh +++ b/copilot_here.sh @@ -158,17 +158,22 @@ __copilot_update() { if curl -fsSL "${COPILOT_HERE_RELEASE_URL}/copilot_here.sh" -o "$tmp_script" 2>/dev/null; then if cat "$tmp_script" > "$script_path" 2>/dev/null; then rm -f "$tmp_script" - + + # Reload before touching profiles: on an upgrade from a release whose + # __copilot_update_profile was itself buggy, the in-memory copy of that + # function is still the old one until this source picks up the fix that + # was just downloaded. Updating profiles first would re-apply the bug. + echo "" + echo "🔄 Reloading shell functions..." + # shellcheck disable=SC1090 + source "$script_path" + # Update shell profiles with marker blocks echo "" echo "🔧 Updating shell profiles..." __copilot_update_profile "$HOME/.bashrc" "bash (.bashrc)" __copilot_update_profile "$HOME/.zshrc" "zsh (.zshrc)" - echo "✅ Profiles updated" - - echo "✅ Update complete! Reloading shell functions..." - # shellcheck disable=SC1090 - source "$script_path" + echo "✅ Update complete!" echo "" echo "[VERSION] Script: $COPILOT_HERE_VERSION" if [ -x "$COPILOT_HERE_BIN" ]; then diff --git a/tests/CopilotHere.UnitTests/ShellIntegrationTests.cs b/tests/CopilotHere.UnitTests/ShellIntegrationTests.cs index df5eb55..1ca60b1 100644 --- a/tests/CopilotHere.UnitTests/ShellIntegrationTests.cs +++ b/tests/CopilotHere.UnitTests/ShellIntegrationTests.cs @@ -181,6 +181,55 @@ public async Task EnsureBlock_NoMarker_AppendsBlock() await Assert.That(result).Contains(block); } + [Test] + public async Task EnsureBlock_StaleBlock_PreservesUtf8Bom() + { + // install.ps1 writes the PowerShell profile as UTF-8 with a BOM so Windows PowerShell + // 5.1 recognizes the encoding instead of falling back to the legacy code page. A rewrite + // of the marked region must not silently drop that BOM. + var profile = Path.Combine(_tempDir, "profile.ps1"); + var bomEncoding = new System.Text.UTF8Encoding(encoderShouldEmitUTF8Identifier: true); + File.WriteAllText(profile, + "# café notes\n" + + $"{MarkerStart}\n" + + "$env:PATH = \"C:\\old\\bin;$env:PATH\"\n" + + $"{MarkerEnd}\n", + bomEncoding); + + var block = $"{MarkerStart}\n$env:PATH = \"$HOME\\.local\\bin;$env:PATH\"\n{MarkerEnd}\n"; + ShellIntegration.EnsureBlock(profile, MarkerStart, MarkerEnd, block); + + var rewrittenBytes = await File.ReadAllBytesAsync(profile); + await Assert.That(rewrittenBytes[0]).IsEqualTo((byte)0xEF); + await Assert.That(rewrittenBytes[1]).IsEqualTo((byte)0xBB); + await Assert.That(rewrittenBytes[2]).IsEqualTo((byte)0xBF); + + var result = File.ReadAllText(profile); + await Assert.That(result).Contains("café notes"); + await Assert.That(result).Contains("$HOME\\.local\\bin"); + } + + [Test] + public async Task EnsureBlock_StaleBlock_NoOriginalBom_WritesWithoutBom() + { + // Unix profiles (.bashrc/.zshrc) are never BOM'd on disk; a rewrite must not introduce + // one, since a leading BOM would corrupt the shell's parsing of the file. + var profile = Path.Combine(_tempDir, ".bashrc"); + File.WriteAllText(profile, + "export EDITOR=vim\n" + + $"{MarkerStart}\n" + + "export PATH=\"/frozen:/usr/bin\"\n" + + $"{MarkerEnd}\n"); + + var block = $"{MarkerStart}\nexport PATH=\"$HOME/.local/bin:$PATH\"\n{MarkerEnd}\n"; + ShellIntegration.EnsureBlock(profile, MarkerStart, MarkerEnd, block); + + var rewrittenBytes = await File.ReadAllBytesAsync(profile); + var startsWithBom = rewrittenBytes.Length >= 3 + && rewrittenBytes[0] == 0xEF && rewrittenBytes[1] == 0xBB && rewrittenBytes[2] == 0xBF; + await Assert.That(startsWithBom).IsFalse(); + } + [Test] public async Task BuildCmdWrapper_UsesArgsSplatForForwarding() { From 18286826424ed0fd45c6abb6496fb0964cd01c45 Mon Sep 17 00:00:00 2001 From: Gordon Beeming Date: Fri, 14 Aug 2026 01:57:38 +1000 Subject: [PATCH 3/5] fix(shell): detect all common BOM encodings, not just UTF-8, on profile rewrite Address Codex round-2 feedback on PR #131: the previous fix only checked for the 3-byte UTF-8 BOM before rewriting a stale marked region, so a profile that predates copilot_here and is already UTF-16 (Notepad's "Unicode" save option and PowerShell ISE both default to it) or UTF-32 would still get silently collapsed to BOM-less UTF-8 on rewrite. File.ReadAllText already auto-detects and decodes all of these correctly - verified empirically against a UTF-16LE file - it just doesn't report which encoding it found. DetectBomEncoding replaces the UTF-8-only check and covers UTF-8, UTF-16LE/BE, and UTF-32LE/BE, checking the longer UTF-32 preambles first since UTF-32LE's BOM starts with the same two bytes as UTF-16LE's. Added a UTF-16 regression test alongside the existing UTF-8-BOM and no-BOM cases. --- app/Infrastructure/ShellIntegration.cs | 50 +++++++++++++++---- .../ShellIntegrationTests.cs | 26 ++++++++++ 2 files changed, 65 insertions(+), 11 deletions(-) diff --git a/app/Infrastructure/ShellIntegration.cs b/app/Infrastructure/ShellIntegration.cs index 4b3d65c..e8f5833 100644 --- a/app/Infrastructure/ShellIntegration.cs +++ b/app/Infrastructure/ShellIntegration.cs @@ -609,12 +609,15 @@ internal static void EnsureBlock(string filePath, string markerStart, string mar var fileExists = File.Exists(filePath); // install.ps1 writes the PowerShell profile as UTF-8 with a BOM (Windows PowerShell 5.1 - // needs the BOM to recognize UTF-8; without it, a profile with non-ASCII content is - // reinterpreted using the legacy code page and comes out garbled). File.ReadAllText - // strips the BOM on decode without recording that it was there, and the WriteAllText - // overload below defaults to BOM-less UTF-8, so a rewrite would silently drop it unless - // the original encoding is detected up front and carried through the write. - var hasBom = fileExists && FileStartsWithUtf8Bom(filePath); + // needs the BOM to recognize UTF-8; without it, a profile with non-ASCII command or string + // content is reinterpreted using the legacy code page and comes out garbled). A profile + // can also predate copilot_here entirely and already be UTF-16 (Notepad's "Unicode" save + // option, or PowerShell ISE, both default to it) or UTF-32. File.ReadAllText auto-detects + // and decodes all of those correctly but doesn't report which one it found, and the + // WriteAllText overload below defaults to BOM-less UTF-8, so a rewrite would silently + // collapse any of these to UTF-8 unless the original encoding is detected up front and + // carried through the write. + var detectedBomEncoding = fileExists ? DetectBomEncoding(filePath) : null; var existing = fileExists ? File.ReadAllText(filePath) : string.Empty; var startIndex = existing.IndexOf(markerStart, StringComparison.Ordinal); @@ -639,7 +642,8 @@ internal static void EnsureBlock(string filePath, string markerStart, string mar if (!string.Equals(current, desired, StringComparison.Ordinal)) { var rewritten = string.Concat(existing.AsSpan(0, startIndex), desired, existing.AsSpan(endIndex)); - File.WriteAllText(filePath, rewritten, new UTF8Encoding(encoderShouldEmitUTF8Identifier: hasBom)); + var writeEncoding = detectedBomEncoding ?? new UTF8Encoding(encoderShouldEmitUTF8Identifier: false); + File.WriteAllText(filePath, rewritten, writeEncoding); } return; @@ -664,11 +668,35 @@ private static bool FileContains(string path, string value) } } - private static bool FileStartsWithUtf8Bom(string filePath) + // Checked longest-preamble-first: UTF-32LE's 4-byte BOM starts with the same 2 bytes as + // UTF-16LE's, so testing UTF-16LE first would misclassify every UTF-32LE file. + private static readonly (byte[] Preamble, Func MakeEncoding)[] BomSignatures = + [ + (new byte[] { 0xFF, 0xFE, 0x00, 0x00 }, () => new UTF32Encoding(bigEndian: false, byteOrderMark: true)), + (new byte[] { 0x00, 0x00, 0xFE, 0xFF }, () => new UTF32Encoding(bigEndian: true, byteOrderMark: true)), + (new byte[] { 0xEF, 0xBB, 0xBF }, () => new UTF8Encoding(encoderShouldEmitUTF8Identifier: true)), + (new byte[] { 0xFF, 0xFE }, () => Encoding.Unicode), + (new byte[] { 0xFE, 0xFF }, () => Encoding.BigEndianUnicode), + ]; + + /// Returns the encoding a leading byte-order mark identifies, or null if the file has none + /// (the caller then falls back to writing BOM-less UTF-8, matching what a marker-less profile + /// already looks like). + private static Encoding? DetectBomEncoding(string filePath) { using var stream = File.OpenRead(filePath); - Span preamble = stackalloc byte[3]; - var read = stream.ReadAtLeast(preamble, preamble.Length, throwOnEndOfStream: false); - return read == 3 && preamble[0] == 0xEF && preamble[1] == 0xBB && preamble[2] == 0xBF; + Span buffer = stackalloc byte[4]; + var read = stream.ReadAtLeast(buffer, buffer.Length, throwOnEndOfStream: false); + var bytes = buffer[..read]; + + foreach (var (preamble, makeEncoding) in BomSignatures) + { + if (bytes.Length >= preamble.Length && bytes[..preamble.Length].SequenceEqual(preamble)) + { + return makeEncoding(); + } + } + + return null; } } diff --git a/tests/CopilotHere.UnitTests/ShellIntegrationTests.cs b/tests/CopilotHere.UnitTests/ShellIntegrationTests.cs index 1ca60b1..bd94318 100644 --- a/tests/CopilotHere.UnitTests/ShellIntegrationTests.cs +++ b/tests/CopilotHere.UnitTests/ShellIntegrationTests.cs @@ -209,6 +209,32 @@ public async Task EnsureBlock_StaleBlock_PreservesUtf8Bom() await Assert.That(result).Contains("$HOME\\.local\\bin"); } + [Test] + public async Task EnsureBlock_StaleBlock_PreservesUtf16Bom() + { + // A profile that predates copilot_here can already be UTF-16 (Notepad's "Unicode" save + // option and PowerShell ISE both default to it). File.ReadAllText decodes it correctly, + // but a naive rewrite that only checks for a UTF-8 BOM would collapse it to UTF-8 anyway. + var profile = Path.Combine(_tempDir, "profile.ps1"); + var content = + "# café notes\n" + + $"{MarkerStart}\n" + + "$env:PATH = \"C:\\old\\bin;$env:PATH\"\n" + + $"{MarkerEnd}\n"; + File.WriteAllText(profile, content, System.Text.Encoding.Unicode); + + var block = $"{MarkerStart}\n$env:PATH = \"$HOME\\.local\\bin;$env:PATH\"\n{MarkerEnd}\n"; + ShellIntegration.EnsureBlock(profile, MarkerStart, MarkerEnd, block); + + var rewrittenBytes = await File.ReadAllBytesAsync(profile); + await Assert.That(rewrittenBytes[0]).IsEqualTo((byte)0xFF); + await Assert.That(rewrittenBytes[1]).IsEqualTo((byte)0xFE); + + var result = File.ReadAllText(profile); + await Assert.That(result).Contains("café notes"); + await Assert.That(result).Contains("$HOME\\.local\\bin"); + } + [Test] public async Task EnsureBlock_StaleBlock_NoOriginalBom_WritesWithoutBom() { From 04cfe8af93ac8339abfa583f37377573b2c3f027 Mon Sep 17 00:00:00 2001 From: Gordon Beeming Date: Fri, 14 Aug 2026 02:07:49 +1000 Subject: [PATCH 4/5] fix(shell): leave undecodable profiles alone instead of corrupting them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A BOM-less profile saved in a legacy code page is not valid UTF-8, so File.ReadAllText substitutes U+FFFD for its non-ASCII bytes. Writing that text back baked the loss in permanently: a CP1252 `é` (0xE9) came out as EF BF BD and the original byte was gone. The drift rewrite is what made this reachable, since the previous early-return never rewrote an existing block at all. Detecting the actual code page is undecidable, but detecting that the decode failed is not: U+FFFD cannot come out of a clean decode, so its presence is a reliable signal. EnsureBlock now returns without touching such a file, which also covers the append path. The trade is deliberate: a stale block in a profile we cannot read is left unrepaired rather than rewritten into garbage. --- app/Infrastructure/ShellIntegration.cs | 10 ++++++++ .../ShellIntegrationTests.cs | 24 +++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/app/Infrastructure/ShellIntegration.cs b/app/Infrastructure/ShellIntegration.cs index e8f5833..33795eb 100644 --- a/app/Infrastructure/ShellIntegration.cs +++ b/app/Infrastructure/ShellIntegration.cs @@ -620,6 +620,16 @@ internal static void EnsureBlock(string filePath, string markerStart, string mar var detectedBomEncoding = fileExists ? DetectBomEncoding(filePath) : null; var existing = fileExists ? File.ReadAllText(filePath) : string.Empty; + // A BOM-less profile saved in a legacy code page decodes with replacement chars, and + // writing that text back destroys the original bytes for good. U+FFFD cannot come out + // of a clean decode, so its presence is a reliable "we failed to read this" flag: leave + // the file alone rather than corrupt it. Detecting the actual code page is undecidable, + // which is why the block goes unrepaired here instead. + if (existing.Contains('\uFFFD')) + { + return; + } + var startIndex = existing.IndexOf(markerStart, StringComparison.Ordinal); if (startIndex >= 0) { diff --git a/tests/CopilotHere.UnitTests/ShellIntegrationTests.cs b/tests/CopilotHere.UnitTests/ShellIntegrationTests.cs index bd94318..33d1cde 100644 --- a/tests/CopilotHere.UnitTests/ShellIntegrationTests.cs +++ b/tests/CopilotHere.UnitTests/ShellIntegrationTests.cs @@ -1,3 +1,4 @@ +using System.Text; using CopilotHere.Infrastructure; using TUnit.Core; @@ -235,6 +236,29 @@ public async Task EnsureBlock_StaleBlock_PreservesUtf16Bom() await Assert.That(result).Contains("$HOME\\.local\\bin"); } + [Test] + public async Task EnsureBlock_UndecodableProfile_IsLeftByteForByteUnchanged() + { + // A BOM-less profile saved in a legacy code page (CP1252 here, 0xE9 for é) is not valid + // UTF-8, so File.ReadAllText substitutes U+FFFD and writing that text back would bake the + // loss in permanently. Asserting on bytes rather than decoded text matters: a text-level + // assertion passes while the corruption it is meant to catch still happens. + var profile = Path.Combine(_tempDir, "legacy-profile.ps1"); + var original = Encoding.ASCII.GetBytes($"{MarkerStart}\nstale-block\n{MarkerEnd}\nWrite-Host 'caf") + .Concat(new byte[] { 0xE9 }) + .Concat(Encoding.ASCII.GetBytes("'\n")) + .ToArray(); + File.WriteAllBytes(profile, original); + + var block = $"{MarkerStart}\nfresh-block\n{MarkerEnd}\n"; + ShellIntegration.EnsureBlock(profile, MarkerStart, MarkerEnd, block); + + // Compared as hex rather than as collections: the equivalency assertion pulls in + // reflection-based structural comparison, which this AOT-compiled project warns on. + var after = await File.ReadAllBytesAsync(profile); + await Assert.That(Convert.ToHexString(after)).IsEqualTo(Convert.ToHexString(original)); + } + [Test] public async Task EnsureBlock_StaleBlock_NoOriginalBom_WritesWithoutBom() { From 60a18a60fc37e899f65e97d52a3bf462d78cdc98 Mon Sep 17 00:00:00 2001 From: Gordon Beeming Date: Fri, 14 Aug 2026 02:28:05 +1000 Subject: [PATCH 5/5] fix(shell): test profile readability by decoding, not by looking for U+FFFD The previous guard treated any U+FFFD in the decoded text as proof the read had failed. That is wrong: U+FFFD is a legal character to write in a file, so a valid UTF-8 profile containing one deliberately was refused, and EnsureBlock returned without installing or repairing the block while the caller still reported success. Decoding strictly is the accurate test. A BOM-less file is now run through a UTF-8 decode that throws on invalid bytes: a valid profile passes whatever characters it contains, and legacy code page bytes throw and leave the file untouched as before. --- app/Infrastructure/ShellIntegration.cs | 33 +++++++++++++++---- .../ShellIntegrationTests.cs | 24 ++++++++++++-- 2 files changed, 47 insertions(+), 10 deletions(-) diff --git a/app/Infrastructure/ShellIntegration.cs b/app/Infrastructure/ShellIntegration.cs index 33795eb..7075844 100644 --- a/app/Infrastructure/ShellIntegration.cs +++ b/app/Infrastructure/ShellIntegration.cs @@ -618,18 +618,21 @@ internal static void EnsureBlock(string filePath, string markerStart, string mar // collapse any of these to UTF-8 unless the original encoding is detected up front and // carried through the write. var detectedBomEncoding = fileExists ? DetectBomEncoding(filePath) : null; - var existing = fileExists ? File.ReadAllText(filePath) : string.Empty; - // A BOM-less profile saved in a legacy code page decodes with replacement chars, and - // writing that text back destroys the original bytes for good. U+FFFD cannot come out - // of a clean decode, so its presence is a reliable "we failed to read this" flag: leave - // the file alone rather than corrupt it. Detecting the actual code page is undecidable, - // which is why the block goes unrepaired here instead. - if (existing.Contains('\uFFFD')) + // A BOM-less profile may be a legacy code page rather than UTF-8, and writing it back as + // UTF-8 destroys its non-ASCII bytes for good. Decoding strictly is the only reliable + // test: a valid UTF-8 profile that happens to contain U+FFFD passes, while CP1252 bytes + // throw. Checking the decoded text for U+FFFD instead would reject that valid profile, + // since the replacement character is itself a legal thing to write in a file. Identifying + // which legacy code page it is remains undecidable, so the block goes unrepaired rather + // than rewritten into garbage. + if (fileExists && detectedBomEncoding is null && !IsValidUtf8(filePath)) { return; } + var existing = fileExists ? File.ReadAllText(filePath) : string.Empty; + var startIndex = existing.IndexOf(markerStart, StringComparison.Ordinal); if (startIndex >= 0) { @@ -689,6 +692,22 @@ private static readonly (byte[] Preamble, Func MakeEncoding)[] BomSign (new byte[] { 0xFE, 0xFF }, () => Encoding.BigEndianUnicode), ]; + /// Strict UTF-8 decode used as a readability test for BOM-less files: throws, and so returns + /// false, only when the bytes genuinely are not UTF-8. + private static bool IsValidUtf8(string filePath) + { + try + { + new UTF8Encoding(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true) + .GetString(File.ReadAllBytes(filePath)); + return true; + } + catch (DecoderFallbackException) + { + return false; + } + } + /// Returns the encoding a leading byte-order mark identifies, or null if the file has none /// (the caller then falls back to writing BOM-less UTF-8, matching what a marker-less profile /// already looks like). diff --git a/tests/CopilotHere.UnitTests/ShellIntegrationTests.cs b/tests/CopilotHere.UnitTests/ShellIntegrationTests.cs index 33d1cde..218b7ef 100644 --- a/tests/CopilotHere.UnitTests/ShellIntegrationTests.cs +++ b/tests/CopilotHere.UnitTests/ShellIntegrationTests.cs @@ -240,9 +240,9 @@ public async Task EnsureBlock_StaleBlock_PreservesUtf16Bom() public async Task EnsureBlock_UndecodableProfile_IsLeftByteForByteUnchanged() { // A BOM-less profile saved in a legacy code page (CP1252 here, 0xE9 for é) is not valid - // UTF-8, so File.ReadAllText substitutes U+FFFD and writing that text back would bake the - // loss in permanently. Asserting on bytes rather than decoded text matters: a text-level - // assertion passes while the corruption it is meant to catch still happens. + // UTF-8, so a rewrite would bake in the replacement characters permanently. Asserting on + // bytes rather than decoded text matters: a text-level assertion passes while the + // corruption it is meant to catch still happens. var profile = Path.Combine(_tempDir, "legacy-profile.ps1"); var original = Encoding.ASCII.GetBytes($"{MarkerStart}\nstale-block\n{MarkerEnd}\nWrite-Host 'caf") .Concat(new byte[] { 0xE9 }) @@ -259,6 +259,24 @@ public async Task EnsureBlock_UndecodableProfile_IsLeftByteForByteUnchanged() await Assert.That(Convert.ToHexString(after)).IsEqualTo(Convert.ToHexString(original)); } + [Test] + public async Task EnsureBlock_ValidUtf8ContainingReplacementChar_IsStillUpdated() + { + // U+FFFD is a legal character to write in a file, so a valid UTF-8 profile can contain one + // deliberately. Treating its presence as "we failed to decode" would silently skip the + // install while still reporting success, so the readability test has to be a strict decode + // of the bytes rather than an inspection of the decoded text. + var profile = Path.Combine(_tempDir, ".bashrc"); + File.WriteAllText(profile, "# legacy note: � marker\n", new UTF8Encoding(false)); + + var block = $"{MarkerStart}\nexport PATH=\"$HOME/.local/bin:$PATH\"\n{MarkerEnd}\n"; + ShellIntegration.EnsureBlock(profile, MarkerStart, MarkerEnd, block); + var result = File.ReadAllText(profile); + + await Assert.That(result).Contains("# legacy note: � marker"); + await Assert.That(result).Contains(block); + } + [Test] public async Task EnsureBlock_StaleBlock_NoOriginalBom_WritesWithoutBom() {