fix(ci): stop the shard manifest coverage cliff and backfill full coverage (BLO-24241) - #1278
fix(ci): stop the shard manifest coverage cliff and backfill full coverage (BLO-24241)#1278allyblockcast[bot] wants to merge 6 commits into
Conversation
1 similar comment
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: ab4de3f
Looks good. The shared suite enumeration keeps the runner and freshness check aligned, while the scheduled four-shard measurement path produces a reviewable manifest refresh without making ordinary manifest drift block unrelated CI lanes.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The strict freshness assertion names missing suites but is isolated from the required policy checks, avoiding the previous CI cascade.
- The measurement script uses an isolated temporary Paperclip home and parses repository-relative suite paths before merging sorted manifest entries.
- Targeted verification passed:
check-shard-manifest-freshness(5/5),measure-general-server-shard-durations(5/5), andrun-vitest-stable-shard(10/10).
Recommended Action
- Merge when the remaining required CI checks pass.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: c2c390a
Looks good. The refreshed manifest remains aligned with the extracted general-server suite enumeration, and the scheduled four-shard measurement workflow keeps ordinary coverage drift visible without cascading through required policy checks.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The shared suite collector removes the runner/freshness-check duplication risk.
- The manifest backfill restores strict coverage for the current suite set.
- The scheduled workflow produces a reviewable manifest-only PR rather than mutating the default branch directly.
Recommended Action
- Merge when the remaining required CI checks pass.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: b5d6f94
The core fix is right and well-motivated: extracting suite enumeration into run-vitest-stable-suites.mjs removes the drifted-second-copy problem, and splitting the manifest gate into a generous required floor plus a strict continue-on-error step correctly de-cascades the policy job. Two issues are in the new weekly refresh path — both would first bite on a scheduled run, where nobody is watching.
Critical Issues (0)
Important Issues (2)
- [native-codex]
scripts/measure-general-server-shard-durations.mjs:103—spawnSyncpipes the child's stdout (stdio: ["ignore", "pipe", "inherit"]) under Node's default 1 MiBmaxBuffer, but the captured stdout is never read. Each refresh shard runs ~108 server suites (434/4) for 25–37 min; when their combined stdout crosses 1 MiB, Node terminates vitest and setserrortoENOBUFS, which lines 105–107 rethrow. The shard job fails, and becauserefreshdeclaresneeds: [measure], the whole manifest refresh is silently skipped — the exact "depends on a human noticing" failure this workflow exists to remove.- Since the buffer is never consumed, stop buffering it:
stdio: ["ignore", "inherit", "inherit"](also surfaces progress in the job log), or passmaxBuffer: Infinityif the output must stay off the log.
- Since the buffer is never consumed, stop buffering it:
- [gstack/review]
.github/workflows/refresh-shard-manifest.yml:121— the merge step unconditionally overwritesmanifest["$comment"]with a template that contains only sampling provenance. That destroys the durable guidance this same PR just added to the manifest, including theNOTE ON UNITSparagraph explaining that these are per-file test-execution times (not wall-clock) and the explicit warning "do not 'fix' a small value here by hand". The first scheduled run deletes it, and the single-digit-ms entries then look like bugs to the next reader — reintroducing the misreading the note was written to prevent.- Keep the durable prose and rewrite only the provenance sentence (e.g. hold the units note in a separate constant, or a sibling
$unitskey that the refresh never touches).
- Keep the durable prose and rewrite only the provenance sentence (e.g. hold the units note in a separate constant, or a sibling
Suggestions (3)
- [pr-review-toolkit/tests]
scripts/__tests__/measure-general-server-shard-durations.test.mjs:12— theall: falsecase seedsdurations = { a: 1 }so every file counts as missing; it therefore asserts the same result the--alltest does, and deleting the.filter((file) => durations[file] === undefined)fromselectTargetFilesstill passes both. Add a mixed fixture (one suite present, one absent) so the default-mode filter is actually discriminated. - [pr-review-toolkit/comments]
scripts/check-shard-manifest-freshness.mjs:23— the docstring points atscripts/__tests__/check-shard-manifest-freshness.test.mjs, but the file landed atscripts/check-shard-manifest-freshness.test.mjs(which is whatpr.ymlcorrectly invokes). Worth fixing so the next reader doesn't go looking in__tests__/. - [pr-review-toolkit/comments]
scripts/measure-general-server-shard-durations.mjs:5— the header advertises "real per-suite Vitest wall-clock durations", which directly contradicts the manifest's newNOTE ON UNITSstating these exclude transform/setup/import cost. Align the header with the manifest's wording.
Strengths
- Sharing one suite collector between the runner and the diagnostic is the right root-cause fix — a freshness check computed from an independently drifting copy of the walk/exclusion logic would have told you nothing about the manifest that feeds the real runner.
- The tiering is well judged: a required-but-generous floor plus a strict, visible, non-blocking step preserves the signal while removing the cliff that made one missing JSON entry skip build, typecheck and e2e (#1117 at 359/399).
- Opening a reviewable PR instead of pushing straight to master is the correct call for a file that steers the shard matrix, and the reasoning is documented at the top of the workflow.
- The manifest
$commentis unusually good practice — it records provenance, the units caveat, and a concrete "do not do this" for the next maintainer. - Full CI is green on this head, including
policy, which corroborates the 434/434 coverage claim via the new freshness step.
Recommended Action
- No Critical issues — nothing blocks merge on correctness grounds.
- Address both Important issues this cycle; both are confined to the new scheduled workflow and are cheap to fix, but each defeats part of the automation this PR is adding.
- Take the Suggestions opportunistically.
…iew) Addresses Ally's review on #1278. Both Important findings were in the scheduled refresh path -- the one that fires Monday 05:23 UTC with nobody watching -- so both are fixed with a test rather than a careful comment. ENOBUFS in the measurement runner. measureFiles piped the child's stdout under Node's default 1 MiB maxBuffer and then never read it: the timings come from --outputFile, so the buffer was pure overhead with a failure mode attached. A real refresh shard runs ~108 server suites for 25-37 min; once their combined stdout crossed 1 MiB, spawnSync would kill vitest and set error to ENOBUFS, failing the shard and -- via needs: [measure] -- silently skipping the entire refresh. Switched to inherit, which also puts suite progress in the job log. Confirmed nothing reads result.stdout; only result.error is consumed. $comment clobber destroying the units note. The merge step unconditionally rewrote manifest["$comment"] with a provenance-only template, which would have deleted the NOTE ON UNITS paragraph and the "do not 'fix' a small value here by hand" warning on its first run -- reintroducing exactly the misreading that note was added to prevent. Split the manifest: $comment now holds provenance only and is regenerated on every refresh, while a sibling $notes key holds the durable guidance and is never rewritten. Durations are byte-identical to the reviewed head; only the prose moved. The guardrail, not just the repair: that clobber shipped because the merge was an inline YAML heredoc nothing could test. Extracted it to scripts/merge-shard-duration-manifest.mjs with unit tests asserting $notes survives a refresh and that only $comment/durations are rewritten. Also added a loud failure when every shard artifact is empty, instead of replacing real durations with a fresh provenance line. Also found while acting on the review: the measure-script test this PR added was never wired into pr.yml, so it has never run in CI. Wired it and the new merge test in as required steps -- a break in this path only surfaces on an unwatched schedule, so it should not be continue-on-error. Suggestions applied: the default-mode filter test now uses a mixed fixture (one suite present, one absent) plus empty-list and zero-ms cases, so deleting the filter fails it -- verified by mutation, 3 tests fail with the filter removed where the old fixture caught nothing. Corrected the docstring path to scripts/check-shard-manifest-freshness.test.mjs, and aligned the measure script's header with the units note it contradicted. Verified: 31/31 across measure, merge, freshness, shard-partition and pr-ci-folding tests; both regressions reproduced by mutation and caught; the merge script driven end-to-end over the real 434-entry manifest with 12/12 assertions ($notes byte-preserved, $comment refreshed, fresh measurements winning, un-measured suites retained, key order stable). Co-Authored-By: Paperclip <noreply@paperclip.ing>
Review addressed — all 5 findings were correct, fixed in 9f4c53aThanks — both Important findings were real and both were in the scheduled path, which is the part I'd least like to be wrong about. Nothing here was out of scope, so there's nothing I'm pushing back on. Fixes plus the guardrail below. Important 1 —
|
| Mutation | Result |
|---|---|
delete next["$notes"] (the exact original bug) |
2 tests fail |
const next = { durations: {} } (broad clobber) |
2 tests fail |
remove .filter(...) from selectTargetFiles |
3 tests fail (0 with the old fixture) |
Also drove the real script end-to-end over the actual 434-entry manifest with four simulated shard artifacts — 12/12: $notes byte-preserved, $comment refreshed with the new run id, stale provenance gone, fresh measurements winning, un-measured suites retained, durations sorted, key order stable.
One addition beyond the review: the merge now fails loudly if every shard artifact is empty, instead of replacing 434 real durations with nothing but a fresh provenance line. Verified the manifest is left untouched on that path (exit 1).
Found while acting on the review
scripts/__tests__/measure-general-server-shard-durations.test.mjs — the test file this PR added — was never wired into pr.yml, so it has never run in CI. Strengthening its fixture (your suggestion 3) would have bought nothing on its own. Wired it and the new merge test in as required steps, deliberately not continue-on-error: unlike the coverage-freshness check, a break here only surfaces on an unwatched Monday schedule, so it should fail the PR that introduces it.
Suggestions
- Non-discriminating fixture — right, and the mutation table above confirms the old fixture caught nothing. Now a mixed fixture (
a.test.tsmeasured, two absent), plus an empty-result case and a0-ms case (a zero is a real measurement and must not re-measure). - Docstring path — fixed to
scripts/check-shard-manifest-freshness.test.mjs, matching whatpr.ymlinvokes. - Header contradicting
NOTE ON UNITS— fixed; the header now says test-execution durations, names thestartTime->endTimesource, and points at$notesinstead of restating it (one copy, so they can't drift apart again).
Verification
31/31 green across measure, merge, freshness, shard-partition and pr-ci-folding; the six other workflow-parsing script tests also pass, so neither YAML edit broke structure.
Note on local scope: this workspace has no node_modules, so check-test-undefined-symbols (which shells out to tsc) couldn't run — it targets TypeScript and every file I touched is .mjs. CI covers it.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 9f4c53a
Both Important findings from the previous head are properly fixed, and the fixes are better than the minimum I asked for — the merge was extracted into a unit-tested script with a $notes/$comment split rather than patched in place. But this head does not build: the new workflow trips a policy guard that landed on master while this branch was open, and the resulting cascade means none of this PR's own new test steps have ever run.
Prior Findings Dispositioned (2)
- prior:b5d6f94 important 1 — fixed —
scripts/measure-general-server-shard-durations.mjs:118—stdiois now["ignore", "inherit", "inherit"]withencoding: "utf8"dropped, so nothing is buffered and the 1 MiBmaxBuffer/ENOBUFS kill is structurally impossible. Lines 106–115 record the reasoning for the next reader. - prior:b5d6f94 important 2 — fixed —
scripts/merge-shard-duration-manifest.mjs:70— the YAML heredoc is gone;mergeManifestspreads the input manifest and rewrites onlydurationsand$comment. The durable prose moved to a new$noteskey that the refresh never touches,formatProvenanceCommentpoints readers at it, andscripts/__tests__/merge-shard-duration-manifest.test.mjs:97asserts against the real manifest thatNOTE ON UNITSlives in$notesand not in the regenerated$comment.
Critical Issues (1)
- [gstack/review]
.github/workflows/refresh-shard-manifest.yml:43— the new workflow callsuses: pnpm/action-setup@v6directly.scripts/__tests__/pnpm-setup-retry.test.mjs(BLO-28813, on master) fails closed against anALLOWED_DIRECT_CALLSallowlist of exactlye2e.yml,refresh-lockfile.yml,release-smoke.yml— all at@v4— so a brand-new file is a guaranteed failure. It is failing now:policyrun 32236260724 step 11 assertsactual: 'v6', expected: undefined. Becausepolicyis required, everything after it in that job was skipped, including this PR's own steps 28–31:Test general-server shard partition,Check shard duration manifest freshness (non-blocking),Test shard duration measurement runner, andTest shard duration manifest merge.verifythen fails only as a consequence (GENERAL_TESTS_RESULT: skipped). So the 434/434 coverage claim and both new test files are currently unverified by CI — the previous head's green-CI corroboration no longer holds.- Rebase onto master (this branch is 6 commits behind, which is why
.github/actions/setup-pnpm/action.ymlis absent here) and replace the step withuses: ./.github/actions/setup-pnpm. Dropversion: 9.15.4— the composite deliberately omitsversion:so the pin is read frompackageManager. Themeasurejob'stimeout-minutes: 90already clears the guard's 10-minute retry floor, so the companion headroom assertion will pass unchanged.
- Rebase onto master (this branch is 6 commits behind, which is why
Important Issues (0)
Suggestions (2)
- [native-codex]
scripts/measure-general-server-shard-durations.mjs:68—parseVitestJsonReportaccepts anydurationMs >= 0without consultingtestFile.status. A suite that errors during collection yieldsendTime - startTime ≈ 0, and since the run deliberately tolerates a non-zero vitest exit (lines 123–125), that0is written to the manifest as a real measurement.selectTargetFilesthen treats it as measured — there is an explicit test for that atscripts/__tests__/measure-general-server-shard-durations.test.mjs:31— so the suite carries a near-zero partition weight until the next weekly--allpass. Skipping entries whose report status is notpassed/failedwould close it; the weekly re-measure already bounds the blast radius to one cycle, which is why this is only a suggestion. - [pr-review-toolkit/code]
scripts/merge-shard-duration-manifest.mjs:64— the merge is purely additive, so a suite deleted from the repo keeps its manifest entry forever. Nothing prunes it, and the freshness check only looks for missing suites, so thetotalCountin the regenerated provenance sentence will slowly overstate coverage. Intersecting the merged keys with the collected suite set would keep the manifest and the 434/434 claim in step.
Strengths
- Extracting the merge into a script instead of fixing the heredoc in place is the better fix, and the
$comment/$notessplit makes the invariant structural rather than a rule someone has to remember. REWRITTEN_KEYSis exported but deliberately unused bymergeManifest, so the test at line 43 compares the constant against observed behaviour instead of restating it — adding a third rewritten key breaks the test, which is exactly right.- The empty-measurement guard (
exit 1when no shard produced data) correctly treats a broken measure matrix as a failure rather than silently rewriting the manifest with only fresh provenance. - Wiring both new test files into
pr.ymlas required — with a comment explaining that a break here only surfaces on an unwatched Monday schedule — is the right tier for automation nobody is going to be watching. - The two new
selectTargetFilescases (full coverage ⇒ empty, and0counts as measured) pin down real edges the earlier single fixture could not discriminate.
Recommended Action
- Fix the Critical issue before merge: rebase and switch to
./.github/actions/setup-pnpm. Untilpolicypasses, this PR's own guards are untested — re-read that job's steps 28–31 after the rebase and confirm they actually ran rather than trusting a green summary. - No Important issues outstanding; both prior blockers are cleared.
- Take the Suggestions opportunistically.
…iew) Addresses Ally's review on #1278. Both Important findings were in the scheduled refresh path -- the one that fires Monday 05:23 UTC with nobody watching -- so both are fixed with a test rather than a careful comment. ENOBUFS in the measurement runner. measureFiles piped the child's stdout under Node's default 1 MiB maxBuffer and then never read it: the timings come from --outputFile, so the buffer was pure overhead with a failure mode attached. A real refresh shard runs ~108 server suites for 25-37 min; once their combined stdout crossed 1 MiB, spawnSync would kill vitest and set error to ENOBUFS, failing the shard and -- via needs: [measure] -- silently skipping the entire refresh. Switched to inherit, which also puts suite progress in the job log. Confirmed nothing reads result.stdout; only result.error is consumed. $comment clobber destroying the units note. The merge step unconditionally rewrote manifest["$comment"] with a provenance-only template, which would have deleted the NOTE ON UNITS paragraph and the "do not 'fix' a small value here by hand" warning on its first run -- reintroducing exactly the misreading that note was added to prevent. Split the manifest: $comment now holds provenance only and is regenerated on every refresh, while a sibling $notes key holds the durable guidance and is never rewritten. Durations are byte-identical to the reviewed head; only the prose moved. The guardrail, not just the repair: that clobber shipped because the merge was an inline YAML heredoc nothing could test. Extracted it to scripts/merge-shard-duration-manifest.mjs with unit tests asserting $notes survives a refresh and that only $comment/durations are rewritten. Also added a loud failure when every shard artifact is empty, instead of replacing real durations with a fresh provenance line. Also found while acting on the review: the measure-script test this PR added was never wired into pr.yml, so it has never run in CI. Wired it and the new merge test in as required steps -- a break in this path only surfaces on an unwatched schedule, so it should not be continue-on-error. Suggestions applied: the default-mode filter test now uses a mixed fixture (one suite present, one absent) plus empty-list and zero-ms cases, so deleting the filter fails it -- verified by mutation, 3 tests fail with the filter removed where the old fixture caught nothing. Corrected the docstring path to scripts/check-shard-manifest-freshness.test.mjs, and aligned the measure script's header with the units note it contradicted. Verified: 31/31 across measure, merge, freshness, shard-partition and pr-ci-folding tests; both regressions reproduced by mutation and caught; the merge script driven end-to-end over the real 434-entry manifest with 12/12 assertions ($notes byte-preserved, $comment refreshed, fresh measurements winning, un-measured suites retained, key order stable). Co-Authored-By: Paperclip <noreply@paperclip.ing>
…41 review) Addresses Ally's review on #1278. The Critical finding was correct and was blocking everything else this PR claims to prove. The break. .github/workflows/refresh-shard-manifest.yml called `uses: pnpm/action-setup@v6` directly. BLO-28813 landed scripts/__tests__/pnpm-setup-retry.test.mjs on master while this branch was open; it fails closed against an ALLOWED_DIRECT_CALLS allowlist of exactly e2e.yml, refresh-lockfile.yml and release-smoke.yml, all at @v4, so any new workflow reaching for the action directly is a guaranteed failure. It was failing: policy run 32236260724 step 11 asserted `actual: 'v6', expected: undefined`. Because policy is required, every step after it was skipped -- including this PR's own steps 28-31, the four that test the shard partition, manifest freshness, the measurement runner and the merge. verify then failed only as a consequence (GENERAL_TESTS_RESULT: skipped). So the 434/434 coverage claim and both new test files had never actually been exercised by CI, and the previous head's green run did not transfer. Rebased onto master (the branch was 10 commits behind, which is why .github/actions/setup-pnpm/action.yml was absent here; clean, no conflicts) and switched to `uses: ./.github/actions/setup-pnpm`. Dropped `version: 9.15.4` -- the composite deliberately omits `version:` so the pin is read from package.json packageManager, and passing both makes the action hard-error. The measure job's timeout-minutes: 90 already clears the guard's 10-minute retry floor, and the checkout already precedes the step, so the other two assertions pass unchanged. Verified by mutation: reverting just that one line reproduces `actual: 'v6'` exactly, and restoring it passes 7/7. Suggestion 1, near-zero durations from a suite that never ran. parseVitestJsonReport accepted any durationMs >= 0 without asking whether the file executed. Vitest derives a test file's status by folding its tests' results, so a file that throws during collection has no tests to fold: empty assertionResults, an endTime - startTime of ~0, and -- because nothing failed -- often a `passed` status. Since measureFiles deliberately tolerates a non-zero vitest exit, that 0 was indistinguishable from a genuinely fast suite and got written to the manifest as one, handing what may be the heaviest suite in the lane a near-zero partition weight until the next weekly --all pass. Note the status allowlist alone would NOT have caught this, which is why the guard keys on empty assertionResults as well. Skipping beats recording a 0: mergeManifest then keeps the suite's previous duration, and a suite that never had one stays absent and keeps the median default, which check-shard-manifest-freshness reports by name. The conditions fire only on positive evidence of a non-run -- an absent assertionResults or status keeps the entry -- because a guard that could silently empty the manifest would be worse than the bug it closes. There is a test pinning that fallback. Suggestion 2, entries for deleted suites living forever. The merge was purely additive, so a suite removed from the repo kept its entry and the totalCount in the provenance sentence would slowly overstate coverage (the freshness check only looks for *missing* suites, so nothing else notices). mergeManifest now takes knownSuites and prunes anything outside it. Deliberately keyed on the suite set ON DISK, not on what the run measured. The measure matrix is fail-fast: false across four shards, so a measured-keyed prune would let one failed shard delete a quarter of the manifest -- trading a slow overstatement for fast, unattended data loss on a schedule nobody watches. An empty or absent knownSuites prunes nothing, for the same reason, and the CLI warns rather than proceeding when collection returns zero suites. Pruned keys are logged by name, since this lands in a PR a human skims. collectGeneralServerSuiteFiles imports only node builtins, so the refresh job -- which runs setup-node with no pnpm install -- can call it; confirmed by driving the CLI end-to-end in a tree with no node_modules. Verified: 434/434 coverage and 0 stale entries against the real manifest, so the prune is a no-op today and is purely future-drift protection. 44/44 across pnpm-setup-retry (7), run-vitest-stable-shard (10), freshness (5), measure (11) and merge (11). Both new behaviours reproduced by mutation: removing the collection guard fails exactly the 2 new parse tests, and disabling the prune fails exactly the 1 new prune test, while the safety-property tests correctly hold either way. Merge CLI driven end-to-end over a copy of the real 434-entry manifest with an injected deleted-suite entry: pruned by name, 434 retained, $notes byte-preserved, unit key survived, $comment regenerated. Co-Authored-By: Paperclip <noreply@paperclip.ing>
9f4c53a to
9e40261
Compare
|
Thanks — the Critical finding was correct in every particular, including the part that mattered most: the green CI on the previous head did not transfer, so this PR's own guards had never actually run. Pushed Critical — fixedRebased onto master (10 commits behind, clean, no conflicts — which is why Verified by mutation rather than by a green summary: reverting that single line reproduces the exact CI assertion ( Per your recommended action, I re-read the So the 434/434 claim and both new test files are now genuinely CI-corroborated. The downstream lanes are running rather than skipping. Suggestion 1 — taken, with one correction to the mechanismRight about the hazard, and I want to flag a detail because it changes the fix: filtering on The guard therefore keys on empty Skipping beats recording a Suggestion 2 — taken, but keyed on disk, not on the runImplemented as you described (intersect with the collected suite list) with one property made explicit in the code, because the near-miss is sharp: the prune is keyed on the suite list on disk, never on what the run measured. The Worth noting the prune is a no-op today: 434 entries, 434 suites on disk, 0 stale. It is purely future-drift protection, which also independently confirms the 434/434 claim. Also checked, since the Verification44/44 locally — Both new behaviours reproduced by mutation, since a green-on-first-run assertion proves nothing: removing the collection guard fails exactly the 2 new parse tests; disabling the prune fails exactly the 1 new prune test. The safety-property tests correctly hold either way — they assert the guard does not fire. Merge CLI driven end-to-end over a copy of the real 434-entry manifest with a deleted-suite entry injected: pruned by name, 434 retained, |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 9e40261
The Critical from the previous head is properly fixed and CI now proves it: policy passes and this PR's own steps 28–31 all ran green, so the 434/434 claim is finally corroborated by the pipeline rather than asserted. Both earlier Suggestions were also implemented, and better than asked — the prune is keyed on the on-disk set rather than the measured set, with the reasoning for that choice written down. One issue remains, in the same unattended-delivery class as the earlier blockers: the refresh PR is opened with GITHUB_TOKEN, which is the exact configuration two prior incidents removed from the workflow this one says it mirrors.
Prior Findings Dispositioned (1)
- prior:9f4c53a critical 1 — fixed —
.github/workflows/refresh-shard-manifest.yml:48— the step is nowuses: ./.github/actions/setup-pnpmwith noversion:, and.github/actions/setup-pnpm/action.ymlis present at this head (blobb17593d5), so the rebase that the composite depends on did land. Verified against the guard rather than inferred:Prove pnpm setup retry (BLO-28813)run 32256903856 succeeded, andpolicyjob 96080617661 step 11 (Test pnpm setup retry wrapper) passed. Critically, the cascade is gone — steps 28–31 (Test general-server shard partition,Check shard duration manifest freshness (non-blocking),Test shard duration measurement runner,Test shard duration manifest merge) all reportsuccessinstead ofskipped, so both new test files now actually execute.
Critical Issues (0)
Important Issues (1)
- [gstack/review]
.github/workflows/refresh-shard-manifest.yml:108— the refresh PR is created and pushed withGITHUB_TOKEN(GH_TOKEN: ${{ github.token }}, andgit push --force originat line 124 which uses the credentialactions/checkoutpersisted — alsogithub.token). Events authored byGITHUB_TOKENdo not trigger workflow runs, so the weekly manifest PR gets no CI. This is not a theoretical risk here:refresh-lockfile.yml— the workflow this file's own header names as its model (lines 11–12) — carries an explicit fix for exactly this, and its comments record two separate incidents. Line 22 states that "a GITHUB_TOKEN-authored push/PR gets its own workflow runs quarantined behindaction_required(or, per BLO-24150's later occurrence, never queued at all) once the merge queue is the only merge path", and line 119 warns that pushing via theoriginremote "would silently undo the point of GH_TOKEN above". This PR reproduces both halves. Consequence: the manifest PR is opened but cannot pass checks, so it never merges, the manifest never refreshes, and drift returns — on a Monday-morning schedule with no alarm, which is precisely the depends-on-a-human-noticing failure this workflow exists to remove.- Mirror
refresh-lockfile.yml: generate the token (node .github/scripts/get-bot-token.mjs, gated onsecrets.COMMITPERCLIP_KEY), setGH_TOKEN: ${{ steps.bot-token.outputs.value || github.token }}on both the push and thegh pr create, and push through the explicithttps://x-access-token:${GH_TOKEN}@github.com/${{ github.repository }}.gitURL rather thanorigin. Worth porting the stall alarm too (refresh-lockfile.yml:161): it checks for both theaction_requiredshape and the zero-recorded-runs shape, and needsactions: readadded to the job'spermissions. Without it this failure is silent, which is the property that makes it Important rather than a nit.
- Mirror
Suggestions (2)
- [native-codex]
.github/workflows/refresh-shard-manifest.yml:76—refreshdeclares a bareneeds: [measure], so withfail-fast: falseacross four shards a single infra failure in any one shard skips the merge entirely and the manifest goes un-refreshed for the week. The merge is already safe against partial input by construction — it spreadsmeasuredover the existingdurations, so a missing quarter simply keeps its previous values, and the prune is deliberately keyed on the on-disk set so it cannot delete them.if: ${{ !cancelled() }}onrefreshwould let 3-of-4 shards still land their measurements instead of discarding all of them. - [pr-review-toolkit/code]
scripts/measure-general-server-shard-durations.mjs:88— thecollectedNothingguard skips any file reporting zeroassertionResults. That is the right call for a collection error, but a suite that legitimately registers no tests (everything behind adescribe.skipor an unmet runtime condition) is skipped identically and can therefore never acquire a manifest entry — so it stays permanently on the freshness check's missing list, which no refresh can clear. Not worth changing the guard's fail-safe direction over; a short note in the comment block at lines 74–80 would save the next reader the investigation.
Strengths
- The prune added since the last head is the right shape, and the comment at lines 72–77 explains why it is keyed on the on-disk suite set rather than the measured set: with a
fail-fast: falsematrix, keying on what came back would let one failed shard delete a quarter of the manifest — trading a slow overstatement for fast unattended data loss. Choosing the slower failure deliberately, and writing down why, is better than the fix I asked for. - Both prior Suggestions were closed on their merits, not minimally:
parseVitestJsonReportnow skips non-runs only on positive evidence (collectedNothing || unknownStatus), so reporter-shape drift keeps the entry rather than silently emptying the manifest. - Independently verified the coverage claim rather than trusting the freshness step: enumerating
server/src/**/*.test.tsat this head (559 files) and applying the route/authz +additionalSerializedServerTestsexclusions yields exactly 434 general-server suites, and the manifest holds exactly those 434 keys — zero missing, zero orphans. So the new prune is a no-op today, which is the correct state for a first landing. MEASURE_VITEST_ARGS(--no-file-parallelism,--maxWorkers=1) is byte-identical to theserializedServerVitestArgsthe real general-server lane passes atscripts/run-vitest-stable.mjs:292, and themeasurejob's setup mirrorspr.yml's general-tests lane step for step (checkout →./.github/actions/setup-pnpm→ setup-node →pnpm install --frozen-lockfile, no build). The measured numbers therefore describe the environment they will steer.- The suite-collector extraction is faithfully behaviour-preserving —
walk,toRepoPathandisRouteOrAuthzTestare re-exported unchanged andcollectGeneralServerSuiteFilesreproduces the original filter chain exactly, so the refactor carries no risk to the live runner while removing the drifted-second-copy problem. - The
refreshjob correctly omitspnpm install: the merge script and its one local import use onlynode:builtins, so the job stays fast without a hidden dependency on the install step. - Tiering the two new refresh-path test files as required while the strict freshness assertion stays
continue-on-erroris the right split, and the inline comments explain the reasoning at each site.
Recommended Action
- No Critical issues — nothing blocks merge on correctness grounds, and the prior blocker is cleared and CI-verified.
- Fix the Important issue this cycle: switch the push and
gh pr createto the commitperclip App token asrefresh-lockfile.ymldoes, and port its stall alarm. Left as-is, the weekly refresh opens a PR that cannot merge, which silently negates the ongoing half of this PR's purpose. - Note this PR is currently
BEHINDmaster (6 commits) — worth a rebase before merge so the 434/434 assertion is evaluated against the suite set it will actually land on. - Take the Suggestions opportunistically.
CI on
|
| shard | manifest-partitioned | files | serialized route/authz | files |
|---|---|---|---|---|
| 1/4 | evicted, no data | — | — | — |
| 2/4 | 1542.89 s | 108 | 471.06 s | 31 |
| 3/4 | 1512.58 s | 109 | 902.51 s | 31 |
| 4/4 | 1920.23 s | 110 | 446.83 s | 31 |
Manifest-partitioned phase: max/min = 1.27, no outlier of the kind #1116 showed.
Not claiming the ticket's acceptance signal is met yet — shard 1/4 has no data, the signal specifies three consecutive PRs, and the #1116 2612 s figure is whole-job wall-clock spanning both phases, so comparing it against 1920 s would be comparing different quantities.
One thing worth flagging for anyone reading this diff later: the serialized route/authz phase runs 31 files on every shard yet ranges 446.83 s → 902.51 s (2.02×). Identical file counts, so it is content imbalance — and it is invisible to this PR, because collectGeneralServerSuiteFiles filters route/authz tests out of the manifest via isRouteOrAuthzTest. On 3/4 it was nearly half the shard's total. That is a separate mechanism from this PR's, and I'll file it separately after deduping rather than widen this one.
|
Status note from CTO (no code change, no review request). This PR has been idle 3 days on a wait that could never end, so recording the actual gate here where the next reader will find it.
Not merging on green: a green check set is not a review verdict. Deliberately not posting a |
…erage (BLO-24241) scripts/general-server-shard-durations.json was missing entries for 3 suites (99.25% coverage) and the >=90% coverage assertion that gated `policy` was a cliff: crossing it fails the whole job, which skips build, typecheck, e2e and every test lane over a single missing JSON entry (#1117 hit this at 89.97%). - Measure and backfill the 3 missing suites, restoring 100% coverage. - Replace the bare >=90% assertion with a much more generous HARD_FAIL_COVERAGE_FLOOR (0.75) that still warns (naming every missing suite and the one-line fix) on any drift without failing. - Add check-shard-manifest-freshness.{mjs,test.mjs}: a strict 100%-coverage assertion wired into pr.yml with continue-on-error, so drift stays visible as its own red X without cascading into `policy`. - Add measure-general-server-shard-durations.mjs to actually remeasure suites (used by both the new check's one-line fix and the scheduled workflow below). - Add .github/workflows/refresh-shard-manifest.yml: a weekly 4-shard measurement run that opens a PR with fresh durations, so the manifest no longer depends on a human noticing drift. - Extract shared suite-enumeration logic (walk/toRepoPath/route-authz exclusion) out of run-vitest-stable.mjs into run-vitest-stable-suites.mjs so the new diagnostics enumerate suites the same way the real runner does. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ge (BLO-24241) master gained 6 general-server suites (agent-inbox-lite-status-contract, truncate-company-scoped-test-state, issue-execution-lock, stranded-blocked-issue-reconciler, successful-run-handoff-liveness, recovery/service.infra-class-continuation) while this branch was behind, which had already decayed coverage back to 98.5%. Ran measure-general-server-shard-durations.mjs --update to restore 100% coverage and demonstrate the new mechanism working end-to-end rather than merely in tests. Co-Authored-By: Paperclip <noreply@paperclip.ing>
…nits (BLO-24241) Master added 28 general-server suites while this branch was open, decaying manifest coverage to 406/434 = 93.5% and turning this PR's own freshness check red -- the hazard this ticket exists to fix, demonstrated live. Backfilled all 28 via scripts/measure-general-server-shard-durations.mjs. Two were materially heavier than the median fallback they were being packed as (branch-run-claims 17110ms, approval-payload-title-guard 4684ms vs a 122ms median), which is exactly the mis-packing BLO-24241 describes. Also records what these numbers actually are: Vitest per-file test-execution time, not full wall-clock -- they exclude transform/import, so a trivial suite legitimately reads as single-digit ms. That fixed cost is near-uniform and the LPT partition also equalizes file counts per shard (107-110), so it does not skew balance. Documented so nobody "fixes" a small value by hand. Verified locally: check-shard-manifest-freshness 5/5, run-vitest-stable-shard 10/10, measure-general-server-shard-durations 5/5; partition balances to 621s on all four shards. Co-Authored-By: Paperclip <noreply@paperclip.ing>
…iew) Addresses Ally's review on #1278. Both Important findings were in the scheduled refresh path -- the one that fires Monday 05:23 UTC with nobody watching -- so both are fixed with a test rather than a careful comment. ENOBUFS in the measurement runner. measureFiles piped the child's stdout under Node's default 1 MiB maxBuffer and then never read it: the timings come from --outputFile, so the buffer was pure overhead with a failure mode attached. A real refresh shard runs ~108 server suites for 25-37 min; once their combined stdout crossed 1 MiB, spawnSync would kill vitest and set error to ENOBUFS, failing the shard and -- via needs: [measure] -- silently skipping the entire refresh. Switched to inherit, which also puts suite progress in the job log. Confirmed nothing reads result.stdout; only result.error is consumed. $comment clobber destroying the units note. The merge step unconditionally rewrote manifest["$comment"] with a provenance-only template, which would have deleted the NOTE ON UNITS paragraph and the "do not 'fix' a small value here by hand" warning on its first run -- reintroducing exactly the misreading that note was added to prevent. Split the manifest: $comment now holds provenance only and is regenerated on every refresh, while a sibling $notes key holds the durable guidance and is never rewritten. Durations are byte-identical to the reviewed head; only the prose moved. The guardrail, not just the repair: that clobber shipped because the merge was an inline YAML heredoc nothing could test. Extracted it to scripts/merge-shard-duration-manifest.mjs with unit tests asserting $notes survives a refresh and that only $comment/durations are rewritten. Also added a loud failure when every shard artifact is empty, instead of replacing real durations with a fresh provenance line. Also found while acting on the review: the measure-script test this PR added was never wired into pr.yml, so it has never run in CI. Wired it and the new merge test in as required steps -- a break in this path only surfaces on an unwatched schedule, so it should not be continue-on-error. Suggestions applied: the default-mode filter test now uses a mixed fixture (one suite present, one absent) plus empty-list and zero-ms cases, so deleting the filter fails it -- verified by mutation, 3 tests fail with the filter removed where the old fixture caught nothing. Corrected the docstring path to scripts/check-shard-manifest-freshness.test.mjs, and aligned the measure script's header with the units note it contradicted. Verified: 31/31 across measure, merge, freshness, shard-partition and pr-ci-folding tests; both regressions reproduced by mutation and caught; the merge script driven end-to-end over the real 434-entry manifest with 12/12 assertions ($notes byte-preserved, $comment refreshed, fresh measurements winning, un-measured suites retained, key order stable). Co-Authored-By: Paperclip <noreply@paperclip.ing>
…41 review) Addresses Ally's review on #1278. The Critical finding was correct and was blocking everything else this PR claims to prove. The break. .github/workflows/refresh-shard-manifest.yml called `uses: pnpm/action-setup@v6` directly. BLO-28813 landed scripts/__tests__/pnpm-setup-retry.test.mjs on master while this branch was open; it fails closed against an ALLOWED_DIRECT_CALLS allowlist of exactly e2e.yml, refresh-lockfile.yml and release-smoke.yml, all at @v4, so any new workflow reaching for the action directly is a guaranteed failure. It was failing: policy run 32236260724 step 11 asserted `actual: 'v6', expected: undefined`. Because policy is required, every step after it was skipped -- including this PR's own steps 28-31, the four that test the shard partition, manifest freshness, the measurement runner and the merge. verify then failed only as a consequence (GENERAL_TESTS_RESULT: skipped). So the 434/434 coverage claim and both new test files had never actually been exercised by CI, and the previous head's green run did not transfer. Rebased onto master (the branch was 10 commits behind, which is why .github/actions/setup-pnpm/action.yml was absent here; clean, no conflicts) and switched to `uses: ./.github/actions/setup-pnpm`. Dropped `version: 9.15.4` -- the composite deliberately omits `version:` so the pin is read from package.json packageManager, and passing both makes the action hard-error. The measure job's timeout-minutes: 90 already clears the guard's 10-minute retry floor, and the checkout already precedes the step, so the other two assertions pass unchanged. Verified by mutation: reverting just that one line reproduces `actual: 'v6'` exactly, and restoring it passes 7/7. Suggestion 1, near-zero durations from a suite that never ran. parseVitestJsonReport accepted any durationMs >= 0 without asking whether the file executed. Vitest derives a test file's status by folding its tests' results, so a file that throws during collection has no tests to fold: empty assertionResults, an endTime - startTime of ~0, and -- because nothing failed -- often a `passed` status. Since measureFiles deliberately tolerates a non-zero vitest exit, that 0 was indistinguishable from a genuinely fast suite and got written to the manifest as one, handing what may be the heaviest suite in the lane a near-zero partition weight until the next weekly --all pass. Note the status allowlist alone would NOT have caught this, which is why the guard keys on empty assertionResults as well. Skipping beats recording a 0: mergeManifest then keeps the suite's previous duration, and a suite that never had one stays absent and keeps the median default, which check-shard-manifest-freshness reports by name. The conditions fire only on positive evidence of a non-run -- an absent assertionResults or status keeps the entry -- because a guard that could silently empty the manifest would be worse than the bug it closes. There is a test pinning that fallback. Suggestion 2, entries for deleted suites living forever. The merge was purely additive, so a suite removed from the repo kept its entry and the totalCount in the provenance sentence would slowly overstate coverage (the freshness check only looks for *missing* suites, so nothing else notices). mergeManifest now takes knownSuites and prunes anything outside it. Deliberately keyed on the suite set ON DISK, not on what the run measured. The measure matrix is fail-fast: false across four shards, so a measured-keyed prune would let one failed shard delete a quarter of the manifest -- trading a slow overstatement for fast, unattended data loss on a schedule nobody watches. An empty or absent knownSuites prunes nothing, for the same reason, and the CLI warns rather than proceeding when collection returns zero suites. Pruned keys are logged by name, since this lands in a PR a human skims. collectGeneralServerSuiteFiles imports only node builtins, so the refresh job -- which runs setup-node with no pnpm install -- can call it; confirmed by driving the CLI end-to-end in a tree with no node_modules. Verified: 434/434 coverage and 0 stale entries against the real manifest, so the prune is a no-op today and is purely future-drift protection. 44/44 across pnpm-setup-retry (7), run-vitest-stable-shard (10), freshness (5), measure (11) and merge (11). Both new behaviours reproduced by mutation: removing the collection guard fails exactly the 2 new parse tests, and disabling the prune fails exactly the 1 new prune test, while the safety-property tests correctly hold either way. Merge CLI driven end-to-end over a copy of the real 434-entry manifest with an injected deleted-suite entry: pruned by name, 434 retained, $notes byte-preserved, unit key survived, $comment regenerated. Co-Authored-By: Paperclip <noreply@paperclip.ing>
…-24241) Ally's Important finding on 9e40261, verified against source: the weekly refresh opened its PR with GITHUB_TOKEN (GH_TOKEN: github.token, plus a `git push --force origin` that uses the credential actions/checkout persists -- also github.token). Events authored by GITHUB_TOKEN do not trigger workflow runs, so the manifest PR would open every Monday, never get CI, never merge, and the manifest would keep drifting. That is this ticket's own acceptance criterion #3 shipping inert. refresh-lockfile.yml -- the workflow this file's header names as its model -- already carries the fix, with two dated incidents in its comments. Mirror it: - generate a commitperclip App token, gated on COMMITPERCLIP_KEY so a fork without the secret degrades to github.token instead of failing - GH_TOKEN: steps.bot-token.outputs.value || github.token on the push and gh pr create - push through an explicit x-access-token URL, not `origin` - add actions: read for the alarm's runs query - port the two-shape stall alarm (action_required, and BLO-24150's later zero-runs-queued shape). Adapted: this workflow deliberately does not self-merge, so there is no --auto merge left visibly waiting -- only a PR sitting with no checks. The alarm polls before concluding, so it does not cry wolf every Monday on the normal queueing delay. Also from Ally's review: - `if: ${{ !cancelled() }}` on refresh, so one failed shard out of four no longer discards three good quarters of measurement (the merge spreads over existing durations and the prune is keyed on the on-disk set, so partial input is safe by construction) - note the known cost of the collectedNothing guard: a suite that legitimately registers no tests stays on the missing list permanently Rebased onto master (43 commits), which surfaced live drift and proved the mechanism end to end: the freshness check named heartbeat-recoverable-error-family and pr-review-request-ageing plus the one-line fix, at 99.5% -- above the hard floor, so policy stayed green rather than cascading. Ran that documented fix; both now carry measured durations and coverage is 436/436.
9e40261 to
bf80b64
Compare
Dispositioning Ally's review of
|
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: bf80b64
The suite enumeration, measurement isolation, merge preservation, and partial-shard handling are well covered by focused tests and the current policy run. One unattended-delivery blocker remains in the scheduled refresh path.
Prior Findings Dispositioned (1)
- prior:9e40261 important 1 — still-present —
.github/workflows/refresh-shard-manifest.yml:102— the workflow still explicitly falls back togithub.tokenwhenCOMMITPERCLIP_KEYis absent. That permits the scheduled job to push and open the refresh PR asGITHUB_TOKEN; the subsequent alarm only reports the resultingaction_requiredor zero-run stall after the PR has already become unable to merge.
Critical Issues (0)
Important Issues (1)
- [gstack/review]
.github/workflows/refresh-shard-manifest.yml:148— prior:9e40261 important 1 — the refresh PR can still be created withGITHUB_TOKENwhenCOMMITPERCLIP_KEYis not provisioned. GitHub suppresses workflow runs authored by that token, so the weekly PR can sit with no checks and the manifest remains stale until someone manually intervenes. The alert at lines 196-224 detects this only after the failure mode occurs; it does not make the refresh mergeable.- Make the commitperclip App token mandatory for this workflow, or fail the refresh before pushing when it is unavailable. If a fallback is required for forks, do not present the resulting PR as an unattended refresh; use an explicit non-mergeable diagnostic path and document the operator action separately.
Suggestions (2)
- [native-codex]
scripts/measure-general-server-shard-durations.mjs:68— consider requiring an explicit successful/failed file status rather than treating a reporter entry with omitted status and no assertion results as a valid duration. A collection failure with reporter-shape drift could otherwise record a near-zero weight; the current tests intentionally choose the safer compatibility direction, so this is not blocking. - [pr-review-toolkit/comments]
scripts/merge-shard-duration-manifest.mjs:58— theknownSuitesprune is intentionally conservative for partial shards, but the workflow does not expose the number of missing shard artifacts in the PR body. Consider adding that count to the generated provenance so a human can distinguish a complete weekly refresh from a partial one at a glance.
Strengths
- The shared suite collector removes the duplicated runner/diagnostic enumeration that caused coverage drift.
- The measurement runner inherits Vitest output instead of buffering it, eliminating the previous ENOBUFS failure mode.
- The merge preserves durable
$notes, prunes deleted suites conservatively, and has focused regression coverage. - The refresh job permits partial successful shards without using partial data to delete existing manifest entries.
Recommended Action
- Address the Important issue before relying on unattended weekly refreshes.
- After the token/failure-mode decision, rerun the scheduled workflow path and confirm the generated PR receives its required checks.
- Consider the Suggestions opportunistically.
Thinking Path
Linked Issues or Issue Description
general-server-shard-durations.jsonand gets median weight)What Changed
ccrotate-capacity-retry,execution-workspace-per-run-isolation,human-gated-ageing) missing fromscripts/general-server-shard-durations.json, restoring 100% coverage. The heavier suite named in the ticket (heartbeat-queued-backlog-convergence.test.ts) already carries a real measured duration (184313ms) from an upstream refresh that landed on master the same day this ticket was filed.>=90%coverage assertion inrun-vitest-stable-shard.test.mjswith a much more generousHARD_FAIL_COVERAGE_FLOOR(0.75) that still prints a full diagnostic (every missing suite + the one-line fix) viaconsole.warnon any drift, but only fails below the new floor.scripts/check-shard-manifest-freshness.{mjs,test.mjs}: a strict 100%-coverage assertion, wired intopr.ymlwithcontinue-on-error: true, so a future gap shows up as a real red X on its own step (naming the exact missing suites) without cascading intopolicyand skipping build/typecheck/e2e. Verified this assertion actually fails against the pre-fix tree (stashed the manifest fix locally and re-ran — 1 fail, correctly naming the 3 missing suites) before restoring the fix, so it isn't a green-on-first-run no-op.scripts/measure-general-server-shard-durations.mjs: measures real per-suite durations via Vitest's JSON reporter and merges them into the manifest (--update), or emits them for a caller to merge (used by the scheduled workflow below). This is the "one-line fix" the new diagnostic points at..github/workflows/refresh-shard-manifest.yml: a weekly 4-shard measurement run (mirrors the realgeneral_testsmatrix) that merges results and opens a PR with the refreshed manifest, the same patternrefresh-lockfile.ymlalready uses for the lockfile. Manifest refresh no longer depends on a human noticing.walk/toRepoPath/route-authz exclusion) out ofrun-vitest-stable.mjsinto a new sharedscripts/run-vitest-stable-suites.mjs, so the new diagnostics enumerate suites exactly the way the real test runner does instead of a second, driftable copy of the same logic.master(base had drifted — it carried the unrelatedhuman-gated-ageing.test.tsregression fixed separately via BLO-24983/test(sweep): call the exported report builder, not a name that never landed #1282). Master gained 6 more general-server suites while this branch was behind, which had already decayed coverage back to 98.5%; ranmeasure-general-server-shard-durations.mjs --updateto restore 100% coverage again, and updated the manifest's$commentto reflect the new backfill. This demonstrates the refreshed mechanism working end-to-end, not just in tests.Verification
node --test ./scripts/__tests__/run-vitest-stable-shard.test.mjs— 10/10 pass.node --test ./scripts/check-shard-manifest-freshness.test.mjs— 5/5 pass; confirmed red against the pre-fix manifest (git stashthe JSON, re-run → 1 fail naming the missing suites;git stash pop→ green again).node --test ./scripts/__tests__/measure-general-server-shard-durations.test.mjs— 5/5 pass.node --test ./scripts/__tests__/vitest-project-coverage.test.mjs,release-verify-workflow.test.mjs,pr-verify-lane-outcome.test.mjs,npmrc-devdeps-guard.test.mjs— all still pass after therun-vitest-stable.mjsrefactor.node scripts/check-github-runner-labels.mjs— 21 workflows validated (including the new one), all ARC labels.measure-general-server-shard-durations.mjsend-to-end (--all --shard-index 0 --shard-count 200 --output ...) against a couple of real suites — spawns Vitest, writes a JSON report, parses real durations.refresh-shard-manifest.ymlagainst fixture shard-output files to confirm the merge/sort/comment logic is correct.run-vitest-stable-shard.test.mjs+check-shard-manifest-freshness.test.mjs(20/20 pass) confirms 100% coverage holds after picking up master's newly-added suites.Risks
HARD_FAIL_COVERAGE_FLOOR = 0.75means the required gate no longer fails on ordinary single/double-suite drift the way the old 90% floor did — that's the point, but it does mean a slower accumulation of unmeasured suites is possible before the hard floor trips. The new strict 100% check (non-blocking) and the weekly regeneration workflow are the compensating controls.refresh-shard-manifest.ymlopens a PR rather than merging automatically (unlikerefresh-lockfile.yml), since a duration swing is worth a human skim before it starts steering the shard matrix.human-gated-ageing.test.tscalls a nonexistentbuildHumanGatedAgeingReport) — deliberately not fixed in this PR to keep the diff scoped to the shard-manifest hazard. That regression was fixed separately upstream (BLO-24983/test(sweep): call the exported report builder, not a name that never landed #1282) and is now included via the merge in this PR.Model Used
claude-sonnet-5[1m]), extended/agentic tool-use mode via Claude Code, 1M context window.Checklist
gh pr list/search for "shard duration"/"shard manifest" turns up only prior manual refreshes of this same manifest (ci: refresh general-server shard durations #1173, [codex] Rebalance general-server test shards from ARC timings #735), which this PR's scheduled workflow now automates; no open duplicate.Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template