fix(shell): judge only added lines in the changed-script allow list check

cmd/shell-allowlist/changed.go

@@ -8,10 +8,12 @@ import (
 	"fmt"
 	"os"
 	"os/exec"
+	"regexp"
+	"strconv"
 	"strings"
 )
 
-// changedFiles selects whole scripts relative to the merge base, including local edits.
+// changedFiles selects the scripts that differ from the merge base, including local edits.
 func changedFiles(base string) ([]string, error) {
 	mergeBase, err := gitOutput("merge-base", base, "HEAD")
 	if err != nil {
@@ -73,3 +75,65 @@ func gitOutput(args ...string) (string, error) {
 	}
 	return string(out), nil
 }
+
+var hunkHeader = regexp.MustCompile(`^@@ -\S+ \+(\d+)(?:,(\d+))? @@`)
+
+// addedLines maps each tracked path to the line numbers it adds relative to the merge base.
+// An untracked path maps to nil because every line of a new file counts as added.
+func addedLines(base string, paths []string) (map[string]map[uint]bool, error) {
+	mergeBase, err := gitOutput("merge-base", base, "HEAD")
+	if err != nil {
+		return nil, err
+	}
+	untracked, err := gitOutput("ls-files", "--others", "--exclude-standard", "-z")
+	if err != nil {
+		return nil, err
+	}
+	isUntracked := map[string]bool{}
+	for path := range strings.SplitSeq(untracked, "\x00") {
+		isUntracked[path] = true
+	}
+	mergeBase = strings.TrimSpace(mergeBase)
+	diffArgs := []string{"diff", "--unified=0", "--no-color", "--no-ext-diff", "--no-textconv", "--text"}
+	added := map[string]map[uint]bool{}
+	for _, path := range paths {
+		if isUntracked[path] {
+			added[path] = nil
+			continue
+		}
+		var diff string
+		if diff, err = gitOutput(append(diffArgs, mergeBase, "--", path)...); err != nil {
+			return nil, err
+		}
+		if added[path], err = parseAdded(diff); err != nil {
+			return nil, fmt.Errorf("%s: %w", path, err)
+		}
+	}
+	return added, nil
+}
+
+// parseAdded reads the new-side line ranges from the hunk headers of a zero-context diff.
+// A header it can't read fails the check rather than shrinking the judged lines.
+func parseAdded(diff string) (map[uint]bool, error) {
+	lines := map[uint]bool{}
+	for line := range strings.SplitSeq(diff, "\n") {
+		match := hunkHeader.FindStringSubmatch(line)
+		if match == nil {
+			continue
+		}
+		start, err := strconv.ParseUint(match[1], 10, 32)
+		if err != nil {
+			return nil, fmt.Errorf("reading diff hunk %q: %w", line, err)
+		}
+		count := uint64(1)
+		if match[2] != "" {
+			if count, err = strconv.ParseUint(match[2], 10, 32); err != nil {
+				return nil, fmt.Errorf("reading diff hunk %q: %w", line, err)
+			}
+		}
+		for n := range count {
+			lines[uint(start+n)] = true
+		}
+	}
+	return lines, nil
+}

cmd/shell-allowlist/changed_test.go

@@ -75,6 +75,102 @@ func TestChangedScripts(t *testing.T) {
 	assert.Equal(t, ".claude/skills/local/scripts/run.sh:1: ruby: banned\n", out.String())
 }
 
+func TestChangedLinesOnly(t *testing.T) {
+	root, err := filepath.Abs("../..")
+	require.NoError(t, err)
+	dir := t.TempDir()
+	gitTest(t, "clone", "--quiet", "--shared", "--no-checkout", root, dir)
+	t.Chdir(dir)
+	gitTest(t, "read-tree", "HEAD")
+	write := func(path, body string) {
+		t.Helper()
+		require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o700))
+		require.NoError(t, os.WriteFile(path, []byte(body), 0o600))
+	}
+	write(".devtools/shell-allowlist.toml", "commands = ['printf']\n")
+	write("lib.sh", "ruby old\nprintf one\nprintf two\n")
+	write("cont.sh", "print\\\nf one\n")
+	write("raw.sh", "printf ok\n")
+	write(".gitattributes", "raw.sh -diff\n")
+	gitTest(t, "add", "--", ".devtools/shell-allowlist.toml", "lib.sh", "cont.sh", "raw.sh", ".gitattributes")
+	gitTest(t, "-c", "user.name=t", "-c", "user.email=t@example.test", "-c", "commit.gpgsign=false",
+		"commit", "--quiet", "--no-verify", "-m", "base")
+	gitTest(t, "update-ref", "refs/remotes/origin/main", "HEAD")
+	check := func() (int, string) {
+		var out bytes.Buffer
+		status := Run([]string{"--changed", "origin/main"}, &out)
+		return status, out.String()
+	}
+
+	// A finding on a line this change didn't add isn't judged.
+	write("lib.sh", "ruby old\nprintf one\nprintf two\nprintf added\n")
+	status, out := check()
+	assert.Equal(t, 0, status)
+	assert.Empty(t, out)
+
+	// A banned command on an added line fails, and the old finding isn't reported.
+	write("lib.sh", "ruby old\nprintf one\nperl added\nprintf two\n")
+	status, out = check()
+	assert.Equal(t, 1, status)
+	assert.Equal(t, "lib.sh:3: perl: banned\n", out)
+
+	// Editing the line with the old finding makes it an added line.
+	write("lib.sh", "ruby edited\nprintf one\nprintf two\n")
+	status, out = check()
+	assert.Equal(t, 1, status)
+	assert.Equal(t, "lib.sh:1: ruby: banned\n", out)
+
+	// A pure deletion adds nothing to judge.
+	write("lib.sh", "ruby old\nprintf two\n")
+	status, out = check()
+	assert.Equal(t, 0, status)
+	assert.Empty(t, out)
+
+	// Editing only a continuation line changes the command word that starts on a preceding line.
+	write("cont.sh", "print\\\ng one\n")
+	status, out = check()
+	assert.Equal(t, 1, status)
+	assert.Equal(t, "cont.sh:1: print"+"g: not on the allow list\n", out)
+	write("cont.sh", "print\\\nf one\n")
+
+	// A -diff attribute doesn't hide an added line from the judge.
+	write("raw.sh", "printf ok\nruby added\n")
+	status, out = check()
+	assert.Equal(t, 1, status)
+	assert.Equal(t, "raw.sh:2: ruby: banned\n", out)
+	write("raw.sh", "printf ok\n")
+
+	// Text that resembles binary diff markers is an ordinary added or deleted line.
+	write("raw.sh", "printf ok\n# GIT binary patch\nprintf 'Binary files changed'\n")
+	status, out = check()
+	assert.Equal(t, 0, status)
+	assert.Empty(t, out)
+	write("raw.sh", "printf ok\n")
+	status, out = check()
+	assert.Equal(t, 0, status)
+	assert.Empty(t, out)
+
+	// An untracked script counts in full.
+	write("lib.sh", "ruby old\nprintf one\nprintf two\n")
+	write("new.sh", "printf ok\nruby new\n")
+	status, out = check()
+	assert.Equal(t, 1, status)
+	assert.Equal(t, "new.sh:2: ruby: banned\n", out)
+}
+
+func TestParseAdded(t *testing.T) {
+	t.Parallel()
+	diff := "diff --git a/x.sh b/x.sh\n@@ -1,2 +1 @@\n@@ -5 +4,0 @@\n@@ -9,0 +8,3 @@\n@@ -20 +22 @@\n"
+	got, err := parseAdded(diff)
+	require.NoError(t, err)
+	assert.Equal(t, map[uint]bool{1: true, 8: true, 9: true, 10: true, 22: true}, got)
+	got, err = parseAdded("")
+	require.NoError(t, err)
+	assert.Empty(t, got)
+	_, err = parseAdded("@@ -1 +99999999999 @@\n")
+	require.Error(t, err)
+}
+
 func TestShellAllowlistGateWiring(t *testing.T) {
 	t.Chdir("../..")
 	data, err := os.ReadFile("mise.toml")

cmd/shell-allowlist/run.go

@@ -12,17 +12,23 @@ import (
 	"github.com/tbhb-dev/devtools/internal/shell"
 )
 
-// Run checks the given paths, or the scripts changed since a base ref when the arguments start with the changed-files flag.
+// Run checks the given paths, or the lines added to scripts since a base ref when the arguments start with the changed-files flag.
 func Run(args []string, out io.Writer) int {
 	if len(args) == 1 && args[0] == "--summary" {
 		return summary(out)
 	}
+	// A missing entry, or a nil one, means every line of the file counts.
+	var added map[string]map[uint]bool
 	if len(args) == 2 && args[0] == "--changed" {
 		paths, err := changedFiles(args[1])
 		if err != nil {
 			fmt.Fprintln(out, err)
 			return 1
 		}
+		if added, err = addedLines(args[1], paths); err != nil {
+			fmt.Fprintln(out, err)
+			return 1
+		}
 		args = paths
 	}
 	allowed, err := readAllowlist()
@@ -32,19 +38,20 @@ func Run(args []string, out io.Writer) int {
 	}
 	status := 0
 	for _, path := range args {
-		if !checkFile(path, allowed, out) {
+		if !checkFile(path, allowed, added[path], out) {
 			status = 1
 		}
 	}
 	return status
 }
 
-func checkFile(path string, allowed *shell.Allowlist, out io.Writer) bool {
-	violations, failed := scanFile(path, allowed, out)
+func checkFile(path string, allowed *shell.Allowlist, only map[uint]bool, out io.Writer) bool {
+	violations, failed := scanFile(path, allowed, only, out)
 	return violations == 0 && !failed
 }
 
-func scanFile(path string, allowed *shell.Allowlist, out io.Writer) (int, bool) {
+// scanFile reports findings on the lines in only, or on every line when only is nil.
+func scanFile(path string, allowed *shell.Allowlist, only map[uint]bool, out io.Writer) (int, bool) {
 	//nolint:gosec // G703: reading the caller's exact file paths is this command's purpose.
 	data, err := os.ReadFile(path)
 	if err != nil {
@@ -57,8 +64,24 @@ func scanFile(path string, allowed *shell.Allowlist, out io.Writer) (int, bool)
 		fmt.Fprintf(out, "%s: invalid shell syntax\n", path)
 		return 0, true
 	}
+	reported := 0
 	for _, finding := range findings {
+		if only != nil && !touches(only, finding) {
+			continue
+		}
 		fmt.Fprintln(out, finding.String())
+		reported++
+	}
+	return reported, false
+}
+
+// touches reports whether an added line falls anywhere in the finding's command word, so
+// an edit to a continuation line of a backslash-continued word still counts.
+func touches(added map[uint]bool, finding shell.Finding) bool {
+	for line := finding.Line; line <= finding.EndLine; line++ {
+		if added[line] {
+			return true
+		}
 	}
-	return len(findings), false
+	return false
 }

cmd/shell-allowlist/summary.go

@@ -32,7 +32,7 @@ func summary(out io.Writer) int {
 	total, errors := 0, 0
 	escape := strings.NewReplacer("&", "&amp;", "<", "&lt;", ">", "&gt;", "|", "&#124;", "\n", "&#10;", "\r", "&#13;")
 	for _, path := range paths {
-		count, failed := scanFile(path, allowed, io.Discard)
+		count, failed := scanFile(path, allowed, nil, io.Discard)
 		analysisErrors := 0
 		if failed {
 			analysisErrors = 1

docs/shell-allowlist.md

@@ -31,3 +31,7 @@ Unsupported escape forms and expanding words produce findings.
 Name checks can miss tools that a permitted wrapper invokes.
 
 Don't treat this check as an execution sandbox.
+
+With `--changed BASE`, the command judges only the lines a change adds. It reads the added line ranges from a zero-context diff against the merge base and reports a finding only when its line is one of them. A script with no committed history counts in full.
+
+Editing an existing script no longer fails on commands the script already ran before the edit. A changed line that runs a banned command still fails.

internal/shell/shell.go

@@ -56,8 +56,10 @@ func isBanned(name string) bool {
 
 // Finding is one reported command.
 type Finding struct {
-	File    string
-	Line    uint
+	File string
+	Line uint
+	// EndLine is the last line of the command word, which differs from Line when a backslash continues it.
+	EndLine uint
 	Command string
 	Kind    Kind
 }
@@ -210,7 +212,7 @@ func (c *checker) call(call *syntax.CallExpr, defined map[string]bool) {
 		kind = NotAllowed
 	}
 	if kind != "" {
-		c.findings = append(c.findings, Finding{c.file, word.Pos().Line(), name, kind})
+		c.findings = append(c.findings, Finding{c.file, word.Pos().Line(), word.End().Line(), name, kind})
 	}
 	if !literal || defined[name] {
 		clear(defined)

internal/shell/shell_test.go

@@ -94,8 +94,8 @@ func TestNotAllowed(t *testing.T) {
 	t.Parallel()
 	got := check(t, repoList(t), "git status\nbash -c true\nrsync a b\n")
 	assert.Equal(t, []shell.Finding{
-		{File: "script.sh", Line: 2, Command: "bash", Kind: shell.NotAllowed},
-		{File: "script.sh", Line: 3, Command: "rsync", Kind: shell.NotAllowed},
+		{File: "script.sh", Line: 2, EndLine: 2, Command: "bash", Kind: shell.NotAllowed},
+		{File: "script.sh", Line: 3, EndLine: 3, Command: "rsync", Kind: shell.NotAllowed},
 	}, got)
 	assert.Equal(t, "script.sh:2: bash: not on the allow list", got[0].String())
 }
@@ -121,7 +121,7 @@ func TestBannedNames(t *testing.T) {
 		require.Len(t, got, 1, line)
 		assert.Equal(
 			t,
-			shell.Finding{File: "script.sh", Line: 1, Command: name, Kind: shell.Banned},
+			shell.Finding{File: "script.sh", Line: 1, EndLine: 1, Command: name, Kind: shell.Banned},
 			got[0],
 			line,
 		)
@@ -133,7 +133,7 @@ func TestBannedHeredocInput(t *testing.T) {
 	src := "python3 <<EOF\nprint(1)\nEOF\n"
 	got := check(t, repoList(t), src)
 	assert.Equal(t, []shell.Finding{
-		{File: "script.sh", Line: 1, Command: "python3", Kind: shell.Banned},
+		{File: "script.sh", Line: 1, EndLine: 1, Command: "python3", Kind: shell.Banned},
 	}, got)
 }
 
@@ -260,7 +260,7 @@ func TestFunctionBindingsRespectOrderAndScope(t *testing.T) {
 			t.Parallel()
 			got := check(t, repoList(t), src)
 			assert.Equal(t, []shell.Finding{
-				{File: "script.sh", Line: 1, Command: "rsync", Kind: shell.NotAllowed},
+				{File: "script.sh", Line: 1, EndLine: 1, Command: "rsync", Kind: shell.NotAllowed},
 			}, got)
 		})
 	}
@@ -284,7 +284,7 @@ func TestCommandQuoteContexts(t *testing.T) {
 			t.Parallel()
 			got := check(t, repoList(t), tc.src)
 			assert.Equal(t, []shell.Finding{
-				{File: "script.sh", Line: 1, Command: tc.command, Kind: tc.kind},
+				{File: "script.sh", Line: 1, EndLine: 1, Command: tc.command, Kind: tc.kind},
 			}, got)
 		})
 	}
@@ -300,7 +300,7 @@ func TestGlobCommandsAreUnresolvable(t *testing.T) {
 	t.Parallel()
 	for _, word := range []string{"git*", "gi?", "./gi[t]", `./gi[t"t"]`, `"git"*`, "~user/git"} {
 		assert.Equal(t, []shell.Finding{
-			{File: "script.sh", Line: 1, Command: word, Kind: shell.Unresolvable},
+			{File: "script.sh", Line: 1, EndLine: 1, Command: word, Kind: shell.Unresolvable},
 		}, check(t, repoList(t), word+" status"))
 	}
 	for _, word := range []string{`'git*'`, `"git*"`, `git\*`} {
@@ -335,14 +335,14 @@ func TestFunctionBindingsStayUncertainAfterChanges(t *testing.T) {
 		"rsync() { :; }; if true; then unset -f rsync; fi; rsync a b",
 	} {
 		assert.Equal(t, []shell.Finding{
-			{File: "script.sh", Line: 1, Command: "rsync", Kind: shell.NotAllowed},
+			{File: "script.sh", Line: 1, EndLine: 1, Command: "rsync", Kind: shell.NotAllowed},
 		}, check(t, repoList(t), src), src)
 	}
 }
 
 func TestNonCatHeredocSubstitutions(t *testing.T) {
 	t.Parallel()
 	assert.Equal(t, []shell.Finding{
-		{File: "script.sh", Line: 2, Command: "python3", Kind: shell.Banned},
+		{File: "script.sh", Line: 2, EndLine: 2, Command: "python3", Kind: shell.Banned},
 	}, check(t, repoList(t), "printf x <<EOF\n$(python3)\nEOF\n"))
 }