fix(skills): fix dep detection false positives and all-or-nothing install

- Filter Python stdlib modules at scan time to prevent false positives
  when the runtime checker fails (e.g. pip:argparse, pip:sys)
- Install pip/npm packages one-by-one instead of batch so partial
  success is preserved when one package fails
- Persist missing deps to DB after install via StoreMissingDeps() so
  reload reflects actual state instead of stale data
- Use explicit master tenant context in handleInstallDeps for
  consistency with rescanAndUpdate()
This commit is contained in:
viettranx committed 2026-04-02 11:42:14 +07:00
1 parent ecf6463d98
commit 899c8eb0b4
5 files changed
+330 -25

No files matched your search

+26 -10
View File
@@ -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{
+20 -12
View File
@@ -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)
+4 -3
View File
@@ -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)
}
}
@@ -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)
}
}
}
+232
View File
@@ -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,
}