feat: extract reusable CLI package primitives - #784

packages/mosaic/src/index.ts (1)

447-447: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the new root re-export with a public-entrypoint test.

The current test only imports from ../src/ai, so it doesn't lock in the new packages/mosaic/src/index.ts wiring. Add one assertion through the package root entrypoint to keep this API covered.

🤖 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/index.ts` at line 447, The new root re-export in the
package entrypoint is not covered by tests, so add a public-entrypoint assertion
for createDuckDbDatabaseAiAdapter through the main index export. Update the
existing public API test suite to import from the package root (the module that
contains the createDuckDbDatabaseAiAdapter re-export) and verify the symbol is
available there, alongside the current ../src/ai coverage.

Source: Coding guidelines

packages/app-runtime/src/HtmlAppRevisionCommands.ts (1)

36-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

.removeDefault() is a deprecated legacy alias in Zod 4.

Confirmed via Zod docs: in Zod 4, .removeDefault() on ZodDefault still works but is a "Legacy method (deprecated)"; .unwrap() is the current equivalent. Not a functional bug, but worth migrating to avoid relying on a deprecated API.

♻️ Proposed fix

 const HtmlAppRestoreRevisionCommandInput =
-  HtmlAppRevisionCommandInput.removeDefault().extend({
+  HtmlAppRevisionCommandInput.unwrap().extend({
     revisionId: z.string().describe('Target revision id for restore.'),
   });

🤖 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/app-runtime/src/HtmlAppRevisionCommands.ts` around lines 36 - 39,
The `HtmlAppRestoreRevisionCommandInput` schema is using the deprecated Zod 4
legacy alias `.removeDefault()`. Update the `HtmlAppRevisionCommandInput`
transformation to use `.unwrap()` instead, keeping the same schema shape while
removing reliance on the deprecated API.

packages/documents/__tests__/createBlockDocumentCommandAiAdapter.test.ts (1)

71-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add coverage for the failure and validation branches.

Current tests only cover the happy path and a compatible-artifact predicate. Missing coverage for: addBlock/ensureBlockDocument throwing when the artifact type doesn't match isBlockDocumentArtifact, and addBlock throwing when result.success is false.

As per coding guidelines, "Add or update tests when changing behavior or public API, when practical."

🤖 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/documents/__tests__/createBlockDocumentCommandAiAdapter.test.ts`
around lines 71 - 111, Add tests for the missing failure paths in
createBlockDocumentCommandAiAdapter: verify addBlock and ensureBlockDocument
reject when the store artifact type does not satisfy isBlockDocumentArtifact,
and verify addBlock throws when invokeCommand returns a result with success set
to false. Extend the existing createMockStore-based setup in
createBlockDocumentCommandAiAdapter.test.ts to simulate these cases, and assert
the errors are surfaced from the adapter methods. Keep the happy-path
expectations intact while covering the validation and command-failure branches.

Source: Coding guidelines

apps/sqlrooms-cli-ui/src/ai/createAddHtmlAppBlockDocumentBlockTool.ts (1)

46-53: 🗄️ Data Integrity & Integration | 🔵 Trivial

Unsafe as cast hides a field mismatch between HtmlAppBlockDocumentBlock and BlockDocumentStatefulBlockBlock.

HtmlAppBlockDocumentBlock (from @sqlrooms/app-runtime) includes an intent field that BlockDocumentStatefulBlockBlock doesn't declare (see companion comment on html-app-block-document.ts). The cast type-checks only because extra properties don't break structural subtyping, but it silently papers over the fact that the two types aren't actually the same shape. If any downstream consumer re-validates against the zod schema, intent would be dropped without a compile-time signal here.

Consider a purpose-built return type (or narrowing/validating explicitly) instead of an as cast, so a future schema/field drift surfaces as a type error rather than a silent runtime behavior change.

🤖 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 `@apps/sqlrooms-cli-ui/src/ai/createAddHtmlAppBlockDocumentBlockTool.ts` around
lines 46 - 53, The unsafe cast in createAddHtmlAppBlockDocumentBlockTool is
masking a shape mismatch between HtmlAppBlockDocumentBlock and
BlockDocumentStatefulBlockBlock. Remove the blanket as assertion and make the
return type explicit by either returning the exact runtime block shape from
createHtmlAppRuntimeBlockDocumentBlock or introducing a dedicated
mapped/validated return type that includes the intent field if it is meant to
survive. Use the createHtmlAppRuntimeBlockDocumentBlock result and the block
return type in this module to ensure any future schema drift becomes a
compile-time error instead of being silently dropped.

Fix all unresolved CodeRabbit comments on this PR:

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

packages/documents/__tests__/createBlockDocumentCommandAiAdapter.test.ts (1)

8-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Missing coverage for setCurrentBlockDocument.

The mock store already tracks setCurrentArtifact (Line 20, exposed at Line 67) but no test ever calls adapter.setCurrentBlockDocument or asserts it delegates to store.getState().artifacts.setCurrentArtifact. Since this is one of the four public methods introduced by the new adapter factory, it's worth adding a small test for parity with the other three methods.

✅ Suggested test addition

+  it('delegates setCurrentBlockDocument to the artifacts store', () => {
+    const {store} = createMockStore();
+    const adapter = createBlockDocumentCommandAiAdapter({store});
+    const spy = jest.spyOn(store.getState().artifacts, 'setCurrentArtifact');
+
+    adapter.setCurrentBlockDocument('doc-1');
+
+    expect(spy).toHaveBeenCalledWith('doc-1');
+  });

As per coding guidelines, "Add or update tests when changing behavior or public API, when practical."

Also applies to: 71-155

🤖 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/documents/__tests__/createBlockDocumentCommandAiAdapter.test.ts`
around lines 8 - 69, Add test coverage for the adapter’s setCurrentBlockDocument
public method by using the existing createMockStore helper and asserting it
calls store.getState().artifacts.setCurrentArtifact; the mock already exposes
setCurrentArtifact but no test exercises this delegation. Update the
createBlockDocumentCommandAiAdapter test suite to invoke
adapter.setCurrentBlockDocument and verify the underlying artifact state setter
is called with the expected block document/artifact input, alongside the other
adapter method parity checks.

Source: Coding guidelines

packages/app-runtime/__tests__/HtmlAppRevisionCommands.test.ts (1)

127-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Rename tests still skip the files-provided branch.

shouldCommitRevision = files || (metadata && previousTitle !== trimmedTitle) in the implementation has a distinct path when files is supplied (forces a commit even without metadata/title change), but no test exercises it here.

As per coding guidelines, "Add or update tests when changing behavior or public API, when practical."

it('commits a revision when files are provided even without a title change', async () => {
  const {commands, state, renameHtmlApp, commitHtmlAppRevision} =
    createState();

  const result = await getCommand(
    commands,
    HTML_APP_RENAME_COMMAND_ID,
  ).execute(createCommandContext(state), {
    appId: 'app-1',
    title: 'App',
    files: {'/index.html': '<html><title>App v2</title></html>'},
  });

  expect(renameHtmlApp).not.toHaveBeenCalled();
  expect(commitHtmlAppRevision).toHaveBeenCalled();
  expect(result).toMatchObject({success: true, commandId: HTML_APP_RENAME_COMMAND_ID});
});

🤖 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/app-runtime/__tests__/HtmlAppRevisionCommands.test.ts` around lines
127 - 215, The rename command tests still miss the branch where files force a
revision commit even when the title does not change. Add a test alongside the
existing HTML_APP_RENAME_COMMAND_ID cases that executes the command with files
provided and the same title, then assert renameHtmlApp is not called,
commitHtmlAppRevision is called, and the result is successful. Use createState
and getCommand to cover the files-driven path in the rename command
implementation.

Source: Coding guidelines