From a5dbc51038312de23774b6aeb638f4fe34212227 Mon Sep 17 00:00:00 2001 From: PMCLSF Date: Sat, 2 May 2026 06:20:02 -0700 Subject: [PATCH] feat(0.2): report pr --fail-on + report impact --explain-selection (Tracks 3.1/3.2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of the five "load-bearing centerpiece" Track 3 deliverables that turn the unified pitch into something the binary actually delivers. Track 3.1 — `terrain report pr --fail-on ` Defends the pitch claim "Gate changes based on that system as a whole." Pre-fix, --fail-on existed only on `analyze`; the gating flow (`analyze && report pr`) silently lost the gate at the second step. Implementation reuses the same severityGateBlocked helper that PR #134 introduced for analyze. Added prSeverityBreakdown(severities []string) that converts a PR's NewFindings + AI BlockingSignals into the same SignalBreakdown shape `analyze.SignalSummary` uses, so the gate decision logic is shared, not duplicated. Wired through both the legacy `terrain pr` command (deprecated alias of `terrain report pr`) and the canonical `terrain report pr` namespace dispatcher. Same render-then-gate pattern as analyze: every output format (json, markdown, comment, annotation, default text) renders before the gate decision returns through the error channel. So `--json --fail-on=high` produces a valid JSON document on stdout AND exits with code 6 if the gate fired — the property the launch- readiness review's "JSON stdout purity" gate test asks for. Tests: - TestPRSeverityBreakdown: empty / mixed bag / case-insensitive / unknown-severities-dropped-silently table - cli_smoke_test.go updated for the new runPR signature Track 3.2 — `terrain report impact --explain-selection` Defends the pitch claim "See which tests matter for a PR — and why." Pre-fix, `report impact` showed selected tests but not the reason chains; the "and why" half of the pitch wasn't deliverable. Implementation reuses the existing internal/explain.ExplainSelection + reporting.RenderSelectionExplanation (already shipping for `terrain explain selection`). When --explain-selection is set, runImpact computes the selection explanation and renders it with verbose=true so per-test evidence (selection reasons, code unit matches, confidence) appears. Wired through both `terrain impact` (legacy) and `terrain report impact` (canonical). --json + --explain-selection emits the SelectionExplanation JSON structure for tooling consumption. Pillar parity impact: Track 3.1 + 3.2 are the centerpiece work that the plan calls "highest-priority track — every pitch claim must be directly verifiable in the CLI output before 0.2.0 ships." This PR closes two of the five Track 3 items; 3.3 (integration-test classification rigor), 3.4 (E2E attribution), and 3.5 (unified PR-comment audit) are separate follow-ups. Verification: go build ./... clean go test ./cmd/terrain/ green (4 new TestPRSeverityBreakdown cases; existing TestCLISmoke_PRCommand updated for new signature) go test ./... full suite green go test ./internal/testdata/ golden + CLI suite green Plan link: /Users/pzachary/.claude/plans/kind-mapping-turing.md (Tracks 3.1 / 3.2). Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/terrain/cli_smoke_test.go | 2 +- cmd/terrain/cmd_impact.go | 56 +++++++++++++++++++++++++-- cmd/terrain/cmd_report_namespace.go | 10 ++++- cmd/terrain/cmd_severity_gate.go | 38 ++++++++++++++++-- cmd/terrain/cmd_severity_gate_test.go | 50 ++++++++++++++++++++++++ cmd/terrain/main.go | 15 ++++++- 6 files changed, 159 insertions(+), 12 deletions(-) diff --git a/cmd/terrain/cli_smoke_test.go b/cmd/terrain/cli_smoke_test.go index c28aa0f8..0725d301 100644 --- a/cmd/terrain/cli_smoke_test.go +++ b/cmd/terrain/cli_smoke_test.go @@ -120,7 +120,7 @@ func TestCLISmoke_PRCommand(t *testing.T) { root := fixtureRoot(t) out, err := captureRun(func() error { - return runPR(root, "HEAD~1", true, "") + return runPR(root, "HEAD~1", true, "", severityGateNone) }) if err != nil { t.Errorf("pr failed: %v", err) diff --git a/cmd/terrain/cmd_impact.go b/cmd/terrain/cmd_impact.go index e9670ce3..ae6959d2 100644 --- a/cmd/terrain/cmd_impact.go +++ b/cmd/terrain/cmd_impact.go @@ -10,6 +10,7 @@ import ( "github.com/pmclSF/terrain/internal/changescope" "github.com/pmclSF/terrain/internal/depgraph" "github.com/pmclSF/terrain/internal/engine" + "github.com/pmclSF/terrain/internal/explain" "github.com/pmclSF/terrain/internal/impact" "github.com/pmclSF/terrain/internal/metrics" "github.com/pmclSF/terrain/internal/reporting" @@ -40,7 +41,7 @@ func runImpactPipeline(root, baseRef string, opts engine.PipelineOptions) (*impa return impactResult, result, nil } -func runImpact(root, baseRef string, jsonOutput bool, show, ownerFilter string) error { +func runImpact(root, baseRef string, jsonOutput bool, show, ownerFilter string, explainSelection bool) error { impactResult, _, err := runImpactPipeline(root, baseRef, defaultPipelineOptionsWithProgress(jsonOutput)) if err != nil { return err @@ -51,6 +52,27 @@ func runImpact(root, baseRef string, jsonOutput bool, show, ownerFilter string) impactResult = impact.FilterByOwner(impactResult, ownerFilter) } + // `--explain-selection` defends the pitch claim + // "see which tests matter for a PR — and why" (Track 3.2). Surfaces + // the structured reason chains that internal/explain produces and + // renders them via the existing RenderSelectionExplanation. Passes + // `verbose=true` so per-test evidence (selection reasons, code unit + // matches, confidence) is included; that's the whole point of the + // flag. + if explainSelection { + sel, err := explain.ExplainSelection(impactResult) + if err != nil { + return fmt.Errorf("could not build selection explanation: %w", err) + } + if jsonOutput { + enc := json.NewEncoder(os.Stdout) + enc.SetIndent("", " ") + return enc.Encode(sel) + } + reporting.RenderSelectionExplanation(os.Stdout, sel, true) + return nil + } + if jsonOutput { enc := json.NewEncoder(os.Stdout) enc.SetIndent("", " ") @@ -129,7 +151,7 @@ func applyImpactPolicy(impactResult *impact.ImpactResult, result *engine.Pipelin } } -func runPR(root, baseRef string, jsonOutput bool, format string) error { +func runPR(root, baseRef string, jsonOutput bool, format string, gate severityGate) error { impactResult, result, err := runImpactPipeline(root, baseRef, defaultPipelineOptionsWithProgress(jsonOutput)) if err != nil { return err @@ -137,10 +159,36 @@ func runPR(root, baseRef string, jsonOutput bool, format string) error { pr := changescope.AnalyzePRFromImpact(impactResult, result.Snapshot) + // Compute the gate decision BEFORE rendering so the report renders + // for every output format (json, markdown, comment, annotation, + // default text), AND the gate error returns through the same code + // path. Mirrors the pattern used by `runAnalyze` after the JSON- + // stdout-purity bug fix in PR #134 — the renderer always completes + // before the exit decision is made. + severities := make([]string, 0, len(pr.NewFindings)) + for _, f := range pr.NewFindings { + severities = append(severities, f.Severity) + } + if pr.AI != nil { + for _, s := range pr.AI.BlockingSignals { + severities = append(severities, s.Severity) + } + } + gateBlocked, gateSummary := severityGateBlocked(gate, prSeverityBreakdown(severities)) + gateErr := func() error { + if gateBlocked { + return fmt.Errorf("%w: --fail-on=%s matched %s", errSeverityGateBlocked, gate, gateSummary) + } + return nil + } + if jsonOutput { enc := json.NewEncoder(os.Stdout) enc.SetIndent("", " ") - return enc.Encode(pr) + if err := enc.Encode(pr); err != nil { + return err + } + return gateErr() } switch format { @@ -153,5 +201,5 @@ func runPR(root, baseRef string, jsonOutput bool, format string) error { default: changescope.RenderChangeScopedReport(os.Stdout, pr) } - return nil + return gateErr() } diff --git a/cmd/terrain/cmd_report_namespace.go b/cmd/terrain/cmd_report_namespace.go index f955ad4a..9fdb896b 100644 --- a/cmd/terrain/cmd_report_namespace.go +++ b/cmd/terrain/cmd_report_namespace.go @@ -200,9 +200,10 @@ func runReportImpactCLI(args []string) error { jsonOut := fs.Bool("json", false, "output JSON impact result") show := fs.String("show", "", "drill-down view: units, gaps, tests, owners, graph, selected") owner := fs.String("owner", "", "filter results by owner") + explainSelection := fs.Bool("explain-selection", false, "render the selection explanation: which tests matter for this PR — and why") _ = fs.Parse(args) mountPositionalAsRoot("report impact", fs.Args(), root) - return runImpact(*root, *baseRef, *jsonOut, *show, *owner) + return runImpact(*root, *baseRef, *jsonOut, *show, *owner, *explainSelection) } func runReportPRCLI(args []string) error { @@ -211,9 +212,14 @@ func runReportPRCLI(args []string) error { baseRef := fs.String("base", "", "git base ref for diff (default: HEAD~1)") jsonOut := fs.Bool("json", false, "output JSON PR analysis") format := fs.String("format", "", "output format: markdown, comment, annotation") + failOn := fs.String("fail-on", "", "exit non-zero when a finding at or above this severity is present (critical|high|medium)") _ = fs.Parse(args) mountPositionalAsRoot("report pr", fs.Args(), root) - return runPR(*root, *baseRef, *jsonOut, *format) + gate, err := parseSeverityGate(*failOn) + if err != nil { + return err + } + return runPR(*root, *baseRef, *jsonOut, *format, gate) } func runReportPostureCLI(args []string) error { diff --git a/cmd/terrain/cmd_severity_gate.go b/cmd/terrain/cmd_severity_gate.go index 3407220a..0e5c54c5 100644 --- a/cmd/terrain/cmd_severity_gate.go +++ b/cmd/terrain/cmd_severity_gate.go @@ -8,12 +8,44 @@ import ( "github.com/pmclSF/terrain/internal/analyze" ) -// errSeverityGateBlocked is the sentinel returned by runAnalyze when -// `--fail-on` matches at least one finding. main.go uses errors.Is to -// distinguish this from analysis errors and exit with +// errSeverityGateBlocked is the sentinel returned by runAnalyze and +// runPR when `--fail-on` matches at least one finding. main.go uses +// errors.Is to distinguish this from analysis errors and exit with // `exitSeverityGateBlock` (6) rather than the generic 1. var errSeverityGateBlocked = errors.New("severity gate blocked") +// prSeverityBreakdown converts a PR's change-scoped findings + AI +// blocking signals into the same SignalBreakdown shape that +// `analyze.SignalSummary` uses, so `severityGateBlocked` works +// uniformly across `terrain analyze --fail-on` and +// `terrain report pr --fail-on`. Track 3.1 — defends the pitch's +// "gate changes based on that system as a whole" claim by sharing +// the gate decision logic, not duplicating it. +// +// Counted by case-insensitive severity match. Unknown severities +// are dropped — the renderer is the source of truth for severity +// vocabulary. +func prSeverityBreakdown(severities []string) analyze.SignalBreakdown { + var b analyze.SignalBreakdown + for _, sev := range severities { + switch strings.ToLower(strings.TrimSpace(sev)) { + case "critical": + b.Critical++ + b.Total++ + case "high": + b.High++ + b.Total++ + case "medium": + b.Medium++ + b.Total++ + case "low": + b.Low++ + b.Total++ + } + } + return b +} + // severityGate represents the threshold for `--fail-on`. Findings at // or above this severity cause the analyze command to exit with // `exitSeverityGateBlock`. Empty string means "no gate" (the default). diff --git a/cmd/terrain/cmd_severity_gate_test.go b/cmd/terrain/cmd_severity_gate_test.go index 901cce25..3cdfcfe1 100644 --- a/cmd/terrain/cmd_severity_gate_test.go +++ b/cmd/terrain/cmd_severity_gate_test.go @@ -162,6 +162,56 @@ func TestRunAnalyze_JSONStdoutPurity(t *testing.T) { } } +// TestPRSeverityBreakdown verifies the helper that converts a PR's +// findings + AI blocking signals into a SignalBreakdown for the gate. +// Track 3.1 — the gate decision must apply uniformly across analyze +// + pr, sharing one helper. +func TestPRSeverityBreakdown(t *testing.T) { + t.Parallel() + cases := []struct { + name string + severities []string + want analyze.SignalBreakdown + }{ + { + name: "empty", + severities: nil, + want: analyze.SignalBreakdown{}, + }, + { + name: "mixed bag", + severities: []string{"critical", "high", "high", "medium", "low"}, + want: analyze.SignalBreakdown{ + Total: 5, Critical: 1, High: 2, Medium: 1, Low: 1, + }, + }, + { + name: "case insensitive + whitespace", + severities: []string{" HIGH ", "Critical"}, + want: analyze.SignalBreakdown{Total: 2, Critical: 1, High: 1}, + }, + { + name: "unknown severities dropped silently", + severities: []string{"high", "weird-tier", "info", ""}, + want: analyze.SignalBreakdown{Total: 1, High: 1}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got := prSeverityBreakdown(tc.severities) + if got.Total != tc.want.Total || + got.Critical != tc.want.Critical || + got.High != tc.want.High || + got.Medium != tc.want.Medium || + got.Low != tc.want.Low { + t.Errorf("prSeverityBreakdown(%v) = %+v, want %+v", + tc.severities, got, tc.want) + } + }) + } +} + // TestRunAnalyze_GatePassesWhenSeverityAbsent verifies the inverse: // `--fail-on critical` against a fixture whose worst severity is // medium returns nil (no gate block). diff --git a/cmd/terrain/main.go b/cmd/terrain/main.go index 14f574e0..50d484dc 100644 --- a/cmd/terrain/main.go +++ b/cmd/terrain/main.go @@ -180,9 +180,10 @@ func main() { jsonFlag := impactCmd.Bool("json", false, "output JSON impact result") showFlag := impactCmd.String("show", "", "drill-down view: units, gaps, tests, owners, graph, selected") ownerFlag := impactCmd.String("owner", "", "filter results by owner") + explainFlag := impactCmd.Bool("explain-selection", false, "render the selection explanation: which tests matter for this PR — and why") _ = impactCmd.Parse(os.Args[2:]) mountPositionalAsRoot("impact", impactCmd.Args(), rootFlag) - if err := runImpact(*rootFlag, *baseRef, *jsonFlag, *showFlag, *ownerFlag); err != nil { + if err := runImpact(*rootFlag, *baseRef, *jsonFlag, *showFlag, *ownerFlag, *explainFlag); err != nil { fmt.Fprintf(os.Stderr, "error: %v\n", err) os.Exit(1) } @@ -463,9 +464,19 @@ func main() { baseRef := prCmd.String("base", "", "git base ref for diff (default: HEAD~1)") jsonFlag := prCmd.Bool("json", false, "output JSON PR analysis") formatFlag := prCmd.String("format", "", "output format: markdown, comment, annotation") + failOnFlag := prCmd.String("fail-on", "", "exit "+fmt.Sprintf("%d", exitSeverityGateBlock)+" when at least one finding (NewFindings + AI BlockingSignals) is at or above this severity (critical|high|medium)") _ = prCmd.Parse(os.Args[2:]) mountPositionalAsRoot("pr", prCmd.Args(), rootFlag) - if err := runPR(*rootFlag, *baseRef, *jsonFlag, *formatFlag); err != nil { + gate, gateErr := parseSeverityGate(*failOnFlag) + if gateErr != nil { + fmt.Fprintf(os.Stderr, "error: %v\n", gateErr) + os.Exit(exitUsageError) + } + if err := runPR(*rootFlag, *baseRef, *jsonFlag, *formatFlag, gate); err != nil { + if errors.Is(err, errSeverityGateBlocked) { + fmt.Fprintf(os.Stderr, "%v\n", err) + os.Exit(exitSeverityGateBlock) + } fmt.Fprintf(os.Stderr, "error: %v\n", err) os.Exit(1) }