Skip to content

Stop the git fixtures from making the local suite red - #178

Merged
VeryComplexAndLongName merged 1 commit into
mainfrom
fix/git-fixture-test-cost
Sep 2, 2026
Merged

Stop the git fixtures from making the local suite red#178
VeryComplexAndLongName merged 1 commit into
mainfrom
fix/git-fixture-test-cost

Conversation

@VeryComplexAndLongName

Copy link
Copy Markdown
Owner

Implements git-fixture-test-cost (18/19) and proposes the one follow-up its verification uncovered.

Result

Full npm run test on this machine, before → after:

Failing files Failing tests
Before 3 8
After 1 1

change-timeline.test.ts and sprint-report.test.ts now pass inside the full run, not only alone — which is the distinction that matters, since they always passed alone.

What made them expensive

initRepo was three processes (init plus two addConfig), and every commit two more. The identity now travels in the env the commit helpers already set, so a repository costs one spawn instead of three. Tests that only read an unchanging repository build it once and share it, with the no-mutation rule stated at the fixture's construction rather than left to be inferred.

Cleanup moved from afterEach to afterAll, and that is what ends the EBUSY: resource busy or locked, rmdir failures — the git processes holding those directories have exited by then.

Both files carry their measured before and after at the top, and each sets its own per-test ceiling sized against the reduced cost. Not the global default: that would hide the same problem in every suite, including ones with no excuse for it.

No assertion moved in either file. Speed here had to come from spawning fewer git processes, never from mocking the git being measured — otherwise the change deletes the coverage it exists to protect.

Seven active task lists carried a note calling these two files "pre-existing Windows timeout flakes … do not attempt to fix them here". That condition is over, so the notes are gone. Archived changes are untouched — their notes were true when written.

Task 6.3 stays open, and why

What still fails under co-load is a different file for a different reason:

FAIL src/harness-chain-runner.test.ts > semi-autonomous
     > confirming a checkpoint resumes into the next stage's agent
AssertionError: expected { kind: 'started', …(4) } to match object { kind: 'completed' }

vi.waitFor defaults to 1000 ms, and that test drives four chain stages through real file writes before the completed event it waits for. No git fixture is involved. Fixing it here would have reached past the two files this change names — which its own task 2.4 explicitly forbids — so it is proposed separately.

load-sensitive-test-timeouts (proposal, included here)

Worth closing rather than living with, for a reason beyond the one test: the failure looks like a product bug and is not. The chain does emit ["plan", "review", "implement", "verify"]; the test simply stops watching too early. A red suite that is actually noise is how this repository lost an eslint error for days, and a suite green only on an idle machine cannot be the gate operations.apply.guidance requires before a commit.

Also closes harness-config-strictness task 7.2, left open pending CI, which went green on #175.

Test plan

  • Both target files pass alone
  • Both target files pass inside the full npm run test
  • openspec change validate --strictgit-fixture-test-cost and load-sensitive-test-timeouts both valid
  • npm run lint:english passed
  • git diff on the two test files shows setup and timeouts only; no expect changed
  • No changeset — test-only, matching archive/2026-09-01-ci-job-timeouts/
  • CI green

🤖 Generated with Claude Code

The full npm run test on this machine went from eight failures in three
files to one failure in one file. change-timeline.test.ts and
sprint-report.test.ts now pass inside the full run, not only alone, which
is the claim this change makes.

The cost came from spawning git. initRepo was three processes, init plus
two addConfig, and every commit two more. The identity now travels in the
env the commit helpers already set, so a repository costs one spawn
instead of three, and the tests that only read an unchanging repository
build it once and share it - each such fixture stating at construction
that nobody may mutate it. Cleanup moved from afterEach to afterAll,
which is what ends the EBUSY: resource busy or locked, rmdir failures:
the git processes holding those directories have exited by then.

Both files carry their measured before and after in a comment at the top,
and each sets its own per-test ceiling sized against the reduced cost -
never the global default, which would hide the same problem in every
suite that has no excuse for it.

No assertion moved in either file. Speed here had to come from spawning
fewer processes, never from mocking the git being measured, or the change
would have deleted the coverage it was meant to protect.

Seven active task lists carried a note calling these two files
pre-existing Windows flakes and telling the reader not to fix them. That
condition is over, so the notes are gone.

Task 6.3 stays open, and the review records why. What still fails under
co-load is harness-chain-runner.test.ts, for a different reason: vi.waitFor
defaults to one second, and that test drives four chain stages through
real file writes before the event it waits for. No git fixture is
involved. Fixing it here would have reached past the two files this
change names, which its own task 2.4 forbids, so it is proposed
separately as load-sensitive-test-timeouts.

That proposal is included here. The failure it removes looks like a
product bug and is not - the chain does emit the sequence the test
asserts, the test just stops watching too early. A red suite that is
actually noise is how this repository lost an eslint error for days, and
a suite green only on an idle machine cannot be the gate before a commit.

Also closes harness-config-strictness task 7.2, which was left open
pending CI and went green on #175.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@VeryComplexAndLongName
VeryComplexAndLongName merged commit 30e19ad into main Sep 2, 2026
7 checks passed
@VeryComplexAndLongName
VeryComplexAndLongName deleted the fix/git-fixture-test-cost branch September 2, 2026 13:34
VeryComplexAndLongName added a commit that referenced this pull request Sep 2, 2026
…in them (#179)

* Add the git stage and mechanical verify checks, and fix four defects in them

Three changes land together because they were implemented into one
working tree and touch the same files: load-sensitive-test-timeouts,
harness-mechanical-checks, and agentic-harness-git-stage.

The first is clean. It sizes each multi-step vi.waitFor against measured
cost - about 453 ms isolated and 1823 ms under deliberate co-load, with a
5000 ms ceiling - and leaves the single-event waits alone, since an
unnecessary ceiling is noise and noise is what makes the necessary ones
unreadable. The full local suite is now green for the first time this
week. It also removed two .tmp log files the run had left in the
repository root; evidence belongs beside the number it justifies.

The other two arrived reported as finished and were not.

npm run typecheck failed with nine errors, none of them in core.
Narrowing HarnessStepAgents to exclude archive was right - the stage is
mechanical and an entry there configures nothing - but every consumer
holding a plain HarnessStage still indexed the record, and core compiling
while the other three packages did not is exactly what a green vitest run
cannot tell you. Fixed with isHarnessStepAgentStage and stepAgentFor in
the type's own module: keeping the archive case in one place is what stops
the removal from becoming six subtly different guards. The settings view
keeps archive in the stage list, as its own task 4.4 requires, and renders
it as a row saying it runs mechanically rather than offering controls the
validator would reject.

The git stage carried three defects, all of them behind task 4.4, the
live smoke test, the only task left unchecked. That was not a coincidence
and the check was not decoration.

gh pr create does not accept --json. The real binary answers "unknown
flag: --json" and exits non-zero, so createPullRequest threw every time
and the stage could never open a pull request. It now reads the URL gh
prints, taking the last line that is one, and treats no URL as an error
rather than a guess.

Every check state outside two hard-coded sets counted as a failure, and
SKIPPED was outside them. Two of the seven checks on this repository's own
PR #178 are SKIPPED - conditional jobs that skip on every pull request by
design - so the gate would have refused every pull request this repository
can produce. Skipped, neutral and stale are now their own set: ran and
decided nothing. A case the original had no answer for is now decided
explicitly - every check skipped is a refusal, not a pass, because nothing
exercised the change, and ADR 0014 already treats an absent result that
way.

The allowlist gated an invocation that was not the one executed. The
stage built git push <remote> <branch>, checked and audited that, then
called a bare git push, which resolves both from the branch's upstream -
possibly a different target, and absent entirely on a branch that has
never been pushed. GitWrapper.push now takes its remote and branch, and no
--set-upstream either, since an extra flag would reopen the same gap from
the other side. The integration test no longer establishes an upstream
first: it pushes a branch that has none, which is what the stage faces.

Also corrected the changeset, which named only core at patch, for the git
stage alone, while both changes moved the public surface of three
packages.

Two tasks stay open by intent. harness-mechanical-checks 6.3 wants every
existing tasks.md asserted against the real openspec/changes/ tree rather
than a fixture, which is not done and is worth doing - the parser is the
one change here whose blast radius is every change in the repository.
And 6.6 and the git stage's 4.4 are human-only.

Typecheck, lint and test green across all four workspaces.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* Test the path escape with a separator both platforms read

`path-unchanged`'s escape test used `..\outside-the-workspace`. A
backslash is a separator only on Windows; on Linux it is an ordinary
filename character, so the path stayed inside the workspace, nothing was
refused, and the assertion that nothing spawned failed on the one runner
that could see it. The implementation was always correct - `path.resolve`
plus `checkCwdSandbox` care about neither platform's spelling.

Now `../outside-the-workspace`, which escapes on both, plus a second case
for a rooted path. `/etc/passwd` was picked over a drive-letter path
because `path.resolve` leaves it alone on POSIX and rewrites it to
`<drive>:\etc\passwd` on Windows, and both land outside a workspace under
a temporary directory. A `C:\...` fixture would have reproduced the
original mistake with the platforms exchanged: an ordinary relative name
on Linux, inside the workspace, legitimately reaching `git diff`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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