Repository navigation
feat: skills subsystem + authoring wizard in @sqlrooms/ai - #574
Merged
Merged
Conversation
Set `types: []` in packages/deck/tsconfig.json so TypeScript does not automatically include every `@types/*` package found by walking up the node_modules tree. Explicit imports in source are unaffected. This avoids collisions when a legacy ambient deck.gl type declaration exists higher up in the module resolution path, which can shadow the real `@deck.gl/core` types and break named exports such as `DeckProps`.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a skills subsystem (manifest parsing/validation, storage interfaces and implementations, composite storage, error types), an authoring flow (draft store, authoring tools, agent, UI), runtime skill execution (querySQL simulation, runSkill), AgentChat UI components, example app wiring with seeded skills/in-memory storage, and README/package/tsconfig updates. ChangesSkills Subsystem and Agent Chat UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mihajlo-stojkovic
marked this pull request as ready for review
May 7, 2026 13:50
- Fix IPC serialization: replace open index signature in SkillErrorContext with JsonValue-constrained extras field - Fix type safety: make SkillAuthoringContext array fields readonly to prevent post-construction mutation - Fix efficiency: rewrite loadSkillFromFiles to single-pass, pre-compile forbidden identifier regexes in tools - Fix UX bug: useStickyScroll now gates auto-scroll on wasAtBottom to avoid interrupting user scroll - Fix silent failures: add error logging to parseUiMessages, refreshSkillListings, runSkillTool execute - Fix redundant state: remove Immer from flat createSkillDraftStore setters, use plain Zustand set - Fix duplicate code: extract checkForbidden helper in tools.ts, use existing isToolPart/isDynamicToolPart guards - Fix InMemorySkillStorage: path-traversal guard, correct error type for unknown root, remove double validation, cache parsed manifests - Fix example: deduplicate getModel in WeatherAgent, remove redundant nonce state in SkillsButton, fix gpt-4.1 fallback model - Export SKILL_AUTHORING_TOOL_NAMES constants; replace string literals in SkillAuthoringPanel - Add Skills and AgentChat sections to @sqlrooms/ai and @sqlrooms/ai-core READMEs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
packages/ai-core/src/components/agent-chat/MessageView.tsx (1)
8-8: 💤 Low valueInline or drop the empty
MARKDOWN_COMPONENTSconstant.
MARKDOWN_COMPONENTSis{}and only used once. Either drop the prop onReactMarkdown(default behavior is identical) or inline it; the named constant suggests there's customization here when there isn't. If it's a placeholder for future overrides, consider a brief comment to that effect.🤖 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/ai-core/src/components/agent-chat/MessageView.tsx` at line 8, The empty MARKDOWN_COMPONENTS constant in MessageView.tsx is unused and misleading; remove the constant and its use by deleting MARKDOWN_COMPONENTS and removing the components={MARKDOWN_COMPONENTS} prop from the ReactMarkdown invocation (ReactMarkdown defaults are sufficient), or if you want to keep a placeholder, inline {} directly as components={{}} on the ReactMarkdown call and add a brief comment indicating it's intentional for future overrides; update the ReactMarkdown usage accordingly.examples/ai-agent/src/store.ts (1)
27-49: ⚖️ Poor tradeoffConsider hoisting
skillStorageand listings into the store.Per coding guidelines, app-level state that needs to coordinate across panels is preferably modeled in the room Zustand store rather than module-level singletons. Putting
skillStorageandcachedListingson a slice would also letgetInstructionsread fromget()instead of a closed-over module variable, and keeps lifecycle/teardown explicit. Current setup works for the example but is something to keep in mind if this graduates beyond the demo.As per coding guidelines: "Use the room Zustand store when state is app-level, cross-panel, or needs to coordinate with other room features".
🤖 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 `@examples/ai-agent/src/store.ts` around lines 27 - 49, Hoist the module-level skillStorage and cachedListings into the room Zustand store by adding them to a slice (e.g., include skillStorage: InMemorySkillStorage and cachedListings: SkillListing[] in the slice state and a refreshSkillListings action), initialize skillStorage inside the slice constructor, call refreshSkillListings once after creation, and subscribe to skillStorage within the slice to update cachedListings via set(); then update any callers (for example getInstructions) to read cachedListings from get() instead of the closed-over module variable so lifecycle and teardown are managed by the store.examples/ai-agent/src/components/SkillsButton.tsx (1)
87-91: 💤 Low valueBound the collision-resolution loop.
while (true)is safe in practice (each iteration incrementscounter, andslugifyalways returns a non-empty base), but a defensive cap avoids an unbounded await loop shouldresolveSkillIdever incorrectly return truthy for unknown IDs. A smallMAX_ATTEMPTSwith a thrown error is cheap insurance.🤖 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 `@examples/ai-agent/src/components/SkillsButton.tsx` around lines 87 - 91, The collision-resolution loop using while(true) must be bounded; replace it with a capped loop (e.g., define a MAX_ATTEMPTS constant and iterate up to that many times while calling skillStorage.resolveSkillId to check candidateId), incrementing counter and updating candidateId as now, and if MAX_ATTEMPTS is exceeded throw a clear error (or reject) mentioning the baseId and that unique ID allocation failed; update the code around candidateId, baseId, counter and the call to skillStorage.resolveSkillId to use this bounded approach.packages/ai/src/skills/storage.ts (1)
69-117: ⚖️ Poor tradeoffConsider adding
subscribeto theSkillStorageinterface for UI reactivity.
InMemorySkillStorageexposes asubscribe(listener) → unsubscribemethod that the example app UI depends on for reactive listings afterwriteSkill/deleteSkill. Becausesubscribeis not on the interface, any component that calls it is implicitly coupled to the concreteInMemorySkillStoragetype rather than theSkillStorageabstraction. If other backends (OPFS, HTTP) are expected to drive reactive UIs, elevatingsubscribeto the interface would keep callers backend-agnostic.🤖 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/ai/src/skills/storage.ts` around lines 69 - 117, The SkillStorage interface lacks a subscribe/unsubscribe API which InMemorySkillStorage currently exposes, causing consumers to depend on the concrete InMemorySkillStorage instead of the abstraction; add a subscribe method to the SkillStorage interface (e.g., subscribe(listener: () => void): () => void) so callers can register a listener that is invoked after mutations (writeSkill/deleteSkill) and receive an unsubscribe function, and then implement/forward that behavior in InMemorySkillStorage (and any other backends) so UI code can remain backend-agnostic.packages/ai-core/src/components/agent-chat/index.tsx (1)
139-150: 💤 Low value
useMessagesObserverfires on mount — consider clarifying the JSDoc.The effect at line 147 fires on initial mount with
messages=initialMessages ?? [], soonMessagesChangeis called immediately even before any user interaction. The comment says "on every stream delta" but the actual behavior is "on every message-array change, including mount." This won't break anything (consumers tolerating redundant calls is safe), but the JSDoc is slightly misleading for hosts that only want post-send updates.📝 Proposed JSDoc clarification
-/** - * Fire `onMessagesChange` on every message delta without re-subscribing when - * the callback identity churns. Ref-guarded so hosts can pass inline - * functions without triggering redundant calls. - */ +/** + * Fire `onMessagesChange` whenever the messages array reference changes, + * including on initial mount with the seed messages. Ref-guarded so hosts + * can pass inline functions without triggering redundant calls. + */🤖 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/ai-core/src/components/agent-chat/index.tsx` around lines 139 - 150, Update the JSDoc for useMessagesObserver to accurately state that the effect runs on initial mount and whenever the messages array changes (not just "stream deltas"), so onMessagesChange will be invoked immediately with the initial messages (initialMessages ?? []) and on every subsequent messages array update; reference the function name useMessagesObserver and its parameters (messages, onMessagesChange) in the comment so consumers know this includes the initial render and can opt out if they only want post-send updates.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@examples/ai-agent/src/skills/InMemorySkillStorage.ts`:
- Around line 128-146: The extraFiles loop must also guard against reserved
filenames to prevent duplicates; update the loop in InMemorySkillStorage (where
content.extraFiles is iterated) to throw a SkillManifestError (including
{rootId}) if any file.relativePath equals 'skill.yaml' or 'SKILL.md' (avoid
case/whitespace surprises by normalizing, e.g., trim and toLowerCase) before
constructing the files array that contains
serializeSkillManifest(content.manifest) and SKILL.md so reserved entries cannot
be shadowed.
In `@examples/ai-agent/src/store.ts`:
- Around line 34-49: The current refreshSkillListings() suffers from race
conditions that let out-of-order async calls overwrite cachedListings; change
refreshSkillListings to use a monotonic sequence token (e.g., incrementing
requestId) captured per invocation and only assign cachedListings if the token
matches the latest seen token, and ensure the initial call is awaited (or its
token respected) so any prompt build sees up-to-date data; update the subscribe
handler to call the guarded refreshSkillListings() (and not fire-and-forget
without sequencing) so only the most recent fetch wins and stale results are
discarded.
In `@packages/ai-core/README.md`:
- Around line 105-118: The README example incorrectly uses the non-existent
createToolLoopAgent factory; replace its usage with instantiation of the
ToolLoopAgent class by using new ToolLoopAgent(...) and pass the same config
(model, system, tools) as the constructor argument; update the import/examples
to reference ToolLoopAgent instead of createToolLoopAgent so the sample compiles
with ai v6.
In `@packages/ai-core/src/components/agent-chat/ChatInput.tsx`:
- Around line 26-33: The Enter key handler (handleKeyDown) currently calls
onSend(value) unconditionally and bypasses the same guards the send button uses;
update handleKeyDown (and the other Enter handler around the 51-56 block) to
perform the same checks as the send button: return early if isStreaming is true
or if the input is empty/whitespace, and only call onSend(value) when those
guards pass — or better, delegate to the same send helper (e.g.,
sendMessage/sendHandler) used by the button so both paths share identical
validation logic.
In `@packages/ai-core/src/components/agent-chat/useAgentChatTransport.ts`:
- Around line 35-48: The parseUiMessages function accepts BodyInit (which can be
Blob/FormData/ArrayBuffer/etc.) but currently calls String(body) and JSON.parse,
producing "[object Blob]" for non-strings; update parseUiMessages to first check
typeof body === "string" before attempting JSON.parse, and if it's not a string
log a clear warning that includes the runtime type (use body?.constructor?.name
or typeof) and return []; keep the existing try/catch for JSON.parse for string
cases to preserve current error logging.
In `@packages/ai/src/skills/authoring/createSkillDraftStore.ts`:
- Around line 8-13: SkillDraft.author is currently required but the manifest
schema and tools (WriteManifestInput, patchManifest) treat it as optional,
causing undefined to be stored while the TS type claims it's string; change the
SkillDraft interface to make author optional (author?: string) and update
initialDraft to set author to undefined (or omit it) for consistency, and in
createSaveSkillTool ensure when building the draft passed to onSave you
normalize the value (use state.author ?? '' or similar) so callers never receive
an unexpectedly undefined string field; update references in
createSaveSkillTool, initialDraft, and any code constructing SkillDraft to match
the optional author type.
In `@packages/ai/src/skills/authoring/SkillDraftPreview.tsx`:
- Around line 27-44: Update the manifest-content detection to include the author
field: modify the hasManifestContent calculation (used to decide rendering of
ManifestCard vs placeholder) to also consider author (e.g., include author in
the Boolean/truthy check, optionally using .trim() if author is a string) so an
author-only draft renders ManifestCard; refer to hasManifestContent and
ManifestCard (props: name, description, author) when making the change.
In `@packages/ai/src/skills/manifest.ts`:
- Around line 27-38: The schema uses the old Zod v3 `{ message: '...' }` error
option (seen on id/version regex and name/description min calls); update all
custom error objects to Zod v4's `{ error: '...' }` form for id, version, name,
and description (e.g. the z.string().regex(KEBAB_CASE, {...}) and
z.string().min(1, {...}) calls) so validation messages follow Zod 4 conventions
and avoid the deprecated API.
---
Nitpick comments:
In `@examples/ai-agent/src/components/SkillsButton.tsx`:
- Around line 87-91: The collision-resolution loop using while(true) must be
bounded; replace it with a capped loop (e.g., define a MAX_ATTEMPTS constant and
iterate up to that many times while calling skillStorage.resolveSkillId to check
candidateId), incrementing counter and updating candidateId as now, and if
MAX_ATTEMPTS is exceeded throw a clear error (or reject) mentioning the baseId
and that unique ID allocation failed; update the code around candidateId,
baseId, counter and the call to skillStorage.resolveSkillId to use this bounded
approach.
In `@examples/ai-agent/src/store.ts`:
- Around line 27-49: Hoist the module-level skillStorage and cachedListings into
the room Zustand store by adding them to a slice (e.g., include skillStorage:
InMemorySkillStorage and cachedListings: SkillListing[] in the slice state and a
refreshSkillListings action), initialize skillStorage inside the slice
constructor, call refreshSkillListings once after creation, and subscribe to
skillStorage within the slice to update cachedListings via set(); then update
any callers (for example getInstructions) to read cachedListings from get()
instead of the closed-over module variable so lifecycle and teardown are managed
by the store.
In `@packages/ai-core/src/components/agent-chat/index.tsx`:
- Around line 139-150: Update the JSDoc for useMessagesObserver to accurately
state that the effect runs on initial mount and whenever the messages array
changes (not just "stream deltas"), so onMessagesChange will be invoked
immediately with the initial messages (initialMessages ?? []) and on every
subsequent messages array update; reference the function name
useMessagesObserver and its parameters (messages, onMessagesChange) in the
comment so consumers know this includes the initial render and can opt out if
they only want post-send updates.
In `@packages/ai-core/src/components/agent-chat/MessageView.tsx`:
- Line 8: The empty MARKDOWN_COMPONENTS constant in MessageView.tsx is unused
and misleading; remove the constant and its use by deleting MARKDOWN_COMPONENTS
and removing the components={MARKDOWN_COMPONENTS} prop from the ReactMarkdown
invocation (ReactMarkdown defaults are sufficient), or if you want to keep a
placeholder, inline {} directly as components={{}} on the ReactMarkdown call and
add a brief comment indicating it's intentional for future overrides; update the
ReactMarkdown usage accordingly.
In `@packages/ai/src/skills/storage.ts`:
- Around line 69-117: The SkillStorage interface lacks a subscribe/unsubscribe
API which InMemorySkillStorage currently exposes, causing consumers to depend on
the concrete InMemorySkillStorage instead of the abstraction; add a subscribe
method to the SkillStorage interface (e.g., subscribe(listener: () => void): ()
=> void) so callers can register a listener that is invoked after mutations
(writeSkill/deleteSkill) and receive an unsubscribe function, and then
implement/forward that behavior in InMemorySkillStorage (and any other backends)
so UI code can remain backend-agnostic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33479267-056d-4c90-a27f-cd4f1943b687
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (40)
examples/ai-agent/README.mdexamples/ai-agent/package.jsonexamples/ai-agent/src/agents/WeatherAgent.tsexamples/ai-agent/src/components/MainView.tsxexamples/ai-agent/src/components/SkillsButton.tsxexamples/ai-agent/src/skills/InMemorySkillStorage.tsexamples/ai-agent/src/skills/getModel.tsexamples/ai-agent/src/skills/querySqlTool.tsexamples/ai-agent/src/skills/runSkillTool.tsexamples/ai-agent/src/skills/seedSkills.tsexamples/ai-agent/src/skills/skillPrompt.tsexamples/ai-agent/src/store.tspackages/ai-core/README.mdpackages/ai-core/src/components/FlatAgentRenderer.tsxpackages/ai-core/src/components/agent-chat/ChatInput.tsxpackages/ai-core/src/components/agent-chat/MessageView.tsxpackages/ai-core/src/components/agent-chat/SuggestionStrip.tsxpackages/ai-core/src/components/agent-chat/ToolActivityLine.tsxpackages/ai-core/src/components/agent-chat/index.tsxpackages/ai-core/src/components/agent-chat/toolCallAdapter.tspackages/ai-core/src/components/agent-chat/useAgentChatTransport.tspackages/ai-core/src/components/agent-chat/useStickyScroll.tspackages/ai-core/src/index.tspackages/ai/README.mdpackages/ai/package.jsonpackages/ai/src/index.tspackages/ai/src/skills/authoring/SkillAuthoringAgent.tspackages/ai/src/skills/authoring/SkillAuthoringContext.tspackages/ai/src/skills/authoring/SkillAuthoringPanel.tsxpackages/ai/src/skills/authoring/SkillDraftPreview.tsxpackages/ai/src/skills/authoring/createSkillDraftStore.tspackages/ai/src/skills/authoring/index.tspackages/ai/src/skills/authoring/systemPrompt.tspackages/ai/src/skills/authoring/tools.tspackages/ai/src/skills/errors.tspackages/ai/src/skills/index.tspackages/ai/src/skills/manifest.tspackages/ai/src/skills/storage.tspackages/deck/package.jsonpackages/deck/tsconfig.json
💤 Files with no reviewable changes (1)
- packages/deck/package.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@examples/ai-agent/src/skills/InMemorySkillStorage.ts`:
- Around line 126-160: The writeSkill implementation in InMemorySkillStorage
silently overwrites an existing skill and doesn't verify the caller-provided id
matches the manifest id; update async writeSkill to first validate that
content.manifest.id (or content.manifest?.id) equals the id parameter (or set
manifest.id = id if intended) and then check rootMap.has(id) before writing—if a
skill already exists, throw a clear SkillManifestError (or a ConflictError) to
prevent silent overwrite; perform these checks prior to creating the files array
and only call rootMap.set(id, ...) and this.notify() after validation succeeds
to ensure no accidental data loss or id mismatch.
- Around line 76-78: The listRoots method currently returns the ROOTS array by
reference which allows callers to mutate stored SkillRoot objects; modify
InMemorySkillStorage.listRoots to return defensive copies by mapping over ROOTS
and returning shallow-cloned entries (e.g., create new objects for each
SkillRoot) so callers cannot alter the internal ROOTS state and writable flags
remain stable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11271d66-7243-4d0f-8297-0f25825558e4
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
examples/ai-agent/src/components/SkillsButton.tsxexamples/ai-agent/src/skills/InMemorySkillStorage.tsexamples/ai-agent/src/store.tspackages/ai-core/README.mdpackages/ai-core/src/components/agent-chat/ChatInput.tsxpackages/ai-core/src/components/agent-chat/MessageView.tsxpackages/ai-core/src/components/agent-chat/useAgentChatTransport.tspackages/ai-core/src/index.tspackages/ai/src/index.tspackages/ai/src/skills/authoring/SkillAuthoringPanel.tsxpackages/ai/src/skills/authoring/SkillDraftPreview.tsxpackages/ai/src/skills/authoring/createSkillDraftStore.tspackages/ai/src/skills/authoring/index.tspackages/ai/src/skills/authoring/tools.tspackages/ai/src/skills/manifest.tspackages/cells/src/components/VegaConfigPanel.tsx
✅ Files skipped from review due to trivial changes (2)
- packages/cells/src/components/VegaConfigPanel.tsx
- packages/ai-core/README.md
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/ai/src/skills/authoring/SkillDraftPreview.tsx
- packages/ai/src/skills/authoring/createSkillDraftStore.ts
- packages/ai-core/src/components/agent-chat/useAgentChatTransport.ts
- packages/ai-core/src/index.ts
- packages/ai-core/src/components/agent-chat/MessageView.tsx
- packages/ai/src/skills/authoring/index.ts
- packages/ai-core/src/components/agent-chat/ChatInput.tsx
- packages/ai/src/index.ts
- examples/ai-agent/src/store.ts
- packages/ai/src/skills/authoring/SkillAuthoringPanel.tsx
- packages/ai/src/skills/manifest.ts
…ages and add optional subscribe method
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ai/src/skills/composite.ts (1)
19-26: ⚡ Quick winConsider parallelizing async aggregation for better I/O performance.
Both
listRoots()and the bulk path oflistSkills()currently aggregate child results sequentially. If children perform I/O, parallelizing withPromise.allwould improve performance.⚡ Proposed refactor to parallelize aggregation
async listRoots(): Promise<SkillRoot[]> { - const out: SkillRoot[] = []; - for (const child of this.children) { - const roots = await child.listRoots(); - out.push(...roots); - } - return out; + const allRoots = await Promise.all( + this.children.map((child) => child.listRoots()), + ); + return allRoots.flat(); }async listSkills(rootId?: string): Promise<SkillListing[]> { if (rootId !== undefined) { const child = await this.findChildForRoot(rootId); return child ? child.listSkills(rootId) : []; } // Duplicates (same id in multiple roots) are intentionally included so // callers can render conflict indicators. - const out: SkillListing[] = []; - for (const child of this.children) { - const listings = await child.listSkills(); - out.push(...listings); - } - return out; + const allListings = await Promise.all( + this.children.map((child) => child.listSkills()), + ); + return allListings.flat(); }Also applies to: 36-39
🤖 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/ai/src/skills/composite.ts` around lines 19 - 26, Both listRoots() and listSkills() are aggregating child results sequentially which hurts I/O-bound performance; change each to kick off all child.listRoots() / child.listSkills() calls concurrently (e.g., map children => promise and await Promise.all(promises)), then flatten the array of arrays into the final SkillRoot[]/Skill[] (use Array.prototype.flat or manual concat) so the children property is queried in parallel.
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@packages/ai/src/skills/composite.ts`:
- Around line 19-26: Both listRoots() and listSkills() are aggregating child
results sequentially which hurts I/O-bound performance; change each to kick off
all child.listRoots() / child.listSkills() calls concurrently (e.g., map
children => promise and await Promise.all(promises)), then flatten the array of
arrays into the final SkillRoot[]/Skill[] (use Array.prototype.flat or manual
concat) so the children property is queried in parallel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b066769d-f331-4b7d-8744-f2f883d87c39
📒 Files selected for processing (5)
examples/ai-agent/src/store.tspackages/ai/README.mdpackages/ai/src/skills/composite.tspackages/ai/src/skills/index.tspackages/ai/src/skills/storage.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/ai/src/skills/index.ts
- packages/ai/src/skills/storage.ts
- examples/ai-agent/src/store.ts
ilyabo
reviewed
May 18, 2026
| * (highest priority first). Children must own disjoint `rootId`s — the | ||
| * composite does not validate this; violating it makes dispatch ambiguous. | ||
| */ | ||
| export class CompositeSkillStorage implements SkillStorage { |
Collaborator
There was a problem hiding this comment.
[P2] CompositeSkillStorage is documented as a public import but is not exported from @sqlrooms/ai. The README shows import {CompositeSkillStorage} from '@sqlrooms/ai', but packages/ai/src/index.ts (line 53) only re-exports the errors, manifest helpers, storage types, and authoring APIs from ./skills. Consumers following the docs will hit “no exported member”. Add CompositeSkillStorage to the package root exports.
…or draft handling
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@packages/ai/src/skills/authoring/SkillAuthoringPanel.tsx`:
- Around line 53-55: The docblock for SkillAuthoringPanel is stale: it says the
component does not render an explicit Save button, but the implementation
renders one when onSave is provided. Update the comment above the
SkillAuthoringPanel component to reflect actual behavior: note that when the
onSave prop is passed the panel renders an explicit Save button (which calls
onSave/saveSkill flow) and otherwise relies on the agent-driven save; reference
the onSave prop and saveSkill function and the footer status pill in the
description so integrators aren't misled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c9ce8e9-31eb-41b3-91ba-c97b6426beca
📒 Files selected for processing (2)
examples/ai-agent/src/components/SkillsButton.tsxpackages/ai/src/skills/authoring/SkillAuthoringPanel.tsx
Comment on lines
+53
to
+55
| * explicit Save button — the agent calls `saveSkill` once the draft is | ||
| * complete, and the status pill in the footer communicates progress. | ||
| * |
Contributor
There was a problem hiding this comment.
Update stale save-behavior docblock.
Line 53 now contradicts the implementation: the panel does render an explicit Save button when onSave is provided. This comment will mislead integrators.
Suggested doc fix
- * explicit Save button — the agent calls `saveSkill` once the draft is
- * complete, and the status pill in the footer communicates progress.
+ * optional explicit Save button — when `onSave` is provided, the footer
+ * shows Save and updates the status pill as persistence progresses.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| * explicit Save button — the agent calls `saveSkill` once the draft is | |
| * complete, and the status pill in the footer communicates progress. | |
| * | |
| * optional explicit Save button — when `onSave` is provided, the footer | |
| * shows Save and updates the status pill as persistence progresses. | |
| * |
🤖 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/ai/src/skills/authoring/SkillAuthoringPanel.tsx` around lines 53 -
55, The docblock for SkillAuthoringPanel is stale: it says the component does
not render an explicit Save button, but the implementation renders one when
onSave is provided. Update the comment above the SkillAuthoringPanel component
to reflect actual behavior: note that when the onSave prop is passed the panel
renders an explicit Save button (which calls onSave/saveSkill flow) and
otherwise relies on the agent-driven save; reference the onSave prop and
saveSkill function and the footer status pill in the description so integrators
aren't misled.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a first-class skills subsystem to
@sqlrooms/ai: a storage abstraction for discovering/persisting skills, a split manifest format, and a prompt-to-skill authoring wizard host apps can drop into a dialog. Also lands anAgentChatprimitive in@sqlrooms/ai-corefor transient, session-less agent conversations (the wizard's chat surface), and wires the whole pattern intoexamples/ai-agentend-to-end.Motivation
Host apps that ship pre-canned "skills" to their agents have had to roll their own everything: storage layout, manifest parsing, an editor, and any kind of authoring UX. The pieces are generic enough to live in the library — only the actual storage backend (filesystem, HTTP, OPFS, IndexedDB…) is host-specific.
This PR introduces the generic primitives and a reference implementation so a consumer only has to swap in their storage implementation and a capability surface.
What's in
@sqlrooms/ai/skillsManifest + storage
skill.yaml(Zod-validated metadata:id,version,name,description, optionalauthor/icon) next to aSKILL.mdbody containing LLM instructions. No frontmatter inside the markdown.parseSkillManifest/serializeSkillManifest/loadSkillFromFiles— YAML + Zod parsing with structured, serializable errors (SkillManifestError,SkillNotFoundError,SkillRootReadOnlyError,SkillConflictError).SkillStorageinterface —listRoots,listSkills,readSkill,writeSkill,deleteSkill,resolveSkillId. Supports multiple prioritized roots (e.g.installed > user > built-in) for conflict resolution. No fs/Node imports; the interface is backend-agnostic.Authoring wizard
createSkillAuthoringAgent({model, context, draftStore, onSave})— aToolLoopAgentfactory with three tools:writeManifest({name, description, author?})writeInstructions({markdown})saveSkill({rootId?})createSkillDraftStore()— per-session Zustand store (Immer) tracking the live draft + status. Fresh instance per wizard opening; no global singleton.SkillAuthoringContext— the host-supplied capability surface the agent may reference:services[],tools[],permissions[], plusforbiddenIdentifiers[]for portability checks. The wizard stays domain-neutral; hosts fill in their own vocabulary.buildSkillAuthoringSystemPrompt(context)— deterministic system prompt covering role, tools, capability surface, a portability rule (no hardcoded table/column names), and compactskill.yaml/SKILL.mdexamples.SkillAuthoringPanel— split-view React component:AgentChaton the left, liveSkillDraftPreview(structured manifest card + rendered markdown) on the right. Knows nothing about filesystems, IPC, or any specific LLM provider — the caller owns the agent and the draft store.Tool validators reject inputs matching any
forbiddenIdentifierstoken via word-boundary, case-insensitive match. This nudges the agent toward portable templates that run against whatever data the user has.@sqlrooms/ai-core—AgentChatA new domain-neutral chat primitive driven by an externally constructed
ToolLoopAgent. Unlike the session-basedChatcomponent,AgentChatowns no session state, no AI slice, no model selection, no persistence — built for transient, single-purpose agent conversations (authoring wizards, inline approvals, one-shot queries).Reuses
FlatAgentRenderer+ToolRenderBehaviorso the tool-call rendering stays consistent with the main chat surface.Example app (
examples/ai-agent)Demonstrates the three extension points a consumer swaps:
InMemorySkillStorage— referenceSkillStorageimpl backed byMaps. Two roots (read-onlybuilt-in, writabledefault). Seeds round-trip throughparseSkillManifestso schema drift fails loudly on load.SkillsButton— header button that opens aDialogcontainingSkillAuthoringPanel. Fresh agent + draft store per open; save flushes intoInMemorySkillStorageand refreshes the listing.runSkillTool— minimal runtime: resolve{skillId}→ read record → spin up a freshToolLoopAgentwith the skill'sSKILL.mdas instructions andquerySQLas its only tool → stream viastreamSubAgentso tool calls surface in the parent's activity log.README has a
Skillssection walking through storage / authoring / runtime.Notes
yamladded as a dependency of@sqlrooms/ai.@sqlrooms/deckout of ambient@typesin its tsconfig (unblocks typecheck), and drop an unused@loaders.gl/coredep.Test plan
pnpm typecheckat repo root — no errorspnpm --filter @sqlrooms/ai test— manifest schema + authoring-tool unit tests passpnpm dev ai-agent-example— open New skill, describe a skill, watch the preview populate, save, and run it from chatFlatAgentRenderer/Chatusages render unchangedSummary by CodeRabbit
New Features
Documentation