Implement Copilot SDK integration and multi-convention commit system #41
Conversation
Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the
Comment |
There was a problem hiding this comment.
Pull request overview
This PR implements GitHub Copilot SDK integration and introduces a multi-convention commit message system with four supported formats: Clean Commit (default), Conventional Commits, Gitmoji, and Simple. The implementation replaces the placeholder Copilot integration with real SDK calls and adds interactive convention switching during the commit process.
Changes:
- Adds pluggable convention system with four pre-configured formats and AI-specific prompts for each
- Implements GitHub Copilot SDK client lifecycle management with automatic OpenAI fallback
- Adds interactive convention switching capability with regeneration loops
- Updates authentication to use GitHub CLI instead of token-based auth
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| source/utils/commit-conventions.js | New file defining four commit conventions with format-specific prompts |
| source/providers/ai-provider.js | Replaces placeholder with real Copilot SDK integration (CopilotClient lifecycle) |
| source/commands/commit.js | Adds regeneration loops with convention switching for file-by-file and single-file workflows |
| source/commands/auth.js | Removes token parameter, validates gh CLI installation and authentication status |
| source/commands/config.js | Adds convention name display in config output |
| source/cli.js | Adds --convention flag and set-convention subcommand with validation |
| source/utils/ui.js | Adds promptConventionSelection and updates confirmCommit to show convention and regeneration option |
| source/utils/config-manager.js | Adds setConvention/getConvention functions with 'clean' default |
| test.js | Adds comprehensive tests for convention system and config management |
Comments suppressed due to low confidence (1)
source/cli.js:64
- Superfluous argument passed to function authenticateWithCopilot.
authenticateWithCopilot(options.token);
|
@copilot apply changes based on the comments in this thread |
Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 10 comments.
Comments suppressed due to low confidence (1)
source/cli.js:65
- Superfluous argument passed to function authenticateWithCopilot.
authenticateWithCopilot(options.token);
source/providers/ai-provider.js
Outdated
|
|
||
| // Create session with model | ||
| const session = await client.createSession({ | ||
| model: this.model || options.model || 'gpt-4.1', |
There was a problem hiding this comment.
The model value 'gpt-4.1' on line 77 may not be a valid OpenAI model identifier. Standard OpenAI model names include 'gpt-4', 'gpt-4-turbo', 'gpt-4o', and 'gpt-3.5-turbo', but not 'gpt-4.1'. This could cause failures when the Copilot SDK or fallback OpenAI provider attempts to use this model.
Consider using 'gpt-4' or 'gpt-4o' as the default, or verify the correct model identifier for the GitHub Copilot SDK.
| model: this.model || options.model || 'gpt-4.1', | |
| model: this.model || options.model || 'gpt-4o', |
There was a problem hiding this comment.
Fixed in commit 5f3d2a6. Changed model from 'gpt-4.1' to 'gpt-4o', which is a valid OpenAI model identifier compatible with both Copilot SDK and OpenAI fallback.
source/providers/ai-provider.js
Outdated
| `GitHub Copilot failed: ${error.message}\n\n` + | ||
| 'Make sure you have:\n' + | ||
| '1. GitHub Copilot subscription\n' + | ||
| '2. Authenticated via: gh auth login\n' + | ||
| '3. GitHub CLI installed and in PATH\n\n' + | ||
| 'Or set OpenAI key as fallback: magicc auth openai <key>', |
There was a problem hiding this comment.
The error message on lines 117-124 provides helpful troubleshooting steps, which is good. However, the message assumes the user needs to authenticate via gh auth login, but they may already be authenticated and the issue could be something else (e.g., no Copilot subscription, network issues, or SDK compatibility problems). Consider making the error message more diagnostic by checking what the actual error type is, or adding a note that if already authenticated, the issue might be elsewhere.
| `GitHub Copilot failed: ${error.message}\n\n` + | |
| 'Make sure you have:\n' + | |
| '1. GitHub Copilot subscription\n' + | |
| '2. Authenticated via: gh auth login\n' + | |
| '3. GitHub CLI installed and in PATH\n\n' + | |
| 'Or set OpenAI key as fallback: magicc auth openai <key>', | |
| `GitHub Copilot failed` + | |
| (error.name ? ` (${error.name})` : '') + | |
| `: ${error.message}\n\n` + | |
| 'This may be caused by:\n' + | |
| '1. Missing GitHub Copilot subscription\n' + | |
| '2. Not being authenticated in GitHub CLI (try: gh auth login)\n' + | |
| '3. Network issues or GitHub API availability problems\n' + | |
| '4. Incompatible or outdated GitHub Copilot SDK or CLI version\n\n' + | |
| 'If the issue persists, you can also set an OpenAI key as a fallback: ' + | |
| 'magicc auth openai <key>', |
There was a problem hiding this comment.
Fixed in commit 5f3d2a6. Enhanced error message to be more diagnostic by including error.name and listing multiple possible causes (missing subscription, not authenticated, network issues, SDK compatibility) rather than assuming authentication is the only issue.
| | Emoji | Type | What it covers | | ||
| |-------|-----------|----------------| |
There was a problem hiding this comment.
The table formatting on lines 15-16 uses pipe characters to create a markdown table header. However, the pipes at the start of line 15 will be interpreted as part of the template string literal, not markdown. When this prompt is sent to the AI, it will receive:
| Emoji | Type | What it covers |
|-------|-----------|----------------|
This is correct for markdown, but if the AI expects plain text formatting, this might not render as intended. Consider verifying that the AI models handle markdown table formatting correctly in prompts, or use plain text formatting for better compatibility.
source/commands/config.js
Outdated
| // Show convention name nicely | ||
| if (key === 'convention') { | ||
| const conv = getConvention(value); | ||
| displayValue = `${value} (${conv.name})`; |
There was a problem hiding this comment.
The convention display logic on lines 36-38 should handle cases where the convention value in config might be invalid or undefined. If a user manually edits their config file and sets an invalid convention name, calling getConvention(value) will return the 'clean' convention as fallback, but the display will show the invalid value alongside "Clean Commit". Consider adding validation or a clearer message when the stored convention is invalid.
| displayValue = `${value} (${conv.name})`; | |
| if (conv && typeof conv.name === 'string') { | |
| // Prefer the resolved convention name; avoid showing invalid raw values | |
| displayValue = conv.name; | |
| } else { | |
| displayValue = value ?? 'Unknown convention'; | |
| } |
There was a problem hiding this comment.
Fixed in commit 5f3d2a6. Added validation in config display to check if stored convention is valid using Object.keys(CONVENTIONS). Invalid conventions now show as "invalid - defaults to [fallback]" to make it clear the value is invalid.
| const validConventions = Object.keys(CONVENTIONS); | ||
| if (!validConventions.includes(type)) { | ||
| showError(`Invalid convention: ${type}`); | ||
| console.log(`Available: ${validConventions.join(', ')}`); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
The validation on lines 96-101 checks if the convention type is valid using Object.keys(CONVENTIONS), which is good. However, this validation is only enforced in the CLI set-convention command. The config-manager's setConvention function doesn't validate input, which means other code paths could potentially set invalid conventions. Consider adding validation in the config-manager itself to ensure consistency across all code paths.
There was a problem hiding this comment.
Fixed in commit 8e01b49. Added validation directly in setConvention() by importing CONVENTIONS and checking against valid convention names. Invalid conventions now trigger a warning and default to 'clean', ensuring consistency across all code paths (CLI commands, config display, and any other code that calls setConvention).
|
@copilot apply changes based on the comments in this thread |
Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
![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 (2)
source/cli.js (1)
63-66:⚠️ Potential issue | 🟡 MinorRemove unused
--tokenoption.The
--tokenoption is passed toauthenticateWithCopilot()but the function no longer accepts parameters (seesource/commands/auth.jsline 22).🔧 Remove the dead code
authCmd .command('copilot') .description('Authenticate with GitHub Copilot') - .option('--token <token>', 'GitHub token') - .action(options => { - authenticateWithCopilot(options.token); + .action(() => { + authenticateWithCopilot(); });source/commands/commit.js (1)
91-106:⚠️ Potential issue | 🟡 MinorConvention support missing in
processAllFiles.Unlike
processFilesInteractivelyandprocessFile, this function:
- Doesn't pass the convention to
generateCommitMessage(line 91)- Doesn't pass the convention to
confirmCommitfor display (line 94)- Doesn't support the regeneration flow when user selects "change-convention"
This means
--allusers won't see convention info or be able to regenerate with different conventions.🔧 Suggested fix to add convention support
export async function processAllFiles(aiProvider, options = {}) { showInfo('Processing all files together...'); // Stage all changes await stageAll(); // Get full diff const diff = await getDiff(); if (!diff) { showWarning('No changes to commit.'); return; } + let currentConvention = options.convention || getConvention(); + let attempts = 0; + const maxAttempts = 5; + + while (attempts < maxAttempts) { - // Generate commit message - showInfo('Generating commit message...'); - const message = await aiProvider.generateCommitMessage(diff, null, options); + showInfo('Generating commit message...'); + const message = await aiProvider.generateCommitMessage(diff, null, { + ...options, + convention: currentConvention, + }); - // Confirm and commit - const result = await confirmCommit(message); + const result = await confirmCommit(message, null, currentConvention); - if (result.action === 'accept') { - const success = await commit(result.message); - if (success) { - showSuccess('Changes committed successfully!'); - console.log(`📝 ${result.message}`); - } else { - showError('Failed to commit changes.'); + if (result.action === 'accept') { + const success = await commit(result.message); + if (success) { + showSuccess('Changes committed successfully!'); + console.log(`📝 ${result.message}`); + } else { + showError('Failed to commit changes.'); + } + return; + } else if (result.action === 'regenerate') { + currentConvention = result.convention; + attempts++; + continue; + } else if (result.action === 'skip') { + showInfo('Commit cancelled.'); + return; } - } else if (result.action === 'skip') { - showInfo('Commit cancelled.'); } + + showWarning('Maximum regeneration attempts reached.'); }
🤖 Fix all issues with AI agents
In `@source/commands/auth.js`:
- Around line 22-24: The CLI's --token option is unused because
authenticateWithCopilot() in source/commands/auth.js no longer accepts a token;
either remove the dead option from the CLI or update the function to accept and
use it — locate authenticateWithCopilot and the CLI registration of the "auth
copilot" command (options.token passed in source/cli.js) and do one of: (A)
delete the --token option from the command definition in source/cli.js and any
references to options.token, or (B) change authenticateWithCopilot to accept a
token parameter (e.g., authenticateWithCopilot(token) or an options object) and
thread options.token from the CLI into that function and use it where
authentication requires a token.
🧹 Nitpick comments (2)
source/utils/config-manager.js (1)
21-26: Consider logging or returning feedback when ignoring unsupported providers.
setTokensilently ignores any provider other than'openai'. While this aligns with the PR's goal to remove Copilot token handling, callers have no way to know if their token was stored. Consider either throwing an error for unsupported providers or returning a boolean to indicate success.♻️ Optional: Add feedback for unsupported providers
export function setToken(provider, token) { if (provider === 'openai') { config.set('openai', token); config.set('authenticatedAt', new Date().toISOString()); + return true; } + return false; }source/cli.js (1)
49-52: Consider validating--conventionoption at CLI level.The
--conventionoption is validated inset-conventionbut not when passed to thecommitcommand. While downstream code handles unknown conventions by falling back to'clean', users won't get immediate feedback about typos.♻️ Optional: Add validation for --convention option
.option( '--convention <type>', 'Commit convention (clean, conventional, gitmoji, simple)', ) .action(options => { + if (options.convention && !Object.keys(CONVENTIONS).includes(options.convention)) { + showError(`Invalid convention: ${options.convention}`); + console.log(`Available: ${Object.keys(CONVENTIONS).join(', ')}`); + process.exit(1); + } commitCommand(options); });
| export async function authenticateWithCopilot() { | ||
| try { | ||
| // Check for token in arguments first | ||
| let authToken = token; | ||
| console.log('🔐 Setting up GitHub Copilot authentication...\n'); |
There was a problem hiding this comment.
Function signature mismatch: --token option is ignored.
authenticateWithCopilot() no longer accepts a token parameter, but the CLI still passes options.token (see source/cli.js line 65). The --token <token> option on the auth copilot command is now dead code.
Either remove the --token option from the CLI or update this function to use it if there's a future use case.
🔧 Option 1: Remove the unused --token option from cli.js
In source/cli.js:
authCmd
.command('copilot')
.description('Authenticate with GitHub Copilot')
- .option('--token <token>', 'GitHub token')
- .action(options => {
- authenticateWithCopilot(options.token);
+ .action(() => {
+ authenticateWithCopilot();
});🤖 Prompt for AI Agents
In `@source/commands/auth.js` around lines 22 - 24, The CLI's --token option is
unused because authenticateWithCopilot() in source/commands/auth.js no longer
accepts a token; either remove the dead option from the CLI or update the
function to accept and use it — locate authenticateWithCopilot and the CLI
registration of the "auth copilot" command (options.token passed in
source/cli.js) and do one of: (A) delete the --token option from the command
definition in source/cli.js and any references to options.token, or (B) change
authenticateWithCopilot to accept a token parameter (e.g.,
authenticateWithCopilot(token) or an options object) and thread options.token
from the CLI into that function and use it where authentication requires a
token.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
Co-authored-by: warengonzaga <15052701+warengonzaga@users.noreply.github.com>
Implementation Plan: GitHub Copilot SDK Integration and Multi-Convention Support
Phase 1: Core Infrastructure
source/utils/commit-conventions.jswith 4 conventions (clean, conventional, gitmoji, simple)source/utils/config-manager.jsPhase 2: AI Provider Integration
source/providers/ai-provider.jswith real Copilot SDK integrationPhase 3: Authentication Updates
source/commands/auth.jsto check for GitHub CLIPhase 4: UI Enhancements
promptConventionSelection()tosource/utils/ui.jsconfirmCommit()to support convention switchingPhase 5: Command Updates
source/commands/commit.jswith regeneration supportPhase 6: CLI Interface
--conventionflag to commit command insource/cli.jsconfig set-conventionsubcommandPhase 7: Testing & Validation
Original prompt
Implement GitHub Copilot SDK Integration and Multi-Convention Support
Objective
Complete the GitHub Copilot SDK integration in PR #40 and add support for multiple commit message conventions with Clean Commit as the default.
Base Branch
This PR should target the
copilot/migrate-to-commander-and-copilotbranch from PR #40.Requirements
1. Create Commit Convention System
Create new file:
source/utils/commit-conventions.js