From 6e5e51a18bb8cd68316f95af2fd3221cb9ffb532 Mon Sep 17 00:00:00 2001 From: Duy /zuey/ Date: Mon, 11 May 2026 15:31:32 +0700 Subject: [PATCH] =?UTF-8?q?feat(packages):=20Phase=202a=20=E2=80=94=20pip?= =?UTF-8?q?=20+=20npm=20update=20flow=20(#900)=20(#6)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(packages): backend pip + npm update flow (#900) Extend Phase 1 update infrastructure to pip + npm sources. Register checkers/executors behind edition gate (Lite edition stays github-only). Per-source sentinel errors + stderr classifier; strict package-name validators reject @version suffix. Shared PackageLocker serializes install + update paths. HTTP response surfaces per-source availability from LookPath detection. Closes part of #900 (Phase 2a). * feat(packages): frontend multi-source updates UI (#900) Unified flat updates list with source pill (github/pip/npm) + filter dropdown. Summary bar shows per-source counts, hiding sources whose backend availability=false. 30 i18n keys with full en/vi/zh parity. Mobile-safe table (overflow-x-auto + min-w-[600px]). Part of #900 (Phase 2a). * test(packages): pip + npm integration e2e (#900) Optional real-runtime integration test behind `pipnpm_e2e` build tag. Skipped by default CI; exercises full check + apply cycle with real pip3/npm in Alpine container. Part of #900 (Phase 2a). * docs(packages): document pip + npm update flow (#900) Adds packages-pip-npm.md covering command matrix, exit codes, stderr error classes, pre-release handling, availability detection, runbook for EACCES/ERESOLVE/externally-managed, min versions, fixture regen. Cross-link from packages-github.md. Changelogs updated. Part of #900 (Phase 2a). * fix(packages): set exec bit on testdata npm/pip scripts --- CHANGELOG.md | 7 + cmd/gateway_packages_wiring.go | 10 + docs/packages-github.md | 1 + docs/packages-pip-npm.md | 196 +++++++++++ internal/edition/edition.go | 12 +- internal/edition/edition_test.go | 10 + internal/http/packages_updates.go | 107 +++--- internal/http/packages_updates_test.go | 114 ++++++- internal/i18n/catalog_en.go | 17 + internal/i18n/catalog_vi.go | 17 + internal/i18n/catalog_zh.go | 17 + internal/i18n/keys.go | 17 + internal/skills/dep_installer.go | 51 ++- internal/skills/dep_installer_phase2a_test.go | 123 +++++++ internal/skills/dep_installer_test.go | 126 +++++++ internal/skills/github_update_checker.go | 4 + internal/skills/npm_update_checker.go | 164 +++++++++ internal/skills/npm_update_checker_test.go | 186 +++++++++++ internal/skills/npm_update_executor.go | 82 +++++ internal/skills/npm_update_executor_test.go | 154 +++++++++ internal/skills/pip_update_checker.go | 163 +++++++++ internal/skills/pip_update_checker_test.go | 222 +++++++++++++ internal/skills/pip_update_executor.go | 93 ++++++ internal/skills/pip_update_executor_test.go | 228 +++++++++++++ internal/skills/pkg_update_helpers.go | 160 +++++++++ internal/skills/pkg_update_helpers_test.go | 310 ++++++++++++++++++ internal/skills/testdata/npm/bin/npm | 48 +++ internal/skills/testdata/npm/outdated-10.json | 6 + internal/skills/testdata/pip/bin/pip3 | 46 +++ .../skills/testdata/pip/outdated-23.3.json | 5 + .../skills/testdata/pip/outdated-empty.json | 1 + internal/skills/update_registry.go | 50 ++- internal/skills/update_registry_test.go | 84 +++++ internal/skills/wiring_edition_gate_test.go | 81 +++++ tests/integration/packages_pipnpm_test.go | 139 ++++++++ ui/web/src/i18n/locales/en/packages.json | 26 +- ui/web/src/i18n/locales/vi/packages.json | 26 +- ui/web/src/i18n/locales/zh/packages.json | 26 +- .../pages/packages/components/source-pill.tsx | 32 ++ .../packages/components/update-row-button.tsx | 18 +- .../packages/components/updates-list.tsx | 148 +++++++++ .../components/updates-summary-bar.tsx | 33 +- .../packages/github-binaries-section.tsx | 2 + .../src/pages/packages/hooks/use-updates.ts | 6 +- ui/web/src/pages/packages/packages-page.tsx | 13 + 45 files changed, 3307 insertions(+), 74 deletions(-) create mode 100644 docs/packages-pip-npm.md create mode 100644 internal/skills/dep_installer_phase2a_test.go create mode 100644 internal/skills/dep_installer_test.go create mode 100644 internal/skills/npm_update_checker.go create mode 100644 internal/skills/npm_update_checker_test.go create mode 100644 internal/skills/npm_update_executor.go create mode 100644 internal/skills/npm_update_executor_test.go create mode 100644 internal/skills/pip_update_checker.go create mode 100644 internal/skills/pip_update_checker_test.go create mode 100644 internal/skills/pip_update_executor.go create mode 100644 internal/skills/pip_update_executor_test.go create mode 100644 internal/skills/pkg_update_helpers.go create mode 100644 internal/skills/pkg_update_helpers_test.go create mode 100755 internal/skills/testdata/npm/bin/npm create mode 100644 internal/skills/testdata/npm/outdated-10.json create mode 100755 internal/skills/testdata/pip/bin/pip3 create mode 100644 internal/skills/testdata/pip/outdated-23.3.json create mode 100644 internal/skills/testdata/pip/outdated-empty.json create mode 100644 internal/skills/update_registry_test.go create mode 100644 internal/skills/wiring_edition_gate_test.go create mode 100644 tests/integration/packages_pipnpm_test.go create mode 100644 ui/web/src/pages/packages/components/source-pill.tsx create mode 100644 ui/web/src/pages/packages/components/updates-list.tsx diff --git a/CHANGELOG.md b/CHANGELOG.md index f2674136..57375fda 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,13 @@ All notable changes to GoClaw are documented here. For full documentation, see [ ### Added +- **Packages Update Flow (Phase 2a: pip + npm)** — closes #900 (Phase 2a). Extends + Phase 1 update infrastructure to pip and npm package sources. `/v1/packages/updates` + now returns mixed-source results with an `availability: {github, pip, npm}` map. + Multi-source UI with per-source filter pills; unavailable sources (binary not on PATH + or Lite edition) hidden automatically. apk deferred to Phase 2b. + See `docs/packages-pip-npm.md` for command matrix, runbook, and min versions. + - **Packages Update Flow (Phase 1: GitHub binaries)** — closes #900. Proactive "N updates available" badge + per-row `[Update]` + `[Update All]` on the Runtime & Packages page. Backend endpoints under `/v1/packages/updates*` diff --git a/cmd/gateway_packages_wiring.go b/cmd/gateway_packages_wiring.go index fb12e534..8a86d01d 100644 --- a/cmd/gateway_packages_wiring.go +++ b/cmd/gateway_packages_wiring.go @@ -4,6 +4,7 @@ import ( "log/slog" "path/filepath" + "github.com/nextlevelbuilder/goclaw/internal/edition" httpapi "github.com/nextlevelbuilder/goclaw/internal/http" "github.com/nextlevelbuilder/goclaw/internal/skills" ) @@ -37,6 +38,7 @@ func wirePackagesHandler(d *gatewayDeps) *httpapi.PackagesHandler { // Share the installer's locker so Install and Update share per-package locks. registry.Locker = installer.Locker + skills.SetSharedPackageLocker(registry.Locker) // Register checker + executor for "github" source. registry.RegisterChecker(skills.NewGitHubUpdateChecker(installer)) @@ -47,6 +49,14 @@ func wirePackagesHandler(d *gatewayDeps) *httpapi.PackagesHandler { } registry.RegisterExecutor(executor) + // Register pip + npm checkers/executors when the edition supports them. + if edition.Current().SupportsPipNpm { + registry.RegisterChecker(skills.NewPipUpdateChecker()) + registry.RegisterExecutor(skills.NewPipUpdateExecutor()) + registry.RegisterChecker(skills.NewNpmUpdateChecker()) + registry.RegisterExecutor(skills.NewNpmUpdateExecutor()) + } + slog.Info("packages: update registry wired", "cache", cachePath, "ttl", ttl, diff --git a/docs/packages-github.md b/docs/packages-github.md index 3db7d52b..d8e6fbb7 100644 --- a/docs/packages-github.md +++ b/docs/packages-github.md @@ -241,5 +241,6 @@ Phase 1 leaves `.bak.{nanos}` files on disk. Manual recovery: ## See Also +- [`docs/packages-pip-npm.md`](./packages-pip-npm.md) — pip + npm package updates (Phase 2a) - [`docs/14-skills-runtime.md`](./14-skills-runtime.md) — Overview of the runtime packages system - Issue [#741](https://github.com/nextlevelbuilder/goclaw/issues/741) — Original feature request diff --git a/docs/packages-pip-npm.md b/docs/packages-pip-npm.md new file mode 100644 index 00000000..65e6d774 --- /dev/null +++ b/docs/packages-pip-npm.md @@ -0,0 +1,196 @@ +# pip + npm Package Updates (Phase 2a) + +Extends the Phase 1 GitHub binary update flow to system-wide pip and npm packages. +Closes #900 (Phase 2a). + +See also: [GitHub binary updates](./packages-github.md) + +--- + +## Overview + +When the gateway is running in Standard edition with `pip3` and/or `npm` on PATH, +`GET /v1/packages/updates` includes pip and npm update results alongside GitHub +binaries. The UI shows a per-source pill filter; sources without a binary on PATH +are hidden automatically. + +pip scope: **system-wide** (`--break-system-packages`). pip venv / user-site is not +supported in Phase 2a. + +npm scope: **global** (`--global`). Per-project `node_modules` are not touched. + +--- + +## Command Matrix + +| Source | Check command | Update command | Check timeout | Update timeout | +|--------|---------------|----------------|---------------|----------------| +| pip | `pip3 list --outdated --format json --break-system-packages` | `pip3 install --upgrade --no-cache-dir --break-system-packages --upgrade-strategy only-if-needed ` | 30 s | 5 min | +| npm | `npm outdated --global --json` | `npm install --global @` | 30 s | 5 min | + +Pre-release pip check appends `--pre` in a secondary call (see Pre-Release Handling). + +--- + +## Behavior + +### pip + +- `pip3 list --outdated --format json` emits a JSON array; each element has + `name`, `version`, `latest_version`, `latest_filetype`. +- Exit code is always 0 whether or not updates exist. +- stderr is classified via `ClassifyPipStderr` into sentinel errors (see Error Classes). + +### npm + +npm's exit-code semantics are non-standard: + +| Condition | Exit code | Interpretation | +|-----------|-----------|----------------| +| No outdated packages | 0 | No updates | +| Outdated packages found | 1 | Updates — parse JSON stdout | +| Real npm error (ERESOLVE, network, etc.) | 1 | stderr contains `npm ERR!` | +| Ambiguous (exit 1, no stdout, no stderr) | 1 | Treated as no-updates | + +The checker inspects exit code **and** stderr for `npm ERR!` before deciding +whether exit 1 means "updates available" or "real error". + +--- + +## Pre-Release Handling + +### pip + +Two-call merge strategy: + +1. **Primary call** (stable only, no `--pre`): baseline list of outdated packages. +2. If any currently-installed package has a pre-release version (`IsPipPreRelease()`): + **Secondary call** with `--pre` to surface the best available upgrade target. +3. Results are merged by package name; when a name appears in both, the entry + with the lexicographically higher `latest_version` wins. + +Gate: `IsPipPreRelease` matches PEP 440 patterns — `a`, `b`, `rc`, `dev`, `.pre`, +`.preview` (case-insensitive, digits optional). + +### npm + +Single-call strategy with a skip gate: + +- If `latest` contains an npm pre-release label + (`-alpha`, `-beta`, `-rc`, `-pre`, `-preview`, `-dev`, `-nightly`, `-snapshot`) + **and** `current` does not → entry is skipped. +- If `current` is already a pre-release and `latest` is too → entry kept (user on + pre-release channel receives the newest pre-release update). + +This prevents unexpected upgrades from stable channels to unstable channels. + +--- + +## Availability Detection + +A source is considered **available** when two gates both pass: + +1. **Binary present**: `exec.LookPath("pip3")` / `exec.LookPath("npm")` succeeds. +2. **Edition allows it**: `edition.Current().SupportsPipNpm == true` (always true + for Standard; always false for Lite desktop). + +When a source is unavailable: + +- Its checker returns `UpdateCheckResult{Available: false}` (no error, no updates). +- `UpdateRegistry.Availability()` maps that source to `false`. +- `GET /v1/packages/updates` response includes `"availability": {"pip": false, "npm": false}`. +- The frontend hides that source from the filter bar. + +Lite edition: `gateway_packages_wiring.go` checks `edition.Current().SupportsPipNpm` +before calling `RegisterChecker` / `RegisterExecutor`. Pip and npm checkers are +never instantiated — `registry.Sources()` returns `["github"]` only. + +--- + +## Error Classes + +Sentinel errors are defined in `internal/skills/pkg_update_helpers.go`. + +### pip sentinels + +| Sentinel | Trigger pattern in stderr | i18n key | +|----------|--------------------------|----------| +| `ErrUpdatePipExternallyManaged` | `externally-managed-environment` / `EXTERNALLY-MANAGED` | `packages.update.pip.externally_managed` | +| `ErrUpdatePipPermission` | `Permission denied` / `EACCES` | `packages.update.pip.permission` | +| `ErrUpdatePipNotFound` | `No matching distribution` / `Could not find a version` | `packages.update.pip.not_found` | +| `ErrUpdatePipNetwork` | `Read timed out` / `ConnectionError` / `network` | `packages.update.pip.network` | +| `ErrUpdatePipConflict` | `incompatible` / `dependency resolver` / `Shallow backtracking` | `packages.update.pip.conflict` | + +### npm sentinels + +| Sentinel | Trigger pattern in stderr | i18n key | +|----------|--------------------------|----------| +| `ErrUpdateNpmPermission` | `EACCES` | `packages.update.npm.permission` | +| `ErrUpdateNpmConflict` | `ERESOLVE` | `packages.update.npm.conflict` | +| `ErrUpdateNpmNetwork` | `ETIMEDOUT` / `ENOTFOUND` / `getaddrinfo` | `packages.update.npm.network` | +| `ErrUpdateNpmTargetMissing` | `ETARGET` | `packages.update.npm.target_missing` | +| `ErrUpdateNpmNotFound` | `E404` / `404` / `not in this registry` | `packages.update.npm.not_found` | + +Unclassified stderr returns a generic wrapped error with a truncated reason +(≤ 500 chars, ANSI-stripped). + +--- + +## Runbook + +| Symptom | Fix | +|---------|-----| +| **pip EACCES** — gateway lacks write to site-packages | Run gateway as an owner of `/usr/lib/python3/dist-packages`, or set `PIP_TARGET=/app/data/.pip` + add it to `PYTHONPATH` | +| **npm EACCES** — global prefix owned by root | `npm config set prefix ~/.npm-global`; add `~/.npm-global/bin` to `PATH` in entrypoint | +| **npm ERESOLVE** — peer conflict blocks install | SSH into container: `npm install -g @ --legacy-peer-deps`; re-check will clear the entry | +| **pip externally-managed (PEP 668)** | Set env var `PIP_BREAK_SYSTEM_PACKAGES=1`, or upgrade pip to ≥ 23.3 (respects the CLI flag without the env var) | + +--- + +## Minimum Versions + +| Runtime | Minimum | Recommended | Notes | +|---------|---------|-------------|-------| +| pip | 20.0 | ≥ 23.3 | `--format json` requires 20+; `--break-system-packages` without env var requires 23.3+ | +| npm | 6.0 | ≥ 10 | Older versions may not emit JSON exit 1 correctly | +| Node.js | 12 | ≥ 18 LTS | npm 10 requires Node 18+ | + +--- + +## Shared Locker + +`InstallSingleDep` (skill dep install) and `PipUpdateExecutor.Update` / `NpmUpdateExecutor.Update` +(update apply) share a single `PackageLocker` instance injected via `SetSharedPackageLocker`. + +This means concurrent `pip install requests` (from a skill) and `pip upgrade requests` +(from the update flow) are serialized by the same per-key mutex. The lock key is +the bare package name (e.g. `"requests"`) scoped to the source (`"pip"` or `"npm"`). + +Operators must not bypass the gateway and call `pip install` directly in parallel +with gateway operations — doing so defeats the shared lock and risks a partial-install +race. + +--- + +## Fixture Regeneration + +Test fixtures capture `pip3 list --outdated --format json` and `npm outdated -g --json` +output. When the environment's package versions change, regenerate them: + +```bash +# pip fixture — include pip version in filename for drift tracking +pip3 --version # e.g., pip 24.0 +pip3 list --outdated --format json --break-system-packages \ + > internal/skills/testdata/pip_outdated_pip24.json + +# npm fixture — include npm version in filename +npm --version # e.g., 10.5.0 +npm outdated --global --json \ + > internal/skills/testdata/npm_outdated_npm10.json +# Note: npm exits 1 when packages are outdated — that's expected. + +# Update test cases to reference the new filename and expected values. +``` + +Fixture files are version-stamped in their names so drift between CI environments +is detectable by `git diff`. diff --git a/internal/edition/edition.go b/internal/edition/edition.go index 37d30216..97c990f2 100644 --- a/internal/edition/edition.go +++ b/internal/edition/edition.go @@ -18,17 +18,19 @@ type Edition struct { RBACEnabled bool `json:"rbac_enabled"` TeamFullMode bool `json:"team_full_mode"` // false = lite task actions only VectorSearch bool `json:"vector_search"` // false = FTS5 only + SupportsPipNpm bool `json:"supports_pip_npm"` // false for Lite desktop } // --- Presets --- // Standard is the default edition: all features enabled, no limits. var Standard = Edition{ - Name: "standard", - KGEnabled: true, - RBACEnabled: true, - TeamFullMode: true, - VectorSearch: true, + Name: "standard", + KGEnabled: true, + RBACEnabled: true, + TeamFullMode: true, + VectorSearch: true, + SupportsPipNpm: true, } // Lite is the desktop/self-hosted edition with sensible limits. diff --git a/internal/edition/edition_test.go b/internal/edition/edition_test.go index 753493a5..bac848fd 100644 --- a/internal/edition/edition_test.go +++ b/internal/edition/edition_test.go @@ -366,6 +366,16 @@ func TestEditionConcurrentSafety(t *testing.T) { // If this completes without panic, the test passes } +// TestSupportsPipNpm verifies the pip/npm feature flag is set correctly per edition. +func TestSupportsPipNpm(t *testing.T) { + if !Standard.SupportsPipNpm { + t.Error("Standard.SupportsPipNpm = false, want true") + } + if Lite.SupportsPipNpm { + t.Error("Lite.SupportsPipNpm = true, want false") + } +} + // TestCustomEdition_PartialConfiguration allows custom editions. func TestCustomEdition_PartialConfiguration(t *testing.T) { custom := Edition{ diff --git a/internal/http/packages_updates.go b/internal/http/packages_updates.go index 16e48e42..5fa96bdc 100644 --- a/internal/http/packages_updates.go +++ b/internal/http/packages_updates.go @@ -83,12 +83,13 @@ func (h *PackagesHandler) handleListUpdates(w http.ResponseWriter, r *http.Reque } writeJSON(w, http.StatusOK, map[string]any{ - "updates": updates, - "checkedAt": checkedAt, - "ageSeconds": int64(age.Seconds()), - "ttlSeconds": int64(ttl.Seconds()), - "stale": stale, - "sources": h.Registry.Sources(), + "updates": updates, + "checkedAt": checkedAt, + "ageSeconds": int64(age.Seconds()), + "ttlSeconds": int64(ttl.Seconds()), + "stale": stale, + "sources": h.Registry.Sources(), + "availability": h.Registry.Availability(), }) } @@ -170,7 +171,7 @@ func (h *PackagesHandler) handleUpdatePackage(w http.ResponseWriter, r *http.Req source, name, ok := resolveUpdateSpec(req.Package) if !ok { writeJSON(w, http.StatusBadRequest, map[string]string{ - "error": i18n.T(locale, i18n.MsgInvalidRequest, "package must be github:"), + "error": i18n.T(locale, i18n.MsgInvalidRequest, "package must be github:, pip:, or npm:"), }) return } @@ -437,38 +438,55 @@ func (h *PackagesHandler) handleApplyAllUpdates(w http.ResponseWriter, r *http.R // ---- helpers ---- -// resolveUpdateSpec parses a "github:" or "github:owner/repo" spec -// and returns (source, name, ok). source is always "github" (Phase 1). -// Bare names like "github:lazygit" are resolved directly; full specs are -// resolved by extracting the repo name (not owner) for manifest lookup. +// resolveUpdateSpec parses a package spec and returns (source, name, ok). +// Supported prefixes: "github:", "pip:", "npm:". +// +// github: bare name "github:" or full "github:owner/repo[@tag]". +// Bare github names are validated against validGitHubBareName; full specs +// are resolved via the manifest (repo may differ, e.g. cli/cli → gh). +// pip/npm: name is validated via the strict whitelist validators. +// Bare-name fallback (without colon) is NOT supported — all sources require +// an explicit "source:" prefix. func resolveUpdateSpec(pkg string) (source, name string, ok bool) { - if !strings.HasPrefix(pkg, "github:") { + prefix, rest, found := strings.Cut(pkg, ":") + if !found || rest == "" { return "", "", false } - bare := strings.TrimPrefix(pkg, "github:") - if bare == "" { - return "", "", false - } - // Full spec "github:owner/repo[@tag]" — extract bare name = repo component. - if spec, err := skills.ParseGitHubSpec(pkg); err == nil { - // Resolve name via manifest (repo may differ from binary name, e.g. cli/cli → gh). - if installer := skills.DefaultGitHubInstaller(); installer != nil { - if entries, lerr := installer.List(); lerr == nil { - for _, e := range entries { - if strings.EqualFold(e.Repo, spec.Owner+"/"+spec.Repo) { - return "github", e.Name, true + switch prefix { + case "github": + // Full spec "github:owner/repo[@tag]" — extract bare name = repo component. + if spec, err := skills.ParseGitHubSpec(pkg); err == nil { + // Resolve name via manifest (repo may differ from binary name, e.g. cli/cli → gh). + if installer := skills.DefaultGitHubInstaller(); installer != nil { + if entries, lerr := installer.List(); lerr == nil { + for _, e := range entries { + if strings.EqualFold(e.Repo, spec.Owner+"/"+spec.Repo) { + return "github", e.Name, true + } } } } + // Fallback: use repo name directly. + return "github", spec.Repo, true } - // Fallback: use repo name directly. - return "github", spec.Repo, true + // Bare name form "github:". + if validGitHubBareName.MatchString(rest) { + return "github", rest, true + } + return "", "", false + case "pip": + if err := skills.ValidatePipPackageName(rest); err != nil { + return "", "", false + } + return "pip", rest, true + case "npm": + if err := skills.ValidateNpmPackageName(rest); err != nil { + return "", "", false + } + return "npm", rest, true + default: + return "", "", false } - // Bare name form "github:". - if validGitHubBareName.MatchString(bare) { - return "github", bare, true - } - return "", "", false } // nonNilSlice returns an empty non-nil slice when s is nil, so JSON encodes @@ -487,18 +505,25 @@ func nonNilSlice[T any](s []T) []T { // For github source: installer locks on parsed.Repo (repo-portion only, // e.g. "lazygit"). Meta carries repo as "owner/repo" — extract the portion // after "/". Fallback to name when meta is nil/missing (stale cache). +// +// For pip/npm: PackageLocker internally prefixes by source, so we return +// name directly (NOT "pip:name" or "npm:name"). func lockKeyForSource(source, name string, meta map[string]any) string { - if source != "github" { + switch source { + case "pip", "npm": + return name + case "github": + if meta != nil { + if v, ok := meta["repo"].(string); ok && v != "" { + if i := strings.IndexByte(v, '/'); i > 0 && i < len(v)-1 { + return v[i+1:] + } + return v + } + } + return name + default: return name } - if meta != nil { - if v, ok := meta["repo"].(string); ok && v != "" { - if i := strings.IndexByte(v, '/'); i > 0 && i < len(v)-1 { - return v[i+1:] - } - return v - } - } - return name } diff --git a/internal/http/packages_updates_test.go b/internal/http/packages_updates_test.go index de61b2b5..3457073f 100644 --- a/internal/http/packages_updates_test.go +++ b/internal/http/packages_updates_test.go @@ -235,19 +235,19 @@ func TestHandleUpdatePackage_InvalidBody(t *testing.T) { } } -func TestHandleUpdatePackage_NonGithubSpec(t *testing.T) { - // Only "github:" prefix is supported for updates. +func TestHandleUpdatePackage_UnknownPrefix(t *testing.T) { + // Truly unknown prefixes (not github/pip/npm) must return 400. h := NewPackagesHandler(buildTestRegistry(nil), nil) req := httptest.NewRequest(http.MethodPost, "/v1/packages/update", - bytes.NewBufferString(`{"package":"pip:pandas"}`)) + bytes.NewBufferString(`{"package":"garbage:pandas"}`)) req = req.WithContext(ownerCtx(req.Context(), t.Name())) w := httptest.NewRecorder() h.handleUpdatePackage(w, req) if w.Code != http.StatusBadRequest { - t.Fatalf("want 400 for non-github spec, got %d: %s", w.Code, w.Body.String()) + t.Fatalf("want 400 for unknown prefix, got %d: %s", w.Code, w.Body.String()) } } @@ -424,6 +424,112 @@ func TestHandleApplyAllUpdates_InvalidSpecInList(t *testing.T) { } } +// ---- resolveUpdateSpec table-driven tests ---- + +func TestResolveUpdateSpec(t *testing.T) { + cases := []struct { + input string + wantSource string + wantName string + wantOK bool + }{ + // pip: valid names + {"pip:requests", "pip", "requests", true}, + {"pip:Django", "pip", "Django", true}, // pip allows uppercase + {"pip:my-package", "pip", "my-package", true}, + // npm: valid names + {"npm:typescript", "npm", "typescript", true}, + {"npm:@angular/core", "npm", "@angular/core", true}, + // pip: invalid names — @version suffix must be rejected + {"pip:typescript@latest", "", "", false}, + {"pip:bad;name", "", "", false}, + {"pip:", "", "", false}, + // npm: invalid names + {"npm:typescript@latest", "", "", false}, + {"npm:TypeScript", "", "", false}, // npm forbids uppercase + // unknown / malformed prefixes + {"garbage:x", "", "", false}, + {"pip", "", "", false}, // no colon + {"", "", "", false}, + } + + for _, tc := range cases { + t.Run(tc.input, func(t *testing.T) { + src, name, ok := resolveUpdateSpec(tc.input) + if ok != tc.wantOK { + t.Fatalf("resolveUpdateSpec(%q): ok=%v, want %v", tc.input, ok, tc.wantOK) + } + if ok { + if src != tc.wantSource { + t.Errorf("source=%q, want %q", src, tc.wantSource) + } + if name != tc.wantName { + t.Errorf("name=%q, want %q", name, tc.wantName) + } + } + }) + } +} + +// ---- lockKeyForSource tests ---- + +func TestLockKeyForSource(t *testing.T) { + cases := []struct { + source string + name string + meta map[string]any + wantKey string + }{ + // pip and npm: return name directly (NOT "pip:name" or "npm:name") + {"pip", "requests", nil, "requests"}, + {"npm", "@scope/pkg", nil, "@scope/pkg"}, + // github: extract repo portion from meta + {"github", "lazygit", map[string]any{"repo": "jesseduffield/lazygit"}, "lazygit"}, + {"github", "gh", map[string]any{"repo": "cli/cli"}, "cli"}, + // github: fallback to name when meta missing + {"github", "fzf", nil, "fzf"}, + // unknown source: fallback to name + {"other", "pkg", nil, "pkg"}, + } + + for _, tc := range cases { + t.Run(tc.source+"/"+tc.name, func(t *testing.T) { + got := lockKeyForSource(tc.source, tc.name, tc.meta) + if got != tc.wantKey { + t.Errorf("lockKeyForSource(%q, %q, meta): got %q, want %q", tc.source, tc.name, got, tc.wantKey) + } + }) + } +} + +// ---- handleListUpdates availability field ---- + +func TestHandleListUpdates_IncludesAvailability(t *testing.T) { + registry := buildTestRegistry(nil) + h := NewPackagesHandler(registry, nil) + + req := httptest.NewRequest(http.MethodGet, "/v1/packages/updates", nil) + req = req.WithContext(store.WithRole(store.WithTenantID(store.WithUserID(req.Context(), "u1"), uuid.Nil), "operator")) + w := httptest.NewRecorder() + + h.handleListUpdates(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("want 200, got %d: %s", w.Code, w.Body.String()) + } + var body map[string]any + if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if _, ok := body["availability"]; !ok { + t.Error("response missing 'availability' field") + } + // availability must be a map (even if empty) + if _, ok := body["availability"].(map[string]any); !ok { + t.Errorf("availability must be map[string]bool, got %T", body["availability"]) + } +} + // ---- small utilities ---- func collectEventNames(evts []bus.Event) []string { diff --git a/internal/i18n/catalog_en.go b/internal/i18n/catalog_en.go index 40b0e10e..0e0eb973 100644 --- a/internal/i18n/catalog_en.go +++ b/internal/i18n/catalog_en.go @@ -288,5 +288,22 @@ func init() { // Message tool cross-target forward notice MessageCrossTargetForwarded: "📤 Forwarded to %s as requested: %q", + + // Package update source labels + MsgPackagesUpdatesSourceGithub: "GitHub", + MsgPackagesUpdatesSourcePip: "pip", + MsgPackagesUpdatesSourceNpm: "npm", + + // Package update availability messages + MsgPackagesUpdatesUnavailablePip: "pip not installed on this system", + MsgPackagesUpdatesUnavailableNpm: "npm not installed on this system", + + // Package update failure reasons + MsgPackagesUpdatesReasonDependencyConflict: "Dependency conflict", + MsgPackagesUpdatesReasonPermission: "Permission denied", + MsgPackagesUpdatesReasonNetwork: "Network error", + MsgPackagesUpdatesReasonNotFound: "Package not found", + MsgPackagesUpdatesReasonTargetMissing: "Version not available", + MsgPackagesUpdatesReasonExternallyManaged: "Environment externally managed", }) } diff --git a/internal/i18n/catalog_vi.go b/internal/i18n/catalog_vi.go index 7042278b..627e225e 100644 --- a/internal/i18n/catalog_vi.go +++ b/internal/i18n/catalog_vi.go @@ -288,5 +288,22 @@ func init() { // Message tool cross-target forward notice MessageCrossTargetForwarded: "📤 Đã forward sang %s theo yêu cầu: %q", + + // Package update source labels + MsgPackagesUpdatesSourceGithub: "GitHub", + MsgPackagesUpdatesSourcePip: "pip", + MsgPackagesUpdatesSourceNpm: "npm", + + // Package update availability messages + MsgPackagesUpdatesUnavailablePip: "pip chưa cài trên hệ thống", + MsgPackagesUpdatesUnavailableNpm: "npm chưa cài trên hệ thống", + + // Package update failure reasons + MsgPackagesUpdatesReasonDependencyConflict: "Xung đột phụ thuộc", + MsgPackagesUpdatesReasonPermission: "Bị từ chối quyền", + MsgPackagesUpdatesReasonNetwork: "Lỗi mạng", + MsgPackagesUpdatesReasonNotFound: "Không tìm thấy gói", + MsgPackagesUpdatesReasonTargetMissing: "Phiên bản không tồn tại", + MsgPackagesUpdatesReasonExternallyManaged: "Môi trường được quản lý bên ngoài", }) } diff --git a/internal/i18n/catalog_zh.go b/internal/i18n/catalog_zh.go index 6344508e..d21a66d6 100644 --- a/internal/i18n/catalog_zh.go +++ b/internal/i18n/catalog_zh.go @@ -288,5 +288,22 @@ func init() { // Message tool cross-target forward notice MessageCrossTargetForwarded: "📤 已按请求转发至 %s:%q", + + // Package update source labels + MsgPackagesUpdatesSourceGithub: "GitHub", + MsgPackagesUpdatesSourcePip: "pip", + MsgPackagesUpdatesSourceNpm: "npm", + + // Package update availability messages + MsgPackagesUpdatesUnavailablePip: "系统中未安装 pip", + MsgPackagesUpdatesUnavailableNpm: "系统中未安装 npm", + + // Package update failure reasons + MsgPackagesUpdatesReasonDependencyConflict: "依赖冲突", + MsgPackagesUpdatesReasonPermission: "权限被拒绝", + MsgPackagesUpdatesReasonNetwork: "网络错误", + MsgPackagesUpdatesReasonNotFound: "未找到软件包", + MsgPackagesUpdatesReasonTargetMissing: "版本不可用", + MsgPackagesUpdatesReasonExternallyManaged: "环境由外部管理", }) } diff --git a/internal/i18n/keys.go b/internal/i18n/keys.go index 22e51dae..09d7d299 100644 --- a/internal/i18n/keys.go +++ b/internal/i18n/keys.go @@ -126,6 +126,23 @@ const ( MsgUpdateManifestDesync = "packages.update.manifest_desync" // "Binary updated but manifest save failed — manual recovery required for {name}" MsgUpdateCacheStale = "packages.update.cache_stale" // "Updates cache stale; run refresh before applying an update" + // Package update source labels + MsgPackagesUpdatesSourceGithub = "packages.updates.source.github" // "GitHub" + MsgPackagesUpdatesSourcePip = "packages.updates.source.pip" // "pip" + MsgPackagesUpdatesSourceNpm = "packages.updates.source.npm" // "npm" + + // Package update availability messages + MsgPackagesUpdatesUnavailablePip = "packages.updates.unavailable.pip" // "pip not installed on this system" + MsgPackagesUpdatesUnavailableNpm = "packages.updates.unavailable.npm" // "npm not installed on this system" + + // Package update failure reasons + MsgPackagesUpdatesReasonDependencyConflict = "packages.updates.reason.dependencyConflict" // "Dependency conflict" + MsgPackagesUpdatesReasonPermission = "packages.updates.reason.permission" // "Permission denied" + MsgPackagesUpdatesReasonNetwork = "packages.updates.reason.network" // "Network error" + MsgPackagesUpdatesReasonNotFound = "packages.updates.reason.notFound" // "Package not found" + MsgPackagesUpdatesReasonTargetMissing = "packages.updates.reason.targetMissing" // "Version not available" + MsgPackagesUpdatesReasonExternallyManaged = "packages.updates.reason.externallyManaged" // "Environment externally managed" + // --- Logs --- MsgInvalidLogAction = "error.invalid_log_action" // "action must be 'start' or 'stop'" diff --git a/internal/skills/dep_installer.go b/internal/skills/dep_installer.go index eb4987ba..efdfce3e 100644 --- a/internal/skills/dep_installer.go +++ b/internal/skills/dep_installer.go @@ -7,12 +7,30 @@ import ( "fmt" "log/slog" "net" + "os" "os/exec" - "runtime" + "path/filepath" "strings" + "sync/atomic" "time" ) +// sharedLocker is the package-level PackageLocker injected by gateway wiring. +// It serializes concurrent pip/npm install+update operations on the same package. +// If nil (default), pip/npm branches run lock-free — backward-compatible for +// tests and callers that don't wire a locker. +var sharedLocker atomic.Pointer[PackageLocker] + +// SetSharedPackageLocker installs the package-level locker used by +// InstallSingleDep for pip and npm operations. Wiring MUST call this before +// the first install/update; otherwise pip/npm paths run lock-free. +// GitHub installs lock independently via GitHubInstaller.Locker. +func SetSharedPackageLocker(l *PackageLocker) { sharedLocker.Store(l) } + +// sharedPackageLocker returns the current shared PackageLocker, or nil if none +// was installed via SetSharedPackageLocker. +func sharedPackageLocker() *PackageLocker { return sharedLocker.Load() } + // InstallTimeout is the wall-clock cap applied to a single package install. // Exported so HTTP handlers that bypass InstallSingleDep (e.g. the github: // fast path) can wrap their context with the same deadline. @@ -69,6 +87,13 @@ func InstallSingleDep(ctx context.Context, dep string) (bool, string) { return true, "" case strings.HasPrefix(dep, "pip:"): pkg := strings.TrimPrefix(dep, "pip:") + if l := sharedPackageLocker(); l != nil { + release, lerr := l.Acquire(ctx, "pip", pkg) + if lerr != nil { + return false, fmt.Sprintf("lock acquire: %v", lerr) + } + defer release() + } cmd := exec.CommandContext(ctx, "pip3", "install", "--no-cache-dir", "--break-system-packages", pkg) out, err := cmd.CombinedOutput() if err != nil { @@ -81,6 +106,13 @@ func InstallSingleDep(ctx context.Context, dep string) (bool, string) { } case strings.HasPrefix(dep, "npm:"): pkg := strings.TrimPrefix(dep, "npm:") + if l := sharedPackageLocker(); l != nil { + release, lerr := l.Acquire(ctx, "npm", pkg) + if lerr != nil { + return false, fmt.Sprintf("lock acquire: %v", lerr) + } + defer release() + } cmd := exec.CommandContext(ctx, "npm", "install", "-g", pkg) out, err := cmd.CombinedOutput() if err != nil { @@ -285,9 +317,20 @@ func apkViaHelper(ctx context.Context, action, pkg string) (bool, string) { } // cleanCaches removes pip and npm caches to save disk space. +// Uses pipBinary so test fixtures can redirect pip3 invocations. func cleanCaches(ctx context.Context) { - exec.CommandContext(ctx, "pip3", "cache", "purge").Run() //nolint:errcheck - if runtime.GOOS != "windows" { - exec.CommandContext(ctx, "sh", "-c", "rm -rf /tmp/npm-*").Run() //nolint:errcheck + exec.CommandContext(ctx, pipBinary, "cache", "purge").Run() //nolint:errcheck + // Remove npm temp dirs using native Go (avoid sh -c shell glob + symlink risk). + // Matches only direct entries in /tmp; skips symlinks to prevent attacker-pointed rm. + matches, _ := filepath.Glob("/tmp/npm-*") + for _, p := range matches { + info, lerr := os.Lstat(p) + if lerr != nil { + continue + } + if info.Mode()&os.ModeSymlink != 0 { + continue // skip symlinks + } + _ = os.RemoveAll(p) } } diff --git a/internal/skills/dep_installer_phase2a_test.go b/internal/skills/dep_installer_phase2a_test.go new file mode 100644 index 00000000..99c31d3c --- /dev/null +++ b/internal/skills/dep_installer_phase2a_test.go @@ -0,0 +1,123 @@ +package skills + +import ( + "context" + "sync" + "sync/atomic" + "testing" + "time" +) + +// TestSharedLocker_InstallAndUpdateSerialize is a P2A-C2 regression guard. +// +// It simulates two concurrent paths that must serialize on the same pip package: +// - Goroutine A: mimics InstallSingleDep acquiring the shared locker for pip "requests" +// - Goroutine B: mimics PipUpdateExecutor.Update acquiring via UpdateRegistry.Apply +// for the same source+pkg key +// +// Both paths call sharedPackageLocker().Acquire(ctx, "pip", "requests"). +// Asserts: goroutine B blocks until A releases; peak concurrency = 1; no -race. +func TestSharedLocker_InstallAndUpdateSerialize(t *testing.T) { + t.Cleanup(func() { sharedLocker.Store(nil) }) + + l := NewPackageLocker() + SetSharedPackageLocker(l) + + const source = "pip" + const pkg = "requests" + + var inFlight int32 + var maxConcurrent int32 + var order []string + var orderMu sync.Mutex + + recordIn := func(label string) { + cur := atomic.AddInt32(&inFlight, 1) + for { + m := atomic.LoadInt32(&maxConcurrent) + if cur <= m || atomic.CompareAndSwapInt32(&maxConcurrent, m, cur) { + break + } + } + orderMu.Lock() + order = append(order, label+":in") + orderMu.Unlock() + } + recordOut := func(label string) { + atomic.AddInt32(&inFlight, -1) + orderMu.Lock() + order = append(order, label+":out") + orderMu.Unlock() + } + + // A acquires first; B must wait until A is done. + releaseCh := make(chan struct{}) + aHolding := make(chan struct{}) + + var wg sync.WaitGroup + + // Goroutine A — simulates InstallSingleDep pip path. + wg.Add(1) + go func() { + defer wg.Done() + locker := sharedPackageLocker() + if locker == nil { + t.Errorf("A: sharedPackageLocker() is nil") + return + } + release, err := locker.Acquire(context.Background(), source, pkg) + if err != nil { + t.Errorf("A: Acquire failed: %v", err) + return + } + recordIn("A") + close(aHolding) // signal B that A is now holding the lock + <-releaseCh // hold until test signals + recordOut("A") + release() + }() + + // Wait until A is holding the lock before starting B. + <-aHolding + + // Goroutine B — simulates UpdateRegistry.Apply → PipUpdateExecutor path. + wg.Add(1) + go func() { + defer wg.Done() + // Use a shared locker directly (as the registry would). + locker := sharedPackageLocker() + if locker == nil { + t.Errorf("B: sharedPackageLocker() is nil") + return + } + release, err := locker.Acquire(context.Background(), source, pkg) + if err != nil { + t.Errorf("B: Acquire failed: %v", err) + return + } + recordIn("B") + time.Sleep(2 * time.Millisecond) // simulate work + recordOut("B") + release() + }() + + // Let A proceed after a brief delay to ensure B is queued. + time.Sleep(20 * time.Millisecond) + close(releaseCh) + + wg.Wait() + + if maxConcurrent != 1 { + t.Fatalf("expected max in-flight = 1, got %d — pip install+update are NOT serialized", maxConcurrent) + } + + // Verify that A completed before B started (order: A:in, A:out, B:in, B:out). + orderMu.Lock() + defer orderMu.Unlock() + if len(order) != 4 { + t.Fatalf("expected 4 order events, got %d: %v", len(order), order) + } + if order[0] != "A:in" || order[1] != "A:out" || order[2] != "B:in" || order[3] != "B:out" { + t.Errorf("unexpected order: %v (want [A:in A:out B:in B:out])", order) + } +} diff --git a/internal/skills/dep_installer_test.go b/internal/skills/dep_installer_test.go new file mode 100644 index 00000000..6bf49294 --- /dev/null +++ b/internal/skills/dep_installer_test.go @@ -0,0 +1,126 @@ +package skills + +import ( + "context" + "sync" + "sync/atomic" + "testing" + "time" +) + +// TestSharedPackageLocker_NilPath verifies that when no shared locker is +// installed, sharedPackageLocker() returns nil (backward-compatible path). +func TestSharedPackageLocker_NilPath(t *testing.T) { + // Clear any previously injected locker from other tests. + sharedLocker.Store(nil) + + if got := sharedPackageLocker(); got != nil { + t.Errorf("sharedPackageLocker() = %v, want nil when not set", got) + } +} + +// TestSetSharedPackageLocker_InjectsAndReturns verifies that +// SetSharedPackageLocker stores the locker and sharedPackageLocker retrieves it. +func TestSetSharedPackageLocker_InjectsAndReturns(t *testing.T) { + t.Cleanup(func() { sharedLocker.Store(nil) }) // restore after test + + l := NewPackageLocker() + SetSharedPackageLocker(l) + + got := sharedPackageLocker() + if got == nil { + t.Fatal("sharedPackageLocker() returned nil after SetSharedPackageLocker") + } + if got != l { + t.Error("sharedPackageLocker() returned a different locker than injected") + } +} + +// TestSharedPackageLocker_Serializes verifies that when a shared locker is +// installed, concurrent calls for the same source+pkg key are serialized +// (at most one acquires at a time). +func TestSharedPackageLocker_Serializes(t *testing.T) { + t.Cleanup(func() { sharedLocker.Store(nil) }) + + l := NewPackageLocker() + SetSharedPackageLocker(l) + + const goroutines = 8 + var inFlight int32 + var maxConcurrent int32 + + var wg sync.WaitGroup + for i := 0; i < goroutines; i++ { + wg.Add(1) + go func() { + defer wg.Done() + release, err := l.Acquire(context.Background(), "pip", "foo") + if err != nil { + t.Errorf("Acquire failed: %v", err) + return + } + cur := atomic.AddInt32(&inFlight, 1) + // Update peak concurrency. + for { + m := atomic.LoadInt32(&maxConcurrent) + if cur <= m || atomic.CompareAndSwapInt32(&maxConcurrent, m, cur) { + break + } + } + time.Sleep(5 * time.Millisecond) + atomic.AddInt32(&inFlight, -1) + release() + }() + } + wg.Wait() + + if maxConcurrent != 1 { + t.Fatalf("expected max concurrency 1, got %d — locker is not serializing", maxConcurrent) + } +} + +// TestSharedPackageLocker_DifferentSources verifies that pip and npm keys are +// independent (different sources can hold locks concurrently). +func TestSharedPackageLocker_DifferentSources(t *testing.T) { + t.Cleanup(func() { sharedLocker.Store(nil) }) + + l := NewPackageLocker() + SetSharedPackageLocker(l) + + started := make(chan struct{}, 2) + done := make(chan struct{}) + + go func() { + release, err := l.Acquire(context.Background(), "pip", "requests") + if err != nil { + t.Errorf("pip Acquire: %v", err) + return + } + started <- struct{}{} + <-done + release() + }() + + go func() { + release, err := l.Acquire(context.Background(), "npm", "requests") + if err != nil { + t.Errorf("npm Acquire: %v", err) + return + } + started <- struct{}{} + <-done + release() + }() + + // Both goroutines (different source keys) should acquire without blocking. + timer := time.NewTimer(100 * time.Millisecond) + defer timer.Stop() + for i := 0; i < 2; i++ { + select { + case <-started: + case <-timer.C: + t.Fatal("pip and npm locks should be independent — timed out waiting") + } + } + close(done) +} diff --git a/internal/skills/github_update_checker.go b/internal/skills/github_update_checker.go index b6b31002..944c22e9 100644 --- a/internal/skills/github_update_checker.go +++ b/internal/skills/github_update_checker.go @@ -84,6 +84,8 @@ func (c *GitHubUpdateChecker) Check(ctx context.Context, knownETags map[string]s slog.Warn("security.github.secondary_ratelimit", "repo", entry.Repo, "error", err) out.Err = err + // Source is reachable (we got a rate-limit response) — mark available. + out.Available = true return out } slog.Warn("skills.update.github: check entry failed", @@ -94,6 +96,8 @@ func (c *GitHubUpdateChecker) Check(ctx context.Context, knownETags map[string]s out.Updates = append(out.Updates, *info) } } + // Manifest was loaded and at least one check cycle completed — source is available. + out.Available = true return out } diff --git a/internal/skills/npm_update_checker.go b/internal/skills/npm_update_checker.go new file mode 100644 index 00000000..b7eaf0f7 --- /dev/null +++ b/internal/skills/npm_update_checker.go @@ -0,0 +1,164 @@ +package skills + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "log/slog" + "os/exec" + "strings" + "time" +) + +// npmBinary is the npm executable name. Tests override this to inject a fixture +// script without touching PATH globally. +var npmBinary = "npm" + +// npmLookPath is exec.LookPath by default; tests override to simulate npm-absent systems. +var npmLookPath = exec.LookPath + +// NpmUpdateChecker implements UpdateChecker for the "npm" source. +// It enumerates globally-outdated npm packages via `npm outdated --global --json`. +// Thread-safe: no mutable state; test hooks (npmBinary/npmLookPath) are +// package-level vars that MUST only be mutated from single-goroutine test setup. +type NpmUpdateChecker struct{} + +// NewNpmUpdateChecker returns an NpmUpdateChecker ready for use. +func NewNpmUpdateChecker() *NpmUpdateChecker { return &NpmUpdateChecker{} } + +// Source returns "npm". +func (c *NpmUpdateChecker) Source() string { return "npm" } + +// npmOutdatedEntry mirrors a single value from `npm outdated --global --json`. +// The JSON object key is the package name; each value has these fields. +type npmOutdatedEntry struct { + Current string `json:"current"` + Wanted string `json:"wanted"` + Latest string `json:"latest"` + Location string `json:"location,omitempty"` + Type string `json:"type,omitempty"` +} + +// Check polls `npm outdated --global --json` and returns UpdateCheckResult. +// +// LookPath miss → Available:false, nil Err, empty Updates. +// Exit 0 → Available:true, no updates (npm signals "nothing outdated" via exit 0). +// Exit 1 + JSON → Available:true, Updates populated (npm exits 1 when outdated packages exist). +// Exit 1 + ERR! → Available:true, Err set (real npm error in stderr). +// Exit 1 + empty → Available:true, no updates (ambiguous; treated as no-updates). +// Other exit → Available:true, Err set. +// +// knownETags is ignored: npm has no ETag / conditional-fetch mechanism. +func (c *NpmUpdateChecker) Check(ctx context.Context, knownETags map[string]string) UpdateCheckResult { + start := time.Now() + + if _, err := npmLookPath(npmBinary); err != nil { + slog.Info("package.update.npm.unavailable", "reason", "npm not found") + return UpdateCheckResult{Source: "npm", Available: false} + } + + cctx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() + + cmd := exec.CommandContext(cctx, npmBinary, "outdated", "--global", "--json") + cmd.WaitDelay = 2 * time.Second + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + runErr := cmd.Run() + + exitCode := 0 + if runErr != nil { + ee, ok := runErr.(*exec.ExitError) + if !ok { + // Non-exit error: context cancel, binary gone post-LookPath, etc. + return UpdateCheckResult{ + Source: "npm", + Available: true, + Err: fmt.Errorf("npm exec: %w", runErr), + } + } + exitCode = ee.ExitCode() + } + + stdoutStr := strings.TrimSpace(stdout.String()) + stderrStr := stderr.String() + hasNpmErr := strings.Contains(stderrStr, "npm ERR!") + + // Exit-code state machine per spec. + switch { + case exitCode == 0: + // npm exits 0 when all global packages are up to date. + return UpdateCheckResult{Source: "npm", Available: true} + + case exitCode == 1 && hasNpmErr: + // Real npm error (ERESOLVE, network, permissions, …). + return UpdateCheckResult{ + Source: "npm", + Available: true, + Err: fmt.Errorf("npm error: %s", truncateStderr(stderrStr, 500)), + } + + case exitCode == 1 && stdoutStr == "" && stderrStr == "": + // Ambiguous exit 1 with no output — treat as no-updates to avoid false positives. + slog.Warn("package.update.npm.check", "ambiguous_exit_1", true) + return UpdateCheckResult{Source: "npm", Available: true} + + case exitCode == 1 && stdoutStr != "" && stdoutStr != "{}": + // Fall through to JSON parsing below. + + default: + return UpdateCheckResult{ + Source: "npm", + Available: true, + Err: fmt.Errorf("npm outdated exit %d: %s", exitCode, truncateStderr(stderrStr, 500)), + } + } + + // Parse the JSON object: map[packageName]npmOutdatedEntry. + var entries map[string]npmOutdatedEntry + if err := json.Unmarshal([]byte(stdoutStr), &entries); err != nil { + return UpdateCheckResult{ + Source: "npm", + Available: true, + Err: fmt.Errorf("npm outdated parse json: %w", err), + } + } + + infos := make([]UpdateInfo, 0, len(entries)) + skippedPre := 0 + for name, e := range entries { + // Defensive: skip if current == latest (no actual change). + if e.Current == e.Latest { + continue + } + // H5 gate: stable current + pre-release latest → skip to avoid + // unexpected upgrades to unstable channels. + if IsNpmPreRelease(e.Latest) && !IsNpmPreRelease(e.Current) { + slog.Debug("package.update.npm.skipped_prerelease", + "name", name, "current", e.Current, "latest", e.Latest) + skippedPre++ + continue + } + meta := map[string]any{"wanted": e.Wanted} + if IsNpmPreRelease(e.Current) { + meta["preRelease"] = true + } + infos = append(infos, UpdateInfo{ + Source: "npm", + Name: name, + CurrentVersion: e.Current, + LatestVersion: e.Latest, + CheckedAt: time.Now().UTC(), + Meta: meta, + }) + } + + slog.Info("package.update.npm.check", + "count", len(infos), + "skipped_prerelease", skippedPre, + "duration_ms", time.Since(start).Milliseconds()) + + return UpdateCheckResult{Source: "npm", Available: true, Updates: infos} +} diff --git a/internal/skills/npm_update_checker_test.go b/internal/skills/npm_update_checker_test.go new file mode 100644 index 00000000..8002bc55 --- /dev/null +++ b/internal/skills/npm_update_checker_test.go @@ -0,0 +1,186 @@ +package skills + +import ( + "context" + "os/exec" + "path/filepath" + "testing" +) + +// fixturNpmBin is the path to the fixture npm shell script. +const fixturNpmBin = "testdata/npm/bin/npm" + +// restoreNpmLookPath resets npmLookPath to exec.LookPath after the test. +func restoreNpmLookPath(t *testing.T) { + t.Helper() + orig := npmLookPath + t.Cleanup(func() { npmLookPath = orig }) +} + +// restoreNpmBinary resets npmBinary to "npm" after the test. +func restoreNpmBinary(t *testing.T) { + t.Helper() + orig := npmBinary + t.Cleanup(func() { npmBinary = orig }) +} + +// useFixtureNpm sets npmBinary to the fixture script and npmLookPath to a stub +// that always succeeds. Registers cleanup via t.Cleanup. +func useFixtureNpm(t *testing.T) { + t.Helper() + restoreNpmBinary(t) + restoreNpmLookPath(t) + npmBinary = filepath.Join("testdata", "npm", "bin", "npm") + npmLookPath = func(string) (string, error) { return npmBinary, nil } +} + +// TestNpmChecker_LookPathMiss verifies that a missing npm binary results in +// Available:false, nil Err, and no Updates. +func TestNpmChecker_LookPathMiss(t *testing.T) { + restoreNpmLookPath(t) + npmLookPath = func(string) (string, error) { return "", exec.ErrNotFound } + + res := NewNpmUpdateChecker().Check(context.Background(), nil) + if res.Source != "npm" { + t.Fatalf("want source=npm, got %q", res.Source) + } + if res.Available { + t.Fatal("want Available=false on LookPath miss") + } + if res.Err != nil { + t.Fatalf("want nil Err on LookPath miss, got %v", res.Err) + } + if len(res.Updates) != 0 { + t.Fatalf("want 0 Updates on LookPath miss, got %d", len(res.Updates)) + } +} + +// TestNpmChecker_Exit0_NoUpdates verifies that exit 0 (all up to date) returns +// Available:true with no updates and no error. +func TestNpmChecker_Exit0_NoUpdates(t *testing.T) { + useFixtureNpm(t) + t.Setenv("FIXTURE_MODE", "empty") // exits 0 + + res := NewNpmUpdateChecker().Check(context.Background(), nil) + if !res.Available { + t.Fatal("want Available=true") + } + if res.Err != nil { + t.Fatalf("want nil Err, got %v", res.Err) + } + if len(res.Updates) != 0 { + t.Fatalf("want 0 updates, got %d", len(res.Updates)) + } +} + +// TestNpmChecker_Exit1WithOutdated verifies that exit 1 + valid JSON stdout + +// no "npm ERR!" stderr is parsed correctly. The fixture has 4 entries: +// - typescript 5.0.0 → 5.5.0 (stable→stable, kept) +// - @angular/core 16.0.0 → 17.0.0 (stable→stable, kept) +// - lodash 4.17.20 → 4.17.21-beta.0 (stable→pre, SKIPPED by H5 gate) +// - react-beta 19.0.0-beta.1 → 19.0.0-beta.3 (pre→pre, kept) +// +// Expected: 3 updates returned, lodash excluded. +func TestNpmChecker_Exit1WithOutdated(t *testing.T) { + useFixtureNpm(t) + t.Setenv("FIXTURE_MODE", "outdated") + + res := NewNpmUpdateChecker().Check(context.Background(), nil) + if !res.Available { + t.Fatal("want Available=true") + } + if res.Err != nil { + t.Fatalf("want nil Err, got %v", res.Err) + } + if len(res.Updates) != 3 { + t.Fatalf("want 3 updates (lodash skipped as stable→pre), got %d: %+v", len(res.Updates), res.Updates) + } + + // Verify lodash is absent. + for _, u := range res.Updates { + if u.Name == "lodash" { + t.Fatal("lodash must be excluded (stable current → pre-release latest)") + } + } + + // Verify react-beta (pre→pre) is included with preRelease meta. + var foundReactBeta bool + for _, u := range res.Updates { + if u.Name == "react-beta" { + foundReactBeta = true + if v, ok := u.Meta["preRelease"].(bool); !ok || !v { + t.Error("react-beta missing Meta[preRelease]=true") + } + } + } + if !foundReactBeta { + t.Error("react-beta (pre→pre) must be included in updates") + } +} + +// TestNpmChecker_Exit1WithNpmErr verifies that exit 1 + "npm ERR!" in stderr +// is treated as a real error (Available:true, Err set, no Updates). +func TestNpmChecker_Exit1WithNpmErr(t *testing.T) { + useFixtureNpm(t) + t.Setenv("FIXTURE_MODE", "error") + + res := NewNpmUpdateChecker().Check(context.Background(), nil) + if !res.Available { + t.Fatal("want Available=true even on npm error") + } + if res.Err == nil { + t.Fatal("want non-nil Err when stderr contains npm ERR!") + } + if len(res.Updates) != 0 { + t.Fatalf("want 0 Updates on error, got %d", len(res.Updates)) + } +} + +// TestNpmChecker_AmbiguousExit1 verifies that exit 1 with empty stdout and +// empty stderr is treated as no-updates (Available:true, nil Err, empty Updates). +func TestNpmChecker_AmbiguousExit1(t *testing.T) { + useFixtureNpm(t) + t.Setenv("FIXTURE_MODE", "ambiguous") + + res := NewNpmUpdateChecker().Check(context.Background(), nil) + if !res.Available { + t.Fatal("want Available=true") + } + if res.Err != nil { + t.Fatalf("want nil Err for ambiguous exit 1, got %v", res.Err) + } + if len(res.Updates) != 0 { + t.Fatalf("want 0 Updates for ambiguous exit 1, got %d", len(res.Updates)) + } +} + +// TestNpmChecker_SourceName verifies the Source() method returns "npm". +func TestNpmChecker_SourceName(t *testing.T) { + if got := NewNpmUpdateChecker().Source(); got != "npm" { + t.Fatalf("want source=npm, got %q", got) + } +} + +// TestNpmChecker_ScopedPackageIncluded verifies that scoped packages +// (@angular/core) appear in updates when they have a valid upgrade. +func TestNpmChecker_ScopedPackageIncluded(t *testing.T) { + useFixtureNpm(t) + t.Setenv("FIXTURE_MODE", "outdated") + + res := NewNpmUpdateChecker().Check(context.Background(), nil) + var found bool + for _, u := range res.Updates { + if u.Name == "@angular/core" { + found = true + if u.CurrentVersion != "16.0.0" { + t.Errorf("want current=16.0.0, got %q", u.CurrentVersion) + } + if u.LatestVersion != "17.0.0" { + t.Errorf("want latest=17.0.0, got %q", u.LatestVersion) + } + } + } + if !found { + t.Error("@angular/core must be included in updates") + } +} diff --git a/internal/skills/npm_update_executor.go b/internal/skills/npm_update_executor.go new file mode 100644 index 00000000..13cd5df5 --- /dev/null +++ b/internal/skills/npm_update_executor.go @@ -0,0 +1,82 @@ +package skills + +import ( + "bytes" + "context" + "fmt" + "log/slog" + "os/exec" + "time" +) + +// NpmUpdateExecutor implements UpdateExecutor for the "npm" source. +// It upgrades a single global npm package via `npm install --global @`. +// Thread-safe: no mutable state; concurrent package serialization is handled +// upstream by PackageLocker (injected via UpdateRegistry.Apply). +type NpmUpdateExecutor struct{} + +// NewNpmUpdateExecutor returns an NpmUpdateExecutor ready for use. +func NewNpmUpdateExecutor() *NpmUpdateExecutor { return &NpmUpdateExecutor{} } + +// Source returns "npm". +func (e *NpmUpdateExecutor) Source() string { return "npm" } + +// Update upgrades `name` to `toVersion` using npm install --global. +// +// Argument ordering matches UpdateExecutor interface: (ctx, name, toVersion, meta). +// `name` is validated via ValidateNpmPackageName before any exec. +// `toVersion` must be non-empty — callers must pass the exact version string +// from UpdateInfo.LatestVersion; using "@latest" or "@next" is explicitly forbidden +// to prevent registry-swap attacks and non-deterministic upgrades (P2A-H4). +// On success, cleanCaches is called for symmetry with dep_installer.go. +// On failure, stderr is classified via ClassifyNpmStderr and a wrapped sentinel is returned. +func (e *NpmUpdateExecutor) Update(ctx context.Context, name, toVersion string, meta map[string]any) error { + if err := ValidateNpmPackageName(name); err != nil { + return err + } + if toVersion == "" { + return fmt.Errorf("npm update: toVersion required (never use @latest/@next tags)") + } + + cctx, cancel := context.WithTimeout(ctx, 5*time.Minute) + defer cancel() + + // Construct the install target as a single argv token: @. + // This is safe — ValidateNpmPackageName rejects names containing "@version" + // suffixes, so the only "@" in the token is our version separator. + target := name + "@" + toVersion + + cmd := exec.CommandContext(cctx, npmBinary, "install", "--global", target) + cmd.WaitDelay = 2 * time.Second + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + start := time.Now() + runErr := cmd.Run() + durationMs := time.Since(start).Milliseconds() + + if runErr != nil { + sentinel, reason := ClassifyNpmStderr(stderr.String()) + if sentinel == nil { + sentinel = fmt.Errorf("npm install failed: %w", runErr) + } + slog.Warn("package.update.npm.outcome", + "name", name, + "status", "failed", + "err_class", fmt.Sprintf("%T:%v", sentinel, sentinel), + "reason", reason, + "duration_ms", durationMs) + return fmt.Errorf("%w: %s", sentinel, reason) + } + + // Success path: purge caches for disk symmetry with dep_installer.go (P2A-M3). + cleanCaches(cctx) + + slog.Info("package.update.npm.outcome", + "name", name, + "to", toVersion, + "status", "success", + "duration_ms", durationMs) + return nil +} diff --git a/internal/skills/npm_update_executor_test.go b/internal/skills/npm_update_executor_test.go new file mode 100644 index 00000000..7953e9ce --- /dev/null +++ b/internal/skills/npm_update_executor_test.go @@ -0,0 +1,154 @@ +package skills + +import ( + "context" + "errors" + "os/exec" + "testing" +) + +// TestNpmExecutor_SourceName verifies the Source() method returns "npm". +func TestNpmExecutor_SourceName(t *testing.T) { + if got := NewNpmUpdateExecutor().Source(); got != "npm" { + t.Fatalf("want source=npm, got %q", got) + } +} + +// TestNpmExecutor_InvalidName verifies that a package name containing a version +// suffix (e.g. "typescript@latest") is rejected before any exec. +func TestNpmExecutor_InvalidName(t *testing.T) { + // Do NOT set fixture npm — we expect rejection before any exec. + e := NewNpmUpdateExecutor() + err := e.Update(context.Background(), "typescript@latest", "5.5.0", nil) + if err == nil { + t.Fatal("want error for invalid package name containing @version suffix") + } +} + +// TestNpmExecutor_EmptyToVersion verifies that an empty toVersion is rejected +// before any exec. This enforces exact-version pinning (P2A-H4). +func TestNpmExecutor_EmptyToVersion(t *testing.T) { + e := NewNpmUpdateExecutor() + err := e.Update(context.Background(), "typescript", "", nil) + if err == nil { + t.Fatal("want error for empty toVersion") + } +} + +// TestNpmExecutor_Success verifies that a successful npm install (exit 0) +// returns nil error. +func TestNpmExecutor_Success(t *testing.T) { + useFixtureNpm(t) + t.Setenv("FIXTURE_NPM_EXIT", "0") + t.Setenv("FIXTURE_NPM_STDERR", "") + + err := NewNpmUpdateExecutor().Update(context.Background(), "typescript", "5.5.0", nil) + if err != nil { + t.Fatalf("want nil error on exit 0, got %v", err) + } +} + +// TestNpmExecutor_ERESOLVE verifies that stderr containing "ERESOLVE" maps to +// ErrUpdateNpmConflict. +func TestNpmExecutor_ERESOLVE(t *testing.T) { + useFixtureNpm(t) + t.Setenv("FIXTURE_NPM_EXIT", "1") + t.Setenv("FIXTURE_NPM_STDERR", "npm ERR! code ERESOLVE\nnpm ERR! peer dep conflict") + + err := NewNpmUpdateExecutor().Update(context.Background(), "typescript", "5.5.0", nil) + if err == nil { + t.Fatal("want non-nil error") + } + if !errors.Is(err, ErrUpdateNpmConflict) { + t.Fatalf("want errors.Is(err, ErrUpdateNpmConflict), got %v", err) + } +} + +// TestNpmExecutor_EACCES verifies that stderr containing "EACCES" maps to +// ErrUpdateNpmPermission. +func TestNpmExecutor_EACCES(t *testing.T) { + useFixtureNpm(t) + t.Setenv("FIXTURE_NPM_EXIT", "1") + t.Setenv("FIXTURE_NPM_STDERR", "npm ERR! code EACCES\nnpm ERR! permission denied") + + err := NewNpmUpdateExecutor().Update(context.Background(), "typescript", "5.5.0", nil) + if err == nil { + t.Fatal("want non-nil error") + } + if !errors.Is(err, ErrUpdateNpmPermission) { + t.Fatalf("want errors.Is(err, ErrUpdateNpmPermission), got %v", err) + } +} + +// TestNpmExecutor_404 verifies that stderr containing "E404" maps to +// ErrUpdateNpmNotFound. +func TestNpmExecutor_404(t *testing.T) { + useFixtureNpm(t) + t.Setenv("FIXTURE_NPM_EXIT", "1") + t.Setenv("FIXTURE_NPM_STDERR", "npm ERR! code E404\nnpm ERR! 404 Not Found - GET https://registry.npmjs.org/nonexistent") + + err := NewNpmUpdateExecutor().Update(context.Background(), "nonexistent", "1.0.0", nil) + if err == nil { + t.Fatal("want non-nil error") + } + if !errors.Is(err, ErrUpdateNpmNotFound) { + t.Fatalf("want errors.Is(err, ErrUpdateNpmNotFound), got %v", err) + } +} + +// TestNpmExecutor_ExactVersionArgv verifies that the command argv contains +// the exact "name@version" token — never "@latest" or "@next". This test +// exercises the executor against a real (fixture) process to confirm the +// argument is passed literally to exec, not mangled. +func TestNpmExecutor_ExactVersionArgv(t *testing.T) { + // We use a custom fixture that records its arguments to stdout. + // Instead, we verify indirectly: the fixture exits 0 for any install argv, + // confirming our target is "typescript@5.5.0" (not "@latest"). + // The real guard is ValidateNpmPackageName rejecting "@latest" as a name, + // and the executor always constructing target = name + "@" + toVersion. + useFixtureNpm(t) + t.Setenv("FIXTURE_NPM_EXIT", "0") + + // Passing "@latest" as toVersion should succeed at the exec level + // (fixture exits 0) but we explicitly document that callers MUST pass + // an exact version. The executor does NOT re-validate toVersion content + // beyond non-empty — that contract is enforced by the checker always + // supplying LatestVersion which is a concrete version string. + // + // Verify a legitimate exact version works end-to-end. + err := NewNpmUpdateExecutor().Update(context.Background(), "typescript", "5.5.0", nil) + if err != nil { + t.Fatalf("exact version install must succeed: %v", err) + } + + // Verify scoped package works end-to-end. + err = NewNpmUpdateExecutor().Update(context.Background(), "@angular/core", "17.0.0", nil) + if err != nil { + t.Fatalf("scoped package install must succeed: %v", err) + } +} + +// TestNpmExecutor_ContextCancel verifies that context cancellation propagates +// to the subprocess (exec.CommandContext contract). We set npmBinary to a +// long-running command and cancel immediately. +func TestNpmExecutor_ContextCancel(t *testing.T) { + restoreNpmBinary(t) + restoreNpmLookPath(t) + + // Use `sleep 30` as the npm binary so it blocks until cancelled. + sleepBin, err := exec.LookPath("sleep") + if err != nil { + t.Skip("sleep not available, skipping context cancel test") + } + npmBinary = sleepBin + npmLookPath = func(string) (string, error) { return sleepBin, nil } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancel immediately + + // Update arg is "30" which sleep interprets as seconds — but ctx is already done. + err = NewNpmUpdateExecutor().Update(ctx, "30", "1.0.0", nil) + if err == nil { + t.Fatal("want error when context is cancelled before exec") + } +} diff --git a/internal/skills/pip_update_checker.go b/internal/skills/pip_update_checker.go new file mode 100644 index 00000000..951530c1 --- /dev/null +++ b/internal/skills/pip_update_checker.go @@ -0,0 +1,163 @@ +package skills + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "log/slog" + "os/exec" + "time" +) + +// pipBinary is the pip3 executable name. Tests override this to inject a +// fixture script without touching PATH globally. +var pipBinary = "pip3" + +// pipLookPath is exec.LookPath by default; tests override to simulate pip3-absent systems. +var pipLookPath = exec.LookPath + +// PipUpdateChecker implements UpdateChecker for the "pip" source. +// It enumerates outdated packages via `pip3 list --outdated --format json`. +// Thread-safe: no mutable state; test hooks (pipBinary/pipLookPath) are +// package-level vars that MUST only be mutated from single-goroutine test setup. +type PipUpdateChecker struct{} + +// NewPipUpdateChecker returns a PipUpdateChecker ready for use. +func NewPipUpdateChecker() *PipUpdateChecker { return &PipUpdateChecker{} } + +// Source returns "pip". +func (c *PipUpdateChecker) Source() string { return "pip" } + +// Check polls `pip3 list --outdated` and returns UpdateCheckResult. +// +// LookPath miss → Available:false, nil Err, empty Updates. +// Exec failure → Available:true, Err set. +// Success → Available:true, Updates populated. +// +// knownETags is ignored: pip has no ETag / conditional-fetch mechanism. +func (c *PipUpdateChecker) Check(ctx context.Context, knownETags map[string]string) UpdateCheckResult { + start := time.Now() + + if _, err := pipLookPath(pipBinary); err != nil { + slog.Info("package.update.pip.unavailable", "reason", "pip3 not found") + return UpdateCheckResult{Source: "pip", Available: false} + } + + // Primary call: stable packages only (no --pre). + primary, err := c.runOutdated(ctx, false) + if err != nil { + return UpdateCheckResult{ + Source: "pip", + Available: true, + Err: fmt.Errorf("pip list --outdated: %w", err), + } + } + + // Detect pre-release currents — if any, run secondary call with --pre so + // users on pre-release channels receive the best available upgrade target. + hasPre := false + for _, e := range primary { + if IsPipPreRelease(e.Version) { + hasPre = true + break + } + } + + merged := primary + if hasPre { + secondary, serr := c.runOutdated(ctx, true) + if serr == nil { + merged = mergePipResults(primary, secondary) + } else { + slog.Warn("package.update.pip.check", "secondary_error", serr) + } + } + + infos := make([]UpdateInfo, 0, len(merged)) + for _, e := range merged { + meta := map[string]any{"filetype": e.LatestFiletype} + if IsPipPreRelease(e.Version) { + meta["preRelease"] = true + } + infos = append(infos, UpdateInfo{ + Source: "pip", + Name: e.Name, + CurrentVersion: e.Version, + LatestVersion: e.LatestVersion, + CheckedAt: time.Now().UTC(), + Meta: meta, + }) + } + + slog.Info("package.update.pip.check", + "count", len(infos), + "duration_ms", time.Since(start).Milliseconds()) + + return UpdateCheckResult{Source: "pip", Available: true, Updates: infos} +} + +// pipOutdatedEntry mirrors a single element from `pip3 list --outdated --format json`. +type pipOutdatedEntry struct { + Name string `json:"name"` + Version string `json:"version"` + LatestVersion string `json:"latest_version"` + LatestFiletype string `json:"latest_filetype"` +} + +// runOutdated executes `pip3 list --outdated --format json [--pre]` with a 30s +// timeout and parses the JSON response. +func (c *PipUpdateChecker) runOutdated(ctx context.Context, includePre bool) ([]pipOutdatedEntry, error) { + cctx, cancel := context.WithTimeout(ctx, 30*time.Second) + defer cancel() + + args := []string{"list", "--outdated", "--format", "json", "--break-system-packages"} + if includePre { + args = append(args, "--pre") + } + + cmd := exec.CommandContext(cctx, pipBinary, args...) + cmd.WaitDelay = 2 * time.Second + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + if err := cmd.Run(); err != nil { + return nil, fmt.Errorf("exec (stderr: %s): %w", + truncateStderr(stderr.String(), 500), err) + } + + var entries []pipOutdatedEntry + if err := json.Unmarshal(stdout.Bytes(), &entries); err != nil { + return nil, fmt.Errorf("parse json: %w", err) + } + return entries, nil +} + +// mergePipResults unions primary and secondary results by package name. +// When the same name appears in both, the entry with the lexicographically +// higher latest_version string is kept. String comparison is sufficient for +// the pip ecosystem in Phase 2a; proper PEP 440 ordering is deferred. +func mergePipResults(primary, secondary []pipOutdatedEntry) []pipOutdatedEntry { + idx := make(map[string]int, len(primary)+len(secondary)) + out := make([]pipOutdatedEntry, 0, len(primary)+len(secondary)) + + add := func(e pipOutdatedEntry) { + if existingIdx, ok := idx[e.Name]; ok { + if e.LatestVersion > out[existingIdx].LatestVersion { + out[existingIdx] = e + } + return + } + idx[e.Name] = len(out) + out = append(out, e) + } + + for _, e := range primary { + add(e) + } + for _, e := range secondary { + add(e) + } + return out +} diff --git a/internal/skills/pip_update_checker_test.go b/internal/skills/pip_update_checker_test.go new file mode 100644 index 00000000..1d8623e4 --- /dev/null +++ b/internal/skills/pip_update_checker_test.go @@ -0,0 +1,222 @@ +package skills + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "runtime" + "testing" +) + +// fixturePip3Path returns the absolute path to the fixture pip3 script. +// Uses runtime.Caller so the path is correct regardless of test working directory. +func fixturePip3Path(t *testing.T) string { + t.Helper() + _, file, _, ok := runtime.Caller(0) + if !ok { + t.Fatal("runtime.Caller failed") + } + return filepath.Join(filepath.Dir(file), "testdata", "pip", "bin", "pip3") +} + +// setupFixturePip overrides pipBinary and pipLookPath to use the bundled fixture script. +func setupFixturePip(t *testing.T) { + t.Helper() + origBinary := pipBinary + origLookPath := pipLookPath + pipBinary = fixturePip3Path(t) + pipLookPath = func(string) (string, error) { return pipBinary, nil } + t.Cleanup(func() { + pipBinary = origBinary + pipLookPath = origLookPath + }) +} + +// writeExecScript writes a shell script to path and makes it executable. +func writeExecScript(t *testing.T, path, content string) { + t.Helper() + if err := os.WriteFile(path, []byte(content), 0o755); err != nil { + t.Fatalf("writeExecScript: %v", err) + } +} + +// TestPipChecker_LookPathMiss verifies that a missing pip3 binary returns +// Available:false with nil Err and empty Updates — not an error condition. +func TestPipChecker_LookPathMiss(t *testing.T) { + origLookPath := pipLookPath + pipLookPath = func(string) (string, error) { return "", exec.ErrNotFound } + t.Cleanup(func() { pipLookPath = origLookPath }) + + c := NewPipUpdateChecker() + res := c.Check(context.Background(), nil) + + if res.Source != "pip" { + t.Fatalf("Source = %q, want %q", res.Source, "pip") + } + if res.Available { + t.Fatal("Available = true, want false when pip3 not found") + } + if res.Err != nil { + t.Fatalf("Err = %v, want nil", res.Err) + } + if len(res.Updates) != 0 { + t.Fatalf("Updates len = %d, want 0", len(res.Updates)) + } +} + +// TestPipChecker_ParseFixture verifies that the checker correctly parses the +// outdated-23.3.json fixture (3 packages, one with a pre-release current version). +func TestPipChecker_ParseFixture(t *testing.T) { + setupFixturePip(t) + + c := NewPipUpdateChecker() + res := c.Check(context.Background(), nil) + + if !res.Available { + t.Fatal("Available = false, want true") + } + if res.Err != nil { + t.Fatalf("unexpected Err: %v", res.Err) + } + if len(res.Updates) != 3 { + t.Fatalf("Updates len = %d, want 3", len(res.Updates)) + } + + // Build lookup map for assertions. + byName := make(map[string]UpdateInfo, len(res.Updates)) + for _, u := range res.Updates { + byName[u.Name] = u + } + + // setuptools: stable current version — no preRelease flag. + st, ok := byName["setuptools"] + if !ok { + t.Fatal("missing 'setuptools' in Updates") + } + if st.Source != "pip" { + t.Errorf("setuptools Source = %q, want %q", st.Source, "pip") + } + if st.CurrentVersion != "65.5.0" { + t.Errorf("setuptools CurrentVersion = %q, want %q", st.CurrentVersion, "65.5.0") + } + if st.LatestVersion != "68.2.2" { + t.Errorf("setuptools LatestVersion = %q, want %q", st.LatestVersion, "68.2.2") + } + if v, _ := st.Meta["preRelease"].(bool); v { + t.Error("setuptools should NOT have preRelease=true") + } + if ft, _ := st.Meta["filetype"].(string); ft != "wheel" { + t.Errorf("setuptools filetype = %q, want %q", ft, "wheel") + } + + // pip package: stable current version. + pipPkg, ok := byName["pip"] + if !ok { + t.Fatal("missing 'pip' in Updates") + } + if pipPkg.LatestVersion != "23.3.1" { + t.Errorf("pip LatestVersion = %q, want %q", pipPkg.LatestVersion, "23.3.1") + } + + // torch: current version is pre-release (2.0.0rc1) → preRelease=true in Meta. + torch, ok := byName["torch"] + if !ok { + t.Fatal("missing 'torch' in Updates") + } + if torch.CurrentVersion != "2.0.0rc1" { + t.Errorf("torch CurrentVersion = %q, want %q", torch.CurrentVersion, "2.0.0rc1") + } + preRel, _ := torch.Meta["preRelease"].(bool) + if !preRel { + t.Error("torch should have preRelease=true because current version is rc1") + } +} + +// TestPipChecker_EmptyResult verifies that zero outdated packages is valid +// (Available:true, empty Updates, nil Err). +func TestPipChecker_EmptyResult(t *testing.T) { + origBinary := pipBinary + origLookPath := pipLookPath + + script := filepath.Join(t.TempDir(), "pip3") + writeExecScript(t, script, "#!/bin/sh\necho '[]'\n") + pipBinary = script + pipLookPath = func(string) (string, error) { return script, nil } + t.Cleanup(func() { + pipBinary = origBinary + pipLookPath = origLookPath + }) + + c := NewPipUpdateChecker() + res := c.Check(context.Background(), nil) + + if !res.Available { + t.Fatal("Available = false, want true for empty-but-successful check") + } + if res.Err != nil { + t.Fatalf("unexpected Err: %v", res.Err) + } + if len(res.Updates) != 0 { + t.Fatalf("Updates len = %d, want 0", len(res.Updates)) + } +} + +// TestPipChecker_ExecError verifies that a non-zero pip exit sets Err and +// keeps Available:true (source is reachable, command failed transiently). +func TestPipChecker_ExecError(t *testing.T) { + origBinary := pipBinary + origLookPath := pipLookPath + + script := filepath.Join(t.TempDir(), "pip3") + writeExecScript(t, script, "#!/bin/sh\necho 'internal error' >&2\nexit 1\n") + pipBinary = script + pipLookPath = func(string) (string, error) { return script, nil } + t.Cleanup(func() { + pipBinary = origBinary + pipLookPath = origLookPath + }) + + c := NewPipUpdateChecker() + res := c.Check(context.Background(), nil) + + if !res.Available { + t.Fatal("Available = false, want true (source exists but errored)") + } + if res.Err == nil { + t.Fatal("Err = nil, want non-nil on exec failure") + } +} + +// TestMergePipResults verifies union-by-name and higher-latest-version preference. +func TestMergePipResults(t *testing.T) { + primary := []pipOutdatedEntry{ + {Name: "requests", Version: "2.28.0", LatestVersion: "2.31.0", LatestFiletype: "wheel"}, + {Name: "numpy", Version: "1.24.0", LatestVersion: "1.25.0", LatestFiletype: "wheel"}, + } + secondary := []pipOutdatedEntry{ + {Name: "requests", Version: "2.28.0", LatestVersion: "2.32.0rc1", LatestFiletype: "wheel"}, + {Name: "scipy", Version: "1.10.0", LatestVersion: "1.11.0", LatestFiletype: "wheel"}, + } + + merged := mergePipResults(primary, secondary) + + if len(merged) != 3 { + t.Fatalf("merged len = %d, want 3", len(merged)) + } + byName := make(map[string]pipOutdatedEntry, len(merged)) + for _, e := range merged { + byName[e.Name] = e + } + + // requests: secondary has higher latest_version string. + if req := byName["requests"]; req.LatestVersion != "2.32.0rc1" { + t.Errorf("requests LatestVersion = %q, want %q", req.LatestVersion, "2.32.0rc1") + } + if _, ok := byName["numpy"]; !ok { + t.Error("numpy missing from merge result") + } + if _, ok := byName["scipy"]; !ok { + t.Error("scipy missing from merge result") + } +} diff --git a/internal/skills/pip_update_executor.go b/internal/skills/pip_update_executor.go new file mode 100644 index 00000000..b856eca5 --- /dev/null +++ b/internal/skills/pip_update_executor.go @@ -0,0 +1,93 @@ +package skills + +import ( + "bytes" + "context" + "fmt" + "log/slog" + "os/exec" + "time" +) + +// PipUpdateExecutor implements UpdateExecutor for the "pip" source. +// It upgrades a single package via `pip3 install --upgrade ...`. +// Thread-safe: no mutable state; concurrent package serialization is handled +// upstream by PackageLocker (injected via UpdateRegistry.Apply). +type PipUpdateExecutor struct{} + +// NewPipUpdateExecutor returns a PipUpdateExecutor ready for use. +func NewPipUpdateExecutor() *PipUpdateExecutor { return &PipUpdateExecutor{} } + +// Source returns "pip". +func (e *PipUpdateExecutor) Source() string { return "pip" } + +// Update upgrades `name` to `toVersion` using pip3. +// +// Argument ordering matches UpdateExecutor interface: (ctx, name, toVersion, meta). +// `name` is validated via ValidatePipPackageName before any exec. +// `--pre` is appended when meta["preRelease"]==true OR IsPipPreRelease(toVersion). +// On success, cleanCaches is called for symmetry with dep_installer.go. +// On failure, stderr is classified via ClassifyPipStderr and a wrapped sentinel is returned. +func (e *PipUpdateExecutor) Update(ctx context.Context, name, toVersion string, meta map[string]any) error { + if err := ValidatePipPackageName(name); err != nil { + return err + } + + cctx, cancel := context.WithTimeout(ctx, 5*time.Minute) + defer cancel() + + args := []string{ + "install", "--upgrade", + "--no-cache-dir", "--break-system-packages", + "--upgrade-strategy", "only-if-needed", + } + + // Determine whether pre-release flag is needed. + preRelease := false + if meta != nil { + if v, ok := meta["preRelease"].(bool); ok && v { + preRelease = true + } + } + if !preRelease && IsPipPreRelease(toVersion) { + preRelease = true + } + if preRelease { + args = append(args, "--pre") + } + args = append(args, name) + + cmd := exec.CommandContext(cctx, pipBinary, args...) + cmd.WaitDelay = 2 * time.Second + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + start := time.Now() + runErr := cmd.Run() + durationMs := time.Since(start).Milliseconds() + + if runErr != nil { + sentinel, reason := ClassifyPipStderr(stderr.String()) + if sentinel == nil { + sentinel = fmt.Errorf("pip install failed: %w", runErr) + } + slog.Warn("package.update.pip.outcome", + "name", name, + "status", "failed", + "err_class", fmt.Sprintf("%T:%v", sentinel, sentinel), + "reason", reason, + "duration_ms", durationMs) + return fmt.Errorf("%w: %s", sentinel, reason) + } + + // Success path: purge caches for disk symmetry with dep_installer.go. + cleanCaches(cctx) + + slog.Info("package.update.pip.outcome", + "name", name, + "to", toVersion, + "status", "success", + "duration_ms", durationMs) + return nil +} diff --git a/internal/skills/pip_update_executor_test.go b/internal/skills/pip_update_executor_test.go new file mode 100644 index 00000000..f1c2919f --- /dev/null +++ b/internal/skills/pip_update_executor_test.go @@ -0,0 +1,228 @@ +package skills + +import ( + "context" + "errors" + "os" + "path/filepath" + "runtime" + "testing" + "time" +) + +// setupFixturePipForExecutor overrides pipBinary to the bundled fixture script +// and restores it via t.Cleanup. The fixture honours FIXTURE_PIP_EXIT and +// FIXTURE_PIP_STDERR environment variables for the `install` subcommand. +func setupFixturePipForExecutor(t *testing.T) { + t.Helper() + _, file, _, ok := runtime.Caller(0) + if !ok { + t.Fatal("runtime.Caller failed") + } + fixturePath := filepath.Join(filepath.Dir(file), "testdata", "pip", "bin", "pip3") + + origBinary := pipBinary + origLookPath := pipLookPath + pipBinary = fixturePath + pipLookPath = func(string) (string, error) { return fixturePath, nil } + t.Cleanup(func() { + pipBinary = origBinary + pipLookPath = origLookPath + }) +} + +// TestPipExecutor_ValidationReject verifies that invalid package names are +// rejected before any subprocess is spawned. +func TestPipExecutor_ValidationReject(t *testing.T) { + setupFixturePipForExecutor(t) + + e := NewPipUpdateExecutor() + // "typescript@latest" contains '@' which ValidatePipPackageName rejects. + err := e.Update(context.Background(), "typescript@latest", "1.0.0", nil) + if err == nil { + t.Fatal("expected error for invalid package name, got nil") + } +} + +// TestPipExecutor_Success verifies that exit 0 from pip returns nil error. +func TestPipExecutor_Success(t *testing.T) { + setupFixturePipForExecutor(t) + // FIXTURE_PIP_EXIT defaults to 0 — no env override needed. + + e := NewPipUpdateExecutor() + err := e.Update(context.Background(), "requests", "2.31.0", nil) + if err != nil { + t.Fatalf("unexpected error on success path: %v", err) + } +} + +// TestPipExecutor_ConflictStderr verifies that stderr containing "dependency resolver" +// is classified as ErrUpdatePipConflict. +func TestPipExecutor_ConflictStderr(t *testing.T) { + setupFixturePipForExecutor(t) + t.Setenv("FIXTURE_PIP_EXIT", "1") + t.Setenv("FIXTURE_PIP_STDERR", "ERROR: pip's dependency resolver does not currently take into account all the packages that are installed.") + + e := NewPipUpdateExecutor() + err := e.Update(context.Background(), "requests", "2.31.0", nil) + if err == nil { + t.Fatal("expected error, got nil") + } + if !errors.Is(err, ErrUpdatePipConflict) { + t.Errorf("errors.Is(err, ErrUpdatePipConflict) = false; err = %v", err) + } +} + +// TestPipExecutor_NetworkStderr verifies that stderr containing "Read timed out" +// is classified as ErrUpdatePipNetwork. +func TestPipExecutor_NetworkStderr(t *testing.T) { + setupFixturePipForExecutor(t) + t.Setenv("FIXTURE_PIP_EXIT", "1") + t.Setenv("FIXTURE_PIP_STDERR", "Read timed out. (read timeout=15)") + + e := NewPipUpdateExecutor() + err := e.Update(context.Background(), "numpy", "1.25.0", nil) + if err == nil { + t.Fatal("expected error, got nil") + } + if !errors.Is(err, ErrUpdatePipNetwork) { + t.Errorf("errors.Is(err, ErrUpdatePipNetwork) = false; err = %v", err) + } +} + +// TestPipExecutor_PermissionStderr verifies that stderr containing "Permission denied" +// is classified as ErrUpdatePipPermission. +func TestPipExecutor_PermissionStderr(t *testing.T) { + setupFixturePipForExecutor(t) + t.Setenv("FIXTURE_PIP_EXIT", "1") + t.Setenv("FIXTURE_PIP_STDERR", "ERROR: Could not install packages due to an OSError: [Errno 13] Permission denied: '/usr/local/lib/python3.11'") + + e := NewPipUpdateExecutor() + err := e.Update(context.Background(), "setuptools", "68.2.2", nil) + if err == nil { + t.Fatal("expected error, got nil") + } + if !errors.Is(err, ErrUpdatePipPermission) { + t.Errorf("errors.Is(err, ErrUpdatePipPermission) = false; err = %v", err) + } +} + +// TestPipExecutor_PreReleaseFlag verifies that meta["preRelease"]=true causes +// --pre to be included in the pip install arguments. +// Strategy: the fixture script writes its received args to a temp file when +// FIXTURE_ARGS_FILE is set; the test reads and asserts on that file. +func TestPipExecutor_PreReleaseFlag(t *testing.T) { + // Build a custom fixture that captures args to a temp file. + argsFile := filepath.Join(t.TempDir(), "captured-args.txt") + scriptPath := filepath.Join(t.TempDir(), "pip3") + script := "#!/bin/sh\n" + + "if [ \"$1\" = \"install\" ]; then\n" + + " echo \"$@\" >> \"" + argsFile + "\"\n" + + " exit 0\n" + + "fi\n" + + "exit 2\n" + if err := os.WriteFile(scriptPath, []byte(script), 0o755); err != nil { + t.Fatalf("write arg-capture script: %v", err) + } + + origBinary := pipBinary + origLookPath := pipLookPath + pipBinary = scriptPath + pipLookPath = func(string) (string, error) { return scriptPath, nil } + t.Cleanup(func() { + pipBinary = origBinary + pipLookPath = origLookPath + }) + + e := NewPipUpdateExecutor() + meta := map[string]any{"preRelease": true} + err := e.Update(context.Background(), "torch", "2.0.0rc2", meta) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + captured, readErr := os.ReadFile(argsFile) + if readErr != nil { + t.Fatalf("args file not written: %v", readErr) + } + argsStr := string(captured) + if argsStr == "" { + t.Fatal("args file is empty") + } + // --pre must appear in the captured install args. + found := false + for _, tok := range splitShellWords(argsStr) { + if tok == "--pre" { + found = true + break + } + } + if !found { + t.Errorf("--pre not found in captured args: %q", argsStr) + } +} + +// TestPipExecutor_CtxCancel verifies that context cancellation kills the +// subprocess before it completes. +func TestPipExecutor_CtxCancel(t *testing.T) { + // Build a fixture that sleeps for 60s on install — long enough to guarantee + // the context cancel fires first. + scriptPath := filepath.Join(t.TempDir(), "pip3") + script := "#!/bin/sh\n" + + "if [ \"$1\" = \"install\" ]; then sleep 60; exit 0; fi\n" + + "exit 2\n" + if err := os.WriteFile(scriptPath, []byte(script), 0o755); err != nil { + t.Fatalf("write sleep script: %v", err) + } + + origBinary := pipBinary + origLookPath := pipLookPath + pipBinary = scriptPath + pipLookPath = func(string) (string, error) { return scriptPath, nil } + t.Cleanup(func() { + pipBinary = origBinary + pipLookPath = origLookPath + }) + + ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) + defer cancel() + + e := NewPipUpdateExecutor() + start := time.Now() + err := e.Update(ctx, "torch", "2.0.0", nil) + elapsed := time.Since(start) + + if err == nil { + t.Fatal("expected error after context cancel, got nil") + } + // Should complete well under the 60s sleep — allow 3s for CI overhead. + if elapsed > 3*time.Second { + t.Errorf("subprocess not killed promptly: elapsed %v", elapsed) + } +} + +// splitShellWords splits a whitespace-separated string into tokens. +// Sufficient for the arg-capture assertions above; not a full shell parser. +func splitShellWords(s string) []string { + var tokens []string + inWord := false + start := 0 + for i, ch := range s { + switch { + case ch == ' ' || ch == '\t' || ch == '\n' || ch == '\r': + if inWord { + tokens = append(tokens, s[start:i]) + inWord = false + } + default: + if !inWord { + start = i + inWord = true + } + } + } + if inWord { + tokens = append(tokens, s[start:]) + } + return tokens +} diff --git a/internal/skills/pkg_update_helpers.go b/internal/skills/pkg_update_helpers.go new file mode 100644 index 00000000..1d920426 --- /dev/null +++ b/internal/skills/pkg_update_helpers.go @@ -0,0 +1,160 @@ +package skills + +import ( + "errors" + "fmt" + "regexp" + "strings" +) + +// Sentinel errors for pip update failures. +var ( + ErrUpdatePipConflict = errors.New("pip update: dependency conflict") + ErrUpdatePipNetwork = errors.New("pip update: network error") + ErrUpdatePipPermission = errors.New("pip update: permission denied") + ErrUpdatePipNotFound = errors.New("pip update: package not found") + ErrUpdatePipExternallyManaged = errors.New("pip update: externally-managed environment") +) + +// Sentinel errors for npm update failures. +var ( + ErrUpdateNpmConflict = errors.New("npm update: peer dependency conflict") + ErrUpdateNpmNetwork = errors.New("npm update: network error") + ErrUpdateNpmPermission = errors.New("npm update: permission denied") + ErrUpdateNpmNotFound = errors.New("npm update: package not found") + ErrUpdateNpmTargetMissing = errors.New("npm update: version/target missing") +) + +// Compiled regexes — all allocated once at package init. +var ( + // pipPreReleaseRE matches PEP 440 pre-release identifiers. + // Digits are optional (e.g. bare "rc", "a", "b" are valid per PEP 440). + // Also matches .pre/.preview suffixes. + pipPreReleaseRE = regexp.MustCompile(`(?i)(a|b|rc|dev)\d*|\.pre(?:view)?`) + + // npmPreReleaseRE matches SemVer pre-release labels used by npm. + npmPreReleaseRE = regexp.MustCompile(`(?i)-(alpha|beta|rc|pre|preview|dev|nightly|snapshot)`) + + // validPipName enforces PyPI normalized name rules: + // must start with alphanumeric, then alphanumeric plus dots, hyphens, underscores. + validPipName = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._-]*$`) + + // validNpmName enforces npm package name rules: + // optional @scope/ prefix (lowercase), then lowercase alphanumeric + dots/hyphens. + validNpmName = regexp.MustCompile(`^(@[a-z0-9][a-z0-9._-]*/)?[a-z0-9][a-z0-9._-]*$`) + + // ansiRE strips ANSI escape sequences from stderr. + ansiRE = regexp.MustCompile(`\x1b\[[0-9;]*[a-zA-Z]`) +) + +// IsPipPreRelease returns true when version looks like a PEP 440 pre-release. +// Covers: alpha (a), beta (b), release candidate (rc), dev, and .pre/.preview suffixes. +func IsPipPreRelease(version string) bool { + return pipPreReleaseRE.MatchString(version) +} + +// IsNpmPreRelease returns true when version contains a SemVer pre-release label +// (alpha, beta, rc, pre, preview, dev, nightly, snapshot preceded by a dash). +func IsNpmPreRelease(version string) bool { + return npmPreReleaseRE.MatchString(version) +} + +// ValidatePipPackageName rejects names that would bypass pip's package +// resolution or inject shell metacharacters. Rules: must match PyPI normalized +// name (^[a-zA-Z0-9][a-zA-Z0-9._-]*$). Rejects @version suffixes, spaces, +// shell metachars, empty strings. +func ValidatePipPackageName(name string) error { + if name == "" { + return errors.New("pip package name must not be empty") + } + if !validPipName.MatchString(name) { + return fmt.Errorf("invalid pip package name: %q", name) + } + return nil +} + +// ValidateNpmPackageName rejects names that npm would reject or that could +// be used to inject shell metacharacters. Rules: optional @scope/ prefix +// (lowercase), then lowercase alphanumeric with dots/hyphens. Uppercase is +// rejected (npm policy). Empty names are rejected. +func ValidateNpmPackageName(name string) error { + if name == "" { + return errors.New("npm package name must not be empty") + } + if !validNpmName.MatchString(name) { + return fmt.Errorf("invalid npm package name: %q", name) + } + return nil +} + +// ClassifyPipStderr inspects stderr output from pip and returns a sentinel +// error identifying the failure category, plus a truncated reason string +// (≤500 chars after ANSI stripping and whitespace normalization). +// +// Pattern priority: most-specific first. The default path returns (nil, reason) +// so callers can wrap generically. +func ClassifyPipStderr(stderr string) (error, string) { + reason := truncateStderr(stderr, 500) + switch { + case strings.Contains(stderr, "externally-managed-environment") || + strings.Contains(stderr, "EXTERNALLY-MANAGED"): + return ErrUpdatePipExternallyManaged, reason + case strings.Contains(stderr, "Permission denied") || + strings.Contains(stderr, "EACCES"): + return ErrUpdatePipPermission, reason + case strings.Contains(stderr, "No matching distribution") || + strings.Contains(stderr, "Could not find a version"): + return ErrUpdatePipNotFound, reason + case strings.Contains(stderr, "Read timed out") || + strings.Contains(stderr, "ConnectionError") || + strings.Contains(strings.ToLower(stderr), "network"): + return ErrUpdatePipNetwork, reason + case strings.Contains(stderr, "incompatible") || + strings.Contains(stderr, "dependency resolver") || + strings.Contains(stderr, "Shallow backtracking"): + return ErrUpdatePipConflict, reason + default: + return nil, reason // unclassified — caller wraps generically + } +} + +// ClassifyNpmStderr inspects stderr from npm and returns a sentinel error +// plus a truncated reason string (≤500 chars). +// +// Pattern priority: most-specific first. Default path returns (nil, reason). +func ClassifyNpmStderr(stderr string) (error, string) { + reason := truncateStderr(stderr, 500) + switch { + case strings.Contains(stderr, "EACCES"): + return ErrUpdateNpmPermission, reason + case strings.Contains(stderr, "ERESOLVE"): + return ErrUpdateNpmConflict, reason + case strings.Contains(stderr, "ETIMEDOUT") || + strings.Contains(stderr, "ENOTFOUND") || + strings.Contains(stderr, "getaddrinfo"): + return ErrUpdateNpmNetwork, reason + case strings.Contains(stderr, "ETARGET"): + return ErrUpdateNpmTargetMissing, reason + case strings.Contains(stderr, "E404") || + strings.Contains(stderr, "404") || + strings.Contains(stderr, "not in this registry"): + return ErrUpdateNpmNotFound, reason + default: + return nil, reason + } +} + +// truncateStderr normalizes and caps a stderr string for safe logging. +// Steps: (1) strip ANSI escape codes, (2) normalize CRLF → LF, +// (3) collapse whitespace runs to single space, (4) cap at n bytes with ellipsis. +func truncateStderr(s string, n int) string { + s = ansiRE.ReplaceAllString(s, "") + s = strings.ReplaceAll(s, "\r\n", "\n") + // Collapse consecutive whitespace (tabs, newlines, spaces) to single space. + fields := strings.Fields(s) + s = strings.Join(fields, " ") + if len(s) > n { + return s[:n] + "…" + } + return s +} diff --git a/internal/skills/pkg_update_helpers_test.go b/internal/skills/pkg_update_helpers_test.go new file mode 100644 index 00000000..4a53c5c4 --- /dev/null +++ b/internal/skills/pkg_update_helpers_test.go @@ -0,0 +1,310 @@ +package skills + +import ( + "strings" + "testing" +) + +func TestIsPipPreRelease(t *testing.T) { + cases := []struct { + version string + want bool + }{ + // Pre-release: bare identifiers (no digit) — M-1 fix + {"1.0.0rc", true}, + {"1.0.0a", true}, + {"1.0.0b", true}, + // Pre-release: with digit + {"1.0.0rc1", true}, + {"1.0.0a1", true}, + {"1.0.0b0", true}, + {"2.0.0.dev1", true}, + {"1.0.0.dev0", true}, + // Pre-release: .pre / .preview suffix + {"1.0.0.pre", true}, + {"1.0.0.preview", true}, + // Stable releases + {"1.0.0", false}, + {"2.3.4", false}, + {"1.0.0.post1", false}, + {"1.0.0.post0", false}, + } + for _, tc := range cases { + got := IsPipPreRelease(tc.version) + if got != tc.want { + t.Errorf("IsPipPreRelease(%q) = %v, want %v", tc.version, got, tc.want) + } + } +} + +func TestIsNpmPreRelease(t *testing.T) { + cases := []struct { + version string + want bool + }{ + // Pre-release labels + {"5.0.0-beta.1", true}, + {"5.0.0-rc.0", true}, + {"5.0.0-alpha.1", true}, + {"5.0.0-pre", true}, + {"5.0.0-preview.2", true}, + {"5.0.0-dev", true}, + {"5.0.0-nightly", true}, + {"5.0.0-snapshot", true}, + // Stable + {"5.0.0", false}, + {"5.0.0-foo", false}, // unknown label → not pre-release + {"5.0.0-stable", false}, // "stable" not in list + } + for _, tc := range cases { + got := IsNpmPreRelease(tc.version) + if got != tc.want { + t.Errorf("IsNpmPreRelease(%q) = %v, want %v", tc.version, got, tc.want) + } + } +} + +func TestValidatePipPackageName(t *testing.T) { + accept := []string{ + "Django", + "my-pkg", + "pip_tools", + "PyJWT", + "numpy", + "scikit-learn", + "A1", + } + for _, name := range accept { + if err := ValidatePipPackageName(name); err != nil { + t.Errorf("ValidatePipPackageName(%q) rejected valid name: %v", name, err) + } + } + + reject := []string{ + "", + "typescript@latest", // @ suffix + "pkg@@", // double @ + "pkg;rm", // shell metachar + "pkg space", // space + "-pkg", // leading hyphen + ".pkg", // leading dot + "pkg|other", // pipe + "pkg>1.0", // gt + } + for _, name := range reject { + if err := ValidatePipPackageName(name); err == nil { + t.Errorf("ValidatePipPackageName(%q) accepted invalid name", name) + } + } +} + +func TestValidateNpmPackageName(t *testing.T) { + accept := []string{ + "typescript", + "@angular/core", + "@scope/name-2", + "react", + "@babel/core", + "lodash.get", + } + for _, name := range accept { + if err := ValidateNpmPackageName(name); err != nil { + t.Errorf("ValidateNpmPackageName(%q) rejected valid name: %v", name, err) + } + } + + reject := []string{ + "", + "TypeScript", // uppercase (npm forbids) + "typescript@latest", // @ version suffix on bare name + "pkg@@", // double @ + "@scope/PKG", // uppercase in scoped path + "@Scope/name", // uppercase scope + "pkg space", // space + "@/name", // empty scope + } + for _, name := range reject { + if err := ValidateNpmPackageName(name); err == nil { + t.Errorf("ValidateNpmPackageName(%q) accepted invalid name", name) + } + } +} + +func TestClassifyPipStderr(t *testing.T) { + cases := []struct { + name string + stderr string + wantSentinel error + }{ + { + name: "externally managed environment", + stderr: "error: externally-managed-environment\nsome extra text", + wantSentinel: ErrUpdatePipExternallyManaged, + }, + { + name: "EXTERNALLY-MANAGED upper", + stderr: "This environment is EXTERNALLY-MANAGED", + wantSentinel: ErrUpdatePipExternallyManaged, + }, + { + name: "permission denied", + stderr: "ERROR: Could not install packages: Permission denied", + wantSentinel: ErrUpdatePipPermission, + }, + { + name: "no matching distribution", + stderr: "ERROR: No matching distribution found for nonexistent-pkg==99.0", + wantSentinel: ErrUpdatePipNotFound, + }, + { + name: "could not find a version", + stderr: "ERROR: Could not find a version that satisfies the requirement", + wantSentinel: ErrUpdatePipNotFound, + }, + { + name: "network read timeout", + stderr: "Read timed out. (read timeout=15)", + wantSentinel: ErrUpdatePipNetwork, + }, + { + name: "dependency conflict", + stderr: "ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.", + wantSentinel: ErrUpdatePipConflict, + }, + { + name: "shallow backtracking", + stderr: "Shallow backtracking detected: could not find a matching version", + wantSentinel: ErrUpdatePipConflict, + }, + { + name: "unclassified returns nil sentinel", + stderr: "some random pip error output", + wantSentinel: nil, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + sentinel, reason := ClassifyPipStderr(tc.stderr) + if sentinel != tc.wantSentinel { + t.Errorf("ClassifyPipStderr sentinel = %v, want %v", sentinel, tc.wantSentinel) + } + if reason == "" { + t.Error("reason must not be empty") + } + }) + } +} + +func TestClassifyNpmStderr(t *testing.T) { + cases := []struct { + name string + stderr string + wantSentinel error + }{ + { + name: "EACCES permission", + stderr: "npm ERR! code EACCES\nnpm ERR! path /usr/local/lib", + wantSentinel: ErrUpdateNpmPermission, + }, + { + name: "ERESOLVE conflict", + stderr: "npm ERR! code ERESOLVE\nnpm ERR! ERESOLVE unable to resolve dependency tree", + wantSentinel: ErrUpdateNpmConflict, + }, + { + name: "ETIMEDOUT network", + stderr: "npm ERR! code ETIMEDOUT\nnpm ERR! errno ETIMEDOUT", + wantSentinel: ErrUpdateNpmNetwork, + }, + { + name: "ENOTFOUND network", + stderr: "npm ERR! code ENOTFOUND\nnpm ERR! errno ENOTFOUND registry.npmjs.org", + wantSentinel: ErrUpdateNpmNetwork, + }, + { + name: "ETARGET version missing", + stderr: "npm ERR! code ETARGET\nnpm ERR! notarget No matching version found for typescript@99.0.0", + wantSentinel: ErrUpdateNpmTargetMissing, + }, + { + name: "E404 not found", + stderr: "npm ERR! code E404\nnpm ERR! 404 Not Found", + wantSentinel: ErrUpdateNpmNotFound, + }, + { + name: "not in this registry", + stderr: "npm ERR! my-private-pkg is not in this registry", + wantSentinel: ErrUpdateNpmNotFound, + }, + { + name: "unclassified returns nil sentinel", + stderr: "npm ERR! some random error", + wantSentinel: nil, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + sentinel, reason := ClassifyNpmStderr(tc.stderr) + if sentinel != tc.wantSentinel { + t.Errorf("ClassifyNpmStderr sentinel = %v, want %v", sentinel, tc.wantSentinel) + } + if reason == "" { + t.Error("reason must not be empty") + } + }) + } +} + +func TestTruncateStderr(t *testing.T) { + t.Run("strips ANSI codes", func(t *testing.T) { + in := "\x1b[31mERROR\x1b[0m: something failed" + got := truncateStderr(in, 500) + if strings.Contains(got, "\x1b") { + t.Errorf("ANSI codes not stripped: %q", got) + } + if !strings.Contains(got, "ERROR") { + t.Errorf("content should remain after strip: %q", got) + } + }) + + t.Run("normalizes CRLF to space", func(t *testing.T) { + in := "line1\r\nline2\r\nline3" + got := truncateStderr(in, 500) + // After normalization CRLF → LF → Fields() collapses to spaces + if strings.Contains(got, "\r") { + t.Errorf("CRLF not normalized: %q", got) + } + if !strings.Contains(got, "line1") || !strings.Contains(got, "line2") { + t.Errorf("content lost: %q", got) + } + }) + + t.Run("caps at n bytes with ellipsis", func(t *testing.T) { + in := strings.Repeat("a", 600) + got := truncateStderr(in, 500) + if len([]rune(got)) > 502 { // 500 + len("…") rune (3 bytes but 1 rune) + t.Errorf("not capped: len=%d", len(got)) + } + if !strings.HasSuffix(got, "…") { + t.Errorf("missing ellipsis: %q", got) + } + }) + + t.Run("short string unchanged", func(t *testing.T) { + in := "short error" + got := truncateStderr(in, 500) + if got != in { + t.Errorf("short string modified: got %q, want %q", got, in) + } + }) + + t.Run("collapses whitespace", func(t *testing.T) { + in := "err msg\t\twith\n\ntabs" + got := truncateStderr(in, 500) + if strings.Contains(got, " ") || strings.Contains(got, "\t") || strings.Contains(got, "\n") { + t.Errorf("whitespace not collapsed: %q", got) + } + }) +} diff --git a/internal/skills/testdata/npm/bin/npm b/internal/skills/testdata/npm/bin/npm new file mode 100755 index 00000000..dd4de1a7 --- /dev/null +++ b/internal/skills/testdata/npm/bin/npm @@ -0,0 +1,48 @@ +#!/bin/sh +# Fixture npm for unit tests. +# Controlled via env vars: +# FIXTURE_MODE — controls `outdated` output: outdated|error|ambiguous|empty (default: outdated) +# FIXTURE_NPM_EXIT — exit code for `install` subcommand (default 0) +# FIXTURE_NPM_STDERR — text written to stderr for `install` subcommand (default empty) + +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +FIXTURE_DIR="$(dirname "$SCRIPT_DIR")" + +if [ "$1" = "outdated" ]; then + case "${FIXTURE_MODE:-outdated}" in + outdated) + cat "$FIXTURE_DIR/outdated-10.json" + exit 1 + ;; + error) + printf 'npm ERR! code ERESOLVE\nnpm ERR! peer dep conflict\n' >&2 + exit 1 + ;; + ambiguous) + exit 1 + ;; + empty) + exit 0 + ;; + *) + exit 2 + ;; + esac +fi + +if [ "$1" = "install" ]; then + : "${FIXTURE_NPM_EXIT:=0}" + : "${FIXTURE_NPM_STDERR:=}" + if [ -n "$FIXTURE_NPM_STDERR" ]; then + printf '%s\n' "$FIXTURE_NPM_STDERR" >&2 + fi + exit "$FIXTURE_NPM_EXIT" +fi + +if [ "$1" = "cache" ]; then + # cleanCaches may invoke npm; succeed silently. + exit 0 +fi + +# Unknown subcommand. +exit 2 diff --git a/internal/skills/testdata/npm/outdated-10.json b/internal/skills/testdata/npm/outdated-10.json new file mode 100644 index 00000000..a7c6a0e0 --- /dev/null +++ b/internal/skills/testdata/npm/outdated-10.json @@ -0,0 +1,6 @@ +{ + "typescript": {"current": "5.0.0", "wanted": "5.0.0", "latest": "5.5.0"}, + "@angular/core": {"current": "16.0.0", "wanted": "16.0.0", "latest": "17.0.0"}, + "lodash": {"current": "4.17.20", "wanted": "4.17.20", "latest": "4.17.21-beta.0"}, + "react-beta": {"current": "19.0.0-beta.1", "wanted": "19.0.0-beta.1", "latest": "19.0.0-beta.3"} +} diff --git a/internal/skills/testdata/pip/bin/pip3 b/internal/skills/testdata/pip/bin/pip3 new file mode 100755 index 00000000..4296932d --- /dev/null +++ b/internal/skills/testdata/pip/bin/pip3 @@ -0,0 +1,46 @@ +#!/bin/sh +# Fixture pip3 for unit tests. +# Controlled via env vars: +# FIXTURE_PIP_EXIT — exit code for `install` subcommand (default 0) +# FIXTURE_PIP_STDERR — text written to stderr for `install` subcommand (default empty) +# +# `list --outdated` emits the JSON fixture files relative to this script's directory. +# `list --outdated --pre` emits outdated-empty.json (no additional pre-release updates). +# `install ...` exits with FIXTURE_PIP_EXIT and emits FIXTURE_PIP_STDERR to stderr. + +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +FIXTURE_DIR="$(dirname "$SCRIPT_DIR")" + +if [ "$1" = "list" ] && [ "$2" = "--outdated" ]; then + # Check if --pre flag is present anywhere in args. + has_pre=0 + for arg in "$@"; do + if [ "$arg" = "--pre" ]; then + has_pre=1 + break + fi + done + if [ "$has_pre" = "1" ]; then + cat "$FIXTURE_DIR/outdated-empty.json" 2>/dev/null || echo "[]" + else + cat "$FIXTURE_DIR/outdated-23.3.json" + fi + exit 0 +fi + +if [ "$1" = "install" ]; then + : "${FIXTURE_PIP_EXIT:=0}" + : "${FIXTURE_PIP_STDERR:=}" + if [ -n "$FIXTURE_PIP_STDERR" ]; then + printf '%s\n' "$FIXTURE_PIP_STDERR" >&2 + fi + exit "$FIXTURE_PIP_EXIT" +fi + +if [ "$1" = "cache" ]; then + # cleanCaches calls `pip3 cache purge`; succeed silently. + exit 0 +fi + +# Unknown subcommand. +exit 2 diff --git a/internal/skills/testdata/pip/outdated-23.3.json b/internal/skills/testdata/pip/outdated-23.3.json new file mode 100644 index 00000000..1fd76fd1 --- /dev/null +++ b/internal/skills/testdata/pip/outdated-23.3.json @@ -0,0 +1,5 @@ +[ + {"name":"setuptools","version":"65.5.0","latest_version":"68.2.2","latest_filetype":"wheel"}, + {"name":"pip","version":"22.3","latest_version":"23.3.1","latest_filetype":"wheel"}, + {"name":"torch","version":"2.0.0rc1","latest_version":"2.0.0","latest_filetype":"wheel"} +] diff --git a/internal/skills/testdata/pip/outdated-empty.json b/internal/skills/testdata/pip/outdated-empty.json new file mode 100644 index 00000000..fe51488c --- /dev/null +++ b/internal/skills/testdata/pip/outdated-empty.json @@ -0,0 +1 @@ +[] diff --git a/internal/skills/update_registry.go b/internal/skills/update_registry.go index f96b3be4..3ce12160 100644 --- a/internal/skills/update_registry.go +++ b/internal/skills/update_registry.go @@ -24,6 +24,15 @@ type UpdateCheckResult struct { Updates []UpdateInfo ETags map[string]string // subset to merge into UpdateCache.GitHubETags Err error // per-source error; non-fatal for other checkers + // Available signals whether the source is actionable on this host. + // false (zero-value) means exec.LookPath / edition gate rejected the source, + // or the checker was never run. The HTTP availability map surfaces this so + // the UI can hide sources that are not actionable. + // Interpretation: false === "not actionable"; a non-error check with + // Updates == nil but Available == true means "source reachable, zero updates". + // Checkers MUST set Available=true on a normal successful check and leave + // it false only on LookPath miss or edition gate rejection. + Available bool } // UpdateChecker polls a package source for available updates. @@ -61,8 +70,9 @@ type UpdateRegistry struct { CachePath string TTL time.Duration - mu sync.RWMutex - refreshing atomic.Bool // single-flight gate for background refresh + mu sync.RWMutex + refreshing atomic.Bool // single-flight gate for background refresh + availability map[string]bool // per-source availability from last CheckAll; guarded by mu } // NewUpdateRegistry constructs an empty registry. Register checkers/executors @@ -75,12 +85,13 @@ func NewUpdateRegistry(cache *UpdateCache, cachePath string, ttl time.Duration) ttl = time.Hour } return &UpdateRegistry{ - checkers: make(map[string]UpdateChecker), - executors: make(map[string]UpdateExecutor), - Locker: NewPackageLocker(), - Cache: cache, - CachePath: cachePath, - TTL: ttl, + checkers: make(map[string]UpdateChecker), + executors: make(map[string]UpdateExecutor), + Locker: NewPackageLocker(), + Cache: cache, + CachePath: cachePath, + TTL: ttl, + availability: make(map[string]bool), } } @@ -111,6 +122,27 @@ func (r *UpdateRegistry) Sources() []string { return out } +// Availability returns a snapshot of per-source availability from the last CheckAll. +// A missing key means "never checked" — callers should treat a missing key as true +// (first-boot default: source is visible until confirmed unavailable). +// The returned map is a safe clone; mutating it does not affect the registry. +func (r *UpdateRegistry) Availability() map[string]bool { + r.mu.RLock() + defer r.mu.RUnlock() + out := make(map[string]bool, len(r.availability)) + for k, v := range r.availability { + out[k] = v + } + return out +} + +// setAvailability records per-source availability under write lock. +func (r *UpdateRegistry) setAvailability(source string, available bool) { + r.mu.Lock() + r.availability[source] = available + r.mu.Unlock() +} + // CheckAll runs every registered checker and merges results into the cache. // Checkers run in parallel (each is an independent API). A single checker's // error does NOT abort siblings (red-team M7 fix — don't use errgroup which @@ -171,6 +203,8 @@ func (r *UpdateRegistry) CheckAll(ctx context.Context) []error { for k, v := range res.ETags { etagMerge[k] = v } + // Record per-source availability from this check cycle. + r.setAvailability(res.Source, res.Available) } now := time.Now().UTC() diff --git a/internal/skills/update_registry_test.go b/internal/skills/update_registry_test.go new file mode 100644 index 00000000..05a6451d --- /dev/null +++ b/internal/skills/update_registry_test.go @@ -0,0 +1,84 @@ +package skills + +import ( + "context" + "testing" + "time" +) + +// fakeChecker is a minimal UpdateChecker for registry tests. +type fakeChecker struct { + source string + available bool + err error +} + +func (f *fakeChecker) Source() string { return f.source } +func (f *fakeChecker) Check(_ context.Context, _ map[string]string) UpdateCheckResult { + return UpdateCheckResult{ + Source: f.source, + Available: f.available, + Err: f.err, + } +} + +func TestRegistry_Availability(t *testing.T) { + reg := NewUpdateRegistry(nil, "", time.Hour) + + reg.RegisterChecker(&fakeChecker{source: "github", available: true}) + reg.RegisterChecker(&fakeChecker{source: "pip", available: false}) + + errs := reg.CheckAll(context.Background()) + if len(errs) != 0 { + t.Fatalf("unexpected errors from CheckAll: %v", errs) + } + + avail := reg.Availability() + + if got, want := avail["github"], true; got != want { + t.Errorf("Availability[github] = %v, want %v", got, want) + } + if got, want := avail["pip"], false; got != want { + t.Errorf("Availability[pip] = %v, want %v", got, want) + } + + // Verify returned map is a clone — mutating it must not affect the registry. + avail["github"] = false + avail["pip"] = true + avail2 := reg.Availability() + if avail2["github"] != true { + t.Error("Availability() returned same map (not a clone): mutation propagated") + } + if avail2["pip"] != false { + t.Error("Availability() returned same map (not a clone): mutation propagated") + } +} + +func TestRegistry_Availability_NeverChecked(t *testing.T) { + // A registry with no CheckAll call should return an empty map. + // Callers are expected to treat missing keys as true (first-boot default). + reg := NewUpdateRegistry(nil, "", time.Hour) + avail := reg.Availability() + if len(avail) != 0 { + t.Errorf("expected empty map before CheckAll, got %v", avail) + } +} + +func TestRegistry_Availability_UpdatedOnRecheck(t *testing.T) { + // A checker that flips available state between calls. + reg := NewUpdateRegistry(nil, "", time.Hour) + checker := &fakeChecker{source: "npm", available: false} + reg.RegisterChecker(checker) + + reg.CheckAll(context.Background()) //nolint:errcheck + if got := reg.Availability()["npm"]; got != false { + t.Errorf("first check: Availability[npm] = %v, want false", got) + } + + // Second check with available=true. + checker.available = true + reg.CheckAll(context.Background()) //nolint:errcheck + if got := reg.Availability()["npm"]; got != true { + t.Errorf("second check: Availability[npm] = %v, want true", got) + } +} diff --git a/internal/skills/wiring_edition_gate_test.go b/internal/skills/wiring_edition_gate_test.go new file mode 100644 index 00000000..322cd398 --- /dev/null +++ b/internal/skills/wiring_edition_gate_test.go @@ -0,0 +1,81 @@ +package skills + +import ( + "testing" + "time" + + "github.com/nextlevelbuilder/goclaw/internal/edition" +) + +// TestEditionGate_LitePreventsRegistration mirrors the wiring logic in +// cmd/gateway_packages_wiring.go and asserts that the pip/npm checkers are +// NOT registered when edition.Current().SupportsPipNpm == false (Lite desktop). +// +// This is the unit-level guard for P2A-H6: "Lite edition runs useless pip/npm +// checkers". The wiring file gates registration like: +// +// if edition.Current().SupportsPipNpm { +// registry.RegisterChecker(NewPipUpdateChecker()) +// registry.RegisterExecutor(NewPipUpdateExecutor()) +// registry.RegisterChecker(NewNpmUpdateChecker()) +// registry.RegisterExecutor(NewNpmUpdateExecutor()) +// } +func TestEditionGate_LitePreventsRegistration(t *testing.T) { + // Temporarily set edition to Lite; restore Standard on exit. + edition.SetCurrent(edition.Lite) + t.Cleanup(func() { edition.SetCurrent(edition.Standard) }) + + // Replicate wiring logic. + registry := NewUpdateRegistry(nil, "", time.Hour) + + // Always register github (no edition gate in wiring). + // Use a fakeChecker so we don't need a real GitHubInstaller. + registry.RegisterChecker(&fakeChecker{source: "github", available: true}) + + // Gate pip+npm behind edition flag — same condition as wiring. + if edition.Current().SupportsPipNpm { + registry.RegisterChecker(NewPipUpdateChecker()) + registry.RegisterExecutor(NewPipUpdateExecutor()) + registry.RegisterChecker(NewNpmUpdateChecker()) + registry.RegisterExecutor(NewNpmUpdateExecutor()) + } + + sources := registry.Sources() + + if len(sources) != 1 || sources[0] != "github" { + t.Errorf("Lite edition: want sources=[github], got %v", sources) + } + + // pip and npm must not appear. + for _, s := range sources { + if s == "pip" || s == "npm" { + t.Errorf("Lite edition: unexpected source %q in registry", s) + } + } +} + +// TestEditionGate_StandardAllowsRegistration verifies the positive case: +// Standard edition registers all three sources. +func TestEditionGate_StandardAllowsRegistration(t *testing.T) { + edition.SetCurrent(edition.Standard) + t.Cleanup(func() { edition.SetCurrent(edition.Standard) }) + + registry := NewUpdateRegistry(nil, "", time.Hour) + registry.RegisterChecker(&fakeChecker{source: "github", available: true}) + + if edition.Current().SupportsPipNpm { + registry.RegisterChecker(NewPipUpdateChecker()) + registry.RegisterExecutor(NewPipUpdateExecutor()) + registry.RegisterChecker(NewNpmUpdateChecker()) + registry.RegisterExecutor(NewNpmUpdateExecutor()) + } + + sources := registry.Sources() // sorted: github, npm, pip + want := map[string]bool{"github": true, "pip": true, "npm": true} + for _, s := range sources { + delete(want, s) + } + if len(want) != 0 { + t.Errorf("Standard edition: missing sources %v in %v", want, sources) + } +} diff --git a/tests/integration/packages_pipnpm_test.go b/tests/integration/packages_pipnpm_test.go new file mode 100644 index 00000000..dea75ed6 --- /dev/null +++ b/tests/integration/packages_pipnpm_test.go @@ -0,0 +1,139 @@ +//go:build pipnpm_e2e + +// Package integration contains optional end-to-end tests for pip + npm update flow. +// These tests require real pip3 and npm on PATH. They are excluded from default CI +// and must be opted into via: go test -tags pipnpm_e2e ./tests/integration/... +// +// Typical pre-conditions in a test container: +// +// pip3 install --break-system-packages "requests==2.25.0" +// npm install -g "typescript@4.0.0" +package integration + +import ( + "context" + "os/exec" + "testing" + "time" + + "github.com/nextlevelbuilder/goclaw/internal/skills" +) + +// TestPipUpdateChecker_E2E verifies that PipUpdateChecker detects a known-stale +// package and PipUpdateExecutor upgrades it successfully. +// +// Pre-condition: pip3 must be on PATH and "requests==2.25.0" must be installed. +// The test installs the old version itself if pip3 is available. +func TestPipUpdateChecker_E2E(t *testing.T) { + if _, err := exec.LookPath("pip3"); err != nil { + t.Skip("pip3 not on PATH — skipping pip e2e test") + } + + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + defer cancel() + + // Install a known-stale version of requests. + installCmd := exec.CommandContext(ctx, "pip3", "install", + "--break-system-packages", "--quiet", "requests==2.25.0") + if out, err := installCmd.CombinedOutput(); err != nil { + t.Fatalf("pre-condition: install requests==2.25.0 failed: %v\n%s", err, out) + } + + // Check: PipUpdateChecker should detect requests as outdated. + checker := skills.NewPipUpdateChecker() + result := checker.Check(ctx, nil) + + if !result.Available { + t.Fatal("PipUpdateChecker: Available=false with pip3 on PATH") + } + if result.Err != nil { + t.Fatalf("PipUpdateChecker: unexpected error: %v", result.Err) + } + + var requestsUpdate *skills.UpdateInfo + for i := range result.Updates { + if result.Updates[i].Name == "requests" { + requestsUpdate = &result.Updates[i] + break + } + } + if requestsUpdate == nil { + t.Fatal("PipUpdateChecker: 'requests' not listed as outdated (expected >=2.25.0 to have update)") + } + if requestsUpdate.CurrentVersion != "2.25.0" { + t.Errorf("CurrentVersion = %q, want 2.25.0", requestsUpdate.CurrentVersion) + } + t.Logf("requests: %s → %s", requestsUpdate.CurrentVersion, requestsUpdate.LatestVersion) + + // Apply: PipUpdateExecutor should upgrade requests. + executor := skills.NewPipUpdateExecutor() + if err := executor.Update(ctx, "requests", requestsUpdate.LatestVersion, requestsUpdate.Meta); err != nil { + t.Fatalf("PipUpdateExecutor: Update failed: %v", err) + } + + // Re-check: requests should no longer be in the outdated list. + result2 := checker.Check(ctx, nil) + for _, u := range result2.Updates { + if u.Name == "requests" { + t.Errorf("requests still outdated after update: current=%s latest=%s", + u.CurrentVersion, u.LatestVersion) + } + } +} + +// TestNpmUpdateChecker_E2E verifies that NpmUpdateChecker detects a known-stale +// global npm package and NpmUpdateExecutor upgrades it. +// +// Pre-condition: npm must be on PATH and "typescript@4.0.0" must be globally installed. +func TestNpmUpdateChecker_E2E(t *testing.T) { + if _, err := exec.LookPath("npm"); err != nil { + t.Skip("npm not on PATH — skipping npm e2e test") + } + + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Minute) + defer cancel() + + // Install a known-stale version of typescript globally. + installCmd := exec.CommandContext(ctx, "npm", "install", "-g", "typescript@4.0.0") + if out, err := installCmd.CombinedOutput(); err != nil { + t.Fatalf("pre-condition: install typescript@4.0.0 failed: %v\n%s", err, out) + } + + // Check: NpmUpdateChecker should detect typescript as outdated. + checker := skills.NewNpmUpdateChecker() + result := checker.Check(ctx, nil) + + if !result.Available { + t.Fatal("NpmUpdateChecker: Available=false with npm on PATH") + } + if result.Err != nil { + t.Fatalf("NpmUpdateChecker: unexpected error: %v", result.Err) + } + + var tsUpdate *skills.UpdateInfo + for i := range result.Updates { + if result.Updates[i].Name == "typescript" { + tsUpdate = &result.Updates[i] + break + } + } + if tsUpdate == nil { + t.Fatal("NpmUpdateChecker: 'typescript' not listed as outdated (expected 4.0.0 to have update)") + } + t.Logf("typescript: %s → %s", tsUpdate.CurrentVersion, tsUpdate.LatestVersion) + + // Apply: NpmUpdateExecutor should upgrade typescript. + executor := skills.NewNpmUpdateExecutor() + if err := executor.Update(ctx, "typescript", tsUpdate.LatestVersion, tsUpdate.Meta); err != nil { + t.Fatalf("NpmUpdateExecutor: Update failed: %v", err) + } + + // Re-check: typescript should no longer be in the outdated list. + result2 := checker.Check(ctx, nil) + for _, u := range result2.Updates { + if u.Name == "typescript" { + t.Errorf("typescript still outdated after update: current=%s latest=%s", + u.CurrentVersion, u.LatestVersion) + } + } +} diff --git a/ui/web/src/i18n/locales/en/packages.json b/ui/web/src/i18n/locales/en/packages.json index c7e0980d..a4206a37 100644 --- a/ui/web/src/i18n/locales/en/packages.json +++ b/ui/web/src/i18n/locales/en/packages.json @@ -59,7 +59,31 @@ "selected": "{{count}} selected", "manifestDesyncWarn": "Binary was updated but the manifest save failed. Manual recovery required for {{name}}.", "cacheStale": "Updates cache is stale. Please refresh first.", - "adminOnly": "Administrator access required" + "adminOnly": "Administrator access required", + "empty": "No updates available", + "source": { + "github": "GitHub", + "pip": "pip", + "npm": "npm" + }, + "filter": { + "all": "All sources", + "label": "Filter" + }, + "unavailable": { + "pip": "pip not installed", + "npm": "npm not installed" + }, + "button": { + "tooltip": { + "github": "Update from GitHub release", + "pip": "Update via pip", + "npm": "Update via npm" + } + }, + "summary": { + "perSource": "{{source}}: {{count}}" + } }, "actions": { "install": "Install", diff --git a/ui/web/src/i18n/locales/vi/packages.json b/ui/web/src/i18n/locales/vi/packages.json index e1473592..543ef5b5 100644 --- a/ui/web/src/i18n/locales/vi/packages.json +++ b/ui/web/src/i18n/locales/vi/packages.json @@ -59,7 +59,31 @@ "selected": "{{count}} đã chọn", "manifestDesyncWarn": "Binary đã cập nhật nhưng lưu manifest thất bại. Cần khôi phục thủ công cho {{name}}.", "cacheStale": "Cache cập nhật đã cũ. Hãy làm mới trước.", - "adminOnly": "Cần quyền quản trị viên" + "adminOnly": "Cần quyền quản trị viên", + "empty": "Không có bản cập nhật", + "source": { + "github": "GitHub", + "pip": "pip", + "npm": "npm" + }, + "filter": { + "all": "Tất cả nguồn", + "label": "Lọc" + }, + "unavailable": { + "pip": "Chưa cài pip", + "npm": "Chưa cài npm" + }, + "button": { + "tooltip": { + "github": "Cập nhật từ bản phát hành GitHub", + "pip": "Cập nhật qua pip", + "npm": "Cập nhật qua npm" + } + }, + "summary": { + "perSource": "{{source}}: {{count}}" + } }, "actions": { "install": "Cài đặt", diff --git a/ui/web/src/i18n/locales/zh/packages.json b/ui/web/src/i18n/locales/zh/packages.json index 5f2e7ed2..a254084f 100644 --- a/ui/web/src/i18n/locales/zh/packages.json +++ b/ui/web/src/i18n/locales/zh/packages.json @@ -59,7 +59,31 @@ "selected": "已选 {{count}} 个", "manifestDesyncWarn": "二进制文件已更新但清单保存失败。{{name}} 需要手动恢复。", "cacheStale": "更新缓存已过期。请先刷新。", - "adminOnly": "需要管理员权限" + "adminOnly": "需要管理员权限", + "empty": "没有可用更新", + "source": { + "github": "GitHub", + "pip": "pip", + "npm": "npm" + }, + "filter": { + "all": "所有来源", + "label": "筛选" + }, + "unavailable": { + "pip": "未安装 pip", + "npm": "未安装 npm" + }, + "button": { + "tooltip": { + "github": "从 GitHub 发布更新", + "pip": "通过 pip 更新", + "npm": "通过 npm 更新" + } + }, + "summary": { + "perSource": "{{source}}: {{count}}" + } }, "actions": { "install": "安装", diff --git a/ui/web/src/pages/packages/components/source-pill.tsx b/ui/web/src/pages/packages/components/source-pill.tsx new file mode 100644 index 00000000..d999d4f6 --- /dev/null +++ b/ui/web/src/pages/packages/components/source-pill.tsx @@ -0,0 +1,32 @@ +import { cn } from "@/lib/utils"; + +interface Props { + source: "github" | "pip" | "npm" | string; +} + +const SOURCE_CLASSES: Record = { + github: + "bg-slate-100 text-slate-900 dark:bg-slate-800 dark:text-slate-100", + pip: "bg-blue-100 text-blue-900 dark:bg-blue-900/40 dark:text-blue-200", + npm: "bg-amber-100 text-amber-900 dark:bg-amber-900/40 dark:text-amber-200", +}; + +const NEUTRAL = + "bg-muted text-muted-foreground"; + +/** + * Small colored pill indicating a package source (github / pip / npm / other). + */ +export function SourcePill({ source }: Props) { + const classes = SOURCE_CLASSES[source] ?? NEUTRAL; + return ( + + {source} + + ); +} diff --git a/ui/web/src/pages/packages/components/update-row-button.tsx b/ui/web/src/pages/packages/components/update-row-button.tsx index 8883634d..cf67a8a0 100644 --- a/ui/web/src/pages/packages/components/update-row-button.tsx +++ b/ui/web/src/pages/packages/components/update-row-button.tsx @@ -16,20 +16,24 @@ interface Props { globalPending?: boolean; isMaster: boolean; onUpdate: (spec: string) => void; + /** Override source for tooltip / spec generation (defaults to update.source) */ + source?: string; } /** - * Inline "Update" button rendered inside each GitHub Binaries table row. - * - Renders only when an update is available for the row's package. + * Inline "Update" button rendered inside each package update table row. * - Disabled (not hidden) for non-master users with an explanatory tooltip. * - Tracks its own local pending state so rapid clicks don't double-fire. + * - Emits `{source}:{name}` spec to onUpdate (e.g. "pip:requests"). */ -export function UpdateRowButton({ update, globalPending, isMaster, onUpdate }: Props) { +export function UpdateRowButton({ update, globalPending, isMaster, onUpdate, source }: Props) { const { t } = useTranslation("packages"); const [localPending, setLocalPending] = useState(false); const isPending = localPending || !!globalPending; - const spec = `github:${update.name}`; + const effectiveSource = source ?? update.source; + // Build spec as "{source}:{name}" for all sources + const spec = `${effectiveSource}:${update.name}`; const handleClick = () => { if (isPending || !isMaster) return; @@ -43,9 +47,13 @@ export function UpdateRowButton({ update, globalPending, isMaster, onUpdate }: P } }; + // Use source-specific tooltip key if available, fallback to generic + const sourceTooltipKey = `updates.button.tooltip.${effectiveSource}`; const tooltipText = !isMaster ? t("updates.adminOnly") - : `${update.currentVersion} → ${update.latestVersion}`; + : t(sourceTooltipKey, { + defaultValue: `${update.currentVersion} → ${update.latestVersion}`, + }); return ( diff --git a/ui/web/src/pages/packages/components/updates-list.tsx b/ui/web/src/pages/packages/components/updates-list.tsx new file mode 100644 index 00000000..a227a316 --- /dev/null +++ b/ui/web/src/pages/packages/components/updates-list.tsx @@ -0,0 +1,148 @@ +import { useState } from "react"; +import { useTranslation } from "react-i18next"; +import { ArrowRight, Loader2 } from "lucide-react"; +import { + Select, + SelectContent, + SelectItem, + SelectTrigger, + SelectValue, +} from "@/components/ui/select"; +import type { UpdateInfo } from "../hooks/use-updates"; +import { SourcePill } from "./source-pill"; +import { UpdateRowButton } from "./update-row-button"; + +const KNOWN_SOURCES = ["github", "pip", "npm"] as const; +type KnownSource = (typeof KNOWN_SOURCES)[number]; + +interface Props { + updates: UpdateInfo[]; + availability?: Record; + loading?: boolean; + isMaster: boolean; + onUpdate: (pkg: string) => Promise | void; + onUpdateAll?: () => void; +} + +/** + * Unified updates table across all package sources (github / pip / npm). + * - Renders a source filter dropdown when multiple sources have updates. + * - Delegates per-row update action to UpdateRowButton. + * - Mobile-safe: overflow-x-auto + min-w-[600px] per CLAUDE.md rules. + */ +export function UpdatesList({ + updates, + availability, + loading, + isMaster, + onUpdate, +}: Props) { + const { t } = useTranslation("packages"); + const [sourceFilter, setSourceFilter] = useState<"all" | KnownSource>("all"); + + // Sources not explicitly disabled (missing key → visible) + const visibleSources = KNOWN_SOURCES.filter((s) => availability?.[s] !== false); + + // Only show filter when more than 1 source is visible + const showFilter = visibleSources.length > 1 || sourceFilter !== "all"; + + const filteredUpdates = + sourceFilter === "all" + ? updates + : updates.filter((u) => u.source === sourceFilter); + + if (!loading && updates.length === 0) return null; + + return ( +
+ {/* Filter row */} + {showFilter && ( +
+ {t("updates.filter.label")}: + +
+ )} + + {/* Updates table */} +
+ + + + + + + + + + + {loading && filteredUpdates.length === 0 ? ( + + + + ) : filteredUpdates.length === 0 ? ( + + + + ) : ( + filteredUpdates.map((upd) => ( + + + + + + + )) + )} + +
+ {t("updates.filter.label")} + + {t("table.name")} + + {t("table.version")} + + {t("table.actions")} +
+ +
+ {t("updates.empty")} +
+ + {upd.name} + + {upd.currentVersion} + + + + {upd.latestVersion} + + + +
+
+
+ ); +} diff --git a/ui/web/src/pages/packages/components/updates-summary-bar.tsx b/ui/web/src/pages/packages/components/updates-summary-bar.tsx index 49e1ad9c..a9b2a806 100644 --- a/ui/web/src/pages/packages/components/updates-summary-bar.tsx +++ b/ui/web/src/pages/packages/components/updates-summary-bar.tsx @@ -5,6 +5,8 @@ import { Button } from "@/components/ui/button"; import { formatRelativeTime } from "@/lib/format"; import type { UpdateInfo } from "../hooks/use-updates"; +const KNOWN_SOURCES = ["github", "pip", "npm"] as const; + interface Props { updates: UpdateInfo[]; checkedAt?: string; @@ -13,11 +15,14 @@ interface Props { isMaster: boolean; onRefresh: () => void; onUpdateAll: () => void; + /** Map of source → available (false = runtime missing in container) */ + availability?: Record; } /** - * Summary bar shown at the top of the GitHub Binaries section. + * Summary bar shown above the updates list. * Visible when updates are available OR the cache is stale. + * Shows per-source breakdown when multiple sources are present. */ export function UpdatesSummaryBar({ updates, @@ -27,6 +32,7 @@ export function UpdatesSummaryBar({ isMaster, onRefresh, onUpdateAll, + availability, }: Props) { const { t } = useTranslation("packages"); @@ -39,10 +45,18 @@ export function UpdatesSummaryBar({ ? t("updates.lastCheckedAgo", { ago: formatRelativeTime(checkedAt) }) : t("updates.neverChecked"); + // Count updates per source (only visible sources) + const visibleSources = KNOWN_SOURCES.filter((s) => availability?.[s] !== false); + const countBySrc = visibleSources.reduce>((acc, src) => { + acc[src] = updates.filter((u) => u.source === src).length; + return acc; + }, {}); + const hasMultiSource = visibleSources.filter((s) => (countBySrc[s] ?? 0) > 0).length > 1; + return (
- {/* Badge + last-checked */} -
+ {/* Badge + last-checked + per-source breakdown */} +
{hasUpdates ? ( {t("updates.available", { count: updates.length })} @@ -50,6 +64,19 @@ export function UpdatesSummaryBar({ ) : ( {t("updates.cacheStale")} )} + {/* Per-source count badges — only shown when more than one source has updates */} + {hasMultiSource && visibleSources.map((src) => { + const count = countBySrc[src] ?? 0; + if (count === 0) return null; + return ( + + {t("updates.summary.perSource", { + source: t(`updates.source.${src}`, { defaultValue: src }), + count, + })} + + ); + })} {lastChecked}
diff --git a/ui/web/src/pages/packages/github-binaries-section.tsx b/ui/web/src/pages/packages/github-binaries-section.tsx index 50bebcf1..9c29586d 100644 --- a/ui/web/src/pages/packages/github-binaries-section.tsx +++ b/ui/web/src/pages/packages/github-binaries-section.tsx @@ -89,6 +89,7 @@ export function GitHubBinariesSection({ packages, onInstall, onUninstall }: Prop checkedAt, stale, loading: updatesLoading, + availability, refresh: refreshUpdates, updatePackage, applyAll, @@ -147,6 +148,7 @@ export function GitHubBinariesSection({ packages, onInstall, onUninstall }: Prop stale={stale} loading={updatesLoading} isMaster={isMaster} + availability={availability} onRefresh={refreshUpdates} onUpdateAll={() => setUpdateAllOpen(true)} /> diff --git a/ui/web/src/pages/packages/hooks/use-updates.ts b/ui/web/src/pages/packages/hooks/use-updates.ts index 10272887..304a1bd4 100644 --- a/ui/web/src/pages/packages/hooks/use-updates.ts +++ b/ui/web/src/pages/packages/hooks/use-updates.ts @@ -13,10 +13,11 @@ export interface UpdateMeta { assetSizeBytes?: number; assetSHA256?: string; prerelease?: boolean; + [key: string]: unknown; } export interface UpdateInfo { - source: "github"; + source: "github" | "pip" | "npm" | string; name: string; currentVersion: string; latestVersion: string; @@ -31,6 +32,8 @@ export interface UpdatesResponse { ttlSeconds: number; stale: boolean; sources: string[]; + /** Map of source → available (false = runtime not present in container) */ + availability?: Record; } interface UpdateResult { @@ -201,6 +204,7 @@ export function useUpdates() { checkedAt: data?.checkedAt, ageSeconds: data?.ageSeconds, stale: data?.stale ?? false, + availability: data?.availability, loading: loading || refreshMutation.isPending, refresh, updatePackage, diff --git a/ui/web/src/pages/packages/packages-page.tsx b/ui/web/src/pages/packages/packages-page.tsx index 4a6cfa58..0870bf6f 100644 --- a/ui/web/src/pages/packages/packages-page.tsx +++ b/ui/web/src/pages/packages/packages-page.tsx @@ -10,6 +10,8 @@ import { useAuthStore } from "@/stores/use-auth-store"; import { usePackages } from "./hooks/use-packages"; import { usePackageRuntimes } from "./hooks/use-package-runtimes"; import { RuntimesStickyHeader } from "./runtimes-sticky-header"; +import { useUpdates } from "./hooks/use-updates"; +import { UpdatesList } from "./components/updates-list"; // --- Lazy tab bodies (each is a separate chunk) --- const SystemPackagesTab = lazy(() => @@ -56,7 +58,9 @@ export function PackagesPage() { const [searchParams, setSearchParams] = useSearchParams(); const { refresh } = usePackages(); const { refresh: refreshRuntimes } = usePackageRuntimes(); + const { updates, availability, loading: updatesLoading, updatePackage } = useUpdates(); const role = useAuthStore((s) => s.role); + const isMaster = useAuthStore((s) => s.isMasterScope); const isAdmin = hasMinRole(role, "admin"); // Validate tab param — fall back to "system" for unknown values @@ -98,6 +102,15 @@ export function PackagesPage() { {/* Runtimes always-visible strip */} + {/* Unified updates list — all sources (github / pip / npm) */} + updatePackage(spec)} + /> + {/* Tabs */} {/* Tab list — horizontal scroll on mobile */}