mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
📍 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.
This commit is contained in:
Danny Avila
parent
82173f7b91
commit
dfc3dfa57f
34 changed files with 2360 additions and 212 deletions
|
|
@ -28,7 +28,7 @@ const { filterFilesByAgentAccess } = require('~/server/services/Files/permission
|
|||
const {
|
||||
getSkillToolDeps,
|
||||
enrichWithSkillConfigurable,
|
||||
buildManualSkillPrimedIdsByName,
|
||||
buildSkillPrimedIdsByName,
|
||||
} = require('./skillDeps');
|
||||
const { getModelsConfig } = require('~/server/controllers/ModelController');
|
||||
const { checkPermission, findAccessibleResources } = require('~/server/services/PermissionService');
|
||||
|
|
@ -190,7 +190,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => {
|
|||
req,
|
||||
ctx.accessibleSkillIds,
|
||||
codeApiKey,
|
||||
ctx.manualSkillPrimedIdsByName,
|
||||
ctx.skillPrimedIdsByName,
|
||||
);
|
||||
},
|
||||
toolEndCallback,
|
||||
|
|
@ -287,6 +287,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => {
|
|||
getCodeGeneratedFiles: db.getCodeGeneratedFiles,
|
||||
filterFilesByAgentAccess,
|
||||
listSkillsByAccess: db.listSkillsByAccess,
|
||||
listAlwaysApplySkills: db.listAlwaysApplySkills,
|
||||
getSkillByName: db.getSkillByName,
|
||||
},
|
||||
);
|
||||
|
|
@ -294,12 +295,15 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => {
|
|||
logger.debug(
|
||||
`[initializeClient] Storing tool context for ${primaryConfig.id}: ${primaryConfig.toolDefinitions?.length ?? 0} tools, registry size: ${primaryConfig.toolRegistry?.size ?? '0'}`,
|
||||
);
|
||||
/** Maps each manually-primed skill name to the `_id` of the exact doc
|
||||
* that was primed. Plumbed to `enrichWithSkillConfigurable` so the
|
||||
* read_file handler can pin same-name collision lookups to the
|
||||
* resolver's chosen doc. */
|
||||
const manualSkillPrimedIdsByName = buildManualSkillPrimedIdsByName(
|
||||
/** Maps each primed skill name (manual `$` or always-apply) to the
|
||||
* `_id` of the exact doc that was primed. Plumbed to
|
||||
* `enrichWithSkillConfigurable` so the read_file handler can pin
|
||||
* same-name collision lookups to the resolver's chosen doc AND relax
|
||||
* the disable-model-invocation gate for skills whose body is already
|
||||
* in this turn's context. */
|
||||
const skillPrimedIdsByName = buildSkillPrimedIdsByName(
|
||||
primaryConfig.manualSkillPrimes,
|
||||
primaryConfig.alwaysApplySkillPrimes,
|
||||
);
|
||||
agentToolContexts.set(primaryConfig.id, {
|
||||
agent: primaryAgent,
|
||||
|
|
@ -308,7 +312,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => {
|
|||
tool_resources: primaryConfig.tool_resources,
|
||||
actionsEnabled: primaryConfig.actionsEnabled,
|
||||
accessibleSkillIds: primaryConfig.accessibleSkillIds,
|
||||
manualSkillPrimedIdsByName,
|
||||
skillPrimedIdsByName,
|
||||
});
|
||||
|
||||
const {
|
||||
|
|
@ -349,6 +353,7 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => {
|
|||
getCodeGeneratedFiles: db.getCodeGeneratedFiles,
|
||||
filterFilesByAgentAccess,
|
||||
listSkillsByAccess: db.listSkillsByAccess,
|
||||
listAlwaysApplySkills: db.listAlwaysApplySkills,
|
||||
getSkillByName: db.getSkillByName,
|
||||
},
|
||||
// The callback fires during BFS, before the helper prunes agents
|
||||
|
|
@ -357,6 +362,13 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => {
|
|||
// set. The per-agent tool context map is OK to keep populated even
|
||||
// for pruned ids: it's only read by closure in ON_TOOL_EXECUTE,
|
||||
// stale entries are unreachable at runtime.
|
||||
//
|
||||
// Handoff agents get the same `skillPrimedIdsByName` plumbing as the
|
||||
// primary so `read_file` can pin same-name collisions to the exact
|
||||
// primed doc AND relax the `disable-model-invocation: true` gate for
|
||||
// skills whose body is already in this turn's context — matters for
|
||||
// handoff agents that have their own always-apply skills bound or
|
||||
// that the user `$`-invokes within the handoff flow.
|
||||
onAgentInitialized: (agentId, agent, config) => {
|
||||
agentToolContexts.set(agentId, {
|
||||
agent,
|
||||
|
|
@ -365,6 +377,10 @@ const initializeClient = async ({ req, res, signal, endpointOption }) => {
|
|||
tool_resources: config.tool_resources,
|
||||
actionsEnabled: config.actionsEnabled,
|
||||
accessibleSkillIds: config.accessibleSkillIds,
|
||||
skillPrimedIdsByName: buildSkillPrimedIdsByName(
|
||||
config.manualSkillPrimes,
|
||||
config.alwaysApplySkillPrimes,
|
||||
),
|
||||
});
|
||||
},
|
||||
// Pass through the `@librechat/api` exports so that tests which
|
||||
|
|
|
|||
|
|
@ -6,24 +6,54 @@ const { enrichWithSkillConfigurable } = require('@librechat/api');
|
|||
const db = require('~/models');
|
||||
|
||||
/**
|
||||
* Builds the `manualSkillPrimedIdsByName` map passed through to
|
||||
* Builds the `skillPrimedIdsByName` map passed through to
|
||||
* `enrichWithSkillConfigurable`. Centralized here so the four CJS call
|
||||
* sites (`initialize.js`, `responses.js` x2, `openai.js`) share one
|
||||
* source of truth — if `ResolvedManualSkill` ever renames `_id` or
|
||||
* gains new identifying fields, only this helper changes.
|
||||
*
|
||||
* Returns `undefined` (not `{}`) when there are no primes, so the
|
||||
* downstream `enrichWithSkillConfigurable` cleanly omits the field
|
||||
* from `mergedConfigurable` rather than threading an empty object.
|
||||
* Combines both manual (`$`-popover) primes AND always-apply primes so
|
||||
* `read_file` can:
|
||||
* - Relax the `disable-model-invocation: true` gate for either source
|
||||
* (the body is already in context; blocking its own files would be
|
||||
* nonsensical).
|
||||
* - Pin same-name collision lookups to the exact `_id` the resolver
|
||||
* primed (otherwise a newer same-name duplicate could shadow the
|
||||
* body/file pair within a single turn).
|
||||
*
|
||||
* On the rare overlap (a name appears in both arrays because upstream
|
||||
* dedup was skipped), manual wins — manual invocation is explicit user
|
||||
* intent and carries the authoritative `_id` for this turn.
|
||||
*
|
||||
* Returns `undefined` (not `{}`) when both arrays are empty, so the
|
||||
* downstream `enrichWithSkillConfigurable` cleanly omits the field from
|
||||
* `mergedConfigurable` rather than threading an empty object.
|
||||
*
|
||||
* @param {Array<{ name: string, _id: { toString(): string } }> | undefined} manualSkillPrimes
|
||||
* @param {Array<{ name: string, _id: { toString(): string } }> | undefined} alwaysApplySkillPrimes
|
||||
* @returns {Record<string, string> | undefined}
|
||||
*/
|
||||
function buildManualSkillPrimedIdsByName(manualSkillPrimes) {
|
||||
if (!manualSkillPrimes?.length) {
|
||||
function buildSkillPrimedIdsByName(manualSkillPrimes, alwaysApplySkillPrimes) {
|
||||
const manualCount = manualSkillPrimes?.length ?? 0;
|
||||
const alwaysApplyCount = alwaysApplySkillPrimes?.length ?? 0;
|
||||
if (manualCount === 0 && alwaysApplyCount === 0) {
|
||||
return undefined;
|
||||
}
|
||||
return Object.fromEntries(manualSkillPrimes.map((p) => [p.name, p._id.toString()]));
|
||||
const out = {};
|
||||
/* Order matters on the edge case where the same name appears in both
|
||||
lists: always-apply goes in first, then manual overwrites — manual
|
||||
wins because it's explicit user intent for this turn. */
|
||||
if (alwaysApplyCount > 0) {
|
||||
for (const p of alwaysApplySkillPrimes) {
|
||||
out[p.name] = p._id.toString();
|
||||
}
|
||||
}
|
||||
if (manualCount > 0) {
|
||||
for (const p of manualSkillPrimes) {
|
||||
out[p.name] = p._id.toString();
|
||||
}
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
/** Skill-related properties for ToolExecuteOptions (stable references, allocated once). */
|
||||
|
|
@ -49,7 +79,7 @@ function getSkillToolDeps() {
|
|||
* @param {object} req - The Express request object
|
||||
* @param {Array} accessibleSkillIds - Pre-computed accessible skill IDs
|
||||
* @param {string} [preResolvedCodeApiKey] - Pre-resolved code API key (skips redundant lookup)
|
||||
* @param {Record<string, string>} [manualSkillPrimedIdsByName] - Map of name → skill id for skills manually invoked this turn via the `$` popover. Pins same-name collision lookups in `read_file`.
|
||||
* @param {Record<string, string>} [skillPrimedIdsByName] - Map of name → skill id for skills primed this turn (manual `$`-popover invocation OR always-apply). Pins same-name collision lookups in `read_file` and relaxes the disable-model-invocation gate for the primed doc.
|
||||
* @returns {Promise<object>} Augmented result with skill configurable
|
||||
*/
|
||||
function enrichConfigurable(
|
||||
|
|
@ -57,7 +87,7 @@ function enrichConfigurable(
|
|||
req,
|
||||
accessibleSkillIds,
|
||||
preResolvedCodeApiKey,
|
||||
manualSkillPrimedIdsByName,
|
||||
skillPrimedIdsByName,
|
||||
) {
|
||||
return enrichWithSkillConfigurable(
|
||||
result,
|
||||
|
|
@ -65,12 +95,12 @@ function enrichConfigurable(
|
|||
accessibleSkillIds,
|
||||
loadAuthValues,
|
||||
preResolvedCodeApiKey,
|
||||
manualSkillPrimedIdsByName,
|
||||
skillPrimedIdsByName,
|
||||
);
|
||||
}
|
||||
|
||||
module.exports = {
|
||||
getSkillToolDeps,
|
||||
enrichWithSkillConfigurable: enrichConfigurable,
|
||||
buildManualSkillPrimedIdsByName,
|
||||
buildSkillPrimedIdsByName,
|
||||
};
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue