Repository navigation
feat: LibreChat v0.8.7 compatibility - hardened (supersedes #73) - #75
Merged
Merged
Conversation
LibreChat skills upload files with relative paths like
"<skillName>/SKILL.md", but every layer flattened them to a basename,
breaking skill directory structure, relative references and causing
cross-skill basename collisions.
Fix flattening across all four layers:
- output.py: add `_is_safe_relative_path` + `sanitize_filepath`
(per-segment sanitize, preserves subdirs; falls back to basename for
unsafe/traversal/absolute/dot inputs). `sanitize_filename` (basename
flatten) retained for output filenames.
- api/files.py: `/upload` and `/upload/batch` use `sanitize_filepath`.
- docker/runner/files.go: Go's multipart parser strips directories per
RFC 7578, so HandleUpload now reads the relative path out-of-band from
a `path` form field (paired by index), MkdirAll's the parent, and
validates via validatePath (authoritative traversal guard; dotfile
rejection preserved). HandleList rewritten with WalkDir (recursive,
relative slash paths).
- manager.py / job_executor.py: send `data={"path": filename}` alongside
the file part so the runner can reconstruct the nested path.
- runner.py: mounted-filename set includes full relative path + basename;
generated-file filtering matches by relative-path-under-/mnt/data or
basename.
Tests: nested round-trip, traversal/absolute/dot fallback, per-segment
sanitize, recursive list, and a LibreChat nested-path upload contract.
Note: the Go runner is baked into all 12 language images, so this fix
requires rebuilding every language image.
Skill/agent bundles are uploaded with read_only=true but the flag was a placeholder: read-only inputs the sandbox touched surfaced as ghost download artifacts, and there was no way to distinguish inherited inputs from generated outputs. - models/files.py: FileInfo carries read_only (default False). - file.py / interfaces.py: store_uploaded_file persists read_only on file metadata; get_file_info reads it back (legacy files default False). - api/files.py: batch /upload threads its read_only form field to storage. - orchestrator.py: mounted file dicts carry read_only (explicit + auto-mount + cross-session consolidation); _build_response echoes read-only inputs in a new ExecResponse.inherited_files list, each FileRef marked inherited=true. - runner.py: _filter_generated_files excludes read-only inputs from the generated files list even when modified (suppresses ghost downloads), via a new _get_readonly_filenames expansion. inherited_files is deliberately separate from files[]: @librechat/agents 3.2.35 maps every files[] entry into a user download artifact and ignores the inherited flag, so emitting inherited entries there would re-create the ghosts. The separate field is forward-compatible for consumers that read it. Tests: read-only suppression in the runner, inherited_files assembly in the orchestrator, batch read_only persistence + default, updated consolidation contract.
Thread kind/id/version/read_only through the single-file /upload handler so it reaches identity parity with /upload/batch. read_only is persisted on the stored file; kind and id land on the session metadata. Existing direct-call unit tests updated to pass the new Form params explicitly, matching the batch test convention.
LibreChat's deleteFile issues DELETE /sessions/{session}/objects/{file}
first and only falls back to DELETE /files/{session}/{file} on failure.
Serving the objects path directly (delegating to delete_file) avoids the
extra 404 round-trip. kind/id/version query params are accepted for
parity with buildCodeEnvDownloadQuery.
* Build the runner image synchronously before other language images. * Prevents build failures due to missing runner image during subsequent builds. * Adds error handling for missing Dockerfile and build failures.
…ract) LibreChat 0.8.7 pins @librechat/agents ^3.2.46, whose CodeSessionFileSummary filters files[] by `file.inherited !== true`, and the host (callbacks.js / tools.js) already skips `if (file.inherited)` when creating user downloads and re-downloads. The canonical wire contract is therefore to echo read-only inputs INSIDE files[] flagged inherited:true — not in a separate field. Replace the separate ExecResponse.inherited_files field with inherited:true entries appended to files[] (orchestrator._build_response). Matches the SaaS code-interpreter reference and what final LibreChat + agents 3.2.46 consume. The runner still excludes read-only inputs from the generated list, so each read-only input appears exactly once (as an inherited entry).
… mode)
@librechat/agents 3.2.46 ships ProgrammaticToolCalling, which POSTs to
${baseURL}/exec/programmatic so the model can write Python that calls
LibreChat tools as async functions. KubeCodeRun execution pods have no
network egress (NetworkPolicy egress: []), so the live-callback bridge is
impossible here; this implements the stateless replay model used by the
reference code-interpreter service instead.
Each round the sandbox re-runs the user code with an injected history file
(_ptc_history.json: call_id -> prior result). A cached call returns
immediately; the first un-cached call is printed inside an execution-scoped
stdout sentinel and the process exits. The API relays that call to LibreChat
and continues via continuation_token + tool_results until the code completes.
- models/programmatic.py: request/response wire models (language/lang alias,
tool_calls {id,name,input}, tool_results {call_id,result,is_error}).
- services/programmatic/: constants (sentinels, reserved-filename guard),
harness (replay preamble + tool stubs + async wrap), sentinel parser,
Redis-backed continuation state, and the replay orchestration service.
- api/programmatic.py: POST /exec/programmatic (auth + user_id like /exec).
- orchestrator: internal extra_files (inject the history file) and
capture_state_override (PTC runs are stateless).
Bash PTC is rejected with a clear 400 for now (Python only). Forged/un-issued
tool_result call_ids are dropped; unregistered tool calls and empty pending
blocks return status=error to avoid tight retry loops.
… validation) - sentinel: match the marker line exactly (tolerating only a trailing CR for CRLF-captured stdout) instead of strip(), so user output that merely contains the marker padded by whitespace can't be mistaken for a sentinel or shift the byte-accurate stdout reconstruction. - service: validate tool names up front — reject empty/whitespace names and names with no identifier-safe characters (which would emit `async def ()` and surface as a confusing "completed" syntax error), and reject two distinct names that normalize to the same Python identifier. Returns a clear 400. - harness: expose normalize_python_function_name (was private) for reuse.
…emas and user code
Three harness-generation bugs, each reproduced before fixing:
- Parameter names from tool JSON schemas were interpolated raw into the
stub signature and input dict: a name like "user-name" generated
`async def search(user-name: str)` — a syntax error surfacing as a
confusing 'completed' response with a traceback. Property names are now
normalized with normalize_python_function_name (dispatch still sends the
original name, JSON-escaped, as the input key), and requests whose
parameter names have no identifier-safe characters or collide after
normalization get a clear 400.
- Tool descriptions ending in '"' (or containing quote runs) terminated
the generated docstring literal early. Every quote is now escaped, not
just literal triple-quote sequences.
- wrap_user_code_in_async re-indented every user line by 4 spaces, which
corrupts the *content* of multi-line string literals ('line1\nline2'
became 'line1\n line2'). User code is now embedded verbatim as a
string literal and compiled at runtime with PyCF_ALLOW_TOP_LEVEL_AWAIT,
preserving bytes exactly, keeping module-level scoping (global,
top-level def/class) and traceback line numbers, while still supporting
top-level await.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ession files _mount_files appended extra_files only when no mounted file shared the name, so a session file named _ptc_history.json (uploadable via /upload — the reserved-name check in ProgrammaticService only covers request.files) silently shadowed the injected replay history. That hands replay control to session content: forged tool results, or a stale history that re-emits the same tool_call_required forever and burns the client's 20-round cap. Extra files now replace same-named mounted entries. The history file is runtime infrastructure; nothing a session contains may impersonate it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…leted The PTC sentinel is the last thing the harness prints, and pod stdout is capped at 64KB by OutputProcessor.sanitize_output — so any run that prints enough before its first tool call gets its sentinel cut off and was previously misreported as status=completed with truncated stdout, silently dropping the pending tool call. When no sentinel is found and stdout carries the truncation notice, the service now returns a structured status=error telling the model to print less before calling tools, and drops the continuation state. The notice string is promoted to OutputProcessor.TRUNCATION_NOTICE so the two sites cannot drift. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…client behavior Verified against @librechat/agents 3.2.46 source (ProgrammaticToolCalling + Bash variant) and LibreChat v0.8.7: - Bind continuation tokens to the identity that started the execution. A token presented under a different user_id now gets the same 'not found or expired' error as an unknown token, instead of resuming someone else's replay state under the shared API key. - Mid-loop failures (unknown/expired token, replay-call cap) now return HTTP 200 with status='error'. The agents client's makeRequest throws on any non-2xx with an unstructured 'CodeAPI request failed' message and never reads the body's error field; only the 200 + status:'error' shape takes its structured path. Up-front request validation (bad tool names, missing code) keeps returning 400 - that path is developer-facing. - Treat any body carrying continuation_token as a continuation. Requiring tool_results too meant a token-only body was misread as an initial request and rejected with a misleading 'Missing required field: code'. - Raise the continuation-state sliding TTL from 600s to 3600s: the client caps the loop at 20 round trips but has NO timeout between rounds - each gap lasts as long as the host takes to run the requested tool. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… deterministic Nested upload paths reopened a hole the old basename-only flow had closed: only the final segment was checked for a leading dot, so '.git/config' or 'skill/.ssh/authorized_keys' could materialize hidden directories inside /mnt/data. Every segment of the relative path is now checked. Also iterate multipart field names in sorted order when pairing files with the out-of-band 'path' values: Go map iteration is randomized, so a request using more than one field name could nondeterministically associate paths with the wrong files. The control plane sends one field per request today, but correctness must not depend on that. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…process Round 1 with empty history must emit the scoped sentinel for the first tool call and preserve user prints; round 2 with the cached result must run to completion; a cached is_error result must raise inside user code. This runs the real generated Python (via PTC_HISTORY_PATH), so harness, sentinel parser and replay semantics are proven together rather than assumed from string assertions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🎉 This PR is included in version 3.7.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Background: this builds directly on @gafda's #73 - all 8 of their commits are kept as-is (skills path preservation, read_only/inherited contract, upload/delete parity,
/exec/programmatic), with 6 commits of fixes on top for defects I found while verifying the whole thing against the real wire contract. I pulled LibreChat v0.8.7 (tag9e74cc0) and@librechat/agents@3.2.46from npm and checked every claim in the PR against the actual client source rather than trusting the description - the file/upload/delete/inherited contract claims all held up (nice work), but the PTC implementation had real bugs, each of which I reproduced by execution before fixing:user-namegeneratedasync def search(user-name: str)- a syntax error that surfaced as a confusing "completed" response with a traceback. param names are now normalized like tool names (dispatch still sends the original name), with clear 400s for names that can't normalize or that collide. tool descriptions ending in"broke the generated docstring the same way"""line1\nline2"""becameline1\n line2). user code is now embedded verbatim as a literal and compiled withPyCF_ALLOW_TOP_LEVEL_AWAIT- preserves bytes exactly, keepsglobal/top-level-def scoping and traceback line numbers, still supports top-level await_ptc_history.json(uploadable via/upload- the reserved-name check only coveredrequest.files) silently replaced the injected replay history, handing tool-result replay to session content. injected extra files now win over same-named mounted filescompleted, silently dropping the pending call. this now returns a structured error telling the model to print lessstatus:"error"becausemakeRequestin agents 3.2.46 throws an unstructured error on any non-2xx and never reads the body'serrorfield; a token-only body is treated as a continuation instead of 400ing with "Missing required field: code"; raised the sliding state TTL 600s -> 3600s since the client runs up to 20 round trips with no timeout between them - each gap lasts as long as the host's tool execution.git/configorskill/.ssh/authorized_keyscould land hidden directories in /mnt/data. every segment is checked now, and multipart field iteration is sorted so path-to-file pairing can't depend on Go's randomized map orderNotably, one contract nuance from the verification:
partial_stdout/partial_stderrdon't exist anywhere in the agents client - they're kept since extra fields are harmless, but nothing reads them.Testing
Full suite:
1617 passedpython unit tests (76 PTC-specific),go test ./...ok for the runner, ruff + ty clean. The newTestHarnessEndToEndruns the generated harness in a real subprocess through a full replay round-trip - round 1 emits the scoped sentinel and preserves user prints, round 2 completes from cached history, and a cachedis_errorresult raises inside user code:Both pre-fix bugs reproduce on #73's head if you want to confirm:
What I haven't done is an end-to-end run against a live LibreChat 0.8.7 - the wire contract is verified against the client source, not a running instance.
Review Strategy
The 6 fix commits are atomic and reviewable independently - commits 1-2 (
aba423d,d7cc129) are the ones with correctness stakes; the rest are contract fidelity and hardening. Gap D (deterministic sessionKey bucketing) stays deferred for the reasons #73 gave - I agree with that call.Closes #73
🤖 Generated with Claude Code