Audit test contracts, stabilize fixtures, and fix uncovered defects - #487
Merged
Merged
Conversation
Tryanks
marked this pull request as ready for review
September 20, 2026 17:11
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CI tests mixed meaningful product contracts with upstream checks, overlapping one-off cases and fixtures that guessed when background work had finished. This audits the repository-owned suite, removes redundant coverage, merges related scenarios, and makes asynchronous assertions wait for observable state.
Rust source tests go from 1,275 to 916 (359 fewer, 28.2%); Browser-auth tests go from 3 to 2. Counts include platform-gated and explicitly ignored tests, so they are not per-run execution totals. Each surviving source test was reviewed against its implementation and recorded with a concrete retention reason; removed/merged names have coverage mappings.
What remains covered
catorshfor their test fixtures.A Windows nextest leak warning also exposed a shared Markdown fixture that returned before its detached Git status probe finished. A two-second slow-Git fixture made the old cleanup contract fail after 0.58 seconds; waiting for the actual Git status callback made it pass after 2.60 seconds. The helper now waits for that completion and shuts its host down on GPUI teardown. The temporary probe assertion was removed; all 28 chat tests and 200 targeted repetitions passed. The final Windows run passed all 878 tests with no LEAK warnings; Linux and macOS also reported none.
Production defects exposed by broader regressions
waitpidbits, e.g. exit 37 as 9472ExitStatus; real PTY input/output/exit regression failed before the fix and passed afterward. Signal termination remainsNone./laterduration panicked inside chronoRead/Glob/Grep/WebSearch; actualcan_use_toolmatrix rejects MCP/lookalike names. Display heuristics no longer authorize execution.tcode_reportmcp__tcode_report__report_resultname and approve only that request. Regression matrices fail on lookalike names, other providers and session-wide approval. Claude wire tests show single approval does not forward permission suggestions.These fixes stay in the existing owners; no new runtime abstraction or dependency was introduced. ACP containment is command-path validation, not a sandbox for downloaded agent code or a defense against concurrent local filesystem replacement. Unknown Claude tools now require normal approval. Providers without Claude's authoritative tool namespace keep their normal approval path.
Validation
Final commit:
123e122b(includes main’s CI simplification from #488). The obsolete Python planner and its tests were removed upstream; that deletion is not counted as this PR’s test reduction.cargo fmt --all --checkcargo clippy --workspace --all-targets --locked -- -D warningscargo build --workspace --locked— passed on the final commit.cargo nextest run --workspace --locked— 900 passed, 0 failed, 6 explicitly skipped (five libtest entries plus the native-overlay harness), no LEAK warnings.cargo test -p gpui-android --lib --locked— 4 pure input tests passed; this excluded workspace package was run explicitly.cargo-machete 0.9.2— passed.node --test crates/web/static/auth.test.mjs— 2 passed./→/tmptransition; native overlay/live registry/credential-dependent usage/manual history benchmark probes remain unrun.Independent reviews: Standards: no actionable findings, including the merge with #488 and final fixture cleanup. The connection-stamp test verifies final field preservation but does not prove a specific scheduler interleaving. Spec: no missing requirements, coverage regressions or scope violations found in the reviewed diff.
Remaining ignored desktop/live-service/manual benchmark cases were reviewed but are not claimed as executed. The ignored child-host entry is a subprocess fixture exercised by its parent liveness test; the foreground-cwd probe was run separately. No provider credentials or live accounts are required by the normal suite. Native GUI permission/input/WebView behavior still needs its opt-in platform probes. The local macOS debug link emits the existing large-unwind-table linker warning; strict Clippy passes.