refactor delegation system, add PR summary comments, and improve code quality (#334)

* refactor delegation system and add PR summary comments Delegation system: - replace mode-based delegation with select_mode → delegate two-step flow - orchestrator crafts self-contained subagent prompts (clean context — no system/repo/event instructions leak) - add role-based tool filtering via FastMCP authenticate hook (?role=subagent hides orchestrator-only tools) - add select_mode tool for orchestrator guidance per mode - add ask_question tool for lightweight research subagents - extract shared subagent lifecycle into subagent.ts (create, complete, stdout, instructions) - route set_output to per-subagent state when activeSubagentId is set - track per-subagent state (SubagentState Map) replacing boolean delegationActive flag - capture and aggregate AgentUsage across all agents (claude, codex, gemini, opencode) - write usage summary table to GitHub job summary - block built-in subagent spawning (Task for Claude, Task(*) for Cursor) - increase activity timeout from 60s to 300s (subagent thinking phases) - fix gh CLI misguidance in system prompt — explicitly forbid usage PR summary comments: - add prSummaryComment trigger (DB schema + migrations + Zod + UI toggle) - dispatch mini-effort summary job alongside PR review on pr.created - add update_pull_request_body MCP tool - add defaultEffort option to webhook dispatch Hardening: - rewrite delegate/selectMode tests with simulated state management - add toolFiltering.test.ts for role extraction, canAccess, set_output routing - remove non-null assertions for PULLFROG_TEMP_DIR (proper error throws) - use fetchWithRetry for direct tarball downloads - DRY fix for rate limit check in test runner Co-authored-by: Cursor <cursoragent@cursor.com> * fix: add type keyword to Effort import in handleWebhook.ts Co-authored-by: Cursor <cursoragent@cursor.com> * clean up delegation system, improve code quality across the codebase - simplify delegate tool to instructions + effort params with subagent lifecycle in subagent.ts - add select_mode and ask_question orchestrator-only tools with canAccess filtering - replace delegate.test.ts/selectMode.test.ts with toolFiltering.test.ts (live MCP integration) - add set_output routing for subagent context and AgentUsage tracking across all agents - add PR summary comment trigger (schema, UI, webhook dispatch with silent flag) - add update_pull_request_body MCP tool - fix changed-agents.sh to always include claude canary for non-agent action changes - fix cursor pagination bug in getSelectedInstallationReposPage - remove destructuring patterns, inline type definitions, and unsafe type casts - replace non-null assertions with explicit checks in install.ts - convert multi-param functions to single param objects (postCleanup, runActionLocal, etc.) - use isHttpError helper in API routes instead of catch-any patterns - add adhoc test fixtures for delegation scenarios (context isolation, error handling, synthesis, etc.) Co-authored-by: Cursor <cursoragent@cursor.com> * no subagent mutation, one mcp per subagent * address review feedback: parallel-safe usage tracking, subagent isolation, minor improvements * fix subagent state isolation: replace Object.freeze with shallow copy Object.freeze throws TypeErrors when subagent tools (checkout_pr, report_progress) write scalar properties to toolState. A shallow copy achieves the same isolation for scalar fields while allowing tools to work normally. Shared references (subagents Map, usageEntries array) remain shared for coordination. --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com>
2026-02-22 14:12:43 +00:00
parent a90743e9fe
commit cfd38d82fc
43 changed files with 3293 additions and 2158 deletions
@@ -1,8 +1,8 @@
+// this must be imported first
 import "./arkConfig.ts";
 import { createServer } from "node:net";
-// this must be imported first
 import { FastMCP, type Tool } from "fastmcp";
-import type { Agent } from "../agents/index.ts";
+import type { Agent, AgentUsage } from "../agents/index.ts";
 import { ghPullfrogMcpName } from "../external.ts";
 import type { Mode } from "../modes.ts";
 import type { PrepResult } from "../prep/index.ts";
@@ -21,6 +21,19 @@ export type StoredPushDest = {
  localBranch: string;
 };

+export type SubagentStatus = "running" | "completed" | "failed";
+
+export type SubagentState = {
+  id: string;
+  status: SubagentStatus;
+  mode: string;
+  stdoutFilePath: string;
+  output: string | undefined;
+  usage: AgentUsage | undefined;
+  startedAt: number;
+  keepAliveInterval: ReturnType<typeof setInterval> | undefined;
+};
+
 export interface ToolState {
  // where we're allowed to push - base repo initially, fork URL for fork PRs
  // set by setupGit, updated by checkout_pr. always set before push validation.
@@ -31,8 +44,10 @@ export interface ToolState {
  // issue or PR number (same number space in GitHub)
  issueNumber?: number;
  selectedMode?: string;
-  // true while a subagent is running via the delegate tool — prevents recursive delegation
-  delegationActive: boolean;
+  // per-subagent lifecycle tracking (keyed by subagent uuid)
+  subagents: Map<string, SubagentState>;
+  // set while a subagent is running — routes set_output to the correct subagent and prevents nesting
+  activeSubagentId: string | undefined;
  backgroundProcesses: Map<string, BackgroundProcess>;
  review?: {
    id: number;
@@ -48,6 +63,7 @@ export interface ToolState {
  lastProgressBody?: string;
  wasUpdated?: boolean;
  output?: string;
+  usageEntries: AgentUsage[];
 }

 interface InitToolStateParams {
@@ -56,7 +72,7 @@ interface InitToolStateParams {

 export function initToolState(params: InitToolStateParams): ToolState {
  const parsed = params.progressCommentId ? parseInt(params.progressCommentId, 10) : NaN;
-  const resolvedId = Number.isNaN(parsed) ? undefined : parsed;
+  const resolvedId = Number.isNaN(parsed) || parsed <= 0 ? undefined : parsed;

  if (resolvedId) {
    log.info(`» using pre-created progress comment: ${resolvedId}`);
@@ -64,8 +80,10 @@ export function initToolState(params: InitToolStateParams): ToolState {

  return {
    progressCommentId: resolvedId,
-    delegationActive: false,
+    subagents: new Map(),
+    activeSubagentId: undefined,
    backgroundProcesses: new Map(),
+    usageEntries: [],
  };
 }

@@ -87,8 +105,27 @@ export interface ToolContext {
  tmpdir: string;
 }

+/**
+ * tool names that are only available to the orchestrator.
+ * subagent MCP servers are started with these tools excluded.
+ *
+ * - delegation tools: only the orchestrator can spawn/manage subagents
+ * - remote-mutating tools: subagents work locally; the orchestrator pushes and creates PRs
+ */
+export const ORCHESTRATOR_ONLY_TOOLS = [
+  "select_mode",
+  "delegate",
+  "ask_question",
+  "push_branch",
+  "push_tags",
+  "delete_branch",
+  "create_pull_request",
+  "update_pull_request_body",
+] as const;
+
 import { log } from "../utils/cli.ts";
 import type { RunContextData } from "../utils/runContextData.ts";
+import { AskQuestionTool } from "./askQuestion.ts";
 import { BashTool, KillBackgroundTool } from "./bash.ts";
 import { CheckoutPrTool } from "./checkout.ts";
 import { GetCheckSuiteLogsTool } from "./checkSuite.ts";
@@ -118,7 +155,7 @@ import { GetIssueEventsTool } from "./issueEvents.ts";
 import { IssueInfoTool } from "./issueInfo.ts";
 import { AddLabelsTool } from "./labels.ts";
 import { SetOutputTool } from "./output.ts";
-import { CreatePullRequestTool } from "./pr.ts";
+import { CreatePullRequestTool, UpdatePullRequestBodyTool } from "./pr.ts";
 import { PullRequestInfoTool } from "./prInfo.ts";
 import { CreatePullRequestReviewTool } from "./review.ts";
 import {
@@ -126,6 +163,7 @@ import {
  ListPullRequestReviewsTool,
  ResolveReviewThreadTool,
 } from "./reviewComments.ts";
+import { SelectModeTool } from "./selectMode.ts";
 import { addTools } from "./shared.ts";
 import { UploadFileTool } from "./upload.ts";

@@ -165,9 +203,10 @@ function isAddressInUse(error: unknown): boolean {
  const message = getErrorMessage(error).toLowerCase();
  return message.includes("eaddrinuse") || message.includes("address already in use");
 }
-function buildTools(ctx: ToolContext): Tool<any, any>[] {
+
+// tools shared by both orchestrator and subagent servers
+function buildCommonTools(ctx: ToolContext): Tool<any, any>[] {
  const tools: Tool<any, any>[] = [
-    DelegateTool(ctx),
    StartDependencyInstallationTool(ctx),
    AwaitDependencyInstallationTool(ctx),
    CreateCommentTool(ctx),
@@ -177,7 +216,6 @@ function buildTools(ctx: ToolContext): Tool<any, any>[] {
    IssueInfoTool(ctx),
    GetIssueCommentsTool(ctx),
    GetIssueEventsTool(ctx),
-    CreatePullRequestTool(ctx),
    CreatePullRequestReviewTool(ctx),
    PullRequestInfoTool(ctx),
    CommitInfoTool(ctx),
@@ -187,11 +225,8 @@ function buildTools(ctx: ToolContext): Tool<any, any>[] {
    ResolveReviewThreadTool(ctx),
    GetCheckSuiteLogsTool(ctx),
    AddLabelsTool(ctx),
-    PushBranchTool(ctx),
    GitTool(ctx),
    GitFetchTool(ctx),
-    DeleteBranchTool(ctx),
-    PushTagsTool(ctx),
    UploadFileTool(ctx),
    SetOutputTool(ctx),
    FileReadTool(ctx),
@@ -199,6 +234,7 @@ function buildTools(ctx: ToolContext): Tool<any, any>[] {
    FileEditTool(ctx),
    FileDeleteTool(ctx),
    ListDirectoryTool(ctx),
+    ReportProgressTool(ctx),
  ];

  // only add BashTool when bash is "restricted"
@@ -210,23 +246,41 @@ function buildTools(ctx: ToolContext): Tool<any, any>[] {
    tools.push(KillBackgroundTool(ctx));
  }

-  tools.push(ReportProgressTool(ctx));
-
  return tools;
 }

+// orchestrator gets common tools + delegation + remote-mutating tools
+function buildOrchestratorTools(ctx: ToolContext): Tool<any, any>[] {
+  return [
+    ...buildCommonTools(ctx),
+    SelectModeTool(ctx),
+    DelegateTool(ctx),
+    AskQuestionTool(ctx),
+    PushBranchTool(ctx),
+    PushTagsTool(ctx),
+    DeleteBranchTool(ctx),
+    CreatePullRequestTool(ctx),
+    UpdatePullRequestBodyTool(ctx),
+  ];
+}
+
+// subagent gets only common tools (no delegation, no remote mutation)
+function buildSubagentTools(ctx: ToolContext): Tool<any, any>[] {
+  return buildCommonTools(ctx);
+}
+
 type McpStartResult = {
  server: FastMCP;
  url: string;
  port: number;
 };

-async function tryStartMcpServer(ctx: ToolContext, port: number): Promise<McpStartResult | null> {
-  const server = new FastMCP({
-    name: ghPullfrogMcpName,
-    version: "0.0.1",
-  });
-  const tools = buildTools(ctx);
+async function tryStartMcpServer(
+  ctx: ToolContext,
+  tools: Tool<any, any>[],
+  port: number
+): Promise<McpStartResult | null> {
+  const server = new FastMCP({ name: ghPullfrogMcpName, version: "0.0.1" });
  addTools(ctx, server, tools);

  try {
@@ -253,13 +307,13 @@ async function tryStartMcpServer(ctx: ToolContext, port: number): Promise<McpSta
  }
 }

-async function selectMcpPort(ctx: ToolContext): Promise<McpStartResult> {
+async function selectMcpPort(ctx: ToolContext, tools: Tool<any, any>[]): Promise<McpStartResult> {
  let lastError: unknown = null;

  const requestedPort = readEnvPort();
  if (requestedPort !== null) {
    if (await isPortAvailable(requestedPort)) {
-      const requestedResult = await tryStartMcpServer(ctx, requestedPort);
+      const requestedResult = await tryStartMcpServer(ctx, tools, requestedPort);
      if (requestedResult) {
        return requestedResult;
      }
@@ -275,7 +329,7 @@ async function selectMcpPort(ctx: ToolContext): Promise<McpStartResult> {
      if (!(await isPortAvailable(port))) {
        continue;
      }
-      const result = await tryStartMcpServer(ctx, port);
+      const result = await tryStartMcpServer(ctx, tools, port);
      if (result) {
        return result;
      }
@@ -315,12 +369,13 @@ async function killBackgroundProcesses(toolState: ToolState): Promise<void> {
 }

 /**
- * Start the MCP HTTP server and return the URL and close function
+ * Start the orchestrator MCP HTTP server (has all tools including push/PR/delegation).
 */
 export async function startMcpHttpServer(
  ctx: ToolContext
 ): Promise<{ url: string; [Symbol.asyncDispose]: () => Promise<void> }> {
-  const startResult = await selectMcpPort(ctx);
+  const tools = buildOrchestratorTools(ctx);
+  const startResult = await selectMcpPort(ctx, tools);

  return {
    url: startResult.url,
@@ -330,3 +385,28 @@ export async function startMcpHttpServer(
    },
  };
 }
+
+export type ManagedMcpServer = {
+  url: string;
+  stop: () => Promise<void>;
+};
+
+/**
+ * Start a per-subagent MCP server (common tools only — no push/PR/delegation).
+ * Each subagent gets its own server; call stop() when the subagent completes.
+ *
+ * The subagent gets its own shallow copy of toolState so scalar writes
+ * (pushUrl, pushDest, selectedMode, etc.) don't mutate the orchestrator's state.
+ * Shared references (subagents Map, usageEntries array, dependencyInstallation)
+ * are intentionally shared for coordination (set_output routing, usage tracking).
+ */
+export async function startSubagentMcpServer(ctx: ToolContext): Promise<ManagedMcpServer> {
+  const subagentToolState: ToolState = {
+    ...ctx.toolState,
+    backgroundProcesses: new Map(),
+  };
+  const subagentCtx: ToolContext = { ...ctx, toolState: subagentToolState };
+  const tools = buildSubagentTools(subagentCtx);
+  const startResult = await selectMcpPort(subagentCtx, tools);
+  return { url: startResult.url, stop: () => startResult.server.stop() };
+}