diff --git a/BREAKING.md b/BREAKING.md index 073df81..345ab39 100644 --- a/BREAKING.md +++ b/BREAKING.md @@ -150,6 +150,51 @@ See `docs/SECURITY.md` for the full vulnerability disclosure and patching policy --- +## v1.10.0 — quality-gate verdicts and checkpoint evidence + +**Feature:** three-state `Verdict`, evidence-backed checkpoints, pre-checkpoint hooks activated +**Package:** `internal/cli/cmdship` + +### What changed + +| Area | Old | New | +|---|---|---| +| `HookResult.Passed` | `bool` | **removed** — replaced by `HookResult.Verdict` | +| Gate outcome | pass / fail | `VerdictUnknown` / `VerdictPass` / `VerdictFail` | +| `Checkpoint` | status only | gains `Evidence []Evidence` | +| `PhasePreCheckpoint` | declared, never invoked | runs before every checkpoint (advisory) | + +`cmdship` is an `internal/` package, so nothing outside this module can import it and no external consumer can break. It is recorded here anyway: the gate that demanded this entry is right to treat the module's own API as worth tracking, and a policy that is waived the first time it is inconvenient stops being a policy. + +### Why `Passed bool` had to go + +A bool forced every gate that *could not check* — artefact missing, tool absent, config it cannot parse — to answer either pass or fail. Authors picked pass, because failing a build over something that is not the user's fault is obviously wrong. So "I did not verify this" and "I verified this and it is fine" became the same value. + +`VerdictUnknown` is the **zero value** deliberately: a handler that forgets to set a verdict yields "unverified", not a false pass. + +### Migration (for anyone patching or forking this package) + +```go +// before +return HookResult{Passed: true} +return HookResult{Passed: false, Message: "..."} + +// after +return gatePass() +return gateFail("...") +return gateUnknown("...") // when the gate could not check — this case is new +``` + +The third constructor is the point of the change. A handler that previously returned `Passed: true` because its artefact was missing must now return `gateUnknown` with a reason. + +### Runtime behaviour changes (no action required) + +- A checkpoint reaching `ok` with **no independent evidence** is downgraded to `warning` and annotated `UNVERIFIED[…]`. It is never escalated to `fail`, and `res.Ready` keys on `fail` — so no working pipeline breaks. +- `self-review-gate` now actually executes. It is advisory: it annotates and downgrades to `warning`, never fails. `HookConfig.Strict` remains the opt-in for making hook findings blocking. +- `.forge/specs//.md` markers gain an `Evidence:` line. + +--- + ## v1.7.0 — LLM-first rearchitecture (piped-output migration) **Feature:** `internal/llmresponse` + `forge ship --human` + 10 MCP tools diff --git a/CHANGELOG.md b/CHANGELOG.md index 60c395c..c6028ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,49 @@ All notable changes to forge will be documented in this file. Format follows [Ke ## [Unreleased] +## [1.10.0] — 2026-08-07 — Quality gates that can fail, admit ignorance, and cite their evidence + +### Added + +- **M1 — a checkpoint may no longer report green on forge's own say-so.** `Checkpoint.Status` is a plain string that any of twenty-odd code paths could set to `"ok"`, and nothing ever required the code setting it to say *why*. Because forge is the actor in all of those paths, the thing being implicitly asserted was always true — "I wrote the file", "I ran the generator", "I completed the step". Those are facts about forge's behaviour, not about whether the change is sound. Every bug in this family has had the same shape: + + | forge verified | what mattered | + |---|---| + | `spec.md` written | `spec.md` is complete | + | test file written | the test will ever run | + | the gate returned | the gate examined anything | + | the checkpoint ran | the checkpoint verified anything | + + Checkpoints now carry `Evidence`, tagged by source. `SourceExternalTool` (a scanner, test runner, linter or git was asked and answered) and `SourceReadBack` (forge re-read the artefact from disk and re-validated it, judging it as it would judge a stranger's) count as independent. `SourceForgeClaim` — forge asserting its own success — is recorded, reported, and never sufficient on its own. + + Enforcement is at the reporting boundary, not via a private field or a mandatory setter: routing every assignment through `Pass(evidence)` would just invite `SourceForgeClaim` boilerplate that satisfies the compiler and nothing else. A checkpoint reaching `"ok"` with no independent evidence is downgraded to `"warning"` and annotated `UNVERIFIED[…]`. + + **This cannot break a working pipeline.** The claim a downgrade makes is "nobody checked" — a reason to withhold confidence, not to block a release. `res.Ready` keys on `"fail"`, which this policy never produces (`TestEvidencePolicy_NeverBlocksARun`). + +- **The gates turned out to be the evidence system already.** A hook returning `VerdictPass` has read an artefact off disk and re-validated it — that *is* read-back evidence, it was simply never recorded as the basis for the status. Wiring it up gave most checkpoints real evidence without touching a single `Status = "ok"` line. Only `VerdictUnknown` contributes nothing, which is exactly what M3 was for. + +- **Checkpoint marker files now record the basis, not just the outcome.** `.forge/specs//.md` gains an `Evidence:` line. The marker is the durable record — what `forge ship status` reads and what someone opens months later to ask "was this actually checked?" — and recording a status without its basis left that question unanswerable. Evidence is also emitted in `forge ship --json` so CI can audit a green run instead of taking the word `ok` for it. + +- **`PhasePreCheckpoint` hooks now actually run.** `self-review-gate` was declared, listed in `defaultHooks()`, documented in the package header, covered by tests — and had never executed, because `runWithOptions` only ever called `runHooks` for the two later phases. Counting it among forge's quality gates was inaccurate from the day it was written. + + It is wired in as **advisory**: findings annotate the checkpoint and downgrade `ok` to `warning`, but do not fail it. Every project using forge has been shipping without this gate, so switching it on as a blocker would break builds over artefacts that were acceptable yesterday. `HookConfig.Strict` is the opt-in for making it stop a run, exactly as with every other hook. All three pre-checkpoint calls are now routed through one `beforeCheckpoint()` helper alongside the snapshot and the agent-mode checkpoint marker, so a future checkpoint cannot pick up two of the three and silently miss the third. + +- **`Verdict` — quality gates now have three outcomes, not two.** `HookResult.Passed bool` is replaced by `Verdict` (`VerdictUnknown` / `VerdictPass` / `VerdictFail`). + + A bool forced every gate that *could not check* — artefact missing, tool not installed, config it cannot parse — to answer either "pass" or "fail". Gate authors almost always picked pass, because failing a build over something that is not the user's fault is obviously wrong. So "I did not verify this" and "I verified this and it is fine" became the same value, and the caller could not tell them apart. Every instance of that in forge has been the same bug: a green checkpoint standing on a check that never ran. + + `VerdictUnknown` is deliberately the **zero value** — a handler that forgets to set a verdict yields "unverified", which is honest, rather than falling into a false pass. Pinned by `TestVerdict_UnknownIsTheZeroValue`, because if `VerdictPass` ever became iota's first value, every incomplete handler in the codebase would silently start reporting success. + + Unverified gates annotate the checkpoint `UNVERIFIED[…]` and never escalate it. They are suppressed on an already-failed checkpoint, which has a real error to show. + +- **Every "could not check" path now says so.** Nine gates returned `Passed: true` when their artefact was missing (`// no spec file yet`, `// no ADR file → nothing to check`, …). All now return `VerdictUnknown` with a reason naming the missing file. Enforced going forward by `TestGateMutation_NoGateReportsCleanOnAnEmptyProject`: no gate may report clean on a project where none of its artefacts exist. + +### Fixed + +- **`spec-code-alignment-gate` reported PASS on projects it had never examined** — the gap the M2 mutation table found on its first run. `auditSlug()` returns early when `spec.md` is absent, skipping every check, and the gate fell through to pass. `forge ship --from=code` on a project whose spec was never written got a green alignment gate that verified nothing. It now returns `VerdictUnknown`: the gate did not find the project acceptable, it found it *unexaminable*, and those are different facts. + +- **`self-review-gate` reported PASS after scanning zero files.** Same shape, found by the same test. + ### Added - **Gate mutation testing (`gate_mutation_test.go`) — tests for the quality gates themselves.** Every other test in the suite asks "does the pipeline behave correctly?"; these ask whether the gates *check anything at all*. Each of the 13 hooks in `defaultHooks()` is now run against a **known-bad** fixture it must reject, and a known-good one it must accept. A gate that cannot fail is not a gate — and that is not hypothetical: 1.8.1's reachability checker compiled `\\.` from config source meaning `\.`, matched nothing in any real path, and reported the dead zone it was written to catch as fine. It was green, it was wrong, and nothing in the suite would have noticed, because every existing test asked only whether *good* input passed. diff --git a/internal/cli/cmdship/evidence.go b/internal/cli/cmdship/evidence.go new file mode 100644 index 0000000..8788e21 --- /dev/null +++ b/internal/cli/cmdship/evidence.go @@ -0,0 +1,173 @@ +// Copyright 2024 The Forge Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// evidence.go — M1: a checkpoint may not report "ok" on forge's own say-so. +// +// # The failure this closes +// +// M2 asked whether the gates check anything. M3 gave them a way to say "I +// could not check". This asks the question one level up: **what is a green +// checkpoint standing on?** +// +// `Checkpoint.Status` is a plain string that any of twenty-odd code paths can +// set to "ok". Nothing has ever required the code setting it to say why. And +// because forge is the actor in every one of those paths, the thing it is +// implicitly asserting is always true — "I wrote the file", "I ran the +// generator", "I completed the step". Those are facts about forge's own +// behaviour, not about whether the change is sound. Every bug in this family +// has had the same shape: +// +// spec.md written ≠ spec.md is complete +// test file written ≠ the test will ever run +// the gate returned ≠ the gate examined anything +// the checkpoint ran ≠ the checkpoint verified anything +// +// # What counts as evidence +// +// Evidence is an observation about the world that did not come from forge +// asserting its own success. Two sources qualify: +// +// SourceExternalTool — something outside forge was asked and answered: +// a test runner, a scanner, git, a linter. +// SourceReadBack — forge re-read what landed on disk and re-validated +// it, judging the artefact as it would judge a +// stranger's, rather than trusting the value it just +// held in memory. +// +// And one does not: +// +// SourceForgeClaim — "I did the thing." Recorded, reported, and never +// sufficient on its own to make a checkpoint green. +// +// # How it is enforced +// +// Not by making Status private or routing every assignment through a setter — +// that invites a SourceForgeClaim boilerplate that satisfies the compiler and +// nothing else. It is enforced where it matters, at the reporting boundary: a +// checkpoint that reaches "ok" with no qualifying evidence is downgraded to +// "warning" and annotated UNVERIFIED. +// +// The downgrade is deliberately not a failure. The claim being made is "nobody +// checked", which is a reason to withhold confidence, not a reason to block a +// release. `res.Ready` is unaffected — it turns on "fail", not "warning" — so +// this cannot break a pipeline that was working. +package cmdship + +import ( + "fmt" + "strings" +) + +// EvidenceSource says where an observation came from, which is the only thing +// that makes it worth anything. +type EvidenceSource string + +const ( + // SourceExternalTool — a tool outside forge was asked and answered. + SourceExternalTool EvidenceSource = "external-tool" + // SourceReadBack — forge re-read the artefact from disk and re-validated it. + SourceReadBack EvidenceSource = "read-back" + // SourceForgeClaim — forge asserting its own success. Never sufficient. + SourceForgeClaim EvidenceSource = "forge-claim" +) + +// Independent reports whether this source counts toward a green checkpoint. +// +// The name is the point: evidence is only worth having when it is independent +// of the party being assessed, and forge assessing forge is not. +func (s EvidenceSource) Independent() bool { + return s == SourceExternalTool || s == SourceReadBack +} + +// Evidence is one observation supporting a checkpoint's status. +type Evidence struct { + // Source is where the observation came from. + Source EvidenceSource `json:"source"` + // Claim is what it establishes, in the checkpoint's own terms + // (e.g. "spec.md has an Acceptance Criteria section with Given/When/Then"). + Claim string `json:"claim"` + // Observed is the raw finding, kept short — the count, the exit code, the + // gate name. It is what a reviewer would ask for when the claim is + // disputed. + Observed string `json:"observed,omitempty"` +} + +// String renders one evidence entry for a checkpoint detail line. +func (e Evidence) String() string { + if e.Observed == "" { + return string(e.Source) + ": " + e.Claim + } + return string(e.Source) + ": " + e.Claim + " (" + e.Observed + ")" +} + +// AddEvidence records an observation on the checkpoint. +func (cp *Checkpoint) AddEvidence(source EvidenceSource, claim, observed string) { + if cp == nil { + return + } + cp.Evidence = append(cp.Evidence, Evidence{Source: source, Claim: claim, Observed: observed}) +} + +// HasIndependentEvidence reports whether anything other than forge's own +// say-so supports this checkpoint. +func (cp *Checkpoint) HasIndependentEvidence() bool { + if cp == nil { + return false + } + for _, e := range cp.Evidence { + if e.Source.Independent() { + return true + } + } + return false +} + +// EvidenceSummary renders the independent evidence for a detail line, or "" if +// there is none. +func (cp *Checkpoint) EvidenceSummary() string { + if cp == nil { + return "" + } + var parts []string + for _, e := range cp.Evidence { + if e.Source.Independent() { + parts = append(parts, e.String()) + } + } + if len(parts) == 0 { + return "" + } + return strings.Join(parts, "; ") +} + +// applyEvidencePolicy is the enforcement point. +// +// A checkpoint that reached "ok" without a single independent observation is +// reporting confidence it has not earned, so it is downgraded to "warning" and +// told to say so. Statuses other than "ok" are left alone: a checkpoint that +// already failed has a real finding to report, and one already at "warning" is +// not claiming anything that needs qualifying. +func applyEvidencePolicy(cp *Checkpoint) { + if cp == nil || cp.Status != "ok" { + return + } + if cp.HasIndependentEvidence() { + return + } + cp.Status = "warning" + cp.Detail += fmt.Sprintf( + " | UNVERIFIED[%s reported ok with no independent evidence: nothing outside forge "+ + "confirmed this, and no artefact was re-read and re-validated]", + strings.ToLower(cp.Name)) +} diff --git a/internal/cli/cmdship/evidence_test.go b/internal/cli/cmdship/evidence_test.go new file mode 100644 index 0000000..d0539a1 --- /dev/null +++ b/internal/cli/cmdship/evidence_test.go @@ -0,0 +1,200 @@ +// Copyright 2024 The Forge Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Test-design checklist (always-write-tests.md 9-point): +// 1. Happy path — a checkpoint with real evidence stays green. +// 2. Boundary — evidence present but all of it forge's own claim. +// 3. Negative — green with no evidence at all is downgraded. +// 4. Idempotency — applying the policy twice changes nothing further. +// 5. Concurrency — every case owns its TempDir. +// 6. Cross-cutting — a real pipeline run records evidence, so the +// policy is neither vacuously satisfied nor +// vacuously triggered. +// 7. Regression — the policy must never turn a working run red. +// 8. Data-accuracy — evidence renders with its source and claim. +// 9. False-positive guard — fail/warning/skipped checkpoints are untouched. +package cmdship + +import ( + "strings" + "testing" +) + +// ── Negative: the whole point ───────────────────────────────────────────────── + +func TestEvidencePolicy_DowngradesGreenWithNoEvidence(t *testing.T) { + t.Parallel() + cp := Checkpoint{Name: "Spec", Status: "ok", Detail: "spec written"} + applyEvidencePolicy(&cp) + + if cp.Status != "warning" { + t.Fatalf("a green checkpoint resting on nothing but forge's own say-so must not "+ + "stay green, got %q", cp.Status) + } + if !strings.Contains(cp.Detail, "UNVERIFIED") { + t.Fatalf("the downgrade must be visible in the detail: %q", cp.Detail) + } + if !strings.Contains(cp.Detail, "no independent evidence") { + t.Fatalf("the detail must say why, not just that something is off: %q", cp.Detail) + } +} + +// ── Boundary: forge's own claim is not evidence ─────────────────────────────── + +func TestEvidencePolicy_ForgeClaimAloneIsNotEnough(t *testing.T) { + t.Parallel() + cp := Checkpoint{Name: "Spec", Status: "ok"} + // "I wrote the file" is a fact about forge's behaviour, not about whether + // the change is sound. If this ever counted, the type would be decoration. + cp.AddEvidence(SourceForgeClaim, "spec.md was written", "1 file") + applyEvidencePolicy(&cp) + + if cp.Status != "warning" { + t.Fatal("SourceForgeClaim must never be sufficient on its own — otherwise every " + + "checkpoint can self-certify and the policy means nothing") + } +} + +// ── Happy path ──────────────────────────────────────────────────────────────── + +func TestEvidencePolicy_IndependentEvidenceKeepsItGreen(t *testing.T) { + t.Parallel() + for _, src := range []EvidenceSource{SourceExternalTool, SourceReadBack} { + cp := Checkpoint{Name: "Ship", Status: "ok", Detail: "clean"} + cp.AddEvidence(src, "scanner reported no findings", "0 findings") + applyEvidencePolicy(&cp) + if cp.Status != "ok" { + t.Errorf("%s is independent evidence and must keep the checkpoint green, got %q", + src, cp.Status) + } + } +} + +// ── False-positive guard ────────────────────────────────────────────────────── + +func TestEvidencePolicy_LeavesNonGreenStatusesAlone(t *testing.T) { + t.Parallel() + for _, status := range []string{"fail", "warning", "skipped"} { + cp := Checkpoint{Name: "Spec", Status: status, Detail: "original"} + applyEvidencePolicy(&cp) + if cp.Status != status { + t.Errorf("status %q must not be rewritten; got %q", status, cp.Status) + } + if cp.Detail != "original" { + t.Errorf("status %q must not be annotated; got %q", status, cp.Detail) + } + } +} + +// ── Idempotency ─────────────────────────────────────────────────────────────── + +func TestEvidencePolicy_IsIdempotent(t *testing.T) { + t.Parallel() + cp := Checkpoint{Name: "Spec", Status: "ok", Detail: "d"} + applyEvidencePolicy(&cp) + first := cp.Detail + applyEvidencePolicy(&cp) + if cp.Detail != first { + t.Fatalf("re-applying must not stack annotations:\n first: %q\nsecond: %q", first, cp.Detail) + } +} + +// ── Source classification ───────────────────────────────────────────────────── + +func TestEvidenceSource_IndependenceIsExplicit(t *testing.T) { + t.Parallel() + if !SourceExternalTool.Independent() || !SourceReadBack.Independent() { + t.Error("asking a tool, and re-reading an artefact from disk, are both independent") + } + if SourceForgeClaim.Independent() { + t.Fatal("forge assessing forge is not independent evidence; if this ever returns " + + "true the entire policy silently stops doing anything") + } + // An unrecognised source must not sneak through as independent: a new + // source has to be argued for, not inherited by accident. + if EvidenceSource("something-new").Independent() { + t.Fatal("an unknown evidence source must default to NOT independent") + } +} + +// ── Cross-cutting: the policy is not vacuous in a real run ──────────────────── + +// TestEvidencePolicy_RealRunRecordsEvidence guards both ways this can go wrong +// at once. If the pipeline recorded no evidence anywhere, the policy would +// downgrade every run and become noise people learn to ignore. If it recorded +// evidence unconditionally, the policy would never fire and the type would be +// decoration. +func TestEvidencePolicy_RealRunRecordsEvidence(t *testing.T) { + t.Parallel() + res := RunWithOptions(RunOptions{ + Root: t.TempDir(), + Description: "evidence policy feature", + NoStrictTesting: true, + }) + + var withEvidence, greenWithout int + for _, cp := range res.Checkpoints { + if cp.HasIndependentEvidence() { + withEvidence++ + } + if cp.Status == "ok" && !cp.HasIndependentEvidence() { + greenWithout++ + } + } + if withEvidence == 0 { + t.Fatal("no checkpoint in a full run recorded independent evidence — the policy " + + "would downgrade every run, which is how a safety check gets switched off") + } + if greenWithout > 0 { + t.Fatalf("%d checkpoint(s) are green with no independent evidence; "+ + "applyEvidencePolicy is not reaching them", greenWithout) + } +} + +// ── Regression: never turn a working pipeline red ───────────────────────────── + +func TestEvidencePolicy_NeverBlocksARun(t *testing.T) { + t.Parallel() + res := RunWithOptions(RunOptions{ + Root: t.TempDir(), + Description: "evidence policy feature", + NoStrictTesting: true, + }) + // The claim a downgrade makes is "nobody checked" — a reason to withhold + // confidence, not to block a release. res.Ready keys on "fail", and this + // policy must never produce one. + for _, cp := range res.Checkpoints { + if cp.Status == "fail" && strings.Contains(cp.Detail, "no independent evidence") { + t.Fatalf("the evidence policy escalated a checkpoint to fail: %s", cp.Detail) + } + } +} + +// ── Data accuracy ───────────────────────────────────────────────────────────── + +func TestEvidence_String_CarriesSourceAndClaim(t *testing.T) { + t.Parallel() + e := Evidence{Source: SourceExternalTool, Claim: "scan clean", Observed: "0 findings"} + got := e.String() + for _, want := range []string{"external-tool", "scan clean", "0 findings"} { + if !strings.Contains(got, want) { + t.Errorf("rendered evidence missing %q: %s", want, got) + } + } + // Observed is optional and must not leave stray punctuation behind. + bare := Evidence{Source: SourceReadBack, Claim: "spec.md has ACs"}.String() + if strings.Contains(bare, "()") { + t.Errorf("empty Observed must be omitted cleanly: %s", bare) + } +} diff --git a/internal/cli/cmdship/gate_mutation_test.go b/internal/cli/cmdship/gate_mutation_test.go index f3523e3..0cf54d0 100644 --- a/internal/cli/cmdship/gate_mutation_test.go +++ b/internal/cli/cmdship/gate_mutation_test.go @@ -42,7 +42,7 @@ // // # Gates that cannot fail // -// Some hooks are advisory by design and can never return Passed=false. Those +// Some hooks are advisory by design and can never return VerdictFail. Those // are declared with alwaysPasses and a written reason, rather than being left // out of the table. Omission and "deliberately cannot fail" look identical in // a diff; one is a decision and the other is an oversight. @@ -68,6 +68,15 @@ type gateMutation struct { good map[string]string // bad is a set the gate must reject. The whole point of the file. bad map[string]string + // selfReports is true when the gate returns a verdict on an EMPTY project + // — i.e. it notices its own artefact is missing and says so, rather than + // reporting clean. Gates that read a file must set this; a gate that reads + // nothing (there are none today) would not. + // + // This is the M3 half of the mutation table. `bad` proves the gate can + // fail on wrong content; this proves it does not silently pass on no + // content at all — the more common and much quieter defect. + selfReports bool // alwaysPasses marks an advisory hook that cannot fail by design. reason // is required with it — an assertion-free entry has to justify itself. alwaysPasses bool @@ -92,6 +101,7 @@ var mutationTable = []gateMutation{ }, // Plausible-looking spec prose with no testable criteria — exactly what // an LLM produces when it drifts into summary mode. + selfReports: true, bad: map[string]string{ "spec.md": "# Spec\n\nThis feature adds rate limiting to the public API so the service stays responsive under load.\n", }, @@ -105,6 +115,7 @@ var mutationTable = []gateMutation{ }, // A decision recorded with no alternatives weighed and no cost stated: // an ADR in name only. + selfReports: true, bad: map[string]string{ "adr.md": "# ADR-001\n\nWe will use a token bucket.\n", }, @@ -118,6 +129,7 @@ var mutationTable = []gateMutation{ }, // A heading immediately followed by another heading — the shape a // truncated or skeleton-only generation leaves behind. + selfReports: true, bad: map[string]string{ "arch.md": "# Architecture\n## Components\n### Limiter\n", }, @@ -131,6 +143,7 @@ var mutationTable = []gateMutation{ }, // The single most dangerous artefact in the whole pipeline: a test file // that runs, passes, and asserts nothing. + selfReports: true, bad: map[string]string{ "tests.md": "# Tests\n\nScenario: over the limit\nGiven 100 requests\n\n```ts\nexpect(true).toBe(true)\n```\n", }, @@ -144,6 +157,7 @@ var mutationTable = []gateMutation{ }, // Checkboxes with nothing after them: a breakdown that counts as work // done while describing no work at all. + selfReports: true, bad: map[string]string{ "tasks.md": "# Tasks\n\n- [ ]\n- [ ]\n", }, @@ -155,6 +169,7 @@ var mutationTable = []gateMutation{ good: map[string]string{ "tasks.md": "# Tasks\n\n- [x] Add the limiter middleware\n- [x] Add the 429 response path\n", }, + selfReports: true, bad: map[string]string{ "tasks.md": "# Tasks\n\n- [x] Add the limiter middleware\n- [ ] Add the 429 response path\n", }, @@ -166,6 +181,7 @@ var mutationTable = []gateMutation{ good: map[string]string{ "impl-notes.md": "# Notes\n\nThe limiter reads its config from the environment via the settings loader.\n", }, + selfReports: true, bad: map[string]string{ "impl-notes.md": "# Notes\n\nFor local testing we set api_key = \"sk-live-not-a-real-key\" in the handler.\n", }, @@ -181,6 +197,7 @@ var mutationTable = []gateMutation{ }, // A QA report that reviews something other than the acceptance criteria // it is supposed to be evidence for. + selfReports: true, bad: map[string]string{ "spec.md": "# Spec\n\nGiven a signed-in user over the limit\n", "qa-report.md": "# QA\n\nRan the suite. Everything looked fine.\n", @@ -196,6 +213,7 @@ var mutationTable = []gateMutation{ }, // Half the review roles missing: a plan that looks complete at a glance // but leaves security and compliance unexamined. + selfReports: true, bad: map[string]string{ "manual-test-plan.md": "# Manual Test Plan\n\n## Product Owner\n- Check the happy path.\n\n## Business Analyst\n- Check the edge cases.\n", }, @@ -209,6 +227,7 @@ var mutationTable = []gateMutation{ }, // Evidence for the two cheap stages and silence on the two that // actually exercise a deployed system. + selfReports: true, bad: map[string]string{ "testing-pipeline.md": "# Testing Pipeline\n\n## Stage 1 — Local\nRan the suite locally.\n\n## Stage 2 — Pre-push\nCI is green.\n", }, @@ -225,6 +244,7 @@ var mutationTable = []gateMutation{ "spec.md": "# Spec\n\nGiven a user over the limit\n", "tasks.md": "# Tasks\n\n- [x] Add the limiter middleware\n", }, + selfReports: true, bad: map[string]string{ "spec.md": "# Spec\n\nGiven a user over the limit\n", "tasks.md": "# Tasks\n\n- [ ] Add the limiter middleware\n", @@ -242,6 +262,7 @@ var mutationTable = []gateMutation{ "spec.md": "# Spec\n\nGiven a user over the limit\n", "tasks.md": "# Tasks\n\n- [x] Add the limiter middleware\n", }, + selfReports: true, bad: map[string]string{ "spec.md": "# Spec\n\nGiven a user over the limit\n", "tasks.md": "# Tasks\n\n- [ ] Add the limiter middleware\n", @@ -254,6 +275,7 @@ var mutationTable = []gateMutation{ good: map[string]string{ "spec.md": "# Spec\n\n## Acceptance Criteria\n\nGiven a user\nWhen over the limit\nThen 429\n", }, + selfReports: true, bad: map[string]string{ "spec.md": "# Spec\n\n## Acceptance Criteria\n\nTODO: write the acceptance criteria\n", }, @@ -308,7 +330,8 @@ Read-only smoke check after promotion; no live mutations. // TestGateMutation_EveryGateRejectsKnownBadInput is the heart of this file. // -// A gate that returns Passed for its known-bad fixture is broken, whatever its +// A gate that does not return VerdictFail for its known-bad fixture is broken, +// whatever its // own unit tests say — those assert that good input passes, which a function // returning `true` unconditionally also does. func TestGateMutation_EveryGateRejectsKnownBadInput(t *testing.T) { @@ -320,7 +343,7 @@ func TestGateMutation_EveryGateRejectsKnownBadInput(t *testing.T) { t.Run(m.hook.Name+"/"+m.checkpoint+"/rejects-bad", func(t *testing.T) { t.Parallel() res := runGateAgainst(t, m, m.bad) - if res.Passed { + if res.Verdict != VerdictFail { t.Fatalf("%s PASSED its known-bad fixture — this gate does not check what it claims to.\n"+ "Bad fixture: %v", m.hook.Name, keysOf(m.bad)) } @@ -345,7 +368,7 @@ func TestGateMutation_EveryGateAcceptsKnownGoodInput(t *testing.T) { t.Run(m.hook.Name+"/"+m.checkpoint+"/accepts-good", func(t *testing.T) { t.Parallel() res := runGateAgainst(t, m, m.good) - if !res.Passed { + if res.Verdict != VerdictPass { t.Fatalf("%s REJECTED its known-good fixture: %s\n"+ "A gate that cries wolf gets switched off, and then it protects nothing.", m.hook.Name, res.Message) @@ -354,6 +377,78 @@ func TestGateMutation_EveryGateAcceptsKnownGoodInput(t *testing.T) { } } +// TestGateMutation_NoGateReportsCleanOnAnEmptyProject is the M3 half. +// +// The `bad` fixtures above prove a gate can fail on *wrong* content. This +// proves it does not report clean on *no* content — a quieter and far more +// common defect, because the code path that produces it reads "the artefact +// isn't there yet, nothing to complain about" and looks entirely reasonable. +// +// spec-code-alignment-gate did exactly that: on a project with unfinished +// tasks and no spec.md it returned pass, having skipped every check. Nothing +// distinguished that from a genuinely clean project until Verdict gained a +// third state. +func TestGateMutation_NoGateReportsCleanOnAnEmptyProject(t *testing.T) { + t.Parallel() + for _, m := range mutationTable { + if m.alwaysPasses || !m.selfReports { + continue + } + t.Run(m.hook.Name+"/"+m.checkpoint+"/empty-project", func(t *testing.T) { + t.Parallel() + res := runGateAgainst(t, m, nil) // no artefacts at all + if res.Verdict == VerdictPass { + t.Fatalf("%s reported PASS on a project with none of its artefacts present. "+ + "It verified nothing and said everything was fine — return VerdictUnknown "+ + "with a reason instead.", m.hook.Name) + } + if strings.TrimSpace(res.Message) == "" { + t.Errorf("%s gave a non-pass verdict with no message; the user cannot act on that", + m.hook.Name) + } + }) + } +} + +// TestVerdict_UnknownIsTheZeroValue pins the property the whole type rests on. +// +// A handler that forgets to set a verdict must yield "unverified", not a false +// pass. If VerdictPass ever becomes iota's first value, every incomplete +// handler in the codebase silently starts reporting success — which is the +// exact failure this type was introduced to make impossible. +func TestVerdict_UnknownIsTheZeroValue(t *testing.T) { + t.Parallel() + var zero Verdict + if zero != VerdictUnknown { + t.Fatal("VerdictUnknown must be the zero value: a forgotten verdict has to mean " + + "'unverified', never 'fine'") + } + if (HookResult{}).Verdict != VerdictUnknown { + t.Fatal("a zero HookResult must be unverified") + } + if VerdictUnknown.String() != "unverified" { + t.Errorf("Verdict.String() must not soften the unknown case: %q", VerdictUnknown.String()) + } +} + +// TestPartitionResults_KeepsFailuresAndUnverifiedApart guards the distinction +// at the point it is consumed. Merging the two buckets would restore the old +// behaviour without touching the type at all. +func TestPartitionResults_KeepsFailuresAndUnverifiedApart(t *testing.T) { + t.Parallel() + failures, unverified := partitionResults([]HookResult{ + {Verdict: VerdictFail, Message: "real problem", HookName: "a"}, + {Verdict: VerdictUnknown, Message: "could not check", HookName: "b"}, + {Verdict: VerdictPass, HookName: "c"}, + }) + if len(failures) != 1 || failures[0].HookName != "a" { + t.Fatalf("failures: %+v", failures) + } + if len(unverified) != 1 || unverified[0].HookName != "b" { + t.Fatalf("unverified: %+v", unverified) + } +} + // TestGateMutation_EveryDefaultHookIsCovered stops this file from decaying. // // Without it, a hook added next year gets no mutation coverage and the suite @@ -397,24 +492,22 @@ func TestGateMutation_AlwaysPassesEntriesAreJustified(t *testing.T) { // ── What the first run of this table found ──────────────────────────────────── -// TestSpecCodeAlignment_SilentlyPassesWithoutSpecMD documents a real gap that -// the mutation table exposed on its very first run, and pins the current -// behaviour so a future change to it is deliberate. +// TestSpecCodeAlignment_ReportsUnverifiedWithoutSpecMD is the mutation table's +// first find, now fixed. // -// auditSlug() returns early when spec.md is absent, so spec-code-alignment-gate -// reports PASS on a project with unfinished tasks — it never looked. In a -// normal pipeline spec.md exists by the time the code checkpoint runs, so this -// is not reachable by accident. It is reachable on purpose: `forge ship -// --from=code` on a project whose spec was never written, or one where spec.md -// was deleted, gets a green alignment gate that verified nothing. +// auditSlug() returns early when spec.md is absent, so every gap check in +// spec-code-alignment-gate is skipped. That used to fall through to PASS: on a +// project with unfinished tasks the gate reported clean, having never looked. +// Not reachable by accident in a normal pipeline, but very reachable on +// purpose — `forge ship --from=code` on a project whose spec was never +// written, or one where spec.md was deleted. // -// This is the M3 problem in miniature — "could not check" and "checked, fine" -// are the same value in a bool. It is left as-is here rather than flipped to a -// failure, because failing every spec-less run is a behaviour change that -// belongs in its own release, not smuggled in with a test file. The point of -// recording it is that it is now a known gap with a name, instead of an -// unexamined green. -func TestSpecCodeAlignment_SilentlyPassesWithoutSpecMD(t *testing.T) { +// It is now VerdictUnknown. This is exactly what the third verdict is for: the +// gate did not find the project acceptable, it found it unexaminable, and +// those are different facts. The checkpoint is annotated UNVERIFIED rather +// than silently green, and the run is not blocked over something that may well +// be intentional. +func TestSpecCodeAlignment_ReportsUnverifiedWithoutSpecMD(t *testing.T) { t.Parallel() root := t.TempDir() slug := slugify(mutationFeature) @@ -434,10 +527,109 @@ func TestSpecCodeAlignment_SilentlyPassesWithoutSpecMD(t *testing.T) { Description: mutationFeature, Result: &Checkpoint{Name: "code", Status: "ok"}, }) - if !res.Passed { - t.Fatal("behaviour changed: the gate now fails without spec.md. That is arguably " + - "the better behaviour — update this test and note it in the changelog as a " + - "deliberate change, rather than deleting the test.") + if res.Verdict == VerdictPass { + t.Fatal("the gate reported PASS without a spec to audit against — it verified nothing") + } + if res.Verdict != VerdictUnknown { + t.Fatalf("want VerdictUnknown (unexaminable, not broken), got %v: %s", res.Verdict, res.Message) + } + if !strings.Contains(res.Message, "spec.md") { + t.Errorf("the message must name what was missing: %q", res.Message) + } +} + +// TestPreCheckpointHooks_ActuallyRun closes the largest gap the mutation work +// turned up. +// +// self-review-gate was declared PhasePreCheckpoint, listed in defaultHooks(), +// documented in the package header, and covered by tests — and had never +// executed. runWithOptions called runHooks for PhasePostCheckpoint and +// PhasePostPipeline only; there was no PhasePreCheckpoint call site anywhere +// in the package. +// +// That is the failure mode one level above a gate that checks nothing: a gate +// that never runs at all. Everything *about* it was correct — handler, tests, +// docs — and none of it was reachable. Counting it among forge's quality gates +// was inaccurate from the day it was written. +// +// This test asserts the phase fires, using an artefact the gate rejects +// outright. A registered-but-unreachable hook is invisible to every other test +// in the suite, which is exactly how it survived. +func TestPreCheckpointHooks_ActuallyRun(t *testing.T) { + t.Parallel() + + var preCheckpoint []string + for _, h := range defaultHooks() { + if h.Phase == PhasePreCheckpoint { + preCheckpoint = append(preCheckpoint, h.Name) + } + } + if len(preCheckpoint) == 0 { + t.Skip("no pre-checkpoint hooks registered; nothing to assert") + } + + root := t.TempDir() + slug := slugify(mutationFeature) + specDir := filepath.Join(root, ".forge", "specs", slug) + if err := os.MkdirAll(specDir, 0o755); err != nil { + t.Fatal(err) + } + // A leftover placeholder from a previous run — the case the gate exists + // for, and one only a *pre*-checkpoint scan can catch, since the spec + // checkpoint is about to overwrite this file. + if err := os.WriteFile(filepath.Join(specDir, "spec.md"), + []byte("# Spec\n\n## Acceptance Criteria\n\nTODO: write these\n"), 0o600); err != nil { + t.Fatal(err) + } + + res := RunWithOptions(RunOptions{ + Root: root, + Description: mutationFeature, + Names: []string{"spec"}, + NoStrictTesting: true, + }) + + var detail string + for _, cp := range res.Checkpoints { + detail += cp.Detail + } + if !strings.Contains(detail, "self-review-gate") { + t.Fatalf("pre-checkpoint hooks did not fire — %v are registered but unreachable again.\n"+ + "Detail was: %s", preCheckpoint, detail) + } +} + +// TestPreCheckpointHooks_DoNotHardFailTheCheckpoint keeps the newly-activated +// phase from becoming a wall. +// +// This gate has never fired, so every project using forge has been shipping +// without it. Switching it on as a blocker would break builds over artefacts +// that were acceptable yesterday. It annotates and downgrades to warning; +// HookConfig.Strict is the opt-in for making it stop a run, same as every +// other hook. +func TestPreCheckpointHooks_DoNotHardFailTheCheckpoint(t *testing.T) { + t.Parallel() + root := t.TempDir() + slug := slugify(mutationFeature) + specDir := filepath.Join(root, ".forge", "specs", slug) + if err := os.MkdirAll(specDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(specDir, "spec.md"), + []byte("# Spec\n\n## Acceptance Criteria\n\nTODO: write these\n"), 0o600); err != nil { + t.Fatal(err) + } + + res := RunWithOptions(RunOptions{ + Root: root, + Description: mutationFeature, + Names: []string{"spec"}, + NoStrictTesting: true, + }) + for _, cp := range res.Checkpoints { + if cp.Status == "fail" { + t.Fatalf("a newly-activated advisory gate must not start failing builds: %s", cp.Detail) + } } } @@ -459,7 +651,7 @@ func runGateAgainst(t *testing.T, m gateMutation, files map[string]string) HookR } } // Result must be non-fail: every post-checkpoint gate short-circuits to - // Passed when the checkpoint already failed, so a "fail" here would make + // not-applicable when the checkpoint already failed, so a "fail" here would make // the whole table pass vacuously. result := &Checkpoint{Name: m.checkpoint, Status: "ok"} return m.hook.Handler(HookContext{ diff --git a/internal/cli/cmdship/hook.go b/internal/cli/cmdship/hook.go index 1a6c3ac..b50951c 100644 --- a/internal/cli/cmdship/hook.go +++ b/internal/cli/cmdship/hook.go @@ -20,11 +20,16 @@ // // Hook phases: // -// PhasePreCheckpoint — runs before the checkpoint LLM call; a Passed=false -// result adds a warning prefix to the checkpoint detail. -// PhasePostCheckpoint — runs after the checkpoint completes; a Passed=false +// PhasePreCheckpoint — declared, but NOT CURRENTLY INVOKED. runWithOptions +// only calls runHooks for the two phases below, so +// self-review-gate — the sole pre-checkpoint hook — has +// never executed in the pipeline. See +// TestPreCheckpointHooks_AreRegisteredButNeverRun. +// PhasePostCheckpoint — runs after the checkpoint completes; a VerdictFail // result flags the checkpoint as "warning" (non-blocking // by default; set HookConfig.Strict to elevate to "fail"). +// A VerdictUnknown result annotates the checkpoint +// UNVERIFIED and never escalates it. // PhasePostPipeline — runs once after all checkpoints pass; used for // learning extraction, review routing, and KB updates. // Failures here are always advisory-only (see the @@ -44,10 +49,10 @@ // security-hygiene-gate — post-checkpoint (code): secret + sandbox path scan // qa-coverage-gate — post-checkpoint (qa-verify): AC items referenced in tests // four-stage-testing-gate — post-checkpoint (qa-verify): testing-pipeline.md evidence -// present for all 4 stages. Advisory (never fails the -// checkpoint) unless HookConfig.StrictTesting is set — -// via .forge/hooks.yaml's "strict-testing: true" or the -// `forge ship --strict-testing` flag. See testing_pipeline.go. +// present for all 4 stages. Blocking by default since +// 1.8.2; waive with `forge ship --no-strict-testing` or +// "strict-testing: false" in .forge/hooks.yaml. +// See testing_pipeline.go. // four-stage-testing-reminder — post-pipeline: always prints the 4-stage testing // pipeline checklist to stderr after a successful run, // regardless of StrictTesting. Pure reminder, never blocks. @@ -95,11 +100,51 @@ type HookContext struct { StrictTesting bool } +// Verdict is the outcome of a quality gate. It has three states, not two, and +// that is the whole point of the type. +// +// A bool forces every gate that *could not check* — artefact missing, tool not +// installed, config it cannot parse — to answer either "pass" or "fail". Gate +// authors almost always pick pass, because failing a build over something that +// is not the user's fault is obviously wrong. The result is that "I did not +// verify this" and "I verified this and it is fine" become the same value, and +// the caller cannot tell them apart. Every instance of that in forge has been +// the same bug: a green checkpoint standing on a check that never ran. +// +// VerdictUnknown is deliberately the zero value. A handler that forgets to set +// a verdict yields "unverified", which is honest, rather than falling into +// either a false pass or a spurious build failure. +type Verdict int + +const ( + // VerdictUnknown means the gate could not determine an answer. It is never + // treated as passing. + VerdictUnknown Verdict = iota + // VerdictPass means the gate checked and found no issue. + VerdictPass + // VerdictFail means the gate checked and found an issue. + VerdictFail +) + +// String renders the verdict for logs and checkpoint details. +func (v Verdict) String() string { + switch v { + case VerdictPass: + return "pass" + case VerdictFail: + return "fail" + default: + return "unverified" + } +} + // HookResult is what a hook handler returns. type HookResult struct { - // Passed is false when the hook detected an issue. - Passed bool - // Message is the human-readable explanation (empty when Passed=true). + // Verdict is the gate's outcome. The zero value is VerdictUnknown. + Verdict Verdict + // Message is the human-readable explanation. Required for VerdictFail and + // VerdictUnknown — a result the user cannot act on is barely better than + // no gate at all. Empty for VerdictPass. Message string // HookName is set by runHooks (not the handler) to the originating // Hook.Name — lets a caller distinguish which specific hook failed @@ -110,6 +155,33 @@ type HookResult struct { HookName string } +// gatePass reports that the gate checked and found no issue. +func gatePass() HookResult { return HookResult{Verdict: VerdictPass} } + +// gateFail reports that the gate checked and found an issue. +func gateFail(format string, args ...any) HookResult { + return HookResult{Verdict: VerdictFail, Message: fmt.Sprintf(format, args...)} +} + +// gateUnknown reports that the gate could not check. +// +// Use this — never gatePass — when the artefact is missing, a tool is absent, +// or input cannot be parsed. Saying "pass" there is the bug this whole type +// exists to prevent: it launders an unexamined state into a verified one, and +// the checkpoint goes green on a check that never ran. +func gateUnknown(format string, args ...any) HookResult { + return HookResult{Verdict: VerdictUnknown, Message: fmt.Sprintf(format, args...)} +} + +// gateNotApplicable is the one case where "we did not check" is uninteresting: +// the checkpoint has already failed, so its gates are moot. It is Unknown like +// any other unchecked state, but the caller suppresses it on a failed +// checkpoint rather than piling "unverified" notes onto a run that already has +// a real error to report. +func gateNotApplicable() HookResult { + return HookResult{Verdict: VerdictUnknown, Message: "checkpoint already failed; gate not evaluated"} +} + // Hook is a single quality gate attached to the pipeline. type Hook struct { // Name uniquely identifies this hook (used in --skip-hook and logs). @@ -230,26 +302,34 @@ var selfReviewGate = Hook{ } filesToScan, ok := artifactsByCheckpoint[ctx.CheckpointName] if !ok { - return HookResult{Passed: true} + return gateUnknown("self-review-gate: no known artefact for checkpoint %q — nothing scanned", + ctx.CheckpointName) } badPatterns := []string{"TODO", "TBD", " 0 { - return HookResult{ - Passed: false, - Message: fmt.Sprintf("task-completion-gate: %d incomplete task(s) remain in tasks.md after code checkpoint", incomplete), - } + return gateFail("task-completion-gate: %d incomplete task(s) remain in tasks.md after code checkpoint", incomplete) } - return HookResult{Passed: true} + return gatePass() }, } @@ -333,30 +404,24 @@ var adrQualityGate = Hook{ Gate: "arch", Handler: func(ctx HookContext) HookResult { if ctx.Result == nil || ctx.Result.Status == "fail" { - return HookResult{Passed: true} + return gateNotApplicable() } slug := slugify(ctx.Description) adrPath := filepath.Join(ctx.Root, ".forge", "specs", slug, "adr.md") data, err := os.ReadFile(adrPath) if err != nil { - return HookResult{Passed: true} // no ADR file → nothing to check + return gateUnknown("adr-quality-gate: adr.md not found — architecture decision unverified") } content := strings.ToLower(string(data)) // Look for at least 2 alternative headings or list items. altCount := strings.Count(content, "alternative") + strings.Count(content, "option ") if altCount < 2 { - return HookResult{ - Passed: false, - Message: "adr-quality-gate: ADR must evaluate ≥2 alternatives; found fewer markers", - } + return gateFail("adr-quality-gate: ADR must evaluate ≥2 alternatives; found fewer markers") } if !strings.Contains(content, "consequence") && !strings.Contains(content, "trade-off") { - return HookResult{ - Passed: false, - Message: "adr-quality-gate: ADR missing consequences/trade-offs section", - } + return gateFail("adr-quality-gate: ADR missing consequences/trade-offs section") } - return HookResult{Passed: true} + return gatePass() }, } @@ -367,13 +432,13 @@ var archFileLint = Hook{ Gate: "arch", Handler: func(ctx HookContext) HookResult { if ctx.Result == nil || ctx.Result.Status == "fail" { - return HookResult{Passed: true} + return gateNotApplicable() } slug := slugify(ctx.Description) archPath := filepath.Join(ctx.Root, ".forge", "specs", slug, "arch.md") data, err := os.ReadFile(archPath) if err != nil { - return HookResult{Passed: true} + return gateUnknown("arch-file-lint: arch.md not found — architecture document unverified") } // Detect consecutive heading followed immediately by another heading // (empty section) or a TODO marker. @@ -383,20 +448,14 @@ var archFileLint = Hook{ if strings.HasPrefix(trimmed, "#") && i+1 < len(lines) { next := strings.TrimSpace(lines[i+1]) if strings.HasPrefix(next, "#") { - return HookResult{ - Passed: false, - Message: fmt.Sprintf("arch-file-lint: empty section detected after %q", trimmed), - } + return gateFail("arch-file-lint: empty section detected after %q", trimmed) } } if strings.Contains(trimmed, "TODO") || strings.Contains(trimmed, "TBD") { - return HookResult{ - Passed: false, - Message: fmt.Sprintf("arch-file-lint: placeholder detected in arch.md: %q", trimmed), - } + return gateFail("arch-file-lint: placeholder detected in arch.md: %q", trimmed) } } - return HookResult{Passed: true} + return gatePass() }, } @@ -409,13 +468,13 @@ var tddGate = Hook{ Gate: "test", Handler: func(ctx HookContext) HookResult { if ctx.Result == nil || ctx.Result.Status == "fail" { - return HookResult{Passed: true} + return gateNotApplicable() } slug := slugify(ctx.Description) testsPath := filepath.Join(ctx.Root, ".forge", "specs", slug, "tests.md") data, err := os.ReadFile(testsPath) if err != nil { - return HookResult{Passed: true} // no test artefact yet + return gateUnknown("tdd-gate: tests.md not found — test quality unverified") } content := string(data) // Detect always-passing anti-patterns. @@ -425,21 +484,15 @@ var tddGate = Hook{ } for _, pat := range alwaysPass { if strings.Contains(content, pat) { - return HookResult{ - Passed: false, - Message: fmt.Sprintf("tdd-gate: always-passing or skipped test pattern detected: %q", pat), - } + return gateFail("tdd-gate: always-passing or skipped test pattern detected: %q", pat) } } // Must reference at least one Given/When/Then or test scenario. if !strings.Contains(content, "Given ") && !strings.Contains(content, "Scenario:") && !strings.Contains(content, "func Test") { - return HookResult{ - Passed: false, - Message: "tdd-gate: tests.md must contain at least one test scenario (Given/When/Then or func Test*)", - } + return gateFail("tdd-gate: tests.md must contain at least one test scenario (Given/When/Then or func Test*)") } - return HookResult{Passed: true} + return gatePass() }, } @@ -452,13 +505,13 @@ var breakdownCompletenessGate = Hook{ Gate: "breakdown", Handler: func(ctx HookContext) HookResult { if ctx.Result == nil || ctx.Result.Status == "fail" { - return HookResult{Passed: true} + return gateNotApplicable() } slug := slugify(ctx.Description) tasksPath := filepath.Join(ctx.Root, ".forge", "specs", slug, "tasks.md") data, err := os.ReadFile(tasksPath) if err != nil { - return HookResult{Passed: true} + return gateUnknown("breakdown-completeness: tasks.md not found — breakdown unverified") } lines := strings.Split(string(data), "\n") empty := 0 @@ -470,12 +523,9 @@ var breakdownCompletenessGate = Hook{ } } if empty > 0 { - return HookResult{ - Passed: false, - Message: fmt.Sprintf("breakdown-completeness: %d task(s) have empty descriptions in tasks.md", empty), - } + return gateFail("breakdown-completeness: %d task(s) have empty descriptions in tasks.md", empty) } - return HookResult{Passed: true} + return gatePass() }, } @@ -487,13 +537,13 @@ var securityHygieneGate = Hook{ Gate: "code", Handler: func(ctx HookContext) HookResult { if ctx.Result == nil || ctx.Result.Status == "fail" { - return HookResult{Passed: true} + return gateNotApplicable() } slug := slugify(ctx.Description) implPath := filepath.Join(ctx.Root, ".forge", "specs", slug, "impl-notes.md") data, err := os.ReadFile(implPath) if err != nil { - return HookResult{Passed: true} + return gateUnknown("security-hygiene-gate: impl-notes.md not found — implementation notes unscanned") } content := string(data) // Secret-like patterns. @@ -503,23 +553,17 @@ var securityHygieneGate = Hook{ } for _, pat := range secretPatterns { if strings.Contains(strings.ToLower(content), strings.ToLower(pat)) { - return HookResult{ - Passed: false, - Message: fmt.Sprintf("security-hygiene-gate: potential secret pattern %q in impl-notes.md", pat), - } + return gateFail("security-hygiene-gate: potential secret pattern %q in impl-notes.md", pat) } } // Shell injection indicators. shellPatterns := []string{"shell=true", "os.system(", "exec.Command(\"sh\",", "exec.Command(\"bash\","} for _, pat := range shellPatterns { if strings.Contains(content, pat) { - return HookResult{ - Passed: false, - Message: fmt.Sprintf("security-hygiene-gate: shell-injection risk: %q found in impl-notes.md", pat), - } + return gateFail("security-hygiene-gate: shell-injection risk: %q found in impl-notes.md", pat) } } - return HookResult{Passed: true} + return gatePass() }, } @@ -531,7 +575,7 @@ var qaCoverageGate = Hook{ Gate: "qa-verify", Handler: func(ctx HookContext) HookResult { if ctx.Result == nil || ctx.Result.Status == "fail" { - return HookResult{Passed: true} + return gateNotApplicable() } slug := slugify(ctx.Description) specPath := filepath.Join(ctx.Root, ".forge", "specs", slug, "spec.md") @@ -540,7 +584,7 @@ var qaCoverageGate = Hook{ specData, specErr := os.ReadFile(specPath) qaData, qaErr := os.ReadFile(qaPath) if specErr != nil || qaErr != nil { - return HookResult{Passed: true} // artefacts not present yet + return gateUnknown("qa-coverage-gate: spec.md or qa-report.md not found — AC coverage unverified") } // Count AC items in spec (lines starting with "Given" or "- AC-"). @@ -564,12 +608,9 @@ var qaCoverageGate = Hook{ } } if acCount > 0 && covered < acCount { - return HookResult{ - Passed: false, - Message: fmt.Sprintf("qa-coverage-gate: %d/%d AC items referenced in qa-report.md", covered, acCount), - } + return gateFail("qa-coverage-gate: %d/%d AC items referenced in qa-report.md", covered, acCount) } - return HookResult{Passed: true} + return gatePass() }, } @@ -578,11 +619,20 @@ var qaCoverageGate = Hook{ // blocking gaps are found (incomplete tasks, untested authz roles, missing event tests). var specCodeAlignmentHandler = func(ctx HookContext) HookResult { if ctx.Result == nil || ctx.Result.Status == "fail" { - return HookResult{Passed: true} + return gateNotApplicable() } result := auditSpecVsCode(ctx.Root, ctx.Description, ctx.SpecName) + // Without spec.md there is nothing to align code against, and auditSlug() + // returns early — so every gap check below is skipped. This used to fall + // through to "pass", which meant `forge ship --from=code` on a project + // whose spec was never written got a green alignment gate that had + // verified nothing. Found by the gate mutation table (M2); this is what + // having a third verdict is for. + if !result.SpecFound { + return gateUnknown("spec-code-alignment-gate: spec.md not found — spec-vs-code alignment unverified") + } if !result.HasBlockingGaps() { - return HookResult{Passed: true} + return gatePass() } var msgs []string for _, g := range result.Gaps { @@ -590,10 +640,7 @@ var specCodeAlignmentHandler = func(ctx HookContext) HookResult { msgs = append(msgs, fmt.Sprintf("[%s] %s (hint: %s)", g.Type, g.Description, g.Hint)) } } - return HookResult{ - Passed: false, - Message: fmt.Sprintf("spec-code-alignment-gate: %d blocking gap(s): %s", len(msgs), strings.Join(msgs, "; ")), - } + return gateFail("spec-code-alignment-gate: %d blocking gap(s): %s", len(msgs), strings.Join(msgs, "; ")) } // specCodeAlignmentGateCode runs the spec-vs-code audit at the code checkpoint. @@ -625,16 +672,13 @@ var manualTestPlanGate = Hook{ Gate: "qa-verify", Handler: func(ctx HookContext) HookResult { if ctx.Result == nil || ctx.Result.Status == "fail" { - return HookResult{Passed: true} + return gateNotApplicable() } slug := slugify(ctx.Description) planPath := filepath.Join(ctx.Root, ".forge", "specs", slug, "manual-test-plan.md") data, err := os.ReadFile(planPath) if err != nil { - return HookResult{ - Passed: false, - Message: "manual-test-plan-gate: manual-test-plan.md not found — run qa-verify with an LLM configured", - } + return gateFail("manual-test-plan-gate: manual-test-plan.md not found — run qa-verify with an LLM configured") } content := strings.ToLower(string(data)) // Each of the 6 role sections must be identifiable by a heading keyword. @@ -656,12 +700,9 @@ var manualTestPlanGate = Hook{ } } if len(missing) > 0 { - return HookResult{ - Passed: false, - Message: fmt.Sprintf("manual-test-plan-gate: missing role sections in manual-test-plan.md: %s", strings.Join(missing, ", ")), - } + return gateFail("manual-test-plan-gate: missing role sections in manual-test-plan.md: %s", strings.Join(missing, ", ")) } - return HookResult{Passed: true} + return gatePass() }, } @@ -707,10 +748,44 @@ func runHooks(phase HookPhase, ctx HookContext, hooks []Hook, cfg HookConfig) [] continue } res := h.Handler(ctx) - if !res.Passed { - res.HookName = h.Name + res.HookName = h.Name + if res.Verdict != VerdictPass { failed = append(failed, res) + continue } + // A gate that returns Pass has read an artefact off disk and + // re-validated it — the definition of read-back evidence (M1). This is + // where most checkpoints earn their green: the gates were already + // doing the verifying, it was simply never recorded as the basis for + // the status. + // + // Only VerdictPass qualifies. Unknown means the gate could not check, + // and M3 exists precisely so that no longer counts for anything. + // + // ctx.Result is nil in the pre-checkpoint phase and AddEvidence is a + // no-op on nil, which is the behaviour we want: a pre-checkpoint scan + // examines the *previous* run's artefact, so it is not evidence about + // what this run is about to produce. + ctx.Result.AddEvidence(SourceReadBack, h.Name+" verified "+ctx.CheckpointName, "gate passed") } return failed } + +// partitionResults splits hook results into the two groups the caller treats +// differently: gates that found a real problem, and gates that could not check. +// +// Keeping them apart is the entire point of Verdict. Merging them would put +// "spec.md not found, nothing verified" and "spec.md is missing its acceptance +// criteria" into one bucket, which is how the two became indistinguishable in +// the first place. +func partitionResults(results []HookResult) (failures, unverified []HookResult) { + for _, r := range results { + switch r.Verdict { + case VerdictFail: + failures = append(failures, r) + case VerdictUnknown: + unverified = append(unverified, r) + } + } + return failures, unverified +} diff --git a/internal/cli/cmdship/hook_test.go b/internal/cli/cmdship/hook_test.go index 7939499..7d4d2a5 100644 --- a/internal/cli/cmdship/hook_test.go +++ b/internal/cli/cmdship/hook_test.go @@ -158,8 +158,8 @@ const completeEvidence = "## Local\nran jest + manual click-through\n\n## Pre-pu func TestFourStageTestingGate_ResultNilIsNoOp(t *testing.T) { res := fourStageTestingGate.Handler(HookContext{Result: nil, StrictTesting: true}) - if !res.Passed { - t.Fatal("nil Result must always pass — nothing to gate yet") + if res.Verdict == VerdictFail { + t.Fatal("nil Result must never fail — there is nothing to gate yet") } } @@ -168,7 +168,7 @@ func TestFourStageTestingGate_UpstreamFailureIsNoOp(t *testing.T) { Result: &Checkpoint{Status: "fail"}, StrictTesting: true, }) - if !res.Passed { + if res.Verdict == VerdictFail { t.Fatal("an already-failed checkpoint must not additionally fail on missing testing evidence") } } @@ -181,7 +181,7 @@ func TestFourStageTestingGate_MissingFile_AdvisoryByDefault(t *testing.T) { Result: &Checkpoint{Status: "ok"}, StrictTesting: false, // default }) - if !res.Passed { + if res.Verdict == VerdictFail { t.Fatalf("StrictTesting=false must never fail on missing testing-pipeline.md, got: %s", res.Message) } } @@ -194,8 +194,8 @@ func TestFourStageTestingGate_MissingFile_BlocksWhenStrict(t *testing.T) { Result: &Checkpoint{Status: "ok"}, StrictTesting: true, }) - if res.Passed { - t.Fatal("StrictTesting=true with no testing-pipeline.md at all must fail") + if res.Verdict != VerdictFail { + t.Fatalf("StrictTesting=true with no testing-pipeline.md at all must FAIL, got %v", res.Verdict) } if !strings.Contains(res.Message, "not found") { t.Fatalf("expected a 'not found' message, got: %s", res.Message) @@ -211,7 +211,7 @@ func TestFourStageTestingGate_IncompleteEvidence_AdvisoryByDefault(t *testing.T) Result: &Checkpoint{Status: "ok"}, StrictTesting: false, }) - if !res.Passed { + if res.Verdict == VerdictFail { t.Fatalf("StrictTesting=false must never fail on incomplete evidence, got: %s", res.Message) } } @@ -225,8 +225,8 @@ func TestFourStageTestingGate_IncompleteEvidence_BlocksWhenStrict(t *testing.T) Result: &Checkpoint{Status: "ok"}, StrictTesting: true, }) - if res.Passed { - t.Fatal("StrictTesting=true with incomplete evidence must fail") + if res.Verdict != VerdictFail { + t.Fatalf("StrictTesting=true with incomplete evidence must FAIL, got %v", res.Verdict) } for _, want := range []string{"Pre-push", "Staging", "Production"} { if !strings.Contains(res.Message, want) { @@ -244,8 +244,8 @@ func TestFourStageTestingGate_CompleteEvidence_PassesEvenWhenStrict(t *testing.T Result: &Checkpoint{Status: "ok"}, StrictTesting: true, }) - if !res.Passed { - t.Fatalf("complete evidence must pass even in strict mode, got: %s", res.Message) + if res.Verdict != VerdictPass { + t.Fatalf("complete evidence must PASS even in strict mode, got %v: %s", res.Verdict, res.Message) } } @@ -260,8 +260,8 @@ func TestFourStageTestingReminder_AlwaysPasses(t *testing.T) { Description: "some feature", StrictTesting: strict, }) - if !res.Passed { - t.Fatalf("post-pipeline reminder hook must always report Passed=true (strict=%v)", strict) + if res.Verdict != VerdictPass { + t.Fatalf("post-pipeline reminder hook must always pass (strict=%v), got %v", strict, res.Verdict) } } } diff --git a/internal/cli/cmdship/ship.go b/internal/cli/cmdship/ship.go index c4ff302..d224323 100644 --- a/internal/cli/cmdship/ship.go +++ b/internal/cli/cmdship/ship.go @@ -138,6 +138,11 @@ type Checkpoint struct { Debate *DebateResult `json:"debate,omitempty"` // populated when --yolo self-debate runs GapAudit *SpecAuditResult `json:"gap_audit,omitempty"` // TG-39: spec-vs-code audit result RemediationRounds int `json:"remediation_rounds,omitempty"` // rounds of LLM-driven gap remediation + // Evidence records what this checkpoint's status actually rests on. A + // status of "ok" requires at least one entry from an independent source — + // see evidence.go. Emitted in --json so a reviewer or CI job can audit the + // basis of a green run rather than taking the word "ok" for it. + Evidence []Evidence `json:"evidence,omitempty"` } // ShipResult summarizes the ship run. @@ -1453,6 +1458,19 @@ func checkVerify(root, description, specName string, pipe *LLMPipe) Checkpoint { } cp.Status = "ok" + // M1: the ship checkpoint has no post-checkpoint gates to earn its green + // from, so it records its own. Unlike most "ok" assignments in this file, + // these are real observations — the scanner and the hygiene checker were + // actually run and actually answered. + cp.AddEvidence(SourceExternalTool, "security scan found no high-confidence findings", + fmt.Sprintf("%d finding(s) total, %d high-confidence", len(scanRes.Findings), len(highFindings))) + cp.AddEvidence(SourceExternalTool, "hygiene check found no unmanaged files", + fmt.Sprintf("%d manifest pattern(s)", patternCount)) + if auditRes.SpecFound { + cp.AddEvidence(SourceReadBack, "spec-vs-code audit found no blocking gaps", + fmt.Sprintf("%d warning-level gap(s)", len(auditRes.Gaps))) + } + warnCount := len(scanRes.Findings) - len(highFindings) if warnCount > 0 { cp.Detail = fmt.Sprintf("security scan: no high-confidence findings (%d medium/low advisory — run `forge scan security` to review); hygiene OK; manifest OK (%d patterns)", warnCount, patternCount) @@ -1990,9 +2008,43 @@ func runWithOptions(opts RunOptions) *ShipResult { agentPaused := func() bool { return opts.AgentBridge.Paused() } serial := opts.AgentBridge != nil + // Pre-checkpoint hooks. Until now this phase was declared, documented, and + // never invoked — self-review-gate has been counted among forge's quality + // gates since it was written without ever executing once. + // + // Findings are stashed rather than acted on immediately: the phase runs + // before the checkpoint exists, so there is no Checkpoint to annotate yet. + // They are attached in the reporting loop below, where the result is. + preHookNotes := map[string][]HookResult{} + runPre := func(name string) { + if len(hooks) == 0 { + return + } + res := runHooks(PhasePreCheckpoint, HookContext{ + Phase: PhasePreCheckpoint, + CheckpointName: name, + Root: root, + Description: opts.Description, + SpecName: opts.SpecName, + Pipe: pipe, + Result: nil, // by definition — the checkpoint has not run + StrictTesting: hookCfg.StrictTesting, + }, hooks, hookCfg) + if len(res) > 0 { + preHookNotes[name] = res + } + } + // beforeCheckpoint bundles the three things that must happen before every + // checkpoint, so a new checkpoint cannot pick up two of them and silently + // miss the third. + beforeCheckpoint := func(name string) { + snapBefore(name) + pipe.SetCheckpoint(name) + runPre(name) + } + if needs("spec") { - snapBefore("spec") - pipe.SetCheckpoint("spec") + beforeCheckpoint("spec") results["spec"] = checkSpec(root, opts.Description, opts.SpecName, pipe) } @@ -2006,13 +2058,11 @@ func runWithOptions(opts RunOptions) *ShipResult { switch { case serial: if runArch { - snapBefore("arch") - pipe.SetCheckpoint("arch") + beforeCheckpoint("arch") archCP = checkArch(root, opts.Description, opts.SpecName, pipe) } if runTest && !agentPaused() { - snapBefore("test") - pipe.SetCheckpoint("test") + beforeCheckpoint("test") testCP = checkTest(root, opts.Description, opts.SpecName, pipe, opts.DryRun) } else { runTest = false @@ -2020,7 +2070,7 @@ func runWithOptions(opts RunOptions) *ShipResult { default: if runArch { dagWG.Add(1) - snapBefore("arch") + beforeCheckpoint("arch") go func() { defer dagWG.Done() archCP = checkArch(root, opts.Description, opts.SpecName, pipe) @@ -2028,7 +2078,7 @@ func runWithOptions(opts RunOptions) *ShipResult { } if runTest { dagWG.Add(1) - snapBefore("test") + beforeCheckpoint("test") go func() { defer dagWG.Done() testCP = checkTest(root, opts.Description, opts.SpecName, pipe, opts.DryRun) @@ -2044,22 +2094,19 @@ func runWithOptions(opts RunOptions) *ShipResult { } if needs("breakdown") && !agentPaused() { - snapBefore("breakdown") - pipe.SetCheckpoint("breakdown") + beforeCheckpoint("breakdown") results["breakdown"] = checkBreakdown(root, opts.Description, opts.SpecName, pipe) } if needs("code") && !agentPaused() { - snapBefore("code") - pipe.SetCheckpoint("code") + beforeCheckpoint("code") results["code"] = checkCode(root, opts.Description, opts.SpecName, pipe) } if needs("ship") && !agentPaused() { - snapBefore("ship") - pipe.SetCheckpoint("ship") + beforeCheckpoint("ship") results["ship"] = checkVerify(root, opts.Description, opts.SpecName, pipe) } if needs("qa-verify") && !agentPaused() { - pipe.SetCheckpoint("qa-verify") + beforeCheckpoint("qa-verify") results["qa-verify"] = checkQAVerify(root, opts.Description, opts.SpecName, pipe) } // PR checkpoint: appended only for full-pipeline runs with --pr. @@ -2125,7 +2172,32 @@ func runWithOptions(opts RunOptions) *ShipResult { Result: &cp, StrictTesting: hookCfg.StrictTesting, } - if failures := runHooks(PhasePostCheckpoint, hookCtx, hooks, hookCfg); len(failures) > 0 { + // Pre-checkpoint findings are merged with the post-checkpoint ones + // so both phases reach the same reporting and escalation rules. + // Keeping them on separate paths is how the pre-checkpoint phase + // stayed unwired and unnoticed for as long as it did. + allResults := runHooks(PhasePostCheckpoint, hookCtx, hooks, hookCfg) + allResults = append(preHookNotes[strings.ToLower(cp.Name)], allResults...) + failures, unverified := partitionResults(allResults) + + // Gates that could not check are reported separately and never + // escalate the checkpoint. They are not evidence of a problem — + // but they are also not evidence of correctness, and reporting a + // checkpoint as clean on the strength of checks that never ran is + // the failure this whole distinction exists to end. + // + // Suppressed on an already-failed checkpoint: that run has a real + // error to show, and burying it under "unverified" notes for gates + // that were moot anyway helps nobody. + if len(unverified) > 0 && cp.Status != "fail" { + var notes []string + for _, u := range unverified { + notes = append(notes, u.Message) + } + cp.Detail += " | UNVERIFIED[" + strings.Join(notes, "; ") + "]" + } + + if len(failures) > 0 { // four-stage-testing-gate only ever fails when StrictTesting // is on (see testing_pipeline.go), so its failure must // escalate the checkpoint even when the unrelated global @@ -2148,6 +2220,12 @@ func runWithOptions(opts RunOptions) *ShipResult { } } } + + // M1: a green checkpoint has to rest on something other than forge + // reporting its own success. Runs last, after every gate has had the + // chance to contribute evidence, so it only fires when genuinely + // nothing independent was observed. + applyEvidencePolicy(&cp) // P1-L2: write checkpoint digest on success for downstream context compression. // J6 (fix-checkpoint-llm-quality-and-observability): digest from the // real generated artefact, not cp.Detail (the one-line status @@ -2172,8 +2250,18 @@ func runWithOptions(opts RunOptions) *ShipResult { cpLowerName := strings.ToLower(cp.Name) markerPath := filepath.Join(root, ".forge", "specs", specSlug, cpLowerName+".md") if _, statErr := os.Stat(markerPath); os.IsNotExist(statErr) { - marker := fmt.Sprintf("# %s checkpoint\n\nStatus: %s\nCompleted: %s\n\n%s\n", - cp.Name, cp.Status, time.Now().UTC().Format(time.RFC3339), cp.Detail) + // M1: the marker is the durable record — what `forge ship + // status` reads and what a human opens months later to ask + // "was this actually checked?". Recording the status without + // its basis leaves that question unanswerable, which is the + // whole failure this work is about. + basis := cp.EvidenceSummary() + if basis == "" { + basis = "none — no independent verification was recorded for this checkpoint" + } + marker := fmt.Sprintf( + "# %s checkpoint\n\nStatus: %s\nCompleted: %s\nEvidence: %s\n\n%s\n", + cp.Name, cp.Status, time.Now().UTC().Format(time.RFC3339), basis, cp.Detail) _ = os.WriteFile(markerPath, []byte(marker), 0o600) } } diff --git a/internal/cli/cmdship/testing_pipeline.go b/internal/cli/cmdship/testing_pipeline.go index 1840174..8b6b94c 100644 --- a/internal/cli/cmdship/testing_pipeline.go +++ b/internal/cli/cmdship/testing_pipeline.go @@ -24,7 +24,7 @@ // is the default, guideline-only behavior for every project. // - fourStageTestingGate (post-checkpoint, qa-verify): checks for // .forge/specs//testing-pipeline.md evidence that all 4 stages -// ran. Advisory (HookResult.Passed always true) unless +// ran. Advisory (VerdictUnknown, never VerdictFail) unless // HookConfig.StrictTesting is set, in which case missing/incomplete // evidence fails the qa-verify checkpoint the same way // manualTestPlanGate already does for the manual test plan. @@ -90,43 +90,37 @@ var fourStageTestingGate = Hook{ Gate: "qa-verify", Handler: func(ctx HookContext) HookResult { if ctx.Result == nil || ctx.Result.Status == "fail" { - return HookResult{Passed: true} + return gateNotApplicable() } path := testingPipelineEvidencePath(ctx.Root, ctx.Description) data, err := os.ReadFile(path) if err != nil { if !ctx.StrictTesting { - return HookResult{Passed: true} - } - return HookResult{ - Passed: false, - Message: fmt.Sprintf( - "four-stage-testing-gate: %s not found. This gate became blocking by "+ - "default in 1.8.2 (re-released as 1.9.0) — if this run passed on an "+ - "earlier version, that is why. "+ - "Either document evidence for all 4 stages (local / pre-push / staging / "+ - "production) in %s, or waive the gate: `forge ship --no-strict-testing` for "+ - "one run, or \"strict-testing: false\" in .forge/hooks.yaml for the project", - filepath.Base(path), filepath.Base(path), - ), + return gatePass() } + return gateFail( + "four-stage-testing-gate: %s not found. This gate became blocking by "+ + "default in 1.8.2 (re-released as 1.9.0) — if this run passed on an "+ + "earlier version, that is why. "+ + "Either document evidence for all 4 stages (local / pre-push / staging / "+ + "production) in %s, or waive the gate: `forge ship --no-strict-testing` for "+ + "one run, or \"strict-testing: false\" in .forge/hooks.yaml for the project", + filepath.Base(path), filepath.Base(path), + ) } missing := missingTestingPipelineStages(string(data)) if len(missing) == 0 { - return HookResult{Passed: true} + return gatePass() } if !ctx.StrictTesting { - return HookResult{Passed: true} - } - return HookResult{ - Passed: false, - Message: fmt.Sprintf( - "four-stage-testing-gate: testing-pipeline.md missing evidence for: %s", - strings.Join(missing, "; "), - ), + return gatePass() } + return gateFail( + "four-stage-testing-gate: testing-pipeline.md missing evidence for: %s", + strings.Join(missing, "; "), + ) }, } @@ -152,6 +146,6 @@ var fourStageTestingReminder = Hook{ filepath.Base(testingPipelineEvidencePath(ctx.Root, ctx.Description)))) } fmt.Fprint(os.Stderr, b.String()) - return HookResult{Passed: true} + return gatePass() }, }