Fix three broadcast defects found by post-merge audit; harden the generator#1581
Fix three broadcast defects found by post-merge audit; harden the generator#1581sbryngelson wants to merge 4 commits into
Conversation
hcid (integer) was broadcast with mpi_p in pre_process's patch loop - the same type-mismatch class as the bc%beg/end fix, moved to the MPI_INTEGER group. post_process never broadcast recon_type and igr_order despite both being consumed on all ranks by s_mpi_decompose_computational_domain (rank-divergent decomposition for MUSCL/IGR post runs); removed from _POST_BCAST_EXCLUDE, and igr_order gains the missing default alongside its case-opt siblings. All pre-existing on master.
…try gaps Audit follow-ups: _mpi_type_for now consults storage_precision (a future real(stp) namelist parameter would have broadcast with mpi_p - silently wrong under --mixed), and an unregistered namelist entry now raises instead of silently skipping (the missing-broadcast class this generator exists to prevent), with an explicit allowlist for post's dead G.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes three pre-existing MPI broadcast defects discovered during post-merge audit and hardens the Fortran broadcast generator to prevent silent omissions/mixed-precision mismatches.
Changes:
- Correct
hcidbroadcast type in pre-process (MPI integer broadcast instead of real/mpi_p). - Add missing default initialization for
igr_order. - Harden generator:
_mpi_type_fornow accounts forstorage_precision, and missing-registry namelist entries now raise unless explicitly allowlisted.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| toolchain/mfc/params/generators/fortran_gen.py | Adds storage-precision-aware MPI type mapping and prevents silent skips for unregistered namelist variables (with an explicit allowlist). |
| src/pre_process/m_mpi_proxy.fpp | Broadcasts hcid with MPI_INTEGER and removes it from the mpi_p real broadcast list. |
| src/common/m_global_parameters_common.fpp | Initializes igr_order to a default value to avoid undefined behavior. |
| continue # no registry entry — skip (e.g. 'G' in post is rank-0-only) | ||
| if name in _NO_REGISTRY_ALLOWLIST: | ||
| continue | ||
| raise ValueError(f"namelist var {name!r} ({target}) has no registry entry - " "register it or allowlist it; a silent skip here means a " "silently missing broadcast") |
|
Addressed - the message is now one explicit multi-line f-string (the implicit concatenation was a formatter artifact). Thanks. |
|
Follow-up hardening on the generator/manual-residue boundary, from the other side: a new precheck/CI source-lint rule ( |
Claude Code ReviewHead SHA: f2e661e Files changed:
FindingsPrecision bug: stp scalar params land in
The PR correctly teaches # changed line
mpi_type = _mpi_type_for(pdef.param_type, getattr(pdef, "storage_precision", False))
if mpi_type == "MPI_INTEGER":
int_vars.append(name)
elif mpi_type == "MPI_LOGICAL":
log_vars.append(name)
else:
real_vars.append(name) # ← mpi_io_p params fall here tooThe unchanged _emit_bcast_group(lines, real_vars, "mpi_p") # ← hardcoded mpi_p for the whole listAny namelist-bound scalar with The array-dim path is fixed correctly ( Fix: split |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1581 +/- ##
=======================================
Coverage 60.94% 60.94%
=======================================
Files 82 82
Lines 19922 19923 +1
Branches 2924 2924
=======================================
+ Hits 12141 12142 +1
Misses 5805 5805
Partials 1976 1976 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Post-merge audit of the #1550-#1572 refactoring series: four independent verification passes over merged master (squash-tree integrity, end-to-end NFC equivalence, first-principles broadcast re-derivation, hostile fresh-eyes review of the riskiest artifacts). Zero regressions from the series itself; the audit instead surfaced three pre-existing defects and two hardening gaps, all fixed here.
Fixes #1579 (hcid integer broadcast as mpi_p in pre_process — moved to the MPI_INTEGER group).
Fixes #1580 (post_process never broadcasts recon_type/igr_order consumed by the all-ranks domain decomposition; igr_order additionally gains its missing default — found independently by two audit passes).
Hardening (second commit):
_mpi_type_fornow consultsstorage_precision(a futurereal(stp)namelist parameter would have broadcast withmpi_p— silently wrong under--mixed), and an unregistered namelist entry now raises instead of silently skipping (the missing-broadcast class the generator exists to prevent), with the one verified-dead exception (post'sG) allowlisted explicitly. The four surviving_POST_BCAST_EXCLUDEentries were each verified benign with consumer-level evidence during the audit.Full golden suite (4-way sharded) running on the fix commit; toolchain pytest green; precheck green.