Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances the tmux pane unblocking mechanism by implementing snapshot-based change detection and increasing the polling limit to better handle delayed prompts. It also includes a new test case for delayed dev-channel prompts. The review feedback recommends refactoring the test file to use a helper function for shared setup, which would reduce boilerplate and improve maintainability across the runInTmux tests.
| test("runInTmux catches a delayed Claude dev-channel prompt", 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; | ||
| return pollCount === 5 ? devChannelsPrompt : ""; | ||
| }, | ||
| 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", | ||
| }); | ||
| }); |
There was a problem hiding this comment.
There's a lot of repeated setup code across the runInTmux tests. To improve maintainability and reduce boilerplate, consider creating a test helper function. This function could encapsulate the common setup for manifest, storage, and the default mocked dependencies for runInTmux.
Each test could then call this helper and override only the specific mocks it needs, like capturePane or sendKeys.
For example, you could have a helper like this:
const setupPairedRunTest = (
overrides: Partial<Parameters<typeof runInTmux>[1]> = {}
) => {
const keyCalls: Array<{ keys: string[]; pane: string }> = [];
let sessionStarted = false;
const manifest = createRunManifest({ ... });
const storage = { ... };
const defaultDeps = {
capturePane: () => "",
cwd: "/repo",
env: {},
findBinary: () => true,
sendKeys: (pane: string, keys: string[]) => {
keyCalls.push({ keys, pane });
},
spawn: (args: string[]) => {
},
...overrides,
};
const run = (
args: string[] = ["--tmux", "--proof", "verify with tests"],
launch?: PairedTmuxLaunch
) => runInTmux(args, defaultDeps, launch ?? { opts: makePairedOptions(), task: "Ship feature" });
return { run, keyCalls, manifest, storage };
};
Summary
Verification