fix(engine): route review via explicit external checkout metadata#1865
Merged
gsxdsm merged 4 commits intoJul 2, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReview-checkout metadata resolution now includes ChangesReview checkout routing
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR tightens review routing in the engine by deriving the reviewer working directory from explicit task metadata (including the new sourceMetadata.externalReviewCheckout) with fail-closed validation, and adds observability + tests to ensure review runs in the intended checkout.
Changes:
- Extend review checkout metadata extraction to include
sourceMetadata.externalReviewCheckoutwith explicit priority ordering and fail-closed behavior. - Add reviewer logging that surfaces when review is routed to an external checkout (and warns when external metadata is present but invalid).
- Add/extend tests covering metadata extraction + routing for both
fn_review_stepand workflowstepReview, plus a changeset.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/engine/src/review-checkout.ts | Adds sourceMetadata as an explicit metadata source and accepts externalReviewCheckout as a candidate field. |
| packages/engine/src/executor.ts | Logs the selected review cwd and warns when external metadata is present but invalid. |
| packages/engine/src/tests/reviewer-workspace.test.ts | Extends workspace/reviewer tests to cover sourceMetadata.externalReviewCheckout for both review seams. |
| packages/engine/src/tests/review-checkout.test.ts | New contract tests for metadata extraction + fail-closed checkout resolution behavior. |
| .changeset/review-checkout-routing.md | Publishes the routing behavior change as a patch fix for @runfusion/fusion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Greptile SummaryThis PR wires
Confidence Score: 5/5Safe to merge — the routing change is strictly additive, fail-closed, and covered by both unit and integration tests. The core logic change is small and self-contained: a new field is recognized, a new priority tier is inserted, and the duplicate logging block is replaced by a helper with a correctly scoped warning condition. All three concerns from the prior review round are addressed. Test coverage explicitly pins the fail-closed invariant across every known invalid-path variant and both executor seams. No correctness gaps identified. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["resolveReviewCheckoutCwd(task, fallbackCwd)"] --> B["getTaskReviewCheckoutPath(task)"]
B --> C{customFields has path?}
C -- yes --> D["candidate = customFields.*"]
C -- no --> E{branchContext has path?}
E -- yes --> F["candidate = branchContext.*"]
E -- no --> G{sourceMetadata has externalReviewCheckout?}
G -- yes --> H["candidate = sourceMetadata.externalReviewCheckout"]
G -- no --> I{root-level path field?}
I -- yes --> J["candidate = task.*"]
I -- no --> K["candidate = undefined → return fallbackCwd"]
D & F & H & J --> L{isAbsolute && exists && isDirectory?}
L -- no --> M["return fallbackCwd"]
L -- yes --> N["git rev-parse --show-toplevel"]
N -- error/empty --> M
N -- success --> O["return realpathSync(topLevel)"]
O --> P["logReviewCheckoutRouting"]
M --> P
P --> Q{reviewCwd != worktreePath?}
Q -- yes --> R["log: routed to external checkout"]
Q -- no --> S{selectedCandidate == sourceMetadata.externalReviewCheckout?}
S -- yes --> T["warn: external metadata present but invalid"]
S -- no --> U["silent fallback"]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["resolveReviewCheckoutCwd(task, fallbackCwd)"] --> B["getTaskReviewCheckoutPath(task)"]
B --> C{customFields has path?}
C -- yes --> D["candidate = customFields.*"]
C -- no --> E{branchContext has path?}
E -- yes --> F["candidate = branchContext.*"]
E -- no --> G{sourceMetadata has externalReviewCheckout?}
G -- yes --> H["candidate = sourceMetadata.externalReviewCheckout"]
G -- no --> I{root-level path field?}
I -- yes --> J["candidate = task.*"]
I -- no --> K["candidate = undefined → return fallbackCwd"]
D & F & H & J --> L{isAbsolute && exists && isDirectory?}
L -- no --> M["return fallbackCwd"]
L -- yes --> N["git rev-parse --show-toplevel"]
N -- error/empty --> M
N -- success --> O["return realpathSync(topLevel)"]
O --> P["logReviewCheckoutRouting"]
M --> P
P --> Q{reviewCwd != worktreePath?}
Q -- yes --> R["log: routed to external checkout"]
Q -- no --> S{selectedCandidate == sourceMetadata.externalReviewCheckout?}
S -- yes --> T["warn: external metadata present but invalid"]
S -- no --> U["silent fallback"]
Reviews (3): Last reviewed commit: "fix(engine): address review checkout rou..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/engine/src/executor.ts (1)
6366-6377: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated review-checkout routing/logging logic.
The routing-visibility block (compute
reviewCwd, compare to the worktree, log the external-checkout route, or warn on invalidsourceMetadata.externalReviewCheckout) is copy-pasted verbatim in both thestepReviewseam andcreateReviewStepTool. Consider extracting a small shared helper (e.g.private logReviewCheckoutRouting(taskId, sourceMetadata, reviewCwd, worktreePath)) to avoid the two copies silently diverging on a future edit.♻️ Proposed extraction
+ private logReviewCheckoutRouting(taskId: string, sourceMetadata: unknown, reviewCwd: string, worktreePath: string): void { + if (reviewCwd !== worktreePath) { + reviewerLog.log(`${taskId}: review routed to external checkout ${reviewCwd} (task worktree: ${worktreePath})`); + return; + } + const sm = sourceMetadata as Record<string, unknown> | undefined; + const hasExternalMeta = sm && typeof sm.externalReviewCheckout === "string" && sm.externalReviewCheckout.trim(); + if (hasExternalMeta) { + reviewerLog.warn(`${taskId}: external review checkout metadata present (${sm!.externalReviewCheckout}) but invalid — reviewing task worktree ${worktreePath}`); + } + }Then both call sites become:
- if (reviewCwd !== worktreePath) { - reviewerLog.log(`${seamTask.id}: review routed to external checkout ${reviewCwd} (task worktree: ${worktreePath})`); - } else { - const sm = detail.sourceMetadata as Record<string, unknown> | undefined; - const hasExternalMeta = sm && typeof sm.externalReviewCheckout === "string" && sm.externalReviewCheckout.trim(); - if (hasExternalMeta) { - reviewerLog.warn(`${seamTask.id}: external review checkout metadata present (${sm!.externalReviewCheckout}) but invalid — reviewing task worktree ${worktreePath}`); - } - } + this.logReviewCheckoutRouting(seamTask.id, detail.sourceMetadata, reviewCwd, worktreePath);Also applies to: 12994-13005
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/executor.ts` around lines 6366 - 6377, The review-checkout routing/logging block is duplicated in both the stepReview seam and createReviewStepTool, so extract the shared logic into a small helper such as logReviewCheckoutRouting on the executor/reviewer path. Move the reviewCwd/worktreePath comparison and the sourceMetadata.externalReviewCheckout validation logging into that helper, then call it from both existing sites so the routing behavior stays consistent and won’t diverge on future edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/engine/src/executor.ts`:
- Around line 6366-6377: The review-checkout routing/logging block is duplicated
in both the stepReview seam and createReviewStepTool, so extract the shared
logic into a small helper such as logReviewCheckoutRouting on the
executor/reviewer path. Move the reviewCwd/worktreePath comparison and the
sourceMetadata.externalReviewCheckout validation logging into that helper, then
call it from both existing sites so the routing behavior stays consistent and
won’t diverge on future edits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bf42558d-ea41-43b0-b575-5f774717b2f8
📒 Files selected for processing (5)
.changeset/review-checkout-routing.mdpackages/engine/src/__tests__/review-checkout.test.tspackages/engine/src/__tests__/reviewer-workspace.test.tspackages/engine/src/executor.tspackages/engine/src/review-checkout.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/cli/src/__tests__/extension-experiment-finalize.test.ts (1)
60-69: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
createWorkflowAuthoringToolsmock returns wrong shape.Real implementation returns
ToolDefinition[], and the extension calls.find(...)on the result. Stubbing it asvi.fn(() => ({}))means any code path reaching workflow tool dispatch in this suite would throwworkflowTools.find is not a function.🔧 Proposed fix
- createWorkflowAuthoringTools: vi.fn(() => ({})), + createWorkflowAuthoringTools: vi.fn(() => []),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/__tests__/extension-experiment-finalize.test.ts` around lines 60 - 69, The `createWorkflowAuthoringTools` mock is returning the wrong type, which breaks any code path that expects workflow tools to behave like a `ToolDefinition[]` and call `.find(...)` on them. Update the test setup in `extension-experiment-finalize.test.ts` so `createWorkflowAuthoringTools` returns an array-shaped mock consistent with the real implementation, using the `createWorkflowAuthoringTools` stub that feeds the extension’s workflow tool dispatch logic.packages/cli/src/__tests__/extension-fn-secret-get.test.ts (1)
16-25: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSame
createWorkflowAuthoringToolsstub shape mismatch as sibling files.Real export returns
ToolDefinition[], and consumers call.find(...)on it (seepackages/engine/src/agent-tools.ts:2404-2419andpackages/cli/src/extension.ts). Returning{}will crash with.find is not a functionif this suite's tools ever reach the workflow-dispatch path.🔧 Proposed fix
- createWorkflowAuthoringTools: vi.fn(() => ({})), + createWorkflowAuthoringTools: vi.fn(() => []),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/__tests__/extension-fn-secret-get.test.ts` around lines 16 - 25, The `createWorkflowAuthoringTools` test stub has the wrong return shape and should match the real `ToolDefinition[]` contract used by `agent-tools` and `extension.ts`. Update the mock in this test to return an array-like tool list (consistent with sibling fixtures) so callers that use `.find(...)` do not fail with a non-function error. Keep the rest of the mocked params unchanged and ensure the stubbed tools expose the fields needed by workflow dispatch.packages/cli/src/__tests__/extension-github-tracking.test.ts (1)
34-43: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSame
createWorkflowAuthoringToolsstub shape mismatch.Should return an array (
ToolDefinition[]) rather than{}, matching the upstream export used by.find(...)calls in the extension.🔧 Proposed fix
- createWorkflowAuthoringTools: vi.fn(() => ({})), + createWorkflowAuthoringTools: vi.fn(() => []),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/__tests__/extension-github-tracking.test.ts` around lines 34 - 43, The `createWorkflowAuthoringTools` stub in the test setup returns an object, but the extension code expects an array and calls `.find(...)` on it. Update the `vi.fn` stub for `createWorkflowAuthoringTools` to return an empty `ToolDefinition[]`-shaped array (or an array of tool definitions) so it matches the real export used by the extension and its `.find(...)` usage.packages/cli/src/__tests__/extension-web-fetch.test.ts (1)
12-21: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSame
createWorkflowAuthoringToolsstub shape mismatch.Consistent with the other three test files — the stub should return
[], not{}, to match the real array-returning export and avoid breaking.find(...)consumers.🔧 Proposed fix
- createWorkflowAuthoringTools: vi.fn(() => ({})), + createWorkflowAuthoringTools: vi.fn(() => []),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/__tests__/extension-web-fetch.test.ts` around lines 12 - 21, The test stub for createWorkflowAuthoringTools is returning the wrong shape and should match the real export’s array result. Update the mock in extension-web-fetch.test.ts so createWorkflowAuthoringTools returns an empty array instead of an object, consistent with the other test files and safe for any .find(...) usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/cli/src/__tests__/extension-experiment-finalize.test.ts`:
- Around line 60-69: The `createWorkflowAuthoringTools` mock is returning the
wrong type, which breaks any code path that expects workflow tools to behave
like a `ToolDefinition[]` and call `.find(...)` on them. Update the test setup
in `extension-experiment-finalize.test.ts` so `createWorkflowAuthoringTools`
returns an array-shaped mock consistent with the real implementation, using the
`createWorkflowAuthoringTools` stub that feeds the extension’s workflow tool
dispatch logic.
In `@packages/cli/src/__tests__/extension-fn-secret-get.test.ts`:
- Around line 16-25: The `createWorkflowAuthoringTools` test stub has the wrong
return shape and should match the real `ToolDefinition[]` contract used by
`agent-tools` and `extension.ts`. Update the mock in this test to return an
array-like tool list (consistent with sibling fixtures) so callers that use
`.find(...)` do not fail with a non-function error. Keep the rest of the mocked
params unchanged and ensure the stubbed tools expose the fields needed by
workflow dispatch.
In `@packages/cli/src/__tests__/extension-github-tracking.test.ts`:
- Around line 34-43: The `createWorkflowAuthoringTools` stub in the test setup
returns an object, but the extension code expects an array and calls
`.find(...)` on it. Update the `vi.fn` stub for `createWorkflowAuthoringTools`
to return an empty `ToolDefinition[]`-shaped array (or an array of tool
definitions) so it matches the real export used by the extension and its
`.find(...)` usage.
In `@packages/cli/src/__tests__/extension-web-fetch.test.ts`:
- Around line 12-21: The test stub for createWorkflowAuthoringTools is returning
the wrong shape and should match the real export’s array result. Update the mock
in extension-web-fetch.test.ts so createWorkflowAuthoringTools returns an empty
array instead of an object, consistent with the other test files and safe for
any .find(...) usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 28a96327-bb1e-48f1-af6d-4751bca776b9
📒 Files selected for processing (4)
packages/cli/src/__tests__/extension-experiment-finalize.test.tspackages/cli/src/__tests__/extension-fn-secret-get.test.tspackages/cli/src/__tests__/extension-github-tracking.test.tspackages/cli/src/__tests__/extension-web-fetch.test.ts
gsxdsm
added a commit
that referenced
this pull request
Jul 2, 2026
Resolves the pnpm-lock.yaml conflict. Main's #1865 (review-checkout routing) auto-merged cleanly with the completion-summary backstop in executor.ts. Main independently pinned pi-claude-cli's pi-ai/pi-coding-agent to ^0.80.3 (e154892) but kept the top-level `getModels` import, which 0.80.3 removed — this branch's migration to `getBuiltinModels` from `/providers/all` is retained as the working fix. Lockfile regenerated against the merged package.json. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
sourceMetadata.externalReviewCheckoutreview routing with fail-closed validation.fn_review_step, and workflowstepReviewrouting.Test Plan
cd packages/engine && corepack pnpm exec vitest run src/__tests__/review-checkout.test.ts src/__tests__/reviewer-workspace.test.tsSummary by CodeRabbit
New Features
Bug Fixes
Tests