feat(0.2.1): --fail-on and --timeout flags on terrain analyze#134
Open
feat(0.2.1): --fail-on and --timeout flags on terrain analyze#134
Conversation
CI integration was the launch-readiness review's top adoption gap
for the platform/SRE audience. Two flags close most of it:
--fail-on critical|high|medium
Exit with the new severity-gate code (6) when at least one
finding at or above the requested severity is present.
Standard CI integration pattern. The report still renders
first; the gate decision is the last thing that happens.
--timeout <duration>
Abort the analysis after the duration elapses (e.g. 5m, 30s).
Wraps the existing SIGINT-aware context with a deadline so a
runaway monorepo scan doesn't block CI indefinitely.
Exit-code conventions extended:
6 — Severity gate block. Returned by analyze --fail-on. Same
pattern as the existing AI-gate code 4. CI scripts can branch
on "the analysis succeeded but the gate blocked us" without
parsing stderr text. Code 3 stays reserved for the planned
policy-vs-usage split.
Implementation:
* cmd/terrain/cmd_severity_gate.go — parseSeverityGate +
severityGateBlocked + errSeverityGateBlocked sentinel. Pure
function over analyze.SignalBreakdown; no engine coupling.
* cmd/terrain/cmd_pipeline_helpers.go — new
runPipelineWithSignalsAndTimeout(...). The original
runPipelineWithSignals is preserved as a 0-timeout shim so the
other six callsites (impact / explain / pr / ai *) keep their
contract; they can adopt the timeout flag in follow-up PRs
without churning this one.
* cmd/terrain/main.go — flag wiring; errors.Is(err,
errSeverityGateBlocked) routes the gate exit code without
confusing it for an analysis crash.
* cmd/terrain/cmd_analyze.go — gate evaluated last so the report
is always rendered before the exit decision.
Tests:
* TestParseSeverityGate covers canonical / case-insensitive /
whitespace / invalid inputs
* TestSeverityGateBlocked covers the threshold cascade
(critical-only, critical+high, critical+high+medium) and
confirms gateNone never blocks
Manual smoke (against the Terrain repo itself):
$ terrain analyze --fail-on critical → exit 0 (no critical)
$ terrain analyze --fail-on medium → exit 6 (matched
"0 critical + 493 high + 169 medium finding(s)")
$ terrain analyze --fail-on bogus → exit 2, usage error
The remaining gate flag from the plan
(--new-findings-only --baseline) is a 0.2.x follow-up; it needs
the suppressions/finding-IDs work to ship clean against renamed
files.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Terrain AI Risk Review
Decision: PASS — AI surfaces are covered. |
[INFO] Terrain — Informational only
Coverage gaps in changed code
4 pre-existing issues on changed files
Recommended tests1 test(s) with exact coverage of 0 impacted unit(s). 4 impacted unit(s) have no covering tests in the selected set.
Limitations
Generated by Terrain · Targeted Test ResultsTerrain selected 1 test(s) instead of the full suite.
|
The launch-readiness review flagged six concrete gaps in PR #134 that block defensible merge. This commit closes all six and fixes one real bug uncovered while writing the e2e tests. Bug fix: the JSON output branch (and SARIF / annotation / HTML branches) early-returned from runAnalyze before reaching the --fail-on gate check. So `terrain analyze --json --fail-on=medium` silently exited 0 even with matching findings — the gate worked in text mode only. The TestRunAnalyze_JSONStdoutPurity test caught this: it expected a gate-blocked error but got nil. The fix factors the gate decision into a closure (gateErr) computed before any rendering branch, then each branch returns gateErr() after its renderer completes. Text, JSON, SARIF, annotation, and HTML output all gate uniformly now. Review gaps closed: Pluralization cmd_severity_gate.go: replaced "finding(s)" with proper plural via plural() helper, mirroring the 0.2 polish work in internal/reporting/plural.go. Negative-timeout validation cmd/terrain/main.go: --timeout < 0 now exits with usage error (code 2) and a clear message, rather than the silent immediate-DeadlineExceeded that read like an analysis crash. Stale "0.1.2 contract" comments cmd/terrain/main.go: exit-code conventions block rewritten as pre-0.1.2 / additive 4+ semantics; "0.2 will move policy violations" deferred phrasing replaced with concrete 0.2.x → 0.3 milestone reference. E2E test for exit code 6 + report-renders-before-exit invariant TestRunAnalyze_GateBlocksOnFixture: runs runAnalyze against the calibration corpus with --fail-on=medium, asserts: 1. Returns errSeverityGateBlocked (so main.go maps to exit 6) 2. Error message contains the --fail-on label + counts 3. stdout is non-empty (report rendered before gate fired) 4. stdout contains the report header JSON stdout purity test TestRunAnalyze_JSONStdoutPurity: with --json + --fail-on matching, the entire stdout body parses as JSON. Verifies the gate message goes to the error channel (stderr via main.go), not into the JSON document. This is the test that uncovered the early-return bug above. Gate-passes inverse test TestRunAnalyze_GatePassesWhenSeverityAbsent: --fail-on=critical against a fixture whose worst severity is below critical returns nil. Locks in the false-positive-prevention property. All three new e2e tests run via captureRun (existing helper at cli_smoke_test.go:154) — function-level invocation, not exec subprocess, so they're fast and don't shell out. Verification: go build ./... clean go test ./... green go test ./internal/testdata/ green make docs-verify green Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the platform/SRE adoption gap flagged in the 0.2.0 launch-readiness review (Item 40, Item 39). Two CI-grade flags on `terrain analyze`:
Approach
Manual smoke (against the Terrain repo itself)
```
$ terrain analyze --fail-on critical → exit 0 (no critical findings)
$ terrain analyze --fail-on medium → exit 6 ("0 critical + 493 high + 169 medium finding(s)")
$ terrain analyze --fail-on bogus → exit 2 (usage error)
$ terrain analyze --timeout 100ms → analysis aborts cleanly with context.DeadlineExceeded
```
Test plan
Out of scope
`--new-findings-only --baseline ` from the plan — needs the stable finding IDs work (Phase 3.1) to ship clean across renamed files. Tracked for a 0.2.x follow-up.
Plan link
`/Users/pzachary/.claude/plans/kind-mapping-turing.md` (Phase 3.6).
🤖 Generated with Claude Code