fix(review): forward installation token, post inline findings, and stream progress (#57)

Author: chrisleekr

docs/BOT-WORKFLOWS.md

@@ -104,10 +104,11 @@ Composite workflows like `ship` insert a child row per step. When the child comp
   - `[major]` — should fix before merge (likely bug, missing test)
   - `[minor]` — nice to fix (readability)
   - `[nit]` — taste, optional
-- **Outputs**: `state.head_sha`, `state.changed_files`, `state.additions`, `state.deletions`, `state.branch_state` (`{ commits_behind_base, commits_ahead_of_base, is_fork }`), `state.report` (full `REVIEW.md` — Summary / What was checked / Findings / Reasoning), `state.costUsd`, `state.turns`. The agent posts findings as a single GitHub Review (`event: "COMMENT"`) with inline `comments[]` via `gh api`.
+- **Outputs**: `state.head_sha`, `state.changed_files`, `state.additions`, `state.deletions`, `state.branch_state` (`{ commits_behind_base, commits_ahead_of_base, is_fork }`), `state.report` (full `REVIEW.md` — Summary / What was checked / Findings / Reasoning), `state.costUsd`, `state.turns`. The agent posts each finding as a separate inline comment via `mcp__github_inline_comment__create_inline_comment` (one MCP call per finding) — never as a single bundled review POST. This guarantees each finding lands on the right line with its own resolvable thread, instead of one wall-of-text comment at the top of the PR.
+- **Progress visibility**: the handler seeds the tracking comment (via `setState` + `tryReserveTrackingCommentId`) before invoking the pipeline and threads the reserved id into `RunPipelineOverrides.trackingCommentId`. The agent updates the same comment via `mcp__github_comment__update_claude_comment` at five `[update tracking comment]` checkpoints in the prompt — branch refresh, file walk, finding count, posting findings, final REVIEW.md — so the user sees live progress instead of waiting blind for a final report. Without the seed/handoff, `pipeline.ts` would create-and-finalize its own comment and the handler's mid-run updates would target the wrong id; with it, the handler owns the comment lifecycle end-to-end.
 - **No-findings case**: the agent MUST still post a top-level review body listing exactly what was checked (files read in full, classes of issue scanned, tests run) and why no issues were flagged. Silence looks indistinguishable from "didn't actually look."
 - **Branch refresh**: if the PR head is behind base AND not on a fork, the agent rebases onto base, resolves conflicts honestly (reads the surrounding code, runs typecheck + tests, doesn't take ours/theirs blindly), and force-pushes with `--force-with-lease`. Fork PRs get a comment asking the contributor to rebase. See `src/workflows/handlers/branch-refresh.ts`.
-- **Stop conditions**: agent writes `REVIEW.md`; pipeline reports success. Handler NEVER calls `pulls.merge` and NEVER posts an `APPROVE` or `REQUEST_CHANGES` review (FR-017 — those are human prerogatives).
+- **Stop conditions**: agent writes `REVIEW.md`; pipeline reports success. **Push policy**: the only acceptable push from `review` is `git push --force-with-lease` after a clean rebase onto base (same diff, fresh head SHA — see Branch refresh above). The handler never creates commits with code changes (those belong to `implement` / `resolve`), never calls `pulls.merge`, and never posts an `APPROVE` or `REQUEST_CHANGES` review (FR-017 — those are human prerogatives).
 - **Example trigger**: add label `bot:review` on the PR, or comment "`@chrisleekr-bot review this PR`"
 
 ### resolve
@@ -117,6 +118,7 @@ Composite workflows like `ship` insert a child row per step. When the child comp
 - **Inputs**: PR title, failing check names, count of open top-level review comments, branch-staleness diagnostics.
 - **Method**: classify each open reviewer comment (Valid / Partially Valid / Invalid / Needs Clarification), fix valid ones with new commits, reply to all four classes appropriately. Fix failing CI when there is a clear root cause. Refresh the branch first if it's stale (same logic as `review`).
 - **Outputs**: `state.failing_checks`, `state.top_level_comments`, `state.branch_state`, `state.report` (full `RESOLVE.md` body — Summary / CI status / Review comments / Commits pushed / Outstanding), `state.costUsd`, `state.turns`. The agent is asked to write `RESOLVE.md` before finishing; the handler captures it pre-cleanup and embeds it in the tracking comment.
+- **Progress visibility**: same seed/handoff pattern as `review` — the handler `setState`s a "Resolve starting" message before the pipeline, hands the reserved tracking-comment id to `RunPipelineOverrides.trackingCommentId`, and the agent posts `[update tracking comment]` checkpoints at branch-refresh, CI-fix, comment-classification, and final RESOLVE.md steps. Reviewer-thread replies are posted via `gh api repos/<owner>/<repo>/pulls/<num>/comments/<id>/replies -X POST` (the bot's `gh` and `git` calls authenticate via `GH_TOKEN` / `GITHUB_TOKEN` injected from the GitHub App installation token by `buildProviderEnv` in `src/core/executor.ts`).
 - **Stop conditions** (from `src/workflows/handlers/resolve.ts`):
   - `FIX_ATTEMPTS_CAP = 3` — max consecutive CI-fix attempts per PR
   - `POLL_WAIT_SECS_CAP = 900` — 15-minute reviewer-patience window

src/core/executor.ts

@@ -26,12 +26,21 @@ function isResultMessage(msg: unknown): msg is SDKResultMessage {
  * The Claude CLI subprocess picks between them via its own documented auth
  * precedence chain (API key at position 3 beats OAuth token at position 5).
  * See https://code.claude.com/docs/en/authentication#authentication-precedence
+ *
+ * When `installationToken` is supplied, it is exported as both `GH_TOKEN` and
+ * `GITHUB_TOKEN` so the agent's shell `gh`/`git` calls authenticate as the
+ * GitHub App installation. MCP servers receive the same token via their own
+ * env mapping in `src/mcp/registry.ts`.
  */
-function buildProviderEnv(): Record<string, string | undefined> {
+function buildProviderEnv(installationToken?: string): Record<string, string | undefined> {
+  const tokenEnv: Record<string, string> =
+    installationToken !== undefined && installationToken !== ""
+      ? { GH_TOKEN: installationToken, GITHUB_TOKEN: installationToken }
+      : {};
   if (config.provider === "bedrock") {
-    return { ...process.env, CLAUDE_CODE_USE_BEDROCK: "1" };
+    return { ...process.env, ...tokenEnv, CLAUDE_CODE_USE_BEDROCK: "1" };
   }
-  return { ...process.env };
+  return { ...process.env, ...tokenEnv };
 }
 
 /**
@@ -51,6 +60,7 @@ export interface ExecuteAgentParams {
   workDir: string;
   allowedTools: string[];
   maxTurns?: number;
+  installationToken?: string;
 }
 
 export async function executeAgent({
@@ -60,6 +70,7 @@ export async function executeAgent({
   workDir,
   allowedTools,
   maxTurns,
+  installationToken,
 }: ExecuteAgentParams): Promise<ExecutionResult> {
   const { log } = ctx;
 
@@ -89,7 +100,7 @@ export async function executeAgent({
     allowedTools,
     mcpServers,
     systemPrompt: { type: "preset", preset: "claude_code" },
-    env: buildProviderEnv(),
+    env: buildProviderEnv(installationToken),
   };
   const resolvedMaxTurns = maxTurns ?? config.agentMaxTurns;
   if (resolvedMaxTurns !== undefined) {

src/core/pipeline.ts

@@ -117,6 +117,15 @@ export interface RunPipelineOverrides {
    * tracking comment without duplicating the pipeline machinery.
    */
   captureFiles?: string[];
+  /**
+   * Pre-existing tracking comment id (typically created by a workflow
+   * handler via `setState` before invoking the pipeline). When set, the
+   * pipeline does NOT call `createTrackingComment`/`finalizeTrackingComment`
+   * — the orchestrator's tracking-mirror owns the comment lifecycle, and
+   * the pipeline only wires the id into the prompt + MCP server so the
+   * agent can post mid-run progress via `update_claude_comment`.
+   */
+  trackingCommentId?: number;
 }
 
 /**
@@ -158,9 +167,20 @@ export async function runPipeline(
   overrides: RunPipelineOverrides = {},
 ): Promise<ExecutionResult> {
   let trackingCommentId: number | undefined;
+  // When the caller (workflow handler) seeded the tracking comment, the
+  // pipeline must NOT finalize it — the handler's terminal `setState` writes
+  // the final body via tracking-mirror, and a pipeline finalize would
+  // overwrite it with the legacy "completed" template.
+  const callerOwnsTrackingComment = overrides.trackingCommentId !== undefined;
 
   try {
-    if (ctx.skipTrackingComments === true) {
+    if (callerOwnsTrackingComment) {
+      trackingCommentId = overrides.trackingCommentId;
+      ctx.log.info(
+        { trackingCommentId },
+        "Using caller-supplied tracking comment (workflow handler owns lifecycle)",
+      );
+    } else if (ctx.skipTrackingComments === true) {
       ctx.log.info("Skipping tracking comment (skipTrackingComments)");
     } else {
       trackingCommentId = await retryWithBackoff(() => createTrackingComment(ctx), {
@@ -222,10 +242,11 @@ export async function runPipeline(
         mcpServers,
         workDir,
         allowedTools,
+        installationToken,
         ...(overrides.maxTurns !== undefined ? { maxTurns: overrides.maxTurns } : {}),
       });
 
-      if (resolvedTrackingCommentId !== undefined) {
+      if (resolvedTrackingCommentId !== undefined && !callerOwnsTrackingComment) {
         try {
           const finalOpts = buildFinalOpts(result);
           await retryWithBackoff(
@@ -279,7 +300,7 @@ export async function runPipeline(
     const err = error instanceof Error ? error : new Error(String(error));
     ctx.log.error({ err }, "Request processing failed");
 
-    if (trackingCommentId !== undefined) {
+    if (trackingCommentId !== undefined && !callerOwnsTrackingComment) {
       const commentId = trackingCommentId;
       try {
         await retryWithBackoff(

src/workflows/handlers/resolve.ts

@@ -1,6 +1,7 @@
 import { runPipeline } from "../../core/pipeline";
 import type { BotContext } from "../../types";
 import type { WorkflowHandler } from "../registry";
+import { findById } from "../runs-store";
 import { type BranchStaleness, formatRefreshDirective, getBranchStaleness } from "./branch-refresh";
 
 /**
@@ -96,6 +97,22 @@ export const handler: WorkflowHandler = async (ctx) => {
 
     const staleness = await getBranchStaleness(octokit, target.owner, target.repo, target.number);
 
+    // Seed the tracking comment up front so the agent can post mid-run
+    // progress against it. See review.ts for the same pattern + rationale.
+    await ctx.setState(
+      {
+        pr_number: target.number,
+        failing_checks: failingChecks,
+        top_level_comments: topLevelComments.length,
+      },
+      `🔎 **Resolve starting** — ${String(failingChecks.length)} failing checks, ${String(topLevelComments.length)} open comment threads. Refreshing branch and classifying feedback…`,
+    );
+    const seededRow = await findById(runId);
+    const trackingCommentId = seededRow?.tracking_comment_id ?? undefined;
+    if (trackingCommentId === undefined || trackingCommentId === null) {
+      log.warn({ runId }, "resolve handler: tracking comment id not found after seed setState");
+    }
+
     const triggerBody = buildResolvePrompt({
       prNumber: target.number,
       prTitle: pr.title,
@@ -121,12 +138,16 @@ export const handler: WorkflowHandler = async (ctx) => {
       headBranch: pr.head.ref,
       baseBranch: pr.base.ref,
       labels: [],
-      skipTrackingComments: true,
       octokit,
       log,
     };
 
-    const result = await runPipeline(botCtx, { captureFiles: ["RESOLVE.md"] });
+    const result = await runPipeline(botCtx, {
+      captureFiles: ["RESOLVE.md"],
+      ...(trackingCommentId !== undefined && trackingCommentId !== null
+        ? { trackingCommentId }
+        : {}),
+    });
     if (!result.success) {
       return { status: "failed", reason: "resolve pipeline execution failed" };
     }
@@ -202,20 +223,29 @@ function buildResolvePrompt(input: {
     ``,
     formatRefreshDirective(input.staleness),
     ``,
+    `## Tools you MUST use`,
+    `- \`mcp__github_comment__update_claude_comment\` — refresh the tracking comment at every checkpoint marked **[update tracking comment]** so the user sees live progress.`,
+    `- \`Bash\` (\`gh\`, \`git\`) — \`gh\` and \`git\` are pre-authenticated as the GitHub App installation in this environment, so \`gh pr view\`, \`gh run view --log-failed\`, \`gh api .../pulls/comments/<id>/replies\`, \`git commit\`, \`git push\` all work without further setup.`,
+    `- \`Read\`, \`Edit\`, \`Grep\`, \`Glob\` — for code edits and exploration.`,
+    ``,
     `Do the following in order:`,
-    `0. **Refresh the branch first if needed** (see "Branch state" above). A senior engineer rebases before triaging anything; resolving feedback against a stale branch is wasted work.`,
-    `1. If failing checks exist, fetch their logs (gh run view --log-failed) and classify the failure. If it's a test / lint / type / build failure with a clear root cause, attempt one fix — diagnose, edit, commit, push. Do NOT retry more than once per run (FIX_ATTEMPTS_CAP=3 is the per-iteration cap; cross-run enforcement is not yet wired).`,
-    `2. For each open comment thread, classify into one of: Valid | Partially Valid | Invalid | Needs Clarification. The count above is an upper bound — some threads may already be resolved, in which case skip them. For Valid/Partially Valid: fix the code, commit, push, and reply to the comment with the commit SHA and a one-sentence explanation. For Invalid: reply with evidence-backed explanation, no code changes. For Needs Clarification: reply asking the specific question needed to proceed.`,
-    `3. If all checks pass AND all comments resolved AND reviewDecision is APPROVED, post a one-line "review complete — ready to merge" comment.`,
-    `4. NEVER call \`gh pr merge\` or \`octokit.pulls.merge\`. Merging is a human action (FR-017).`,
-    `5. NEVER push to the base branch ${input.baseBranch}.`,
-    `6. Before finishing, write \`RESOLVE.md\` at the repo root summarizing this resolve iteration.`,
+    `0. **[update tracking comment]** Post: "🔎 Resolve — refreshing branch (if needed)."`,
+    `1. **Refresh the branch first if needed** (see "Branch state" above). A senior engineer rebases before triaging anything; resolving feedback against a stale branch is wasted work.`,
+    `2. **[update tracking comment]** Post: "🔎 Resolve — diagnosing N failing checks." (replace N; skip this step if N=0)`,
+    `3. If failing checks exist, fetch their logs (\`gh run view --log-failed\`) and classify the failure. If it's a test / lint / type / build failure with a clear root cause, attempt one fix — diagnose, edit, commit, push. Do NOT retry more than once per run (FIX_ATTEMPTS_CAP=3 is the per-iteration cap; cross-run enforcement is not yet wired).`,
+    `4. **[update tracking comment]** Post: "🔎 Resolve — classifying K open comment threads." (replace K; skip if K=0)`,
+    `5. For each open comment thread, classify into one of: Valid | Partially Valid | Invalid | Needs Clarification. The count above is an upper bound — some threads may already be resolved, in which case skip them. For Valid/Partially Valid: fix the code, commit, push, and reply to the comment via \`gh api repos/OWNER/REPO/pulls/${String(input.prNumber)}/comments/<comment_id>/replies -X POST -f body="..."\` with the commit SHA and a one-sentence explanation. For Invalid: reply with evidence-backed explanation, no code changes. For Needs Clarification: reply asking the specific question needed to proceed.`,
+    `6. If all checks pass AND all comments resolved AND reviewDecision is APPROVED, post a one-line "review complete — ready to merge" comment via \`update_claude_comment\`.`,
+    `7. NEVER call \`gh pr merge\` or \`octokit.pulls.merge\`. Merging is a human action (FR-017).`,
+    `8. NEVER push to the base branch ${input.baseBranch}.`,
+    `9. **Before finishing**, write \`RESOLVE.md\` at the repo root summarizing this resolve iteration.`,
     `   Required sections:`,
     `   ## Summary — one paragraph: what state the PR is in now and what's left.`,
     `   ## CI status — list each failing check and what you did about it.`,
     `   ## Review comments — for each comment: classification, action taken, commit/reply link.`,
     `   ## Commits pushed — sha · subject.`,
     `   ## Outstanding — what still blocks merge (if anything).`,
-    `   This becomes the tracking comment body — be specific, cite files and links.`,
+    `   This becomes the final tracking comment body — be specific, cite files and links.`,
+    `10. **[update tracking comment]** Final: paste the full RESOLVE.md contents.`,
   ].join("\n");
 }