diff --git a/README.md b/README.md index 03ee3d274..132e01131 100644 --- a/README.md +++ b/README.md @@ -32,7 +32,8 @@ your agentic loop while context is fresh. ```bash roborev init # layer 1: per-commit reviews roborev skills install -roborev agent-hook install # layer 2: mid-session fix loop +roborev agent-hook install # layer 2: mid-session fix loop (Codex/Claude) +roborev agent-hook install --agent droid # layer 2: mid-session fix loop (Factory Droid) ``` Before you ship, run the `/roborev-refine` skill: it re-reviews and fixes your @@ -52,8 +53,9 @@ roborev tui # View reviews in interactive UI If roborev is managed by a version manager, `roborev init` and `roborev agent-hook install` try to install hooks with the stable shim/symlink. You can also choose the exact binary path with -`roborev init --binary ~/.local/share/mise/shims/roborev` or -`roborev agent-hook install --binary ~/.local/share/mise/shims/roborev`. +`roborev init --binary ~/.local/share/mise/shims/roborev`, +`roborev agent-hook install --binary ~/.local/share/mise/shims/roborev`, or +`roborev agent-hook install --agent droid --binary ~/.local/share/mise/shims/roborev`. ![roborev review](https://roborev.io/assets/generated/tui-review.svg) @@ -63,8 +65,9 @@ You can also choose the exact binary path with git hooks. No remote review workflow required. - **Auto-Fix** - `roborev fix` feeds review findings to an agent that applies fixes and commits. `roborev refine` iterates until reviews pass. -- **Agent Hook** - Optional Codex and Claude Code harness hooks can prompt - active sessions to run `$roborev-fix` when roborev has open failed reviews. +- **Agent Hook** - Optional Codex, Claude Code, and Factory Droid harness hooks + can prompt active sessions to run the fix skill when roborev has open failed + reviews. - **Code Analysis** - Built-in analysis types (duplication, complexity, refactoring, test fixtures, dead code, security) that agents can fix automatically. @@ -92,11 +95,12 @@ command line non-interactively with `roborev fix`. changes and commits. The new commit gets reviewed automatically, closing the loop. -For Codex and Claude Code sessions, `roborev agent-hook install` can add an -optional harness hook that prompts the active session to invoke `$roborev-fix` -after configured turn, commit, or failed-review thresholds are met. The hook -uses a separate local `roborev-agent-hook` daemon for session counters; it does -not run inside the main roborev daemon. +For Codex, Claude Code, and Factory Droid sessions, `roborev agent-hook install` +can add an optional harness hook that prompts the active session to invoke +`$roborev-fix` (or `/roborev-fix` for Droid) after configured turn, commit, or +failed-review thresholds are met. +The hook uses a separate local `roborev-agent-hook` daemon for session counters; +it does not run inside the main roborev daemon. For fully automated iteration (advanced feature), use `refine`: @@ -192,6 +196,7 @@ If the hook rewrites files, re-stage them and re-run `git commit`. Use | `roborev refine` | Auto-fix loop: fix, re-review, repeat | | `roborev analyze ` | Run code analysis with optional auto-fix | | `roborev agent-hook install` | Install optional Codex/Claude agent harness hooks | +| `roborev agent-hook install --agent droid` | Install optional Factory Droid harness hooks | | `roborev compact` | Verify and consolidate open review findings | | `roborev show [sha]` | Display review for commit | | `roborev run ""` | Execute a task with an AI agent | @@ -274,6 +279,9 @@ hook, so a configured integration never goes dark unnoticed. | `ROBOREV_AGENT_HOOK_TURN_THRESHOLD` | Override agent-hook Stop threshold | | `ROBOREV_AGENT_HOOK_COMMIT_THRESHOLD` | Override agent-hook commit threshold | | `ROBOREV_AGENT_HOOK_FAILED_REVIEW_THRESHOLD` | Override agent-hook failed-review threshold | +| `ROBOREV_DROID_HOOK_TURN_THRESHOLD` | Override Factory Droid agent-hook Stop threshold | +| `ROBOREV_DROID_HOOK_COMMIT_THRESHOLD` | Override Factory Droid agent-hook commit threshold | +| `ROBOREV_DROID_HOOK_FAILED_REVIEW_THRESHOLD` | Override Factory Droid agent-hook failed-review threshold | | `NO_COLOR` | Set to any value to disable all color output ([no-color.org](https://no-color.org)) | ## Supported Agents diff --git a/cmd/roborev/agent_hook_cmd.go b/cmd/roborev/agent_hook_cmd.go index 9bfed0ce5..2e308abd9 100644 --- a/cmd/roborev/agent_hook_cmd.go +++ b/cmd/roborev/agent_hook_cmd.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "strconv" + "strings" "time" "github.com/spf13/cobra" @@ -31,13 +32,14 @@ func agentHookCmd() *cobra.Command { func agentHookRunCmd() *cobra.Command { opts := agenthook.DefaultOptions() + agent := "" cmd := &cobra.Command{ Use: "run", Short: "Read an agent hook payload from stdin and emit hook JSON", Args: cobra.NoArgs, DisableFlagsInUseLine: true, RunE: func(cmd *cobra.Command, _ []string) error { - resolved, err := agenthook.ResolveOptions(opts, agentHookFlagChanges(cmd)) + resolved, err := agenthook.ResolveOptionsForAgent(agent, opts, agentHookFlagChanges(cmd)) if err != nil { return err } @@ -45,6 +47,7 @@ func agentHookRunCmd() *cobra.Command { }, } addAgentHookRunFlags(cmd, &opts) + cmd.Flags().StringVar(&agent, "agent", agent, "hook option profile for this run: droid or empty/default") return cmd } @@ -117,6 +120,7 @@ func agentHookInstallCmd() *cobra.Command { Agent: "all", CodexConfigPath: agenthook.DefaultCodexHooksPath(), ClaudeConfigPath: agenthook.DefaultClaudeSettingsPath(), + Scope: "user", Timeout: 10 * time.Second, } cmd := &cobra.Command{ @@ -125,7 +129,11 @@ func agentHookInstallCmd() *cobra.Command { Args: cobra.NoArgs, DisableFlagsInUseLine: true, RunE: func(cmd *cobra.Command, _ []string) error { - command, notice, err := agenthook.ResolveHookCommandWithBinary(opts.Command, hookBinary) + runner := "agent-hook run" + if strings.EqualFold(strings.TrimSpace(opts.Agent), "droid") { + runner = "agent-hook run --agent droid" + } + command, notice, err := agenthook.ResolveHookCommandWithRunner(opts.Command, hookBinary, runner) if err != nil { return err } @@ -136,11 +144,13 @@ func agentHookInstallCmd() *cobra.Command { return agenthook.RunInstall(opts, cmd.OutOrStdout()) }, } - cmd.Flags().StringVar(&opts.Agent, "agent", opts.Agent, "agent config to update: codex, claude, or all") + cmd.Flags().StringVar(&opts.Agent, "agent", opts.Agent, "agent config to update: codex, claude, droid, or all") cmd.Flags().StringVar(&opts.Command, "command", opts.Command, "hook command to install; defaults to this binary plus 'agent-hook run'") cmd.Flags().StringVar(&hookBinary, "binary", "", "roborev binary path to bake into agent hooks (for version-manager shims)") + cmd.Flags().StringVar(&opts.ConfigPath, "config", opts.ConfigPath, "hook config path for a single selected agent") cmd.Flags().StringVar(&opts.CodexConfigPath, "codex-config", opts.CodexConfigPath, "Codex hooks.json path") cmd.Flags().StringVar(&opts.ClaudeConfigPath, "claude-config", opts.ClaudeConfigPath, "Claude settings.json path") + cmd.Flags().StringVar(&opts.Scope, "scope", opts.Scope, "Factory Droid config scope to update: user") cmd.Flags().Var(&agentHookSecondsOrDuration{d: &opts.Timeout}, "timeout", "Codex hook timeout (e.g. 10s, 1m, or bare integer seconds)") cmd.Flags().BoolVar(&opts.DryRun, "dry-run", opts.DryRun, "print what would change without writing files") return cmd @@ -154,22 +164,27 @@ func agentHookDumpCmd() *cobra.Command { Args: cobra.NoArgs, DisableFlagsInUseLine: true, RunE: func(cmd *cobra.Command, _ []string) error { - command, notice, err := agenthook.ResolveHookCommand(opts.Command) + runner := "agent-hook run" + if strings.EqualFold(strings.TrimSpace(opts.Agent), "droid") { + runner = "agent-hook run --agent droid" + } + command, notice, err := agenthook.ResolveHookCommandWithRunner(opts.Command, "", runner) if err != nil { return err } // Notices are advisory warnings; keep them off stdout so the dumped // JSON config stays clean for piping. if notice != "" { - fmt.Fprintln(cmd.ErrOrStderr(), notice) + fmt.Fprintln(cmd.ErrOrStderr(), agenthook.TranslateBinaryNotice(notice)) } opts.Command = command return agenthook.RunDump(opts, cmd.OutOrStdout()) }, } - cmd.Flags().StringVar(&opts.Agent, "agent", opts.Agent, "agent config to dump: codex or claude") + cmd.Flags().StringVar(&opts.Agent, "agent", opts.Agent, "agent config to dump: codex, claude, or droid") cmd.Flags().StringVar(&opts.Command, "command", opts.Command, "hook command to install; defaults to this binary plus 'agent-hook run'") cmd.Flags().StringVar(&opts.ConfigPath, "config", opts.ConfigPath, "config path to read and merge into; defaults to the agent's standard path") + cmd.Flags().StringVar(&opts.Scope, "scope", opts.Scope, "Factory Droid config scope to dump: user") cmd.Flags().Var(&agentHookSecondsOrDuration{d: &opts.Timeout}, "timeout", "Codex hook timeout (e.g. 10s, 1m, or bare integer seconds)") return cmd } @@ -206,12 +221,20 @@ func agentHookResetCmd() *cobra.Command { } func runAgentHook(opts agenthook.Options, stdin io.Reader, stdout, stderr io.Writer) error { + return runHook(opts, "agent-hook", stdin, stdout, stderr) +} + +// runHook is the shared core behind the agent-hook run command. It reads an +// agent harness hook payload from stdin, records it with the shared +// agenthook daemon, and emits the harness-compatible JSON output. label is used +// in diagnostics so the invoking agent knows which integration produced them. +func runHook(opts agenthook.Options, label string, stdin io.Reader, stdout, stderr io.Writer) error { var input agenthook.Input if err := json.NewDecoder(stdin).Decode(&input); err != nil { - return fmt.Errorf("decode agent hook input: %w", err) + return fmt.Errorf("decode %s input: %w", label, err) } if input.SessionID == "" { - return fmt.Errorf("agent hook input missing session_id") + return fmt.Errorf("%s input missing session_id", label) } resp, err := postAgentHook(context.Background(), agenthook.Request{ @@ -223,7 +246,7 @@ func runAgentHook(opts agenthook.Options, stdin io.Reader, stdout, stderr io.Wri RoborevServerAddr: opts.RoborevServerAddr, }) if err != nil { - fmt.Fprintf(stderr, "roborev agent-hook: %v\n", err) + fmt.Fprintf(stderr, "roborev %s: %v\n", label, err) return json.NewEncoder(stdout).Encode(map[string]any{}) } diff --git a/cmd/roborev/agent_hook_test.go b/cmd/roborev/agent_hook_test.go index c3de2cdd0..aa647eea6 100644 --- a/cmd/roborev/agent_hook_test.go +++ b/cmd/roborev/agent_hook_test.go @@ -44,6 +44,35 @@ func TestAgentHookDumpCodexCreatesHookConfig(t *testing.T) { assert.InDelta(10, firstAgentHookCommandTimeout(t, root, "Stop", command), 0) } +func TestAgentHookDumpDroidCreatesHookConfig(t *testing.T) { + assert := assert.New(t) + path := filepath.Join(t.TempDir(), "hooks.json") + command := "/tmp/roborev agent-hook run --agent droid" + + cmd := agentHookCmd() + var stdout bytes.Buffer + cmd.SetOut(&stdout) + cmd.SetArgs([]string{ + "dump", + "--agent", "droid", + "--command", command, + "--config", path, + "--scope", "user", + }) + + require.NoError(t, cmd.Execute()) + + var root map[string]any + require.NoError(t, json.Unmarshal(stdout.Bytes(), &root)) + assertAgentHookCommandCount(t, root, "PreToolUse", command, 1) + assertAgentHookCommandCount(t, root, "PostToolUse", command, 1) + assertAgentHookCommandCount(t, root, "Stop", command, 1) + assert.Equal("Execute", firstAgentHookMatcher(t, root, "PreToolUse")) + assert.Equal("Execute", firstAgentHookMatcher(t, root, "PostToolUse")) + assert.Empty(firstAgentHookMatcher(t, root, "Stop")) + assert.InDelta(10, firstAgentHookCommandTimeout(t, root, "Stop", command), 0) +} + func TestAgentHookInstallSupportsBinaryOverride(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "hooks.json") @@ -70,6 +99,31 @@ func TestAgentHookInstallSupportsBinaryOverride(t *testing.T) { assertAgentHookCommandContains(t, root, "Stop", binPath, 1) } +func TestAgentHookInstallDroidWritesFactoryHooks(t *testing.T) { + path := filepath.Join(t.TempDir(), "hooks.json") + + cmd := agentHookCmd() + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&out) + cmd.SetArgs([]string{ + "install", + "--agent", "droid", + "--command", "/tmp/roborev agent-hook run --agent droid", + "--config", path, + "--scope", "user", + }) + + require.NoError(t, cmd.Execute()) + assert.Contains(t, out.String(), "installed Factory Droid agent hooks") + + body, err := os.ReadFile(path) + require.NoError(t, err) + assert.Contains(t, string(body), "agent-hook run --agent droid") + assert.Contains(t, string(body), `"Execute"`) + assert.Contains(t, string(body), `"Stop"`) +} + func TestAgentHookDaemonHasLifecycleSubcommands(t *testing.T) { daemonCmd, _, err := agentHookCmd().Find([]string{"daemon"}) require.NoError(t, err) diff --git a/cmd/roborev/completions.go b/cmd/roborev/completions.go index cf5ab3c3f..aba99c78f 100644 --- a/cmd/roborev/completions.go +++ b/cmd/roborev/completions.go @@ -33,7 +33,12 @@ func registerReasoningCompletion(cmd *cobra.Command) { // Panics if the flag doesn't exist on the command (programming error). func registerReviewTypeCompletion(cmd *cobra.Command) { if err := cmd.RegisterFlagCompletionFunc("type", func(_ *cobra.Command, _ []string, _ string) ([]cobra.Completion, cobra.ShellCompDirective) { - return []cobra.Completion{config.ReviewTypeSecurity, config.ReviewTypeDesign, config.ReviewTypeLookahead}, cobra.ShellCompDirectiveNoFileComp + types := config.ExplicitReviewTypes() + completions := make([]cobra.Completion, len(types)) + for i, t := range types { + completions[i] = cobra.Completion(t) + } + return completions, cobra.ShellCompDirectiveNoFileComp }); err != nil { panic(fmt.Sprintf("registering review type completion for %s: %v", cmd.Name(), err)) } diff --git a/cmd/roborev/init_cmd.go b/cmd/roborev/init_cmd.go index af83ae888..c734f9036 100644 --- a/cmd/roborev/init_cmd.go +++ b/cmd/roborev/init_cmd.go @@ -146,7 +146,7 @@ func initCmd() *cobra.Command { }, } - cmd.Flags().StringVar(&agent, "agent", "", "default agent (codex, claude-code, gemini, copilot, opencode, cursor, kiro, kilo)") + cmd.Flags().StringVar(&agent, "agent", "", "default agent (codex, claude-code, gemini, copilot, opencode, cursor, kiro, kilo, droid, pi)") cmd.Flags().BoolVar(&noDaemon, "no-daemon", false, "skip auto-starting daemon (useful with systemd/launchd)") cmd.Flags().StringVar(&hookBinary, "binary", "", "roborev binary path to bake into git hooks (for version-manager shims)") registerAgentCompletion(cmd) diff --git a/cmd/roborev/quickstart.go b/cmd/roborev/quickstart.go index 48a1ec002..ba0c6995f 100644 --- a/cmd/roborev/quickstart.go +++ b/cmd/roborev/quickstart.go @@ -251,6 +251,7 @@ func agentsWithRequiredQuickstartSkills(statuses []skills.AgentStatus) []string labels := map[skills.Agent]string{ skills.AgentClaude: "Claude Code", skills.AgentCodex: "Codex", + skills.AgentDroid: "Factory Droid", } var installedFor []string for _, status := range statuses { diff --git a/cmd/roborev/review.go b/cmd/roborev/review.go index 99e239222..271af78b6 100644 --- a/cmd/roborev/review.go +++ b/cmd/roborev/review.go @@ -151,8 +151,10 @@ Examples: } // Validate --type flag - if reviewType != "" && reviewType != "security" && reviewType != "design" && reviewType != "lookahead" { - return usageErr(cmd, fmt.Errorf("invalid --type %q (valid: security, design, lookahead)", reviewType)) + switch reviewType { + case "", config.ReviewTypeSecurity, config.ReviewTypeDesign, config.ReviewTypeLookahead: + default: + return usageErr(cmd, fmt.Errorf("invalid --type %q (valid: %s)", reviewType, config.ExplicitReviewTypesHelp())) } // Auto-install/upgrade hooks when running from CLI @@ -455,10 +457,7 @@ func runLocalReview(cmd *cobra.Command, repoPath, gitRef, diffContent string, di } // Map review_type to config workflow (matches daemon behavior) - workflow := "review" - if !config.IsDefaultReviewType(reviewType) { - workflow = reviewType - } + workflow := config.WorkflowForReviewType(reviewType) if err := config.ValidateRepoConfig(repoPath); err != nil { return fmt.Errorf("resolve workflow config: %w", err) } diff --git a/cmd/roborev/skills.go b/cmd/roborev/skills.go index 71b06fa1c..611d64508 100644 --- a/cmd/roborev/skills.go +++ b/cmd/roborev/skills.go @@ -2,6 +2,7 @@ package main import ( "fmt" + "slices" "strings" "github.com/spf13/cobra" @@ -13,7 +14,7 @@ func skillsCmd() *cobra.Command { cmd := &cobra.Command{ Use: "skills", Short: "Manage AI agent skills", - Long: "Install and manage roborev skills for AI agents (Claude Code, Codex)", + Long: "Install and manage roborev skills for AI agents (Claude Code, Codex, Factory Droid)", RunE: func(cmd *cobra.Command, args []string) error { available, err := skills.ListSkills() if err != nil { @@ -35,6 +36,7 @@ func skillsCmd() *cobra.Command { agents := []agentLabel{ {skills.AgentClaude, "Claude Code", "/"}, {skills.AgentCodex, "Codex", "$"}, + {skills.AgentDroid, "Factory Droid", "/"}, } fmt.Println("Skills:") @@ -45,6 +47,10 @@ func skillsCmd() *cobra.Command { } for _, a := range agents { + if !slices.Contains(s.SupportedAgents, a.agent) { + continue + } + var as *skills.AgentStatus for i := range statuses { if statuses[i].Agent == a.agent { @@ -108,6 +114,7 @@ func skillsCmd() *cobra.Command { Skills are installed for agents whose config directories exist: - Claude Code: ~/.claude/skills/ - Codex: ~/.codex/skills/ + - Factory Droid: ~/.factory/skills/ This command is idempotent - running it multiple times is safe.`, RunE: func(cmd *cobra.Command, args []string) error { @@ -153,7 +160,7 @@ This command is idempotent - running it multiple times is safe.`, } if !anyInstalled { - fmt.Println("\nNo agents found. Install Claude Code or Codex first, then run this command.") + fmt.Println("\nNo agents found. Install Claude Code, Codex, or Factory Droid first, then run this command.") } else { fmt.Println("\nSkills installed! Try:") for _, agent := range installedAgents { @@ -162,6 +169,8 @@ This command is idempotent - running it multiple times is safe.`, fmt.Println(" Claude Code: /roborev-review, /roborev-review-branch, /roborev-design-review, /roborev-design-review-branch, /roborev-fix, /roborev-respond") case skills.AgentCodex: fmt.Println(" Codex: $roborev-review, $roborev-review-branch, $roborev-design-review, $roborev-design-review-branch, $roborev-fix, $roborev-respond") + case skills.AgentDroid: + fmt.Println(" Factory Droid: /roborev-review, /roborev-review-branch, /roborev-design-review, /roborev-design-review-branch, /roborev-lookahead-review, /roborev-lookahead-review-branch, /roborev-fix, /roborev-respond") } } } diff --git a/cmd/roborev/update.go b/cmd/roborev/update.go index 7cf2031bc..2787d848a 100644 --- a/cmd/roborev/update.go +++ b/cmd/roborev/update.go @@ -465,7 +465,7 @@ launchd or systemd).`, // Update skills using the NEW binary (current process has old embedded skills) // Use "skills update" to only update agents that already have skills installed - if skills.IsInstalled(skills.AgentClaude) || skills.IsInstalled(skills.AgentCodex) { + if skills.IsInstalled(skills.AgentClaude) || skills.IsInstalled(skills.AgentCodex) || skills.IsInstalled(skills.AgentDroid) { fmt.Print("Updating skills... ") newBinary := filepath.Join(binDir, "roborev") if runtime.GOOS == "windows" { diff --git a/docs/agent-hook.md b/docs/agent-hook.md index ebf6453c2..2fc4a1ffc 100644 --- a/docs/agent-hook.md +++ b/docs/agent-hook.md @@ -1,10 +1,10 @@ --- title: Agent Hook -description: Let Codex and Claude Code sessions run roborev-fix mid-session by watching the agent boundary +description: Let Codex, Claude Code, and Factory Droid sessions run roborev-fix mid-session by watching the agent boundary --- -`roborev agent-hook` is an opt-in integration with the Codex and Claude Code harness hook systems. roborev reviews your commits in the background; `agent-hook` watches the agent boundary and, once review work has piled up, returns one instruction telling the agent to run the `$roborev-fix` skill before the session goes cold. It is the in-tree replacement for the standalone `roborev-hook` tool. +`roborev agent-hook` is an opt-in integration with the Codex, Claude Code, and Factory Droid harness hook systems. roborev reviews your commits in the background; `agent-hook` watches the agent boundary and, once review work has piled up, returns one instruction telling the agent to run the fix skill before the session goes cold. It is the in-tree replacement for the standalone `roborev-hook` tool. !!! note This is different from [Review Hooks](/guides/hooks/), which run your own shell commands when a review completes. Agent Hook plugs into the coding agent's own hook system to steer the agent itself. @@ -17,7 +17,7 @@ description: Let Codex and Claude Code sessions run roborev-fix mid-session by w Agents are good at making progress. They are worse at remembering to come back after a background reviewer finishes, especially when reviews happen out of band like they do with roborev. -`agent-hook` closes that gap. It sits behind Codex or Claude Code hooks, counts what happened in the current session, checks roborev for failed reviews, and returns one direct instruction when there is review work to fix: +`agent-hook` closes that gap. It sits behind Codex, Claude Code, or Factory Droid hooks, counts what happened in the current session, checks roborev for failed reviews, and returns one direct instruction when there is review work to fix: ```text Invoke the $roborev-fix skill now. @@ -26,21 +26,21 @@ Invoke the $roborev-fix skill now. That turns review into part of the agent's normal rhythm: write code, get reviewed, fix the review, continue. !!! note - The default instruction uses Codex's `$roborev-fix` skill syntax. Claude Code refers to the same skill as `/roborev-fix` (see [Agent-Specific Syntax](/guides/agent-skills/#agent-specific-syntax)). A single instruction string is sent to both harnesses; override `instruction` (see [Configuration](#configuration)) if you prefer different wording. + The default Codex/Claude instruction uses Codex's `$roborev-fix` skill syntax. Claude Code refers to the same skill as `/roborev-fix` (see [Agent-Specific Syntax](/guides/agent-skills/#agent-specific-syntax)). Factory Droid uses a separate default instruction that names `roborev-fix` generically. Override `instruction` (see [Configuration](#configuration)) if you prefer different wording. ## What It Watches `agent-hook` tracks three signals per session: - **Turns.** `Stop` hooks, so long-running sessions get periodic review repair. -- **Commits.** `PostToolUse` Bash hooks that produce commits. A `PreToolUse` Bash hook seeds the per-commit baseline so the count stays accurate. +- **Commits.** `PostToolUse` shell hooks that produce commits. Codex and Claude Code use Bash hooks; Factory Droid uses the `Execute` tool. A matching `PreToolUse` hook seeds the per-commit baseline so the count stays accurate. - **Failed reviews.** Open, non-closed roborev reviews with a failed verdict. `agent-hook` resolves the repository from the agent's working directory, so outside a git repository it returns `{}` and stays out of the way. Reminders also depend on the roborev daemon reporting an open failed review, so a repository roborev does not track never produces a reminder. If the main roborev daemon is unavailable, the failed-review check is skipped. Turn and commit counts still work through the local hook daemon, but they only prompt the agent once roborev reports at least one open failed review. -Commit-producing Bash calls are counted by default, but commit-based prompts stay off unless `commit_threshold` is set above `0`. Failed-review counts are scoped to the current git branch. Older jobs without a stored branch are included, matching `roborev fix` discovery. +Commit-producing shell calls are counted by default, but commit-based prompts stay off unless `commit_threshold` is set above `0`. Failed-review counts are scoped to the current git branch. Older jobs without a stored branch are included, matching `roborev fix` discovery. ## Quick Start @@ -58,10 +58,19 @@ roborev agent-hook install By default this updates both `~/.codex/hooks.json` and `~/.claude/settings.json`, registering `PreToolUse`, `PostToolUse`, and `Stop` hooks. Existing hooks are preserved, and repeated installs are idempotent. Use `--agent codex` or `--agent claude` to update only one harness, and `--dry-run` to report what would change without writing. +For Factory Droid, install the Droid profile explicitly: + +```bash +roborev agent-hook install --agent droid +``` + +This updates the user-scoped `~/.factory/hooks.json`, registering `PreToolUse`, `PostToolUse` (both matching Droid's `Execute` tool), and `Stop` hooks. Project-scoped Factory hooks are intentionally not supported by roborev because `.factory/hooks.json` is executable repo-local configuration. Do not commit Factory hook commands to a repository; install the Droid hook in your user scope instead. + When roborev is installed through a version manager such as mise, `agent-hook install` resolves the same stable roborev shim used by `roborev init`. To pin the exact binary path baked into the agent hook command, use `--binary`: ```bash roborev agent-hook install --binary ~/.local/share/mise/shims/roborev +roborev agent-hook install --agent droid --binary ~/.local/share/mise/shims/roborev ``` Use `--command` only when you want to provide the full hook command yourself. `--binary` and `--command` are mutually exclusive. @@ -71,6 +80,7 @@ For declarative setups (Nix home-manager, dotfiles) where editing those files in ```bash roborev agent-hook dump --agent codex roborev agent-hook dump --agent claude +roborev agent-hook dump --agent droid --scope user ``` ## Runtime Model @@ -81,7 +91,13 @@ Agent harnesses invoke: roborev agent-hook run ``` -`run` reads a hook payload on stdin, talks to a small local `roborev-agent-hook` daemon, and emits the hook response JSON the harness expects. This daemon is separate from the main roborev daemon and stores only local session counters under: +Factory Droid invokes the same runtime with its profile selected: + +```bash +roborev agent-hook run --agent droid +``` + +`run` reads a hook payload on stdin, talks to a small local `roborev-agent-hook` daemon, and emits the hook response JSON the harness expects. This daemon is shared by Codex, Claude Code, and Factory Droid profiles. It is separate from the main roborev daemon and stores only local session counters under: ```text ${ROBOREV_DATA_DIR:-~/.roborev}/agent-hook/ @@ -126,9 +142,35 @@ run flags > environment variables > [agent_hook] config > defaults `ROBOREV_AGENT_HOOK_ROBOREV_ADDR` and `ROBOREV_AGENT_HOOK_DAEMON_ADDR` are operational overrides only and are not persisted in TOML. `ROBOREV_AGENT_HOOK_DAEMON_ADDR` points `run` at a specific local hook daemon address. +Factory Droid uses its own `[droid_hook]` section and environment variables: + +```toml +[droid_hook] +turn_threshold = 5 +commit_threshold = 0 +failed_review_threshold = 4 +instruction = "Run the roborev-fix skill to address the unresolved roborev findings, then continue." +``` + +| Trigger | Default | TOML key | `run` flag | Environment variable | +|---------|---------|----------|------------|----------------------| +| Stop hooks (turns) | `5` | `turn_threshold` | `--turn-threshold` | `ROBOREV_DROID_HOOK_TURN_THRESHOLD` | +| Commit-producing `Execute` calls | `0` | `commit_threshold` | `--commit-threshold` | `ROBOREV_DROID_HOOK_COMMIT_THRESHOLD` | +| Open failed reviews | `4` | `failed_review_threshold` | `--failed-review-threshold` | `ROBOREV_DROID_HOOK_FAILED_REVIEW_THRESHOLD` | +| Continuation instruction | `Run the roborev-fix skill...` | `instruction` | `--instruction` | `ROBOREV_DROID_HOOK_INSTRUCTION` | +| roborev daemon address | runtime discovery | | `--roborev-server` | `ROBOREV_DROID_HOOK_ROBOREV_ADDR` | + +Droid values resolve in this order: + +```text +run flags > environment variables > [droid_hook] config > defaults +``` + +`ROBOREV_DROID_HOOK_ROBOREV_ADDR` is an operational override only and is not persisted in TOML. `ROBOREV_AGENT_HOOK_DAEMON_ADDR` still points `run` at a specific local hook daemon address shared by every agent-hook profile. + ## Inspecting Sessions -Inspect tracked session counters, including `remind_count` (the number of `$roborev-fix` reminders emitted), as JSON: +Inspect tracked session counters, including `remind_count` (the number of fix-skill reminders emitted), as JSON. Because the daemon is shared, this shows sessions from every integration: ```bash roborev agent-hook status diff --git a/docs/commands.md b/docs/commands.md index 324d8889e..e3d373bb0 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -595,12 +595,29 @@ roborev agent-hook daemon start # start | status | stop | restart | Flag | Description | |------|-------------| -| `--agent ` | Target harness: `codex`, `claude`, or `all` (`all` for `install` only) | +| `--agent ` | Target harness: `codex`, `claude`, `droid`, or `all` (`all` for `install` only) | | `--dry-run` | Report whether each target needs changes without writing (`install`) | | `--command ` | Override the installed hook command (default: resolved roborev binary + `agent-hook run`) | | `--binary ` | Resolve and bake this roborev binary path into installed agent hooks. Mutually exclusive with `--command` | +| `--scope user` | Factory Droid config scope (`--agent droid` only) | -`roborev agent-hook` is an opt-in Codex and Claude Code integration that prompts the agent to run `$roborev-fix` when review work piles up. See [Agent Hook](/agent-hook/). +`roborev agent-hook` is an opt-in Codex, Claude Code, and Factory Droid integration that prompts the agent to run the fix skill when review work piles up. See [Agent Hook](/agent-hook/). + +```bash +roborev agent-hook install --agent droid # Install Factory Droid hook entries (user scope) +roborev agent-hook install --agent droid --binary ~/.local/bin/roborev +roborev agent-hook dump --agent droid --scope user # Print hook config JSON (declarative setups) +roborev agent-hook run --agent droid # Read a hook payload from stdin (Droid calls this) +roborev agent-hook status # Tracked session counters as JSON (shared daemon) +roborev agent-hook reset # Reset one session (or --all) +``` + +Use `--agent droid` to install Factory Droid hook entries that prompt Droid to +run `/roborev-fix` when review work piles up, sharing the same local state +daemon. The Droid profile installs to user scope by default +(`~/.factory/hooks.json`); roborev does not install project-scoped Factory hooks +because `.factory/hooks.json` is executable repo-local configuration. See +[Agent Hook](/agent-hook/). ## Checking Agents diff --git a/docs/index.md b/docs/index.md index cc5bede42..9571165d5 100644 --- a/docs/index.md +++ b/docs/index.md @@ -64,7 +64,7 @@ AI coding agents write code fast, but they make mistakes. Most review feedback c 1. **Ask your agents to commit often**, ideally every turn of work 2. **roborev reviews** each commit in the background -3. **Bring review work back into the agent session** with [`agent-hook`](/agent-hook/) or check the TUI (`roborev tui`) as findings arrive +3. **Bring review work back into the agent session** with [`agent-hook`](/agent-hook/) (`--agent droid` for Factory Droid) or check the TUI (`roborev tui`) as findings arrive 4. **Address findings** by letting the hook prompt the fix skill, copying reviews into your agent, using [`/roborev-fix`](/guides/agent-skills/), or running `roborev fix` Every commit gets reviewed. Issues surface in seconds, not hours. Open reviews stay in the TUI queue until explicitly addressed and closed, so nothing falls through the cracks. @@ -77,7 +77,7 @@ Every commit gets reviewed. Issues surface in seconds, not hours. Open reviews s - **Agent-Ready Feedback** - Use `agent-hook` to prompt Codex or Claude Code to run the fix skill when review work piles up. You can also copy findings into your agent session, use `/roborev-fix`, or run `roborev fix` to apply fixes automatically. + Use `agent-hook` to prompt Codex, Claude Code, or Droid (`--agent droid`) to run the fix skill when review work piles up. You can also copy findings into your agent session, use `/roborev-fix`, or run `roborev fix` to apply fixes automatically. - **Code Analysis** diff --git a/internal/agenthook/config.go b/internal/agenthook/config.go index a905642bc..47bf9ac2b 100644 --- a/internal/agenthook/config.go +++ b/internal/agenthook/config.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "strconv" + "strings" "go.kenn.io/roborev/internal/config" ) @@ -13,6 +14,7 @@ const ( DefaultCommitThreshold = 0 DefaultFailedReviewThreshold = 4 DefaultInstruction = "Invoke the $roborev-fix skill now." + DefaultDroidInstruction = "Run the roborev-fix skill to address the unresolved roborev findings, then continue." TurnThresholdEnv = "ROBOREV_AGENT_HOOK_TURN_THRESHOLD" CommitThresholdEnv = "ROBOREV_AGENT_HOOK_COMMIT_THRESHOLD" @@ -20,6 +22,12 @@ const ( InstructionEnv = "ROBOREV_AGENT_HOOK_INSTRUCTION" RoborevServerEnv = "ROBOREV_AGENT_HOOK_ROBOREV_ADDR" DaemonAddrEnv = "ROBOREV_AGENT_HOOK_DAEMON_ADDR" + + DroidTurnThresholdEnv = "ROBOREV_DROID_HOOK_TURN_THRESHOLD" + DroidCommitThresholdEnv = "ROBOREV_DROID_HOOK_COMMIT_THRESHOLD" + DroidFailedReviewThresholdEnv = "ROBOREV_DROID_HOOK_FAILED_REVIEW_THRESHOLD" + DroidInstructionEnv = "ROBOREV_DROID_HOOK_INSTRUCTION" + DroidRoborevServerEnv = "ROBOREV_DROID_HOOK_ROBOREV_ADDR" ) type Options struct { @@ -42,6 +50,22 @@ func DefaultOptions() Options { } func ResolveOptions(cli Options, changed map[string]bool) (Options, error) { + return ResolveOptionsForAgent("", cli, changed) +} + +func ResolveOptionsForAgent(agent string, cli Options, changed map[string]bool) (Options, error) { + agent = strings.ToLower(strings.TrimSpace(agent)) + switch agent { + case "", "agent", "codex", "claude": + return resolveAgentOptions(cli, changed) + case "droid": + return resolveDroidOptions(cli, changed) + default: + return Options{}, fmt.Errorf("agent must be codex, claude, droid, or empty") + } +} + +func resolveAgentOptions(cli Options, changed map[string]bool) (Options, error) { opts := DefaultOptions() if changed["config"] { opts.ConfigPath = cli.ConfigPath @@ -77,6 +101,43 @@ func ResolveOptions(cli Options, changed map[string]bool) (Options, error) { return opts, nil } +func resolveDroidOptions(cli Options, changed map[string]bool) (Options, error) { + opts := DefaultOptions() + opts.Instruction = DefaultDroidInstruction + if changed["config"] { + opts.ConfigPath = cli.ConfigPath + } + if err := applyDroidConfig(&opts); err != nil { + return Options{}, err + } + applyDroidEnv(&opts) + if changed["turn-threshold"] { + opts.TurnThreshold = cli.TurnThreshold + } + if changed["commit-threshold"] { + opts.CommitThreshold = cli.CommitThreshold + } + if changed["failed-review-threshold"] { + opts.FailedReviewThreshold = cli.FailedReviewThreshold + } + if changed["instruction"] { + opts.Instruction = cli.Instruction + } + if changed["roborev-server"] { + opts.RoborevServerAddr = cli.RoborevServerAddr + } + if opts.TurnThreshold < 0 { + return Options{}, fmt.Errorf("turn threshold must be >= 0") + } + if opts.CommitThreshold < 0 { + return Options{}, fmt.Errorf("commit threshold must be >= 0") + } + if opts.FailedReviewThreshold < 0 { + return Options{}, fmt.Errorf("failed review threshold must be >= 0") + } + return opts, nil +} + func applyConfig(opts *Options) error { cfg, err := config.LoadGlobalFrom(opts.ConfigPath) if err != nil { @@ -91,6 +152,20 @@ func applyConfig(opts *Options) error { return nil } +func applyDroidConfig(opts *Options) error { + cfg, err := config.LoadGlobalFrom(opts.ConfigPath) + if err != nil { + return fmt.Errorf("load roborev config %s: %w", opts.ConfigPath, err) + } + opts.TurnThreshold = cfg.DroidHook.TurnThreshold + opts.CommitThreshold = cfg.DroidHook.CommitThreshold + opts.FailedReviewThreshold = cfg.DroidHook.FailedReviewThreshold + if cfg.DroidHook.Instruction != "" { + opts.Instruction = cfg.DroidHook.Instruction + } + return nil +} + func applyEnv(opts *Options) { if v, ok := envIntValue(TurnThresholdEnv); ok { opts.TurnThreshold = v @@ -109,6 +184,24 @@ func applyEnv(opts *Options) { } } +func applyDroidEnv(opts *Options) { + if v, ok := envIntValue(DroidTurnThresholdEnv); ok { + opts.TurnThreshold = v + } + if v, ok := envIntValue(DroidCommitThresholdEnv); ok { + opts.CommitThreshold = v + } + if v, ok := envIntValue(DroidFailedReviewThresholdEnv); ok { + opts.FailedReviewThreshold = v + } + if v := os.Getenv(DroidInstructionEnv); v != "" { + opts.Instruction = v + } + if v := os.Getenv(DroidRoborevServerEnv); v != "" { + opts.RoborevServerAddr = v + } +} + func envIntValue(name string) (int, bool) { raw := os.Getenv(name) if raw == "" { diff --git a/internal/agenthook/config_test.go b/internal/agenthook/config_test.go index 878349849..1ff787a04 100644 --- a/internal/agenthook/config_test.go +++ b/internal/agenthook/config_test.go @@ -39,6 +39,54 @@ instruction = "Run roborev fix." assert.Equal(t, "Run roborev fix.", opts.Instruction) } +func TestResolveOptionsForAgentDroidUsesDroidHookConfig(t *testing.T) { + clearAgentHookEnv(t) + path := writeAgentHookConfig(t, ` +[agent_hook] +turn_threshold = 99 +instruction = "agent instruction" + +[droid_hook] +turn_threshold = 6 +commit_threshold = 2 +failed_review_threshold = 3 +instruction = "droid instruction" +`) + + opts, err := ResolveOptionsForAgent("droid", Options{ConfigPath: path}, map[string]bool{"config": true}) + + require.NoError(t, err) + assert.Equal(t, 6, opts.TurnThreshold) + assert.Equal(t, 2, opts.CommitThreshold) + assert.Equal(t, 3, opts.FailedReviewThreshold) + assert.Equal(t, "droid instruction", opts.Instruction) +} + +func TestResolveOptionsForAgentDroidEnvOverridesConfig(t *testing.T) { + clearAgentHookEnv(t) + path := writeAgentHookConfig(t, ` +[droid_hook] +turn_threshold = 6 +commit_threshold = 2 +failed_review_threshold = 3 +instruction = "config instruction" +`) + t.Setenv(DroidTurnThresholdEnv, "7") + t.Setenv(DroidCommitThresholdEnv, "4") + t.Setenv(DroidFailedReviewThresholdEnv, "5") + t.Setenv(DroidInstructionEnv, "env instruction") + t.Setenv(DroidRoborevServerEnv, "127.0.0.1:9999") + + opts, err := ResolveOptionsForAgent("droid", Options{ConfigPath: path}, map[string]bool{"config": true}) + + require.NoError(t, err) + assert.Equal(t, 7, opts.TurnThreshold) + assert.Equal(t, 4, opts.CommitThreshold) + assert.Equal(t, 5, opts.FailedReviewThreshold) + assert.Equal(t, "env instruction", opts.Instruction) + assert.Equal(t, "127.0.0.1:9999", opts.RoborevServerAddr) +} + func TestResolveOptionsAllowsZeroTurnThresholdFromConfig(t *testing.T) { clearAgentHookEnv(t) path := writeAgentHookConfig(t, ` @@ -162,6 +210,11 @@ func clearAgentHookEnv(t *testing.T) { InstructionEnv, RoborevServerEnv, DaemonAddrEnv, + DroidTurnThresholdEnv, + DroidCommitThresholdEnv, + DroidFailedReviewThresholdEnv, + DroidInstructionEnv, + DroidRoborevServerEnv, } { t.Setenv(name, "") } diff --git a/internal/agenthook/detect.go b/internal/agenthook/detect.go index 8651aeac0..c0158339e 100644 --- a/internal/agenthook/detect.go +++ b/internal/agenthook/detect.go @@ -30,7 +30,7 @@ func Installed(path string) (bool, error) { func jsonContainsRoborevHook(v any) bool { switch t := v.(type) { case string: - return isRoborevAgentHookCommand(t) + return isRoborevHookCommand(t, agentHookRunner) case []any: if slices.ContainsFunc(t, jsonContainsRoborevHook) { return true diff --git a/internal/agenthook/install.go b/internal/agenthook/install.go index 5e22288ba..b88e14d40 100644 --- a/internal/agenthook/install.go +++ b/internal/agenthook/install.go @@ -7,17 +7,39 @@ import ( "io" "os" "path/filepath" + "runtime" "strings" "time" + gitpkg "go.kenn.io/roborev/internal/git" "go.kenn.io/roborev/internal/githook" ) +// agentHookRunner is the roborev subcommand suffix baked into Codex/Claude hook +// commands. It also serves as the stale-command sentinel: an install replaces +// any prior command hook that invokes this runner, regardless of the roborev +// binary path baked in by an earlier install. +const agentHookRunner = "agent-hook run" + +// droidAgentHookRunner selects the Factory Droid profile through the shared +// agent-hook runtime. It is deliberately more specific than agentHookRunner so a +// Droid install never clobbers plain Codex/Claude hook entries and vice versa. +const droidAgentHookRunner = "agent-hook run --agent droid" + +// ExecuteMatcher is the Factory Droid tool name for shell commands. PreToolUse +// and PostToolUse hooks match it to track turns and commits, mirroring the +// Codex/Claude Bash matcher. +const ExecuteMatcher = "Execute" + +var droidPathCaseInsensitive = runtime.GOOS == "windows" + type InstallOptions struct { Agent string Command string + ConfigPath string CodexConfigPath string ClaudeConfigPath string + Scope string Timeout time.Duration DryRun bool } @@ -26,10 +48,15 @@ type DumpOptions struct { Agent string Command string ConfigPath string + Scope string Timeout time.Duration } -type installSpec struct { +// InstallSpec describes a single hook entry to install: the harness event, an +// optional tool-name matcher, the command to run, and an optional timeout. It is +// shared by every integration that reuses this package's JSON hook format +// (Codex, Claude, and Factory Droid all use the same shape). +type InstallSpec struct { Event string Matcher string Command string @@ -42,30 +69,62 @@ func RunInstall(opts InstallOptions, stdout io.Writer) error { if agent == "" { agent = "all" } - if agent != "all" && agent != "codex" && agent != "claude" { - return fmt.Errorf("agent must be codex, claude, or all") + if agent != "all" && agent != "codex" && agent != "claude" && agent != "droid" { + return fmt.Errorf("agent must be codex, claude, droid, or all") } if opts.Timeout < 0 { return fmt.Errorf("timeout must be >= 0") } - command, err := resolveInstallCommand(opts.Command) + command, err := resolveInstallCommand(agent, opts.Command) if err != nil { return err } + if agent == "all" && opts.ConfigPath != "" { + return fmt.Errorf("--config is only supported when installing a single agent") + } if agent == "all" || agent == "codex" { - changed, err := installSpecs(opts.CodexConfigPath, codexSpecs(command, opts.Timeout), opts.DryRun) + path := opts.CodexConfigPath + if agent == "codex" && opts.ConfigPath != "" { + path = opts.ConfigPath + } + changed, err := InstallSpecs(path, codexSpecs(command, opts.Timeout), agentHookRunner, opts.DryRun) if err != nil { return err } - printInstallResult(stdout, "Codex", opts.CodexConfigPath, changed, opts.DryRun) + printInstallResult(stdout, "Codex", path, changed, opts.DryRun) } if agent == "all" || agent == "claude" { - changed, err := installSpecs(opts.ClaudeConfigPath, claudeSpecs(command), opts.DryRun) + path := opts.ClaudeConfigPath + if agent == "claude" && opts.ConfigPath != "" { + path = opts.ConfigPath + } + changed, err := InstallSpecs(path, claudeSpecs(command), agentHookRunner, opts.DryRun) + if err != nil { + return err + } + printInstallResult(stdout, "Claude", path, changed, opts.DryRun) + } + if agent == "droid" { + scope, err := normalizeDroidScope(opts.Scope) if err != nil { return err } - printInstallResult(stdout, "Claude", opts.ClaudeConfigPath, changed, opts.DryRun) + path := opts.ConfigPath + if path == "" { + path = DefaultDroidHooksPath(scope) + } + if path == "" { + return fmt.Errorf("could not resolve Factory Droid hooks path for scope %q", scope) + } + if err := validateDroidHooksPath(path); err != nil { + return err + } + changed, err := InstallSpecs(path, droidSpecs(command, opts.Timeout), droidAgentHookRunner, opts.DryRun) + if err != nil { + return err + } + printInstallResult(stdout, "Factory Droid", path, changed, opts.DryRun) } return nil } @@ -75,13 +134,14 @@ func RunDump(opts DumpOptions, stdout io.Writer) error { if opts.Timeout < 0 { return fmt.Errorf("timeout must be >= 0") } - command, err := resolveInstallCommand(opts.Command) + command, err := resolveInstallCommand(agent, opts.Command) if err != nil { return err } path := opts.ConfigPath - var specs []installSpec + var specs []InstallSpec + runner := agentHookRunner switch agent { case "codex": if path == "" { @@ -93,15 +153,31 @@ func RunDump(opts DumpOptions, stdout io.Writer) error { path = DefaultClaudeSettingsPath() } specs = claudeSpecs(command) + case "droid": + scope, err := normalizeDroidScope(opts.Scope) + if err != nil { + return err + } + if path == "" { + path = DefaultDroidHooksPath(scope) + } + if path == "" { + return fmt.Errorf("could not resolve Factory Droid hooks path for scope %q", scope) + } + if err := validateDroidHooksPath(path); err != nil { + return err + } + specs = droidSpecs(command, opts.Timeout) + runner = droidAgentHookRunner default: - return fmt.Errorf("agent must be codex or claude") + return fmt.Errorf("agent must be codex, claude, or droid") } - root, _, _, err := planSpecs(path, specs) + root, _, _, err := PlanSpecs(path, specs, runner) if err != nil { return err } - body, err := marshalJSONConfig(root) + body, err := MarshalJSONConfig(root) if err != nil { return fmt.Errorf("encode %s: %w", path, err) } @@ -109,9 +185,13 @@ func RunDump(opts DumpOptions, stdout io.Writer) error { return err } -func resolveInstallCommand(command string) (string, error) { +func resolveInstallCommand(agent, command string) (string, error) { command = strings.TrimSpace(command) if command == "" { + if agent == "droid" { + command, _, err := ResolveHookCommandWithRunner("", "", droidAgentHookRunner) + return command, err + } return defaultInstallCommand() } return command, nil @@ -130,9 +210,9 @@ func printInstallResult(stdout io.Writer, name, path string, changed, dryRun boo } } -func codexSpecs(command string, timeout time.Duration) []installSpec { +func codexSpecs(command string, timeout time.Duration) []InstallSpec { secs := int(timeout.Seconds()) - return []installSpec{ + return []InstallSpec{ { Event: "PreToolUse", Matcher: "^Bash$", @@ -156,8 +236,8 @@ func codexSpecs(command string, timeout time.Duration) []installSpec { } } -func claudeSpecs(command string) []installSpec { - return []installSpec{ +func claudeSpecs(command string) []InstallSpec { + return []InstallSpec{ { Event: "PreToolUse", Matcher: "Bash", @@ -175,8 +255,39 @@ func claudeSpecs(command string) []installSpec { } } -func installSpecs(path string, specs []installSpec, dryRun bool) (bool, error) { - root, mode, changed, err := planSpecs(path, specs) +func droidSpecs(command string, timeout time.Duration) []InstallSpec { + secs := int(timeout.Seconds()) + return []InstallSpec{ + { + Event: "PreToolUse", + Matcher: ExecuteMatcher, + Command: command, + Timeout: secs, + IncludeTimeout: true, + }, + { + Event: "PostToolUse", + Matcher: ExecuteMatcher, + Command: command, + Timeout: secs, + IncludeTimeout: true, + }, + { + Event: "Stop", + Command: command, + Timeout: secs, + IncludeTimeout: true, + }, + } +} + +// InstallSpecs writes specs into the hook config at path, collapsing any prior +// roborev command hooks for runner into a single up-to-date entry. runner is the +// subcommand suffix (e.g. "agent-hook run") used to identify stale roborev +// commands from earlier installs. It reports whether the config +// changed. When dryRun is set, it computes the change without writing. +func InstallSpecs(path string, specs []InstallSpec, runner string, dryRun bool) (bool, error) { + root, mode, changed, err := PlanSpecs(path, specs, runner) if err != nil { return false, err } @@ -189,7 +300,11 @@ func installSpecs(path string, specs []installSpec, dryRun bool) (bool, error) { return true, nil } -func planSpecs(path string, specs []installSpec) (map[string]any, os.FileMode, bool, error) { +// PlanSpecs reads the hook config at path and computes the merged config that +// would result from installing specs (identified by runner for stale-command +// detection) without writing. It returns the root object, its file mode, and +// whether the config would change. +func PlanSpecs(path string, specs []InstallSpec, runner string) (map[string]any, os.FileMode, bool, error) { if path == "" { return nil, 0, false, fmt.Errorf("config path is required") } @@ -204,7 +319,7 @@ func planSpecs(path string, specs []installSpec) (map[string]any, os.FileMode, b changed := false for _, spec := range specs { - specChanged, err := ensureSpec(hooks, spec) + specChanged, err := ensureSpec(hooks, spec, runner) if err != nil { return nil, 0, false, fmt.Errorf("%s hook: %w", spec.Event, err) } @@ -238,7 +353,9 @@ func readJSONConfig(path string) (map[string]any, os.FileMode, error) { return root, mode, nil } -func marshalJSONConfig(root map[string]any) ([]byte, error) { +// MarshalJSONConfig encodes a hook config root as indented JSON with a trailing +// newline, suitable for writing to a hooks.json or settings.json file. +func MarshalJSONConfig(root map[string]any) ([]byte, error) { body, err := json.MarshalIndent(root, "", " ") if err != nil { return nil, err @@ -247,7 +364,7 @@ func marshalJSONConfig(root map[string]any) ([]byte, error) { } func writeJSONConfig(path string, root map[string]any, mode os.FileMode) error { - body, err := marshalJSONConfig(root) + body, err := MarshalJSONConfig(root) if err != nil { return fmt.Errorf("encode %s: %w", path, err) } @@ -295,7 +412,7 @@ func configObject(root map[string]any) (map[string]any, error) { return hooks, nil } -func ensureSpec(hooks map[string]any, spec installSpec) (bool, error) { +func ensureSpec(hooks map[string]any, spec InstallSpec, runner string) (bool, error) { entries, err := eventEntries(hooks, spec.Event) if err != nil { return false, err @@ -329,7 +446,7 @@ func ensureSpec(hooks map[string]any, spec installSpec) (bool, error) { if err != nil { return false, err } - updated, changed := upsertCommandHook(entryHookList, commandHook, spec) + updated, changed := upsertCommandHook(entryHookList, commandHook, spec, runner) entry["hooks"] = updated hooks[spec.Event] = entries return changed, nil @@ -350,17 +467,17 @@ func eventEntries(hooks map[string]any, event string) ([]any, error) { } // upsertCommandHook installs commandHook into list, collapsing any prior roborev -// agent-hook command hooks - including ones carrying a stale binary path from an +// command hooks for runner - including ones carrying a stale binary path from an // earlier install - into this single hook rather than appending a duplicate // beside them. Non-roborev hooks are left untouched. It reports whether the list // changed. -func upsertCommandHook(list []any, commandHook map[string]any, spec installSpec) ([]any, bool) { +func upsertCommandHook(list []any, commandHook map[string]any, spec InstallSpec, runner string) ([]any, bool) { updated := make([]any, 0, len(list)+1) placed := false changed := false for _, raw := range list { hook, ok := raw.(map[string]any) - if !ok || !replaceableCommandHook(hook, spec) { + if !ok || !replaceableCommandHook(hook, spec, runner) { updated = append(updated, raw) continue } @@ -385,19 +502,19 @@ func upsertCommandHook(list []any, commandHook map[string]any, spec installSpec) // replaceableCommandHook reports whether an existing command hook should be // replaced by the spec's command: either it already uses the exact command (an -// idempotent re-install) or it is a roborev agent-hook command that may carry a +// idempotent re-install) or it is a roborev command for runner that may carry a // stale binary path from a prior install. -func replaceableCommandHook(hook map[string]any, spec installSpec) bool { +func replaceableCommandHook(hook map[string]any, spec InstallSpec, runner string) bool { if hook["type"] != "command" { return false } cmd, _ := hook["command"].(string) - return cmd == spec.Command || isRoborevAgentHookCommand(cmd) + return cmd == spec.Command || isRoborevHookCommand(cmd, runner) } // commandHookCurrent reports whether hook already matches spec exactly, so it // needs no rewrite. -func commandHookCurrent(hook map[string]any, spec installSpec) bool { +func commandHookCurrent(hook map[string]any, spec InstallSpec) bool { if cmd, _ := hook["command"].(string); cmd != spec.Command { return false } @@ -408,11 +525,66 @@ func commandHookCurrent(hook map[string]any, spec installSpec) bool { return ok && int(curr) == spec.Timeout } -// isRoborevAgentHookCommand reports whether a hook command invokes roborev's -// agent-hook runner, regardless of binary path or quoting, so an install can -// replace command hooks that carry a stale or versioned roborev path. -func isRoborevAgentHookCommand(command string) bool { - return strings.Contains(command, "agent-hook run") && strings.Contains(command, "roborev") +// isRoborevHookCommand reports whether a hook command invokes the roborev +// runner (e.g. "agent-hook run"), regardless of binary path or quoting, so an +// install can replace command hooks that carry a stale or versioned roborev +// path. runner is the subcommand suffix to match. +func isRoborevHookCommand(command, runner string) bool { + if !strings.Contains(command, "roborev") { + return false + } + + baseRunner := runner + if runner == droidAgentHookRunner { + baseRunner = agentHookRunner + } + + suffix, ok := hookCommandRunnerSuffix(command, baseRunner) + if !ok { + return false + } + + switch runner { + case agentHookRunner: + return !selectsDroidAgent(suffix) + case droidAgentHookRunner: + return selectsDroidAgent(suffix) + default: + return true + } +} + +func hookCommandRunnerSuffix(command, runner string) (string, bool) { + idx := strings.Index(command, runner) + if idx == -1 { + return "", false + } + after := idx + len(runner) + if after == len(command) { + return "", true + } + next := command[after : after+1] + if strings.ContainsAny(next, "\"'") { + return "", true + } + if !strings.ContainsAny(next, " \t\r\n") { + return "", false + } + return strings.TrimSpace(command[after:]), true +} + +func selectsDroidAgent(suffix string) bool { + fields := shellFields(suffix) + for i, field := range fields { + field = cleanShellToken(field) + if field == "--agent=droid" { + return true + } + if field == "--agent" && i+1 < len(fields) && cleanShellToken(fields[i+1]) == "droid" { + return true + } + } + return false } func findEntry(entries []any, matcher string) (int, error) { @@ -449,21 +621,32 @@ func entryHooks(entry map[string]any) ([]any, error) { // a stable shim over a versioned or temporary install path - and returns any // advisory notice from that resolution so callers can surface it. A non-empty // override is used verbatim with no notice, letting callers pin an exact command. +// The notice is translated for command-only flows (dump), which expose +// --command rather than --binary. func ResolveHookCommand(override string) (command, notice string, err error) { - if override = strings.TrimSpace(override); override != "" { - return override, "", nil - } - command, notice, err = resolveHookCommandFromBinary("") + command, notice, err = ResolveHookCommandWithRunner(override, "", agentHookRunner) if err != nil { return "", "", err } - return command, agentHookNotice(notice), nil + return command, TranslateBinaryNotice(notice), nil } // ResolveHookCommandWithBinary returns the command to install for agent hooks. // commandOverride is used verbatim when set. binaryOverride is resolved and -// quoted before appending the agent-hook runner subcommand. +// quoted before appending the agent-hook runner subcommand. The returned notice +// is raw (mentions --binary), since the install flow exposes --binary. func ResolveHookCommandWithBinary(commandOverride, binaryOverride string) (command, notice string, err error) { + return ResolveHookCommandWithRunner(commandOverride, binaryOverride, agentHookRunner) +} + +// ResolveHookCommandWithRunner returns the command to install for a roborev hook +// integration identified by runner (e.g. "agent-hook run"). +// commandOverride is used verbatim when set; otherwise binaryOverride (or an +// auto-resolved roborev binary) is quoted and suffixed with runner. The returned +// notice is raw (mentions --binary); callers that expose --command instead +// should pass it through TranslateBinaryNotice. commandOverride and +// binaryOverride are mutually exclusive. +func ResolveHookCommandWithRunner(commandOverride, binaryOverride, runner string) (command, notice string, err error) { commandOverride = strings.TrimSpace(commandOverride) binaryOverride = strings.TrimSpace(binaryOverride) if commandOverride != "" && binaryOverride != "" { @@ -472,22 +655,22 @@ func ResolveHookCommandWithBinary(commandOverride, binaryOverride string) (comma if commandOverride != "" { return commandOverride, "", nil } - return resolveHookCommandFromBinary(binaryOverride) + return resolveHookCommandFromBinary(binaryOverride, runner) } -func resolveHookCommandFromBinary(binaryOverride string) (command, notice string, err error) { +func resolveHookCommandFromBinary(binaryOverride, runner string) (command, notice string, err error) { res, err := githook.ResolveRoborevPath(binaryOverride) if err != nil { return "", "", fmt.Errorf("resolve roborev binary: %w", err) } - return shellQuote(res.Path) + " agent-hook run", res.Notice, nil + return shellQuote(res.Path) + " " + runner, res.Notice, nil } -// agentHookNotice adapts a binary-resolution notice for command-only agent-hook -// commands. The shared resolver phrases its stable-binary guidance for --binary; +// TranslateBinaryNotice adapts a binary-resolution notice for command-only hook +// flows. The shared resolver phrases its stable-binary guidance for --binary; // dump exposes --command instead, so the flag name is translated to avoid // pointing users at a flag that command does not have. -func agentHookNotice(notice string) string { +func TranslateBinaryNotice(notice string) string { return strings.ReplaceAll(notice, "--binary", "--command") } @@ -515,6 +698,231 @@ func DefaultClaudeSettingsPath() string { return filepath.Join(home, ".claude", "settings.json") } +// DefaultDroidHooksPath returns the user-scoped Factory Droid hooks.json path. +// Unsupported scopes return an empty path. HOME is checked first so tests and +// POSIX-style environments work on Windows, where os.UserHomeDir returns +// USERPROFILE instead. +func DefaultDroidHooksPath(scope string) string { + scope = strings.ToLower(strings.TrimSpace(scope)) + if scope != "" && scope != "user" { + return "" + } + home := os.Getenv("HOME") + if home == "" { + var err error + home, err = os.UserHomeDir() + if err != nil || home == "" { + return "" + } + } + return filepath.Join(home, ".factory", "hooks.json") +} + +func validateDroidHooksPath(path string) error { + if isUserDroidHooksPath(path) { + if resolved, ok := evalExistingParentPath(path); ok && !isUserDroidHooksPath(resolved) && isProjectDroidHooksPath(resolved) { + return fmt.Errorf("project-scoped Factory Droid hook config is not supported; use the user-scoped Factory hooks path instead") + } + return nil + } + if isProjectDroidHooksPath(path) { + return fmt.Errorf("project-scoped Factory Droid hook config is not supported; use the user-scoped Factory hooks path instead") + } + if resolved, ok := evalExistingParentPath(path); ok && isProjectDroidHooksPath(resolved) { + return fmt.Errorf("project-scoped Factory Droid hook config is not supported; use the user-scoped Factory hooks path instead") + } + return nil +} + +func isUserDroidHooksPath(path string) bool { + return sameCleanAbsPath(path, DefaultDroidHooksPath("user")) +} + +func isProjectDroidHooksPath(path string) bool { + path = strings.TrimSpace(path) + if path == "" { + return false + } + clean := filepath.Clean(path) + projectRel := filepath.Join(".factory", "hooks.json") + if sameDroidPath(clean, projectRel) { + return true + } + if isTargetRepoDroidHooksPath(clean) { + return true + } + if repoRoot, err := gitpkg.GetRepoRoot("."); err == nil && repoRoot != "" && + sameCleanAbsPath(clean, filepath.Join(repoRoot, projectRel)) { + return true + } + if !filepath.IsAbs(clean) { + return false + } + wd, err := os.Getwd() + if err != nil || wd == "" { + return false + } + projectAbs, err := filepath.Abs(filepath.Join(wd, projectRel)) + if err != nil { + return false + } + return sameCleanAbsPath(clean, projectAbs) +} + +func isTargetRepoDroidHooksPath(path string) bool { + abs, ok := cleanAbsPath(path) + if !ok || !sameDroidPathName(filepath.Base(abs), "hooks.json") { + return false + } + factoryDir := filepath.Dir(abs) + if !sameDroidPathName(filepath.Base(factoryDir), ".factory") { + return false + } + candidateRoot := filepath.Dir(factoryDir) + repoRoot, err := gitpkg.GetRepoRoot(candidateRoot) + if err != nil || repoRoot == "" { + return false + } + return sameCleanAbsPath(candidateRoot, repoRoot) +} + +func sameCleanAbsPath(a, b string) bool { + a = strings.TrimSpace(a) + b = strings.TrimSpace(b) + if a == "" || b == "" { + return false + } + aAbs, okA := cleanAbsPath(a) + bAbs, okB := cleanAbsPath(b) + if okA && okB { + return sameDroidPath(filepath.Clean(aAbs), filepath.Clean(bAbs)) + } + return sameDroidPath(filepath.Clean(a), filepath.Clean(b)) +} + +func cleanAbsPath(path string) (string, bool) { + path = strings.TrimSpace(path) + if path == "" { + return "", false + } + abs, err := filepath.Abs(filepath.Clean(path)) + if err != nil { + return "", false + } + return canonicalExistingPath(filepath.Clean(abs)), true +} + +func canonicalExistingPath(path string) string { + path = filepath.Clean(path) + volume := filepath.VolumeName(path) + rest := strings.TrimPrefix(path, volume) + rooted := strings.HasPrefix(rest, string(filepath.Separator)) + rest = strings.TrimPrefix(rest, string(filepath.Separator)) + parts := strings.Split(rest, string(filepath.Separator)) + + current := volume + if rooted { + current += string(filepath.Separator) + } + for i, part := range parts { + if part == "" { + continue + } + next := filepath.Join(current, part) + if _, err := os.Lstat(next); err != nil { + for _, remaining := range parts[i:] { + if remaining != "" { + current = filepath.Join(current, remaining) + } + } + return filepath.Clean(current) + } + if actual := actualDirEntryName(current, part); actual != "" { + part = actual + } + current = filepath.Join(current, part) + } + return filepath.Clean(current) +} + +func actualDirEntryName(parent, name string) string { + if parent == "" { + parent = "." + } + entries, err := os.ReadDir(parent) + if err != nil { + return "" + } + for _, entry := range entries { + if entry.Name() == name { + return name + } + } + for _, entry := range entries { + if strings.EqualFold(entry.Name(), name) { + return entry.Name() + } + } + return "" +} + +func sameDroidPath(a, b string) bool { + if droidPathCaseInsensitive { + return strings.EqualFold(a, b) + } + return a == b +} + +func sameDroidPathName(a, b string) bool { + if droidPathCaseInsensitive { + return strings.EqualFold(a, b) + } + return a == b +} + +// evalExistingParentPath resolves the longest existing ancestor of path and +// returns the full path with symlinks evaluated on the existing portion. This +// catches symlinked parent directories even when the final path components do +// not exist yet (e.g., hooks.json has not been created). +func evalExistingParentPath(path string) (string, bool) { + path = strings.TrimSpace(path) + if path == "" { + return "", false + } + clean := filepath.Clean(path) + existing := clean + remaining := "" + for existing != "." && existing != string(filepath.Separator) { + if _, err := os.Lstat(existing); err == nil { + break + } + remaining = filepath.Join(filepath.Base(existing), remaining) + existing = filepath.Dir(existing) + } + if existing == "." || existing == string(filepath.Separator) { + return cleanAbsPath(clean) + } + resolved, err := filepath.EvalSymlinks(existing) + if err != nil { + return cleanAbsPath(clean) + } + return cleanAbsPath(filepath.Join(resolved, remaining)) +} + +func normalizeDroidScope(scope string) (string, error) { + scope = strings.ToLower(strings.TrimSpace(scope)) + if scope == "" { + return "user", nil + } + if scope == "user" { + return scope, nil + } + if scope == "project" { + return "", fmt.Errorf("project scope is not supported for Factory Droid agent hooks; use user scope because project hooks are executable repo-local configuration") + } + return "", fmt.Errorf("scope must be user") +} + func shellQuote(s string) string { if s == "" { return "''" diff --git a/internal/agenthook/install_test.go b/internal/agenthook/install_test.go index 0d424aa47..9eb601384 100644 --- a/internal/agenthook/install_test.go +++ b/internal/agenthook/install_test.go @@ -11,6 +11,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "go.kenn.io/roborev/internal/testutil" ) func TestRunDumpCodexCreatesHookConfig(t *testing.T) { @@ -34,6 +36,31 @@ func TestRunDumpCodexCreatesHookConfig(t *testing.T) { assert.InDelta(t, 10, firstCommandTimeout(t, root, "Stop", command), 0) } +func TestRunDumpDroidCreatesHookConfig(t *testing.T) { + path := filepath.Join(t.TempDir(), "hooks.json") + command := "/tmp/roborev agent-hook run --agent droid" + + var stdout bytes.Buffer + err := RunDump(DumpOptions{ + Agent: "droid", + Command: command, + ConfigPath: path, + Scope: "user", + Timeout: 10 * time.Second, + }, &stdout) + + require.NoError(t, err) + var root map[string]any + require.NoError(t, json.Unmarshal(stdout.Bytes(), &root)) + assertCommandCount(t, root, "PreToolUse", command, 1) + assertCommandCount(t, root, "PostToolUse", command, 1) + assertCommandCount(t, root, "Stop", command, 1) + assert.Equal(t, ExecuteMatcher, firstMatcher(t, root, "PreToolUse")) + assert.Equal(t, ExecuteMatcher, firstMatcher(t, root, "PostToolUse")) + assert.Empty(t, firstMatcher(t, root, "Stop")) + assert.InDelta(t, 10, firstCommandTimeout(t, root, "Stop", command), 0) +} + func TestRunInstallCodexIsIdempotent(t *testing.T) { path := filepath.Join(t.TempDir(), "hooks.json") command := "/tmp/roborev agent-hook run" @@ -59,6 +86,36 @@ func TestRunInstallCodexIsIdempotent(t *testing.T) { assert.Contains(t, second.String(), "Codex agent hooks already installed") } +func TestRunInstallDroidIsIdempotent(t *testing.T) { + path := filepath.Join(t.TempDir(), "hooks.json") + command := "/tmp/roborev agent-hook run --agent droid" + + var first bytes.Buffer + err := RunInstall(InstallOptions{ + Agent: "droid", + Command: command, + ConfigPath: path, + Scope: "user", + Timeout: 10 * time.Second, + }, &first) + require.NoError(t, err) + assert.Contains(t, first.String(), "installed Factory Droid agent hooks") + + var second bytes.Buffer + err = RunInstall(InstallOptions{ + Agent: "droid", + Command: command, + ConfigPath: path, + Scope: "user", + Timeout: 10 * time.Second, + }, &second) + require.NoError(t, err) + assert.Contains(t, second.String(), "Factory Droid agent hooks already installed") + + root := readJSONFile(t, path) + assertCommandCount(t, root, "Stop", command, 1) +} + func TestRunInstallMigratesStaleRoborevHookCommand(t *testing.T) { assert := assert.New(t) path := filepath.Join(t.TempDir(), "hooks.json") @@ -97,9 +154,487 @@ func TestRunInstallMigratesStaleRoborevHookCommand(t *testing.T) { assert.Contains(out.String(), "installed Codex agent hooks", "migrating a stale command counts as a change") } +func TestRunInstallDroidLeavesPlainAgentHookEntriesUntouched(t *testing.T) { + assert := assert.New(t) + path := filepath.Join(t.TempDir(), "hooks.json") + agentCommand := "/stable/bin/roborev agent-hook run" + droidCommand := "/stable/bin/roborev agent-hook run --agent droid" + + writeJSONFile(t, path, map[string]any{ + "hooks": map[string]any{ + "Stop": []any{map[string]any{ + "hooks": []any{commandHookJSON(agentCommand, 10)}, + }}, + }, + }) + + var out bytes.Buffer + err := RunInstall(InstallOptions{ + Agent: "droid", + Command: droidCommand, + ConfigPath: path, + Scope: "user", + Timeout: 10 * time.Second, + }, &out) + require.NoError(t, err) + + root := readJSONFile(t, path) + assertCommandCount(t, root, "Stop", agentCommand, 1) + assertCommandCount(t, root, "Stop", droidCommand, 1) + assert.Contains(out.String(), "installed Factory Droid agent hooks") +} + +func TestRunInstallCodexLeavesDroidAgentHookEntriesUntouched(t *testing.T) { + assert := assert.New(t) + path := filepath.Join(t.TempDir(), "hooks.json") + droidCommand := "/stable/bin/roborev agent-hook run --agent droid" + codexCommand := "/stable/bin/roborev agent-hook run" + + writeJSONFile(t, path, map[string]any{ + "hooks": map[string]any{ + "Stop": []any{map[string]any{ + "hooks": []any{commandHookJSON(droidCommand, 10)}, + }}, + }, + }) + + var out bytes.Buffer + err := RunInstall(InstallOptions{ + Agent: "codex", + Command: codexCommand, + CodexConfigPath: path, + Timeout: 10 * time.Second, + }, &out) + require.NoError(t, err) + + root := readJSONFile(t, path) + assertCommandCount(t, root, "Stop", droidCommand, 1) + assertCommandCount(t, root, "Stop", codexCommand, 1) + assert.Contains(out.String(), "installed Codex agent hooks") +} + +func TestRunInstallCodexMigratesStaleHookCommandWithRunFlags(t *testing.T) { + assert := assert.New(t) + path := filepath.Join(t.TempDir(), "hooks.json") + oldCommand := "/old/versioned/1.2.3/bin/roborev agent-hook run --turn-threshold 3" + newCommand := "/stable/bin/roborev agent-hook run" + + writeJSONFile(t, path, map[string]any{ + "hooks": map[string]any{ + "Stop": []any{map[string]any{ + "hooks": []any{commandHookJSON(oldCommand, 10)}, + }}, + }, + }) + + var out bytes.Buffer + err := RunInstall(InstallOptions{ + Agent: "codex", + Command: newCommand, + CodexConfigPath: path, + Timeout: 10 * time.Second, + }, &out) + require.NoError(t, err) + + root := readJSONFile(t, path) + assertCommandCount(t, root, "Stop", newCommand, 1) + assertCommandCount(t, root, "Stop", oldCommand, 0) + assert.Contains(out.String(), "installed Codex agent hooks") +} + +func TestRunInstallDroidMigratesStaleHookCommandWithRunFlags(t *testing.T) { + cases := []struct { + name string + oldCommand string + }{ + { + name: "agent flag before config", + oldCommand: "/old/versioned/1.2.3/bin/roborev agent-hook run --agent droid --config /tmp/roborev.toml", + }, + { + name: "agent flag after config", + oldCommand: "/old/versioned/1.2.3/bin/roborev agent-hook run --config /tmp/roborev.toml --agent droid", + }, + { + name: "agent equals form", + oldCommand: "/old/versioned/1.2.3/bin/roborev agent-hook run --agent=droid", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert := assert.New(t) + path := filepath.Join(t.TempDir(), "hooks.json") + newCommand := "/stable/bin/roborev agent-hook run --agent droid" + + writeJSONFile(t, path, map[string]any{ + "hooks": map[string]any{ + "Stop": []any{map[string]any{ + "hooks": []any{commandHookJSON(tc.oldCommand, 10)}, + }}, + }, + }) + + var out bytes.Buffer + err := RunInstall(InstallOptions{ + Agent: "droid", + Command: newCommand, + ConfigPath: path, + Scope: "user", + Timeout: 10 * time.Second, + }, &out) + require.NoError(t, err) + + root := readJSONFile(t, path) + assertCommandCount(t, root, "Stop", newCommand, 1) + assertCommandCount(t, root, "Stop", tc.oldCommand, 0) + assert.Contains(out.String(), "installed Factory Droid agent hooks") + }) + } +} + +func TestRunInstallDroidRejectsUnknownScope(t *testing.T) { + var out bytes.Buffer + err := RunInstall(InstallOptions{ + Agent: "droid", + Command: "/tmp/roborev agent-hook run --agent droid", + Scope: "team", + Timeout: 10 * time.Second, + }, &out) + require.Error(t, err) + assert.Contains(t, err.Error(), "scope must be user") +} + +func TestRunInstallDroidRejectsProjectScope(t *testing.T) { + var out bytes.Buffer + err := RunInstall(InstallOptions{ + Agent: "droid", + Command: "/tmp/roborev agent-hook run --agent droid", + Scope: "project", + Timeout: 10 * time.Second, + }, &out) + require.Error(t, err) + assert.ErrorContains(t, err, "project scope is not supported for Factory Droid agent hooks") +} + +func TestRunDumpDroidRejectsProjectScope(t *testing.T) { + var out bytes.Buffer + err := RunDump(DumpOptions{ + Agent: "droid", + Command: "/tmp/roborev agent-hook run --agent droid", + Scope: "project", + Timeout: 10 * time.Second, + }, &out) + require.Error(t, err) + assert.ErrorContains(t, err, "project scope is not supported for Factory Droid agent hooks") +} + +func TestRunInstallDroidRejectsProjectConfigPath(t *testing.T) { + var out bytes.Buffer + err := RunInstall(InstallOptions{ + Agent: "droid", + Command: "/tmp/roborev agent-hook run --agent droid", + ConfigPath: filepath.Join(".factory", "hooks.json"), + Scope: "user", + Timeout: 10 * time.Second, + }, &out) + require.Error(t, err) + assert.ErrorContains(t, err, "project-scoped Factory Droid hook config is not supported") +} + +func TestRunDumpDroidRejectsProjectConfigPath(t *testing.T) { + var out bytes.Buffer + err := RunDump(DumpOptions{ + Agent: "droid", + Command: "/tmp/roborev agent-hook run --agent droid", + ConfigPath: filepath.Join(".factory", "hooks.json"), + Scope: "user", + Timeout: 10 * time.Second, + }, &out) + require.Error(t, err) + assert.ErrorContains(t, err, "project-scoped Factory Droid hook config is not supported") +} + +func TestRunInstallDroidRejectsMixedCaseProjectConfigPathsWhenCaseInsensitive(t *testing.T) { + stubDroidPathCaseInsensitive(t, true) + repo := testutil.NewGitRepo(t) + chdirForTest(t, repo.Path()) + + for _, configPath := range []string{ + filepath.Join(".Factory", "hooks.JSON"), + filepath.Join(repo.Path(), ".Factory", "hooks.JSON"), + } { + t.Run(configPath, func(t *testing.T) { + var out bytes.Buffer + err := RunInstall(InstallOptions{ + Agent: "droid", + Command: "/tmp/roborev agent-hook run --agent droid", + ConfigPath: configPath, + Scope: "user", + Timeout: 10 * time.Second, + DryRun: true, + }, &out) + require.Error(t, err) + assert.ErrorContains(t, err, "project-scoped Factory Droid hook config is not supported") + }) + } +} + +func TestRunDumpDroidRejectsMixedCaseProjectConfigPathWhenCaseInsensitive(t *testing.T) { + stubDroidPathCaseInsensitive(t, true) + repo := testutil.NewGitRepo(t) + chdirForTest(t, repo.Path()) + + var out bytes.Buffer + err := RunDump(DumpOptions{ + Agent: "droid", + Command: "/tmp/roborev agent-hook run --agent droid", + ConfigPath: filepath.Join(".Factory", "hooks.JSON"), + Scope: "user", + Timeout: 10 * time.Second, + }, &out) + require.Error(t, err) + assert.ErrorContains(t, err, "project-scoped Factory Droid hook config is not supported") +} + +func TestRunInstallDroidRejectsMixedCaseExistingProjectConfigOnCaseInsensitiveFS(t *testing.T) { + repo := testutil.NewGitRepo(t) + chdirForTest(t, repo.Path()) + requireFilesystemCaseInsensitive(t, repo.Path()) + stubDroidPathCaseInsensitive(t, false) + + require.NoError(t, os.MkdirAll(filepath.Join(repo.Path(), ".factory"), 0o755)) + writeJSONFile(t, filepath.Join(repo.Path(), ".factory", "hooks.json"), map[string]any{}) + + var out bytes.Buffer + err := RunInstall(InstallOptions{ + Agent: "droid", + Command: "/tmp/roborev agent-hook run --agent droid", + ConfigPath: filepath.Join(repo.Path(), ".Factory", "hooks.JSON"), + Scope: "user", + Timeout: 10 * time.Second, + DryRun: true, + }, &out) + require.Error(t, err) + assert.ErrorContains(t, err, "project-scoped Factory Droid hook config is not supported") +} + +func TestRunInstallDroidRejectsSymlinkToProjectConfigPath(t *testing.T) { + repo := testutil.NewGitRepo(t) + targetDir := filepath.Join(repo.Path(), ".factory") + require.NoError(t, os.MkdirAll(targetDir, 0o755)) + targetPath := filepath.Join(targetDir, "hooks.json") + writeJSONFile(t, targetPath, map[string]any{}) + + linkPath := filepath.Join(t.TempDir(), "hooks.json") + err := os.Symlink(targetPath, linkPath) + if err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + var out bytes.Buffer + err = RunInstall(InstallOptions{ + Agent: "droid", + Command: "/tmp/roborev agent-hook run --agent droid", + ConfigPath: linkPath, + Scope: "user", + Timeout: 10 * time.Second, + }, &out) + require.Error(t, err) + assert.ErrorContains(t, err, "project-scoped Factory Droid hook config is not supported") +} + +func TestRunInstallDroidAllowsUserScopeWhenHomeIsCWD(t *testing.T) { + wd, err := os.Getwd() + require.NoError(t, err) + t.Setenv("HOME", wd) + + var out bytes.Buffer + err = RunInstall(InstallOptions{ + Agent: "droid", + Command: "/tmp/roborev agent-hook run --agent droid", + ConfigPath: filepath.Join(wd, ".factory", "hooks.json"), + Scope: "user", + Timeout: 10 * time.Second, + DryRun: true, + }, &out) + require.NoError(t, err) + assert.Contains(t, out.String(), "would update Factory Droid agent hooks") +} + +func TestRunDumpDroidAllowsUserScopeWhenHomeIsCWD(t *testing.T) { + wd, err := os.Getwd() + require.NoError(t, err) + t.Setenv("HOME", wd) + + var out bytes.Buffer + err = RunDump(DumpOptions{ + Agent: "droid", + Command: "/tmp/roborev agent-hook run --agent droid", + ConfigPath: filepath.Join(wd, ".factory", "hooks.json"), + Scope: "user", + Timeout: 10 * time.Second, + }, &out) + require.NoError(t, err) + assert.Contains(t, out.String(), "agent-hook run --agent droid") +} + +func TestRunInstallDroidRejectsSymlinkedParentToRepoFactory(t *testing.T) { + repo := testutil.NewGitRepo(t) + require.NoError(t, os.MkdirAll(filepath.Join(repo.Path(), ".factory"), 0o755)) + + home := t.TempDir() + t.Setenv("HOME", home) + err := os.Symlink(filepath.Join(repo.Path(), ".factory"), filepath.Join(home, ".factory")) + if err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + var out bytes.Buffer + err = RunInstall(InstallOptions{ + Agent: "droid", + Command: "/tmp/roborev agent-hook run --agent droid", + Scope: "user", + Timeout: 10 * time.Second, + DryRun: true, + }, &out) + require.Error(t, err) + assert.ErrorContains(t, err, "project-scoped Factory Droid hook config is not supported") +} + +func TestRunDumpDroidRejectsSymlinkedParentToRepoFactory(t *testing.T) { + repo := testutil.NewGitRepo(t) + require.NoError(t, os.MkdirAll(filepath.Join(repo.Path(), ".factory"), 0o755)) + + home := t.TempDir() + t.Setenv("HOME", home) + err := os.Symlink(filepath.Join(repo.Path(), ".factory"), filepath.Join(home, ".factory")) + if err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + + var out bytes.Buffer + err = RunDump(DumpOptions{ + Agent: "droid", + Command: "/tmp/roborev agent-hook run --agent droid", + Scope: "user", + Timeout: 10 * time.Second, + }, &out) + require.Error(t, err) + assert.ErrorContains(t, err, "project-scoped Factory Droid hook config is not supported") +} + +func TestRunInstallDroidRejectsRepoRootProjectConfigFromSubdir(t *testing.T) { + repo := testutil.NewGitRepo(t) + subdir := filepath.Join(repo.Path(), "subdir") + require.NoError(t, os.MkdirAll(subdir, 0o755)) + chdirForTest(t, subdir) + + for _, configPath := range []string{ + filepath.Join("..", ".factory", "hooks.json"), + filepath.Join(repo.Path(), ".factory", "hooks.json"), + } { + t.Run(configPath, func(t *testing.T) { + var out bytes.Buffer + err := RunInstall(InstallOptions{ + Agent: "droid", + Command: "/tmp/roborev agent-hook run --agent droid", + ConfigPath: configPath, + Scope: "user", + Timeout: 10 * time.Second, + DryRun: true, + }, &out) + require.Error(t, err) + assert.ErrorContains(t, err, "project-scoped Factory Droid hook config is not supported") + }) + } +} + +func TestRunDumpDroidRejectsRepoRootProjectConfigFromSubdir(t *testing.T) { + repo := testutil.NewGitRepo(t) + subdir := filepath.Join(repo.Path(), "subdir") + require.NoError(t, os.MkdirAll(subdir, 0o755)) + chdirForTest(t, subdir) + + for _, configPath := range []string{ + filepath.Join("..", ".factory", "hooks.json"), + filepath.Join(repo.Path(), ".factory", "hooks.json"), + } { + t.Run(configPath, func(t *testing.T) { + var out bytes.Buffer + err := RunDump(DumpOptions{ + Agent: "droid", + Command: "/tmp/roborev agent-hook run --agent droid", + ConfigPath: configPath, + Scope: "user", + Timeout: 10 * time.Second, + }, &out) + require.Error(t, err) + assert.ErrorContains(t, err, "project-scoped Factory Droid hook config is not supported") + }) + } +} + +func TestRunInstallDroidRejectsTargetRepoProjectConfigFromOutsideRepo(t *testing.T) { + repo := testutil.NewGitRepo(t) + parent := filepath.Dir(repo.Path()) + chdirForTest(t, parent) + + for _, configPath := range []string{ + filepath.Join(filepath.Base(repo.Path()), ".factory", "hooks.json"), + filepath.Join(repo.Path(), ".factory", "hooks.json"), + } { + t.Run(configPath, func(t *testing.T) { + var out bytes.Buffer + err := RunInstall(InstallOptions{ + Agent: "droid", + Command: "/tmp/roborev agent-hook run --agent droid", + ConfigPath: configPath, + Scope: "user", + Timeout: 10 * time.Second, + DryRun: true, + }, &out) + require.Error(t, err) + assert.ErrorContains(t, err, "project-scoped Factory Droid hook config is not supported") + }) + } +} + +func TestRunDumpDroidRejectsTargetRepoProjectConfigFromOutsideRepo(t *testing.T) { + repo := testutil.NewGitRepo(t) + parent := filepath.Dir(repo.Path()) + chdirForTest(t, parent) + + for _, configPath := range []string{ + filepath.Join(filepath.Base(repo.Path()), ".factory", "hooks.json"), + filepath.Join(repo.Path(), ".factory", "hooks.json"), + } { + t.Run(configPath, func(t *testing.T) { + var out bytes.Buffer + err := RunDump(DumpOptions{ + Agent: "droid", + Command: "/tmp/roborev agent-hook run --agent droid", + ConfigPath: configPath, + Scope: "user", + Timeout: 10 * time.Second, + }, &out) + require.Error(t, err) + assert.ErrorContains(t, err, "project-scoped Factory Droid hook config is not supported") + }) + } +} + +func TestDefaultDroidHooksPath(t *testing.T) { + dir := t.TempDir() + t.Setenv("HOME", dir) + + assert.Equal(t, filepath.Join(dir, ".factory", "hooks.json"), DefaultDroidHooksPath("user")) + assert.Equal(t, filepath.Join(dir, ".factory", "hooks.json"), DefaultDroidHooksPath("")) + assert.Empty(t, DefaultDroidHooksPath("project")) +} + func TestUpsertCommandHookCollapsesDuplicatesAndKeepsOthers(t *testing.T) { assert := assert.New(t) - spec := installSpec{ + spec := InstallSpec{ Event: "PostToolUse", Matcher: "^Bash$", Command: "/new/roborev agent-hook run", Timeout: 10, IncludeTimeout: true, } @@ -110,7 +645,7 @@ func TestUpsertCommandHookCollapsesDuplicatesAndKeepsOthers(t *testing.T) { commandHookJSON("/old/b/roborev agent-hook run", 10), } - updated, changed := upsertCommandHook(list, commandHook, spec) + updated, changed := upsertCommandHook(list, commandHook, spec, agentHookRunner) assert.True(changed) // Both stale roborev hooks collapse into one new command at the first one's @@ -127,10 +662,10 @@ func TestAgentHookNoticeTranslatesBinaryFlagForCommandOnlyFlows(t *testing.T) { notice := "Warning: roborev appears to be running from a versioned mise install (/x); " + "use --binary to install hooks with a stable shim if available" - got := agentHookNotice(notice) + got := TranslateBinaryNotice(notice) assert.NotContains(got, "--binary", "command-only flows do not expose --binary") assert.Contains(got, "--command", "the override flag is translated to --command") - assert.Empty(agentHookNotice(""), "an empty notice stays empty") + assert.Empty(TranslateBinaryNotice(""), "an empty notice stays empty") } func TestResolveHookCommandOverrideIsVerbatim(t *testing.T) { @@ -225,6 +760,37 @@ func eventEntriesForTest(t *testing.T, root map[string]any, event string) []any return entries } +func chdirForTest(t *testing.T, dir string) { + t.Helper() + oldWD, err := os.Getwd() + require.NoError(t, err) + require.NoError(t, os.Chdir(dir)) + t.Cleanup(func() { + require.NoError(t, os.Chdir(oldWD)) + }) +} + +func stubDroidPathCaseInsensitive(t *testing.T, enabled bool) { + t.Helper() + old := droidPathCaseInsensitive + droidPathCaseInsensitive = enabled + t.Cleanup(func() { + droidPathCaseInsensitive = old + }) +} + +func requireFilesystemCaseInsensitive(t *testing.T, dir string) { + t.Helper() + probe := filepath.Join(dir, "case-probe") + require.NoError(t, os.WriteFile(probe, []byte("probe"), 0o600)) + t.Cleanup(func() { + require.NoError(t, os.Remove(probe)) + }) + if _, err := os.Stat(filepath.Join(dir, "CASE-PROBE")); err != nil { + t.Skip("filesystem is case-sensitive") + } +} + func commandHookJSON(command string, timeout int) map[string]any { return map[string]any{"type": "command", "command": command, "timeout": float64(timeout)} } diff --git a/internal/agenthook/state.go b/internal/agenthook/state.go index b66b857ee..fd7517997 100644 --- a/internal/agenthook/state.go +++ b/internal/agenthook/state.go @@ -207,7 +207,7 @@ func (s *StateStore) recordStop(req Request) (Response, error) { } func (s *StateStore) recordPreToolUse(req Request) (Response, error) { - if req.Event.ToolName != "" && req.Event.ToolName != "Bash" { + if !isShellCommandTool(req.Event.ToolName) { return Response{ SessionID: req.Event.SessionID, CommitThreshold: req.CommitThreshold, @@ -258,7 +258,7 @@ func (s *StateStore) recordPreToolUse(req Request) (Response, error) { } func (s *StateStore) recordPostToolUse(req Request) (Response, error) { - if req.Event.ToolName != "" && req.Event.ToolName != "Bash" { + if !isShellCommandTool(req.Event.ToolName) { return Response{ SessionID: req.Event.SessionID, CommitThreshold: req.CommitThreshold, @@ -430,6 +430,10 @@ func thresholdReady(countSincePrompt, threshold int) bool { return threshold > 0 && countSincePrompt >= threshold } +func isShellCommandTool(toolName string) bool { + return toolName == "" || toolName == "Bash" || toolName == ExecuteMatcher +} + // resetPromptCounters restarts the per-prompt counters after a reminder fires. // StopCountSincePrompt is session-wide, but commit counts are cleared only for // the checkout being prompted so a prompt in one repo or branch cannot discard a diff --git a/internal/agenthook/state_test.go b/internal/agenthook/state_test.go index 6815f6d64..523211a74 100644 --- a/internal/agenthook/state_test.go +++ b/internal/agenthook/state_test.go @@ -386,6 +386,63 @@ func TestRecordPostToolUseFailedReviewPromptUsesNewBranchLineageKey(t *testing.T assert.Equal("failed_reviews", featureResp.TriggeredBy) } +func TestRecordToolUseAcceptsDroidExecuteForCommitTracking(t *testing.T) { + assert := assert.New(t) + repo := testutil.NewGitRepo(t) + initial := repo.CommitFile("main.go", "package main\n", "initial") + store := &StateStore{path: filepath.Join(t.TempDir(), "state.json"), sessions: map[string]SessionState{}} + + record := func(eventName, command string) Response { + resp, err := store.Record(Request{ + Event: Input{ + SessionID: "session-1", + CWD: repo.Path(), + HookEventName: eventName, + ToolName: "Execute", + ToolInput: map[string]json.RawMessage{"command": json.RawMessage(`"` + command + `"`)}, + }, + CommitThreshold: 1, + }) + require.NoError(t, err) + return resp + } + + preResp := record("PreToolUse", "git commit -m second") + branchKey := repoHeadKey(repo.Path(), "main") + assert.False(preResp.Skipped) + assert.Equal(initial, store.sessions["session-1"].RepoHeads[branchKey]) + + next := repo.CommitFile("second.go", "package main\n", "second") + postResp := record("PostToolUse", "git commit -m second") + assert.False(postResp.Skipped) + assert.Equal(1, postResp.CommitCount) + assert.Equal(next, store.sessions["session-1"].RepoHeads[branchKey]) + assert.Equal([]string{next}, store.sessions["session-1"].CommitSHAsSincePrompt[branchKey]) +} + +func TestRecordToolUseSkipsNonShellToolNames(t *testing.T) { + assert := assert.New(t) + repo := testutil.NewGitRepo(t) + repo.CommitFile("main.go", "package main\n", "initial") + store := &StateStore{path: filepath.Join(t.TempDir(), "state.json"), sessions: map[string]SessionState{}} + + for _, eventName := range []string{"PreToolUse", "PostToolUse"} { + resp, err := store.Record(Request{ + Event: Input{ + SessionID: "session-1", + CWD: repo.Path(), + HookEventName: eventName, + ToolName: "Read", + ToolInput: map[string]json.RawMessage{"command": json.RawMessage(`"git commit -m ignored"`)}, + }, + CommitThreshold: 1, + }) + require.NoError(t, err) + assert.True(resp.Skipped) + } + assert.Empty(store.sessions) +} + func TestRecordStopFailedReviewPromptUsesNewDetachedLineageKey(t *testing.T) { assert := assert.New(t) repo := testutil.NewGitRepo(t) @@ -940,6 +997,64 @@ func TestRecordPreToolUseBaselinesUntrackedRepoForLaterPostCommitRegistration(t assert.Equal("commit", post.TriggeredBy) } +func TestRecordPreAndPostToolUseTrackDroidExecuteCommits(t *testing.T) { + assert := assert.New(t) + repo := testutil.NewGitRepo(t) + repo.CommitFile("main.go", "package main\n", "initial") + + closed := false + verdict := "F" + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/repos/resolve" { + assert.NoError(json.NewEncoder(w).Encode(map[string]any{ + "tracked": true, + "repo": map[string]string{ + "root_path": repo.Path(), + "name": filepath.Base(repo.Path()), + }, + })) + return + } + assert.Equal("/api/jobs", r.URL.Path) + assert.NoError(json.NewEncoder(w).Encode(jobsResponse{ + Jobs: []storage.ReviewJob{ + {Status: storage.JobStatusDone, Closed: &closed, Verdict: &verdict, Branch: "main"}, + }, + })) + })) + t.Cleanup(server.Close) + + store := &StateStore{ + path: filepath.Join(t.TempDir(), "state.json"), + sessions: map[string]SessionState{}, + } + req := Request{ + Event: Input{ + SessionID: "session-1", + CWD: repo.Path(), + HookEventName: "PreToolUse", + ToolName: "Execute", + ToolInput: map[string]json.RawMessage{"command": json.RawMessage(`"git commit -m feature"`)}, + }, + CommitThreshold: 1, + Instruction: "Run roborev fix.", + RoborevServerAddr: server.URL, + } + + pre, err := store.Record(req) + require.NoError(t, err) + assert.False(pre.Skipped, "Droid Execute must seed the commit baseline") + + repo.CommitFile("feature.go", "package main\n", "feature") + postReq := req + postReq.Event.HookEventName = "PostToolUse" + post, err := store.Record(postReq) + require.NoError(t, err) + + assert.True(post.Triggered, "Droid Execute must count the commit after the baseline") + assert.Equal("commit", post.TriggeredBy) +} + func TestRecordStopTriggersFailedReviewOnDetachedHead(t *testing.T) { assert := assert.New(t) repo := testutil.NewGitRepo(t) diff --git a/internal/config/config.go b/internal/config/config.go index 50706f6bc..0194c4217 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -66,6 +66,12 @@ type AgentHookConfig struct { Instruction string `toml:"instruction" comment:"Instruction emitted when the agent hook decides a review/fix pass is needed."` } +type DroidHookConfig struct { + TurnThreshold int `toml:"turn_threshold" comment:"Stop hook threshold; 0 disables Stop-based prompting."` + CommitThreshold int `toml:"commit_threshold" comment:"PostToolUse commit threshold; 0 disables commit-based prompting."` + FailedReviewThreshold int `toml:"failed_review_threshold" comment:"Open failed roborev review threshold; 0 disables review-based prompting."` + Instruction string `toml:"instruction" comment:"Instruction emitted when the Droid hook decides a review/fix pass is needed."` +} type CodexConfig struct { DisableReviewSkills bool `toml:"disable_review_skills" comment:"Disable Codex skill instructions for review jobs."` IgnoreReviewUserConfig bool `toml:"ignore_review_user_config" comment:"Pass --ignore-user-config to Codex for review jobs."` @@ -251,6 +257,8 @@ type Config struct { // Optional agent harness hook integration AgentHook AgentHookConfig `toml:"agent_hook"` + // Optional Factory Droid harness hook integration + DroidHook DroidHookConfig `toml:"droid_hook"` // Kata task-context integration for review prompts KataContext KataContextConfig `toml:"kata_context"` @@ -468,6 +476,12 @@ func DefaultConfig() *Config { FailedReviewThreshold: 4, Instruction: "Invoke the $roborev-fix skill now.", }, + DroidHook: DroidHookConfig{ + TurnThreshold: 5, + CommitThreshold: 0, + FailedReviewThreshold: 4, + Instruction: "Run the roborev-fix skill to address the unresolved roborev findings, then continue.", + }, KataContext: KataContextConfig{Mode: KataModeOff, MaxChars: defaultKataMaxChars}, Agent: AgentConfig{ Codex: CodexConfig{ diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 29f9cacd5..c20522da2 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -3339,8 +3339,8 @@ func TestValidateReviewTypes(t *testing.T) { }, { name: "mixed valid with dedup", - input: []string{"security", "review", "security"}, - want: []string{"security", "default"}, + input: []string{"security", "review", "lookahead", "security"}, + want: []string{"security", "default", "lookahead"}, }, { name: "invalid type returns error", diff --git a/internal/config/panels.go b/internal/config/panels.go index b6437f586..6e06a5e53 100644 --- a/internal/config/panels.go +++ b/internal/config/panels.go @@ -406,17 +406,6 @@ func resolveSynthesisFromConfig( }, nil } -// WorkflowForReviewType maps a canonical review type to the workflow name used -// for agent/model fallback resolution. Default reviews use the "review" -// workflow; every specialized type (security, design, lookahead, ...) uses its -// own name, which also keys the generic [analyze.] override. -func WorkflowForReviewType(reviewType string) string { - if IsDefaultReviewType(reviewType) { - return "review" - } - return reviewType -} - // canonicalMemberReviewType canonicalizes a subagent's review_type, treating // empty as "default". func canonicalMemberReviewType(reviewType string) (string, error) { diff --git a/internal/config/workflow.go b/internal/config/workflow.go index b915ccfa5..f301a348b 100644 --- a/internal/config/workflow.go +++ b/internal/config/workflow.go @@ -57,6 +57,25 @@ func ValidateReviewTypes(types []string) ([]string, error) { return canonical, nil } +func ExplicitReviewTypes() []string { + return []string{ReviewTypeSecurity, ReviewTypeDesign, ReviewTypeLookahead} +} + +func ExplicitReviewTypesHelp() string { + return strings.Join(ExplicitReviewTypes(), ", ") +} + +func ValidReviewTypesHelp() string { + return "default, " + ExplicitReviewTypesHelp() +} + +func WorkflowForReviewType(reviewType string) string { + if IsDefaultReviewType(reviewType) { + return "review" + } + return reviewType +} + // NormalizeReasoning validates and normalizes a reasoning level string. // Returns the canonical form (maximum, thorough, medium, standard, fast) or an error if invalid. // Returns empty string (no error) for empty input. diff --git a/internal/daemon/ci_poller.go b/internal/daemon/ci_poller.go index 2675f124e..6a956947d 100644 --- a/internal/daemon/ci_poller.go +++ b/internal/daemon/ci_poller.go @@ -694,10 +694,7 @@ func (p *CIPoller) resolveMatrixMemberAgent( entry config.AgentReviewType, reasoning string, ) (string, string, error) { - workflow := "review" - if !config.IsDefaultReviewType(entry.ReviewType) { - workflow = entry.ReviewType - } + workflow := config.WorkflowForReviewType(entry.ReviewType) resolution, err := agent.ResolveWorkflowConfigFromConfig(entry.Agent, repoCfg, cfg, workflow, reasoning) if err != nil { return "", "", fmt.Errorf("resolve workflow config: %w", err) diff --git a/internal/daemon/server.go b/internal/daemon/server.go index e9bb92858..aa03015cb 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -658,17 +658,12 @@ func (s *Server) SetCIPoller(cp *CIPoller) { } func workflowForJob(jobType, reviewType string) string { - // "default" uses the standard "review" workflow; others use their own name. // Fix and compact jobs use the "fix" workflow since they're part of // that pipeline. - workflow := "review" if jobType == storage.JobTypeFix || jobType == storage.JobTypeCompact { return "fix" } - if !config.IsDefaultReviewType(reviewType) { - return reviewType - } - return workflow + return config.WorkflowForReviewType(reviewType) } func validatedWorktreePath(worktreePath, repoPath string) string { diff --git a/internal/daemon/server_actions_test.go b/internal/daemon/server_actions_test.go index e30424eb3..596b70aec 100644 --- a/internal/daemon/server_actions_test.go +++ b/internal/daemon/server_actions_test.go @@ -564,6 +564,7 @@ func TestWorkflowForJobFixType(t *testing.T) { assert.Equal("fix", workflowForJob(storage.JobTypeCompact, config.ReviewTypeDefault)) assert.Equal("review", workflowForJob(storage.JobTypeReview, config.ReviewTypeDefault)) assert.Equal("security", workflowForJob(storage.JobTypeReview, "security")) + assert.Equal("lookahead", workflowForJob(storage.JobTypeReview, "lookahead")) } func TestResolveRerunModelProviderUsesWorktreeConfig(t *testing.T) { diff --git a/internal/daemon/server_integration_test.go b/internal/daemon/server_integration_test.go index c67e69f74..2352ad024 100644 --- a/internal/daemon/server_integration_test.go +++ b/internal/daemon/server_integration_test.go @@ -30,6 +30,7 @@ func TestHandleEnqueueReviewTypeNormalization(t *testing.T) { {name: "default stored as-is", reviewType: "default", wantCode: http.StatusCreated, wantStored: "default"}, {name: "security stored as-is", reviewType: "security", wantCode: http.StatusCreated, wantStored: "security"}, {name: "design stored as-is", reviewType: "design", wantCode: http.StatusCreated, wantStored: "design"}, + {name: "lookahead stored as-is", reviewType: "lookahead", wantCode: http.StatusCreated, wantStored: "lookahead"}, {name: "invalid type rejected", reviewType: "bogus", wantCode: http.StatusBadRequest, wantErrorMsg: "invalid review_type"}, } diff --git a/internal/daemon/worker.go b/internal/daemon/worker.go index 7819d4ed3..040bd0711 100644 --- a/internal/daemon/worker.go +++ b/internal/daemon/worker.go @@ -1180,16 +1180,13 @@ func (wp *WorkerPool) failoverOrFailNonRetryableAgent( } // failoverWorkflow returns the config workflow key for backup -// agent/model resolution. Fix jobs map to "fix"; security/design -// jobs use their ReviewType; everything else maps to "review". +// agent/model resolution. Fix jobs map to "fix"; specialized review +// jobs use their review-type workflow mapping. func failoverWorkflow(job *storage.ReviewJob) string { if job.IsFixJob() { return "fix" } - if !config.IsDefaultReviewType(job.ReviewType) { - return job.ReviewType - } - return "review" + return config.WorkflowForReviewType(job.ReviewType) } // resolveBackupAgent determines the backup agent for a job. An explicit diff --git a/internal/prompt/templates.go b/internal/prompt/templates.go index f7aabacb5..9c2cc015d 100644 --- a/internal/prompt/templates.go +++ b/internal/prompt/templates.go @@ -59,10 +59,10 @@ func getSystemPrompt(agentName string, promptType string, now func() time.Time) fallbackName = "default_address.md.gotmpl" case "security": fallbackName = "default_security.md.gotmpl" - case "design-review": - fallbackName = "default_design_review.md.gotmpl" case "lookahead": fallbackName = "default_lookahead.md.gotmpl" + case "design-review": + fallbackName = "default_design_review.md.gotmpl" case "run": // No default run preamble - return empty so raw prompts are used return "" diff --git a/internal/prompt/templates_test.go b/internal/prompt/templates_test.go index 332249f36..8bd4ddbd6 100644 --- a/internal/prompt/templates_test.go +++ b/internal/prompt/templates_test.go @@ -151,6 +151,18 @@ func TestGetSystemPrompt_SecurityPromptRequiresExploitabilityGate(t *testing.T) assert.NotContains(t, got, "low**: Defense-in-depth improvement") } +func TestGetSystemPrompt_LookaheadPromptFocusesOnTemporalLeakage(t *testing.T) { + fixedTime := time.Date(2030, 6, 15, 0, 0, 0, 0, time.UTC) + mockNow := func() time.Time { return fixedTime } + + got := getSystemPrompt("test", "lookahead", mockNow) + + assert.Contains(t, got, "look-ahead") + assert.Contains(t, got, "future-data leakage") + assert.Contains(t, got, "information not yet available at the point in time it claims to represent") + assert.Contains(t, got, "Separate multiple findings with `---` on its own line.") +} + func TestGetSystemPrompt_DateInjection(t *testing.T) { fixedTime := time.Date(2030, 6, 15, 0, 0, 0, 0, time.UTC) fixedDateStr := "Current date: 2030-06-15 (UTC)" diff --git a/internal/review/batch.go b/internal/review/batch.go index 5d4a32e3e..23006cd7b 100644 --- a/internal/review/batch.go +++ b/internal/review/batch.go @@ -84,10 +84,7 @@ func runSingle( // Map review type to workflow name for config // resolution (same mapping as CI poller). - workflow := "review" - if !config.IsDefaultReviewType(reviewType) { - workflow = reviewType - } + workflow := config.WorkflowForReviewType(reviewType) // Workflow-aware agent/model resolution when config // is available; otherwise use the agent name as-is. diff --git a/internal/skills/claude/roborev-design-review-branch/SKILL.md b/internal/skills/claude/roborev-design-review-branch/SKILL.md index 78a9e9659..e65d89f29 100644 --- a/internal/skills/claude/roborev-design-review-branch/SKILL.md +++ b/internal/skills/claude/roborev-design-review-branch/SKILL.md @@ -33,11 +33,7 @@ When the user invokes `/roborev-design-review-branch [--base ] [--panel ### 1. Validate inputs -If a base branch is provided, verify it resolves to a valid ref: - -```bash -git rev-parse --verify -- -``` +If a base branch is provided, use the base-branch command snippet below; it stores and validates the ref before invoking `roborev review`. If validation fails, inform the user the ref is invalid. Do not proceed. @@ -45,8 +41,20 @@ If validation fails, inform the user the ref is invalid. Do not proceed. Construct the review command: +If no base branch is specified, run: + +```bash +roborev review --branch --wait --type design [--panel |none] ``` -roborev review --branch --wait --type design [--base ] [--panel |none] + +If a base branch is specified, run: + +```bash +read -r branch <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$branch" || exit 1 +roborev review --branch --wait --type design --base "$branch" [--panel |none] ``` - If `--base` is specified, include it (otherwise auto-detects the base branch) @@ -58,8 +66,20 @@ Launch a background task that runs the command. This lets the user continue work Use the `Task` tool with `run_in_background: true` and `subagent_type: "Bash"`: +If no base branch is specified, run: + +```bash +roborev review --branch --wait --type design [--panel |none] ``` -roborev review --branch --wait --type design [--base ] [--panel |none] + +If a base branch is specified, run: + +```bash +read -r branch <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$branch" || exit 1 +roborev review --branch --wait --type design --base "$branch" [--panel |none] ``` Tell the user that the design review has been submitted and they can continue working. You will present the results when the review completes. diff --git a/internal/skills/claude/roborev-design-review/SKILL.md b/internal/skills/claude/roborev-design-review/SKILL.md index 795874fd3..1cbaff8d6 100644 --- a/internal/skills/claude/roborev-design-review/SKILL.md +++ b/internal/skills/claude/roborev-design-review/SKILL.md @@ -33,11 +33,7 @@ When the user invokes `/roborev-design-review [commit] [--panel |none]`: ### 1. Validate inputs -If a commit ref is provided, verify it resolves to a valid commit: - -```bash -git rev-parse --verify -- ^{commit} -``` +If a commit ref is provided, use the commit-provided command snippet below; it stores and validates the ref before invoking `roborev review`. If validation fails, inform the user the ref is invalid. Do not proceed. @@ -45,11 +41,22 @@ If validation fails, inform the user the ref is invalid. Do not proceed. Construct the review command: +If no commit is specified: + +``` +roborev review --wait --type design [--panel |none] +``` + +If a commit is specified: + ``` -roborev review [commit] --wait --type design [--panel |none] +read -r commit <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$commit^{commit}" || exit 1 +roborev review "$commit" --wait --type design [--panel |none] ``` -- If no commit is specified, omit it (defaults to HEAD) - If `--panel ` is specified, include it (fans out to the named config panel); `--panel none` forces a single-agent review ### 3. Run the review in the background @@ -58,8 +65,20 @@ Launch a background task that runs the command. This lets the user continue work Use the `Task` tool with `run_in_background: true` and `subagent_type: "Bash"`: +If no commit is specified: + +``` +roborev review --wait --type design [--panel |none] +``` + +If a commit is specified: + ``` -roborev review [commit] --wait --type design [--panel |none] +read -r commit <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$commit^{commit}" || exit 1 +roborev review "$commit" --wait --type design [--panel |none] ``` Tell the user that the design review has been submitted and they can continue working. You will present the results when the review completes. diff --git a/internal/skills/claude/roborev-fix/SKILL.md b/internal/skills/claude/roborev-fix/SKILL.md index e91714de8..fcca8957b 100644 --- a/internal/skills/claude/roborev-fix/SKILL.md +++ b/internal/skills/claude/roborev-fix/SKILL.md @@ -156,12 +156,19 @@ it. Run these as **separate commands**, but only run `roborev close` after confirming the comment succeeded: ```bash -roborev comment --commenter roborev-fix --job "" +roborev comment --commenter roborev-fix --job -m "$(cat <<'ROBOREV_COMMENT' + +ROBOREV_COMMENT +)" # Only if the comment above succeeded: roborev close ``` -The comment should reference each finding by severity and file, state what was fixed, and note any findings intentionally skipped. Keep it concise (1-3 sentences). Escape quotes and special characters in the bash command. +**Important:** Always pass the comment text via a heredoc as shown above, never +by interpolating dynamic text directly into a shell string. Review-derived +content, file paths, and summaries may contain shell metacharacters. + +The comment should reference each finding by severity and file, state what was fixed, and note any findings intentionally skipped. Keep it concise (1-3 sentences). ### 6. Commit @@ -202,9 +209,9 @@ Agent: 4. Fixes all 3 findings across both reviews, sorted by severity, grouped by file 5. Runs `go test ./...` to verify 6. Records comments and closes reviews: - - `roborev comment --commenter roborev-fix --job 1019 "Fixed null check and added error handling"` + - Records a heredoc comment for job 1019 summarizing the fixed null check and added error handling - `roborev close 1019` - - `roborev comment --commenter roborev-fix --job 1021 "Fixed missing validation"` + - Records a heredoc comment for job 1021 summarizing the fixed missing validation - `roborev close 1021` 7. Commits the changes per project conventions, or commits before step 6 if repository policy requires a SHA in close comments 8. Audits jobs 1019 and 1021 with `roborev show --job --json` and verifies `closed=true` @@ -219,7 +226,7 @@ Agent: 3. Fixes the 2 findings from job 1019 4. Runs `go test ./...` to verify 5. Records comment and closes review: - - `roborev comment --commenter roborev-fix --job 1019 "Fixed null check in foo.go and error handling in bar.go"` + - Records a heredoc comment for job 1019 summarizing the fixed null check in `foo.go` and error handling in `bar.go` - `roborev close 1019` 6. Commits the changes per project conventions, or commits before step 5 if repository policy requires a SHA in close comments 7. Audits job 1019 with `roborev show --job 1019 --json` and verifies `closed=true` diff --git a/internal/skills/claude/roborev-lookahead-review-branch/SKILL.md b/internal/skills/claude/roborev-lookahead-review-branch/SKILL.md index b59cba2fa..daccc0bbc 100644 --- a/internal/skills/claude/roborev-lookahead-review-branch/SKILL.md +++ b/internal/skills/claude/roborev-lookahead-review-branch/SKILL.md @@ -36,11 +36,7 @@ When the user invokes `/roborev-lookahead-review-branch [--base ] [--pan ### 1. Validate inputs -If a base branch is provided, verify it resolves to a valid ref: - -```bash -git rev-parse --verify -- -``` +If a base branch is provided, use the base-branch command snippet below; it stores and validates the ref before invoking `roborev review`. If validation fails, inform the user the ref is invalid. Do not proceed. @@ -48,8 +44,20 @@ If validation fails, inform the user the ref is invalid. Do not proceed. Construct the review command: +If no base branch is specified, run: + +```bash +roborev review --branch --wait --type lookahead [--panel |none] ``` -roborev review --branch --wait --type lookahead [--base ] [--panel |none] + +If a base branch is specified, run: + +```bash +read -r branch <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$branch" || exit 1 +roborev review --branch --wait --type lookahead --base "$branch" [--panel |none] ``` - If `--base` is specified, include it (otherwise auto-detects the base branch) @@ -61,8 +69,20 @@ Launch a background task that runs the command. This lets the user continue work Use the `Task` tool with `run_in_background: true` and `subagent_type: "Bash"`: +If no base branch is specified, run: + +```bash +roborev review --branch --wait --type lookahead [--panel |none] ``` -roborev review --branch --wait --type lookahead [--base ] [--panel |none] + +If a base branch is specified, run: + +```bash +read -r branch <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$branch" || exit 1 +roborev review --branch --wait --type lookahead --base "$branch" [--panel |none] ``` Tell the user that the look-ahead review has been submitted and they can continue working. You will present the results when the review completes. diff --git a/internal/skills/claude/roborev-lookahead-review/SKILL.md b/internal/skills/claude/roborev-lookahead-review/SKILL.md index 0326250d7..625823109 100644 --- a/internal/skills/claude/roborev-lookahead-review/SKILL.md +++ b/internal/skills/claude/roborev-lookahead-review/SKILL.md @@ -36,11 +36,7 @@ When the user invokes `/roborev-lookahead-review [commit] [--panel |none]` ### 1. Validate inputs -If a commit ref is provided, verify it resolves to a valid commit: - -```bash -git rev-parse --verify -- ^{commit} -``` +If a commit ref is provided, use the commit-provided command snippet below; it stores and validates the ref before invoking `roborev review`. If validation fails, inform the user the ref is invalid. Do not proceed. @@ -48,11 +44,22 @@ If validation fails, inform the user the ref is invalid. Do not proceed. Construct the review command: +If no commit is specified: + +``` +roborev review --wait --type lookahead [--panel |none] +``` + +If a commit is specified: + ``` -roborev review [commit] --wait --type lookahead [--panel |none] +read -r commit <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$commit^{commit}" || exit 1 +roborev review "$commit" --wait --type lookahead [--panel |none] ``` -- If no commit is specified, omit it (defaults to HEAD) - If `--panel ` is specified, include it (fans out to the named config panel); `--panel none` forces a single-agent review ### 3. Run the review in the background @@ -61,8 +68,20 @@ Launch a background task that runs the command. This lets the user continue work Use the `Task` tool with `run_in_background: true` and `subagent_type: "Bash"`: +If no commit is specified: + +``` +roborev review --wait --type lookahead [--panel |none] +``` + +If a commit is specified: + ``` -roborev review [commit] --wait --type lookahead [--panel |none] +read -r commit <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$commit^{commit}" || exit 1 +roborev review "$commit" --wait --type lookahead [--panel |none] ``` Tell the user that the look-ahead review has been submitted and they can continue working. You will present the results when the review completes. diff --git a/internal/skills/claude/roborev-refine/SKILL.md b/internal/skills/claude/roborev-refine/SKILL.md index 4e0aa7e55..bece2ce9c 100644 --- a/internal/skills/claude/roborev-refine/SKILL.md +++ b/internal/skills/claude/roborev-refine/SKILL.md @@ -53,8 +53,7 @@ When the user invokes `/roborev-refine [--since ] [--branch ] [--m If `--branch` is provided, verify the current branch matches before doing any work. If it does not, stop and tell the user. -If `--since` is provided, verify it resolves to a valid commit and is an -ancestor of `HEAD`. +If `--since` is provided, use the since-scoped review snippets below; they store the raw value safely, verify it resolves to a valid commit and is an ancestor of `HEAD`, then run the review in the same shell invocation. If `--since` is not provided, ensure you are not refining the default branch. This matches `roborev refine`, which refuses to run on the default branch @@ -68,7 +67,12 @@ of fix-review cycles, not the total number of reviews. Choose the review command that matches the requested scope: ```bash -roborev review --since --wait +read -r since <<'ROBOREV_REF' + +ROBOREV_REF +resolved_since=$(git rev-parse --verify -- "$since^{commit}") || exit 1 +git merge-base --is-ancestor "$resolved_since" HEAD || exit 1 +roborev review --since "$since" --wait ``` or, if `--since` was not provided: @@ -179,7 +183,12 @@ below. Now run the explicit full-scope review. If refining with `--since`: ```bash -roborev review --since --wait +read -r since <<'ROBOREV_REF' + +ROBOREV_REF +resolved_since=$(git rev-parse --verify -- "$since^{commit}") || exit 1 +git merge-base --is-ancestor "$resolved_since" HEAD || exit 1 +roborev review --since "$since" --wait ``` If refining without `--since`: diff --git a/internal/skills/claude/roborev-review-branch/SKILL.md b/internal/skills/claude/roborev-review-branch/SKILL.md index e3d7e8a50..a74453c6f 100644 --- a/internal/skills/claude/roborev-review-branch/SKILL.md +++ b/internal/skills/claude/roborev-review-branch/SKILL.md @@ -33,11 +33,7 @@ When the user invokes `/roborev-review-branch [--base ] [--type security ### 1. Validate inputs -If a base branch is provided, verify it resolves to a valid ref: - -```bash -git rev-parse --verify -- -``` +If a base branch is provided, use the base-branch command snippet below; it stores and validates the ref before invoking `roborev review`. If validation fails, inform the user the ref is invalid. Do not proceed. @@ -45,8 +41,20 @@ If validation fails, inform the user the ref is invalid. Do not proceed. Construct the review command: +If no base branch is specified, run: + +```bash +roborev review --branch --wait [--type ] [--panel |none] ``` -roborev review --branch --wait [--base ] [--type ] [--panel |none] + +If a base branch is specified, run: + +```bash +read -r branch <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$branch" || exit 1 +roborev review --branch --wait --base "$branch" [--type ] [--panel |none] ``` - If `--base` is specified, include it (otherwise auto-detects the base branch) @@ -59,8 +67,20 @@ Launch a background task that runs the command. This lets the user continue work Use the `Task` tool with `run_in_background: true` and `subagent_type: "Bash"`: +If no base branch is specified, run: + +```bash +roborev review --branch --wait [--type ] [--panel |none] ``` -roborev review --branch --wait [--base ] [--type ] [--panel |none] + +If a base branch is specified, run: + +```bash +read -r branch <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$branch" || exit 1 +roborev review --branch --wait --base "$branch" [--type ] [--panel |none] ``` Tell the user that the branch review has been submitted and they can continue working. You will present the results when the review completes. diff --git a/internal/skills/claude/roborev-review/SKILL.md b/internal/skills/claude/roborev-review/SKILL.md index 964cbcb3d..b42f76bad 100644 --- a/internal/skills/claude/roborev-review/SKILL.md +++ b/internal/skills/claude/roborev-review/SKILL.md @@ -33,11 +33,7 @@ When the user invokes `/roborev-review [commit] [--type security|design] [--pane ### 1. Validate inputs -If a commit ref is provided, verify it resolves to a valid commit: - -```bash -git rev-parse --verify -- ^{commit} -``` +If a commit ref is provided, use the commit-provided command snippet below; it stores and validates the ref before invoking `roborev review`. If validation fails, inform the user the ref is invalid. Do not proceed. @@ -45,11 +41,22 @@ If validation fails, inform the user the ref is invalid. Do not proceed. Construct the review command: +If no commit is specified: + +``` +roborev review --wait [--type ] [--panel |none] +``` + +If a commit is specified: + ``` -roborev review [commit] --wait [--type ] [--panel |none] +read -r commit <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$commit^{commit}" || exit 1 +roborev review "$commit" --wait [--type ] [--panel |none] ``` -- If no commit is specified, omit it (defaults to HEAD) - If `--type` is specified, include it - If `--panel ` is specified, include it (fans out to the named config panel); `--panel none` forces a single-agent review @@ -59,8 +66,20 @@ Launch a background task that runs the command. This lets the user continue work Use the `Task` tool with `run_in_background: true` and `subagent_type: "Bash"`: +If no commit is specified: + +``` +roborev review --wait [--type ] [--panel |none] +``` + +If a commit is specified: + ``` -roborev review [commit] --wait [--type ] [--panel |none] +read -r commit <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$commit^{commit}" || exit 1 +roborev review "$commit" --wait [--type ] [--panel |none] ``` Tell the user that the review has been submitted and they can continue working. You will present the results when the review completes. diff --git a/internal/skills/codex/roborev-design-review-branch/SKILL.md b/internal/skills/codex/roborev-design-review-branch/SKILL.md index c584a5bfa..6d3f431d7 100644 --- a/internal/skills/codex/roborev-design-review-branch/SKILL.md +++ b/internal/skills/codex/roborev-design-review-branch/SKILL.md @@ -33,11 +33,7 @@ When the user invokes `$roborev-design-review-branch [--base ] [--panel ### 1. Validate inputs -If a base branch is provided, verify it resolves to a valid ref: - -```bash -git rev-parse --verify -- -``` +If a base branch is provided, use the base-branch command snippet below; it stores and validates the ref before invoking `roborev review`. If validation fails, inform the user the ref is invalid. Do not proceed. @@ -45,8 +41,20 @@ If validation fails, inform the user the ref is invalid. Do not proceed. Construct and execute the review command: +If no base branch is specified, run: + +```bash +roborev review --branch --wait --type design [--panel |none] +``` + +If a base branch is specified, run: + ```bash -roborev review --branch --wait --type design [--base ] [--panel |none] +read -r branch <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$branch" || exit 1 +roborev review --branch --wait --type design --base "$branch" [--panel |none] ``` - If `--base` is specified, include it (otherwise auto-detects the base branch) @@ -102,7 +110,7 @@ Agent: User: `$roborev-design-review-branch --base develop` Agent: -1. Validates: `git rev-parse --verify -- develop` +1. Validates: `git rev-parse --verify -- "develop"` 2. Executes `roborev review --branch --wait --type design --base develop` 3. Presents the verdict and findings 4. If findings exist: "Would you like me to address these findings? Run `$roborev-fix 1043`" diff --git a/internal/skills/codex/roborev-design-review/SKILL.md b/internal/skills/codex/roborev-design-review/SKILL.md index 2d4146579..9176f1cfd 100644 --- a/internal/skills/codex/roborev-design-review/SKILL.md +++ b/internal/skills/codex/roborev-design-review/SKILL.md @@ -33,11 +33,7 @@ When the user invokes `$roborev-design-review [commit] [--panel |none]`: ### 1. Validate inputs -If a commit ref is provided, verify it resolves to a valid commit: - -```bash -git rev-parse --verify -- ^{commit} -``` +If a commit ref is provided, use the commit-provided command snippet below; it stores and validates the ref before invoking `roborev review`. If validation fails, inform the user the ref is invalid. Do not proceed. @@ -45,11 +41,22 @@ If validation fails, inform the user the ref is invalid. Do not proceed. Construct and execute the review command: +If no commit is specified, run: + +```bash +roborev review --wait --type design [--panel |none] +``` + +If a commit is specified, run: + ```bash -roborev review [commit] --wait --type design [--panel |none] +read -r commit <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$commit^{commit}" || exit 1 +roborev review "$commit" --wait --type design [--panel |none] ``` -- If no commit is specified, omit it (defaults to HEAD) - If `--panel ` is specified, include it (fans out to the named config panel); `--panel none` forces a single-agent review The `--wait` flag blocks until the review completes. @@ -102,7 +109,7 @@ Agent: User: `$roborev-design-review abc123` Agent: -1. Validates: `git rev-parse --verify -- abc123^{commit}` +1. Validates: `git rev-parse --verify -- "abc123^{commit}"` 2. Executes `roborev review abc123 --wait --type design` 3. Presents the verdict and findings 4. If findings exist: "Would you like me to address these findings? Run `$roborev-fix 1043`" diff --git a/internal/skills/codex/roborev-fix/SKILL.md b/internal/skills/codex/roborev-fix/SKILL.md index 833459b3c..aada6daa4 100644 --- a/internal/skills/codex/roborev-fix/SKILL.md +++ b/internal/skills/codex/roborev-fix/SKILL.md @@ -156,12 +156,19 @@ it. Run these as **separate commands**, but only run `roborev close` after confirming the comment succeeded: ```bash -roborev comment --commenter roborev-fix --job "" +roborev comment --commenter roborev-fix --job -m "$(cat <<'ROBOREV_COMMENT' + +ROBOREV_COMMENT +)" # Only if the comment above succeeded: roborev close ``` -The comment should reference each finding by severity and file, state what was fixed, and note any findings intentionally skipped. Keep it concise (1-3 sentences). Escape quotes and special characters in the bash command. +**Important:** Always pass the comment text via a heredoc as shown above, never +by interpolating dynamic text directly into a shell string. Review-derived +content, file paths, and summaries may contain shell metacharacters. + +The comment should reference each finding by severity and file, state what was fixed, and note any findings intentionally skipped. Keep it concise (1-3 sentences). ### 6. Commit @@ -202,9 +209,9 @@ Agent: 4. Fixes all 3 findings across both reviews, sorted by severity, grouped by file 5. Runs `go test ./...` to verify 6. Records comments and closes reviews: - - `roborev comment --commenter roborev-fix --job 1019 "Fixed null check and added error handling"` + - Records a heredoc comment for job 1019 summarizing the fixed null check and added error handling - `roborev close 1019` - - `roborev comment --commenter roborev-fix --job 1021 "Fixed missing validation"` + - Records a heredoc comment for job 1021 summarizing the fixed missing validation - `roborev close 1021` 7. Commits the changes per project conventions, or commits before step 6 if repository policy requires a SHA in close comments 8. Audits jobs 1019 and 1021 with `roborev show --job --json` and verifies `closed=true` @@ -219,7 +226,7 @@ Agent: 3. Fixes the 2 findings from job 1019 4. Runs `go test ./...` to verify 5. Records comment and closes review: - - `roborev comment --commenter roborev-fix --job 1019 "Fixed null check in foo.go and error handling in bar.go"` + - Records a heredoc comment for job 1019 summarizing the fixed null check in `foo.go` and error handling in `bar.go` - `roborev close 1019` 6. Commits the changes per project conventions, or commits before step 5 if repository policy requires a SHA in close comments 7. Audits job 1019 with `roborev show --job 1019 --json` and verifies `closed=true` diff --git a/internal/skills/codex/roborev-lookahead-review-branch/SKILL.md b/internal/skills/codex/roborev-lookahead-review-branch/SKILL.md index 3c3175aee..3a130561b 100644 --- a/internal/skills/codex/roborev-lookahead-review-branch/SKILL.md +++ b/internal/skills/codex/roborev-lookahead-review-branch/SKILL.md @@ -36,11 +36,7 @@ When the user invokes `$roborev-lookahead-review-branch [--base ] [--pan ### 1. Validate inputs -If a base branch is provided, verify it resolves to a valid ref: - -```bash -git rev-parse --verify -- -``` +If a base branch is provided, use the base-branch command snippet below; it stores and validates the ref before invoking `roborev review`. If validation fails, inform the user the ref is invalid. Do not proceed. @@ -48,8 +44,20 @@ If validation fails, inform the user the ref is invalid. Do not proceed. Construct and execute the review command: +If no base branch is specified, run: + +```bash +roborev review --branch --wait --type lookahead [--panel |none] +``` + +If a base branch is specified, run: + ```bash -roborev review --branch --wait --type lookahead [--base ] [--panel |none] +read -r branch <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$branch" || exit 1 +roborev review --branch --wait --type lookahead --base "$branch" [--panel |none] ``` - If `--base` is specified, include it (otherwise auto-detects the base branch) diff --git a/internal/skills/codex/roborev-lookahead-review/SKILL.md b/internal/skills/codex/roborev-lookahead-review/SKILL.md index 3d793af94..e63273b64 100644 --- a/internal/skills/codex/roborev-lookahead-review/SKILL.md +++ b/internal/skills/codex/roborev-lookahead-review/SKILL.md @@ -36,11 +36,7 @@ When the user invokes `$roborev-lookahead-review [commit] [--panel |none]` ### 1. Validate inputs -If a commit ref is provided, verify it resolves to a valid commit: - -```bash -git rev-parse --verify -- ^{commit} -``` +If a commit ref is provided, use the commit-provided command snippet below; it stores and validates the ref before invoking `roborev review`. If validation fails, inform the user the ref is invalid. Do not proceed. @@ -48,11 +44,22 @@ If validation fails, inform the user the ref is invalid. Do not proceed. Construct and execute the review command: +If no commit is specified, run: + +```bash +roborev review --wait --type lookahead [--panel |none] +``` + +If a commit is specified, run: + ```bash -roborev review [commit] --wait --type lookahead [--panel |none] +read -r commit <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$commit^{commit}" || exit 1 +roborev review "$commit" --wait --type lookahead [--panel |none] ``` -- If no commit is specified, omit it (defaults to HEAD) - If `--panel ` is specified, include it (fans out to the named config panel); `--panel none` forces a single-agent review The `--wait` flag blocks until the review completes. @@ -105,7 +112,7 @@ Agent: User: `$roborev-lookahead-review abc123` Agent: -1. Validates: `git rev-parse --verify -- abc123^{commit}` +1. Validates: `git rev-parse --verify -- "abc123^{commit}"` 2. Executes `roborev review abc123 --wait --type lookahead` 3. Presents the verdict and findings 4. If findings exist: "Would you like me to address these findings? Run `$roborev-fix 1043`" diff --git a/internal/skills/codex/roborev-refine/SKILL.md b/internal/skills/codex/roborev-refine/SKILL.md index 50be424b3..f1ca81d47 100644 --- a/internal/skills/codex/roborev-refine/SKILL.md +++ b/internal/skills/codex/roborev-refine/SKILL.md @@ -53,8 +53,7 @@ When the user invokes `$roborev-refine [--since ] [--branch ] [--m If `--branch` is provided, verify the current branch matches before doing any work. If it does not, stop and tell the user. -If `--since` is provided, verify it resolves to a valid commit and is an -ancestor of `HEAD`. +If `--since` is provided, use the since-scoped review snippets below; they store the raw value safely, verify it resolves to a valid commit and is an ancestor of `HEAD`, then run the review in the same shell invocation. If `--since` is not provided, ensure you are not refining the default branch. This matches `roborev refine`, which refuses to run on the default branch @@ -68,7 +67,12 @@ of fix-review cycles, not the total number of reviews. Choose the review command that matches the requested scope: ```bash -roborev review --since --wait +read -r since <<'ROBOREV_REF' + +ROBOREV_REF +resolved_since=$(git rev-parse --verify -- "$since^{commit}") || exit 1 +git merge-base --is-ancestor "$resolved_since" HEAD || exit 1 +roborev review --since "$since" --wait ``` or, if `--since` was not provided: @@ -179,7 +183,12 @@ below. Now run the explicit full-scope review. If refining with `--since`: ```bash -roborev review --since --wait +read -r since <<'ROBOREV_REF' + +ROBOREV_REF +resolved_since=$(git rev-parse --verify -- "$since^{commit}") || exit 1 +git merge-base --is-ancestor "$resolved_since" HEAD || exit 1 +roborev review --since "$since" --wait ``` If refining without `--since`: diff --git a/internal/skills/codex/roborev-review-branch/SKILL.md b/internal/skills/codex/roborev-review-branch/SKILL.md index 5d3b9e429..57d5e8968 100644 --- a/internal/skills/codex/roborev-review-branch/SKILL.md +++ b/internal/skills/codex/roborev-review-branch/SKILL.md @@ -33,11 +33,7 @@ When the user invokes `$roborev-review-branch [--base ] [--type security ### 1. Validate inputs -If a base branch is provided, verify it resolves to a valid ref: - -```bash -git rev-parse --verify -- -``` +If a base branch is provided, use the base-branch command snippet below; it stores and validates the ref before invoking `roborev review`. If validation fails, inform the user the ref is invalid. Do not proceed. @@ -45,8 +41,20 @@ If validation fails, inform the user the ref is invalid. Do not proceed. Construct and execute the review command: +If no base branch is specified, run: + +```bash +roborev review --branch --wait [--type ] [--panel |none] +``` + +If a base branch is specified, run: + ```bash -roborev review --branch --wait [--base ] [--type ] [--panel |none] +read -r branch <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$branch" || exit 1 +roborev review --branch --wait --base "$branch" [--type ] [--panel |none] ``` - If `--base` is specified, include it (otherwise auto-detects the base branch) @@ -103,7 +111,7 @@ Agent: User: `$roborev-review-branch --base develop --type security` Agent: -1. Validates: `git rev-parse --verify -- develop` +1. Validates: `git rev-parse --verify -- "develop"` 2. Executes `roborev review --branch --wait --base develop --type security` 3. Presents the verdict and findings 4. If findings exist: "Would you like me to address these findings? Run `$roborev-fix 1043`" diff --git a/internal/skills/codex/roborev-review/SKILL.md b/internal/skills/codex/roborev-review/SKILL.md index afb9d6e93..03cc696fd 100644 --- a/internal/skills/codex/roborev-review/SKILL.md +++ b/internal/skills/codex/roborev-review/SKILL.md @@ -33,11 +33,7 @@ When the user invokes `$roborev-review [commit] [--type security|design] [--pane ### 1. Validate inputs -If a commit ref is provided, verify it resolves to a valid commit: - -```bash -git rev-parse --verify -- ^{commit} -``` +If a commit ref is provided, use the commit-provided command snippet below; it stores and validates the ref before invoking `roborev review`. If validation fails, inform the user the ref is invalid. Do not proceed. @@ -45,11 +41,22 @@ If validation fails, inform the user the ref is invalid. Do not proceed. Construct and execute the review command: +If no commit is specified, run: + +```bash +roborev review --wait [--type ] [--panel |none] +``` + +If a commit is specified, run: + ```bash -roborev review [commit] --wait [--type ] [--panel |none] +read -r commit <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$commit^{commit}" || exit 1 +roborev review "$commit" --wait [--type ] [--panel |none] ``` -- If no commit is specified, omit it (defaults to HEAD) - If `--type` is specified, include it - If `--panel ` is specified, include it (fans out to the named config panel); `--panel none` forces a single-agent review @@ -103,7 +110,7 @@ Agent: User: `$roborev-review abc123 --type security` Agent: -1. Validates: `git rev-parse --verify -- abc123^{commit}` +1. Validates: `git rev-parse --verify -- "abc123^{commit}"` 2. Executes `roborev review abc123 --wait --type security` 3. Presents the verdict and findings 4. If findings exist: "Would you like me to address these findings? Run `$roborev-fix 1043`" diff --git a/internal/skills/droid/roborev-design-review-branch/SKILL.md b/internal/skills/droid/roborev-design-review-branch/SKILL.md new file mode 100644 index 000000000..065a017b7 --- /dev/null +++ b/internal/skills/droid/roborev-design-review-branch/SKILL.md @@ -0,0 +1,122 @@ +--- +name: roborev-design-review-branch +description: Request a design review for all commits on the current branch and present the results +--- + +# roborev-design-review-branch + +Request a design review for all commits on the current branch and present the results. + +## Usage + +``` +/roborev-design-review-branch [--base ] [--panel |none] +``` + +## When NOT to invoke this skill + +Do NOT invoke this skill when the user is presenting or pasting existing review +results. Messages that contain review findings, verdicts, or summaries are +outputs — not requests to start a new review. + +## IMPORTANT + +This skill requires you to **execute bash commands** to validate inputs and run the review. The task is not complete until the review finishes and you present the results to the user. + +These instructions are guidelines, not a rigid script. Use the conversation +context. Skip steps that are already satisfied. Defer to project-level +AGENTS.md instructions when they conflict with these steps. + +## Instructions + +When the user invokes `/roborev-design-review-branch [--base ] [--panel |none]`: + +### 1. Validate inputs + +If a base branch is provided, use the base-branch command snippet below; it stores and validates the ref before invoking `roborev review`. + +If validation fails, inform the user the ref is invalid. Do not proceed. + +### 2. Build and run the command + +Construct and execute the review command: + +If no base branch is specified, run: + +```bash +roborev review --branch --wait --type design [--panel |none] +``` + +If a base branch is specified, run: + +```bash +read -r branch <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$branch" || exit 1 +roborev review --branch --wait --type design --base "$branch" [--panel |none] +``` + +- If `--base` is specified, include it (otherwise auto-detects the base branch) +- If `--panel ` is specified, include it (fans out to the named config panel); `--panel none` forces a single-agent review + +The `--wait` flag blocks until the review completes. + +### 3. Present the results + +If the command output contains an error (e.g., daemon not running, repo not initialized, review errored), report it to the user. Suggest `roborev status` to check the daemon, `roborev init` if the repo is not initialized, or re-running the review. + +Otherwise, present the review to the user: +- Show the verdict prominently (Pass or Fail) +- If there are findings, list them grouped by severity with file paths and line numbers so the user can navigate directly +- If the review passed, a brief confirmation is sufficient + +#### Panels (multi-reviewer reviews) + +If you pass `--panel `, or a `default_panel` is configured for explicit +reviews, the review fans out to a panel of reviewers. In that case the +`Enqueued job ` is the **synthesis (parent)** job that aggregates them, and +its verdict and findings are the synthesized result across the whole panel. +Present that synthesized verdict/findings, and offer fix on that parent id — +never an individual reviewer. `roborev show` prints a one-line reviewers summary +(e.g. `3 reviewers: bug P, security F`) for a synthesis job. `--panel none` +forces a single-agent review, and automatic post-commit hook reviews stay +single-agent regardless of `default_panel`. + +### 4. Offer next steps + +If the review has findings (verdict is Fail), offer to address them: + +- "Would you like me to fix these findings? You can run `/roborev-fix `" + +Extract the job ID from the review output to include in the suggestion. Look for it in the `Enqueued job for ...` line or in the review header. For a panel review this id is the synthesis parent. + +If the review passed, confirm the result and do not offer `/roborev-fix`. + +## Examples + +**Default branch design review:** + +User: `/roborev-design-review-branch` + +Agent: +1. Executes `roborev review --branch --wait --type design` +2. Presents the verdict and findings grouped by severity +3. If findings exist: "Would you like me to address these findings? Run `/roborev-fix 1042`" +4. If passed: "Branch design review passed with no findings." + +**Design review against a specific base:** + +User: `/roborev-design-review-branch --base develop` + +Agent: +1. Validates: `git rev-parse --verify -- "develop"` +2. Executes `roborev review --branch --wait --type design --base develop` +3. Presents the verdict and findings +4. If findings exist: "Would you like me to address these findings? Run `/roborev-fix 1043`" + +## See also + +- `/roborev-review-branch --type design` — equivalent, with additional `--type` flexibility +- `/roborev-design-review` — design review a single commit +- `/roborev-fix` — fix a review's findings in code diff --git a/internal/skills/droid/roborev-design-review/SKILL.md b/internal/skills/droid/roborev-design-review/SKILL.md new file mode 100644 index 000000000..ab352f537 --- /dev/null +++ b/internal/skills/droid/roborev-design-review/SKILL.md @@ -0,0 +1,121 @@ +--- +name: roborev-design-review +description: Request a design review for a commit and present the results +--- + +# roborev-design-review + +Request a design review for a commit and present the results. + +## Usage + +``` +/roborev-design-review [commit] [--panel |none] +``` + +## When NOT to invoke this skill + +Do NOT invoke this skill when the user is presenting or pasting existing review +results. Messages that contain review findings, verdicts, or summaries are +outputs — not requests to start a new review. + +## IMPORTANT + +This skill requires you to **execute bash commands** to validate the commit and run the review. The task is not complete until the review finishes and you present the results to the user. + +These instructions are guidelines, not a rigid script. Use the conversation +context. Skip steps that are already satisfied. Defer to project-level +AGENTS.md instructions when they conflict with these steps. + +## Instructions + +When the user invokes `/roborev-design-review [commit] [--panel |none]`: + +### 1. Validate inputs + +If a commit ref is provided, use the commit-provided command snippet below; it stores and validates the ref before invoking `roborev review`. + +If validation fails, inform the user the ref is invalid. Do not proceed. + +### 2. Build and run the command + +Construct and execute the review command: + +If no commit is specified, run: + +```bash +roborev review --wait --type design [--panel |none] +``` + +If a commit is specified, run: + +```bash +read -r commit <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$commit^{commit}" || exit 1 +roborev review "$commit" --wait --type design [--panel |none] +``` + +- If `--panel ` is specified, include it (fans out to the named config panel); `--panel none` forces a single-agent review + +The `--wait` flag blocks until the review completes. + +### 3. Present the results + +If the command output contains an error (e.g., daemon not running, repo not initialized, review errored), report it to the user. Suggest `roborev status` to check the daemon, `roborev init` if the repo is not initialized, or re-running the review. + +Otherwise, present the review to the user: +- Show the verdict prominently (Pass or Fail) +- If there are findings, list them grouped by severity with file paths and line numbers so the user can navigate directly +- If the review passed, a brief confirmation is sufficient + +#### Panels (multi-reviewer reviews) + +If you pass `--panel `, or a `default_panel` is configured for explicit +reviews, the review fans out to a panel of reviewers. In that case the +`Enqueued job ` is the **synthesis (parent)** job that aggregates them, and +its verdict and findings are the synthesized result across the whole panel. +Present that synthesized verdict/findings, and offer fix on that parent id — +never an individual reviewer. `roborev show` prints a one-line reviewers summary +(e.g. `3 reviewers: bug P, security F`) for a synthesis job. `--panel none` +forces a single-agent review, and automatic post-commit hook reviews stay +single-agent regardless of `default_panel`. + +### 4. Offer next steps + +If the review has findings (verdict is Fail), offer to address them: + +- "Would you like me to fix these findings? You can run `/roborev-fix `" + +Extract the job ID from the review output to include in the suggestion. Look for it in the `Enqueued job for ...` line or in the review header. For a panel review this id is the synthesis parent. + +If the review passed, confirm the result and do not offer `/roborev-fix`. + +## Examples + +**Default design review of HEAD:** + +User: `/roborev-design-review` + +Agent: +1. Executes `roborev review --wait --type design` +2. Presents the verdict and findings grouped by severity +3. If findings exist: "Would you like me to address these findings? Run `/roborev-fix 1042`" +4. If passed: "Design review passed with no findings." + +**Design review of a specific commit:** + +User: `/roborev-design-review abc123` + +Agent: +1. Validates: `git rev-parse --verify -- "abc123^{commit}"` +2. Executes `roborev review abc123 --wait --type design` +3. Presents the verdict and findings +4. If findings exist: "Would you like me to address these findings? Run `/roborev-fix 1043`" + +## See also + +- `/roborev-review --type design` — equivalent, with additional `--type` flexibility +- `/roborev-design-review-branch` — design review all commits on the current branch +- `/roborev-fix` — fix a review's findings in code diff --git a/internal/skills/droid/roborev-fix/SKILL.md b/internal/skills/droid/roborev-fix/SKILL.md new file mode 100644 index 000000000..e09573f88 --- /dev/null +++ b/internal/skills/droid/roborev-fix/SKILL.md @@ -0,0 +1,236 @@ +--- +name: roborev-fix +description: Use when the user asks to fix open failing reviews, invokes /roborev-fix, or provides job IDs; do not use when the user only pastes review findings with no request to discover or close reviews +--- + +# roborev-fix + +Fix all open failing review findings in one pass. + +## Usage + +``` +/roborev-fix [job_id...] +``` + +## When NOT to invoke this skill + +Do NOT invoke this skill just because the user pasted existing review +findings or review text into the conversation. + +If the prompt already contains the findings to fix, treat that as direct fix +input and work on the code normally. The presence of verdicts, severities, +file paths, suggested fixes, or copied review summaries is not by itself a +request to run `/roborev-fix`. + +Use this skill when the user explicitly invokes `/roborev-fix`, asks to fix +open failing/unaddressed reviews (in any phrasing), provides job IDs that need +fetching, or gives a mix of job IDs and pasted findings. + +## IMPORTANT + +You must **execute bash commands** to complete this task. Skip steps already satisfied by conversation context. Defer to AGENTS.md when it conflicts. + +## Instructions + +When the user invokes `/roborev-fix [job_id...]`: + +### 1. Gather findings + +**Check the conversation first.** If the user has already pasted review +findings (verdicts, severities, file paths, suggested fixes), use those +directly. Do not re-fetch reviews that are already present in the +conversation. When reusing pasted findings, collect any job IDs mentioned +alongside them — step 5 needs these to comment on and close the reviews. +If job IDs are missing from the pasted output, discover them via +`roborev fix --list` and match each pasted finding to the correct +job by commit SHA or reviewed file paths. If a finding cannot be +confidently matched to a specific job, ask the user for the job ID +rather than closing the wrong review. + +If job IDs are provided and findings are NOT already in the conversation, +fetch them: + +```bash +roborev show --job --json +``` + +If no job IDs are provided and no findings are in the conversation, discover +open failing reviews: + +```bash +roborev fix --list +``` + +This lists each actionable open failing job with its ID, commit SHA/ref, agent, and summary (a panel review shows as its synthesis parent). +Collect the job IDs from the output. + +If the command fails, report the error to the user. Common causes: the daemon +is not running, or the repo is not initialized (suggest `roborev init`). + +If no open failing reviews are found, inform the user there is nothing to fix. + +### 2. Fetch reviews (if needed) + +Skip this step if findings are already available from step 1. + +For each job ID, fetch the full review as JSON: + +```bash +roborev show --job --json +``` + +If the command fails for a job ID, report the error and continue with the remaining jobs. + +The JSON output has this structure: +- `job_id`: the job ID +- `output`: the review text containing findings +- `job.verdict`: `"P"` for pass, `"F"` for fail (may be empty if the review errored) +- `job.git_ref`: the reviewed git ref (SHA, range, or synthetic ref) +- `closed`: whether this review has already been closed +- `comments`: array of comments left on this review (may be empty or absent) + - Each comment has `responder` (who left it) and `response` (the text) + - Comments from `roborev-fix` or `roborev-refine` are automated tool records + - All other comments are from the developer (user feedback) + +A discovered actionable open failing job may be a **synthesis (panel) parent**. Its `output` and +`job.verdict` are the synthesized result across the panel's reviewers, so fix +from the parent exactly as you would a single review. When the job is a panel, +`show --json` also includes an additive top-level `panel` block: + +- `run_uuid`, `name`, `synthesis_job_id` +- `members`: array of reviewers, each with `job_id`, `name`, `agent`, + `review_type`, `status`, and `verdict` (empty or absent until the member finishes) + +Discovery lists parents only (synthesis jobs and non-panel reviews), never +individual members. Comment on and close the parent. Drill into a member's own +review (`show --json --job `) only if the user explicitly asks. + +Skip any reviews where `job.verdict` is `"P"` (passing reviews have no findings to fix). +Skip any reviews where `job.verdict` is empty or missing (the review may have errored and is not actionable). +Skip any reviews where `closed` is `true`, unless the user explicitly provided that job ID (in which case, warn them and ask to confirm). + +If all discovered reviews are passed, closed, or otherwise skipped, inform the user there is nothing to fix. + +If the review has `comments`, respect any developer feedback (false positives, preferred approaches). + +The actionable closure set is exactly the non-skipped failing job IDs collected +in steps 1-2. Keep this original job list separate from any jobs created later +by commit hooks or follow-up reviews. + +### 3. Fix all findings + +If a finding's context is unclear from the review output alone and `job.git_ref` is not `"dirty"`, run `git show ` to see the original diff. Only do this when needed — the review output usually contains enough detail (file paths, line numbers, descriptions) to fix findings directly. + +Parse findings from the `output` field of all failing reviews. Collect every finding with its severity, file path, and line number. Then: + +1. **Sort by severity**: fix HIGH findings first, then MEDIUM, then LOW +2. **Group by file**: within each severity level, batch edits to the same file to minimize context switches +3. If the same file has findings from multiple reviews, fix them all together in one edit +4. If some findings cannot be fixed (false positives, intentional design), note them for the comment rather than silently skipping them + +### 4. Run tests + +Run the project's test suite to verify all fixes work: + +```bash +go test ./... +``` + +Or whatever test command the project uses. If tests fail, fix the regressions before proceeding. + +### 5. Record comments and close reviews + +Closure ordering is mandatory. After fixes are verified, comment on and close +exactly the original actionable job IDs from steps 1-2 before waiting on, +fetching, or responding to any new review created by commit hooks. Do not treat +a post-fix auto-review as a prerequisite for closing the original addressed +reviews; handle that new review in a separate `/roborev-fix` cycle. + +If repository policy requires committing before close comments can reference a +SHA, perform step 6 first, then immediately return here and close the original +job set. Otherwise, close before committing. + +For each original job that was fixed, record a summary comment and then close +it. Run these as **separate commands**, but only run `roborev close` after +confirming the comment succeeded: + +```bash +roborev comment --commenter roborev-fix --job -m "$(cat <<'ROBOREV_COMMENT' + +ROBOREV_COMMENT +)" +# Only if the comment above succeeded: +roborev close +``` + +**Important:** Always pass the comment text via a heredoc as shown above, never +by interpolating dynamic text directly into a shell string. Review-derived +content, file paths, and summaries may contain shell metacharacters. + +The comment should reference each finding by severity and file, state what was fixed, and note any findings intentionally skipped. Keep it concise (1-3 sentences). + +### 6. Commit + +Follow the project's commit conventions (see AGENTS.md). If the project +instructs you to always commit, do so without asking. + +### 7. Audit original closures + +Before the final response, explicitly audit the original actionable job IDs and +verify each reports `closed=true`: + +```bash +roborev show --job --json +``` + +Do not rely on `roborev list --open` for this audit; unrelated open reviews can +obscure whether the original closure set was handled. + +## Examples + +**Pasted findings in the prompt:** + +User: "Roborev found HIGH in foo.go:42 and MEDIUM in bar.go:10 ..." + +Agent: +1. Treats the pasted findings as direct fix input +2. Fixes the code directly without invoking `/roborev-fix` +3. Only uses roborev commands if the user later asks to comment on or close a specific review + +**Auto-discovery:** + +User: `/roborev-fix` + +Agent: +1. Runs `roborev fix --list` and finds 2 open failing reviews: job 1019 and job 1021 +2. Fetches both reviews with `roborev show --job 1019 --json` and `roborev show --job 1021 --json` +3. Runs `git show ` for one review where the finding lacked enough context +4. Fixes all 3 findings across both reviews, sorted by severity, grouped by file +5. Runs `go test ./...` to verify +6. Records comments and closes reviews: + - Records a heredoc comment for job 1019 summarizing the fixed null check and added error handling + - `roborev close 1019` + - Records a heredoc comment for job 1021 summarizing the fixed missing validation + - `roborev close 1021` +7. Commits the changes per project conventions, or commits before step 6 if repository policy requires a SHA in close comments +8. Audits jobs 1019 and 1021 with `roborev show --job --json` and verifies `closed=true` + +**Explicit job IDs:** + +User: `/roborev-fix 1019 1021` + +Agent: +1. Skips discovery, fetches job 1019 and 1021 directly +2. Job 1019 is verdict Fail with 2 findings; job 1021 is verdict Pass — skips 1021, informs user +3. Fixes the 2 findings from job 1019 +4. Runs `go test ./...` to verify +5. Records comment and closes review: + - Records a heredoc comment for job 1019 summarizing the fixed null check in `foo.go` and error handling in `bar.go` + - `roborev close 1019` +6. Commits the changes per project conventions, or commits before step 5 if repository policy requires a SHA in close comments +7. Audits job 1019 with `roborev show --job 1019 --json` and verifies `closed=true` + +## See also + +- `/roborev-respond` — comment on a review and close it without fixing code diff --git a/internal/skills/droid/roborev-lookahead-review-branch/SKILL.md b/internal/skills/droid/roborev-lookahead-review-branch/SKILL.md new file mode 100644 index 000000000..7cbe9d2d5 --- /dev/null +++ b/internal/skills/droid/roborev-lookahead-review-branch/SKILL.md @@ -0,0 +1,125 @@ +--- +name: roborev-lookahead-review-branch +description: Request a time-series look-ahead (a.k.a. peekahead / future-data leakage) review for all commits on the current branch and present the results +--- + +# roborev-lookahead-review-branch + +Request a time-series look-ahead review for all commits on the current branch and +present the results. A look-ahead review checks whether the change uses +information that would not yet be available at the point in time it represents — +also called peekahead, future leakage, or temporal leakage. + +## Usage + +``` +/roborev-lookahead-review-branch [--base ] [--panel |none] +``` + +## When NOT to invoke this skill + +Do NOT invoke this skill when the user is presenting or pasting existing review +results. Messages that contain review findings, verdicts, or summaries are +outputs — not requests to start a new review. + +## IMPORTANT + +This skill requires you to **execute bash commands** to validate inputs and run the review. The task is not complete until the review finishes and you present the results to the user. + +These instructions are guidelines, not a rigid script. Use the conversation +context. Skip steps that are already satisfied. Defer to project-level +AGENTS.md instructions when they conflict with these steps. + +## Instructions + +When the user invokes `/roborev-lookahead-review-branch [--base ] [--panel |none]`: + +### 1. Validate inputs + +If a base branch is provided, use the base-branch command snippet below; it stores and validates the ref before invoking `roborev review`. + +If validation fails, inform the user the ref is invalid. Do not proceed. + +### 2. Build and run the command + +Construct and execute the review command: + +If no base branch is specified, run: + +```bash +roborev review --branch --wait --type lookahead [--panel |none] +``` + +If a base branch is specified, run: + +```bash +read -r branch <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$branch" || exit 1 +roborev review --branch --wait --type lookahead --base "$branch" [--panel |none] +``` + +- If `--base` is specified, include it (otherwise auto-detects the base branch) +- If `--panel ` is specified, include it (fans out to the named config panel); `--panel none` forces a single-agent review + +The `--wait` flag blocks until the review completes. + +### 3. Present the results + +If the command output contains an error (e.g., daemon not running, repo not initialized, review errored), report it to the user. Suggest `roborev status` to check the daemon, `roborev init` if the repo is not initialized, or re-running the review. + +Otherwise, present the review to the user: +- Show the verdict prominently (Pass or Fail) +- If there are findings, list them grouped by severity with file paths and line numbers so the user can navigate directly +- If the review passed, a brief confirmation is sufficient + +#### Panels (multi-reviewer reviews) + +If you pass `--panel `, or a `default_panel` is configured for explicit +reviews, the review fans out to a panel of reviewers. In that case the +`Enqueued job ` is the **synthesis (parent)** job that aggregates them, and +its verdict and findings are the synthesized result across the whole panel. +Present that synthesized verdict/findings, and offer fix on that parent id — +never an individual reviewer. `roborev show` prints a one-line reviewers summary +(e.g. `3 reviewers: bug P, security F`) for a synthesis job. `--panel none` +forces a single-agent review, and automatic post-commit hook reviews stay +single-agent regardless of `default_panel`. + +### 4. Offer next steps + +If the review has findings (verdict is Fail), offer to address them: + +- "Would you like me to fix these findings? You can run `/roborev-fix `" + +Extract the job ID from the review output to include in the suggestion. Look for it in the `Enqueued job for ...` line or in the review header. For a panel review this id is the synthesis parent. + +If the review passed, confirm the result and do not offer `/roborev-fix`. + +## Examples + +**Default branch look-ahead review:** + +User: `/roborev-lookahead-review-branch` + +Agent: +1. Executes `roborev review --branch --wait --type lookahead` +2. Presents the verdict and findings grouped by severity +3. If findings exist: "Would you like me to address these findings? Run `/roborev-fix 1042`" +4. If passed: "Branch look-ahead review passed with no findings." + +**Look-ahead review against a specific base:** + +User: `/roborev-lookahead-review-branch --base develop` + +Agent: +1. Validates `develop` resolves to a valid ref +2. Executes `roborev review --branch --wait --type lookahead --base develop` +3. Presents the verdict and findings +4. If findings exist: "Would you like me to address these findings? Run `/roborev-fix 1043`" + +## See also + +- `/roborev-review-branch --type lookahead` — equivalent, with additional `--type` flexibility +- `/roborev-lookahead-review` — look-ahead review a single commit +- `/roborev-fix` — fix a review's findings in code diff --git a/internal/skills/droid/roborev-lookahead-review/SKILL.md b/internal/skills/droid/roborev-lookahead-review/SKILL.md new file mode 100644 index 000000000..110039329 --- /dev/null +++ b/internal/skills/droid/roborev-lookahead-review/SKILL.md @@ -0,0 +1,124 @@ +--- +name: roborev-lookahead-review +description: Request a time-series look-ahead (a.k.a. peekahead / future-data leakage) review for a commit and present the results +--- + +# roborev-lookahead-review + +Request a time-series look-ahead review for a commit and present the results. A +look-ahead review checks whether the change uses information that would not yet +be available at the point in time it represents — also called peekahead, future +leakage, or temporal leakage. + +## Usage + +``` +/roborev-lookahead-review [commit] [--panel |none] +``` + +## When NOT to invoke this skill + +Do NOT invoke this skill when the user is presenting or pasting existing review +results. Messages that contain review findings, verdicts, or summaries are +outputs — not requests to start a new review. + +## IMPORTANT + +This skill requires you to **execute bash commands** to validate the commit and run the review. The task is not complete until the review finishes and you present the results to the user. + +These instructions are guidelines, not a rigid script. Use the conversation +context. Skip steps that are already satisfied. Defer to project-level +AGENTS.md instructions when they conflict with these steps. + +## Instructions + +When the user invokes `/roborev-lookahead-review [commit] [--panel |none]`: + +### 1. Validate inputs + +If a commit ref is provided, use the commit-provided command snippet below; it stores and validates the ref before invoking `roborev review`. + +If validation fails, inform the user the ref is invalid. Do not proceed. + +### 2. Build and run the command + +Construct and execute the review command: + +If no commit is specified, run: + +```bash +roborev review --wait --type lookahead [--panel |none] +``` + +If a commit is specified, run: + +```bash +read -r commit <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$commit^{commit}" || exit 1 +roborev review "$commit" --wait --type lookahead [--panel |none] +``` + +- If `--panel ` is specified, include it (fans out to the named config panel); `--panel none` forces a single-agent review + +The `--wait` flag blocks until the review completes. + +### 3. Present the results + +If the command output contains an error (e.g., daemon not running, repo not initialized, review errored), report it to the user. Suggest `roborev status` to check the daemon, `roborev init` if the repo is not initialized, or re-running the review. + +Otherwise, present the review to the user: +- Show the verdict prominently (Pass or Fail) +- If there are findings, list them grouped by severity with file paths and line numbers so the user can navigate directly +- If the review passed, a brief confirmation is sufficient + +#### Panels (multi-reviewer reviews) + +If you pass `--panel `, or a `default_panel` is configured for explicit +reviews, the review fans out to a panel of reviewers. In that case the +`Enqueued job ` is the **synthesis (parent)** job that aggregates them, and +its verdict and findings are the synthesized result across the whole panel. +Present that synthesized verdict/findings, and offer fix on that parent id — +never an individual reviewer. `roborev show` prints a one-line reviewers summary +(e.g. `3 reviewers: bug P, security F`) for a synthesis job. `--panel none` +forces a single-agent review, and automatic post-commit hook reviews stay +single-agent regardless of `default_panel`. + +### 4. Offer next steps + +If the review has findings (verdict is Fail), offer to address them: + +- "Would you like me to fix these findings? You can run `/roborev-fix `" + +Extract the job ID from the review output to include in the suggestion. Look for it in the `Enqueued job for ...` line or in the review header. For a panel review this id is the synthesis parent. + +If the review passed, confirm the result and do not offer `/roborev-fix`. + +## Examples + +**Default look-ahead review of HEAD:** + +User: `/roborev-lookahead-review` + +Agent: +1. Executes `roborev review --wait --type lookahead` +2. Presents the verdict and findings grouped by severity +3. If findings exist: "Would you like me to address these findings? Run `/roborev-fix 1042`" +4. If passed: "Look-ahead review passed with no findings." + +**Look-ahead review of a specific commit:** + +User: `/roborev-lookahead-review abc123` + +Agent: +1. Validates: `git rev-parse --verify -- "abc123^{commit}"` +2. Executes `roborev review abc123 --wait --type lookahead` +3. Presents the verdict and findings +4. If findings exist: "Would you like me to address these findings? Run `/roborev-fix 1043`" + +## See also + +- `/roborev-review --type lookahead` — equivalent, with additional `--type` flexibility +- `/roborev-lookahead-review-branch` — look-ahead review all commits on the current branch +- `/roborev-fix` — fix a review's findings in code diff --git a/internal/skills/droid/roborev-refine/SKILL.md b/internal/skills/droid/roborev-refine/SKILL.md new file mode 100644 index 000000000..f89e5729c --- /dev/null +++ b/internal/skills/droid/roborev-refine/SKILL.md @@ -0,0 +1,258 @@ +--- +name: roborev-refine +description: Iterative review-fix loop for the current branch — reviews via daemon, fixes inline, re-reviews until passing or max iterations reached +--- + +# roborev-refine + +Iterative review-fix loop: review the current branch or commit range, fix +findings, commit, re-review, and repeat until all reviews pass or the +iteration limit is reached. + +Unlike `/roborev-fix` (single-pass fix without re-review), refine closes the +loop by re-reviewing after each fix to verify the findings are resolved. + +This skill should perform the refine workflow inside the current coding agent +CLI. Do not simply shell out to `roborev refine`. + +## Usage + +``` +/roborev-refine [--since ] [--branch ] [--max-iterations ] +``` + +- `--since `: refine commits after this commit (exclusive); required on the default branch +- `--branch `: validate that the current branch matches before refining +- `--max-iterations `: maximum fix-review cycles (default: 10) + +This skill intentionally focuses on the current branch flow. It does not expose +`roborev refine --all-branches` or `roborev refine --list`. + +## When NOT to invoke this skill + +Do NOT invoke this skill when the user is presenting or pasting existing review +results, or when they only want a single review without fixing. Use +`/roborev-review-branch` for review-only and `/roborev-fix` for fix-only. + +## IMPORTANT + +This skill requires you to **execute bash commands** to validate inputs, launch +reviews, and wait for re-review. The task is not complete until the refine loop +finishes and you present the result to the user. + +These instructions are guidelines, not a rigid script. Use the conversation +context. Skip steps that are already satisfied. Defer to project-level +AGENTS.md instructions when they conflict with these steps. + +## Instructions + +When the user invokes `/roborev-refine [--since ] [--branch ] [--max-iterations ]`: + +### 1. Validate inputs and refine context + +If `--branch` is provided, verify the current branch matches before doing any +work. If it does not, stop and tell the user. + +If `--since` is provided, use the since-scoped review snippets below; they store the raw value safely, verify it resolves to a valid commit and is an ancestor of `HEAD`, then run the review in the same shell invocation. + +If `--since` is not provided, ensure you are not refining the default branch. +This matches `roborev refine`, which refuses to run on the default branch +without `--since`. + +Parse `--max-iterations` if provided (default: 10). This is the maximum number +of fix-review cycles, not the total number of reviews. + +### 2. Run the initial review + +Choose the review command that matches the requested scope: + +```bash +read -r since <<'ROBOREV_REF' + +ROBOREV_REF +resolved_since=$(git rev-parse --verify -- "$since^{commit}") || exit 1 +git merge-base --is-ancestor "$resolved_since" HEAD || exit 1 +roborev review --since "$since" --wait +``` + +or, if `--since` was not provided: + +```bash +roborev review --branch --wait +``` + +`--since` is the closest manual equivalent to `roborev refine --since`. +`--branch` reviews the current branch relative to its merge-base. + +**Note:** `--wait` exits with code 1 when the verdict is Fail. This is +expected. Always capture the command output regardless of exit code and inspect +it to determine pass vs fail. + +When the review completes, read and parse the output. Extract the job ID from +the `Enqueued job for ...` line or the review header — you will need it +for commenting and closing later. + +**Panels:** if a `default_panel` is configured (or the review otherwise fans out +to a panel), the `Enqueued job ` is the synthesis (parent) job. Gate every +pass/fail decision on the parent's verdict, and comment on and close that parent +— never an individual member job. A re-review re-runs the same panel. + +If the command output contains an error (daemon not running, repo not +initialized, review errored), report it. Suggest `roborev status` to check the +daemon or `roborev init` if the repo is not initialized. + +If the review **passed**, inform the user and stop. No fixes needed. + +### 3. Fix-review loop + +If the review **failed**, begin the iterative loop. For each iteration +(up to `--max-iterations`): + +#### 3a. Fix the findings + +Parse findings from the review output. Collect every finding with its severity, +file path, and line number. Then: + +1. **Sort by severity**: fix HIGH findings first, then MEDIUM, then LOW +2. **Group by file**: within each severity level, batch edits to the same file + to minimize context switches +3. If some findings cannot be fixed (false positives, intentional design), note + them for the comment rather than silently skipping them + +If a finding's context is unclear, read the relevant source files to understand +the code before making changes. + +#### 3b. Run tests + +Run the project's test suite to verify the fixes: + +```bash +go test ./... +``` + +Or whatever test command the project uses. If tests fail, fix the regressions +before proceeding. + +#### 3c. Commit, then record comment and close review + +Commit first per the project's conventions (see AGENTS.md). Only after the +commit succeeds, record a summary comment on the review and close it: + +```bash +roborev comment --commenter roborev-refine --job -m "$(cat <<'ROBOREV_COMMENT' + +ROBOREV_COMMENT +)" +# Only if the comment above succeeded: +roborev close +``` + +For a panel review, `` is the synthesis parent; closing it closes the +review — do not close individual member jobs. + +**Important:** Always pass the comment text via a heredoc as shown above, never +by interpolating dynamic text directly into a shell string. Review-derived +content may contain shell metacharacters that could cause unintended execution. + +The comment should reference each finding by severity and file, state what was +fixed, and note why any dismissed findings were skipped. These comments are +included in re-review prompts. Keep it concise. + +#### 3d. Re-review + +After committing, run an explicit full-scope re-review that matches the +original refine scope. Do not treat a passing `roborev wait` result for the new +commit as sufficient to stop — the full branch or commit-range review must pass +before you report success. + +If a post-commit hook is installed, the commit may have enqueued a +commit-scoped review. Check for it so you can clean it up: + +```bash +roborev wait +``` + +If `roborev wait` finds a job, remember its job ID (from the output) as the +hook review job. If it reports "No job found", continue without one. + +**Note:** `roborev wait` resolves HEAD to a single SHA, so it only finds +commit-scoped hook reviews. Branch-mode hook reviews (stored under range refs) +are not discoverable this way — they will be superseded by the explicit review +below. + +Now run the explicit full-scope review. If refining with `--since`: + +```bash +read -r since <<'ROBOREV_REF' + +ROBOREV_REF +resolved_since=$(git rev-parse --verify -- "$since^{commit}") || exit 1 +git merge-base --is-ancestor "$resolved_since" HEAD || exit 1 +roborev review --since "$since" --wait +``` + +If refining without `--since`: + +```bash +roborev review --branch --wait +``` + +**Retrieving the job ID:** extract it from the +`Enqueued job for ...` line in the review command output. + +If you found a hook review job earlier, close it now so refine does not leave +stale commit-level reviews open: + +```bash +roborev close +``` + +- If the explicit full-scope review **passed**: inform the user and stop. The + branch or requested commit range is clean. +- If the review **failed**: continue to the next iteration (back to step 3a) + using the new job ID. + +### 4. Iteration limit reached + +If the maximum iterations are exhausted and the explicit full-scope review +still fails, inform the user how many iterations were completed, what findings +remain, and suggest they review the remaining findings manually or run +`/roborev-fix` for a targeted pass. + +## Examples + +**Default refine on a feature branch:** + +User: `/roborev-refine` + +Agent: +1. Validates that the current branch is not the default branch +2. Runs `roborev review --branch --wait` +3. Review returns verdict Fail with 2 findings +4. Fixes both findings in code +5. Runs `go test ./...` — passes +6. Commits changes +7. Records comment and closes the old review +8. Runs `roborev wait` — if hook review found, remembers job ID to close later +9. Runs `roborev review --branch --wait` +10. Closes hook review job if one was found +11. Full branch review returns Pass +12. Tells user: "Branch review passed after 1 fix iteration. All findings resolved." + +**Refine from a specific starting commit:** + +User: `/roborev-refine --since abc123 --max-iterations 3` + +Agent: +1. Validates `abc123` resolves and is an ancestor of `HEAD` +2. Runs `roborev review --since abc123 --wait` +3. Review returns verdict Fail +4. Fixes findings, tests, commits, comments, closes +5. Checks for hook review via `roborev wait` — if a commit-scoped hook review is found, remembers it to close after the next explicit `roborev review --since abc123 --wait` +6. Continues until the full requested range passes or 3 iterations are exhausted + +## See also + +- `/roborev-review-branch` — review without fixing +- `/roborev-fix` — single-pass fix without re-review +- `/roborev-respond` — comment on a review and close it without fixing code diff --git a/internal/skills/droid/roborev-respond/SKILL.md b/internal/skills/droid/roborev-respond/SKILL.md new file mode 100644 index 000000000..04311f074 --- /dev/null +++ b/internal/skills/droid/roborev-respond/SKILL.md @@ -0,0 +1,95 @@ +--- +name: roborev-respond +description: Add a comment to a roborev code review and close it +--- + +# roborev-respond + +Record a comment on a roborev code review and close it. + +## Usage + +``` +/roborev-respond [message] +``` + +## IMPORTANT + +This skill requires you to **execute bash commands** to record the comment and close the review. The task is not complete until you run both commands and see confirmation output. + +These instructions are guidelines, not a rigid script. Use the conversation +context. Skip steps that are already satisfied. Defer to project-level +AGENTS.md instructions when they conflict with these steps. + +## Instructions + +When the user invokes `/roborev-respond [message]`: + +### 1. Validate input + +If no job_id is provided, inform the user that a job ID is required. Suggest `roborev status` or `roborev fix --list` to find job IDs. Discovery surfaces synthesis parents (and non-panel reviews), never individual panel members, so the job ID you comment on and close is the parent. + +If a job_id is provided, inspect it before closing: + +```bash +roborev show --job --json +``` + +If `job.panel_role` is `"member"`, do **not** comment on or close that job. +Resolve the synthesis parent for the same `job.panel_run_uuid` if it is already +known from the conversation or discovery output; otherwise ask the user for the +synthesis parent ID. Only continue once the resolved job ID is a synthesis +parent or a non-panel review. + +### 2. Record the comment and close the review + +**If a message is provided**, immediately execute: +```bash +roborev comment --job "" && roborev close +``` + +If the message contains quotes or special characters, escape them properly in the bash command. + +**If no message is provided**, ask the user what they'd like to say, then execute the commands with their comment. + +### 3. Verify success + +Both commands will output confirmation. If either fails, report the error to the user. Common causes: +- The daemon is not running +- The job ID does not exist +- The repo is not initialized (suggest `roborev init`) +- The review is already closed (not an error, but worth noting to the user) + +The comment is recorded in roborev's database and the review is closed. View results with `roborev show`. + +## Examples + +**With message provided:** + +User: `/roborev-respond 1019 Fixed all issues` + +Agent action: +```bash +roborev comment --job 1019 "Fixed all issues" && roborev close 1019 +``` +Then confirm: "Comment recorded and review #1019 closed." + +--- + +**Without message:** + +User: `/roborev-respond 1019` + +Agent: "What would you like to say about review #1019?" + +User: "The null check was a false positive" + +Agent action: +```bash +roborev comment --job 1019 "The null check was a false positive" && roborev close 1019 +``` +Then confirm: "Comment recorded and review #1019 closed." + +## See also + +- `/roborev-fix` — fix a review's findings in code, then comment and close it diff --git a/internal/skills/droid/roborev-review-branch/SKILL.md b/internal/skills/droid/roborev-review-branch/SKILL.md new file mode 100644 index 000000000..4d47ff481 --- /dev/null +++ b/internal/skills/droid/roborev-review-branch/SKILL.md @@ -0,0 +1,123 @@ +--- +name: roborev-review-branch +description: Request a code review for all commits on the current branch and present the results +--- + +# roborev-review-branch + +Request a code review for all commits on the current branch and present the results. + +## Usage + +``` +/roborev-review-branch [--base ] [--type security|design] [--panel |none] +``` + +## When NOT to invoke this skill + +Do NOT invoke this skill when the user is presenting or pasting existing review +results. Messages that contain review findings, verdicts, or summaries are +outputs — not requests to start a new review. + +## IMPORTANT + +This skill requires you to **execute bash commands** to validate inputs and run the review. The task is not complete until the review finishes and you present the results to the user. + +These instructions are guidelines, not a rigid script. Use the conversation +context. Skip steps that are already satisfied. Defer to project-level +AGENTS.md instructions when they conflict with these steps. + +## Instructions + +When the user invokes `/roborev-review-branch [--base ] [--type security|design] [--panel |none]`: + +### 1. Validate inputs + +If a base branch is provided, use the base-branch command snippet below; it stores and validates the ref before invoking `roborev review`. + +If validation fails, inform the user the ref is invalid. Do not proceed. + +### 2. Build and run the command + +Construct and execute the review command: + +If no base branch is specified, run: + +```bash +roborev review --branch --wait [--type ] [--panel |none] +``` + +If a base branch is specified, run: + +```bash +read -r branch <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$branch" || exit 1 +roborev review --branch --wait --base "$branch" [--type ] [--panel |none] +``` + +- If `--base` is specified, include it (otherwise auto-detects the base branch) +- If `--type` is specified, include it +- If `--panel ` is specified, include it (fans out to the named config panel); `--panel none` forces a single-agent review + +The `--wait` flag blocks until the review completes. + +### 3. Present the results + +If the command output contains an error (e.g., daemon not running, repo not initialized, review errored), report it to the user. Suggest `roborev status` to check the daemon, `roborev init` if the repo is not initialized, or re-running the review. + +Otherwise, present the review to the user: +- Show the verdict prominently (Pass or Fail) +- If there are findings, list them grouped by severity with file paths and line numbers so the user can navigate directly +- If the review passed, a brief confirmation is sufficient + +#### Panels (multi-reviewer reviews) + +If you pass `--panel `, or a `default_panel` is configured for explicit +reviews, the review fans out to a panel of reviewers. In that case the +`Enqueued job ` is the **synthesis (parent)** job that aggregates them, and +its verdict and findings are the synthesized result across the whole panel. +Present that synthesized verdict/findings, and offer fix on that parent id — +never an individual reviewer. `roborev show` prints a one-line reviewers summary +(e.g. `3 reviewers: bug P, security F`) for a synthesis job. `--panel none` +forces a single-agent review, and automatic post-commit hook reviews stay +single-agent regardless of `default_panel`. + +### 4. Offer next steps + +If the review has findings (verdict is Fail), offer to address them: + +- "Would you like me to fix these findings? You can run `/roborev-fix `" + +Extract the job ID from the review output to include in the suggestion. Look for it in the `Enqueued job for ...` line or in the review header. For a panel review this id is the synthesis parent. + +If the review passed, confirm the result and do not offer `/roborev-fix`. + +## Examples + +**Default branch review:** + +User: `/roborev-review-branch` + +Agent: +1. Executes `roborev review --branch --wait` +2. Presents the verdict and findings grouped by severity +3. If findings exist: "Would you like me to address these findings? Run `/roborev-fix 1042`" +4. If passed: "Branch review passed with no findings." + +**Security review against a specific base:** + +User: `/roborev-review-branch --base develop --type security` + +Agent: +1. Validates: `git rev-parse --verify -- "develop"` +2. Executes `roborev review --branch --wait --base develop --type security` +3. Presents the verdict and findings +4. If findings exist: "Would you like me to address these findings? Run `/roborev-fix 1043`" + +## See also + +- `/roborev-design-review-branch` — shorthand for `/roborev-review-branch --type design` +- `/roborev-fix` — fix a review's findings in code +- `/roborev-review` — review a single commit diff --git a/internal/skills/droid/roborev-review/SKILL.md b/internal/skills/droid/roborev-review/SKILL.md new file mode 100644 index 000000000..4caee5659 --- /dev/null +++ b/internal/skills/droid/roborev-review/SKILL.md @@ -0,0 +1,122 @@ +--- +name: roborev-review +description: Request a code review for a commit and present the results +--- + +# roborev-review + +Request a code review for a commit and present the results. + +## Usage + +``` +/roborev-review [commit] [--type security|design] [--panel |none] +``` + +## When NOT to invoke this skill + +Do NOT invoke this skill when the user is presenting or pasting existing review +results. Messages that contain review findings, verdicts, or summaries are +outputs — not requests to start a new review. + +## IMPORTANT + +This skill requires you to **execute bash commands** to validate the commit and run the review. The task is not complete until the review finishes and you present the results to the user. + +These instructions are guidelines, not a rigid script. Use the conversation +context. Skip steps that are already satisfied. Defer to project-level +AGENTS.md instructions when they conflict with these steps. + +## Instructions + +When the user invokes `/roborev-review [commit] [--type security|design] [--panel |none]`: + +### 1. Validate inputs + +If a commit ref is provided, use the commit-provided command snippet below; it stores and validates the ref before invoking `roborev review`. + +If validation fails, inform the user the ref is invalid. Do not proceed. + +### 2. Build and run the command + +Construct and execute the review command: + +If no commit is specified, run: + +```bash +roborev review --wait [--type ] [--panel |none] +``` + +If a commit is specified, run: + +```bash +read -r commit <<'ROBOREV_REF' + +ROBOREV_REF +git rev-parse --verify -- "$commit^{commit}" || exit 1 +roborev review "$commit" --wait [--type ] [--panel |none] +``` + +- If `--type` is specified, include it +- If `--panel ` is specified, include it (fans out to the named config panel); `--panel none` forces a single-agent review + +The `--wait` flag blocks until the review completes. + +### 3. Present the results + +If the command output contains an error (e.g., daemon not running, repo not initialized, review errored), report it to the user. Suggest `roborev status` to check the daemon, `roborev init` if the repo is not initialized, or re-running the review. + +Otherwise, present the review to the user: +- Show the verdict prominently (Pass or Fail) +- If there are findings, list them grouped by severity with file paths and line numbers so the user can navigate directly +- If the review passed, a brief confirmation is sufficient + +#### Panels (multi-reviewer reviews) + +If you pass `--panel `, or a `default_panel` is configured for explicit +reviews, the review fans out to a panel of reviewers. In that case the +`Enqueued job ` is the **synthesis (parent)** job that aggregates them, and +its verdict and findings are the synthesized result across the whole panel. +Present that synthesized verdict/findings, and offer fix on that parent id — +never an individual reviewer. `roborev show` prints a one-line reviewers summary +(e.g. `3 reviewers: bug P, security F`) for a synthesis job. `--panel none` +forces a single-agent review, and automatic post-commit hook reviews stay +single-agent regardless of `default_panel`. + +### 4. Offer next steps + +If the review has findings (verdict is Fail), offer to address them: + +- "Would you like me to fix these findings? You can run `/roborev-fix `" + +Extract the job ID from the review output to include in the suggestion. Look for it in the `Enqueued job for ...` line or in the review header. For a panel review this id is the synthesis parent. + +If the review passed, confirm the result and do not offer `/roborev-fix`. + +## Examples + +**Default review of HEAD:** + +User: `/roborev-review` + +Agent: +1. Executes `roborev review --wait` +2. Presents the verdict and findings grouped by severity +3. If findings exist: "Would you like me to address these findings? Run `/roborev-fix 1042`" +4. If passed: "Review passed with no findings." + +**Security review of a specific commit:** + +User: `/roborev-review abc123 --type security` + +Agent: +1. Validates: `git rev-parse --verify -- "abc123^{commit}"` +2. Executes `roborev review abc123 --wait --type security` +3. Presents the verdict and findings +4. If findings exist: "Would you like me to address these findings? Run `/roborev-fix 1043`" + +## See also + +- `/roborev-design-review` — shorthand for `/roborev-review --type design` +- `/roborev-fix` — fix a review's findings in code +- `/roborev-review-branch` — review all commits on the current branch diff --git a/internal/skills/skills.go b/internal/skills/skills.go index 64ce0145a..68f3bc2ba 100644 --- a/internal/skills/skills.go +++ b/internal/skills/skills.go @@ -1,5 +1,5 @@ -// Package skills provides embedded skill files for AI agents (Claude Code, Codex) -// and installation utilities. +// Package skills provides embedded skill files for AI agents (Claude Code, Codex, +// Factory Droid) and installation utilities. package skills import ( @@ -21,12 +21,16 @@ var claudeSkills embed.FS //go:embed codex/*/SKILL.md var codexSkills embed.FS +//go:embed droid/*/SKILL.md +var droidSkills embed.FS + // Agent represents a supported AI agent type Agent string const ( AgentClaude Agent = "claude" AgentCodex Agent = "codex" + AgentDroid Agent = "droid" ) type agentSpec struct { @@ -46,8 +50,11 @@ type embeddedSkill struct { var supportedAgents = []agentSpec{ {agent: AgentClaude, configDirName: ".claude", embedFS: claudeSkills, embedDir: "claude"}, {agent: AgentCodex, configDirName: ".codex", embedFS: codexSkills, embedDir: "codex"}, + {agent: AgentDroid, configDirName: ".factory", embedFS: droidSkills, embedDir: "droid"}, } +var userHomeDir = os.UserHomeDir + // InstallResult contains the result of a skill installation type InstallResult struct { Agent Agent @@ -72,13 +79,13 @@ func Install() ([]InstallResult, error) { // IsInstalled checks if any roborev skills are installed for the given agent func IsInstalled(agent Agent) bool { - home, err := os.UserHomeDir() - if err != nil { + spec, ok := lookupAgent(agent) + if !ok { return false } - spec, ok := lookupAgent(agent) - if !ok { + home, err := homeDirForAgent(spec) + if err != nil { return false } @@ -119,6 +126,15 @@ func agentSkillsDir(home string, spec agentSpec) string { return filepath.Join(agentConfigDir(home, spec), "skills") } +func homeDirForAgent(spec agentSpec) (string, error) { + if spec.agent == AgentDroid { + if home := os.Getenv("HOME"); home != "" { + return home, nil + } + } + return userHomeDir() +} + func skillInstallPath(skillsDir, skillName string) string { return filepath.Join(skillsDir, skillName, "SKILL.md") } @@ -207,13 +223,12 @@ func installedSkillFilePaths(home string, spec agentSpec) ([]string, error) { // Update updates skills for agents that already have them installed // and removes legacy skills that are no longer shipped. func Update() ([]InstallResult, error) { - home, err := os.UserHomeDir() - if err != nil { - return nil, fmt.Errorf("get home dir: %w", err) - } - var results []InstallResult for _, spec := range supportedAgents { + home, err := homeDirForAgent(spec) + if err != nil { + return nil, fmt.Errorf("get home dir: %w", err) + } installed, err := installedSkillFilePaths(home, spec) if err != nil { return nil, fmt.Errorf("update %s skills: %w", spec.agent, err) @@ -235,7 +250,7 @@ func Update() ([]InstallResult, error) { // removeLegacySkills deletes skill directories that are no longer // embedded in the binary. func removeLegacySkills(spec agentSpec) error { - home, err := os.UserHomeDir() + home, err := homeDirForAgent(spec) if err != nil { return fmt.Errorf("get home dir: %w", err) } @@ -253,7 +268,7 @@ func removeLegacySkills(spec agentSpec) error { func installAgent(spec agentSpec) (InstallResult, error) { result := InstallResult{Agent: spec.agent} - home, err := os.UserHomeDir() + home, err := homeDirForAgent(spec) if err != nil { return result, fmt.Errorf("get home dir: %w", err) } @@ -304,9 +319,10 @@ func installAgent(spec agentSpec) (InstallResult, error) { // SkillInfo describes an available skill. type SkillInfo struct { - DirName string // e.g. "roborev-fix" - Name string // e.g. "roborev-fix" - Description string + DirName string // e.g. "roborev-fix" + Name string // e.g. "roborev-fix" + Description string + SupportedAgents []Agent } // SkillState describes whether a skill is installed and up to date for an agent. @@ -329,7 +345,7 @@ type AgentStatus struct { // directory name. When the same skill exists across multiple agents, the // first agent's metadata is used. func ListSkills() ([]SkillInfo, error) { - seen := make(map[string]bool) + seen := make(map[string]int) var out []SkillInfo for _, spec := range supportedAgents { skills, err := embeddedSkillsForAgent(spec) @@ -337,14 +353,18 @@ func ListSkills() ([]SkillInfo, error) { return nil, err } for _, skill := range skills { - if seen[skill.DirName] { + if idx, ok := seen[skill.DirName]; ok { + if !slices.Contains(out[idx].SupportedAgents, spec.agent) { + out[idx].SupportedAgents = append(out[idx].SupportedAgents, spec.agent) + } continue } - seen[skill.DirName] = true + seen[skill.DirName] = len(out) out = append(out, SkillInfo{ - DirName: skill.DirName, - Name: skill.Name, - Description: skill.Description, + DirName: skill.DirName, + Name: skill.Name, + Description: skill.Description, + SupportedAgents: []Agent{spec.agent}, }) } } @@ -353,13 +373,12 @@ func ListSkills() ([]SkillInfo, error) { // Status returns per-agent, per-skill installation state. func Status() []AgentStatus { - home, err := os.UserHomeDir() - if err != nil { - return nil - } - var out []AgentStatus for _, spec := range supportedAgents { + home, err := homeDirForAgent(spec) + if err != nil { + return nil + } status := AgentStatus{ Agent: spec.agent, Skills: make(map[string]SkillState), diff --git a/internal/skills/skills_test.go b/internal/skills/skills_test.go index 3e7a2726e..953ea920d 100644 --- a/internal/skills/skills_test.go +++ b/internal/skills/skills_test.go @@ -3,6 +3,7 @@ package skills import ( "os" "path/filepath" + "strings" "testing" "testing/fstest" @@ -20,6 +21,7 @@ type agentCase struct { var agentCases = []agentCase{ {agent: AgentClaude, configDir: ".claude", legacyDir: ".claude", displayName: string(AgentClaude)}, {agent: AgentCodex, configDir: ".codex", legacyDir: ".codex", displayName: string(AgentCodex)}, + {agent: AgentDroid, configDir: ".factory", legacyDir: ".factory", displayName: string(AgentDroid)}, } func setupTestEnv(t *testing.T) string { @@ -36,20 +38,20 @@ func setupTestEnv(t *testing.T) string { func createMockSkill(t *testing.T, homeDir string, agent Agent, skill string) { t.Helper() - dir := filepath.Join(homeDir, "."+string(agent), "skills", skill) + spec, ok := lookupAgent(agent) + require.True(t, ok, "unsupported agent %s", agent) + dir := filepath.Join(homeDir, spec.configDirName, "skills", skill) require.NoError(t, os.MkdirAll(dir, 0o755)) require.NoError(t, os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte("old"), 0o644)) } -func expectedSkillDirNames(t *testing.T) []string { +func expectedSkillDirNamesForAgent(t *testing.T, agent Agent) []string { t.Helper() - skills, err := ListSkills() - require.NoError(t, err) + spec, ok := lookupAgent(agent) + require.True(t, ok, "unsupported agent %s", agent) - names := make([]string, 0, len(skills)) - for _, skill := range skills { - names = append(names, skill.DirName) - } + names, err := embeddedSkillDirNames(spec) + require.NoError(t, err) return names } @@ -82,7 +84,7 @@ func assertSkillsInstalled(t *testing.T, homeDir string, tc agentCase) { t.Helper() skillsDir := filepath.Join(homeDir, tc.configDir, "skills") - for _, skill := range expectedSkillDirNames(t) { + for _, skill := range expectedSkillDirNamesForAgent(t, tc.agent) { path := filepath.Join(skillsDir, skill, "SKILL.md") _, err := os.Stat(path) require.NoError(t, err, "expected %s to exist", path) @@ -101,10 +103,9 @@ func TestInstallClaudeSkipsWhenDirMissing(t *testing.T) { } func TestInstallWhenDirExists(t *testing.T) { - expectedSkills := expectedSkillDirNames(t) - for _, tc := range agentCases { t.Run(tc.displayName, func(t *testing.T) { + expectedSkills := expectedSkillDirNamesForAgent(t, tc.agent) tmpHome := setupTestEnv(t) agentDir := filepath.Join(tmpHome, tc.configDir) require.NoError(t, os.MkdirAll(agentDir, 0o755)) @@ -129,7 +130,7 @@ func TestInstallIdempotent(t *testing.T) { results1, err := Install() require.NoError(t, err, "First install failed: %v", err) - expectedSkills := expectedSkillDirNames(t) + expectedSkills := expectedSkillDirNamesForAgent(t, AgentClaude) claude1 := findResultByAgent(t, results1, AgentClaude) require.Len(t, claude1.Installed, len(expectedSkills), "first install: expected %d installed, got %d", len(expectedSkills), len(claude1.Installed)) @@ -184,22 +185,17 @@ func TestIsInstalled(t *testing.T) { }, } - expectedSkills := expectedSkillDirNames(t) - for _, skill := range expectedSkills { - - s := skill - tests = append(tests, testCase{ - name: "Claude with skill " + s, - agent: AgentClaude, - setup: func(t *testing.T, h string) { createMockSkill(t, h, AgentClaude, s) }, - shouldExist: true, - }) - tests = append(tests, testCase{ - name: "Codex with skill " + s, - agent: AgentCodex, - setup: func(t *testing.T, h string) { createMockSkill(t, h, AgentCodex, s) }, - shouldExist: true, - }) + for _, tc := range agentCases { + for _, skill := range expectedSkillDirNamesForAgent(t, tc.agent) { + s := skill + agent := tc.agent + tests = append(tests, testCase{ + name: tc.displayName + " with skill " + s, + agent: agent, + setup: func(t *testing.T, h string) { createMockSkill(t, h, agent, s) }, + shouldExist: true, + }) + } } tests = append(tests, testCase{ @@ -262,10 +258,9 @@ func TestUpdateRemovesLegacySkills(t *testing.T) { } func TestUpdateLegacyOnlyInstall(t *testing.T) { - expectedSkills := expectedSkillDirNames(t) - for _, tc := range agentCases { t.Run(tc.displayName, func(t *testing.T) { + expectedSkills := expectedSkillDirNamesForAgent(t, tc.agent) tmpHome := setupTestEnv(t) // User only has the deprecated skill — no current skills @@ -287,7 +282,7 @@ func TestUpdateLegacyOnlyInstall(t *testing.T) { } func TestUpdateOnlyUpdatesInstalled(t *testing.T) { - expectedSkillCount := len(expectedSkillDirNames(t)) + expectedSkillCount := len(expectedSkillDirNamesForAgent(t, AgentClaude)) tests := []struct { name string @@ -431,6 +426,26 @@ func TestListSkillsUsesFirstAgentMetadata(t *testing.T) { } } +func TestListSkillsReportsSupportedAgents(t *testing.T) { + skills, err := ListSkills() + require.NoError(t, err) + + skillsByDir := make(map[string]SkillInfo, len(skills)) + for _, skill := range skills { + skillsByDir[skill.DirName] = skill + } + + assert.ElementsMatch(t, + []Agent{AgentClaude, AgentCodex, AgentDroid}, + skillsByDir["roborev-review"].SupportedAgents) + assert.ElementsMatch(t, + []Agent{AgentClaude, AgentCodex, AgentDroid}, + skillsByDir["roborev-lookahead-review"].SupportedAgents) + assert.ElementsMatch(t, + []Agent{AgentClaude, AgentCodex, AgentDroid}, + skillsByDir["roborev-lookahead-review-branch"].SupportedAgents) +} + func TestDirNameEnumerationDoesNotReadContent(t *testing.T) { // embeddedSkillDirNames only enumerates directories, so it must // succeed even when SKILL.md files are absent. This guards against @@ -467,3 +482,126 @@ func TestDirNameEnumerationDoesNotReadContent(t *testing.T) { "path should end with SKILL.md: %s", p) } } + +func TestDroidSkillsUseDroidAdaptations(t *testing.T) { + // Droid skills are derived from the Codex skills (agent-agnostic, synchronous + // --wait, no Claude-specific Task tool) with two Factory-specific + // adaptations: slash invocation (/roborev-X, matching Factory's /skill-name + // convention) and AGENTS.md (Factory's project-instructions file) instead of + // the Codex $roborev-X and CLAUDE.md forms. + spec, ok := lookupAgent(AgentDroid) + require.True(t, ok) + skills, err := embeddedSkillsForAgent(spec) + require.NoError(t, err) + require.NotEmpty(t, skills) + for _, s := range skills { + content := string(s.Content) + assert.NotContains(t, content, "$roborev", "droid skill %s must use /roborev slash invocation, not $roborev", s.DirName) + assert.NotContains(t, content, "CLAUDE.md", "droid skill %s must reference AGENTS.md, not CLAUDE.md", s.DirName) + assert.Contains(t, content, "AGENTS.md", "droid skill %s should reference AGENTS.md", s.DirName) + assert.Contains(t, content, "/roborev-", "droid skill %s should use /roborev- slash invocation", s.DirName) + } +} + +func TestFixSkillsUseHeredocForCommentText(t *testing.T) { + for _, agent := range []Agent{AgentClaude, AgentCodex, AgentDroid} { + t.Run(string(agent), func(t *testing.T) { + spec, ok := lookupAgent(agent) + require.True(t, ok) + skills, err := embeddedSkillsForAgent(spec) + require.NoError(t, err) + + var content string + for _, skill := range skills { + if skill.DirName == "roborev-fix" { + content = strings.ReplaceAll(string(skill.Content), "\r\n", "\n") + } + } + require.NotEmpty(t, content, "missing roborev-fix skill for %s", agent) + assert.Contains(t, content, "cat <<'ROBOREV_COMMENT'") + assert.Contains(t, content, "never\nby interpolating dynamic text directly into a shell string") + assert.NotContains(t, content, `""`) + assert.NotContains(t, content, "Escape quotes and special characters in the bash command") + assert.Equal(t, 0, strings.Count(content, `roborev comment --commenter roborev-fix --job 1019 "`)) + assert.Equal(t, 0, strings.Count(content, `roborev comment --commenter roborev-fix --job 1021 "`)) + }) + } +} + +func TestDroidSkillsInstallToFactoryDir(t *testing.T) { + // Droid skills install under ~/.factory/skills (Factory's personal skills + // location), not ~/.droid, and are skipped when ~/.factory is absent so the + // install stays opt-in for Factory users. + t.Run("installs under .factory when present", func(t *testing.T) { + tmpHome := setupTestEnv(t) + require.NoError(t, os.MkdirAll(filepath.Join(tmpHome, ".factory"), 0o755)) + + results, err := Install() + require.NoError(t, err) + res := findResultByAgent(t, results, AgentDroid) + assert.False(t, res.Skipped) + for _, name := range expectedSkillDirNamesForAgent(t, AgentDroid) { + _, err := os.Stat(filepath.Join(tmpHome, ".factory", "skills", name, "SKILL.md")) + require.NoError(t, err, "expected %s skill to install under .factory", name) + } + _, err = os.Stat(filepath.Join(tmpHome, ".droid")) + assert.True(t, os.IsNotExist(err), "no .droid dir should be created") + }) + + t.Run("skipped when .factory absent", func(t *testing.T) { + setupTestEnv(t) + results, err := Install() + require.NoError(t, err) + res := findResultByAgent(t, results, AgentDroid) + assert.True(t, res.Skipped, "Droid should be skipped when ~/.factory does not exist") + }) +} + +func TestDroidSkillOperationsUseHomeEnvWhenUserHomeDirDiffers(t *testing.T) { + envHome := t.TempDir() + userHome := t.TempDir() + t.Setenv("HOME", envHome) + stubUserHomeDir(t, userHome) + require.NoError(t, os.MkdirAll(filepath.Join(envHome, ".factory"), 0o755)) + + results, err := Install() + require.NoError(t, err) + droidInstall := findResultByAgent(t, results, AgentDroid) + require.False(t, droidInstall.Skipped, "Droid should use HOME for Factory config discovery") + assertSkillsInstalled(t, envHome, agentCase{ + agent: AgentDroid, + configDir: ".factory", + displayName: string(AgentDroid), + }) + _, err = os.Stat(filepath.Join(userHome, ".factory")) + require.ErrorIs(t, err, os.ErrNotExist) + + assert.True(t, IsInstalled(AgentDroid), "Droid installed detection should use HOME") + + updates, err := Update() + require.NoError(t, err) + droidUpdate := findResultByAgent(t, updates, AgentDroid) + assert.NotEmpty(t, droidUpdate.Updated, "Droid update should use HOME") + + var droidStatus AgentStatus + for _, status := range Status() { + if status.Agent == AgentDroid { + droidStatus = status + } + } + assert.True(t, droidStatus.Available, "Droid status should use HOME") + for _, name := range expectedSkillDirNamesForAgent(t, AgentDroid) { + assert.Equal(t, SkillCurrent, droidStatus.Skills[name]) + } +} + +func stubUserHomeDir(t *testing.T, home string) { + t.Helper() + old := userHomeDir + userHomeDir = func() (string, error) { + return home, nil + } + t.Cleanup(func() { + userHomeDir = old + }) +}