Fix abort when a property call argument fails to resolve

[yusuftor]

Summary

Root-cause fix for superwall/Superwall-iOS#500: a malformed audience filter authored in the dashboard (e.g. daysSince(app_install) — unquoted argument, i.e. an undeclared reference) hit an .unwrap() in the computed/device property closure (src/lib.rs:491). With panic = "abort" in the release profile, that panic called std::process::abort() and killed the host app during launch preloading — an unrecoverable crash loop, uncatchable from Swift/Kotlin, re-triggered on every relaunch because the same config is refetched.

Changes

1. Propagate argument-resolution errors instead of unwrapping. The property-call closure now collects resolved args into a Result and returns the ExecutionError. execute_with already degrades Undeclared reference errors to Null, so the filter simply doesn't match — the Swift/Kotlin layers already handle that correctly.
2. Make panics catchable and catch them at the FFI boundary. Release profile switched to panic = "unwind" (and the -Zbuild-std lists in build_ios.sh from panic_abort to panic_unwind). All four uniffi entry points (evaluateWithContext, evaluateAstWithContext, evaluateAst, parseToAst) now wrap their bodies in catch_unwind, returning {"Err": "Expression evaluation panicked: …"} instead of aborting the host process. This is required inside the Rust bodies: the udl declares these as plain string-returning, so a panic reaching uniffi's scaffolding would still fatal-error on the Swift side. Likely also covers the panic class in iOS crashed #48.
3. parseToAst returns a serialized error for unparseable input instead of panicking. Success shape unchanged.
4. Version → 1.0.15 (1.0.14 is skipped: superscript-ios-next already has a 1.0.14 tag, and its release workflow resolves the version from this Cargo.toml and skips existing tags — 1.0.14 would silently not publish).

Tests

Six new tests in cargo test --lib (114 pass), including the exact incident filter mirrored from the SDK's execution context:

• daysSince(app_install) >= 1, device.daysSince(app_install) >= 1, and (size(device.activeEntitlements) == 0) && (daysSince(app_install) >= 1) → {"Ok":{"type":"Null"}} (no abort)
• control: daysSince("app_install") >= 1 still resolves via the host → true
• panic guard → {"Err": …}; parseToAst("daysSince(") → {"Err": …}

Note: the three targets under tests/ (integration_tests, coverage_tests, display_tests) don't compile on clean master either (they use use super::*; and private types) — pre-existing, untouched here.

Notes for review

• The nightly -Zbuild-std watchOS/visionOS builds aren't verifiable locally; the dispatch-triggered CI run of build_ios.sh after merge is the real check for those targets.
• Expect a small binary-size increase from unwind tables — the cost of making panics catchable.
• Merging to master auto-dispatches builds to Superscript-iOS (legacy), superscript-ios-next, and Android — merge is effectively release.
• Downstream after ios-next publishes 1.0.15: bump the .exact pin in Superwall-iOS Package.swift/podspec (and the Android equivalent). The dashboard-side hardening suggested in [BUG] Crash on launch due to misconfigured filter Superwall-iOS#500 (validate expression_cel on save, property-picker free-text commit) is separate work.

🤖 Generated with Claude Code

[yusuftor]

@pullfrog

[pullfrog]

Important

The root-cause fix is correct and well-tested, but two things deserve a decision before merge: the catch_unwind safety net is inert on the wasm32-unknown-unknown artifact published from this same repo, and the nightly watchOS/visionOS panic_unwind legs cannot be exercised until after merge — which is also the release.

Reviewed changes — full diff of ddf39a3 (4 files) plus the surrounding execute_with / prop_for code, the build scripts, and the release workflows. I ran cargo test --lib locally: 114 pass.

• Argument-resolution errors propagate instead of panicking — the computed/device property closure now collects args into Result<Vec<_>, ExecutionError> and ?-propagates (src/lib.rs new 530-537). Verified against cel-interpreter 0.8.1: a closure's ExecutionError is returned verbatim by Value::resolve's FunctionCall arm (no FunctionError wrapping), so ExecutionError::UndeclaredReference reaches the error_msg.contains("Undeclared reference") degrade in execute_with and yields Null. The happy path is bit-for-bit equivalent, and both old and new code short-circuit on the first argument in the same order.
• recovering_from_panics guard on all four FFI entry points — the cel.udl namespace exports exactly these four, so coverage is complete; the _impl split keeps the bodies unchanged. Payload downcast handles both &str and String panic payloads.
• Release profile panic = "abort" → "unwind", with -Zbuild-std lists updated — Cargo.toml new 41-43 and build_ios.sh new 98/101/105/108/110.
• parse_to_ast returns {"Err": …} instead of .unwrap()-panicking — success shape is unchanged (bare serialized AST), and the doc comment was updated to match.
• Version → 1.0.15, CHANGELOG entry — 1.0.14 skipped, with the reason recorded as a Cargo.toml comment.

On test quality: of the six new tests, the three undeclared_reference ones are genuine regression tests (restoring the .unwrap() turns {"Ok":{"type":"Null"}} into the guard's {"Err": "Expression evaluation panicked: …"}). The other three are a happy-path control and unit coverage for the two new/rewritten helpers — useful, but not regression coverage for the .unwrap() itself.

⚠️ The catch_unwind net does not protect the WASM/npm artifact, but the changelog promises it does

wasm32-unknown-unknown hard-codes panic-strategy: abort in its target spec, and Cargo silently drops a profile's panic = "unwind" for targets that can't unwind rather than erroring. So on the JS/npm distribution built from this same repo, recovering_from_panics cannot catch anything and a panic still traps the module. The root-cause fix (the ?-propagation) does apply there, so the #500 class is genuinely fixed on every target — it's the belt-and-braces layer and the changelog's "any future evaluator panic is returned as an {"Err": ...} result instead of killing the host process" (CHANGELOG new line 8) that don't hold for WASM consumers.

Technical details

# `panic = "unwind"` is a no-op on `wasm32-unknown-unknown`, so the FFI panic guard is inert there

## Evidence
Independently reproduced on this branch with the repo's own stable toolchain (1.97.1):

```
$ rustc --target wasm32-unknown-unknown -C panic=unwind --crate-type cdylib probe.rs
error: the crate `panic_unwind` does not have the panic strategy `unwind`
```

`cargo build --lib --release --target wasm32-unknown-unknown -v` still succeeds and emits **no** `-C panic=` flag at all — Cargo omits it silently, per <https://doc.rust-lang.org/cargo/reference/profiles.html#panic> ("the actual value depends on the default of the target platform"). Reaching real unwind support on this target requires nightly `-Zbuild-std`, per <https://doc.rust-lang.org/rustc/platform-support/wasm32-unknown-unknown.html#unwinding>; neither `build_wasm.sh` nor `.github/workflows/build-test-PR-superscript-npm.yml` (stable, no `-Zbuild-std`) does that.

## Affected sites
- `CHANGELOG.md:8` — "guards all FFI entry points … so any future evaluator panic is returned as an `{"Err": ...}` result instead of killing the host process". Untrue for the npm/WASM package.
- `Cargo.toml:41-42` — "Must stay \"unwind\": the FFI entry points rely on `catch_unwind` so a panic … can't abort the host app." Correct for the uniffi targets, misleading as a blanket statement.
- `src/lib.rs:60-65` — same wording in the `recovering_from_panics` doc comment.
- `src/lib.rs` ~402-420 (pre-existing, not in this diff) — the `#[cfg(target_arch = "wasm32")]` `prop_for` still has two `.expect("Failed to serialize args …")` calls that will trap the module rather than surface an `Err`. Listed for context, not as something this PR must fix.

## Required outcome
- The user-facing claim matches reality: panics are caught on the uniffi (iOS/Android) targets; the WASM target still traps on panic and is protected only by the error-propagation fix.

## Suggested approach (optional)
- Scope the CHANGELOG bullet and the two Rust/Cargo comments to the uniffi entry points, and note that `wasm32-unknown-unknown` is abort-only so panic-freedom there depends on not panicking rather than on catching.

## Open questions for the human
- Is a follow-up wanted to close the WASM gap — either `-Zbuild-std` for the wasm build, or replacing the remaining `.expect(...)`/`.unwrap(...)` calls on the wasm path with error returns? The npm CI job builds and smoke-tests the bundle but never feeds it a malformed expression, so this gap is currently invisible to CI.

⚠️ The riskiest part of this change cannot be verified before merge, and merge is the release

trigger-supercel-ios.yml fires only on push: branches: [master] — there is no workflow_dispatch — so the four nightly -Zbuild-std … panic_unwind legs are first exercised by the dispatch that is the release. The evidence says this should work (none of the watchOS/visionOS target specs override panic_strategy, so they already default to unwind, and panic-unwind is a default -Zbuild-std-features value), but arm64_32-apple-watchos is tier 3 with no published precedent for panic_unwind and a history of unwind-symbol link failures (rust-lang/rust#103508).

Technical details

# No pre-merge validation path for the nightly Apple `panic_unwind` builds

## Affected sites
- `build_ios.sh:98,101,105,108,110` — `panic_abort` → `panic_unwind` on the visionOS and watchOS `-Zbuild-std` invocations. Unverifiable on Linux; the local toolchain is stable rustc 1.97.1 with no Xcode SDKs.
- `.github/workflows/trigger-supercel-ios.yml` — `on: push: branches: [master]` only, so the downstream `build_ios.sh` run happens after merge.

## What was checked
- rustc target specs for `arm64_32-apple-watchos`, `armv7k-apple-watchos`, `aarch64-apple-watchos-sim`, `x86_64-apple-watchos-sim`, `aarch64-apple-visionos{,-sim}` and the shared `spec/base/apple/mod.rs`: none set `panic_strategy`, and `TargetOptions::default()` is `PanicStrategy::Unwind`. So the profile setting is not fighting the target.
- `library/std/Cargo.toml`: `panic_abort` is a non-optional dep and `panic_unwind` sits behind the `panic-unwind` feature, which <https://doc.rust-lang.org/cargo/reference/unstable.html#build-std-features> lists as a default `-Zbuild-std-features`. The crate-list swap in `build_ios.sh` is therefore consistent but largely documentary — `-Cpanic=unwind` from the profile is what actually decides.
- `library/panic_unwind` / `library/unwind` exclude only `os=none`, `uefi`, `espidf`, `nvptx64`, `avr`; no Apple OS is excluded.

## Required outcome
- The full xcframework (all slots, including `arm64_32-apple-watchos` and `x86_64-apple-watchos-sim`) is known to build and link with `panic_unwind` before the change reaches consumers.

## Suggested approach (optional)
- Run `./build_ios.sh` once on a macOS machine from this branch, or add `workflow_dispatch` to `trigger-supercel-ios.yml` (with a ref input) so the downstream build can be exercised pre-merge.
- Worth capturing the resulting `.a` sizes while you're there, since the PR expects growth from unwind tables and there is no size gate in CI to catch a surprise.

ℹ️ A malformed dashboard filter now fails silently, with no signal to anyone

The abort was awful, but it was also the reason #500 was found. After this change a filter like daysSince(app_install) >= 1 evaluates to {"Ok":{"type":"Null"}}, indistinguishable at the SDK boundary from a legitimately-null result, so the audience silently never matches. The PR explicitly defers dashboard-side validation of expression_cel to separate work, which leaves no path by which a broken filter gets noticed at all.

Technical details

# Degrading to `Null` loses the only existing signal that a filter is malformed

## Affected sites
- `src/lib.rs` new 530-537 — argument-resolution failures now flow into the pre-existing `error_msg.contains("Undeclared reference")` → `Ok(Value::Null)` degrade in `execute_with`, which is indistinguishable from a genuine `Null`.

## Required outcome
- A decision on whether "degraded because the expression was malformed" needs to be observable somewhere, rather than an unconditional silent non-match.

## Open questions for the human
- Is the intent that dashboard-side validation is the only detection mechanism, or should the Rust layer emit something distinguishable (a log line via the host context, or a distinct result shape) so a bad `expression_cel` in production is discoverable? Note the `catch_unwind` path is fine here — it returns a message — it is specifically the `Null` degrade that is silent.

ℹ️ Nitpicks

• wasm/Cargo.toml:22 still sets panic = "abort", which Cargo ignores for non-root workspace members (warning: profiles for the non root package will be ignored shows up on every build). It now visibly contradicts the root profile, so the new Cargo.toml comment's "keep these in sync" instruction points at an incomplete set — deleting the dead block, or naming it in the comment, would keep that instruction honest.
• The reason 1.0.14 is skipped lives only in a Cargo.toml comment; CHANGELOG.md jumps 1.0.15 → 1.0.13 with no explanation. A one-line note in the changelog would save the next person the archaeology.

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}

[pullfrog]

ℹ️ One observation on the force-push. The two prior ### ⚠️ concerns are unaffected by it and still stand.

Reviewed changes — range-diff of ddf39a3 → 7739ad1. The only delta is in Cargo.toml: the two-line comment above version = "1.0.15" explaining why 1.0.14 was skipped has been removed. The version bump, the panic = "unwind" switch and its comment, build_ios.sh, src/lib.rs and CHANGELOG.md are byte-identical to what I reviewed.

ℹ️ The skipped 1.0.14 is now unexplained anywhere in the repo

Dropping the comment removes the only record of why the version jumps 1.0.13 → 1.0.15. CHANGELOG.md has no 1.0.14 entry either, so the next person to touch the version — or anyone auditing why a tag is missing from the release history — has nothing to go on but the superscript-ios-next tag list. If the comment was cut for tidiness, a one-line ## 1.0.14 — skipped, see … note in the changelog would preserve the reasoning without cluttering the manifest.

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}

[pullfrog]

ℹ️ No critical issues — one accuracy nit inline, plus one coverage gap below.

Reviewed changes — no prior pullfrog review exists on this PR, so the whole branch was reviewed (1 commit, 4 files) rather than a delta. cargo test --lib passes locally at 7739ad1 (114 passed, 0 failed).

• Propagated argument-resolution errors — the computed/device property closure in execute_with collects resolved args into Result<Vec<_>, ExecutionError> and ?-propagates instead of .unwrap()ing, so daysSince(app_install) becomes an Undeclared reference error that the existing downgrade turns into Null.
• Switched the release panic strategy to unwind and updated the five -Zbuild-std lists in build_ios.sh from panic_abort to panic_unwind.
• Guarded all four uniffi entry points with a new recovering_from_panics helper that serializes a caught panic to {"Err":"Expression evaluation panicked: …"}.
• Made parse_to_ast return a serialized error on parse failure instead of .unwrap()ing it; the success shape (bare AST JSON) is unchanged.
• Bumped the crate to 1.0.15 and added the matching CHANGELOG section.
• Added six --lib tests — the incident filter in three shapes, a quoted-argument control that still resolves through the host, the panic guard, and parse_to_ast("daysSince(").

Worth recording since it is load-bearing and non-obvious: cargo only emits -C panic=<x> when <x> is not unwind, so this diff removes the previous -C panic=abort and lets each target's spec default apply. Every Apple target in build_ios.sh and every Android target in build_android.sh defaults to unwind, so catch_unwind genuinely becomes live on those platforms.

ℹ️ Nothing in CI runs the Rust tests, so both the new regression tests and the panic strategy are unenforced

The four workflows only build the wasm/npm package and dispatch downstream releases — none invokes cargo test, so the six new tests run on developer machines only. Since merging to master auto-dispatches the iOS and Android builds, a later edit that restores panic = "abort" (or drops recovering_from_panics) would silently re-arm the abort-on-panic crash with no check failing.

Technical details

# The abort-safety property has no automated enforcement

## Affected sites
- `.github/workflows/` — `build-test-PR-superscript-npm.yml`, `build-test-publish-superscript-npm.yml`, `trigger-supercel-ios.yml`, `trigger-supercel-android.yml`. None runs `cargo test`; the only cargo invocations are inside `build_wasm.sh`.
- `Cargo.toml:41` — `panic = "unwind"` is now load-bearing for the whole fix, but only a code comment protects it.
- `src/lib.rs:1688-1691` — `test_panic_guard_converts_panic_to_err_json` passes under the dev profile (unwind by default) even if the release profile is switched back to abort, so it cannot detect that regression.

## Required outcome
- CI fails if the `--lib` tests break.
- CI fails if the shipped profile stops unwinding.

## Suggested approach
- Add a `cargo test --lib` step to the PR workflow (or a new Rust workflow).
- Note that `cargo test --release` would *not* catch a panic-strategy regression: cargo drops `-C panic` for test targets, so the release profile's `panic` value never reaches the test binary. A compile-time guard is the direct check:

```rust
#[cfg(all(panic = "abort", not(target_family = "wasm")))]
compile_error!("cel-eval requires panic = \"unwind\": the FFI entry points rely on catch_unwind");
```

  The `not(target_family = "wasm")` exclusion is required because `wasm32-unknown-unknown` declares `"panic-strategy": "abort"` in its target spec and cannot unwind.

## Open questions for the human
- Is the absence of a `cargo test` job deliberate (run elsewhere), or just never added?
- The three targets under `tests/` don't compile on clean `master` either. Are they slated for repair or deletion? Until one happens, a `cargo test` job would need `--lib` scoping.

ℹ️ Nitpicks

• tests/integration_tests.rs:289 disables a test with the comment "Temporarily disabled due to parse_to_ast panicking on some invalid expressions" — that reason no longer holds after this change, though re-enabling is blocked on making that file compile again.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}

[pullfrog]

✅ No new issues found.

Reviewed changes — range-diff of 7739ad1 → ac6cc18, plus the current state of the touched files. The delta is documentation and dead-config removal; no evaluator, FFI, or build-flag behavior changed.

• Scoped the panic-guard claims to the native targets — CHANGELOG.md:8, the Cargo.toml:39-41 comment, and the recovering_from_panics doc comment (src/lib.rs:65-66) now state that the catch_unwind net covers the uniffi iOS/Android builds and that wasm32-unknown-unknown is abort-only, where protection comes from the error-propagation fix instead. That matches what was verified: cargo emits -C panic=<x> only when <x> != unwind, and the wasm target spec hard-codes abort.
• Recorded why 1.0.14 was skipped — CHANGELOG.md:11 now notes the tag was already used by the superscript-ios-next release pipeline, so the version jump is explained in the repo again.
• Deleted the ignored [profile.release] from wasm/Cargo.toml — confirmed inert before and after: cargo metadata shows wasm is a member of the root workspace (cargo ignores non-root member profiles, and build_wasm.sh:10 still resolves that root), and the root profile's opt-level = "z" / lto = "fat" / codegen-units = 1 / strip match what was removed, so the wasm artifact is unchanged.

The earlier body-level items — no CI enforcement for panic = "unwind" or the new --lib tests, no pre-merge run of the nightly Apple panic_unwind legs, and the silent Null degrade for a malformed filter — are untouched by this delta and still stand as decisions for you rather than defects in the code.

  ^{｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}

[yusuftor]

Addressing the three review findings:

⚠️ wasm guard inert / changelog overstated — fixed in ac6cc18 (see inline thread): claims scoped to the native uniffi targets, wasm abort-only caveat stated, dead [profile.release] in wasm/Cargo.toml deleted. Hardening the wasm path itself (the remaining .expect/.unwrap calls that trap the module) is queued as follow-up work.

⚠️ nightly panic_unwind legs unverifiable pre-merge — verified locally on macOS from this branch at ac6cc18, mirroring build_ios.sh exactly (RUSTFLAGS="-C opt-level=3 -C debuginfo=0 -C strip=symbols", -Zbuild-std=std,core,alloc,panic_unwind), with rustc 1.99.0-nightly (3d6c19bb9 2026-08-11) and Xcode 26.5 SDKs:

target	result	libcel_eval.a
aarch64-apple-visionos	✅ 32s	24.8 MB
aarch64-apple-visionos-sim	✅ 31s	24.8 MB
arm64_32-apple-watchos (tier 3)	✅ 30s	24.6 MB
aarch64-apple-watchos-sim	✅ 31s	24.7 MB
x86_64-apple-watchos-sim	✅ 27s	24.5 MB

Size cost of unwind, isolated on aarch64-apple-ios (identical flags, -C panic=abort vs profile unwind): 30.02 MB → 31.82 MB, +6.0% on the static archive; linked app slices will see less after dead-stripping. Caveat: this validates compile + archive — the same surface the post-merge build_ios.sh run exercises — so the first full link of a watchOS slice still happens in a consumer app after release.

ℹ️ silent Null degrade — intentional and consistent with the pre-existing graceful design: Unknown function and Null can not be compared already degrade to Null identically, and at the SDK boundary the {"Err": …} shape is surfaced (SuperwallKit's CELEvaluator logs .failure as a warning). Making the malformed-filter case observable belongs to the dashboard-side validation tracked in SW-5708, which can reject a bad expression_cel at save time — earlier than any runtime signal could.

🤖 Generated with Claude Code