Validate the trunk merge for #28 (do not merge) - #394
Closed
mokagio wants to merge 6 commits into
Closed
Conversation
Three conflicts, all resolved in trunk's favour: - `.buildkite/pipeline.yml`: this branch ran `npm run build:once` as its own Buildkite step before `npm run dist:win`. Trunk's `dist:win` now chains `build:once` itself (#122), so the explicit step is redundant. The hardening this branch adds — `Invoke-NativeCommand` around the native calls, and the extracted `verify_windows_signature.ps1` — is kept. - `scripts/azure-sign.cjs`: trunk landed the same `/debug` passthrough with an explanatory comment; kept the comment. - `test/azure-sign.test.cjs`: trunk moved the suite to `tests/unit/`, so the `require` of `scripts/azure-sign.cjs` gains a path segment. `npm test` (1046 passing) and `npm run lint` are green on the merge result. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Validation-only PR carrying #28’s Windows signing hardening after merging current trunk.
Changes:
- Makes Windows native build commands fail closed.
- Validates signing tools and packaged artifact identity.
- Adds a Buildkite unit-test step.
Review findings: 1 [fix here] · 0 [follow-up].
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
.buildkite/pipeline.yml |
Hardens Windows execution and delegates signature verification. |
.buildkite/commands/verify_windows_signature.ps1 |
Verifies artifacts and signer identity. |
.buildkite/commands/setup_windows_code_signing.ps1 |
Adds integrity checks for signing tools. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Write-Host "$Description Authenticode signature is valid: $subject" | ||
| } | ||
|
|
||
| Assert-SigningToolIntegrity ` |
Added here on 2026-07-06, when `npm test` ran in no CI at all. #68 landed three weeks later and now runs the unit suite in GitHub Actions on macOS and Windows, on both the system Node and the one Electron bundles — so this step is a strict subset of that, on a self-hosted agent that holds the signing credentials. The suite needs no secrets, which is what makes Actions the right place for it: #65 sets the split as everything secretless on Actions, Buildkite only for the signed artifacts, and lists "Buildkite stays untouched and keeps producing the signed artifacts" as a constraint. --- Generated with the help of Claude Code, https://claude.com/claude-code Co-Authored-By: Claude Opus 5 <noreply@anthropic.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 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.
Validation only — do not merge. Throwaway PR to prove #28 still works on current
trunk. Close it once CI has answered; the merge commit it carries is what I intend to push to #28's own branch.Why
#28 was opened on 2026-07-06 and
trunkhas moved a long way since — 164 commits, including the move oftest/totests/unit/,dist:wingaining its own renderer build (#122), and the whole GitHub Actions suite (#68, #362). Mergingtrunkinto it produced three conflicts. This PR exists so CI answers whether my resolutions are right, rather than me asserting it.What changes
Nothing of its own. The head is
hardening/windows-buildkite-signing-verificationwithorigin/trunkmerged in, so the diff againsttrunkis what #28 actually still contributes after the merge — three files, down from five.The two that vanished did so because #27 already landed them:
scripts/azure-sign.cjsand its suite are now identical totrunk, which is the clearest evidence the conflict resolutions went the right way.The three conflicts, all resolved toward
trunk:.buildkite/pipeline.yml— the branch rannpm run build:onceas its own step beforenpm run dist:win.trunk'sdist:winnow chainsbuild:onceitself, so the step is redundant. The hardening the branch adds —Invoke-NativeCommandaround the native calls, and the extractedverify_windows_signature.ps1— is kept.scripts/azure-sign.cjs—trunklanded the same/debugpassthrough with an explanatory comment. Kept the comment.test/azure-sign.test.cjs—trunkmoved the suite totests/unit/, so therequiregains a path segment.How to test this
Platforms: any — the question is whether CI is green, and the interesting jobs are the ones that run on macOS and Windows themselves.
Starting state: this branch, with
node_modulesinstalled perAGENTS.md§ Bootstrap.unit (macos-latest),unit (windows-latest),eslint, and the four E2E jobs on this PR. All must pass.npm test— 1046 passing on macOS at the time of writing — andnpm run lint, clean..buildkite/pipeline.ymldiff againsttrunkand confirm the Windows step still fails closed:$ErrorActionPreference,Invoke-NativeCommandon both native calls, and verification delegated to the script.What must not have happened: a conflict resolved by quietly dropping one side. The renderer bundle is the one to check — if
build:onceno longer runs before packaging,dist:winproduces an artifact with a stale or missing bundle and nothing in the unit suite notices. The packaged smoke job ine2e.ymlis what would catch it.Risks and limitations
The Buildkite side is not validated at all, and not for the reason I expected. No Buildkite build reported against this PR — nor against #28, nor #392: all three have zero commit statuses and no Buildkite check runs. So the Windows signing path #28 hardens has never been exercised by CI on any pull request, and this PR cannot change that. Worth confirming separately whether the pipeline is meant to build pull requests.
Relatedly, #28 currently shows no checks whatsoever — its head predates every workflow, and GitHub does not retroactively run a workflow on an open PR when it lands on the base branch. Pushing this merge to #28's branch will trigger Actions on it for the first time.
A second commit drops the
:node: TestsBuildkite step. It was right when written —npm testran in no CI at all on 2026-07-06 — but #68 landed three weeks later and now runs the unit suite on macOS and Windows across two Node runtimes. The suite needs no secrets, which is the whole basis of #65's split: everything secretless on Actions, Buildkite only for signed artifacts. The green Actions run on this PR is the evidence for that condition.Related
Validates the
trunkmerge for #28. Shaped after #392.