diff --git a/internal/http/skills.go b/internal/http/skills.go index 872303ac..71d8219a 100644 --- a/internal/http/skills.go +++ b/internal/http/skills.go @@ -255,7 +255,11 @@ func (h *SkillsHandler) handleInstallDeps(w http.ResponseWriter, r *http.Request if !h.requireMasterTenant(w, r) { return } - dirs := h.skills.ListSystemSkillDirs(r.Context()) + // Use explicit master tenant context for system skill operations, + // consistent with rescanAndUpdate() pattern. + masterCtx := store.WithTenantID(r.Context(), store.MasterTenantID) + + dirs := h.skills.ListSystemSkillDirs(masterCtx) if len(dirs) == 0 { writeJSON(w, http.StatusOK, map[string]string{"message": "no system skills"}) return @@ -274,25 +278,24 @@ func (h *SkillsHandler) handleInstallDeps(w http.ResponseWriter, r *http.Request }) } - result, err := installManagedDeps(r.Context(), manifest, missing) + result, err := installManagedDeps(masterCtx, manifest, missing) if err != nil { writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()}) return } - // Re-check all system skills and update status after install - allSkills := h.skills.ListAllSkills(r.Context()) + // Re-check all system skills, persist missing deps, and update status. + allSkills := h.skills.ListAllSkills(masterCtx) + statusChanged := false for _, sk := range allSkills { if !sk.IsSystem { continue } - dir, exists := dirs[sk.Slug] - if !exists { + if _, exists := dirs[sk.Slug]; !exists { continue } m := h.scanWithFallback(sk) if m == nil || m.IsEmpty() { - _ = dir // dir was used for direct scan; fallback uses sk.BaseDir continue } ok, miss := skills.CheckSkillDeps(m) @@ -300,10 +303,20 @@ func (h *SkillsHandler) handleInstallDeps(w http.ResponseWriter, r *http.Request if err != nil { continue } - if ok && sk.Status == "archived" { - _ = h.skills.UpdateSkill(r.Context(), id, map[string]any{"status": "active"}) - h.skills.BumpVersion() + + // Persist actual missing deps to DB so reload reflects reality. + _ = h.skills.StoreMissingDeps(masterCtx, id, miss) + + // Update status in both directions. + switch { + case ok && sk.Status == "archived": + _ = h.skills.UpdateSkill(masterCtx, id, map[string]any{"status": "active"}) + statusChanged = true + case !ok && sk.Status != "archived": + _ = h.skills.UpdateSkill(masterCtx, id, map[string]any{"status": "archived"}) + statusChanged = true } + status := "active" if !ok { status = "archived" @@ -319,6 +332,9 @@ func (h *SkillsHandler) handleInstallDeps(w http.ResponseWriter, r *http.Request }) } } + if statusChanged { + h.skills.BumpVersion() + } if h.msgBus != nil { h.msgBus.Broadcast(bus.Event{ diff --git a/internal/skills/dep_installer.go b/internal/skills/dep_installer.go index 80cfc426..f1fcfca1 100644 --- a/internal/skills/dep_installer.go +++ b/internal/skills/dep_installer.go @@ -119,26 +119,34 @@ func InstallDeps(ctx context.Context, manifest *SkillManifest, missing []string) result.System = successful } + // Pip packages: install one by one for partial-success resilience. if len(pipPkgs) > 0 { slog.Info("skills: installing pip packages", "pkgs", pipPkgs) - args := append([]string{"install", "--no-cache-dir", "--break-system-packages"}, pipPkgs...) - cmd := exec.CommandContext(ctx, "pip3", args...) - if out, err := cmd.CombinedOutput(); err != nil { - result.Errors = append(result.Errors, fmt.Sprintf("pip: %s (%v)", strings.TrimSpace(string(out)), err)) - } else { - result.Pip = pipPkgs + var successful []string + for _, pkg := range pipPkgs { + cmd := exec.CommandContext(ctx, "pip3", "install", "--no-cache-dir", "--break-system-packages", pkg) + if out, err := cmd.CombinedOutput(); err != nil { + result.Errors = append(result.Errors, fmt.Sprintf("pip %s: %s (%v)", pkg, strings.TrimSpace(string(out)), err)) + } else { + successful = append(successful, pkg) + } } + result.Pip = successful } + // Npm packages: install one by one for partial-success resilience. if len(npmPkgs) > 0 { slog.Info("skills: installing npm packages", "pkgs", npmPkgs) - args := append([]string{"install", "-g"}, npmPkgs...) - cmd := exec.CommandContext(ctx, "npm", args...) - if out, err := cmd.CombinedOutput(); err != nil { - result.Errors = append(result.Errors, fmt.Sprintf("npm: %s (%v)", strings.TrimSpace(string(out)), err)) - } else { - result.Npm = npmPkgs + var successful []string + for _, pkg := range npmPkgs { + cmd := exec.CommandContext(ctx, "npm", "install", "-g", pkg) + if out, err := cmd.CombinedOutput(); err != nil { + result.Errors = append(result.Errors, fmt.Sprintf("npm %s: %s (%v)", pkg, strings.TrimSpace(string(out)), err)) + } else { + successful = append(successful, pkg) + } } + result.Npm = successful } cleanCaches(ctx) diff --git a/internal/skills/dep_scanner.go b/internal/skills/dep_scanner.go index 446a0107..57455598 100644 --- a/internal/skills/dep_scanner.go +++ b/internal/skills/dep_scanner.go @@ -87,10 +87,11 @@ func scanScriptsDir(scriptsDir string) *SkillManifest { for b := range binaries { m.Requires = append(m.Requires, b) } - // Store raw import names — skip local module dirs (subdirs of scriptsDir). - // dep_checker.go handles stdlib/pip resolution via PYTHONPATH. + // Store raw import names — skip local modules and Python stdlib. + // Stdlib is also resolved at check time via actual import, but filtering here + // prevents false positives when the checker fails (timeout, env issue, crash). for pkg := range pyImports { - if !localModules[pkg] { + if !localModules[pkg] && !pythonStdlib[pkg] { m.RequiresPython = append(m.RequiresPython, pkg) } } diff --git a/internal/skills/dep_scanner_false_positive_test.go b/internal/skills/dep_scanner_false_positive_test.go index 806319a4..a8cf0957 100644 --- a/internal/skills/dep_scanner_false_positive_test.go +++ b/internal/skills/dep_scanner_false_positive_test.go @@ -3,6 +3,7 @@ package skills import ( "os" "path/filepath" + "slices" "testing" ) @@ -89,3 +90,50 @@ TEMPLATE = """ t.Error("FALSE POSITIVE: lodash detected as Python import") } } + +func TestScanScriptsDir_FiltersStdlib(t *testing.T) { + scriptsDir := filepath.Join(t.TempDir(), "scripts") + if err := os.MkdirAll(scriptsDir, 0755); err != nil { + t.Fatal(err) + } + + // Script imports stdlib modules + one real pip dep + content := `import sys +import os +import json +import argparse +import subprocess +from pathlib import Path +from datetime import datetime + +import requests +from PIL import Image +` + if err := os.WriteFile(filepath.Join(scriptsDir, "main.py"), []byte(content), 0644); err != nil { + t.Fatal(err) + } + + m := scanScriptsDir(scriptsDir) + + // Only real pip deps should appear in RequiresPython — NOT stdlib. + for _, pkg := range m.RequiresPython { + if pythonStdlib[pkg] { + t.Errorf("stdlib module %q should have been filtered from RequiresPython", pkg) + } + } + + // Real deps must be present. + if !slices.Contains(m.RequiresPython, "requests") { + t.Error("expected 'requests' in RequiresPython") + } + if !slices.Contains(m.RequiresPython, "PIL") { + t.Error("expected 'PIL' in RequiresPython") + } + + // Stdlib must NOT be present. + for _, stdlib := range []string{"sys", "os", "json", "argparse", "subprocess", "pathlib", "datetime"} { + if slices.Contains(m.RequiresPython, stdlib) { + t.Errorf("stdlib %q should NOT be in RequiresPython", stdlib) + } + } +} diff --git a/internal/skills/python_stdlib.go b/internal/skills/python_stdlib.go new file mode 100644 index 00000000..3182851b --- /dev/null +++ b/internal/skills/python_stdlib.go @@ -0,0 +1,232 @@ +package skills + +// pythonStdlib contains top-level public module names from Python 3.10–3.14 stdlib. +// Includes deprecated modules (removed in 3.12+) for backward compatibility. +// Filtered at scan time to prevent false positives when the runtime checker +// fails (timeout, env issue) — without this, stdlib modules get reported +// as pip deps with wrong names (e.g. "pip:argparse", "pip:sys"). +// +// Source: Python 3.14 sys.stdlib_module_names + deprecated modules from 3.11 +var pythonStdlib = map[string]bool{ + "__future__": true, + "_thread": true, + "abc": true, + "aifc": true, + "annotationlib": true, + "antigravity": true, + "argparse": true, + "array": true, + "ast": true, + "asynchat": true, + "asyncio": true, + "asyncore": true, + "atexit": true, + "audioop": true, + "base64": true, + "bdb": true, + "binascii": true, + "binhex": true, + "bisect": true, + "builtins": true, + "bz2": true, + "calendar": true, + "cgi": true, + "cgitb": true, + "chunk": true, + "cmath": true, + "cmd": true, + "code": true, + "codecs": true, + "codeop": true, + "collections": true, + "colorsys": true, + "compileall": true, + "compression": true, + "concurrent": true, + "configparser": true, + "contextlib": true, + "contextvars": true, + "copy": true, + "copyreg": true, + "cProfile": true, + "crypt": true, + "csv": true, + "ctypes": true, + "curses": true, + "dataclasses": true, + "datetime": true, + "dbm": true, + "decimal": true, + "difflib": true, + "dis": true, + "distutils": true, + "doctest": true, + "email": true, + "encodings": true, + "ensurepip": true, + "enum": true, + "errno": true, + "faulthandler": true, + "fcntl": true, + "filecmp": true, + "fileinput": true, + "fnmatch": true, + "fractions": true, + "ftplib": true, + "functools": true, + "gc": true, + "genericpath": true, + "getopt": true, + "getpass": true, + "gettext": true, + "glob": true, + "graphlib": true, + "grp": true, + "gzip": true, + "hashlib": true, + "heapq": true, + "hmac": true, + "html": true, + "http": true, + "idlelib": true, + "imaplib": true, + "imghdr": true, + "imp": true, + "importlib": true, + "inspect": true, + "io": true, + "ipaddress": true, + "itertools": true, + "json": true, + "keyword": true, + "lib2to3": true, + "linecache": true, + "locale": true, + "logging": true, + "lzma": true, + "mailbox": true, + "mailcap": true, + "marshal": true, + "math": true, + "mimetypes": true, + "mmap": true, + "modulefinder": true, + "msvcrt": true, + "multiprocessing": true, + "netrc": true, + "nis": true, + "nt": true, + "ntpath": true, + "nturl2path": true, + "nntplib": true, + "numbers": true, + "opcode": true, + "operator": true, + "optparse": true, + "os": true, + "ossaudiodev": true, + "pathlib": true, + "pdb": true, + "pickle": true, + "pickletools": true, + "pipes": true, + "pkgutil": true, + "platform": true, + "plistlib": true, + "poplib": true, + "posix": true, + "posixpath": true, + "pprint": true, + "profile": true, + "pstats": true, + "pty": true, + "pwd": true, + "py_compile": true, + "pyclbr": true, + "pydoc": true, + "pydoc_data": true, + "pyexpat": true, + "queue": true, + "quopri": true, + "random": true, + "re": true, + "readline": true, + "reprlib": true, + "resource": true, + "rlcompleter": true, + "runpy": true, + "sched": true, + "secrets": true, + "select": true, + "selectors": true, + "shelve": true, + "shlex": true, + "shutil": true, + "signal": true, + "site": true, + "smtpd": true, + "smtplib": true, + "sndhdr": true, + "socket": true, + "socketserver": true, + "sqlite3": true, + "sre_compile": true, + "sre_constants": true, + "sre_parse": true, + "ssl": true, + "stat": true, + "statistics": true, + "string": true, + "stringprep": true, + "struct": true, + "subprocess": true, + "sunau": true, + "symtable": true, + "sys": true, + "sysconfig": true, + "syslog": true, + "tabnanny": true, + "tarfile": true, + "telnetlib": true, + "tempfile": true, + "termios": true, + "test": true, + "textwrap": true, + "this": true, + "threading": true, + "time": true, + "timeit": true, + "tkinter": true, + "token": true, + "tokenize": true, + "tomllib": true, + "trace": true, + "traceback": true, + "tracemalloc": true, + "tty": true, + "turtle": true, + "turtledemo": true, + "types": true, + "typing": true, + "unicodedata": true, + "unittest": true, + "urllib": true, + "uu": true, + "uuid": true, + "venv": true, + "warnings": true, + "wave": true, + "weakref": true, + "webbrowser": true, + "winreg": true, + "winsound": true, + "wsgiref": true, + "xdrlib": true, + "xml": true, + "xmlrpc": true, + "zipapp": true, + "zipfile": true, + "zipimport": true, + "zlib": true, + "zoneinfo": true, +}