Surface structured parse diagnostics from PolicySet.parsePolicies

[jamesmulcahy]

Problem

PolicySet.parsePolicies throws InternalException whose message is format!("Internal JNI Error: {e}") (CedarJavaFFI/src/interface.rs, in jni_failed). For a parse failure that discards everything miette recorded — the source span, the tokens the parser expected, the help text — and because ParseErrors' Display prints only its first error, every subsequent error is lost too.

A policy author writing this:

forbid(principal, Foo::Action::"Read", resource == Foo::Table::"t");

sees only:

Internal error: Internal JNI Error: unexpected token `::`

No location, and no hint that the mistake is a missing action ==. The Rust CLI, on the same input:

× failed to parse policy set
╰─▶ unexpected token `::`
 ╭────
 1 │ forbid(principal, Foo::Action::"Read", resource == Foo::Table::"t");
 ·                      ─┬
 ·                       ╰── expected `!=`, `)`, `,`, `:`, `<`, `<=`, `==`, `>`, `>=`, `in`, or `is`
 ╰────

Measured against cedar-java 4.10.0:

PolicySet.parsePolicies("forbid(principal, Foo::Action::\"Read\", resource);");
// getMessage(): Internal error: Internal JNI Error: unexpected token `::`
// getErrors():  [Internal JNI Error: unexpected token `::`]   // size 1, always

Change

Everything needed already exists in the crate:

• PolicySet::from_str returns ParseErrors, which is IntoIterator<Item = ParseError>
• each ParseError implements miette::Diagnostic
• cedar_policy::ffi::DetailedError already has impl<E: miette::Diagnostic + ?Sized> From<&E> — the impl the validation path already uses

So parsePoliciesJni downcasts ParseErrors and throws a new PolicyParseException extends InternalException carrying List<DetailedError>, the same representation AuthorizationEngine.validate already returns:

PolicyParseException e = ...;
e.getDetailedErrors();
// [message=unexpected token `::`,
//  sourceLocations=[SourceLabel{label="expected `!=`, `)`, `,`, `:`, `<`, `<=`, `==`,
//                   `>`, `>=`, `in`, or `is`", start=21, end=23}]]

Existing catch (InternalException e) blocks are unaffected. If building the richer exception fails for any reason, the generic path is used, so a parse error can never turn into a different kind of failure.

Two message changes, both up for discussion

	before	after
getMessage()	Internal error: Internal JNI Error: unexpected token ...	Internal error: unexpected token ...
getErrors()	[Internal JNI Error: unexpected token ...]	[unexpected token ...]

1. The "Internal JNI Error: " prefix is dropped — it describes the binding rather than the policy, and reading "Internal error" for an ordinary typo suggests a fault in the library rather than something the caller can fix.
2. getErrors() carries one entry per parse error rather than a single entry for the whole document, which is what its plural contract always implied.

Both are independent of the diagnostics and easy to drop if you'd rather keep the strings frozen — happy to revise.

Breaking?

Not to any type or signature: PolicyParseException is a subclass and no existing method changes shape. Callers string-matching on the message text of a parse failure would see the two differences above.

Testing

• 6 new tests in PolicyParseDiagnosticsTests — span covers the offending token, all errors reported not just the first, help text survives where Cedar supplies it, still catchable as InternalException, message/getErrors() strings pinned, valid policy sets unaffected.
• Full CedarJava suite: 68,723 tests, 0 failures.
• cargo test in CedarJavaFFI: 69 passed.

Local FFI builds used cargo build --release --features partial-eval for the host target rather than the cargo zigbuild cross-compile path, since zig was not available in my environment; CI exercises the normal path.

Scope

Deliberately limited to parsePolicies. Policy.parseStaticPolicy, Policy.parsePolicyTemplate, Schema.parse and PolicyFormatter lose detail the same way and would be natural follow-ups — kept out here to keep the change focused, per CONTRIBUTING.

Related: #68 asks for the id of a malformed policy, which is adjacent but stops short of diagnostics.

[jamesmulcahy]

Full disclosure -- I've not written any rust before myself, and Claude helped with this change. Background/motivation is well summarized by Claude above. The TL;DR is that the cedar CLI gives much better error output than the Java API -- and I'm trying to improve the experience through Java so our users can be more meaningful & actionable feedback when they provide an invalid policy.

[jamesmulcahy]

Hi @lianah @mark-creamer-amazon @muditchaudhary -- James from Netflix here, we met a few weeks ago!

Our Cedar usage is going well, but we've run into some UX friction with policy validation that should be improved by this PR. Would appreciate your time in reviewing! Thank you!

[mark-creamer-amazon]

Hey James, I'll take a look today. Thanks for the PR!

[mark-creamer-amazon]

I agree with the direction of this PR, but I do worry about breaking existing consumers here.

1. Removing the Internal JNI Error: prefix could of course technically break a consumer inspecting and branching on this prefix's presence (unless they strip it).
   • I think this is the smaller risk I'd personally be willing to move forward regardless
2. Similarly I've seen at least one downstream consumer that is doing a strict regex match on getMessage(), which would break with this change, e.g. java.util.regex.Pattern.compile("^Internal error: Internal JNI Error: unexpected token ([^]*)$");`. More commonly there's likely other consumers that branch based on what's expected to be a single message, e.g.

   if (e.getMessage().equals("Internal error: Internal JNI Error: failure a")) {
       ...
   } else if (e.getMessage().equals("Internal error: Internal JNI Error: failure b")) {
       ...
   }
   

   I think this class of issue class is enough where
3. While getErrors does return a List and implies plurality, all InternalException's from the FFI thus far have been using the basic constructor, which only populates one element. But as getErrors() returns a List, I'll concede that perhaps it was never safe for a consumer to expect .size() == 1 or refer solely to errs.get(0) as the only element. I personally think that us fully populating the getErrors() result list is the right direction.

I think I'm hesitant towards the getMessage() behavior change, but I'm fine with getErrors() changing to return the other errors.

[jamesmulcahy]

I agree with the direction of this PR, but I do worry about breaking existing consumers here.

[...]

I think I'm hesitant towards the getMessage() behavior change, but I'm fine with getErrors() changing to return the other errors.

Understood, I think that's a reasonable take. I'll update the PR to conform with your guidance. Thanks for the review!

[jamesmulcahy]

@mark-creamer-amazon — Claude Code here, posting as James's agent; he's asked me to follow up on your review and has reviewed this reply before it went out.

Two commits pushed, branch now at ffdf01b.

getMessage() is unchanged

Point 2 taken in full — the message is now byte-for-byte what the generic error path produced, prefix and all. Your example pattern

Pattern.compile("^Internal error: Internal JNI Error: unexpected token `([^`]*)`$");

still matches, and it can't be widened by a document with several errors: ParseErrors' Display prints only the first error, and the message is still exactly that, so populating the list doesn't leak into the string. There's a test pinning the literal value now (messageIsUnchangedForBackCompat) — it asserts the inverse of what the earlier revision asserted, so the regression you flagged is now guarded rather than introduced.

Mechanically: PolicyParseException takes the message as a constructor argument, and the Rust side has a single internal_error_message() helper used by both the enriched path and the plain InternalException fallback, so the two can't drift apart. InternalException gained a protected constructor that sets message and error list independently — the existing constructors derive the message by joining the list, which is what made a per-error list necessarily widen the message. Existing constructors are untouched.

The one point I'd like your explicit call on: the prefix on getErrors() entries

getErrors() now returns one entry per parse error, per your point 3. What's still a judgement call is what each entry contains. As pushed:

getMessage() -> "Internal error: Internal JNI Error: unexpected token `::`"   (unchanged, prefix retained)
getErrors()  -> ["unexpected token `::`", "unexpected token `}`"]             (bare, no prefix)

To be explicit: the original error message keeps the prefix exactly where it has always been. The prefix is absent only from the individual list elements. The reasoning is that "Internal JNI Error: " describes the binding rather than any one parse error, so once the list is per-error, repeating it on every element reads as though each error were its own separate JNI failure.

Your point 1 said you'd accept removing the prefix as the smaller risk, but that was said about getMessage(), and I don't want to assume it carries over to the list. The conservative alternative is to prefix element 0 only — preserving the exact former value of getErrors().get(0) for anyone reading just that element, leaving 1..n bare. Strictly non-breaking, though incoherent as a contract.

Does the bare-element version match your expectations, or would you prefer element 0 keep the prefix? Either is fine by us; it's a small change and we'd rather have your explicit sign-off than guess.

Also in this push

• getDetailedErrors() returns Cedar's miette diagnostics per error — message, source span, expected tokens, help text. This is where the new information lives now that the message is fixed.
• Javadoc on DetailedError.SourceLabel documenting that the spans are UTF-8 byte offsets, not String indices. Worth calling out because the obvious source.substring(start, end) throws StringIndexOutOfBoundsException on any policy text containing a non-ASCII character, which we hit while testing. It also records that offsets are absolute within the whole parsed text rather than per-policy, that a span can be empty (an unterminated string literal produces one), and that the span covers the unexpected token — which for a missing operand is the token that followed it, sometimes a line later.
• assertDoesNotThrow import nit from your inline comment.
• A checkstyleMain whitespace violation this PR had introduced in PolicyParseException — that was failing the build and I should have caught it earlier.

Verified locally: the 6 PolicyParseDiagnosticsTests pass, javadoc and checkstyleMain are clean, and the full suite's failure count is identical to the pre-change baseline on this tree (58, all in SharedIntegrationTests and unrelated to parsing — happy to share the before/after if useful).

Thanks again for the careful review — the getMessage() objection was the right call and the change is better for it.

🤖 Generated with Claude Code

[jamesmulcahy]

@mark-creamer-amazon I pushed a fix for the CI failure; if you can approve again I'm optimistic we'll get a clean run, but I'll keep an eye on it!

[muditchaudhary]

Note to myself: CHANGELOG.md in main is stale. The features under "Unreleased" are now released in CedarJava 4.8. I will update it and also place this change under the correct section.

[muditchaudhary]

Regarding removing the prefix in getErrors, it seems fine to me. @mark-creamer-amazon what do you think? IIRC Cedar does not treat mutating error messages as a breaking change.

[muditchaudhary]

throw_internal can be reached with a Java exception already pending: every ? in build_parse_exception leaves one parked when it fails, although this is rare (E.g., new_object -> NoClassDefFoundError if the .jar predates the .so, new_string/new_object_array -> OutOfMemoryError).

In these cases throw_new then returns Err(Error::JavaException) because the crate correctly refuses to call JNI in that state, and .unwrap() panics which I believe would abort the JVM.

One fix would be to reuse jni_failed, which already guards on exception_check() and formats the identical message or add env.exception_check().unwrap_or_default() check here.

Rest of the PR looks good to me. Thank you for working on this!

[jamesmulcahy]

Good catch — you're right, and thank you for tracing it so precisely. Fixed in 129ef7d.

Confirming the chain for the record: each ? in build_parse_exception bottoms out in a jni-rs call whose jni_non_null_call!/jni_non_void_call! macro ends in check_exception!, which returns Error::JavaException while leaving the exception pending — it detects, it doesn't clear. throw_internal then resolves its class argument through Desc::lookup → find_class, which hits that same check and returns Err before ThrowNew is ever reached. The .unwrap() panics, and since jni_fn expands parsePoliciesJni to an extern "C" function, unwinding across that boundary aborts the process. So a recoverable NoClassDefFoundError from a .jar/.so version skew — the realistic trigger — took the JVM down.

I went with the exception_check() guard inside throw_internal rather than reusing jni_failed, because jni_failed's signature isn't quite the right fit here: it takes &dyn Error and returns a jvalue, neither of which this call site wants (it's already holding a &ParseErrors, and it's on a path that returns ()), so routing through it needed more adaptation at the call sites than the guard itself costs. Putting the guard in throw_internal rather than at the two call sites also covers the env.throw(exception).is_err() branch — benign today, since throw on an already-constructed JThrowable does no checked lookup, but it would be a latent instance of the same bug. Entirely happy to switch to jni_failed if you'd prefer the single implementation; it's a small change either way.

One note on the resulting behaviour, which I think is a slight improvement beyond the crash fix: returning early means the pending OutOfMemoryError/NoClassDefFoundError propagates to the caller when the native method returns, rather than being replaced by an InternalException whose message names a parse error. The original error is much more diagnostic for that class of failure. I've written that reasoning into the comment on the guard, along with why the unwrap below it is sound once the guard is in place — the existing "only valid move is ending the process" comment was inherited from jni_failed, where the guard is precisely what justifies it.

Testing

• cargo test --features partial-eval: 71 passed, 0 failed.
• cargo fmt --check: clean. The two build warnings (unused variable: warnings, method get is never used) are pre-existing on main.
• ./gradlew check: clean, SpotBugs and Checkstyle included.
• Full CedarJava suite: 68,030 tests, 58 failures — all in SharedIntegrationTests, all the same unable to find an applicable action given the policy scope constraints assertion. I re-ran with this commit reverted and got an identical 58, and the branch touches no corpus resources or validation code, so they're pre-existing in my environment rather than anything from this PR. (My earlier "68,723 tests, 0 failures" was from an environment where that corpus resolved differently; CI is the authority here.)

Same local-build caveat as before: host cargo build --release --features partial-eval with the .so staged manually, as zig isn't available to me for the zigbuild cross-compile path.

I'll leave the CHANGELOG placement to you as you noted, and the getErrors() prefix question with you and @mark-creamer-amazon — happy to restore the prefix if you'd rather keep those strings frozen.

[jamesmulcahy]

Yikes, Claude's just way too verbose here -- my apologies. I'll vet it more closely next time. I asked it to mention two things, and it's written a thesis 🤦