From 539c4c7e4d085c24052a718c0c867e335bc7c7b1 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 19 Apr 2026 08:56:23 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=8E=AC=20feat:=20Prime=20Manually-Invoked?= =?UTF-8?q?=20Skills=20via=20$=20Popover=20(#12709)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🎬 feat: Prime Manually-Invoked Skills via $ Popover Lands the backend for manual skill invocation, making the $ popover deterministically prime SKILL.md before the LLM turn instead of leaving the model to discover the skill via the catalog. Flow: popover drains pendingManualSkillsByConvoId on submit, attaches names to the ask payload, controllers forward to initializeAgent, and initialize resolves each name to its body (ACL + active-state filtered, reusing the same rules as catalog injection). AgentClient splices the primes as meta HumanMessages before the user's current message. - Extract primeManualSkill / resolveManualSkills in packages/api/src/agents/skills.ts and reuse primeManualSkill inside handleSkillToolCall for a single shape source. - Thread manualSkills + getSkillByName through InitializeAgentParams / DbMethods and all three initializeAgent call sites (initialize.js, responses.js, openai.js). - Splice HumanMessage primes in client.js chatCompletion after formatAgentMessages, shifting indexTokenCountMap so hydrate still fills fresh positions correctly. - Carry isMeta / source / skillName in additional_kwargs for downstream filtering. * πŸ›‘οΈ fix: Scope manual skill primes to single-agent + cap resolver input Two follow-ups to the Phase 3 priming path flagged in Codex review. Multi-agent runs: skipping the splice when agentConfigs is non-empty. `initialMessages` is shared across every agent in `createRun`, so splicing a skill body there would bypass Phase 1's per-agent `scopeSkillIds` contract β€” a handoff / added-convo agent with a different skill scope would see content its configuration excludes. Warn + skip is the minimal correct behavior; lifting this to per-agent initial state is a follow-up. Input bounding: `resolveManualSkills` now truncates to `MAX_MANUAL_SKILLS` (10) after dedup, with a warn listing the dropped tail. Controllers only validate `Array.isArray(req.body.manualSkills)`, so a crafted payload could otherwise fan out into an unbounded `Promise.all` of concurrent `getSkillByName` DB lookups. Cap lives in the resolver so every caller (including future `always-apply` in Phase 5) inherits it. * πŸ§ͺ refactor: Testable Helpers + Payload Validation for Manual Skill Primes Follow-ups from the comprehensive review. No behavior change for the happy path β€” these are architectural and defensive improvements that shrink the JS surface in /api, tighten the request-body contract, and cover the delicate splice logic with proper unit tests. - Extract `injectManualSkillPrimes` into packages/api/src/agents/skills.ts so the message-array splice and `indexTokenCountMap` shift are unit- testable in TS. client.js now calls the helper. Tests pin the `>=` vs `>` boundary condition β€” a regression here would silently corrupt token accounting for every message after the insertion point. - Extract `extractManualSkills(body)` and use in all three controllers (initialize.js, responses.js, openai.js). Replaces copy-pasted `Array.isArray(...) ? ... : undefined` with a helper that also filters non-string / empty elements β€” closes a type-safety gap where a crafted payload like `{"manualSkills": [123, {"$gt":""}]}` would otherwise reach `getSkillByName` and waste DB round-trips. - Rename `primeManualSkill` β†’ `buildSkillPrimeMessage`. The helper serves three invocation modes (`$` popover, `always-apply`, model-invoked); the old name misled readers coming from `handleSkillToolCall`. - Add `loadable.state === 'hasValue'` guard in `drainPendingManualSkills` β€” defensive, since the atom has a synchronous `[]` default, but the previous `.contents` cast would have been unsound under loading/error. - Document why `resolveManualSkills` honors the active-state filter even for explicit `$` selections (Phase 2 popover filter + API-direct hardening). - Remove stray `void Types;` in initialize.test.ts β€” `Types` is already consumed elsewhere in that test. * πŸ”– refactor: Single source for the skill-message source marker Export `SKILL_MESSAGE_SOURCE = 'skill'` and use it in both construction paths that stamp skill-primed messages β€” `buildSkillPrimeMessage` (for the model-invoked tool path) and `injectManualSkillPrimes` (for the user-invoked splice path). Downstream filtering and telemetry read this marker, so the two paths must agree; keeping the literal in one place removes the risk of them drifting when Phase 5's `always-apply` adds a third caller. * ♻️ refactor: Drop Multi-Agent Guard + Review Polish - Remove the multi-agent skip in `AgentClient.chatCompletion`. Leaking primes to handoff / added-convo agents via shared `initialMessages` is the agents SDK's concern to scope; this layer should just inject and let the graph handle agent-scoped state. The guard was well-intended but produced a silent-drop UX where `$skill` in a multi-agent run did nothing. - Bound the `[resolveManualSkills] Truncating ...` warn output to the first 5 dropped names plus a count suffix. A malicious payload of 1000 names was previously spilling all ~990 names into the log line. - Remove dead `?? []` from the `hasValue`-guarded loadable read in `drainPendingManualSkills` β€” the atom always yields a string[] when resolved, so the nullish fallback was unreachable. - Reorder skills.ts imports to follow the style guide: value imports shortest-to-longest (`data-schemas` β†’ `langchain/core/messages` β†’ multi-line `@librechat/agents`), type imports longest-to-shortest. * 🧠 fix: Strip Skill Primes from Memory Window + Unbreak CI Mocks Two fixes after the last push. CI unbreak: `responses.unit.spec.js` and `openai.spec.js` mock `@librechat/api` and the mock didn't expose the new `extractManualSkills` symbol, so every test in those files crashed before reaching the `recordCollectedUsage` assertion. Added `extractManualSkills: jest.fn()` returning `undefined` to both mocks; the controllers now no-op on manualSkills as the tests expect. Codex P2: `runMemory` passes `messages` straight through to the memory processor, so after the splice in `injectManualSkillPrimes`, SKILL.md bodies ride along as if they were real user chat. That pollutes memory extraction with synthetic instruction content and crowds out real turns from the window. - Export `isSkillPrimeMessage(msg)` from `packages/api/src/agents/skills.ts` β€” a predicate keyed on the shared `SKILL_MESSAGE_SOURCE` marker. - Filter `chatMessages = messages.filter(m => !isSkillPrimeMessage(m))` at the top of `runMemory` before the window-sizing logic. Keeps the primes visible to the LLM (they still ride in `initialMessages`) but invisible to the memory layer. - 5 new tests for the predicate covering marker-present, plain messages, different source, non-object inputs, and array filter integration. * πŸ“œ feat: Show Skill-Loaded Cards for Manually-Invoked Skills The $ popover was priming SKILL.md bodies into the turn but leaving no visible trace on the assistant response β€” from the user's view it looked like the `$name ` cosmetic text did nothing. Now each manually-invoked skill renders the same "Skill X loaded" tool-call card that model-invoked skills already produce via PR #12684's SkillCall renderer. Approach: post-run prepend to `this.contentParts`. The aggregator owns per-step indices during the run, so pre-seeding collides; waiting until `await runAgents(...)` returns lets the graph settle before synthetic parts slot in at the front. - Export `buildSkillPrimeContentParts(primes, { runId })` from `packages/api/src/agents/skills.ts`. Returns completed tool_call parts (`progress: 1`, args JSON-encoded with `{skillName}`, output matching the model-invoked path's wording) that the existing `SkillCall.tsx` renderer draws identically. - In `AgentClient.chatCompletion`, prepend the built parts to `this.contentParts` immediately after `await runAgents`. Persistence and the final-event reconcile come for free β€” `sendCompletion` already reads `this.contentParts` verbatim. - Card ordering: skills appear first in the assistant message, reflecting that priming ran before the LLM's turn. Live-during-streaming cards are a separate follow-up β€” the graph's index-based aggregator makes that a bigger lift and this change delivers the core UX win without fighting the stream ordering. 6 new unit tests covering part shape, args JSON contract, output text, unique IDs, empty input, and startOffset ID differentiation. * ⚑ feat: Emit Optimistic Skill Cards + Wire Primes in OpenAI/Responses Two follow-ups from testing. Optimistic card emit: the main chat path was only showing "Skill X loaded" cards at final-reconcile time, so the user saw nothing happen until the stream finished. Now emit synthetic ON_RUN_STEP + ON_RUN_STEP_COMPLETED events right before `runAgents` starts β€” same pattern the MCP OAuth flow uses in `ToolService` β€” so the cards appear immediately. The graph's content at index 0 may overwrite them during streaming, but the post-run `contentParts` prepend (unchanged) restores them on final reconcile. OpenAI + Responses parity: both controllers were resolving `manualSkillPrimes` via `initializeAgent` but never injecting them into `formattedMessages` before the run. Manual invocation silently did nothing on `/v1/chat/completions` and the Responses API path. Now both call `injectManualSkillPrimes` on the formatted messages so the model sees SKILL.md bodies on every path. LibreChat-style card SSE events don't apply to these OpenAI-shaped responses, so the live-emit is chat-path-only. - Export `buildSkillPrimeStepEvents(primes, { runId })` from `packages/api/src/agents/skills.ts`. Uses `Constants.USE_PRELIM_RESPONSE_MESSAGE_ID` by default so the frontend maps events to the in-flight preliminary response message, matching the OAuth emitter. - In `AgentClient.chatCompletion`, emit via `sendEvent` (or `GenerationJobManager.emitChunk` in resumable mode) after `injectManualSkillPrimes` runs, before the LLM turn begins. - Wire `injectManualSkillPrimes` into `openai.js` + `responses.js` after `formatAgentMessages`. Refactored the destructure to `let` on `indexTokenCountMap` so the injector's returned map is usable. - 8 new unit tests covering the step-event builder: pair cardinality, default/custom runId, TOOL_CALLS shape + JSON args, progress:1 on completion, index ordering, stepId/toolCallId pairing, empty input. * 🎯 fix: Route Skill Prime Events to the Real Response + Sparse-Array Offset Two bugs in the optimistic-card emit from the last pass. 1. Wrong runId. The events used `USE_PRELIM_RESPONSE_MESSAGE_ID` (the MCP OAuth pattern), but OAuth emits DURING tool loading β€” before the real response messageId exists. By the time skill priming fires, the graph is about to emit with `this.responseMessageId`, so the PRELIM runId orphaned every card onto the client's placeholder response entry in `messageMap`, separate from the one the LLM's events were building. Net effect: cards never rendered mid-stream. Now passing `this.responseMessageId` β€” the same ID `createRun` receives β€” so synthetic and real steps land on the same `messageMap` entry. 2. Index 0 collision. With the runId fixed, card-at-0 would have hit `updateContent`'s type-mismatch guard when the LLM's text delta arrived at the same index, suppressing the whole text stream. New `SKILL_PRIME_INDEX_OFFSET` = 100 placed on both the live SSE emit and the server-side `contentParts` assignment. Sparse array during streaming renders as `[llm_text, ..., card]` (skip-holes via `Array#filter` / `Array#map`). `filterMalformedContentParts` from `sendCompletion` compacts to dense `[text, card]` before persistence, so streaming UI and saved message agree on order β€” no finalize reorder jank. Post-run switches from `contentParts.unshift` to `contentParts[OFFSET + i] = part` to mirror the live placement. - Add `startIndex` option to `buildSkillPrimeStepEvents` with `SKILL_PRIME_INDEX_OFFSET` default. Export the constant from `@librechat/api` so `client.js` can reuse it for the post-run splice. - Update the existing index-ordering test to the new default and add a new test for the explicit `startIndex` override. * πŸŽ—οΈ feat: Replace \$skill-name Text with Pills on the User Message The `$skill-name ` cosmetic text the popover was inserting into the textarea had two problems: it lingered in the user message forever (the card is a more meaningful marker), and it implied that free-form text invocation like \"\$foo help me\" should work β€” which it doesn't, and supporting it would mean another parsing layer nobody asked for. Dropped the textarea insertion. Visual confirmation after submit now comes from a compact `ManualSkillPills` row on the user bubble that self-extinguishes once the backend's live skill-card stream (`buildSkillPrimeStepEvents` from the last commit) populates the sibling assistant response. Multiple skills render as multiple pills β€” the atom was already a string array, so multi-select works for free. - `SkillsCommand.tsx`: select handler no longer writes to the textarea. Still drops the trigger `$` via `removeCharIfLast`, still pushes to `pendingManualSkillsByConvoId`, still flips `ephemeralAgent.skills`. - `families.ts`: new `attachedSkillsByMessageId` atomFamily keyed by user messageId. `useChatFunctions.ask` writes the drained skill list here on every fresh submit (regenerate/continue/edit still skip). - `ManualSkillPills.tsx` renders pills conditionally: hidden when the message isn't a user message, when no skills are attached, or when the sibling assistant response already carries a `skill` tool_call content part (the live card took over). Reads messages via React Query so we don't re-render on every message-state keystroke. - `Container.tsx` mounts the pills above the user message text, parallel to the existing `Files` slot. - Updated the SkillsCommand select-flow spec to assert the textarea is cleared of `$` instead of populated with `\$name `. 5 new tests for `ManualSkillPills` covering empty state, non-user message guard, multi-skill rendering, the skill-card hide condition, and the text-only-content-doesn't-hide case. * πŸŽ›οΈ feat: Manual Skills as Persisted Message Field + Compose-Time Chips Three problems with the previous pass: 1. Cards rendered BELOW the LLM text on the assistant message (and stayed there on reload) because the sparse index-100 offset put them after the model's content. Now back to `unshift` β€” cards at the top, same as before the live-emit detour. 2. Pills on the user message disappeared the moment the live card arrived, so users barely saw them. The live-emit channel also added meaningful complexity and relied on a per-message Recoil atom that had no clean cleanup story. 3. No visual cue at all during new-chat compose β€” the `$name ` text was removed, the submitted-message pills weren't there yet, and the popover closes after selection. User had no way to see what they'd queued up before sending. New architecture: `manualSkills` is a first-class field on `TMessage`, persisted by the backend on the user message. `ManualSkillPills` reads straight from `message.manualSkills` β€” no atom, no sibling-lookup β€” so pills survive reload, show in history, and stay for the lifetime of the message. Compose-time chips above the textarea read the existing `pendingManualSkillsByConvoId` atom and let users Γ— skills out before submitting. Backend reverts: - `client.js`: dropped the `ON_RUN_STEP` live-emit loop, restored `this.contentParts.unshift(...primeParts)` so cards sit at the top of the persisted assistant response. - `skills.ts`: removed `buildSkillPrimeStepEvents` and `SKILL_PRIME_INDEX_OFFSET` (both unused now). `GraphEvents`, `StepTypes`, and `Constants` imports went with them. Removed 8 tests. Field persistence: - `tMessageSchema` gains `manualSkills: z.array(z.string()).optional()`. - Mongoose message schema gains `manualSkills: { type: [String] }` with matching `IMessage` TS field. - `BaseClient.js` reads `req.body.manualSkills` on user-message save, filters to non-empty strings, pins onto `userMessage` before `saveMessageToDatabase`. Mirrors the existing `files` pattern right above it. Runtime resolution still reads top-level `req.body.manualSkills` β€” persistence and resolution are separate concerns. Frontend: - `useChatFunctions.ask` sets `currentMsg.manualSkills` directly; the drained atom value goes onto the message, not a separate atom. Removed the `attachSkillsToMessage` Recoil callback. - `ManualSkillPills`: pure render of `message.manualSkills`. No more `useQueryClient`, no sibling scan, no atom read. Loses the auto-hide-when-card-arrives behavior β€” pills stay on the user bubble, cards live on the assistant bubble, both are informative. - Dropped the `attachedSkillsByMessageId` atomFamily and its export. - New `PendingManualSkillsChips` above the textarea reads the compose-time atom and renders chips with Γ— to remove. Mounted in `ChatForm` right after `TextareaHeader`. Naturally hides on submit when the atom drains. Tests: updated `ManualSkillPills` suite to the new field-based reads (5 passing). New `PendingManualSkillsChips` suite covering empty state, multi-chip render, single Γ— removal, and full-clear (4 passing). Backend suite trimmed to 89 (was 97) from the step-events test removal β€” no regressions on the remaining helpers. * πŸ§ͺ feat: Assistant-Side Skill-Loading Chips + Pill Padding Two small UX fixes on top of the field-on-message architecture. Pill padding: bumped the user-side `ManualSkillPills` from `py-0.5` to `py-1` on each chip and added `py-0.5` to the wrapper so the row breathes a little without feeling tall. Mid-stream indicator: new `InvokingSkillsIndicator` mirrors the parent user message's `manualSkills` onto the assistant bubble as transient "Running X" chips while the real card is in flight. Renders above `ContentParts` in `MessageParts`. Hides itself when the assistant's own `content` grows a `skill` tool_call β€” the authoritative card from `buildSkillPrimeContentParts.unshift` is showing, so the placeholder steps aside. No SSE emit, no aggregator injection, no index collision with the LLM's streaming content: just a render slot keyed off the parent's field. Why not stream the cards live: whichever content index we'd choose either blocks the LLM's text stream (`updateContent` type-mismatch at index 0) or lands below the response after sparse compaction (index 100+). Mirroring the parent field sidesteps the aggregator entirely and gives the user an immediate "skill is loading" signal that naturally gives way to the real card at finalize. Covers the gap the user flagged: pills on the user message said "I asked for these" but nothing on the assistant side said "we're working on it" until the stream finished. 5 new tests for the indicator: user-msg guard, missing parent-field guard, multi-chip render, hides-on-card-landing, orphan-parent guard. * πŸ” fix: Indicator Visibility + Carry Manual Skills Through Regenerate/Edit Two bugs. Indicator never rendered: `InvokingSkillsIndicator` looked up the parent user message via `queryClient.getQueryData([QueryKeys.messages, convoId])`, but on a new chat the React Query cache is keyed by `"new"` (the URL `paramId`) until the server assigns a real conversation ID β€” while `message.conversationId` on the assistant message is already the server ID. Lookup missed, `skills.length === 0`, nothing rendered. Switched to `useChatContext().getMessages()`, which reads from the same `paramId` the rest of the UI uses, so new-chat and existing-chat cases both resolve to the correct message list. Regenerate / save-and-submit dropped manual skills: the compose-time `pendingManualSkillsByConvoId` atom is drained on the first submit, so replaying that turn later found an empty atom and sent `manualSkills: []`. The pills were still on the user bubble, so from the user's point of view the model was running primed β€” but the backend saw nothing and produced an unprimed response. - Added `overrideManualSkills?: string[]` to `TOptions`. Callers with a reference message pass its persisted `manualSkills`; `useChatFunctions.ask` uses the override verbatim when present, otherwise falls back to the existing drain-or-empty logic. - `regenerate` in `useChatFunctions` passes `parentMessage.manualSkills` β€” the user message being regenerated has the field persisted by the backend, so the second turn primes the same skills as the first. - `EditMessage.resubmitMessage` covers both edit branches: - User-message save-and-submit: forwards the edited message's own `manualSkills` so the new sibling turn primes identically. - Assistant-response edit: forwards the parent user message's `manualSkills` for the same reason. Indicator test suite converted from `@tanstack/react-query` harness to a jest-mocked `useChatContext().getMessages()`. 6 tests (was 5), added a cache-miss case. * 🧭 fix: Drive Mid-Stream Skill Chips from Submission Atom, Not Message Lookup Message-ID-keyed lookups kept racing the stream: the user message flips from its client-side intermediate UUID to the server-assigned ID mid-run, conversation IDs flip from the URL `paramId="new"` to the real convo ID on brand-new chats, and the React Query cache splits briefly between the two. Previous attempts β€” direct `queryClient.getQueryData` and then `useChatContext().getMessages()` β€” each missed a different window. `TSubmission.manualSkills` is already populated at `ask()` time and the submission atom (`store.submissionByIndex(index)`) is the single stable anchor across the whole lifecycle: set once at submit, lives through every SSE event, cleared when the stream ends. No ID lookups, no cache timing. - `InvokingSkillsIndicator` now reads `submissionByIndex(index)` via Recoil. Shows chips when: β€’ the message is assistant-side, β€’ a submission is in flight with non-empty `manualSkills`, β€’ the assistant's `parentMessageId` matches `submission.userMessage.messageId` (so chips appear only on the bubble for the current turn, never on siblings), β€’ the assistant's own content doesn't yet carry a `skill` tool_call (real card takes over from the server's post-run `contentParts.unshift`). - Drops the `useChatContext().getMessages()` dependency and the `useQueryClient` dependency before that. No more lookups by conversationId or messageId. Test suite now mocks `useChatContext` to supply `index: 0` and seeds the `submissionByIndex(0)` atom via Recoil initializer. 6 cases cover user-side, no-submission history, empty `manualSkills`, multi-chip render, hides-on-card-landing, and wrong-turn guard. * 🌱 fix: Seed Response manualSkills in createdHandler, Indicator Becomes Pure The mid-stream indicator kept getting wired off state I don't own: first `queryClient.getQueryData` (raced the new-chat paramId flip), then `useChatContext().getMessages()` (same cache, same race), then `useRecoilValue(submissionByIndex)` (pulled every message into the submission subscription β€” re-renders all indicators on any submission change, exactly the "limit hooks in rendering" concern). Cleanest path is the one the user pointed at: the submission owns the data, `useSSE` / `useEventHandlers` owns the save points, so seed the field ONTO the response message at the save site and let the indicator be a pure prop-read. - `createdHandler` now writes `manualSkills` onto the initial response from `submission.manualSkills` at the moment the placeholder enters the messages array. The field rides through the normal mutation pipeline via spreads (`useStepHandler` response creation, `updateContent` result returns) β€” no special handling needed. - `InvokingSkillsIndicator` drops the Recoil / context / queryClient reads. Pure function of `message`: if assistant, has `manualSkills`, and `content` hasn't grown a `skill` tool_call yet, render chips. Only `useLocalize` left, which was already unavoidable for the i18n string. - Renders decouple: no single state change (`submissionByIndex` flip, React Query cache update) forces every indicator in the message list to re-render anymore. Only the message whose prop changed re-runs. Finalize story unchanged: server's `responseMessage` doesn't carry the frontend-only `manualSkills` field, so `finalHandler`'s replacement drops it β€” but by then the real `skill` tool_call is in `content` and the indicator's content-scan hides itself anyway. Test suite back to pure prop mocks: 7 cases covering user-guard, no-seed, multi-chip render, skill-card-hide, non-skill-tool-call-keeps, text-only-keeps, and missing message. * πŸͺž fix: Render Skill Indicator Inside ContentParts, Adjacent to Parts The indicator still wasn't showing because even though MessageParts mounted it as a sibling of ContentParts, ContentParts is a `memo`'d component that owns the only rendering path that refreshes in lockstep with content deltas. Mounting above it put the indicator one layer further out β€” reachable, but not exercised on the same render cycle that processes the streaming `message` prop. Moved the indicator into ContentParts itself, rendered at the top of both the sequential and parallel branches. Reads the `message` prop (newly threaded through as an optional prop alongside `content`), so: - Same render cycle as Parts β€” updates from the SSE pipeline flow through the same pathway. - Lives outside the `content.map`, so delta-driven content reshuffles never wipe it. - Still a pure prop-read inside the indicator itself (no Recoil, queryClient, context hooks). The only dep is `useLocalize`. Thread: - `ContentPartsProps` gains `message?: TMessage`. - `MessageParts` passes `message={message}` through, drops its own indicator mount + import. - `ContentParts` renders `` in both the parallel-content and sequential-content branches, right under `MemoryArtifacts` and before the empty-cursor / parts map. Companion data flow (unchanged): `createdHandler` seeds `initialResponse.manualSkills` from `submission.manualSkills`; the field rides through `useStepHandler` via spreads; indicator hides on `skill` tool_call landing in `content`. * πŸ”Ž refactor: Narrow Skill Components to Scalar skills Prop, Kill Memo Churn Passing the full `message` object into presentational components busts `React.memo` shallow comparisons every time the message reference changes for unrelated reasons. Swap to scalar `skills?: string[]` throughout: - `InvokingSkillsIndicator`: props-only (`skills?: string[]`); visibility logic (user-vs-assistant, skill tool_call arrival) now lives in the caller so this stays pure presentational. - `ManualSkillPills`: props-only (`skills?: string[]`). - `ContentParts`: takes `manualSkills?: string[]` scalar, computes `showInvokingSkills` once per render from `manualSkills` + content scan for the `skill` tool_call, then mounts the indicator with `skills=` prop in both parallel and sequential branches. - `MessageParts`: passes `manualSkills={message.manualSkills}` through to `ContentParts`. - `Container`: passes `skills={message.manualSkills}` to `ManualSkillPills`. - Tests updated to exercise the narrowed prop surface. * πŸ“œ feat: Mid-Stream Skill Cards via SkillCall, Drop Custom Indicator Instead of a separate `InvokingSkillsIndicator` chip component, render pending skill placeholders through the existing `SkillCall` renderer β€” same component the backend's finalized prime part uses. The loading visual (`progress < 1` + empty output β†’ pulsing "Running X") and the completed visual ("Ran X") now come from one source of truth. `ContentParts` computes `pendingSkillNames` from `manualSkills` minus any `skill` tool_call already in `content` (dedupe by `args.skillName` since the synthetic's id differs from the real one). Those names render through a separate slot ABOVE the Parts iteration β€” not prepended to the content array, which would shift React keys on every downstream streaming text / tool part and force unmount/remount mid-stream. When the real prime `tool_call` lands at finalize (backend unshifts to content[0..]), `collectExistingSkillNames` picks it up, the pending set empties, and the real part takes over rendering in the Parts iteration. Layout is identical either way because primes are always at the top of content. - `InvokingSkillsIndicator.tsx` + test deleted (no longer referenced) - `ContentParts.tsx` renders `` directly for pending names, mirrors `Part.tsx`'s usage of the same component - `createdHandler` doc comment updated to reflect the new flow * βœ‚οΈ fix: Render Interim Skill Cards From manualSkills Only, Leave Content Untouched Previous revision read `content` to de-dupe pending cards against real `skill` tool_calls, so any optimistic skill part streamed from the backend would race our placeholder off the screen mid-turn β€” exactly the "getting overridden" symptom. Now: interim `SkillCall` cards are driven purely by the response message's `manualSkills` field. `content` is never inspected here, so no backend delta can pull the cards down. The field is now seeded directly onto the assistant placeholder in `useChatFunctions` (not only in `createdHandler`) so the cards appear from the first render, before the `created` SSE event round-trip. Lifecycle: - `useChatFunctions` puts `manualSkills` on the freshly-minted `initialResponse` β€” cards render the instant the placeholder lands. - `createdHandler` keeps its own re-seed (idempotent; safe) so a regenerate / save-and-submit flow that hits that path still works. - `useStepHandler` spread operations preserve the field through every content update. - `finalHandler` replaces the message with the server-backed `responseMessage` (no `manualSkills`) β€” cards disappear, and the real `skill` tool_call part in `content` takes over. ContentParts changes: - Drop `collectExistingSkillNames` / `parseJsonField` dedupe path. - `renderPendingSkills` reads only `manualSkills` + `isCreatedByUser`. - Simpler control flow β€” one boolean (`hasPendingSkills`) gates the early return, one function renders. * 🩹 fix: Codex Review Resolutions β€” Localization, Guards, Tests, Docs Addresses seven findings from comprehensive code review: Finding 1 (MAJOR) β€” Document sticky re-priming as intentional - `buildSkillPrimeContentParts`: expanded doc comment explaining synthetic `skill` tool_calls persist and get re-primed on every subsequent turn via `extractInvokedSkillsFromPayload` (shape parity with model-invoked skills). This matches the UX: the assistant skill card is a visible, persistent signal that the skill is active for the conversation. Not a bug β€” called out explicitly so future maintainers don't mistake it for one. Finding 2 (MAJOR) β€” Add ContentParts render tests - New `ContentParts.test.tsx` with 7 cases covering the interim skill card logic: assistant-only rendering, user-message suppression, undefined-content safety, parallel+sequential branch integration, progress<1 (pending) state. Child components mocked so the test exercises only the branching and prop wiring ContentParts owns. Finding 3 (MINOR) β€” Localize hardcoded aria-labels - Added `com_ui_skills_manual_invoked` + `com_ui_skills_queued` keys. - Reused existing `com_ui_remove_skill_var` for the remove-button aria-label. - `PendingManualSkillsChips` and `ManualSkillPills` now call `useLocalize()`. Test mocks updated to the label-echo pattern. Finding 4 (MINOR) β€” Max-length guard in `extractManualSkills` - New `MAX_SKILL_NAME_LENGTH = 200` constant and filter. Blocks a crafted payload like `{ manualSkills: ['a'.repeat(100000)] }` from reaching `getSkillByName` / Mongo's query planner. Finding 5 (NIT) β€” `BaseClient.js` comment contradicted itself - Rewrote to call the filter what it is: defense-in-depth on top of Mongoose schema validation, not a redundant second layer. Finding 6 (NIT) β€” `ManualSkillPills` now wrapped in `React.memo` - Consistent with peer components (`PendingManualSkillsChips`, `ContentParts`). Rendered inside `Container`, which re-renders on every content update, so the memo is a real cycle savings. Finding 7 (NIT) β€” Redundant guard in `ContentParts.renderPendingSkills` - Collapsed the duplicate null-check by computing `pendingSkills` as a `useMemo`'d array (`[]` when not applicable), and mapping directly. `hasPendingSkills` now derives from the array length β€” one source of truth, no redundant gate inside the render function. * πŸ”§ fix: Update ParallelContent to Handle Optional Content Prop Modified the `ParallelContentRendererProps` to make the `content` prop optional, ensuring safer access within the component. Adjusted the calculation of `lastContentIdx` to handle cases where `content` may be undefined, preventing potential runtime errors. This change enhances the robustness of the component when dealing with varying message structures. * 🎯 fix: Thread manualSkills Through ContentRender β€” The Real Renderer This is why the interim skill cards never appeared across many rounds of iteration: `ContentRender.tsx` (the memo'd renderer used by most paths, including the agents endpoint) was calling `ContentParts` without the `manualSkills` prop. Only `MessageParts.tsx` had it wired up β€” and that's not the component that actually renders the assistant response in production. Two fixes: 1. Pass `manualSkills={msg.manualSkills}` to the `ContentParts` call. 2. Extend the `areContentRenderPropsEqual` memo comparator to include `manualSkills.length`, otherwise a message update that adds the field (seeded by `useChatFunctions` on the initialResponse) would be bailed out by the memo and never re-render. Verified the two ContentParts call sites are now consistent; Container usages for `ManualSkillPills` on the user side were already correct. * 🧹 polish: Address Audit Follow-Up (F1/F3/F6) F1 β€” Clarify sticky re-priming opt-out path. The previous comment said "regenerate without the pick" as one opt-out, but `useChatFunctions.regenerate` forwards the original picks via `overrideManualSkills`, so regeneration alone keeps the skill sticky. Updated to: edit the originating message to remove the pills and resubmit, or start a new conversation. F3 β€” Add DOM-order assertions to the parallel + sequential tests. The two "alongside" tests verified both elements existed but didn't pin the ordering contract. Both now use `compareDocumentPosition` to assert the pending SkillCall precedes the real content, matching the backend semantic (`contentParts.unshift(...primeParts)` puts primes at the top). F6 β€” Fix package import order in PendingManualSkillsChips. `recoil` (58 chars) was listed before `lucide-react` (45 chars) which violates the "shortest to longest after react" rule in AGENTS.md. Swapped order; no behavior change. F2 / F4 / F5 from the audit were confirmed as non-issues (React-safe empty map, cosmetic test-mock artifact, accepted memo tradeoff) and require no change. * ✨ feat: Dedicated PendingSkillCall + Runningβ†’Ran Transition on Real Content UX polish on the interim skill card now that it's actually rendering: 1. New `PendingSkillCall` component (mirrors `SkillCall` visually but drops the expand affordance). `SkillCall`'s underlying `ProgressText` always renders a chevron + clickable button when any input is present, which on a card with empty output points at nothing β€” misleading cursor:pointer and a no-op toggle. The pending variant has only the icon + label, no button wrapper, no chevron. 2. "Running X" β†’ "Ran X" transition when real content lands. `ContentParts` computes `hasRealContent` (any non-text part, or a text part with non-empty content β€” placeholder empty-text parts don't count) and passes `loaded={hasRealContent}` to `PendingSkillCall`. Matches what users see for model-invoked skills as they finish priming: pulsing shimmer β†’ static icon. 3. Cleanup: - Dropped direct `SkillCall` import from `ContentParts` (replaced by `PendingSkillCall`). `SkillCall` is still used by `Part` for real `skill` tool_call content parts β€” no behavior change there. - Removed the now-redundant explicit `manualSkills` assignment in `createdHandler`. `useChatFunctions` seeds the field on `initialResponse` at construction, so the `...submission.initialResponse` spread already carries it through β€” the re-assignment was defensive belt-and-suspenders doing the same work twice. Comment rewritten to describe the actual lifecycle. Tests updated to the new component (12/12 pass): two new cases pin the loaded-state transition (unloaded when content has no real parts, flips to loaded once a non-empty text part lands). --- api/app/clients/BaseClient.js | 16 + .../agents/__tests__/openai.spec.js | 1 + .../agents/__tests__/responses.unit.spec.js | 1 + api/server/controllers/agents/client.js | 75 ++- api/server/controllers/agents/openai.js | 31 +- api/server/controllers/agents/responses.js | 31 +- .../services/Endpoints/agents/initialize.js | 13 + client/src/common/types.ts | 8 + client/src/components/Chat/Input/ChatForm.tsx | 2 + .../Chat/Input/PendingManualSkillsChips.tsx | 61 +++ .../components/Chat/Input/SkillsCommand.tsx | 28 +- .../PendingManualSkillsChips.spec.tsx | 59 +++ .../Input/__tests__/SkillsCommand.spec.tsx | 13 +- .../Chat/Messages/Content/Container.tsx | 2 + .../Chat/Messages/Content/ContentParts.tsx | 108 +++- .../Chat/Messages/Content/EditMessage.tsx | 8 + .../Messages/Content/ManualSkillPills.tsx | 42 ++ .../Chat/Messages/Content/ParallelContent.tsx | 4 +- .../Content/Parts/PendingSkillCall.tsx | 53 ++ .../Content/__tests__/ContentParts.test.tsx | 140 +++++ .../__tests__/ManualSkillPills.test.tsx | 34 ++ .../components/Chat/Messages/MessageParts.tsx | 1 + .../src/components/Messages/ContentRender.tsx | 4 +- client/src/hooks/Chat/useChatFunctions.ts | 73 ++- client/src/hooks/SSE/useEventHandlers.ts | 10 + client/src/locales/en/translation.json | 2 + client/src/store/families.ts | 13 +- .../src/agents/__tests__/initialize.test.ts | 155 ++++++ .../api/src/agents/__tests__/skills.test.ts | 486 +++++++++++++++++- packages/api/src/agents/handlers.ts | 5 +- packages/api/src/agents/initialize.ts | 52 +- packages/api/src/agents/skills.ts | 366 ++++++++++++- packages/data-provider/src/createPayload.ts | 2 + packages/data-provider/src/schemas.ts | 9 + packages/data-provider/src/types.ts | 9 + packages/data-schemas/src/schema/message.ts | 8 + packages/data-schemas/src/types/message.ts | 2 + 37 files changed, 1849 insertions(+), 78 deletions(-) create mode 100644 client/src/components/Chat/Input/PendingManualSkillsChips.tsx create mode 100644 client/src/components/Chat/Input/__tests__/PendingManualSkillsChips.spec.tsx create mode 100644 client/src/components/Chat/Messages/Content/ManualSkillPills.tsx create mode 100644 client/src/components/Chat/Messages/Content/Parts/PendingSkillCall.tsx create mode 100644 client/src/components/Chat/Messages/Content/__tests__/ContentParts.test.tsx create mode 100644 client/src/components/Chat/Messages/Content/__tests__/ManualSkillPills.test.tsx diff --git a/api/app/clients/BaseClient.js b/api/app/clients/BaseClient.js index 905cadfd23..ab79b3bb53 100644 --- a/api/app/clients/BaseClient.js +++ b/api/app/clients/BaseClient.js @@ -492,6 +492,22 @@ class BaseClient { } delete userMessage.image_urls; } + /** + * Persist the user's manual skill picks onto the user message so the + * frontend `ManualSkillPills` component can render them in history + * after reload. UI-only metadata β€” the runtime skill resolution + * pipeline reads the top-level `req.body.manualSkills` separately. + * Filter is defense-in-depth on top of Mongoose schema validation: + * keeps the DB row free of empty/non-string entries even if a + * crafted payload slips past schema checks upstream. + */ + const rawManualSkills = this.options.req?.body?.manualSkills; + if (Array.isArray(rawManualSkills) && rawManualSkills.length > 0) { + const skills = rawManualSkills.filter((s) => typeof s === 'string' && s.length > 0); + if (skills.length > 0) { + userMessage.manualSkills = skills; + } + } userMessagePromise = this.saveMessageToDatabase(userMessage, saveOptions, user).catch( (err) => { logger.error('[BaseClient] Failed to save user message:', err); diff --git a/api/server/controllers/agents/__tests__/openai.spec.js b/api/server/controllers/agents/__tests__/openai.spec.js index fbabe1bda3..d0ccb7045c 100644 --- a/api/server/controllers/agents/__tests__/openai.spec.js +++ b/api/server/controllers/agents/__tests__/openai.spec.js @@ -58,6 +58,7 @@ jest.mock('@librechat/api', () => ({ createErrorResponse: jest.fn(), getTransactionsConfig: mockGetTransactionsConfig, recordCollectedUsage: mockRecordCollectedUsage, + extractManualSkills: jest.fn().mockReturnValue(undefined), buildNonStreamingResponse: jest.fn().mockReturnValue({ id: 'resp-123' }), createOpenAIStreamTracker: jest.fn().mockReturnValue({ addText: jest.fn(), diff --git a/api/server/controllers/agents/__tests__/responses.unit.spec.js b/api/server/controllers/agents/__tests__/responses.unit.spec.js index 0824d38d1a..809790b43c 100644 --- a/api/server/controllers/agents/__tests__/responses.unit.spec.js +++ b/api/server/controllers/agents/__tests__/responses.unit.spec.js @@ -60,6 +60,7 @@ jest.mock('@librechat/api', () => ({ getBalanceConfig: mockGetBalanceConfig, getTransactionsConfig: mockGetTransactionsConfig, recordCollectedUsage: mockRecordCollectedUsage, + extractManualSkills: jest.fn().mockReturnValue(undefined), createToolExecuteHandler: jest.fn().mockReturnValue({ handle: jest.fn() }), // Responses API writeDone: jest.fn(), diff --git a/api/server/controllers/agents/client.js b/api/server/controllers/agents/client.js index 7828048991..2df4c12a9b 100644 --- a/api/server/controllers/agents/client.js +++ b/api/server/controllers/agents/client.js @@ -28,6 +28,9 @@ const { filterMalformedContentParts, countFormattedMessageTokens, hydrateMissingIndexTokenCounts, + injectManualSkillPrimes, + isSkillPrimeMessage, + buildSkillPrimeContentParts, } = require('@librechat/api'); const { Callback, @@ -603,18 +606,27 @@ class AgentClient extends BaseClient { const memoryConfig = appConfig.memory; const messageWindowSize = memoryConfig?.messageWindowSize ?? 5; - let messagesToProcess = [...messages]; - if (messages.length > messageWindowSize) { - for (let i = messages.length - messageWindowSize; i >= 0; i--) { - const potentialWindow = messages.slice(i, i + messageWindowSize); + /** + * Strip skill-primed meta messages before memory extraction. The primes + * sit next to the latest user message and carry large SKILL.md bodies, + * so letting them into the window would crowd out real chat turns and + * pollute extracted memories with synthetic instruction content the + * user never typed. + */ + const chatMessages = messages.filter((m) => !isSkillPrimeMessage(m)); + + let messagesToProcess = [...chatMessages]; + if (chatMessages.length > messageWindowSize) { + for (let i = chatMessages.length - messageWindowSize; i >= 0; i--) { + const potentialWindow = chatMessages.slice(i, i + messageWindowSize); if (potentialWindow[0]?.role === 'user') { messagesToProcess = [...potentialWindow]; break; } } - if (messagesToProcess.length === messages.length) { - messagesToProcess = [...messages.slice(-messageWindowSize)]; + if (messagesToProcess.length === chatMessages.length) { + messagesToProcess = [...chatMessages.slice(-messageWindowSize)]; } } @@ -759,6 +771,32 @@ class AgentClient extends BaseClient { `[AgentClient] Boundary token adjustment: ${boundaryTokenAdjustment.original} β†’ ${boundaryTokenAdjustment.adjusted} (${boundaryTokenAdjustment.remainingChars}/${boundaryTokenAdjustment.totalChars} chars)`, ); } + + /** + * Phase 3 manual skill priming β€” injected by user via `$` popover. + * + * Splice + index-shift logic lives in `injectManualSkillPrimes` + * (packages/api/src/agents/skills.ts) so the delicate position math + * can be unit-tested in TS without standing up AgentClient. Runs for + * both single-agent and multi-agent runs; how primes interact with + * handoff / added-convo agents' per-agent state is an agents-SDK + * concern, not this layer's to gate. + */ + const manualSkillPrimes = this.options.agent?.manualSkillPrimes; + if (manualSkillPrimes && manualSkillPrimes.length > 0) { + const primeResult = injectManualSkillPrimes({ + initialMessages, + indexTokenCountMap, + manualSkillPrimes, + }); + indexTokenCountMap = primeResult.indexTokenCountMap; + if (primeResult.inserted > 0) { + logger.debug( + `[AgentClient] Primed ${primeResult.inserted} manual skill(s) at message index ${primeResult.insertIdx}: ${manualSkillPrimes.map((p) => p.name).join(', ')}`, + ); + } + } + if (indexTokenCountMap && isEnabled(process.env.AGENT_DEBUG_LOGGING)) { const entries = Object.entries(indexTokenCountMap); const perMsg = entries.map(([idx, count]) => { @@ -876,6 +914,31 @@ class AgentClient extends BaseClient { const hideSequentialOutputs = config.configurable.hide_sequential_outputs; await runAgents(initialMessages); + /** + * Surface a completed `skill` tool_call content part per manually- + * invoked skill so the existing `SkillCall` frontend renderer shows + * a "Skill X loaded" card on the assistant response. Applied after + * the graph finishes to avoid clashing with the aggregator's own + * per-step content indexing. Prepended (not appended) so cards sit + * above the model's output β€” priming ran before the turn, the + * reply follows. + * + * Live streaming display of cards is handled on the user side via + * `ManualSkillPills` reading the message's `manualSkills` field; + * no separate SSE emit is needed here, and trying to stream a + * mid-run tool_call at index 0 collided with the LLM's first text + * content, while emitting at a sparse offset pushed the card below + * the reply on finalize. Post-run unshift keeps the final + * responseMessage.content in the right order. + */ + const primedSkills = this.options.agent?.manualSkillPrimes; + if (primedSkills && primedSkills.length > 0) { + const primeParts = buildSkillPrimeContentParts(primedSkills, { + runId: this.responseMessageId ?? 'manual-skill', + }); + this.contentParts.unshift(...primeParts); + } + /** @deprecated Agent Chain */ if (hideSequentialOutputs) { this.contentParts = this.contentParts.filter((part, index) => { diff --git a/api/server/controllers/agents/openai.js b/api/server/controllers/agents/openai.js index 4a5dd7b423..c2bc5135a6 100644 --- a/api/server/controllers/agents/openai.js +++ b/api/server/controllers/agents/openai.js @@ -20,6 +20,7 @@ const { validateRequest, initializeAgent, getBalanceConfig, + extractManualSkills, createErrorResponse, recordCollectedUsage, getTransactionsConfig, @@ -28,6 +29,7 @@ const { buildNonStreamingResponse, createOpenAIStreamTracker, createOpenAIContentAggregator, + injectManualSkillPrimes, isChatCompletionValidationFailure, discoverConnectedAgents, getRemoteAgentPermissions, @@ -243,6 +245,7 @@ const OpenAIChatCompletionController = async (req, res) => { getToolFilesByIds: db.getToolFilesByIds, getCodeGeneratedFiles: db.getCodeGeneratedFiles, listSkillsByAccess: db.listSkillsByAccess, + getSkillByName: db.getSkillByName, }; const enabledCapabilities = new Set(agentsEConfig?.capabilities); @@ -264,6 +267,8 @@ const OpenAIChatCompletionController = async (req, res) => { accessibleSkillIds, }); + const manualSkills = extractManualSkills(req.body); + const primaryConfig = await initializeAgent( { req, @@ -283,6 +288,7 @@ const OpenAIChatCompletionController = async (req, res) => { codeEnvAvailable: enabledCapabilities.has(AgentCapabilities.execute_code), skillStates, defaultActiveOnShare, + manualSkills, }, dbMethods, ); @@ -431,11 +437,26 @@ const OpenAIChatCompletionController = async (req, res) => { const openaiMessages = convertMessages(request.messages); const toolSet = buildToolSet(primaryConfig); - const { - messages: formattedMessages, - indexTokenCountMap, - summary: initialSummary, - } = formatAgentMessages(openaiMessages, {}, toolSet); + const formatted = formatAgentMessages(openaiMessages, {}, toolSet); + const formattedMessages = formatted.messages; + const initialSummary = formatted.summary; + let indexTokenCountMap = formatted.indexTokenCountMap; + + /** + * Inject manual skill primes so the model sees SKILL.md bodies for this + * turn β€” parity with AgentClient's chat path. OpenAI-compatible streaming + * uses its own tracker/aggregator shape, so the LibreChat-style card SSE + * events don't apply here; only the message-context part carries over. + */ + const manualSkillPrimes = primaryConfig.manualSkillPrimes; + if (manualSkillPrimes && manualSkillPrimes.length > 0) { + const primeResult = injectManualSkillPrimes({ + initialMessages: formattedMessages, + indexTokenCountMap, + manualSkillPrimes, + }); + indexTokenCountMap = primeResult.indexTokenCountMap; + } /** * Create a simple handler that processes data diff --git a/api/server/controllers/agents/responses.js b/api/server/controllers/agents/responses.js index 585a41c4a4..bbb986905f 100644 --- a/api/server/controllers/agents/responses.js +++ b/api/server/controllers/agents/responses.js @@ -19,6 +19,8 @@ const { getBalanceConfig, recordCollectedUsage, getTransactionsConfig, + extractManualSkills, + injectManualSkillPrimes, createToolExecuteHandler, discoverConnectedAgents, getRemoteAgentPermissions, @@ -370,6 +372,7 @@ const createResponse = async (req, res) => { getToolFilesByIds: db.getToolFilesByIds, getCodeGeneratedFiles: db.getCodeGeneratedFiles, listSkillsByAccess: db.listSkillsByAccess, + getSkillByName: db.getSkillByName, }; const enabledCapabilities = new Set( @@ -393,6 +396,8 @@ const createResponse = async (req, res) => { accessibleSkillIds, }); + const manualSkills = extractManualSkills(req.body); + const primaryConfig = await initializeAgent( { req, @@ -412,6 +417,7 @@ const createResponse = async (req, res) => { codeEnvAvailable: enabledCapabilities.has(AgentCapabilities.execute_code), skillStates, defaultActiveOnShare, + manualSkills, }, dbMethods, ); @@ -521,11 +527,26 @@ const createResponse = async (req, res) => { const allMessages = [...previousMessages, ...inputMessages]; const toolSet = buildToolSet(primaryConfig); - const { - messages: formattedMessages, - indexTokenCountMap, - summary: initialSummary, - } = formatAgentMessages(allMessages, {}, toolSet); + const formatted = formatAgentMessages(allMessages, {}, toolSet); + const formattedMessages = formatted.messages; + const initialSummary = formatted.summary; + let indexTokenCountMap = formatted.indexTokenCountMap; + + /** + * Inject manual skill primes so the model sees SKILL.md bodies for this + * turn β€” parity with AgentClient's chat path. The Responses API uses its + * own response-builder shape, so LibreChat-style card SSE events don't + * apply; only the message-context part carries over. + */ + const manualSkillPrimes = primaryConfig.manualSkillPrimes; + if (manualSkillPrimes && manualSkillPrimes.length > 0) { + const primeResult = injectManualSkillPrimes({ + initialMessages: formattedMessages, + indexTokenCountMap, + manualSkillPrimes, + }); + indexTokenCountMap = primeResult.indexTokenCountMap; + } // Create tracker for streaming or aggregator for non-streaming const tracker = actuallyStreaming ? createResponseTracker() : null; diff --git a/api/server/services/Endpoints/agents/initialize.js b/api/server/services/Endpoints/agents/initialize.js index 32f897b23f..19b9ffcfe1 100644 --- a/api/server/services/Endpoints/agents/initialize.js +++ b/api/server/services/Endpoints/agents/initialize.js @@ -6,6 +6,7 @@ const { initializeAgent, primeInvokedSkills, validateAgentModel, + extractManualSkills, GenerationJobManager, getCustomEndpointConfig, discoverConnectedAgents, @@ -233,6 +234,15 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { const conversationId = req.body.conversationId; /** @type {string | undefined} */ const parentMessageId = req.body.parentMessageId; + /** + * Skill names the user invoked via the `$` popover for this turn. Only flows + * to the primary agent β€” handoff agents are follow-up turns that don't see + * the user's per-submission `$` selections. `extractManualSkills` also + * drops non-string / empty elements so a crafted payload can't reach the + * `getSkillByName` DB query with nonsense values. + * @type {string[] | undefined} + */ + const manualSkills = extractManualSkills(req.body); const primaryConfig = await initializeAgent( { @@ -253,6 +263,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { codeEnvAvailable: enabledCapabilities.has(AgentCapabilities.execute_code), skillStates, defaultActiveOnShare, + manualSkills, }, { getFiles: db.getFiles, @@ -266,6 +277,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { getCodeGeneratedFiles: db.getCodeGeneratedFiles, filterFilesByAgentAccess, listSkillsByAccess: db.listSkillsByAccess, + getSkillByName: db.getSkillByName, }, ); @@ -319,6 +331,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => { getCodeGeneratedFiles: db.getCodeGeneratedFiles, filterFilesByAgentAccess, listSkillsByAccess: db.listSkillsByAccess, + getSkillByName: db.getSkillByName, }, // The callback fires during BFS, before the helper prunes agents // whose edges end up filtered. Don't populate `agentConfigs` here β€” diff --git a/client/src/common/types.ts b/client/src/common/types.ts index 6ca408685f..1db8ba3b3e 100644 --- a/client/src/common/types.ts +++ b/client/src/common/types.ts @@ -349,6 +349,14 @@ export type TOptions = { isResubmission?: boolean; /** Currently only utilized when `isResubmission === true`, uses that message's currently attached files */ overrideFiles?: t.TMessage['files']; + /** + * Carry forward a user message's manually-invoked skills when the caller + * is resubmitting / regenerating that same message β€” the compose-time + * atom has already been drained on the original submit, so without this + * the second turn would run without any manual priming even though the + * pills are still visible on the user bubble. + */ + overrideManualSkills?: string[]; /** Added conversation for multi-convo feature - sent to server as part of submission payload */ addedConvo?: t.TConversation; }; diff --git a/client/src/components/Chat/Input/ChatForm.tsx b/client/src/components/Chat/Input/ChatForm.tsx index 3f62b716b4..a4f4f062f5 100644 --- a/client/src/components/Chat/Input/ChatForm.tsx +++ b/client/src/components/Chat/Input/ChatForm.tsx @@ -26,6 +26,7 @@ import AttachFileChat from './Files/AttachFileChat'; import FileFormChat from './Files/FileFormChat'; import { cn, removeFocusRings } from '~/utils'; import TextareaHeader from './TextareaHeader'; +import PendingManualSkillsChips from './PendingManualSkillsChips'; import SkillsCommand from './SkillsCommand'; import PromptsCommand from './PromptsCommand'; import AudioRecorder from './AudioRecorder'; @@ -272,6 +273,7 @@ const ChatForm = memo(function ChatForm({ )} > + {/* WIP */} { + setSkills((prev) => prev.filter((s) => s !== name)); + }, + [setSkills], + ); + + if (skills.length === 0) { + return null; + } + + return ( +
+ {skills.map((name) => ( + + + ))} +
+ ); +} + +export default memo(PendingManualSkillsChips); diff --git a/client/src/components/Chat/Input/SkillsCommand.tsx b/client/src/components/Chat/Input/SkillsCommand.tsx index ab768ab8bd..88b94edb51 100644 --- a/client/src/components/Chat/Input/SkillsCommand.tsx +++ b/client/src/components/Chat/Input/SkillsCommand.tsx @@ -214,30 +214,18 @@ function SkillsCommandContent({ return { ...(prev || {}), skills: true }; }); - /* Structured channel for manual skill invocations. The follow-up PR - will read this in the submit pipeline and prime the corresponding - SKILL.md as a meta user message before the LLM turn, mirroring - Claude Code's `/skill` deterministic injection. The textual - `$skill-name ` insertion below remains as user-visible confirmation - and is treated as cosmetic by that future pipeline. */ + /* Structured channel for manual skill invocations. The submit + pipeline reads this and primes SKILL.md as a meta user message + before the LLM turn β€” no textarea-level marker is needed, and + injecting `$skill-name ` as text would mislead users into thinking + free-form text invocation is supported. Visual confirmation after + submit comes from `ManualSkillPills` on the user message bubble + until the live skill-card stream takes over. */ setPendingManualSkills((prev) => prev.includes(mention.value) ? prev : [...prev, mention.value], ); - const textarea = textAreaRef.current; - if (textarea) { - const insertion = `$${mention.value} `; - const nativeInputValueSetter = Object.getOwnPropertyDescriptor( - HTMLTextAreaElement.prototype, - 'value', - )?.set; - if (nativeInputValueSetter) { - nativeInputValueSetter.call(textarea, insertion); - textarea.dispatchEvent(new Event('input', { bubbles: true })); - } - textarea.focus(); - textarea.setSelectionRange(insertion.length, insertion.length); - } + textAreaRef.current?.focus(); }, [ setSearchValue, diff --git a/client/src/components/Chat/Input/__tests__/PendingManualSkillsChips.spec.tsx b/client/src/components/Chat/Input/__tests__/PendingManualSkillsChips.spec.tsx new file mode 100644 index 0000000000..a8eb7984f4 --- /dev/null +++ b/client/src/components/Chat/Input/__tests__/PendingManualSkillsChips.spec.tsx @@ -0,0 +1,59 @@ +import React from 'react'; +import { RecoilRoot, MutableSnapshot } from 'recoil'; +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; + +jest.mock('~/hooks', () => ({ + useLocalize: () => (key: string, params?: Record) => + `${key}:${params?.[0] ?? ''}`, +})); + +import PendingManualSkillsChips from '../PendingManualSkillsChips'; +import store from '~/store'; + +const CONVO_ID = 'convo-1'; + +const renderWithSkills = (initialSkills: string[]) => { + const initializeState = (snapshot: MutableSnapshot) => { + snapshot.set(store.pendingManualSkillsByConvoId(CONVO_ID), initialSkills); + }; + return render( + + + , + ); +}; + +describe('PendingManualSkillsChips', () => { + it('renders nothing when no skills are queued', () => { + const { container } = renderWithSkills([]); + expect(container.firstChild).toBeNull(); + }); + + it('renders one chip per queued skill', () => { + renderWithSkills(['brand-guidelines', 'pptx']); + const items = screen.getAllByRole('listitem'); + expect(items).toHaveLength(2); + expect(items[0]).toHaveTextContent('brand-guidelines'); + expect(items[1]).toHaveTextContent('pptx'); + }); + + it('removes the chip when its Γ— button is clicked', async () => { + const user = userEvent.setup(); + renderWithSkills(['brand-guidelines', 'pptx']); + const removeBrand = screen.getByRole('button', { + name: 'com_ui_remove_skill_var:brand-guidelines', + }); + await user.click(removeBrand); + const items = screen.getAllByRole('listitem'); + expect(items).toHaveLength(1); + expect(items[0]).toHaveTextContent('pptx'); + }); + + it('clears the list when every chip is dismissed', async () => { + const user = userEvent.setup(); + const { container } = renderWithSkills(['a']); + await user.click(screen.getByRole('button', { name: 'com_ui_remove_skill_var:a' })); + expect(container.firstChild).toBeNull(); + }); +}); diff --git a/client/src/components/Chat/Input/__tests__/SkillsCommand.spec.tsx b/client/src/components/Chat/Input/__tests__/SkillsCommand.spec.tsx index 3366779cb8..18ac124f77 100644 --- a/client/src/components/Chat/Input/__tests__/SkillsCommand.spec.tsx +++ b/client/src/components/Chat/Input/__tests__/SkillsCommand.spec.tsx @@ -190,7 +190,7 @@ describe('SkillsCommand', () => { expect(container).toBeEmptyDOMElement(); }); - it('selecting a skill pushes to pendingManualSkillsByConvoId, flips ephemeralAgent.skills, inserts $name into textarea, and closes the popover', async () => { + it('selecting a skill pushes to pendingManualSkillsByConvoId, flips ephemeralAgent.skills, strips the $ trigger from the textarea, and closes the popover', async () => { const user = userEvent.setup(); const textAreaRef = makeTextarea('$'); render(); @@ -200,8 +200,8 @@ describe('SkillsCommand', () => { await user.click(skillButton); }); - /* Structured channel: the skill name is pushed into the per-convo atom, - which is the contract the follow-up PR depends on. */ + /* Structured channel: the skill name is pushed into the per-convo atom + and drained by `useChatFunctions.ask` on submission. */ expect(mockSetPendingManualSkills).toHaveBeenCalledTimes(1); const updater = mockSetPendingManualSkills.mock.calls[0][0] as (prev: string[]) => string[]; expect(updater([])).toEqual(['brand-guidelines']); @@ -216,8 +216,11 @@ describe('SkillsCommand', () => { expect(agentUpdater(null)).toEqual({ skills: true }); expect(agentUpdater({ skills: true })).toEqual({ skills: true }); - /* Cosmetic textarea insertion remains in place for user feedback. */ - expect(textAreaRef.current?.value).toBe('$brand-guidelines '); + /* Textarea is cleared of the `$` trigger but no `$skill-name ` cue is + inserted β€” visual confirmation is the `ManualSkillPills` row that + renders on the submitted user message, and injecting text would + mislead users into thinking free-form `$name` invocation works. */ + expect(textAreaRef.current?.value).toBe(''); /* Popover dismisses on selection. */ expect(mockSetShowSkillsPopover).toHaveBeenCalledWith(false); diff --git a/client/src/components/Chat/Messages/Content/Container.tsx b/client/src/components/Chat/Messages/Content/Container.tsx index 7e208f3bd1..6a34016c48 100644 --- a/client/src/components/Chat/Messages/Content/Container.tsx +++ b/client/src/components/Chat/Messages/Content/Container.tsx @@ -1,5 +1,6 @@ import { TMessage } from 'librechat-data-provider'; import Files from './Files'; +import ManualSkillPills from './ManualSkillPills'; const Container = ({ children, message }: { children: React.ReactNode; message?: TMessage }) => (
{message?.isCreatedByUser === true && } + {message?.isCreatedByUser === true && } {children}
); diff --git a/client/src/components/Chat/Messages/Content/ContentParts.tsx b/client/src/components/Chat/Messages/Content/ContentParts.tsx index 65ebc66908..0380e77f80 100644 --- a/client/src/components/Chat/Messages/Content/ContentParts.tsx +++ b/client/src/components/Chat/Messages/Content/ContentParts.tsx @@ -10,6 +10,7 @@ import { ParallelContentRenderer, type PartWithIndex } from './ParallelContent'; import { mapAttachments, groupSequentialToolCalls } from '~/utils'; import { MessageContext, SearchContext } from '~/Providers'; import { EditTextPart, EmptyText } from './Parts'; +import PendingSkillCall from './Parts/PendingSkillCall'; import MemoryArtifacts from './MemoryArtifacts'; import ToolCallGroup from './ToolCallGroup'; import Container from './Container'; @@ -73,6 +74,15 @@ const PartWithContext = memo(function PartWithContext({ type ContentPartsProps = { content: Array | undefined; messageId: string; + /** + * Skill names the user invoked manually via the `$` popover on this turn. + * `createdHandler` seeds this on the assistant placeholder from + * `submission.manualSkills`, and `finalHandler`'s server-backed + * `responseMessage` replacement drops it β€” so the field is naturally + * present only for the lifetime of the stream. Scalar string array (not + * the full message object) so `React.memo` stays shallow-happy. + */ + manualSkills?: string[]; conversationId?: string | null; attachments?: TAttachment[]; searchResults?: { [key: string]: SearchResultData }; @@ -99,6 +109,7 @@ const ContentParts = memo(function ContentParts({ edit, isLast, content, + manualSkills, messageId, enterEdit, siblingIdx, @@ -113,6 +124,63 @@ const ContentParts = memo(function ContentParts({ const attachmentMap = useMemo(() => mapAttachments(attachments ?? []), [attachments]); const effectiveIsSubmitting = isLatestMessage ? isSubmitting : false; + /** + * Interim skill cards β€” rendered in a separate slot ABOVE the Parts + * iteration based purely on the `manualSkills` message field. `content` + * is only read to determine the "Running β†’ Ran" visual transition + * (`hasRealContent`), never to gate visibility, so backend deltas / + * optimistic emissions can't race the pending cards off the screen. + * + * Lifecycle: + * - `useChatFunctions` seeds `manualSkills` on the assistant placeholder + * at construction β†’ cards appear immediately on submit, with the + * shimmering "Running X" state (no content yet). + * - Through the stream, `useStepHandler` spreads the response on every + * update so `manualSkills` rides along; once the first real content + * part lands, `hasRealContent` flips true and the cards switch to + * the static "Ran X" state β€” matching what users see for + * model-invoked skills as they finish priming. + * - At finalize, `finalHandler` replaces the message with the server + * response (no `manualSkills` field) β†’ interim cards disappear and + * the real `skill` tool_call part in `content` takes over. + * + * Skipped on the user side (they get `ManualSkillPills` on the user + * bubble) and when no skills were invoked on this turn. + */ + const pendingSkills = useMemo( + () => (!isCreatedByUser && manualSkills != null ? manualSkills : []), + [isCreatedByUser, manualSkills], + ); + const hasPendingSkills = pendingSkills.length > 0; + + /** + * True once the assistant has started streaming something meaningful β€” + * any non-text part, OR a text part with non-empty content. Drives the + * "Running X β†’ Ran X" transition on pending cards. An empty-text + * placeholder (some endpoints seed one in `initialResponse.content` on + * assistant-side) does NOT count as real content, to avoid flipping + * the transition before the model has actually produced anything. + */ + const hasRealContent = useMemo( + () => + (content ?? []).some((part) => { + if (part == null) { + return false; + } + if (part.type !== ContentTypes.TEXT) { + return true; + } + const text = typeof part.text === 'string' ? part.text : (part.text?.value ?? ''); + return text.length > 0; + }), + [content], + ); + + const renderPendingSkills = () => + pendingSkills.map((name) => ( + + )); + const renderPart = useCallback( (part: TMessageContentParts, idx: number, isLastPart: boolean) => { const toolCallId = (part?.[ContentTypes.TOOL_CALL] as Agents.ToolCall | undefined)?.id ?? ''; @@ -145,16 +213,17 @@ const ContentParts = memo(function ContentParts({ ], ); - // Early return: no content - if (!content) { + // Early return: no content to render AND no pending skill cards + if (!content && !hasPendingSkills) { return null; } - // Edit mode: render editable text parts + // Edit mode: render editable text parts. Interim skill cards are a + // mid-stream concern, not relevant in edit mode. if (edit === true && enterEdit && setSiblingIdx) { return ( <> - {content.map((part, idx) => { + {(content ?? []).map((part, idx) => { if (!part) { return null; } @@ -190,28 +259,32 @@ const ContentParts = memo(function ContentParts({ ); } - const showEmptyCursor = content.length === 0 && effectiveIsSubmitting; - const lastContentIdx = content.length - 1; + const safeContent = content ?? []; + const showEmptyCursor = safeContent.length === 0 && effectiveIsSubmitting; + const lastContentIdx = safeContent.length - 1; // Parallel content: use dedicated renderer with columns (TMessageContentParts includes ContentMetadata) - const hasParallelContent = content.some((part) => part?.groupId != null); + const hasParallelContent = safeContent.some((part) => part?.groupId != null); if (hasParallelContent) { return ( - + <> + {renderPendingSkills()} + + ); } // Sequential content: render parts in order (90% of cases) const sequentialParts: PartWithIndex[] = []; - content.forEach((part, idx) => { + safeContent.forEach((part, idx) => { if (part) { sequentialParts.push({ part, idx }); } @@ -221,6 +294,7 @@ const ContentParts = memo(function ContentParts({ return ( + {renderPendingSkills()} {showEmptyCursor && ( diff --git a/client/src/components/Chat/Messages/Content/EditMessage.tsx b/client/src/components/Chat/Messages/Content/EditMessage.tsx index 0b4a15f7cb..53fa4f8800 100644 --- a/client/src/components/Chat/Messages/Content/EditMessage.tsx +++ b/client/src/components/Chat/Messages/Content/EditMessage.tsx @@ -61,6 +61,10 @@ const EditMessage = ({ }, { overrideFiles: message.files, + /** Pills on the edited user message stay visible after save-and-submit; + * carry the picks forward so the new turn primes the same skills + * instead of running unprimed. */ + overrideManualSkills: message.manualSkills, addedConvo: getAddedConvo() || undefined, }, ); @@ -80,6 +84,10 @@ const EditMessage = ({ editedMessageId: messageId, isRegenerate: true, isEdited: true, + /** Edit-assistant-response flow replays the parent user turn; keep + * the same manual skills so the regenerated response is primed + * identically. */ + overrideManualSkills: parentMessage.manualSkills, addedConvo: getAddedConvo() || undefined, }, ); diff --git a/client/src/components/Chat/Messages/Content/ManualSkillPills.tsx b/client/src/components/Chat/Messages/Content/ManualSkillPills.tsx new file mode 100644 index 0000000000..de5fcd6322 --- /dev/null +++ b/client/src/components/Chat/Messages/Content/ManualSkillPills.tsx @@ -0,0 +1,42 @@ +import { memo } from 'react'; +import { ScrollText } from 'lucide-react'; +import { useLocalize } from '~/hooks'; + +/** + * Compact pill row rendered on a submitted user message, one chip per skill + * the user invoked via the `$` popover. Presentational component β€” takes + * only the scalar `skills` array, no full message object (keeps + * `React.memo` comparisons on parent wrappers shallow and cheap). + * + * Backend persists the source field (`message.manualSkills`), so callers + * reading from the message pass `skills={message.manualSkills}` and pills + * survive page reloads / history renders. + */ +function ManualSkillPills({ skills }: { skills?: string[] }) { + const localize = useLocalize(); + + if (!skills || skills.length === 0) { + return null; + } + + return ( +
+ {skills.map((name) => ( + + + ))} +
+ ); +} + +export default memo(ManualSkillPills); diff --git a/client/src/components/Chat/Messages/Content/ParallelContent.tsx b/client/src/components/Chat/Messages/Content/ParallelContent.tsx index b66720cad7..2c56bc04e3 100644 --- a/client/src/components/Chat/Messages/Content/ParallelContent.tsx +++ b/client/src/components/Chat/Messages/Content/ParallelContent.tsx @@ -191,7 +191,7 @@ export const ParallelColumns = memo(function ParallelColumns({ }); type ParallelContentRendererProps = { - content: Array; + content?: Array; messageId: string; conversationId?: string | null; attachments?: TAttachment[]; @@ -218,7 +218,7 @@ export const ParallelContentRenderer = memo(function ParallelContentRenderer({ [content], ); - const lastContentIdx = content.length - 1; + const lastContentIdx = (content?.length ?? 0) - 1; // Split sequential parts into before/after parallel sections const { before, after } = useMemo(() => { diff --git a/client/src/components/Chat/Messages/Content/Parts/PendingSkillCall.tsx b/client/src/components/Chat/Messages/Content/Parts/PendingSkillCall.tsx new file mode 100644 index 0000000000..15b0d1f2a7 --- /dev/null +++ b/client/src/components/Chat/Messages/Content/Parts/PendingSkillCall.tsx @@ -0,0 +1,53 @@ +import { ScrollText } from 'lucide-react'; +import { useLocalize } from '~/hooks'; +import { cn } from '~/utils'; + +/** + * Minimal variant of `SkillCall` used for the interim window between + * submit and the backend's real `skill` tool_call content part landing + * on the message. + * + * Strips the expand/collapse affordance that `SkillCall` inherits from + * `ProgressText` β€” there's nothing to expand during priming (empty + * output), and leaving the chevron + cursor-pointer on a non-functional + * button is misleading. Same ScrollText icon, same shimmer, same + * localized label strings (`com_ui_skill_running` / `com_ui_skill_finished`) + * so the visual language matches the real card it'll be replaced by. + * + * `loaded=false` β†’ "Running X" with pulsing icon + shimmer. + * `loaded=true` β†’ "Ran X" with static icon; driven by + * `ContentParts.hasRealContent`, which flips true as + * soon as any streamed content part arrives, matching + * the transition users see for model-invoked skills. + */ +export default function PendingSkillCall({ + skillName, + loaded, +}: { + skillName: string; + loaded: boolean; +}) { + const localize = useLocalize(); + const text = loaded + ? localize('com_ui_skill_finished', { 0: skillName }) + : localize('com_ui_skill_running', { 0: skillName }); + + return ( +
+
+
+
+
+
+
+
+ ); +} diff --git a/client/src/components/Chat/Messages/Content/__tests__/ContentParts.test.tsx b/client/src/components/Chat/Messages/Content/__tests__/ContentParts.test.tsx new file mode 100644 index 0000000000..55312cb8d5 --- /dev/null +++ b/client/src/components/Chat/Messages/Content/__tests__/ContentParts.test.tsx @@ -0,0 +1,140 @@ +import React from 'react'; +import { render, screen } from '@testing-library/react'; +import { ContentTypes } from 'librechat-data-provider'; +import type { TMessageContentParts } from 'librechat-data-provider'; + +jest.mock('~/utils', () => ({ + mapAttachments: () => ({}), + groupSequentialToolCalls: (parts: Array<{ part: unknown; idx: number }>) => + parts.map((p) => ({ type: 'single' as const, part: p })), +})); + +jest.mock('~/Providers', () => ({ + MessageContext: { + Provider: ({ children }: { children: React.ReactNode }) => <>{children}, + }, + SearchContext: { + Provider: ({ children }: { children: React.ReactNode }) => <>{children}, + }, +})); + +jest.mock('../Parts', () => ({ + EditTextPart: () =>
, + EmptyText: () =>
, +})); + +jest.mock('../MemoryArtifacts', () => ({ + __esModule: true, + default: () =>
, +})); + +jest.mock('../Parts/PendingSkillCall', () => ({ + __esModule: true, + default: ({ skillName, loaded }: { skillName: string; loaded: boolean }) => ( +
+ ), +})); + +jest.mock('../ToolCallGroup', () => ({ + __esModule: true, + default: () =>
, +})); + +jest.mock('../Container', () => ({ + __esModule: true, + default: ({ children }: { children: React.ReactNode }) => ( +
{children}
+ ), +})); + +jest.mock('../Part', () => ({ + __esModule: true, + default: ({ part }: { part: TMessageContentParts }) => ( +
+ ), +})); + +jest.mock('../ParallelContent', () => ({ + ParallelContentRenderer: () =>
, +})); + +import ContentParts from '../ContentParts'; + +const baseProps = { + messageId: 'msg-1', + isLast: false, + isSubmitting: false, + isLatestMessage: false, + isCreatedByUser: false, + content: [], +}; + +describe('ContentParts β€” interim skill cards', () => { + it('renders a PendingSkillCall per manual skill on assistant messages', () => { + render(); + const cards = screen.getAllByTestId('pending-skill-call'); + expect(cards).toHaveLength(2); + expect(cards[0]).toHaveAttribute('data-skill', 'brand-guidelines'); + expect(cards[1]).toHaveAttribute('data-skill', 'pptx'); + }); + + it('starts pending skill cards in the not-loaded state (no real content yet)', () => { + render(); + expect(screen.getByTestId('pending-skill-call')).toHaveAttribute('data-loaded', 'false'); + }); + + it('flips pending cards to loaded once any real content part arrives', () => { + const content: TMessageContentParts[] = [ + { type: ContentTypes.TEXT, text: 'streamed' } as unknown as TMessageContentParts, + ]; + render(); + expect(screen.getByTestId('pending-skill-call')).toHaveAttribute('data-loaded', 'true'); + }); + + it('does NOT render skill cards on user messages', () => { + render(); + expect(screen.queryByTestId('pending-skill-call')).toBeNull(); + }); + + it('renders nothing when manualSkills is empty and content is undefined', () => { + const { container } = render( + , + ); + expect(container.firstChild).toBeNull(); + }); + + it('renders pending skill cards even when content is undefined', () => { + render(); + expect(screen.getAllByTestId('pending-skill-call')).toHaveLength(1); + }); + + it('renders pending skill cards above parallel content', () => { + const parallelContent: TMessageContentParts[] = [ + { + type: ContentTypes.TEXT, + text: 'parallel', + groupId: 'group-1', + } as unknown as TMessageContentParts, + ]; + render(); + const skillCard = screen.getByTestId('pending-skill-call'); + const parallelRenderer = screen.getByTestId('parallel-renderer'); + expect(skillCard).toBeTruthy(); + expect(parallelRenderer).toBeTruthy(); + expect(skillCard.compareDocumentPosition(parallelRenderer)).toBe( + Node.DOCUMENT_POSITION_FOLLOWING, + ); + }); + + it('renders pending skill cards above sequential content', () => { + const sequentialContent: TMessageContentParts[] = [ + { type: ContentTypes.TEXT, text: 'streamed' } as unknown as TMessageContentParts, + ]; + render(); + const skillCard = screen.getByTestId('pending-skill-call'); + const textPart = screen.getByTestId(`real-part-${ContentTypes.TEXT}`); + expect(skillCard).toBeTruthy(); + expect(textPart).toBeTruthy(); + expect(skillCard.compareDocumentPosition(textPart)).toBe(Node.DOCUMENT_POSITION_FOLLOWING); + }); +}); diff --git a/client/src/components/Chat/Messages/Content/__tests__/ManualSkillPills.test.tsx b/client/src/components/Chat/Messages/Content/__tests__/ManualSkillPills.test.tsx new file mode 100644 index 0000000000..8066c507f3 --- /dev/null +++ b/client/src/components/Chat/Messages/Content/__tests__/ManualSkillPills.test.tsx @@ -0,0 +1,34 @@ +import React from 'react'; +import { render, screen } from '@testing-library/react'; + +jest.mock('~/hooks', () => ({ + useLocalize: () => (key: string, params?: Record) => + `${key}:${params?.[0] ?? ''}`, +})); + +import ManualSkillPills from '../ManualSkillPills'; + +describe('ManualSkillPills', () => { + it('renders nothing when skills is undefined', () => { + const { container } = render(); + expect(container.firstChild).toBeNull(); + }); + + it('renders nothing when skills is empty', () => { + const { container } = render(); + expect(container.firstChild).toBeNull(); + }); + + it('renders one pill per entry', () => { + render(); + const items = screen.getAllByRole('listitem'); + expect(items).toHaveLength(2); + expect(items[0]).toHaveTextContent('brand-guidelines'); + expect(items[1]).toHaveTextContent('pptx'); + }); + + it('localizes the list aria-label', () => { + render(); + expect(screen.getByRole('list')).toHaveAttribute('aria-label', 'com_ui_skills_manual_invoked:'); + }); +}); diff --git a/client/src/components/Chat/Messages/MessageParts.tsx b/client/src/components/Chat/Messages/MessageParts.tsx index 2d0e3d512b..ff6b01b2dd 100644 --- a/client/src/components/Chat/Messages/MessageParts.tsx +++ b/client/src/components/Chat/Messages/MessageParts.tsx @@ -141,6 +141,7 @@ export default function Message(props: TMessageProps) { attachments={attachments} isSubmitting={isSubmitting} searchResults={searchResults} + manualSkills={message.manualSkills} messageId={message.messageId} setSiblingIdx={setSiblingIdx} isCreatedByUser={message.isCreatedByUser} diff --git a/client/src/components/Messages/ContentRender.tsx b/client/src/components/Messages/ContentRender.tsx index 4ba8db36f8..bb7925cf1a 100644 --- a/client/src/components/Messages/ContentRender.tsx +++ b/client/src/components/Messages/ContentRender.tsx @@ -78,7 +78,8 @@ function areContentRenderPropsEqual(prev: ContentRenderProps, next: ContentRende prevMsg.endpoint === nextMsg.endpoint && prevMsg.iconURL === nextMsg.iconURL && prevMsg.feedback?.rating === nextMsg.feedback?.rating && - (prevMsg.attachments?.length ?? 0) === (nextMsg.attachments?.length ?? 0) + (prevMsg.attachments?.length ?? 0) === (nextMsg.attachments?.length ?? 0) && + (prevMsg.manualSkills?.length ?? 0) === (nextMsg.manualSkills?.length ?? 0) ); } @@ -216,6 +217,7 @@ const ContentRender = memo(function ContentRender({ messageId={msg.messageId} attachments={attachments} searchResults={searchResults} + manualSkills={msg.manualSkills} setSiblingIdx={setSiblingIdx} isLatestMessage={isLatestMessage} isSubmitting={isSubmitting} diff --git a/client/src/hooks/Chat/useChatFunctions.ts b/client/src/hooks/Chat/useChatFunctions.ts index 18aaf0daee..34c6990b07 100644 --- a/client/src/hooks/Chat/useChatFunctions.ts +++ b/client/src/hooks/Chat/useChatFunctions.ts @@ -2,7 +2,7 @@ import { v4 } from 'uuid'; import { cloneDeep } from 'lodash'; import { useNavigate } from 'react-router-dom'; import { useQueryClient } from '@tanstack/react-query'; -import { useSetRecoilState, useResetRecoilState, useRecoilValue } from 'recoil'; +import { useSetRecoilState, useResetRecoilState, useRecoilValue, useRecoilCallback } from 'recoil'; import { Constants, QueryKeys, @@ -76,6 +76,30 @@ export default function useChatFunctions({ const setShowStopButton = useSetRecoilState(store.showStopButtonByIndex(index)); const resetLatestMultiMessage = useResetRecoilState(store.latestMessageFamily(index + 1)); + /** + * Atomically read + reset the per-conversation queue of manually-invoked + * skills from the `$` popover. Reading and resetting in a single Recoil + * snapshot guarantees that if the user selects more skills between here and + * the next submission, their picks are never silently lost into a reset atom. + * + * The `hasValue` guard is defensive: this atom has a synchronous default of + * `[]` so `.contents` is always the resolved value in practice, but reading + * `.contents` on a loading/errored loadable yields a Promise/Error, which + * would make the `string[]` cast unsound. + */ + const drainPendingManualSkills = useRecoilCallback( + ({ snapshot, reset }) => + (convoId: string): string[] => { + const loadable = snapshot.getLoadable(store.pendingManualSkillsByConvoId(convoId)); + const skills = loadable.state === 'hasValue' ? (loadable.contents as string[]) : []; + if (skills.length > 0) { + reset(store.pendingManualSkillsByConvoId(convoId)); + } + return skills; + }, + [], + ); + const ask: TAskFunction = ( { text, @@ -93,6 +117,7 @@ export default function useChatFunctions({ isEdited = false, overrideMessages, overrideFiles, + overrideManualSkills, addedConvo, } = {}, ) => { @@ -124,6 +149,23 @@ export default function useChatFunctions({ } const ephemeralAgent = getEphemeralAgent(conversationId ?? Constants.NEW_CONVO); + /** + * Manual skill selection resolution: + * - Explicit `overrideManualSkills` wins (regenerate / save-and-submit + * pass the original user message's persisted `manualSkills` so the + * resubmitted turn primes the same skills β€” the pills are still + * visible to the user, it would be strange to quietly drop them). + * - Regenerate / continue / edit without an override β†’ empty, and the + * compose-time atom is deliberately NOT drained (those flows replay + * a prior turn, not compose a new one). + * - Fresh submit β†’ drain the per-convo atom into the message. + */ + const manualSkills = + overrideManualSkills != null + ? overrideManualSkills + : isRegenerate || isContinued || isEdited + ? [] + : drainPendingManualSkills(conversationId ?? Constants.NEW_CONVO); const isEditOrContinue = isEdited || isContinued; let currentMessages: TMessage[] | null = overrideMessages ?? getMessages() ?? []; @@ -215,6 +257,13 @@ export default function useChatFunctions({ messageId: isContinued && messageId != null && messageId ? messageId : intermediateId, thread_id, error: false, + /** + * UI-only metadata. Survives reload because the backend persists the + * field on the message schema, and `ManualSkillPills` reads straight + * off the message so there's no Recoil state to clean up. Runtime + * skill resolution reads the top-level `manualSkills` payload field. + */ + manualSkills: manualSkills.length > 0 ? manualSkills : undefined, }; const submissionFiles = overrideFiles ?? targetParentMessage?.files; @@ -261,6 +310,16 @@ export default function useChatFunctions({ model: convo?.model, error: false, iconURL, + /** + * Seed the assistant placeholder with the turn's manually-invoked + * skill names so `ContentParts` can render interim `SkillCall` cards + * from the very first render β€” no round-trip through the `created` + * SSE event required. Rides along with every subsequent spread + * (`useStepHandler` response construction, `updateContent` result + * spreads) and drops out naturally at `finalHandler` when the + * server-backed `responseMessage` replacement takes over. + */ + manualSkills: manualSkills.length > 0 ? manualSkills : undefined, }; if (isAssistantsEndpoint(endpoint)) { @@ -332,6 +391,7 @@ export default function useChatFunctions({ ephemeralAgent, editedContent, addedConvo, + manualSkills: manualSkills.length > 0 ? manualSkills : undefined, }; if (isRegenerate) { @@ -354,7 +414,16 @@ export default function useChatFunctions({ if (parentMessage && parentMessage.isCreatedByUser) { ask( { ...parentMessage }, - { isRegenerate: true, addedConvo: options?.addedConvo ?? undefined }, + { + isRegenerate: true, + addedConvo: options?.addedConvo ?? undefined, + /** Carry the original user message's manual skill picks forward + * so the regenerated response is primed with the same skills. + * The compose-time atom was drained on the first submit; without + * this the model sees an unprimed turn even though the pills + * still show on the user bubble. */ + overrideManualSkills: parentMessage.manualSkills, + }, ); } else { console.error( diff --git a/client/src/hooks/SSE/useEventHandlers.ts b/client/src/hooks/SSE/useEventHandlers.ts index 366775c4c1..2759fb0527 100644 --- a/client/src/hooks/SSE/useEventHandlers.ts +++ b/client/src/hooks/SSE/useEventHandlers.ts @@ -347,6 +347,16 @@ export default function useEventHandlers({ queryClient.invalidateQueries([QueryKeys.mcpConnectionStatus]); queryClient.invalidateQueries([QueryKeys.mcpTools]); const { messages, userMessage, isRegenerate = false, isTemporary = false } = submission; + /** + * The spread carries `manualSkills` through from + * `submission.initialResponse` β€” `useChatFunctions` seeds the field + * there at construction so the assistant placeholder already has it + * by the time this handler fires. Subsequent `useStepHandler` + * spreads and `updateContent` spreads preserve it, and + * `finalHandler`'s server-backed `responseMessage` replacement + * drops it, which is the right behavior: by finalize the real + * `skill` tool_call is in `content` and takes over rendering. + */ const initialResponse = { ...submission.initialResponse, parentMessageId: userMessage.messageId, diff --git a/client/src/locales/en/translation.json b/client/src/locales/en/translation.json index 24d26be0b7..e3bed4f7e7 100644 --- a/client/src/locales/en/translation.json +++ b/client/src/locales/en/translation.json @@ -1523,6 +1523,8 @@ "com_ui_skills_command_placeholder": "Select a Skill by name", "com_ui_skills_empty": "No skills yet", "com_ui_skills_load_error": "Failed to load skills", + "com_ui_skills_manual_invoked": "Manually invoked skills", + "com_ui_skills_queued": "Skills queued for next submission", "com_ui_sr_public_skill": "Public skill", "com_ui_special": "special", "com_ui_special_var_current_date": "Current Date", diff --git a/client/src/store/families.ts b/client/src/store/families.ts index f35bebc0d6..7e754e05ce 100644 --- a/client/src/store/families.ts +++ b/client/src/store/families.ts @@ -307,13 +307,12 @@ const showSkillsPopoverFamily = atomFamily({ /** * Per-conversation queue of skill names the user invoked manually via the - * `$` popover for the next submission. Acts as the structured channel - * paired with the cosmetic `$skill-name ` text inserted into the textarea. - * - * Phase 1: only the writer (SkillsCommand) is wired; the submit pipeline - * does not yet read or clear this atom. The follow-up PR will read this - * at `ask()` time, attach to the payload, and reset to `[]`. Until then - * the backend continues to receive only the textual `$name` reference. + * `$` popover for the next submission. Structured channel that the submit + * pipeline (`useChatFunctions.ask`) drains and pins onto the user message's + * `manualSkills` field (also echoed at the top of the payload for the + * runtime resolver), then resets to `[]`. Compose-time chips above the + * textarea read this atom directly so users see (and can dismiss) their + * current selection before hitting send. */ const pendingManualSkillsByConvoId = atomFamily({ key: 'pendingManualSkillsByConvoId', diff --git a/packages/api/src/agents/__tests__/initialize.test.ts b/packages/api/src/agents/__tests__/initialize.test.ts index be7377954a..f73e8953f2 100644 --- a/packages/api/src/agents/__tests__/initialize.test.ts +++ b/packages/api/src/agents/__tests__/initialize.test.ts @@ -422,3 +422,158 @@ describe('initializeAgent β€” maxContextTokens', () => { expect(result.maxContextTokens).toBe(1024); }); }); + +describe('initializeAgent β€” manual skill priming (Phase 3)', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + /** + * Minimal listSkillsByAccess that satisfies `injectSkillCatalog` so the + * manualSkills resolver branch runs. Returns an empty page β€” we don't care + * about the catalog here, only that `accessibleSkillIds` is non-empty so + * the manual-invocation block gets reached. + */ + const emptyListSkillsByAccess: InitializeAgentDbMethods['listSkillsByAccess'] = async () => ({ + skills: [], + has_more: false, + after: null, + }); + + it('attaches resolved manual skill primes to the initialized agent', async () => { + const { agent, req, res, loadTools, db } = createMocks(); + const { Types } = await import('mongoose'); + const skillId = new Types.ObjectId(); + /** + * Ownership-based active-state default only kicks in when + * `skill.author.toString() === userId`. The default mock user id is a + * literal string, not an ObjectId, so align the skill author with it so + * `resolveSkillActive` treats the skill as owned and active. + */ + const ownerAuthor = { + toString: () => req.user?.id, + } as unknown as import('mongoose').Types.ObjectId; + + const getSkillByName: InitializeAgentDbMethods['getSkillByName'] = jest.fn().mockResolvedValue({ + _id: skillId, + name: 'brand-guidelines', + body: '# Brand guidelines\nUse blue.', + author: ownerAuthor, + }); + + const result = await initializeAgent( + { + req, + res, + agent, + loadTools, + endpointOption: { endpoint: EModelEndpoint.agents }, + allowedProviders: new Set([Providers.OPENAI]), + isInitialAgent: true, + accessibleSkillIds: [skillId], + manualSkills: ['brand-guidelines'], + }, + { ...db, listSkillsByAccess: emptyListSkillsByAccess, getSkillByName }, + ); + + expect(result.manualSkillPrimes).toEqual([ + { name: 'brand-guidelines', body: '# Brand guidelines\nUse blue.' }, + ]); + expect(getSkillByName).toHaveBeenCalledWith('brand-guidelines', [skillId]); + }); + + it('leaves manualSkillPrimes undefined when no manualSkills are provided', async () => { + const { agent, req, res, loadTools, db } = createMocks(); + const { Types } = await import('mongoose'); + const skillId = new Types.ObjectId(); + + const result = await initializeAgent( + { + req, + res, + agent, + loadTools, + endpointOption: { endpoint: EModelEndpoint.agents }, + allowedProviders: new Set([Providers.OPENAI]), + isInitialAgent: true, + accessibleSkillIds: [skillId], + }, + { ...db, listSkillsByAccess: emptyListSkillsByAccess }, + ); + + expect(result.manualSkillPrimes).toBeUndefined(); + }); + + it('returns empty array when every manual skill is unresolvable (no primes, no throw)', async () => { + const { agent, req, res, loadTools, db } = createMocks(); + const { Types } = await import('mongoose'); + const skillId = new Types.ObjectId(); + + const getSkillByName: InitializeAgentDbMethods['getSkillByName'] = jest + .fn() + .mockResolvedValue(null); + + const result = await initializeAgent( + { + req, + res, + agent, + loadTools, + endpointOption: { endpoint: EModelEndpoint.agents }, + allowedProviders: new Set([Providers.OPENAI]), + isInitialAgent: true, + accessibleSkillIds: [skillId], + manualSkills: ['does-not-exist'], + }, + { ...db, listSkillsByAccess: emptyListSkillsByAccess, getSkillByName }, + ); + + expect(result.manualSkillPrimes).toEqual([]); + }); + + it('skips resolution entirely when accessibleSkillIds is empty (user has no skill access)', async () => { + const { agent, req, res, loadTools, db } = createMocks(); + const getSkillByName: InitializeAgentDbMethods['getSkillByName'] = jest.fn(); + + const result = await initializeAgent( + { + req, + res, + agent, + loadTools, + endpointOption: { endpoint: EModelEndpoint.agents }, + allowedProviders: new Set([Providers.OPENAI]), + isInitialAgent: true, + accessibleSkillIds: [], + manualSkills: ['anything'], + }, + { ...db, getSkillByName }, + ); + + expect(result.manualSkillPrimes).toBeUndefined(); + expect(getSkillByName).not.toHaveBeenCalled(); + }); + + it('silently no-ops when getSkillByName is not provided in db methods', async () => { + const { agent, req, res, loadTools, db } = createMocks(); + const { Types } = await import('mongoose'); + const skillId = new Types.ObjectId(); + + const result = await initializeAgent( + { + req, + res, + agent, + loadTools, + endpointOption: { endpoint: EModelEndpoint.agents }, + allowedProviders: new Set([Providers.OPENAI]), + isInitialAgent: true, + accessibleSkillIds: [skillId], + manualSkills: ['foo'], + }, + { ...db, listSkillsByAccess: emptyListSkillsByAccess }, + ); + + expect(result.manualSkillPrimes).toBeUndefined(); + }); +}); diff --git a/packages/api/src/agents/__tests__/skills.test.ts b/packages/api/src/agents/__tests__/skills.test.ts index 739502de31..6dfa100604 100644 --- a/packages/api/src/agents/__tests__/skills.test.ts +++ b/packages/api/src/agents/__tests__/skills.test.ts @@ -27,7 +27,19 @@ jest.mock('@librechat/agents', () => ({ })); import { Types } from 'mongoose'; -import { scopeSkillIds, resolveSkillActive, injectSkillCatalog } from '../skills'; +import { HumanMessage, AIMessage } from '@langchain/core/messages'; +import { + scopeSkillIds, + resolveSkillActive, + injectSkillCatalog, + buildSkillPrimeMessage, + resolveManualSkills, + injectManualSkillPrimes, + extractManualSkills, + isSkillPrimeMessage, + buildSkillPrimeContentParts, + MAX_MANUAL_SKILLS, +} from '../skills'; import { extractInvokedSkillsFromPayload } from '../run'; type PageSkill = { @@ -559,3 +571,475 @@ describe('injectSkillCatalog', () => { expect(result.activeSkillIds).toEqual([]); }); }); + +describe('buildSkillPrimeMessage', () => { + it('produces a meta user message carrying SKILL.md body and skillName marker', () => { + const msg = buildSkillPrimeMessage({ + name: 'brand-guidelines', + body: '# Brand guidelines\nUse blue.', + }); + expect(msg).toEqual({ + role: 'user', + content: '# Brand guidelines\nUse blue.', + isMeta: true, + source: 'skill', + skillName: 'brand-guidelines', + }); + }); + + it('preserves empty-body content verbatim (resolver should filter before, but helper is agnostic)', () => { + const msg = buildSkillPrimeMessage({ name: 'bare', body: '' }); + expect(msg.content).toBe(''); + expect(msg.isMeta).toBe(true); + }); +}); + +describe('resolveManualSkills', () => { + const userId = new Types.ObjectId().toString(); + const userOid = new Types.ObjectId(userId); + const otherAuthor = new Types.ObjectId(); + + type SkillDoc = { + _id: Types.ObjectId; + name: string; + body: string; + author: Types.ObjectId; + }; + + const buildGetSkillByName = + (skills: Record) => + async (name: string, _accessibleIds: Types.ObjectId[]) => + skills[name] ?? null; + + const mkSkill = (name: string, author: Types.ObjectId, body = `body of ${name}`): SkillDoc => ({ + _id: new Types.ObjectId(), + name, + body, + author, + }); + + it('returns empty when no names supplied', async () => { + const result = await resolveManualSkills({ + names: [], + getSkillByName: buildGetSkillByName({}), + accessibleSkillIds: [new Types.ObjectId()], + userId, + }); + expect(result).toEqual([]); + }); + + it('returns empty when no accessible IDs (ACL empty)', async () => { + const result = await resolveManualSkills({ + names: ['foo'], + getSkillByName: buildGetSkillByName({ foo: mkSkill('foo', userOid) }), + accessibleSkillIds: [], + userId, + }); + expect(result).toEqual([]); + }); + + it('resolves owned skills to { name, body } pairs by default', async () => { + const owned = mkSkill('my-skill', userOid, 'MY SKILL BODY'); + const result = await resolveManualSkills({ + names: ['my-skill'], + getSkillByName: buildGetSkillByName({ 'my-skill': owned }), + accessibleSkillIds: [owned._id], + userId, + }); + expect(result).toEqual([{ name: 'my-skill', body: 'MY SKILL BODY' }]); + }); + + it('silently skips names with no backing skill (typo / ACL miss) without failing the batch', async () => { + const real = mkSkill('real', userOid); + const result = await resolveManualSkills({ + names: ['real', 'typo'], + getSkillByName: buildGetSkillByName({ real }), + accessibleSkillIds: [real._id], + userId, + }); + expect(result).toEqual([{ name: 'real', body: 'body of real' }]); + }); + + it('dedupes repeated names, preserving first occurrence order', async () => { + const a = mkSkill('a', userOid); + const b = mkSkill('b', userOid); + const result = await resolveManualSkills({ + names: ['a', 'b', 'a', 'b', 'a'], + getSkillByName: buildGetSkillByName({ a, b }), + accessibleSkillIds: [a._id, b._id], + userId, + }); + expect(result.map((r) => r.name)).toEqual(['a', 'b']); + }); + + it('filters shared skills when defaultActiveOnShare is false (unless override active=true)', async () => { + const shared = mkSkill('shared', otherAuthor); + const result = await resolveManualSkills({ + names: ['shared'], + getSkillByName: buildGetSkillByName({ shared }), + accessibleSkillIds: [shared._id], + userId, + defaultActiveOnShare: false, + }); + expect(result).toEqual([]); + }); + + it('allows shared skills when defaultActiveOnShare is true', async () => { + const shared = mkSkill('shared', otherAuthor, 'shared-body'); + const result = await resolveManualSkills({ + names: ['shared'], + getSkillByName: buildGetSkillByName({ shared }), + accessibleSkillIds: [shared._id], + userId, + defaultActiveOnShare: true, + }); + expect(result).toEqual([{ name: 'shared', body: 'shared-body' }]); + }); + + it('drops explicitly-deactivated skills (skillStates override wins over ownership default)', async () => { + const owned = mkSkill('owned-off', userOid); + const result = await resolveManualSkills({ + names: ['owned-off'], + getSkillByName: buildGetSkillByName({ 'owned-off': owned }), + accessibleSkillIds: [owned._id], + userId, + skillStates: { [owned._id.toString()]: false }, + }); + expect(result).toEqual([]); + }); + + it('allows explicitly-activated shared skill even when defaultActiveOnShare is false', async () => { + const shared = mkSkill('shared-on', otherAuthor, 'on-body'); + const result = await resolveManualSkills({ + names: ['shared-on'], + getSkillByName: buildGetSkillByName({ 'shared-on': shared }), + accessibleSkillIds: [shared._id], + userId, + defaultActiveOnShare: false, + skillStates: { [shared._id.toString()]: true }, + }); + expect(result).toEqual([{ name: 'shared-on', body: 'on-body' }]); + }); + + it('skips skills with empty bodies (priming nothing adds no value)', async () => { + const empty = mkSkill('empty', userOid, ''); + const result = await resolveManualSkills({ + names: ['empty'], + getSkillByName: buildGetSkillByName({ empty }), + accessibleSkillIds: [empty._id], + userId, + }); + expect(result).toEqual([]); + }); + + it('swallows getSkillByName errors per-name so one bad lookup does not drop the rest', async () => { + const good = mkSkill('good', userOid, 'good-body'); + const getSkillByName = async (name: string) => { + if (name === 'boom') { + throw new Error('db exploded'); + } + return name === 'good' ? good : null; + }; + const result = await resolveManualSkills({ + names: ['boom', 'good'], + getSkillByName, + accessibleSkillIds: [good._id], + userId, + }); + expect(result).toEqual([{ name: 'good', body: 'good-body' }]); + }); + + it('truncates manual skill lists above MAX_MANUAL_SKILLS to bound concurrent DB lookups', async () => { + const skills: Record = {}; + const names: string[] = []; + for (let i = 0; i < MAX_MANUAL_SKILLS + 5; i++) { + const name = `s${i}`; + skills[name] = mkSkill(name, userOid, `body-${i}`); + names.push(name); + } + const getSkillByName = jest.fn(buildGetSkillByName(skills)); + const result = await resolveManualSkills({ + names, + getSkillByName, + accessibleSkillIds: names.map((n) => skills[n]._id), + userId, + }); + expect(result).toHaveLength(MAX_MANUAL_SKILLS); + expect(getSkillByName).toHaveBeenCalledTimes(MAX_MANUAL_SKILLS); + // Preserves input order β€” drops the tail, keeps the head. + expect(result.map((r) => r.name)).toEqual(names.slice(0, MAX_MANUAL_SKILLS)); + }); + + it('applies the cap AFTER dedup so repeated names do not consume slots', async () => { + const skills: Record = {}; + const uniqueCount = MAX_MANUAL_SKILLS; + for (let i = 0; i < uniqueCount; i++) { + const name = `u${i}`; + skills[name] = mkSkill(name, userOid, `body-${i}`); + } + // Duplicate every name 3x β€” total length 3 Γ— MAX_MANUAL_SKILLS, unique = MAX_MANUAL_SKILLS. + const names: string[] = []; + for (let pass = 0; pass < 3; pass++) { + for (let i = 0; i < uniqueCount; i++) { + names.push(`u${i}`); + } + } + const result = await resolveManualSkills({ + names, + getSkillByName: buildGetSkillByName(skills), + accessibleSkillIds: Object.values(skills).map((s) => s._id), + userId, + }); + expect(result).toHaveLength(uniqueCount); + }); +}); + +describe('injectManualSkillPrimes', () => { + const prime = (name: string, body: string) => ({ name, body }); + + it('no-ops when manualSkillPrimes is empty', () => { + const messages = [new HumanMessage('hello')]; + const map = { 0: 10 }; + const result = injectManualSkillPrimes({ + initialMessages: messages, + indexTokenCountMap: map, + manualSkillPrimes: [], + }); + expect(result.inserted).toBe(0); + expect(result.insertIdx).toBe(-1); + expect(messages).toHaveLength(1); + expect(result.indexTokenCountMap).toBe(map); + }); + + it('no-ops when initialMessages is empty', () => { + const messages: HumanMessage[] = []; + const result = injectManualSkillPrimes({ + initialMessages: messages, + indexTokenCountMap: undefined, + manualSkillPrimes: [prime('foo', 'body')], + }); + expect(result.inserted).toBe(0); + expect(result.insertIdx).toBe(-1); + expect(messages).toHaveLength(0); + }); + + it('inserts a single prime right before the last message when there is only one message', () => { + const userMsg = new HumanMessage('What is X?'); + const messages = [userMsg]; + const result = injectManualSkillPrimes({ + initialMessages: messages, + indexTokenCountMap: { 0: 7 }, + manualSkillPrimes: [prime('x', 'X means...')], + }); + expect(result.inserted).toBe(1); + expect(result.insertIdx).toBe(0); + expect(messages).toHaveLength(2); + expect(messages[0].content).toBe('X means...'); + expect(messages[1]).toBe(userMsg); + // The prior-index-0 count follows the message it was attached to: now at idx 1. + expect(result.indexTokenCountMap).toEqual({ 1: 7 }); + }); + + it('inserts multiple primes in input order right before the last message', () => { + const messages: (HumanMessage | AIMessage)[] = [ + new HumanMessage('turn 1 user'), + new AIMessage('turn 1 reply'), + new HumanMessage('turn 2 user'), + ]; + const result = injectManualSkillPrimes({ + initialMessages: messages, + indexTokenCountMap: { 0: 1, 1: 2, 2: 3 }, + manualSkillPrimes: [prime('a', 'A body'), prime('b', 'B body')], + }); + expect(result.inserted).toBe(2); + expect(result.insertIdx).toBe(2); + expect(messages).toHaveLength(5); + expect(messages[0].content).toBe('turn 1 user'); + expect(messages[1].content).toBe('turn 1 reply'); + expect(messages[2].content).toBe('A body'); + expect(messages[3].content).toBe('B body'); + expect(messages[4].content).toBe('turn 2 user'); + // idx 0 unchanged, idx 1 unchanged, idx 2 shifts to 4 (+ numPrimes). + expect(result.indexTokenCountMap).toEqual({ 0: 1, 1: 2, 4: 3 }); + }); + + it('primes carry isMeta / source / skillName advisory markers in additional_kwargs', () => { + const messages = [new HumanMessage('hi')]; + injectManualSkillPrimes({ + initialMessages: messages, + indexTokenCountMap: undefined, + manualSkillPrimes: [prime('brand', 'brand-body')], + }); + const primed = messages[0] as HumanMessage; + expect(primed.additional_kwargs).toEqual({ + isMeta: true, + source: 'skill', + skillName: 'brand', + }); + }); + + it('uses `>=` when shifting the map so the message originally at insertIdx follows its count forward', () => { + // Regression guard: a `>` bug here would leave the last user message's + // token count attached to one of the new primes instead. + const messages = [new HumanMessage('older'), new HumanMessage('latest')]; + const result = injectManualSkillPrimes({ + initialMessages: messages, + indexTokenCountMap: { 0: 5, 1: 11 }, + manualSkillPrimes: [prime('x', 'x body')], + }); + // insertIdx = 1, numPrimes = 1. Entry at idx 1 must move to idx 2. + expect(result.indexTokenCountMap).toEqual({ 0: 5, 2: 11 }); + }); + + it('handles undefined indexTokenCountMap (formatAgentMessages omitted the map)', () => { + const messages = [new HumanMessage('hi')]; + const result = injectManualSkillPrimes({ + initialMessages: messages, + indexTokenCountMap: undefined, + manualSkillPrimes: [prime('x', 'body')], + }); + expect(result.inserted).toBe(1); + expect(result.indexTokenCountMap).toBeUndefined(); + }); +}); + +describe('extractManualSkills', () => { + it('returns undefined for null / non-object bodies', () => { + expect(extractManualSkills(null)).toBeUndefined(); + expect(extractManualSkills(undefined)).toBeUndefined(); + expect(extractManualSkills('string')).toBeUndefined(); + expect(extractManualSkills(42)).toBeUndefined(); + }); + + it('returns undefined when manualSkills is missing or not an array', () => { + expect(extractManualSkills({})).toBeUndefined(); + expect(extractManualSkills({ manualSkills: 'foo' })).toBeUndefined(); + expect(extractManualSkills({ manualSkills: { 0: 'foo' } })).toBeUndefined(); + }); + + it('passes through valid string[] input untouched', () => { + expect(extractManualSkills({ manualSkills: ['a', 'b'] })).toEqual(['a', 'b']); + }); + + it('filters out non-string elements that a TS-oblivious payload might include', () => { + expect( + extractManualSkills({ + manualSkills: ['ok', 123, null, { $gt: '' }, undefined, 'also-ok'], + }), + ).toEqual(['ok', 'also-ok']); + }); + + it('filters out empty strings', () => { + expect(extractManualSkills({ manualSkills: ['', 'real', ''] })).toEqual(['real']); + }); + + it('returns undefined when the array is present but contains no valid strings', () => { + expect(extractManualSkills({ manualSkills: [123, null, ''] })).toBeUndefined(); + expect(extractManualSkills({ manualSkills: [] })).toBeUndefined(); + }); +}); + +describe('isSkillPrimeMessage', () => { + it('returns true for a prime produced by injectManualSkillPrimes', () => { + const messages = [new HumanMessage('user turn')]; + injectManualSkillPrimes({ + initialMessages: messages, + indexTokenCountMap: undefined, + manualSkillPrimes: [{ name: 'x', body: 'x body' }], + }); + expect(isSkillPrimeMessage(messages[0])).toBe(true); + }); + + it('returns false for plain HumanMessage / AIMessage without the marker', () => { + expect(isSkillPrimeMessage(new HumanMessage('hi'))).toBe(false); + expect(isSkillPrimeMessage(new AIMessage('reply'))).toBe(false); + }); + + it('returns false for additional_kwargs with a different source', () => { + const m = new HumanMessage({ + content: 'x', + additional_kwargs: { source: 'not-a-skill' }, + }); + expect(isSkillPrimeMessage(m)).toBe(false); + }); + + it('returns false for non-object / null / undefined inputs', () => { + expect(isSkillPrimeMessage(null)).toBe(false); + expect(isSkillPrimeMessage(undefined)).toBe(false); + expect(isSkillPrimeMessage('just a string')).toBe(false); + expect(isSkillPrimeMessage(42)).toBe(false); + }); + + it('filter() on a mixed array keeps the non-prime messages', () => { + const user = new HumanMessage('real turn'); + const messages: (HumanMessage | AIMessage)[] = [new AIMessage('reply'), user]; + injectManualSkillPrimes({ + initialMessages: messages, + indexTokenCountMap: undefined, + manualSkillPrimes: [{ name: 'x', body: 'x body' }], + }); + // After priming: [AIMessage, primeHumanMessage, user] + const stripped = messages.filter((m) => !isSkillPrimeMessage(m)); + expect(stripped).toHaveLength(2); + expect(stripped[1]).toBe(user); + }); +}); + +describe('buildSkillPrimeContentParts', () => { + it('produces one completed tool_call content part per prime', () => { + const parts = buildSkillPrimeContentParts( + [ + { name: 'brand', body: '# Brand' }, + { name: 'legal', body: '# Legal' }, + ], + { runId: 'msg_abc' }, + ); + expect(parts).toHaveLength(2); + expect(parts[0].type).toBe('tool_call'); + expect(parts[0].tool_call.name).toBe('skill'); + expect(parts[0].tool_call.progress).toBe(1); + expect(parts[0].tool_call.type).toBe('tool_call'); + }); + + it('encodes skillName as a JSON string in args (matches the SkillCall renderer contract)', () => { + const [part] = buildSkillPrimeContentParts([{ name: 'pdf-reader', body: '...' }], { + runId: 'msg_1', + }); + expect(JSON.parse(part.tool_call.args)).toEqual({ skillName: 'pdf-reader' }); + }); + + it('produces a human-readable output string the frontend can expand', () => { + const [part] = buildSkillPrimeContentParts([{ name: 'research', body: '...' }], { + runId: 'msg_1', + }); + expect(part.tool_call.output).toContain('research'); + expect(part.tool_call.output).toContain('loaded'); + }); + + it('assigns unique tool_call IDs seeded from runId + index', () => { + const parts = buildSkillPrimeContentParts( + [ + { name: 'a', body: 'a' }, + { name: 'b', body: 'b' }, + ], + { runId: 'msg_xyz' }, + ); + const ids = parts.map((p) => p.tool_call.id); + expect(new Set(ids).size).toBe(2); + expect(ids[0]).toContain('msg_xyz'); + expect(ids[1]).toContain('msg_xyz'); + }); + + it('returns an empty array for empty input', () => { + expect(buildSkillPrimeContentParts([], { runId: 'msg_1' })).toEqual([]); + }); + + it('respects startOffset when seeding IDs (callers prepending to a populated list)', () => { + const first = buildSkillPrimeContentParts([{ name: 'x', body: 'x' }], { runId: 'msg_1' }); + const second = buildSkillPrimeContentParts([{ name: 'x', body: 'x' }], { + runId: 'msg_1', + startOffset: 5, + }); + expect(first[0].tool_call.id).not.toBe(second[0].tool_call.id); + }); +}); diff --git a/packages/api/src/agents/handlers.ts b/packages/api/src/agents/handlers.ts index 8ec138ffe4..778540d384 100644 --- a/packages/api/src/agents/handlers.ts +++ b/packages/api/src/agents/handlers.ts @@ -14,6 +14,7 @@ import type { StructuredToolInterface } from '@langchain/core/tools'; import type { ServerRequest } from '~/types'; import { primeSkillFiles } from './skillFiles'; import type { SkillFileRecord } from './skillFiles'; +import { buildSkillPrimeMessage } from './skills'; import { runOutsideTracing } from '~/utils'; export interface ToolEndCallbackData { @@ -437,9 +438,7 @@ async function handleSkillToolCall( body = body.replace(/\$ARGUMENTS/g, args.args); } - const injectedMessages: InjectedMessage[] = [ - { role: 'user', content: body, isMeta: true, source: 'skill', skillName: skill.name }, - ]; + const injectedMessages: InjectedMessage[] = [buildSkillPrimeMessage({ name: skill.name, body })]; const contentText = `Skill "${args.skillName}" loaded. Follow the instructions below.`; let artifact: diff --git a/packages/api/src/agents/initialize.ts b/packages/api/src/agents/initialize.ts index 4e887caecb..f94eb6c012 100644 --- a/packages/api/src/agents/initialize.ts +++ b/packages/api/src/agents/initialize.ts @@ -30,7 +30,8 @@ import { import { filterFilesByEndpointConfig } from '~/files'; import { generateArtifactsPrompt } from '~/prompts'; import { getProviderConfig } from '~/endpoints'; -import { injectSkillCatalog } from './skills'; +import { injectSkillCatalog, resolveManualSkills } from './skills'; +import type { ResolvedManualSkill } from './skills'; import { primeResources } from './resources'; import type { TFilterFilesByAgentAccess } from './resources'; @@ -71,6 +72,13 @@ export type InitializedAgent = Agent & { accessibleSkillIds?: import('mongoose').Types.ObjectId[]; /** Number of skills in the catalog (used to determine if SkillTool should be registered) */ skillCount?: number; + /** + * Skills the user manually invoked for this turn via the `$` popover, resolved + * to their SKILL.md bodies. The AgentClient injects these as meta user + * messages right before the latest user message in the LLM's formatted + * message array β€” deterministic priming without a tool roundtrip. + */ + manualSkillPrimes?: ResolvedManualSkill[]; }; export const DEFAULT_MAX_CONTEXT_TOKENS = 32000; @@ -127,6 +135,13 @@ export interface InitializeAgentParams { skillStates?: Record; /** Admin-configured default for shared skills (`true` = shared skills auto-activate). */ defaultActiveOnShare?: boolean; + /** + * Skill names the user invoked manually for this turn via the `$` popover. + * Resolved here (ACL + active-state filtered) and attached to the returned + * InitializedAgent as `manualSkillPrimes` for the AgentClient to inject as + * meta user messages before the LLM call. + */ + manualSkills?: string[]; } /** @@ -173,6 +188,20 @@ export interface InitializeAgentDbMethods extends EndpointDbMethods { has_more?: boolean; after?: string | null; }>; + /** + * Load a single skill by name, constrained to an ACL-accessible ID set. + * Returns the full document (including `body`) so manual invocation can + * prime SKILL.md without a second DB round-trip. + */ + getSkillByName?: ( + name: string, + accessibleIds: import('mongoose').Types.ObjectId[], + ) => Promise<{ + _id: import('mongoose').Types.ObjectId; + name: string; + body: string; + author: import('mongoose').Types.ObjectId; + } | null>; } /** @@ -454,6 +483,7 @@ export async function initializeAgent( * LLM (or a direct-invocation path) names one. */ let executableSkillIds = params.accessibleSkillIds; + let manualSkillPrimes: ResolvedManualSkill[] | undefined; const { accessibleSkillIds } = params; if (accessibleSkillIds && accessibleSkillIds.length > 0) { const skillResult = await injectSkillCatalog({ @@ -471,6 +501,25 @@ export async function initializeAgent( toolDefinitions = skillResult.toolDefinitions; skillCount = skillResult.skillCount; executableSkillIds = skillResult.activeSkillIds; + + /** + * Resolve manually-invoked skills against the same ACL set used for the + * catalog. We use `accessibleSkillIds` (not `activeSkillIds`) because the + * catalog is capped at a token budget β€” a skill that fell outside that cap + * can still be authorized for direct manual invocation. The active-state + * filter is re-applied inside `resolveManualSkills` so deactivated skills + * never leak through this path either. + */ + if (params.manualSkills?.length && db.getSkillByName) { + manualSkillPrimes = await resolveManualSkills({ + names: params.manualSkills, + getSkillByName: db.getSkillByName, + accessibleSkillIds, + userId: req.user?.id, + skillStates: params.skillStates, + defaultActiveOnShare: params.defaultActiveOnShare, + }); + } } const agentMaxContextNum = Number(agentMaxContextTokens) || DEFAULT_MAX_CONTEXT_TOKENS; @@ -506,6 +555,7 @@ export async function initializeAgent( baseContextTokens, skillCount, accessibleSkillIds: executableSkillIds, + manualSkillPrimes, attachments: finalAttachments, toolContextMap: toolContextMap ?? {}, useLegacyContent: !!options.useLegacyContent, diff --git a/packages/api/src/agents/skills.ts b/packages/api/src/agents/skills.ts index 5227875c02..6d9eafbff3 100644 --- a/packages/api/src/agents/skills.ts +++ b/packages/api/src/agents/skills.ts @@ -1,13 +1,15 @@ +import { logger } from '@librechat/data-schemas'; +import { HumanMessage } from '@langchain/core/messages'; import { formatSkillCatalog, SkillToolDefinition, ReadFileToolDefinition, BashExecutionToolDefinition, } from '@librechat/agents'; -import type { LCToolRegistry, LCTool } from '@librechat/agents'; -import { logger } from '@librechat/data-schemas'; -import type { Types } from 'mongoose'; +import type { LCToolRegistry, LCTool, InjectedMessage } from '@librechat/agents'; +import type { BaseMessage } from '@langchain/core/messages'; import type { Agent } from 'librechat-data-provider'; +import type { Types } from 'mongoose'; import type { InitializeAgentDbMethods } from './initialize'; const SKILL_CATALOG_LIMIT = 100; @@ -15,6 +17,45 @@ const SKILL_CATALOG_LIMIT = 100; const MAX_CATALOG_PAGES = 10; /** Page size used when paginating to fill the active-skill quota. */ const CATALOG_PAGE_SIZE = 100; +/** + * Hard ceiling on skill names resolved per request via `$` popover or + * `always-apply`. The popover realistically surfaces only a few per turn; + * the cap is a defense-in-depth against a crafted payload fanning out into + * many concurrent `getSkillByName` DB lookups. + */ +export const MAX_MANUAL_SKILLS = 10; + +/** + * Hard ceiling on individual skill name length. Real skill names are + * short slugs (e.g. `pptx`, `brand-guidelines`); anything beyond this is + * a crafted payload. Filtered out before the DB round-trip so pathological + * strings can't reach `getSkillByName` / Mongo's query planner. + */ +export const MAX_SKILL_NAME_LENGTH = 200; + +/** + * Marker tagged onto every skill-primed message (as `additional_kwargs.source` + * on a LangChain `HumanMessage`, or as `source` on the `InjectedMessage` that + * `handleSkillToolCall` emits). Downstream filtering/telemetry keys off this, + * so both construction paths must agree β€” keep the literal exported from one + * place rather than repeated inline. + */ +export const SKILL_MESSAGE_SOURCE = 'skill'; + +/** + * Predicate that identifies a LangChain message as one we spliced in via + * `injectManualSkillPrimes` (or the equivalent model-invoked path). Callers + * like `runMemory` use this to strip synthetic skill content from windows + * that should only contain real user chat β€” without this filter, SKILL.md + * bodies pollute memory extraction and crowd out genuine turns. + */ +export function isSkillPrimeMessage(msg: unknown): boolean { + if (!msg || typeof msg !== 'object') { + return false; + } + const kwargs = (msg as { additional_kwargs?: { source?: unknown } }).additional_kwargs; + return !!kwargs && kwargs.source === SKILL_MESSAGE_SOURCE; +} /** * Scopes user-accessible skill IDs to only those configured on the agent. @@ -240,3 +281,322 @@ export async function injectSkillCatalog( activeSkillIds: activeSkills.map((s) => s._id), }; } + +/** + * Builds the meta user message that carries a skill's SKILL.md body into a + * turn's context. Shape mirrors what `handleSkillToolCall` emits when the + * model invokes the skill tool directly, so downstream message handling + * treats all three paths identically. + * + * Used by: + * - Phase 3 manual invocation (`$skill-name` popover) β€” called at turn start. + * - Phase 5 `always-apply` frontmatter β€” called at turn start. + * - `handleSkillToolCall` for model-invoked skills β€” called from the tool + * execution handler. + */ +export function buildSkillPrimeMessage(skill: { name: string; body: string }): InjectedMessage { + return { + role: 'user', + content: skill.body, + isMeta: true, + source: SKILL_MESSAGE_SOURCE, + skillName: skill.name, + }; +} + +export interface ResolveManualSkillsParams { + /** Skill names the user invoked (via `$` popover or `always-apply`). */ + names: string[]; + /** DB lookup: name β†’ skill doc, constrained to ACL-accessible IDs. */ + getSkillByName: ( + name: string, + accessibleIds: Types.ObjectId[], + ) => Promise<{ + _id: Types.ObjectId; + name: string; + body: string; + author: Types.ObjectId | string; + } | null>; + /** ACL-accessible skill IDs for this user (already scoped by `scopeSkillIds`). */ + accessibleSkillIds: Types.ObjectId[]; + /** Current user ID β€” required for ownership-based active-state defaults. */ + userId?: string; + /** Per-user skill active/inactive overrides. */ + skillStates?: Record; + /** Admin-configured default for shared skills. */ + defaultActiveOnShare?: boolean; +} + +export interface ResolvedManualSkill { + name: string; + body: string; +} + +/** + * Resolves user-provided skill names to `{ name, body }` pairs ready for + * priming. Filters out: + * - names not backed by an accessible skill (ACL miss or typo), + * - skills the user has toggled inactive (respects ownership-aware defaults). + * + * The active-state filter is intentional even for explicit `$` selections: + * 1. Phase 2 closes the loop on the UI side by hiding inactive skills from + * the popover, so a deactivated skill shouldn't be reachable through the + * normal flow in the first place. + * 2. For API-direct callers bypassing the popover, honoring the user's + * own activation toggle is the safer default β€” an opt-out toggle that + * stops working the moment someone crafts a raw payload isn't much of + * a toggle. + * + * Silently skips unresolvable names with a warn log β€” a missing `$skill` must + * never block the user's actual message from going through. + * + * Preserves input order and drops duplicate names (first wins) so a user who + * types `$foo $foo` doesn't end up with the body primed twice. + */ +export async function resolveManualSkills( + params: ResolveManualSkillsParams, +): Promise { + const { names, getSkillByName, accessibleSkillIds, userId, skillStates, defaultActiveOnShare } = + params; + + if (!names.length || accessibleSkillIds.length === 0) { + return []; + } + + const seen = new Set(); + const uniqueNames = names.filter((n) => { + if (!n || seen.has(n)) { + return false; + } + seen.add(n); + return true; + }); + + /** + * Truncate after dedup so a user repeating the same name doesn't consume + * cap slots. Kept inside the function (not at the controller) so every + * caller β€” including future internal ones β€” inherits the protection. + */ + let boundedNames = uniqueNames; + if (uniqueNames.length > MAX_MANUAL_SKILLS) { + const droppedAll = uniqueNames.slice(MAX_MANUAL_SKILLS); + const DROPPED_LOG_SAMPLE = 5; + const droppedSample = droppedAll.slice(0, DROPPED_LOG_SAMPLE).join(', '); + const droppedSuffix = + droppedAll.length > DROPPED_LOG_SAMPLE + ? `, ... (${droppedAll.length - DROPPED_LOG_SAMPLE} more)` + : ''; + logger.warn( + `[resolveManualSkills] Truncating manual skill list from ${uniqueNames.length} to ${MAX_MANUAL_SKILLS}: dropped [${droppedSample}${droppedSuffix}]`, + ); + boundedNames = uniqueNames.slice(0, MAX_MANUAL_SKILLS); + } + + const resolved = await Promise.all( + boundedNames.map(async (name) => { + try { + const skill = await getSkillByName(name, accessibleSkillIds); + if (!skill) { + logger.warn(`[resolveManualSkills] Skill "${name}" not found or not accessible`); + return null; + } + if (!skill.body) { + logger.warn(`[resolveManualSkills] Skill "${name}" has empty body β€” skipping`); + return null; + } + const active = resolveSkillActive({ + skill: { _id: skill._id, author: skill.author }, + skillStates, + userId, + defaultActiveOnShare, + }); + if (!active) { + logger.warn(`[resolveManualSkills] Skill "${name}" is inactive for this user β€” skipping`); + return null; + } + return { name: skill.name, body: skill.body }; + } catch (err) { + logger.warn( + `[resolveManualSkills] Failed to resolve skill "${name}":`, + err instanceof Error ? err.message : err, + ); + return null; + } + }), + ); + + return resolved.filter((r): r is ResolvedManualSkill => r !== null); +} + +export interface InjectManualSkillPrimesParams { + /** Formatted LangChain messages produced by `formatAgentMessages`. Mutated in place. */ + initialMessages: BaseMessage[]; + /** Per-index token count map returned by `formatAgentMessages`. */ + indexTokenCountMap: Record | undefined; + /** Resolved skill primes to splice in. */ + manualSkillPrimes: ResolvedManualSkill[]; +} + +export interface InjectManualSkillPrimesResult { + /** Same array reference as input (mutated). */ + initialMessages: BaseMessage[]; + /** New token map with indices shifted; same reference as input when no-op. */ + indexTokenCountMap: Record | undefined; + /** Number of prime messages actually inserted (for logging by the caller). */ + inserted: number; + /** Position where primes were inserted, or -1 when no-op. */ + insertIdx: number; +} + +/** + * Splices skill prime messages into a formatted message array just before + * the latest user message, and shifts the `indexTokenCountMap` so downstream + * `hydrateMissingIndexTokenCounts` fills counts for the new positions + * without corrupting accounting for pre-existing ones. + * + * Insertion semantics: + * - `insertIdx = initialMessages.length - 1`. Empty input β†’ no-op. + * - Single-message input β†’ `insertIdx = 0`, primes appear before the lone message. + * - Map shift: entries with `idx >= insertIdx` move forward by `numPrimes`. + * Using `>=` (not `>`) is load-bearing: the message that was at `insertIdx` + * is pushed to `insertIdx + numPrimes` by the splice, so its count must + * follow it. + * + * Callers are responsible for scoping (e.g. single-agent runs only β€” see + * `AgentClient.chatCompletion`). This helper is agent-agnostic. + */ +export function injectManualSkillPrimes( + params: InjectManualSkillPrimesParams, +): InjectManualSkillPrimesResult { + const { initialMessages, manualSkillPrimes } = params; + let { indexTokenCountMap } = params; + + if (manualSkillPrimes.length === 0 || initialMessages.length === 0) { + return { initialMessages, indexTokenCountMap, inserted: 0, insertIdx: -1 }; + } + + const insertIdx = initialMessages.length - 1; + const numPrimes = manualSkillPrimes.length; + + if (indexTokenCountMap) { + const shifted: Record = {}; + for (const [idxStr, count] of Object.entries(indexTokenCountMap)) { + const idx = Number(idxStr); + shifted[idx >= insertIdx ? idx + numPrimes : idx] = count; + } + indexTokenCountMap = shifted; + } + + const primeMessages = manualSkillPrimes.map( + (p) => + new HumanMessage({ + content: p.body, + additional_kwargs: { isMeta: true, source: SKILL_MESSAGE_SOURCE, skillName: p.name }, + }), + ); + initialMessages.splice(insertIdx, 0, ...primeMessages); + + return { initialMessages, indexTokenCountMap, inserted: numPrimes, insertIdx }; +} + +export interface SkillPrimeContentPart { + type: 'tool_call'; + tool_call: { + id: string; + name: string; + args: string; + output: string; + progress: number; + type: 'tool_call'; + }; +} + +export interface BuildSkillPrimeContentPartsParams { + /** Run / response message ID. Used as a stable seed for synthetic tool_call IDs. */ + runId: string; + /** + * Optional index offset. When the primes are prepended to an existing + * contentParts array, callers can leave this 0 (IDs still stay unique + * within the message). Exposed for explicitness only. + */ + startOffset?: number; +} + +/** + * Build completed tool_call content parts for each manually-invoked skill. + * + * Matches the shape the aggregator produces for a model-invoked skill call + * on `ON_RUN_STEP_COMPLETED` β€” `{ type: 'tool_call', tool_call: { id, name, + * args, output, progress: 1, type: 'tool_call' } }` β€” so the existing + * `SkillCall` renderer on the frontend shows identical "Skill X loaded" + * cards without any new client work. + * + * Callers (e.g. `AgentClient.chatCompletion`) prepend these to + * `this.contentParts` after `runAgents` returns so the cards persist on + * the response message and appear ahead of the model's content β€” matching + * the semantic that priming ran before the LLM turn. + * + * ────────────────────────────────────────────────────────────────────── + * Intentional: sticky re-priming across turns. + * ────────────────────────────────────────────────────────────────────── + * Shape parity with model-invoked tool_calls means `extractInvokedSkills + * FromPayload` (packages/api/src/agents/run.ts) treats these as ordinary + * skill invocations when scanning history, so `primeInvokedSkills` re- + * prepares the files + body on every subsequent turn for the rest of + * the conversation. This is deliberate: the skill-card UX on the assistant + * message is the user's persistent visual cue that the skill is active, + * and re-priming keeps the LLM's view consistent with what the user sees + * (and matches how model-invoked skills behave β€” there is no separate + * "one-shot vs. conversation-scoped" mode). To opt out of a sticky skill, + * users need to start a new conversation or edit the originating user + * message to remove the pills and resubmit β€” regenerate alone preserves + * the picks (`useChatFunctions.regenerate` forwards them via + * `overrideManualSkills`). A future "one-shot manual prime" mode would + * need a distinct marker on the synthetic tool_call so the history + * scanner could skip it. + */ +export function buildSkillPrimeContentParts( + primes: ResolvedManualSkill[], + { runId, startOffset = 0 }: BuildSkillPrimeContentPartsParams, +): SkillPrimeContentPart[] { + return primes.map((prime, i) => { + const args = JSON.stringify({ skillName: prime.name }); + return { + type: 'tool_call', + tool_call: { + id: `call_manual_skill_${runId}_${startOffset + i}`, + name: SkillToolDefinition.name, + args, + output: `Skill "${prime.name}" loaded. Follow the instructions below.`, + progress: 1, + type: 'tool_call', + }, + }; + }); +} + +/** + * Safely pulls `manualSkills` from an untyped request body. + * + * Accepts only string[] in practice: filters out non-string elements, + * empty strings, and pathologically long names so a crafted payload + * (numbers, objects, nulls, giga-strings) can't reach `getSkillByName` + * and waste DB round-trips, matching the TypeScript contract + * (`manualSkills?: string[]` on TPayload). Returns `undefined` for + * missing / non-array inputs OR for arrays that contain no valid strings β€” + * callers treat undefined as "no manual invocation this turn." + */ +export function extractManualSkills(body: unknown): string[] | undefined { + if (!body || typeof body !== 'object') { + return undefined; + } + const raw = (body as { manualSkills?: unknown }).manualSkills; + if (!Array.isArray(raw)) { + return undefined; + } + const filtered = raw.filter( + (entry): entry is string => + typeof entry === 'string' && entry.length > 0 && entry.length <= MAX_SKILL_NAME_LENGTH, + ); + return filtered.length > 0 ? filtered : undefined; +} diff --git a/packages/data-provider/src/createPayload.ts b/packages/data-provider/src/createPayload.ts index 3056a7021b..783ad7f9dd 100644 --- a/packages/data-provider/src/createPayload.ts +++ b/packages/data-provider/src/createPayload.ts @@ -14,6 +14,7 @@ export default function createPayload(submission: t.TSubmission) { editedContent, ephemeralAgent, endpointOption, + manualSkills, } = submission; const { conversationId } = s.tConvoUpdateSchema.parse(conversation); const { endpoint: _e, endpointType } = endpointOption as { @@ -40,6 +41,7 @@ export default function createPayload(submission: t.TSubmission) { conversationId, isContinued: !!(isEdited && isContinued), ephemeralAgent: s.isAssistantsEndpoint(endpoint) ? undefined : ephemeralAgent, + manualSkills: s.isAssistantsEndpoint(endpoint) ? undefined : manualSkills, }; return { server, payload }; diff --git a/packages/data-provider/src/schemas.ts b/packages/data-provider/src/schemas.ts index 5b840801d4..0eb64fa71b 100644 --- a/packages/data-provider/src/schemas.ts +++ b/packages/data-provider/src/schemas.ts @@ -680,6 +680,15 @@ export const tMessageSchema = z.object({ ), }) .optional(), + /** + * Skill names the user invoked manually via the `$` popover on this turn. + * Purely UI metadata β€” `ManualSkillPills` renders these above the message + * bubble so users can see which skills they asked for in history and on + * reload. Runtime resolution uses the top-level payload field with the + * same name. Empty / absent for model-invoked skills (shown as tool_call + * content parts on the assistant message instead). + */ + manualSkills: z.array(z.string()).optional(), }); export type MemoryArtifact = { diff --git a/packages/data-provider/src/types.ts b/packages/data-provider/src/types.ts index b5f032cd79..a142f720a2 100644 --- a/packages/data-provider/src/types.ts +++ b/packages/data-provider/src/types.ts @@ -116,6 +116,13 @@ export type TPayload = Partial & editedContent?: TEditedContent | null; /** Added conversation for multi-convo feature */ addedConvo?: TConversation; + /** + * Skills the user selected via the `$` popover for this turn. Names, not IDs + * β€” the backend resolves them against the user's ACL-accessible skill set, + * loads each SKILL.md body, and prepends one meta user message per skill + * before the LLM turn runs. + */ + manualSkills?: string[]; }; export type TEditedContent = @@ -145,6 +152,8 @@ export type TSubmission = { editedContent?: TEditedContent | null; /** Added conversation for multi-convo feature */ addedConvo?: TConversation; + /** Skills the user invoked via the `$` popover for this submission. */ + manualSkills?: string[]; }; export type EventSubmission = Omit & { initialResponse: TMessage }; diff --git a/packages/data-schemas/src/schema/message.ts b/packages/data-schemas/src/schema/message.ts index 9879efae55..aeba714b89 100644 --- a/packages/data-schemas/src/schema/message.ts +++ b/packages/data-schemas/src/schema/message.ts @@ -122,6 +122,14 @@ const messageSchema: Schema = new Schema( default: undefined, }, attachments: { type: [{ type: mongoose.Schema.Types.Mixed }], default: undefined }, + /** + * Skill names the user invoked manually via the `$` popover on this turn. + * UI metadata only β€” `ManualSkillPills` on the frontend renders these on + * the user message bubble so the selection persists through reload and + * shows in history. Runtime skill resolution lives separately on the + * request body, not on the message itself. + */ + manualSkills: { type: [String], default: undefined }, /* attachments: { type: [ diff --git a/packages/data-schemas/src/types/message.ts b/packages/data-schemas/src/types/message.ts index 201e5650ef..7d607d61d9 100644 --- a/packages/data-schemas/src/types/message.ts +++ b/packages/data-schemas/src/types/message.ts @@ -44,6 +44,8 @@ export interface IMessage extends Document { encoding?: string; }; attachments?: unknown[]; + /** Skills the user invoked manually via the `$` popover on this turn. UI-only metadata for `ManualSkillPills`. */ + manualSkills?: string[]; expiredAt?: Date | null; createdAt?: Date; updatedAt?: Date;