fix(shell): stop the profile block baking in a snapshot of PATH - #131
Conversation
__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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthrough
ChangesProfile integrity
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The profile-generation fix and repair behavior are covered by targeted regression tests, and no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Fixes a shell profile regression where __copilot_update_profile wrote a PATH snapshot (expanding $HOME/$PATH at write time) into .bashrc/.zshrc, causing subsequent shells—especially on WSL—to lose dynamic PATH entries (e.g., Windows interop) and breaking commands like code .. The PR makes the shell-writer emit the same literal $HOME/$PATH block as the .NET ShellIntegration writer, and updates the .NET writer to heal already-corrupted blocks by reconciling drift inside the marked region.
Changes:
copilot_here.sh: quote the heredoc delimiter and emit the marker block (including$HOME/$PATH) literally to prevent install-time expansion.ShellIntegration.EnsureBlock: rewrite the marked region if it differs from the desired block (while preserving surrounding user config and avoiding unsafe rewrites when the end marker is missing).- Tests: add a regression integration test to ensure the profile block keeps literal
$PATH, and add unit tests covering the newEnsureBlockbehavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
copilot_here.sh |
Prevents install-time expansion by writing the profile heredoc block with a quoted delimiter and literal $HOME/$PATH. |
app/Infrastructure/ShellIntegration.cs |
Updates EnsureBlock to detect and repair stale/incorrect marked blocks without touching surrounding user content. |
tests/integration/test_cli.sh |
Adds an end-to-end regression test ensuring profile updates don’t bake PATH snapshots, plus fixes cleanup path resolution. |
tests/CopilotHere.UnitTests/ShellIntegrationTests.cs |
Adds focused unit coverage for EnsureBlock rewrite/append/no-op cases, including missing end-marker safety. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4867a68586
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Action performedReview finished.
|
…ewrite 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.
|
@codex review |
|
@coderabbitai review |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3f9ef1f98
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…le 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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
app/Infrastructure/ShellIntegration.cs:673
FileStartsWithUtf8Bomonly detects the UTF-8 BOM. If a PowerShell profile starts with a UTF-16/UTF-32 BOM,EnsureBlockwill treat it as BOM-less and rewrite the file as BOM-less UTF-8, which can reintroduce the same PowerShell 5.1 encoding problem this change is trying to avoid. Consider treating any Unicode BOM as “has BOM” for purposes of re-emitting a BOM on rewrite.
using var stream = File.OpenRead(filePath);
Span<byte> 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;
}
app/Infrastructure/ShellIntegration.cs:615
- The comment says "install.ps1 writes the PowerShell profile as UTF-8 with a BOM", but
install.ps1doesn’t write the PowerShell profile content (it only downloads/sourcescopilot_here.ps1and runs commands). This looks like it should reference the PowerShell profile updater (e.g.,copilot_here.ps1/Update-ProfileWithMarkers) to avoid misleading future maintainers about where the BOM originates.
// 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
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1828682642
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
app/Infrastructure/ShellIntegration.cs:651
- EnsureBlock() detects the original profile encoding (via BOM) and preserves it on the rewrite path, but the no-marker path immediately below uses File.AppendAllText without specifying encoding. If the existing profile is UTF-16/UTF-32 or UTF-8-with-BOM, appending with the default UTF-8 writer can corrupt the file by mixing encodings. Use the AppendAllText overload that takes an Encoding (or rewrite via WriteAllText) so the appended block uses the detected encoding too.
return;
}
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04cfe8af93
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…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.
Summary
__copilot_update_profileincopilot_here.shwrote the shell profile block with an unquoted heredoc delimiter, so$HOMEand$PATHexpanded while the file was being written. Profiles ended up holding the literal value of PATH from install time, and every shell start after that replaced the live PATH with that copy instead of prepending~/.local/binto it. On WSL that throws away the Windows interop entries, which is whycode .stopped resolving in #128.I reproduced the reported block byte for byte before changing anything, and confirmed
origin/mainstill carries the bug atcopilot_here.sh:235.Two things worth calling out:
case ":$PATH:"guard word was frozen too, so it tested the install-time PATH rather than the current one. On a second install the snapshot already contains~/.local/bin, the guard matches, and the export never runs. That is why this doesn't affect everyone.EnsureSourcedInUnixProfile) always wrote the literal$HOME/$PATHand was never the problem. Butinstall.shruns--update(the broken bash writer) before--install-shells, andEnsureBlockreturned early whenever it saw the start marker, so the correct writer could never repair the damage.Changes
copilot_here.sh: quote the heredoc delimiter, and write the markers and script path literally. The output is now byte-identical to the blockShellIntegrationwrites.app/Infrastructure/ShellIntegration.cs:EnsureBlockcompares the existing marked region against the block it wants and rewrites on drift. Config above and below the markers is untouched, and a start marker with no matching end is left alone so it can't swallow the rest of the file.tests/integration/test_cli.sh: regression test for the profile block, plus a fix forcleanup(), which built paths from$BASH_SOURCEafter the tests had changed directory and so never removedpublish/cli-test.tests/CopilotHere.UnitTests/ShellIntegrationTests.cs: cases for the newEnsureBlockbehaviour, including BOM preservation.Review found four more problems in the first version of this branch, all fixed here:
__copilot_updatedownloaded the new script but called__copilot_update_profilebefore sourcing it, so the first update after a broken release still ran the old in-memory writer and re-baked the snapshot. The reload now happens before the profile writes. Without this, healing an affected machine would have taken two updates rather than one.EnsureBlock's rewrite path used theFile.WriteAllTextoverload that emits BOM-less UTF-8.install.ps1writes the PowerShell profile as UTF-8 with a BOM, which Windows PowerShell 5.1 needs to avoid falling back to the legacy code page, so a rewrite would have stripped it and garbled any non-ASCII content in the profile. The rewrite now detects the original BOM state and preserves it, across UTF-8, UTF-16 and UTF-32.File.ReadAllTextsubstitutes U+FFFD and writing that text back destroyed the original bytes. A CP1252é(0xE9) came out asEF BF BD.EnsureBlocknow leaves such a file untouched. Identifying which legacy code page it is remains undecidable, so the trade is deliberate: a stale block in a profile we cannot read stays unrepaired rather than being rewritten into garbage.Anyone already affected gets healed by a single
copilot_here --updateonce this ships, with no manual.bashrcediting.Test plan
dotnet test tests/CopilotHere.UnitTests: 620 passed, 0 failed.dotnet test tests/CopilotHere.IntegrationTests: 5 passed, 0 failed.bash tests/integration/test_cli.sh: 12 passed, 0 failed.--install-shellsagainst a throwaway$HOMEholding the corrupted block. It rewrote the block in place and kept the surrounding config.Closes #128
Summary by CodeRabbit
Bug Fixes
$HOMEand$PATHreferences, preventing incorrect installation-time substitutions.Tests