[codex] Add Notary CEL integration - #219
Open
jeremi wants to merge 9 commits into
Open
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces integration with Registry Notary evidence services, adding a pure Python client (spp_notary_client) and an Odoo integration module (spp_notary_evidence) to support CEL external variables. Key changes include scoping cache lookups to specific providers, adding a catalog sync wizard, and implementing live evaluation hooks. The feedback highlights several critical performance and resource management improvements: batching cache misses in _exec_external_metric to avoid N+1 HTTP requests, implementing context manager methods in NotaryClient to prevent connection leaks, and batching database creations in _apply_notary_claim_catalog to reduce database roundtrips.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 19.0 #219 +/- ##
==========================================
+ Coverage 71.50% 71.95% +0.45%
==========================================
Files 1001 1015 +14
Lines 58626 59685 +1059
==========================================
+ Hits 41921 42948 +1027
- Misses 16705 16737 +32
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
jeremi
marked this pull request as ready for review
May 29, 2026 11:21
Member
Author
|
Addressed the Gemini review items and rechecked CI. Changes pushed:
Verification:
I also resolved the Gemini review threads after applying the fixes. |
Member
Author
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces the spp_notary_client and spp_notary_evidence modules to integrate Registry Notary evidence services with Odoo, allowing external CEL variables to resolve via provider-specific refresh hooks and scoped caching. Feedback on these changes highlights critical resource leaks where NotaryClient is instantiated without a context manager, a performance bottleneck in _ensure_cel_variable due to database operations inside a loop, and a potential crash during datetime parsing if datetime.fromisoformat raises an uncaught ValueError.
Member
Author
|
Coverage and Gemini follow-up pushed through Coverage:
Gemini:
Verification:
|
Member
Author
|
Addressed the consolidated review tail in Fixed HIGH correctness/spec items:
Operator/spec polish:
Verification run locally:
Not addressed in this commit: the explicitly acknowledged PR-tail items around single-subject speculative session fetch, alias/adopt UX, group XML-ID naming deviation, and docker-compose e2e PR 8. |
gonzalesedwin1123
requested changes
Aug 24, 2026
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
Member
There was a problem hiding this comment.
Expert review — verification-gap fixes are not complete
Thorough pass over both new modules, the spp_cel_domain changes, and the drift against today's 19.0 (the branch is 516 commits behind and conflicting). Findings below were verified against the code — several by executing it — not taken from the PR description. All suites are green on the branch (spp_notary_client 21/0/0 Odoo + 22 passed/1 clean skip host pytest, spp_notary_evidence 36/0/0, spp_cel_domain 591/0/0), ACLs exist for all new models, and the test quality is genuinely good (real non-admin ACL tests, real executor integration, mocking only at the HTTP boundary). The problems are in what the fixes claim versus what they do, and in gaps the "Remaining gaps" list doesn't mention.
Verdicts on the claimed fixes
spp_notary_client
| Claim | Verdict |
|---|---|
| Context-manager client lifecycle | PARTIAL — with paths correct (incl. injected-client nullcontext), but close() never resets _http_client (reuse-after-close raises raw RuntimeError), and the public _notary_client() helper leaks open httpx pools when used without with |
| Sanitized outgoing logging | VALID for the logging code — but unmapped pydantic.ValidationError escapes to tracebacks with raw subject values (input_value=770123456789), and NotaryError.details["response"] carries whole raw response bodies |
x-api-key default header |
VALID for the notary path — but the "or configured header" half is dead code (reads api_key_header/notary_api_key_header, neither exists on spp.data.provider), and the change to spp_cel_domain._test_connection regresses every existing provider that expected Authorization: Bearer (see Critical 4) |
| Typed errors, no raw leaks | PARTIAL — 5 verified leak paths: httpx.DecodingError, TooManyRedirects, CookieConflict, InvalidURL/ValueError on malformed base_url, RuntimeError after close. Malformed/HTML 200 bodies fail open: _response_payload swallows to {} → valid empty response → "subject has no evidence" |
| Versioned claim refs + batch evaluate | PARTIAL — versioning valid and tested; batching real, but no client-side batch cap (50,000 subjects accepted in one POST), retry_max is passed as "max_retries" and silently ignored, and per-item batch failures are dropped without reading item.errors |
spp_notary_evidence / spp_cel_domain
| Claim | Verdict |
|---|---|
stale_cache_with_audit re-raises original NotaryError |
PARTIAL — the re-raise is real, but per-item batch failures bypass the policy silently; _first_matching_result → None bypasses it; and NotaryConfigurationError sits outside the try in the batch path but inside it in the single path (asymmetric raw traceback) |
| Audit payload keys + secret gate | PARTIAL — keys present, gate real, but evaluation_id is always None in production: the code reads error.details["evaluation_id"] while the client sets details={"response": payload}. The test that "proves" it hand-constructs a details shape the client never produces. Also, the audit write is best-effort (log_call swallows all exceptions, return unchecked) — "with audit" is unenforced |
| Batched external metric cache misses | VALID with caveats — no N+1 left; but the base fallback logs one warning per subject (2,000 per batch), and the external path skips preview_cache_only and async_threshold (Criticals 7, 8) |
| Batched catalog claim creation + preload | PARTIAL — preload misses archived claims (active_test) → guaranteed IntegrityError on re-sync (Critical 5); updates and deactivation are still per-row writes |
| Wizard collision blocking + pinned_version | PARTIAL — rows and pinned_version correct, but the collision block is wizard-UX only: the provider form's action_sync_notary_catalog, create(), and write() all bypass it, and _ensure_cel_variable silently adopts and rewrites any existing variable with the same name. The unique constraints are per-provider, so cross-provider accessor collisions are never blocked at the DB level |
Critical (must fix)
- Batch responses can attribute one subject's claim value to a different subject.
spp_notary_evidence/models/data_provider.py:385falls back to the positionalenumerateindex whenitem.input_indexisNone(it isOptionalin the schema). A server omitting it — or returning items only for succeeded subjects — writes the value to the wrongres.partner, persists it tospp.data.value, and CEL eligibility reads it. The existinginput_index=Nonetest uses a single subject, exactly the case that hides this. Match onitem.subjectidentity; never positional order. - The
idempotency-keyis an unkeyed, reproducible hash of the subject's national ID and never rotates.spp_notary_client/services/client.py:509-531usesuuid5(NAMESPACE_URL, ...)— no secret, so anyone observing the header can confirm a guessed subject ID; and the key is identical for the same (subject, claim, purpose) forever, so any server honouring idempotency replays a cached evaluation indefinitely, silently defeatingexpires_at/TTL. Generate a random per-call key (stable only across_requestretries), or HMAC the seed. - Released
spp_cel_domainis changed with no version bump, no HISTORY fragment, no README regen, and no migration — including a newrequired=Truefield (provider_kind), aread_values(..., params=)signature change, and a_provider_clausesemantics change. Manifest still reads19.0.2.0.0;19.0is at19.0.2.1.0. _test_connectionregressed for every existing provider:spp_cel_domain/models/data_provider.py:334-336switches all providers fromAuthorization: Bearertox-api-key, the config escape hatch reads fields that don't exist (dead code), and the newbearerauth type isn't handled there at all. This is out of the PR's stated scope — split it out or branch onprovider_kind.- Catalog re-sync crashes with an IntegrityError once any claim was archived. The preload
Claim.search()is subject toactive_test, so a claim that disappears and reappears upstream is routed tocreate()and violates the unique constraint. The wizard preview (One2many,active_test=False) meanwhile reports "no change" — preview and confirm disagree. Usewith_context(active_test=False). - Ordinary officers get an
AccessErrorevaluating any notary metric. The evaluation path dereferencesspp.notary.claim(ACL: notary groups only) andprovider.codeas the acting user with nosudo()(data_provider.py:259,287,375-377;cel_executor.py:1328,1333). The non-admin tests all use notary-group users, so they cannot catch this. - Expression preview triggers live upstream disclosures.
_exec_external_metricreturns beforepreview_cache_only_modeis consulted, so previewing an expression ships subject national IDs to the provider even on deployments configured for cache-only previews. - Unbounded synchronous fan-out. The external path iterates the entire base domain with no cap and no queue_job, skipping the
async_thresholdguard the indicator path has — a 1M-record cohort issues ~1,000 sequential HTTP calls inside one web transaction and discloses 1M IDs. - The audited purpose is caller-forgeable.
notary_purposeis read fromcel_cfgor rawenv.context(client-supplied over RPC) and becomes both the upstreamdata-purposeheader and the recorded audit purpose. No allowlist, no binding to consent records — validate against registered purposes, default-deny. - Cross-provider CEL variable hijack.
_ensure_cel_variable(notary_claim.py:175-193) adopts any existing variable by name and rewritessource_type/external_provider_id/notary_claim_id. The slug join is ambiguous (providerx+ claimy_z== providerx_y+ claimz), so a notary manager on provider B can silently repoint provider A's live variable at B'sbase_url. The rename guard exists; an adopt/hijack guard doesn't. UnboundLocalErroron empty cohorts:cache_paramsis assigned inside the batch loop and referenced after it (cel_executor.py:1328/1359); reachable whenever the SQL fast path doesn't short-circuit and the domain yields no ids.- Rebase hazard — Data API exposure. Since #257 (merged 2026-06-30, after this branch's base),
is_data_api_pullablereturns True for exactly the variables this PR generates (source_type='external'+ provider), so cached Notary claim values become retrievable through the generic/Data/pullAPI.spp_dci_indicatorsdeliberately overrides this to exclude its providers;spp_notary_evidenceneeds the same decision made explicitly.
Rebase / coordination (516 commits behind, conflicting)
- Textual conflicts:
spp_cel_domain/models/cel_executor.py,spp_cel_domain/tests/__init__.py. - Semantic drift in the hooked region: upstream now derives
params_hashfromp.params(parameterized metrics) — this PR's external path ignoresp.paramsentirely — and changed theas_root/override_domainmaterialization semantics. - Open PR #272 rewrites the same
_provider_clausefallback this PR edits (same security direction, must be reconciled by hand; whoever lands second re-reviews). spp_cel_domainversion queue:19.0at 2.1.0, #272 → 2.1.1, this PR needs a higher bump plus HISTORY and CI-generated README.
Important (should fix)
- Wrap every
model_validatein a typed error carrying field paths only — pydantic messages currently put subject values into logs (hard no-PII rule). - Malformed/non-JSON 200 responses must be a typed error, not an empty "no evidence" result.
NotaryClientConfigdefaultreprprintsbearer_token,api_key, andsubject_log_secret—field(repr=False)all three.- Per-call timeouts are ignored: the owned client is cached from the first config, and injected clients ignore
timeout_secondsentirely; pass the timeout per-request. base_urlhas no scheme/host validation (http:// and even file:/// normalize fine) and is writable bygroup_cel_domain_manager, who cannot read the bearer token — a repoint-and-exfiltrate path for both the token and subject IDs. Enforce https and consider a host allowlist.- Secrets are plaintext
Charcolumns whilespp.data.credential(encrypted, admin-gated) already exists and is ignored; no entropy validation onnotary_subject_log_secret. - HMAC subject hashes are brute-forceable over the national-ID space for anyone holding the (plaintext) secret; no domain separation between client-log and stale-cache hashes; on unresolvable reg_id the code silently hashes the
res.partnerDB id instead, indistinguishable in the payload. - Batch evaluations write no per-subject audit trail (only
subject_count); one subject without the configured ID type aborts the whole batch; a mid-batch chunk failure discards already-fetched chunks (re-disclosure on retry);retry_maxis dead config andRetry-Afteron 429 is ignored. - Dual cache partitions:
data_evaluatorwrites provider + no params + local TTL (ignoring upstreamexpires_at) while the notary path writes provider + version params +expires_at. Neither satisfies the other — doubled upstream disclosures, and evidence retained past its upstream expiry. - Catalog sync re-activates archived claims and resets operator-set states on every run.
spp.notary.claimstorescompany_idbut has no multi-company record rule; notespp.data.valueis readable bybase.group_user, and notary evidence lands there.- Unbounded external input:
extra="allow"schemas, no catalog/claim-id length caps, slug collisions crash with a raw IntegrityError, and the raw payload is stored wholesale on the wizard. _external_variable_for_metricruns an uncachedspp.cel.variablesearch on every metric evaluation (hot path, external or not),limit=1over an OR with noorder(nondeterministic), and is not company-scoped — useormcache.metrics_infofrom the external path omitsbase/have/stalekeys; asearch_countresult is computed and discarded on every external metric.- 535 lines of pytest-style tests never execute anywhere (no CI pytest job; excluded from the Odoo
__init__) and near-duplicate the Odoo-runner suite — keep one. spp_notary_clientis missingreadme/HISTORY.md; its DESCRIPTION.md is one line (principle asks for structured 25-60 lines);spp_notary_evidence's HISTORY heading level will need checking against the README generator.- A docstring claims "session-scoped batching so per-subject calls amortize to a single upstream HTTP request" while the PR's own Remaining gaps says that isn't implemented — align the comment with reality.
- The description's validation numbers are stale (May): the Odoo suite is now 21/36/591, not 2/23/590.
Suggestions
Add a NotaryServerError for unmapped 5xx; remove the unreachable terminal raise in the retry loop; put error codes, not raw bodies, in details; catch ImportError in the audit wrapper and warn once, not per call; _read_config_value treats 0 as absent; the group_notary_evidence_read group is dead (no ACL/rule/privilege); declare spp_vocabulary directly; _compute_effective_purpose_url and _compute_notary_sensitive_subject_id_type are missing @api.depends; the stale-cache log records a status_code/url implying a call that didn't happen; verify the search_default_group_by_status filter exists in the outgoing-log search view.
What holds up well
The client layer is genuinely Odoo-free; TLS verification stays on and redirects off; no print()/bare except/cr.commit(); the implied_ids audit is clean; Odoo 19 compat is clean (models.Constraint, <list>, Command, chatter); and the tests are real integration tests with non-admin ACL coverage, not mock theatre.
Recommendation
Requesting changes. Criticals 1, 5, 6, and 11 are crashes or data-integrity bugs; 2, 7, 8, 9, 10, and 12 need design-level answers (who may trigger a disclosure, is the purpose trustworthy, is preview a dry run, is cached evidence Data-API-exposed) before this is safe against a real Notary. Critical 4 (the _test_connection change) should be split out of this PR entirely. After that: rebase across the 516 commits coordinated with #272, add the is_data_api_pullable decision, bump spp_cel_domain with HISTORY, and re-run the full validation.
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
3 participants
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
Adds the Notary CEL integration and a follow-up correction pass for the reviewer-identified verification gaps.
What changed
spp_notary_clientsupport for Registry Notary discovery, evaluate, batch-evaluate, typed errors, sanitized outgoing logging, and versioned claim refs.spp_notary_evidenceprovider configuration, claim catalog rows, CEL external variable generation, Notary evaluate hooks, cache writes, stale-cache audit behavior, and catalog sync UX.pinned_versionan actual version string, treats upstream version bumps asversion_drift, and keeps one claim row per provider/external ID.Validation
ruff check spp_notary_client spp_notary_evidencepython -m pytest spp_notary_client/tests -qgenerated20 passed, 1 skipped./spp t spp_notary_clientgenerated2 passed./spp t spp_notary_evidencegenerated23 passed./spp t spp_cel_domaingenerated590 passedbash .claude/scripts/audit-security.sh spp_notary_evidencegenerated0 errors, 0 warningsbash .claude/scripts/audit-security.sh spp_notary_clientgenerated0 errors, 0 warningspython -m compileall -q spp_notary_client spp_notary_evidencegit diff --checkRemaining gaps called out for reviewers
notary_subject_log_secretis not auto-generated at install time. Subject-scoped logged calls now fail safely if it is missing.