Open
Conversation
- Provider health monitoring (CLI probes for codex + claude) - Provider discovery service (capabilities, skills, models) - Managed terminal wrappers (CLI identity tracking scripts) - Project favicon route handler - Project metadata projection for orchestration - Git error types and repo detection utility
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 42 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR introduces foundational infrastructure across multiple domains: Git error handling and repository validation, project metadata orchestration with event-based snapshots, project favicon serving with fallback resolution, provider health monitoring for external CLIs, and managed terminal wrapper setup for CLI interception and configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as HTTP Server
participant FS as FileSystem
participant Resolver as Icon Resolver
Client->>Server: GET /api/project-favicon?cwd=...
Server->>FS: stat ./favicon.svg
alt Well-known favicon found
FS-->>Server: file exists
Server->>FS: read file
FS-->>Server: file content
Server-->>Client: 200 (image/svg+xml)
else Well-known favicon missing
FS-->>Server: ENOENT
Server->>Resolver: parse index.html for link rel=icon
Resolver->>FS: read index.html
FS-->>Resolver: file content
Resolver->>Resolver: extract href from link tag
Resolver->>FS: stat public/{href}
alt Icon file found
FS-->>Resolver: file exists
Resolver->>FS: read icon file
FS-->>Resolver: file content
Resolver-->>Server: icon path & content
Server-->>Client: 200 (icon content)
else Icon missing & fallback enabled
FS-->>Resolver: ENOENT
Resolver-->>Server: no icon found
Server-->>Client: 200 (fallback SVG)
else fallback=none
Server-->>Client: 204 (no content)
end
end
sequenceDiagram
participant Layer as ProviderHealthLive
participant Spawner as ChildProcessSpawner
participant CLI as CLI (codex/claude)
participant FS as FileSystem
participant Cache as Service Cache
Layer->>Layer: layer init (fork both providers)
par Codex Health Check
Layer->>Spawner: spawn codex --version
Spawner->>CLI: execute
CLI-->>Spawner: stdout + exit code
Spawner-->>Layer: {stdout, stderr, code}
Layer->>FS: read ~/.codex/config.toml
FS-->>Layer: config content
Layer->>Layer: parse model_provider
alt Custom model_provider detected
Layer->>Layer: skip auth probe, mark ready
else OpenAI (default)
Layer->>Spawner: spawn codex login status
Spawner->>CLI: execute
CLI-->>Spawner: JSON/text output
Spawner-->>Layer: result
Layer->>Layer: parseAuthStatusFromOutput
end
and Claude Health Check
Layer->>Spawner: spawn claude --version
Spawner->>CLI: execute
CLI-->>Spawner: stdout + exit code
Layer->>Spawner: spawn claude auth status
Spawner->>CLI: execute
CLI-->>Spawner: JSON output
Layer->>Layer: parseClaudeAuthStatusFromOutput
end
Layer->>Cache: store joined status results
Cache-->>Layer: ProviderHealthShape ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
vouch:trusted
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
apps/server/src/projectFaviconRoute.ts (1)
119-136: Extract shared candidate traversal to avoid security drift.
tryResolvedPathsandtryCandidatesduplicate near-identical stat/containment/serve flow. This already drifted from the Effect resolver hardening. A shared helper would reduce future divergence.Also applies to: 163-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/projectFaviconRoute.ts` around lines 119 - 136, The two functions tryResolvedPaths and tryCandidates duplicate the same containment check, fs.stat and serve logic (using isPathWithinProject and serveFaviconFile), which causes security drift; refactor by extracting a single helper (e.g., iterateAndServeCandidate or validateAndServeCandidate) that accepts an array of candidate paths and a callback/onExhausted, performs isPathWithinProject checks, fs.stat to ensure isFile, and calls serveFaviconFile for the first valid file, then replace both tryResolvedPaths and tryCandidates with calls to this shared helper to remove duplication and centralize hardening.apps/server/src/projectFaviconRoute.test.ts (1)
88-181: Add a regression test for symlink-based escape.Once the resolver hardening is in place, add a test ensuring a symlinked favicon that points outside
cwdis rejected (and falls back/204). This will lock in the security fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/projectFaviconRoute.test.ts` around lines 88 - 181, Add a regression test in projectFaviconRoute.test.ts that creates a projectDir via makeTempDir, creates an external file outside that cwd (e.g., externalDir/logo.svg), then fs.symlinkSync a link inside the project (e.g., public/brand/logo.svg -> external path) and calls the route via withRouteServer + request to /api/project-favicon?cwd=... to assert the symlinked favicon is rejected: when no fallback param expect a served fallback (response.contentType contains image/svg+xml and response.body contains 'data-fallback="project-favicon"'), and when calling with &fallback=none expect a 204 and empty body; use the same helpers (makeTempDir, withRouteServer, request) and paths used in existing tests so the spec locks in the resolver hardening.apps/server/src/provider/Layers/ProviderHealth.ts (2)
17-17: Remove unusedArrayimport.The
Arraymodule from Effect is imported but never used. Line 66 uses nativeArray.isArray(), not the Effect variant.♻️ Proposed fix
-import { Array, Effect, Fiber, FileSystem, Layer, Option, Path, Result, Stream } from "effect"; +import { Effect, Fiber, FileSystem, Layer, Option, Path, Result, Stream } from "effect";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ProviderHealth.ts` at line 17, The import list includes the unused Effect Array symbol; remove Array from the import statement (the list containing Array, Effect, Fiber, FileSystem, Layer, Option, Path, Result, Stream) so only used symbols remain—keep all other imports intact; confirm there are no references to the Effect Array API (the code uses native Array.isArray) and run a quick typecheck to ensure no other code depends on Array.
242-282: Consider extracting shared command runner.
runCodexCommandandrunClaudeCommandare nearly identical, differing only in the CLI binary name. A shared helper could reduce duplication.♻️ Example refactor
+const runCliCommand = (binary: string, args: ReadonlyArray<string>) => + Effect.gen(function* () { + const spawner = yield* ChildProcessSpawner.ChildProcessSpawner; + const command = ChildProcess.make(binary, [...args], { + shell: process.platform === "win32", + }); + + const child = yield* spawner.spawn(command); + + const [stdout, stderr, exitCode] = yield* Effect.all( + [ + collectStreamAsString(child.stdout), + collectStreamAsString(child.stderr), + child.exitCode.pipe(Effect.map(Number)), + ], + { concurrency: "unbounded" }, + ); + + return { stdout, stderr, code: exitCode } satisfies CommandResult; + }).pipe(Effect.scoped); + +const runCodexCommand = (args: ReadonlyArray<string>) => runCliCommand("codex", args); +const runClaudeCommand = (args: ReadonlyArray<string>) => runCliCommand("claude", args);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/provider/Layers/ProviderHealth.ts` around lines 242 - 282, runCodexCommand and runClaudeCommand are duplicated; extract a shared helper like makeRunner or runCommand(binary: string, args: ReadonlyArray<string>) that encapsulates getting ChildProcessSpawner.ChildProcessSpawner, creating the command via ChildProcess.make, spawning, collecting stdout/stderr with collectStreamAsString and mapping child.exitCode to Number, and applying .pipe(Effect.scoped); then have runCodexCommand and runClaudeCommand delegate to that helper (e.g., return runCommand("codex", args) and runCommand("claude", args)) while preserving the returned shape that satisfies CommandResult.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/git/isRepo.ts`:
- Around line 4-5: The isGitRepository function should not just check for the
existence of a .git path; update isGitRepository to validate that .git is a real
git repository by: (1) using fs.stat to confirm join(cwd, ".git") is a directory
(stat.isDirectory()), and if not a directory, (2) read the .git file to detect a
"gitdir: <path>" pointer and resolve that path and confirm it exists and
contains git metadata (e.g., HEAD and refs) before returning true; otherwise
return false. Target the isGitRepository function for these changes and ensure
you handle errors (ENOENT, permission) cleanly and synchronously/asynchronously
consistent with the surrounding code.
In `@apps/server/src/orchestration/projectMetadataProjection.ts`:
- Around line 42-78: The projection currently silently ignores
"project.meta-updated" and "project.deleted" events when
projectionProjectRepository.getById(...) returns None, which allows
advanceProjectMetadataSnapshotState(...) to mark the sequence applied and
permanently drift; fix by failing fast: in both the "project.meta-updated" and
"project.deleted" cases, detect Option.isNone(existingRow) and return/throw an
explicit projection error (or call the module's projection-failure helper)
rather than silently continuing, so the snapshot advancement is prevented until
the base row exists. Ensure you reference the existingRow variable and the
projectionProjectRepository.getById(...) call and produce a clear error (e.g.,
throw new Error or a ProjectionError) so the caller/runner can stop and
retry/backfill before advancing state.
In `@apps/server/src/projectFaviconRoute.ts`:
- Around line 68-71: The lexical containment check in isPathWithinProject allows
symlink escapes; update the route to resolve canonical paths like
ProjectFaviconResolver does: use fs.promises.realpath (or fs.realpathSync) to
get realProject = realpath(projectCwd) and realCandidate =
realpath(candidatePath) before checking containment, then ensure realCandidate
=== realProject or realCandidate.startsWith(realProject + path.sep) before
calling serveFaviconFile; also catch realpath errors (treat non-existent or
unreadable targets as disallowed) and keep the existing fallback behavior.
In `@apps/server/src/provider/Layers/ProviderHealth.test.ts`:
- Around line 4-5: The PlatformError import path is wrong; replace the import of
PlatformError from "effect/PlatformError" with the correct module
"@effect/platform/Error" so that references like PlatformError.systemError()
resolve; update the import statement that currently brings in PlatformError to
import { PlatformError } from "@effect/platform/Error" ensuring other imports
(e.g., ChildProcessSpawner) remain unchanged.
In `@apps/server/src/provider/Services/ProviderHealth.ts`:
- Around line 10-21: The file is missing the Context import and has an unused
ServiceMap import: add an import for Context from the "effect" package and
remove the unused ServiceMap import; specifically update the top imports so
ProviderHealth (which uses Context.Service) and ProviderHealthShape compile
correctly by importing Context and deleting ServiceMap from the import list.
In `@apps/server/src/terminal/managedTerminalWrappers.ts`:
- Around line 173-257: The script built by buildCodexWrapperScript can orphan
the background watcher (T3CODE_CODEX_START_WATCHER_PID) on signals; add a trap
right before the watcher is started that kills and waits for
$T3CODE_CODEX_START_WATCHER_PID on INT, TERM, HUP and EXIT (e.g. trap 'kill
"$T3CODE_CODEX_START_WATCHER_PID" >/dev/null 2>&1 || true; wait
"$T3CODE_CODEX_START_WATCHER_PID" 2>/dev/null || true' INT TERM HUP EXIT) so the
watcher is cleaned up whether the script exits normally or via a signal.
---
Nitpick comments:
In `@apps/server/src/projectFaviconRoute.test.ts`:
- Around line 88-181: Add a regression test in projectFaviconRoute.test.ts that
creates a projectDir via makeTempDir, creates an external file outside that cwd
(e.g., externalDir/logo.svg), then fs.symlinkSync a link inside the project
(e.g., public/brand/logo.svg -> external path) and calls the route via
withRouteServer + request to /api/project-favicon?cwd=... to assert the
symlinked favicon is rejected: when no fallback param expect a served fallback
(response.contentType contains image/svg+xml and response.body contains
'data-fallback="project-favicon"'), and when calling with &fallback=none expect
a 204 and empty body; use the same helpers (makeTempDir, withRouteServer,
request) and paths used in existing tests so the spec locks in the resolver
hardening.
In `@apps/server/src/projectFaviconRoute.ts`:
- Around line 119-136: The two functions tryResolvedPaths and tryCandidates
duplicate the same containment check, fs.stat and serve logic (using
isPathWithinProject and serveFaviconFile), which causes security drift; refactor
by extracting a single helper (e.g., iterateAndServeCandidate or
validateAndServeCandidate) that accepts an array of candidate paths and a
callback/onExhausted, performs isPathWithinProject checks, fs.stat to ensure
isFile, and calls serveFaviconFile for the first valid file, then replace both
tryResolvedPaths and tryCandidates with calls to this shared helper to remove
duplication and centralize hardening.
In `@apps/server/src/provider/Layers/ProviderHealth.ts`:
- Line 17: The import list includes the unused Effect Array symbol; remove Array
from the import statement (the list containing Array, Effect, Fiber, FileSystem,
Layer, Option, Path, Result, Stream) so only used symbols remain—keep all other
imports intact; confirm there are no references to the Effect Array API (the
code uses native Array.isArray) and run a quick typecheck to ensure no other
code depends on Array.
- Around line 242-282: runCodexCommand and runClaudeCommand are duplicated;
extract a shared helper like makeRunner or runCommand(binary: string, args:
ReadonlyArray<string>) that encapsulates getting
ChildProcessSpawner.ChildProcessSpawner, creating the command via
ChildProcess.make, spawning, collecting stdout/stderr with collectStreamAsString
and mapping child.exitCode to Number, and applying .pipe(Effect.scoped); then
have runCodexCommand and runClaudeCommand delegate to that helper (e.g., return
runCommand("codex", args) and runCommand("claude", args)) while preserving the
returned shape that satisfies CommandResult.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3f3c980-4bd3-4b26-8598-6508935ed4b8
📒 Files selected for processing (9)
apps/server/src/git/Errors.tsapps/server/src/git/isRepo.tsapps/server/src/orchestration/projectMetadataProjection.tsapps/server/src/projectFaviconRoute.test.tsapps/server/src/projectFaviconRoute.tsapps/server/src/provider/Layers/ProviderHealth.test.tsapps/server/src/provider/Layers/ProviderHealth.tsapps/server/src/provider/Services/ProviderHealth.tsapps/server/src/terminal/managedTerminalWrappers.ts
Comment on lines
+4
to
+5
| export function isGitRepository(cwd: string): boolean { | ||
| return existsSync(join(cwd, ".git")); |
There was a problem hiding this comment.
Harden repo detection to avoid false positives from arbitrary .git artifacts.
The current predicate returns true for any existing .git path, even malformed files. Since this result gates git workflows, a malformed .git can incorrectly enable downstream git operations.
🔧 Proposed hardening
-import { existsSync } from "node:fs";
+import { existsSync, lstatSync, readFileSync } from "node:fs";
import { join } from "node:path";
export function isGitRepository(cwd: string): boolean {
- return existsSync(join(cwd, ".git"));
+ const gitPath = join(cwd, ".git");
+ if (!existsSync(gitPath)) return false;
+
+ try {
+ const stat = lstatSync(gitPath);
+ if (stat.isDirectory()) return true;
+ if (stat.isFile()) {
+ return readFileSync(gitPath, "utf8").startsWith("gitdir:");
+ }
+ return false;
+ } catch {
+ return false;
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/git/isRepo.ts` around lines 4 - 5, The isGitRepository
function should not just check for the existence of a .git path; update
isGitRepository to validate that .git is a real git repository by: (1) using
fs.stat to confirm join(cwd, ".git") is a directory (stat.isDirectory()), and if
not a directory, (2) read the .git file to detect a "gitdir: <path>" pointer and
resolve that path and confirm it exists and contains git metadata (e.g., HEAD
and refs) before returning true; otherwise return false. Target the
isGitRepository function for these changes and ensure you handle errors (ENOENT,
permission) cleanly and synchronously/asynchronously consistent with the
surrounding code.
Comment on lines
+42
to
+78
| case "project.meta-updated": { | ||
| const existingRow = yield* input.projectionProjectRepository.getById({ | ||
| projectId: input.event.payload.projectId, | ||
| }); | ||
| if (Option.isSome(existingRow)) { | ||
| yield* input.projectionProjectRepository.upsert({ | ||
| ...existingRow.value, | ||
| ...(input.event.payload.title !== undefined | ||
| ? { title: input.event.payload.title } | ||
| : {}), | ||
| ...(input.event.payload.workspaceRoot !== undefined | ||
| ? { workspaceRoot: input.event.payload.workspaceRoot } | ||
| : {}), | ||
| ...(input.event.payload.defaultModelSelection !== undefined | ||
| ? { defaultModelSelection: input.event.payload.defaultModelSelection } | ||
| : {}), | ||
| ...(input.event.payload.scripts !== undefined | ||
| ? { scripts: input.event.payload.scripts } | ||
| : {}), | ||
| updatedAt: input.event.payload.updatedAt, | ||
| }); | ||
| } | ||
| break; | ||
| } | ||
|
|
||
| case "project.deleted": { | ||
| const existingRow = yield* input.projectionProjectRepository.getById({ | ||
| projectId: input.event.payload.projectId, | ||
| }); | ||
| if (Option.isSome(existingRow)) { | ||
| yield* input.projectionProjectRepository.upsert({ | ||
| ...existingRow.value, | ||
| deletedAt: input.event.payload.deletedAt, | ||
| updatedAt: input.event.payload.deletedAt, | ||
| }); | ||
| } | ||
| break; |
There was a problem hiding this comment.
Fail fast when update/delete arrives before the row exists.
If getById(...) returns None on Line 43 or Line 68, these branches silently drop the event. In the same module, advanceProjectMetadataSnapshotState(...) can still record the sequence as applied, which makes the projection drift permanent after replay gaps or partial backfills. Return an explicit projection error here, or otherwise prevent snapshot advancement until the base row exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/orchestration/projectMetadataProjection.ts` around lines 42 -
78, The projection currently silently ignores "project.meta-updated" and
"project.deleted" events when projectionProjectRepository.getById(...) returns
None, which allows advanceProjectMetadataSnapshotState(...) to mark the sequence
applied and permanently drift; fix by failing fast: in both the
"project.meta-updated" and "project.deleted" cases, detect
Option.isNone(existingRow) and return/throw an explicit projection error (or
call the module's projection-failure helper) rather than silently continuing, so
the snapshot advancement is prevented until the base row exists. Ensure you
reference the existingRow variable and the
projectionProjectRepository.getById(...) call and produce a clear error (e.g.,
throw new Error or a ProjectionError) so the caller/runner can stop and
retry/backfill before advancing state.
Comment on lines
+68
to
+71
| function isPathWithinProject(projectCwd: string, candidatePath: string): boolean { | ||
| const relative = path.relative(path.resolve(projectCwd), path.resolve(candidatePath)); | ||
| return relative === "" || (!relative.startsWith("..") && !path.isAbsolute(relative)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify canonical-path guard parity between the two implementations.
rg -n -C2 'realPath|realpath|isPathWithinProject|serveFaviconFile' \
apps/server/src/project/Layers/ProjectFaviconResolver.ts \
apps/server/src/projectFaviconRoute.tsRepository: aaditagrawal/t3code
Length of output: 4506
🏁 Script executed:
#!/bin/bash
# Check serveFaviconFile function and surrounding context
sed -n '73,90p' apps/server/src/projectFaviconRoute.ts
echo "---"
# Check full context at lines 120-140
sed -n '120,140p' apps/server/src/projectFaviconRoute.ts
echo "---"
# Check full context at lines 165-180
sed -n '165,180p' apps/server/src/projectFaviconRoute.tsRepository: aaditagrawal/t3code
Length of output: 1655
Block symlink escape before serving favicon files.
The isPathWithinProject() function (line 68-71) enforces only lexical path containment without resolving symlinks. At lines 125-134 and 169-178, the code checks containment and immediately serves the file without canonical-path validation. A symlink inside projectCwd (e.g., favicon.svg -> /etc/passwd) passes the lexical check and leaks files outside the project when serveFaviconFile() reads its target.
The ProjectFaviconResolver.ts implementation correctly uses realPath() to resolve both projectCwd and candidate paths before serving, preventing this attack. Align this route handler with that pattern.
Suggested fix
function isPathWithinProject(projectCwd: string, candidatePath: string): boolean {
const relative = path.relative(path.resolve(projectCwd), path.resolve(candidatePath));
return relative === "" || (!relative.startsWith("..") && !path.isAbsolute(relative));
}
+
+function withValidatedRealPath(
+ projectCwd: string,
+ candidate: string,
+ onValid: (realCandidate: string) => void,
+ onInvalid: () => void,
+): void {
+ fs.realpath(projectCwd, (rootErr, realProjectCwd) => {
+ const effectiveRoot = rootErr ? path.resolve(projectCwd) : realProjectCwd;
+ fs.realpath(candidate, (candidateErr, realCandidate) => {
+ if (candidateErr || !isPathWithinProject(effectiveRoot, realCandidate)) {
+ onInvalid();
+ return;
+ }
+ onValid(realCandidate);
+ });
+ });
+}
@@
fs.stat(candidate, (err, stats) => {
if (err || !stats?.isFile()) {
tryResolvedPaths(paths, index + 1, onExhausted);
return;
}
- serveFaviconFile(candidate, res);
+ withValidatedRealPath(
+ projectCwd,
+ candidate,
+ (safePath) => serveFaviconFile(safePath, res),
+ () => tryResolvedPaths(paths, index + 1, onExhausted),
+ );
});
};
@@
fs.stat(candidate, (err, stats) => {
if (err || !stats?.isFile()) {
tryCandidates(index + 1);
return;
}
- serveFaviconFile(candidate, res);
+ withValidatedRealPath(
+ projectCwd,
+ candidate,
+ (safePath) => serveFaviconFile(safePath, res),
+ () => tryCandidates(index + 1),
+ );
});
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/projectFaviconRoute.ts` around lines 68 - 71, The lexical
containment check in isPathWithinProject allows symlink escapes; update the
route to resolve canonical paths like ProjectFaviconResolver does: use
fs.promises.realpath (or fs.realpathSync) to get realProject =
realpath(projectCwd) and realCandidate = realpath(candidatePath) before checking
containment, then ensure realCandidate === realProject or
realCandidate.startsWith(realProject + path.sep) before calling
serveFaviconFile; also catch realpath errors (treat non-existent or unreadable
targets as disallowed) and keep the existing fallback behavior.
Comment on lines
+4
to
+5
| import * as PlatformError from "effect/PlatformError"; | ||
| import { ChildProcessSpawner } from "effect/unstable/process"; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
effect-ts PlatformError import path effect/PlatformError
💡 Result:
The import path for PlatformError in Effect-TS (Effect Platform) is from "@effect/platform/Error". Usage example: import { PlatformError, isPlatformError } from "@effect/platform/Error" PlatformError is a union type: type PlatformError = BadArgument | SystemError It includes refinements like isPlatformError(u: unknown): u is PlatformError, and is used in platform services like FileSystem, Terminal, etc., where operations can fail with PlatformError (e.g., Effect.Effect<void, PlatformError, FileSystem>).
Citations:
- 1: https://effect-ts.github.io/effect/platform/Error.ts.html
- 2: https://effect-ts.github.io/effect/docs/platform
- 3: https://effect.website/docs/platform/platformlogger/
- 4: https://effect.website/docs/platform/terminal/
🏁 Script executed:
sed -n '1,10p' apps/server/src/provider/Layers/ProviderHealth.test.tsRepository: aaditagrawal/t3code
Length of output: 468
🏁 Script executed:
grep -n "PlatformError" apps/server/src/provider/Layers/ProviderHealth.test.tsRepository: aaditagrawal/t3code
Length of output: 158
Fix incorrect PlatformError import path.
The import at line 4 should be changed from effect/PlatformError to @effect/platform/Error. The current path does not resolve correctly in Effect-TS and will cause a module resolution error. Since PlatformError.systemError() is used at line 52, this import must be corrected.
Correct import:
import { PlatformError } from "@effect/platform/Error";
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/provider/Layers/ProviderHealth.test.ts` around lines 4 - 5,
The PlatformError import path is wrong; replace the import of PlatformError from
"effect/PlatformError" with the correct module "@effect/platform/Error" so that
references like PlatformError.systemError() resolve; update the import statement
that currently brings in PlatformError to import { PlatformError } from
"@effect/platform/Error" ensuring other imports (e.g., ChildProcessSpawner)
remain unchanged.
Comment on lines
+173
to
+257
| function buildCodexWrapperScript(input: { | ||
| codexHomeDir: string; | ||
| notifyHookPath: string; | ||
| targetPath: string; | ||
| }): string { | ||
| const { codexHomeDir, notifyHookPath, targetPath } = input; | ||
| return [ | ||
| `export CODEX_HOME=${shellQuote(codexHomeDir)}`, | ||
| `if [ -f ${shellQuote(notifyHookPath)} ]; then`, | ||
| " export CODEX_TUI_RECORD_SESSION=1", | ||
| ' if [ -z "${CODEX_TUI_SESSION_LOG_PATH:-}" ]; then', | ||
| ' _t3code_codex_ts="$(date +%s 2>/dev/null || echo "$$")"', | ||
| ' export CODEX_TUI_SESSION_LOG_PATH="${TMPDIR:-/tmp}/t3code-codex-session-$$_${_t3code_codex_ts}.jsonl"', | ||
| " fi", | ||
| " (", | ||
| ' _t3code_log="$CODEX_TUI_SESSION_LOG_PATH"', | ||
| ` _t3code_notify=${shellQuote(notifyHookPath)}`, | ||
| ' _t3code_last_turn_id=""', | ||
| ' _t3code_last_approval_id=""', | ||
| ' _t3code_last_exec_call_id=""', | ||
| " _t3code_approval_fallback_seq=0", | ||
| "", | ||
| " _t3code_emit_event() {", | ||
| ' _t3code_event="$1"', | ||
| ` _t3code_payload=$(printf '{"hook_event_name":"%s"}' "$_t3code_event")`, | ||
| ' "$_t3code_notify" "$_t3code_payload" >/dev/null 2>&1 || true', | ||
| " }", | ||
| "", | ||
| " _t3code_i=0", | ||
| ' while [ ! -f "$_t3code_log" ] && [ "$_t3code_i" -lt 200 ]; do', | ||
| " _t3code_i=$((_t3code_i + 1))", | ||
| " sleep 0.05", | ||
| " done", | ||
| ' if [ ! -f "$_t3code_log" ]; then', | ||
| " exit 0", | ||
| " fi", | ||
| "", | ||
| ' tail -n 0 -F "$_t3code_log" 2>/dev/null | while IFS= read -r _t3code_line; do', | ||
| ' case "$_t3code_line" in', | ||
| ` *'"dir":"to_tui"'*'"kind":"codex_event"'*'"msg":{"type":"task_started"'*)`, | ||
| ` _t3code_turn_id=$(printf '%s\n' "$_t3code_line" | awk -F'"turn_id":"' 'NF > 1 { sub(/".*/, "", $2); print $2; exit }')`, | ||
| ' [ -n "$_t3code_turn_id" ] || _t3code_turn_id="task_started"', | ||
| ' if [ "$_t3code_turn_id" != "$_t3code_last_turn_id" ]; then', | ||
| ' _t3code_last_turn_id="$_t3code_turn_id"', | ||
| ' _t3code_emit_event "Start"', | ||
| " fi", | ||
| " ;;", | ||
| ` *'"dir":"to_tui"'*'"kind":"codex_event"'*'"msg":{"type":"'*'_approval_request"'*)`, | ||
| ` _t3code_approval_id=$(printf '%s\n' "$_t3code_line" | awk -F'"id":"' 'NF > 1 { sub(/".*/, "", $2); print $2; exit }')`, | ||
| ` [ -n "$_t3code_approval_id" ] || _t3code_approval_id=$(printf '%s\n' "$_t3code_line" | awk -F'"approval_id":"' 'NF > 1 { sub(/".*/, "", $2); print $2; exit }')`, | ||
| ` [ -n "$_t3code_approval_id" ] || _t3code_approval_id=$(printf '%s\n' "$_t3code_line" | awk -F'"call_id":"' 'NF > 1 { sub(/".*/, "", $2); print $2; exit }')`, | ||
| ' if [ -z "$_t3code_approval_id" ]; then', | ||
| " _t3code_approval_fallback_seq=$((_t3code_approval_fallback_seq + 1))", | ||
| ' _t3code_approval_id="approval_request_${_t3code_approval_fallback_seq}"', | ||
| " fi", | ||
| ' if [ "$_t3code_approval_id" != "$_t3code_last_approval_id" ]; then', | ||
| ' _t3code_last_approval_id="$_t3code_approval_id"', | ||
| ' _t3code_emit_event "PermissionRequest"', | ||
| " fi", | ||
| " ;;", | ||
| ` *'"dir":"to_tui"'*'"kind":"codex_event"'*'"msg":{"type":"exec_command_begin"'*)`, | ||
| ` _t3code_exec_call_id=$(printf '%s\n' "$_t3code_line" | awk -F'"call_id":"' 'NF > 1 { sub(/".*/, "", $2); print $2; exit }')`, | ||
| ' if [ -n "$_t3code_exec_call_id" ]; then', | ||
| ' if [ "$_t3code_exec_call_id" != "$_t3code_last_exec_call_id" ]; then', | ||
| ' _t3code_last_exec_call_id="$_t3code_exec_call_id"', | ||
| ' _t3code_emit_event "Start"', | ||
| " fi", | ||
| " else", | ||
| ' _t3code_emit_event "Start"', | ||
| " fi", | ||
| " ;;", | ||
| " esac", | ||
| " done", | ||
| " ) &", | ||
| " T3CODE_CODEX_START_WATCHER_PID=$!", | ||
| "fi", | ||
| `${shellQuote(targetPath)} --enable codex_hooks -c ${shellQuote(`notify=["bash",${JSON.stringify(notifyHookPath)}]`)} "$@"`, | ||
| "_t3code_status=$?", | ||
| 'if [ -n "${T3CODE_CODEX_START_WATCHER_PID:-}" ]; then', | ||
| ' kill "$T3CODE_CODEX_START_WATCHER_PID" >/dev/null 2>&1 || true', | ||
| ' wait "$T3CODE_CODEX_START_WATCHER_PID" 2>/dev/null || true', | ||
| "fi", | ||
| 'exit "$_t3code_status"', | ||
| ].join("\n"); | ||
| } |
There was a problem hiding this comment.
Consider adding signal trap for background watcher cleanup.
The background session watcher (spawned at line 246) is cleaned up on normal exit (lines 251-254), but if the script receives a signal (SIGTERM/SIGINT) during codex execution, the watcher process will be orphaned.
🛡️ Proposed fix to add signal trap
Add a trap before spawning the background process:
) &
T3CODE_CODEX_START_WATCHER_PID=$!
+ trap 'kill "$T3CODE_CODEX_START_WATCHER_PID" 2>/dev/null || true' INT TERM
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/terminal/managedTerminalWrappers.ts` around lines 173 - 257,
The script built by buildCodexWrapperScript can orphan the background watcher
(T3CODE_CODEX_START_WATCHER_PID) on signals; add a trap right before the watcher
is started that kills and waits for $T3CODE_CODEX_START_WATCHER_PID on INT,
TERM, HUP and EXIT (e.g. trap 'kill "$T3CODE_CODEX_START_WATCHER_PID" >/dev/null
2>&1 || true; wait "$T3CODE_CODEX_START_WATCHER_PID" 2>/dev/null || true' INT
TERM HUP EXIT) so the watcher is cleaned up whether the script exits normally or
via a signal.
- Define ServerProviderStatus/ServerProviderStatusState locally in Services/ProviderHealth instead of importing non-existent names from contracts - Fix Context.Service usage in ProviderHealth service definition - Add missing unref property to ChildProcessHandle mock in test - Add terminalThreads.ts from shared terminal utilities and register its subpath export in @t3tools/shared
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
Ports server-side utilities from the dpcode fork.
Test plan
Summary by CodeRabbit
New Features
Tests