From 1c60f0975c165363718f8a4df2b606d05f615e48 Mon Sep 17 00:00:00 2001 From: agenticode <16611333+agenticode@users.noreply.github.com> Date: Wed, 26 Aug 2026 19:28:19 +0900 Subject: [PATCH] feat(explain): supply Explanation.Verdict from the operational path, or say it was not computed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Explanation.Verdict was nil-only. It is now supplied from the production recommendation path's readout (recommend.Verdicts) rather than by re-running decision.Evaluate here — a second evaluation can diverge from the verdict operations actually reached, which is a fabricated audit trail. Where the path reached none, the payload says so as a first-class VerdictOrigin state: thin is not absent. Also closes the §6.1 gap where a subject with no stored evidence exited 0 with a 200 payload that grounded none of its claims. Grounding state is now explicit. BuildExplain now errors on a decision.Verdict whose Action is not one of the three, and on Verdict and RecVerdict both being supplied. Both in-repo call sites pass nil, so today's blast radius is zero. Goldens are unmodified and pass unregenerated; BuildExplain's signature is unchanged. Co-authored-by: kording <74226694+kording@users.noreply.github.com> --- pkg/explain/VERDICT-FINDINGS.md | 495 ++++++++++++++++++++++++++++++ pkg/explain/grounding.go | 190 ++++++++++++ pkg/explain/grounding_test.go | 369 ++++++++++++++++++++++ pkg/explain/payload.go | 198 ++++++++++-- pkg/explain/verdict.go | 132 ++++++++ pkg/explain/verdict_test.go | 527 ++++++++++++++++++++++++++++++++ 6 files changed, 1893 insertions(+), 18 deletions(-) create mode 100644 pkg/explain/VERDICT-FINDINGS.md create mode 100644 pkg/explain/grounding.go create mode 100644 pkg/explain/grounding_test.go create mode 100644 pkg/explain/verdict.go create mode 100644 pkg/explain/verdict_test.go diff --git a/pkg/explain/VERDICT-FINDINGS.md b/pkg/explain/VERDICT-FINDINGS.md new file mode 100644 index 0000000..2af3374 --- /dev/null +++ b/pkg/explain/VERDICT-FINDINGS.md @@ -0,0 +1,495 @@ +# The verdict bridge, and the boundary between "thin" and "absent" + +Two payloads in `pkg/explain` sounded more certain than their inputs. Both are +closed. New code lives in `pkg/explain/verdict.go` (132 lines), +`pkg/explain/grounding.go` (190 lines) and 180 added lines in `payload.go`; +tests in `verdict_test.go` (527) and `grounding_test.go` (369). **No existing +test file was touched and no golden fixture was regenerated** — `testdata/` +is byte-identical, which is a deliberate design constraint discharged in §4.3. + +`gofmt`, `go vet ./...`, `go build ./...`, `go test -race -count=1 +./pkg/explain/...` and `go test -race -short ./...` (37 packages) all green. +Package coverage **92.0 %**. `go.mod` and `go.sum` are byte-identical +(`shasum` before and after); the two new intra-repo imports were already +imported by `payload.go`. + +**Actuators.** Nothing here moves toward them. `Action` can now hold a real +value, and that value is a *string in a payload*: this package still has no +reference to `pkg/ec2` or `pkg/rds`, no writer of any kind, and +`TestNoSecondEvaluationInThisPackage` (§1.3) fails on any call into another +decision plane, which is the same fence one notch tighter. + +--- + +## 1. Is the verdict bridge buildable without a second evaluation? **Yes — and it is built. It is just not a *verdict* yet.** + +### 1.1 The seam does not carry one today, and that is checkable, not assumed + +`(*Recommender).Verdicts(snap)` returns `[]recommend.Verdict`. The +`decision.Verdict` inside each one is unexported and reachable only through +`Decision() (decision.Verdict, bool)`, whose `ok` is false for every verdict +the recommender constructs — `Verdicts` sets `state: VerdictNotComputed` on +every element and no exported constructor can set anything else +(`pkg/recommend/VERDICT-FINDINGS.md` §7). So the answer to "did the +operational path produce a verdict?" is a boolean this package can *read* +rather than infer, which is the whole reason the bridge is honest. + +Both branches are implemented and both are tested: + +- **`ok == true`** → the `decision.Verdict` is copied into the payload + unchanged and `Action`, `Confidence` and `Refusal` come from it. Nothing is + re-derived. Reachable today only through the seam's own + `UnmarshalJSON` (which is how `TestComputedVerdictIsCopiedNotRecomputed` + reaches it); reachable from production the moment + `pkg/recommend/VERDICT-FINDINGS.md` §7 step 4 lands, with **no further + change in this package**. +- **`ok == false`** → the typed "not computed" state, §2. This is what every + production readout produces today. + +### 1.2 The proof that it is a copy and not a recomputation + +Three independent proofs, because "we don't recompute" is exactly the claim a +reader should not take on trust. + +1. **Behavioural, and adversarial.** `TestComputedVerdictIsCopiedNotRecomputed` + hands the payload a readout whose verdict says `act`, over a store whose + evidence a second evaluation refuses. The test *computes the second + opinion itself* — `decision.Evaluate` over the same subject's evidence, + assembled the way a caller filling the gap would assemble it — and + `t.Fatal`s if that second opinion happens to agree, so the test cannot + quietly stop being a test. The payload must still say `act`, with the + readout's `Confidence` structurally equal (`reflect.DeepEqual`) to the one + supplied. A recomputation flips it to `refuse` and the test says so. +2. **Structural, and fail-closed.** `TestNoSecondEvaluationInThisPackage` + parses every non-test file in the package and fails on any call whose + receiver is the `decision` or `recommend` package, except an allowlist of + two type conversions. A *new* `decision.X(...)` call fails the test until + someone justifies it — the list is of what is permitted, not of what is + forbidden, so a function that does not exist yet is already covered. It + also fails if the scan finds no selectors at all, which is how a scan + silently stops looking. +3. **Arithmetic, by absence.** This package computes no confidence, no + refusal predicate and no action. `Confidence` is assigned from + `verdict.Confidence` and from nowhere else. + +### 1.3 Where the seam is *not* closed, named exactly + +The bridge is buildable; the **verdict** is not, and that limit is +`pkg/recommend`'s, not this package's. `pkg/recommend/VERDICT-FINDINGS.md` §1 +establishes with a predicate-by-predicate table that none of `pkg/decision`'s +eight refusal predicates runs on the production path, no `Action` is chosen +(the act threshold lives in `plan.Config.MinConfidence`), and the confidence +that does exist is a bare float with no `Basis` — so there is nothing to read +out. This package re-confirms that from its own side rather than trusting it: +`rv.Decision()` returns `ok == false` for the readout every fixture builds +straight from `recommend.Verdict`. + +**So `kilter explain` still prints `unknown`, and that is the correct +answer.** What changed is that it can now say *which* unknown, and why. + +--- + +## 2. "Refused", "not computed" and "unknown" — the type-level difference + +These are three facts and the payload previously had two shapes for them. +Every field below is a field a consumer can switch on, in Go and on the wire. + +| | `Action` | `Refusal` | `Confidence` | `VerdictOrigin` | `VerdictState()` | `Refused()` | +|---|---|---|---|---|---|---| +| **refused** | `"refuse"` | non-nil, with `Code`/`Detail`/`Until` | non-nil | nil | `computed` | **true** | +| **not computed** | `"unknown"` | **nil** | **nil** | non-nil, `State: "not-computed"`, `Disposition` set | `not-computed` | false | +| **unknown** | `"unknown"` | nil | nil | **nil** | `unknown` | false | + +The distinction that carries the weight is the second row against the first. +A `recommend.Disposition` is a report of a branch the recommender took; a +`decision.Refusal` is a judgement with a code you can cite and a time it +clears. `recommend.DispositionInsufficientHistory` and +`decision.CodeInsufficientHistory` share a name and, at default config, the +same two numbers — and they are still two thresholds in two independently +settable `Config`s (`pkg/recommend/VERDICT-FINDINGS.md` §1.2). Rendering one +as the other would be a refusal nobody issued, on a subject nobody judged. + +**Prose distinguishes all three**, because the no-model path is what most +operators actually read: + +``` +Verdict: refuse (post-change-soak) — deploy 6h ago… Clears no earlier than … +Verdict: not computed — the recommendation path reported disposition + "insufficient-history" (12 samples over 3h0m0s). That is an absent + verdict, not a negative one. +Verdict: none recorded. +``` + +### The tests pinning each + +| Claim | Test | +|---|---| +| a readout with no verdict is not-computed, not refused, and carries the disposition, samples and window | `TestReadoutWithoutAVerdictIsNotComputedNotRefused` | +| **no** disposition — all four walked — produces a `Refusal`, a refusal `Driver`, a non-unknown `Action`, or a `"refusal"` key on the wire | `TestNoDispositionIsEverRenderedAsARefusalCode` | +| the three states differ in `Action`/`Refusal`/`VerdictOrigin`/`Refused()` **and render three distinct prose lines** (a set is built and asserted to have size 3) | `TestTheThreeStatesAreDistinguishable` | +| the two absences survive JSON as different documents; a decoded not-computed payload does not read as refused | `TestVerdictStateSurvivesJSON` | +| a computed verdict is reported verbatim, not re-derived | `TestComputedVerdictIsCopiedNotRecomputed` | +| the readout's `Recommendation` is the payload's sizing | `TestReadoutSuppliesTheServedRecommendation` | + +Five mutations were applied to the shipped files, the suite run, then +reverted. All were caught by name: + +| Mutation | Caught by | +|---|---| +| `originOf` reports `VerdictComputed` | 3 tests, incl. the prose-distinctness set | +| a disposition is mapped onto `ActionRefuse` + `CodeInsufficientHistory` | `TestNoDispositionIsEverRenderedAsARefusalCode` (all 4 subtests) | +| the copied verdict replaced by a `decision.Decide` call | `TestComputedVerdictIsCopiedNotRecomputed` **and** the structural scan | +| two verdict sources silently accepted | `TestTwoVerdictSourcesAreRefused` | +| a readout about another container accepted | `TestReadoutMustBeAboutTheSubject` | + +### 2.1 Three fabrications the request now refuses outright + +A bridge that accepts contradictory inputs invents an answer by choosing one. +`ExplainRequest.validate` therefore rejects, with the reason in the error: + +- **Both `Verdict` and `RecVerdict`.** Two sources for one disposition; the + payload would have to pick and picking silently is the failure. +- **A readout about a different container**, or a container readout on a + workload subject. Attributing one container's disposition to another + subject is the same fabrication as inventing it. +- **`Rec` disagreeing with `RecVerdict.Rec`** — either a different sizing, or + a `Rec` supplied alongside a disposition that carries none. The sizing and + the disposition must come from the same answer. Identical sizings pass. + +--- + +## 3. The absent-vs-thin boundary, and its arithmetic + +### 3.1 The old rule was `len(Citations) == 0`, and it was wrong in both directions + +That test conflates "the store holds nothing about this subject" with "the +payload could not cite what the store does hold". A **demonstrated** +false positive, now a regression test +(`TestUsageOutsideTheRequestedTierIsThinNotAbsent`): the dossier's usage +summary queries *every* stored tier, while the `usage-history` driver may only +cite digests of the **requested** tier. Two samples that have not yet rolled +into an hourly digest therefore produce a payload carrying that subject's +usage — `"samples": 2`, real percentiles — under the note *"no evidence is +stored for this subject in this window"*. The payload contradicted itself two +fields apart. + +### 3.2 The rule + +The state is computed from what the **store returned for the subject**, never +from what the payload managed to cite: + +``` +own := Digests + Events + Decisions + UsageWindows + Samples + Withheld (Grounding.Any) + ── ParentEvents excluded ── + +absent ⟺ own == 0 +thin ⟺ own > 0 ∧ len(Citations) == 0 +grounded ⟺ own > 0 ∧ len(Citations) > 0 +unknown ⟺ no Grounding report on the payload ∧ len(Citations) == 0 +``` + +`GroundingError()` returns a `*NoEvidenceError` (wrapping `ErrNoEvidence`) for +**`absent` and nothing else**. `thin` keeps its payload and no error, which is +the requirement that made this a boundary problem rather than a deletion. + +Three terms in that sum are load-bearing and each has a test: + +- **`UsageWindows`/`Samples` is the witness a caller cannot suppress.** + `BuildDossier` computes the usage summary through a query of its own, not + gated by `MaxDigests`, falling through every tier. So "no digest in the + requested tier" can never be read as "no usage data" (§3.1's bug), and + digest absence needs no separate proof. +- **`Withheld` is what makes absence honest under a caller's own caps.** + `MaxEvents < 0` empties the events section — but the dossier still reports + how many it dropped, and a drop is proof of existence. Without this term, a + caller who asked for nothing is told the subject does not exist: + `TestAbsenceIsNeverClaimedAboutASectionNobodyAskedFor` sets all three caps + negative over an events-only store (usage deliberately empty, so the + suppressed section is the *only* evidence) and requires `thin`. +- **`ParentEvents` is excluded on purpose.** `BuildExplain` borrows the parent + workload's deploys and HPA actions so a container's post-change refusal can + be explained at all. Those describe the *workload*. A container that never + ran, under a workload that deploys weekly, would otherwise be declared + grounded by its parent's history — which is precisely the mistyped-container + case. `TestParentEventsDoNotGroundTheSubject` builds it: the payload cites + the borrowed deploy, `Any()` is false, the state is `absent`, the error + fires, and the note says the citations are borrowed rather than repeating + "grounds none of it", which would be false in that one case. + +`GroundingUnknown` exists so a hand-built or re-decoded payload that never +computed absence cannot assert it. It does **not** produce the error: absence +is a computed fact and only a computed fact may be claimed +(`TestGroundingStateNeverSilentlyClaimsAbsence`). + +### 3.3 What was left alone, deliberately + +There is no sample-count or window threshold anywhere in this file. "Weak" +is a policy and this package owns no policy — it prices nothing and will not +guess, and `recommend.Config.MinSamples` is not its constant to read. The +boundary above is purely structural: it asks *does a record exist*, never +*is the record good enough*. A caller wanting a stricter notion of thin has +`Usage.Samples`, `Usage.Windows` and the whole `Grounding` block to apply its +own threshold to. `TestThinHistoryStaysGrounded` pins the consequence: five +samples — thin by the recommender's own gate of thirty — is `grounded`, has +no error, and ships. + +### 3.4 The mutations + +| Mutation | Caught by | +|---|---| +| `absent` collapses into `thin` | 4 tests, incl. the arithmetic table | +| borrowed parent events count as the subject's own | `TestParentEventsDoNotGroundTheSubject` + the table | +| a cap-suppressed section reads as absence (`Withheld` zeroed) | `TestAbsenceIsNeverClaimedAboutASectionNobodyAskedFor` | +| the error widens to cover `thin` (deleting the thin payload) | 4 tests | + +--- + +## 4. Blast radius + +### 4.1 Signatures changed: **none** + +`BuildExplain(ExplainRequest) (*Explanation, error)` is untouched, and so is +every other exported function. This was a constraint, not an accident: +`pkg/api/explainroutes.go:205` and `cmd/kilter/explain.go:538` are the only +two call sites in the repo and both are being edited by other agents right +now. The typed signal is a method on the payload +(`(*Explanation).GroundingError`), so a caller that has not adopted it +compiles and behaves exactly as before. The cost is that a caller can forget +to call it; §5 gives both call sites verbatim, and both were compiled and +tested here before being reverted. + +### 4.2 One behaviour change inside an unchanged signature + +`BuildExplain` now returns an error for a `Verdict` whose `Action` is not one +of `pkg/decision`'s three. The zero `decision.Verdict` has `Action: ""`, +which used to reach `Prose` and render as a blank verdict — a payload +asserting a disposition that is not one. `decision.Decide` cannot produce it, +so it is a bug at the call site and now says so. + +**Blast radius, enumerated:** every in-repo caller that passes a +`*decision.Verdict`. There are two `BuildExplain` call sites +(`pkg/api/explainroutes.go:205`, `cmd/kilter/explain.go:538`) and **both pass +`Verdict: nil` today**, so neither is affected. `go build ./...` and +`go test -race -short ./...` across 37 packages confirm it. The only two +`explain.ExplainRequest` literals outside this package are those same two call +sites (`cmd/kilter/explain.go:531`, `pkg/api/explainroutes.go:205`); no test +in any other package builds one. + +### 4.3 Wire compatibility, and why the goldens did not move + +`Explanation` gained two fields, both pointers with `omitempty`, both +populated **only when the payload is not in the good state** — +`VerdictOrigin` only for `not-computed`, `Grounding` only for `thin`/`absent`. +A grounded payload carrying a verdict serializes to the same bytes it did +before, which is why `testdata/golden/explain_recommendation.json` and its +prose fixture are unmodified and pass unregenerated. +`TestGroundedPayloadCarriesNoNewBytes` pins that as an intended property +rather than a coincidence, so a future field cannot silently break the +fixtures either. + +The trade-off, stated because it is a real one: on the wire, the *absence* of +`"grounding"` means grounded and the absence of `"verdictOrigin"` means the +verdict was computed. That is the inverse of the pattern +`recommend.Verdict.MarshalJSON` uses (always emit `verdictState`), and it is +weaker. Two things make it safe here. `Action` is a total function of the +verdict's existence — only a supplied `decision.Verdict` can set it to +anything but `unknown`, now that §4.2 rejects the third possibility — so +`VerdictState()` reconstructs `computed` from a field that is always present. +And `GroundingState()` falls back to `GroundingUnknown`, never to +`GroundingGrounded`, when a payload has neither a report nor citations. Both +fallbacks are tested against decoded documents, not just constructed ones. + +### 4.4 Callers that read `Explanation.Notes` by string + +The absent case's note text is **unchanged, character for character**, and +the new thin-case note deliberately does *not* contain the substring +`"no evidence is stored"`. Anything matching on that phrase keeps working and +stops matching the cases where the phrase was false. + +--- + +## 5. Caller changes specified but NOT made + +Both were applied in this worktree, compiled, run against their existing +suites (`pkg/api` and `cmd/kilter`, both green), verified end to end against +the §6.1 scenario, and then **reverted**. `pkg/api` and `cmd/` are other +agents' scope. + +### 5.1 `pkg/api/explainroutes.go` — the 422 + +In `(*Brain).Explain`, after the `Verify` gate (~line 219): + +```go + // §5.7's publish gate, before anything is shown. + if err := payload.Verify(explain.Resolver{Store: b.mem}); err != nil { + return nil, unverifiable{err} + } + // A subject the substrate holds no record of is a 422, not a 200 carrying + // a payload that grounds nothing (§3.2; cmd/BRAINWIRE-FINDINGS.md §6.1). + // Thin evidence is NOT this case and still returns its payload. + if err := payload.GroundingError(); err != nil { + return nil, notEnoughEvidence{err} + } + return payload, nil +``` + +`explainStatus` already maps `notEnoughEvidence` to +`http.StatusUnprocessableEntity`, so this is the whole change: no new error +type, no route edit. Verified: `go test ./pkg/api/...` green with it applied +— no existing test asserts a 200 for an unknown subject. + +**A choice left to that package:** `writeErr` discards the payload, so the 422 +carries a message rather than the (honest, window-stating) document. Serving +the payload *with* a 422 status is a one-line alternative in the route and is +arguably better for a UI. Not decided here. + +### 5.2 `cmd/kilter/explain.go` — the non-zero exit + +At the end of `runExplainTo` (~line 545), replacing the tail call: + +```go + if err := renderExplanation(w, key, start, end, payload, *jsonOut); err != nil { + return err + } + // The payload is printed first — it is honest and it names the window — + // and then the command fails, because the subject the operator asked + // about has no record in the substrate. + // + // Returned unwrapped: the error already names the package, the subject + // and the window, and a second "explain:" prefix reads as a stutter. + if err := payload.GroundingError(); err != nil { + return err + } + return nil +``` + +`explainFromBrain` (the `--db` path) needs **no edit**: it returns whatever +`brain.Explain` returns, so §5.1 gives it the non-zero exit for free — at the +cost of not printing the payload, which is the same trade-off §5.1 names. + +Verified end to end against `cmd/BRAINWIRE-FINDINGS.md` §6.1's exact +scenario, using the repo's own `cluster.json` fixture and a one-character +container typo: + +``` +kilter explain — Deployment/default/api/ap over [2026-08-23T12:15:00Z, 2026-08-26T12:00:01Z] + +Subject prod-eks/container/Deployment/default/api/ap over 2026-08-23 12:15Z → 2026-08-26 12:00Z. +Verdict: none recorded. +Because: + note: no evidence is stored for this subject in this window; the payload states the decision but grounds none of it + +$ echo $? # was 0 +1 +error: explain: no evidence is stored for subject + prod-eks/container/Deployment/default/api/ap in [2026-08-23T12:15:00Z, 2026-08-26T12:00:01Z) +``` + +The real container (`api`) still exits 0 in the same run — asserted in the +same throwaway test, which was deleted with the reverts. + +### 5.3 `cmd/kilter/explain.go` — the verdict readout + +This is `pkg/recommend/VERDICT-FINDINGS.md` §5's wiring, updated for what +this package now offers. That document's advice was to pass +`Verdict: nil` plus a hand-built `Note` appended after `BuildExplain`; do +**not** do that any more. Pass the readout and let the payload type carry it, +so the disposition lands in a typed field instead of a string a consumer has +to parse. + +Replace the `Recommendations` scan at `cmd/kilter/explain.go:523–529` and the +request literal that follows it (`:531`): + +```go + var readout *recommend.Verdict + for _, v := range rec.Verdicts(series[len(series)-1]) { + if v.Key == key { + readout = &v + break + } + } + + req := explain.ExplainRequest{ + Cluster: cluster, + Subject: evidence.ContainerSubject(cluster, key), + From: start, To: end, + Store: store, + RecVerdict: readout, // supplies Rec too; do not also set Rec + } +``` + +Three rules, all load-bearing: + +1. **Do not set `Rec` as well.** `RecVerdict.Rec` is byte-for-byte the + `Recommendation` production served for that key on that snapshot, and + `BuildExplain` takes it. Setting both is accepted only if they are + identical and is an error otherwise (§2.1) — passing just the readout + makes the question moot. +2. **`Action` stays `unknown`, and that is the win.** `Decision()` is + `ok == false` for every readout the recommender produces today, so the + payload says `not computed` and names the branch, instead of a bare + `unknown` that could mean anything. +3. **Never map a `Disposition` onto a `decision.Action` or a + `decision.Refusal`.** The type gives you nowhere to put it, which is on + purpose: `VerdictOrigin.Disposition` is a separate field precisely so it + cannot be mistaken for a refusal code. + +`readout = &v` is safe under Go 1.22+ loop semantics (`go 1.26.4` in +`go.mod`), where each iteration has its own `v`. + +Verified: applied here, `go build ./...` and `go test ./cmd/...` green, then +reverted. The user-visible change on the repo's own fixture, which is what an +operator running `explain` is actually asking for: + +``` +- Verdict: none recorded. ++ Verdict: not computed — the recommendation path reported disposition ++ "recommended" (288 samples over 71h45m0s). That is an absent ++ verdict, not a negative one. +``` + +Note what that says: the recommender *did* produce a recommendation for this +container — the sizing is right there in the payload — and still reached no +decision-quality verdict, because nothing on that path evaluates one. A bare +`unknown` could not tell those two things apart. + +The same change applies to `pkg/api`'s `(*Brain).Explain`, which today scans +`b.Recommendations(cluster)` for the key: `b.rec.Verdicts(snap)` yields the +readout and the recommendation together, so the two can no longer come from +two different reads of the recommender. **[unverified]** — unlike §5.1 and +§5.2 this variant was not compiled here, because it needs a snapshot argument +that `Brain.Explain` does not currently hold in scope. + +--- + +## 6. What would make `Action` real, and what this package will need + +Nothing, in this package. When `pkg/recommend/VERDICT-FINDINGS.md` §7 step 4 +sets `state = VerdictComputed`, `rv.Decision()` starts returning `ok == true`, +`BuildExplain` copies the verdict, `Action` becomes `act` /`recommend-only` / +`refuse`, the `Confidence.Basis` terms become individually-grounded +`Driver`s through the code that already exists, and `VerdictOrigin` stops +being emitted. The path is exercised today by +`TestComputedVerdictIsCopiedNotRecomputed` — the branch is not speculative +code, it is tested code waiting for a caller. + +One thing that will need attention when it lands, flagged now: a refusal +`Driver` must be citable, and `refusalCitations` grounds each code in the +evidence class that justifies it. A refusal arriving for a subject whose +grounding state is `thin` will have its driver dropped and counted in +`Ungrounded`. That is correct behaviour — an ungrounded reason is worse than +a missing one — but it means a refusal can be *reported in `Action` and +`Refusal`* while having no `Driver`. The payload already says so via +`Ungrounded` and its note. **[unverified]**: no fixture exercises a refusal +over a thin store today, because no production path produces a refusal at +all. + +## 7. Determinism + +No clock and no map iteration is reachable from any new code. Both new fields +are computed from integer counts already in the dossier, and the dossier is +itself deterministic. `TestAbsentPayloadIsDeterministic` marshals the absent +payload 16 times over independently built stores and requires byte equality; +the pre-existing `TestExplainIsDeterministic` still covers the grounded one. +The `Window` in `VerdictOrigin` is copied from the readout, never derived from +`From`/`To`, so it cannot drift with the requested window. diff --git a/pkg/explain/grounding.go b/pkg/explain/grounding.go new file mode 100644 index 0000000..1b88ea8 --- /dev/null +++ b/pkg/explain/grounding.go @@ -0,0 +1,190 @@ +package explain + +import ( + "errors" + "fmt" + "time" + + "github.com/agenticode/kilter/pkg/evidence" +) + +// This file answers one question about a finished payload: how much of what +// it asserts stands on evidence the store actually holds *for the subject*. +// +// The question exists because BuildExplain answers for any well-formed +// subject, whether or not the substrate has ever heard of it. A mistyped +// container name therefore produced a payload — an honest one, it says it +// grounds nothing — which the HTTP route served as 200 and the CLI printed +// with exit 0 (cmd/BRAINWIRE-FINDINGS.md §6.1). A confident-looking answer to +// a question about a subject that does not exist is the failure this file +// makes typed. + +// GroundingState says how much of the payload stands on stored evidence. +// +// The load-bearing distinction is absent vs thin, and it is not a matter of +// degree: +// +// - **absent** is a statement about the *subject*. The store returned no +// record of any kind for it in this window — no digest in any tier, no +// event, no decision, and nothing a cap withheld. Either the subject was +// mistyped or it never ran here. There is nothing to explain, and saying +// so is the only answer that is not a guess. +// - **thin** is a statement about the *history*. Records exist; they are +// just too few, or of the wrong kind, for any driver to stand on. That is +// a legitimate answer callers ask for on purpose — a young workload has +// thin history and an operator still wants to see its usage and its +// window — so it must stay a payload. +// +// Collapsing the two either deletes the thin answer or hands a typo a 200. +type GroundingState string + +const ( + // GroundingGrounded: the store holds records for this subject in this + // window, and at least one driver cites them. + GroundingGrounded GroundingState = "grounded" + // GroundingThin: records exist for this subject, but no driver could be + // grounded in them, so the payload cites nothing. An answer, not an + // error: the usage summary, the window and the sizing are all still + // true, they simply have no citable reason attached. + GroundingThin GroundingState = "thin" + // GroundingAbsent: the store holds no record of this subject in this + // window at all. This is the state [Explanation.GroundingError] turns + // into an error — a 422 and a non-zero exit — and the only one it does. + GroundingAbsent GroundingState = "absent" + // GroundingUnknown: the payload carries no [Grounding] report and cites + // nothing, so whether evidence exists was never computed. BuildExplain + // never produces this; a hand-built payload, or one decoded from a + // document whose grounding object was stripped, does. It deliberately + // does NOT read as absent: absence is a computed fact, and a payload + // that did not compute it may not claim it. + GroundingUnknown GroundingState = "unknown" +) + +// Grounding is the arithmetic behind [GroundingState] — the counts the state +// was computed from, so a reader can check the verdict rather than trust it. +// +// Every count except ParentEvents is a record the store returned *for the +// subject*. ParentEvents is kept out of that sum on purpose: they are the +// parent workload's deploys and HPA actions, borrowed by BuildExplain so a +// container's post-change refusal can be explained at all. They describe the +// workload. A container that never ran under a workload that deploys weekly +// has a parent with plenty of events and still has no history of its own. +type Grounding struct { + State GroundingState `json:"state"` + + // Digests, Events and Decisions are what the dossier returned for the + // subject after its caps were applied. + Digests int `json:"digests"` + Events int `json:"events"` + Decisions int `json:"decisions"` + // UsageWindows and Samples come from the dossier's usage summary, which + // is computed by a query of its own across every stored tier and is not + // gated by MaxDigests. It is therefore the one witness a caller cannot + // suppress, and the reason "no digests in the requested tier" can never + // be mistaken for "no usage data". + UsageWindows int `json:"usageWindows"` + Samples int64 `json:"samples"` + // Withheld counts records the store did return for the subject that a + // count cap or the byte cap dropped before the payload saw them. It is + // what makes absence honest under a caller who asked for nothing: + // MaxEvents < 0 empties Events, but the dossier still reports what it + // dropped, so the section was queried and the answer is "records exist", + // not "no records". + Withheld int `json:"withheld,omitempty"` + // ParentEvents counts change events borrowed from the parent workload. + // Not evidence about the subject; see the type comment. + ParentEvents int `json:"parentEvents,omitempty"` + // Citations is len(Explanation.Citations) — how many ids the finished + // payload actually leans on. + Citations int `json:"citations"` +} + +// Any reports whether the store holds any record of the subject itself in +// this window. It is the absent/not-absent test, and the whole of it. +func (g Grounding) Any() bool { + return g.Digests > 0 || g.Events > 0 || g.Decisions > 0 || + g.UsageWindows > 0 || g.Samples > 0 || g.Withheld > 0 +} + +// stateFor resolves the three computable states. Citations is passed rather +// than read off the struct so the caller cannot forget to set it first. +func (g Grounding) stateFor(citations int) GroundingState { + switch { + case !g.Any(): + return GroundingAbsent + case citations == 0: + return GroundingThin + default: + return GroundingGrounded + } +} + +// ErrNoEvidence reports a subject the store holds no record of in the +// requested window. Callers match it with errors.Is; pkg/api maps it to 422 +// and cmd/kilter to a non-zero exit. +var ErrNoEvidence = errors.New("explain: no evidence is stored for this subject in this window") + +// NoEvidenceError is the typed form, carrying the subject, the window and +// the counts the answer was computed from — everything an operator needs to +// tell "I mistyped the name" from "I asked about the wrong window". +// +// It wraps [ErrNoEvidence]. The payload is deliberately not attached: a +// caller that wants it already has it, because this error is produced from a +// payload rather than instead of one. +type NoEvidenceError struct { + Subject evidence.SubjectRef + From, To time.Time + Grounding Grounding +} + +func (e *NoEvidenceError) Error() string { + s := fmt.Sprintf("explain: no evidence is stored for subject %s in [%s, %s)", + e.Subject, e.From.UTC().Format(time.RFC3339), e.To.UTC().Format(time.RFC3339)) + if e.Grounding.ParentEvents > 0 { + s += fmt.Sprintf("; the %d citation%s in the payload %s borrowed from the parent workload", + e.Grounding.ParentEvents, pluralS(e.Grounding.ParentEvents), + pluralVerb(e.Grounding.ParentEvents, "is", "are")) + } + return s +} + +func (e *NoEvidenceError) Unwrap() error { return ErrNoEvidence } + +// GroundingState reports how much of this payload stands on stored evidence. +// It never returns "". +// +// A payload BuildExplain produced always carries a [Grounding] report unless +// it is grounded, so the fallbacks below only fire for a hand-built or +// re-decoded payload: citations mean grounded, and nothing at all means +// [GroundingUnknown], never absent. +func (e *Explanation) GroundingState() GroundingState { + if e.Grounding != nil && e.Grounding.State != "" { + return e.Grounding.State + } + if len(e.Citations) > 0 { + return GroundingGrounded + } + return GroundingUnknown +} + +// GroundingError returns a [*NoEvidenceError] when, and only when, the store +// held no record of this subject in this window — the state a caller turns +// into a 422 and a non-zero exit. It returns nil for thin history, because +// thin history is an answer, and nil for [GroundingUnknown], because a +// payload that never computed absence must not assert it. +// +// It is a method rather than a second return value from BuildExplain on +// purpose: BuildExplain's signature is load-bearing in pkg/api and cmd/, and +// an additive check breaks no caller that has not adopted it. The cost is +// that a caller can forget to call it; VERDICT-FINDINGS.md §5 gives both call +// sites verbatim. +func (e *Explanation) GroundingError() error { + if e.GroundingState() != GroundingAbsent { + return nil + } + var g Grounding + if e.Grounding != nil { + g = *e.Grounding + } + return &NoEvidenceError{Subject: e.Subject, From: e.From, To: e.To, Grounding: g} +} diff --git a/pkg/explain/grounding_test.go b/pkg/explain/grounding_test.go new file mode 100644 index 0000000..b6ec093 --- /dev/null +++ b/pkg/explain/grounding_test.go @@ -0,0 +1,369 @@ +package explain + +import ( + "encoding/json" + "errors" + "strings" + "testing" + "time" + + "github.com/agenticode/kilter/pkg/evidence" +) + +// emptyStore is a substrate that has never heard of anything. +func emptyStore(t *testing.T) *evidence.Memory { + t.Helper() + mem, err := evidence.NewMemory(evidence.Config{}) + if err != nil { + t.Fatalf("NewMemory: %v", err) + } + return mem +} + +// typoSubject is the shape of cmd/BRAINWIRE-FINDINGS.md §6.1's observation: a +// container name that does not exist, differing from a real one by a +// character. Nothing about it is malformed, so validation cannot catch it. +func typoSubject() evidence.SubjectRef { + return evidence.ContainerSubject(testCluster, containerKey("payments", "api", "serve")) +} + +// TestAbsentSubjectIsTypedNotJustNoted is the §6.1 gap. The payload was +// already honest in prose; what it lacked was a signal a route could turn +// into a 422 and a shell into a non-zero exit. +func TestAbsentSubjectIsTypedNotJustNoted(t *testing.T) { + req := explainRequest(t) + req.Store = explainStore(t) + req.Subject = typoSubject() + req.Rec, req.Verdict = nil, nil + + ex := mustExplain(t, req) + + if got := ex.GroundingState(); got != GroundingAbsent { + t.Fatalf("GroundingState = %q, want %q", got, GroundingAbsent) + } + err := ex.GroundingError() + if err == nil { + t.Fatal("an unknown subject produced no error; this is the 200-and-exit-0 bug") + } + if !errors.Is(err, ErrNoEvidence) { + t.Errorf("error %v does not match ErrNoEvidence, so callers cannot switch on it", err) + } + var ne *NoEvidenceError + if !errors.As(err, &ne) { + t.Fatalf("error %v is not a *NoEvidenceError", err) + } + if ne.Subject != ex.Subject || !ne.From.Equal(ex.From) || !ne.To.Equal(ex.To) { + t.Errorf("error names %s over [%v,%v), payload is %s over [%v,%v)", + ne.Subject, ne.From, ne.To, ex.Subject, ex.From, ex.To) + } + // The payload is still returned. The route may answer 422 with it; the + // point is that it now has the choice. + if len(ex.Notes) == 0 || !strings.Contains(strings.Join(ex.Notes, " "), "no evidence is stored") { + t.Errorf("the note that always said this must survive: %v", ex.Notes) + } + if err := ex.Verify(Resolver{Store: req.Store}); err != nil { + t.Fatalf("an ungrounded payload must still verify: %v", err) + } +} + +// TestThinEvidenceIsNotAbsent is the other half, and the one that makes the +// fix safe: a subject the store DOES know, whose records simply ground no +// driver, must keep its payload and must not raise the error. +func TestThinEvidenceIsNotAbsent(t *testing.T) { + mem := emptyStore(t) + subj := explainSubject() + // One OOMKill and nothing else. It is a real record about a real + // subject, and no driver can stand on it without a recommendation to + // attach the OOM floor to — so the payload cites nothing. + if err := mem.Append(ev(t0.Add(hours(3)), evidence.EventOOMKill, evidence.SeverityCritical, + subj, map[string]string{"container": "server"})); err != nil { + t.Fatalf("Append: %v", err) + } + req := explainRequest(t) + req.Store, req.Rec, req.Verdict = mem, nil, nil + + ex := mustExplain(t, req) + + if got := ex.GroundingState(); got != GroundingThin { + t.Fatalf("GroundingState = %q, want %q (grounding=%+v)", got, GroundingThin, ex.Grounding) + } + if err := ex.GroundingError(); err != nil { + t.Fatalf("thin evidence must not be an error, it is an answer: %v", err) + } + if len(ex.Citations) != 0 { + t.Fatalf("fixture is wrong: expected an uncitable payload, got %v", ex.Citations) + } + if ex.Grounding == nil || ex.Grounding.Events != 1 { + t.Errorf("the stored event must be counted: %+v", ex.Grounding) + } + joined := strings.Join(ex.Notes, " ") + if strings.Contains(joined, "no evidence is stored") { + t.Errorf("a subject with a stored event must not be told it has no evidence: %v", ex.Notes) + } + if !strings.Contains(joined, "but no driver could be grounded in it") { + t.Errorf("the thin case needs its own sentence: %v", ex.Notes) + } +} + +// TestThinHistoryStaysGrounded pins the path the fix must not break: five +// samples is thin history by any standard — the recommender wants thirty — +// and it is a perfectly good explanation that cites its digests. +func TestThinHistoryStaysGrounded(t *testing.T) { + mem := emptyStore(t) + subj := explainSubject() + for i := 0; i < 5; i++ { + if err := mem.ObserveSample(subj, evidence.Sample{ + At: t0.Add(time.Duration(i) * 40 * time.Minute), MilliCPU: 100, MemoryBytes: 1 << 20, + }); err != nil { + t.Fatalf("ObserveSample: %v", err) + } + } + req := explainRequest(t) + req.Store, req.Rec, req.Verdict = mem, nil, nil + + ex := mustExplain(t, req) + + if got := ex.GroundingState(); got != GroundingGrounded { + t.Fatalf("GroundingState = %q, want %q: thin is not absent and neither is small", + got, GroundingGrounded) + } + if err := ex.GroundingError(); err != nil { + t.Fatalf("two samples is an answer, not an error: %v", err) + } + if ex.Grounding != nil { + t.Errorf("a grounded payload carries no grounding report, its citations are the report: %+v", ex.Grounding) + } + if len(ex.Citations) == 0 { + t.Error("five samples must still produce a citable usage-history driver") + } +} + +// TestUsageOutsideTheRequestedTierIsThinNotAbsent is a bug this unit found +// while drawing the boundary. The usage summary queries every stored tier; +// the usage-history driver may only cite digests of the REQUESTED tier. A +// subject whose history has not yet rolled into an hourly digest therefore +// gets a payload with a populated Usage block, real sample counts — and zero +// citations. Under the old rule (citations == 0) it was told "no evidence is +// stored for this subject in this window", while carrying that subject's +// usage two fields higher up. The statement was simply false. +func TestUsageOutsideTheRequestedTierIsThinNotAbsent(t *testing.T) { + mem := emptyStore(t) + subj := explainSubject() + for i := 0; i < 2; i++ { + if err := mem.ObserveSample(subj, evidence.Sample{ + At: t0.Add(time.Duration(i) * time.Minute), MilliCPU: 100, MemoryBytes: 1 << 20, + }); err != nil { + t.Fatalf("ObserveSample: %v", err) + } + } + req := explainRequest(t) + req.Store, req.Rec, req.Verdict = mem, nil, nil + req.DigestTier = evidence.TierHourly // the default; two samples have not rolled into one + + ex := mustExplain(t, req) + + if ex.Usage.Samples != 2 { + t.Fatalf("fixture is wrong: usage reports %d samples, want 2", ex.Usage.Samples) + } + if len(ex.Citations) != 0 { + t.Fatalf("fixture is wrong: expected no citable hourly digest, got %v", ex.Citations) + } + if got := ex.GroundingState(); got != GroundingThin { + t.Fatalf("GroundingState = %q, want %q", got, GroundingThin) + } + if err := ex.GroundingError(); err != nil { + t.Fatalf("a subject with two stored samples is not an unknown subject: %v", err) + } + if strings.Contains(strings.Join(ex.Notes, " "), "no evidence is stored") { + t.Errorf("the payload states this subject's usage; it may not also say there is none: %v", ex.Notes) + } + if ex.Grounding == nil || ex.Grounding.UsageWindows == 0 { + t.Errorf("the unsuppressable usage witness must be counted: %+v", ex.Grounding) + } +} + +// TestAbsenceIsNeverClaimedAboutASectionNobodyAskedFor: a caller who +// suppresses a section must not be told the subject does not exist. The +// dossier reports what its caps dropped, and that report is proof of +// existence. +func TestAbsenceIsNeverClaimedAboutASectionNobodyAskedFor(t *testing.T) { + mem := emptyStore(t) + subj := explainSubject() + // Events only: no samples, so the usage summary — the witness a caller + // cannot suppress — is empty and the suppressed section is the only + // evidence there is. + for i := 0; i < 3; i++ { + if err := mem.Append(ev(t0.Add(hours(i+1)), evidence.EventOOMKill, evidence.SeverityCritical, + subj, map[string]string{"container": "server"})); err != nil { + t.Fatalf("Append: %v", err) + } + } + req := explainRequest(t) + req.Store, req.Rec, req.Verdict = mem, nil, nil + req.MaxEvents, req.MaxDecisions, req.MaxDigests = -1, -1, -1 + + ex := mustExplain(t, req) + + if ex.Grounding == nil || ex.Grounding.Withheld != 3 { + t.Fatalf("the three withheld events must be counted: %+v", ex.Grounding) + } + if got := ex.GroundingState(); got != GroundingThin { + t.Fatalf("GroundingState = %q, want %q: a cap is not an absence", got, GroundingThin) + } + if err := ex.GroundingError(); err != nil { + t.Fatalf("suppressing a section must not manufacture a 422: %v", err) + } +} + +// TestParentEventsDoNotGroundTheSubject is the sharp edge of the boundary: a +// container that never ran, under a workload that deploys. The payload can +// cite the parent's deploy — BuildExplain borrows those on purpose — but the +// citation describes the workload. The subject still has no history, and the +// answer is still 422. +func TestParentEventsDoNotGroundTheSubject(t *testing.T) { + mem := emptyStore(t) + if err := mem.Append(ev(t0.Add(hours(6)), evidence.EventDeploy, evidence.SeverityInfo, + workloadSubject("payments", "api"), map[string]string{"replicas": "12"})); err != nil { + t.Fatalf("Append: %v", err) + } + req := explainRequest(t) + req.Store, req.Rec, req.Verdict = mem, nil, nil + req.Subject = typoSubject() // a container of that same real workload + + ex := mustExplain(t, req) + + if len(ex.Citations) == 0 { + t.Fatal("fixture is wrong: the parent's deploy should have been borrowed") + } + if ex.Grounding == nil || ex.Grounding.ParentEvents != 1 { + t.Fatalf("the borrowed event must be counted separately: %+v", ex.Grounding) + } + if ex.Grounding.Any() { + t.Errorf("a borrowed event must not count as the subject's own record: %+v", ex.Grounding) + } + if got := ex.GroundingState(); got != GroundingAbsent { + t.Fatalf("GroundingState = %q, want %q: the workload's history is not the container's", + got, GroundingAbsent) + } + if ex.GroundingError() == nil { + t.Error("a container with only its parent's events must still be a 422") + } + note := strings.Join(ex.Notes, " ") + if !strings.Contains(note, "no evidence is stored") || !strings.Contains(note, "parent workload") { + t.Errorf("the note must say the citations are borrowed: %v", ex.Notes) + } +} + +// TestGroundingArithmetic pins the boundary itself, independently of any +// store: which counts mean which state. +func TestGroundingArithmetic(t *testing.T) { + cases := []struct { + name string + g Grounding + citations int + want GroundingState + }{ + {"nothing at all", Grounding{}, 0, GroundingAbsent}, + {"borrowed events only", Grounding{ParentEvents: 4}, 4, GroundingAbsent}, + {"one digest", Grounding{Digests: 1}, 0, GroundingThin}, + {"one event", Grounding{Events: 1}, 0, GroundingThin}, + {"one decision", Grounding{Decisions: 1}, 0, GroundingThin}, + {"one usage window", Grounding{UsageWindows: 1}, 0, GroundingThin}, + {"samples without a window count", Grounding{Samples: 7}, 0, GroundingThin}, + {"withheld by a cap", Grounding{Withheld: 2}, 0, GroundingThin}, + {"cited", Grounding{UsageWindows: 1}, 1, GroundingGrounded}, + {"cited over withheld", Grounding{Withheld: 9}, 3, GroundingGrounded}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := tc.g.stateFor(tc.citations); got != tc.want { + t.Errorf("stateFor(%d) on %+v = %q, want %q", tc.citations, tc.g, got, tc.want) + } + }) + } +} + +// TestGroundingStateNeverSilentlyClaimsAbsence: a payload that never computed +// grounding reports unknown, and unknown does not raise the error. Absence is +// a computed fact and only a computed fact may be asserted. +func TestGroundingStateNeverSilentlyClaimsAbsence(t *testing.T) { + handBuilt := &Explanation{Subject: explainSubject(), Action: ActionUnknown} + if got := handBuilt.GroundingState(); got != GroundingUnknown { + t.Errorf("GroundingState = %q, want %q", got, GroundingUnknown) + } + if err := handBuilt.GroundingError(); err != nil { + t.Errorf("a payload that did not compute absence must not assert it: %v", err) + } + cited := &Explanation{Subject: explainSubject(), Citations: []ID{"dig/1/x@1"}} + if got := cited.GroundingState(); got != GroundingGrounded { + t.Errorf("GroundingState = %q, want %q", got, GroundingGrounded) + } +} + +// TestGroundingSurvivesJSON: the state a caller acts on must survive the wire, +// because pkg/api serves this payload and cmd/kilter reads it back. +func TestGroundingSurvivesJSON(t *testing.T) { + req := explainRequest(t) + req.Store, req.Rec, req.Verdict = emptyStore(t), nil, nil + req.Subject = typoSubject() + ex := mustExplain(t, req) + + raw, err := json.Marshal(ex) + if err != nil { + t.Fatalf("marshal: %v", err) + } + var back Explanation + if err := json.Unmarshal(raw, &back); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if got := back.GroundingState(); got != GroundingAbsent { + t.Errorf("decoded GroundingState = %q, want %q", got, GroundingAbsent) + } + if back.GroundingError() == nil { + t.Error("a decoded absent payload must still be an error") + } + if !strings.Contains(string(raw), `"grounding"`) { + t.Error("the absent state must be visible to a JSON consumer, not only to Go") + } +} + +// TestGroundedPayloadIsByteIdenticalToBefore: the report is omitted from a +// grounded payload, which is what keeps the golden fixtures byte-identical +// and every existing consumer unaffected. +func TestGroundedPayloadCarriesNoNewBytes(t *testing.T) { + ex := mustExplain(t, explainRequest(t)) + if ex.Grounding != nil || ex.VerdictOrigin != nil { + t.Fatalf("a grounded, verdict-carrying payload must gain no fields: %+v / %+v", + ex.Grounding, ex.VerdictOrigin) + } + raw, err := json.Marshal(ex) + if err != nil { + t.Fatalf("marshal: %v", err) + } + for _, k := range []string{`"grounding"`, `"verdictOrigin"`} { + if strings.Contains(string(raw), k) { + t.Errorf("%s must be absent from a grounded payload", k) + } + } +} + +// TestAbsentPayloadIsDeterministic: the new fields are computed from counts, +// so they must not introduce map iteration or a clock. +func TestAbsentPayloadIsDeterministic(t *testing.T) { + var want []byte + for i := 0; i < 16; i++ { + req := explainRequest(t) + req.Subject = typoSubject() + got, err := json.Marshal(mustExplain(t, req)) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if i == 0 { + want = got + continue + } + if string(got) != string(want) { + t.Fatalf("run %d differed\n got: %s\nwant: %s", i, got, want) + } + } +} diff --git a/pkg/explain/payload.go b/pkg/explain/payload.go index 8cebcaa..4c89aca 100644 --- a/pkg/explain/payload.go +++ b/pkg/explain/payload.go @@ -108,6 +108,15 @@ type Explanation struct { // dossier the payload was built from hit a cap. Truncated *evidence.Truncation `json:"truncated,omitempty"` Notes []string `json:"notes,omitempty"` + + // VerdictOrigin says which kind of absence Action == unknown is, and is + // present only when the verdict was not computed. See [VerdictState]. + VerdictOrigin *VerdictOrigin `json:"verdictOrigin,omitempty"` + // Grounding is the evidence arithmetic behind the payload, present only + // when the payload is not grounded — i.e. exactly when a caller needs to + // act on it. See [Explanation.GroundingState] and + // [Explanation.GroundingError]. + Grounding *Grounding `json:"grounding,omitempty"` } // ActionUnknown is the Action of an explanation built without a Verdict — @@ -126,8 +135,21 @@ type ExplainRequest struct { From, To time.Time Store evidence.Store - Rec *recommend.Recommendation + Rec *recommend.Recommendation + // Verdict is the decision-quality verdict the operational path reached, + // copied into the payload unchanged. This package never computes one: + // see verdict.go's file comment for why a second evaluation here would + // be a fabricated audit trail. Verdict *decision.Verdict + // RecVerdict is the production recommendation path's readout for this + // subject — one element of recommend.Recommender.Verdicts. It supplies + // the verdict when the path reached one (Decision()'s comma-ok, copied + // verbatim) and the typed "not computed" state when it did not, which is + // every readout the recommender produces today. + // + // Supply this OR Verdict, never both: two sources for one disposition is + // how a payload ends up reporting a verdict nobody reached. + RecVerdict *recommend.Verdict // SavingsMonthlyUSD is priced by the caller (pkg/plan owns that math). // Reported only when SavingsKnown is set — an explicit flag beats a zero @@ -172,6 +194,56 @@ func (r *ExplainRequest) validate() error { if r.SavingsKnown && (math.IsNaN(r.SavingsMonthlyUSD) || math.IsInf(r.SavingsMonthlyUSD, 0)) { return fmt.Errorf("explain: savings %v is not a usable amount", r.SavingsMonthlyUSD) } + return r.validateVerdict() +} + +// validateVerdict enforces that the payload has exactly one source for its +// disposition, and that the source is one a verdict could actually come from. +// Every rule here refuses a fabrication rather than a typo. +func (r *ExplainRequest) validateVerdict() error { + if r.Verdict != nil && r.RecVerdict != nil { + return fmt.Errorf("explain: Verdict and RecVerdict are two sources for one disposition; " + + "supply exactly one, because the payload cannot report both and must not pick") + } + if v := r.Verdict; v != nil { + switch v.Action { + case decision.ActionAct, decision.ActionRecommendOnly, decision.ActionRefuse: + default: + // A zero decision.Verdict has Action "", which used to render as + // a blank verdict — a payload asserting a disposition that is + // not one. decision.Decide cannot produce it, so this is a bug + // at the call site and says so rather than degrading quietly. + return fmt.Errorf("explain: verdict action %q is not one of %q, %q or %q; "+ + "a verdict with no action is not a verdict", + v.Action, decision.ActionAct, decision.ActionRecommendOnly, decision.ActionRefuse) + } + } + rv := r.RecVerdict + if rv == nil { + return nil + } + // The readout names a container. Attributing one container's disposition + // to another subject is the same fabrication as inventing it. + if r.Subject.Kind != evidence.SubjectContainer { + return fmt.Errorf("explain: RecVerdict is a container readout but the subject is %q; "+ + "a workload has no single disposition to report", r.Subject.Kind) + } + if got := rv.Key.String(); got != r.Subject.Key { + return fmt.Errorf("explain: RecVerdict is about %s but the subject is %s; "+ + "a readout may only explain the container it is about", got, r.Subject.Key) + } + if r.Rec == nil { + return nil + } + if rv.Rec == nil { + return fmt.Errorf("explain: RecVerdict reports disposition %q, which carries no recommendation, "+ + "but Rec was supplied; the sizing and the disposition would come from different answers", + rv.Disposition) + } + if *r.Rec != *rv.Rec { + return fmt.Errorf("explain: Rec and RecVerdict.Rec size %s differently; "+ + "one of them is stale and the payload must not choose", rv.Key) + } return nil } @@ -207,7 +279,12 @@ func BuildExplain(req ExplainRequest) (*Explanation, error) { // container template inside it, so a container's case file is missing // exactly the events that explain a post-change refusal. Pull the parent // workload's change events in as well, under their own bound. + // ownEvents is fixed before the parent workload's events are folded in: + // everything after this point can tell the subject's own record from a + // borrowed one, which is what makes "absent" a statement about the + // subject rather than about its workload. See grounding.go. events := dos.Events + ownEvents := len(dos.Events) if parent, ok := parentWorkload(subject); ok && req.MaxEvents > 0 { pe, err := req.Store.Events(parent, req.From, req.To, evidence.EventDeploy, evidence.EventHPAScale, evidence.EventRegimeChange) @@ -228,14 +305,38 @@ func BuildExplain(req ExplainRequest) (*Explanation, error) { Decisions: dos.Decisions, Truncated: dos.Truncated, } - if req.Verdict != nil { - ex.Action = string(req.Verdict.Action) - ex.Refusal = req.Verdict.Refusal - conf := req.Verdict.Confidence + // The verdict. There is exactly one source for it and this package is + // never that source: whichever of the two inputs the caller supplied, the + // decision.Verdict below is COPIED, never derived. Nothing in this + // function calls decision.Evaluate, decision.Decide or decision.Compose. + verdict := req.Verdict + rec := req.Rec + if rv := req.RecVerdict; rv != nil { + if d, ok := rv.Decision(); ok { + // The production path reached a verdict. Report that one. + verdict = &d + } else { + // It did not. "Not computed" is the fact, and it is a different + // fact from a refusal — Action stays unknown, Refusal stays nil, + // and the origin says which of the four branches produced the + // silence. + ex.VerdictOrigin = originOf(rv) + ex.Notes = append(ex.Notes, ex.VerdictOrigin.notComputedNote()) + } + if rec == nil { + // Byte-for-byte the Recommendation the production path served + // for this container, or nil for the three silent dispositions. + rec = rv.Rec + } + } + if verdict != nil { + ex.Action = string(verdict.Action) + ex.Refusal = verdict.Refusal + conf := verdict.Confidence ex.Confidence = &conf } - if req.Rec != nil { - ex.Sizing = sizingOf(req.Rec, req.SavingsMonthlyUSD, req.SavingsKnown) + if rec != nil { + ex.Sizing = sizingOf(rec, req.SavingsMonthlyUSD, req.SavingsKnown) } // Citation pools, computed once and shared by the drivers below. @@ -282,20 +383,20 @@ func BuildExplain(req ExplainRequest) (*Explanation, error) { confidenceCitations(t.Name, digestIDs, changeIDs, decisionIDs)) } } - if req.Rec != nil { - if req.Rec.OOMCount > 0 { + if rec != nil { + if rec.OOMCount > 0 { push(Driver{Kind: DriverOOMFloor, Detail: fmt.Sprintf( "%d OOMKill%s in the window %s the memory request at or above its floor", - req.Rec.OOMCount, pluralS(req.Rec.OOMCount), - pluralVerb(req.Rec.OOMCount, "holds", "hold"))}, oomIDs) + rec.OOMCount, pluralS(rec.OOMCount), + pluralVerb(rec.OOMCount, "holds", "hold"))}, oomIDs) } - if req.Rec.CPUSkipped { + if rec.CPUSkipped { push(Driver{Kind: DriverHPAGuard, Detail: "an HPA scales this workload on CPU, so the CPU request is left alone"}, hpaIDs, decisionIDs) } - if req.Rec.Class != "" { - push(Driver{Kind: DriverClass, Name: string(req.Rec.Class), Detail: fmt.Sprintf( - "behavior class %q selects the sizing policy applied", req.Rec.Class)}, digestIDs) + if rec.Class != "" { + push(Driver{Kind: DriverClass, Name: string(rec.Class), Detail: fmt.Sprintf( + "behavior class %q selects the sizing policy applied", rec.Class)}, digestIDs) } } if dos.Usage.Samples > 0 { @@ -343,12 +444,64 @@ func BuildExplain(req ExplainRequest) (*Explanation, error) { "%d driver%s were computed but dropped for want of a resolvable evidence id; an ungrounded reason is worse than a missing one", ex.Ungrounded, pluralS(ex.Ungrounded))) } - if len(ex.Citations) == 0 { - ex.Notes = append(ex.Notes, "no evidence is stored for this subject in this window; the payload states the decision but grounds none of it") - } + ex.noteGrounding(groundingOf(dos, ownEvents, len(events), len(ex.Citations))) return ex, nil } +// groundingOf counts what the store returned for the subject and resolves the +// state from it. The counts are witnesses, not an inventory: digests appear +// in both Digests and UsageWindows, because the usage summary runs its own +// query across every stored tier and is the one section a caller's caps +// cannot suppress. +func groundingOf(dos *evidence.Dossier, ownEvents, allEvents, citations int) *Grounding { + g := &Grounding{ + Digests: len(dos.Digests), + Events: ownEvents, + Decisions: len(dos.Decisions), + UsageWindows: dos.Usage.Windows, + Samples: dos.Usage.Samples, + ParentEvents: allEvents - ownEvents, + Citations: citations, + } + // A cap that dropped records is proof records exist. Without this the + // caller who asks for MaxEvents < 0 gets told the subject does not exist. + if t := dos.Truncated; t != nil { + g.Withheld = t.Digests + t.Events + t.Decisions + } + g.State = g.stateFor(citations) + return g +} + +// noteGrounding attaches the report and the sentence that goes with it. The +// report is dropped for a grounded payload: Citations already prove it, and +// every field would be redundant with the drivers above. +func (e *Explanation) noteGrounding(g *Grounding) { + switch g.State { + case GroundingGrounded: + return + case GroundingAbsent: + // The leading clause is the same sentence this payload has always + // carried for an empty store, because it is still exactly true. + note := "no evidence is stored for this subject in this window; the payload states the decision but grounds none of it" + if g.Citations > 0 { + // It cites something anyway: the parent workload's change + // events, which describe the workload and not this subject. + note = fmt.Sprintf("no evidence is stored for this subject in this window; "+ + "the %d citation%s below %s the parent workload's change event%s, which describe%s the workload, not this subject", + g.Citations, pluralS(g.Citations), pluralVerb(g.Citations, "is", "are"), + pluralS(g.ParentEvents), pluralVerb(g.ParentEvents, "s", "")) + } + e.Notes = append(e.Notes, note) + case GroundingThin: + e.Notes = append(e.Notes, fmt.Sprintf( + "evidence is stored for this subject in this window (%d digest%s, %d event%s, %d decision%s, %d usage window%s) "+ + "but no driver could be grounded in it; the payload states the decision and cites nothing", + g.Digests, pluralS(g.Digests), g.Events, pluralS(g.Events), + g.Decisions, pluralS(g.Decisions), g.UsageWindows, pluralS(g.UsageWindows))) + } + e.Grounding = g +} + // parentWorkload maps a container subject to the workload subject that owns // it: ContainerKey renders as "Kind/namespace/name/container", so the parent // is the key minus its last segment. @@ -599,6 +752,15 @@ func (e *Explanation) Prose() string { } b.WriteString("\n") case e.Action == ActionUnknown: + // Two absences, two sentences. "None recorded" for a payload nobody + // told anything; "not computed" for one where the engine considered + // the subject and reached no verdict. Neither may read as a refusal. + if o := e.VerdictOrigin; o != nil && o.State == VerdictNotComputed { + fmt.Fprintf(&b, "Verdict: not computed — the recommendation path reported disposition %q "+ + "(%d sample%s over %s). That is an absent verdict, not a negative one.\n", + o.Disposition, o.Samples, pluralS(o.Samples), o.Window) + break + } b.WriteString("Verdict: none recorded.\n") default: fmt.Fprintf(&b, "Verdict: %s", e.Action) diff --git a/pkg/explain/verdict.go b/pkg/explain/verdict.go new file mode 100644 index 0000000..6044111 --- /dev/null +++ b/pkg/explain/verdict.go @@ -0,0 +1,132 @@ +package explain + +import ( + "fmt" + "time" + + "github.com/agenticode/kilter/pkg/decision" + "github.com/agenticode/kilter/pkg/recommend" +) + +// This file plumbs the production path's verdict into the payload, and +// refuses to invent one when the production path did not reach one. +// +// The tempting shortcut is to import pkg/decision here — this file already +// does, for the types — and call decision.Evaluate on evidence assembled on +// the spot. pkg/recommend/FINDINGS.md §6.4-1 refused that and +// pkg/recommend/VERDICT-FINDINGS.md §1 refused it again: a second evaluation +// is a second opinion, and it can differ from the one the operational path +// actually served. An explanation reporting a verdict the engine never +// reached is not a weaker audit trail, it is a fabricated one. +// +// So there is exactly one way a decision.Verdict enters this package: a +// caller hands one over, and it is copied into the payload unchanged. This +// file constructs no Verdict, no Refusal and no Confidence, and calls nothing +// in pkg/decision. What it adds is the third state — the one that was +// previously indistinguishable from the second. + +// VerdictState says whether a decision-quality verdict (pkg/decision) stands +// behind [Explanation.Action]. Three facts, and the payload could previously +// only tell two of them apart: +// +// refused Action == "refuse", Refusal != nil. A verdict exists and a +// refusal predicate fired. Citable, with a code and a detail. +// not-computed Action == "unknown", VerdictOrigin.State == "not-computed". +// The production path considered this subject and took a named +// branch, but evaluated no refusal predicate and chose no +// action, so no verdict exists to report. Refusal is nil. +// unknown Action == "unknown", VerdictOrigin == nil. Nobody said +// anything about this subject — not even whether the engine +// looked at it. +// +// The first is a decision. The second and third are both absences, and they +// are different absences: "the engine considered it and reached no verdict" +// is a fact about the engine, "nothing was supplied" is a fact about the +// caller. Neither is a negative verdict, and rendering either as one would be +// the lie this package exists to prevent. +type VerdictState string + +const ( + // VerdictUnknown: no verdict and no readout was supplied. + VerdictUnknown VerdictState = "unknown" + // VerdictNotComputed: a production readout was supplied and it reports + // that no decision-quality verdict exists. This is what every + // recommend.Verdict says today (pkg/recommend/VERDICT-FINDINGS.md §1). + VerdictNotComputed VerdictState = "not-computed" + // VerdictComputed: a decision.Verdict was reached on the production path + // and this payload reports it verbatim. + VerdictComputed VerdictState = "computed" +) + +// VerdictOrigin records where Action came from when it did not come from a +// verdict — which of the two absences applies, and what the recommendation +// path did instead. +// +// It is absent from a payload whose verdict was computed, because for that +// case Action, Confidence and Refusal already say everything and Action +// itself is the machine-readable proof (only a real verdict can set it to +// act, recommend-only or refuse). Use [Explanation.VerdictState], which +// resolves all three states from whichever fields are present. +type VerdictOrigin struct { + State VerdictState `json:"state"` + // Disposition is the recommendation path's own word for the branch it + // took: recommended, never-observed, insufficient-history or + // no-significant-change. It is NOT a refusal code and must never be + // rendered as one — recommend.DispositionInsufficientHistory and + // decision.CodeInsufficientHistory are two thresholds in two + // independently settable Configs that happen to share their defaults + // (pkg/recommend/VERDICT-FINDINGS.md §1.2). + Disposition string `json:"disposition,omitempty"` + // Samples and Window are the learned history the disposition was + // reached on, straight off the readout. Zero for never-observed. + Samples int `json:"samples"` + Window time.Duration `json:"window"` +} + +// originOf projects a production readout into the payload's own vocabulary. +// It copies; it derives nothing. +func originOf(rv *recommend.Verdict) *VerdictOrigin { + return &VerdictOrigin{ + State: VerdictNotComputed, + Disposition: string(rv.Disposition), + Samples: rv.Samples, + Window: rv.Window, + } +} + +// notComputedNote is the sentence that keeps "no verdict" from reading as +// "a negative verdict" in a payload a human or a narrating model will read. +func (o *VerdictOrigin) notComputedNote() string { + return fmt.Sprintf("no decision verdict was computed on the production path; "+ + "the recommendation path's disposition for this subject was %q (%d sample%s over %s). "+ + "An absent verdict is not a refusal.", + o.Disposition, o.Samples, pluralS(o.Samples), o.Window) +} + +// VerdictState reports which of the three states this payload is in. It +// never returns "". +// +// The computed case is derived from Action rather than stored, so a payload +// built before this field existed — or decoded from one — still answers +// correctly: only a supplied decision.Verdict can make Action anything other +// than [ActionUnknown], and BuildExplain rejects a verdict whose Action is +// not one of pkg/decision's three. +func (e *Explanation) VerdictState() VerdictState { + if e.VerdictOrigin != nil && e.VerdictOrigin.State != "" { + return e.VerdictOrigin.State + } + switch decision.Action(e.Action) { + case decision.ActionAct, decision.ActionRecommendOnly, decision.ActionRefuse: + return VerdictComputed + } + return VerdictUnknown +} + +// Refused reports a payload carrying an actual refusal — a verdict was +// computed and a refusal predicate fired. It is false for both absences, and +// that is the distinction the type exists to make: a caller asking "did the +// engine say no?" must not get "yes" from a subject the engine never judged. +func (e *Explanation) Refused() bool { + return e.VerdictState() == VerdictComputed && + decision.Action(e.Action) == decision.ActionRefuse +} diff --git a/pkg/explain/verdict_test.go b/pkg/explain/verdict_test.go new file mode 100644 index 0000000..3d0a78f --- /dev/null +++ b/pkg/explain/verdict_test.go @@ -0,0 +1,527 @@ +package explain + +import ( + "encoding/json" + "go/ast" + "go/parser" + "go/token" + "io/fs" + "reflect" + "strings" + "testing" + "time" + + "github.com/agenticode/kilter/pkg/decision" + "github.com/agenticode/kilter/pkg/model" + "github.com/agenticode/kilter/pkg/patterns" + "github.com/agenticode/kilter/pkg/recommend" +) + +// readoutWire mirrors recommend.Verdict's wire shape. It exists because the +// decision verdict inside a recommend.Verdict is unexported and reachable +// only through Decision()'s comma-ok — deliberately, so nothing can +// manufacture one — and UnmarshalJSON is the seam's own supported way to +// produce a computed readout. Nothing in this package needs it; the tests +// need it to exercise the branch production will take once +// pkg/recommend/VERDICT-FINDINGS.md §7 lands. +type readoutWire struct { + Key model.ContainerKey `json:"key"` + Disposition recommend.Disposition `json:"disposition"` + CurrentRequest model.Resources `json:"currentRequest"` + CurrentLimit model.Resources `json:"currentLimit"` + Samples int `json:"samples"` + Window time.Duration `json:"window"` + Rec *recommend.Recommendation `json:"recommendation,omitempty"` + VerdictState recommend.VerdictState `json:"verdictState"` + Decision *decision.Verdict `json:"verdict,omitempty"` +} + +func decodeReadout(t *testing.T, w readoutWire) recommend.Verdict { + t.Helper() + raw, err := json.Marshal(w) + if err != nil { + t.Fatalf("marshal readout: %v", err) + } + var rv recommend.Verdict + if err := json.Unmarshal(raw, &rv); err != nil { + t.Fatalf("unmarshal readout: %v", err) + } + return rv +} + +// notComputedReadout is what recommend.Verdicts returns for every container +// today: a real disposition, and no decision verdict at all. +func notComputedReadout() recommend.Verdict { + return recommend.Verdict{ + Key: explainKey, + Disposition: recommend.DispositionInsufficientHistory, + Samples: 12, + Window: hours(3), + } +} + +// TestReadoutWithoutAVerdictIsNotComputedNotRefused is the whole point of the +// bridge. The production path reached a branch; it did not reach a judgement. +// The payload must be able to say the first without implying the second. +func TestReadoutWithoutAVerdictIsNotComputedNotRefused(t *testing.T) { + rv := notComputedReadout() + if _, ok := rv.Decision(); ok { + t.Fatal("fixture is wrong: this readout must carry no decision verdict") + } + req := explainRequest(t) + req.Verdict, req.Rec = nil, nil + req.RecVerdict = &rv + + ex := mustExplain(t, req) + + if got := ex.VerdictState(); got != VerdictNotComputed { + t.Fatalf("VerdictState = %q, want %q", got, VerdictNotComputed) + } + if ex.Action != ActionUnknown { + t.Errorf("Action = %q, want %q: an uncomputed verdict has no action", ex.Action, ActionUnknown) + } + if ex.Refusal != nil { + t.Errorf("Refusal = %+v, want nil: a disposition is not a refusal", ex.Refusal) + } + if ex.Refused() { + t.Error("Refused() must be false: the engine reached no judgement to refuse with") + } + if ex.Confidence != nil { + t.Errorf("Confidence = %+v, want nil: no verdict, no scored basis", ex.Confidence) + } + if ex.VerdictOrigin == nil || ex.VerdictOrigin.Disposition != string(recommend.DispositionInsufficientHistory) { + t.Fatalf("the disposition must be reported: %+v", ex.VerdictOrigin) + } + if ex.VerdictOrigin.Samples != 12 || ex.VerdictOrigin.Window != hours(3) { + t.Errorf("the history the disposition was reached on must come through: %+v", ex.VerdictOrigin) + } + note := strings.Join(ex.Notes, " ") + if !strings.Contains(note, "no decision verdict was computed") || + !strings.Contains(note, "An absent verdict is not a refusal") { + t.Errorf("the note must state the absence and disclaim the refusal: %v", ex.Notes) + } + prose := ex.Prose() + if !strings.Contains(prose, "Verdict: not computed") || + !strings.Contains(prose, `"insufficient-history"`) { + t.Errorf("prose must name the branch production took:\n%s", prose) + } + if strings.Contains(prose, "Verdict: refuse") { + t.Fatalf("prose reads as a refusal:\n%s", prose) + } +} + +// TestNoDispositionIsEverRenderedAsARefusalCode walks all four dispositions. +// insufficient-history the disposition and CodeInsufficientHistory the +// refusal share a name and a default threshold and are not the same fact +// (pkg/recommend/VERDICT-FINDINGS.md §1.2). +func TestNoDispositionIsEverRenderedAsARefusalCode(t *testing.T) { + for _, d := range []recommend.Disposition{ + recommend.DispositionRecommended, + recommend.DispositionNeverObserved, + recommend.DispositionInsufficientHistory, + recommend.DispositionNoSignificantChange, + } { + t.Run(string(d), func(t *testing.T) { + rv := notComputedReadout() + rv.Disposition = d + if d == recommend.DispositionRecommended { + rv.Rec = explainRec() + } + req := explainRequest(t) + req.Verdict, req.Rec = nil, nil + req.RecVerdict = &rv + + ex := mustExplain(t, req) + + if ex.Refusal != nil || ex.Refused() { + t.Fatalf("disposition %q produced a refusal: %+v", d, ex.Refusal) + } + if ex.Action != ActionUnknown { + t.Errorf("disposition %q set Action to %q", d, ex.Action) + } + for _, drv := range ex.Drivers { + if drv.Kind == DriverRefusal { + t.Errorf("disposition %q produced a refusal driver: %+v", d, drv) + } + } + raw, err := json.Marshal(ex) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if strings.Contains(string(raw), `"refusal"`) { + t.Errorf("disposition %q put a refusal on the wire: %s", d, raw) + } + }) + } +} + +// TestReadoutSuppliesTheServedRecommendation: when the disposition is +// "recommended" the readout carries the very Recommendation production +// served, so the caller does not have to fetch it a second way and risk a +// different answer. +func TestReadoutSuppliesTheServedRecommendation(t *testing.T) { + rv := notComputedReadout() + rv.Disposition = recommend.DispositionRecommended + rv.Rec = explainRec() + + req := explainRequest(t) + req.Verdict, req.Rec = nil, nil + req.RecVerdict = &rv + + ex := mustExplain(t, req) + + if ex.Sizing == nil { + t.Fatal("the readout's recommendation must become the payload's sizing") + } + if ex.Sizing.Container != explainKey.Container || ex.Sizing.Reason != rv.Rec.Reason { + t.Errorf("sizing does not match the served recommendation: %+v", ex.Sizing) + } + if ex.Sizing.TargetRequest != rv.Rec.TargetRequest { + t.Errorf("target request = %+v, want %+v", ex.Sizing.TargetRequest, rv.Rec.TargetRequest) + } +} + +// TestComputedVerdictIsCopiedNotRecomputed is the trap, pinned. +// +// The readout carries a verdict of "act". The evidence in the store, fed to +// decision.Evaluate as a caller would assemble it, refuses — there is a +// deploy inside the soak window. If this package ever re-evaluated instead of +// reporting, the payload would say "refuse" and this test would say so. +func TestComputedVerdictIsCopiedNotRecomputed(t *testing.T) { + served := decision.Verdict{ + Action: decision.ActionAct, + Confidence: decision.Compose( + decision.TermHistoryDepth(120, 200), + decision.TermWindowSpan(hours(30), hours(48)), + ), + } + // A second opinion over the same subject's evidence, computed the way a + // caller tempted to "fill in the gap" would compute it. + secondOpinion := decision.Evaluate(decision.Evidence{ + Samples: 120, + Window: hours(30), + LastSample: t0.Add(hours(30)), + Class: patterns.ClassSteady, + LastChange: t0.Add(hours(28)), // the deploy the fixture store records + }, decision.DefaultConfig(), t0.Add(hours(30))) + if secondOpinion == nil { + t.Fatal("fixture is wrong: the second opinion must disagree for this test to mean anything") + } + if secondOpinion.Code != decision.CodePostChangeSoak { + t.Logf("second opinion refuses with %q", secondOpinion.Code) + } + + rv := decodeReadout(t, readoutWire{ + Key: explainKey, Disposition: recommend.DispositionRecommended, + Samples: 120, Window: hours(30), Rec: explainRec(), + VerdictState: recommend.VerdictComputed, Decision: &served, + }) + source, ok := rv.Decision() + if !ok { + t.Fatal("fixture is wrong: this readout must carry a computed verdict") + } + + req := explainRequest(t) + req.Verdict, req.Rec = nil, nil + req.RecVerdict = &rv + + ex := mustExplain(t, req) + + if got := ex.VerdictState(); got != VerdictComputed { + t.Fatalf("VerdictState = %q, want %q", got, VerdictComputed) + } + if ex.Action != string(decision.ActionAct) { + t.Fatalf("Action = %q, want %q — the payload re-derived a disposition instead of reporting one", + ex.Action, decision.ActionAct) + } + if ex.Refusal != nil { + t.Fatalf("Refusal = %+v: this is the second opinion leaking into the payload", ex.Refusal) + } + // Byte-for-byte the verdict the readout carried, not one shaped like it. + if ex.Confidence == nil || !reflect.DeepEqual(*ex.Confidence, source.Confidence) { + t.Errorf("confidence = %+v, want the readout's %+v", ex.Confidence, source.Confidence) + } + if ex.VerdictOrigin != nil { + t.Errorf("a computed verdict needs no origin, Action proves it: %+v", ex.VerdictOrigin) + } + if ex.Refused() { + t.Error("Refused() must be false for an act verdict") + } +} + +// TestTheThreeStatesAreDistinguishable is the type-level claim: refused, not +// computed and unknown differ in fields a consumer can switch on, in Go and +// on the wire. +func TestTheThreeStatesAreDistinguishable(t *testing.T) { + refusalVerdict := decision.Verdict{ + Action: decision.ActionRefuse, + Confidence: decision.Compose(decision.TermHistoryDepth(120, 200)), + Refusal: &decision.Refusal{ + Code: decision.CodePostChangeSoak, + Detail: "deploy 6h ago; steady soak is 6h", + Until: t0.Add(hours(30)), + }, + } + rv := notComputedReadout() + + cases := []struct { + name string + mut func(*ExplainRequest) + wantState VerdictState + wantAct string + refused bool + refusal bool + origin bool + }{ + {"refused", func(r *ExplainRequest) { r.Verdict = &refusalVerdict }, + VerdictComputed, string(decision.ActionRefuse), true, true, false}, + {"not computed", func(r *ExplainRequest) { r.Verdict, r.RecVerdict = nil, &rv }, + VerdictNotComputed, ActionUnknown, false, false, true}, + {"unknown", func(r *ExplainRequest) { r.Verdict, r.RecVerdict = nil, nil }, + VerdictUnknown, ActionUnknown, false, false, false}, + } + seen := map[string]string{} + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + req := explainRequest(t) + req.Rec = nil + tc.mut(&req) + ex := mustExplain(t, req) + + if got := ex.VerdictState(); got != tc.wantState { + t.Errorf("VerdictState = %q, want %q", got, tc.wantState) + } + if ex.Action != tc.wantAct { + t.Errorf("Action = %q, want %q", ex.Action, tc.wantAct) + } + if ex.Refused() != tc.refused { + t.Errorf("Refused() = %v, want %v", ex.Refused(), tc.refused) + } + if (ex.Refusal != nil) != tc.refusal { + t.Errorf("Refusal present = %v, want %v", ex.Refusal != nil, tc.refusal) + } + if (ex.VerdictOrigin != nil) != tc.origin { + t.Errorf("VerdictOrigin present = %v, want %v", ex.VerdictOrigin != nil, tc.origin) + } + // The prose a human reads must differ too: three states that + // render identically are three states nobody can act on. + line := verdictLine(ex.Prose()) + if prev, dup := seen[line]; dup { + t.Errorf("state %q renders exactly like %q: %q", tc.name, prev, line) + } + seen[line] = tc.name + }) + } + if len(seen) != 3 { + t.Errorf("the three states produced %d distinct verdict lines", len(seen)) + } +} + +// verdictLine extracts the "Verdict: ..." line from a prose rendering. +func verdictLine(prose string) string { + for _, l := range strings.Split(prose, "\n") { + if strings.HasPrefix(l, "Verdict:") { + return l + } + } + return "" +} + +// TestVerdictStateSurvivesJSON: a consumer reading the payload back must +// still be able to tell the two absences apart. +func TestVerdictStateSurvivesJSON(t *testing.T) { + rv := notComputedReadout() + req := explainRequest(t) + req.Verdict, req.Rec = nil, nil + req.RecVerdict = &rv + + raw, err := json.Marshal(mustExplain(t, req)) + if err != nil { + t.Fatalf("marshal: %v", err) + } + var back Explanation + if err := json.Unmarshal(raw, &back); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if got := back.VerdictState(); got != VerdictNotComputed { + t.Errorf("decoded VerdictState = %q, want %q", got, VerdictNotComputed) + } + if back.Refused() { + t.Error("a decoded not-computed payload must not read as refused") + } + if !strings.Contains(string(raw), `"state": "not-computed"`) && + !strings.Contains(string(raw), `"state":"not-computed"`) { + t.Errorf("the not-computed state must be explicit on the wire: %s", raw) + } +} + +// TestTwoVerdictSourcesAreRefused: a payload with two sources for one +// disposition would have to pick, and picking silently is how a payload ends +// up reporting a verdict nobody reached. +func TestTwoVerdictSourcesAreRefused(t *testing.T) { + rv := notComputedReadout() + req := explainRequest(t) + req.RecVerdict = &rv // explainRequest already sets Verdict + _, err := BuildExplain(req) + if err == nil { + t.Fatal("supplying both a Verdict and a RecVerdict must fail") + } + if !strings.Contains(err.Error(), "two sources for one disposition") { + t.Errorf("error %q does not say why", err) + } +} + +// TestVerdictWithoutAnActionIsRejected: the zero decision.Verdict used to +// render as a blank verdict — a payload asserting a disposition that is not +// one of the three. +func TestVerdictWithoutAnActionIsRejected(t *testing.T) { + req := explainRequest(t) + req.Verdict = &decision.Verdict{} + _, err := BuildExplain(req) + if err == nil { + t.Fatal("a verdict with no action must be rejected, not rendered blank") + } + if !strings.Contains(err.Error(), "is not a verdict") { + t.Errorf("error %q does not say why", err) + } + req.Verdict = &decision.Verdict{Action: decision.Action("maybe")} + if _, err := BuildExplain(req); err == nil { + t.Fatal("an action outside pkg/decision's three must be rejected") + } +} + +// TestReadoutMustBeAboutTheSubject: attributing one container's disposition +// to another subject is the same fabrication as inventing it. +func TestReadoutMustBeAboutTheSubject(t *testing.T) { + other := notComputedReadout() + other.Key = containerKey("payments", "api", "sidecar") + + req := explainRequest(t) + req.Verdict, req.Rec = nil, nil + req.RecVerdict = &other + _, err := BuildExplain(req) + if err == nil { + t.Fatal("a readout about another container must be rejected") + } + if !strings.Contains(err.Error(), "may only explain the container it is about") { + t.Errorf("error %q does not say why", err) + } + + rv := notComputedReadout() + req = explainRequest(t) + req.Verdict, req.Rec = nil, nil + req.RecVerdict = &rv + req.Subject = workloadSubject("payments", "api") + if _, err := BuildExplain(req); err == nil { + t.Fatal("a container readout must not be attached to a workload subject") + } +} + +// TestRecAndReadoutMustAgree: the sizing and the disposition must come from +// the same answer. +func TestRecAndReadoutMustAgree(t *testing.T) { + t.Run("silent disposition with a Rec", func(t *testing.T) { + rv := notComputedReadout() // insufficient-history: Rec is nil + req := explainRequest(t) + req.Verdict = nil + req.RecVerdict = &rv // Rec is still explainRec() + _, err := BuildExplain(req) + if err == nil { + t.Fatal("a silent disposition alongside a recommendation must be rejected") + } + if !strings.Contains(err.Error(), "different answers") { + t.Errorf("error %q does not say why", err) + } + }) + t.Run("two different sizings", func(t *testing.T) { + rv := notComputedReadout() + rv.Disposition = recommend.DispositionRecommended + served := explainRec() + rv.Rec = served + stale := explainRec() + stale.TargetRequest.MilliCPU = 999 + + req := explainRequest(t) + req.Verdict = nil + req.Rec = stale + req.RecVerdict = &rv + _, err := BuildExplain(req) + if err == nil { + t.Fatal("two disagreeing sizings must be rejected, not silently reconciled") + } + if !strings.Contains(err.Error(), "differently") { + t.Errorf("error %q does not say why", err) + } + }) + t.Run("agreeing sizings pass", func(t *testing.T) { + rv := notComputedReadout() + rv.Disposition = recommend.DispositionRecommended + rv.Rec = explainRec() + req := explainRequest(t) + req.Verdict = nil + req.Rec = explainRec() + req.RecVerdict = &rv + if _, err := BuildExplain(req); err != nil { + t.Fatalf("identical sizings must be accepted: %v", err) + } + }) +} + +// TestNoSecondEvaluationInThisPackage is the trap as a structural check +// rather than a promise. pkg/explain may name pkg/decision's types; it may +// not call its computations. A new call fails this test until someone +// justifies it, which is the point — pkg/recommend/FINDINGS.md §6.4-1 refused +// a second evaluation site and this keeps the refusal enforced from the other +// side of the seam. +func TestNoSecondEvaluationInThisPackage(t *testing.T) { + // Type conversions read as calls in the AST. Only these are allowed. + allowed := map[string]map[string]bool{ + "decision": {"Action": true, "RefusalCode": true}, + "recommend": {}, + } + fset := token.NewFileSet() + pkgs, err := parser.ParseDir(fset, ".", func(fi fs.FileInfo) bool { + return !strings.HasSuffix(fi.Name(), "_test.go") + }, parser.SkipObjectResolution) + if err != nil { + t.Fatalf("parse package: %v", err) + } + var checked, found int + for _, pkg := range pkgs { + for name, file := range pkg.Files { + checked++ + ast.Inspect(file, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + ident, ok := sel.X.(*ast.Ident) + if !ok { + return true + } + ok2, watched := allowed[ident.Name] + if !watched { + return true + } + found++ + if !ok2[sel.Sel.Name] { + t.Errorf("%s: %s.%s is a computation in another package's plane; "+ + "an explanation reports the answer the engine reached, it does not reach one", + fset.Position(call.Pos()), ident.Name, sel.Sel.Name) + } + return true + }) + _ = name + } + } + if checked == 0 { + t.Fatal("the scan read no files, so it proves nothing") + } + if found == 0 { + t.Fatal("the scan found no decision/recommend selector at all; it is not looking where it thinks") + } +}