[maxscheurer]

Closes #68

Behaviour-neutral cleanup after the protein, Amber, parametrization-spec and mdp stack (PR 59, PR 63, PR 65, PR 67). It removes compatibility leftovers, dead or write-only code and duplication flagged by the round 3 composite review on PR 59. This is a clean cut: no aliases, fallbacks or migration text. Generated mdps, build hashes, topologies and cache layouts stay identical.

Release-notes line: the old parametrization: cgenff | smirnoff | pdb2gmx values and the parametrization_config block were removed in the previous release. Use parametrization.family with protein, small_molecules and water_model.

The implementation plan is posted as a comment below.

[maxscheurer]

Implementation Plan

Analysis

The protein, Amber, parametrization-spec and mdp stack (PR 59, PR 63, PR 65, PR 67) is merged into develop as c6bd73f, which is the same tree as the reviewed tip eed1131. The round 3 composite review on PR 59 (findings 16 to 27) found compatibility leftovers, dead or write-only code and duplication. This PR removes them as a clean cut with no behaviour change: generated mdps, build hashes, topologies and cache layouts must stay byte-identical.

Main points from the exploration:

• Settings.DEFAULT_CONFIG has more users than the issue lists. The issue names conftest.py, test_csv_backend.py and test_push.py. The attribute is also read by test_push_analysis.py (11 sites), test_push_artifacts.py (9 sites) and test_sync_config_local_paths.py. That makes about 35 copies of the same patched_init loop. The twin table holds empty paths, while _get_defaults() returns real platformdirs paths. Tests that relied on empty paths must set them explicitly. get_data_dir() does not create directories, so switching has no side effects.
• Finding 21 is mostly neutral; only provenance.json needs care:
  • protein is declared on the base _FamilySpec. OpenFFParametrization only re-declares it with a "Not supported" description and adds a _check_no_protein validator. resolve() already rejects protein for every OpenFF simulation type. I checked that removing the override keeps the resolved dump, and so the hash, identical. Only the error text changes: "does not apply to simulation type …" replaces "use family 'amber'".
  • Dropping provenance.json itself would change the cache layout, so it is still written.
• The topology walkers differ in small ways. The differences are inline-comment stripping, case of section names, whether # lines are skipped, early exit after the target section, and keeping the raw line for rewriting. Shared generators must keep each caller's current results. Two walkers that copy raw blocks (extract_reusable_parts_from_cgenff_gmx_top, which depends on CGenFF marker strings, and _extract_defaults_block) are outside the generator's model and stay as they are.
• Import cycle. utils/setup_utilities.py imports models.composition. The shared N_AVOGADRO and unit constants therefore cannot live in setup_utilities.py if models/composition.py is to import them; they need a leaf module.
• The 6.022e23 values are equal. The literal in models/composition.py has the same value as N_AVOGADRO (6.022e23), so unifying them is numerically neutral.
• charmm36m-ljpme. It is reachable only through download_all_forcefields() (the config wizard). CharmmParametrization already rejects the name. Removing the registry entry leaves the validator in place, so a user's local LJ-PME bundle is still refused with the same error.

Deliverables

One PR into develop, split into reviewable commits in this order:

1. Compatibility leftovers (issue section 1, findings 16 to 19)

   • Delete "Migrating from the old keys" from parametrization.mdx. No other page links to its anchor. The one-line breaking-change note goes into the PR body for the release notes. There is no changelog file in the repo.
   • Delete Settings.DEFAULT_CONFIG and its comment. All test users switch to Settings._get_defaults(). Where the ~35 identical patched_init bodies can become one conftest.py helper, do that.
   • LNP leniency:
     • Reword the "pre-existing behavior" and "relies on" comments in build.py and simulation/openmm_utils.py so they describe the current behaviour.
     • Report the missed target in finish_compression with loguru logger.warning instead of print.
     • Keep the require_convergence and require_target_density flags.
   • Make these parameters required:
     • forcefield in _apply_protonation_states and _resolve_protonation_residue_name, which becomes str. The if forcefield and guard goes.
     • chains in run_pdb2gmx. The chains = chains or [] line goes.
     • forcefield_types in validate_cgenff_atomtype_compatibility. The ff_dir re-read fallback goes, and so does the ff_dir parameter itself if nothing else uses it.
     • out_path in generate_gromacs_topology_with_protein.

2. Dead or write-only code (issue section 2, finding 20). Grep mdfactory/ docs/content examples SKILLS.md README.md before each deletion.

   • Delete SimulationSettings.identity().
   • Use SimulationType as the annotation of BuildInput.simulation_type. It is the same Literal, so the schema and hash are unchanged.
   • Delete the GromacsProteinParameterSet fields forcefield, water_model and chains, plus the code that fills them in setup/protein.py. Tests that assert them switch to observable behaviour (for example the chain includes) or are dropped.
   • Delete workflows.run_build_from_file and its resolve_protein_pdb_path import. The test_proteinbox.py test becomes a CLI-level test (_build_from_yaml, which already resolves the path). Update SKILLS.md lines 12 and 152 to 159.
   • Make restraint_k (on compress_box and relax_with_protein_restraints) and overshoot_tolerance module constants. finish_compression stops returning a value, and its test is adjusted.
   • Inline the default-only parameters: cutoff on _check_no_protein_clashes and _check_no_solution_clashes, cutoff_angstrom on _build_disulfide_prompt_input, water_name on update_topology_molecules, and tolerance on check_defaults_compatible.
   • Remove charmm36m-ljpme from FORCEFIELD_REGISTRY. Keep the LJ-PME rejection validator and its tests.

3. Duplication in the source (findings 22 to 25)

   • In utils/topology_utilities.py, add iter_topology_entries(...) (section-aware, comment-aware, yields enough to support in-place rewrites) and iter_includes(...). Port the section walkers and include parsers whose semantics fit:
     • topology_utilities.py: read_itp_atoms, write_itp_with_atom_names, both loops of rename_atomtypes, read_topology_defaults, collect_forcefield_atomtype_definitions (its #ifdef tracking either goes into the generator or stays local).
     • setup/protein.py: _parse_protein_molecule_names, _sum_atoms_column, min_gmx_version_for_topology, bundle_forcefield_into_topology.
     • parametrize.py: _read_include_layout, plus the [ molecules ] collector in generate_gromacs_topology_with_protein, if it fits.
   • Extract _iter_prm_atomtypes(prm_files) and use it in both validate_cgenff_atomtype_compatibility and check_atomtype_collisions.
   • Native species:
     • One table in models/species.py mapping SMILES to resname, moleculetype and atom names (O→SOL/OW HW1 HW2, [Na+]→NA, [Cl-]→CL).
     • species.py validation, NATIVE_ION_MOLECULETYPES in parametrize.py, the atom-name lookup in utils/setup_utilities.py, the SingleMoleculeSpecies(... resname="SOL"/"NA"/"CL") literals in build.py and the SOL default in setup/protein.py all use it.
     • Merge the two native GromacsSingleMoleculeParameterSet constructors in parametrize.py. Only the ion ITP name differs (ions.itp vs ions_<water>.itp), and water_model is omitted for CHARMM ions. Both differences must be kept.
   • Type sets and constants:
     • Delete PROTEIN_PDB_SIMULATION_TYPES in prepare.py. That code and the literal tuple at models/input.py:250 use PROTEIN_SIMULATION_TYPES.
     • Derive the simulation-type spellings (the SimulationType Literal, PROTEIN_SIMULATION_TYPES, SMALL_MOLECULE_SIMULATION_TYPES and the CHARMM family's set) from one definition, for example via typing.get_args. Dicts keyed by type stay as data, but a test asserts that their keys cover the Literal.
     • Spell StageSpec.mdp_file in orchestration/stages.py from run_schedules.gromacs.STAGE_FILES. This import direction is allowed; the reverse is not.
     • Add one N_AVOGADRO and named ų→L and ų→cm³ constants in a leaf module. Replace the 1e-27, 1e24, 1e-24 and 6.022e23 literals in build.py, utils/setup_utilities.py and models/composition.py.
   • prepare.py: one relative-pdb_path resolver helper, used by both resolve_protein_pdb_path and the CSV column path.
   • One helper reads build_metadata.json and builds the protein row. Simulation.metadata and analysis/utils.system_chemistry both use it.
   • Docs: state the nonbonded table once in simulation-settings.mdx and the coupling table once. parametrization.mdx (and troubleshooting.mdx, where it restates them) link there instead of repeating values.

4. Test duplication (finding 27)

   • Move WATER, EXAMPLES_DIR and the protein-type constant into conftest.py. The protein type is imported from PROTEIN_SIMULATION_TYPES rather than re-spelled.
   • Create one native force-field stub helper or fixture. This consolidates _amber_ff_dir and the inline charmm36.ff, charmm36m.ff and selected.ff stubs, where they are the same thing.
   • Merge the three copies of the spec rejection and frozen tests into test_parametrization_schema.py. Every case is kept as a parametrized row.
   • The two CGenFF tests in test_parametrization.py (lines 79 and 128) that assign config.parameter_store switch to monkeypatch, or reuse the existing parameter_store fixture.

5. Over-engineering, the neutral subset (finding 21)

   • Drop the small_molecules_model ClassVar and derive it from the small_molecules field annotation. A ClassVar is not dumped, so the hash is unchanged.
   • Pass the minimum GROMACS version as the parsed tuple rather than a string that gets re-parsed. This affects stages.py, simulate.py and apps._build_gmx_version_check, and the generated bash check must stay the same.
   • is_complete relies on the .complete marker plus the artifact files, and provenance.json is no longer required. The file is still written, so the cache layout is unchanged.
   • Remove the OpenFFParametrization.protein re-declaration and _check_no_protein. The inherited base field stays, so the dump and hash are identical. The rejection is still raised by resolve(), and the test_parametrization_schema.py row for {"family": "openff", "protein": "x"} matches the generic "does not apply" message.

6. Builder duplication (issue section 4, finding 26). Separate last commit, with an end-to-end rerun.

   • Shared helpers in build.py for:
     • parametrizer plus topology-function resolution (create_mixedbox_by_compression, create_bilayer_by_compression);
     • solvate plus zero-water guard plus the water species (build_bilayer, build_lnp, build_proteinbox). The two TODOs go.
   • build_proteinbox protein staging reuses the staging core of _stage_protein_topology where the steps are the same. proteinbox bundles the force field via includes while protein_mixedbox copies it, so this difference must be kept.
   • One bounding-box extent/center helper for build_lnp, _center_in_cubic_box, _reimage_protein_centered and build_protein_mixedbox.
   • build.py:119 uses a dilution constant instead of the 0.1 literal. Either reuse PROTEIN_MIXEDBOX_DILUTION_FACTOR under a neutral name, or define a sibling constant.
   • create_sphere uses _universe_from_simulation. This needs a non-periodic option, because create_sphere sets dimensions = None.

Acceptance criteria

• pytest -k "not build" mdfactory passes. No test is removed except tests of deleted code (identity(), run_build_from_file, the deleted GromacsProteinParameterSet fields, the ljpme registry entry), and every merged rejection or frozen case is still present as a parametrized row.
• Pre-commit (ruff format and lint) passes.
• Hash and mdp neutrality. A snapshot taken on develop (c6bd73f) before any change is identical after every commit. The snapshot is a throwaway script under /tmp/$USER, not committed. For every examples/**/*.yaml, every row of examples/**/*.csv and the example protocols in examples/simulation_settings/ and examples/pipeline/, it records BuildInput.hash, model_dump_json() and the text of the four rendered mdp files. The diff result is posted on the PR.
• Topology neutrality. One build per system type is run before and after on a compute node (srun, --account=gdi-compchem-hpc; never on the login node): mixedbox charmm and openff, bilayer, lnp, proteinbox charmm and amber, and protein_mixedbox amber and charmm. For each, topology.top, the local .itp files, the mdps, the [ molecules ] counts and the ion counts are diffed. The comparison is posted on the PR. Only this check covers the commit 6 builder refactor.
• Each of the following finds no definitions or callers left in mdfactory/ docs/content examples SKILLS.md README.md:
  • DEFAULT_CONFIG, def identity, run_build_from_file, PROTEIN_PDB_SIMULATION_TYPES, charmm36m-ljpme
  • 6.022e23 outside the constants module
  • "Migrating from the old keys"
• A grep for backward, legacy, deprecated and compat in the changed files finds nothing that introduces compatibility behaviour.
• The parameter-store layout for OpenFF and CGenFF entries is unchanged (same file names, still written).

Files to create or modify

File	Change
docs/content/docs/user-guide/parametrization.mdx	Delete the migration section; link the nonbonded and coupling tables instead of restating them
docs/content/docs/user-guide/simulation-settings.mdx	Single home for the nonbonded and coupling tables
docs/content/docs/user-guide/troubleshooting.mdx	Link instead of restating values, where applicable
SKILLS.md	Replace the run_build_from_file references
mdfactory/settings.py	Delete DEFAULT_CONFIG
mdfactory/build.py	Comments, native species table, unit constants, required args, inlined clash cutoffs; commit 6 builder helpers
mdfactory/simulation/openmm_utils.py	logger.warning, comment wording, module constants for restraint_k and overshoot_tolerance, finish_compression returns nothing; create_sphere uses _universe_from_simulation
mdfactory/setup/protein.py	Required forcefield and chains; remove write-only fields; registry without ljpme; inline default-only params; port walkers and include parsers
mdfactory/parametrize.py	Required out_path; one native-set constructor; native table; port _read_include_layout; provenance not required by is_complete
mdfactory/utils/topology_utilities.py	iter_topology_entries, iter_includes, _iter_prm_atomtypes; port walkers; required forcefield_types; inline tolerance
mdfactory/utils/setup_utilities.py	Native atom names from the species table; unit constants from the leaf module
mdfactory/models/species.py	The native species table
mdfactory/models/parametrization.py	One simulation-type definition; no small_molecules_model ClassVar; GromacsProteinParameterSet without write-only fields
mdfactory/models/input.py	SimulationType annotation; PROTEIN_SIMULATION_TYPES instead of the literal tuple
mdfactory/models/composition.py	Unit and Avogadro constants
mdfactory/models/simulation_settings.py	Delete identity()
New leaf constants module (for example mdfactory/utils/units.py)	N_AVOGADRO, ų conversions
mdfactory/prepare.py	No PROTEIN_PDB_SIMULATION_TYPES; one relative-path resolver
mdfactory/workflows.py	Delete run_build_from_file and its import
mdfactory/orchestration/stages.py, simulate.py, apps.py	mdp_file from STAGE_FILES; version as parsed tuple
mdfactory/analysis/simulation.py, mdfactory/analysis/utils.py	Shared build_metadata.json read and protein row
mdfactory/tests/conftest.py	Shared WATER, EXAMPLES_DIR, protein types, native ff stub, settings-defaults helper
mdfactory/tests/test_csv_backend.py, test_push.py, test_push_analysis.py, test_push_artifacts.py, test_sync_config_local_paths.py	Use Settings._get_defaults() or the conftest helper
mdfactory/tests/test_parametrization_schema.py, test_proteinbox.py, test_protein_mixedbox.py, test_parametrization.py, test_run_schedules.py, test_simulation_settings.py, test_topology_utilities.py, test_e2e_regressions.py, test_amber_gmx_build.py	Follow the API changes; deduplicate; new tests below

Testing approach

New tests

• test_topology_utilities.py:
  • iter_topology_entries: inline comments are stripped while the raw line is kept for rewriting; blank and ; lines are skipped; # directives are not yielded as data; section names match case-insensitively; behaviour with an early stop.
  • iter_includes: quoted paths, a commented-out ;#include is ignored, .ff/ and local includes, line indices for rewriting.
  • _iter_prm_atomtypes across several files with header and comment lines.
  • The existing tests of the ported walkers stay unchanged and pin each caller's current behaviour. Where a ported function has no direct test today (read_topology_defaults, _parse_protein_molecule_names, _read_include_layout), add one small contract test before porting, so the port is checked against it.
• models or species tests: the native table drives both the SingleMoleculeSpecies resname validation and the atom-name lookup. These are contract checks (a wrong resname is still rejected; water gives three atom names), not a copy of the table.
• test_parametrization.py: the merged native constructor gives the same GromacsSingleMoleculeParameterSet fields as before, for CHARMM water and ions and for OpenFF water and ions. This covers the ions.itp vs ions_<water>.itp difference and water_model on ions.
• Simulation types: the SimulationType args equal the keys of the composition-class mapping in models/input.py, and PROTEIN_SIMULATION_TYPES and SMALL_MOLECULE_SIMULATION_TYPES are subsets of them.
• test_e2e_regressions.py: the non-converged, non-required finish_compression emits a loguru warning (using the existing logger.add sink pattern) and returns None.
• test_run_schedules.py: the existing test_stage_files_match_orchestration_stages already guards the STAGE_FILES link. Keep it.

Updated tests

• Pass forcefield and chains explicitly (the None cases become a non-CHARMM name such as "amber14sb").
• Pass forcefield_types and out_path explicitly.
• Drop the asserts on the deleted GromacsProteinParameterSet fields; test_run_pdb2gmx_multichain_collects_chain_includes still checks the chain includes.
• Move the run_build_from_file test to the CLI path.
• Keep the test that charmm36m-ljpme is rejected.

Evidence beyond CI (-k "not build" deselects the real builders)

• The snapshot diff and the per-type build diff from the acceptance criteria, posted on the PR.
• test_build.py and test_amber_gmx_build.py run on a compute node with gmx and CGenFF available.

Risks and open questions

• Walker ports changing results in rare formats. For example, _sum_atoms_column currently treats #ifdef lines as data and ignores them only because float() fails silently, and _parse_protein_molecule_names would treat a # line in [ molecules ] as an entry. Mitigation: contract tests first, the topology diff on real builds, and keeping out of the generator any walker whose semantics do not fit.
• The DEFAULT_CONFIG switch changes what tests see: real platformdirs paths instead of empty strings. A test that relied on an empty FORCEFIELD_DIR, PARAMETERS or DB path must set it explicitly. Setting MDFACTORY_DATA_DIR to tmp_path in the shared helper would also keep tests off the user's data directory.
• Builder refactor (commit 6). CI does not cover it. Packing is random, so system.pdb coordinates are not compared; topology text, counts and mdps are. The e2e run needs CGenFF (SILCSBIODIR), GROMACS 2026 for Amber, and GPU or CPU time.
• create_sphere. It sets dimensions = None, so _universe_from_simulation needs a small option for non-periodic output. If that makes the helper less clear than the duplicate, leave create_sphere alone and say so on the PR.
• OpenFF protein error text. After the override is removed, {"family": "openff", "protein": …} on a small-molecule type reports the generic "does not apply" message instead of pointing to family amber. Protein simulation types with family openff still get "Allowed families: ['amber', 'charmm']", so users are still pointed to the right family there.
• Docs tables. Linking from parametrization.mdx loses the at-a-glance family summary. A one-line pointer per family that names the scheme without numbers is the compromise.

---

Plan created by mach6

[maxscheurer]

Progress Update

All six plan stages are implemented in one commit (the stages overlap in files). No review has been run.

Changes

• Compat leftovers: removed Settings.DEFAULT_CONFIG (tests use one default_settings_loader) and the docs migration section. forcefield, chains, forcefield_types and out_path are now required, and a missed LNP density target is logged with loguru.
• Dead code: removed identity(), run_build_from_file, the write-only GromacsProteinParameterSet fields and the charmm36m-ljpme registry entry. restraint_k and overshoot_tolerance are module constants, and the default-only parameters are inlined.
• Source duplication: shared iter_topology_entries, iter_includes and _iter_prm_atomtypes; one NATIVE_SPECIES table and one _native_parameter_set; simulation types derived from get_args; utils/units.py; mdp_file from STAGE_FILES; one relative-path resolver; shared build_metadata.json reader and protein row; nonbonded and coupling tables stated once in the docs.
• Test duplication: shared WATER, EXAMPLES_DIR, PROTEIN_TYPES and make_forcefield_dir in conftest.py; spec rejection and frozen tests merged into parametrized rows; two CGenFF tests use the parameter_store fixture.
• Neutral over-engineering: removed the small_molecules_model ClassVar, passed the minimum GROMACS version as a tuple, dropped provenance.json from is_complete (it is still written), and removed the OpenFFParametrization.protein override.
• Builders: added _resolve_engine_functions, _solvate_with_water, _bounding_box, _copy_protein_topology and _native_species. create_sphere uses _universe_from_simulation(..., periodic=False). PROTEIN_MIXEDBOX_DILUTION_FACTOR became LOOSE_PACKING_FACTOR and replaces the 0.1 literal.

Kept local on purpose: collect_forcefield_atomtype_definitions (it tracks #ifdef) and the [ molecules ] collector in generate_gromacs_topology_with_protein, because the shared walker cannot reproduce their semantics exactly.

Acceptance evidence

• Tests: pytest -k "not build and not simultaneously" on a cpu node gives 3425 passed. test_parametrization_simultaneously hangs on the base tree too, so it is deselected. Ruff is clean except PLW0108 at test_proteinbox.py:1062, which is already on develop.
• Hash and mdp snapshot vs c6bd73f: 190 inputs and 160 models (every examples/** yaml and csv row, with each protocol file). Hash, model_dump_json, the four mdps and metadata are byte-identical after every stage.
• Generated bash: the grompp script with the GROMACS version check is byte-identical to the base for 2026 and 2026.1.
• Builds on compute nodes, base vs new, all rc=0:
  • Exact match (topology lines, [ molecules ] counts, itps, mdps, atoms per resname, build_metadata species/ions): mixedbox charmm and openff, proteinbox charmm and amber, protein_mixedbox charmm and amber.
  • Pre-existing nondeterminism: the include order in topology.top and extra_params.itp varies between runs (set iteration order), so I compared them as sorted lines.
  • Bilayer and LNP: itps and mdps are identical. The water, ion and LNP lipid counts differ because the compression is not seeded, so I could not compare those exactly.
• Grep criteria: DEFAULT_CONFIG, def identity, run_build_from_file, PROTEIN_PDB_SIMULATION_TYPES, "Migrating from the old keys" and 6.022e23 outside utils/units.py are all gone. The only charmm36m-ljpme hits are the two rejection test rows. The compat grep on the changed files found nothing new.

Commit: 20b1571

---

Progress tracked by mach6

[gregorweiss]

thanks for the refactoring

[maxscheurer]

Unverified Review Candidates — Pending Assessment

Review round: 1
Reviewed commit: 20b1571

These are unverified candidates. Severity reflects reviewer confidence; do not treat any item as a merge blocker until the assessment comment is posted.

Emphasis requested: simplification.

Critical

None.

Important

1. Simulation-type tests restate production definitions; protein set can shrink silently (test_parametrization_schema.py:374-384, conftest.py:19). CharmmParametrization.simulation_types == ALL_SIMULATION_TYPES and the OpenFF disjointness check repeat the class definitions. PROTEIN_SIMULATION_TYPES is still a hand-written frozenset and now drives PROTEIN_TYPES, so dropping a type would remove parametrized cases without failing. Amber's set is not asserted. (test-reviewer, 80)
2. No unit test for _universe_from_simulation(periodic=False) / create_sphere (simulation/openmm_utils.py:51-71, 444). The non-default branch is only covered by the manual LNP build. (test-reviewer, 80)
3. OpenFFCacheEntry.is_complete no longer requiring provenance.json is not pinned by a test (parametrize.py:292-295). The only is_complete tests are OpenFF-gated and check marker/params removal. (test-reviewer, 80)
4. Builder refactor not delivered as a separate last commit (plan deliverable 6 / issue section 4). Everything is in 20b1571. Rerun evidence for builds exists in the progress comment. (completeness-checker, 85)

Suggestions

5. get_db_path patching still copied inline in ~24 tests (test_push_analysis.py, test_push_artifacts.py, test_push.py); the existing temp_analysis_db / temp_artifact_db fixtures in conftest.py do the same. (simplifier, 88)
6. Redundant conditional sep + comment if sep else "" (utils/topology_utilities.py:398): str.partition already yields "". (simplifier, 95)
7. native_atom_names() is a one-caller wrapper over NATIVE_SPECIES (utils/setup_utilities.py:972-989, caller at 1023); inline it. (simplifier, 82)
8. not self.is_water in is_ion is dead (models/species.py:79-84); water's O has charge 0. (simplifier, 85)
9. _bounding_box returns three values, only one caller uses all (build.py:69; callers 324, 735, 927, 1093); build_lnp computes a dead center and its comment is stale. (simplifier, 80)
10. _build_gmx_version_check rebinds a tuple parameter to a string (orchestration/apps.py:411-412). (simplifier, 80)
11. Hand-rolled stub ff directories remain in test_proteinbox.py (lines 184, 189, 539-542, 592-595) although make_forcefield_dir exists. (simplifier, 80)
12. Pre-existing dead/compat code in touched files (outside plan scope): generate_lipid_structure_legacy in setup_utilities.py:33-105 (raises first, unreachable body), backward-compat re-export in analysis/submit.py:18-26 with its test, re-export comment in orchestration/build.py:28, stale "backwards compatibility" comment in analysis/constants.py:55-57. (simplifier, 80)

Strengths

• Code-reviewer and error-auditor independently traced every topology-walker port, native-species merge, unit constant, builder helper, GROMACS version tuple and the OpenFF protein override removal against origin/develop; no behaviour change found.
• Every planned deliverable is implemented or explicitly kept local with a reason; removed-name greps are clean.
• Parametrized/merged rejection and frozen tests lost no cases; planned new contract tests for the iterators were added.
• Snapshot (190 inputs) and per-type build comparisons are posted.

Agents run: simplifier, code-reviewer, error-auditor (no findings), test-reviewer, completeness-checker

---

Reviewed by mach6

[maxscheurer]

Review Assessment

Findings comment: #69 (comment)

Round 1, reviewed commit 20b1571. Independent assessor ran alone; the parent adjudicated finding 4 (see below).

Classifications

Finding	Classification	Reasoning
1 Simulation-type tests restate definitions	nitpick	Factual: two asserts (Charm == ALL_SIMULATION_TYPES, OpenFF disjointness) are tautological. The "set can shrink silently" claim is wrong: Amber's simulation_types is PROTEIN_SIMULATION_TYPES, so dropping a type fails the Amber example parse tests and the snapshot. Scope: the plan only asks for the derived sets and the planned subset tests, which exist. Practical: no regression slips through.
2 No unit test for _universe_from_simulation(periodic=False)	deferred	Factual: true, but the code matches the old inline behaviour. Scope: plan says commit-6 builders are covered by the end-to-end rerun, which was done. Practical: OpenMM-gated test, follow-up only.
3 is_complete without provenance not pinned by a test	deferred	Factual: true. Scope: the plan requires the change, not a test. Practical: worst case is an unneeded re-parametrization, and provenance is still written (existing test reads it).
4 Builder refactor not a separate last commit	useful follow-up (adjudicated; assessor said merge blocker)	Factual: true, single commit 20b1571, stated openly in the progress comment. Scope: it is a plan Deliverables ordering instruction, not an item of the Acceptance criteria list. Practical: the purpose is reviewability/revertability of the one CI-uncovered part, but this repo squash-merges PRs (linear (#N) history on develop), so the split would not survive the merge, and the builder evidence (rc=0 builds, topology/itp/mdp comparison) is posted. Trigger sequence for harm: none that a split would change. Not a blocker; reviewers can use the builder-related hunks listed in the progress comment.
5 get_db_path patching copied in ~24 tests	deferred	Factual: real duplication but pre-existing on develop, and the fixtures are not drop-in (test_push.py patches other DBs, test_init_* tests init itself, temp_artifact_db is local to test_push_artifacts.py). Scope: plan only covered the patched_init/DEFAULT_CONFIG loops. Practical: test-only.
6 sep + comment if sep else ""	nitpick	Factual: correct, str.partition gives "". Practical: identical results; one-line touch-up.
7 Inline native_atom_names()	nitpick	Factual: one production caller, but it is a public helper used directly by tests in test_models.py and test_protein_mixedbox.py. Practical: inlining saves ~3 lines and rewrites tests.
8 not self.is_water in is_ion	nitpick	Factual: result-neutral, but the guard short-circuits before the RDKit molecule build for water. Practical: none.
9 _bounding_box returns three values	nitpick	Factual: call sites confirmed, but the "dead center in build_lnp" claim is wrong; the plan asked for exactly one extent/center helper. Practical: splitting adds code.
10 _build_gmx_version_check rebinds tuple to string	nitpick	Factual: true; generated bash verified byte-identical. Practical: readability only, one-line touch-up.
11 Stub ff dirs remain in test_proteinbox.py	nitpick	Factual: bare mkdir() calls and nested-parent stubs that make_forcefield_dir can't build without parents=True. Scope: plan says consolidate where they are the same thing. Practical: none.
12 Pre-existing dead/compat code in untouched files	deferred	Factual: true (generate_lipid_structure_legacy unreachable body, compat re-exports in analysis/submit.py, orchestration/build.py, stale comment in constants.py). Scope: issue 68 excludes pre-existing code the stack did not touch. Practical: nothing new introduced. Good candidate for a follow-up issue given the clean-cut policy.

Action Plan

No merge blockers.

---

Assessment by mach6