Reject unconsumed CLI arguments instead of silently dropping them

cmd/specsync/argcheck_guard_test.go

@@ -0,0 +1,97 @@
+package main
+
+import (
+	"go/ast"
+	"go/parser"
+	"go/token"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// subcommandsExemptFromArgCheck lists run* functions taking `args []string`
+// that intentionally do not call checkArgs, with the reason it's safe —
+// keep this list short and every entry justified, since it's the one place
+// a future subcommand could quietly opt back out of the check.
+var subcommandsExemptFromArgCheck = map[string]string{
+	"runDoctor":    "reorders flags first (reorderFlagsFirst) and errors loudly on any unrecognized subcommand word — the silent-drop failure mode doesn't apply",
+	"runAgentHelp": "reorders flags first (reorderFlagsFirst) and errors loudly on any unrecognized subcommand word — the silent-drop failure mode doesn't apply",
+	"runRelate":    "dead code: not dispatched from main() or knownSubcommands",
+}
+
+// TestEveryRunFuncChecksArgs guards against the exact way this incident's
+// defect could silently regress: a future subcommand added to cmd/specsync
+// that parses a flag.FlagSet but never calls checkArgs after fs.Parse.
+// Hand-adding checkArgs at each call site (rather than one central dispatch
+// point) means nothing else stops that omission — this test is that stop.
+func TestEveryRunFuncChecksArgs(t *testing.T) {
+	fset := token.NewFileSet()
+	paths, err := filepath.Glob("*.go")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	checked := 0
+	for _, path := range paths {
+		if strings.HasSuffix(path, "_test.go") {
+			continue
+		}
+		src, err := os.ReadFile(path)
+		if err != nil {
+			t.Fatal(err)
+		}
+		f, err := parser.ParseFile(fset, path, src, 0)
+		if err != nil {
+			t.Fatalf("parse %s: %v", path, err)
+		}
+		for _, decl := range f.Decls {
+			fn, ok := decl.(*ast.FuncDecl)
+			if !ok || fn.Body == nil || fn.Recv != nil {
+				continue
+			}
+			name := fn.Name.Name
+			if !strings.HasPrefix(name, "run") || !takesArgsSlice(fn) {
+				continue
+			}
+			if _, exempt := subcommandsExemptFromArgCheck[name]; exempt {
+				continue
+			}
+			checked++
+			if !callsCheckArgs(fn.Body) {
+				t.Errorf("%s (%s) parses a flag.FlagSet from an `args []string` parameter but never calls checkArgs after fs.Parse — call it, or add %q to subcommandsExemptFromArgCheck with a reason", name, path, name)
+			}
+		}
+	}
+	if checked == 0 {
+		t.Fatal("scan found no run*(args []string) functions — the scan itself is broken")
+	}
+}
+
+// takesArgsSlice reports whether fn has exactly one parameter of type
+// []string — the signature every dispatched subcommand handler uses.
+func takesArgsSlice(fn *ast.FuncDecl) bool {
+	if fn.Type.Params == nil || len(fn.Type.Params.List) != 1 {
+		return false
+	}
+	arr, ok := fn.Type.Params.List[0].Type.(*ast.ArrayType)
+	if !ok || arr.Len != nil {
+		return false
+	}
+	ident, ok := arr.Elt.(*ast.Ident)
+	return ok && ident.Name == "string"
+}
+
+// callsCheckArgs reports whether body contains a call to checkArgs anywhere.
+func callsCheckArgs(body ast.Node) bool {
+	found := false
+	ast.Inspect(body, func(n ast.Node) bool {
+		if call, ok := n.(*ast.CallExpr); ok {
+			if ident, ok := call.Fun.(*ast.Ident); ok && ident.Name == "checkArgs" {
+				found = true
+			}
+		}
+		return true
+	})
+	return found
+}

cmd/specsync/argcheck_test.go

@@ -0,0 +1,178 @@
+package main
+
+import (
+	"flag"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"sync"
+	"testing"
+)
+
+// TestShadowedFlags pins the exact 2026-09-24 incident shape: a flag placed
+// after a positional-looking token is never seen by fs.Parse (Go's flag
+// package stops parsing at the first non-flag argument) and must be reported
+// as shadowed instead of silently keeping its zero value.
+func TestShadowedFlags(t *testing.T) {
+	newFS := func() *flag.FlagSet {
+		fs := flag.NewFlagSet("sync", flag.ContinueOnError)
+		fs.String("repo", "", "")
+		fs.String("change", "", "")
+		fs.Bool("dry-run", false, "")
+		return fs
+	}
+
+	tests := []struct {
+		name string
+		args []string
+		want []string
+	}{
+		{
+			name: "incident shape: url before -dry-run",
+			args: []string{"-repo", "ExopenGitHub/portal", "https://github.com/ExopenGitHub/portal/issues/4304", "-dry-run"},
+			want: []string{"-dry-run"},
+		},
+		{
+			name: "no positional: nothing shadowed",
+			args: []string{"-repo", "owner/repo", "-dry-run"},
+			want: nil,
+		},
+		{
+			name: "flags before positional: nothing shadowed",
+			args: []string{"-dry-run", "-repo", "owner/repo", "some-positional"},
+			want: nil,
+		},
+		{
+			name: "value flag after positional is also shadowed",
+			args: []string{"some-positional", "-repo", "owner/repo"},
+			want: []string{"-repo"},
+		},
+		{
+			name: "inline = value flag after positional",
+			args: []string{"some-positional", "-repo=owner/repo"},
+			want: []string{"-repo"},
+		},
+		{
+			name: "unregistered flag after positional is not reported",
+			args: []string{"some-positional", "-not-a-real-flag"},
+			want: nil,
+		},
+		{
+			name: "value flag immediately followed by another shadowed flag: both reported",
+			args: []string{"some-positional", "-repo", "-dry-run"},
+			want: []string{"-repo", "-dry-run"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			fs := newFS()
+			got := shadowedFlags(fs, tt.args)
+			if strings.Join(got, ",") != strings.Join(tt.want, ",") {
+				t.Fatalf("shadowedFlags(%v) = %v, want %v", tt.args, got, tt.want)
+			}
+		})
+	}
+}
+
+// TestCheckArgs verifies checkArgs reports both an unconsumed-positional
+// problem and a shadowed-flag problem in the same call when both are
+// present — the exact combination that turned the 2026-09-24 incident's
+// mistyped invocation into a full live sync with zero feedback.
+func TestCheckArgs(t *testing.T) {
+	fs := flag.NewFlagSet("sync", flag.ContinueOnError)
+	fs.String("repo", "", "")
+	fs.Bool("dry-run", false, "")
+
+	rawArgs := []string{"-repo", "ExopenGitHub/portal", "https://github.com/ExopenGitHub/portal/issues/4304", "-dry-run"}
+	_ = fs.Parse(rawArgs)
+	if fs.NArg() != 2 {
+		t.Fatalf("expected 2 leftover args, got %d: %v", fs.NArg(), fs.Args())
+	}
+
+	err := checkArgsErr(fs, rawArgs, 0)
+	if err == nil {
+		t.Fatal("expected checkArgs to report a problem, got nil")
+	}
+	if !strings.Contains(err.Error(), "-dry-run") {
+		t.Errorf("error %q does not name the shadowed flag -dry-run", err.Error())
+	}
+	if !strings.Contains(err.Error(), "https://github.com/ExopenGitHub/portal/issues/4304") {
+		t.Errorf("error %q does not name the unconsumed URL argument", err.Error())
+	}
+	if strings.Count(err.Error(), "-dry-run") != 1 {
+		t.Errorf("error %q names -dry-run more than once — it should be reported only as shadowed, not also as a separate unexpected argument", err.Error())
+	}
+}
+
+var buildOnce sync.Once
+var builtBinary string
+var buildErr error
+
+// buildSpecsyncBinary compiles the specsync CLI once per test run and
+// returns the path to the resulting binary.
+func buildSpecsyncBinary(t *testing.T) string {
+	t.Helper()
+	buildOnce.Do(func() {
+		dir := t.TempDir()
+		builtBinary = filepath.Join(dir, "specsync-test-bin")
+		cmd := exec.Command("go", "build", "-o", builtBinary, ".")
+		cmd.Dir = "."
+		out, err := cmd.CombinedOutput()
+		if err != nil {
+			buildErr = err
+			t.Logf("go build output: %s", out)
+		}
+	})
+	if buildErr != nil {
+		t.Fatalf("build specsync binary: %v", buildErr)
+	}
+	return builtBinary
+}
+
+// TestIncidentRegression reproduces the exact 2026-09-24 incident's argument
+// shape end to end against the compiled binary: `specsync -repo <repo> <url>
+// -dry-run` run against a directory whose openspec/changes/ holds changes
+// unrelated to <repo> must fail with a clear error, make zero provider
+// calls, and exit non-zero. A stub `gh` on PATH records every invocation so
+// a live sync (the actual incident) would be unmistakable.
+func TestIncidentRegression(t *testing.T) {
+	bin := buildSpecsyncBinary(t)
+
+	fixture := t.TempDir()
+	changeDir := filepath.Join(fixture, "openspec", "changes", "unrelated-change")
+	if err := os.MkdirAll(changeDir, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(changeDir, "proposal.md"), []byte("# Unrelated change\n\n## Why\n\nUnrelated.\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	// Stub `gh` that records every call it receives; the fix must produce
+	// zero calls to it.
+	binDir := t.TempDir()
+	callLog := filepath.Join(binDir, "gh-calls.log")
+	ghStub := "#!/bin/sh\necho \"$@\" >> " + callLog + "\necho '[]'\n"
+	ghPath := filepath.Join(binDir, "gh")
+	if err := os.WriteFile(ghPath, []byte(ghStub), 0o755); err != nil {
+		t.Fatal(err)
+	}
+
+	cmd := exec.Command(bin, "-repo", "ExopenGitHub/portal",
+		"https://github.com/ExopenGitHub/portal/issues/4304", "-dry-run")
+	cmd.Dir = fixture
+	cmd.Env = append(os.Environ(), "PATH="+binDir+":"+os.Getenv("PATH"))
+	out, err := cmd.CombinedOutput()
+
+	if err == nil {
+		t.Fatalf("expected non-zero exit, got success; output:\n%s", out)
+	}
+	if !strings.Contains(string(out), "unexpected argument") && !strings.Contains(string(out), "not applied") {
+		t.Errorf("expected a clear unexpected-argument/shadowed-flag error, got:\n%s", out)
+	}
+	if _, statErr := os.Stat(callLog); statErr == nil {
+		logged, _ := os.ReadFile(callLog)
+		t.Fatalf("expected zero gh calls, got:\n%s", logged)
+	}
+}

cmd/specsync/changelog.go

@@ -33,6 +33,7 @@ func runChangelog(args []string) {
 	providerName := fs.String("provider", "github", "work provider for -resolve-refs: github (default) or beads")
 	failOnUnlinkedCommits := fs.Bool("fail-on-unlinked-commits", false, "exit non-zero when a conventional commit in range isn't linked to its change's issue (would otherwise render as a raw title+hash fallback)")
 	_ = fs.Parse(args)
+	checkArgs(fs, args, 0)
 
 	abs, err := filepath.Abs(*openspec)
 	if err != nil {

cmd/specsync/epic.go

@@ -24,6 +24,7 @@ func runEpic(args []string) {
 	fs.Var(&children, "child", "a child to attach: local change slug, owner/repo#N, bare #N (resolved against --repo), or issue URL (repeatable)")
 	dryRun := fs.Bool("dry-run", false, "print what would happen without creating or editing any issue")
 	_ = fs.Parse(args)
+	checkArgs(fs, args, -1) // the title is a free-form positional
 
 	title := strings.TrimSpace(strings.Join(fs.Args(), " "))
 	if title == "" {

cmd/specsync/installskill.go

@@ -35,6 +35,7 @@ func runInstallSkill(args []string) {
 		flags[i] = fs.Bool(t.flag, false, fmt.Sprintf("install to %s (~/%s)", t.label, filepath.Join(t.relPath...)))
 	}
 	_ = fs.Parse(args)
+	checkArgs(fs, args, 0)
 
 	home, err := os.UserHomeDir()
 	if err != nil {