From 20da9f87aef5df8a93112b94c0cb3df55719b48b Mon Sep 17 00:00:00 2001 From: Aaron Brethorst Date: Tue, 26 May 2026 14:44:03 -0700 Subject: [PATCH] exit code is always 0 on a completed run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A FAIL verdict now produces process exit code 0; the verdict travels via `summary.verdict` in the JSON report and the result-sink row's `result_data`. The exit code is reserved for "the validator could not run" (exit 2 on config error). This keeps a Render cron green when the validator successfully evaluated the OBA server but found real server bugs — the caller learns the verdict from the sink, not from a "failed" job status. - `Report.ExitCode()` always returns 0; godoc updated with rationale. - JSON schema tightens `summary.exitCode` from `enum: [0, 1]` to `const: 0` with a description explaining the field is retained for stability. - CLAUDE.md / README.md document the new policy; the severity-model entry no longer claims "Failures set exit code 1". - Adds `TestRunFailVerdictReturnsZero` pinning the policy end-to-end through `run()` (the unit-level test alone wouldn't catch a regression in main.go). Historical design specs under `docs/superpowers/specs/` still describe the old 0/1 contract; left as historical record per the project convention that CLAUDE.md is the living source of truth. --- CLAUDE.md | 4 +-- README.md | 12 +++++--- cmd/oba-validator/main_test.go | 39 +++++++++++++++++++++++-- report/document.go | 6 +++- report/document_test.go | 4 +-- schema/oba-validator-report.schema.json | 6 +++- validator/result.go | 10 ++++--- validator/result_test.go | 26 +++++++++++------ 8 files changed, 81 insertions(+), 26 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index fce7bfd..a6aefe4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -23,7 +23,7 @@ go run ./cmd/oba-validator [flags] OBA_VALIDATOR_LIVE=1 go test ./validator/ -run TestLiveKingCountyMetro -v ``` -Exit codes (also returned by `Report.ExitCode()`): `0` = no failures, `1` = ≥1 failure, `2` = config/usage error. Warnings and skips never affect the exit code. +Exit codes: `0` = the validator produced a report (PASS *or* FAIL verdict); `2` = the validator could not run (config/usage error, or `validator.Run` returned an error). The verdict is deliberately *not* in the exit code — it lives in the JSON report's `summary.verdict` and the result-sink row's `result_data`, so a Render cron that surfaces real server bugs still completes as "succeeded" and the caller learns the verdict from the sink. `Report.ExitCode()` (and `summary.exitCode`) is always `0`. ## Architecture @@ -49,7 +49,7 @@ Each check is a small struct in its own `check_*.go` file, returns `[]Result`, a This is the core design discipline. Severity is **evidence-based** — see `docs/superpowers/specs/2026-05-24-oba-validator-design.md`: -- **`Fail`** only when the feed *has* an entity but the API contradicts or is missing it (genuine server breakage). Failures set exit code 1. +- **`Fail`** only when the feed *has* an entity but the API contradicts or is missing it (genuine server breakage). A `Fail` drives the report's `summary.verdict` to `"FAIL"` but does **not** change the process exit code (see exit-code policy above). - **`Warn`** for valid-but-empty / unsamplable / unconfirmed conditions: empty feed, vehicle that moved, or an ID that didn't match on shape alone. - **`Skip`** when a prerequisite failed earlier in a dependent chain. diff --git a/README.md b/README.md index 5b68472..5d657c5 100644 --- a/README.md +++ b/README.md @@ -11,8 +11,10 @@ its REST API against the authoritative static GTFS and GTFS-realtime feeds Flags: `--json`, `--sample-size`, `--freshness`, `--timeout`, `--cache-dir`, `--no-cache`, `--refresh`. -Exit codes: `0` = no failures, `1` = at least one failure, `2` = config/usage -error. Warnings and skips do not affect the exit code. +Exit codes: `0` = the validator produced a report (read `summary.verdict` for +PASS/FAIL); `2` = the validator could not run (config/usage error). The process +exit code is intentionally not used to convey a FAIL verdict — that's what the +JSON report's `summary.verdict` and the result-sink row are for. ## Config @@ -119,8 +121,10 @@ From Ruby (e.g. an obacloud job), this mirrors the existing pattern: start_command = "/app/entrypoint.sh #{encoded}" Validator flags go after the token (`/app/entrypoint.sh --json`). The -job's exit status is the validator's exit code (`0` no failures, `1` ≥1 failure, -`2` config/usage error), so a failed validation shows as a failed run. +job's exit status is the validator's exit code — `0` when a report was produced +(PASS *or* FAIL verdict), `2` only when the validator couldn't run (config/usage +error). A FAIL verdict therefore shows as a *succeeded* Render run; the caller +reads the verdict from `summary.verdict` in the JSON report or the sink row. See `docs/superpowers/specs/2026-05-24-oba-validator-design.md` for the validator design and `docs/superpowers/specs/2026-05-25-render-deployment-design.md` for the diff --git a/cmd/oba-validator/main_test.go b/cmd/oba-validator/main_test.go index e0ad003..71e31c6 100644 --- a/cmd/oba-validator/main_test.go +++ b/cmd/oba-validator/main_test.go @@ -130,6 +130,38 @@ func TestRunJSONOutputShape(t *testing.T) { } } +// A FAIL verdict must come back through run() as process exit code 0; the +// PASS/FAIL signal lives in summary.verdict, not the exit code. This pins the +// CLAUDE.md exit-code policy end-to-end (newStubOBA returns an empty agencies +// list, which makes the endpoints check emit at least one Fail). +func TestRunFailVerdictReturnsZero(t *testing.T) { + t.Setenv("ONEBUSAWAY_API_KEY", "") + obaSrv := newStubOBA(t) + feedSrv := newStubFeed(t) + cfg := `{"obaServerURL":"` + obaSrv.URL + `","apiKey":"k","dataSources":[{"staticGtfsFeedURL":"` + feedSrv.URL + `/gtfs.zip"}]}` + var stdout, stderr bytes.Buffer + code := run([]string{"oba-validator", "--json", "--no-cache", cfg}, &stdout, &stderr) + + if code != 0 { + t.Fatalf("FAIL verdict must produce exit code 0, got %d\nstderr: %s", code, stderr.String()) + } + var doc struct { + Summary struct { + Verdict string `json:"verdict"` + ExitCode int `json:"exitCode"` + } `json:"summary"` + } + if err := json.Unmarshal(stdout.Bytes(), &doc); err != nil { + t.Fatalf("stdout not JSON: %v\n%s", err, stdout.String()) + } + if doc.Summary.Verdict != "FAIL" { + t.Errorf("summary.verdict = %q, want %q (stub returns empty agencies list)", doc.Summary.Verdict, "FAIL") + } + if doc.Summary.ExitCode != 0 { + t.Errorf("summary.exitCode = %d, want 0", doc.Summary.ExitCode) + } +} + func TestRunJSONConfigErrorRedactsInlineAPIKey(t *testing.T) { t.Setenv("ONEBUSAWAY_API_KEY", "") var stdout, stderr bytes.Buffer @@ -235,9 +267,10 @@ func TestRunSinkErrorDoesNotAlterExitCode(t *testing.T) { var stdout, stderr bytes.Buffer code := run([]string{"oba-validator", "--json", "--no-cache", cfg}, &stdout, &stderr) - // Sink failure must not change the validator exit code (0 or 1, never 2). - if code == 2 { - t.Errorf("sink failure should not produce exit code 2, got %d", code) + // Sink failure must not change the validator exit code (always 0 on a + // completed run; 2 is reserved for config/usage errors). + if code != 0 { + t.Errorf("sink failure should not change exit code from 0, got %d", code) } if !strings.Contains(stderr.String(), "result sink write failed") { t.Errorf("stderr should log the sink failure: %s", stderr.String()) diff --git a/report/document.go b/report/document.go index 86dd265..e46c228 100644 --- a/report/document.go +++ b/report/document.go @@ -42,7 +42,11 @@ type MetaSource struct { // Summary is the run-wide verdict and tallies. type Summary struct { - Verdict string `json:"verdict"` // PASS | FAIL + Verdict string `json:"verdict"` // PASS | FAIL + // ExitCode mirrors validator.Report.ExitCode() and is always 0 for any + // completed run. The PASS/FAIL verdict lives in Verdict above; the process + // exit code is reserved for "the validator could not run" (exit 2 on + // config error). Retained for schema stability — see CLAUDE.md. ExitCode int `json:"exitCode"` Total int `json:"total"` Counts Counts `json:"counts"` diff --git a/report/document_test.go b/report/document_test.go index fc87cd0..55c4d5e 100644 --- a/report/document_test.go +++ b/report/document_test.go @@ -105,8 +105,8 @@ func TestBuildDocument_CountsVerdictExit(t *testing.T) { if doc.Summary.Total != 4 { t.Errorf("total=%d want 4", doc.Summary.Total) } - if doc.Summary.Verdict != "FAIL" || doc.Summary.ExitCode != 1 { - t.Errorf("verdict/exit = %q/%d want FAIL/1", doc.Summary.Verdict, doc.Summary.ExitCode) + if doc.Summary.Verdict != "FAIL" || doc.Summary.ExitCode != 0 { + t.Errorf("verdict/exit = %q/%d want FAIL/0", doc.Summary.Verdict, doc.Summary.ExitCode) } if doc.SchemaVersion != SchemaVersion { t.Errorf("schemaVersion=%q", doc.SchemaVersion) diff --git a/schema/oba-validator-report.schema.json b/schema/oba-validator-report.schema.json index 0a45fee..669114e 100644 --- a/schema/oba-validator-report.schema.json +++ b/schema/oba-validator-report.schema.json @@ -81,7 +81,11 @@ "required": ["verdict", "exitCode", "total", "counts"], "properties": { "verdict": { "type": "string", "enum": ["PASS", "FAIL"] }, - "exitCode": { "type": "integer", "enum": [0, 1] }, + "exitCode": { + "type": "integer", + "const": 0, + "description": "Always 0 for a completed run. The PASS/FAIL verdict is conveyed by 'verdict' above, not by this field; the process exit code is reserved for 'the validator could not run' (exit 2 on config error). Field is retained for schema stability." + }, "total": { "type": "integer", "minimum": 0 }, "counts": { "$ref": "#/$defs/counts" } } diff --git a/validator/result.go b/validator/result.go index 4cbd89e..a49ff82 100644 --- a/validator/result.go +++ b/validator/result.go @@ -31,10 +31,12 @@ func (r Report) Worst() Status { return worst } -// ExitCode is 1 if any result failed, else 0. +// ExitCode is always 0 once a report has been produced. A FAIL verdict is +// reported via Worst() and the JSON document's summary.verdict; the process +// exit code is reserved for "the validator could not run" (config error in +// main.go returns 2). This keeps the Render cron status green when the +// validator successfully evaluated the OBA server, even if checks failed — +// callers read the verdict from the result-sink row. func (r Report) ExitCode() int { - if r.Worst() == Fail { - return 1 - } return 0 } diff --git a/validator/result_test.go b/validator/result_test.go index fb53b05..c465dd9 100644 --- a/validator/result_test.go +++ b/validator/result_test.go @@ -2,17 +2,16 @@ package validator import "testing" -func TestReportWorstAndExitCode(t *testing.T) { +func TestReportWorst(t *testing.T) { cases := []struct { name string statuses []Status worst Status - exit int }{ - {"all pass", []Status{Pass, Pass}, Pass, 0}, - {"warn only", []Status{Pass, Warn, Skip}, Warn, 0}, - {"any fail", []Status{Pass, Warn, Fail}, Fail, 1}, - {"empty", nil, Pass, 0}, + {"all pass", []Status{Pass, Pass}, Pass}, + {"warn only", []Status{Pass, Warn, Skip}, Warn}, + {"any fail", []Status{Pass, Warn, Fail}, Fail}, + {"empty", nil, Pass}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { @@ -23,9 +22,18 @@ func TestReportWorstAndExitCode(t *testing.T) { if got := r.Worst(); got != c.worst { t.Errorf("Worst()=%v want %v", got, c.worst) } - if got := r.ExitCode(); got != c.exit { - t.Errorf("ExitCode()=%d want %d", got, c.exit) - } }) } } + +// ExitCode is intentionally constant: a completed run always returns 0, +// including when checks failed. The FAIL verdict is conveyed via Worst() and +// the JSON summary.verdict, not the process exit code. +func TestReportExitCodeAlwaysZero(t *testing.T) { + for _, s := range []Status{Pass, Warn, Skip, Fail} { + r := Report{Results: []Result{{Status: s}}} + if got := r.ExitCode(); got != 0 { + t.Errorf("ExitCode() with %v result = %d, want 0", s, got) + } + } +}