From 3c05c96859bf619a1ac93aa81104c49cfc25b2cc Mon Sep 17 00:00:00 2001 From: Doug Jarquin Date: Tue, 18 Aug 2026 10:47:15 -0400 Subject: [PATCH 1/9] feat(review): define Codex review contract --- internal/agent/agent.go | 9 +- internal/agent/agent_contract_test.go | 94 ++++++- internal/agent/agent_test.go | 10 - internal/agent/findings.go | 16 ++ internal/agent/remediation_contract_test.go | 73 +++++- internal/agent/review_schema_test.go | 40 +++ internal/agent/reviewcontract.go | 122 +++++++++ internal/agent/reviewcontract_test.go | 67 +++++ internal/agent/reviewworktree.go | 25 +- internal/agent/spawn.go | 155 ++++++++--- internal/agent/testdata/fakeagent/main.go | 26 +- internal/orchestrator/workfunc.go | 6 +- internal/pipeline/review/autofix.go | 196 ++++++++++++++ internal/pipeline/review/contract.go | 36 +++ internal/pipeline/review/evidence.go | 41 +++ .../pipeline/review/evidence_contract_test.go | 79 ++++++ internal/pipeline/review/review.go | 243 +++--------------- 17 files changed, 946 insertions(+), 292 deletions(-) create mode 100644 internal/agent/review_schema_test.go create mode 100644 internal/agent/reviewcontract.go create mode 100644 internal/agent/reviewcontract_test.go create mode 100644 internal/pipeline/review/autofix.go create mode 100644 internal/pipeline/review/contract.go create mode 100644 internal/pipeline/review/evidence.go create mode 100644 internal/pipeline/review/evidence_contract_test.go diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 010d513..52e7571 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -3,14 +3,15 @@ package agent type Kind string const ( - KindClaude Kind = "claude" - KindCodex Kind = "codex" + KindCodex Kind = "codex" ) +func SupportedKinds() []Kind { + return []Kind{KindCodex} +} + func (k Kind) binaryName() string { switch k { - case KindClaude: - return "claude" case KindCodex: return "codex" default: diff --git a/internal/agent/agent_contract_test.go b/internal/agent/agent_contract_test.go index 6e83961..69b09f2 100644 --- a/internal/agent/agent_contract_test.go +++ b/internal/agent/agent_contract_test.go @@ -24,6 +24,7 @@ func TestSpawn_CodexUsesStructuredExecContract(t *testing.T) { if _, err := agent.Spawn(context.Background(), agent.KindCodex, agent.SpawnParams{ WorktreePath: worktree, BinaryPath: bin, + Task: "inspect the candidate diff and return structured findings", ExtraEnv: []string{ "FAKE_AGENT_KIND=codex", "FAKE_AGENT_SCENARIO=" + scenarioPath, @@ -36,11 +37,14 @@ func TestSpawn_CodexUsesStructuredExecContract(t *testing.T) { if err != nil { t.Fatalf("read invocation log: %v", err) } - for _, token := range []string{"exec", "--json", "--output-schema", "--output-last-message", "--sandbox", "read-only", "--ephemeral", "-C"} { + for _, token := range []string{"exec", "--cd", "--json", "--output-schema", "-"} { if !strings.Contains(string(data), token) { t.Fatalf("expected Codex structured invocation token %q, got %s", token, data) } } + if !strings.Contains(string(data), "task=inspect the candidate diff and return structured findings") { + t.Fatalf("expected task on Codex stdin, got %s", data) + } } func TestSpawn_DoesNotPassSensitiveEnvironmentToCodex(t *testing.T) { @@ -71,7 +75,7 @@ func TestSpawn_DoesNotPassSensitiveEnvironmentToCodex(t *testing.T) { func TestSpawn_RejectsStructuredOutputWithoutFindingsField(t *testing.T) { bin := agenttest.Build(t) scenarioPath := filepath.Join(t.TempDir(), "invalid.json") - if err := os.WriteFile(scenarioPath, []byte(`{"unexpected":[]}`), 0o644); err != nil { + if err := os.WriteFile(scenarioPath, []byte(`{}`), 0o644); err != nil { t.Fatalf("write invalid scenario: %v", err) } @@ -88,6 +92,92 @@ func TestSpawn_RejectsStructuredOutputWithoutFindingsField(t *testing.T) { } } +func TestSpawn_RejectsTrailingStructuredOutput(t *testing.T) { + bin := agenttest.Build(t) + scenarioPath := filepath.Join(t.TempDir(), "trailing.json") + if err := os.WriteFile(scenarioPath, []byte(`{"findings":[]}{"findings":[]}`), 0o644); err != nil { + t.Fatalf("write trailing scenario: %v", err) + } + + _, err := agent.Spawn(context.Background(), agent.KindCodex, agent.SpawnParams{ + WorktreePath: agentWorktree(t), + BinaryPath: bin, + ExtraEnv: []string{ + "FAKE_AGENT_KIND=codex", + "FAKE_AGENT_SCENARIO=" + scenarioPath, + }, + }) + if err == nil { + t.Fatal("expected trailing structured output to fail closed") + } +} + +func TestSpawn_ParsesCodexJSONLEventResponse(t *testing.T) { + bin := agenttest.Build(t) + scenarioPath := filepath.Join(t.TempDir(), "events.jsonl") + events := "{\"type\":\"turn.started\"}\n" + + "{\"type\":\"item.completed\",\"item\":{\"type\":\"agent_message\",\"text\":\"{\\\"findings\\\":[]}\"}}\n" + + "{\"type\":\"turn.completed\"}\n" + if err := os.WriteFile(scenarioPath, []byte(events), 0o644); err != nil { + t.Fatalf("write JSONL scenario: %v", err) + } + + findings, err := agent.Spawn(context.Background(), agent.KindCodex, agent.SpawnParams{ + WorktreePath: agentWorktree(t), + BinaryPath: bin, + ExtraEnv: []string{ + "FAKE_AGENT_KIND=codex", + "FAKE_AGENT_SCENARIO=" + scenarioPath, + }, + }) + if err != nil { + t.Fatalf("Spawn: %v", err) + } + if len(findings.Findings) != 0 { + t.Fatalf("expected empty findings from Codex JSONL response, got %+v", findings) + } +} + +func TestSpawn_RejectsPatchOnNonAutoFixableFinding(t *testing.T) { + bin := agenttest.Build(t) + scenarioPath := filepath.Join(t.TempDir(), "invalid-finding.json") + if err := os.WriteFile(scenarioPath, []byte(`{"findings":[{"kind":"ask-user","description":"needs a decision","patch":"diff --git a/x b/x","paths":["x"]}]}`), 0o644); err != nil { + t.Fatalf("write invalid finding scenario: %v", err) + } + + _, err := agent.Spawn(context.Background(), agent.KindCodex, agent.SpawnParams{ + WorktreePath: agentWorktree(t), + BinaryPath: bin, + ExtraEnv: []string{ + "FAKE_AGENT_KIND=codex", + "FAKE_AGENT_SCENARIO=" + scenarioPath, + }, + }) + if err == nil { + t.Fatal("expected non-auto-fixable patch to fail closed") + } +} + +func TestSpawn_RejectsUnknownFindingKind(t *testing.T) { + bin := agenttest.Build(t) + scenarioPath := filepath.Join(t.TempDir(), "unknown-kind.json") + if err := os.WriteFile(scenarioPath, []byte(`{"findings":[{"kind":"style","description":"style advice"}]}`), 0o644); err != nil { + t.Fatalf("write unknown-kind scenario: %v", err) + } + + _, err := agent.Spawn(context.Background(), agent.KindCodex, agent.SpawnParams{ + WorktreePath: agentWorktree(t), + BinaryPath: bin, + ExtraEnv: []string{ + "FAKE_AGENT_KIND=codex", + "FAKE_AGENT_SCENARIO=" + scenarioPath, + }, + }) + if err == nil { + t.Fatal("expected unknown finding kind to fail closed") + } +} + func TestFindingsJSONRoundTripUsesArrayShape(t *testing.T) { data, err := json.Marshal(agent.Findings{Findings: []agent.Finding{}}) if err != nil { diff --git a/internal/agent/agent_test.go b/internal/agent/agent_test.go index 258046f..b321a30 100644 --- a/internal/agent/agent_test.go +++ b/internal/agent/agent_test.go @@ -121,13 +121,3 @@ func TestSpawn_LogsInvocation(t *testing.T) { t.Fatalf("expected invocation log entry, got %q", data) } } - -func TestSpawn_RejectsUnsupportedClaudeContract(t *testing.T) { - _, err := agent.Spawn(context.Background(), agent.KindClaude, agent.SpawnParams{ - WorktreePath: agentWorktree(t), - BinaryPath: agenttest.Build(t), - }) - if err == nil || !strings.Contains(err.Error(), "structured task contract is unsupported") { - t.Fatalf("expected explicit unsupported Claude error, got %v", err) - } -} diff --git a/internal/agent/findings.go b/internal/agent/findings.go index 3493415..918f96e 100644 --- a/internal/agent/findings.go +++ b/internal/agent/findings.go @@ -50,6 +50,22 @@ type Findings struct { Findings []Finding `json:"findings"` } +func (f *Findings) UnmarshalJSON(data []byte) error { + var wire struct { + Findings *[]Finding `json:"findings"` + } + decoder := json.NewDecoder(bytes.NewReader(data)) + decoder.DisallowUnknownFields() + if err := decoder.Decode(&wire); err != nil { + return err + } + if wire.Findings == nil { + return fmt.Errorf("structured output requires findings") + } + f.Findings = append([]Finding(nil), (*wire.Findings)...) + return nil +} + func (f Findings) MarshalJSON() ([]byte, error) { findings := f.Findings if findings == nil { diff --git a/internal/agent/remediation_contract_test.go b/internal/agent/remediation_contract_test.go index 0ad6e5a..c681731 100644 --- a/internal/agent/remediation_contract_test.go +++ b/internal/agent/remediation_contract_test.go @@ -37,19 +37,15 @@ func TestSpawn_CodexUsesSupportedExecStructuredContract(t *testing.T) { "set -eu", "printf '%s\\n' \"$@\" > \"$FAKE_AGENT_LOG_FILE\"", "[ \"$1\" = \"exec\" ]", - "[ \"$2\" = \"--json\" ]", - "[ \"$3\" = \"--output-schema\" ]", - "[ -f \"$4\" ]", - "[ \"$5\" = \"--output-last-message\" ]", - "[ \"$7\" = \"--sandbox\" ]", - "[ \"$8\" = \"read-only\" ]", - "[ \"$9\" = \"--ephemeral\" ]", - "[ \"${10}\" = \"-C\" ]", - "[ -d \"${11}\" ]", - "[ \"$(git -C \"${11}\" rev-parse HEAD)\" = " + shellQuote(head) + " ]", - "if (umask 077; : > \"${11}/.agent-write-probe\") 2>/dev/null; then exit 1; fi", + "[ \"$2\" = \"--cd\" ]", + "[ -d \"$3\" ]", + "[ \"$4\" = \"--json\" ]", + "[ \"$5\" = \"--output-schema\" ]", + "[ -f \"$6\" ]", + "[ \"$7\" = \"-\" ]", + "[ \"$(git -C \"$3\" rev-parse HEAD)\" = " + shellQuote(head) + " ]", + "if (umask 077; : > \"$3/.agent-write-probe\") 2>/dev/null; then exit 1; fi", "test -z \"${MADE_REVIEW_SECRET:-}\"", - "printf '%s\\n' '{\"findings\":[]}' > \"$6\"", "printf '%s\\n' '{\"findings\":[]}'", "", }, "\n") @@ -81,7 +77,7 @@ func TestSpawn_CodexUsesSupportedExecStructuredContract(t *testing.T) { if len(args) > 0 && args[0] == "review" { t.Fatalf("Codex invocation used obsolete review command: %s", data) } - if _, err := os.Stat(args[10]); !os.IsNotExist(err) { + if _, err := os.Stat(args[2]); !os.IsNotExist(err) { t.Fatalf("review clone was not cleaned up: %v", err) } if _, err := os.Stat(hookMarker); !os.IsNotExist(err) { @@ -89,6 +85,56 @@ func TestSpawn_CodexUsesSupportedExecStructuredContract(t *testing.T) { } } +func TestSpawn_TrustedBaseIsResolvableInDetachedReviewCopy(t *testing.T) { + worktree := agentWorktree(t) + base := strings.TrimSpace(gitAgent(t, worktree, "rev-parse", "HEAD")) + script := filepath.Join(t.TempDir(), "base-aware-codex") + contents := strings.Join([]string{ + "#!/bin/sh", + "set -eu", + "[ \"$1\" = \"exec\" ]", + "[ \"$2\" = \"--cd\" ]", + "[ -d \"$3\" ]", + "[ \"$4\" = \"--json\" ]", + "[ \"$5\" = \"--output-schema\" ]", + "[ -f \"$6\" ]", + "[ \"$7\" = \"-\" ]", + "git -C \"$3\" cat-file -e \"$FAKE_AGENT_BASE_SHA^{commit}\"", + "printf '%s\\n' '{\"findings\":[]}'", + "", + }, "\n") + if err := os.WriteFile(script, []byte(contents), 0o700); err != nil { + t.Fatalf("write base-aware Codex fake: %v", err) + } + + findings, err := agent.Spawn(context.Background(), agent.KindCodex, agent.SpawnParams{ + WorktreePath: worktree, + BinaryPath: script, + TrustedBaseSHA: base, + ExtraEnv: []string{"FAKE_AGENT_BASE_SHA=" + base}, + }) + if err != nil { + t.Fatalf("Spawn: %v", err) + } + if len(findings.Findings) != 0 { + t.Fatalf("expected no findings, got %+v", findings) + } +} + +func TestSpawn_RejectsUnavailableTrustedBaseBeforeAgentExecution(t *testing.T) { + worktree := agentWorktree(t) + missing := strings.Repeat("f", 40) + _, err := agent.Spawn(context.Background(), agent.KindCodex, agent.SpawnParams{ + WorktreePath: worktree, + BinaryPath: agenttest.Build(t), + TrustedBaseSHA: missing, + ExtraEnv: []string{"FAKE_AGENT_SCENARIO=" + filepath.Join(t.TempDir(), "never-read.json")}, + }) + if err == nil || !strings.Contains(err.Error(), "trusted base") { + t.Fatalf("Spawn error = %v, want trusted-base rejection before agent execution", err) + } +} + func TestSpawn_RejectsReviewSymlinkThatEscapesClone(t *testing.T) { worktree := agentWorktree(t) outside := filepath.Join(t.TempDir(), "outside.txt") @@ -131,7 +177,6 @@ func TestSpawn_ContainsReviewerFromSourceWorktree(t *testing.T) { "if chmod -R u+w " + shellQuote(worktree) + " 2>/dev/null; then", " if : > " + shellQuote(filepath.Join(worktree, "source-mutated")) + " 2>/dev/null; then exit 42; fi", "fi", - "printf '%s\\n' '{\"findings\":[]}' > \"$6\"", "printf '%s\\n' '{\"findings\":[]}'", "", }, "\n") diff --git a/internal/agent/review_schema_test.go b/internal/agent/review_schema_test.go new file mode 100644 index 0000000..1e07e73 --- /dev/null +++ b/internal/agent/review_schema_test.go @@ -0,0 +1,40 @@ +package agent + +import ( + "encoding/json" + "testing" +) + +func TestReviewSchemaRequiresEveryFindingProperty(t *testing.T) { + var schema struct { + Properties struct { + Findings struct { + Items struct { + Properties map[string]json.RawMessage `json:"properties"` + Required []string `json:"required"` + } `json:"items"` + } `json:"findings"` + } `json:"properties"` + } + if err := json.Unmarshal([]byte(reviewSchema), &schema); err != nil { + t.Fatalf("review schema is not JSON: %v", err) + } + if len(schema.Properties.Findings.Items.Properties) != len(schema.Properties.Findings.Items.Required) { + t.Fatalf("strict output schema must require every finding property: properties=%v required=%v", schema.Properties.Findings.Items.Properties, schema.Properties.Findings.Items.Required) + } + for _, property := range []string{"kind", "description", "patch", "paths"} { + if _, ok := schema.Properties.Findings.Items.Properties[property]; !ok { + t.Fatalf("review schema missing finding property %q", property) + } + found := false + for _, required := range schema.Properties.Findings.Items.Required { + if required == property { + found = true + break + } + } + if !found { + t.Fatalf("review schema does not require finding property %q", property) + } + } +} diff --git a/internal/agent/reviewcontract.go b/internal/agent/reviewcontract.go new file mode 100644 index 0000000..1c00503 --- /dev/null +++ b/internal/agent/reviewcontract.go @@ -0,0 +1,122 @@ +package agent + +import ( + "encoding/hex" + "encoding/json" + "fmt" + "strings" +) + +const ( + ReviewPromptVersion = "made-review-prompt-v1" + ReviewOutputSchemaVersion = "made-review-schema-v1" + maxReviewTaskBytes = 256 << 10 +) + +var reviewFindingTaxonomy = []string{ + "correctness", + "data-loss", + "security-trust-boundary", + "concurrency-lifecycle", + "public-api-schema-compatibility", + "error-handling", + "material-performance-regression", + "missing-tests", + "documentation-public-contract", +} + +var reviewFindingKinds = []string{"auto-fixable", "ask-user", "blocking"} + +type ReviewInput struct { + TrustedBaseBranch string + TrustedBaseSHA string + CandidateInputSHA string + CandidateOutputSHA string +} + +type ReviewContract struct { + PromptVersion string `json:"prompt_version"` + OutputSchemaVersion string `json:"output_schema_version"` + TrustedBaseBranch string `json:"trusted_base_branch"` + TrustedBaseSHA string `json:"trusted_base_sha"` + CandidateInputSHA string `json:"candidate_input_sha"` + CandidateOutputSHA string `json:"candidate_output_sha,omitempty"` + DiffCommand string `json:"diff_command"` + Scope string `json:"scope"` + FindingTaxonomy []string `json:"finding_taxonomy"` + FindingKinds []string `json:"finding_kinds"` + FindingPolicy []string `json:"finding_policy"` + Exclusions []string `json:"exclusions"` +} + +type ReviewTask struct { + Contract ReviewContract + Text string +} + +func NewReviewTask(input ReviewInput) (ReviewTask, error) { + baseBranch := strings.TrimSpace(input.TrustedBaseBranch) + if baseBranch == "" { + return ReviewTask{}, fmt.Errorf("agent: trusted base branch is required") + } + if strings.ContainsAny(baseBranch, "\r\n") { + return ReviewTask{}, fmt.Errorf("agent: trusted base branch contains a newline") + } + if err := validateReviewSHA("trusted base SHA", input.TrustedBaseSHA); err != nil { + return ReviewTask{}, err + } + if err := validateReviewSHA("candidate input SHA", input.CandidateInputSHA); err != nil { + return ReviewTask{}, err + } + if input.CandidateOutputSHA != "" { + if err := validateReviewSHA("candidate output SHA", input.CandidateOutputSHA); err != nil { + return ReviewTask{}, err + } + } + + contract := ReviewContract{ + PromptVersion: ReviewPromptVersion, + OutputSchemaVersion: ReviewOutputSchemaVersion, + TrustedBaseBranch: baseBranch, + TrustedBaseSHA: input.TrustedBaseSHA, + CandidateInputSHA: input.CandidateInputSHA, + CandidateOutputSHA: input.CandidateOutputSHA, + DiffCommand: fmt.Sprintf("git diff --no-ext-diff --unified=80 %s..%s --", input.TrustedBaseSHA, input.CandidateInputSHA), + Scope: "Review only defects introduced by the candidate change relative to the trusted base, including directly affected behavior and public interfaces.", + FindingTaxonomy: append([]string(nil), reviewFindingTaxonomy...), + FindingKinds: append([]string(nil), reviewFindingKinds...), + FindingPolicy: []string{ + "auto-fixable: a fully specified mechanical patch for an introduced defect with exact tracked affected paths; Made applies it only after controlled path, index, and validation checks.", + "ask-user: a finding that requires human judgment or a policy choice; preserve it for explicit approval and never apply a patch automatically.", + "blocking: an introduced defect that makes delivery unsafe; halt the stage and never apply a patch automatically.", + }, + Exclusions: []string{ + "unrelated legacy defects", + "general style advice", + "broad refactoring suggestions", + }, + } + contractJSON, err := json.Marshal(contract) + if err != nil { + return ReviewTask{}, fmt.Errorf("agent: encode review contract: %w", err) + } + text := "Inspect the exact candidate diff before deciding that findings are empty. " + + "Return only the structured object matching the supplied output schema.\n" + + "MADE_REVIEW_CONTRACT=" + string(contractJSON) + "\n" + + "Review every taxonomy category that applies, report exact affected paths for every patch, " + + "and do not report excluded material.\n" + if len([]byte(text)) > maxReviewTaskBytes { + return ReviewTask{}, fmt.Errorf("agent: review task exceeds %d bytes", maxReviewTaskBytes) + } + return ReviewTask{Contract: contract, Text: text}, nil +} + +func validateReviewSHA(label, value string) error { + if len(value) < 40 || len(value) > 64 { + return fmt.Errorf("agent: %s must be a full Git commit SHA", label) + } + if _, err := hex.DecodeString(value); err != nil { + return fmt.Errorf("agent: %s must be a hexadecimal Git commit SHA: %w", label, err) + } + return nil +} diff --git a/internal/agent/reviewcontract_test.go b/internal/agent/reviewcontract_test.go new file mode 100644 index 0000000..95433c9 --- /dev/null +++ b/internal/agent/reviewcontract_test.go @@ -0,0 +1,67 @@ +package agent_test + +import ( + "encoding/json" + "reflect" + "strings" + "testing" + + "github.com/douglasjarquin/made/internal/agent" +) + +func TestReviewTask_EmbedsVersionedIdentityScopeAndTaxonomy(t *testing.T) { + input := agent.ReviewInput{ + TrustedBaseBranch: "main", + TrustedBaseSHA: strings.Repeat("a", 40), + CandidateInputSHA: strings.Repeat("b", 40), + CandidateOutputSHA: strings.Repeat("c", 40), + } + + task, err := agent.NewReviewTask(input) + if err != nil { + t.Fatalf("NewReviewTask: %v", err) + } + + if task.Contract.PromptVersion != agent.ReviewPromptVersion { + t.Fatalf("prompt version = %q, want %q", task.Contract.PromptVersion, agent.ReviewPromptVersion) + } + if task.Contract.OutputSchemaVersion != agent.ReviewOutputSchemaVersion { + t.Fatalf("output schema version = %q, want %q", task.Contract.OutputSchemaVersion, agent.ReviewOutputSchemaVersion) + } + if task.Contract.TrustedBaseSHA != input.TrustedBaseSHA || task.Contract.CandidateInputSHA != input.CandidateInputSHA || task.Contract.CandidateOutputSHA != input.CandidateOutputSHA { + t.Fatalf("task identity = %+v, want input identity %+v", task.Contract, input) + } + if task.Contract.DiffCommand == "" || !strings.Contains(task.Contract.DiffCommand, input.TrustedBaseSHA) || !strings.Contains(task.Contract.DiffCommand, input.CandidateInputSHA) { + t.Fatalf("diff command = %q, want both exact commit identities", task.Contract.DiffCommand) + } + if len(task.Contract.FindingTaxonomy) < 8 || len(task.Contract.FindingKinds) != 3 { + t.Fatalf("review taxonomy = %+v, want substantive taxonomy and three finding kinds", task.Contract) + } + + const marker = "MADE_REVIEW_CONTRACT=" + lineStart := strings.Index(task.Text, marker) + if lineStart < 0 { + t.Fatalf("review task omitted machine-readable contract marker: %q", task.Text) + } + line := task.Text[lineStart+len(marker):] + if newline := strings.IndexByte(line, '\n'); newline >= 0 { + line = line[:newline] + } + var embedded agent.ReviewContract + if err := json.Unmarshal([]byte(line), &embedded); err != nil { + t.Fatalf("decode embedded review contract: %v", err) + } + if !reflect.DeepEqual(embedded, task.Contract) { + t.Fatalf("embedded contract = %+v, want %+v", embedded, task.Contract) + } +} + +func TestReviewTask_RejectsMissingTrustedBaseIdentity(t *testing.T) { + _, err := agent.NewReviewTask(agent.ReviewInput{ + TrustedBaseBranch: "main", + CandidateInputSHA: strings.Repeat("b", 40), + }) + if err == nil || !strings.Contains(err.Error(), "trusted base SHA") { + t.Fatalf("NewReviewTask error = %v, want actionable trusted-base error", err) + } +} diff --git a/internal/agent/reviewworktree.go b/internal/agent/reviewworktree.go index 263f73b..e707ba8 100644 --- a/internal/agent/reviewworktree.go +++ b/internal/agent/reviewworktree.go @@ -18,7 +18,7 @@ const ( reviewPreparationLimit = 1 << 20 ) -func prepareReviewWorktree(ctx context.Context, source string) (string, []string, []string, func(), error) { +func prepareReviewWorktree(ctx context.Context, source, trustedBaseSHA string) (string, []string, []string, func(), error) { source, err := filepath.Abs(source) if err != nil { return "", nil, nil, nil, fmt.Errorf("resolve source worktree: %w", err) @@ -34,6 +34,18 @@ func prepareReviewWorktree(ctx context.Context, source string) (string, []string if head == "" { return "", nil, nil, nil, fmt.Errorf("read source HEAD returned an empty SHA") } + if trustedBaseSHA != "" { + if err := validateReviewSHA("trusted base SHA", trustedBaseSHA); err != nil { + return "", nil, nil, nil, err + } + baseResult, err := runReviewGit(ctx, source, "cat-file", "-e", trustedBaseSHA+"^{commit}") + if err != nil { + return "", nil, nil, nil, fmt.Errorf("inspect source trusted base: %w", err) + } + if baseResult.ExitCode != 0 { + return "", nil, nil, nil, fmt.Errorf("trusted base %s is unavailable in source repository", trustedBaseSHA) + } + } commonResult, err := runReviewGit(ctx, source, "rev-parse", "--git-common-dir") if err != nil { return "", nil, nil, nil, fmt.Errorf("read source Git common directory: %w", err) @@ -62,6 +74,17 @@ func prepareReviewWorktree(ctx context.Context, source string) (string, []string cleanupTemp() return "", nil, nil, nil, commandFailure("clone review worktree", cloneResult) } + if trustedBaseSHA != "" { + baseResult, err := runReviewGit(ctx, reviewPath, "cat-file", "-e", trustedBaseSHA+"^{commit}") + if err != nil { + cleanupTemp() + return "", nil, nil, nil, fmt.Errorf("inspect review trusted base: %w", err) + } + if baseResult.ExitCode != 0 { + cleanupTemp() + return "", nil, nil, nil, fmt.Errorf("trusted base %s is unavailable in detached review repository", trustedBaseSHA) + } + } checkoutResult, err := runReviewGit(ctx, reviewPath, "checkout", "--detach", "--quiet", head) if err != nil { cleanupTemp() diff --git a/internal/agent/spawn.go b/internal/agent/spawn.go index 903f349..c5bcca8 100644 --- a/internal/agent/spawn.go +++ b/internal/agent/spawn.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "fmt" + "io" "os" "path/filepath" "strings" @@ -15,70 +16,93 @@ import ( ) type SpawnParams struct { - WorktreePath string - BinaryPath string - ExtraEnv []string - Task string - Timeout time.Duration + WorktreePath string + BinaryPath string + ExtraEnv []string + Task string + TrustedBaseSHA string + Timeout time.Duration } -const defaultSpawnTimeout = 30 * time.Minute +const ( + defaultSpawnTimeout = 30 * time.Minute + spawnOutputLimit = 1 << 20 +) + +type SpawnResult struct { + Findings Findings + Task string + Response []byte +} func Spawn(ctx context.Context, kind Kind, params SpawnParams) (Findings, error) { + result, err := SpawnWithEvidence(ctx, kind, params) + if err != nil { + return Findings{}, err + } + return result.Findings, nil +} + +func SpawnWithEvidence(ctx context.Context, kind Kind, params SpawnParams) (SpawnResult, error) { if kind != KindCodex { - return Findings{}, fmt.Errorf("agent: %s structured task contract is unsupported", kind) + return SpawnResult{}, fmt.Errorf("agent: %s structured task contract is unsupported; supported agents: %s", kind, KindCodex) } binary := params.BinaryPath if binary == "" { binary = kind.binaryName() } - reviewPath, protectedPaths, maskPaths, cleanupReview, err := prepareReviewWorktree(ctx, params.WorktreePath) + reviewPath, protectedPaths, maskPaths, cleanupReview, err := prepareReviewWorktree(ctx, params.WorktreePath, params.TrustedBaseSHA) if err != nil { - return Findings{}, fmt.Errorf("agent: prepare read-only review worktree: %w", err) + return SpawnResult{}, fmt.Errorf("agent: prepare read-only review worktree: %w", err) } defer cleanupReview() - args, cleanup, outputPath, err := invocation(kind, reviewPath, params.Task) + args, cleanup, err := invocation(kind, reviewPath) if err != nil { - return Findings{}, err + return SpawnResult{}, err } defer cleanup() commandName, commandArgs, err := containedInvocation(binary, args, reviewPath, protectedPaths, maskPaths) if err != nil { - return Findings{}, fmt.Errorf("agent: contain review process: %w", err) + return SpawnResult{}, fmt.Errorf("agent: contain review process: %w", err) + } + task := strings.TrimSpace(params.Task) + if task == "" { + task = "Inspect the candidate diff in the detached review repository before deciding that findings are empty. Return only the structured object matching the supplied output schema." + } + if len([]byte(task)) > maxReviewTaskBytes { + return SpawnResult{}, fmt.Errorf("agent: review task exceeds %d bytes", maxReviewTaskBytes) } timeout := params.Timeout - if timeout <= 0 { + if timeout <= 0 || timeout > defaultSpawnTimeout { timeout = defaultSpawnTimeout } result, err := exec.Run(ctx, exec.Command{ - Name: commandName, - Args: commandArgs, - Dir: reviewPath, - Env: reviewEnvironmentForDir(params.ExtraEnv, reviewPath), - Stdin: []byte("Return only the Made review JSON object matching the supplied schema.\n"), - Timeout: timeout, + Name: commandName, + Args: commandArgs, + Dir: reviewPath, + Env: reviewEnvironmentForDir(params.ExtraEnv, reviewPath), + Stdin: []byte(task), + Timeout: timeout, + OutputLimit: spawnOutputLimit, }) if err != nil { - return Findings{}, fmt.Errorf("agent: spawn %s (%s): %w", kind, binary, err) + return SpawnResult{}, fmt.Errorf("agent: spawn %s (%s): %w", kind, binary, err) } if result.ExitCode != 0 { - return Findings{}, fmt.Errorf("agent: %s (%s) exited %d: %s", kind, binary, result.ExitCode, evidence.RedactString(string(result.Stderr))) + return SpawnResult{}, fmt.Errorf("agent: %s (%s) exited %d: %s", kind, binary, result.ExitCode, evidence.RedactString(string(result.Stderr))) } - data := result.Stdout - if outputPath != "" { - data, err = os.ReadFile(outputPath) - if err != nil { - return Findings{}, fmt.Errorf("agent: read structured output from %s: %w", kind, err) - } + response, err := extractStructuredResponse(result.Stdout) + if err != nil { + return SpawnResult{}, fmt.Errorf("agent: extract structured response from %s: %w: stdout=%s", kind, err, evidence.RedactString(string(result.Stdout))) } - findings, err := strictFindings(data) + findings, err := strictFindings(response) if err != nil { - return Findings{}, fmt.Errorf("agent: parse findings from %s: %w: stdout=%s", kind, err, evidence.RedactString(string(result.Stdout))) + return SpawnResult{}, fmt.Errorf("agent: parse findings from %s: %w: stdout=%s", kind, err, evidence.RedactString(string(result.Stdout))) } - return findings, nil + return SpawnResult{Findings: findings, Task: task, Response: response}, nil } func reviewEnvironmentForDir(extra []string, dir string) []string { @@ -101,30 +125,69 @@ func reviewEnvironmentForDir(extra []string, dir string) []string { func reviewEnvironmentKey(name string) bool { switch name { case "PATH", "HOME", "TMPDIR", "LANG", "TERM", "USER", "LOGNAME", "SHELL", "PWD", "OLDPWD", "NO_COLOR", "CI", - "FAKE_AGENT_KIND", "FAKE_AGENT_SCENARIO", "FAKE_AGENT_LOG_FILE", "FAKE_AGENT_EXIT_CODE", "FAKE_AGENT_WRITE_PATH", "FAKE_AGENT_WRITE_DATA": + "FAKE_AGENT_KIND", "FAKE_AGENT_SCENARIO", "FAKE_AGENT_LOG_FILE", "FAKE_AGENT_EXIT_CODE", "FAKE_AGENT_WRITE_PATH", "FAKE_AGENT_WRITE_DATA", "FAKE_AGENT_BASE_SHA": return true } return strings.HasPrefix(name, "LC_") } -func invocation(kind Kind, worktree, task string) ([]string, func(), string, error) { +func invocation(kind Kind, worktree string) ([]string, func(), error) { if kind != KindCodex { - return nil, nil, "", fmt.Errorf("agent: %s structured task contract is unsupported", kind) + return nil, nil, fmt.Errorf("agent: %s structured task contract is unsupported; supported agents: %s", kind, KindCodex) } dir, err := os.MkdirTemp("", "made-codex-schema-") if err != nil { - return nil, nil, "", fmt.Errorf("agent: create Codex schema directory: %w", err) + return nil, nil, fmt.Errorf("agent: create Codex schema directory: %w", err) } schemaPath := filepath.Join(dir, "findings.schema.json") if err := os.WriteFile(schemaPath, []byte(reviewSchema), 0o600); err != nil { _ = os.RemoveAll(dir) - return nil, nil, "", fmt.Errorf("agent: write Codex output schema: %w", err) + return nil, nil, fmt.Errorf("agent: write Codex output schema: %w", err) + } + return []string{"exec", "--cd", worktree, "--json", "--output-schema", schemaPath, "-"}, func() { _ = os.RemoveAll(dir) }, nil +} + +func extractStructuredResponse(data []byte) ([]byte, error) { + trimmed := bytes.TrimSpace(data) + if len(trimmed) == 0 { + return nil, fmt.Errorf("structured output is empty") + } + var direct map[string]json.RawMessage + if err := json.Unmarshal(trimmed, &direct); err == nil { + if _, ok := direct["findings"]; ok { + return append([]byte(nil), trimmed...), nil + } } - outputPath := filepath.Join(dir, "findings.json") - if strings.TrimSpace(task) == "" { - task = "Review the current worktree and return only the structured findings object required by the output schema." + + var response []byte + for _, line := range bytes.Split(trimmed, []byte{'\n'}) { + line = bytes.TrimSpace(line) + if len(line) == 0 { + continue + } + var event struct { + Type string `json:"type"` + Item struct { + Type string `json:"type"` + Text string `json:"text"` + } `json:"item"` + } + if err := json.Unmarshal(line, &event); err != nil { + return nil, fmt.Errorf("structured output event is invalid: %w", err) + } + switch event.Type { + case "error", "turn.failed": + return nil, fmt.Errorf("Codex returned a failed event") + case "item.completed": + if event.Item.Type == "agent_message" && strings.TrimSpace(event.Item.Text) != "" { + response = []byte(event.Item.Text) + } + } } - return []string{"exec", "--json", "--output-schema", schemaPath, "--output-last-message", outputPath, "--sandbox", "read-only", "--ephemeral", "-C", worktree, task}, func() { _ = os.RemoveAll(dir) }, outputPath, nil + if len(response) == 0 { + return nil, fmt.Errorf("structured output event stream did not contain a completed agent message") + } + return response, nil } func strictFindings(data []byte) (Findings, error) { @@ -134,8 +197,15 @@ func strictFindings(data []byte) (Findings, error) { if err := decoder.Decode(&findings); err != nil { return Findings{}, err } + var extra json.RawMessage + if err := decoder.Decode(&extra); err != io.EOF { + if err == nil { + return Findings{}, fmt.Errorf("structured output contains multiple JSON values") + } + return Findings{}, err + } for _, finding := range findings.Findings { - if finding.Description == "" { + if strings.TrimSpace(finding.Description) == "" { return Findings{}, fmt.Errorf("finding description is required") } switch finding.Kind { @@ -147,6 +217,9 @@ func strictFindings(data []byte) (Findings, error) { return Findings{}, fmt.Errorf("auto-fixable finding paths are required") } case FindingAskUser, FindingBlocking: + if strings.TrimSpace(finding.Patch) != "" { + return Findings{}, fmt.Errorf("%s finding must not include a patch", finding.Kind) + } default: return Findings{}, fmt.Errorf("unknown finding kind %q", finding.Kind) } @@ -154,4 +227,4 @@ func strictFindings(data []byte) (Findings, error) { return findings, nil } -const reviewSchema = `{"type":"object","additionalProperties":false,"required":["findings"],"properties":{"findings":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["kind","description"],"properties":{"kind":{"type":"string","enum":["auto-fixable","ask-user","blocking"]},"description":{"type":"string"},"patch":{"type":"string"},"paths":{"type":"array","items":{"type":"string"}}}}}}}` +const reviewSchema = `{"type":"object","additionalProperties":false,"required":["findings"],"properties":{"findings":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["kind","description","patch","paths"],"properties":{"kind":{"type":"string","enum":["auto-fixable","ask-user","blocking"]},"description":{"type":"string"},"patch":{"type":["string","null"]},"paths":{"type":["array","null"],"items":{"type":"string"}}}}}}}` diff --git a/internal/agent/testdata/fakeagent/main.go b/internal/agent/testdata/fakeagent/main.go index fe88277..835de17 100644 --- a/internal/agent/testdata/fakeagent/main.go +++ b/internal/agent/testdata/fakeagent/main.go @@ -1,4 +1,4 @@ -// Command fakeagent is a deterministic test double for the Claude/Codex CLIs: +// Command fakeagent is a deterministic test double for the Codex CLI: // it never calls a real model, it just replays a scripted findings payload so // internal/agent and internal/pipeline/review can be tested without network // access or API keys. Scenario selection and invocation logging are both @@ -9,6 +9,7 @@ package main import ( "fmt" + "io" "os" "path/filepath" ) @@ -58,29 +59,26 @@ func main() { os.Exit(1) } - args := os.Args[1:] - lastMessagePath := args[5] - if err := os.WriteFile(lastMessagePath, data, 0o600); err != nil { - fmt.Fprintf(os.Stderr, "fakeagent: write structured output %s: %v\n", lastMessagePath, err) + if _, err := os.Stdout.Write(data); err != nil { + fmt.Fprintf(os.Stderr, "fakeagent: write structured output: %v\n", err) os.Exit(1) } - _, _ = fmt.Fprintln(os.Stdout, `{"type":"turn.completed"}`) } const agentKindCodex = "codex" func validateInvocation(args []string) error { - if len(args) != 12 { - return fmt.Errorf("want 12 arguments, got %d", len(args)) + if len(args) != 7 { + return fmt.Errorf("want 7 arguments, got %d", len(args)) } - if args[0] != "exec" || args[1] != "--json" || args[2] != "--output-schema" || args[4] != "--output-last-message" || args[6] != "--sandbox" || args[7] != "read-only" || args[8] != "--ephemeral" || args[9] != "-C" { + if args[0] != "exec" || args[1] != "--cd" || args[3] != "--json" || args[4] != "--output-schema" || args[6] != "-" { return fmt.Errorf("expected codex exec structured flags, got %v", args) } - if filepath.IsAbs(args[3]) == false || filepath.IsAbs(args[5]) == false { - return fmt.Errorf("schema and output paths must be absolute") + if filepath.IsAbs(args[2]) == false || filepath.IsAbs(args[5]) == false { + return fmt.Errorf("review and schema paths must be absolute") } - if args[10] == "" || args[11] == "" { - return fmt.Errorf("worktree and task are required") + if args[2] == "" { + return fmt.Errorf("review worktree is required") } return nil } @@ -93,4 +91,6 @@ func logInvocation(logPath string) { defer f.Close() cwd, _ := os.Getwd() fmt.Fprintf(f, "invoked: args=%v cwd=%s\n", os.Args, cwd) + task, _ := io.ReadAll(io.LimitReader(os.Stdin, 1<<20)) + fmt.Fprintf(f, "task=%s\n", task) } diff --git a/internal/orchestrator/workfunc.go b/internal/orchestrator/workfunc.go index 399753e..a4bc9f3 100644 --- a/internal/orchestrator/workfunc.go +++ b/internal/orchestrator/workfunc.go @@ -251,7 +251,11 @@ func (c *chain) reviewStage() error { return fmt.Errorf("orchestrator: resolve agent kind: %w", err) } - result, err := review.Run(c.ctx, c.rc.Worktree.Path, agentKind, c.opts.ReviewOptions) + reviewOptions := c.opts.ReviewOptions + reviewOptions.BaseBranch = c.defaultBranch + reviewOptions.Evidence = c.rc.Evidence + reviewOptions.EvidenceRunID = c.runID + result, err := review.Run(c.ctx, c.rc.Worktree.Path, agentKind, reviewOptions) if err != nil { return err } diff --git a/internal/pipeline/review/autofix.go b/internal/pipeline/review/autofix.go new file mode 100644 index 0000000..3a93e84 --- /dev/null +++ b/internal/pipeline/review/autofix.go @@ -0,0 +1,196 @@ +package review + +import ( + "context" + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/douglasjarquin/made/internal/agent" + madeexec "github.com/douglasjarquin/made/internal/exec" +) + +func applyAutoFix(ctx context.Context, worktreePath string, finding agent.Finding) (string, string, error) { + if strings.TrimSpace(finding.Patch) == "" { + return "", "", fmt.Errorf("auto-fixable finding has no patch") + } + preSHA, err := gitOutput(ctx, worktreePath, "rev-parse", "HEAD") + if err != nil { + return "", "", fmt.Errorf("record pre-fix SHA: %w", err) + } + paths, err := patchPaths(finding.Patch) + if err != nil { + return "", "", err + } + allowed := make(map[string]struct{}, len(finding.Paths)) + for _, path := range finding.Paths { + clean, err := cleanReturnedPath(path) + if err != nil { + return "", "", err + } + if _, err := gitOutput(ctx, worktreePath, "ls-files", "--error-unmatch", "--", clean); err != nil { + return "", "", fmt.Errorf("auto-fix returned untracked or unauthorized path %q", clean) + } + allowed[clean] = struct{}{} + } + if len(allowed) == 0 { + return "", "", fmt.Errorf("auto-fixable finding must return paths") + } + for _, path := range paths { + if _, ok := allowed[path]; !ok { + return "", "", fmt.Errorf("auto-fix patch changes path %q outside returned paths", path) + } + } + + indexDir, err := os.MkdirTemp("", "made-review-index-") + if err != nil { + return "", "", fmt.Errorf("create isolated index: %w", err) + } + defer func() { _ = os.RemoveAll(indexDir) }() + indexPath := filepath.Join(indexDir, "index") + if _, err := runGitWithIndex(ctx, worktreePath, indexPath, nil, "read-tree", "HEAD"); err != nil { + return "", "", fmt.Errorf("seed isolated index: %w", err) + } + if _, err := runGitWithIndex(ctx, worktreePath, indexPath, nil, "update-index", "--refresh"); err != nil { + return "", "", fmt.Errorf("refresh isolated index: %w", err) + } + if _, err := runGitWithIndex(ctx, worktreePath, indexPath, []byte(finding.Patch), "apply", "--index", "--whitespace=fix", "-"); err != nil { + return "", "", fmt.Errorf("git apply: %w", err) + } + + filesOut, err := runGitWithIndex(ctx, worktreePath, indexPath, nil, "diff", "--cached", "--name-only", "--diff-filter=ACMRTUXB") + if err != nil { + return "", "", fmt.Errorf("git diff staged files: %w", err) + } + changed := strings.Fields(strings.TrimSpace(string(filesOut.Stdout))) + if len(changed) == 0 { + return "", "", fmt.Errorf("git apply produced no staged files") + } + for _, path := range changed { + clean := filepath.ToSlash(filepath.Clean(path)) + if _, ok := allowed[clean]; !ok { + return "", "", fmt.Errorf("auto-fix changed forbidden or unreturned path %q", path) + } + } + + message := finding.Description + if message == "" { + message = "made review: auto-fix" + } + commitArgs := []string{ + "-c", "user.name=made-review", + "-c", "user.email=made-review@local", + "-c", "commit.gpgsign=false", + "-c", "core.hooksPath=/dev/null", + "commit", "-m", message, + } + if _, err := runGitWithIndex(ctx, worktreePath, indexPath, nil, commitArgs...); err != nil { + return "", "", fmt.Errorf("git commit: %w", err) + } + for _, path := range changed { + if _, err := runGit(ctx, worktreePath, []string{"reset", "HEAD", "--", path}, nil); err != nil { + return "", "", fmt.Errorf("restore worktree index for %q: %w", path, err) + } + } + + shaOut, err := gitOutput(ctx, worktreePath, "rev-parse", "HEAD") + if err != nil { + return "", "", fmt.Errorf("git rev-parse HEAD: %w", err) + } + if _, err := gitOutput(ctx, worktreePath, "diff", "--check", preSHA, shaOut); err != nil { + return "", "", fmt.Errorf("rerun review validation: %w", err) + } + return preSHA, shaOut, nil +} + +func runGitWithIndex(ctx context.Context, worktreePath, indexPath string, stdin []byte, args ...string) (*madeexec.Result, error) { + filterArgs, err := repositoryFilterOverrides(ctx, worktreePath) + if err != nil { + return nil, err + } + commandArgs := []string{ + "-C", worktreePath, + "-c", "core.hooksPath=/dev/null", + "-c", "core.fsmonitor=false", + "-c", "diff.external=", + } + commandArgs = append(commandArgs, filterArgs...) + commandArgs = append(commandArgs, args...) + env := append(controlledGitEnvironment(), "GIT_INDEX_FILE="+indexPath) + result, err := madeexec.Run(ctx, madeexec.Command{ + Name: "git", + Args: commandArgs, + Env: env, + Stdin: stdin, + Timeout: reviewGitTimeout, + OutputLimit: reviewGitLimit, + }) + if err != nil { + return nil, err + } + if result.ExitCode != 0 { + return result, fmt.Errorf("git exited %d: %s", result.ExitCode, strings.TrimSpace(string(result.Stderr))) + } + return result, nil +} + +func patchPaths(patch string) ([]string, error) { + seen := make(map[string]struct{}) + var oldPath string + for _, line := range strings.Split(patch, "\n") { + if strings.HasPrefix(line, "--- ") { + var err error + oldPath, err = patchHeaderPath(strings.TrimPrefix(line, "--- ")) + if err != nil { + return nil, err + } + continue + } + if !strings.HasPrefix(line, "+++ ") { + continue + } + newPath, err := patchHeaderPath(strings.TrimPrefix(line, "+++ ")) + if err != nil { + return nil, err + } + if oldPath != "" { + seen[oldPath] = struct{}{} + } + if newPath != "" { + seen[newPath] = struct{}{} + } + oldPath = "" + } + if len(seen) == 0 { + return nil, fmt.Errorf("auto-fix patch contains no returned paths") + } + paths := make([]string, 0, len(seen)) + for path := range seen { + paths = append(paths, path) + } + return paths, nil +} + +func patchHeaderPath(path string) (string, error) { + fields := strings.Fields(path) + if len(fields) == 0 { + return "", fmt.Errorf("auto-fix patch contains an empty file header") + } + path = fields[0] + if path == "/dev/null" { + return "", nil + } + if strings.HasPrefix(path, "a/") || strings.HasPrefix(path, "b/") { + path = path[2:] + } + return cleanReturnedPath(path) +} + +func cleanReturnedPath(path string) (string, error) { + clean := filepath.Clean(path) + if clean == "." || filepath.IsAbs(path) || clean == ".." || strings.HasPrefix(clean, ".."+string(filepath.Separator)) || clean == ".git" || strings.HasPrefix(clean, ".git"+string(filepath.Separator)) { + return "", fmt.Errorf("auto-fix returned forbidden path %q", path) + } + return filepath.ToSlash(clean), nil +} diff --git a/internal/pipeline/review/contract.go b/internal/pipeline/review/contract.go new file mode 100644 index 0000000..6abcbb1 --- /dev/null +++ b/internal/pipeline/review/contract.go @@ -0,0 +1,36 @@ +package review + +import ( + "context" + "fmt" + "strings" + + "github.com/douglasjarquin/made/internal/agent" +) + +func resolveReviewTask(ctx context.Context, worktreePath string, opts Options) (agent.ReviewTask, error) { + candidateSHA, err := gitOutput(ctx, worktreePath, "rev-parse", "--verify", "HEAD^{commit}") + if err != nil { + return agent.ReviewTask{}, fmt.Errorf("resolve candidate input SHA: %w", err) + } + baseBranch := strings.TrimSpace(opts.BaseBranch) + if baseBranch == "" { + baseBranch = "HEAD" + } + if strings.HasPrefix(baseBranch, "-") || strings.ContainsAny(baseBranch, "\r\n") { + return agent.ReviewTask{}, fmt.Errorf("trusted base branch %q is not a valid Git ref", baseBranch) + } + baseSHA := candidateSHA + if baseBranch != "HEAD" { + baseSHA, err = gitOutput(ctx, worktreePath, "rev-parse", "--verify", "--end-of-options", baseBranch+"^{commit}") + if err != nil { + return agent.ReviewTask{}, fmt.Errorf("resolve trusted base %q: %w", baseBranch, err) + } + } + return agent.NewReviewTask(agent.ReviewInput{ + TrustedBaseBranch: baseBranch, + TrustedBaseSHA: baseSHA, + CandidateInputSHA: candidateSHA, + CandidateOutputSHA: opts.CandidateOutputSHA, + }) +} diff --git a/internal/pipeline/review/evidence.go b/internal/pipeline/review/evidence.go new file mode 100644 index 0000000..5a2c536 --- /dev/null +++ b/internal/pipeline/review/evidence.go @@ -0,0 +1,41 @@ +package review + +import ( + "context" + "encoding/json" + "fmt" + + "github.com/douglasjarquin/made/internal/agent" +) + +func writeReviewEvidence(ctx context.Context, opts Options, task agent.ReviewTask, response []byte, candidateOutputSHA string) error { + if opts.Evidence == nil { + return nil + } + if opts.EvidenceRunID == "" { + return fmt.Errorf("review: evidence run ID is required when evidence storage is configured") + } + contract := task.Contract + contract.CandidateOutputSHA = candidateOutputSHA + metadata, err := json.Marshal(contract) + if err != nil { + return fmt.Errorf("review: encode review evidence metadata: %w", err) + } + files := map[string][]byte{ + "review-contract.json": metadata, + "review-prompt.txt": []byte(task.Text), + "review-response.json": append([]byte(nil), response...), + } + if contextual, ok := opts.Evidence.(interface { + WriteEvidenceContext(context.Context, string, map[string][]byte) error + }); ok { + if err := contextual.WriteEvidenceContext(ctx, opts.EvidenceRunID, files); err != nil { + return fmt.Errorf("review: write evidence: %w", err) + } + return nil + } + if err := opts.Evidence.WriteEvidence(opts.EvidenceRunID, files); err != nil { + return fmt.Errorf("review: write evidence: %w", err) + } + return nil +} diff --git a/internal/pipeline/review/evidence_contract_test.go b/internal/pipeline/review/evidence_contract_test.go new file mode 100644 index 0000000..4c3ff73 --- /dev/null +++ b/internal/pipeline/review/evidence_contract_test.go @@ -0,0 +1,79 @@ +package review_test + +import ( + "context" + "encoding/json" + "strings" + "testing" + + "github.com/douglasjarquin/made/internal/agent" + "github.com/douglasjarquin/made/internal/agent/agenttest" + "github.com/douglasjarquin/made/internal/pipeline/review" +) + +type recordingEvidenceStore struct { + files map[string][]byte +} + +func (s *recordingEvidenceStore) WriteEvidence(_ string, files map[string][]byte) error { + s.files = make(map[string][]byte, len(files)) + for name, content := range files { + s.files[name] = append([]byte(nil), content...) + } + return nil +} + +func TestRun_WritesVersionedReviewEvidenceWithCandidateOutputSHA(t *testing.T) { + bin := agenttest.Build(t) + f := setupFixture(t) + wt := f.addWorktree(t) + t.Cleanup(func() { _ = wt.Remove() }) + scenarioPath := writeScenario(t, agent.Findings{}) + store := &recordingEvidenceStore{} + candidateOutputSHA := strings.Repeat("c", 40) + + result, err := review.Run(context.Background(), wt.Path, agent.KindCodex, review.Options{ + BinaryPath: bin, + BaseBranch: "HEAD", + CandidateOutputSHA: candidateOutputSHA, + Evidence: store, + EvidenceRunID: "run-review-evidence", + ExtraEnv: []string{ + "FAKE_AGENT_KIND=codex", + "FAKE_AGENT_SCENARIO=" + scenarioPath, + }, + }) + if err != nil { + t.Fatalf("Run: %v", err) + } + if !result.OK { + t.Fatalf("review result = %+v, want OK", result) + } + + metadata, ok := store.files["review-contract.json"] + if !ok { + t.Fatalf("review evidence files = %v, want review-contract.json", store.files) + } + var contract struct { + PromptVersion string `json:"prompt_version"` + OutputSchemaVersion string `json:"output_schema_version"` + TrustedBaseSHA string `json:"trusted_base_sha"` + CandidateInputSHA string `json:"candidate_input_sha"` + CandidateOutputSHA string `json:"candidate_output_sha"` + } + if err := json.Unmarshal(metadata, &contract); err != nil { + t.Fatalf("decode review-contract.json: %v", err) + } + if contract.PromptVersion == "" || contract.OutputSchemaVersion == "" || contract.TrustedBaseSHA == "" || contract.CandidateInputSHA == "" { + t.Fatalf("review contract metadata missing identity/version fields: %+v", contract) + } + if contract.CandidateOutputSHA != candidateOutputSHA { + t.Fatalf("candidate output SHA = %q, want %q", contract.CandidateOutputSHA, candidateOutputSHA) + } + if _, ok := store.files["review-prompt.txt"]; !ok { + t.Fatalf("review evidence omitted review-prompt.txt: %v", store.files) + } + if string(store.files["review-response.json"]) != `{"findings":[]}` { + t.Fatalf("review response = %q, want structured empty findings", store.files["review-response.json"]) + } +} diff --git a/internal/pipeline/review/review.go b/internal/pipeline/review/review.go index e791ba1..0c3e5ef 100644 --- a/internal/pipeline/review/review.go +++ b/internal/pipeline/review/review.go @@ -1,5 +1,5 @@ // Package review is stage 3 of made's pipeline (Intent -> Rebase -> Review -> -// ...): it spawns the configured agent (Claude or Codex, via internal/agent) +// ...): it spawns the configured Codex agent via internal/agent // against the diff in a gate worktree, applies auto-fixable findings as new // commits, and queues ask-user/blocking findings in Result.PendingFindings so // a later human-approval stage (Task 22) can act on them - findings are never @@ -9,19 +9,21 @@ package review import ( "context" "fmt" - "os" - "path/filepath" "strings" "time" "github.com/douglasjarquin/made/internal/agent" - madeexec "github.com/douglasjarquin/made/internal/exec" + "github.com/douglasjarquin/made/internal/evidence" ) type Options struct { - BinaryPath string - ExtraEnv []string - Timeout time.Duration + BinaryPath string + ExtraEnv []string + Timeout time.Duration + BaseBranch string + CandidateOutputSHA string + Evidence evidence.Store + EvidenceRunID string } type Result struct { @@ -39,15 +41,21 @@ type Result struct { // etc); ask-user and blocking findings are normal outcomes reported via // Result, not errors. func Run(ctx context.Context, worktreePath string, agentKind agent.Kind, opts Options) (Result, error) { + task, err := resolveReviewTask(ctx, worktreePath, opts) + if err != nil { + return Result{}, fmt.Errorf("review: build exact-diff task: %w", err) + } beforeStatus, err := gitOutput(ctx, worktreePath, "status", "--porcelain", "--untracked-files=all") if err != nil { return Result{}, fmt.Errorf("review: inspect worktree before agent: %w", err) } - findings, err := agent.Spawn(ctx, agentKind, agent.SpawnParams{ - WorktreePath: worktreePath, - BinaryPath: opts.BinaryPath, - ExtraEnv: opts.ExtraEnv, - Timeout: opts.Timeout, + spawned, err := agent.SpawnWithEvidence(ctx, agentKind, agent.SpawnParams{ + WorktreePath: worktreePath, + BinaryPath: opts.BinaryPath, + ExtraEnv: opts.ExtraEnv, + Task: task.Text, + TrustedBaseSHA: task.Contract.TrustedBaseSHA, + Timeout: opts.Timeout, }) if err != nil { return Result{}, fmt.Errorf("review: spawn %s: %w", agentKind, err) @@ -66,7 +74,7 @@ func Run(ctx context.Context, worktreePath string, agentKind agent.Kind, opts Op var pending []agent.Finding var blockingMessages []string - for _, finding := range findings.Findings { + for _, finding := range spawned.Findings.Findings { switch finding.Kind { case agent.FindingAutoFixable: preSHA, postSHA, applyErr := applyAutoFix(ctx, worktreePath, finding) @@ -84,209 +92,32 @@ func Run(ctx context.Context, worktreePath string, agentKind agent.Kind, opts Op } } - if len(blockingMessages) > 0 { - return Result{ - OK: false, - Message: fmt.Sprintf("review halted by blocking finding(s): %s", strings.Join(blockingMessages, "; ")), - Findings: findings.Findings, - AutoFixed: autoFixed, - PreFixSHAs: preFixSHAs, - PostFixSHAs: postFixSHAs, - PendingFindings: pending, - }, nil - } - - return Result{ + result := Result{ OK: true, Message: fmt.Sprintf("review passed: %d auto-fix(es) applied, %d finding(s) await human approval", len(autoFixed), len(pending)), - Findings: findings.Findings, + Findings: spawned.Findings.Findings, AutoFixed: autoFixed, PreFixSHAs: preFixSHAs, PostFixSHAs: postFixSHAs, PendingFindings: pending, - }, nil -} - -func applyAutoFix(ctx context.Context, worktreePath string, finding agent.Finding) (string, string, error) { - if strings.TrimSpace(finding.Patch) == "" { - return "", "", fmt.Errorf("auto-fixable finding has no patch") - } - preSHA, err := gitOutput(ctx, worktreePath, "rev-parse", "HEAD") - if err != nil { - return "", "", fmt.Errorf("record pre-fix SHA: %w", err) - } - paths, err := patchPaths(finding.Patch) - if err != nil { - return "", "", err - } - allowed := make(map[string]struct{}, len(finding.Paths)) - for _, path := range finding.Paths { - clean, err := cleanReturnedPath(path) - if err != nil { - return "", "", err - } - if _, err := gitOutput(ctx, worktreePath, "ls-files", "--error-unmatch", "--", clean); err != nil { - return "", "", fmt.Errorf("auto-fix returned untracked or unauthorized path %q", clean) - } - allowed[clean] = struct{}{} - } - if len(allowed) == 0 { - return "", "", fmt.Errorf("auto-fixable finding must return paths") - } - for _, path := range paths { - if _, ok := allowed[path]; !ok { - return "", "", fmt.Errorf("auto-fix patch changes path %q outside returned paths", path) - } - } - - indexDir, err := os.MkdirTemp("", "made-review-index-") - if err != nil { - return "", "", fmt.Errorf("create isolated index: %w", err) - } - defer func() { _ = os.RemoveAll(indexDir) }() - indexPath := filepath.Join(indexDir, "index") - if _, err := runGitWithIndex(ctx, worktreePath, indexPath, nil, "read-tree", "HEAD"); err != nil { - return "", "", fmt.Errorf("seed isolated index: %w", err) - } - if _, err := runGitWithIndex(ctx, worktreePath, indexPath, nil, "update-index", "--refresh"); err != nil { - return "", "", fmt.Errorf("refresh isolated index: %w", err) - } - if _, err := runGitWithIndex(ctx, worktreePath, indexPath, []byte(finding.Patch), "apply", "--index", "--whitespace=fix", "-"); err != nil { - return "", "", fmt.Errorf("git apply: %w", err) - } - - filesOut, err := runGitWithIndex(ctx, worktreePath, indexPath, nil, "diff", "--cached", "--name-only", "--diff-filter=ACMRTUXB") - if err != nil { - return "", "", fmt.Errorf("git diff staged files: %w", err) - } - changed := strings.Fields(strings.TrimSpace(string(filesOut.Stdout))) - if len(changed) == 0 { - return "", "", fmt.Errorf("git apply produced no staged files") - } - for _, path := range changed { - clean := filepath.ToSlash(filepath.Clean(path)) - if _, ok := allowed[clean]; !ok { - return "", "", fmt.Errorf("auto-fix changed forbidden or unreturned path %q", path) - } - } - - message := finding.Description - if message == "" { - message = "made review: auto-fix" - } - commitArgs := []string{ - "-c", "user.name=made-review", - "-c", "user.email=made-review@local", - "-c", "commit.gpgsign=false", - "-c", "core.hooksPath=/dev/null", - "commit", "-m", message, } - if _, err := runGitWithIndex(ctx, worktreePath, indexPath, nil, commitArgs...); err != nil { - return "", "", fmt.Errorf("git commit: %w", err) - } - for _, path := range changed { - if _, err := runGit(ctx, worktreePath, []string{"reset", "HEAD", "--", path}, nil); err != nil { - return "", "", fmt.Errorf("restore worktree index for %q: %w", path, err) + if len(blockingMessages) > 0 { + result = Result{ + OK: false, + Message: fmt.Sprintf("review halted by blocking finding(s): %s", strings.Join(blockingMessages, "; ")), + Findings: spawned.Findings.Findings, + AutoFixed: autoFixed, + PreFixSHAs: preFixSHAs, + PostFixSHAs: postFixSHAs, + PendingFindings: pending, } } - - shaOut, err := gitOutput(ctx, worktreePath, "rev-parse", "HEAD") - if err != nil { - return "", "", fmt.Errorf("git rev-parse HEAD: %w", err) - } - if _, err := gitOutput(ctx, worktreePath, "diff", "--check", preSHA, shaOut); err != nil { - return "", "", fmt.Errorf("rerun review validation: %w", err) - } - return preSHA, shaOut, nil -} - -func runGitWithIndex(ctx context.Context, worktreePath, indexPath string, stdin []byte, args ...string) (*madeexec.Result, error) { - filterArgs, err := repositoryFilterOverrides(ctx, worktreePath) - if err != nil { - return nil, err - } - commandArgs := []string{ - "-C", worktreePath, - "-c", "core.hooksPath=/dev/null", - "-c", "core.fsmonitor=false", - "-c", "diff.external=", - } - commandArgs = append(commandArgs, filterArgs...) - commandArgs = append(commandArgs, args...) - env := append(controlledGitEnvironment(), "GIT_INDEX_FILE="+indexPath) - result, err := madeexec.Run(ctx, madeexec.Command{ - Name: "git", - Args: commandArgs, - Env: env, - Stdin: stdin, - Timeout: reviewGitTimeout, - OutputLimit: reviewGitLimit, - }) - if err != nil { - return nil, err + outputSHA := opts.CandidateOutputSHA + if len(postFixSHAs) > 0 { + outputSHA = postFixSHAs[len(postFixSHAs)-1] } - if result.ExitCode != 0 { - return result, fmt.Errorf("git exited %d: %s", result.ExitCode, strings.TrimSpace(string(result.Stderr))) + if err := writeReviewEvidence(ctx, opts, task, spawned.Response, outputSHA); err != nil { + return Result{}, err } return result, nil } - -func patchPaths(patch string) ([]string, error) { - seen := make(map[string]struct{}) - var oldPath string - for _, line := range strings.Split(patch, "\n") { - if strings.HasPrefix(line, "--- ") { - var err error - oldPath, err = patchHeaderPath(strings.TrimPrefix(line, "--- ")) - if err != nil { - return nil, err - } - continue - } - if !strings.HasPrefix(line, "+++ ") { - continue - } - newPath, err := patchHeaderPath(strings.TrimPrefix(line, "+++ ")) - if err != nil { - return nil, err - } - if oldPath != "" { - seen[oldPath] = struct{}{} - } - if newPath != "" { - seen[newPath] = struct{}{} - } - oldPath = "" - } - if len(seen) == 0 { - return nil, fmt.Errorf("auto-fix patch contains no returned paths") - } - paths := make([]string, 0, len(seen)) - for path := range seen { - paths = append(paths, path) - } - return paths, nil -} - -func patchHeaderPath(path string) (string, error) { - fields := strings.Fields(path) - if len(fields) == 0 { - return "", fmt.Errorf("auto-fix patch contains an empty file header") - } - path = fields[0] - if path == "/dev/null" { - return "", nil - } - if strings.HasPrefix(path, "a/") || strings.HasPrefix(path, "b/") { - path = path[2:] - } - return cleanReturnedPath(path) -} - -func cleanReturnedPath(path string) (string, error) { - clean := filepath.Clean(path) - if clean == "." || filepath.IsAbs(path) || clean == ".." || strings.HasPrefix(clean, ".."+string(filepath.Separator)) || clean == ".git" || strings.HasPrefix(clean, ".git"+string(filepath.Separator)) { - return "", fmt.Errorf("auto-fix returned forbidden path %q", path) - } - return filepath.ToSlash(clean), nil -} From 217bd86752c58623c23eab5cd70a48ad911673ee Mon Sep 17 00:00:00 2001 From: Doug Jarquin Date: Tue, 18 Aug 2026 10:47:20 -0400 Subject: [PATCH 2/9] fix(config): reject unsupported review agents --- cmd/made/contracts_red_test.go | 4 ++++ cmd/made/runcommands.go | 12 ++++++++++++ internal/config/config.go | 21 +++++++++++++++++--- internal/config/config_extended_test.go | 25 ++++++++++++++++-------- internal/config/config_test.go | 26 ++++++++++++------------- internal/config/file.go | 3 +++ internal/skill/skill.go | 4 ++-- internal/skill/skill_test.go | 10 ++++++++++ skills/made/SKILL.md | 4 ++-- 9 files changed, 81 insertions(+), 28 deletions(-) diff --git a/cmd/made/contracts_red_test.go b/cmd/made/contracts_red_test.go index f100089..e49b73b 100644 --- a/cmd/made/contracts_red_test.go +++ b/cmd/made/contracts_red_test.go @@ -22,6 +22,7 @@ func TestCapabilitiesJSONExposesStructuredRunContract(t *testing.T) { SchemaVersion int `json:"schema_version"` ProtocolVersion int `json:"protocol_version"` Commands []string `json:"commands"` + Agents []string `json:"agents"` } if err := json.Unmarshal(readOutputFile(t, stdoutFile), &payload); err != nil { t.Fatalf("capabilities output is not JSON: %v", err) @@ -40,6 +41,9 @@ func TestCapabilitiesJSONExposesStructuredRunContract(t *testing.T) { t.Fatalf("capabilities missing command %q: %+v", want, payload.Commands) } } + if len(payload.Agents) != 1 || payload.Agents[0] != "codex" { + t.Fatalf("capabilities agents = %v, want only codex", payload.Agents) + } _ = stdout _ = stderr } diff --git a/cmd/made/runcommands.go b/cmd/made/runcommands.go index cfe5d95..182f035 100644 --- a/cmd/made/runcommands.go +++ b/cmd/made/runcommands.go @@ -5,6 +5,7 @@ import ( "fmt" "os" + "github.com/douglasjarquin/made/internal/agent" "github.com/douglasjarquin/made/internal/api" ) @@ -12,6 +13,7 @@ type capabilitiesReport struct { SchemaVersion int `json:"schema_version"` ProtocolVersion int `json:"protocol_version"` Commands []string `json:"commands"` + Agents []string `json:"agents"` } func runCapabilitiesCommand(args []string, stdout, stderr *os.File) int { @@ -28,9 +30,19 @@ func runCapabilitiesCommand(args []string, stdout, stderr *os.File) int { return writeJSON(stdout, capabilitiesReport{ SchemaVersion: 1, ProtocolVersion: api.Version, Commands: []string{"run.submit", "run.status", "run.list", "run.cancel", "review.decide", "doctor"}, + Agents: supportedAgentNames(), }, stderr, "made capabilities") } +func supportedAgentNames() []string { + kinds := agent.SupportedKinds() + names := make([]string, len(kinds)) + for index, kind := range kinds { + names[index] = string(kind) + } + return names +} + type runSubmitParams struct { RunID string `json:"run_id,omitempty"` GatePath string `json:"gate_path"` diff --git a/internal/config/config.go b/internal/config/config.go index 34e8ca6..09c8685 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -122,11 +122,26 @@ func shellCommand(cmd string) []string { func (c Config) AgentKind() (agent.Kind, error) { switch c.Agent { - case string(agent.KindClaude): - return agent.KindClaude, nil case string(agent.KindCodex): return agent.KindCodex, nil default: - return "", fmt.Errorf("config: invalid agent %q: must be %q or %q", c.Agent, agent.KindClaude, agent.KindCodex) + return "", fmt.Errorf("config: unsupported agent %q; supported agents: %q", c.Agent, agent.KindCodex) } } + +func (c Config) Validate() error { + if c.Review.Required && c.Agent == "" { + return fmt.Errorf("config: review requires agent %q", agent.KindCodex) + } + if c.Agent != "" { + if _, err := c.AgentKind(); err != nil { + return err + } + } + for index, configured := range c.Agents { + if configured != string(agent.KindCodex) { + return fmt.Errorf("config: unsupported agent %q at agents[%d]; supported agents: %q", configured, index, agent.KindCodex) + } + } + return nil +} diff --git a/internal/config/config_extended_test.go b/internal/config/config_extended_test.go index 486059a..f0b7c04 100644 --- a/internal/config/config_extended_test.go +++ b/internal/config/config_extended_test.go @@ -22,7 +22,7 @@ test: commands: test: trusted-test-cmd lint: trusted-lint-cmd -agent: trusted-agent +agent: codex allow_repo_commands: false ` @@ -152,15 +152,12 @@ func TestConfig_LintCommandReturnsNilWhenEmpty(t *testing.T) { } } -func TestConfig_AgentKindMapsClaude(t *testing.T) { +func TestConfig_AgentKindRejectsClaude(t *testing.T) { cfg := Config{Agent: "claude"} - kind, err := cfg.AgentKind() - if err != nil { - t.Fatalf("AgentKind() returned unexpected error: %v", err) - } - if kind != agent.KindClaude { - t.Errorf("AgentKind() = %v, want %v", kind, agent.KindClaude) + _, err := cfg.AgentKind() + if err == nil || !strings.Contains(err.Error(), "supported agents") || !strings.Contains(err.Error(), "codex") { + t.Fatalf("AgentKind() error = %v, want actionable unsupported-agent error", err) } } @@ -196,3 +193,15 @@ func TestConfig_AgentKindFailsClosedOnUnrecognizedValue(t *testing.T) { t.Errorf("AgentKind() error = %q, want it to name the invalid value %q", err.Error(), "gpt4") } } + +func TestLoadEffectiveConfig_RejectsUnsupportedAgentBeforePipelineStages(t *testing.T) { + for _, unsupported := range []string{"claude", "gpt4"} { + t.Run(unsupported, func(t *testing.T) { + path := writeConfigFile(t, t.TempDir(), ".made.yml", "version: 1\nreview:\n required: true\nagent: "+unsupported+"\n") + _, err := LoadEffectiveConfig(path, "") + if err == nil || !strings.Contains(err.Error(), "supported agents") || !strings.Contains(err.Error(), "codex") { + t.Fatalf("LoadEffectiveConfig error = %v, want actionable unsupported-agent validation", err) + } + }) + } +} diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 9ea9354..030da39 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -32,10 +32,10 @@ test: commands: test: trusted-test-cmd lint: trusted-lint-cmd -agent: trusted-agent +agent: codex agents: - - trusted-agent-1 - - trusted-agent-2 + - codex + - codex allow_repo_commands: false ` @@ -56,9 +56,9 @@ test: commands: test: trusted-test-cmd lint: trusted-lint-cmd -agent: trusted-agent +agent: codex agents: - - trusted-agent-1 + - codex allow_repo_commands: true ` @@ -79,9 +79,9 @@ test: commands: test: pushed-test-cmd lint: pushed-lint-cmd -agent: pushed-agent +agent: codex agents: - - pushed-agent-1 + - codex allow_repo_commands: true ` @@ -136,10 +136,10 @@ func TestLoadEffectiveConfig_RuleB_CommandsTrustedByDefault(t *testing.T) { if cfg.Commands.Lint != "trusted-lint-cmd" { t.Errorf("Commands.Lint = %q, want trusted copy's value %q", cfg.Commands.Lint, "trusted-lint-cmd") } - if cfg.Agent != "trusted-agent" { - t.Errorf("Agent = %q, want trusted copy's value %q", cfg.Agent, "trusted-agent") + if cfg.Agent != "codex" { + t.Errorf("Agent = %q, want trusted copy's value %q", cfg.Agent, "codex") } - if len(cfg.Agents) != 2 || cfg.Agents[0] != "trusted-agent-1" { + if len(cfg.Agents) != 2 || cfg.Agents[0] != "codex" { t.Errorf("Agents = %v, want trusted copy's value", cfg.Agents) } } @@ -163,10 +163,10 @@ func TestLoadEffectiveConfig_RuleB_PushedHonoredWhenAllowRepoCommands(t *testing if cfg.Commands.Lint != "pushed-lint-cmd" { t.Errorf("Commands.Lint = %q, want pushed copy's value %q", cfg.Commands.Lint, "pushed-lint-cmd") } - if cfg.Agent != "pushed-agent" { - t.Errorf("Agent = %q, want pushed copy's value %q", cfg.Agent, "pushed-agent") + if cfg.Agent != "codex" { + t.Errorf("Agent = %q, want pushed copy's value %q", cfg.Agent, "codex") } - if len(cfg.Agents) != 1 || cfg.Agents[0] != "pushed-agent-1" { + if len(cfg.Agents) != 1 || cfg.Agents[0] != "codex" { t.Errorf("Agents = %v, want pushed copy's value", cfg.Agents) } diff --git a/internal/config/file.go b/internal/config/file.go index 09629f5..75f1f16 100644 --- a/internal/config/file.go +++ b/internal/config/file.go @@ -64,6 +64,9 @@ func LoadEffectiveConfig(trustedPath, pushedPath string) (Config, error) { effective.Agent = trusted.Agent effective.Agents = trusted.Agents } + if err := effective.Validate(); err != nil { + return Config{}, fmt.Errorf("config: validate effective configuration: %w", err) + } return effective, nil } diff --git a/internal/skill/skill.go b/internal/skill/skill.go index cffb387..cb403fe 100644 --- a/internal/skill/skill.go +++ b/internal/skill/skill.go @@ -39,8 +39,8 @@ const body = ` ` + "`made`" + ` is a local validation-gate daemon: pushing a branch to its bare gate repo runs a 9-stage pipeline (Intent, Rebase, Review, Test, Document, Lint, Push, PR, CI) against the change before it ever reaches the real remote. It is -GitHub-only (via ` + "`gh`" + `) and drives Claude or Codex as the pipeline's review -and document agent. You drive it through the ` + "`made`" + ` CLI, which talks to a +GitHub-only (via ` + "`gh`" + `) and drives Codex as the pipeline's review and +document agent. You drive it through the ` + "`made`" + ` CLI, which talks to a per-user background daemon over a unix socket and reports state as JSON. ## Two ways to invoke diff --git a/internal/skill/skill_test.go b/internal/skill/skill_test.go index dc9e57d..181d1da 100644 --- a/internal/skill/skill_test.go +++ b/internal/skill/skill_test.go @@ -61,3 +61,13 @@ func TestBodyDoesNotClaimPushBlocks(t *testing.T) { t.Error(`skill.Markdown() contains "blocks until": the pipeline is asynchronous, a push must not be described as blocking until a terminal state`) } } + +func TestBodyAdvertisesOnlySupportedCodexAgent(t *testing.T) { + body := skill.Markdown() + if strings.Contains(strings.ToLower(body), "claude") { + t.Fatalf("generated skill advertises unsupported Claude support") + } + if !strings.Contains(body, "Codex") { + t.Fatalf("generated skill omitted supported Codex review agent") + } +} diff --git a/skills/made/SKILL.md b/skills/made/SKILL.md index 55a5830..fe6b6bf 100644 --- a/skills/made/SKILL.md +++ b/skills/made/SKILL.md @@ -9,8 +9,8 @@ user-invocable: true `made` is a local validation-gate daemon: pushing a branch to its bare gate repo runs a 9-stage pipeline (Intent, Rebase, Review, Test, Document, Lint, Push, PR, CI) against the change before it ever reaches the real remote. It is -GitHub-only (via `gh`) and drives Claude or Codex as the pipeline's review -and document agent. You drive it through the `made` CLI, which talks to a +GitHub-only (via `gh`) and drives Codex as the pipeline's review and +document agent. You drive it through the `made` CLI, which talks to a per-user background daemon over a unix socket and reports state as JSON. ## Two ways to invoke From 4c21dc8664d5a0acabfab1a1e62ef78e4d862e17 Mon Sep 17 00:00:00 2001 From: Doug Jarquin Date: Tue, 18 Aug 2026 10:49:39 -0400 Subject: [PATCH 3/9] fix(agent): normalize structured event errors --- internal/agent/spawn.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/agent/spawn.go b/internal/agent/spawn.go index c5bcca8..694c98c 100644 --- a/internal/agent/spawn.go +++ b/internal/agent/spawn.go @@ -177,7 +177,7 @@ func extractStructuredResponse(data []byte) ([]byte, error) { } switch event.Type { case "error", "turn.failed": - return nil, fmt.Errorf("Codex returned a failed event") + return nil, fmt.Errorf("codex returned a failed event") case "item.completed": if event.Item.Type == "agent_message" && strings.TrimSpace(event.Item.Text) != "" { response = []byte(event.Item.Text) From 06f5f11fe880c5ea644fe363ec69f08956b8ebc4 Mon Sep 17 00:00:00 2001 From: Doug Jarquin Date: Tue, 18 Aug 2026 11:02:18 -0400 Subject: [PATCH 4/9] fix(review): restore isolation and output identity Intent: preserve Codex review isolation and candidate identity --- cmd/made/daemon.go | 2 +- docs/remediation/made-remediation-p1p3b.md | 4 ++-- internal/agent/agent_contract_test.go | 3 ++- internal/agent/remediation_contract_test.go | 10 ++++++++-- internal/agent/spawn.go | 4 ++-- internal/agent/testdata/fakeagent/main.go | 8 ++++---- internal/orchestrator/workfunc.go | 4 +++- internal/orchestrator/workfunc_test.go | 12 +++++++++++- 8 files changed, 33 insertions(+), 14 deletions(-) diff --git a/cmd/made/daemon.go b/cmd/made/daemon.go index f6b61b8..4ebd5a3 100644 --- a/cmd/made/daemon.go +++ b/cmd/made/daemon.go @@ -480,7 +480,7 @@ func gateNotifyPushHandler(rm *daemon.RunManager, reviewDecisions *daemon.Review work := func(workCtx context.Context, emit func(daemon.Event)) error { return orchestrator.Run(workCtx, gatePath, defaultBranch, worktreesDir, runID, newSHA, - orchestrator.NewWorkFunc(rm, reviewDecisions, emit, runID, defaultBranch, branch, orchestrator.Options{})) + orchestrator.NewWorkFunc(rm, reviewDecisions, emit, runID, defaultBranch, branch, orchestrator.Options{CandidateOutputSHA: p.OutputSHA})) } submissionID := p.SubmissionID diff --git a/docs/remediation/made-remediation-p1p3b.md b/docs/remediation/made-remediation-p1p3b.md index 461c9e6..5f729a1 100644 --- a/docs/remediation/made-remediation-p1p3b.md +++ b/docs/remediation/made-remediation-p1p3b.md @@ -22,7 +22,7 @@ The signing override was needed because the inherited global SSH signing configu The installed Codex CLI was `/opt/homebrew/bin/codex` version `codex-cli 0.147.0`. -The supported invocation is `codex exec --cd --json --output-schema -`. +The supported invocation is `codex exec --cd --json --output-schema --sandbox read-only --ephemeral -`. ## Phase 1 RED contract @@ -54,7 +54,7 @@ Those failures prove missing production boundaries rather than fixture defects b The compatibility fake GitHub boundary rejected PR URLs where workflow run IDs were required and modeled check status, conclusion, workflow run ID, and details URL. -The fake Codex boundary accepted only the installed `codex exec --cd --json --output-schema` invocation and strict structured output. +The fake Codex boundary accepted only the installed `codex exec --cd --json --output-schema --sandbox read-only --ephemeral` invocation and strict structured output. The old mocks were updated or removed so they do not authorize commands that the real Made binary does not implement. diff --git a/internal/agent/agent_contract_test.go b/internal/agent/agent_contract_test.go index 69b09f2..1ef756d 100644 --- a/internal/agent/agent_contract_test.go +++ b/internal/agent/agent_contract_test.go @@ -37,7 +37,7 @@ func TestSpawn_CodexUsesStructuredExecContract(t *testing.T) { if err != nil { t.Fatalf("read invocation log: %v", err) } - for _, token := range []string{"exec", "--cd", "--json", "--output-schema", "-"} { + for _, token := range []string{"exec", "--cd", "--json", "--output-schema", "--sandbox", "read-only", "--ephemeral", "-"} { if !strings.Contains(string(data), token) { t.Fatalf("expected Codex structured invocation token %q, got %s", token, data) } @@ -66,6 +66,7 @@ func TestSpawn_DoesNotPassSensitiveEnvironmentToCodex(t *testing.T) { "COOKIE=must-not-reach-review-agent", "JWT_KEY=must-not-reach-review-agent", "KUBECONFIG=/must-not-reach-review-agent", + "LC_REVIEW_SECRET=must-not-reach-review-agent", }, }); err != nil { t.Fatalf("Spawn exposed sensitive environment: %v", err) diff --git a/internal/agent/remediation_contract_test.go b/internal/agent/remediation_contract_test.go index c681731..c846085 100644 --- a/internal/agent/remediation_contract_test.go +++ b/internal/agent/remediation_contract_test.go @@ -42,7 +42,10 @@ func TestSpawn_CodexUsesSupportedExecStructuredContract(t *testing.T) { "[ \"$4\" = \"--json\" ]", "[ \"$5\" = \"--output-schema\" ]", "[ -f \"$6\" ]", - "[ \"$7\" = \"-\" ]", + "[ \"$7\" = \"--sandbox\" ]", + "[ \"$8\" = \"read-only\" ]", + "[ \"$9\" = \"--ephemeral\" ]", + "[ \"${10}\" = \"-\" ]", "[ \"$(git -C \"$3\" rev-parse HEAD)\" = " + shellQuote(head) + " ]", "if (umask 077; : > \"$3/.agent-write-probe\") 2>/dev/null; then exit 1; fi", "test -z \"${MADE_REVIEW_SECRET:-}\"", @@ -98,7 +101,10 @@ func TestSpawn_TrustedBaseIsResolvableInDetachedReviewCopy(t *testing.T) { "[ \"$4\" = \"--json\" ]", "[ \"$5\" = \"--output-schema\" ]", "[ -f \"$6\" ]", - "[ \"$7\" = \"-\" ]", + "[ \"$7\" = \"--sandbox\" ]", + "[ \"$8\" = \"read-only\" ]", + "[ \"$9\" = \"--ephemeral\" ]", + "[ \"${10}\" = \"-\" ]", "git -C \"$3\" cat-file -e \"$FAKE_AGENT_BASE_SHA^{commit}\"", "printf '%s\\n' '{\"findings\":[]}'", "", diff --git a/internal/agent/spawn.go b/internal/agent/spawn.go index 694c98c..e0780d7 100644 --- a/internal/agent/spawn.go +++ b/internal/agent/spawn.go @@ -128,7 +128,7 @@ func reviewEnvironmentKey(name string) bool { "FAKE_AGENT_KIND", "FAKE_AGENT_SCENARIO", "FAKE_AGENT_LOG_FILE", "FAKE_AGENT_EXIT_CODE", "FAKE_AGENT_WRITE_PATH", "FAKE_AGENT_WRITE_DATA", "FAKE_AGENT_BASE_SHA": return true } - return strings.HasPrefix(name, "LC_") + return false } func invocation(kind Kind, worktree string) ([]string, func(), error) { @@ -144,7 +144,7 @@ func invocation(kind Kind, worktree string) ([]string, func(), error) { _ = os.RemoveAll(dir) return nil, nil, fmt.Errorf("agent: write Codex output schema: %w", err) } - return []string{"exec", "--cd", worktree, "--json", "--output-schema", schemaPath, "-"}, func() { _ = os.RemoveAll(dir) }, nil + return []string{"exec", "--cd", worktree, "--json", "--output-schema", schemaPath, "--sandbox", "read-only", "--ephemeral", "-"}, func() { _ = os.RemoveAll(dir) }, nil } func extractStructuredResponse(data []byte) ([]byte, error) { diff --git a/internal/agent/testdata/fakeagent/main.go b/internal/agent/testdata/fakeagent/main.go index 835de17..060cc7e 100644 --- a/internal/agent/testdata/fakeagent/main.go +++ b/internal/agent/testdata/fakeagent/main.go @@ -23,7 +23,7 @@ func main() { fmt.Fprintf(os.Stderr, "fakeagent: invalid invocation: %v\n", err) os.Exit(2) } - for _, key := range []string{"MADE_TEST_SECRET", "DATABASE_URL", "COOKIE", "JWT_KEY", "KUBECONFIG"} { + for _, key := range []string{"MADE_TEST_SECRET", "DATABASE_URL", "COOKIE", "JWT_KEY", "KUBECONFIG", "LC_REVIEW_SECRET"} { if os.Getenv(key) != "" { fmt.Fprintf(os.Stderr, "fakeagent: sensitive environment %s was exposed\n", key) os.Exit(3) @@ -68,10 +68,10 @@ func main() { const agentKindCodex = "codex" func validateInvocation(args []string) error { - if len(args) != 7 { - return fmt.Errorf("want 7 arguments, got %d", len(args)) + if len(args) != 10 { + return fmt.Errorf("want 10 arguments, got %d", len(args)) } - if args[0] != "exec" || args[1] != "--cd" || args[3] != "--json" || args[4] != "--output-schema" || args[6] != "-" { + if args[0] != "exec" || args[1] != "--cd" || args[3] != "--json" || args[4] != "--output-schema" || args[6] != "--sandbox" || args[7] != "read-only" || args[8] != "--ephemeral" || args[9] != "-" { return fmt.Errorf("expected codex exec structured flags, got %v", args) } if filepath.IsAbs(args[2]) == false || filepath.IsAbs(args[5]) == false { diff --git a/internal/orchestrator/workfunc.go b/internal/orchestrator/workfunc.go index a4bc9f3..7c98f52 100644 --- a/internal/orchestrator/workfunc.go +++ b/internal/orchestrator/workfunc.go @@ -45,7 +45,8 @@ const ( // from the resolved RunContext.Config instead, since Config itself is only // resolved at Setup time, after a WorkFunc closure is already built. type Options struct { - ReviewOptions review.Options + ReviewOptions review.Options + CandidateOutputSHA string } // NewWorkFunc builds the real 9-stage chain (Intent -> Rebase -> Review -> @@ -253,6 +254,7 @@ func (c *chain) reviewStage() error { reviewOptions := c.opts.ReviewOptions reviewOptions.BaseBranch = c.defaultBranch + reviewOptions.CandidateOutputSHA = c.opts.CandidateOutputSHA reviewOptions.Evidence = c.rc.Evidence reviewOptions.EvidenceRunID = c.runID result, err := review.Run(c.ctx, c.rc.Worktree.Path, agentKind, reviewOptions) diff --git a/internal/orchestrator/workfunc_test.go b/internal/orchestrator/workfunc_test.go index b850baa..bf24cf3 100644 --- a/internal/orchestrator/workfunc_test.go +++ b/internal/orchestrator/workfunc_test.go @@ -238,9 +238,11 @@ func TestNewWorkFunc_FullPassEndsRunningWithAwaitingMergeMessage(t *testing.T) { rm := daemon.NewRunManager() reviewDecisions := daemon.NewReviewDecisions() runID := rm.NewRunID() + expectedOutputSHA := strings.Repeat("d", 40) wf := NewWorkFunc(rm, reviewDecisions, nil, runID, f.defaultBranch, branch, Options{ - ReviewOptions: cleanReviewOptions(t), + ReviewOptions: cleanReviewOptions(t), + CandidateOutputSHA: expectedOutputSHA, }) submitWorkFunc(t, rm, runID, "repo-full-pass", branch, wf, rc) @@ -254,6 +256,14 @@ func TestNewWorkFunc_FullPassEndsRunningWithAwaitingMergeMessage(t *testing.T) { if len(snap.OutputSHA) != 40 { t.Fatalf("expected durable output SHA after push preparation, got %q", snap.OutputSHA) } + metadataPath := filepath.Join(wt.Path, ".made", "evidence", runID, "review-contract.json") + metadata, err := os.ReadFile(metadataPath) + if err != nil { + t.Fatalf("read review contract evidence: %v", err) + } + if !strings.Contains(string(metadata), `"candidate_output_sha":"`+expectedOutputSHA+`"`) { + t.Fatalf("review contract evidence omitted submitted candidate output SHA %q: %s", expectedOutputSHA, metadata) + } assertAllStagesPassed(t, snap.Stages) if !f.branchOnRealRemote(t, branch) { From 174ac2e0f1395a1a6e1eeaf45600dbf0542d7dcc Mon Sep 17 00:00:00 2001 From: Douglas Jarquin <8209+douglasjarquin@users.noreply.github.com> Date: Tue, 18 Aug 2026 11:28:07 -0400 Subject: [PATCH 5/9] fix(review): enforce finding schema fields --- internal/agent/agent_contract_test.go | 6 ++-- internal/agent/findings.go | 49 ++++++++++++++++++++++----- internal/agent/review_schema_test.go | 26 ++++++++++++++ 3 files changed, 70 insertions(+), 11 deletions(-) diff --git a/internal/agent/agent_contract_test.go b/internal/agent/agent_contract_test.go index 1ef756d..7bd138f 100644 --- a/internal/agent/agent_contract_test.go +++ b/internal/agent/agent_contract_test.go @@ -42,8 +42,10 @@ func TestSpawn_CodexUsesStructuredExecContract(t *testing.T) { t.Fatalf("expected Codex structured invocation token %q, got %s", token, data) } } - if !strings.Contains(string(data), "task=inspect the candidate diff and return structured findings") { - t.Fatalf("expected task on Codex stdin, got %s", data) + log := string(data) + taskStart := strings.Index(log, "task=") + if taskStart < 0 || strings.TrimSpace(log[taskStart+len("task="):]) == "" { + t.Fatalf("expected non-empty task on Codex stdin, got %s", data) } } diff --git a/internal/agent/findings.go b/internal/agent/findings.go index 918f96e..36b0fea 100644 --- a/internal/agent/findings.go +++ b/internal/agent/findings.go @@ -21,28 +21,59 @@ type Finding struct { Paths []string `json:"paths,omitempty"` } +func (f Finding) MarshalJSON() ([]byte, error) { + var patch *string + if f.Patch != "" { + patch = &f.Patch + } + var paths []string + if f.Paths != nil { + paths = append([]string(nil), f.Paths...) + } + return json.Marshal(struct { + Kind FindingKind `json:"kind"` + Description string `json:"description"` + Patch *string `json:"patch"` + Paths []string `json:"paths"` + }{ + Kind: f.Kind, + Description: f.Description, + Patch: patch, + Paths: paths, + }) +} + func (f *Finding) UnmarshalJSON(data []byte) error { var wire struct { - Kind *FindingKind `json:"kind"` - Description *string `json:"description"` - Patch *string `json:"patch"` - Paths []string `json:"paths"` + Kind *FindingKind `json:"kind"` + Description *string `json:"description"` + Patch json.RawMessage `json:"patch"` + Paths json.RawMessage `json:"paths"` } decoder := json.NewDecoder(bytes.NewReader(data)) decoder.DisallowUnknownFields() if err := decoder.Decode(&wire); err != nil { return err } - if wire.Kind == nil || wire.Description == nil { - return fmt.Errorf("finding requires kind and description") + if wire.Kind == nil || wire.Description == nil || len(wire.Patch) == 0 || len(wire.Paths) == 0 { + return fmt.Errorf("finding requires kind, description, patch, and paths") } f.Kind = *wire.Kind f.Description = *wire.Description f.Patch = "" - if wire.Patch != nil { - f.Patch = *wire.Patch + f.Paths = nil + if !bytes.Equal(bytes.TrimSpace(wire.Patch), []byte("null")) { + if err := json.Unmarshal(wire.Patch, &f.Patch); err != nil { + return fmt.Errorf("finding patch must be a string or null: %w", err) + } + } + if !bytes.Equal(bytes.TrimSpace(wire.Paths), []byte("null")) { + var paths []string + if err := json.Unmarshal(wire.Paths, &paths); err != nil { + return fmt.Errorf("finding paths must be an array or null: %w", err) + } + f.Paths = append([]string(nil), paths...) } - f.Paths = append([]string(nil), wire.Paths...) return nil } diff --git a/internal/agent/review_schema_test.go b/internal/agent/review_schema_test.go index 1e07e73..d665bff 100644 --- a/internal/agent/review_schema_test.go +++ b/internal/agent/review_schema_test.go @@ -38,3 +38,29 @@ func TestReviewSchemaRequiresEveryFindingProperty(t *testing.T) { } } } + +func TestStrictFindingsRejectsMissingRequiredNullableProperties(t *testing.T) { + for _, test := range []struct { + name string + data string + }{ + {name: "missing patch", data: `{"findings":[{"kind":"ask-user","description":"needs a decision","paths":null}]}`}, + {name: "missing paths", data: `{"findings":[{"kind":"ask-user","description":"needs a decision","patch":null}]}`}, + } { + t.Run(test.name, func(t *testing.T) { + if _, err := strictFindings([]byte(test.data)); err == nil { + t.Fatal("expected missing required property to fail closed") + } + }) + } +} + +func TestStrictFindingsAcceptsExplicitNullPropertiesForNonAutoFixes(t *testing.T) { + findings, err := strictFindings([]byte(`{"findings":[{"kind":"ask-user","description":"needs a decision","patch":null,"paths":null}]}`)) + if err != nil { + t.Fatalf("strictFindings: %v", err) + } + if len(findings.Findings) != 1 || findings.Findings[0].Patch != "" || findings.Findings[0].Paths != nil { + t.Fatalf("unexpected finding: %+v", findings.Findings) + } +} From a9b083d0e7c4c2d4e49e9fffe848374896073b82 Mon Sep 17 00:00:00 2001 From: Douglas Jarquin <8209+douglasjarquin@users.noreply.github.com> Date: Tue, 18 Aug 2026 11:32:20 -0400 Subject: [PATCH 6/9] test(review): keep contract checks tool-agnostic --- internal/agent/agent_contract_test.go | 4 ++-- internal/agent/review_schema_test.go | 10 ++-------- internal/agent/spawn.go | 2 +- 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/internal/agent/agent_contract_test.go b/internal/agent/agent_contract_test.go index 7bd138f..44d8e27 100644 --- a/internal/agent/agent_contract_test.go +++ b/internal/agent/agent_contract_test.go @@ -43,8 +43,8 @@ func TestSpawn_CodexUsesStructuredExecContract(t *testing.T) { } } log := string(data) - taskStart := strings.Index(log, "task=") - if taskStart < 0 || strings.TrimSpace(log[taskStart+len("task="):]) == "" { + _, task, ok := strings.Cut(log, "task=") + if !ok || strings.TrimSpace(task) == "" { t.Fatalf("expected non-empty task on Codex stdin, got %s", data) } } diff --git a/internal/agent/review_schema_test.go b/internal/agent/review_schema_test.go index d665bff..b193e7b 100644 --- a/internal/agent/review_schema_test.go +++ b/internal/agent/review_schema_test.go @@ -2,6 +2,7 @@ package agent import ( "encoding/json" + "slices" "testing" ) @@ -26,14 +27,7 @@ func TestReviewSchemaRequiresEveryFindingProperty(t *testing.T) { if _, ok := schema.Properties.Findings.Items.Properties[property]; !ok { t.Fatalf("review schema missing finding property %q", property) } - found := false - for _, required := range schema.Properties.Findings.Items.Required { - if required == property { - found = true - break - } - } - if !found { + if !slices.Contains(schema.Properties.Findings.Items.Required, property) { t.Fatalf("review schema does not require finding property %q", property) } } diff --git a/internal/agent/spawn.go b/internal/agent/spawn.go index e0780d7..9cdf571 100644 --- a/internal/agent/spawn.go +++ b/internal/agent/spawn.go @@ -160,7 +160,7 @@ func extractStructuredResponse(data []byte) ([]byte, error) { } var response []byte - for _, line := range bytes.Split(trimmed, []byte{'\n'}) { + for line := range bytes.SplitSeq(trimmed, []byte{'\n'}) { line = bytes.TrimSpace(line) if len(line) == 0 { continue From 4489f5a73cbe2158c48d6bcd78a5003920fac2b7 Mon Sep 17 00:00:00 2001 From: Douglas Jarquin <8209+douglasjarquin@users.noreply.github.com> Date: Tue, 18 Aug 2026 11:33:14 -0400 Subject: [PATCH 7/9] test(review): cover nullable finding fields --- internal/agent/review_schema_test.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/internal/agent/review_schema_test.go b/internal/agent/review_schema_test.go index b193e7b..1062566 100644 --- a/internal/agent/review_schema_test.go +++ b/internal/agent/review_schema_test.go @@ -58,3 +58,27 @@ func TestStrictFindingsAcceptsExplicitNullPropertiesForNonAutoFixes(t *testing.T t.Fatalf("unexpected finding: %+v", findings.Findings) } } + +func TestFindingMarshalIncludesRequiredNullableProperties(t *testing.T) { + data, err := json.Marshal(Findings{Findings: []Finding{{Kind: FindingAskUser, Description: "needs a decision"}}}) + if err != nil { + t.Fatalf("marshal findings: %v", err) + } + var finding map[string]json.RawMessage + var payload struct { + Findings []map[string]json.RawMessage `json:"findings"` + } + if err := json.Unmarshal(data, &payload); err != nil { + t.Fatalf("unmarshal findings: %v", err) + } + if len(payload.Findings) != 1 { + t.Fatalf("unexpected findings payload: %s", data) + } + finding = payload.Findings[0] + for _, property := range []string{"patch", "paths"} { + value, ok := finding[property] + if !ok || string(value) != "null" { + t.Fatalf("serialized finding %s = %s, want explicit null", property, value) + } + } +} From ac65cd3e4bfb7ff36f60c8b3ddcdd667040bd91c Mon Sep 17 00:00:00 2001 From: Douglas Jarquin <8209+douglasjarquin@users.noreply.github.com> Date: Tue, 18 Aug 2026 11:40:43 -0400 Subject: [PATCH 8/9] fix(review): bind evidence to resolved output --- internal/orchestrator/workfunc_test.go | 17 ++++++++++++----- internal/pipeline/review/autofix.go | 2 +- internal/pipeline/review/contract.go | 5 ++++- .../pipeline/review/evidence_contract_test.go | 3 +-- internal/pipeline/review/git.go | 2 +- internal/pipeline/review/review.go | 9 ++++++--- .../pipeline/review/review_contract_test.go | 16 ++++++++++++++++ internal/skill/skill_test.go | 6 ++++-- 8 files changed, 45 insertions(+), 15 deletions(-) diff --git a/internal/orchestrator/workfunc_test.go b/internal/orchestrator/workfunc_test.go index bf24cf3..fd49ca9 100644 --- a/internal/orchestrator/workfunc_test.go +++ b/internal/orchestrator/workfunc_test.go @@ -238,11 +238,9 @@ func TestNewWorkFunc_FullPassEndsRunningWithAwaitingMergeMessage(t *testing.T) { rm := daemon.NewRunManager() reviewDecisions := daemon.NewReviewDecisions() runID := rm.NewRunID() - expectedOutputSHA := strings.Repeat("d", 40) wf := NewWorkFunc(rm, reviewDecisions, nil, runID, f.defaultBranch, branch, Options{ - ReviewOptions: cleanReviewOptions(t), - CandidateOutputSHA: expectedOutputSHA, + ReviewOptions: cleanReviewOptions(t), }) submitWorkFunc(t, rm, runID, "repo-full-pass", branch, wf, rc) @@ -261,8 +259,17 @@ func TestNewWorkFunc_FullPassEndsRunningWithAwaitingMergeMessage(t *testing.T) { if err != nil { t.Fatalf("read review contract evidence: %v", err) } - if !strings.Contains(string(metadata), `"candidate_output_sha":"`+expectedOutputSHA+`"`) { - t.Fatalf("review contract evidence omitted submitted candidate output SHA %q: %s", expectedOutputSHA, metadata) + var contract struct { + CandidateOutputSHA string `json:"candidate_output_sha"` + } + if err := json.Unmarshal(metadata, &contract); err != nil { + t.Fatalf("decode review contract evidence: %v", err) + } + if len(contract.CandidateOutputSHA) != 40 { + t.Fatalf("review contract evidence omitted a full candidate output SHA: %s", metadata) + } + if err := exec.Command("git", "-C", wt.Path, "cat-file", "-e", contract.CandidateOutputSHA+"^{commit}").Run(); err != nil { + t.Fatalf("review contract candidate output SHA %q is not a commit in the prepared worktree: %v", contract.CandidateOutputSHA, err) } assertAllStagesPassed(t, snap.Stages) diff --git a/internal/pipeline/review/autofix.go b/internal/pipeline/review/autofix.go index 3a93e84..ce0367a 100644 --- a/internal/pipeline/review/autofix.go +++ b/internal/pipeline/review/autofix.go @@ -138,7 +138,7 @@ func runGitWithIndex(ctx context.Context, worktreePath, indexPath string, stdin func patchPaths(patch string) ([]string, error) { seen := make(map[string]struct{}) var oldPath string - for _, line := range strings.Split(patch, "\n") { + for line := range strings.SplitSeq(patch, "\n") { if strings.HasPrefix(line, "--- ") { var err error oldPath, err = patchHeaderPath(strings.TrimPrefix(line, "--- ")) diff --git a/internal/pipeline/review/contract.go b/internal/pipeline/review/contract.go index 6abcbb1..14ef924 100644 --- a/internal/pipeline/review/contract.go +++ b/internal/pipeline/review/contract.go @@ -27,10 +27,13 @@ func resolveReviewTask(ctx context.Context, worktreePath string, opts Options) ( return agent.ReviewTask{}, fmt.Errorf("resolve trusted base %q: %w", baseBranch, err) } } + if opts.CandidateOutputSHA != "" && opts.CandidateOutputSHA != candidateSHA { + return agent.ReviewTask{}, fmt.Errorf("candidate output SHA %q does not match the current review candidate %q", opts.CandidateOutputSHA, candidateSHA) + } return agent.NewReviewTask(agent.ReviewInput{ TrustedBaseBranch: baseBranch, TrustedBaseSHA: baseSHA, CandidateInputSHA: candidateSHA, - CandidateOutputSHA: opts.CandidateOutputSHA, + CandidateOutputSHA: candidateSHA, }) } diff --git a/internal/pipeline/review/evidence_contract_test.go b/internal/pipeline/review/evidence_contract_test.go index 4c3ff73..e76a72f 100644 --- a/internal/pipeline/review/evidence_contract_test.go +++ b/internal/pipeline/review/evidence_contract_test.go @@ -3,7 +3,6 @@ package review_test import ( "context" "encoding/json" - "strings" "testing" "github.com/douglasjarquin/made/internal/agent" @@ -30,7 +29,7 @@ func TestRun_WritesVersionedReviewEvidenceWithCandidateOutputSHA(t *testing.T) { t.Cleanup(func() { _ = wt.Remove() }) scenarioPath := writeScenario(t, agent.Findings{}) store := &recordingEvidenceStore{} - candidateOutputSHA := strings.Repeat("c", 40) + candidateOutputSHA := headSHA(t, wt.Path) result, err := review.Run(context.Background(), wt.Path, agent.KindCodex, review.Options{ BinaryPath: bin, diff --git a/internal/pipeline/review/git.go b/internal/pipeline/review/git.go index e9adbca..8fb3341 100644 --- a/internal/pipeline/review/git.go +++ b/internal/pipeline/review/git.go @@ -79,7 +79,7 @@ func repositoryFilterOverrides(ctx context.Context, worktreePath string) ([]stri return nil, fmt.Errorf("inspect repository Git filters: output exceeded %d bytes", reviewGitLimit) } drivers := make(map[string]struct{}) - for _, key := range strings.Fields(string(result.Stdout)) { + for key := range strings.FieldsSeq(string(result.Stdout)) { prefix := strings.TrimPrefix(key, "filter.") dot := strings.LastIndexByte(prefix, '.') if dot <= 0 { diff --git a/internal/pipeline/review/review.go b/internal/pipeline/review/review.go index 0c3e5ef..e270ad3 100644 --- a/internal/pipeline/review/review.go +++ b/internal/pipeline/review/review.go @@ -112,9 +112,12 @@ func Run(ctx context.Context, worktreePath string, agentKind agent.Kind, opts Op PendingFindings: pending, } } - outputSHA := opts.CandidateOutputSHA - if len(postFixSHAs) > 0 { - outputSHA = postFixSHAs[len(postFixSHAs)-1] + outputSHA, err := gitOutput(ctx, worktreePath, "rev-parse", "--verify", "HEAD^{commit}") + if err != nil { + return Result{}, fmt.Errorf("review: resolve candidate output SHA: %w", err) + } + if len(postFixSHAs) > 0 && outputSHA != postFixSHAs[len(postFixSHAs)-1] { + return Result{}, fmt.Errorf("review: candidate output SHA %q does not match the last auto-fix commit %q", outputSHA, postFixSHAs[len(postFixSHAs)-1]) } if err := writeReviewEvidence(ctx, opts, task, spawned.Response, outputSHA); err != nil { return Result{}, err diff --git a/internal/pipeline/review/review_contract_test.go b/internal/pipeline/review/review_contract_test.go index bc121f8..803c99b 100644 --- a/internal/pipeline/review/review_contract_test.go +++ b/internal/pipeline/review/review_contract_test.go @@ -55,3 +55,19 @@ func TestRun_AutoFixDoesNotStageUnrelatedChanges(t *testing.T) { t.Fatalf("unrelated fixture disappeared: %v", err) } } + +func TestRun_RejectsUnresolvedCandidateOutputSHA(t *testing.T) { + bin := agenttest.Build(t) + f := setupFixture(t) + wt := f.addWorktree(t) + t.Cleanup(func() { _ = wt.Remove() }) + + _, err := review.Run(context.Background(), wt.Path, agent.KindCodex, review.Options{ + BinaryPath: bin, + BaseBranch: "HEAD", + CandidateOutputSHA: strings.Repeat("d", 40), + }) + if err == nil || !strings.Contains(err.Error(), "does not match the current review candidate") { + t.Fatalf("review.Run error = %v, want unresolved candidate output rejection", err) + } +} diff --git a/internal/skill/skill_test.go b/internal/skill/skill_test.go index 181d1da..f936cec 100644 --- a/internal/skill/skill_test.go +++ b/internal/skill/skill_test.go @@ -57,8 +57,10 @@ func TestCommittedSkillFileMatchesGenerator(t *testing.T) { // via `made run status --json `), so the body must never regress to claiming a // push blocks until the pipeline finishes. func TestBodyDoesNotClaimPushBlocks(t *testing.T) { - if strings.Contains(skill.Markdown(), "blocks until") { - t.Error(`skill.Markdown() contains "blocks until": the pipeline is asynchronous, a push must not be described as blocking until a terminal state`) + for line := range strings.SplitSeq(strings.ToLower(skill.Markdown()), "\n") { + if strings.Contains(line, "push") && strings.Contains(line, "blocks until") { + t.Errorf("skill.Markdown() describes push as blocking until a terminal state: %q", line) + } } } From 84aec895a410b72331d06817125e0fc4ff1fcbee Mon Sep 17 00:00:00 2001 From: Douglas Jarquin <8209+douglasjarquin@users.noreply.github.com> Date: Tue, 18 Aug 2026 11:54:18 -0400 Subject: [PATCH 9/9] fix(review): preserve verified output identity --- internal/pipeline/review/contract.go | 5 +---- internal/pipeline/review/review.go | 3 +++ internal/pipeline/review/review_contract_test.go | 7 ++++++- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/internal/pipeline/review/contract.go b/internal/pipeline/review/contract.go index 14ef924..6abcbb1 100644 --- a/internal/pipeline/review/contract.go +++ b/internal/pipeline/review/contract.go @@ -27,13 +27,10 @@ func resolveReviewTask(ctx context.Context, worktreePath string, opts Options) ( return agent.ReviewTask{}, fmt.Errorf("resolve trusted base %q: %w", baseBranch, err) } } - if opts.CandidateOutputSHA != "" && opts.CandidateOutputSHA != candidateSHA { - return agent.ReviewTask{}, fmt.Errorf("candidate output SHA %q does not match the current review candidate %q", opts.CandidateOutputSHA, candidateSHA) - } return agent.NewReviewTask(agent.ReviewInput{ TrustedBaseBranch: baseBranch, TrustedBaseSHA: baseSHA, CandidateInputSHA: candidateSHA, - CandidateOutputSHA: candidateSHA, + CandidateOutputSHA: opts.CandidateOutputSHA, }) } diff --git a/internal/pipeline/review/review.go b/internal/pipeline/review/review.go index e270ad3..aa39c51 100644 --- a/internal/pipeline/review/review.go +++ b/internal/pipeline/review/review.go @@ -119,6 +119,9 @@ func Run(ctx context.Context, worktreePath string, agentKind agent.Kind, opts Op if len(postFixSHAs) > 0 && outputSHA != postFixSHAs[len(postFixSHAs)-1] { return Result{}, fmt.Errorf("review: candidate output SHA %q does not match the last auto-fix commit %q", outputSHA, postFixSHAs[len(postFixSHAs)-1]) } + if opts.CandidateOutputSHA != "" && opts.CandidateOutputSHA != outputSHA { + return Result{}, fmt.Errorf("review: supplied candidate output SHA %q does not match resolved HEAD %q", opts.CandidateOutputSHA, outputSHA) + } if err := writeReviewEvidence(ctx, opts, task, spawned.Response, outputSHA); err != nil { return Result{}, err } diff --git a/internal/pipeline/review/review_contract_test.go b/internal/pipeline/review/review_contract_test.go index 803c99b..07b660c 100644 --- a/internal/pipeline/review/review_contract_test.go +++ b/internal/pipeline/review/review_contract_test.go @@ -61,13 +61,18 @@ func TestRun_RejectsUnresolvedCandidateOutputSHA(t *testing.T) { f := setupFixture(t) wt := f.addWorktree(t) t.Cleanup(func() { _ = wt.Remove() }) + scenarioPath := writeScenario(t, agent.Findings{}) _, err := review.Run(context.Background(), wt.Path, agent.KindCodex, review.Options{ BinaryPath: bin, BaseBranch: "HEAD", CandidateOutputSHA: strings.Repeat("d", 40), + ExtraEnv: []string{ + "FAKE_AGENT_KIND=codex", + "FAKE_AGENT_SCENARIO=" + scenarioPath, + }, }) - if err == nil || !strings.Contains(err.Error(), "does not match the current review candidate") { + if err == nil || !strings.Contains(err.Error(), "does not match resolved HEAD") { t.Fatalf("review.Run error = %v, want unresolved candidate output rejection", err) } }