diff --git a/CHANGELOG.md b/CHANGELOG.md index ad2dda2..f173e60 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,70 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Gated tools: a new optional manifest key, `gatedTools`, for skill + targets whose directory is too generic to detect by existence alone. + A gated tool is active only while its `requireMarker` file exists and + names the tool's target, and that file must be inside `~/.pilot`. The + marker holds `skills_dir=` and `skill_format=` + lines; the tool is on only when `skills_dir` is the row's `skillsDir` + (after resolving symlinks) and `skill_format` its `skillFormat`. An empty + marker, one for another folder, and one for the canonical frontmatter + turn nothing on, so an installer run for another agent's skills folder, + or with the Muse frontmatter turned off, never makes the daemon write or + rewrite `~/workspace/skills`. The marker must be a regular file of at + most 4 KiB. Without a matching marker nothing under the tool's `rootDir` + is read, written or removed, by a tick or by `Uninstall`. + Only the entrypoint skill copy is installed; there is no heartbeat or + plugin. `rootDir` must be inside the home directory, `skillsDir` inside + `rootDir`. Every file operation goes through an `os.Root` on `rootDir`, + and a symlink at the skill file or at any directory between `rootDir` + and it is refused with an error row. Writes use an `O_EXCL` temp file + with a random name. A gated row that resolves to a regular tool's skill + copy is refused, so the two never rewrite each other. The key is new on + purpose: every released version decodes the manifest with plain + `json.Unmarshal` into a struct without it, so released daemons ignore + these rows. As a `tools` row, the same entry would be installed by every + released daemon on any host where the directory exists, with no marker + check. +- `skillFormat: "muse"` (`SkillFormatMuse`) for Meta Muse, which loads + skills from `~/workspace/skills`: the entrypoint SKILL.md frontmatter is + rewritten to `name: ""` and a one-line + quoted `description` (folded values joined, capped at 1024 bytes), and + every other key is dropped. The body is unchanged. The output is byte + for byte what the pilot-skills Muse installer (`muse/install.sh`) writes, + so the two do not rewrite each other's copy. + `TestMuseSkillMD_MatchesInstaller` runs the installer's own shell + function against the Go port. + +- `Config.ProxyCommand` and credential refresh for the default HTTP + client. Egress proxies that rotate the credentials in `HTTPS_PROXY` + (Meta Muse) answered every tick after the first rotation with 407, + because the daemon keeps its launch-time environment. When + `Config.HTTPClient` is nil, the client is now a + `netproxy.RefreshingTransport` whose refresh command is + `Config.ProxyCommand`, else `$PILOT_PROXY_CMD`, else `"proxy_cmd"` in + `~/.pilot/config.json`, the same settings pilot-daemon reads. It runs at + the start of each tick, once a minute while it runs, and on a 407, and + the refused request is retried once. `PILOT_PROXY` or `config.json` + `"proxy"` set to off/none/no/false/direct turns it off. Without a + command the client is the plain one, and a 407 reported as a + `*netproxy.ConnectError` (pilot-daemon's `http.DefaultTransport` does so + after refreshing its own credentials) is retried once; a rejection the + proxy garbles ("malformed HTTP status code", Meta Muse's form, which never + reaches the transport's CONNECT hook) is retried once after 2s, when the + transport's resolver has had a chance to re-read the credentials in the + background. + +### Changed + +- `canonicalPath` resolves a path that does not exist yet through its + nearest existing ancestor, not only its parent directory, so two paths + that will name the same file compare equal before either is written. + +## [v0.2.4] - 2026-09-23 + ### Fixed - Heartbeat blocks are inserted as literal text. Rewrites went through diff --git a/README.md b/README.md index 1223ef2..76b5de0 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,8 @@ Skill injector plugin for the Pilot Protocol daemon. Installs and keeps current the `SKILL.md` files in each detected agent tool's well-known -directory (Claude Code, OpenClaw, PicoClaw, OpenHands, Hermes). +directory (Claude Code, OpenClaw, PicoClaw, OpenHands, Hermes, and Meta +Muse on hosts marked as Muse targets). Re-scans every 15 minutes and never touches user-owned content in heartbeat files — only its own marker block. @@ -57,6 +58,25 @@ to be earned with full transparency, so here is the whole story: rewritten. The retired plugin's `index.mjs` is replaced with a no-op instead, and the daemon log and report say so once. Every removal is logged with its path. +- **Generic directories need an explicit opt-in.** Tools listed under the + manifest's `gatedTools` key (today Meta Muse, which loads skills from + `~/workspace/skills`) are installed only on hosts that carry the tool's + marker file under `~/.pilot`, and only when that marker names the tool's + skills directory and format. The Muse installer writes + `~/.pilot/targets/muse` when it installs the Muse-format skills into + `~/workspace/skills`: + + ``` + skills_dir=/root/workspace/skills + skill_format=muse + ``` + + An empty marker, or one for another folder or for the canonical + frontmatter (`skill_format=canonical`), turns nothing on. Without a + matching marker nothing in that directory is read, written or removed. + Writes stay inside the tool's `rootDir`, never follow a symlink, and only + the entrypoint `SKILL.md` is written (rewritten into the frontmatter shape + Muse loads). Releases that predate the key ignore it. See `gated.go`. - **It is opt-out, anytime.** Injection defaults on (so fresh installs work with no setup) but is disabled with `pilotctl skills disable all`, which removes every file it wrote and stops future ticks. The flag persists in @@ -92,6 +112,27 @@ report, err = skillinject.Plan(ctx, skillinject.Config{ /* ... */ }) removed, err := skillinject.Uninstall(ctx, skillinject.Config{ /* ... */ }) ``` +### Egress proxies that rotate their credentials + +Fetches follow the proxy environment (`HTTPS_PROXY`, `NO_PROXY`, ...). Some +sandboxes (Meta Muse) rotate the credentials in `HTTPS_PROXY` every few +minutes, and a long-running daemon keeps the ones it was started with. When +`Config.HTTPClient` is nil, the client re-reads them with the same refresh +command pilot-daemon uses: `Config.ProxyCommand`, else `$PILOT_PROXY_CMD`, +else `"proxy_cmd"` in `~/.pilot/config.json`. On such hosts the Pilot +installer (pilot-protocol/release#49) saves the sandbox command there, +`pilotctl daemon start` (pilotprotocol#470) and pilot-sandbox's `pilot-up.sh` +hand it to the daemon as `$PILOT_PROXY_CMD`, and pilot-mcp setup does the +same: `bash -c 'case $https_proxy in *@*) printf %s "$https_proxy";; *) printf %s "${HTTPS_PROXY:-$https_proxy}";; esac'`. +The command runs at the start of each tick, again once a minute while it +runs, and when the proxy rejects the credentials (a 407, or an answer +net/http cannot parse), after which the refused request is retried once. +Its output is never logged. `PILOT_PROXY=off` (or `config.json` `"proxy": +"off"`) turns this off. With no command in sight (a daemon given only the +`-proxy-cmd` flag) the plain client retries a 407 its transport reports +once, and a garbled rejection once after 2s, which succeeds when the +daemon's own resolver re-read the credentials meanwhile. See `proxy.go`. + ## Layout | File | What it does | @@ -102,6 +143,9 @@ removed, err := skillinject.Uninstall(ctx, skillinject.Config{ /* ... */ }) | `reconcile.go` | Per-tick state machine: Absent → install, Drifted → rewrite, Identical → noop. | | `state.go` | File-state classifier (sha256 + heartbeat-marker parsing). | | `uninstall.go` | Strip-only on co-inhabited files; delete-safe in pilot-owned subdirs. | +| `gated.go` | Marker-gated targets (`gatedTools`, e.g. Meta Muse): active only while `requireMarker` names the target; contained writes. | +| `proxy.go` | Default HTTP client: re-reads rotating egress proxy credentials with the daemon's refresh command. | +| `skillformat.go` | Per-target SKILL.md rewrites (`skillFormat: "muse"`). | | `retired.go` | Surfaces older manifests installed and the current one dropped; removed on every tick and on uninstall. | | `plugin_allowlist.go` | OpenClaw allow-list JSON merge and `.pilot-bak` snapshot. | | `service.go` | `*Service` — `coreapi.Service` adapter. Build tag `!no_skillinject`. | diff --git a/gated.go b/gated.go new file mode 100644 index 0000000..2db2530 --- /dev/null +++ b/gated.go @@ -0,0 +1,490 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package skillinject + +// Gated tools. +// +// A regular tool is detected by its rootDir existing (~/.claude, +// ~/.openclaw, ...). That does not work for Meta Muse, which loads skills +// from ~/workspace/skills: many hosts have a directory by that name that +// has nothing to do with Muse. A gated tool is therefore active only while +// its marker file exists and names the tool's skills directory and format. +// The marker has to be inside ~/.pilot, which only Pilot's own installers +// write; the Muse installer creates ~/.pilot/targets/muse when it installs +// the Muse-format skills into ~/workspace/skills. +// +// Gated tools come from the manifest's "gatedTools" key, never from +// "tools". Released daemons decode the manifest into a struct without that +// field and encoding/json drops the key, so they never act on these rows. +// A row in "tools" would be installed by every released daemon on every +// host where the directory exists, with no marker check and no frontmatter +// rewrite. +// +// Rules: +// +// - requireMarker must resolve inside ~/.pilot. Otherwise the row is an +// error and nothing happens. +// - The marker has to say which target it is for (see "Marker contents" +// below). While it is absent, or names another skills directory or +// format, the tool is skipped: nothing under rootDir is read, written +// or removed, by a tick or by Uninstall. +// - rootDir must be inside the home directory and exist, skillsDir must +// be rootDir or inside it, and the skill file must be inside skillsDir. +// - Every file operation goes through an os.Root opened on rootDir, so no +// symlink can take a read, write or removal outside rootDir, even one +// created between a check and the write. On top of that, the skill file +// and every directory between rootDir and it must not be symlinks at +// all: a link there is not ours, and following it would write into +// someone else's skill. +// - The new content goes to a temp file created with O_EXCL under a +// random name and is renamed over the skill file, so a link planted at +// a predictable temp name is never followed. +// - Only the entrypoint skill copy is installed. There is no heartbeat and +// no plugin. +// +// Marker contents. A marker file only proves that some Pilot installer ran, +// and the Muse installer can be pointed at any skills folder +// (MUSE_SKILLS_DIR) or told to keep the canonical frontmatter +// (PILOT_MUSE_FRONTMATTER=0). So the marker records what the installer set +// up, one key=value per line: +// +// skills_dir=/root/workspace/skills +// skill_format=muse +// +// skills_dir is the folder the installer wrote the skills to (an absolute +// path, or one starting with "~/"), and skill_format the frontmatter it gave +// them ("muse", or "canonical" for the SKILL.md as published). The tool is +// active only when skills_dir is the row's skillsDir (compared once +// symlinks are resolved) and skill_format is the row's skillFormat. Blank +// lines, lines starting with '#' and unknown keys are ignored. A marker +// that is empty (a bare touch), is not a regular file, is larger than +// 4 KiB, has a line without '=', or sets a key twice attests nothing, and +// the tool stays off. Otherwise the daemon would write the Muse copy into +// an unrelated ~/workspace/skills on a host where the installer served +// another agent's folder, and rewrite a canonical copy the operator asked +// for on every tick. + +import ( + "bufio" + "bytes" + "crypto/rand" + "encoding/hex" + "errors" + "fmt" + "io" + "io/fs" + "log/slog" + "os" + "path/filepath" + "strings" +) + +// gatedTarget is a gated tool resolved against the home directory. +type gatedTarget struct { + rootDir string // absolute and clean + skillRel string // the skill file, relative to rootDir + path string // the skill file, absolute (for reports) + flat bool // skillNaming "flat": the file sits directly in skillsDir +} + +// gatedMarkerPath resolves gt.RequireMarker and checks that it is inside +// ~/.pilot. +func gatedMarkerPath(gt ManifestGatedTool, home string) (string, error) { + if gt.RequireMarker == "" { + return "", fmt.Errorf("gated tool %q has no requireMarker", gt.Name) + } + pilotDir := filepath.Join(home, ".pilot") + marker := filepath.Clean(expandHome(gt.RequireMarker, home)) + if !filepath.IsAbs(marker) || marker == pilotDir || !pathWithin(pilotDir, marker) { + return "", fmt.Errorf("gated tool %q: requireMarker %q must be a path inside ~/.pilot", gt.Name, gt.RequireMarker) + } + return marker, nil +} + +// Marker keys and limits (see "Marker contents" above). +const ( + markerKeySkillsDir = "skills_dir" + markerKeySkillFormat = "skill_format" + // markerFormatCanonical is the skill_format of a SKILL.md copied as + // published, which a row with an empty skillFormat writes. + markerFormatCanonical = "canonical" + markerMaxBytes = 4 << 10 +) + +// gatedMarker is what a marker file says. err is set when the file is +// there but attests nothing (not a regular file, too large, malformed). +type gatedMarker struct { + skillsDir string + skillFormat string + err error +} + +// readGatedMarker reads the marker at path. present is false when there +// is no such file, or it cannot be checked, which keeps the tool off. +func readGatedMarker(path string) (m gatedMarker, present bool) { + fi, err := os.Lstat(path) + if err != nil { + return gatedMarker{}, false + } + if !fi.Mode().IsRegular() { + return gatedMarker{err: errors.New("the marker is not a regular file")}, true + } + f, err := os.Open(path) + if err != nil { + return gatedMarker{err: err}, true + } + defer f.Close() + // The file opened must be the one checked: a symlink swapped in since + // Lstat would read a file outside ~/.pilot. + if ofi, err := f.Stat(); err != nil || !os.SameFile(fi, ofi) { + return gatedMarker{err: errors.New("the marker changed while it was read")}, true + } + raw, err := io.ReadAll(io.LimitReader(f, markerMaxBytes+1)) + if err != nil { + return gatedMarker{err: err}, true + } + if len(raw) > markerMaxBytes { + return gatedMarker{err: fmt.Errorf("the marker is larger than %d bytes", markerMaxBytes)}, true + } + return parseGatedMarker(raw), true +} + +// parseGatedMarker parses the key=value lines of a marker. +func parseGatedMarker(raw []byte) gatedMarker { + var m gatedMarker + seen := map[string]bool{} + sc := bufio.NewScanner(bytes.NewReader(raw)) + for n := 1; sc.Scan(); n++ { + line := strings.TrimSpace(sc.Text()) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + key, value, ok := strings.Cut(line, "=") + if !ok { + return gatedMarker{err: fmt.Errorf("marker line %d is not key=value", n)} + } + key, value = strings.TrimSpace(key), strings.TrimSpace(value) + if seen[key] { + return gatedMarker{err: fmt.Errorf("marker sets %s twice", key)} + } + seen[key] = true + switch key { + case markerKeySkillsDir: + m.skillsDir = value + case markerKeySkillFormat: + m.skillFormat = value + } + } + if err := sc.Err(); err != nil { + return gatedMarker{err: err} + } + return m +} + +// attests reports whether the marker is for gt's target: the same skills +// directory, once symlinks are resolved, and the same skill format. The +// error says what did not match. +func (m gatedMarker) attests(gt ManifestGatedTool, home string) error { + if m.err != nil { + return m.err + } + if m.skillsDir == "" { + return fmt.Errorf("the marker does not say which skills directory it is for (%s=)", markerKeySkillsDir) + } + dir := expandHome(m.skillsDir, home) + if !filepath.IsAbs(dir) { + return fmt.Errorf("the marker's %s %q is not an absolute path", markerKeySkillsDir, m.skillsDir) + } + want := filepath.Clean(expandHome(gt.SkillsDir, home)) + if canonicalPath(dir) != canonicalPath(want) { + return fmt.Errorf("the marker is for %s, not %s", dir, want) + } + format := gt.SkillFormat + if format == "" { + format = markerFormatCanonical + } + if m.skillFormat != format { + return fmt.Errorf("the marker says %s=%q, the row writes %q", markerKeySkillFormat, m.skillFormat, format) + } + return nil +} + +// logInactive notes, at debug level, why a marked gated tool is off. +func logInactive(gt ManifestGatedTool, marker string, err error) { + slog.Debug("skillinject: gated tool inactive: its marker does not attest this target", + "tool", gt.Name, "marker", marker, "reason", err) +} + +// resolveGatedTarget checks gt's paths and returns where its skill copy +// goes. The marker is checked separately (gatedMarkerPath). +func resolveGatedTarget(gt ManifestGatedTool, entrypoint, home string) (gatedTarget, error) { + if !validIdentifier(gt.Name) { + return gatedTarget{}, fmt.Errorf("gated tool has invalid name %q", gt.Name) + } + if !validIdentifier(entrypoint) { + return gatedTarget{}, fmt.Errorf("gated tool %q: entrypoint %q is not a plain name", gt.Name, entrypoint) + } + home = filepath.Clean(home) + root := filepath.Clean(expandHome(gt.RootDir, home)) + if gt.RootDir == "" || !filepath.IsAbs(root) || root == home || !pathWithin(home, root) { + return gatedTarget{}, fmt.Errorf("gated tool %q: rootDir %q must be a directory inside the home directory", gt.Name, gt.RootDir) + } + skills := filepath.Clean(expandHome(gt.SkillsDir, home)) + if gt.SkillsDir == "" || !pathWithin(root, skills) { + return gatedTarget{}, fmt.Errorf("gated tool %q: skillsDir %q must be inside rootDir %q", gt.Name, gt.SkillsDir, gt.RootDir) + } + flat := gt.SkillNaming == "flat" + path := filepath.Join(skills, entrypoint, "SKILL.md") + if flat { + path = filepath.Join(skills, entrypoint+".md") + } + rel, err := filepath.Rel(root, path) + if err != nil || rel == "." || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { + return gatedTarget{}, fmt.Errorf("gated tool %q: skill path %q escapes rootDir %q", gt.Name, path, gt.RootDir) + } + return gatedTarget{rootDir: root, skillRel: rel, path: path, flat: flat}, nil +} + +// validIdentifier accepts a single plain path element made of letters, +// digits, '-', '_' and '.', and not "." or "..". +func validIdentifier(s string) bool { + if s == "" || s == "." || s == ".." { + return false + } + for _, r := range s { + switch { + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9', + r == '-', r == '_', r == '.': + default: + return false + } + } + return true +} + +// reconcileGatedTool installs or refreshes one gated tool's skill copy. +// skipped is true when the tool is inactive (marker absent or not for +// this target, or rootDir absent); the Outcome is then empty. taken maps +// the canonical skill paths the regular tools reconciled this tick to +// their tool names, so a gated row that points at one of them is refused +// instead of rewriting it with different bytes on every tick. +func reconcileGatedTool(gt ManifestGatedTool, entrypoint, home string, skillBody []byte, taken map[string]string, dryRun bool) (o Outcome, skipped bool) { + o = Outcome{Tool: gt.Name, Kind: KindSkill} + fail := func(err error) (Outcome, bool) { + o.Action = ActionError + o.Err = err.Error() + return o, false + } + + marker, err := gatedMarkerPath(gt, home) + if err != nil { + o.Path = gt.RequireMarker + return fail(err) + } + decl, present := readGatedMarker(marker) + if !present { + return Outcome{}, true + } + // The row is checked before the marker's contents, so a broken row is + // an error on every marked host, whatever the marker says. + t, err := resolveGatedTarget(gt, entrypoint, home) + if err != nil { + o.Path = gt.SkillsDir + return fail(err) + } + o.Path = t.path + want, err := formatSkill(skillBody, gt.SkillFormat, entrypoint) + if err != nil { + return fail(err) + } + if !dirExists(t.rootDir) { + return Outcome{}, true + } + if owner, ok := taken[canonicalPath(t.path)]; ok { + return fail(fmt.Errorf("%s is also the %s skill copy; refusing to write it twice", t.path, owner)) + } + if err := decl.attests(gt, home); err != nil { + logInactive(gt, marker, err) + return Outcome{}, true + } + o.Hash = sha256Hex(want) + + root, err := os.OpenRoot(t.rootDir) + if err != nil { + return fail(err) + } + defer root.Close() + + state, err := classifyGatedSkill(root, t.skillRel, o.Hash) + if err != nil { + return fail(err) + } + o.State = state + o.Action = actionFor(state) + if o.Action != ActionNoop && !dryRun { + if err := writeGatedSkill(root, t.skillRel, want); err != nil { + return fail(err) + } + } + return o, false +} + +// removeGatedTool is Uninstall for one gated tool: it deletes the skill +// copy and, for the directory layout, the entrypoint directory if that +// leaves it empty. It returns no rows while the tool is inactive (marker +// absent or not for this target), since nothing under rootDir may be +// touched then. +func removeGatedTool(gt ManifestGatedTool, entrypoint, home string) []Removal { + r := Removal{Tool: gt.Name, Kind: KindSkill} + fail := func(err error) []Removal { + r.Action = RemovalError + r.Err = err.Error() + return []Removal{r} + } + + marker, err := gatedMarkerPath(gt, home) + if err != nil { + r.Path = gt.RequireMarker + return fail(err) + } + decl, present := readGatedMarker(marker) + if !present { + return nil + } + t, err := resolveGatedTarget(gt, entrypoint, home) + if err != nil { + r.Path = gt.SkillsDir + return fail(err) + } + if err := decl.attests(gt, home); err != nil { + // Not this target: the file there is someone else's. + logInactive(gt, marker, err) + return nil + } + r.Path = t.path + if !dirExists(t.rootDir) { + r.Action = RemovalNoop + return []Removal{r} + } + root, err := os.OpenRoot(t.rootDir) + if err != nil { + return fail(err) + } + defer root.Close() + + if err := checkNoLinks(root, t.skillRel); err != nil { + return fail(err) + } + fi, err := root.Lstat(t.skillRel) + switch { + case errors.Is(err, fs.ErrNotExist): + r.Action = RemovalNoop + return []Removal{r} + case err != nil: + return fail(err) + case !fi.Mode().IsRegular(): + return fail(fmt.Errorf("%s is not a regular file; leaving it in place", t.path)) + } + if err := root.Remove(t.skillRel); err != nil { + return fail(err) + } + r.Action = RemovalDeleted + if dir := filepath.Dir(t.skillRel); !t.flat && dir != "." && filepath.Base(dir) == entrypoint { + // Remove fails on a directory that still has entries, which is + // the emptiness check. + _ = root.Remove(dir) + } + return []Removal{r} +} + +// classifyGatedSkill is classifySkill for a file under root. +func classifyGatedSkill(root *os.Root, rel, wantHash string) (State, error) { + if err := checkNoLinks(root, rel); err != nil { + return "", err + } + cur, err := root.ReadFile(rel) + if errors.Is(err, fs.ErrNotExist) { + return StateAbsent, nil + } + if err != nil { + return "", err + } + if sha256Hex(cur) == wantHash { + return StateIdentical, nil + } + return StateDrifted, nil +} + +// checkNoLinks fails when rel, or any directory between root and rel, is +// a symlink. A component that does not exist ends the check: nothing +// below it exists either. +func checkNoLinks(root *os.Root, rel string) error { + cur := "" + for _, part := range strings.Split(rel, string(os.PathSeparator)) { + cur = filepath.Join(cur, part) + fi, err := root.Lstat(cur) + if errors.Is(err, fs.ErrNotExist) { + return nil + } + if err != nil { + return err + } + if fi.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("%s is a symlink; refusing to follow it", filepath.Join(root.Name(), cur)) + } + } + return nil +} + +// writeGatedSkill writes content to rel under root: missing directories +// are created, the bytes go to an O_EXCL temp file with a random name in +// the same directory, and that file is renamed over rel. +func writeGatedSkill(root *os.Root, rel string, content []byte) error { + dir := filepath.Dir(rel) + if dir != "." { + if err := root.MkdirAll(dir, 0o755); err != nil { + return err + } + } + if err := checkNoLinks(root, rel); err != nil { + return err + } + f, tmp, err := createGatedTemp(root, dir, filepath.Base(rel)) + if err != nil { + return err + } + _, werr := f.Write(content) + if werr == nil { + werr = f.Chmod(0o644) // the mode writeFile gives skill copies + } + if err := errors.Join(werr, f.Close()); err != nil { + _ = root.Remove(tmp) + return err + } + if err := root.Rename(tmp, rel); err != nil { + _ = root.Remove(tmp) + return err + } + return nil +} + +// createGatedTemp creates and opens a file named "..pilot-" +// in dir with O_EXCL. It returns the file and its path relative to root. +func createGatedTemp(root *os.Root, dir, base string) (*os.File, string, error) { + for range 8 { + var b [8]byte + if _, err := rand.Read(b[:]); err != nil { + return nil, "", err + } + name := filepath.Join(dir, "."+base+".pilot-"+hex.EncodeToString(b[:])) + f, err := root.OpenFile(name, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) + if errors.Is(err, fs.ErrExist) { + continue + } + if err != nil { + return nil, "", err + } + return f, name, nil + } + return nil, "", fmt.Errorf("could not create a temp file in %s", filepath.Join(root.Name(), dir)) +} diff --git a/manifest.go b/manifest.go index 1856acd..49e2219 100644 --- a/manifest.go +++ b/manifest.go @@ -15,7 +15,6 @@ import ( "path/filepath" "strings" "text/template" - "time" ) // TODO: update to pilot-protocol/pilot-skills once that repo is transferred from TeoSlayer. @@ -43,6 +42,41 @@ type Manifest struct { // longer manages; every tick and Uninstall remove them. Merged with the // built-in list in retired.go. Optional. Retired *ManifestRetired `json:"retired,omitempty"` + // GatedTools are skill targets that are active only on hosts that + // opted in by creating a marker file under ~/.pilot (see gated.go). + // They have their own key, not rows in Tools, because releases that + // predate this field drop the key when they decode the manifest. A + // row in Tools would be installed by those releases on every host + // where its rootDir exists, with no marker check. Optional. + GatedTools []ManifestGatedTool `json:"gatedTools,omitempty"` +} + +// ManifestGatedTool is one "gatedTools" row: a skill target whose +// directory is too generic to detect by existence alone (Meta Muse loads +// skills from ~/workspace/skills). Only the entrypoint skill copy is +// installed; there is no heartbeat or plugin. +type ManifestGatedTool struct { + Name string `json:"name"` + // RootDir must exist, and must be inside the home directory. Every + // file operation for this tool stays inside it (see gated.go). + RootDir string `json:"rootDir"` + // SkillsDir must be RootDir or inside it. + SkillsDir string `json:"skillsDir"` + // SkillNaming is "" (directory layout, the default) or "flat", as + // for ManifestTool. + SkillNaming string `json:"skillNaming,omitempty"` + // RequireMarker is the file that turns the tool on, e.g. + // "~/.pilot/targets/muse". It must be inside ~/.pilot, and it must + // name SkillsDir and SkillFormat ("skills_dir=..." and + // "skill_format=..." lines, see gated.go). While it is absent, or + // names another directory or format, nothing under RootDir is read, + // written or removed. + RequireMarker string `json:"requireMarker"` + // SkillFormat names a rewrite applied to the entrypoint SKILL.md + // before it is written: "" copies it unchanged, SkillFormatMuse + // rewrites the frontmatter (see skillformat.go). An unknown value is + // an error row and nothing is written. + SkillFormat string `json:"skillFormat,omitempty"` } // ManifestHelper is one helper script the daemon installs at a @@ -156,12 +190,17 @@ type fetcher struct { publicKey ed25519.PublicKey // nil = no key resolved keyErr error // non-nil when a configured key failed to decode requireSig bool // fail fetches when publicKey is nil + // ownsTransport: httpClient is the refreshing client newFetcher built + // (see proxy.go), which retries a 407 itself and whose idle + // connections close drops. + ownsTransport bool } func newFetcher(cfg Config) *fetcher { c := cfg.HTTPClient + owns := false if c == nil { - c = &http.Client{Timeout: 30 * time.Second} + c, owns = defaultHTTPClient(cfg) } mu := cfg.ManifestURL if mu == "" { @@ -176,12 +215,22 @@ func newFetcher(cfg Config) *fetcher { } key, keyErr := resolveManifestPublicKey(cfg) return &fetcher{ - httpClient: c, - manifestURL: mu, - repoBase: rb, - publicKey: key, - keyErr: keyErr, - requireSig: requireSignedManifest(cfg), + httpClient: c, + manifestURL: mu, + repoBase: rb, + publicKey: key, + keyErr: keyErr, + requireSig: requireSignedManifest(cfg), + ownsTransport: owns, + } +} + +// close releases the idle connections of a transport newFetcher built. A +// client from Config.HTTPClient, or one on the shared +// http.DefaultTransport, is left alone. +func (f *fetcher) close() { + if f.ownsTransport { + f.httpClient.CloseIdleConnections() } } @@ -273,12 +322,29 @@ func decodeEd25519PublicKey(s string) (ed25519.PublicKey, error) { } func (f *fetcher) get(ctx context.Context, url string) ([]byte, error) { - req, err := http.NewRequestWithContext(ctx, "GET", url, nil) - if err != nil { - return nil, err + resp, err := f.do(ctx, url) + if err != nil && !f.ownsTransport && ctx.Err() == nil { + switch { + case proxyAuthRejected(err): + // The proxy refused the credentials, and the transport says so + // with a *netproxy.ConnectError: it follows a proxy resolver + // that refreshes them on a 407 (pilot-daemon's + // http.DefaultTransport does), so a second request goes out + // with the new ones. + resp, err = f.do(ctx, url) + case unreadableProxyReply(err): + // An answer net/http could not parse, which is how some + // proxies (Meta Muse's) reject expired credentials. The + // transport never saw a status, so it refreshed nothing + // itself; its resolver re-reads the credentials in the + // background once its interval has passed (pilot-daemon's: + // 60s, started by the lookup that just failed). Give that a + // moment, then retry once. + if sleepCtx(ctx, unreadableReplyRetryDelay) { + resp, err = f.do(ctx, url) + } + } } - req.Header.Set("User-Agent", "pilot-daemon/skillinject") - resp, err := f.httpClient.Do(req) if err != nil { return nil, err } @@ -290,6 +356,16 @@ func (f *fetcher) get(ctx context.Context, url string) ([]byte, error) { return io.ReadAll(io.LimitReader(resp.Body, maxBody)) } +// do sends one GET for url. +func (f *fetcher) do(ctx context.Context, url string) (*http.Response, error) { + req, err := http.NewRequestWithContext(ctx, "GET", url, nil) + if err != nil { + return nil, err + } + req.Header.Set("User-Agent", "pilot-daemon/skillinject") + return f.httpClient.Do(req) +} + // fetchManifest grabs and parses the manifest from the configured URL. func (f *fetcher) fetchManifest(ctx context.Context) (*Manifest, error) { body, err := f.getOrVerify(ctx, f.manifestURL) diff --git a/proxy.go b/proxy.go new file mode 100644 index 0000000..4cf6935 --- /dev/null +++ b/proxy.go @@ -0,0 +1,217 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package skillinject + +// Egress proxies that rotate their credentials. +// +// Some sandboxes (Meta Muse) put the egress proxy's credentials in +// HTTPS_PROXY and replace them every few minutes. A new shell sees the +// current ones. The daemon this package runs in keeps the copy it was +// started with, and net/http reads the proxy environment only once per +// process, so after the first rotation the proxy answers every new CONNECT +// with 407 and every tick after the first one fails. +// +// pilot-daemon handles this with a refresh command, a shell command that +// prints the current proxy URL (common/netproxy WithRefreshCommand). The +// daemon takes it from its -proxy-cmd flag, $PILOT_PROXY_CMD, or "proxy_cmd" +// in ~/.pilot/config.json. On sandbox hosts the Pilot installer saves the +// sandbox command there, and pilotctl, pilot-up.sh and pilot-mcp hand it to +// the daemon as $PILOT_PROXY_CMD: +// +// bash -c 'case $https_proxy in *@*) printf %s "$https_proxy";; *) printf %s "${HTTPS_PROXY:-$https_proxy}";; esac' +// +// When Config.HTTPClient is nil, the client newFetcher builds uses the same +// command (Config.ProxyCommand, then $PILOT_PROXY_CMD, then config.json): +// a netproxy.RefreshingTransport whose Resolver runs it when the tick starts, +// again once a minute while it runs, and when the proxy answers 407, after +// which the refused request is retried once. With no command the client is +// the plain one it always was, and a fetch the proxy refused with 407 is +// retried once if the process's transport reports the refusal as a +// *netproxy.ConnectError, which is what pilot-daemon's http.DefaultTransport +// does after refreshing its own credentials. A refusal the proxy garbles +// never reaches that transport's hook; it is retried once after +// unreadableReplyRetryDelay, by when the transport's resolver has usually +// re-read the credentials in the background. + +import ( + "context" + "encoding/json" + "errors" + "log/slog" + "net/http" + "os" + "strings" + "time" + + "github.com/pilot-protocol/common/netproxy" +) + +// EnvProxyCommand is the environment variable holding the refresh command +// (see Config.ProxyCommand). It is common/netproxy's EnvRefreshCommand, +// which pilot-daemon and pilotctl read too. +const EnvProxyCommand = netproxy.EnvRefreshCommand + +// EnvProxy is pilot-daemon's proxy setting. Only its "off" words matter +// here: with the daemon's proxy turned off, no refresh command is run. +const EnvProxy = "PILOT_PROXY" + +// config.json keys pilot-daemon reads its proxy settings from. +const ( + configProxyKey = "proxy" + configProxyCmdKey = "proxy_cmd" +) + +// fetchTimeout bounds one HTTP request of a tick. +const fetchTimeout = 30 * time.Second + +// proxyOffWords are the settings of PILOT_PROXY / config.json "proxy" that +// turn pilot-daemon's proxy off. +var proxyOffWords = map[string]bool{"off": true, "none": true, "no": true, "false": true, "direct": true} + +// proxyCommand returns the refresh command for the default client: +// cfg.ProxyCommand, else $PILOT_PROXY_CMD, else "proxy_cmd" in +// ~/.pilot/config.json. It returns "" when none is set, and when +// PILOT_PROXY (or, without it, config.json "proxy") turns the proxy off. +func proxyCommand(cfg Config) string { + if c := strings.TrimSpace(cfg.ProxyCommand); c != "" { + return c + } + conf := readProxyConfig(cfg.Home) + setting, fromEnv := os.LookupEnv(EnvProxy) + if !fromEnv || strings.TrimSpace(setting) == "" { + setting = conf.proxy + } + if proxyOffWords[strings.ToLower(strings.TrimSpace(setting))] { + return "" + } + if c := strings.TrimSpace(os.Getenv(EnvProxyCommand)); c != "" { + return c + } + return strings.TrimSpace(conf.proxyCmd) +} + +// proxyConfig is the proxy part of ~/.pilot/config.json. +type proxyConfig struct { + proxy string + proxyCmd string +} + +// readProxyConfig reads "proxy" and "proxy_cmd" from ~/.pilot/config.json. +// A missing or unreadable file, or a value that is not a string, reads as +// unset. +func readProxyConfig(home string) proxyConfig { + if home == "" { + h, err := os.UserHomeDir() + if err != nil { + return proxyConfig{} + } + home = h + } + b, err := os.ReadFile(configFilePath(home)) + if err != nil { + return proxyConfig{} + } + var raw map[string]json.RawMessage + if json.Unmarshal(b, &raw) != nil { + return proxyConfig{} + } + str := func(key string) string { + var s string + if v, ok := raw[key]; ok && json.Unmarshal(v, &s) == nil { + return s + } + return "" + } + return proxyConfig{proxy: str(configProxyKey), proxyCmd: str(configProxyCmdKey)} +} + +// defaultHTTPClient builds the client used when Config.HTTPClient is nil. +// refreshing reports that it re-reads the proxy credentials with a refresh +// command (and so retries a 407 itself); it then owns its transport. +func defaultHTTPClient(cfg Config) (c *http.Client, refreshing bool) { + cmd := proxyCommand(cfg) + if cmd == "" { + return &http.Client{Timeout: fetchTimeout}, false + } + r, err := netproxy.NewResolver(netproxy.ModeAuto, + netproxy.WithRefreshCommand(cmd), + netproxy.WithRefreshErrorHandler(func(err error) { + slog.Warn("skillinject: proxy credential refresh failed; keeping the last proxy settings", "err", err) + })) + if err != nil { + // An unusable HTTPS_PROXY. net/http rejects it as well; its error + // then names the problem on every fetch. + slog.Warn("skillinject: proxy settings unusable; fetching without the credential refresh", "err", err) + return &http.Client{Timeout: fetchTimeout}, false + } + return &http.Client{ + Timeout: fetchTimeout, + Transport: netproxy.RefreshingTransport(proxyBaseTransport(), r), + }, true +} + +// proxyBaseTransport returns the transport the refreshing client is built +// on: a copy of http.DefaultTransport without its CONNECT response hook. A +// host that routes DefaultTransport through its own proxy resolver (as +// pilot-daemon does) installs a hook that turns a 407 into its own error, +// which would hide the refusal from RefreshingTransport's retry. A +// variable so tests can add their CA. +var proxyBaseTransport = func() *http.Transport { + dt, ok := http.DefaultTransport.(*http.Transport) + if !ok { + return nil // RefreshingTransport starts from a zero http.Transport + } + tr := dt.Clone() + tr.OnProxyConnectResponse = nil + return tr +} + +// proxyAuthRejected reports whether err is a proxy refusing the +// credentials with 407, as a netproxy-aware transport reports it. +func proxyAuthRejected(err error) bool { + var ce *netproxy.ConnectError + return errors.As(err, &ce) && ce.StatusCode == http.StatusProxyAuthRequired +} + +// unreadableReplyRetryDelay is how long get waits before retrying a fetch +// whose proxy answer could not be parsed (see unreadableProxyReply). A +// variable so tests can shorten it. +var unreadableReplyRetryDelay = 2 * time.Second + +// unreadableProxyFaults are net/http's complaints about a response it +// cannot parse (the ones common/netproxy treats as a garbled CONNECT +// answer). Only these fixed descriptions are matched; the rest of such a +// message quotes the offending bytes. +var unreadableProxyFaults = []string{ + "malformed HTTP status code", + "malformed HTTP response", + "malformed HTTP version", + "malformed MIME header", +} + +// unreadableProxyReply reports whether err is net/http failing to parse a +// response ("malformed HTTP status code", ...), as it does for a proxy +// that rejects expired credentials with a garbled CONNECT answer (Meta +// Muse's). netproxy reports the same case from its own transports as +// "read CONNECT response: ... (response text withheld)". +func unreadableProxyReply(err error) bool { + msg := err.Error() + for _, fault := range unreadableProxyFaults { + if strings.Contains(msg, fault) { + return true + } + } + return false +} + +// sleepCtx waits d, or until ctx is done; it reports whether d passed. +func sleepCtx(ctx context.Context, d time.Duration) bool { + t := time.NewTimer(d) + defer t.Stop() + select { + case <-t.C: + return true + case <-ctx.Done(): + return false + } +} diff --git a/reconcile.go b/reconcile.go index 2542f3a..b59c393 100644 --- a/reconcile.go +++ b/reconcile.go @@ -345,16 +345,22 @@ func writeUserFile(path string, content []byte) error { // canonicalPath names the file p refers to once symlinks are resolved, so // two paths that reach the same file compare equal. A path that does not -// exist yet is resolved through its parent directory, and failing that is -// only cleaned. +// exist yet is resolved through its nearest existing ancestor, and failing +// that is only cleaned. func canonicalPath(p string) string { - if r, err := filepath.EvalSymlinks(p); err == nil { - return r - } - if d, err := filepath.EvalSymlinks(filepath.Dir(p)); err == nil { - return filepath.Join(d, filepath.Base(p)) + p = filepath.Clean(p) + rest := "" + for dir := p; ; { + if r, err := filepath.EvalSymlinks(dir); err == nil { + return filepath.Join(r, rest) + } + parent := filepath.Dir(dir) + if parent == dir { + return p + } + rest = filepath.Join(filepath.Base(dir), rest) + dir = parent } - return filepath.Clean(p) } // frontmatterRE matches a YAML frontmatter block at the very start of a diff --git a/skillformat.go b/skillformat.go new file mode 100644 index 0000000..dd7b241 --- /dev/null +++ b/skillformat.go @@ -0,0 +1,230 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package skillinject + +import ( + "bytes" + "fmt" + "regexp" + "strings" +) + +// SkillFormatMuse is the ManifestGatedTool.SkillFormat value for Meta +// Muse. Muse is known to load a SKILL.md whose frontmatter is exactly +// +// --- +// name: "" +// description: "" +// --- +// +// (a quoted name that differs from the folder name, a one-line quoted +// description, no folded YAML). museSkillMD produces that shape. +const SkillFormatMuse = "muse" + +// museDescriptionMax is the byte cap on the rewritten description. +const museDescriptionMax = 1024 + +// formatSkill applies a gated tool's SkillFormat to the entrypoint +// SKILL.md. entrypoint is the manifest entrypoint (the skill's folder +// name). +func formatSkill(body []byte, format, entrypoint string) ([]byte, error) { + switch format { + case "": + return body, nil + case SkillFormatMuse: + return museSkillMD(body, strings.ReplaceAll(entrypoint, "-", "_")), nil + default: + return nil, fmt.Errorf("unsupported skillFormat %q", format) + } +} + +// museSkillMD rewrites the frontmatter of a SKILL.md into the shape Muse +// loads (see SkillFormatMuse) and keeps the body byte for byte. Every +// frontmatter key other than name and description is dropped. A file +// that does not start with a closed "---" block is returned unchanged. +// +// It is a port of muse_frontmatter in pilot-skills muse/install.sh, which +// rewrites the skills that installer copies into ~/workspace/skills. The +// two have to produce the same bytes: otherwise the daemon rewrites the +// installer's copy on its first tick and the installer puts its own back +// on every rerun. The steps below follow that function in order, and +// TestMuseSkillMD_MatchesInstaller runs both on the same inputs. +func museSkillMD(src []byte, name string) []byte { + lines, offsets := splitRecords(src) + if len(lines) == 0 || !isFenceLine(lines[0]) { + return src + } + end := -1 + for i := 1; i < len(lines); i++ { + if isFenceLine(lines[i]) { + end = i + break + } + } + if end < 0 { + return src + } + + desc := normalizeMuseDescription(extractDescription(lines[1:end]), name) + + // The body is everything after the closing fence's line, as + // `tail -n +` prints it. + body := []byte(nil) + if end+1 < len(offsets) { + body = src[offsets[end+1]:] + } + + var out bytes.Buffer + out.Grow(len(name) + len(desc) + len(body) + 40) + out.WriteString("---\nname: \"") + out.WriteString(name) + out.WriteString("\"\ndescription: \"") + out.WriteString(desc) + out.WriteString("\"\n---\n") + out.Write(body) + return out.Bytes() +} + +// splitRecords splits s into awk records (lines without their "\n"; a +// trailing "\n" does not start another record) and returns the byte +// offset at which each record starts. +func splitRecords(s []byte) ([]string, []int) { + var lines []string + var offsets []int + start := 0 + for start < len(s) { + offsets = append(offsets, start) + i := bytes.IndexByte(s[start:], '\n') + if i < 0 { + lines = append(lines, string(s[start:])) + start = len(s) + break + } + lines = append(lines, string(s[start:start+i])) + start += i + 1 + } + return lines, offsets +} + +// isFenceLine matches awk's /^---\r?$/. +func isFenceLine(l string) bool { + return l == "---" || l == "---\r" +} + +var ( + // descBlockRE is a block scalar indicator (| or >, with optional + // chomping/indent indicators and a trailing comment). + descBlockRE = regexp.MustCompile(`^[|>][-+0-9]*([ \t]+#.*)?$`) + // descCommentRE is a trailing " # comment" on a plain scalar. + descCommentRE = regexp.MustCompile(`[ \t]#.*$`) +) + +// trimBlank trims spaces and tabs, as the installer's awk trim(). +func trimBlank(s string) string { + return strings.Trim(s, " \t") +} + +// extractDescription returns the raw description value from the +// frontmatter lines between the fences: the text after "description:", +// with continuation lines (indented or empty) trimmed and joined by one +// space. Block scalars and quoted scalars keep " #"; plain scalars lose +// a trailing comment. +func extractDescription(lines []string) string { + const ( + seeking = iota + collecting + done + ) + state := seeking + block, quoted := false, false + value := "" + for _, l := range lines { + l = strings.TrimSuffix(l, "\r") + if state == collecting { + if strings.HasPrefix(l, " ") || strings.HasPrefix(l, "\t") || l == "" { + line := trimBlank(l) + if !block && !quoted { + line = stripComment(line) + } + if line != "" { + value += " " + line + } + continue + } + state = done + } + if state == seeking && strings.HasPrefix(l, "description:") { + v := trimBlank(strings.TrimPrefix(l, "description:")) + switch { + case descBlockRE.MatchString(v): + block = true + v = "" + case strings.HasPrefix(v, `"`) || strings.HasPrefix(v, "'"): + quoted = true + default: + v = stripComment(v) + } + value = v + state = collecting + } + } + return value +} + +func stripComment(s string) string { + if loc := descCommentRE.FindStringIndex(s); loc != nil { + return s[:loc[0]] + } + return s +} + +// normalizeMuseDescription turns the raw value into the one-line, +// double-quote-escaped text written after description:. +func normalizeMuseDescription(v, name string) string { + // tr -d '\000-\010\013-\037' | tr '\t\n' ' ' | tr -s ' ' + b := make([]byte, 0, len(v)) + for i := 0; i < len(v); i++ { + c := v[i] + switch { + case c <= 0x08, c >= 0x0b && c <= 0x1f: + continue + case c == '\t', c == '\n': + c = ' ' + } + if c == ' ' && len(b) > 0 && b[len(b)-1] == ' ' { + continue + } + b = append(b, c) + } + d := string(b) + d = strings.TrimPrefix(d, " ") + d = strings.TrimSuffix(d, " ") + + // Unquote a quoted scalar. + switch { + case len(d) >= 2 && d[0] == '"' && d[len(d)-1] == '"': + d = d[1 : len(d)-1] + d = strings.ReplaceAll(d, `\\`, "\x01") + d = strings.ReplaceAll(d, `\"`, `"`) + d = strings.ReplaceAll(d, `\n`, " ") + d = strings.ReplaceAll(d, `\t`, " ") + d = strings.ReplaceAll(d, "\x01", `\`) + case len(d) >= 2 && d[0] == '\'' && d[len(d)-1] == '\'': + d = d[1 : len(d)-1] + d = strings.ReplaceAll(d, "''", "'") + } + + if d == "" { + d = "Pilot Protocol skill " + name + } + if len(d) > museDescriptionMax { + d = d[:museDescriptionMax-4] + if i := strings.LastIndex(d, " "); i >= 0 { + d = d[:i] + } + d += "..." + } + d = strings.ReplaceAll(d, `\`, `\\`) + d = strings.ReplaceAll(d, `"`, `\"`) + return d +} diff --git a/skillinject.go b/skillinject.go index f88b587..811fd16 100644 --- a/skillinject.go +++ b/skillinject.go @@ -72,8 +72,20 @@ type Config struct { // RepoBaseURL overrides the prefix used to resolve relative paths in // the manifest (skills//SKILL.md, heartbeats/.md). RepoBaseURL string - // HTTPClient overrides the HTTP client used for fetching. + // HTTPClient overrides the HTTP client used for fetching. When nil, + // the client follows the proxy environment and, with a ProxyCommand, + // re-reads rotating proxy credentials (see proxy.go). HTTPClient *http.Client + // ProxyCommand is a shell command that prints the current egress + // proxy URL, for proxies that rotate the credentials in HTTPS_PROXY + // (Meta Muse): e.g. bash -c 'printf %s "$https_proxy"' (see proxy.go for the sandbox command). + // The default client runs it at the start of each tick, again once a + // minute while the tick runs, and when the proxy answers 407, and + // retries the refused request once. Empty means $PILOT_PROXY_CMD, then + // "proxy_cmd" in ~/.pilot/config.json (pilot-daemon's own settings), + // unless PILOT_PROXY or config.json "proxy" turns the proxy off. + // Ignored when HTTPClient is set. + ProxyCommand string // ManifestPublicKey, when set, enables Ed25519 detached-signature // verification on manifest + all fetched repo files. The daemon // fetches .sig alongside each resource and verifies before @@ -215,6 +227,7 @@ func tick(ctx context.Context, cfg Config, dryRun bool) (*Report, error) { } f := newFetcher(cfg) + defer f.close() manifest, err := f.fetchManifest(ctx) if err != nil { @@ -251,6 +264,11 @@ func tick(ctx context.Context, cfg Config, dryRun bool) (*Report, error) { type hbOwner struct{ tool, hash string } hbOwners := map[string]hbOwner{} + // Skill copies the regular tools reconciled this tick, by the file + // they resolve to. A gated tool pointing at one of them is refused + // (see reconcileGatedTool). + skillOwners := map[string]string{} + // (0) install host-wide helpers (e.g. ~/.pilot/bin/pilot-ask). These // are tool-agnostic and referenced from every tool's heartbeat // directive. Failure is best-effort: we record an error outcome and @@ -296,6 +314,7 @@ func tick(ctx context.Context, cfg Config, dryRun bool) (*Report, error) { // (a) skill copy skillPath := skillTargetPath(mt, manifest.Entrypoint, home) + skillOwners[canonicalPath(skillPath)] = mt.Name state := classifySkill(skillPath, skillHash) action := actionFor(state) o := Outcome{ @@ -355,6 +374,17 @@ func tick(ctx context.Context, cfg Config, dryRun bool) (*Report, error) { } } + // Gated tools (gated.go): the skill copy only, and only on hosts that + // carry the tool's marker under ~/.pilot. + for _, gt := range manifest.GatedTools { + o, skipped := reconcileGatedTool(gt, manifest.Entrypoint, home, skillBody, skillOwners, dryRun) + if skipped { + report.Skipped = append(report.Skipped, gt.Name) + continue + } + report.Outcomes = append(report.Outcomes, o) + } + // (e) retired surfaces: marker blocks, plugins and helpers that an // earlier manifest installed and the current one no longer manages. // Runs after the active surfaces so a plugin allow-list merge above diff --git a/testdata/inject-manifest-gated.json b/testdata/inject-manifest-gated.json new file mode 100644 index 0000000..e8381e4 --- /dev/null +++ b/testdata/inject-manifest-gated.json @@ -0,0 +1,149 @@ +{ + "version": 1, + "entrypoint": "pilotctl", + "description": "Routing manifest consumed by the pilot-daemon's skillinject loop. Declares the entrypoint skill, the curated set of skills referenced from the entrypoint, and per-tool injection paths. The pilot-daemon fetches this file at runtime from the pilot-skills repo over HTTPS — there is no embedded copy.", + "referencedSkills": [ + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-protocol/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-momy/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-sandbox/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-verify/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-trust-circle/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-auto-trust/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-chat/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-group-chat/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-announce-capabilities/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-academic/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-books/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-climate/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-culture/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-data/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-dev/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-economics/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-entertainment/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-finance/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-flights/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-food/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-geo/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-gov-finance/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-government/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-health/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-infra/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-knowledge/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-language/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-music/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-nature/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-news/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-packages/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-reference/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-science/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-security/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-space/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-sports/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-traffic/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-transit/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-vehicles/SKILL.md", + "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/TeoSlayer/pilot-skills/blob/main/skills/pilot-service-agents-weather/SKILL.md" + ], + "helpers": [], + "tools": [ + { + "name": "openclaw", + "rootDir": "~/.openclaw", + "skillsDir": "~/.openclaw/skills", + "heartbeatPath": "~/.openclaw/workspace/AGENTS.md", + "heartbeatTemplate": "heartbeats/openclaw.md", + "plugins": [ + { + "id": "pilotprotocol-webhook-receiver", + "installPath": "~/.openclaw/extensions/pilotprotocol-webhook-receiver", + "files": [ + { + "name": "openclaw.plugin.json", + "src": "workflow-injection/openclaw/pilotprotocol-webhook-receiver/openclaw.plugin.json" + }, + { + "name": "index.mjs", + "src": "workflow-injection/openclaw/pilotprotocol-webhook-receiver/index.mjs" + } + ], + "allowList": { + "configPath": "~/.openclaw/openclaw.json", + "allowListJsonPath": "plugins.allow", + "entriesJsonPath": "plugins.entries" + } + } + ], + "webhookURL": "http://127.0.0.1:18789/pilot-webhook" + }, + { + "name": "picoclaw", + "rootDir": "~/.picoclaw", + "skillsDir": "~/.picoclaw/workspace/skills", + "heartbeatPath": "~/.picoclaw/workspace/HEARTBEAT.md", + "heartbeatTemplate": "heartbeats/picoclaw.md" + }, + { + "name": "hermes", + "rootDir": "~/.hermes", + "skillsDir": "~/.hermes/skills", + "heartbeatPath": "~/.hermes/SOUL.md", + "heartbeatTemplate": "heartbeats/hermes.md", + "webhookRoutes": [ + { + "configPath": "~/.hermes/config.yaml", + "routesYamlPath": "platforms.webhook.extra.routes", + "routeName": "pilot-events", + "route": { + "secret": "PILOT_PLACEHOLDER_OPERATOR_MUST_REPLACE", + "events": [ + "message.received", + "file.received", + "handshake.received", + "trust.changed" + ], + "prompt": "Pilot {event}: from={data.from} size={data.size}", + "deliver_only": true + } + } + ], + "webhookURL": "http://127.0.0.1:8644/pilot-events" + }, + { + "name": "claude-code", + "rootDir": "~/.claude", + "skillsDir": "~/.claude/skills", + "heartbeatPath": "~/.claude/CLAUDE.md", + "heartbeatTemplate": "heartbeats/claude-code.md" + }, + { + "name": "openhands", + "rootDir": "~/.openhands", + "skillsDir": "~/.openhands/microagents", + "skillNaming": "flat", + "selfHeartbeat": true + }, + { + "name": "goose", + "rootDir": "~/.config/goose", + "skillsDir": "~/.config/goose/skills", + "heartbeatPath": "~/.config/goose/.goosehints", + "heartbeatTemplate": "heartbeats/goose.md" + }, + { + "name": "opencode", + "rootDir": "~/.config/opencode", + "skillsDir": "~/.config/opencode/skills", + "heartbeatPath": "~/.config/opencode/AGENTS.md", + "heartbeatTemplate": "heartbeats/opencode.md" + } + ], + "gatedTools": [ + { + "name": "muse", + "rootDir": "~/workspace/skills", + "skillsDir": "~/workspace/skills", + "requireMarker": "~/.pilot/targets/muse", + "skillFormat": "muse" + } + ] +} diff --git a/testdata/muse_frontmatter.sh b/testdata/muse_frontmatter.sh new file mode 100644 index 0000000..bcd8a21 --- /dev/null +++ b/testdata/muse_frontmatter.sh @@ -0,0 +1,75 @@ +# muse_frontmatter, copied verbatim from pilot-skills muse/install.sh +# (TeoSlayer/pilot-skills@71789adfe784d0f55ae592f137e4b1aabc45be97, PR #34). +# The Muse installer rewrites the skills it copies into ~/workspace/skills with +# this function; museSkillMD (skillformat.go) must produce the same bytes. +# TestMuseSkillMD_MatchesInstaller sources this file and compares the two. +# If the installer's function changes, copy it here again. + +muse_frontmatter() { + local file="$1" name="${2:-}" end desc + if [ -z "$name" ]; then + name="$(basename "$(dirname "$file")")" + name="${name//-/_}" + fi + end="$(awk 'NR == 1 { if ($0 !~ /^---\r?$/) exit; next } /^---\r?$/ { print NR; exit }' "$file")" || return 1 + [ -n "$end" ] || return 0 + desc="$(awk -v end="$end" ' + function trim(s) { sub(/^[ \t]+/, "", s); sub(/[ \t]+$/, "", s); return s } + NR == 1 { next } + NR >= end { exit } + { + sub(/\r$/, "") + if (state == 1) { + if ($0 ~ /^[ \t]/ || $0 == "") { + line = trim($0) + if (!block && !quoted) sub(/[ \t]#.*$/, "", line) + if (line != "") value = value " " line + next + } + state = 2 + } + if (state == 0 && $0 ~ /^description:/) { + v = $0 + sub(/^description:/, "", v) + v = trim(v) + if (v ~ /^[|>][-+0-9]*([ \t]+#.*)?$/) { block = 1; v = "" } + else if (v ~ /^["\047]/) quoted = 1 + else sub(/[ \t]#.*$/, "", v) + value = v + state = 1 + } + } + END { printf "%s", value } + ' "$file")" || return 1 + desc="$(printf '%s' "$desc" | tr -d '\000-\010\013-\037' | tr '\t\n' ' ' | tr -s ' ')" + desc="${desc# }" + desc="${desc% }" + case "$desc" in + \"*\") + desc="${desc#\"}" + desc="${desc%\"}" + desc="${desc//\\\\/$'\001'}" # protect escaped backslashes + desc="${desc//\\\"/\"}" + desc="${desc//\\n/ }" + desc="${desc//\\t/ }" + desc="${desc//$'\001'/\\}" + ;; + \'*\') + local q="'" + desc="${desc#"$q"}" + desc="${desc%"$q"}" + desc="${desc//"$q$q"/$q}" + ;; + esac + if [ -z "$desc" ]; then desc="Pilot Protocol skill ${name}"; fi + if [ "$(printf '%s' "$desc" | LC_ALL=C wc -c | tr -d ' ')" -gt 1024 ]; then + desc="$(printf '%s' "$desc" | LC_ALL=C cut -c1-1020)" + desc="${desc% *}..." + fi + desc="${desc//\\/\\\\}" + desc="${desc//\"/\\\"}" + { + printf -- '---\nname: "%s"\ndescription: "%s"\n---\n' "$name" "$desc" + tail -n "+$((end + 1))" "$file" + } > "$file.muse-tmp" && mv -f "$file.muse-tmp" "$file" +} diff --git a/uninstall.go b/uninstall.go index d136604..86677cd 100644 --- a/uninstall.go +++ b/uninstall.go @@ -89,6 +89,8 @@ func (r *RemovalReport) Counts() map[RemovalKind]int { // - Surfaces an earlier manifest installed and the current one has // retired (retired.go) are removed with the same rules, so a path the // manifest stopped writing is not left behind. +// - A gated tool's skill copy is deleted only while the tool's marker +// exists (gated.go); without it nothing under its rootDir is touched. // // Network failures are tolerated: if the manifest can't be fetched we // fall back to the cached copy under ~/.pilot/skills-cache/. If that's @@ -182,6 +184,12 @@ func Uninstall(ctx context.Context, cfg Config) (*RemovalReport, error) { } } + // Gated tools (gated.go): the skill copy, and only while the tool's + // marker exists. + for _, gt := range manifest.GatedTools { + report.Removals = append(report.Removals, removeGatedTool(gt, manifest.Entrypoint, home)...) + } + // (e) Retired surfaces. After the active plugins, because restoring // openclaw.json from .pilot-bak above can bring back a retired id that // was in the config when the snapshot was taken. @@ -195,6 +203,7 @@ func Uninstall(ctx context.Context, cfg Config) (*RemovalReport, error) { // manifest, whether we used the offline fallback, and any error. func loadManifestForUninstall(ctx context.Context, cfg Config, home string) (*Manifest, bool, error) { f := newFetcher(cfg) + defer f.close() if m, err := f.fetchManifest(ctx); err == nil { return m, false, nil } diff --git a/zz_gated_marker_test.go b/zz_gated_marker_test.go new file mode 100644 index 0000000..9c01d0f --- /dev/null +++ b/zz_gated_marker_test.go @@ -0,0 +1,274 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package skillinject_test + +import ( + "context" + "os" + "path/filepath" + "sort" + "strings" + "testing" + + "github.com/pilot-protocol/skillinject" +) + +// canonicalMuseCopy is what muse/install.sh writes with +// PILOT_MUSE_FRONTMATTER=0: the SKILL.md as published. +const canonicalMuseCopy = museEntrypoint + +// treeUnder lists every path under dir, relative to it, sorted. +func treeUnder(t *testing.T, dir string) []string { + t.Helper() + var out []string + err := filepath.WalkDir(dir, func(p string, _ os.DirEntry, err error) error { + if err != nil { + return err + } + if rel, _ := filepath.Rel(dir, p); rel != "." { + out = append(out, rel) + } + return nil + }) + if err != nil { + t.Fatal(err) + } + sort.Strings(out) + return out +} + +// A marker that does not name ~/workspace/skills and the muse format +// turns nothing on. Each case starts from a ~/workspace/skills that holds +// an unrelated skill and a pilotctl copy that is not the daemon's; a tick, +// Plan and Uninstall must leave both alone and report muse as skipped. +func TestGated_MarkerMustAttestTarget(t *testing.T) { + t.Parallel() + other := func(home string) string { return filepath.Join(home, ".agentx", "skills") } + cases := map[string]func(t *testing.T, home string){ + // The finding: the installer ran with MUSE_SKILLS_DIR pointing at + // another agent's folder. + "another skills dir": func(t *testing.T, home string) { + writeMarker(t, home, "skills_dir="+other(home)+"\nskill_format=muse\n") + }, + "another skills dir, tilde": func(t *testing.T, home string) { + writeMarker(t, home, "skills_dir=~/.agentx/skills\nskill_format=muse\n") + }, + // PILOT_MUSE_FRONTMATTER=0: the installer kept the canonical copy. + "canonical format": func(t *testing.T, home string) { + writeMarker(t, home, "skills_dir="+filepath.Join(home, "workspace", "skills")+"\nskill_format=canonical\n") + }, + "bare touch": func(t *testing.T, home string) { + writeMarker(t, home, "") + }, + "only comments": func(t *testing.T, home string) { + writeMarker(t, home, "# muse\n\n") + }, + "no skill_format": func(t *testing.T, home string) { + writeMarker(t, home, "skills_dir="+filepath.Join(home, "workspace", "skills")+"\n") + }, + "no skills_dir": func(t *testing.T, home string) { + writeMarker(t, home, "skill_format=muse\n") + }, + "relative skills_dir": func(t *testing.T, home string) { + writeMarker(t, home, "skills_dir=workspace/skills\nskill_format=muse\n") + }, + "parent of skills_dir": func(t *testing.T, home string) { + writeMarker(t, home, "skills_dir="+filepath.Join(home, "workspace")+"\nskill_format=muse\n") + }, + "line without =": func(t *testing.T, home string) { + writeMarker(t, home, "skills_dir="+filepath.Join(home, "workspace", "skills")+"\nskill_format=muse\nmuse\n") + }, + "key set twice": func(t *testing.T, home string) { + writeMarker(t, home, "skills_dir="+other(home)+"\nskills_dir="+filepath.Join(home, "workspace", "skills")+"\nskill_format=muse\n") + }, + "larger than 4 KiB": func(t *testing.T, home string) { + writeMarker(t, home, "skills_dir="+filepath.Join(home, "workspace", "skills")+"\nskill_format=muse\n#"+strings.Repeat("x", 4096)+"\n") + }, + "marker is a symlink": func(t *testing.T, home string) { + decl := filepath.Join(home, "decl") + mustWriteFile(t, decl, "skills_dir="+filepath.Join(home, "workspace", "skills")+"\nskill_format=muse\n", 0o644) + mustMkdirAll(t, filepath.Join(home, ".pilot", "targets")) + if err := os.Symlink(decl, filepath.Join(home, ".pilot", "targets", "muse")); err != nil { + t.Fatal(err) + } + }, + "marker is a directory": func(t *testing.T, home string) { + mustMkdirAll(t, filepath.Join(home, ".pilot", "targets", "muse")) + }, + } + for name, mark := range cases { + t.Run(name, func(t *testing.T) { + t.Parallel() + home := t.TempDir() + ws := filepath.Join(home, "workspace") + mustMkdirAll(t, filepath.Join(ws, "skills", "their-skill")) + mustWriteFile(t, filepath.Join(ws, "skills", "their-skill", "SKILL.md"), "theirs", 0o644) + mustMkdirAll(t, filepath.Dir(museSkillPath(home))) + mustWriteFile(t, museSkillPath(home), canonicalMuseCopy, 0o644) + mustMkdirAll(t, other(home)) + mark(t, home) + before := treeUnder(t, ws) + r := newGatedRepo(t, museGated()) + + for i := 0; i < 2; i++ { + rep, err := skillinject.Tick(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Tick: %v", err) + } + if o, ok := toolOutcome(rep, "muse"); ok { + t.Fatalf("muse acted on a marker that does not attest it: %+v", o) + } + if !contains(rep.Skipped, "muse") { + t.Errorf("muse should be skipped, Skipped=%v", rep.Skipped) + } + } + rep, err := skillinject.Plan(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Plan: %v", err) + } + if o, ok := toolOutcome(rep, "muse"); ok { + t.Errorf("Plan planned muse: %+v", o) + } + rr, err := skillinject.Uninstall(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Uninstall: %v", err) + } + for _, x := range rr.Removals { + if x.Tool == "muse" { + t.Errorf("Uninstall acted on muse: %+v", x) + } + } + if got := treeUnder(t, ws); strings.Join(got, ",") != strings.Join(before, ",") { + t.Errorf("~/workspace changed: %v, was %v", got, before) + } + if got := mustRead(t, museSkillPath(home)); got != canonicalMuseCopy { + t.Errorf("the pilotctl copy was rewritten: %q", got) + } + if entries, _ := os.ReadDir(other(home)); len(entries) != 0 { + t.Errorf("the marker's own skills_dir was written: %v", entries) + } + }) + } +} + +// The spellings a correct marker may use: "~/", a trailing slash, +// surrounding blanks, CRLF, comments, unknown keys, and a path that +// reaches ~/workspace/skills through a symlink. +func TestGated_MarkerAttestsTarget(t *testing.T) { + t.Parallel() + cases := map[string]func(home string) string{ + "absolute": func(home string) string { + return "skills_dir=" + filepath.Join(home, "workspace", "skills") + "\nskill_format=muse\n" + }, + "tilde": func(string) string { return "skills_dir=~/workspace/skills\nskill_format=muse" }, + "trailing /": func(string) string { return "skills_dir=~/workspace/skills/\nskill_format=muse\n" }, + "blanks": func(string) string { return " skills_dir = ~/workspace/skills \n\tskill_format= muse\n" }, + "crlf": func(string) string { return "skills_dir=~/workspace/skills\r\nskill_format=muse\r\n" }, + "unknown keys": func(string) string { + return "# by muse/install.sh\nversion=2\nskill_format=muse\ninstalled_by=muse/install.sh\nskills_dir=~/workspace/skills\n" + }, + "through link": func(string) string { return "skills_dir=~/ws-link/skills\nskill_format=muse\n" }, + "real via link": func(home string) string { + return "skills_dir=" + filepath.Join(home, "real-ws", "skills") + "\nskill_format=muse\n" + }, + } + for name, decl := range cases { + t.Run(name, func(t *testing.T) { + t.Parallel() + home := t.TempDir() + switch name { + case "through link": + mustMkdirAll(t, filepath.Join(home, "workspace", "skills")) + if err := os.Symlink(filepath.Join(home, "workspace"), filepath.Join(home, "ws-link")); err != nil { + t.Fatal(err) + } + case "real via link": + // ~/workspace itself is a link to where the files live. + mustMkdirAll(t, filepath.Join(home, "real-ws", "skills")) + if err := os.Symlink(filepath.Join(home, "real-ws"), filepath.Join(home, "workspace")); err != nil { + t.Fatal(err) + } + default: + mustMkdirAll(t, filepath.Join(home, "workspace", "skills")) + } + writeMarker(t, home, decl(home)) + r := newGatedRepo(t, museGated()) + + rep, err := skillinject.Tick(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Tick: %v", err) + } + if o, ok := toolOutcome(rep, "muse"); !ok || o.Action != skillinject.ActionCreate { + t.Fatalf("muse outcome = %+v (found %v), want create", o, ok) + } + if got := mustRead(t, museSkillPath(home)); got != museWant { + t.Errorf("muse SKILL.md = %q", got) + } + }) + } +} + +// PILOT_MUSE_FRONTMATTER=0 and =1 runs of the installer, in turn, over one +// ~/workspace/skills: the daemon follows the marker the last run left and +// never flips the file back on its own. +func TestGated_FormatOptOutIsNotOverridden(t *testing.T) { + t.Parallel() + home := t.TempDir() + mustMkdirAll(t, filepath.Dir(museSkillPath(home))) + r := newGatedRepo(t, museGated()) + install := func(format, copy string) { + mustWriteFile(t, museSkillPath(home), copy, 0o644) + writeMarker(t, home, "skills_dir=~/workspace/skills\nskill_format="+format+"\n") + } + tick := func() (skillinject.Outcome, bool) { + t.Helper() + rep, err := skillinject.Tick(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Tick: %v", err) + } + return toolOutcome(rep, "muse") + } + + install("canonical", canonicalMuseCopy) + for i := 0; i < 2; i++ { + if o, ok := tick(); ok { + t.Fatalf("tick %d acted on the canonical copy: %+v", i, o) + } + if got := mustRead(t, museSkillPath(home)); got != canonicalMuseCopy { + t.Fatalf("tick %d rewrote the canonical copy: %q", i, got) + } + } + + install("muse", museWant) + if o, _ := tick(); o.Action != skillinject.ActionNoop { + t.Errorf("after a muse-format install, muse outcome = %+v, want noop", o) + } + + install("canonical", canonicalMuseCopy) + if o, ok := tick(); ok { + t.Errorf("after going back to canonical, muse acted: %+v", o) + } + if got := mustRead(t, museSkillPath(home)); got != canonicalMuseCopy { + t.Errorf("canonical copy rewritten: %q", got) + } +} + +// A row that is broken is an error on a marked host whatever the marker +// says, so a manifest mistake is not hidden behind a marker mismatch. +func TestGated_BrokenRowIsAnErrorEvenIfMarkerIsForAnotherTarget(t *testing.T) { + t.Parallel() + home := t.TempDir() + mustMkdirAll(t, filepath.Join(home, "workspace", "skills")) + writeMarker(t, home, "skills_dir=~/.agentx/skills\nskill_format=muse\n") + g := museGated() + g.SkillsDir = "~/.ssh" + r := newGatedRepo(t, g) + + rep, err := skillinject.Tick(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Tick: %v", err) + } + if o, _ := toolOutcome(rep, "muse"); o.Action != skillinject.ActionError { + t.Errorf("muse outcome = %+v, want an error row", o) + } +} diff --git a/zz_gated_released_internal_test.go b/zz_gated_released_internal_test.go new file mode 100644 index 0000000..b6ca9c9 --- /dev/null +++ b/zz_gated_released_internal_test.go @@ -0,0 +1,191 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package skillinject + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" +) + +// gatedManifestFixture is pilot-skills inject-manifest.json with the Meta +// Muse "gatedTools" row, byte for byte what that repo ships. +const gatedManifestFixture = "testdata/inject-manifest-gated.json" + +// releasedManifest is Manifest as every release decodes it: v0.2.2 and +// v0.2.3, the 902f745 build that pilot v1.13.2 to v1.13.10-rc.1 ship, +// and v0.2.4 (which added Retired). None has GatedTools. All of them use +// plain json.Unmarshal, which drops keys the struct does not name. +type releasedManifest struct { + Version int `json:"version"` + Entrypoint string `json:"entrypoint"` + Description string `json:"description,omitempty"` + Tools []ManifestTool `json:"tools"` + Helpers []ManifestHelper `json:"helpers,omitempty"` + Retired *ManifestRetired `json:"retired,omitempty"` +} + +// releasedParse is the parse and the checks fetchManifest makes in every +// release, unchanged since v0.2.2. +func releasedParse(body []byte) (*releasedManifest, error) { + var m releasedManifest + if err := json.Unmarshal(body, &m); err != nil { + return nil, err + } + if m.Version != 1 || m.Entrypoint == "" || len(m.Tools) == 0 { + return nil, os.ErrInvalid + } + return &m, nil +} + +// hardeningWriteRoots is the tool write-root allowlist from the pending +// path-hardening work (validateManifestPaths, branch +// wip/preserve-2026-08-02 a72ae88). It is in no release, but it rejects a +// whole manifest when any "tools" path falls outside these roots, so +// gated rows must never move into "tools" once it lands. +var hardeningWriteRoots = []string{ + "~/.pilot/bin", "~/.claude", "~/.codex", "~/.openclaw", "~/.picoclaw", + "~/.openhands", "~/.hermes", "~/.config/goose", "~/.config/opencode", +} + +func withinWriteRoots(p, home string) bool { + for _, r := range hardeningWriteRoots { + if pathWithin(expandHome(r, home), expandHome(p, home)) { + return true + } + } + return false +} + +// The manifest with the new key is accepted by the released decoder and +// checks, and nothing a released daemon acts on points at the Muse +// directory: the muse row reaches only builds that know "gatedTools". +func TestGatedManifest_ValidUnderReleasedRules(t *testing.T) { + t.Parallel() + body, err := os.ReadFile(gatedManifestFixture) + if err != nil { + t.Fatal(err) + } + var raw map[string]json.RawMessage + if err := json.Unmarshal(body, &raw); err != nil { + t.Fatal(err) + } + if _, ok := raw["gatedTools"]; !ok { + t.Fatal("fixture has no gatedTools key") + } + + old, err := releasedParse(body) + if err != nil { + t.Fatalf("released parse rejects the manifest: %v", err) + } + home := "/home/u" + for _, mt := range old.Tools { + if mt.Name == "muse" { + t.Errorf("a released daemon would see a muse tool row: %+v", mt) + } + for _, p := range []string{mt.RootDir, mt.SkillsDir, mt.HeartbeatPath} { + if p != "" && pathWithin(expandHome("~/workspace", home), expandHome(p, home)) { + t.Errorf("released tool %s writes under ~/workspace: %s", mt.Name, p) + } + } + // Every "tools" path also passes the write-root allowlist of the + // pending path hardening, so landing it does not reject this + // manifest. + for _, p := range []string{mt.RootDir, mt.SkillsDir, mt.HeartbeatPath, skillTargetPath(mt, old.Entrypoint, home)} { + if p != "" && !withinWriteRoots(p, home) { + t.Errorf("tools row %s: %s is outside the hardening write roots", mt.Name, p) + } + } + } + + // The muse row as a "tools" row would fail that allowlist and take the + // whole manifest down with it. + var cur Manifest + if err := json.Unmarshal(body, &cur); err != nil { + t.Fatal(err) + } + if len(cur.GatedTools) != 1 { + t.Fatalf("GatedTools = %+v, want the muse row", cur.GatedTools) + } + g := cur.GatedTools[0] + if withinWriteRoots(g.RootDir, home) { + t.Errorf("muse rootDir %s unexpectedly inside the tool write roots", g.RootDir) + } + want := ManifestGatedTool{ + Name: "muse", RootDir: "~/workspace/skills", SkillsDir: "~/workspace/skills", + RequireMarker: "~/.pilot/targets/muse", SkillFormat: SkillFormatMuse, + } + if g != want { + t.Errorf("muse row = %+v, want %+v", g, want) + } + if _, err := gatedMarkerPath(g, home); err != nil { + t.Errorf("muse marker rejected: %v", err) + } + if _, err := resolveGatedTarget(g, cur.Entrypoint, home); err != nil { + t.Errorf("muse row rejected: %v", err) + } +} + +// The fixture served over HTTP goes through this build's fetchManifest, +// is cached with its gatedTools key (so an offline Uninstall still finds +// the muse row), and the cached copy still parses under the released +// rules (a downgraded binary reads the same cache). +func TestGatedManifest_FetchAndCache(t *testing.T) { + t.Parallel() + body, err := os.ReadFile(gatedManifestFixture) + if err != nil { + t.Fatal(err) + } + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch strings.TrimPrefix(r.URL.Path, "/") { + case "inject-manifest.json": + _, _ = w.Write(body) + case "skills/pilotctl/SKILL.md": + _, _ = w.Write([]byte("---\nname: pilotctl\ndescription: d\n---\nbody\n")) + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + + home := t.TempDir() + cfg := Config{Home: home, ManifestURL: srv.URL + "/inject-manifest.json", RepoBaseURL: srv.URL + "/"} + if _, err := Tick(context.Background(), cfg); err != nil { + t.Fatalf("Tick: %v", err) + } + cached, err := os.ReadFile(filepath.Join(cacheDir(home), manifestCacheRel)) + if err != nil { + t.Fatal(err) + } + var m Manifest + if err := json.Unmarshal(cached, &m); err != nil || len(m.GatedTools) != 1 || m.GatedTools[0].Name != "muse" { + t.Fatalf("cached manifest gatedTools = %+v, %v", m.GatedTools, err) + } + if _, err := releasedParse(cached); err != nil { + t.Errorf("released parse rejects the cached manifest: %v", err) + } +} + +func TestCanonicalPath_MissingDescendantOfLink(t *testing.T) { + t.Parallel() + dir := t.TempDir() + real := filepath.Join(dir, "real") + if err := os.MkdirAll(real, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(real, filepath.Join(dir, "link")); err != nil { + t.Fatal(err) + } + // Neither the file nor its parent exists, so resolution has to go + // through the nearest existing ancestor ("link"). + a := canonicalPath(filepath.Join(dir, "link", "a", "b", "SKILL.md")) + b := canonicalPath(filepath.Join(dir, "real", "a", "b", "SKILL.md")) + if a != b { + t.Errorf("canonicalPath through a link = %q, direct = %q", a, b) + } +} diff --git a/zz_gated_test.go b/zz_gated_test.go new file mode 100644 index 0000000..d17238b --- /dev/null +++ b/zz_gated_test.go @@ -0,0 +1,606 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package skillinject_test + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/pilot-protocol/skillinject" +) + +// museEntrypoint is the entrypoint body served by the gated-tool tests: +// canonical pilot-skills frontmatter (folded description, extra keys). +const museEntrypoint = "---\nname: pilotctl\ndescription: >\n Entrypoint for Pilot Protocol.\n Load it for live data.\ntags:\n - pilot-protocol\nlicense: AGPL-3.0\n---\n\n# pilotctl\n\nbody.\n" + +// museWant is museEntrypoint in the Muse format. +const museWant = "---\nname: \"pilotctl\"\ndescription: \"Entrypoint for Pilot Protocol. Load it for live data.\"\n---\n\n# pilotctl\n\nbody.\n" + +// museGated is the pilot-skills "gatedTools" row for Meta Muse. +func museGated() skillinject.ManifestGatedTool { + return skillinject.ManifestGatedTool{ + Name: "muse", + RootDir: "~/workspace/skills", + SkillsDir: "~/workspace/skills", + RequireMarker: "~/.pilot/targets/muse", + SkillFormat: skillinject.SkillFormatMuse, + } +} + +// newGatedRepo serves claudeOnly() plus the given gated rows. +func newGatedRepo(t *testing.T, gated ...skillinject.ManifestGatedTool) *fakeRepo { + t.Helper() + r := newFakeRepo(t) + r.withTools(claudeOnly()) + r.setSkillBody([]byte(museEntrypoint)) + r.manifest.GatedTools = gated + return r +} + +func museSkillPath(home string) string { + return filepath.Join(home, "workspace", "skills", "pilotctl", "SKILL.md") +} + +// markMuse writes the marker the Muse installer leaves after installing +// Muse-format skills into ~/workspace/skills. +func markMuse(t *testing.T, home string) { + t.Helper() + writeMarker(t, home, "# written by muse/install.sh\nskills_dir="+filepath.Join(home, "workspace", "skills")+"\nskill_format=muse\n") +} + +func writeMarker(t *testing.T, home, content string) { + t.Helper() + mustMkdirAll(t, filepath.Join(home, ".pilot", "targets")) + mustWriteFile(t, filepath.Join(home, ".pilot", "targets", "muse"), content, 0o644) +} + +func toolOutcome(rep *skillinject.Report, tool string) (skillinject.Outcome, bool) { + for _, o := range rep.Outcomes { + if o.Tool == tool { + return o, true + } + } + return skillinject.Outcome{}, false +} + +func TestGated_NoMarker_DoesNothing(t *testing.T) { + t.Parallel() + home := t.TempDir() + mustMkdirAll(t, filepath.Join(home, "workspace", "skills")) + mustMkdirAll(t, filepath.Join(home, ".claude")) + r := newGatedRepo(t, museGated()) + + rep, err := skillinject.Tick(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Tick: %v", err) + } + if _, ok := toolOutcome(rep, "muse"); ok { + t.Errorf("muse produced an outcome without its marker: %+v", rep.Outcomes) + } + if !contains(rep.Skipped, "muse") { + t.Errorf("muse should be skipped, Skipped=%v", rep.Skipped) + } + entries, _ := os.ReadDir(filepath.Join(home, "workspace", "skills")) + if len(entries) != 0 { + t.Errorf("~/workspace/skills was written without the marker: %v", entries) + } + // The regular tools are unaffected. + mustExist(t, filepath.Join(home, ".claude", "skills", "pilotctl", "SKILL.md")) +} + +func TestGated_Marker_WritesMuseFormatAndIsIdempotent(t *testing.T) { + t.Parallel() + home := t.TempDir() + mustMkdirAll(t, filepath.Join(home, "workspace", "skills")) + markMuse(t, home) + r := newGatedRepo(t, museGated()) + + rep, err := skillinject.Tick(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Tick: %v", err) + } + o, ok := toolOutcome(rep, "muse") + if !ok || o.Action != skillinject.ActionCreate || o.Kind != skillinject.KindSkill || o.Path != museSkillPath(home) { + t.Fatalf("first tick muse outcome = %+v (found %v)", o, ok) + } + if got := mustRead(t, museSkillPath(home)); got != museWant { + t.Errorf("muse SKILL.md =\n%q\nwant\n%q", got, museWant) + } + fi, err := os.Stat(museSkillPath(home)) + if err != nil || fi.Mode().Perm() != 0o644 { + t.Errorf("muse SKILL.md mode = %v, %v; want 0644", fi.Mode().Perm(), err) + } + + // Later ticks are noops: the hash is taken over the rewritten bytes. + for i := 0; i < 3; i++ { + rep, err = skillinject.Tick(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Tick %d: %v", i, err) + } + if o, _ := toolOutcome(rep, "muse"); o.Action != skillinject.ActionNoop || o.State != skillinject.StateIdentical { + t.Fatalf("tick %d muse outcome = %+v, want identical/noop", i, o) + } + } + // No temp files are left next to the skill. + entries, _ := os.ReadDir(filepath.Dir(museSkillPath(home))) + if len(entries) != 1 { + t.Errorf("skill dir holds %d entries, want only SKILL.md", len(entries)) + } + + // A new upstream SKILL.md is shipped on the next tick. + r.setSkillBody([]byte(strings.Replace(museEntrypoint, "body.", "new body.", 1))) + rep, err = skillinject.Tick(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Tick: %v", err) + } + if o, _ := toolOutcome(rep, "muse"); o.Action != skillinject.ActionRewrite { + t.Errorf("after upstream change muse outcome = %+v, want rewrite", o) + } + if got := mustRead(t, museSkillPath(home)); !strings.HasSuffix(got, "new body.\n") { + t.Errorf("rewrite did not ship the new body: %q", got) + } + + // A local edit is drift and is put back. + mustWriteFile(t, museSkillPath(home), "edited", 0o644) + rep, _ = skillinject.Tick(context.Background(), r.cfg(home)) + if o, _ := toolOutcome(rep, "muse"); o.State != skillinject.StateDrifted || o.Action != skillinject.ActionRewrite { + t.Errorf("after local edit muse outcome = %+v, want drifted/rewrite", o) + } +} + +func TestGated_MarkerWithoutRootDir_Skipped(t *testing.T) { + t.Parallel() + home := t.TempDir() + markMuse(t, home) + r := newGatedRepo(t, museGated()) + + rep, err := skillinject.Tick(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Tick: %v", err) + } + if !contains(rep.Skipped, "muse") { + t.Errorf("muse should be skipped without ~/workspace/skills, Skipped=%v", rep.Skipped) + } + mustNotExist(t, filepath.Join(home, "workspace")) +} + +func TestGated_PlanWritesNothing(t *testing.T) { + t.Parallel() + home := t.TempDir() + mustMkdirAll(t, filepath.Join(home, "workspace", "skills")) + markMuse(t, home) + r := newGatedRepo(t, museGated()) + + rep, err := skillinject.Plan(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Plan: %v", err) + } + if o, _ := toolOutcome(rep, "muse"); o.State != skillinject.StateAbsent || o.Action != skillinject.ActionCreate { + t.Errorf("plan muse outcome = %+v, want absent/create", o) + } + mustNotExist(t, filepath.Join(home, "workspace", "skills", "pilotctl")) +} + +func TestGated_RejectsUnsafeRows(t *testing.T) { + t.Parallel() + cases := map[string]func(*skillinject.ManifestGatedTool){ + "marker outside ~/.pilot": func(g *skillinject.ManifestGatedTool) { g.RequireMarker = "~/workspace/skills" }, + "marker escapes ~/.pilot": func(g *skillinject.ManifestGatedTool) { g.RequireMarker = "~/.pilot/../workspace/x" }, + "marker is ~/.pilot": func(g *skillinject.ManifestGatedTool) { g.RequireMarker = "~/.pilot" }, + "marker absolute elsewhere": func(g *skillinject.ManifestGatedTool) { g.RequireMarker = "/tmp/muse" }, + "no marker": func(g *skillinject.ManifestGatedTool) { g.RequireMarker = "" }, + "rootDir outside home": func(g *skillinject.ManifestGatedTool) { g.RootDir = "/etc"; g.SkillsDir = "/etc" }, + "rootDir is home": func(g *skillinject.ManifestGatedTool) { g.RootDir = "~"; g.SkillsDir = "~/workspace/skills" }, + "skillsDir outside rootDir": func(g *skillinject.ManifestGatedTool) { g.SkillsDir = "~/.claude/skills" }, + "skillsDir escapes rootDir": func(g *skillinject.ManifestGatedTool) { g.SkillsDir = "~/workspace/skills/../../.ssh" }, + "unknown skillFormat": func(g *skillinject.ManifestGatedTool) { g.SkillFormat = "yaml2" }, + "bad name": func(g *skillinject.ManifestGatedTool) { g.Name = "../muse" }, + } + for name, mutate := range cases { + t.Run(name, func(t *testing.T) { + t.Parallel() + home := t.TempDir() + mustMkdirAll(t, filepath.Join(home, "workspace", "skills")) + markMuse(t, home) + g := museGated() + mutate(&g) + r := newGatedRepo(t, g) + + rep, err := skillinject.Tick(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Tick: %v", err) + } + o, ok := toolOutcome(rep, g.Name) + if !ok || o.Action != skillinject.ActionError || o.Err == "" { + t.Fatalf("outcome = %+v (found %v), want an error row", o, ok) + } + mustNotExist(t, museSkillPath(home)) + // The regular tool rows are unaffected by a bad gated row. + if c, ok := toolOutcome(rep, "claude-code"); ok && c.Action == skillinject.ActionError { + t.Errorf("claude-code errored: %+v", c) + } + }) + } +} + +func TestGated_BadEntrypointRefused(t *testing.T) { + t.Parallel() + home := t.TempDir() + mustMkdirAll(t, filepath.Join(home, "workspace", "skills")) + markMuse(t, home) + r := newGatedRepo(t, museGated()) + r.manifest.Entrypoint = "../../.ssh" + r.files["skills/../../.ssh/SKILL.md"] = []byte(museEntrypoint) + + rep, _ := skillinject.Tick(context.Background(), r.cfg(home)) + if rep != nil { + if o, ok := toolOutcome(rep, "muse"); ok && o.Action != skillinject.ActionError { + t.Errorf("muse outcome with a traversal entrypoint = %+v, want error", o) + } + } + mustNotExist(t, filepath.Join(home, ".ssh", "SKILL.md")) +} + +func TestGated_SymlinkedSkillDirRefused(t *testing.T) { + t.Parallel() + for name, inside := range map[string]bool{"outside rootDir": false, "inside rootDir": true} { + t.Run(name, func(t *testing.T) { + t.Parallel() + home := t.TempDir() + skills := filepath.Join(home, "workspace", "skills") + mustMkdirAll(t, skills) + markMuse(t, home) + target := filepath.Join(home, "elsewhere") + if inside { + target = filepath.Join(skills, "other") + } + mustMkdirAll(t, target) + if err := os.Symlink(target, filepath.Join(skills, "pilotctl")); err != nil { + t.Fatal(err) + } + r := newGatedRepo(t, museGated()) + + rep, err := skillinject.Tick(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Tick: %v", err) + } + if o, _ := toolOutcome(rep, "muse"); o.Action != skillinject.ActionError || !strings.Contains(o.Err, "symlink") { + t.Errorf("muse outcome = %+v, want a symlink error", o) + } + mustNotExist(t, filepath.Join(target, "SKILL.md")) + + // Uninstall leaves the link and its target alone as well. + mustWriteFile(t, filepath.Join(target, "SKILL.md"), "theirs", 0o644) + rr, err := skillinject.Uninstall(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Uninstall: %v", err) + } + for _, x := range rr.Removals { + if x.Tool == "muse" && x.Action != skillinject.RemovalError { + t.Errorf("muse removal = %+v, want error", x) + } + } + if got := mustRead(t, filepath.Join(target, "SKILL.md")); got != "theirs" { + t.Errorf("link target was changed: %q", got) + } + }) + } +} + +func TestGated_SymlinkedSkillFileRefused(t *testing.T) { + t.Parallel() + home := t.TempDir() + dir := filepath.Join(home, "workspace", "skills", "pilotctl") + mustMkdirAll(t, dir) + markMuse(t, home) + victim := filepath.Join(home, "victim.md") + mustWriteFile(t, victim, "keep", 0o644) + if err := os.Symlink(victim, filepath.Join(dir, "SKILL.md")); err != nil { + t.Fatal(err) + } + r := newGatedRepo(t, museGated()) + + rep, err := skillinject.Tick(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Tick: %v", err) + } + if o, _ := toolOutcome(rep, "muse"); o.Action != skillinject.ActionError { + t.Errorf("muse outcome = %+v, want error", o) + } + if got := mustRead(t, victim); got != "keep" { + t.Errorf("symlink target overwritten: %q", got) + } + if fi, err := os.Lstat(filepath.Join(dir, "SKILL.md")); err != nil || fi.Mode()&os.ModeSymlink == 0 { + t.Errorf("the link was replaced: %v %v", fi, err) + } +} + +// A link planted at the temp name older code used (.tmp) is not +// followed: gated writes use a random O_EXCL name. +func TestGated_PlantedTempLinkNotFollowed(t *testing.T) { + t.Parallel() + home := t.TempDir() + dir := filepath.Join(home, "workspace", "skills", "pilotctl") + mustMkdirAll(t, dir) + markMuse(t, home) + victim := filepath.Join(home, "victim") + mustWriteFile(t, victim, "keep", 0o644) + if err := os.Symlink(victim, filepath.Join(dir, "SKILL.md.tmp")); err != nil { + t.Fatal(err) + } + r := newGatedRepo(t, museGated()) + + if _, err := skillinject.Tick(context.Background(), r.cfg(home)); err != nil { + t.Fatalf("Tick: %v", err) + } + if got := mustRead(t, victim); got != "keep" { + t.Errorf("planted temp link was followed: %q", got) + } + if got := mustRead(t, museSkillPath(home)); got != museWant { + t.Errorf("muse SKILL.md = %q", got) + } +} + +func TestGated_SameFileAsRegularToolRefused(t *testing.T) { + t.Parallel() + home := t.TempDir() + mustMkdirAll(t, filepath.Join(home, ".claude")) + markMuse(t, home) + // A gated row whose skill file is claude-code's skill copy would + // rewrite it with different bytes on every tick. + g := museGated() + g.RootDir, g.SkillsDir = "~/.claude", "~/.claude/skills" + r := newGatedRepo(t, g) + + rep, err := skillinject.Tick(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Tick: %v", err) + } + if o, _ := toolOutcome(rep, "muse"); o.Action != skillinject.ActionError || !strings.Contains(o.Err, "claude-code") { + t.Errorf("muse outcome = %+v, want an error naming claude-code", o) + } + if got := mustRead(t, filepath.Join(home, ".claude", "skills", "pilotctl", "SKILL.md")); got != museEntrypoint { + t.Errorf("claude-code skill copy was changed: %q", got) + } +} + +func TestGated_FlatNaming(t *testing.T) { + t.Parallel() + home := t.TempDir() + mustMkdirAll(t, filepath.Join(home, "workspace", "skills")) + markMuse(t, home) + g := museGated() + g.SkillNaming = "flat" + r := newGatedRepo(t, g) + + if _, err := skillinject.Tick(context.Background(), r.cfg(home)); err != nil { + t.Fatalf("Tick: %v", err) + } + flat := filepath.Join(home, "workspace", "skills", "pilotctl.md") + if got := mustRead(t, flat); got != museWant { + t.Errorf("flat skill = %q", got) + } + rr, err := skillinject.Uninstall(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Uninstall: %v", err) + } + mustNotExist(t, flat) + mustExist(t, filepath.Join(home, "workspace", "skills")) + _ = rr +} + +func TestGated_Uninstall(t *testing.T) { + t.Parallel() + home := t.TempDir() + mustMkdirAll(t, filepath.Join(home, "workspace", "skills")) + markMuse(t, home) + r := newGatedRepo(t, museGated()) + if _, err := skillinject.Tick(context.Background(), r.cfg(home)); err != nil { + t.Fatalf("Tick: %v", err) + } + mustExist(t, museSkillPath(home)) + + rr, err := skillinject.Uninstall(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Uninstall: %v", err) + } + var got []skillinject.Removal + for _, x := range rr.Removals { + if x.Tool == "muse" { + got = append(got, x) + } + } + if len(got) != 1 || got[0].Action != skillinject.RemovalDeleted || got[0].Path != museSkillPath(home) { + t.Errorf("muse removals = %+v", got) + } + mustNotExist(t, museSkillPath(home)) + mustNotExist(t, filepath.Dir(museSkillPath(home))) // empty entrypoint dir pruned + mustExist(t, filepath.Join(home, "workspace", "skills")) + + // A second uninstall is a noop. + rr, _ = skillinject.Uninstall(context.Background(), r.cfg(home)) + for _, x := range rr.Removals { + if x.Tool == "muse" && x.Action != skillinject.RemovalNoop { + t.Errorf("second uninstall muse removal = %+v, want noop", x) + } + } +} + +func TestGated_UninstallKeepsOtherFilesInSkillDir(t *testing.T) { + t.Parallel() + home := t.TempDir() + mustMkdirAll(t, filepath.Join(home, "workspace", "skills")) + markMuse(t, home) + r := newGatedRepo(t, museGated()) + if _, err := skillinject.Tick(context.Background(), r.cfg(home)); err != nil { + t.Fatalf("Tick: %v", err) + } + extra := filepath.Join(filepath.Dir(museSkillPath(home)), "notes.md") + mustWriteFile(t, extra, "mine", 0o644) + if _, err := skillinject.Uninstall(context.Background(), r.cfg(home)); err != nil { + t.Fatalf("Uninstall: %v", err) + } + mustNotExist(t, museSkillPath(home)) + mustExist(t, extra) +} + +// Without the marker, Uninstall touches nothing under rootDir either: a +// ~/workspace/skills/pilotctl/SKILL.md on a host that never opted in is +// not ours. +func TestGated_UninstallWithoutMarkerLeavesFiles(t *testing.T) { + t.Parallel() + home := t.TempDir() + mustMkdirAll(t, filepath.Dir(museSkillPath(home))) + mustWriteFile(t, museSkillPath(home), "someone else's skill", 0o644) + r := newGatedRepo(t, museGated()) + + rr, err := skillinject.Uninstall(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Uninstall: %v", err) + } + for _, x := range rr.Removals { + if x.Tool == "muse" { + t.Errorf("muse removal without marker: %+v", x) + } + } + if got := mustRead(t, museSkillPath(home)); got != "someone else's skill" { + t.Errorf("file changed: %q", got) + } +} + +// Uninstall works offline from the cached manifest, which keeps the +// gatedTools key. +func TestGated_UninstallOfflineFromCache(t *testing.T) { + t.Parallel() + home := t.TempDir() + mustMkdirAll(t, filepath.Join(home, "workspace", "skills")) + markMuse(t, home) + r := newGatedRepo(t, museGated()) + if _, err := skillinject.Tick(context.Background(), r.cfg(home)); err != nil { + t.Fatalf("Tick: %v", err) + } + cfg := r.cfg(home) + r.srv.Close() + + rr, err := skillinject.Uninstall(context.Background(), cfg) + if err != nil { + t.Fatalf("Uninstall: %v", err) + } + if !rr.ManifestOffline { + t.Error("expected the cached manifest to be used") + } + mustNotExist(t, museSkillPath(home)) +} + +// The disable flow (Uninstall, then disabled mode) leaves the file gone: +// disabled ticks do not reinstall it. +func TestGated_DisabledModeDoesNotReinstall(t *testing.T) { + t.Parallel() + home := t.TempDir() + mustMkdirAll(t, filepath.Join(home, "workspace", "skills")) + markMuse(t, home) + r := newGatedRepo(t, museGated()) + if _, err := skillinject.Tick(context.Background(), r.cfg(home)); err != nil { + t.Fatalf("Tick: %v", err) + } + if _, err := skillinject.Uninstall(context.Background(), r.cfg(home)); err != nil { + t.Fatalf("Uninstall: %v", err) + } + if err := skillinject.SetMode(home, skillinject.ModeDisabled); err != nil { + t.Fatal(err) + } + rep, err := skillinject.Tick(context.Background(), r.cfg(home)) + if err != nil || !rep.Disabled { + t.Fatalf("Tick in disabled mode: %+v, %v", rep, err) + } + mustNotExist(t, museSkillPath(home)) +} + +func TestGated_WriteErrorIsReported(t *testing.T) { + t.Parallel() + if os.Geteuid() == 0 { + t.Skip("root ignores directory permissions") + } + home := t.TempDir() + skills := filepath.Join(home, "workspace", "skills") + mustMkdirAll(t, skills) + markMuse(t, home) + if err := os.Chmod(skills, 0o555); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(skills, 0o755) }) + r := newGatedRepo(t, museGated()) + + rep, err := skillinject.Tick(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Tick: %v", err) + } + if o, _ := toolOutcome(rep, "muse"); o.Action != skillinject.ActionError || o.Err == "" { + t.Errorf("muse outcome = %+v, want a write error", o) + } +} + +func TestGated_UninstallEdgeCases(t *testing.T) { + t.Parallel() + t.Run("not a regular file", func(t *testing.T) { + t.Parallel() + home := t.TempDir() + mustMkdirAll(t, museSkillPath(home)) // a directory named SKILL.md + markMuse(t, home) + r := newGatedRepo(t, museGated()) + rr, err := skillinject.Uninstall(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Uninstall: %v", err) + } + for _, x := range rr.Removals { + if x.Tool == "muse" && x.Action != skillinject.RemovalError { + t.Errorf("muse removal = %+v, want error", x) + } + } + mustExist(t, museSkillPath(home)) + }) + t.Run("rootDir missing", func(t *testing.T) { + t.Parallel() + home := t.TempDir() + markMuse(t, home) + r := newGatedRepo(t, museGated()) + rr, err := skillinject.Uninstall(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Uninstall: %v", err) + } + for _, x := range rr.Removals { + if x.Tool == "muse" && x.Action != skillinject.RemovalNoop { + t.Errorf("muse removal = %+v, want noop", x) + } + } + }) + t.Run("invalid rows", func(t *testing.T) { + t.Parallel() + home := t.TempDir() + mustMkdirAll(t, filepath.Join(home, "workspace", "skills")) + markMuse(t, home) + badMarker, badRoot := museGated(), museGated() + badMarker.Name, badMarker.RequireMarker = "bad-marker", "~/workspace/m" + badRoot.Name, badRoot.RootDir = "bad-root", "/etc" + r := newGatedRepo(t, badMarker, badRoot) + rr, err := skillinject.Uninstall(context.Background(), r.cfg(home)) + if err != nil { + t.Fatalf("Uninstall: %v", err) + } + n := 0 + for _, x := range rr.Removals { + if x.Tool == "bad-marker" || x.Tool == "bad-root" { + n++ + if x.Action != skillinject.RemovalError { + t.Errorf("%s removal = %+v, want error", x.Tool, x) + } + } + } + if n != 2 { + t.Errorf("got %d rows for the invalid gated tools, want 2", n) + } + }) +} diff --git a/zz_proxy_refresh_internal_test.go b/zz_proxy_refresh_internal_test.go new file mode 100644 index 0000000..b0798c6 --- /dev/null +++ b/zz_proxy_refresh_internal_test.go @@ -0,0 +1,590 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package skillinject + +import ( + "bufio" + "context" + "crypto/tls" + "crypto/x509" + "encoding/base64" + "encoding/json" + "fmt" + "io" + "net" + "net/http" + "net/http/httptest" + "net/url" + "os" + "path/filepath" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/pilot-protocol/common/netproxy" +) + +// rotatingProxy is a Muse-like egress proxy: CONNECT to :443 only, Basic +// auth, and a password that rotate() replaces. Every CONNECT, whatever +// host it names, is tunnelled to target, so https://example.com/ reaches +// the test's TLS server (whose certificate covers example.com) while the +// client still has to go through the proxy (it never proxies loopback). +type rotatingProxy struct { + t *testing.T + ln net.Listener + target string + gen atomic.Int64 + // credFile holds the current proxy URL, as a fresh Muse shell sees it. + credFile string + + // garble: reject credentials with a status line net/http cannot parse + // ("HTTP/1.1 4O7 ..."), as Meta Muse's proxy does, instead of a 407. + garble atomic.Bool + + mu sync.Mutex + log []string // "ALLOW gen=N" / "DENY 407 gen=N" +} + +func newRotatingProxy(t *testing.T, target string) *rotatingProxy { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + p := &rotatingProxy{t: t, ln: ln, target: target, credFile: filepath.Join(t.TempDir(), "https_proxy")} + p.gen.Store(1) + p.writeCreds() + go p.serve() + t.Cleanup(func() { _ = ln.Close() }) + return p +} + +func (p *rotatingProxy) password(gen int64) string { return fmt.Sprintf("gen%d-s3cret", gen) } + +// url is the proxy URL with generation gen's credentials. +func (p *rotatingProxy) url(gen int64) string { + return "http://muse:" + p.password(gen) + "@" + p.ln.Addr().String() +} + +func (p *rotatingProxy) writeCreds() { + if err := os.WriteFile(p.credFile, []byte(p.url(p.gen.Load())), 0o600); err != nil { + p.t.Error(err) + } +} + +// rotate replaces the credentials; a fresh shell (credFile) sees the new +// ones, the process environment keeps the old. +func (p *rotatingProxy) rotate() { + p.gen.Add(1) + p.writeCreds() +} + +// refreshCommand prints the current proxy URL, as +// bash -c 'printf %s "$https_proxy"' does in Muse. +func (p *rotatingProxy) refreshCommand() string { return "cat '" + p.credFile + "'" } + +func (p *rotatingProxy) record(s string) { + p.mu.Lock() + p.log = append(p.log, s) + p.mu.Unlock() +} + +func (p *rotatingProxy) events() []string { + p.mu.Lock() + defer p.mu.Unlock() + return append([]string(nil), p.log...) +} + +func (p *rotatingProxy) count(prefix string) int { + n := 0 + for _, e := range p.events() { + if strings.HasPrefix(e, prefix) { + n++ + } + } + return n +} + +func (p *rotatingProxy) serve() { + for { + c, err := p.ln.Accept() + if err != nil { + return + } + go p.handle(c) + } +} + +func (p *rotatingProxy) handle(c net.Conn) { + defer c.Close() + br := bufio.NewReader(c) + req, err := http.ReadRequest(br) + if err != nil { + return + } + gen := p.gen.Load() + if req.Method != http.MethodConnect || !strings.HasSuffix(req.Host, ":443") { + p.record("DENY 403 " + req.Method + " " + req.Host) + _, _ = io.WriteString(c, "HTTP/1.1 403 Forbidden\r\n\r\n") + return + } + want := "Basic " + base64.StdEncoding.EncodeToString([]byte("muse:"+p.password(gen))) + if req.Header.Get("Proxy-Authorization") != want { + p.record(fmt.Sprintf("DENY 407 gen=%d", gen)) + if p.garble.Load() { + _, _ = io.WriteString(c, "HTTP/1.1 4O7 Proxy Authentication Required\r\n\r\n") + return + } + _, _ = io.WriteString(c, "HTTP/1.1 407 Proxy Authentication Required\r\nProxy-Authenticate: Basic realm=\"muse\"\r\n\r\n") + return + } + up, err := net.Dial("tcp", p.target) + if err != nil { + _, _ = io.WriteString(c, "HTTP/1.1 502 Bad Gateway\r\n\r\n") + return + } + defer up.Close() + p.record(fmt.Sprintf("ALLOW gen=%d", gen)) + _, _ = io.WriteString(c, "HTTP/1.1 200 Connection Established\r\n\r\n") + go func() { _, _ = io.Copy(up, br); _ = up.Close() }() + _, _ = io.Copy(c, up) +} + +// proxyRepo is a TLS pilot-skills stand-in: a manifest with the claude-code +// tool and the gated muse row, and the entrypoint SKILL.md. onManifest runs +// before the manifest is served. +type proxyRepo struct { + srv *httptest.Server + onManifest func(w http.ResponseWriter) +} + +func newProxyRepo(t *testing.T) *proxyRepo { + t.Helper() + r := &proxyRepo{} + manifest, err := json.Marshal(Manifest{ + Version: 1, + Entrypoint: "pilotctl", + Tools: []ManifestTool{{Name: "claude-code", RootDir: "~/.claude", SkillsDir: "~/.claude/skills"}}, + GatedTools: []ManifestGatedTool{{ + Name: "muse", RootDir: "~/workspace/skills", SkillsDir: "~/workspace/skills", + RequireMarker: "~/.pilot/targets/muse", SkillFormat: SkillFormatMuse, + }}, + }) + if err != nil { + t.Fatal(err) + } + r.srv = httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + switch strings.TrimPrefix(req.URL.Path, "/") { + case "inject-manifest.json": + if r.onManifest != nil { + r.onManifest(w) + } + _, _ = w.Write(manifest) + case "skills/pilotctl/SKILL.md": + _, _ = w.Write([]byte("---\nname: pilotctl\ndescription: Pilot.\n---\nbody\n")) + default: + http.NotFound(w, req) + } + })) + t.Cleanup(r.srv.Close) + return r +} + +// trustRepo makes the refreshing client trust the repo's certificate. +func trustRepo(t *testing.T, r *proxyRepo) { + t.Helper() + prev := proxyBaseTransport + pool := x509.NewCertPool() + pool.AddCert(r.srv.Certificate()) + proxyBaseTransport = func() *http.Transport { + tr := prev() + if tr == nil { + tr = &http.Transport{} + } + tr.TLSClientConfig = tr.TLSClientConfig.Clone() + if tr.TLSClientConfig == nil { + tr.TLSClientConfig = new(tls.Config) + } + tr.TLSClientConfig.RootCAs = pool + return tr + } + t.Cleanup(func() { proxyBaseTransport = prev }) +} + +// museProxyHome is a Muse VM home after the installer ran: ~/workspace/skills, +// the marker for it, and ~/.claude. +func museProxyHome(t *testing.T) string { + t.Helper() + home := t.TempDir() + for _, d := range []string{"workspace/skills", ".pilot/targets", ".claude"} { + if err := os.MkdirAll(filepath.Join(home, d), 0o755); err != nil { + t.Fatal(err) + } + } + decl := "skills_dir=" + filepath.Join(home, "workspace", "skills") + "\nskill_format=muse\n" + if err := os.WriteFile(filepath.Join(home, ".pilot", "targets", "muse"), []byte(decl), 0o644); err != nil { + t.Fatal(err) + } + return home +} + +// launchEnv is the proxy environment the daemon was started with: the +// credentials that were current then, and nothing that exempts the repo. +func launchEnv(t *testing.T, proxyURL string) { + t.Helper() + for _, k := range []string{"HTTPS_PROXY", "https_proxy", "ALL_PROXY", "all_proxy", "HTTP_PROXY", "http_proxy", "NO_PROXY", "no_proxy", EnvProxy, EnvProxyCommand} { + t.Setenv(k, "") + } + t.Setenv("HTTPS_PROXY", proxyURL) +} + +func repoConfig(home string) Config { + return Config{ + Home: home, + ManifestURL: "https://example.com/inject-manifest.json", + RepoBaseURL: "https://example.com/", + } +} + +// noSecrets fails when err's text holds any generation's password. +func noSecrets(t *testing.T, p *rotatingProxy, err error) { + t.Helper() + if err == nil { + return + } + for g := int64(1); g <= p.gen.Load()+1; g++ { + if strings.Contains(err.Error(), p.password(g)) { + t.Errorf("error leaks proxy credentials: %v", err) + } + } +} + +// The review repro: the daemon's environment keeps the launch-time +// credentials, the proxy rotates them between ticks, and every tick after +// the first used to fail with 407. With the refresh command every tick +// goes out with the current credentials and keeps the Muse copy current. +func TestProxyRefresh_TicksSurviveRotation(t *testing.T) { + repo := newProxyRepo(t) + p := newRotatingProxy(t, repo.srv.Listener.Addr().String()) + trustRepo(t, repo) + launchEnv(t, p.url(1)) + home := museProxyHome(t) + cfg := repoConfig(home) + cfg.ProxyCommand = p.refreshCommand() + skill := filepath.Join(home, "workspace", "skills", "pilotctl", "SKILL.md") + + for tick := 1; tick <= 4; tick++ { + if tick > 1 { + p.rotate() + // A local edit the next tick has to repair. + if err := os.WriteFile(skill, []byte("edited"), 0o644); err != nil { + t.Fatal(err) + } + } + rep, err := Tick(context.Background(), cfg) + noSecrets(t, p, err) + if err != nil { + t.Fatalf("tick %d (credentials gen %d): %v\nproxy: %v", tick, p.gen.Load(), err, p.events()) + } + var muse *Outcome + for i := range rep.Outcomes { + if rep.Outcomes[i].Tool == "muse" { + muse = &rep.Outcomes[i] + } + } + if muse == nil || muse.Action == ActionError || muse.Action == ActionNoop { + t.Fatalf("tick %d muse outcome = %+v, want create/rewrite", tick, muse) + } + if b, _ := os.ReadFile(skill); !strings.HasPrefix(string(b), "---\nname: \"pilotctl\"") { + t.Fatalf("tick %d left %q", tick, b) + } + } + if n := p.count("ALLOW gen=4"); n == 0 { + t.Errorf("no CONNECT with the gen 4 credentials: %v", p.events()) + } +} + +// Rotation in the middle of a tick: the manifest response closes its +// connection and the credentials rotate while it is served, so the +// SKILL.md fetch needs a new CONNECT, meets 407, and RefreshingTransport +// re-runs the command and retries it once. +func TestProxyRefresh_RotationMidTickIsRetried(t *testing.T) { + repo := newProxyRepo(t) + p := newRotatingProxy(t, repo.srv.Listener.Addr().String()) + var once sync.Once + repo.onManifest = func(w http.ResponseWriter) { + w.Header().Set("Connection", "close") + once.Do(p.rotate) + } + trustRepo(t, repo) + launchEnv(t, p.url(1)) + home := museProxyHome(t) + cfg := repoConfig(home) + cfg.ProxyCommand = p.refreshCommand() + + if _, err := Tick(context.Background(), cfg); err != nil { + t.Fatalf("Tick: %v\nproxy: %v", err, p.events()) + } + ev := strings.Join(p.events(), ", ") + if !strings.Contains(ev, "ALLOW gen=1, DENY 407 gen=2, ALLOW gen=2") { + t.Errorf("proxy events = %s, want a gen 1 tunnel, a 407, then a retry with gen 2", ev) + } +} + +// The command comes from $PILOT_PROXY_CMD or ~/.pilot/config.json +// "proxy_cmd" when Config.ProxyCommand is empty: the daemon registers +// skillinject with a zero Config. +func TestProxyRefresh_CommandFromDaemonSettings(t *testing.T) { + for _, src := range []string{"env", "config.json"} { + t.Run(src, func(t *testing.T) { + repo := newProxyRepo(t) + p := newRotatingProxy(t, repo.srv.Listener.Addr().String()) + trustRepo(t, repo) + launchEnv(t, p.url(1)) + home := museProxyHome(t) + if src == "env" { + t.Setenv(EnvProxyCommand, p.refreshCommand()) + } else { + conf, _ := json.Marshal(map[string]any{"transport": "compat", "proxy_cmd": p.refreshCommand()}) + if err := os.WriteFile(filepath.Join(home, ".pilot", "config.json"), conf, 0o600); err != nil { + t.Fatal(err) + } + } + p.rotate() + p.rotate() + if _, err := Tick(context.Background(), repoConfig(home)); err != nil { + t.Fatalf("Tick after two rotations: %v\nproxy: %v", err, p.events()) + } + if n := p.count("DENY"); n != 0 { + t.Errorf("stale credentials were sent: %v", p.events()) + } + }) + } +} + +// A refresh command that fails keeps the last good settings (here the +// environment's), and nothing it prints ends up in an error. +func TestProxyRefresh_FailingCommandFallsBackToEnvironment(t *testing.T) { + repo := newProxyRepo(t) + p := newRotatingProxy(t, repo.srv.Listener.Addr().String()) + trustRepo(t, repo) + launchEnv(t, p.url(1)) + home := museProxyHome(t) + + cfg := repoConfig(home) + cfg.ProxyCommand = "exit 3" + if _, err := Tick(context.Background(), cfg); err != nil { + t.Fatalf("Tick with a failing command and current env credentials: %v", err) + } + + // Stale environment and a command printing garbage that holds a + // secret: the tick fails, and neither secret is in the error. + p.rotate() + cfg.ProxyCommand = "printf %s 'not-a-url " + p.password(2) + "'" + _, err := Tick(context.Background(), cfg) + if err == nil { + t.Fatal("Tick succeeded with stale credentials and an unusable command") + } + noSecrets(t, p, err) + if strings.Contains(err.Error(), "not-a-url") { + t.Errorf("error quotes the command's output: %v", err) + } +} + +// With no command, a 407 the host's own transport reports as a +// *netproxy.ConnectError is retried once: that is how pilot-daemon's +// http.DefaultTransport answers a 407 after refreshing its credentials. +func TestFetch_RetriesProxyAuthRejectionOnce(t *testing.T) { + for _, refreshes := range []bool{true, false} { + t.Run(fmt.Sprintf("host refreshes=%v", refreshes), func(t *testing.T) { + repo := newProxyRepo(t) + p := newRotatingProxy(t, repo.srv.Listener.Addr().String()) + launchEnv(t, "") + home := museProxyHome(t) + var cur atomic.Pointer[url.URL] + u, _ := url.Parse(p.url(1)) + cur.Store(u) + p.rotate() + + pool := x509.NewCertPool() + pool.AddCert(repo.srv.Certificate()) + tr := &http.Transport{ + Proxy: func(*http.Request) (*url.URL, error) { return cur.Load(), nil }, + TLSClientConfig: &tls.Config{RootCAs: pool}, + OnProxyConnectResponse: func(_ context.Context, _ *url.URL, req *http.Request, res *http.Response) error { + if res.StatusCode == http.StatusOK { + return nil + } + if refreshes { + fresh, _ := url.Parse(p.url(p.gen.Load())) + cur.Store(fresh) + } + return &netproxy.ConnectError{Target: req.Host, StatusCode: res.StatusCode} + }, + } + cfg := repoConfig(home) + cfg.HTTPClient = &http.Client{Transport: tr, Timeout: 10 * time.Second} + + _, err := Tick(context.Background(), cfg) + if refreshes { + if err != nil { + t.Fatalf("Tick: %v\nproxy: %v", err, p.events()) + } + if got := p.count("DENY 407"); got != 1 { + t.Errorf("407s = %d, want 1: %v", got, p.events()) + } + return + } + if err == nil || !proxyAuthRejected(err) { + t.Fatalf("Tick = %v, want the 407", err) + } + if got := p.count("DENY 407"); got != 2 { + t.Errorf("407s = %d, want 2 (one retry, no loop): %v", got, p.events()) + } + }) + } +} + +// si42-proxycmd-flag-only-unparseable-rejection-no-retry: with a transport +// setup does not own (pilot-daemon's http.DefaultTransport, when the daemon +// got -proxy-cmd only as a flag), a rejection the proxy garbles ("HTTP/1.1 +// 4O7") never reaches the transport's CONNECT hook, so nothing refreshes on +// it. get waits a moment and retries once: when the transport's resolver +// re-read the credentials in the background meanwhile (as netproxy's does +// once its interval has passed, from the lookup that failed), the retry +// succeeds; when nothing refreshed, it fails after exactly one retry. +func TestFetch_RetriesUnreadableProxyReplyOnce(t *testing.T) { + old := unreadableReplyRetryDelay + unreadableReplyRetryDelay = 100 * time.Millisecond + t.Cleanup(func() { unreadableReplyRetryDelay = old }) + for _, background := range []bool{true, false} { + t.Run(fmt.Sprintf("background_refresh=%v", background), func(t *testing.T) { + repo := newProxyRepo(t) + p := newRotatingProxy(t, repo.srv.Listener.Addr().String()) + p.garble.Store(true) + launchEnv(t, "") + home := museProxyHome(t) + var cur atomic.Pointer[url.URL] + u, _ := url.Parse(p.url(1)) + cur.Store(u) + p.rotate() + + var refreshing atomic.Bool + pool := x509.NewCertPool() + pool.AddCert(repo.srv.Certificate()) + tr := &http.Transport{ + Proxy: func(*http.Request) (*url.URL, error) { + // A resolver whose refresh interval has passed starts + // a background re-read on a lookup and answers with + // the settings in hand. + if background && refreshing.CompareAndSwap(false, true) { + go func() { + time.Sleep(20 * time.Millisecond) + fresh, _ := url.Parse(p.url(p.gen.Load())) + cur.Store(fresh) + }() + } + return cur.Load(), nil + }, + TLSClientConfig: &tls.Config{RootCAs: pool}, + // The hook of pilot-daemon's transport; a garbled answer + // never reaches it. + OnProxyConnectResponse: func(_ context.Context, _ *url.URL, req *http.Request, res *http.Response) error { + if res.StatusCode == http.StatusOK { + return nil + } + return &netproxy.ConnectError{Target: req.Host, StatusCode: res.StatusCode} + }, + } + cfg := repoConfig(home) + cfg.HTTPClient = &http.Client{Transport: tr, Timeout: 10 * time.Second} + + _, err := Tick(context.Background(), cfg) + if background { + if err != nil { + t.Fatalf("Tick: %v\nproxy: %v", err, p.events()) + } + if got := p.count("DENY 407"); got != 1 { + t.Errorf("rejections = %d, want 1: %v", got, p.events()) + } + return + } + if err == nil || !unreadableProxyReply(err) { + t.Fatalf("Tick = %v, want the garbled rejection", err) + } + if got := p.count("DENY 407"); got != 2 { + t.Errorf("rejections = %d, want 2 (one retry, no loop): %v", got, p.events()) + } + }) + } +} + +func TestProxyCommand_Precedence(t *testing.T) { + type env map[string]string + cases := []struct { + name string + env env + config map[string]any + cfg string + want string + }{ + {name: "nothing set", want: ""}, + {name: "config proxy_cmd", config: map[string]any{"proxy_cmd": "cfg-cmd"}, want: "cfg-cmd"}, + {name: "env beats config", env: env{EnvProxyCommand: "env-cmd"}, config: map[string]any{"proxy_cmd": "cfg-cmd"}, want: "env-cmd"}, + {name: "Config beats env", env: env{EnvProxyCommand: "env-cmd"}, cfg: "field-cmd", want: "field-cmd"}, + {name: "blank env falls through", env: env{EnvProxyCommand: " "}, config: map[string]any{"proxy_cmd": "cfg-cmd"}, want: "cfg-cmd"}, + {name: "PILOT_PROXY=off", env: env{EnvProxy: "off", EnvProxyCommand: "env-cmd"}, want: ""}, + {name: "PILOT_PROXY=Direct", env: env{EnvProxy: "Direct"}, config: map[string]any{"proxy_cmd": "cfg-cmd"}, want: ""}, + {name: "config proxy none", config: map[string]any{"proxy": "none", "proxy_cmd": "cfg-cmd"}, want: ""}, + {name: "PILOT_PROXY URL beats config off", env: env{EnvProxy: "http://relay:3128"}, config: map[string]any{"proxy": "off", "proxy_cmd": "cfg-cmd"}, want: "cfg-cmd"}, + {name: "Config beats off", env: env{EnvProxy: "off"}, cfg: "field-cmd", want: "field-cmd"}, + {name: "non-string proxy_cmd", config: map[string]any{"proxy_cmd": []string{"x"}}, want: ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Setenv(EnvProxy, "") + t.Setenv(EnvProxyCommand, "") + for k, v := range tc.env { + t.Setenv(k, v) + } + home := t.TempDir() + if tc.config != nil { + b, _ := json.Marshal(tc.config) + if err := os.MkdirAll(filepath.Join(home, ".pilot"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(home, ".pilot", "config.json"), b, 0o600); err != nil { + t.Fatal(err) + } + } + if got := proxyCommand(Config{Home: home, ProxyCommand: tc.cfg}); got != tc.want { + t.Errorf("proxyCommand = %q, want %q", got, tc.want) + } + }) + } +} + +// Without a command the default client is the one it always was: no +// transport of its own, so it follows http.DefaultTransport, and a +// Config.HTTPClient is used as given. +func TestDefaultClient_WithoutCommandIsPlain(t *testing.T) { + t.Setenv(EnvProxy, "") + t.Setenv(EnvProxyCommand, "") + f := newFetcher(Config{Home: t.TempDir()}) + if f.ownsTransport || f.httpClient.Transport != nil || f.httpClient.Timeout != fetchTimeout { + t.Errorf("default client = %+v (owns %v), want a plain client on DefaultTransport", f.httpClient, f.ownsTransport) + } + given := &http.Client{} + t.Setenv(EnvProxyCommand, "true") + if f := newFetcher(Config{HTTPClient: given}); f.httpClient != given || f.ownsTransport { + t.Error("Config.HTTPClient was not used as given") + } + // An HTTPS_PROXY netproxy cannot use: the plain client, which reports + // the bad setting on every fetch as it always did. + t.Setenv("HTTPS_PROXY", "socks5://127.0.0.1:1080") + if f := newFetcher(Config{Home: t.TempDir()}); f.ownsTransport || f.httpClient.Transport != nil { + t.Errorf("unusable HTTPS_PROXY: client = %+v (owns %v), want the plain one", f.httpClient, f.ownsTransport) + } +} diff --git a/zz_skillformat_internal_test.go b/zz_skillformat_internal_test.go new file mode 100644 index 0000000..336db36 --- /dev/null +++ b/zz_skillformat_internal_test.go @@ -0,0 +1,189 @@ +// SPDX-License-Identifier: AGPL-3.0-or-later + +package skillinject + +import ( + "bytes" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// museCases are SKILL.md inputs covering every branch of the Muse +// frontmatter rewrite. TestMuseSkillMD_MatchesInstaller runs each through +// the installer's shell function as well. +var museCases = map[string]string{ + "folded": "---\nname: pilotctl\ndescription: >\n Entrypoint for Pilot Protocol, the overlay\n network. Load it for `pilotctl`.\ntags:\n - pilot-protocol\nlicense: AGPL-3.0\nallowed-tools:\n - Bash\n---\n\n# pilotctl\n\nbody line\n", + "plain-comment": "---\nname: x\ndescription: Plain text # a comment\n---\nbody\n", + "double-quoted": "---\nname: x\ndescription: \"Say \\\"hi\\\" \\\\ then\\nnew\\tline # not a comment\"\n---\nbody\n", + "single-quoted": "---\nname: x\ndescription: 'It''s quoted # kept'\n---\nbody\n", + "plain-multiline": "---\nname: x\ndescription: first part\n second part # dropped\n\n third\nlicense: MIT\n---\nbody\n", + "literal-block": "---\nname: x\ndescription: |- # comment\n line one\n\n line two # kept\nlicense: MIT\n---\nbody\n", + "empty-value-indent": "---\nname: x\ndescription:\n continued value\n---\nbody\n", + "no-description": "---\nname: x\nlicense: MIT\n---\nbody\n", + "crlf": "---\r\nname: x\r\ndescription: >\r\n crlf folded\r\n text\r\n---\r\nbody\r\nmore\r\n", + "no-frontmatter": "# Title\n\ndescription: not frontmatter\n", + "unclosed": "---\nname: x\ndescription: never closed\n", + "fence-trailing-ws": "--- \nname: x\ndescription: y\n---\nbody\n", + "empty": "", + "only-fences": "---\n---\n", + "fence-at-eof": "---\nname: x\ndescription: at eof\n---", + "quotes-in-plain": "---\nname: x\ndescription: say \"yes\" to C:\\path\n---\nbody\n", + "quoted-then-cmt": "---\nname: x\ndescription: \"quoted\" # trailing\n---\nbody\n", + "control-chars": "---\nname: x\ndescription: a\x01b\x0bc\td e\n---\nbody\n", + "key-after": "---\nname: x\ntags:\n - a\ndescription: middle\nlicense: MIT\ndescription: second is ignored\n---\nbody\n", + "long-spaces": "---\nname: x\ndescription: " + strings.Repeat("word ", 300) + "\n---\nbody\n", + "long-no-space": "---\nname: x\ndescription: " + strings.Repeat("x", 1500) + "\n---\nbody\n", + "long-exact": "---\nname: x\ndescription: " + strings.Repeat("y", 1024) + "\n---\nbody\n", + "long-utf8": "---\nname: x\ndescription: " + strings.Repeat("é ab ", 250) + "\n---\nbody\n", + "body-keeps-fences": "---\nname: x\ndescription: d\n---\n---\nnot: frontmatter\n---\n", +} + +func TestMuseSkillMD_Golden(t *testing.T) { + t.Parallel() + cases := []struct{ name, in, want string }{ + {"folded", museCases["folded"], + "---\nname: \"pilotctl\"\ndescription: \"Entrypoint for Pilot Protocol, the overlay network. Load it for `pilotctl`.\"\n---\n\n# pilotctl\n\nbody line\n"}, + {"plain-comment", museCases["plain-comment"], + "---\nname: \"pilotctl\"\ndescription: \"Plain text\"\n---\nbody\n"}, + {"double-quoted", museCases["double-quoted"], + "---\nname: \"pilotctl\"\ndescription: \"Say \\\"hi\\\" \\\\ then new line # not a comment\"\n---\nbody\n"}, + {"single-quoted", museCases["single-quoted"], + "---\nname: \"pilotctl\"\ndescription: \"It's quoted # kept\"\n---\nbody\n"}, + {"plain-multiline", museCases["plain-multiline"], + "---\nname: \"pilotctl\"\ndescription: \"first part second part third\"\n---\nbody\n"}, + {"no-description", museCases["no-description"], + "---\nname: \"pilotctl\"\ndescription: \"Pilot Protocol skill pilotctl\"\n---\nbody\n"}, + {"crlf", museCases["crlf"], + "---\nname: \"pilotctl\"\ndescription: \"crlf folded text\"\n---\nbody\r\nmore\r\n"}, + {"no-frontmatter", museCases["no-frontmatter"], museCases["no-frontmatter"]}, + {"unclosed", museCases["unclosed"], museCases["unclosed"]}, + {"empty", "", ""}, + {"fence-at-eof", museCases["fence-at-eof"], + "---\nname: \"pilotctl\"\ndescription: \"at eof\"\n---\n"}, + {"quotes-in-plain", museCases["quotes-in-plain"], + "---\nname: \"pilotctl\"\ndescription: \"say \\\"yes\\\" to C:\\\\path\"\n---\nbody\n"}, + {"control-chars", museCases["control-chars"], + "---\nname: \"pilotctl\"\ndescription: \"abc d e\"\n---\nbody\n"}, + {"body-keeps-fences", museCases["body-keeps-fences"], + "---\nname: \"pilotctl\"\ndescription: \"d\"\n---\n---\nnot: frontmatter\n---\n"}, + } + for _, c := range cases { + if got := string(museSkillMD([]byte(c.in), "pilotctl")); got != c.want { + t.Errorf("%s:\n got %q\nwant %q", c.name, got, c.want) + } + } +} + +func TestMuseSkillMD_LongDescriptionIsCapped(t *testing.T) { + t.Parallel() + for _, name := range []string{"long-spaces", "long-no-space", "long-utf8"} { + out := string(museSkillMD([]byte(museCases[name]), "x")) + line := strings.Split(out, "\n")[2] + desc := strings.TrimSuffix(strings.TrimPrefix(line, `description: "`), `"`) + if len(desc) > museDescriptionMax { + t.Errorf("%s: description is %d bytes, want <= %d", name, len(desc), museDescriptionMax) + } + if !strings.HasSuffix(desc, "...") { + t.Errorf("%s: capped description should end in ...: %q", name, desc[len(desc)-10:]) + } + } + // Exactly at the cap is kept whole. + out := string(museSkillMD([]byte(museCases["long-exact"]), "x")) + if !strings.Contains(out, strings.Repeat("y", 1024)+"\"") { + t.Error("a 1024-byte description was cut") + } +} + +func TestFormatSkill(t *testing.T) { + t.Parallel() + in := []byte(museCases["folded"]) + if got, err := formatSkill(in, "", "pilot-ctl"); err != nil || !bytes.Equal(got, in) { + t.Errorf("empty format should copy verbatim, got %q, %v", got, err) + } + got, err := formatSkill(in, SkillFormatMuse, "pilot-ctl") + if err != nil || !strings.HasPrefix(string(got), "---\nname: \"pilot_ctl\"\n") { + t.Errorf("muse format should name the skill pilot_ctl, got %q, %v", got, err) + } + if _, err := formatSkill(in, "other", "pilotctl"); err == nil { + t.Error("unknown format should be an error") + } +} + +// TestMuseSkillMD_MatchesInstaller runs the Muse installer's own +// muse_frontmatter (testdata/muse_frontmatter.sh) and museSkillMD on the +// same inputs and requires identical bytes. Set PILOT_SKILLS_CORPUS to a +// pilot-skills checkout to also compare every skills/*/SKILL.md in it. +func TestMuseSkillMD_MatchesInstaller(t *testing.T) { + bash, err := exec.LookPath("bash") + if err != nil { + t.Skip("bash not found") + } + for _, tool := range []string{"awk", "tr", "cut", "wc", "tail", "mv"} { + if _, err := exec.LookPath(tool); err != nil { + t.Skipf("%s not found", tool) + } + } + fn, err := filepath.Abs(filepath.Join("testdata", "muse_frontmatter.sh")) + if err != nil { + t.Fatal(err) + } + + inputs := map[string][]byte{} + for name, body := range museCases { + inputs[name] = []byte(body) + } + if dir := os.Getenv("PILOT_SKILLS_CORPUS"); dir != "" { + files, _ := filepath.Glob(filepath.Join(dir, "skills", "*", "SKILL.md")) + if len(files) == 0 { + t.Fatalf("PILOT_SKILLS_CORPUS=%s has no skills/*/SKILL.md", dir) + } + for _, f := range files { + b, err := os.ReadFile(f) + if err != nil { + t.Fatal(err) + } + inputs["corpus:"+filepath.Base(filepath.Dir(f))] = b + } + t.Logf("comparing %d corpus files from %s", len(files), dir) + } + + tmp := t.TempDir() + i := 0 + for name, body := range inputs { + i++ + skill := "pilot-ctl" + p := filepath.Join(tmp, fmt.Sprintf("c%04d", i), skill, "SKILL.md") + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, body, 0o644); err != nil { + t.Fatal(err) + } + // Same shell options and call as the installer: + // muse_frontmatter "$dest/$skill/SKILL.md" "${skill//-/_}". + cmd := exec.Command(bash, "-c", `set -euo pipefail; . "$0"; muse_frontmatter "$1" "$2"`, fn, p, "pilot_ctl") + if out, err := cmd.CombinedOutput(); err != nil { + t.Errorf("%s: installer function failed: %v\n%s", name, err, out) + continue + } + want, err := os.ReadFile(p) + if err != nil { + t.Fatal(err) + } + if got := museSkillMD(body, "pilot_ctl"); !bytes.Equal(got, want) { + t.Errorf("%s: museSkillMD differs from the installer\n got %q\nwant %q", name, head(got), head(want)) + } + } +} + +// head returns the frontmatter part of a SKILL.md for failure messages. +func head(b []byte) string { + if len(b) > 1400 { + return string(b[:1400]) + "..." + } + return string(b) +}