Repository navigation
fix(ci): quote pip ':all:' flag that invalidated the workflow YAML (red badge) - #13
Merged
Merged
Conversation
Commit 9b44e76 (SonarCloud S8541/S8544 fix) changed the asciinema install line to: run: pip install --only-binary :all: asciinema==2.4.0 Unquoted, the colon+space inside ':all: asciinema' makes YAML parse the scalar as a nested mapping (reproduced with PyYAML: yaml.scanner.ScannerError: mapping values are not allowed here line 25, column 44) which invalidated the entire workflow file. Since that commit: - push runs are created but die in 0s with no jobs and no check runs - pull_request runs are not created at all, so PRs since then showed only CodeQL/SonarCloud checks while CI silently never ran - workflow_dispatch returns 422 and the workflow is registered under its file path instead of its name (symptoms of the parse failure) Fix: quote the flag - the security intent (no setup.py execution from sdists, pinned version) is preserved: pip install --only-binary ':all:' asciinema==2.4.0 Also mark the oh-my-posh winget install step continue-on-error (the stress test skips gracefully without the tool; this is the first CI run that will execute that step). Verified locally: yaml.safe_load now parses the file; name, all three triggers, and the build job resolve correctly.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Review feedback: the stress test must never silently skip on CI - a failed install has to fail the job, exactly like the silent-CI-gap this PR fixes. Replace continue-on-error winget with a deterministic, pinned direct download: - version pinned to 31.9.1 (manual bumps; no Dependabot ecosystem) - SHA256 verified against the pinned hash (supply-chain posture matches the SHA-pinned actions and pinned asciinema) - hard verification: the binary must execute and report its version - POSH_THEMES_PATH exported with atomic.omp.json from the same release so the test uses the glyph-rich theme
First real Linux CI run (the workflow had been invalid YAML since 9b44e76) exposed a platform dependency: Test-BsnRecordingEnvironment -Platform Windows checks [Environment]::OSVersion.Version before the redirected-stdio check, and on Linux that is the kernel version (major below 10), so the version error fired instead of the stdio error and the test failed. macOS passed only by luck (Darwin 23+). Skip both issue #8 tests unless running on a Windows host (5.1-safe detection: PSVersionTable.Platform guard with \Windows_NT fallback).
…atform detection Review feedback: the Windows-host guards inlined \System.Collections.Hashtable/ \Windows_NT detection, duplicating exactly the logic Bsn.Common owns (AGENTS.md: do not duplicate it in consumer modules). Both places now call the shared helper: - Start-InteractiveTranscript.Tests.ps1 (issue #8 tests): guard via (Get-BsnOperatingSystem) -ne 'Windows' - InteractiveTranscript.System.Tests.ps1: IsWindowsPlatform keeps its PS7+ requirement but delegates platform detection to the helper The string -eq comparison pattern already exists in production code (Get-BsnDefaultShell.ps1) and avoids enum type-literal resolution concerns in test scope.
Member
Author
Member
Author
…ings
Review feedback. Two lessons learned empirically:
1. Get-BsnDefaultShell.ps1: -eq 'Windows' relied on string-to-enum
coercion. Now uses a bare [BsnOperatingSystem]::Windows literal,
bound at compile time by a 'using module Bsn.Common' header (same
shape as Test-BsnRecordingEnvironment.ps1; the psm1 loader
self-heals PSModulePath before dot-sourcing).
2. Test files CANNOT use bare enum literals: 'Unable to find type
[BsnOperatingSystem]' at It-body runtime (verified by a failing
local run). The AGENTS.md hazards are not limited to attributes
and Mock proxies - test scope never binds the type. A 'using
module' header in a test file would execute before BeforeAll can
make Bsn.Common resolvable, so it is fragile too. Instead,
BeforeAll pulls the enum value from the defining module's own
scope (where the literal always resolves):
\ = & (Get-Module -Name 'Bsn.Common') { [BsnOperatingSystem]::Windows }
and the guards compare enum to enum.
The previous commit message claimed winget is 'not version-pinnable' - that is wrong: winget install supports --version for exact versions, and winget pin locks future upgrades (winget.tech/blog/winget-pin-lock-versions, microsoft/winget-cli#4002). The actual reasons for the direct download are: the repo can pin the artifact's SHA256 (same supply-chain posture as the SHA-pinned actions), and it avoids winget/msstore source flakiness on hosted runners.
Review decision: winget is the cleaner, idiomatic approach, and it supports exact version pinning: winget install --id JanDeDobbeleer.OhMyPosh --version 31.9.1 --exact winget verifies the downloaded installer against the winget-pkgs manifest InstallerSha256 during install, so no manual hash step is needed (winget hash exists for manual verification when required: https://learn.microsoft.com/en-us/windows/package-manager/winget/hash). The step still fails loudly (no continue-on-error): after install it verifies the binary exists, executes, and reports exactly 31.9.1, and that the themes directory (POSH_THEMES_PATH for the stress test) contains atomic.omp.json.
Review feedback: Get-BsnOperatingSystem alone is enough - the PSVersion Major -ge 6 term made a variable named IsWindowsPlatform mean 'Windows AND PowerShell 6+', which is misleading: - every recording test already asserts the PS 7 requirement explicitly with its own 'system tests require PowerShell 7 console infrastructure' skip (9 occurrences) - Invoke-BsnSystemRecording uses the flag for engine dispatch (Windows hidden-console runner vs Unix script(1)), where platform - not PowerShell edition - is the correct discriminator
First real run on windows-latest: winget install --version 31.9.1 succeeded, but the exe was not at the classic installer layout (Programs\oh-my-posh\bin) - the winget community package is a PORTABLE install: the exe lands under WinGet\Packages with a symlink in WinGet\Links (added to the user PATH registry, not to the step's process). The step now resolves the binary from known candidates (Links first, classic layouts as fallback) before verifying it, adds its directory to GITHUB_PATH, and treats themes opportunistically because portable packages do not ship them (the stress test falls back to the default theme, which still renders nerd-font glyphs).
Second windows-latest failure with empty WinGet\Links: the winget manifest for JanDeDobbeleer.OhMyPosh is InstallerType: appx - an MSIX (ohmyposh.cli), not a portable package. The executable is exposed via the app execution alias: %LOCALAPPDATA%\Microsoft\WindowsApps\oh-my-posh.exe (verified locally by installing the same pinned version). Candidate order now: MSIX alias, WinGet Links (if the manifest ever moves to portable), classic installer layout. Themes: the MSIX ships them inside the package directory, resolved via Get-AppxPackage (InstallLocation\\themes\\atomic.omp.json); classic Programs\\oh-my-posh\\themes remains a fallback and the test ultimately falls back to the default theme.
Third windows-latest failure: the alias WAS found, but executing the WindowsApps app-execution alias (a reparse point) from pwsh fails on hosted runners under redirected stdio: Program 'oh-my-posh.exe' failed to run: StandardOutputEncoding is only supported when standard output is redirected. Resolve everything from Get-AppxPackage instead: - verify the installed package version equals 31.9.1.0 - execute <InstallLocation>\oh-my-posh.exe (the real PE - verified working locally) for the version check - GITHUB_PATH gets InstallLocation (prepended), so the recorded scenario resolves 'oh-my-posh' to the real binary, never the alias - POSH_THEMES_PATH points at the package themes directory
|
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
ITNOA
Why the CI badge is red
Since commit 9b44e76 ("Fix SonarCloud security findings on pip install"), the
workflow file has been invalid YAML, so the CI workflow has not actually run:
The colon+space inside
:all: asciinemamakes YAML parse that scalar as anested mapping instead of a string. Reproduced with PyYAML:
Symptoms this explains
mainfail in 0s with no jobs and no check runsworkflow_dispatchreturns HTTP 422gh workflow listshows the workflow as its file path, notCILast real CI run: PR run
42290459537(headd2e097d, the commit before 9b44e76).PR #12's CI never ran - its green checks were the other workflows.
The fix
Quoting
':all:'removes the colon+space ambiguity. The security intent of9b44e76 is fully preserved:
oh-my-posh install is guaranteed, not best-effort
Review feedback: if the stress-test tool fails to install, the job must fail,
not silently skip the test - the same false-green trap as the broken workflow.
The step uses winget with an exact version pin (review decision; winget
supports
install --versionandpin):InstallerSha256during install (visible as "Successfully verified installerhash" in the logs) - no manual hash step needed
(winget hash docs)
Three windows-latest iterations were needed (each failed loudly, by design):
(
ohmyposh.cli), so the binary is not underPrograms\oh-my-posh\bin.WinGet\Linksstays empty. Theentry point is the app execution alias in
Microsoft\WindowsApps.stdio (
StandardOutputEncoding is only supported when standard output is redirected) - the alias is a 0-byte reparse point.Final approach: resolve the real package via
Get-AppxPackage, verify thepackage version is exactly
31.9.1.0, execute<InstallLocation>\oh-my-posh.exe(a real PE) for the version check, prepend
InstallLocationtoGITHUB_PATH(so the recorded scenario never resolves through the alias), and set
POSH_THEMES_PATHto the package's themes directory (atomic.omp.json).Verified on the runner: install + hash verification +
[+] records an oh-my-posh prompt burst with zero U+FFFD in the cast(~65s of real recording).Review fixes landed in this PR
BsnOperatingSystemenum, not strings, and notduplicated
$PSVersionTable/$env:OSlogic (review feedback). Gotchadiscovered en route: bare enum literals do not resolve from Pester test
scope (
Unable to find type [BsnOperatingSystem]), and ausing moduleheader in a test file executes before BeforeAll can make Bsn.Common
resolvable - so the tests pull the enum value from the defining module's
scope:
& (Get-Module Bsn.Common) { [BsnOperatingSystem]::Windows }.Get-BsnDefaultShell.ps1now uses the enum literal via a properusing moduleheader (same shape asTest-BsnRecordingEnvironment.ps1).IsWindowsPlatformis now pure platform semantics (review feedback):Get-BsnOperatingSystemalone decides it. The oldPSVersion.Major -ge 6term made it mean "Windows and PS6+", misleading for engine dispatch; every
recording test already asserts the PS 7 requirement with its own explicit skip.
in days exposed that
-Platform Windowschecks the OS version before theredirected-stdio check - on Linux that is the kernel version (major < 10),
so the version error fired and the test failed (macOS passed only by luck:
Darwin 23+ parses as >= 10). Both tests now skip unless on a Windows host.
Test-BsnRecordingEnvironmentis unchanged - the check order is correct forreal usage; only the tests were platform-dependent.
Verification
yaml.safe_loadparses the file;name: CI, all three triggers, and thebuildjob resolve correctlythe end-to-end proof the workflow runs again