feat: add test orchestration

[ianrayianray]

Summary

This PR completes roadmap item 6: Test orchestration.

• Adds bcdev_test_orchestrate.
• Repeats one selected test plan 2–20 times through the existing TestRunnerHub client.
• Retains every enriched attempt, including raw failure evidence and a final aborted attempt when later setup or authorization fails.
• Reports exact additions and removals between adjacent passed and failed sets.
• Classifies each test method as stablePassed, stableFailed, stableSkipped, flaky, inconsistent, or incomplete.
• Uses flaky only when the same method has both passing and failing evidence.
• Returns complete: false when an observation is missing, duplicated, ambiguous, or aborted.
• Stops after an aborted attempt because server-side cancellation cannot be confirmed. Later requested observations become missing.
• Reports runsAttempted separately from runsRequested.
• Holds the shared singleton test-run slot for the full repeat sequence.
• Returns result-aware nextSteps for passed, failed, unstable, and incomplete outcomes.

Orchestration does not aggregate coverage. Use bcdev_test_run for coverage and changed-procedure analysis.

Compatibility and safety

• Existing test-run payloads and raw output remain unchanged inside each retained attempt.
• Failure call stacks keep their raw lines and use best-effort local source mapping.
• The tool uses the existing connection, authorization, feature preflight, and TestRunnerHub client.
• One orchestration excludes direct, debug-bound, native-runtime, and other orchestrated test runs until it finishes.
• Client cancellation is observed between attempts. It does not claim to cancel an active Business Central run.
• Overlapping codeunit/method selections are rejected; disjoint method groups for one codeunit remain valid.
• AL identifier casing uses the same length-preserving normalization as procedure identity analysis.
• An all-skipped selection keeps the existing passed convention and emits a warning that no passing execution evidence exists.
• Test suites with disabled isolation or external effects can repeat those effects on every attempt.

Validation

• bun test: 641 passed, 0 failed
• bun run typecheck: passed
• bun run build: passed
• Embedded-skill drift check: passed

SaaS Sandbox

The live Sandbox run covered:

• Default three-run stable pass
• Deterministic stable failure with retained and parsed failure evidence
• Mixed pass/fail and whole-codeunit selections
• Case-insensitive method matching
• Two-codeunit sequential plans
• The maximum 20-run bound with 19 empty adjacent diffs
• Missing method and missing codeunit fail-closed behavior
• A real aborted attempt that stopped the sequence and marked the later observation missing
• Live contention from a direct test run and a second orchestration
• Lock release and a successful recovery run

The expanded live harness completed 399 checks across 14 scenarios. No deliberately flaky or skipped Sandbox fixture was added. Deterministic tests cover those classifications, mid-sequence authorization rejection, between-attempt cancellation, overlapping selections, bounded ambiguity warnings, analyzer preconditions, all-skipped guidance, and debug-bound/native-runtime contention.

Redacted evidence is in scripts/e2e-test-orchestration-2026-07-29.md. No Production or on-premises call was made.

Merge order

Merge this PR before #13. Afterward, #13 should be rebased onto main so the combined tool count and test badge are updated once against the merged tree.

[SShadowS]

Review

Reviewed on a clean worktree of 2b93b3c, plus a second independent pass from another model. Verified locally: bun install --frozen-lockfile clean, bun run typecheck clean, bun test 636 pass / 0 fail across 45 files, bun run build clean with zero embed-skills drift. Docs are internally consistent (tool count 22 to 23, test badge 622 to 636, roadmap item 6 marked shipped, e2e.md checklist updated).

The analyzer is genuinely well built and fails closed where it should: complete: false on missing/duplicate/aborted, flaky only on positive pass-and-fail evidence, coverage deliberately not aggregated. Independently checked and cleared: observation padding to runsRequested, flaky-beats-incomplete precedence, diff loop bounded by actual runs so no phantom diffs, claimTestRun before try so a rejected concurrent claim cannot clobber the owner's lock, enrichTestRun always setting summary so the required field holds for aborted runs too, core purity, Node 18 compat, no new deps, no wire changes.

Findings below.

---

1. Blocking: mid-sequence rejection discards all retained evidence

src/mcp/tools/test-tools.ts:308-318

The run loop has no per-iteration try/catch. The abort path is safe, because TestRunnerClient.run() resolves rather than throws (src/core/hubs/test-runner-hub.ts:134-142 funnels start/Initialize/RunTests failures into finish(reason)). But await authorization.getAuthorizationHeader() at test-runner-hub.ts:36 sits outside that never-reject promise and can reject.

Concrete trigger: AzureCliAuthorizationProvider.getAuthorizationHeader re-runs az account get-access-token once the cached token is inside REFRESH_WINDOW_MS (src/core/authorization.ts:38, :100-108), a 5 minute window. A long orchestration (up to 20 sequential runs) crossing that boundary with a transient az failure on run N throws out of the loop and discards runs 1..N-1, including parsed failure evidence. Only the JSON error body comes back.

That contradicts the tool's headline claim ("retain every enriched run", also in the README section and the PR description), and SaaS Sandbox is exactly the environment the live E2E ran against. On prem basic auth cannot reject, so this is a cloud only failure. mapOrchestrationSources and analyzeTestOrchestration after the loop share the discard-on-throw exposure, though realistically only auth rejects (enrichTestRun is pure string parsing, the index build is already caught).

Fix is small and matches the abort semantics already in the design: catch per iteration, synthesize a final { runAborted: true, abortReason } attempt, break. The analyzer already handles runs.length < runsRequested correctly, producing missing observations plus an "Only N of M" warning.

Test gap: nothing in tests/mcp/test-orchestration-tools.test.ts pins mid-loop-throw behaviour. A case with a throwing authorizationFactory on the second call would pin it.

2. No deadline, cancellation, or progress across up to 20 sequential runs

src/mcp/tools/test-tools.ts:300-328, src/mcp/server.ts:95

The singleton test slot is held for the whole sequence, so bcdev_test_run, bcdev_debug_run_tests, native runtime, and a second orchestration all return TEST_RUN_ACTIVE for the duration. The registerTool callback is async (params) => ..., so the SDK's extra (carrying signal) is dropped, and there are no progress notifications anywhere. A client cancellation or request timeout kills the response while the loop keeps running and holds the slot to completion: the caller ends up locked out and blind, with no partial result.

To be fair, this is an amplification of a gap every tool has rather than a regression introduced here. bcdev_test_run has the identical trait. Twenty sequential AL suite runs is what makes it matter.

Cheapest meaningful fix is plumbing an AbortSignal into ToolDefinition.handler and checking it between iterations, which also releases the slot early on cancel. An explicit doc caveat would be the minimum.

3. upperInvariantUtf16 duplicated verbatim

src/core/test-orchestration.ts:87-95 is byte identical to src/core/al-procedures.ts:988-996. The al-procedures copy is module private, which explains the copy.

The comment above the new copy says the transformation matches "the compiler-facing normalization used by the procedure identity engine", which is an invariant a silent copy breaks. Scoping it honestly: orchestration keys never mix with the procedure-identity engine's keys, so drift would produce inconsistent normalization between two features, not corrupted shared identities. Real maintainability defect (a comment asserting a sync the code does not enforce), not a correctness bug. Export one copy.

4. Duplicate codeunit coverage in the plan is not rejected

src/mcp/tools/shared.ts:286-295

codeunitsShape has no uniqueness constraint. The PR's own live evidence records the consequence: a duplicate group produced one retained pass, then an aborted attempt and a missing later observation. It does fail closed (retained partial run, incomplete outcome, warning), so this is ergonomics rather than correctness, but it is cheaper to reject at the boundary than to explain the abort afterwards.

Two cautions for whoever fixes it. Naive unique id validation would reject the legitimate split-group form [{ id: X, methods: ["A"] }, { id: X, methods: ["B"] }]; the correct constraint is duplicate (id, method) coverage. And the shape is shared with bcdev_test_run, so an orchestration-only refine forks it.

5. destructiveHint: false copied without its caveat

src/mcp/tools/test-tools.ts:286

bcdev_test_run carries // destructiveHint:false assumes BC test isolation (TestIsolation=Codeunit) rolls back data changes. at :182. The new tool copies the annotation but not the comment, and the isolation assumption is doing 20x the work here: any side effect that is not rolled back (a TestIsolation = Disabled suite, outbound HTTP) repeats once per run. One line to add the comment; worth naming the repetition in the tool description too.

6. Analyzer precondition hole

src/core/test-orchestration.ts:256,260 index test.observations[index - 1] with a non-null assertion, but observations are sized to runsRequested. Calling analyzeTestOrchestration with runs.length > runsRequested (and >= 2) throws a TypeError. Unreachable through the handler thanks to the loop bound plus break, and statusSet at :137 guards the same access with ?., so the guarding is inconsistent within one function. Guard it or document the precondition.

7. Ambiguity warnings scale with methods times runs

src/core/test-orchestration.ts:213-216 pushes the ambiguity warning from inside the per-run observations map, so it fires once per identity per run. A duplicated group covering M methods over R runs emits up to M x R warning strings, on top of the per-identity incomplete line. Response size and noise only, but the consumers here are agents.

8. Identity casing split between tests[] and runs[]

tests[] and diffs[] render the first-seen spelling, and the plan is processed before the runs (src/core/test-orchestration.ts:165-181), so the caller's casing wins. The mIxEd test pins this deliberately. Meanwhile runs[].results[].method carries the server's spelling. An agent that string matches tests[].method against runs[].results[].method will miss. One sentence in the schema description resolves it.

9. All-skipped selection reports outcome: "passed"

A selection where every method is stableSkipped yields outcome: "passed" with stablePassed: 0. Consistent with bcdev_test_run's outcome convention, but for a stability tool, "passed" on zero passing evidence is worth either a warning or a documented sentence.

---

Nits

• test-tools.ts:323: { run: index + 1, ...run } lets the run's own keys win. If RunTestsResult ever gains a run field, the orchestration index vanishes silently and the output schema's run goes wrong. Flip to { ...run, run: index + 1 }.
• test-tools.ts:6 and :12: two separate import type statements from ../../core/types. Merge them.
• runsCompleted counts attempts retained, including an aborted one that did not complete, which is why the schema description has to walk it back. runsAttempted would need no disclaimer.
• README line 93: coverage—use is an unspaced em dash, inconsistent with the surrounding style.

---

Test coverage

Strong, and it pins the things that actually break: sequential execution asserted via maxActive() === 1, all four contention paths (bcdev_test_run, a second orchestrate, bcdev_debug_run_tests, bcdev_native_call) each returning TEST_RUN_ACTIVE, lock release on preflight failure with hubs.length === 0, a real socket-drop abort stopping the sequence, unreadable project staying nonfatal, and outputSchema.parse(result) round tripped against real handler output in two tests. Analyzer vectors cover duplicate-row ambiguity, case-insensitive matching, flaky-but-incomplete, and zero runs.

The one gap is the mid-loop throw in finding 1.

---

Merge gate

• Block on 1. The evidence-retention guarantee is the tool's headline claim and it breaks on the exact environment the E2E validated. Fix plus one pinning test is small.
• Before or immediately after merge: finding 2 as at minimum a between-iterations signal check or an explicit doc caveat, finding 5's comment, and a sentence resolving the casing split in finding 8.
• Should fix, non blocking: findings 3, 4, 6, 7, 9 and the nits.

Nothing else surfaced in the diff or classification math. The off-by-one surfaces, identity normalization, and the runsRequested / runs.length interaction are correct as shipped.

[SShadowS]

All review findings closed in b90c77e. Verified locally on that head: typecheck clean, build clean with zero embed-skills drift, bun test 641 pass / 0 fail, README badge updated to match.

Evidence retention now survives a mid-sequence rejection (synthesized aborted attempt with a redacted abortReason), client cancellation is observed between attempts without cancelling an in-flight server run, upperInvariantUtf16 is shared from src/core/al-identifiers.ts, duplicate (id, method) coverage is rejected while split groups still validate, and the destructiveHint isolation caveat is documented. The analyzer precondition guards, the collapsed ambiguity warning, the documented casing split, and the all-skipped warning are all in as well.

Approving.

[ianrayianray]

Resolved the review in b90c77e.

• A per-attempt catch now converts a later setup/authorization rejection into a retained aborted attempt, preserving all earlier evidence. The regression test crosses the authorization boundary, checks redaction, and confirms lock release.
• MCP request AbortSignal now reaches tool handlers through the agent-response wrapper. Orchestration observes it between attempts and explicitly does not claim active server-run cancellation.
• The duplicated AL identifier normalization is now one shared core utility.
• Shared codeunit validation rejects overlapping (codeunit, method) coverage while accepting disjoint same-codeunit groups.
• The destructive annotation now documents repeated non-rolled-back side effects.
• Analyzer bounds are explicit, adjacent-diff indexing no longer relies on non-null assertions, duplicate warnings are aggregated per run, and all-skipped results warn about absent passing evidence.
• runsCompleted is now runsAttempted; retained run numbering can no longer be overwritten; schema/docs explain identity casing.

Verification on the pushed head: 641 tests passed, typecheck passed, build passed, embedded-skill drift passed, and CI is green. No additional Sandbox call was needed; the dated live evidence now distinguishes the historical duplicate-plan observation from the corrected input boundary.