fix(engine): relational compare over string-bound operands must fail safe, not lexically

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)

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:

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. |

docs/planning/predicate-scope.md

@@ -35,6 +35,12 @@ admission object).
 - `facts.<provider>.<output>` and `mr.<field>` 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.

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.

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 {