Pass Windows packaged smoke marker through environment - #98
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: dbalders/TritonAI-Installer/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
dbalders
left a comment
dbalders
left a comment
There was a problem hiding this comment.
TritonAI Review
No actionable findings in the inspected changes; see coverage limits.
Reviewed d800a51ba114f11a2907c20a8b85a11ba67b6d98 against 20ebc290eec74a4ca99cf88b9910224ed837d3f7.
Inspected 1 file(s) in 1 part(s); reused 0 unchanged review part(s).
Changes
scripts/verify-windows-packaged-boot.ps1: scripts/verify-windows-packaged-boot.ps1 now passes the packaged-boot smoke trigger to both Windows candidates (portable NSIS wrapper and installed exe) via the TRITONAI_INSTALLER_SMOKE_MARKER environment variable using Start-Process -Environment, replacing the previous NSIS command-line argument. The marker parent directory is resolved with node -p "require('node:os').tmpdir()" instead of [IO.Path]::GetTempPath(), and candidate-process diagnostics now include CommandLine.
Coverage limits
- scripts/verify-windows-packaged-boot.ps1: For the P2 finding above: no CI/release-runner definition is included, so I could not determine whether the hosted packaging environment pins a pwsh version >= 7.4. If such a pin exists, the finding should be treated as a hardening/pinning gap rather than an active break; if not, it is a live release-gate failure on older pwsh.
- scripts/verify-windows-packaged-boot.ps1: Some candidate findings were omitted because their evidence or location could not be verified.
- scripts/verify-windows-packaged-boot.ps1: The diff switches the Windows smoke transport to an environment variable and to a Node-resolved temp directory, but the Electron-side contract is unchanged code outside this diff and is not included in the context. I could not independently verify (a) that the app's packaged-boot smoke trigger reads exactly TRITONAI_INSTALLER_SMOKE_MARKER, (b) how strictly the app compares the marker's parent directory against its own os.tmpdir() (i.e. whether the system-Node lookup at RIGHT line 74 truly matches the Electron 42 embedded-Node computation), or (c) how the app derives the
$MarkerPath.userdatadirectory that the finally block deletes. The PR states local validation passed, which supports the parity claim, but confirmation requires the app-side smoke implementation and the macOS packaging script that already exercises this transport.
CI snapshot
Combined commit status: success
- status CodeRabbit: success Review skipped: automatic reviews are disabled
Check runs:
- Greptile Review: status=completed conclusion=success app=
- Macroscope - Correctness Check: status=completed conclusion=skipped app=
- macos-latest: status=completed conclusion=success app=
- windows-latest: status=in_progress conclusion=none app=
CI is reported separately. This code review does not claim to have run tests.
Comment /triton-review to request a fresh review. Findings are advisory; GitHub branch protection remains authoritative.
dbalders
left a comment
dbalders
left a comment
There was a problem hiding this comment.
TritonAI Review
No actionable findings in the inspected changes; see coverage limits.
Reviewed ca98ead9d9558c2d64aa74709c5490b47654f342 against 20ebc290eec74a4ca99cf88b9910224ed837d3f7.
Inspected 1 file(s) in 1 part(s); reused 0 unchanged review part(s).
Changes
scripts/verify-windows-packaged-boot.ps1: This PR changes the Windows packaged-boot verification script to trigger the Installer's smoke mode via the TRITONAI_INSTALLER_SMOKE_MARKER environment variable (using ProcessStartInfo with UseShellExecute=false) instead of the previous --tritonai-installer-smoke-marker command-line argument, resolves the marker's parent directory with node's os.tmpdir() rather than [IO.Path]::GetTempPath(), and adds CommandLine to the candidate-process diagnostics printed when the marker never appears. Readiness-marker validation, timeout/termination handling, and cleanup flow are otherwise unchanged.
Coverage limits
- scripts/verify-windows-packaged-boot.ps1: Only an excerpt of the script is supplied; lines 1-54 (parameter declarations and defaults such as $TerminationTimeoutMilliseconds, any $ErrorActionPreference setting, and the definitions of Wait-ForOwnedProcess, Wait-ForPathState, and Test-SmokeMarker) are omitted. The changed lines depend on these (e.g., the & node invocation at line 74 relies on the script's error-action preference to fail safely if node is not resolvable, and lines 109/112-117 rely on Wait-ForOwnedProcess's exit semantics). Also, scripts/test-windows-signing.ts is only partially shown; the omitted portion could contain assertions on the removed command-line smoke transport, which would be caught only by running the test suite.
- scripts/verify-windows-packaged-boot.ps1: The macOS packaging/harness code that sets the smoke environment variable and the packaged-boot smoke contract test are not provided, so the claimed macOS/Windows transport parity and the PR's statement that 'smoke request contract tests pass locally' cannot be independently confirmed from this context.
- scripts/verify-windows-packaged-boot.ps1: The new transport at scripts/verify-windows-packaged-boot.ps1 lines 84-87 ('$StartInfo.Environment['TRITONAI_INSTALLER_SMOKE_MARKER'] = $MarkerPath') cannot be verified against the app's actual smoke trigger because src/packaged-boot-smoke.ts is not in the supplied context. Manual inspection is required to confirm (a) the environment variable name and casing match the app's reader exactly, (b) the env-var trigger is not gated to darwin (the PR only asserts it is 'already exercised by Mac packaging'), and (c) the app's strict Windows marker-path validation (which the new node os.tmpdir() lookup at lines 73-76 is meant to satisfy) performs the same TEMP/TMP resolution Node uses, so pwsh's node output is byte-identical to the packaged Electron child's tmpdir. A mismatch in any of these would make both Windows boot candidates time out and fail the release gate.
CI snapshot
Combined commit status: success
- status CodeRabbit: success Review skipped: automatic reviews are disabled
Check runs:
- Greptile Review: status=completed conclusion=success app=
- macos-latest: status=completed conclusion=success app=
- windows-latest: status=completed conclusion=success app=
- Macroscope - Correctness Check: status=completed conclusion=skipped app=
CI is reported separately. This code review does not claim to have run tests.
Comment /triton-review to request a fresh review. Findings are advisory; GitHub branch protection remains authoritative.
1 participant
The signed portable wrapper launches the Installer process, but the child never produces the smoke readiness marker even after the extraction allowance. Use the app's existing environment-variable smoke trigger, already exercised by Mac packaging, rather than relying on NSIS command-line forwarding.
Resolve the marker's parent with Node's own temporary-directory lookup to match Electron's strict path check on Windows. Keep all readiness, health, signature, and process-exit requirements. Include command lines in candidate-process diagnostics if launch still fails.
Validation: build, Windows signing tests, and smoke request contract tests pass locally. Native CI parses the PowerShell script; hosted packaging must still prove both Windows boot paths.