chore: enable Effect linting across monorepo - #6304
Conversation
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@edcae5572d70dbbdadfd8c6c452b0d436a28ce2ePreview package for commit |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edcae5572d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -1,106 +0,0 @@ | |||
| import { randomUUID } from "node:crypto"; | |||
There was a problem hiding this comment.
Running test:e2e:live now fails before collecting tests because vitest.live.config.ts still declares globalSetup: ["tests/live-setup.ts"], while this change deletes that module without replacing or updating the configuration. This also disables the scheduled live workflow and removes the provisioning and guaranteed teardown that the suite depends on. Restore the setup module or point the configuration at an equivalent implementation.
AGENTS.md reference: apps/cli-e2e/AGENTS.md:L148-L149
Useful? React with 👍 / 👎.
| const canAccessSocket = (socket: string) => | ||
| socketAccess !== undefined | ||
| ? Effect.sync(() => socketAccess(socket)) | ||
| : fs.access(socket).pipe( |
There was a problem hiding this comment.
When a candidate Podman socket exists but is not readable and writable by the current user—for example, a root-owned /run/podman/podman.sock during a rootless Podman run—FileSystem.access(socket) without access options checks only existence. The previous implementation explicitly requested R_OK | W_OK, so this now selects and mounts an unusable socket, configures Vector with docker_logs, and can leave Vector unhealthy instead of falling back to internal_logs. Pass the readable and writable access options here; the socketAccess test seam currently masks this production-path regression.
Useful? React with 👍 / 👎.
| ); | ||
| } | ||
| yield* fs.makeDirectory(keyDir, { recursive: true }); | ||
| const nextIndex = yield* nextFixtureIndex(keyDir); |
There was a problem hiding this comment.
When two record-mode requests for the same normalized endpoint finish concurrently, both asynchronous recordFixture executions can read the same directory contents here, choose the same nextIndex, and overwrite the same request/response pair. The old implementation performed this whole operation synchronously, so its numbered queue could not race. Protect each key's cleanup/index/write sequence with a per-key semaphore or allocate indices from serialized in-memory state; otherwise recording concurrent API or Docker traffic silently drops interactions and produces replay fixtures with the wrong sequence.
AGENTS.md reference: apps/cli-e2e/AGENTS.md:L28-L34
Useful? React with 👍 / 👎.
| dockerHost: env.get("DOCKER_HOST"), | ||
| dockerContext: env.get("DOCKER_CONTEXT"), | ||
| dockerConfig: env.get("DOCKER_CONFIG"), | ||
| home: env.get("HOME"), |
There was a problem hiding this comment.
On Windows when DOCKER_CONFIG and HOME are unset, as is common when invoking the CLI from PowerShell where the home directory is exposed through USERPROFILE, this replacement for os.homedir() cannot locate ~/.docker. Consequently an active remote Docker context from config.json is ignored and legacyGetHostname falls back to 127.0.0.1, causing commands such as status, storage, and local database operations to report or connect to the wrong host. Resolve the platform home directory through an injected platform service or include the Windows home environment fallback.
Useful? React with 👍 / 👎.
| const decodedHeader = yield* Schema.decodeEffect(Schema.fromJsonString(Schema.Unknown))( | ||
| new TextDecoder().decode(base64UrlToBytes(parts[0]!)), |
There was a problem hiding this comment.
For a three-part token whose header is invalid base64, base64UrlToBytes calls atob and throws before the schema Effect is constructed. That throw becomes a defect, so the later Effect.mapError and the caller's typed Effect.catch do not handle it; a user-supplied malformed Authorization token can therefore reject the edge-runtime request handler instead of returning the established 401 invalid-format response. Wrap the base64 decode in Effect.try and map it to JwtDecodeError.
AGENTS.md reference: AGENTS.md:L147-L152
Useful? React with 👍 / 👎.
| if (forwardedSignal !== undefined) { | ||
| return { _tag: "signal" as const, signal: forwardedSignal }; |
There was a problem hiding this comment.
When the shim receives a forwarded signal but the compiled child handles it and exits normally—for example, a foreground command performs graceful SIGTERM cleanup and returns exit code 0—this branch ignores the successful child.exitCode and self-terminates with the earlier signal. The previous shim mirrored the child's actual (code, signal) result, so supervisors now observe 143/130 instead of the child's exit status. Use forwardedSignal only to forward the signal; decide the shim outcome from how the child ultimately exited.
Useful? React with 👍 / 👎.
| const ClaimRecordSchema = Schema.fromJsonString( | ||
| Schema.Struct({ pid: Schema.Finite, token: Schema.String }), | ||
| ); |
There was a problem hiding this comment.
A malformed or stale claim containing a finite but non-positive PID now passes this schema, whereas the previous parser required Number.isInteger(pid) && pid > 0. On Unix, process.kill(0, 0) checks the current process group and negative PIDs target process groups, so such a record can be classified as a live owner indefinitely instead of aging out after 30 seconds, permanently making the corresponding port appear unavailable. Refine the schema or filter the decoded value to a positive integer before treating it as a claim record.
AGENTS.md reference: AGENTS.md:L184-L186
Useful? React with 👍 / 👎.
| yield* child.exitCode.pipe( | ||
| Effect.map((code) => ({ code, signal: null }) satisfies ChildExit), | ||
| Effect.flatMap((exit) => Deferred.succeed(childExit, exit)), | ||
| Effect.forkChild, |
There was a problem hiding this comment.
When the supervised child exits because of a signal—such as SIGKILL from an OOM or the default action for SIGTERM—the Effect process API fails child.exitCode rather than yielding a numeric code. This fiber only completes childExit on success, so an unexpected signal death leaves the supervisor running forever and the orchestrator continues to believe the service is alive; during requested shutdown it instead waits through the graceful and force-kill timeouts. Observe the full exit/cause and complete the Deferred for both normal and signaled termination.
Useful? React with 👍 / 👎.
1 participant
Summary
Reviewer context
Enabling the recommended rules surfaced diagnostics across every TypeScript workspace, so this is intentionally a broad migration. It also replaces timing-sensitive subprocess and filesystem test coordination encountered while validating the Effect-native changes.