fix(review): wire Session Manager to /api/sessions/unified contract, stop Ctrl+K PTY leak, finish shortcut registry (PR #146)

- Session Manager (COD-121/192): align _loadSessionManagerList() with the
  merged #139 endpoint — map UnifiedSessionItem fields (lastActivityAt
  epoch-ms → lastModified, optional sizeBytes/firstPrompt/name) to the
  history-record shape _buildHistoryItem renders; surface non-2xx /
  error-envelope responses as a visible message instead of a silent
  "No sessions found"; route clicks by liveness (live row → selectSession,
  history row → resumeHistorySession by conversation UUID) via a new
  onActivate option so a live session is never duplicate-resumed
- Ctrl+K double-dispatch: gate the palette chord in
  attachCustomKeyEventHandler (return false on keydown) so xterm never
  writes 0x0b kill-line into the PTY while the palette opens; gate is
  registry-aware so a rebound/disabled palette shortcut restores normal
  terminal Ctrl+K
- Shortcut registry (COD-157) finished per maintainer decision: document
  keydown now dispatches through getShortcutRegistry() +
  matchesShortcutEvent() (legacy SHORTCUTS table removed), honoring
  per-shortcut disable and rebinds incl. the palette chord; overrides
  persist via saveAppSettingsToStorage() (correct device key + cache
  coherence, was orphaned 'codeman:settings'); Shortcuts tab renders on
  open via switchSettingsTab hook; capture uses a persistent listener that
  ignores bare modifier keydowns (combos now capturable) and requires a
  Ctrl/Cmd/Alt chord; settings rows use delegated listeners instead of
  inline onclick (JS-string injection sink) and overrides can no longer
  clobber id/label/action; added the missing row + overlay CSS
- matchesShortcutEvent: reject undeclared extra modifiers (Ctrl+Shift+K
  no longer hijacked from Firefox devtools) while keeping Ctrl/Cmd
  interchangeable; match physical code OR produced key for layout parity
- Registry/dispatch gaps: added restore-terminal-size entry, documented
  Ctrl+Shift+R again in the help modal (test flipped to assert presence),
  Ctrl+?/Alt+? now really open the registry-driven shortcut overlay, and
  Escape closes it
- Palette new-session pick routes through selectQuickStartCase() so the
  searchable combobox, dir display, and lastUsedCase stay in sync
- Removed fork cherry-pick debris: dead _onSessionListMaybeChanged(),
  orphaned .session-row-menu CSS, nonexistent closeMobileHeaderUtilities
  calls
- Tests: functional vm-harness coverage for the unified-list field
  mapping + error state + liveness routing, palette chord shift/disable/
  rebind handling, override persistence round-trip, capture flow, tab
  render hook, and source guards for the PTY gate + registry dispatch

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

test/command-palette-ui.test.ts

@@ -107,7 +107,6 @@ function loadPaletteHarness(overrides: Record<string, any> = {}) {
   app.cases = [{ name: 'plex-previews' }, { name: 'flux-player' }, { name: 'api-tools' }];
   app.selectSession = vi.fn();
   app.run = vi.fn();
-  app.closeMobileHeaderUtilities = vi.fn();
   app.getShortId = (id: string) => id.slice(0, 8);
   app.getSessionName = (session: any) =>
     session.name || session.workingDir?.split('/').pop() || app.getShortId(session.id);
@@ -177,6 +176,37 @@ describe('Command-K session palette', () => {
     ).toBe(true);
   });
 
+  it('rejects the palette chord when extra modifiers are held (Ctrl+Shift+K is the Firefox devtools console)', () => {
+    const { app } = loadPaletteHarness();
+
+    expect(
+      app.shouldOpenCommandPaletteFromShortcut({ key: 'K', code: 'KeyK', ctrlKey: true, shiftKey: true, target: null })
+    ).toBe(false);
+  });
+
+  it('honors a disabled or rebound palette shortcut from the registry', () => {
+    const { app } = loadPaletteHarness();
+
+    // Disabled entry → never opens, even for the default chord.
+    app.getShortcutRegistry = () => [
+      { id: 'command-palette', disabled: true, bindings: [{ modifiers: ['ctrl'], key: 'k', code: 'KeyK' }] },
+    ];
+    app.matchesShortcutEvent = () => true;
+    expect(app.shouldOpenCommandPaletteFromShortcut({ key: 'k', code: 'KeyK', ctrlKey: true, target: null })).toBe(
+      false
+    );
+
+    // Rebound entry → the new chord opens, the old default no longer does.
+    app.getShortcutRegistry = () => [{ id: 'command-palette', bindings: [{ modifiers: ['ctrl'], code: 'KeyP' }] }];
+    app.matchesShortcutEvent = (e: any, s: any) => e.code === s.bindings[0].code;
+    expect(app.shouldOpenCommandPaletteFromShortcut({ key: 'k', code: 'KeyK', ctrlKey: true, target: null })).toBe(
+      false
+    );
+    expect(app.shouldOpenCommandPaletteFromShortcut({ key: 'p', code: 'KeyP', ctrlKey: true, target: null })).toBe(
+      true
+    );
+  });
+
   it('opens and focuses the palette search box', () => {
     const { app, elements } = loadPaletteHarness();
 
@@ -258,6 +288,21 @@ describe('Command-K session palette', () => {
     expect(app.run).toHaveBeenCalledTimes(1);
   });
 
+  it('routes the new-session case pick through selectQuickStartCase when the picker mixin is loaded', async () => {
+    const { app } = loadPaletteHarness();
+    app.selectQuickStartCase = vi.fn();
+
+    const newSession = app.buildCommandPaletteItems('flux').find((item: any) => item.type === 'new-session');
+    app.commandPaletteItems = [newSession];
+    app.commandPaletteActiveIndex = 0;
+    await app.activateCommandPaletteItem();
+
+    // Keeps the searchable combobox, dir display, and lastUsedCase in sync
+    // instead of silently mutating the hidden native <select>.
+    expect(app.selectQuickStartCase).toHaveBeenCalledWith('flux-player');
+    expect(app.run).toHaveBeenCalledTimes(1);
+  });
+
   it('activates the highlighted session result', async () => {
     const { app } = loadPaletteHarness();
     app.openCommandPalette();
@@ -296,6 +341,88 @@ describe('Command-K session palette', () => {
   });
 });
 
+describe('Session Manager unified list', () => {
+  it('maps UnifiedSessionItem fields to the history-record shape and routes clicks by liveness', async () => {
+    const { app, elements } = loadPaletteHarness({
+      fetch: async (url: string) => {
+        expect(url).toBe('/api/sessions/unified?limit=200&q=api');
+        return {
+          ok: true,
+          status: 200,
+          json: async () => ({
+            success: true,
+            data: {
+              sessions: [
+                {
+                  sessionId: 'sess-alpha',
+                  name: 'Alpha API cleanup',
+                  workingDir: '/repo/api',
+                  lastActivityAt: 1751000000000,
+                  sources: ['live'],
+                },
+                {
+                  sessionId: 'conv-uuid-1',
+                  workingDir: '/repo/old',
+                  sizeBytes: 2048,
+                  firstPrompt: 'old prompt',
+                  lastActivityAt: 1750000000000,
+                  sources: ['history'],
+                },
+              ],
+              total: 2,
+            },
+          }),
+        };
+      },
+    });
+    elements.sessionManagerList = { replaceChildren: vi.fn(), appendChild: vi.fn() };
+    app._buildHistoryItem = vi.fn(() => ({}));
+    app.resumeHistorySession = vi.fn();
+
+    await app._loadSessionManagerList('api');
+
+    expect(app._buildHistoryItem).toHaveBeenCalledTimes(2);
+    const [liveRecord, , liveOptions] = app._buildHistoryItem.mock.calls[0];
+    expect(liveRecord).toMatchObject({
+      sessionId: 'sess-alpha',
+      workingDir: '/repo/api',
+      sizeBytes: 0,
+      firstPrompt: 'Alpha API cleanup',
+    });
+    expect(new Date(liveRecord.lastModified).getTime()).toBe(1751000000000);
+    expect(liveOptions.showViewAll).toBe(false);
+
+    // Live row → switch to the session (resuming it would spawn a duplicate).
+    liveOptions.onActivate();
+    expect(app.selectSession).toHaveBeenCalledWith('sess-alpha');
+    expect(app.resumeHistorySession).not.toHaveBeenCalled();
+
+    // History row → resume by conversation UUID.
+    const [historyRecord, , historyOptions] = app._buildHistoryItem.mock.calls[1];
+    expect(historyRecord).toMatchObject({ sessionId: 'conv-uuid-1', sizeBytes: 2048, firstPrompt: 'old prompt' });
+    historyOptions.onActivate();
+    expect(app.resumeHistorySession).toHaveBeenCalledWith('conv-uuid-1', '/repo/old');
+  });
+
+  it('surfaces an error message instead of an empty list when the endpoint fails', async () => {
+    const appended: any[] = [];
+    const { app, elements } = loadPaletteHarness({
+      fetch: async () => ({
+        ok: false,
+        status: 503,
+        json: async () => ({ success: false, error: 'unified list unavailable', errorCode: 'OPERATION_FAILED' }),
+      }),
+    });
+    elements.sessionManagerList = { replaceChildren: vi.fn(), appendChild: (el: any) => appended.push(el) };
+
+    await app._loadSessionManagerList('');
+
+    expect(appended).toHaveLength(1);
+    expect(appended[0].textContent).toBe('unified list unavailable');
+    expect(appended[0].textContent).not.toBe('No sessions found');
+  });
+});
+
 describe('panel close helpers', () => {
   it('closes panels when the mobile header helper is unavailable', () => {
     const CodemanApp = function CodemanApp(this: any) {};

test/help-modal-shortcuts.test.ts

@@ -54,9 +54,10 @@ describe('help modal shortcuts', () => {
     expectShortcut(helpModal, ['Ctrl', '-'], 'Decrease Font');
     expectShortcut(helpModal, ['Shift', 'Enter'], 'Insert Newline');
     expectShortcut(helpModal, ['Ctrl', 'Enter'], 'Insert Newline');
+    // Ctrl+Shift+R (restore terminal size) is still dispatched — keep it documented.
+    expectShortcut(helpModal, ['Ctrl', 'Shift', 'R'], 'Restore Terminal Size');
 
     expect(helpModal).not.toMatch(/Ctrl<\/kbd>\s*\+\s*<kbd>K<\/kbd>/i);
-    expect(helpModal).not.toMatch(/Ctrl<\/kbd>\s*\+\s*<kbd>Shift<\/kbd>\s*\+\s*<kbd>R<\/kbd>/i);
     expect(helpModal).not.toMatch(/Ctrl<\/kbd>\s*\+\s*<kbd>Enter<\/kbd>.*?(Run|Start)/i);
   });
 

test/keyboard-shortcuts.test.ts

@@ -40,4 +40,22 @@ describe('keyboard shortcuts', () => {
     expect(helpHtml).toContain('<kbd>Ctrl/Cmd/Option</kbd>+<kbd>K</kbd>');
     expect(readme).toMatch(/\| `Ctrl\/Cmd\/Option\+K`\s+\| Find open session or start a new one\s+\|/);
   });
+
+  it('gates the palette chord in the xterm custom key handler (no 0x0b kill-line into the PTY)', () => {
+    // The document-level capture handler opens the palette, but preventDefault()
+    // does NOT stop xterm from evaluating Ctrl+K into 0x0b and writing it to the
+    // live PTY — terminal-ui.js must return false for the palette chord.
+    expect(terminalUiSource).toMatch(/ev\.type === 'keydown' && this\.shouldOpenCommandPaletteFromShortcut\?\.\(ev\)/);
+  });
+
+  it('dispatches document shortcuts through the shortcut registry (rebind/disable aware)', () => {
+    // The legacy hardcoded SHORTCUTS table must stay gone — dispatch goes through
+    // getShortcutRegistry() + matchesShortcutEvent() so overrides and per-shortcut
+    // disables (App Settings → Shortcuts) actually take effect.
+    expect(appSource).not.toContain('const SHORTCUTS = [');
+    expect(appSource).toContain('const SHORTCUT_ACTIONS = {');
+    expect(appSource).toContain('for (const shortcut of this.getShortcutRegistry())');
+    expect(appSource).toContain('if (this.matchesShortcutEvent(e, shortcut))');
+    expect(appSource).toContain('if (shortcut.disabled || !shortcut.action) continue;');
+  });
 });

test/shortcut-registry-overlay.test.ts

@@ -1,5 +1,7 @@
 import { readFileSync } from 'node:fs';
-import { describe, expect, it } from 'vitest';
+import { resolve } from 'node:path';
+import vm from 'node:vm';
+import { describe, expect, it, vi } from 'vitest';
 
 const appSource = readFileSync('src/web/public/app.js', 'utf8');
 const settingsSource = readFileSync('src/web/public/settings-ui.js', 'utf8');
@@ -50,4 +52,165 @@ describe('shortcut registry and overlay', () => {
     expect(settingsSource).toContain('shortcut-reset-btn');
     expect(settingsSource).toContain('shortcut-enabled-checkbox');
   });
+
+  it('styles the shortcut settings rows and overlay (no unstyled tab)', () => {
+    const css = readFileSync('src/web/public/styles.css', 'utf8');
+    expect(css).toContain('.shortcut-setting-row {');
+    expect(css).toContain('.shortcut-capture-btn,');
+    expect(css).toContain('.shortcut-overlay-row {');
+  });
+});
+
+// ─── Functional coverage (vm-sandbox harness, mirrors run-mode-ui.test.ts) ────
+// The grep assertions above pin the wiring; these exercise the actual
+// persistence round-trip and capture flow that were broken in review.
+
+function makeLocalStorage() {
+  const store = new Map<string, string>();
+  return {
+    getItem: (k: string) => (store.has(k) ? store.get(k)! : null),
+    setItem: (k: string, v: string) => void store.set(k, String(v)),
+    removeItem: (k: string) => void store.delete(k),
+    key: (i: number) => [...store.keys()][i] ?? null,
+    get length() {
+      return store.size;
+    },
+  };
+}
+
+function loadSettingsHarness() {
+  const CodemanApp = function CodemanApp(this: any) {};
+  const localStorage = makeLocalStorage();
+  const elements: Record<string, any> = {};
+  const holder: { queryResult: any } = { queryResult: null };
+  const context = vm.createContext({
+    CodemanApp,
+    MobileDetection: { getDeviceType: () => 'desktop', isMobile: () => false, isTouchDevice: () => false },
+    localStorage,
+    document: {
+      getElementById: (id: string) => elements[id] ?? null,
+      querySelector: () => holder.queryResult,
+    },
+    console,
+    escapeHtml: (s: string) => String(s),
+  });
+
+  const settingsUi = readFileSync(resolve(import.meta.dirname, '../src/web/public/settings-ui.js'), 'utf8');
+  vm.runInContext(settingsUi, context, { filename: 'settings-ui.js' });
+
+  const app = new (CodemanApp as any)();
+  return { app, localStorage, elements, holder };
+}
+
+describe('shortcut settings persistence and capture', () => {
+  it('persists overrides under the app-settings storage key and round-trips through the cache', () => {
+    const { app, localStorage } = loadSettingsHarness();
+
+    app.toggleShortcutEnabled('close-session', false);
+
+    // Written to the SAME key loadAppSettingsFromStorage() reads (NOT the
+    // legacy 'codeman:settings' key), and the in-memory cache stays coherent.
+    const raw = localStorage.getItem('codeman-app-settings');
+    expect(raw).toBeTruthy();
+    expect(JSON.parse(raw!).shortcutOverrides['close-session']).toMatchObject({ disabled: true });
+    expect(localStorage.getItem('codeman:settings')).toBeNull();
+    expect(app.readShortcutOverridesFromSettings()['close-session']).toMatchObject({ disabled: true });
+
+    app.resetShortcutOverride('close-session');
+    const after = JSON.parse(localStorage.getItem('codeman-app-settings')!);
+    expect(after.shortcutOverrides['close-session']).toBeUndefined();
+    expect(app.readShortcutOverridesFromSettings()['close-session']).toBeUndefined();
+  });
+
+  it('captures multi-modifier combos: bare modifier keydowns do not end the capture', () => {
+    const { app, localStorage, holder } = loadSettingsHarness();
+    const listeners: Array<(e: any) => void> = [];
+    const input = {
+      value: '',
+      focus: vi.fn(),
+      addEventListener: vi.fn((_ev: string, fn: (e: any) => void) => listeners.push(fn)),
+      removeEventListener: vi.fn(),
+    };
+    holder.queryResult = input;
+
+    app.startShortcutCapture('clear-terminal');
+    expect(input.value).toBe('Press keys…');
+    const handler = listeners[0];
+
+    // First keydown of Ctrl+Shift+P is 'Control' — must not finalize.
+    handler({ key: 'Control', ctrlKey: true, preventDefault: vi.fn(), stopPropagation: vi.fn() });
+    expect(input.removeEventListener).not.toHaveBeenCalled();
+
+    handler({
+      key: 'P',
+      code: 'KeyP',
+      ctrlKey: true,
+      shiftKey: true,
+      preventDefault: vi.fn(),
+      stopPropagation: vi.fn(),
+    });
+    expect(input.removeEventListener).toHaveBeenCalledTimes(1);
+    const stored = JSON.parse(localStorage.getItem('codeman-app-settings')!);
+    expect(stored.shortcutOverrides['clear-terminal'].bindings[0]).toMatchObject({
+      modifiers: ['ctrl', 'shift'],
+      key: 'P',
+      code: 'KeyP',
+    });
+  });
+
+  it('rejects captures without a Ctrl/Cmd/Alt modifier (a bare key would fire while typing)', () => {
+    const { app, localStorage, holder } = loadSettingsHarness();
+    const listeners: Array<(e: any) => void> = [];
+    holder.queryResult = {
+      value: '',
+      focus: vi.fn(),
+      addEventListener: vi.fn((_ev: string, fn: (e: any) => void) => listeners.push(fn)),
+      removeEventListener: vi.fn(),
+    };
+    app.showToast = vi.fn();
+
+    app.startShortcutCapture('clear-terminal');
+    listeners[0]({ key: 'x', code: 'KeyX', preventDefault: vi.fn(), stopPropagation: vi.fn() });
+
+    expect(localStorage.getItem('codeman-app-settings')).toBeNull();
+    expect(app.showToast).toHaveBeenCalledWith('Shortcut must include Ctrl, Cmd, or Alt', 'error');
+  });
+
+  it('renders the shortcuts list when the Shortcuts settings tab is opened', () => {
+    const { app, elements } = loadSettingsHarness();
+    elements.appSettingsModal = { querySelectorAll: () => [] };
+    app.renderShortcutSettingsList = vi.fn();
+
+    app.switchSettingsTab('settings-shortcuts');
+    expect(app.renderShortcutSettingsList).toHaveBeenCalledTimes(1);
+
+    app.switchSettingsTab('settings-display');
+    expect(app.renderShortcutSettingsList).toHaveBeenCalledTimes(1);
+  });
+
+  it('renders configurable rows with delegated controls (no inline onclick) and fixed rows read-only', () => {
+    const { app, elements } = loadSettingsHarness();
+    const list: any = { innerHTML: '', dataset: {}, addEventListener: vi.fn() };
+    elements.appSettingsShortcutsList = list;
+    app.getShortcutRegistry = () => [
+      {
+        id: 'clear-terminal',
+        group: 'Terminal',
+        label: 'Clear Terminal',
+        bindings: [{ modifiers: ['ctrl'], key: 'l' }],
+        action: 'clearTerminal',
+      },
+      { id: 'close-panels', group: 'Panels', label: 'Close Panels', displayBindings: ['Escape'] },
+    ];
+
+    app.renderShortcutSettingsList();
+
+    expect(list.innerHTML).not.toContain('onclick=');
+    expect((list.innerHTML.match(/shortcut-capture-btn/g) || []).length).toBe(1);
+    expect(list.innerHTML).toContain('shortcut-setting-row--fixed');
+    // Delegated listeners wired exactly once.
+    expect(list.addEventListener).toHaveBeenCalledTimes(2);
+    app.renderShortcutSettingsList();
+    expect(list.addEventListener).toHaveBeenCalledTimes(2);
+  });
 });