Skip to content

fix(#77): surface parser policy ids in diagnostics + add id_annotations map#78

Merged
skuenzli merged 4 commits into
mainfrom
fix/issue-77-policy-ids-to-annotations
May 14, 2026
Merged

fix(#77): surface parser policy ids in diagnostics + add id_annotations map#78
skuenzli merged 4 commits into
mainfrom
fix/issue-77-policy-ids-to-annotations

Conversation

@skuenzli
Copy link
Copy Markdown
Contributor

Summary

  • Reverts the 4.8.2 behavior of renaming AuthzResult.diagnostics.reasons entries and ValidationError.policy_id to the @id annotation value, which collapsed identity when multiple policies shared the same @id (e.g. the multi-tenant baseline-permit pattern reported in v4.8.2: diagnostics.reasons collapses tenant identity when two policies share an @id annotation #77). Both surfaces are back to the parser-generated PolicyId (e.g. "policy0").
  • Exposes the @id label via a new parallel id_annotations: Dict[str, str] map on Diagnostics and ValidationResult, keyed by the parser id. An entry is present whenever the policy declares an @id annotation, with the literal value — so @id("foo") contributes "foo", and @id("") / bare @id (which the Cedar docs define as equivalent to @id("")) contributes "". Policies with no @id annotation are omitted from the map.

Why

Issue #77 reported a regression in the diagnostic surface: in 4.8.1, two policies sharing @id("baseline-permit-all") returned distinct reasons ("policy0" vs "policy1"); in 4.8.2 they collapsed to the same string, silently misrouting in callers that used reasons as the lookup key into their own {positional_id → caller_id} map.

This PR keeps the 4.8.2 ergonomics gain — recover the @id label without rebuilding the policy set — while restoring the 4.8.1 disambiguation property.

Shape of the change

result = is_authorized(request, policies, entities)
result.diagnostics.reasons              # ["policy0"]  — parser-generated id, stable per policy
result.diagnostics.id_annotations       # {"policy0": "baseline-permit-all"}  — present iff @id declared

vr = validate_policies(policies, schema)
vr.errors[0].policy_id                  # "policy0"
vr.id_annotations                       # {"policy0": "baseline-permit-all"}

Implementation notes:

  • reason is now typed HashSet<PolicyId> (was HashSet<String>) — JSON serialization unchanged (PolicyId emits a string), but the Rust type is tighter now that the field no longer holds annotation values.
  • lookup_id_annotation replaces resolve_display_id; it is purely an existence check on the @id annotation key with raw &str matching (no PolicySet::annotation identifier-parse cost on the hot path).

Behavior of id_annotations

policy declares id_annotations[pid]
@id("foo") "foo"
@id("") ""
@id (no parens, per docs ≡ @id("")) ""
no @id omitted

Test plan

  • Unit tests rewritten for the new shape; new test_issue_77_multi_tenant_baseline_permit_disambiguates exercises the reproducer from the issue.
  • pytest tests/unit — 63 passed, 2 subtests passed.
  • make benchmark-compare (release-mode build, 5% median / 15% mean gates) — 26 passed, no regression. Largest movement is test_large_entity_set at ~4% faster than baseline; everything else within ±2% on median.

Closes

Closes #77.

🤖 Generated with Claude Code

skuenzli and others added 3 commits May 12, 2026 15:39
Adds the v4.8.2 tag to STATES + states-manifest so HISTORY.md pins
the release commit alongside the development states. Code at v4.8.2
is functionally identical to faf92a4 (PR #75 Path B merge) — the
release-bump commits between them touched only Cargo.toml,
Cargo.lock, README, CHANGELOG, and Makefile / benchmark tooling.
Capture is a documentation milestone, not a new perf data point.

Future: a pre-release / rc tag scheme could pin benchmark capture
points more legibly than ad-hoc PR-merge SHAs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reverts the 4.8.2 behavior of renaming `diagnostics.reasons` entries and
`ValidationError.policy_id` to the `@id` annotation value, which collapsed
identity when two policies shared the same `@id` (e.g. multi-tenant
templated permits). Both surfaces are back to the parser-generated
`PolicyId`. The `@id` label is now exposed via a parallel `id_annotations`
map on `Diagnostics` and `ValidationResult`, keyed by parser id and
populated only for policies that carry a non-empty `@id`.

Closes #77.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously `lookup_id_annotation` collapsed empty `@id` annotations
(`@id` / `@id("")`) to `None`, treating them as absent. That behavior
made sense when the annotation value was being used as a display id in
`reasons`, but now that `reasons` always reports the parser-generated
PolicyId, the map can faithfully echo what the policy declared.

New contract: the map carries an entry whenever `@id` is present, with
the literal value as the map value. Policies with no `@id` annotation
at all are still omitted.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@skuenzli skuenzli self-assigned this May 13, 2026
@skuenzli
Copy link
Copy Markdown
Contributor Author

What do you think about this approach @aashitk and @Iamrodos ?

@Iamrodos
Copy link
Copy Markdown
Contributor

See my commend in #77 suggesting reasons_ids (growing as my preferred) or reasons_as_ids

Renames the map to surface its keying in the property name itself,
following the Pythonic `<values>_by_<key>` convention:

- Diagnostics.id_annotations            -> id_annotations_by_reason
- ValidationResult.id_annotations       -> id_annotations_by_policy_id

Asymmetric on purpose: the diagnostic surface keys by entries in
`reasons`, while the validation surface keys by `errors[*].policy_id`.
Verified the validation key invariant: Cedar's `ValidationError` is an
enum where every (current) variant exposes a total `policy_id()`, and
the only insertion site uses `e.policy_id().to_string()` -- so map keys
are always parser-generated policy ids.

Also adds `test_id_annotations_by_reason_lookup_for_human_readable_diagnostics`
demonstrating the caller-side pattern: iterate `reasons`, look each
parser id up in `id_annotations_by_reason`, format. Mirrors what a CLI
or higher-level library would do to surface `@id` labels in messages
while keeping parser ids as the stable, unique lookup keys.

Rust struct fields and JSON wire keys renamed in lockstep with the
Python properties.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@skuenzli
Copy link
Copy Markdown
Contributor Author

re reasons_ids:

from #77 (comment)

Good approach. As policies as passed, id_annotations could speak to a parsing use case or any number of other things. I would prefer to see reasons in the name so it's linked to what the values are. Potentially reasons_as_ids or reasons_ids.

Thank you. I think including 'reason` in the name makes a lot of sense to connect with related data.

I brainstormed names that would connect the id annotations to authz diagnostic reasons and validate policies in the most Pythonic way and settled on:

  • Diagnostics.id_annotations_by_reason
  • ValidationResult.id_annotations_by_policy_id (this validation result is always for policy validation)

@skuenzli skuenzli merged commit ff5a38d into main May 14, 2026
7 checks passed
@skuenzli
Copy link
Copy Markdown
Contributor Author

Merging to restore unambiguous (policy id) reasons and support improved ergonomics with supplemental Diagnostics.id_annotations_by_reason and ValidationResult.id_annotations_by_policy_id fields.

@skuenzli skuenzli mentioned this pull request May 14, 2026
7 tasks
dannypsnl pushed a commit to dannypsnl/cedar-py that referenced this pull request May 24, 2026
Patch release fixing a behavior regression introduced in 4.8.2.

### Changed

- **Behavior change (partial revert of 4.8.2).** `AuthzResult.diagnostics.reasons` and `ValidationError.policy_id` once again surface the parser-generated `PolicyId` (e.g., `policy0`), restoring the 4.8.1 contract that was relied on for multi-tenant disambiguation. The `@id("...")` annotation value is now exposed in a parallel map keyed by the parser id: `Diagnostics.id_annotations_by_reason` and `ValidationResult.id_annotations_by_policy_id`. Entries are present whenever the policy declares an `@id` annotation, with the literal annotation value as the map value -- so `@id("foo")` contributes `"foo"`, and `@id("")` / bare `@id` (which the Cedar docs define as equivalent to `@id("")`) contributes `""`. Policies with no `@id` annotation are omitted from the map. This keeps the 4.8.2 ergonomics gain (recover the `@id` label without rebuilding the policy set) while preventing identity collapse when two policies share the same `@id` (k9securityio#77, k9securityio#78).

Cedar Policy engine version is unchanged at v4.8.2.
@skuenzli skuenzli deleted the fix/issue-77-policy-ids-to-annotations branch May 25, 2026 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

v4.8.2: diagnostics.reasons collapses tenant identity when two policies share an @id annotation

2 participants