Require worktree isolation for spawned agent teams

[axeldelafosse]

Summary

Update agent-team prompts to explicitly require worktree isolation when teams are spawned, and keep the requirement shared across prompt builders.

Changes

• Added a shared instruction constant for team-spawning prompts.
• Updated work and review prompts to include the required phrase.
• Added test coverage for the new instruction in tests/loop/prompts.test.ts.

Notes

• Prompt behavior for proof/callback instructions remains unchanged.

[gemini-code-assist]

Code Review

The pull request introduces significant changes to the main.ts, pr.ts, prompts.ts, and worktree.ts files, primarily to enhance the agent loop's functionality by incorporating worktree isolation and interactive follow-up prompts. The changes in main.ts refactor the main loop into a runIterations function and introduce an interactive mode for follow-up tasks. pr.ts is updated to handle existing pull requests by prompting for follow-up commits instead of creating new PRs. prompts.ts now includes a constant for worktree isolation phrasing, ensuring consistency across prompts. Finally, worktree.ts is modified to detect existing worktrees and move PLAN.md into the worktree, improving the isolation and setup process. The tests have been updated to reflect these new behaviors, covering the interactive mode, follow-up commits, and worktree detection logic.

I am having trouble creating individual review comments. Click here to see my feedback.

src/loop/main.ts (1)

The createInterface import is added but not used in the runIterations function. It's only used in the runLoop function. It would be better to move this import to where it's actually used to improve modularity and reduce unnecessary imports in runIterations.

src/loop/main.ts (70)

The interactive variable checks for process.stdin.isTTY or process.env.TMUX. While process.stdin.isTTY correctly identifies an interactive terminal, relying on process.env.TMUX might not be universally reliable across all environments or tmux configurations. It might be more robust to check for a specific --interactive flag if interactive behavior is critical, or ensure that TMUX is a consistent indicator in your environment.

src/loop/pr.ts (4-17)

The buildDraftPrPrompt function now takes hasExistingPr as a parameter to conditionally build different prompts. This logic could be slightly cleaner by extracting the two prompt arrays into named constants or separate helper functions, improving readability and maintainability, especially if the prompts become more complex.

const PR_EXISTS_PROMPT_PARTS = [
  "A PR already exists for this branch. Send a follow-up commit to it.",
  (task: string) => `Task context:\n${task.trim()}`,
  "Commit your follow-up changes, push them, and return the commit in your final response.",
];

const NEW_PR_PROMPT_PARTS = [
  "Create a draft GitHub pull request for the current branch.",
  (task: string) => `Task context:\n${task.trim()}`,
  "Use `gh pr create --draft` with a clear title and description.",
  "If a PR already exists for this branch, do not create another one.",
  "Return the PR URL in your final response.",
];

const buildDraftPrPrompt = (task: string, hasExistingPr: boolean): string =>
  hasExistingPr
    ? PR_EXISTS_PROMPT_PARTS.map(part => typeof part === 'function' ? part(task) : part).join("\n\n")
    : NEW_PR_PROMPT_PARTS.map(part => typeof part === 'function' ? part(task) : part).join("\n\n");

src/loop/worktree.ts (1)

The rmSync import is added but it's only used within the moveFile function in defaultDeps. It might be cleaner to define moveFile directly within defaultDeps or ensure all file system operations are grouped logically.

src/loop/worktree.ts (109-111)

The moveFile implementation uses cpSync followed by rmSync. While this works, it's not an atomic operation. If the process crashes between cpSync and rmSync, the file might be duplicated or in an inconsistent state. For critical file movements, consider using fs.renameSync which is atomic, or implement more robust error handling and cleanup.

  moveFile: (source: string, target: string) => {
    renameSync(source, target);
  },

src/loop/worktree.ts (201)

The removal of the deps.pathExists(candidatePath) check here means that the tryCreateWorktree function will now attempt to create a worktree even if the path already exists. This might lead to isWorktreeConflict being triggered more often, which is handled, but it changes the initial check. Ensure this is the intended behavior.