Skip to content

fix(tests): give the oversized-command test a budget above its own 15000ms assertion - #25

Merged
andrei-hasna merged 1 commit into
mainfrom
d8d59fd6
Aug 8, 2026
Merged

fix(tests): give the oversized-command test a budget above its own 15000ms assertion#25
andrei-hasna merged 1 commit into
mainfrom
d8d59fd6

Conversation

@andrei-hasna

@andrei-hasna andrei-hasna commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

The defect

hooks/codewith-native-common.test.ts:1774"a command too large to tokenize is decided, not left to the timeout" — asserts an internal budget on line 1781:

expect(performance.now() - started).toBeLessThan(15000);

but declared no per-test timeout, so bun's 5000ms default killed it before its own
assertion could run. The file carried zero per-test timeout arguments, against a control
of 124 test( declarations, and the repo has no bunfig.toml and no --timeout in its
test script — so the 5000ms default applied everywhere.

The consequence is wider than one red test: it is the only failure in the suite, so it made
@hasna/hooks fail base_verify.

Reproduction — same worktree, same sha, only the budget differs

Measured on station01 at load ~28-32, bun 1.3.14, base c47fd8b:

command result
bun test hooks/codewith-native-common.test.ts 123 pass, 1 fail, rc=1 — this test timed out after 5000ms
bun test hooks/codewith-native-common.test.ts --timeout 60000 124 pass, 0 fail, rc=0

The test's real cost, isolated (-t filter) against an empty-filter baseline that runs 0
tests, three repeats:

run 1: TARGET 11.243s | BASELINE 0.247s | DELTA 10.996s
run 2: TARGET 10.304s | BASELINE 0.286s | DELTA 10.018s
run 3: TARGET 10.289s | BASELINE 0.369s | DELTA  9.919s

The fix, and why the number is 30000

A per-test timeout on that one test. The suite-wide default is not raised, the 280 MB
input is not reduced, and the assertion is not weakened — the test is checking that
an oversized command is decided rather than left to the timeout, which is a real property
of the guard.

The budget has to sit strictly above the 15000ms assertion, because that assertion is the
actual gate. Set it near the measured ~10.5s cost and a genuine slowdown gets reported as an
infrastructure timeout instead of as the regression it is.

Proof the fix discriminates

Every run below at the default command, no flags, so nothing here depends on a flag the
CI does not pass. Implementation restored byte-identical to origin/main after each mutant
(sha 4cf8718... both sides).

mutant outcome
correctness broken (block: false on the oversized path) fails at :1779Expected: true, Received: false, rc=1
+8s injected slowdown fails at :1781Expected: < 15000, Received: 16503.348659, rc=1
+50s injected hang backstop firesthis test timed out after 30000ms, rc=1

The middle row is the one that matters: the assertion speaks, not the runner. The bottom
row shows the 30000 backstop is live rather than decorative.

Worth noting from the first row: it ran 12819ms and still reported the real assertion
failure. Under the old 5000ms default that same regression would have been killed at 5s and
reported as a timeout, hiding its cause.

Gates — the repo's own commands

gate before after
bun run test 1078 pass, 1 fail, rc=1, 21 files 1079 pass, 0 fail, rc=0, 21 files
bun run typecheck rc=0, 0 error TS#### (pattern control fires on a synthetic error)

Full-suite runtime 157-167s. The Failed to read settings ... JSON Parse error lines in
src/lib/installer.test.ts are that suite's own negative-path fixtures in a temp home, not
failures.

Notes

  • Base c47fd8b. The shared checkout at /home/hasna/workspace/hasna/opensource/open-hooks
    is at d8c0e8a (2026-07-26) and was not used for any measurement; every read was taken at
    the ref or in this worktree, whose copy of the file hashes identical to the origin/main
    blob.
  • The operator's real ~/.claude/settings.json was hashed before and after every run and is
    unchanged — relevant because task b18511f4 records this suite writing to it.

Task: d8d59fd6

Agent: Silvanus


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

…sertion

The test "a command too large to tokenize is decided, not left to the timeout"
asserts `performance.now() - started < 15000` but declared no per-test timeout,
so bun's 5000ms default killed it at 5s and the assertion never ran. The whole
file carried zero per-test timeout arguments across 124 test declarations.

Measured on station01 at load ~30, same worktree and sha both ways:

  bun run test  (before)   1078 pass, 1 fail, rc=1, "timed out after 5000ms"
  bun run test  (after)    1079 pass, 0 fail, rc=0

The test's real cost is ~10.0-11.0s (three isolated repeats: 11.24s / 10.30s /
10.29s, against a 0.25-0.37s empty-filter baseline). The 280 MB input is the
point of the test, so it is not reduced and the assertion is not weakened.

The budget is 30000 rather than something nearer the measured cost because the
15000ms assertion is the actual gate and has to stay reachable. Verified by
mutation, all at the default command with no flags:

  correctness broken   -> fails at :1779 "Expected: true, Received: false"
  +8s slowdown         -> fails at :1781 "Expected: < 15000, Received: 16503.35"
  +50s hang            -> backstop fires, "timed out after 30000ms"

So the assertion speaks for a real regression and the runner budget only catches
a true hang. The suite-wide default is untouched.

Agent: Silvanus
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #25 @ 930a3d9 — lens: correctness+security+gates, reviewer Aruns (1 of 1)

Reviewed exact candidate 930a3d9 against freshly fetched origin/main c47fd8b.

What I read:

  • git log --oneline origin/main..HEAD and git diff origin/main...HEAD --stat.
  • The full diff of hooks/codewith-native-common.test.ts.
  • Surrounding test setup and helpers, the changed oversized-command regression, MAX_ANALYSABLE_COMMAND_LENGTH, the oversized-command branch in classifyDangerousOperation, and the declared package scripts.

What I ran:

  • bun install — exit 0. Setup only; 177 packages installed. This is not a test gate.
  • bun run typecheck — exit 0. The gate emitted no pass/fail counts.
  • bun run test — exit 0. 1079 pass, 0 fail, 3803 expect() calls, 21 files. The changed test reported: (pass) destructive shell guard - rm -rf /* incident regression > a command too large to tokenize is decided, not left to the timeout [3556.89ms].
  • git diff --check origin/main...HEAD — exit 0, no output.

Blocking P0/P1 findings: none.

The change preserves the test's real <15000ms assertion and moves only Bun's runner backstop from the 5000ms default to 30000ms, so the assertion remains capable of failing before the runner timeout. The exact gate passed with the changed test well below both thresholds.

Non-blocking follow-ups: none.

@andrei-hasna
andrei-hasna merged commit d1e0d39 into main Aug 8, 2026
2 checks passed
@andrei-hasna
andrei-hasna deleted the d8d59fd6 branch August 8, 2026 11:47
@andrei-hasna

Copy link
Copy Markdown
Contributor Author

[REVIEW] GO — #25 @ 930a3d9 — lens: correctness+performance, reviewer Seneca (1 of 1)

Independent adversarial review. All runs performed in a fresh worktree at
~/.hasna/repos/worktrees/open-hooks/review-pr25, base c47fd8b (= current origin/main, so
no base-moved staleness applies), bun 1.3.14, station01 at loadavg 26–46 during the runs.
Capture path: every command redirected to files, exit codes read directly.

1. Both arms, run myself

Target file, PR head 930a3d9:

 124 pass
 0 fail
 300 expect() calls
Ran 124 tests across 1 file. [33.03s]

rc=0.

Target file, base c47fd8b (no change):

(fail) destructive shell guard - rm -rf /* incident regression > a command too large to tokenize is decided, not left to the timeout [5774.21ms]
  ^ this test timed out after 5000ms.
 123 pass
 1 fail

rc=1.

Full suite (bun run test), PR head, deps installed:

 1079 pass
 0 fail
 4061 expect() calls
Ran 1079 tests across 21 files. [167.07s]

rc=0. Full suite, base:

 1077 pass
 2 fail
Ran 1079 tests across 21 files. [155.95s]

rc=1 — the target test (timed out after 5000ms) plus one unrelated pre-existing flake (see
follow-ups). Environment note: my first full-suite run showed 100 fails because I had not run
bun install in the fresh worktree (Cannot find package 'pg'); after install the numbers
above reproduced the PR body exactly. Recorded so nobody re-litigates that artefact.

2. Remedy vs mask — measured, not reasoned

The test was BORN unable to pass the runner default. At 53505e2 — the commit that added it,
2026-07-27 08:53 — the same test already dies at the default:

  ^ this test timed out after 5000ms.
 0 pass
 1 fail
Ran 1 test across 1 file. [6.46s]

Same test with --timeout 60000, same load window, at the birth commit vs current main
(the implementation changed once after birth, at cc24af0):

53505e2 (birth impl):  Ran 1 test across 1 file. [10.92s] / [11.27s]
c47fd8b (current impl): Ran 1 test across 1 file. [10.55s] / [10.15s]

No performance regression — the current implementation is marginally faster than the one the
test was written against. The internal expect(performance.now() - started).toBeLessThan(15000)
is the real gate; the runner budget merely lets execution reach it. The change hides nothing.

I also verified the discrimination claim with my own mutant rather than trusting the PR body:
+4s injected per classifyDangerousOperation call (+8s for the test's two calls), default
flags, on the PR head:

Expected: < 15000
Received: 16757.082674999998
(fail) ... a command too large to tokenize is decided, not left to the timeout [18187.66ms]

rc=1 — the ASSERTION speaks at 18.2s wall, inside the 30000 budget. A genuine slowdown is
reported as the regression it is, not as an infrastructure timeout. Mutant reverted
(git checkout --, git status --short clean).

3. Budget value

Measured wall time of the test in isolation on the PR head, three runs at loadavg 30–46:
[12.31s] [13.21s] [13.51s]. Against 30000ms that is ~2.2–2.4x headroom on a heavily
contended box, and the value sits 2x above the 15000 internal assertion — the ordering that
matters, since any run between 15s and 30s fails on the assertion's message (proven above)
and only a true hang reaches the backstop. Defensible.

4. Blast radius

git diff c47fd8b..930a3d9 --numstat
7	1	hooks/codewith-native-common.test.ts

One file, one test: the , 30000 timeout argument plus a 6-line comment. No implementation
code, no suite-wide default, no other test touched. The internal assertion is unweakened.
~/.claude/settings.json sha256 identical before and after all runs
(b62c80d5...a648ee28c both sides).

Non-blocking follow-ups (not this PR's defects)

  • Pre-existing flake, same class: on my base full-suite run,
    (fail) CLI > hooks install + doctor + remove E2E > full lifecycle via JSON [5000.88ms] — timed out after 5000ms
    — an unrelated test that can exceed the 5000ms default under station load (it passed on the
    head run). The suite still has other tests sitting near the default budget; worth a sweep.
  • The new comment embeds measured numbers ("~10.0-11.0s", "load ~30") that will rot with
    hardware and load; the load-bearing sentence (budget strictly above the assertion) is
    durable, so this is cosmetic.

Verdict: GO. The failing arm reproduces, the fix discriminates in all three directions, no
performance regression is being masked, and the blast radius is exactly the one test.

Agent: Seneca

andrei-hasna added a commit that referenced this pull request Aug 8, 2026
…default (#27)

Give the two unbudgeted tests explicit per-test timeouts, and record the rule that produced them.

`full lifecycle via JSON` and `a long relative cd chain` declared no per-test budget and inherited
bun's 5000ms default. Both are intrinsically expensive: an independent reviewer measured the
lifecycle test at up to 6.5s isolated, above the default at ordinary load.

Budgets are 20000ms each, sized per test rather than to a common multiple. The cd-chain value is
set by intent, not by observed cost: its own three `toBeLessThan(3000)` assertions permit 9000ms,
so any runner budget at or below that would kill a run the assertions consider passing — the same
defect #25 fixed.

The suite default is untouched, so the 5000ms backstop keeps working as a regression signal for
the 1038 tests that finish under a second.

The contributor rule has two prongs because one would have missed a motivating case: a test that
spawns a subprocess more than twice, OR asserts on its own elapsed time, declares an explicit
budget. The cd-chain test spawns nothing and is caught only by the second prong.

Reviewed at 754acbc with both arms run independently, the override mechanism proven directly at
`--timeout 1000`, and a negative control at `--timeout 200` confirming the flag kills budget-less
tests.

Agent: Silvanus
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant