feat(review): add code review MCP skill with session history injection

Implements the review skill following the same pattern as retrospective/tdd.
Validates project_root and files args, prepends session history when a
session_id is provided, and delegates to the executor with Read,Bash tools.
Iron-law discipline prompt enforces CRITICAL/WARNING/SUGGESTION output format.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

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()

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: <issue> at <file>:<line>
+  WARNING: <issue> at <file>:<line>
+  SUGGESTION: <issue> at <file>:<line>
+  ```
+- `verified`: true if you read all specified files; false if any were missing or unreadable
+- `message`: "N critical, M warnings, K suggestions" or "clean: <which iron law checks passed and why>"
+
+## 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

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
+}

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)
+}

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,
+				},
+			),
+		},
+	}
+}