Codex/skills appserver handoff#1766
Conversation
|
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 |
github-actions
bot
added
size:XXL
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5e5f8d2. Configure here.
| [17, "ProjectionThreadsArchivedAt", Migration0017], | ||
| [18, "ProjectionThreadsArchivedAtIndex", Migration0018], | ||
| [19, "ProjectionSnapshotLookupIndexes", Migration0019], | ||
| [17, "ThreadHandoffMetadata", Migration0017], |
There was a problem hiding this comment.
New installs missing archived_at column from replaced migration
High Severity
Migration 017_ProjectionThreadsArchivedAt (which adds the archived_at column to projection_threads) was replaced by 017_ThreadHandoffMetadata, which only adds handoff_json and source. However, SQL queries in ProjectionThreads.ts and ProjectionSnapshotQuery.ts still SELECT archived_at AS "archivedAt", and the ProjectionThread schema still declares the archivedAt field. On a fresh installation the archived_at column will never be created, causing all thread queries to fail at runtime.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 5e5f8d2. Configure here.
| return Effect.void; | ||
| } | ||
| return revalidateAndEmitSafely; | ||
| }).pipe(Effect.ignoreCause({ log: true }), Effect.forkIn(watcherScope), Effect.asVoid); |
There was a problem hiding this comment.
Keybindings watcher debounce removed despite documented necessity
Low Severity
The file-system watcher for keybinding config changes previously debounced events by 100ms with an explicit comment explaining that editors emit multiple events per save (truncate, write, rename) and fs.watch can fire before content is flushed. The debounce and the filter are now replaced by inline processing of every event, which can cause the config to be re-read while partially written, triggering transient parse failures or stale reads during saves.
Reviewed by Cursor Bugbot for commit 5e5f8d2. Configure here.
| const DEFAULT_RESOLVED_KEYBINDINGS = compileResolvedKeybindingsConfig(DEFAULT_KEYBINDINGS); | ||
|
|
||
| const RawKeybindingsEntries = fromLenientJson(Schema.Array(Schema.Unknown)); | ||
| const RawKeybindingsEntries = Schema.fromJsonString(Schema.Array(Schema.Unknown)); |
There was a problem hiding this comment.
Strict JSON parsing replaces lenient parser for user config
Medium Severity
RawKeybindingsEntries was changed from fromLenientJson(...) to Schema.fromJsonString(...). The lenient parser likely tolerated common user-edited JSON variations (trailing commas, comments, etc.). The strict JSON.parse-based decoder will now reject previously valid user keybinding config files, causing startup or reload failures for users with non-standard JSON in their config.
Reviewed by Cursor Bugbot for commit 5e5f8d2. Configure here.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| threadId: "thread_1", | ||
| }); | ||
|
|
||
| expect(resolveContextForDiscovery).toHaveBeenCalledWith("thread_1"); |
There was a problem hiding this comment.
🟡 Medium src/codexAppServerManager.test.ts:696
The test assertion on line 696 checks resolveContextForDiscovery was called with only "thread_1", but the actual listSkills method passes two arguments: input.threadId and cwd. Vitest's toHaveBeenCalledWith requires exact argument matching, so this test will fail. The assertion should include both arguments: "thread_1", "/repo".
- expect(resolveContextForDiscovery).toHaveBeenCalledWith("thread_1");
+ expect(resolveContextForDiscovery).toHaveBeenCalledWith("thread_1", "/repo");🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/codexAppServerManager.test.ts around line 696:
The test assertion on line 696 checks `resolveContextForDiscovery` was called with only `"thread_1"`, but the actual `listSkills` method passes two arguments: `input.threadId` and `cwd`. Vitest's `toHaveBeenCalledWith` requires exact argument matching, so this test will fail. The assertion should include both arguments: `"thread_1", "/repo"`.
Evidence trail:
apps/server/src/codexAppServerManager.test.ts line 696 (REVIEWED_COMMIT): `expect(resolveContextForDiscovery).toHaveBeenCalledWith("thread_1");` - assertion with only one argument.
apps/server/src/codexAppServerManager.ts line 1082 (REVIEWED_COMMIT): `const context = await this.resolveContextForDiscovery(input.threadId, cwd);` - implementation passes two arguments.
Vitest documentation confirms toHaveBeenCalledWith requires exact argument matching (https://github.com/vitest-dev/vitest/issues/870).
| @@ -80,12 +125,7 @@ describe("CheckpointDiffQueryLive", () => { | |||
| Layer.provideMerge(Layer.succeed(CheckpointStore, checkpointStore)), | |||
| Layer.provideMerge( | |||
There was a problem hiding this comment.
🟢 Low Layers/CheckpointDiffQuery.test.ts:126
The test mocks ProjectionSnapshotQuery with only getSnapshot, but CheckpointDiffQueryLive calls getThreadCheckpointContext. At runtime this throws because the method is undefined on the mock. Add getThreadCheckpointContext to the mock and have it return the thread context from getSnapshot.
🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/checkpointing/Layers/CheckpointDiffQuery.test.ts around line 126:
The test mocks `ProjectionSnapshotQuery` with only `getSnapshot`, but `CheckpointDiffQueryLive` calls `getThreadCheckpointContext`. At runtime this throws because the method is undefined on the mock. Add `getThreadCheckpointContext` to the mock and have it return the thread context from `getSnapshot`.
Evidence trail:
- apps/server/src/checkpointing/Layers/CheckpointDiffQuery.test.ts lines 126-129: Mock only provides `getSnapshot`
- apps/server/src/checkpointing/Layers/CheckpointDiffQuery.test.ts lines 140-143: Test calls getTurnDiff with fromTurnCount: 0, toTurnCount: 1
- apps/server/src/checkpointing/Layers/CheckpointDiffQuery.ts lines 27-28: Early return only when fromTurnCount === toTurnCount
- apps/server/src/checkpointing/Layers/CheckpointDiffQuery.ts line 44: Calls `projectionSnapshotQuery.getThreadCheckpointContext()`
- apps/server/src/orchestration/Services/ProjectionSnapshotQuery.ts lines 63-66: `getThreadCheckpointContext` is part of the interface
| function formatRelativeTime(iso: string): string { | ||
| const diff = Date.now() - new Date(iso).getTime(); |
There was a problem hiding this comment.
🟢 Low components/Sidebar.tsx:168
formatRelativeTime returns "NaNd" when iso is malformed because new Date(iso).getTime() produces NaN and all comparisons with NaN are false. Consider validating the parsed date (e.g., checking Number.isFinite(diff)) before computing the relative time, or returning a fallback value like "now" or null for invalid inputs.
| function formatRelativeTime(iso: string): string { | |
| const diff = Date.now() - new Date(iso).getTime(); | |
| function formatRelativeTime(iso: string): string { | |
| + const parsed = new Date(iso).getTime(); | |
| + if (!Number.isFinite(parsed)) { | |
| + return "now"; | |
| + } | |
| + const diff = Date.now() - parsed; |
🤖 Copy this AI Prompt to have your agent fix this:
In file apps/web/src/components/Sidebar.tsx around lines 168-169:
`formatRelativeTime` returns `"NaNd"` when `iso` is malformed because `new Date(iso).getTime()` produces `NaN` and all comparisons with `NaN` are false. Consider validating the parsed date (e.g., checking `Number.isFinite(diff)`) before computing the relative time, or returning a fallback value like `"now"` or `null` for invalid inputs.
Evidence trail:
apps/web/src/components/Sidebar.tsx lines 168-176 at REVIEWED_COMMIT shows the `formatRelativeTime` function with no input validation. JavaScript specification confirms that NaN comparisons always return false and `new Date(invalidString).getTime()` returns NaN.
| import * as SqlClient from "effect/unstable/sql/SqlClient"; | ||
| import * as Effect from "effect/Effect"; | ||
|
|
||
| export default Effect.gen(function* () { |
There was a problem hiding this comment.
🟢 Low Migrations/017_ThreadHandoffMetadata.ts:4
The migration uses Effect.catchTag("SqlError", () => Effect.void) to handle all SQL errors, so any failure—including missing table, database corruption, or permission issues—returns success silently. This leaves the schema in an inconsistent state because the migration reports as applied even when the ALTER TABLE failed. Consider catching only the specific "column already exists" error (e.g., by checking error message or code) and re-throwing other errors so genuine failures propagate.
🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/persistence/Migrations/017_ThreadHandoffMetadata.ts around line 4:
The migration uses `Effect.catchTag("SqlError", () => Effect.void)` to handle all SQL errors, so any failure—including missing table, database corruption, or permission issues—returns success silently. This leaves the schema in an inconsistent state because the migration reports as applied even when the ALTER TABLE failed. Consider catching only the specific "column already exists" error (e.g., by checking error message or code) and re-throwing other errors so genuine failures propagate.
Evidence trail:
apps/server/src/persistence/Migrations/017_ThreadHandoffMetadata.ts lines 1-15 at REVIEWED_COMMIT - shows `Effect.catchTag("SqlError", () => Effect.void)` catching all SQL errors for both ALTER TABLE statements
| const DEFAULT_RESOLVED_KEYBINDINGS = compileResolvedKeybindingsConfig(DEFAULT_KEYBINDINGS); | ||
|
|
||
| const RawKeybindingsEntries = fromLenientJson(Schema.Array(Schema.Unknown)); | ||
| const RawKeybindingsEntries = Schema.fromJsonString(Schema.Array(Schema.Unknown)); |
There was a problem hiding this comment.
🟢 Low src/keybindings.ts:421
Switching from fromLenientJson to Schema.fromJsonString changes the JSON parser from lenient to strict. User configuration files with trailing commas, comments, or other non-strict JSON syntax that previously parsed successfully will now fail, causing loadRuntimeCustomKeybindingsConfig and loadWritableCustomKeybindingsConfig to reject configs on upgrade. If strict parsing is intentional, consider documenting this breaking change and providing a migration path for users.
🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/keybindings.ts around line 421:
Switching from `fromLenientJson` to `Schema.fromJsonString` changes the JSON parser from lenient to strict. User configuration files with trailing commas, comments, or other non-strict JSON syntax that previously parsed successfully will now fail, causing `loadRuntimeCustomKeybindingsConfig` and `loadWritableCustomKeybindingsConfig` to reject configs on upgrade. If strict parsing is intentional, consider documenting this breaking change and providing a migration path for users.
Evidence trail:
1. apps/server/src/keybindings.ts:421 (REVIEWED_COMMIT) - shows change from `fromLenientJson` to `Schema.fromJsonString` 2. packages/shared/src/schemaJson.ts (MERGE_BASE commit cf2c628bc) lines 51-96 - shows `fromLenientJson` strips comments and trailing commas via regex before JSON.parse 3. apps/server/src/keybindings.ts:586 and :634 - shows RawKeybindingsEntries is used to parse user config files in loadWritableCustomKeybindingsConfig and loadRuntimeCustomKeybindingsConfig 4. apps/server/src/keybindings.ts:567-570 - shows readRawConfig reads user's keybindings config file from disk
There was a problem hiding this comment.
🟢 Low
t3code/apps/server/src/keybindings.ts
Line 677 in 5e5f8d2
Removing the Effect.ensuring cleanup from writeConfigAtomically causes temp files to be orphaned on disk if any step in the write pipeline fails. When fs.rename fails after fs.writeFileString succeeds, the temp file at ${keybindingsConfigPath}.${process.pid}.${Date.now()}.tmp persists indefinitely. Consider restoring the Effect.ensuring cleanup to remove the temp file on failure.
🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/keybindings.ts around line 677:
Removing the `Effect.ensuring` cleanup from `writeConfigAtomically` causes temp files to be orphaned on disk if any step in the write pipeline fails. When `fs.rename` fails after `fs.writeFileString` succeeds, the temp file at `${keybindingsConfigPath}.${process.pid}.${Date.now()}.tmp` persists indefinitely. Consider restoring the `Effect.ensuring` cleanup to remove the temp file on failure.
Evidence trail:
git_diff base=MERGE_BASE head=REVIEWED_COMMIT path=apps/server/src/keybindings.ts shows removal of line 685: `Effect.ensuring(fs.remove(tempPath, { force: true }).pipe(Effect.ignore({ log: true })))`. Current code at apps/server/src/keybindings.ts lines 677-692 shows `writeConfigAtomically` function with no cleanup mechanism for the temp file. The temp file is created with pattern `${keybindingsConfigPath}.${process.pid}.${Date.now()}.tmp` at line 679 and written at line 684 via `fs.writeFileString(tempPath, encoded)`, then renamed at line 685 via `fs.rename(tempPath, keybindingsConfigPath)`. If rename fails after write succeeds, no cleanup occurs.
| export const GitCommitIcon: LucideIcon = (props) => ( | ||
| <PiGitCommit className={props.className} style={props.style} /> | ||
| ); |
There was a problem hiding this comment.
🟢 Low lib/icons.tsx:99
GitCommitIcon only forwards className and style, but the LucideIcon type signature requires forwarding all SVGProps<SVGSVGElement>. Props like onClick, width, height, and aria-label are silently dropped, breaking accessibility and event handling compared to other icons that use adaptIcon. Consider spreading all props.
| export const GitCommitIcon: LucideIcon = (props) => ( | |
| <PiGitCommit className={props.className} style={props.style} /> | |
| ); | |
| export const GitCommitIcon: LucideIcon = (props) => ( | |
| <PiGitCommit {...props} /> | |
| ); |
🤖 Copy this AI Prompt to have your agent fix this:
In file apps/web/src/lib/icons.tsx around lines 99-101:
`GitCommitIcon` only forwards `className` and `style`, but the `LucideIcon` type signature requires forwarding all `SVGProps<SVGSVGElement>`. Props like `onClick`, `width`, `height`, and `aria-label` are silently dropped, breaking accessibility and event handling compared to other icons that use `adaptIcon`. Consider spreading all props.
Evidence trail:
apps/web/src/lib/icons.tsx line 64: `LucideIcon` type definition as `FC<SVGProps<SVGSVGElement>>`; lines 66-70: `adaptIcon` function that spreads all props; line 99: `GitCommitIcon` only forwarding `className` and `style` instead of spreading all props like other icons do.
| export function StoreProvider({ children }: { children: ReactNode }) { | ||
| useEffect(() => { | ||
| persistState(useStore.getState()); | ||
| }, []); | ||
| return createElement(Fragment, null, children); |
There was a problem hiding this comment.
🟡 Medium src/store.ts:465
StoreProvider calls persistState(useStore.getState()) immediately on mount, before syncServerReadModel has populated the store. This overwrites localStorage with empty arrays for expandedProjectCwds and projectOrderCwds, dropping the user's saved project expansion and order preferences if they refresh before the subscription's debounced persist fires.
| export function StoreProvider({ children }: { children: ReactNode }) { | |
| useEffect(() => { | |
| persistState(useStore.getState()); | |
| }, []); | |
| return createElement(Fragment, null, children); | |
| export function StoreProvider({ children }: { children: ReactNode }) { | |
| return createElement(Fragment, null, children); | |
| } |
🤖 Copy this AI Prompt to have your agent fix this:
In file apps/web/src/store.ts around lines 465-469:
`StoreProvider` calls `persistState(useStore.getState())` immediately on mount, before `syncServerReadModel` has populated the store. This overwrites localStorage with empty arrays for `expandedProjectCwds` and `projectOrderCwds`, dropping the user's saved project expansion and order preferences if they refresh before the subscription's debounced persist fires.
Evidence trail:
apps/web/src/store.ts lines 32-36 (initialState with empty projects), lines 44-62 (readPersistedState populates module-level vars but returns initialState), lines 70-79 (persistState writes projects array to localStorage), lines 463-466 (StoreProvider useEffect calls persistState immediately), line 95 (debouncedPersistState has 500ms wait), lines 439-441 (store initialized with readPersistedState). apps/web/src/routes/__root.tsx lines 512-514 (syncServerReadModel called after async api.orchestration.getSnapshot() call)
| if (importedMessages.length === 0) { | ||
| return false; | ||
| } | ||
| if (input.thread.handoff !== null) { |
There was a problem hiding this comment.
🟢 Low lib/threadHandoff.ts:82
input.thread.handoff !== null evaluates to true when handoff is undefined, causing the function to incorrectly require native messages when no prior handoff exists. Since handoff is typed as handoff?: ThreadHandoff | null, the property can be omitted (undefined) or explicitly null. The check should use input.thread.handoff != null to handle both absent and explicit null cases.
- if (input.thread.handoff !== null) {
+ if (input.thread.handoff != null) {🤖 Copy this AI Prompt to have your agent fix this:
In file apps/web/src/lib/threadHandoff.ts around line 82:
`input.thread.handoff !== null` evaluates to `true` when `handoff` is `undefined`, causing the function to incorrectly require native messages when no prior handoff exists. Since `handoff` is typed as `handoff?: ThreadHandoff | null`, the property can be omitted (`undefined`) or explicitly `null`. The check should use `input.thread.handoff != null` to handle both absent and explicit null cases.
Evidence trail:
1. `apps/web/src/lib/threadHandoff.ts` line 82 (REVIEWED_COMMIT): `if (input.thread.handoff !== null)`
2. `apps/web/src/lib/threadHandoff.ts` lines 64-68: function signature shows `input.thread` uses `Pick<Thread, "handoff" | "messages" | "session">`
3. `apps/web/src/types.ts` line 113 (REVIEWED_COMMIT): `handoff?: ThreadHandoff | null;` - confirms optional property that can be undefined
4. JavaScript semantics: `undefined !== null` evaluates to `true`, `undefined != null` evaluates to `false`
| cached: false, | ||
| }; | ||
| } | ||
| return yield* adapter.listModels(); |
There was a problem hiding this comment.
🟢 Low Layers/ProviderDiscoveryService.ts:93
In listModels, adapter.listModels() is called with no arguments on line 93, but listSkills passes parsed to adapter.listSkills(parsed) on line 75. The adapter's listModels method likely needs the parsed input for provider-specific filtering or pagination, so the input is silently dropped.
- return yield* adapter.listModels();
+ return yield* adapter.listModels(parsed);🤖 Copy this AI Prompt to have your agent fix this:
In file apps/server/src/provider/Layers/ProviderDiscoveryService.ts around line 93:
In `listModels`, `adapter.listModels()` is called with no arguments on line 93, but `listSkills` passes `parsed` to `adapter.listSkills(parsed)` on line 75. The adapter's `listModels` method likely needs the parsed input for provider-specific filtering or pagination, so the input is silently dropped.
Evidence trail:
- `apps/server/src/provider/Layers/ProviderDiscoveryService.ts` lines 75 and 93 (REVIEWED_COMMIT): Shows `adapter.listSkills(parsed)` vs `adapter.listModels()` call pattern
- `apps/server/src/provider/Layers/ClaudeAdapter.ts` lines 3120-3127 (REVIEWED_COMMIT): Shows `listSkills` uses `input.threadId` and `input.cwd` from the input parameter
- `apps/server/src/codexAppServerManager.ts` line 1108 (REVIEWED_COMMIT): Shows `listModels(threadId?: string)` expects threadId parameter
ApprovabilityVerdict: Needs human review 4 blocking correctness issues found. Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
1 participant
What Changed
Why
UI Changes
Checklist
Note
Medium Risk
Adds new persistence fields (
handoff_json, messagesource) plus new provider discovery flows and session-start options; regressions could affect thread rendering or provider startup/turn sending.Overview
Adds thread handoff support end-to-end: new
thread.handoff.createcommand,handoffmetadata on threads, and logic to bootstrap the first provider turn with a truncated summary of imported messages (then marks bootstrap as completed).Extends projections/persistence to store
handoff_jsononprojection_threadsand asourcefield onprojection_thread_messages(migration included), updates snapshot/query/projector logic accordingly, and adjusts tests to build fullOrchestrationReadModelsnapshots.Enhances the Codex app-server integration with skill discovery/model listing (cached, with
cwds→cwdfallback) and the ability to include selected skills as structuredturn/startinput items; session startup now acceptsproviderOptions.codexinstead of top-levelbinaryPath/homePath, and stderr now emits errors instead of notifications.Reviewed by Cursor Bugbot for commit 5e5f8d2. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add thread handoff between providers with skill discovery and composer skill tokens
thread.handoff.createorchestration command that creates a new thread seeded with imported messages from a source thread, including re-handoff guards requiring at least one native message before allowing chained handoffs.CodexAppServerManagerandClaudeAdapter: both can now list skills (with caching and retry logic), report composer capabilities, and spawn ephemeral discovery sessions when no active session exists.ComposerSkillNode, a new atomic Lexical token in the prompt editor for inline skill chips, with$//prefix detection in the prompt parser and command menu skill item rendering.handoffmetadata on threads andsourceon messages via a new DB migration (017_ThreadHandoffMetadata.ts) and updates projection, repository, and snapshot layers throughout.sidebarThreadsById/threadIdsByProjectIdandapplyOrchestrationEvent, instead persisting project expansion and order to localStorage and hydrating fromsyncServerReadModel.chat.visible.next/previousfor keyboard thread cycling usinggetNextVisibleSidebarThreadId.applyOrchestrationEventis removed from the store — any callers relying on incremental event-driven state updates must migrate to the new functions orsyncServerReadModel.📊 Macroscope summarized 5e5f8d2. 40 files reviewed, 17 issues evaluated, 1 issue filtered, 14 comments posted
🗂️ Filtered Issues
apps/server/src/codexAppServerManager.ts — 0 comments posted, 1 evaluated, 1 filtered
input.threadId?.trim()on line 1070, butresolveContextForDiscoveryon line 1082 receives the untrimmedinput.threadId. IfthreadIdis" abc ", the cache key uses"abc"while the actual context resolution uses" abc ". A subsequent call withthreadId = "abc"will hit the cache and return results from a different context, or conversely, different whitespace-padded variants of the same logical ID will bypass cache when they shouldn't. [ Cross-file consolidated ]