fix: use selected provider for git commit/PR/changelog generation#34
Open
fix: use selected provider for git commit/PR/changelog generation#34
Conversation
📝 WalkthroughWalkthroughAdds optional provider-aware text generation to support both Codex and GitHub Copilot. Backend: introduces Copilot-specific constants, error normalization, JSON extraction helpers, and a parallel Copilot execution flow wired into existing commit/PR/branch generation paths. Services and GitManager thread an optional provider through generation calls. Frontend: passes activeProvider into ChatHeader and GitActionsControl, persists provider with pending actions, and includes it in mutation payloads and React Query types. Contracts: stacked-action RPC input gains an optional provider field. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/git/Layers/CodexTextGeneration.ts (1)
623-625:⚠️ Potential issue | 🟠 MajorCopilot branch-name generation silently drops the actual image inputs.
materializeImageAttachments()still resolves image paths before provider selection, but only the Codex branch passes them downstream. Wheninput.provider === "copilot", the prompt still says images are primary context even though the model only gets attachment metadata, so screenshot-driven branch naming will regress on the new provider. Either send the images to Copilot too, or fail/fallback when attachments are present.As per coding guidelines,
apps/server/**: Prioritize correctness, reliability, and predictable behavior under reconnects, session restarts, partial streams, and provider failures.Also applies to: 657-671
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/git/Layers/CodexTextGeneration.ts` around lines 623 - 625, When generating branch names, the call to materializeImageAttachments("generateBranchName", input.attachments) resolves and returns imagePaths but those image files are only passed into the Codex path; when input.provider === "copilot" the prompt still references images even though Copilot only receives attachment metadata, causing silent loss of image context. Fix by either sending the actual imagePaths to the Copilot provider path or, if Copilot cannot accept images, reject/fallback when attachments exist: update the generateBranchName flow to check input.provider and imagePaths and (a) include imagePaths in the Copilot request if the Copilot client supports binary/image content, or (b) throw/return a deterministic fallback (e.g., fail with a clear error or route to Codex) when attachments.length > 0 and input.provider === "copilot"; adjust code around materializeImageAttachments, the branch that handles input.provider, and any prompt construction to ensure prompts only claim image context when the chosen provider actually receives the images.
🧹 Nitpick comments (2)
apps/server/src/git/Layers/GitManager.ts (1)
637-637: Prefer a single provider type alias to avoid literal-union drift.The repeated
"codex" | "copilot" | undefinedsignatures are easy to desync over time. Consider deriving one local alias fromGitManagerShape["runStackedAction"]and reusing it.♻️ Suggested refactor
+type GitStackedActionProvider = Parameters<GitManagerShape["runStackedAction"]>[0]["provider"]; + const resolveCommitAndBranchSuggestion = (input: { cwd: string; - provider?: "codex" | "copilot"; + provider?: GitStackedActionProvider; branch: string | null; commitMessage?: string; @@ const runCommitStep = ( cwd: string, - provider: "codex" | "copilot" | undefined, + provider: GitStackedActionProvider, branch: string | null, @@ const runPrStep = ( cwd: string, - provider: "codex" | "copilot" | undefined, + provider: GitStackedActionProvider, fallbackBranch: string | null, ) => @@ const runFeatureBranchStep = ( cwd: string, - provider: "codex" | "copilot" | undefined, + provider: GitStackedActionProvider, branch: string | null,Also applies to: 683-683, 713-713, 981-981
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/git/Layers/GitManager.ts` at line 637, Multiple places declare provider?: "codex" | "copilot" causing drift; create a single local type alias (e.g., type Provider = GitManagerShape["runStackedAction"] extends (args: infer A) => any ? A extends { provider?: infer P } ? P : never : never) and replace all explicit `"codex" | "copilot"` literals with that alias; update the provider properties/signatures for the provider?: fields and parameters used across GitManager (e.g., the provider property on relevant interfaces/objects and the parameters of runStackedAction and any callers) so they all reference the new Provider type alias.apps/server/src/git/Layers/GitManager.test.ts (1)
850-902: Add coverage for the feature-branch provider path too.This regression proves provider propagation into
generateCommitMessageandgeneratePrContent, butgenerateBranchNameis never exercised. A future drop in the feature-branch path would still pass here, so please add a case that runs withfeatureBranch: trueand asserts the selected provider reaches branch-name generation as well.As per coding guidelines,
apps/server/**: Prioritize correctness, reliability, and predictable behavior under reconnects, session restarts, partial streams, and provider failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/git/Layers/GitManager.test.ts` around lines 850 - 902, The test currently checks provider propagation into generateCommitMessage and generatePrContent but never exercises generateBranchName; update the test that uses makeManager and runStackedAction to include featureBranch: true and wire generateBranchName into the textGeneration stub (push the seen provider into seenProviders inside generateBranchName) and assert that seenProviders becomes ["copilot","copilot","copilot"] (or includes the extra "copilot" entry) after runStackedAction; reference the existing symbols generateBranchName, generateCommitMessage, generatePrContent, makeManager, runStackedAction and the seenProviders array when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/git/Layers/CodexTextGeneration.ts`:
- Around line 470-477: The code currently calls client.createSession(...) with
onPermissionRequest: approveAll which unconditionally grants Copilot permission
requests; replace this by implementing mode-aware permission gating similar to
CopilotAdapter: detect the runtime mode/privilege (e.g., full-access vs
read-only/ephemeral) and either (a) suspend and forward permission requests for
explicit user approval in non-full-access modes or (b) reject/deny requests
outright when the session is sandboxed/read-only, instead of using approveAll;
update the createSession call (the client.start()/client.createSession(...)
block and COPILOT_MODEL usage) to pass a permission handler that checks the
current mode and calls the appropriate approve/deny/suspend logic consistent
with CopilotAdapter.
---
Outside diff comments:
In `@apps/server/src/git/Layers/CodexTextGeneration.ts`:
- Around line 623-625: When generating branch names, the call to
materializeImageAttachments("generateBranchName", input.attachments) resolves
and returns imagePaths but those image files are only passed into the Codex
path; when input.provider === "copilot" the prompt still references images even
though Copilot only receives attachment metadata, causing silent loss of image
context. Fix by either sending the actual imagePaths to the Copilot provider
path or, if Copilot cannot accept images, reject/fallback when attachments
exist: update the generateBranchName flow to check input.provider and imagePaths
and (a) include imagePaths in the Copilot request if the Copilot client supports
binary/image content, or (b) throw/return a deterministic fallback (e.g., fail
with a clear error or route to Codex) when attachments.length > 0 and
input.provider === "copilot"; adjust code around materializeImageAttachments,
the branch that handles input.provider, and any prompt construction to ensure
prompts only claim image context when the chosen provider actually receives the
images.
---
Nitpick comments:
In `@apps/server/src/git/Layers/GitManager.test.ts`:
- Around line 850-902: The test currently checks provider propagation into
generateCommitMessage and generatePrContent but never exercises
generateBranchName; update the test that uses makeManager and runStackedAction
to include featureBranch: true and wire generateBranchName into the
textGeneration stub (push the seen provider into seenProviders inside
generateBranchName) and assert that seenProviders becomes
["copilot","copilot","copilot"] (or includes the extra "copilot" entry) after
runStackedAction; reference the existing symbols generateBranchName,
generateCommitMessage, generatePrContent, makeManager, runStackedAction and the
seenProviders array when making the change.
In `@apps/server/src/git/Layers/GitManager.ts`:
- Line 637: Multiple places declare provider?: "codex" | "copilot" causing
drift; create a single local type alias (e.g., type Provider =
GitManagerShape["runStackedAction"] extends (args: infer A) => any ? A extends {
provider?: infer P } ? P : never : never) and replace all explicit `"codex" |
"copilot"` literals with that alias; update the provider properties/signatures
for the provider?: fields and parameters used across GitManager (e.g., the
provider property on relevant interfaces/objects and the parameters of
runStackedAction and any callers) so they all reference the new Provider type
alias.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b656944f-115e-46f7-bdc9-812bdce72935
📒 Files selected for processing (10)
apps/server/src/git/Layers/CodexTextGeneration.tsapps/server/src/git/Layers/GitManager.test.tsapps/server/src/git/Layers/GitManager.tsapps/server/src/git/Services/TextGeneration.tsapps/web/src/components/ChatView.tsxapps/web/src/components/GitActionsControl.tsxapps/web/src/components/chat/ChatHeader.tsxapps/web/src/lib/gitReactQuery.test.tsapps/web/src/lib/gitReactQuery.tspackages/contracts/src/git.ts
Co-authored-by: Capy <capy@capy.ai>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/git/Layers/CodexTextGeneration.ts`:
- Around line 663-677: The Copilot branch-name path currently calls
runCopilotJson without imagePaths, dropping screenshot context; update the logic
in the generateBranchName branch to either pass imagePaths into runCopilotJson
(add imagePaths to the argument object) so Copilot receives attachments, or, if
Copilot cannot accept images yet, detect when imagePaths.length > 0 and fail
fast or explicitly fall back to runCodexJson (or return an error) so you don't
silently degrade output; modify the branch that checks input.provider and the
call to runCopilotJson (and related prompt/outputSchemaJson handling)
accordingly to surface attachments or fail/route predictably.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bb389e1f-c883-49e2-94ca-0b4aae4ca4bc
📒 Files selected for processing (1)
apps/server/src/git/Layers/CodexTextGeneration.ts
Co-authored-by: Capy <capy@capy.ai>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/server/src/git/Layers/CodexTextGeneration.ts`:
- Around line 171-191: withPromiseTimeout currently only rejects the wrapper
promise on timeout and doesn't stop the underlying Copilot work; update it so
the timeout path explicitly cancels the in-flight operation (e.g., invoke the
Copilot cancellation/stop API or abort callback) before rejecting. Specifically,
change withPromiseTimeout to call a cancellation function (provided via the
existing onTimeout or an additional parameter) that invokes
client.start()/createSession()/sendAndWait() cleanup (for example calling
session.cancel() or client.stop() or triggering an AbortSignal) and wait for
that cancellation to be initiated (but not indefinitely) before calling
reject(onTimeout()); apply the same change to the other instance referenced
around lines 498-530 so every timeout both rejects and actively interrupts the
in-flight Copilot session.
- Around line 510-523: Wrap the call to session.sendAndWait(...) in a try-catch
inside the function/method where COPILOT_TIMEOUT_MS is used; on success proceed
as before, but in the catch block call session.getMessages().catch(() => [] as
SessionEvent[]) and run extractLastCopilotAssistantText(history) to try to
recover assistantText from the session history, returning it if non-empty, and
only rethrow the original error (or throw a new error) if history-based recovery
yields no assistant content; ensure you preserve the existing logic that trims
content and checks assistantText.length === 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 407cd84e-e75b-48fc-a05a-613857b90def
📒 Files selected for processing (1)
apps/server/src/git/Layers/CodexTextGeneration.ts
Co-authored-by: Capy <capy@capy.ai>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/server/src/git/Layers/CodexTextGeneration.ts (1)
576-579:⚠️ Potential issue | 🟠 MajorBound teardown awaits in
finallyto preserve timeout guarantees.On timeout/failure paths, Line 577 and Line 578 still await cleanup without a deadline. If Copilot teardown hangs, request completion can exceed
COPILOT_TIMEOUT_MS.⏱️ Suggested fix
} finally { - await session?.destroy().catch(() => undefined); - await client.stop().catch(() => []); + await Promise.race([ + (session?.destroy().catch(() => undefined) ?? Promise.resolve(undefined)), + new Promise<void>((resolve) => + setTimeout(resolve, COPILOT_TIMEOUT_CANCELLATION_GRACE_MS), + ), + ]); + await Promise.race([ + client.stop().catch(() => undefined), + new Promise<void>((resolve) => + setTimeout(resolve, COPILOT_TIMEOUT_CANCELLATION_GRACE_MS), + ), + ]); }As per coding guidelines, "Prioritize correctness, reliability, and predictable behavior under reconnects, session restarts, partial streams, and provider failures."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/git/Layers/CodexTextGeneration.ts` around lines 576 - 579, The finally block currently awaits session?.destroy() and client.stop() with no deadline, which can let a hanging teardown exceed COPILOT_TIMEOUT_MS; change the teardown to use bounded awaits (e.g., Promise.race with a timeout based on COPILOT_TIMEOUT_MS or an AbortController) so both session?.destroy() and client.stop() are given a short, shared deadline and then ignored/caught if they time out; ensure you still call both teardown functions (session?.destroy and client.stop), catch and swallow/ log errors, and do not block the request beyond the configured COPILOT_TIMEOUT_MS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/server/src/git/Layers/CodexTextGeneration.ts`:
- Around line 576-579: The finally block currently awaits session?.destroy() and
client.stop() with no deadline, which can let a hanging teardown exceed
COPILOT_TIMEOUT_MS; change the teardown to use bounded awaits (e.g.,
Promise.race with a timeout based on COPILOT_TIMEOUT_MS or an AbortController)
so both session?.destroy() and client.stop() are given a short, shared deadline
and then ignored/caught if they time out; ensure you still call both teardown
functions (session?.destroy and client.stop), catch and swallow/ log errors, and
do not block the request beyond the configured COPILOT_TIMEOUT_MS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 441f6430-aca9-4c8b-a7d4-0eb743b692bd
📒 Files selected for processing (1)
apps/server/src/git/Layers/CodexTextGeneration.ts
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
1 participant
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.
This PR fixes issue #33 where auto-generated git commit messages and PR content always used Codex, regardless of the user's selected provider. It threads the selected provider from the UI through the git action pipeline to the text generation service, enabling Copilot to be used when selected.
provider?: ProviderKindtoGitRunStackedActionInputinpackages/contracts/src/git.ts.TextGenerationservice interfaces to acceptproviderinapps/server/src/git/Services/TextGeneration.ts.GitManagerinapps/server/src/git/Layers/GitManager.tsto forwardproviderto commit, feature branch, and PR generation steps.runCopilotJsoninapps/server/src/git/Layers/CodexTextGeneration.tsusing the Copilot SDK withapproveAllpermissions andsendAndWaitfor structured output.runCopilotJsonandrunCodexJsonbased oninput.provider.activeProviderthrough UI components inapps/web/src/components/GitActionsControl.tsx,ChatView.tsx, andchat/ChatHeader.tsx.gitRunStackedActionMutationOptionsplumbing inapps/web/src/lib/gitReactQuery.ts.apps/server/src/git/Layers/GitManager.test.tsandapps/web/src/lib/gitReactQuery.test.ts.Summary by CodeRabbit
New Features
Tests