Add plan review pass and improve Codex streaming output#14
Add plan review pass and improve Codex streaming output#14axeldelafosse merged 1 commit intomainfrom
Conversation
Summary of ChangesHello @axeldelafosse, 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 enhances the Highlights
Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 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
The pull request introduces a new plan review pass and improves the Codex streaming output. The --proof argument is now optional, and a new --review-plan argument has been added to specify a reviewer for the automatic plan review step. The Codex output rendering has been enhanced to better handle raw delta/completed events, preserving nested text and inserting clean message boundaries. The changes also include updates to the README, help text, and expanded tests for the new functionalities. Overall, the changes improve the flexibility and output clarity of the loop tool.
| const reviewer = resolvePlanReviewer(opts.reviewPlan, opts.agent); | ||
| console.log(`\n[loop] reviewing PLAN.md with ${reviewer}.`); | ||
| const reviewPrompt = buildPlanReviewPrompt(task); | ||
| const review = await runAgent(reviewer, reviewPrompt, opts); | ||
| if (review.exitCode !== 0) { | ||
| throw new Error( | ||
| `[loop] plan review ${reviewer} exited with code ${review.exitCode}` | ||
| ); | ||
| } |
There was a problem hiding this comment.
This new block introduces the plan review step after PLAN.md is created. It uses the resolvePlanReviewer to select the reviewer and throws an error if the review fails. This is a critical new feature for improving plan quality.
| test("parseArgs rejects invalid --review-plan value", () => { | ||
| expect(() => | ||
| parseArgs(["--review-plan=claudex", "--proof", "verify"]) | ||
| ).toThrow("Invalid --review-plan value: claudex"); | ||
| }); |
There was a problem hiding this comment.
This test correctly asserts that an invalid value for --review-plan (like claudex) throws an error. This is crucial for maintaining data integrity and providing clear error messages to users.
| test("runAgent inserts pretty-mode blank line after message completion", async () => { | ||
| runCodexTurn.mockImplementation((_prompt, _opts, callbacks) => { | ||
| callbacks.onRaw( | ||
| JSON.stringify({ | ||
| method: "item/agentMessage/delta", | ||
| params: { itemId: "item-1", delta: "I checked the repo." }, | ||
| }) | ||
| ); | ||
| callbacks.onRaw( | ||
| JSON.stringify({ | ||
| method: "item/completed", | ||
| params: { | ||
| item: { | ||
| id: "item-1", | ||
| type: "agentMessage", | ||
| content: [{ text: "I checked the repo." }], | ||
| }, | ||
| }, | ||
| }) | ||
| ); | ||
| callbacks.onRaw( | ||
| JSON.stringify({ | ||
| method: "item/agentMessage/delta", | ||
| params: { itemId: "item-2", delta: "I am updating PLAN.md." }, | ||
| }) | ||
| ); | ||
| return Promise.resolve(makeResult()); | ||
| }); | ||
|
|
||
| const originalWrite = process.stdout.write; | ||
| const writes: string[] = []; | ||
| const writeSpy = mock((chunk: string | Uint8Array): boolean => { | ||
| writes.push( | ||
| typeof chunk === "string" ? chunk : Buffer.from(chunk).toString("utf8") | ||
| ); | ||
| return true; | ||
| }); | ||
| (process.stdout as { write: typeof writeSpy }).write = writeSpy; | ||
|
|
||
| try { | ||
| const result = await runAgent( | ||
| "codex", | ||
| "say hi", | ||
| makeOptions({ format: "pretty" }) | ||
| ); | ||
| expect(result.parsed).toBe("I checked the repo.\nI am updating PLAN.md."); | ||
| expect(writes.join("")).toContain( | ||
| "I checked the repo.\n\nI am updating PLAN.md.\n" | ||
| ); | ||
| } finally { | ||
| process.stdout.write = originalWrite; | ||
| } | ||
| }); |
There was a problem hiding this comment.
This new test case verifies that runAgent correctly inserts a blank line after a message completion in pretty mode. This is a crucial test for the improved Codex streaming output's readability.
| test("runAgent preserves nested delta newline content in pretty mode", async () => { | ||
| runCodexTurn.mockImplementation((_prompt, _opts, callbacks) => { | ||
| callbacks.onRaw( | ||
| JSON.stringify({ | ||
| method: "item/agentMessage/delta", | ||
| params: { | ||
| delta: { | ||
| text: "Heading", | ||
| content: [{ text: "\n- one" }, { text: "\n- two" }], | ||
| }, | ||
| }, | ||
| }) | ||
| ); | ||
| return Promise.resolve(makeResult()); | ||
| }); | ||
|
|
||
| const originalWrite = process.stdout.write; | ||
| const writes: string[] = []; | ||
| const writeSpy = mock((chunk: string | Uint8Array): boolean => { | ||
| writes.push( | ||
| typeof chunk === "string" ? chunk : Buffer.from(chunk).toString("utf8") | ||
| ); | ||
| return true; | ||
| }); | ||
| (process.stdout as { write: typeof writeSpy }).write = writeSpy; | ||
|
|
||
| try { | ||
| const result = await runAgent( | ||
| "codex", | ||
| "say hi", | ||
| makeOptions({ format: "pretty" }) | ||
| ); | ||
| expect(result.parsed).toBe("Heading\n- one\n- two"); | ||
| expect(writes.join("")).toContain("Heading\n- one\n- two\n"); | ||
| } finally { | ||
| process.stdout.write = originalWrite; | ||
| } | ||
| }); |
There was a problem hiding this comment.
This new test case ensures that runAgent correctly preserves nested newline content within delta events in pretty mode. This is important for accurately rendering complex output from Codex.
| test("resolveTask throws when plan review exits non-zero", async () => { | ||
| let calls = 0; | ||
| const { resolveTask } = await loadResolveTask({ | ||
| isFile: (path) => path === "PLAN.md", | ||
| runAgent: () => { | ||
| calls += 1; | ||
| return { | ||
| combined: "", | ||
| exitCode: calls === 1 ? 0 : 2, | ||
| parsed: "", | ||
| }; | ||
| }, | ||
| }); | ||
|
|
||
| await expect(resolveTask(makeOptions("ship feature"))).rejects.toThrow( | ||
| "[loop] plan review claude exited with code 2" | ||
| ); | ||
| }); |
There was a problem hiding this comment.
This new test case verifies that resolveTask correctly throws an error when the plan review step exits with a non-zero code. This is important for robust error handling in the new workflow.
1 participant
Summary
--proofoptional and only inject proof requirements into prompts when non-empty--review-plan [other|claude|codex]plus a plan-review step after plain-text prompt planningTesting
bun run checkbun test