fix(shell): stop the profile block baking in a snapshot of PATH

app/Infrastructure/ShellIntegration.cs

@@ -1,4 +1,5 @@
 using System.Net.Http;
+using System.Text;
 
 namespace CopilotHere.Infrastructure;
 
@@ -597,18 +598,67 @@ 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))
     {
       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 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;
+
+    // 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;
 
-    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))
+      {
+        var rewritten = string.Concat(existing.AsSpan(0, startIndex), desired, existing.AsSpan(endIndex));
+        var writeEncoding = detectedBomEncoding ?? new UTF8Encoding(encoderShouldEmitUTF8Identifier: false);
+        File.WriteAllText(filePath, rewritten, writeEncoding);
+      }
+
       return;
     }
 
@@ -630,4 +680,52 @@ private static bool FileContains(string path, string value)
       return false;
     }
   }
+
+  // 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<Encoding> 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),
+  ];
+
+  /// 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).
+  private static Encoding? DetectBomEncoding(string filePath)
+  {
+    using var stream = File.OpenRead(filePath);
+    Span<byte> 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;
+  }
 }

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
@@ -205,8 +210,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,21 +235,26 @@ __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
     *":$HOME/.local/bin:"*) ;;
     *) 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;

tests/CopilotHere.UnitTests/ShellIntegrationTests.cs

@@ -1,3 +1,4 @@
+using System.Text;
 using CopilotHere.Infrastructure;
 using TUnit.Core;
 
@@ -117,6 +118,186 @@ 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 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_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_UndecodableProfile_IsLeftByteForByteUnchanged()
+  {
+    // A BOM-less profile saved in a legacy code page (CP1252 here, 0xE9 for é) is not valid
+    // 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 })
+      .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_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()
+  {
+    // 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()
   {