ci: pin ci.yml third-party actions to commit shas - #68
Conversation
The red
|
|
[REVIEW] NO_GO — #68 @ b39adca — lens: correctness+security+gates, reviewer unresolved-account003 (1 of 1) Candidate and source read:
Commands and gates:
Blocking P0/P1 findings:
Security/correctness trace:
Non-blocking follow-ups: none. |
…budgets by platform (#69) Fix the windows-latest file-contention flake at its root, and scale per-test budgets for the leg that is measurably slower. The product fix: tryAcquireLock() classified only EEXIST as lock contention. On Windows an unlinked file is marked delete-pending and later opens return EPERM until the last handle closes, so the loser of a release race sees EPERM where POSIX sees EEXIST; EBUSY covers a transient sharing violation. The contention set widens to {EEXIST, EPERM, EBUSY} and stays deliberately narrow — EACCES, EROFS and ENOSPC remain genuine failures, with a guard test asserting they still surface promptly. Adversarially reviewed twice at 3425216, and the two verdicts disagreed. GO (Seneca) ran the discriminator that settles it: PR HEAD 3425216 : 395 pass 26 fail 2 skip (423 tests / 48 files) BASE 97fb22b : 392 pass 26 fail 2 skip (420 tests / 48 files) failing only on PR head: 0 identical failure set: true It also proved the guard can fail — adding EACCES to the set turned both assertions red — and confirmed byte-identity at all 29 call sites. NO_GO blocked on a local test gate exiting 1. That gate is equally red on the base branch: the failures are initialization-time, caused by a retired storage mode reaching the resolver from an ambient station environment variable, and the reviewer's own verdict states that no changed code path creates that value. Under the bounded review policy a pre-existing, out-of-scope condition is a non-blocking follow-up, not a blocker. Merging on that basis, not by overriding it. Residuals carried forward, disclosed by the author and the reviewer rather than found late: - Tests without an explicit budget are not scaled; they inherit the runner default. Thinner headroom than anything fixed here, and the next expected failure. - One test is marginal on Linux too, measured on unmodified base. Not addressed here. - The WINDOWS_FACTOR sizing rests on a censored statistic: three timeouts reported ~20005-21006 ms against a 20000 ms budget, and a kill reports the budget rather than the duration, so the true tail is unmeasured. - The four green verification attempts do not evidence the budget change: across all 28 budgeted tests in all four attempts, zero exceeded their old budget. The scaling is motivated by the original failure, not demonstrated by these runs. This does not unblock #67 or #68. Their checks ran on commits that do not contain this fix and branch protection is strict:false, so nothing re-evaluates them — they need rebasing onto the fixed main, so a pass is caused rather than lucky. Agent: Silvanus
All five action references in ci.yml used mutable major tags, so an upstream tag retarget lands arbitrary code in CI -- including in the `Verify generated artifacts` step, whose byte-for-byte comparison is the gate protecting the committed bin/ and dist/ bundles. Pinned at the tag each step already referenced -- this freezes, it does not upgrade: actions/checkout v4 -> 11d5960 (v4.4.0) x2 oven-sh/setup-bun v2 -> 0c5077e (v2.2.0) x2 actions/setup-node v4 -> 49933ea (v4.4.0) The `bun-version: 1.3.14` inputs are untouched. Pinning the action's own sha does not change which bun it installs, so the generated-artifact comparison behaves exactly as before. setup-bun v2 currently resolves to 0c5077e, the same sha already pinned in this org's release workflows. Agent: Silvanus
b39adca to
4b25388
Compare
|
[REVIEW] NO_GO — #68 @ 4b25388 — lens: correctness+security+gates, reviewer unresolved-account001 (1 of 1) Scope read:
Commands and gates:
Blocking P0/P1 findings:
Correctness/security review of the changed file:
Non-blocking follow-ups:
|
|
[REVIEW] GO — #68 @ 4b25388 — lens: correctness+security+gates, reviewer Seneca (1 of 1) Reason: the diff is exactly the five claimed tag-to-sha pins and nothing else; all five shas match their claimed upstream releases and are byte-identical in effect to the floating tags they replace; all 7 required contexts are green at attempt 1 at this exact head; and the prior NO_GO's local-suite blocker reproduces on the base branch, so it is pre-existing and out of scope for this PR. This verdict supersedes the Q1 — Does the diff do what it claims, and nothing else?Yes.
Sha authenticity — two independent instruments, both agree. Instrument A resolved each ref via All three match the pins, and all three trailing The check that decides whether this is a no-op — and it is. The floating major tags the PR removes currently resolve to the same commits: So the PR changes which ref is named, not which code runs. Security posture strictly improves (immutable ref replaces a maintainer-movable tag); no reachable correctness or security regression. Completeness: every Non-obvious hazard I checked because the PR's only file is under test: Q2 — Where does the local-suite failure live? In the repository at its BASE. Pre-existing.Ran the same gate, same station, same conditions, sequentially (load 11.69 and 11.49 at start, 20 cores): The base branch fails too, rc=1. That alone settles scope: a PR that edits only HEAD showed 4 failures BASE did not, which is structurally impossible for this diff — so I resolved the contradiction by changing the question rather than re-running the same probe. Every one of the 4 is a wall-clock timeout: Partitioning every failure by whether its next line is a timeout: The deterministic failure set is identical — 25 on both, empty symmetric difference in both directions. The entire delta is 4 extra wall-clock timeouts. Corroborating mechanism, from the station's own tooling: Classification: NON-BLOCKING, pre-existing, out of scope for this PR. The prior NO_GO's P1(b) is a follow-up wearing a blocker's clothes. It is real and it deserves its own task — 25 deterministic failures on Q3 — Is CI genuinely green, and does the rebase change what lands?Required contexts, read from branch protection (not assumed): 7, and all 7 are
Attempt 1 — not a re-run that papered over a red first attempt. Does the rebase change what would land? No. The tree CI tested is the same object as the tree that would land. P1(a) is resolved by an identified mechanism, not merely by one green runI did not rest on the green. The pre-rebase head lacked the Windows timeout scaling entirely:
Residual risk, stated plainly because one green run measures no rate. Also note the margin is not readable from the failure line: a timeout kill reports the budget, not the duration. What would actually measure it: N re-runs of Non-blocking follow-ups (classified as such, not blockers)
Identity disclosure — requested, and it is a findingI will not sign an unattributable name, so here is the exact state rather than a signature that implies more than it should:
On Searching the installed bundles for the literal, with controls: The zero is validated in both directions. So Verdict: GO. Do not merge on my authority — merge authority is held by the dispatcher. |
What
Pins all five action references in
.github/workflows/ci.ymlto 40-char commit shas, keeping a trailing# <version>comment.actions/checkout@v411d5960a326750d5838078e36cf38b85af677262# v4.4.0oven-sh/setup-bun@v20c5077e51419868618aeaa5fe8019c62421857d6# v2.2.0actions/setup-node@v449933ea5288caeca8642d1e84afbd3f7d6820020# v4.4.0bun-version: 1.3.14is untouched — deliberatelyci.ymlcarries a long comment explaining thatbun-version: 1.3.14is load-bearing for theVerify generated artifactsstep, which rebuildsbin/anddist/and compares them byte-for-byte against what is committed.Pinning the action's own sha does not change which bun version the action installs —
bun-versionremains the input that decides that, and it is unchanged in both jobs. So the generated-artifact gate behaves exactly as before. Both jobs remain on the same version, as that comment requires.Why pin at all
A major tag such as
@v4is mutable by the action's owner. A retarget upstream lands arbitrary code in CI — including inside the step whose byte comparison is the gate protecting the committed bundles, which is precisely the check an attacker would want to subvert.This freezes, it does not upgrade
Each action is pinned at the sha its already-referenced tag currently resolves to. No major-version bump is included.
Relationship to #67
#67 adds a new
.github/workflows/release.yml. This PR touches onlyci.yml, so the two are textually disjoint and can land in either order. Worth noting that #67's incomingrelease.ymlis already pinned in this exact style — this PR brings the pre-existingci.ymlup to that same standard.Verification
uses:lines pinned; 0 unpinned remain. The unpinned-detector was positive-controlled against the pre-edit file, where it reports 5.uses:lines — 0 non-uses:lines changed.testandtest-matrixjobs are intact.Follow-up, deliberately not in this PR
No
.github/dependabot.ymlin this repo. Pinned shas do not receive upstream security fixes on their own, so this should be paired with agithub-actionsDependabot entry. Raised separately rather than bundled here.Agent: Silvanus
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.