Audit and harden the test suite

[morluto]

Why this PR exists

The test suite had accumulated several patterns that made failures harder to trust:

• asynchronous tests often waited by sleeping for an arbitrary number of milliseconds or yielding a fixed number of times
• some concurrency tests asserted only an upper bound, so they could pass even when the intended concurrency was never exercised
• a few tests inspected source text, symbol names, constant inventories, or reflected storage shape instead of observable behavior
• unused test helpers and duplicated fixture builders increased maintenance cost and obscured the real test contracts
• ledger metadata had drifted from the executable suite, including missing rows, incorrect classifications, and malformed columns

The result was a mix of false-pass risk, timing-dependent failures, misleading failure names, and avoidable maintenance overhead. This PR audits those patterns across the root and provider test trees and applies only changes with a clear regression-value improvement.

Review principles

The cleanup follows these rules:

1. Protect user-visible, protocol, persistence, security, and lifecycle behavior—not implementation spelling.
2. Prefer deterministic gates and observable state over elapsed wall-clock guesses.
3. Keep timeout diagnostics bounded and monotonic.
4. Remove a test only when stronger behavioral coverage already protects the contract or the assertion has no credible behavioral oracle.
5. Keep the curated test-contract ledger synchronized with executable test changes.

Issues found and addressed

Flaky and brittle asynchronous waits

The audit found repeated variants of:

• fixed sleeps used to wait for propagation
• fixed-count Task.yield() loops with no real time bound
• polling against Date() rather than a monotonic clock
• unbounded waits that could hang a shard
• helpers that returned false silently on timeout
• negative-time assertions that did not prove the competing task had actually reached the blocked state

These were replaced with bounded AsyncTestWait conditions, explicit actor hops, or deterministic gates where the state transition is observable.

Weak concurrency oracles

The Git admission budget test previously asserted only that concurrency stayed below configured limits. That could pass if the implementation accidentally serialized everything.

The revised test proves both sides of the contract:

• both global permits become active
• the per-repository limit remains one
• repo-a is deterministically active before the remaining repositories race
• all queued work eventually completes

Autoreview caught and helped repair an intermediate scheduling race in this test before landing.

Compile-time hazards introduced during cleanup

Structured autoreview caught two Swift concurrency/XCTest mistakes in the initial cleanup:

• @MainActor predicates passed directly into a nonisolated async waiter
• awaited actor calls placed inside XCTest synchronous autoclosures

The final patch explicitly hops to MainActor and awaits values into locals before asserting.

A parallel ledger/diff audit also caught a mistakenly removed WorkspaceSwitchRecoveryTests.waitUntil helper that still had live callers. It was restored as a bounded AsyncTestWait wrapper.

Low-value structural coverage

Five executable tests were removed because they asserted source layout, source-fragment ordering, reflected stored-property types, or constant inventories rather than runtime behavior:

• root/RepoPromptTests.BindContextRoutingRecoveryTests/testMCPConnectionManagerHasNoIncompleteBindContextFastPathOrUnsafeRegisteredServicesSnapshot
• root/RepoPromptTests.StoreBackedWorkspaceSearchTests/testBroadSearchOrchestrationChecksScopeAndReadinessBeforeAndAfterAdmission
• root/RepoPromptTests.StoreBackedWorkspaceSearchTests/testSearchScopeParserKeepsRequiredResolutionOrder
• root/RepoPromptTests.CodeMapV6CacheDeletionTests/testReportTelemetryShapeContainsOnlyNumericStoredFields
• root/RepoPromptTests.ContextBuilderMCPProgressTimelineTests/testPhaseCatalogCoversExpectedDiscoveryAndGenerationSequence

These tests were vulnerable to harmless refactors and could still pass when behavior was wrong. Existing behavioral tests continue to cover routing, search scope/readiness, cache deletion outcomes, and progress timeline transitions.

Dead and duplicated test support

The cleanup removes unused actors, continuations, recorders, source readers, benchmark formatting helpers, Git fixture helpers, and wait helpers.

It also consolidates two duplicated write_ledger implementations in the CI app-test runner tests. The surviving helper supports both existing call shapes and remains covered by all 52 runner self-tests.

Ledger drift and classification

The ledger was reconciled surgically rather than regenerated:

• added two missing default-executable test rows
• removed the five deleted executable IDs
• corrected scenario accounting
• reclassified production-shaped worktree smoke tests as integration tests
• corrected VCS ownership metadata
• fixed malformed TSV field alignment
• reviewed affected fixture, oracle, risk, shared-state, and lifecycle metadata

Final static ledger state:

• 3,144 unique default executable IDs
• 28,886 scenarios
• no duplicate IDs
• no overflow columns
• no ledger-only rows
• two source-only methods are expected opt-in benchmarks compiled only with RPCE_BENCHMARK_TESTS

Commit structure

1. test(ci): consolidate ledger fixture helper
2. test: harden async synchronization helpers
3. test: remove brittle structural coverage

Validation performed

• make ci-app-test-runner-selftest — 52/52 passed
• all changed Swift files parsed with swiftc -frontend -parse
• SwiftFormat lint passed for all changed Swift files
• git diff --check
• repository guardrails
• staged-index and outgoing-range secret scans
• three structured Codex autoreview passes
  • pass 1 found MainActor isolation hazards
  • pass 2 found the XCTest async-autoclosure issue and Git scheduling race
  • pass 3 returned no accepted/actionable findings

CI handoff

Local execution intentionally did not run the macOS test suite. Hosted CI is the authoritative proof for:

• macOS compilation and Swift concurrency checking
• root XCTest shards
• provider tests
• authoritative root/provider test lists
• verify-ledger
• style and Sentry-enabled build lanes

Scope intentionally left unchanged

Two opt-in report-only benchmark methods remain behind RPCE_BENCHMARK_TESTS. They may be better represented as diagnostics in a future change, but moving benchmark ownership is separate from this test-quality cleanup.

Several large omnibus tests also remain intact because splitting them would create substantial executable-ID and ledger churn without fixing a concrete behavioral defect.

[chatgpt-codex-connector]

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