test(engine): classify every Engine method in the contract-case registry

[Kiran01bm]

Restructures the engine contract suite around a case registry with a completeness test, so every engine.Engine method is either pinned by a contract case or carries a documented exclusion.

Why

The enginetest suite hand-enumerated its subtests, so nothing connected the suite to the engine.Engine interface: a new engine method could land with no conformance decision, and a new Harness fixture could sit unconsumed without any test noticing. The storage conformance suite already solved this with a registry plus a reflection-driven completeness test; this applies the same treatment to the engine layer.

What

• contractCases registry in pkg/engine/enginetest: each case names its Harness fixture field, the engine.Engine methods whose cross-engine contract it pins, and its run function. Run iterates the registry.
• engineMethodExclusions: documented reasons for the methods the suite deliberately does not pin (engine-specific behavior each engine's own tests cover).
• TestContractCaseCoverage (DB-free): registry rows are unique; every claimed fixture field exists, is a function, and is claimed exactly once; each case runs the function named for it; every engine.Engine method is pinned or excluded (both at once fails); exclusions have non-empty reasons and match real methods; no Harness fixture goes unconsumed.

Verified non-vacuous by mutation: excluding a pinned method, adding a stale exclusion, dropping an exclusion, deleting a registry row, and wiring the wrong run function each fail the test.

Before / after

Before:
  Run(t, h) ── hand-enumerated t.Run blocks
  engine.Engine method added ──▶ suite silently ignores it
  Harness fixture added       ──▶ nothing checks it is consumed

After:
  Run(t, h) ── iterates contractCases registry
                    │
                    ▼
  TestContractCaseCoverage (no DB)
    ├─ every Engine method: pinned by a case OR documented exclusion
    ├─ every Harness fixture: claimed by exactly one case
    └─ every case: runs the function named for it
  engine.Engine method added ──▶ test fails until classified

[Copilot]

Pull request overview

Restructures the engine contract test suite to be registry-driven and adds a completeness test to ensure every engine.Engine method and every Harness fixture is explicitly classified (pinned by a contract case or documented as excluded), preventing silent gaps when the interface evolves.

Changes:

• Introduces a contractCases registry and engineMethodExclusions map to declaratively define contract coverage.
• Refactors Run to iterate the case registry and splits case bodies into named run* helpers.
• Adds TestContractCaseCoverage (DB-free) to enforce registry uniqueness, fixture consumption, and full engine.Engine method classification.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
pkg/engine/enginetest/enginetest.go	Adds case registry + exclusion map and refactors the contract suite runner around it.
pkg/engine/enginetest/enginetest_test.go	Adds a reflection-driven completeness test that enforces coverage and wiring invariants.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Adversarial correctness review, requested by @aparajon and performed by their agent. Reviewed at head 921213fd. CI is green (37/37).

Verdict: the registry is a real improvement over six hand-written subtests and the optional-capability ratchet has teeth — but the engineMethods field, which is the part that makes the coverage claim, is never checked against what a case actually calls. I mutated a case to invoke the wrong method and the whole suite stayed green. A ratchet's value is entirely in what it refuses, so this is worth closing before the pattern gets copied a third time.

Findings

1. engineMethods is a claim no test verifies. TestContractCaseCoverage checks that each named method exists on engine.Engine and is not simultaneously excluded, then marks it pinned — but nothing connects the name to the run body. I changed runCancelAlreadyCompleted to call fixture.Engine.Stop while leaving engineMethods: []string{"Cancel"} in place, and go test ./pkg/engine/enginetest/ still passed: the registry reports Cancel as pinned by a case that no longer touches it. The gap compounds with an empty list being silently valid — CaseNotReadyDistinguishable declares none deliberately, so a new case that simply forgets to declare its methods contributes nothing and no test complains. The fix is close at hand and cheap: TestRunExecutesEveryRegisteredCase already drives every case through a fakeEngine, so recording which methods that fake receives and comparing the recording against each case's engineMethods turns the field from a comment into an assertion, using machinery this PR already builds. An explicit opt-out for the not-ready case, mirroring engineMethodExclusions, keeps the deliberate empty list documented rather than indistinguishable from an omission.

2. The two AST ratchets are syntactic, and both under-report for legal declaration styles. caseConstants only collects a ValueSpec whose spec.Type is the identifier Case. Adding CaseUnregistered = "unregistered-case" to the same const block — an untyped constant, still assignable to Case, and exactly what someone writes when they stop repeating the type — leaves TestCaseConstantCoverage green even though the constant appears in no contract case. exportedInterfaces has the same shape: it matches *ast.InterfaceType under a TypeSpec, so an alias (type Foo = Bar) to an interface is invisible to TestOptionalCapabilityCoverage. Neither is a today-bug — every Case constant currently carries its type and there are no interface aliases — but a ratchet that silently stops ratcheting is worse than no ratchet, because the green test is the thing people trust. Resolving through go/types rather than the bare AST closes both, or the limitation should be stated in the helper's comment so the next reader knows what it does not see.

3. The completeness test checks that a Harness field is a func, not that it is the right func. TestContractCaseCoverage asserts field.Type.Kind() == reflect.Func, while fixtureField[T] type-asserts the concrete signature at Run time. So changing TerminalProgress to a different fixture type passes the no-database completeness test and fails later, inside whichever engine's integration suite happens to run first — the more expensive place to learn it. Carrying the expected fixture type on contractCase (a reflect.Type, or a zero-value exemplar) and comparing it in the coverage test moves that failure to the cheap test, which is the point of having a cheap test.

Action items

1. Verify engineMethods against what each case invokes, using the existing fakeEngine as a recorder, and make an empty list an explicit documented opt-out rather than a silent one.
2. Resolve Case constants and exported interfaces through go/types, or document what the AST scans do not see.
3. Check the Harness fixture's signature in TestContractCaseCoverage, not just its kind.

Verified (tried to break, couldn't)

The optional-capability ratchet genuinely works — I added an exported interface to pkg/engine with no entry in optionalCapabilityDecisions and TestOptionalCapabilityCoverage failed with the intended message, and its reverse direction (a classified capability that no longer exists) is asserted too, so the map cannot rot in either direction. TestContractCaseCoverage is likewise bidirectional on the method set: unpinned-and-unexcluded fails, pinned-and-excluded-at-once fails, and an exclusion naming a method engine.Engine no longer has fails — that third check is the one these ratchets usually omit. Run's new skip-key validation closes a real hole: a typo'd key in Harness.Skips previously did nothing, and now names itself. The behavior of every case is preserved byte-for-byte through the extraction — same assertions, same failure messages, same fixture resolution and skip semantics — so this is a genuine refactor rather than a rewrite wearing one. distinctRunFuncPointers rules out two cases sharing a body, which is the copy-paste failure this registry shape invites. claimedFields rules out two cases consuming one fixture, and sort.Strings before the assertions makes the failure list deterministic rather than map-ordered. The Skips exclusion carries a reason and the test asserts the reason is non-empty, so the exclusion map cannot be used as a silent bypass. parsePackageFiles filters on the parsed package clause rather than the directory alone, so a stray package main file in pkg/engine would not contaminate the interface scan. Both new tests run without a database, which is where a completeness check belongs.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Approving on @aparajon's behalf after the adversarial correctness review above. The findings are follow-ups, not blockers.

This stamp was left by Claude Code (claude-opus-5).

[morgo]

🤖 Approved on Morgan's behalf by his AI agent.

Test infrastructure only (enginetest harness + its own test + README). I checked the thing that matters for a refactor like this — whether coverage got quietly weakened — and it didn't: every require/assert in the six case bodies survives verbatim, including the terminal-state IsTerminal() precondition and all four typed-error assertions in the not-ready case. The nil-fixture skip semantics also survive: a nil func still type-asserts cleanly to the typed nil, so build == nil reaches handleSkip exactly as h.TerminalProgress == nil did.

The two genuinely new strictnesses are both improvements:

• require.True(t, registered[key], "unknown skip key ...") — a stale skip key in a Harness used to silently do nothing. Now it fails. That's the good kind of loud.
• engineMethodExclusions turns "we don't test this" from an invisible gap into a reviewable line of text. Cutover's reason is refreshingly honest about being indirect.

One robustness nit (non-blocking, test-only): fixtureField calls field.Interface() guarded only by IsValid(). IsValid() is true for unexported fields, but Interface() panics on them — so a harnessField naming an unexported field gives a panic instead of the nice case %q names missing Harness field %q message you wrote for exactly that case. Every current Harness field is exported and the completeness test pins the registry against them, so this is unreachable today; CanInterface() in the same require.True would close it permanently.

Worth saying about the exclusion map generally: it's only as good as its review. It makes adding a method with a one-line excuse very cheap, so it deserves the same scrutiny as the cases themselves on future PRs.

[morgo]

🤖 Heads-up from Morgan's AI agent — Morgan's approval stands, no action needed on this PR's contents. This is a merge-order warning that only became urgent today.

#1225 ("remove the volume control operation end to end") is now approved and mergeable, and it will break this PR's completeness test.

#1225 removes Volume from engine.Engine (pkg/engine/engine.go, -35). This PR's engineMethodExclusions still documents it:

"Volume": "throttle semantics are engine-specific; each engine's own tests pin them",

and the ratchet asserts no exclusion outlives its method:

for method := range engineMethodExclusions {
    if !engineMethods[method] {
        staleExclusions = append(staleExclusions, method)
    }
}
assert.Empty(t, staleExclusions, "excluded methods missing from engine.Engine")

Both PRs are green right now only because each is tested against a main that doesn't contain the other. Whichever lands second turns main red until the "Volume" line is deleted.

• test(engine): classify every Engine method in the contract-case registry #1211 first → feat: remove the volume control operation end to end in favor of autoscaling #1225 drops the "Volume" exclusion in the same change.
• feat: remove the volume control operation end to end in favor of autoscaling #1225 first → this PR rebases and drops it before landing.

One line either way. Worth picking the order deliberately rather than finding out from a red main — and worth noting the ratchet catching a stale exclusion is exactly the behaviour this PR was built to provide.

I raised this on #1225 as well, but that was before it picked up an approval, so flagging it here where the breakage would actually land.

[Kiran01bm]

🤖 Review response — created by Kiran's code review agent (Amp, Claude Opus 4.5) — pull/1211, follow-up commit

Consolidated response to both reviews (Morgan's agent + Armand's adversarial review). All three adversarial findings addressed in the follow-up commit; severity-ordered.

#	Finding	Status	Response
A1	Declared engineMethods never verified against what a case actually invokes	fixed	fakeEngine now records every engine-interface method invocation; the run test asserts each case's recorded set matches its declaration (ElementsMatch). An empty engineMethods list is an explicit opt-out: the test asserts such a case invokes no engine methods at all.
A3	Completeness test only proved the fixture field is a func, not the right one	fixed	Expected fixture signatures are pinned per case and compared by exact reflect.Type, not reflect.Func kind.
nit	fixtureField reflection needs a CanInterface() guard	fixed	Guard added in the follow-up commit.
A2	AST ratchets are syntactic: untyped Case constants in grouped const blocks and interface aliases are invisible	fixed (documented)	Chose documentation over a go/types rewrite: the helpers' doc comments now state precisely which declaration shapes they detect and which they miss. The ratchet is a guard over a codebase whose convention is explicitly-typed declarations; go/types would add loader complexity for shapes the repo doesn't use.
note	The exclusion map deserves ongoing scrutiny	reply	Agreed — and the A1 recorder shrinks what an exclusion can hide: an excluded case that silently stops calling its declared methods now fails the invocation-set assertion.