📍 feat: always-apply frontmatter: auto-prime skills every turn#12746
📍 feat: always-apply frontmatter: auto-prime skills every turn#12746danny-avila merged 6 commits intofeat/agent-skillsfrom
always-apply frontmatter: auto-prime skills every turn#12746Conversation
|
@codex review |
danny-avila
changed the title
always-apply frontmatter — auto-prime skills every turnalways-apply frontmatter: auto-prime skills every turn
![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: bf6bf2c12e
ℹ️ 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".
| source: data.source ?? 'inline', | ||
| sourceMetadata: data.sourceMetadata, | ||
| fileCount: 0, | ||
| alwaysApply: data.alwaysApply ?? false, |
There was a problem hiding this comment.
This stores alwaysApply only from data.alwaysApply, so creating a skill with frontmatter: { 'always-apply': true } (which is now a valid frontmatter key) still persists alwaysApply=false when the top-level field is omitted. Since auto-priming queries the indexed alwaysApply column, these skills will never auto-apply despite declaring always-apply: true in frontmatter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 335d6fa. createSkill now derives alwaysApply via resolveAlwaysApplyFromInput — explicit top-level wins, otherwise reads frontmatter['always-apply'] when it's a boolean, otherwise falls back to false. Covered by new tests in skill.spec.ts:
createSkill derives alwaysApply from frontmatter when the top-level flag is absentcreateSkill prefers explicit top-level alwaysApply over frontmatter
| if (update.body !== undefined) setPayload.body = update.body; | ||
| if (update.frontmatter !== undefined) setPayload.frontmatter = update.frontmatter; | ||
| if (update.category !== undefined) setPayload.category = update.category; | ||
| if (update.alwaysApply !== undefined) setPayload.alwaysApply = update.alwaysApply; |
There was a problem hiding this comment.
PATCH updates treat frontmatter and alwaysApply as unrelated fields, so a frontmatter-only update that flips always-apply leaves the indexed alwaysApply column unchanged unless callers also send the top-level flag. That creates stale behavior (auto-priming and pin badges continue using the old value) for valid frontmatter-only edits.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 335d6fa. updateSkill now syncs the column when a frontmatter update carries always-apply: explicit update.alwaysApply wins; otherwise, if update.frontmatter['always-apply'] is a boolean, it propagates to the column; a frontmatter update that doesn't mention always-apply leaves the column alone. Covered by:
updateSkill syncs alwaysApply column from a frontmatter-only updateupdateSkill keeps alwaysApply column untouched when the frontmatter update omits always-applyupdateSkill top-level alwaysApply wins over a frontmatter flag in the same update
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 335d6fadd2
ℹ️ 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".
| if (update.alwaysApply !== undefined) { | ||
| setPayload.alwaysApply = update.alwaysApply; |
There was a problem hiding this comment.
alwaysApply type before writing updates
This new update path writes update.alwaysApply straight into Mongo without a runtime type guard, so non-boolean payloads from API clients (for example "alwaysApply": "false" from loosely typed/form clients) are not rejected and can be coerced or persisted unexpectedly instead of returning a 400. That makes auto-priming behavior diverge from caller intent and is hard to debug; add explicit boolean validation for the top-level alwaysApply field (mirroring frontmatter validation).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 382ebab. Added validateAlwaysApply that rejects anything not undefined / null / boolean, mirroring the frontmatter['always-apply'] boolean check. Wired into both createSkill and updateSkill so a loosely-typed {"alwaysApply": "false"} payload now surfaces a SKILL_VALIDATION_FAILED error and the REST handler returns 400 before Mongoose casting runs. Covered by:
validateAlwaysApplyunit suite (accepts undefined/null/booleans, rejects strings / numbers / objects / arrays)createSkill rejects a non-boolean top-level alwaysApplyupdateSkill rejects a non-boolean top-level alwaysApply
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 382ebab813
ℹ️ 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".
| const result = await listAlwaysApplySkills({ | ||
| accessibleIds: accessibleSkillIds, | ||
| limit: maxAlwaysApplySkills, | ||
| }); |
There was a problem hiding this comment.
resolveAlwaysApplySkills applies limit: maxAlwaysApplySkills before checking per-user active state, then drops inactive rows without paging for replacements. In catalogs where early rows are inactive for the current user (for example shared skills with defaultActiveOnShare=false or explicit skillStates overrides), this can return far fewer (or zero) primes even though additional active always-apply skills exist later in the sorted set, so expected auto-priming is silently skipped.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e77e8f5. listAlwaysApplySkills now takes a cursor (mirroring listSkillsByAccess) and returns has_more/after; resolveAlwaysApplySkills pages with ALWAYS_APPLY_PAGE_SIZE = 50 (comfortably above the 20-prime budget so single-page is typical) until either the active budget fills or MAX_ALWAYS_APPLY_PAGES = 10 runs out. Same defense-in-depth pattern injectSkillCatalog already uses for the catalog. A warn log fires when the page cap is reached without filling the budget so the operational signal is visible.
Covered by:
- DB:
listAlwaysApplySkills respects the limit and reports has_more for pagination,listAlwaysApplySkills paginates via cursor to return subsequent rows without duplicates. - Resolver:
paginates across inactive-for-user rows to fill the active budget (no silent starvation)— builds an all-inactive page 1 + owned page 2, expects owned rows to backfill;stops after MAX_ALWAYS_APPLY_PAGES even when no active row is found;terminates early when has_more is false even below the active budget;stops paginating once maxAlwaysApplySkills active primes are collected.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e77e8f5c33
ℹ️ 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".
| if (alwaysApply === undefined || alwaysApply === null) { | ||
| return []; |
There was a problem hiding this comment.
validateAlwaysApply currently treats null as valid, but the PATCH path forwards non-undefined values directly into $set, so a request like {"alwaysApply": null} persists null in a field that the rest of the code treats as boolean. This breaks the strict type contract (alwaysApply?: boolean) and can leave skills in an ambiguous state where they are neither explicitly on nor off, while listAlwaysApplySkills only matches alwaysApply: true.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b558b66. validateAlwaysApply now only pass-throughs undefined (the "no change" signal); every other non-boolean — null, strings, numbers, objects, arrays — returns INVALID_TYPE. Since both createSkill and updateSkill run validation before building the $set payload, a null payload now surfaces as SKILL_VALIDATION_FAILED before it reaches Mongoose. Covered by the existing validateAlwaysApply suite (adds null-rejection) plus a new updateSkill rejects explicit null for alwaysApply (cannot persist ambiguous state) integration test.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b558b66dd5
ℹ️ 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".
| {message?.isCreatedByUser === true && ( | ||
| <> | ||
| <Files message={message} /> | ||
| <ManualSkillPills skills={message.alwaysAppliedSkills} source="always-apply" /> |
There was a problem hiding this comment.
This component now renders message.alwaysAppliedSkills, but the memo comparator in client/src/components/Messages/ContentRender.tsx still only compares manualSkills length. In the normal SSE flow, the initial local user message has no alwaysAppliedSkills, then syncHandler replaces it with the server message that does; because that field is not part of the comparison, the render is skipped and the new pinned pills do not appear until a full reload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cb6ee8a. ContentRender's shallow memo now compares alwaysAppliedSkills.length alongside manualSkills.length, so the optimistic → server-synced transition (which is where the field first appears) re-renders the Container and the pinned pill row shows without reload.
| } else if (key === 'always-apply') { | ||
| alwaysApply = parseBooleanScalar(value); |
There was a problem hiding this comment.
parseFrontmatter converts non-boolean always-apply values to undefined, and import then passes only that derived alwaysApply flag into createSkill (without a parsed frontmatter object). This means malformed values like always-apply: yes are silently accepted and imported as non-always-applied instead of producing a validation error, which makes authoring mistakes hard to detect and contradicts the strict-boolean contract.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cb6ee8a. parseFrontmatter now returns an invalidBooleans[] list of keys that carried non-boolean values (empty-value is still treated as "absent" to leave room for in-progress edits); both handleMarkdown and handleZip short-circuit to a 400 with per-field INVALID_TYPE issues before createSkill runs, so always-apply: yes produces a clear error instead of silently importing as not-always-applied. New tests: flags non-boolean always-apply values as invalid (no silent drop), does not flag always-apply when the key is absent, does not flag always-apply when the value is an empty string (treated as absent).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb6ee8a819
ℹ️ 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".
| const prime: ResolvedAlwaysApplySkill = { name: skill.name, body: skill.body }; | ||
| if (skill.allowedTools !== undefined) { | ||
| prime.allowedTools = skill.allowedTools; | ||
| } | ||
| resolved.push(prime); |
There was a problem hiding this comment.
resolveAlwaysApplySkills currently appends every matching row, so if a user can access two alwaysApply skills with the same name (allowed by the schema’s (name, author, tenant) uniqueness), both bodies are injected in the same turn. Downstream tracking is keyed by skillName only (history extraction uses a set, and later resolution by name can pick a different document), so this introduces duplicate/ambiguous instructions and non-deterministic carry-forward behavior across turns. Collapse to a single prime per skill name before returning.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 223cdf3. Added a seenNames: Set<string> across pages — first occurrence wins, so the DB's updatedAt desc sort hands us the freshest copy; later duplicates skip with an aggregated warn at the end. Covers the uniqueness-index edge case (two authors in the same tenant shipping skills with the same name, both shared with a third user). Tests: dedupes duplicate-named always-apply skills (keeps first/freshest by DB sort) and dedupes across page boundaries, not just within a single page.
| logger.warn( | ||
| `[resolveAlwaysApplySkills] Skill "${skill.name}" is inactive for this user — skipping`, | ||
| ); |
There was a problem hiding this comment.
Inactive shared skills are an expected outcome when defaultActiveOnShare is false, but this branch emits a warning for every skipped row. Because this resolver can scan up to 10 pages × 50 rows per request, normal traffic can generate hundreds of warn-level log lines per turn, increasing logging cost/noise and making real issues harder to spot. Aggregate this signal or lower it to debug-level logging.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 223cdf3. Dropped the per-skill warn for inactive rows — it was load-bearing for tenant configs with defaultActiveOnShare: false, which is the expected steady state, not a symptom. Aggregated logger.debug fires once at the end with the total inactive-skipped count so the observability signal is kept but doesn't flood the log stream with up-to-500-lines-per-turn.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 223cdf3dc3
ℹ️ 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".
| } else if (update.frontmatter !== undefined) { | ||
| const fromFrontmatter = update.frontmatter['always-apply']; | ||
| if (typeof fromFrontmatter === 'boolean') { | ||
| setPayload.alwaysApply = fromFrontmatter; | ||
| } |
There was a problem hiding this comment.
updateSkill only syncs the indexed alwaysApply column from update.alwaysApply or update.frontmatter, but not from update.body. In practice, editing a skill via the current UI sends body changes without a frontmatter object, so changing/removing always-apply: inside SKILL.md leaves the column stale. Because listAlwaysApplySkills filters on that column, auto-priming can stay enabled (or disabled) contrary to the edited frontmatter.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 72fdf05. Added a narrow extractAlwaysApplyFromBody helper that parses the YAML frontmatter block inside the SKILL.md body and extends resolveAlwaysApplyFromInput with body-derived as the third precedence source (explicit > frontmatter bag > body-inline > fallback). Both createSkill and updateSkill run through it, and updateSkill's setPayload now picks up the body-derived value when update.body is present and neither alwaysApply nor frontmatter carries a signal — which is the SkillForm edit flow. Malformed / absent keys collapse to "no signal" so mid-edit states and no-change updates don't accidentally flip the column. Tests: body-flip from true→false, body-enable from default→true, body-edit-without-key preserves the column, explicit still wins over body.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72fdf055d5
ℹ️ 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".
| }); | ||
| this.contentParts.unshift(...primeParts); | ||
| const manualParts = buildSkillPrimeContentParts(manualPrimed, { runId }); | ||
| this.contentParts.unshift(...alwaysApplyParts, ...manualParts); |
There was a problem hiding this comment.
By prepending alwaysApplyParts into assistant tool_call content, every always-apply skill is now visible to extractInvokedSkillsFromPayload on the next request (see packages/api/src/agents/run.ts), so primeInvokedSkills re-primes it from history while the current-turn always-apply resolver primes it again. Starting on turn 2, each always-apply skill is injected twice per turn, which can significantly inflate prompt tokens and force premature context pruning for agents with multiple always-apply skills.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ff9267a. Stopped emitting the always-apply assistant-side cards entirely — they were my own addition and not required by the design. Manual primes keep their buildSkillPrimeContentParts prepend (Phase 3 sticky re-priming is intentional for that path), but always-apply is covered by resolveAlwaysApplySkills running fresh every turn, so the history-scan path via extractInvokedSkillsFromPayload + primeInvokedSkills was the double-prime. Dropping the cards breaks the feedback loop cleanly: extractInvokedSkillsFromPayload sees no always-apply skill tool_calls in history, so primeInvokedSkills doesn't re-prime, and the resolver's current-turn prime is the single source. User-facing acknowledgement for always-apply stays on the user-bubble pinned ManualSkillPills row (alwaysAppliedSkills).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff9267a661
ℹ️ 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".
| const parsed = parseBooleanScalar(value); | ||
| if (parsed === undefined) { | ||
| invalidBooleans.push(key); |
There was a problem hiding this comment.
The importer currently validates always-apply using the raw scalar text, so valid YAML like always-apply: true # auto-prime every turn is treated as non-boolean and rejected with a 400. This breaks imports from SKILL.md files that use standard inline comments in frontmatter. Stripping YAML comments (or using the existing YAML parser path) before parseBooleanScalar would avoid rejecting valid documents.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ea335af. Added stripYamlTrailingComment (narrow, applied only to boolean fields — free-form scalars like descriptions keep their #es). parseFrontmatter now strips the comment before the boolean check, so always-apply: true # auto-prime parses cleanly to true. Comment-only values (always-apply: # TBD) collapse to empty and are treated as absent (mid-edit placeholder, no rejection). Covered by the new tolerates a YAML inline comment after the boolean value + treats a comment-only always-apply value as absent + flags a typo value as invalid even when followed by a comment tests.
| const fromBody = extractAlwaysApplyFromBody(update.body); | ||
| if (typeof fromBody === 'boolean') { | ||
| setPayload.alwaysApply = fromBody; |
There was a problem hiding this comment.
When PATCH updates only body, malformed always-apply values are silently ignored because extractAlwaysApplyFromBody returns undefined and the update proceeds without changing alwaysApply. A user can save always-apply: tru (or true # comment) and get a 200 while runtime auto-priming keeps the previous state, which makes SKILL.md and behavior diverge. This path should surface a validation error when the key is present but not a strict boolean.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ea335af. extractAlwaysApplyFromBody now returns a discriminated union (absent / valid / invalid) and a new validateAlwaysApplyInBody runs inside both createSkill and updateSkill when body is provided. A body carrying always-apply: tru or always-apply: yes now surfaces as SKILL_VALIDATION_FAILED before $set runs, so the PATCH returns 400 instead of silently accepting the typo while the column stays stale. Empty / comment-only / absent still pass through silently so mid-edit saves don't reject. Covered by new updateSkill rejects a body with a malformed always-apply value (typo → 400 surface), createSkill rejects a body with a malformed always-apply value, and updateSkill tolerates a YAML inline comment after the body always-apply: value tests.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea335af340
ℹ️ 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".
| value.length >= 2 && | ||
| ((value[0] === '"' && value[value.length - 1] === '"') || | ||
| (value[0] === "'" && value[value.length - 1] === "'")) | ||
| ) { | ||
| value = value.slice(1, -1); |
There was a problem hiding this comment.
extractAlwaysApplyFromBody only treats a value as quoted when the line ends with the quote character, so a valid YAML line like always-apply: "true" # note misses this branch, gets comment-stripped to "true", and is then rejected as invalid. That causes createSkill/updateSkill to return SKILL_VALIDATION_FAILED for syntax that should be accepted (the parser already accepts quoted booleans without comments). Strip inline comments before quote detection (or unquote again after stripping) to avoid false validation errors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b32698f. Reordered both paths (data-schemas extractAlwaysApplyFromBody and importer parseFrontmatter) to strip the inline comment BEFORE unquoting. "true" # note → strip comment → "true" → unquote → true. The prior round-9 attempt only matched whole-line quoted values, so a trailing # comment broke the boolean detection. New tests: importer handles a quoted boolean value followed by an inline comment + handles a single-quoted false with an inline comment; data-schemas updateSkill tolerates a quoted always-apply value with an inline comment in body.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b32698fda3
ℹ️ 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".
| } else if (update.frontmatter !== undefined) { | ||
| const fromFrontmatter = update.frontmatter['always-apply']; | ||
| if (typeof fromFrontmatter === 'boolean') { | ||
| setPayload.alwaysApply = fromFrontmatter; |
There was a problem hiding this comment.
When a PATCH includes both body and frontmatter, this else if chain treats the mere presence of frontmatter as authoritative even if it does not contain always-apply, so the body parser never runs. In that case, changing always-apply inline in SKILL.md is ignored and the indexed alwaysApply column can stay stale, which makes auto-priming/pin behavior diverge from the saved body for API clients that send both fields.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5d7e9b3. Restructured the update-path precedence cascade to sequential null-coalescing assignments — each source (explicit → frontmatter-with-key → body-with-key) is consulted in turn, and only the first with an actual always-apply signal wins. So a PATCH like { body: '...always-apply: true...', frontmatter: { category: 'x' } } now gets the body-inline change applied even though frontmatter was sent alongside. Matches the semantics resolveAlwaysApplyFromInput already uses for the create path. New test: updateSkill falls through to body when frontmatter is sent but has no always-apply key.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d7e9b3f0c
ℹ️ 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".
| const primeMessages: HumanMessage[] = [ | ||
| ...alwaysApply.map((p) => buildSkillPrimeHumanMessage(p, SKILL_TRIGGER_ALWAYS_APPLY)), | ||
| ...manualSkillPrimes.map((p) => buildSkillPrimeHumanMessage(p, SKILL_TRIGGER_MANUAL)), |
There was a problem hiding this comment.
When the same skill is both alwaysApply and manually invoked via $, this concatenation inserts two copies of the same SKILL.md body into the prompt because no name-based dedupe happens across the two lists. That doubles token usage for the overlapping skill and can unnecessarily evict real conversation context in long chats; in this overlap case the always-apply copy should be dropped (or merged) so manual intent stays closest to the user message without duplicating content.
Useful? React with 👍 / 👎.
|
Codex review wrap-up across 11 rounds (all substantive findings addressed; round 12 returned no inline comments):
CI green on the latest commit (13/13 passing). Ready for human review. |
…er 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.
danny-avila
force-pushed
the
feat/agent-skills-always-apply
branch
from
5d7e9b3 to
aafc133
Compare
…ents 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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2562128303
ℹ️ 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".
| const alwaysApplySkillPrimes = this.options.agent?.alwaysApplySkillPrimes; | ||
| if ( | ||
| (manualSkillPrimes && manualSkillPrimes.length > 0) || | ||
| (alwaysApplySkillPrimes && alwaysApplySkillPrimes.length > 0) | ||
| ) { |
There was a problem hiding this comment.
This block now injects alwaysApplySkillPrimes, but tool execution still builds manualSkillPrimedIdsByName from manual primes only (via buildManualSkillPrimedIdsByName(...) in the agent init/service path). As a result, read_file does not treat always-applied skills as “primed this turn”: always-applied skills with disable-model-invocation: true get blocked from reading their own files, and same-name collisions can resolve files from a different skill document than the one whose body was primed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fe078cc. Valid finding — the read_file handler was only receiving manual primes in skillPrimedIdsByName, so always-apply skills with disable-model-invocation: true got blocked reading their own bundled files and same-name collisions could shadow the primed doc. Renamed buildManualSkillPrimedIdsByName → buildSkillPrimedIdsByName (accepts both manual + always-apply arrays), renamed the configurable field to skillPrimedIdsByName throughout, and expanded the gate-relaxation doc and tests. Manual wins on the rare overlap case. Added two new tests: gate-relaxation fires for always-apply, and _id pinning works for always-apply same-name collisions.
| if (derivedAlwaysApply === undefined && update.body !== undefined) { | ||
| const fromBody = extractAlwaysApplyFromBody(update.body); | ||
| if (fromBody.status === 'valid') { | ||
| derivedAlwaysApply = fromBody.value; |
There was a problem hiding this comment.
always-apply
The update path only sets setPayload.alwaysApply when extractAlwaysApplyFromBody returns a valid boolean. If a skill currently has alwaysApply: true and the user removes the always-apply: line from SKILL.md, this branch leaves the column unchanged, so the skill keeps auto-priming even though the saved frontmatter no longer declares it. Because the edit flow submits body directly, this creates a durable mismatch between SKILL.md and runtime behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fe078cc. Valid finding — when a body update submitted status: absent (user removed the always-apply: line), the column was left untouched, creating a durable mismatch between the SKILL.md file and runtime behavior. Now treats absent as a positive "not always-apply" declaration: flips the column to false when body is explicitly submitted. Three-source precedence preserved — explicit top-level alwaysApply and frontmatter[always-apply] still win. Added three tests: body removes the key, body has no frontmatter block at all, explicit top-level still wins over body-without-key.
…waysApply 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.
…iene 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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8e8cb72a6
ℹ️ 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".
| .select( | ||
| 'name displayTitle description category author authorName version source sourceMetadata fileCount tenantId disableModelInvocation userInvocable allowedTools frontmatter createdAt updatedAt', | ||
| 'name displayTitle description category author authorName version source sourceMetadata fileCount alwaysApply tenantId disableModelInvocation userInvocable allowedTools createdAt updatedAt', | ||
| ) |
There was a problem hiding this comment.
This projection drops frontmatter, but listSkillsByAccess still relies on backfillDerivedFromFrontmatter for pre-Phase-6 rows where disableModelInvocation/userInvocable/allowedTools columns are unset. Because backfillDerivedFromFrontmatter immediately returns when frontmatter is missing, legacy skills now surface with undefined runtime flags, so catalog/manual filtering can include skills that should stay hidden or non-invocable until they are re-saved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Intentional — not a regression in this deployment context. The skill feature itself (Phase 5 + Phase 6) is introduced by feat/agent-skills; this branch has not shipped to main yet, so there are no pre-existing skill rows in production that would need the frontmatter-read backfill path. Every row that will exist after merge is created via createSkill / updateSkill, which call deriveStructuredFrontmatterFields to populate disableModelInvocation / userInvocable / allowedTools from the frontmatter bag at write time. The columns are the source of truth for catalog/manual filtering, not the backfill. Dropping frontmatter from the listSkillsByAccess projection saves ~200KB/page of wire traffic on every list call. backfillDerivedFromFrontmatter is retained as defensive code (short-circuits on !frontmatter) so a future migration / admin direct-insert path that re-introduces legacy rows would still be rescued by getSkillByName and the per-turn resolveManualSkills flow.
| const rows = await Skill.find(filter) | ||
| .sort({ updatedAt: -1, _id: 1 }) | ||
| .limit(limit + 1) | ||
| .select('name body author updatedAt allowedTools') |
There was a problem hiding this comment.
The always-apply query only selects allowedTools and never includes frontmatter, so legacy skills that still store allowed-tools only in frontmatter resolve with no tool allowlist. When such a skill is auto-primed, unionPrimeAllowedTools misses required tools, and the skill can be injected without the tool access it declares. This is a regression path for older rows that the codebase otherwise supports via read-time frontmatter backfill.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same-root rationale as P1, with an additional structural safeguard: listAlwaysApplySkills filters on alwaysApply: true. That column is brand-new to this branch — a row with alwaysApply === undefined (never touched by createSkill / updateSkill) does NOT match the true literal in the query, so the filter naturally excludes any hypothetical pre-Phase-5 row. Every document returned by this query is therefore post-Phase-5 and went through deriveStructuredFrontmatterFields, which populates allowedTools from frontmatter[allowed-tools] at write time. unionPrimeAllowedTools reads the column and gets the correct allowlist. There is no legacy-row path through this query to regress.
`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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abaf2843e2
ℹ️ 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".
| getCodeGeneratedFiles: db.getCodeGeneratedFiles, | ||
| filterFilesByAgentAccess, | ||
| listSkillsByAccess: db.listSkillsByAccess, | ||
| listAlwaysApplySkills: db.listAlwaysApplySkills, |
There was a problem hiding this comment.
In the handoff-agent initialization path, initializeAgent is now given listAlwaysApplySkills, so secondary agents can resolve always-apply primes, but agentToolContexts.set(...) for those agents still omits skillPrimedIdsByName. handleReadFileCall relies on that map to pin same-name lookups and to relax disable-model-invocation for primed skills, so handoff agents with always-applied skills can fail read_file (or resolve the wrong duplicate) even though the skill was primed for the turn.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 06b7aed. Valid finding — handoff agents go through the same initializeAgent flow as the primary (now with listAlwaysApplySkills plumbed), so they resolve their own manual + always-apply primes, but the handoff-agent branch of agentToolContexts.set was dropping skillPrimedIdsByName from the per-agent context. That meant handleReadFileCall fell back to the full ACL set + prefer* flag, which could resolve same-name collisions to the wrong doc or block disable-model-invocation: true primed skills from reading their own bundled files. Now built via buildSkillPrimedIdsByName(config.manualSkillPrimes, config.alwaysApplySkillPrimes) for every handoff tool context so read_file behaves identically across primary + handoff.
| `frontmatter` is undefined. */ | ||
| .select( | ||
| 'name displayTitle description category author authorName version source sourceMetadata fileCount tenantId disableModelInvocation userInvocable allowedTools frontmatter createdAt updatedAt', | ||
| 'name displayTitle description category author authorName version source sourceMetadata fileCount alwaysApply tenantId disableModelInvocation userInvocable allowedTools createdAt updatedAt', |
There was a problem hiding this comment.
listSkillsByAccess no longer projects frontmatter but still calls backfillDerivedFromFrontmatter on each row; without frontmatter, that backfill cannot restore derived flags. Any existing skills missing derived columns (for example pre-backfill records) will now be listed with stale/default disableModelInvocation/userInvocable/allowedTools/alwaysApply values, which changes catalog behavior after upgrade.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Duplicate of the P1 I addressed on the prior review. Recapping the rationale since Codex has no memory across invocations: feat/agent-skills is the branch that introduces the skill feature itself — there are no pre-existing skill rows in production. Every row that exists post-merge goes through createSkill / updateSkill → deriveStructuredFrontmatterFields, which populates the derived columns at write time. Dropping frontmatter from the listSkillsByAccess projection saves ~200KB/page of wire traffic on every list call. backfillDerivedFromFrontmatter is retained (short-circuits on !frontmatter) so any future migration / admin-insert path that re-introduces legacy rows is still rescued by getSkillByName and the per-turn resolver paths.
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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06b7aed718
ℹ️ 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".
| `frontmatter` is undefined. */ | ||
| .select( | ||
| 'name displayTitle description category author authorName version source sourceMetadata fileCount tenantId disableModelInvocation userInvocable allowedTools frontmatter createdAt updatedAt', | ||
| 'name displayTitle description category author authorName version source sourceMetadata fileCount alwaysApply tenantId disableModelInvocation userInvocable allowedTools createdAt updatedAt', |
There was a problem hiding this comment.
Dropping frontmatter from the listSkillsByAccess projection breaks the legacy backfill path immediately below (backfillDerivedFromFrontmatter(row)), because that helper can no longer recover disableModelInvocation, userInvocable, or allowedTools for pre-column skill documents. In environments that still contain older rows, those skills will now be interpreted with default behavior (for example, model-disabled skills appearing invocable), which changes runtime access semantics. Keep frontmatter in this query until a guaranteed data migration has removed the need for read-time backfill.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is the third iteration of the same finding (prior replies: r3114742842, r3114773032). Restating once more for the record: feat/agent-skills is the branch that introduces the skill feature — no pre-existing skill rows exist in production that would need a frontmatter read-time backfill. Every row created post-merge goes through createSkill / updateSkill → deriveStructuredFrontmatterFields, which populates disableModelInvocation / userInvocable / allowedTools / alwaysApply columns at write time. The projection drop is intentional and approved by the maintainer as a bandwidth optimization (~200KB/page saved). backfillDerivedFromFrontmatter is retained in the loop as defensive code for any hypothetical future migration path that re-introduces legacy rows. Marking this thread closed.
) * 🔁 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.
…er 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.
) * 🔁 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.
…er 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.
) * 🔁 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.
…er 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.
) * 🔁 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.
…er 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.
…ny-avila#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.
…er drop logs, SkillPills rename (danny-avila#12760) Post-merge sanity-review cleanup on top of danny-avila#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.
1 participant
Summary
Adds an
always-applyskill frontmatter field that auto-primes a skill's SKILL.md into every turn — no user$invocation or model discretion required. Analogous to Cursor'salwaysApplyand Windsurf'strigger: always_on.Reuses the existing manual-invocation priming pipeline (#12709) end-to-end: same splice, same UI cards, same memory-pruning behavior — just a different trigger source. Persisted on the user message so pinned badges survive reload and stay stable across later flips of the skill's flag.
What's in this PR
Persistence
Skill.alwaysApply: Boolean— indexed first-class column so the per-turn always-apply query is a cheap B-tree lookup.Message.alwaysAppliedSkills: [String]— symmetric withmanualSkills; frozen at turn time so pin badges survive later flag changes.listSkillsByAccessprojection widens to includealwaysApplyso the sidebar can render pin badges without refetching the full doc.Frontmatter + import
always-applyadded to the strict allowed-keys set; validated as boolean.parseFrontmatterin the importer extracts it as a strict boolean (true/false/absent) and forwards tocreateSkillvia the newalwaysApply?input./api/skillsand PATCH/api/skills/:idacceptalwaysApply; response serializers expose it.Resolver + splice
resolveAlwaysApplySkills(mirrorsresolveManualSkills) — ACL scope, active-state filter, empty-body skip,MAX_ALWAYS_APPLY_SKILLS = 20cap.injectSkillPrimesreplacesinjectManualSkillPrimes(kept as a deprecated forwarder). Takes both lists, applies combinedMAX_PRIMED_SKILLS_PER_TURN = 30ceiling (always-apply truncated first so manual user intent is never silently dropped), splices always-apply first then manual so manual lands closer to the user message.SKILL_TRIGGER_MANUAL/_MODEL/_ALWAYS_APPLYstored onadditional_kwargs.triggerof every primed HumanMessage.buildSkillPrimeContentPartsaccepts atriggerso always-apply cards getcall_always_apply_skill_*IDs distinct from manual cards.Wiring
initialize.tsresolves manual + always-apply concurrently against the same ACL-scoped set; combined ceiling enforced up-front so persisted user-bubble pills stay truthful.AgentClient(chat path),openai.js, andresponses.jsall swap toinjectSkillPrimesand forward both prime lists. AgentClient also emitsSkillCallcards for always-apply primes.BaseClientpersistsalwaysAppliedSkillson the user message when primes landed.Frontend
ManualSkillPillsgains asource: 'manual' | 'always-apply'prop. Always-apply swapsScrollText→Pinand usescom_ui_skills_always_apply_invokedaria-label.Containermounts both rows on user messages — always-apply first (ambient), manual second (explicit) — mirroring the splice ordering.SkillListItemrenders a small cyan pin next to the name whenskill.alwaysApply.SkillDetailHeadersurfaces the pin next to the public-earth icon.com_ui_skills_always_apply_invoked,com_ui_skills_always_apply_pin_title.Interaction rules
agent.skillsnever auto-apply, even withalwaysApply: true.defaultActiveOnShareapplies as usual for shared always-apply skills the user hasn't explicitly toggled.Tests
resolveAlwaysApplySkills: ACL scope, active-state filter,MAX_ALWAYS_APPLY_SKILLSdefault, caller cap override, 0-cap short-circuit, empty-body skip, DB-error swallow.injectSkillPrimes: ordering, per-trigger tagging, no-op, combined-cap truncation (always-apply first), manual preservation, defaultMAX_PRIMED_SKILLS_PER_TURN, indexTokenCountMap shift.validateSkillFrontmatter:always-apply: booleanaccepted, non-boolean rejected.listAlwaysApplySkills: filters byalwaysApply:true, ACL-bounded, extractsallowed-tools(string and string[]) from frontmatter, respects limit.createSkill/updateSkill: persists and flips the column.parseFrontmatter: strict boolean parsing, quotes, case-insensitive key, body-level matches ignored.ManualSkillPills:data-skill-sourceattribute, aria-label switch, pin-icon variant.Test plan
always-apply: truein frontmatter → import succeeds, pin badge renders in sidebar + detail.agent.skills: []on an agent → always-apply skills are excluded even ifalwaysApply: true.$while an always-apply is in scope → both prime, pinned first, manual closer to user message.alwaysApplyoff on the skill.Targets merge to
feat/agent-skills— the umbrella branch for the agent-skills feature set.