feat: Deck map ai tool - #654

packages/mosaic/src/ai.ts (2)

172-172: 💤 Low value

Implicit naming contract on create_dashboard_map.

The prompt text hardcodes the expected tool name create_dashboard_map, but extraTools lets hosts register tools under any key. If a host names its map tool differently, these instructions become misleading. Consider documenting the expected key in the extraTools JSDoc, or exposing a small constant/registration helper so the prompt and the tool key stay in sync.

Also applies to: 197-197

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/mosaic/src/ai.ts` at line 172, The prompt text hardcodes the tool
name create_dashboard_map while hosts can register tools under any key via
extraTools; update the code to avoid the implicit naming contract by introducing
a single exported constant (e.g., MAP_TOOL_KEY) or a small registration helper
(e.g., registerMapTool) and reference that symbol in the prompt text instead of
the literal string, and add JSDoc on extraTools to document the expected
key/registration helper so prompt and tool key stay in sync (also apply the same
change for the other occurrence noted around line 197).

---

485-491: 💤 Low value

Host tools can silently override built-in tools.

...hostTools is spread after the built-in entries, so a host returning a key like create_dashboard_artifact or any chart tool name will shadow the Mosaic-owned implementation without warning. If override is intentional, fine; otherwise consider spreading hostTools first (so built-ins win), or detecting collisions and warning. Worth a quick decision since this is a public extension point.

Optional defensive variant

-  const hostTools = extraTools?.(deps) ?? {};
+  const hostTools = extraTools?.(deps) ?? {};
+  if (process.env.NODE_ENV !== 'production') {
+    for (const key of Object.keys(hostTools)) {
+      if (key in builtInToolKeys) {
+        console.warn(`[mosaic] extraTools "${key}" overrides a built-in dashboard tool.`);
+      }
+    }
+  }

Also applies to: 524-524

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/mosaic/src/ai.ts` around lines 485 - 491, Host-provided tools
(hostTools from extraTools) are currently spread after built-in entries in
CreateDashboardAiToolsOptions (in the function creating chartTools and
hostTools), allowing silent overrides of built-in tools like
create_dashboard_artifact or any chart tool; change the merge order or add
collision detection: either spread hostTools before the built-ins so built-ins
win, or, inside the function that builds the final tool map (where
createChartTools, resolvedChartTypes, hostTools and chartTools are combined),
iterate hostTools keys and check for existing keys in chartTools/built-ins and
log/warn or throw if a collision is found (refer to hostTools, createChartTools,
createDashboardToolDeps, createDefaultChartTypes, and the built-in key names
such as create_dashboard_artifact) and choose the desired behavior (prevent
override by default or allow with explicit warning).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/deck/__tests__/dashboard-ai-map.test.ts (1)

140-157: ⚡ Quick win

Consider strengthening the semicolon-stripping assertion.

The current assertion only checks the wrapped subquery still appears in the SQL but does not actually prove trailing semicolons were stripped — e.g. FROM (SELECT * FROM earthquakes; ;) AS "..." would also pass toContain(...). Asserting the absence of ; inside the parenthesized inner SQL (or matching the full normalized substring) would make the test meaningful for the stated behavior.

♻️ Proposed tightening

-    expect(
-      deps.panels[0].config.datasets.earthquakes.source.sqlQuery,
-    ).toContain(
-      'FROM (SELECT * FROM earthquakes) AS "__sqlrooms_dashboard_map_source"',
-    );
+    const wrappedSql: string =
+      deps.panels[0].config.datasets.earthquakes.source.sqlQuery;
+    expect(wrappedSql).toContain(
+      'FROM (SELECT * FROM earthquakes) AS "__sqlrooms_dashboard_map_source"',
+    );
+    expect(wrappedSql).not.toMatch(/;\s*\)\s*AS\s+"__sqlrooms_dashboard_map_source"/);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/deck/__tests__/dashboard-ai-map.test.ts` around lines 140 - 157, The
test 'strips trailing semicolons from wrapped source SQL queries' currently only
checks that the wrapped subquery string exists but not that trailing semicolons
were removed; update the assertion on
deps.panels[0].config.datasets.earthquakes.source.sqlQuery (after calling (tool
as any).execute with createDeckMapDashboardTool(createDeps())) to either (a)
assert the exact normalized wrapped substring equals FROM (SELECT * FROM
earthquakes) AS "__sqlrooms_dashboard_map_source" or (b) assert that there is no
';' character inside the parenthesized inner SQL (i.e., within the parentheses
following FROM), so the test proves trailing semicolons were stripped. Ensure
you reference the same fields used now:
deps.panels[0].config.datasets.earthquakes.source.sqlQuery and the execute call
on tool.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/deck/src/mapConfigUtils.ts (1)

126-131: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use the cleaned SQL value to decide subquery wrapping.

Line 129 checks the raw options.sourceSqlQuery, but Line 130 injects cleanedSourceSqlQuery. Inputs like " ; " become empty after cleaning and produce invalid SQL (FROM () AS ...).

Proposed fix

   const cleanedSourceSqlQuery = options.sourceSqlQuery
     ?.trim()
     .replace(/(?:\s*;+\s*)+$/, '');
-  const baseSource = options.sourceSqlQuery
+  const baseSource = cleanedSourceSqlQuery
     ? `(${cleanedSourceSqlQuery}) AS "__sqlrooms_dashboard_map_source"`
     : quoteDeckMapSqlTableReference(options.tableReference);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/deck/src/mapConfigUtils.ts` around lines 126 - 131, The code uses
options.sourceSqlQuery to decide whether to wrap a subquery but inserts
cleanedSourceSqlQuery into the final SQL, which can be empty (e.g., " ; ") and
produce invalid SQL; change the decision to use the cleaned value: compute
cleanedSourceSqlQuery first, then set baseSource to `(${cleanedSourceSqlQuery})
AS "__sqlrooms_dashboard_map_source"` only when cleanedSourceSqlQuery is
non-empty (truthy), otherwise fall back to
quoteDeckMapSqlTableReference(options.tableReference); reference
cleanedSourceSqlQuery, options.sourceSqlQuery, baseSource, and
quoteDeckMapSqlTableReference when making this change.

packages/mosaic/src/chart-runtime.ts (1)

118-129: ⚡ Quick win

Make disabled override semantics explicit.

At Line 118, only truthy override.disabled is handled, so disabled: false currently behaves like “not provided” and inherits the base value. If this is intended as a true override object, it should either apply explicit false or narrow the type to a one-way flag (disabled?: true).

Suggested adjustment (support explicit false override)

-  if (override.disabled) {
-    return {
-      ...(basePolicy ?? {}),
-      disabled: true,
-    };
-  }
-
   return {
     ...(basePolicy ?? {}),
+    ...(override.disabled !== undefined ? {disabled: override.disabled} : {}),
     ...(override.maxRows !== undefined ? {maxRows: override.maxRows} : {}),
     ...(override.reason !== undefined ? {reason: override.reason} : {}),
   };

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/mosaic/src/chart-runtime.ts` around lines 118 - 129, The current
merge logic in chart-runtime.ts treats override.disabled truthily, so a consumer
cannot explicitly set disabled: false; update the merge in the function that
constructs the final policy (the block currently returning ...(basePolicy ??
{}), ...(override.maxRows...), etc.) to treat disabled as an explicit override
by checking override.disabled !== undefined and, when present, setting disabled:
override.disabled; alternatively (if you prefer one-way flag semantics) change
the override type to only allow disabled?: true and keep the existing truthy
check—apply the first option to preserve explicit false semantics on the
returned policy.

packages/deck/src/ai.ts (1)

101-129: 💤 Low value

Duplicate reasoning field in schema extension.

DeckMapDashboardToolParameters extends DeckMapConfigToolParameters which already defines a reasoning field (lines 92-94). The extension redefines reasoning again at lines 126-128, which shadows the inherited field. While Zod will use the extended definition, this redundancy is confusing and may cause maintenance issues.

♻️ Remove the duplicate reasoning field

 export const DeckMapDashboardToolParameters =
   DeckMapConfigToolParameters.extend({
     artifactId: z
       .string()
       .optional()
       .describe(
         'Optional dashboard artifact ID. Defaults to current dashboard.',
       ),
     tableName: z
       .string()
       .optional()
       .describe(
         'Optional table name used only to select/resolve the target dashboard table. Data sources still come from config.datasets.',
       ),
     createArtifactIfMissing: z
       .boolean()
       .optional()
       .default(true)
       .describe('If true, create dashboard artifact if missing.'),
     panelId: z
       .string()
       .optional()
       .describe(
         'Optional panel ID. If provided, updates the existing map panel instead of creating a new one.',
       ),
-    reasoning: z
-      .string()
-      .describe('Brief rationale for creating the map panel.'),
   });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/deck/src/ai.ts` around lines 101 - 129,
DeckMapDashboardToolParameters currently redefines the reasoning field that is
already declared on DeckMapConfigToolParameters; remove the duplicate reasoning
entry from the DeckMapDashboardToolParameters schema so the inherited reasoning
from DeckMapConfigToolParameters is used (leave other fields like artifactId,
tableName, createArtifactIfMissing, and panelId intact and unchanged).