fix(sdk): address PR review for preset registry (SD-3128)

Three review findings from PR 3541:

1. Restore structured ToolCatalog.tools type. The refactor narrowed the
   public catalog row to `unknown[]`, breaking TS consumers that read
   tools[i].toolName etc. Move ToolCatalogEntry + ToolCatalogOperation
   into presets.ts as public types and tighten the catalog signature.

2. Fail fast on malformed provider bundles. Node and Python preset
   loaders previously coerced a missing or non-array `tools` field to
   `[]`, hiding broken codegen output behind a silently empty tool
   surface. Restore the pre-presets TOOLS_ASSET_INVALID throw at the
   preset boundary.

3. Cross-lang parity for empty-string presets. Python choose_tools
   treated `{'preset': ''}` as legacy via `or DEFAULT_PRESET`; Node and
   MCP both raise PRESET_NOT_FOUND. Use an explicit None check so
   Python matches.

Tests added covering structural catalog access, empty-string preset
fail-fast, and cross-lang parity for the empty-string case.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: andrii-harbour

packages/sdk/langs/node/src/__tests__/presets.test.ts

@@ -50,6 +50,39 @@ describe('preset registry', () => {
       expect(details.availablePresets).toContain('legacy');
     }
   });
+
+  test('getPreset("") throws PRESET_NOT_FOUND (empty string is not the default)', () => {
+    try {
+      getPreset('');
+      throw new Error('Expected getPreset("") to throw.');
+    } catch (error) {
+      expect(error).toBeInstanceOf(SuperDocCliError);
+      expect((error as SuperDocCliError).code).toBe('PRESET_NOT_FOUND');
+    }
+  });
+
+  test('chooseTools({preset: ""}) throws PRESET_NOT_FOUND (cross-lang parity)', async () => {
+    await expect(chooseTools({ provider: 'openai', preset: '' })).rejects.toMatchObject({
+      code: 'PRESET_NOT_FOUND',
+    });
+  });
+});
+
+describe('public ToolCatalog type — structural access', () => {
+  test('getToolCatalog().tools entries expose typed properties', async () => {
+    const catalog = await getToolCatalog();
+    expect(catalog.tools.length).toBeGreaterThan(0);
+    const first = catalog.tools[0]!;
+    // These property accesses validate that ToolCatalog.tools is structurally
+    // typed (ToolCatalogEntry[]) — not unknown[]. Compile failure here means
+    // the public catalog row type regressed.
+    expect(typeof first.toolName).toBe('string');
+    expect(typeof first.description).toBe('string');
+    expect(typeof first.mutates).toBe('boolean');
+    expect(Array.isArray(first.operations)).toBe(true);
+    expect(typeof first.operations[0]?.operationId).toBe('string');
+    expect(typeof first.operations[0]?.intentAction).toBe('string');
+  });
 });
 
 describe('chooseTools — default preset equivalence', () => {

packages/sdk/langs/node/src/index.ts

@@ -262,6 +262,8 @@ export type {
   CacheStrategy,
   SystemPromptForProviderResult,
   ToolCatalog,
+  ToolCatalogEntry,
+  ToolCatalogOperation,
   ToolChooserInput,
   ToolProvider,
 } from './tools.js';

packages/sdk/langs/node/src/presets.ts

@@ -44,6 +44,33 @@ export type ToolProvider = 'openai' | 'anthropic' | 'vercel' | 'generic';
  */
 export type CacheStrategy = 'explicit' | 'automatic' | 'unsupported' | 'disabled';
 
+/**
+ * One operation row in a {@link ToolCatalogEntry}. Each catalog entry can
+ * dispatch to one or more operations (e.g. multi-action intent tools), so
+ * the catalog records the operation id and the action discriminator that
+ * routes to it.
+ */
+export type ToolCatalogOperation = {
+  operationId: string;
+  intentAction: string;
+  required?: string[];
+  requiredOneOf?: string[][];
+};
+
+/**
+ * One entry in the {@link ToolCatalog}. Matches the shape of the catalog
+ * emitted by the legacy preset's codegen — kept stable as the public
+ * catalog row shape so TypeScript consumers can introspect `tools[i]`
+ * without losing property typing.
+ */
+export type ToolCatalogEntry = {
+  toolName: string;
+  description: string;
+  inputSchema: Record<string, unknown>;
+  mutates: boolean;
+  operations: ToolCatalogOperation[];
+};
+
 /**
  * Full tool catalog shape. The legacy preset returns the existing codegen
  * catalog with `contractVersion`, `generatedAt`, `toolCount`, `tools`.
@@ -52,7 +79,7 @@ export type ToolCatalog = {
   contractVersion: string;
   generatedAt: string | null;
   toolCount: number;
-  tools: unknown[];
+  tools: ToolCatalogEntry[];
 };
 
 export interface GetToolsOptions {

packages/sdk/langs/node/src/presets/legacy.ts

@@ -21,7 +21,15 @@ import type { BoundDocApi } from '../generated/client.js';
 import type { InvokeOptions } from '../runtime/process.js';
 import { SuperDocCliError } from '../runtime/errors.js';
 import { dispatchIntentTool } from '../generated/intent-dispatch.generated.js';
-import type { PresetDescriptor, GetToolsOptions, GetToolsResult, ToolCatalog, ToolProvider } from '../presets.js';
+import type {
+  GetToolsOptions,
+  GetToolsResult,
+  PresetDescriptor,
+  ToolCatalog,
+  ToolCatalogEntry,
+  ToolCatalogOperation,
+  ToolProvider,
+} from '../presets.js';
 
 // Resolve tools directory relative to package root (works from both src/ and dist/)
 const toolsDir = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..', 'tools');
@@ -33,23 +41,6 @@ const providerFileByName: Record<ToolProvider, string> = {
   generic: 'tools.generic.json',
 };
 
-type OperationEntry = {
-  operationId: string;
-  intentAction: string;
-  required?: string[];
-  requiredOneOf?: string[][];
-};
-
-type ToolCatalogEntry = {
-  toolName: string;
-  description: string;
-  inputSchema: Record<string, unknown>;
-  mutates: boolean;
-  operations: OperationEntry[];
-};
-
-type LegacyToolCatalog = ToolCatalog & { tools: ToolCatalogEntry[] };
-
 const STRIP_EMPTY_OPTIONAL_ARGS = new Set(['parentId', 'parentCommentId', 'id', 'status']);
 
 function isRecord(value: unknown): value is Record<string, unknown> {
@@ -103,11 +94,11 @@ async function readProviderTools(provider: ToolProvider): Promise<{
 }
 
 // Cached catalog instance — loaded once per process.
-let _catalogCache: LegacyToolCatalog | null = null;
+let _catalogCache: ToolCatalog | null = null;
 
-async function getCachedCatalog(): Promise<LegacyToolCatalog> {
+async function getCachedCatalog(): Promise<ToolCatalog> {
   if (_catalogCache == null) {
-    _catalogCache = await readJson<LegacyToolCatalog>('catalog.json');
+    _catalogCache = await readJson<ToolCatalog>('catalog.json');
   }
   return _catalogCache;
 }
@@ -210,7 +201,7 @@ function validateToolArgs(toolName: string, args: Record<string, unknown>, tool:
   // 3. Reject missing per-operation required keys. For multi-action tools,
   //    resolve the operation by action; for single-op tools, use the sole entry.
   const action = args.action;
-  let op: OperationEntry | undefined;
+  let op: ToolCatalogOperation | undefined;
   if (typeof action === 'string' && tool.operations.length > 1) {
     op = tool.operations.find((o) => o.intentAction === action);
   } else if (tool.operations.length === 1) {
@@ -230,7 +221,7 @@ function validateOperationRequired(
   toolName: string,
   action: unknown,
   args: Record<string, unknown>,
-  op: OperationEntry,
+  op: ToolCatalogOperation,
 ): void {
   const actionLabel = typeof action === 'string' ? ` action "${action}"` : '';
 
@@ -262,8 +253,16 @@ function validateOperationRequired(
 
 async function legacyGetTools(provider: ToolProvider, options?: GetToolsOptions): Promise<GetToolsResult> {
   const { tools } = await readProviderTools(provider);
-  const rawTools = Array.isArray(tools) ? tools : [];
-  return applyCacheMarkers(rawTools, provider, options?.cache === true);
+  // Fail fast on malformed provider artifacts so agents don't silently boot
+  // with zero tools. Matches the pre-presets behavior of the public
+  // `listTools` path (TOOLS_ASSET_INVALID).
+  if (!Array.isArray(tools)) {
+    throw new SuperDocCliError('Tool provider bundle is missing tools array.', {
+      code: 'TOOLS_ASSET_INVALID',
+      details: { provider },
+    });
+  }
+  return applyCacheMarkers(tools, provider, options?.cache === true);
 }
 
 async function legacyGetCatalog(): Promise<ToolCatalog> {
@@ -316,8 +315,7 @@ async function legacyDispatch(
   }
 
   const catalog = await getCachedCatalog();
-  const entries = catalog.tools as ToolCatalogEntry[];
-  const tool = entries.find((t) => t.toolName === toolName);
+  const tool = catalog.tools.find((t) => t.toolName === toolName);
   if (tool == null) {
     throw new SuperDocCliError(`Unknown tool: ${toolName}`, {
       code: 'TOOL_DISPATCH_NOT_FOUND',

packages/sdk/langs/node/src/tools.ts

@@ -16,11 +16,13 @@ import {
   listPresets,
   type CacheStrategy,
   type ToolCatalog,
+  type ToolCatalogEntry,
+  type ToolCatalogOperation,
   type ToolProvider,
 } from './presets.js';
 
 export { DEFAULT_PRESET, getPreset, listPresets };
-export type { CacheStrategy, ToolCatalog, ToolProvider };
+export type { CacheStrategy, ToolCatalog, ToolCatalogEntry, ToolCatalogOperation, ToolProvider };
 
 // ---------------------------------------------------------------------------
 // chooseTools — provider-shaped tool list with optional cache markers

packages/sdk/langs/python/superdoc/presets/legacy.py

@@ -147,8 +147,16 @@ def _legacy_get_tools(provider: ToolProvider, *, cache: bool = False) -> Dict[st
         raise SuperDocError('provider is required.', code='INVALID_ARGUMENT', details={'provider': provider})
     provider_file = _read_json_asset(_PROVIDER_FILE[provider])
     tools = provider_file.get('tools')
-    raw_tools = tools if isinstance(tools, list) else []
-    return _apply_cache_markers(cast(List[Any], raw_tools), provider, cache)
+    # Fail fast on malformed provider artifacts so agents don't silently boot
+    # with zero tools. Matches the Node legacy preset's behavior and the
+    # pre-presets contract of the public list_tools path.
+    if not isinstance(tools, list):
+        raise SuperDocError(
+            'Tool provider bundle is missing tools array.',
+            code='TOOLS_ASSET_INVALID',
+            details={'provider': provider},
+        )
+    return _apply_cache_markers(cast(List[Any], tools), provider, cache)
 
 
 def _legacy_get_catalog() -> Dict[str, Any]:

packages/sdk/langs/python/superdoc/tools_api.py

@@ -77,7 +77,12 @@ def choose_tools(input: ToolChooserInput) -> Dict[str, Any]:
             details={'provider': provider},
         )
 
-    preset_id = input.get('preset') or DEFAULT_PRESET
+    # Default only when `preset` is absent. An explicit empty string is passed
+    # through to get_preset() so it raises PRESET_NOT_FOUND, matching Node/MCP
+    # fail-fast behavior. Using `or DEFAULT_PRESET` would silently treat
+    # `preset: ''` as legacy and hide misconfiguration.
+    preset_arg = input.get('preset')
+    preset_id = preset_arg if preset_arg is not None else DEFAULT_PRESET
     cache_requested = bool(input.get('cache'))
 
     preset = get_preset(preset_id)

packages/sdk/langs/python/tests/test_presets.py

@@ -57,6 +57,21 @@ def test_get_preset_nonexistent_raises_preset_not_found():
     assert 'legacy' in excinfo.value.details['availablePresets']
 
 
+def test_get_preset_empty_string_raises_preset_not_found():
+    """Empty string is NOT the default — it must fail fast like Node."""
+    with pytest.raises(SuperDocError) as excinfo:
+        get_preset('')
+    assert excinfo.value.code == 'PRESET_NOT_FOUND'
+
+
+def test_choose_tools_empty_preset_raises_preset_not_found():
+    """Cross-lang parity with Node: chooseTools({preset: ''}) must throw, not
+    silently use legacy."""
+    with pytest.raises(SuperDocError) as excinfo:
+        choose_tools({'provider': 'openai', 'preset': ''})
+    assert excinfo.value.code == 'PRESET_NOT_FOUND'
+
+
 # ---------------------------------------------------------------------------
 # choose_tools — default preset equivalence
 # ---------------------------------------------------------------------------