Integrate GitHub Copilot CLI support and update documentation#549
Draft
Yintel12138 wants to merge 22 commits intositeboon:mainfrom
Draft
Integrate GitHub Copilot CLI support and update documentation#549Yintel12138 wants to merge 22 commits intositeboon:mainfrom
Yintel12138 wants to merge 22 commits intositeboon:mainfrom
Conversation
Co-authored-by: Yintel12138 <61579702+Yintel12138@users.noreply.github.com>
Co-authored-by: Yintel12138 <61579702+Yintel12138@users.noreply.github.com>
Co-authored-by: Yintel12138 <61579702+Yintel12138@users.noreply.github.com>
Co-authored-by: Yintel12138 <61579702+Yintel12138@users.noreply.github.com>
…ration Co-authored-by: Yintel12138 <61579702+Yintel12138@users.noreply.github.com>
Co-authored-by: Yintel12138 <61579702+Yintel12138@users.noreply.github.com>
…rTimeout Co-authored-by: Yintel12138 <61579702+Yintel12138@users.noreply.github.com>
…-support feat: Add GitHub Copilot CLI provider support
…ase version v1.25.2 Co-authored-by: Yintel12138 <61579702+Yintel12138@users.noreply.github.com>
Update README.zh-CN.md: comprehensive Chinese running guide with release version
Co-authored-by: Yintel12138 <61579702+Yintel12138@users.noreply.github.com>
…iles docs: sync all markdown files to reflect GitHub Copilot CLI support
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR integrates GitHub Copilot CLI as a fully supported provider alongside existing agents (Claude, Cursor, Codex, Gemini). Changes include backend process spawning with WebSocket streaming, API routes for authentication and session management, frontend UI components for provider/model selection, type updates, and documentation/localization additions. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Client
participant ChatUI as Chat UI
participant Hook as useChatComposerState
participant Server as Server
participant CopilotCLI as Copilot CLI<br/>Process
participant WS as WebSocket
User->>ChatUI: Select Copilot Provider &<br/>Send Message
ChatUI->>Hook: Execute Command<br/>(type: copilot-command)
Hook->>Server: Send copilot-command<br/>via WebSocket
Server->>CopilotCLI: spawnCopilot(command,<br/>options, ws)
CopilotCLI->>CopilotCLI: Resolve binary path<br/>Build args (--model,<br/>--output-format json, etc)
CopilotCLI->>CopilotCLI: Spawn process<br/>Stream stdout
CopilotCLI->>CopilotCLI: Parse line-delimited JSON
loop Message Types: system, user, assistant, result
CopilotCLI->>WS: Emit copilot-system<br/>(session init)
WS->>Hook: Handle copilot-system
Hook->>ChatUI: Update session & navigate
CopilotCLI->>WS: Emit copilot-output<br/>(terminal data)
WS->>Hook: Handle copilot-output<br/>(strip terminal noise)
Hook->>ChatUI: Stream output chunks
CopilotCLI->>WS: Emit copilot-result<br/>(final response)
WS->>Hook: Handle copilot-result
Hook->>ChatUI: Finalize message &<br/>clear stream buffer
end
CopilotCLI->>CopilotCLI: Process exit
CopilotCLI->>WS: Emit copilot-complete
WS->>Hook: Cleanup & resolve promise
sequenceDiagram
participant Settings as Settings UI
participant AuthHook as useSettingsController
participant AuthAPI as /api/cli/copilot/status
participant AuthModule as checkCopilotCredentials
participant FileSystem as File System
Settings->>Settings: User clicks Copilot Login
Settings->>AuthHook: Trigger Copilot Auth Check
AuthHook->>AuthAPI: GET /api/cli/copilot/status
AuthAPI->>AuthModule: checkCopilotCredentials()
alt GITHUB_TOKEN env set
AuthModule->>AuthModule: Return authenticated: true,<br/>email: "GitHub Token Auth"
else Check ~/.copilot/hosts.json
AuthModule->>FileSystem: Read & parse hosts.json
AuthModule->>AuthModule: Find github.com oauth_token,<br/>extract user email
else Check ~/.config/gh/hosts.yml
AuthModule->>FileSystem: Read & parse GH CLI config
AuthModule->>AuthModule: Find oauth_token,<br/>return "GitHub CLI Auth"
else
AuthModule->>AuthModule: Return authenticated: false,<br/>error: "Copilot CLI not configured"
end
AuthModule->>AuthAPI: Return status object
AuthAPI->>AuthHook: Return response
AuthHook->>Settings: Update copilotAuthStatus
Settings->>Settings: Show login prompt or<br/>authenticated state
Possibly Related PRs
Suggested Reviewers
Poem
🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/chat/view/ChatInterface.tsx (1)
273-280:⚠️ Potential issue | 🟡 MinorMissing
copilotcase inselectedProviderLabelternary.When
provider === 'copilot'and no project is selected, the label will incorrectly fall back tot('messageTypes.claude')instead of displaying "Copilot". This inconsistency will confuse users.🐛 Proposed fix to add copilot case
const selectedProviderLabel = provider === 'cursor' ? t('messageTypes.cursor') : provider === 'codex' ? t('messageTypes.codex') : provider === 'gemini' ? t('messageTypes.gemini') - : t('messageTypes.claude'); + : provider === 'copilot' + ? t('messageTypes.copilot') + : t('messageTypes.claude');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/view/ChatInterface.tsx` around lines 273 - 280, The selectedProviderLabel ternary in ChatInterface.tsx misses the 'copilot' case so when provider === 'copilot' it falls back to Claude; update the conditional that sets selectedProviderLabel to include provider === 'copilot' ? t('messageTypes.copilot') (placed before the final fallback) so Copilot correctly maps to its translation; locate the selectedProviderLabel variable in the ChatInterface component and add the copilot branch returning t('messageTypes.copilot').src/components/chat/hooks/useChatRealtimeHandlers.ts (1)
143-154:⚠️ Potential issue | 🟠 MajorTreat
copilot-resultas a terminal lifecycle event.
copilot-resultclears loading state in its case block, but it is missing fromlifecycleMessageTypes. In the pending/unbound-session path, the session filter above can return before this case runs, leaving Copilot sessions stuck in processing/loading state.✅ Minimal fix
'codex-error', 'gemini-error', 'copilot-error', + 'copilot-result', 'error',Also applies to: 826-892
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/hooks/useChatRealtimeHandlers.ts` around lines 143 - 154, The lifecycleMessageTypes Set in useChatRealtimeHandlers.ts is missing the 'copilot-result' event which causes Copilot sessions to remain in loading state when the pending/unbound-session filter returns early; add 'copilot-result' to the lifecycleMessageTypes Set so the pending/unbound-session path recognizes it as a terminal lifecycle event (update the Set definition that contains 'claude-complete', 'codex-complete', ... 'error' to include 'copilot-result').src/components/settings/hooks/useSettingsController.ts (1)
251-275:⚠️ Potential issue | 🟠 MajorLoad Copilot auth status when the settings dialog opens.
This adds the new Copilot auth state/setter, but the
isOpeneffect around Line 829 still never callscheckAuthStatus('copilot'). Until that happens,copilotAuthStatusstays atDEFAULT_AUTH_STATUS, so an already-authenticated Copilot account looks signed out on first open.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/settings/hooks/useSettingsController.ts` around lines 251 - 275, The isOpen effect isn't invoking checkAuthStatus for the new Copilot state so copilotAuthStatus (and setCopilotAuthStatus) remain DEFAULT_AUTH_STATUS; update the effect that runs on dialog open (the isOpen effect) to call checkAuthStatus('copilot') when isOpen becomes true (alongside existing calls for other providers), ensuring the effect's dependency array includes checkAuthStatus so the hook re-checks Copilot auth on open.
🧹 Nitpick comments (6)
e2e/app.spec.ts (1)
243-261: Consider usingtest.beforeEachor a shared fixture for authentication.Using
test.beforeAllwith therequestfixture in Playwright may not work as expected becauserequestis typically a per-test fixture. TheauthTokenvariable will be shared across tests in the describe block, but if tests run in different worker processes (even withworkers: 1), the token may not be available.Consider using a setup project or
test.beforeEachto ensure each test has a valid token.♻️ Alternative: Use test.beforeEach for token acquisition
test.describe('Provider CLI Status Endpoints', () => { - let authToken: string; - - test.beforeAll(async ({ request }) => { + test.beforeEach(async ({ request }, testInfo) => { + // Skip token acquisition if already stored in testInfo + if (testInfo.project.metadata?.authToken) return; + // Get auth token const statusRes = await request.get('/api/auth/status'); const status = await statusRes.json(); if (status.needsSetup) { const registerRes = await request.post('/api/auth/register', { data: { username: TEST_USER.username, password: TEST_USER.password }, }); const body = await registerRes.json(); - authToken = body.token; + testInfo.project.metadata = { authToken: body.token }; } else { const loginRes = await request.post('/api/auth/login', { data: { username: TEST_USER.username, password: TEST_USER.password }, }); const body = await loginRes.json(); - authToken = body.token; + testInfo.project.metadata = { authToken: body.token }; } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/app.spec.ts` around lines 243 - 261, The current use of test.beforeAll with the per-test Playwright "request" fixture risks missing auth tokens across workers; change the setup to acquire the token in test.beforeEach (or implement a shared fixture) so each test receives a fresh valid token using the "request" fixture, moving the logic that calls request.get('/api/auth/status'), request.post('/api/auth/register') and request.post('/api/auth/login') into test.beforeEach and assigning the result to the authToken used by tests (or create a custom fixture that returns authToken) to ensure per-test availability.server/routes/cli-auth.js (1)
482-498: YAML parsing is fragile - consider edge cases.The line-by-line YAML parsing could produce false positives if
oauth_token:appears as a value or in a comment, or if it belongs to a different host thangithub.com. The current logic checks for bothgithub.com:andoauth_token:existing anywhere in the file, but doesn't verify thatoauth_tokenis nested undergithub.com.For a more robust solution, consider using a YAML parser or checking that
oauth_token:appears aftergithub.com:.♻️ Optional: More precise YAML parsing
// Check gh CLI auth as fallback try { const ghConfigDir = path.join(os.homedir(), '.config', 'gh'); const hostsPath = path.join(ghConfigDir, 'hosts.yml'); const content = await fs.readFile(hostsPath, 'utf8'); - // Parse line-by-line to avoid substring false positives. - // The hosts.yml format has top-level host keys like "github.com:" - const lines = content.split('\n'); - const hasGitHubHost = lines.some(line => line.trimStart().startsWith('github.com:')); - const hasOAuthToken = lines.some(line => line.trimStart().startsWith('oauth_token:')); - if (hasGitHubHost && hasOAuthToken) { + // Parse line-by-line, tracking which host section we're in + const lines = content.split('\n'); + let inGitHubSection = false; + let foundOAuthInGitHub = false; + + for (const line of lines) { + const trimmed = line.trimStart(); + // Top-level host (no leading whitespace in original) + if (line === trimmed && trimmed.endsWith(':')) { + inGitHubSection = trimmed === 'github.com:'; + } else if (inGitHubSection && trimmed.startsWith('oauth_token:')) { + foundOAuthInGitHub = true; + break; + } + } + + if (foundOAuthInGitHub) { return { authenticated: true, email: 'GitHub CLI Auth' }; } } catch (e) { // Fall through }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/cli-auth.js` around lines 482 - 498, The current fallback GH CLI auth check in the async block that builds ghConfigDir/hostsPath and computes lines/hasGitHubHost/hasOAuthToken is fragile because it treats any separate occurrences as a match; update this logic to ensure the oauth_token is nested under the github.com host (either by parsing hosts.yml with a YAML parser or by verifying that an "oauth_token:" line appears after the "github.com:" host entry and is indented beneath it). Concretely, replace the line-by-line global checks (hasGitHubHost/hasOAuthToken) with a proper YAML load of hosts.yml (or a scoped scan that finds the github.com line index and then looks for an indented oauth_token in subsequent lines) and only return authenticated: true/email: 'GitHub CLI Auth' when the token is confirmed under the github.com key.server/copilot-cli.js (3)
190-209: Consider handling multiple content blocks.The current implementation only processes the first content block (
response.message.content[0]). If Copilot returns multi-block responses (e.g., text + code), subsequent blocks will be silently dropped.♻️ Suggested improvement to handle all content blocks
case 'assistant': // Accumulate assistant message chunks if (response.message && response.message.content && response.message.content.length > 0) { - const contentBlock = response.message.content[0]; - const textContent = contentBlock.text || ''; - - // Send as Claude-compatible format for frontend - ws.send({ - type: 'claude-response', - data: { - type: 'content_block_delta', - delta: { - type: 'text_delta', - text: textContent - } - }, - sessionId: capturedSessionId || sessionId || null - }); + // Process all content blocks + for (const contentBlock of response.message.content) { + const textContent = contentBlock.text || ''; + if (textContent) { + // Send as Claude-compatible format for frontend + ws.send({ + type: 'claude-response', + data: { + type: 'content_block_delta', + delta: { + type: 'text_delta', + text: textContent + } + }, + sessionId: capturedSessionId || sessionId || null + }); + } + } } break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/copilot-cli.js` around lines 190 - 209, The handler under case 'assistant' currently only reads response.message.content[0], dropping any additional content blocks; update it to iterate over response.message.content (check response.message and response.message.content exist and are arrays) and for each contentBlock send a separate ws.send with the same 'claude-response' / 'content_block_delta' shape (use contentBlock.text or other relevant fields if present), preserving sessionId (capturedSessionId || sessionId || null); ensure you handle empty text fields and non-text block types consistently so multi-block responses (e.g., text + code) are forwarded instead of discarded.
23-331: Consider adding a process timeout.The
spawnCopilotfunction has no timeout mechanism. If the Copilot CLI hangs or becomes unresponsive, the process will run indefinitely, potentially consuming resources.Consider adding a configurable timeout (e.g., 5-10 minutes) that terminates the process if no output is received within that period. This aligns with resilience patterns used for external process management.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/copilot-cli.js` around lines 23 - 331, spawnCopilot currently lacks a timeout so a hung Copilot CLI can run forever; add a configurable inactivity timeout (default e.g. 5–10 minutes) inside runCopilotProcess that tracks lastOutputTime and resets on stdout/stderr data events, and if elapsed > timeout calls copilotProcess.kill(), activeCopilotProcesses.delete(processKey/capturedSessionId), notifyRunFailed/notifyTerminalState with a clear timeout error, and call settleOnce to reject the Promise; ensure the timeout is cleared on normal process close/error and on successful resolve/reject (reference functions/vars: spawnCopilot, runCopilotProcess, copilotProcess, stdoutLineBuffer, processKey, capturedSessionId, settleOnce).
10-10: Useconstfor the Map declaration.Since
Mapis mutable, its contents can be modified without reassignment. Usingconstbetter expresses intent and prevents accidental reassignment.♻️ Suggested fix
-let activeCopilotProcesses = new Map(); // Track active processes by session ID +const activeCopilotProcesses = new Map(); // Track active processes by session ID🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/copilot-cli.js` at line 10, Replace the mutable variable declaration for activeCopilotProcesses with a constant: change the declaration of activeCopilotProcesses (the Map used to track active processes by session ID) from let to const, and ensure there are no later reassignments to activeCopilotProcesses elsewhere in the code (only mutate the Map contents via .set/.delete/.get). This preserves mutability of the Map itself while preventing accidental reassignment.src/components/chat/view/subcomponents/MessageComponent.tsx (1)
177-177: Consider refactoring the deeply nested ternary to a lookup map.The provider label logic has grown to handle 5 providers via nested ternaries, which impacts readability. A lookup object would be cleaner and easier to extend.
♻️ Suggested refactor using a lookup map
- {message.type === 'error' ? t('messageTypes.error') : message.type === 'tool' ? t('messageTypes.tool') : (provider === 'cursor' ? t('messageTypes.cursor') : provider === 'codex' ? t('messageTypes.codex') : provider === 'gemini' ? t('messageTypes.gemini') : provider === 'copilot' ? t('messageTypes.copilot') : t('messageTypes.claude'))} + {message.type === 'error' + ? t('messageTypes.error') + : message.type === 'tool' + ? t('messageTypes.tool') + : t(`messageTypes.${provider}`, { defaultValue: t('messageTypes.claude') })}Alternatively, define a constant outside the component:
const PROVIDER_MESSAGE_TYPES: Record<string, string> = { cursor: 'messageTypes.cursor', codex: 'messageTypes.codex', gemini: 'messageTypes.gemini', copilot: 'messageTypes.copilot', claude: 'messageTypes.claude', };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/view/subcomponents/MessageComponent.tsx` at line 177, Replace the deeply nested ternary in MessageComponent.tsx that computes the provider label (currently using message.type and provider) with a lookup map: create a constant like PROVIDER_MESSAGE_TYPES mapping provider keys ('cursor','codex','gemini','copilot','claude') to i18n keys, then compute the label by first checking message.type === 'error' or 'tool' and otherwise using t(PROVIDER_MESSAGE_TYPES[provider] ?? 'messageTypes.claude'); update the JSX expression to use this single lookup result so the logic in MessageComponent is clearer and easier to extend.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 10-14: The CHANGELOG.md entries currently link to the forked repo
(Yintel12138) instead of the main siteboon/claudecodeui repository; update the
two PR link references in the entries (the lines mentioning GitHub Copilot CLI
provider and README.zh-CN.md) to point to the correct main-repo PRs—either
replace the full fork URLs with the correct main-repo URLs
(https://github.com/siteboon/claudecodeui/pull/<main-pr-number>) or use the
relative shorthand (#<main-pr-number>) once you confirm the final PR numbers.
In `@README.zh-CN.md`:
- Around line 76-79: Update the "自托管（开源）" quick-start section to add a clear
prerequisite that, in addition to Node.js v22+, users must have at least one
supported CLI installed; explicitly list the supported tools (Claude Code CLI,
Cursor CLI, Codex, Gemini-CLI, GitHub Copilot CLI) and state “至少安装一种受支持的 CLI 工具”
near the existing npx usage instructions so readers know both Node and at least
one CLI are required.
- Around line 80-87: Add language identifiers to the unlabeled fenced code
blocks containing the shell commands to satisfy markdownlint MD040: change the
block that contains "npx `@siteboon/claude-code-ui`" to start with ```bash and
likewise change the block containing "npm install -g `@siteboon/claude-code-ui`"
(and the following "cloudcli" line) to start with ```bash so both fenced blocks
are explicitly marked as bash shell code blocks.
In `@server/copilot-cli.js`:
- Around line 16-21: Update getCopilotBinaryPath to reliably locate the Copilot
CLI: implement a PATH lookup first (use `which copilot` on Unix / `where
copilot` on Windows) and if not found on PATH, on Unix check
$HOME/.local/bin/copilot and /usr/local/bin/copilot before falling back to
process.env.COPILOT_CLI_PATH; on Windows avoid hard-coded AppData paths and
instead respect process.env.COPILOT_CLI_PATH or resolve the binary via shell
`where` (or let the shell execute without a full path) so the WinGet-managed
location is discovered dynamically; keep the function name getCopilotBinaryPath
to locate and update the code.
In `@server/routes/copilot.js`:
- Around line 7-24: The GET /sessions/:sessionId/messages handler currently
calls sessionManager.getSessionMessages(sessionId) without verifying ownership;
update the handler to first resolve the session owner (e.g., via
sessionManager.getSession(sessionId) or
sessionManager.getSessionOwner(sessionId)) and compare it to req.user.id,
returning 403 if they differ, and only then call getSessionMessages; apply the
same ownership check pattern to the corresponding DELETE handler
(router.delete('/sessions/:sessionId')) so both routes enforce per-session
ownership rather than just token auth.
In `@src/components/chat/hooks/useChatRealtimeHandlers.ts`:
- Around line 894-900: The regex-based control-character stripping in the
'copilot-output' branch should be moved into a helper function (e.g.,
stripTerminalNoise) to satisfy Biome's noControlCharactersInRegex rule:
implement stripTerminalNoise(value: string): string to iterate characters,
detect and consume ANSI escape sequences starting at 0x1B, skip control chars
below 0x20 except allow 0x09 (TAB), 0x0A (LF) and 0x0D (CR), and append all
other characters; then replace the current raw/cleaned logic in
useChatRealtimeHandlers (case 'copilot-output') to call
stripTerminalNoise(String(latestMessage.data ?? '')).trim() instead of using
regex literals with control-character escapes.
In `@src/components/settings/view/Settings.tsx`:
- Around line 165-170: The ProviderLoginModal's isAuthenticated check currently
omits Copilot, so even when copilotAuthStatus.authenticated is true the modal
reports Copilot as unauthenticated; update the isAuthenticated selector/variable
used in ProviderLoginModal (the code around isAuthenticated and the modal prop
wiring) to include copilot by OR-ing in copilotAuthStatus.authenticated (the
same way other providers like claude/cursor/codex/gemini are checked) so the
modal correctly reflects copilot's authenticated state when
copilotAuthStatus.authenticated is true.
In
`@src/components/settings/view/tabs/agents-settings/sections/AgentSelectorSection.tsx`:
- Around line 28-29: In AgentSelectorSection.tsx the Copilot branch maps to
'bg-gray-500' which conflicts with AgentListItem.tsx where the gray variant uses
'bg-foreground/60'; update the Copilot case in the ternary (the agent ===
'copilot' branch) to use 'bg-foreground/60' for visual consistency, or
alternatively extract the color mapping into a shared constant used by both
AgentSelectorSection and AgentListItem (e.g., a COLORS or AGENT_BADGE_CLASS map)
and reference that constant in the agent === 'copilot' branch.
---
Outside diff comments:
In `@src/components/chat/hooks/useChatRealtimeHandlers.ts`:
- Around line 143-154: The lifecycleMessageTypes Set in
useChatRealtimeHandlers.ts is missing the 'copilot-result' event which causes
Copilot sessions to remain in loading state when the pending/unbound-session
filter returns early; add 'copilot-result' to the lifecycleMessageTypes Set so
the pending/unbound-session path recognizes it as a terminal lifecycle event
(update the Set definition that contains 'claude-complete', 'codex-complete',
... 'error' to include 'copilot-result').
In `@src/components/chat/view/ChatInterface.tsx`:
- Around line 273-280: The selectedProviderLabel ternary in ChatInterface.tsx
misses the 'copilot' case so when provider === 'copilot' it falls back to
Claude; update the conditional that sets selectedProviderLabel to include
provider === 'copilot' ? t('messageTypes.copilot') (placed before the final
fallback) so Copilot correctly maps to its translation; locate the
selectedProviderLabel variable in the ChatInterface component and add the
copilot branch returning t('messageTypes.copilot').
In `@src/components/settings/hooks/useSettingsController.ts`:
- Around line 251-275: The isOpen effect isn't invoking checkAuthStatus for the
new Copilot state so copilotAuthStatus (and setCopilotAuthStatus) remain
DEFAULT_AUTH_STATUS; update the effect that runs on dialog open (the isOpen
effect) to call checkAuthStatus('copilot') when isOpen becomes true (alongside
existing calls for other providers), ensuring the effect's dependency array
includes checkAuthStatus so the hook re-checks Copilot auth on open.
---
Nitpick comments:
In `@e2e/app.spec.ts`:
- Around line 243-261: The current use of test.beforeAll with the per-test
Playwright "request" fixture risks missing auth tokens across workers; change
the setup to acquire the token in test.beforeEach (or implement a shared
fixture) so each test receives a fresh valid token using the "request" fixture,
moving the logic that calls request.get('/api/auth/status'),
request.post('/api/auth/register') and request.post('/api/auth/login') into
test.beforeEach and assigning the result to the authToken used by tests (or
create a custom fixture that returns authToken) to ensure per-test availability.
In `@server/copilot-cli.js`:
- Around line 190-209: The handler under case 'assistant' currently only reads
response.message.content[0], dropping any additional content blocks; update it
to iterate over response.message.content (check response.message and
response.message.content exist and are arrays) and for each contentBlock send a
separate ws.send with the same 'claude-response' / 'content_block_delta' shape
(use contentBlock.text or other relevant fields if present), preserving
sessionId (capturedSessionId || sessionId || null); ensure you handle empty text
fields and non-text block types consistently so multi-block responses (e.g.,
text + code) are forwarded instead of discarded.
- Around line 23-331: spawnCopilot currently lacks a timeout so a hung Copilot
CLI can run forever; add a configurable inactivity timeout (default e.g. 5–10
minutes) inside runCopilotProcess that tracks lastOutputTime and resets on
stdout/stderr data events, and if elapsed > timeout calls copilotProcess.kill(),
activeCopilotProcesses.delete(processKey/capturedSessionId),
notifyRunFailed/notifyTerminalState with a clear timeout error, and call
settleOnce to reject the Promise; ensure the timeout is cleared on normal
process close/error and on successful resolve/reject (reference functions/vars:
spawnCopilot, runCopilotProcess, copilotProcess, stdoutLineBuffer, processKey,
capturedSessionId, settleOnce).
- Line 10: Replace the mutable variable declaration for activeCopilotProcesses
with a constant: change the declaration of activeCopilotProcesses (the Map used
to track active processes by session ID) from let to const, and ensure there are
no later reassignments to activeCopilotProcesses elsewhere in the code (only
mutate the Map contents via .set/.delete/.get). This preserves mutability of the
Map itself while preventing accidental reassignment.
In `@server/routes/cli-auth.js`:
- Around line 482-498: The current fallback GH CLI auth check in the async block
that builds ghConfigDir/hostsPath and computes lines/hasGitHubHost/hasOAuthToken
is fragile because it treats any separate occurrences as a match; update this
logic to ensure the oauth_token is nested under the github.com host (either by
parsing hosts.yml with a YAML parser or by verifying that an "oauth_token:" line
appears after the "github.com:" host entry and is indented beneath it).
Concretely, replace the line-by-line global checks (hasGitHubHost/hasOAuthToken)
with a proper YAML load of hosts.yml (or a scoped scan that finds the github.com
line index and then looks for an indented oauth_token in subsequent lines) and
only return authenticated: true/email: 'GitHub CLI Auth' when the token is
confirmed under the github.com key.
In `@src/components/chat/view/subcomponents/MessageComponent.tsx`:
- Line 177: Replace the deeply nested ternary in MessageComponent.tsx that
computes the provider label (currently using message.type and provider) with a
lookup map: create a constant like PROVIDER_MESSAGE_TYPES mapping provider keys
('cursor','codex','gemini','copilot','claude') to i18n keys, then compute the
label by first checking message.type === 'error' or 'tool' and otherwise using
t(PROVIDER_MESSAGE_TYPES[provider] ?? 'messageTypes.claude'); update the JSX
expression to use this single lookup result so the logic in MessageComponent is
clearer and easier to extend.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b161a650-4eb0-4d42-b0a8-10ab8807a728
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonpublic/icons/copilot-white.svgis excluded by!**/*.svgpublic/icons/copilot.svgis excluded by!**/*.svg
📒 Files selected for processing (49)
.gitignoreCHANGELOG.mdCONTRIBUTING.mdREADME.de.mdREADME.ja.mdREADME.ko.mdREADME.mdREADME.ru.mdREADME.zh-CN.mde2e/app.spec.tspackage.jsonplaywright.config.tsserver/copilot-cli.jsserver/index.jsserver/projects.jsserver/routes/cli-auth.jsserver/routes/copilot.jsshared/modelConstants.jssrc/components/chat/hooks/useChatComposerState.tssrc/components/chat/hooks/useChatProviderState.tssrc/components/chat/hooks/useChatRealtimeHandlers.tssrc/components/chat/view/ChatInterface.tsxsrc/components/chat/view/subcomponents/AssistantThinkingIndicator.tsxsrc/components/chat/view/subcomponents/ChatMessagesPane.tsxsrc/components/chat/view/subcomponents/ClaudeStatus.tsxsrc/components/chat/view/subcomponents/MessageComponent.tsxsrc/components/chat/view/subcomponents/ProviderSelectionEmptyState.tsxsrc/components/llm-logo-provider/CopilotLogo.tsxsrc/components/llm-logo-provider/SessionProviderLogo.tsxsrc/components/onboarding/view/utils.tssrc/components/provider-auth/types.tssrc/components/provider-auth/view/ProviderLoginModal.tsxsrc/components/settings/constants/constants.tssrc/components/settings/hooks/useSettingsController.tssrc/components/settings/types/types.tssrc/components/settings/view/Settings.tsxsrc/components/settings/view/tabs/agents-settings/AgentListItem.tsxsrc/components/settings/view/tabs/agents-settings/AgentsSettingsTab.tsxsrc/components/settings/view/tabs/agents-settings/sections/AgentSelectorSection.tsxsrc/components/settings/view/tabs/agents-settings/sections/content/AccountContent.tsxsrc/components/settings/view/tabs/agents-settings/types.tssrc/components/sidebar/hooks/useSidebarController.tssrc/components/sidebar/types/types.tssrc/components/sidebar/utils/utils.tssrc/i18n/locales/en/chat.jsonsrc/i18n/locales/en/settings.jsonsrc/i18n/locales/zh-CN/chat.jsonsrc/types/app.tssrc/utils/api.js
src/components/settings/view/tabs/agents-settings/sections/AgentSelectorSection.tsx
Outdated
Show resolved
Hide resolved
- Fix CHANGELOG.md PR links to point to siteboon#549 - Fix README.zh-CN.md: add CLI prerequisite text, add bash language to code blocks - Fix copilot-cli.js: getCopilotBinaryPath now uses PATH lookup first via which/where, then checks official install paths ($HOME/.local/bin, /usr/local/bin), falls back to bare command name - Fix copilot routes: add session ownership check (critical security fix) - Fix useChatRealtimeHandlers.ts: refactor control-char regex to stripTerminalNoise helper to pass Biome noControlCharactersInRegex - Fix Settings.tsx: add copilot and gemini to isAuthenticated selector - Fix AgentSelectorSection.tsx: copilot color bg-gray-500 → bg-foreground/60 Co-authored-by: Yintel12138 <61579702+Yintel12138@users.noreply.github.com>
…rship check Co-authored-by: Yintel12138 <61579702+Yintel12138@users.noreply.github.com>
fix: address CodeRabbit CR comments on Copilot CLI integration
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
README.zh-CN.md (2)
2-4: Unify product naming in the header for consistency.The header mixes
CloudCLI UIandCloud CLI; consider using one canonical name throughout this block to avoid user confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.zh-CN.md` around lines 2 - 4, The header currently uses two variants ("CloudCLI UI" in the img alt and "Cloud CLI（又名 Claude Code UI）" in the h1), causing inconsistency; pick a single canonical product name (e.g., "Cloud CLI") and update both the <img> alt attribute ("Cloud CLI") and the h1 text to use that canonical name (remove the "又名 Claude Code UI" parenthetical), ensuring any links or surrounding copy in this block remain consistent with the chosen name.
304-323: Consider reducing hardcoded release data in README.This section is useful, but hardcoding multiple versions/dates tends to go stale quickly. Prefer a shorter “latest release + changelog link” pattern unless this table is updated every release.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.zh-CN.md` around lines 304 - 323, Replace the hardcoded multi-row release table and extensive historical entries in README.zh-CN.md with a concise "latest release" summary and a changelog link: keep the current Latest Release header (e.g., “当前版本：v1.25.2（2026-03-11）” or update to point to the releases/latest URL), remove the long version table and historical rows, and add a single line pointing to CHANGELOG.md or the GitHub releases page (e.g., “查看完整更新日志 →”); update the "最新更新亮点（v1.25.2）" block to show only the short highlights for the latest version and ensure links point to the canonical changelog/releases so future updates don’t require editing this file.server/copilot-cli.js (2)
367-376:abortCopilotSessionshould also remove ownership mapping.When a session is aborted, the process is cleaned up but
copilotSessionOwnersretains the stale entry. For consistency with the DELETE route which callsremoveCopilotSessionOwner, consider adding cleanup here:♻️ Proposed fix
function abortCopilotSession(sessionId) { const process = activeCopilotProcesses.get(sessionId); if (process) { console.log(`Aborting Copilot session: ${sessionId}`); process.kill('SIGTERM'); activeCopilotProcesses.delete(sessionId); + copilotSessionOwners.delete(sessionId); return true; } return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/copilot-cli.js` around lines 367 - 376, abortCopilotSession currently kills and removes the process from activeCopilotProcesses but leaves the stale owner mapping in copilotSessionOwners; update abortCopilotSession to also remove the session owner by calling removeCopilotSessionOwner(sessionId) or deleting copilotSessionOwners.get/sessionId (use the same removal approach as the DELETE route) after successfully killing the process so both activeCopilotProcesses and copilotSessionOwners are cleaned up consistently.
310-338: Session ownership persists after process exits.When the Copilot process closes (line 315 removes from
activeCopilotProcesses), the ownership mapping incopilotSessionOwnersis not cleaned up. This is likely intentional since sessions may persist across process restarts, but could lead to stale ownership data if sessions are never explicitly deleted.If ownership should only apply to active sessions, consider adding cleanup:
activeCopilotProcesses.delete(finalSessionId); + // Optional: clean up ownership if session won't persist + // copilotSessionOwners.delete(finalSessionId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/copilot-cli.js` around lines 310 - 338, When the Copilot process closes, remove any stale ownership entry for that session from copilotSessionOwners so ownership doesn't persist for inactive sessions: in the copilotProcess.on('close', ...) handler (the block that computes finalSessionId and calls activeCopilotProcesses.delete(finalSessionId)), also remove the corresponding entry from copilotSessionOwners (use copilotSessionOwners.delete(finalSessionId) if it's a Map or delete copilotSessionOwners[finalSessionId] if it's an object), optionally guarded by has/check and possibly log the removal; keep this cleanup before resolving/rejecting and sending the final ws 'claude-complete' message.src/components/chat/hooks/useChatRealtimeHandlers.ts (2)
789-811: Consider usingstripTerminalNoisehere for consistency.The
cursor-outputhandler still uses regex literals with control-character escapes (lines 792-794), which would trigger the same BiomenoControlCharactersInRegexrule that prompted the creation ofstripTerminalNoise. For consistency withcopilot-output, consider refactoring this handler to use the helper function.♻️ Suggested refactor
case 'cursor-output': try { const raw = String(latestMessage.data ?? ''); - const cleaned = raw - .replace(/\x1b\[[0-9;?]*[A-Za-z]/g, '') - .replace(/[\x00-\x08\x0B\x0C\x0E-\x1F]/g, '') - .trim(); + const cleaned = stripTerminalNoise(raw).trim(); if (cleaned) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/hooks/useChatRealtimeHandlers.ts` around lines 789 - 811, In the 'cursor-output' case inside useChatRealtimeHandlers.ts replace the inline regex cleaning (the String(latestMessage.data) normalization and .replace(...) chains on lines handling cursor-output) with a call to the existing helper stripTerminalNoise(raw) to remove terminal/control characters; keep the buffering logic using streamBufferRef, streamTimerRef and the appendStreamingChunk(setChatMessages, chunk, true) call unchanged and add/import stripTerminalNoise at the top of the module if not already imported.
858-924: Code duplication withcursor-resulthandler.This handler is nearly identical to
cursor-result(lines 718-787), with only variable naming differences. Consider extracting the shared logic into a helper function to reduce duplication and simplify future maintenance.♻️ Example extraction
const handleProviderResult = ( provider: 'cursor' | 'copilot', latestMessage: LatestChatMessage, currentSessionId: string | null, selectedSession: ProjectSession | null, // ... other dependencies ) => { const completedSessionId = latestMessage.sessionId || currentSessionId; const pendingSessionId = sessionStorage.getItem('pendingSessionId'); finalizeLifecycleForCurrentView( completedSessionId, currentSessionId, selectedSession?.id, pendingSessionId, ); // ... shared result handling logic };This would allow both handlers to call
handleProviderResult('cursor', ...)andhandleProviderResult('copilot', ...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/hooks/useChatRealtimeHandlers.ts` around lines 858 - 924, The copilot-result and cursor-result branches duplicate lifecycle finalization and result merging logic; extract that shared logic into a helper (e.g., handleProviderResult) that accepts the provider tag ('copilot' | 'cursor'), latestMessage, currentSessionId, selectedSession, refs (streamTimerRef, streamBufferRef), and state setters (setChatMessages, setCurrentSessionId). Inside the helper, call finalizeLifecycleForCurrentView with computed completedSessionId and pendingSessionId, clear streamTimerRef, drain streamBufferRef, perform the identical message normalization/merge/update logic, and handle setting current session + sessionStorage removal + optional window.refreshProjects; then replace both branch bodies to call handleProviderResult with the appropriate arguments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/copilot-cli.js`:
- Around line 183-192: The code accesses ws.setSessionId without confirming ws
is non-null; change the guard to check ws first and then the method: replace the
block using "if (ws.setSessionId && typeof ws.setSessionId === 'function')" with
a null-safe check such as "if (ws && typeof ws.setSessionId === 'function')"
before calling ws.setSessionId(capturedSessionId); this ensures ws is defined
when invoking setSessionId while keeping session owner tracking via
copilotSessionOwners.set(capturedSessionId, ownerId) unchanged.
In `@server/routes/copilot.js`:
- Around line 16-22: The ownership check currently skips authorization when
getCopilotSessionOwner(sessionId) returns a falsy owner, allowing any
authenticated user to access legacy/unowned sessions; update the logic in the
handler that uses getCopilotSessionOwner(sessionId) so that if ownerId is falsy
you also deny access (e.g., return res.status(403).json({ success: false, error:
'Access denied' })), and only allow access when req.user exists and req.user.id
=== ownerId; reference getCopilotSessionOwner, sessionId, and req.user to locate
the check and enforce denying access for unowned sessions or require explicit
ownership assignment before granting access.
- Around line 56-58: The DELETE handler currently calls
sessionManager.deleteSession(sessionId), sessionNamesDb.deleteName(sessionId,
'copilot'), and removeCopilotSessionOwner(sessionId) but never terminates the
running Copilot CLI; import and invoke abortCopilotSession(sessionId) (the
function managing active copilot processes) before performing the DB/owner
cleanup so any running subprocess is stopped first; ensure abortCopilotSession
is imported at the top of server/routes/copilot.js and called
synchronously/awaited in the same async DELETE handler using the same sessionId
to avoid leaving dangling processes.
In `@src/components/chat/hooks/useChatRealtimeHandlers.ts`:
- Around line 12-38: The stripTerminalNoise function incorrectly treats '[' as
the end of an ESC sequence; update stripTerminalNoise to detect CSI sequences
specifically: when you see ESC (0x1b) peek the next character and if it is '['
set a dedicated inCsi flag (inEscapeSequence + inCsi) and consume characters
until you encounter a final byte in the range 0x40..0x7E (which terminates CSI),
otherwise fall back to the existing logic where a single-byte OSC/ESC sequence
ends on a byte in '@'..'~'. Use the existing inEscapeSequence variable (add an
inCsi boolean) and the function name stripTerminalNoise to locate where to
implement this change.
---
Nitpick comments:
In `@README.zh-CN.md`:
- Around line 2-4: The header currently uses two variants ("CloudCLI UI" in the
img alt and "Cloud CLI（又名 Claude Code UI）" in the h1), causing inconsistency;
pick a single canonical product name (e.g., "Cloud CLI") and update both the
<img> alt attribute ("Cloud CLI") and the h1 text to use that canonical name
(remove the "又名 Claude Code UI" parenthetical), ensuring any links or
surrounding copy in this block remain consistent with the chosen name.
- Around line 304-323: Replace the hardcoded multi-row release table and
extensive historical entries in README.zh-CN.md with a concise "latest release"
summary and a changelog link: keep the current Latest Release header (e.g.,
“当前版本：v1.25.2（2026-03-11）” or update to point to the releases/latest URL),
remove the long version table and historical rows, and add a single line
pointing to CHANGELOG.md or the GitHub releases page (e.g., “查看完整更新日志 →”);
update the "最新更新亮点（v1.25.2）" block to show only the short highlights for the
latest version and ensure links point to the canonical changelog/releases so
future updates don’t require editing this file.
In `@server/copilot-cli.js`:
- Around line 367-376: abortCopilotSession currently kills and removes the
process from activeCopilotProcesses but leaves the stale owner mapping in
copilotSessionOwners; update abortCopilotSession to also remove the session
owner by calling removeCopilotSessionOwner(sessionId) or deleting
copilotSessionOwners.get/sessionId (use the same removal approach as the DELETE
route) after successfully killing the process so both activeCopilotProcesses and
copilotSessionOwners are cleaned up consistently.
- Around line 310-338: When the Copilot process closes, remove any stale
ownership entry for that session from copilotSessionOwners so ownership doesn't
persist for inactive sessions: in the copilotProcess.on('close', ...) handler
(the block that computes finalSessionId and calls
activeCopilotProcesses.delete(finalSessionId)), also remove the corresponding
entry from copilotSessionOwners (use copilotSessionOwners.delete(finalSessionId)
if it's a Map or delete copilotSessionOwners[finalSessionId] if it's an object),
optionally guarded by has/check and possibly log the removal; keep this cleanup
before resolving/rejecting and sending the final ws 'claude-complete' message.
In `@src/components/chat/hooks/useChatRealtimeHandlers.ts`:
- Around line 789-811: In the 'cursor-output' case inside
useChatRealtimeHandlers.ts replace the inline regex cleaning (the
String(latestMessage.data) normalization and .replace(...) chains on lines
handling cursor-output) with a call to the existing helper
stripTerminalNoise(raw) to remove terminal/control characters; keep the
buffering logic using streamBufferRef, streamTimerRef and the
appendStreamingChunk(setChatMessages, chunk, true) call unchanged and add/import
stripTerminalNoise at the top of the module if not already imported.
- Around line 858-924: The copilot-result and cursor-result branches duplicate
lifecycle finalization and result merging logic; extract that shared logic into
a helper (e.g., handleProviderResult) that accepts the provider tag ('copilot' |
'cursor'), latestMessage, currentSessionId, selectedSession, refs
(streamTimerRef, streamBufferRef), and state setters (setChatMessages,
setCurrentSessionId). Inside the helper, call finalizeLifecycleForCurrentView
with computed completedSessionId and pendingSessionId, clear streamTimerRef,
drain streamBufferRef, perform the identical message normalization/merge/update
logic, and handle setting current session + sessionStorage removal + optional
window.refreshProjects; then replace both branch bodies to call
handleProviderResult with the appropriate arguments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: edf5c03a-4ad5-4b1a-9232-36e3a0be2353
📒 Files selected for processing (7)
CHANGELOG.mdREADME.zh-CN.mdserver/copilot-cli.jsserver/routes/copilot.jssrc/components/chat/hooks/useChatRealtimeHandlers.tssrc/components/settings/view/Settings.tsxsrc/components/settings/view/tabs/agents-settings/sections/AgentSelectorSection.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
Comment on lines
+183
to
+192
| // Track session ownership for authorization checks | ||
| const ownerId = ws?.userId || null; | ||
| if (ownerId) { | ||
| copilotSessionOwners.set(capturedSessionId, ownerId); | ||
| } | ||
|
|
||
| // Set session ID on writer (for API endpoint compatibility) | ||
| if (ws.setSessionId && typeof ws.setSessionId === 'function') { | ||
| ws.setSessionId(capturedSessionId); | ||
| } |
Contributor
There was a problem hiding this comment.
Consider null-safety when accessing ws properties.
At line 184, ws?.userId is used safely, but at lines 190-192, ws.setSessionId is accessed without null-checking ws first. If ws is null/undefined, this will throw.
🛡️ Proposed fix
// Set session ID on writer (for API endpoint compatibility)
- if (ws.setSessionId && typeof ws.setSessionId === 'function') {
+ if (ws?.setSessionId && typeof ws.setSessionId === 'function') {
ws.setSessionId(capturedSessionId);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/copilot-cli.js` around lines 183 - 192, The code accesses
ws.setSessionId without confirming ws is non-null; change the guard to check ws
first and then the method: replace the block using "if (ws.setSessionId &&
typeof ws.setSessionId === 'function')" with a null-safe check such as "if (ws
&& typeof ws.setSessionId === 'function')" before calling
ws.setSessionId(capturedSessionId); this ensures ws is defined when invoking
setSessionId while keeping session owner tracking via
copilotSessionOwners.set(capturedSessionId, ownerId) unchanged.
Comment on lines
+16
to
+22
| // Verify session ownership: reject if the session belongs to a different user | ||
| const ownerId = getCopilotSessionOwner(sessionId); | ||
| if (ownerId) { | ||
| if (!req.user || req.user.id !== ownerId) { | ||
| return res.status(403).json({ success: false, error: 'Access denied' }); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Ownership check bypassed when session has no tracked owner.
When ownerId is falsy (e.g., session created before ownership tracking was added, or ws.userId was null during spawn), the entire ownership block is skipped, allowing any authenticated user to access the session.
Consider treating unowned sessions as inaccessible, or require explicit ownership assignment:
🛡️ Proposed fix
// Verify session ownership: reject if the session belongs to a different user
const ownerId = getCopilotSessionOwner(sessionId);
- if (ownerId) {
- if (!req.user || req.user.id !== ownerId) {
- return res.status(403).json({ success: false, error: 'Access denied' });
- }
+ if (!req.user || (ownerId && req.user.id !== ownerId)) {
+ return res.status(403).json({ success: false, error: 'Access denied' });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Verify session ownership: reject if the session belongs to a different user | |
| const ownerId = getCopilotSessionOwner(sessionId); | |
| if (ownerId) { | |
| if (!req.user || req.user.id !== ownerId) { | |
| return res.status(403).json({ success: false, error: 'Access denied' }); | |
| } | |
| } | |
| // Verify session ownership: reject if the session belongs to a different user | |
| const ownerId = getCopilotSessionOwner(sessionId); | |
| if (!req.user || (ownerId && req.user.id !== ownerId)) { | |
| return res.status(403).json({ success: false, error: 'Access denied' }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/routes/copilot.js` around lines 16 - 22, The ownership check currently
skips authorization when getCopilotSessionOwner(sessionId) returns a falsy
owner, allowing any authenticated user to access legacy/unowned sessions; update
the logic in the handler that uses getCopilotSessionOwner(sessionId) so that if
ownerId is falsy you also deny access (e.g., return res.status(403).json({
success: false, error: 'Access denied' })), and only allow access when req.user
exists and req.user.id === ownerId; reference getCopilotSessionOwner, sessionId,
and req.user to locate the check and enforce denying access for unowned sessions
or require explicit ownership assignment before granting access.
Comment on lines
+56
to
+58
| await sessionManager.deleteSession(sessionId); | ||
| sessionNamesDb.deleteName(sessionId, 'copilot'); | ||
| removeCopilotSessionOwner(sessionId); |
Contributor
There was a problem hiding this comment.
Missing abortCopilotSession call leaves dangling processes.
The DELETE handler cleans up database state but doesn't terminate an active Copilot CLI process. If the session is still running, the process will continue executing with no way to stop it.
Import and call abortCopilotSession before cleanup:
🔧 Proposed fix
-import { getCopilotSessionOwner, removeCopilotSessionOwner } from '../copilot-cli.js';
+import { getCopilotSessionOwner, removeCopilotSessionOwner, abortCopilotSession } from '../copilot-cli.js';+ // Terminate active process if running
+ abortCopilotSession(sessionId);
+
await sessionManager.deleteSession(sessionId);
sessionNamesDb.deleteName(sessionId, 'copilot');
removeCopilotSessionOwner(sessionId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/routes/copilot.js` around lines 56 - 58, The DELETE handler currently
calls sessionManager.deleteSession(sessionId),
sessionNamesDb.deleteName(sessionId, 'copilot'), and
removeCopilotSessionOwner(sessionId) but never terminates the running Copilot
CLI; import and invoke abortCopilotSession(sessionId) (the function managing
active copilot processes) before performing the DB/owner cleanup so any running
subprocess is stopped first; ensure abortCopilotSession is imported at the top
of server/routes/copilot.js and called synchronously/awaited in the same async
DELETE handler using the same sessionId to avoid leaving dangling processes.
Comment on lines
+12
to
+38
| const stripTerminalNoise = (value: string): string => { | ||
| let result = ''; | ||
| let inEscapeSequence = false; | ||
|
|
||
| for (const char of value) { | ||
| const code = char.charCodeAt(0); | ||
|
|
||
| if (inEscapeSequence) { | ||
| if (char >= '@' && char <= '~') { | ||
| inEscapeSequence = false; | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if (code === 0x1b) { | ||
| inEscapeSequence = true; | ||
| continue; | ||
| } | ||
|
|
||
| // Allow TAB (0x09), LF (0x0A), CR (0x0D), and printable chars (>= 0x20) | ||
| if (code === 0x09 || code === 0x0a || code === 0x0d || code >= 0x20) { | ||
| result += char; | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| }; |
Contributor
There was a problem hiding this comment.
CSI escape sequence handling ends prematurely.
The termination condition char >= '@' && char <= '~' (line 20) incorrectly ends the escape sequence when encountering [ (ASCII 0x5B), which is within that range. For CSI sequences like \x1b[31m, this results in 31m appearing in the output instead of being fully stripped.
A more robust approach would track whether we're in a CSI sequence specifically:
🛠️ Proposed fix for proper CSI sequence handling
const stripTerminalNoise = (value: string): string => {
let result = '';
let inEscapeSequence = false;
+ let inCsiSequence = false;
for (const char of value) {
const code = char.charCodeAt(0);
if (inEscapeSequence) {
- if (char >= '@' && char <= '~') {
+ if (char === '[') {
+ inCsiSequence = true;
+ continue;
+ }
+ // Non-CSI escape: single char after ESC ends it
+ inEscapeSequence = false;
+ continue;
+ }
+
+ if (inCsiSequence) {
+ // CSI sequences end with a letter (0x40-0x7E range)
+ if (code >= 0x40 && code <= 0x7e) {
+ inCsiSequence = false;
inEscapeSequence = false;
}
continue;
}
if (code === 0x1b) {
inEscapeSequence = true;
continue;
}
// Allow TAB (0x09), LF (0x0A), CR (0x0D), and printable chars (>= 0x20)
if (code === 0x09 || code === 0x0a || code === 0x0d || code >= 0x20) {
result += char;
}
}
return result;
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/chat/hooks/useChatRealtimeHandlers.ts` around lines 12 - 38,
The stripTerminalNoise function incorrectly treats '[' as the end of an ESC
sequence; update stripTerminalNoise to detect CSI sequences specifically: when
you see ESC (0x1b) peek the next character and if it is '[' set a dedicated
inCsi flag (inEscapeSequence + inCsi) and consume characters until you encounter
a final byte in the range 0x40..0x7E (which terminates CSI), otherwise fall back
to the existing logic where a single-byte OSC/ESC sequence ends on a byte in
'@'..'~'. Use the existing inEscapeSequence variable (add an inCsi boolean) and
the function name stripTerminalNoise to locate where to implement this change.
Contributor
|
@yintel thank you for your contribution. As this concerns two separate topics (e2e testing & GitHub copilot cli), could you please create two separate PRs, one for each topic ? |
Co-authored-by: Yintel12138 <61579702+Yintel12138@users.noreply.github.com>
…ng-copilot-cli Separate Copilot CLI integration from E2E testing (split PR siteboon#549)
Author
|
@viper151 I have removed E2E testing |
Collaborator
|
Hey @Yintel12138, thanks for the PR. Please look at the contributing guide and update your PR description. I have marked this as Draft. You can make it "ready for review" after it's done. |
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 by CodeRabbit
New Features
Documentation