From 4a5684963de6bde32fa9f7ab15a526e0d031a82f Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Thu, 24 Sep 2026 04:54:49 +0300 Subject: [PATCH 1/3] Add marker-gated skill targets (gatedTools) and the Meta Muse format Meta Muse loads skills from ~/workspace/skills. That directory is too generic to detect by existence, and the entry cannot go in "tools": every released skillinject (v0.2.2 through v0.2.4, and the 902f745 build pilot v1.13.x ships) would then write the unconverted SKILL.md into ~/workspace/skills/pilotctl/ on any host that has the directory, with no gate. New optional manifest key "gatedTools". Released builds decode the manifest with plain json.Unmarshal into a struct without it, so they ignore these rows. A gated tool: - is active only while its requireMarker file exists, and that file must be inside ~/.pilot (the Muse installer creates ~/.pilot/targets/muse). Without the marker nothing under rootDir is read, written or removed, by a tick or by Uninstall; - installs only the entrypoint skill copy (no heartbeat, no plugin); - requires rootDir inside home, skillsDir inside rootDir. All file operations go through os.Root on rootDir, a symlink at the skill file or any directory between rootDir and it is an error row, and writes go through an O_EXCL temp file with a random name; - is refused when it resolves to a regular tool's skill copy. skillFormat "muse" rewrites the entrypoint frontmatter to name: " _>" plus a one-line quoted description and keeps the body. It is a port of muse_frontmatter in pilot-skills muse/install.sh and produces the same bytes; a test runs the installer's own shell function against it (all 157 pilot-skills SKILL.md files match locally with PILOT_SKILLS_CORPUS). canonicalPath now resolves a not-yet-existing path through its nearest existing ancestor, so collision checks work before files are written. Tests: gated tick/plan/uninstall/offline-cache/disabled, marker and path validation, symlink and planted-temp-link refusal, and a test that the pilot-skills manifest with the new key passes the released parse and checks with no row a released daemon acts on under ~/workspace. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 37 ++ README.md | 14 +- gated.go | 340 ++++++++++++++++ manifest.go | 32 ++ reconcile.go | 22 +- skillformat.go | 230 +++++++++++ skillinject.go | 17 + testdata/inject-manifest-gated.json | 149 +++++++ testdata/muse_frontmatter.sh | 75 ++++ uninstall.go | 8 + zz_gated_released_internal_test.go | 191 +++++++++ zz_gated_test.go | 599 ++++++++++++++++++++++++++++ zz_skillformat_internal_test.go | 189 +++++++++ 13 files changed, 1894 insertions(+), 9 deletions(-) create mode 100644 gated.go create mode 100644 skillformat.go create mode 100644 testdata/inject-manifest-gated.json create mode 100644 testdata/muse_frontmatter.sh create mode 100644 zz_gated_released_internal_test.go create mode 100644 zz_gated_test.go create mode 100644 zz_skillformat_internal_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index ad2dda2..610e56b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,43 @@ 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 + that file must be inside `~/.pilot`. Without it 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. + +### 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..c9aaadf 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,15 @@ 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` (the Muse installer creates + `~/.pilot/targets/muse`). Without it 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 @@ -102,6 +112,8 @@ 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` exists; contained writes. | +| `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..784cecd --- /dev/null +++ b/gated.go @@ -0,0 +1,340 @@ +// 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. The marker has to be inside ~/.pilot, which only +// Pilot's own installers write; the Muse installer creates +// ~/.pilot/targets/muse. +// +// 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. +// - While the marker is absent 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. + +import ( + "crypto/rand" + "encoding/hex" + "errors" + "fmt" + "io/fs" + "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 +} + +// markerPresent reports whether the marker exists. A marker that cannot +// be checked counts as absent, which keeps the tool off. +func markerPresent(path string) bool { + _, err := os.Stat(path) + return err == nil +} + +// 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 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) + } + if !markerPresent(marker) { + return Outcome{}, true + } + t, err := resolveGatedTarget(gt, entrypoint, home) + if err != nil { + o.Path = gt.SkillsDir + return fail(err) + } + o.Path = t.path + 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)) + } + want, err := formatSkill(skillBody, gt.SkillFormat, entrypoint) + if err != nil { + return fail(err) + } + 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, 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) + } + if !markerPresent(marker) { + return nil + } + t, err := resolveGatedTarget(gt, entrypoint, home) + if err != nil { + r.Path = gt.SkillsDir + return fail(err) + } + 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..c68e611 100644 --- a/manifest.go +++ b/manifest.go @@ -43,6 +43,38 @@ 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 whose existence turns the tool on, e.g. + // "~/.pilot/targets/muse". It must be inside ~/.pilot. While it is + // absent, 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 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..c211f46 100644 --- a/skillinject.go +++ b/skillinject.go @@ -251,6 +251,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 +301,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 +361,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..cb21e10 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. 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..981e22f --- /dev/null +++ b/zz_gated_test.go @@ -0,0 +1,599 @@ +// 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") +} + +func markMuse(t *testing.T, home string) { + t.Helper() + mustMkdirAll(t, filepath.Join(home, ".pilot", "targets")) + mustWriteFile(t, filepath.Join(home, ".pilot", "targets", "muse"), "", 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_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) +} From 1f6744536510c78c136fd306875d74977586e9db Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Thu, 24 Sep 2026 05:30:26 +0300 Subject: [PATCH 2/3] gatedTools: marker must name the target; refresh rotating proxy credentials Two review findings on the Muse target. 1. A marker proved only that some installer ran. muse/install.sh writes ~/.pilot/targets/muse whatever MUSE_SKILLS_DIR and PILOT_MUSE_FRONTMATTER say, so on a host where it installed another agent's skills folder the daemon created ~/workspace/skills/pilotctl/SKILL.md in an unrelated directory, and after a PILOT_MUSE_FRONTMATTER=0 install the daemon and the installer rewrote each other's copy on every run. The marker now has to say what it is for, one key=value per line: skills_dir=/root/workspace/skills skill_format=muse A gated tool is active only when skills_dir is the row's skillsDir (compared after resolving symlinks) and skill_format its skillFormat ("canonical" for a row that copies SKILL.md unchanged). An empty marker (a bare touch), one for another folder or format, a symlink or directory, a file over 4 KiB, a line without '=', or a key set twice turns nothing on: tick, Plan and Uninstall leave the directory alone. The row itself is still checked first, so a broken row is an error on every marked host whatever the marker says. 2. The daemon registers skillinject with a zero Config, so fetches went through http.DefaultTransport with the proxy credentials from the daemon's launch environment. The Muse proxy rotates them every few minutes, and every tick after the first rotation failed with 407. When Config.HTTPClient is nil, the client is now a netproxy.RefreshingTransport whose refresh command is the new Config.ProxyCommand, else $PILOT_PROXY_CMD, else "proxy_cmd" in ~/.pilot/config.json, the settings pilot-daemon itself reads (the installers save bash -c 'printf %s "${https_proxy:-$HTTPS_PROXY}"' there on such hosts). The command 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 / config.json "proxy" set to an off word disables it. Without a command the client is the plain one, and a 407 reported as a *netproxy.ConnectError (what pilot-daemon's http.DefaultTransport returns after refreshing its own credentials) is retried once. The base transport is a copy of DefaultTransport without its CONNECT hook, so a host hook cannot hide the 407. Tests: marker attestation (14 rejected shapes, 8 accepted spellings including symlinked paths, the canonical/muse opt-out sequence, a broken row under a foreign marker); a Muse-like rotating CONNECT proxy for ticks across rotations with local-edit repair, a rotation mid-tick (407 then retry), the command from $PILOT_PROXY_CMD and config.json, a failing command falling back to the environment with no secret in any error, the one retry for a host transport's 407, and the command precedence rules. The new marker tests fail on the previous commit. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 29 +- README.md | 39 ++- gated.go | 194 ++++++++++-- manifest.go | 61 +++- proxy.go | 168 ++++++++++ skillinject.go | 15 +- uninstall.go | 1 + zz_gated_marker_test.go | 274 ++++++++++++++++ zz_gated_test.go | 9 +- zz_proxy_refresh_internal_test.go | 508 ++++++++++++++++++++++++++++++ 10 files changed, 1248 insertions(+), 50 deletions(-) create mode 100644 proxy.go create mode 100644 zz_gated_marker_test.go create mode 100644 zz_proxy_refresh_internal_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 610e56b..472623b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,9 +11,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - 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 - that file must be inside `~/.pilot`. Without it nothing under the tool's - `rootDir` is read, written or removed, by a tick or by `Uninstall`. + 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`, @@ -36,6 +44,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `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. + ### Changed - `canonicalPath` resolves a path that does not exist yet through its diff --git a/README.md b/README.md index c9aaadf..a302ccb 100644 --- a/README.md +++ b/README.md @@ -61,12 +61,22 @@ to be earned with full transparency, so here is the whole story: - **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` (the Muse installer creates - `~/.pilot/targets/muse`). Without it 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`. + 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 @@ -102,6 +112,20 @@ 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` (the Pilot installers save +`bash -c 'printf %s "${https_proxy:-$HTTPS_PROXY}"'` there on such hosts). +The command runs at the start of each tick, again once a minute while it +runs, and when the proxy answers 407, after which the refused request is +retried once. Its output is never logged. `PILOT_PROXY=off` (or +`config.json` `"proxy": "off"`) turns this off. See `proxy.go`. + ## Layout | File | What it does | @@ -112,7 +136,8 @@ 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` exists; contained writes. | +| `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. | diff --git a/gated.go b/gated.go index 784cecd..2db2530 100644 --- a/gated.go +++ b/gated.go @@ -8,9 +8,10 @@ package skillinject // ~/.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. The marker has to be inside ~/.pilot, which only -// Pilot's own installers write; the Muse installer creates -// ~/.pilot/targets/muse. +// 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 @@ -23,8 +24,10 @@ package skillinject // // - requireMarker must resolve inside ~/.pilot. Otherwise the row is an // error and nothing happens. -// - While the marker is absent the tool is skipped: nothing under rootDir -// is read, written or removed, by a tick or by Uninstall. +// - 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 @@ -38,13 +41,39 @@ package skillinject // 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" @@ -72,11 +101,118 @@ func gatedMarkerPath(gt ManifestGatedTool, home string) (string, error) { return marker, nil } -// markerPresent reports whether the marker exists. A marker that cannot -// be checked counts as absent, which keeps the tool off. -func markerPresent(path string) bool { - _, err := os.Stat(path) - return err == 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 @@ -127,11 +263,11 @@ func validIdentifier(s string) bool { } // reconcileGatedTool installs or refreshes one gated tool's skill copy. -// skipped is true when the tool is inactive (marker 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. +// 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) { @@ -145,24 +281,31 @@ func reconcileGatedTool(gt ManifestGatedTool, entrypoint, home string, skillBody o.Path = gt.RequireMarker return fail(err) } - if !markerPresent(marker) { + 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)) } - want, err := formatSkill(skillBody, gt.SkillFormat, entrypoint) - if err != nil { - return fail(err) + if err := decl.attests(gt, home); err != nil { + logInactive(gt, marker, err) + return Outcome{}, true } o.Hash = sha256Hex(want) @@ -188,8 +331,9 @@ func reconcileGatedTool(gt ManifestGatedTool, entrypoint, home string, skillBody // 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, since -// nothing under rootDir may be touched then. +// 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 { @@ -203,7 +347,8 @@ func removeGatedTool(gt ManifestGatedTool, entrypoint, home string) []Removal { r.Path = gt.RequireMarker return fail(err) } - if !markerPresent(marker) { + decl, present := readGatedMarker(marker) + if !present { return nil } t, err := resolveGatedTarget(gt, entrypoint, home) @@ -211,6 +356,11 @@ func removeGatedTool(gt ManifestGatedTool, entrypoint, home string) []Removal { 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 diff --git a/manifest.go b/manifest.go index c68e611..e8b38f0 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. @@ -66,9 +65,12 @@ type ManifestGatedTool struct { // SkillNaming is "" (directory layout, the default) or "flat", as // for ManifestTool. SkillNaming string `json:"skillNaming,omitempty"` - // RequireMarker is the file whose existence turns the tool on, e.g. - // "~/.pilot/targets/muse". It must be inside ~/.pilot. While it is - // absent, nothing under RootDir is read, written or removed. + // 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 @@ -188,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 == "" { @@ -208,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() } } @@ -305,12 +322,14 @@ 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 && proxyAuthRejected(err) && ctx.Err() == nil { + // 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) } - req.Header.Set("User-Agent", "pilot-daemon/skillinject") - resp, err := f.httpClient.Do(req) if err != nil { return nil, err } @@ -322,6 +341,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..bb86c59 --- /dev/null +++ b/proxy.go @@ -0,0 +1,168 @@ +// 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; the Pilot installers save the Muse one there: +// +// bash -c 'printf %s "${https_proxy:-$HTTPS_PROXY}"' +// +// 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. + +import ( + "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 +} diff --git a/skillinject.go b/skillinject.go index c211f46..d3691f5 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:-$HTTPS_PROXY}"'. + // 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 { diff --git a/uninstall.go b/uninstall.go index cb21e10..86677cd 100644 --- a/uninstall.go +++ b/uninstall.go @@ -203,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_test.go b/zz_gated_test.go index 981e22f..d17238b 100644 --- a/zz_gated_test.go +++ b/zz_gated_test.go @@ -44,10 +44,17 @@ 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"), "", 0o644) + mustWriteFile(t, filepath.Join(home, ".pilot", "targets", "muse"), content, 0o644) } func toolOutcome(rep *skillinject.Report, tool string) (skillinject.Outcome, bool) { diff --git a/zz_proxy_refresh_internal_test.go b/zz_proxy_refresh_internal_test.go new file mode 100644 index 0000000..4bfbc0a --- /dev/null +++ b/zz_proxy_refresh_internal_test.go @@ -0,0 +1,508 @@ +// 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 + + 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)) + _, _ = 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()) + } + }) + } +} + +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) + } +} From 908afcc3fe300e8a2aef61073fb10179a1e969a9 Mon Sep 17 00:00:00 2001 From: Teodor Calin Date: Thu, 24 Sep 2026 12:46:20 +0300 Subject: [PATCH 3/3] fetch: retry a garbled proxy rejection once on a transport skillinject does not own si42-proxycmd-flag-only-unparseable-rejection-no-retry: with no proxy command in sight (pilot-daemon given only the -proxy-cmd flag), fetches use the process's http.DefaultTransport, and only a well-formed 407 reached its CONNECT hook (which refreshes the credentials) and got a retry. A rejection the proxy garbles ("HTTP/1.1 4O7", "malformed HTTP status code", Meta Muse's form) never reaches that hook, so the tick failed with no retry. get now retries such an answer once after unreadableReplyRetryDelay (2s), by when the transport's resolver has usually re-read the credentials in the background (netproxy starts that from the lookup that failed once its 60s interval has passed). One retry only: without a refresh it fails after exactly two attempts. TestFetch_RetriesUnreadableProxyReplyOnce covers both (background refresh: success after one rejection; none: two rejections, no loop); it fails with the retry disabled. The rotating test proxy can now garble its rejection. Docs: README/CHANGELOG/proxy.go name who supplies the command on sandbox hosts (release#49 saves proxy_cmd; pilotctl daemon start, pilot-up.sh and pilot-mcp hand the daemon $PILOT_PROXY_CMD) and the credential-preferring sandbox command they all use. go test -race ./... ok; go vet clean. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 6 ++- README.md | 17 +++++-- manifest.go | 27 +++++++--- proxy.go | 55 +++++++++++++++++++-- skillinject.go | 2 +- zz_proxy_refresh_internal_test.go | 82 +++++++++++++++++++++++++++++++ 6 files changed, 173 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 472623b..f173e60 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,7 +57,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `"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. + 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 diff --git a/README.md b/README.md index a302ccb..76b5de0 100644 --- a/README.md +++ b/README.md @@ -119,12 +119,19 @@ 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` (the Pilot installers save -`bash -c 'printf %s "${https_proxy:-$HTTPS_PROXY}"'` there on such hosts). +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 answers 407, after which the refused request is -retried once. Its output is never logged. `PILOT_PROXY=off` (or -`config.json` `"proxy": "off"`) turns this off. See `proxy.go`. +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 diff --git a/manifest.go b/manifest.go index e8b38f0..49e2219 100644 --- a/manifest.go +++ b/manifest.go @@ -323,12 +323,27 @@ func decodeEd25519PublicKey(s string) (ed25519.PublicKey, error) { func (f *fetcher) get(ctx context.Context, url string) ([]byte, error) { resp, err := f.do(ctx, url) - if err != nil && !f.ownsTransport && proxyAuthRejected(err) && ctx.Err() == nil { - // 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) + 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) + } + } } if err != nil { return nil, err diff --git a/proxy.go b/proxy.go index bb86c59..4cf6935 100644 --- a/proxy.go +++ b/proxy.go @@ -14,9 +14,11 @@ package skillinject // 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; the Pilot installers save the Muse one there: +// 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 'printf %s "${https_proxy:-$HTTPS_PROXY}"' +// 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): @@ -26,9 +28,13 @@ package skillinject // 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. +// 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" @@ -166,3 +172,46 @@ 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/skillinject.go b/skillinject.go index d3691f5..811fd16 100644 --- a/skillinject.go +++ b/skillinject.go @@ -78,7 +78,7 @@ type Config struct { 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:-$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 diff --git a/zz_proxy_refresh_internal_test.go b/zz_proxy_refresh_internal_test.go index 4bfbc0a..b0798c6 100644 --- a/zz_proxy_refresh_internal_test.go +++ b/zz_proxy_refresh_internal_test.go @@ -39,6 +39,10 @@ type rotatingProxy struct { // 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" } @@ -129,6 +133,10 @@ func (p *rotatingProxy) handle(c net.Conn) { 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 } @@ -439,6 +447,80 @@ func TestFetch_RetriesProxyAuthRejectionOnce(t *testing.T) { } } +// 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 {