Skip to content

Health-audit fixes + behavior-preserving split of calc_orchestrator God Object#7

Merged
ackness merged 17 commits into
masterfrom
fix/health-audit-2026-06-27
Jun 28, 2026
Merged

Health-audit fixes + behavior-preserving split of calc_orchestrator God Object#7
ackness merged 17 commits into
masterfrom
fix/health-audit-2026-06-27

Conversation

@ackness

@ackness ackness commented Jun 28, 2026

Copy link
Copy Markdown
Owner

Summary

Two phases from a full health-audit of the workspace, both verified behavior-preserving against the parity gate.

Phase 1 — Audit fixes (correctness / robustness / docs)

  • fix(parser): form value extractor now rejects non-numeric captures (FormReject::Nil → text becomes Unsupported) instead of silently injecting a value=0 Modifier into the ModDb. Zero behavior change for inputs that parse today.
  • refactor(env_finalize): forward_enemy_modifiers dedup no longer keys on format!("{m:?}") (Debug-format coupling + per-modifier String alloc on a hot path); uses a local field key (f64 via to_bits) instead. Also documents why the charm-limit drop order is already deterministic (single shared ModName bucket, insertion order).
  • fix(build-import): surface silently-skipped config inputs / items / text fragments in the PoB2 XML importer (separate Eof from Err, count skipped items, keep lossy text on unescape failure). Import tolerance preserved (parity still 0 parse/calc failed).
  • fix(calc-cache): include OrchestratorOptions (base_input + extra_modifier_texts) in the cache key — same Build with different options previously returned a stale cached OutputTable. Adds a regression test; documents that BuildSnapshot::content_hash is not a sufficient calculate_with_data cache key.
  • docs: correct workspace/crate/command drift in CLAUDE.md + architecture docs (15 members not 14; pobr-item is implemented; pobr-trade is a standalone leaf; parity test target is parity not ninja_parity; pobr-data-adapter default is warn-and-degrade; deprecation banner on the stale 11-implementation-progress.md).
  • test(golden): rename misleading *_GOLDEN_* constants to *_NEUTRAL_* and clarify the suite is a pipeline-smoke + determinism guard (loads no items → all-neutral values), not PoB2 value parity. Assertions/tolerances/values unchanged.

Note: the audit's "charm nondeterminism" finding turned out to be a misdiagnosis — the verify gate's nextest caught it; reverted, with a comment pinning the insertion-order semantics.

Phase 2 — Split the calc_orchestrator.rs God Object (behavior-preserving)

Decomposed the 8782-line calc_orchestrator.rs into a directory module (calc_orchestrator/mod.rs + 9 submodules) via pure code moves + visibility adjustments, zero logic change:

submodule lines responsibility
skill_resolve 1000 main-skill / gem property·quality·level / Kalandra / minions
triggers 928 trigger fixpoint + support judgement (thread-local #1)
stat_map 614 StatMap/curse/debuff translation + STAT_MAP_CTX (thread-local #2)
collect 576 passive nodes / jewel expansion / item·gem collection
buffs 504 herald / aura / buff specs / spirit
skill_mods 452 skill base mods / DoT / corpse / crossbow
weapon 428 weapon contribution / unarmed / local parsing
conditions 306 skill_types → flags/conditions
defence 302 armour / evasion / ES / ward
  • mod.rs retains the 994-line calculate_with_data spine (intentionally not split — that changes data flow, deferred to a separate task), the public entry points, pub types, and the catch-all test block.
  • lib.rs has zero diff (pub mod/pub use is transparent to .rs/mod.rs).
  • Extracted sequentially, one commit per submodule, each independently compiling → clean git bisect.

Verification

  • cargo clippy --workspace --all-targets -- -D warnings — clean
  • cargo nextest run --workspace2092 passed / 0 failed (5 skipped)
  • cargo test -p pobr-build --test paritydef25 84.2% / defcore 91.7% / off 76.2% / dot 54.1% @5%, 0 parse/calc failed — zero drift vs baseline (the proof that the refactor is behavior-preserving)

Deferred (separate tasks, each with a reason)

  • 994-line calculate_with_data spine split (changes data flow, not a pure move)
  • legacy.rs retirement (still the parser fallback path)
  • offensive-side parity tightening (panel-mode hit rate is only 48.8%)
  • restore the parser-rules drift gate (needs vendor a82a33b bump + golden re-baseline)

ackness added 17 commits June 27, 2026 23:44
…lback

Value-consuming forms now route formCap[1] through value1().ok_or(FormReject::Nil),
so a malformed numeric upgrades the whole text to Unsupported instead of injecting a
value=0 Modifier into the ModDb with no error and no unsupported-text record. Non-numeric
forms (FLAG/DMG/DOUBLED) are unaffected. Zero behavior change for inputs that parse today.
…field key

forward_enemy_modifiers dedup no longer keys on format!("{m:?}") (coupled to the
non-contract Debug format + a per-modifier String alloc on a hot path); it now uses a
local DedupSeed key built from semantic fields (f64 via to_bits) + derived PartialEq.
Also documents why the charm-limit drop order is already deterministic (all charms share
one ModName bucket, iterated in insertion order) to prevent a misguided re-fix.
…parser

Separate Eof from Err in config scanning, count and report skipped malformed items, and
keep lossy text on unescape failure instead of dropping fragments. Import tolerance for
the real builds is preserved (parity still 0 parse/calc failed).
get_or_compute keyed only on the build snapshot, so the same Build with different options
(base_input / extra_modifier_texts) wrongly returned the first cached OutputTable. Key is
now combined(snapshot_hash, options_hash). Adds a regression test and documents that
BuildSnapshot::content_hash is not sufficient as a calculate_with_data cache key.
…ture docs

15 members (was 14, add precompile-mods); pobr-item is implemented (edit-state); pobr-trade
is a standalone leaf (does not depend on pobr-data); parity command target is 'parity' not
'ninja_parity'; data-adapter default is warn-and-degrade, --strict-columns to fail-fast;
deprecation banner on the stale 11-implementation-progress.md; refreshed tool list in 02.
…scope

The suite loads no items, so its *_GOLDEN_* values are all neutral empty-build defaults; it
is an end-to-end pipeline smoke + determinism guard, not a PoB2 value parity. Renamed the
constants and rewrote the module doc to say so. Assertions, tolerances and values unchanged.
… mv)

git mv calc_orchestrator.rs -> calc_orchestrator/mod.rs with zero content change.
lib.rs pub mod / pub use is transparent to .rs -> /mod.rs. Prepares for per-cluster
submodule extraction. cargo check -p pobr-build --tests passes.
…preserving

Full gate green: nextest 2092 passed/0 failed; parity def25=84.2 defcore=91.7 off=76.2 dot=54.1 @5% zero-drift; clippy clean. 994-line calculate_with_data spine kept intact (separate future task).
@ackness
ackness merged commit e38fa39 into master Jun 28, 2026
@ackness
ackness deleted the fix/health-audit-2026-06-27 branch June 28, 2026 04:18
ackness added a commit that referenced this pull request Jul 17, 2026
… effect chain

Backlog item #7-1 (pre-0.5.4b gap, golden unchanged across the flip).
Oracle decomposition on monk-invoker-frost-bomb pinned two independent
root causes:

1. Archmage buff not applied: 'DamageGainAsLightning' BASE 4 per 100 max
   Mana (vendor act_int.lua:229-231, PerStat Mana div 100) was dropped at
   two points of the player-buff statmap channel:
   - SkillType tag arm only accepted Attack/Spell and rejected 'neg'
     (Archmage payload is Channel-neg + Spell). Now resolves any enum
     name via the single-source SkillTypes::from_pob2_name (the old
     whitelist rationale predates full-enum skill_type_bits) and
     translates neg via the new ModTag::SkillTypesNeg (vendor
     ModStore.lua:829-833; new variant keeps the precompiled-cache Debug
     format of ModTag::SkillTypes byte-stable).
   - translate_player_buff_mod_name now admits DamageGainAsLightning
     (same consumer as the existing DamageGainAsFire: gain-as matrix).
   Lightning component now exact vs oracle (stored 1771.52 vs 1771.5).

2. Curse chain under-scaled Elemental Weakness (enemy resist 9 vs oracle
   -7; oracle enemyMitigation.resistMods: config 50, frost-bomb exposure
   -12, EW -45):
   - curse gem stats were fetched at raw gem level; vendor applyGemMods
     (CalcSetup.lua:410-435) applies '+N to Level of all Spell Skills'
     to every gem effect (EW 19+8 -> 27, payload -58 -> -66).
   - the skill-local CurseEffect segment of the curse multiplier
     (CalcPerform.lua:2423/:2427 reads skillModList) was missing: EW
     quality (+10) and group supports judged compatible via
     judge_group_supports (Heightened Curse +25). New
     stat_map_engine::curse_local_effect folds bare CurseEffect INC/MORE
     from the statmap global section; BuffSpec carries it as
     local_effect_inc/local_effect_more into the buff_pass curse branch.
   -66 x (1+35/100) x 0.5 (boss CurseEffectOnSelf) = -45, matching
   vendor exactly.

Result: frost-bomb TotalDPS 4143.75 -> 6265.34 vs golden 6264.82
(0.66x -> 1.00x); CombinedDPS 0.66x -> 1.00x; dot 0.87x -> 1.07x.
Collateral: druid-oracle-comet TotalDotDPS 1.04x -> 1.06x (its EW is
absent from vendor's enemy resistMods — a pre-existing PoBR curse-slot
overshoot now amplified; left as a tracked lead).

Baselines (honest ratchet): off @5% 58->59, @10% 64->65; dot @10%
21->23 (@5% net zero: frost-bomb CombinedDPS in, druid dot out).
Also adds POBR_DBG_ENEMYMIT per-type enemy mitigation dump used for the
oracle diff.
ackness added a commit that referenced this pull request Jul 17, 2026
…thDotDPS 1.36x overestimate

Backlog item #7-2 (pre-0.5.4b gap; golden byte-identical across the flip).
PoBR overestimated sorceress-chronomancer-essence-drain WithDotDPS by a
phantom hit: TotalDPS 120.87 where PoB2 has 0 (WithDotDPS 455.87 vs 335;
TotalDotDPS was already exact at 335).

Root cause is in the data pipeline, not the calc: GGG's
GrantedEffectStatSets carries a RemoveStats column (community dat-schema
name: IgnoredStats) listing stats a stat set removes from the base-set
merge. PoB2's export zeroes those values (Export/Scripts/skills.lua:
572-597) — Essence Drain's 'Damage over Time' set removes the projectile
set's spell_min/max_base_chaos_damage, making it a pure-DoT set (vendor
act_int.lua rows {0, 0, dot}). PoBR's pipeline never downloaded the
column, so the adapter's additional-set merge kept the concatenated hit
damage (62/115 at gem level 14) and the engine produced a hit.

Changes:
- pipeline/config.json: add IgnoredStats to the GrantedEffectStatSets
  column list (vendor Export/spec.lua calls it RemoveStats; verified
  against the community schema validFor=2 table, column 8). Tables
  re-extracted offline from the cached 4.5.4.3 bundles; all other
  adapter inputs byte-identical to the previous extraction.
- pobr-data-adapter stat_sets.rs: apply the vendor zeroing semantics —
  for each set, zero the first occurrence of every ignored stat in the
  (merged) level row; missing column degrades to no-removal (warn-only,
  consistent with the resilience policy). required_columns registers
  the new column.
- data/4.5.4.3/base/granted_effect_stat_sets.json regenerated: 55
  effects change, all of the same shape (DoT/infusion variant sets
  zeroing the base set's damage stats — e.g. Comet Fire-Infused zeroes
  base cold, verified line-for-line against vendor act_int.lua). All
  other regenerated base files byte-identical to the committed ones.
- tools/pob2-oracle: skillInfo now reports the selected statSet index/
  label and ChaosMin/Max/Dot skillData (used to pin this diagnosis:
  the build XML selects statSet 2 via the <StatSetIndex> child).

Result: essence-drain TotalDPS 120.87 -> 0 (golden 0), WithDotDPS /
CombinedDPS 1.36x -> 1.00x. No other parity cell moved.
Baselines (honest ratchet): off @5% 59->60, @10% 65->66; dot @5% 16->18,
@10% 23->25; panel off 40->41 / 41->42.
ackness added a commit that referenced this pull request Jul 17, 2026
…phantom charm Guard, halved Str life, Spirit denominator, pool-name normalization

Backlog items #7-3 and #7-4 (pre-0.5.4b gaps).

#7-3 ritualist TotalEHP 1.10x: the suspected LowLife LifeRecoverable cap
was a red herring — vendor caps only under an explicit conditionLowLife
config input (CalcDefence.lua:2670-2676) and both sides already agree
(LifeRecoverable = LifeUnreserved = 23). The real cause: the curated
special mod 'also_grants_guard' (charm 'Also grants N Guard' -> 491
shared GuardAbsorb) has no vendor counterpart — PoB2's ModParser does
not parse the line at all (oracle AnyGuard=False; the entry itself was
marked verified:false / 'more modeling than PoB2'). Against the PoB2
golden this is a phantom +491 to every max-hit pool. Removed from the
overlay (re-add when vendor models it); precompiled parsed_mods.json
regenerated (the lines now fall to visible Unsupported).
Result: ritualist TotalEHP/5xMaxHit all exact (1.19x chaos -> 1.00x);
monk-twister maxhits 1.06x -> 0.97x, EHP 1.05x -> 0.98x.

#7-4 wolf-pack (oracle enemyMitigation + extraModList pinned each):
- Life 1.11x: Giant's Blood keystone 'Inherent Life granted by Strength
  is halved' parsed to the HalvesLifeFromStrength flag but had no
  consumer — attribute derivation now mirrors vendor
  CalcPerform.lua:500-505 (Str x1 instead of x2, class-baked part
  corrected). 2973.6 -> 2674.1 vs golden 2674 (exact).
- Armour 0.98x: PerStat Spirit denominator was BASE-only (300);
  vendor PerStat reads output.Spirit (336). New
  CalculationSession::spirit_total (calc_spirit_pool) feeds the
  multiplier/stat backfill. Perfidy '+2 Armour per 1 Spirit' now 672;
  Armour 18169.78 -> 18579.90 vs golden 18580 (exact).
- Mana 0.94x -> 0.99x and gemling Life/Mana/5xMaxHit/EHP all -> 1.00x:
  the overlay-common 'threshold_10_{red,blue}_support_gems_5pct'
  entries emitted vendor names Life/Mana, which are dead buckets in
  PoBR (pool aggregation reads MaximumLife/MaximumMana per the parser
  name_map) — normalized.
- ChaosMaxHit/EHP remainder decomposed, NOT closed (companion project):
  vendor extends every hit pool by ally absorption — the wolf companion
  takes CompanionAllyDamageMitigation=10% of damage capped by
  TotalCompanionLife=2262, i.e. pool/(1-0.10)=3826.67 vs raw 3444
  (oracle pinned; CalcDefence.lua:3567-3595 allies layer). PoBR has no
  companion actor yet, so maxhits sit at ~0.82x honestly (previously
  masked to ~0.96x by the phantom guard).

Also: POBR_DBG_EHPPOOL pool-decomposition dump (ehp.rs) and
POBR_DUMP_DEFENCE on the dump_components example (both used for the
oracle diffs above).

Baselines (honest): def 25-col @5% 413->425 (@10% 434 unchanged),
def core-8 @5% 140->142; off/dot/panel unchanged.
ackness added a commit that referenced this pull request Jul 17, 2026
…modeling

Backlog sweep #7 removed the also_grants_guard curated entry (phantom
Guard pool that vendor ModParser never parses). Two consumers still
pinned the old modeled behavior and slipped through #7's targeted gate:
the charm ingest test now asserts the guard line is loudly reported as
unsupported (matching PoB2), and the parse-coverage ratchet baseline
drops 0.803738 -> 0.803271 for the two corpus lines that honestly
returned to unsupported.
ackness added a commit that referenced this pull request Jul 18, 2026
… with #7

memory_backend_matches_dir_backend only staged the version directory, so
the P1-3 overlay-common layer was silently absent from the in-memory
backend (production wasm bundles it via web sync-data). Stage it under
the overlay-common/ key prefix the loader resolves.

Two unsupported_specials tests still pinned the pre-#7 also_grants_guard
phantom modeling; guard lines now assert loud unsupported reporting and
no hit-pool extension, matching PoB2. The wasm suite was in no wave's
merge gate, which is how both slipped through.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant