AI Assistant skill menu: one mixed chooser for cards + skill files#5547
AI Assistant skill menu: one mixed chooser for cards + skill files#5547lukemelia wants to merge 6 commits into
Conversation
28eab3f to
cd62452
Compare
Collapse the two skill-attach buttons into a single action that opens the
mixed cards + files chooser, and retire the file-tree path for skills.
- One `Choose a Skill to add` button opens `chooseCard({ includeFiles: true })`
with a base filter that spans both kinds: `any` of the Skill card type and
MarkdownDef files whose `kind` is `skill`. Each pick is routed by its kind —
a card to `onChooseCard`, a file to `onChooseSkillMarkdown`.
- Already-attached skills are excluded by `not: { eq: { id } }` clauses built
client-side from the menu's own skill list: card skills are excluded
immediately, and file skills once their rows carry `id` in the search doc
(Phase 1 reindex), so this ships without waiting on a deployed reindex.
- The separate skill-file button and its `chooseFile`-based attach are removed.
`chooseFile` / `FileChooserModal` stay for their other consumers (message
file attach, Playground, markdown-embed).
Tests: the single button attaches a skill card and a skill markdown file;
already-attached card and markdown skills are excluded; the retired skill-file
button is gone; canceling the chooser re-enables the add button.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The skill picker opens the mixed chooser (`includeFiles`), which renders a tile per matching row. With no realm scope it fanned out across every server realm — including large shared realms — so the picker stalled on rendering and the intended skills may never appear. Pass `realms: userRealmIdentifiers` so the picker searches only the user's own workspaces, where the skills they attach live. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lies The skill menu scopes its mixed chooser to `realmServer.userRealmIdentifiers`. Under mock matrix that list stayed empty (only `activeRealms` was set), so the scope was a no-op and the chooser fanned out across every server realm — including the large shared skills realm — and the intended skill tiles never rendered in time. Set `activeRealmServers` in the mock-matrix setup (as the workspace-chooser integration test does) so the user realm list populates with the test realm, exercising the same scoped path the skill menu takes in production. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
d88beea to
e10622e
Compare
…lected
The skill menu's base filter is `any: [{ type: skillCardRef }, { on:
markdownDefRef, eq: { kind: 'skill' } }]`. When the type picker seeds a
selected type, `buildSearchQuery` stripped the base filter's type constraint —
but `stripTypeFromFilter` collapsed the `any` (dropping the `skillCardRef`
branch) down to the lone `{ on, eq }` markdown branch, which then AND-ed into
the top-level `every`. Every row was required to be a `kind: 'skill'` markdown
file, so skill *cards* were excluded and only markdown skills rendered.
Only strip a simple single-type base filter (which is all the optimization was
meant for); keep a compound base filter intact so its alternation survives.
Same guard applied to `buildRecentsQuery`.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| // waiting on that reindex. | ||
| let exclusions = | ||
| this.args.skills?.map((skill: RoomSkill) => ({ | ||
| not: { eq: { id: skill.cardId } }, |
There was a problem hiding this comment.
[Claude Code 🤖] Correctness — not:{eq:{id}} exclusions hide ALL skill files on un-reindexed realms.
These client-built exclusions are ANDed into the mixed every. A skill-markdown row on a realm not yet carrying the Phase-1 stamped id has search_doc->>'id' = NULL, so NOT (id = <cardId>) is NULL under three-valued logic and the row fails the WHERE clause — for every skill-file row, not just the attached one. Since the assistant typically boots with skills already attached, a user on an un-reindexed realm opens the chooser and sees only skill cards; no skill files appear.
CI is green because its index is fresh (all files carry id), so this only bites deployed realms pre-reindex. The header comment says un-stamped files "are excluded once their rows carry id" (i.e. would still show) — NULL-under-NOT does the opposite and hides them all. Consider making a missing id not drop the row (exclude the file kind by url, or apply id-exclusions to card rows only).
| // across every server realm — including large shared realms — and the | ||
| // intended skills may never finish rendering. Skills a user attaches | ||
| // come from their workspaces. | ||
| realms: this.realmServer.userRealmIdentifiers, |
There was a problem hiding this comment.
[Claude Code 🤖] Behavior change (intentional — flagging for the record) — scoping to userRealmIdentifiers drops shared/catalog skills.
userRealmIdentifiers is availableRealms filtered to type === 'user', so skills that live in a shared/catalog/base realm (e.g. a dedicated skills realm) fall out of scope and no longer appear — a capability the old card/file choosers had. This was a deliberate call to stop the mixed chooser fanning out across every realm, but it narrows discoverability. If shared-realm skills should stay attachable, broaden the scope (e.g. [...userRealmIdentifiers, ...catalogRealmIdentifiers]).
| // across every server realm — including large shared realms — and the | ||
| // intended skills may never finish rendering. Skills a user attaches | ||
| // come from their workspaces. | ||
| realms: this.realmServer.userRealmIdentifiers, |
There was a problem hiding this comment.
[Claude Code 🤖] Correctness (edge case) — an empty userRealmIdentifiers silently re-opens the fan-out.
realms is snapshotted here at click time. If no realm is typed 'user' (or the list is not yet populated), userRealmIdentifiers is [], and the modal guard if (this.state?.requestedRealms?.length) (card-chooser/modal.gts:266) is falsy for an empty array → it falls back to searching all availableRealmUrls. That is exactly the fan-out this scoping was meant to prevent. Worth handling [] deliberately rather than treating it as "unscoped".
| // clause — which would wrongly exclude the other branch's kind (skill cards | ||
| // vanish, only markdown skills survive). | ||
| const effectiveBaseFilter = | ||
| typeFilter && isCardTypeFilter(baseFilter) |
There was a problem hiding this comment.
[Claude Code 🤖] Cleanup — the any-collapse fix is band-aided at the call site; the defect stays latent in stripTypeFromFilter.
Both call sites now gate on isCardTypeFilter(baseFilter), so stripTypeFromFilter is only ever passed a bare {type} node — its every/any/not recursion is now unreachable from here. The underlying bug (recursing into an any and stripping a branch collapses the OR) still lives in the function, so a future caller passing a compound base filter without this guard re-hits the skill-card-vanishes bug. Consider fixing inside stripTypeFromFilter (don't strip within an any) so the invariant holds regardless of caller.
…helper
Two review fixes:
- The client-built already-attached exclusions were `not: { eq: { id } }`,
which drop any row whose `id` is absent (`NOT (NULL = X)` is NULL under
three-valued logic). On a realm not yet carrying the stamped file `id`, that
hid *every* skill-file row whenever any skill was attached. Wrap each in
`any: [{ eq: { id: null } }, { not: { eq: { id } } }]` so a null id keeps the
row while a present-and-matching id still excludes it.
- Move the `any`-collapse guard into `stripTypeFromFilter` itself: never strip
within an `any` (dropping/collapsing a branch changes which rows match), so
the invariant holds for any caller. Drops the band-aid `isCardTypeFilter`
guards at the two call sites.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Attachable skills live mostly in the shared Boxel Skills realm, which the user-realms-only scope excluded, so they were not selectable. Scope the mixed chooser to the user's own workspaces plus `skillsRealmURL`. Kept bounded (rather than searching every server realm) so the federated search does not stall on unrelated realms' contents. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Closes CS-12206. Phase 3 (the user-facing payoff) of the mixed cards+files chooser (parent CS-11809). Stacked on #5546 (CS-12205, mixed chooser) → #5531 (CS-12204, file
id). Review/merge those first.What
One button in the AI Assistant skill menu now attaches either a Skill card or a skill-bearing markdown file, from a single search chooser.
attachSkillCard+attachSkillMarkdowninto one action that opens the mixed chooser viachooseCard(query, { includeFiles: true }). The base filter spans both kinds —anyof{ type: skillCardRef }and{ on: markdownDefRef, eq: { kind: 'skill' } }.onChooseCard(id), a file pick →onChooseSkillMarkdown(id). Both already flow to the sameattachSkillTaskinmatrix/room.gts, so no consumer change.not: { eq: { id } }clauses built client-side from the menu's own skill list. Card skills are excluded immediately; file skills once their rows carryidin the search doc (Phase 1 reindex) — so the feature ships without waiting on a deployed reindex.chooseFile-based attach are removed.chooseFile/FileChooserModalstay for their other consumers (message file attach, Playground, markdown-embed) — untouched.Tests (
skills-test.gts)lint:types(host), eslint, and ember-template-lint are green.Note
Depends on #5546 and #5531. In CI the index is fresh (Phase 1
idstamping active), so the markdown-skill exclusion is exercised end-to-end. In a deployed environment, file-skill exclusion becomes effective after the affected realms are reindexed; card-skill exclusion needs no reindex.🤖 Generated with Claude Code