refactor: clean up design debt — deprecated APIs, duplicated validation, dead plugin wrappers (#1065)

Three improvements from the design debt audit:

1. Remove deprecated `tabId` field and `getActiveTabId()` method
   - Delete `tabId` from DaemonCommand (daemon-client.ts) and Command (protocol.ts)
   - Delete `getActiveTabId()` from IPage interface (types.ts) and Page class (page.ts)
   - Update extension resolveCommandTabId() to remove legacy fallback
   - Update handleTabs select case to remove tabId check
   - The tab→page migration is now complete

2. Unify argument validation into single code path
   - Remove `normalizeArgValue()` from commanderAdapter.ts
   - Commander adapter now passes raw values to prepareCommandArgs()
   - All coercion (bool, int, number) and validation (required, choices)
     happens once in coerceAndValidateArgs() in execution.ts
   - Eliminates duplicated boolean normalization

3. Remove dead plugin filesystem wrappers
   - Delete `promoteDir()` — never called in production code
   - Delete `replaceDir()` — thin wrapper over beginReplaceDir, never called
   - Remove corresponding test-only exports and tests
   - Rename PromoteDirFsOps → ReplaceDirFsOps to match remaining usage
   - Transaction infrastructure (runTransaction, beginReplaceDir,
     beginReplaceSymlink) retained — used by publishStandalonePlugin
     and publishMonorepoPlugins for atomic multi-step operations

Author: jackwener

extension/src/background.ts

@@ -416,12 +416,12 @@ function setWorkspaceSession(workspace: string, session: Omit<AutomationSession,
 }
 
 /**
- * Resolve tabId from command's page (targetId) or legacy tabId field.
- * page (targetId) takes precedence. Returns undefined if neither is provided.
+ * Resolve tabId from command's page (targetId).
+ * Returns undefined if no page identity is provided.
  */
 async function resolveCommandTabId(cmd: Command): Promise<number | undefined> {
   if (cmd.page) return identity.resolveTabId(cmd.page);
-  return cmd.tabId;
+  return undefined;
 }
 
 type ResolvedTab = { tabId: number; tab: chrome.tabs.Tab | null };
@@ -686,7 +686,7 @@ async function handleTabs(cmd: Command, workspace: string): Promise<Result> {
       return { id: cmd.id, ok: true, data: { closed: closedPage } };
     }
     case 'select': {
-      if (cmd.index === undefined && cmd.page === undefined && cmd.tabId === undefined)
+      if (cmd.index === undefined && cmd.page === undefined)
         return { id: cmd.id, ok: false, error: 'Missing index or page' };
       const cmdTabId = await resolveCommandTabId(cmd);
       if (cmdTabId !== undefined) {

extension/src/protocol.ts

@@ -25,10 +25,8 @@ export interface Command {
   id: string;
   /** Action type */
   action: Action;
-  /** Target page identity (targetId). Cross-layer contract — preferred over tabId. */
+  /** Target page identity (targetId). Cross-layer contract with the daemon. */
   page?: string;
-  /** @deprecated Legacy tab ID — use `page` (targetId) instead. Kept for backward compat. */
-  tabId?: number;
   /** JS code to evaluate in page context (exec action) */
   code?: string;
   /** Logical workspace for automation session reuse */

src/browser/daemon-client.ts

@@ -22,10 +22,8 @@ function generateId(): string {
 export interface DaemonCommand {
   id: string;
   action: 'exec' | 'navigate' | 'tabs' | 'cookies' | 'screenshot' | 'close-window' | 'sessions' | 'set-file-input' | 'insert-text' | 'bind-current' | 'network-capture-start' | 'network-capture-read' | 'cdp';
-  /** Target page identity (targetId). Cross-layer contract — preferred over tabId. */
+  /** Target page identity (targetId). Cross-layer contract with the extension. */
   page?: string;
-  /** @deprecated Legacy tab ID — use `page` (targetId) instead. */
-  tabId?: number;
   code?: string;
   workspace?: string;
   url?: string;

src/browser/page.ts

@@ -108,11 +108,6 @@ export class Page extends BasePage {
     return this._page;
   }
 
-  /** @deprecated Use getActivePage() instead */
-  getActiveTabId(): number | undefined {
-    return undefined;
-  }
-
   private _markUnsupportedNetworkCapture(): void {
     this._networkCaptureUnsupported = true;
     if (this._networkCaptureWarned) return;

src/commanderAdapter.test.ts

@@ -77,7 +77,7 @@ describe('commanderAdapter arg passing', () => {
 
     await program.parseAsync(['node', 'opencli', 'paperreview', 'submit', './paper.pdf', '--dry-run', 'maybe']);
 
-    // normalizeArgValue validates bools eagerly; executeCommand should not be reached
+    // prepareCommandArgs validates bools before dispatch; executeCommand should not be reached
     expect(mockExecuteCommand).not.toHaveBeenCalled();
   });
 });

src/commanderAdapter.ts

@@ -20,23 +20,10 @@ import { executeCommand, prepareCommandArgs } from './execution.js';
 import {
   CliError,
   EXIT_CODES,
-  ArgumentError,
   toEnvelope,
 } from './errors.js';
 import { isDiagnosticEnabled } from './diagnostic.js';
 
-export function normalizeArgValue(argType: string | undefined, value: unknown, name: string): unknown {
-  if (argType !== 'bool' && argType !== 'boolean') return value;
-  if (typeof value === 'boolean') return value;
-  if (value == null || value === '') return false;
-
-  const normalized = String(value).trim().toLowerCase();
-  if (normalized === 'true') return true;
-  if (normalized === 'false') return false;
-
-  throw new ArgumentError(`"${name}" must be either "true" or "false".`);
-}
-
 /**
  * Register a single CliCommand as a Commander subcommand.
  */
@@ -85,7 +72,7 @@ export function registerCommandToProgram(siteCmd: Command, cmd: CliCommand): voi
         if (arg.positional) continue;
         const camelName = arg.name.replace(/-([a-z])/g, (_m, ch: string) => ch.toUpperCase());
         const v = optionsRecord[arg.name] ?? optionsRecord[camelName];
-        if (v !== undefined) rawKwargs[arg.name] = normalizeArgValue(arg.type, v, arg.name);
+        if (v !== undefined) rawKwargs[arg.name] = v;
       }
       const kwargs = prepareCommandArgs(cmd, rawKwargs);
 

src/plugin.test.ts

@@ -20,8 +20,6 @@ const {
   _getCommitHash,
   _installDependencies,
   _postInstallMonorepoLifecycle,
-  _promoteDir,
-  _replaceDir,
   installPlugin,
   listPlugins,
   _readLockFile,
@@ -1048,68 +1046,6 @@ describe('moveDir', () => {
   });
 });
 
-describe('promoteDir', () => {
-  it('cleans up temporary publish dir when final rename fails', () => {
-    const staging = path.join(os.tmpdir(), 'opencli-promote-stage');
-    const dest = path.join(os.tmpdir(), 'opencli-promote-dest');
-    const publishErr = new Error('publish failed');
-    const existsSync = vi.fn(() => false);
-    const mkdirSync = vi.fn(() => undefined);
-    const cpSync = vi.fn(() => undefined);
-    const rmSync = vi.fn(() => undefined);
-    const renameSync = vi.fn((src, _target) => {
-      if (String(src) === staging) return;
-      throw publishErr;
-    });
-
-    expect(() => _promoteDir(staging, dest, { existsSync, mkdirSync, renameSync, cpSync, rmSync })).toThrow(publishErr);
-
-    const tempDest = renameSync.mock.calls[0][1];
-    expect(renameSync).toHaveBeenNthCalledWith(1, staging, tempDest);
-    expect(renameSync).toHaveBeenNthCalledWith(2, tempDest, dest);
-    expect(rmSync).toHaveBeenCalledWith(tempDest, { recursive: true, force: true });
-  });
-});
-
-describe('replaceDir', () => {
-  it('rolls back the original destination when swap fails', () => {
-    const staging = path.join(os.tmpdir(), 'opencli-replace-stage');
-    const dest = path.join(os.tmpdir(), 'opencli-replace-dest');
-    const publishErr = new Error('swap failed');
-    const existingPaths = new Set([dest]);
-    const existsSync = vi.fn((p) => existingPaths.has(String(p)));
-    const mkdirSync = vi.fn(() => undefined);
-    const cpSync = vi.fn(() => undefined);
-    const rmSync = vi.fn(() => undefined);
-    const renameSync = vi.fn((src, target) => {
-      if (String(src) === staging) {
-        existingPaths.add(String(target));
-        return;
-      }
-      if (String(src) === dest) {
-        existingPaths.delete(dest);
-        existingPaths.add(String(target));
-        return;
-      }
-      if (String(target) === dest) throw publishErr;
-      if (existingPaths.has(String(src))) {
-        existingPaths.delete(String(src));
-        existingPaths.add(String(target));
-      }
-    });
-
-    expect(() => _replaceDir(staging, dest, { existsSync, mkdirSync, renameSync, cpSync, rmSync })).toThrow(publishErr);
-
-    const tempDest = renameSync.mock.calls[0][1];
-    const backupDest = renameSync.mock.calls[1][1];
-    expect(renameSync).toHaveBeenNthCalledWith(1, staging, tempDest);
-    expect(renameSync).toHaveBeenNthCalledWith(2, dest, backupDest);
-    expect(renameSync).toHaveBeenNthCalledWith(3, tempDest, dest);
-    expect(renameSync).toHaveBeenNthCalledWith(4, backupDest, dest);
-    expect(rmSync).toHaveBeenCalledWith(tempDest, { recursive: true, force: true });
-  });
-});
-
 describe('installPlugin transactional staging', () => {
   const standaloneSource = 'github:user/opencli-plugin-__test-transactional-standalone__';
   const standaloneName = '__test-transactional-standalone__';

src/plugin.ts

@@ -217,39 +217,13 @@ function moveDir(src: string, dest: string, fsOps: MoveDirFsOps = fs): void {
   }
 }
 
-type PromoteDirFsOps = MoveDirFsOps & Pick<typeof fs, 'existsSync' | 'mkdirSync'>;
+type ReplaceDirFsOps = MoveDirFsOps & Pick<typeof fs, 'existsSync' | 'mkdirSync'>;
 
 function createSiblingTempPath(dest: string, kind: 'tmp' | 'bak'): string {
   const suffix = `${process.pid}-${Date.now()}-${Math.random().toString(16).slice(2)}`;
   return path.join(path.dirname(dest), `.${path.basename(dest)}.${kind}-${suffix}`);
 }
 
-/**
- * Promote a prepared staging directory into its final location.
- * The final path is only exposed after the directory has been fully prepared.
- */
-function promoteDir(stagingDir: string, dest: string, fsOps: PromoteDirFsOps = fs): void {
-  if (fsOps.existsSync(dest)) {
-    throw new PluginError(`Destination already exists: ${dest}`);
-  }
-
-  fsOps.mkdirSync(path.dirname(dest), { recursive: true });
-  const tempDest = createSiblingTempPath(dest, 'tmp');
-
-  try {
-    moveDir(stagingDir, tempDest, fsOps);
-    fsOps.renameSync(tempDest, dest);
-  } catch (err) {
-    try { fsOps.rmSync(tempDest, { recursive: true, force: true }); } catch {}
-    throw err;
-  }
-}
-
-function replaceDir(stagingDir: string, dest: string, fsOps: PromoteDirFsOps = fs): void {
-  const replacement = beginReplaceDir(stagingDir, dest, fsOps);
-  replacement.finalize();
-}
-
 function cloneRepoToTemp(cloneUrl: string): string {
   const tmpCloneDir = path.join(
     os.tmpdir(),
@@ -359,7 +333,7 @@ function runTransaction<T>(work: (tx: Transaction) => T): T {
 function beginReplaceDir(
   stagingDir: string,
   dest: string,
-  fsOps: PromoteDirFsOps = fs,
+  fsOps: ReplaceDirFsOps = fs,
 ): TransactionHandle {
   const destExisted = fsOps.existsSync(dest);
   fsOps.mkdirSync(path.dirname(dest), { recursive: true });
@@ -1590,8 +1564,6 @@ export {
   installLocalPlugin as _installLocalPlugin,
   isLocalPluginSource as _isLocalPluginSource,
   moveDir as _moveDir,
-  promoteDir as _promoteDir,
-  replaceDir as _replaceDir,
   resolvePluginSource as _resolvePluginSource,
   resolveStoredPluginSource as _resolveStoredPluginSource,
   toStoredPluginSource as _toStoredPluginSource,

src/types.ts

@@ -86,8 +86,6 @@ export interface IPage {
   getCurrentUrl?(): Promise<string | null>;
   /** Returns the active page identity (targetId), or undefined if not yet resolved. */
   getActivePage?(): string | undefined;
-  /** @deprecated Use getActivePage() instead */
-  getActiveTabId?(): number | undefined;
   /** Send a raw CDP command via chrome.debugger passthrough. */
   cdp?(method: string, params?: Record<string, unknown>): Promise<unknown>;
   /** Click at native coordinates via CDP Input.dispatchMouseEvent. */