diff --git a/docs/plans/2026-02-22-tool-command-path-safety.md b/docs/plans/2026-02-22-tool-command-path-safety.md new file mode 100644 index 00000000..b52a3674 --- /dev/null +++ b/docs/plans/2026-02-22-tool-command-path-safety.md @@ -0,0 +1,105 @@ +# Tool Command Hardcoded Path Detection + +**Date:** 2026-02-22 +**Status:** Proposed +**Problem:** A `tool_command` node with a hardcoded `cd /absolute/path &&` prefix silently +overrides the engine's worktree CWD, causing the command to run against the wrong checkout. +This caused `verify_clippy` to fail on a lab-bench run: the engine correctly set +`cmd.Dir = execCtx.WorktreeDir`, but the shell command itself started with +`cd /Users/.../lab-bench-poc &&`, which jumped to the main branch checkout where the new +crate didn't exist. Meanwhile `verify_build` and `verify_tests` (without `cd` prefixes) +ran correctly in the worktree. + +The root cause was a DOT file generation bug (fixed in orchestration commit `6633e67`), +but the engine has no guard against this class of error. + +## Goal + +Detect `tool_command` strings that contain absolute paths conflicting with the worktree +directory and either warn or fail at preflight/validation time, before the run starts. + +## Design + +### Level of effort: Small + +Two complementary checks: + +1. **Preflight warning** — during `kilroy attractor run` preflight, scan all + `tool_command` attributes for `cd /` patterns. If found, warn the operator. +2. **Validate subcommand** — `kilroy attractor validate --graph ` should + flag `tool_command` nodes containing hardcoded absolute `cd` paths as a lint warning. + +Neither check should be a hard error — there may be legitimate uses of `cd` in tool +commands — but it should surface a visible warning. + +## Data + +The engine already parses tool_command in `ToolHandler.Execute()`: + +``` +File: internal/attractor/engine/handlers.go line 534 +cmdStr := node.Attr("tool_command", "") +``` + +And sets the correct working directory at line 561: + +```go +cmd.Dir = execCtx.WorktreeDir +``` + +The problem is that `bash -c "cd /other/path && cargo clippy"` ignores `cmd.Dir` +because the shell's `cd` takes precedence. + +## Changes + +### 1. Add path lint to graph validation + +**File:** `internal/attractor/engine/validate.go` (or wherever `validate --graph` runs) + +For each node with `shape=parallelogram` (tool handler), check if `tool_command` +matches the pattern `cd\s+/` (absolute cd). If so, emit a warning: + +``` +WARN: node "verify_clippy" tool_command contains "cd /..." which overrides + the worktree working directory. Remove the cd prefix — the engine + sets CWD to the worktree automatically. +``` + +### 2. Add runtime warning in ToolHandler.Execute + +**File:** `internal/attractor/engine/handlers.go` ~line 534 + +After reading `cmdStr`, check for the pattern and log a warning before execution: + +```go +cmdStr := node.Attr("tool_command", "") +if hasCDAbsPath(cmdStr) { + log.Warnf("node %q tool_command contains 'cd /' which overrides "+ + "worktree CWD (%s) — this is usually a DOT file bug", node.ID, execCtx.WorktreeDir) +} +``` + +The helper `hasCDAbsPath` can use a simple regex: `cd\s+/[^\s]`. + +### 3. Add the warning to preflight report + +**File:** wherever preflight validation runs (likely `internal/attractor/engine/preflight.go` +or similar) + +During the preflight check that already validates the graph, add the same lint. Include +it in `preflight_report.json` as a warning (not a blocker). + +## Files to modify + +| File | Change | +|------|--------| +| `internal/attractor/engine/handlers.go` | Runtime warning when tool_command contains `cd /` | +| `internal/attractor/engine/validate.go` | Lint warning in `validate --graph` | +| Preflight validation file | Warning in preflight report | + +## Testing + +1. Create a test DOT file with `tool_command="cd /tmp && echo hello"` — verify warning +2. Create a test DOT file with `tool_command="cargo test"` — verify no warning +3. Verify `kilroy attractor validate --graph` surfaces the warning +4. `go test ./internal/attractor/engine/...` diff --git a/internal/attractor/engine/handlers.go b/internal/attractor/engine/handlers.go index 557c0a2a..4179ee64 100644 --- a/internal/attractor/engine/handlers.go +++ b/internal/attractor/engine/handlers.go @@ -8,6 +8,7 @@ import ( "os" "os/exec" "path/filepath" + "regexp" "strings" "time" @@ -513,6 +514,8 @@ func (h *WaitHumanHandler) Execute(ctx context.Context, exec *Execution, node *m }, nil } +var toolCommandAbsPathRE = regexp.MustCompile(`cd\s+/`) + type ToolHandler struct{} func (h *ToolHandler) Execute(ctx context.Context, execCtx *Execution, node *model.Node) (runtime.Outcome, error) { @@ -521,6 +524,9 @@ func (h *ToolHandler) Execute(ctx context.Context, execCtx *Execution, node *mod if cmdStr == "" { return runtime.Outcome{Status: runtime.StatusFail, FailureReason: "no tool_command specified"}, nil } + if toolCommandAbsPathRE.MatchString(cmdStr) { + warnEngine(execCtx, fmt.Sprintf("tool_command for node %q contains 'cd /…' which overrides worktree CWD %q", node.ID, execCtx.WorktreeDir)) + } timeout := parseDuration(node.Attr("timeout", ""), 0) if timeout <= 0 { timeout = 10 * time.Second diff --git a/internal/attractor/validate/validate.go b/internal/attractor/validate/validate.go index e9d4939f..4a11a9f4 100644 --- a/internal/attractor/validate/validate.go +++ b/internal/attractor/validate/validate.go @@ -66,6 +66,7 @@ func Validate(g *model.Graph, extraRules ...LintRule) []Diagnostic { diags = append(diags, lintFailLoopFailureClassGuard(g)...) diags = append(diags, lintEscalationModelsSyntax(g)...) diags = append(diags, lintAllConditionalEdges(g)...) + diags = append(diags, lintToolCommandAbsPath(g)...) // Run custom lint rules (spec §7.3: extra_rules appended after built-in rules). for _, rule := range extraRules { @@ -1100,6 +1101,34 @@ func (r *TypeKnownRule) Apply(g *model.Graph) []Diagnostic { return diags } +var toolCommandAbsPathPattern = regexp.MustCompile(`cd\s+/`) + +func lintToolCommandAbsPath(g *model.Graph) []Diagnostic { + var diags []Diagnostic + for id, n := range g.Nodes { + if n == nil { + continue + } + if !nodeResolvesToTool(n) { + continue + } + cmd := strings.TrimSpace(n.Attr("tool_command", "")) + if cmd == "" { + continue + } + if toolCommandAbsPathPattern.MatchString(cmd) { + diags = append(diags, Diagnostic{ + Rule: "tool_command_abs_path", + Severity: SeverityWarning, + Message: fmt.Sprintf("tool_command contains 'cd /…' which overrides the engine worktree CWD"), + NodeID: id, + Fix: "remove the 'cd /…' prefix; the engine sets the working directory to the worktree automatically", + }) + } + } + return diags +} + // lintAllConditionalEdges warns when a non-terminal node has outgoing edges but // all are conditional (no unconditional fallback). This creates a routing gap: // if no condition matches at runtime, the engine has no edge to follow. diff --git a/internal/attractor/validate/validate_test.go b/internal/attractor/validate/validate_test.go index 56884785..0d5af6fc 100644 --- a/internal/attractor/validate/validate_test.go +++ b/internal/attractor/validate/validate_test.go @@ -572,6 +572,40 @@ func assertNoRule(t *testing.T, diags []Diagnostic, rule string) { } } +// --- Tests for tool_command_abs_path lint rule --- + +func TestValidate_ToolCommandAbsPath_WarnsOnCdAbsolutePath(t *testing.T) { + g, err := dot.Parse([]byte(` +digraph G { + start [shape=Mdiamond] + exit [shape=Msquare] + t [shape=parallelogram, tool_command="cd /tmp && echo hello"] + start -> t -> exit +} +`)) + if err != nil { + t.Fatalf("parse: %v", err) + } + diags := Validate(g) + assertHasRule(t, diags, "tool_command_abs_path", SeverityWarning) +} + +func TestValidate_ToolCommandAbsPath_NoWarningOnRelativeCommand(t *testing.T) { + g, err := dot.Parse([]byte(` +digraph G { + start [shape=Mdiamond] + exit [shape=Msquare] + t [shape=parallelogram, tool_command="cargo test"] + start -> t -> exit +} +`)) + if err != nil { + t.Fatalf("parse: %v", err) + } + diags := Validate(g) + assertNoRule(t, diags, "tool_command_abs_path") +} + // --- Tests for V7.2: type_known lint rule --- func TestValidate_TypeKnownRule_RecognizedType_NoWarning(t *testing.T) {