Merge upstream pingdotgg/t3code main (v0.0.5 → v0.0.9)#668
Closed
aaditagrawal wants to merge 26 commits intopingdotgg:mainfrom
Closed
Merge upstream pingdotgg/t3code main (v0.0.5 → v0.0.9)#668aaditagrawal wants to merge 26 commits intopingdotgg:mainfrom
aaditagrawal wants to merge 26 commits intopingdotgg:mainfrom
Conversation
Co-authored-by: codex <codex@users.noreply.github.com>
…hored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
Co-authored-by: codex <codex@users.noreply.github.com>
…and Kilo adapters Add provider adapter infrastructure for OpenCode, Copilot, Amp, GeminiCli, and Kilo alongside the existing Codex and Claude Code adapters. Includes server managers, runtime ingestion enhancements, provider health monitoring, UI settings for provider selection, sidebar improvements, accent color theming, and upstream PR track sync tooling.
- Fix claude-agent-sdk.d.ts query signature (prompt accepts string, options optional, Query return type) - Forward numTurns in rollbackThread for Kilo, OpenCode, GeminiCli, Amp adapters - Guard ClaudeCodeAdapter.startSession against duplicate threadIds - Reject unsupported attachments in ClaudeCodeAdapter instead of silently dropping - Replace ClaudeCodeAdapter rollback with explicit not-supported error - Add CopilotAdapter layer cleanup finalizer, fix turn bookkeeping, emit teardown events - Convert CopilotAdapter attachment validation to use Effect.forEach - Read workspace from OpenCode resume cursor, guard against unsupported attachments - Fix diff-only assistant turn suppression in ChatView - Preserve isCustom flag in ModelOptionEntry, add staleTime to model queries - Add WCAG contrast checking for accent colors with fallback - Derive contrast-safe terminal colors for ThreadTerminalDrawer - Drop invalid provider accent overrides instead of normalizing to default - Fail fast on duplicate provider adapter registration - Fix README headings, add Kilo to supported agents - Remove duplicate ProviderRuntimeIngestion test - Make Gemini CLI live tests explicitly opt-in - Fix hardcoded runtimeMode in GeminiCli listSessions - Add stable activeAssistantItemId for Amp content deltas - Kill child process on Kilo server start timeout - Derive repo URL from git remote in sync script - Update plan docs for multi-provider scope
ampServerManager: - Emit turn.completed when AMP process dies with active turn - Reject concurrent turns in sendTurn - Consistent tool item types between start/completion events - Sanitize raw provider output in JSONL parse logging - Guard against duplicate turn.completed emissions geminiCliServerManager: - Reject overlapping turns in sendTurn - Always emit terminal turn.completed on child close - Respect input.model override, reject unsupported attachments - Fix test to simulate closed-but-present session path - Add tests for concurrent turn rejection and attachment rejection kiloServerManager: - Reject unsupported attachments in sendTurn - Clean up listeners on server start promise resolve/reject - Remove double-cast by using ProviderRuntimeEvent directly - Remove no-op readJsonData pass-through function CopilotAdapter: - Reorder reconfigureSession to create new before destroying old - Guard against NaN percentUsed when limit is 0 - Clean up client on createSession/resumeSession failure - Update stale runtimeMode on session reuse early return Types and registry: - Replace any with unknown in SDKMessage for stricter typing - Use Effect.die for idiomatic duplicate registration error - Add comment explaining global usage accumulator intent - Fix test to use different thread for cross-thread verification Other: - Merge discovered models by slug preserving metadata in ChatView - Fix deriveRepoUrl .git suffix handling - Expand plan docs: credential security, rollback criteria, chaos testing
ampServerManager: - Reject unsupported attachments before mutating session state - Don't mark interrupted turn as ready; let close/error handlers finalize - Defer session deletion until child exit handlers emit terminal events geminiCliServerManager: - Assert stored geminiSessionId in init event test - Emit effectiveModel (input.model ?? session.model) in turn.started - Use signal param in close handler; emit "interrupted" state on SIGINT - Remove premature ready status in interruptTurn - listSessions exposes actual session status instead of coercing to "ready" kiloServerManager: - Preserve workspace in resume state alongside sessionId - Reject overlapping turns before overwriting activeTurnId - Emit terminal turn events on interrupt and stream failure paths ClaudeCodeAdapter: - classifyRequestType properly routes MCP/dynamic tools via switch - Clear lastError on successful turn completion and turn start CopilotAdapter: - Emit failed turn.completed (not completed) when session.idle follows error - Store tool item type on start, reuse on complete for lifecycle stability - Widen client leak catch to cover validateSessionConfiguration failures ChatView: - Remove staleTime: Infinity so model discovery respects shared cache TTL - Render pricing tier badges in grouped provider submenus sync-upstream-pr-tracks: - Add ssh:// URL format support in deriveRepoUrl - Throw on failure instead of falling back to hardcoded URL
ampServerManager: - Clean up dead/closed sessions in startSession instead of rejecting - Always delete session from map on terminal state - Finalize subagent tasks and drain tool items on close/error paths - Move state mutations after stdin.write with try/catch rollback - readThread/rollbackThread throw unsupported errors instead of fake snapshots geminiCliServerManager: - Return normalized runtimeMode from startSession (not raw input) - Finalize open assistant/tool items on all terminal paths ClaudeCodeAdapter: - Defer existing session teardown until replacement runtime is ready - Validate buildUserMessage before opening turn state/emitting turn.started CopilotAdapter: - Install new session before destroying old in reconfigureSession - Wrap previousSession.destroy in try/catch to keep replacement live - Reject overlapping sendTurn calls with ProviderAdapterValidationError ChatView: - Add serviceTier to plan refine/implement dispatch payloads - Add geminiCli and amp model discovery queries - Include both in mergeDiscoveredModels and useMemo deps kiloServerManager: - Accept optional existing manager in fetchKiloModels to avoid respawning
…95-364 feat: multi-provider adapter support
- Add Copilot turn tracking, cursor usage, command palette, plan sidebar, ghostty terminal split view, project thread navigation, and session text generation - Fix ProviderStartOptions schema: restore opencode/kilo fields and value re-export - Fix CopilotAdapter: restore concurrent-turn guard, session cleanup on failure, lastError lifecycle, teardown event emission, pending resolver cleanup, reconfigure error guard, attachment validation Effect channel - Fix orchestration: interrupt re-raise in approval/user-input handlers, threadProviderOptions cleanup on stop and delete - Fix frontend: CommandPalette render mutation, stale drag closure, assistantDeliveryMode dedup, timeout cleanups, onInterrupt memoization, PlanSidebar duplicate keys, settings navigation, onDragLeave handler - Fix server: satisfies vs as type safety, stopSession error logging, os.homedir() for macOS paths, editor app detection/launch mismatch
- ProviderCommandReactor: don't mark session stopped after failed stop; document provider options only apply at session start - ClaudeCodeAdapter: move executable path resolution inside Effect.try for proper error mapping in packaged environments - ProviderHealth: replace unsafe getQuotaInfo cast with documented client.rpc.account.getQuota() SDK API - ProviderService: preserve providerOptions across all directory.upsert calls (sendTurn, recovery, stopAll) to prevent silent data loss - ChatView: add removeKeybinding API to clean up stale shortcuts on script deletion; document snapshot resync trade-offs - CommandPalette: catch async onSelect failures to prevent unhandled promise rejections - ProjectScriptsControl: await onDeleteScript and surface errors via validation state instead of fire-and-forget - Settings: add aria-label to provider accent color pickers - wsTransport: close sockets that finish connecting after disposal - orchestration.ts: redact credentials (username/password) from persisted event payloads; use PositiveInt for port validation - install.sh: read prompts from /dev/tty for curl|bash compatibility; fix word-splitting vulnerability in find_bin path probing
CodeRabbit round 3: - decider.ts: move credential redaction to persistence/WS boundaries so ProviderCommandReactor receives full providerOptions at runtime - ProviderCommandReactor: stop active session on thread.deleted before cleanup; don't mark session stopped after failed stop - ChatView: await clipboard.writeText before showing success indicator; fix asymmetric drag enter/leave bookkeeping; scope thread rollback to pre-start failures only - ProviderHealth: fix catch(() => []) to catch(() => undefined) - install.sh: remove unreachable MINGW64* pattern; remove unused NODE_BIN Internal specialist audit: - Security: redact credentials in ProviderService runtimePayload before SQLite persistence (all 3 upsert boundaries) - Remove 6 unused imports in ChatView.tsx (lint warnings eliminated) - Extract toMessage() to shared module (deduplicated across 4 adapters) - CursorAdapter: early return in extractCursorChunkText to prevent duplicate text when both text and content fields are populated - SessionTextGeneration: handle session.exited event to fail fast instead of blocking 180s on provider process crash - CopilotAdapter: eliminate double trimToUndefined call
feat: new features with code review fixes
# Conflicts: # .github/VOUCHED.td # .github/workflows/pr-vouch.yml # CONTRIBUTING.md # README.md # apps/desktop/src/main.ts # apps/server/scripts/cli.ts # apps/server/src/orchestration/Layers/ProviderCommandReactor.ts # apps/server/src/orchestration/Layers/ProviderRuntimeIngestion.ts # apps/server/src/provider/Layers/ProviderService.ts # apps/web/src/components/ChatView.tsx # apps/web/src/components/GitActionsControl.tsx # apps/web/src/components/PlanSidebar.tsx # apps/web/src/components/ProjectScriptsControl.tsx # apps/web/src/components/Sidebar.tsx # packages/contracts/src/orchestration.ts # packages/contracts/src/provider.ts
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Author
|
Opened against wrong repo — this should target the fork. Closing. |
| state.currentTurnId = state.pendingTurnIds.shift() ?? state.currentTurnId ?? providerTurnId; |
Contributor
There was a problem hiding this comment.
🟢 Low Layers/copilotTurnTracking.ts:30
When pendingTurnIds is empty, state.currentTurnId is set to itself instead of the new providerTurnId, causing all subsequent turns to reuse the first turn's ID. The expression state.pendingTurnIds.shift() ?? state.currentTurnId ?? providerTurnId prevents providerTurnId from ever being used once currentTurnId is populated. Consider removing ?? state.currentTurnId so the fallback chain correctly prefers the new providerTurnId when no client ID is pending.
Suggested change
| state.currentTurnId = state.pendingTurnIds.shift() ?? state.currentTurnId ?? providerTurnId; | |
| state.currentTurnId = state.pendingTurnIds.shift() ?? providerTurnId; |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/copilotTurnTracking.ts around line 30:
When `pendingTurnIds` is empty, `state.currentTurnId` is set to itself instead of the new `providerTurnId`, causing all subsequent turns to reuse the first turn's ID. The expression `state.pendingTurnIds.shift() ?? state.currentTurnId ?? providerTurnId` prevents `providerTurnId` from ever being used once `currentTurnId` is populated. Consider removing `?? state.currentTurnId` so the fallback chain correctly prefers the new `providerTurnId` when no client ID is pending.
Evidence trail:
apps/server/src/provider/Layers/copilotTurnTracking.ts line 30: `state.currentTurnId = state.pendingTurnIds.shift() ?? state.currentTurnId ?? providerTurnId;` - line 29 shows `state.currentProviderTurnId = providerTurnId;` which IS updated each turn. apps/server/src/provider/Layers/CopilotAdapter.ts lines 1063-1091 shows `beginCopilotTurn` called on each `assistant.turn_start` event, and `clearTurnTracking` only called on `session.idle` or `abort`. Test file copilotTurnTracking.test.ts shows initial state has `pendingTurnIds: []` and `currentTurnId: undefined`.
|
|
||
| // ── Session teardown ────────────────────────────────────────────── | ||
|
|
||
| stopSession(threadId: ThreadId): void { |
Contributor
There was a problem hiding this comment.
🟢 Low src/ampServerManager.ts:410
stopSession returns immediately after calling process.kill(), but the session entry is not removed from this.sessions until the asynchronous 'close' event fires. This creates a race where startSession immediately called after stopSession finds the stale entry and throws "AMP session already exists". Consider returning a promise that resolves after cleanup completes.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/ampServerManager.ts around line 410:
`stopSession` returns immediately after calling `process.kill()`, but the session entry is not removed from `this.sessions` until the asynchronous `'close'` event fires. This creates a race where `startSession` immediately called after `stopSession` finds the stale entry and throws "AMP session already exists". Consider returning a promise that resolves after cleanup completes.
Evidence trail:
apps/server/src/ampServerManager.ts lines 410-421 (stopSession returns void, calls process.kill() and returns immediately), lines 250-275 (close event handler where session.status is set to 'closed' at line 267 and this.sessions.delete(threadId) at line 275), lines 182-188 (startSession checks existing session and throws if status !== 'closed')
Comment on lines
+22
to
+31
| export function beginCopilotTurn( | ||
| state: CopilotTurnTrackingState, | ||
| providerTurnId: TurnId, | ||
| ): void { | ||
| state.pendingCompletionTurnId = undefined; | ||
| state.pendingCompletionProviderTurnId = undefined; | ||
| state.pendingTurnUsage = undefined; | ||
| state.currentProviderTurnId = providerTurnId; | ||
| state.currentTurnId = state.pendingTurnIds.shift() ?? state.currentTurnId ?? providerTurnId; | ||
| } |
Contributor
There was a problem hiding this comment.
Layers/copilotTurnTracking.ts:22
beginCopilotTurn clears pendingCompletionTurnId, pendingCompletionProviderTurnId, and pendingTurnUsage on lines 26–28. If a new turn starts before the previous turn's async completion logic finishes (e.g., usage event still pending), this wipes Turn A's buffered data and completion references. Any subsequent completionTurnRefs call falls back to state.currentTurnId (now Turn B), causing Turn A's metrics to be incorrectly attributed to Turn B. Consider preserving the pending completion state until it is explicitly consumed rather than clearing it on every new turn.
+export function beginCopilotTurn(
+ state: CopilotTurnTrackingState,
+ providerTurnId: TurnId,
+): void {
+ state.currentProviderTurnId = providerTurnId;
+ state.currentTurnId = state.pendingTurnIds.shift() ?? state.currentTurnId ?? providerTurnId;
+}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/copilotTurnTracking.ts around lines 22-31:
`beginCopilotTurn` clears `pendingCompletionTurnId`, `pendingCompletionProviderTurnId`, and `pendingTurnUsage` on lines 26–28. If a new turn starts before the previous turn's async completion logic finishes (e.g., usage event still pending), this wipes Turn A's buffered data and completion references. Any subsequent `completionTurnRefs` call falls back to `state.currentTurnId` (now Turn B), causing Turn A's metrics to be incorrectly attributed to Turn B. Consider preserving the pending completion state until it is explicitly consumed rather than clearing it on every new turn.
Evidence trail:
apps/server/src/provider/Layers/copilotTurnTracking.ts:26-28 (beginCopilotTurn clears pending state), apps/server/src/provider/Layers/copilotTurnTracking.ts:15-19 (completionTurnRefs fallback logic), apps/server/src/provider/Layers/CopilotAdapter.ts:1063 (beginCopilotTurn called on turn_start), apps/server/src/provider/Layers/CopilotAdapter.ts:1090 (markTurnAwaitingCompletion called on turn_end), apps/server/src/provider/Layers/CopilotAdapter.ts:713 (completionTurnRefs used for usage event)
Comment on lines
+462
to
+485
| private handleOutputLine(threadId: ThreadId, line: string): void { | ||
| const session = this.sessions.get(threadId); | ||
| if (!session) return; | ||
|
|
||
| const trimmed = line.trim(); | ||
| if (!trimmed) return; | ||
|
|
||
| let msg: AmpJsonlMessage; | ||
| try { | ||
| msg = JSON.parse(trimmed) as AmpJsonlMessage; | ||
| } catch { | ||
| // Non-JSON output — treat as raw assistant text. | ||
| this.logger.warn("Failed to parse JSONL line", { length: trimmed.length }); | ||
| this.emitEvent(threadId, session.activeTurnId, { | ||
| type: "content.delta", | ||
| payload: { | ||
| streamKind: "assistant_text", | ||
| delta: trimmed + "\n", | ||
| }, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| switch (msg.type) { |
Contributor
There was a problem hiding this comment.
🟢 Low src/ampServerManager.ts:462
handleOutputLine parses untrusted input with JSON.parse and immediately accesses msg.type. If the external process sends the string "null", JSON.parse returns null, causing a TypeError: Cannot read property 'type' of null that crashes the Node.js process. Consider checking that msg is a non-null object before the switch statement.
- switch (msg.type) {
+ if (!msg || typeof msg !== "object") {
+ this.logger.warn("Invalid JSONL message", { line: trimmed.slice(0, 200) });
+ return;
+ }
+ switch (msg.type) {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/ampServerManager.ts around lines 462-485:
`handleOutputLine` parses untrusted input with `JSON.parse` and immediately accesses `msg.type`. If the external process sends the string `"null"`, `JSON.parse` returns `null`, causing a `TypeError: Cannot read property 'type' of null` that crashes the Node.js process. Consider checking that `msg` is a non-null object before the switch statement.
Evidence trail:
apps/server/src/ampServerManager.ts lines 462-485 (at REVIEWED_COMMIT): `handleOutputLine` method parses JSON at line 470 with `msg = JSON.parse(trimmed) as AmpJsonlMessage`, catch block handles parse errors at lines 471-480, then line 485 has `switch (msg.type)` with no null check between parsing and access.
| commandId: event.commandId, | ||
| payloadJson: event.payload, | ||
| metadataJson: event.metadata, | ||
| const append: OrchestrationEventStoreShape["append"] = (event) => { |
Contributor
There was a problem hiding this comment.
🟢 Low Layers/OrchestrationEventStore.ts:182
The append method stores a redacted event to disk and returns the redacted version to the caller. This strips username/password from providerOptions before the event is handed back, so any immediate downstream use of that event (e.g., calling an LLM provider) fails due to missing credentials. Return the original unredacted event augmented with the assigned sequence instead, keeping the redaction for persistence only.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/persistence/Layers/OrchestrationEventStore.ts around line 182:
The `append` method stores a redacted event to disk and returns the redacted version to the caller. This strips `username`/`password` from `providerOptions` before the event is handed back, so any immediate downstream use of that event (e.g., calling an LLM provider) fails due to missing credentials. Return the original unredacted event augmented with the assigned `sequence` instead, keeping the redaction for persistence only.
Evidence trail:
apps/server/src/persistence/Layers/OrchestrationEventStore.ts lines 182-195 (append method redacts then stores), apps/server/src/orchestration/redactEvent.ts lines 31-45 (redactEventForBoundary strips username/password), apps/server/src/orchestration/Layers/OrchestrationEngine.ts lines 124-165 (savedEvent pushed to eventPubSub), apps/server/src/orchestration/Layers/ProviderCommandReactor.ts lines 762-775 (subscribes to streamDomainEvents), lines 455-501 (processTurnStartRequested uses event.payload.providerOptions), lines 354-362 (sendTurnForThread passes providerOptions), lines 240-259 (effectiveProviderOptions passed to providerService.startSession)
Comment on lines
+728
to
+741
| function readProviderListResponse( | ||
| value: | ||
| | ProviderListResponse | ||
| | { data: ProviderListResponse; error?: undefined } | ||
| | { data?: undefined; error: unknown }, | ||
| ): ProviderListResponse { | ||
| if ("all" in value && "connected" in value) { | ||
| return value; | ||
| } | ||
| if (value.data !== undefined) { | ||
| return value.data; | ||
| } | ||
| throw new Error("OpenCode SDK returned an empty provider list response"); | ||
| } |
Contributor
There was a problem hiding this comment.
🟢 Low src/opencodeServerManager.ts:728
When the SDK returns an error response like { error: "Unauthorized" }, readProviderListResponse falls through to the final throw with message "OpenCode SDK returned an empty provider list response", suppressing the actual error. Consider rethrowing or including value.error in the error message so callers can see the root cause.
- if (value.data !== undefined) {
- return value.data;
- }
- throw new Error("OpenCode SDK returned an empty provider list response");
+ if (value.data !== undefined) {
+ return value.data;
+ }
+ throw new Error(`OpenCode SDK returned an empty provider list response: ${value.error}`);
Also found in 1 other location(s)
apps/server/src/kiloServerManager.ts:703
The function
readProviderListResponsefails to handle the error case of the input union type. Ifvalueis an error wrapper (e.g.{ error: "Access Denied", data: undefined }),value.datais undefined, causing the code to fall through to line 703. This throws a genericError("Kilo SDK returned an empty provider list response")instead of propagating the actual error. This swallows the underlying exception from the SDK, making debugging impossible. The function should check for and throwvalue.errorif present.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/opencodeServerManager.ts around lines 728-741:
When the SDK returns an error response like `{ error: "Unauthorized" }`, `readProviderListResponse` falls through to the final `throw` with message "OpenCode SDK returned an empty provider list response", suppressing the actual error. Consider rethrowing or including `value.error` in the error message so callers can see the root cause.
Evidence trail:
apps/server/src/opencodeServerManager.ts lines 728-743 at REVIEWED_COMMIT - The `readProviderListResponse` function accepts `{ data?: undefined; error: unknown }` as a valid input type, but the function body only checks for `"all" in value && "connected" in value` and `value.data !== undefined`. When an error response is passed, both conditions are false, and the function throws a generic error without using `value.error`.
Also found in 1 other location(s):
- apps/server/src/kiloServerManager.ts:703 -- The function `readProviderListResponse` fails to handle the error case of the input union type. If `value` is an error wrapper (e.g. `{ error: "Access Denied", data: undefined }`), `value.data` is undefined, causing the code to fall through to line 703. This throws a generic `Error("Kilo SDK returned an empty provider list response")` instead of propagating the actual error. This swallows the underlying exception from the SDK, making debugging impossible. The function should check for and throw `value.error` if present.
Comment on lines
+45
to
+61
| export function fetchAmpUsage(): ProviderUsageResult { | ||
| const acc = _ampUsageAccumulator; | ||
| let sessionUsage: ProviderSessionUsage | undefined; | ||
| if (acc.turnCount > 0) { | ||
| sessionUsage = { | ||
| inputTokens: acc.inputTokens, | ||
| outputTokens: acc.outputTokens, | ||
| ...(acc.cachedTokens > 0 ? { cachedTokens: acc.cachedTokens } : {}), | ||
| totalTokens: acc.inputTokens + acc.outputTokens, | ||
| turnCount: acc.turnCount, | ||
| }; | ||
| } | ||
| return { | ||
| provider: PROVIDER, | ||
| ...(sessionUsage ? { sessionUsage } : {}), | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
src/ampServerManager.ts:45
fetchAmpUsage returns sessionUsage only when turnCount > 0, but token usage is accumulated in _ampUsageAccumulator even when a session crashes or is interrupted before turnCount increments. This causes the function to return { provider: "amp" } with no usage data even when tokens were consumed, leading to silent under-reporting of costs for failed or interrupted sessions. Consider gating on total token presence rather than turn count, or ensure turnCount is incremented whenever usage is recorded.
+export function fetchAmpUsage(): ProviderUsageResult {
+ const acc = _ampUsageAccumulator;
+ let sessionUsage: ProviderSessionUsage | undefined;
+ if (acc.inputTokens > 0 || acc.outputTokens > 0) {
+ sessionUsage = {
+ inputTokens: acc.inputTokens,
+ outputTokens: acc.outputTokens,
+ ...(acc.cachedTokens > 0 ? { cachedTokens: acc.cachedTokens } : {}),
+ totalTokens: acc.inputTokens + acc.outputTokens,
+ turnCount: acc.turnCount,
+ };
+ }
+ return {
+ provider: PROVIDER,
+ ...(sessionUsage ? { sessionUsage } : {}),
+ };
+}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/ampServerManager.ts around lines 45-61:
`fetchAmpUsage` returns `sessionUsage` only when `turnCount > 0`, but token usage is accumulated in `_ampUsageAccumulator` even when a session crashes or is interrupted before `turnCount` increments. This causes the function to return `{ provider: "amp" }` with no usage data even when tokens were consumed, leading to silent under-reporting of costs for failed or interrupted sessions. Consider gating on total token presence rather than turn count, or ensure `turnCount` is incremented whenever usage is recorded.
Evidence trail:
apps/server/src/ampServerManager.ts lines 45-57 (fetchAmpUsage returns sessionUsage only when turnCount > 0), lines 431-443 (token accumulation occurs whenever inner.usage is present), line 452 (turnCount++ only when stop_reason === 'end_turn'), lines 229-249 (close handler doesn't increment turnCount), lines 251-272 (error handler doesn't increment turnCount), lines 315-322 (interruptTurn doesn't increment turnCount). REVIEWED_COMMIT.
Comment on lines
+1267
to
+1275
| const onChunk = (chunk: Buffer) => { | ||
| output += chunk.toString(); | ||
| const url = parseServerUrl(output); | ||
| if (!url) { | ||
| return; | ||
| } | ||
| clearTimeout(timeout); | ||
| resolve(url); | ||
| }; |
Contributor
There was a problem hiding this comment.
🟡 Medium src/opencodeServerManager.ts:1267
The onChunk listener is never removed from child.stdout and child.stderr after the server URL is detected (line 1273). The listener keeps accumulating all server output into the output string for the lifetime of the child process, causing unbounded memory growth and repeated parseServerUrl calls on ever-growing data. Consider removing the listeners with child.stdout.off('data', onChunk) and child.stderr.off('data', onChunk) after resolving the promise.
const onChunk = (chunk: Buffer) => {
output += chunk.toString();
const url = parseServerUrl(output);
if (!url) {
return;
}
clearTimeout(timeout);
+ child.stdout.off("data", onChunk);
+ child.stderr.off("data", onChunk);
resolve(url);
};🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/opencodeServerManager.ts around lines 1267-1275:
The `onChunk` listener is never removed from `child.stdout` and `child.stderr` after the server URL is detected (line 1273). The listener keeps accumulating all server output into the `output` string for the lifetime of the child process, causing unbounded memory growth and repeated `parseServerUrl` calls on ever-growing data. Consider removing the listeners with `child.stdout.off('data', onChunk)` and `child.stderr.off('data', onChunk)` after resolving the promise.
Evidence trail:
apps/server/src/opencodeServerManager.ts lines 1250-1310 at REVIEWED_COMMIT: Line 1267 defines `onChunk` that appends to `output` and calls `parseServerUrl`. Lines 1276-1277 attach listeners with `.on('data', onChunk)`. Line 1273 calls `resolve(url)` without removing the listeners. The child process is stored in `this.server.child` (line 1303-1307) and kept alive, meaning listeners remain active indefinitely.
| if (!trimmed || !trimmed.includes(" - ")) { | ||
| continue; | ||
| } | ||
| const match = /^(?<slug>[a-z0-9./-]+)\s+-\s+(?<name>.+?)(?:\s+\((?:current|default)\))*$/i.exec( |
Contributor
There was a problem hiding this comment.
🟢 Low Layers/CursorAdapter.ts:149
The regex [a-z0-9./-]+ on line 149 rejects model slugs containing colons or underscores, so identifiers like llama2:7b or my_model are silently discarded. Consider expanding the character class to include : and _.
- const match = /^(?<slug>[a-z0-9./-]+)\s+-\s+(?<name>.+?)(?:\s+\((?:current|default)\))*$/i.exec(
+ const match = /^(?<slug>[a-z0-9.:/_-]+)\s+-\s+(?<name>.+?)(?:\s+\((?:current|default)\))*$/i.exec(🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/CursorAdapter.ts around line 149:
The regex `[a-z0-9./-]+` on line 149 rejects model slugs containing colons or underscores, so identifiers like `llama2:7b` or `my_model` are silently discarded. Consider expanding the character class to include `:` and `_`.
Evidence trail:
apps/server/src/provider/Layers/CursorAdapter.ts lines 140-161 at REVIEWED_COMMIT. Line 149 shows regex `/^(?<slug>[a-z0-9./-]+)\s+-\s+(?<name>.+?)(?:\s+\((?:current|default)\))*$/i` where the character class `[a-z0-9./-]+` excludes colons and underscores. Lines 150-152 show `if (!match?.groups) { continue; }` which silently skips non-matching lines.
Comment on lines
+1054
to
+1056
| if (typeof value === "string" && value.length > 0) { | ||
| orderedAnswers[question.answerIndex] = [value]; | ||
| } |
Contributor
There was a problem hiding this comment.
🟢 Low src/kiloServerManager.ts:1054
In respondToUserInput, scalar answers that are not strings (numbers, booleans) are silently discarded because of typeof value === "string", leaving the answer slot as []. This contradicts the array branch which converts values with .map(String), implying non-strings should be converted rather than dropped. Consider converting scalar values with String(value) instead of filtering them out.
- if (typeof value === "string" && value.length > 0) {
- orderedAnswers[question.answerIndex] = [value];
+ if (value != null && value !== "" && value !== false) {
+ orderedAnswers[question.answerIndex] = [String(value)];
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/kiloServerManager.ts around lines 1054-1056:
In `respondToUserInput`, scalar answers that are not strings (numbers, booleans) are silently discarded because of `typeof value === "string"`, leaving the answer slot as `[]`. This contradicts the array branch which converts values with `.map(String)`, implying non-strings should be converted rather than dropped. Consider converting scalar values with `String(value)` instead of filtering them out.
Evidence trail:
apps/server/src/kiloServerManager.ts lines 1048-1057 at REVIEWED_COMMIT: Line 1048 initializes orderedAnswers with empty arrays. Lines 1051-1053 handle arrays with `.map(String)` conversion. Lines 1055-1056 only accept strings with `typeof value === "string" && value.length > 0`, meaning numeric/boolean scalars are never assigned and the slot remains `[]`.
Contributor
|
I really feel like you need to fix your workflow, because this isnt great that it keeps making PR's against the upstream repo |
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
3 participants
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
pingdotgg/t3code:maininto our forkdeleteProjectScriptdeclaration, mismatched parentheses inChatView.tsx)Upstream changes included
Bug fixes:
wss://when page is served over HTTPS (fix(web): use wss:// when page is served over HTTPS #391)Features & UI:
deleteoption for project actions (Adddeleteoption for project actions #223)Desktop:
CI/Infra:
Conflict resolution strategy
Test plan
bun typecheckpasses (all 7 packages)bun lintpasses (0 errors, 14 pre-existing warnings)🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Note
Merge upstream v0.0.5→v0.0.9 and add multi‑provider adapters, model selection (Cursor/Claude Code traits), provider usage/model APIs, and UI for command palette, Ghostty terminal, accent color, and project reorder across web, server, and shared contracts
Add provider adapters and session routing for
copilot,claudeCode,cursor,opencode,geminiCli,amp, andkilo, extend shared model contracts and resolution (Cursor families, Claude Code effort), expose providerlistModels/getUsagevia WebSocket and React Query, and update web UI with command palette, Ghostty terminal, accent color theming, provider model picker with traits, sidebar usage, thread search, and project drag‑reorder; persist project order and redact provider options in persisted/runtime events.📍Where to Start
Start with provider adapter registration in
makeProviderAdapterRegistryat apps/server/src/provider/Layers/ProviderAdapterRegistry.ts, then review shared model contracts in packages/contracts/src/model.ts and provider kind/options in packages/contracts/src/orchestration.ts.📊 Macroscope summarized 1087311. 29 files reviewed, 51 issues evaluated, 5 issues filtered, 12 comments posted
🗂️ Filtered Issues
apps/server/src/ampServerManager.ts — 4 comments posted, 9 evaluated, 1 filtered
sendTurnfunction constructs the user message payload using onlyinput.input(text), completely ignoring any tool results that may be present in theinputobject. When the assistant initiates a tool call (tool_use), the runtime executes it and callssendTurnwith the result. By omitting these results from the JSON sent to the AMP process (hardcodingtype: "text"), the assistant never receives the tool output, breaking the tool usage lifecycle. [ Out of scope (triage) ]apps/server/src/kiloServerManager.ts — 1 comment posted, 14 evaluated, 1 filtered
readProviderListResponsefails to handle the error case of the input union type. Ifvalueis an error wrapper (e.g.{ error: "Access Denied", data: undefined }),value.datais undefined, causing the code to fall through to line 703. This throws a genericError("Kilo SDK returned an empty provider list response")instead of propagating the actual error. This swallows the underlying exception from the SDK, making debugging impossible. The function should check for and throwvalue.errorif present. [ Cross-file consolidated ]apps/server/src/opencodeServerManager.ts — 2 comments posted, 8 evaluated, 1 filtered
handleMessagePartUpdatedEventfunction updates the internalpartStreamByIdmap fortextandreasoningparts but fails to emit any runtime event (e.g.,completion.chunkoritem.updated) before returning. This effectively swallows the AI's textual response, preventing it from being streamed to the client application, which will result in an empty or non-updating response in the UI. This contrasts with thetoolbranch, which correctly delegates to a handler that emits events. [ Out of scope (triage) ]apps/server/src/provider/Layers/ClaudeCodeAdapter.ts — 0 comments posted, 3 evaluated, 2 filtered
classifyToolItemTypefunction uses the single keyword "file" (line 348) to classify a tool asfile_change. This heuristic is overly broad and incorrect for standard read-only tools likeread_file,list_files,file_exists, orsearch_files. It causes these read operations to be categorized asfile_changeitems, potentially leading to incorrect UI states (e.g., displaying a diff view for a search operation) and feeding incorrect data into theclassifyRequestTypelogic. [ Cross-file consolidated ]logNativeSdkMessageusescrypto.randomUUID()which is neither imported nor guaranteed to be available in the global scope (e.g., in Node.js < 19 without flags or specific polyfills). This will cause aReferenceErrorand crash the event processing loop ifnativeEventLoggeris enabled. The codebase consistently usesyield* Random.nextUUIDv4from theeffectlibrary elsewhere (e.g., line 1590, 1889), and should do so here as well to ensure runtime compatibility and consistency with the Effect runtime. [ Out of scope (triage) ]