Conversation
The orchestrator phase re-did ctx.modelRegistry.find() after resolveTierModel had already resolved the model. Custom providers that re-register their model list asynchronously (e.g. radicalai gateway refresh) could cause the second lookup to fail. Retain the Model<Api> reference from initial resolution instead.
Removes provider/modelId strings that duplicated model.provider/model.id. Updates all call sites to derive from the retained Model reference. Narrows the doc comment to accurately scope the fix.
The post-exec picker popped immediately after the agent presented auto-review discussion findings, before the user could respond. Adds an "awaiting-discussion" phase that holds the picker until a user-initiated turn completes. Also suppresses redundant /review offers (in the picker and in /commit) when auto-review already ran.
There was a problem hiding this comment.
Pull request overview
This PR adjusts the post-execution auto-review flow in the review and modes extensions to (1) avoid a model-registry re-lookup race by retaining the resolved Model reference, and (2) prevent the post-exec picker from appearing before the user has had a chance to respond to surfaced auto-review findings. It also updates the lockfile via npm audit fix.
Changes:
- Retain the resolved
Model<Api>inResolvedTierModeland pass it directly into the orchestrator session (avoids a second registry lookup). - Add an
awaiting-discussionphase inmodesto defer the post-exec picker until after a user-initiated response turn completes, and suppress redundant/reviewoffers when auto-review already ran. - Update
package-lock.jsonwith patched transitive dependency versions.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/review/auto-review.ts | Keeps the resolved model object and threads it through orchestrator/reviewer invocations to avoid registry re-lookup races. |
| packages/modes/index.ts | Introduces awaiting-discussion, adds autoReviewRan suppression logic, and adjusts picker/commit dispatch to avoid redundant review prompts. |
| package-lock.json | Applies dependency updates from npm audit fix. |
Comment on lines
940
to
+941
| pi.on("before_agent_start", async () => { | ||
| userInitiatedTurn = true; |
Comment on lines
+1275
to
+1293
| const hasSurfaced = (result.surfaced?.length ?? 0) > 0; | ||
| const hasAutoApplied = (result.autoApplied?.length ?? 0) > 0; | ||
| if (hasSurfaced || hasAutoApplied) { | ||
| if (modeState) { | ||
| modeState.autoReviewRan = true; | ||
| // Discussion findings need user interaction before | ||
| // the picker pops. Fix-only can proceed immediately. | ||
| modeState.phase = hasSurfaced | ||
| ? "awaiting-discussion" | ||
| : "awaiting-fix"; | ||
| } | ||
| persist(); | ||
| updateWidget(ctx); | ||
| return; | ||
| } | ||
| if (result.ran && modeState) { | ||
| modeState.autoReviewRan = true; | ||
| persist(); | ||
| } |
Comment on lines
27
to
33
| import { readFileSync } from "node:fs"; | ||
| import { dirname, join } from "node:path"; | ||
| import { fileURLToPath } from "node:url"; | ||
| import type { Api, Model } from "@mariozechner/pi-ai"; | ||
| import { | ||
| createAgentSession, | ||
| DefaultResourceLoader, |
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.
Fixes two issues in the auto-review post-execution flow:
Registry re-lookup race — The orchestrator phase called
ctx.modelRegistry.find()afterresolveTierModelhad already resolved the model. Custom providers that re-register their model list asynchronously (e.g. radicalai gateway refresh) could cause the second lookup to fail. Now retains theModel<Api>reference from initial resolution and passes it directly.Picker pops before user responds — The post-exec picker appeared immediately after the agent presented discussion findings, before the user could address them. Adds an
"awaiting-discussion"phase that holds the picker until a user-initiated turn completes.Other changes:
provider/modelIdfields fromResolvedTierModel(derive frommodel.provider/model.id)/reviewoffers in the picker and/commitwhen auto-review already rannpm audit fixfor transitive dependency vulnerabilities