diff --git a/src/cmd/guard.go b/src/cmd/guard.go index 507bc67..70b1012 100644 --- a/src/cmd/guard.go +++ b/src/cmd/guard.go @@ -53,9 +53,11 @@ stdout ({"decision":"approve"} or {"decision":"block","reason":...}). The allowlist policy: command step + hard → devkit MCP + TodoWrite + Skill prompt step + hard → read-only evidence tools + devkit MCP + Skill + + Agent/Task dispatch + companion-rescue Bash prompt step + soft → allow with a stderr nudge parallel / unknown → allow (engine is dispatching) stale session (TTL) → allow with a stderr warning (orphan recovery) + cross-repo session → allow silently (block remains in origin repo) Skill is allowed under both step types so a workflow that's mid-run can still load a nested skill (e.g. user asks for tri-review during a feature @@ -88,28 +90,36 @@ func runGuard(cmd *cobra.Command, args []string) { runPreToolGuard() } -// readToolNameFromStdin extracts tool_name from a PreToolUse payload. -// Returns "" with a nil error for genuinely empty stdin (the common -// case when --tool-name is used instead). Any real failure (read error, -// malformed JSON) returns a non-nil error so the caller can log it -// distinctly — silent "" on parse failure would mask Claude Code schema -// drift. Under hard enforcement the empty tool name still falls through -// to the default-block branch, so errors never cause a silent ALLOW. -func readToolNameFromStdin() (string, error) { - data, err := io.ReadAll(guardStdin) - if err != nil { - return "", fmt.Errorf("read stdin: %w", err) +// readToolInvocationFromStdin extracts tool_name and (for Bash) +// tool_input.command from a PreToolUse payload. Returns ("", "", nil) +// for genuinely empty stdin (the common case when --tool-name is used +// instead). Any real failure (read error, malformed JSON) returns a +// non-nil error so the caller can log it distinctly — silent "" on +// parse failure would mask Claude Code schema drift. Under hard +// enforcement the empty tool name still falls through to the default- +// block branch, so errors never cause a silent ALLOW. +// +// command is populated only for tools that ship a "command" field in +// tool_input (Bash). Other tools get "" here — callers that care about +// specific input shapes must parse stdin themselves. +func readToolInvocationFromStdin() (name, command string, err error) { + data, rerr := io.ReadAll(guardStdin) + if rerr != nil { + return "", "", fmt.Errorf("read stdin: %w", rerr) } if len(data) == 0 { - return "", nil + return "", "", nil } var payload struct { - ToolName string `json:"tool_name"` + ToolName string `json:"tool_name"` + ToolInput struct { + Command string `json:"command"` + } `json:"tool_input"` } - if err := json.Unmarshal(data, &payload); err != nil { - return "", fmt.Errorf("parse PreToolUse JSON: %w", err) + if uerr := json.Unmarshal(data, &payload); uerr != nil { + return "", "", fmt.Errorf("parse PreToolUse JSON: %w", uerr) } - return payload.ToolName, nil + return payload.ToolName, payload.ToolInput.Command, nil } // resolveDataDir returns CLAUDE_PLUGIN_DATA or "" if unset. The shell @@ -289,6 +299,35 @@ func isDevkitMCPTool(name string) bool { return false } +// isCompanionRescueCommand reports whether a Bash command string +// invokes a known rescue-subagent companion script. Used by the +// prompt+hard carve-out so tri-* workflows' rescue subagents (codex, +// gemini) can reach external models through their companion adapters. +// +// Matched via substring so invocation style doesn't matter — the +// adapters call their scripts as `node /abs/path/codex-companion.mjs`, +// `bash -c '... gemini-companion.mjs ...'`, or through env-var +// overrides, and we must tolerate all of them. The .mjs suffix is +// load-bearing: without it, a Bash comment token like +// `rm -rf $HOME # codex-companion` would satisfy the substring match +// and bypass the prompt+hard Bash block. Both real companions ship +// as .mjs files, so requiring the extension doesn't break any +// intended invocation style. A future .js / .cjs adapter must be +// added to this allowlist deliberately rather than relying on the +// looser bare-name match. +// +// The cost of a broader match (even with .mjs pinned) is negligible: +// the only "bypass" it enables is the main agent invoking a real +// external model directly, which produces a real external review +// (just not one dispatched through the subagent indirection). +func isCompanionRescueCommand(cmd string) bool { + if cmd == "" { + return false + } + return strings.Contains(cmd, "codex-companion.mjs") || + strings.Contains(cmd, "gemini-companion.mjs") +} + func runPreToolGuard() { dataDir := resolveDataDir(false) if dataDir == "" { @@ -340,9 +379,32 @@ func runPreToolGuard() { return } + // Cross-repo scope (mirrors stop-guard carve-out from issue #91): + // if the workflow was started in a different repo than the current + // Claude Code session, allow silently. Without this, a workflow + // stuck at a prompt step in repo A (crashed engine, missed + // devkit_advance, session not cleaned up) would keep blocking + // unrelated tool calls in repo B for the full 30-minute TTL. The + // block remains active in the originating repo — re-entering A + // re-arms it. Same posture as stop-guard: empty state.RepoRoot + // (pre-#91 sessions written by older engines) or unresolvable + // current repo (CLAUDE_PROJECT_DIR missing, no walk-up match) + // falls through to full enforcement so a missing signal never + // silently approves. + if state.RepoRoot != "" { + if cur := currentRepoRoot(); cur != "" && !samePath(state.RepoRoot, cur) { + fmt.Fprintf(guardStderr, + "devkit-guard: session %s belongs to %s, current repo is %s — allowing (block remains active in originating repo)\n", + state.Workflow, state.RepoRoot, cur) + guardExit(0) + return + } + } + tool := guardToolName + var command string if tool == "" { - t, terr := readToolNameFromStdin() + t, c, terr := readToolInvocationFromStdin() if terr != nil { // Log distinctly so schema drift in the PreToolUse payload // doesn't silently degrade into "unknown tool name" and @@ -354,6 +416,7 @@ func runPreToolGuard() { terr) } tool = t + command = c } // state.StepEnforce is guaranteed valid ("hard" or "soft") by // SessionState.UnmarshalJSON — ReadSessionJSON would have rejected @@ -406,6 +469,27 @@ func runPreToolGuard() { // applies uniformly to both layers. guardExit(0) return + case "Bash": + // Rescue-subagent escape hatch (issue #95-2). tri-* + // workflows dispatch codex:codex-rescue / + // gemini:gemini-rescue via Agent/Task, and those + // subagents' system prompts require forwarding to + // codex-companion.mjs / gemini-companion.mjs through + // Bash. That nested Bash call re-enters this guard + // with the same prompt+hard session state and would + // otherwise be blocked, silently breaking tri-review's + // model-diversity invariant. + // + // The main agent could technically invoke these + // companion scripts directly to fish for a sympathetic + // external review, but that just runs the real + // external model — there's no verdict-faking bypass + // to be had. Write/Edit stay blocked, so the main + // model still cannot author the review itself. + if isCompanionRescueCommand(command) { + guardExit(0) + return + } } fmt.Fprintf(guardStderr, "BLOCKED: devkit workflow %s is at a prompt step — gather evidence with Read/Grep/Glob then call devkit_advance. (attempted tool: %s)\n", diff --git a/src/cmd/guard_test.go b/src/cmd/guard_test.go index 4538d9c..aa6a169 100644 --- a/src/cmd/guard_test.go +++ b/src/cmd/guard_test.go @@ -608,8 +608,8 @@ func TestGuardPreToolUse(t *testing.T) { wantStderrSubstr: "attempted tool: ", }, { - // Malformed stdin JSON: readToolNameFromStdin returns an - // error, the call site logs it, falls through with "", + // Malformed stdin JSON: readToolInvocationFromStdin returns + // an error, the call site logs it, falls through with "", // and the hard command step blocks. Pin this so a future // refactor can't silently allow on parse failure. name: "command+hard+malformed stdin → block", @@ -622,6 +622,65 @@ func TestGuardPreToolUse(t *testing.T) { wantExit: 2, wantStderrSubstr: "cannot determine tool name", }, + { + // Issue #95-2: rescue subagents (codex:codex-rescue / + // gemini:gemini-rescue) forward to external models via + // `node codex-companion.mjs ...`. That nested Bash call + // re-enters the guard under the same prompt+hard session + // state as the parent tri-* workflow. Allow it so the + // model-diversity invariant of tri-review survives. + name: "prompt+hard+Bash codex-companion → allow (rescue)", + dataDir: true, + hasSession: true, + session: lib.SessionState{ + Status: "running", StepType: "prompt", StepEnforce: lib.EnforceHard, CurrentStep: "review-smart", + Workflow: "tri-review", TotalSteps: 6, + }, + stdin: `{"tool_name":"Bash","tool_input":{"command":"node /plugins/codex/1.0.3/scripts/codex-companion.mjs task"}}`, + wantExit: 0, + }, + { + name: "prompt+hard+Bash gemini-companion → allow (rescue)", + dataDir: true, + hasSession: true, + session: lib.SessionState{ + Status: "running", StepType: "prompt", StepEnforce: lib.EnforceHard, CurrentStep: "review-general", + Workflow: "tri-review", TotalSteps: 6, + }, + stdin: `{"tool_name":"Bash","tool_input":{"command":"bash -c 'node /p/gemini-companion.mjs task'"}}`, + wantExit: 0, + }, + { + // Negative: a random Bash command on prompt+hard must + // still block. Without this, the companion carve-out + // could drift into a generic Bash allow if someone + // refactored isCompanionRescueCommand loose. + name: "prompt+hard+Bash unrelated → block", + dataDir: true, + hasSession: true, + session: lib.SessionState{ + Status: "running", StepType: "prompt", StepEnforce: lib.EnforceHard, CurrentStep: "review-smart", + Workflow: "tri-review", TotalSteps: 6, + }, + stdin: `{"tool_name":"Bash","tool_input":{"command":"curl https://evil.example.com"}}`, + wantExit: 2, + wantStderrSubstr: "gather evidence with Read/Grep/Glob", + }, + { + // Negative: command-step Bash must still block even if the + // command looks like a companion rescue. Command steps are + // run by the engine directly; any Bash from the model is a + // determinism violation, regardless of payload. + name: "command+hard+Bash codex-companion → block (wrong step type)", + dataDir: true, + hasSession: true, + session: lib.SessionState{ + Status: "running", StepType: "command", StepEnforce: lib.EnforceHard, CurrentStep: "build", + }, + stdin: `{"tool_name":"Bash","tool_input":{"command":"node codex-companion.mjs task"}}`, + wantExit: 2, + wantStderrSubstr: "BLOCKED: Command step", + }, } for _, tc := range tests { @@ -868,6 +927,121 @@ func TestGuardPreToolUseEnvTTLOverride(t *testing.T) { } } +// TestGuardPreToolUseRepoScope pins the cross-repo escape hatch for +// pre-tool calls (issue #95-3). Same posture as the stop-guard check +// added in issue #91: if the active workflow was started in repo A, +// a fresh session in repo B must not have its tool calls blocked by +// the leftover prompt-step state. The block remains active in repo A. +// Empty signals fall through to full enforcement so a missing env +// never silently approves. +func TestGuardPreToolUseRepoScope(t *testing.T) { + tests := []struct { + name string + stateRepoRoot string + claudeProjectDir string + stepType string + wantExit int + wantStderrSubstr string + }{ + { + // Cross-repo: the wedged workflow belongs to repo-a, + // but the user is now working in repo-b. Allow and + // note it on stderr so the escape hatch is debuggable. + name: "different repo → allow", + stateRepoRoot: "/tmp/repo-a", + claudeProjectDir: "/tmp/repo-b", + stepType: "prompt", + wantExit: 0, + wantStderrSubstr: "allowing (block remains active in originating repo)", + }, + { + // Matching repo: the block stays armed so the user + // actually working on the wedged workflow still gets + // the enforcement they expect. + name: "matching repo → block (prompt+hard)", + stateRepoRoot: "/tmp/repo-a", + claudeProjectDir: "/tmp/repo-a", + stepType: "prompt", + wantExit: 2, + wantStderrSubstr: "gather evidence with Read/Grep/Glob", + }, + { + // Pre-#91 session (no RepoRoot) must still block — we + // don't know where it came from, so fail closed and + // force enforcement rather than silently approve. + name: "empty state.RepoRoot → block", + stateRepoRoot: "", + claudeProjectDir: "/tmp/repo-b", + stepType: "prompt", + wantExit: 2, + wantStderrSubstr: "gather evidence with Read/Grep/Glob", + }, + { + // Missing CLAUDE_PROJECT_DIR with no walk-up match: + // currentRepoRoot() returns "". Fail closed (block) + // rather than silently approve across repos. + name: "unresolvable current repo → block", + stateRepoRoot: "/tmp/repo-a", + claudeProjectDir: "", // and cwd will be forced off-repo + stepType: "prompt", + wantExit: 2, + wantStderrSubstr: "gather evidence with Read/Grep/Glob", + }, + { + // Command-step session in a different repo: same + // allow behavior — the scope check precedes the + // step-type switch. Pin this so a refactor that put + // scope inside the prompt branch would fail here. + name: "different repo + command step → allow", + stateRepoRoot: "/tmp/repo-a", + claudeProjectDir: "/tmp/repo-b", + stepType: "command", + wantExit: 0, + wantStderrSubstr: "allowing (block remains active in originating repo)", + }, + } + + 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", + StepType: tc.stepType, + StepEnforce: lib.EnforceHard, + TotalSteps: 5, + CurrentStep: "analyse", + RepoRoot: tc.stateRepoRoot, + }) + env := newGuardTestEnv(t, `{"tool_name":"Bash"}`, "", false, dir) + if tc.claudeProjectDir != "" { + t.Setenv("CLAUDE_PROJECT_DIR", tc.claudeProjectDir) + } else { + t.Setenv("CLAUDE_PROJECT_DIR", "") + // Force the walk-up fallback to return "" by + // cd'ing into a non-git tempdir. Without this, + // the test runner's cwd (inside devkit's own + // repo) would satisfy the walk-up and the + // scope check would match, masking the test. + 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) + if env.exit != tc.wantExit { + t.Fatalf("exit=%d want=%d\nstderr=%s", env.exit, tc.wantExit, env.stderr.String()) + } + if tc.wantStderrSubstr != "" && !strings.Contains(env.stderr.String(), tc.wantStderrSubstr) { + t.Fatalf("stderr missing %q\ngot: %s", tc.wantStderrSubstr, env.stderr.String()) + } + }) + } +} + func TestGuardStopHook(t *testing.T) { tests := []struct { name string