diff --git a/CHANGELOG.md b/CHANGELOG.md index 51d5914..bb14d3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -85,6 +85,10 @@ D-113 immutability holds — only the algorithm computing the pin moved, version - :memo: docs(adr): record the AUD-S12 malformed-marker behaviour change in ADR-0019 (review F3) - :memo: docs(adr): correct the convergence mechanism for a skipped bot marker (review F8) - :memo: docs(planning): mark E10 design-note steps 1-2 shipped by AUD-S15 +- :memo: docs: renumber this lane's decision row D-129 -> D-131 (D-129/D-130 claimed by the containment lane) +- :memo: docs(engine): the text-order refusal sentence reaches no adopter surface +- :memo: docs: record the review close-out for the three non-blocking notes in D-131 +- :memo: docs(engine): narrow the bytes-binding claim to what was verified ### Features - :sparkles: feat(cli): dispatch-table help listing the real subcommands (REQ-AUD-S05-01) @@ -101,6 +105,8 @@ D-113 immutability holds — only the algorithm computing the pin moved, version - :bug: fix(release): stop stripping the changelog header from the GitHub Release body - :bug: fix(forge): treat an over-limit body as deterministic, not retryable (AUD-S10 x S11) - :bug: fix(forge): carry reconcile warnings on refusal paths too (review F1) +- :bug: fix(engine): relational compare over string-bound operands must fail safe, not lexically +- :bug: fix(engine): ordering a bytes operand is text ordering too — refuse it (D-131) ### Other - :construction_worker: ci(lint): depguard deny-rules for the D-123 pure tree (REQ-AUD-S07-01) @@ -157,6 +163,7 @@ D-113 immutability holds — only the algorithm computing the pin moved, version - :white_check_mark: test(cmd): pin policySha to raw policy bytes (D-121 byte-vs-document split) - :white_check_mark: test(lint): close the aliased-import evasion in the ARCH-02 gate - :white_check_mark: fix(test): make the exec-timeout tests deterministic under load +- :white_check_mark: test(engine): pin how an unrepresentable numeric renders in {{ }} ## [0.1.0] - 2026-08-05 ### Chores diff --git a/docs/adr/0013-assert-syntax-and-backend.md b/docs/adr/0013-assert-syntax-and-backend.md index 39da6b4..8e2bdd2 100644 --- a/docs/adr/0013-assert-syntax-and-backend.md +++ b/docs/adr/0013-assert-syntax-and-backend.md @@ -105,6 +105,38 @@ judgment (subjective); dependency scores are measured. spec with multiple implementations. Swapping *away from CEL* would break authored policies — that part of the decision is effectively one-way once packs exist. +## Amendment 1 (2026-08-08, D-131): ordering operators refuse text operands + +Residual code risk (1) above — "numeric type coercion YAML/HCL→CEL … highest risk" — turned out +to have a fail-open in it. CEL defines `<`, `<=`, `>`, `>=` over **strings** as a lexical +compare, so an ordering leaf silently answers a boolean when its operands bind as text rather +than numbers, and lexically `"6" >= "12"` is **true**: a quoted `partitions: "12"` → `"6"` +shrink evaluated `new >= old` to true, proved `non-destructive`, and reached **APPROVE**. The +converse is equally wrong — a legitimate grow `"6"` → `"12"` evaluated false and **BLOCKed**. + +**Amendment: in tier-1 `assert`, an ordering operator whose operand actually evaluates to text — +a CEL `string` or `bytes` value — is an evaluation ERROR, not an answer** (fail-safe direction, +GUIDELINES §2: the error routes to `predicate.error` → REVIEW). The check is on the value at +evaluation time, not on the leaf's syntax — whether `new >= old` is sound depends on the +adopter's data, so no static check of the policy can decide it. + +Consequences for authors: + +- Ordering **quoted** numerics means coercing first: `int(new) >= int(old)` (already the idiom + in this repo's tests), or `double(...)`. Comparing ISO-8601 dates means `timestamp(a) < timestamp(b)`. +- **Ordering raw text is no longer expressible in tier-1** and graduates to the Rego escape + hatch (ADR-0002) — consistent with this ADR's design taste: don't grow a programming language + in YAML. There is no exempt spelling: `string(a) < string(b)` and the byte-wise + `bytes(a) < bytes(b)` are the same lexical sort as `a < b` and are refused with it. No policy + in the corpus, the comparison suite, or either dogfood pack ordered text. +- The refusal set is exactly CEL's two text-shaped types, `string` and `bytes`. Every other + `<`-comparable type in the frozen predicate scope — `int`, `uint`, `double`, `bool`, + `duration`, `timestamp` — is a genuine ordering and is untouched; `list`, `map`, `type` and + mixed numeric pairs have no `<` overload to begin with. That census is what makes the claim + above true rather than merely intended, and a test pins it against env drift. +- Equality (`==`/`!=`), membership (`in`), and the string member functions (`startsWith`, + `contains`, `matches`, `size`) are exact rather than ordering and are untouched. + ## Counterpoints considered - *"This isn't real Kyverno syntax, so D-006 is betrayed."* — Strongest objection. Answer: diff --git a/docs/decisions/decisions.md b/docs/decisions/decisions.md index 94a44fa..bf40fb2 100644 --- a/docs/decisions/decisions.md +++ b/docs/decisions/decisions.md @@ -130,3 +130,4 @@ project/process decisions. | D-123 | 2026-08-06 | **ARCH-01 — boundary enforcement automated (depguard + extended purity walk); ADR-0011 Amendment 3 truths the "arch-lint enforced" claim.** Two layers: (1) golangci `depguard` deny-rules — `internal/core/**`, `internal/change/**`, `internal/glob`, `internal/lint`, `internal/catalogue`, `internal/evaldecode`, `internal/compare`, `schemas/**` may import none of `internal/forge/**`, `internal/render/**`, `cmd/**`, `net/**`; (2) `TestCorePurity` walk extends to `../evaldecode`, `../compare`, and `../../schemas` (call-level: `time.Now`/`os.Getenv`/`os.Environ`/rand/net, adversarial self-test retained). Scope note: this EXTENDS the AGENTS.md rule-7 pure tree — `internal/evaldecode` (engine input decode) and `internal/compare` (D-116/D-117 gate determinism) join the determinism guard; `schemas` is embedded compile-time authority. Acceptance: a synthetic violating import/call fails CI both ways. Revert: drop depguard rules + walk dirs and re-amend ADR-0011. | | D-124 | 2026-08-07 | **AUD-S06 residual — the two docs truth-lag gates exist but are UNWIRED; wiring is assigned to Lane B.** `hack/docs/readme_smoke_test.sh` (REQ-AUD-S06-01, executes every README quick-start command) and `hack/docs/truthlag_pins_test.sh` (REQ-AUD-S06-02, 18 grep/diff pins over DOC-05/06/09/10/11 + the ADR status index) are green and mutation-proven, but nothing invokes them: `Taskfile.yml` and `.github/workflows/**` belong to **Lane B** (AUD-S02/S03/S09/S14), so AUD-S06 could not add them. **Consequence, stated plainly: until they are wired, a future README or docs edit reopens DOC-07 (a quick-start that exits 2) or any pinned claim with NOTHING going red** — the mechanism is a manual gate, which is the same class of defect AUD-S06 exists to close. Assignment: Lane B adds a `docs-gates` task (`bash hack/docs/readme_smoke_test.sh && bash hack/docs/truthlag_pins_test.sh`) as a `check:` dependency, following the `hack/compare/exitgate_test.sh` precedent (D-118), alongside its AUD-S09/S14 workflow work. **Extend when wiring** (known pin gaps, both accepted for now): (a) the DOC-09 walkthrough check asserts banner PRESENCE, not polarity — flipping a step's `Planned` to `Shipped` stays green; (b) the DOC-05 link-resolution loop is scoped to `README.md`, so the relative links in `examples/README.md` are unpinned; (c) AUD-S05's `TestNoStaleProductClaims` (`cmd/assent/main_help_test.go`, Lane A5's file) walks only `cmd/`, `internal/` and `docs/` — markdown under `hack/`, `.github/` and `test/` is grepped by no pin at all. **Known unfixed truth-lag, deliberately not corrected in AUD-S06 to keep the reviewed diff narrow — fix when next editing these files:** (i) `docs/planning/meta-plan.md` closes the Phase-5 epic table with "Ordering constraint: E7 starts early (alongside E1)", directly under the new heading asserting the table is the numbering that actually executed — E7 in fact landed after E6 (E6 tip `ec91226` is an ancestor of E7-S08 `f27457d`, both 2026-08-04); (ii) `docs/usage/install.md` credits a stamped version to "the Homebrew **bottle**", but `.goreleaser.yaml`'s `brews:` block publishes a **Formula** whose `url_template` points at the release archive — no bottle is built or hosted; the version claim is true, only the term is wrong. Revert: delete the two scripts and reopen DOC-05/06/07/09/10/11 as live findings. | | D-125 | 2026-08-07 | **AUD-S02 judgment call (b) — the CHANGELOG drift gate runs in `task check` on every local commit and in CI on push-to-main + schedule, NOT on `pull_request`.** The spec asked for both placements; the PR placement is not merely noisy, it is red by construction. Evidence (reproduced locally, not reasoned): `hack/release/verify-changelog.sh` diffs the WHOLE generated changelog against the committed file, and `cliff.toml`'s parser list ends in a catch-all `{ message = ".*", group = "Other" }`, so merge commits are rendered — merging a probe branch put `- Merge 1234567 into 89abcde` in the generated output. On `pull_request`, `actions/checkout` checks out `refs/pull/N/merge`: a merge commit minted at CI time whose subject is exactly that shape. No committed `CHANGELOG.md` can contain a line naming a SHA that did not exist when it was written, so a PR-scoped step fails on every PR with no author fix (and would push commit SHAs into the changelog, contra **D-101**). Walk-back taken under the spec's decide-and-log sanction, using the `release-exitgate` precedent already in the file: `if: github.event_name != 'pull_request'`. **Consequence, stated plainly:** PR CI does not catch changelog drift. The `check:` entry does — one commit later, by construction: `task check` is green at HEAD, the next commit makes `CHANGELOG.md` stale, and the following `task check` is red until `task changelog-write` is committed. Push-to-main is the backstop, so a lane that lands without regenerating reds main until a regeneration commit follows. **Working rule:** a regeneration commit must be subject-prefixed `:memo: chore(release):` or `:wrench: chore(release):` — the two forms `cliff.toml` skips — or it creates fresh drift itself; and a lane must regenerate AFTER its last content commit and after any `git merge origin/main`. Two companion changes ship with it: version headings render Keep-a-Changelog style (`## [0.1.0] - 2026-08-05`, matching the hand-written `[0.0.0]` stub), and the **D-120** `pins.toolDigest` record-consumer warning lives in `cliff.toml`'s `[changelog] header` — CHANGELOG.md is regenerated in full, so a hand-edit there would be wiped by the next `changelog-write` with the drift gate still green. **Closes the D-124 assignment** in the same `check:` list: `docs-gates` (`hack/docs/readme_smoke_test.sh` + `truthlag_pins_test.sh`) and `lint-depguard-test` (`hack/lint/depguard_test.sh`) are now sequential `check:` commands — sequential, not `deps:`, because go-task runs deps in parallel and the smoke test builds a binary while `fmt` rewrites the tree. The depguard proof is local-only: CI lints via `golangci-lint-action`, which leaves no binary on PATH for a later step, and that gate refuses to skip when `golangci-lint` is absent. D-124's known pin gaps (a)/(b)/(c) and truth-lag items (i)/(ii) are NOT addressed here and remain open. Revert: drop the `check:` entries and the verify.yaml step — reopens RELSE-01 and D-124. | +| D-131 | 2026-08-08 | **Engine P1 fail-open — an ordering operator over a TEXT operand is an evaluation error, not a lexical answer (ADR-0013 Amendment 1).** CEL defines `<` `<=` `>` `>=` over strings as a lexical compare, so a `when: new >= old` leaf answered a boolean whenever its operands bound as text — and both answers are wrong. Reproduced end-to-end through the production entry point (`change.Diff` → `evaldecode.DecodeCanonical` → `aggregate.Cover`) on a routine adopter shape, a QUOTED numeric: `partitions: "12"` → `"6"` is a `!!str` the differ tag-discriminates and the decoder keeps a Go string BY DESIGN, so the D-016 `partitions-must-not-shrink` rule evaluated the lexical `"6" >= "12"` = **true**, proved `non-destructive`, fired nothing, and returned **APPROVE with zero findings** — a BLOCK→APPROVE flip. The mirror case is equally wrong: a legitimate grow `"6"` → `"12"` evaluated false and BLOCKed. Second, narrower instance: a numeric literal representable as neither int64 nor float64 fell back to its STRING form in `toCEL`, so `9e399 > 1e400` was lexically true (numerically false). **Fix, at the `evalLeaf`/`toCEL` seam only:** (1) `evalLeaf` plants a watcher (a cel-go `CustomDecorator`) on every operand of every relational operator; if an operand ACTUALLY evaluates to text — a CEL `string` OR `bytes` value — the leaf returns an error → `predicate.error` → REVIEW. Value-based, not syntax-based, and deliberately so: whether `new >= old` is sound depends on the adopter's DATA, not the policy text, so **lint cannot catch this class** (`checkLeafScope`/`checkPredicateScope`/`checkFactsShape` were checked; none could) — the information exists only at evaluation. Watching (not reading post-eval `EvalState`) is what makes EVERY comprehension iteration visible, since state keeps one value per node id. Short-circuited compares never run their watcher, so a policy cannot flip to REVIEW for a compare that did not happen. The guard is applied to **both** evaluation seams: `evalLeaf` (the E2 production path) and `evalRule` (the walking-skeleton `Aggregate` path, exported and test-only today, whose env declares `old`/`new` as `StringType` and binds the RAW canonical strings — so every bare relational there was lexical by construction, with only convention mandating `int()`). One unguarded evaluator is how this class returns; same drift argument as D-055(c). Its cost-budget test doubles as the interaction proof that cel-go still charges a decorated operand (`costBombWhen` is the left operand of `> 0`; cel-go v0.30.0). (2) `toCEL` binds an unrepresentable numeric literal as a CEL error value instead of its string form; an over-int64 but float64-representable literal still binds lossily — ADR-0013 residual #1's lossy half is UNCHANGED and still live. **Behaviour change, stated plainly: a policy that previously evaluated (wrongly) now ERRORS.** Any leaf that ordered text — in either direction, at any decision — becomes `predicate.error` → REVIEW. Nothing in `examples/`, the comparison corpus, either dogfood pack or the test corpus ordered text (all relational leaves are numeric: partitions, memory_mb, replicas, `size()`), so **no golden, fixture or gate output changed**; the only changed test is `internal/evaldecode`'s mutation proof `TestStringOldNewFailsOpen`, renamed `TestUndecodedStringOldNewFailsSafe` — it asserted APPROVE to document the fail-open the decoder closes, and now asserts the discriminating pair (decoded → BLOCK + `partition-count-shrunk`; un-decoded → REVIEW + `predicate.error`), so neither layer can rot silently. **Consequence for authors:** ordering quoted numerics means coercing first — `int(new) >= int(old)` (already the repo idiom) or `double(...)`; dates use `timestamp(...)`. **Ordering raw text is no longer expressible in tier-1 `assert`, in ANY spelling** — `string(a) < string(b)` and the byte-wise `bytes(a) < bytes(b)` are the same lexical sort and are refused with the bare form — and graduates to Rego (ADR-0002 escape hatch), consistent with ADR-0013's "don't grow a programming language in YAML". Equality, `in` and the string functions are untouched. Scope call: the guard is on `evalLeaf` only, NOT on `EvalScalar` — `{{ }}` message interpolation renders text and decides nothing, so guarding it would widen the blast radius for no safety gain. Records: **no `openspec/` change entry** — `openspec/changes/` holds only its README (this repo has never used change proposals in practice), and a fail-safe correction of an existing frozen semantic is proportionate to a D-row + ADR amendment + failing-test-first. Revert: drop `textOrderGuard` and restore `toCEL`'s `x.String()` fallback — reopens the APPROVE flip. **Review close-out (three non-blocking notes, decided by the author):** (F-1) the first cut watched only `types.String`, so `bytes(a) < bytes(b)` — the IDENTICAL byte-wise compare — slipped through and `bytes(new) >= bytes(old)` over the quoted `"12"` → `"6"` shrink still answered `true`. Not accident-reachable (the defect class is a quoted numeric in adopter YAML, not a deliberate `bytes()` call), so the choice was real: exempt it as a sanctioned explicit escape hatch, or refuse it. **Refused** — the lane had already rejected exempting `string(...)` because it "re-admits the identical defect", and that reasoning applies verbatim; a documented exemption would also advertise a one-token workaround for the guard's own error message, converting "not accident-reachable" into "the documented way round the guard". The refusal set is now `string` + `bytes`, and it is provably COMPLETE: the `_<_` overloads reachable from `newEvalEnv` were enumerated empirically — `int`/`uint`/`double`/`bool`/`duration`/`timestamp` are genuine orderings and pass through, `list`/`map`/`type`/mixed-numeric have no overload at all — so ADR-0013 Amendment 1's universal claim is now true as written rather than merely intended. `TestOnlyTextShapedOperandsAreRefused` pins all three buckets and reds if the env gains a comparable type (e.g. `CrossTypeNumericComparisons`). (F-2) the guard's refusal sentence was commented "adopter-facing"; it is not — both consumers use the error only for its non-nil-ness (`coverSubject` renders the AUTHORED leaf message, `evalRule`'s caller renders none), so no surface prints it today. Comment corrected to say what the consumers do; the sentence stays plain-worded so it is fit to surface if an engine error channel is ever added. (F-3) `{{ old }}`/`{{ new }}` over an unrepresentable literal interpolate the CEL error VALUE, rendering toCEL's sentence where a number belongs. Left as-is deliberately and GOLDENED: it is authored prose, not a Go-internals leak (GUIDELINES §Contracts), it decides nothing (the leaf already errored → REVIEW), it says WHY the rule could not decide where a bare `{{ old }}` placeholder would not, and re-opening this row's explicit "guard `evalLeaf`, not `EvalScalar`" scope call for a cosmetic P3 buys no safety. The golden also pins the DIVERGENCE between the two interpolators (`expandMessage` renders the sentence; `internal/render`'s `EvalScalar` path rejects the error value), so a future unification cannot change either silently. | diff --git a/docs/planning/predicate-scope.md b/docs/planning/predicate-scope.md index d13b39b..cceaa68 100644 --- a/docs/planning/predicate-scope.md +++ b/docs/planning/predicate-scope.md @@ -35,6 +35,14 @@ admission object). - `facts..` and `mr.` are the only two fields with a further-nested, provider/forge-defined shape; every other field's shape is fixed by the schemas in this epic (`EntryRef`, `Change`, the four matcher-domain shapes). +- The ordering operators `<`, `<=`, `>`, `>=` compare **numbers**, not text. If an operand + actually evaluates to a string — a YAML `!!str` such as `partitions: "12"` stays a string by + design — the leaf is an evaluation error (→ `predicate.error` → REVIEW), never a lexical + answer (ADR-0013 Amendment 1, D-131). The same holds for a `bytes` operand: `bytes(a) < bytes(b)` + is the identical byte-wise sort and is refused too, as is `string(a) < string(b)` — there is no + exempt spelling. Order quoted numerics with `int(new) >= int(old)` or `double(...)`, and dates + with `timestamp(a) < timestamp(b)`; `duration`, `bool`, `uint` and the other numeric orderings + are unaffected. Equality, `in`, and the string functions are unaffected. - Adding a field to this table requires a schema-fixture change (a new positive fixture that exercises it) — this table and `merge-policy.schema.json`'s `assert`/`cel` `description` stay in lockstep by construction, not by convention. diff --git a/internal/core/aggregate/aggregate.go b/internal/core/aggregate/aggregate.go index d862913..cd88ad8 100644 --- a/internal/core/aggregate/aggregate.go +++ b/internal/core/aggregate/aggregate.go @@ -20,15 +20,20 @@ // // CEL numeric coercion (constraint c). change.Change.Old/New are the differ's // CANONICAL, TAG-DISCRIMINATING STRINGS ("12" for int 12, "\"12\"" for the -// string "12", "016" kept literal). They are bound to CEL as raw string values; -// the CEL EXPRESSION's own int()/double() conversions do the coercion. A -// non-numeric or lossy input makes int()/double() ERROR (empirically verified in -// cel-go v0.29.2: the error surfaces via BOTH the Eval error slot AND a -// types.Err result value — this package checks both), which the tri-state routes -// to REVIEW. We deliberately do NOT strconv-coerce Go-side with a 0/false/"" -// default: that would fail OPEN to APPROVE on a parse failure. A lexical string -// compare is likewise wrong ("9" > "12" is true lexically), so numeric rules MUST -// use int()/double() in the `when` expression; the differ doc mandates this. +// string "12", "016" kept literal). internal/evaldecode INVERTS that render +// before the engine sees it, so a numeric literal arrives as a json.Number and +// toCEL binds it as int64/float64 — `new >= old` is a NUMERIC compare and needs +// no int() in the expression. A non-numeric input to an explicit int()/double() +// still ERRORS (empirically verified in cel-go: the error surfaces via BOTH the +// Eval error slot AND a types.Err result value — this package checks both), which +// the tri-state routes to REVIEW. We deliberately do NOT strconv-coerce Go-side +// with a 0/false/"" default: that would fail OPEN to APPROVE on a parse failure. +// A value that is GENUINELY text (a YAML !!str, e.g. `partitions: "12"`) still +// binds as a string, and a lexical compare over it is wrong in both directions +// ("6" >= "12" is lexically true, "12" >= "6" lexically false) — so since D-131 +// evalLeaf's textOrderGuard makes an ordering operator over a text operand an +// EVALUATION ERROR (-> predicate.error -> REVIEW), never an answer. Ordering +// quoted numerics deliberately means coercing first: int(new) >= int(old). // // Change-ness signal (constraint d). The PRESENCE of an entry in the ChangeSet is // the "this field changed" signal. Old==New string-equal can still be a real @@ -426,17 +431,27 @@ func bindActivation(cs change.ChangeSet) map[string]any { } // evalRule compiles and evaluates one `when` expression. It returns (satisfied, -// nil) ONLY when the predicate compiled, evaluated without error, and produced a -// boolean. Every other outcome — a compile error (undecidable `when`), an eval -// error (incl. numeric-coercion failure, surfaced via the error slot OR a -// types.Err value), or a non-boolean result — returns a non-nil error so the -// caller fails safe to REVIEW. It NEVER returns (true, nil) for a malformed rule. +// nil) ONLY when the predicate compiled, evaluated without error, produced a +// boolean, and ordered nothing lexically. Every other outcome — a compile error +// (undecidable `when`), an eval error (incl. numeric-coercion failure, surfaced +// via the error slot OR a types.Err value), a non-boolean result, or an ordering +// operator over a text operand — returns a non-nil error so the caller fails safe +// to REVIEW. It NEVER returns (true, nil) for a malformed rule. +// +// The D-131 textOrderGuard is applied here too, not only in evalLeaf. This +// walking-skeleton env declares old/new as StringType and binds the differ's RAW +// canonical strings, so EVERY bare relational here is a lexical compare — the +// path mandated int()/double() by convention alone, with nothing enforcing it. +// Guarding both evaluators is deliberate: one evaluation seam left unguarded is +// how this class of fail-open comes back (the same drift argument that pulled the +// canonical decoder into internal/evaldecode, D-055c). func evalRule(env *cel.Env, activation map[string]any, when string) (bool, error) { ast, iss := env.Compile(when) if iss != nil && iss.Err() != nil { return false, fmt.Errorf("compile when %q: %w", when, iss.Err()) } - prg, err := env.Program(ast, cel.CostLimit(celCostBudget)) + guard := newTextOrderGuard(ast) + prg, err := env.Program(ast, cel.CostLimit(celCostBudget), cel.CustomDecorator(guard.decorate)) if err != nil { return false, fmt.Errorf("program when %q: %w", when, err) } @@ -449,6 +464,9 @@ func evalRule(env *cel.Env, activation map[string]any, when string) (bool, error if out == nil || types.IsError(out) { return false, fmt.Errorf("eval when %q produced an error value", when) } + if err := guard.err(); err != nil { + return false, fmt.Errorf("eval when %q: %w", when, err) + } b, ok := out.Value().(bool) if !ok { // A non-boolean `when` is malformed; it must NOT be read as true/false. diff --git a/internal/core/aggregate/asserttree.go b/internal/core/aggregate/asserttree.go index a058f23..bded3ad 100644 --- a/internal/core/aggregate/asserttree.go +++ b/internal/core/aggregate/asserttree.go @@ -173,6 +173,14 @@ var tmplPlaceholder = regexp.MustCompile(`\{\{[^{}]*\}\}`) // that does not resolve to a scalar is left LITERAL (never a panic, never a silent // blank) — the authoring-time in-scope check is E3's lint, not this expander's job. // A message with no placeholder is returned unchanged (fast path). +// +// One binding is neither a scalar nor a container: since D-131 toCEL binds a numeric literal +// beyond the representable range as a CEL error VALUE, and being printable it substitutes as +// its own refusal sentence (toCEL owns the wording) where the number would have gone. +// Deliberate, and pinned by test: the leaf already errored, so the finding is +// a predicate.error REVIEW either way, and the sentence at least says why the rule could not +// decide — more than a bare `{{ old }}` left literal would. internal/render's separate +// EvalScalar-based interpolator diverges here and rejects the error value instead. func expandMessage(msg string, in EvaluationInput, ch EvalChange, envLabel string) string { if !strings.Contains(msg, "{{") { return msg diff --git a/internal/core/aggregate/evalrule_cost_test.go b/internal/core/aggregate/evalrule_cost_test.go index ac97830..1b2f977 100644 --- a/internal/core/aggregate/evalrule_cost_test.go +++ b/internal/core/aggregate/evalrule_cost_test.go @@ -13,6 +13,11 @@ const costBombWhen = `[1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22, // TestEvalRuleEnforcesCostBudget — REL-02 / AUD-02: production evalRule must apply // the same celCostBudget as evalLeaf/evalscalar/message-template paths. +// +// It doubles as the D-131 interaction proof: costBombWhen's expensive node is the +// LEFT OPERAND of `> 0`, so it is exactly the kind of node textOrderGuard wraps +// (cel.CustomDecorator). This asserts the cost observer still charges a wrapped +// operand — keep the `> 0` shape if this expression is ever rewritten. func TestEvalRuleEnforcesCostBudget(t *testing.T) { env, err := newEvalEnv() if err != nil { diff --git a/internal/core/aggregate/evaluate.go b/internal/core/aggregate/evaluate.go index 8ebab7f..34bd278 100644 --- a/internal/core/aggregate/evaluate.go +++ b/internal/core/aggregate/evaluate.go @@ -5,7 +5,11 @@ import ( "fmt" "github.com/google/cel-go/cel" + "github.com/google/cel-go/common/ast" + "github.com/google/cel-go/common/operators" "github.com/google/cel-go/common/types" + "github.com/google/cel-go/common/types/ref" + "github.com/google/cel-go/interpreter" ) // celCostBudget bounds a single leaf evaluation (ADR-0013: predicates are @@ -41,16 +45,21 @@ func newEvalEnv() (*cel.Env, error) { // activation built for the handed change (E2-S02). It does NO matching/selection // — the caller (a test here, E2-S04's coverage loop in production) hands it the // change. It returns (satisfied, nil) ONLY when the expression compiled, -// evaluated under the cost budget without error, and produced a boolean; every -// other outcome (undeclared reference, coercion/type error, cost overrun, -// non-boolean result) returns a non-nil error so the caller fails safe. It NEVER +// evaluated under the cost budget without error, produced a boolean, AND ordered +// nothing lexically (textOrderGuard, D-131); every other outcome (undeclared +// reference, coercion/type error, cost overrun, non-boolean result, a relational +// compare over text) returns a non-nil error so the caller fails safe. It NEVER // returns (true, nil) for a malformed or type-erroring predicate. func evalLeaf(env *cel.Env, in EvaluationInput, ch EvalChange, envLabel, expr string) (bool, error) { - ast, iss := env.Compile(expr) + checked, iss := env.Compile(expr) if iss != nil && iss.Err() != nil { return false, fmt.Errorf("compile when %q: %w", expr, iss.Err()) } - prg, err := env.Program(ast, cel.CostLimit(celCostBudget)) + // The guard watches every operand of every relational operator AS IT IS + // EVALUATED (D-131) — see newTextOrderGuard. It is per-program state, and this + // program is built and used exactly once, here. + guard := newTextOrderGuard(checked) + prg, err := env.Program(checked, cel.CostLimit(celCostBudget), cel.CustomDecorator(guard.decorate)) if err != nil { return false, fmt.Errorf("program when %q: %w", expr, err) } @@ -61,6 +70,12 @@ func evalLeaf(env *cel.Env, in EvaluationInput, ch EvalChange, envLabel, expr st if out == nil || types.IsError(out) { return false, fmt.Errorf("eval when %q produced an error value", expr) } + // BEFORE the boolean is trusted: if anything was ordered lexically the answer + // is unsound in BOTH directions (lexically "6" >= "12" is true and "12" >= "6" + // is false). Fail safe on either. + if err := guard.err(); err != nil { + return false, fmt.Errorf("eval when %q: %w", expr, err) + } b, ok := out.Value().(bool) if !ok { return false, fmt.Errorf("when %q result is %s, not bool", expr, out.Type().TypeName()) @@ -68,6 +83,166 @@ func evalLeaf(env *cel.Env, in EvaluationInput, ch EvalChange, envLabel, expr st return b, nil } +// textOrderGuard is the D-131 fail-safe guard. CEL defines < <= > >= over strings +// as a LEXICAL (character-by-character) compare, so an ordering rule such as the +// D-016 `new >= old` silently answers a BOOLEAN when its operands bind as text +// instead of numbers — and lexically "6" >= "12" is TRUE, which APPROVEs a +// partition shrink. Text binds routinely: internal/change tag-discriminates a +// YAML !!str, so `partitions: "12"` decodes to the Go string "12" BY DESIGN +// (evaldecode.DecodeCanonical) — a quoted numeric is not the number 12. +// +// The guard is VALUE-based, not syntax-based, because whether an operand is text +// is a property of the adopter's DATA, not of the policy text: `new >= old` is +// correct over numbers and unsound over strings, and no static check of the leaf +// can tell the two apart. (That is why this defect is not lintable, D-131: lint +// sees the rule, never the change it will judge.) It plants a watcher +// on each operand of each relational operator and inspects what that operand +// ACTUALLY produced, every time it was evaluated: +// +// - a relational compare that short-circuited away never runs its watcher, so +// an existing policy cannot flip to REVIEW for a compare that did not happen; +// - EVERY iteration of a comprehension body is seen (watching beats reading the +// post-eval EvalState, which keeps only the last value per node id); +// - ANY text operand is refused, in either position and for either answer — a +// wrong `false` (a legitimate grow judged destructive) is as unacceptable as +// a wrong `true`. A mixed text/number relational already errors inside cel-go +// (no such overload), so it never reaches here. +// +// "Text" means BOTH text-shaped CEL types, `string` and `bytes`: the two are the +// same character/byte-wise sort, so an exemption for either re-admits the +// identical defect through a tier-1 form (`string(a) < string(b)` and +// `bytes(a) < bytes(b)` are refused for the same reason `a < b` is). That pair is +// the complete refusal set, and the completeness is what makes the ADR's claim +// true: every other `<`-comparable type reachable from newEvalEnv — int, uint, +// double, bool, duration, timestamp — is a genuine ordering and passes through, +// and list/map/type/cross-type-numeric have no `<` overload at all. The census is +// pinned by TestOnlyTextShapedOperandsAreRefused, which reds if the env ever +// gains a new comparable type. +// +// Deliberate ordering stays expressible by coercing first — `int(new) >= int(old)` +// (already the repo's idiom), `double(...)`, or `timestamp(a) < timestamp(b)` for +// ISO-8601 dates. Ordering raw text is NOT expressible in tier-1 `assert` and +// graduates to Rego (ADR-0013 Amendment 1). +// +// Purity/determinism: watching only reads values that were computed anyway; it +// adds no clock, randomness or I/O, and the recorded hit is the FIRST in +// evaluation order, so the error is identical on every replay of the same input. +type textOrderGuard struct { + // operands maps a relational operand's AST node id to the operator symbol it + // feeds, e.g. `new`'s id in `new >= old` -> ">=". + operands map[int64]string + hitOp string + hitValue string + hit bool +} + +// relationalOperators are the four CEL operators whose (string, string) and +// (bytes, bytes) overloads compare LEXICALLY — the only standard operators that +// silently turn a text value into an ordering answer. Equality (== / !=), +// membership (in) and the string member functions are exact, not ordering, and +// are deliberately excluded. +var relationalOperators = map[string]string{ + operators.Less: "<", + operators.LessEquals: "<=", + operators.Greater: ">", + operators.GreaterEquals: ">=", +} + +// newTextOrderGuard collects the operand node ids of every relational call in the +// checked expression. A guard with no operands decorates nothing. +func newTextOrderGuard(checked *cel.Ast) *textOrderGuard { + g := &textOrderGuard{operands: map[int64]string{}} + if checked == nil { + return g + } + calls := ast.MatchDescendants(ast.NavigateAST(checked.NativeRep()), func(e ast.NavigableExpr) bool { + if e.Kind() != ast.CallKind { + return false + } + _, ok := relationalOperators[e.AsCall().FunctionName()] + return ok + }) + for _, call := range calls { + op := relationalOperators[call.AsCall().FunctionName()] + for _, arg := range call.AsCall().Args() { + g.operands[arg.ID()] = op + } + } + return g +} + +// decorate wraps the planned program step for a relational operand so the value +// it yields is inspected. Only those operands are wrapped: their sole consumer is +// the binary relational call itself, which needs nothing beyond Interpretable, so +// wrapping cannot disturb attribute/qualifier planning elsewhere in the program. +func (g *textOrderGuard) decorate(i interpreter.Interpretable) (interpreter.Interpretable, error) { + if op, ok := g.operands[i.ID()]; ok { + return &textOrderWatch{Interpretable: i, guard: g, op: op}, nil + } + return i, nil +} + +// record keeps the FIRST text operand seen (deterministic: evaluation order is +// fixed for a given expression and input). +func (g *textOrderGuard) record(op, value string) { + if g.hit { + return + } + g.hit, g.hitOp, g.hitValue = true, op, value +} + +// err reports the refusal when the evaluation ordered text. +// +// Where this sentence goes today: NOWHERE an adopter can read it. Both consumers +// of the returned error use it for its non-nil-ness only — coverSubject maps it +// to a `predicate.error` require-review Finding whose Message is the AUTHORED +// leaf message (walkAssertTree returns the two separately), and the evalRule +// caller in aggregate.go builds its Finding with no Message at all. So this is an +// engine-internal diagnostic that no rendered finding, comment or `assent test` +// output prints. It is nonetheless written in plain adopter language, and names +// the concrete fix, so that it is fit to surface unchanged if an engine debug or +// explain channel is ever wired (ADR-0012's `debug:` section is authored per-rule +// today and carries no engine error text). +func (g *textOrderGuard) err() error { + if !g.hit { + return nil + } + return fmt.Errorf( + "the %s comparison received the text value %q; assent will not order text, because ordering text sorts it character by character (which puts \"6\" after \"12\"). If that value is a number quoted in the YAML, unquote it or compare with int(...) or double(...); compare dates with timestamp(...)", + g.hitOp, g.hitValue) +} + +// textOrderWatch is a pass-through program step that reports a text result to its +// guard. It never alters the value or the control flow. +type textOrderWatch struct { + interpreter.Interpretable + guard *textOrderGuard + op string +} + +func (w *textOrderWatch) Eval(activation interpreter.Activation) ref.Val { + v := w.Interpretable.Eval(activation) + switch t := v.(type) { + case types.String: + w.guard.record(w.op, string(t)) + case types.Bytes: + // bytes is the same text ordering wearing a different type: CEL compares + // bytes byte-by-byte, so `bytes(new) >= bytes(old)` over the quoted shrink + // 12 -> 6 answers TRUE for exactly the reason `new >= old` did. In practice + // a bytes value here came from an explicit bytes(...) call or a b"..." + // literal — i.e. from text: the decoder feeding old/new emits only strings, + // bools, nil and json.Number, and no policy in the corpus or either dogfood + // pack binds bytes. That is an observation, not a guarantee — toCEL passes + // an unrecognized Go value through untouched, so a provider-supplied + // Fact.Value could in principle arrive as []byte. Refusing to ORDER it + // either way is the fail-safe direction, and exempting bytes would leave a + // tier-1 form that orders text, which is precisely what ADR-0013 + // Amendment 1 says is no longer expressible. + w.guard.record(w.op, string(t)) + } + return v +} + // entryOr returns the reconstructed entry object when one is present, else the // scalar fallback (ch.New/ch.Old). A nil entry is the current, all-callers state // and yields the exact pre-S02 scalar binding — an absent/unreconstructable @@ -179,6 +354,15 @@ func mrToCEL(mr MR) map[string]any { // native Go types cel-go adapts: an integral json.Number -> int64 (injective, no // float64 collapse — mirroring internal/change's numeric discipline), a decimal // -> float64, maps/slices recursively, everything else passed through. +// +// A numeric literal that fits NEITHER int64 nor float64 (|v| beyond ~1.8e308) +// binds a CEL ERROR value, not its string form (D-131). The string form was a +// silent demotion to text: `new > old` over 9e399 and 1e400 became the lexical +// "9e399" > "1e400" = TRUE, the numerically wrong answer, with no error. An error +// value propagates through every operator that touches it -> the caller's +// fail-safe path. Only the unrepresentable extreme is refused: an over-int64 +// literal that IS float64-representable still binds (lossily) as before — the +// documented ADR-0013 residual #1, unchanged. func toCEL(v any) any { switch x := v.(type) { case json.Number: @@ -188,7 +372,7 @@ func toCEL(v any) any { if f, err := x.Float64(); err == nil { return f } - return x.String() + return types.NewErr("the number %s is too large for assent to compare (it exceeds the representable numeric range)", x.String()) case map[string]any: m := make(map[string]any, len(x)) for k, val := range x { diff --git a/internal/core/aggregate/relational_string_test.go b/internal/core/aggregate/relational_string_test.go new file mode 100644 index 0000000..ccf524f --- /dev/null +++ b/internal/core/aggregate/relational_string_test.go @@ -0,0 +1,451 @@ +package aggregate + +// relational_string_test.go is the regression suite for the D-131 fail-open: +// CEL's relational operators (< <= > >=) are DEFINED over strings and compare +// them LEXICALLY, so a change value that binds as a Go string (a YAML !!str the +// decoder deliberately keeps a string, or a numeric literal too large to +// represent) turned an ordering rule into a silently wrong boolean instead of an +// error. Both polarities are wrong: lexically "6" >= "12" is TRUE (a shrink +// APPROVEs) and "12" >= "6" is FALSE (a grow BLOCKs). +// +// The tests below pin BOTH directions of the fix: the string-operand compares +// must now ERROR (-> predicate.error -> REVIEW, the fail-safe path), and every +// legitimate compare — numeric, explicitly coerced, equality, string membership, +// short-circuited-away — must still evaluate exactly as before. + +import ( + "encoding/json" + "testing" + + "github.com/PlatformRelay/assent/internal/core/policy" +) + +// shrinkRulePolicy is the D-016 `partitions-must-not-shrink` shape: a +// valueChanges /partitions modify rule proving `non-destructive` with the BARE +// `new >= old` (no int() coercion), blocking on failure. The binding requires +// only that obligation, so the decision is driven solely by the compare. +func shrinkRulePolicy() (*policy.MergePolicy, *policy.Binding) { + mp := &policy.MergePolicy{ + Spec: policy.MergePolicySpec{ + Rules: []policy.Rule{{ + Name: "partitions-must-not-shrink", + Phase: policy.PhaseEnforce, + Match: policy.Match{ValueChanges: &policy.ValueChangesMatch{Pointers: []string{"/partitions"}, Kinds: []string{"modify"}}}, + Prove: &policy.Prove{Obligation: "non-destructive", When: policy.AssertTree{Leaf: &policy.Leaf{CEL: "new >= old"}}}, + OnFailure: &policy.OnFailure{Effect: policy.EffectBlock, Code: "partition-count-shrunk"}, + Points: 10, + }}, + }, + } + bind := &policy.Binding{Class: "topic-registry", Environment: "prod", Risk: policy.Risk{Threshold: 1}, Require: []string{"non-destructive"}} + return mp, bind +} + +func stringScalarInput(oldVal, newVal string) *EvaluationInput { + return &EvaluationInput{ + ChangeSet: ChangeSet{Changes: []EvalChange{{ + Subject: "topic-registry:orders.events.v1", + File: "topics/prod/orders-events.yaml", + Path: "/partitions", + Kind: "modify", + Old: oldVal, + New: newVal, + }}}, + Facts: map[string]map[string]Fact{}, + Require: []string{"non-destructive"}, + } +} + +func onlyPredicateError(t *testing.T, res Result) { + t.Helper() + if res.Decision != DecisionReview { + t.Fatalf("decision = %q, want REVIEW (a relational compare over text must fail SAFE)", res.Decision) + } + if len(res.Findings) != 1 { + t.Fatalf("findings = %+v, want exactly one predicate.error", res.Findings) + } + if f := res.Findings[0]; f.Code != "predicate.error" || f.Effect != EffectRequireReview { + t.Fatalf("finding = %+v, want code=predicate.error effect=require-review", f) + } +} + +// TestQuotedNumericShrinkMustNotApproveThroughCover is THE reproduction, driven +// through the production entry point aggregate.Cover (not evalLeaf): a +// `partitions: "12"` -> `"6"` change — quoted in the adopter's YAML, so the +// decoder KEEPS it a Go string by design — used to make `new >= old` the lexical +// "6" >= "12" = TRUE, proving `non-destructive`, firing nothing, and returning +// APPROVE with zero findings. A BLOCK -> APPROVE flip. It must now fail safe. +func TestQuotedNumericShrinkMustNotApproveThroughCover(t *testing.T) { + mp, bind := shrinkRulePolicy() + res, err := Cover(mp, bind, stringScalarInput("12", "6")) + if err != nil { + t.Fatalf("Cover: %v", err) + } + if res.Decision == DecisionApprove { + t.Fatalf("decision = APPROVE with findings %+v — the lexical fail-open is OPEN: \"6\" >= \"12\" is lexically true", res.Findings) + } + onlyPredicateError(t, res) +} + +// TestQuotedNumericGrowMustNotBlockLexically is the OTHER polarity of the same +// defect and proves the guard is not merely "erroring in the APPROVE direction": +// `partitions: "6"` -> `"12"` is a legitimate GROW, but lexically "12" >= "6" is +// FALSE, so the rule fired and BLOCKed a change that satisfies the policy. A +// wrong answer either way — the compare must error, never decide. +func TestQuotedNumericGrowMustNotBlockLexically(t *testing.T) { + mp, bind := shrinkRulePolicy() + res, err := Cover(mp, bind, stringScalarInput("6", "12")) + if err != nil { + t.Fatalf("Cover: %v", err) + } + if res.Decision == DecisionBlock { + t.Fatalf("decision = BLOCK with findings %+v — a lexical compare judged a legitimate grow 6->12 destructive", res.Findings) + } + onlyPredicateError(t, res) +} + +// TestRelationalOverStringOperandsErrors pins the seam itself: EVERY relational +// operator over two string-bound operands errors, in both argument orders, and +// never returns a boolean. +func TestRelationalOverStringOperandsErrors(t *testing.T) { + env, err := newEvalEnv() + if err != nil { + t.Fatalf("newEvalEnv: %v", err) + } + in := *stringScalarInput("12", "6") + ch := in.ChangeSet.Changes[0] + for _, expr := range []string{ + "new >= old", "new > old", "new <= old", "new < old", + "old >= new", "old > new", "old <= new", "old < new", + `new >= "12"`, `"12" <= new`, // a string LITERAL operand is no exemption + "entry >= oldEntry", // the scalar-fallback entry binding + "string(new) >= string(old)", // an explicit coercion TO string is still text ordering + "mr.author < path", // two unrelated string-typed scope fields + // bytes() is the SAME byte-wise lexical compare wearing a different type: + // `bytes("6") >= bytes("12")` is true for exactly the reason `"6" >= "12"` + // is. Exempting it would re-admit the identical defect through a tier-1 + // form — the same reasoning that refuses the string(...) coercion above. + "bytes(new) >= bytes(old)", + "bytes(new) < bytes(old)", + `bytes(new) >= b"12"`, // a bytes LITERAL operand is no exemption either + `b"6" >= bytes(old)`, + "dyn(new) >= dyn(old)", // dyn() does not launder a text operand + } { + got, err := evalLeaf(env, in, ch, "prod", expr) + if err == nil { + t.Errorf("evalLeaf(%q) = (%v, nil) — a relational compare over text must ERROR, never answer", expr, got) + } + } +} + +// TestLegitimateComparesStillEvaluate is the required opposite polarity: a fix +// that errors on everything is not a fix. Numeric ordering, explicit coercion +// (the repo's `int(new)` idiom — the supported way to order quoted numerics), +// equality, string predicates and membership must all still evaluate cleanly. +func TestLegitimateComparesStillEvaluate(t *testing.T) { + env, err := newEvalEnv() + if err != nil { + t.Fatalf("newEvalEnv: %v", err) + } + numeric := EvaluationInput{ChangeSet: ChangeSet{Changes: []EvalChange{{ + Subject: "s:1", File: "f.yaml", Path: "/partitions", Kind: "modify", + Old: json.Number("12"), New: json.Number("6"), + }}}, Facts: map[string]map[string]Fact{}, MR: MR{Author: "alice"}} + quoted := *stringScalarInput("12", "6") + + cases := []struct { + in EvaluationInput + expr string + want bool + }{ + {numeric, "new >= old", false}, + {numeric, "new < old", true}, + {numeric, "new >= 6", true}, + {numeric, "new <= 5.5", false}, + // The supported way to order a QUOTED numeric: coerce, then compare. + {quoted, "int(new) >= int(old)", false}, + {quoted, "int(new) < int(old)", true}, + {quoted, "double(new) < double(old)", true}, + // Equality, string predicates and membership are untouched by the guard. + {quoted, `new == "6"`, true}, + {quoted, `new != old`, true}, + {quoted, `new.startsWith("6")`, true}, + {quoted, `path == "/partitions"`, true}, + {quoted, `kind in ["modify", "add"]`, true}, + {quoted, `size(new) < 3`, true}, // size() is an int — an int compare, not text + } + for _, tc := range cases { + ch := tc.in.ChangeSet.Changes[0] + got, err := evalLeaf(env, tc.in, ch, "prod", tc.expr) + if err != nil { + t.Errorf("evalLeaf(%q) errored: %v — a legitimate compare must still evaluate", tc.expr, err) + continue + } + if got != tc.want { + t.Errorf("evalLeaf(%q) = %v, want %v", tc.expr, got, tc.want) + } + } +} + +// TestOnlyTextShapedOperandsAreRefused is the COMPLETENESS proof behind ADR-0013 +// Amendment 1's universal claim that "ordering raw text is no longer expressible +// in tier-1". That claim is only true if the guard covers EVERY `<`-comparable +// type in the frozen predicate scope whose compare is a character/byte-wise sort +// — so this test enumerates the whole overload surface of `_<_` reachable from +// newEvalEnv and asserts each type lands in the right bucket: +// +// - text-shaped (string, bytes): REFUSED. bytes is not a theoretical case — it +// is the one form that used to slip through, because `bytes(a) < bytes(b)` +// performs the identical byte-wise compare as `a < b` (D-131 follow-up). +// - genuine orderings (int, uint, double, bool, duration, timestamp): still +// evaluate. A guard that errors on everything is not a guard, and +// `timestamp(a) < timestamp(b)` is the ADR's own stated migration path for +// ISO-8601 dates — it must keep working. +// - no `<` overload at all (list, map, type, cross-type numeric): a COMPILE +// error, so they need no guard. +// +// Mutations that red it: widening textOrderWatch to a legitimate type (add +// types.Timestamp or types.Int) reds the second bucket; narrowing it back to +// string only reds the first; and enabling cel.CrossTypeNumericComparisons or a +// list/sort extension in newEvalEnv reds the third — which is exactly the drift +// that would silently invalidate the ADR's census. +func TestOnlyTextShapedOperandsAreRefused(t *testing.T) { + env, err := newEvalEnv() + if err != nil { + t.Fatalf("newEvalEnv: %v", err) + } + in := *stringScalarInput("12", "6") + ch := in.ChangeSet.Changes[0] + + for _, expr := range []string{ + `"a" < "b"`, + `bytes("a") < bytes("b")`, + `b"a" < b"b"`, + } { + if got, err := evalLeaf(env, in, ch, "prod", expr); err == nil { + t.Errorf("evalLeaf(%q) = (%v, nil) — a text-shaped ordering must ERROR", expr, got) + } + } + + for _, tc := range []struct { + expr string + want bool + }{ + {`1 < 2`, true}, + {`uint(2) < uint(1)`, false}, + {`1.5 < 2.5`, true}, + {`false < true`, true}, + {`duration("1s") < duration("2s")`, true}, + // The ADR's stated migration path for ISO-8601 dates. + {`timestamp("2026-01-01T00:00:00Z") < timestamp("2026-02-01T00:00:00Z")`, true}, + {`timestamp("2026-02-01T00:00:00Z") < timestamp("2026-01-01T00:00:00Z")`, false}, + } { + got, err := evalLeaf(env, in, ch, "prod", tc.expr) + if err != nil { + t.Errorf("evalLeaf(%q) errored: %v — a genuine (non-text) ordering must still evaluate", tc.expr, err) + continue + } + if got != tc.want { + t.Errorf("evalLeaf(%q) = %v, want %v", tc.expr, got, tc.want) + } + } + + for _, expr := range []string{ + `[1] < [2]`, + `{1:1} < {2:2}`, + `type(1) < type(2)`, + `1 < 2.5`, + } { + if got, err := evalLeaf(env, in, ch, "prod", expr); err == nil { + t.Errorf("evalLeaf(%q) = (%v, nil) — this type pair had NO `<` overload when the census was taken; the guard's completeness claim must be re-derived", expr, got) + } + } +} + +// TestShortCircuitedStringRelationalStillEvaluates proves the guard inspects what +// was ACTUALLY evaluated, not what the expression mentions: a string relational +// behind a false conjunct never runs, so the leaf stays a clean false. Erroring +// here would flip working policies to REVIEW for a compare that never happened. +func TestShortCircuitedStringRelationalStillEvaluates(t *testing.T) { + env, err := newEvalEnv() + if err != nil { + t.Fatalf("newEvalEnv: %v", err) + } + in := *stringScalarInput("12", "6") + ch := in.ChangeSet.Changes[0] + got, err := evalLeaf(env, in, ch, "prod", `kind == "add" && new >= old`) + if err != nil { + t.Fatalf("evalLeaf on a short-circuited string relational errored: %v", err) + } + if got { + t.Fatalf("got true, want false (kind is modify)") + } +} + +// TestComprehensionTextCompareCaughtEveryIteration pins the guard's per-iteration +// reach. A comprehension body is evaluated once per element, but the interpreter +// keeps only ONE recorded value per AST node, so reading state after the fact +// would see the LAST iteration only: `changes.all(c, c.new > c.old)` over a text +// change followed by a numeric one would come back a clean `true` with the text +// compare invisible. The guard watches the operands as they evaluate, so every +// iteration is seen — and a compare that short-circuits away is still not judged. +func TestComprehensionTextCompareCaughtEveryIteration(t *testing.T) { + env, err := newEvalEnv() + if err != nil { + t.Fatalf("newEvalEnv: %v", err) + } + textChange := EvalChange{Subject: "s:1", File: "f.yaml", Path: "/a", Kind: "modify", Old: "12", New: "6"} + numChange := EvalChange{Subject: "s:2", File: "f.yaml", Path: "/b", Kind: "modify", Old: json.Number("1"), New: json.Number("9")} + + // all() visits every element: the text compare is the FIRST iteration and the + // numeric one overwrites the recorded state — it must still be caught. + textFirst := EvaluationInput{ChangeSet: ChangeSet{Changes: []EvalChange{textChange, numChange}}, Facts: map[string]map[string]Fact{}} + if got, err := evalLeaf(env, textFirst, textChange, "prod", "changes.all(c, c.new > c.old)"); err == nil { + t.Errorf("all() over a text-then-number changeset = (%v, nil) — the first iteration ordered text lexically", got) + } + numFirst := EvaluationInput{ChangeSet: ChangeSet{Changes: []EvalChange{numChange, textChange}}, Facts: map[string]map[string]Fact{}} + if got, err := evalLeaf(env, numFirst, numChange, "prod", "changes.all(c, c.new > c.old)"); err == nil { + t.Errorf("all() over a number-then-text changeset = (%v, nil) — the second iteration ordered text lexically", got) + } + + // exists() short-circuits on the first true, so the text element is never + // compared at all — nothing was ordered, and the clean answer stands. + got, err := evalLeaf(env, numFirst, numChange, "prod", "changes.exists(c, c.new > c.old)") + if err != nil { + t.Fatalf("exists() short-circuiting before the text element must not error: %v", err) + } + if !got { + t.Fatalf("exists() = false, want true (9 > 1 on the first element)") + } +} + +// TestUnrepresentableNumericFailsSafe covers the ADR-0013 residual #1 arm: a +// numeric literal that fits neither int64 nor float64 used to fall back to its +// STRING form, so `9e399 > 1e400` was the lexical "9e399" > "1e400" = true — a +// numerically FALSE compare answered true, with no error. It must fail safe. +func TestUnrepresentableNumericFailsSafe(t *testing.T) { + env, err := newEvalEnv() + if err != nil { + t.Fatalf("newEvalEnv: %v", err) + } + in := EvaluationInput{ChangeSet: ChangeSet{Changes: []EvalChange{{ + Subject: "s:1", File: "f.yaml", Path: "/x", Kind: "modify", + Old: json.Number("1e400"), New: json.Number("9e399"), + }}}, Facts: map[string]map[string]Fact{}} + ch := in.ChangeSet.Changes[0] + for _, expr := range []string{"new > old", "new >= old", `new == "9e399"`} { + if got, err := evalLeaf(env, in, ch, "prod", expr); err == nil { + t.Errorf("evalLeaf(%q) = (%v, nil) — an unrepresentable numeric literal must never bind to a comparable value", expr, got) + } + } + // A large-but-representable literal still binds (lossily, as float64) — the + // documented residual, unchanged by this fix. + big := EvaluationInput{ChangeSet: ChangeSet{Changes: []EvalChange{{ + Subject: "s:1", File: "f.yaml", Path: "/x", Kind: "modify", + Old: json.Number("18446744073709551617"), New: json.Number("18446744073709551618"), + }}}, Facts: map[string]map[string]Fact{}} + if _, err := evalLeaf(env, big, big.ChangeSet.Changes[0], "prod", "new >= old"); err != nil { + t.Errorf("an over-int64 but float64-representable literal must still compare: %v", err) + } +} + +// TestLegacyAggregatePathAlsoRefusesTextOrdering covers the OTHER evaluation +// seam. The walking-skeleton Aggregate/evalRule path declares old/new as CEL +// StringType and binds the differ's RAW canonical strings, so a bare `new >= old` +// there is ALWAYS lexical — it relied on authors writing int()/double(), with +// nothing enforcing it. One unguarded evaluator is how this fail-open returns, so +// the guard is applied to both. Coerced compares on that path still evaluate. +func TestLegacyAggregatePathAlsoRefusesTextOrdering(t *testing.T) { + env, err := newCELEnv() + if err != nil { + t.Fatalf("newCELEnv: %v", err) + } + act := map[string]any{"old": "12", "new": "6", "changes": []map[string]string{}} + + // The bytes rows belong on THIS seam too: one unguarded evaluator is how the + // class returns, and that argument applies per-form as much as per-seam. + for _, expr := range []string{"new >= old", "new < old", "old > new", "bytes(new) >= bytes(old)", `b"6" >= b"12"`} { + if got, err := evalRule(env, act, expr); err == nil { + t.Errorf("evalRule(%q) = (%v, nil) — the walking-skeleton path ordered raw canonical text", expr, got) + } + } + // Both polarities: the coerced forms this path always mandated still work. + for expr, want := range map[string]bool{ + "int(new) >= int(old)": false, + "int(new) < int(old)": true, + `new == "6"`: true, + "old == new": false, + } { + got, err := evalRule(env, act, expr) + if err != nil { + t.Errorf("evalRule(%q) errored: %v", expr, err) + continue + } + if got != want { + t.Errorf("evalRule(%q) = %v, want %v", expr, got, want) + } + } +} + +// TestUnrepresentableNumericInterpolatesItsRefusal pins what an adopter actually +// READS when a leaf whose operand toCEL refused to bind also interpolates that +// operand into its message. `{{ old }}` / `{{ new }}` resolve straight out of the +// activation, so since D-131 they resolve to the CEL error VALUE and render its +// sentence where a number used to appear — an extreme corner (|v| beyond +// ~1.8e308) with no safety consequence, since the leaf already errored and the +// finding is predicate.error -> REVIEW either way. +// +// It is goldened deliberately, exact sentence and all: the rendering is a +// judgement call (authored prose in a value slot, chosen over a bare `{{ old }}` +// placeholder because it says WHY the rule could not decide), and this row makes +// any future change to it a deliberate act rather than a silent side effect of +// editing toCEL's message. MUTATION: restore toCEL's old `return x.String()` +// fallback and the message renders "1e400 -> 9e399" — the numbers, from the very +// demotion to text that D-131 closed. +func TestUnrepresentableNumericInterpolatesItsRefusal(t *testing.T) { + mp, bind := shrinkRulePolicy() + mp.Spec.Rules[0].Prove.When.Leaf.Message = "partitions may not decrease ({{ old }} -> {{ new }})" + in := EvaluationInput{ChangeSet: ChangeSet{Changes: []EvalChange{{ + Subject: "topic-registry:orders.events.v1", + File: "topics/prod/orders-events.yaml", + Path: "/partitions", + Kind: "modify", + Old: json.Number("1e400"), + New: json.Number("9e399"), + }}}, Facts: map[string]map[string]Fact{}, Require: []string{"non-destructive"}} + + res, err := Cover(mp, bind, &in) + if err != nil { + t.Fatalf("Cover: %v", err) + } + onlyPredicateError(t, res) + + const want = "partitions may not decrease (" + + "the number 1e400 is too large for assent to compare (it exceeds the representable numeric range)" + + " -> " + + "the number 9e399 is too large for assent to compare (it exceeds the representable numeric range))" + if got := res.Findings[0].Message; got != want { + t.Errorf("finding message =\n %q\nwant\n %q", got, want) + } + + // The OTHER message path diverges on this same input, and pinning that is the + // point: internal/render goes through EvalScalar, which rejects an error value + // (types.IsError) and propagates a Go error instead of rendering a sentence. + // Any future unification of the two interpolators changes one of these two + // behaviours — this row makes that visible instead of silent. + act := LeafActivation(in, in.ChangeSet.Changes[0], "prod") + if v, err := EvalScalar("new", act); err == nil { + t.Errorf("EvalScalar(\"new\") = (%v, nil) — an unrepresentable numeric must not render as a value on the render path", v) + } +} + +// TestToCELNeverYieldsAStringForANumericLiteral pins the unit-level invariant the +// evaldecode package doc asserts: toCEL never converts a numeric literal into its +// string form (the silent lexical demotion). +func TestToCELNeverYieldsAStringForANumericLiteral(t *testing.T) { + for _, lit := range []string{"12", "-4", "3.5", "18446744073709551617", "9e399", "1e400", "-1e400"} { + if got := toCEL(json.Number(lit)); got == lit { + t.Errorf("toCEL(json.Number(%q)) returned the STRING form — a numeric literal must never demote to text", lit) + } + } +} diff --git a/internal/evaldecode/evaldecode.go b/internal/evaldecode/evaldecode.go index da88b18..7e2fe1f 100644 --- a/internal/evaldecode/evaldecode.go +++ b/internal/evaldecode/evaldecode.go @@ -31,6 +31,20 @@ // (typed); an absent/undecodable value becomes nil (a numeric/relational compare // over nil ERRORS in cel-go -> the engine's tri-state fail-safe -> REVIEW), never a // permissive string. +// +// WHAT THIS PACKAGE DOES NOT CLOSE (and where that is closed). Decoding cannot be +// the whole answer, because a value that is GENUINELY text stays text: a YAML +// `partitions: "12"` is a !!str and this package keeps it the Go string "12" by +// design (the differ deliberately distinguishes it from the number 12). CEL then +// binds a string, and CEL's < <= > >= are DEFINED over strings as a lexical +// compare — so before D-131 the D-016 `new >= old` over a quoted shrink 12->6 +// answered the lexical "6" >= "12" = TRUE and the shrink APPROVEd. Decoding could +// not prevent that; only the evaluator can. Since D-131 the engine seam refuses +// it: aggregate's textOrderGuard makes any relational compare whose operand +// actually evaluates to text an evaluation ERROR -> predicate.error -> REVIEW. +// The two layers are complementary and BOTH load-bearing — this package makes a +// numeric literal compare numerically (BLOCK, the right answer); the guard makes +// a genuinely-textual operand fail safe (REVIEW) instead of answering wrongly. package evaldecode import ( @@ -51,17 +65,22 @@ import ( // "True"/"False" to json.Number, fall to a lexical STRING compare, and mis-order. // - `"..."` -> the unquoted string (a !!str value; stays a STRING so a // numeric rule over it does NOT numeric-compare it — the differ deliberately -// kept a string "12" distinct from the number 12). +// kept a string "12" distinct from the number 12. An ORDERING rule over that +// string does not compare it lexically either: since D-131 the evaluator +// refuses to order text and fails safe to REVIEW). // - anything else -> json.Number(literal) (a numeric literal, bound typed by // toCEL). Every bool (all six emitted spellings), JSON-quoted string, and null // is handled ABOVE, so the fallthrough is exactly a numeric literal — never a // capitalized bool and never a raw string. // -// LIMITATION (documented, NOT this lane's bug — an S02 limitation): a numeric -// literal larger than int64 flows through as json.Number and toCEL falls back to -// float64 (a lossy compare) or its string form. That is the ADR-0013 residual #1 -// the S02 evaluator owns; DecodeCanonical preserves the literal faithfully and -// leaves that edge to toCEL. +// LIMITATION (the ADR-0013 residual #1 the evaluator owns, not the decoder): a +// numeric literal larger than int64 flows through as json.Number and toCEL binds +// it as float64 — a LOSSY compare, which is still live: two distinct literals +// beyond 2^53 can compare equal. What is no longer live is the second half of the +// old wording, "or its string form": since D-131 a literal representable as +// NEITHER int64 nor float64 (beyond ~1.8e308) binds a CEL error value, so it can +// never be ordered as text; it fails safe instead. DecodeCanonical itself is +// unchanged — it preserves the literal faithfully and leaves the edge to toCEL. func DecodeCanonical(s string) any { switch s { case "", "null": diff --git a/internal/evaldecode/evaldecode_test.go b/internal/evaldecode/evaldecode_test.go index 8b27365..5204830 100644 --- a/internal/evaldecode/evaldecode_test.go +++ b/internal/evaldecode/evaldecode_test.go @@ -131,33 +131,103 @@ func TestRealDiffNumericShrinkBlocks(t *testing.T) { assertHasFinding(t, res.Findings, aggregate.EffectBlock, "partition-count-shrunk") } -// TestStringOldNewFailsOpen is the MUTATION proof that the decoder is load-bearing. -// With the SAME rule but Old/New left as the RAW canonical strings "12"/"6" (the -// un-decoded shape), `new >= old` is a LEXICAL compare and "6" >= "12" is TRUE — the -// obligation "proves", nothing fires, and the shrink APPROVEs. That is exactly the -// forbidden outcome decodeCanonical closes; if this ever stops APPROVing, the engine -// changed and the gate above no longer demonstrates the decoder matters. -func TestStringOldNewFailsOpen(t *testing.T) { - in := aggregate.EvaluationInput{ - ChangeSet: aggregate.ChangeSet{Changes: []aggregate.EvalChange{{ - Subject: "file:topics/prod/orders-events.yaml", - File: "topics/prod/orders-events.yaml", - Path: "/partitions", - Kind: "modify", - Old: "12", // RAW canonical string (the fail-open shape the decoder replaces) - New: "6", - }}}, - Facts: map[string]map[string]aggregate.Fact{}, - Require: []string{"non-destructive"}, +// TestUndecodedStringOldNewFailsSafe is the MUTATION proof that the decoder is +// load-bearing — rewritten for D-131. It used to assert that the un-decoded shape +// APPROVEs a shrink (documenting the lexical fail-open as the thing the decoder +// closes). That fail-open is now closed a SECOND time, at the engine seam: a +// relational compare over two string-bound operands errors instead of answering +// lexically. So the mutation no longer flips the decision to APPROVE — it degrades +// it to the fail-safe REVIEW. +// +// The proof stays DISCRIMINATING by asserting the pair, not merely "not APPROVE": +// decoded (json.Number) -> BLOCK + partition-count-shrunk (the policy's real +// answer); un-decoded (raw canonical strings) -> REVIEW + predicate.error. Delete +// the decoder and the first arm fails; delete the engine guard and the second arm +// goes back to APPROVE. Neither layer can rot silently. +func TestUndecodedStringOldNewFailsSafe(t *testing.T) { + mkInput := func(oldVal, newVal any) aggregate.EvaluationInput { + return aggregate.EvaluationInput{ + ChangeSet: aggregate.ChangeSet{Changes: []aggregate.EvalChange{{ + Subject: "file:topics/prod/orders-events.yaml", + File: "topics/prod/orders-events.yaml", + Path: "/partitions", + Kind: "modify", + Old: oldVal, + New: newVal, + }}}, + Facts: map[string]map[string]aggregate.Fact{}, + Require: []string{"non-destructive"}, + } } + mp, bind := shrinkPolicy() + + decoded := mkInput(json.Number("12"), json.Number("6")) + resDecoded, err := aggregate.CoverWithApproval(mp, bind, &decoded, nil) + if err != nil { + t.Fatalf("CoverWithApproval (decoded): %v", err) + } + if resDecoded.Decision != aggregate.DecisionBlock { + t.Fatalf("decoded decision = %q, want BLOCK — the typed numeric compare 6 >= 12 is false", resDecoded.Decision) + } + assertHasFinding(t, resDecoded.Findings, aggregate.EffectBlock, "partition-count-shrunk") + + undecoded := mkInput("12", "6") // RAW canonical strings: the un-decoded shape + resStr, err := aggregate.CoverWithApproval(mp, bind, &undecoded, nil) + if err != nil { + t.Fatalf("CoverWithApproval (un-decoded): %v", err) + } + if resStr.Decision == aggregate.DecisionApprove { + t.Fatalf("un-decoded decision = APPROVE — the lexical fail-open (\"6\" >= \"12\" is true) is OPEN again") + } + if resStr.Decision != aggregate.DecisionReview { + t.Fatalf("un-decoded decision = %q, want REVIEW (the relational-over-text guard fails safe)", resStr.Decision) + } + assertHasFinding(t, resStr.Findings, aggregate.EffectRequireReview, "predicate.error") +} + +// TestQuotedNumericShrinkFailsSafeEndToEnd is the REACHABLE half of D-131, driven +// through the whole production chain (change.Diff -> DecodeCanonical -> Cover) on +// a real base/head YAML pair whose `partitions` is QUOTED — routine in adopter +// YAML. The decoder keeps a !!str a Go string BY DESIGN (a quoted "12" is not the +// number 12), so before the fix the D-016 rule's bare `new >= old` became the +// lexical "6" >= "12" = TRUE: `non-destructive` "proved", nothing fired, and a +// partition shrink came out APPROVE with zero findings. The compare must error. +func TestQuotedNumericShrinkFailsSafeEndToEnd(t *testing.T) { + path := "topics/prod/orders-events.yaml" + base := readFixture(t, "shrink-diff-quoted", "base", path) + head := readFixture(t, "shrink-diff-quoted", "head", path) + + cs, err := change.Diff(path, base, head) + if err != nil { + t.Fatalf("change.Diff: %v", err) + } + if len(cs.Changes) != 1 { + t.Fatalf("changes = %d (%+v), want exactly 1 (/partitions modify)", len(cs.Changes), cs.Changes) + } + // The differ's canonical render JSON-QUOTES a !!str, discriminating it from the + // !!int literal — that tag discrimination is what makes the value a Go string. + if c := cs.Changes[0]; c.Path != "/partitions" || c.Old != `"12"` || c.New != `"6"` { + t.Fatalf("change = %+v, want /partitions old=%q new=%q (JSON-quoted !!str render)", c, `"12"`, `"6"`) + } + + in := evaldecode.BuildEvaluationInput(cs, aggregate.MR{}, []string{"non-destructive"}) + got := in.ChangeSet.Changes[0] + if got.Old != "12" || got.New != "6" { + t.Fatalf("decoded old/new = %#v/%#v, want the bare Go strings \"12\"/\"6\" (a !!str stays a string)", got.Old, got.New) + } + mp, bind := shrinkPolicy() res, err := aggregate.CoverWithApproval(mp, bind, &in, nil) if err != nil { t.Fatalf("CoverWithApproval: %v", err) } - if res.Decision != aggregate.DecisionApprove { - t.Fatalf("decision = %q, want APPROVE — this test DOCUMENTS the lexical fail-open (\"6\" >= \"12\" is true) that decodeCanonical closes; a non-APPROVE here means the mutation proof is stale", res.Decision) + if res.Decision == aggregate.DecisionApprove { + t.Fatalf("decision = APPROVE with findings %+v — a quoted-numeric partition shrink 12->6 reached APPROVE through the real differ", res.Findings) } + if res.Decision != aggregate.DecisionReview { + t.Fatalf("decision = %q, want REVIEW (predicate error over text operands)", res.Decision) + } + assertHasFinding(t, res.Findings, aggregate.EffectRequireReview, "predicate.error") } // TestCapitalizedBoolDecodesAsBool is the F1 proof: a !!bool field rendered @@ -232,9 +302,14 @@ func TestSubjectOf(t *testing.T) { func readShrinkFixture(t *testing.T, side, path string) []byte { t.Helper() - b, err := os.ReadFile(filepath.Join("testdata", "shrink-diff", side, filepath.FromSlash(path))) //nolint:gosec // fixed test fixture path, not user input. + return readFixture(t, "shrink-diff", side, path) +} + +func readFixture(t *testing.T, dir, side, path string) []byte { + t.Helper() + b, err := os.ReadFile(filepath.Join("testdata", dir, side, filepath.FromSlash(path))) //nolint:gosec // fixed test fixture path, not user input. if err != nil { - t.Fatalf("read %s fixture %s: %v", side, path, err) + t.Fatalf("read %s/%s fixture %s: %v", dir, side, path, err) } return b } diff --git a/internal/evaldecode/testdata/shrink-diff-quoted/base/topics/prod/orders-events.yaml b/internal/evaldecode/testdata/shrink-diff-quoted/base/topics/prod/orders-events.yaml new file mode 100644 index 0000000..c82d5bf --- /dev/null +++ b/internal/evaldecode/testdata/shrink-diff-quoted/base/topics/prod/orders-events.yaml @@ -0,0 +1,4 @@ +metadata: + name: orders.events.v1 +partitions: "12" +retentionMs: 604800000 diff --git a/internal/evaldecode/testdata/shrink-diff-quoted/head/topics/prod/orders-events.yaml b/internal/evaldecode/testdata/shrink-diff-quoted/head/topics/prod/orders-events.yaml new file mode 100644 index 0000000..662e4e6 --- /dev/null +++ b/internal/evaldecode/testdata/shrink-diff-quoted/head/topics/prod/orders-events.yaml @@ -0,0 +1,4 @@ +metadata: + name: orders.events.v1 +partitions: "6" +retentionMs: 604800000