Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughDeleted legacy ensureAgentHooks; added idempotent write-if-changed helpers and Superset-managed hook preservation; reworked shell wrappers and exported getShellArgs; centralized shell-arg usage in Session and refactored snapshot boundaries to a waiter system; introduced polyfills and dynamic imports in tests; made several concurrency and tooling adjustments. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts (1)
133-139: Inconsistent shell-name detection across functions.
getShellArgsusesshell.split("/").pop()for exact name matching against an allow-list, butgetShellEnv(Line 124) andgetCommandShellArgs(Lines 151, 154) still useshell.includes("zsh")/shell.includes("bash"). This divergence means edge-case paths are handled differently—e.g., a shell path like/usr/local/bin/fizshwould matchincludes("zsh")ingetShellEnv(getting ZDOTDIR overrides) but would not match the allow-list ingetShellArgs(getting no-l).Consider normalizing all three functions to use the same
shell.split("/").pop()approach for consistency.♻️ Suggested refactor
Extract the shell name derivation into a shared helper:
+function getShellName(shell: string): string { + return shell.split("/").pop() || shell; +} + export function getShellEnv(shell: string): Record<string, string> { - if (shell.includes("zsh")) { + if (getShellName(shell) === "zsh") { return { SUPERSET_ORIG_ZDOTDIR: process.env.ZDOTDIR || os.homedir(), ZDOTDIR: ZSH_DIR, }; } return {}; } export function getShellArgs(shell: string): string[] { - const shellName = shell.split("/").pop() || shell; + const shellName = getShellName(shell); if (["zsh", "bash", "sh", "ksh", "fish"].includes(shellName)) { return ["-l"]; } return []; }Apply similarly to
getCommandShellArgs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts` around lines 133 - 139, The three functions use different shell-detection heuristics causing inconsistent behavior; create a small helper (e.g., deriveShellName(shell: string) that returns shell.split("/").pop() || shell) and replace all usages of shell.includes("...") in getShellEnv and getCommandShellArgs with the helper plus exact-name checks (same allow-list used by getShellArgs: zsh, bash, sh, ksh, fish) so all three functions detect shells the same way and apply ZDOTDIR/`-l` handling consistently.apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts (1)
96-101: Test covers most shells but misseskshand the fallback (unknown shell) case.
getShellArgshaskshin its allow-list, but the test doesn't exercise it. More importantly, there's no assertion that an unrecognized shell (e.g.,"/bin/csh") returns[], which is the other branch in the function.💚 Suggested additions
it("uses login args for interactive shells", () => { expect(getShellArgs("/bin/zsh")).toEqual(["-l"]); expect(getShellArgs("/bin/bash")).toEqual(["-l"]); expect(getShellArgs("/bin/sh")).toEqual(["-l"]); + expect(getShellArgs("/bin/ksh")).toEqual(["-l"]); expect(getShellArgs("/opt/homebrew/bin/fish")).toEqual(["-l"]); }); + + it("returns empty args for unrecognized shells", () => { + expect(getShellArgs("/bin/csh")).toEqual([]); + expect(getShellArgs("powershell")).toEqual([]); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts` around lines 96 - 101, Add assertions to cover the ksh allow-list and the unknown-shell fallback in the getShellArgs tests: include expect(getShellArgs("/bin/ksh")).toEqual(["-l"]) (or the exact ksh path used in the codebase) and assert that an unrecognized shell like expect(getShellArgs("/bin/csh")).toEqual([]) to verify the fallback branch returns an empty array; update the test case in shell-wrappers.test.ts that currently checks zsh/bash/sh/fish to include these two additional expectations for getShellArgs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts`:
- Around line 96-101: Add assertions to cover the ksh allow-list and the
unknown-shell fallback in the getShellArgs tests: include
expect(getShellArgs("/bin/ksh")).toEqual(["-l"]) (or the exact ksh path used in
the codebase) and assert that an unrecognized shell like
expect(getShellArgs("/bin/csh")).toEqual([]) to verify the fallback branch
returns an empty array; update the test case in shell-wrappers.test.ts that
currently checks zsh/bash/sh/fish to include these two additional expectations
for getShellArgs.
In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts`:
- Around line 133-139: The three functions use different shell-detection
heuristics causing inconsistent behavior; create a small helper (e.g.,
deriveShellName(shell: string) that returns shell.split("/").pop() || shell) and
replace all usages of shell.includes("...") in getShellEnv and
getCommandShellArgs with the helper plus exact-name checks (same allow-list used
by getShellArgs: zsh, bash, sh, ksh, fish) so all three functions detect shells
the same way and apply ZDOTDIR/`-l` handling consistently.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
apps/desktop/src/main/lib/agent-setup/notify-hook.ts (1)
15-34: ExtractwriteFileIfChangedto sharedutils.tsto eliminate duplication across three files.This utility function is currently defined independently in
notify-hook.ts,agent-wrappers.ts, andshell-wrappers.ts—all within the same directory. Moving it to the existingutils.tswould provide a single canonical implementation and reduce maintenance burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/notify-hook.ts` around lines 15 - 34, Extract the writeFileIfChanged implementation into the shared utils.ts: move the function body and signature (writeFileIfChanged(filePath: string, content: string, mode: number): boolean) into utils.ts, export it, then replace the local definitions in notify-hook.ts, agent-wrappers.ts and shell-wrappers.ts with an import of writeFileIfChanged from utils.ts and remove the duplicated code; keep the behavior identical (including chmod best-effort) and update any local references to call the imported function.apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts (2)
118-217: Missing idempotency assertion — the core invariant of this PR is untested.The PR commits are titled "Add idempotency" / "Add more idempotency", but neither new test verifies the central property: calling the hook-content function a second time on the already-updated config produces no additional/duplicate entries.
A minimal idempotency check would be:
// after the first call const content2 = getCursorHooksJsonContent(currentHookPath); expect(JSON.parse(content2)).toEqual(JSON.parse(content));The same pattern applies to the Gemini test. Without this, a regression that re-appends our hook on every call would pass both tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts` around lines 118 - 217, Add an idempotency check by calling getCursorHooksJsonContent a second time and asserting the parsed result is identical to the first call (e.g., call getCursorHooksJsonContent(currentHookPath) again into content2 and expect JSON.parse(content2) toEqual JSON.parse(content)); do the same in the Gemini test by calling getGeminiSettingsJsonContent(currentHookPath) a second time and asserting equality of the parsed results to ensure no duplicated/extra entries are produced by repeated invocations.
119-124: RedundantmockedHomeDirassignment — same path asbeforeEachdefault.
homeDirresolves topath.join(TEST_ROOT, "home"), which is identical to the value already set bybeforeEach. The assignmentmockedHomeDir = homeDiron line 124 (and similarly line 170) is a no-op. The same observation applies to lines 165–170 in the Gemini test.If the intent is to verify behaviour with a distinct mock home directory (e.g., simulating a relocated Superset installation), both tests need to set
mockedHomeDirto a path that differs from thebeforeEachdefault. Otherwise, the localhomeDirvariable and themockedHomeDirreassignment can simply be removed.♻️ Proposed cleanup (if a distinct path isn't actually needed)
it("replaces stale Cursor hook commands from old superset paths", () => { - const homeDir = path.join(TEST_ROOT, "home"); - const cursorHooksPath = path.join(homeDir, ".cursor", "hooks.json"); + const cursorHooksPath = path.join(mockedHomeDir, ".cursor", "hooks.json"); const staleHookPath = "/tmp/.superset-old/hooks/cursor-hook.sh"; const currentHookPath = "/tmp/.superset-new/hooks/cursor-hook.sh"; - mockedHomeDir = homeDir; mkdirSync(path.dirname(cursorHooksPath), { recursive: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts` around lines 119 - 124, The assignment mockedHomeDir = homeDir is redundant because beforeEach already sets mockedHomeDir to path.join(TEST_ROOT, "home"); either remove the local homeDir variable and the mockedHomeDir reassignment in the tests around cursorHooksPath/staleHookPath/currentHookPath, or if you meant to test a different home location set mockedHomeDir to a distinct path (e.g. path.join(TEST_ROOT, "alt-home")) so it differs from the beforeEach default; update the Gemini test similarly (lines referencing mockedHomeDir) and keep cursorHooksPath/staleHookPath/currentHookPath computed from the chosen mockedHomeDir.apps/desktop/src/main/terminal-host/session.ts (2)
579-616:flushToSnapshotBoundary— early-return condition is correct but reads oddly.Line 583:
targetProcessedItems <= this.emulatorWriteProcessedItemsalgebraically simplifies toqueue.length <= 0(i.e., queue is empty), which is always the intent. The logic is correct, but a reader unfamiliar with the derivation might find the comparison betweentargetProcessedItems(just computed asprocessed + queue.length) andthis.emulatorWriteProcessedItemsconfusing. A comment or simplification would aid readability.♻️ Slightly clearer alternative
- const targetProcessedItems = - this.emulatorWriteProcessedItems + this.emulatorWriteQueue.length; - - if (targetProcessedItems <= this.emulatorWriteProcessedItems) { - return true; // Already flushed - } + if (this.emulatorWriteQueue.length === 0) { + return true; // Already flushed + } + + const targetProcessedItems = + this.emulatorWriteProcessedItems + this.emulatorWriteQueue.length;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/session.ts` around lines 579 - 616, In flushToSnapshotBoundary, the early-return condition compares targetProcessedItems to this.emulatorWriteProcessedItems which is correct but unclear; replace that check with a direct, readable condition like this.emulatorWriteQueue.length === 0 (or add a one-line comment explaining the algebraic simplification) so readers immediately see we’re returning when the write queue is empty—update the condition where targetProcessedItems is compared and reference the variables targetProcessedItems, this.emulatorWriteProcessedItems, and this.emulatorWriteQueue in the change.
180-189:envSourcefallback and type assertion look reasonable.The intent is clear: prefer the caller-provided
envand fall back toprocess.envonly when nothing was provided. Theas Record<string, string>cast onprocess.envis technically lossy (values can beundefined), butbuildSafeEnviterates viaObject.entrieswhich omitsundefined-valued keys in Node.js, so it's safe in practice.One note: an empty
envobject{}(explicitly passed with no keys) will trigger theprocess.envfallback. If a caller intentionally passes an empty env to get a minimal environment, this would be surprising. Consider checkingenv !== undefinedinstead of key count if that scenario is possible.♻️ Alternative check if empty-env intent matters
- const envSource = - Object.keys(env).length > 0 - ? env - : (process.env as Record<string, string>); + const envSource = + options.env != null + ? env + : (process.env as Record<string, string>);This would require adjusting the destructuring above to keep
options.envavailable for the null check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/session.ts` around lines 180 - 189, The current env fallback uses Object.keys(env).length and thus treats an explicitly passed empty object as "no env" — change the logic to detect undefined-only inputs so an explicitly passed {} is respected: use a presence check (env !== undefined) when choosing envSource (the variable used to call buildSafeEnv), ensuring you preserve the original options.env in the surrounding scope if you need to test for undefined; update the envSource assignment that feeds buildSafeEnv and leave processEnv.TERM and subsequent getShellArgs(this.shell) untouched.apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts (1)
146-154:getShellEnvuses substring match whilegetShellArgsuses exact basename match — inconsistent dispatch.
getShellEnvchecksshell.includes("zsh")(Line 147) whilegetShellArgsextracts the basename and matches against an explicit allow-list (Line 157-158). A path like/usr/bin/zsh-customwould matchgetShellEnvbut notgetShellArgs. Consider aligning both functions to use the same basename extraction strategy.♻️ Suggested alignment
export function getShellEnv(shell: string): Record<string, string> { - if (shell.includes("zsh")) { + const shellName = shell.split("/").pop() || shell; + if (shellName === "zsh") { return { SUPERSET_ORIG_ZDOTDIR: process.env.ZDOTDIR || os.homedir(), ZDOTDIR: ZSH_DIR, }; } return {}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts` around lines 146 - 154, getShellEnv currently uses substring matching (shell.includes("zsh")) which is inconsistent with getShellArgs' basename-based allow-list; change getShellEnv to extract the executable basename (e.g., using path.basename or same helper used by getShellArgs) and perform the same exact match/allow-list check for "zsh" before returning ZDOTDIR/SUPERSET_ORIG_ZDOTDIR (reference getShellEnv, getShellArgs, and ZSH_DIR) so both functions dispatch identically for inputs like "/usr/bin/zsh-custom".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts`:
- Around line 156-162: getShellEnv() currently returns an empty env for bash so
the rcfile produced by createBashWrapper() (which contains
buildPathPrependFunction() and buildShimFunctions()) is never sourced; update
getShellEnv() to set BASH_ENV to the full path of the bash rcfile when the
detected shell is "bash" (or shellName === "bash") so interactive/login bash
sessions will source the wrapper, ensuring BIN_DIR gets prepended to PATH and
shims (claude, codex, etc.) are available; keep using
getShellArgs()/getShellArgs()'s "-l" for login shells and do not switch to
--rcfile.
---
Nitpick comments:
In `@apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts`:
- Around line 118-217: Add an idempotency check by calling
getCursorHooksJsonContent a second time and asserting the parsed result is
identical to the first call (e.g., call
getCursorHooksJsonContent(currentHookPath) again into content2 and expect
JSON.parse(content2) toEqual JSON.parse(content)); do the same in the Gemini
test by calling getGeminiSettingsJsonContent(currentHookPath) a second time and
asserting equality of the parsed results to ensure no duplicated/extra entries
are produced by repeated invocations.
- Around line 119-124: The assignment mockedHomeDir = homeDir is redundant
because beforeEach already sets mockedHomeDir to path.join(TEST_ROOT, "home");
either remove the local homeDir variable and the mockedHomeDir reassignment in
the tests around cursorHooksPath/staleHookPath/currentHookPath, or if you meant
to test a different home location set mockedHomeDir to a distinct path (e.g.
path.join(TEST_ROOT, "alt-home")) so it differs from the beforeEach default;
update the Gemini test similarly (lines referencing mockedHomeDir) and keep
cursorHooksPath/staleHookPath/currentHookPath computed from the chosen
mockedHomeDir.
In `@apps/desktop/src/main/lib/agent-setup/notify-hook.ts`:
- Around line 15-34: Extract the writeFileIfChanged implementation into the
shared utils.ts: move the function body and signature
(writeFileIfChanged(filePath: string, content: string, mode: number): boolean)
into utils.ts, export it, then replace the local definitions in notify-hook.ts,
agent-wrappers.ts and shell-wrappers.ts with an import of writeFileIfChanged
from utils.ts and remove the duplicated code; keep the behavior identical
(including chmod best-effort) and update any local references to call the
imported function.
In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts`:
- Around line 146-154: getShellEnv currently uses substring matching
(shell.includes("zsh")) which is inconsistent with getShellArgs' basename-based
allow-list; change getShellEnv to extract the executable basename (e.g., using
path.basename or same helper used by getShellArgs) and perform the same exact
match/allow-list check for "zsh" before returning ZDOTDIR/SUPERSET_ORIG_ZDOTDIR
(reference getShellEnv, getShellArgs, and ZSH_DIR) so both functions dispatch
identically for inputs like "/usr/bin/zsh-custom".
In `@apps/desktop/src/main/terminal-host/session.ts`:
- Around line 579-616: In flushToSnapshotBoundary, the early-return condition
compares targetProcessedItems to this.emulatorWriteProcessedItems which is
correct but unclear; replace that check with a direct, readable condition like
this.emulatorWriteQueue.length === 0 (or add a one-line comment explaining the
algebraic simplification) so readers immediately see we’re returning when the
write queue is empty—update the condition where targetProcessedItems is compared
and reference the variables targetProcessedItems,
this.emulatorWriteProcessedItems, and this.emulatorWriteQueue in the change.
- Around line 180-189: The current env fallback uses Object.keys(env).length and
thus treats an explicitly passed empty object as "no env" — change the logic to
detect undefined-only inputs so an explicitly passed {} is respected: use a
presence check (env !== undefined) when choosing envSource (the variable used to
call buildSafeEnv), ensuring you preserve the original options.env in the
surrounding scope if you need to test for undefined; update the envSource
assignment that feeds buildSafeEnv and leave processEnv.TERM and subsequent
getShellArgs(this.shell) untouched.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/lib/terminal-host/headless-emulator.test.ts (1)
528-529:⚠️ Potential issue | 🟠 Major
HeadlessEmulatortype annotation at line 529 is invalid after switching to dynamic import.Line 22 uses
const { HeadlessEmulator } = await import(...), which creates only a value binding. Line 53 correctly usesInstanceType<typeof HeadlessEmulator>for type safety, but line 529 still usesHeadlessEmulatordirectly as a type annotation. This will cause TypeScript to fail sinceHeadlessEmulatoris not available as a type.Proposed fix
async function applySnapshotAsync( - emulator: HeadlessEmulator, + emulator: InstanceType<typeof HeadlessEmulator>, snapshot: { rehydrateSequences: string; snapshotAnsi: string }, ): Promise<void> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/terminal-host/headless-emulator.test.ts` around lines 528 - 529, The parameter type annotation on applySnapshotAsync is invalid because HeadlessEmulator is only a runtime value from the dynamic import; change the signature to use the same safe type used elsewhere—replace the direct type HeadlessEmulator with InstanceType<typeof HeadlessEmulator> (or create a local type alias using that) so the parameter is a proper compile-time type and matches the usage in applySnapshotAsync.
🧹 Nitpick comments (3)
apps/desktop/src/main/lib/terminal/session.test.ts (1)
4-12: Polyfill + dynamic imports look good; consider extracting the shared polyfill.The polyfill guard and dynamic import pattern is correct and consistent with
headless-emulator.test.ts. Since this exact three-line polyfill block is duplicated across at least two test files, you could extract it into a shared test-setup module (e.g.,test-setup.ts) that both files import, reducing future maintenance if the polyfill logic changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/terminal/session.test.ts` around lines 4 - 12, The duplicated polyfill block (the typeof window === "undefined" guard that sets (globalThis as Record<string, unknown>).window = globalThis) should be extracted into a shared test setup module (e.g., test-setup.ts) and then imported at the top of tests like session.test.ts and headless-emulator.test.ts; create the module that performs the same guard and assignment, export nothing (or an init function) and replace the three-line polyfill in files that currently perform the assignment (session.test.ts referencing SerializeAddon/HeadlessTerminal and any other test files) with a single import of the shared test-setup module so future changes to the polyfill are made in one place.apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts (2)
168-221: Same redundantmockedHomeDirassignment (line 174); consider coveringAfterAgent/AfterToolevents.
mockedHomeDir = homeDiron line 174 is a no-op for the same reason as in the Cursor test above.
getGeminiSettingsJsonContentalso processesAfterAgentandAfterToolevents (seeagent-wrappers.tslines 427-452). The fixture only sets stale data onBeforeAgent, leavingAfterAgentandAfterToolunverified. Adding stale entries there and asserting their replacement would give fuller coverage without much extra code.🧹 Proposed cleanup + coverage expansion
it("replaces stale Gemini hook commands from old superset paths", () => { - const homeDir = path.join(TEST_ROOT, "home"); - const geminiSettingsPath = path.join(homeDir, ".gemini", "settings.json"); + const geminiSettingsPath = path.join(mockedHomeDir, ".gemini", "settings.json"); const staleHookPath = "/tmp/.superset-old/hooks/gemini-hook.sh"; const currentHookPath = "/tmp/.superset-new/hooks/gemini-hook.sh"; - mockedHomeDir = homeDir; mkdirSync(path.dirname(geminiSettingsPath), { recursive: true }); writeFileSync( geminiSettingsPath, JSON.stringify( { hooks: { BeforeAgent: [ { hooks: [{ type: "command", command: staleHookPath }], }, { hooks: [{ type: "command", command: "/opt/custom-hook.sh" }], }, ], + AfterAgent: [ + { hooks: [{ type: "command", command: staleHookPath }] }, + ], + AfterTool: [ + { hooks: [{ type: "command", command: staleHookPath }] }, + ], }, }, null, 2, ), ); const content = getGeminiSettingsJsonContent(currentHookPath); // ... existing assertions ... + for (const eventName of ["AfterAgent", "AfterTool"] as const) { + const entries = parsed.hooks[eventName]; + expect( + entries.some((def) => def.hooks.some((h) => h.command === currentHookPath)), + ).toBe(true); + expect( + entries.some((def) => def.hooks.some((h) => h.command.includes(staleHookPath))), + ).toBe(false); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts` around lines 168 - 221, Remove the redundant mockedHomeDir = homeDir assignment (it’s a no-op here) and expand the test for getGeminiSettingsJsonContent to also place stale hook entries under AfterAgent and AfterTool and assert they are replaced with currentHookPath just like BeforeAgent; locate the test block using mockedHomeDir and getGeminiSettingsJsonContent and add similar stale definitions and expectation checks for the parsed.hooks.AfterAgent and parsed.hooks.AfterTool arrays to verify replacement and preserve the existing check for the custom hook path.
122-166: Nit:mockedHomeDir = homeDir(line 128) is redundant.
beforeEachalready resetsmockedHomeDirtopath.join(TEST_ROOT, "home"), which is the same value assigned at line 128. The localhomeDirvariable and the subsequent re-assignment can be dropped.🧹 Proposed cleanup
it("replaces stale Cursor hook commands from old superset paths", () => { - const homeDir = path.join(TEST_ROOT, "home"); - const cursorHooksPath = path.join(homeDir, ".cursor", "hooks.json"); + const cursorHooksPath = path.join(mockedHomeDir, ".cursor", "hooks.json"); const staleHookPath = "/tmp/.superset-old/hooks/cursor-hook.sh"; const currentHookPath = "/tmp/.superset-new/hooks/cursor-hook.sh"; - mockedHomeDir = homeDir; mkdirSync(path.dirname(cursorHooksPath), { recursive: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts` around lines 122 - 166, In the "replaces stale Cursor hook commands from old superset paths" test remove the redundant local homeDir and the line assigning mockedHomeDir = homeDir; instead use the existing mockedHomeDir (already initialized in beforeEach) when building cursorHooksPath and any other paths (e.g. cursorHooksPath = path.join(mockedHomeDir, ".cursor", "hooks.json")). This eliminates the duplicate assignment while keeping the rest of the test logic (writing hooks.json, calling getCursorHooksJsonContent, and the assertions) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/desktop/src/main/lib/terminal-host/headless-emulator.test.ts`:
- Around line 528-529: The parameter type annotation on applySnapshotAsync is
invalid because HeadlessEmulator is only a runtime value from the dynamic
import; change the signature to use the same safe type used elsewhere—replace
the direct type HeadlessEmulator with InstanceType<typeof HeadlessEmulator> (or
create a local type alias using that) so the parameter is a proper compile-time
type and matches the usage in applySnapshotAsync.
---
Nitpick comments:
In `@apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts`:
- Around line 168-221: Remove the redundant mockedHomeDir = homeDir assignment
(it’s a no-op here) and expand the test for getGeminiSettingsJsonContent to also
place stale hook entries under AfterAgent and AfterTool and assert they are
replaced with currentHookPath just like BeforeAgent; locate the test block using
mockedHomeDir and getGeminiSettingsJsonContent and add similar stale definitions
and expectation checks for the parsed.hooks.AfterAgent and
parsed.hooks.AfterTool arrays to verify replacement and preserve the existing
check for the custom hook path.
- Around line 122-166: In the "replaces stale Cursor hook commands from old
superset paths" test remove the redundant local homeDir and the line assigning
mockedHomeDir = homeDir; instead use the existing mockedHomeDir (already
initialized in beforeEach) when building cursorHooksPath and any other paths
(e.g. cursorHooksPath = path.join(mockedHomeDir, ".cursor", "hooks.json")). This
eliminates the duplicate assignment while keeping the rest of the test logic
(writing hooks.json, calling getCursorHooksJsonContent, and the assertions)
unchanged.
In `@apps/desktop/src/main/lib/terminal/session.test.ts`:
- Around line 4-12: The duplicated polyfill block (the typeof window ===
"undefined" guard that sets (globalThis as Record<string, unknown>).window =
globalThis) should be extracted into a shared test setup module (e.g.,
test-setup.ts) and then imported at the top of tests like session.test.ts and
headless-emulator.test.ts; create the module that performs the same guard and
assignment, export nothing (or an init function) and replace the three-line
polyfill in files that currently perform the assignment (session.test.ts
referencing SerializeAddon/HeadlessTerminal and any other test files) with a
single import of the shared test-setup module so future changes to the polyfill
are made in one place.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts (3)
10-11: Optional: consolidate the twonode:osimports.Both
realOsandtmpdirare imported from"node:os".tmpdircan be accessed asrealOs.tmpdir(), removing the redundant import.♻️ Proposed refactor
-import * as realOs from "node:os"; -import { tmpdir } from "node:os"; +import * as realOs from "node:os";And update usage:
-const TEST_ROOT = path.join( - tmpdir(), +const TEST_ROOT = path.join( + realOs.tmpdir(), `superset-agent-wrappers-${process.pid}-${Date.now()}`, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts` around lines 10 - 11, Consolidate the duplicate node:os imports by removing the named import tmpdir and using realOs.tmpdir() instead; update any references to tmpdir() in agent-wrappers.test.ts to call realOs.tmpdir() and remove the line importing { tmpdir } while keeping import * as realOs from "node:os".
122-168: Consider asserting the other Cursor event types generated from scratch.The test only pre-populates
beforeSubmitPromptand asserts on it.getCursorHooksJsonContentalso generatesstop,beforeShellExecution, andbeforeMCPExecution(all created fresh since they are absent from the initial file). At minimum, asserting their presence inparsed.hookswould prevent silent regression if those entries stop being written.♻️ Proposed additions
+ expect(parsed.hooks.stop).toBeDefined(); + expect(parsed.hooks.beforeShellExecution).toBeDefined(); + expect(parsed.hooks.beforeMCPExecution).toBeDefined();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts` around lines 122 - 168, The test populates only beforeSubmitPrompt but should also assert that getCursorHooksJsonContent writes the freshly generated event keys; update the test around getCursorHooksJsonContent and parsed (the parsed JSON) to assert parsed.hooks contains keys "stop", "beforeShellExecution", and "beforeMCPExecution" (in addition to beforeSubmitPrompt) and that each is an array (and non-empty if expected) so missing generation of those entries will fail the test; locate the assertions near the existing checks for beforeSubmitPrompt in the test that references getCursorHooksJsonContent and parsed.hooks.
170-229: Consider assertingAfterAgentandAfterToolentries in the Gemini test.
getGeminiSettingsJsonContentunconditionally writes all three event types (BeforeAgent,AfterAgent,AfterTool), but onlyBeforeAgentis asserted. Adding presence checks for the other two would catch regressions in the full hook set.♻️ Proposed additions
+ expect(parsed.hooks.AfterAgent).toBeDefined(); + expect(parsed.hooks.AfterTool).toBeDefined();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts` around lines 170 - 229, Add assertions in the test that verify the getGeminiSettingsJsonContent output also contains AfterAgent and AfterTool entries and that they follow the same replacement/retention rules as BeforeAgent: parse the JSON (already done into parsed) and assert parsed.hooks.AfterAgent and parsed.hooks.AfterTool exist, that each contains at least one hook entry whose command equals currentHookPath, that neither contains the staleHookPath, and that any custom commands like "/opt/custom-hook.sh" are retained; apply the same checks to content2 (parsed from JSON.parse(content2)) to ensure parity.apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts (1)
96-107:getShellArgscoverage looks good; consider adding a path-with-version test.The allow-list and rejection cases are well covered. One untested edge: versioned shell paths like
/usr/local/bin/bash-5.2or/usr/bin/fish3—getShellNamewould returnbash-5.2/fish3, which would fall through to[]. If that's unintentional, a test would catch it. If intentional, a comment ingetShellArgswould clarify.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts` around lines 96 - 107, Add a unit test in apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts that covers versioned shell paths (e.g. "/usr/local/bin/bash-5.2" and "/usr/bin/fish3") and assert they return the same args as their base shells via getShellArgs; to make the test pass, update getShellName to normalize versioned names by stripping common version suffixes (e.g. remove trailing "-<version>" or trailing digits/letters) so "bash-5.2" => "bash" and "fish3" => "fish", then ensure getShellArgs uses the normalized name for its allow-list matching.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts`:
- Around line 28-31: The current call to fs.writeFileSync(filePath, content, {
mode }) won't change permissions for an existing file; detect whether the file
existed before writing (or simply always enforce permissions) and call
fs.chmodSync(filePath, mode) after writing to ensure the requested mode is
applied; update the logic around writeFileSync, the pre-write existence check,
and the existing chmodSync branch so that permissions are corrected for both
newly created and overwritten files (referencing filePath, content, mode,
writeFileSync, and chmodSync).
---
Duplicate comments:
In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts`:
- Around line 150-167: The change is intentional: keep getShellEnv limited to
zsh (setting SUPERSET_ORIG_ZDOTDIR and ZDOTDIR using ZSH_DIR) and do not add
BASH_ENV for bash; getShellArgs already returns ["-l"] for bash/zsh/bash-like
shells so bash will be launched as a login shell and will source profile
files—do not modify getShellEnv or add BASH_ENV for bash, leave getShellArgs and
the zsh-specific env injection as-is (refer to getShellEnv, getShellArgs,
BASH_ENV, SUPERSET_ORIG_ZDOTDIR, ZDOTDIR, and ZSH_DIR).
---
Nitpick comments:
In `@apps/desktop/src/main/lib/agent-setup/agent-wrappers.test.ts`:
- Around line 10-11: Consolidate the duplicate node:os imports by removing the
named import tmpdir and using realOs.tmpdir() instead; update any references to
tmpdir() in agent-wrappers.test.ts to call realOs.tmpdir() and remove the line
importing { tmpdir } while keeping import * as realOs from "node:os".
- Around line 122-168: The test populates only beforeSubmitPrompt but should
also assert that getCursorHooksJsonContent writes the freshly generated event
keys; update the test around getCursorHooksJsonContent and parsed (the parsed
JSON) to assert parsed.hooks contains keys "stop", "beforeShellExecution", and
"beforeMCPExecution" (in addition to beforeSubmitPrompt) and that each is an
array (and non-empty if expected) so missing generation of those entries will
fail the test; locate the assertions near the existing checks for
beforeSubmitPrompt in the test that references getCursorHooksJsonContent and
parsed.hooks.
- Around line 170-229: Add assertions in the test that verify the
getGeminiSettingsJsonContent output also contains AfterAgent and AfterTool
entries and that they follow the same replacement/retention rules as
BeforeAgent: parse the JSON (already done into parsed) and assert
parsed.hooks.AfterAgent and parsed.hooks.AfterTool exist, that each contains at
least one hook entry whose command equals currentHookPath, that neither contains
the staleHookPath, and that any custom commands like "/opt/custom-hook.sh" are
retained; apply the same checks to content2 (parsed from JSON.parse(content2))
to ensure parity.
In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts`:
- Around line 96-107: Add a unit test in
apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts that covers
versioned shell paths (e.g. "/usr/local/bin/bash-5.2" and "/usr/bin/fish3") and
assert they return the same args as their base shells via getShellArgs; to make
the test pass, update getShellName to normalize versioned names by stripping
common version suffixes (e.g. remove trailing "-<version>" or trailing
digits/letters) so "bash-5.2" => "bash" and "fish3" => "fish", then ensure
getShellArgs uses the normalized name for its allow-list matching.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/desktop/src/main/terminal-host/session.test.ts (1)
94-96:| undefinedon theJSON.parsecast is redundant.
JSON.parseeither returns a parsed value or throws — it never returnsundefined. The| undefinedwidening is misleading.♻️ Tightened type cast
- ) as { args?: string[] } | undefined; + ) as { args?: string[] };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/session.test.ts` around lines 94 - 96, The explicit "| undefined" in the cast for spawnPayload is redundant because JSON.parse never returns undefined; update the cast on the parsed value to a concrete type (e.g., change "as { args?: string[] } | undefined" to "as { args?: string[] }") so spawnPayload has the correct tightened type; locate the expression that constructs spawnPayload (the JSON.parse(...) assigned to spawnPayload) and remove the "| undefined" branch from its type assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts`:
- Around line 96-101: The test fails because BASH_RCFILE is computed at module
load (so it captures the real path) instead of using the mocked BASH_DIR; update
shell-wrappers.ts by removing the module-level BASH_RCFILE constant and compute
the rcfile path inside getShellArgs (use path.join(BASH_DIR, "rcfile") when
handling "/bin/bash") so the function reads the mocked BASH_DIR at test time;
ensure getShellArgs is the only place that constructs the rcfile path and adjust
any imports/usages that referenced BASH_RCFILE to use the inline computation.
In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts`:
- Around line 163-165: Replace the module-level BASH_RCFILE constant with an
inline path.join(BASH_DIR, "rcfile") so the value is resolved against the live
BASH_DIR binding at call time: update the branch that returns ["--rcfile",
BASH_RCFILE] to use path.join(BASH_DIR, "rcfile") and likewise change the
fs.existsSync(BASH_RCFILE) check and the shell 'source' invocation inside
getCommandShellArgs to use path.join(BASH_DIR, "rcfile"); remove the now-unused
BASH_RCFILE constant if it is no longer referenced.
---
Duplicate comments:
In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts`:
- Around line 28-31: In writeFileIfChanged, when fs.writeFileSync(filePath,
content, { mode }) is used to overwrite an existing file, the mode option is a
no-op so permissions never get corrected; update the write path in
writeFileIfChanged to call fs.chmodSync(filePath, mode) after writing (same as
the no-change branch) so existing files receive the intended permissions; locate
writeFileIfChanged and ensure fs.chmodSync is invoked whenever content was
changed (i.e., after fs.writeFileSync) and reference fs.writeFileSync and
fs.chmodSync in the fix.
---
Nitpick comments:
In `@apps/desktop/src/main/terminal-host/session.test.ts`:
- Around line 94-96: The explicit "| undefined" in the cast for spawnPayload is
redundant because JSON.parse never returns undefined; update the cast on the
parsed value to a concrete type (e.g., change "as { args?: string[] } |
undefined" to "as { args?: string[] }") so spawnPayload has the correct
tightened type; locate the expression that constructs spawnPayload (the
JSON.parse(...) assigned to spawnPayload) and remove the "| undefined" branch
from its type assertion.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
5 tasks
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
1 participant
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.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
Bug Fixes
Refactor
Performance
Tests
Stability
Chores