From c8fe1cd3907882117357c34f9aa2d649a35e31a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gil=20Assun=C3=A7=C3=A3o?= Date: Wed, 24 Jun 2026 11:34:25 +0100 Subject: [PATCH 1/8] feat(files): preserve nested skill file paths end-to-end LibreChat skills upload files with relative paths like "/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. --- docker/runner/files.go | 76 +++++++++++--- docker/runner/files_test.go | 129 ++++++++++++++++++++++++ src/api/files.py | 11 +- src/services/execution/output.py | 47 +++++++++ src/services/execution/runner.py | 39 +++++-- src/services/kubernetes/job_executor.py | 4 + src/services/kubernetes/manager.py | 5 + tests/unit/test_execution_runner.py | 33 ++++++ tests/unit/test_librechat_contract.py | 29 ++++++ tests/unit/test_output_processor.py | 64 ++++++++++++ 10 files changed, 410 insertions(+), 27 deletions(-) diff --git a/docker/runner/files.go b/docker/runner/files.go index a1d66b8..0e2e4b8 100644 --- a/docker/runner/files.go +++ b/docker/runner/files.go @@ -46,18 +46,40 @@ func (h *FileHandler) validatePath(reqPath string) (string, error) { } // HandleUpload processes POST /files (multipart file upload). +// +// RFC 7578 (and Go's net/http multipart parser) strip directory components +// from the part filename, so nested skill bundles cannot ride on the filename +// alone. The control plane therefore sends the intended relative path in an +// out-of-band ``path`` form field (one value per file, in order). When absent, +// the basenamed filename is used, preserving legacy single-file behavior. func (h *FileHandler) HandleUpload(w http.ResponseWriter, r *http.Request) { if err := r.ParseMultipartForm(32 << 20); err != nil { writeJSON(w, http.StatusBadRequest, map[string]string{"detail": "Invalid multipart form"}) return } - var uploaded []FileInfo + absWD, _ := filepath.Abs(h.workingDir) + explicitPaths := r.MultipartForm.Value["path"] + var uploaded []FileInfo + idx := 0 for _, fileHeaders := range r.MultipartForm.File { for _, fh := range fileHeaders { - safeName := filepath.Base(fh.Filename) - if safeName == "" || safeName[0] == '.' { + // Prefer the explicit relative path (preserves nested layout); + // fall back to the parser-basenamed filename. + rawName := fh.Filename + if idx < len(explicitPaths) && explicitPaths[idx] != "" { + rawName = explicitPaths[idx] + } + idx++ + + destPath, err := h.validatePath(filepath.ToSlash(rawName)) + if err != nil { + continue + } + base := filepath.Base(destPath) + if base == "" || base == "." || base == ".." || base[0] == '.' || destPath == absWD { + // Skip empty, traversal, hidden, or working-dir targets. continue } @@ -66,7 +88,11 @@ func (h *FileHandler) HandleUpload(w http.ResponseWriter, r *http.Request) { continue } - destPath := filepath.Join(h.workingDir, safeName) + if err := os.MkdirAll(filepath.Dir(destPath), 0o755); err != nil { + src.Close() + continue + } + dst, err := os.Create(destPath) if err != nil { src.Close() @@ -84,8 +110,12 @@ func (h *FileHandler) HandleUpload(w http.ResponseWriter, r *http.Request) { modTime = fi.ModTime().Unix() } + rel, err := filepath.Rel(absWD, destPath) + if err != nil { + rel = base + } uploaded = append(uploaded, FileInfo{ - Name: safeName, + Name: filepath.ToSlash(rel), Path: destPath, Size: n, ModTime: modTime, @@ -97,29 +127,43 @@ func (h *FileHandler) HandleUpload(w http.ResponseWriter, r *http.Request) { } // HandleList processes GET /files (list working directory). +// +// The listing is recursive and returns each regular file's path relative to +// the working directory (e.g. "skillName/SKILL.md"). This keeps nested skill +// bundles and nested generated files visible to the control plane, which +// matches them against mounted files by relative path. func (h *FileHandler) HandleList(w http.ResponseWriter, r *http.Request) { - entries, err := os.ReadDir(h.workingDir) + absWD, err := filepath.Abs(h.workingDir) if err != nil { http.Error(w, `{"detail":"Working directory not found"}`, http.StatusNotFound) return } - files := make([]FileInfo, 0, len(entries)) - for _, e := range entries { - info, err := e.Info() + files := make([]FileInfo, 0) + walkErr := filepath.WalkDir(h.workingDir, func(p string, d os.DirEntry, walkErr error) error { + if walkErr != nil || d.IsDir() { + return nil + } + info, err := d.Info() if err != nil { - continue + return nil } - size := info.Size() - if e.IsDir() { - size = 0 + rel, err := filepath.Rel(absWD, p) + if err != nil { + return nil } + relSlash := filepath.ToSlash(rel) files = append(files, FileInfo{ - Name: e.Name(), - Path: e.Name(), - Size: size, + Name: relSlash, + Path: relSlash, + Size: info.Size(), ModTime: info.ModTime().Unix(), }) + return nil + }) + if walkErr != nil { + http.Error(w, `{"detail":"Working directory not found"}`, http.StatusNotFound) + return } writeJSON(w, http.StatusOK, map[string]any{"files": files}) diff --git a/docker/runner/files_test.go b/docker/runner/files_test.go index 59d938c..9c0bc60 100644 --- a/docker/runner/files_test.go +++ b/docker/runner/files_test.go @@ -1,6 +1,11 @@ package main import ( + "bytes" + "encoding/json" + "mime/multipart" + "net/http" + "net/http/httptest" "os" "path/filepath" "testing" @@ -107,3 +112,127 @@ func TestHandleListIncludesModTime(t *testing.T) { _ = h // keep linter happy } + +// uploadMultipart builds a POST /files multipart request. The file part is +// sent with `basename` as its filename (Go's parser strips directories anyway) +// and the nested layout is carried out-of-band in the `path` form field, which +// mirrors how the control plane transports skill bundles like +// "skillName/SKILL.md". +func uploadMultipart(t *testing.T, relPath string, content []byte) *http.Request { + t.Helper() + var body bytes.Buffer + mw := multipart.NewWriter(&body) + part, err := mw.CreateFormFile("files", filepath.Base(relPath)) + if err != nil { + t.Fatalf("CreateFormFile: %v", err) + } + if _, err := part.Write(content); err != nil { + t.Fatalf("write part: %v", err) + } + if err := mw.WriteField("path", relPath); err != nil { + t.Fatalf("write path field: %v", err) + } + if err := mw.Close(); err != nil { + t.Fatalf("close writer: %v", err) + } + req := httptest.NewRequest(http.MethodPost, "/files", &body) + req.Header.Set("Content-Type", mw.FormDataContentType()) + return req +} + +func TestHandleUploadPreservesNestedPath(t *testing.T) { + dir := t.TempDir() + h := NewFileHandler(dir) + + rr := httptest.NewRecorder() + h.HandleUpload(rr, uploadMultipart(t, "skillName/SKILL.md", []byte("# skill"))) + + if rr.Code != http.StatusOK { + t.Fatalf("expected 200, got %d", rr.Code) + } + + // The file must land at the nested path, not be flattened to the basename. + nested := filepath.Join(dir, "skillName", "SKILL.md") + if _, err := os.Stat(nested); err != nil { + t.Fatalf("expected nested file at %s: %v", nested, err) + } + if _, err := os.Stat(filepath.Join(dir, "SKILL.md")); err == nil { + t.Error("file must not be flattened to working-dir root") + } + + var resp struct { + Uploaded []FileInfo `json:"uploaded"` + } + if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode response: %v", err) + } + if len(resp.Uploaded) != 1 || resp.Uploaded[0].Name != "skillName/SKILL.md" { + t.Errorf("expected uploaded name skillName/SKILL.md, got %+v", resp.Uploaded) + } +} + +func TestHandleUploadRejectsTraversal(t *testing.T) { + dir := t.TempDir() + h := NewFileHandler(dir) + + rr := httptest.NewRecorder() + h.HandleUpload(rr, uploadMultipart(t, "../escape.txt", []byte("pwned"))) + + // The traversal target must never be written outside the working dir. + if _, err := os.Stat(filepath.Join(filepath.Dir(dir), "escape.txt")); err == nil { + t.Fatal("path traversal escaped the working directory") + } + + var resp struct { + Uploaded []FileInfo `json:"uploaded"` + } + if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode response: %v", err) + } + if len(resp.Uploaded) != 0 { + t.Errorf("traversal upload must be skipped, got %+v", resp.Uploaded) + } +} + +func TestHandleListIsRecursive(t *testing.T) { + dir := t.TempDir() + h := NewFileHandler(dir) + + if err := os.MkdirAll(filepath.Join(dir, "skillName"), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.WriteFile(filepath.Join(dir, "skillName", "SKILL.md"), []byte("x"), 0o644); err != nil { + t.Fatalf("write nested: %v", err) + } + if err := os.WriteFile(filepath.Join(dir, "top.txt"), []byte("y"), 0o644); err != nil { + t.Fatalf("write top: %v", err) + } + + rr := httptest.NewRecorder() + h.HandleList(rr, httptest.NewRequest(http.MethodGet, "/files", nil)) + if rr.Code != http.StatusOK { + t.Fatalf("expected 200, got %d", rr.Code) + } + + var resp struct { + Files []FileInfo `json:"files"` + } + if err := json.Unmarshal(rr.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode response: %v", err) + } + + names := map[string]bool{} + for _, f := range resp.Files { + names[f.Name] = true + } + if !names["skillName/SKILL.md"] { + t.Errorf("expected recursive listing to include skillName/SKILL.md, got %+v", resp.Files) + } + if !names["top.txt"] { + t.Errorf("expected listing to include top.txt, got %+v", resp.Files) + } + // Directories themselves must not be reported as files. + if names["skillName"] { + t.Error("directory entries must not appear in the file listing") + } +} diff --git a/src/api/files.py b/src/api/files.py index a69ac30..ea687c8 100644 --- a/src/api/files.py +++ b/src/api/files.py @@ -160,9 +160,12 @@ async def upload_file( # Read file content content = await file.read() - # Sanitize filename before storage so the name on disk in the - # execution pod matches what LibreChat reports to the model. - sanitized_name = OutputProcessor.sanitize_filename(file.filename) + # Sanitize the upload name before storage so the name on disk in + # the execution pod matches what LibreChat reports to the model. + # sanitize_filepath preserves safe nested directories (e.g. + # skill bundles uploaded as ``skillName/SKILL.md``) instead of + # flattening them to a basename. + sanitized_name = OutputProcessor.sanitize_filepath(file.filename) # Store file with the sanitized name file_id = await file_service.store_uploaded_file( @@ -317,7 +320,7 @@ async def upload_files_batch( raise ValueError(f"File {upload.filename} exceeds maximum size of {settings.max_file_size_mb}MB") content = await upload.read() - sanitized_name = OutputProcessor.sanitize_filename(upload.filename or "file") + sanitized_name = OutputProcessor.sanitize_filepath(upload.filename or "file") file_id = await file_service.store_uploaded_file( session_id=session_id, filename=sanitized_name, diff --git a/src/services/execution/output.py b/src/services/execution/output.py index 972de67..bd512e1 100644 --- a/src/services/execution/output.py +++ b/src/services/execution/output.py @@ -260,3 +260,50 @@ def normalize_filename(cls, filename: str) -> str: sanitize_filename which matches LibreChat's sanitization logic. """ return cls.sanitize_filename(filename) + + @classmethod + def _is_safe_relative_path(cls, path: str) -> bool: + """True if ``path`` is a safe, nested-but-non-escaping relative path. + + Mirrors LibreChat's ``isSafeCodeEnvFilepath`` + (packages/api/src/files/code/form.ts): a path is safe only when it is + non-empty, not absolute, and every ``/``-delimited segment is a real + directory/file name (never ``''``, ``.`` or ``..``). Character-level + cleaning is deferred to per-segment :meth:`sanitize_filename`, so this + check is purely structural — it decides whether the directory layout + may be preserved or must collapse to a basename. + """ + if not path or path.startswith("/"): + return False + return all(segment not in ("", ".", "..") for segment in path.split("/")) + + @classmethod + def sanitize_filepath(cls, input_path: str) -> str: + """Sanitize an upload name while preserving safe nested directories. + + LibreChat skill priming uploads files whose multipart filename carries + a relative path (e.g. ``skillName/SKILL.md``) via + ``getCodeEnvFileOptions`` — see + api/server/services/Files/Code/crud.js and packages/api .../form.ts. + The legacy :meth:`sanitize_filename` flattens these with + ``os.path.basename``, which collapses skill directory structure, breaks + relative references inside skills, and causes cross-skill name + collisions. + + This preserves a safe relative layout: each path segment is sanitized + independently (re-using :meth:`sanitize_filename`) and rejoined with + ``/``. Unsafe inputs (absolute paths or ``.`` / ``..`` / empty + segments) collapse to a single sanitized basename — the legacy + behavior — so path traversal can never widen the stored name. The Go + runner's ``validatePath`` is the authoritative second line of defense + when the file is materialized into the pod. + """ + if not input_path: + return "_" + + normalized = input_path.replace("\\", "/") + if not cls._is_safe_relative_path(normalized): + return cls.sanitize_filename(normalized) + + segments = [cls.sanitize_filename(segment) for segment in normalized.split("/")] + return "/".join(segments) diff --git a/src/services/execution/runner.py b/src/services/execution/runner.py index 8fff3a5..8a01599 100644 --- a/src/services/execution/runner.py +++ b/src/services/execution/runner.py @@ -327,7 +327,12 @@ def _process_outputs(self, stdout: str, stderr: str, timestamp: datetime) -> lis return outputs def _get_mounted_filenames(self, files: list[dict[str, Any]] | None) -> set: - """Get set of mounted filenames for filtering.""" + """Get set of mounted file names for output filtering. + + Includes the full (possibly nested) relative name, its basename, and + the sanitized variants, so a generated-file match works whether the + runner reports a nested path (``skillName/SKILL.md``) or a flat name. + """ mounted = set() if files: try: @@ -335,11 +340,20 @@ def _get_mounted_filenames(self, files: list[dict[str, Any]] | None) -> set: name = f.get("filename") or f.get("name") if name: mounted.add(name) + mounted.add(Path(name).name) mounted.add(OutputProcessor.normalize_filename(name)) except Exception: pass return mounted + @staticmethod + def _relative_under_data(path: str) -> str: + """Return the path relative to /mnt/data, or the basename as fallback.""" + prefix = "/mnt/data/" + if path.startswith(prefix): + return path[len(prefix) :] + return Path(path).name + def _filter_generated_files( self, generated: list[dict[str, Any]], @@ -348,13 +362,20 @@ def _filter_generated_files( ) -> list[dict[str, Any]]: """Filter generated files, keeping new files and modified mounted files. - A mounted file is included if its mod_time indicates it was modified - during or after execution started (fixes issue #56). + Matching is done on the relative path under /mnt/data (and basename as + a fallback) so nested mounted files (e.g. skill bundles uploaded as + ``skillName/SKILL.md``) are correctly recognized as mounted rather than + reported as freshly generated. A mounted file is still included if its + mod_time indicates it was modified during or after execution started + (fixes issue #56). """ result = [] for f in generated: - name = Path(f.get("path", "")).name - if name not in mounted_filenames: + path = f.get("path", "") + rel = self._relative_under_data(path) + base = Path(path).name + is_mounted = rel in mounted_filenames or base in mounted_filenames + if not is_mounted: # New file — always include result.append(f) elif execution_start_unix and f.get("mod_time", 0) >= execution_start_unix: @@ -401,9 +422,13 @@ async def _detect_generated_files(self, handle: PodHandle) -> list[dict[str, Any files = data.get("files", []) generated_files = [] for f in files: - # Skip code files (source files written by the runner) + # Skip code files (source files written by the runner). + # ``name`` may be a nested relative path (e.g. + # ``skillName/out.png``) since the runner lists + # recursively; compare the basename against the known + # source filenames. name = f.get("name", "") - if name in _CODE_FILENAMES: + if Path(name).name in _CODE_FILENAMES: continue if f.get("size", 0) > settings.max_file_size_mb * 1024 * 1024: continue diff --git a/src/services/kubernetes/job_executor.py b/src/services/kubernetes/job_executor.py index ac51000..4fe7f11 100644 --- a/src/services/kubernetes/job_executor.py +++ b/src/services/kubernetes/job_executor.py @@ -358,9 +358,13 @@ async def _upload_files( for file_data in files: try: files_payload = {"files": (file_data.filename, file_data.content)} + # The nested relative path is sent out-of-band because RFC 7578 + # strips directories from the multipart filename; the runner + # recreates the directory tree from this ``path`` value. await client.post( f"{runner_url}/files", files=files_payload, + data={"path": file_data.filename}, timeout=30, ) except Exception as e: diff --git a/src/services/kubernetes/manager.py b/src/services/kubernetes/manager.py index 98ef0a8..f0a0a58 100644 --- a/src/services/kubernetes/manager.py +++ b/src/services/kubernetes/manager.py @@ -349,9 +349,14 @@ async def copy_files_to_pod( async with httpx.AsyncClient(timeout=30.0) as client: for file_data in files: try: + # RFC 7578 strips directories from the multipart filename, + # so the nested layout (e.g. skill bundles uploaded as + # "skillName/SKILL.md") is sent out-of-band in the ``path`` + # field for the runner to recreate the directory tree. await client.post( f"{handle.runner_url}/files", files={"files": (file_data.filename, file_data.content)}, + data={"path": file_data.filename}, ) except Exception as e: logger.error( diff --git a/tests/unit/test_execution_runner.py b/tests/unit/test_execution_runner.py index 0a3ddb3..1c02ede 100644 --- a/tests/unit/test_execution_runner.py +++ b/tests/unit/test_execution_runner.py @@ -383,6 +383,13 @@ def test_get_mounted_filenames_multiple(self, runner): assert "test.txt" in result assert "data.csv" in result + def test_get_mounted_filenames_nested_includes_basename(self, runner): + """Nested mounted paths expose both the full relative path and basename.""" + files = [{"filename": "skillName/SKILL.md"}] + result = runner._get_mounted_filenames(files) + assert "skillName/SKILL.md" in result + assert "SKILL.md" in result + class TestFilterGeneratedFiles: """Tests for _filter_generated_files method.""" @@ -442,6 +449,32 @@ def test_filter_no_execution_start_falls_back_to_exclude(self, runner): assert len(result) == 1 assert result[0]["path"] == "/mnt/data/output.txt" + def test_filter_nested_mounted_excluded_when_unmodified(self, runner): + """A nested mounted skill file is matched by relative path and excluded.""" + generated = [ + {"path": "/mnt/data/skillName/SKILL.md", "mod_time": 900}, + {"path": "/mnt/data/skillName/out.png", "mod_time": 1005}, + ] + mounted = {"skillName/SKILL.md", "SKILL.md"} + + result = runner._filter_generated_files(generated, mounted, execution_start_unix=1000) + + # SKILL.md (mounted, unmodified) excluded; out.png (new) kept. + assert len(result) == 1 + assert result[0]["path"] == "/mnt/data/skillName/out.png" + + def test_filter_nested_mounted_kept_when_modified(self, runner): + """A nested mounted file modified during execution is retained (issue #56).""" + generated = [ + {"path": "/mnt/data/skillName/SKILL.md", "mod_time": 1004}, + ] + mounted = {"skillName/SKILL.md", "SKILL.md"} + + result = runner._filter_generated_files(generated, mounted, execution_start_unix=1000) + + assert len(result) == 1 + assert result[0]["path"] == "/mnt/data/skillName/SKILL.md" + def test_filter_empty_generated(self, runner): """Test filtering with no generated files.""" result = runner._filter_generated_files([], {"input.csv"}) diff --git a/tests/unit/test_librechat_contract.py b/tests/unit/test_librechat_contract.py index 8a72796..27f9cb3 100644 --- a/tests/unit/test_librechat_contract.py +++ b/tests/unit/test_librechat_contract.py @@ -162,6 +162,35 @@ async def test_upload_emits_storage_session_id(self): assert result["session_id"] == "sess-1" assert result["files"][0]["fileId"] == "fid-1" + @pytest.mark.asyncio + async def test_upload_preserves_nested_skill_path(self): + """Skill bundles upload files whose multipart filename carries a + relative path (e.g. ``skillName/SKILL.md`` — see packages/api + .../form.ts ``getCodeEnvFileOptions``). The stored name must keep the + directory structure instead of being flattened to a basename, so the + skill's relative references resolve under /mnt/data.""" + session_service = MagicMock() + session_service.list_sessions_by_entity = AsyncMock(return_value=[]) + session_service.create_session = AsyncMock(return_value=_mock_session("sess-1")) + + file_service = MagicMock() + file_service.store_uploaded_file = AsyncMock(return_value="fid-1") + + result = await upload_file( + request=_anon_http_request(), + file=_mock_file(filename="skillName/SKILL.md"), + files=None, + entity_id=None, + user_id_header=None, + x_user_id_header=None, + file_service=file_service, + session_service=session_service, + ) + + stored_name = file_service.store_uploaded_file.call_args.kwargs["filename"] + assert stored_name == "skillName/SKILL.md" + assert result["files"][0]["filename"] == "skillName/SKILL.md" + class TestUploadBatchEndpoint: @pytest.mark.asyncio diff --git a/tests/unit/test_output_processor.py b/tests/unit/test_output_processor.py index 26c6c3f..44373f9 100644 --- a/tests/unit/test_output_processor.py +++ b/tests/unit/test_output_processor.py @@ -105,6 +105,70 @@ def test_long_filename_truncated(self): assert "-" in result +class TestSanitizeFilepath: + """Tests for the path-preserving sanitize_filepath method (Gap G).""" + + def test_preserves_safe_nested_path(self): + """Nested skill paths keep their directory structure.""" + result = OutputProcessor.sanitize_filepath("skillName/SKILL.md") + assert result == "skillName/SKILL.md" + + def test_preserves_deeply_nested_path(self): + """Multi-level nesting is preserved.""" + result = OutputProcessor.sanitize_filepath("skillName/sub/dir/file.py") + assert result == "skillName/sub/dir/file.py" + + def test_sanitizes_each_segment(self): + """Each path segment is individually sanitized while '/' is kept.""" + result = OutputProcessor.sanitize_filepath("my skill/data (v2).csv") + assert result == "my_skill/data__v2_.csv" + + def test_backslashes_normalized(self): + """Windows-style separators normalize to forward slashes.""" + result = OutputProcessor.sanitize_filepath("skillName\\SKILL.md") + assert result == "skillName/SKILL.md" + + def test_absolute_path_falls_back_to_basename(self): + """Absolute paths are unsafe and collapse to a sanitized basename.""" + result = OutputProcessor.sanitize_filepath("/etc/passwd") + assert result == "passwd" + + def test_parent_traversal_falls_back_to_basename(self): + """'..' segments are unsafe and collapse to a sanitized basename.""" + result = OutputProcessor.sanitize_filepath("../../etc/passwd") + assert result == "passwd" + + def test_embedded_traversal_falls_back_to_basename(self): + """A traversal segment anywhere makes the whole path unsafe.""" + result = OutputProcessor.sanitize_filepath("skillName/../../../etc/passwd") + assert result == "passwd" + + def test_dot_segment_falls_back_to_basename(self): + """A '.' segment is rejected, collapsing to the basename.""" + result = OutputProcessor.sanitize_filepath("skillName/./file.txt") + assert result == "file.txt" + + def test_empty_segment_falls_back_to_basename(self): + """Double slashes create an empty segment and are rejected.""" + result = OutputProcessor.sanitize_filepath("skillName//file.txt") + assert result == "file.txt" + + def test_flat_filename_unchanged(self): + """A non-nested name behaves like sanitize_filename.""" + result = OutputProcessor.sanitize_filepath("data.csv") + assert result == "data.csv" + + def test_empty_returns_underscore(self): + """Empty input returns a safe placeholder.""" + assert OutputProcessor.sanitize_filepath("") == "_" + + def test_no_segment_can_become_traversal(self): + """Sanitized output must never contain a usable '..' traversal segment.""" + result = OutputProcessor.sanitize_filepath("ok/normal/file.txt") + assert ".." not in result.split("/") + assert not result.startswith("/") + + class TestNormalizeFilename: """Tests for the deprecated normalize_filename method.""" From a7d91d773ec1ac87690d2ca1cdd8cb7070a20328 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gil=20Assun=C3=A7=C3=A3o?= Date: Wed, 24 Jun 2026 12:22:56 +0100 Subject: [PATCH 2/8] feat(files): enforce read_only inputs and echo inherited passthroughs 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. --- src/api/files.py | 7 ++++ src/models/exec.py | 19 +++++++++ src/models/files.py | 8 ++++ src/services/execution/runner.py | 40 +++++++++++++++++- src/services/file.py | 3 ++ src/services/interfaces.py | 7 +++- src/services/orchestrator.py | 19 +++++++++ tests/unit/test_cross_session_files.py | 1 + tests/unit/test_execution_runner.py | 30 +++++++++++++ tests/unit/test_librechat_contract.py | 56 +++++++++++++++++++++++++ tests/unit/test_orchestrator.py | 58 ++++++++++++++++++++++++++ 11 files changed, 245 insertions(+), 3 deletions(-) diff --git a/src/api/files.py b/src/api/files.py index ea687c8..78e5669 100644 --- a/src/api/files.py +++ b/src/api/files.py @@ -314,6 +314,12 @@ async def upload_files_batch( succeeded = 0 failed = 0 + # ``read_only=true`` marks every file in the batch as infrastructure (a + # skill bundle). Persisted on file metadata so the executor can echo it + # back as an inherited input rather than surfacing it as a generated + # artifact (mirrors code-interpreter's X-Read-Only semantics). + read_only_flag = (read_only or "").strip().lower() == "true" + for upload in upload_files: try: if upload.size and upload.size > settings.max_file_size_mb * 1024 * 1024: @@ -326,6 +332,7 @@ async def upload_files_batch( filename=sanitized_name, content=content, content_type=upload.content_type, + read_only=read_only_flag, ) results.append( { diff --git a/src/models/exec.py b/src/models/exec.py index b6e6976..19681db 100644 --- a/src/models/exec.py +++ b/src/models/exec.py @@ -26,6 +26,15 @@ class FileRef(BaseModel): resource_id: str | None = None kind: str | None = None # 'skill' | 'agent' | 'user' version: int | None = None + inherited: bool = Field( + default=False, + description=( + "True when this entry is an unchanged passthrough of a read-only " + "input the caller already owns (skill/agent bundle), surfaced via " + "ExecResponse.inherited_files so callers can render inputs " + "distinctly from generated outputs." + ), + ) @computed_field # type: ignore[prop-decorator] @property @@ -84,6 +93,16 @@ class ExecResponse(BaseModel): files: list[FileRef] = Field(default_factory=list) stdout: str = "" stderr: str = "" + inherited_files: list[FileRef] = Field( + default_factory=list, + description=( + "Read-only inputs (skill/agent bundles) the caller already owns, " + "echoed as unchanged passthroughs. Kept out of `files` so the " + "current @librechat/agents consumer does not surface them as " + "generated download artifacts; each entry carries `inherited: true` " + "for forward-compatible consumers." + ), + ) # State persistence fields (Python only) has_state: bool = Field( default=False, diff --git a/src/models/files.py b/src/models/files.py index 9041c4f..aea6d58 100644 --- a/src/models/files.py +++ b/src/models/files.py @@ -39,6 +39,14 @@ class FileInfo(BaseModel): content_type: str created_at: datetime path: str = Field(..., description="File path in the session") + read_only: bool = Field( + default=False, + description=( + "True when the file was uploaded as infrastructure (e.g. a skill " + "bundle marked read_only). Read-only inputs are echoed back as " + "inherited passthroughs rather than surfaced as generated outputs." + ), + ) @field_serializer("created_at") def serialize_created_at(self, value: datetime) -> str: diff --git a/src/services/execution/runner.py b/src/services/execution/runner.py index 8a01599..03f7e75 100644 --- a/src/services/execution/runner.py +++ b/src/services/execution/runner.py @@ -225,8 +225,12 @@ async def execute( ] mounted_filenames = self._get_mounted_filenames(files) + readonly_filenames = self._get_readonly_filenames(files) filtered_files = self._filter_generated_files( - generated_files, mounted_filenames, execution_start_unix=int(start_time.timestamp()) + generated_files, + mounted_filenames, + execution_start_unix=int(start_time.timestamp()), + readonly_filenames=readonly_filenames, ) for file_info in filtered_files: @@ -346,6 +350,31 @@ def _get_mounted_filenames(self, files: list[dict[str, Any]] | None) -> set: pass return mounted + def _get_readonly_filenames(self, files: list[dict[str, Any]] | None) -> set: + """Get the set of mounted file names flagged ``read_only``. + + Read-only files are infrastructure inputs (skill/agent bundles) the + caller already owns. They must never be surfaced as generated + artifacts even if the sandbox touched them, so callers don't get + ghost downloads for files they uploaded. Mirrors the full/basename/ + normalized expansion of ``_get_mounted_filenames`` so the match works + for nested skill paths. + """ + readonly = set() + if files: + try: + for f in files: + if not f.get("read_only"): + continue + name = f.get("filename") or f.get("name") + if name: + readonly.add(name) + readonly.add(Path(name).name) + readonly.add(OutputProcessor.normalize_filename(name)) + except Exception: + pass + return readonly + @staticmethod def _relative_under_data(path: str) -> str: """Return the path relative to /mnt/data, or the basename as fallback.""" @@ -359,6 +388,7 @@ def _filter_generated_files( generated: list[dict[str, Any]], mounted_filenames: set, execution_start_unix: int = 0, + readonly_filenames: set | None = None, ) -> list[dict[str, Any]]: """Filter generated files, keeping new files and modified mounted files. @@ -367,13 +397,19 @@ def _filter_generated_files( ``skillName/SKILL.md``) are correctly recognized as mounted rather than reported as freshly generated. A mounted file is still included if its mod_time indicates it was modified during or after execution started - (fixes issue #56). + (fixes issue #56) — UNLESS it is flagged ``read_only``: read-only + inputs (skill/agent bundles) are always excluded so the caller never + sees a ghost download for an infrastructure file it already owns. """ + readonly_filenames = readonly_filenames or set() result = [] for f in generated: path = f.get("path", "") rel = self._relative_under_data(path) base = Path(path).name + if rel in readonly_filenames or base in readonly_filenames: + # Read-only input — never surface as a generated artifact. + continue is_mounted = rel in mounted_filenames or base in mounted_filenames if not is_mounted: # New file — always include diff --git a/src/services/file.py b/src/services/file.py index 1f7e0cf..a219445 100644 --- a/src/services/file.py +++ b/src/services/file.py @@ -239,6 +239,7 @@ async def get_file_info(self, session_id: str, file_id: str) -> FileInfo | None: content_type=metadata["content_type"], created_at=metadata["created_at"], path=metadata["path"], + read_only=metadata.get("read_only", False), ) async def list_files(self, session_id: str) -> list[FileInfo]: @@ -472,6 +473,7 @@ async def store_uploaded_file( filename: str, content: bytes, content_type: str | None = None, + read_only: bool = False, ) -> str: """Store an uploaded file directly.""" await self._ensure_bucket_exists() @@ -510,6 +512,7 @@ async def store_uploaded_file( "size": len(content), "path": f"/{filename}", "type": "upload", # Mark as uploaded file + "read_only": read_only, } await self._store_file_metadata(session_id, file_id, metadata) diff --git a/src/services/interfaces.py b/src/services/interfaces.py index 0ab2cf9..b7179d5 100644 --- a/src/services/interfaces.py +++ b/src/services/interfaces.py @@ -148,7 +148,12 @@ async def cleanup_session_files(self, session_id: str) -> int: @abstractmethod async def store_uploaded_file( - self, session_id: str, filename: str, content: bytes, content_type: str | None = None + self, + session_id: str, + filename: str, + content: bytes, + content_type: str | None = None, + read_only: bool = False, ) -> str: """Store an uploaded file directly. Returns file_id.""" pass diff --git a/src/services/orchestrator.py b/src/services/orchestrator.py index 336b5a8..efeb2a6 100644 --- a/src/services/orchestrator.py +++ b/src/services/orchestrator.py @@ -444,6 +444,7 @@ async def _is_file_ref_authorized(file_session_id: str) -> bool: "session_id": file_ref.session_id, "content": content, "auto_mounted": False, + "read_only": file_info.read_only, } ) mounted_keys.add(key) @@ -459,6 +460,7 @@ async def _is_file_ref_authorized(file_session_id: str) -> bool: filename=file_info.filename, content=content, content_type=file_info.content_type, + read_only=file_info.read_only, ) logger.info( "Consolidated cross-session file", @@ -526,6 +528,7 @@ async def _is_file_ref_authorized(file_session_id: str) -> bool: "session_id": ctx.session_id, "content": content, "auto_mounted": True, + "read_only": file_info.read_only, } ) mounted_keys.add(key) @@ -786,11 +789,27 @@ def _build_response(self, ctx: ExecutionContext) -> ExecResponse: auto_mounted = sum(1 for f in ctx.mounted_files if f.get("auto_mounted")) if ctx.mounted_files else 0 + # Echo read-only inputs (skill/agent bundles the caller already owns) + # as inherited passthroughs. They are deliberately kept out of `files` + # so the current @librechat/agents consumer does not surface them as + # generated download artifacts (it ignores the `inherited` flag). + inherited_files = [ + FileRef( + id=f["file_id"], + name=f["filename"], + session_id=f.get("session_id"), + inherited=True, + ) + for f in (ctx.mounted_files or []) + if f.get("read_only") + ] + return ExecResponse( session_id=ctx.session_id, files=ctx.generated_files or [], stdout=ctx.stdout, stderr=ctx.stderr, + inherited_files=inherited_files, has_state=has_state, state_size=state_size, state_hash=state_hash, diff --git a/tests/unit/test_cross_session_files.py b/tests/unit/test_cross_session_files.py index 91196c4..802cf77 100644 --- a/tests/unit/test_cross_session_files.py +++ b/tests/unit/test_cross_session_files.py @@ -168,6 +168,7 @@ async def get_file_info_side_effect(session_id, file_id): filename="data_b.csv", content=b"csv data", content_type="text/csv", + read_only=False, ) @pytest.mark.asyncio diff --git a/tests/unit/test_execution_runner.py b/tests/unit/test_execution_runner.py index 1c02ede..3b60e97 100644 --- a/tests/unit/test_execution_runner.py +++ b/tests/unit/test_execution_runner.py @@ -480,6 +480,36 @@ def test_filter_empty_generated(self, runner): result = runner._filter_generated_files([], {"input.csv"}) assert result == [] + def test_filter_readonly_excluded_even_when_modified(self, runner): + """Read-only inputs are never surfaced as generated artifacts (Gap A).""" + generated = [ + {"path": "/mnt/data/skillName/SKILL.md", "mod_time": 1005}, + {"path": "/mnt/data/out.png", "mod_time": 1005}, + ] + mounted = {"skillName/SKILL.md", "SKILL.md"} + readonly = {"skillName/SKILL.md", "SKILL.md"} + + result = runner._filter_generated_files( + generated, mounted, execution_start_unix=1000, readonly_filenames=readonly + ) + + # SKILL.md (read-only, even though modified) excluded; out.png kept. + assert len(result) == 1 + assert result[0]["path"] == "/mnt/data/out.png" + + def test_get_readonly_filenames_only_flagged(self, runner): + """Only files flagged read_only are returned, with basename expansion.""" + files = [ + {"filename": "skillName/SKILL.md", "read_only": True}, + {"filename": "user_upload.csv", "read_only": False}, + {"filename": "no_flag.txt"}, + ] + result = runner._get_readonly_filenames(files) + assert "skillName/SKILL.md" in result + assert "SKILL.md" in result + assert "user_upload.csv" not in result + assert "no_flag.txt" not in result + def test_filter_empty_mounted(self, runner): """Test filtering with no mounted files.""" generated = [{"path": "/mnt/data/output.txt"}] diff --git a/tests/unit/test_librechat_contract.py b/tests/unit/test_librechat_contract.py index 27f9cb3..43ecc81 100644 --- a/tests/unit/test_librechat_contract.py +++ b/tests/unit/test_librechat_contract.py @@ -225,6 +225,62 @@ async def test_batch_returns_storage_session_id_and_counts(self): assert result["failed"] == 0 assert {f["fileId"] for f in result["files"]} == {"fid-a", "fid-b"} + @pytest.mark.asyncio + async def test_batch_read_only_threaded_to_storage(self): + """``read_only=true`` is persisted on each stored file so the executor + can echo it as an inherited input instead of a generated artifact.""" + session_service = MagicMock() + session_service.list_sessions_by_entity = AsyncMock(return_value=[]) + session_service.create_session = AsyncMock(return_value=_mock_session("sess-b")) + + file_service = MagicMock() + file_service.store_uploaded_file = AsyncMock(side_effect=["fid-a", "fid-b"]) + + await upload_files_batch( + request=_anon_http_request(), + file=[_mock_file("skillName/SKILL.md", b"x"), _mock_file("skillName/run.py", b"y")], + files=None, + entity_id=None, + kind="skill", + id="skill-123", + version="3", + read_only="true", + user_id_header=None, + x_user_id_header=None, + file_service=file_service, + session_service=session_service, + ) + + assert file_service.store_uploaded_file.await_count == 2 + assert all(c.kwargs["read_only"] is True for c in file_service.store_uploaded_file.await_args_list) + + @pytest.mark.asyncio + async def test_batch_read_only_defaults_false(self): + """Without ``read_only``, stored files default to writable (read_only=False).""" + session_service = MagicMock() + session_service.list_sessions_by_entity = AsyncMock(return_value=[]) + session_service.create_session = AsyncMock(return_value=_mock_session("sess-b")) + + file_service = MagicMock() + file_service.store_uploaded_file = AsyncMock(return_value="fid-a") + + await upload_files_batch( + request=_anon_http_request(), + file=[_mock_file("data.csv", b"x")], + files=None, + entity_id=None, + kind="user", + id=None, + version=None, + read_only=None, + user_id_header=None, + x_user_id_header=None, + file_service=file_service, + session_service=session_service, + ) + + assert file_service.store_uploaded_file.await_args.kwargs["read_only"] is False + @pytest.mark.asyncio async def test_batch_kind_skill_persists_on_session_metadata(self): """``kind`` / ``id`` form fields land on session.metadata so the diff --git a/tests/unit/test_orchestrator.py b/tests/unit/test_orchestrator.py index 749c4d0..29f8e0d 100644 --- a/tests/unit/test_orchestrator.py +++ b/tests/unit/test_orchestrator.py @@ -1227,6 +1227,64 @@ def test_build_response_with_state(self, orchestrator, mock_state_service): assert response.state_size == len(state_bytes) assert response.state_hash == "abc123" + def test_build_response_echoes_readonly_as_inherited(self, orchestrator): + """Read-only mounted inputs are echoed in inherited_files, not files (Gap A).""" + request = ExecRequest(code="print('hi')", lang="py") + ctx = ExecutionContext( + request=request, + request_id="req-123", + session_id="session-123", + stdout="", + stderr="", + new_state=None, + mounted_files=[ + { + "file_id": "skill-1", + "filename": "skillName/SKILL.md", + "session_id": "upload-sess", + "read_only": True, + "auto_mounted": False, + }, + { + "file_id": "user-1", + "filename": "data.csv", + "session_id": "upload-sess", + "read_only": False, + "auto_mounted": False, + }, + ], + ) + + response = orchestrator._build_response(ctx) + + assert len(response.inherited_files) == 1 + inherited = response.inherited_files[0] + assert inherited.id == "skill-1" + assert inherited.name == "skillName/SKILL.md" + assert inherited.session_id == "upload-sess" + assert inherited.inherited is True + # Read-only inputs must NOT leak into the generated files list. + assert all(f.name != "skillName/SKILL.md" for f in response.files) + + def test_build_response_no_inherited_when_no_readonly(self, orchestrator): + """No read-only inputs means an empty inherited_files list.""" + request = ExecRequest(code="print('hi')", lang="py") + ctx = ExecutionContext( + request=request, + request_id="req-123", + session_id="session-123", + stdout="", + stderr="", + new_state=None, + mounted_files=[ + {"file_id": "user-1", "filename": "data.csv", "read_only": False}, + ], + ) + + response = orchestrator._build_response(ctx) + + assert response.inherited_files == [] + class TestCleanupExtended: """Tests for _cleanup method - extended.""" From 390360a82fbb13a0e2c1579665998603bf3175ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gil=20Assun=C3=A7=C3=A3o?= Date: Wed, 24 Jun 2026 14:11:35 +0100 Subject: [PATCH 3/8] feat(files): add identity form fields to single /upload 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. --- src/api/files.py | 16 ++++++ tests/unit/test_api_files.py | 28 +++++++++++ tests/unit/test_cross_session_files.py | 20 ++++++++ tests/unit/test_librechat_contract.py | 69 ++++++++++++++++++++++++++ 4 files changed, 133 insertions(+) diff --git a/src/api/files.py b/src/api/files.py index 78e5669..c541c42 100644 --- a/src/api/files.py +++ b/src/api/files.py @@ -49,6 +49,10 @@ async def upload_file( file: UploadFile | None = File(None), files: list[UploadFile] | None = File(None), entity_id: str | None = Form(None), + kind: str | None = Form(None), + id: str | None = Form(None), + version: str | None = Form(None), + read_only: str | None = Form(None), user_id_header: str | None = Header(None, alias="User-Id"), x_user_id_header: str | None = Header(None, alias="X-User-Id"), file_service: FileServiceDep = None, @@ -59,6 +63,12 @@ async def upload_file( Accepts files in either 'file' (singular) or 'files' (plural) field names. LibreChat uses 'file' while our tests use 'files'. + Identity form fields (``kind`` / ``id`` / ``version`` / ``read_only``) + mirror ``/upload/batch`` so single-file uploads carry the same LibreChat + ``CodeEnvFile`` discriminator. ``kind`` / ``id`` land on session metadata + and ``read_only`` is persisted per file so skill/agent inputs are echoed + as inherited passthroughs rather than generated artifacts. + user_id resolution (most-trustworthy first): 1. JWT.sub via request.state.user_id (cryptographically authenticated by SecurityMiddleware when codeapi_jwt_enabled). @@ -71,6 +81,7 @@ async def upload_file( """ jwt_user_id = getattr(request.state, "user_id", None) if request else None request_user_id = jwt_user_id or user_id_header or x_user_id_header + read_only_flag = (read_only or "").strip().lower() == "true" try: # Handle both singular and plural field names upload_files = [] @@ -153,6 +164,10 @@ async def upload_file( session_metadata["entity_id"] = entity_id if request_user_id: session_metadata["user_id"] = request_user_id + if kind: + session_metadata["kind"] = kind + if id: + session_metadata["resource_id"] = id session = await session_service.create_session(SessionCreate(metadata=session_metadata)) session_id = session.session_id @@ -173,6 +188,7 @@ async def upload_file( filename=sanitized_name, content=content, content_type=file.content_type, + read_only=read_only_flag, ) uploaded_files.append( diff --git a/tests/unit/test_api_files.py b/tests/unit/test_api_files.py index 01a12f3..44a8f52 100644 --- a/tests/unit/test_api_files.py +++ b/tests/unit/test_api_files.py @@ -140,6 +140,10 @@ async def test_upload_single_file( file=mock_upload_file, files=None, entity_id="entity-123", + kind=None, + id=None, + version=None, + read_only=None, file_service=mock_file_service, session_service=mock_session_service, ) @@ -176,6 +180,10 @@ async def test_upload_sanitizes_filename_before_storage( file=file, files=None, entity_id=None, + kind=None, + id=None, + version=None, + read_only=None, file_service=mock_file_service, session_service=mock_session_service, ) @@ -199,6 +207,10 @@ async def test_upload_multiple_files( file=None, files=files, entity_id="entity-123", + kind=None, + id=None, + version=None, + read_only=None, file_service=mock_file_service, session_service=mock_session_service, ) @@ -216,6 +228,10 @@ async def test_upload_no_files(self, mock_file_service, mock_http_request): file=None, files=None, entity_id="entity-123", + kind=None, + id=None, + version=None, + read_only=None, file_service=mock_file_service, ) @@ -235,6 +251,10 @@ async def test_upload_file_too_large(self, mock_file_service, mock_upload_file, file=mock_upload_file, files=None, entity_id="entity-123", + kind=None, + id=None, + version=None, + read_only=None, file_service=mock_file_service, ) @@ -255,6 +275,10 @@ async def test_upload_too_many_files(self, mock_file_service, mock_upload_file, file=None, files=files, entity_id="entity-123", + kind=None, + id=None, + version=None, + read_only=None, file_service=mock_file_service, ) @@ -277,6 +301,10 @@ async def test_upload_file_service_error( file=mock_upload_file, files=None, entity_id="entity-123", + kind=None, + id=None, + version=None, + read_only=None, file_service=mock_file_service, session_service=mock_session_service, ) diff --git a/tests/unit/test_cross_session_files.py b/tests/unit/test_cross_session_files.py index 802cf77..c97c44a 100644 --- a/tests/unit/test_cross_session_files.py +++ b/tests/unit/test_cross_session_files.py @@ -298,6 +298,10 @@ async def test_upload_reuses_session_with_entity_id_same_user(self): file=mock_file, files=None, entity_id="conversation-123", + kind=None, + id=None, + version=None, + read_only=None, user_id_header="user-A", x_user_id_header=None, file_service=mock_file_service, @@ -342,6 +346,10 @@ async def test_upload_does_NOT_reuse_session_for_different_user(self): file=mock_file, files=None, entity_id="conversation-123", + kind=None, + id=None, + version=None, + read_only=None, user_id_header="user-B", x_user_id_header=None, file_service=mock_file_service, @@ -380,6 +388,10 @@ async def test_upload_persists_user_id_on_session(self): file=mock_file, files=None, entity_id="conv-1", + kind=None, + id=None, + version=None, + read_only=None, user_id_header="user-A", x_user_id_header=None, file_service=mock_file_service, @@ -416,6 +428,10 @@ async def test_upload_creates_session_without_entity_id(self): file=mock_file, files=None, entity_id=None, + kind=None, + id=None, + version=None, + read_only=None, user_id_header=None, x_user_id_header=None, file_service=mock_file_service, @@ -452,6 +468,10 @@ async def test_upload_creates_session_when_entity_lookup_fails(self): file=mock_file, files=None, entity_id="conversation-123", + kind=None, + id=None, + version=None, + read_only=None, user_id_header="user-A", x_user_id_header=None, file_service=mock_file_service, diff --git a/tests/unit/test_librechat_contract.py b/tests/unit/test_librechat_contract.py index 43ecc81..8797d32 100644 --- a/tests/unit/test_librechat_contract.py +++ b/tests/unit/test_librechat_contract.py @@ -151,6 +151,10 @@ async def test_upload_emits_storage_session_id(self): file=_mock_file(), files=None, entity_id=None, + kind=None, + id=None, + version=None, + read_only=None, user_id_header=None, x_user_id_header=None, file_service=file_service, @@ -181,6 +185,10 @@ async def test_upload_preserves_nested_skill_path(self): file=_mock_file(filename="skillName/SKILL.md"), files=None, entity_id=None, + kind=None, + id=None, + version=None, + read_only=None, user_id_header=None, x_user_id_header=None, file_service=file_service, @@ -191,6 +199,67 @@ async def test_upload_preserves_nested_skill_path(self): assert stored_name == "skillName/SKILL.md" assert result["files"][0]["filename"] == "skillName/SKILL.md" + @pytest.mark.asyncio + async def test_upload_threads_identity_fields(self): + """Single /upload accepts kind/id/version/read_only like /upload/batch: + ``read_only`` is persisted on the file and ``kind`` / ``id`` land on + session metadata.""" + from src.models.session import SessionCreate + + session_service = MagicMock() + session_service.list_sessions_by_entity = AsyncMock(return_value=[]) + session_service.create_session = AsyncMock(return_value=_mock_session("sess-1")) + + file_service = MagicMock() + file_service.store_uploaded_file = AsyncMock(return_value="fid-1") + + await upload_file( + request=_anon_http_request(), + file=_mock_file(filename="skillName/SKILL.md"), + files=None, + entity_id=None, + kind="skill", + id="skill-123", + version="3", + read_only="true", + user_id_header="user-A", + x_user_id_header=None, + file_service=file_service, + session_service=session_service, + ) + + assert file_service.store_uploaded_file.call_args.kwargs["read_only"] is True + sc: SessionCreate = session_service.create_session.call_args.args[0] + assert sc.metadata.get("kind") == "skill" + assert sc.metadata.get("resource_id") == "skill-123" + + @pytest.mark.asyncio + async def test_upload_read_only_defaults_false(self): + """Without identity fields, single /upload defaults read_only=False.""" + session_service = MagicMock() + session_service.list_sessions_by_entity = AsyncMock(return_value=[]) + session_service.create_session = AsyncMock(return_value=_mock_session("sess-1")) + + file_service = MagicMock() + file_service.store_uploaded_file = AsyncMock(return_value="fid-1") + + await upload_file( + request=_anon_http_request(), + file=_mock_file(), + files=None, + entity_id=None, + kind=None, + id=None, + version=None, + read_only=None, + user_id_header=None, + x_user_id_header=None, + file_service=file_service, + session_service=session_service, + ) + + assert file_service.store_uploaded_file.call_args.kwargs["read_only"] is False + class TestUploadBatchEndpoint: @pytest.mark.asyncio From 3c92153c352fa71cc9d22d675bb9652df8783d31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gil=20Assun=C3=A7=C3=A3o?= Date: Wed, 24 Jun 2026 14:15:01 +0100 Subject: [PATCH 4/8] feat(files): add DELETE /sessions/{session}/objects/{file} alias 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. --- src/api/files.py | 20 ++++++++++++++++++++ tests/unit/test_api_files.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/src/api/files.py b/src/api/files.py index c541c42..8913767 100644 --- a/src/api/files.py +++ b/src/api/files.py @@ -698,3 +698,23 @@ async def delete_file(session_id: str, file_id: str, file_service: FileServiceDe error=str(e), ) raise HTTPException(status_code=500, detail="Failed to delete file") + + +@router.delete("/sessions/{session_id}/objects/{file_id}") +async def delete_session_object( + session_id: str, + file_id: str, + kind: str | None = Query(None, description="Resource kind: 'skill', 'agent', or 'user'"), + resource_id: str | None = Query(None, alias="id", description="Resource id (userId / agentId / skillId)"), + version: int | None = Query(None, description="Resource version (only meaningful for kind=skill)"), + file_service: FileServiceDep = None, +): + """Delete a session object - LibreChat compatible alias of ``delete_file``. + + LibreChat's ``deleteFile`` (process.js) issues ``DELETE /sessions/{session}/ + objects/{file}`` first and only falls back to ``DELETE /files/{session}/ + {file}`` on failure. Serving this path directly avoids the extra 404 + round-trip. Query params ``kind``/``id``/``version`` are accepted for + parity with ``buildCodeEnvDownloadQuery`` but are not enforced today. + """ + return await delete_file(session_id, file_id, file_service) diff --git a/tests/unit/test_api_files.py b/tests/unit/test_api_files.py index 44a8f52..d5a2ac3 100644 --- a/tests/unit/test_api_files.py +++ b/tests/unit/test_api_files.py @@ -10,6 +10,7 @@ _ascii_fallback_filename, _build_content_disposition, delete_file, + delete_session_object, download_file, download_file_options, list_files, @@ -596,3 +597,36 @@ async def test_delete_file_error(self, mock_file_service, mock_file_info): ) assert exc_info.value.status_code == 500 + + @pytest.mark.asyncio + async def test_delete_session_object_aliases_delete_file(self, mock_file_service, mock_file_info): + """The /sessions/{s}/objects/{f} alias deletes via the same path as + /files/{s}/{f}; LibreChat tries this route first to avoid a 404 fallback.""" + mock_file_service.get_file_info.return_value = mock_file_info + mock_file_service.delete_file.return_value = True + + response = await delete_session_object( + session_id="session-123", + file_id="file-123", + kind="skill", + resource_id="skill-1", + version=2, + file_service=mock_file_service, + ) + + assert response.status_code == 200 + mock_file_service.delete_file.assert_called_once_with("session-123", "file-123") + + @pytest.mark.asyncio + async def test_delete_session_object_not_found(self, mock_file_service): + """Missing file via the alias still raises 404.""" + mock_file_service.get_file_info.return_value = None + + with pytest.raises(HTTPException) as exc_info: + await delete_session_object( + session_id="session-123", + file_id="nonexistent", + file_service=mock_file_service, + ) + + assert exc_info.value.status_code == 404 From 6bc3d2a11b19d27b77e6f5f92259adb1e63c5339 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gil=20Assun=C3=A7=C3=A3o?= Date: Wed, 24 Jun 2026 15:30:27 +0100 Subject: [PATCH 5/8] feat(build): ensure runner image builds first as dependency * 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. --- scripts/build-images.sh | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/scripts/build-images.sh b/scripts/build-images.sh index 18593f5..7598d26 100755 --- a/scripts/build-images.sh +++ b/scripts/build-images.sh @@ -362,10 +362,41 @@ main() { echo "Building ${#all_images[@]} images..." echo "━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━" + # Build the runner image FIRST (synchronously). Every language image does + # `FROM ${RUNNER_IMAGE} AS runner`, so the runner must already be built and + # tagged locally before those builds start — otherwise their `FROM` + # resolves against the registry tag (which only exists after a push) and + # fails with "not found". Gate the rest of the build on its success. + for entry in "${INFRA_IMAGES[@]}"; do + IFS=':' read -r dockerfile image_name context_dir <<< "$entry" + [[ "$image_name" != "runner" ]] && continue + + if [[ ! -f "$DOCKER_DIR/$dockerfile" ]]; then + echo "Error: runner Dockerfile not found: $dockerfile" + exit 1 + fi + + echo "Starting: runner (dependency — built first)" + build_image_wrapper "$dockerfile" "$image_name" "$context_dir" + if ! ( source "$RESULTS_DIR/${image_name}.result"; [[ "$EXIT_CODE" -eq 0 ]] ); then + echo "" + echo "runner image build failed — aborting (all language images depend on it)." + if [[ -f "$RESULTS_DIR/${image_name}.result.log" ]]; then + echo "━━━ runner ━━━" + tail -n 30 "$RESULTS_DIR/${image_name}.result.log" + fi + exit 1 + fi + break + done + for entry in "${all_images[@]}"; do # Parse entry: dockerfile:image_name:context_dir IFS=':' read -r dockerfile image_name context_dir <<< "$entry" + # runner was already built above as a prerequisite for the rest + [[ "$image_name" == "runner" ]] && continue + if [[ ! -f "$DOCKER_DIR/$dockerfile" ]]; then echo "Warning: Dockerfile not found: $dockerfile" continue From 502abefc865936f38b612de926bad6fcee9e5835 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gil=20Assun=C3=A7=C3=A3o?= Date: Thu, 25 Jun 2026 14:44:15 +0100 Subject: [PATCH 6/8] refactor(files): echo inherited inputs inside files[] (canonical contract) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- src/models/exec.py | 18 +++++------------- src/services/orchestrator.py | 12 +++++++----- tests/unit/test_librechat_contract.py | 19 +++++++++++++++++++ tests/unit/test_orchestrator.py | 21 ++++++++++----------- 4 files changed, 41 insertions(+), 29 deletions(-) diff --git a/src/models/exec.py b/src/models/exec.py index 19681db..1268528 100644 --- a/src/models/exec.py +++ b/src/models/exec.py @@ -30,9 +30,11 @@ class FileRef(BaseModel): default=False, description=( "True when this entry is an unchanged passthrough of a read-only " - "input the caller already owns (skill/agent bundle), surfaced via " - "ExecResponse.inherited_files so callers can render inputs " - "distinctly from generated outputs." + "input the caller already owns (skill/agent bundle). LibreChat's " + "host (callbacks.js / tools.js) and @librechat/agents " + "(CodeSessionFileSummary) skip inherited entries when creating user " + "download artifacts, re-downloads, and the model-facing summary, so " + "they are never surfaced as generated outputs." ), ) @@ -93,16 +95,6 @@ class ExecResponse(BaseModel): files: list[FileRef] = Field(default_factory=list) stdout: str = "" stderr: str = "" - inherited_files: list[FileRef] = Field( - default_factory=list, - description=( - "Read-only inputs (skill/agent bundles) the caller already owns, " - "echoed as unchanged passthroughs. Kept out of `files` so the " - "current @librechat/agents consumer does not surface them as " - "generated download artifacts; each entry carries `inherited: true` " - "for forward-compatible consumers." - ), - ) # State persistence fields (Python only) has_state: bool = Field( default=False, diff --git a/src/services/orchestrator.py b/src/services/orchestrator.py index efeb2a6..7e80f37 100644 --- a/src/services/orchestrator.py +++ b/src/services/orchestrator.py @@ -790,9 +790,12 @@ def _build_response(self, ctx: ExecutionContext) -> ExecResponse: auto_mounted = sum(1 for f in ctx.mounted_files if f.get("auto_mounted")) if ctx.mounted_files else 0 # Echo read-only inputs (skill/agent bundles the caller already owns) - # as inherited passthroughs. They are deliberately kept out of `files` - # so the current @librechat/agents consumer does not surface them as - # generated download artifacts (it ignores the `inherited` flag). + # inside `files` flagged inherited=True. LibreChat's host + # (callbacks.js / tools.js) and @librechat/agents + # (CodeSessionFileSummary) skip inherited entries when building user + # download artifacts, re-downloads, and the model-facing summary, so + # they are never surfaced as generated outputs. This matches the SaaS + # code-interpreter wire contract. inherited_files = [ FileRef( id=f["file_id"], @@ -806,10 +809,9 @@ def _build_response(self, ctx: ExecutionContext) -> ExecResponse: return ExecResponse( session_id=ctx.session_id, - files=ctx.generated_files or [], + files=(ctx.generated_files or []) + inherited_files, stdout=ctx.stdout, stderr=ctx.stderr, - inherited_files=inherited_files, has_state=has_state, state_size=state_size, state_hash=state_hash, diff --git a/tests/unit/test_librechat_contract.py b/tests/unit/test_librechat_contract.py index 8797d32..e0016e1 100644 --- a/tests/unit/test_librechat_contract.py +++ b/tests/unit/test_librechat_contract.py @@ -102,6 +102,25 @@ def test_resource_id_kind_version_excluded_when_none(self): assert "version" not in dumped +class TestFileRefInherited: + """``inherited`` is emitted inside each files[] entry so LibreChat's host + (callbacks.js / tools.js ``if (file.inherited) continue``) and + @librechat/agents (CodeSessionFileSummary ``file.inherited !== true``) can + skip read-only passthroughs when building user downloads and the model + summary. The flag lives on the file ref, not a separate response field.""" + + def test_inherited_defaults_false(self): + ref = FileRef(id="f", name="out.png", session_id="sess-A") + assert ref.model_dump()["inherited"] is False + + def test_inherited_true_round_trips(self): + ref = FileRef(id="skill-1", name="skillName/SKILL.md", session_id="sess-A", inherited=True) + dumped = ref.model_dump() + assert dumped["inherited"] is True + assert dumped["id"] == "skill-1" + assert dumped["name"] == "skillName/SKILL.md" + + # --------------------------------------------------------------------------- # Endpoint contract — /upload, /upload/batch, /files/{session_id} # --------------------------------------------------------------------------- diff --git a/tests/unit/test_orchestrator.py b/tests/unit/test_orchestrator.py index 29f8e0d..0521b26 100644 --- a/tests/unit/test_orchestrator.py +++ b/tests/unit/test_orchestrator.py @@ -1228,7 +1228,7 @@ def test_build_response_with_state(self, orchestrator, mock_state_service): assert response.state_hash == "abc123" def test_build_response_echoes_readonly_as_inherited(self, orchestrator): - """Read-only mounted inputs are echoed in inherited_files, not files (Gap A).""" + """Read-only mounted inputs are echoed in files[] flagged inherited=True (Gap A).""" request = ExecRequest(code="print('hi')", lang="py") ctx = ExecutionContext( request=request, @@ -1257,17 +1257,16 @@ def test_build_response_echoes_readonly_as_inherited(self, orchestrator): response = orchestrator._build_response(ctx) - assert len(response.inherited_files) == 1 - inherited = response.inherited_files[0] - assert inherited.id == "skill-1" - assert inherited.name == "skillName/SKILL.md" - assert inherited.session_id == "upload-sess" - assert inherited.inherited is True - # Read-only inputs must NOT leak into the generated files list. - assert all(f.name != "skillName/SKILL.md" for f in response.files) + # The read-only input is echoed inside files[] flagged inherited=True. + inherited = [f for f in response.files if f.inherited] + assert len(inherited) == 1 + assert inherited[0].id == "skill-1" + assert inherited[0].name == "skillName/SKILL.md" + assert inherited[0].session_id == "upload-sess" + assert inherited[0].inherited is True def test_build_response_no_inherited_when_no_readonly(self, orchestrator): - """No read-only inputs means an empty inherited_files list.""" + """No read-only inputs means no inherited entries in files[].""" request = ExecRequest(code="print('hi')", lang="py") ctx = ExecutionContext( request=request, @@ -1283,7 +1282,7 @@ def test_build_response_no_inherited_when_no_readonly(self, orchestrator): response = orchestrator._build_response(ctx) - assert response.inherited_files == [] + assert all(f.inherited is False for f in response.files) class TestCleanupExtended: From d3409aa95c6f4038105ef753e62b76f169dabc03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gil=20Assun=C3=A7=C3=A3o?= Date: Thu, 25 Jun 2026 15:22:51 +0100 Subject: [PATCH 7/8] feat(exec): add /exec/programmatic (Programmatic Tool Calling, replay 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. --- src/api/programmatic.py | 96 +++++++++ src/main.py | 4 +- src/models/programmatic.py | 93 +++++++++ src/services/orchestrator.py | 43 +++- src/services/programmatic/__init__.py | 11 ++ src/services/programmatic/constants.py | 84 ++++++++ src/services/programmatic/harness.py | 253 ++++++++++++++++++++++++ src/services/programmatic/sentinel.py | 90 +++++++++ src/services/programmatic/service.py | 216 ++++++++++++++++++++ src/services/programmatic/state.py | 109 ++++++++++ tests/unit/test_librechat_contract.py | 69 +++++++ tests/unit/test_programmatic_harness.py | 129 ++++++++++++ tests/unit/test_programmatic_service.py | 209 ++++++++++++++++++++ 13 files changed, 1404 insertions(+), 2 deletions(-) create mode 100644 src/api/programmatic.py create mode 100644 src/models/programmatic.py create mode 100644 src/services/programmatic/__init__.py create mode 100644 src/services/programmatic/constants.py create mode 100644 src/services/programmatic/harness.py create mode 100644 src/services/programmatic/sentinel.py create mode 100644 src/services/programmatic/service.py create mode 100644 src/services/programmatic/state.py create mode 100644 tests/unit/test_programmatic_harness.py create mode 100644 tests/unit/test_programmatic_service.py diff --git a/src/api/programmatic.py b/src/api/programmatic.py new file mode 100644 index 0000000..9643563 --- /dev/null +++ b/src/api/programmatic.py @@ -0,0 +1,96 @@ +"""Programmatic Tool Calling endpoint compatible with @librechat/agents. + +``POST /exec/programmatic`` runs the stateless replay loop: the model's code +calls tools as async functions; the sandbox replays prior results from an +injected history file and surfaces the first un-cached call, which LibreChat +executes and feeds back via ``continuation_token`` + ``tool_results``. + +Auth + user_id resolution mirror ``/exec`` (JWT.sub > body > User-Id header). +""" + +import structlog +from fastapi import APIRouter, Request +from fastapi.responses import JSONResponse + +from ..dependencies.services import ( + ExecutionServiceDep, + FileServiceDep, + SessionServiceDep, + StateArchivalServiceDep, + StateServiceDep, +) +from ..models.programmatic import ProgrammaticRequest, ProgrammaticResponse +from ..services.orchestrator import ExecutionOrchestrator +from ..services.programmatic import ProgrammaticError, ProgrammaticService +from ..utils.id_generator import generate_request_id + +logger = structlog.get_logger(__name__) +router = APIRouter() + + +@router.post("/exec/programmatic", response_model=ProgrammaticResponse) +async def execute_programmatic( + request: ProgrammaticRequest, + http_request: Request, + session_service: SessionServiceDep, + file_service: FileServiceDep, + execution_service: ExecutionServiceDep, + state_service: StateServiceDep, + state_archival_service: StateArchivalServiceDep, +): + """Run one Programmatic Tool Calling iteration (replay mode). + + The body is either an **initial** request (``code`` + ``tools``) or a + **continuation** (``continuation_token`` + ``tool_results``). Returns + ``tool_call_required`` (with the next tool call) or ``completed``. + """ + request_id = generate_request_id()[:8] + + api_key_hash = getattr(http_request.state, "api_key_hash", None) + is_env_key = getattr(http_request.state, "is_env_key", False) + + # Resolve user_id: JWT.sub (verified) > body > User-Id/X-User-Id header. + jwt_user_id = getattr(http_request.state, "user_id", None) + user_id = jwt_user_id or request.user_id + if not user_id: + user_id = http_request.headers.get("user-id") or http_request.headers.get("x-user-id") + + logger.info( + "Programmatic execution request", + request_id=request_id, + language=request.resolved_language, + is_continuation=request.is_continuation, + tool_count=len(request.tools or []), + user_id=user_id, + ) + + orchestrator = ExecutionOrchestrator( + session_service=session_service, + file_service=file_service, + execution_service=execution_service, + state_service=state_service, + state_archival_service=state_archival_service, + ) + service = ProgrammaticService(orchestrator) + + try: + response = await service.execute( + request, + user_id=user_id, + request_id=request_id, + api_key_hash=api_key_hash, + is_env_key=is_env_key, + ) + except ProgrammaticError as exc: + logger.warning( + "Programmatic request rejected", request_id=request_id, status=exc.status_code, error=exc.message + ) + return JSONResponse(status_code=exc.status_code, content={"error": exc.message}) + + logger.info( + "Programmatic execution response", + request_id=request_id, + status=response.status, + session_id=response.session_id, + ) + return response diff --git a/src/main.py b/src/main.py index 3b4fbd0..a6ca512 100644 --- a/src/main.py +++ b/src/main.py @@ -16,7 +16,7 @@ # Local application imports from ._version import __version__ -from .api import admin, dashboard_metrics, exec, files, health, state +from .api import admin, dashboard_metrics, exec, files, health, programmatic, state from .config import settings from .middleware.metrics import MetricsMiddleware from .middleware.security import RequestLoggingMiddleware, SecurityMiddleware @@ -322,6 +322,8 @@ async def config_info(): app.include_router(exec.router, tags=["exec"]) +app.include_router(programmatic.router, tags=["exec"]) + app.include_router(health.router, tags=["health", "monitoring"]) app.include_router(state.router, tags=["state"]) diff --git a/src/models/programmatic.py b/src/models/programmatic.py new file mode 100644 index 0000000..c9e3c7a --- /dev/null +++ b/src/models/programmatic.py @@ -0,0 +1,93 @@ +"""Pydantic models for the Programmatic Tool Calling endpoint. + +Wire contract mirrors the reference ``code-interpreter`` service +(``ProgrammaticRequestBody`` / ``ProgrammaticResponse``) so the +``@librechat/agents`` PTC client interoperates without changes. +""" + +from typing import Any, Literal + +from pydantic import BaseModel, ConfigDict, Field, model_validator + +from .exec import FileRef, RequestFile + + +class ProgrammaticTool(BaseModel): + """A tool definition the sandbox exposes as an async function.""" + + model_config = ConfigDict(extra="allow") + + name: str + description: str | None = None + parameters: dict[str, Any] | None = None + + +class ToolResultIn(BaseModel): + """A tool result returned by LibreChat for a previously requested call.""" + + call_id: str + result: Any = None + is_error: bool = False + error_message: str | None = None + + +class ProgrammaticRequest(BaseModel): + """Request body for ``POST /exec/programmatic``. + + Two shapes flow through this model: + * **initial** — ``code`` + ``tools`` (+ optional ``session_id`` / + ``files`` / ``timeout`` / ``language``). + * **continuation** — ``continuation_token`` + ``tool_results``. + """ + + model_config = ConfigDict(populate_by_name=True) + + code: str | None = None + tools: list[ProgrammaticTool] | None = None + session_id: str | None = None + timeout: int | None = Field(default=None, description="Per-iteration sandbox timeout (milliseconds)") + continuation_token: str | None = None + tool_results: list[ToolResultIn] | None = None + user_id: str | None = None + files: list[RequestFile] = Field(default_factory=list) + language: Literal["python", "bash"] | None = None + # Back-compat alias: the agents bash PTC client sends ``lang``. + lang: Literal["python", "bash"] | None = None + + @model_validator(mode="after") + def _normalize_language(self) -> "ProgrammaticRequest": + # ``language`` wins when both are present (mirrors the reference router). + if self.language is None and self.lang is not None: + self.language = self.lang + return self + + @property + def resolved_language(self) -> str: + return self.language or "python" + + @property + def is_continuation(self) -> bool: + return bool(self.continuation_token) and self.tool_results is not None + + +class ProgrammaticToolCall(BaseModel): + """A pending tool call the sandbox requested (sent to LibreChat).""" + + id: str + name: str + input: dict[str, Any] = Field(default_factory=dict) + + +class ProgrammaticResponse(BaseModel): + """Response body for ``POST /exec/programmatic``.""" + + status: Literal["tool_call_required", "completed", "error"] + continuation_token: str | None = None + tool_calls: list[ProgrammaticToolCall] | None = None + partial_stdout: str | None = None + partial_stderr: str | None = None + stdout: str | None = None + stderr: str | None = None + files: list[FileRef] | None = None + session_id: str | None = None + error: str | None = None diff --git a/src/services/orchestrator.py b/src/services/orchestrator.py index 7e80f37..6f169ca 100644 --- a/src/services/orchestrator.py +++ b/src/services/orchestrator.py @@ -74,6 +74,10 @@ class ExecutionContext: is_env_key: bool = False container_source: str = "pool_hit" # pool_hit, pool_miss, pool_disabled execution_start_time: datetime | None = None + # Programmatic Tool Calling: extra inline files to mount (name/content/read_only) + # and an explicit override of Python state capture (PTC runs are stateless). + extra_files: list[dict[str, Any]] | None = None + capture_state_override: bool | None = None class ExecutionOrchestrator: @@ -109,6 +113,8 @@ async def execute( request_id: str = "", api_key_hash: str | None = None, is_env_key: bool = False, + extra_files: list[dict[str, Any]] | None = None, + capture_state_override: bool | None = None, ) -> ExecResponse: """Execute code and return LibreChat-compatible response. @@ -117,6 +123,11 @@ async def execute( request_id: Optional request ID for logging api_key_hash: Hash of the API key for metrics tracking is_env_key: True if using env var API key (no rate limiting) + extra_files: Optional inline files to mount in addition to + request.files (each ``{"filename", "content", "read_only"?}``). + Used by Programmatic Tool Calling to inject the replay history. + capture_state_override: When set, forces Python state capture on/off + (PTC runs are stateless and pass ``False``). Returns: ExecResponse: LibreChat-compatible response with session_id, files, stdout, stderr @@ -127,6 +138,8 @@ async def execute( api_key_hash=api_key_hash, is_env_key=is_env_key, execution_start_time=datetime.now(), + extra_files=extra_files, + capture_state_override=capture_state_override, ) try: @@ -551,6 +564,31 @@ async def _is_file_ref_authorized(file_session_id: str) -> bool: explicit=len(mounted) - auto_mount_count, ) + # Append caller-supplied inline files (Programmatic Tool Calling injects + # the replay history here). They carry content directly and are mounted + # read-only so they never surface as generated outputs. + if ctx.extra_files: + for extra in ctx.extra_files: + filename = extra["filename"] + if filename in mounted_filenames: + continue + content = extra["content"] + if isinstance(content, str): + content = content.encode("utf-8") + mounted.append( + { + "file_id": filename, + "filename": filename, + "path": f"/mnt/data/{filename}", + "size": len(content), + "session_id": ctx.session_id, + "content": content, + "auto_mounted": False, + "read_only": extra.get("read_only", True), + } + ) + mounted_filenames.add(filename) + return mounted async def _load_state(self, ctx: ExecutionContext) -> None: @@ -649,8 +687,11 @@ async def _execute_code(self, ctx: ExecutionContext) -> Any: timeout=settings.max_execution_time, ) - # Determine if we should use state persistence (Python only) + # Determine if we should use state persistence (Python only). + # PTC runs are stateless and pass capture_state_override=False. use_state = settings.state_persistence_enabled and ctx.request.lang == "py" + if ctx.capture_state_override is not None: + use_state = ctx.capture_state_override # execute_code returns (execution, container, new_state, state_errors, container_source) tuple ( diff --git a/src/services/programmatic/__init__.py b/src/services/programmatic/__init__.py new file mode 100644 index 0000000..5e0014c --- /dev/null +++ b/src/services/programmatic/__init__.py @@ -0,0 +1,11 @@ +"""Programmatic Tool Calling (PTC) replay implementation.""" + +from .service import ProgrammaticError, ProgrammaticService +from .state import ExecutionState, ProgrammaticStateStore + +__all__ = [ + "ProgrammaticError", + "ProgrammaticService", + "ProgrammaticStateStore", + "ExecutionState", +] diff --git a/src/services/programmatic/constants.py b/src/services/programmatic/constants.py new file mode 100644 index 0000000..5191e27 --- /dev/null +++ b/src/services/programmatic/constants.py @@ -0,0 +1,84 @@ +"""Shared constants for Programmatic Tool Calling (PTC). + +PTC lets the model write Python/Bash code that calls LibreChat tools as +async functions. Because KubeCodeRun execution pods have **no network +egress** (NetworkPolicy ``egress: []``), the live-callback bridge used by +hosted code interpreters is impossible here. Instead PTC uses the +**stateless replay model**: each round the sandbox re-runs the user code +from scratch, replaying prior tool results from an injected history file +(``_ptc_history.json``); on the first un-cached tool call it prints a +scoped sentinel block and exits, and the API relays that call to LibreChat +which executes it and continues the loop. + +This mirrors the reference ``code-interpreter`` service so the +``@librechat/agents`` ``ProgrammaticToolCalling`` client (which POSTs to +``/exec/programmatic``) works unchanged. +""" + +import re + +# Relative filename of the replay history fixture injected into the sandbox. +PTC_HISTORY_FILENAME = "_ptc_history.json" + +# Absolute path inside the sandbox (mounted files land under /mnt/data). +PTC_HISTORY_SANDBOX_PATH = f"/mnt/data/{PTC_HISTORY_FILENAME}" + +# Sentinel prefixes the replay preamble emits on stdout to surface pending +# tool calls. The runtime markers embed the execution id so user code cannot +# forge a well-formed sentinel block by printing the raw literals. +PTC_SENTINEL_START_PREFIX = "__PTC_PENDING_V1_START__" +PTC_SENTINEL_END_PREFIX = "__PTC_PENDING_V1_END__" + +# Execution-id charset, enforced before interpolation into generated code. +_EXECUTION_ID_RE = re.compile(r"^[A-Za-z0-9_-]{1,128}$") + +# Guard rails (mirror the reference service caps). +MAX_TOOLS_PER_REQUEST = 128 +MAX_REPLAY_CALLS = 200 +EXECUTION_STATE_TTL_SECONDS = 600 + + +def build_scoped_sentinel(execution_id: str) -> tuple[str, str]: + """Return execution-scoped (start, end) sentinel markers. + + Raises: + ValueError: if ``execution_id`` contains characters outside the + allowed charset (defense against code injection through the + interpolated preamble). + """ + if not _EXECUTION_ID_RE.match(execution_id): + raise ValueError(f'executionId "{execution_id}" contains invalid characters') + return ( + f"{PTC_SENTINEL_START_PREFIX}__{execution_id}", + f"{PTC_SENTINEL_END_PREFIX}__{execution_id}", + ) + + +def is_reserved_ptc_filename(name: str) -> bool: + """True for any filename the caller must not supply. + + A name is reserved when its post-normalization basename is + ``_ptc_history.json`` (it would shadow the injected replay fixture and + silently corrupt replay correctness) or when it escapes the submission + directory via leading ``..``. Normalization folds backslashes to ``/``, + drops empty/``.`` segments and resolves ``..`` by popping. + """ + if not isinstance(name, str) or name == "": + return False + unified = name.replace("\\", "/") + segments: list[str] = [] + escapes = False + for seg in unified.split("/"): + if seg in ("", "."): + continue + if seg == "..": + if not segments: + escapes = True + else: + segments.pop() + continue + segments.append(seg) + if escapes: + return True + basename = segments[-1] if segments else "" + return basename == PTC_HISTORY_FILENAME diff --git a/src/services/programmatic/harness.py b/src/services/programmatic/harness.py new file mode 100644 index 0000000..4d35574 --- /dev/null +++ b/src/services/programmatic/harness.py @@ -0,0 +1,253 @@ +"""Generate the Python replay harness for Programmatic Tool Calling. + +This is a faithful port of the reference ``code-interpreter`` service's +``generateReplayPreamble`` + ``wrapUserCodeInAsync``. The generated +``main.py`` is: + + # tool stubs + history replay + sentinel + + +On each round the sandbox loads ``/mnt/data/_ptc_history.json`` (a map of +``call_001`` -> prior tool result). A tool call that is already in history +returns immediately; the first un-cached call is appended to a pending list, +printed inside execution-scoped sentinel markers, and the process exits 0. +The API parses the sentinel, relays the call to LibreChat, merges the result +into history and re-runs — until the code completes without a new call. +""" + +from typing import Any + +from .constants import PTC_HISTORY_SANDBOX_PATH, build_scoped_sentinel + +_PYTHON_KEYWORDS = { + "False", "None", "True", "and", "as", "assert", "async", "await", + "break", "class", "continue", "def", "del", "elif", "else", "except", + "finally", "for", "from", "global", "if", "import", "in", "is", + "lambda", "nonlocal", "not", "or", "pass", "raise", "return", + "try", "while", "with", "yield", +} # fmt: skip + + +def _normalize_python_function_name(name: str) -> str: + """Normalize a tool name into a valid, non-keyword Python identifier.""" + normalized = "".join("_" if ch in "- \t" else ch for ch in name) + normalized = "".join(ch for ch in normalized if ch.isalnum() or ch == "_") + if normalized and normalized[0].isdigit(): + normalized = "_" + normalized + if normalized in _PYTHON_KEYWORDS: + normalized = normalized + "_tool" + return normalized + + +def _json_schema_to_python_type(schema: dict[str, Any]) -> str: + schema_type = schema.get("type") + if not schema_type: + return "Any" + if schema_type == "string": + return "str" + if schema_type == "integer": + return "int" + if schema_type in ("number", "float"): + return "float" + if schema_type == "boolean": + return "bool" + if schema_type == "array": + items = schema.get("items") + item_type = _json_schema_to_python_type(items) if isinstance(items, dict) else "Any" + return f"List[{item_type}]" + if schema_type == "object": + return "Dict[str, Any]" + if schema_type == "null": + return "None" + return "Any" + + +def _sorted_property_names(names: list[str], required: list[str]) -> list[str]: + required_set = set(required) + return [n for n in names if n in required_set] + [n for n in names if n not in required_set] + + +def _schema_to_params(schema: dict[str, Any] | None) -> str: + properties = (schema or {}).get("properties") + if not properties: + return "" + required = (schema or {}).get("required") or [] + required_set = set(required) + params: list[str] = [] + for name in _sorted_property_names(list(properties.keys()), required): + py_type = _json_schema_to_python_type(properties[name]) + if name in required_set: + params.append(f"{name}: {py_type}") + else: + params.append(f"{name}: Optional[{py_type}] = None") + return ", ".join(params) + + +def _generate_input_dict(schema: dict[str, Any] | None) -> str: + properties = (schema or {}).get("properties") + if not properties: + return "" + return ", ".join(f'"{name}": {name}' for name in properties) + + +def _infer_return_type(description: str | None) -> str: + if not description: + return "Any" + desc = description.lower() + if "returns list" in desc or "returns array" in desc or "list of" in desc: + return "List[Dict[str, Any]]" + if "returns dict" in desc or "returns object" in desc: + return "Dict[str, Any]" + return "Any" + + +def _escape_docstring(text: str) -> str: + # Keep the generated triple-quoted string literal valid. + return text.replace("\\", "\\\\").replace('"""', '\\"\\"\\"') + + +def _generate_docstring(tool: dict[str, Any]) -> str: + doc = tool.get("description") or "No description available." + parameters = tool.get("parameters") or {} + properties = parameters.get("properties") + if properties: + doc += "\n\n Parameters:" + required = parameters.get("required") or [] + required_set = set(required) + for name in _sorted_property_names(list(properties.keys()), required): + prop = properties[name] + kind = "required" if name in required_set else "optional" + desc = prop.get("description") or "No description" + doc += f"\n {name} ({kind}): {desc}" + return _escape_docstring(doc) + + +def _generate_tool_stub(tool: dict[str, Any]) -> str: + name = tool["name"] + params = _schema_to_params(tool.get("parameters")) + return_type = _infer_return_type(tool.get("description")) + docstring = _generate_docstring(tool) + input_dict = _generate_input_dict(tool.get("parameters")) + py_name = _normalize_python_function_name(name) + name_comment = f" # Original tool name: {name}\n" if py_name != name else "" + return ( + f"async def {py_name}({params}) -> {return_type}:\n" + f' """{docstring}"""\n' + f"{name_comment}" + f" _input = {{{input_dict}}}\n" + " _input = {k: v for k, v in _input.items() if v is not None}\n" + f' return await _execute_tool_internal_async("{name}", _input)\n' + ) + + +# Literal body of the replay preamble (no interpolation — contains braces). +_REPLAY_PREAMBLE_BODY = ''' +def _ptc_load_history() -> Dict[str, Any]: + try: + with open(_PTC_HISTORY_PATH, "r", encoding="utf-8") as _hf: + data = json.load(_hf) + if isinstance(data, dict): + return data + except FileNotFoundError: + return {} + except Exception: + return {} + return {} + +_PTC_HISTORY = _ptc_load_history() + +_TOOL_CALL_COUNTER = 0 +_PTC_PENDING: List[Dict[str, Any]] = [] + +class ToolExecutionError(Exception): + """Raised when a cached tool result is marked as an error.""" + pass + +def _ptc_emit_pending_and_exit() -> None: + try: + sys.stdout.flush() + sys.stderr.flush() + except Exception: + pass + try: + payload = json.dumps({"pending": _PTC_PENDING}) + except Exception as e: + payload = json.dumps({"pending": [], "error": f"serialize_failed: {e}"}) + sys.stdout.write("\\n" + _PTC_SENTINEL_START + "\\n") + sys.stdout.write(payload + "\\n") + sys.stdout.write(_PTC_SENTINEL_END + "\\n") + try: + sys.stdout.flush() + except Exception: + pass + os._exit(0) + +async def _execute_tool_internal_async(tool_name: str, tool_input: Dict[str, Any]) -> Any: + """Replay-aware tool dispatch: consult history, else emit pending and exit.""" + global _TOOL_CALL_COUNTER + _TOOL_CALL_COUNTER += 1 + call_id = f"call_{_TOOL_CALL_COUNTER:03d}" + + entry = _PTC_HISTORY.get(call_id) + if entry is not None: + if isinstance(entry, dict) and entry.get("is_error"): + raise ToolExecutionError(entry.get("error_message") or "tool execution failed") + if isinstance(entry, dict): + return entry.get("result") + return entry + + _PTC_PENDING.append({ + "call_id": call_id, + "tool_name": tool_name, + "input": tool_input, + }) + _ptc_emit_pending_and_exit() +''' + + +def build_replay_preamble(execution_id: str, tools: list[dict[str, Any]]) -> str: + """Build the replay-mode Python preamble (infrastructure + tool stubs).""" + start, end = build_scoped_sentinel(execution_id) + header = ( + "# ============================================================================\n" + "# PROGRAMMATIC TOOL CALLING INFRASTRUCTURE (replay mode)\n" + "# Auto-generated - do not modify\n" + "# ============================================================================\n\n" + "import json\n" + "import sys\n" + "import os\n" + "import asyncio\n" + "from typing import Any, Dict, List, Optional, Union\n\n" + f'_EXECUTION_ID = "{execution_id}"\n' + f'_PTC_SENTINEL_START = "{start}"\n' + f'_PTC_SENTINEL_END = "{end}"\n' + f'_PTC_HISTORY_PATH = os.environ.get("PTC_HISTORY_PATH") or "{PTC_HISTORY_SANDBOX_PATH}"\n' + ) + stubs = "\n# ============================================================================\n" + stubs += "# TOOL DEFINITIONS\n" + stubs += "# ============================================================================\n\n" + for tool in tools: + stubs += _generate_tool_stub(tool) + "\n\n" + return header + _REPLAY_PREAMBLE_BODY + "\n" + stubs + + +def wrap_user_code_in_async(user_code: str) -> str: + """Wrap user code in an async main so top-level await works.""" + wrapped = ( + "# ============================================================================\n" + "# USER CODE BEGINS BELOW\n" + "# ============================================================================\n\n" + "async def __user_main__():\n" + ' """Auto-generated wrapper for user code to support top-level await"""\n' + ) + for line in user_code.split("\n"): + wrapped += "\n" if line.strip() == "" else " " + line + "\n" + wrapped += '\nif __name__ == "__main__":\n' + wrapped += " import asyncio\n" + wrapped += " asyncio.run(__user_main__())\n" + return wrapped + + +def build_python_code(execution_id: str, tools: list[dict[str, Any]], user_code: str) -> str: + """Assemble the full ``main.py`` (preamble + async-wrapped user code).""" + return build_replay_preamble(execution_id, tools) + wrap_user_code_in_async(user_code) diff --git a/src/services/programmatic/sentinel.py b/src/services/programmatic/sentinel.py new file mode 100644 index 0000000..a9314a7 --- /dev/null +++ b/src/services/programmatic/sentinel.py @@ -0,0 +1,90 @@ +"""Parse the replay sentinel block the PTC harness emits on stdout. + +Mirrors the reference ``extractPendingFromStdout``: locate the last line +exactly equal to the execution-scoped start marker, the matching end marker +after it, parse the JSON payload between them, and return the user-visible +stdout with that one sentinel block removed. +""" + +import json +from typing import Any + +from .constants import build_scoped_sentinel + + +def extract_pending_from_stdout( + stdout: str | None, + execution_id: str, +) -> tuple[str, list[dict[str, Any]] | None]: + """Return ``(clean_stdout, pending)``. + + ``pending`` is ``None`` when no sentinel block is present (the run either + completed or errored). Otherwise it is the list of pending tool calls, + each ``{"call_id", "tool_name", "input"}``, and ``clean_stdout`` has the + sentinel block stripped so user prints can surface as partial output. + """ + if stdout is None: + return "", None + + start_marker, end_marker = build_scoped_sentinel(execution_id) + lines = stdout.split("\n") + # Absolute char offset where each line starts (line i is preceded by i newlines). + line_start_offsets = [0] + for i in range(len(lines) - 1): + line_start_offsets.append(line_start_offsets[i] + len(lines[i]) + 1) + + start_idx = _find_last_line(lines, start_marker, 0) + if start_idx is None: + return stdout, None + end_idx = _find_last_line(lines, end_marker, start_idx + 1) + if end_idx is None: + return stdout, None + + raw_payload = "\n".join(lines[start_idx + 1 : end_idx]).strip() + try: + parsed = json.loads(raw_payload) + except (json.JSONDecodeError, ValueError): + return stdout, None + if not isinstance(parsed, dict): + return stdout, None + pending_field = parsed.get("pending") + if not isinstance(pending_field, list): + return stdout, None + + pending: list[dict[str, Any]] = [] + for entry in pending_field: + if ( + isinstance(entry, dict) + and isinstance(entry.get("call_id"), str) + and isinstance(entry.get("tool_name"), str) + ): + raw_input = entry.get("input") + pending.append( + { + "call_id": entry["call_id"], + "tool_name": entry["tool_name"], + "input": raw_input if isinstance(raw_input, dict) else {}, + } + ) + + # Rebuild user stdout byte-accurately: everything before the start marker + # (dropping the single runtime-inserted "\n" the harness emits right before + # it) concatenated with everything after the end marker's newline. No other + # bytes are touched, so exact-format user output is preserved. + start_offset = line_start_offsets[start_idx] + raw_head = stdout[:start_offset] + head = raw_head[:-1] if raw_head.endswith("\n") else raw_head + if end_idx + 1 < len(line_start_offsets): + tail_start = line_start_offsets[end_idx + 1] + else: + tail_start = len(stdout) + tail = stdout[tail_start:] + cleaned = head + tail + return cleaned, pending + + +def _find_last_line(lines: list[str], marker: str, search_from: int) -> int | None: + for i in range(len(lines) - 1, search_from - 1, -1): + if lines[i].strip() == marker: + return i + return None diff --git a/src/services/programmatic/service.py b/src/services/programmatic/service.py new file mode 100644 index 0000000..89780a5 --- /dev/null +++ b/src/services/programmatic/service.py @@ -0,0 +1,216 @@ +"""Programmatic Tool Calling orchestration (replay loop). + +Ties the harness, sentinel parser and Redis state store to KubeCodeRun's +existing execution path. Each ``execute`` call runs one sandbox iteration via +``ExecutionOrchestrator`` (injecting the replay history as an inline file) and +maps the result onto the PTC wire contract: + + * sentinel present -> ``tool_call_required`` (+ continuation_token) + * no sentinel -> ``completed`` (stdout / stderr / files) + +Bash PTC is not yet supported; such requests are rejected with a clear 400. +""" + +import json +import uuid + +import structlog + +from ...models.exec import ExecRequest +from ...models.programmatic import ( + ProgrammaticRequest, + ProgrammaticResponse, + ProgrammaticToolCall, +) +from ..orchestrator import ExecutionOrchestrator +from .constants import ( + MAX_REPLAY_CALLS, + MAX_TOOLS_PER_REQUEST, + PTC_HISTORY_FILENAME, + is_reserved_ptc_filename, +) +from .harness import build_python_code +from .sentinel import extract_pending_from_stdout +from .state import ExecutionState, ProgrammaticStateStore + +logger = structlog.get_logger(__name__) + + +class ProgrammaticError(Exception): + """Raised for client-facing PTC failures (mapped to an HTTP status).""" + + def __init__(self, status_code: int, message: str): + super().__init__(message) + self.status_code = status_code + self.message = message + + +class ProgrammaticService: + """Drives the stateless replay loop for ``POST /exec/programmatic``.""" + + def __init__( + self, + orchestrator: ExecutionOrchestrator, + state_store: ProgrammaticStateStore | None = None, + ): + self.orchestrator = orchestrator + self.state = state_store or ProgrammaticStateStore() + + async def execute( + self, + request: ProgrammaticRequest, + user_id: str | None = None, + request_id: str = "", + api_key_hash: str | None = None, + is_env_key: bool = False, + ) -> ProgrammaticResponse: + if request.is_continuation: + return await self._continue(request, user_id, request_id, api_key_hash, is_env_key) + return await self._initial(request, user_id, request_id, api_key_hash, is_env_key) + + async def _initial( + self, + request: ProgrammaticRequest, + user_id: str | None, + request_id: str, + api_key_hash: str | None, + is_env_key: bool, + ) -> ProgrammaticResponse: + if not request.code: + raise ProgrammaticError(400, "Missing required field: code") + if not request.tools: + raise ProgrammaticError(400, "Missing required field: tools (must be a non-empty array)") + if len(request.tools) > MAX_TOOLS_PER_REQUEST: + raise ProgrammaticError( + 400, f"Too many tools provided ({len(request.tools)}). Maximum is {MAX_TOOLS_PER_REQUEST}." + ) + if request.resolved_language == "bash": + raise ProgrammaticError(400, "bash programmatic tool calling is not supported by this server") + if request.resolved_language != "python": + raise ProgrammaticError(400, f'Unsupported language "{request.resolved_language}"') + for f in request.files: + if f.name and is_reserved_ptc_filename(f.name): + raise ProgrammaticError( + 400, f'files[].name "{f.name}" is reserved for PTC runtime and cannot be supplied by callers' + ) + + state = ExecutionState( + execution_id=uuid.uuid4().hex, + session_id=request.session_id or "", + language="python", + code=request.code, + tools=request.tools, + files=request.files, + timeout=request.timeout, + ) + return await self._run_and_respond(state, user_id, request_id, api_key_hash, is_env_key) + + async def _continue( + self, + request: ProgrammaticRequest, + user_id: str | None, + request_id: str, + api_key_hash: str | None, + is_env_key: bool, + ) -> ProgrammaticResponse: + execution_id = request.continuation_token or "" + state = await self.state.load(execution_id) + if state is None: + raise ProgrammaticError(404, "Execution not found or expired") + + self.state.merge_tool_results(state, request.tool_results or []) + if state.call_count > MAX_REPLAY_CALLS: + await self.state.delete(execution_id) + raise ProgrammaticError(400, f"Exceeded maximum tool calls ({MAX_REPLAY_CALLS})") + + return await self._run_and_respond(state, user_id, request_id, api_key_hash, is_env_key) + + async def _run_and_respond( + self, + state: ExecutionState, + user_id: str | None, + request_id: str, + api_key_hash: str | None, + is_env_key: bool, + ) -> ProgrammaticResponse: + tools = [t.model_dump() for t in state.tools] + harness_code = build_python_code(state.execution_id, tools, state.code) + history_json = json.dumps(state.history) + extra_files = [{"filename": PTC_HISTORY_FILENAME, "content": history_json, "read_only": True}] + + exec_request = ExecRequest( + code=harness_code, + lang="py", + session_id=state.session_id or None, + user_id=user_id, + files=state.files, + ) + resp = await self.orchestrator.execute( + exec_request, + request_id=request_id, + api_key_hash=api_key_hash, + is_env_key=is_env_key, + extra_files=extra_files, + capture_state_override=False, + ) + # Reuse the resolved session across rounds so /mnt/data persists. + state.session_id = resp.session_id + + clean_stdout, pending = extract_pending_from_stdout(resp.stdout, state.execution_id) + + if pending is not None: + return await self._respond_tool_calls(state, pending, clean_stdout, resp.stderr) + + # No new tool call: the run completed. + await self.state.delete(state.execution_id) + files = [f for f in (resp.files or []) if f.name != PTC_HISTORY_FILENAME] + return ProgrammaticResponse( + status="completed", + stdout=clean_stdout, + stderr=resp.stderr, + files=files, + session_id=state.session_id, + ) + + async def _respond_tool_calls( + self, + state: ExecutionState, + pending: list[dict], + clean_stdout: str, + stderr: str, + ) -> ProgrammaticResponse: + if not pending: + await self.state.delete(state.execution_id) + return ProgrammaticResponse( + status="error", + error="sandbox emitted an empty pending tool call block", + stdout=clean_stdout, + stderr=stderr, + session_id=state.session_id, + ) + + registered = {t.name for t in state.tools} + unregistered = next((p for p in pending if p["tool_name"] not in registered), None) + if unregistered is not None: + await self.state.delete(state.execution_id) + return ProgrammaticResponse( + status="error", + error=f"Sandbox requested an unregistered tool: {unregistered['tool_name']}", + stdout=clean_stdout, + stderr=stderr, + session_id=state.session_id, + ) + + for p in pending: + if p["call_id"] not in state.emitted_call_ids: + state.emitted_call_ids.append(p["call_id"]) + await self.state.save(state) + + return ProgrammaticResponse( + status="tool_call_required", + continuation_token=state.execution_id, + tool_calls=[ProgrammaticToolCall(id=p["call_id"], name=p["tool_name"], input=p["input"]) for p in pending], + partial_stdout=clean_stdout or None, + partial_stderr=stderr or None, + session_id=state.session_id, + ) diff --git a/src/services/programmatic/state.py b/src/services/programmatic/state.py new file mode 100644 index 0000000..7993dee --- /dev/null +++ b/src/services/programmatic/state.py @@ -0,0 +1,109 @@ +"""Redis-backed continuation state for Programmatic Tool Calling. + +Replay mode persists the full request (``code`` + ``tools`` + ``files``) plus +the accumulated tool-result ``history`` keyed by ``execution_id`` so each +continuation can re-run the sandbox without the client re-sending it. The +``continuation_token`` handed back to LibreChat is the ``execution_id``. +""" + +from __future__ import annotations + +import time +from typing import Any + +import redis.asyncio as redis +import structlog +from pydantic import BaseModel, Field + +from ...models.exec import RequestFile +from ...models.programmatic import ProgrammaticTool, ToolResultIn +from .constants import EXECUTION_STATE_TTL_SECONDS + +logger = structlog.get_logger(__name__) + +_KEY_PREFIX = "ptc:state:" + + +class ExecutionState(BaseModel): + """Persisted state for one PTC execution across replay round-trips.""" + + execution_id: str + session_id: str + language: str = "python" + code: str + tools: list[ProgrammaticTool] = Field(default_factory=list) + files: list[RequestFile] = Field(default_factory=list) + timeout: int | None = None + call_count: int = 0 + # call_id -> {"result", "is_error", "error_message"} + history: dict[str, dict[str, Any]] = Field(default_factory=dict) + # call_ids the server has handed to the client (guards against forged ids). + emitted_call_ids: list[str] = Field(default_factory=list) + created_at: float = Field(default_factory=time.time) + last_activity: float = Field(default_factory=time.time) + + +class ProgrammaticStateStore: + """Persists :class:`ExecutionState` in Redis with a bounded TTL.""" + + def __init__(self, redis_client: redis.Redis | None = None): + self._redis = redis_client + + @property + def redis(self) -> redis.Redis: + if self._redis is None: + from ...core.pool import redis_pool + + self._redis = redis_pool.get_client() + return self._redis + + @staticmethod + def _key(execution_id: str) -> str: + return f"{_KEY_PREFIX}{execution_id}" + + async def save(self, state: ExecutionState) -> None: + state.last_activity = time.time() + await self.redis.set( + self._key(state.execution_id), + state.model_dump_json(), + ex=EXECUTION_STATE_TTL_SECONDS, + ) + + async def load(self, execution_id: str) -> ExecutionState | None: + raw = await self.redis.get(self._key(execution_id)) + if raw is None: + return None + if isinstance(raw, bytes): + raw = raw.decode("utf-8") + try: + return ExecutionState.model_validate_json(raw) + except Exception as exc: + logger.warning("Failed to deserialize PTC execution state", execution_id=execution_id, error=str(exc)) + return None + + async def delete(self, execution_id: str) -> None: + await self.redis.delete(self._key(execution_id)) + + @staticmethod + def merge_tool_results(state: ExecutionState, tool_results: list[ToolResultIn]) -> None: + """Merge tool results into history (idempotent by ``call_id``). + + Only results for call_ids the server actually issued are accepted; a + forged id would otherwise be served as a cache hit on the next sandbox + run, silently skipping the real tool call the user code makes. + """ + issued = set(state.emitted_call_ids) + for result in tool_results: + if result.call_id not in issued: + logger.warning( + "Ignoring tool_result for un-issued call_id", + execution_id=state.execution_id, + call_id=result.call_id, + ) + continue + state.history[result.call_id] = { + "result": result.result, + "is_error": result.is_error, + "error_message": result.error_message, + } + state.call_count = len(state.history) diff --git a/tests/unit/test_librechat_contract.py b/tests/unit/test_librechat_contract.py index e0016e1..405d4df 100644 --- a/tests/unit/test_librechat_contract.py +++ b/tests/unit/test_librechat_contract.py @@ -654,3 +654,72 @@ async def test_returns_last_activity_for_inactive_session(self): ) assert result["lastModified"] == "2026-03-15T10:30:00.000Z" + + +class TestProgrammaticToolCallingContract: + """Wire contract for ``POST /exec/programmatic`` (@librechat/agents PTC). + + The agents ``ProgrammaticToolCalling`` client POSTs ``{code, tools, ...}`` + (initial) or ``{continuation_token, tool_results}`` (continuation) and reads + ``{status, tool_calls:[{id,name,input}], continuation_token}`` / + ``{status:'completed', stdout, stderr, files}``. + """ + + def test_language_lang_alias_normalizes(self): + from src.models.programmatic import ProgrammaticRequest + + # bash client sends `lang`; canonical field is `language` + assert ProgrammaticRequest(code="x", lang="bash").resolved_language == "bash" + # `language` wins when both present + assert ProgrammaticRequest(code="x", language="python", lang="bash").resolved_language == "python" + # default is python + assert ProgrammaticRequest(code="x").resolved_language == "python" + + def test_continuation_detection(self): + from src.models.programmatic import ProgrammaticRequest, ToolResultIn + + initial = ProgrammaticRequest(code="x", tools=[]) + assert initial.is_continuation is False + cont = ProgrammaticRequest(continuation_token="tok", tool_results=[ToolResultIn(call_id="call_001", result=1)]) + assert cont.is_continuation is True + + def test_tool_result_shape(self): + from src.models.programmatic import ToolResultIn + + tr = ToolResultIn(call_id="call_001", result={"k": "v"}, is_error=False) + assert tr.call_id == "call_001" + assert tr.result == {"k": "v"} + assert tr.is_error is False + # error result carries a message + err = ToolResultIn(call_id="call_002", result=None, is_error=True, error_message="boom") + assert err.is_error is True + assert err.error_message == "boom" + + def test_tool_call_out_shape(self): + from src.models.programmatic import ProgrammaticToolCall + + tc = ProgrammaticToolCall(id="call_001", name="get_weather", input={"city": "SF"}) + dumped = tc.model_dump() + assert dumped == {"id": "call_001", "name": "get_weather", "input": {"city": "SF"}} + + def test_response_status_values(self): + from src.models.programmatic import ProgrammaticResponse + + assert ProgrammaticResponse(status="tool_call_required").status == "tool_call_required" + assert ProgrammaticResponse(status="completed").status == "completed" + assert ProgrammaticResponse(status="error").status == "error" + + def test_completed_response_carries_files_and_session(self): + from src.models.programmatic import ProgrammaticResponse + + resp = ProgrammaticResponse( + status="completed", + stdout="done", + stderr="", + files=[FileRef(id="f1", name="out.csv", session_id="s1")], + session_id="s1", + ) + dumped = resp.model_dump() + assert dumped["status"] == "completed" + assert dumped["files"][0]["storage_session_id"] == "s1" + assert dumped["session_id"] == "s1" diff --git a/tests/unit/test_programmatic_harness.py b/tests/unit/test_programmatic_harness.py new file mode 100644 index 0000000..26ce392 --- /dev/null +++ b/tests/unit/test_programmatic_harness.py @@ -0,0 +1,129 @@ +"""Unit tests for the Programmatic Tool Calling harness, sentinel, constants.""" + +import json + +import pytest + +from src.services.programmatic.constants import ( + PTC_HISTORY_FILENAME, + build_scoped_sentinel, + is_reserved_ptc_filename, +) +from src.services.programmatic.harness import ( + _normalize_python_function_name, + build_python_code, + build_replay_preamble, + wrap_user_code_in_async, +) +from src.services.programmatic.sentinel import extract_pending_from_stdout + +WEATHER_TOOL = { + "name": "get-weather", + "description": "Returns dict of current weather", + "parameters": { + "type": "object", + "properties": { + "city": {"type": "string", "description": "City name"}, + "units": {"type": "string", "description": "metric or imperial"}, + }, + "required": ["city"], + }, +} + + +class TestHarnessGeneration: + def test_generated_code_is_valid_python(self): + code = build_python_code("exec1", [WEATHER_TOOL], "print(await get_weather(city='SF'))") + compile(code, "main.py", "exec") # raises SyntaxError if invalid + + def test_tool_stub_async_with_required_then_optional_params(self): + preamble = build_replay_preamble("exec1", [WEATHER_TOOL]) + # required param first, optional with default after + assert "async def get_weather(city: str, units: Optional[str] = None)" in preamble + assert 'return await _execute_tool_internal_async("get-weather", _input)' in preamble + + def test_original_tool_name_preserved_in_dispatch(self): + # hyphenated name normalizes to identifier but dispatch keeps original + preamble = build_replay_preamble("exec1", [WEATHER_TOOL]) + assert "async def get_weather(" in preamble + assert "# Original tool name: get-weather" in preamble + + def test_async_wrap_runs_user_main(self): + wrapped = wrap_user_code_in_async("x = 1\nprint(x)") + assert "async def __user_main__():" in wrapped + assert "asyncio.run(__user_main__())" in wrapped + assert " x = 1" in wrapped # indented into the wrapper + + def test_preamble_embeds_scoped_sentinel_and_history_path(self): + preamble = build_replay_preamble("execABC", [WEATHER_TOOL]) + start, end = build_scoped_sentinel("execABC") + assert f'_PTC_SENTINEL_START = "{start}"' in preamble + assert f'_PTC_SENTINEL_END = "{end}"' in preamble + assert f"/mnt/data/{PTC_HISTORY_FILENAME}" in preamble + + @pytest.mark.parametrize( + "raw, expected", + [ + ("get-weather", "get_weather"), + ("my tool", "my_tool"), + ("class", "class_tool"), # python keyword gets suffix + ("123tool", "_123tool"), # leading digit + ("a.b/c", "abc"), # strip invalid chars + ], + ) + def test_normalize_python_function_name(self, raw, expected): + assert _normalize_python_function_name(raw) == expected + + +class TestSentinelExtraction: + def test_no_sentinel_returns_none(self): + clean, pending = extract_pending_from_stdout("plain output\n", "e1") + assert pending is None + assert clean == "plain output\n" + + def test_extracts_single_pending_call(self): + start, end = build_scoped_sentinel("e1") + body = json.dumps({"pending": [{"call_id": "call_001", "tool_name": "get-weather", "input": {"city": "SF"}}]}) + stdout = "before\n" + "\n" + start + "\n" + body + "\n" + end + "\n" + clean, pending = extract_pending_from_stdout(stdout, "e1") + assert clean == "before\n" + assert pending == [{"call_id": "call_001", "tool_name": "get-weather", "input": {"city": "SF"}}] + + def test_user_output_without_trailing_newline_preserved(self): + start, end = build_scoped_sentinel("e1") + body = json.dumps({"pending": [{"call_id": "call_001", "tool_name": "t", "input": {}}]}) + stdout = "hello" + "\n" + start + "\n" + body + "\n" + end + "\n" + clean, _ = extract_pending_from_stdout(stdout, "e1") + assert clean == "hello" + + def test_scoped_to_execution_id(self): + start, end = build_scoped_sentinel("e1") + body = json.dumps({"pending": [{"call_id": "call_001", "tool_name": "t", "input": {}}]}) + stdout = start + "\n" + body + "\n" + end + "\n" + # parsing under a different execution id must not match + clean, pending = extract_pending_from_stdout(stdout, "OTHER") + assert pending is None + assert clean == stdout + + def test_malformed_json_payload_returns_none(self): + start, end = build_scoped_sentinel("e1") + stdout = start + "\nnot-json\n" + end + "\n" + clean, pending = extract_pending_from_stdout(stdout, "e1") + assert pending is None + + +class TestReservedFilenames: + @pytest.mark.parametrize( + "name", + ["_ptc_history.json", "sub/../_ptc_history.json", "../escape.txt", "..\\etc\\passwd"], + ) + def test_reserved_true(self, name): + assert is_reserved_ptc_filename(name) is True + + @pytest.mark.parametrize("name", ["data.csv", "_ptc_data.csv", "notes_ptc_history.json", ""]) + def test_reserved_false(self, name): + assert is_reserved_ptc_filename(name) is False + + def test_invalid_execution_id_rejected(self): + with pytest.raises(ValueError): + build_scoped_sentinel("bad id with spaces") diff --git a/tests/unit/test_programmatic_service.py b/tests/unit/test_programmatic_service.py new file mode 100644 index 0000000..1eafbf9 --- /dev/null +++ b/tests/unit/test_programmatic_service.py @@ -0,0 +1,209 @@ +"""Round-trip tests for ProgrammaticService (replay loop) with a fake sandbox.""" + +import json +import re + +import pytest + +from src.models.exec import ExecResponse, FileRef +from src.models.programmatic import ProgrammaticRequest, ProgrammaticTool, ToolResultIn +from src.services.programmatic.constants import build_scoped_sentinel +from src.services.programmatic.service import ProgrammaticError, ProgrammaticService +from src.services.programmatic.state import ExecutionState, ProgrammaticStateStore + +WEATHER_TOOL = { + "name": "get_weather", + "description": "Returns weather", + "parameters": {"type": "object", "properties": {"city": {"type": "string"}}, "required": ["city"]}, +} + +_EXEC_ID_RE = re.compile(r'_EXECUTION_ID = "([^"]+)"') + + +class FakeStateStore(ProgrammaticStateStore): + """In-memory replacement for the Redis-backed store (JSON round-trips like Redis).""" + + def __init__(self): + self._mem: dict[str, str] = {} + + async def save(self, state: ExecutionState) -> None: + self._mem[state.execution_id] = state.model_dump_json() + + async def load(self, execution_id: str) -> ExecutionState | None: + raw = self._mem.get(execution_id) + return ExecutionState.model_validate_json(raw) if raw else None + + async def delete(self, execution_id: str) -> None: + self._mem.pop(execution_id, None) + + +def _sentinel_stdout(execution_id: str, pending: list[dict], user_out: str = "") -> str: + start, end = build_scoped_sentinel(execution_id) + body = json.dumps({"pending": pending}) + return user_out + "\n" + start + "\n" + body + "\n" + end + "\n" + + +class FakeOrchestrator: + """Simulates the sandbox replay: emits one tool call until its result is in history.""" + + def __init__(self, pending_factory=None, completed_stdout="weather is sunny", completed_files=None): + self.calls: list[dict] = [] + self._pending_factory = pending_factory or ( + lambda: [{"call_id": "call_001", "tool_name": "get_weather", "input": {"city": "SF"}}] + ) + self._completed_stdout = completed_stdout + self._completed_files = completed_files or [] + + async def execute( + self, request, request_id="", api_key_hash=None, is_env_key=False, extra_files=None, capture_state_override=None + ): + self.calls.append({"code": request.code, "session_id": request.session_id, "extra_files": extra_files}) + execution_id = _EXEC_ID_RE.search(request.code).group(1) + history = json.loads(extra_files[0]["content"]) if extra_files else {} + pending = self._pending_factory() + unresolved = [p for p in pending if p["call_id"] not in history] + if unresolved: + stdout = _sentinel_stdout(execution_id, unresolved[:1]) + else: + stdout = self._completed_stdout + return ExecResponse( + session_id="sess-ptc", + files=self._completed_files, + stdout=stdout, + stderr="", + ) + + +@pytest.fixture +def service(): + return ProgrammaticService(FakeOrchestrator(), state_store=FakeStateStore()) + + +def _initial(code="print(await get_weather(city='SF'))"): + return ProgrammaticRequest(code=code, tools=[ProgrammaticTool(**WEATHER_TOOL)]) + + +class TestReplayLoop: + @pytest.mark.asyncio + async def test_initial_returns_tool_call_required(self, service): + resp = await service.execute(_initial()) + assert resp.status == "tool_call_required" + assert resp.continuation_token + assert len(resp.tool_calls) == 1 + tc = resp.tool_calls[0] + assert tc.id == "call_001" + assert tc.name == "get_weather" + assert tc.input == {"city": "SF"} + assert resp.session_id == "sess-ptc" + + @pytest.mark.asyncio + async def test_continuation_completes(self, service): + first = await service.execute(_initial()) + cont = ProgrammaticRequest( + continuation_token=first.continuation_token, + tool_results=[ToolResultIn(call_id="call_001", result={"temp": 21})], + ) + resp = await service.execute(cont) + assert resp.status == "completed" + assert resp.stdout == "weather is sunny" + assert resp.continuation_token is None + + @pytest.mark.asyncio + async def test_history_threaded_to_sandbox_on_continuation(self, service): + first = await service.execute(_initial()) + cont = ProgrammaticRequest( + continuation_token=first.continuation_token, + tool_results=[ToolResultIn(call_id="call_001", result={"temp": 21})], + ) + await service.execute(cont) + # second sandbox call must have received the merged history file + history = json.loads(service.orchestrator.calls[1]["extra_files"][0]["content"]) + assert history["call_001"]["result"] == {"temp": 21} + + @pytest.mark.asyncio + async def test_completed_filters_history_file_from_files(self): + orch = FakeOrchestrator( + completed_files=[FileRef(id="f1", name="chart.png"), FileRef(id="h", name="_ptc_history.json")] + ) + svc = ProgrammaticService(orch, state_store=FakeStateStore()) + first = await svc.execute(_initial()) + resp = await svc.execute( + ProgrammaticRequest( + continuation_token=first.continuation_token, + tool_results=[ToolResultIn(call_id="call_001", result=1)], + ) + ) + names = {f.name for f in resp.files} + assert "chart.png" in names + assert "_ptc_history.json" not in names + + +class TestValidation: + @pytest.mark.asyncio + async def test_missing_code_rejected(self, service): + with pytest.raises(ProgrammaticError) as exc: + await service.execute(ProgrammaticRequest(tools=[ProgrammaticTool(**WEATHER_TOOL)])) + assert exc.value.status_code == 400 + + @pytest.mark.asyncio + async def test_missing_tools_rejected(self, service): + with pytest.raises(ProgrammaticError) as exc: + await service.execute(ProgrammaticRequest(code="print(1)")) + assert exc.value.status_code == 400 + + @pytest.mark.asyncio + async def test_bash_rejected(self, service): + with pytest.raises(ProgrammaticError) as exc: + await service.execute( + ProgrammaticRequest(code="x", tools=[ProgrammaticTool(**WEATHER_TOOL)], language="bash") + ) + assert exc.value.status_code == 400 + + @pytest.mark.asyncio + async def test_reserved_filename_rejected(self, service): + from src.models.exec import RequestFile + + req = ProgrammaticRequest( + code="print(1)", + tools=[ProgrammaticTool(**WEATHER_TOOL)], + files=[RequestFile(id="x", session_id="s", name="_ptc_history.json")], + ) + with pytest.raises(ProgrammaticError) as exc: + await service.execute(req) + assert exc.value.status_code == 400 + + @pytest.mark.asyncio + async def test_unknown_continuation_token(self, service): + with pytest.raises(ProgrammaticError) as exc: + await service.execute( + ProgrammaticRequest( + continuation_token="does-not-exist", tool_results=[ToolResultIn(call_id="call_001", result=1)] + ) + ) + assert exc.value.status_code == 404 + + @pytest.mark.asyncio + async def test_unregistered_tool_call_errors(self): + orch = FakeOrchestrator( + pending_factory=lambda: [{"call_id": "call_001", "tool_name": "rogue_tool", "input": {}}] + ) + svc = ProgrammaticService(orch, state_store=FakeStateStore()) + resp = await svc.execute(_initial()) + assert resp.status == "error" + assert "rogue_tool" in resp.error + + @pytest.mark.asyncio + async def test_forged_call_id_ignored_in_history(self, service): + first = await service.execute(_initial()) + # client returns a result for a call_id the server never issued + cont = ProgrammaticRequest( + continuation_token=first.continuation_token, + tool_results=[ + ToolResultIn(call_id="call_999", result="forged"), + ToolResultIn(call_id="call_001", result="legit"), + ], + ) + await service.execute(cont) + history = json.loads(service.orchestrator.calls[1]["extra_files"][0]["content"]) + assert "call_999" not in history + assert history["call_001"]["result"] == "legit" From e6533a48ec1dba71d0dd73acd76afa9ed205de69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gil=20Assun=C3=A7=C3=A3o?= Date: Fri, 26 Jun 2026 15:38:41 +0100 Subject: [PATCH 8/8] fix(programmatic): address PR review (exact sentinel match, tool-name validation) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- src/services/programmatic/harness.py | 10 ++++++--- src/services/programmatic/sentinel.py | 7 +++++- src/services/programmatic/service.py | 30 ++++++++++++++++++++++++- tests/unit/test_programmatic_harness.py | 22 ++++++++++++++++-- tests/unit/test_programmatic_service.py | 18 +++++++++++++++ 5 files changed, 80 insertions(+), 7 deletions(-) diff --git a/src/services/programmatic/harness.py b/src/services/programmatic/harness.py index 4d35574..d77d7c5 100644 --- a/src/services/programmatic/harness.py +++ b/src/services/programmatic/harness.py @@ -28,8 +28,12 @@ } # fmt: skip -def _normalize_python_function_name(name: str) -> str: - """Normalize a tool name into a valid, non-keyword Python identifier.""" +def normalize_python_function_name(name: str) -> str: + """Normalize a tool name into a valid, non-keyword Python identifier. + + Returns an empty string when ``name`` contains no identifier-safe + characters (callers should reject such tool names before generation). + """ normalized = "".join("_" if ch in "- \t" else ch for ch in name) normalized = "".join(ch for ch in normalized if ch.isalnum() or ch == "_") if normalized and normalized[0].isdigit(): @@ -128,7 +132,7 @@ def _generate_tool_stub(tool: dict[str, Any]) -> str: return_type = _infer_return_type(tool.get("description")) docstring = _generate_docstring(tool) input_dict = _generate_input_dict(tool.get("parameters")) - py_name = _normalize_python_function_name(name) + py_name = normalize_python_function_name(name) name_comment = f" # Original tool name: {name}\n" if py_name != name else "" return ( f"async def {py_name}({params}) -> {return_type}:\n" diff --git a/src/services/programmatic/sentinel.py b/src/services/programmatic/sentinel.py index a9314a7..a02cc2a 100644 --- a/src/services/programmatic/sentinel.py +++ b/src/services/programmatic/sentinel.py @@ -84,7 +84,12 @@ def extract_pending_from_stdout( def _find_last_line(lines: list[str], marker: str, search_from: int) -> int | None: + # Match the sentinel line exactly. The harness emits the marker on its own + # line, so we tolerate only a trailing carriage return (CRLF-captured + # stdout). We deliberately do NOT strip surrounding whitespace: that would + # let user output which merely contains the marker padded by spaces be + # mistaken for a sentinel and shift the byte-accurate reconstruction. for i in range(len(lines) - 1, search_from - 1, -1): - if lines[i].strip() == marker: + if lines[i].rstrip("\r") == marker: return i return None diff --git a/src/services/programmatic/service.py b/src/services/programmatic/service.py index 89780a5..5d63b30 100644 --- a/src/services/programmatic/service.py +++ b/src/services/programmatic/service.py @@ -20,6 +20,7 @@ from ...models.programmatic import ( ProgrammaticRequest, ProgrammaticResponse, + ProgrammaticTool, ProgrammaticToolCall, ) from ..orchestrator import ExecutionOrchestrator @@ -29,7 +30,7 @@ PTC_HISTORY_FILENAME, is_reserved_ptc_filename, ) -from .harness import build_python_code +from .harness import build_python_code, normalize_python_function_name from .sentinel import extract_pending_from_stdout from .state import ExecutionState, ProgrammaticStateStore @@ -45,6 +46,32 @@ def __init__(self, status_code: int, message: str): self.message = message +def _validate_tool_names(tools: list[ProgrammaticTool]) -> None: + """Reject tool names that would generate invalid Python in the replay harness. + + An empty/whitespace name or one with no identifier-safe characters yields + ``async def ()`` (a syntax error) which would otherwise surface as a + confusing ``completed`` response carrying a traceback instead of a clear + 400. Two distinct names that normalize to the same identifier are also + rejected, since the second stub would silently shadow the first. + """ + seen: dict[str, str] = {} + for tool in tools: + if not tool.name or not tool.name.strip(): + raise ProgrammaticError(400, "Each tool must have a non-empty name") + normalized = normalize_python_function_name(tool.name) + if not normalized: + raise ProgrammaticError(400, f'Tool name "{tool.name}" has no identifier-safe characters') + prior = seen.get(normalized) + if prior is not None and prior != tool.name: + raise ProgrammaticError( + 400, + f'Tool names "{prior}" and "{tool.name}" both normalize to the Python ' + f'identifier "{normalized}"; rename one to avoid a collision', + ) + seen[normalized] = tool.name + + class ProgrammaticService: """Drives the stateless replay loop for ``POST /exec/programmatic``.""" @@ -84,6 +111,7 @@ async def _initial( raise ProgrammaticError( 400, f"Too many tools provided ({len(request.tools)}). Maximum is {MAX_TOOLS_PER_REQUEST}." ) + _validate_tool_names(request.tools) if request.resolved_language == "bash": raise ProgrammaticError(400, "bash programmatic tool calling is not supported by this server") if request.resolved_language != "python": diff --git a/tests/unit/test_programmatic_harness.py b/tests/unit/test_programmatic_harness.py index 26ce392..6fb9ea0 100644 --- a/tests/unit/test_programmatic_harness.py +++ b/tests/unit/test_programmatic_harness.py @@ -10,9 +10,9 @@ is_reserved_ptc_filename, ) from src.services.programmatic.harness import ( - _normalize_python_function_name, build_python_code, build_replay_preamble, + normalize_python_function_name, wrap_user_code_in_async, ) from src.services.programmatic.sentinel import extract_pending_from_stdout @@ -72,7 +72,7 @@ def test_preamble_embeds_scoped_sentinel_and_history_path(self): ], ) def test_normalize_python_function_name(self, raw, expected): - assert _normalize_python_function_name(raw) == expected + assert normalize_python_function_name(raw) == expected class TestSentinelExtraction: @@ -111,6 +111,24 @@ def test_malformed_json_payload_returns_none(self): clean, pending = extract_pending_from_stdout(stdout, "e1") assert pending is None + def test_whitespace_padded_marker_not_matched(self): + # User output that merely contains the marker surrounded by whitespace + # must NOT be treated as a sentinel (exact line match required). + start, end = build_scoped_sentinel("e1") + body = json.dumps({"pending": [{"call_id": "call_001", "tool_name": "t", "input": {}}]}) + stdout = " " + start + " \n" + body + "\n" + end + "\n" + clean, pending = extract_pending_from_stdout(stdout, "e1") + assert pending is None + assert clean == stdout + + def test_trailing_carriage_return_tolerated(self): + # CRLF-captured stdout: the marker line ends with "\r"; still matches. + start, end = build_scoped_sentinel("e1") + body = json.dumps({"pending": [{"call_id": "call_001", "tool_name": "t", "input": {}}]}) + stdout = "out\r\n" + start + "\r\n" + body + "\r\n" + end + "\r\n" + clean, pending = extract_pending_from_stdout(stdout, "e1") + assert pending == [{"call_id": "call_001", "tool_name": "t", "input": {}}] + class TestReservedFilenames: @pytest.mark.parametrize( diff --git a/tests/unit/test_programmatic_service.py b/tests/unit/test_programmatic_service.py index 1eafbf9..bd8e093 100644 --- a/tests/unit/test_programmatic_service.py +++ b/tests/unit/test_programmatic_service.py @@ -151,6 +151,24 @@ async def test_missing_tools_rejected(self, service): await service.execute(ProgrammaticRequest(code="print(1)")) assert exc.value.status_code == 400 + @pytest.mark.asyncio + @pytest.mark.parametrize("bad_name", ["", " ", "!!!"]) + async def test_invalid_tool_name_rejected(self, service, bad_name): + with pytest.raises(ProgrammaticError) as exc: + await service.execute(ProgrammaticRequest(code="x", tools=[ProgrammaticTool(name=bad_name)])) + assert exc.value.status_code == 400 + + @pytest.mark.asyncio + async def test_colliding_tool_names_rejected(self, service): + # "get-weather" and "get_weather" both normalize to "get_weather" + req = ProgrammaticRequest( + code="x", + tools=[ProgrammaticTool(name="get-weather"), ProgrammaticTool(name="get_weather")], + ) + with pytest.raises(ProgrammaticError) as exc: + await service.execute(req) + assert exc.value.status_code == 400 + @pytest.mark.asyncio async def test_bash_rejected(self, service): with pytest.raises(ProgrammaticError) as exc: