2234 commits

Author	SHA1	Message	Date
Danny Avila	91cd3f7b7c	🧽 refactor: Skills polish: precedence-aware body validation, controller drop logs, SkillPills rename (#12760) ... Post-merge sanity-review cleanup on top of #12746: - `createSkill` / `updateSkill` now parse SKILL.md body's always-apply status once and reuse it for both validation and derivation (was parsing the same YAML block twice per call). - Body-inline `always-apply:` validation becomes precedence-aware: a caller sending an explicit top-level `alwaysApply` or a structured `frontmatter['always-apply']` no longer gets rejected for a typo in the body — the body value is never consulted at derivation time when a higher-precedence source wins. New tests cover the three relevant interactions (explicit+body-typo, frontmatter+body-typo, body-only typo still rejects). - OpenAI and Responses controllers now emit a `logger.warn` when `injectSkillPrimes` drops always-apply primes to stay under `MAX_PRIMED_SKILLS_PER_TURN`. `injectSkillPrimes` already logs internally; the controller-level warn adds endpoint context so operators can identify which path hit the cap at a glance. Mirrors AgentClient's existing log. - Rename `ManualSkillPills` → `SkillPills` (component + type + file + test + all JSDoc references). The component handles both manual and always-apply pills now; the original name was carried over from the manual-only Phase 3 and misleads new readers. - Drive-by fix: declare `appConfig = req.config` at the top of `createResponse` in `responses.js` — it was used unqualified on lines 381/396, which silently evaluated to `undefined` (via optional chaining) and disabled the skills-capability check on the Responses endpoint. Pre-existing, surfaced by lint on the touched file.	2026-04-25 04:02:01 -04:00
Danny Avila	dfc3dfa57f	📍 feat: always-apply frontmatter: auto-prime skills every turn (#12746) ... * 🔁 refactor: Rebase always-apply work onto merged structured-frontmatter columns Phase 6 (disable-model-invocation / user-invocable / allowed-tools) landed first on feat/agent-skills. Reconcile this branch with the new mainline: - Thread alwaysApplySkillPrimes through unionPrimeAllowedTools alongside manualSkillPrimes, applying the combined MAX_PRIMED_SKILLS_PER_TURN ceiling before loading tools. - Add `_id` to ResolvedAlwaysApplySkill to match Phase 6's ResolvedManualSkill shape (read_file name-collision protection). - Register 'always-apply' in ALLOWED_FRONTMATTER_KEYS / FRONTMATTER_KIND so Phase 6's validator recognizes it. - Drop frontmatter from the listSkillsByAccess projection; the backfill helper remains as defensive code but its read path is no longer exercised on summary rows (no legacy rows exist — the branch never shipped), saving ~200KB per page. - Retire the corresponding "backfills legacy on summaries" test. - Plumb listAlwaysApplySkills through the JS controllers + endpoint initializer so the always-apply resolver sees a real DB method. * 🧹 fix: Dedupe manual/always-apply overlap, share YAML util, tidy comments Addresses review findings: - Cross-list dedup: when a user $-invokes a skill that is also marked always-apply, the always-apply copy is now dropped so the same SKILL.md body never primes twice in one turn. Manual wins (explicit intent, closer to the user message). Dedup runs in both initializeAgent (so persisted user-bubble pills stay in sync) and injectSkillPrimes (defense-in-depth at splice time). New test cases cover single-overlap, partial-overlap, and dedup-before-cap. - DRY: extract stripYamlTrailingComment to packages/data-schemas/src/utils/yaml.ts; packages/api/src/skills/import.ts now imports the shared helper. Also drop the redundant inner stripYamlTrailingComment call inside parseBooleanScalar — the call site already strips. - Mark injectManualSkillPrimes as @deprecated in favor of injectSkillPrimes (kept for external consumers of @librechat/api). - Document SKILL_TRIGGER_MODEL as forward-looking plumbing for the model-invoked path rather than leaving it as a bare unused export. - Replace the stale "frontmatter is included" comment on listSkillsByAccess with an accurate explanation of why it was intentionally excluded. * 🔒 fix: Include always-apply primes in skillPrimedIdsByName + clear alwaysApply on body opt-out Two bugs flagged by Codex review: P1 (read_file): `manualSkillPrimedIdsByName` only carried manual-invocation primes, so an always-apply skill with `disable-model-invocation: true` was blocked from reading its own bundled files, and same-name collisions could resolve to a different doc than the one whose body got primed. - Rename `buildManualSkillPrimedIdsByName` → `buildSkillPrimedIdsByName` (accepts both manual + always-apply prime arrays). - Rename the configurable field `manualSkillPrimedIdsByName` → `skillPrimedIdsByName` throughout the plumbing (skillConfigurable.ts, handlers.ts, CJS callers, tests). - Overlap resolution: manual wins on the rare edge case where the same name appears in both arrays (upstream dedup should prevent this, but defensive merging treats manual as authoritative). - New tests: (1) gate-relaxation fires for always-apply primes, (2) `_id` pinning works for always-apply same-name collisions. P2 (updateSkill): when a body update had no `always-apply:` key, `extractAlwaysApplyFromBody` returned `absent` and the column was left untouched. A skill that was once `alwaysApply: true` would keep auto-priming even after its SKILL.md no longer declared the flag. - Treat `absent` as a positive "not always-apply" declaration when the body is explicitly submitted; flip the column to `false`. - Explicit top-level `alwaysApply` still wins (three-source precedence unchanged). - New tests: body removes key → false, body has no frontmatter at all → false, explicit + body-without-key → explicit wins. * 🧵 refactor: Collapse duplicate prime types + tighten parse + test hygiene Sanity-check review follow-ups: - Collapse `ResolvedManualSkill` / `ResolvedAlwaysApplySkill` into a single `ResolvedSkillPrime` canonical interface with two backward- compatible type aliases. Both resolvers feed the same pipeline stages (injectSkillPrimes, unionPrimeAllowedTools, buildSkillPrimedIdsByName); the per-source distinction lives on `additional_kwargs.trigger`, not on the resolver output. - Move the `always-apply` branch in `parseFrontmatter` to operate on the raw post-colon text. The outer `unquoteYaml` was fine today because it's idempotent on non-quoted strings, but running it twice (once per line, once after stripping the inline comment) would be fragile if the unquoter ever grows richer YAML-escape handling. - Add the missing `alwaysApplyDedupedFromManual: 0` field to the `injectSkillPrimes` mocks in `openai.spec.js` and `responses.unit.spec.js` so they match the full `InjectSkillPrimesResult` contract. - Insert the blank line between the `unionPrimeAllowedTools` and `resolveAlwaysApplySkills` describe blocks. * 🔧 fix(tsc): Cast mock.calls via `unknown` for strict tuple destructure `getSkillByName.mock.calls[0]` is typed as `[]` by jest's generic default; a direct cast to `[string, ..., ...]` fails TS2352 under `--noEmit` even though the runtime shape matches. Go through `as unknown as [...]` like the earlier test in the same file so CI's type-check step stays green. * 🪢 fix: Propagate skillPrimedIdsByName into handoff agent tool context Handoff agents go through the same `initializeAgent` flow as the primary (with `listAlwaysApplySkills` now plumbed), so they resolve their own `manualSkillPrimes` and `alwaysApplySkillPrimes` — but the `agentToolContexts.set(...)` for handoff agents didn't carry `skillPrimedIdsByName` into the per-agent context. That meant `handleReadFileCall` fell back to the full ACL set + a `prefer*` flag for handoff agents: same-name collisions could resolve to a different doc than the one whose body got primed, and a `disable-model-invocation: true` skill primed via manual `$` or always-apply inside the handoff flow would be blocked from reading its own bundled files. Build the map via `buildSkillPrimedIdsByName(config.manualSkillPrimes, config.alwaysApplySkillPrimes)` for every handoff tool context so `read_file` behaves identically across primary and handoff agents.	2026-04-25 04:02:00 -04:00
Danny Avila	82173f7b91	🛡️ feat: Persist & enforce disable-model-invocation / user-invocable / allowed-tools (#12745) ... * 🧬 feat: Persist `disable-model-invocation` / `user-invocable` / `allowed-tools` Adds first-class columns mirroring the three runtime-enforced frontmatter fields, with a `deriveStructuredFrontmatterFields` helper that maps from frontmatter at create/update time and re-syncs (via `$unset`) when fields are removed. `listSkillsByAccess` projection includes them so the Phase 6 catalog filter and popover filter can both read off the summary row. Marks `invocationMode` as @deprecated on `TSkill` and the `InvocationMode` enum — the runtime now reads the persisted pair instead. * 🛡️ feat: Enforce frontmatter at runtime (catalog, skill tool, manual resolver, tool union) Wires the persisted columns into actual runtime behavior across all four invocation paths: - `injectSkillCatalog` excludes `disableModelInvocation: true` skills before catalog formatting — they cost zero context tokens and stay invisible to the model. - `handleSkillToolCall` rejects with a clear error when the model names a skill marked `disable-model-invocation: true` (defends against a stale-cache or hallucinated invocation getting past the catalog filter). - `resolveManualSkills` skips `userInvocable: false` skills with a warn log so an API-direct caller can't bypass the popover-side filter. - `unionPrimeAllowedTools` collects skill-declared `allowed-tools` minus what's already on the agent; `initialize.ts` re-runs `loadTools` for the extras and merges resulting `toolDefinitions` into the agent's effective set for the turn. Tool-name resolution is tolerant — unknown names silently drop with a debug log so cross-ecosystem skills referencing yet-to-be-implemented tools (Claude Code's `edit_file`, etc.) import without breaking. The agent document is never modified; the union is turn-scoped. Helper exports (`unionPrimeAllowedTools`) are structured so Phase 5's always-apply primes flow through the same union (combined `[...manualPrimes, ...alwaysApplyPrimes]`) once the resolver lands. Skill handler wire format gains the three fields so clients can render them on detail / list views. * 🎛️ feat: `$` popover reads `userInvocable` instead of UI-only `invocationMode` Replaces the phase-1 UI-only `invocationMode` check with the persisted `userInvocable` field (mirrors the `user-invocable` frontmatter). Skills authored with `user-invocable: false` no longer surface in the popover; the backend resolver enforces the same rule for defense-in-depth. Default-visible behavior is preserved: skills without an explicit `userInvocable` value (older rows, freshly imported skills that don't declare the field) stay visible — only an explicit `false` hides them. Test fixture updated to reflect the new field. * 🔧 fix: Address Phase 6 review findings Codex P2 + reviewer #1: Single `loadTools` call with the union of `agent.tools + allowed-tools`. The earlier two-call approach dropped `userMCPAuthMap` / `toolContextMap` / `actionsEnabled` from the skill-added pass — an MCP tool gained via `allowed-tools` would be visible to the model but fail at execution without per-user auth context. Resolution of `manualSkillPrimes` is hoisted before `loadTools` so the union can be computed up-front; the dropped-tools debug log now compares loaded vs. requested across the single call. Codex P3 + reviewer #2: `injectSkillCatalog.activeSkillIds` now includes `disable-model-invocation: true` skills. The runtime ACL check in `handleSkillToolCall` previously couldn't reach the explicit "cannot be invoked by the model" rejection because the broader access set excluded those skills. Catalog text and tool registration still gate on the visible subset (zero-context-token guarantee preserved); only the per-user `isActive` filter is a hard exclusion now. Reviewer #1 (try/catch around loadTools, MAJOR): A single bad `allowed-tools` entry from a shared skill could crash the entire turn. Now wrapped — on failure with extras, retry with just `agent.tools` and continue (the dropped-tools debug log surfaces what vanished). If the retry-without-extras still throws, propagate; the agent's own tools are the load-bearing surface. Reviewer #3 (integration tests, MAJOR): Added six tests in `initialize.test.ts` covering the full `allowed-tools` loading path: union pass-through, no-extras short-circuit, agent-baseline dedup, loadTools throw + retry, propagated throw without extras, and the empty-tools edge case. Smaller cleanups bundled in: - Reviewer #4: Moved `logger` import to the package-imports section (was wedged among local imports). - Reviewer #5: Removed unused index on `disableModelInvocation` (filtering happens application-side in `injectSkillCatalog`; index cost write overhead for zero query benefit). - Reviewer #6: Swapped order of `userInvocable` and body checks in `resolveManualSkills` so the more authoritative author-decision reason surfaces first when both apply. - Reviewer #8: Documented the `allowedTools` enforcement gap on the schema + type — model-invoked skills (mid-turn `skill` tool calls) do NOT trigger tool union, since adding tools after the graph starts would require a rebuild. Manual / always-apply (Phase 5) primes are the supported paths. - Reviewer #9: Renamed `dmi` / `ui` / `at` locals to `disableModelInvocationRaw` / `userInvocableRaw` / `allowedToolsRaw` in `deriveStructuredFrontmatterFields`. Reviewer #7 (DRY shared `getSkillByName` return type) deferred — field sets diverge meaningfully across the three call sites (handler needs `body + fileCount`; resolver needs `author + allowedTools + userInvocable`; the InitializeAgentDbMethods contract needs the superset). A `Pick<>`-based consolidation is a follow-up cleanup. * 🔧 fix: Address codex iter 2 — catalog quota + duplicate-name dedup P1: `injectSkillCatalog` cap now counts only model-visible skills, not the merged active set. The previous behavior let a tenant with many `disable-model-invocation: true` rows near the top of the cursor exhaust the 100-slot quota before any invocable skill got scanned — the catalog could end up empty even though invocable skills existed further down the paginated results. `MAX_CATALOG_PAGES` stays the ceiling on scan budget; only `visibleCount` drives the early-exit on quota fill. P2: When an invocable and a `disable-model-invocation: true` skill share a name, drop the disabled doc(s) from `activeSkillIds`. Without this dedup, `getSkillByName` (which sorts by `updatedAt` desc) could pick the disabled doc and every model call to the cataloged name would fail with "cannot be invoked by the model" instead of executing the visible skill. When ONLY a disabled doc exists for a name, it stays in `activeSkillIds` so the explicit-rejection error path still fires for hallucinated invocations. Tests: 3 new cases in `injectSkillCatalog` covering (a) cap counted on visible skills only, (b) same-name collision drops disabled doc, (c) sole-disabled-name case keeps the disabled doc. * 🔒 fix: Apply `disable-model-invocation` gate to read_file too (codex iter 3 P1) `activeSkillIds` is shared between the `skill` and `read_file` handlers. The skill-tool gate was applied last iteration, but `handleReadFileCall` authorized purely on `getSkillByName(..., accessibleIds)` — so a model that learned a hidden skill's name (stale catalog or hallucination) could still read its `SKILL.md` body or bundled files via `read_file`, defeating the contract. Same explicit rejection now fires from both handlers; no change needed to the ACL set itself (disabled docs stay in `activeSkillIds` so the explicit error path keeps firing). Two new tests in `handlers.spec.ts` cover the read_file gate and regression-protect the happy path. * 🔧 fix: Address codex iter 4 — manual-prime exception + legacy frontmatter backfill P1: Scope the `read_file` `disableModelInvocation` gate to AUTONOMOUS model probes only. A user-invoked `$` skill that is also marked `disable-model-invocation: true` had its bundled `references/*` / `scripts/*` files unreadable, leaving the manually-primed skill body referencing files the model couldn't load. Now the handler bypasses the gate when the skill name appears in `manualSkillNames` (the per-turn allowlist threaded from `manualSkillPrimes` → `agentToolContexts` → `enrichWithSkillConfigurable` → `mergedConfigurable`). Defense-in-depth: the bypass is scoped to the specific names in the allowlist; a different disabled skill name is still rejected. P2: Read-time fallback for legacy skills authored before Phase 6 landed the structured columns. `user-invocable: false` / `disable-model-invocation: true` set in `frontmatter` (the validator already accepted those keys) but with no derived column would incorrectly evaluate as "user-invocable / model-allowed" until a save backfilled the columns. New `backfillDerivedFromFrontmatter` helper fills undefined columns from frontmatter at read time in both `getSkillByName` and `listSkillsByAccess` — column wins when both are set, frontmatter fills the gap when only it's set. No DB writes; the next `updateSkill` naturally persists. `listSkillsByAccess` projection expanded to include `frontmatter` (bounded by validator, payload impact small) so summaries can also be backfilled. Sticky-primed disabled skills (ones invoked in prior turns of the same conversation) are not yet in the manual-prime allowlist — same- turn manual invocation is the load-bearing path codex flagged; the sticky-turn case is a known limitation tracked for a follow-up. Tests: 2 new in handlers.spec.ts (manual-prime allows + name-scoped block holds), 3 new in skill.spec.ts (legacy backfill via getSkillByName + listSkillsByAccess + column-wins precedence). * 🔧 fix: Address codex iter 5 — propagate manualSkillNames + keep read_file P1: `enrichWithSkillConfigurable` is also called from `openai.js` and `responses.js` (the OpenAI Responses + completions endpoints). Both were ignoring the new `manualSkillNames` parameter, which meant the manual-prime exception in the `read_file` gate (iter 4) only worked on the agents endpoint. Now all three call sites pass `primaryConfig.manualSkillPrimes?.map(p => p.name)` so manual `$` invocations of disabled skills work consistently across endpoints. P2: When every accessible skill is `disable-model-invocation: true`, the catalog text and `skill` tool are correctly omitted (no model- reachable targets) — but `read_file` and `bash_tool` MUST still be registered. A user manually invoking such a skill gets its SKILL.md body primed into context; if the body references `references/foo.md` or `scripts/run.sh`, those reads need a registered tool. Restructured `injectSkillCatalog` so `skill` registration is gated on `catalogVisibleSkills.length > 0` while `read_file` (always) and `bash_tool` (when codeEnvAvailable) register whenever any active skill is in scope. Tests: existing all-disabled test rewritten to assert read_file IS registered + skill is NOT; new test confirms bash_tool joins it when codeEnvAvailable. * 🔧 fix: Address codex iter 6 — name-collision consistency via preferInvocable P2a (resolveManualSkills): a name collision between an older user-invocable doc and a newer non-user-invocable doc made manual `$` invocation silently no-op. The popover surfaced the older invocable doc; resolver looked it up by name; `getSkillByName` returned the newer non-invocable doc; resolver skipped on `userInvocable: false`. P2b (handler / runtime ACL): with same-name duplicates (e.g. older invocable + newer disabled), the manual prime resolved to one doc while later `read_file` / `skill` execution resolved a different doc through `activeSkillIds`. Model could follow one SKILL.md body while reading files from a different skill. Both root-cause: `getSkillByName` always returned the newest match and let the caller filter, but with collisions the newest can be something the caller didn't want. Fix: extend `getSkillByName` with `options.preferInvocable`. When true, prefer the newest doc satisfying BOTH `userInvocable !== false` AND `disableModelInvocation !== true` (with frontmatter backfill); fall back to the newest match otherwise. Fast path preserved when caller doesn't opt in. Callers passing `preferInvocable: true`: - `resolveManualSkills` — picks the popover-visible invocable doc even when a newer disabled / non-user-invocable duplicate exists. - `handleSkillToolCall` — keeps execution aligned with the catalog; falls back to the disabled doc only when no invocable variant exists (so the explicit "cannot be invoked by the model" gate still fires for the hallucinated-disabled-name case). - `handleReadFileCall` — same alignment, plus the manual-prime exception added in iter 4 still applies. Tests: - 2 new in skill.spec.ts (preferInvocable picks invocable when collision exists; falls back to newest when no clean-invocable exists). - 1 new in skills.test.ts (resolver passes preferInvocable through). - 2 new in handlers.spec.ts (skill tool + read_file pass it). - Existing initialize.test.ts assertion updated for the new option. * 🔧 fix: Address codex iter 7 — split preferInvocable into per-axis flags The previous unified `preferInvocable` filter required BOTH `userInvocable !== false` AND `disableModelInvocation !== true`. That was wrong for the model paths: `userInvocable: false` skills are model-only and remain valid `skill` / `read_file` invocation targets. A duplicate-name scenario where the newer cataloged doc was model- only would let the older user-invocable variant shadow it on every model call. Split the option into two independent axes: - `preferUserInvocable` — for manual paths (`$` popover). Skips docs with `userInvocable: false`. Disable-model-invocation status is irrelevant; iter 4 explicitly supports manual prime of disabled skills. - `preferModelInvocable` — for model paths (`skill` / `read_file` handlers). Skips docs with `disableModelInvocation: true`. User- invocable status is irrelevant; model-only skills are valid here. Both flags fall back to the newest match when no preferred doc exists, so the explicit-rejection error paths still fire correctly in the sole-disabled-name case. Callers updated: - `resolveManualSkills` → `preferUserInvocable: true` - `handleSkillToolCall` / `handleReadFileCall` → `preferModelInvocable: true` Tests: - New spec test for preferModelInvocable not filtering on userInvocable. - Existing preferInvocable test renamed/split to cover the new axes. - New test asserts preferUserInvocable still returns disabled docs (preserves iter 4 manual-disabled support). - Caller tests assert each path passes the right single flag and does NOT pass the wrong one. * 🔧 fix: TypeScript type-check failure in handlers.spec.ts (CI green) `jest.fn(async () => ...)` without explicit args infers an empty tuple for the call signature, so `mock.calls[0][2]` flagged as "Tuple type '[]' has no element at index '2'." Cast to `unknown[]` then narrow to the expected option shape. Behavior unchanged. Caught by the `Type check @librechat/api` CI step (.github/workflows/backend-review.yml). * 🔧 fix: Address codex iter 8 — undefined-result fallback + read_file alignment P1 (loadTools returning undefined): Production loaders (`createToolLoader` in `initialize.js` / `openai.js` / `responses.js`) wrap `loadAgentTools` in try/catch and return `undefined` on failure rather than throwing. Without explicit handling, my iter-1 try/catch only fired for thrown errors — a silent-failure on a skill-added tool would fall through to the empty fallback and silently DROP the agent's baseline tools for the turn (much worse than just losing the extras). Added an `undefined`-result branch that retries with just `agent.tools`, mirroring the throw branch. Test pins both behaviors. P2 (read_file alignment with manual prime): When a skill is in this turn's `manualSkillNames`, the `read_file` handler now uses `preferUserInvocable` instead of `preferModelInvocable`. Same name-collision rule as `resolveManualSkills`, so the doc whose files get read is the same doc whose body got primed. For autonomous probes (skill not in `manualSkillNames`), the handler keeps `preferModelInvocable` to align with the catalog the model saw. Two new tests cover both branches and regression-protect that the wrong flag isn't passed. * 🔧 fix: Address codex iter 9 — pin read_file lookup to primed skill _id P1 (manually-primed disabled IDs were dropped from activeSkillIds): The `executableSkills` dedup in `injectSkillCatalog` correctly drops `disable-model-invocation: true` duplicates when an invocable doc shares the name — but `resolveManualSkills` legitimately primes disabled docs (iter 4 supports manual `$` invocation of disabled skills). When the resolver primed a disabled doc, the read_file handler couldn't find it in the (deduped) `activeSkillIds` and either resolved a different same-name skill or returned not-found. Fix: `ResolvedManualSkill` now carries `_id`; the legacy `initialize.js` / `openai.js` / `responses.js` controllers build a `manualSkillPrimedIdsByName` map and `enrichWithSkillConfigurable` passes it into `mergedConfigurable`. `handleReadFileCall` now pins its lookup's `accessibleIds` to `[primedId]` whenever the requested skill is in the map. The constrained set guarantees the lookup returns the EXACT doc the resolver primed — body/files come from the same source even when same-name duplicates exist or the dedup removed the prime's id from `activeSkillIds`. Autonomous read_file probes (skill not in the manual-primed map) keep the full ACL set + `preferModelInvocable` so they align with the catalog the model saw and the disabled-only case still fires the explicit-rejection gate. Test fixture changes flow from `_id` becoming required on `ResolvedManualSkill`. `buildSkillPrimeContentParts` / `injectManualSkillPrimes` widen their param types to `Pick<...>` because they only read `name` / `body` and shouldn't force test literals to invent placeholder ids. * 🧹 fix: Address independent reviewer findings (DRY + types + tests + docs) Sanity-pass review surfaced 7 findings; addressed 6 (the 7th — DRY on inline `getSkillByName` return types — is acknowledged tech debt deferred to a follow-up). #1 [MAJOR, DRY]: The 4-line `manualSkillPrimedIdsByName` map construction was duplicated across 4 CJS call sites (openai.js, responses.js x2, initialize.js). Extracted `buildManualSkillPrimedIdsByName` helper in `skillDeps.js`; all four sites now call the helper. If `ResolvedManualSkill` ever renames `_id` or gains identifying fields, only the helper changes. #2 [MINOR, type safety]: `handleReadFileCall` was casting a hex string to `Types.ObjectId[]` via `as unknown as`, relying on mongoose's auto-cast in `$in` queries. Replaced with `new Types.ObjectId(...)` so any future consumer comparing with `.equals()` / `===` gets the correct value type. Imported `Types` as a value (was type-only). #5 [MINOR, test gap]: Added a test for the worst-case silent-failure path — both the union and base-only `loadTools` calls return undefined. The agent gets no tools but the turn doesn't crash hard; pinning that contract. #4 [MINOR, performance]: Added a TODO on the `listSkillsByAccess` projection noting the `frontmatter` field can be dropped once a write migration backfills all pre-Phase-6 skills' columns. ~2KB/skill × 100/page is wasted bandwidth post-backfill. #6 [NIT, docs]: `backfillDerivedFromFrontmatter` JSDoc said "Pure" right before "mutates its undefined fields in place". Replaced with "Side-effect-free w.r.t. the DB (no writes), but mutates its argument in place" which describes both halves accurately. #7 [NIT, test determinism]: Replaced `await new Promise(r => setTimeout(r, 5))` in two same-name collision tests with explicit `updateOne` setting `updatedAt: new Date(Date.now() - 1000)` on the older doc. Removes the wall-clock race on fast CI runners. The pagination test (line 480) still uses setTimeout — that test is pre-existing and order is incidental, not load-bearing. Existing test fixtures updated to use valid 24-char hex ObjectIds (required by the iter-9 test that constructs a real `ObjectId`). #3 [MINOR, deferred]: Inline `getSkillByName` return type duplicated across `handlers.ts`, `initialize.ts`, `skills.ts`. Reviewer acknowledged this as deferred; field sets diverge across call sites (handler needs `fileCount`, resolver needs `author`/`allowedTools`). A `Pick<>`-based consolidation is a clean follow-up.	2026-04-25 04:02:00 -04:00
Danny Avila	539c4c7e4d	🎬 feat: Prime Manually-Invoked Skills via $ Popover (#12709) ... * 🎬 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 `<InvokingSkillsIndicator message={message} />` 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 `<SkillCall .../>` 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).	2026-04-25 04:02:00 -04:00
Danny Avila	3f1bde4831	🪆 feat: Compose Agent Scope and Active-State Filters in $ Popover (#12708) ... * feat: compose per-agent scope and per-user active-state filters in $ popover Stack two runtime-truth filters on top of the existing `isUserInvocable` check so the `$` skill popover matches what will actually be available at turn time: - Per-agent skill scope from `agent.skills` (resolved via useChatContext + useAgentsMapContext), mirroring backend `scopeSkillIds` semantics: `undefined`/`null` → no scope, `[]` → empty, non-empty → intersection. - Per-user ownership-aware active state via `useSkillActiveState().isActive`. The filters are composed in a pure, exported `filterSkillsForPopover` helper so the agent-scope ∩ active ∩ invocable matrix can be unit-tested without rendering the component. Short-circuits on cheapest check first (agent scope → active → invocation mode). Backend still enforces both filters at runtime; this PR is a UX mirror so users do not see popover entries that would be filtered out by the time the LLM turn begins. * refactor: thread agentId into SkillsCommand as a prop Drop the direct `useChatContext()` call inside SkillsCommand in favor of receiving `agentId` from ChatForm. The parent already subscribes to the conversation via its single useChatContext call, and SkillsCommand is wrapped in React.memo — threading the id as a prop means the popover only re-renders when agent_id actually changes instead of on every unrelated conversation-shape mutation. Mirrors the pattern AttachFileChat already uses for `conversation` / `agentId`. No behavior change; filter semantics and test coverage are identical. * fix: fail closed when agent skill scope cannot be resolved Previously, if `conversation.agent_id` was set but the agents map had no entry for it (hydration pending, query failure, or missing VIEW access), the popover treated the scope as `undefined` and showed the full ACL catalog. That leaks options the backend will reject at turn time, the opposite of what this phase is meant to do. Distinguish the unresolved cases ("map undefined" and "agent not in map") from the intentionally-unconfigured case ("agent exists, no `skills` field") and return `[]` for the former, preserving the backend semantics of `scopeSkillIds` only for the latter. Adds two tests covering both fail-closed branches. * fix: surface agent.skills in list projection and treat ephemeral ids as unscoped Two holes flagged on the earlier fail-closed commit: 1. The list-agents projection in `getListAgentsByAccess` omitted the `skills` field, so `agentsMap[agentId].skills` was always undefined and the popover fell back to the full ACL catalog for every scoped agent — the opposite of this phase's intent. Add `skills: 1` to the projection and lock it in with a new test. 2. Conversations can carry ephemeral agent ids (e.g. `ephemeral` after switching off the agents endpoint) that intentionally don't live in the agents map. The prior fail-closed branch blanked the popover in those cases. Treat anything that doesn't start with `agent_` as unscoped via the existing `isEphemeralAgent` helper so the popover shows the full ACL-visible catalog, matching how no-agent convos already behave. Frontend: 18 tests pass (adds one ephemeral-id case). Backend: 10 getListAgentsByAccess tests pass (adds one projection case). * refactor: pass through hydration race, only fail closed when map is authoritative Split the two "cannot resolve scope" cases that were previously collapsed onto the same fail-closed branch: - `agentsMap === undefined` means the agents list query has not settled yet. Return `undefined` (full catalog). The map typically hydrates well before the first `$` open, and the backend still scopes at turn time — blanking the popover during a sub-second race produces worse UX with no security benefit. - `agentsMap` is populated but the agent is absent means the agent was deleted or the user's VIEW access was revoked mid-session. That is authoritative missing state, so keep the fail-closed behavior — the full catalog would be misleading. Updates the associated test case to assert the hydration-race path now shows the catalog, and rewrites the in-code comment to distinguish the two branches. * refactor: drop `as string` in agent scope memo by tightening the guard `isEphemeralAgent` returns true for null/undefined so the original code was runtime-safe, but its signature returns plain `boolean` rather than a type predicate, so TypeScript never narrowed `agentId` to `string` and the subsequent map lookup required an `as string` assertion. Split the guard into `!agentId || isEphemeralAgent(agentId)` so the narrowing falls out naturally and the assertion can be removed. No behavior change; 18 tests pass.	2026-04-25 04:02:00 -04:00
Danny Avila	9225a279eb	🎚️ feat: Per-User Skill Active/Inactive Toggle with Ownership-Aware Defaults (#12692) ... * feat: per-user skill active/inactive toggle with ownership-aware defaults - Add `skillStates` map (Record<string, boolean>) to user schema for per-user active/inactive overrides on skills - Add `defaultActiveOnShare` to interface.skills config (default: false) so admins can control whether shared skills auto-activate - Add GET/POST /api/user/settings/skills/active endpoints with validation - Add React Query hooks with optimistic mutations for skill states - Add useSkillActiveState hook with ownership-aware resolution: owned skills default active, shared skills default inactive - Add toggle switch UI to SkillListItem and SkillDetail components - Filter inactive skills in injectSkillCatalog before agent injection - Add localization keys for active/inactive labels * fix: use Record instead of Map for IUser.skillStates Mongoose .lean() flattens Map to a plain object, causing type incompatibility with IUser in methods that return lean documents. * fix: address review findings for skill active states - Fail-closed when userId is absent: filter rejects all shared skills instead of passing them through unfiltered (Codex P1) - Validate Mongoose Map key characters (reject . and $) in controller to return 400 instead of a 500 from schema validation (Codex P2) - Block toggle while initial skill states query is loading to prevent overwriting server-side overrides with an empty snapshot (Codex P2) - Extract shared SkillToggle component, eliminating duplicate toggle markup in SkillListItem and SkillDetail (Finding #3) - Move skill state query/mutation hooks from Favorites.ts to Skills/queries.ts per feature-directory convention (Finding #4) - Fix hardcoded English aria-label in SkillListItem by passing the localized string from the parent SkillList (Finding #5) - Fix inline arrow in SkillList render loop: pass stable callback reference so SkillListItem memo() is not invalidated (Finding #1) - Extract toRecord() helper in controller to DRY the Map-to-Object conversion (Finding #6) - Remove Promise.resolve wrapping synchronous config read (Finding #8) - Remove unused TUpdateSkillStatesRequest type (Finding #12) * fix: forward tabIndex on SkillToggle to preserve list keyboard nav The original inline toggle had tabIndex={-1} so the row itself remained the sole tab target. The extraction into SkillToggle dropped this prop, making every list toggle a tab stop. Add an optional tabIndex prop and pass -1 from SkillListItem. * fix: plumb skillStates to all agent entry points, isolate toggle keydown - Add skillStates/defaultActiveOnShare loading to openai.js and responses.js controllers so shared-skill activation is respected across all agent entry points, not just initialize.js (Codex P1) - Stop keydown propagation on SkillToggle so Enter/Space does not bubble to the parent row's navigation handler (Codex P2) * fix: paginate catalog fetch and serialize toggle writes - Paginate listSkillsByAccess (up to 10 pages of 100) until the active catalog quota is filled, so inactive shared skills in recent positions do not starve active owned skills past the first page (Codex P1) - Extend listSkillsByAccess interface with cursor/has_more/after for catalog pagination - Serialize skill-state writes via a ref queue: one in-flight request at a time, with the latest desired state sent when the previous one settles. Prevents last-response-wins races where an older request overwrites newer toggles (Codex P2) * fix: share write queue across hook instances, block toggle on fetch error - Move the write queue from a per-instance useRef to a module-scoped object so every mount of useSkillActiveState (SkillList, SkillDetail, etc.) serializes against the same in-flight slot. Prior per-instance queues allowed two components to race full-map POSTs (Codex P1) - Extend the toggle guard beyond isLoading: also block when isError is true or data is undefined. Prevents a failed GET from seeding a toggle with an empty baseline that would wipe server-side overrides on the next successful POST (Codex P1) * fix: stale closure, orphan cleanup, and cap-error UX - Read toggle baseline from React Query cache via queryClient.getQueryData instead of the captured skillStates closure. The closure can be stale between onMutate's setQueryData and the next render, so rapid successive toggles would build on old state and drop earlier changes (Codex P1) - Surface the MAX_SKILL_STATES_EXCEEDED error code with a specific toast key (com_ui_skill_states_limit) so users understand the 200-cap rather than seeing a generic error - Prune orphaned entries (skillIds whose Skill doc no longer exists) on both GET and POST in SkillStatesController. Self-heals over time without needing cascade-delete hooks or a migration job. Uses one indexed Skill._id query per request * test: pin skill active-state precedence with unit tests Extract the active-state resolution logic from a closure inside injectSkillCatalog into an exported resolveSkillActive helper, then cover every branch of the precedence matrix: - Fails closed when userId is absent (even with defaultActiveOnShare=true) - Explicit override wins over ownership and config (both true and false) - Owned skills default to active when no override is set - Shared skills default to defaultActiveOnShare value - Undefined skillStates behaves identically to an empty object - defaultActiveOnShare defaults to false when omitted - Owned skills ignore defaultActiveOnShare entirely Closes Finding #2 from the pre-rebase comprehensive review. Mirrors the existing scopeSkillIds test style; injectSkillCatalog now calls resolveSkillActive instead of inlining the closure. * refactor: limit skill active toggle to detail header, drop label - Remove the per-row toggle from SkillListItem and the active-state plumbing (hook call, isSkillEnabled/onToggleEnabled/toggleAriaLabel props) from SkillList. The detail view is now the single place to change a skill's active state - Drop dim/muted styling for inactive skills in the sidebar: without a control there, the visual indication has nowhere to land - Resize SkillToggle to match neighbor buttons: outer h-9 container, h-6 w-11 track with size-5 knob, no label span. The 'Active' / 'Inactive' text that accompanied the detail-view toggle is removed - Remove the now-unused label prop and tabIndex prop (the tabIndex existed only for the list-row context) from SkillToggle. Drop the onKeyDown stopPropagation for the same reason - Remove now-orphaned com_ui_skill_active / com_ui_skill_inactive translation keys * style: shrink SkillToggle track to h-5 w-9 with size-4 knob Container stays at h-9 to match neighbor button heights. The toggle track itself drops from h-6 w-11 to h-5 w-9, with a size-4 knob travelling 1.125rem on activation. Visually lighter inside the row. * fix: remove redundant skillStates entries that match the resolved default When a toggle lands on the ownership/config default, delete the key from the map instead of persisting `{id: defaultValue}`. Without this, a user toggling a skill off and back on would leave `{id: true}` for an owned skill (whose default is already true), silently consuming a slot against the 200-entry cap. Repeated round-trip toggles could exhaust the quota with zero meaningful overrides (Codex P2). Preserves the exceptions-list invariant that the runtime-resolution design depends on. * fix: prune before enforcing skill-state cap; reject non-ObjectId keys Reorder the update controller so pruneOrphans runs before the 200-cap check. Without this, a user near the cap with some orphaned entries (skills deleted since their last GET) could send a payload that would pass after pruning but gets rejected by the raw-size check first. Add a sanity cap on raw payload size (2 * MAX_SKILL_STATES) so abusive inputs do not reach the DB query, and enforce the real cap on the pruned result instead. Harden pruneOrphans: the earlier early-return path could pass non-ObjectId keys through unchanged. Now only valid ObjectIds are returned, and the Skill-model-unavailable fallback filters by format. Also add isValidObjectIdString validation at the input boundary so malformed (but otherwise non-Mongo-unsafe) keys never reach persistence (Codex P2 x2). * fix: enforce active filter at execute time, prune revoked shares, scope queue per user P1: injectSkillCatalog now returns activeSkillIds (the filtered set that appears in the catalog). initializeAgent uses that set as the stored accessibleSkillIds on the initialized agent, so getSkillByName at runtime cannot resolve a deactivated skill — even if the LLM hallucinates a name or the user invokes by direct-invocation shorthand. Previously the executor authorized against the full ACL set, bypassing the active-state guarantee (Codex P1). P2: pruneOrphans now checks user access via findAccessibleResources in addition to skill existence. When a share is revoked, the user's skillStates entry for that skill had no cleanup path and silently consumed the 200-cap. Self-heals on both GET and POST. One extra ACL query per settings read/write; scoped to a single user so no N-user amplification (Codex P2). P2: the write queue moves from a single module-scoped object to a Map keyed by userId. Logout/login in the same tab can no longer flush the previous user's pending snapshot under the new session's auth. Each userId gets its own pending/inFlight slot; the in-flight request retains its original auth via the cookie already attached when sent, so the race window closes (Codex P2). * refactor: extract skillStates helpers to packages/api; add tests; polish Address the remaining valid findings from the comprehensive review: - Extract toRecord, loadSkillStates, validateSkillStatesPayload, and pruneOrphanSkillStates into packages/api/src/skills/skillStates.ts as TypeScript. The controller in /api shrinks to a ~90-line thin wrapper that builds live dependency adapters for Mongoose + the permission service (Review #2 DRY, #3 workspace boundary) - Replace the triplicated 12-line skillStates loading block in initialize.js, openai.js, and responses.js with a single call to loadSkillStates from @librechat/api. One helper, three sites - Swap console.error for the project logger in the controller (Review #7) - Remove the redundant INVALID_KEY_PATTERN regex: a valid ObjectId cannot contain . or $, so isValidObjectIdString already covers it (Review #11) - Parameterize the 200-cap error toast with {{0}} interpolation driven by the error response's `limit` field, so future changes to MAX_SKILL_STATES update the UI message automatically (Review #12) - Add 24 unit tests for the new skillStates helpers (toRecord, resolveDefaultActiveOnShare, loadSkillStates, validateSkillStates- Payload, pruneOrphanSkillStates) covering success paths, malformed input, cap boundaries, and parallel-query behavior (Review #4) - Add 10 tests for injectSkillCatalog pagination covering empty accessible set, missing listSkillsByAccess, single-page filter, owned-vs-shared defaults, explicit-override precedence, multi-page collection, MAX_CATALOG_PAGES safety cap, early termination on has_more=false, additional_instructions injection, and fail-closed without userId (Review #5) Total test count: 60 (was 26 on this surface). * fix: rename skillStates ValidationError to avoid barrel-export collision packages/api/src/types/error.ts already exports a ValidationError (MongooseError extension). Re-exporting a different shape from skills/skillStates.ts through the skills barrel caused TS2308 in CI because the root index re-exports both. Rename to SkillStatesValidationError to keep the exports disjoint. * refactor: tighten tests and absorb caller guard into loadSkillStates Address the followup review findings: - Add optional `accessibleSkillIds` param to loadSkillStates so the helper short-circuits to defaults when no skills are accessible. All three controllers drop the residual 7-line conditional wrapper in favor of a single destructured call (Review #2) - Remove the unreachable `typeof key !== 'string'` check from validateSkillStatesPayload: Object.entries always yields string keys per the JS spec (Review #3) - Replace the two `as unknown as` agent casts in the injectSkillCatalog tests with a `makeAgent()` factory typed directly as the function's parameter shape (Review #4) - Tighten the MAX_CATALOG_PAGES assertion from `toBeLessThanOrEqual(11)` to `toHaveBeenCalledTimes(10)` — the loop deterministically makes exactly 10 page fetches before hitting the cap (Review #1) - Rewrite the parallel-execution test for pruneOrphanSkillStates using deferred promises instead of microtask-order assertions. The test now inspects `toHaveBeenCalledTimes(1)` on both mocks after a single Promise.resolve() yield, pinning Promise.all usage without relying on push-order into a shared array (Review #5) - Evict stale writeQueue entries on user change via a module-scoped `lastSeenUserId` sentinel. When a different user's toggle is the first one after a logout/login, the previous user's queue entry is deleted. Keeps the Map bounded without adding hook-instance effect cleanup (Review #6) * fix(test): mock loadSkillStates in openai and responses controller specs The prior refactor replaced the inline 12-line skillStates loading block with a call to loadSkillStates from @librechat/api. Both controller spec files mock @librechat/api as a flat object, so any new named import from that package is undefined in the test env. Calling `await loadSkillStates(...)` threw before recordCollectedUsage ran, surfacing as "undefined is not iterable" on the test's array destructure of `mockRecordCollectedUsage.mock.calls[0]`. Add the missing mock to both spec files alongside the existing scopeSkillIds stub. * fix: abandon stale skillStates write queues on user switch Close the cross-session leak window where an in-flight flush loop still holds a reference to a previous user's queue: it could fire its next mutateAsync under the new session's auth cookies and persist the stale snapshot to the new user's document (Codex P1). Add an `abandoned` flag on `WriteQueue`. Three mechanisms cooperate: - `getWriteQueue` marks every non-active queue abandoned when the user differs from the last-seen identity (pre-existing eviction site, now more aggressive). - A `useEffect` on `userId` calls the same abandonment pass on every render with a new active identity, covering the window between logout/login and the new user's first toggle (when `getWriteQueue` would otherwise not fire). - The flush loop checks `!queue.abandoned` in its while condition so the second and later iterations exit without firing another `mutateAsync` after the session changes. The first iteration's in-flight request (already dispatched under the original user's cookies) still runs to completion or failure on its own — only the subsequent iterations, which are the dangerous ones, are blocked.	2026-04-25 04:02:00 -04:00
Danny Avila	3e064c2f2b	🎯 feat: Per-Agent Skill Selection in Builder and Runtime Scoping (#12689) ... * feat: per-agent skill selection in builder and runtime scoping Wire skills persistence on the Agent model and enable the skills section in the agents builder panel. At runtime, scope the skill catalog to only the skills configured on each agent (intersected with user ACL). When no skills are configured, the full user catalog is used as the default. The ephemeral chat toggle overrides per-agent scoping to provide the full catalog. * fix: add scopeSkillIds to @librechat/api mock in responses unit test The test mocks @librechat/api but was missing the newly imported scopeSkillIds, causing createResponse to throw before reaching the assertions. Added a passthrough mock that returns the input array. * fix: scope primeInvokedSkills by agent's configured skills primeInvokedSkills was receiving the full unscoped accessibleSkillIds, bypassing the per-agent skill scoping applied to initializeAgent. This allowed previously invoked skills from message history to be resolved and primed even when excluded from the agent's configured skill set. Apply the same scopeSkillIds filtering to match the initializeAgent calls, so skill resolution is consistent across catalog injection and history priming. * fix: preserve agent skills through form reset and union prime scope Two related bugs in the per-agent skill selection flow: 1. resetAgentForm dropped the persisted skills array because the generic fall-through at the end of the loop excludes object/array values. Combined with composeAgentUpdatePayload always emitting skills, this caused any save of a previously-configured agent to silently overwrite skills with an empty array. Add an explicit case for skills mirroring the agent_ids handling. 2. primeInvokedSkills processes the full conversation payload, including prior handoff-agent invocations. Scoping it to only primaryAgent.skills meant a skill invoked by a handoff agent in a prior turn could not be resolved when the current primary agent had a different scope, leaving message history reconstruction incomplete. Union the per-agent scoped accessibleSkillIds across primary plus all loaded handoff agents so any skill any active agent could invoke is resolvable from history. * fix: mark inline skill removals as dirty The inline X button on the skills list called setValue without shouldDirty: true, so removing a skill via this control did not mark the skills field as dirty in react-hook-form state. When a user removed a skill with the X button and also staged an avatar upload in the same save, isAvatarUploadOnlyDirty returned true and onSubmit short-circuited to avatar-only upload, silently dropping the PATCH that would persist the skill removal. The dialog path (SkillSelectDialog) already passes shouldDirty: true on add/remove; this aligns the inline control with that behavior. * fix: restore full ACL scope for primeInvokedSkills history reconstruction Reverting the earlier scoping of primeInvokedSkills to the active-agent union. That change conflated runtime invocation scoping (which correctly gates what the model can call now) with history reconstruction (which restores bodies the model already saw in prior turns). Per-agent scoping still applies at: - Catalog injection (injectSkillCatalog via initializeAgent) - Runtime invocation (handleSkillToolCall via enrichWithSkillConfigurable, using each agent's scoped accessibleSkillIds in agentToolContexts) History priming is a read of past context, not a grant of new capability. Scoping it causes historical skill bodies to vanish from formatAgentMessages when an agent's skills list is edited mid-conversation or when the ephemeral toggle flips, which breaks message reconstruction and drops code-env file continuity for /mnt/data/{skillName}/ references. The user's ACL-accessible set is the correct and sufficient gate for history reconstruction. * fix: close openai.js skill gap and pin undefined vs [] semantics Three related gaps surfaced in review: 1. api/server/controllers/agents/openai.js was a third skill resolution site alongside responses.js and initialize.js, but still used the old activation gate (required ephemeralAgent.skills === true) and never passed accessibleSkillIds through scopeSkillIds. Per-agent scoping silently did not apply on this route. Mirror the same pattern used in responses.js so all three routes behave identically. 2. scopeSkillIds previously collapsed undefined and [] into the same "full catalog" fallback, making it impossible for a user to express "this agent has no skills." Tighten the semantics before any data is written under the old behavior: - undefined / null = not configured, full catalog - [] = explicitly none, returns [] - non-empty = intersection with ACL-accessible set Update defaultAgentFormValues.skills from [] to undefined so a brand new agent whose skills UI was never touched does not accidentally persist "explicit none" on first save (removeNullishValues strips undefined from the payload server side). 3. Add direct unit tests for scopeSkillIds covering all five cases (undefined, null, empty, disjoint, overlap, exact match, empty accessible set). 16 tests total in skills.test.ts pass. * fix: add scopeSkillIds to @librechat/api mock in openai unit test Same pattern as the earlier responses.unit.spec.js fix: the test mocks @librechat/api with an explicit object, so each newly imported symbol must be added to the mock. Without scopeSkillIds, OpenAIChatCompletion controller throws on destructuring before reaching recordCollectedUsage, causing the token usage assertions to fail.	2026-04-25 04:02:00 -04:00
Danny Avila	9b4ae068b2	💲 feat: Manual Skill Invocation via $ Command Popover (UI only) (#12690) ... * feat: add $ command popover for manual skill invocation Adds a new `$` command trigger in the chat textarea that opens a searchable popover listing user-invocable skills. Selecting a skill inserts `$skill-name` into the message and enables the skills badge on the ephemeral agent. Follows the same patterns as `@` mentions and `/` prompts. * test: update useHandleKeyUp tests for $ skill command Add showSkillsPopoverFamily, dollarCommand, and SKILLS permission to the store/recoil/access mocks. Include test cases for the $ trigger, toggle gating, and permission gating. * fix: address review findings in SkillsCommand - Exclude auto-mode skills from popover (isUserInvocable now returns false for InvocationMode.auto) - Rewrite JSDoc to match the corrected filter logic - Guard ArrowUp/ArrowDown against NaN when matches is empty - Replace useRecoilState with useSetRecoilState for ephemeralAgent to avoid subscribing to unrelated agent state changes - Move skills guard into setter callback and drop ephemeralAgent from handleSelect dependency array - Add e.preventDefault() for Tab key alongside Enter - Render error state (com_ui_skills_load_error) on query failure - Render empty state (com_ui_skills_empty) when no matches - Hoist ScrollText icon to module-level constant - Export isUserInvocable for testability * fix: address follow-up review findings - Differentiate empty-catalog ("No skills yet") from no-match search ("No skills found") using existing com_ui_no_skills_found - Clamp activeIndex when matches shrink from search filtering to prevent silent Enter/Tab failures - Add early return after Escape handler to skip redundant checks - Reorder package imports shortest-to-longest after react - Add fast-typing test case for $ command ("$sk" at position 3) * fix: gate $ command on assistants endpoint, fix Tab on empty matches - Block $ popover on assistants/azureAssistants endpoints where ephemeralAgent is not sent in the submission payload - Allow Tab to close the popover when matches is empty instead of trapping keyboard focus - Add endpoint gating tests for $ on both assistants endpoints * fix: reset skills popover on assistants switch + paginate skills query - Close $ skills popover when endpoint switches to assistants or azureAssistants, mirroring the existing + command reset - Switch SkillsCommand from a single-page list query to the cursor-paginated useSkillsInfiniteQuery and auto-fetch all pages so client-side search covers the full catalog instead of only the first 100 entries - Show the spinner during background page fetches and suppress the empty-state copy until paging completes - Add test for popover reset on endpoint switch * fix: prevent currency hijack and form submit on $ command - Reject the $ trigger when fast-typed text after $ does not start with a lowercase letter, so currency input like $100 or $5.99 no longer opens the skills popover and clears the textarea - preventDefault() on Enter when the popover has no matches so the surrounding chat form does not submit when the user dismisses the popover via Enter - Add tests for $100 and $5.99 currency inputs * fix: defer $ popover to second keystroke and stop pagination on errors - Defer opening the skills popover until a letter follows the $ character. Bare $ no longer triggers the popover, so starting a message with $100, $5.99, or $EUR is fully preserved (the textarea is not cleared by useInitPopoverInput on the first keystroke). $a, $skill, $my-skill still open as expected. - Add a sticky paginationBlockedRef circuit breaker on the auto fetchNextPage effect so a transient page request error cannot spin into an unbounded retry loop when isError flips back to false on the next attempt. - Update tests: bare $ no longer triggers, $a does, currency cases remain blocked. * feat: scaffold structured manual-skill channel for follow-up PR Add a per-conversation pendingManualSkillsByConvoId atom family that SkillsCommand appends to on selection. This is the writer half of the structured channel that will let a follow-up PR deterministically prime SKILL.md as a meta user message before the LLM turn (mirroring Claude Code's `/skill` invocation), so the backend never has to regex-parse `$name` out of user text. The submit pipeline does not yet read this atom; the textual `$skill-name ` insertion remains the authoritative signal until the follow-up wires the read + reset on submit. Reset is already wired into useClearStates so the atom does not leak across conversation switches. * test: cover SkillsCommand selection-flow contract Add a component test that locks in the contract the follow-up manualSkills PR has to honor when a user picks a skill in the $ popover: - Pushes the skill name onto pendingManualSkillsByConvoId (the per-conversation structured channel), with dedup - Flips ephemeralAgent.skills to true via the callback-form setter - Inserts $skill-name into the textarea as cosmetic confirmation - Closes the popover via setShowSkillsPopover(false) - Renders nothing when the popover atom is false Mocks recoil setters and the skills query so the test exercises the real component logic without spinning up the full provider stack. * revert: drop $ defer-until-letter guard for sibling consistency Bring $ in line with @, /, and +: open the popover on the bare trigger character. The earlier defer guard kept currency input like \$100 from clobbering the textarea, but it broke the natural "type \$ to browse skills" UX and was inconsistent with the other trigger chars, none of which gate on the follow-up character (/path/to/file, @username, +1 all clear the textarea the same way). The currency hijack remains recoverable via Escape. Drop the corresponding paste-protection tests for \$100 and \$5.99, restore the bare-\$ trigger test.	2026-04-25 04:02:00 -04:00
Danny Avila	3b820415ad	🎭 feat: Custom UI Renderers for Skill Tool Calls (#12684) ... * feat: Custom UI renderers for skill, read_file, and bash_tool Add specialized tool call components for the three skill tools, replacing the generic ToolCall fallback with contextual UI. * fix: Address review findings for skill tool UI renderers - Fix Codex P2: read skillName (camelCase) matching agent pipeline - Fix Codex P2: remove error regex from ReadFileCall to avoid false positives on normal file content containing "Error:" tokens - Extract useToolCallState hook to eliminate ~60% boilerplate duplication across SkillCall, ReadFileCall, and BashCall - Extract parseJsonField utility with consistent escaped-char-aware regex fallback, shared by all three components - Gate SkillCall bordered card on hasOutput to prevent empty card when expanded before output arrives - Skip highlightAuto for plaintext lang to avoid expensive auto-detection on files with unknown extensions - Expand LANG_MAP with php, cs, kt, swift, scss, less, lua, r; add FILENAME_MAP for Makefile and Dockerfile - Export langFromPath for testability - Add unit tests for parseJsonField, langFromPath, and ToolIcon skill type branches * refactor: Redesign BashCall as minimal terminal widget Replace the ExecuteCode-clone pattern with a purpose-built terminal UI: $ prompt prefix, dark background command zone, icon-only copy button, and raw monospace output. Drops useLazyHighlight, CodeWindowHeader, Stdout, and the "Output" label in favor of a cleaner two-zone layout that feels native to the terminal. * fix: parseJsonField unescape ordering and ReadFileCall empty card Replace the sequential .replace() chain in parseJsonField's regex fallback with a single-pass /\(.)/g replacement. The old chain processed \n before \, so \n (JSON-escaped literal backslash + n) was incorrectly decoded as a newline instead of \n. Gate ReadFileCall's bordered card on hasOutput (matching SkillCall's pattern) so the card does not render as an empty rounded box during streaming before output arrives. Add regression tests for \n decoding and unknown escape sequences. * fix: Followup review fixes - Refactor ExecuteCode to use shared useToolCallState hook, eliminating the last copy of the inline state machine - Escape regex metacharacters in parseJsonField to prevent injection from field names containing ., +, (, etc. - Fix contradictory test description in langFromPath tests * fix: Surface tool failure state in skill tool renderers Add error detection to useToolCallState via the shared isError check so tool calls that complete with an error prefix show a "failed" suffix instead of a success label. Prevents misleading users when read_file, skill, or bash_tool returns an error (e.g. file not found, skill not accessible). Matches the error handling pattern already used by the generic ToolCall component. * feat: Add bash syntax highlighting to BashCall command zone Reuse the shared useLazyHighlight singleton (already loaded by ReadFileCall and ExecuteCode) to highlight the command with bash grammar. Falls back to plain text while lowlight is loading. * fix: Align BashCall scrollbar to span full card width Move max-h/overflow-auto from the inner pre to the outer container so the scrollbar spans the full width like the output zone. Float the copy button with sticky positioning so it stays visible while scrolling long commands. * feat: Use GNU Bash icon for bash_tool progress header and ToolIcon Replace the generic SquareTerminal lucide icon with the GNU Bash logo (already in the project via LangIcon/langIconPaths) for both the BashCall progress header and the ToolIcon stacked icon mapping. * fix: Render raw content while highlighter loads, preserve command text on copy - ReadFileCall: fall back to raw output when useLazyHighlight returns null, preventing a blank code panel on first render before lowlight finishes its dynamic import - BashCall: drop .trim() from the copy handler so the clipboard receives exactly what's displayed (WYSIWYG copy) * fix: Alphabetize new translation keys within en/translation.json Relocate read_file, skill_finished, and skill_running into their correct alphabetical positions within the overall key list. * fix: Surface error state in ExecuteCode, fix BashCall import order - ExecuteCode now uses hasError from useToolCallState to show the "failed" suffix on failed code executions, matching the three new renderers - Reorder BashCall local imports to longest-to-shortest per project style	2026-04-25 04:02:00 -04:00
Danny Avila	64ec5f18b8	⚙️ feat: Skill runtime integration: catalog, tools, execution, file priming (#12649) ... * feat: Skill runtime integration — catalog injection, tool registration, execute handler Wires the @librechat/agents SkillTool primitive into LibreChat's agent runtime: **Enums:** - Add `skills` to AgentCapabilities + defaultAgentCapabilities **Data layer:** - Add `getSkillByName(name, accessibleIds)` — compound query that combines name lookup + ACL check in one findOne **Agent initialization (packages/api/src/agents/initialize.ts):** - Accept `accessibleSkillIds` param and `listSkillsByAccess` db method - Query accessible skills, format catalog via `formatSkillCatalog()`, append to `additional_instructions` (appears in agent system prompt) - Register `SkillToolDefinition` + `createSkillTool()` when catalog is non-empty (tool appears in model's tool list) - Store `accessibleSkillIds` and `skillCount` on InitializedAgent **Execute handler (packages/api/src/agents/handlers.ts):** - Add `getSkillByName` to `ToolExecuteOptions` - `handleSkillToolCall()` intercepts `Constants.SKILL_TOOL`: extracts skillName, loads body from DB with ACL check, substitutes $ARGUMENTS, returns ToolExecuteResult with injectedMessages (skill body as isMeta user message) **Caller wiring:** - initialize.js: query skill IDs via findAccessibleResources, pass to initializeAgent + store on agentToolContexts, add getSkillByName to toolExecuteOptions, pass accessibleSkillIds through loadTools configurable - openai.js + responses.js: same pattern for their flows Requires @librechat/agents >= 3.1.65 (PR #91 exports). * feat: Skills toggle in tools menu + backend capability gating Frontend: - Add skills?: boolean to TEphemeralAgent type - Add LAST_SKILLS_TOGGLE_ to LocalStorageKeys for persistence - Add skillsEnabled to useAgentCapabilities hook - Add skills useToolToggle to BadgeRowContext with localStorage init - New Skills.tsx badge component (Scroll icon, cyan theme, permission-gated via PermissionTypes.SKILLS) - Add skills entry to ToolsDropdown with toggle + pin - Render Skills badge in BadgeRow ephemeral section Backend: - Extract injectSkillCatalog() into packages/api/src/agents/skills.ts (reduces initializeAgent module size, reusable helper) - initializeAgent delegates to helper instead of inline block - Capability-gate the findAccessibleResources query: - Agents endpoint: checks AgentCapabilities.skills in admin config - OpenAI/Responses controllers: checks ephemeralAgent.skills toggle - ACL query runs once per run, result shared across all agents * refactor: remove createSkillTool() instance from injectSkillCatalog SkillTool is event-driven only. The tool definition in toolDefinitions is sufficient for the LLM to see the tool schema. No tool instance is needed since the host handler intercepts via ON_TOOL_EXECUTE before tool.invoke() is ever called. Removes tools from InjectSkillCatalogParams/Result, drops the createSkillTool import. * feat: skill file priming, bash tool, and invoked skills state Multi-file skill support: - New primeSkillFiles() helper (packages/api/src/agents/skillFiles.ts) uploads skill files + SKILL.md body to code execution environment - handleSkillToolCall primes files on invocation when skill.fileCount > 0, returns session info as artifact so ToolNode stores the session - Skill-primed files available to subsequent bash/code tool calls Bash tool auto-registration: - BashExecutionToolDefinition added alongside SkillToolDefinition when skills are enabled, giving the model a bash tool for running scripts Conversation state: - Add invokedSkillIds field to conversation schema (Mongoose + Zod) - handleSkillToolCall updates conversation with $addToSet on success - Enables re-priming skill files on subsequent runs (future) Dependency wiring: - Pass listSkillFiles, getStrategyFunctions, uploadCodeEnvFile, updateConversation through ToolExecuteOptions - Pass req and codeApiKey through mergedConfigurable - All three controller entry points wired (initialize.js, openai.js, responses.js) * fix: load bash_tool instance in loadToolsForExecution, remove file listing - Add createBashExecutionTool to loadToolsForExecution alongside PTC/ToolSearch pattern: loads CODE_API_KEY, creates bash tool instance on demand - Add BASH_TOOL and SKILL_TOOL to specialToolNames set so they don't go through the generic loadTools path (bash is created here, skill is intercepted in handler before tool.invoke) - Remove file name listing from skill content text — it's the skill author's responsibility to disclose files in SKILL.md, not the framework * feat: batch upload for skill files, replace sequential uploads - Add batchUploadCodeEnvFiles() to crud.js: single POST to /upload/batch with all files in one multipart request, returns shared session_id - Rewrite primeSkillFiles to collect all streams (SKILL.md + bundled files) then do one batch upload instead of N sequential uploads - Replace uploadCodeEnvFile with batchUploadCodeEnvFiles across all callers (handlers.ts, initialize.js, openai.js, responses.js) * refactor: remove invokedSkillIds from conversation schema Skills aren't re-loaded between runs, so conversation-level state for invoked skills doesn't help. Skill state will live on messages instead (like tool_search discoveredTools and summaries), enabling in-place re-injection on follow-up runs. Removes invokedSkillIds from: convo Mongoose schema, IConversation interface, Zod schema, ToolExecuteOptions.updateConversation, and all three caller wiring points. * feat: smart skill file re-priming with session freshness checking Schema: - Add codeEnvIdentifier field to ISkillFile (type + Mongoose schema) - Add updateSkillFileCodeEnvIds batch method (uses tenantSafeBulkWrite) - Export checkIfActive from Code/process.js Extraction: - Add extractInvokedSkillsFromHistory() to run.ts — scans message history for AIMessage tool_calls where name === 'skill', extracts skillName args. Follows same pattern as extractDiscoveredToolsFromHistory. Smart re-priming in primeSkillFiles: - Before batch uploading, checks if existing codeEnvIdentifiers are still active via getSessionInfo + checkIfActive (23h threshold) - If session is still active, returns cached references (zero uploads) - If stale or missing, batch-uploads everything and persists new identifiers on SkillFile documents (fire-and-forget) - Single session check covers all files (batch shares one session_id) Wiring: - Pass getSessionInfo, checkIfActive, updateSkillFileCodeEnvIds through ToolExecuteOptions and all three controller entry points * feat: wire skill file re-priming at run start via initialSessions Flow: 1. initialize.js creates primeInvokedSkills callback with all deps 2. client.js calls it with message history before createRun 3. extractInvokedSkillsFromHistory scans for skill tool calls 4. For each invoked skill with files, primeSkillFiles uploads/checks 5. Returns initialSessions map passed to createRun 6. createRun passes initialSessions to Run.create (via RunConfig) 7. Run constructor seeds Graph.sessions, making skill files available to subsequent bash/code tool calls via ToolNode session injection Requires @librechat/agents with initialSessions on RunConfig (PR #94). * refactor: use CODE_EXECUTION_TOOLS set for code tool checks Import CODE_EXECUTION_TOOLS from @librechat/agents and replace inline constant checks in handlers.ts and callbacks.js. Fixes missing bash tool coverage in the session context injection (handlers.ts) and code output processing (callbacks.js). * refactor: move primeInvokedSkills to packages/api, add skill body re-injection Moves primeInvokedSkills from an inline closure in initialize.js (with dynamic requires) to a proper exported function in packages/api skillFiles.ts with explicit typed dependencies. Key changes: - primeInvokedSkills now returns both initialSessions (for file priming) AND injectedMessages (skill bodies for context continuity) - createRun accepts invokedSkillMessages and appends skill bodies to systemContent so the model retains skill instructions across runs - initialize.js calls the packaged function with all deps passed explicitly - client.js passes both initialSessions and injectedMessages to createRun * fix: move dynamic requires to top-level module imports Move primeInvokedSkills, getStrategyFunctions, batchUploadCodeEnvFiles, getSessionInfo, and checkIfActive from inline requires to top-level module requires where they belong. * refactor: skill body reconstruction via formatAgentMessages, not systemContent Replaces the lazy systemContent approach with proper message-level reconstruction: SDK (formatAgentMessages): - New invokedSkillBodies param (Map<string, string>) - Reconstructs HumanMessages after skill ToolMessages at the correct position in the message sequence, matching where ToolNode originally injected them LibreChat: - extractInvokedSkillsFromPayload replaces extractInvokedSkillsFromHistory (works with raw TPayload before formatAgentMessages, not BaseMessage[]) - primeInvokedSkills now takes payload instead of messages, returns skillBodies Map instead of injectedMessages - client.js calls primeInvokedSkills BEFORE formatAgentMessages, passes skillBodies through as the 4th param - Removed invokedSkillMessages from createRun (no more systemContent hack) - Single-pass: skill detection happens inside formatAgentMessages' existing tool_call processing loop, zero extra message iterations * refactor: rename skillBodies to skills for consistency with SDK param * refactor: move auth loading into primeInvokedSkills, pass loadAuthValues as dep The payload/accessibleSkillIds guard and CODE_API_KEY loading now live inside primeInvokedSkills (packages/api) rather than in the CJS caller. initialize.js passes loadAuthValues as a dependency and the callback is only created when skillsCapabilityEnabled. * feat: ReadFile tool + conditional bash registration + skill path namespacing ReadFile tool (read_file): - General-purpose file reader, event-driven (ON_TOOL_EXECUTE) - Schema: { file_path: string } — "{skillName}/{path}" convention - handleReadFileCall: resolves skill name from path, ACL check, reads from DB cache or storage, binary detection, size limits (256KB), lazy caching (512KB), line numbers in output - SKILL.md special case: reads skill.body directly - Dispatched alongside SKILL_TOOL in createToolExecuteHandler - Added to specialToolNames in ToolService Conditional tool registration: - ReadFile + SkillTool: always registered when skills enabled - BashTool: only registered when codeEnvAvailable === true - codeEnvAvailable passed through InitializeAgentParams from caller Skill file path namespacing: - primeSkillFiles now uploads as "{skillName}/SKILL.md" and "{skillName}/{relativePath}" instead of flat names - Prevents file collisions when multiple skills are invoked Wiring: - getSkillFileByPath + updateSkillFileContent passed through ToolExecuteOptions in all three callers * feat: return images/PDFs as artifacts from read_file, tighten caching Binary artifact support: - Images (png, jpeg, gif, webp) returned as base64 in artifact.content with type: 'image_url', processed by existing callback attachment flow - PDFs returned as base64 artifact similarly - Binary size limit: 10MB (MAX_BINARY_BYTES) - Other binary files still return metadata + bash fallback Caching: - Text cached only on first read (file.content == null check) - Binary flag cached only on first detection (file.isBinary == null) - Skill files are immutable; no redundant cache writes Registration: - ReadFileToolDefinition now includes responseFormat: 'content_and_artifact' * chore: update @librechat/agents to version 3.1.66-dev.0 and add peer dependencies in package-lock.json and package.json files * fix: resolve review findings #1,#2,#4,#5,#6,#10,#13 Critical: - #1: primeInvokedSkills now accumulates files across all skills into one session entry instead of overwriting. Parallel processing via Promise.allSettled. - #2: codeEnvAvailable now computed and passed in openai.js and responses.js (was missing, bash tool never registered in those flows) Major: - #4: relativePath in updateSkillFileCodeEnvIds now strips the {skillName}/ prefix to match SkillFile documents. SKILL.md filter uses endsWith instead of exact match. - #5: File priming guarded on apiKey being non-empty (skip when not configured instead of failing with auth error) - #6: Skills processed in parallel via Promise.allSettled instead of sequential for-of loop Minor: - #10: Use top-level imports in initialize.js instead of inline requires - #13: Log warning when skill catalog reaches the 100-skill limit * fix: resolve followup review findings N1,N2,N4 N1 (CRITICAL): Wire skill deps into responses.js non-streaming path. Was completely missing getSkillByName, file strategy functions, etc. N2 (MAJOR): Single batch upload for ALL skills' files. Resolves skills in parallel (Phase 1), then collects all file streams across skills and does ONE batchUploadCodeEnvFiles call (Phase 2). All files share one session_id, eliminating cross-session isolation issues. N4 (MINOR): Move inline require() to top-level in openai.js and responses.js, consistent with initialize.js. * fix: add mocks for new file strategy imports in controller tests * fix: restore session freshness check, parallelize file lookups, add warnings R1: Re-add session freshness check before batch upload. Checks any existing codeEnvIdentifier via getSessionInfo + checkIfActive. If the session is still active (23h window), returns cached file references with zero re-uploads. R2: listSkillFiles calls parallelized via Promise.all (were sequential in the for-of loop). R3: Log warning when skill record lookup fails during identifier persistence (was a silent empty-string fallback). * fix: guard freshness cache on single-session consistency * fix: multi-session freshness check (code env handles mixed sessions natively) The code execution environment fetches each file by its own {session_id, fileId} pair independently — no single-session requirement. Removed the sessionIds.size === 1 guard. Now checks ALL distinct sessions for freshness. If every session is still active (23h window), returns cached references with per-file session_ids preserved. If any session expired, falls through to re-upload everything in a single batch. * perf: parallelize session freshness checks via Promise.all * fix: add optional chaining for session info retrieval in primeInvokedSkills Updated the primeInvokedSkills function to use optional chaining for getSessionInfo and checkIfActive methods, ensuring safer access and preventing potential runtime errors when these methods are undefined. * fix: address review findings #1-#9 + Codex P1/P2 + session probe Critical: - #1/Codex P1: Add codeApiKey loading to openai.js and responses.js loadTools configurable (was missing, file priming broken in 2/3 paths) - Codex P1: Fix cached file name prefix in primeSkillFiles cache path (was sf.relativePath, now ${skill.name}/${sf.relativePath}) Major: - Codex P2: Honor ephemeral skills toggle in agents endpoint (check ephemeralAgent?.skills !== false alongside admin capability) - #4: Early size check using file.bytes from DB before streaming (prevents full-file buffer for oversized files) Minor: - #5: Replace Record<string, any> with Record<string, boolean | string> - #6: Localize Pin/Unpin aria-labels with com_ui_pin/com_ui_unpin - #8: Parallelize stream acquisition in primeSkillFiles via Promise.allSettled - #9: Log warning for partial batch upload failures with filenames Performance: - Session probe optimization: getSessionInfo now hits per-object endpoint (GET /sessions/{sid}/objects/{fid}) instead of listing entire session (GET /files/{sid}?detail=summary). O(1) stat vs O(N) list + linear scan. * refactor: extract shared skill wiring helper + add unit tests DRY (#3): - New skillDeps.js exports getSkillToolDeps() with all 9 skill-related deps (getSkillByName, listSkillFiles, getStrategyFunctions, etc.) - Replaces 5 identical copy-paste blocks across initialize.js, openai.js, responses.js (streaming + non-streaming paths) - One place to maintain when skill deps change Tests (#2): - 8 unit tests for extractInvokedSkillsFromPayload covering: string args, object args, missing skill tool_calls, non-assistant messages, malformed JSON, empty skillName, empty payload, dedup * fix: remove @jest/globals import, use global jest env * fix: resolve round 2 review findings R2-1 through R2-7 R2-1 (toggle semantics): openai.js + responses.js now check admin capability (AgentCapabilities.skills) alongside ephemeral toggle. Aligns with initialize.js. R2-2 (swallowed error): primeInvokedSkills now logs updateSkillFileCodeEnvIds failures (was .catch(() => {})) R2-4 (test cast): Record<string, string> → Record<string, unknown> R2-5 (DRY regression): Extract enrichWithSkillConfigurable() into skillDeps.js. Replaces 4 identical loadAuthValues blocks. Each loadTools callback is now a one-liner. JSDoc added (R2-6). R2-7 (sequential streams): primeInvokedSkills now uses Promise.allSettled for parallel stream acquisition. * fix: require explicit skills toggle + treat partial cache as miss - initialize.js: change ephemeralSkillsToggle !== false to === true (unset toggle no longer enables skills) - primeSkillFiles cache: require ALL files to have codeEnvIdentifier before using cache (partial persistence = cache miss = re-upload) - primeInvokedSkills cache: same check (allFilesWithIds.length must equal total file count) * fix: pass entity_id=skillId on batch upload, eliminates per-user cache thrashing primeSkillFiles now passes entity_id: skill._id.toString() to batchUploadCodeEnvFiles. This scopes the code env session to the skill, not the user. All users sharing a skill share the same uploaded files — no more cache thrashing from overwriting each other's codeEnvIdentifier. The stored codeEnvIdentifier now includes ?entity_id= suffix so freshness checks pass the entity_id through to the per-object stat endpoint. Both primeSkillFiles and primeInvokedSkills store consistent identifier formats. * fix: pass entity_id on multi-skill batch upload, consistent identifier format * Revert "fix: pass entity_id on multi-skill batch upload, consistent identifier format" This reverts commit c85ce2161e. * refactor: per-skill upload in primeInvokedSkills, eliminate multi-skill batch Replace the monolithic multi-skill batch upload with per-skill primeSkillFiles calls. Each skill gets its own session with entity_id=skillId, ensuring: - Correct session auth (entity_id matches on freshness checks) - Per-skill freshness caching (only expired skills re-upload) - Shared skill sessions work across users (same entity_id=skillId) - Code env handles mixed session_ids natively The big batch block (stream collection, single upload, identifier mapping) is replaced by a simple loop over primeSkillFiles, which already handles freshness caching, batch upload, and identifier persistence per-skill. * fix: resolve review findings #1,#3-5,#7,#9-11 Critical: - #1: Strip ?entity_id= query string before splitting codeEnvIdentifier into session_id/fileId (was corrupting cached file IDs in 4 locations) Major: - #4: Parallelize per-skill primeSkillFiles via Promise.allSettled - #5: Add logger.warn to all empty .catch(() => {}) on cache writes Minor: - #7: Add logger.debug to enrichWithSkillConfigurable catch block - #9: Use error instanceof Error guard in batchUploadCodeEnvFiles - #10: Move enrichWithSkillConfigurable to TypeScript in packages/api (skillConfigurable.ts), skillDeps.js wraps with loadAuthValues dep - #11: Reduce MAX_BINARY_BYTES from 10MB to 5MB (~11.5MB peak with b64) * fix: forward entity_id in session probe + always register bash tool Codex P2 (entity_id in probe): getSessionInfo now preserves and forwards query params (including entity_id) to the per-object stat endpoint. Without this, identifiers stored as ...?entity_id=... would fail auth checks because the entity_id scope was dropped. Codex P2 (bash tool availability): Remove codeEnvAvailable gate from injectSkillCatalog. Bash tool definition is now always registered when skills are enabled. Actual tool instance creation still happens at execution time in loadToolsForExecution (which loads per-user credentials). This ensures users with per-user CODE_API_KEY get bash without requiring a global env var at init time. Removes codeEnvAvailable from InjectSkillCatalogParams, InitializeAgentParams, and all three controller entry points. * fix: add debug logging to primeInvokedSkills catch, rename export alias * fix: stub bash tool when no key + remove PDF artifact path Codex P1 (bash tool): When CODE_API_KEY is unavailable, create a stub tool that returns "Code execution is not available. Use read_file instead." This prevents "tool not found" errors from the model repeatedly calling bash_tool in no-code-env deployments while still registering the definition for per-user credential users. Codex P2 (PDF artifacts): Remove PDF image_url artifact path. The host artifact pipeline processes image_url via saveBase64Image which fails for PDFs. PDFs now fall through to the generic binary handler ("Use bash to process"). TODO comment for future document artifact support. Also: isImageOrPdf → isImage in early size checks (PDFs are no longer treated as artifact candidates). * fix: remove dead PDF_MIME constant, hoist skillToolDeps, document session_id - #7: Remove unused PDF_MIME constant (dead code after PDF artifact removal) - #11: Hoist skillToolDeps to module-level constant (avoid per-call allocation) - #6: Document that CodeSessionContext.session_id is a representative value; ToolNode uses per-file session_id from the files array * fix: call toolEndCallback for skill/read_file artifacts + clear codeEnvIdentifier on re-upload Codex P1 (toolEndCallback bypass): skill and read_file handler branches returned early, bypassing the toolEndCallback that processes artifacts (image attachments). Now calls toolEndCallback when the result has an artifact, using the same metadata pattern as the normal tool.invoke path. Codex P1 (stale identifiers): upsertSkillFile now $unset's codeEnvIdentifier alongside content and isBinary when a file is re-uploaded. Prevents the freshness cache from returning references to old file content after a skill file is replaced. * fix: add session_id comment at cached path, rename skillResult to handlerResult * fix: return content_and_artifact from bash stub so result.content is populated * fix: deterministic skill lookup, dedup warning, and multi-session freshness check - getSkillByName: add sort({updatedAt:-1}) so name collisions resolve deterministically to the most recently updated skill - injectSkillCatalog: warn when multiple accessible skills share a name - primeSkillFiles: check ALL distinct sessions for freshness, not just the first file's session, preventing stale refs after partial bulkWrite * refactor: update icon import in Skills component - Replaced the Scroll icon with ScrollText in the Skills component for improved clarity and consistency in the UI. * fix: SKILL.md cache parity, gate bash_tool on code env, fix read_file too-large message - primeSkillFiles: filter SKILL.md from returned files array on fresh upload so cached and non-cached paths return identical file sets (SKILL.md is still on disk in the session for bash access) - injectSkillCatalog: only register bash_tool when codeEnvAvailable is true; thread the flag from all three CJS callers via execute_code capability check - handleReadFileCall: tell the model to invoke the skill first before suggesting /mnt/data paths for oversized files * fix: use EnvVar constant, deduplicate auth lookup, validate batch upload, stream byte limit - Replace hardcoded 'LIBRECHAT_CODE_API_KEY' with EnvVar.CODE_API_KEY in skillConfigurable.ts and skillFiles.ts - Resolve code API key once at run start in initialize.js and pass to both primeInvokedSkills and enrichWithSkillConfigurable via optional preResolvedCodeApiKey param, eliminating redundant loadAuthValues calls - Add response structure validation in batchUploadCodeEnvFiles before accessing session_id/files to surface unexpected responses early - Add streaming byte counter in handleReadFileCall that aborts and destroys the stream when accumulated bytes exceed MAX_BINARY_BYTES, preventing full file buffering when DB metadata is inaccurate * refactor: update icon import in ToolsDropdown component - Replaced the Scroll icon with ScrollText in the ToolsDropdown component for improved clarity and consistency in the UI. * fix: partial upload failure detection, EnvVar in initialize.js, declaration ordering - primeSkillFiles: return null (failure) when batch upload partially succeeds — missing bundled files would cause runtime bash/read failures with missing paths in code env - initialize.js: replace hardcoded 'LIBRECHAT_CODE_API_KEY' with EnvVar.CODE_API_KEY imported from @librechat/agents - initialize.js: move enabledCapabilities, accessibleSkillIds, and codeApiKey declarations before the toolExecuteOptions closure that references them (eliminates reliance on temporal dead zone hoisting)	2026-04-25 04:02:00 -04:00
Marco Beretta	f6ee2ea0ee	📜 feat: Skills UI + Initial E2E CRUD / Sharing (#12580) ... * 🎨 feat: Skills UI — Create/Edit/Share/List with Conditional File Tree First-pass UI on top of the CRUD API scaffolding (#12613). Ships the full user-facing flow for inline, single-SKILL.md skills and leaves a clean drop-in for phase-2 multi-file support. - Create a skill from /skills/new with name (kebab-case, validated), description, and SKILL.md body — wired to the real `useCreateSkillMutation` and `TCreateSkill` payload. - List skills in a sidebar (SkillsSidePanel) via `useListSkillsQuery` with live search filtering. - Edit any skill the caller has EDIT permission on — `useUpdateSkillMutation` passes `expectedVersion` for optimistic concurrency and surfaces 409 conflicts as a warning toast + cache refetch. - Non-blocking `TSkillWarning[]` (e.g. "description too short") are shown inline above the form after a successful create/patch. - Read-only mode when the current user lacks EDIT — the form still renders but inputs are marked `readOnly` and the save/reset buttons are hidden. - Share via ACL using the existing `GenericGrantAccessDialog` — the `ShareSkill` button is gated on the SHARE permission. - Delete with confirmation, driven by `useDeleteSkillMutation({ id })`. - Conditional file tree: only rendered when `useListSkillFilesQuery` returns > 0 files. The tree groups flat `relativePath` strings into a nested view (no `react-arborist` dependency) and supports per-file deletion via `useDeleteSkillFileMutation`. Upload is intentionally deferred — the backend stubs it at 501 in phase 1. - New routes: `/skills`, `/skills/new`, `/skills/:skillId`. - Sidebar accordion (`SkillsAccordion` wrapping `SkillsSidePanel`) added to `useSideNavLinks` gated on `PermissionTypes.SKILLS` USE. The initial UI branch (#12580) shipped a lot of exploration code on top of a now-superseded placeholder backend. Kept as complementary: the `Skills/` component tree, translation keys, role descriptions, `PublicSharingToggle` SKILL mapping, `resources.ts` SKILL config, `useCanSharePublic` SKILL mapping, and `data-provider/roles.ts` `useUpdateSkillPermissionsMutation`. Deferred out of this first pass: - Skill favorites (`useSkillFavorites`, `getSkillFavorites` endpoint) — the backend route doesn't exist yet; saving for a follow-up. - AgentConfig `SkillSelectDialog` integration — the UI branch had this gated behind `false &&`; rolled back with the config. - `InvocationMode` / `CategorySelector` / `parseSkillMd` / tree-node mutations — not in the Anthropic skill spec and not in the CRUD API. - `react-arborist` dependency — replaced with a hand-rolled recursive tree built from flat `TSkillFile[]`. - 38 data-schemas skill model tests: pass - 25 api skill route tests: pass - 16 user-controller cleanup tests: pass * 🔐 feat: Default-On Skills in Interface Config and Role Seeder The skills accordion was registered in the side nav gated on `PermissionTypes.SKILLS` USE, but no one was actually seeding that permission on startup, so a fresh install had the USER role with zero skill permissions and the accordion never rendered. Fixes three gaps: 1. `interfaceSchema` in data-provider's `config.ts` had no `skills` field at all. Added it alongside the existing agents/prompts shape (boolean | { use, create, share, public }) and a default of `{ use: true, create: true, share: false, public: false }`. 2. `loadDefaultInterface` in data-schemas passed every interface key through to the loaded config EXCEPT `skills`. Added the one-line passthrough so `appConfig.interfaceConfig.skills` is actually populated on boot. 3. `updateInterfacePermissions` in packages/api/src/app/permissions.ts seeds role permissions from the interface config on every restart. Added: - `SKILLS` case to `hasExplicitConfig` - `skillsDefaultUse/Create/Share/Public` extraction (mirrors prompts/agents) - `PermissionTypes.SKILLS` block in `allPermissions` that falls through config → roleDefaults → schema default, same pattern as AGENTS and PROMPTS - `SKILLS` entry in the share-backfill array so that pre-existing SKILL role docs missing SHARE/SHARE_PUBLIC get them filled on the next restart Test expectations updated: seven `expectedPermissionsFor(User|Admin)` blocks in `permissions.spec.ts` now include SKILLS, matching the role-default values (USER: use+create true, share/public false; ADMIN: all true). Result: on a fresh install, a regular USER gets skill USE/CREATE and the "Skills" accordion shows up in the chat side panel without any yaml config. Admins can lock it down per role or per tenant via `interface.skills` in librechat.yaml. Tests: - 34 packages/api permissions.spec.ts: pass - 151 packages/api app tests: pass - 38 data-schemas skill.spec.ts: pass - 928 data-provider tests: pass - 25 api skills.test.js: pass * ♻️ fix: Resolve Skills UI Review Findings Addresses the 13 findings from the PR review against the prior commit. 1. **canEdit consistency** — extracted `useSkillPermissions(skill)` as the single source of truth for owner/admin/ACL gating. `SkillsView`, `SkillForm`, `ShareSkill` all consume it; `SkillFileTree`'s per-file delete button now honors admin + EDIT-bit permissions instead of just ownership. Unit tests cover owner, admin, editor-ACL, viewer-ACL, owner-ACL, loading, and undefined-skill cases. 2. **Disabled submit buttons** — create/edit form submit buttons now set native `disabled` (not just `aria-disabled`) during `isLoading`. `onSubmit` also guards with an early return when the mutation is still in-flight so a duplicate enter-key submit can't create two skills. 3. **Wrong maxLength error message** — description/name `maxLength` rules no longer re-use `com_ui_skill_*_required`. Added dedicated `com_ui_skill_name_too_long` and `com_ui_skill_description_too_long` keys with the literal limit interpolated (`{{0}}`). 4. **Search debouncing** — `SkillsSidePanel` now threads the filter input through the existing `useDebounce` hook (250ms) so typing "skills" no longer fires six separate list queries. 5. **Frontend test coverage** — added: - `tree.test.ts` (9 tests) covering `buildTree` / `nodeKey` edge cases: empty input, single root file, multiple roots, nested folders, deeply-nested trees, lexicographic sort, empty paths, stable keys - `useSkillPermissions.test.ts` (7 tests) covering every precedence branch (owner / admin / EDIT / VIEW / owner-ACL / loading / undef) Form integration tests proved flaky against react-hook-form's async `isValid` with our jest-dom mock setup; deferred to a follow-up PR with a proper `@librechat/client` test harness. 6. **Shared `SKILL_NAME_PATTERN`** — promoted the regex plus the four length constants (`SKILL_NAME_MAX_LENGTH`, `SKILL_DESCRIPTION_MAX_LENGTH`, `SKILL_DESCRIPTION_SHORT_THRESHOLD`, `SKILL_DISPLAY_TITLE_MAX_LENGTH`, `SKILL_BODY_MAX_LENGTH`) out of `packages/data-schemas/src/methods/skill.ts` and into `packages/data-provider/src/types/skills.ts`. The data-schemas module now aliases the shared exports so the backend validator and the frontend form share one source of truth. Also fixed a latent bug: the client regex was stricter than the backend (`^[a-z0-9]+(?:-[a-z0-9]+)*$` vs. the real `^[a-z0-9][a-z0-9-]*$`), which would have rejected valid names like `foo--bar` client-side. 7. **Removed hardcoded "Claude"** — replaced `com_ui_skill_description_help` ("Claude uses this to...") with a new `com_ui_skill_create_subtitle` for the form header and `com_ui_skill_description_field_hint` ("This is what the model reads to decide...") for the inline hint. LibreChat is LLM-agnostic; the old copy misled GPT/Gemini users. 8. **Lifted tree mutation hook** — `useDeleteSkillFileMutation` is now instantiated once in `SkillFileTree` (not per `TreeRow`). A `TreeContext` provides `onDeleteFile` + `isDeleting` + `canEdit` to rows. A 60-node tree used to instantiate 60 mutation hooks; it now instantiates one. 9. **List O(n) re-render** — `SkillListItem` no longer reads `useParams()` directly. `SkillList` reads the active id once and passes `isActive` as a prop, so navigation only re-renders the two items whose `isActive` flipped (memo'd), not all N items. 10. **Deduped help text** — the field-level hint and form-level subtitle now use different translation keys with distinct copy instead of showing the same sentence twice on the same page. 11. **Removed ineffective `useCallback`** — `DeleteSkill.handleDelete`, `CreateSkillForm.onSubmit` / `.handleCancel`, `SkillForm.onSubmit`, and `SkillFileTree.handleDeleteFile` all wrapped closures around React Query `mutation` refs, whose identities change every render. Their dep arrays invalidated every render, making the memo a no-op with extra overhead. `SkillFileTree` now destructures the stable `mutate` function and inlines the arrow inside the memoized `contextValue` — one stable reference per deps change. 12. **Import order** — fixed shortest→longest package ordering and longest→shortest local ordering across all touched skill files per AGENTS.md. `react` always first where imported. 13. **Memoization principle** — documented the rule with inline comments: `memo` on components that appear in repeated contexts (`TreeRow`, `SkillListItem`) or as children of frequently-re-rendering parents (`ShareSkill` / `DeleteSkill` under `SkillForm`'s per-keystroke form-state updates). Removed `memo` from `SkillFileTree` since its parent `SkillDetailPanel` only re-renders on query-data changes. - 38 data-schemas skill.spec.ts - 34 packages/api permissions.spec.ts - 25 api skills.test.js - 16 client unit tests (9 buildTree + 7 useSkillPermissions) - All type-checks + eslint clean on touched files * 🧹 fix: Skills Duplication, Input Styling, Remove LLM-specific Copy Three UI fixes from an in-chat review pass: 1. **Sidebar duplication** — `SkillsView` was rendering its own `SkillsSidePanel` aside alongside the chat side panel's `SkillsAccordion`, so on `/skills` the user saw the skill list twice. Fixed by mirroring the `InlinePromptsView` pattern: the route content is now just the detail / create panel and the chat side panel is the sole list. Added `/skills → /skills/new` redirect and a `/skills/new` literal route so `useParams().skillId` is `undefined` for "new" (matches prompts). 2. **Name Input styling** — the big floating-label pattern used by prompts/agents for the primary name field was replaced with a conventional `<Label>` + `<Input>` above it, diverging from the rest of the app. Restored the prompts-style `text-2xl` input with the peer-focus animated label on both `CreateSkillForm` and `SkillForm`. Kept the conventional pattern for description and body since they're textareas. 3. **Remove LLM-specific copy from skill translations** — dropped `com_ui_skill_description_help` ("Claude uses this to...") and the transitional "This is what the model reads..." phrasing. Field hint is now a neutral "Be specific about when this skill should apply." and the create-page subtitle is a neutral "Author a new skill your agents can invoke." LibreChat is LLM-agnostic; baking product names into user-facing copy is wrong outside the `com_endpoint_anthropic_*` keys where the setting actually only applies to Claude models. Side-effect: the `SkillDetailView` wrapper in `SkillsView` now only renders the file-tree aside when the skill has > 0 files — same conditional-tree behavior as before, just scoped to this route instead of also trying to also render a list sidebar. - 16 client skill tests still pass - Type-check + eslint clean on touched files * 🎁 feat: Restore Skills UI from PR #12580 Brings back everything the original UI PR (#12580, commit da039917c) shipped that my earlier rebase dropped. Verbatim restores where possible; adapts the new hooks/types where the backend contract has shifted. **Scoped-out / gated-off (now restored as inert UI scaffolding):** - `hooks/useSkillFavorites.ts` + `utils/favoritesError.ts` + the `useGetSkillFavoritesQuery` / `useUpdateSkillFavoritesMutation` additions in `data-provider/Favorites.ts`. The backend route doesn't exist yet — the data-service functions resolve with empty arrays so the Star UI is a visual-only no-op until phase 2. - `dialogs/SkillSelectDialog.tsx` + the "Add Skills" section in `SidePanel/Agents/AgentConfig.tsx` (still gated behind the original `false &&`) + `skills?: string[]` on `AgentForm` / `Agent` / `AgentCreateParams` / `AgentUpdateParams` + the `skills: []` entry in `defaultAgentFormValues`. - `TUserFavorite.skillId` reserved on the shared favorites type. **Concept-is-gone / deleted-types (restored as UI-only types + stubs):** - `InvocationMode` enum and `TSkillNode`, `TSkillTreeResponse`, `TCreateSkillNodeRequest`, `TUpdateSkillNodeRequest` types in `packages/data-provider/src/types.ts`. UI-facing only; the backend flat `TSkillFile[]` contract is unchanged. - `TSkill.invocationMode?: InvocationMode` as an optional field. Forms read/write it in local state and deliberately drop it from the PATCH payload until the backend column lands. - `tree/SkillFileTree.tsx` (`react-arborist`-based), `SkillTreeNode.tsx`, `TreeToolbar.tsx`, `SkillFileEditor.tsx`, `SkillFilePreview.tsx` — full filesystem-style browser UI restored verbatim. - `data-provider/Skills/tree-queries.ts` + `tree-mutations.ts` hooks (`useGetSkillTreeQuery`, `useCreateSkillNodeMutation`, etc.). The `data-service` stubs them: `getSkillTree` returns `{ nodes: [] }`, `createSkillNode` / `updateSkillNode` / `updateSkillNodeContent` return synthetic node shapes, `deleteSkillNode` resolves void. Hooks compile and run; tree is empty until phase 2 wires a real backend. - `MutationKeys.createSkillNode` / `updateSkillNode` / `deleteSkillNode` / `updateSkillNodeContent` + `CreateSkillNodeBody` / `UpdateSkillNodeVariables` / `DeleteSkillNodeBody` / `UpdateSkillNodeContentVariables` types. - `QueryKeys.skillTree` / `skillNodeContent` / `skillFavorites` / `favorites` and the `skillTree()` endpoint helper. **Scope-simplified (restored with minimal adaptation):** - `display/SkillDetailHeader.tsx` + `display/SkillDetail.tsx`. Header now falls back to `InvocationMode.auto` when `skill.invocationMode` is undefined. - `forms/SkillContentEditor.tsx` — click-to-edit markdown preview toggle for the SKILL.md body field. Wired into both `CreateSkillForm` and `SkillForm` replacing the plain `<TextareaAutosize>`. (Needed `@ts-ignore` on `remarkPlugins` / `rehypePlugins` for the same `PluggableList` vs `Pluggable[]` shape drift `MarkdownLite.tsx` already works around.) - `forms/InvocationModePicker.tsx` + `forms/CategorySelector.tsx` — the auto/manual/both dropdown and the skill category selector. Wired into both forms inside a `FormProvider` so the Controller-based widgets can read `useFormContext`. `category` flows to the PATCH / POST payload as before; `invocationMode` is UI-only per the type note above. - `buttons/CreateSkillMenu.tsx` + `utils/parseSkillMd.ts` — dropdown with AI / Manual / Upload SKILL.md entries + the YAML frontmatter parser for the upload path. `CreateSkillForm.defaultValues` now accepts the parsed shape, so the upload → redirect → pre-populated form flow works again. - `buttons/AdminSettings.tsx` — admin permissions dialog. Uses the existing `useUpdateSkillPermissionsMutation` which was already wired. - `sidebar/FilterSkills.tsx` — restored filter + AdminSettings + CreateSkillMenu wrapper. `SkillsSidePanel.tsx` is back to the original `FilterSkills`-based layout. - `lists/SkillList.tsx` + `lists/SkillListItem.tsx` — restored verbatim. - `layouts/SkillsView.tsx` — restored the full tree + file editor + file preview layout. The chat side panel keeps its own accordion list; this view is the inline detail experience. - `hooks/Generic/useUnsavedChangesPrompt.ts` — route-leave guard hook. - `useGetSkillByIdQuery` is aliased to `useGetSkillQuery` so restored components (`SkillsView`, `SkillForm`) that import the old name resolve to the new hook. - `SkillSelectDialog` + `AgentConfig` coerce `skillsData?.skills` instead of `.data` (list response shape drift from the CRUD PR). - `CreateSkillForm` / `SkillForm` wrap their JSX in `FormProvider` so the restored `CategorySelector` and `SkillContentEditor` components — which read `useFormContext` — work inside the existing forms without another refactor. - `CreateSkillForm.defaultValues` prop accepts `Partial<Values> & { invocationMode?: unknown }` so the upload flow's `{ name, description, invocationMode }` shape passes through cleanly. - `SkillsView` route map gains `/skills/:skillId/edit` and `/skills/:skillId/file/:nodeId` so the tree-navigation URLs the original view produces actually resolve. - `client/package.json` gains `react-arborist@^3.4.3`. - ~60 translation keys the restored files reference — invocation labels, edit/create page titles, file editor chrome, tree toolbar tooltips, favorites, admin allow-settings, unknown-file-type, sr_public_skill, delete/rename _var variants — all added to `en/translation.json`. - Prompts-style floating-label name input — kept from my earlier commit so it matches the rest of the app (user reviewed and approved that styling). Hidden skill-body textarea is replaced by `SkillContentEditor` in both forms. - 38 data-schemas skill.spec.ts - 34 packages/api permissions.spec.ts - 25 api skills.test.js - 7 client useSkillPermissions.test.ts - Type-check: pre-existing error count (188) dropped to 120 because my restorations fixed some previously-broken field types. * chore: Update package-lock.json to include react-arborist and memoize-one * feat: Add support for react-arborist in Vite configuration This update introduces a new condition in the Vite configuration to handle the 'react-arborist' package, ensuring it is properly recognized during the build process. This change enhances compatibility with the recently added 'react-arborist' dependency in the project. * 🩹 fix: Hide InvocationMode, Fix SkillContentEditor Click-to-Edit 1. Hide InvocationModePicker from both CreateSkillForm and SkillForm. Component stays on disk for when the backend lands the column. 2. Fix "Click to edit" doing nothing on SkillContentEditor. The `onBlur={() => setIsEditing(false)}` on the TextareaAutosize was racing with `autoFocus` — React renders the textarea, autoFocus fires, then a layout/reconciliation blur fires immediately, bouncing back to preview mode before the user can interact. Removed onBlur; users toggle via the header button or Escape key. * 🎨 feat: Reader-First Skills UI — Match Claude.ai Layout Reworks the Skills UI from form-first to reader-first, matching Claude.ai's skill detail pattern. **Default view is now read-only.** Clicking a skill in the sidebar navigates to `/skills/:id` which renders `SkillDetail` — a clean content view with: - Skill name as the primary heading - Metadata row: "Added by" + "Last updated" (formatted date) - Description block - Rendered SKILL.md body in a bordered card with a source/rendered toggle (eye + code icons, matching Claude.ai's segmented control) No form fields, no save/cancel buttons. The user reads the skill first and takes action deliberately. **Create is now a dialog.** The `/skills/new` route is gone. `CreateSkillMenu` (the + dropdown in the sidebar) now opens `CreateSkillDialog` — a minimal modal with name, description, and instructions fields. Upload-from-file still works: parse → populate dialog → create. Matches Claude.ai's "Write skill instructions" modal. **Edit is behind an action.** The detail view shows an "Edit" button (permission-gated) that navigates to `/skills/:id/edit`, rendering the existing `SkillForm`. The edit route is preserved for direct linking. **Navigation goes to detail, not edit.** `SkillListItem` now navigates to `/skills/:id` (detail) instead of `/skills/:id/edit`. - `display/SkillMarkdownRenderer.tsx` — shared ReactMarkdown component extracted from `SkillContentEditor`. Same remark/rehype plugins, no form dependency. - `display/SkillDetail.tsx` — the reader-first view (replaces the old thin wrapper). - `dialogs/CreateSkillDialog.tsx` — OGDialog modal for skill creation. - `layouts/SkillsView.tsx` — gutted and rebuilt. Three states: no-skill (empty state), skillId (SkillDetail), skillId+edit (SkillForm). Removed full-page CreateSkillForm, removed TreeView. - `buttons/CreateSkillMenu.tsx` — opens dialog instead of navigating to `/skills/new`. Upload flow: parse → set dialog defaults → open. - `lists/SkillListItem.tsx` — navigate to detail, not edit. - `routes/index.tsx` — removed `/skills/new` and file/nodeId routes; `/skills` renders SkillsView directly (empty state). - `display/index.ts`, `dialogs/index.ts` — added new exports. - `locales/en/translation.json` — added ~10 new keys for metadata, toggle labels, dialog title, empty state. * 🩹 fix: SkillContentEditor click-to-edit z-index — button was z-0 behind rendered content * 🩹 fix: Align Edit button size with Share/Delete (size-9) * 🎨 feat: Claude.ai-Style Skill List Panel Rewrites the skills sidebar to match Claude.ai's panel layout: - Header: "Skills" title + search icon (toggles input) + add icon (opens CreateSkillDialog directly, no dropdown menu) - Collapsible "Skills" section with chevron toggle - Skill items: 24px icon badge (rounded square with ScrollText icon) + name only. No description text in the list — that lives in the detail view. Active item gets highlighted bg + bold font. - Removed AdminSettings button from sidebar header — admin config is accessible via the admin dashboard, not cluttering every user's skill list. - Removed FilterSkills wrapper (was Filter + AdminSettings + CreateSkillMenu). The search + create are now inline in the panel header. Files changed: - sidebar/SkillsSidePanel.tsx — full rewrite - sidebar/SkillsAccordion.tsx — simplified wrapper - lists/SkillList.tsx — collapsible section, no description - lists/SkillListItem.tsx — icon badge + name, memo'd * 🎨 fix: Align Skills UI Styling with Prompts Patterns Style alignment pass based on direct comparison with claude.ai and the existing prompts preview dialog. SkillsSidePanel search now replaces the title in the header row when toggled (search icon + input + X close), matching Claude.ai's pattern. Previously it pushed a separate input below the header, wasting vertical space. Close button clears the search term. Replaced `text-text-tertiary` with `text-text-secondary` across SkillDetail, SkillList, SkillForm, CreateSkillForm, CreateSkillDialog, SkillContentEditor. Tertiary was too dark / low contrast. SkillList section chevron label now reads "Personal skills" (matching Claude.ai) via the existing `com_ui_my_skills` key, instead of the generic "Skills" which duplicated the header. Aligned with `PromptDetailHeader` styling: - 48px round icon (ScrollText in bg-surface-secondary circle) - Name + public badge in the icon row - Metadata below the icon: User icon + author, Calendar icon + date (text-xs text-text-secondary with gap-3, matching prompts exactly) - Description uses the same label-above-text pattern as prompts - Content card uses `bg-transparent` border (not bg-surface-primary-alt) - Toggle buttons use size-5 icons and text-text-secondary for inactive Changed from `max-w-lg p-0` to `max-w-5xl` with the same max-height and padding pattern as the prompts PreviewPrompt dialog: `max-h-[80vh] p-1 sm:p-2 gap-3 sm:gap-4`. Close button now renders via default OGDialogContent behavior (removed showCloseButton=false). * 🩹 fix: SkillDetail fills parent height, tighter spacing (px-6 pb-6 gap-2) * 🩹 fix: Align Skills panel header padding (px-4) with list content below * 🩹 fix: Reduce Skills header top padding (pt-2) to align with sidebar icon strip * 🩹 fix: Tighten Skills header (py-2) and detail top (py-2) to align with sidebar icons and match edit view * 🩹 fix: Offset SidePanel Nav pt-2 with -mt-2 on SkillsAccordion so Skills header aligns with icon strip * 🛠️ fix: Increase Node memory limit for production build in package.json * 🩹 fix: Remove top padding from SkillDetail header row (py-2 → pb-2) * 🏗️ refactor: Move pt-2 from SidePanel/Nav wrapper to each panel Removed the global `pt-2` from `SidePanel/Nav.tsx` and pushed it into each panel's own top-level wrapper. This lets each panel own its vertical alignment independently — Skills can sit flush at the top to align with the sidebar icon strip, while other panels keep their original spacing. Panels updated with `pt-2`: - PromptsAccordion (via className on PromptSidePanel) - BookmarkPanel - FilesPanel - MemoryPanel - MCPBuilderPanel - AgentPanel (form wrapper) - AssistantPanel (form wrapper) - ParametersPanel (already had pt-2) SkillsAccordion: removed the -mt-2 hack, now naturally flush. * 🧹 fix: Align CreateSkillDialog field styling + remove 19 unused i18n keys Dialog fields: all three inputs now use consistent `rounded-xl border-border-medium px-3 py-2 text-sm` styling. Replaced the `<Input>` component with a plain `<input>` to avoid the component's built-in `rounded-lg border-border-light` overriding the dialog's border style. Labels use `font-medium` for consistency. Removed 19 unused translation keys from translation.json: com_ui_skill_body, com_ui_skill_body_placeholder, com_ui_skill_create_subtitle, com_ui_skill_file_delete_confirm, com_ui_skill_file_delete_error, com_ui_skill_file_deleted, com_ui_skill_files_empty, com_ui_skill_files_multi_hint, com_ui_skill_list, com_ui_skill_load_error, com_ui_skill_resize_file_tree, com_ui_skill_select_file, com_ui_skill_select_file_desc, com_ui_skills_load_error, com_ui_add_first_skill, com_ui_create_skill_page, com_ui_edit_skill_page, com_ui_save_skill, com_ui_no_skills_title * 🎁 feat: Upload Skill Dialog + Simplified Create Menu New `UploadSkillDialog` matching Claude.ai's upload modal: - Dashed drop zone with drag-and-drop support - Accepts .md, .zip, .skill files - Phase 1: processes .md files (parses YAML frontmatter → creates skill with body as the full file content) - Shows file requirements below the drop zone - On success: navigates to the new skill's detail view `CreateSkillMenu` now has two flat options (no sub-menu): - "Write skill instructions" → opens `CreateSkillDialog` - "Upload a skill" → opens `UploadSkillDialog` Removed the disabled "Create with AI" option and the old file input hidden-element approach. The sidebar `+` button now renders `CreateSkillMenu` directly instead of a standalone create dialog. - Removed 5 unused i18n keys (com_ui_skill_added_by, com_ui_skill_last_updated, com_ui_skills_add_first, com_ui_skills_filter_placeholder, com_ui_skills_new) - Tightened metadata gap in SkillDetail (mt-1 → mt-0.5) - Added 7 new upload-related i18n keys * 🔒 feat: Zip/Skill File Upload Support with Safety Limits Rewrites UploadSkillDialog to properly handle all three accepted file types: - `.md` — reads as text, parses YAML frontmatter, creates skill - `.zip` / `.skill` — reads as ArrayBuffer, extracts with JSZip, finds SKILL.md (at root or one level deep), parses its content, creates skill. Shows spinner during processing. Security guards against zip bombs: - MAX_ZIP_SIZE: 50MB compressed file limit - MAX_ENTRIES: 500 file limit inside the archive - Path traversal rejection: skips entries with `..` or leading `/` - SKILL.md search limited to depth ≤ 2 segments Added `jszip@^3.10.1` to client dependencies (already in the monorepo's node_modules from backend usage). The name is inferred from the zip filename if SKILL.md frontmatter doesn't have one (e.g. `skills-autofix.zip` → `skills-autofix`). * 🚀 feat: Backend Skill Import + Live File Upload Endpoints New endpoint that accepts a single multipart file (.md, .zip, .skill) and creates a skill with all its files in one request: - **.md**: parse YAML frontmatter → create skill with body - **.zip / .skill**: extract with JSZip, find SKILL.md (root or one level deep), create skill from its content, then persist every additional file via `upsertSkillFile` + local file storage strategy. Returns the created skill + an `_importSummary` with per-file results. Security: - 50MB compressed file size limit (multer) - 500 max entries in archive - 10MB per individual file - Path traversal rejection (no `..`, no absolute, validated charset) - File type filter: only .md/.zip/.skill accepted - Rate limited via existing `fileUploadIpLimiter` + `fileUploadUserLimiter` Handler lives in `packages/api/src/skills/import.ts` with injectable deps (`createSkill`, `upsertSkillFile`, `saveBuffer`) for testability. Replaced the 501 stub with a real handler: - Accepts multipart FormData with `file` + `relativePath` - Saves file via local storage strategy - Calls `upsertSkillFile` to persist the SkillFile record - Returns the upserted document - Rate limited, ACL-gated (EDIT permission required) - 10MB per file limit `UploadSkillDialog` now sends the file to `/api/skills/import` via `dataService.importSkill(formData)` — no more client-side JSZip. Removed `jszip` from client dependencies (only backend needs it). Added `importSkill()` in data-service + `importSkill()` endpoint builder in api-endpoints. Updated the file upload test from expecting 501 stub to expecting 400 "no file provided" (live validation). All 25 skill route tests pass. * 🔒 fix: Complete Import Handler — Validation, Ownership, Error Surfacing Fixes several gaps in the skill import flow: 1. **Skill validation now runs and surfaces properly.** The import handler calls the real `createSkill(CreateSkillInput)` which runs `validateSkillName`, `validateSkillDescription`, `validateSkillBody`. Validation errors (SKILL_VALIDATION_FAILED) are caught and returned as 400 with the issue messages. Duplicate-key errors return 409. Previously all errors were swallowed into a generic 500. 2. **`authorName` is now populated.** The `CreateSkillInput` requires `authorName` which was missing — resolved from `req.user.name ?? req.user.username ?? 'Unknown'`, matching the existing create handler. 3. **SKILL_OWNER permission is granted after import.** Calls `grantPermission` with `AccessRoleIds.SKILL_OWNER` so the uploader can edit/delete/share the imported skill. This was entirely missing — imported skills would have been ownerless. 4. **`tenantId` propagated.** Both the skill and each SkillFile record receive `req.user.tenantId` for multi-tenant deployments. 5. **SkillFile records are created in the DB.** Each non-SKILL.md file in the zip is saved to file storage via `saveBuffer` and recorded via `upsertSkillFile`, which validates the relativePath, infers the category from the path prefix, and atomically bumps the skill's `fileCount` and `version`. Import deps now include `grantPermission` from PermissionService, injected in `api/server/routes/skills.js`. * 🐛 fix: Import grant uses accessRoleId (not roleId) — fixes skill not appearing in list * 🎨 fix: Cache invalidation, file tree, frontmatter rendering Three fixes for the skill detail view: 1. **Cache invalidation after import.** UploadSkillDialog now calls `queryClient.invalidateQueries([QueryKeys.skills])` after a successful import so the sidebar list picks up the new skill without requiring a page refresh. 2. **File tree in detail view.** When a skill has `fileCount > 0`, the detail view now queries `useListSkillFilesQuery` and renders a file list below the body card — SKILL.md first, then folders and root files. Icons: Folder for directories, FileText for files. 3. **Frontmatter stripped and rendered as metadata.** YAML frontmatter (`---\nversion: 0.1.0\ntriggers: ...\n---`) is now parsed out of the body before markdown rendering. The `name` and `description` fields are skipped (already shown in the header). Remaining fields (version, triggers, dependencies, etc.) are displayed in a Claude.ai–style grid: label on the left, value on the right, above the rendered markdown content. Source view still shows the full raw body including frontmatter. * 🩹 fix: Always fetch skill files — fileCount may be stale in cached skill object * 🌳 feat: Inline File Tree in Sidebar Skill List Moves the file tree from the bottom of SkillDetail into the sidebar list, matching Claude.ai's pattern: - Multi-file skills show a chevron toggle on the right side of the skill list item - Clicking the chevron expands an inline file tree below the skill name: SKILL.md first, then folders (with folder icon + right chevron) and root files - File list is fetched lazily (only when expanded) via useListSkillFilesQuery - Clicking a file navigates to the skill detail view - Files section removed from SkillDetail — the sidebar is now the sole file tree location, keeping the detail panel clean SkillDetail cleaned up: removed groupFiles helper, file-related state, useListSkillFilesQuery import, FileText/Folder icon imports. * 🌲 feat: Virtualized inline file tree with react-vtree Replace hand-rolled recursive FolderRow/FileRow buttons with a proper virtualized FixedSizeTree from react-vtree for the sidebar skill list. Dynamic height tracks open folders; capped at 350px with smooth expand/collapse transitions. * chore: Remove no longer used SkillFileTree and SkillTreeNode components * chore: Update Vite config to replace 'react-arborist' with 'react-vtree' for module resolution * feat: Skill file content viewing with lazy DB caching - Add `skills` field to `fileStrategiesSchema` so operators can configure a dedicated storage backend for skill files. Falls back by type (image/document) when unset. - Fix hardcoded `FileSources.local` in skill save/import — now uses the resolved strategy via `getFileStrategy(req.config, { context })`. - Replace 501 download stub with real handler that streams from any storage backend and returns JSON `{ content, mimeType, isBinary }`. - Binary detection (null-byte + non-printable ratio on first 8 KB) flags files on first read so they're never re-fetched. - Text content ≤ 512 KB is cached in the SkillFile MongoDB document; subsequent reads skip storage entirely. - Clicking a skill row now expands inline files (not just chevron). - Clicking a file navigates to `?file=<path>` and renders content in a new SkillFileViewer (markdown, code, images, binary placeholder). * chore: Remove react-window and its type definitions from package.json and package-lock.json - Deleted `react-window` and `@types/react-window` dependencies from both `package.json` and `package-lock.json` to streamline the project and reduce unnecessary bloat. * fix: Build errors — remove endpoints import, fix Uint8Array cast - Replace `import { endpoints }` (not public) with inline URL in SkillFileViewer - Remove `as Uint8Array` cast in stream chunk handling - Extend getSkillFileByPath return type with content/isBinary to decouple from data-schemas build artifact resolution * chore: Remove 8 unused i18next keys com_ui_create_skill_ai, com_ui_create_skill_manual, com_ui_delete_folder_confirm_var, com_ui_delete_skill, com_ui_delete_skill_confirm_var, com_ui_delete_var, com_ui_rename_var, com_ui_skill_files * fix: Add configMiddleware to skills router, handle SKILL.md in viewer - Add configMiddleware to skills router so req.config is populated when getLocalFileStream (or any strategy) reads file paths. - Handle SKILL.md in download handler — serves skill.body directly from the Skill document instead of looking for a SkillFile record. - Clicking SKILL.md in sidebar tree now opens the file viewer (matching Claude.ai behavior: file view vs default detail view). * ci: Run unit tests on PRs to any branch Remove the branches filter from both test workflows so contributor PRs targeting feature branches (not just main/dev) get CI coverage. Path filters are kept so tests only run when relevant files change. * fix: Update skills route tests for download handler changes - Mock configMiddleware (sets req.config for file storage access) - Mock getStrategyFunctions and getFileStrategy (storage strategy deps) - Replace 501 stub test with SKILL.md content test + 404 test * fix: Auto-expand files, frontmatter parsing, select-none, prefetch - Auto-expand file tree when navigating directly to a skill URL - Prefetch files for the active skill (eliminates first-expand lag) - Fix frontmatter parser to handle multi-line YAML list values (triggers field was missing because it uses list syntax) - SkillFileViewer now parses frontmatter for .md files — shows structured grid + rendered body (matching SkillDetail's display) with source/rendered toggle - Add select-none to all sidebar skill and file tree buttons * refactor: Derive expanded state from isActive instead of useEffect Replace useEffect sync with deterministic derivation: expanded = hasFiles && (isActive || !collapsed) Active skill is always open. collapsed is a manual toggle that only takes effect on non-active items. * fix: Remove empty space above body card — overlay view toggle Move the rendered/source toggle from a dedicated row (40px of empty space) to an absolute-positioned overlay in the card's top-right corner, matching Claude.ai's layout. * fix: Remove header bars from content editors — overlay action buttons Collapse the full-width header bars ("Skill Content", "Text") in SkillContentEditor, PromptTextCard, and PromptEditor. Action buttons (edit/save toggle, copy, variables) are now absolute-positioned in the card's top-right corner, reclaiming ~46px of vertical space. * fix: Spinner visibility in file viewer — use text-text-secondary * fix: Address review findings — security, correctness, code quality Codex P1: Use $unset instead of undefined to clear cached content and isBinary fields on file re-upload (Mongoose strips undefined). Codex P2: Match skill-file validation errors by error.code instead of error.message substring. F1: Zip bomb defense — track cumulative decompressed bytes (500 MB cap), check declared uncompressed size before buffering each entry. F2: Remove misleading "atomically" from import handler JSDoc. F3: Static import for isBinaryBuffer instead of dynamic import(). F4: Replace console.error with logger in upload handler. F6: Add multer error handler middleware to skills router. F7: Move React import to top of SkillDetail.tsx. F9: Fix variable shadowing (trimmed → item) in parseFrontmatter. F11: Replace JSON.parse(JSON.stringify()) with toJSON() for Mongoose document serialization. F12: Remove dead dynamic import('fs') fallback (memoryStorage always provides file.buffer). F13: Hoist MIME_MAP to module scope to avoid per-call allocation. F16: Share single multer.memoryStorage() instance. * fix: Follow-up review — close zip bomb gap, fix error handler F1: Add post-decompression cumulative byte check with break (the pre-decompression check relies on undocumented JSZip internals that may be absent; this closes the gap unconditionally). F2+F3: Multer error handler now forwards non-multer errors via next(err) instead of swallowing them. Also catches file filter rejections (plain Error, not MulterError) by message prefix. F4: Move isBinaryBuffer import to local imports section per CLAUDE.md import order rules. F5: Simplify dead toJSON branch — createSkill returns a POJO. * nit: Link filter error message to handler prefix check * feat: Accordion expansion + active file highlight in sidebar - Only one skill's file tree can be expanded at a time (accordion). Expansion state lifted from SkillListItem to SkillList. - Selected file gets bg-surface-active highlight in the tree. Skill row uses subtle style (no background) when a file is active, matching Claude.ai's pattern where the file — not the skill — carries the selection state. * style: Adjust margin for file tree in SkillListItem component - Reduced left margin from 10 to 5 for improved layout consistency in the file tree display. * fix: TS error on FileTreeNode, nested ternary, chevron collapse - Make style prop optional to match react-vtree's NodeComponentProps - Flatten nested ternary for skill row active styles - Skill row click expands (but doesn't collapse) files + navigates - Chevron click explicitly toggles collapse (matching Claude.ai where clicking the chevron is how you collapse files) * fix: Upload basePath, reject SKILL.md uploads, add skills permission route - Pass basePath: 'uploads' in per-file upload handler (was defaulting to 'images' path, inconsistent with the import flow). - Reject uploads targeting SKILL.md (reserved path — download handler special-cases it to return skill.body, making an uploaded file unreachable via the API). - Add skills entry to roles router permissionConfigs so PUT /api/roles/:roleName/skills actually reaches a handler instead of returning 404. * feat: Expand content area, move controls to header, reduce padding Default detail view: - Remove rounded-xl bordered card wrapper — content flows directly into the article, capitalizing on full screen width - Move eye/code toggle inline with the divider row - Reduce px-6/pb-6 to px-4/pb-4 File viewer: - Move eye/code toggle from card overlay to the header bar - Add copy-to-clipboard button for text files in the header bar - Remove rounded-xl bordered card wrapper for markdown content - Remove bordered pre wrapper for non-markdown text - Reduce px-6/py-4 to px-4/py-3 Both views maximize content space over decorative chrome. * fix: Stable header height, restore some padding - Fix layout shift in file viewer header: use fixed h-10 so the bar height stays constant whether the eye/code toggle renders (markdown) or not (plain text). - Bump content padding from px-4/py-3 back to px-5/py-4 in both views — the previous reduction was too aggressive. * fix: Grant rollback, path validation, error format, dead code cleanup F2: grantOwnership now rolls back (compensating delete) on failure, matching the create handler. Both markdown and zip import paths check the result and return 500 on grant failure. F4: Upload handler validates relativePath with regex + traversal check before calling downstream upsertSkillFile. F5: Document JSZip _data.uncompressedSize as best-effort; the post-decompression cumulative check is the real safety net. F10: Standardize all upload handler error responses to { error } (was { message }, inconsistent with handlers.ts). F13: Single-pass fileResults accumulation in import handler. F1-5: Remove dead uploadFileStubHandler (no route references it). Codex P2: Fix delete nav from /skills/new to /skills. F12: Use cn() in UploadSkillDialog instead of template literals. * perf: Stream-first binary detection + O(1) public skill check F1: Download handler now reads only the first 8 KB for binary detection. If binary, the stream is destroyed immediately without buffering the remaining file. Text files continue reading for caching. Eliminates buffering up to 10 MB per request for binary files under concurrent load. F7: Single-skill GET and PATCH now use hasPublicPermission (O(1) ACL lookup) instead of getPublicSkillIdSet (queries ALL public skill IDs). The list handler still uses the Set approach since it serializes multiple skills. serializeSkill/serializeSkillSummary now accept boolean | Set for flexibility. * fix: Update test to match { error } response format * fix: Critical stream truncation bug, grantedBy, error format NF-1 (CRITICAL): Rewrite binary detection to single for-await loop. Breaking out of for-await-of destroys the stream via iterator.return(), so the previous two-loop approach silently truncated text files > 8KB. Now: one loop collects chunks, checks binary after 8KB accumulated, and either destroys+returns (binary) or continues reading (text). NF-2: Add grantedBy to import handler's grantPermission call and interface (was missing, inconsistent with create handler). NF-3: Standardize all import handler error responses from { message } to { error }, matching handlers.ts convention. Update client's UploadSkillDialog to read response.data.error accordingly. * fix: Prefer specific validation message over generic error field * fix: YAML quote stripping, saveBuffer null guard, dot segment rejection - Strip surrounding YAML quotes from frontmatter values so name: "my-skill" parses as my-skill (not "my-skill" with quotes that fails the name validator). - Guard resolveSkillStorage against backends with saveBuffer: null (e.g. OpenAI/vector strategies) — throws a descriptive error caught by the handler's try/catch instead of a TypeError. - Tighten upload path validation to reject . segments (e.g. docs/./a.md) matching the model-layer validator, preventing storage writes for paths the DB will reject. * fix: Orphan cleanup, stream errors, malformed zip, cache latency F1: Upload handler now deletes the stored blob if the subsequent DB upsert fails, preventing orphaned files on disk/cloud. F2: Multer error handler returns { error } (was { message }). F3: Wrap JSZip.loadAsync in try/catch — malformed zip returns 400 instead of falling through to 500. F4: Raw download stream gets an error handler — logs the error and destroys the response if headers were already sent. F8: Strip leading hyphens from inferred skill name so filenames like _my-skill.zip don't produce -my-skill (invalid name pattern). F9: Fire-and-forget all updateSkillFileContent cache writes so the response is sent immediately. Cache failures are logged but don't block or fail the read. * fix: Import orphan cleanup + Content-Disposition sanitization Finding A: Add deleteFile dep to ImportSkillDeps. The per-file loop in handleZip now cleans up stored blobs when upsertSkillFile fails, closing the second half of the F1 orphan fix (upload handler was already fixed). Finding B: Sanitize filename in Content-Disposition header for raw downloads — strip quotes, backslashes, and newlines to prevent header injection from user-uploaded filenames. * security: Prevent stored XSS via raw file downloads Non-image files served via ?raw=true now use Content-Disposition: attachment (force download) instead of inline. An uploaded .html or .svg file served inline from the LibreChat origin could execute scripts with access to the user's session — this closes that vector. Images stay inline (needed for <img> rendering in SkillFileViewer). X-Content-Type-Options: nosniff added to prevent MIME sniffing. * security: Block SVG XSS — allowlist safe raster MIME types for inline SVG (image/svg+xml) passed the startsWith('image/') check and was served inline, but SVG is a scriptable format — embedded <script> tags execute in the LibreChat origin. Replace the prefix match with a Set of safe raster-only MIME types (png, jpeg, gif, webp, avif, bmp). SVGs and any future scriptable image/* subtypes now get Content-Disposition: attachment (forced download). * fix: Cap JSON text response at 1MB, consistent md name inference F3: Text files > 1MB now return { isBinary: false } with no content field, forcing the client to use ?raw=true for download. Prevents buffering 10MB files into heap for JSON serialization. Frontend shows a download fallback when content is absent. F4: handleMarkdown now infers skill name from filename (same as handleZip) when frontmatter has no name, instead of rejecting with 400. Consistent behavior across import paths. F1 (reviewer concern): upsertSkillFile is NOT affected — it uses { new: false } for insert-vs-replace detection but does a follow-up findOne (lines 855-859) to return the post-upsert document. * fix: deleteFile arg shape, raw URL base path, hoist SAFE_INLINE_MIMES Codex P2: deleteFile expects { filepath } object, not a raw string. Both upload handler cleanup and import handler cleanup now pass { filepath } to match the strategy contract (deleteLocalFile, deleteFileFromS3 all expect a file object). Codex P2: Raw download URL in SkillFileViewer now uses apiBaseUrl prefix so subpath deployments (/chat, etc.) resolve correctly. NIT: Hoist SAFE_INLINE_MIMES Set to factory scope — was re-allocated per raw download request inside the if block. * fix: Remove inert cache write for large text files, localize aria-label N2: The { isBinary: false } cache write for text files > 1MB had no effect — subsequent requests still fell through to stream read since neither isBinary nor content provided a fast-path short-circuit. Removed the pointless DB updateOne per request. N4: Replace hardcoded "Back to skill" aria-label with localize(). * refactor: Extract shared parseFrontmatter, widen deleteFile type N3: Extract parseFrontmatter into Skills/utils/frontmatter.ts — single implementation shared by SkillDetail and SkillFileViewer. Accepts optional skipKeys set so callers control which frontmatter fields are excluded (SkillDetail skips name/description since they're shown in the header; other .md files show all fields). N5: Widen ImportSkillDeps.deleteFile file param from { filepath } to { filepath; [key: string]: unknown } to signal extensibility if strategies start accessing additional file properties. * fix: Advance i past list items for skipped keys, DRY parseSkillMd Finding A: parseFrontmatter now consumes multi-line YAML list items before checking skipKeys — prevents list lines from leaking into subsequent key parsing as spurious fields. Finding B: parseSkillMd now delegates to the shared parseFrontmatter instead of re-implementing the same frontmatter scanning loop. Reduces client-side parseFrontmatter implementations from 3 to 1. * fix: Call apiBaseUrl(), delete storage blob on file removal - apiBaseUrl is a function, not a string — call it in the template literal so raw download URLs resolve correctly. - deleteFileHandler now looks up the file record before deleting, then fire-and-forget deletes the storage blob via the strategy's deleteFile. Previously only the DB record was removed, leaving orphaned blobs in local/S3/Firebase/Azure storage. * fix: Clean up storage blobs when deleting an entire skill deleteHandler now lists all files for the skill before calling deleteSkill, then fire-and-forget deletes each blob via the storage strategy. Previously only per-file deletion cleaned up blobs — deleting a whole skill left all associated files orphaned in local/S3/Firebase/Azure storage. * refactor: useImportSkillMutation hook, fix TSkill[] unsafe cast - Create useImportSkillMutation in mutations.ts + ImportSkillOptions type. UploadSkillDialog now uses the mutation hook instead of calling dataService.importSkill directly with manual useState loading management. Eliminates unmounted-component state update risk and aligns with the React Query mutation pattern used by every other mutation in the codebase. - SkillSelectDialog: replace as unknown as TSkill[] with proper TSkillSummary typing. SkillCard props updated to TSkillSummary. The dialog only uses summary-level fields (name, description, category, author) — the cast was hiding a type mismatch. * fix: Use saved source for import cleanup, delete old blob on replace Codex P2: Import cleanup now uses file.source (the backend the file was actually saved to) instead of re-resolving from config. In mixed strategy setups, the previous approach could target the wrong backend. Codex P2: When re-uploading a file to an existing relativePath, the old blob is now deleted after successful upsert. Previously only the DB record was replaced, leaving the old storage object orphaned. * fix: Register PUT /:roleName/skills route in roles router * fix: Re-read skill after zip file processing for fresh metadata The import response was built from the skill object created before the file loop, but each upsertSkillFile bumps version and fileCount. Clients caching the stale response would get 409 conflicts on first edit and see incorrect file counts. Now re-reads the skill via getSkillById after the loop so the response reflects the current version, fileCount, and updatedAt. * fix: Size-check SKILL.md before decompression, don't gate on fileCount P1: SKILL.md was decompressed before any size accounting. A crafted archive could expand SKILL.md past 10MB before validation ran. Now checks declared size pre-decompression and actual size post, both against MAX_SINGLE_FILE_BYTES. P2: File list query was gated on cached fileCount which can be stale after mutations. Now fetches files for the active skill regardless of fileCount. hasFiles derived from fetched data with fileCount as fallback, so newly uploaded files appear without hard refresh. * fix: Move files declaration before hasFiles to avoid TDZ error * security: Stream-decompress zip entries with enforced byte cap Replace zipEntry.async('nodebuffer') (buffers entire entry before checking limits) with zipEntry.nodeStream('nodebuffer') piped through a byte counter that destroys the stream when the per-file or cumulative limit is exceeded. Previously, when JSZip's _data.uncompressedSize was absent (the common case), a high-ratio entry could allocate hundreds of MB before the post-decompression check caught it. Now decompression is aborted mid-stream at the exact byte threshold — no entry can exceed its limit regardless of compression ratio. * refactor: Reorganize access check for prompts in useSideNavLinks hook Moved the prompts access check to a new position in the code to improve readability and maintainability. This change ensures that the prompts link is added to the navigation only if the user has the appropriate access, without altering the existing functionality. --------- Co-authored-by: Danny Avila <danny@librechat.ai>	2026-04-25 04:02:00 -04:00
Danny Avila	963068b112	🧬 feat: Scaffold Skills CRUD with ACL Sharing and File Schema (#12613) ... * 🧬 feat: Scaffold Skills CRUD with ACL Sharing and File Schema Adds Skills as a new first-class resource modeled on Anthropic's Agent Skills, reusing the existing Prompt ACL stack for sharing. Lays the groundwork for multi-file skills (SkillFile schema + metadata routes) without wiring upload processing — single-file skills (inline SKILL.md body) work end-to-end, multi-file uploads are stubbed for phase 2. * 🔬 fix: Wire Skill Cleanup, AccessRole Enum, and Express 5 Path Params CI surfaced four follow-ups from the initial Skills scaffolding commit that local builds missed: - AccessRole's resourceType field had a hardcoded enum that didn't include `'skill'`, blocking SKILL_OWNER/EDITOR/VIEWER role creation in every test that hit the AccessRole model. - The seedDefaultRoles assertion in accessRole.spec.ts hard-listed the expected role IDs and needed the new SKILL_* entries. - deleteUserController had no cleanup for skills, and the deleteUserResourceCoverage guard test enforces every ResourceType has a documented handler — wired in db.deleteUserSkills(user._id) and added the entry to HANDLED_RESOURCE_TYPES. - Express 5's path-to-regexp v6 rejects the legacy `(*)` named-group glob syntax. The two skill file routes now use a plain `:relativePath` param; the client already encodeURIComponents the path, so a single param is sufficient and decoded server-side. * 🪡 fix: Make Skill Name Uniqueness Application-Level Resolve three more CI failures from the Skills scaffolding PR: - Mongoose creates indexes asynchronously and mongodb-memory-server tests can race ahead of the unique (name, author, tenantId) index being built, so the duplicate-name uniqueness test was flaky. Added an explicit findOne pre-check inside createSkill that throws with code 11000 (mimicking the index violation), giving deterministic behavior. The unique index stays as the persistent guarantee. - The deleteUser.spec.js and UserController.spec.js suites mock the ~/models module directly and were missing deleteUserSkills, causing deleteUserController to throw and return 500 instead of 200. - Removed two doc-comment claims that the SKILL_NAME_MAX_LENGTH and SKILL_DESCRIPTION_MAX_LENGTH constants "match Anthropic's API". The values themselves are reasonable but the comments were misleading about who enforces them. * 🪢 fix: Address Code Review Findings on Skills Scaffolding Resolve all 15 findings from the comprehensive PR review: Critical: - Rollback the created skill when grantPermission throws so a transient ACL failure cannot leave an orphaned, inaccessible skill in the DB. - Fix infinite query cache corruption in useUpdateSkillMutation helpers. setQueriesData([QueryKeys.skills]) matches useSkillsInfiniteQuery's InfiniteData cache entries, which have { pages, pageParams } shape — spreading data.skills on those would throw. Added an isInfiniteSkillData guard and per-page transform so both flat and infinite caches update correctly. Major: - Fix TUpdateSkillContext type: the public type declared previousListData but onMutate actually returns previousListSnapshots (a [key, value] tuple array). Updated the type + added TSkillCacheEntry as a shared export from data-provider. - Add cancelQueries calls before optimistic update in onMutate so in-flight refetches cannot clobber the optimistic state. - Parallelize deleteUserSkills ACL removal via Promise.allSettled instead of a sequential await loop — O(1) round-trip vs O(n). - Stub mockDeleteUserSkills in stubDeletionMocks() and assert it's called with user.id in the deleteUser.spec.js happy-path test. - Add idResolver: getSkillById to the SKILL branch in accessPermissions.js so GET /api/permissions/skill/<missing-id> returns 404 instead of 403. Minor: - Reuse resolved skill from req.resourceAccess.resourceInfo in getHandler to eliminate a redundant getSkillById call per GET /api/skills/:id. - Reject PATCH /api/skills/:id requests whose body contains only expectedVersion — previously they silently bumped version with no changes, triggering spurious 409s for collaborators. - Make TSkill.frontmatter optional (wire type) and add serializeFrontmatter / serializeSourceMetadata helpers that return undefined for empty objects instead of casting incomplete data to SkillFrontmatter. - Standardize deleteUserSkills to accept string | ObjectId and convert internally, matching deleteUserPrompts's signature; UserController now passes user.id consistently. - Replace bumpSkillVersionAndRecount (read-then-write, racy) with bumpSkillVersionAndAdjustFileCount using atomic $inc. upsertSkillFile pre-checks existence to distinguish insert (+1) from replace (0). - Add DELETE /api/skills/:id/files/:relativePath integration tests covering success, 404, and 403 paths. Nits: - Drop trivial resolveSkillId wrapper — pass getSkillById directly. - Remove dead staleTime: 1000 * 10 from useListSkillsQuery since all refetch triggers are already disabled. * 🧭 fix: Resolve Second Skills Review Pass — Cache, Gate, TOCTOU Address 13 of 14 findings from the second code review; reject #13 as misread of the AGENTS.md import-order rule (package types correctly precede local types regardless of length). Major: - Fix addSkillToCachedLists closure bug: a hoisted `prepended` flag was shared across every cache entry matched by setQueriesData, so concurrent flat + infinite caches would silently drop the prepend on whichever was processed second. Replaced the shared helper with three per-entry inline updaters that handle InfiniteData at the page level (page 0 only for prepend, all pages for replace/remove). - Tighten patchHandler's expectedVersion validation: NaN passes `typeof === 'number'` and would previously leak current skill state via a misleading 409. Now requires finite positive integer and returns 400 otherwise. - Guard decodeURIComponent in deleteFileHandler with try/catch — malformed percent encoding now returns 400 instead of 500. - Add PermissionTypes.SKILLS + skillPermissionsSchema + TSkillPermissions in data-provider; seed default SKILLS permissions for ADMIN (all true) and USER (use + create only); wire checkSkillAccess / checkSkillCreate via generateCheckAccess onto the skills router mirroring the prompts pattern. Skills route now enforces role-based capability gates alongside per-resource ACLs. Test suite adds a mocked getRoleByName returning permissive SKILLS. - Fix upsertSkillFile TOCTOU: replaced the pre-check + upsert pair with a single `findOneAndUpdate({ new: false, upsert: true })` call that atomically returns the pre-update doc (null ⇒ insert) so fileCount delta can't double-count on concurrent same-path uploads. Minor: - Add `sourceMetadata` to listSkillsByAccess .select() so summaries no longer silently drop the field for GitHub/Notion-synced skills. - Include `cursor` in useListSkillsQuery's query key so manual pagination doesn't alias across pages. - Clean up TSkillSummary to `Omit<TSkill, 'body' | 'frontmatter'>` matching what serializeSkillSummary actually emits; drop the Omit-then-re-add noise. - Skip getPublicSkillIdSet in createHandler; a newly-created skill cannot have a PUBLIC ACL entry, so pass an empty set directly instead of paying a DB round-trip. - Trim SkillMethods public surface: drop internal helpers countSkillFiles / deleteSkillFilesBySkillId / getSkillFile from the return object; inline the file cascade into deleteSkill. - Use TSkillConflictResponse at the PATCH 409 call site instead of an inline ad-hoc object literal. - Drop the now-unused EXPECTED_VERSION_ERROR module constant. * 🧩 fix: Extend Role Schema + Types with SKILLS PermissionType CI type-check and unit test failures from the PermissionTypes.SKILLS addition surfaced three unrelated places that all hardcode the permission-type set: - IRole.permissions in data-schemas/types/role.ts enumerates every PermissionTypes key as an optional field. Adding SKILLS to the enum without updating the interface caused TS7053 'expression of type PermissionTypes can't be used to index type' errors in role.methods.spec.ts (lines 407-408, 477-478) because Object.values(PermissionTypes) now yielded a value the interface didn't cover. - schema/role.ts rolePermissionsSchema mirrors the interface at the Mongoose layer; also needed SKILLS added so the persisted role document can actually store skill permissions. - data-provider/roles.spec.ts has a guard test that every permission type carrying CREATE/SHARE/SHARE_PUBLIC must be explicitly "tracked" either in RESOURCE_PERMISSION_TYPES or in the PROMPTS/AGENTS/MEMORIES exemption list. Added SKILLS to the exemption list since skills follow the same default model as prompts/agents (USE + CREATE on for USER, SHARE / SHARE_PUBLIC off). All three are additive pass-throughs with no behavior change. * 🏷️ refactor: Introduce ISkillSummary for Narrow List Projection Follow-up NITs from the second review pass on the Skills PR: - Define ISkillSummary = Omit<ISkill, 'body' | 'frontmatter'> and use it as the element type in ListSkillsByAccessResult. The list query's .select() intentionally omits body and frontmatter for payload size, but the previous type claimed both fields were present — a type lie that would mislead future readers even though serializeSkillSummary never touches those fields at runtime. handlers.ts's signature for serializeSkillSummary now accepts ISkillSummary too. - Document the intentional second-round-trip `findOne` in upsertSkillFile. Switching to `findOneAndUpdate({ new: false })` was required for TOCTOU-safe insert-vs-replace detection, which means the handler needs a follow-up query to return the post-upsert document. A comment now explains the tradeoff so future readers don't silently "optimize" it away. No behavior change. * 🌐 fix: Wire SKILL into SHARE_PUBLIC Resource Maps Address codex comment #1 — making a skill public was blocked on two hardcoded resource→permission-type maps that didn't know about SKILL: - api/server/middleware/checkSharePublicAccess.js's resourceToPermissionType map was missing ResourceType.SKILL, so PUT /api/permissions/skill/:id with { public: true } would fall through to the 400 "Unsupported resource type for public sharing" path even though PermissionTypes.SKILLS exists and ADMIN has SHARE_PUBLIC configured. Added the mapping. - client/src/hooks/Sharing/useCanSharePublic.ts has an identical client-side map used to gate the "Make Public" UI toggle. Without the SKILL mapping the hook returned false for everyone, so the toggle wouldn't render for skills once the sharing UI lands in phase 2. Added the mapping. Codex comment #2 (create/update cache writes inject skills into unrelated filtered lists) is invalid — it flags a pattern that mirrors useUpdatePromptGroup (which the PR description explicitly cites as the model) and is a deliberate optimistic-update tradeoff. Trying to match each cache key's embedded filter would couple the mutation callback to query-key internals, which is exactly what setQueriesData is designed to avoid. No change there. * 🧪 feat: Frontmatter Validation, Reserved-Name Fixes, Coaching Warnings Address the follow-up review notes on the Skills PR. This commit closes the gap between the wire-type promise and what the backend actually enforces, tightens the reserved-name rules, and adds a non-blocking coaching tier for validators. Frontmatter validation (new): - Add `validateSkillFrontmatter` in data-schemas/methods/skill.ts with strict mode — unknown keys are rejected so expanding the allowed set is an intentional code change. Known keys are type-checked against a `FrontmatterKind` table derived from Anthropic's Agent Skills spec (name, description, when-to-use, allowed-tools, arguments, argument-hint, user-invocable, disable-model-invocation, model, effort, context, agent, paths, shell, hooks, version, metadata). - `hooks` and `metadata` get a shallow JSON-safety check (max depth 4, max string 2000, max array 100) instead of a full schema, since their full shapes live outside this module. - Wired into BOTH createSkill AND updateSkill so the PATCH path can't smuggle invalid frontmatter past the validator. Validation warning tier (new): - Add optional `severity: 'error' | 'warning'` to `ValidationIssue` (defaults to error). `partitionIssues` splits an issue list into blocking errors and non-blocking warnings. - `createSkill` / `updateSkill` filter on errors for the throw check and return warnings in a new `warnings: ValidationIssue[]` field on their result objects (`CreateSkillResult` / `UpdateSkillResult`). - `validateSkillDescription` now emits a `TOO_SHORT` warning for descriptions under 20 chars — the primary triggering field, so a little coaching goes a long way. - `createHandler` / `patchHandler` in packages/api surface the warnings via a new `attachWarnings` helper that decorates the serialized response with a `warnings?: TSkillWarning[]` field. - `TSkill` gains an optional `warnings?: TSkillWarning[]` field documented as "present on POST/PATCH, never on GET". Reserved-name filter (tightened): - Replace the substring match (`.includes('anthropic')`) with prefix matching on `anthropic-` and `claude-` plus exact-match rejection of CLI slash-command collisions (help, clear, compact, model, exit, quit, settings, plus the bare `anthropic` / `claude` words). Both the pure validator (`methods/skill.ts`) and the Mongoose schema validator (`schema/skill.ts`) updated in lockstep; comments on each reference the other to prevent drift. - `research-anthropic-helper` and `about-claude` are now allowed; `anthropic-helper`, `claude-bot`, and `settings` are still rejected. Documentation: - Add docstrings on `ISkill`, `schema/skill.ts`, and `TSkill` explaining the semantics of `name` (Claude-visible identifier, kebab-case, stable), `displayTitle` (UI-only cosmetic label, NOT sent to Claude), `description` (highest-leverage trigger field), and `source` / `sourceMetadata` (reserved for phase 2+ external sync). - Add a detailed consistency comment on `bumpSkillVersionAndAdjustFileCount` explaining that it runs as a separate MongoDB operation from upsertSkillFile/deleteSkillFile, so `fileCount` can drift if the second op fails — options listed, tradeoff documented, phase 1 risk window noted as closed because upload is still stubbed. Tests: - data-schemas skill.spec.ts: destructure `{ skill, warnings }` from createSkill at every call site; add a TOO_SHORT warning test, a frontmatter strict-mode test, reserved-prefix tests (including positive cases for substring names that should pass), CLI reserved word tests, and a full `validateSkillFrontmatter` describe block covering unknown keys, type mismatches, and deep-nesting rejection. - api/server/routes/skills.test.js: bump default test description above the 20-char threshold, add a warning-emission test, add reserved-prefix + reserved-CLI-word tests, add an unknown-frontmatter- key test asserting the 400 response carries `issues` with `UNKNOWN_KEY`. * 📦 fix: Export CreateSkillResult from data-schemas Methods Index `CreateSkillResult` was defined in `methods/skill.ts` and consumed by `packages/api/src/skills/handlers.ts` but never re-exported from the methods barrel, so the type-check job failed with TS2724 "'@librechat/data-schemas' has no exported member named 'CreateSkillResult'". Rollup's bundle-mode build picked up the type via its internal resolver, but the standalone `tsc --noEmit` type-check ran against the package's public entrypoint and couldn't see it. Added the type import + export alongside the existing `UpdateSkillResult` export, which fixes the CI type-check without any runtime change.	2026-04-25 04:01:59 -04:00
Danny Avila	f7d59d3285	📦 chore: Update TypeScript Config for TS v7 (#12794) ... - Enabled `esModuleInterop` in `client/tsconfig.json` for better module compatibility. - Changed `moduleResolution` from `node` to `bundler` in `client/tsconfig.json`. - Set `noEmit` to `true` in several `tsconfig.json` files to prevent output generation. - Removed `baseUrl` from various `tsconfig.json` files to simplify path resolution. - Updated path mappings in multiple packages to reflect new directory structures. These changes aim to streamline TypeScript configurations and improve module resolution across the project.	2026-04-23 12:51:03 -04:00
Danny Avila	9ccc8d9bef	✨ v0.8.5 (#12727)	2026-04-22 13:10:19 -07:00
github-actions[bot]	997f765d6b	🌍 i18n: Update translation.json with latest translations (#12783) ... Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>	2026-04-22 12:44:30 -07:00
jschmetzer	40742f9191	🔏 fix: Prevent Browser Autofill From Silently Dropping MCP CustomUserVars on Save (#12770) ... * fix: prevent browser autofill from silently dropping MCP customUserVars on save The customUserVars form in CustomUserVarsSection renders each credential as a plain `<input type="text">` with no autofill guards. This caused two user-visible problems: 1. Browser password managers (Chrome's built-in, 1Password, LastPass, Bitwarden) treat the fields as savable and offer to store values, which undermines the per-user credential model -- the whole point of customUserVars is that each user's own secrets stay in their own session and backend-encrypted, not cached by the browser. 2. When autofill fills a field, it does so via DOM mutation that does NOT fire React's synthetic `onChange`. react-hook-form's Controller therefore never sees the value, the form state stays `""`, and on submit the backend receives an empty string for every affected field. The user's typed credentials are silently dropped -- LibreChat stores 16 encrypted-empty-string rows in pluginauths, tool calls subsequently fail with "API key not set" errors, and the UI shows an "Unset" pill next to fields the user is certain they filled in. The fix matches the pattern already used in `SidePanel/Builder/ActionsAuth.tsx` for the analogous actions credential input: type="new-password" autoComplete="new-password" plus vendor-specific ignore attributes for LastPass and 1Password: data-lpignore="true" data-1p-ignore="true" (Modern browsers ignore `autocomplete="off"` for password-shaped fields, so the vendor attributes are the reliable defence against LastPass and 1Password, which have their own heuristics.) No behavioural change beyond preventing autofill: the input still accepts typed or pasted values, react-hook-form still collects them, submit still writes to the backend. The difference is that the values now actually reach the submit handler. * test: add regression guard for MCP customUserVars autofill prevention Condenses the rationale comment on the credential `<Input>` and adds a render test asserting that the autofill-prevention attributes (`type`, `autoComplete`, `data-lpignore`, `data-1p-ignore`) remain on the rendered input. The underlying bug (browser DOM mutations bypassing React's synthetic onChange) is severe enough that a future refactor accidentally dropping these attributes would silently re-introduce credential data loss. --------- Co-authored-by: Danny Avila <danny@librechat.ai>	2026-04-22 08:35:14 -07:00
github-actions[bot]	2427edef90	🌍 i18n: Update translation.json with latest translations (#12711) ... Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>	2026-04-21 11:41:33 -04:00
Luna	8f39f31c87	🤝 fix: Normalize Empty Handoff Fields to Restore Default Fallback (#12707) ... * fix(agents): normalize handoff prompt and parameter values to ensure default instructions * fix(agents): address lint errors and codex review comments	2026-04-19 11:53:09 -04:00
Danny Avila	b579390287	📦 chore: npm audit & bump @librechat/agents to v3.1.67 (#12710) ... * chore: Update package-lock.json with new dependencies and version upgrades - Added new dependencies for @langchain/anthropic and @langchain/core, including @anthropic-ai/sdk and fast-xml-parser. - Updated existing dependencies for @librechat/agents, @opentelemetry/api-logs, @opentelemetry/core, and related packages to their latest versions. - Enhanced integrity checks and licensing information for new and updated packages. * chore: Update @librechat/agents dependency to version 3.1.66 in package.json and package-lock.json - Bumped the version of @librechat/agents from 3.1.65 to 3.1.66 across multiple package.json files to ensure consistency and access to the latest features and fixes. * chore: Update dompurify and fast-xml-parser dependencies to version 3.4.0 and 5.6.0 respectively - Bumped the version of dompurify across multiple package.json files to ensure consistency and access to the latest features and security fixes. - Updated fast-xml-parser to the latest version in relevant package.json files for improved functionality. * chore: Update @librechat/agents dependency to version 3.1.67 in package.json and package-lock.json - Bumped the version of @librechat/agents from 3.1.66 to 3.1.67 across multiple package.json files to ensure consistency and access to the latest features and fixes.	2026-04-16 22:44:00 -04:00
Danny Avila	034b672d0c	🫧 feat: Claude Opus 4.7 Reasoning Visibility (#12701) ... * 🫧 fix: Restore Claude Opus 4.7 Reasoning Visibility Claude Opus 4.7 omits `thinking` content from Messages API responses by default — empty thinking blocks still stream, but the `thinking` field is blank unless the caller passes `display: "summarized"` in the adaptive thinking config. This silenced the LibreChat "Thoughts" UI for Anthropic (and Anthropic-on-Bedrock) adaptive models. - Extend `ThinkingConfigAdaptive` in `packages/api/src/types/anthropic.ts` with an optional `display: 'summarized' | 'omitted'` field - Emit `{ type: 'adaptive', display: 'summarized' }` from `configureReasoning` in `packages/api/src/endpoints/anthropic/helpers.ts` - Emit `{ type: 'adaptive', display: 'summarized' }` from `bedrockInputParser` in `packages/data-provider/src/bedrock.ts` and update the local `ThinkingConfig` union - Update existing adaptive-thinking assertions to include the new field - Add dedicated tests asserting `display: 'summarized'` flows through both the Anthropic endpoint and the Bedrock parser See https://platform.claude.com/docs/en/about-claude/models/whats-new-claude-4-7#thinking-content-omitted-by-default * refactor: Gate `display: summarized` on Opus 4.7+ Narrow the reasoning-visibility opt-in to the models that actually omit thinking content by default, instead of applying it to every adaptive model. Pre-Opus-4.7 adaptive models (Opus 4.6, Sonnet 4.6) already return summaries, so sending the field is unnecessary noise. - Add `omitsThinkingByDefault(model)` in `packages/data-provider/src/bedrock.ts` that returns true only for Opus 4.7+ (including future majors like Opus 5+) - Bedrock parser now only attaches `display: 'summarized'` when the helper matches, keeping the adaptive object unchanged for older models - Anthropic endpoint `configureReasoning` uses the same helper so its emit path matches the Bedrock one - Tests: replace the blanket `display: 'summarized'` assertions with model-specific ones (Opus 4.7 gets it, Opus 4.6 / Sonnet 4.6 do not), add a dedicated `omitsThinkingByDefault` suite covering naming variants and future versions * feat: Configurable Thought Visibility for Anthropic Adaptive Models Expose the Anthropic `thinking.display` API field as a user-facing parameter so users can override the `auto` default (which stays as the Opus-4.7+ opt-in added earlier in this PR). Also fixes the CI type error by widening the adaptive thinking type assignment via a resolver helper that returns a properly-typed object. - Add `ThinkingDisplay` enum (`auto` | `summarized` | `omitted`) and matching zod schema in `packages/data-provider/src/schemas.ts` - Add `thinkingDisplay` to `tConversationSchema`, `anthropicSettings`, and the pick lists for Bedrock input/parser + Anthropic agent params - Add `resolveThinkingDisplay(model, explicit)` helper in `packages/data-provider/src/bedrock.ts` that returns the wire value or undefined (auto → model default, explicit → always honored) - `bedrockInputParser` now reads `thinkingDisplay` from input and emits `display` only when the resolver returns a value; strips the field on non-adaptive-model branches so it does not leak - `configureReasoning` in the Anthropic endpoint threads `thinkingDisplay` through, uses the resolver, and casts the adaptive config to `AnthropicClientOptions['thinking']` so the widened shape compiles against the stale installed SDK types - Add UI slider for `thinkingDisplay` in `parameterSettings.ts` next to `effort`, with three-position `com_ui_auto` / `com_ui_summarized` / `com_ui_omitted` labels - Add translation keys `com_endpoint_anthropic_thinking_display`, `com_endpoint_anthropic_thinking_display_desc`, `com_ui_summarized`, `com_ui_omitted` - Add tests: `resolveThinkingDisplay` suite (5 cases covering auto / explicit / unknown input), parser round-trip tests for all three modes on Opus 4.6 and Opus 4.7, Anthropic endpoint tests for explicit summarized/omitted overrides * fix: Drop `thinkingDisplay` When Adaptive Thinking Is Disabled If a user turns adaptive thinking off but had previously selected a `thinkingDisplay` value, the stale field was left in `additionalFields` and ended up merged into the Bedrock request's `additionalModelRequestFields`. That leaks a non-Bedrock key into the payload and can round-trip back into `llmConfig`. - Delete `additionalFields.thinkingDisplay` alongside `thinking` and `thinkingBudget` in the `thinking === false` branch of `bedrockInputParser` - Add a regression test asserting `thinking`, `thinkingBudget`, and `thinkingDisplay` are all absent when adaptive thinking is disabled on an Opus 4.7 request Reported by chatgpt-codex-connector on PR #12701. * refactor: Consolidate `ThinkingDisplay` Types and Preserve Persisted Display Address review findings on PR #12701: - [Codex P2] `bedrockInputSchema.transform` now extracts `thinking.display` from persisted `additionalModelRequestFields` back into the top-level `thinkingDisplay` field so explicit `'omitted'` round-trips through storage instead of being silently reverted to `'summarized'` on the next parse. - [Codex P2] `getLLMConfig` in the Anthropic endpoint now reads `.display` from a persisted `thinking` object (agents store the full Anthropic shape) and uses it as the fallback for `thinkingDisplay` when no top-level override is present. - [Audit #2] Collapse the three parallel wire-value types into a single `ThinkingDisplayWireValue = Exclude<ThinkingDisplay, 'auto'>` exported from `schemas.ts`; remove the duplicate `ThinkingDisplay` alias in `packages/api/src/types/anthropic.ts` (which collided with the enum name) and the `ThinkingDisplayValue` alias in `bedrock.ts`. - [Audit #3] Add `thinkingDisplay` to the `TEndpointOption` pick list next to `effort`. - [Audit #4] Add a TODO comment next to the `as AnthropicClientOptions['thinking']` cast explaining the stale `@librechat/agents` SDK types that require it. - Add tests: four round-trip cases asserting `bedrockInputSchema` recovers `display` from persisted AMRF (Opus 4.7 omitted, pre-4.7 summarized, unknown-value ignore, explicit top-level wins), and two `getLLMConfig` cases asserting the Anthropic endpoint preserves and overrides persisted `thinking.display`. * fix: Preserve Persisted `thinking.display` in bedrockInputParser The parser constructed a fresh adaptive thinking config without looking at any `display` already embedded in the incoming `additionalModelRequestFields.thinking`. On round-trip through `initializeBedrock`, a persisted user choice of `'omitted'` on Opus 4.7+ was silently reverted to `'summarized'` by the auto fallback. - Extract `extractPersistedDisplay` helper and reuse it in both the schema transform (form-state round-trip) and the parser (wire-request round-trip) - `bedrockInputParser` now feeds the persisted display as the resolver's explicit value when no top-level `thinkingDisplay` override is set - Add regression tests: parser preserves `display: 'omitted'` for persisted Opus 4.7 AMRF, and top-level `thinkingDisplay` still wins over persisted AMRF display Reported by chatgpt-codex-connector (P1) on PR #12701.	2026-04-16 21:56:52 -04:00
Danny Avila	49f228de78	🔼 refactor: Improve UX for Command Popovers (#12677) ... * refactor: Improve UX for Command Popovers * Added loading state handling in Mention and PromptsCommand components to display a spinner when data is being fetched. * Refactored onFocus logic to clear the textarea and set the search value based on command character input. * Introduced a new `isLoading` state in the useMentions hook to manage loading indicators across multiple data queries. * Added unit tests for the useHandleKeyUp hook to ensure command triggering works correctly under various conditions. * ci: useHandleKeyUp tests for command navigation * Added tests to ensure that the command popovers do not trigger when the cursor is mid-text after pressing ArrowLeft or Delete. * Updated the shouldTriggerCommand function to refine the conditions under which commands are triggered based on cursor position. * Improved agent query handling in useMentions hook for better performance and clarity. * refactor: Optimize Mention and PromptsCommand Components * Refactored Mention and PromptsCommand components to utilize Recoil state for popover visibility, improving state management and reducing prop drilling. * Simplified onFocus logic to enhance user experience when interacting with command inputs. * Added unit tests for useHandleKeyUp to ensure proper command handling and popover visibility based on user input. * Improved performance by memoizing popover state and reducing unnecessary re-renders. * fix: Address review findings for command popover refactor - Fix endpointType regression: add effectiveEndpointByIndex selector that returns endpointType ?? endpoint, matching the original ChatForm guard for custom endpoints proxying assistants - Extract duplicated initInputRef callback into shared useInitPopoverInput hook, used by both Mention and PromptsCommand - Add navigation keys (ArrowLeft, ArrowRight, ArrowDown, Home, End, Delete) to invalidKeys to prevent false popover triggers - Add endpoint gating tests for assistants/azureAssistants blocking the + command - Remove unused _index param from MentionContent	2026-04-15 17:47:24 -04:00
Danny Avila	dd26a2fda5	🧩 style: Agent Side Panel Layout and Consistency Fixes (#12676) ... * style: Update padding in ActionsPanel, ModelPanel, and AdvancedPanel for improved layout * Adjusted padding in ActionsPanel, ModelPanel, and AdvancedPanel components to enhance visual consistency and layout. * Changed `py-4` to `pt-2` in the main container of each panel to reduce vertical spacing and improve overall design aesthetics. * style: Update text size in MCPTool component for improved accessibility * Changed text size in the MCPTool component to `text-sm` for better readability and consistency across the UI. * This adjustment enhances the user experience by ensuring that text is appropriately sized for various display settings. * style: Enhance layout and accessibility in ActionsInput, ActionsPanel, and AgentPanel components * Updated the layout in ActionsInput to improve flex properties and ensure better responsiveness. * Refined the structure of ActionsPanel for a more consistent visual hierarchy and added accessibility features. * Adjusted the AgentPanel form layout for improved usability and streamlined component integration. * Changed text size in Dropdown component to `text-sm` for better readability across the UI. * style: Refactor layout in ActionsInput component for improved responsiveness * Updated the layout of the ActionsInput component to enhance flex properties and ensure better responsiveness. * Adjusted the textarea styling for improved usability and consistency in design. * Removed commented-out code related to example functionality to clean up the component structure. * refactor: Simplify single line code detection in MarkdownComponents * Introduced a new utility function `isSingleLineCode` to streamline the logic for determining if code is a single line. * Updated references in the `MarkdownCode` and `MarkdownCodeNoExecution` components to use the new utility function for improved readability and maintainability. * Enhanced the `processChildren` function in the Markdown editor to handle non-code elements more effectively.	2026-04-15 14:27:13 -04:00
Danny Avila	b40e8be7c8	🖼️ fix: Hide Duplicate Image Placeholder During Image Generation (#12654) ... * fix: Hide duplicate image placeholder during image generation * test: Update OpenAIImageGen tests for conditional Image rendering	2026-04-14 07:53:23 -04:00
github-actions[bot]	5d108df665	🌍 i18n: Update translation.json with latest translations (#12646) ... Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>	2026-04-13 22:32:02 -04:00
Danny Avila	60cee6eec7	🔍 fix: Anthropic Web Search Multi-Turn Issue and Attachment Results (#12651) ... * 🔍 fix: Improve WebSearch Progress Handling Based on Attachment Results - Adjusted progress handling in the WebSearch component to treat searches as complete if attachments contain results, addressing issues with server tool calls not receiving completion signals. - Introduced `effectiveProgress` to reflect the actual state of progress based on the presence of results, enhancing the accuracy of cancellation and completion states. - Updated related logic to ensure proper handling of search completion and finalization states based on the new progress calculations. * fix: only override progress when not streaming During streaming (isSubmitting=true), use actual progress so the searching/processing/reading states display correctly. Only override to 1 after streaming completes to prevent the cancelled check from hiding the component. * chore: Update @librechat/agents and mathjs dependencies to latest versions * chore: Upgrade mathjs dependency to version 15.2.0 across package-lock and package.json files	2026-04-13 15:44:41 -04:00
Phạm Trung Nam	5cc783b8e8	🎯 fix: Preserve Selected Artifact When Clicking Artifact Button (#12601) ... * fix: preserve selected artifact when clicking artifact button * fix: preserve artifact selection on click and during streaming * fix: remove broken streaming guard, add JSDoc and test - Remove `userHasManualSelection` guard from effect #3: it cannot distinguish manual clicks from system auto-selection, blocking auto-advancement to new artifacts during streaming. - Add JSDoc on `currentArtifactIdRef` explaining why it must not be added to effect deps (toggle-close regression). - Add test verifying auto-advancement during streaming. * style: use standard multi-line JSDoc format for ref comment --------- Co-authored-by: Danny Avila <danny@librechat.ai>	2026-04-13 10:37:30 -04:00
Danny Avila	7b48203906	🗂️ feat: Sidebar Icon Toggle & New Chat History Switch (#12642) ... * 🗂️ feat: Sidebar Icon Toggle & New Chat History Switch Add collapse-on-active-click for sidebar icons (VSCode-style) and optionally switch to Chat History panel when creating a new chat. * fix: Address review findings — extract DEFAULT_PANEL constant, add tests Export DEFAULT_PANEL from ActivePanelContext and use it in ExpandedPanel instead of hardcoding 'conversations'. Add ExpandedPanel tests covering NavIconButton collapse toggle and NewChatButton panel switch behaviors. * fix: Address review — prop-drill setActive, test disabled setting, strengthen assertions Pass setActive as a prop to NewChatButton instead of subscribing to ActivePanelContext, avoiding wasted re-renders on every panel switch. Add negative-path test for switchToHistory=false. Add positive panel assertions to inactive-icon click tests. Fix import order.	2026-04-13 09:46:38 -04:00
Danny Avila	55840286d4	🔬 fix: Scope Web Search Results to Own Turn (#12631) ... * 🔬 fix: Scope Web Search Results to Own Turn in WebSearch Component Fix duplicated search results when multiple web_search tool calls occur in a single response. Each WebSearch instance now displays only its own turn's results instead of aggregating all turns. * test: Add WebSearch turn-scoping tests Cover the regression-prone turn isolation logic: verify each WebSearch instance renders only its own turn's sources when sharing a SearchContext, test the attachments-first priority path and the searchResults fallback, and assert component states (cancelled, error, streaming, complete). * fix: Use getAllByText for duplicate sr-only + visible text in tests WebSearch renders status text in both an sr-only aria-live region and a visible span, causing getByText to fail with multiple matches. * refactor: Make ownTurn a string to avoid repeated conversions	2026-04-12 18:38:26 -04:00
Danny Avila	4f133f8955	✨ v0.8.5-rc1 (#12569)	2026-04-09 20:06:31 -04:00
UnicronBE	46b529a86a	📩 fix: Restore Primary Action Button Visibility in Light Mode (#12591) ... The default <Button> variant (bg-primary / text-primary-foreground) renders invisible in light mode on the Agent Marketplace 'Start Chat' button and the Grant Access dialog 'Save Changes' button, making these primary actions undiscoverable without hovering. Switch these three affirmative-action buttons to variant='submit', which uses the hardcoded 'bg-surface-submit text-white' combination already defined in the Button component specifically to avoid the contrast issues of the default variant (see the comment above the 'submit' variant in packages/client/src/components/Button.tsx). No visual change in dark mode; in light mode the buttons now render as the green submit color with white text, matching the semantic intent of the action. Co-authored-by: Timothy Look <timothy.look@pmv.eu>	2026-04-09 18:45:54 -04:00
Marco Beretta	1a83f36cda	📌 feat: Add Pin Support for Model Specs (#11219) ... * feat: Enhance favorites functionality to support model specs * refactor(FavoritesList): reorder imports based on lenght * feat: improve favorite modelSpec controller; refactor: useIsActiveItem hook * refactor: consolidate Favorite type, harden controller, add tests - Add canonical TUserFavorite type in data-provider, replace three duplicate definitions (data-service, data-schemas, store/favorites) - Consolidate FavoritesController spec validation into single block, add return on 500 paths, add maxlength to mongoose sub-schema - Fix import order in FavoritesList.tsx, merge namespace type imports in FavoriteItem.tsx and FavoritesList.tsx - Add focus-visible:ring-inset on pin buttons to prevent ring clipping - Add explicit return type and JSDoc on useIsActiveItem hook - Use props.type for narrowing consistency in FavoriteItem getTypeLabel - Add 22 backend tests for FavoritesController (spec validation, typeCount exclusivity, persistence, GET path) - Add 40 frontend tests: useFavorites spec methods, useIsActiveItem observer lifecycle, ModelSpecItem pin button, FavoriteItem all three type branches, FavoritesList spec rendering * fix: address PR review findings for pin model specs - Harden backend validation to reject partial cross-type fields (e.g. spec+endpoint, agentId+model without endpoint) - Add stale-spec auto-cleanup in FavoritesList mirroring agent cleanup - Add type="button" to pin buttons in ModelSpecItem/EndpointModelItem - Fix import order violations in EndpointModelItem and ModelSpecItem - Remove hollow test, dead key prop, inline trivial helpers - Fix misleading test description, add onSelectSpec to test mock - Add return to controller success responses for consistency - Add 6 backend tests for partial cross-type field validation * fix: guard stale-spec cleanup against unloaded startupConfig Prevents race condition where spec favorites are incorrectly deleted on cold start before startupConfig has loaded. Mirrors the existing agentsMap === undefined guard pattern used for stale agent cleanup. Also adds tests for stale-spec cleanup persistence and fixes namespace import pattern in FavoritesList.spec.tsx. * fix: replace nested ternaries with if/else in FavoriteItem Resolves ESLint no-nested-ternary warnings for name and typeLabel derivations. --------- Co-authored-by: Danny Avila <danny@librechat.ai>	2026-04-09 18:37:25 -04:00
Danny Avila	daa8f0ea6b	📂 fix: Respect supportedMimeTypes Config in File Picker Accept Filter (#12596) ... * fix: respect supportedMimeTypes config in file picker accept filter The browser file picker's accept attribute was hardcoded by provider identity, ignoring the endpoint's supportedMimeTypes from fileConfig. Users who configured permissive MIME types (e.g., '.*') still saw a restrictive filter in the upload dialog. Add isPermissiveMimeConfig utility that detects wildcard patterns in the endpoint's supportedMimeTypes. When permissive, the file picker accept attribute is set to empty (unrestricted). Non-permissive configs retain the existing provider-based defaults. Closes #12589 * fix: address review findings for isPermissiveMimeConfig - Use non-standard MIME namespace probe (x-librechat/x-probe) so category-wildcard patterns like ^application\/.*$ no longer false-positive as permissive - Add single-line JSDoc to isPermissiveMimeConfig - Use !== undefined instead of != null (fileType is never null) - Add endpointFileConfig to dropdownItems useMemo deps to prevent stale closure when config changes without endpoint change - Add tests for broad application and multi-category patterns * fix: wrap handleUploadClick in useCallback to satisfy exhaustive-deps handleUploadClick is captured inside the dropdownItems useMemo but was not in its dependency array. Wrap it in useCallback with endpointFileConfig.supportedMimeTypes as the sole dependency, then reference the stable callback in the useMemo deps.	2026-04-09 17:43:53 -04:00
github-actions[bot]	0d97f7354a	🌍 i18n: Update translation.json with latest translations (#12588) ... Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>	2026-04-09 15:26:37 -04:00
github-actions[bot]	72cdeb0437	🌍 i18n: Update translation.json with latest translations (#12583) ... Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>	2026-04-09 07:45:13 -04:00
Marco Beretta	b8158a613e	🧑‍🎨 refactor: Prompts/Sidebar styles for improved UI Consistency (#12426) ... * style(sidebar): polish button styles, icon sizes, and nav separator - Match new chat button style to sidebar toggle - Unify icon sizes and stroke weights across sidebar icons - Add separator between new chat and nav links feat(prompts): inline prompt editing with route-based navigation - Show prompts dashboard inline when editing/creating from chat - Use route-based navigation instead of state-driven view switching - Strip inline view to form-only without duplicate sidebar style(prompts): unify borders, backgrounds, and admin controls - Unify borders to border-medium and remove opaque backgrounds - Add admin/advanced controls to sidebar - Match chat background, move advanced toggle to versions panel refactor(prompts): consolidate list actions into dropdown with compact layout - Unify prompt list to single ChatGroupItem with all actions - Consolidate actions into dropdown menu - Distinct dropdown icons with status icon tooltips - Remove rename action, use Trash icon matching convo items refactor(prompts): inline-edit title with click-to-edit and save indicator - Click title text to enter edit mode with pencil icon on hover - Clean title input with inline save status indicator - Uniform h-9 header height, smaller title, icon-only save status - Remove separate edit icon button and confirm/cancel controls style(prompts): sticky click-to-edit pill with surface-primary background fix: preserve AuthContext identity across Vite HMR updates createContext() was re-executed on every HMR module replacement, creating a new context object that disconnected the existing provider from its consumers. useAuthContext() would throw because useContext(newContext) returned undefined while the provider was still on the old context. Stash the context object in import.meta.hot.data so it survives HMR re-execution. Dead-code-eliminated in production builds. feat: add advanced prompts editor toggle to Chat settings - Add AdvancedPrompts switch in Settings > Chat that maps the promptsEditorMode enum (simple/advanced) to a boolean toggle - Preserve existing behavior: switching to simple forces alwaysMakeProd - Add focus styling to PromptName input (border-medium on focus, outline on focus-visible, no ring) - Add localization keys: com_nav_advanced_prompts, com_nav_advanced_prompts_desc - Various Prompts UI refinements style(sidebar): update nav icons and reorder links for clarity - Prompts: MessageSquareQuote → NotebookPen (distinct from chat bubble) - Agent Builder: Blocks → Bot (no longer identical to Assistants) - Memories: Database → BrainCircuit (brain + AI circuit) - Parameters: Settings2 → SlidersHorizontal (tuning-specific) - Reorder: Chat → Prompts → Agents → Assistants → Memories → Bookmarks → Files → Parameters → MCP (frequency and grouping) refactor(prompts): remove AdvancedSwitch component The simple/advanced toggle is now in Settings > Chat, making the standalone AdvancedSwitch redundant. Remove the component, its barrel exports, and all render sites (PromptForm, DashBreadcrumb, PromptsView). style(prompts): tighten panel layout and spacing to match sidebar conventions - Remove top margin from form wrapper, add mt-2 to header row - Reduce header gaps and remove flex-wrap for tighter alignment - Shrink prompt title from text-lg/h-9 to text-base/h-8 - Add pl-2 inner padding to PromptName input and static button - Compact versions panel and deploy button container padding - Reduce editor header and content spacing - Compact mobile versions panel header fix(prompts): rebuild mobile versions panel with proper transitions - Add background (bg-surface-primary-alt) and border-l so content doesn't bleed through - Replace conditional render with CSS opacity/translate so both overlay fade-out and panel slide-out animate on close - Use inert attribute to disable focus when panel is hidden - Replace raw × character with X icon from lucide-react - Use size-icon button variant for close instead of text sm - Use Tailwind classes (w-80, translate-x-full) instead of inline style object for width and transform - Remove unused sidePanelWidth constant - Overlay stays in DOM with pointer-events-none when hidden chore: remove stale tooling artifacts Remove .bg-shell/manifest.json and .gsd symlink that were accidentally committed — these are local dev tooling files. fix: resolve review issues in PromptName and ChatGroupItem - Fix Escape/onBlur race in PromptName via cancelledRef guard - Clear existing timer before creating new one to prevent status flicker - Guard delete mutation against empty group id - Attach menuButtonRef to MenuButton for proper focus restoration after preview dialog closes fix: address review findings across prompts UI and sidebar PromptName: - Fix Enter key double-save: set skipBlurRef before inline save to prevent onBlur from re-firing saveName on input unmount - Add isError prop to distinguish mutation failure from success, preventing false 'saved' checkmark on error - Remove dead cn ternary that always evaluated to opacity-100 - Remove unused cn import PromptForm: - Pass isError from updateGroupMutation to PromptName - Remove dead default arg (= {}) on function component ChatGroupItem: - Stabilize handleDelete via ref pattern to prevent memo-defeating recreation on every render (deleteGroup is a new ref each render) - Disable delete button during mutation to prevent double-fire ExpandedPanel: - Restore <a> element for NewChatButton to fix middle-click (open in new tab) regression caused by anchor-to-button migration DashGroupItem: - Delete dead file (no longer exported or imported anywhere) fix: address review findings across prompts UI and sidebar - PromptName: render error state (red X) when save fails, extract shared commitName helper to deduplicate blur/Enter save paths - ChatGroupItem: navigate away after deleting the active prompt in sidebar view; use context-aware route prefix (/prompts vs /d/prompts) for edit and card-click navigation - InlinePromptsView: redirect to /c/new when user lacks prompts access instead of rendering a blank screen - Remove dead ManagePrompts component and its barrel exports (no remaining consumers after GroupSidePanel cleanup) fix: remove duplicate showThinking Recoil atom key The 'showThinking' key was defined in both store/settings.ts (Recoil) and store/showThinking.ts (Jotai). Only the Jotai atom is consumed; the stale Recoil duplicate causes 'A key option with a unique string value must be provided' at startup. refactor: remove /d/prompts dashboard route and dead code The prompts UI now lives at /prompts/* inline under the chat layout. The old /d/prompts/* dashboard route, its layout (PromptsView), and its breadcrumb (DashBreadcrumb) are no longer used. - Delete PromptsView and DashBreadcrumb (zero consumers) - Delete BackToChat button (zero consumers) - Replace /d/prompts/* child routes with a redirect to /prompts/new - Add /prompts index route that redirects to /prompts/new - Update all /d/prompts navigation to /prompts: - ChatGroupItem: always use /prompts prefix - NoPromptGroup: navigate to /prompts - CreatePromptForm: fallback navigate to /prompts/:id - CreatePromptButton: simplified to /prompts/new (no dual-path) - Strip GroupSidePanel of dashboard-only breadcrumb nav, recoil state clearing, and useDashboardContext dependency refactor: remove dead Dashboard code and unused translation keys - Delete DashboardContext provider (zero remaining consumers) - Simplify DashboardRoute layout to a plain Outlet - Remove commented-out file/vector-store route blocks - Change catch-all redirect from /d/files to /c/new - Remove 7 unused translation keys (com_nav_toggle_sidebar, com_ui_back_to_chat, com_ui_dashboard, com_ui_delete_prompt_name, com_ui_global_group, com_ui_prompt_renamed, com_ui_rename_prompt, com_ui_rename_prompt_name) fix: add Babel plugin to transform import.meta.hot for Jest `babel-plugin-transform-import-meta` handles standard properties (url, filename, dirname, resolve) but not Vite's `hot` property. Jest runs in CommonJS where `import.meta` is unavailable, so `import.meta.hot` in AuthContext.tsx (added for HMR preservation) causes a SyntaxError that breaks 27 test suites. Add a small Babel plugin that replaces `import.meta.hot` with `undefined` during Jest transforms, making the HMR guard blocks dead-code in tests. fix: address review findings — inert typing, accessibility, and cleanup - Add React type augmentation for `inert` attribute (React 18 compat) - Replace spread hack `{...{ inert: }}` with direct prop in all 3 files - Add `aria-hidden` to mobile overlay in PromptForm for screen readers - Simplify deleteGroupRef pattern to direct mutation call - Remove unused `useEffect` import and stale useMemo dependency - Add clarifying comment for skipBlurRef mechanism in PromptName fix: mobile UX for marketplace and prompts views - Remove mobile new chat button from chat history section - Add OpenSidebar entry points to marketplace and prompts views on mobile - Move marketplace admin settings to a compact mobile top row - Restructure prompt forms to surface category selector beside sidebar toggle - Make versions panel slide content like the main sidebar and drop redundant borders - Collapse versions button to icon-only on mobile - Remove theme selector from prompt panel navigation * fix: address PR review findings for prompts refactor - Preserve prompt ID in /d/prompts/:id → /prompts/:id redirect - Gate PreviewPrompt and VariableDialog behind isChatRoute to avoid mounting dead dialogs in dashboard mode - Add onError handler to useDeletePromptGroup and close dialog on success - Use useId() instead of hardcoded labelId in AdvancedPrompts - Extract shared lazy loader for InlinePromptsView routes * fix: complete review fixes for prompts refactor - Move OGDialog (delete) inside isChatRoute gate with other dialogs - Use useId() for both Switch id and label id in AdvancedPrompts - Add com_ui_prompt_delete_error i18n key for actionable error context - Drop no-op useCallback on handleDelete (unstable deleteGroup dep) * refactor: hoist promptPath to module-scope constant Eliminates stale-closure lint concern in dropdownItems useMemo and removes the unnecessary dep array entry from onCardClick. * refactor(sidebar): update icons and reorder links for clarity - Replace Blocks icon with OpenAIMinimalIcon for the Assistant Builder link. - Update Memories icon from BrainCircuit to Brain. - Reintroduce Prompts link conditionally based on access permissions. - Change Conversations icon from MessageSquare to MessagesSquare for consistency. * refactor(sidebar): update icons and improve file attachment link - Replace NewChatIcon with SquarePen in the NewChatButton for better visual consistency. - Change AttachmentIcon to Paperclip in the file attachment link for clarity. * refactor(sidebar): update file attachment icon for consistency - Replace Paperclip icon with AttachmentIcon in the file attachment link for improved clarity and visual consistency. * refactor(admin-settings): remove unused button and streamline dialog integration - Eliminate the Admin button and its associated icon from the AdminSettings component for a cleaner interface. - Simplify the confirm dialog integration by directly using the OGDialog without the button trigger. * fix: context HMR issue * style(prompts): enhance component structure and accessibility - Update AutoSendPrompt button class for improved styling. - Refactor List component to streamline loading and empty states. - Ensure FilterPrompts handles context gracefully with null checks. - Modify GroupSidePanel to prevent rendering without context. - Simplify PromptsAccordion layout for better readability. - Adjust CategoryIcon fallback behavior for undefined categories. * refactor(useMCPServerManager): clean up import statements - Remove duplicate import of MCPServerInitState for better clarity and organization. - Adjust import order to maintain consistency with project structure. * refactor(GroupSidePanel): restructure layout for improved readability and accessibility - Adjust the structure of the GroupSidePanel component to enhance layout clarity. - Move the PanelNavigation component into a more appropriate position within the hierarchy. - Ensure consistent styling and behavior based on the isChatRoute condition. * style(GroupSidePanel): adjust padding for improved layout consistency - Update padding in the GroupSidePanel component to enhance visual alignment and readability. - Ensure consistent styling across the component for a better user experience. * fix(Conversations): add cache clearing and row height recomputation on search query change - Implement useEffect to clear cache and recompute row heights when the search query changes. - Enhance performance and responsiveness of the Conversations component during search operations. * refactor(GroupSidePanel, PromptsAccordion): simplify layout and improve styling * chore: import order * fix: redirect users without CREATE permission from /prompts/new Users with USE but not CREATE permission were seeing a blank page at /prompts/new because InlinePromptsView only checked USE access. CreatePromptForm's internal redirect was bypassed by the onSuccess prop always being passed. Add CREATE check in InlinePromptsView so the redirect happens before CreatePromptForm mounts. * fix: restore dropdown actions for all routes and handle non-creator landing - Remove isChatRoute gate on dropdown menu so preview, edit, and delete actions are available on the prompts management route - Un-gate PreviewPrompt and OGDialog (delete) since both are triggered from the now-always-visible dropdown - Keep VariableDialog gated behind isChatRoute (chat submission only) - Show EmptyPromptPreview for non-creators at /prompts/new instead of redirecting to /c/new, so they stay in the prompts section with sidebar access to browse existing prompts * fix: add isPublic to TPromptGroup type The database schema (IPromptGroup in data-schemas) has isPublic but the shared TPromptGroup type in data-provider was missing it, causing a TS2339 error in ChatGroupItem. * fix: prevent duplicate rename and restore name on error in PromptName - Block re-entry to edit mode while a save is in flight by guarding the click handler with isLoading/saveStatus checks - Reset newName to the prop value when mutation fails so the UI doesn't display the unsaved name after the error icon clears * fix: address review findings across prompts refactor - Consolidate duplicate usePromptGroupsContext() calls in PromptForm - Remove invalid aria-labelledby (text string, not ID) from AutoSendPrompt checkbox that is already aria-hidden - Remove useMemo wrapping trivial `disabled ?? false` in ToolsDropdown - Remove dead context spread in PromptsAccordion (GroupSidePanel reads context internally) - Wrap search cache-clear effect in requestAnimationFrame to match favorites effect pattern in Conversations - Use Set for O(1) lookups in MCPSelect server filtering - Fix unnecessary JSX expression wrapper on string literal in CreatePromptButton Link * style(PromptTextCard): update icon classes for improved accessibility - Add 'text-text-secondary' class to Check and Copy icons for better visibility and consistency in the PromptTextCard component. --------- Co-authored-by: Danny Avila <danny@librechat.ai>	2026-04-09 00:02:31 -04:00
github-actions[bot]	c940486a06	🌍 i18n: Update translation.json with latest translations (#12571) ... Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>	2026-04-08 09:12:12 -04:00
Danny Avila	d350c58633	🚫 fix: Hide Delete Account Button When ALLOW_ACCOUNT_DELETION Is Disabled (#12568) ... * fix: hide Delete Account button when ALLOW_ACCOUNT_DELETION is false * fix: add admin bypass, inline env read, and tests for allowAccountDeletion - Show delete button for admin users even when ALLOW_ACCOUNT_DELETION=false, matching the canDeleteAccount middleware's ACCESS_ADMIN bypass - Move env var read inline in buildSharedPayload() for per-request evaluation - Add 4 frontend tests for Account conditional rendering - Add 3 backend tests for allowAccountDeletion config field * fix: use server-side ACCESS_ADMIN capability check instead of frontend role check - Replace frontend SystemRoles.ADMIN check with server-side hasCapability() in the authenticated config route, matching canDeleteAccount middleware exactly - Admin bypass now evaluates ACCESS_ADMIN capability per-user in GET /api/config, so users with the grant (regardless of role) see the button, and admins without the grant do not - Add 3 authenticated backend tests: without capability, with capability, and skip-when-already-enabled - Simplify frontend to pure config check (no role logic) - Remove redundant jest-dom import; add inline env var comment * test: add missing toHaveBeenCalled assertion in ACCESS_ADMIN test	2026-04-07 23:51:23 -04:00
Danny Avila	b44ce264a4	📦 chore: Bump mongodb-memory-server to v11.0.1, mermaid to v11.14.0, npm audit (#12543) ... * 🔧 chore: Update `mongodb-memory-server` to v11.0.1 - Bump `mongodb-memory-server` version in `package-lock.json`, `api/package.json`, and `packages/data-schemas/package.json` from 10.1.4 to 11.0.1. - Update related dependencies in `mongodb-memory-server` and `mongodb-memory-server-core` to ensure compatibility with the new version. - Adjust `tslib` version in `mongodb-memory-server` to 2.8.1 and `debug` to 4.4.3 for consistency. * chore: npm audit fix * chore: Update `mermaid` dependency to version 11.14.0 in `package-lock.json` and `client/package.json` * fix: use deterministic timestamps in convoStructure test MongoDB 8.x (from mongodb-memory-server v11) no longer guarantees insertion-order return for documents with identical timestamps. Use sequential timestamps with overrideTimestamp to ensure buildTree processes parents before children.	2026-04-03 17:01:11 -04:00
Daniel Lew	162ac9c253	📝 fix: Properly Restore Draft Text When Switching Conversations (#12384) ... Right now, if you have draft text in conversation A, but no draft text in conversation B, then switching from A -> B inserts the draft from A into B (oops). This was caused by a bug in the `restoreText()` logic which did not restore *blank* text as the saved draft. Now, it'll always restore whatever is found as a draft (or set to blank if there is no draft).	2026-04-03 14:08:21 -04:00
Dustin Healy	261941c05f	🔨 fix: Custom Role Permissions (#12528) ... * fix: Resolve custom role permissions not loading in frontend Users assigned to custom roles (non-USER/ADMIN) had all permission checks fail because AuthContext only fetched system role permissions. The roles map keyed by USER/ADMIN never contained the custom role name, so useHasAccess returned false for every feature gate. - Fetch the user's custom role in AuthContext and include it in the roles map so useHasAccess can resolve permissions correctly - Use encodeURIComponent instead of toLowerCase for role name URLs to preserve custom role casing through the API roundtrip - Only uppercase system role names on the backend GET route; pass custom role names through as-is for exact DB lookup - Allow users to fetch their own assigned role without READ_ROLES capability * refactor: Normalize all role names to uppercase Custom role names were stored in original casing, causing case-sensitivity bugs across the stack — URL lowercasing, route uppercasing, and case-sensitive DB lookups all conflicted for mixed-case custom roles. Enforce uppercase normalization at every boundary: - createRoleByName trims and uppercases the name before storage - createRoleHandler uppercases before passing to createRoleByName - All admin route handlers (get, update, delete, members, permissions) uppercase the :name URL param before DB lookups - addRoleMemberHandler uppercases before setting user.role - Startup migration (normalizeRoleNames) finds non-uppercase custom roles, renames them, and updates affected user.role values with collision detection Legacy GET /api/roles/:roleName retains always-uppercase behavior. Tests updated to expect uppercase role names throughout. * fix: Use case-preserved role names with strict equality Remove uppercase normalization — custom role names are stored and compared exactly as the user sets them, with only trimming applied. USER and ADMIN remain reserved case-insensitively via isSystemRoleName. - Remove toUpperCase from createRoleByName, createRoleHandler, and all admin route handlers (get, update, delete, members, permissions) - Remove toUpperCase from legacy GET and PUT routes in roles.js; the frontend now sends exact casing via encodeURIComponent - Remove normalizeRoleNames startup migration - Revert test expectations to original casing * fix: Format useMemo dependency array for Prettier * feat: Add custom role support to admin settings + review fixes - Add backend tests for isOwnRole authorization gate on GET /api/roles/:roleName - Add frontend tests for custom role detection and fetching in AuthContext - Fix transient null permission flash by only spreading custom role once loaded - Add isSystemRoleName helper to data-provider for case-insensitive system role detection - Use sentinel value in useGetRole to avoid ghost cache entry from empty string - Add useListRoles hook and listRoles data service for fetching all roles - Update AdminSettingsDialog and PeoplePickerAdminSettings to dynamically list custom roles in the role dropdown, with proper fallback defaults * fix: Address review findings for custom role permissions - Add assertions to AuthContext test verifying custom role in roles map - Fix empty array bypassing nullish coalescing fallback in role dropdowns - Add null/undefined guard to isSystemRoleName helper - Memoize role dropdown items to avoid unnecessary re-renders - Apply sentinel pattern to useGetRole in admin settings for consistency - Mark ListRolesResponse description as required to match schema * fix: Prevent prototype pollution in role authorization gate - Replace roleDefaults[roleName] with Object.hasOwn to prevent prototype chain bypass for names like constructor or __proto__ - Add dedicated rolesList query key to avoid cache collision when a custom role is named 'list' - Add regression test for prototype property name authorization * fix: Resolve Prettier formatting and unused variable lint errors * fix: Address review findings for custom role permissions - Add ADMIN self-read test documenting isOwnRole bypass behavior - Guard save button while custom role data loads to prevent data loss - Extract useRoleSelector hook eliminating ~55 lines of duplication - Unify defaultValues/useEffect permission resolution (fixes inconsistency) - Make ListRolesResponse.description and _id optional to match schema - Fix vacuous test assertions to verify sentinel calls exist - Only fetch userRole when user.role === USER (avoid unnecessary requests) - Remove redundant empty string guard in custom role detection * fix: Revert USER role fetch restriction to preserve admin settings Admins need the USER role loaded in AuthContext.roles so the admin settings dialog shows persisted USER permissions instead of defaults. * fix: Remove unused useEffect import from useRoleSelector * fix: Clean up useRoleSelector hook - Use existing isCustom variable instead of re-calling isSystemRoleName - Remove unused roles and availableRoleNames from return object * fix: Address review findings for custom role permissions - Use Set-based isSystemRoleName to auto-expand with future SystemRoles - Add isCustomRoleError handling: guard useEffect reset and disable Save - Remove resolvePermissions from hook return; use defaultValues in useEffect to eliminate redundant computation and stale-closure reset race - Rename customRoleName to userRoleName in AuthContext for clarity * fix: Request server-max roles for admin dropdown listRoles now passes limit=200 (the server's MAX_PAGE_LIMIT) so the admin role selector shows all roles instead of silently truncating at the default page size of 50. --------- Co-authored-by: Danny Avila <danny@librechat.ai>	2026-04-03 13:24:11 -04:00
Daniel Lew	936936596b	🔍 fix: only show Searchbar if enabled (#12424) ... The search bar was showing even if you have no search capability; now it respects the `enabled` field.	2026-04-03 12:24:59 -04:00
Danny Avila	ea28dbfa89	🧹 chore: Clean Up Config Fields (#12537) ... * chore: remove unused `interface.endpointsMenu` config field * chore: address review — restore JSDoc UI-only example, add Zod strip test * chore: remove unused `interface.sidePanel` config field * chore: restrict fileStrategy/fileStrategies schema to valid storage backends * fix: use valid FileStorage value in AppService test * chore: address review — version bump, exhaustiveness guard, JSDoc, configSchema test * chore: remove debug logger.log from MessageIcon render path * fix: rewrite MessageIcon render tests to use render counting instead of logger spying * chore: bump librechat-data-provider to 0.8.407 * chore: sync example YAML version to 1.3.7	2026-04-03 12:22:58 -04:00
Danny Avila	b4d97bd888	🗜️ refactor: Eliminate Unstable React Keys During SSE Lifecycle (#12536) ... * debug: add instrumentation to MessageIcon arePropsEqual + render cycle tests Temporary debug commit to identify which field triggers MessageIcon re-renders during message creation and streaming. arePropsEqual now logs 'icon_memo_diff' with the exact field name and prev/next values whenever it returns false. Filter browser console for 'icon_memo_diff' to see the trigger. Also adds render-level integration tests that simulate the message lifecycle (initial mount, streaming chunks, context updates) and assert render counts via logger spy. * perf: stabilize MultiMessage key to prevent unmount/remount during SSE lifecycle messageId changes 3 times during the SSE message lifecycle: 1. useChatFunctions creates initialResponse with client-generated UUID 2. createdHandler replaces it with userMessageId + '_' 3. finalHandler replaces it with server-assigned messageId Since MultiMessage used key={message.messageId}, each change caused React to destroy and recreate the entire message component subtree, unmounting MessageIcon and all memoized children. This produced visible icon/image flickering that no memo comparator could prevent. Switch to key={parentMessageId + '_' + siblingIdx}: - parentMessageId is stable from creation through final response - siblingIdx ensures sibling switches still get clean remounts - Eliminates 2 unnecessary unmount/remount cycles per message Add key stability tests verifying: - Current key={messageId} causes 3 mounts / 2 unmounts per lifecycle - Stable key causes 1 mount / 0 unmounts per lifecycle - Sibling switches still trigger clean remounts with stable key * perf: stabilize root MultiMessage key across new conversation lifecycle When a user sends their first message in a new conversation, conversationId transitions from null/'new' to the server-assigned UUID. MessagesView used key={conversationId} on the root MultiMessage, so this transition destroyed the entire message tree and rebuilt it from scratch — causing all MessageIcons to unmount/remount (visible as image flickering). Use a ref-based stable key that captures the first real conversationId and only changes on genuine conversation switches (navigating to a different conversation), not on the null→UUID transition within the same conversation. * debug: add mount/unmount lifecycle tracking to MessageIcon Adds icon_lifecycle logs (MOUNT/UNMOUNT) and render count to distinguish between fresh mounts (memo comparator not called) and internal re-renders (hook bypassing memo). Enable: localStorage.setItem('DEBUG_LOGGING', 'icon_lifecycle,icon_data,icon_memo_diff') * debug: add key and root tracking to MultiMessage and MessagesView Logs multi_message_key (stableKey, messageId, parentMessageId, route) and messages_view_key (rootKey, conversationId) to trace which key changes trigger unmount/remount cycles. Enable: localStorage.setItem('DEBUG_LOGGING', 'icon_lifecycle,icon_data,icon_memo_diff,multi_message_key,messages_view_key') * perf: remove key from root MultiMessage to prevent tree destruction The ref-based stable key still changed during 'new' → real UUID transition, destroying the entire tree. The root MultiMessage is the sole child at its position, so React reuses the instance via positional reconciliation without any key. The messageId prop (conversationId) naturally resets Recoil siblingIdxFamily state on conversation switches. * perf: remove unstable keys from MultiMessage to prevent SSE lifecycle remounts Both messageId and parentMessageId change during the SSE lifecycle (client UUID → CREATED server ID → FINAL server ID), making neither viable as a stable React key. Each key change caused React to destroy and recreate the entire message component subtree, including all memoized children — visible as icon/image flickering. Remove explicit keys entirely and rely on React's positional reconciliation. MultiMessage always renders exactly one child at the same position, so React reuses the component instance and updates props in place. The existing memo comparators on ContentRender/MessageRender handle field-level diffing correctly. Update tests to verify: key={messageId} causes 3 mounts/2 unmounts per lifecycle, while no key causes 1 mount/0 unmounts. * perf: remove unstable keys from child MultiMessage in message wrappers Message.tsx, MessageContent.tsx, and MessageParts.tsx each render a child MultiMessage with key={messageId} for the current message's children. Since messageId changes during the SSE lifecycle (CREATED event replaces the user message ID), the child MultiMessage gets destroyed and recreated, unmounting the entire agent response subtree including its MessageIcon. Remove these keys for the same reason as the parent MultiMessage: each child MultiMessage renders exactly one child at a fixed position, so positional reconciliation correctly reuses the instance. * chore: remove MultiMessage key tests — they test React behavior, not our code The tests verified that key={messageId} causes remounts while no key doesn't, but this is React's own reconciliation behavior. No unit test can prevent someone from re-adding a key prop to JSX. The JSDoc comments on MultiMessage document the decision and rationale.	2026-04-02 22:29:54 -04:00
Danny Avila	275af48592	🎯 fix: MCP Tool Misclassification from Action Delimiter Collision (#12512) ... * fix: prevent MCP tools with `_action` in name from being misclassified as OpenAPI action tools Add `isActionTool()` helper that checks for the `_action_` delimiter while guarding against cross-delimiter collision with `_mcp_`. Replace all `includes(actionDelimiter)` classification checks with the new helper across backend and frontend. * test: add coverage for MCP/action cross-delimiter collision Verify that `isActionTool` correctly rejects MCP tool names containing `_action` and that `loadAgentTools` does not filter them based on `actionsEnabled`. Add ToolIcon and definitions test cases. * fix: simplify isActionTool to handle all MCP name patterns - Use `!toolName.includes('_mcp_')` instead of checking only after the first `_action_` occurrence, which missed MCP tools with `_action_` in the middle of their name (e.g. `get_action_data_mcp_myserver`). - Reference `Constants.mcp_delimiter` value via a local const to avoid circular import from config.ts, with a comment explaining why. - Remove dead `actionDelimiter` import from definitions.ts. - Replace double-filter with single-pass partition in loadToolsForExecution. - Add test for mid-name `_action_` collision case. * fix: narrow MCP exclusion to delimiter position in isActionTool Only reject when `_mcp_` appears after `_action_` (the MCP suffix position). `_mcp_` before `_action_` is part of the operationId and is valid — e.g. `sync_mcp_state_action_api---example---com` is a legitimate action tool whose operationId happens to contain `_mcp_`. * fix: document positional _mcp_ guard and known RFC-invalid domain limitation Expand JSDoc on isActionTool to explain the action/MCP format disambiguation and the theoretical false negative for non-RFC-compliant domains containing `_mcp_`. Add test documenting this known edge case.	2026-04-01 22:36:21 -04:00
Danny Avila	611a1ef5dc	🏖️ fix: Sandpack ExternalResources for Static HTML Artifact Previews (#12509) ... * fix: omit externalResources for static Sandpack previews The Tailwind CDN URL lacks a file extension, causing Sandpack's static template to throw a runtime injection error. Static previews already load Tailwind via a script tag in the shared index.html, so externalResources is unnecessary for them. Closes #12507 * refactor: extract buildSandpackOptions and add tests - Surgically omit only externalResources for static templates instead of discarding all sharedOptions, preventing future regression if new template-agnostic options are added. - Extract options logic into a pure, testable helper function. - Add unit tests covering all template/config combinations. * chore: fix import order and pin test assertions * fix: use URL fragment hint instead of omitting externalResources Sandpack's static template regex detects resource type from the URL's last file extension. The versioned CDN path (/3.4.17) matched ".17" instead of ".js", throwing "Unable to determine file type". Rather than omitting externalResources for static templates (which would remove the only Tailwind injection path for HTML artifacts that don't embed their own script tag), append a #tailwind.js fragment hint so the regex matches ".js". Fragments are not sent to the server, so the CDN response is unchanged.	2026-04-01 22:06:42 -04:00
Danny Avila	7b368916d5	🔑 fix: Auth-Aware Startup Config Caching for Fresh Sessions (#12505) ... * fix: auth-aware config caching for fresh sessions - Add auth state to startup config query key via shared `startupConfigKey` builder so login (unauthenticated) and chat (authenticated) configs are cached independently - Disable queries during login onMutate to prevent premature unauthenticated refetches after cache clear - Re-enable queries in setUserContext only after setTokenHeader runs, with positive-only guard to avoid redundant disable on logout - Update all getQueryData call sites to use the shared key builder - Fall back to getConfigDefaults().interface in useEndpoints, hoisted to module-level constant to avoid per-render recomputation * fix: address review findings for auth-aware config caching - Move defaultInterface const after all imports in ModelSelector.tsx - Remove dead QueryKeys import, use import type for TStartupConfig in ImportConversations.tsx - Spread real exports in useQueryParams.spec.ts mock to preserve startupConfigKey, fixing TypeError in all 6 tests * chore: import order * fix: re-enable queries on login failure When login fails, onSuccess never fires so queriesEnabled stays false. Re-enable in onError so the login page can re-fetch config (needed for LDAP username validation and social login options).	2026-04-01 17:20:39 -04:00
Danny Avila	7181174c3b	🖼️ fix: Message Icon Flickering from Context-Triggered Re-renders (#12489) ... * perf: add custom memo comparator to MessageIcon for stable re-render gating MessageIcon receives full `agent` and `assistant` objects as props from useMessageActions, which recomputes them when AgentsMapContext or AssistantsMapContext update (e.g., react-query refetch on window focus). These context-triggered re-renders bypass MessageRender's React.memo, producing new object references for agent/assistant even when the underlying data is unchanged. The default shallow comparison in MessageIcon's memo then fails, causing unnecessary re-renders that manifest as visible icon flickering. Add arePropsEqual comparator that checks only the fields MessageIcon actually uses (name, avatar filepath, metadata avatar) instead of object identity, so the component correctly bails out when icon-relevant data hasn't changed. * refactor: export arePropsEqual, drop redundant useMemos, add JSDoc - Export arePropsEqual so it can be tested in isolation - Add JSDoc documenting which fields are intentionally omitted (id) and why iconData uses reference equality - Replace five trivial useMemo calls (agent?.name ?? '', etc.) with direct computed values — the custom comparator already gates re-renders, so these memos only add closure/dep-array overhead without ever providing cache hits - Remove unused React import * test: add unit tests for MessageIcon arePropsEqual comparator Exercise the custom memo comparator to ensure: - New object references with same display fields are treated as equal - Changed name or avatar filepath triggers re-render - iconData reference change triggers re-render - undefined→defined agent with undefined fields is treated as equal * fix: replace nested ternary with if-else for eslint compliance * test: add comment on subtle equality invariant and defined→undefined case * perf: compare iconData by field values instead of reference iconData is a useMemo'd object from the parent, but comparing by reference still causes unnecessary re-renders when the parent recomputes the memo with identical primitive values. Compare all five fields individually (endpoint, model, iconURL, modelLabel, isCreatedByUser) for consistency with how agent/assistant are handled.	2026-03-31 18:24:18 -04:00
Danny Avila	d9f216c11a	📦 chore: bump dependabot packages (#12487) ... * chore: Update Handlebars and package versions in package-lock.json and package.json - Upgrade Handlebars from version 4.7.7 to 4.7.9 in both package-lock.json and package.json for improved performance and security. - Update librechat-data-provider version from 0.8.401 to 0.8.406 in package-lock.json. - Update @librechat/data-schemas version from 0.0.40 to 0.0.48 in package-lock.json. * chore: Upgrade @happy-dom/jest-environment and happy-dom versions in package-lock.json and package.json - Update @happy-dom/jest-environment from version 20.8.3 to 20.8.9 for improved compatibility. - Upgrade happy-dom from version 20.8.3 to 20.8.9 to ensure consistency across dependencies. * chore: Upgrade @rollup/plugin-terser to version 1.0.0 in package-lock.json and package.json - Update @rollup/plugin-terser from version 0.4.4 to 1.0.0 in both package-lock.json and package.json for improved performance and compatibility. - Reflect the new version in the dependencies of data-provider and data-schemas packages. * chore: Upgrade rollup-plugin-typescript2 to version 0.37.0 in package-lock.json and package.json - Update rollup-plugin-typescript2 from version 0.35.0 to 0.37.0 in package-lock.json and all relevant package.json files for improved compatibility and performance. - Adjust dependencies for semver and tslib to their latest versions in line with the rollup-plugin-typescript2 upgrade. * chore: Upgrade nodemailer to version 8.0.4 in package-lock.json and package.json - Update nodemailer from version 7.0.11 to 8.0.4 in both package-lock.json and package.json to enhance functionality and security. * chore: Upgrade picomatch, yaml, brace-expansion versions in package-lock.json - Update picomatch from version 4.0.3 to 4.0.4 across multiple dependencies for improved functionality. - Upgrade brace-expansion from version 2.0.2 to 2.0.3 and from 5.0.3 to 5.0.5 to enhance compatibility and performance. - Update yaml from version 1.10.2 to 1.10.3 for better stability.	2026-03-31 13:36:20 -04:00
github-actions[bot]	56d994e9ec	🌍 i18n: Update translation.json with latest translations (#12458) ... Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>	2026-03-29 18:43:43 -04:00
Danny Avila	7e2b51697e	🪢 refactor: Eliminate Unnecessary Re-renders During Message Streaming (#12454) ... * refactor: add TMessageChatContext type for stable context passing Defines a type for a stable context object that wrapper components pass to memo'd message components, avoiding direct ChatContext subscriptions that bypass React.memo during streaming. * perf: remove ChatContext subscription from useMessageActions useMessageActions previously called useChatContext() inside memo'd components (MessageRender, ContentRender), bypassing React.memo when isSubmitting changed during streaming. Now accepts a stable chatContext param instead, using a ref for the isSubmitting guard in regenerateMessage. Also stabilizes handleScroll in useMessageProcess by using a ref for isSubmitting instead of including it in useCallback deps. * perf: pass stable chatContext to memo'd message components Wrapper components (Message, MessageContent) now create a stable chatContext object via useMemo with a getter-backed isSubmitting, and compute effectiveIsSubmitting (false for non-latest messages). This ensures MessageRender and ContentRender (both React.memo'd) only re-render for the latest message during streaming, preventing unnecessary re-renders of all prior messages and their SubRow, HoverButtons, and SiblingSwitch children. * perf: add custom memo comparators to prevent message reference re-renders buildTree creates new message objects on every streaming update for ALL messages, not just the changed one. This defeats React.memo's default shallow comparison since the message prop has a new reference even when the content hasn't changed. Custom areEqual comparators now compare message by key fields (messageId, text, error, depth, children length, etc.) instead of reference equality, preventing unnecessary re-renders of SubRow, Files, HoverButtons and other children for non-latest messages. * perf: memoize ChatForm children to prevent streaming re-renders - Wrap StopButton in React.memo - Wrap AudioRecorder in React.memo, use ref for isSubmitting in onTranscriptionComplete callback to stabilize it - Remove useChatContext() from FileFormChat (bypassed its memo during streaming), accept files/setFiles/setFilesLoading as props from ChatForm instead * perf: stabilize ChatForm child props to prevent cascading re-renders ChatForm re-renders frequently during streaming (ChatContext changes). This caused StopButton and AttachFileChat/AttachFileMenu to re-render despite being memo'd, because their props were new references each time. - Wrap handleStopGenerating in a ref-based stable callback so StopButton always receives the same function reference - Create stableConversation via useMemo keyed on rendering-relevant fields only (conversationId, endpoint, agent_id, etc.), so AttachFileChat and FileFormChat don't re-render from unrelated conversation metadata updates (e.g., title generation) * perf: remove ChatContext subscription from AttachFileMenu and FileFormChat Both components used useFileHandling() which internally calls useChatContext(), bypassing their React.memo wrappers and causing re-renders on every streaming chunk. Switch to useFileHandlingNoChatContext() which accepts file state as parameters. The state (files, setFiles, setFilesLoading, conversation) is passed down from ChatForm → AttachFileChat → AttachFileMenu as props, keeping the memo chain intact. * fix: update imports and test mocks for useFileHandlingNoChatContext - Re-export useFileHandlingNoChatContext from hooks barrel - Import from ~/hooks instead of direct path for test compatibility - Add useToastContext mock to @librechat/client in AttachFileMenu tests since useFileHandlingNoChatContext runs the core hook which needs it - Add useFileHandlingNoChatContext to ~/hooks test mock * perf: fix remaining ChatForm streaming re-renders - Switch AttachFileMenu from useSharePointFileHandling (subscribes to ChatContext) to useSharePointFileHandlingNoChatContext with explicit file state props - Memoize ChatForm textarea onFocus/onBlur handlers with useCallback to prevent TextareaAutosize re-renders (inline arrow functions and .bind() created new references on every ChatForm render) - Update AttachFileMenu test mocks for new hook variants * refactor: add displayName to ChatForm for React DevTools * perf: prevent ChatForm re-renders during streaming via wrapper pattern ChatForm was re-rendering on every streaming chunk because it subscribed to useChatContext() internally, and the ChatContext value changed frequently during streaming. Extract context subscription into a ChatFormWrapper that: - Subscribes to useChatContext() (re-renders on every chunk, cheap) - Stabilizes conversation via selective useMemo - Stabilizes handleStopGenerating via ref-based callback - Passes individual stable values as props to ChatForm ChatForm (memo'd) now receives context values as props instead of subscribing directly. Since individual values (files, setFiles, isSubmitting, etc.) are stable references during streaming, ChatForm's memo prevents re-renders entirely — it only re-renders when isSubmitting actually toggles (2x per stream: start/end). * perf: stabilize newConversation prop and memoize CollapseChat - Wrap newConversation in ref-based stable callback in ChatFormWrapper (was the remaining unstable prop causing ChatForm to re-render) - Wrap CollapseChat in React.memo to prevent re-renders from parent * perf: memoize useAddedResponse return value useAddedResponse returned a new object literal on every render, causing AddedChatContext.Provider to trigger re-renders of all consumers (including ChatForm) on every streaming chunk. Wrap in useMemo so the context value stays referentially stable. * perf: memoize TextareaHeader to prevent re-renders from ChatForm * perf: address review findings for streaming render optimization Finding 1: Switch AttachFile.tsx from useFileHandling to useFileHandlingNoChatContext, closing the optimization hole for standard (non-agent) chat endpoints. Finding 2: Replace content reference equality with length comparison in both memo comparators — safer against buildTree array reconstruction. Finding 3: Add conversation?.model to stableConversation deps in ChatFormWrapper so file uploads use the correct model after switches. Finding 4/14: Fix stableNewConversation to explicitly return the underlying call's result instead of discarding it via `as` cast. Finding 5/6: Extract useMemoizedChatContext hook shared by Message.tsx and MessageContent.tsx — eliminates ~70 lines of duplication and stabilizes chatContext.conversation via selective useMemo to prevent post-stream metadata updates from re-rendering all messages. Finding 8: Use TMessage type for regenerate param instead of Record<string, unknown>. Finding 9: Use FileSetter alias in FileFormChat instead of inline type. Finding 11: Fix pre-existing broken throttle in useMessageProcess — was creating a new throttle instance per call, providing zero deduplication. Now retains the instance via useMemo. Finding 12: Initialize isSubmittingRef with chatContext.isSubmitting instead of false for consistency. Finding 13: Add ChatFormWrapper displayName. * fix: revert content comparison to reference equality in memo comparators The length-based comparison (content?.length) missed updates within existing content parts during streaming — text chunks update a part's content without changing the array length, so the comparator returned true and skipped re-renders for the latest message. Reference equality (===) is correct here: buildTree preserves content array references for unchanged messages via shallow spread, while React Query gives the latest message a new reference when its content updates during streaming. * fix: cancel throttled handleScroll on unmount and remove unused import * fix: use chatContext getter directly in regenerateMessage callback The local isSubmittingRef was stale for non-latest messages (which don't re-render during streaming by design). chatContext.isSubmitting is a getter backed by the wrapper's ref, so reading it at call-time always returns the current value regardless of whether the component has re-rendered. * fix: remove unused useCallback import from useMemoizedChatContext * fix: pass global isSubmitting to HoverButtons for action gating HoverButtons uses isSubmitting via useGenerationsByLatest to disable regenerate and hide edit buttons during streaming. Passing the effective value (false for non-latest messages) re-enabled those actions mid-stream, risking overlapping edits/regenerations. Use chatContext.isSubmitting (getter, always returns current value) for HoverButtons while keeping the effective value for rendering-only UI (cursor, placeholder, streaming indicator). * fix: address second review — stale HoverButtons, messages dep, cleanup - Add isSubmitting to chatContext useMemo deps in useMemoizedChatContext so HoverButtons correctly updates when streaming starts/ends (2 extra re-renders per session, belt-and-suspenders for post-stream state) - Change conversation?.messages?.length dep to boolean in ChatFormWrapper stableConversation — only need 0↔1+ transition for landing page check, not exact count on every message addition - Add defensive comment at chatContext destructuring point in useMessageActions explaining why isSubmitting must not be destructured - Remove dead mockUseFileHandling.mockReturnValue from AttachFileMenu tests * chore: remove dead useFileHandling mock artifacts from AttachFileMenu tests * fix: resolve eslint warnings for useMemo dependencies - Extract complex expression (conversation?.messages?.length ?? 0) > 0 to hasMessages variable for static analysis in ChatFormWrapper - Add eslint-disable for intentional isSubmitting dep in useMemoizedChatContext (forces new chatContext reference on streaming start/end so HoverButtons re-renders)	2026-03-29 17:05:12 -04:00