diff --git a/.github/coverage-floors.txt b/.github/coverage-floors.txt index 8a30bf685..8a7fe670b 100644 --- a/.github/coverage-floors.txt +++ b/.github/coverage-floors.txt @@ -131,6 +131,7 @@ internal/plugin/scaffold 64 # measured 66.3 2026-10-05 internal/plugin/verify 85 # measured 87.2 2026-10-05 internal/plugin/verify/signature 95 # measured 99.1 2026-10-13 (P7-PLUG-23) internal/plugin/verify/signature/signaturetest 0 # measured 0.0 2026-10-13; test-only key-table swap helper, exercised from other packages' tests (P7-PLUG-23) +internal/pluginname 85 # measured 90.0 2026-10-26 (D-0602) internal/portable 85 # measured 87.5 2026-10-05 internal/ports 9 # measured 11.5 2026-10-05 internal/postgres 49 # measured 51.4 2026-10-05 diff --git a/cmd/commands/backup_credentials_test.go b/cmd/commands/backup_credentials_test.go index 4245e2354..c025a6add 100644 --- a/cmd/commands/backup_credentials_test.go +++ b/cmd/commands/backup_credentials_test.go @@ -53,6 +53,9 @@ func TestBackupStreamProdMinioCredentials(t *testing.T) { func prodBackupMinioProject(t *testing.T) { t.Helper() + // The loader exports this .env into the process; keep it from leaking into + // later v1.5 remote-deploy tests, which refuse process-only config (D-0275). + restoreProcessEnv(t) for _, key := range []string{"ENV", "MINIO_ENABLED", "STORAGE_ENABLED", "MINIO_ROOT_USER", "MINIO_ROOT_PASSWORD", "NSELF_V15", "IMAGE_PINNING"} { t.Setenv(key, "") } diff --git a/cmd/commands/db_hasura.go b/cmd/commands/db_hasura.go index 0963863f4..184e0aa3e 100644 --- a/cmd/commands/db_hasura.go +++ b/cmd/commands/db_hasura.go @@ -90,11 +90,14 @@ func runDBHasuraMetadataReload(cmd *cobra.Command, _ []string) error { } func runDBVerifyChecksums(cmd *cobra.Command, _ []string) error { + plugin, _ := cmd.Flags().GetString("plugin") + if err := validatePluginFlag(plugin); err != nil { + return err + } cfg, err := loadProjectConfig() if err != nil { return err } - plugin, _ := cmd.Flags().GetString("plugin") mismatches, err := database.VerifyChecksums(cmd.Context(), cfg, plugin) if err != nil { return fmt.Errorf("verify checksums: %w", err) diff --git a/cmd/commands/db_verify_checksums_plugin_test.go b/cmd/commands/db_verify_checksums_plugin_test.go new file mode 100644 index 000000000..6aab6852e --- /dev/null +++ b/cmd/commands/db_verify_checksums_plugin_test.go @@ -0,0 +1,37 @@ +package commands + +// Purpose: `db verify-checksums --plugin` runs the same plugin-name grammar as +// `db migrate` (D-0602): a backup directory or a reserved name is refused +// before any project config or database is touched. + +import ( + "context" + "strings" + "testing" + + "github.com/nself-org/cli/internal/errs" + "github.com/spf13/cobra" +) + +func TestDBVerifyChecksumsRefusesNonPluginNames(t *testing.T) { + // The config loads fine from an empty dir and database.VerifyChecksums + // re-checks the name (wrapped as "verify checksums: ..."), so E401 alone + // does not prove the guard. Only the guard returns the error unwrapped, + // before the config load and the database layer are reached. + t.Chdir(t.TempDir()) + for _, bad := range []string{"notify.prev", "bin", "common", "status", "../x", "a/b"} { + cmd := &cobra.Command{Use: "verify-checksums"} + cmd.Flags().String("plugin", "", "") + if err := cmd.Flags().Set("plugin", bad); err != nil { + t.Fatal(err) + } + cmd.SetContext(context.Background()) + err := runDBVerifyChecksums(cmd, nil) + if d := errs.Describe(err); d == nil || d.Code != "E401" { + t.Errorf("--plugin %q: err = %v, want E401", bad, err) + } + if err != nil && strings.Contains(err.Error(), "verify checksums") { + t.Errorf("--plugin %q: err = %v came from the database layer, want the guard's error", bad, err) + } + } +} diff --git a/cmd/commands/deploy_env_resolve_test.go b/cmd/commands/deploy_env_resolve_test.go index 67f64f907..1a9e32702 100644 --- a/cmd/commands/deploy_env_resolve_test.go +++ b/cmd/commands/deploy_env_resolve_test.go @@ -87,6 +87,7 @@ func TestReadEnvFileOverrides_ParsesAndStripsQuotes(t *testing.T) { // a stale dev-tier value that no longer matches what was baked into // docker-compose.yml. func TestWriteResolvedDeployEnv_LaterFilesOverrideEarlier(t *testing.T) { + isolateDeployConfigEnv(t) dir := t.TempDir() if err := os.WriteFile(filepath.Join(dir, ".env"), []byte("POSTGRES_DB=dev_db\nSHARED_KEY=from_dev\n"), 0o600); err != nil { t.Fatalf("write .env: %v", err) @@ -128,6 +129,7 @@ func TestWriteResolvedDeployEnv_LaterFilesOverrideEarlier(t *testing.T) { // function actually deletes the temp snapshot so repeated deploys don't // accumulate stray files in the project root. func TestWriteResolvedDeployEnv_CleanupRemovesFile(t *testing.T) { + isolateDeployConfigEnv(t) dir := t.TempDir() path, cleanup, err := writeResolvedDeployEnv(dir, "prod") if err != nil { diff --git a/cmd/commands/deploy_env_scope_test.go b/cmd/commands/deploy_env_scope_test.go index 29324eabd..a9cda292e 100644 --- a/cmd/commands/deploy_env_scope_test.go +++ b/cmd/commands/deploy_env_scope_test.go @@ -167,6 +167,7 @@ func scopeFixture(t *testing.T, unreachable bool) (string, *scopeProber) { // leaks into a fixture and a cascade load never leaks out. func isolateDeployEnv(t *testing.T) { t.Helper() + isolateDeployConfigEnv(t) for _, kv := range os.Environ() { if k, _, ok := strings.Cut(kv, "="); ok && strings.HasPrefix(k, "NSELF_DEPLOY_HOST_") { t.Setenv(k, "") diff --git a/cmd/commands/deploy_follow.go b/cmd/commands/deploy_follow.go index e8fd3c284..0685d9741 100644 --- a/cmd/commands/deploy_follow.go +++ b/cmd/commands/deploy_follow.go @@ -19,6 +19,7 @@ import ( "time" "github.com/nself-org/cli/internal/controlplane" + "github.com/nself-org/cli/internal/docker" "github.com/nself-org/cli/internal/ui" "github.com/nself-org/cli/sdk/go/v2/remote" "github.com/spf13/cobra" @@ -31,11 +32,8 @@ func followDeployLogs(cmd *cobra.Command, workdir, target, strategy string, star host := legacyDeployHost(target) if host == "" { // Local host: stream logs directly. - lc := exec.CommandContext(cmd.Context(), "docker", "compose", "logs", "-f", "--tail=50") - lc.Dir = workdir - lc.Stdout = os.Stdout - lc.Stderr = os.Stderr - _ = lc.Run() + // Ctrl-C and a non-zero exit of the tail are not errors. + _ = docker.Stream(cmd.Context(), workdir, []string{"compose", "logs", "-f", "--tail=50"}, os.Stdout, os.Stderr) return nil } // Remote follow: tail logs on the remote host via SSH. diff --git a/cmd/commands/deploy_scratch_nginx_test.go b/cmd/commands/deploy_scratch_nginx_test.go index 049e116d3..41fb5c069 100644 --- a/cmd/commands/deploy_scratch_nginx_test.go +++ b/cmd/commands/deploy_scratch_nginx_test.go @@ -53,6 +53,7 @@ func stdoutOf(t *testing.T, fn func()) string { // nginxProject is an operator project with marked, unmarked and conf.d nginx files. func nginxProject(t *testing.T, env string) string { t.Helper() + isolateDeployConfigEnv(t) root := t.TempDir() wf(t, filepath.Join(root, ".env"), "PROJECT_NAME=demo\n"+env) wf(t, filepath.Join(root, "nginx", "sites", "stale.conf"), testBuildMarker+"# stale generated output\n") diff --git a/cmd/commands/doctor_drift_plugindir_test.go b/cmd/commands/doctor_drift_plugindir_test.go new file mode 100644 index 000000000..805a66c54 --- /dev/null +++ b/cmd/commands/doctor_drift_plugindir_test.go @@ -0,0 +1,43 @@ +package commands + +// Purpose: the standalone doctor drift check resolves "@plugins/" record keys +// under the plugin directory the project's env cascade names (D-0644). +// Constraints: temp dirs only; the shell leaves NSELF_PLUGIN_DIR unset. + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "os" + "path/filepath" + "testing" + + "github.com/nself-org/cli/internal/reconcile" +) + +func TestDoctorDriftPluginDirOnlyInProjectEnv(t *testing.T) { + isolateDoctorHome(t) + t.Setenv("NSELF_PLUGIN_DIR", "") + dir, plugins := t.TempDir(), t.TempDir() + env := "PROJECT_NAME=fx\nENV=dev\nBASE_DOMAIN=example.test\nSSL_MODE=none\nNSELF_PLUGIN_DIR=" + plugins + "\n" + if err := os.WriteFile(filepath.Join(dir, ".env"), []byte(env), 0o600); err != nil { + t.Fatal(err) + } + const key = reconcile.PluginsPrefix + "fxplug/docker-compose.plugin.yml" + fragment := filepath.Join(plugins, "fxplug", "docker-compose.plugin.yml") + if err := os.MkdirAll(filepath.Dir(fragment), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(fragment, []byte("services: {}\n# hand edit\n"), 0o644); err != nil { + t.Fatal(err) + } + sum := sha256.Sum256([]byte("services: {}\n")) // the bytes the record says were written + st := &reconcile.GeneratedState{Files: map[string]string{key: hex.EncodeToString(sum[:])}} + if err := st.Save(dir); err != nil { + t.Fatal(err) + } + r, _, err := checkGeneratedDrift(context.Background(), dir) + if err != nil || r.Status != "fail" || r.Message != "drift: "+key+" (hand-edited)" { + t.Fatalf("hand-edited fragment under the .env plugin dir: %+v, %v", r, err) + } +} diff --git a/cmd/commands/functions_logs.go b/cmd/commands/functions_logs.go index c5bdb30b9..c1d78f50a 100644 --- a/cmd/commands/functions_logs.go +++ b/cmd/commands/functions_logs.go @@ -11,12 +11,10 @@ package commands // functionsCmd (parent) remain in functions.go. import ( - "bufio" "fmt" - "io" - "os/exec" "strings" + "github.com/nself-org/cli/internal/docker" "github.com/spf13/cobra" ) @@ -68,33 +66,12 @@ func runFunctionsLogs(cmd *cobra.Command, args []string) error { dockerArgs = append(dockerArgs, fmt.Sprintf("--tail=%d", functionsLogsFlags.tail)) dockerArgs = append(dockerArgs, containerID) - logsCmd := exec.CommandContext(cmd.Context(), "docker", dockerArgs...) - stdout, err := logsCmd.StdoutPipe() - if err != nil { - return fmt.Errorf("pipe: %w", err) - } - stderr, err := logsCmd.StderrPipe() - if err != nil { - return fmt.Errorf("stderr pipe: %w", err) - } - - if err := logsCmd.Start(); err != nil { - return fmt.Errorf("starting docker logs: %w", err) - } - - // Filter lines containing the function name and print. - filterAndPrint := func(r io.Reader) { - scanner := bufio.NewScanner(r) - for scanner.Scan() { - line := scanner.Text() - if strings.Contains(line, name) || strings.Contains(line, "/v1/"+name) { - fmt.Println(line) - } + // Both docker streams carry function output; keep the lines that mention + // the function. + return docker.StreamLines(cmd.Context(), "", dockerArgs, nil, true, maxLogLineBytes, func(line string) error { + if strings.Contains(line, name) || strings.Contains(line, "/v1/"+name) { + fmt.Println(line) } - } - - go filterAndPrint(stderr) - filterAndPrint(stdout) - - return logsCmd.Wait() + return nil + }) } diff --git a/cmd/commands/logs_stream.go b/cmd/commands/logs_stream.go index 83d81a19b..1b7b2c55d 100644 --- a/cmd/commands/logs_stream.go +++ b/cmd/commands/logs_stream.go @@ -12,13 +12,14 @@ package commands // docker spawn of the logs stream (human, legacy JSON and v1.5 JSON paths). import ( - "bufio" "context" "fmt" "io" "os" "os/exec" "strings" + + "github.com/nself-org/cli/internal/docker" ) // streamFilteredLogs runs docker with composeArgs and streams filtered/formatted output to w. @@ -44,56 +45,13 @@ const maxLogLineBytes = 4 << 20 // error from fn stops the stream, kills docker and is returned. A cancelled // parent context ends the stream without an error. func streamLogLines(parent context.Context, workdir string, composeArgs []string, opts LogsOptions, fn func(line string) error) error { - ctx, cancel := context.WithCancel(parent) - defer cancel() - dockerCmd := exec.CommandContext(ctx, "docker", composeArgs...) - dockerCmd.Dir = workdir - dockerCmd.Stderr = os.Stderr - - if opts.Follow { - dockerCmd.Stdin = os.Stdin - } - - stdout, err := dockerCmd.StdoutPipe() - if err != nil { - return fmt.Errorf("creating stdout pipe: %w", err) - } - - if err := dockerCmd.Start(); err != nil { - return fmt.Errorf("starting docker: %w", err) - } - - var fnErr error - scanner := bufio.NewScanner(stdout) - scanner.Buffer(make([]byte, 0, 64*1024), maxLogLineBytes) - for scanner.Scan() { - filtered, ok := filterLogLine(scanner.Text(), opts) + return docker.StreamLines(parent, workdir, composeArgs, os.Stderr, false, maxLogLineBytes, func(line string) error { + filtered, ok := filterLogLine(line, opts) if !ok { - continue - } - if fnErr = fn(filtered); fnErr != nil { - cancel() - break + return nil } - } - - // A read error (a line over maxLogLineBytes) must not pass for the end of - // the logs: stop docker so Wait cannot block on a full pipe, and report it. - if fnErr == nil { - if serr := scanner.Err(); serr != nil { - cancel() - fnErr = fmt.Errorf("reading docker output: %w", serr) - } - } - - waitErr := dockerCmd.Wait() - if fnErr != nil { - return fnErr - } - if parent.Err() != nil { - return nil - } - return waitErr + return fn(filtered) + }) } // getRunningServices returns the list of service names from docker compose ps --services. diff --git a/cmd/commands/plugin_logs.go b/cmd/commands/plugin_logs.go index eec795698..2a402885c 100644 --- a/cmd/commands/plugin_logs.go +++ b/cmd/commands/plugin_logs.go @@ -7,6 +7,7 @@ import ( "regexp" "strings" + "github.com/nself-org/cli/internal/docker" "github.com/spf13/cobra" ) @@ -66,13 +67,7 @@ func runPluginLogs(cmd *cobra.Command, args []string) error { if grepPattern == "" { // Simple passthrough. - dockerCmd := exec.Command("docker", dockerArgs...) - dockerCmd.Stdout = os.Stdout - dockerCmd.Stderr = os.Stderr - if err := dockerCmd.Run(); err != nil { - return fmt.Errorf("docker logs: %w", err) - } - return nil + return docker.Stream(cmd.Context(), "", dockerArgs, os.Stdout, os.Stderr) } // Validate the grep pattern before use so we get a helpful error on invalid regex. @@ -81,34 +76,33 @@ func runPluginLogs(cmd *cobra.Command, args []string) error { } // With grep: pipe docker logs output through grep. - dockerCmd := exec.Command("docker", dockerArgs...) grepCmd := exec.Command("grep", "-E", grepPattern) - - dockerOut, err := dockerCmd.StdoutPipe() + grepIn, err := grepCmd.StdinPipe() if err != nil { - return fmt.Errorf("creating docker stdout pipe: %w", err) + return fmt.Errorf("creating grep stdin pipe: %w", err) } - dockerCmd.Stderr = os.Stderr - grepCmd.Stdin = dockerOut grepCmd.Stdout = os.Stdout grepCmd.Stderr = os.Stderr - - if err := dockerCmd.Start(); err != nil { - return fmt.Errorf("starting docker logs: %w", err) - } if err := grepCmd.Start(); err != nil { return fmt.Errorf("starting grep: %w", err) } // Wait for docker to finish, then grep will EOF. - _ = dockerCmd.Wait() + streamErr := docker.Stream(cmd.Context(), "", dockerArgs, grepIn, os.Stderr) + _ = grepIn.Close() // grep exits 1 on "no match" (normal) and 2+ on error. Only surface errors. - if err := grepCmd.Wait(); err != nil { - if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 1 { + waitErr := grepCmd.Wait() + // A docker that failed to start or exited non-zero is the root cause; it + // outranks grep's "no match" exit. + if streamErr != nil { + return streamErr + } + if waitErr != nil { + if exitErr, ok := waitErr.(*exec.ExitError); ok && exitErr.ExitCode() == 1 { // No lines matched — not an error. return nil } - return fmt.Errorf("grep error: %w", err) + return fmt.Errorf("grep error: %w", waitErr) } return nil } diff --git a/cmd/commands/plugin_logs_test.go b/cmd/commands/plugin_logs_test.go new file mode 100644 index 000000000..9bb463fed --- /dev/null +++ b/cmd/commands/plugin_logs_test.go @@ -0,0 +1,48 @@ +package commands + +import ( + "context" + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +// stubPluginLogsDocker puts a fake `docker` first on PATH. inspect succeeds so the +// container resolves; `logs` runs logsBody. +func stubPluginLogsDocker(t *testing.T, logsBody string) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("fake docker is a shell script") + } + dir := t.TempDir() + script := "#!/bin/sh\ncase \"$1\" in\n inspect) echo /nself-plugin-demo; exit 0;;\n logs) " + logsBody + ";;\nesac\n" + if err := os.WriteFile(filepath.Join(dir, "docker"), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) +} + +func runPluginLogsWith(t *testing.T, grep string) error { + t.Helper() + pluginLogsCmd.Flags().Set("grep", grep) + t.Cleanup(func() { pluginLogsCmd.Flags().Set("grep", "") }) + pluginLogsCmd.SetContext(context.Background()) + return runPluginLogs(pluginLogsCmd, []string{"demo"}) +} + +func TestPluginLogsGrepSurfacesDockerFailure(t *testing.T) { + stubPluginLogsDocker(t, "echo 'no such container' >&2; exit 1") + err := runPluginLogsWith(t, "ERROR") + if err == nil || !strings.Contains(err.Error(), "docker logs") { + t.Fatalf("want docker logs error, got %v", err) + } +} + +func TestPluginLogsGrepNoMatchIsNotAnError(t *testing.T) { + stubPluginLogsDocker(t, "echo hello; exit 0") + if err := runPluginLogsWith(t, "ERROR"); err != nil { + t.Fatalf("no match must not error, got %v", err) + } +} diff --git a/internal/database/plugin_arg.go b/internal/database/plugin_arg.go index fa551fe93..1c7479ec2 100644 --- a/internal/database/plugin_arg.go +++ b/internal/database/plugin_arg.go @@ -1,31 +1,29 @@ package database -// Purpose: refuse a path-shaped --plugin argument before any migration scan -// (P7-PLUG-20 fix). migrationsDir joins the plugin name to the plugin +// Purpose: refuse a non-plugin --plugin argument before any migration scan +// (P7-PLUG-20 fix, D-0602). migrationsDir joins the plugin name to the plugin // directory, so `nself db migrate up --plugin ../victim` would scan and apply -// SQL from a sibling directory. +// SQL from a sibling directory, and `--plugin notify.prev` the update backup's. // Inputs: the --plugin value ("" means the project's own migrations). // Outputs: nil, or an E401 error. -// Constraints: no I/O. This layer only guarantees the name is one path segment. -// The full plugin-name grammar (plugin.ValidateName) runs at the command layer -// (validatePluginFlag); importing internal/plugin here would add a same-layer -// L2 edge that layers.yaml does not allow. +// Constraints: no I/O. The grammar is pluginname.Reason, the same check +// plugin.ValidateName runs at the command layer: one path segment, lowercase +// slug, not a backup directory and not a reserved name (bin, common, status). import ( - "path/filepath" - "strings" - "github.com/nself-org/cli/internal/errs" + "github.com/nself-org/cli/internal/pluginname" ) -// validatePluginArg refuses a non-empty plugin argument that is not exactly one -// path segment under the plugin directory. +// validatePluginArg refuses a non-empty plugin argument that is not a plain, +// non-reserved plugin name. func validatePluginArg(plugin string) error { if plugin == "" { return nil } - if plugin == "." || plugin == ".." || strings.ContainsAny(plugin, "/\\\x00") || filepath.VolumeName(plugin) != "" { - return errs.Newf("E401", "invalid plugin name %q: must be a single plugin name, not a path", plugin) + if reason := pluginname.Reason(plugin); reason != "" { + return errs.Newf("E401", "invalid plugin name %q: %s", plugin, reason). + WithFix("use the plugin name as shown by 'nself plugin list', for example 'nself db migrate up --plugin notify'") } return nil } diff --git a/internal/database/plugin_arg_test.go b/internal/database/plugin_arg_test.go index 1190705ed..f4c5529a3 100644 --- a/internal/database/plugin_arg_test.go +++ b/internal/database/plugin_arg_test.go @@ -14,7 +14,7 @@ import ( func TestPluginArgRefusedBeforeAnyScan(t *testing.T) { cfg := &config.Config{} - for _, name := range []string{"../victim", "./x", "a/b", "..", ".", "/etc", `a\b`, "a\x00b"} { + for _, name := range []string{"../victim", "./x", "a/b", "..", ".", "/etc", `a\b`, "a\x00b", "notify.prev", "bin", "common", "status"} { if err := validatePluginArg(name); err == nil { t.Errorf("validatePluginArg(%q) = nil", name) } diff --git a/internal/docker/compose_states.go b/internal/docker/compose_states.go index 4c2e1a7a5..05e48bb38 100644 --- a/internal/docker/compose_states.go +++ b/internal/docker/compose_states.go @@ -164,8 +164,10 @@ func (c *Compose) startedAt(ctx context.Context, id string) (string, error) { // StartMarks maps container ID to StartedAt for each exited container of // service. Taken before `up`, it lets FreshStates tell a one-shot that ran -// again (same container, new start time) from an old exit. The whole read -// (ps and every inspect) shares one stateCallTimeout. +// again (same container, new start time) from an old exit. A container removed +// between the ps and its inspect is left out (it has no mark to compare, as in +// FreshStates). The whole read (ps and every inspect) shares one +// stateCallTimeout. func (c *Compose) StartMarks(ctx context.Context, workdir, service string) (map[string]string, error) { marks := map[string]string{} err := bounded(ctx, "docker state read for "+service, func(ctx context.Context) error { @@ -175,9 +177,14 @@ func (c *Compose) StartMarks(ctx context.Context, workdir, service string) (map[ } for _, s := range cs { if s.State == "exited" && s.ID != "" { - if marks[s.ID], err = c.startedAt(ctx, s.ID); err != nil { + at, err := c.startedAt(ctx, s.ID) + if errors.Is(err, errContainerGone) { + continue + } + if err != nil { return err } + marks[s.ID] = at } } return nil diff --git a/internal/docker/compose_states_bound_test.go b/internal/docker/compose_states_bound_test.go index 77217beb6..0fd3e63bb 100644 --- a/internal/docker/compose_states_bound_test.go +++ b/internal/docker/compose_states_bound_test.go @@ -87,6 +87,27 @@ func TestFreshStatesDropsVanishedContainer(t *testing.T) { } } +func TestStartMarksDropsVanishedContainer(t *testing.T) { + ps := "echo '[{\"ID\":\"c1\",\"Name\":\"p-a-1\",\"State\":\"exited\"},{\"ID\":\"c2\",\"Name\":\"p-a-2\",\"State\":\"exited\"},{\"ID\":\"c3\",\"Name\":\"p-a-3\",\"State\":\"running\"}]'\n" + for name, tc := range map[string]struct { + stderr string + want map[string]string + wantErr bool + }{ + "no such object": {"Error: No such object: c2", map[string]string{"c1": "t1"}, false}, + "no such container": {"Error: No such container: c2", map[string]string{"c1": "t1"}, false}, + "other failure": {"Cannot connect to the Docker daemon", nil, true}, + } { + t.Run(name, func(t *testing.T) { + c, dir := scriptDocker(t, "if [ \"$1\" = inspect ]; then\n if [ \"$5\" = c2 ]; then echo '"+tc.stderr+"' >&2; exit 1; fi\n echo t1; exit 0; fi\n"+ps) + marks, err := c.StartMarks(context.Background(), dir, "a") + if (err != nil) != tc.wantErr || len(marks) != len(tc.want) || (!tc.wantErr && marks["c1"] != "t1") { + t.Errorf("marks %+v, err %v; want %+v, err %v", marks, err, tc.want, tc.wantErr) + } + }) + } +} + func TestHealthGateCancelAndKilledRead(t *testing.T) { old := gateReadGrace gateReadGrace = 50 * time.Millisecond diff --git a/internal/docker/compose_unix.go b/internal/docker/compose_unix.go index ab5672a12..76390c1e0 100644 --- a/internal/docker/compose_unix.go +++ b/internal/docker/compose_unix.go @@ -21,3 +21,13 @@ func killProcessGroup(cmd *exec.Cmd) { _ = syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) } } + +// termProcessGroup sends SIGTERM to the entire process group of cmd: the docker +// CLI and the compose plugin it runs as a child both get it, so neither is +// orphaned when a stream is stopped. +func termProcessGroup(cmd *exec.Cmd) error { + if cmd.Process == nil { + return nil + } + return syscall.Kill(-cmd.Process.Pid, syscall.SIGTERM) +} diff --git a/internal/docker/compose_windows.go b/internal/docker/compose_windows.go index 9f9dd7952..adf7b1d6d 100644 --- a/internal/docker/compose_windows.go +++ b/internal/docker/compose_windows.go @@ -13,3 +13,11 @@ func setProcGroupAttr(_ *exec.Cmd) {} // killProcessGroup is a no-op on Windows because there is no unix-style // process group to signal. The Cancel hook falls through to cmd.Process.Kill(). func killProcessGroup(_ *exec.Cmd) {} + +// termProcessGroup kills the immediate process: Windows has no SIGTERM to send. +func termProcessGroup(cmd *exec.Cmd) error { + if cmd.Process == nil { + return nil + } + return cmd.Process.Kill() +} diff --git a/internal/docker/stream.go b/internal/docker/stream.go new file mode 100644 index 000000000..5df776269 --- /dev/null +++ b/internal/docker/stream.go @@ -0,0 +1,109 @@ +package docker + +// stream.go — long-running docker output streams (`logs -f`), D-0711. +// +// Purpose: the one place a command streams a never-ending docker process to the +// operator. Compose.Run discards its output when stdout is not a TTY, so it +// cannot carry `deploy --follow | tee`; Stream forwards stdout and stderr always. +// Inputs: the docker arguments (after "docker"), a working directory and the +// writers (nil discards that stream; callers pass os.Stdout / os.Stderr). +// Outputs: the child's output on the writers. A stop by the caller's context, +// SIGINT or SIGTERM ends the stream with a nil error; a failure to start or a +// non-zero exit is returned. +// Constraints: the child runs in its own process group and a stop sends it +// SIGTERM through that group (SIGKILL after streamWaitDelay), so Ctrl-C leaves +// no orphaned docker or compose plugin behind. + +import ( + "bufio" + "context" + "errors" + "fmt" + "io" + "os" + "os/exec" + "os/signal" + "syscall" + "time" +) + +// streamWaitDelay is how long a stopped stream gets to exit on SIGTERM before +// the child is killed. A var so tests can shorten it. +var streamWaitDelay = 5 * time.Second + +// Stream runs `docker args...` in dir until it exits or is stopped, forwarding +// its stdout and stderr to the writers. +func Stream(ctx context.Context, dir string, args []string, stdout, stderr io.Writer) error { + if stdout == nil { + stdout = io.Discard + } + if stderr == nil { + stderr = io.Discard + } + sigCtx, stop := signal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM) + defer stop() + cmd := exec.CommandContext(sigCtx, "docker", args...) + cmd.Dir = dir + cmd.Stdout, cmd.Stderr = stdout, stderr + setProcGroupAttr(cmd) + cmd.Cancel = func() error { return termProcessGroup(cmd) } + cmd.WaitDelay = streamWaitDelay + err := cmd.Run() + if sigCtx.Err() != nil { + killProcessGroup(cmd) // a grandchild that ignored SIGTERM + return nil + } + if err != nil && !errors.Is(err, exec.ErrWaitDelay) { + return fmt.Errorf("docker %s: %w", firstArg(args), err) + } + return nil +} + +func firstArg(args []string) string { + if len(args) == 0 { + return "" + } + return args[0] +} + +// StreamLines is Stream with stdout delivered line by line to fn. mergeStderr +// sends stderr through fn too; otherwise it goes to stderr (nil discards it). maxLine caps one line: a longer line ends the stream with an +// error instead of a silent stop. An error from fn stops docker and is returned. +func StreamLines(ctx context.Context, dir string, args []string, stderr io.Writer, mergeStderr bool, maxLine int, fn func(line string) error) error { + ctx, cancel := context.WithCancel(ctx) + defer cancel() + pr, pw := io.Pipe() + errw := stderr + if mergeStderr { + errw = pw + } + done := make(chan error, 1) + go func() { + err := Stream(ctx, dir, args, pw, errw) + _ = pw.Close() + done <- err + }() + + var ferr error + sc := bufio.NewScanner(pr) + sc.Buffer(make([]byte, 0, min(64*1024, maxLine)), maxLine) // the cap bounds a token too + for sc.Scan() { + if ferr = fn(sc.Text()); ferr != nil { + break + } + } + if ferr == nil { + if serr := sc.Err(); serr != nil { + ferr = fmt.Errorf("reading docker output: %w", serr) + } + } + if ferr != nil { + cancel() // stop docker so it cannot block on a pipe nobody reads + } + _ = pr.Close() + serr := <-done + if ferr != nil { + return ferr + } + return serr +} diff --git a/internal/docker/stream_test.go b/internal/docker/stream_test.go new file mode 100644 index 000000000..c3d4d3c72 --- /dev/null +++ b/internal/docker/stream_test.go @@ -0,0 +1,178 @@ +//go:build !windows + +package docker + +// stream_test.go — Stream and StreamLines against a fake docker script (D-0711): +// output is forwarded without a TTY, a stop leaves no orphan, SIGINT is caught. + +import ( + "bytes" + "context" + "errors" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + "time" +) + +// fakeDockerOnPath puts a docker stub with body first on PATH. +func fakeDockerOnPath(t *testing.T, body string) { + t.Helper() + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "docker"), []byte("#!/bin/sh\n"+body), 0o755); err != nil { //nolint:gosec // test stub must be executable + t.Fatal(err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) +} + +func TestStreamForwardsBothStreamsWithoutTTY(t *testing.T) { + fakeDockerOnPath(t, "echo \"args:$*\"\necho oops >&2\nexit 0\n") + var out, errb bytes.Buffer + if err := Stream(context.Background(), t.TempDir(), []string{"compose", "logs", "-f"}, &out, &errb); err != nil { + t.Fatal(err) + } + if out.String() != "args:compose logs -f\n" || errb.String() != "oops\n" { + t.Errorf("stdout %q, stderr %q", out.String(), errb.String()) + } +} + +func TestStreamReturnsNonZeroExit(t *testing.T) { + fakeDockerOnPath(t, "exit 3\n") + err := Stream(context.Background(), t.TempDir(), []string{"logs"}, &bytes.Buffer{}, &bytes.Buffer{}) + var ee *exec.ExitError + if !errors.As(err, &ee) || ee.ExitCode() != 3 { + t.Errorf("err = %v, want an ExitError with code 3", err) + } +} + +// waitFor polls until path exists. +func waitFor(t *testing.T, path string) { + t.Helper() + for i := 0; i < 200; i++ { + if _, err := os.Stat(path); err == nil { + return + } + time.Sleep(25 * time.Millisecond) + } + t.Fatalf("%s never appeared", path) +} + +// alive reports whether pid still runs. +func alive(pid int) bool { return syscall.Kill(pid, 0) == nil } + +// longRunning is a docker stub that records a grandchild's pid in marks/gc and +// then waits on it, like docker spawning the compose plugin. +func longRunning(t *testing.T) (marks string) { + t.Helper() + marks = t.TempDir() + fakeDockerOnPath(t, "sleep 30 &\necho $! > '"+marks+"/gc.tmp'\nmv '"+marks+"/gc.tmp' '"+marks+"/gc'\nwait\n") + return marks +} + +func grandchild(t *testing.T, marks string) int { + t.Helper() + raw, err := os.ReadFile(filepath.Join(marks, "gc")) + if err != nil { + t.Fatal(err) + } + pid, err := strconv.Atoi(strings.TrimSpace(string(raw))) + if err != nil { + t.Fatal(err) + } + return pid +} + +func awaitGone(t *testing.T, pid int) { + t.Helper() + for i := 0; i < 200; i++ { + if !alive(pid) { + return + } + time.Sleep(25 * time.Millisecond) + } + t.Errorf("process %d was orphaned", pid) +} + +func TestStreamContextStopLeavesNoOrphan(t *testing.T) { + marks := longRunning(t) + ctx, cancel := context.WithCancel(context.Background()) + res := make(chan error, 1) + go func() { res <- Stream(ctx, t.TempDir(), []string{"logs"}, &bytes.Buffer{}, &bytes.Buffer{}) }() + waitFor(t, filepath.Join(marks, "gc")) + cancel() + if err := <-res; err != nil { + t.Errorf("a stop is not an error: %v", err) + } + awaitGone(t, grandchild(t, marks)) +} + +// TestStreamSIGINTStopsTheChildNotTheProcess: Ctrl-C reaches the stream through +// its NotifyContext; the test process survives and the child group is gone. +func TestStreamSIGINTStopsTheChildNotTheProcess(t *testing.T) { + // On Linux CI (non-TTY, no controlling terminal), sending SIGINT to + // os.Getpid() from the test goroutine races with signal.NotifyContext + // registration inside Stream's goroutine: if the signal arrives first, + // the default action kills the test binary. Instead, have the fake docker + // send SIGINT to its parent after it starts, guaranteeing the handler + // is installed before the signal lands. + marks := t.TempDir() + fakeDockerOnPath(t, "sleep 30 &\necho $! > '"+marks+"/gc.tmp'\nmv '"+marks+"/gc.tmp' '"+marks+"/gc'\nkill -INT $PPID\nwait\n") + res := make(chan error, 1) + go func() { + res <- Stream(context.Background(), t.TempDir(), []string{"logs"}, &bytes.Buffer{}, &bytes.Buffer{}) + }() + waitFor(t, filepath.Join(marks, "gc")) + select { + case err := <-res: + if err != nil { + t.Errorf("a stop is not an error: %v", err) + } + case <-time.After(10 * time.Second): + t.Fatal("SIGINT did not stop the stream") + } + awaitGone(t, grandchild(t, marks)) +} + +func TestStreamLines(t *testing.T) { + fakeDockerOnPath(t, "echo one\necho err1 >&2\necho two\n") + collect := func(merge bool) (lines []string, errb string) { + var eb bytes.Buffer + err := StreamLines(context.Background(), t.TempDir(), []string{"logs"}, &eb, merge, 1<<20, func(l string) error { + lines = append(lines, l) + return nil + }) + if err != nil { + t.Fatal(err) + } + return lines, eb.String() + } + if lines, eb := collect(false); strings.Join(lines, ",") != "one,two" || eb != "err1\n" { + t.Errorf("unmerged: lines %q, stderr %q", lines, eb) + } + if lines, eb := collect(true); len(lines) != 3 || eb != "" { + t.Errorf("merged: lines %q, stderr %q", lines, eb) + } +} + +func TestStreamLinesCallbackErrorStopsDocker(t *testing.T) { + marks := t.TempDir() + fakeDockerOnPath(t, "sleep 30 &\necho $! > '"+marks+"/gc'\necho first\nwait\n") + boom := errors.New("boom") + err := StreamLines(context.Background(), t.TempDir(), []string{"logs"}, nil, false, 1<<20, func(string) error { return boom }) + if !errors.Is(err, boom) { + t.Fatalf("err = %v, want boom", err) + } + awaitGone(t, grandchild(t, marks)) +} + +func TestStreamLinesOverlongLineIsAnError(t *testing.T) { + fakeDockerOnPath(t, "head -c 5000 /dev/zero | tr '\\0' x\necho\n") + err := StreamLines(context.Background(), t.TempDir(), []string{"logs"}, nil, false, 1024, func(string) error { return nil }) + if err == nil || !strings.Contains(err.Error(), "reading docker output") { + t.Errorf("err = %v, want a read error", err) + } +} diff --git a/internal/plugin/name_validate.go b/internal/plugin/name_validate.go index 2d8bfa0c0..7482a0d2d 100644 --- a/internal/plugin/name_validate.go +++ b/internal/plugin/name_validate.go @@ -3,16 +3,14 @@ package plugin // Purpose: ValidateName is the one check every plugin name must pass before it // is joined to the plugin directory. Without it `nself plugin remove ../../.ssh` // reached os.RemoveAll on a path outside the plugin directory (P7-PLUG-20). +// The grammar itself lives in internal/pluginname, so internal/database (a +// same-layer package) applies the identical rules (D-0602). // Inputs: a plugin name exactly as the caller received it (CLI arg, MCP/admin // request, registry or bundle member name). // Outputs: nil for a plain plugin slug, or an E401 (invalid usage) error naming // the rule that failed. -// Constraints: no I/O, no normalisation. The shape is exactly the one -// manifest.go enforces on installed plugin names (namePattern: lowercase -// letters, digits and hyphen, starting with a letter). No '.' or '_': a dotted -// backup directory such as "notify.prev" is not a plugin and must never be -// addressable by remove, and sanitizeSchemaName maps '-' and '_' to the same -// schema so '_' would make two names collide. +// Constraints: no I/O, no normalisation. A dotted backup directory such as +// "notify.prev" is not a plugin and must never be addressable by remove. // Callers must use the SAME string for the bundle ledger lookup and for the // removal, so they validate first and never lower-case or trim afterwards. @@ -22,128 +20,48 @@ import ( "strings" "github.com/nself-org/cli/internal/errs" + "github.com/nself-org/cli/internal/pluginname" ) -// pgIdentMax is PostgreSQL's identifier limit in bytes (NAMEDATALEN-1). Longer -// identifiers are silently truncated, so two long names could share one schema. -const pgIdentMax = 63 +// pgIdentMax is PostgreSQL's identifier limit in bytes (see pluginname.PgIdentMax). +const pgIdentMax = pluginname.PgIdentMax -// longestIdentAffix is the extra bytes around a plugin name in the longest -// identifier generated from it: "np_" + name + "_role". -const longestIdentAffix = len("np_") + len("_role") +// maxPluginNameLen is the longest accepted plugin name (see pluginname.MaxLen). +const maxPluginNameLen = pluginname.MaxLen -// maxPluginNameLen is the longest accepted plugin name: the longest generated -// identifier (np__role) must fit in pgIdentMax bytes. The longest real -// plugin name in the tree is far below this. -const maxPluginNameLen = pgIdentMax - longestIdentAffix +// pluginSlugRe is the allowed plugin name shape, identical to namePattern in +// manifest.go. +var pluginSlugRe = pluginname.SlugRe -// pluginSlugRe is the allowed shape, identical to namePattern in manifest.go: -// a lowercase letter, then lowercase letters, digits or '-'. -var pluginSlugRe = regexp.MustCompile(`^[a-z][a-z0-9-]*$`) +// reservedNames are plugin-shaped names that already mean something else to the +// CLI (see pluginname.Reserved). +var reservedNames = pluginname.Reserved // binaryNameRe is the allowed shape of a published command binary name (one // path element). It is wider than a plugin slug only for '.', '_' and capitals. var binaryNameRe = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._-]*$`) -// reservedNames are plugin-shaped names that already mean something else to the -// CLI, so no plugin may take them. Every entry is derived from code that creates -// or reads the object, and a test (TestReservedNamesCoverSharedSchemas) fails when -// a new shared np_ schema appears without a matching entry here. -// -// Direct children of the plugin directory that are NOT plugins: -// - "bin": the shared command-binary directory (router.go pluginBinDir, -// skipped by mount/discover.go). `plugin remove bin` would delete every -// published binary. -// - ".nself-state" (bundle ledger), ".install.lock", ".staging-thirdparty-*", -// ".bundle-rollback-*" and ".prev" backups all start with '.' or hold a -// '.', so pluginSlugRe already refuses them; TestInfrastructureEntriesNeverSlugs -// pins that. -// -// Schemas and roles a plugin name can map onto (np_, and -// np__role): -// - "common": schema np_common, the shared ledger schema (np_common.schema_versions, -// np_common.plugin_schema_versions) created by database/migrate_ledger.go and -// plugin/schema_reconcile.go. Removing plugin "common" would run -// DROP SCHEMA "np_common" CASCADE. -// - "status": schema np_status, created by the licensed nself-status-page plugin -// (plugins-pro). A table-owning plugin named "status" removed without -// --keep-data would run DROP SCHEMA "np_status" CASCADE and delete the -// status-page data. -// -// np_plugins and np_ai_usage are tables, not schemas, so they do not collide. -var reservedNames = map[string]string{ - "bin": "it is the shared plugin command-binary directory", - "common": "it maps to np_common, the shared ledger schema", - "status": "it maps to np_status, the schema of the nself-status-page plugin", -} - -// slugReason returns why name is not a plain single-element slug, or "". -func slugReason(name string) string { - switch { - case name == "": - return "it is empty" - case name == "." || name == "..": - return "it is a directory reference" - case len(name) > maxPluginNameLen: - return "it is too long" - case strings.ContainsAny(name, `/\`) || strings.ContainsRune(name, filepath.Separator): - return "it contains a path separator" - case filepath.IsAbs(name) || filepath.VolumeName(name) != "": - return "it is an absolute path" - case strings.HasPrefix(name, "-"): - return "it starts with '-'" - case hasControlChar(name): - return "it contains a control character" - case filepath.Base(name) != name: - return "it is not a single path element" - case !pluginSlugRe.MatchString(name): - return "plugin names use lowercase letters, digits and '-', and start with a letter" - } - return "" -} - // ValidateName reports whether name is a plain plugin slug that is safe to join -// to the plugin directory and is not a reserved infrastructure name. It rejects -// the empty string, ".", "..", any path separator (slash or backslash, and the -// OS separator), absolute paths, a leading '-', NUL and other control -// characters, whitespace, and anything filepath.Base would change. +// to the plugin directory and is not a reserved infrastructure name. func ValidateName(name string) error { - reason := slugReason(name) - if reason == "" { - reason = reservedNames[name] - if reason != "" { - reason = "it is reserved: " + reason - } + if reason := pluginname.Reason(name); reason != "" { + return errs.Newf("E401", "invalid plugin name %q: %s", name, reason). + WithFix("use the plugin name as shown by 'nself plugin list', for example 'nself plugin remove notify'") } - if reason == "" { - return nil - } - return errs.Newf("E401", "invalid plugin name %q: %s", name, reason). - WithFix("use the plugin name as shown by 'nself plugin list', for example 'nself plugin remove notify'") + return nil } // ValidateCommandName reports whether cmd is a plain command name: the part after // "nself-" in a published binary, and the first argument of the command proxy. // Same shape as a plugin slug, but reserved plugin names are allowed. func ValidateCommandName(cmd string) error { - if reason := slugReason(cmd); reason != "" { + if reason := pluginname.SlugReason(cmd); reason != "" { return errs.Newf("E401", "invalid command name %q: %s", cmd, reason). WithFix("command names are lowercase letters, digits and '-'") } return nil } -// hasControlChar reports whether s holds NUL, any other ASCII control -// character, or DEL. -func hasControlChar(s string) bool { - for _, r := range s { - if r < 0x20 || r == 0x7f { - return true - } - } - return false -} - // ValidateBinaryName reports whether bin is a single plain path element that is // safe to join to the plugin bin directory. A manifest's binaryName and // cliCommands names come from the plugin author, so they get the same checks as @@ -162,7 +80,7 @@ func ValidateBinaryName(bin string) error { reason = "it contains a path separator" case filepath.IsAbs(bin) || filepath.VolumeName(bin) != "": reason = "it is an absolute path" - case hasControlChar(bin): + case pluginname.HasControlChar(bin): reason = "it contains a control character" case filepath.Base(bin) != bin: reason = "it is not a single path element" diff --git a/internal/pluginname/name.go b/internal/pluginname/name.go new file mode 100644 index 000000000..a74a2c688 --- /dev/null +++ b/internal/pluginname/name.go @@ -0,0 +1,126 @@ +// Package pluginname holds the plugin-name grammar: the one check every plugin +// name passes before it is joined to the plugin directory. +// +// Purpose: internal/plugin and internal/database are same-layer (L2) packages +// with no import edge between them, yet both join a plugin name to the plugin +// directory. The grammar lives here, below both, so the database layer refuses +// a backup directory ("notify.prev") or a reserved name ("bin", "common", +// "status") exactly as the command layer does (D-0602, P7-PLUG-20). +// Inputs: a plugin name exactly as the caller received it (CLI arg, MCP/admin +// request, registry or bundle member name). +// Outputs: "" for a plain plugin slug, or the rule that failed. Callers wrap the +// reason in their own E401 (invalid usage) error: this package imports nothing +// from the module, so it adds no importer to internal/errs (fan-in ratchet). +// Constraints: no I/O, no normalisation. The shape is exactly the one +// plugin/manifest.go enforces on installed plugin names (lowercase letters, +// digits and hyphen, starting with a letter). No '.' or '_': a dotted backup +// directory such as "notify.prev" is not a plugin, and sanitizeSchemaName maps +// '-' and '_' to the same schema so '_' would make two names collide. +package pluginname + +import ( + "path/filepath" + "regexp" + "strings" +) + +// PgIdentMax is PostgreSQL's identifier limit in bytes (NAMEDATALEN-1). Longer +// identifiers are silently truncated, so two long names could share one schema. +const PgIdentMax = 63 + +// longestIdentAffix is the extra bytes around a plugin name in the longest +// identifier generated from it: "np_" + name + "_role". +const longestIdentAffix = len("np_") + len("_role") + +// MaxLen is the longest accepted plugin name: the longest generated identifier +// (np__role) must fit in PgIdentMax bytes. The longest real plugin name +// in the tree is far below this. +const MaxLen = PgIdentMax - longestIdentAffix + +// SlugRe is the allowed shape: a lowercase letter, then lowercase letters, +// digits or '-'. +var SlugRe = regexp.MustCompile(`^[a-z][a-z0-9-]*$`) + +// Reserved are plugin-shaped names that already mean something else to the +// CLI, so no plugin may take them. Every entry is derived from code that creates +// or reads the object, and a test in internal/plugin +// (TestReservedNamesCoverSharedSchemas) fails when a new shared np_ schema +// appears without a matching entry here. +// +// Direct children of the plugin directory that are NOT plugins: +// - "bin": the shared command-binary directory (router.go pluginBinDir, +// skipped by mount/discover.go). `plugin remove bin` would delete every +// published binary. +// - ".nself-state" (bundle ledger), ".install.lock", ".staging-thirdparty-*", +// ".bundle-rollback-*" and ".prev" backups all start with '.' or hold a +// '.', so SlugRe already refuses them; TestInfrastructureEntriesNeverSlugs +// pins that. +// +// Schemas and roles a plugin name can map onto (np_, and +// np__role): +// - "common": schema np_common, the shared ledger schema (np_common.schema_versions, +// np_common.plugin_schema_versions) created by database/migrate_ledger.go and +// plugin/schema_reconcile.go. Removing plugin "common" would run +// DROP SCHEMA "np_common" CASCADE. +// - "status": schema np_status, created by the licensed nself-status-page plugin +// (plugins-pro). A table-owning plugin named "status" removed without +// --keep-data would run DROP SCHEMA "np_status" CASCADE and delete the +// status-page data. +// +// np_plugins and np_ai_usage are tables, not schemas, so they do not collide. +var Reserved = map[string]string{ + "bin": "it is the shared plugin command-binary directory", + "common": "it maps to np_common, the shared ledger schema", + "status": "it maps to np_status, the schema of the nself-status-page plugin", +} + +// SlugReason returns why name is not a plain single-element slug, or "". +func SlugReason(name string) string { + switch { + case name == "": + return "it is empty" + case name == "." || name == "..": + return "it is a directory reference" + case len(name) > MaxLen: + return "it is too long" + case strings.ContainsAny(name, `/\`) || strings.ContainsRune(name, filepath.Separator): + return "it contains a path separator" + case filepath.IsAbs(name) || filepath.VolumeName(name) != "": + return "it is an absolute path" + case strings.HasPrefix(name, "-"): + return "it starts with '-'" + case HasControlChar(name): + return "it contains a control character" + case filepath.Base(name) != name: + return "it is not a single path element" + case !SlugRe.MatchString(name): + return "plugin names use lowercase letters, digits and '-', and start with a letter" + } + return "" +} + +// Reason returns why name is not a plain plugin slug that is safe to join to +// the plugin directory and not a reserved infrastructure name, or "" when it is. +// It rejects the empty string, ".", "..", any path separator (slash or +// backslash, and the OS separator), absolute paths, a leading '-', NUL and +// other control characters, whitespace, and anything filepath.Base would change. +func Reason(name string) string { + if reason := SlugReason(name); reason != "" { + return reason + } + if reason := Reserved[name]; reason != "" { + return "it is reserved: " + reason + } + return "" +} + +// HasControlChar reports whether s holds NUL, any other ASCII control +// character, or DEL. +func HasControlChar(s string) bool { + for _, r := range s { + if r < 0x20 || r == 0x7f { + return true + } + } + return false +} diff --git a/internal/pluginname/name_test.go b/internal/pluginname/name_test.go new file mode 100644 index 000000000..31f586d37 --- /dev/null +++ b/internal/pluginname/name_test.go @@ -0,0 +1,30 @@ +package pluginname + +import ( + "strings" + "testing" +) + +func TestReason(t *testing.T) { + for _, ok := range []string{"notify", "ai", "a-b-c", "x1", strings.Repeat("a", MaxLen)} { + if r := Reason(ok); r != "" { + t.Errorf("Reason(%q) = %q, want empty", ok, r) + } + } + for _, bad := range []string{"", ".", "..", "../x", "a/b", `a\b`, "/abs", "-x", "a\x00b", "a b", "Notify", "notify.prev", "a_b", "1a", + "bin", "common", "status", strings.Repeat("a", MaxLen+1)} { + if Reason(bad) == "" { + t.Errorf("Reason(%q) is empty, want a reason", bad) + } + } +} + +func TestSlugReasonAllowsReservedNames(t *testing.T) { + // Reserved names are plain slugs: only Reason refuses them, so + // ValidateCommandName (which uses SlugReason) can still accept "status". + for name := range Reserved { + if r := SlugReason(name); r != "" { + t.Errorf("SlugReason(%q) = %q, want empty", name, r) + } + } +} diff --git a/internal/reconcile/handedit.go b/internal/reconcile/handedit.go index 074a8a7cb..c13ecded8 100644 --- a/internal/reconcile/handedit.go +++ b/internal/reconcile/handedit.go @@ -54,8 +54,12 @@ func (s *GeneratedState) HandEdited(path string, current []byte) bool { // resolves each display path to its location on disk and reads the file's // current bytes. A nil record (no state yet) answers false for everything: // the first run on an existing project records and never blocks. +// "@plugins/" paths resolve under the plugin directory the project's env +// cascade names (NSELF_PLUGIN_DIR in .env wins over the shell), the directory +// a build resolves after config.Load, so a caller outside a build (doctor) +// judges the same files the build records (D-0644). func (s *GeneratedState) HandEditedFn(projectDir string) func(path string) bool { - return s.handEditedFnWith(projectDir, nil, "") + return s.handEditedFnWith(projectDir, nil, strings.TrimSpace(cascadeValue(projectDir, nil, "NSELF_PLUGIN_DIR"))) } // handEditedFnWith is HandEditedFn with the env files a pending change will diff --git a/internal/reconcile/handedit_plugindir_test.go b/internal/reconcile/handedit_plugindir_test.go new file mode 100644 index 000000000..a8189e264 --- /dev/null +++ b/internal/reconcile/handedit_plugindir_test.go @@ -0,0 +1,40 @@ +package reconcile + +// Purpose: HandEditedFn resolves "@plugins/" paths under the plugin directory +// the project's env cascade names, not the shell's (D-0644). +// Inputs: a record, a project whose .env sets NSELF_PLUGIN_DIR, and a shell +// that leaves it unset or points it at a different directory. +// Outputs: pass/fail on whether a hand-edited fragment is seen. +// Constraints: temp dirs only. + +import ( + "os" + "path/filepath" + "testing" +) + +func TestHandEditedFnPluginDirOnlyInProjectEnv(t *testing.T) { + for _, tc := range shellPluginDirs { + t.Run(tc.name, func(t *testing.T) { + isolateEnv(t) + project, plugins := t.TempDir(), t.TempDir() + tc.shell(t) + if err := os.WriteFile(filepath.Join(project, ".env"), []byte("NSELF_PLUGIN_DIR="+plugins+"\n"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(plugins, "p.yml"), []byte("edited\n"), 0o644); err != nil { + t.Fatal(err) + } + s := &GeneratedState{Files: map[string]string{PluginsPrefix + "p.yml": sha256Hex([]byte("rendered\n"))}} + if !s.HandEditedFn(project)(PluginsPrefix + "p.yml") { + t.Fatal("a fragment edited under the .env plugin directory must be seen") + } + if err := os.WriteFile(filepath.Join(plugins, "p.yml"), []byte("rendered\n"), 0o644); err != nil { + t.Fatal(err) + } + if s.HandEditedFn(project)(PluginsPrefix + "p.yml") { + t.Fatal("an unchanged fragment must not report drift") + } + }) + } +} diff --git a/internal/reconcile/state.go b/internal/reconcile/state.go index 0ab728b3c..a1e65999f 100644 --- a/internal/reconcile/state.go +++ b/internal/reconcile/state.go @@ -144,8 +144,9 @@ func PlannedRecord(state *GeneratedState, pb *nbuild.PlannedBuild) (*GeneratedSt keep[displayPath(pb.PluginDir, key)] = true } // D-0318: the build no longer rewrites shared plugin fragments, so a - // legacy "@plugins/" key (D-0557) can only go stale and leaves the record. - next.pruneLegacyPluginKeys(keep) + // legacy "@plugins/" key (D-0557), or an "@abs/" key for a plugin fragment + // (D-0644), can only go stale and leaves the record. + next.pruneLegacyPluginKeys(keep, pb.PluginDir) body, err := next.body() if err != nil { return nil, nil, err diff --git a/internal/reconcile/state_prune.go b/internal/reconcile/state_prune.go index b97165279..1ce815255 100644 --- a/internal/reconcile/state_prune.go +++ b/internal/reconcile/state_prune.go @@ -1,7 +1,7 @@ package reconcile // state_prune.go — drop legacy plugin-fragment keys from the generated-state -// record (debts D-0557, D-0318). +// record (debts D-0557, D-0318, D-0644). // // Purpose: before D-0318 the build rewrote a plugin's compose fragment inside // the shared plugin directory and recorded it as @@ -9,19 +9,44 @@ package reconcile // project-local overlay (.nself/plugins//, a project file) and never // touches the shared fragment, so an old "@plugins/" key can only go stale: a // later plugin update changes the shared file and doctor would read the record -// as a hand edit forever. -// Inputs: the record being folded and the set of keys the build writes this run. -// Outputs: the record's Files without any "@plugins/" key the build is not -// writing. +// as a hand edit forever. A record written before cli#695 named the same file +// "@abs/" when the shell's plugin directory differed from the build's; +// those keys go stale the same way (D-0644). +// Inputs: the record being folded, the set of keys the build writes this run +// and the build's plugin directory ("" means DefaultPluginDir). +// Outputs: the record's Files without any "@plugins/" key, nor any "@abs/" key +// under the plugin directory, that the build is not writing. // Constraints: pure on the record (no disk), so plan == apply stays exact. -import "strings" +import ( + "path/filepath" + "strings" +) -// pruneLegacyPluginKeys deletes every "@plugins/" key except those in keep. -func (s *GeneratedState) pruneLegacyPluginKeys(keep map[string]bool) { +// pruneLegacyPluginKeys deletes every "@plugins/" key except those in keep, and +// every "@abs/" key that lies under pluginDir (a plugin fragment recorded under +// its absolute name before cli#695). +func (s *GeneratedState) pruneLegacyPluginKeys(keep map[string]bool, pluginDir string) { for key := range s.Files { - if strings.HasPrefix(key, PluginsPrefix) && !keep[key] { + if keep[key] { + continue + } + if strings.HasPrefix(key, PluginsPrefix) || absPluginKey(key, pluginDir) { delete(s.Files, key) } } } + +// absPluginKey reports whether key is an "@abs/" key for a file under pluginDir. +func absPluginKey(key, pluginDir string) bool { + if !strings.HasPrefix(key, AbsPrefix) { + return false + } + // The key drops only the leading "/" of the path (overlay.display), so a + // Windows drive path ("C:/x") is already absolute and needs no separator. + abs := filepath.FromSlash(strings.TrimPrefix(key, AbsPrefix)) + if !filepath.IsAbs(abs) { + abs = string(filepath.Separator) + abs + } + return strings.HasPrefix(displayPath(pluginDir, abs), PluginsPrefix) +} diff --git a/internal/reconcile/state_prune_test.go b/internal/reconcile/state_prune_test.go index 16bfd219a..fc7e1a4e8 100644 --- a/internal/reconcile/state_prune_test.go +++ b/internal/reconcile/state_prune_test.go @@ -30,7 +30,7 @@ func TestPruneLegacyPluginKeys(t *testing.T) { PluginsPrefix + "planned/docker-compose.plugin.yml": "w", PluginsPrefix + "flat.yml": "f", }} - s.pruneLegacyPluginKeys(map[string]bool{PluginsPrefix + "planned/docker-compose.plugin.yml": true}) + s.pruneLegacyPluginKeys(map[string]bool{PluginsPrefix + "planned/docker-compose.plugin.yml": true}, "") for _, k := range []string{"docker-compose.yml", ".nself/plugins/live/docker-compose.plugin.yml", PluginsPrefix + "planned/docker-compose.plugin.yml"} { if _, ok := s.Files[k]; !ok { @@ -164,3 +164,36 @@ func TestPluginDirOnlyInProjectEnv(t *testing.T) { }) } } + +// TestPlannedRecordDropsLegacyAbsPluginKeys (D-0644): a plugin fragment that +// an old record named "@abs/" (the shell's plugin directory differed from +// the build's) is under the build's plugin directory, so it leaves the record +// like its "@plugins/" twin. An "@abs/" key outside the plugin directory stays. +func TestPlannedRecordDropsLegacyAbsPluginKeys(t *testing.T) { + isolateEnv(t) + plugins := t.TempDir() + abs := func(p string) string { return AbsPrefix + strings.TrimPrefix(filepath.ToSlash(p), "/") } + legacy := abs(filepath.Join(plugins, "alpha", "docker-compose.plugin.yml")) + outside := abs(filepath.Join(t.TempDir(), "elsewhere.conf")) + cur := &GeneratedState{Files: map[string]string{legacy: "old", outside: "keep", "docker-compose.yml": "c"}} + pb := &nbuild.PlannedBuild{Files: map[string]nbuild.PlannedFile{}, PluginDir: plugins} + + next, body, err := PlannedRecord(cur, pb) + if err != nil { + t.Fatal(err) + } + if _, ok := next.Files[legacy]; ok { + t.Fatalf("legacy @abs/ plugin key must leave the record: %v", next.Files) + } + for _, k := range []string{outside, "docker-compose.yml"} { + if _, ok := next.Files[k]; !ok { + t.Fatalf("%s must stay in the record: %v", k, next.Files) + } + } + if _, ok := cur.Files[legacy]; !ok { + t.Fatal("PlannedRecord must not mutate the current record") + } + if _, again, err := PlannedRecord(next, pb); err != nil || string(again) != string(body) { + t.Fatalf("folding twice must be idempotent: %v", err) + } +} diff --git a/internal/repoqa/docker_funnel_test.go b/internal/repoqa/docker_funnel_test.go index 90e5401a9..8d25fc52f 100644 --- a/internal/repoqa/docker_funnel_test.go +++ b/internal/repoqa/docker_funnel_test.go @@ -5,7 +5,7 @@ import "testing" // dockerBasisTotal is the hard-coded total of testdata/docker-exec-allowlist.txt // (rule D4). The allowlist may only shrink: lowering a line without lowering // this constant fails, and raising either needs a reviewed edit of both. -const dockerBasisTotal = 107 +const dockerBasisTotal = 102 // TestDockerFunnel freezes direct docker process execution outside // internal/docker (EPIC G1, G4). diff --git a/internal/repoqa/testdata/docker-exec-allowlist.txt b/internal/repoqa/testdata/docker-exec-allowlist.txt index 200d93118..70036f7df 100644 --- a/internal/repoqa/testdata/docker-exec-allowlist.txt +++ b/internal/repoqa/testdata/docker-exec-allowlist.txt @@ -6,17 +6,15 @@ cmd/commands/clean.go 2 cmd/commands/db_admin.go 3 cmd/commands/db_migrate.go 1 cmd/commands/deploy_diagnostics.go 1 -cmd/commands/deploy_follow.go 1 cmd/commands/deploy_lifecycle.go 1 cmd/commands/doctor_checks_env.go 3 cmd/commands/functions_deploy.go 1 -cmd/commands/functions_logs.go 1 cmd/commands/logs.go 1 cmd/commands/logs_metrics.go 2 cmd/commands/logs_reports.go 1 -cmd/commands/logs_stream.go 2 +cmd/commands/logs_stream.go 1 cmd/commands/mcp_tools_ops.go 1 -cmd/commands/plugin_logs.go 4 +cmd/commands/plugin_logs.go 2 cmd/commands/plugin_test_cmd.go 2 cmd/commands/security_checks_system.go 1 cmd/commands/start_health.go 1