Uh oh!
There was an error while loading. Please reload this page.
fix(windows): strip UTF-8 BOM from install.ps1 - #178
Merged
Conversation
install.ps1 was committed with a UTF-8 BOM (EF BB BF). When the documented official install command -- irm https://aka.ms/conductor/install.ps1 | iex -- is run, Invoke-RestMethod returns the body as a single System.String with the BOM surviving as U+FEFF at index 0. Piping that string to Invoke-Expression makes PowerShell's in-memory parser fail on the [CmdletBinding()] attribute that follows the comment header: Unexpected attribute 'CmdletBinding'. This breaks two user-facing flows: 1. The README install/update command (irm | iex) -- nothing installs. 2. conductor update --apply -- the spawned new console runs the same irm | iex command and dies with the same parse error. Direct file invocation (powershell.exe -File install.ps1) does NOT exhibit the bug because PowerShell's file loader handles the BOM differently from the in-memory iex parser, which is why the existing install-script integration tests (which all use -File) didn't catch it. Fix: re-save install.ps1 as UTF-8 without BOM. One-byte content change, zero behavior change. Regression prevention: - tests/test_integration/test_install_script_files.py: fast byte-level static checks (BOM-free, LF line endings, shebang). Runs on every CI test job, not gated behind the install_scripts marker. - tests/test_integration/test_install_scripts.py: a new Windows-only test that mirrors the irm | iex code path by reading install.ps1 bytes ourselves and feeding them to [ScriptBlock]::Create -- which is what iex uses internally to parse input. It does NOT execute the script (so it stays fast and safe) but does surface the BOM-induced parse error if one regresses. Gated behind the install_scripts marker so it runs in the existing Windows install-scripts CI job. - .gitattributes: pins install.ps1 to UTF-8 without BOM and LF line endings so editors that auto-add BOMs are caught at git add time. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CI failure on Windows-latest after the BOM strip revealed a deeper encoding issue: Windows PowerShell 5.1 (the `powershell.exe` shipped with Windows 10/11) does NOT default to UTF-8 for files without a BOM. It falls back to the system code page (Windows-1252 on US/EU systems). The script's multi-byte UTF-8 characters (->, [OK], [X], em-dash, ellipsis, bullet, box-drawing) got mis-decoded into Windows-1252 sequences containing U+201C (left curly quote, byte 0x93) -- which PowerShell 5.1 treats as a valid alternative string delimiter. That derailed the parser and produced cascading "unexpected token '}'" errors at every function boundary. The BOM that this PR stripped to fix issue #175 was load-bearing for the `-File` invocation on PS 5.1: it forced UTF-8 interpretation. Now that the BOM is gone, the script must be encoding-independent. Fixes: 1. install.ps1: replace all non-ASCII chars with ASCII equivalents: -> for -> (U+2192 right arrow) [OK] for check (U+2713) [X] for cross (U+2717) -- for em-dash (U+2014) ... for ellipsis (U+2026) * for bullet (U+2022) - for box horiz (U+2500) Now the script is pure ASCII and parses identically regardless of how PowerShell guesses the encoding. Test fixes from PR review: 2. test_install_ps1_parses_via_iex_pipeline (CRITICAL): The test was using `[System.IO.File]::ReadAllText(path)` (one-arg overload), which constructs a StreamReader with `detectEncodingFromByteOrderMarks: true` and SILENTLY STRIPS the UTF-8 BOM. So a regressed BOM would never reach `[ScriptBlock]::Create` and the test would falsely pass. Switched to `ReadAllBytes` + `Encoding.UTF8.GetString` which preserves the BOM in the resulting string -- exactly mirroring what `Invoke-RestMethod` does. Added a "DO NOT swap" warning in the docstring so a future maintainer doesn't simplify it back. 3. New: test_iex_pipeline_test_actually_catches_bom_regression "Test the test" -- prepends a literal U+FEFF in PowerShell and asserts `[ScriptBlock]::Create` rejects it. If this ever passes without raising, the parse-path test above has rotted. 4. New: test_install_ps1_is_pure_ascii Locks in the ASCII-only invariant so a future maintainer can't re-introduce a Unicode character that breaks PS 5.1 again. 5. New: test_install_ps1_has_no_utf16_bom UTF-16 BOMs would also break `irm | iex` and are an easy mistake in Windows editors. Cheap defense in depth. 6. New: test_install_sh_uses_lf_line_endings Mirrors the LF check on install.ps1. CRLF in install.sh would break the shebang and produce "$'\r': command not found" errors. Comment fixes from PR review: 7. .gitattributes: removed misleading `working-tree-encoding=UTF-8` attribute (it does NOT enforce no-BOM -- it only controls encoding *conversion* between repo and working tree). Added a note clarifying that the byte-level test is the actual guarantee. 8. test_install_scripts.py: replaced the inline "here-string" comment (incorrect PowerShell terminology) with "Python f-string" and tightened the parse-path docstring to explicitly explain the file-loader-strips-BOM-but-irm-doesn't asymmetry. 9. test_install_sh_has_no_utf8_bom: tightened rationale to mention the shebang-detection failure first (more universally true than the "literal command" symptom). 10. Replaced em-dashes in Python docstrings with `--` to keep the style consistent across the repo's install-script test files. Verification: - All 7 static tests pass in 0.01s (default suite). - Negative test confirmed: re-introducing any non-ASCII byte fires test_install_ps1_is_pure_ascii with a clear message. - Negative test confirmed: re-introducing a BOM fires test_install_ps1_has_no_utf8_bom. - Full default suite: 2521 passed, 9 skipped, 1 unrelated flaky perf test (passes on retry; excluded from CI anyway). - make lint clean. - install.ps1 brace balance verified. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Apply low-risk simplifications surfaced by the code-simplifier review on the BOM-fix PR. Behavior unchanged; tests still pass. S1: Extract `_run_pwsh()` helper in test_install_scripts.py. Both new Windows-only parse tests duplicated the same 11-line `subprocess.run(["powershell.exe", "-NoProfile", ...], ...)` boilerplate. Hoisted into a single helper alongside `_kill`. S2: Use `bytes.isascii()` in test_install_ps1_is_pure_ascii. The previous `[(i, b) for i, b in enumerate(data) if b > 127]` always allocated the full list of offsets even on the (common) passing case. `data.isascii()` is the idiomatic fast-path; we only do the full scan to find the first offending byte for the error message when the assertion is about to fail. S4: Drop redundant `install.sh text eol=lf` from .gitattributes. Already covered by the `*.sh text eol=lf` pattern below. Also tightened the comment header to be more concise and pointed at the test as the canonical source of the BOM rationale. Skipped from the review: - S3 (consolidate per-test BOM docstrings into the module docstring): per-test docstrings are read in isolation when a test fails. A pointer to "see module docstring" forces the developer to navigate away from pytest output. Drift risk on these static encoding facts is low; standalone-readable failure context is more valuable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two follow-up cleanups surfaced during PR review and made visible by
the ASCII-fication of install.ps1:
1. install.ps1: harmonize Write-Warn's prefix.
After replacing the Unicode check/cross with [OK]/[X], Write-Warn
was the odd one out using a bare `!`. Bracket it as `[!]` so all
four status writers use a consistent two-character bracketed
marker. Write-Info keeps `->` since it's a pointer (continuation
of action), not a status. Final mapping in install.ps1:
Info -> "->"
Ok -> "[OK]"
Warn -> "[!]"
Err -> "[X]"
install.sh is intentionally not changed -- POSIX terminals
handle Unicode fine, so it keeps its richer ✓/✗/→/! prefixes.
The PowerShell side is the one that has to be ASCII because
PS 5.1 mangles UTF-8 without a BOM.
2. tests/test_integration/test_install_scripts.py: update the
comment in _assert_install_ok to reflect the new ASCII output
("[OK] Verified:" instead of "✓ Verified:"). The assertion logic
is unchanged -- it grep-matches "Conductor"/"conductor", which
is unaffected by the prefix swap.
No consumers (logs, downstream parsers, docs) referenced the old
bare-`!` prefix, verified by grep across src/, tests/, docs/, and
the install scripts themselves.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>Uh oh!
There was an error while loading. Please reload this page.
Jason Robert (jrob5756) added a commit
that referenced
this pull request
May 13, 2026
Release notes for 0.1.15 added to CHANGELOG.md. Added: - Per-agent timeout_seconds for hard wall-clock timeouts (#150) - Auto-discovery of .github/instructions/**/*.instructions.md with applyTo filtering, plus polymorphic conventions refactor (#169) Fixed: - system_prompt is now rendered and forwarded to providers (#179) - conductor update on Windows: install scripts become the single upgrade path; install.sh reaches parity (#171) - install.ps1 UTF-8 BOM breaking the irm | iex one-liner (#178) - conductor stop crash on Windows from os.kill(pid, 0) liveness probe (#176) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#175.
Summary
install.ps1was committed with a UTF-8 BOM (EF BB BF). When the documented official install command ---- is run,
Invoke-RestMethodreturns the body as a singleSystem.Stringwith the BOM surviving asU+FEFFat index 0. Piping that string toInvoke-Expressionmakes PowerShell's in-memory parser fail on the[CmdletBinding()]attribute right after the comment header:This breaks two user-facing flows that the previous PR (#171) shipped as the headline supported install/upgrade path:
irm https://aka.ms/conductor/install.ps1 | iex) -- nothing installs.conductor update --apply-- the spawned new console runs the sameirm | iexand dies with the same parse error.install.shis unaffected (no BOM).Root cause and why CI didn't catch it
PowerShell handles BOMs differently across two parse paths:
powershell.exe -File install.ps1Invoke-RestMethod | Invoke-Expression(or[ScriptBlock]::Create($string))U+FEFFin the string and breaks the parser. Fails.Every existing test in
tests/test_integration/test_install_scripts.pyinvokes the script viapowershell.exe -File ...(the file-loader path), so the bug was invisible to CI even though it broke the canonical README command real users hit.Fix
One-byte content change: re-save
install.ps1as UTF-8 without BOM. Zero behavior change to the script itself.Regression prevention
To make sure this class of bug can't sneak back in:
1.
tests/test_integration/test_install_script_files.py(new, 4 tests, runs in default suite)Fast byte-level static checks that read the script bytes from disk and assert:
install.ps1does not start withEF BB BFinstall.shdoes not start withEF BB BFinstall.shstarts with a#!shebanginstall.ps1uses LF line endings onlyThese are microsecond-scale and run in the regular
make testsuite, so a regressed BOM fails CI on every PR -- not just the slower install-scripts job that runs after lint.2.
tests/test_integration/test_install_scripts.py(one new test,install_scriptsmarker, Windows-only)test_install_ps1_parses_via_iex_pipelinemirrors exactly theirm | iexcode path that the existing-Filetests miss. It readsinstall.ps1bytes itself (just likeInvoke-RestMethodwould) and feeds them to[ScriptBlock]::Create-- the same parse pathInvoke-Expressiontakes internally. It does not execute the script, so it stays fast and safe, but it does surface the BOM-induced parse error.3.
.gitattributes(new)Pins
install.ps1to UTF-8 without BOM and LF line endings so editors (especially on Windows) that auto-add BOMs are caught atgit addtime before the bytes ever reach a commit.Verification
make lintclean.[ScriptBlock]::Createtest will run on the Windows leg of the existinginstall-scriptsCI job.Files changed
install.ps1.gitattributestests/test_integration/test_install_script_files.pytests/test_integration/test_install_scripts.py[ScriptBlock]::Createparse test