ci: name every artifact per run instead of overwriting a shared name (#3314) - #3400
Merged
Merged
Conversation
…3314) `overwrite: true` was on all nine upload-artifact steps in quality.yml since #3319, on the reading that a name which may already exist needs v4's conflict policy. It does not mean "the newest run wins": it deletes the existing artifact of that name before uploading, so on a name two runs share, the name resolves to whichever run got there last and the earlier run's evidence is gone. Every upload now ends its name in run_id and run_attempt, so a name is claimed exactly once and there is nothing left to overwrite. run_attempt is needed as well as run_id because a re-run of a run keeps its run_id. The same pass found the scripts-and-compose upload never uploaded anything at all: upload-artifact v4.4+ skips hidden paths, and `path: .ci-artifacts/` is a hidden directory, so the action logged "No files were found with the provided path" and, under if-no-files-found: ignore, said so quietly. The ml-worker and auth-events-worker JUnit files that retention was added for have therefore never been retained. Naming the two files fixes that and bounds every artifact's contents to the report its own lane wrote. 7-day retention and the always()/failure() split from #3319 are unchanged, no upload was removed, and the ADVISORY set in the zizmor gate is untouched.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Xore
enabled auto-merge (squash)
September 27, 2026 11:13
Xore
added a commit
that referenced
this pull request
Sep 27, 2026
… module move broke (#3325) The rebase onto #3400 brought main's fixes in beside #3325's move of the handler modules and the route table out of `src/main.rs` and into `src/lib.rs`. Three things were red as a result. None of them is a behaviour regression; all three are places where a gate reads a file, and the move changed which file. `zizmor` reported a blocking `artipacked` on this branch's own `weekly-schemathesis.yml`: an `actions/checkout` without `persist-credentials: false`. #3388 added that to every checkout in the tree and #3400 finished the job, but this workflow was added after that work and never met it. Fixed the way every other one is -- the `with:` block, and the version comment normalised from `# v7` to the `# v7.0.1` that the other 38 checkouts in the tree spell. No rule was added to the ADVISORY allowlist; the gate now reports only the allowlisted `dangerous-triggers` finding, which is what it reported on main. `scripts/tests/test_3316_image_boot_smoke.py` asserted that each path the boot smoke probes is registered by looking for `.route("<path>"` in `src/main.rs`. Two moves broke that string, not one: the table moved to `src/lib.rs`, and then #3325's own commit replaced every `.route(path, method(handler))` with `.routes(utoipa_axum::routes!(handler))`, which takes the path off the handler's `#[utoipa::path]`. So there is no path literal in the table at all any more. It now reads the paths declared by the annotations, which is where they live, and keeps the literal form so a table written in plain axum is still checked rather than passing on an empty scan. The scan is guarded on `assertGreaterEqual(len(declared), 4)`, and both patterns require a leading `/`, because `openapi.rs`'s own `BYPASSING_ROUTES` holds the literal strings `".route("` and friends and a looser pattern reads that Rust string as a route registration. `scripts/tests/test_3315_image_revision.py` pinned the Rust normalizer to the shared revision corpus by asserting on `src/main.rs` too, and `normalize_revision`, `REVISION_UNKNOWN` and the test module that reads the corpus all moved to `src/lib.rs`. Repointed at the file they moved to. It reads one named file rather than scanning the tree: an assertion against all of `src/` dumps every module into the failure message, and if the next move relocates these the assertion should fail with the file it looked in. Both test repoints are verified to still fail for the right reason -- renaming the `/livez` annotation, dropping the `/readyz` annotation, and pointing the corpus `include_str!` at a PNG each fail with the intended message, so the gates are not weakened into passing.
Xore
added a commit
that referenced
this pull request
Sep 27, 2026
… it weekly (#3325) (#3382) * feat(backend-service): publish the OpenAPI contract for /api and fuzz it weekly (#3325) Adds a machine-readable OpenAPI 3.1 contract for backend-service's /api surface -- 130 registered paths, 139 operations -- and the two gates that keep it from drifting, plus the weekly fuzz job the issue asked for. The contract is hand-maintained in a new src/openapi.rs rather than derived with utoipa. Almost every handler returns Json<Value>, and the property the issue wants pinned lives in the route table and the extractor signatures, not the response bodies, so a #[utoipa::path] on 138 handlers would learn only that GET /api/v1/events takes a Query<EventsQuery> and answers 401 without a service token. Three gates, failing in different directions on purpose: - contract_covers_every_router_route reads main.rs and fails if the router and the contract disagree about which (path, method) pairs exist. A route added without a contract row is the direction that rots, since it drops a fuzz target silently. - checked_in_contract_is_current fails if the committed openapi.json is not what the module renders. - quality.yml runs the same generator as a diff -u in both backend jobs, because a test failure says "stale" while the diff says what changed. Response bodies stay free-form on purpose: 138 transcribed shapes would be 138 chances to assert something the code does not enforce. The document pins the auth tier, the request parameters (including the enums the handlers actually validate), the declared status codes, and every response's media type. weekly-schemathesis.yml runs Mondays 04:23 UTC and on workflow_dispatch, booting the service against a single-node Elasticsearch service container -- without a cluster every ES-backed route answers 502 and the run measures nothing. Only the auth gate can fail the job; both schemathesis passes are advisory, because this API validates aggressively enough that a hard gate would be red on its first run and train everyone to ignore it. That gate is scripts/check-api-auth-tier.py, not schemathesis's ignored_auth. ignored_auth skips any operation the contract declares public, so demoting a live route in the document turns the check green while the route keeps serving anonymous callers -- reproduced on this service, where marking /api/v1/events public gave a passing run and one warning. The script reads the same document for expected answers but never trusts it about which routes are secured; that direction is the Rust test, which asserts the public set is exactly /healthz and /metrics. One row does not follow the file's pattern and is worth flagging in review: GET /api/v1/webhook-delivery, added by #3373 on main during this branch's rebase, is the only ES-backed read that declares no 502. Its handler catches the cluster error and answers 200 with `available: false` and the reason in the body, so a stated empty card beats a page that fails to render. Every neighbouring diagnostics row 502s because its extractor turns a dead cluster into a status code; copying the pattern here would have been a guess the source does not support. Running the job before shipping it found eight real omissions in the first draft of the contract, all now fixed: 415 and 422 from axum's Json<T> rejection on all 25 body routes, 400 from its Query<T> rejection, four services routes answering JSON errors declared as text/plain, a text/plain export declared as JSON, 404 from the correlations and reporter-stats lookups, and 422/501 from the report renderer's own error mapping. None of them are reachable from the router's source -- nothing in main.rs mentions 415 -- so the builders now add the extractor-level statuses instead of leaving each row to remember. The passes report those two finding classes by name so a regression in the document is visible even though it cannot fail the job. Against a booted service the document is now clean: 136 secured operations refuse an anonymous caller, and the authenticated pass finds nothing against the contract itself. Refs #3325 * fix(contract): cover /livez and /readyz, and satisfy the zizmor gate (#3325) The rebase onto #3388 pulled in main's /livez and /readyz, which the contract test caught as registered-but-undocumented. Documented both, and widened the public-route invariant in the same test to name all four public routes instead of prefix-matching two. Zizmor: this branch introduces weekly-schemathesis.yml, which #3388 never saw, so it kept floating action tags and expanded a workflow_dispatch input inline into a run block. Pin both actions and route max_examples through env like every other value in that run block. * refactor(backend-service): move the handler modules and route table into the library being transcribed by hand, and that needs a shape the crate did not have: `#[utoipa::path]` annotations live on the handlers, and the second binary that prints the document cannot see a first binary's modules. So the modules move into `src/lib.rs`, which already existed for exactly this reason, and the route table moves with them so the document is generated from the same `Router` the process serves. The move is mechanical on purpose. No module, type or function is renamed or split, and the ~320 `crate::` references the handlers make resolve to the same items as before, because `AppState` and the probe handlers landed in the same place the crate root used to be. `main.rs` keeps what is genuinely a process: the environment, the #2183 boot gate, state construction, the listener. `allow_unauth_dev_from_env` becomes `pub` because `main.rs` now reaches it through the library rather than declaring it in the crate root. One test changes, and only in what it reads: `contract_covers_every_router_route` parsed `.route(...)` out of `src/main.rs`, which no longer holds the route table. It reads `src/lib.rs` now. Every assertion is unchanged -- including the `registered.len() > 100` guard that would have caught a stale path -- so the gate is exactly as strong, pointed at the file the code moved to. 544 passed, 0 failed, 1 ignored: same as before the move. `openapi.json` is byte-identical, which the checked_in_contract_is_current gate asserts. * refactor(backend-service): generate the OpenAPI contract with utoipa (#3325) Replaces the 1820-line hand-written table in src/openapi.rs with utoipa/utoipa-axum, so the contract is derived from the annotations and the route table rather than transcribed alongside them. The 128 /api paths and 4 health probes come out byte-identical, which the the_document_is_byte_stable gate asserts against the committed openapi.json. The route table is now an index, not a second copy. Every entry moved from `.route(path, method(handler))` to `.routes(utoipa_axum::routes!(handler))`, in the original order, one handler per call: `routes!` panics if two handlers in a single call share an HTTP method, and the table has methods that repeat across adjacent entries. Paths and methods now come from `#[utoipa::path]`, so they cannot disagree with the annotations the document is built from. `#[derive(OpenApi)]` is deliberately not used. It tags every operation with a module-path tag; utoipa-axum's OpenApiRouter does not, and the committed document's tag list is per-area, not per-module. `OpenApiRouter::default()` rather than `new()`, because `new()` fills info.contact and info.license from utoipa's own Cargo.toml. The `axum_extras` feature stays off so the annotations are authoritative rather than inferred from extractor types. 141 handlers annotated, `inline(...)` throughout -- no components/schemas and no $refs anywhere in the document. The two `Option<Json<T>>` bodies and the `Option<T>` parameters cannot express `pattern`/`maxLength`/`minimum`/ `maximum` or inline enums at container level through utoipa's derive, so 16 named parameter-shape types live in a new src/contract.rs behind a `contract_schema!` macro; STORE_NAMES moved there with them and is re-exported from openapi.rs for the handside allowlist test. A post-generation transform in render() reconciles the document with the committed one. Six steps, each documented at the point of implementation and counted in the module doc: merge the middleware 401, derive `operationId` and `tags` as pure functions of the path, read `Option` as an absence rather than a null (this is also what turns `x-optional-body` into `required: false`), strip the descriptions utoipa derives from doc comments, pin the document-level literals, and emit `parameters: []` on operations that take none. contract_covers_every_router_route is rewritten, not just repointed. It parsed `.route(...)` calls out of the source, and that form no longer exists. The two doors that remain open are now the two it closes: the BYPASSING_ROUTES list (`.route(`, `.route_service(`, `.nest_service(`) must not appear in src/lib.rs, because OpenApiRouter inherits all three as pass-throughs that serve a route with no OpenAPI operation; and every `#[utoipa::path]` in src/**/*.rs must be routed. Both directions were exercised rather than assumed -- injecting a bare `.route("/api/v1/sneaky")` and removing an existing `routes!` entry each fail with the intended message. One intentional line of difference in openapi.json. The serviceToken security-scheme description reads "lib.rs's require_service_token" where it used to read "main.rs's", because Stage 1 moved that middleware out of the crate root. Publishing the old string would state something untrue about where the check lives. That is the only byte that changed, in 296852. 546 passed, 0 failed, 1 ignored: the 544 baseline plus the endless-stream invariant and the new byte-stability test. `cargo build --release` succeeds (the Dockerfile copies src/ wholesale), and clippy --lib --all-targets is clean. * fix(ci): close the artipacked advisory, and repoint the two gates the module move broke (#3325) The rebase onto #3400 brought main's fixes in beside #3325's move of the handler modules and the route table out of `src/main.rs` and into `src/lib.rs`. Three things were red as a result. None of them is a behaviour regression; all three are places where a gate reads a file, and the move changed which file. `zizmor` reported a blocking `artipacked` on this branch's own `weekly-schemathesis.yml`: an `actions/checkout` without `persist-credentials: false`. #3388 added that to every checkout in the tree and #3400 finished the job, but this workflow was added after that work and never met it. Fixed the way every other one is -- the `with:` block, and the version comment normalised from `# v7` to the `# v7.0.1` that the other 38 checkouts in the tree spell. No rule was added to the ADVISORY allowlist; the gate now reports only the allowlisted `dangerous-triggers` finding, which is what it reported on main. `scripts/tests/test_3316_image_boot_smoke.py` asserted that each path the boot smoke probes is registered by looking for `.route("<path>"` in `src/main.rs`. Two moves broke that string, not one: the table moved to `src/lib.rs`, and then #3325's own commit replaced every `.route(path, method(handler))` with `.routes(utoipa_axum::routes!(handler))`, which takes the path off the handler's `#[utoipa::path]`. So there is no path literal in the table at all any more. It now reads the paths declared by the annotations, which is where they live, and keeps the literal form so a table written in plain axum is still checked rather than passing on an empty scan. The scan is guarded on `assertGreaterEqual(len(declared), 4)`, and both patterns require a leading `/`, because `openapi.rs`'s own `BYPASSING_ROUTES` holds the literal strings `".route("` and friends and a looser pattern reads that Rust string as a route registration. `scripts/tests/test_3315_image_revision.py` pinned the Rust normalizer to the shared revision corpus by asserting on `src/main.rs` too, and `normalize_revision`, `REVISION_UNKNOWN` and the test module that reads the corpus all moved to `src/lib.rs`. Repointed at the file they moved to. It reads one named file rather than scanning the tree: an assertion against all of `src/` dumps every module into the failure message, and if the next move relocates these the assertion should fail with the file it looked in. Both test repoints are verified to still fail for the right reason -- renaming the `/livez` annotation, dropping the `/readyz` annotation, and pointing the corpus `include_str!` at a PNG each fail with the intended message, so the gates are not weakened into passing. * docs(CI-CD): point the #3325 section at where the contract actually lives (#3325) The rebase's own conflict resolution made these three claims false, and the file is one this branch already owns, so they are corrected here rather than left for the next reader to trip over. "the source of truth is the operation table in src/openapi.rs" was true when the table was hand-written there and stopped being true when it was generated from the `#[utoipa::path]` annotations and the route table in src/lib.rs. "contract_covers_every_router_route reads src/main.rs" named a file that no longer holds a route table. "Fix those by editing the row in operations()" named a function that no longer exists -- the edit point is the annotation on the handler, or the shape it names in src/contract.rs. The counts were stale in the same way: the section claimed 129 registered /api paths and 138 operations, and the committed document has 132 paths and 141 operations -- 128 /api paths and 137 /api operations behind the token, plus the four public probes. Now stated as counted, with the public set named. No behaviour and no generated contract changes: `openapi.json` is byte-identical before and after, which `the_document_is_byte_stable` and `checked_in_contract_is_current` both assert.
Xore
added a commit
that referenced
this pull request
Sep 27, 2026
…overy guard (#3318) frontend-next ran vitest and Playwright in quality.yml and collected no coverage, so there was no baseline: a change could delete tested behaviour and CI stayed green, because the only thing that noticed was a reviewer noticing the test that asserted it was gone. Three things, all measured rather than chosen. 1. Measure it. @vitest/coverage-v8 (matching the vitest 4.1.11 the lockfile already resolves) and a `test:coverage` script. Line and branch coverage over src/ only -- not the tests, not the config, not the generated route tree -- which is 127 files and is the only thing a coverage number should be about. `npm test` is untouched and stays uninstrumented, so deploy.yml, the README and a developer's loop keep the cheap command. The report is uploaded from the EXISTING frontend-next job, per-(run, attempt) named like every other artifact since #3400, and mirrored into its GitHub-hosted twin because the twins are byte-identical by convention and a ratchet that only ran on the self-hosted executor would skip every degraded day. 2. Record the baseline, and hold it. coverage-baseline.json holds what the suite actually measures today: 806/7359 lines (10.95%) and 346/7250 branches (4.77%), taken on the node:22 image this job runs. That is low because the tests concentrate on src/lib server logic and most of src/routes and src/components is held by the Playwright matrix, which the v8 provider cannot see. It is recorded as measured. No threshold was imported and no number was picked to look respectable -- a 60% baseline would have been red on arrival and would have taught the gate nothing. The ratchet has two independent gates. tolerance.coveredCount is 0, so no covered line or branch may stop being covered: that is what catches a change that deletes tested behaviour, and no shrinking-denominator trick can satisfy it. tolerance.pctPoints is 1.0, which catches a change that adds a lot of untested source. At an ~11% baseline the second is coarse -- it needs several hundred new uncovered lines -- and the first is what carries the weight. Updating the baseline is `npm run coverage:baseline` and a visible diff, never an autoUpdate, so a floor cannot rewrite itself. Proved, not asserted: with one real test file temporarily moved out of the include globs, coverage fell 23 covered lines and 37 covered branches and the ratchet exited 1. That drop is 0.31pp of lines, inside the 1.0pp tolerance -- so the percentage gate alone would have passed it, which is the reason the covered-count gate exists. The file was restored and `git diff HEAD` on it is empty. 3. Test-discovery guard. A test-shaped file that no runner collects is worse than no test, because the file is the evidence. The guard asks the real runners -- `vitest list --filesOnly` and `playwright test --list` -- what they collect and fails on the difference, rather than reimplementing their globs and agreeing with them right up until it does not. Both runners are asked because two own .ts/.spec.ts here: asking only vitest would flag e2e/dashboard.spec.ts, and the fix would be a suppression list. Proved with a canary .spec.ts under src/ -- a shape the vitest include globs do not name -- which failed the guard and named the file, then was removed; the guard is green again. One thing that needed handling: Playwright's collection imports every file its testMatch reaches, including e2e/fake-backend.test.mjs, and importing that node:test module writes a TAP banner to stdout ahead of the JSON. The report is read from a file via PLAYWRIGHT_JSON_OUTPUT_NAME so nothing can interleave with it. Both proofs are pinned as cases in scripts/tests/test_3318_coverage_ratchet.py (17 tests, auto-discovered by the existing scripts/tests lane), so a later PR cannot widen a tolerance or drop a CI step without that suite going red. Not done, deliberately: e2e/fake-backend.test.mjs is a node:test file that no configured runner collects either, so the guard's own contract has a pre-existing exception. Wiring it up is a different runner in a different tier and out of scope here, so the guard is .ts/.tsx only rather than a new gate that is red on day one. zizmor reports the same single already-allowlisted advisory as before this change and nothing new; no new action was added, and the one used is the SHA already pinned in this file. actionlint clean.
Xore
added a commit
that referenced
this pull request
Sep 27, 2026
…overy guard (#3318) frontend-next ran vitest and Playwright in quality.yml and collected no coverage, so there was no baseline: a change could delete tested behaviour and CI stayed green, because the only thing that noticed was a reviewer noticing the test that asserted it was gone. Three things, all measured rather than chosen. 1. Measure it. @vitest/coverage-v8 (matching the vitest 4.1.11 the lockfile already resolves) and a `test:coverage` script. Line and branch coverage over src/ only -- not the tests, not the config, not the generated route tree -- which is 127 files and is the only thing a coverage number should be about. `npm test` is untouched and stays uninstrumented, so deploy.yml, the README and a developer's loop keep the cheap command. The report is uploaded from the EXISTING frontend-next job, per-(run, attempt) named like every other artifact since #3400, and mirrored into its GitHub-hosted twin because the twins are byte-identical by convention and a ratchet that only ran on the self-hosted executor would skip every degraded day. 2. Record the baseline, and hold it. coverage-baseline.json holds what the suite actually measures today: 806/7359 lines (10.95%) and 346/7250 branches (4.77%), taken on the node:22 image this job runs. That is low because the tests concentrate on src/lib server logic and most of src/routes and src/components is held by the Playwright matrix, which the v8 provider cannot see. It is recorded as measured. No threshold was imported and no number was picked to look respectable -- a 60% baseline would have been red on arrival and would have taught the gate nothing. The ratchet has two independent gates. tolerance.coveredCount is 0, so no covered line or branch may stop being covered: that is what catches a change that deletes tested behaviour, and no shrinking-denominator trick can satisfy it. tolerance.pctPoints is 1.0, which catches a change that adds a lot of untested source. At an ~11% baseline the second is coarse -- it needs several hundred new uncovered lines -- and the first is what carries the weight. Updating the baseline is `npm run coverage:baseline` and a visible diff, never an autoUpdate, so a floor cannot rewrite itself. Proved, not asserted: with one real test file temporarily moved out of the include globs, coverage fell 23 covered lines and 37 covered branches and the ratchet exited 1. That drop is 0.31pp of lines, inside the 1.0pp tolerance -- so the percentage gate alone would have passed it, which is the reason the covered-count gate exists. The file was restored and `git diff HEAD` on it is empty. 3. Test-discovery guard. A test-shaped file that no runner collects is worse than no test, because the file is the evidence. The guard asks the real runners -- `vitest list --filesOnly` and `playwright test --list` -- what they collect and fails on the difference, rather than reimplementing their globs and agreeing with them right up until it does not. Both runners are asked because two own .ts/.spec.ts here: asking only vitest would flag e2e/dashboard.spec.ts, and the fix would be a suppression list. Proved with a canary .spec.ts under src/ -- a shape the vitest include globs do not name -- which failed the guard and named the file, then was removed; the guard is green again. One thing that needed handling: Playwright's collection imports every file its testMatch reaches, including e2e/fake-backend.test.mjs, and importing that node:test module writes a TAP banner to stdout ahead of the JSON. The report is read from a file via PLAYWRIGHT_JSON_OUTPUT_NAME so nothing can interleave with it. Both proofs are pinned as cases in scripts/tests/test_3318_coverage_ratchet.py (17 tests, auto-discovered by the existing scripts/tests lane), so a later PR cannot widen a tolerance or drop a CI step without that suite going red. Not done, deliberately: e2e/fake-backend.test.mjs is a node:test file that no configured runner collects either, so the guard's own contract has a pre-existing exception. Wiring it up is a different runner in a different tier and out of scope here, so the guard is .ts/.tsx only rather than a new gate that is red on day one. zizmor reports the same single already-allowlisted advisory as before this change and nothing new; no new action was added, and the one used is the SHA already pinned in this file. actionlint clean.
Xore
added a commit
that referenced
this pull request
Sep 27, 2026
) (#3417) * ci(frontend-next): give the tier a measured coverage floor and a discovery guard (#3318) frontend-next ran vitest and Playwright in quality.yml and collected no coverage, so there was no baseline: a change could delete tested behaviour and CI stayed green, because the only thing that noticed was a reviewer noticing the test that asserted it was gone. Three things, all measured rather than chosen. 1. Measure it. @vitest/coverage-v8 (matching the vitest 4.1.11 the lockfile already resolves) and a `test:coverage` script. Line and branch coverage over src/ only -- not the tests, not the config, not the generated route tree -- which is 127 files and is the only thing a coverage number should be about. `npm test` is untouched and stays uninstrumented, so deploy.yml, the README and a developer's loop keep the cheap command. The report is uploaded from the EXISTING frontend-next job, per-(run, attempt) named like every other artifact since #3400, and mirrored into its GitHub-hosted twin because the twins are byte-identical by convention and a ratchet that only ran on the self-hosted executor would skip every degraded day. 2. Record the baseline, and hold it. coverage-baseline.json holds what the suite actually measures today: 806/7359 lines (10.95%) and 346/7250 branches (4.77%), taken on the node:22 image this job runs. That is low because the tests concentrate on src/lib server logic and most of src/routes and src/components is held by the Playwright matrix, which the v8 provider cannot see. It is recorded as measured. No threshold was imported and no number was picked to look respectable -- a 60% baseline would have been red on arrival and would have taught the gate nothing. The ratchet has two independent gates. tolerance.coveredCount is 0, so no covered line or branch may stop being covered: that is what catches a change that deletes tested behaviour, and no shrinking-denominator trick can satisfy it. tolerance.pctPoints is 1.0, which catches a change that adds a lot of untested source. At an ~11% baseline the second is coarse -- it needs several hundred new uncovered lines -- and the first is what carries the weight. Updating the baseline is `npm run coverage:baseline` and a visible diff, never an autoUpdate, so a floor cannot rewrite itself. Proved, not asserted: with one real test file temporarily moved out of the include globs, coverage fell 23 covered lines and 37 covered branches and the ratchet exited 1. That drop is 0.31pp of lines, inside the 1.0pp tolerance -- so the percentage gate alone would have passed it, which is the reason the covered-count gate exists. The file was restored and `git diff HEAD` on it is empty. 3. Test-discovery guard. A test-shaped file that no runner collects is worse than no test, because the file is the evidence. The guard asks the real runners -- `vitest list --filesOnly` and `playwright test --list` -- what they collect and fails on the difference, rather than reimplementing their globs and agreeing with them right up until it does not. Both runners are asked because two own .ts/.spec.ts here: asking only vitest would flag e2e/dashboard.spec.ts, and the fix would be a suppression list. Proved with a canary .spec.ts under src/ -- a shape the vitest include globs do not name -- which failed the guard and named the file, then was removed; the guard is green again. One thing that needed handling: Playwright's collection imports every file its testMatch reaches, including e2e/fake-backend.test.mjs, and importing that node:test module writes a TAP banner to stdout ahead of the JSON. The report is read from a file via PLAYWRIGHT_JSON_OUTPUT_NAME so nothing can interleave with it. Both proofs are pinned as cases in scripts/tests/test_3318_coverage_ratchet.py (17 tests, auto-discovered by the existing scripts/tests lane), so a later PR cannot widen a tolerance or drop a CI step without that suite going red. Not done, deliberately: e2e/fake-backend.test.mjs is a node:test file that no configured runner collects either, so the guard's own contract has a pre-existing exception. Wiring it up is a different runner in a different tier and out of scope here, so the guard is .ts/.tsx only rather than a new gate that is red on day one. zizmor reports the same single already-allowlisted advisory as before this change and nothing new; no new action was added, and the one used is the SHA already pinned in this file. actionlint clean. * ci(frontend-next): track the vitest 5 major in the coverage plugin The coverage ratchet pinned @vitest/coverage-v8 to ^4.1.11 because vitest was 4.1.11 when it was written. #3100 moved vitest to 5 on main while this branch was queued, and the peer dependency is major-matched: vitest 4 declares peerOptional @vitest/coverage-v8@4.1.11. Resolving the conflict by keeping the old pin would install a mismatched pair that fails at test time rather than at install time. Bump the plugin to ^5.0.1 so the pair stays same-major, and regenerate the lockfile so its root entry matches the manifest again. * fix(frontend-next): regenerate the lockfile with the npm that builds the image `npm ci` in the dashboard-next container failed with EUSAGE, "Missing: ioredis@5.11.1 from lock file" (and @ioredis/commands, redis-parser). The Dockerfile pins node:22-alpine@sha256:c610fcd, which ships npm 10.9.8, but the lockfile had been regenerated with npm 11. Both write lockfileVersion 3, so the version number hid it: npm 11 hoists ioredis under node_modules/nitro/node_modules/ and omits the top-level entries npm 10's `npm ci` requires. Regenerated with the npm inside the pinned image rather than on a workstation, which is the only place the two resolvers agree. npm ci in that image now installs 420 packages and exits 0; package.json engines, dependencies and devDependencies still match the lockfile root entry.
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
1 participant
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.
Summary
The
Workflow security audit (zizmor, #3314)gate'sartipackedclass wasaddressed on
main: all nineoverwrite: trueflags are gone from.github/workflows/quality.yml, and the same pass found that one of thoseuploads was silently uploading nothing at all.
One correction to the reported finding, verified against the pinned
auditor. The gate runs zizmor v1.30.1 (
quality.ymlpins the version andits SHA256), and that build's
artipackedaudit never inspectsoverwrite. Itssource (
crates/zizmor/src/audit/artipacked.rs) fires on two things only: anactions/checkoutstep that does not setpersist-credentials: false, and anactions/upload-artifactstep whosepathis.,./,..,../, or anexpression containing
github.workspace. Reproduced on this tree with the samebinary the gate uses:
That is byte-identical before and after this PR:
mainwas not red on thisrule, and no
artipackedfinding is reported at any severity, persona orseverity floor (checked with
--persona auditor --min-severity lowand--persona pedantic).The underlying weakness is real and is fixed here anyway.
overwrite: truedoesnot mean "the newest run's results replace the old ones": it deletes the
existing artifact of that name before uploading the new one, so on a name two
runs share, the name resolves to whichever run got there last and the earlier
run's evidence is destroyed. Nine steps carried it.
The fix
Every upload's
namenow ends in-<github.run_id>-<github.run_attempt>and noupload sets
overwrite:run_idalone would not be enough — a GitHub re-run of a run keeps itsrun_id, so the second attempt would still collide, which is the collisionoverwritewas really covering for. With both, a name is unique per(run, attempt), and each writing step runs once per(run, attempt): thepair is what makes upload-artifact v4's immutability mean anything here.
opposite answer from
ci-target, so no two steps in a run claim the same name(verified against each pair's
if:).actions/download-artifactanywhere in
.github/.The second, larger bug: one upload had never uploaded anything
path: .ci-artifacts/on thescripts-and-composematrix job uploaded zerofiles on every run since #3319.
upload-artifactv4.4+ excludes hidden pathsby default, and a directory whose own name starts with
.is hidden, so theaction reported "No files were found" and — under
if-no-files-found: ignore—said so quietly. From the run log of the green push run
36311141257(
ml-worker anomaly pipeline contracts, job108597191294):The
auth-events-workerrow (10 passed) lost its JUnit file the same way. Thetwo files this retention was added for have never been retained; the run's
artifact list carries only the two uploads that named their file explicitly.
Naming the two files fixes the loss and, as a side effect, bounds every
artifact's contents to the report its own lane wrote.
Final path lists
path:frontend-next/-cloudfrontend-next-unit-junit-<run_id>-<run_attempt>.ci-artifacts/frontend-next-unit-junit.xmlfrontend-next-browser/-cloudfrontend-next-browser-junit-….ci-artifacts/playwright-junit.xmlfrontend-next-browser/-cloudfrontend-next-browser-report-……/frontend-next/playwright-report/…/frontend-next/test-results/outputDirand thehtmlreporter's output dir, created and owned by this lane in a fresh checkoutbackend-service/-cloudbackend-service-cargo-test-log-….ci-artifacts/cargo-test.logcargo testlog this job tees itselfscripts-and-composescripts-<matrix.name>-results-<run_id>-<run_attempt>.ci-artifacts/ml-worker-junit.xml.ci-artifacts/auth-events-worker-junit.xml--junitxmlflags write; every other row writes no report and uploads nothing (unchangedif-no-files-found: ignore)Issues
Refs #3314 (the audit gate), Refs #3319 (the evidence-retention work whose
behaviour is preserved and, in the matrix job, repaired).
Security impact
.envfiles were added.No
artipackedentry was added to the gate'sADVISORYset, no upload wasremoved, no retention was shortened, and no
always()/failure()conditionwas changed.
Validation
The Rust lane is a no-diff lane: this PR touches no
.rs, noCargo.*and noRust-adjacent file. Its suite was run green on the unmodified tree for that
proof — one test target, 541 tests collected (540 passed, 1 ignored:
report_pdf::tests::scratch_dump_multi_page_pdf, a manual-inspection scratchdump). Note the brief's figure of 544 does not match the crate as it stands on
main; 541 is whatcargo testcollects today.Not validated: no workflow run against GitHub's artifact service — the
upload behaviour change is argued from the action's documented hidden-path
default plus the run log quoted above, not from a fresh CI run. The same
scripts/tests row has 2 pre-existing local failures in
test_compose_drift_watch_sweep.py(privileged-fallback findings), identicalbefore and after this change (confirmed with the change stashed) and unrelated
to it.
Rollout
None.
Left standing, deliberately
frontend-testing-pilot.yml'sfrontend-mutation-reportupload still setsoverwrite: true(14-day retention, nightly). Same defect, different file anddifferent lane — out of scope here, and it deserves its own change.
scripts-and-composerow names contain a/, which GitHub rejects in anartifact name. They are harmless today because they write no report and
upload-artifact only reaches the name once it has files to upload; a row that
both wrote a report and had a
/in its name would fail its upload. Noted inthe workflow rather than papered over, since there is no expression-level way
to sanitise a free-text field.