Classify errors as bugs vs runtime input errors

[elazarg]

Summary

• Introduces a small, deliberately shallow error hierarchy (PrevailError, RuntimeInputError, InternalError) in a new dependency-free header crab_utils/prevail_errors.hpp. The single load-bearing distinction is bug vs runtime input error; sub-classifying RuntimeInputError further is deferred until a handler differentiation justifies it.
• main.cpp's top-level catch arms now distinguish the two: InternalError exits 2 with a "this is a bug; please file an issue" message; RuntimeInputError exits 1 with the existing error: ... shape.
• Adds missing handling at input boundaries (YAML file-open check, yaml-cpp wrapping with line/column context, assembly-parser numeric and lookup wrappers).
• Drops dead code (SafeI64 / num_safeint.hpp) in a separate commit.

Why

The codebase mixed exceptions, variants, optionals, catch-alls, and bug-style throws. A user seeing error: ... from main.cpp could not tell whether their input was bad or Prevail had a bug. CRAB_ERROR and InvalidControlFlow were both used for either category. VerificationError derived from std::runtime_error despite never being thrown (only stored in optional<>). The goal here is classification only — making the categories greppable, typed, and surfaced distinctly to the user. The eventual policy decision (abort vs throw vs std::expected vs contracts) is left for follow-up work.

What changed

Hierarchy and routing (crab_utils/prevail_errors.hpp, crab_utils/debug.hpp)

• PrevailError, RuntimeInputError, InternalError defined in a new neutral header.
• CRAB_ERROR(...) now throws InternalError instead of bare std::runtime_error.
• UnmarshalError and InvalidControlFlow reparented under RuntimeInputError.
• Decorative MalformedElf collapsed into UnmarshalError (never separately caught anywhere).

Throw-site classification

• Generic runtime_error throws at user/input boundaries → RuntimeInputError: printing.cpp output-file open, ir/parse.cpp constraint parsing, linux/linux_platform.cpp map creation, crab/type_domain.cpp unsupported type names.
• Residual invariant-style throws → CRAB_ERROR: string_constraints.hpp "cannot iterate bottom", printing.cpp label lookup during disassembly, crab/ebpf_transformer.cpp logic_error, the cfg_builder.cpp site whose own message said "internal error" but threw InvalidControlFlow.
• ir/marshal.cpp IR-shape checks → CRAB_ERROR. The marshaller has no production callers — it is exercised only by test/test_marshal.cpp round-trip oracles, and reaching these throws means a test constructed an IR shape that doesn't have a valid bytecode encoding (a Prevail-side gap, not user input).

VerificationError becomes a plain struct

• It was a value type pretending to be an exception (: std::runtime_error, never actually caught polymorphically). Now: struct VerificationError { std::string message; std::optional<Label> where; }.
• The local non-local-exit inside EbpfChecker is preserved by introducing a small file-private VerificationFailureSignal exception, converted at the catch site into the value-typed VerificationError.

Missing handling added

• read_suite() checks the ifstream open before YAML::LoadAll (a missing test file used to silently produce zero documents).
• yaml-cpp parsing in read_suite is wrapped to preserve line/column context with the suite path.
• Assembly parser gains parse_with_context wrappers around stoi/stoll/stoull and named lookup helpers (lookup_op, lookup_label) so errors point at the offending token instead of leaking generic "stoi" / "map::at" what() strings.

Other small improvements

• RejectionReason consolidation: added an enforce_instruction_feature_support helper so the value-returning checker remains pure-query while the throw-shape conversion happens in one named place.
• [[nodiscard]] added to unmarshal(), ebpf_domain_check, check_loop_bound, check_instruction_feature_support.
• Dead num_safeint.hpp deleted (zero references; its checked_div divided before checking for zero).

Out of scope

• The error-handling policy itself (Define a project-level error-handling policy #1079) — abort vs throw vs std::expected vs contracts. This PR makes the categories greppable so the policy decision can be made site-by-site without re-auditing throw sites.
• Sub-classifying RuntimeInputError (IoError, UnsupportedFeature, InvalidProgramStructure) — deferred until a handler differentiation justifies it.
• Untangling test-helper string-parsing (parse_linear_constraints) from the production API of EbpfDomain / AnalysisResult — tracked in Untangle test-helper string-parsing from the production API #1103.
• num_big.hpp Number parse-string and narrow<T> family — these CRAB_ERROR sites are reachable from input-driven values and may belong on the runtime-input side per call site; left alone for case-by-case reclassification.
• Public-API C++ standard floor — owned by the public-API stability workstream.
• Relocation skip in elf_reader.cpp:641-643 — plausibly intentional for multi-program reloc sections; needs a comment + targeted test before changing behavior.

Test plan

• cmake --build build --parallel clean.
• ./bin/tests passes at the documented baseline: 1557 cases = 1320 passed + 236 failed-as-expected + 1 skipped, 8842 assertions = 8606 passed + 236 failed-as-expected. Zero unexpected failures.
• DCO sign-off, license headers, and project formatter all pass on the commits.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Rate limit exceeded

@elazarg has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 25 minutes and 23 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: b1ce3a44-7df9-4b48-baef-148ee55df82a

📥 Commits

Reviewing files that changed from the base of the PR and between 84edc72 and bec90cc.

📒 Files selected for processing (3)

• src/main.cpp
• src/string_constraints.hpp
• src/test/ebpf_yaml.cpp

📝 Walkthrough

Walkthrough

The PR establishes a project-wide error hierarchy (PrevailError, RuntimeInputError, InternalError) and migrates exception handling throughout the codebase. It refactors VerificationError from a runtime_error subclass to a lightweight struct, changes internal failure paths from throwing generic exceptions to using CRAB_ERROR, and adds [[nodiscard]] attributes to functions returning error states.

Changes

Cohort / File(s)	Summary
Error Hierarchy & Infrastructure src/crab_utils/prevail_errors.hpp	Introduces project-wide error base class and two primary exception subclasses: RuntimeInputError for input/parsing failures and InternalError for invariant violations.
CRAB_ERROR Macro Updates src/crab_utils/debug.hpp	Updates CRAB_ERROR macro to throw InternalError instead of std::runtime_error.
eBPF Verification Error Handling src/crab/ebpf_domain.hpp, src/crab/ebpf_checker.cpp, src/crab/ebpf_transformer.cpp	Converts VerificationError from exception subclass to lightweight struct with message field, marks ebpf_domain_check as [[nodiscard]], and replaces exception throws with CRAB_ERROR/signal-based error handling.
Input Validation & Parsing src/crab/type_domain.cpp, src/io/elf_loader.hpp, src/io/elf_reader.cpp	Standardizes error reporting in parsing/validation: changes from std::runtime_error to RuntimeInputError and UnmarshalError for input-related failures.
Program IR & Parsing src/ir/parse.cpp, src/ir/program.hpp, src/ir/marshal.cpp, src/ir/unmarshal.hpp	Introduces parse_with_context helper to wrap numeric conversions, migrates error types to RuntimeInputError, updates InvalidControlFlow to inherit from RuntimeInputError, and marks unmarshal functions [[nodiscard]].
Control Flow Analysis src/ir/cfg_builder.cpp, src/fwd_analyzer.cpp	Centralizes instruction validation error reporting and marks checker/bound-validation functions [[nodiscard]]; replaces exception throws with CRAB_ERROR for missing kfunc resolution.
Runtime Diagnostics & Output src/printing.cpp, src/main.cpp, src/string_constraints.hpp, src/linux/linux_platform.cpp	Updates error output to use VerificationError.message, marks file-open failures as RuntimeInputError, routes InternalError with exit code 2 in main error handlers, and converts inline errors to CRAB_ERROR.
Test Infrastructure src/test/ebpf_yaml.cpp	Adds RuntimeInputError handling for file open failures and YAML parsing errors; refactors exception capture logic for test cases.
Removed Code src/arith/num_safeint.hpp	Deletes entire prevail::SafeI64 class and supporting implementation.

Possibly related issues

• Define a project-level error-handling policy #1079: Implements the project-level error taxonomy and exception-handling policy requested in this issue.

Possibly related PRs

• Implement checks for the --line-info flag and move program selection into read_elf #946: Modifies unmarshal function declarations in src/ir/unmarshal.hpp (this PR adds [[nodiscard]] while that PR changes signatures to require options parameter).
• No transform when checking + check code reorg (second attempt) #796: Modifies eBPF verification error handling in src/crab/ebpf_checker.cpp, src/crab/ebpf_domain.hpp, and related files.
• Remove thread_local_program_info; make Program self-contained #1091: Refactors eBPF domain checking and ebpf_domain_check error-return surface along with VerificationError usage.

Suggested labels

codex, feature

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 10.87% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Classify errors as bugs vs runtime input errors' directly reflects the main objective of the PR, which is to introduce an error hierarchy distinguishing internal bugs from runtime input errors.
Description check	✅ Passed	The description comprehensively covers the changes: the new error hierarchy, reclassification of throw sites, conversion of VerificationError to a struct, additions of missing input handling, and dead code removal.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch better-error-handling

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/string_constraints.hpp (1)

42-58: ⚠️ Potential issue | 🟡 Minor

contains() still bypasses the new bottom check.

value() now raises CRAB_ERROR, but contains() still calls maybe_inv.value() directly, so bottom-state access will keep surfacing as std::bad_optional_access instead of the project’s internal-error classification.

Proposed fix

-    bool contains(const std::string& item) const {
-        return maybe_inv.value().contains(item);
-    }
+    bool contains(const std::string& item) const {
+        return value().contains(item);
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/string_constraints.hpp` around lines 42 - 58, The contains() method
currently calls maybe_inv.value() directly and can throw
std::bad_optional_access; change it to use the existing value() accessor (or
perform the same is_bottom() check and CRAB_ERROR as in value()) so bottom-state
access is handled consistently; update StringInvariant::contains to call
value().contains(item) (or check is_bottom() then CRAB_ERROR) and refer to
maybe_inv, value(), contains(), and is_bottom() to locate the change.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/main.cpp`:
- Around line 169-175: The code currently catches std::runtime_error and treats
all such exceptions as user load failures; change the catch to specifically
catch RuntimeInputError (for errors from user input in ElfObject::get_programs()
/ ElfObject::list_programs()) and assign its message to load_error, then add a
separate fallback catch (e.g., catch(const std::exception& e)) that logs or
handles unexpected runtime exceptions differently (preserving the existing
InternalError behavior and returning the proper exit code). Update the catch
block that currently reads catch(const std::runtime_error& e) to catch
RuntimeInputError first and then add the generic fallback catch below it so
residual bugs aren’t masked.

In `@src/test/ebpf_yaml.cpp`:
- Around line 375-380: The catch around read_case currently catches all
std::exception (swallowing InternalError); change the catch so InternalError
escapes: in the catch(const std::exception& ex) for read_case(), check if
dynamic_cast<const InternalError*>(&ex) (or use typeid) and rethrow if true,
otherwise set tc.actual_exception = ex.what(); this keeps InternalError from
being recorded as an expected_exception while still capturing input/parsing
failures.

---

Outside diff comments:
In `@src/string_constraints.hpp`:
- Around line 42-58: The contains() method currently calls maybe_inv.value()
directly and can throw std::bad_optional_access; change it to use the existing
value() accessor (or perform the same is_bottom() check and CRAB_ERROR as in
value()) so bottom-state access is handled consistently; update
StringInvariant::contains to call value().contains(item) (or check is_bottom()
then CRAB_ERROR) and refer to maybe_inv, value(), contains(), and is_bottom() to
locate the change.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5dda2780-67c3-4411-aec5-f0bbb17d9354

📥 Commits

Reviewing files that changed from the base of the PR and between 305f59f and 84edc72.

📒 Files selected for processing (20)

• src/arith/num_safeint.hpp
• src/crab/ebpf_checker.cpp
• src/crab/ebpf_domain.hpp
• src/crab/ebpf_transformer.cpp
• src/crab/type_domain.cpp
• src/crab_utils/debug.hpp
• src/crab_utils/prevail_errors.hpp
• src/fwd_analyzer.cpp
• src/io/elf_loader.hpp
• src/io/elf_reader.cpp
• src/ir/cfg_builder.cpp
• src/ir/marshal.cpp
• src/ir/parse.cpp
• src/ir/program.hpp
• src/ir/unmarshal.hpp
• src/linux/linux_platform.cpp
• src/main.cpp
• src/printing.cpp
• src/string_constraints.hpp
• src/test/ebpf_yaml.cpp

💤 Files with no reviewable changes (1)

• src/arith/num_safeint.hpp

[elazarg]

Addressed all three CodeRabbit findings in bec90cc:

1. main.cpp:169-175 and :196-203 — Narrowed both blocks to catch (RuntimeInputError&) + catch (std::exception&) fallback (after the existing InternalError arm), so non-categorized exceptions no longer get quietly absorbed as load errors.

2. test/ebpf_yaml.cpp:375-380 — Narrowed the expected-exception matcher to catch (RuntimeInputError&) + catch (YAML::Exception&) so InternalError escapes upward instead of satisfying an expected-exception clause. The schema-validation throws inside parse_observations/raw_options_to_options were retagged from bare std::runtime_error to RuntimeInputError to keep the narrowed catch matching them.

3. string_constraints.hpp outside-diff — contains() now routes through the value() accessor so the bottom check goes through CRAB_ERROR consistently with the other bottom-state accesses, instead of falling through to std::bad_optional_access.

Tests still pass at baseline (1320 passed + 236 failed-as-expected + 1 skipped).