refactor: inject process handles so tests never mutate the global process (#183)#185
Merged
Merged
Conversation
…dencies (#183) - Add ProcessLike interface (src/interfaces/process-interfaces.ts): minimal current-process surface, structurally satisfied by both the global process and EventEmitter-backed fakes - ProxyRunner: options.proc (default global process); all ~20 process touchpoints routed through this.proc; worker exit hook threaded as (code) => this.proc.exit(code) (identical to the worker default in prod) - detectExecutionMode takes a narrow injectable handle - createProductionDependencies(proc): messageSender IPC/stdout channel injectable - Delete dead standalone setupGlobalErrorHandlers (only tests called it; diverged from the ProxyRunner method prod uses); update error-handling docs - New FakeCurrentProcess test fake; migrate dap-proxy-core, dap-proxy-dependencies, and dap-proxy-worker tests off all global-process mutation; remove the #159 process.exit spy safety nets (no longer reachable) Verified: proxy suites green under LEAK_GUARD_STRICT=1; production entry (dap-proxy-entry.ts) unchanged - defaults preserve byte-identical behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…f utilities; enforce strict leak guard in CI (#183) - stdio/http/sse commands + error-handlers: proc?: ProcessLike dependency; signal registration, env gates, exit diagnostics, and the exitProcess default all route through the injected handle - stdin-watchdog: stdin option widened to NodeJS.ReadableStream; callers pass env from the handle - jvm-orphan-reaper + logger stale-log cleanup: injectable SignalFn seam for pid-liveness instead of global process.kill spies - adapter-policy resolveExecutablePath + buildRdbgInvocation: optional platform parameter (defaults to process.platform) - Test migrations: no CLI/utils test reassigns process.on, spies process.exit/kill, or redefines process.platform anymore; the one sanctioned boundary test (ProcessEnvironment cwd delegation) is documented - CI: LEAK_GUARD_STRICT=1 in ci.yml and flake-hunt.yml; new test:strict script Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…er (#183) Caught by the acceptance grep: the killProcessTree shutdown tests redefined process.platform globally. The win32 tree-kill branch (#156) now reads an injected platform (default: real platform); tests construct managers with explicit win32/linux platforms instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
Fixes #183.
What
The proper fix behind the #159 CI flake: production code no longer hardwires the global
process, so tests no longer have to mutate it to drive the code under test. With the offenders gone, the #184 leak guard is promoted from lenient to enforcing: CI now runs withLEAK_GUARD_STRICT=1.New abstraction
ProcessLike(src/interfaces/process-interfaces.ts) — minimal current-process surface (on/removeListener/listeners/rawListeners/listenerCount/exit/send?/connected/env/argv/uptime/stdin/stdout). Hand-rolled rather thanPick<NodeJS.Process, ...>: the exact Node types (tty stream intersections,never-returningexit, per-event listener overloads) make fakes unimplementable. Structurally satisfied by both the globalprocessand the new EventEmitter-backedFakeCurrentProcesstest fake (tests/test-utils/mocks/fake-current-process.ts).Injection sites (all default to the global
process— production behavior unchanged, entry files untouched)ProxyRunner(ProxyRunnerOptions.proc): all ~20 touchpoints — IPC mode detection, heartbeat interval, 10s init timeout,message/disconnect/errorlisteners, readline stdio, and thesetupGlobalErrorHandlersmethod. The worker exit hook is threaded as(code) => this.proc.exit(code)(identical to the worker's own default in prod).detectExecutionMode(proc?),createProductionDependencies(proc?)(messageSender IPC/stdout channel).stdio,http,sse,error-handlers):proc?: ProcessLikein each dependencies object; signal registration, env gates, exit diagnostics, and theexitProcessdefault route through it. Stdin watchdog getsenvfrom the handle.GenericAdapterManager: constructor-injectedplatform(win32 tree-kill branch, Ruby launch orphans the debuggee ruby.exe on Windows: worker destroys the DAP socket before sending disconnect, and tree-kill only fires on the slow path #156 heritage).ProcessLikewould be ceremony:SignalFnfor pid-liveness (jvm-orphan-reaper, logger stale-log cleanup);platformparams onresolveExecutablePathandbuildRdbgInvocation.Deleted
setupGlobalErrorHandlers()indap-proxy-dependencies.ts— dead in production (only its own tests called it) and behaviorally divergent from theProxyRunnermethod prod uses (it exited onunhandledRejection; the real one doesn't). Docs updated.process.exitspy nets in dap-proxy-core/worker tests, listener-baseline belt-and-braces) — unreachable now that every runner/worker gets an injected handle.Deliberate exceptions
proxy-bootstrap.jsuntouched (plain-JS pre-TS orphan-prevention layer).environment-impl.test.tskeeps itsprocess.cwdspy, documented as the sanctioned boundary test —ProcessEnvironmentIS the process abstraction; injecting a fake there is tautological.packages/adapter-*platform-redefinition tests (~60 sites, cannot trip the leak guard) deferred to a follow-up issue to keep this reviewable.Verification
ProxyRunnerrewrite, the unmodified old tests still passed (default = global process, byte-identical)LEAK_GUARD_STRICT=1 pnpm run test:ci-coverage(same command as CI, both projects) — green, 0 leak messagesLEAK_GUARD_STRICT=1 FLAKE_RUNS=3 pnpm run test:flake— 3/3 shuffled runs greenprocess.on =reassignment, nodelete process.send/defineProperty(process, ...), novi.spyOn(process, 'exit'|'kill'|'on')in migrated suites;dap-proxy-core.tstouches the global only in its two= processdefaultsnpm run lintcleanNew convenience script:
pnpm run test:strict(strict guard locally; default local runs stay lenient).🤖 Generated with Claude Code