From d78a4b6323e7fc7de1b93165dec0d9baabf113d7 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 13:46:59 -0400 Subject: [PATCH 01/11] fix(db): verify-checksums --plugin and the database layer refuse backup and reserved plugin names (D-0602) --- cmd/commands/db_hasura.go | 5 +- .../db_verify_checksums_plugin_test.go | 29 ++++ internal/database/plugin_arg.go | 29 ++-- internal/database/plugin_arg_test.go | 2 +- internal/plugin/name_validate.go | 124 +++------------- internal/pluginname/name.go | 132 ++++++++++++++++++ internal/pluginname/name_test.go | 33 +++++ 7 files changed, 228 insertions(+), 126 deletions(-) create mode 100644 cmd/commands/db_verify_checksums_plugin_test.go create mode 100644 internal/pluginname/name.go create mode 100644 internal/pluginname/name_test.go 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..2088464e2 --- /dev/null +++ b/cmd/commands/db_verify_checksums_plugin_test.go @@ -0,0 +1,29 @@ +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" + "testing" + + "github.com/nself-org/cli/internal/errs" + "github.com/spf13/cobra" +) + +func TestDBVerifyChecksumsRefusesNonPluginNames(t *testing.T) { + t.Chdir(t.TempDir()) // no project here: the guard must fire before the config load + 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) + } + } +} diff --git a/internal/database/plugin_arg.go b/internal/database/plugin_arg.go index fa551fe93..40da9cf41 100644 --- a/internal/database/plugin_arg.go +++ b/internal/database/plugin_arg.go @@ -1,31 +1,22 @@ 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.Validate, 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" +import "github.com/nself-org/cli/internal/pluginname" - "github.com/nself-org/cli/internal/errs" -) - -// 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) - } - return nil + return pluginname.Validate(plugin) } 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/plugin/name_validate.go b/internal/plugin/name_validate.go index 2d8bfa0c0..45a8706dc 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,44 @@ 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 == "" { - 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 pluginname.Validate(name) } // 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 +76,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..ba470bc0c --- /dev/null +++ b/internal/pluginname/name.go @@ -0,0 +1,132 @@ +// 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: 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 +// 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" + + "github.com/nself-org/cli/internal/errs" +) + +// 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 "" +} + +// Validate 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. +func Validate(name string) error { + reason := SlugReason(name) + if reason == "" { + reason = Reserved[name] + if reason != "" { + reason = "it is reserved: " + reason + } + } + 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'") +} + +// 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..59aa60e46 --- /dev/null +++ b/internal/pluginname/name_test.go @@ -0,0 +1,33 @@ +package pluginname + +import ( + "strings" + "testing" + + "github.com/nself-org/cli/internal/errs" +) + +func TestValidate(t *testing.T) { + for _, ok := range []string{"notify", "ai", "a-b-c", "x1", strings.Repeat("a", MaxLen)} { + if err := Validate(ok); err != nil { + t.Errorf("Validate(%q) = %v, want nil", ok, err) + } + } + 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)} { + err := Validate(bad) + if d := errs.Describe(err); d == nil || d.Code != "E401" { + t.Errorf("Validate(%q) = %v, want E401", bad, err) + } + } +} + +func TestSlugReasonAllowsReservedNames(t *testing.T) { + // Reserved names are plain slugs: only Validate 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) + } + } +} From ddcacb1edf33ceb5f7d8e7ae8bfd7e3599045bb5 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 13:47:30 -0400 Subject: [PATCH 02/11] fix(docker): StartMarks drops a container removed between ps and inspect (D-0589) --- internal/docker/compose_states.go | 13 +++++++++--- internal/docker/compose_states_bound_test.go | 21 ++++++++++++++++++++ 2 files changed, 31 insertions(+), 3 deletions(-) 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 From b30b3f93a1ce1a368007a30b7dc844e55e7433cb Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 13:52:25 -0400 Subject: [PATCH 03/11] fix(reconcile): resolve @plugins/ keys from the project env cascade for doctor and prune legacy @abs/ plugin keys (D-0644) --- cmd/commands/doctor_drift_plugindir_test.go | 43 +++++++++++++++++++ internal/reconcile/handedit.go | 6 ++- internal/reconcile/handedit_plugindir_test.go | 40 +++++++++++++++++ internal/reconcile/state.go | 5 ++- internal/reconcile/state_prune.go | 38 ++++++++++++---- internal/reconcile/state_prune_test.go | 35 ++++++++++++++- 6 files changed, 154 insertions(+), 13 deletions(-) create mode 100644 cmd/commands/doctor_drift_plugindir_test.go create mode 100644 internal/reconcile/handedit_plugindir_test.go 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/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..56ca36c5d 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,39 @@ 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 + } + abs := string(filepath.Separator) + filepath.FromSlash(strings.TrimPrefix(key, AbsPrefix)) + 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) + } +} From d5422ecca335fd9eff602e3ab025f6806d566a91 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Sun, 25 Oct 2026 13:55:03 -0400 Subject: [PATCH 04/11] refactor(docker): stream logs -f through docker.Stream with a process group and signal stop (D-0711) --- cmd/commands/deploy_follow.go | 8 +- cmd/commands/functions_logs.go | 39 +--- cmd/commands/logs_stream.go | 56 +----- cmd/commands/plugin_logs.go | 24 +-- internal/docker/compose_unix.go | 10 + internal/docker/compose_windows.go | 8 + internal/docker/stream.go | 109 +++++++++++ internal/docker/stream_test.go | 174 ++++++++++++++++++ internal/repoqa/docker_funnel_test.go | 2 +- .../repoqa/testdata/docker-exec-allowlist.txt | 6 +- 10 files changed, 328 insertions(+), 108 deletions(-) create mode 100644 internal/docker/stream.go create mode 100644 internal/docker/stream_test.go 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/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..cf2374254 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,27 +76,20 @@ 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() + _ = 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 { 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..232c7cb39 --- /dev/null +++ b/internal/docker/stream_test.go @@ -0,0 +1,174 @@ +//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) { + marks := longRunning(t) + 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")) + if err := syscall.Kill(os.Getpid(), syscall.SIGINT); err != nil { + t.Fatal(err) + } + 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/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 From 7392240c0b23882cf49bed9f89f47841595f25e8 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Mon, 26 Oct 2026 16:22:16 -0400 Subject: [PATCH 05/11] fix(pluginname): return the reason, not an errs error, so the new package adds no internal/errs importer (D-0602) --- internal/database/plugin_arg.go | 13 +++++++++--- internal/plugin/name_validate.go | 6 +++++- internal/pluginname/name.go | 34 +++++++++++++------------------- internal/pluginname/name_test.go | 15 ++++++-------- 4 files changed, 35 insertions(+), 33 deletions(-) diff --git a/internal/database/plugin_arg.go b/internal/database/plugin_arg.go index 40da9cf41..1c7479ec2 100644 --- a/internal/database/plugin_arg.go +++ b/internal/database/plugin_arg.go @@ -6,11 +6,14 @@ package database // 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. The grammar is pluginname.Validate, the same check +// 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 "github.com/nself-org/cli/internal/pluginname" +import ( + "github.com/nself-org/cli/internal/errs" + "github.com/nself-org/cli/internal/pluginname" +) // validatePluginArg refuses a non-empty plugin argument that is not a plain, // non-reserved plugin name. @@ -18,5 +21,9 @@ func validatePluginArg(plugin string) error { if plugin == "" { return nil } - return pluginname.Validate(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/plugin/name_validate.go b/internal/plugin/name_validate.go index 45a8706dc..7482a0d2d 100644 --- a/internal/plugin/name_validate.go +++ b/internal/plugin/name_validate.go @@ -44,7 +44,11 @@ var binaryNameRe = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._-]*$`) // 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. func ValidateName(name string) error { - return pluginname.Validate(name) + 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'") + } + return nil } // ValidateCommandName reports whether cmd is a plain command name: the part after diff --git a/internal/pluginname/name.go b/internal/pluginname/name.go index ba470bc0c..a74a2c688 100644 --- a/internal/pluginname/name.go +++ b/internal/pluginname/name.go @@ -8,8 +8,9 @@ // "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: nil for a plain plugin slug, or an E401 (invalid usage) error naming -// the rule that failed. +// 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 @@ -21,8 +22,6 @@ import ( "path/filepath" "regexp" "strings" - - "github.com/nself-org/cli/internal/errs" ) // PgIdentMax is PostgreSQL's identifier limit in bytes (NAMEDATALEN-1). Longer @@ -100,24 +99,19 @@ func SlugReason(name string) string { return "" } -// Validate 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. -func Validate(name string) error { - reason := SlugReason(name) - if reason == "" { - reason = Reserved[name] - if reason != "" { - reason = "it is reserved: " + reason - } +// 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 == "" { - return nil + if reason := Reserved[name]; reason != "" { + return "it is reserved: " + 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'") + return "" } // HasControlChar reports whether s holds NUL, any other ASCII control diff --git a/internal/pluginname/name_test.go b/internal/pluginname/name_test.go index 59aa60e46..31f586d37 100644 --- a/internal/pluginname/name_test.go +++ b/internal/pluginname/name_test.go @@ -3,27 +3,24 @@ package pluginname import ( "strings" "testing" - - "github.com/nself-org/cli/internal/errs" ) -func TestValidate(t *testing.T) { +func TestReason(t *testing.T) { for _, ok := range []string{"notify", "ai", "a-b-c", "x1", strings.Repeat("a", MaxLen)} { - if err := Validate(ok); err != nil { - t.Errorf("Validate(%q) = %v, want nil", ok, err) + 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)} { - err := Validate(bad) - if d := errs.Describe(err); d == nil || d.Code != "E401" { - t.Errorf("Validate(%q) = %v, want E401", bad, err) + if Reason(bad) == "" { + t.Errorf("Reason(%q) is empty, want a reason", bad) } } } func TestSlugReasonAllowsReservedNames(t *testing.T) { - // Reserved names are plain slugs: only Validate refuses them, so + // 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 != "" { From 5aa36bc7b5ccddaf07c522ec58a6a69fe05fb1be Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Mon, 26 Oct 2026 17:42:15 -0400 Subject: [PATCH 06/11] fix(ci): keep the backup credential tests' .env out of the process env; read Windows drive @abs/ keys as absolute (D-0275, D-0644) --- cmd/commands/backup_credentials_test.go | 3 +++ internal/reconcile/state_prune.go | 7 ++++++- 2 files changed, 9 insertions(+), 1 deletion(-) 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/internal/reconcile/state_prune.go b/internal/reconcile/state_prune.go index 56ca36c5d..1ce815255 100644 --- a/internal/reconcile/state_prune.go +++ b/internal/reconcile/state_prune.go @@ -42,6 +42,11 @@ func absPluginKey(key, pluginDir string) bool { if !strings.HasPrefix(key, AbsPrefix) { return false } - abs := string(filepath.Separator) + filepath.FromSlash(strings.TrimPrefix(key, AbsPrefix)) + // 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) } From 60f1c26d32721d89686e400c5ef91deb027f8ea1 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Mon, 26 Oct 2026 18:20:33 -0400 Subject: [PATCH 07/11] test(db): make the verify-checksums --plugin test fail without the validatePluginFlag guard (D-0602) --- cmd/commands/db_verify_checksums_plugin_test.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/cmd/commands/db_verify_checksums_plugin_test.go b/cmd/commands/db_verify_checksums_plugin_test.go index 2088464e2..6aab6852e 100644 --- a/cmd/commands/db_verify_checksums_plugin_test.go +++ b/cmd/commands/db_verify_checksums_plugin_test.go @@ -6,6 +6,7 @@ package commands import ( "context" + "strings" "testing" "github.com/nself-org/cli/internal/errs" @@ -13,7 +14,11 @@ import ( ) func TestDBVerifyChecksumsRefusesNonPluginNames(t *testing.T) { - t.Chdir(t.TempDir()) // no project here: the guard must fire before the config load + // 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", "", "") @@ -25,5 +30,8 @@ func TestDBVerifyChecksumsRefusesNonPluginNames(t *testing.T) { 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) + } } } From 89876d6e7bd1b98a9bce6330dce9d484118ea371 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Mon, 26 Oct 2026 19:39:53 -0400 Subject: [PATCH 08/11] test(deploy): isolate config keys in the remote-deploy fixtures so earlier project loads cannot trip the v1.5 process-only guard (D-0260, D-0275) --- cmd/commands/deploy_env_resolve_test.go | 2 ++ cmd/commands/deploy_env_scope_test.go | 1 + cmd/commands/deploy_scratch_nginx_test.go | 1 + 3 files changed, 4 insertions(+) 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_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") From 15972849ff2b3736cca6ce1504011a8cd9257cb7 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Mon, 26 Oct 2026 19:50:46 -0400 Subject: [PATCH 09/11] fix(plugin): return the docker logs start/exec error in the --grep path instead of swallowing it (D-0711) --- cmd/commands/plugin_logs.go | 14 +++++++--- cmd/commands/plugin_logs_test.go | 48 ++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 4 deletions(-) create mode 100644 cmd/commands/plugin_logs_test.go diff --git a/cmd/commands/plugin_logs.go b/cmd/commands/plugin_logs.go index cf2374254..2a402885c 100644 --- a/cmd/commands/plugin_logs.go +++ b/cmd/commands/plugin_logs.go @@ -88,15 +88,21 @@ func runPluginLogs(cmd *cobra.Command, args []string) error { } // Wait for docker to finish, then grep will EOF. - _ = docker.Stream(cmd.Context(), "", dockerArgs, grepIn, os.Stderr) + 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) + } +} From f75dc445c88a4a883f81deaaaf55a9036c701305 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Mon, 26 Oct 2026 21:07:54 -0400 Subject: [PATCH 10/11] ci(coverage): add a floor for internal/pluginname so the strict covfloor gate stops failing on an unfloored package (D-0602) --- .github/coverage-floors.txt | 1 + 1 file changed, 1 insertion(+) 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 From 931adf18277b21e5bb7d376187680ac3478d9d48 Mon Sep 17 00:00:00 2001 From: Aric Camarata Date: Tue, 27 Oct 2026 06:35:28 -0400 Subject: [PATCH 11/11] fix(test): avoid SIGINT race in docker stream test on Linux CI TestStreamSIGINTStopsTheChildNotTheProcess sent SIGINT to the test process itself, which races with signal.NotifyContext registration inside Stream's goroutine on non-TTY Linux runners: if the signal arrives before the handler is installed, the default action kills the test binary. Have the fake docker send SIGINT to its parent after it starts instead, guaranteeing the handler is ready. --- internal/docker/stream_test.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/internal/docker/stream_test.go b/internal/docker/stream_test.go index 232c7cb39..c3d4d3c72 100644 --- a/internal/docker/stream_test.go +++ b/internal/docker/stream_test.go @@ -113,15 +113,19 @@ func TestStreamContextStopLeavesNoOrphan(t *testing.T) { // TestStreamSIGINTStopsTheChildNotTheProcess: Ctrl-C reaches the stream through // its NotifyContext; the test process survives and the child group is gone. func TestStreamSIGINTStopsTheChildNotTheProcess(t *testing.T) { - marks := longRunning(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")) - if err := syscall.Kill(os.Getpid(), syscall.SIGINT); err != nil { - t.Fatal(err) - } select { case err := <-res: if err != nil {