Phase 2 of #149: the correctness fixes

[grzanka]

Implements Phase 2 — "the correctness fixes" of the plan of action in #149, building on Phase 0 (#150) and Phase 1 (#151), both merged. This is the phase that changes computed values, unlike Phase 0/1: each finding below gets its own dedicated regression test, on top of the exhaustive availability sweep from Phase 1.

Updated after a critical review (see the review thread below) that found real issues beyond the original scope — an undocumented value change reaching DEDX_MSTAR, a genuinely broken MSTAR coefficient branch A7 didn't fully close, a new out-of-bounds read, an availability-API accuracy regression, and config corruption on a failed compound load. All are fixed; details in the "Review round" section below the original findings.

Changes

A1 🔴 — load_compound() mixed energy grids across tiers, and could sum uninitialized heap

DEDX_AUTO resolves each compound constituent independently — a tabulated report first, falling back to the Bethe-Bloch formula (122-point grid) only for elements no report tabulates (133-point grid family). load_compound() used to Bragg-sum every constituent over compound_data[0].length, on the x-axis of whichever constituent happened to load last — silently mixing bin boundaries, or summing uninitialized heap for a constituent shorter than that length.

load_compound() now carries each constituent's own energy grid, verifies every constituent resolved onto the identical grid (length and memcmp of the knots), and rejects a mismatch with DEDX_ERR_INCONSISTENT_ENERGY_GRID instead of serving a wrong or partially-uninitialized number. (This code was reused from DEDX_ERR_INCONSISTENT_COMPOUND originally; split into its own code in review — see B4 below, the caller's compound specification here is perfectly valid, the problem is purely mismatched grids.) load_bethe_2() now zero-initializes its output struct up front (matching what read_embedded_stopping_data() already did), and the final sum uses min(length_i) across constituents as belt-and-braces.

A2 🔴 — element/compound boundary was off by one: material 99 is a compound, not an element

dedx_elements.h runs elements 1–98, then compounds from 99 (DEDX_A150_TISSUE_EQUIVALENT_PLASTIC = 99), but five call sites across dedx.c, dedx_validate.c and dedx_embedded_metadata.c used <= 99 / > 99 as the element/compound test. A150 tissue-equivalent plastic was therefore evaluated by the Bethe path as elemental einsteinium (Z=99, A=252) instead of its real composition, and got the elemental solid-state I-value correction on top.

Introduces DEDX_MAX_ELEMENT_ID (98) and replaces every such literal. Regression test asserts bragg_used == 1 and Bethe ≈ tabulated within 2% for id 99 (previously ~29% off).

A3 🟠 — load_compound() silently ignored compound_state for its own I-value lookups

It seeded each constituent's I-value via the public dedx_get_i_value(), which hardcodes DEDX_GAS — so config->compound_state was silently ignored whenever elements_i_value wasn't already supplied by the caller, which is the common case for DEDX_AUTO (dedx_internal_evaluate_i_pot() only runs for program >= DEDX_DEFAULT). load_compound() now resolves the compound's own state itself when still unset, and calls the state-aware internal accessor directly. (This has a real, documented side effect on DEDX_MSTAR — see B1 below.)

A5 🟠 — a missing density row blocked tabulated programs — 407 false negatives in the availability API

dedx_internal_validate_rho() used to fail whenever a target had no embedded density row, for every program, even table lookups that never read rho. Exactly one material was affected (FERROUSOXIDE, id 159), but it broke it for all 407 program/ion pairs that advertised it. rho is now only required where it's actually used (program >= DEDX_DEFAULT, or DEDX_AUTO's Bethe fallback, checked at the point of need inside load_bethe_2()), and the missing density row is added — cited in data/README.md (added in review) as the standard literature FeO/wüstite density. FERROUSOXIDE's I-value is deliberately left unfabricated — no raw source exists to verify it against — so DEDX_DEFAULT/DEDX_BETHE_EXT00, which read a compound's own I-value directly rather than Bragg-averaging it, still correctly fail for it; material_id_supported() now predicts that gap too (see B4 below), so it's a closed gap on the advertising side even though the value itself stays unfilled.

A6 🟠 — a zero in the embedded ICRU73 table silently downgraded interpolation with no way to detect it

The one non-positive value in the whole ICRU73 table (Na in Ar, first energy point) made dedx_internal_calculate_coefficients() fall back from the requested log-log interpolation to linear — silently, because loaded_data->interpolation_mode kept reporting whatever was requested, not what was actually used. The function now returns the mode it actually applied, load_data() records that instead of the request, and a new dedx_get_effective_interpolation_mode() accessor exposes it to callers (returns -1 on a bad cfg_id, not 0, so a caller that forgets to check *err can't mistake failure for DEDX_INTERPOLATION_LOG_LOG — fixed in review). (Deciding the fate of the underlying zero value itself, and build-time validation in tools/dat2c.py, are left for Phase 4/E4 — no authoritative replacement value exists to fabricate one from.)

A7 🟠 — an invalid mstar_mode produced an all-zero table with no error

dedx_mpaul.c's mode switch fell through an empty "illegal mode" branch, leaving the computed table all zero with err == 0. dedx_internal_validate_config() now validates mstar_mode against the documented DEDX_MSTAR_MODE_* set (only for DEDX_MSTAR) and returns the new DEDX_ERR_INVALID_MSTAR_MODE. This only covers an invalid top-level mstar_mode, though — review found the same "illegal mode" fallthrough is also reachable with a valid, resolved mode for specific ion/effective-charge combinations; see B2 below for that fix.

A8 🟡 — check_ion() was skipped on the custom-compound path

dedx_load_config() dispatched straight to load_compound() whenever elements_id was set, bypassing check_ion() (only load_config_clean()'s tabulated-program path called it). A custom compound with an ion its program doesn't support failed deep inside Bragg decomposition with DEDX_ERR_COMBINATION_NOT_FOUND instead of the immediate, clear DEDX_ERR_ION_NOT_SUPPORTED an elemental target already got. check_ion() moves into dedx_internal_validate_config() (as dedx_internal_check_ion(), relocated to dedx_validate.c) so both paths agree. (This surfaced a pre-existing out-of-bounds read for an unrecognized program on the newly-reachable custom-compound path — see B3 below.)

Review round: B1–B5, S1–S5, nits

A critical review (thread below) reproduced the branch against main with a differential sweep over every (program, ion, material) triple and targeted ASan probes, and found real issues. All fixed:

• B1 — A3's compound-state resolution has a genuine, undocumented side effect on DEDX_MSTAR (resolve_mstar_mode() reads the same config->compound_state field). Kept the corrected behavior (a compound's gas/condensed state is a property of the compound, not each constituent atom — consistent with the I-value convention A3 already applies), documented it explicitly here and in code, and added a pinned regression test (test_gas_compound_uses_compound_state_not_constituent in test_mstar.c) so a future change to this path is visible instead of silent. These pinned values are the branch's own current output, not independently verified against dedx_web or the MSTAR literature — that cross-check is out of scope for what a unit test can do.
• B2 — dedx_mpaul.c's mode 'h' branch has no coefficients for ion 12-15 (or mode 'd' for effective charge ≤4/≥93); both fell through to an "illegal mode" else that silently computed nonsense with err == 0. Reproduced identically against main (DEDX_MSTAR + ion 12 + DEDX_ARGON → 21936 with err=0 there too) — this predates Phase 2 entirely and is structurally distinct from A7 (reachable via a valid, resolved mode). Both branches now set *err (reusing DEDX_ERR_ION_NOT_SUPPORTED_MSTAR, exactly as issue Deep audit: DEDX_AUTO returns garbage/unusable results for 174 materials, element-boundary off-by-one, 2 API-reachable memory-safety bugs, plus API/doc inconsistencies — with a plan of action #149's own A7 text suggested).
• B3 — dedx_internal_check_ion() indexed dedx_program_available_ions[prog] with no bounds check; an unrecognized prog walked off the end of the table (reproduced as an ASan global-buffer-overflow). Pre-existing on the elemental path; A8 made it newly reachable from the custom-compound path. Now validates prog against dedx_get_program_list() first.
• B4 — material_id_supported() didn't know about A1's grid-tier constraint or the FERROUSOXIDE I-value gap, so the availability API's false-advertisement rate went up 11x from this PR's own A1 fix (exactly the defect class Deep audit: DEDX_AUTO returns garbage/unusable results for 174 materials, element-boundary off-by-one, 2 API-reachable memory-safety bugs, plus API/doc inconsistencies — with a plan of action #149 opened on). It now predicts both, mirroring find_data()'s actual dispatch: TOTAL_COMBINATIONS 101886 → 100222, load failures 1470 → 108, bound mismatches 480 → 470. Also: dedx.h's DEDX_AUTO doc now documents the grid-mismatch exception explicitly, and the grid-mismatch error code is split out of DEDX_ERR_INCONSISTENT_COMPOUND (see A1 above). Resampling onto a common grid instead of rejecting the mismatch (an alternative the review raised) is explicitly not taken on here — a real physics behavior change, not just an advertising fix, needing its own scope.
• B5 — A failed load_compound() left config->target pointing at whichever constituent it was mid-resolving, not the compound the caller asked for (plus i_value/_temp_i_value potentially clobbered) — silent corruption for a caller that reuses the config after a failed load. Restructured around one cleanup path that restores all three on every failure.
• S1 — dedx_get_effective_interpolation_mode() now returns -1 on error (see A6 above).
• S2 — test_availability_exhaustive.c's load-failure ratchet now asserts per error code (equality), not one aggregate ceiling that could absorb a new failure mode.
• S3 — FERROUSOXIDE's density now has a citation in data/README.md, and its test asserts the exact value (5.7), not just > 0.
• S4 — Cross-referenced the isnan() self-rescue in dedx_internal_evaluate_spline() against A6's new accessor (deliberately redundant, not alternatives) in both directions. The new accessor is already usable from C++ (the RAII header exposes raw dedx_workspace/dedx_config pointers, no wrapper needed); Python's ctypes bindings don't cover the workspace/config API at all today (only the simple/table convenience functions), so wiring this one function in means exposing that whole layer first — a follow-up, not something to rush here.
• S5 — Noted, not changed: A3's own test uses a synthetic compound because A1 now rejects most real multi-tier compounds before reaching A3's code path; the solid-boron ×1.13 correction question is a pre-existing physics-modeling question outside what this PR can resolve.
• Nits — // clang-format off/on around dedx_embedded_compos_rows[] so one added row can't reflow the whole hand-maintained table again.

Verification

Locally, after every round of changes:

• Full ctest suite (33/33) green in a plain -DDEDX_WERROR=ON build.
• Full ctest suite (33/33) green under -fsanitize=address,undefined via the sanitize preset.
• Every test_* binary except test_availability_exhaustive passes under valgrind --leak-check=full --track-origins=yes (0 errors, all heap blocks freed).
• clang-format-19 (CI's exact version, installed locally to match) clean on every changed file.
• Each finding double-checked against its own reproducer, including the review's: B1's MSTAR/BUTANE numbers, B2's ion-12-15 discontinuity (on both main and this branch, before/after), B3's ASan repro, B4's advertised-vs-load-succeeds counts.

Scope

Deliberately excludes:

• A4 (target-aware energy bounds) — tracked separately, not part of this phase's plan.
• Fabricating any physics value with no authoritative source to check it against — FERROUSOXIDE's I-value (A5) and the ICRU73 Na→Ar zero itself (A6) stay open, documented gaps rather than invented numbers.
• tools/dat2c.py build-time validation (A6's other half) and the Bethe evaluator de-duplication (E3) — Phase 4 territory.
• Resampling DEDX_AUTO constituents onto a common grid instead of rejecting a tier mismatch (B4) — a real computed-value change needing its own scope.
• Exposing dedx_get_effective_interpolation_mode() (or the workspace/config API generally) to Python (S4) — needs that whole layer wired up first.
• Phase 3 (API contracts: dedx_config_init(), dedx_unload_config(), *err semantics, the general list-accessor bounds-checking sweep) and Phase 4 (architecture/data generators) are otherwise untouched.

Closes nothing on its own; #149 stays open for the remaining phases.

---

Generated by Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 91.33333% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.04%. Comparing base (5f345aa) to head (996d55a).

Files with missing lines	Patch %	Lines
src/dedx.c	90.72%	9 Missing ⚠️
src/dedx_validate.c	93.02%	3 Missing ⚠️
src/dedx_spline.c	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #152      +/-   ##
==========================================
+ Coverage   85.32%   86.04%   +0.72%     
==========================================
  Files          12       12              
  Lines        1649     1749     +100     
  Branches      320      348      +28     
==========================================
+ Hits         1407     1505      +98     
- Misses        242      244       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

This PR implements Phase 2 (“correctness fixes”) of #149 by tightening validation and fixing several silent-miscompute or silent-degradation cases across compound loading, element/compound dispatch, interpolation-mode reporting, density validation, and MSTAR config validation, with targeted regression tests and an updated exhaustive availability baseline.

Changes:

• Fixes DEDX_AUTO compound loading to reject mixed energy grids, avoid summing uninitialized data, and honor compound_state for constituent I-value lookups.
• Corrects the element/compound boundary (material id 99 is a compound) by introducing DEDX_MAX_ELEMENT_ID and replacing off-by-one literals.
• Makes interpolation downgrade observable via dedx_get_effective_interpolation_mode(), validates mstar_mode, unifies ion validation, and updates/extends regression tests (including the exhaustive sweep ratchet baselines).

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/test_validate_internal.c	Ensures custom-compound validation tests set an explicit ion.
tests/test_mstar.c	Adds regression coverage for invalid mstar_mode handling and default behavior.
tests/test_material_availability.c	Adds regression tests for A1/A2/A3/A5 behaviors and updates expectations (e.g., grid mismatch rejection).
tests/test_interpolation.c	Adds regression tests for effective interpolation-mode downgrade visibility and API guard behavior.
tests/test_error_codes.c	Updates/extends regression tests for A8 ion validation and custom-compound ion rejection.
tests/test_availability_exhaustive.c	Updates baseline constants and expands commentary to reflect Phase 2 outcomes.
src/dedx.c	Implements core correctness fixes (grid consistency checks, element boundary constant usage, effective interpolation-mode recording/accessor, rho checks, etc.).
src/dedx_validate.h	Exposes dedx_internal_check_ion() for shared ion validation.
src/dedx_validate.c	Moves ion validation into config validation, scopes rho requirements, and validates mstar_mode.
src/dedx_spline.h	Updates spline coefficient builder to return the effective interpolation mode.
src/dedx_spline.c	Implements returning the effective interpolation mode (log-log vs linear fallback).
src/dedx_embedded_metadata.c	Uses DEDX_MAX_ELEMENT_ID for element/compound boundary logic.
src/data/embedded/dedx_metadata.h	Adds missing FERROUSOXIDE density row and updates embedded row count.
include/dedx.h	Adds public dedx_get_effective_interpolation_mode() API documentation and declaration.
include/dedx_error.h	Adds DEDX_ERR_INVALID_MSTAR_MODE.
include/dedx_elements.h	Introduces DEDX_MAX_ELEMENT_ID and documents correct element/compound boundary usage.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[grzanka]

Critical review of Phase 2

I reviewed this by building a548076 and origin/main side by side and running a differential sweep over every (program, ion, material) triple for the 9 implemented programs, sampling dE/dx at 0.1 / 1 / 10 / 100 MeV/u (~102k data points per build), plus targeted ASAN/UBSan probes. Everything below is reproduced against the branch as it stands, not read off the diff.

The ctest suite is green (33/33) and the individual fixes are, in isolation, well-reasoned and unusually well-commented. But the sweep surfaces three behaviour changes that are not in the PR description at all, one of them badly wrong, plus a new memory-safety exposure. I don't think this is mergeable as-is.

---

Blocking

B1 — A3 silently changes DEDX_MSTAR results for 56 compounds × 16 ions, undocumented and untested

The description scopes A3 to "load_compound()'s own I-value lookups". It isn't scoped that way in practice. load_compound() now resolves config->compound_state before the constituent loop — and resolve_mstar_mode() (src/dedx_mstar.c:21) also reads config->compound_state:

static char resolve_mstar_mode(char state, dedx_config *config, int *err) {
    int target_state = config->compound_state;
    ...
    if (target_state == DEDX_DEFAULT_STATE) {
        if (dedx_internal_target_is_gas(config->target, err) != 0) ...   /* config->target == the *constituent element* here */

Before this PR, MSTAR compounds reached that function with compound_state == DEDX_DEFAULT_STATE and config->target already overwritten with the current constituent element, so the gas/condensed mode was picked from the element, not from the compound. Now it is picked from the compound. Result of the sweep, main → this branch:

program	changed sample points	materials affected	>50% change
DEDX_MSTAR (4)	3584	56	575
DEDX_AUTO (10)	2000	—	—
DEDX_DEFAULT / DEDX_BETHE_EXT00	80 / 80	material 99 only (A2, expected)	—

Affected MSTAR materials include BERYLLIUMOXIDE, BORONOXIDE, ADENINE, GUANINE, UREA, EYELENS_ICRP, TISSUE_SOFT_ICRUFOUR_COMPONENT, FERRICOXIDE — up to 94% change in stopping power, for all 16 MSTAR ions.

The new behaviour is arguably the more correct one (a compound's state should come from the compound). But a 94% change to a published, literature-backed dataset cannot land as an unremarked side effect of a fix described as being about I-values. This needs: an explicit section in the description, a value-pinning regression test on at least one MSTAR compound, and a check of the new numbers against MSTAR/dedx_web reference output.

B2 — B1 routes MSTAR ions 12–15 on gaseous compounds into dedx_mpaul.c's "illegal mode" branch — A7's root cause is still there

dedx_internal_calculate_mspaul_coef() enumerates mode 'h' only for ions 3–11, 16, 17, 18 (src/dedx_mpaul.c:73-129). Ions 12–15 fall through to

    } else {
        // illegal mode
    }

leaving a = 5.0, b = -1, c = -1 — the defaults — which then feed the 'g' || 'h' evaluation branch. err stays DEDX_OK. Because B1 now resolves BUTANE/ETHANE (gases) to mode 'h' instead of 'g', this is newly reachable. MSTAR, BUTANE, 10 MeV/u, on this branch:

ion 10 stp   5016.1
ion 11 stp   5677.7
ion 12 stp  26504.8   <-- 4.7x discontinuity
ion 13 stp  31086.0
ion 14 stp  36023.6
ion 15 stp  41313.8
ion 16 stp  11547.7   <-- back on the tabulated 'h' branch

Main returns 6992 / … / 16185 here (the 'g' path, smooth). This is a 1546% error at 0.1 MeV/u for BUTANE and ETHANE, silently, with err == 0.

Independently of B1, the same branch is already reachable on main with an elemental gas target and the default mode: DEDX_MSTAR + ion 12–15 + DEDX_ARGON shows the identical discontinuity (21936 vs. ~4044 for mode 'g'). So A7 as implemented fixes the symptom for undocumented letters while the actual silent-garbage branch — reachable with a documented mode 'h' and the default '\0'/'b' config — is untouched. A7 should not be marked done without setting *err in that else branch (and in the mode == 'd' illegal branch at dedx_mpaul.c:181).

B3 — new out-of-bounds read (ASAN) on the custom-compound path with an unvalidated program

Moving check_ion() into dedx_internal_validate_config() (A8) makes it run before anything validates config->program, on a path that previously never called it. dedx_get_ion_list() (src/dedx.c:348) indexes dedx_program_available_ions[program] with no bounds check.

cfg->program = -1;              /* or 50, or any typo'd/future constant */
cfg->ion = DEDX_PROTON;
cfg->target = 0;
cfg->elements_id = {1, 8}; cfg->elements_atoms = {2, 1};
dedx_load_config(ws, cfg, &err);

• main: returns err = 4, clean.
• this branch: AddressSanitizer: global-buffer-overflow ... in dedx_internal_check_ion (dedx_validate.c:73), reading past dedx_available_programs / running off the end of the 8800-byte dedx_program_available_ions table (program = 50 walks the whole table looking for a -1 terminator that a zero-filled row doesn't have).

The underlying gap is pre-existing (the elemental path hits it on main too), but A8 is what extends it to configs that were previously safe, so the guard belongs in this PR: bounds-check program in dedx_get_ion_list()/dedx_internal_check_ion(), or validate program at the top of dedx_internal_validate_config(). The ASAN CI job doesn't catch it because no test passes an invalid program.

B4 — A1 turns 70 materials into hard failures for DEDX_AUTO, while the docs and the availability API still promise otherwise

DEDX_AUTO + proton: 179 → 117 loadable compounds. Across all ions, 1246 advertised combinations now fail (DEDX_ERR_INCONSISTENT_COMPOUND) vs 112 before — 70 distinct materials, including BLOOD_ICRP, BRAIN_ICRP, CONCRETE_PORTLAND, CALCIUMCARBONATE, CALCIUMOXIDE, BARIUMSULFATE, CHLOROFORM, the FREONs, BORONCARBIDE.

I accept the premise — those numbers were genuinely wrong before. But three things don't hold together:

1. dedx.h's DEDX_AUTO doc block is unchanged. It still says the point of DEDX_AUTO is to fall back "rather than failing outright", and that compounds "resolve through the same tabulated-first path". For a third of all compounds that is now false. This is a contract change and needs to be documented where users read it, not only in a test comment.
2. material_id_supported() was taught the density constraint (A5) but not the grid constraint (A1). It already resolves each constituent through element_supported_for_ion(), so it knows which tier each constituent lands on and could decline to advertise a mixed-tier compound. As it stands the availability API's false-advertisement rate for DEDX_AUTO went from 112 to 1246 — 11× worse — which is the exact defect class Deep audit: DEDX_AUTO returns garbage/unusable results for 174 materials, element-boundary off-by-one, 2 API-reachable memory-safety bugs, plus API/doc inconsistencies — with a plan of action #149 opened on.
3. Rejecting isn't the only option. The constituents that force the mismatch are precisely the ones with no tabulated report, i.e. the ones already computed from a formula. Evaluating all constituents on the Bethe grid whenever any constituent needs it (or interpolating the tabulated constituent onto that grid) keeps DEDX_AUTO's "always give a best-effort answer" contract and removes the mismatch class entirely. That would be a strictly better outcome than 70 materials going dark; if it's out of scope for Phase 2, please say so explicitly in Deep audit: DEDX_AUTO returns garbage/unusable results for 174 materials, element-boundary off-by-one, 2 API-reachable memory-safety bugs, plus API/doc inconsistencies — with a plan of action #149 rather than leaving it implied by a baseline bump.

Also: DEDX_ERR_INCONSISTENT_COMPOUND ("Compound specification is inconsistent") is the wrong code to reuse. The caller passed a single valid material id; nothing about their specification is inconsistent. A distinct code (DEDX_ERR_INCONSISTENT_ENERGY_GRID) would be diagnosable.

B5 — a failed compound load leaves the caller's dedx_config corrupted

Every early return -1 in load_compound() — including the new grid-mismatch one, which is now the common outcome for 70 materials — returns without restoring config->target (saved into the local target at src/dedx.c:792 and only restored on the success path at line 864):

AUTO+B4C:  err=211  cfg->target now = 6 (expected 121)   rho=2.52
retry PSTAR with same cfg: err=202  target=5

After a failed load the user's config silently points at a constituent element. A caller who reuses the config — a perfectly reasonable "try AUTO, fall back to PSTAR" pattern — then computes for the wrong target, or gets a nonsense error. config->_temp_i_value and config->i_value are likewise left clobbered. The pattern predates this PR but was nearly unreachable; A1 makes it the norm. A single goto cleanup that restores target/i_value fixes all of them at once.

---

Should fix

S1 — dedx_get_effective_interpolation_mode()'s error return is a valid mode

It returns 0 on DEDX_ERR_INVALID_DATASET_ID, and DEDX_INTERPOLATION_LOG_LOG == 0 (dedx.h:106). A caller that forgets to check *err is told "log-log", which is exactly the wrong answer for a function whose entire purpose is detecting a silent downgrade. Return -1, and document it.

S2 — the ratchet was raised, which is what the ratchet exists to prevent

BASELINE_LOAD_FAILURES 407 → 1470. The traced breakdown in the test comment is genuinely excellent work and I verified the 1246 figure independently — it is exactly right. But the mechanism no longer does its job: a future regression that adds, say, 40 new DEDX_ERR_INCONSISTENT_COMPOUND failures now hides inside a four-digit budget. Since the failures are already traced by error code, assert them that way — e.g. {DEDX_ERR_TARGET_NOT_FOUND: 224, DEDX_ERR_INCONSISTENT_COMPOUND: 1246} with equality per code. Then the numbers can't drift silently and the comment becomes executable instead of prose.

S3 — A5's density value is hand-added to a generated header, with no provenance and no test

{159, 5.70000000e+00f, 0.0f, 0} is added directly to src/data/embedded/dedx_metadata.h. I confirmed tools/dat2c.py does not generate this header (no reference to it), so the PR's "no raw source" claim holds and hand-editing really is the only option — but then:

• the value needs a citation in data/README.md (which is where the file's provenance is documented), not just "the real-world density of FeO/wustite" in a code comment;
• nothing tests it. test_ferrous_oxide_tabulated_program() asserts rho > 0, so a typo'd 57.0 or 0.57 would pass, and this density feeds the Bethe density-effect term for DEDX_AUTO's fallback tier — it's live physics, not metadata;
• it sits awkwardly next to the (correct, and well-argued) refusal to invent an I-value in the same finding. Please state the policy explicitly: verifiable-from-literature values are fine, unverifiable ones are not.

Separately: the 224 remaining false advertisements could be removed without fabricating anything, by having material_id_supported() check I-value availability for program >= DEDX_DEFAULT, mirroring the density check you just added three lines above. That would take the FERROUSOXIDE gap to zero on the advertising side and leave the data gap honestly unfilled.

S4 — A6 is correct, and narrower than the description implies (this is a compliment, but worth stating)

I verified DEDX_ICRU73 + Na + Ar returns byte-identical values on both builds at 0.03/0.1/1/10/100 MeV — so recording the effective mode really is pure observability, no value change. Good. Two follow-ups: the isnan(coef[i].log_x) self-rescue in dedx_internal_evaluate_spline() (dedx_spline.c:174) is now the redundant twin of this fix and should at least cross-reference it; and the new accessor is C-only — it isn't in python/libdedx/_api.py or the C++ wrappers, so the downgrade stays undetectable from Python.

S5 — A3 has no coverage on a real material, and the ×1.13 convention deserves a second look

test_compound_state_affects_bethe_fallback() uses a synthetic single-element custom compound and only asserts gas ≠ condensed. After A1, most real compounds with a Bethe-tier constituent fail to load, so A3's elemental-I-value path is only exercised for compounds where every constituent is untabulated — a case no test covers. Worth adding one, or noting in #149 that A1 and A3 partially cancel.

While in there: dedx_embedded_get_i_value() applies the 1.13 factor to elements that are not gases (state == 2 && !target_is_gas). Elements with a real phase-dependent I-value (H, C, N, O, F, Cl) carry explicit state rows and are unaffected; the multiplier lands on solids such as boron (76 → 85.9 eV), which is not the ICRU 37 recommendation for solid boron. This is pre-existing, but A3 makes it live on a new path, so it's worth confirming the direction of that correction before shipping the value change.

---

Nits

• The clang-format reflow (3976f50) rewrites all 286 rows of dedx_embedded_compos_rows[] — 426 diff lines to review one added row. A // clang-format off / on pair around the table would keep future data diffs readable, which matters for exactly this kind of audit.
• dedx_embedded_read_effective_charge()'s id < 99 → id <= DEDX_MAX_ELEMENT_ID is a pure rename (identical semantics); the description's "five call sites ... treated it as an element" reads as if all five were behavioural.
• load_compound()'s min(length_i) loop is dead by construction given the equality check immediately above it. The comment says so, which is fine — just flagging that coverage tooling will show it as never-varying.

---

What I checked and found solid

• A1's calloc + zero-init of load_bethe_2()'s output struct, and the follow-up in a548076 restoring data.ion/data.target — correct, and the Copilot catch was real.
• A2's boundary fix: verified DEDX_BETHE_EXT00 + proton + material 99 now tracks PSTAR (the einsteinium misclassification is gone), and the only DEDX_DEFAULT/DEDX_BETHE_EXT00 value changes in the whole 102k-point sweep are material 99. Clean, surgical, exactly as advertised.
• A5's relaxation of dedx_internal_validate_rho() plus the point-of-need check in load_bethe_2() — right shape, right place.
• The traced baseline accounting in test_availability_exhaustive.c — I reproduced 1246 and the TOTAL_COMBINATIONS delta independently. The honesty of that comment is the reason B1/B2 were findable at all.

The blocking items are B1/B2 (an undocumented, partly badly-wrong MSTAR value change), B3 (new OOB read), B4 (contract/doc/advertising drift) and B5 (config corruption). B1 in particular I'd want reference-checked before merge.

[grzanka]

Thank you for this — the differential sweep and ASan probes caught real issues the original PR missed, and the "verified solid" section confirms A1/A2/A5 and the baseline accounting held up under independent scrutiny, which is useful to know too.

All blocking and should-fix items are addressed in b6bfe76, verified the same way you found them (a differential sweep against main, and against the pre-review state of this branch), plus the existing full local battery (ctest plain/ASan+UBSan/Valgrind, clang-format-19). Summary — full detail is in the PR description's new "Review round" section:

• B1 (undocumented MSTAR value change): confirmed real via the same mechanism you found (resolve_mstar_mode() reading the same config->compound_state A3 now resolves early). Kept the new behavior — I'd argue it's the more defensible convention, and you flagged it that way too — but it's now explicit in the PR description, in code comments, and pinned with a regression test on a real material (BUTANE) rather than left as a silent diff. I want to be upfront about a limit here: the pinned values are this branch's own current output, not something I can independently verify against dedx_web or the MSTAR literature — I don't have access to either. If you're able to spot-check a couple of the pinned BUTANE numbers against a reference, that would close this out with actual confidence rather than my own regression-pin-only assurance.
• B2 (the real root cause A7 didn't reach): fixed at the actual point of failure in dedx_mpaul.c, both branches (ion 12-15 under mode 'h', and effective-charge ≤4/≥93 under mode 'd'). Reproduced your exact discontinuity against main first to confirm it predates this PR.
• B3 (new OOB read): fixed with a prog membership check before dedx_get_ion_list() is ever called from dedx_internal_check_ion(). Reproduced your repro under ASan before and after.
• B4 (advertising drift + docs + error code): all three parts done — material_id_supported() now predicts the grid-tier mismatch and the I-value gap instead of only reflecting dedx_load_config()'s answer after the fact (this is what actually dropped load failures 1470→108, not a raised ceiling), dedx.h documents the exception explicitly, and the grid-mismatch case now uses a dedicated DEDX_ERR_INCONSISTENT_ENERGY_GRID instead of reusing DEDX_ERR_INCONSISTENT_COMPOUND. I did not take on resampling onto a common grid — agreed with your own framing that it's a real computed-value change deserving its own scope, not something to fold in here.
• B5 (config corruption): fixed with a single cleanup path in load_compound() restoring target/i_value/_temp_i_value on every failure branch, plus a regression test on the exact BoronCarbide scenario.
• S1–S3: done as suggested (−1 sentinel, per-error-code ratchet, cited + tightened FeO density test).
• S4: cross-referenced the isnan() self-rescue both directions. On the Python/C++ gap — turned out asymmetric: C++ already has access via the RAII header's raw pointers, no wrapper needed; Python's ctypes layer doesn't wrap the workspace/config API at all today (only the simple/table convenience calls), so this isn't a one-function gap, it's "that whole layer isn't there yet." Left as a follow-up rather than doing it partially under time pressure.
• S5: agreed and left alone — noted both points in the PR description rather than acting on them, since the ×1.13-on-solid-boron question needs domain judgment I don't have, and forcing A3 coverage onto a real multi-tier compound would fight against A1's own correctness fix.
• Nits: clang-format off/on added around the table.

---

Generated by Claude Code

[grzanka]

Pushed a small follow-up (734c562) on top of the review-response commit, closing the two gaps flagged in my earlier reply:

1. The residual 108-combo MSTAR advertising gap. material_id_supported() learned to predict DEDX_AUTO's grid-tier mismatch and the FERROUSOXIDE I-value gap, but MSTAR's own coefficient-availability gap (ions 12-15 on gas targets, per B2's fix) was left unmodeled — those combinations stayed advertised even though dedx_load_config() now correctly rejects them. element_supported_for_ion()/material_id_supported() now probe the actual coefficient computation under find_data()'s own default mstar_mode assumption, threading a compound's own resolved gas/condensed state down to its constituents (mirroring how load_compound() resolves compound_state once, from the compound's own id, per finding A3) — a naive per-constituent check would both under- and over-advertise. TOTAL_COMBINATIONS 100222 → 100114, load_failures 108 → 0; LOAD_FAILURE_BASELINES is now empty.

2. dedx_get_ion_list() is public API, directly callable with any int, and was still indexing dedx_program_available_ions[program] unchecked — the B3 fix guarded the internal dedx_internal_check_ion() call site but not this function itself. Reproduced under ASan/UBSan (dedx_get_ion_list(-1000) reads garbage) and bounds-checked against the table's actually-populated range.

Verified: full ctest (33/33) green under -DDEDX_WERROR=ON and under -fsanitize=address,undefined with leak detection; both issues reproduced-then-fixed under ASan before landing; clang-format-19 clean.

[grzanka]

The build_and_test (windows-latest) failure on 734c562 was a real portability bug, not a flake: LOAD_FAILURE_BASELINES[] = {} is a GCC/Clang extension (empty-initializer zero-size array), which MSVC rejects (C7757), along with the resulting zero-length load_failures_by_code[0] struct member. Fixed in 996d55a with a one-element {-1, 0} sentinel (-1 is never a real DEDX_ERR_* value, so it can't accidentally match a real failure) — verified load_failures still reads 0, full ctest green plain and under ASan+UBSan, clang-format-19 clean, Valgrind clean. I also read through the rest of 734c562 (the mstar_state threading through element_supported_for_ion()/material_id_supported() and the dedx_get_ion_list() bounds check) — looks correct and consistent with how load_compound()/resolve_mstar_mode() actually resolve state at runtime.

---

Generated by Claude Code

[Copilot]

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated no new comments.