From cf958b5f53e0ce3455e89174cdf626cfe4c35bd9 Mon Sep 17 00:00:00 2001 From: Tym Rabchuk Date: Sat, 18 Apr 2026 11:25:28 -0400 Subject: [PATCH 1/3] feat(cmd): add approve subcommand for workflow gates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `devkit approve ` which writes `.devkit/gates/.approved` with a timestamp and git-config approver identity. Idempotent on repeat invocation, rejects path-traversal and otherwise-unsafe names. Unblocks a polling-gate pattern for workflows where a shell step waits on a marker file before the pipeline continues — e.g. a plan-review gate in a long-running feature workflow. --- src/cmd/approve.go | 131 +++++++++++++++++++++++++++++++++++ src/cmd/approve_test.go | 146 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 277 insertions(+) create mode 100644 src/cmd/approve.go create mode 100644 src/cmd/approve_test.go diff --git a/src/cmd/approve.go b/src/cmd/approve.go new file mode 100644 index 0000000..52d9f4e --- /dev/null +++ b/src/cmd/approve.go @@ -0,0 +1,131 @@ +package cmd + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "regexp" + "strings" + "time" + + "github.com/spf13/cobra" +) + +// approveNamePattern bounds the gate name to chars that are safe as a +// filename on every target filesystem and unambiguously non-traversal. +// Rejecting leading `.` also prevents shadowing dotfiles like `.gitignore` +// if someone ever puts the gates dir in a more sensitive location. +var approveNamePattern = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9_-]{0,63}$`) + +var approveCmd = &cobra.Command{ + Use: "approve ", + Short: "Approve a workflow gate", + Long: `Approve a named gate so a polling gate step in a workflow can continue. + +A gate step in a workflow is a command that blocks until a marker file +exists under .devkit/gates/. This command writes that marker with a +timestamp and approver so the gate step can exit cleanly. + +The name must match [a-zA-Z0-9][a-zA-Z0-9_-]{0,63} — no path separators, +no leading dot, 1-64 chars.`, + Example: ` devkit approve plan + devkit approve deploy-prod`, + Args: cobra.ExactArgs(1), + // Skip the root PersistentPreRunE — approve only needs a repo root, + // not the sessions DB. Avoids creating .devkit/devkit.db for users + // who only want to approve a gate without ever running a workflow + // in this directory (e.g. CI runners approving from a script). + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { return nil }, + PersistentPostRunE: func(cmd *cobra.Command, args []string) error { return nil }, + RunE: func(cmd *cobra.Command, args []string) error { + return runApprove(args[0]) + }, +} + +func init() { + rootCmd.AddCommand(approveCmd) +} + +func runApprove(name string) error { + if !approveNamePattern.MatchString(name) { + return fmt.Errorf("invalid gate name %q — must match [a-zA-Z0-9][a-zA-Z0-9_-]{0,63}", name) + } + + root, err := findRepoRoot() + if err != nil { + return fmt.Errorf("not inside a git repo — run devkit approve from a project directory") + } + + gatesDir := filepath.Join(root, ".devkit", "gates") + if err := os.MkdirAll(gatesDir, 0o700); err != nil { + return fmt.Errorf("create gates dir: %w", err) + } + + markerPath := filepath.Join(gatesDir, name+".approved") + + if existing, err := os.ReadFile(markerPath); err == nil { + fmt.Printf("gate %q already approved:\n%s", name, existing) + return nil + } else if !os.IsNotExist(err) { + return fmt.Errorf("read existing marker: %w", err) + } + + approver := approverIdentity() + content := fmt.Sprintf("approved_at: %s\napproved_by: %s\n", + time.Now().UTC().Format(time.RFC3339), + approver) + + // O_EXCL so a concurrent `devkit approve` can't race us and end up + // with a half-written marker. The earlier ReadFile check handles the + // idempotent case; this handles the narrow race where two approvers + // hit it between the stat and the create. + f, err := os.OpenFile(markerPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) + if err != nil { + if os.IsExist(err) { + existing, rerr := os.ReadFile(markerPath) + if rerr == nil { + fmt.Printf("gate %q already approved:\n%s", name, existing) + return nil + } + } + return fmt.Errorf("write marker: %w", err) + } + if _, werr := f.WriteString(content); werr != nil { + f.Close() + os.Remove(markerPath) + return fmt.Errorf("write marker: %w", werr) + } + if cerr := f.Close(); cerr != nil { + return fmt.Errorf("close marker: %w", cerr) + } + + fmt.Printf("gate %q approved by %s\nmarker: %s\n", name, approver, markerPath) + return nil +} + +// approverIdentity prefers git config so the marker reflects the repo's +// commit-author identity (which is what reviewers recognise). Falls back +// through $USER and finally "unknown" so the marker is always written — +// refusing to approve because we can't identify the user would be worse +// than a marker that says "unknown". +func approverIdentity() string { + if name := gitConfig("user.name"); name != "" { + if email := gitConfig("user.email"); email != "" { + return fmt.Sprintf("%s <%s>", name, email) + } + return name + } + if u := strings.TrimSpace(os.Getenv("USER")); u != "" { + return u + } + return "unknown" +} + +func gitConfig(key string) string { + out, err := exec.Command("git", "config", "--get", key).Output() + if err != nil { + return "" + } + return strings.TrimSpace(string(out)) +} diff --git a/src/cmd/approve_test.go b/src/cmd/approve_test.go new file mode 100644 index 0000000..6ba6dd0 --- /dev/null +++ b/src/cmd/approve_test.go @@ -0,0 +1,146 @@ +package cmd + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +func TestApproveNameValidation(t *testing.T) { + tests := []struct { + name string + gate string + wantErr bool + }{ + {"simple", "plan", false}, + {"with dash", "deploy-prod", false}, + {"with underscore", "gate_1", false}, + {"alphanumeric", "Gate42", false}, + {"max length", strings.Repeat("a", 64), false}, + {"empty", "", true}, + {"too long", strings.Repeat("a", 65), true}, + {"leading dot", ".hidden", true}, + {"leading dash", "-flag", true}, + {"path traversal dotdot", "../escape", true}, + {"path separator unix", "a/b", true}, + {"path separator windows", "a\\b", true}, + {"space", "plan v2", true}, + {"null byte", "plan\x00", true}, + {"leading digit ok", "1plan", false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := approveNamePattern.MatchString(tc.gate) + if got == tc.wantErr { + t.Fatalf("name %q: match=%v, wantErr=%v", tc.gate, got, tc.wantErr) + } + }) + } +} + +func TestRunApproveWritesMarker(t *testing.T) { + dir := newTestRepo(t) + t.Chdir(dir) + + if err := runApprove("plan"); err != nil { + t.Fatalf("runApprove: %v", err) + } + + markerPath := filepath.Join(dir, ".devkit", "gates", "plan.approved") + data, err := os.ReadFile(markerPath) + if err != nil { + t.Fatalf("read marker: %v", err) + } + if !strings.Contains(string(data), "approved_at:") { + t.Errorf("marker missing approved_at: %q", data) + } + if !strings.Contains(string(data), "approved_by:") { + t.Errorf("marker missing approved_by: %q", data) + } +} + +func TestRunApproveIdempotent(t *testing.T) { + dir := newTestRepo(t) + t.Chdir(dir) + + if err := runApprove("plan"); err != nil { + t.Fatalf("first approve: %v", err) + } + markerPath := filepath.Join(dir, ".devkit", "gates", "plan.approved") + first, err := os.ReadFile(markerPath) + if err != nil { + t.Fatalf("read after first: %v", err) + } + + if err := runApprove("plan"); err != nil { + t.Fatalf("second approve: %v", err) + } + second, err := os.ReadFile(markerPath) + if err != nil { + t.Fatalf("read after second: %v", err) + } + + if string(first) != string(second) { + t.Errorf("marker content changed on idempotent re-approve\nfirst: %q\nsecond: %q", first, second) + } +} + +func TestRunApproveRejectsInvalidName(t *testing.T) { + dir := newTestRepo(t) + t.Chdir(dir) + + err := runApprove("../escape") + if err == nil { + t.Fatal("expected error for path traversal name") + } + if !strings.Contains(err.Error(), "invalid gate name") { + t.Errorf("expected 'invalid gate name' in error, got: %v", err) + } + + // Ensure no file was created at the traversal target. + escaped := filepath.Join(dir, "..", "escape.approved") + if _, err := os.Stat(escaped); err == nil { + t.Errorf("traversal created %s", escaped) + } +} + +func TestRunApproveOutsideRepo(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + + err := runApprove("plan") + if err == nil { + t.Fatal("expected error outside git repo") + } + if !strings.Contains(err.Error(), "not inside a git repo") { + t.Errorf("expected 'not inside a git repo' in error, got: %v", err) + } +} + +// newTestRepo creates a temp directory with a .git sentinel dir so +// findRepoRoot resolves to it. No real git history is needed — the repo +// root check is a single filepath.Stat for .git. We also pre-seed a +// minimal git config so approverIdentity doesn't reach up to the real +// user's ~/.gitconfig and leak their identity into a CI test log. +func newTestRepo(t *testing.T) string { + t.Helper() + dir := t.TempDir() + if err := os.Mkdir(filepath.Join(dir, ".git"), 0o755); err != nil { + t.Fatalf("mkdir .git: %v", err) + } + // Confine git config lookups to this repo so the test is hermetic. + t.Setenv("GIT_CONFIG_GLOBAL", filepath.Join(dir, ".gitconfig-global")) + t.Setenv("GIT_CONFIG_SYSTEM", "/dev/null") + t.Setenv("HOME", dir) + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not in PATH — approver identity test needs git") + } + cmd := exec.Command("git", "-C", dir, "init", "--quiet") + if err := cmd.Run(); err == nil { + _ = exec.Command("git", "-C", dir, "config", "user.name", "Test User").Run() + _ = exec.Command("git", "-C", dir, "config", "user.email", "test@example.com").Run() + } + return dir +} From febcd8049259aa92bf00b2ec852cfe39456e23d6 Mon Sep 17 00:00:00 2001 From: Tym Rabchuk Date: Sat, 18 Apr 2026 11:41:13 -0400 Subject: [PATCH 2/3] fix(cmd): address tri-review findings on approve subcommand Atomic marker publish (the NO-SHIP blocker) plus cheap cleanups. - Publish marker via temp-file + rename so a failed approve can no longer leave a visible marker that unblocks a polling gate - Single git subprocess via --get-regexp instead of two - 2s timeout on git config so a locked/misconfigured repo can't hang - Route output through cmd.OutOrStdout() for testability and redirection - Include regex anchors in the invalid-name error message - Test mtime preservation on idempotent re-approve; assert null-byte rejection happens at the regex layer; cover parseGitUserRegexp - Use os.DevNull instead of hardcoded /dev/null in test fixture --- src/cmd/approve.go | 110 +++++++++++++++++++++++++++------------- src/cmd/approve_test.go | 84 +++++++++++++++++++++++++++--- 2 files changed, 154 insertions(+), 40 deletions(-) diff --git a/src/cmd/approve.go b/src/cmd/approve.go index 52d9f4e..c577669 100644 --- a/src/cmd/approve.go +++ b/src/cmd/approve.go @@ -1,7 +1,9 @@ package cmd import ( + "context" "fmt" + "io" "os" "os/exec" "path/filepath" @@ -39,7 +41,7 @@ no leading dot, 1-64 chars.`, PersistentPreRunE: func(cmd *cobra.Command, args []string) error { return nil }, PersistentPostRunE: func(cmd *cobra.Command, args []string) error { return nil }, RunE: func(cmd *cobra.Command, args []string) error { - return runApprove(args[0]) + return runApprove(cmd.Context(), cmd.OutOrStdout(), args[0]) }, } @@ -47,9 +49,9 @@ func init() { rootCmd.AddCommand(approveCmd) } -func runApprove(name string) error { +func runApprove(ctx context.Context, out io.Writer, name string) error { if !approveNamePattern.MatchString(name) { - return fmt.Errorf("invalid gate name %q — must match [a-zA-Z0-9][a-zA-Z0-9_-]{0,63}", name) + return fmt.Errorf("invalid gate name %q — must match ^[a-zA-Z0-9][a-zA-Z0-9_-]{0,63}$", name) } root, err := findRepoRoot() @@ -63,44 +65,55 @@ func runApprove(name string) error { } markerPath := filepath.Join(gatesDir, name+".approved") - if existing, err := os.ReadFile(markerPath); err == nil { - fmt.Printf("gate %q already approved:\n%s", name, existing) + fmt.Fprintf(out, "gate %q already approved:\n%s", name, existing) return nil } else if !os.IsNotExist(err) { return fmt.Errorf("read existing marker: %w", err) } - approver := approverIdentity() + approver := approverIdentity(ctx) content := fmt.Sprintf("approved_at: %s\napproved_by: %s\n", time.Now().UTC().Format(time.RFC3339), approver) - // O_EXCL so a concurrent `devkit approve` can't race us and end up - // with a half-written marker. The earlier ReadFile check handles the - // idempotent case; this handles the narrow race where two approvers - // hit it between the stat and the create. - f, err := os.OpenFile(markerPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) + // Atomic publish: write to a temp file in the same directory, then + // rename. A polling gate does `[ -f ... ]`, which sees the marker + // only after rename makes the directory entry visible — so a crash + // or write error mid-approve can no longer unblock the workflow on + // a partially-written file. The O_EXCL-on-the-final-path approach + // was unsafe because the marker became visible before WriteString + // and Close had returned. + tmp, err := os.CreateTemp(gatesDir, name+".approved.tmp-*") if err != nil { - if os.IsExist(err) { - existing, rerr := os.ReadFile(markerPath) - if rerr == nil { - fmt.Printf("gate %q already approved:\n%s", name, existing) - return nil - } + return fmt.Errorf("create temp marker: %w", err) + } + tmpPath := tmp.Name() + committed := false + defer func() { + if !committed { + os.Remove(tmpPath) } - return fmt.Errorf("write marker: %w", err) + }() + + if _, err := tmp.WriteString(content); err != nil { + tmp.Close() + return fmt.Errorf("write temp marker: %w", err) } - if _, werr := f.WriteString(content); werr != nil { - f.Close() - os.Remove(markerPath) - return fmt.Errorf("write marker: %w", werr) + if err := tmp.Sync(); err != nil { + tmp.Close() + return fmt.Errorf("sync temp marker: %w", err) } - if cerr := f.Close(); cerr != nil { - return fmt.Errorf("close marker: %w", cerr) + if err := tmp.Close(); err != nil { + return fmt.Errorf("close temp marker: %w", err) + } + + if err := os.Rename(tmpPath, markerPath); err != nil { + return fmt.Errorf("publish marker: %w", err) } + committed = true - fmt.Printf("gate %q approved by %s\nmarker: %s\n", name, approver, markerPath) + fmt.Fprintf(out, "gate %q approved by %s\nmarker: %s\n", name, approver, markerPath) return nil } @@ -109,23 +122,52 @@ func runApprove(name string) error { // through $USER and finally "unknown" so the marker is always written — // refusing to approve because we can't identify the user would be worse // than a marker that says "unknown". -func approverIdentity() string { - if name := gitConfig("user.name"); name != "" { - if email := gitConfig("user.email"); email != "" { +// +// Runs git under a 2s timeout because `git config --get` has been +// observed to hang on locked-index or misconfigured repos, which would +// deadlock the CLI indefinitely. A single --get-regexp call fetches +// both user.name and user.email in one subprocess instead of two. +func approverIdentity(ctx context.Context) string { + tctx, cancel := context.WithTimeout(ctx, 2*time.Second) + defer cancel() + + out, err := exec.CommandContext(tctx, "git", "config", "--get-regexp", `^user\.(name|email)$`).Output() + if err == nil { + name, email := parseGitUserRegexp(string(out)) + switch { + case name != "" && email != "": return fmt.Sprintf("%s <%s>", name, email) + case name != "": + return name } - return name } + if u := strings.TrimSpace(os.Getenv("USER")); u != "" { return u } return "unknown" } -func gitConfig(key string) string { - out, err := exec.Command("git", "config", "--get", key).Output() - if err != nil { - return "" +// parseGitUserRegexp extracts user.name and user.email from the output +// of `git config --get-regexp`. Each line is " " separated +// by a single space; values may themselves contain spaces (e.g. a full +// name) so we split on the first space only. +func parseGitUserRegexp(out string) (name, email string) { + for _, line := range strings.Split(out, "\n") { + line = strings.TrimRight(line, "\r") + if line == "" { + continue + } + key, val, ok := strings.Cut(line, " ") + if !ok { + continue + } + switch key { + case "user.name": + name = val + case "user.email": + email = val + } } - return strings.TrimSpace(string(out)) + return } diff --git a/src/cmd/approve_test.go b/src/cmd/approve_test.go index 6ba6dd0..b492ea8 100644 --- a/src/cmd/approve_test.go +++ b/src/cmd/approve_test.go @@ -1,6 +1,9 @@ package cmd import ( + "bytes" + "context" + "io" "os" "os/exec" "path/filepath" @@ -44,7 +47,8 @@ func TestRunApproveWritesMarker(t *testing.T) { dir := newTestRepo(t) t.Chdir(dir) - if err := runApprove("plan"); err != nil { + var out bytes.Buffer + if err := runApprove(context.Background(), &out, "plan"); err != nil { t.Fatalf("runApprove: %v", err) } @@ -59,13 +63,16 @@ func TestRunApproveWritesMarker(t *testing.T) { if !strings.Contains(string(data), "approved_by:") { t.Errorf("marker missing approved_by: %q", data) } + if !strings.Contains(out.String(), "approved by") { + t.Errorf("stdout missing approval confirmation: %q", out.String()) + } } func TestRunApproveIdempotent(t *testing.T) { dir := newTestRepo(t) t.Chdir(dir) - if err := runApprove("plan"); err != nil { + if err := runApprove(context.Background(), io.Discard, "plan"); err != nil { t.Fatalf("first approve: %v", err) } markerPath := filepath.Join(dir, ".devkit", "gates", "plan.approved") @@ -73,31 +80,50 @@ func TestRunApproveIdempotent(t *testing.T) { if err != nil { t.Fatalf("read after first: %v", err) } + firstStat, err := os.Stat(markerPath) + if err != nil { + t.Fatalf("stat after first: %v", err) + } - if err := runApprove("plan"); err != nil { + if err := runApprove(context.Background(), io.Discard, "plan"); err != nil { t.Fatalf("second approve: %v", err) } second, err := os.ReadFile(markerPath) if err != nil { t.Fatalf("read after second: %v", err) } + secondStat, err := os.Stat(markerPath) + if err != nil { + t.Fatalf("stat after second: %v", err) + } if string(first) != string(second) { t.Errorf("marker content changed on idempotent re-approve\nfirst: %q\nsecond: %q", first, second) } + // mtime must be preserved on a no-op re-approve — if it changed we + // know the file was rewritten with identical content, which a naive + // content-equality check would miss. + if !firstStat.ModTime().Equal(secondStat.ModTime()) { + t.Errorf("marker mtime changed on idempotent re-approve: %v -> %v", firstStat.ModTime(), secondStat.ModTime()) + } } func TestRunApproveRejectsInvalidName(t *testing.T) { dir := newTestRepo(t) t.Chdir(dir) - err := runApprove("../escape") + err := runApprove(context.Background(), io.Discard, "../escape") if err == nil { t.Fatal("expected error for path traversal name") } if !strings.Contains(err.Error(), "invalid gate name") { t.Errorf("expected 'invalid gate name' in error, got: %v", err) } + // Error message should include the regex anchors so users don't + // read the allowlist as a partial-match pattern. + if !strings.Contains(err.Error(), "^") || !strings.Contains(err.Error(), "$") { + t.Errorf("expected regex anchors in error message, got: %v", err) + } // Ensure no file was created at the traversal target. escaped := filepath.Join(dir, "..", "escape.approved") @@ -106,11 +132,27 @@ func TestRunApproveRejectsInvalidName(t *testing.T) { } } +func TestRunApproveRejectsNullByteAtRegex(t *testing.T) { + // Null-byte rejection must happen at the regex layer, not via the + // filesystem refusing the name later. Otherwise the same input + // could sneak past on an OS with lax filename validation. + dir := newTestRepo(t) + t.Chdir(dir) + + err := runApprove(context.Background(), io.Discard, "plan\x00") + if err == nil { + t.Fatal("expected error for null-byte name") + } + if !strings.Contains(err.Error(), "invalid gate name") { + t.Errorf("expected regex-layer rejection, got: %v", err) + } +} + func TestRunApproveOutsideRepo(t *testing.T) { dir := t.TempDir() t.Chdir(dir) - err := runApprove("plan") + err := runApprove(context.Background(), io.Discard, "plan") if err == nil { t.Fatal("expected error outside git repo") } @@ -119,6 +161,34 @@ func TestRunApproveOutsideRepo(t *testing.T) { } } +func TestParseGitUserRegexp(t *testing.T) { + tests := []struct { + name string + input string + wantName string + wantEmail string + }{ + {"both present", "user.name Alice Example\nuser.email alice@example.com\n", "Alice Example", "alice@example.com"}, + {"name only", "user.name Bob\n", "Bob", ""}, + {"email only", "user.email c@d.io\n", "", "c@d.io"}, + {"empty", "", "", ""}, + {"crlf", "user.name Dana\r\nuser.email d@d.io\r\n", "Dana", "d@d.io"}, + {"blank lines", "\n\nuser.name Eve\n\n", "Eve", ""}, + {"name with spaces", "user.name Two Spaces\n", " Two Spaces", ""}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + n, e := parseGitUserRegexp(tc.input) + if n != tc.wantName { + t.Errorf("name: got %q, want %q", n, tc.wantName) + } + if e != tc.wantEmail { + t.Errorf("email: got %q, want %q", e, tc.wantEmail) + } + }) + } +} + // newTestRepo creates a temp directory with a .git sentinel dir so // findRepoRoot resolves to it. No real git history is needed — the repo // root check is a single filepath.Stat for .git. We also pre-seed a @@ -131,8 +201,10 @@ func newTestRepo(t *testing.T) string { t.Fatalf("mkdir .git: %v", err) } // Confine git config lookups to this repo so the test is hermetic. + // os.DevNull is portable across Unix and Windows ("/dev/null" vs + // "NUL"); hardcoding a Unix path would break the test on Windows. t.Setenv("GIT_CONFIG_GLOBAL", filepath.Join(dir, ".gitconfig-global")) - t.Setenv("GIT_CONFIG_SYSTEM", "/dev/null") + t.Setenv("GIT_CONFIG_SYSTEM", os.DevNull) t.Setenv("HOME", dir) if _, err := exec.LookPath("git"); err != nil { t.Skip("git not in PATH — approver identity test needs git") From ed954bd93631e3e782d7f6a8e7ef25de0cfba64b Mon Sep 17 00:00:00 2001 From: Tym Rabchuk Date: Sat, 18 Apr 2026 11:48:42 -0400 Subject: [PATCH 3/3] fix(cmd): address pr-review findings on approve subcommand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Cover approverIdentity fallback chain: $USER and "unknown" paths had no coverage because newTestRepo always seeded a valid git identity. Add isolateGitConfig helper + two tests so a regression that reorders or drops the fallback branches fails loudly. - Rename wantErr -> wantMatch in the name-validation table. The old field was readable backwards ("wantErr: false" meant "expect match") and a reviewer misread the assertion as a no-op. The logic was correct; the naming was the bug. - Backdate mtime between the two idempotent approves so the "file was not rewritten" check works on filesystems with 1-second mtime resolution (ext3, older HFS+, some CI overlays). Previously flaky off APFS/ext4 — the two approves finish in microseconds. - Log git failures in approverIdentity to stderr before falling back. Silent swallow left users with approver=unknown and no signal that git was consulted and failed (timeout, corrupt config, locked index). --- src/cmd/approve.go | 14 +++++- src/cmd/approve_test.go | 95 +++++++++++++++++++++++++++++------------ 2 files changed, 79 insertions(+), 30 deletions(-) diff --git a/src/cmd/approve.go b/src/cmd/approve.go index c577669..786611e 100644 --- a/src/cmd/approve.go +++ b/src/cmd/approve.go @@ -142,10 +142,20 @@ func approverIdentity(ctx context.Context) string { } } + fallback := "unknown" if u := strings.TrimSpace(os.Getenv("USER")); u != "" { - return u + fallback = u } - return "unknown" + // Surface git failures so the user can tell "git timed out" from + // "git wasn't configured" from "git config is corrupt" — without + // this, every such case silently produces approver= + // with no diagnostic trail. We only log when git actually failed + // (err != nil); a git run that succeeded but returned no user.* keys + // is a legitimate unconfigured-repo case and doesn't need noise. + if err != nil { + fmt.Fprintf(os.Stderr, "devkit approve: git identity unavailable (%v); approver=%s\n", err, fallback) + } + return fallback } // parseGitUserRegexp extracts user.name and user.email from the output diff --git a/src/cmd/approve_test.go b/src/cmd/approve_test.go index b492ea8..a848add 100644 --- a/src/cmd/approve_test.go +++ b/src/cmd/approve_test.go @@ -9,35 +9,36 @@ import ( "path/filepath" "strings" "testing" + "time" ) func TestApproveNameValidation(t *testing.T) { tests := []struct { - name string - gate string - wantErr bool + name string + gate string + wantMatch bool }{ - {"simple", "plan", false}, - {"with dash", "deploy-prod", false}, - {"with underscore", "gate_1", false}, - {"alphanumeric", "Gate42", false}, - {"max length", strings.Repeat("a", 64), false}, - {"empty", "", true}, - {"too long", strings.Repeat("a", 65), true}, - {"leading dot", ".hidden", true}, - {"leading dash", "-flag", true}, - {"path traversal dotdot", "../escape", true}, - {"path separator unix", "a/b", true}, - {"path separator windows", "a\\b", true}, - {"space", "plan v2", true}, - {"null byte", "plan\x00", true}, - {"leading digit ok", "1plan", false}, + {"simple", "plan", true}, + {"with dash", "deploy-prod", true}, + {"with underscore", "gate_1", true}, + {"alphanumeric", "Gate42", true}, + {"max length", strings.Repeat("a", 64), true}, + {"leading digit ok", "1plan", true}, + {"empty", "", false}, + {"too long", strings.Repeat("a", 65), false}, + {"leading dot", ".hidden", false}, + {"leading dash", "-flag", false}, + {"path traversal dotdot", "../escape", false}, + {"path separator unix", "a/b", false}, + {"path separator windows", "a\\b", false}, + {"space", "plan v2", false}, + {"null byte", "plan\x00", false}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { got := approveNamePattern.MatchString(tc.gate) - if got == tc.wantErr { - t.Fatalf("name %q: match=%v, wantErr=%v", tc.gate, got, tc.wantErr) + if got != tc.wantMatch { + t.Fatalf("name %q: match=%v, wantMatch=%v", tc.gate, got, tc.wantMatch) } }) } @@ -80,9 +81,15 @@ func TestRunApproveIdempotent(t *testing.T) { if err != nil { t.Fatalf("read after first: %v", err) } - firstStat, err := os.Stat(markerPath) - if err != nil { - t.Fatalf("stat after first: %v", err) + + // Backdate the marker's mtime so the "mtime preserved" check below + // works on filesystems with second-level mtime resolution (ext3, + // older HFS+, some CI overlays). Without this, two back-to-back + // approves finish in microseconds and the mtime comparison becomes + // fs-dependent — green on APFS/ext4, flaky on CI. + backdated := time.Now().Add(-10 * time.Second).Truncate(time.Second) + if err := os.Chtimes(markerPath, backdated, backdated); err != nil { + t.Fatalf("chtimes: %v", err) } if err := runApprove(context.Background(), io.Discard, "plan"); err != nil { @@ -100,11 +107,8 @@ func TestRunApproveIdempotent(t *testing.T) { if string(first) != string(second) { t.Errorf("marker content changed on idempotent re-approve\nfirst: %q\nsecond: %q", first, second) } - // mtime must be preserved on a no-op re-approve — if it changed we - // know the file was rewritten with identical content, which a naive - // content-equality check would miss. - if !firstStat.ModTime().Equal(secondStat.ModTime()) { - t.Errorf("marker mtime changed on idempotent re-approve: %v -> %v", firstStat.ModTime(), secondStat.ModTime()) + if !secondStat.ModTime().Equal(backdated) { + t.Errorf("marker mtime changed on idempotent re-approve: want %v, got %v — file was rewritten", backdated, secondStat.ModTime()) } } @@ -161,6 +165,26 @@ func TestRunApproveOutsideRepo(t *testing.T) { } } +func TestApproverIdentityFallbackToUser(t *testing.T) { + isolateGitConfig(t) + t.Setenv("USER", "alice") + + got := approverIdentity(context.Background()) + if got != "alice" { + t.Errorf("want 'alice' (USER fallback), got %q", got) + } +} + +func TestApproverIdentityFallbackToUnknown(t *testing.T) { + isolateGitConfig(t) + t.Setenv("USER", "") + + got := approverIdentity(context.Background()) + if got != "unknown" { + t.Errorf("want 'unknown' (final fallback), got %q", got) + } +} + func TestParseGitUserRegexp(t *testing.T) { tests := []struct { name string @@ -189,6 +213,21 @@ func TestParseGitUserRegexp(t *testing.T) { } } +// isolateGitConfig points git at empty config files for the duration of +// the test so approverIdentity sees no user.name/user.email anywhere — +// exercising the USER / "unknown" fallback paths that newTestRepo hides +// by pre-seeding a valid identity. Also switches HOME so any ambient +// ~/.gitconfig is ignored and we don't leak the real developer identity +// into test output on a machine that has a global git config. +func isolateGitConfig(t *testing.T) { + t.Helper() + dir := t.TempDir() + t.Setenv("GIT_CONFIG_GLOBAL", filepath.Join(dir, "empty-global")) + t.Setenv("GIT_CONFIG_SYSTEM", os.DevNull) + t.Setenv("HOME", dir) + t.Chdir(dir) +} + // newTestRepo creates a temp directory with a .git sentinel dir so // findRepoRoot resolves to it. No real git history is needed — the repo // root check is a single filepath.Stat for .git. We also pre-seed a