Integrate T3Code roadmap slices#98
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds WSL-aware backend resolution and discovery, managed worktree cleanup classification, provider update-check settings, a Copilot spike with ADR, OpenCode resume hardening, provider usage UI, chat markdown wrapping, and timeline activity details/minimap updates. ChangesBackend WSL Path Resolution and Registry
Managed Worktree Cleanup
Provider Update Checks and Copilot Spike
Chat Markdown Wrap and Provider Update Check Settings
Provider Usage UI and Runtime Controls
Composer Prompt Traits Extraction
Chat Timeline Activity Details and Minimap
Cockpit Design System Documentation
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
vouch:trusted
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
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/routes/_chat.settings.tsx (1)
540-543: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate thread-linking logic diverges from
classifyManagedWorktreeCleanupChoices.
normalizeManagedWorktreePathand thelinkedThreadsFromSnapshotfilter here re-implement the same worktree/thread path-matching logic already defined inworktreeCleanup.ts'snormalizeWorktreePathandclassifyManagedWorktreeCleanupChoices. Since this copy runs against a freshly-fetched snapshot rather than the memoizedmanagedWorktreeChoices, the two implementations can silently diverge (e.g., a future change to the matching rule in one file won't be reflected in the other), producing inconsistent confirmation-dialog messaging vs. actual cleanup-safety classification.♻️ Suggested consolidation
-function normalizeManagedWorktreePath(value: string | null | undefined): string | null { - const trimmed = value?.trim(); - return trimmed && trimmed.length > 0 ? trimmed : null; -} +// Reuse the shared helper from worktreeCleanup.ts instead of duplicating it here.Then export
normalizeWorktreePath(and ideally a smalllinkThreadsToWorktreePathhelper) fromworktreeCleanup.tsand call it from both the memoized classification and the fresh-snapshot lookup indeleteManagedWorktree.Also applies to: 1306-1312
🤖 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 `@apps/web/src/routes/_chat.settings.tsx` around lines 540 - 543, The thread-linking and worktree path matching logic in `deleteManagedWorktree`/`linkedThreadsFromSnapshot` is duplicated from `worktreeCleanup.ts`, which risks the two paths diverging over time. Move the canonical matching into `worktreeCleanup.ts` by exporting `normalizeWorktreePath` and, ideally, a shared `linkThreadsToWorktreePath` helper, then have both `classifyManagedWorktreeCleanupChoices` and the fresh-snapshot lookup in `deleteManagedWorktree` call that shared logic instead of `normalizeManagedWorktreePath` and the local filter.apps/web/src/components/BranchToolbar.tsx (1)
34-50: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winProvider usage resolution diverges from ChatView, causing inconsistent chip state for draft threads.
activeProviderhere only readsserverThread, so for any thread that hasn't started yet (serverThreadisundefined), it resolves tonullregardless of the composer's selected/default provider.usageSummaryis then computed from thatnullprovider, and bothBranchToolbarEnvironmentPicker's rate-limit panel and theRuntimeUsageControlsrendered here will show "no provider" data.Meanwhile,
ChatView.tsxcomputes its ownactiveProviderUsageSummarywith a full fallback chain (session.provider ?? modelSelection.provider ?? selectedProvider) and spreads it intobranchToolbarProps— butBranchToolbarPropsdoesn't declareprovider/providerRateLimits/providerUsageLines/providerUsageIsLoading/providerUsageLearnMoreHref, so those computed values are silently dropped when passed to<BranchToolbar {...branchToolbarProps} />. Worse,ChatViewrenders the sameRuntimeUsageControlswidget directly (with its own, more complete provider) wheneverisGitRepois false, so the exact same UI element shows different provider/usage data depending on whether the project is a git repo.🔧 Suggested fix: accept the already-computed provider/usage props instead of recomputing locally
interface BranchToolbarProps { threadId: ThreadId; className?: string; onEnvModeChange: (mode: EnvMode) => void; envLocked: boolean; runtimeMode?: RuntimeMode; onRuntimeModeChange?: (mode: RuntimeMode) => void; + activeProvider?: ProviderKind | null; onHandoffToWorktree?: () => void; onHandoffToLocal?: () => void; handoffBusy?: boolean; onCheckoutPullRequestRequest?: (reference: string) => void; onComposerFocusRequest?: () => void; contextWindow?: ContextWindowSnapshot | null; cumulativeCostUsd?: number | null; activeContextWindowLabel?: string | null; pendingContextWindowLabel?: string | null; }- const activeProvider = - serverThread?.session?.provider ?? serverThread?.modelSelection.provider ?? null; + const activeProvider = + activeProviderProp ?? + serverThread?.session?.provider ?? + serverThread?.modelSelection.provider ?? + null;Then thread
activeProvider(renamed prop) fromChatView's already-computed value instead of relying solely onserverThread.Also applies to: 83-84, 194-198, 244-259
🤖 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 `@apps/web/src/components/BranchToolbar.tsx` around lines 34 - 50, BranchToolbar is recomputing provider usage from serverThread and ignores the already-computed provider data coming from ChatView, which causes draft threads to show inconsistent “no provider” state. Update BranchToolbarProps and BranchToolbar to accept and use the provider/usage props that ChatView already prepares (activeProvider, providerRateLimits, providerUsageLines, providerUsageIsLoading, providerUsageLearnMoreHref) instead of deriving usage locally, and thread the renamed activeProvider through the BranchToolbar/RuntimeUsageControls/BranchToolbarEnvironmentPicker path so both git and non-git flows render the same provider data.apps/web/src/session-logic.ts (1)
49-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the redundant
DetailedWorkLogEntryalias
WorkLogEntryalready includesoutput,stdout,stderr,exitCode,durationMs, andpatch, so this local intersection can be simplified toWorkLogEntryunless thereadonlymodifiers are intentional.🤖 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 `@apps/web/src/session-logic.ts` around lines 49 - 72, Simplify the local work-log type by removing the redundant DetailedWorkLogEntry intersection and using WorkLogEntry directly in session-logic.ts. The relevant symbols are WorkLogEntry and the alias currently defined around it; if any readonly intent was meant for subagent-related fields, preserve that explicitly instead of duplicating the same output/stdout/stderr/exitCode/durationMs/patch properties.
🤖 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 `@apps/server/src/git/Layers/GitCore.ts`:
- Around line 458-460: The `listManagedWorktrees` parsing in `GitCore` only
matches an exact `prunable` line, so it misses `git worktree list --porcelain`
entries that include a reason suffix. Update the `prunable` handling in the
worktree entry parser to recognize lines that start with `prunable` and still
set `currentPrunable`, preserving the reason text for any existing parse flow.
Keep the fix localized around the `listManagedWorktrees` logic and the
`prunable` branch in `GitCore`.
In `@apps/server/src/provider/Layers/OpenCodeAdapter.test.ts`:
- Around line 1484-1531: The OpenCodeAdapter resume test only checks session
reuse, so it can still pass even if the resumed event changes or session.update
drops permission data. Strengthen the existing test in OpenCodeAdapter.test by
asserting the resumed turn emits the expected resumed event and that the
sessionUpdate payload includes the runtime-mode permissions for the resumed
session. Use the existing session and turn assertions as anchors, and add
focused expectations around the mocked runtime event and sessionUpdate call
details.
In `@apps/server/src/provider/Layers/ProviderHealth.ts`:
- Around line 2044-2055: `enrichStatuses` is refetching settings instead of
using the snapshot already loaded by `loadProviderStatuses`, which can cause
inconsistent enrichment behavior. Update `enrichStatuses` to accept the upstream
`settings` (or at least `enableProviderUpdateChecks`) as an argument and pass it
through from the `loadProviderStatuses` call site. Keep the existing
`Effect.forEach`/`enrichProviderStatusWithVersionAdvisory` flow unchanged, but
remove the internal `serverSettings.getSettings` read so the same settings
snapshot is reused end to end.
In `@apps/web/src/components/chat/MessagesTimeline.tsx`:
- Around line 1895-1896: The expand/collapse state in SimpleWorkEntryRow is
shared across every changed-file button, so multi-file entries toggle together
and hide the per-file diff-open behavior. Update the per-file rendering in
MessagesTimeline to scope expansion by changed file path (or move the patch
toggle out of the file buttons), and make the
FileChangeActivityDetails/ActivityEntryDetails flow target the clicked file
instead of one shared expanded boolean. Preserve the existing
onOpenTurnDiff(turnId, changedFilePath) behavior for entries with patch data so
a file click can still open that specific diff when applicable.
In `@apps/web/src/components/chat/MessagesTimelineActivityDetails.tsx`:
- Around line 40-46: The command-activity check is duplicated in
isCommandActivity and can drift from the existing isCommandEntry logic in
session-logic.ts. Move or export the shared predicate from toDerivedWorkLogEntry
in session-logic.ts, then import and reuse it in MessagesTimelineActivityDetails
so both places rely on the same command-classification rule.
- Around line 175-184: CommandOutputBlock truncates long command output without
telling the user, so add a visible truncation note when visibleLines is shorter
than lines. Update CommandOutputBlock in MessagesTimelineActivityDetails to
render a small “showing last 40 lines” style indicator alongside the
ActivityDetailBlock content whenever COMMAND_OUTPUT_TAIL_LINES is applied, using
the existing props.title, getRenderableCommandOutputLines, and visibleLines
logic.
- Around line 1-204: Extract the pure predicate/formatting helpers from
MessagesTimelineActivityDetails into a separate
MessagesTimelineActivityDetails.logic.ts module and add matching tests in a
.logic.test.ts file. Move functions like hasExpandableActivityDetails,
hasCommandActivityDetails, isCommandActivity, hasFileChangeActivityDetails,
isFileChangeActivity, formatWorkspaceRelativePath,
getRenderableCommandOutputLines, and related output-detection helpers so the
React component only renders JSX and imports the logic. Keep the component names
ActivityEntryDetails, CommandActivityDetails, and FileChangeActivityDetails as
the UI entry points, and cover the extracted logic with unit tests.
In `@apps/web/src/components/chat/MessagesTimelineMinimap.tsx`:
- Around line 65-76: Memoize the timeline item derivation in TimelineMinimap so
it does not rescan rows on every parent re-render. Update
MessagesTimelineMinimap.tsx by using useMemo around
deriveTimelineMinimapItems(rows), and consider wrapping TimelineMinimap with
memo if it still re-renders with unchanged props. Keep the existing
TIMELINE_MINIMAP_MIN_ITEMS guard and onJump props behavior unchanged.
In `@apps/web/src/components/ChatView.structure.test.ts`:
- Around line 6-11: The structural test for runtimeUsageControlsProps is too
dependent on the exact ChatView.tsx text layout and adjacent branchToolbarProps
ordering, so it can fail on harmless reformatting. Update
ChatView.structure.test.ts to avoid the brittle regex in
extractRuntimeUsageControlsPropsSource() and instead verify the wiring through a
render-based assertion or AST-based inspection using a stable symbol from
ChatView.tsx such as runtimeUsageControlsProps (and, if needed, the ChatView
component) so the test remains valid across formatting changes.
In `@apps/web/src/index.css`:
- Around line 823-828: Remove the deprecated word-break: break-word declaration
from the wrapped chat markdown code block styles in index.css, since
overflow-wrap: anywhere already covers the wrapping behavior. Update both
affected rule blocks that target .chat-markdown
.chat-markdown-codeblock--wrapped pre and .chat-markdown
.chat-markdown-codeblock--wrapped pre code so only the non-deprecated
white-space and overflow-wrap declarations remain.
- Around line 1143-1156: The wrap toggle button in the chat table scroll
container is positioned in a way that lets it move out of view during horizontal
scrolling. Update the `.chat-markdown .chat-markdown-table-wrap-button` behavior
so it stays visible inside `.chat-markdown-table-scroll` when the table
overflows horizontally, using the existing table scroll/wrap styles as the
anchor for the fix. Keep the hover/focus visibility logic for
`.chat-markdown-table-scroll:hover .chat-markdown-table-wrap-button` and
`.chat-markdown-table-scroll:focus-within .chat-markdown-table-wrap-button`
intact while changing the positioning approach so the button remains accessible
in the unwrapped state.
In `@apps/web/src/session-logic.ts`:
- Around line 737-739: The command classification logic is duplicated between
session-logic extraction and MessagesTimelineActivityDetails.tsx rendering,
which risks future drift. Extract the predicate from session-logic.ts into a
shared helper such as isCommandWorkLogEntry and use it both in the command-entry
handling path around extractCommandResult/extractChangedFiles/extractToolPatch
and in the existing isCommandActivity logic. Keep the helper colocated with the
current classification symbols so both modules import the same source of truth.
In `@docs/adr/README.md`:
- Line 28: Update the metadata table in the README to keep it current after
adding the new ADR row. Specifically, adjust the “Last reviewed” value in the
document’s metadata table so it reflects this change, and make sure the entry in
the ADR index that includes 0009 is accompanied by the updated review date. Use
the README’s metadata table and ADR list entry as the reference points when
making the change.
In `@packages/contracts/src/backend.ts`:
- Around line 30-37: The Backend schema currently allows invalid combinations
because Backend.kind and Backend.connection.kind are modeled as separate
independent fields. Update Backend in backend.ts so kind is derived from
connection (or refactor the schema into a single discriminated union) and make
sure the Backend and BackendConnection definitions enforce only valid local/wsl
combinations. Keep the fix centered on the Backend and BackendConnection symbols
so the type no longer represents domain-invalid states.
---
Outside diff comments:
In `@apps/web/src/components/BranchToolbar.tsx`:
- Around line 34-50: BranchToolbar is recomputing provider usage from
serverThread and ignores the already-computed provider data coming from
ChatView, which causes draft threads to show inconsistent “no provider” state.
Update BranchToolbarProps and BranchToolbar to accept and use the provider/usage
props that ChatView already prepares (activeProvider, providerRateLimits,
providerUsageLines, providerUsageIsLoading, providerUsageLearnMoreHref) instead
of deriving usage locally, and thread the renamed activeProvider through the
BranchToolbar/RuntimeUsageControls/BranchToolbarEnvironmentPicker path so both
git and non-git flows render the same provider data.
In `@apps/web/src/routes/_chat.settings.tsx`:
- Around line 540-543: The thread-linking and worktree path matching logic in
`deleteManagedWorktree`/`linkedThreadsFromSnapshot` is duplicated from
`worktreeCleanup.ts`, which risks the two paths diverging over time. Move the
canonical matching into `worktreeCleanup.ts` by exporting
`normalizeWorktreePath` and, ideally, a shared `linkThreadsToWorktreePath`
helper, then have both `classifyManagedWorktreeCleanupChoices` and the
fresh-snapshot lookup in `deleteManagedWorktree` call that shared logic instead
of `normalizeManagedWorktreePath` and the local filter.
In `@apps/web/src/session-logic.ts`:
- Around line 49-72: Simplify the local work-log type by removing the redundant
DetailedWorkLogEntry intersection and using WorkLogEntry directly in
session-logic.ts. The relevant symbols are WorkLogEntry and the alias currently
defined around it; if any readonly intent was meant for subagent-related fields,
preserve that explicitly instead of duplicating the same
output/stdout/stderr/exitCode/durationMs/patch properties.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 004b6917-a9b6-4219-b98b-97bd28619f3c
📒 Files selected for processing (59)
DESIGN.mdapps/server/src/backend/backendPathResolver.tsapps/server/src/backend/backendRegistry.test.tsapps/server/src/backend/backendRegistry.tsapps/server/src/git/Layers/GitCore.test.tsapps/server/src/git/Layers/GitCore.tsapps/server/src/git/Services/GitCore.tsapps/server/src/provider/Layers/OpenCodeAdapter.test.tsapps/server/src/provider/Layers/OpenCodeAdapter.tsapps/server/src/provider/Layers/ProviderHealth.tsapps/server/src/provider/copilotProviderSpike.test.tsapps/server/src/provider/copilotProviderSpike.tsapps/server/src/provider/providerMaintenance.test.tsapps/server/src/provider/providerMaintenance.tsapps/server/src/serverSettings.test.tsapps/server/src/wsRpc.tsapps/web/src/appSettings.test.tsapps/web/src/appSettings.tsapps/web/src/chat-scroll.test.tsapps/web/src/chat-scroll.tsapps/web/src/components/BranchToolbar.structure.test.tsapps/web/src/components/BranchToolbar.tsxapps/web/src/components/BranchToolbarEnvironmentPicker.tsxapps/web/src/components/ChatMarkdown.test.tsxapps/web/src/components/ChatMarkdown.tsxapps/web/src/components/ChatView.structure.test.tsapps/web/src/components/ChatView.tsxapps/web/src/components/GitActionsControl.tsxapps/web/src/components/ProviderUsageStatusChip.logic.test.tsapps/web/src/components/ProviderUsageStatusChip.logic.tsapps/web/src/components/ProviderUsageStatusChip.tsxapps/web/src/components/RuntimeUsageControls.tsxapps/web/src/components/VcsCommandCenterStatusPanel.logic.tsapps/web/src/components/VcsCommandCenterStatusPanel.test.tsxapps/web/src/components/VcsCommandCenterStatusPanel.tsxapps/web/src/components/chat/MessagesTimeline.browser.tsxapps/web/src/components/chat/MessagesTimeline.test.tsxapps/web/src/components/chat/MessagesTimeline.tsxapps/web/src/components/chat/MessagesTimelineActivityDetails.tsxapps/web/src/components/chat/MessagesTimelineMinimap.test.tsxapps/web/src/components/chat/MessagesTimelineMinimap.tsxapps/web/src/components/chat/composerProviderRegistry.test.tsxapps/web/src/components/chat/composerProviderRegistry.tsxapps/web/src/components/chat/composerTraits.tsapps/web/src/index.cssapps/web/src/routes/_chat.settings.tsxapps/web/src/session-logic.test.tsapps/web/src/session-logic.tsapps/web/src/worktreeCleanup.test.tsapps/web/src/worktreeCleanup.tsdocs/adr/0009-copilot-provider-entry-path.mddocs/adr/README.mdpackages/contracts/src/backend.test.tspackages/contracts/src/backend.tspackages/contracts/src/index.tspackages/contracts/src/server.test.tspackages/contracts/src/server.tspackages/contracts/src/settings.test.tspackages/contracts/src/settings.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/server/src/provider/Layers/ProviderHealth.ts (1)
2044-2055: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThread the same
settingssnapshot into capability resolution too.This still mixes snapshots:
loadProviderStatusesprobes providers with the outersettings, butgetProviderMaintenanceCapabilities(status.provider)re-reads settings during enrichment. A concurrent settings edit can therefore return provider statuses from one snapshot and version-advisory/update capability data from another in the same refresh. Pass the already-loadedsettingsthrough this path as well.Proposed fix
- const getProviderMaintenanceCapabilities = Effect.fn("getProviderMaintenanceCapabilities")( - function* (provider: ProviderKind) { - const settings = yield* serverSettings.getSettings; + const getProviderMaintenanceCapabilities = Effect.fn("getProviderMaintenanceCapabilities")( + function* (provider: ProviderKind, settings?: ServerSettings) { + const resolvedSettings = settings ?? (yield* serverSettings.getSettings); if (provider === "cursor") { return makeProviderMaintenanceCapabilities({ provider, packageName: null, - updateExecutable: getProviderBinaryPath(provider, settings) || "agent", + updateExecutable: getProviderBinaryPath(provider, resolvedSettings) || "agent", updateArgs: ["update"], updateLockKey: "cursor-agent", }); } - if (provider === "opencode" && isExternalOpenCodeRuntimeActive(settings)) { + if (provider === "opencode" && isExternalOpenCodeRuntimeActive(resolvedSettings)) { return makeProviderMaintenanceCapabilities({ provider, packageName: null, @@ - const binaryPath = getProviderBinaryPath(provider, settings) || null; + const binaryPath = getProviderBinaryPath(provider, resolvedSettings) || null; return yield* resolveProviderMaintenanceCapabilitiesEffect(definition, { binaryPath, env: process.env, platform: process.platform, }).pipe(Effect.provideService(FileSystem.FileSystem, fileSystem)); }, ); @@ - getProviderMaintenanceCapabilities(status.provider).pipe( + getProviderMaintenanceCapabilities(status.provider, settings).pipe(Also applies to: 2079-2109
🤖 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 `@apps/server/src/provider/Layers/ProviderHealth.ts` around lines 2044 - 2055, The enrichment path in enrichProviderStatuses is re-reading settings during capability lookup, which can mix snapshots from different refreshes. Update getProviderMaintenanceCapabilities usage in enrichProviderStatuses (and the related flow covered by loadProviderStatuses/enrichProviderStatusWithVersionAdvisory) to accept the already-loaded settings snapshot, so provider status enrichment and capability resolution both use the same ServerSettings instance. Ensure the settings object passed into enrichProviderStatuses is threaded through the capability resolution call instead of fetching settings again.
🤖 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 `@apps/web/src/session-logic.ts`:
- Around line 81-85: The command work-log detection in isCommandWorkLogEntry is
using nullish coalescing, which skips a valid raw command when command is an
empty string. Update the boolean check to fall back with logical OR on
workEntry.command and workEntry.rawCommand so empty command values still count
as command entries. This should preserve the derived entry handling in
toDerivedWorkLogEntry so command rows keep output, stdout, stderr, exitCode, and
durationMs.
---
Duplicate comments:
In `@apps/server/src/provider/Layers/ProviderHealth.ts`:
- Around line 2044-2055: The enrichment path in enrichProviderStatuses is
re-reading settings during capability lookup, which can mix snapshots from
different refreshes. Update getProviderMaintenanceCapabilities usage in
enrichProviderStatuses (and the related flow covered by
loadProviderStatuses/enrichProviderStatusWithVersionAdvisory) to accept the
already-loaded settings snapshot, so provider status enrichment and capability
resolution both use the same ServerSettings instance. Ensure the settings object
passed into enrichProviderStatuses is threaded through the capability resolution
call instead of fetching settings again.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dc619534-6ef7-4016-b9d8-739c7f8026fd
📒 Files selected for processing (18)
apps/server/src/backend/backendPathResolver.tsapps/server/src/backend/backendRegistry.test.tsapps/server/src/git/Layers/GitCore.test.tsapps/server/src/git/Layers/GitCore.tsapps/server/src/provider/Layers/OpenCodeAdapter.test.tsapps/server/src/provider/Layers/ProviderHealth.tsapps/web/src/components/ChatView.structure.test.tsapps/web/src/components/chat/MessagesTimeline.test.tsxapps/web/src/components/chat/MessagesTimeline.tsxapps/web/src/components/chat/MessagesTimelineActivityDetails.logic.test.tsapps/web/src/components/chat/MessagesTimelineActivityDetails.logic.tsapps/web/src/components/chat/MessagesTimelineActivityDetails.tsxapps/web/src/components/chat/MessagesTimelineMinimap.tsxapps/web/src/index.cssapps/web/src/session-logic.tsdocs/adr/README.mdpackages/contracts/src/backend.test.tspackages/contracts/src/backend.ts
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/server/src/provider/Layers/OpenCodeAdapter.ts (1)
3928-3934: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle
session.updatenot-found as a stale cursor too.Line 3929 can still race: the probed session may be deleted after
session.getsucceeds but before permissions are updated, causingstartSessionto fail instead of creating a fresh session. Treat only update-time 404/not-found as stale and fall through tosession.create; keep other update failures surfaced. Add a focused regression for this path. As per coding guidelines, provider adapters must preserve provider error/state paths and add targeted regression tests rather than suppressing errors broadly.Suggested direction
if (reusableSessionId) { - yield* runOpenCodeSdk("session.update", () => - client.session.update({ - sessionID: reusableSessionId, - permission: buildOpenCodePermissionRules(input.runtimeMode), - }), - ); - return { - sessionScope, - server, - client, - openCodeSessionId: reusableSessionId, - created: false, - }; + const updateExit = yield* Effect.exit( + runOpenCodeSdk("session.update", () => + client.session.update({ + sessionID: reusableSessionId, + permission: buildOpenCodePermissionRules(input.runtimeMode), + }), + ), + ); + if (Exit.isSuccess(updateExit)) { + return { + sessionScope, + server, + client, + openCodeSessionId: reusableSessionId, + created: false, + }; + } + if (!isOpenCodeSessionNotFound(Cause.squash(updateExit.cause))) { + return yield* Effect.failCause(updateExit.cause); + } }🤖 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 `@apps/server/src/provider/Layers/OpenCodeAdapter.ts` around lines 3928 - 3934, In startSession inside OpenCodeAdapter, handle a 404/not-found returned by client.session.update as a stale reusable session the same way session.get is handled, so the code falls through to session.create instead of failing. Keep non-not-found update errors surfaced by limiting the fallback to the update-time not-found case, using the existing runOpenCodeSdk("session.update", ...) and reusableSessionId path. Add a focused regression test for the race where session.get succeeds but session.update returns not-found, verifying a new session is created.Source: Coding guidelines
apps/web/src/components/ChatView.tsx (1)
854-854: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winUse the derived all-threads selector here
store.threadscan lag behind hot-path thread updates (syncServerThreadDetailHotPathintentionally leaves the array untouched), souseProviderUsageSummarymay show stale rate-limit data. Switch this to the samecreateAllThreadsSelector()pattern used elsewhere in the app.🤖 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 `@apps/web/src/components/ChatView.tsx` at line 854, The thread list in ChatView is reading the raw store.threads array, which can be stale after hot-path updates and lead to outdated usage summaries. Update the selector used in ChatView to the derived all-threads selector by wiring in createAllThreadsSelector(), matching the existing pattern used elsewhere and ensuring useProviderUsageSummary sees the latest thread data.Source: Path instructions
🤖 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 `@apps/server/src/wsRpc.ts`:
- Around line 921-923: The project-level worktree aggregation is swallowing
failures from git.listManagedWorktrees via Effect.orElseSucceed, so add logging
before falling back to noManagedWorktrees. Update the managed-worktree
collection flow in wsRpc.ts to use the existing logger/process logger around the
listManagedWorktrees call, emit the project/workspaceRoot and error details when
it fails, and then return noManagedWorktrees so the endpoint still completes.
- Around line 1741-1744: The owner-guard handling in providerBootstrapRuntime
and providerRepairRuntime still uses the older ownerOnly wrapper instead of the
refactored withCurrentSession(requireOwnerWsRpcAccess, ...) pattern used by the
rest of the WS dispatch table. Update these two handlers to match the new
session-based guard approach, using the same style as the nearby provider
methods and preserving the existing rpcEffect error messages. This keeps the
auth-sensitive logic consistent and removes the parallel owner-check
implementation from wsRpc.
- Around line 161-176: The legacy websocket owner bypass in
resolveLocalLegacyWsAuthSession is using a timing-sensitive equality check and
should not be allowed for non-loopback access. Replace the direct authToken vs
legacyToken comparison with a constant-time secret comparison, and ensure this
shortcut only succeeds for loopback requests in the /ws flow. Also verify the
legacy path still goes through the normal session lifecycle so
sessions.markConnected and sessions.markDisconnected are not skipped.
In `@apps/web/src/components/chat/MessagesTimeline.browser.tsx`:
- Around line 23-50: The browser test is bypassing the production integration
path by using ActivityDetailsHarness instead of the real row component that owns
the expand/collapse wiring. Update the test to render SimpleWorkEntryRow from
MessagesTimeline.tsx, or a thin wrapper around it, so it exercises the actual
threading of workEntry and workspaceRoot and verifies the expand button behavior
in the real component tree.
---
Outside diff comments:
In `@apps/server/src/provider/Layers/OpenCodeAdapter.ts`:
- Around line 3928-3934: In startSession inside OpenCodeAdapter, handle a
404/not-found returned by client.session.update as a stale reusable session the
same way session.get is handled, so the code falls through to session.create
instead of failing. Keep non-not-found update errors surfaced by limiting the
fallback to the update-time not-found case, using the existing
runOpenCodeSdk("session.update", ...) and reusableSessionId path. Add a focused
regression test for the race where session.get succeeds but session.update
returns not-found, verifying a new session is created.
In `@apps/web/src/components/ChatView.tsx`:
- Line 854: The thread list in ChatView is reading the raw store.threads array,
which can be stale after hot-path updates and lead to outdated usage summaries.
Update the selector used in ChatView to the derived all-threads selector by
wiring in createAllThreadsSelector(), matching the existing pattern used
elsewhere and ensuring useProviderUsageSummary sees the latest thread data.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b745d144-a491-4355-93b5-ae6909a3239c
📒 Files selected for processing (12)
DESIGN.mdapps/server/src/provider/Layers/OpenCodeAdapter.test.tsapps/server/src/provider/Layers/OpenCodeAdapter.tsapps/server/src/wsRpc.tsapps/web/src/components/ChatView.browser.tsxapps/web/src/components/ChatView.tsxapps/web/src/components/chat/ChatTranscriptPane.browser.tsxapps/web/src/components/chat/MessagesTimeline.browser.tsxdocs/adr/0011-copilot-provider-entry-path.mddocs/adr/README.mdpackages/contracts/src/index.tspackages/contracts/src/settings.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/server/src/provider/Layers/OpenCodeAdapter.ts (1)
3928-3934: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle
session.updatenot-found as a stale cursor too.Line 3929 can still race: the probed session may be deleted after
session.getsucceeds but before permissions are updated, causingstartSessionto fail instead of creating a fresh session. Treat only update-time 404/not-found as stale and fall through tosession.create; keep other update failures surfaced. Add a focused regression for this path. As per coding guidelines, provider adapters must preserve provider error/state paths and add targeted regression tests rather than suppressing errors broadly.Suggested direction
if (reusableSessionId) { - yield* runOpenCodeSdk("session.update", () => - client.session.update({ - sessionID: reusableSessionId, - permission: buildOpenCodePermissionRules(input.runtimeMode), - }), - ); - return { - sessionScope, - server, - client, - openCodeSessionId: reusableSessionId, - created: false, - }; + const updateExit = yield* Effect.exit( + runOpenCodeSdk("session.update", () => + client.session.update({ + sessionID: reusableSessionId, + permission: buildOpenCodePermissionRules(input.runtimeMode), + }), + ), + ); + if (Exit.isSuccess(updateExit)) { + return { + sessionScope, + server, + client, + openCodeSessionId: reusableSessionId, + created: false, + }; + } + if (!isOpenCodeSessionNotFound(Cause.squash(updateExit.cause))) { + return yield* Effect.failCause(updateExit.cause); + } }🤖 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 `@apps/server/src/provider/Layers/OpenCodeAdapter.ts` around lines 3928 - 3934, In startSession inside OpenCodeAdapter, handle a 404/not-found returned by client.session.update as a stale reusable session the same way session.get is handled, so the code falls through to session.create instead of failing. Keep non-not-found update errors surfaced by limiting the fallback to the update-time not-found case, using the existing runOpenCodeSdk("session.update", ...) and reusableSessionId path. Add a focused regression test for the race where session.get succeeds but session.update returns not-found, verifying a new session is created.Source: Coding guidelines
apps/web/src/components/ChatView.tsx (1)
854-854: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winUse the derived all-threads selector here
store.threadscan lag behind hot-path thread updates (syncServerThreadDetailHotPathintentionally leaves the array untouched), souseProviderUsageSummarymay show stale rate-limit data. Switch this to the samecreateAllThreadsSelector()pattern used elsewhere in the app.🤖 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 `@apps/web/src/components/ChatView.tsx` at line 854, The thread list in ChatView is reading the raw store.threads array, which can be stale after hot-path updates and lead to outdated usage summaries. Update the selector used in ChatView to the derived all-threads selector by wiring in createAllThreadsSelector(), matching the existing pattern used elsewhere and ensuring useProviderUsageSummary sees the latest thread data.Source: Path instructions
🤖 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 `@apps/server/src/wsRpc.ts`:
- Around line 921-923: The project-level worktree aggregation is swallowing
failures from git.listManagedWorktrees via Effect.orElseSucceed, so add logging
before falling back to noManagedWorktrees. Update the managed-worktree
collection flow in wsRpc.ts to use the existing logger/process logger around the
listManagedWorktrees call, emit the project/workspaceRoot and error details when
it fails, and then return noManagedWorktrees so the endpoint still completes.
- Around line 1741-1744: The owner-guard handling in providerBootstrapRuntime
and providerRepairRuntime still uses the older ownerOnly wrapper instead of the
refactored withCurrentSession(requireOwnerWsRpcAccess, ...) pattern used by the
rest of the WS dispatch table. Update these two handlers to match the new
session-based guard approach, using the same style as the nearby provider
methods and preserving the existing rpcEffect error messages. This keeps the
auth-sensitive logic consistent and removes the parallel owner-check
implementation from wsRpc.
- Around line 161-176: The legacy websocket owner bypass in
resolveLocalLegacyWsAuthSession is using a timing-sensitive equality check and
should not be allowed for non-loopback access. Replace the direct authToken vs
legacyToken comparison with a constant-time secret comparison, and ensure this
shortcut only succeeds for loopback requests in the /ws flow. Also verify the
legacy path still goes through the normal session lifecycle so
sessions.markConnected and sessions.markDisconnected are not skipped.
In `@apps/web/src/components/chat/MessagesTimeline.browser.tsx`:
- Around line 23-50: The browser test is bypassing the production integration
path by using ActivityDetailsHarness instead of the real row component that owns
the expand/collapse wiring. Update the test to render SimpleWorkEntryRow from
MessagesTimeline.tsx, or a thin wrapper around it, so it exercises the actual
threading of workEntry and workspaceRoot and verifies the expand button behavior
in the real component tree.
---
Outside diff comments:
In `@apps/server/src/provider/Layers/OpenCodeAdapter.ts`:
- Around line 3928-3934: In startSession inside OpenCodeAdapter, handle a
404/not-found returned by client.session.update as a stale reusable session the
same way session.get is handled, so the code falls through to session.create
instead of failing. Keep non-not-found update errors surfaced by limiting the
fallback to the update-time not-found case, using the existing
runOpenCodeSdk("session.update", ...) and reusableSessionId path. Add a focused
regression test for the race where session.get succeeds but session.update
returns not-found, verifying a new session is created.
In `@apps/web/src/components/ChatView.tsx`:
- Line 854: The thread list in ChatView is reading the raw store.threads array,
which can be stale after hot-path updates and lead to outdated usage summaries.
Update the selector used in ChatView to the derived all-threads selector by
wiring in createAllThreadsSelector(), matching the existing pattern used
elsewhere and ensuring useProviderUsageSummary sees the latest thread data.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b745d144-a491-4355-93b5-ae6909a3239c
📒 Files selected for processing (12)
DESIGN.mdapps/server/src/provider/Layers/OpenCodeAdapter.test.tsapps/server/src/provider/Layers/OpenCodeAdapter.tsapps/server/src/wsRpc.tsapps/web/src/components/ChatView.browser.tsxapps/web/src/components/ChatView.tsxapps/web/src/components/chat/ChatTranscriptPane.browser.tsxapps/web/src/components/chat/MessagesTimeline.browser.tsxdocs/adr/0011-copilot-provider-entry-path.mddocs/adr/README.mdpackages/contracts/src/index.tspackages/contracts/src/settings.ts
🛑 Comments failed to post (4)
apps/server/src/wsRpc.ts (3)
161-176: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Should websocket bearer token comparisons use crypto.timingSafeEqual in Node.js?💡 Result:
Yes, you should use crypto.timingSafeEqual in Node.js when comparing sensitive information like bearer tokens [1][2][3]. Using standard equality operators (=== or!==) for secrets is vulnerable to timing side-channel attacks [1][2][4]. Because these operators "short-circuit"—they return false as soon as the first mismatched character is found—the time taken to perform the comparison correlates with the number of correct characters in the input [1][2][4]. By measuring response latency over many requests, an attacker can incrementally guess the token character by character [1][5][4]. To implement this correctly: 1. Convert both the received token and the expected (server-side) secret to Buffers or TypedArrays, as crypto.timingSafeEqual only accepts these types [6][7][2]. 2. Use crypto.timingSafeEqual directly [8]. It is designed to perform a constant-time comparison [1][2]. Note on Implementation: Some older implementation patterns suggested adding an explicit length check (e.g.,
if (a.length !== b.length) return false;) before calling crypto.timingSafeEqual to prevent errors [7][2][3]. However, this can inadvertently leak information about the length of the secret [8]. It is more secure to ensure the comparison is handled in constant time regardless of length [8]. According to common usage patterns in Node.js, simply passing the buffers to crypto.timingSafeEqual is the preferred approach, provided the inputs are correctly normalized to Buffers [8]. Ensure that both inputs are either the same length or padded correctly if you need to strictly maintain length obfuscation, though crypto.timingSafeEqual itself is the standard tool for mitigating the timing oracle vulnerability [2][8].Citations:
- 1: moeru-ai/airi#1445
- 2: https://www.authgear.com/post/nodejs-security-best-practices/
- 3: https://eslint.interlace.tools/docs/security/plugin-node-security/rules/no-timing-unsafe-compare
- 4: superglue-ai/superglue#670
- 5: GHSA-rfxf-mf63-cpqv
- 6: https://nodejs.org/api/crypto.html
- 7: https://til.simonwillison.net/node/constant-time-compare-strings
- 8: samanhappy/mcphub#687
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the relevant file and locate websocket auth/session plumbing. ast-grep outline apps/server/src/wsRpc.ts --view expanded || true printf '\n---\n' rg -n "resolveLocalLegacyWsAuthSession|authenticateWebSocketUpgrade|markConnected|markDisconnected|token query|legacyToken|authToken|wsRpc" apps/server/src -SRepository: Jay1/jcode
Length of output: 8236
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read the relevant region of wsRpc.ts with line numbers. wc -l apps/server/src/wsRpc.ts printf '\n---\n' sed -n '1,260p' apps/server/src/wsRpc.ts | cat -nRepository: Jay1/jcode
Length of output: 10943
🏁 Script executed:
#!/bin/bash set -euo pipefail # Find where the websocket upgrade is exposed and how the server is bound. rg -n "wsRpc|WebSocket|upgrade|listen\\(|127\\.0\\.0\\.1|localhost|0\\.0\\.0\\.0|::" apps/server/src -SRepository: Jay1/jcode
Length of output: 32429
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the websocket upgrade handler and nearby helpers. sed -n '1800,1855p' apps/server/src/wsRpc.ts | cat -n printf '\n---\n' # Inspect the startup-access helpers that gate loopback/wildcard hosts and remote addresses. sed -n '1,140p' apps/server/src/startupAccess.ts | cat -n printf '\n---\n' # Inspect the server binding / host-selection logic. sed -n '210,260p' apps/server/src/main.ts | cat -nRepository: Jay1/jcode
Length of output: 8006
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n "legacy websocket|local legacy|legacyToken|resolveLocalLegacyWsAuthSession|bearer-session-token|websocket connections|wsToken|token=" apps/server/src apps/server -SRepository: Jay1/jcode
Length of output: 16432
Guard the legacy WS owner bypass. This plain
===comparison on a bearer secret is timing-sensitive, and the/wsroute is reachable whenever the server binds off-loopback. Use a constant-time compare here, and keep the legacy shortcut constrained to loopback-only access; it also skipssessions.markConnected/sessions.markDisconnected.🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').(detect-child-process-typescript)
🤖 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 `@apps/server/src/wsRpc.ts` around lines 161 - 176, The legacy websocket owner bypass in resolveLocalLegacyWsAuthSession is using a timing-sensitive equality check and should not be allowed for non-loopback access. Replace the direct authToken vs legacyToken comparison with a constant-time secret comparison, and ensure this shortcut only succeeds for loopback requests in the /ws flow. Also verify the legacy path still goes through the normal session lifecycle so sessions.markConnected and sessions.markDisconnected are not skipped.
921-923: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Log project-level
listManagedWorktreesfailures instead of silently dropping them.
Effect.orElseSucceed(() => noManagedWorktrees)swallows any failure fromgit.listManagedWorktrees(project.workspaceRoot)with no logging. Since this is the endpoint backing the worktree cleanup-safety UI, a project whose listing fails (e.g. transient git error, permission issue, deleted directory) will simply vanish from the aggregated result with no trace in logs and no signal to the operator — making it hard to diagnose why a worktree that should appear for cleanup review is missing.🪵 Suggested fix: log the failure before falling back
const worktreeGroups = yield* Effect.forEach( snapshot.projects, (project) => git .listManagedWorktrees(project.workspaceRoot) - .pipe(Effect.orElseSucceed(() => noManagedWorktrees)), + .pipe( + Effect.tapError((cause) => + Effect.logWarning("Failed to list managed worktrees for project", { + workspaceRoot: project.workspaceRoot, + cause, + }), + ), + Effect.orElseSucceed(() => noManagedWorktrees), + ), { concurrency: 4 }, );Also applies to: 1333-1351
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').(detect-child-process-typescript)
🤖 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 `@apps/server/src/wsRpc.ts` around lines 921 - 923, The project-level worktree aggregation is swallowing failures from git.listManagedWorktrees via Effect.orElseSucceed, so add logging before falling back to noManagedWorktrees. Update the managed-worktree collection flow in wsRpc.ts to use the existing logger/process logger around the listManagedWorktrees call, emit the project/workspaceRoot and error details when it fails, and then return noManagedWorktrees so the endpoint still completes.
1741-1744: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Inconsistent owner-guard pattern left over from the refactor.
providerBootstrapRuntime/providerRepairRuntimestill use the olderownerOnly/requireOwnerSessionhelper (defined pre-existing at Lines 898-920), while every other handler in this diff was migrated towithCurrentSession(requireOwnerWsRpcAccess, ...)(e.g. Line 1319, Line 1734). Having two parallel "require owner" implementations in the same auth-sensitive dispatch table is a duplication/consistency risk — a future change to one (e.g. adding audit logging or a new role check) could easily be missed in the other.♻️ Suggested consolidation
- [WS_METHODS.providerBootstrapRuntime]: (input) => - rpcEffect(ownerOnly(bootstrapOpenCodeRuntime(input)), "Failed to bootstrap runtime"), - [WS_METHODS.providerRepairRuntime]: (input) => - rpcEffect(ownerOnly(repairOpenCodeRuntime(input)), "Failed to repair runtime"), + [WS_METHODS.providerBootstrapRuntime]: (input) => + withCurrentSession( + requireOwnerWsRpcAccess, + rpcEffect(bootstrapOpenCodeRuntime(input), "Failed to bootstrap runtime"), + ), + [WS_METHODS.providerRepairRuntime]: (input) => + withCurrentSession( + requireOwnerWsRpcAccess, + rpcEffect(repairOpenCodeRuntime(input), "Failed to repair runtime"), + ),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.[WS_METHODS.providerBootstrapRuntime]: (input) => withCurrentSession( requireOwnerWsRpcAccess, rpcEffect(bootstrapOpenCodeRuntime(input), "Failed to bootstrap runtime"), ), [WS_METHODS.providerRepairRuntime]: (input) => withCurrentSession( requireOwnerWsRpcAccess, rpcEffect(repairOpenCodeRuntime(input), "Failed to repair runtime"), ),🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').(detect-child-process-typescript)
🤖 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 `@apps/server/src/wsRpc.ts` around lines 1741 - 1744, The owner-guard handling in providerBootstrapRuntime and providerRepairRuntime still uses the older ownerOnly wrapper instead of the refactored withCurrentSession(requireOwnerWsRpcAccess, ...) pattern used by the rest of the WS dispatch table. Update these two handlers to match the new session-based guard approach, using the same style as the nearby provider methods and preserving the existing rpcEffect error messages. This keeps the auth-sensitive logic consistent and removes the parallel owner-check implementation from wsRpc.apps/web/src/components/chat/MessagesTimeline.browser.tsx (1)
23-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Test harness bypasses the real integration point.
ActivityDetailsHarnessreimplements expand/collapse state and directly wireshasExpandableActivityDetails/ActivityEntryDetailsitself, instead of exercising the actual production component (SimpleWorkEntryRowinMessagesTimeline.tsx) that owns this wiring per the stack outline. This test now only verifies the logic functions compose correctly in isolation, not that the real timeline row correctly threadsworkEntry/workspaceRootand renders the expand button — regressions in the actual wiring could slip through undetected.Consider rendering the real row component (or a thin wrapper around it) so this browser test continues to guard the production integration path.
[recommended_refactor]
[test_coverage]🤖 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 `@apps/web/src/components/chat/MessagesTimeline.browser.tsx` around lines 23 - 50, The browser test is bypassing the production integration path by using ActivityDetailsHarness instead of the real row component that owns the expand/collapse wiring. Update the test to render SimpleWorkEntryRow from MessagesTimeline.tsx, or a thin wrapper around it, so it exercises the actual threading of workEntry and workspaceRoot and verifies the expand button behavior in the real component tree.
1 participant
Summary
Verification
Notes
Summary by CodeRabbit