From 9efd61d57d4949bd7e441b59e20395104b0bb1d0 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Tue, 5 May 2026 18:35:09 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=90=20fix:=20Forward=20per-file=20enti?= =?UTF-8?q?ty=5Fid=20through=20code-env=20priming=20(#12958)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🔐 fix: Forward per-file `entity_id` through code-env priming Skill files and persisted code-env files now carry their `entity_id` on the in-memory file refs that seed `Graph.sessions`. Without this, an execute call that mixes a skill file (uploaded with `entity_id=skillId`) and a user attachment (uploaded with no `entity_id`) collapses onto a single request-level entity at the codeapi authorization step and one side 403s. With per-file `entity_id`, codeapi resolves sessionKey per file and both authorize. - `primeSkillFiles` / `primeInvokedSkills`: thread `entity_id` through fresh-upload, cache-hit, and per-skill-batch paths in `packages/api/src/agents/skillFiles.ts`. - `primeFiles` (Code/process.js): parse `entity_id` from the persisted `codeEnvIdentifier` query string once per iteration; forward through `pushFile`, including the reupload path which re-parses the fresh identifier returned by codeapi. - Tests: extend `skillFiles.spec.ts` with two cases — fresh-upload propagation and cached-hot-path parsing. Companion PRs in flight on `@librechat/agents` (forward `entity_id` through `_injected_files`) and codeapi (per-file authorization). All three are wire-back-compat: an absent `entity_id` falls back to the existing request-level resolution. * 🔧 chore: Update dependencies in package-lock.json and package.json - Bump `@librechat/agents` to version `3.1.78-dev.0` across multiple package files. - Upgrade `@langchain/langgraph-checkpoint` to version `1.0.2` and update its peer dependency for `@langchain/core` to `^1.1.44`. - Update `axios` to version `1.16.0` and `follow-redirects` to version `1.16.0`. - Add `@types/diff` as a new dependency at version `7.0.2` and include `diff` at version `9.0.0` in the `@librechat/agents` module. - Introduce optional peer dependency `@anthropic-ai/sandbox-runtime` for `@librechat/agents` with metadata indicating it is optional. * 🐛 fix: Make skill code-env cache persistence observable Two changes to surface the skill-bundle re-upload issue without behavioral changes to tenant scoping (root cause to be confirmed via the new warn log): 1. `primeSkillFiles` now awaits `updateSkillFileCodeEnvIds` instead of firing-and-forgetting it. The prior shape could race with the next prime (read-before-write) even when the bulkWrite itself succeeds, producing a silent cache miss. Latency cost: ~10–50ms on first prime; in exchange every subsequent prime can rely on the identifier being persisted by the time it reads. 2. `updateSkillFileCodeEnvIds` now returns `{matchedCount, modifiedCount}` from the underlying bulkWrite. `primeSkillFiles` warn-logs when `modifiedCount < updates.length`, making any silent drop visible — whether the cause is tenant filtering, a `relativePath` mismatch, schema-plugin scoping, or something else. Prior shape returned `Promise` so any zero-modification result was invisible. Tests: - `skill.spec.ts`: real-MongoDB happy path (counts match), no-match case (modifiedCount=0), and empty-input contract. - `skillFiles.spec.ts`: deferred-promise harness proving the call site awaits the persist (prime stays pending until the persist resolves) and forwards partial-write counts. Deliberately narrower than the original draft of this commit, which also bypassed `tenantSafeBulkWrite` for the codeEnvIdentifier write on the speculative diagnosis that tenant filtering was the cause. That change was a behavior shift on tenant scoping without confirmation; reverted pending real-world signal from the new warn log. * 🐛 fix: Justify await for skill code-env persistence under concurrency The await on `updateSkillFileCodeEnvIds` isn't a defensive nicety — it's load-bearing for cache effectiveness under concurrent priming. Verified with an out-of-tree harness (`config/test-skill-cache.ts`, not committed) that wires `primeSkillFiles` against a real codeapi stack: - With fire-and-forget (prior shape after this branch's revert): back-to-back primes for the same skill miss the cache. Call N+1 reads SkillFile docs before Call N's write commits, sees no `codeEnvIdentifier`, re-uploads, fires its own forget that Call N+2 also races. Steady-state stays in cache miss for the full burst. - With await: the prime that does the upload commits its persist before resolving, so the next concurrent prime observes the cache pointer instead of racing the read. Latency cost ~10–50ms on the upload prime; subsequent concurrent primes save an entire batch upload. In production with primes seconds apart this race is rare; at scale with many users hitting the same skill in the same second it's the difference between M and N×M uploads. Updates the regression test to assert the await contract (deferred persist promise → prime stays pending until persist resolves). Comment in `skillFiles.ts` rewritten to document the concurrency rationale rather than the weaker "race-with-next-prime" framing the prior commit used. --- api/package.json | 2 +- api/server/services/Files/Code/process.js | 31 ++-- package-lock.json | 53 ++++-- packages/api/package.json | 2 +- packages/api/src/agents/skillFiles.spec.ts | 151 ++++++++++++++++++ packages/api/src/agents/skillFiles.ts | 75 +++++++-- .../data-schemas/src/methods/skill.spec.ts | 56 +++++++ packages/data-schemas/src/methods/skill.ts | 20 ++- 8 files changed, 350 insertions(+), 40 deletions(-) diff --git a/api/package.json b/api/package.json index c7bdd1145a..fb1ef00966 100644 --- a/api/package.json +++ b/api/package.json @@ -45,7 +45,7 @@ "@azure/storage-blob": "^12.30.0", "@google/genai": "^1.19.0", "@keyv/redis": "^4.3.3", - "@librechat/agents": "^3.1.77", + "@librechat/agents": "^3.1.78-dev.0", "@librechat/api": "*", "@librechat/data-schemas": "*", "@microsoft/microsoft-graph-client": "^3.0.7", diff --git a/api/server/services/Files/Code/process.js b/api/server/services/Files/Code/process.js index 3594cd2cb0..4348d2766f 100644 --- a/api/server/services/Files/Code/process.js +++ b/api/server/services/Files/Code/process.js @@ -430,9 +430,14 @@ const primeFiles = async (options) => { const [path, queryString] = file.metadata.fileIdentifier.split('?'); const [session_id, id] = path.split('/'); + let queryParams = {}; + if (queryString) { + queryParams = Object.fromEntries(new URLSearchParams(queryString).entries()); + } + /** * `pushFile` accepts optional overrides so the reupload path can - * push the FRESH `(session_id, id)` parsed off the new + * push the FRESH `(session_id, id, entity_id)` parsed off the new * `fileIdentifier`. Without these overrides, the closure would * capture the stale pre-reupload refs from the outer loop and * the in-memory `files` array (now consumed by @@ -441,8 +446,12 @@ const primeFiles = async (options) => { * gets the new identifier via `updateFile`, but the seed would * still inject the old one — bash_tool / read_file would 404 * trying to mount the file until the next turn re-reads metadata. + * + * `entity_id` is forwarded so codeapi can resolve sessionKey + * per-file, allowing one execute to mix files uploaded under + * different entities (e.g. a skill bundle plus a user attachment). */ - const pushFile = (overrideSessionId, overrideId) => { + const pushFile = (overrideSessionId, overrideId, overrideEntityId) => { if (!toolContext) { toolContext = `- Note: The following files are available in the "${Tools.execute_code}" tool environment:`; } @@ -455,11 +464,13 @@ const primeFiles = async (options) => { : ' (attached by user)'; } + const entity_id = overrideEntityId ?? queryParams.entity_id; toolContext += `\n\t- /mnt/data/${file.filename}${fileSuffix}`; files.push({ id: overrideId ?? id, session_id: overrideSessionId ?? session_id, name: file.filename, + ...(entity_id ? { entity_id } : {}), }); }; @@ -468,11 +479,6 @@ const primeFiles = async (options) => { continue; } - let queryParams = {}; - if (queryString) { - queryParams = Object.fromEntries(new URLSearchParams(queryString).entries()); - } - const reuploadFile = async () => { try { const { getDownloadStream } = getStrategyFunctions(file.source); @@ -504,11 +510,18 @@ const primeFiles = async (options) => { * top of this iteration refer to the old, expired/missing * sandbox object — using them here would silently re-introduce * the bug `Graph.sessions` seeding is supposed to fix. + * + * `entity_id` survives the round-trip: the upload was tagged + * with `queryParams.entity_id` above, so the new identifier + * carries the same scope. */ - const [newPath] = fileIdentifier.split('?'); + const [newPath, newQuery] = fileIdentifier.split('?'); const [newSessionId, newId] = newPath.split('/'); + const newQueryParams = newQuery + ? Object.fromEntries(new URLSearchParams(newQuery).entries()) + : {}; sessions.set(newSessionId, true); - pushFile(newSessionId, newId); + pushFile(newSessionId, newId, newQueryParams.entity_id); } catch (error) { logger.error( `Error re-uploading file ${id} in session ${session_id}: ${error.message}`, diff --git a/package-lock.json b/package-lock.json index d578383bce..9280ec1208 100644 --- a/package-lock.json +++ b/package-lock.json @@ -60,7 +60,7 @@ "@azure/storage-blob": "^12.30.0", "@google/genai": "^1.19.0", "@keyv/redis": "^4.3.3", - "@librechat/agents": "^3.1.77", + "@librechat/agents": "^3.1.78-dev.0", "@librechat/api": "*", "@librechat/data-schemas": "*", "@microsoft/microsoft-graph-client": "^3.0.7", @@ -11683,9 +11683,9 @@ } }, "node_modules/@langchain/langgraph-checkpoint": { - "version": "1.0.1", - "resolved": "https://registry.npmjs.org/@langchain/langgraph-checkpoint/-/langgraph-checkpoint-1.0.1.tgz", - "integrity": "sha512-HM0cJLRpIsSlWBQ/xuDC67l52SqZ62Bh2Y61DX+Xorqwoh5e1KxYvfCD7GnSTbWWhjBOutvnR0vPhu4orFkZfw==", + "version": "1.0.2", + "resolved": "https://registry.npmjs.org/@langchain/langgraph-checkpoint/-/langgraph-checkpoint-1.0.2.tgz", + "integrity": "sha512-F4E5Tr0nt8FGghgdscJtHw+ABzChOHeI80R7Y1pjIHdiJom6c2ieo76vL+FWiny80JmoGqhrVAEIWrw0cXKPxg==", "license": "MIT", "dependencies": { "uuid": "^10.0.0" @@ -11694,7 +11694,7 @@ "node": ">=18" }, "peerDependencies": { - "@langchain/core": "^1.0.1" + "@langchain/core": "^1.1.44" } }, "node_modules/@langchain/langgraph-checkpoint/node_modules/uuid": { @@ -11988,9 +11988,9 @@ } }, "node_modules/@librechat/agents": { - "version": "3.1.77", - "resolved": "https://registry.npmjs.org/@librechat/agents/-/agents-3.1.77.tgz", - "integrity": "sha512-qPuINvvSHd3ZoHWtHkiyt9O2l4B+G4Lbb9oXYz2HGzzCrrX1NPw021DvAA+vmnAFm0mRGEQwfZBed1tBochHxw==", + "version": "3.1.78-dev.0", + "resolved": "https://registry.npmjs.org/@librechat/agents/-/agents-3.1.78-dev.0.tgz", + "integrity": "sha512-XOVS1pyBEV/HaBfmOasuT8wXceBhGzvk3gM5ba1iY+u4RS6HM53lRCvn1FX7vCBWv9anc/eV+m9G60tJbnZ/Sg==", "license": "MIT", "dependencies": { "@anthropic-ai/sdk": "^0.92.0", @@ -12013,9 +12013,11 @@ "@langfuse/tracing": "^4.3.0", "@opentelemetry/sdk-node": "^0.207.0", "@scarf/scarf": "^1.4.0", + "@types/diff": "^7.0.2", "ai-tokenizer": "^1.0.6", - "axios": "^1.15.0", + "axios": "^1.16.0", "cheerio": "^1.0.0", + "diff": "^9.0.0", "dotenv": "^16.4.7", "https-proxy-agent": "^7.0.6", "mathjs": "^15.2.0", @@ -12026,6 +12028,14 @@ }, "engines": { "node": ">=20.0.0" + }, + "peerDependencies": { + "@anthropic-ai/sandbox-runtime": "^0.0.49" + }, + "peerDependenciesMeta": { + "@anthropic-ai/sandbox-runtime": { + "optional": true + } } }, "node_modules/@librechat/agents/node_modules/@langchain/langgraph": { @@ -12067,6 +12077,15 @@ "uuid": "dist/bin/uuid" } }, + "node_modules/@librechat/agents/node_modules/diff": { + "version": "9.0.0", + "resolved": "https://registry.npmjs.org/diff/-/diff-9.0.0.tgz", + "integrity": "sha512-svtcdpS8CgJyqAjEQIXdb3OjhFVVYjzGAPO8WGCmRbrml64SPw/jJD4GoE98aR7r25A0XcgrK3F02yw9R/vhQw==", + "license": "BSD-3-Clause", + "engines": { + "node": ">=0.3.1" + } + }, "node_modules/@librechat/agents/node_modules/openai": { "version": "6.35.0", "resolved": "https://registry.npmjs.org/openai/-/openai-6.35.0.tgz", @@ -21008,6 +21027,12 @@ "@types/ms": "*" } }, + "node_modules/@types/diff": { + "version": "7.0.2", + "resolved": "https://registry.npmjs.org/@types/diff/-/diff-7.0.2.tgz", + "integrity": "sha512-JSWRMozjFKsGlEjiiKajUjIJVKuKdE3oVy2DNtK+fUo8q82nhFZ2CPQwicAIkXrofahDXrWJ7mjelvZphMS98Q==", + "license": "MIT" + }, "node_modules/@types/estree": { "version": "1.0.8", "resolved": "https://registry.npmjs.org/@types/estree/-/estree-1.0.8.tgz", @@ -22646,12 +22671,12 @@ } }, "node_modules/axios": { - "version": "1.15.0", - "resolved": "https://registry.npmjs.org/axios/-/axios-1.15.0.tgz", - "integrity": "sha512-wWyJDlAatxk30ZJer+GeCWS209sA42X+N5jU2jy6oHTp7ufw8uzUTVFBX9+wTfAlhiJXGS0Bq7X6efruWjuK9Q==", + "version": "1.16.0", + "resolved": "https://registry.npmjs.org/axios/-/axios-1.16.0.tgz", + "integrity": "sha512-6hp5CwvTPlN2A31g5dxnwAX0orzM7pmCRDLnZSX772mv8WDqICwFjowHuPs04Mc8deIld1+ejhtaMn5vp6b+1w==", "license": "MIT", "dependencies": { - "follow-redirects": "^1.15.11", + "follow-redirects": "^1.16.0", "form-data": "^4.0.5", "proxy-from-env": "^2.1.0" } @@ -44421,7 +44446,7 @@ "@azure/storage-blob": "^12.30.0", "@google/genai": "^1.19.0", "@keyv/redis": "^4.3.3", - "@librechat/agents": "^3.1.77", + "@librechat/agents": "^3.1.78-dev.0", "@librechat/data-schemas": "*", "@modelcontextprotocol/sdk": "^1.29.0", "@smithy/node-http-handler": "^4.4.5", diff --git a/packages/api/package.json b/packages/api/package.json index 6a8a408a97..fe836a9862 100644 --- a/packages/api/package.json +++ b/packages/api/package.json @@ -98,7 +98,7 @@ "@azure/storage-blob": "^12.30.0", "@google/genai": "^1.19.0", "@keyv/redis": "^4.3.3", - "@librechat/agents": "^3.1.77", + "@librechat/agents": "^3.1.78-dev.0", "@librechat/data-schemas": "*", "@modelcontextprotocol/sdk": "^1.29.0", "@smithy/node-http-handler": "^4.4.5", diff --git a/packages/api/src/agents/skillFiles.spec.ts b/packages/api/src/agents/skillFiles.spec.ts index 4303a69589..e4475c2048 100644 --- a/packages/api/src/agents/skillFiles.spec.ts +++ b/packages/api/src/agents/skillFiles.spec.ts @@ -127,4 +127,155 @@ describe('primeInvokedSkills — execute_code capability gate', () => { expect(result).toEqual({}); expect(deps.getSkillByName).not.toHaveBeenCalled(); }); + + it('forwards entity_id on every file in initialSessions after fresh upload', async () => { + /* Regression: codeapi now resolves sessionKey per-file using `entity_id`. + * Skill files must carry `entity_id=` through priming so that + * a subsequent execute mixing skill files and a user attachment can be + * authorized — both files in the same call resolve to their own scope + * instead of collapsing onto a single request-level entity. */ + const listSkillFiles = jest.fn().mockResolvedValue([ + { + relativePath: 'references/style.md', + filename: 'style.md', + filepath: '/storage/brand-guidelines/references/style.md', + source: 's3', + bytes: 256, + }, + ]); + const getStrategyFunctions = jest.fn().mockReturnValue({ + getDownloadStream: jest.fn().mockResolvedValue(Readable.from(Buffer.from(''))), + }); + const batchUploadCodeEnvFiles = jest.fn().mockResolvedValue({ + session_id: 'session-42', + files: [{ fileId: 'file-1', filename: 'brand-guidelines/references/style.md' }], + }); + + const deps = makeDeps({ + codeEnvAvailable: true, + listSkillFiles, + getStrategyFunctions, + batchUploadCodeEnvFiles, + }); + + const result = await primeInvokedSkills(deps); + + const codeSession = result.initialSessions?.get('execute_code'); + expect(codeSession?.files).toEqual([ + { + id: 'file-1', + name: 'brand-guidelines/references/style.md', + session_id: 'session-42', + entity_id: SKILL_ID.toString(), + }, + ]); + }); + + it('awaits updateSkillFileCodeEnvIds before resolving to avoid concurrent-prime cache misses', async () => { + /* Concurrency regression: when many users hit the same skill at + * once, fire-and-forget keeps the cache in miss-steady-state for + * the burst — User N's prime reads SkillFile docs before User N-1's + * persist commits, sees no `codeEnvIdentifier`, re-uploads, and + * fires its own forget that User N+1 also races. Awaiting the + * persist before the prime resolves ensures the next concurrent + * caller observes the cache pointer instead of racing a write. */ + const fileRecords = [ + { + relativePath: 'references/style.md', + filename: 'style.md', + filepath: '/storage/brand-guidelines/references/style.md', + source: 's3', + bytes: 256, + }, + ]; + const listSkillFiles = jest.fn().mockResolvedValue(fileRecords); + const getStrategyFunctions = jest.fn().mockReturnValue({ + getDownloadStream: jest.fn().mockResolvedValue(Readable.from(Buffer.from(''))), + }); + const batchUploadCodeEnvFiles = jest.fn().mockResolvedValue({ + session_id: 'session-42', + files: [{ fileId: 'file-1', filename: 'brand-guidelines/references/style.md' }], + }); + + /* Defer resolution so we can assert the prime hasn't returned yet + * — proves the call site is awaiting, not fire-and-forget. */ + let resolvePersist!: (v: { matchedCount: number; modifiedCount: number }) => void; + const persistGate = new Promise<{ matchedCount: number; modifiedCount: number }>((r) => { + resolvePersist = r; + }); + const updateSkillFileCodeEnvIds = jest.fn().mockReturnValue(persistGate); + + const deps = makeDeps({ + codeEnvAvailable: true, + listSkillFiles, + getStrategyFunctions, + batchUploadCodeEnvFiles, + updateSkillFileCodeEnvIds, + }); + + let resolved = false; + const primePromise = primeInvokedSkills(deps).then((r) => { + resolved = true; + return r; + }); + + /* Drain the microtask queue. The prime should still be pending + * because persistGate hasn't resolved. */ + await new Promise((r) => setImmediate(r)); + expect(resolved).toBe(false); + + /* Resolve as a successful write to confirm the prime completes + * after the persist returns. */ + resolvePersist({ matchedCount: 1, modifiedCount: 1 }); + await primePromise; + + expect(updateSkillFileCodeEnvIds).toHaveBeenCalledTimes(1); + const [updates] = updateSkillFileCodeEnvIds.mock.calls[0]; + expect(updates).toEqual([ + { + skillId: SKILL_ID, + relativePath: 'references/style.md', + codeEnvIdentifier: `session-42/file-1?entity_id=${SKILL_ID.toString()}`, + }, + ]); + }); + + it('parses entity_id off codeEnvIdentifier in the cached-skills hot path', async () => { + /* When all skill files are still active in codeapi, primeInvokedSkills + * skips the batch upload entirely and reconstructs file refs from each + * skill file's persisted `codeEnvIdentifier`. The query string carries + * `?entity_id=`, which must survive into `_injected_files` so + * downstream authorization still uses the skill's scope. */ + const listSkillFiles = jest.fn().mockResolvedValue([ + { + relativePath: 'references/style.md', + filename: 'style.md', + filepath: '/storage/brand-guidelines/references/style.md', + source: 's3', + bytes: 256, + codeEnvIdentifier: `session-cached/file-cached?entity_id=${SKILL_ID.toString()}`, + }, + ]); + const batchUploadCodeEnvFiles = jest.fn(); + const deps = makeDeps({ + codeEnvAvailable: true, + listSkillFiles, + batchUploadCodeEnvFiles, + getSessionInfo: jest.fn().mockResolvedValue('2026-05-05T00:00:00Z'), + checkIfActive: jest.fn().mockReturnValue(true), + }); + + const result = await primeInvokedSkills(deps); + + expect(batchUploadCodeEnvFiles).not.toHaveBeenCalled(); + const codeSession = result.initialSessions?.get('execute_code'); + expect(codeSession?.files).toEqual([ + { + id: 'file-cached', + name: 'brand-guidelines/references/style.md', + session_id: 'session-cached', + entity_id: SKILL_ID.toString(), + }, + ]); + }); }); diff --git a/packages/api/src/agents/skillFiles.ts b/packages/api/src/agents/skillFiles.ts index 989e27f384..9f274f6a99 100644 --- a/packages/api/src/agents/skillFiles.ts +++ b/packages/api/src/agents/skillFiles.ts @@ -39,19 +39,31 @@ export interface PrimeSkillFilesParams { getSessionInfo?: (fileIdentifier: string) => Promise; /** 23-hour freshness check */ checkIfActive?: (dateString: string) => boolean; - /** Persists codeEnvIdentifier on skill files after upload */ + /** Persists codeEnvIdentifier on skill files after upload. Implementations + * warn-log on partial writes (matchedCount/modifiedCount mismatch) + * internally — caller can fire-and-forget without losing visibility. */ updateSkillFileCodeEnvIds?: ( updates: Array<{ skillId: Types.ObjectId | string; relativePath: string; codeEnvIdentifier: string; }>, - ) => Promise; + ) => Promise<{ matchedCount: number; modifiedCount: number } | void>; } export interface PrimeSkillFilesResult { session_id: string; - files: Array<{ id: string; session_id: string; name: string }>; + files: Array<{ id: string; session_id: string; name: string; entity_id?: string }>; +} + +/** Parses `entity_id` out of a persisted `codeEnvIdentifier`'s query string. + * Returns `undefined` when the identifier has no query string or no + * `entity_id` (legacy user-attachment uploads). */ +function parseEntityIdFromCodeEnvIdentifier(identifier: string): string | undefined { + const queryString = identifier.split('?')[1]; + if (!queryString) return undefined; + const value = new URLSearchParams(queryString).get('entity_id'); + return value && value.length > 0 ? value : undefined; } /** @@ -109,8 +121,15 @@ export async function primeSkillFiles( if (allActive) { const files: PrimeSkillFilesResult['files'] = []; for (const sf of skillFiles) { - const [sid, fid] = (sf.codeEnvIdentifier as string).split('?')[0].split('/'); - files.push({ id: fid, session_id: sid, name: `${skill.name}/${sf.relativePath}` }); + const identifier = sf.codeEnvIdentifier as string; + const [sid, fid] = identifier.split('?')[0].split('/'); + const entity_id = parseEntityIdFromCodeEnvIdentifier(identifier); + files.push({ + id: fid, + session_id: sid, + name: `${skill.name}/${sf.relativePath}`, + ...(entity_id != null ? { entity_id } : {}), + }); } if (files.length > 0) { @@ -183,6 +202,7 @@ export async function primeSkillFiles( id: f.fileId, session_id: result.session_id, name: f.filename, + entity_id: entityId, })); // Treat partial upload failures as a priming failure — missing bundled @@ -199,7 +219,19 @@ export async function primeSkillFiles( return null; } - // Persist codeEnvIdentifiers on skill files (fire-and-forget) + /** + * Persist codeEnvIdentifiers on skill files. Awaited (not + * fire-and-forget) so the next prime — which can start within + * milliseconds when many users hit the same skill concurrently — + * sees the cache pointer instead of racing the read against an + * in-flight write. Without the await, a fire-and-forget under + * concurrency stays in cache-miss steady-state for the duration + * of the burst (each user's prime reads stale, re-uploads, then + * fires its own forget that the next user also misses). Latency + * cost is ~10–50ms on the prime that does the upload; subsequent + * primes save an entire batch upload. Failures don't fail the + * prime — the file refs returned to the caller are still valid. + */ if (updateSkillFileCodeEnvIds) { const updates = result.files .filter((f) => !f.filename.endsWith('/SKILL.md')) @@ -209,12 +241,14 @@ export async function primeSkillFiles( codeEnvIdentifier: `${result.session_id}/${f.fileId}?entity_id=${entityId}`, })); if (updates.length > 0) { - updateSkillFileCodeEnvIds(updates).catch((err: unknown) => { + try { + await updateSkillFileCodeEnvIds(updates); + } catch (err: unknown) { logger.warn( '[primeSkillFiles] Failed to persist codeEnvIdentifiers:', err instanceof Error ? err.message : err, ); - }); + } } } @@ -349,8 +383,15 @@ export async function primeInvokedSkills( r.files .filter((f) => f.codeEnvIdentifier) .map((f) => { - const [sid, fid] = (f.codeEnvIdentifier as string).split('?')[0].split('/'); - return { id: fid, name: `${r.skill.name}/${f.relativePath}`, session_id: sid }; + const identifier = f.codeEnvIdentifier as string; + const [sid, fid] = identifier.split('?')[0].split('/'); + const entity_id = parseEntityIdFromCodeEnvIdentifier(identifier); + return { + id: fid, + name: `${r.skill.name}/${f.relativePath}`, + session_id: sid, + ...(entity_id != null ? { entity_id } : {}), + }; }), ); if (cachedFiles.length > 0) { @@ -374,7 +415,12 @@ export async function primeInvokedSkills( // Per-skill upload: each skill gets its own session with entity_id=skillId. // primeSkillFiles handles freshness caching per-skill, so only expired // skills re-upload. The code env handles mixed session_ids natively. - const allPrimedFiles: Array<{ id: string; name: string; session_id: string }> = []; + const allPrimedFiles: Array<{ + id: string; + name: string; + session_id: string; + entity_id?: string; + }> = []; const primeResults = await Promise.allSettled( fileListResults.map(async ({ skill, files }) => { const result = await primeSkillFiles({ @@ -393,7 +439,12 @@ export async function primeInvokedSkills( for (const r of primeResults) { if (r.status === 'fulfilled' && r.value.result) { for (const f of r.value.result.files) { - allPrimedFiles.push({ id: f.id, name: f.name, session_id: f.session_id }); + allPrimedFiles.push({ + id: f.id, + name: f.name, + session_id: f.session_id, + ...(f.entity_id != null ? { entity_id: f.entity_id } : {}), + }); } } else if (r.status === 'rejected') { logger.warn('[primeInvokedSkills] Failed to prime skill files:', r.reason); diff --git a/packages/data-schemas/src/methods/skill.spec.ts b/packages/data-schemas/src/methods/skill.spec.ts index a3c62887eb..e28269152c 100644 --- a/packages/data-schemas/src/methods/skill.spec.ts +++ b/packages/data-schemas/src/methods/skill.spec.ts @@ -1365,6 +1365,62 @@ describe('SkillFile methods', () => { }), ).rejects.toMatchObject({ code: 'SKILL_FILE_VALIDATION_FAILED' }); }); + + describe('updateSkillFileCodeEnvIds (skill-bundle cache pointer)', () => { + /** + * The returned `{matchedCount, modifiedCount}` shape is the diagnostic + * contract `primeSkillFiles` relies on — a silently-dropped write + * turns every subsequent prime into a fresh codeapi upload (N×M + * egress per chat load). Pinning the contract here so the caller + * can warn-log on partial writes instead of failing closed. + */ + it('persists codeEnvIdentifier and reports matched/modified counts', async () => { + const { skill } = await methods.createSkill(makeSkillInput()); + await methods.upsertSkillFile({ + skillId: skill._id, + relativePath: 'scripts/a.sh', + file_id: 'f1', + filename: 'a.sh', + filepath: '/a', + source: 'local', + mimeType: 'text/plain', + bytes: 1, + author: owner._id, + }); + + const result = await methods.updateSkillFileCodeEnvIds([ + { + skillId: skill._id, + relativePath: 'scripts/a.sh', + codeEnvIdentifier: `session-1/file-1?entity_id=${skill._id.toString()}`, + }, + ]); + + expect(result.matchedCount).toBe(1); + expect(result.modifiedCount).toBe(1); + + const files = await methods.listSkillFiles(skill._id); + expect(files[0].codeEnvIdentifier).toBe(`session-1/file-1?entity_id=${skill._id.toString()}`); + }); + + it('reports modifiedCount=0 when no SkillFile rows match the (skillId, relativePath) filter', async () => { + const { skill } = await methods.createSkill(makeSkillInput()); + const result = await methods.updateSkillFileCodeEnvIds([ + { + skillId: skill._id, + relativePath: 'does/not/exist.sh', + codeEnvIdentifier: 'sid/fid?entity_id=x', + }, + ]); + expect(result.modifiedCount).toBe(0); + expect(result.matchedCount).toBe(0); + }); + + it('returns zero counts when called with an empty update list', async () => { + const result = await methods.updateSkillFileCodeEnvIds([]); + expect(result).toEqual({ matchedCount: 0, modifiedCount: 0 }); + }); + }); }); describe('deleteUserSkills', () => { diff --git a/packages/data-schemas/src/methods/skill.ts b/packages/data-schemas/src/methods/skill.ts index 79387bc7a2..2e2ca5f052 100644 --- a/packages/data-schemas/src/methods/skill.ts +++ b/packages/data-schemas/src/methods/skill.ts @@ -1491,8 +1491,8 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk relativePath: string; codeEnvIdentifier: string; }>, - ): Promise { - if (updates.length === 0) return; + ): Promise<{ matchedCount: number; modifiedCount: number }> { + if (updates.length === 0) return { matchedCount: 0, modifiedCount: 0 }; const SkillFile = mongoose.models.SkillFile as Model; const ops = updates.map((u) => ({ updateOne: { @@ -1500,7 +1500,21 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk update: { $set: { codeEnvIdentifier: u.codeEnvIdentifier } }, }, })); - await tenantSafeBulkWrite(SkillFile, ops); + + /** + * The returned `{matchedCount, modifiedCount}` lets callers warn on + * partial writes — a silent miss here turns every subsequent prime + * into a fresh upload (massive egress at scale). If the wrapper's + * tenant injection ends up dropping rows, the warn log makes it + * visible instead of failing closed. + */ + const result = await tenantSafeBulkWrite(SkillFile, ops); + if (result.modifiedCount < updates.length) { + logger.warn( + `[updateSkillFileCodeEnvIds] Persisted ${result.modifiedCount}/${updates.length} codeEnvIdentifiers (matched ${result.matchedCount}). Subsequent primes for unmatched files will re-upload.`, + ); + } + return { matchedCount: result.matchedCount, modifiedCount: result.modifiedCount }; } return {