[maxscheurer]

Closes #60

Adds an open-source parametrization route (parametrization: amber) for proteinbox and protein_mixedbox that needs no CGenFF/SILCSBIO license:

• Protein: Amber ff14SB, using the GROMACS-native amber14sb.ff via pdb2gmx.
• Small molecules: OpenFF Sage.
• Water and ions: native TIP3P and Joung–Cheatham ions from the same force field.

It also adds a GROMACS ≥ 2026 version gate (build and simulate time), a run schedule with Sage-style nonbonded settings, and pixi GROMACS 2026 environments.

Stacked on PR #59 (feature/protein-mixedbox).

Implementation plan posted as a comment below.

[maxscheurer]

Implementation Plan — Amber ff14SB + OpenFF Sage for proteinbox / protein_mixedbox

Base branch: feature/protein-mixedbox (PR 59). This PR targets that branch, not develop.

Problem

The protein system types can only be parametrized with CHARMM36m (pdb2gmx) plus CGenFF, and CGenFF needs a SILCSBIO license. We need a fully open-source route:

• Protein: amber14sb.ff as shipped with GROMACS ≥ 2026, run through pdb2gmx.
• Small molecules: OpenFF Sage, using the existing parametrize_smirnoff_gromacs.
• Water and ions: native TIP3P and Joung–Cheatham ions from amber14sb.ff.

The result must stay simulable with GROMACS. The OpenMM-prep risk (gate 1) has been cleared: the spike passed, with GromacsTopFile matching gmx 2026.3 per-term energies. What remains is mostly generalizing code that hard-codes CHARMM, plus the version gate and a run schedule.

Design decisions (resolving the assessment's open questions)

1. One config for both system types: AmberConfig, type: "amber". Naming is provisional, since issue 61 is a clean break.
   • Fields: forcefield="amber14sb" and water_model="tip3p" (both pinned by a validator), ignore_hydrogens=True, merge_all=False, sage_forcefield="openff-2.2.0.offxml" (same default as SmirnoffConfig.forcefield), and charge_method (same default as SmirnoffConfig).
   • It does not derive from _CharmmFamilyConfig, so the CHARMM validator stays untouched.
   • run_pdb2gmx keeps reading only the shared forcefield / water_model / ignore_hydrogens / merge_all fields. Its type hint is widened.
   • For proteinbox, the Sage fields are accepted and ignored, because there are no small molecules. The docstring says so.
2. Allowed parametrizations. proteinbox → {"pdb2gmx", "amber"} and protein_mixedbox → {"charmm", "amber"}. "amber" is added to the BuildInput.parametrization Literal, the default-config map and the ParametrizationConfig union. No existing model gains a field, so existing BuildInput hashes do not change.
3. Parameter sets for Sage solutes keep parametrization="smirnoff".
   • Each carries a SmirnoffConfig built from AmberConfig (sage_forcefield, charge_method), so it shares the existing SMIRNOFF cache layout. GromacsSingleMoleculeParameterSet is left unchanged.
   • Water and Na⁺/Cl⁻ on this path are native sets: itp points at amber14sb.ff/tip3p.itp or ions_tip3p.itp, with parameter_itp=None and forcefield_itp=<ff>/forcefield.itp.
4. Native short-circuit lives in parametrize_smirnoff_gromacs.
   • It gains an optional native_forcefield_dir, plus the water model used to pick ions_<water>.itp, mirroring parametrize_cgenff_gromacs.
   • When the argument is given, water and ions return before any OpenFF/Interchange work and never touch the SMIRNOFF cache. Behaviour without the argument is unchanged.
   • The per-build FF path is bound with partial, as on the CGenFF path, so the cache decorator is unaffected.
5. [ defaults ] comparison is numeric with an explicit tolerance:
   • nbfunc, comb-rule and gen-pairs must match exactly;
   • fudgeLJ and fudgeQQ must agree within abs=1e-6. This is needed because Sage writes 0.833333 and amber14sb writes 0.83333333333333333.
   • If they match, the Sage block is dropped. Otherwise a ValueError names both files and both values.
6. Run schedule selection is parametrization-aware with minimal machinery.
   • run_schedules.yaml entries may carry an optional parametrization: key.
   • RunScheduleManager methods take an optional parametrization. An entry whose key matches wins; otherwise an entry without the key wins. Today's callers pass nothing and get identical results.
   • New template directory run_schedules/gromacs/proteinbox_amber/ (em/nvt/npt/md), shared by both protein types through template_dir.
   • Issue 62 will replace this mechanism; it is deliberately small.
7. GROMACS version gate. A small table maps force fields that need a minimum GROMACS version (amber14sb → 2026) to that version, so there is one source of truth.
   • Build time: _prepare_protein resolves the binary with check_gmx_available(), parses gmx --version (GROMACS version: 2026.3-conda_forge) and raises a clear RuntimeError before pdb2gmx runs when the requirement is not met. The message names the binary, its version and the required version, and points at the pixi env / [gromacs] gmx_path. The CHARMM paths skip the check.
   • Simulate time: the generated grompp bash script gets an optional version check right after $GMX_BIN detection. It runs on the compute node against the exact binary the job will use, and before any mdrun, so failure is early and unambiguous.
     • The requirement comes from the sim dir's topology.top force-field include (e.g. amber14sb.ff/forcefield.itp → 2026), using the same table.
     • Older builds and CHARMM builds get no check, so their scripts are unchanged.
     • Rationale: a submit-host check in Python could inspect a different binary (for example, a gmx_mpi from the 2024.3 module on the node), and the bundled amber14sb.ff would otherwise be read by GROMACS 2024, which silently ignores _FF_AMBER_LEAP_ATOM_REORDERING.
8. pixi. conda-forge has gromacs 2026.3 nompi for linux-64, osx-arm64 and osx-64 (checked with pixi search). Two additions:
   • feature gmx (gromacs >=2026, nompi, all platforms) with environment gmx = [gmx];
   • feature gmx-cuda (linux-64 only, nompi_cuda build, system-requirements.cuda) with environment gmx-cuda.
   • default and dev stay unchanged, so CI and the lock solve for existing envs are not disturbed. pixi.lock is regenerated.

Deliverables

• AmberConfig and the amber parametrization, valid for proteinbox and protein_mixedbox.
• build_proteinbox accepts Pdb2gmxConfig | AmberConfig.
• build_protein_mixedbox selects its solute parametrizer from the config through one small helper:
  • CHARMM: cgenff + parametrize_cgenff_gromacs, exactly as today.
  • Amber: smirnoff + parametrize_smirnoff_gromacs(smirnoff_config=…, native_forcefield_dir=protein_ff_dir).
  • The same parametrizer is used for the final ion species.
  • The "does not include a CHARMM force field" error text becomes generic.
• generate_gromacs_topology_with_protein gets a Sage branch:
  • checks [ defaults ] numerically against <ff_dir>/forcefield.itp, then drops it;
  • merges Sage [ atomtypes ] and parameters into extra_params.itp;
  • raises on atom-type name collisions, either with a force-field type or between Sage files with different signatures;
  • skips validate_cgenff_atomtype_compatibility.
  • Mixing CGenFF and SMIRNOFF solutes in one topology raises. The CGenFF branch's output is byte-identical to today's.
• Native-name helpers. A generic native_atom_names (OW/HW1/HW2, NA, CL), with charmm_native_atom_names kept as an alias so existing tests and imports still work. topology_atom_names already reads the ITP [ atoms ] names for any solute with a parameter_itp, so it covers Sage solutes; this is verified by a test. The SolutionSpecies error message becomes FF-neutral ("Native water/ion species …").
• Version gate: a parser plus a require_gmx_version-style helper in setup/protein.py, the build-time call, and the simulate-time script check described in decision 7.
• Run schedule: run_schedules/gromacs/proteinbox_amber/{em,nvt,npt,md}.mdp.
  • They are copied from proteinbox/ with only the nonbonded block changed: coulombtype = PME, rcoulomb = rvdw = 0.9, vdwtype = cutoff, vdw-modifier = Potential-switch, rvdw-switch = 0.8, DispCorr = EnerPres.
  • Header comments become Amber/Sage.
  • Two YAML entries are added, and _finalize_protein_build passes inp.parametrization.
• pixi: the gmx / gmx-cuda features and environments, and the updated pixi.lock.
• Examples:
  • examples/proteinbox/lysozyme_amber14sb.yaml plus an amber row in proteins.csv (or a separate proteins_amber.csv);
  • examples/protein_mixedbox/lysozyme_amber_fixed_box.yaml (lysozyme + ethanol) and a water-only Amber variant;
  • an Amber row or CSV for protein_mixedbox.
• Docs: a short amber section in docs/content/docs/user-guide/parametrization.mdx covering:
  • what it uses;
  • the GROMACS ≥ 2026 requirement and the pixi gmx env;
  • that no CGenFF/SILCSBIO is needed.
  • Also a note in installation.mdx that the Amber route needs GROMACS ≥ 2026.

Acceptance criteria

• BuildInput accepts parametrization: amber (with default and explicit AmberConfig) for proteinbox and protein_mixedbox, and rejects it for mixedbox, bilayer and lnp.
• AmberConfig rejects any force field other than amber14sb and any water model other than tip3p.
• Existing CHARMM/CGenFF/pdb2gmx inputs validate as before, with identical BuildInput.hash for the existing example YAMLs. This is checked manually before and after the change and noted in the PR.
• Every pre-existing test passes. The only test edit is the SolutionSpecies message match.
• Lysozyme proteinbox and lysozyme + ethanol protein_mixedbox build end-to-end with GROMACS 2026.3 (pixi gmx env) without any CGenFF/SILCSBIO configured, and the build's grompp -maxwarn 0 validation on em.mdp passes.
• Topology inspection of the final topology.top:
  • it includes amber14sb.ff/forcefield.itp, tip3p.itp and ions_tip3p.itp;
  • its extra_params.itp holds Sage atom types and no [ defaults ];
  • it includes the Sage solute ITPs.
• With a GROMACS < 2026 binary, both of these fail early with a message naming the binary and versions:
  • an Amber build, before pdb2gmx;
  • an Amber mdfactory simulate, inside the first grompp step and before any mdrun.
• The CHARMM grompp scripts are unchanged.
• Amber builds install em/nvt/npt/md.mdp with the Sage-style nonbonded settings. CHARMM builds still install the existing proteinbox mdps.
• EM → NVT → NPT → short production completes through mdfactory simulate for both lysozyme Amber examples:
  • on the cpu partition with the gmx env;
  • on the gpu partition with the gmx-cuda env, --gres=gpu:l40s:1.
  • Results are posted as a PR comment.
• Mocked-gmx unit tests (listed below) pass under pytest -k "not build" mdfactory (CI: pixi run -e dev test).

Files to create or modify

File	Change
mdfactory/models/parametrization.py	AmberConfig (pinned FF/water validator, Sage fields, a helper that returns the matching SmirnoffConfig), union tag amber
mdfactory/models/input.py	"amber" in the Literal, allowed_parametrizations and the default-config map; the merge_all/chains validator applies to Amber too
mdfactory/models/species.py	FF-neutral native-species error message
mdfactory/parametrize.py	native_forcefield_dir short-circuit in parametrize_smirnoff_gromacs; Sage branch, [ defaults ] check and collision check in generate_gromacs_topology_with_protein; generic comments
mdfactory/utils/topology_utilities.py	[ defaults ] parse/compare helper; atom-type collision helper (reuses _atomtype_signature)
mdfactory/utils/setup_utilities.py	native_atom_names plus the charmm_native_atom_names alias; docstring update
mdfactory/build.py	build_proteinbox accepts AmberConfig; parametrizer selection helper used by build_protein_mixedbox; version gate call in _prepare_protein; _finalize_protein_build passes the parametrization
mdfactory/setup/protein.py	GROMACS version parsing and gate, the min-version table, run_pdb2gmx type hint
mdfactory/run_schedules/__init__.py, run_schedules.yaml, run_schedules/gromacs/proteinbox_amber/*.mdp	optional parametrization selection; Amber schedule
mdfactory/orchestration/apps.py, simulate.py (and the stage/script call site)	optional min-version check in the grompp script, with the requirement derived from the sim dir topology
pyproject.toml, pixi.lock	gmx / gmx-cuda features and environments
examples/proteinbox/*, examples/protein_mixedbox/*	Amber YAML and CSV examples
docs/content/docs/user-guide/parametrization.mdx, installation.mdx	Amber route and the GROMACS ≥ 2026 note
tests (below)	new and extended tests

Testing approach

All of the following are mocked: no real gmx and no CGenFF. They run under -k "not build". Tests assert contracts (selected files, raised errors, presence of settings), not incidental exact output.

• tests/test_proteinbox.py
  • AmberConfig: defaults, frozen, rejects a non-amber14sb force field and a non-tip3p water model.
  • BuildInput with amber for both protein types, as YAML-style dict and through CSV row parsing. Rejected for non-protein types.
  • _prepare_protein / version gate with a mocked gmx --version:
    • 2026.3-conda_forge passes;
    • 2024.3 raises before the pdb2gmx subprocess is called;
    • the CHARMM config never calls the gate.
  • run_pdb2gmx with AmberConfig passes -ff amber14sb -water tip3p, reusing _patch_pdb2gmx. The HIE protonation is passed through unchanged (no CHARMM alias).
  • Version-string parser: 2026.3-conda_forge, 2024.3, 2026.0-dev-…, 2026-beta1, and garbage, which raises a clear error.
• tests/test_protein_mixedbox.py
  • parametrize_smirnoff_gromacs(..., native_forcefield_dir=fake_ff): water and Na⁺/Cl⁻ return tip3p.itp / ions_tip3p.itp with parameter_itp=None, and no SMIRNOFF cache directory is created. Done with a fake ff dir under tmp_path and a temporary settings.parameter_store.
  • Parametrizer selection helper: CharmmConfig gives cgenff + parametrize_cgenff_gromacs; AmberConfig gives smirnoff + parametrize_smirnoff_gromacs with the native dir bound.
  • TestCompositeTopology extended with fixture Sage _params.itp files and a fake amber ff_dir:
    • a matching [ defaults ] (0.833333 vs 0.83333333333333333) is dropped and the Sage atom types are present in extra_params.itp;
    • a mismatching [ defaults ] (e.g. fudgeQQ 1.0 or comb-rule 3) raises;
    • an atom-type name collision with an FF type raises;
    • an inter-file conflicting duplicate raises;
    • validate_cgenff_atomtype_compatibility is not called (monkeypatched to raise);
    • small-molecule ITPs are included before the water include;
    • mixed CGenFF + SMIRNOFF raises.
  • topology_atom_names for a Sage solute ITP returns the ITP atom names.
  • native_atom_names and the alias.
  • TestExampleFiles loads the new Amber examples.
  • TestRunSchedule: Amber selection returns proteinbox_amber files whose parsed mdp (via the existing mdp parser) has the six Sage-style settings. The default (no parametrization) still returns the CHARMM files.
• tests/test_orchestration_* (simulate/script tests)
  • The grompp script includes the version check only when a minimum is given.
  • The requirement is derived from a topology including amber14sb.ff/forcefield.itp (2026) and from a CHARMM topology (none).
  • One cheap bash test runs the generated detection and check snippet with a fake gmx on PATH printing 2024.3; it must exit non-zero with the message. A fake printing 2026.3 must pass.
• Real-GROMACS test (opt-in): skipped unless gmx ≥ 2026 is on PATH, and named with build so CI skips it. It runs pdb2gmx amber14sb on a tiny peptide fixture plus a Sage solute composite topology and grompp -maxwarn 0 with the Amber em.mdp.
• E2E (cluster, cpu / gpu partitions, scratch dir mdfactory_e2e/): the two lysozyme Amber examples through build and mdfactory simulate (EM/NVT/NPT/short MD), plus a check with the 2024.3 module that the gate fires. The results go in a PR progress comment.

Risks and open questions

• GPU build. The conda-forge nompi_cuda 2026.3 build has not been tested on the gpu (l40s) nodes; driver and CUDA compatibility is unknown. Fallback: run GPU e2e with an env-local build, or report CPU-only and keep the GPU criterion open.
• Which gmx the jobs see. $GMX_BIN detection prefers gmx on PATH, so the job must activate the pixi env (worker_init / SLURM preamble, related to issue 22). If a module gmx_mpi wins, the new gate makes this fail early instead of silently, which satisfies the criterion. But the e2e run needs the env activated, and the plan must document how.
• Sage [ atomtypes ] column format inside a protein topology. Interchange writes a different column layout from amber14sb's ffnonbonded.itp. grompp parses per line, and the SMIRNOFF mixedbox path already relies on this, but it is only proven by the real-gmx test and e2e.
• Atom-type collisions between two Sage species that share a resname (<RESNAME>_<i>). The new collision check turns this into a clear error. The real fix (unique type naming) belongs to issue 61.
• SMIRNOFF cache key ignores charge_method / water_model. This is a pre-existing problem; the Amber path shares the cache with plain smirnoff builds that use the same offxml. Out of scope (issue 61); mentioned in the docs note.
• Net charge. Sage solutes are neutral for ethanol. Charged solutes with neutralize: false rely on the existing allow_net_charge grompp path, which is unchanged.
• LEaP improper reordering vs the OpenMM prep stage. No difference for lysozyme in the spike. Unusual residues could differ, but only in the restrained prep stages. Accepted.
• pixi lock churn. Adding features re-solves the lock. Keeping gromacs out of default/dev limits the impact on CI.

Out of scope

• ff19SB / OPC / OPC3;
• the SMIRNOFF ff14SB port;
• ParmEd/Interchange OpenMM→GROMACS conversion;
• the 2024.3 cluster module;
• the nomenclature/config refactor (issue 61);
• generated mdp schedules (issue 62);
• aligning the OpenMM prep nonbonded settings;
• the lnp mdp directory, issue 55, and the CHARMM MET1 tdb bug.

---

Plan created by mach6

[maxscheurer]

Progress Update

All deliverables from the plan are implemented, and the end-to-end runs have passed, on CPU and on GPU.

Implementation

• Config: AmberConfig / parametrization: amber, valid for proteinbox and protein_mixedbox. It accepts only amber14sb with tip3p. Its Sage fields map to a SmirnoffConfig. amber is added to the Literal, the allowed-parametrizations map, the default-config map and the config union. Existing BuildInput hashes are unchanged: 12 of 12 example YAML/CSV inputs were checked before and after.

• Native short-circuit: parametrize_smirnoff_gromacs(native_forcefield_dir=..., native_water_model=...) returns tip3p.itp / ions_tip3p.itp from the bundled amber14sb.ff, with no OpenFF work and no SMIRNOFF cache access.

• Solute parametrizer: build_protein_mixedbox picks it through _protein_solution_parametrizer: CGenFF for CHARMM, Sage for Amber. build_proteinbox accepts AmberConfig.

• Sage branch in generate_gromacs_topology_with_protein:

  • compares [ defaults ] numerically against forcefield.itp (fudge factors within 1e-6), then drops it;
  • raises on atom-type collisions, either with the force field or between solutes with different parameters;
  • skips the CGenFF validation;
  • raises when CGenFF and SMIRNOFF solutes are mixed.

  The CGenFF output is unchanged.

• Run schedule: run_schedules/gromacs/proteinbox_amber/ uses Sage-style nonbonded settings (PME, rcoulomb = rvdw = 0.9, Potential-switch, rvdw-switch = 0.8, DispCorr = EnerPres). It is selected through an optional parametrization: key in run_schedules.yaml.

• GROMACS ≥ 2026 gate: one table keyed by force field drives both checks.

  • Build time: _prepare_protein checks the binary before pdb2gmx.
  • Simulate time: the grompp script runs a version check on the compute node, using the requirement derived from topology.top. CHARMM grompp scripts are unchanged.

• pixi: new gmx environment (gromacs >=2026, nompi) and gmx-cuda environment (linux-64, nompi_cuda). The lock entries for default, dev and docs are unchanged.

• Examples and docs: Amber examples for proteinbox (YAML and CSV) and protein_mixedbox (ethanol, water-only, CSV). An amber section in parametrization.mdx and a GROMACS 2026 note in installation.mdx.

• Tests:

  • test_orchestration_gmx_version.py, including a bash run of the generated check with a fake gmx;
  • Amber tests in test_proteinbox.py and test_protein_mixedbox.py;
  • the opt-in real-GROMACS test test_amber_gmx_build.py.

  Results: pytest -k "not build" gives 3171 passed. The real-GROMACS test passes with the gmx environment.

Deviations from the plan

• Unique Sage atom names. Interchange names atoms by element only (C C O H ...). OpenMM's PDB reader drops atoms whose name repeats within a residue, which broke compression (RMSDForce: Number of reference positions does not equal number of particles). The Sage ITP copies in the protein topology now get unique names (C1 C2 O1 H1 ...), and topology_atom_names gives the coordinates the same names.
• Ethanol example uses 53 M water. The CHARMM example's 55 M water + 1 M ethanol stalls at 1.045 g/mL at 1000 bar with TIP3P + Sage; the target is 1.052 g/mL. 53 M gives a solvent density of about 1.0 g/mL.
• pixi environments. The gmx and gmx-cuda environments need their own solve groups, because joining the default group re-solves default and dev. As a result they get a newer stack (Python 3.13, openff-toolkit 0.19, OpenMM 8.6), and packmol is listed explicitly. gmx-cuda uses the deprecated feature-level system-requirements table, so pixi prints a warning. The newer per-platform syntax would add a CUDA platform to every environment.

E2E results

The runs used no CGenFF/SILCSBIO configuration. Scratch directory: mdfactory_e2e/ (jobs jobs/job_amber_*.sh).

Case	Env / node	Result
proteinbox lysozyme YAML / CSV	pixi gmx (2026.3), cpu	build OK, grompp -maxwarn 0 validation passed
protein_mixedbox lysozyme + ethanol	gmx-cuda, gpu l40s	build OK (67 s)
protein_mixedbox lysozyme water-only	gmx, cpu	build OK
Amber build with GROMACS 2024.3 (gmx_mpi module)	cpu	fails before pdb2gmx: Force field 'amber14sb' requires GROMACS >= 2026, but '.../gromacs-2024.3-.../bin/gmx_mpi' is GROMACS 2024.3 ...
Amber simulate with 2024.3 on PATH	cpu	first grompp step exits 1 before any mdrun: ERROR: the topology's force field requires GROMACS >= 2026, but gmx_mpi (...) is GROMACS 2024.3-spack.
CHARMM build dir, simulate --dry-run	cpu	no version requirement emitted
EM → NVT → NPT → 10 ps MD, proteinbox	gmx, cpu	OK (~210 ns/day)
EM → NVT → NPT → 10 ps MD, lysozyme + ethanol	gmx, cpu	OK (~195 ns/day)
EM → NVT → NPT → 10 ps MD, proteinbox	gmx-cuda, gpu l40s, --slurm	OK (~1100 ns/day, CUDA)
EM → NVT → NPT → 10 ps MD, lysozyme + ethanol	gmx-cuda, gpu l40s, --slurm	OK (~900 ns/day, CUDA)

Topology of the lysozyme + ethanol build. It includes, in order:

1. amber14sb.ff/forcefield.itp
2. extra_params.itp, which holds 9 Sage ETH_* atom types and no [ defaults ]
3. posre.itp
4. the Sage ethanol ITP
5. amber14sb.ff/tip3p.itp
6. amber14sb.ff/ions_tip3p.itp

Related findings (not fixed, out of scope)

• Cross-session cancellation. On exit, the parsl_session "squeue safety net" scancels every parsl-named SLURM job the user owns, including blocks of other concurrent mdfactory sessions. It killed a GPU block mid-NVT when an unrelated session exited.
• Exit code. A failed simulation still exits mdfactory simulate with rc 0. The grompp stderr, including the version-gate message, appears only in the Parsl worker logs.
• Dry-run output. --dry-run prints grompp: <could not resolve command>.
• Duplicated mdp files. The Amber route needed its own copy of four mdp files that differ only in the nonbonded block. Generated mdp schedules are needed, which is issue 62.

Commit: ce2a170

---

Progress tracked by mach6

[maxscheurer]

Unverified Review Candidates — Pending Assessment

Review round: 1
Reviewed commit: ce2a170

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

Critical

None.

Important

None.

Suggestions

1. Tautological "script unchanged without minimum" assertion (test-reviewer, confidence 85) — mdfactory/tests/test_orchestration_gmx_version.py, TestGromppScript.test_no_check_without_minimum. The second assertion compares two calls of the same function with the same default, so it cannot fail. The criterion "CHARMM grompp scripts are unchanged" is only checked by "GMX_VERSION" not in script.
2. test_frozen swallows any exception (test-reviewer, confidence 80) — mdfactory/tests/test_proteinbox.py, TestAmberConfig.test_frozen. pytest.raises(Exception) would also pass if the assignment failed for an unrelated reason. Use pydantic.ValidationError with a valid value.
3. get_gmx_version failure modes untested (test-reviewer, confidence 80) — mdfactory/setup/protein.py. No test for OSError/timeout or unparseable output (both wrapped in RuntimeError naming the binary), or that require_gmx_version never calls the binary for CHARMM force fields.
4. New NATIVE_ION_MOLECULETYPES duplicates two inline ion maps (simplifier, confidence 82) — mdfactory/parametrize.py. The same {"[Na+]": "NA", "[Cl-]": "CL"} mapping now exists three times.
5. scheme_label display string used as control flow (simplifier, confidence 80) — mdfactory/parametrize.py, generate_gromacs_topology_with_protein. The label is compared to "SMIRNOFF" to select branches; a boolean would decouple the two.
6. charmm_native_atom_names alias unused in production code (simplifier, confidence 80) — mdfactory/utils/setup_utilities.py. Only test files use it. The plan deliberately kept it as an alias, so this is a judgement call.

Strengths

• Version gate verified by code-reviewer in bash against fake binaries (older, dev, beta, missing version line), and Python/bash parsers agree; scripts without a requirement are byte-identical to before.
• Run schedule selection leaves existing callers unchanged; the new mdps differ from the CHARMM ones only in the six nonbonded settings.
• Topology generation checked against the real amber14sb.ff layout. The [ defaults ] tolerance is safe for the real file, and atom renaming is applied identically to topology and coordinates.
• The opt-in real-GROMACS test (test_amber_gmx_build.py) passes with GROMACS 2026.3; 190 targeted unit tests pass.
• All plan deliverables and acceptance criteria are present in the code (completeness-checker found no gaps).

Other notes (below threshold, not candidates): the stored parametrization_config on native TIP3P water/ion sets carries SmirnoffConfig's default water_model (opc3.offxml), which is misleading metadata only. test_parametrization.py::test_parametrization_simultaneously (CGenFF-only, concurrent parametrization) failed in one reviewer run; it exercises code this PR does not change, and the base-branch status was not checked.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier

---

Reviewed by mach6

[maxscheurer]

Review Assessment

Findings comment: #63 (comment)

Review round: 1 — Reviewed commit: ce2a170

Classifications

Finding	Classification	Reasoning
1. Tautological "script unchanged" assertion	nitpick	Factual: Partly true; the second assertion compares the function with itself, but the first ("GMX_VERSION" not in script) is a real check. Scope: The criterion is met in the code: with no minimum, the version-check block is an empty string, so the script is byte-identical to before. run_stage tests also assert CHARMM topologies pass no minimum. Practical: A regression would need a non-empty minimum, which those tests catch. No harm.
2. test_frozen uses pytest.raises(Exception)	nitpick	Factual: True; it copies the existing CHARMM test pattern. Scope: No criterion requires a specific exception type. Practical: AmberConfig is declared frozen=True; the test is only loose.
3. get_gmx_version failure modes untested	deferred	Factual: The CHARMM no-op claim is false (covered by test_charmm_configs_skip_gate), and parser-level garbage is tested. OSError/timeout paths are untested. Scope: The criterion asks for mocked tests of the gate; pass, fail-before-pdb2gmx and CHARMM-skip are covered. Practical: Every untested path is fail-closed (RuntimeError before pdb2gmx); only message wording is unpinned.
4. NATIVE_ION_MOLECULETYPES duplicates inline ion maps	nitpick	Factual: True. Scope: The plan requires the CGenFF branch to stay unchanged. Practical: All three maps are identical; no behavioural effect.
5. scheme_label used as control flow	nitpick	Factual: True. Scope: Mechanism is not specified by the plan. Practical: Logic is correct (mixed schemes raise first; parametrization is a strict Literal). Style only.
6. charmm_native_atom_names alias only used in tests	false positive	Factual: Usage observation is true. Scope: The plan explicitly requires keeping the alias so existing tests and imports work. Practical: Removing it would break pre-existing tests and contradict the plan.

Additional notes

• test_parametrization_simultaneously (CGenFF-only stress test) cannot have been caused by this PR: parametrize_cgenff_gromacs, retrieve_or_deposit_parameters and the CGenFF helpers are untouched, and the topology-utility changes are additive. It is most plausibly pre-existing flakiness (lock contention or the external CGenFF binary).
• The stored parametrization_config on native TIP3P water and ion sets carries the default water_model="opc3.offxml". This is cosmetic: nothing reads it on the Amber path and these sets are not persisted. Candidate for issue 61 (pin to tip3p or drop the field).

Action Plan

No merge blockers.

---

Assessment by mach6