Clean up stale tmux MCP bridge state#38
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the stability and resource management of the tmux bridge by introducing robust mechanisms to automatically detect and clean up stale tmux session data and associated Claude MCP servers. It also refines the interaction with Claude's startup prompts within tmux, ensuring smoother and more reliable automated operations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces functionality to automatically clean up stale tmux sessions and associated Claude channel servers, improving the robustness of the bridge. It also enhances the handling of Claude startup prompts within tmux, specifically for 'Bypass Permissions mode' and 'development channels', by making prompt detection more robust and key sending more reliable. New tests have been added to cover these cleanup and prompt handling scenarios. Feedback suggests logging errors during Claude channel server removal for better debugging and refactoring duplicated test setup code in tmux.test.ts for improved readability and maintainability.
src/loop/bridge.ts
Outdated
| } catch { | ||
| // Cleanup should not fail the bridge flow. | ||
| } |
There was a problem hiding this comment.
Swallowing errors completely can make debugging difficult if an unexpected issue occurs during cleanup (e.g., the claude command is not found). While the cleanup shouldn't block the main flow, logging the error would be beneficial for observability.
Consider adding a log statement to record the error.
} catch (error) {
// Cleanup should not fail the bridge flow, but we should log the error for debugging.
console.error(`[loop] Failed to remove Claude channel server for run ${runId}:`, error);
}| test("runInTmux confirms wrapped Claude dev-channel prompts", async () => { | ||
| const keyCalls: Array<{ keys: string[]; pane: string }> = []; | ||
| let sessionStarted = false; | ||
| let pollCount = 0; | ||
| const devChannelsPrompt = [ | ||
| "WARNING: Loading development channels", | ||
| "", | ||
| "--dangerously-load-development-channels is for local channel development only.", | ||
| "", | ||
| "1. I am using this for local", | ||
| "development", | ||
| ].join("\n"); | ||
| const manifest = createRunManifest({ | ||
| cwd: "/repo", | ||
| mode: "paired", | ||
| pid: 1234, | ||
| repoId: "repo-123", | ||
| runId: "1", | ||
| status: "running", | ||
| }); | ||
| const storage = { | ||
| manifestPath: "/repo/.loop/runs/1/manifest.json", | ||
| repoId: "repo-123", | ||
| runDir: "/repo/.loop/runs/1", | ||
| runId: "1", | ||
| storageRoot: "/repo/.loop/runs", | ||
| transcriptPath: "/repo/.loop/runs/1/transcript.jsonl", | ||
| }; | ||
|
|
||
| await runInTmux( | ||
| ["--tmux", "--proof", "verify with tests"], | ||
| { | ||
| capturePane: () => { | ||
| pollCount += 1; | ||
| if (pollCount === 1) { | ||
| return devChannelsPrompt; | ||
| } | ||
| return ""; | ||
| }, | ||
| cwd: "/repo", | ||
| env: {}, | ||
| findBinary: () => true, | ||
| getCodexAppServerUrl: () => "ws://127.0.0.1:4500", | ||
| getLastCodexThreadId: () => "codex-thread-1", | ||
| isInteractive: () => false, | ||
| launchArgv: ["bun", "/repo/src/cli.ts"], | ||
| log: (): void => undefined, | ||
| makeClaudeSessionId: () => "claude-session-1", | ||
| preparePairedRun: (nextOpts) => { | ||
| nextOpts.codexMcpConfigArgs = [ | ||
| "-c", | ||
| 'mcp_servers.loop-bridge.command="loop"', | ||
| ]; | ||
| return { manifest, storage }; | ||
| }, | ||
| sendKeys: (pane: string, keys: string[]) => { | ||
| keyCalls.push({ keys, pane }); | ||
| }, | ||
| sendText: (): void => undefined, | ||
| sleep: () => Promise.resolve(), | ||
| startCodexProxy: () => Promise.resolve("ws://127.0.0.1:4600/"), | ||
| startPersistentAgentSession: () => Promise.resolve(undefined), | ||
| spawn: (args: string[]) => { | ||
| if (args[0] === "tmux" && args[1] === "has-session") { | ||
| return sessionStarted | ||
| ? { exitCode: 0, stderr: "" } | ||
| : { exitCode: 1, stderr: "" }; | ||
| } | ||
| if (args[0] === "tmux" && args[1] === "new-session") { | ||
| sessionStarted = true; | ||
| } | ||
| return { exitCode: 0, stderr: "" }; | ||
| }, | ||
| updateRunManifest: (_path, update) => update(manifest), | ||
| }, | ||
| { opts: makePairedOptions(), task: "Ship feature" } | ||
| ); | ||
|
|
||
| expect(keyCalls).toContainEqual({ | ||
| keys: ["Enter"], | ||
| pane: "repo-loop-1:0.0", | ||
| }); | ||
| }); | ||
|
|
||
| test("runInTmux confirms the current Claude bypass prompt wording", async () => { | ||
| const keyCalls: Array<{ keys: string[]; pane: string }> = []; | ||
| let sessionStarted = false; | ||
| let pollCount = 0; | ||
| const bypassPrompt = [ | ||
| "Bypass Permissions mode", | ||
| "", | ||
| "1. No, exit", | ||
| "2. Yes, I accept", | ||
| ].join("\n"); | ||
| const manifest = createRunManifest({ | ||
| cwd: "/repo", | ||
| mode: "paired", | ||
| pid: 1234, | ||
| repoId: "repo-123", | ||
| runId: "1", | ||
| status: "running", | ||
| }); | ||
| const storage = { | ||
| manifestPath: "/repo/.loop/runs/1/manifest.json", | ||
| repoId: "repo-123", | ||
| runDir: "/repo/.loop/runs/1", | ||
| runId: "1", | ||
| storageRoot: "/repo/.loop/runs", | ||
| transcriptPath: "/repo/.loop/runs/1/transcript.jsonl", | ||
| }; | ||
|
|
||
| await runInTmux( | ||
| ["--tmux", "--proof", "verify with tests"], | ||
| { | ||
| capturePane: () => { | ||
| pollCount += 1; | ||
| if (pollCount === 1) { | ||
| return bypassPrompt; | ||
| } | ||
| return ""; | ||
| }, | ||
| cwd: "/repo", | ||
| env: {}, | ||
| findBinary: () => true, | ||
| getCodexAppServerUrl: () => "ws://127.0.0.1:4500", | ||
| getLastCodexThreadId: () => "codex-thread-1", | ||
| isInteractive: () => false, | ||
| launchArgv: ["bun", "/repo/src/cli.ts"], | ||
| log: (): void => undefined, | ||
| makeClaudeSessionId: () => "claude-session-1", | ||
| preparePairedRun: (nextOpts) => { | ||
| nextOpts.codexMcpConfigArgs = [ | ||
| "-c", | ||
| 'mcp_servers.loop-bridge.command="loop"', | ||
| ]; | ||
| return { manifest, storage }; | ||
| }, | ||
| sendKeys: (pane: string, keys: string[]) => { | ||
| keyCalls.push({ keys, pane }); | ||
| }, | ||
| sendText: (): void => undefined, | ||
| sleep: () => Promise.resolve(), | ||
| startCodexProxy: () => Promise.resolve("ws://127.0.0.1:4600/"), | ||
| startPersistentAgentSession: () => Promise.resolve(undefined), | ||
| spawn: (args: string[]) => { | ||
| if (args[0] === "tmux" && args[1] === "has-session") { | ||
| return sessionStarted | ||
| ? { exitCode: 0, stderr: "" } | ||
| : { exitCode: 1, stderr: "" }; | ||
| } | ||
| if (args[0] === "tmux" && args[1] === "new-session") { | ||
| sessionStarted = true; | ||
| } | ||
| return { exitCode: 0, stderr: "" }; | ||
| }, | ||
| updateRunManifest: (_path, update) => update(manifest), | ||
| }, | ||
| { opts: makePairedOptions(), task: "Ship feature" } | ||
| ); | ||
|
|
||
| expect(keyCalls).toContainEqual({ | ||
| keys: ["Down"], | ||
| pane: "repo-loop-1:0.0", | ||
| }); | ||
| expect(keyCalls).toContainEqual({ | ||
| keys: ["Enter"], | ||
| pane: "repo-loop-1:0.0", | ||
| }); | ||
| }); |
There was a problem hiding this comment.
There's significant boilerplate duplication across the new tests (runInTmux confirms wrapped Claude dev-channel prompts and runInTmux confirms the current Claude bypass prompt wording) and other tests in this file. This harms readability and maintainability.
Consider extracting the common test setup into a helper function. This function could accept parameters for test-specific logic, like the capturePane mock's return value and the assertions on keyCalls.
For example: const testTmuxPromptHandling = async (promptText, expectedKeyCalls) => { ... };
1 participant
Summary
Testing