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

[konih]

Closes a P1 decision-path fail-open: CEL's relational operators are defined over strings and compare them lexically, so an ordering leaf silently answered a boolean whenever its operands bound as text. Base e54a243 (lane started from 163e91d, rebased).

Reproduction (done first, both levels, before any fix)

End-to-end through the production chain (change.Diff → evaldecode.DecodeCanonical → aggregate.Cover), on a routine adopter shape — a quoted numeric. New fixture internal/evaldecode/testdata/shrink-diff-quoted/{base,head} is partitions: "12" → "6"; that is a !!str, so the differ renders it JSON-quoted and the decoder keeps it the Go string "12" by design (a quoted "12" is not the number 12):

decision = APPROVE with findings [] — a quoted-numeric partition shrink 12->6
reached APPROVE through the real differ

The D-016 partitions-must-not-shrink rule (when: new >= old) evaluated the lexical "6" >= "12" = true, proved non-destructive, fired nothing → BLOCK → APPROVE flip, zero findings.

The mirror case is equally wrong and is now pinned as its own test: a legitimate grow "6" → "12" evaluated "12" >= "6" = false, so the rule fired and BLOCKed a change the policy permits:

decision = BLOCK with findings [{Rule:partitions-must-not-shrink … Code:partition-count-shrunk}]
— a lexical compare judged a legitimate grow 6->12 destructive

Unit level (all four operators, both argument orders, all returned a boolean with err == nil): new >= old → true, new <= old → false, old >= new → false, … plus entry >= oldEntry, mr.author < path, and the overflow arm 9e399 > 1e400 → true (numerically false), from toCEL's x.String() fallback.

Design choice — value-based guard at the evaluator, strict, no exemption

evalLeaf builds its program with a cel.CustomDecorator that plants a watcher on every operand of every relational operator (<, <=, >, >=). If an operand actually evaluates to a string, the leaf returns an error → predicate.error → REVIEW.

• Why value-based, not syntax-based, and why lint cannot do this. Whether new >= old is sound is a property of the adopter's data, not of the policy text: correct over numbers, unsound over strings, and identical source either way. I checked checkLeafScope, checkPredicateScope and checkFactsShape — none could refuse this without also refusing the D-016 rule the whole corpus depends on. Lint sees the rule; it never sees the change it will judge. So no lint rule was added, deliberately.
• Why watching, not reading post-eval EvalState. My first implementation used cel.OptTrackState and had a real hole: state keeps one value per AST node id, so changes.all(c, c.new > c.old) over a text element followed by a numeric one came back a clean true with the text compare overwritten and invisible. Watching sees every iteration. TestComprehensionTextCompareCaughtEveryIteration pins it in both element orders.
• Short-circuited compares are never judged. A watcher on a branch that did not run never fires, so no existing policy can flip to REVIEW for a compare that did not happen (kind == "add" && new >= old stays a clean false).
• No string-literal exemption. I considered exempting operands that are string literals or explicit string(...) as "declared intent". Rejected: it re-admits the identical defect — new >= "12" with new bound to !!str "6" is still lexically true, still APPROVE. A quoted numeric in a policy leaf is the same authoring slip as a quoted numeric in adopter YAML. Strict is also the smaller diff with no second code path.
• Second seam closed too. The guard is applied to evalRule as well (the exported walking-skeleton Aggregate path, test-only today), whose env declares old/new as StringType and binds the raw canonical strings — every bare relational there was lexical by construction, with only convention mandating int(). One unguarded evaluator is how this class comes back; same drift argument that pulled the decoder into internal/evaldecode (D-055c).
• Scope call: not applied to EvalScalar. {{ }} message interpolation renders text and decides nothing; guarding it would widen blast radius for no safety gain.
• toCEL second arm. A numeric literal representable as neither int64 nor float64 now binds a CEL error value instead of its string form. The over-int64-but-float64-representable lossy case is unchanged and still live — that half of ADR-0013 residual build(deps): bump gitleaks/gitleaks-action from 2 to 3 #1 stays open, and the docs now say exactly that.

Legitimate string relational compares — the search, and the answer

Searched examples/ (policies, packs, comparison corpus, lint fixtures, repo corpus), the adopter packs, cmd/assent/testdata, internal/adoptertest/testdata, the ADRs and every Go test. Every relational leaf in the repo is numeric: new >= old / new <= facts.quota.max_partitions.value (partitions), new >= facts.band.memory_mb.value.min, new >= entry.min_replicas (replicas/memory), size(...) > 0, and the already-coerced int(new) <= facts.quota.max.value. Nothing orders text — no ISO-date, semver or name ordering anywhere. (Worth noting the julieops corpus does carry quoted numerics — num.partitions: "1" — which is the real-world evidence that the reachable case is routine, not exotic.)

So the strict rule breaks nothing that exists, and deliberate ordering stays expressible by coercing first: int(new) >= int(old) (already the repo idiom), double(...), or timestamp(a) < timestamp(b) for ISO dates. What is genuinely foreclosed — stated plainly rather than hidden — is ordering raw text in tier-1 assert; that graduates to the Rego escape hatch, consistent with ADR-0013's "don't grow a programming language in YAML". Equality, in and the string functions are untouched, and TestLegitimateComparesStillEvaluate pins twelve of them still evaluating to the right booleans.

Changed outputs — the null result, stated affirmatively

No golden, fixture, corpus or gate output changed. D-016 replay, the conformance suite, both comparison suites + the E6 seed, both dogfood packs (service-catalog, infra-vars), the coverage gate and the determinism gate are all green unchanged — expected, given the search above.

Exactly one existing test changed, and it is a finding rather than a regeneration: internal/evaldecode's TestStringOldNewFailsOpen asserted APPROVE to document the lexical fail-open as the decoder's mutation proof. That mutation now degrades to REVIEW instead of flipping to APPROVE, so the old assertion was blessing behaviour that no longer exists. Renamed TestUndecodedStringOldNewFailsSafe and rewritten to assert the discriminating pair rather than the weaker "not APPROVE" (which would pass with the decoder deleted):

input	decision	finding
decoded (json.Number)	BLOCK	partition-count-shrunk
un-decoded (raw strings)	REVIEW	predicate.error

Delete the decoder and the first arm reds; delete the guard and the second arm goes back to APPROVE. Neither layer can rot silently.

Mutation evidence

Every mutation applied, run, and reverted with git diff confirming the edit landed:

1. textOrderGuard.record neutered → 6 tests red across two packages (TestQuotedNumericShrinkMustNotApproveThroughCover, TestQuotedNumericGrowMustNotBlockLexically, TestRelationalOverStringOperandsErrors, TestComprehensionTextCompareCaughtEveryIteration, TestUndecodedStringOldNewFailsSafe, TestQuotedNumericShrinkFailsSafeEndToEnd).
2. cel.CustomDecorator(guard.decorate) removed from the Program call → the same 6 red. (This one documents the fragile plan-time injection for a future cel-go bump.)
3. toCEL's types.NewErr reverted to x.String() → TestToCELNeverYieldsAStringForANumericLiteral and TestUnrepresentableNumericFailsSafe red. Note the layering visible here: with the string form back, the relational arms were still caught by the guard and only new == "9e399" slipped — which is precisely why both arms exist.

RED-before-GREEN is in the history of this lane: every new test was written first and observed failing with the exact wrong values quoted above; -v used throughout so a non-matching -run cannot masquerade as a pass.

cel-go interaction check (v0.30.0): textOrderWatch wraps operands as a plain Interpretable, unlike cel-go's own decObserveEval which preserves InterpretableAttribute/InterpretableConst. Safe because a relational operand's only consumer is the binary call itself — and proven, not merely argued: TestEvalRuleEnforcesCostBudget's costBombWhen puts the expensive comprehension as the left operand of > 0, so it is exactly a wrapped node, and the cost budget still fires. A comment now pins that shape so a future rewrite doesn't silently drop the proof.

Docs truth (F3) — and a second stale claim found

• internal/evaldecode/evaldecode.go: the package doc claimed a value "must NEVER silently become a lexical string … never a permissive string" while a !!str did exactly that. It now says what is true: decoding cannot close this, because a genuinely-textual value legitimately stays text; the engine seam closes it, and the two layers are complementary and both load-bearing. The LIMITATION no longer says "a lossy compare or its string form" — the string-form half is closed; the lossy half is named as still live.
• Second docs-truth defect found and fixed (claiming it explicitly, since it is beyond the reported finding): internal/core/aggregate's package doc still described the pre-evaldecode world — "They are bound to CEL as raw string values" and "numeric rules MUST use int()/double() in the when expression". Both false since the decoder landed (the corpus is full of bare new >= old). Rewritten to the current truth plus the new guard.

Records

• D-129 logged (next free row; D-125 is the last on main, leaving 126–128 for the concurrent session's uncommitted rows). It states the behaviour change plainly: a policy that previously evaluated (wrongly) now errors → REVIEW.
• ADR-0013 Amendment 1 — the semantics change belongs in the ADR that owns assert and that named this exact residual ("numeric type coercion YAML/HCL→CEL — highest risk"). Follows the ADR-0007 Amendment 2 / ADR-0011 Amendment 3 precedent.
• docs/planning/predicate-scope.md gains the authoring rule (that file is the frozen authoring contract).
• No openspec/ change entry — judged and stated: openspec/changes/ contains only its README.md, so this repo has never used change proposals in practice, and a fail-safe correction of an existing frozen semantic (no new contract surface, no schema change) is proportionate to failing test first → D-row → ADR amendment. schemas/ is byte-unchanged (the compare exit gate's git diff schemas/ guard proves it).
• OQ-27: absent from docs/planning/open-questions.md at e54a243 — checked at lane start and again at PR time; the concurrent lane has not landed it. I did not invent a row that would collide. If it lands before this merges, mark it resolved → D-129; if it does not, D-129 carries the resolution on its own.

Gates

gate	result
task check	exit 0, all 14 stages ran to completion: fmt · vet · lint · test · coverage · build · dogfood-comparison · compare-exitgate-test · changelog-verify · release-changelog-gate-test · release-verify-tag-gate-test · docs-gates · lint-depguard-test · lint-workflow-pins-test
task determinism	exit 0
task dogfood-examples	exit 0 (all cases PASS, coverage OK — 4 rules, both polarities)
bash hack/compare/exitgate_test.sh	exit 0 (incl. git diff schemas/ empty)
CI=true go test ./...	green
go vet -tags e2e ./...	clean
TestExecDigestPin	did not fire — no regression of the PR #27 flake

task check was run again after the final task changelog-write commit, and is green at the pushed tip.

Risks

• The plan-time decorator is the one piece coupled to cel-go internals (cel.CustomDecorator + interpreter.Interpretable). A cel-go major bump should re-run mutation 2 and the cost-budget test. Mitigated by pinning the reasoning in comments at both sites.
• The behaviour change is real: any adopter policy that today orders text — in either direction — becomes REVIEW rather than a wrong answer. That is the intended fail-safe direction, and nothing in-tree is affected, but it is a semantic change to a frozen surface, hence the ADR amendment.
• Aggregate/evalRule has no non-test caller today; guarding it is defensive rather than load-bearing, and it is the one part of the diff beyond the reported finding.

Scope respected: internal/provider/builtin/{repo_file,resource_owner}.go, .github/workflows/**, Taskfile.yml, cliff.toml and internal/forge/** untouched.