claudetool/bashkit: add sketch-wip branch protection with process-level tracking
Add a best-effort check to reject git commands that would change the
container-side 'sketch-wip' git branch. The protection prevents:
1. Branch renaming: git branch -m sketch-wip newname
2. Branch switching: git checkout otherbranch, git switch otherbranch
3. Force branch renaming: git branch -M sketch-wip newname
The check allows legitimate operations like:
- File checkout: git checkout -- file.txt
- Path operations: git checkout src/main.go
- Branch creation: git switch -c newbranch
- Standard git operations: git commit, git status, etc.
Key features:
- Process-level tracking: Shows warning only once per process
- Informative error message explaining why it's blocked
- Suggests using 'set-slug' tool for external branch naming
- Tells user they can repeat the command if really needed
Implementation:
- Added process-aware check alongside existing static checks
- Process-level tracking via mutex-protected boolean
- Comprehensive test coverage including edge cases
- Maintains backward compatibility with existing Check() function
This prevents agents from inadvertently breaking the outie's ability
to detect and push changes to GitHub by changing the expected branch name.
Co-Authored-By: sketch <hello@sketch.dev>
Change-ID: s3bb00ecac8a4badek
diff --git a/claudetool/bashkit/bashkit.go b/claudetool/bashkit/bashkit.go
index 23dc1b4..5bdabf5 100644
--- a/claudetool/bashkit/bashkit.go
+++ b/claudetool/bashkit/bashkit.go
@@ -3,6 +3,7 @@
import (
"fmt"
"strings"
+ "sync"
"mvdan.cc/sh/v3/syntax"
)
@@ -12,6 +13,24 @@
noBlindGitAdd,
}
+// Process-level checks that track state across calls
+var processAwareChecks = []func(*syntax.CallExpr) error{
+ noSketchWipBranchChangesOnce,
+}
+
+// Track whether sketch-wip branch warning has been shown in this process
+var (
+ sketchWipWarningMu sync.Mutex
+ sketchWipWarningShown bool
+)
+
+// ResetSketchWipWarning resets the warning state for testing purposes
+func ResetSketchWipWarning() {
+ sketchWipWarningMu.Lock()
+ sketchWipWarningShown = false
+ sketchWipWarningMu.Unlock()
+}
+
// Check inspects bashScript and returns an error if it ought not be executed.
// Check DOES NOT PROVIDE SECURITY against malicious actors.
// It is intended to catch straightforward mistakes in which a model
@@ -36,12 +55,20 @@
if !ok {
return true
}
+ // Run regular checks
for _, check := range checks {
err = check(callExpr)
if err != nil {
return false
}
}
+ // Run process-aware checks
+ for _, check := range processAwareChecks {
+ err = check(callExpr)
+ if err != nil {
+ return false
+ }
+ }
return true
})
@@ -187,3 +214,79 @@
return false
}
+
+// noSketchWipBranchChangesOnce checks for git commands that would change the sketch-wip branch.
+// It rejects commands that would rename the sketch-wip branch or switch away from it.
+// This check only shows the warning once per process.
+func noSketchWipBranchChangesOnce(cmd *syntax.CallExpr) error {
+ if hasSketchWipBranchChanges(cmd) {
+ // Check if we've already warned in this process
+ sketchWipWarningMu.Lock()
+ alreadyWarned := sketchWipWarningShown
+ if !alreadyWarned {
+ sketchWipWarningShown = true
+ }
+ sketchWipWarningMu.Unlock()
+
+ if !alreadyWarned {
+ return fmt.Errorf("permission denied: changing the 'sketch-wip' branch is not allowed. The outie needs this branch name to detect and push your changes to GitHub. If you want to change the external GitHub branch name, use the 'set-slug' tool instead. This warning is shown once per session - you can repeat the command if you really need to do this")
+ }
+ }
+ return nil
+}
+
+// hasSketchWipBranchChanges checks if a git command would change the sketch-wip branch.
+func hasSketchWipBranchChanges(cmd *syntax.CallExpr) bool {
+ if len(cmd.Args) < 2 {
+ return false
+ }
+ if cmd.Args[0].Lit() != "git" {
+ return false
+ }
+
+ // Look for subcommands that could change the sketch-wip branch
+ for i := 1; i < len(cmd.Args); i++ {
+ arg := cmd.Args[i].Lit()
+ switch arg {
+ case "branch":
+ // Check for branch rename: git branch -m sketch-wip newname or git branch -M sketch-wip newname
+ if i+2 < len(cmd.Args) {
+ // Look for -m or -M flag
+ for j := i + 1; j < len(cmd.Args)-1; j++ {
+ flag := cmd.Args[j].Lit()
+ if flag == "-m" || flag == "-M" {
+ // Check if sketch-wip is the source branch
+ if cmd.Args[j+1].Lit() == "sketch-wip" {
+ return true
+ }
+ }
+ }
+ }
+ case "checkout":
+ // Check for branch switching: git checkout otherbranch
+ // But allow git checkout files/paths
+ if i+1 < len(cmd.Args) {
+ nextArg := cmd.Args[i+1].Lit()
+ // Skip if it's a flag
+ if !strings.HasPrefix(nextArg, "-") {
+ // This might be a branch checkout - we'll be conservative and warn
+ // unless it looks like a file path
+ if !strings.Contains(nextArg, "/") && !strings.Contains(nextArg, ".") {
+ return true
+ }
+ }
+ }
+ case "switch":
+ // Check for branch switching: git switch otherbranch
+ if i+1 < len(cmd.Args) {
+ nextArg := cmd.Args[i+1].Lit()
+ // Skip if it's a flag
+ if !strings.HasPrefix(nextArg, "-") {
+ return true
+ }
+ }
+ }
+ }
+
+ return false
+}
diff --git a/claudetool/bashkit/bashkit_test.go b/claudetool/bashkit/bashkit_test.go
index 706790d..ee40582 100644
--- a/claudetool/bashkit/bashkit_test.go
+++ b/claudetool/bashkit/bashkit_test.go
@@ -3,6 +3,8 @@
import (
"strings"
"testing"
+
+ "mvdan.cc/sh/v3/syntax"
)
func TestCheck(t *testing.T) {
@@ -284,3 +286,267 @@
})
}
}
+
+func TestSketchWipBranchProtection(t *testing.T) {
+ tests := []struct {
+ name string
+ script string
+ wantErr bool
+ errMatch string
+ resetBefore bool // if true, reset warning state before test
+ }{
+ {
+ name: "git branch rename sketch-wip",
+ script: "git branch -m sketch-wip new-branch",
+ wantErr: true,
+ errMatch: "changing the 'sketch-wip' branch is not allowed",
+ resetBefore: true,
+ },
+ {
+ name: "git branch force rename sketch-wip",
+ script: "git branch -M sketch-wip new-branch",
+ wantErr: false, // second call should not error (already warned)
+ errMatch: "",
+ resetBefore: false,
+ },
+ {
+ name: "git checkout to other branch",
+ script: "git checkout main",
+ wantErr: false, // third call should not error (already warned)
+ errMatch: "",
+ resetBefore: false,
+ },
+ {
+ name: "git switch to other branch",
+ script: "git switch main",
+ wantErr: false, // fourth call should not error (already warned)
+ errMatch: "",
+ resetBefore: false,
+ },
+ {
+ name: "git checkout file (should be allowed)",
+ script: "git checkout -- file.txt",
+ wantErr: false,
+ errMatch: "",
+ resetBefore: false,
+ },
+ {
+ name: "git checkout path (should be allowed)",
+ script: "git checkout -- src/main.go",
+ wantErr: false,
+ errMatch: "",
+ resetBefore: false,
+ },
+ {
+ name: "git commit (should be allowed)",
+ script: "git commit -m 'test'",
+ wantErr: false,
+ errMatch: "",
+ resetBefore: false,
+ },
+ {
+ name: "git status (should be allowed)",
+ script: "git status",
+ wantErr: false,
+ errMatch: "",
+ resetBefore: false,
+ },
+ {
+ name: "git branch rename other branch (should be allowed)",
+ script: "git branch -m old-branch new-branch",
+ wantErr: false,
+ errMatch: "",
+ resetBefore: false,
+ },
+ }
+
+ for _, tc := range tests {
+ t.Run(tc.name, func(t *testing.T) {
+ if tc.resetBefore {
+ ResetSketchWipWarning()
+ }
+ err := Check(tc.script)
+ if (err != nil) != tc.wantErr {
+ t.Errorf("Check() error = %v, wantErr %v", err, tc.wantErr)
+ return
+ }
+ if tc.wantErr && err != nil && !strings.Contains(err.Error(), tc.errMatch) {
+ t.Errorf("Check() error message = %v, want containing %v", err, tc.errMatch)
+ }
+ })
+ }
+}
+
+func TestHasSketchWipBranchChanges(t *testing.T) {
+ tests := []struct {
+ name string
+ script string
+ wantHas bool
+ }{
+ {
+ name: "git branch rename sketch-wip",
+ script: "git branch -m sketch-wip new-branch",
+ wantHas: true,
+ },
+ {
+ name: "git branch force rename sketch-wip",
+ script: "git branch -M sketch-wip new-branch",
+ wantHas: true,
+ },
+ {
+ name: "git checkout to branch",
+ script: "git checkout main",
+ wantHas: true,
+ },
+ {
+ name: "git switch to branch",
+ script: "git switch main",
+ wantHas: true,
+ },
+ {
+ name: "git checkout file",
+ script: "git checkout -- file.txt",
+ wantHas: false,
+ },
+ {
+ name: "git checkout path",
+ script: "git checkout src/main.go",
+ wantHas: false,
+ },
+ {
+ name: "git checkout with .extension",
+ script: "git checkout file.go",
+ wantHas: false,
+ },
+ {
+ name: "git status",
+ script: "git status",
+ wantHas: false,
+ },
+ {
+ name: "git commit",
+ script: "git commit -m 'test'",
+ wantHas: false,
+ },
+ {
+ name: "git branch rename other",
+ script: "git branch -m old-branch new-branch",
+ wantHas: false,
+ },
+ {
+ name: "git switch with flag",
+ script: "git switch -c new-branch",
+ wantHas: false,
+ },
+ {
+ name: "git checkout with flag",
+ script: "git checkout -b new-branch",
+ wantHas: false,
+ },
+ {
+ name: "not a git command",
+ script: "echo hello",
+ wantHas: false,
+ },
+ {
+ name: "empty command",
+ script: "",
+ wantHas: false,
+ },
+ }
+
+ for _, tc := range tests {
+ t.Run(tc.name, func(t *testing.T) {
+ r := strings.NewReader(tc.script)
+ parser := syntax.NewParser()
+ file, err := parser.Parse(r, "")
+ if err != nil {
+ if tc.wantHas {
+ t.Errorf("Parse error: %v", err)
+ }
+ return
+ }
+
+ found := false
+ syntax.Walk(file, func(node syntax.Node) bool {
+ callExpr, ok := node.(*syntax.CallExpr)
+ if !ok {
+ return true
+ }
+ if hasSketchWipBranchChanges(callExpr) {
+ found = true
+ return false
+ }
+ return true
+ })
+
+ if found != tc.wantHas {
+ t.Errorf("hasSketchWipBranchChanges() = %v, want %v", found, tc.wantHas)
+ }
+ })
+ }
+}
+
+func TestEdgeCases(t *testing.T) {
+ tests := []struct {
+ name string
+ script string
+ wantErr bool
+ resetBefore bool // if true, reset warning state before test
+ }{
+ {
+ name: "git branch -m with current branch to sketch-wip (should be allowed)",
+ script: "git branch -m current-branch sketch-wip",
+ wantErr: false,
+ resetBefore: true,
+ },
+ {
+ name: "git branch -m sketch-wip with no destination (should be blocked)",
+ script: "git branch -m sketch-wip",
+ wantErr: true,
+ resetBefore: true,
+ },
+ {
+ name: "git branch -M with current branch to sketch-wip (should be allowed)",
+ script: "git branch -M current-branch sketch-wip",
+ wantErr: false,
+ resetBefore: true,
+ },
+ {
+ name: "git checkout with -- flags (should be allowed)",
+ script: "git checkout -- --weird-filename",
+ wantErr: false,
+ resetBefore: true,
+ },
+ {
+ name: "git switch with create flag (should be allowed)",
+ script: "git switch --create new-branch",
+ wantErr: false,
+ resetBefore: true,
+ },
+ {
+ name: "complex git command with sketch-wip rename",
+ script: "git add . && git commit -m \"test\" && git branch -m sketch-wip production",
+ wantErr: true,
+ resetBefore: true,
+ },
+ {
+ name: "git switch with -c short form (should be allowed)",
+ script: "git switch -c feature-branch",
+ wantErr: false,
+ resetBefore: true,
+ },
+ }
+
+ for _, tc := range tests {
+ t.Run(tc.name, func(t *testing.T) {
+ if tc.resetBefore {
+ ResetSketchWipWarning()
+ }
+ err := Check(tc.script)
+ if (err != nil) != tc.wantErr {
+ t.Errorf("Check() error = %v, wantErr %v", err, tc.wantErr)
+ }
+ })
+ }
+}