feat(engine): inspect request body in policy rules

[dpup]

Summary

Lets Keep policy rules match on the parsed HTTP request body (params.body), and gives gatekeepers a cheap, compile-time signal for whether buffering the body is even necessary for a given scope.

Motivation: a gatekeeper sitting in front of an LLM/API gateway wants to apply rules like params.body.model == 'gpt-4' or hasSecrets(params.body.prompt) — but buffering and parsing request bodies is expensive, so it should only do so when some rule in the active scope actually inspects the body.

What's included

• NewHTTPCallWithBody(method, host, path, body) (helpers.go) — builds on the existing NewHTTPCall and additionally sets params.body. NewHTTPCall is left untouched for back-compat.
• Engine.RequiresBody(scope) bool (keep.go) — the trigger signal a gatekeeper reads. Returns false for an unknown scope.
• Compile-time reference analysis (internal/cel/env.go) — each compiled Program records the set of top-level params.* fields it references, computed once by walking the CEL AST. Exposed as the generalized Program.ReferencesParam(field); RequiresBody is built on ReferencesParam("body"). Detection handles:
  • dot-selection: params.body
  • index access with a string literal: params["body"]
  • nested access: params.body.model, params.body[0], hasSecrets(params.body.x), has(params.body)
  • references hidden behind rule aliases (analysis runs on the resolved expression)

CEL eval, nested navigation, hasSecrets, and redaction already operate on params.body for free — confirmed by an end-to-end test that denies a gpt-4 request.

Testing

• internal/cel/refs_test.go — table-driven coverage of ReferencesParam (dot/index/nested/negative cases, multiple fields, nil program).
• internal/engine/requires_body_test.go — Evaluator.RequiresBody across rule shapes, including alias indirection.
• keep_test.go — Engine.RequiresBody (positive, no-body, unknown scope) plus an end-to-end deny via NewHTTPCallWithBody.
• helpers_test.go — NewHTTPCallWithBody including nil-body handling.

make test-unit (race), make lint, go vet, and gofmt all pass.

🤖 Generated with Claude Code

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @dpup's task in 3m 14s —— View job

---

Code Review

• Gather context and read changed files
• Review helpers.go and helpers_test.go
• Review internal/cel/env.go and internal/cel/refs_test.go
• Review internal/engine/eval.go and internal/engine/requires_body_test.go
• Review keep.go and keep_test.go
• Synthesize findings and post review

---

Summary

The design is clean and well-motivated. The CEL AST walk is the right approach — computing param references at compile time rather than runtime avoids any overhead on the hot evaluation path, and using PostOrderVisit means nested access (params.body.model) correctly folds into a top-level "body" reference. Thread safety in Engine.RequiresBody (read lock + map lookup + delegate) is correct. Test coverage is thorough: nil-program, alias indirection, index vs. dot-select, and an end-to-end deny case.

Three findings, one of which is worth fixing before merge:

1. Evaluator.RequiresBody() is O(n) per call, not O(1) as documented (see inline comment)

The PR description and docstring both say the answer is "fixed at compile time" and a "cheap signal", but the implementation iterates all compiled rules on every call. For a gatekeeper checking this on each incoming request, that's a linear scan per request. The fix is trivial — precompute a requiresBody bool in NewEvaluator once the rules loop completes and store it on the struct. Fix this →

2. Nil body semantics should be documented (see inline comment)

When nil is passed as body, body-inspection rules silently don't match (CEL treats nil as a missing value). This is probably the right behaviour but worth one sentence in the docstring so callers don't have to infer it.

3. Optional-select coverage gap (low priority) (see inline comment)

collectParamRefs handles SelectKind and index CallKind but not CEL optional-chaining selects (params?.body). Whether this is a gap depends on how your CEL version AST-encodes optional selects. Adding a test case would confirm it one way or the other.

---

Overall this is solid work — the only change I'd call necessary before merge is the O(1) precompute fix, since the current implementation silently contradicts its own documented guarantee.

[claude]

The comment says "fixed at compile time" and the PR description describes this as a "cheap signal", but RequiresBody currently iterates all rules on every call — it's O(n) per call, not O(1). For a gatekeeper checking this on every incoming request, that's a linear scan per request instead of a map lookup.

Since the answer genuinely is fixed at compile time, it should be precomputed. For example, store a requiresBody bool field on Evaluator and set it in NewEvaluator once all rules are compiled:

// in NewEvaluator, after the rules loop:
requiresBody := false
for _, cr := range compiled {
    if cr.program.ReferencesParam("body") {
        requiresBody = true
        break
    }
}
// then: ev.requiresBody = requiresBody

func (ev *Evaluator) RequiresBody() bool {
    return ev.requiresBody
}

This also lets the docstring accurately say "O(1) lookup".

[claude]

Passing a nil body sets params["body"] = nil (typed nil map). CEL expressions like params.body.model == 'gpt-4' will then fail with a "no such key" or nil-navigation error, which the engine suppresses to false — so the rule won't match. This is likely the desired behaviour but it's a silent semantic difference from body being absent entirely (i.e. calling NewHTTPCall).

Worth adding a sentence to the doc comment that makes this explicit: "Passing a nil body means body-inspection rules will not match, since CEL treats nil as a missing value." That way callers understand the consequence of buffering but finding no body to parse.

[claude]

Minor: CEL also has an optional-chaining select (params?.body, represented as SelectKind with optional=true in some CEL versions, or as a distinct OptSelectKind). If your CEL version exposes SelectKind for optional selects too, the current check already covers it. If it uses a separate kind, it would be silently missed. Worth verifying, or at minimum adding a test case {"optional select", "params?.body == 'x'", "body", true} — if it fails you'll know there's a gap to fill.

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @dpup's task in 4m 22s —— View job

---

Code Review

• Read changed files
• Review internal/cel/env.go (AST reference analysis)
• Review internal/engine/eval.go (RequiresBody implementation)
• Review helpers.go and helpers_test.go
• Review keep.go, keep_test.go, and docs
• Synthesize findings and post review

---

Summary

The opaque-access fail-safe (introduced in the latest commits) is well-designed and well-tested — detecting size(params), dyn(params).body, computed index keys, and correctly NOT marking single-field accesses as opaque. The alias-resolution-before-analysis approach is exactly right. Test coverage across refs_test.go, requires_body_test.go, and the end-to-end case is solid.

Two of the three findings from the previous review remain open.

1. O(n) scan — still not fixed (see inline comment)

Evaluator.RequiresBody() still iterates all compiled rules on every call. The docstring, keep.go's comment, and the guide all say the answer is "fixed at compile time", but the implementation doesn't honour that. The fix — add a precomputed requiresBody bool field set once in NewEvaluator — is four lines and was already spelled out in the first review.

2. size(params.body) nil-guard advice is inaccurate (see inline comment)

The helpers.go docstring suggests size(params.body) as an alternative to has() for guarding against nil. size(nil) produces a CEL type-error (swallowed to false) — it's not a nil test, it tests size of a non-nil value. The correct advice is params.body != null only.

3. Optional dot-select test still missing (see inline comment) — minor

operators.OptIndex is now handled for params?["body"], but there's no test for params?.body. One table row would confirm it either works (via SelectKind's optional flag) or reveal the gap.

[claude]

Still O(n) per call — iterates all rules on every RequiresBody() invocation. The docstring (and keep.go's doc + the guide) all say the answer is "fixed at compile time", but the implementation doesn't precompute it.

For a gatekeeper calling engine.RequiresBody(scope) on every incoming request, this is a linear scan over all rules every time. The fix is trivial: add a requiresBody bool field to Evaluator, set it once in NewEvaluator after the rules loop, and return it directly:

// in NewEvaluator, after `compiled` is fully built:
requiresBody := false
for _, cr := range compiled {
    if cr.program.ReferencesParam(ParamBody) {
        requiresBody = true
        break
    }
}

// store on the struct, then:
func (ev *Evaluator) RequiresBody() bool {
    return ev.requiresBody
}

This was flagged in the previous review (#discussion_r3438035577) but wasn't addressed in the follow-up commits. The commit message fix(cel): fail safe on opaque params access; resolve review findings suggests it was meant to be fixed.

[dpup]

Fixed in d920432 (pushed just after this review run): `RequiresBody` is now precomputed once in `NewEvaluator` via `computeRequiresBody` and stored on the `Evaluator`, so it is an O(1) field read per request. Docstrings updated to say so.

[claude]

The size(params.body) suggestion is inaccurate as a nil-guard. size(nil) produces a CEL type-error (swallowed to false), so size(params.body) > 0 would happen to return false for a nil body, but for the wrong reason — it's not a presence test, it's a size test that errors. It also returns false for an empty-but-present body ({}), which is a different semantic.

The nil-guard advice should just be params.body != null. Suggest updating the docstring:

Suggested change

	return call
	// body is the decoded request body and may be any JSON-shaped value: an object
	// (map[string]any), an array ([]any), or a scalar. It is stored as-is, so a nil
	// body still sets params.body (to nil) — meaning has(params.body) is always
	// true for calls built with this helper; test for a populated body with
	// params.body != null instead. Use Engine.RequiresBody to decide whether
	// buffering and parsing the body is worthwhile before calling this.

[dpup]

Applied in 5db6a91. The inaccurate `size(params.body)` hint is gone; the doc now recommends `params.body != null`. Verified: with a nil body it evaluates false, with a populated body true, and with an absent body false.

[claude]

The previous review flagged optional dot-select (params?.body) as an uncovered case. operators.OptIndex is now handled for the index form (params?["body"]), but there's still no test for the dot-select form. Worth adding one case to confirm coverage (or document if it's deliberately excluded):

{"optional dot select", "params?.body == 'x'", "body", true},

If CEL represents params?.body via SelectKind (with an optional flag), the existing SelectKind handler covers it and the test will pass. If it's a different AST kind, the test will fail and reveal the gap. Either way, the test documents the behaviour.

[dpup]

Optional dot-select (params?.body) and optional index (params[?"body"]) do not compile in Keep's CEL env — OptionalTypes is not enabled, so both are syntax errors and can never reach the walker. Adding the suggested test would fail at Compile, not at detection. The OptIndex branch is kept as future-proofing, and if OptionalTypes is ever enabled the new fail-safe opaque path (any unrecognized use of params ⇒ ReferencesParam returns true) covers these forms regardless.

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @dpup's task in 4m 20s —— View job

---

Code Review

• Gather context and read changed files
• Verify previous review findings were addressed
• Review internal/engine/eval.go (RequiresBody precompute)
• Review helpers.go (nil-body docs)
• Review internal/cel/env.go and internal/cel/refs_test.go
• Review remaining files
• Synthesize findings and post review

---

Summary

All three findings from the previous rounds are now resolved. The O(1) precompute, the nil-body doc fix, and the removal of the misleading size() advice are all in place. The opaque-access fail-safe design is solid and well-tested.

Two remaining issues, one of which is worth fixing before merge:

---

1. Doc middleware example silently drops non-object JSON bodies — fix before merge

In the new 06-go-library.md section (Inspecting the request body), the middleware snippet declares body as map[string]any:

var body map[string]any
if engine.RequiresBody(scope) {
    raw, _ := io.ReadAll(r.Body)
    r.Body = io.NopCloser(bytes.NewReader(raw))
    _ = json.Unmarshal(raw, &body)
}
call := keep.NewHTTPCallWithBody(r.Method, r.Host, r.URL.Path, body)

If the request body is a JSON array (e.g. a batch endpoint sending [{"model":"gpt-4"}]), json.Unmarshal(raw, &body) returns a type error and leaves body nil. NewHTTPCallWithBody then sets params.body = nil, so array-inspection rules like params.body[0].model == 'gpt-4' silently fail to match.

The docstring already says body accepts "a JSON object (map[string]any), an array ([]any), or a scalar", so the example should honour that:

var body any
if engine.RequiresBody(scope) {
    raw, _ := io.ReadAll(r.Body)
    r.Body = io.NopCloser(bytes.NewReader(raw))
    _ = json.Unmarshal(raw, &body)
}
call := keep.NewHTTPCallWithBody(r.Method, r.Host, r.URL.Path, body)

Fix this →

---

2. params?.body optional dot-select still untested — minor

operators.OptIndex is handled for params?["body"], but there's no test for the dot-select form params?.body. In cel-go v0.27 this will either be caught precisely via SelectKind (if optional select compiles to that kind) or conservatively marked opaque — so it's never wrong, just untested. Adding one table row would pin the behavior:

{"optional dot select", "params?.body == 'x'", "body", true},

---

What was fixed (confirmed)

Finding	Status
O(n) scan on every RequiresBody() call	✅ Fixed — requiresBody bool precomputed in NewEvaluator, O(1) field return
Nil-body docs: size(params.body) nil-guard advice	✅ Fixed — removed; docstring now accurately describes nil-navigation → false and has() → true
Opaque fail-safe missing	✅ Added — paramsOpaque correctly triggers on size(params), dyn(params).body, computed index keys

[claude]

📦 Previous Review (superseded by newer review)

Claude finished @dpup's task in 4m 42s —— View job

---

Code Review

• Read changed files and understand the PR
• Verify previous review findings were addressed
• Review internal/cel/env.go (AST reference analysis, opaque fail-safe)
• Review internal/engine/eval.go (RequiresBody precompute)
• Review helpers.go and helpers_test.go (nil-body docs)
• Review keep.go, keep_test.go, and docs
• Review internal/cel/refs_test.go and internal/engine/requires_body_test.go
• Synthesize findings and post review

---

Summary

All three findings from the previous reviews are properly resolved:

• O(1) precompute (d920432) — requiresBody bool is computed once in computeRequiresBody() inside NewEvaluator and stored on the struct. RequiresBody() is now a pure field read. ✅
• nil-body docs (5db6a91) — The inaccurate size(params.body) hint is gone; the docstring now correctly recommends params.body != null. ✅
• Optional dot-select test — Clarified: OptionalTypes is not enabled in Keep's CEL env, so params?.body is a compile-time syntax error and cannot reach the walker. The OptIndex branch is intentional future-proofing, and the opaque fail-safe would cover it if OptionalTypes were ever enabled. ✅

The core implementation is solid throughout:

• collectParamRefs correctly tracks total (every bare params ident) vs consumed (every recognized field access) and sets opaque = total > consumed, giving the right fail-safe behaviour for size(params), dyn(params).body, and computed index keys without over-triggering for normal single-field rules.
• PostOrder traversal means nested access like params.body.model only registers body once — the outer .model select has params.body as its operand (a SelectKind, not a bare ident), so isParamsIdent returns false and it doesn't double-count. ✅
• deepLowerStrings correctly recurses into map[string]any and []any body values via lowerValue, so case-normalisation handles every body shape consistently. ✅
• nil.ReferencesParam returns false (nil guard at the top of ReferencesParam), so rules without a when clause correctly don't contribute a body reference. ✅
• Thread safety is correct throughout: Engine.RequiresBody holds the read lock, Evaluator.requiresBody is written once before any concurrent use, and paramRefs is read-only after construction.

One finding worth fixing before merge:

var body map[string]any in the docs middleware example silently drops array bodies — docs/content/guides/06-go-library.md:105

The code snippet is:

var body map[string]any
if engine.RequiresBody(scope) {
    raw, _ := io.ReadAll(r.Body)
    r.Body = io.NopCloser(bytes.NewReader(raw))
    _ = json.Unmarshal(raw, &body)
}
call := keep.NewHTTPCallWithBody(r.Method, r.Host, r.URL.Path, body)

json.Unmarshal(raw, &body) fails silently when the body is a JSON array (e.g. a batch request), leaving body == nil. The PR description and NewHTTPCallWithBody's own docstring both explicitly state that array bodies ([]any) are supported — so the example undercuts that claim. Change to var body any so all JSON shapes are correctly decoded:

var body any
if engine.RequiresBody(scope) {
    raw, _ := io.ReadAll(r.Body)
    r.Body = io.NopCloser(bytes.NewReader(raw))
    _ = json.Unmarshal(raw, &body)
}
call := keep.NewHTTPCallWithBody(r.Method, r.Host, r.URL.Path, body)

Fix this →

Otherwise this is ready to merge.

[claude]

Claude finished @dpup's task in 3m 50s —— View job

---

Code Review

• Read changed files
• Review internal/cel/env.go (AST reference analysis)
• Review internal/engine/eval.go (RequiresBody precompute)
• Review helpers.go and helpers_test.go
• Review keep.go, keep_test.go, and docs
• Check previous review findings are resolved
• Synthesize findings and post review

---

Summary

All three findings from the previous review rounds are resolved. The implementation is solid throughout.

Previous findings — confirmed fixed

Finding	Status
O(n) scan on every RequiresBody() call	✅ requiresBody bool precomputed in NewEvaluator via computeRequiresBody, O(1) field return
size(params.body) nil-guard advice in helpers.go	✅ Removed; docstring now correctly recommends params.body != null
var body map[string]any in docs example silently dropped array bodies	✅ Fixed to var body any in 6cb54ae; error handling added too
Optional dot-select coverage	✅ N/A — OptionalTypes not enabled in Keep's CEL env; params?.body is a compile-time error

Core implementation — verified correct

• collectParamRefs AST walk: PostOrder traversal correctly folds nested access (params.body.model) into a single top-level "body" ref without double-counting. The total > consumed opaque-flag logic fires precisely for unrecognised params uses (size(params), dyn(params).body, computed index keys) without over-triggering for normal single-field rules. CEL macro expansions (has, comprehensions, in operator) are covered because PostOrderVisit visits every child node.
• Nil program guard: ReferencesParam returns false for a nil *Program, so rules without a when clause never falsely contribute a body reference.
• Thread safety: requiresBody is written once before the Evaluator is returned; Engine.RequiresBody holds the read lock correctly; paramRefs is read-only after compile.
• Case normalisation with array/scalar bodies: deepLowerStrings calls lowerValue on the stored body, which already handles []any and string scalars correctly — nothing new needed.
• Fail-safe for unknown scopes: Engine.RequiresBody returns true for unknown scopes, preventing a misconfigured scope name from silently skipping body buffering.

One minor finding

docs/content/guides/06-go-library.md:83 — the informational blockquote uses a bare > but docs/STYLE-GUIDE.md requires a bold label: > **Note:** .... See inline comment for the one-line fix. Everything else is ready to merge.