test(windows-prep): add Stream 0 regression-net tests for POSIX behavior#1278
test(windows-prep): add Stream 0 regression-net tests for POSIX behavior#1278
Conversation
Tests-only PR locking in existing POSIX (macOS/Linux) behavior before three parallel Windows-readiness refactor streams begin: A (Docker/vault), B (cross-platform shellouts), and C (signals/env/paths). No production code modified. Coverage added: - DockerExecutionDriver: SIGTERM-trapping child error path; interleaved stdout/stderr capture without loss - LocalEncryptionVaultProvider: vault directory created with mode 0o700 - HttpUpdateChecker.downloadAndInstall: tarball extract + chmod 0o755 end-to-end; macOS quarantine xattr cleared on darwin host - executeProcess: AbortSignal mid-execution surfaces AbortError; timeout sends SIGTERM (not SIGKILL) and surfaces "timed out" - swamp doctor audit (integration): JSON contract holds with --tool none - swamp serve (integration): SIGINT triggers clean shutdown within 6s - datastore sync coordinator: SIGINT handler releases locks within the 5s force-exit deadline (subprocess form) - input_parser: ~/file expands via HOME on POSIX; literal path preserved when HOME unset - ModelType: normalize/toDirectoryPath always emit forward slashes - ExtensionAutoResolver: candidate names preserve forward-slash separators Two items deferred: - embedded_deno_runtime findSystemDeno multi-line which output: helper is private with no injection seam; only invoked in standalone-mode failure path which dev-mode tests can't reach. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Clean, well-structured tests-only PR. All 13 regression-net tests are well-organized, properly gate on ignore: Deno.build.os === "windows" (or !== "darwin" for the xattr test), and clean up resources in finally blocks. No blocking issues found.
Verified:
- AGPLv3 license headers present on both new files and all modified files
- No libswamp internal-path import violations — all test imports use adjacent module paths correctly
- Test naming follows existing conventions in each file (
-in docker/extension tests,:in process/model tests) @std/pathused correctly for all path operations (no hand-rolled path concatenation)- Subprocess lifecycle properly managed (SIGKILL fallback in
finally,sanitizeResources: false/sanitizeOps: falsewhere needed) - No security concerns — temp dirs, local
port: 0servers, proper cleanup - DDD layer organization correct (domain tests in
src/domain/, infrastructure insrc/infrastructure/, integration inintegration/)
Blocking Issues
None.
Suggestions
-
Comment density: Several tests have multi-line comment blocks (4-7 lines) above them. CLAUDE.md prefers minimal comments ("one short line max"). For regression-net tests the WHY is valuable, but some of the longer blocks could be condensed — e.g., the block at
serve_shutdown_test.ts:24-30repeats information already in the test name and PR description. -
waitForLinetimeout robustness (serve_shutdown_test.ts:175-208): The timeout check is betweenreader.read()calls, so if the subprocess never writes to stdout, the function blocks onreader.read()indefinitely until the Deno test timeout fires. Wrapping the read in aPromise.racewith a timer would make the timeout self-contained. Not a practical concern since the Deno test timeout is the fallback, but worth noting. -
Uncleaned
setTimeoutinPromise.racepatterns (serve_shutdown_test.ts:260-269,datastore_sync_coordinator_test.ts:608-618): The reject timers are never cleared when the process exits before the deadline. WithsanitizeOps: falsethis doesn't cause failures, but storing the timer ID and callingclearTimeouton the success path would be tidier.
There was a problem hiding this comment.
Adversarial Review
Critical / High
No critical or high severity findings.
Medium
-
src/domain/drivers/docker_execution_driver_test.ts:496-515— Conditional assertion block silently skips stdout verification if output kind changes.The interleaved stdout/stderr test wraps all stdout content assertions inside
if (result.outputs[0].kind === "pending"). If a refactor changes the output kind (e.g., to"resolved"), everyassertStringIncludesand occurrence-count check is silently skipped, and the test passes with zero stdout verification. This defeats the purpose of a regression net.Breaking example: Production code changes
kind: "pending"tokind: "resolved"→ theifblock is skipped → test passes green despite potentially broken stdout capture.Suggested fix: Assert the kind unconditionally before the content checks:
assertEquals(result.outputs[0].kind, "pending"); const text = new TextDecoder().decode(result.outputs[0].content); // ... rest of assertions without the if wrapper
-
src/infrastructure/update/http_update_checker_test.ts:1031-1083(macOS xattr test) — Vacuous assertion: locally-written files never carrycom.apple.quarantine.The test verifies that the installed binary doesn't have the quarantine xattr. But files written locally by Deno (not downloaded via a browser or
curl) are never tagged withcom.apple.quarantinein the first place. The localDeno.serve→fetchpath doesn't trigger macOS quarantine tagging. This means the assertion passes regardless of whether theremoveQuarantine()call exists or is deleted.The test comment acknowledges this ("Files written via Deno on a local filesystem typically aren't tagged"), which means the test is documenting a known gap rather than providing real coverage. It would only catch the narrow scenario where a refactor both removes the xattr call AND introduces quarantine tagging — an unlikely conjunction.
Impact: Low practical risk since the test at least exercises the full
downloadAndInstallpath on darwin. But it doesn't verify the quarantine removal code actually runs. Consider adding a step that explicitly sets the quarantine xattr before callingdownloadAndInstall, then verifying it's removed:// Tag the target path (or extracted binary) before install await new Deno.Command("xattr", { args: ["-w", "com.apple.quarantine", "0081;...", binaryPath], }).output(); await checker.downloadAndInstall(url, binaryPath, checksum); // Now the assertion actually tests removeQuarantine()
Low
-
integration/serve_shutdown_test.ts:175-192—waitForLinetimeout is only checked betweenreader.read()calls. Ifreader.read()blocks indefinitely (e.g., subprocess hangs producing no output and doesn't exit), the timeout never fires and the test hangs. In practice, the 15s Deno test timeout or CI timeout would catch this, so the blast radius is limited to slow CI feedback rather than incorrect results. -
integration/serve_shutdown_test.ts:155-169—CLI_LAUNCH_ARGSduplicatesintegration/test_helpers.ts:CLI_ARGSwith the addition of--allow-net. Not a correctness issue, but ifCLI_ARGSis updated (e.g., new unstable flags), this copy won't track. A minor maintenance concern. -
src/infrastructure/persistence/datastore_sync_coordinator_test.ts:741-767— The child program passed via stdin uses an untypedfnparameter in the mock lock'swithLock(fn). This works because the child runs as a standalone Deno process without the project's strict tsconfig, but it means the mock doesn't enforce the lock interface contract at compile time. Purely theoretical since the child is a runtime-only fixture.
Verdict
PASS — Tests-only PR with solid regression coverage across POSIX-specific behaviors. The conditional assertion in the Docker interleaved test (Medium #1) is the most actionable finding — a one-line fix to make the assertion unconditional. The macOS xattr test (Medium #2) is acknowledged-weak coverage. No production code is modified, no security concerns, and no blocking issues.
Summary
Adds 13 regression-net tests on macOS/Linux that lock in existing POSIX behavior before three planned Windows-readiness refactor streams (Docker/vault, cross-platform shellouts, signal/env helpers). Tests-only PR — no production code modified.
The high-leverage additions plug previously-zero-coverage code paths:
swamp updateend-to-end (download → extract → chmod → macOS xattr clear)swamp serveSIGINT shutdown via real subprocess + signalswamp doctor auditintegration test0o700invariantThe remaining additions pin existing contracts that were previously implicit:
executeProcessAbortSignal handling (streaming-mode contract)executeProcesstimeout SIGTERM behavior~/expansion via HOME env var@std/path)One item deferred:
findSystemDenomulti-line output parsing. The helper is private and only reachable whenDeno.build.standalone === trueAND the embedded binary's health check fails — no public seam exists for testing without modifying production code. Will land alongside the cross-platformresolve_commandhelper in the next stream.Test Plan
deno fmt --checkclean (1277 files)deno lintclean (1156 files)deno checkcleandeno run test— 5099 passed, 0 failed, 23 ignored