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

[chrisleekr]

Summary

The review workflow ran but never posted inline review comments. Three issues compounded into the same symptom and are fixed together:

1. Installation token wasn't reaching the agent subprocess. buildProviderEnv() in src/core/executor.ts did not export the token, so gh/git calls inside the multi-turn agent ran unauthenticated and silently dropped.
2. Review prompt told the agent to POST a single blob review via gh api .../reviews, producing one wall-of-text comment on the PR instead of per-line findings.
3. Both review and resolve handlers ran with skipTrackingComments: true, so users got no mid-run progress visibility — just a final report (or, in the failure case above, nothing at all).

What changed

• src/core/executor.ts — buildProviderEnv() now accepts an optional installationToken and exports it as GH_TOKEN + GITHUB_TOKEN. ExecuteAgentParams gains the same field.
• src/core/pipeline.ts — runPipeline threads ctx.installationToken into executeAgent. Adds RunPipelineOverrides.trackingCommentId so handlers can seed the tracking comment up front and pass ownership in. When the caller owns the comment, the pipeline skips its own create/finalize calls (both success and error paths).
• src/workflows/handlers/review.ts — Handler now setState(...)s a "Code review starting" message before invoking the pipeline, reads back the reserved tracking comment id via findById, and passes it as trackingCommentId. buildReviewPrompt rewritten to require mcp__github_inline_comment__create_inline_comment per finding (with [blocker]/[major]/[minor]/[nit] severity tags) and explicitly forbids the previous gh api .../reviews blob path. Five [update tracking comment] checkpoints stream progress.
• src/workflows/handlers/resolve.ts — Same seed/handoff pattern with a "🔎 Resolve starting" message. Prompt notes that gh/git are pre-authenticated as the GitHub App installation, lays out a 10-step procedure with checkpoints, and uses gh api .../comments/<id>/replies to reply on review threads.
• test/workflows/handlers/review.test.ts — Mocks runs-store.findById (handler now imports it) and asserts two setState calls: seed (Code review starting…) + finalize (Code review complete…).
• test/workflows/dispatcher.test.ts — Adds findById to the runs-store mock so the dispatcher's transitive handler module loading still resolves.

Verification

End-to-end on PR #54 (refactor/shared-test-factories):

• Review run build(deps): bump the production-dependencies group with 4 updates #2 (after fix): 3 inline [minor] findings posted at the right files/lines via MCP, tracking comment 4319228452 streamed mid-run progress through to final REVIEW.md.
• Resolve run (after fix): bot pushed two commits (fix + RESOLVE.md) and replied to all three review threads. CI green on bot's pushes.
• PR refactor(testing): extract shared test factories #54 ended in MERGEABLE / BLOCKED state — blocked only on human review (per FR-017 the bot never APPROVEs).

Test plan

• bun test test/workflows/handlers/review.test.ts — 7 pass
• bun test test/workflows/dispatcher.test.ts — module loads + all asserts pass
• bash scripts/test-isolated.sh — same pre-existing failures as main (test/db/migrate.test.ts, test/orchestrator/liveness-reaper.test.ts); no regressions from this PR
• Live end-to-end on PR refactor(testing): extract shared test factories #54 (review + resolve) — see Verification above

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Tracking comments now update live during agent execution, providing real-time progress at key checkpoints for both resolve and review workflows.
  • Code review handler now creates inline comments on specific code lines and posts detailed review reports.
  • Agent provides structured status updates throughout the run instead of only showing final results.

[coderabbitai]

Warning

Rate limit exceeded

@chrisleekr has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 30 minutes and 35 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 30 minutes and 35 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2fc63ca3-0fb2-43a8-87d5-57aa944ed154

📥 Commits

Reviewing files that changed from the base of the PR and between 5c50cae and 2278647.

📒 Files selected for processing (2)

• docs/BOT-WORKFLOWS.md
• src/workflows/handlers/review.ts

📝 Walkthrough

Walkthrough

The PR refactors tracking comment lifecycle management to be caller-owned. Workflow handlers now retrieve tracking comment IDs from the runs store and pass them into the pipeline, which conditionally skips automatic comment creation/finalization. The executor accepts an optional installation token for subprocess environment injection, and agent prompts are updated to use MCP tools for live progress updates at explicit checkpoints.

Changes

Cohort / File(s)	Summary
Core Execution Pipeline src/core/executor.ts, src/core/pipeline.ts	Added optional installationToken parameter to executeAgent. Pipeline now supports caller-provided trackingCommentId via RunPipelineOverrides to suppress automatic comment creation/finalization while preserving the ID for agent use.
Workflow Handlers src/workflows/handlers/resolve.ts, src/workflows/handlers/review.ts	Both handlers now seed workflow state early, retrieve tracking_comment_id via findById, and pass it to runPipeline. Agent prompts substantially updated to use MCP tools for creating inline comments and updating tracking comments at explicit checkpoints during execution.
Tests test/workflows/dispatcher.test.ts, test/workflows/handlers/review.test.ts	Added mock for runs-store.findById. Updated review handler tests to validate two setState calls reflecting the seeded-state and final-state pattern, rather than a single call.

Sequence Diagram(s)

sequenceDiagram
    participant Handler as Workflow Handler
    participant Store as Runs Store
    participant Pipeline as runPipeline
    participant Executor as executeAgent
    participant MCP as MCP Tools
    participant GitHub as GitHub API

    Handler->>Handler: Seed workflow state<br/>(PR info, check count, etc.)
    Handler->>Store: findById(runId)
    Store-->>Handler: tracking_comment_id
    Handler->>Pipeline: runPipeline(...)<br/>with trackingCommentId
    Pipeline->>Executor: executeAgent(...)<br/>with installationToken
    Executor->>Executor: buildProviderEnv<br/>inject GH_TOKEN, GITHUB_TOKEN
    Executor->>MCP: Run agent with MCP tools
    MCP->>GitHub: Create inline comments
    MCP->>GitHub: Update tracking comment<br/>at checkpoints
    MCP-->>Executor: Progress updates
    Executor-->>Pipeline: Agent completion
    Pipeline->>Pipeline: Skip createTrackingComment<br/>(caller owns lifecycle)
    Pipeline->>Pipeline: Skip finalizeTrackingComment<br/>(caller owns lifecycle)
    Pipeline-->>Handler: Result with tracking comment ID
    Handler->>Pipeline: Handler-driven finalization<br/>(if needed)

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• feat(workflows): add label-dispatched bot workflow foundation #49: Introduced the workflow runs/tracking comment plumbing via findById and runs store integration that this PR directly extends and operationalizes.
• refactor(workflows): honest verbs, senior-dev review, mandatory triage reproduction #56: Modifies the same handler files and pipeline/executor call signatures, introducing related changes to review and resolve workflow execution patterns.

Suggested labels

type: fix 🐞

Poem

🐰 Tracking comments hop through pipelines bright,
No longer born, now caller-owned outright,
State seeds early, IDs fetch with care,
MCP tools update with live progress to share,
The agent runs free, with tokens to spare! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title concisely captures the three main changes: forwarding the installation token, enabling inline findings posting, and streaming progress via tracking comments.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/review-token-and-progress-visibility

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (4)

src/workflows/handlers/resolve.ts (2)

110-114: Dead === null / !== null branches after ?? undefined narrowing.

seededRow?.tracking_comment_id ?? undefined already collapses both null and undefined (and a missing row) to undefined, so trackingCommentId is typed number | undefined. The subsequent === null / !== null checks are unreachable and just add noise.

♻️ Proposed simplification

     const seededRow = await findById(runId);
     const trackingCommentId = seededRow?.tracking_comment_id ?? undefined;
-    if (trackingCommentId === undefined || trackingCommentId === null) {
+    if (trackingCommentId === undefined) {
       log.warn({ runId }, "resolve handler: tracking comment id not found after seed setState");
     }
@@
     const result = await runPipeline(botCtx, {
       captureFiles: ["RESOLVE.md"],
-      ...(trackingCommentId !== undefined && trackingCommentId !== null
-        ? { trackingCommentId }
-        : {}),
+      ...(trackingCommentId !== undefined ? { trackingCommentId } : {}),
     });

Also applies to: 145-150

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/workflows/handlers/resolve.ts` around lines 110 - 114, The code assigns
trackingCommentId as const trackingCommentId = seededRow?.tracking_comment_id ??
undefined, which already normalizes null to undefined, so remove the unreachable
=== null / !== null branches; instead update the checks in the resolve handler
to only test for undefined (e.g., trackingCommentId === undefined) and adjust
the corresponding log/branch logic accordingly, and apply the same
simplification to the other occurrence that references trackingCommentId later
in this file.

---

238-249: Step 6's "ready to merge" message is immediately overwritten by step 10.

Both step 6 and step 10 call update_claude_comment, and the latter "paste full RESOLVE.md" replaces the body wholesale. The user only ever sees the RESOLVE.md content as the final tracking-comment state, so the transient "ready to merge" line never surfaces. Either fold the merge-readiness signal into a required section of RESOLVE.md (so step 10 carries it), or move the "ready to merge" announcement to a separate channel (top-level PR comment) instead of the same tracking comment that's about to be replaced.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/workflows/handlers/resolve.ts` around lines 238 - 249, The "ready to
merge" message posted via update_claude_comment is being overwritten by the
RESOLVE.md paste in step 10; update the workflow so the merge-readiness signal
survives: either (A) include a clear "Ready to merge" section or flag inside the
RESOLVE.md assembled in step 10 (ensure the code that builds RESOLVE.md adds a
one-line readiness status when reviewDecision === 'APPROVED' and all
checks/comments resolved), or (B) post the one-line readiness as a separate
persistent PR comment instead of the tracking comment (use a new call separate
from update_claude_comment, e.g., post_pr_comment/update_pr_comment or a
distinct update_claude_comment target) so step 10 can still replace the tracking
comment with full RESOLVE.md; locate and change the logic around
update_claude_comment and the RESOLVE.md assembly to implement one of these
options.

src/workflows/handlers/review.ts (1)

71-75: Same dead === null / !== null branches as resolve.ts.

After ?? undefined, trackingCommentId is number | undefined; the null checks are unreachable.

♻️ Proposed simplification

     const seededRow = await findById(runId);
     const trackingCommentId = seededRow?.tracking_comment_id ?? undefined;
-    if (trackingCommentId === undefined || trackingCommentId === null) {
+    if (trackingCommentId === undefined) {
       log.warn({ runId }, "review handler: tracking comment id not found after seed setState");
     }
@@
     const result = await runPipeline(botCtx, {
       captureFiles: ["REVIEW.md"],
-      ...(trackingCommentId !== undefined && trackingCommentId !== null
-        ? { trackingCommentId }
-        : {}),
+      ...(trackingCommentId !== undefined ? { trackingCommentId } : {}),
     });

Also applies to: 108-113

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/workflows/handlers/review.ts` around lines 71 - 75, The code assigns
trackingCommentId via seededRow?.tracking_comment_id ?? undefined which yields a
number | undefined, so the subsequent checks like trackingCommentId === null are
unreachable; change the condition to check only for undefined (e.g., if
(trackingCommentId === undefined) ...) or use a single nullish check (if
(trackingCommentId == null) ...) but do not include both === null and ===
undefined; update the check in the block using findById / seededRow /
trackingCommentId and make the identical simplification in the other occurrence
that mirrors this logic.

test/workflows/handlers/review.test.ts (1)

32-42: LGTM — test mocks and assertions align with the new two-step setState flow.

findById returning a deterministic tracking_comment_id matches the handler's read-back, and the seed/final substring assertions fix the test to the actual call sequence. One small follow-up worth considering: there's no test that exercises the tracking_comment_id == null warn path (handler keeps going without a caller-owned id and the pipeline falls back to createTrackingComment). Optional — not a regression, but it's the only branch in this handler that isn't covered.

Also applies to: 184-194

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/workflows/handlers/review.test.ts` around lines 32 - 42, Add a unit test
in review.test.ts that stubs runs-store.findById to return an object with
tracking_comment_id: null and asserts that the handler logs/warns about missing
tracking id and proceeds to call createTrackingComment; specifically, mock
"../../../src/workflows/runs-store" so findById returns id: "run-1" and
tracking_comment_id: null, spy on or mock createTrackingComment (the function
the handler should call when no tracking id exists), invoke the handler flow
that triggers the read-back, and assert that a warning was emitted and
createTrackingComment was invoked to cover the tracking_comment_id == null
branch.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/workflows/handlers/review.ts`:
- Around line 199-203: The handler currently calls
formatRefreshDirective(input.staleness) but also enforces a "NEVER push commits"
read-only rule, causing conflicting behavior; fix by choosing one of the two
flows: (a) read-only flow — remove/skip calling formatRefreshDirective in review
handler (src/workflows/handlers/review.ts) and instead append a staleness note
to the generated REVIEW.md (use input.staleness and input.isFork to indicate
state) so the review proceeds against current HEAD without any push; or (b)
abort-on-stale flow — detect staleness via input.staleness (and non-fork), emit
a tracking comment telling the user to run bot:resolve and return early (no
push), so only bot:resolve performs force-pushes; implement one of these,
referencing formatRefreshDirective, REVIEW.md, and bot:resolve, and ensure the
handler never initiates pushes to comply with the "NEVER push commits" (FR-017)
rule.

---

Nitpick comments:
In `@src/workflows/handlers/resolve.ts`:
- Around line 110-114: The code assigns trackingCommentId as const
trackingCommentId = seededRow?.tracking_comment_id ?? undefined, which already
normalizes null to undefined, so remove the unreachable === null / !== null
branches; instead update the checks in the resolve handler to only test for
undefined (e.g., trackingCommentId === undefined) and adjust the corresponding
log/branch logic accordingly, and apply the same simplification to the other
occurrence that references trackingCommentId later in this file.
- Around line 238-249: The "ready to merge" message posted via
update_claude_comment is being overwritten by the RESOLVE.md paste in step 10;
update the workflow so the merge-readiness signal survives: either (A) include a
clear "Ready to merge" section or flag inside the RESOLVE.md assembled in step
10 (ensure the code that builds RESOLVE.md adds a one-line readiness status when
reviewDecision === 'APPROVED' and all checks/comments resolved), or (B) post the
one-line readiness as a separate persistent PR comment instead of the tracking
comment (use a new call separate from update_claude_comment, e.g.,
post_pr_comment/update_pr_comment or a distinct update_claude_comment target) so
step 10 can still replace the tracking comment with full RESOLVE.md; locate and
change the logic around update_claude_comment and the RESOLVE.md assembly to
implement one of these options.

In `@src/workflows/handlers/review.ts`:
- Around line 71-75: The code assigns trackingCommentId via
seededRow?.tracking_comment_id ?? undefined which yields a number | undefined,
so the subsequent checks like trackingCommentId === null are unreachable; change
the condition to check only for undefined (e.g., if (trackingCommentId ===
undefined) ...) or use a single nullish check (if (trackingCommentId == null)
...) but do not include both === null and === undefined; update the check in the
block using findById / seededRow / trackingCommentId and make the identical
simplification in the other occurrence that mirrors this logic.

In `@test/workflows/handlers/review.test.ts`:
- Around line 32-42: Add a unit test in review.test.ts that stubs
runs-store.findById to return an object with tracking_comment_id: null and
asserts that the handler logs/warns about missing tracking id and proceeds to
call createTrackingComment; specifically, mock
"../../../src/workflows/runs-store" so findById returns id: "run-1" and
tracking_comment_id: null, spy on or mock createTrackingComment (the function
the handler should call when no tracking id exists), invoke the handler flow
that triggers the read-back, and assert that a warning was emitted and
createTrackingComment was invoked to cover the tracking_comment_id == null
branch.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: bd4bd5f9-5cea-4ddd-b5c2-d108547f704d

📥 Commits

Reviewing files that changed from the base of the PR and between 817e9ae and 5c50cae.

📒 Files selected for processing (6)

• src/core/executor.ts
• src/core/pipeline.ts
• src/workflows/handlers/resolve.ts
• src/workflows/handlers/review.ts
• test/workflows/dispatcher.test.ts
• test/workflows/handlers/review.test.ts

[chrisleekr]

🎉 This PR is included in version 1.3.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀