mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-05-13 16:07:30 +00:00
📂 fix: Preserve Nested Folder Paths for Code-Execution Artifacts (#12848)
* 📂 fix: Preserve Nested Folder Paths for Code-Execution Artifacts When codeapi reports a generated file at a nested path (`a/b/file.txt`), `processCodeOutput` was running it through `sanitizeFilename` — which calls `path.basename()` and then collapses `/` to `_`. The DB row ended up with `filename: "file.txt"`, `primeFiles` shipped that flat name back to the next sandbox session, and `cat /mnt/data/a/b/file.txt` 404'd. Fix: split the sanitizer into two helpers in `packages/api/src/utils/files.ts`: - `sanitizeArtifactPath` — segment-wise sanitize while preserving `/`. Falls back to basename on `..` traversal, absolute paths, and other malformed inputs. The DB record uses this so the next prime() can recreate the nested path in the sandbox. - `flattenArtifactPath` — encode `/` as `__` for the local `saveBuffer` strategies, which key by single-component filename and would otherwise create unintended subdirectories under uploads/. `process.js` is updated to use both: DB filename keeps the path, storage key flattens. `claimCodeFile` is also keyed on `safeName` so the (filename, conversationId) compound key stays consistent with the record `createFile` writes. Tests: +13 unit tests in `files.spec.ts` (sanitizeArtifactPath table, flattenArtifactPath round-trip). +1 integration test in `process.spec.js` asserting the DB-row vs storage-key split for a nested path. Updated `process-traversal.spec.js` to mock the new helpers. 64 pass / 0 fail across `Files/Code/`; 36 pass / 0 fail in `packages/api/src/utils/files.spec.ts`. Companion: ClickHouse/ai#1327 — the codeapi-side counterpart that stops phantom file IDs from reaching this code path in the first place. These two are independent but the matplotlib bug is most cleanly resolved when both ship. * 🛡️ fix: Re-add 255-char per-segment cap in sanitizeArtifactPath (codex review P2) `sanitizeArtifactPath` dropped the 255-char basename cap that `sanitizeFilename` enforces. Long artifact names then flowed unbounded into `processCodeOutput`'s storage key (`${file_id}__${flatName}`) and tripped `ENAMETOOLONG` on filesystems that enforce `NAME_MAX` — saveBuffer fails, and the file falls back to a download URL instead of persisting / priming. This was a regression specifically for flat filenames that the original `sanitizeFilename` would have truncated safely. Re-add the cap as a per-path-component limit so it applies cleanly to both flat and nested paths: - Leaf segment: extension-preserving truncation, matching `sanitizeFilename`'s shape (`<truncated-stem>-<6 hex>.<ext>`). - Non-leaf (directory) segments: plain truncate-and-disambiguate (`<truncated-name>-<6 hex>`); directory names don't carry semantic extensions worth preserving. - Defensive fallback when `path.extname` returns a pathologically long "extension" (e.g. `_.aaaa…aaa` after the dotfile underscore prefix rewrite turns a long hidden file into a non-dotfile with a 300-char "extension"): collapse to whole-segment truncation rather than leaving the cap unmet. +6 unit tests covering: long leaf (regression case), long leaf under a preserved directory, long non-leaf segment, deeply nested mixed-length, exact-255 boundary (no truncation), and the dotfile + truncation interaction. * 🛡️ fix: Cap flattened storage key against NAME_MAX in processCodeOutput (codex review P1) Per-segment caps on the path-preserving form aren't enough. Once segments are joined with `__` for the storage key, deeply-nested or moderately long paths can still produce a flat form that overflows once `${file_id}__` is prepended — `${file_id}__a__b__c.csv` for a 3-level 100-char-each path is ~344 chars, well past filesystem NAME_MAX (255). saveBuffer then trips ENAMETOOLONG and falls back to a download URL, and the artifact never persists / primes. `flattenArtifactPath` gets an optional `maxLength` parameter. When set, the function truncates the flat form to fit, preserving the leaf extension with the same disambiguating-hex-suffix shape sanitizeFilename uses. Default (`undefined`) keeps existing call sites uncapped — the cap is opt-in for callers that are actually building a filesystem key. Pathologically long "extensions" from `path.extname` (e.g. `.aaaa…aaa`) fall back to whole-key truncation rather than leaving the cap unmet. processCodeOutput composes the storage key after `file_id` is known and passes `255 - file_id.length - 2` as the budget so the full `${file_id}__${flatName}` string fits in one filesystem path component. +7 unit tests in files.spec.ts: - Pass-through when no maxLength supplied (cap is opt-in). - Pass-through when flat form fits within maxLength. - Truncation with leaf extension preserved (the regression case). - Leaf-only overflow with extension preservation. - Pathological long-extension fallback (whole-key truncation). - No-extension stem truncation. - Boundary equality (off-by-one guard). +1 integration test in process.spec.js: processCodeOutput passes the file_id-aware budget (`255 - file_id.length - 2`) to flattenArtifactPath. 114/114 across files.spec.ts + Files/Code (49 + 65). * 🛡️ fix: Determinize + clamp artifact-path truncation (codex review P2 ×2) Two follow-ups to Codex review on the path/flat-key cap: 1. **Deterministic truncation suffixes**. The previous helpers used `crypto.randomBytes(3)` for the disambiguator, mirroring `sanitizeFilename`'s shape. That made the truncated form non- deterministic: a re-upload of the same long filename would compute a *different* storage key, orphaning the previous on-disk file under the reused `file_id` returned by `claimCodeFile`. New `deterministicHexSuffix(input)` helper hashes the input with SHA-256 and takes the first 6 hex chars. Same input → same suffix (storage key stable across re-uploads); different inputs sharing a truncation prefix still get different suffixes (collision avoidance). 24 bits ≈ 16M values is collision-safe for our scale (single-digit artifacts per turn per (filename, conversationId) bucket). Applied to `truncateLeafSegment`, `truncateDirSegment`, and `flattenArtifactPath` — every truncation site in the new helpers. `sanitizeFilename` (pre-existing) is intentionally left alone; its tests rely on the random-bytes mock and it's outside this PR's scope. 2. **Final clamp on flattenArtifactPath result**. The old `Math.max(1, maxLength - ext.length - 7)` floor could let the result slip past `maxLength` when the extension was nearly as large as the budget (e.g. `maxLength=5`, `ext=".txt"`: budget computed as 0, but result was `-<6 hex>.txt` = 11 chars). Drop the `Math.max(1, …)` floor and add a final `truncated.slice(0, maxLength)` so the contract holds for any input. Also short-circuit `maxLength <= 0` to `''` for pathological budgets. Tests updated to compute the expected hash inline (the existing `randomBytes` mock doesn't apply to the new code path), plus 4 new regression tests: - sanitizeArtifactPath: same input → same output, different inputs → different outputs (determinism + collision avoidance). - flattenArtifactPath: same input → same output, different inputs sharing a truncation prefix → different outputs. - flattenArtifactPath: clamp holds when ext.length > maxLength - 7. - flattenArtifactPath: returns '' for maxLength <= 0. 53 unit tests pass. 65 integration tests pass. * 🛡️ fix: Total-path cap + basename for classifier (codex P2 + comprehensive review) Four follow-ups from the latest reviews on this PR: 1. **Codex P2: total-path cap in sanitizeArtifactPath**. Per-segment caps weren't enough — a deeply nested path (3+ at-cap segments) can still produce a joined form past Mongo's 1024-byte indexed-key limit (4.0 and earlier reject; later versions configurable). Added `ARTIFACT_PATH_TOTAL_MAX = 512` and a leaf-only fallback when the joined form exceeds it. Same shape as the absolute-path / `..`-traversal fallbacks above; the leaf is already segment-capped to ≤255, so the final result stays within bounds. 2. **Codex P2: pass basename to classifier/extractor in process.js**. With the path-preserving sanitizer, `safeName` can now be a nested string like `reports.v1/Makefile`. The classifier's `extensionOf` reads that as `v1/Makefile` (the slice after the dot in the directory name) and the bare-name branch rejects because it sees a `.` anywhere. Result: extensionless artifacts under dotted folders (Makefile, Dockerfile, etc.) get misclassified as `other` and skip text extraction. Pass `path.basename(safeName)` to both `classifyCodeArtifact` and `extractCodeArtifactText` so classification matches what the old flat-name flow produced. 3. **Review nit: drop dead `sanitizeFilename` mock in process.spec.js**. process.js no longer imports `sanitizeFilename`; the mock was misleading dead code. 4. **Review nit: rename misleading `'embedded parent traversal'` test**. `path.posix.normalize('a/../escape.txt')` resolves to `escape.txt` which goes through the normal segment-split path, not the `sanitizeFilename` fallback. Test name now says "resolves embedded parent traversal via path normalization" to match the actual code path. +3 regression tests: - sanitizeArtifactPath falls back to leaf-only when joined > 512. - sanitizeArtifactPath keeps nested path within the 512 budget. - process.spec: passes basename (`Makefile` from `reports.v1/Makefile`) to classifyCodeArtifact + extractCodeArtifactText. Existing "caps every segment in a deeply-nested path" test now uses 2 segments (not 3) so the joined form stays under the new total cap; the 3-segment scenario is covered by the new fallback test instead. 55 unit + 66 integration = 121/121 pass. * 📝 docs: Correct sanitizeArtifactPath JSDoc to match actual schema index Two doc-only fixes from the latest comprehensive review (both NIT): 1. **Index field list was wrong**. JSDoc claimed the compound unique index was `{ file_id, filename, conversationId, context }`. The actual index in `packages/data-schemas/src/schema/file.ts:92-95` is `{ filename, conversationId, context, tenantId }` with a partial filter for `context: FileContext.execute_code`. The cap rationale (Mongo 4.0 indexed-key limit) is correct and unchanged; just the field list was wrong. Added the schema file path so future readers can find the source of truth. 2. **Trade-off acknowledgement**. The reviewer noted that the leaf-only fallback loses directory structure, which means the model's `cat /mnt/data/<deep>/<path>/file.txt` would 404 on the pathological-depth case — partially re-introducing the original flat-name bug for >512-char paths. This is intentional (DB write failure is strictly worse than losing structure), but the trade-off wasn't called out explicitly in the JSDoc. Added a paragraph acknowledging it and noting that the cap is monotonically better than the pre-PR behavior, where ALL artifacts were treated this way regardless of depth. No code or test changes — pure JSDoc correction. Tests still 55/0. * 🛡️ fix: Disambiguate sanitized artifact names to keep claimCodeFile keys unique (codex P2) `sanitizeArtifactPath` is not injective — multiple raw inputs can collapse onto the same regex-and-normalize output. Codex's example: `reports 2026/out.csv` and `reports_2026/out.csv` both sanitize to `reports_2026/out.csv`. `claimCodeFile` is keyed on the schema's compound unique `(filename, conversationId, context, tenantId)` index, so the later upload silently matches the earlier record and overwrites the first artifact's bytes via the reused `file_id` — a single conversation can drop files when both names are valid in the sandbox. This collision space isn't strictly new — pre-PR `sanitizeFilename` (basename-only) had the same property — but the path-preserving form gives us enough information to fix it for the first time. **Fix.** When character-level sanitization changed something (regex replacement, path normalization, dotfile prefix, empty-segment collapse), embed a deterministic SHA-256 prefix of the **raw input** in the leaf segment via the new `embedDisambiguatorInLeaf` helper. Same raw input → same safe form (idempotent for re-uploads); different raw inputs that would have collided → different safe forms. **Why "character-level"** specifically: - The disambiguator fires when `preCapJoined !== inputName` (post-regex + dotfile + empty-segment, BUT pre-truncation). - Truncation alone is already disambiguated by `truncateLeafSegment`'s own seg-hash; firing the input-hash branch on truncation would just stack a second hash for no collision-avoidance benefit and clutter human-readable filenames. **Three known collision shapes covered:** 1. `out 1.csv` vs `out_1.csv` (and `out@1.csv` vs `out#1.csv`, etc.) 2. `dir//file.txt` vs `dir/file.txt` (empty-segment collapse) 3. `.x` vs `_.x` (dotfile-prefix step) **Disambiguator + truncation interaction:** for very long mutated leaves, `truncateLeafSegment` caps at 255 first, then `embedDisambiguatorInLeaf` re-trims to insert the input hash. The seg-hash from the first pass is replaced by the input-hash from the second pass — that's intentional (input-hash is the load-bearing collision-avoidance suffix; seg-hash was only ever decorative once the input-hash exists). Final clamp ensures the result never exceeds `ARTIFACT_PATH_SEGMENT_MAX` regardless of input. **Disambiguator + total-cap fallback:** when joined > 512, we fall back to the leaf-only form. The leaf has already had the disambiguator embedded, so collision avoidance survives the pathological-depth case. **`embedDisambiguatorInLeaf`** uses `dot <= 1` to detect "no real extension" (covers extensionless names AND dotfile-prefixed leaves like `_.hidden` — without this, `_.hidden` would split as stem `_` + ext `.hidden` and produce the awkward `_-<hash>.hidden`). **Updated 5 existing tests** that asserted the old collision-prone outputs — they now verify the disambiguator-included form. The character-level-only firing rule was load-bearing here: tests for "clean inputs (no mutation)" and "long inputs (truncation only)" still pass without any disambiguator clutter. **+7 regression tests** in a new `collision avoidance (Codex review P2)` describe block: 1. Different raw inputs sanitizing to the same form get distinct safes 2. Whitespace-vs-underscore in directory segment 3. Dotfile-prefix collision 4. Idempotency: same raw → same safe across calls 5. Clean inputs skip the disambiguator (cosmetic guarantee) 6. Disambiguator survives leaf truncation (long mutated leaf) 7. Disambiguator survives total-cap fallback (pathological depth) 62 unit + 66 integration = 128/128 pass.
This commit is contained in:
Danny Avila
GitHub
parent
7070eb76aa
commit
c9dee962e7
5 changed files with 805 additions and 26 deletions
|
|
@ -8,7 +8,8 @@ jest.mock('@librechat/agents', () => ({
|
|||
getCodeBaseURL: jest.fn(() => 'http://localhost:8000'),
|
||||
}));
|
||||
|
||||
const mockSanitizeFilename = jest.fn();
|
||||
const mockSanitizeArtifactPath = jest.fn();
|
||||
const mockFlattenArtifactPath = jest.fn((name) => name.replace(/\//g, '__'));
|
||||
|
||||
const mockAxios = jest.fn().mockResolvedValue({
|
||||
data: Buffer.from('file-content'),
|
||||
|
|
@ -21,7 +22,8 @@ jest.mock('@librechat/api', () => {
|
|||
return {
|
||||
logAxiosError: jest.fn(),
|
||||
getBasePath: jest.fn(() => ''),
|
||||
sanitizeFilename: mockSanitizeFilename,
|
||||
sanitizeArtifactPath: mockSanitizeArtifactPath,
|
||||
flattenArtifactPath: mockFlattenArtifactPath,
|
||||
createAxiosInstance: jest.fn(() => mockAxios),
|
||||
classifyCodeArtifact: jest.fn(() => 'other'),
|
||||
extractCodeArtifactText: jest.fn(async () => null),
|
||||
|
|
@ -92,23 +94,25 @@ describe('processCodeOutput path traversal protection', () => {
|
|||
jest.clearAllMocks();
|
||||
});
|
||||
|
||||
test('sanitizeFilename is called with the raw artifact name', async () => {
|
||||
mockSanitizeFilename.mockReturnValueOnce('output.csv');
|
||||
test('sanitizeArtifactPath is called with the raw artifact name', async () => {
|
||||
mockSanitizeArtifactPath.mockReturnValueOnce('output.csv');
|
||||
await processCodeOutput({ ...baseParams, name: 'output.csv' });
|
||||
expect(mockSanitizeFilename).toHaveBeenCalledWith('output.csv');
|
||||
expect(mockSanitizeArtifactPath).toHaveBeenCalledWith('output.csv');
|
||||
});
|
||||
|
||||
test('sanitized name is used in saveBuffer fileName', async () => {
|
||||
mockSanitizeFilename.mockReturnValueOnce('sanitized-name.txt');
|
||||
test('sanitized name is used in saveBuffer fileName (and flattened to a single component)', async () => {
|
||||
mockSanitizeArtifactPath.mockReturnValueOnce('sanitized-name.txt');
|
||||
await processCodeOutput({ ...baseParams, name: '../../../tmp/poc.txt' });
|
||||
|
||||
expect(mockSanitizeFilename).toHaveBeenCalledWith('../../../tmp/poc.txt');
|
||||
expect(mockSanitizeArtifactPath).toHaveBeenCalledWith('../../../tmp/poc.txt');
|
||||
const call = mockSaveBuffer.mock.calls[0][0];
|
||||
/* `flattenArtifactPath` is identity for already-flat names; the assert
|
||||
* is against the storage-key composition (`<file_id>__<flat>`). */
|
||||
expect(call.fileName).toBe('mock-uuid__sanitized-name.txt');
|
||||
});
|
||||
|
||||
test('sanitized name is stored as filename in the file record', async () => {
|
||||
mockSanitizeFilename.mockReturnValueOnce('safe-output.csv');
|
||||
mockSanitizeArtifactPath.mockReturnValueOnce('safe-output.csv');
|
||||
await processCodeOutput({ ...baseParams, name: 'unsafe/../../output.csv' });
|
||||
|
||||
const fileArg = createFile.mock.calls[0][0];
|
||||
|
|
@ -122,10 +126,10 @@ describe('processCodeOutput path traversal protection', () => {
|
|||
bytes: 100,
|
||||
});
|
||||
|
||||
mockSanitizeFilename.mockReturnValueOnce('safe-chart.png');
|
||||
mockSanitizeArtifactPath.mockReturnValueOnce('safe-chart.png');
|
||||
await processCodeOutput({ ...baseParams, name: '../../../chart.png' });
|
||||
|
||||
expect(mockSanitizeFilename).toHaveBeenCalledWith('../../../chart.png');
|
||||
expect(mockSanitizeArtifactPath).toHaveBeenCalledWith('../../../chart.png');
|
||||
const fileArg = createFile.mock.calls[0][0];
|
||||
expect(fileArg.filename).toBe('safe-chart.png');
|
||||
});
|
||||
|
|
|
|||
|
|
@ -5,7 +5,8 @@ const { getCodeBaseURL } = require('@librechat/agents');
|
|||
const {
|
||||
getBasePath,
|
||||
logAxiosError,
|
||||
sanitizeFilename,
|
||||
sanitizeArtifactPath,
|
||||
flattenArtifactPath,
|
||||
createAxiosInstance,
|
||||
classifyCodeArtifact,
|
||||
codeServerHttpAgent,
|
||||
|
|
@ -134,14 +135,32 @@ const processCodeOutput = async ({
|
|||
|
||||
const fileIdentifier = `${session_id}/${id}`;
|
||||
|
||||
/* `safeName` keeps the directory structure (`a/b/file.txt` -> `a/b/file.txt`)
|
||||
* so the next prime() can place the file at the same nested path in the
|
||||
* sandbox; flattening would re-create the bug where every nested artifact
|
||||
* collapsed into the root and read_file calls 404'd. The flat-form
|
||||
* storage key is composed below once `file_id` is known so we can cap
|
||||
* the total length at filesystem NAME_MAX. */
|
||||
const safeName = sanitizeArtifactPath(name);
|
||||
if (safeName !== name) {
|
||||
logger.warn(
|
||||
`[processCodeOutput] Filename sanitized: "${name}" -> "${safeName}" | conv=${conversationId}`,
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Atomically claim a file_id for this (filename, conversationId, context) tuple.
|
||||
* Uses $setOnInsert so concurrent calls for the same filename converge on
|
||||
* a single record instead of creating duplicates (TOCTOU race fix).
|
||||
*
|
||||
* Claim by `safeName` (not raw `name`) so the claim and the eventual
|
||||
* `createFile` agree on the filename column — otherwise weird inputs
|
||||
* (e.g. `"proj name/file@v1.txt"`) would claim under the raw name and
|
||||
* then write under the sanitized one, leaving the claim row orphaned.
|
||||
*/
|
||||
const newFileId = v4();
|
||||
const claimed = await claimCodeFile({
|
||||
filename: name,
|
||||
filename: safeName,
|
||||
conversationId,
|
||||
file_id: newFileId,
|
||||
user: req.user.id,
|
||||
|
|
@ -151,14 +170,7 @@ const processCodeOutput = async ({
|
|||
|
||||
if (isUpdate) {
|
||||
logger.debug(
|
||||
`[processCodeOutput] Updating existing file "${name}" (${file_id}) instead of creating duplicate`,
|
||||
);
|
||||
}
|
||||
|
||||
const safeName = sanitizeFilename(name);
|
||||
if (safeName !== name) {
|
||||
logger.warn(
|
||||
`[processCodeOutput] Filename sanitized: "${name}" -> "${safeName}" | conv=${conversationId}`,
|
||||
`[processCodeOutput] Updating existing file "${safeName}" (${file_id}) instead of creating duplicate`,
|
||||
);
|
||||
}
|
||||
|
||||
|
|
@ -226,7 +238,19 @@ const processCodeOutput = async ({
|
|||
);
|
||||
}
|
||||
|
||||
const fileName = `${file_id}__${safeName}`;
|
||||
/* Compose the storage key here, after `file_id` is known, so the
|
||||
* `flattenArtifactPath` cap budget can be calculated against the
|
||||
* actual prefix length. The full key has to fit in one filesystem
|
||||
* path component (NAME_MAX = 255 on most filesystems); without this
|
||||
* cap, deeply-nested artifact paths whose individual segments were
|
||||
* within bounds can still produce a flat form that overflows once
|
||||
* `${file_id}__` is prepended, causing `ENAMETOOLONG` inside
|
||||
* saveBuffer and falling back to a download URL. The 255 figure is
|
||||
* the conservative cross-platform NAME_MAX (Linux ext4, NTFS, APFS).
|
||||
*/
|
||||
const NAME_MAX = 255;
|
||||
const flatName = flattenArtifactPath(safeName, NAME_MAX - file_id.length - 2);
|
||||
const fileName = `${file_id}__${flatName}`;
|
||||
const filepath = await saveBuffer({
|
||||
userId: req.user.id,
|
||||
buffer,
|
||||
|
|
@ -234,8 +258,19 @@ const processCodeOutput = async ({
|
|||
basePath: 'uploads',
|
||||
});
|
||||
|
||||
const category = classifyCodeArtifact(safeName, mimeType);
|
||||
const text = await extractCodeArtifactText(buffer, safeName, mimeType, category);
|
||||
/* `classifyCodeArtifact` and `extractCodeArtifactText` make
|
||||
* extension/bare-name decisions on the input string. With the
|
||||
* path-preserving sanitizer they can now receive a nested path like
|
||||
* `reports.v1/Makefile`, which the classifier's `extensionOf` reads
|
||||
* as `v1/Makefile` (the slice after the dot in the directory name)
|
||||
* and the bare-name branch rejects because it sees a `.` anywhere in
|
||||
* the string. Result: extensionless artifacts under dotted folders
|
||||
* (Makefile, Dockerfile, etc.) get misclassified as `other` and
|
||||
* skip text extraction. Pass the basename so classification matches
|
||||
* what it would have gotten with the old flat-name flow. */
|
||||
const leafName = path.basename(safeName);
|
||||
const category = classifyCodeArtifact(leafName, mimeType);
|
||||
const text = await extractCodeArtifactText(buffer, leafName, mimeType, category);
|
||||
|
||||
const file = {
|
||||
file_id,
|
||||
|
|
|
|||
|
|
@ -49,7 +49,8 @@ jest.mock('@librechat/api', () => {
|
|||
return {
|
||||
logAxiosError: jest.fn(),
|
||||
getBasePath: jest.fn(() => ''),
|
||||
sanitizeFilename: jest.fn((name) => name),
|
||||
sanitizeArtifactPath: jest.fn((name) => name),
|
||||
flattenArtifactPath: jest.fn((name) => name.replace(/\//g, '__')),
|
||||
createAxiosInstance: jest.fn(() => mockAxios),
|
||||
/**
|
||||
* Arrow-function indirection (vs. a direct `jest.fn()` reference) so
|
||||
|
|
@ -274,6 +275,94 @@ describe('Code Process', () => {
|
|||
expect(result.bytes).toBe(100);
|
||||
});
|
||||
|
||||
it('preserves nested directory paths in the DB record while flattening the storage key', async () => {
|
||||
/* Regression test for the silent-data-loss path: when codeapi reports a
|
||||
* file with a nested name like "test_folder/test_file.txt", LibreChat
|
||||
* used to feed it through `sanitizeFilename` (basename-only), which
|
||||
* persisted "test_file.txt" to the DB and made the file un-locatable on
|
||||
* the next prime() (cat /mnt/data/test_folder/test_file.txt would
|
||||
* 404). The fix: keep the path on the DB record (so primeFiles can
|
||||
* place it back at the same nested location), but flatten it for the
|
||||
* storage key (saveBuffer strategies key by single component). */
|
||||
const smallBuffer = Buffer.alloc(100);
|
||||
mockAxios.mockResolvedValue({ data: smallBuffer });
|
||||
const mockSaveBuffer = jest.fn().mockResolvedValue('/uploads/saved.txt');
|
||||
getStrategyFunctions.mockReturnValue({ saveBuffer: mockSaveBuffer });
|
||||
|
||||
const result = await processCodeOutput({
|
||||
...baseParams,
|
||||
name: 'test_folder/test_file.txt',
|
||||
});
|
||||
|
||||
// Storage key flattens `/` to `__` so on-disk strategies don't
|
||||
// accidentally create real subdirectories under uploads/.
|
||||
expect(mockSaveBuffer).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
fileName: 'mock-uuid-1234__test_folder__test_file.txt',
|
||||
}),
|
||||
);
|
||||
// DB row keeps the nested path verbatim — that's what primeFiles
|
||||
// ships back to the sandbox on the next turn.
|
||||
expect(result.filename).toBe('test_folder/test_file.txt');
|
||||
// Claim is also keyed by the path-preserving name so the
|
||||
// (filename, conversationId) compound key stays consistent.
|
||||
expect(mockClaimCodeFile).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ filename: 'test_folder/test_file.txt' }),
|
||||
);
|
||||
});
|
||||
|
||||
it('passes a NAME_MAX-aware budget to flattenArtifactPath when composing the storage key', async () => {
|
||||
/* Codex review P1: per-segment caps on the path-preserving form
|
||||
* aren't enough — once the segments are joined with `__` for the
|
||||
* storage key, deeply-nested or moderately long paths can still
|
||||
* exceed filesystem NAME_MAX (255) and cause `ENAMETOOLONG` in
|
||||
* saveBuffer. processCodeOutput must pass a file_id-aware budget
|
||||
* to flattenArtifactPath so the cap holds end-to-end. The unit
|
||||
* tests in `packages/api/src/utils/files.spec.ts` cover the
|
||||
* truncation logic itself; this test covers the integration. */
|
||||
const smallBuffer = Buffer.alloc(100);
|
||||
mockAxios.mockResolvedValue({ data: smallBuffer });
|
||||
const mockSaveBuffer = jest.fn().mockResolvedValue('/uploads/saved.bin');
|
||||
getStrategyFunctions.mockReturnValue({ saveBuffer: mockSaveBuffer });
|
||||
|
||||
const flattenSpy = require('@librechat/api').flattenArtifactPath;
|
||||
flattenSpy.mockClear();
|
||||
|
||||
await processCodeOutput({ ...baseParams, name: 'a/b/c.csv' });
|
||||
|
||||
// The handler should call flattenArtifactPath with both the
|
||||
// safeName AND a budget = NAME_MAX (255) minus the prefix
|
||||
// (`${file_id}__`). file_id mock is `mock-uuid-1234` (14 chars),
|
||||
// so the budget should be 255 - 14 - 2 = 239.
|
||||
expect(flattenSpy).toHaveBeenCalledWith(expect.any(String), 239);
|
||||
});
|
||||
|
||||
it('passes the basename (not the full nested path) to classifyCodeArtifact and extractCodeArtifactText', async () => {
|
||||
/* Codex review P2: with the path-preserving sanitizer, `safeName`
|
||||
* can be a nested string like `reports.v1/Makefile`. The
|
||||
* classifier reads `extensionOf` against the full string, which
|
||||
* sees `.v1/Makefile` after the dotted-dir's first dot and
|
||||
* misclassifies the file as `other` (so text extraction is
|
||||
* skipped). Pass `path.basename(safeName)` instead. */
|
||||
const smallBuffer = Buffer.alloc(100);
|
||||
mockAxios.mockResolvedValue({ data: smallBuffer });
|
||||
const mockSaveBuffer = jest.fn().mockResolvedValue('/uploads/saved.txt');
|
||||
getStrategyFunctions.mockReturnValue({ saveBuffer: mockSaveBuffer });
|
||||
|
||||
await processCodeOutput({
|
||||
...baseParams,
|
||||
name: 'reports.v1/Makefile',
|
||||
});
|
||||
|
||||
expect(mockClassifyCodeArtifact).toHaveBeenCalledWith('Makefile', expect.any(String));
|
||||
expect(mockExtractCodeArtifactText).toHaveBeenCalledWith(
|
||||
expect.any(Buffer),
|
||||
'Makefile',
|
||||
expect.any(String),
|
||||
expect.any(String),
|
||||
);
|
||||
});
|
||||
|
||||
it('should detect MIME type from buffer', async () => {
|
||||
const smallBuffer = Buffer.alloc(100);
|
||||
mockAxios.mockResolvedValue({ data: smallBuffer });
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue