feat: Effect-TS migration of fileCheckers and launcherCommon#567
Merged
timbrinded merged 19 commits intomainfrom Feb 16, 2026
Merged
feat: Effect-TS migration of fileCheckers and launcherCommon#567timbrinded merged 19 commits intomainfrom
timbrinded merged 19 commits intomainfrom
Conversation
- Extract enrichReadOnlyContext() as best-effort async helper - Convert executeTests() from Promise anti-pattern to Effect.gen - Add TestExecutionError tagged error for structured error handling - Add commandParsers.test.ts for withPorts() coverage Reverts the --rpc-port=0 change from #479 — the OS-assigned port approach caused CI failures for fork_test and dev_tanssi where the port discovery + provider connection timing was unreliable. Closes #559 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace deterministic shard-based port calculation (10000 + offsets) with OS port 0 allocation. Binds a temporary server to port 0, reads the kernel-assigned ephemeral port, then releases it. This avoids conflicts with services in the 10000-11000 range that caused #479. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
greet() returns a Promise — accessing .rtName/.rtVersion without await
yielded undefined (pre-existing bug from main, now fixed). Also guard
MoonwallContext.destroy() with contextCreated flag so it doesn't throw
invariant("No context to destroy") if contextCreator() fails.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove stale shardManager import and initializeSharding call from runTests (only consumer was deleted port calculation) - Document accepted TOCTOU window in getFreePort() JSDoc - Add test for RECYCLE mode when MOONWALL_RPC_PORT is unset Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Under CI load, the collator's tx pool access intermittently times out
("Timeout fired waiting for transaction pool at block"), producing
empty blocks without user transactions. The relay chain finalization
also lags far behind block production, making isFinalized subscriptions
unreliable.
Replace the fixed waitBlock(4) with a polling loop that checks the
balance after each new block (up to 20), breaking early on success.
Uses BigInt comparison for reliable assertions.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
shardManager was only used by the old deterministic port calculation (10000 + shard offsets). Now that getFreePort() uses OS port 0 allocation, no consumers remain. The call in runNetwork.tsx defaulted to no-op values and nothing read the state afterward. CI sharding still works via vitest's native --shard flag passed through as a vitest option. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…services Replace imperative async/throw patterns with idiomatic Effect.gen pipelines. Introduce CommandRunner, Prompter, and DockerClient service tags for dependency injection, enabling pure Effect-native testing without vi.mock(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request migrates fileCheckers.ts and launcherCommon.ts to idiomatic Effect-TS patterns, introducing three new service tags (CommandRunner, Prompter, DockerClient) for dependency injection and five new tagged error types. The migration enables pure Effect-native testing without vi.mock() while preserving all existing caller contracts via thin Promise wrappers.
Changes:
- Migrated 7 functions in fileCheckers.ts and 5 functions in launcherCommon.ts to Effect.gen pipelines with typed errors
- Introduced CommandRunner, Prompter, and DockerClient service tags with live Layer implementations
- Added BinaryNotFoundError, BinaryArchMismatchError, BinaryPermissionError, UserAbortError, and ScriptExecutionError
- Removed shardManager and simplified port allocation to use OS ephemeral port allocation
- Added comprehensive test coverage (17 tests for fileCheckers, 11 for launcherCommon)
- Increased forking node startup timeout from 30s to 2 minutes
- Made zombie test T04 more robust by polling for balance changes instead of fixed block count
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/CommandRunner.ts | New service tag wrapping execSync for command execution with typed errors |
| src/services/Prompter.ts | New service tag wrapping @inquirer/prompts for user prompts |
| src/services/DockerClient.ts | New service tag wrapping dockerode for container management |
| src/services/errors.ts | Added 6 new tagged error types including TestExecutionError and binary-related errors |
| src/services/index.ts | Exported new service modules |
| src/cli/internal/fileCheckers.ts | Migrated to Effect with 7 Effect functions + Promise wrappers preserving API |
| src/cli/internal/launcherCommon.ts | Migrated to Effect with 5 Effect functions + Promise wrappers preserving API |
| src/cli/internal/commandParsers.ts | Simplified getFreePort to use OS ephemeral allocation, removed shard-based logic |
| src/cli/internal/effect/tests/fileCheckers.test.ts | Added 17 tests with mock layers for all Effect functions |
| src/cli/internal/effect/tests/launcherCommon.test.ts | Added 11 tests covering zombie/dev/Docker/script execution paths |
| src/cli/internal/effect/tests/commandParsers.test.ts | Added 6 tests for port allocation logic |
| src/cli/cmds/runTests.ts | Refactored executeTests to Effect.gen, added enrichReadOnlyContext function |
| src/cli/cmds/runNetwork.tsx | Removed shardManager initialization |
| src/cli/lib/shardManager.ts | Deleted entire file (105 lines) - shard functionality removed |
| src/cli/lib/globalContext.ts | Added forking node detection for 2-minute startup timeout |
| test/suites/zombie/test_basic.ts | Made balance change test more robust with polling loop |
| test/suites/fork_test/test_fork.ts | Made fork URL configurable via MOONBEAM_HTTP_ENDPOINT env var |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix broken grammar in checkListeningPortsEffect log message - Wrap err.cause in String() for defensive logging in executeScriptEffect - Capitalize Tanssi as proper noun in getFreePort JSDoc Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tests for zombie/Docker prompt behavior (abort, kill, quit) need the non-CI code path. Save/restore CI env var with Effect.ensuring for safe cleanup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix checkListeningPortsEffect to use regex for IPv6-safe port extraction - Extract binPath split once in checkAccessEffect (was split twice) - Remove unused "pgrep" | "lsof" from ProcessError operation union - Remove unused encoding option from CommandRunner.exec interface - Add JSDoc to CommandRunner methods documenting shell execution - Remove vacuous expect(true).toBe(true) assertions - Split downloadBinsIfMissing tests into explicit x64/non-x64 branches - Add tests: IPv6 lsof, duplicate port dedup, multi-PID kill - Remove unused Exit import Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
timbrinded
force-pushed
the
fix/effect-test-runner-and-port-race
branch
from
564dad5 to
d74019c
Compare
Without -P, lsof resolves port numbers to service names (e.g. 9955 becomes "alljoyn-stm"), causing the port regex to miss them. Without -i, lsof dumps all file descriptors instead of just network sockets. The -a flag ensures -p and -i are ANDed (lsof ORs them by default). The -n flag prevents DNS lookups on addresses. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.
Summary
fileCheckers.ts(7 functions) andlauncherCommon.ts(5 functions) to idiomatic Effect.gen pipelines with typed tagged errorsCommandRunner,Prompter, andDockerClientservice tags for dependency injection — enables pure Effect-native testing withoutvi.mock()Data.TaggedErrortypes (BinaryNotFoundError,BinaryArchMismatchError,BinaryPermissionError,UserAbortError,ScriptExecutionError)executeScriptEffect,commonChecksEffect,zombieBinCheckEffect) for testabilityTest plan
fileCheckers.test.ts— 17 tests covering all Effect functions with mock layerslauncherCommon.test.ts— 11 tests covering zombie/dev/Docker/script pathsLayer.succeed()mock layers forCommandRunner/Prompter/DockerClientpnpm typecheckcleanpnpm fmt:fix/pnpm lint:fixclean🤖 Generated with Claude Code