Audit: eeb8cb05 mixes cleanup with install defects and loses coverage

[cyfung1031]

Context

This is a source-grounded audit of eeb8cb05c5a969c59a4276a7964933cffcfc7e5f (parent 65121faaa5376eaa0d04657c2d1855a402bd153d), the commit titled “✅ 清理高置信冗余测试 (#1653)”. It follows issue #1671, which correctly questioned why a test-cleanup change modified implementation files.

The audit reconstructed the target incrementally from its parent, with independent PickInvariant reviews and repository validation. The reconstructed HEAD passes 310/310 Vitest files (3441/3441 tests), typecheck, lint, and build. The original target was not tested as a standalone checkout.

Confirmed defects in the original target

1. Direct DNR URL installs lose the byWebRequest matching signal (P1)

The target adds byWebRequest=1 to the DNR redirect in service_worker/script.ts, but its raw-url path still calls loadFromInfo(..., {}) in useInstallData.ts.

That option is consumed by prepareScriptByCode to perform the existing-script match by origin/name. Consequently, a web-request install through the direct DNR URL can fail to reuse the existing script identity when its metadata name changed, potentially creating a duplicate active script or matching the wrong same-origin/recycle-bin entry.

The reconstruction passes { byWebRequest: byWebRequestRef.current } through this raw-URL branch and adds a regression assertion in useInstallData.test.ts.

2. The new install tests leak spies across tests (P2)

The target adds spies for window.close, history.back, and the history.length getter in useInstallData.test.ts, but the file’s afterEach only calls vi.clearAllMocks(). That clears call history without restoring property spies. With the repository’s non-isolated UI test configuration, the getter and method replacements can affect later tests or make results depend on execution order.

The reconstruction adds vi.restoreAllMocks() to the file cleanup.

3. A unique initial-mobile regression contract was deleted (P2 coverage loss)

The target removes the useIsMobile test that starts with matchMedia(...).matches === true from use-is-mobile.test.ts. The surviving change-event test starts at false, and the subscription-race test is not an initial-mobile render test. The implementation takes its initial snapshot from matchMedia.matches, so the true initial branch was no longer protected.

The reconstruction retains this test while removing only the redundant initial-false case.

4. A unique nested ZIP end-to-end contract was deleted (P2 coverage loss)

The target removes the nested ZIP test from skill-zip.test.ts. The deleted contract covered the material chain:

nested ZIP paths → extracted SKILL.md → parseSkillMd → extracted script → parseSkillScriptMetadata → references.

The remaining tests separately checked nested filenames/counts or root-level script parsing, but did not cover this combined nested extraction-to-script parsing path. The reconstruction retains the end-to-end test.

5. The refactored keep-alive helper can leak its page on setup failure (P2 test-infrastructure defect)

In the target keep-alive.spec.ts, openRuntimeSettings creates a page and performs readiness waits/navigation before either caller enters its try/finally. If readiness or navigation fails, that page is not closed by the test body and remains until fixture teardown.

The reconstruction closes the page on helper failure before rethrowing, while preserving the callers’ existing CDP cleanup.

Scope/process defect

The parent-to-target change was presented as high-confidence redundant-test cleanup, but the target changes production behavior and production-adjacent configuration in service_worker/script.ts, useInstallData.ts, RuntimeSection.tsx, and vitest.config.ts. The explicit history-length behavior change may be a legitimate bug fix, but it is still outside the stated cleanup scope and should have been isolated and reviewed as behavior change.

Residual validation limits

• No direct unit assertion currently inspects the generated DNR regexSubstitution; the consumer and install matching paths are covered, but the producer payload remains a low-severity coverage gap.
• The keep-alive Playwright tests could not reach their assertions in this macOS environment. The original launcher died before page load with SIGABRT; the skill-directed temporary headless: true probe also died before navigation with SIGTRAP and thermal_state_observer_mac.mm power-notification registration errors. The temporary launcher change was reverted. This is an environment limitation, not evidence that the keep-alive behavior itself is broken.
• Other deleted tests in the target were reviewed against surviving caller/branch coverage and were not found to remove a material unique contract.

Suggested follow-up

1. Keep the direct DNR producer and consumer provenance assertions together.
2. Keep the initial-mobile and nested-ZIP end-to-end regression tests.
3. Restore test spies in any test that replaces browser globals.
4. Put future production behavior fixes in separately scoped commits/PRs from redundancy cleanup.
5. Add a browser or service-worker fixture assertion for the generated DNR redirect rule when a stable browser runner is available.

[cyfung1031]

incremental_source_ground_audit_prompt.md