✂️ fix: Deduplicate Skill Bodies Across Fresh Primes and History#13610
Conversation
When a skill is primed fresh this turn (manual $-popover or always-apply) AND also appears in history as a `skill` tool_call, its SKILL.md body was injected twice — once by injectSkillPrimes and once reconstructed by formatAgentMessages. - add `collectFreshSkillPrimeNames` helper (packages/api) — union of manual + always-apply prime names - client.js: pass the set as `skipSkillBodyNames` to formatAgentMessages for both the initialMessages and memoryMessages paths so the body reconstructs once. Names not primed this turn still reconstruct (sticky manual re-prime). Requires `@librechat/agents` with `skipSkillBodyNames` support; the published dist silently ignores the unknown option until upgraded.
|
Companion (SDK) PR: danny-avila/agents#231 — adds the |
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR prevents duplicated SKILL.md body injection when the same skill is (a) primed fresh in the current turn (manual $ popover and/or always-apply frontmatter) and (b) also exists in history as a skill tool_call that would otherwise be reconstructed by formatAgentMessages.
Changes:
- Add
collectFreshSkillPrimeNames()helper to compute the union of skill names primed in the current turn (manual + always-apply). - Pass the resulting set as
skipSkillBodyNamesintoformatAgentMessagesso historical reconstruction doesn’t re-inject bodies already injected via fresh priming. - Add unit tests covering union, dedupe, and empty/undefined input handling for the helper.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/api/src/agents/skills.ts | Adds collectFreshSkillPrimeNames() to compute the current-turn primed skill name set for dedupe behavior. |
| packages/api/src/agents/tests/skills.test.ts | Adds unit tests for collectFreshSkillPrimeNames() covering union/dedupe/empty inputs. |
| api/server/controllers/agents/client.js | Uses the helper and forwards skipSkillBodyNames to formatAgentMessages for both initial and memory formatting paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d078b5f71d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ? { | ||
| ...(needsDeepSeekFormat ? { provider: Providers.DEEPSEEK } : {}), | ||
| ...(freshSkillPrimeNames.size > 0 | ||
| ? { skipSkillBodyNames: freshSkillPrimeNames } |
There was a problem hiding this comment.
In deployments built from this repo/lockfile, npm ci still installs @librechat/agents 3.2.31 (package-lock.json resolves that exact version), and the commit notes that version silently ignores skipSkillBodyNames. When a fresh always-apply/manual skill is also present in historical skill tool calls, this option therefore has no effect and the duplicate SKILL.md body problem remains until the companion agents build is actually pulled into the lockfile.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and intentional — this is the cross-repo sequencing noted under Dependency in the PR description. The runtime behavior is gated on the companion SDK PR (danny-avila/agents#231), which adds the skipSkillBodyNames option. Until that ships and @librechat/agents is bumped in package.json + the lockfile, the published 3.2.31 dist safely ignores the unknown option (no regression, no effect).
Sequencing: merge & release agents#231 → bump @librechat/agents here → this PR's dedupe takes effect. I'll push the dependency bump as a follow-up commit once the SDK version is published; happy to convert this to a draft until then if you'd prefer it not merge ahead of the bump.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
danny-avila
changed the title
Problem
A skill that is primed fresh this turn (manual
$-popover or always-apply frontmatter) and also present in history as askilltool_call had its SKILL.md body injected twice:injectSkillPrimessplices the fresh body in before the latest user message, andformatAgentMessagesreconstructs the historical body from theskilltool_call.Repro: turn 1 invoke a skill manually/model-side, turn 2 have the same skill active as always-apply → two SKILL.md bodies, and prompt-cache markers could pin to the duplicated synthetic prefix.
Changes
packages/api— newcollectFreshSkillPrimeNames({ manualSkillPrimes, alwaysApplySkillPrimes })helper returning the union of names primed this turn (TS, unit-tested).api/server/controllers/agents/client.js— build that set and pass it asskipSkillBodyNamestoformatAgentMessagesfor both theinitialMessagesandmemoryMessagespaths. A skill primed this turn reconstructs once (the fresh prime); names not primed this turn still reconstruct from history, preserving sticky manual re-priming.initialSessions/ skill-file availability untouched.Dependency
The
skipSkillBodyNamesoption requires a build of@librechat/agentsthat supports it (companion PR ondanny-avila/agents). The currently-published dist silently ignores the unknown option key until upgraded — safe to merge independently.Tests
packages/api/src/agents/__tests__/skills.test.ts:collectFreshSkillPrimeNamesunion, manual+always-apply dedupe, empty/undefined inputs, one-sided inputs. 150 tests pass; lint + typecheck clean.