Upstream sync: provider registry, configurable base dir, UI fixes#30
Upstream sync: provider registry, configurable base dir, UI fixes#30aaditagrawal wants to merge 7 commits intomainfrom
Conversation
Co-authored-by: Mohammed Muzammil Anwar <muzammil@pluginhive.com> Co-authored-by: Julius Marminge <julius0216@outlook.com>
…ngdotgg#1121) Co-authored-by: Julius Marminge <julius0216@outlook.com> Co-authored-by: codex <codex@users.noreply.github.com>
github-actions
bot
added
size:XXL
📝 WalkthroughWalkthroughThe PR refactors the application's filesystem directory model from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/src/components/chat/ProviderModelPicker.tsx (1)
271-282:⚠️ Potential issue | 🟡 MinorRemove unused
handleModelChangefunction; inconsistent resolver usage.The
handleModelChangefunction (lines 271–282) is defined but never invoked in this component. All actual model selection logic usesonModelSelect(line 337), which calls the localresolveModelForProviderPickerinstead of the importedresolveSelectableModel. This creates an unused function and leaves a partially completed migration.Either remove
handleModelChangeor migrateonModelSelectto useresolveSelectableModel(ensuring cursor-specific family handling is preserved).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/chat/ProviderModelPicker.tsx` around lines 271 - 282, There is an unused function handleModelChange and inconsistent resolver usage: either delete handleModelChange or consolidate selection logic by updating onModelSelect to call the imported resolveSelectableModel(provider, value, props.modelOptionsByProvider[provider]) instead of resolveModelForProviderPicker, then remove handleModelChange; if you migrate, ensure you preserve existing cursor-specific family handling currently done in resolveModelForProviderPicker by applying the same post-resolution mapping (e.g., translate cursor family/model identifiers into the expected selectable model shape) before calling props.onProviderModelChange(provider, resolvedModel) and closing the menu.apps/web/src/composerDraftStore.ts (1)
104-110:⚠️ Potential issue | 🟠 MajorDon't tighten the persisted draft schema without a compatible decode path.
These fields are now required on disk, but the store version is still
2.readPersistedAttachmentIdsFromStorage()later decodes the whole payload viaPersistedComposerDraftStoreStorage, so a pre-upgrade snapshot that lacksstickyModel/stickyModelOptionsis treated as empty until some later write rewrites the key. That can make persisted attachments disappear on the first launch after upgrading. Keep the decode path tolerant of missing sticky fields, or bump the persisted version and migrate before that helper reads it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/composerDraftStore.ts` around lines 104 - 110, PersistedComposerDraftStoreState was tightened to require stickyModel and stickyModelOptions which breaks decoding older on-disk data read by readPersistedAttachmentIdsFromStorage via PersistedComposerDraftStoreStorage; either relax the schema back to accept missing fields (e.g., make stickyModelOptions optional/NullOr) or add a migration bumping the persisted store version and migrate existing payloads before readPersistedAttachmentIdsFromStorage runs so pre-upgrade snapshots aren’t treated as empty. Update the schema or add a versioned decode/migration path in the code that constructs/uses PersistedComposerDraftStoreStorage and ensure readPersistedAttachmentIdsFromStorage uses that tolerant path.apps/server/src/main.ts (1)
91-104:⚠️ Potential issue | 🟠 MajorHandle legacy
T3CODE_STATE_DIR/--state-direxplicitly.The env half now only reads
T3CODE_HOME, and the CLI half only accepts--home-dir. Existing launch scripts that still setT3CODE_STATE_DIRwill silently fall back to the default home directory, while--state-dirnow hard-fails parsing. That can look like data loss because the server starts against a different state root. Please keep a deprecation shim or fail fast with a clear migration message when the legacy inputs are present.Also applies to: 298-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/main.ts` around lines 91 - 104, The code currently only reads T3CODE_HOME (Config.string("T3CODE_HOME") in CliEnvConfig) and the CLI only accepts --home-dir, so legacy T3CODE_STATE_DIR / --state-dir are ignored or hard-fail; update the config and CLI parsing to detect the legacy inputs and provide a clear migration path: in CliEnvConfig add detection for Config.string("T3CODE_STATE_DIR") and if present map it to the t3Home value while emitting a deprecation warning (or, if both T3CODE_HOME and T3CODE_STATE_DIR are set and conflict, fail fast with a clear error message telling the user to migrate to T3CODE_HOME), and in the CLI arg parsing (the block referred at ~298-300) accept --state-dir as an alias for --home-dir or produce a descriptive error instructing users to switch to --home-dir; ensure all messages include the exact legacy key/flag names and recommended replacements so behavior is explicit and no silent fallback occurs.
🧹 Nitpick comments (4)
.gitignore (1)
20-21: LGTM! Consider cleaning up the redundant pattern.The new ignore patterns are correct:
.vitest-*properly ignores Vitest cache/temporary files__screenshots__/correctly ignores screenshot directories (trailing slash ensures directory-only matching)However, line 19 (
apps/web/src/components/__screenshots__) is now redundant since line 21 matches all__screenshots__/directories throughout the repository.♻️ Optional cleanup to remove redundancy
apps/web/.playwright apps/web/playwright-report -apps/web/src/components/__screenshots__ .vitest-* __screenshots__/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 20 - 21, Remove the redundant explicit ignore entry "apps/web/src/components/__screenshots__" since the broader "__screenshots__/" pattern already ignores all such directories; update .gitignore by deleting the specific apps/web/src/components/__screenshots__ line and keep the general "__screenshots__/" and ".vitest-*" entries to avoid duplication.apps/web/src/components/Sidebar.tsx (1)
1853-1861: Consider memoizingprojectStatusto avoid redundant re-computation.The current implementation calls
derivePendingApprovalsandderivePendingUserInputsfor every thread in every project on each render. For projects with many threads or threads with large activity arrays, this could become a performance bottleneck.Consider extracting this computation into a
useMemohook or a separate memoized selector to avoid redundant work when unrelated state changes trigger re-renders.Example memoization approach
You could create a memoized map of project statuses outside the render loop:
const projectStatusById = useMemo(() => { const map = new Map<ProjectId, ReturnType<typeof resolveProjectStatusIndicator>>(); for (const project of projects) { const projectThreads = threads.filter((t) => t.projectId === project.id); const status = resolveProjectStatusIndicator( projectThreads.map((thread) => resolveThreadStatusPill({ thread, hasPendingApprovals: derivePendingApprovals(thread.activities).length > 0, hasPendingUserInput: derivePendingUserInputs(thread.activities).length > 0, }), ), ); map.set(project.id, status); } return map; }, [projects, threads]);Then use
projectStatusById.get(project.id)inside the render.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/Sidebar.tsx` around lines 1853 - 1861, The projectStatus computation repeatedly calls derivePendingApprovals and derivePendingUserInputs for every thread on each render; wrap the logic that builds projectStatus (the resolveProjectStatusIndicator call that maps over projectThreads and calls resolveThreadStatusPill) in a useMemo (or move to a memoized selector) keyed on the relevant inputs (projects and threads or threads' activities) so you compute a Map<ProjectId, status> once and then lookup with project.id in render; ensure the memo dependencies include projects and threads (or a derived stable key for thread.activities) and keep references to resolveProjectStatusIndicator, resolveThreadStatusPill, derivePendingApprovals, and derivePendingUserInputs unchanged.apps/web/src/hooks/useHandleNewThread.ts (1)
53-59: Redundant aliasing of the same store methods.
setModelandsetDraftModelare aliases for the same function, as aresetProviderandsetDraftProvider. This creates confusion about which should be used where.Consider using a single name for each method throughout the function.
♻️ Suggested simplification
const { clearProjectDraftThreadId, draftsByThreadId, getDraftThread, getDraftThreadByProjectId, setModel, setModelOptions, setProvider, setDraftThreadContext, - setModel: setDraftModel, setProjectDraftThreadId, - setProvider: setDraftProvider, } = useComposerDraftStore.getState();Then use
setModelandsetProviderconsistently below (lines 136-140).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useHandleNewThread.ts` around lines 53 - 59, The destructure in useHandleNewThread pulls duplicate aliases (setModel and setModel: setDraftModel, setProvider and setProvider: setDraftProvider); remove the redundant aliases so each store method appears once (keep setModel and setProvider) and update all usages in this module (e.g., the places currently calling setDraftModel and setDraftProvider) to call setModel and setProvider instead so there’s a single canonical name for each action (check usages around where the draft is initialized and the calls currently at lines ~136-140).apps/web/src/components/ChatView.browser.tsx (1)
1285-1293: Seed sticky state through the public store actions in these tests.Using raw
setState()here bypassessetStickyModel()andsetStickyModelOptions(), so these cases won't catch regressions in the new normalization path that production uses. Prefer setting up the sticky state through the store actions instead.Also applies to: 1331-1339, 1407-1415
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/ChatView.browser.tsx` around lines 1285 - 1293, Replace direct useComposerDraftStore.setState({...}) with the public store actions so the normalization path is exercised: call useComposerDraftStore.getState().setStickyModel("gpt-5.3-codex") and useComposerDraftStore.getState().setStickyModelOptions({ codex: { reasoningEffort: "medium", fastMode: true } }) (or the equivalent action signatures) in the affected test setup blocks (also update the similar occurrences around the other ranges noted) so tests seed stickyModel and stickyModelOptions via setStickyModel and setStickyModelOptions instead of bypassing them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.docs/scripts.md:
- Around line 6-8: Update the docs to use the new environment variable name:
replace the obsolete T3CODE_STATE_DIR with T3CODE_HOME in the sentence that
describes defaulting dev commands; specifically change the phrase "Dev commands
default `T3CODE_STATE_DIR` to `~/.t3/dev`" to use `T3CODE_HOME` so it matches
the renamed variable used throughout the codebase and remains consistent with
the `--base-dir` example.
In `@apps/desktop/src/main.ts`:
- Around line 64-65: BASE_DIR is currently taken directly from
process.env.T3CODE_HOME which can be relative and cause path drift; normalize it
to an absolute path at startup (e.g., compute BASE_DIR from
process.env.T3CODE_HOME if present, otherwise default to Path.join(OS.homedir(),
".t3"), then pass that value through Path.resolve or Path.normalize to produce
an absolute BASE_DIR) and then derive STATE_DIR from that resolved BASE_DIR so
both desktop and backend use the same absolute path; update the BASE_DIR and
STATE_DIR assignments (symbols: BASE_DIR, STATE_DIR, T3CODE_HOME) accordingly.
In `@apps/server/integration/OrchestrationEngineHarness.integration.ts`:
- Around line 237-246: rootDir is created with makeTempDirectoryScoped but its
scope is not closed in the harness, so the temp directory outlives the harness;
allocate a harness-owned Scope (e.g., harnessScope) and call
makeTempDirectoryScoped within that scope to produce rootDir (or capture the
returned disposal handle) and store the scope/handle on the harness; then update
dispose to close that same harnessScope (or call the temp-directory disposal) so
the temporary directory is reliably cleaned up when dispose runs; refer to
makeTempDirectoryScoped, rootDir, deriveServerPaths, and dispose to locate where
to attach and close the scope.
In `@apps/server/src/git/Layers/GitManager.test.ts`:
- Around line 518-526: The temporary baseDir created by ServerConfig.layerTest
(via makeTempDirectoryScoped) is being finalized before manager methods run
because ServerConfigLayer is only provided when constructing manager via
Effect.provide(managerLayer); fix by making the ServerConfigLayer live for the
full manager lifetime—either merge ServerConfigLayer into the managerLayer
composition (e.g., include ServerConfigLayer via Layer.provideMerge when
building gitCoreLayer/managerLayer so it remains part of the returned manager's
scope) or build the manager inside a scoped effect (use
Effect.scoped/Layer.build and run manager methods inside that scope) so the
temporary baseDir created by ServerConfig.layerTest remains alive while manager
methods (like manager.* in tests) execute.
In `@apps/web/src/appSettings.test.ts`:
- Around line 269-315: The tests hard-code expectations for only two providers
but the shared helpers now expose all 8 providers; update the assertions to
reflect the full provider list and their defaults. Replace the expected array in
the MODEL_PROVIDER_SETTINGS test to include all 8 provider.provider values,
update getCustomModelsForProvider expectations to assert the custom model lists
for each of the 8 providers (using their actual names as returned by
MODEL_PROVIDER_SETTINGS), update the defaults object and
getDefaultCustomModelsForProvider assertions to include the 8 corresponding
default keys (e.g., customCodexModels, customClaudeModels, customCopilotModels,
customCursorModels, customOpencodeModels, customGeminiCliModels,
customAmpModels, customKiloModels), adjust the patchCustomModels assertions for
any additional providers if needed, and change the getCustomModelsByProvider
expectation to include entries for all 8 providers mapping to their expected
model arrays.
In `@scripts/dev-runner.ts`:
- Around line 104-114: The resolveBaseDir function currently passes a configured
baseDir directly to Path.resolve, which treats a leading "~" as a literal
segment; update resolveBaseDir to expand a leading tilde before resolving:
either import and call the existing expandHomePath from
apps/server/src/os-jank.ts (preferred) or implement expansion by replacing a
leading "~" with OS.homedir() joined via Path.join, then call Path.resolve on
the expanded value; ensure both places that call resolveBaseDir and the
DEFAULT_T3_HOME fallback behavior remain correct.
---
Outside diff comments:
In `@apps/server/src/main.ts`:
- Around line 91-104: The code currently only reads T3CODE_HOME
(Config.string("T3CODE_HOME") in CliEnvConfig) and the CLI only accepts
--home-dir, so legacy T3CODE_STATE_DIR / --state-dir are ignored or hard-fail;
update the config and CLI parsing to detect the legacy inputs and provide a
clear migration path: in CliEnvConfig add detection for
Config.string("T3CODE_STATE_DIR") and if present map it to the t3Home value
while emitting a deprecation warning (or, if both T3CODE_HOME and
T3CODE_STATE_DIR are set and conflict, fail fast with a clear error message
telling the user to migrate to T3CODE_HOME), and in the CLI arg parsing (the
block referred at ~298-300) accept --state-dir as an alias for --home-dir or
produce a descriptive error instructing users to switch to --home-dir; ensure
all messages include the exact legacy key/flag names and recommended
replacements so behavior is explicit and no silent fallback occurs.
In `@apps/web/src/components/chat/ProviderModelPicker.tsx`:
- Around line 271-282: There is an unused function handleModelChange and
inconsistent resolver usage: either delete handleModelChange or consolidate
selection logic by updating onModelSelect to call the imported
resolveSelectableModel(provider, value, props.modelOptionsByProvider[provider])
instead of resolveModelForProviderPicker, then remove handleModelChange; if you
migrate, ensure you preserve existing cursor-specific family handling currently
done in resolveModelForProviderPicker by applying the same post-resolution
mapping (e.g., translate cursor family/model identifiers into the expected
selectable model shape) before calling props.onProviderModelChange(provider,
resolvedModel) and closing the menu.
In `@apps/web/src/composerDraftStore.ts`:
- Around line 104-110: PersistedComposerDraftStoreState was tightened to require
stickyModel and stickyModelOptions which breaks decoding older on-disk data read
by readPersistedAttachmentIdsFromStorage via PersistedComposerDraftStoreStorage;
either relax the schema back to accept missing fields (e.g., make
stickyModelOptions optional/NullOr) or add a migration bumping the persisted
store version and migrate existing payloads before
readPersistedAttachmentIdsFromStorage runs so pre-upgrade snapshots aren’t
treated as empty. Update the schema or add a versioned decode/migration path in
the code that constructs/uses PersistedComposerDraftStoreStorage and ensure
readPersistedAttachmentIdsFromStorage uses that tolerant path.
---
Nitpick comments:
In @.gitignore:
- Around line 20-21: Remove the redundant explicit ignore entry
"apps/web/src/components/__screenshots__" since the broader "__screenshots__/"
pattern already ignores all such directories; update .gitignore by deleting the
specific apps/web/src/components/__screenshots__ line and keep the general
"__screenshots__/" and ".vitest-*" entries to avoid duplication.
In `@apps/web/src/components/ChatView.browser.tsx`:
- Around line 1285-1293: Replace direct useComposerDraftStore.setState({...})
with the public store actions so the normalization path is exercised: call
useComposerDraftStore.getState().setStickyModel("gpt-5.3-codex") and
useComposerDraftStore.getState().setStickyModelOptions({ codex: {
reasoningEffort: "medium", fastMode: true } }) (or the equivalent action
signatures) in the affected test setup blocks (also update the similar
occurrences around the other ranges noted) so tests seed stickyModel and
stickyModelOptions via setStickyModel and setStickyModelOptions instead of
bypassing them.
In `@apps/web/src/components/Sidebar.tsx`:
- Around line 1853-1861: The projectStatus computation repeatedly calls
derivePendingApprovals and derivePendingUserInputs for every thread on each
render; wrap the logic that builds projectStatus (the
resolveProjectStatusIndicator call that maps over projectThreads and calls
resolveThreadStatusPill) in a useMemo (or move to a memoized selector) keyed on
the relevant inputs (projects and threads or threads' activities) so you compute
a Map<ProjectId, status> once and then lookup with project.id in render; ensure
the memo dependencies include projects and threads (or a derived stable key for
thread.activities) and keep references to resolveProjectStatusIndicator,
resolveThreadStatusPill, derivePendingApprovals, and derivePendingUserInputs
unchanged.
In `@apps/web/src/hooks/useHandleNewThread.ts`:
- Around line 53-59: The destructure in useHandleNewThread pulls duplicate
aliases (setModel and setModel: setDraftModel, setProvider and setProvider:
setDraftProvider); remove the redundant aliases so each store method appears
once (keep setModel and setProvider) and update all usages in this module (e.g.,
the places currently calling setDraftModel and setDraftProvider) to call
setModel and setProvider instead so there’s a single canonical name for each
action (check usages around where the draft is initialized and the calls
currently at lines ~136-140).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65d9b5f5-4b15-47e2-882d-08d9b5328a37
📒 Files selected for processing (61)
.docs/scripts.md.gitignoreREMOTE.mdapps/desktop/src/main.tsapps/server/integration/OrchestrationEngineHarness.integration.tsapps/server/integration/orchestrationEngine.integration.test.tsapps/server/src/attachmentPaths.tsapps/server/src/attachmentStore.test.tsapps/server/src/attachmentStore.tsapps/server/src/config.tsapps/server/src/git/Layers/CodexTextGeneration.test.tsapps/server/src/git/Layers/CodexTextGeneration.tsapps/server/src/git/Layers/GitCore.test.tsapps/server/src/git/Layers/GitCore.tsapps/server/src/git/Layers/GitManager.test.tsapps/server/src/keybindings.test.tsapps/server/src/main.test.tsapps/server/src/main.tsapps/server/src/orchestration/Layers/CheckpointReactor.test.tsapps/server/src/orchestration/Layers/OrchestrationEngine.test.tsapps/server/src/orchestration/Layers/ProjectionPipeline.test.tsapps/server/src/orchestration/Layers/ProjectionPipeline.tsapps/server/src/orchestration/Layers/ProviderCommandReactor.test.tsapps/server/src/os-jank.tsapps/server/src/persistence/Layers/Sqlite.tsapps/server/src/provider/Layers/ClaudeAdapter.test.tsapps/server/src/provider/Layers/ClaudeAdapter.tsapps/server/src/provider/Layers/CodexAdapter.tsapps/server/src/provider/Layers/CopilotAdapter.tsapps/server/src/serverLayers.tsapps/server/src/serverLogger.tsapps/server/src/telemetry/Identify.tsapps/server/src/telemetry/Layers/AnalyticsService.test.tsapps/server/src/terminal/Layers/Manager.tsapps/server/src/wsServer.test.tsapps/server/src/wsServer.tsapps/web/src/appSettings.test.tsapps/web/src/appSettings.tsapps/web/src/components/ChatView.browser.tsxapps/web/src/components/ChatView.tsxapps/web/src/components/Sidebar.logic.test.tsapps/web/src/components/Sidebar.logic.tsapps/web/src/components/Sidebar.tsxapps/web/src/components/chat/ClaudeTraitsPicker.browser.tsxapps/web/src/components/chat/ClaudeTraitsPicker.tsxapps/web/src/components/chat/CodexTraitsPicker.browser.tsxapps/web/src/components/chat/CodexTraitsPicker.tsxapps/web/src/components/chat/ProviderHealthBanner.tsxapps/web/src/components/chat/ProviderModelPicker.browser.tsxapps/web/src/components/chat/ProviderModelPicker.tsxapps/web/src/components/chat/composerProviderRegistry.test.tsxapps/web/src/components/chat/composerProviderRegistry.tsxapps/web/src/composerDraftStore.test.tsapps/web/src/composerDraftStore.tsapps/web/src/hooks/useHandleNewThread.tsapps/web/src/routes/_chat.settings.tsxpackages/shared/src/model.test.tspackages/shared/src/model.tsscripts/dev-runner.test.tsscripts/dev-runner.tsturbo.json
| - Dev commands default `T3CODE_STATE_DIR` to `~/.t3/dev` to keep dev state isolated from desktop/prod state. | ||
| - Override server CLI-equivalent flags from root dev commands with `--`, for example: | ||
| `bun run dev -- --state-dir ~/.t3/another-dev-state` | ||
| `bun run dev -- --base-dir ~/.t3-2` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if T3CODE_STATE_DIR is still used anywhere in the codebase
echo "=== Searching for T3CODE_STATE_DIR usage ==="
rg -n "T3CODE_STATE_DIR" --type-add 'config:*.{json,md,ts,js}' -t config
echo ""
echo "=== Searching for T3CODE_HOME usage ==="
rg -n "T3CODE_HOME" --type-add 'config:*.{json,md,ts,js}' -t configRepository: aaditagrawal/t3code
Length of output: 1631
Update T3CODE_STATE_DIR to T3CODE_HOME on line 6.
The environment variable was renamed to T3CODE_HOME throughout the codebase. Line 6 still references the obsolete T3CODE_STATE_DIR, which no longer exists in the project. Update to match the current variable name and be consistent with line 8's correct usage of --base-dir.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.docs/scripts.md around lines 6 - 8, Update the docs to use the new
environment variable name: replace the obsolete T3CODE_STATE_DIR with
T3CODE_HOME in the sentence that describes defaulting dev commands; specifically
change the phrase "Dev commands default `T3CODE_STATE_DIR` to `~/.t3/dev`" to
use `T3CODE_HOME` so it matches the renamed variable used throughout the
codebase and remains consistent with the `--base-dir` example.
| const BASE_DIR = process.env.T3CODE_HOME?.trim() || Path.join(OS.homedir(), ".t3"); | ||
| const STATE_DIR = Path.join(BASE_DIR, "userdata"); |
There was a problem hiding this comment.
Resolve BASE_DIR to an absolute path to avoid desktop/backend path drift.
If T3CODE_HOME is relative, desktop log paths can diverge from backend paths (different CWDs). Normalize once at startup.
Suggested fix
-const BASE_DIR = process.env.T3CODE_HOME?.trim() || Path.join(OS.homedir(), ".t3");
+const BASE_DIR = Path.resolve(process.env.T3CODE_HOME?.trim() || Path.join(OS.homedir(), ".t3"));
const STATE_DIR = Path.join(BASE_DIR, "userdata");Also applies to: 932-933
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main.ts` around lines 64 - 65, BASE_DIR is currently taken
directly from process.env.T3CODE_HOME which can be relative and cause path
drift; normalize it to an absolute path at startup (e.g., compute BASE_DIR from
process.env.T3CODE_HOME if present, otherwise default to Path.join(OS.homedir(),
".t3"), then pass that value through Path.resolve or Path.normalize to produce
an absolute BASE_DIR) and then derive STATE_DIR from that resolved BASE_DIR so
both desktop and backend use the same absolute path; update the BASE_DIR and
STATE_DIR assignments (symbols: BASE_DIR, STATE_DIR, T3CODE_HOME) accordingly.
| const rootDir = yield* fileSystem.makeTempDirectoryScoped({ | ||
| prefix: "t3-orchestration-integration-", | ||
| }); | ||
| const workspaceDir = path.join(rootDir, "workspace"); | ||
| const stateDir = path.join(rootDir, "state"); | ||
| const dbPath = path.join(stateDir, "state.sqlite"); | ||
| fs.mkdirSync(workspaceDir, { recursive: true }); | ||
| fs.mkdirSync(stateDir, { recursive: true }); | ||
| initializeGitWorkspace(workspaceDir); | ||
| const { stateDir, dbPath } = yield* deriveServerPaths(rootDir, undefined).pipe( | ||
| Effect.provideService(Path.Path, path), | ||
| ); | ||
| yield* fileSystem.makeDirectory(workspaceDir, { recursive: true }); | ||
| yield* fileSystem.makeDirectory(stateDir, { recursive: true }); | ||
| yield* initializeGitWorkspace(workspaceDir); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'makeTempDirectoryScoped|Scope\.make|Scope\.close|dispose = Effect\.gen' apps/server/integration/OrchestrationEngineHarness.integration.tsRepository: aaditagrawal/t3code
Length of output: 1233
Tie the temp directory to the harness-owned lifecycle.
rootDir is acquired with makeTempDirectoryScoped, but dispose only closes the reactor scope (line 353 and 464). The temp directory lifetime is now decoupled from the harness lifecycle: it will be cleaned up by the outer generator scope, not by dispose. Create rootDir under a harness-owned Scope and close that same scope in dispose, or restore explicit cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/integration/OrchestrationEngineHarness.integration.ts` around
lines 237 - 246, rootDir is created with makeTempDirectoryScoped but its scope
is not closed in the harness, so the temp directory outlives the harness;
allocate a harness-owned Scope (e.g., harnessScope) and call
makeTempDirectoryScoped within that scope to produce rootDir (or capture the
returned disposal handle) and store the scope/handle on the harness; then update
dispose to close that same harnessScope (or call the temp-directory disposal) so
the temporary directory is reliably cleaned up when dispose runs; refer to
makeTempDirectoryScoped, rootDir, deriveServerPaths, and dispose to locate where
to attach and close the scope.
| const ServerConfigLayer = ServerConfig.layerTest(process.cwd(), { | ||
| prefix: "t3-git-manager-test-", | ||
| }); | ||
|
|
||
| const gitCoreLayer = GitCoreLive.pipe( | ||
| Layer.provideMerge(GitServiceLive), | ||
| Layer.provideMerge(NodeServices.layer), | ||
| Layer.provideMerge(ServerConfigLayer), | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Effect-TS, if a Layer allocates a scoped resource (for example via makeTempDirectoryScoped) and that layer is supplied with Effect.provide(effect, layer), is the scoped resource finalized when the provided effect completes?
💡 Result:
Yes—when you do Effect.provide(effect, layer), the layer is built into a scope tied to the lifetime of the provided effect, so any scoped resources the layer acquires (e.g. via makeTempDirectoryScoped / Layer.scoped(...)) are finalized when that effect finishes (success, failure, or interruption), because the scope is closed at that point and layer resources are released on scope close. [1] [2]
The main exception is if you intentionally extend the layer’s scope (e.g. Layer.extendScope(layer)), in which case the resources are not released when the provided effect completes, but only when the outer scope is closed. [3]
Sources:
[1] https://effect-ts.github.io/effect/effect/Layer.ts.html
[2] https://effect-ts.github.io/effect/effect/Effect.ts.html
[3] https://effect-ts.github.io/effect/effect/Layer.ts.html
🏁 Script executed:
# Find the ServerConfig implementation to verify if layerTest uses makeTempDirectoryScoped
fd -e ts -e js config | grep -E "(config|Config)" | head -20Repository: aaditagrawal/t3code
Length of output: 279
🏁 Script executed:
# Search for ServerConfig.layerTest definition
rg -A 10 "layerTest" --type tsRepository: aaditagrawal/t3code
Length of output: 37350
🏁 Script executed:
# Look for makeTempDir and makeTempDirectoryScoped definitions
rg "makeTempDir" --type tsRepository: aaditagrawal/t3code
Length of output: 10867
🏁 Script executed:
# Examine the actual test file to confirm the code context
cat -n apps/server/src/git/Layers/GitManager.test.ts | sed -n '500,550p'Repository: aaditagrawal/t3code
Length of output: 2306
🏁 Script executed:
# Check how makeManager() is called in the tests
rg "makeManager\(\)" apps/server/src/git/Layers/GitManager.test.ts -A 3 | head -40Repository: aaditagrawal/t3code
Length of output: 1171
🏁 Script executed:
# Look at the full context of makeManager usage - check if manager is used after Effect.runPromise or similar
rg "manager\." apps/server/src/git/Layers/GitManager.test.ts -B 2 -A 1 | head -50Repository: aaditagrawal/t3code
Length of output: 1095
🏁 Script executed:
# Verify the actual signature and behavior of makeTempDir in GitManager.test.ts
cat -n apps/server/src/git/Layers/GitManager.test.ts | sed -n '490,510p'Repository: aaditagrawal/t3code
Length of output: 785
🏁 Script executed:
# Check if there are any other test patterns that properly handle scoped resources for comparison
rg "Effect.gen.*makeTempDir" apps/server/src/git/Layers/GitManager.test.ts -B 2 -A 10 | head -60Repository: aaditagrawal/t3code
Length of output: 45
🏁 Script executed:
# Find the makeTempDir function definition in GitManager.test.ts
rg -A 5 "^function makeTempDir" apps/server/src/git/Layers/GitManager.test.tsRepository: aaditagrawal/t3code
Length of output: 349
🏁 Script executed:
# Check actual test examples showing how makeManager() is used and how the manager is consumed
cat -n apps/server/src/git/Layers/GitManager.test.ts | sed -n '543,580p'Repository: aaditagrawal/t3code
Length of output: 1709
🏁 Script executed:
# Check if there are other test files that handle scoped resources correctly by examining similar patterns
rg "Effect.gen.*yield\*.*makeManager" apps/server/src/git/Layers/GitManager.test.ts -B 5 -A 15 | head -80Repository: aaditagrawal/t3code
Length of output: 45
🏁 Script executed:
# Look at how other test helpers in OrchestrationEngine handle baseDir allocation to see the correct pattern
grep -A 30 "function makeManager" apps/server/src/orchestration/Layers/ProviderCommandReactor.test.ts | head -40Repository: aaditagrawal/t3code
Length of output: 45
🏁 Script executed:
# Search for how baseDir is allocated in other tests - look at ProviderCommandReactor test
rg -B 5 -A 20 "ServerConfig.layerTest.*baseDir" apps/server/src/orchestration/Layers/ProviderCommandReactor.test.tsRepository: aaditagrawal/t3code
Length of output: 1245
🏁 Script executed:
# Check how the return type of makeManager is used - is it an Effect or a raw value?
cat -n apps/server/src/git/Layers/GitManager.test.ts | sed -n '535,540p'Repository: aaditagrawal/t3code
Length of output: 229
Keep the test baseDir alive for the full manager lifetime.
ServerConfig.layerTest(..., { prefix }) allocates baseDir via makeTempDirectoryScoped, but here that layer is only used inside Effect.provide(managerLayer) to construct and return manager. The scoped resource is finalized when that effect completes, before later manager.* calls run, which makes the worktree-path tests operate against a deleted base directory.
🛠️ Suggested fix
function makeManager(input?: {
ghScenario?: FakeGhScenario;
textGeneration?: Partial<FakeGitTextGeneration>;
sessionTextGeneration?: Partial<FakeGitTextGeneration>;
}) {
- const { service: gitHubCli, ghCalls } = createGitHubCliWithFakeGh(input?.ghScenario);
- const textGeneration = createTextGeneration(input?.textGeneration);
- const sessionTextGeneration = createSessionTextGeneration(input?.sessionTextGeneration);
- const ServerConfigLayer = ServerConfig.layerTest(process.cwd(), {
- prefix: "t3-git-manager-test-",
- });
-
- const gitCoreLayer = GitCoreLive.pipe(
- Layer.provideMerge(GitServiceLive),
- Layer.provideMerge(NodeServices.layer),
- Layer.provideMerge(ServerConfigLayer),
- );
-
- const managerLayer = Layer.mergeAll(
- Layer.succeed(GitHubCli, gitHubCli),
- Layer.succeed(TextGeneration, textGeneration),
- Layer.succeed(SessionTextGeneration, sessionTextGeneration),
- gitCoreLayer,
- ).pipe(Layer.provideMerge(NodeServices.layer));
-
- return makeGitManager.pipe(
- Effect.provide(managerLayer),
- Effect.map((manager) => ({ manager, ghCalls })),
- );
+ return Effect.gen(function* () {
+ const baseDir = yield* makeTempDir("t3-git-manager-test-");
+ const { service: gitHubCli, ghCalls } = createGitHubCliWithFakeGh(input?.ghScenario);
+ const textGeneration = createTextGeneration(input?.textGeneration);
+ const sessionTextGeneration = createSessionTextGeneration(input?.sessionTextGeneration);
+ const ServerConfigLayer = ServerConfig.layerTest(process.cwd(), baseDir);
+
+ const gitCoreLayer = GitCoreLive.pipe(
+ Layer.provideMerge(GitServiceLive),
+ Layer.provideMerge(NodeServices.layer),
+ Layer.provideMerge(ServerConfigLayer),
+ );
+
+ const managerLayer = Layer.mergeAll(
+ Layer.succeed(GitHubCli, gitHubCli),
+ Layer.succeed(TextGeneration, textGeneration),
+ Layer.succeed(SessionTextGeneration, sessionTextGeneration),
+ gitCoreLayer,
+ ).pipe(Layer.provideMerge(NodeServices.layer));
+
+ const manager = yield* makeGitManager.pipe(Effect.provide(managerLayer));
+ return { manager, ghCalls };
+ });
}Also applies to: 528-538
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/server/src/git/Layers/GitManager.test.ts` around lines 518 - 526, The
temporary baseDir created by ServerConfig.layerTest (via
makeTempDirectoryScoped) is being finalized before manager methods run because
ServerConfigLayer is only provided when constructing manager via
Effect.provide(managerLayer); fix by making the ServerConfigLayer live for the
full manager lifetime—either merge ServerConfigLayer into the managerLayer
composition (e.g., include ServerConfigLayer via Layer.provideMerge when
building gitCoreLayer/managerLayer so it remains part of the returned manager's
scope) or build the manager inside a scoped effect (use
Effect.scoped/Layer.build and run manager methods inside that scope) so the
temporary baseDir created by ServerConfig.layerTest remains alive while manager
methods (like manager.* in tests) execute.
| it("exports one provider config per provider", () => { | ||
| expect(MODEL_PROVIDER_SETTINGS.map((config) => config.provider)).toEqual([ | ||
| "codex", | ||
| "claudeAgent", | ||
| ]); | ||
| }); | ||
|
|
||
| it("reads custom models for each provider", () => { | ||
| expect(getCustomModelsForProvider(settings, "codex")).toEqual(["custom/codex-model"]); | ||
| expect(getCustomModelsForProvider(settings, "claudeAgent")).toEqual(["claude/custom-opus"]); | ||
| }); | ||
|
|
||
| it("reads default custom models for each provider", () => { | ||
| const defaults = { | ||
| customCodexModels: ["default/codex-model"], | ||
| customClaudeModels: ["claude/default-opus"], | ||
| customCopilotModels: [], | ||
| customCursorModels: [], | ||
| customOpencodeModels: [], | ||
| customGeminiCliModels: [], | ||
| customAmpModels: [], | ||
| customKiloModels: [], | ||
| } as const; | ||
|
|
||
| expect(getDefaultCustomModelsForProvider(defaults, "codex")).toEqual(["default/codex-model"]); | ||
| expect(getDefaultCustomModelsForProvider(defaults, "claudeAgent")).toEqual([ | ||
| "claude/default-opus", | ||
| ]); | ||
| }); | ||
|
|
||
| it("patches custom models for codex", () => { | ||
| expect(patchCustomModels("codex", ["custom/codex-model"])).toEqual({ | ||
| customCodexModels: ["custom/codex-model"], | ||
| }); | ||
| }); | ||
|
|
||
| it("patches custom models for claude", () => { | ||
| expect(patchCustomModels("claudeAgent", ["claude/custom-opus"])).toEqual({ | ||
| customClaudeModels: ["claude/custom-opus"], | ||
| }); | ||
| }); | ||
|
|
||
| it("builds a complete provider-indexed custom model record", () => { | ||
| expect(getCustomModelsByProvider(settings)).toEqual({ | ||
| codex: ["custom/codex-model"], | ||
| claudeAgent: ["claude/custom-opus"], | ||
| }); |
There was a problem hiding this comment.
Fix the provider-count expectations in this block.
These assertions still hard-code the old 2-provider output, but the shared helpers now intentionally expose all 8 providers. Vitest is already red here on Line 270 and Line 312.
🧪 Suggested expectation update
- expect(MODEL_PROVIDER_SETTINGS.map((config) => config.provider)).toEqual([
- "codex",
- "claudeAgent",
- ]);
+ expect(MODEL_PROVIDER_SETTINGS.map((config) => config.provider)).toEqual([
+ "codex",
+ "copilot",
+ "claudeAgent",
+ "cursor",
+ "opencode",
+ "geminiCli",
+ "amp",
+ "kilo",
+ ]);
@@
- expect(getCustomModelsByProvider(settings)).toEqual({
- codex: ["custom/codex-model"],
- claudeAgent: ["claude/custom-opus"],
- });
+ expect(getCustomModelsByProvider(settings)).toEqual({
+ codex: ["custom/codex-model"],
+ copilot: [],
+ claudeAgent: ["claude/custom-opus"],
+ cursor: [],
+ opencode: [],
+ geminiCli: [],
+ amp: [],
+ kilo: [],
+ });🧰 Tools
🪛 GitHub Actions: CI
[error] 270-270: Vitest failed test "exports one provider config per provider" with AssertionError: expected [ 'codex', 'claudeAgent' ] but received ['codex','copilot','claudeAgent','cursor','opencode','geminiCli','amp','kilo'] (MODEL_PROVIDER_SETTINGS.map(...).provider).
[error] 312-312: Vitest failed test "builds a complete provider-indexed custom model record" with AssertionError: expected { codex: ["custom/codex-model"], claudeAgent: ["claude/custom-opus"] } but received additional empty provider keys/arrays: copilot: [], cursor: [], opencode: [], geminiCli: [], kilo: [], amp: []. (getCustomModelsByProvider(settings)).
🪛 GitHub Check: Format, Lint, Typecheck, Test, Browser Test, Build
[failure] 312-312: src/appSettings.test.ts > provider-indexed custom model settings > builds a complete provider-indexed custom model record
AssertionError: expected { …(8) } to deeply equal { …(2) }
- Expected
-
Received
{
-
"amp": [],
"claudeAgent": [
"claude/custom-opus",
],
"codex": [
"custom/codex-model",
], -
"copilot": [],
-
"cursor": [],
-
"geminiCli": [],
-
"kilo": [],
-
"opencode": [],
}
❯ src/appSettings.test.ts:312:49
[failure] 270-270: src/appSettings.test.ts > provider-indexed custom model settings > exports one provider config per provider
AssertionError: expected [ 'codex', 'copilot', …(6) ] to deeply equal [ 'codex', 'claudeAgent' ]
- Expected
-
Received
[
"codex", -
"copilot",
"claudeAgent", -
"cursor",
-
"opencode",
-
"geminiCli",
-
"amp",
-
"kilo",
]
❯ src/appSettings.test.ts:270:70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/appSettings.test.ts` around lines 269 - 315, The tests hard-code
expectations for only two providers but the shared helpers now expose all 8
providers; update the assertions to reflect the full provider list and their
defaults. Replace the expected array in the MODEL_PROVIDER_SETTINGS test to
include all 8 provider.provider values, update getCustomModelsForProvider
expectations to assert the custom model lists for each of the 8 providers (using
their actual names as returned by MODEL_PROVIDER_SETTINGS), update the defaults
object and getDefaultCustomModelsForProvider assertions to include the 8
corresponding default keys (e.g., customCodexModels, customClaudeModels,
customCopilotModels, customCursorModels, customOpencodeModels,
customGeminiCliModels, customAmpModels, customKiloModels), adjust the
patchCustomModels assertions for any additional providers if needed, and change
the getCustomModelsByProvider expectation to include entries for all 8 providers
mapping to their expected model arrays.
| function resolveBaseDir(baseDir: string | undefined): Effect.Effect<string, never, Path.Path> { | ||
| return Effect.gen(function* () { | ||
| const path = yield* Path.Path; | ||
| const configured = stateDir?.trim(); | ||
| const configured = baseDir?.trim(); | ||
|
|
||
| if (configured) { | ||
| // Resolve relative paths against cwd (monorepo root) before turbo changes directories. | ||
| return path.resolve(configured); | ||
| } | ||
|
|
||
| return yield* DEFAULT_DEV_STATE_DIR; | ||
| return yield* DEFAULT_T3_HOME; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'NODE'
const path = require("node:path");
const os = require("node:os");
const input = "~/.t3-review-check";
console.log({ input, resolved: path.resolve(input), home: os.homedir() });
NODERepository: aaditagrawal/t3code
Length of output: 175
🏁 Script executed:
# Check if os-jank.ts exists and how it handles path resolution
find . -name "os-jank.ts" -type fRepository: aaditagrawal/t3code
Length of output: 91
🏁 Script executed:
# Read the relevant lines from scripts/dev-runner.ts
sed -n '100,120p' scripts/dev-runner.ts
sed -n '145,165p' scripts/dev-runner.tsRepository: aaditagrawal/t3code
Length of output: 1394
🏁 Script executed:
# Read the os-jank.ts file to see how it differs in path resolution
cat -n ./apps/server/src/os-jank.tsRepository: aaditagrawal/t3code
Length of output: 1309
Expand ~ in --home-dir before resolving paths.
The path.resolve() function does not expand the home directory; a leading ~ is treated as a literal path segment. This means dev-runner --home-dir ~/.t3-dev incorrectly resolves to a path like /path/to/cwd/~/.t3-dev instead of /home/user/.t3-dev, exporting the wrong T3CODE_HOME.
The server already has the correct implementation in apps/server/src/os-jank.ts (the expandHomePath function). Reuse that resolver, or add explicit tilde expansion using OS.homedir() and Path.join() before calling path.resolve().
This affects both resolveBaseDir() calls (lines 104–114 and 149–159).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/dev-runner.ts` around lines 104 - 114, The resolveBaseDir function
currently passes a configured baseDir directly to Path.resolve, which treats a
leading "~" as a literal segment; update resolveBaseDir to expand a leading
tilde before resolving: either import and call the existing expandHomePath from
apps/server/src/os-jank.ts (preferred) or implement expansion by replacing a
leading "~" with OS.homedir() joined via Path.join, then call Path.resolve on
the expanded value; ensure both places that call resolveBaseDir and the
DEFAULT_T3_HOME fallback behavior remain correct.
7 participants
Summary
Integrates 7 upstream commits from
pingdotgg/t3code, preserving all 8 providers.Upstream commits cherry-picked:
bc124217— Show project status dots when collapsed in sidebar (Show project status dots when collapsed in the sidebar pingdotgg/t3code#1097)46e63b36— fix(ChatView): scroll button style improvement (fix(ChatView): update scroll button styles to improve user interaction pingdotgg/t3code#1246)25c70032— fix(web): health banner for selected provider (fix(web): show health banner for selected provider instead of defaulting to Codex pingdotgg/t3code#1223)34cef620— fix(web): add-project toggle label in cancel state (fix(web): update add-project toggle label in cancel state pingdotgg/t3code#1248)ef256919— Prefer Claude user ID for telemetry identity (Prefer Claude user ID for telemetry identity pingdotgg/t3code#1249)9e29c9d7— feat: configurable base directory (feat: added configurable base directory. pingdotgg/t3code#826) — 37 files, refactors all path derivationd3733897— feat(web): persist modelOptions, refactor provider-specific logic (feat(web): persist modelOptions, refactor provider specific logic pingdotgg/t3code#1121) — 20 files, introducescomposerProviderRegistryKey integration decisions:
Configurable base directory — Clean cherry-pick. Updated our
CopilotAdapterto use newattachmentsDirparameter (upstream only had codex + claude adapters to update).Provider registry — Adopted upstream's
composerProviderRegistrypattern (mapsProviderKind→ state/rendering functions) and extended it with entries for all 8 providers. Providers without custom traits pickers (copilot, opencode, geminiCli, amp, kilo) get minimal entries returning null. Cursor gets a custom entry preserving ourCursorTraitsPicker.appSettings — Took upstream's
ProviderCustomModelConfigandMODEL_PROVIDER_SETTINGSpatterns, extended both to all 8 providers. Kept ourwithDefaultshelper (handles both construction and decoding defaults — technically better than upstream's approach). Kept ourmigratePersistedAppSettingswithproviderAccentColorsandgitTextGenerationModelByProviderlegacy key migrations.composerDraftStore — Kept our version (already ahead of upstream with 8-provider
normalizeProviderModelOptionsand sticky model support).Verification
bun typecheck— 7/7 packages passbun lint— 0 errors, 12 warningsTest plan
bun typecheckpassesbun linthas no errorsSummary by CodeRabbit
Release Notes
New Features
Refactor
--home-dirflag andT3CODE_HOMEenvironment variableDocumentation