From 4d487fa15b96394b957d9ca515a8522b2017a6dc Mon Sep 17 00:00:00 2001 From: Victor Hang Date: Sun, 27 Sep 2026 19:19:57 +0000 Subject: [PATCH 1/2] feat(module): let modules declare per-workspace bind mounts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A module.yml can now list `mounts` (host `source`, absolute or `~/`-relative, container `target`, `readOnly`, `optional`). They are bind-mounted only into the containers of the workspaces that have the module installed, unlike the global `extraMounts`, so access to a host path — e.g. a login shared with the host rather than copied — can be granted per workspace. The mounts are fingerprinted into OCM_MODULE_MOUNTS_SHA256, so the container is recreated when the set changes. AddModule records such a module before starting the container, so its install already sees the mounts (and rolls the entry back if it fails); RemoveModule recreates the container after the uninstall. Mount points in the workspace home are created on the host first. A module with mounts cannot set restartServer: false, since recreation interrupts a running task. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 1 + docs/writing-modules.md | 39 +++++ internal/module/module.go | 59 +++++++ internal/module/module_test.go | 54 ++++++ internal/workspace/lifecycle.go | 15 ++ internal/workspace/lifecycle_drift_test.go | 12 ++ internal/workspace/modulemounts_test.go | 128 ++++++++++++++ internal/workspace/modules.go | 187 ++++++++++++++++++++- 8 files changed, 494 insertions(+), 1 deletion(-) create mode 100644 internal/workspace/modulemounts_test.go diff --git a/AGENTS.md b/AGENTS.md index d36b24b..e26a4c0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -26,6 +26,7 @@ - A module.yml may declare `key: ` to become multi-instance: it can be installed several times per workspace (e.g. one `ssh` entry per host, one `aws` entry per profile), each distinguished by that prompt's value. The instance identity is `name:keyvalue` (`ModuleInstance.InstanceID`); the key prompt must be required and non-secret. Without `key` a module is a singleton and a second install replaces the first. The install/uninstall scripts must be instance-scoped by the key (e.g. ssh writes `~/.ssh/config.d/$OCM_HOST.conf`) so multiple entries coexist and each uninstall removes only its own. - The `key` prompt of a multi-instance module may carry an `optionsCommand` (the only non-select prompt allowed to). It runs on the host to list accounts importable as new instances (e.g. `aws`/`outscale` list profiles, `ssh` lists `~/.ssh/config` host aliases). In the editor's add flow this drives an import picker: selecting one or more host accounts creates one instance each storing only the key value, with the actual credentials pulled by the module's `resolve` hook at install time (so secrets never reach the manifest); the picker also offers an "Add manually…" option that opens a single-page form (all fields at once) for an account not present on the host, whose typed values are stored as usual. The key prompt stays a free-text `string` so manual entry still works; `resolve` must emit nothing for an account it does not find on the host, leaving the manually-typed values in place. - A module.yml may declare `restartServer: false` to mark itself environment-neutral: it writes only its own config files (e.g. `~/.kube/config`, `~/.aws/credentials`) that tools read live, never `~/.env`. Such a module is never bounced (the manager skips the server restart even if `~/.env` somehow changed — and logs a warning, since that means the manifest lies) and, crucially, its add/remove is allowed from the editor **while a task is running**, because nothing interrupts the in-flight session. Omitting the key (or setting `restartServer: true`) keeps the conservative default: the module is assumed to touch `~/.env`, so the manager bounces the server when `~/.env` changes and the editor blocks the edit until the workspace is idle. All current built-ins (`aws`, `git`, `kubernetes`, `outscale`, `ssh`) are `restartServer: false`. +- A module.yml may declare `mounts` (`source` absolute or `~/`-relative on the host, absolute `target`, `readOnly`, `optional`). They are bind-mounted only into the containers of workspaces that have the module installed: `provision` collects them from the manifest's modules (`installedModuleMounts`), creates their mount points inside the workspace home on the host, and records a fingerprint in `OCM_MODULE_MOUNTS_SHA256`, so `containerSpecDrift` recreates the container when the set changes. `AddModule` records such a module in the manifest before starting the container, so its install runs with the mounts in place (rolled back on failure); `RemoveModule` recreates the container after the uninstall. A module with mounts cannot be `restartServer: false`, since recreation interrupts a running task. An `optional` mount with a missing source is skipped; a required one fails the start. - Workspace containers are long-lived and tied to the workspace lifecycle. - Shared OpenCode config lives in `~/.config/opencode-manager/opencode/` and is copied one-way into each workspace's `home/.config/opencode/` on provision, startup reconcile, and source changes while the manager runs. A per-workspace journal permits safe removal of previously managed source entries. The shared source must not contain generated top-level state (`.gitignore`, package manifests/lockfiles, or `node_modules`); those are preserved per workspace. Existing legacy root templates migrate only when `opencode/` is absent or empty. - Opt-in `selfImprovement.enabled` creates an OpenCode-only internal instance under `/internal/self-improvement`, outside `Registry.List`. `ocm improve` or dashboard `i` launches a fresh local OpenCode TUI with an analysis prompt. It mounts the entire manager configuration read-write, ordinary workspace homes read-only, and named `selfImprovement.directories` roots. Its port still participates in allocation. Agents/commands/helpers are manager-owned; personal `self-improvement/AGENTS.md` extends or replaces the built-in protocol. `sessions.ts` enforces a persistent initial baseline, incremental chunk budgets and checkpoints; `memory.ts` retains cumulative evidence and decisions and applies accepted proposals with hash checks/backups, independently of Git. See `docs/self-improvement.md`; regressions run with `bun test internal/workspace/improvement_sessions.test.ts` (also part of `npm test`). diff --git a/docs/writing-modules.md b/docs/writing-modules.md index 8e13345..c00b899 100644 --- a/docs/writing-modules.md +++ b/docs/writing-modules.md @@ -59,6 +59,7 @@ prompts: | `restartServer` | `true` (default) if the module touches `~/.env`; `false` if it only writes its own config files. | | `key` | Name of a prompt that makes the module **multi-instance** (one install per distinct value). | | `prompts` | The values to collect from the user. | +| `mounts` | Optional host paths bind-mounted into the containers of the workspaces that have the module (see [Mounts](#mounts)). | ### Prompt types @@ -101,6 +102,42 @@ and the editor blocks edits while a task is running. If your module only writes its own config files that tools read live (like `~/.aws` or `~/.kube/config`), set `restartServer: false` — it is never bounced and can be edited mid-task. +## Mounts + +A module can bind-mount host paths into the container of each workspace it is +installed in — and only those. Unlike the global +[`extraMounts`](configuration.md#extramounts), this lets you pick per workspace +which ones get access to a host file, such as a login shared with the host +instead of copied: + +```yaml +name: claude-auth +version: 1 +description: Share this host's Claude Code login with the workspace. +mounts: + - source: ~/.claude/.credentials.json + target: /home/debian/.claude/.credentials.json + optional: true +``` + +| Field | Meaning | +| --- | --- | +| `source` | Host path: absolute, or starting with `~/` for the host user's home. | +| `target` | Absolute path inside the container. Cannot be `/` or the workspace home itself. | +| `readOnly` | `true` to prevent container writes. Default `false`. | +| `optional` | `true` to skip the mount while `source` does not exist instead of failing the workspace start; it is added on the first start after it appears. | + +The mounts are part of the container's spec, so adding or removing such a +module **recreates the workspace container** (the home and the other modules +are preserved and reinstalled). On add, the module is recorded and the container +recreated before `install` runs, so the script already sees the mounted paths; on +removal the container is recreated after `uninstall`, so access ends right away. +Because recreation interrupts a running task, a module with mounts cannot set +`restartServer: false`. + +Mount points inside the workspace home are created on the host first (an empty +file or a directory, owned by you), so the runtime never creates them as root. + ## Host hooks (optional) Two optional hooks run **on the host** (with full host access), as part of an @@ -138,5 +175,7 @@ leaving any manually-typed values in place. - [ ] `uninstall` fully reverses `install` for that instance. - [ ] Secrets resolved on the host via `resolve` rather than stored in the manifest, where applicable. +- [ ] `mounts` only when the container must use the host path itself (live, + shared state) rather than a copy made by `resolve`. - [ ] Tested by adding/removing the module on a running workspace from the editor. diff --git a/internal/module/module.go b/internal/module/module.go index b9199a3..45bfb4d 100644 --- a/internal/module/module.go +++ b/internal/module/module.go @@ -72,6 +72,37 @@ type Module struct { // lives under (e.g. "cloud" for modules/cloud/aws). It organizes the module // browser in the TUI and is mirrored in the container mount path. Category string + // Mounts are host paths bind-mounted into the container of every workspace + // that has this module installed, and only those. Adding or removing such a + // module recreates the workspace container, so it always restarts the server. + Mounts []Mount +} + +// Mount is a host path a module bind-mounts into the workspace container. +type Mount struct { + // Source is the host path: absolute, or relative to the host user's home + // when it starts with "~/". + Source string `yaml:"source"` + // Target is the absolute path inside the container. + Target string `yaml:"target"` + ReadOnly bool `yaml:"readOnly"` + // Optional skips the mount while Source does not exist on the host instead + // of failing the workspace start. The mount is added on the first start + // after it appears. + Optional bool `yaml:"optional"` +} + +// HostSource returns Source with a leading "~/" expanded to the host user's +// home directory. +func (m Mount) HostSource() (string, error) { + if !strings.HasPrefix(m.Source, "~/") { + return m.Source, nil + } + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("find user home directory: %w", err) + } + return filepath.Join(home, m.Source[2:]), nil } // Multi reports whether the module can be installed as multiple independent @@ -136,6 +167,7 @@ type definition struct { Key string `yaml:"key"` RestartServer *bool `yaml:"restartServer"` Prompts []Prompt `yaml:"prompts"` + Mounts []Mount `yaml:"mounts"` } // HasResolveHook reports whether the module ships a host-side resolve script. @@ -163,6 +195,7 @@ func Load(dir string) (Module, error) { Prompts: def.Prompts, Key: def.Key, RestartServer: def.RestartServer == nil || *def.RestartServer, + Mounts: def.Mounts, Dir: dir, // The category is the name of the directory the module lives under, e.g. // modules/cloud/aws -> "cloud". @@ -172,6 +205,11 @@ func Load(dir string) (Module, error) { if err := mod.validate(); err != nil { return Module{}, fmt.Errorf("invalid module %q: %w", path, err) } + // A module with mounts recreates the container on add and remove, which + // interrupts a running task just like a server bounce does. + if len(mod.Mounts) > 0 && !mod.RestartServer { + return Module{}, fmt.Errorf("invalid module %q: a module with mounts cannot set restartServer: false (adding or removing it recreates the container)", path) + } return mod, nil } @@ -253,6 +291,27 @@ func (m Module) validate() error { } } + targets := map[string]bool{} + for _, mount := range m.Mounts { + if mount.Source == "" || mount.Target == "" { + return fmt.Errorf("mount source and target are required") + } + if !filepath.IsAbs(mount.Source) && !strings.HasPrefix(mount.Source, "~/") { + return fmt.Errorf("mount source %q must be absolute or start with ~/", mount.Source) + } + if !filepath.IsAbs(mount.Target) { + return fmt.Errorf("mount target %q must be absolute", mount.Target) + } + target := filepath.Clean(mount.Target) + if target == "/" || target == "/home/debian" { + return fmt.Errorf("mount target %q cannot replace %s", mount.Target, target) + } + if targets[target] { + return fmt.Errorf("mount target %q is duplicated", mount.Target) + } + targets[target] = true + } + if err := checkExecutable(filepath.Join(m.Dir, InstallScript)); err != nil { return err } diff --git a/internal/module/module_test.go b/internal/module/module_test.go index b40871d..9c2a381 100644 --- a/internal/module/module_test.go +++ b/internal/module/module_test.go @@ -402,3 +402,57 @@ func TestBuiltinModulesAreValid(t *testing.T) { t.Fatal("kubernetes context prompt should expose an optionsCommand for the import picker") } } + +func TestLoadMounts(t *testing.T) { + root := t.TempDir() + dir := writeModule(t, root, "claude", `name: claude +version: 1 +mounts: + - source: ~/.claude/.credentials.json + target: /home/debian/.claude/.credentials.json + optional: true + - { source: /etc/hosts, target: /etc/host-hosts, readOnly: true } +`) + + mod, err := Load(dir) + if err != nil { + t.Fatalf("Load: %v", err) + } + if len(mod.Mounts) != 2 || !mod.Mounts[0].Optional || !mod.Mounts[1].ReadOnly { + t.Fatalf("unexpected mounts: %+v", mod.Mounts) + } + if !mod.RestartServer { + t.Fatal("a module with mounts must restart the server") + } + + home, err := os.UserHomeDir() + if err != nil { + t.Fatal(err) + } + source, err := mod.Mounts[0].HostSource() + if err != nil { + t.Fatalf("HostSource: %v", err) + } + if want := filepath.Join(home, ".claude/.credentials.json"); source != want { + t.Fatalf("HostSource = %q, want %q", source, want) + } +} + +func TestLoadRejectsInvalidMounts(t *testing.T) { + cases := map[string]string{ + "relative source": "mounts: [{ source: foo, target: /a }]", + "relative target": "mounts: [{ source: /foo, target: a }]", + "missing target": "mounts: [{ source: /foo }]", + "home target": "mounts: [{ source: /foo, target: /home/debian/ }]", + "duplicate target": "mounts: [{ source: /foo, target: /a }, { source: /bar, target: /a }]", + "no server restart": "restartServer: false\nmounts: [{ source: /foo, target: /a }]", + } + for name, extra := range cases { + t.Run(name, func(t *testing.T) { + dir := writeModule(t, t.TempDir(), "m", "name: m\nversion: 1\n"+extra+"\n") + if _, err := Load(dir); err == nil { + t.Fatal("expected an error") + } + }) + } +} diff --git a/internal/workspace/lifecycle.go b/internal/workspace/lifecycle.go index beeb30e..28475af 100644 --- a/internal/workspace/lifecycle.go +++ b/internal/workspace/lifecycle.go @@ -371,6 +371,12 @@ func (l Lifecycle) provisionWithOptions(ctx context.Context, summary Summary, re // scripts are runnable inside the container. mounts = append(mounts, moduleMounts(l.cfg)...) mounts = append(mounts, extraMounts(l.cfg)...) + // Mounts declared by the modules installed in this workspace only. + installedMounts, installedMountsFingerprint, err := l.installedModuleMounts(manifest) + if err != nil { + return runtime.StatusUnknown, runtime.ContainerSpec{}, err + } + mounts = append(mounts, installedMounts...) if l.isImprovement(summary) { internalMounts, err := l.improvementMounts() if err != nil { @@ -417,6 +423,9 @@ func (l Lifecycle) provisionWithOptions(ctx context.Context, summary Summary, re if fingerprint := extraMountsFingerprint(l.cfg.ExtraMounts); fingerprint != "" { env[extraMountsFingerprintEnv] = fingerprint } + if installedMountsFingerprint != "" { + env[moduleMountsFingerprintEnv] = installedMountsFingerprint + } if l.isImprovement(summary) { privateMounts := make([]config.ExtraMount, 0, len(mounts)) for _, mount := range mounts { @@ -557,6 +566,7 @@ const openCodeAuthRelPath = ".local/share/opencode/auth.json" const ( extraCACertificateFingerprintEnv = "OCM_EXTRA_CA_CERTIFICATE_SHA256" extraMountsFingerprintEnv = "OCM_EXTRA_MOUNTS_SHA256" + moduleMountsFingerprintEnv = "OCM_MODULE_MOUNTS_SHA256" workspaceEnvKeysEnv = "OCM_WORKSPACE_ENV_KEYS" ) @@ -892,6 +902,11 @@ func (l Lifecycle) containerSpecDrift(ctx context.Context, manifest Manifest, sp return true } + if rc.Env[moduleMountsFingerprintEnv] != spec.Env[moduleMountsFingerprintEnv] { + slog.Debug("container module mounts differ from desired", "workspace", manifest.Name, "container", manifest.ContainerName) + return true + } + if rc.Env[workspaceEnvKeysEnv] != spec.Env[workspaceEnvKeysEnv] { slog.Debug("container workspace environment keys differ from desired", "workspace", manifest.Name, "container", manifest.ContainerName) return true diff --git a/internal/workspace/lifecycle_drift_test.go b/internal/workspace/lifecycle_drift_test.go index f3113dd..52653b5 100644 --- a/internal/workspace/lifecycle_drift_test.go +++ b/internal/workspace/lifecycle_drift_test.go @@ -89,6 +89,18 @@ func TestContainerSpecDrift(t *testing.T) { spec: runtime.ContainerSpec{Env: map[string]string{extraMountsFingerprintEnv: "new"}}, want: true, }, + { + name: "module mount added", + rc: runtime.ContainerRuntimeConfig{NetworkMode: "bridge", Env: map[string]string{OpenCodePortEnv: "4097"}}, + spec: runtime.ContainerSpec{Env: map[string]string{moduleMountsFingerprintEnv: "new"}}, + want: true, + }, + { + name: "module mount removed", + rc: runtime.ContainerRuntimeConfig{NetworkMode: "bridge", Env: map[string]string{OpenCodePortEnv: "4097", moduleMountsFingerprintEnv: "old"}}, + spec: runtime.ContainerSpec{Env: map[string]string{}}, + want: true, + }, { name: "workspace environment changed", rc: runtime.ContainerRuntimeConfig{NetworkMode: "bridge", Env: map[string]string{OpenCodePortEnv: "4097", workspaceEnvKeysEnv: "API_TOKEN", "API_TOKEN": "old"}}, diff --git a/internal/workspace/modulemounts_test.go b/internal/workspace/modulemounts_test.go new file mode 100644 index 0000000..27e88a4 --- /dev/null +++ b/internal/workspace/modulemounts_test.go @@ -0,0 +1,128 @@ +package workspace + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/mickael-menu/opencode-manager/internal/config" +) + +// writeMountingModule creates a module in /tools/ declaring mounts. +func writeMountingModule(t *testing.T, root, name, mounts string) { + t.Helper() + dir := filepath.Join(root, "tools", name) + writeTestFile(t, filepath.Join(dir, "module.yml"), []byte("name: "+name+"\nversion: 1\nmounts:\n"+mounts)) + for _, script := range []string{"install", "uninstall"} { + if err := os.WriteFile(filepath.Join(dir, script), []byte("#!/bin/sh\n"), 0o755); err != nil { + t.Fatal(err) + } + } +} + +func TestInstalledModuleMountsOnlyForInstalledModules(t *testing.T) { + modules := t.TempDir() + hostDir := t.TempDir() + creds := filepath.Join(hostDir, "credentials.json") + writeTestFile(t, creds, []byte("{}")) + + writeMountingModule(t, modules, "claude-auth", " - { source: "+creds+", target: /home/debian/.claude/.credentials.json }\n") + writeMountingModule(t, modules, "other", " - { source: "+hostDir+", target: /opt/other }\n") + writeMountingModule(t, modules, "maybe", " - { source: "+filepath.Join(hostDir, "missing")+", target: /opt/maybe, optional: true }\n") + + l := Lifecycle{cfg: config.Config{ModuleDirs: []string{modules}}} + home := t.TempDir() + manifest := Manifest{HomeDir: home} + + mounts, fingerprint, err := l.installedModuleMounts(manifest) + if err != nil || mounts != nil || fingerprint != "" { + t.Fatalf("no modules: got %v %q %v", mounts, fingerprint, err) + } + + manifest.Modules = []ModuleInstance{ + {Name: "claude-auth", Category: "tools", Version: 1}, + {Name: "maybe", Category: "tools", Version: 1}, + } + mounts, fingerprint, err = l.installedModuleMounts(manifest) + if err != nil { + t.Fatalf("installedModuleMounts: %v", err) + } + if len(mounts) != 1 || mounts[0].Source != creds || mounts[0].Target != "/home/debian/.claude/.credentials.json" || mounts[0].ReadOnly { + t.Fatalf("unexpected mounts: %+v", mounts) + } + if fingerprint == "" { + t.Fatal("expected a fingerprint") + } + info, err := os.Stat(filepath.Join(home, ".claude", ".credentials.json")) + if err != nil || info.IsDir() { + t.Fatalf("mount point not created as a file in the workspace home: %v", err) + } + + // The optional source appearing changes the fingerprint, so the container is + // recreated with the new mount. + if err := os.Mkdir(filepath.Join(hostDir, "missing"), 0o755); err != nil { + t.Fatal(err) + } + mounts, withOptional, err := l.installedModuleMounts(manifest) + if err != nil || len(mounts) != 2 || withOptional == fingerprint { + t.Fatalf("optional mount: got %+v %q (was %q) %v", mounts, withOptional, fingerprint, err) + } +} + +func TestInstalledModuleMountsRequiredSourceMissing(t *testing.T) { + modules := t.TempDir() + writeMountingModule(t, modules, "need", " - { source: /nonexistent/ocm-test, target: /opt/need }\n") + l := Lifecycle{cfg: config.Config{ModuleDirs: []string{modules}}} + manifest := Manifest{HomeDir: t.TempDir(), Modules: []ModuleInstance{{Name: "need", Category: "tools", Version: 1}}} + if _, _, err := l.installedModuleMounts(manifest); err == nil { + t.Fatal("expected an error for a missing required mount source") + } +} + +func TestAddMountingModuleRecordsBeforeInstall(t *testing.T) { + modules := t.TempDir() + creds := filepath.Join(t.TempDir(), "credentials.json") + writeTestFile(t, creds, []byte("{}")) + writeMountingModule(t, modules, "claude-auth", " - { source: "+creds+", target: /home/debian/.claude/.credentials.json }\n") + + workspacePath := t.TempDir() + home := filepath.Join(workspacePath, "home") + if err := os.MkdirAll(filepath.Join(home, ".config", "opencode"), 0o755); err != nil { + t.Fatal(err) + } + manifest := Manifest{Name: "demo", Runtime: "docker", ImageName: "img", ContainerName: "c", HomeDir: home} + if err := SaveManifest(filepath.Join(workspacePath, ManifestFile), manifest); err != nil { + t.Fatal(err) + } + + fake := &fakeDriver{output: func(args []string) []byte { return nil }} + l := Lifecycle{cfg: config.Config{ModuleDirs: []string{modules}}, driver: fake} + catalog, err := l.Catalog() + if err != nil || len(catalog) != 1 { + t.Fatalf("Catalog: %v %v", catalog, err) + } + + if err := l.AddModule(context.Background(), Summary{Manifest: manifest, Path: workspacePath}, catalog[0], nil); err != nil { + t.Fatalf("AddModule: %v", err) + } + + saved, err := LoadManifest(filepath.Join(workspacePath, ManifestFile)) + if err != nil { + t.Fatal(err) + } + if len(saved.Modules) != 1 || saved.Modules[0].Name != "claude-auth" { + t.Fatalf("manifest modules: %+v", saved.Modules) + } + installs := 0 + for _, args := range fake.gotArgs { + if contains(args, "/opt/opencode-manager/modules/tools/claude-auth/install") { + installs++ + } + } + // The fake marker never records it, so reconcile and the explicit run both + // install; what matters is that it ran after the manifest listed it. + if installs == 0 { + t.Fatal("install script never ran") + } +} diff --git a/internal/workspace/modules.go b/internal/workspace/modules.go index 0f5484e..0a1f9e7 100644 --- a/internal/workspace/modules.go +++ b/internal/workspace/modules.go @@ -94,6 +94,109 @@ func (l Lifecycle) Catalog() ([]module.Module, error) { return module.Catalog([]string{dir}) } +// installedModuleMounts returns the bind mounts declared by the modules installed +// in the workspace, and a fingerprint of them that changes whenever the set does, +// so a workspace container is recreated exactly when one of its mounting modules +// is added or removed (or an optional source appears). Mount points under the +// workspace home are created on the host first, owned by the host user, so the +// runtime does not create them as root. +func (l Lifecycle) installedModuleMounts(manifest Manifest) ([]runtime.Mount, string, error) { + if len(manifest.Modules) == 0 { + return nil, "", nil + } + catalog, err := l.Catalog() + if err != nil { + return nil, "", err + } + byName := make(map[string]module.Module, len(catalog)) + for _, mod := range catalog { + byName[mod.Name] = mod + } + + var mounts []runtime.Mount + targets := map[string]string{} + for _, inst := range manifest.Modules { + mod, ok := byName[inst.Name] + if !ok { + continue + } + for _, declared := range mod.Mounts { + source, err := declared.HostSource() + if err != nil { + return nil, "", err + } + info, err := os.Stat(source) + if err != nil { + if os.IsNotExist(err) && declared.Optional { + slog.Debug("skipping optional module mount with a missing source", "module", mod.Name, "source", source) + continue + } + return nil, "", fmt.Errorf("module %s mount source %q: %w", mod.Name, source, err) + } + target := filepath.Clean(declared.Target) + if previous, ok := targets[target]; ok { + // Several instances of one module declare the same mount. + if previous == source { + continue + } + return nil, "", fmt.Errorf("module %s mounts %q onto %s, already used by %q", mod.Name, source, target, previous) + } + targets[target] = source + if err := ensureHomeMountPoint(manifest.HomeDir, target, info.IsDir()); err != nil { + return nil, "", err + } + mounts = append(mounts, runtime.Mount{Source: source, Target: target, ReadOnly: declared.ReadOnly}) + } + } + if len(mounts) == 0 { + return nil, "", nil + } + data, err := json.Marshal(mounts) + if err != nil { + return nil, "", fmt.Errorf("encode module mounts: %w", err) + } + sum := sha256.Sum256(data) + return mounts, hex.EncodeToString(sum[:]), nil +} + +// ensureHomeMountPoint creates the host-side mount point of a target inside the +// workspace home (a directory or an empty file), with its parent directories. +// Targets outside the home are left to the runtime. +func ensureHomeMountPoint(homeDir, target string, dir bool) error { + rel, err := filepath.Rel(openCodeHomeDir, target) + if homeDir == "" || err != nil || rel == "." || strings.HasPrefix(rel, "..") { + return nil + } + path := filepath.Join(homeDir, rel) + if dir { + if err := os.MkdirAll(path, 0o700); err != nil { + return fmt.Errorf("create module mount point %q: %w", path, err) + } + return nil + } + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + return fmt.Errorf("create module mount point %q: %w", filepath.Dir(path), err) + } + if _, err := os.Lstat(path); err == nil { + return nil + } + file, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY, 0o600) + if err != nil { + return fmt.Errorf("create module mount point %q: %w", path, err) + } + return file.Close() +} + +// moduleHasMounts reports whether an installed module declares mounts. +func (l Lifecycle) moduleHasMounts(category, name string) bool { + dir := primaryModuleDir(l.cfg) + if dir == "" || category == "" { + return false + } + mod, err := module.Load(filepath.Join(dir, category, name)) + return err == nil && len(mod.Mounts) > 0 +} + // AddModule installs a module into the workspace and records it in the manifest. // The install script runs inside the container as the workspace user (which has // passwordless sudo) with the prompt values passed as OCM_* environment @@ -106,6 +209,9 @@ func (l Lifecycle) AddModule(ctx context.Context, summary Summary, mod module.Mo } defer unlock() slog.Info("adding module to workspace", "workspace", summary.Manifest.Name, "module", mod.Name) + if len(mod.Mounts) > 0 { + return l.addMountingModule(ctx, summary, mod, values) + } if err := l.ensureStarted(ctx, summary, false); err != nil { return err } @@ -145,6 +251,75 @@ func (l Lifecycle) AddModule(ctx context.Context, summary Summary, mod module.Mo return nil } +// addMountingModule adds a module that declares mounts. The mounts must exist +// before its install script runs, so the module is recorded in the manifest +// first and the container recreated with them; the start's reconcile then runs +// the install. When the install does not succeed the manifest entry is rolled +// back, and the stray mount goes away on the next start. +func (l Lifecycle) addMountingModule(ctx context.Context, summary Summary, mod module.Module, values map[string]string) error { + id := mod.InstanceID(values) + manifestPath := filepath.Join(summary.Path, ManifestFile) + manifest, err := LoadManifest(manifestPath) + if err != nil { + return err + } + var previous *ModuleInstance + for _, m := range manifest.Modules { + if m.InstanceID() == id { + m := m + previous = &m + } + } + manifest.Modules = upsertModule(manifest.Modules, id, mod.Name, mod.Category, mod.Version, values) + manifest.UpdatedAt = time.Now().UTC() + if err := SaveManifest(manifestPath, manifest); err != nil { + return err + } + + rollback := func(cause error) error { + current, err := LoadManifest(manifestPath) + if err != nil { + return fmt.Errorf("%w (roll back manifest: %v)", cause, err) + } + current.Modules = removeModule(current.Modules, id) + if previous != nil { + current.Modules = append(current.Modules, *previous) + } + current.UpdatedAt = time.Now().UTC() + if err := SaveManifest(manifestPath, current); err != nil { + return fmt.Errorf("%w (roll back manifest: %v)", cause, err) + } + return cause + } + + summary.Manifest = manifest + if err := l.ensureStarted(ctx, summary, false); err != nil { + return rollback(err) + } + + // Reconcile already installed it unless the container was not recreated (the + // mounts were unchanged) or its install failed; run it here then, so the + // error reaches the user. + if v, ok := l.readMarker(ctx, manifest.ContainerName)[id]; !ok || v != mod.Version { + homeDir := manifest.HomeDir + before := envHash(homeDir) + installVals, err := l.resolveInstallValues(ctx, mod.Category, mod.Name, values) + if err != nil { + return rollback(err) + } + if err := l.runModuleScript(ctx, summary, mod.Category, mod.Name, module.InstallScript, "install", installVals); err != nil { + return rollback(err) + } + if err := l.writeMarker(ctx, manifest.ContainerName, manifest.Modules); err != nil { + return err + } + l.maybeBounce(ctx, manifest.ContainerName, mod.Name, mod.RestartServer, before, envHash(homeDir)) + } + + slog.Info("module added", "workspace", manifest.Name, "module", mod.Name) + return nil +} + // RemoveModule runs a module's uninstall script and drops it from the manifest. // id is the instance identity (ModuleInstance.InstanceID): the module name for a // singleton, or "name:keyvalue" for one entry of a multi-instance module. @@ -170,6 +345,7 @@ func (l Lifecycle) RemoveModule(ctx context.Context, summary Summary, id string) } } restartServer := l.moduleRestartServer(category, modName) + hasMounts := l.moduleHasMounts(category, modName) homeDir := summary.Manifest.HomeDir before := envHash(homeDir) @@ -194,7 +370,16 @@ func (l Lifecycle) RemoveModule(ctx context.Context, summary Summary, id string) return err } - l.maybeBounce(ctx, manifest.ContainerName, modName, restartServer, before, envHash(homeDir)) + if hasMounts { + // Recreate the container without the module's mounts, so the workspace + // loses access to them right away. The fresh server reads ~/.env anyway. + summary.Manifest = manifest + if err := l.ensureStarted(ctx, summary, false); err != nil { + return err + } + } else { + l.maybeBounce(ctx, manifest.ContainerName, modName, restartServer, before, envHash(homeDir)) + } slog.Info("module removed", "workspace", manifest.Name, "module", id) return nil From 4551ab5da0d4a16f0949fe5cce67f66257feaf31 Mon Sep 17 00:00:00 2001 From: Victor Hang Date: Mon, 28 Sep 2026 22:25:03 +0000 Subject: [PATCH 2/2] fix(module): drop a failed mounting module's mounts right away A failed add of a module with mounts rolled back the manifest but left the container it had recreated with the module's mounts running, so the workspace kept access to the host path until the next start. Reconcile the container with the restored manifest after the rollback, or stop it when that fails. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 2 +- internal/workspace/modulemounts_test.go | 94 +++++++++++++++++++++++++ internal/workspace/modules.go | 12 +++- 3 files changed, 106 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index e26a4c0..3b62dce 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -26,7 +26,7 @@ - A module.yml may declare `key: ` to become multi-instance: it can be installed several times per workspace (e.g. one `ssh` entry per host, one `aws` entry per profile), each distinguished by that prompt's value. The instance identity is `name:keyvalue` (`ModuleInstance.InstanceID`); the key prompt must be required and non-secret. Without `key` a module is a singleton and a second install replaces the first. The install/uninstall scripts must be instance-scoped by the key (e.g. ssh writes `~/.ssh/config.d/$OCM_HOST.conf`) so multiple entries coexist and each uninstall removes only its own. - The `key` prompt of a multi-instance module may carry an `optionsCommand` (the only non-select prompt allowed to). It runs on the host to list accounts importable as new instances (e.g. `aws`/`outscale` list profiles, `ssh` lists `~/.ssh/config` host aliases). In the editor's add flow this drives an import picker: selecting one or more host accounts creates one instance each storing only the key value, with the actual credentials pulled by the module's `resolve` hook at install time (so secrets never reach the manifest); the picker also offers an "Add manually…" option that opens a single-page form (all fields at once) for an account not present on the host, whose typed values are stored as usual. The key prompt stays a free-text `string` so manual entry still works; `resolve` must emit nothing for an account it does not find on the host, leaving the manually-typed values in place. - A module.yml may declare `restartServer: false` to mark itself environment-neutral: it writes only its own config files (e.g. `~/.kube/config`, `~/.aws/credentials`) that tools read live, never `~/.env`. Such a module is never bounced (the manager skips the server restart even if `~/.env` somehow changed — and logs a warning, since that means the manifest lies) and, crucially, its add/remove is allowed from the editor **while a task is running**, because nothing interrupts the in-flight session. Omitting the key (or setting `restartServer: true`) keeps the conservative default: the module is assumed to touch `~/.env`, so the manager bounces the server when `~/.env` changes and the editor blocks the edit until the workspace is idle. All current built-ins (`aws`, `git`, `kubernetes`, `outscale`, `ssh`) are `restartServer: false`. -- A module.yml may declare `mounts` (`source` absolute or `~/`-relative on the host, absolute `target`, `readOnly`, `optional`). They are bind-mounted only into the containers of workspaces that have the module installed: `provision` collects them from the manifest's modules (`installedModuleMounts`), creates their mount points inside the workspace home on the host, and records a fingerprint in `OCM_MODULE_MOUNTS_SHA256`, so `containerSpecDrift` recreates the container when the set changes. `AddModule` records such a module in the manifest before starting the container, so its install runs with the mounts in place (rolled back on failure); `RemoveModule` recreates the container after the uninstall. A module with mounts cannot be `restartServer: false`, since recreation interrupts a running task. An `optional` mount with a missing source is skipped; a required one fails the start. +- A module.yml may declare `mounts` (`source` absolute or `~/`-relative on the host, absolute `target`, `readOnly`, `optional`). They are bind-mounted only into the containers of workspaces that have the module installed: `provision` collects them from the manifest's modules (`installedModuleMounts`), creates their mount points inside the workspace home on the host, and records a fingerprint in `OCM_MODULE_MOUNTS_SHA256`, so `containerSpecDrift` recreates the container when the set changes. `AddModule` records such a module in the manifest before starting the container, so its install runs with the mounts in place; on failure the manifest entry is rolled back and the container recreated without the mounts; `RemoveModule` recreates the container after the uninstall. A module with mounts cannot be `restartServer: false`, since recreation interrupts a running task. An `optional` mount with a missing source is skipped; a required one fails the start. - Workspace containers are long-lived and tied to the workspace lifecycle. - Shared OpenCode config lives in `~/.config/opencode-manager/opencode/` and is copied one-way into each workspace's `home/.config/opencode/` on provision, startup reconcile, and source changes while the manager runs. A per-workspace journal permits safe removal of previously managed source entries. The shared source must not contain generated top-level state (`.gitignore`, package manifests/lockfiles, or `node_modules`); those are preserved per workspace. Existing legacy root templates migrate only when `opencode/` is absent or empty. - Opt-in `selfImprovement.enabled` creates an OpenCode-only internal instance under `/internal/self-improvement`, outside `Registry.List`. `ocm improve` or dashboard `i` launches a fresh local OpenCode TUI with an analysis prompt. It mounts the entire manager configuration read-write, ordinary workspace homes read-only, and named `selfImprovement.directories` roots. Its port still participates in allocation. Agents/commands/helpers are manager-owned; personal `self-improvement/AGENTS.md` extends or replaces the built-in protocol. `sessions.ts` enforces a persistent initial baseline, incremental chunk budgets and checkpoints; `memory.ts` retains cumulative evidence and decisions and applies accepted proposals with hash checks/backups, independently of Git. See `docs/self-improvement.md`; regressions run with `bun test internal/workspace/improvement_sessions.test.ts` (also part of `npm test`). diff --git a/internal/workspace/modulemounts_test.go b/internal/workspace/modulemounts_test.go index 27e88a4..63a0e45 100644 --- a/internal/workspace/modulemounts_test.go +++ b/internal/workspace/modulemounts_test.go @@ -2,11 +2,14 @@ package workspace import ( "context" + "errors" "os" "path/filepath" + "strings" "testing" "github.com/mickael-menu/opencode-manager/internal/config" + "github.com/mickael-menu/opencode-manager/internal/runtime" ) // writeMountingModule creates a module in /tools/ declaring mounts. @@ -126,3 +129,94 @@ func TestAddMountingModuleRecordsBeforeInstall(t *testing.T) { t.Fatal("install script never ran") } } + +// mountDriver keeps the spec of the container it last created, reports it back +// as the running container's config, and fails the scripts matching failExec. +type mountDriver struct { + *fakeDriver + current *runtime.ContainerSpec + created int + failExec string +} + +func (d *mountDriver) ContainerStatus(context.Context, string) (string, error) { + if d.current == nil { + return runtime.StatusMissing, nil + } + return runtime.StatusRunning, nil +} +func (d *mountDriver) ContainerRuntimeConfig(context.Context, string) (runtime.ContainerRuntimeConfig, error) { + if d.current == nil { + return runtime.ContainerRuntimeConfig{}, errors.New("no container") + } + return runtime.ContainerRuntimeConfig{Env: d.current.Env}, nil +} +func (d *mountDriver) CreateContainer(_ context.Context, spec runtime.ContainerSpec) error { + d.current = &spec + d.created++ + return nil +} +func (d *mountDriver) RemoveContainer(context.Context, string) error { d.current = nil; return nil } +func (d *mountDriver) Exec(ctx context.Context, spec runtime.ExecSpec) ([]byte, error) { + if d.failExec != "" && contains(spec.Args, d.failExec) { + return nil, errors.New("install failed") + } + return d.fakeDriver.Exec(ctx, spec) +} + +func TestAddMountingModuleFailedInstallRemovesMount(t *testing.T) { + modules := t.TempDir() + creds := filepath.Join(t.TempDir(), "credentials.json") + writeTestFile(t, creds, []byte("{}")) + writeMountingModule(t, modules, "claude-auth", " - { source: "+creds+", target: /home/debian/.claude/.credentials.json }\n") + + workspacePath := t.TempDir() + home := filepath.Join(workspacePath, "home") + if err := os.MkdirAll(filepath.Join(home, ".config", "opencode"), 0o755); err != nil { + t.Fatal(err) + } + manifest := Manifest{Name: "demo", Runtime: "docker", ImageName: "img", ContainerName: "c", HomeDir: home, OpenCodePort: 4100} + manifestPath := filepath.Join(workspacePath, ManifestFile) + if err := SaveManifest(manifestPath, manifest); err != nil { + t.Fatal(err) + } + + d := &mountDriver{ + fakeDriver: &fakeDriver{output: func([]string) []byte { return nil }}, + failExec: "/opt/opencode-manager/modules/tools/claude-auth/install", + } + l := Lifecycle{cfg: config.Config{ModuleDirs: []string{modules}}, driver: d} + summary := Summary{Manifest: manifest, Path: workspacePath} + if err := l.ensureStarted(context.Background(), summary, false); err != nil { + t.Fatalf("ensureStarted: %v", err) + } + catalog, err := l.Catalog() + if err != nil || len(catalog) != 1 { + t.Fatalf("Catalog: %v %v", catalog, err) + } + + err = l.AddModule(context.Background(), summary, catalog[0], nil) + if err == nil || !strings.Contains(err.Error(), "install failed") { + t.Fatalf("AddModule error = %v, want the install failure", err) + } + // The container was recreated with the mount for the install, then again + // without it after the rollback. + if d.created != 3 { + t.Fatalf("containers created = %d, want 3", d.created) + } + for _, m := range d.current.Mounts { + if m.Source == creds { + t.Fatalf("failed module mount %+v still in the container", m) + } + } + if _, ok := d.current.Env[moduleMountsFingerprintEnv]; ok { + t.Fatal("container still carries the module mounts fingerprint") + } + saved, err := LoadManifest(manifestPath) + if err != nil { + t.Fatal(err) + } + if len(saved.Modules) != 0 { + t.Fatalf("manifest modules after rollback: %+v", saved.Modules) + } +} diff --git a/internal/workspace/modules.go b/internal/workspace/modules.go index 0a1f9e7..1ee512a 100644 --- a/internal/workspace/modules.go +++ b/internal/workspace/modules.go @@ -255,7 +255,8 @@ func (l Lifecycle) AddModule(ctx context.Context, summary Summary, mod module.Mo // before its install script runs, so the module is recorded in the manifest // first and the container recreated with them; the start's reconcile then runs // the install. When the install does not succeed the manifest entry is rolled -// back, and the stray mount goes away on the next start. +// back and the container reconciled with it, so the workspace does not keep a +// mount that no installed module grants. func (l Lifecycle) addMountingModule(ctx context.Context, summary Summary, mod module.Module, values map[string]string) error { id := mod.InstanceID(values) manifestPath := filepath.Join(summary.Path, ManifestFile) @@ -289,6 +290,15 @@ func (l Lifecycle) addMountingModule(ctx context.Context, summary Summary, mod m if err := SaveManifest(manifestPath, current); err != nil { return fmt.Errorf("%w (roll back manifest: %v)", cause, err) } + // The container may already run with the module's mounts; recreate it + // with the restored mount set. Failing that, stop it: the next start + // recreates it from the manifest. + if err := l.ensureStarted(ctx, Summary{Manifest: current, Path: summary.Path}, false); err != nil { + if stopErr := l.driver.StopContainer(ctx, current.ContainerName); stopErr != nil { + return fmt.Errorf("%w (remove module mounts: %v; stop container: %v)", cause, err, stopErr) + } + return fmt.Errorf("%w (remove module mounts: %v; container stopped)", cause, err) + } return cause }