From 219ce7c71c940e21d5fc4fe5ce8fe5e36aad0ebf Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Thu, 15 Oct 2026 13:57:13 -0400 Subject: [PATCH 1/4] fix(invoke): recheck child command exposure (D-0837) --- .github/wiki/Config-Env-Vars.md | 1 + .github/wiki/MCP-Server.md | 2 + cmd/commands/invoke_exposure.go | 69 ++++++++++++++ cmd/commands/invoke_exposure_test.go | 131 ++++++++++++++++++++++++++ cmd/commands/invoke_pin.go | 6 +- internal/invoke/exec.go | 3 +- internal/invoke/transport_env_test.go | 27 ++++++ internal/invoke/types.go | 4 +- 8 files changed, 239 insertions(+), 4 deletions(-) create mode 100644 cmd/commands/invoke_exposure.go create mode 100644 cmd/commands/invoke_exposure_test.go create mode 100644 internal/invoke/transport_env_test.go diff --git a/.github/wiki/Config-Env-Vars.md b/.github/wiki/Config-Env-Vars.md index 06339acfa..95c9daeb4 100644 --- a/.github/wiki/Config-Env-Vars.md +++ b/.github/wiki/Config-Env-Vars.md @@ -110,6 +110,7 @@ These vars control the top-level identity and behavior of a project. | `NSELF_UPDATE_CHECK` | `1` to opt in | unset (off) | No | Process environment only. With `1`, a successful command prints at most one stderr line per 24 h when a newer CLI release exists (cached in `~/.nself/cache/update-check.json`, refreshed in the background from `ping.nself.org/version`). Never in JSON mode, CI or with a non-TTY stderr. See [[Update-Notifications]]. | | `NSELF_MCP_TOKEN` | string | unset | No | Bearer token for the `sse` and `http` transports of `nself mcp`. Wins over `~/.nself/mcp-token` (mode `0600`). With neither, a v1.5 loopback start generates the file and prints only its path; a v1.4 loopback start serves open with one warning. A non-loopback `--host` needs this or the file plus `--allowed-host`. Never passed to commands the server runs. Secret names (`config.IsSecretKey`, see Masking below) are masked in output and redacted in the machine audit log. See [[MCP-Server]]. | | `NSELF_INVOKE_PATH` | string | unset | No | Internal. Set by the machine invoker (MCP, HTTP) on the `nself` child it runs: the command path it checked. The child refuses with `E421` when its own command lookup resolves a different command, and removes the variable before running. Never set it by hand. See [[MCP-Server]]. | +| `NSELF_INVOKE_TRANSPORT` | string | unset | No | Internal. The invoker sets `mcp`, `http`, or `http-ndjson` so the child can recheck exposure against its mounted tree. Removed before execution. Never set it by hand. | | `NSELF_INVOKE_SECRETS` | JSON | unset | No | Internal. Set by the machine invoker (MCP, HTTP) on the `nself` child it runs: a JSON bundle (a nonce and the values) of the secret-marked arguments and flags, so the secrets never appear on the child argv. The child puts the values back before it parses its arguments, then removes the variable. It is ignored unless `NSELF_INVOKED_BY` is set, and a bad or unmatched bundle fails with `E421`. Never set it by hand. See [[MCP-Server]]. | | `IMAGE_PINNING` | `lock` or `legacy` | unset (`legacy` in 1.4.x, `lock` from 1.5.0) | No | How generated compose names images. `legacy` emits the pre-lock references (`redis:7-alpine`) when no version is supplied; `lock` emits `repository:version@sha256:` from the image lock. In lock mode, an empty `*_VERSION`, the lock version, or the frozen `legacy_ref` tag selects the locked image. Any other `*_VERSION` is used as given, unpinned. To run the pre-lock tag itself, use `IMAGE_PINNING=legacy`. Any `IMAGE_PINNING` value other than `lock` or `legacy` is an error. See [[Image-Lock]]. | diff --git a/.github/wiki/MCP-Server.md b/.github/wiki/MCP-Server.md index a4d6a2eb1..2f694ddab 100644 --- a/.github/wiki/MCP-Server.md +++ b/.github/wiki/MCP-Server.md @@ -19,6 +19,8 @@ A machine request names a command from the **machine registry** and gives typed The child refuses to run anything but the command the invoker checked: the invoker sets `NSELF_INVOKE_PATH` to the classified path, and the child resolves its own argv with the same lookup it is about to execute and answers `E421` when the two differ (for example after a plugin upgrade gave a leaf new subcommands, or the binary was replaced, while the server kept its registry from start-up). The variable is removed before the command runs, so nothing the command spawns inherits it. +After the path matches, the child rebuilds the registry from its mounted command tree and rechecks exposure for the transport and named flags. A plugin upgrade that changes only exposure attributes, such as `surface: cli-only` or removal of JSON output, is refused with `E421` before the handler runs. The internal `NSELF_INVOKE_TRANSPORT` marker distinguishes HTTP document requests from NDJSON streams and is removed before execution. + Running as a child keeps global state, `os.Exit` and the project operation lock identical to a typed command. There is no shell anywhere: every request value is exactly one argv element. The **machine registry** is the v1.5 registry of the running binary with installed plugin commands mounted. It is the same in both compat modes ([[Compat-V15]]), so tool names and routes are the v1.5 shape in 1.4.x too. The plugin set is read when the server starts; install a plugin, then restart the server. diff --git a/cmd/commands/invoke_exposure.go b/cmd/commands/invoke_exposure.go new file mode 100644 index 000000000..0af283398 --- /dev/null +++ b/cmd/commands/invoke_exposure.go @@ -0,0 +1,69 @@ +package commands + +// Purpose: recheck machine exposure against the child's freshly mounted tree. +// Inputs: the prepared root, pinned command path, argv and invocation transport. +// Outputs: nil when exposed, otherwise E421 before any command handler runs. +// Constraints: no cached parent registry, tree mutation or plugin execution. +// SPORT: CLI-CMD-MCP-001 + +import ( + "os" + "strings" + + "github.com/nself-org/cli/internal/cmdregistry" + "github.com/nself-org/cli/internal/errs" + "github.com/nself-org/cli/internal/invoke" + "github.com/spf13/cobra" +) + +// checkInvokeExposure checks the same rules as the parent using the actual +// child tree, including installed-plugin attributes and named flag overrides. +func checkInvokeExposure(root *cobra.Command, argv []string, path string) error { + transport := os.Getenv(invoke.TransportEnv) + _ = os.Unsetenv(invoke.TransportEnv) + by := os.Getenv(invoke.InvokedByEnv) + if by == "" { + return nil + } + if transport == "" { + transport = by // compatibility with invokers predating the transport marker + } + refuse := func(why string) error { + return errs.New("E421", "command "+quoteInvokePath(path)+" is not exposed").WithWhy(why) + } + if transport != invoke.TransportMCP && transport != invoke.TransportHTTP && transport != invoke.TransportHTTPStream { + return refuse("unknown child invocation transport") + } + c, err := canonLoad(true) + if err != nil { + return refuse("child command canon: " + err.Error()) + } + reg, err := cmdregistry.Build(root, c, jsonDataTypes, cmdregistry.BuildOptions{ + V15: true, V15OnlyEnvelope: jsonV15OnlyEnvelope, + }) + if err != nil { + return refuse("child command registry: " + err.Error()) + } + row, _ := reg.Lookup(path) + if ok, why := invoke.Exposure(row, transport, invokeArgvFlags(argv)); !ok { + return refuse("child exposure check: " + why) + } + return nil +} + +// invokeArgvFlags recovers named flags from the invoker's canonical argv: +// --name=value, with positional values following --. Presence counts even +// for --flag=false, exactly as invoke.SetFlags does in the parent. +func invokeArgvFlags(argv []string) map[string]bool { + set := map[string]bool{} + for _, word := range argv { + if word == "--" { + break + } + if strings.HasPrefix(word, "--") { + name, _, _ := strings.Cut(strings.TrimPrefix(word, "--"), "=") + set[name] = true + } + } + return set +} diff --git a/cmd/commands/invoke_exposure_test.go b/cmd/commands/invoke_exposure_test.go new file mode 100644 index 000000000..d31c01ad8 --- /dev/null +++ b/cmd/commands/invoke_exposure_test.go @@ -0,0 +1,131 @@ +package commands + +import ( + "encoding/json" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/nself-org/cli/internal/canon" + "github.com/nself-org/cli/internal/cmdregistry" + "github.com/nself-org/cli/internal/errs" + "github.com/nself-org/cli/internal/invoke" +) + +// TestInvokePinExposureUpgrade keeps the path fixed while the child mounts a +// plugin whose exposure attributes differ from the parent's registry snapshot. +func TestInvokePinExposureUpgrade(t *testing.T) { + cases := []struct { + name, replacement, transport string + allowed bool + }{ + {"unchanged", `"side_effect": "read"`, "mcp", true}, + {"cli only", `"side_effect": "read", "surface": "cli-only"`, "mcp", false}, + {"no envelope", `"side_effect": "read", "json": "none"`, "http", false}, + {"interactive", `"side_effect": "read", "output": "interactive"`, "mcp", false}, + {"document became stream", `"side_effect": "read", "output": "stream"`, "http", false}, + {"stream transport", `"side_effect": "read", "output": "stream"`, "http-ndjson", true}, + {"flag became stream", `"side_effect": "read"`, "http", false}, + {"lost gate", `"side_effect": "destructive", "confirm": null`, "mcp", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dir := mountFixture(t) + path := filepath.Join(dir, "demo", "plugin.json") + raw, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + parentJSON := strings.Replace(string(raw), `"side_effect": "write"`, `"json": "envelope", "output": "document", "side_effect": "read"`, 1) + write := func(s string) { + t.Helper() + if err := os.WriteFile(path, []byte(s), 0644); err != nil { + t.Fatal(err) + } + } + write(parentJSON) + parent, _ := mountRoot(t, dir) + c := &canon.File{SchemaVersion: 1, Verbs: []string{"status"}, Commands: map[string]canon.Entry{}} + reg, err := cmdregistry.Build(parent, c, nil, cmdregistry.BuildOptions{V15: true}) + if err != nil { + t.Fatal(err) + } + row, _ := reg.Lookup("demo sub") + if ok, why := invoke.Exposure(row, invoke.TransportMCP, nil); !ok { + t.Fatalf("parent fixture must be exposed: %s", why) + } + childJSON := strings.Replace(parentJSON, `"side_effect": "read"`, tc.replacement, 1) + if tc.name == "flag became stream" { + childJSON = strings.Replace(childJSON, `"type": "bool"`, `"type": "bool", "output": "stream"`, 1) + } + // Replace existing fields instead of leaving duplicate JSON keys. + for _, key := range []string{"json", "output"} { + if strings.Contains(tc.replacement, `"`+key+`"`) { + value := map[string]string{"json": "envelope", "output": "document"}[key] + childJSON = strings.Replace(childJSON, `"`+key+`": "`+value+`", `, "", 1) + } + } + if tc.name == "lost gate" { + var manifest map[string]any + if err := json.Unmarshal([]byte(childJSON), &manifest); err != nil { + t.Fatal(err) + } + sub := manifest["commands"].(map[string]any)["subcommands"].([]any)[0].(map[string]any) + delete(sub, "confirm") + b, err := json.Marshal(manifest) + if err != nil { + t.Fatal(err) + } + childJSON = string(b) + } + write(childJSON) + child, _ := mountRoot(t, dir) + oldLoad := canonLoad + oldTypes, oldEnvelopes := jsonDataTypes, jsonV15OnlyEnvelope + jsonDataTypes, jsonV15OnlyEnvelope = nil, nil + canonLoad = func(bool) (*canon.File, error) { return c, nil } + t.Cleanup(func() { + canonLoad = oldLoad + jsonDataTypes, jsonV15OnlyEnvelope = oldTypes, oldEnvelopes + }) + t.Setenv(invoke.PathEnv, "demo sub") + t.Setenv(invoke.InvokedByEnv, strings.Split(tc.transport, "-")[0]) + t.Setenv("NSELF_INVOKE_TRANSPORT", tc.transport) + err = checkInvokePin(child, []string{"demo", "sub", "--json", "--yes=false"}) + if _, exists := os.LookupEnv(invoke.TransportEnv); exists { + t.Error("child transport marker must not reach grandchildren") + } + if tc.allowed { + if err != nil { + t.Fatal(err) + } + } else { + var coded *errs.CLIError + if !errors.As(err, &coded) || coded.Code != "E421" { + t.Fatalf("child must refuse changed exposure with E421, got %v", err) + } + } + }) + } +} + +// TestInvokePinExposureCore proves the prepared production tree builds a valid +// child registry and still exposes an ordinary core command. +func TestInvokePinExposureCore(t *testing.T) { + if !inFreshProcess(t, "TestInvokePinExposureCore") { + return + } + t.Setenv("HOME", t.TempDir()) + t.Setenv("NSELF_V15", "1") + t.Setenv(invoke.PathEnv, "status") + t.Setenv(invoke.InvokedByEnv, "mcp") + t.Setenv(invoke.TransportEnv, invoke.TransportMCP) + defer PrepareTreeForGeneration()() + installInvocationDecorator(RootCmd) + if err := checkInvokePin(RootCmd, []string{"status", "--json"}); err != nil { + t.Fatal(err) + } + t.Log("CHECKED-TestInvokePinExposureCore") +} diff --git a/cmd/commands/invoke_pin.go b/cmd/commands/invoke_pin.go index 1bcd5e945..8b5cbf281 100644 --- a/cmd/commands/invoke_pin.go +++ b/cmd/commands/invoke_pin.go @@ -9,8 +9,10 @@ package commands // refuses (E421) when the two differ. This closes the bare-flag lookup // shift on flag-less plugin nodes and a registry snapshot that went stale // (plugin upgrade or binary swap) after the server started. +// A matching path is also checked against the child's exposure attributes +// before execution (invoke_exposure.go). // Inputs: NSELF_INVOKE_PATH, the prepared root command and its argv. -// Outputs: nil, or an E421 error naming both paths. +// Outputs: nil, or an E421 error naming a path mismatch or exposure refusal. // Constraints: the variable is removed from this process's environment before // the check, so commands this child spawns never inherit the pin. Unset // means no pin (every non-machine invocation). @@ -38,7 +40,7 @@ func checkInvokePin(root *cobra.Command, argv []string) error { got = strings.TrimSpace(strings.TrimPrefix(c.CommandPath(), root.Name())) } if want = strings.TrimSpace(want); want != "" && got == want { - return nil // a blank pin never matches: the root is never a classified command + return checkInvokeExposure(root, argv, got) } return errs.New("E421", "command "+quoteInvokePath(want)+" is not exposed"). WithWhy("the child resolved its arguments to " + quoteInvokePath(got) + ", not the command the invoker checked; restart the server after a plugin or binary change") diff --git a/internal/invoke/exec.go b/internal/invoke/exec.go index e44e17064..5043b82d4 100644 --- a/internal/invoke/exec.go +++ b/internal/invoke/exec.go @@ -53,7 +53,7 @@ func SelfExecutable() (string, error) { var ownedEnv = map[string]bool{ TokenEnv: true, "NSELF_V15": true, "NSELF_NONINTERACTIVE": true, "CI": true, "NO_COLOR": true, InvokedByEnv: true, DeadlineEnv: true, PathEnv: true, - SecretsEnv: true, + SecretsEnv: true, TransportEnv: true, } // envCaseInsensitive is true where environment names compare case-insensitively @@ -88,6 +88,7 @@ func childEnv(parent []string, transport, path string, deadline time.Duration) ( } } env = append(env, "NSELF_V15=1", "NSELF_NONINTERACTIVE=1", "CI=1", "NO_COLOR=1", InvokedByEnv+"="+invokedBy) + env = append(env, TransportEnv+"="+transport) if path != "" { env = append(env, PathEnv+"="+path) } diff --git a/internal/invoke/transport_env_test.go b/internal/invoke/transport_env_test.go new file mode 100644 index 000000000..7eda59f86 --- /dev/null +++ b/internal/invoke/transport_env_test.go @@ -0,0 +1,27 @@ +package invoke + +import ( + "strings" + "testing" +) + +// TestChildTransportEnv proves the exact exposure transport survives re-exec +// and an inherited value cannot turn a document request into a stream request. +func TestChildTransportEnv(t *testing.T) { + const key = "NSELF_INVOKE_TRANSPORT" + for _, transport := range []string{TransportMCP, TransportHTTP, TransportHTTPStream} { + env, err := childEnv([]string{key + "=attacker"}, transport, "demo sub", 0) + if err != nil { + t.Fatal(err) + } + var values []string + for _, kv := range env { + if strings.HasPrefix(kv, key+"=") { + values = append(values, strings.TrimPrefix(kv, key+"=")) + } + } + if len(values) != 1 || values[0] != transport { + t.Errorf("%s: transport values = %v", transport, values) + } + } +} diff --git a/internal/invoke/types.go b/internal/invoke/types.go index ea8664151..83ea9f2ed 100644 --- a/internal/invoke/types.go +++ b/internal/invoke/types.go @@ -41,7 +41,9 @@ const ( SelfExecOverrideEnv = "NSELF_MCP_EXEC_OVERRIDE" TokenEnv = "NSELF_MCP_TOKEN" InvokedByEnv = "NSELF_INVOKED_BY" - DeadlineEnv = "NSELF_INVOKE_DEADLINE_MS" + // TransportEnv preserves HTTP document versus NDJSON for child exposure. + TransportEnv = "NSELF_INVOKE_TRANSPORT" + DeadlineEnv = "NSELF_INVOKE_DEADLINE_MS" // PathEnv pins the command path the invoker classified; the child refuses // to run when its own command lookup resolves a different command (E421). PathEnv = "NSELF_INVOKE_PATH" From 9c0ddb06cd10f4fed2ba9f226ae2a0743a076b8d Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Thu, 15 Oct 2026 11:58:01 -0400 Subject: [PATCH 2/4] fix(config): honor explain flags before monorepo redirect (D-0445) --- .github/wiki/Configuration-Precedence.md | 2 + cmd/commands/config_explain_monorepo_test.go | 79 ++++++++++++++++++++ cmd/commands/root.go | 7 ++ 3 files changed, 88 insertions(+) create mode 100644 cmd/commands/config_explain_monorepo_test.go diff --git a/.github/wiki/Configuration-Precedence.md b/.github/wiki/Configuration-Precedence.md index 8aa64c965..3664278e9 100644 --- a/.github/wiki/Configuration-Precedence.md +++ b/.github/wiki/Configuration-Precedence.md @@ -31,6 +31,8 @@ nself config explain BASE_DOMAIN --json # one envelope For example, the command `init` with the flag `--domain` resolves to `BASE_DOMAIN`. +From a monorepo root, `config explain` follows the detected `backend/` or `.backend/` project. Use `nself config explain BASE_DOMAIN --no-monorepo --json` to explain the current directory instead. With `--json`, any redirect notice goes to stderr so stdout remains one JSON document. + The answer lists the env name, the default, the flags that supply the key, the MCP tool and HTTP route parameter that carry each flag, the `nself.yaml` row, every cascade file that sets the key and the winner. A key that is not a known configuration key, and a flag that supplies no key, exit 1 with `E434`. `nself config env explain VAR` walks the same cascade and prints the same winner. ## Flags that supply a key diff --git a/cmd/commands/config_explain_monorepo_test.go b/cmd/commands/config_explain_monorepo_test.go new file mode 100644 index 000000000..f64effc85 --- /dev/null +++ b/cmd/commands/config_explain_monorepo_test.go @@ -0,0 +1,79 @@ +package commands + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/nself-org/cli/internal/output" +) + +// TestConfigExplainMonorepoExecution exercises Cobra's full hook order against +// distinct root/backend values. Raw flags must take effect before any redirect, +// and the redirect notice must never precede the JSON document on stdout. +func TestConfigExplainMonorepoExecution(t *testing.T) { + for _, tc := range []struct { + name string + flags []string + want string + redirect bool + }{ + {"disabled", []string{"--no-monorepo"}, "root.test", false}, + {"default", nil, "backend.test", true}, + {"explicit false", []string{"--no-monorepo=false"}, "backend.test", true}, + {"last value wins", []string{"--no-monorepo", "--no-monorepo=false"}, "backend.test", true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("OTEL_EXPORTER_OTLP_ENDPOINT", "") + t.Setenv("ENV", "dev") + t.Setenv("NSELF_LEGACY_ENV_ORDER", "") + root := envExplainProject(t, map[string]string{".env": "BASE_DOMAIN=root.test\n"}) + backend := filepath.Join(root, "backend") + if err := os.Mkdir(backend, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(backend, ".env"), []byte("BASE_DOMAIN=backend.test\n"), 0o600); err != nil { + t.Fatal(err) + } + for _, name := range []string{"json", "no-monorepo"} { + flag := RootCmd.PersistentFlags().Lookup(name) + value, changed := flag.Value.String(), flag.Changed + if err := flag.Value.Set("false"); err != nil { + t.Fatal(err) + } + flag.Changed = false + t.Cleanup(func() { _ = flag.Value.Set(value); flag.Changed = changed }) + } + oldWriter := pilotWriter + pilotWriter = func() output.Writer { return output.Writer{Out: os.Stdout, Err: os.Stderr} } + output.ResetState() + t.Cleanup(func() { pilotWriter = oldWriter; output.ResetState(); RootCmd.SetArgs(nil) }) + args := append([]string{"config", "explain", "BASE_DOMAIN", "--json", "--reveal"}, tc.flags...) + RootCmd.SetArgs(args) + var stdout string + var runErr error + stderr := captureStderr(t, func() { + stdout, runErr = captureStdout(t, RootCmd.Execute) + }) + if runErr != nil { + t.Fatal(runErr) + } + data := singleDoc(t, stdout)["data"].(map[string]any) + if got := data["effective"].(map[string]any)["value"]; got != tc.want { + t.Errorf("effective value = %v, want %s", got, tc.want) + } + if got := strings.Contains(stderr, "Detected monorepo layout"); got != tc.redirect { + t.Errorf("redirect notice = %v, want %v: %s", got, tc.redirect, stderr) + } + wantDir := root + if tc.redirect { + wantDir = backend + } + if cwd, err := os.Getwd(); err != nil || cwd != wantDir { + t.Errorf("cwd = %q (%v), want %q", cwd, err, wantDir) + } + }) + } +} diff --git a/cmd/commands/root.go b/cmd/commands/root.go index 11df98c0e..2eaaf28a4 100644 --- a/cmd/commands/root.go +++ b/cmd/commands/root.go @@ -89,6 +89,13 @@ func init() { // (invocation.go) refuses it on commands that cannot produce JSON. RootCmd.PersistentFlags().Bool("json", false, "Machine-readable output: one JSON document on stdout (see JSON-Output)") RootCmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error { + // Explain keeps target flags as data. Apply its global options before + // root hooks inspect them, especially the monorepo redirect below. + if cmd == configExplainCmd && cmd.DisableFlagParsing { + if err := prepareConfigExplain(cmd, args); err != nil { + return err + } + } // ── OTel tracing ────────────────────────────────────────────────────── // InitTracer is only called when OTEL_EXPORTER_OTLP_ENDPOINT is set. // Leaving it unset is the zero-config path: no tracer, no side-effects. From 9367aea628cffa5fbedfedebf4b648e3fd11dc5c Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Thu, 15 Oct 2026 13:56:08 -0400 Subject: [PATCH 3/4] fix(db): remove previously absent env keys on image rollback (D-0804) --- .github/wiki/pgvector.md | 2 +- cmd/commands/config_set_restore.go | 8 ++ cmd/commands/db_image.go | 6 +- cmd/commands/db_image_deps.go | 43 +++++++-- cmd/commands/db_image_env_remove.go | 26 ++++++ cmd/commands/db_image_presence_test.go | 88 +++++++++++++++++++ .../postgres/imageswitch/env_presence_test.go | 27 ++++++ internal/postgres/imageswitch/run.go | 2 +- internal/postgres/imageswitch/state.go | 3 +- internal/postgres/imageswitch/types.go | 6 +- internal/reconcile/apply.go | 6 +- internal/reconcile/overlay.go | 24 ++++- internal/reconcile/overlay_envremove.go | 54 ++++++++++++ internal/reconcile/overlay_envremove_test.go | 40 +++++++++ internal/reconcile/planner.go | 4 +- internal/reconcile/request.go | 2 + 16 files changed, 317 insertions(+), 24 deletions(-) create mode 100644 cmd/commands/db_image_env_remove.go create mode 100644 cmd/commands/db_image_presence_test.go create mode 100644 internal/postgres/imageswitch/env_presence_test.go create mode 100644 internal/reconcile/overlay_envremove.go create mode 100644 internal/reconcile/overlay_envremove_test.go diff --git a/.github/wiki/pgvector.md b/.github/wiki/pgvector.md index f38e7bcca..4dccbb762 100644 --- a/.github/wiki/pgvector.md +++ b/.github/wiki/pgvector.md @@ -76,7 +76,7 @@ nself db image switch --rollback # undo; from any phase nself db image switch --rollback --yes # undo after a finished switch ``` -`--rollback` writes the previous env values back (the two keys are left set to an empty value), regenerates compose and the lock, starts postgres on the old volume, resets the read-only setting and starts the stack. After a finished switch it needs `--yes`, because rows written since the switch exist only in the new volume. That volume is kept and named; it is never removed for you. +`--rollback` restores the previous env values, regenerates compose and the lock, starts postgres on the old volume, resets the read-only setting and starts the stack. Previously absent keys are removed; explicitly empty values are preserved. States recorded by older versions retain their recorded empty assignments. After a finished switch it needs `--yes`, because rows written since the switch exist only in the new volume. That volume is kept and named; it is never removed for you. A second `nself db image switch --to pgvector` on a project that already runs a pgvector image prints that and changes nothing. diff --git a/cmd/commands/config_set_restore.go b/cmd/commands/config_set_restore.go index 703f4cd5d..7bba15bf6 100644 --- a/cmd/commands/config_set_restore.go +++ b/cmd/commands/config_set_restore.go @@ -40,6 +40,11 @@ type envLineWrite struct { // write holds interrupts, records the current bytes, then writes the line. func (w *envLineWrite) write(key, value string) error { + return w.writeEnvPresence(key, value, true) +} + +// writeEnvPresence records the file before setting or removing one dotenv statement. +func (w *envLineWrite) writeEnvPresence(key, value string, present bool) error { w.holdSignals() data, err := os.ReadFile(w.path) switch { @@ -52,6 +57,9 @@ func (w *envLineWrite) write(key, value string) error { w.danglingTarget = danglingLinkTarget(w.path) } w.written = true // set before the write: a partial write is restored too + if !present { + return w.removeEnvLine(key) + } w.updated, err = setEnvFileLine(w.path, key, value) return err } diff --git a/cmd/commands/db_image.go b/cmd/commands/db_image.go index 66432a791..4d105f831 100644 --- a/cmd/commands/db_image.go +++ b/cmd/commands/db_image.go @@ -83,7 +83,11 @@ func runDBImageSwitch(cmd *cobra.Command, _ []string) error { } ctx, stop := signal.NotifyContext(cmdContext(cmd), os.Interrupt, syscall.SIGTERM, syscall.SIGHUP) defer stop() - sw := imageswitch.New(imageSwitchConfig(dir, cfg), imageSwitchDepsFor(cmd, dir, asJSON)) + switchConfig, err := imageSwitchConfig(dir, cfg) + if err != nil { + return err + } + sw := imageswitch.New(switchConfig, imageSwitchDepsFor(cmd, dir, asJSON)) var rep *imageswitch.Report switch { case plan: diff --git a/cmd/commands/db_image_deps.go b/cmd/commands/db_image_deps.go index cbc23e8b0..2c3fec5a5 100644 --- a/cmd/commands/db_image_deps.go +++ b/cmd/commands/db_image_deps.go @@ -15,6 +15,7 @@ import ( "context" "fmt" "io" + "os" "path/filepath" "sort" "strconv" @@ -31,12 +32,22 @@ import ( ) // imageSwitchConfig builds the engine's Config from the loaded project config. -func imageSwitchConfig(dir string, cfg *config.Config) imageswitch.Config { +func imageSwitchConfig(dir string, cfg *config.Config) (imageswitch.Config, error) { + previous, err := config.ReadEnvFile(filepath.Join(dir, ".env")) + if err != nil && !os.IsNotExist(err) { + return imageswitch.Config{}, config.EnvReadError(filepath.Join(dir, ".env"), err) + } + prev := make(map[string]string) + for _, key := range []string{imageswitch.EnvImage, imageswitch.EnvDataVolume} { + if value, present := previous[key]; present { + prev[key] = value + } + } return imageswitch.Config{ ProjectDir: dir, ProjectName: cfg.ProjectName, Service: "postgres", Container: cfg.ProjectName + "_postgres", User: cfg.Postgres.User, Password: cfg.Postgres.Password, TargetFor: pgvectorImageForMajor, - PrevEnv: map[string]string{imageswitch.EnvImage: cfg.Postgres.Image, imageswitch.EnvDataVolume: cfg.Postgres.DataVolume}, - } + PrevEnv: prev, + }, nil } // pgvectorImageForMajor is the pgvector image compose would run for a Postgres major. @@ -138,10 +149,17 @@ type envApplier struct { } func (a *envApplier) request(env map[string]string) reconcile.Request { + var remove []string + for _, key := range []string{imageswitch.EnvImage, imageswitch.EnvDataVolume} { + if _, present := env[key]; !present { + remove = append(remove, key) + } + } return reconcile.Request{ ProjectDir: a.dir, Command: reconcile.CmdConfigSet, Trigger: reconcile.Trigger{Kind: reconcile.TriggerConfig, Subject: imageswitch.EnvDataVolume}, EnvOverlay: map[string]map[string]string{".env": env}, Stderr: a.stderr, DiffOut: a.diff, + EnvRemove: map[string][]string{".env": remove}, } } @@ -160,12 +178,18 @@ func (a *envApplier) Plan(ctx context.Context, env map[string]string) (any, erro // Apply writes the lines and regenerates; on failure the env file is restored. func (a *envApplier) Apply(ctx context.Context, env map[string]string) error { - envFile := filepath.Join(a.dir, ".env") - keys := make([]string, 0, len(env)) - for k := range env { - keys = append(keys, k) + defer build.SnapshotEnv()() + // config.Load exports the switched values. Removed lines must not inherit + // those stale process values when reconcile renders the restored cascade. + for _, key := range []string{imageswitch.EnvImage, imageswitch.EnvDataVolume} { + if _, present := env[key]; !present { + if err := os.Unsetenv(key); err != nil { + return err + } + } } - sort.Strings(keys) + envFile := filepath.Join(a.dir, ".env") + keys := []string{imageswitch.EnvImage, imageswitch.EnvDataVolume} var writes []*envLineWrite defer func() { for _, w := range writes { @@ -178,7 +202,8 @@ func (a *envApplier) Apply(ctx context.Context, env map[string]string) error { for _, k := range keys { w := &envLineWrite{path: envFile} writes = append(writes, w) - if err := w.write(k, env[k]); err != nil { + value, present := env[k] + if err := w.writeEnvPresence(k, value, present); err != nil { return err } } diff --git a/cmd/commands/db_image_env_remove.go b/cmd/commands/db_image_env_remove.go new file mode 100644 index 000000000..82414aedd --- /dev/null +++ b/cmd/commands/db_image_env_remove.go @@ -0,0 +1,26 @@ +package commands + +// Purpose: remove rollback-only dotenv keys using the planner's statement grammar. +// Inputs: the captured envLineWrite and key. Outputs: a private env file or error. +// Constraints: the caller holds interrupts and restores captured bytes on failure. + +import ( + "os" + + "github.com/nself-org/cli/internal/reconcile" + "github.com/nself-org/cli/internal/security" +) + +func (w *envLineWrite) removeEnvLine(key string) error { + if !w.existed { + return nil + } + data, err := reconcile.EnvFileWithoutLine(w.prev, key) + if err != nil { + return err + } + if err := os.WriteFile(w.path, data, 0o600); err != nil { + return err + } + return security.EnforceFilePermissions(w.path, 0o600) +} diff --git a/cmd/commands/db_image_presence_test.go b/cmd/commands/db_image_presence_test.go new file mode 100644 index 000000000..fc9d004b6 --- /dev/null +++ b/cmd/commands/db_image_presence_test.go @@ -0,0 +1,88 @@ +package commands + +import ( + "bytes" + "context" + "io" + "os" + "path/filepath" + "testing" + + "github.com/nself-org/cli/internal/config" + "github.com/nself-org/cli/internal/postgres/imageswitch" +) + +func TestImageSwitchConfigPreservesAbsentEnvKeys(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, ".env"), []byte("PROJECT_NAME=p\n"), 0o600); err != nil { + t.Fatal(err) + } + cfg, err := imageSwitchConfig(dir, &config.Config{Postgres: config.PostgresConfig{Image: "inherited:16", DataVolume: "inherited_data"}}) + if err != nil { + t.Fatal(err) + } + for _, key := range []string{imageswitch.EnvImage, imageswitch.EnvDataVolume} { + if _, present := cfg.PrevEnv[key]; present { + t.Errorf("absent %s was captured as an assignment; rollback must remove it", key) + } + } +} + +func TestImageSwitchEnvApplierRestoresPresence(t *testing.T) { + for _, previous := range []string{"", "POSTGRES_IMAGE=\"\"\n", "POSTGRES_IMAGE=postgres:16-alpine\nPOSTGRES_DATA_VOLUME=original_data\n"} { + t.Run(previous, func(t *testing.T) { + restoreProcessEnv(t) + for _, key := range []string{imageswitch.EnvImage, imageswitch.EnvDataVolume} { + if err := os.Unsetenv(key); err != nil { + t.Fatal(err) + } + } + dir := makeNSelfProject(t, "PROJECT_NAME=p\nENV=dev\nBASE_DOMAIN=example.test\nSSL_MODE=none\nMINIO_ENABLED=false\n"+previous) + cfg, err := imageSwitchConfig(dir, &config.Config{}) + if err != nil { + t.Fatal(err) + } + ap := &envApplier{dir: dir, stderr: io.Discard} + if err := ap.Apply(context.Background(), cfg.PrevEnv); err != nil { + t.Fatal(err) + } + baselineEnv, err := os.ReadFile(filepath.Join(dir, ".env")) + if err != nil { + t.Fatal(err) + } + baseline, err := os.ReadFile(filepath.Join(dir, "docker-compose.yml")) + if err != nil { + t.Fatal(err) + } + if err := ap.Apply(context.Background(), map[string]string{imageswitch.EnvImage: "pgvector/pgvector:pg16", imageswitch.EnvDataVolume: "new_data"}); err != nil { + t.Fatal(err) + } + // A separate rollback invocation first loads the switched config. + if _, err := config.Load(dir); err != nil { + t.Fatal(err) + } + if err := ap.Apply(context.Background(), cfg.PrevEnv); err != nil { + t.Fatal(err) + } + restored, err := os.ReadFile(filepath.Join(dir, "docker-compose.yml")) + if err != nil || !bytes.Equal(restored, baseline) { + t.Fatalf("rollback did not regenerate the original compose: %v", err) + } + restoredEnv, err := os.ReadFile(filepath.Join(dir, ".env")) + if err != nil || !bytes.Equal(restoredEnv, baselineEnv) { + t.Fatalf("rollback did not restore the original env bytes: %v", err) + } + got, err := config.ReadEnvFile(filepath.Join(dir, ".env")) + if err != nil { + t.Fatal(err) + } + for _, key := range []string{imageswitch.EnvImage, imageswitch.EnvDataVolume} { + value, present := got[key] + want, had := cfg.PrevEnv[key] + if value != want || present != had { + t.Errorf("%s = %q present=%v, want %q present=%v", key, value, present, want, had) + } + } + }) + } +} diff --git a/internal/postgres/imageswitch/env_presence_test.go b/internal/postgres/imageswitch/env_presence_test.go new file mode 100644 index 000000000..3613459c5 --- /dev/null +++ b/internal/postgres/imageswitch/env_presence_test.go @@ -0,0 +1,27 @@ +package imageswitch + +import ( + "context" + "testing" +) + +func TestRollbackPersistsAbsentAndExplicitEmptyEnv(t *testing.T) { + _, s := newFake(t) + s.cfg.PrevEnv = map[string]string{EnvImage: ""} + if _, err := s.Run(context.Background(), RunOptions{}); err != nil { + t.Fatal(err) + } + st, err := LoadState(s.cfg.ProjectDir) + if err != nil { + t.Fatal(err) + } + if value, present := st.PrevEnv[EnvImage]; !present || value != "" { + t.Fatalf("explicit empty image was lost: %v", st.PrevEnv) + } + if _, present := st.PrevEnv[EnvDataVolume]; present { + t.Fatalf("absent volume became an empty assignment: %v", st.PrevEnv) + } + if _, err := New(s.cfg, s.d).Rollback(context.Background(), true); err != nil { + t.Fatal(err) + } +} diff --git a/internal/postgres/imageswitch/run.go b/internal/postgres/imageswitch/run.go index 8c90a2a14..8054dfee0 100644 --- a/internal/postgres/imageswitch/run.go +++ b/internal/postgres/imageswitch/run.go @@ -103,7 +103,7 @@ func (s *Switch) newState(r *preflightResult) (*State, error) { if project == "" { project = s.cfg.ProjectName } - prev := map[string]string{EnvImage: "", EnvDataVolume: ""} + prev := make(map[string]string) for k, v := range s.cfg.PrevEnv { prev[k] = v } diff --git a/internal/postgres/imageswitch/state.go b/internal/postgres/imageswitch/state.go index 8592be86f..4d47b47ba 100644 --- a/internal/postgres/imageswitch/state.go +++ b/internal/postgres/imageswitch/state.go @@ -43,7 +43,8 @@ type State struct { StoppedServices []string `json:"stopped_services"` // NewVolumeCreated is true once this switch created the new volume (it may remove it). NewVolumeCreated bool `json:"new_volume_created"` - // PrevEnv is the raw value of each env key the switch writes, before the switch. + // PrevEnv holds previous .env values; missing keys must be removed on rollback. + // Legacy states contain both keys, including explicit empty values. PrevEnv map[string]string `json:"prev_env"` Tolerated []string `json:"tolerated,omitempty"` Tables int `json:"tables,omitempty"` diff --git a/internal/postgres/imageswitch/types.go b/internal/postgres/imageswitch/types.go index 24a55ebfe..6612bc785 100644 --- a/internal/postgres/imageswitch/types.go +++ b/internal/postgres/imageswitch/types.go @@ -76,8 +76,8 @@ type Config struct { // TargetFor returns the pgvector image of the given Postgres major, or an // error when there is none. TargetFor func(major int) (string, error) - // PrevEnv holds the current raw values of EnvImage and EnvDataVolume in the - // project (empty when unset); rollback writes them back. + // PrevEnv holds the current values in the project's .env. Missing keys + // were absent; rollback removes them. Present empty values stay explicit. PrevEnv map[string]string } @@ -133,7 +133,7 @@ type Stack interface { } // Applier writes env lines and regenerates compose and lock through the build -// plan path (reconcile). A value of "" means the key is set empty (unset). +// plan path (reconcile). Missing switch keys are removed; "" sets an explicit empty value. type Applier interface { // Plan returns what Apply would change, as JSON-ready data; nothing is written. Plan(ctx context.Context, env map[string]string) (any, error) diff --git a/internal/reconcile/apply.go b/internal/reconcile/apply.go index 474149f34..c37867c50 100644 --- a/internal/reconcile/apply.go +++ b/internal/reconcile/apply.go @@ -65,11 +65,11 @@ func ApplyBuild(ctx context.Context, req Request, opt ApplyOptions) (applied *Pl defer rollbackOnFailure(&opt, &err)() // The env a pending line changes is resolved once, before anything is // written: every render of this apply judges confirmation against it. - if len(req.EnvOverlay) > 0 { + if len(req.EnvOverlay) > 0 || len(req.EnvRemove) > 0 { if req.EnvBefore == "" { req.EnvBefore = diskEnv(req.ProjectDir) } - if _, req.envChanged, err = pendingEnv(req.ProjectDir, req.EnvOverlay); err != nil { + if _, req.envChanged, err = pendingEnv(req.ProjectDir, req.EnvOverlay, req.EnvRemove); err != nil { return nil, nil, err } req.envChangedSet = true @@ -87,7 +87,7 @@ func ApplyBuild(ctx context.Context, req Request, opt ApplyOptions) (applied *Pl if req.HandEdited == nil { req.HandEdited = state.HandEditedFn(req.ProjectDir) } - env, _, err := pendingEnv(req.ProjectDir, req.EnvOverlay) + env, _, err := pendingEnv(req.ProjectDir, req.EnvOverlay, req.EnvRemove) if err != nil { return nil, nil, err } diff --git a/internal/reconcile/overlay.go b/internal/reconcile/overlay.go index 4cd463d67..ef2f05524 100644 --- a/internal/reconcile/overlay.go +++ b/internal/reconcile/overlay.go @@ -102,13 +102,26 @@ const pendingEnvPerm fs.FileMode = 0o600 // file's bytes differ from the disk (or the file does not exist yet). It reads the disk each time, // so a plan recomputed after the line was written sees the same bytes // (EnvFileWithLine is idempotent) and an edit made meanwhile is not hidden. -func pendingEnv(dir string, in map[string]map[string]string) (map[string][]byte, bool, error) { - if len(in) == 0 { +func pendingEnv(dir string, in map[string]map[string]string, removals ...map[string][]string) (map[string][]byte, bool, error) { + remove := map[string][]string{} + if len(removals) > 0 { + remove = removals[0] + } + files := make(map[string]map[string]string, len(in)+len(remove)) + for name, kv := range in { + files[name] = kv + } + for name := range remove { + if _, ok := files[name]; !ok { + files[name] = nil + } + } + if len(files) == 0 { return nil, false, nil } out := make(map[string][]byte, len(in)) changed := false - for name, kv := range in { + for name, kv := range files { if !validEnvName(name) { return nil, false, fmt.Errorf("env overlay: %q is not an env file at the project root", name) } @@ -118,6 +131,11 @@ func pendingEnv(dir string, in map[string]map[string]string) (map[string][]byte, return nil, false, fmt.Errorf("reading %s for the plan: %w", name, err) } was := string(data) + for _, key := range remove[name] { + if data, err = EnvFileWithoutLine(data, key); err != nil { + return nil, false, fmt.Errorf("scanning %s: %w", name, err) + } + } keys := make([]string, 0, len(kv)) for k := range kv { keys = append(keys, k) diff --git a/internal/reconcile/overlay_envremove.go b/internal/reconcile/overlay_envremove.go new file mode 100644 index 000000000..2d8fbf192 --- /dev/null +++ b/internal/reconcile/overlay_envremove.go @@ -0,0 +1,54 @@ +package reconcile + +// Purpose: plan and apply dotenv removals with the same statement boundaries. +// Inputs: dotenv bytes and a key. Outputs: bytes without that key or a safe error. +// Constraints: quoted multiline values and unrelated statements stay intact. + +import ( + "fmt" + "strings" + + "github.com/nself-org/cli/internal/config" +) + +// EnvFileWithoutLine removes every statement defining key, including export and multiline forms. +func EnvFileWithoutLine(old []byte, key string) ([]byte, error) { + before, err := config.ParseEnvBytes("the env file", old) + if err != nil { + return nil, err + } + if _, present := before[key]; !present { + return old, nil + } + stmts, err := splitEnvStatements(old) + if err != nil { + return nil, err + } + if err := proveStatements(stmts, key); err != nil { + return nil, err + } + var lines []string + for _, st := range stmts { + if st.key != key { + lines = append(lines, st.lines...) + } + } + out := []byte(strings.Join(lines, "\n")) + if len(lines) > 0 { + out = append(out, '\n') + } + after, err := config.ParseEnvBytes("the pending env file", out) + if err != nil { + return nil, err + } + delete(before, key) + if _, present := after[key]; present || len(after) != len(before) { + return nil, fmt.Errorf("cannot remove %s without breaking the env file", key) + } + for other := range before { + if _, present := after[other]; !present { + return nil, fmt.Errorf("cannot remove %s without breaking the env file", key) + } + } + return out, nil +} diff --git a/internal/reconcile/overlay_envremove_test.go b/internal/reconcile/overlay_envremove_test.go new file mode 100644 index 000000000..806acc4cf --- /dev/null +++ b/internal/reconcile/overlay_envremove_test.go @@ -0,0 +1,40 @@ +package reconcile + +import ( + "os" + "path/filepath" + "testing" +) + +func TestEnvFileWithoutLine(t *testing.T) { + const old = "# keep\nOTHER=\"one\nPOSTGRES_IMAGE=inside\ntwo\"\nexport POSTGRES_IMAGE=\"old\nimage\"\nPOSTGRES_IMAGE=duplicate\nEMPTY=\n" + const want = "# keep\nOTHER=\"one\nPOSTGRES_IMAGE=inside\ntwo\"\nEMPTY=\n" + got, err := EnvFileWithoutLine([]byte(old), "POSTGRES_IMAGE") + if err != nil || string(got) != want { + t.Fatalf("got %q, err %v", got, err) + } + again, err := EnvFileWithoutLine(got, "POSTGRES_IMAGE") + if err != nil || string(again) != want { + t.Fatalf("removal not idempotent: %q, %v", again, err) + } +} + +func TestPendingEnvRemovalMatchesDisk(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, ".env") + if err := os.WriteFile(path, []byte("POSTGRES_IMAGE=new\nOTHER=keep\n"), 0o600); err != nil { + t.Fatal(err) + } + remove := map[string][]string{".env": {"POSTGRES_IMAGE"}} + before, changed, err := pendingEnv(dir, nil, remove) + if err != nil || !changed || string(before[".env"]) != "OTHER=keep\n" { + t.Fatalf("pending removal: %q, changed=%v, err=%v", before, changed, err) + } + if err := os.WriteFile(path, before[".env"], 0o600); err != nil { + t.Fatal(err) + } + after, changed, err := pendingEnv(dir, nil, remove) + if err != nil || changed || string(before[".env"]) != string(after[".env"]) { + t.Fatalf("after removal: %q, changed=%v, err=%v", after, changed, err) + } +} diff --git a/internal/reconcile/planner.go b/internal/reconcile/planner.go index f3af96998..a758ce189 100644 --- a/internal/reconcile/planner.go +++ b/internal/reconcile/planner.go @@ -60,7 +60,7 @@ func compute(ctx context.Context, req Request) (*computed, error) { if len(req.Seed) > 0 { opts.Rand = newSeededRand(req.Seed) } - env, envChanged, err := pendingEnv(req.ProjectDir, req.EnvOverlay) + env, envChanged, err := pendingEnv(req.ProjectDir, req.EnvOverlay, req.EnvRemove) if err != nil { return nil, err } @@ -120,7 +120,7 @@ func compute(ctx context.Context, req Request) (*computed, error) { if p.Command == "" { p.Command = CmdBuild } - if len(req.EnvOverlay) > 0 { + if len(req.EnvOverlay) > 0 || len(req.EnvRemove) > 0 { p.PendingEnv = envChanged p.EnvBefore = req.EnvBefore if p.EnvBefore == "" { diff --git a/internal/reconcile/request.go b/internal/reconcile/request.go index 908d2ea94..acf42475d 100644 --- a/internal/reconcile/request.go +++ b/internal/reconcile/request.go @@ -59,6 +59,8 @@ type Request struct { // pendingEnv); nothing is written by Plan or Compute. Apply's write build // reads the disk, so the caller writes the lines in BeforeWrite. EnvOverlay map[string]map[string]string + // EnvRemove names keys to remove from pending env files, before EnvOverlay is set. + EnvRemove map[string][]string // EnvBefore is the project's ENV before the pending lines (empty: resolved // from the disk as config.Load would, when EnvOverlay is set). A prod-class EnvBefore makes a // non-empty plan need confirmation even when the lines change ENV to a From afddfb79e00b0835bee57a7ddb91993e1e8daf83 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Wed, 28 Oct 2026 17:22:40 -0400 Subject: [PATCH 4/4] fix: mount disabled builtin families during child invoke pin/exposure check The child registry build in checkInvokeExposure did not mount disabled builtin families, so pinned MCP/HTTP children whose path lived under a disabled family (e.g. admin) failed E421 even though the parent classified them as exposed. The same gap existed in checkInvokePin where root.Find could not resolve the path at all. Both now reuse mountDisabledBuiltinFamilies for the duration of the check, matching the parent registry-build steps (D-0863). A regression test proves the disabled-builtin-family case. --- cmd/commands/invoke_exposure.go | 7 ++++++ cmd/commands/invoke_exposure_test.go | 34 ++++++++++++++++++++++++++++ cmd/commands/invoke_pin.go | 5 ++++ 3 files changed, 46 insertions(+) diff --git a/cmd/commands/invoke_exposure.go b/cmd/commands/invoke_exposure.go index 0af283398..c5df2edf1 100644 --- a/cmd/commands/invoke_exposure.go +++ b/cmd/commands/invoke_exposure.go @@ -34,6 +34,13 @@ func checkInvokeExposure(root *cobra.Command, argv []string, path string) error if transport != invoke.TransportMCP && transport != invoke.TransportHTTP && transport != invoke.TransportHTTPStream { return refuse("unknown child invocation transport") } + // A disabled builtin family keeps its canon entries and data types, so the + // child registry must be built with every family mounted, exactly as the + // parent does (D-0863). Without this, a pinned MCP/HTTP child whose path + // lives under a disabled builtin family fails E421 even though the parent + // classified it as exposed. + undoFamilies := mountDisabledBuiltinFamilies(root) + defer undoFamilies() c, err := canonLoad(true) if err != nil { return refuse("child command canon: " + err.Error()) diff --git a/cmd/commands/invoke_exposure_test.go b/cmd/commands/invoke_exposure_test.go index d31c01ad8..c7154ed67 100644 --- a/cmd/commands/invoke_exposure_test.go +++ b/cmd/commands/invoke_exposure_test.go @@ -129,3 +129,37 @@ func TestInvokePinExposureCore(t *testing.T) { } t.Log("CHECKED-TestInvokePinExposureCore") } + +// TestInvokeExposureDisabledBuiltinFamily proves the child registry is built +// with disabled builtin families mounted, so a pinned MCP/HTTP child whose +// path lives under a disabled builtin family does not fail E421. Without the +// mount step in checkInvokeExposure, the lookup returns no row and the pin +// refuses even though the parent classified the command as exposed (D-0863). +func TestInvokeExposureDisabledBuiltinFamily(t *testing.T) { + if !inFreshProcess(t, "TestInvokeExposureDisabledBuiltinFamily") { + return + } + t.Setenv("HOME", t.TempDir()) + t.Setenv("NSELF_V15", "1") + // Disable the admin builtin family via its plugin disable marker so the + // runtime tree omits it but the registry must still contain its commands. + pluginDir := filepath.Join(os.Getenv("HOME"), ".nself", "plugins") + if err := os.MkdirAll(filepath.Join(pluginDir, "admin"), 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(pluginDir, "admin", ".disabled"), []byte("review regression"), 0644); err != nil { + t.Fatal(err) + } + t.Setenv(invoke.PathEnv, "admin health") + t.Setenv(invoke.InvokedByEnv, "mcp") + t.Setenv(invoke.TransportEnv, invoke.TransportMCP) + defer PrepareTreeForGeneration()() + installInvocationDecorator(RootCmd) + // The admin subtree is not mounted on RootCmd because of the disable + // marker; checkInvokeExposure must still resolve "admin status" by + // mounting disabled builtin families for the child registry build. + if err := checkInvokePin(RootCmd, []string{"admin", "health"}); err != nil { + t.Fatalf("disabled builtin family must remain exposed to pinned children: %v", err) + } + t.Log("CHECKED-TestInvokeExposureDisabledBuiltinFamily") +} diff --git a/cmd/commands/invoke_pin.go b/cmd/commands/invoke_pin.go index 8b5cbf281..8f77ef252 100644 --- a/cmd/commands/invoke_pin.go +++ b/cmd/commands/invoke_pin.go @@ -35,6 +35,11 @@ func checkInvokePin(root *cobra.Command, argv []string) error { return nil } _ = os.Unsetenv(invoke.PathEnv) + // A disabled builtin family keeps its canon entries, so the child tree + // must include it for the duration of the pin check; otherwise Find + // cannot resolve a path the parent classified as exposed (D-0863). + undoFamilies := mountDisabledBuiltinFamilies(root) + defer undoFamilies() got := "" if c, _, _ := root.Find(argv); c != nil { got = strings.TrimSpace(strings.TrimPrefix(c.CommandPath(), root.Name()))