fix(#63): eliminate the ~2s whole-repo TS-service build in AgentRunner tests (pre-validate false-BLOCK)#171
Open
agjs wants to merge 4 commits into
Open
fix(#63): eliminate the ~2s whole-repo TS-service build in AgentRunner tests (pre-validate false-BLOCK)#171agjs wants to merge 4 commits into
agjs wants to merge 4 commits into
Conversation
Follow-up to the panel's review of the global timeout bump (major/gate-relaxed,
agreement:1 across reviewers): a suite-wide 30s ceiling weakened fail-fast for
ALL 273 files, not just the load-sensitive integration tests, and could let an
unrelated regression that overshoots 5s pass.
Root cause (measured): every test in agent-runner.test.ts constructs
`new AgentRunner(...).run({ cwd: REPO })`, which pays a fixed ~2s startup cost
against the real repo — even the immediate-abort / no-op cases. In isolation the
file runs ~1.9s/test, under bun's 5000ms default; only under the full suite's
273-file concurrency does CPU contention inflate that startup past 5s, spuriously
timing out and false-BLOCKing the harness-review pre-validate gate.
Fix: setDefaultTimeout(15_000) at the top of agent-runner.test.ts — module-scoped,
so ONLY this file's ceiling rises. The 5s fail-fast default and its infinite-loop
detection stay intact for the other 272 files. 15s is generous margin over the
real ~2s work under contention while still failing a genuine hang here. Gate
stays FULL; not a relaxation.
…ly AgentRunner tests Follow-up to the panel's finding (major/gate-relaxed, agreement:1): file-scoped setDefaultTimeout(15s) also raised the ceiling for the parseAgentSpec and loadAgentSpecs tests, which do NOT construct AgentRunner — over-broadening the relaxation and contradicting the comment's 'every test here' claim. Move those pure config-spec tests to a new agent-specs.test.ts (they belong there anyway — fast, hermetic, no repo). agent-runner.test.ts now contains ONLY AgentRunner-against-repo tests, so setDefaultTimeout(15_000) is precisely scoped to exactly the load-sensitive tests; every other file (including the moved config tests) keeps bun's 5s fail-fast default. Comment corrected to match. 14 tests unchanged (11 AgentRunner + 3 config), all green; typecheck/lint/format clean.
… service) — no timeout change Panel BLOCK on the scoped timeout (major/gate-relaxed): a reviewer held that ANY raised ceiling lets a 5-15s regression in these tests pass — a relaxation, even scoped. They're right that the timeout was treating a symptom. Root cause (found): AgentRunner.run calls buildTsService(cwd) when no tsService is passed. With cwd: REPO that builds a TypeScript service over the ENTIRE tsforge repo — ~2s PER test — even for immediate-abort / no-op cases. That fixed cost, not the loop work, blew bun's 5s default under full-suite CPU contention. Fix at the root: pass tsService: null in every run() here (as many sibling tests already do). These read-only tests exercise tool-gating / events / maxTurns / abort / policy — none use the type-aware tools — so a null service is faithful. Result: the file drops from ~24.6s to ~5.3s (11 tests), every test well under the 5s fail-fast default. setDefaultTimeout REMOVED entirely — NO timeout change, so nothing is relaxed; fail-fast + infinite-loop detection are fully intact. (Pure config tests remain split into agent-specs.test.ts from the prior step.)
…all missed + fix stale comment
Panel caught (agreement:1, major): the two structured-mode tests use the
single-line .run({ provider, cwd: REPO, ... }) form, which my cwd:REPO-line
replace_all didn't match — so they still called buildTsService(REPO) (~2s) and
the 'every run passes tsService: null' comment was false. Add tsService: null to
both; now EVERY run() is stubbed (file ~24.6s -> ~0.4s). Also fix the stale
agent-specs.test.ts header comment that referenced a 'raised per-file timeout'
(removed in this PR). All 11 AgentRunner + 3 config tests green; typecheck/lint/format clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The
harness-reviewpre-review gate runs the fullbun run validateand BLOCKs review on any non-zero exit. Under full-suite concurrency itsbun test packagesphase spuriously timed out, false-BLOCKing every review — even a twice-PASSed diff (observed on PR #170's branch: 9 timeout "fails" blocked a clean change). This is issue #63.Root cause
AgentRunner.runcallsbuildTsService(cwd)when notsServiceis passed.agent-runner.test.tsruns the agent against the whole tsforge repo (cwd: REPO), so every test built a TypeScript service over the entire project — ~2s per test, even the immediate-abort / no-op cases. In isolation the file ran ~24.6s (~1.8s/test), under bun's 5000ms default; under 273-file CPU contention that startup inflated past 5s → spurious timeouts.Fix (at the root — no gate change)
tsService: nullon everyrun()inagent-runner.test.ts(as many sibling tests already do). These read-only tests exercise tool-gating / events / maxTurns / abort / policy / structured-mode — none use the type-aware tools (type_at/diagnostics) — so a null service is faithful and removes the cost at the source. File drops ~24.6s → ~0.4s; every test is far under the 5s default.parseAgentSpec/loadAgentSpecsconfig tests intoagent-specs.test.ts(they don't constructAgentRunner; they belong in their own fast file).No timeout was raised —
setDefaultTimeoutis not used. Nothing is relaxed; bun's 5s fail-fast default and its infinite-loop detection stay intact for every file. The fullbun run validategate is unchanged.Review
--timeoutbump, then a file-scoped one) and caught an incompletereplace_allthat left two structured-mode runs unstubbed — all fixed here.