fix(cel): recognise 'me' as a context identifier (dotted DCI validation false-positive)#342
Draft
gonzalesedwin1123 wants to merge 4 commits into
Draft
fix(cel): recognise 'me' as a context identifier (dotted DCI validation false-positive)#342gonzalesedwin1123 wants to merge 4 commits into
gonzalesedwin1123 wants to merge 4 commits into
Conversation
…iable
TDD red: the DCI resolver rewrites dotted accessors to metric('...', me)
before the base resolver extracts identifiers, and 'me' is missing from
CEL_CONTEXT_IDENTIFIERS, so validate_expression/validate_formula_expression
report 'Undefined variables: me' for valid dotted DCI expressions. New tests
assert me is recognised (base resolver + cel.service formula path + DCI
dotted-accessor end-to-end).
'me' is the individual record proxy in the CEL eval context (bound as
'me': SafeRecordProxy(beneficiary) and treated as a record root in
cel_predicate_guard), and the resolver emits metric('<accessor>', me) for
cached/dotted variables. Because 'me' was missing from
CEL_CONTEXT_IDENTIFIERS, once the DCI override rewrites a dotted accessor
into metric('...', me) before identifier extraction, validate_expression /
validate_formula_expression reported valid expressions as
'Undefined variables: me' (compile_expression was unaffected as it does
not reject missing variables). Add 'me' to CEL_CONTEXT_IDENTIFIERS so it
is skipped as a reserved context identifier in both consumers
(resolver + cel_expression). Bump spp_cel_domain to 19.0.2.1.1.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #342 +/- ##
==========================================
- Coverage 74.28% 69.28% -5.00%
==========================================
Files 372 266 -106
Lines 25385 20612 -4773
==========================================
- Hits 18857 14282 -4575
+ Misses 6528 6330 -198 Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…d set Staff-review nit: the fallback set used when the cel_parser import fails omitted 'me', so the fix would not hold on that (catastrophic) path. Mirror 'me' into the fallback for consistency.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
validate_expression()andvalidate_formula_expression()wrongly rejected valid dotted DCI CEL expressions withUndefined variables: me.Root cause: the DCI resolver override (
cel_variable_resolver_dci.py) rewrites a dotted cached accessor such asr.dci.crvs.is_aliveintometric('r.dci.crvs.is_alive', me)in a top-level pre-pass, before the base resolver extracts identifiers. The base_extract_variable_namesthen lexes that rewritten string and returns the bareme(an IDENT outside the quoted accessor) as a potential variable.mewas missing fromCEL_CONTEXT_IDENTIFIERS(cel_parser.pyhadm,e,rbut notme), so it wasn't in the resolver's reserved words → looked up as a variable → not found → reported as a missing/undefined variable.compile_expression()was unaffected because it does not rejectmissing_variables, which is why enrollment/eligibility worked while validation-only callers (UI checks, API) failed.meis a genuine context identifier, not a typo:spp_programs/models/cel/entitlement_amount_cel.py:299→"me": SafeRecordProxy(beneficiary);spp_cel_domain/services/cel_predicate_guard.py:35→_RECORD_ROOTS = ("r", "me");metric('<var>', me). It previously only ever appeared in replacement output, never in a scanned expression, so only the DCI dotted path tripped.Fix
One line in the source layer: add
"me"toCEL_CONTEXT_IDENTIFIERSinspp_cel_domain/services/cel_parser.py. That set feedsreserved_wordsin both consumers (cel_variable_resolverandcel_expression), so the first-pass filter skipsmeeverywhere. Fixes all callers, not just DCI — chosen over patching the DCI module, which would leave the same latent gap for any other caller that putsmein a scanned expression. Version bumpspp_cel_domain19.0.2.1.0 → 19.0.2.1.1. No migration (a constant change; no schema/data).Tests (TDD)
tests/test_cel_me_identifier.py):meis inCEL_CONTEXT_IDENTIFIERS;expand_expression/validate_expressionon an expression containing bareme(metric('foo', me) == true) does not flag it;cel.service.validate_formula_expressionlikewise. Red before the fix, green after.tests/test_dci_cel_validation.py, test-only, no version bump): the seeded dotted accessorr.dci.crvs.is_aliveexpands tometric('r.dci.crvs.is_alive', me)andvalidate_expression("r.dci.crvs.is_alive == true")is valid with noUndefined variables: me— the exact reported scenario.spp_cel_domain652/652,spp_dci_indicators84/84; ruff/ruff-format clean. Verified nospp.cel.variableis literally named/accessedme(adding the reserved word only removes such a name from variable lookup;mewould be an illegal name anyway since it is the record root).Merge order (spp_cel_domain pair)
Per the cross-PR interaction analysis (2026-07-24,
internal/plans/security-prs-interaction-analysis.md): merge after #272. Both PRs bumpspp_cel_domainto the identical19.0.2.1.1, and that manifest line auto-merges with no conflict — so this PR's post-#272 rebase must deliberately:spp_cel_domainto19.0.2.1.2;### 19.0.2.1.2heading above security(cel): key metric cache lookups strictly by requested params #272's### 19.0.2.1.1;spp_cel_domain/tests/__init__.py;Code paths are provably disjoint (this PR: validation identifier set; #272: execution cache keying) — no semantic interaction.