diff --git a/src/cmd/guard.go b/src/cmd/guard.go index 6950b53..73a57ac 100644 --- a/src/cmd/guard.go +++ b/src/cmd/guard.go @@ -155,6 +155,53 @@ func staleTTL() time.Duration { return time.Duration(n) * time.Second } +// currentRepoRoot resolves the absolute repo root the current Claude +// Code session is operating in, for the stop-guard scope check (issue +// #91). Prefer CLAUDE_PROJECT_DIR because Claude Code sets it per +// session for every hook invocation and it is not spoofable from +// within a prompt — Claude cannot edit its own session env. Fall back +// to walking up from pwd so a non-Claude-Code caller (direct stdio +// test) still resolves something sensible. Returns "" if no repo root +// can be determined; callers fall through to the legacy block in that +// case so a missing signal never silently approves. +func currentRepoRoot() string { + if dir := strings.TrimSpace(os.Getenv("CLAUDE_PROJECT_DIR")); dir != "" { + return dir + } + if dir, err := os.Getwd(); err == nil { + for { + if _, err := os.Stat(filepath.Join(dir, ".git")); err == nil { + return dir + } + parent := filepath.Dir(dir) + if parent == dir { + return "" + } + dir = parent + } + } + return "" +} + +// samePath compares two filesystem paths by resolving symlinks and +// cleaning separators. Used by the stop-guard repo-match check so +// symlinked checkouts ("/Users/me/repos/x" via "/tmp/x") resolve +// equal. Falls back to cleaned-path comparison if EvalSymlinks fails +// (path does not exist yet, permission denied) so the check never +// panics on transient filesystem state. +func samePath(a, b string) bool { + norm := func(p string) string { + if abs, err := filepath.Abs(p); err == nil { + p = abs + } + if resolved, err := filepath.EvalSymlinks(p); err == nil { + return filepath.Clean(resolved) + } + return filepath.Clean(p) + } + return norm(a) == norm(b) +} + // sessionIsStale mirrors the lib/read-session.sh TTL rule: prefer // UpdatedAt, fall back to StartedAt for pre-UpdatedAt state files // written by older engine binaries. An unparseable (zero) timestamp is @@ -451,6 +498,27 @@ func runStopGuard() { return } + // Scope restriction (issue #91): if the workflow was started in a + // different repo than the one the current Claude Code session is + // operating in, approve silently. The block is not a bypass — it + // remains active when the user returns to the originating repo; + // this only prevents a stuck workflow in repo A from nagging every + // turn of unrelated work in repo B. No TTL escape, no env override, + // no branch carve-out: if the session started in repo A, only repo A + // sees the block. Empty state.RepoRoot (pre-#91 sessions) or empty + // current repo root (hook env missing) falls through to the legacy + // block behavior — we do not silently approve on missing signals. + if state.RepoRoot != "" { + if cur := currentRepoRoot(); cur != "" && !samePath(state.RepoRoot, cur) { + fmt.Fprintf(guardStderr, + "devkit-stop-guard: session %s belongs to %s, current repo is %s — approving Stop (block remains active in originating repo)\n", + state.Workflow, state.RepoRoot, cur) + writeStopVerdict(stopVerdict{Decision: "approve"}) + guardExit(0) + return + } + } + remaining := 0 if state.TotalSteps > 0 { remaining = state.TotalSteps - state.CurrentIndex diff --git a/src/cmd/guard_test.go b/src/cmd/guard_test.go index d6677e1..b691a8f 100644 --- a/src/cmd/guard_test.go +++ b/src/cmd/guard_test.go @@ -934,6 +934,235 @@ func TestGuardStopHook(t *testing.T) { } } +// TestGuardStopHookRepoScope pins the issue-#91 scope restriction: +// the stop-guard nag fires only when the current Claude Code session's +// repo matches the repo that started the workflow. Cross-repo sessions +// approve silently; the block remains active when the user returns to +// the originating repo. The legacy empty-RepoRoot case (pre-#91 +// sessions) falls through to the historical block behavior — no silent +// bypass on missing scope signal. +func TestGuardStopHookRepoScope(t *testing.T) { + tests := []struct { + name string + stateRepoRoot string + claudeProjectDir string + wantDecision string + }{ + { + name: "matching repo → block", + stateRepoRoot: "/tmp/repo-a", + claudeProjectDir: "/tmp/repo-a", + wantDecision: "block", + }, + { + name: "matching repo with trailing slash → block", + stateRepoRoot: "/tmp/repo-a", + claudeProjectDir: "/tmp/repo-a/", + wantDecision: "block", + }, + { + name: "different repo → approve", + stateRepoRoot: "/tmp/repo-a", + claudeProjectDir: "/tmp/repo-b", + wantDecision: "approve", + }, + { + name: "empty state.RepoRoot (pre-#91) → block", + stateRepoRoot: "", + claudeProjectDir: "/tmp/repo-b", + wantDecision: "block", + }, + { + name: "empty CLAUDE_PROJECT_DIR (cannot resolve current) → block", + stateRepoRoot: "/tmp/repo-a", + claudeProjectDir: "", + wantDecision: "block", + }, + } + + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + writeSession(t, dir, lib.SessionState{ + Status: "running", + Workflow: "pr-ready", + StepEnforce: lib.EnforceHard, + TotalSteps: 5, + RepoRoot: tc.stateRepoRoot, + }) + env := newGuardTestEnv(t, "", "", true, dir) + // CLAUDE_PROJECT_DIR drives currentRepoRoot() for the + // stop-guard's repo-match check. Unset forces the + // pwd-walk fallback, which tempdirs won't satisfy → "". + if tc.claudeProjectDir != "" { + t.Setenv("CLAUDE_PROJECT_DIR", tc.claudeProjectDir) + } else { + t.Setenv("CLAUDE_PROJECT_DIR", "") + // Also move pwd into a non-git tempdir so the + // fallback walk returns "" deterministically. + prev, _ := os.Getwd() + nowhere := t.TempDir() + if err := os.Chdir(nowhere); err != nil { + t.Fatalf("chdir: %v", err) + } + t.Cleanup(func() { os.Chdir(prev) }) + } + runGuard(guardCmd, nil) + var v stopVerdict + if err := json.Unmarshal(env.stdout.Bytes(), &v); err != nil { + t.Fatalf("invalid JSON: %v", err) + } + if v.Decision != tc.wantDecision { + t.Fatalf("decision=%q want=%q (stderr=%q)", v.Decision, tc.wantDecision, env.stderr.String()) + } + }) + } +} + +// TestGuardStopHookRepoScopeWalkUp covers the pwd-walk fallback path in +// currentRepoRoot() — all cases in TestGuardStopHookRepoScope set +// CLAUDE_PROJECT_DIR, so the `.git`-finding loop never executes there. +// Also covers the subdirectory case (session cwd several levels below +// the repo root). +func TestGuardStopHookRepoScopeWalkUp(t *testing.T) { + // Build a fake repo with a nested subdir: repo/a/b. + repo := t.TempDir() + if err := os.Mkdir(filepath.Join(repo, ".git"), 0o755); err != nil { + t.Fatalf("mkdir .git: %v", err) + } + sub := filepath.Join(repo, "a", "b") + if err := os.MkdirAll(sub, 0o755); err != nil { + t.Fatalf("mkdir sub: %v", err) + } + + tests := []struct { + name string + stateRepoRoot string + cwd string + wantDecision string + }{ + { + name: "walk-up from repo root resolves match → block", + stateRepoRoot: repo, + cwd: repo, + wantDecision: "block", + }, + { + name: "walk-up from nested subdir resolves match → block", + stateRepoRoot: repo, + cwd: sub, + wantDecision: "block", + }, + } + + for _, tc := range tests { + tc := tc + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + writeSession(t, dir, lib.SessionState{ + Status: "running", + Workflow: "pr-ready", + StepEnforce: lib.EnforceHard, + TotalSteps: 5, + RepoRoot: tc.stateRepoRoot, + }) + env := newGuardTestEnv(t, "", "", true, dir) + // Unset CLAUDE_PROJECT_DIR to force the pwd-walk + // branch of currentRepoRoot(). + t.Setenv("CLAUDE_PROJECT_DIR", "") + prev, _ := os.Getwd() + if err := os.Chdir(tc.cwd); err != nil { + t.Fatalf("chdir: %v", err) + } + t.Cleanup(func() { os.Chdir(prev) }) + runGuard(guardCmd, nil) + var v stopVerdict + if err := json.Unmarshal(env.stdout.Bytes(), &v); err != nil { + t.Fatalf("invalid JSON: %v", err) + } + if v.Decision != tc.wantDecision { + t.Fatalf("decision=%q want=%q (stderr=%q)", v.Decision, tc.wantDecision, env.stderr.String()) + } + }) + } +} + +// TestGuardStopHookRepoScopeSymlink proves samePath() actually uses +// EvalSymlinks rather than bare Clean(): a symlinked path pointing at +// the originating repo must compare equal and keep the block active. +// Regression guard for macOS /var → /private/var and user-created +// symlinks to worktrees. +func TestGuardStopHookRepoScopeSymlink(t *testing.T) { + real := t.TempDir() + if err := os.Mkdir(filepath.Join(real, ".git"), 0o755); err != nil { + t.Fatalf("mkdir .git: %v", err) + } + linkParent := t.TempDir() + link := filepath.Join(linkParent, "linked-repo") + if err := os.Symlink(real, link); err != nil { + t.Skipf("symlink not supported on this platform: %v", err) + } + + dir := t.TempDir() + writeSession(t, dir, lib.SessionState{ + Status: "running", + Workflow: "pr-ready", + StepEnforce: lib.EnforceHard, + TotalSteps: 5, + RepoRoot: real, + }) + env := newGuardTestEnv(t, "", "", true, dir) + t.Setenv("CLAUDE_PROJECT_DIR", link) + + runGuard(guardCmd, nil) + var v stopVerdict + if err := json.Unmarshal(env.stdout.Bytes(), &v); err != nil { + t.Fatalf("invalid JSON: %v", err) + } + if v.Decision != "block" { + t.Fatalf("decision=%q want=%q — samePath should resolve the symlink (stderr=%q)", v.Decision, "block", env.stderr.String()) + } +} + +// TestGuardStopHookRepoScopeWorktree pins currentRepoRoot()'s support +// for git worktrees, where `.git` is a FILE (gitlink) instead of a +// directory. devkit itself runs from worktrees; a future refactor to +// IsDir() on the walk-up check would silently bypass the block for +// every worktree user without this test. +func TestGuardStopHookRepoScopeWorktree(t *testing.T) { + worktree := t.TempDir() + // Worktrees have .git as a regular file containing `gitdir: ...`. + if err := os.WriteFile(filepath.Join(worktree, ".git"), []byte("gitdir: /tmp/fake-main/.git/worktrees/wt\n"), 0o644); err != nil { + t.Fatalf("write .git file: %v", err) + } + + dir := t.TempDir() + writeSession(t, dir, lib.SessionState{ + Status: "running", + Workflow: "pr-ready", + StepEnforce: lib.EnforceHard, + TotalSteps: 5, + RepoRoot: worktree, + }) + env := newGuardTestEnv(t, "", "", true, dir) + t.Setenv("CLAUDE_PROJECT_DIR", "") + prev, _ := os.Getwd() + if err := os.Chdir(worktree); err != nil { + t.Fatalf("chdir: %v", err) + } + t.Cleanup(func() { os.Chdir(prev) }) + + runGuard(guardCmd, nil) + var v stopVerdict + if err := json.Unmarshal(env.stdout.Bytes(), &v); err != nil { + t.Fatalf("invalid JSON: %v", err) + } + if v.Decision != "block" { + t.Fatalf("decision=%q want=%q — worktree .git as file must still resolve as repo root (stderr=%q)", v.Decision, "block", env.stderr.String()) + } +} + func TestGuardStopHookStaleSession(t *testing.T) { // Stale-during-Stop → approve so a crashed engine doesn't trap the // user in an un-stoppable session. diff --git a/src/lib/state_json.go b/src/lib/state_json.go index 2cd4b3f..f5ca538 100644 --- a/src/lib/state_json.go +++ b/src/lib/state_json.go @@ -75,6 +75,16 @@ type SessionState struct { Busy bool `json:"busy,omitempty"` LoopIteration int `json:"loop_iteration,omitempty"` // current loop count for loop steps LoopMax int `json:"loop_max,omitempty"` // max iterations for current loop + // RepoRoot is the absolute path of the repository whose MCP server + // started this workflow. The Stop hook uses it to scope the + // incomplete-workflow nag to the originating repo: opening a Claude + // Code session in a different repo sees an approve verdict instead + // of blocking on a workflow that does not belong to that project. + // This is scope restriction, not a bypass — returning to the + // originating repo resumes the block. Empty on sessions written by + // pre-#91 engines; the guard treats empty as "no scoping, preserve + // legacy block behavior." + RepoRoot string `json:"repo_root,omitempty"` } // UnmarshalJSON validates StepEnforce at read time so a stale or diff --git a/src/mcp/tools.go b/src/mcp/tools.go index a5a5f95..633ed09 100644 --- a/src/mcp/tools.go +++ b/src/mcp/tools.go @@ -168,6 +168,7 @@ func (s *Server) startTool() (mcpmcp.Tool, mcpgo.ToolHandlerFunc) { Status: "starting", StartedAt: time.Now(), Outputs: map[string]string{}, + RepoRoot: s.repoRoot, }, nil }) if err != nil { diff --git a/workflows/tri-debug.yml b/workflows/tri-debug.yml index ce94545..b3c3b7a 100644 --- a/workflows/tri-debug.yml +++ b/workflows/tri-debug.yml @@ -5,24 +5,32 @@ steps: - id: smart-diagnosis model: smart prompt: | - Debug this issue: {{input}} - - Trace the root cause. Read the relevant code, check assumptions, - and propose a specific fix with reasoning. + DISPATCH, DO NOT ANSWER. Tri-debug requires three DIFFERENT models + diagnose independently. Invoke the Agent tool with a non-Claude + smart-tier subagent (`codex:codex-rescue` is the standard external + diagnostician). Pass the issue and report verbatim. If no external + subagent is available, stop and tell the user — do NOT answer as + yourself. Subagent prompt: "Debug this issue: {{input}}. Trace the + root cause. Read the relevant code, check assumptions, and propose + a specific fix with reasoning." - id: general-diagnosis model: general prompt: | - Debug this issue: {{input}} - - What's the most likely cause? Suggest a fix. + DISPATCH, DO NOT ANSWER. Invoke the Agent tool with a non-Claude + general-tier subagent (prefer a Gemini-backed agent, or a different + external than smart-diagnosis used). Report verbatim. Stop if none + available. Subagent prompt: "Debug this issue: {{input}}. What's + the most likely cause? Suggest a fix." - id: fast-diagnosis model: fast prompt: | - Debug this issue: {{input}} - - Quick diagnosis — what's probably wrong and how to fix it. + DISPATCH, DO NOT ANSWER. Invoke the Agent tool with a fast-tier + external subagent (distinct from the other two tiers). Report + verbatim. Stop if none available. Subagent prompt: "Debug this + issue: {{input}}. Quick diagnosis — what's probably wrong and how + to fix it." - id: dispatch-debug parallel: [smart-diagnosis, general-diagnosis, fast-diagnosis] diff --git a/workflows/tri-dispatch.yml b/workflows/tri-dispatch.yml index c74abb7..31329e5 100644 --- a/workflows/tri-dispatch.yml +++ b/workflows/tri-dispatch.yml @@ -5,23 +5,28 @@ steps: - id: smart-take model: smart prompt: | - {{input}} - - Give your best, most thorough answer. + DISPATCH, DO NOT ANSWER. Tri-dispatch requires three DIFFERENT + models answer independently. Invoke the Agent tool with a + non-Claude smart-tier subagent (`codex:codex-rescue` or equivalent + external). Report verbatim. Stop if no external subagent is + available — do NOT answer as yourself. Subagent prompt: "{{input}} + Give your best, most thorough answer." - id: general-take model: general prompt: | - {{input}} - - Give a clear, balanced answer. + DISPATCH, DO NOT ANSWER. Invoke the Agent tool with a non-Claude + general-tier subagent (prefer Gemini; at minimum distinct from + smart-take's subagent). Report verbatim. Stop if none available. + Subagent prompt: "{{input}} Give a clear, balanced answer." - id: fast-take model: fast prompt: | - {{input}} - - Give a quick, direct answer. + DISPATCH, DO NOT ANSWER. Invoke the Agent tool with a fast-tier + external subagent (distinct from the other two). Report verbatim. + Stop if none available. Subagent prompt: "{{input}} Give a quick, + direct answer." - id: dispatch-all parallel: [smart-take, general-take, fast-take] diff --git a/workflows/tri-review.yml b/workflows/tri-review.yml index eb561fd..0a5ed91 100644 --- a/workflows/tri-review.yml +++ b/workflows/tri-review.yml @@ -13,29 +13,40 @@ steps: - id: review-smart model: smart prompt: | - You are the senior reviewer. Review this code thoroughly: - - {{gather}} - - Focus on architecture, correctness, and security. Be detailed. + DISPATCH, DO NOT ANSWER. This is tri-review — the whole point is + model diversity. You (the calling model) must NOT write this review + yourself. Invoke the Agent tool with a non-Claude subagent + (`codex:codex-rescue` is the standard smart-tier external reviewer; + any available Gemini or external senior reviewer also qualifies). + Pass the diff from {{gather}} and report the subagent's verdict + verbatim. If no external-model subagent is available, stop and tell + the user — do NOT answer as yourself and advance. The subagent + prompt should be: "You are the senior reviewer. Review this code + thoroughly — architecture, correctness, security. Be detailed. + Diff: {{gather}}" - id: review-general model: general prompt: | - Review this code for quality and maintainability: - - {{gather}} - - Focus on readability, naming, duplication, and test coverage. + DISPATCH, DO NOT ANSWER. Invoke the Agent tool with a non-Claude + general-tier subagent (e.g. a Gemini-backed reviewer plugin, or + `codex:codex-rescue` if no Gemini is available — but prefer a + different model than review-smart used, or the tri-* property is + lost). Report verbatim. Stop if no external subagent is available. + The subagent prompt should be: "Review this code for quality and + maintainability — readability, naming, duplication, test coverage. + Diff: {{gather}}" - id: review-fast model: fast prompt: | - Quick review of this code: - - {{gather}} - - Flag anything obviously wrong: typos, missing error handling, dead code. + DISPATCH, DO NOT ANSWER. Invoke the Agent tool with a fast-tier + external subagent (Haiku-backed, Gemini-Flash-backed, or the fast + tier of whatever is available — distinct from review-smart and + review-general). Report verbatim. Stop if no external subagent is + available. The subagent prompt should be: "Quick review — flag + anything obviously wrong: typos, missing error handling, dead code. + Diff: {{gather}}" - id: dispatch-reviews parallel: [review-smart, review-general, review-fast] diff --git a/workflows/tri-security.yml b/workflows/tri-security.yml index 833e507..ad0cf76 100644 --- a/workflows/tri-security.yml +++ b/workflows/tri-security.yml @@ -13,36 +13,38 @@ steps: - id: audit-injection model: smart prompt: | - Security audit — injection and input validation: - - {{gather}} - - Check for: SQL injection, command injection, XSS, path traversal, - template injection, SSRF, deserialization attacks. - Rate each finding: critical, high, medium, low. + DISPATCH, DO NOT ANSWER. Tri-security requires three DIFFERENT + models audit in parallel. Invoke the Agent tool with a non-Claude + smart-tier subagent (`codex:codex-rescue` or equivalent external). + Report verbatim. Stop if no external subagent is available — do + NOT audit as yourself. Subagent prompt: "Security audit — injection + and input validation. Diff: {{gather}}. Check for: SQL injection, + command injection, XSS, path traversal, template injection, SSRF, + deserialization attacks. Rate each finding: critical, high, medium, + low." - id: audit-auth model: general prompt: | - Security audit — authentication and authorization: - - {{gather}} - - Check for: broken auth, missing access controls, insecure session - handling, privilege escalation, IDOR, JWT issues, hardcoded secrets. - Rate each finding: critical, high, medium, low. + DISPATCH, DO NOT ANSWER. Invoke the Agent tool with a non-Claude + general-tier subagent (prefer Gemini; at minimum distinct from + audit-injection's subagent). Report verbatim. Stop if none + available. Subagent prompt: "Security audit — authentication and + authorization. Diff: {{gather}}. Check for: broken auth, missing + access controls, insecure session handling, privilege escalation, + IDOR, JWT issues, hardcoded secrets. Rate each finding: critical, + high, medium, low." - id: audit-config model: fast prompt: | - Security audit — configuration and dependencies: - - {{gather}} - - Check for: exposed secrets, debug mode in prod, permissive CORS, - missing security headers, outdated dependencies with known CVEs, - insecure defaults. - Rate each finding: critical, high, medium, low. + DISPATCH, DO NOT ANSWER. Invoke the Agent tool with a fast-tier + external subagent (distinct from the other two). Report verbatim. + Stop if none available. Subagent prompt: "Security audit — + configuration and dependencies. Diff: {{gather}}. Check for: + exposed secrets, debug mode in prod, permissive CORS, missing + security headers, outdated dependencies with known CVEs, insecure + defaults. Rate each finding: critical, high, medium, low." - id: dispatch-audits parallel: [audit-injection, audit-auth, audit-config]