bb97ffb5aa28ee569bcc2b744bab8f7b8e45f12c
12 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Tom Boucher
|
448e148058 |
fix(#2455): abort remaining chunks when one hits the per-chunk timeout (#2465)
The per-chunk timeout exists so a bad chunk fails loudly 'rather than silently burn the job's wall-clock budget until the CI runner cancels the whole job' (run-tests.cjs:633-637). The control flow defeated that: after a timeout kill the loop fell through to the next chunk. Since the timeout (600000ms) is half the 20m job cap and a healthy Windows full pass is ~11m42s, continuing after a timeout can essentially never finish. Observed on run 29749380190 (windows shard 2/3): chunk 1/5 was killed at exactly 600s, the loop pressed on through chunks 2-4, and the job was cancelled mid-chunk-5 at the 20m wall. The failure surfaced as '##[error]The operation was canceled.' — the timeout diagnostic ended up ~38,000 log lines from the end and 'gh run view --log-failed' returned nothing, making the real cause very hard to find. Abort the remaining chunks on a timeout so the diagnostic survives as the visible failure. Ordinary test failures still run every chunk, so the operator keeps seeing all failures in one pass. Fixes #2455 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Tom Boucher
|
953b8043ea |
fix(#2456): weight test chunks by measured cost and pack with LPT (#2463)
* fix(#2456): weight test chunks by measured cost and pack with LPT scripts/run-tests.cjs guessed each test file's cost from its filename (basename matching /^(?:install|codex-)/ scored 12, everything else 1). Measured durations show that guess is wrong in both directions: installer-migration-authoring.test.cjs scored 12 while running ~0.1s, and the two most expensive files in the suite both scored 1 — run-tests-harness.test.cjs never matched the prefix, and release-tarball-smoke.install.test.cjs was missed because the regex is anchored to the START of the basename. Chunks were therefore balanced by file COUNT, not cost. On the real shard 2/3 the two heaviest files packed into the SAME chunk, leaving the slowest chunk 2.8x the lightest and sitting near the 600s per-chunk timeout while other chunks idled. Weight each file by its measured duration from a checked-in, regenerable timings table and pack with LPT (heaviest first, into the lightest chunk). On the same shard this drops the slowest chunk from 383s to 238s and the imbalance from 2.79x to 1.00x, and separates the two heavy files. Timings are advisory, never gated: an unknown file falls back to the table's median weight, a missing or corrupt table falls back to uniform weight, and a count-based floor guarantees the packer never produces fewer chunks than plain count-based packing would. Closes #2456 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2456): harden chunk packing against degenerate knobs and table keys Follow-up hardening found while reviewing the packer, fixed inline. The chunk knobs are read from the environment with Number(), so a typo (RUN_TESTS_MAX_FILES_PER_CHUNK=abc) yields NaN and an explicit 0 yields 0. Both flow into the new chunk-count arithmetic: NaN made Math.ceil return NaN, Array.from({length: NaN}) produce zero bins, and packChunks' retry loop spin forever — a hung CI job with no output. Zero made the count Infinity and threw RangeError: Invalid array length. The previous count-based packer degraded to a single chunk instead, so this was a regression introduced by the LPT rewrite. Normalize the knobs at the environment boundary (positiveNumberEnv: anything not a positive finite number falls back to the default) and guard packChunks itself, since it is exported and cannot assume its caller normalized. Non-finite weights from an arbitrary weightOf are clamped too. RUN_TESTS_CHUNK_TIMEOUT_MS gets the same treatment. Also resolve timing-table lookups with Object.hasOwn: the table is JSON-parsed, so a bare index would walk the prototype chain and return a function for a file named constructor.test.cjs or toString.test.cjs. The typeof guard already rejected that, but the lookup now resolves correctly rather than relying on the downstream check. Refs #2456 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2456): correct prototype-lookup rationale and guard generator keys Two findings from independent security review, fixed inline. The makeFileWeigher comment claimed a bare table lookup "would return a FUNCTION for a file named constructor.test.cjs". That premise is false: basename('constructor.test.cjs') is 'constructor.test.cjs', which is not an Object.prototype key, and walkTestFiles only ever collects *.test.cjs. The prototype chain was never reachable from a real selection, and the existing typeof guard already rejected the function it would return, so Object.hasOwn is defense-in-depth rather than a behavior change. The comment now says that instead of asserting something untrue. The accompanying test inherited the same false premise: it fed constructor.test.cjs and asserted a median fallback that would have held with or without the guard, so it passed for a reason unrelated to what it claimed to prove. It now uses BARE keys (constructor, toString, valueOf, hasOwnProperty, __proto__) — the only inputs that actually resolve on Object.prototype — and asserts the real exported contract: any key absent from the table weighs the median, never a function. gen-test-timings.cjs built its output object by computed-key assignment from basenames taken out of a reporter stream it does not control — the js/prototype-polluting-assignment shape, and this repo has a CodeQL barrier for exactly that pattern. It was not exploitable (the value is always a rounded number, so the __proto__ setter is a silent no-op), but it silently DROPPED such an entry rather than reporting it. Validate every key against a test-basename pattern and fail loudly instead, and build the table with a null prototype. Refs #2456 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2456): replace tautological chunking tests and clamp chunk count Six findings from independent correctness review, all reproduced and fixed inline. The two subprocess tests written to carry the #2088 guarantee forward were tautological: every seeded file weighed exactly 1, so both passed under the OLD prefix-heuristic packer and with the timings file deleted entirely. Neither could fail for the reason it existed. Both are rebuilt so the old algorithm produces a different packing and the assertion goes red: the spread test now uses three expensive files named so the old heuristic scored them 1 alongside three trivial `install-`-prefixed files it scored 12 — inverted from real cost, giving {2,2,1,1} under the old packer versus {2,2,2} under measured weights. The companion test covers the other direction: four trivial `install-` files the old heuristic split into four single-file chunks now stay in one. packChunks clamped the chunk count from below but not above, so a legitimate but tiny budget (RUN_TESTS_MAX_FILES_PER_CHUNK=1e-9, which positiveNumberEnv accepts) asked for 637,000,000,000 bins and threw RangeError. More chunks than files is never useful; the count now clamps at one file per chunk. The generator's basename-collision guard compared full dirnames, so two OS lanes reporting the same file under different container roots (/work/tests vs C:/work/tests) flagged every shared basename as a collision — on the script's own documented multi-lane usage. Detection is now scoped per stream, where the root is constant; a genuine same-lane collision is still caught. Also: the LPT tie-break compared raw paths, so a path separator (0x2F vs 0x5C) could order a subdir file differently per platform, contradicting the documented byte-identical guarantee — it now normalizes separators. loadTestTimings now honors schema_version instead of writing it and never reading it, falling back to uniform weight on an unknown version. A comment claiming an all-uniform suite "chunks exactly as it did before" was false and contradicted by this PR's own test: the chunk count is preserved, the composition is not. And the missing-table test created a temp dir it never cleaned up, for a path that only needed to not exist. Refs #2456 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Tom Boucher
|
8b99f4f3c3 |
fix(#2088): weight install-heavy test files so they spread across chunks
The targeted CI lane runs changed files UNSHARDED; #2088 touched 13 install-heavy test files that all landed in one chunk, blowing the 600s per-chunk backstop on the slow Windows runner (pure slowness, not a leak — per run-tests.cjs's own comment). Weight install*/codex-* files (~10x a unit file) toward the per-chunk budget so they spread across chunks instead of clustering; light-file chunking is unchanged (weight 1). Adds harness regression tests (heavy split vs light control). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Tom Boucher
|
6d072435d0 |
test(#1975): consolidate 51 CLI + scripts-tooling regression tests into module suites
Fold 51 issue-named CLI black-box + scripts-tooling regression files into their canonical module suites (runtime-launcher-parity, worktree-safety, install-*, managed-hooks, read-guard, capability-registry, etc.), plus a NEW slash-command-namespace.test.cjs grouping the 4 slash/colon-namespace-leak invariant suites that had no canonical owner. Verbatim block-scoped describe wrappers; 427 subtests conserved 1:1. Host-env pre-check (per B2): no CLI-receiving host sets a redirecting GSD_WORKSTREAM/GSD_PROJECT value. One folded suite (bug-3668 runtime resolver) creates an extension-less PATH gsd-tools stub + bash -c; co-locating it with the host's chmodSync tripped local/no-unguarded-nonportable-exec, so it's now Windows-guarded (skip on win32) matching the host suite's own bash -c guard. Regenerates regression-name allowlist (222->182), ratchets file-count allowlist (graphify 7->6, docs entry removed), makes 26 relocated allow-test-rule exemptions issue-ref-compliant (ADR-456; prunes stale ids). Repoints 13 tests/ references across CONTEXT.md, COMMANDS.md/FEATURES.md (EN + ja/ko/pt/zh) and ADR-0002. lint:ci green. Part of epic #1969. Closes #1975. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Tom Boucher
|
22f56f4431 |
ci(#1212): shard windows full-test lane to remove timeout cliff (#1222)
The `full test (windows-latest, *)` lane ran the entire unit suite (~740+ files) in one job whose wall-clock crept against the 20m cap and intermittently CANCELLED (false-negative gate, observed on PR #1207). Prior tactical fixes #869 (15→20m bump) and #1051 (handle-leak) deferred the cliff structurally. Shard the unit suite across 3 parallel runners per OS/node leg so per-job wall-clock is O(total/3) and stays under the cap as the suite grows. - scripts/run-tests.cjs: add `--shard <i>/<n>` — a deterministic, balanced round-robin partition (fileIndex % n === i-1) over the SORTED selected file list. parseShardArg strictly validates i∈1..n, n≥1, integer-only; n=1 is a pure no-op. The 28K Windows argv chunking is preserved within each shard. A legitimately-empty shard (n > file count) exits 0; a selection empty BEFORE sharding still hits the discovery hard error. Composes with --suite and is order-independent (sorted before partition). Exports selectShard/parseShardArg. - .github/workflows/test.yml: test-full becomes the 3 legs × 3 shards = 9-job cross-product (explicit include rows — a base shard dim does not cross-product with include legs, and a nested matrix.leg.os is unresolvable by the H1 shell-policy linter). Unit suite runs sharded; integration/security run once per leg (shard 1). The Required tests fan-in is unchanged: it already needs test-full and checks the matrix-aggregate result, so a failed/cancelled shard fails the gate; the branch-protection check name is preserved. - tests: partition/CLI + pure selectShard contract (completeness, disjointness, balance, determinism, boundaries, fast-check property) + parseShardArg validation, in run-tests-harness.test.cjs; a DEFECT.GENERATIVE-FIX parity guard (per-row shard values 1..N, every leg runs all shards, N == --shard /N denominator) + Required-tests name/needs pin, in ci-test-scope.test.cjs. Closes #1212 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Tom Boucher
|
5fa4dcd78c |
fix: recover silently-excluded test dirs + test-architecture audit hardening (#1195)
* fix: recurse test discovery so subdir test suites actually run
scripts/run-tests.cjs discovered tests with a flat readdirSync(testDir),
silently excluding tests/observability/ (4 files), tests/dispatch/ (1) and
tests/installer-migrations/ (1) — 94 passing tests — from `npm test` and all
CI lanes. Walk the tree recursively (relative subpaths preserved), classify
suites by basename, and add a fail-on-zero-executed guard for suite/default
runs (escape hatch GSD_ALLOW_EMPTY_SUITE=1) while preserving the empty
--files/--files-from path the CI inert lane relies on.
Unit suite 735 -> 741 files; surfaces ADR-227's observability/dispatch seam.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test: retire 5 verified-worthless tests
Adversarial verification confirmed these 5 prove nothing — their coverage is
provided more strictly elsewhere:
- enh-2790 'has a name: field' spot-checks (command-contract enforces /^gsd[:-]/)
- command-routing-hub duplicate construct + duplicate ERROR_KINDS assertions
- no-cjs-sdk-handsync-tooling (guarded files that never existed on main; bug-190
covers the real retired SDK artifacts)
- runtime-artifact-layout cline edge case (subsumed by the explicit-global test
and bug-782-cline-skills-emission)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test: add ADR-218 release version-validation coverage
ADR-218 (reject leading-zero versions like 1.01.0; npm duplicate pre-check) had
zero tests — the logic lived only in release.yml bash. Add a test that extracts
the actual rejection regexes from the workflow and exercises them against a
boundary table (leading-zero/malformed rejected, valid accepted) plus structural
wiring assertions. Goes red if the regex is reverted to [0-9]+.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test: redesign weak tests into behavioral, deterministic assertions
Per the ADR test audit, rewrite 27 weak test files (test-only, no source
changes) so each can go red for the defect it guards:
- kill pass-always assert.ok(true) placeholders (research-cli, worktree-baseref,
bug-260 security guard, eslint-rules x24, clusters '|| true')
- replace source-text grep with behavioral calls (install Kilo, sh-hook-paths,
plan-review-convergence) and add a repo-layout governance test
- de-flake real-clock/Math.random coupling (phase last_updated, bug-3707 mtime,
context-utilization property, feat-3594)
- fix independence/shared-state violations (bug-492 singleton, issue-844 tmpRoot,
core reapStaleTempFiles, active-workstream TTY, feat-488 GSD_HOME)
- strengthen property/shape-only tests (research-provider/store classification +
collision) and unconditional plugin.json schema validation (issue-766)
Verified: all 28 files run together 1220 pass / 0 fail / 1 skip.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* chore: add no-tautological-assert lint rule, error in test suite
New custom ESLint rule (eslint-rules/no-tautological-assert.cjs) bans asserts
that can never fail: assert(true)/assert.ok(<always-truthy literal>),
'cond || true' inside an assert, and equality asserts comparing two identical
literals. Wired as error on tests/**; full sweep confirmed zero existing
violations so the suite stays green. Prevents the placeholder-assert regressions
the audit redesigns just removed. RuleTester coverage added (6 valid, 8 invalid).
Note: no-only-tests was already enforced via eslint-plugin-no-only-tests, so no
duplicate rule was added.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* chore: gate new allow-test-rule exemptions to require an issue ref
ADR-456 requires any allow-test-rule exemption added after the ADR to carry a
tracking issue number, but nothing enforced it. New ratchet gate
(scripts/lint-allow-test-rule-refs.cjs, wired into lint:ci) fails when a NEW
allow-test-rule comment lacks a #NNN/URL reference; the 323 existing untracked
exemptions are grandfathered in an allowlist that ratchets down as they gain
refs. Red-green verified (novel untracked offender fails; compliant passes).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* docs: add ADR test-audit evidence report (#1192)
Full risk-first qa-test-architect audit of the ADR portfolio (37 ADRs + 4
platform lenses, adversarial verification of retire verdicts) that drove the
P0 discovery fix, ADR-218 coverage, 5 retires, 27 redesigns, and the two new
lint gates. Filed as point-in-time evidence under docs/issueevidence/, named
for tracking issue #1192.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test: replace pre-existing raw NUL byte with escape in feat-3594 fixture
feat-3594's null-byte parser fixture contained a literal NUL byte (pre-existing
on next at
|
||
|
Tom Boucher
|
fd01e7a12e |
feat(#1132): complete contribution hook prerequisite
Closes #1132 |
||
|
Tom Boucher
|
4698b3e349 |
fix(#1051): force-exit + per-chunk timeout for the windows full-test lane; close leaked test handles (#1054)
The `full test (windows-latest, 22)` job intermittently got CANCELLED at its 20m wall-clock cap with no failed test step — a false-negative gate (recurrence of #869). Root cause: a unit test leaves an open event-loop handle, so the chunk's `node --test` child hangs ~150s on Windows after its last test prints; two such stalls push the already-~13m job past 20m. Fix (defense in depth): - run-tests.cjs: pass --test-force-exit (Node >=22; engines requires >=22.0.0) so the runner exits once all tests finish regardless of lingering handles — the durable backstop. Account for the flag in the argv-length ceiling. - run-tests.cjs: add a per-chunk execFileSync timeout (default 600000ms, env RUN_TESTS_CHUNK_TIMEOUT_MS) that fails loudly with a diagnostic naming the chunk's files, so a hung chunk can never silently eat the job budget. - perf-316 test: terminate both Worker threads on all paths (afterEach + finally) so they cannot outlive the test. - locking-bugs test: kill spawned children in a finally that wraps the whole spawn -> waitFor -> barrier-release -> Promise.all sequence, so a barrier timeout no longer leaks live child processes. - Refresh the stale synckit comment (synckit/SDK bridge was removed). Regression tests in run-tests-harness: a hung chunk hits the per-chunk timeout and fails with a clear message; force-exit lets a chunk with a leaked handle exit cleanly. Closes #1051 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Tom Boucher
|
a480510f54 |
fix(#872): make roadmap-phase-fallback tests hermetic against ambient GSD env (#873)
extractCurrentMilestone reads STATE.md via planningDir(cwd), which is workstream-aware (honours GSD_PROJECT/GSD_WORKSTREAM). The fixtures write STATE.md to the plain <tmp>/.planning/STATE.md, so a developer shell inside a GSD workstream (GSD_WORKSTREAM exported) redirected the read to a non-existent workstream subdir -> version=null -> closed milestone sections leaked into the slice and assertions failed. Clean CI/Docker env never hit it. Not a Node-26 regex bug; reproduces identically on any Node with GSD_WORKSTREAM set. - scripts/run-tests.cjs: strip GSD_PROJECT/GSD_WORKSTREAM before spawning test children so the local runner env matches clean CI/Docker. - tests/roadmap-phase-fallback.test.cjs: file-level beforeEach/afterEach save/delete/restore of both vars; new regression test pinning workstream-aware STATE.md resolution. - tests/run-tests-harness.test.cjs: guard asserting the runner strips both vars (so removing the deletion fails clean CI). Closes #872 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Tom Boucher
|
9dd2c87c6c |
ci: supersede #367 with combined tiered PR pipeline redesign (#369)
* ci: streamline PR pipeline gates * test: annotate ci-test-scope stderr assertions for lint rule --------- Co-authored-by: Colin <colin@solvely.net> |
||
|
Tom Boucher
|
52f23ac0a0 |
fix(3597): chunk node --test spawn to survive Windows CreateProcess limit
Windows CreateProcess caps lpCommandLine at 32,767 chars. The original `execFileSync(node, ['--test', ...546 paths])` exceeded that on every Windows runner and exited within ~70ms with no test output. Linux/macOS allow ~2 MB ARG_MAX so the same call worked there. `scripts/run-tests.cjs` now splits selected files into chunks that keep each spawn's argv under 28,000 chars (operator-overridable via RUN_TESTS_MAX_CMDLINE_CHARS), runs them sequentially, and reports the first non-zero exit. Cross-platform regression test forces chunking with a low ceiling and asserts the `run-tests: chunk N/M …` stderr marker. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> |
||
|
Tom Boucher
|
e82876fe45 |
feat(3597): split test suites and add Node 22/24/26 OS matrix
scripts/run-tests.cjs gains `--suite <name>` filtering using a filename
suffix convention (`*.security.test.cjs`, `*.integration.test.cjs`, …).
Files with no marker are `unit` (the default fast lane); files with a
marker land in the matching suite. No `--suite` flag preserves the prior
behavior of running every test (backcompat for `npm test` and
`npm run test:coverage`).
New package scripts wire the suites to stable entrypoints:
test:unit, test:integration, test:install, test:security, test:slow,
test:coverage:unit, test:coverage:all. Unknown suite → exit 2 with the
list of valid suites; empty suite → exit 0 with a stderr notice so empty
lanes (e.g. `security` before adversarial tests land) don't gate CI.
CI matrix grows from `ubuntu × {22,24}` + a single macOS lane to
`{ubuntu, macos, windows} × {22, 24, 26}`. `fail-fast: false` so one
lane failure doesn't cancel siblings. Node 26 is `continue-on-error`
until actions/setup-node stabilises that image. PR CI runs unit +
integration + security on every cell; `install` and `slow` only on
`main` push. A dedicated `coverage` job runs `test:coverage:unit` on
ubuntu/Node 24 and uploads the report.
Grouping policy lives in docs/TESTING-SUITES.md with a pointer from
CONTRIBUTING.md. New harness test covers arg parsing, filter selection,
empty-suite behavior, and failure propagation.
Closes #3597.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|