refactor: resolve host process state through Effect#2959
Open
juliusmarminge wants to merge 3 commits into
Open
refactor: resolve host process state through Effect#2959juliusmarminge wants to merge 3 commits into
juliusmarminge wants to merge 3 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
vouch:trusted
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: WoA heuristics ignore build platform
- Added an optional buildPlatform parameter to resolveHostProcessArch and passed the build platform from getDefaultBuildArch, so WoA env var heuristics are skipped when the target platform is not Windows.
Or push these changes by commenting:
@cursor push c3131c09e1
Preview (c3131c09e1)
diff --git a/scripts/lib/build-target-arch.test.ts b/scripts/lib/build-target-arch.test.ts
--- a/scripts/lib/build-target-arch.test.ts
+++ b/scripts/lib/build-target-arch.test.ts
@@ -83,7 +83,7 @@
it.effect("does not apply Windows host env heuristics for non-Windows targets", () =>
Effect.gen(function* () {
const arch = yield* getDefaultBuildArch("linux", { archChoices: ["x64", "arm64"] }).pipe(
- withHostRuntime("linux", "x64", {
+ withHostRuntime("win32", "x64", {
PROCESSOR_ARCHITECTURE: "AMD64",
PROCESSOR_ARCHITEW6432: "ARM64",
}),
diff --git a/scripts/lib/build-target-arch.ts b/scripts/lib/build-target-arch.ts
--- a/scripts/lib/build-target-arch.ts
+++ b/scripts/lib/build-target-arch.ts
@@ -40,13 +40,19 @@
const optionToUndefined = <A>(value: Option.Option<A>): A | undefined =>
Option.getOrUndefined(value);
-export const resolveHostProcessArch = Effect.fn("resolveHostProcessArch")(function* () {
+export const resolveHostProcessArch = Effect.fn("resolveHostProcessArch")(function* (
+ buildPlatform?: BuildPlatform,
+) {
const platform = yield* HostProcessPlatform;
const processArch = yield* HostProcessArchitecture;
if (processArch === "arm64") return "arm64";
if (processArch === "x64") {
if (platform !== "win32") return "x64";
+ // Only apply WoA heuristics when building for Windows (or when no build
+ // platform is specified, e.g. bare host-arch queries).
+ if (buildPlatform !== undefined && buildPlatform !== "win") return "x64";
+
// On Windows-on-Arm, x64 Node/Bun can run under emulation while the host
// still reports ARM64 via the processor environment variables.
const env = yield* WindowsProcessorArchitectureConfig;
@@ -63,7 +69,7 @@
platform: BuildPlatform,
platformConfig: PlatformConfig,
) {
- const hostArch = yield* resolveHostProcessArch();
+ const hostArch = yield* resolveHostProcessArch(platform);
if (hostArch && platformConfig.archChoices.includes(hostArch)) {
return hostArch;
}You can send follow-ups to the cloud agent here.
Contributor
ApprovabilityVerdict: Needs human review 2 blocking correctness issues found. Major infrastructure refactor introducing Effect-based host process state injection across the codebase. Unresolved review comments identify a high-severity bug in child process environment handling (dev-runner.ts) and a medium-severity bug in execute permission checking (shell.ts) that could cause runtime issues. You can customize Macroscope's approvability policy. Learn more. |
Contributor
|
🚀 Expo continuous deployment is ready!
|
juliusmarminge
force-pushed
the
codex/effect-runtime-build-target
branch
from
0e3f530 to
761cc7f
Compare
github-actions
Bot
added
size:L
juliusmarminge
force-pushed
the
codex/effect-runtime-build-target
branch
from
761cc7f to
d3e884f
Compare
juliusmarminge
changed the title
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
juliusmarminge
force-pushed
the
codex/effect-runtime-build-target
branch
from
d3e884f to
5744aa5
Compare
macroscopeapp
Bot
dismissed
their stale review
Dismissing prior approval to re-evaluate 5744aa5
github-actions
Bot
added
size:XL
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Editor check ignores host env
- Replaced isCommandAvailable() (which uses process.platform/process.env) with isCommandAvailableForPlatform() using the yielded HostProcessPlatform and HostProcessEnv values, making the availability check consistent with the spawn behavior.
Or push these changes by commenting:
@cursor push 26a4a08a64
Preview (26a4a08a64)
diff --git a/apps/server/src/process/externalLauncher.ts b/apps/server/src/process/externalLauncher.ts
--- a/apps/server/src/process/externalLauncher.ts
+++ b/apps/server/src/process/externalLauncher.ts
@@ -14,7 +14,6 @@
} from "@t3tools/contracts";
import { HostProcessEnv, HostProcessPlatform } from "@t3tools/shared/hostProcess";
import {
- isCommandAvailable,
isCommandAvailableForPlatform,
type PlatformCommandAvailabilityOptions,
} from "@t3tools/shared/shell";
@@ -347,13 +346,14 @@
export const launchEditorProcess = Effect.fn("externalLauncher.launchEditorProcess")(function* (
launch: EditorLaunch,
): Effect.fn.Return<void, ExternalLauncherError, ChildProcessSpawner.ChildProcessSpawner> {
- if (!isCommandAvailable(launch.command)) {
+ const platform = yield* HostProcessPlatform;
+ const env = yield* HostProcessEnv;
+
+ if (!isCommandAvailableForPlatform(launch.command, { platform, env })) {
return yield* new ExternalLauncherError({
message: `Editor command not found: ${launch.command}`,
});
}
-
- const platform = yield* HostProcessPlatform;
const isWin32 = platform === "win32";
yield* launchAndUnref(
{You can send follow-ups to the cloud agent here.
juliusmarminge
force-pushed
the
codex/effect-runtime-build-target
branch
from
5744aa5 to
8857505
Compare
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Terminal drops non-whitelisted env
- Restored HostProcessEnv Context.Reference in hostProcess.ts and used it as the base for createTerminalSpawnEnv so spawned terminals inherit the full host process.env (minus blocklisted keys), while keeping readTerminalHostEnv for shell resolution.
Or push these changes by commenting:
@cursor push d2667f4b40
Preview (d2667f4b40)
diff --git a/apps/server/src/terminal/Layers/Manager.ts b/apps/server/src/terminal/Layers/Manager.ts
--- a/apps/server/src/terminal/Layers/Manager.ts
+++ b/apps/server/src/terminal/Layers/Manager.ts
@@ -10,7 +10,7 @@
type TerminalSummary,
} from "@t3tools/contracts";
import { makeKeyedCoalescingWorker } from "@t3tools/shared/KeyedCoalescingWorker";
-import { HostProcessPlatform } from "@t3tools/shared/hostProcess";
+import { HostProcessEnv, HostProcessPlatform } from "@t3tools/shared/hostProcess";
import { getTerminalLabel } from "@t3tools/shared/terminalLabels";
import * as Config from "effect/Config";
import * as DateTime from "effect/DateTime";
@@ -990,6 +990,7 @@
const logsDir = options.logsDir;
const historyLineLimit = options.historyLineLimit ?? DEFAULT_HISTORY_LINE_LIMIT;
const platform = yield* HostProcessPlatform;
+ const hostEnv = yield* HostProcessEnv;
const baseEnv = yield* readTerminalHostEnv;
const shellResolver = options.shellResolver ?? (() => defaultShellResolver(platform, baseEnv));
const processRunner = yield* ProcessRunner.ProcessRunner;
@@ -1668,7 +1669,7 @@
Effect.andThen(
Effect.gen(function* () {
const shellCandidates = resolveShellCandidates(shellResolver, platform, baseEnv);
- const terminalEnv = createTerminalSpawnEnv(baseEnv, session.runtimeEnv);
+ const terminalEnv = createTerminalSpawnEnv(hostEnv, session.runtimeEnv);
const spawnResult = yield* trySpawn(shellCandidates, terminalEnv, session);
ptyProcess = spawnResult.process;
startedShell = spawnResult.shellLabel;
diff --git a/packages/shared/src/hostProcess.ts b/packages/shared/src/hostProcess.ts
--- a/packages/shared/src/hostProcess.ts
+++ b/packages/shared/src/hostProcess.ts
@@ -15,4 +15,11 @@
},
);
+export const HostProcessEnv = Context.Reference<NodeJS.ProcessEnv>(
+ "@t3tools/shared/hostProcess/HostProcessEnv",
+ {
+ defaultValue: () => process.env,
+ },
+);
+
export const isHostWindows = Effect.map(HostProcessPlatform, (platform) => platform === "win32");You can send follow-ups to the cloud agent here.
Contributor
There was a problem hiding this comment.
🟢 Low
The cached ensureNodePtySpawnHelperExecutableCached effect is missing the HostProcessPlatform service. After the refactoring, ensureNodePtySpawnHelperExecutable now requires HostProcessPlatform, but lines 114-117 only provide FileSystem.FileSystem and Path.Path. When spawn is called, the cached effect will fail at runtime with a service-not-found error.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/terminal/Layers/NodePTY.ts around line 114:
The cached `ensureNodePtySpawnHelperExecutableCached` effect is missing the `HostProcessPlatform` service. After the refactoring, `ensureNodePtySpawnHelperExecutable` now requires `HostProcessPlatform`, but lines 114-117 only provide `FileSystem.FileSystem` and `Path.Path`. When `spawn` is called, the cached effect will fail at runtime with a service-not-found error.
Evidence trail:
apps/server/src/terminal/Layers/NodePTY.ts lines 41-59 (ensureNodePtySpawnHelperExecutable yields HostProcessPlatform at line 43), lines 18-39 (resolveNodePtySpawnHelperPath yields HostProcessPlatform line 22 and HostProcessArchitecture line 23), lines 104-123 (layer construction with cached effect only providing FileSystem and Path at lines 115-116). apps/server/src/terminal/Services/PTY.ts line 52 (spawn interface: Effect<PtyProcess, PtySpawnError> with R=never). https://github.com/Effect-TS/effect-smol packages/effect/src/Effect.ts line 6986 (Effect.cached signature: <A, E, R>(self: Effect<A, E, R>) => Effect<Effect<A, E, R>>, inner effect preserves R).
0d4ef3a to
cf151de
Compare
github-actions
Bot
added
size:XXL
| hasExplicitDevUrl: input.devUrl !== undefined, | ||
| }); | ||
|
|
||
| const hostPlatform = yield* HostProcessPlatform; |
Contributor
There was a problem hiding this comment.
🟠 High scripts/dev-runner.ts:418
Changing baseEnv from process.env to {} combined with extendEnv: true breaks the delete operations in createDevRunnerEnv. The function deletes variables like T3CODE_MODE, T3CODE_NO_BROWSER, and T3CODE_HOST from output to prevent them from reaching the child, but since baseEnv is now an empty object and extendEnv: true copies process.env into the child separately, these deletes become no-ops. The child will incorrectly inherit those variables from process.env even when the code attempts to exclude them. For example, if T3CODE_MODE=desktop is set in the parent environment and the user runs dev:web mode, the child will receive T3CODE_MODE=desktop despite the delete statements.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file scripts/dev-runner.ts around line 418:
Changing `baseEnv` from `process.env` to `{}` combined with `extendEnv: true` breaks the `delete` operations in `createDevRunnerEnv`. The function deletes variables like `T3CODE_MODE`, `T3CODE_NO_BROWSER`, and `T3CODE_HOST` from `output` to prevent them from reaching the child, but since `baseEnv` is now an empty object and `extendEnv: true` copies `process.env` into the child separately, these deletes become no-ops. The child will incorrectly inherit those variables from `process.env` even when the code attempts to exclude them. For example, if `T3CODE_MODE=desktop` is set in the parent environment and the user runs `dev:web` mode, the child will receive `T3CODE_MODE=desktop` despite the delete statements.
Evidence trail:
...
cf151de to
c395992
Compare
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Empty env blocks PATH fallback
- Added Effect.map after orElseSucceed in both readCommandLookupEnv definitions to detect empty env objects and fall back to process.env, ensuring the nullish coalescing in resolveCommandPathForPlatform correctly uses the host PATH.
Or push these changes by commenting:
@cursor push c445860c82
Preview (c445860c82)
diff --git a/apps/server/src/process/externalLauncher.ts b/apps/server/src/process/externalLauncher.ts
--- a/apps/server/src/process/externalLauncher.ts
+++ b/apps/server/src/process/externalLauncher.ts
@@ -96,7 +96,10 @@
}).pipe(Config.map(compactEnv));
const readBrowserLaunchEnv = BrowserLaunchEnvConfig.pipe(Effect.orElseSucceed(() => ({})));
-const readCommandLookupEnv = CommandLookupEnvConfig.pipe(Effect.orElseSucceed(() => ({})));
+const readCommandLookupEnv = CommandLookupEnvConfig.pipe(
+ Effect.orElseSucceed(() => ({})),
+ Effect.map((env) => (Object.keys(env).length > 0 ? env : process.env)),
+);
function parseTargetPathAndPosition(target: string): Option.Option<TargetPathAndPosition> {
const match = TARGET_WITH_POSITION_PATTERN.exec(target);
diff --git a/apps/server/src/provider/providerMaintenance.ts b/apps/server/src/provider/providerMaintenance.ts
--- a/apps/server/src/provider/providerMaintenance.ts
+++ b/apps/server/src/provider/providerMaintenance.ts
@@ -35,7 +35,10 @@
PATHEXT: Config.string("PATHEXT").pipe(Config.option),
}).pipe(Config.map(compactEnv));
-const readCommandLookupEnv = CommandLookupEnvConfig.pipe(Effect.orElseSucceed(() => ({})));
+const readCommandLookupEnv = CommandLookupEnvConfig.pipe(
+ Effect.orElseSucceed(() => ({})),
+ Effect.map((env) => (Object.keys(env).length > 0 ? env : process.env)),
+);
export interface ProviderMaintenanceCapabilities {
readonly provider: ProviderDriverKind;You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit c395992. Configure here.
| } | ||
|
|
||
| function isExecutableFile( | ||
| const isExecutableFile = Effect.fn("shell.isExecutableFile")(function* ( |
Contributor
There was a problem hiding this comment.
🟡 Medium src/shell.ts:377
The execute permission check in isExecutableFile changed from accessSync(..., constants.X_OK) (verifies current user can execute) to (stat.mode & 0o111) !== 0 (any execute bit set). This returns true for files the current user cannot execute—for example, a file with mode 0o700 owned by root returns true when running as non-root, causing resolveCommandPathForPlatform to return paths that fail at actual execution time.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file packages/shared/src/shell.ts around line 377:
The execute permission check in `isExecutableFile` changed from `accessSync(..., constants.X_OK)` (verifies current user can execute) to `(stat.mode & 0o111) !== 0` (any execute bit set). This returns `true` for files the current user cannot execute—for example, a file with mode `0o700` owned by root returns `true` when running as non-root, causing `resolveCommandPathForPlatform` to return paths that fail at actual execution time.
Evidence trail:
- Diff confirmed the change: packages/shared/src/shell.ts, old code `accessSync(filePath, constants.X_OK)` replaced with `(stat.mode & 0o111) !== 0` at line 396 (commit REVIEWED_COMMIT vs MERGE_BASE)
- Effect FileSystem AccessFileOptions at https://github.com/Effect-TS/effect/blob/main/packages/platform/src/FileSystem.ts — only supports `ok`, `readable`, `writable` (no `executable`)
- `isExecutableFile` consumed by `resolveCommandPathForPlatform` at lines 399-445 (packages/shared/src/shell.ts)
c395992 to
1f88949
Compare
juliusmarminge
force-pushed
the
codex/effect-runtime-build-target
branch
2 times, most recently
from
4cc4960 to
93faea0
Compare
juliusmarminge
force-pushed
the
codex/effect-runtime-build-target
branch
from
93faea0 to
646023a
Compare
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
1 participant
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
@t3tools/shared/hostProcessreferences for host platform, architecture, and process envprocess.*accesspnpm-lock.yamlonly records the new@t3tools/tailscale -> @t3tools/sharedworkspace dependencyValidation
vp installvp install --frozen-lockfilevp checkvp run typecheckvp test scripts/lib/build-target-arch.test.ts scripts/build-desktop-artifact.test.ts packages/ssh/src/auth.test.ts packages/ssh/src/command.test.ts packages/ssh/src/tunnel.test.ts packages/tailscale/src/tailscale.test.tsNotes
vp test, but stopped it after it started collecting vendored.repostests and hit existing unrelated web test failures in saved-environment/thread-subscription/message timeline suites.process.*uses are mostly pure helper seams, terminal internals, config/bootstrap paths, test fixtures/examples, package dist output, and the shared reference defaults.Note
Medium Risk
Wide cross-cutting refactor touching spawn options, PATH/command resolution, and many formerly sync APIs; behavior should match defaults but regressions on Windows/WSL and provider child processes are plausible.
Overview
Introduces
HostProcessPlatformandHostProcessArchitectureas Effect context references (withprocess.*defaults) and routes platform/arch decisions through them across desktop, server, providers, terminals, diagnostics, telemetry, and build scripts.Server/runtime behavior shifts from ad hoc
process.platform/process.envreads toyield* HostProcessPlatform, Config-backed env (browser launch, terminals, maintenance), andextendEnvon spawns where only overrides are passed. Several helpers become Effect APIs (fixPath,readProcessRows,resolveBrowserLaunch,isCommandAvailableForPlatform,resolveWindowsEnvironment, etc.), andresolveFdPathnow requires an explicit platform argument.Desktop wires platform into
DesktopEnvironmentand window/menu code; standalone.mjsscripts usenode:oswith lint exceptions instead ofprocess.platform. Tests gainHostProcessPlatformlayers instead of mutatingprocess.platformor passing platform into every call.Reviewed by Cursor Bugbot for commit 646023a. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Replace direct
process.platform/process.archreads with injectableHostProcessPlatformandHostProcessArchitectureEffect servicesHostProcessPlatformandHostProcessArchitectureasContext.Referencevalues inpackages/shared/src/hostProcess.ts, defaulting toprocess.platform/process.archso existing runtimes are unaffected.process.platform/process.archreads across the server, desktop, SSH, terminal, provider, and script layers withyield* HostProcessPlatform/yield* HostProcessArchitecture, enabling injection in tests and controlled environments.t3code/no-global-process-runtimeoxlint rule that flags directprocess.platform,process.arch, andnode:osplatform/arch calls in application code, enforcing the pattern going forward.isCommandAvailable,resolveCommandPath,isWindowsCommandNotFound,fixPath,resolveWindowsEnvironment, etc.) toEffect.fngenerators that read platform from the service rather than globals.process.envreads in environment-sensitive paths (browser launch, command lookup, terminal env, SSH display) with scopedEffect Configproviders, reducing implicit global state.shell: trueonly on Windows (viaHostProcessPlatform); previously some paths used unconditional or inconsistent shell settings.extendEnv: truereplaces manualprocess.envspreading in several spawn sites.Macroscope summarized 646023a.