From f4678d639772699d9f3504dcd05bc9e311f67527 Mon Sep 17 00:00:00 2001 From: Tam Nhu Tran Date: Wed, 4 Mar 2026 16:37:23 +0700 Subject: [PATCH] fix(copilot): sync alias UX and harden daemon stop safety --- src/ccs.ts | 24 +----- src/commands/copilot-command.ts | 19 ++--- src/copilot/constants.ts | 43 +++++++++++ src/copilot/copilot-daemon.ts | 43 +++++++++-- src/copilot/copilot-executor.ts | 2 +- src/copilot/daemon-process-ownership.ts | 76 +++++++++++++++++++ .../copilot/copilot-command-aliases.test.ts | 35 +++++++++ tests/unit/copilot/copilot-daemon.test.ts | 75 +++++++++++++++++- 8 files changed, 272 insertions(+), 45 deletions(-) create mode 100644 src/copilot/constants.ts create mode 100644 src/copilot/daemon-process-ownership.ts create mode 100644 tests/unit/copilot/copilot-command-aliases.test.ts diff --git a/src/ccs.ts b/src/ccs.ts index 8790e839..99c57ca3 100644 --- a/src/ccs.ts +++ b/src/ccs.ts @@ -30,6 +30,7 @@ import { getGlobalEnvConfig } from './config/unified-config-loader'; import { ensureProfileHooks as ensureImageAnalyzerHooks } from './utils/hooks/image-analyzer-profile-hook-injector'; import { getImageAnalysisHookEnv } from './utils/hooks'; import { fail, info, warn } from './utils/ui'; +import { COPILOT_SUBCOMMAND_TOKENS } from './copilot/constants'; // Import centralized error handling import { handleError, runCleanup } from './errors'; @@ -573,28 +574,7 @@ async function main(): Promise { // Special case: copilot command (GitHub Copilot integration) // Only route to command handler for known subcommands, otherwise treat as profile - const COPILOT_SUBCOMMANDS = [ - 'auth', - 'status', - 'models', - 'usage', - 'start', - 'stop', - 'enable', - 'disable', - '--auth', - '--status', - '--models', - '--usage', - '--start', - '--stop', - '--enable', - '--disable', - 'help', - '--help', - '-h', - ]; - if (firstArg === 'copilot' && args.length > 1 && COPILOT_SUBCOMMANDS.includes(args[1])) { + if (firstArg === 'copilot' && args.length > 1 && COPILOT_SUBCOMMAND_TOKENS.includes(args[1])) { // `ccs copilot ` - route to copilot command handler const { handleCopilotCommand } = await import('./commands/copilot-command'); const exitCode = await handleCopilotCommand(args.slice(1)); diff --git a/src/commands/copilot-command.ts b/src/commands/copilot-command.ts index 7184a120..960a7328 100644 --- a/src/commands/copilot-command.ts +++ b/src/commands/copilot-command.ts @@ -16,23 +16,13 @@ import { import { loadOrCreateUnifiedConfig, saveUnifiedConfig } from '../config/unified-config-loader'; import { DEFAULT_COPILOT_CONFIG } from '../config/unified-config-types'; import { ok, fail, info, color } from '../utils/ui'; +import { normalizeCopilotSubcommand } from '../copilot/constants'; /** * Handle copilot subcommand. */ export async function handleCopilotCommand(args: string[]): Promise { - const subcommandAliasMap: Record = { - '--auth': 'auth', - '--status': 'status', - '--models': 'models', - '--usage': 'usage', - '--start': 'start', - '--stop': 'stop', - '--enable': 'enable', - '--disable': 'disable', - }; - const rawSubcommand = args[0]; - const subcommand = (rawSubcommand && subcommandAliasMap[rawSubcommand]) || rawSubcommand; + const subcommand = normalizeCopilotSubcommand(args[0]); switch (subcommand) { case 'auth': @@ -88,6 +78,11 @@ function handleHelp(): number { console.log(' 3. ccs copilot start # Start daemon'); console.log(' 4. ccs copilot usage # Check quota usage'); console.log(''); + console.log('Flag aliases:'); + console.log( + ' ccs copilot --auth | --status | --models | --usage | --start | --stop | --enable | --disable' + ); + console.log(''); console.log('Or use the web UI: ccs config → Copilot tab'); console.log(''); return 0; diff --git a/src/copilot/constants.ts b/src/copilot/constants.ts new file mode 100644 index 00000000..c4c0b42f --- /dev/null +++ b/src/copilot/constants.ts @@ -0,0 +1,43 @@ +/** + * Shared Copilot command tokens and aliases. + * Keep all copilot subcommand routing in one place to avoid drift. + */ + +export const COPILOT_SUBCOMMANDS = [ + 'auth', + 'status', + 'models', + 'usage', + 'start', + 'stop', + 'enable', + 'disable', +] as const; + +export type CopilotSubcommand = (typeof COPILOT_SUBCOMMANDS)[number]; + +export const COPILOT_FLAG_ALIASES: Readonly> = + Object.freeze({ + '--auth': 'auth', + '--status': 'status', + '--models': 'models', + '--usage': 'usage', + '--start': 'start', + '--stop': 'stop', + '--enable': 'enable', + '--disable': 'disable', + }); + +export const COPILOT_SUBCOMMAND_TOKENS = Object.freeze([ + ...COPILOT_SUBCOMMANDS, + ...Object.keys(COPILOT_FLAG_ALIASES), + 'help', + '--help', + '-h', +]); + +export function normalizeCopilotSubcommand(token?: string): string | undefined { + if (!token) return token; + const alias = COPILOT_FLAG_ALIASES[token as keyof typeof COPILOT_FLAG_ALIASES]; + return alias || token; +} diff --git a/src/copilot/copilot-daemon.ts b/src/copilot/copilot-daemon.ts index dc6cbbef..2dae17be 100644 --- a/src/copilot/copilot-daemon.ts +++ b/src/copilot/copilot-daemon.ts @@ -12,8 +12,13 @@ import * as http from 'http'; import { CopilotDaemonStatus } from './types'; import { CopilotConfig } from '../config/unified-config-types'; import { getCopilotDir, getCopilotApiBinPath } from './copilot-package-manager'; +import { verifyCopilotDaemonOwnership } from './daemon-process-ownership'; -const PID_FILE = path.join(getCopilotDir(), 'daemon.pid'); +const DAEMON_HEALTH_MARKER = 'server running'; + +function getPidFilePath(): string { + return path.join(getCopilotDir(), 'daemon.pid'); +} /** * Check if copilot-api daemon is running on the specified port. @@ -43,7 +48,7 @@ export async function isDaemonRunning(port: number): Promise { return; } - resolve(body.trim().toLowerCase().includes('server running')); + resolve(body.trim().toLowerCase().includes(DAEMON_HEALTH_MARKER)); }); } ); @@ -79,9 +84,10 @@ export async function getDaemonStatus(port: number): Promise` + const lower = commandLine.toLowerCase(); + const looksLikeCopilotDaemon = lower.includes('copilot-api') && lower.includes(' start'); + + return looksLikeCopilotDaemon ? 'owned' : 'not-owned'; +} diff --git a/tests/unit/copilot/copilot-command-aliases.test.ts b/tests/unit/copilot/copilot-command-aliases.test.ts new file mode 100644 index 00000000..4cc4b7ae --- /dev/null +++ b/tests/unit/copilot/copilot-command-aliases.test.ts @@ -0,0 +1,35 @@ +import { describe, expect, it } from 'bun:test'; +import { + COPILOT_SUBCOMMANDS, + COPILOT_SUBCOMMAND_TOKENS, + normalizeCopilotSubcommand, +} from '../../../src/copilot/constants'; + +describe('copilot command aliases', () => { + it('normalizes all supported flag aliases', () => { + for (const subcommand of COPILOT_SUBCOMMANDS) { + expect(normalizeCopilotSubcommand(`--${subcommand}`)).toBe(subcommand); + } + }); + + it('keeps canonical subcommands unchanged', () => { + for (const subcommand of COPILOT_SUBCOMMANDS) { + expect(normalizeCopilotSubcommand(subcommand)).toBe(subcommand); + } + }); + + it('returns unknown tokens unchanged', () => { + expect(normalizeCopilotSubcommand('--unknown')).toBe('--unknown'); + expect(normalizeCopilotSubcommand('unknown')).toBe('unknown'); + }); + + it('exposes complete routing token list for ccs entrypoint', () => { + for (const subcommand of COPILOT_SUBCOMMANDS) { + expect(COPILOT_SUBCOMMAND_TOKENS).toContain(subcommand); + expect(COPILOT_SUBCOMMAND_TOKENS).toContain(`--${subcommand}`); + } + expect(COPILOT_SUBCOMMAND_TOKENS).toContain('help'); + expect(COPILOT_SUBCOMMAND_TOKENS).toContain('--help'); + expect(COPILOT_SUBCOMMAND_TOKENS).toContain('-h'); + }); +}); diff --git a/tests/unit/copilot/copilot-daemon.test.ts b/tests/unit/copilot/copilot-daemon.test.ts index 9de0c02e..ad8e6b6f 100644 --- a/tests/unit/copilot/copilot-daemon.test.ts +++ b/tests/unit/copilot/copilot-daemon.test.ts @@ -1,8 +1,21 @@ -import { afterEach, describe, expect, it } from 'bun:test'; +import { afterEach, beforeEach, describe, expect, it } from 'bun:test'; import * as http from 'http'; -import { isDaemonRunning } from '../../../src/copilot/copilot-daemon'; +import * as fs from 'fs'; +import * as path from 'path'; +import * as os from 'os'; +import { spawn } from 'child_process'; +import { isDaemonRunning, stopDaemon } from '../../../src/copilot/copilot-daemon'; +import { getCcsDir } from '../../../src/utils/config-manager'; const activeServers: http.Server[] = []; +let originalCcsHome: string | undefined; +let tempDir: string; + +beforeEach(() => { + originalCcsHome = process.env.CCS_HOME; + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ccs-copilot-daemon-test-')); + process.env.CCS_HOME = tempDir; +}); afterEach(async () => { await Promise.all( @@ -13,6 +26,18 @@ afterEach(async () => { }) ) ); + + if (originalCcsHome !== undefined) { + process.env.CCS_HOME = originalCcsHome; + } else { + delete process.env.CCS_HOME; + } + + try { + fs.rmSync(tempDir, { recursive: true, force: true }); + } catch { + // Ignore cleanup errors + } }); async function createServer( @@ -59,6 +84,16 @@ describe('copilot daemon health detection', () => { expect(running).toBe(false); }); + it('returns false when root endpoint returns 200 with empty body', async () => { + const port = await createServer((_req, res) => { + res.writeHead(200, { 'Content-Type': 'text/plain' }); + res.end(''); + }); + + const running = await isDaemonRunning(port); + expect(running).toBe(false); + }); + it('returns false when root endpoint is non-200', async () => { const port = await createServer((_req, res) => { res.writeHead(503, { 'Content-Type': 'text/plain' }); @@ -69,3 +104,39 @@ describe('copilot daemon health detection', () => { expect(running).toBe(false); }); }); + +describe('copilot daemon stop safety', () => { + it('does not terminate unrelated process from stale PID file', async () => { + const unrelatedProcess = spawn(process.execPath, ['-e', 'setInterval(() => {}, 1000);'], { + detached: true, + stdio: 'ignore', + }); + unrelatedProcess.unref(); + + const unrelatedPid = unrelatedProcess.pid; + expect(unrelatedPid).toBeDefined(); + if (!unrelatedPid) { + throw new Error('Failed to spawn unrelated process'); + } + + const pidFile = path.join(getCcsDir(), 'copilot', 'daemon.pid'); + fs.mkdirSync(path.dirname(pidFile), { recursive: true }); + fs.writeFileSync(pidFile, String(unrelatedPid)); + + try { + const result = await stopDaemon(); + if (!result.success) { + expect(result.error).toContain('unable to verify daemon ownership'); + } + + // Unrelated process should still be alive. + expect(() => process.kill(unrelatedPid, 0)).not.toThrow(); + } finally { + try { + process.kill(unrelatedPid, 'SIGTERM'); + } catch { + // Process already exited. + } + } + }); +});