Conversation
ca7770e to
eadce10
Compare
|
The This branch changes three files: Checked rather than asserted, on Windows 11 with So it is not a red main either, which was the other candidate worth ruling out. GitHub would not let me re-run the single job, so I rebased onto current The other red, |
eadce10 to
02db514
Compare
|
Rebased onto current One check is still red, and it needs a maintainer rather than a commit:
Why there is a workflow hunk here at allI would rather not touch
What the hunk actually is, so the review is cheapThirteen lines, all of them two
Both steps invoke a script in The diff is the whole thing: - name: Language mode preflight (pwsh 7)
shell: pwsh
run: pwsh -NoProfile -ExecutionPolicy Bypass -File scripts/tests/test-install-ps1-language-mode.ps1
- name: Language mode preflight (Windows PowerShell 5.1)
shell: powershell
run: powershell -NoProfile -ExecutionPolicy Bypass -File scripts/tests/test-install-ps1-language-mode.ps1If you would rather not label itEntirely reasonable, and the alternative is one commit away: say the word and I will drop the workflow hunk, leaving this PR as Either way, the fix itself and its regression test are unaffected and green. |
|
Historical current-main validation receipt, superseded by the later topology decision below:
This comment no longer requests replacement or closure of #90128. |
|
Current-main validation receipt: #91196 rebased this exact three-file implementation surface onto current GitHub currently reports this original PR mergeable. I am keeping #90128 as the canonical contribution/provenance owner and closing #91196 as a validation duplicate rather than replacing the original contributor's PR. |
…NET error AppLocker and WDAC enforcement put PowerShell in ConstrainedLanguage, which refuses method calls on non-core .NET types. install.ps1 makes more than forty of those calls, and the first runs at script scope inside Set-LongProfileEnvVars, so the script dies before it honors any parameter. Even -Manifest and -ProtocolVersion, which are read-only queries that touch nothing, fail with MethodInvocationNotSupportedInConstrainedLanguage naming a line number inside a cached copy of a script the operator never wrote (NousResearch#89857). Detect the language mode first, using only constructs the mode allows, and fail with what is wrong and what an administrator has to change. This is a refusal rather than a workaround: the restriction is on the language, not on this script, so no flag can make the rest of the file run. The message pre-empts -ExecutionPolicy Bypass explicitly, because execution policy and language mode are separate controls and reaching for the first is the reported next step. Refs NousResearch#89857
02db514 to
8b9984b
Compare
|
@andrexibiza that's a genuinely unusual thing to do and I want to name it rather than just say thanks: you did the rebase work, proved it green across CI/Docker/Nix, and then closed your own PR so the provenance stayed with the original contributor. Plenty of projects would have just merged #91196 and moved on. Noted and appreciated. I've now put that on the canonical PR so your validation isn't stranded on a closed one. Re-ran the suite locally on the rebased head, Windows 11 / PowerShell 5.1, all 13 assertions pass: The two negative assertions are the ones I'd point a reviewer at: the raw .NET error never reaching the operator, and no The only red check remaining is One thing still open from my side, unchanged by the rebase: the guard refuses rather than degrading, so an operator in ConstrainedLanguage gets a clear stop instead of a partial install. I think refusing is right for an installer, but if maintainers would rather it attempt a reduced-functionality path, that's a policy call and I'd rather hear it than assume. |
Read this first: it does not make the install work
Refs #89857, notFixes. This does not let anyone install under Constrained Language Mode. It makes the failure legible, and it stops the installer from dying in a way that points the operator at the wrong thing.Whether Hermes should support CLM at all is a maintainer decision, and I have put the measurements for it at the bottom rather than deciding it in a PR.
What does this PR do?
install.ps1makes more than forty method calls on non-core .NET types. Constrained Language Mode refuses every one of them. The first is at script scope, so it fires before the script has looked at a single parameter:That is why the report's log shows
stage=__manifest__:-Manifestis a read-only query that touches nothing on disk, and it still cannot answer. Same for-ProtocolVersion. The bootstrap gets exit 1 and forwards a raw .NET error, localized into the host language, naming line 312 of a file inAppData\Local\hermes\bootstrap-cache\that the operator did not write. Nothing in that output contains the words "language mode".This adds a preflight at the top of the script that detects the mode and says so. Every construct in it is CLM-legal: a property read on an automatic variable, string concatenation,
Write-Host, andWrite-Error.$host.UI.WriteErrorLineis not legal there, and neither is[Console]::Error.WriteLine(whichWrite-PathDiaguses) -- both are method calls on non-core types and would throw the exact error the block exists to explain.Related Issue
Refs #89857
Type of Change
Changes Made
scripts/install.ps1- a language mode preflight immediately after$ErrorActionPreference = "Stop", before the UTF-8 console block and well before the 8.3 normalization that currently kills the run. Fires on anything that is notFullLanguage, prints an explanation, and exits 1. 57 lines added, 0 changed - no existing line is touched.scripts/tests/test-install-ps1-language-mode.ps1- new, following theAssert-Equal/Assert-Trueshape of the three tests already in that directory..github/workflows/installer-tests.yml- runs the new file on pwsh 7 and Windows PowerShell 5.1, the same pair the 8.3 test already uses.What the operator sees now
The
-ExecutionPolicy Bypasssentence is there because it is the reporter's own step 2. They tried it, it did not help, and nothing told them why. It is the sentence I would most want kept if the message gets trimmed.Three decisions worth a maintainer's eye
1. It refuses rather than degrades. A partial port that made line 312 CLM-safe would move the crash to the next blocked call and leave a half-configured tree behind. I would rather the installer stop while it can still explain itself. If you would prefer best-effort-then-fail, the guard becomes a warning and the mutation table below tells you which test to expect to flip (M3).
2. Exit code 1, not a new one. The stage protocol documents
0success,1generic failure,2unknown stage. A distinct code (say3) would let the Rust bootstrap render this specially instead of forwarding text, but that extends a documented contract and drivers would need to learn it. I used1. Say the word and I will add3plus the protocol doc entry.3. No override flag. There is deliberately no
-IgnoreLanguageMode. Anything that got past the guard would fail ~200 lines later with the original error, so the flag's only real effect would be to restore the confusing failure.How to Test
No AppLocker or WDAC policy is needed on the runner: the test drops a child runspace into
ConstrainedLanguagewith$ExecutionContext.SessionState.LanguageMode = 'ConstrainedLanguage', which is the same restriction the policy applies, and runsinstall.ps1inside it as a real subprocess.To see the old behavior, revert
scripts/install.ps1and re-run the first file: five assertions fail and the raw error comes back.Verification
The failure reproduces exactly, on a real Windows 11 box. Against pristine
upstream/mainin a constrained child:Same line, same character offset, same error id as the report -- in English rather than the reporter's Spanish, which is itself the point: the text an operator gets is host-localized, so it cannot be searched for.
Mutation proof - 6 mutations, 6 caught:
FullLanguageinsteadexit 1removed, so it warns and continues$host.UI.WriteErrorLineinstead ofWrite-Error-ExecutionPolicy Bypasssentence droppedMutation 6 is the one worth looking at: the guard is only useful before
Set-LongProfileEnvVarsruns at script scope, and nothing about the code's appearance says so. That assertion is what stops a future reshuffle from silently reverting this.The test cannot pass vacuously. Its first two assertions check the harness itself: that the child reports
ConstrainedLanguage, and that[Environment]::GetEnvironmentVariablereally is refused inside it. Without those, a host where the mode assignment silently did not take would run every remaining assertion against FullLanguage and report green.Platform: Windows 11, Windows PowerShell 5.1. I could not run pwsh 7 locally - it is not installed on this machine - so the pwsh 7 job added to
installer-tests.ymlis the first real execution of this file under 7. Flagging that rather than implying I tested both.scripts/install.ps1stays pure ASCII with CRLF endings, as its own header requires.Checklist
Code
ConstrainedLanguage/LanguageModeacross open PRs returns nothingDocumentation & Housekeeping
cli-config.yaml.exampleif I added/changed config keys or N/A: no config keysCONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows or N/AFullLanguage(which includes every non-Windows PowerShell) the guard is a single string comparison and does nothingWhat a real CLM port would cost, if you want one
I measured this before choosing to refuse, and the answer is not "impossible", it is "a decision with trade-offs":
[Environment]::GetEnvironmentVariable($n)(process scope)(Get-Item "Env:$n").Valueor$env:$nSet-Item "Env:$name"for the write one line below the read that fails[Environment]::GetEnvironmentVariable("Path","User"|"Machine")and the matchingSetEnvironmentVariableGet-ItemProperty/Set-ItemPropertyonHKCU:\Environmentand the machine keyWM_SETTINGCHANGEbroadcast .NET does, so open shells keep a stale PATH[Environment]::GetFolderPath(...)Get-LongProfileRoot; the shortcut sites at ~4131 have no equivalentNew-Object System.Diagnostics.ProcessStart-Process[System.IO.File]::WriteAllText($p,$s,$utf8NoBom)Set-Content -Encoding utf8UTF8Encoding($false)deliberatelyAdd-TypeP/Invoke forGetLongPathNameWI verified each replacement in a constrained runspace rather than reasoning about it --
Get-Item Env:,$env:,Get-ItemProperty HKCU:\EnvironmentandConvertTo-Jsonare all allowed;[Environment]::*,[Console]::*and$host.UI.WriteErrorLineare all refused.The two rows that make it a decision rather than a chore are the PATH broadcast and the BOM. Both are silent behavior changes on hosts that are working fine today, in exchange for supporting a locked-down configuration. Happy to do the port as a follow-up if that trade is one you want to make, but it should be its own PR with its own argument, not smuggled in behind an error message.