Skip to content

Validate a stage entry against what actually runs it - #175

Merged
VeryComplexAndLongName merged 1 commit into
mainfrom
feat/harness-config-strictness
Sep 2, 2026
Merged

Validate a stage entry against what actually runs it#175
VeryComplexAndLongName merged 1 commit into
mainfrom
feat/harness-config-strictness

Conversation

@VeryComplexAndLongName

Copy link
Copy Markdown
Owner

Implements harness-config-strictness (25/27; 7.5 human-only, 7.2 gated on CI — see below).

What it fixes

Three defects in assertValidStepAgents, all of the same family: a configuration that presents itself as effective and is not.

1. An agent that never runs had to be named. "apply": { "agent": "claude-cli", "dispatch": "vscode-chat" }agent is required and must be a known id, while chat dispatch spawns no process. Chat dispatch is now a step-runner id of its own (vscode-chat) and dispatch is retired. Old files are read and migrated, reported once, not refused — this repository's own config used the old shape.

2. model, effort and budget were silently ignored under chat dispatch. The validator compared each to the agent; claude-cli accepts all three, so the entry passed — while no argv was ever built for the flag to reach. "effort": "high" bought a run with no elevated effort and no message. Now refused, with a message saying the parameter cannot reach anything in that mode rather than that it is merely unaccepted — different problems for whoever reads it.

That was already forbidden here: ADR 0019 and harness-step-effort-and-budget task 2.2 both require refusing, never ignoring. The rule existed; the check compared the setting to the agent instead of to how the stage is delivered.

3. Unknown keys loaded silently. { "agent": "claude-cli", "modle": "claude-opus-5" } ran the default model. Now an error naming the key and the accepted set, at the top level and inside budget. Warning instead was rejected on this repository's own evidence — npm run lint was expected-red for days and a real error passed unread.

Every existing per-agent check is untouched: modelFlag, the accepted effort set, budgetField, Copilot's credit floor. Those were already thorough; this adds a dimension.

Review findings, fixed in this PR

Task 6.4's test could not fail. The task asked for an assertion against the real openspec/agent-harness.json rather than a fixture, because a schema change that breaks the workspace it ships in is what it guards. The test did this:

const workspaceRoot = path.resolve(here, "..", "..", "..", "..");

here is packages/core/src; four .. land one level above the repository root. No config exists there, so readGlobalHarnessConfig returned DEFAULT_HARNESS_CONFIG, whose autonomyLevel: "assisted" and reviewGate.mode: "human-required" are exactly the two values the test asserted. It passed without ever reading the file it names.

Now three .., with the file's existence asserted first (so a moved config fails loudly instead of falling back to defaults again) and stepAgents asserted non-empty — the part the defaults cannot produce.

openspec/agent-harness.json had also lost its trailing newline.

Test plan

  • openspec change validate --strict harness-config-strictness
  • npm run typecheck — exit 0
  • npm run lint and lint:english — exit 0
  • harness-config.test.ts — 62 tests, covering each refusal message, both unknown-key paths, both migration paths, and the real repository config
  • packages/core/src/agents/ untouched apart from registry.ts's comment (task 7.3)
  • Task 7.2 left unchecked. core's change-timeline.test.ts and sprint-report.test.ts fail on this machine with EBUSY: resource busy or locked, rmdir and 5-second timeouts while cleaning up git fixtures. Reproduced in isolation, unrelated to this change (it touches neither file nor anything either imports), and the subject of git-fixture-test-cost. harness-chain-runner.test.ts also failed under full load and passes alone (27/27). CI's Linux runner is the gate.
  • Task 7.5 is human-only: set a stage to vscode-chat, confirm the chat opens and the stage shows handed off; then add "effort": "high" and confirm the load is refused with a message naming why.

🤖 Generated with Claude Code

A stage's configuration named an agent and then a dispatch mode that
overrode it, so "apply": { "agent": "claude-cli", "dispatch":
"vscode-chat" } required naming an agent that never ran. Chat dispatch is
now a step-runner id of its own, vscode-chat, and the dispatch key is
retired - read and migrated on load, reported once, not refused, because
this repository's own configuration used the old shape.

model, effort and budget are refused when the stage is dispatched to
chat. They were accepted before: the validator compared each one to the
agent, and claude-cli accepts all three, so the entry passed while no
argv was ever built for the flag to reach. Setting "effort": "high" cost
a run that had no elevated effort and produced no message. The refusal
says the parameter cannot reach anything in that mode rather than merely
that it is unaccepted, because the two are different problems for whoever
reads it. A configuration that combined dispatch with one of them now
fails the load after migration, deliberately - those values never had an
effect.

An unrecognized key is an error naming the key and the accepted set, at
the top level and inside budget. "modle": "claude-opus-5" previously
loaded and ran the default model. Warning instead was rejected on this
repository's own evidence: npm run lint was expected-red for days and a
real error passed unread.

Every existing per-agent check is untouched - modelFlag, the accepted
effort set, budgetField, and Copilot's credit floor were already thorough.
This adds a dimension rather than revisiting them.

Reviewed before merge; two defects found and fixed here.

Task 6.4 asked for an assertion against the real
openspec/agent-harness.json rather than a fixture, because a schema
change that breaks the workspace it ships in is what the task guards. The
test resolved the workspace root with four ".." from packages/core/src,
which lands above the repository. No configuration exists there, so
readGlobalHarnessConfig returned DEFAULT_HARNESS_CONFIG, whose
autonomyLevel "assisted" and reviewGate.mode "human-required" are exactly
the two values the test asserted. It could not fail, and it never read
the file it named. Now three "..", with the file's existence asserted
first and stepAgents asserted non-empty - the part the defaults cannot
produce.

openspec/agent-harness.json had lost its trailing newline.

Implemented via the Agentic Harness (claude-cli, apply stage), 25/27.
Task 7.5 is human-only. Task 7.2 is left unchecked: core's
change-timeline and sprint-report tests fail on this machine with EBUSY
on rmdir and 5-second timeouts while cleaning up git fixtures,
reproducible in isolation, unrelated to this change, and the subject of
git-fixture-test-cost. harness-chain-runner also failed under full load
and passes alone. Typecheck and lint are green; CI is the gate.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@VeryComplexAndLongName
VeryComplexAndLongName merged commit 8a69ea0 into main Sep 2, 2026
7 checks passed
@VeryComplexAndLongName
VeryComplexAndLongName deleted the feat/harness-config-strictness branch September 2, 2026 12:05
VeryComplexAndLongName added a commit that referenced this pull request Sep 2, 2026
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>
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