From 98f3a3eda074dc6757538a6c271216140d9019c1 Mon Sep 17 00:00:00 2001 From: Michal Date: Sun, 8 Mar 2026 01:17:03 +0000 Subject: [PATCH] refactor: consolidate restore under backup command mcpctl backup restore list/diff/to instead of separate mcpctl restore. Co-Authored-By: Claude Opus 4.6 --- completions/mcpctl.bash | 24 ++----------- completions/mcpctl.fish | 18 ++-------- src/cli/src/commands/backup.ts | 15 ++++---- src/cli/src/index.ts | 7 +--- src/cli/tests/commands/backup.test.ts | 50 +++++++++----------------- src/cli/tests/completions.test.ts | 2 +- src/cli/tests/e2e/cli-commands.test.ts | 1 - 7 files changed, 32 insertions(+), 85 deletions(-) diff --git a/completions/mcpctl.bash b/completions/mcpctl.bash index 8fd2048..9fca0e3 100644 --- a/completions/mcpctl.bash +++ b/completions/mcpctl.bash @@ -5,7 +5,7 @@ _mcpctl() { local cur prev words cword _init_completion || return - local commands="status login logout config get describe delete logs create edit apply patch backup restore approve console cache" + local commands="status login logout config get describe delete logs create edit apply patch backup approve console cache" local project_commands="get describe delete logs create edit attach-server detach-server" local global_opts="-v --version --daemon-url --direct -p --project -h --help" local resources="servers instances secrets templates projects users groups rbac prompts promptrequests serverattachments proxymodels all" @@ -235,7 +235,7 @@ _mcpctl() { backup) local backup_sub=$(_mcpctl_get_subcmd $subcmd_pos) if [[ -z "$backup_sub" ]]; then - COMPREPLY=($(compgen -W "log key help" -- "$cur")) + COMPREPLY=($(compgen -W "log key restore help" -- "$cur")) else case "$backup_sub" in log) @@ -244,27 +244,9 @@ _mcpctl() { key) COMPREPLY=($(compgen -W "-h --help" -- "$cur")) ;; - *) + restore) COMPREPLY=($(compgen -W "-h --help" -- "$cur")) ;; - esac - fi - return ;; - restore) - local restore_sub=$(_mcpctl_get_subcmd $subcmd_pos) - if [[ -z "$restore_sub" ]]; then - COMPREPLY=($(compgen -W "list diff to help" -- "$cur")) - else - case "$restore_sub" in - list) - COMPREPLY=($(compgen -W "-n --limit -h --help" -- "$cur")) - ;; - diff) - COMPREPLY=($(compgen -W "-h --help" -- "$cur")) - ;; - to) - COMPREPLY=($(compgen -W "--force -h --help" -- "$cur")) - ;; *) COMPREPLY=($(compgen -W "-h --help" -- "$cur")) ;; diff --git a/completions/mcpctl.fish b/completions/mcpctl.fish index 6818520..3b4fb9c 100644 --- a/completions/mcpctl.fish +++ b/completions/mcpctl.fish @@ -4,7 +4,7 @@ # Erase any stale completions from previous versions complete -c mcpctl -e -set -l commands status login logout config get describe delete logs create edit apply patch backup restore approve console cache +set -l commands status login logout config get describe delete logs create edit apply patch backup approve console cache set -l project_commands get describe delete logs create edit attach-server detach-server # Disable file completions by default @@ -228,7 +228,6 @@ complete -c mcpctl -n "not __mcpctl_has_project; and not __fish_seen_subcommand_ complete -c mcpctl -n "not __mcpctl_has_project; and not __fish_seen_subcommand_from $commands" -a apply -d 'Apply declarative configuration from a YAML or JSON file' complete -c mcpctl -n "not __mcpctl_has_project; and not __fish_seen_subcommand_from $commands" -a patch -d 'Patch a resource field (e.g. mcpctl patch project myproj llmProvider=none)' complete -c mcpctl -n "not __mcpctl_has_project; and not __fish_seen_subcommand_from $commands" -a backup -d 'Git-based backup status and management' -complete -c mcpctl -n "not __mcpctl_has_project; and not __fish_seen_subcommand_from $commands" -a restore -d 'Restore mcpctl state from backup history' complete -c mcpctl -n "not __mcpctl_has_project; and not __fish_seen_subcommand_from $commands" -a approve -d 'Approve a pending prompt request (atomic: delete request, create prompt)' complete -c mcpctl -n "not __mcpctl_has_project; and not __fish_seen_subcommand_from $commands" -a console -d 'Interactive MCP console — unified timeline with tools, provenance, and lab replay' complete -c mcpctl -n "not __mcpctl_has_project; and not __fish_seen_subcommand_from $commands" -a cache -d 'Manage ProxyModel pipeline cache' @@ -354,25 +353,14 @@ complete -c mcpctl -n "__mcpctl_subcmd_active create promptrequest" -l content-f complete -c mcpctl -n "__mcpctl_subcmd_active create promptrequest" -l priority -d 'Priority 1-10 (default: 5, higher = more important)' -x # backup subcommands -set -l backup_cmds log key +set -l backup_cmds log key restore complete -c mcpctl -n "__fish_seen_subcommand_from backup; and not __fish_seen_subcommand_from $backup_cmds" -a log -d 'Show backup commit history' complete -c mcpctl -n "__fish_seen_subcommand_from backup; and not __fish_seen_subcommand_from $backup_cmds" -a key -d 'Show SSH public key for deploy key setup' +complete -c mcpctl -n "__fish_seen_subcommand_from backup; and not __fish_seen_subcommand_from $backup_cmds" -a restore -d 'Restore mcpctl state from backup history' # backup log options complete -c mcpctl -n "__mcpctl_subcmd_active backup log" -s n -l limit -d 'number of commits to show' -x -# restore subcommands -set -l restore_cmds list diff to -complete -c mcpctl -n "__fish_seen_subcommand_from restore; and not __fish_seen_subcommand_from $restore_cmds" -a list -d 'List available restore points' -complete -c mcpctl -n "__fish_seen_subcommand_from restore; and not __fish_seen_subcommand_from $restore_cmds" -a diff -d 'Preview what restoring to a commit would change' -complete -c mcpctl -n "__fish_seen_subcommand_from restore; and not __fish_seen_subcommand_from $restore_cmds" -a to -d 'Restore to a specific commit' - -# restore list options -complete -c mcpctl -n "__mcpctl_subcmd_active restore list" -s n -l limit -d 'number of entries' -x - -# restore to options -complete -c mcpctl -n "__mcpctl_subcmd_active restore to" -l force -d 'skip confirmation' - # cache subcommands set -l cache_cmds stats clear complete -c mcpctl -n "__fish_seen_subcommand_from cache; and not __fish_seen_subcommand_from $cache_cmds" -a stats -d 'Show cache statistics' diff --git a/src/cli/src/commands/backup.ts b/src/cli/src/commands/backup.ts index 87c779e..146ddaa 100644 --- a/src/cli/src/commands/backup.ts +++ b/src/cli/src/commands/backup.ts @@ -112,14 +112,11 @@ export function createBackupCommand(deps: BackupDeps): Command { deps.log('Add this key as a deploy key (with write access) in your Git hosting provider.'); }); - return cmd; -} - -export function createRestoreCommand(deps: BackupDeps): Command { - const cmd = new Command('restore') + // ── Restore subcommand group ── + const restore = new Command('restore') .description('Restore mcpctl state from backup history'); - cmd + restore .command('list') .description('List available restore points') .option('-n, --limit ', 'number of entries', '30') @@ -156,7 +153,7 @@ export function createRestoreCommand(deps: BackupDeps): Command { } }); - cmd + restore .command('diff ') .description('Preview what restoring to a commit would change') .action(async (commit: string) => { @@ -186,7 +183,7 @@ export function createRestoreCommand(deps: BackupDeps): Command { deps.log(`Total: ${preview.added.length} added, ${preview.modified.length} modified, ${preview.removed.length} removed`); }); - cmd + restore .command('to ') .description('Restore to a specific commit') .option('--force', 'skip confirmation', false) @@ -236,6 +233,8 @@ export function createRestoreCommand(deps: BackupDeps): Command { } }); + cmd.addCommand(restore); + return cmd; } diff --git a/src/cli/src/index.ts b/src/cli/src/index.ts index edc4fa2..eccd191 100644 --- a/src/cli/src/index.ts +++ b/src/cli/src/index.ts @@ -10,7 +10,7 @@ import { createLogsCommand } from './commands/logs.js'; import { createApplyCommand } from './commands/apply.js'; import { createCreateCommand } from './commands/create.js'; import { createEditCommand } from './commands/edit.js'; -import { createBackupCommand, createRestoreCommand } from './commands/backup.js'; +import { createBackupCommand } from './commands/backup.js'; import { createLoginCommand, createLogoutCommand } from './commands/auth.js'; import { createAttachServerCommand, createDetachServerCommand, createApproveCommand } from './commands/project-ops.js'; import { createMcpCommand } from './commands/mcp.js'; @@ -191,11 +191,6 @@ export function createProgram(): Command { log: (...args) => console.log(...args), })); - program.addCommand(createRestoreCommand({ - client, - log: (...args) => console.log(...args), - })); - const projectOpsDeps = { client, log: (...args: string[]) => console.log(...args), diff --git a/src/cli/tests/commands/backup.test.ts b/src/cli/tests/commands/backup.test.ts index fa89aac..2872649 100644 --- a/src/cli/tests/commands/backup.test.ts +++ b/src/cli/tests/commands/backup.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { createBackupCommand, createRestoreCommand } from '../../src/commands/backup.js'; +import { createBackupCommand } from '../../src/commands/backup.js'; const mockClient = { get: vi.fn(), @@ -10,14 +10,17 @@ const mockClient = { const log = vi.fn(); +function makeCmd() { + return createBackupCommand({ client: mockClient as never, log }); +} + describe('backup command', () => { beforeEach(() => { vi.resetAllMocks(); }); it('creates backup command', () => { - const cmd = createBackupCommand({ client: mockClient as never, log }); - expect(cmd.name()).toBe('backup'); + expect(makeCmd().name()).toBe('backup'); }); it('shows status when enabled', async () => { @@ -31,8 +34,7 @@ describe('backup command', () => { pendingCount: 0, }); - const cmd = createBackupCommand({ client: mockClient as never, log }); - await cmd.parseAsync([], { from: 'user' }); + await makeCmd().parseAsync([], { from: 'user' }); expect(mockClient.get).toHaveBeenCalledWith('/api/v1/backup/status'); expect(log).toHaveBeenCalledWith(expect.stringContaining('ssh://git@10.0.0.194:2222/michal/mcp-backup.git')); @@ -50,8 +52,7 @@ describe('backup command', () => { pendingCount: 0, }); - const cmd = createBackupCommand({ client: mockClient as never, log }); - await cmd.parseAsync([], { from: 'user' }); + await makeCmd().parseAsync([], { from: 'user' }); expect(log).toHaveBeenCalledWith(expect.stringContaining('disabled')); }); @@ -67,8 +68,7 @@ describe('backup command', () => { pendingCount: 5, }); - const cmd = createBackupCommand({ client: mockClient as never, log }); - await cmd.parseAsync([], { from: 'user' }); + await makeCmd().parseAsync([], { from: 'user' }); expect(log).toHaveBeenCalledWith(expect.stringContaining('5 changes pending')); }); @@ -76,8 +76,7 @@ describe('backup command', () => { it('shows SSH public key', async () => { mockClient.get.mockResolvedValue({ publicKey: 'ssh-ed25519 AAAA... mcpd@mcpctl.local' }); - const cmd = createBackupCommand({ client: mockClient as never, log }); - await cmd.parseAsync(['key'], { from: 'user' }); + await makeCmd().parseAsync(['key'], { from: 'user' }); expect(mockClient.get).toHaveBeenCalledWith('/api/v1/backup/key'); expect(log).toHaveBeenCalledWith('ssh-ed25519 AAAA... mcpd@mcpctl.local'); @@ -91,28 +90,20 @@ describe('backup command', () => { ], }); - const cmd = createBackupCommand({ client: mockClient as never, log }); - await cmd.parseAsync(['log'], { from: 'user' }); + await makeCmd().parseAsync(['log'], { from: 'user' }); expect(mockClient.get).toHaveBeenCalledWith('/api/v1/backup/log?limit=20'); - // Header expect(log).toHaveBeenCalledWith(expect.stringContaining('COMMIT')); - // Entries expect(log).toHaveBeenCalledWith(expect.stringContaining('abc1234')); expect(log).toHaveBeenCalledWith(expect.stringContaining('[manual]')); }); }); -describe('restore command', () => { +describe('backup restore subcommands', () => { beforeEach(() => { vi.resetAllMocks(); }); - it('creates restore command', () => { - const cmd = createRestoreCommand({ client: mockClient as never, log }); - expect(cmd.name()).toBe('restore'); - }); - it('lists restore points', async () => { mockClient.get.mockResolvedValue({ entries: [ @@ -120,8 +111,7 @@ describe('restore command', () => { ], }); - const cmd = createRestoreCommand({ client: mockClient as never, log }); - await cmd.parseAsync(['list'], { from: 'user' }); + await makeCmd().parseAsync(['restore', 'list'], { from: 'user' }); expect(mockClient.get).toHaveBeenCalledWith('/api/v1/backup/log?limit=30'); expect(log).toHaveBeenCalledWith(expect.stringContaining('abc1234')); @@ -137,8 +127,7 @@ describe('restore command', () => { modified: ['projects/default.yaml'], }); - const cmd = createRestoreCommand({ client: mockClient as never, log }); - await cmd.parseAsync(['diff', 'abc1234'], { from: 'user' }); + await makeCmd().parseAsync(['restore', 'diff', 'abc1234'], { from: 'user' }); expect(mockClient.post).toHaveBeenCalledWith('/api/v1/backup/restore/preview', { commit: 'abc1234' }); expect(log).toHaveBeenCalledWith(expect.stringContaining('+ servers/new.yaml')); @@ -156,17 +145,14 @@ describe('restore command', () => { modified: [], }); - const cmd = createRestoreCommand({ client: mockClient as never, log }); - await cmd.parseAsync(['to', 'abc1234'], { from: 'user' }); + await makeCmd().parseAsync(['restore', 'to', 'abc1234'], { from: 'user' }); - // Should show preview but NOT call restore endpoint expect(mockClient.post).toHaveBeenCalledWith('/api/v1/backup/restore/preview', { commit: 'abc1234' }); expect(mockClient.post).not.toHaveBeenCalledWith('/api/v1/backup/restore', expect.anything()); expect(log).toHaveBeenCalledWith(expect.stringContaining('--force')); }); it('executes restore with --force', async () => { - // First call: preview, second call: restore mockClient.post .mockResolvedValueOnce({ targetCommit: 'abc1234567890', @@ -183,8 +169,7 @@ describe('restore command', () => { errors: [], }); - const cmd = createRestoreCommand({ client: mockClient as never, log }); - await cmd.parseAsync(['to', 'abc1234', '--force'], { from: 'user' }); + await makeCmd().parseAsync(['restore', 'to', 'abc1234', '--force'], { from: 'user' }); expect(mockClient.post).toHaveBeenCalledWith('/api/v1/backup/restore', { commit: 'abc1234' }); expect(log).toHaveBeenCalledWith(expect.stringContaining('1 applied')); @@ -208,8 +193,7 @@ describe('restore command', () => { errors: ['Failed to apply servers/broken.yaml: invalid YAML'], }); - const cmd = createRestoreCommand({ client: mockClient as never, log }); - await cmd.parseAsync(['to', 'abc1234', '--force'], { from: 'user' }); + await makeCmd().parseAsync(['restore', 'to', 'abc1234', '--force'], { from: 'user' }); expect(log).toHaveBeenCalledWith('Errors:'); expect(log).toHaveBeenCalledWith(expect.stringContaining('invalid YAML')); diff --git a/src/cli/tests/completions.test.ts b/src/cli/tests/completions.test.ts index c178201..a4197db 100644 --- a/src/cli/tests/completions.test.ts +++ b/src/cli/tests/completions.test.ts @@ -119,7 +119,7 @@ describe('fish completions', () => { }); it('non-project commands do not show with --project', () => { - const nonProjectCmds = ['status', 'login', 'logout', 'config', 'apply', 'backup', 'restore']; + const nonProjectCmds = ['status', 'login', 'logout', 'config', 'apply', 'backup']; const lines = fishFile.split('\n').filter((l) => l.startsWith('complete') && l.includes('-a ')); for (const cmd of nonProjectCmds) { diff --git a/src/cli/tests/e2e/cli-commands.test.ts b/src/cli/tests/e2e/cli-commands.test.ts index fe6a5db..04cb3e2 100644 --- a/src/cli/tests/e2e/cli-commands.test.ts +++ b/src/cli/tests/e2e/cli-commands.test.ts @@ -22,7 +22,6 @@ describe('CLI command registration (e2e)', () => { expect(commandNames).toContain('create'); expect(commandNames).toContain('edit'); expect(commandNames).toContain('backup'); - expect(commandNames).toContain('restore'); }); it('old project and claude top-level commands are removed', () => {