diff --git a/cmd/supervisor/main.go b/cmd/supervisor/main.go index 4d69c42..8749e06 100644 --- a/cmd/supervisor/main.go +++ b/cmd/supervisor/main.go @@ -13,6 +13,7 @@ import ( "github.com/mathiasbq/supervisor/internal/skills/brain" "github.com/mathiasbq/supervisor/internal/skills/org" "github.com/mathiasbq/supervisor/internal/skills/retrospective" + "github.com/mathiasbq/supervisor/internal/skills/review" "github.com/mathiasbq/supervisor/internal/skills/sessionlog" "github.com/mathiasbq/supervisor/internal/skills/tdd" "github.com/mathiasbq/supervisor/internal/tier" @@ -51,6 +52,12 @@ func main() { os.Exit(1) } + reviewPrompt, err := os.ReadFile(cfg.ConfigDir + "/review.md") + if err != nil { + logger.Error("read review.md", "path", cfg.ConfigDir+"/review.md", "err", err) + os.Exit(1) + } + executor := iexec.New(iexec.Config{ SystemPrompt: string(systemPrompt), LiteLLMBaseURL: cfg.LiteLLMBaseURL, @@ -84,6 +91,12 @@ func main() { SessionsDir: cfg.SessionsDir, ExecutorFn: executor.Run, })) + reg.Register(review.New(review.Config{ + SkillPrompt: string(reviewPrompt), + DefaultModel: models.Resolve("review", ""), + ExecutorFn: executor.Run, + SessionsDir: cfg.SessionsDir, + })) srv := mcp.NewServer(reg) mux := http.NewServeMux() diff --git a/config/supervisor/review.md b/config/supervisor/review.md new file mode 100644 index 0000000..453607c --- /dev/null +++ b/config/supervisor/review.md @@ -0,0 +1,30 @@ +# Code Review Discipline + +You are a disciplined code reviewer. Read files carefully before commenting. + +## Iron laws +1. Never approve security vulnerabilities: command injection, SQL injection, credential exposure, path traversal, unchecked input at system boundaries +2. Never approve silently swallowed errors — `err != nil` without wrapping or handling is always wrong +3. Never approve missing validation at system boundaries (user input, external APIs, file reads) + +## Output contract +Return JSON result with: +- `status`: "pass" if no blocking issues; "fail" if any iron law is violated +- `phase`: "review" +- `skill`: "review" +- `file_path`: first file reviewed +- `runner_output`: full review formatted as: + ``` + CRITICAL: at : + WARNING: at : + SUGGESTION: at : + ``` +- `verified`: true if you read all specified files; false if any were missing or unreadable +- `message`: "N critical, M warnings, K suggestions" or "clean: " + +## Rules +1. Read every file listed before writing feedback +2. Check iron laws first — any violation is CRITICAL and sets status to "fail" +3. Then check: correctness, test coverage for new code, Go style conventions +4. Never rubber-stamp — if nothing is wrong, explain specifically which iron law checks you ran and why they passed +5. Line references are required for every finding — "roughly around the middle" is not acceptable diff --git a/internal/skills/review/handlers.go b/internal/skills/review/handlers.go new file mode 100644 index 0000000..b5da04f --- /dev/null +++ b/internal/skills/review/handlers.go @@ -0,0 +1,81 @@ +// internal/skills/review/handlers.go +package review + +import ( + "context" + "encoding/json" + "fmt" + "strings" + + iexec "github.com/mathiasbq/supervisor/internal/exec" + "github.com/mathiasbq/supervisor/internal/session" +) + +type reviewArgs struct { + ProjectRoot string `json:"project_root"` + Files []string `json:"files"` + Context string `json:"context"` + Model string `json:"model"` + SessionID string `json:"session_id"` +} + +// Handle dispatches the MCP tool call to the appropriate handler. +func (s *Skill) Handle(ctx context.Context, tool string, args json.RawMessage) (json.RawMessage, error) { + if tool != "review" { + return nil, fmt.Errorf("unknown tool: %s", tool) + } + var a reviewArgs + if err := json.Unmarshal(args, &a); err != nil { + return nil, fmt.Errorf("parse args: %w", err) + } + if a.ProjectRoot == "" { + return nil, fmt.Errorf("project_root is required") + } + if len(a.Files) == 0 { + return nil, fmt.Errorf("files is required") + } + + model := a.Model + if model == "" { + model = s.cfg.DefaultModel + } + + task := fmt.Sprintf( + "phase: review\nproject_root: %s\nfiles: %s\ncontext: %s\nmodel: %s", + a.ProjectRoot, strings.Join(a.Files, ", "), a.Context, model, + ) + task = s.prependHistory(a.SessionID, "review", task) + + if s.cfg.ExecutorFn == nil { + return nil, fmt.Errorf("no executor configured") + } + result, err := s.cfg.ExecutorFn(ctx, iexec.Request{ + SkillPrompt: s.cfg.SkillPrompt, + TaskPrompt: task, + Model: model, + Tools: "Read,Bash", + }) + if err != nil { + return nil, err + } + b, err := json.Marshal(result) + if err != nil { + return nil, fmt.Errorf("marshal result: %w", err) + } + return b, nil +} + +func (s *Skill) prependHistory(sessionID, currentPhase, task string) string { + if sessionID == "" || s.cfg.SessionsDir == "" { + return task + } + entries, err := session.Read(s.cfg.SessionsDir, sessionID) + if err != nil || len(entries) == 0 { + return task + } + history := session.FormatHistory(entries, currentPhase) + if history == "" { + return task + } + return history + "\n---\n\n" + task +} diff --git a/internal/skills/review/handlers_test.go b/internal/skills/review/handlers_test.go new file mode 100644 index 0000000..2d32397 --- /dev/null +++ b/internal/skills/review/handlers_test.go @@ -0,0 +1,61 @@ +// internal/skills/review/handlers_test.go +package review_test + +import ( + "context" + "encoding/json" + "testing" + + iexec "github.com/mathiasbq/supervisor/internal/exec" + "github.com/mathiasbq/supervisor/internal/skills/review" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestReviewToolRegistered(t *testing.T) { + sk := review.New(review.Config{SkillPrompt: "review rules"}) + names := make([]string, 0) + for _, tool := range sk.Tools() { + names = append(names, tool.Name) + } + assert.Contains(t, names, "review") +} + +func TestReviewRequiresProjectRoot(t *testing.T) { + sk := review.New(review.Config{SkillPrompt: "r"}) + _, err := sk.Handle(context.Background(), "review", json.RawMessage(`{"files":["main.go"]}`)) + assert.ErrorContains(t, err, "project_root") +} + +func TestReviewRequiresFiles(t *testing.T) { + sk := review.New(review.Config{SkillPrompt: "r"}) + _, err := sk.Handle(context.Background(), "review", json.RawMessage(`{"project_root":"/tmp"}`)) + assert.ErrorContains(t, err, "files") +} + +func TestReviewCallsExecutor(t *testing.T) { + called := false + var capturedTask string + fakeFn := func(_ context.Context, req iexec.Request) (iexec.Result, error) { + called = true + capturedTask = req.TaskPrompt + return iexec.Result{ + Status: "pass", Phase: "review", Skill: "review", + Verified: true, ModelUsed: "self", Message: "2 warnings found", + }, nil + } + + sk := review.New(review.Config{SkillPrompt: "review rules", ExecutorFn: fakeFn, SessionsDir: t.TempDir()}) + out, err := sk.Handle(context.Background(), "review", json.RawMessage( + `{"project_root":"/tmp/proj","files":["internal/foo/foo.go"],"context":"PR: add Foo helper"}`, + )) + require.NoError(t, err) + assert.True(t, called) + assert.Contains(t, capturedTask, "internal/foo/foo.go") + assert.Contains(t, capturedTask, "PR: add Foo helper") + + var result iexec.Result + require.NoError(t, json.Unmarshal(out, &result)) + assert.Equal(t, "pass", result.Status) + assert.Equal(t, "review", result.Phase) +} diff --git a/internal/skills/review/skill.go b/internal/skills/review/skill.go new file mode 100644 index 0000000..25a6936 --- /dev/null +++ b/internal/skills/review/skill.go @@ -0,0 +1,55 @@ +// internal/skills/review/skill.go +package review + +import ( + "context" + "encoding/json" + + iexec "github.com/mathiasbq/supervisor/internal/exec" + "github.com/mathiasbq/supervisor/internal/registry" +) + +// ExecutorFn is the function signature for running a worker subprocess. +type ExecutorFn func(ctx context.Context, req iexec.Request) (iexec.Result, error) + +// Config holds dependencies for the review skill. +type Config struct { + SkillPrompt string + DefaultModel string + ExecutorFn ExecutorFn + SessionsDir string +} + +// Skill implements the review MCP tool. +type Skill struct{ cfg Config } + +// New creates a new review Skill. +func New(cfg Config) *Skill { return &Skill{cfg: cfg} } + +// Name returns the skill identifier. +func (s *Skill) Name() string { return "review" } + +// Tools returns the MCP tool definitions for this skill. +func (s *Skill) Tools() []registry.ToolDef { + schema := func(required []string, props map[string]any) json.RawMessage { + b, _ := json.Marshal(map[string]any{"type": "object", "required": required, "properties": props}) + return b + } + str := map[string]any{"type": "string"} + return []registry.ToolDef{ + { + Name: "review", + Description: "Perform a structured code review of the specified files. Returns findings with severity levels.", + InputSchema: schema( + []string{"project_root", "files"}, + map[string]any{ + "project_root": str, + "files": map[string]any{"type": "array", "items": map[string]any{"type": "string"}}, + "context": str, + "model": str, + "session_id": str, + }, + ), + }, + } +}