From acb2415d2b7ed7af0edd0f11b48e84a94693575e Mon Sep 17 00:00:00 2001 From: Konrad Heimel Date: Sat, 8 Aug 2026 15:42:45 +0200 Subject: [PATCH 1/2] :bug: fix(engine): relational compare over string-bound operands must fail safe, not lexically CEL defines < <= > >= over strings as a LEXICAL compare, so an ordering leaf silently 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 `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, and returned APPROVE with zero findings. The mirror case is equally wrong: a legitimate grow "6" -> "12" evaluated false and BLOCKed. evalLeaf now plants a watcher on every operand of every relational operator; an operand that ACTUALLY evaluates to text makes the leaf an evaluation error -> predicate.error -> REVIEW. Value-based, not syntax-based: whether `new >= old` is sound depends on the adopter data, so lint cannot decide it. Watching (rather than reading post-eval EvalState, which keeps one value per node id) makes every comprehension iteration visible; a short-circuited compare never runs its watcher, so a policy cannot flip to REVIEW for a compare that did not happen. toCEL also stops demoting an unrepresentable numeric literal to its string form (9e399 > 1e400 was lexically true, numerically false); it binds a CEL error value instead. The over-int64-but-float64-representable lossy case is unchanged and still live. Behaviour change: a policy that previously evaluated (wrongly) now errors. Nothing in examples/, the comparison corpus, either dogfood pack or the test corpus orders text, so no golden or gate output changed. Ordering quoted numerics means coercing first -- int(new) >= int(old). ADR-0013 Amendment 1, D-129. --- docs/adr/0013-assert-syntax-and-backend.md | 25 ++ docs/decisions/decisions.md | 1 + docs/planning/predicate-scope.md | 6 + internal/core/aggregate/aggregate.go | 48 ++- internal/core/aggregate/evalrule_cost_test.go | 5 + internal/core/aggregate/evaluate.go | 158 ++++++++- .../core/aggregate/relational_string_test.go | 313 ++++++++++++++++++ internal/evaldecode/evaldecode.go | 31 +- internal/evaldecode/evaldecode_test.go | 119 +++++-- .../base/topics/prod/orders-events.yaml | 4 + .../head/topics/prod/orders-events.yaml | 4 + 11 files changed, 665 insertions(+), 49 deletions(-) create mode 100644 internal/core/aggregate/relational_string_test.go create mode 100644 internal/evaldecode/testdata/shrink-diff-quoted/base/topics/prod/orders-events.yaml create mode 100644 internal/evaldecode/testdata/shrink-diff-quoted/head/topics/prod/orders-events.yaml diff --git a/docs/adr/0013-assert-syntax-and-backend.md b/docs/adr/0013-assert-syntax-and-backend.md index 39da6b4..050fe42 100644 --- a/docs/adr/0013-assert-syntax-and-backend.md +++ b/docs/adr/0013-assert-syntax-and-backend.md @@ -105,6 +105,31 @@ 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-129): 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 +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. No policy in the corpus, the comparison suite, or either dogfood pack ordered text. +- 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..1b73768 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-129 | 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 a string 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`** 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. | diff --git a/docs/planning/predicate-scope.md b/docs/planning/predicate-scope.md index d13b39b..517d4a4 100644 --- a/docs/planning/predicate-scope.md +++ b/docs/planning/predicate-scope.md @@ -35,6 +35,12 @@ 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-129). Order quoted numerics with `int(new) >= int(old)` or + `double(...)`, and dates with `timestamp(a) < timestamp(b)`. 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..814e195 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-129 +// 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-129 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/evalrule_cost_test.go b/internal/core/aggregate/evalrule_cost_test.go index ac97830..856a1a6 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-129 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..88b2f85 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-129); 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-129) — 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,128 @@ func evalLeaf(env *cel.Env, in EvaluationInput, ch EvalChange, envLabel, expr st return b, nil } +// textOrderGuard is the D-129 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-129: 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. +// +// 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) overload +// compares 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 adopter-facing refusal when the evaluation ordered 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) + if s, isText := v.(types.String); isText { + w.guard.record(w.op, string(s)) + } + 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 +316,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-129). 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 +334,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..7cf11c1 --- /dev/null +++ b/internal/core/aggregate/relational_string_test.go @@ -0,0 +1,313 @@ +package aggregate + +// relational_string_test.go is the regression suite for the D-129 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 + } { + 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) + } + } +} + +// 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{}} + + for _, expr := range []string{"new >= old", "new < old", "old > new"} { + 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) + } + } +} + +// 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..aa80681 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-129 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-129 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-129 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-129 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..e80ac2e 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-129. 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-129, 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 From 74192ce62e02b193c6dd877bf87066a6c627948e Mon Sep 17 00:00:00 2001 From: Konrad Heimel Date: Sat, 8 Aug 2026 15:55:17 +0200 Subject: [PATCH 2/2] :wrench: chore(release): regenerate CHANGELOG.md for the relational-string fail-safe lane --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 51d5914..37c6e98 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -101,6 +101,7 @@ 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 ### Other - :construction_worker: ci(lint): depguard deny-rules for the D-123 pure tree (REQ-AUD-S07-01)