refactor: resolve host process state through Effect#2959
Open
juliusmarminge wants to merge 5 commits into
Open
refactor: resolve host process state through Effect#2959juliusmarminge wants to merge 5 commits into
juliusmarminge wants to merge 5 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 refactor converting platform/architecture detection to Effect-based dependency injection across 60+ files. Unresolved review comments identify functional bugs: a HIGH severity issue where child process environment variable filtering is broken, and a MEDIUM severity issue where execute permission checks have incorrect semantics. 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
3 times, most recently
from
93faea0 to
646023a
Compare
… args Review follow-ups for the host-process refactor: - terminals inherit the full host env again (blocklist semantics) instead of a Config-backed allowlist; `options.env` is the test seam - drop cmd.exe shell mode for real executables (git, ssh, tailscale, powershell.exe, node test peers) — shell-mode arg joining re-tokenizes pipes and paths with spaces; resolve `.exe` names per platform instead - add escapeWindowsShellArg/sanitizeShellModeArgs (cross-spawn style) and apply them at every spawn that keeps shell mode for npm `.cmd` shims (provider binaries, ACP agents, codex app-server, editor launches) - restore the electron-builder env copy + empty-string scrub in build-desktop-artifact (set-but-empty CSC_* vars are treated as enabled) - downgrade fixPath defects to warnings on the win32 branch so PATH hydration can never fail server startup - collapse mergeProviderInstanceEnvironmentEffect back to the sync helper and drop the duplicate getAvailableEditors alias - migrate GrokProvider (new on main) to HostProcessPlatform + sanitizer Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
juliusmarminge
force-pushed
the
codex/effect-runtime-build-target
branch
from
646023a to
8e368a7
Compare
…tandalone effect-acp and effect-codex-app-server should not depend on @t3tools/shared. Remove layerCommand (the only consumer of the platform/sanitizer helpers) and have callers spawn the child themselves and hand a handle to layerChildProcess — the same shape effect-acp already uses. - CodexProvider's probe now spawns `codex app-server` itself (host platform + sanitized shell-mode args + spawn-error mapping) and builds the client via layerChildProcess - probe/example scripts read the platform from node:os like other standalone scripts - both package manifests no longer reference @t3tools/shared; lockfile again only records the deliberate @t3tools/tailscale -> shared link Co-Authored-By: Claude Opus 4.8 <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
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 changes to process spawning, PATH hydration, and provider/terminal startup on all OSes; Windows shell and env behavior is intentionally adjusted, so regressions are possible outside pure refactors.
Overview
Introduces
HostProcessPlatformandHostProcessArchitectureas EffectContext.Referenceservices (defaulting toprocess.platform/process.arch) and threads them through desktop, server, providers, terminals, diagnostics, and build scripts instead of reading globals directly. Standalone Node scripts without an Effect runtime keep usingnode:oswith explicit lint exceptions.Platform-sensitive behavior now resolves inside Effect layers: environment labels, bootstrap FD paths, editor/browser launch, command-on-PATH checks, provider CLI spawns, OpenCode/Codex/ACP runtimes, server CLI
shellflags, and telemetry properties. Several helpers becomeEffect.fn(fixPath,readProcessRows,isWindowsCommandNotFound,resolveBrowserLaunch, etc.), and tests inject platform viaLayer.succeed(HostProcessPlatform, …)instead of mutatingprocess.platform.Child-process changes on Windows: provider and tooling spawns use
sanitizeShellModeArgswhenshell: true(npm.cmdshims); real executables likegit,powershell.exe, and diagnosticpsavoid shell mode so cmd.exe does not re-tokenize complex arguments. Spawn sites increasingly useextendEnv: truerather than copyingprocess.env. PATH hydration on Windows isEffect-based with defect handling so startup does not crash. Command lookup and browser-launch env move toward scoped EffectConfiginstead of implicitprocess.envreads.Reviewed by Cursor Bugbot for commit 8e368a7. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Replace direct
process.platform/process.archaccess with Effect-injectedHostProcessPlatformandHostProcessArchitectureservicesHostProcessPlatformandHostProcessArchitectureas EffectContext.Referenceservices inpackages/shared/src/hostProcess.ts, defaulting toprocess.platform/process.archbut overridable via layers.process.platform/process.archglobals to these services.sanitizeShellModeArgsandescapeWindowsShellArginpackages/shared/src/shell.tsto correctly escape arguments for Windows cmd.exe shell-mode spawning; these are now applied wherever shell-mode processes are launched.isCommandAvailable,resolveCommandPath, andresolveWindowsEnvironmentto Effect-based APIs (isCommandAvailableForPlatform,resolveCommandPathForPlatform) that depend onFileSystem/Pathservices and fail with typedCommandResolutionError.t3code/no-global-process-runtimethat forbids direct reads ofprocess.platformandprocess.arch(andnode:osequivalents) outside the designated host process reference file.isExecutableFilenow checks any execute bit rather thanX_OK, so files executable only for another user may be treated as executable during PATH scanning on non-Windows systems.Macroscope summarized b6da521.