[codex] Persist project SSH terminal sessions#300
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📜 Recent review details⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🧰 Additional context used📓 Path-based instructions (8)**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{sh,bash,py,js,ts,jsx,tsx,go,java,rb,php}📄 CodeRabbit inference engine (Custom checks)
Files:
**/*.{py,js,ts,jsx,tsx,go,java,rb,php,sh,bash,c,cpp}📄 CodeRabbit inference engine (Custom checks)
Files:
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,sh,bash,yml,yaml,json,env*,toml,cfg,config,dockerfile,dockerignore}📄 CodeRabbit inference engine (Custom checks)
Files:
**/*⚙️ CodeRabbit configuration file
Files:
**/*.test.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{test,spec}.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🔇 Additional comments (25)
📝 WalkthroughSummary by CodeRabbit
WalkthroughДобавлена durable-архитектура для терминальных сессий: схемы и операции чтения/записи ChangesPersistent Terminal Sessions
Sequence DiagramssequenceDiagram
participant App as Application
participant Runtime as Session Runtime
participant Durable as Durable Storage
participant PTY as PTY/tmux Process
App->>Runtime: createTerminalSession(projectId, options)
Runtime->>Runtime: registerRecord(sessionId, requestId)
Runtime->>Durable: upsertDurableSession (with tmuxName)
Runtime->>PTY: startTerminalPty (renderRemoteTmuxCommand)
PTY->>PTY: execute tmux session
Note over Runtime,Durable: During runtime changes
Runtime->>Durable: updateSession → patchDurableSession
Note over Runtime,Durable: On WebSocket connection
App->>Runtime: WebSocket upgrade (sessionId from URL)
Runtime->>Runtime: resolveParsedTerminalRecord
alt Session in runtime memory
Runtime->>App: found TerminalRecord
else Session not in memory
Runtime->>Durable: hydrateTerminalRecordByProjectId
Durable->>Runtime: TerminalRecord from disk
Runtime->>App: hydrated TerminalRecord
end
App->>Runtime: handleUpgrade (now verified)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/api/src/services/terminal-sessions.ts`:
- Around line 810-819: The current renderRemoteTmuxCommand uses a hardcoded
tmuxMissingMessage and only detects tmux at connection time; add a proactive
tmux availability probe during project startup (e.g., in prepareProjectSsh or
probeProjectSshReady) that runs a container exec "command -v tmux" (see
suggested probeTmuxAvailable behavior) and returns a boolean so the UI can warn
early when tmux is absent; also soften or contextualize tmuxMissingMessage used
by renderRemoteTmuxCommand (or make it configurable) to avoid confusing “rebuild
image” instructions for freshly started projects.
- Line 23: Remove direct synchronous node:fs imports and calls in
readTerminalSessionFile, writeTerminalSessionFile, upsertDurableSession,
patchDurableSession, and deleteDurableSession; instead make these persistence
functions return Effect types and call the FileSystem API from `@effect/platform`
(FileSystem.FileSystem) via the Effect-TS Layer pattern (inject the FileSystem
dependency), using the asynchronous/read/write/mkdir equivalents provided by
that interface, and compose errors into the returned Effect; ensure no direct
fs.*Sync usage remains and that the functions' signatures reflect Effect<...> so
they can be provided the FileSystem layer by callers.
- Around line 171-172: The terminal session handlers are vulnerable to path
traversal because terminalSessionStatePath concatenates an unvalidated
projectId; fix by validating the project before any filesystem use: call
getProject(projectId) (as other /projects/* routes do) at the start of
createTerminalSession, listProjectTerminalSessions, and deleteTerminalSession to
ensure the project exists and reject invalid IDs, and additionally harden
terminalSessionStatePath/writeTerminalSessionFile by resolving the final
statePath with path.resolve(projectRoot, ".orch", "state",
"terminal-sessions.json") (or verify path.resolve(projectRoot,
terminalSessionStateRelativePath) startsWith the expected projectRoot) so writes
cannot escape the project directory.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8bd27f68-2a9b-4a30-8cfd-87fb112e4e30
📒 Files selected for processing (6)
packages/api/src/services/terminal-sessions.tspackages/api/tests/terminal-sessions.test.tspackages/app/src/lib/core/templates/dockerfile.tspackages/app/tests/docker-git/actions-projects.test.tspackages/lib/src/core/templates/dockerfile.tspackages/lib/tests/core/templates.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Implement Functional Core, Imperative Shell (FCIS) pattern: CORE layer contains only pure functions with immutable data and mathematical operations; SHELL layer isolates all effects (IO, network, database). Strict dependency direction: SHELL → CORE (never reverse).
Never useany,unknown,eslint-disable,ts-ignore, orastype assertions (except in rigorously justified cases with documentation). Always use exhaustive union type analysis through.exhaustive()pattern matching.
All external dependencies must be wrapped through typed interfaces and injected via Effect-TS Layer pattern. Never call external services directly from CORE functions.
Use monadic composition with Effect-TS for all effects:Effect<Success, Error, Requirements>. Compose effects throughpipe()andEffect.flatMap(). Implement dependency injection via Layer pattern. Handle errors without try/catch blocks.
All functions must be pure in the CORE layer: no side effects (logging, console output, IO operations, mutations). Separate all side effects into the SHELL layer.
Use exhaustive pattern matching with Effect.Match instead of switch statements. Example:Match.value(item).pipe(Match.when(...), Match.exhaustive).
Document all functions with comprehensive TSDoc including:@pure(true/false),@effect(required services),@invariant(mathematical invariants),@precondition,@postcondition,@complexity(time and space),@throwsNever (errors must be typed in Effect).
Use functional comment markers for code clarity: CHANGE (brief description), WHY (mathematical/architectural justification), QUOTE(ТЗ) (requirement citation), REF (RTM or message ID), SOURCE (external source with quote), FORMAT THEOREM (∀x ∈ Domain: P(x) → Q(f(x))), PURITY (CORE|SHELL), EFFECT (Effect type signature), INVARIANT (mathematical invariant), COMPLEXITY (time/space).
Define all external service dependencies as Context.Tag classes with fully typed methods returning Effect types. Example: `class Da...
Files:
packages/lib/tests/core/templates.test.tspackages/lib/src/core/templates/dockerfile.tspackages/app/src/lib/core/templates/dockerfile.tspackages/app/tests/docker-git/actions-projects.test.tspackages/api/tests/terminal-sessions.test.tspackages/api/src/services/terminal-sessions.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Implement property-based testing using fast-check for mathematical properties and invariants. Example:fc.property(fc.array(messageArbitrary), (messages) => isChronologicallySorted(sortMessagesByTimestamp(messages))).
Mock external dependencies in unit tests using Effect's testing utilities. Run tests without Effect runtime for speed. Example:Effect.provide(MockService), Effect.runPromise.
Files:
packages/lib/tests/core/templates.test.tspackages/app/tests/docker-git/actions-projects.test.tspackages/api/tests/terminal-sessions.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Forbidden constructs in CORE code:any,eslint-disable,ts-ignore,async/await, raw Promise chains (then/catch),Promise.all,try/catchfor logic control,console.*, switch statements (use Match with .exhaustive() instead)
All functions must use Effect-TS for composing effects:Effect<Success, Error, Requirements>. No direct async/await, Promise chains, or try/catch in product logic.
Functional comments must include: CHANGE, WHY, QUOTE(ТЗ) or n/a, REF, SOURCE or n/a, FORMAT THEOREM, PURITY (CORE|SHELL), EFFECT signature for SHELL functions, INVARIANT, and COMPLEXITY.
All data mutations must use immutable patterns (ReadonlyArray, readonly properties, Object.freeze); mutation in SHELL only when absolutely necessary and documented.
Files:
packages/lib/tests/core/templates.test.tspackages/lib/src/core/templates/dockerfile.tspackages/app/src/lib/core/templates/dockerfile.tspackages/app/tests/docker-git/actions-projects.test.tspackages/api/tests/terminal-sessions.test.tspackages/api/src/services/terminal-sessions.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Property-based tests (fast-check) must verify mathematical invariants; unit tests must use Effect test utilities without async/await.
Files:
packages/lib/tests/core/templates.test.tspackages/app/tests/docker-git/actions-projects.test.tspackages/api/tests/terminal-sessions.test.ts
**/*.{sh,bash,py,js,ts,jsx,tsx,go,java,rb,php}
📄 CodeRabbit inference engine (Custom checks)
Fail if changed files introduce command injection or unsafe shell/process execution with user-controlled input
Files:
packages/lib/tests/core/templates.test.tspackages/lib/src/core/templates/dockerfile.tspackages/app/src/lib/core/templates/dockerfile.tspackages/app/tests/docker-git/actions-projects.test.tspackages/api/tests/terminal-sessions.test.tspackages/api/src/services/terminal-sessions.ts
**/*.{py,js,ts,jsx,tsx,go,java,rb,php,sh,bash,c,cpp}
📄 CodeRabbit inference engine (Custom checks)
Fail if changed files introduce path traversal or writes outside intended project/container state directories
Files:
packages/lib/tests/core/templates.test.tspackages/lib/src/core/templates/dockerfile.tspackages/app/src/lib/core/templates/dockerfile.tspackages/app/tests/docker-git/actions-projects.test.tspackages/api/tests/terminal-sessions.test.tspackages/api/src/services/terminal-sessions.ts
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,sh,bash,yml,yaml,json,env*,toml,cfg,config,dockerfile,dockerignore}
📄 CodeRabbit inference engine (Custom checks)
Fail if changed files expose credentials, tokens, private-keys, or PII in source, generated config, logs, or CI output
Files:
packages/lib/tests/core/templates.test.tspackages/lib/src/core/templates/dockerfile.tspackages/app/src/lib/core/templates/dockerfile.tspackages/app/tests/docker-git/actions-projects.test.tspackages/api/tests/terminal-sessions.test.tspackages/api/src/services/terminal-sessions.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Ты строгий ревьюер SPEC DRIVEN DEVELOPMENT.Перед выводами изучи README.md, другие *.md файлы, linked issues,
PR description, PR comments/discussion и релевантную кодовую базу.Сверь изменения с исходным ТЗ/спекой и обсуждением. Флагай любой уход
от спеки, недокументированное изменение поведения, отсутствие тестов
для заявленного поведения и security-риск. Если спека не видна,
попроси автора добавить ее в issue или PR description.Проверь решение с точки зрения формальной верификации: какие инварианты,
предусловия и постусловия можно доказать математически, а где доказуемость
слабая. Оцени решение с точки зрения теории игр: устойчивы ли стимулы,
нет ли выгодного обхода правил, и какое решение было бы сильнее.
Files:
packages/lib/tests/core/templates.test.tspackages/lib/src/core/templates/dockerfile.tspackages/app/src/lib/core/templates/dockerfile.tspackages/app/tests/docker-git/actions-projects.test.tspackages/api/tests/terminal-sessions.test.tspackages/api/src/services/terminal-sessions.ts
🔇 Additional comments (19)
packages/lib/src/core/templates/dockerfile.ts (1)
33-33: LGTM!packages/lib/tests/core/templates.test.ts (1)
68-68: LGTM!packages/app/src/lib/core/templates/dockerfile.ts (1)
33-33: LGTM!packages/api/src/services/terminal-sessions.ts (14)
134-153: LGTM!
207-210: LGTM!
212-250: LGTM!
252-310: LGTM!
837-839: LGTM!Also applies to: 854-856
873-894: LGTM!
913-978: LGTM!
1028-1036: LGTM!Also applies to: 1049-1057
1107-1115: LGTM!
1127-1133: LGTM!
1142-1152: LGTM!
1190-1220: LGTM!
1223-1242: LGTM!
1328-1361: LGTM!packages/app/tests/docker-git/actions-projects.test.ts (1)
152-153: LGTM!Also applies to: 168-168, 184-184, 201-201, 208-211
packages/api/tests/terminal-sessions.test.ts (1)
2-4: LGTM!Also applies to: 21-21, 28-28, 32-32, 46-46, 53-108, 127-144, 147-183, 202-246, 285-305
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/api/src/services/terminal-sessions.ts (2)
374-407: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winРассмотрите добавление observability для ошибок персистентности.
runTerminalSessionPersistenceглушит все ошибки черезcatchAll(() => Effect.void)(строка 385). Это приемлемо для best-effort фоновой операции, но усложняет диагностику проблем с записью state на диск.Рекомендация: добавьте структурированное логирование ошибок перед
Effect.void, чтобы операторы видели проблемы с FileSystem (например, заполненный диск, проблемы с правами).♻️ Предлагаемое улучшение observability
const runTerminalSessionPersistence = ( projectId: string, effect: Effect.Effect<void, ApiInternalError, FileSystem.FileSystem> ): void => { const previous = terminalSessionPersistenceQueues.get(projectId) ?? Promise.resolve() const next = previous .catch(() => undefined) .then(() => Effect.runPromise( effect.pipe( Effect.provide(NodeContext.layer), + Effect.tapError((error) => + Effect.sync(() => { + console.error(`[terminal-sessions] Failed to persist state for project ${projectId}:`, error) + }) + ), Effect.catchAll(() => Effect.void) ) ) )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/src/services/terminal-sessions.ts` around lines 374 - 407, runTerminalSessionPersistence currently swallows all persistence errors via Effect.catchAll(() => Effect.void), which hides FileSystem failures; change it to log structured error details before returning Effect.void so operators can diagnose issues: inside the Effect.catchAll handler for Effect.runPromise(effect.pipe(...)), capture the error and call the module's logger (or inject/utilize an existing logger) with context (projectId, session id from the provided effect or record) and the error object, then return Effect.void; ensure updateSession still calls runTerminalSessionPersistence(record.projectId, patchDurableSession(record, patch)) so the same logging path runs for session persists.
619-667: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winУточните семантику status="ready" после выхода PTY с кодом 0.
В строке 654
finalizeRecordустанавливаетnextStatus = "ready"дляexitCode === 0 || exitCode === 130. Это отличается от традиционной семантики, где завершение с кодом 0 означаетstatus="exited".В контексте tmux-backed сессий это, вероятно, правильно: выход из PTY не означает завершение сессии в tmux (пользователь может reattach). Однако это меняет поведение API — после detach сессия остаётся в статусе "ready", а не переходит в финальное состояние.
Рекомендация: убедитесь, что клиенты API понимают новую семантику, и добавьте inline комментарий, объясняющий, почему exitCode=0 не переводит сессию в "exited" (например, "tmux session survives PTY exit and remains reattachable").
📝 Предлагаемое улучшение документации
const finalizeRecord = ( record: TerminalRecord, status: Extract<TerminalSessionStatus, "exited" | "failed">, exitCode: number | null, signal: number | null ): void => { + // CHANGE: tmux-backed sessions remain reattachable after PTY exit (status="ready") + // WHY: PTY exit does not terminate the underlying tmux session + // exitCode 0 = clean shell exit, 130 = SIGINT (Ctrl+C), both indicate intentional detach const nextStatus = exitCode === 0 || exitCode === 130 ? "ready" : status🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/src/services/terminal-sessions.ts` around lines 619 - 667, The finalizeRecord logic sets nextStatus = "ready" when exitCode === 0 || exitCode === 130 which deviates from typical "exited" semantics; update the code by adding a concise inline comment next to the nextStatus calculation inside finalizeRecord explaining the intended semantics (e.g., "tmux-backed session: PTY exit with code 0 or 130 does not terminate the tmux session; session remains reattachable so we mark it 'ready' instead of 'exited'") and, if applicable, add a short note near detachRecordPty or session API docs to ensure API clients understand that "ready" can represent a reattachable session after a clean PTY exit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/api/src/services/terminal-sessions.ts`:
- Around line 374-407: runTerminalSessionPersistence currently swallows all
persistence errors via Effect.catchAll(() => Effect.void), which hides
FileSystem failures; change it to log structured error details before returning
Effect.void so operators can diagnose issues: inside the Effect.catchAll handler
for Effect.runPromise(effect.pipe(...)), capture the error and call the module's
logger (or inject/utilize an existing logger) with context (projectId, session
id from the provided effect or record) and the error object, then return
Effect.void; ensure updateSession still calls
runTerminalSessionPersistence(record.projectId, patchDurableSession(record,
patch)) so the same logging path runs for session persists.
- Around line 619-667: The finalizeRecord logic sets nextStatus = "ready" when
exitCode === 0 || exitCode === 130 which deviates from typical "exited"
semantics; update the code by adding a concise inline comment next to the
nextStatus calculation inside finalizeRecord explaining the intended semantics
(e.g., "tmux-backed session: PTY exit with code 0 or 130 does not terminate the
tmux session; session remains reattachable so we mark it 'ready' instead of
'exited'") and, if applicable, add a short note near detachRecordPty or session
API docs to ensure API clients understand that "ready" can represent a
reattachable session after a clean PTY exit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7e8acdfe-26fc-46f1-ae41-a7326de5bde7
📒 Files selected for processing (4)
packages/api/src/http.tspackages/api/src/services/container-tasks.tspackages/api/src/services/terminal-sessions.tspackages/api/tests/terminal-sessions.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Implement Functional Core, Imperative Shell (FCIS) pattern: CORE layer contains only pure functions with immutable data and mathematical operations; SHELL layer isolates all effects (IO, network, database). Strict dependency direction: SHELL → CORE (never reverse).
Never useany,unknown,eslint-disable,ts-ignore, orastype assertions (except in rigorously justified cases with documentation). Always use exhaustive union type analysis through.exhaustive()pattern matching.
All external dependencies must be wrapped through typed interfaces and injected via Effect-TS Layer pattern. Never call external services directly from CORE functions.
Use monadic composition with Effect-TS for all effects:Effect<Success, Error, Requirements>. Compose effects throughpipe()andEffect.flatMap(). Implement dependency injection via Layer pattern. Handle errors without try/catch blocks.
All functions must be pure in the CORE layer: no side effects (logging, console output, IO operations, mutations). Separate all side effects into the SHELL layer.
Use exhaustive pattern matching with Effect.Match instead of switch statements. Example:Match.value(item).pipe(Match.when(...), Match.exhaustive).
Document all functions with comprehensive TSDoc including:@pure(true/false),@effect(required services),@invariant(mathematical invariants),@precondition,@postcondition,@complexity(time and space),@throwsNever (errors must be typed in Effect).
Use functional comment markers for code clarity: CHANGE (brief description), WHY (mathematical/architectural justification), QUOTE(ТЗ) (requirement citation), REF (RTM or message ID), SOURCE (external source with quote), FORMAT THEOREM (∀x ∈ Domain: P(x) → Q(f(x))), PURITY (CORE|SHELL), EFFECT (Effect type signature), INVARIANT (mathematical invariant), COMPLEXITY (time/space).
Define all external service dependencies as Context.Tag classes with fully typed methods returning Effect types. Example: `class Da...
Files:
packages/api/src/services/container-tasks.tspackages/api/src/http.tspackages/api/tests/terminal-sessions.test.tspackages/api/src/services/terminal-sessions.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Forbidden constructs in CORE code:any,eslint-disable,ts-ignore,async/await, raw Promise chains (then/catch),Promise.all,try/catchfor logic control,console.*, switch statements (use Match with .exhaustive() instead)
All functions must use Effect-TS for composing effects:Effect<Success, Error, Requirements>. No direct async/await, Promise chains, or try/catch in product logic.
Functional comments must include: CHANGE, WHY, QUOTE(ТЗ) or n/a, REF, SOURCE or n/a, FORMAT THEOREM, PURITY (CORE|SHELL), EFFECT signature for SHELL functions, INVARIANT, and COMPLEXITY.
All data mutations must use immutable patterns (ReadonlyArray, readonly properties, Object.freeze); mutation in SHELL only when absolutely necessary and documented.
Files:
packages/api/src/services/container-tasks.tspackages/api/src/http.tspackages/api/tests/terminal-sessions.test.tspackages/api/src/services/terminal-sessions.ts
**/*.{sh,bash,py,js,ts,jsx,tsx,go,java,rb,php}
📄 CodeRabbit inference engine (Custom checks)
Fail if changed files introduce command injection or unsafe shell/process execution with user-controlled input
Files:
packages/api/src/services/container-tasks.tspackages/api/src/http.tspackages/api/tests/terminal-sessions.test.tspackages/api/src/services/terminal-sessions.ts
**/*.{py,js,ts,jsx,tsx,go,java,rb,php,sh,bash,c,cpp}
📄 CodeRabbit inference engine (Custom checks)
Fail if changed files introduce path traversal or writes outside intended project/container state directories
Files:
packages/api/src/services/container-tasks.tspackages/api/src/http.tspackages/api/tests/terminal-sessions.test.tspackages/api/src/services/terminal-sessions.ts
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,sh,bash,yml,yaml,json,env*,toml,cfg,config,dockerfile,dockerignore}
📄 CodeRabbit inference engine (Custom checks)
Fail if changed files expose credentials, tokens, private-keys, or PII in source, generated config, logs, or CI output
Files:
packages/api/src/services/container-tasks.tspackages/api/src/http.tspackages/api/tests/terminal-sessions.test.tspackages/api/src/services/terminal-sessions.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Ты строгий ревьюер SPEC DRIVEN DEVELOPMENT.Перед выводами изучи README.md, другие *.md файлы, linked issues,
PR description, PR comments/discussion и релевантную кодовую базу.Сверь изменения с исходным ТЗ/спекой и обсуждением. Флагай любой уход
от спеки, недокументированное изменение поведения, отсутствие тестов
для заявленного поведения и security-риск. Если спека не видна,
попроси автора добавить ее в issue или PR description.Проверь решение с точки зрения формальной верификации: какие инварианты,
предусловия и постусловия можно доказать математически, а где доказуемость
слабая. Оцени решение с точки зрения теории игр: устойчивы ли стимулы,
нет ли выгодного обхода правил, и какое решение было бы сильнее.
Files:
packages/api/src/services/container-tasks.tspackages/api/src/http.tspackages/api/tests/terminal-sessions.test.tspackages/api/src/services/terminal-sessions.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Implement property-based testing using fast-check for mathematical properties and invariants. Example:fc.property(fc.array(messageArbitrary), (messages) => isChronologicallySorted(sortMessagesByTimestamp(messages))).
Mock external dependencies in unit tests using Effect's testing utilities. Run tests without Effect runtime for speed. Example:Effect.provide(MockService), Effect.runPromise.
Files:
packages/api/tests/terminal-sessions.test.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Property-based tests (fast-check) must verify mathematical invariants; unit tests must use Effect test utilities without async/await.
Files:
packages/api/tests/terminal-sessions.test.ts
🔇 Additional comments (29)
packages/api/src/services/terminal-sessions.ts (18)
1-52: LGTM!
81-90: LGTM!
91-176: LGTM!
180-196: LGTM!
198-228: LGTM!
230-260: LGTM!
261-304: LGTM!
306-370: LGTM!
423-448: LGTM!
898-923: LGTM!
925-963: LGTM!
965-1036: LGTM!
1038-1112: LGTM!
1145-1198: LGTM!
1238-1280: LGTM!
1282-1371: LGTM!
1373-1397: LGTM!
1482-1523: LGTM!packages/api/src/services/container-tasks.ts (1)
369-376: LGTM!packages/api/src/http.ts (2)
1441-1450: LGTM!
1491-1498: LGTM!packages/api/tests/terminal-sessions.test.ts (8)
1-52: LGTM!
56-122: LGTM!
133-153: LGTM!
155-218: LGTM!
220-260: LGTM!
262-301: LGTM!
303-345: LGTM!
347-360: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/api/src/services/terminal-sessions.ts`:
- Around line 81-90: The types TerminalSessionRuntime and
TerminalSessionStateRuntime are currently declared as unions (|) but should be
intersections (&) because Effect dependencies require all services; change both
declarations from using | to using & so the types require
CommandExecutor.CommandExecutor & FileSystem.FileSystem & PlatformPath.Path,
ensuring callers must provide all services when using these runtimes.
- Around line 1369-1376: The current Effect.catchAll in
lookupTerminalSessionById converts every error (including ApiInternalError) into
ApiNotFoundError, hiding real internal failures; change the error handling so
that only genuine "not found" cases are mapped to ApiNotFoundError while other
errors (e.g., ApiInternalError or unknown errors) are rethrown/preserved.
Specifically, replace the broad Effect.catchAll with a selective catcher (e.g.,
Effect.catchSome or an if-check) that returns Effect.fail(error) when error is
an instance of ApiInternalError (or any non-not-found error), and only returns
Effect.fail(new ApiNotFoundError(...)) when the original error indicates a true
not-found condition; reference lookupTerminalSessionById, Effect.catchAll,
ApiNotFoundError, ApiInternalError, and sessionId when applying the fix.
- Around line 1175-1177: The requestId passed from the client
(options.requestId) is being used as sessionId in registerRecord and stored via
upsertDurableSession, which risks session collisions and predictable IDs; either
enforce UUID format validation on StartProjectTerminalSessionRequestSchema for
requestId (reject non-UUIDs) or stop trusting the client value: generate a
server-side sessionId (and use tmuxNameForSessionId to sanitize when creating
tmux names), store that generated ID in registerRecord/upsertDurableSession, and
return it to the client; update any call sites that currently pass
options.requestId (e.g., the registerRecord invocation and session creation
flow) to use the validated or generated server-side ID.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 34d31149-4c2b-4a50-a682-b10d2b198808
📒 Files selected for processing (1)
packages/api/src/services/terminal-sessions.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Implement Functional Core, Imperative Shell (FCIS) pattern: CORE layer contains only pure functions with immutable data and mathematical operations; SHELL layer isolates all effects (IO, network, database). Strict dependency direction: SHELL → CORE (never reverse).
Never useany,unknown,eslint-disable,ts-ignore, orastype assertions (except in rigorously justified cases with documentation). Always use exhaustive union type analysis through.exhaustive()pattern matching.
All external dependencies must be wrapped through typed interfaces and injected via Effect-TS Layer pattern. Never call external services directly from CORE functions.
Use monadic composition with Effect-TS for all effects:Effect<Success, Error, Requirements>. Compose effects throughpipe()andEffect.flatMap(). Implement dependency injection via Layer pattern. Handle errors without try/catch blocks.
All functions must be pure in the CORE layer: no side effects (logging, console output, IO operations, mutations). Separate all side effects into the SHELL layer.
Use exhaustive pattern matching with Effect.Match instead of switch statements. Example:Match.value(item).pipe(Match.when(...), Match.exhaustive).
Document all functions with comprehensive TSDoc including:@pure(true/false),@effect(required services),@invariant(mathematical invariants),@precondition,@postcondition,@complexity(time and space),@throwsNever (errors must be typed in Effect).
Use functional comment markers for code clarity: CHANGE (brief description), WHY (mathematical/architectural justification), QUOTE(ТЗ) (requirement citation), REF (RTM or message ID), SOURCE (external source with quote), FORMAT THEOREM (∀x ∈ Domain: P(x) → Q(f(x))), PURITY (CORE|SHELL), EFFECT (Effect type signature), INVARIANT (mathematical invariant), COMPLEXITY (time/space).
Define all external service dependencies as Context.Tag classes with fully typed methods returning Effect types. Example: `class Da...
Files:
packages/api/src/services/terminal-sessions.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Forbidden constructs in CORE code:any,eslint-disable,ts-ignore,async/await, raw Promise chains (then/catch),Promise.all,try/catchfor logic control,console.*, switch statements (use Match with .exhaustive() instead)
All functions must use Effect-TS for composing effects:Effect<Success, Error, Requirements>. No direct async/await, Promise chains, or try/catch in product logic.
Functional comments must include: CHANGE, WHY, QUOTE(ТЗ) or n/a, REF, SOURCE or n/a, FORMAT THEOREM, PURITY (CORE|SHELL), EFFECT signature for SHELL functions, INVARIANT, and COMPLEXITY.
All data mutations must use immutable patterns (ReadonlyArray, readonly properties, Object.freeze); mutation in SHELL only when absolutely necessary and documented.
Files:
packages/api/src/services/terminal-sessions.ts
**/*.{sh,bash,py,js,ts,jsx,tsx,go,java,rb,php}
📄 CodeRabbit inference engine (Custom checks)
Fail if changed files introduce command injection or unsafe shell/process execution with user-controlled input
Files:
packages/api/src/services/terminal-sessions.ts
**/*.{py,js,ts,jsx,tsx,go,java,rb,php,sh,bash,c,cpp}
📄 CodeRabbit inference engine (Custom checks)
Fail if changed files introduce path traversal or writes outside intended project/container state directories
Files:
packages/api/src/services/terminal-sessions.ts
**/*.{js,ts,jsx,tsx,py,java,go,rb,php,sh,bash,yml,yaml,json,env*,toml,cfg,config,dockerfile,dockerignore}
📄 CodeRabbit inference engine (Custom checks)
Fail if changed files expose credentials, tokens, private-keys, or PII in source, generated config, logs, or CI output
Files:
packages/api/src/services/terminal-sessions.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Ты строгий ревьюер SPEC DRIVEN DEVELOPMENT.Перед выводами изучи README.md, другие *.md файлы, linked issues,
PR description, PR comments/discussion и релевантную кодовую базу.Сверь изменения с исходным ТЗ/спекой и обсуждением. Флагай любой уход
от спеки, недокументированное изменение поведения, отсутствие тестов
для заявленного поведения и security-риск. Если спека не видна,
попроси автора добавить ее в issue или PR description.Проверь решение с точки зрения формальной верификации: какие инварианты,
предусловия и постусловия можно доказать математически, а где доказуемость
слабая. Оцени решение с точки зрения теории игр: устойчивы ли стимулы,
нет ли выгодного обхода правил, и какое решение было бы сильнее.
Files:
packages/api/src/services/terminal-sessions.ts
🔇 Additional comments (9)
packages/api/src/services/terminal-sessions.ts (9)
1-51: LGTM!
91-176: LGTM!
261-370: LGTM!
374-412: LGTM!
623-672: LGTM!
903-968: LGTM!
970-1117: LGTM!
1378-1402: LGTM!Also applies to: 1494-1522
190-196: 🏗️ Heavy liftВалидация
projectIdпротив реальных проектов системы происходит вgetProject()→findProjectById()до передачи в дurable session функции. Произвольные пути вроде"../../etc"не пройдут whitelist-проверку вfindProjectById()и не достигнутterminalSessionStatePath(). ФункцияisPathInsideDirectoryслужит дополнительной защитой, а не основной линией обороны.> Likely an incorrect or invalid review comment.
| type TerminalSessionRuntime = | ||
| | CommandExecutor.CommandExecutor | ||
| | FileSystem.FileSystem | ||
| | PlatformPath.Path | ||
|
|
||
| type TerminalSessionStateRuntime = | ||
| | CommandExecutor.CommandExecutor | ||
| | FileSystem.FileSystem | ||
| | PlatformPath.Path | ||
|
|
There was a problem hiding this comment.
Неверный тип: объединение (|) вместо пересечения (&) для требований Effect.
Типы TerminalSessionRuntime и TerminalSessionStateRuntime используют union (|), что означает "один из сервисов". Для зависимостей Effect должно быть пересечение (&), означающее "все эти сервисы обязательны".
С текущей сигнатурой TypeScript позволит вызвать функцию, предоставив только один из сервисов, что приведёт к runtime-ошибке при попытке использовать отсутствующий сервис.
🐛 Исправление типов
type TerminalSessionRuntime =
- | CommandExecutor.CommandExecutor
- | FileSystem.FileSystem
- | PlatformPath.Path
+ & CommandExecutor.CommandExecutor
+ & FileSystem.FileSystem
+ & PlatformPath.Path
type TerminalSessionStateRuntime =
- | CommandExecutor.CommandExecutor
- | FileSystem.FileSystem
- | PlatformPath.Path
+ & CommandExecutor.CommandExecutor
+ & FileSystem.FileSystem
+ & PlatformPath.Path🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/api/src/services/terminal-sessions.ts` around lines 81 - 90, The
types TerminalSessionRuntime and TerminalSessionStateRuntime are currently
declared as unions (|) but should be intersections (&) because Effect
dependencies require all services; change both declarations from using | to
using & so the types require CommandExecutor.CommandExecutor &
FileSystem.FileSystem & PlatformPath.Path, ensuring callers must provide all
services when using these runtimes.
1 participant
Summary
.orch/state/terminal-sessions.jsonWhy
Project SSH terminals were held in API memory and browser-local workspace state, so session URLs and lists disappeared after API/browser restart. This change makes the backend the durable source of truth for project terminal sessions while preserving the existing route and response shapes.
Closes #286
Validation
bun run --filter @effect-template/api testbun run --filter @prover-coder-ai/docker-git testbun run --filter @effect-template/lib testbun run typecheckgit diff --checkNot run: browser-context Docker e2e restart test; the repo does not currently have a browser e2e harness for that exact scenario.