diff --git a/.gitignore b/.gitignore index f68e6826..87892f70 100644 --- a/.gitignore +++ b/.gitignore @@ -173,3 +173,4 @@ tests/data/minimal_data/iteration_1/t3_pdep_qm_budget.yml tests/data/minimal_data/iteration_1/t3_pdep_network_assessments.yml tests/data/pdep_network/iteration_1/t3_pdep_qm_budget.yml tests/data/pdep_network/iteration_1/t3_pdep_network_assessments.yml +docs/contracts/ diff --git a/t3/pdep/explorer/input_file.py b/t3/pdep/explorer/input_file.py index 8cdf0177..6f7b21f8 100644 --- a/t3/pdep/explorer/input_file.py +++ b/t3/pdep/explorer/input_file.py @@ -27,6 +27,7 @@ from dataclasses import dataclass, field from t3.pdep.hashing import hash_bytes +from t3.pdep.hybrid import _MODEL_CHEMISTRY_CALL_NAMES, _validate_model_chemistry_expression from t3.utils.writer import METHOD_LINE_CANDIDATE_RE, METHOD_MAP, rewrite_arkane_method_line # The only top-level Arkane DSL calls this module needs to recognize while walking the source's @@ -1533,6 +1534,29 @@ def _validate_source_statements(tree: ast.Module, text: str, source_path: str) - f"Arkane itself defines in the namespace it loads an input file in. Rebinding it shadows " f"the real one for every statement that follows -- including the directives this module " f"generates and appends -- so a source may not assign to it, however harmless the value.") + # ``modelChemistry`` is the one directive whose real ARC/hybrid value is a bare + # ``LevelOfTheory(...)``/``CompositeLevelOfTheory(...)`` call (which Arkane execs at load + # time), not an ``ast.literal_eval``-able literal. Rather than grow a second model-chemistry + # allowlist here, route it through the SAME structural checker T3 uses when it EMITS this + # directive (``t3.pdep.hybrid._validate_model_chemistry_expression``); the AST is what + # decides exec semantics and the verbatim splice reproduces it, so validating the + # ``ast.unparse``-round-tripped node -- which that string-taking checker re-parses -- is + # equivalent to validating the spliced text. The gate is deliberately on a genuine call + # node to one of the two known names: the checker accepts any NON-call string as a plain + # label (injection-char check only), so a computed ``modelChemistry`` value must keep + # falling through to the refusal below rather than being handed over as a bare label. + if (target == 'modelChemistry' and isinstance(node.value, ast.Call) + and isinstance(node.value.func, ast.Name) + and node.value.func.id in _MODEL_CHEMISTRY_CALL_NAMES): + try: + _validate_model_chemistry_expression(target, ast.unparse(node.value)) + except ValueError as e: + raise ValueError( + f"Refusing to use '{source_path}' as an Arkane explorer/network source: line " + f"{node.lineno} assigns a 'modelChemistry' value that fails structural validation " + f"({_source_snippet(node, text)!r}): {e}. This source's text is spliced verbatim into " + f"a NEW file Arkane will exec, so a malformed model-chemistry call is refused.") from e + continue try: ast.literal_eval(node.value) except (ValueError, TypeError, SyntaxError, MemoryError, RecursionError) as e: diff --git a/t3/pdep/hybrid.py b/t3/pdep/hybrid.py index fefd1b29..4ad7af00 100644 --- a/t3/pdep/hybrid.py +++ b/t3/pdep/hybrid.py @@ -137,7 +137,7 @@ def _model_chemistry_ast_call(value: str) -> ast.Call | None: return None -def _validate_level_of_theory_call(call: ast.Call, value: str) -> None: +def _validate_level_of_theory_call(call: ast.Call, value: str, field_name: str) -> None: """ Structurally validate a ``LevelOfTheory(...)`` call node against the real Arkane ``LevelOfTheory`` constructor schema (see ``_LEVEL_OF_THEORY_FIELD_TYPES``): only keyword @@ -151,6 +151,11 @@ def _validate_level_of_theory_call(call: ast.Call, value: str) -> None: Args: call (ast.Call): The ``LevelOfTheory(...)`` call node to validate. value (str): The original ``model_chemistry`` string (used in error messages). + field_name (str): The name of the field ``value`` came from, as the caller knows it + (e.g. ``QMEnergySettings.model_chemistry`` when validating settings, or + ``modelChemistry`` when validating a directive in an Arkane source). + Reported verbatim in error messages, so a failure names the field the + caller actually has rather than one from an unrelated context. Raises: ValueError: If ``call`` has positional args, ``**kwargs``, a duplicate keyword, a keyword @@ -159,33 +164,33 @@ def _validate_level_of_theory_call(call: ast.Call, value: str) -> None: ``method`` keyword. """ if call.args: - raise ValueError(f"QMEnergySettings.model_chemistry's LevelOfTheory(...) call must not have positional " + raise ValueError(f"{field_name}'s LevelOfTheory(...) call must not have positional " f"arguments, got {value!r}.") seen_keywords = set() for kw in call.keywords: if kw.arg is None: - raise ValueError(f"QMEnergySettings.model_chemistry's LevelOfTheory(...) call must not use " + raise ValueError(f"{field_name}'s LevelOfTheory(...) call must not use " f"**kwargs, got {value!r}.") if kw.arg in seen_keywords: - raise ValueError(f"QMEnergySettings.model_chemistry's LevelOfTheory(...) call must not repeat " + raise ValueError(f"{field_name}'s LevelOfTheory(...) call must not repeat " f"keyword {kw.arg!r}, got {value!r}.") seen_keywords.add(kw.arg) allowed_types = _LEVEL_OF_THEORY_FIELD_TYPES.get(kw.arg) if allowed_types is None: - raise ValueError(f"QMEnergySettings.model_chemistry's LevelOfTheory(...) call has unknown keyword " + raise ValueError(f"{field_name}'s LevelOfTheory(...) call has unknown keyword " f"{kw.arg!r}, got {value!r}.") # Compare exact type(), not isinstance(): isinstance(True, int) is True in Python's numeric # tower, so an isinstance check would silently accept e.g. method=True as if it were a str. if not isinstance(kw.value, ast.Constant) or type(kw.value.value) not in allowed_types: - raise ValueError(f"QMEnergySettings.model_chemistry's LevelOfTheory(...) keyword {kw.arg!r} must be a " + raise ValueError(f"{field_name}'s LevelOfTheory(...) keyword {kw.arg!r} must be a " f"literal of type {allowed_types!r}, got {value!r}.") missing = [field for field in _LEVEL_OF_THEORY_REQUIRED_FIELDS if field not in seen_keywords] if missing: - raise ValueError(f"QMEnergySettings.model_chemistry's LevelOfTheory(...) call is missing required " + raise ValueError(f"{field_name}'s LevelOfTheory(...) call is missing required " f"keyword(s) {missing!r}, got {value!r}.") -def _validate_composite_level_of_theory_call(call: ast.Call, value: str) -> None: +def _validate_composite_level_of_theory_call(call: ast.Call, value: str, field_name: str) -> None: """ Structurally validate a ``CompositeLevelOfTheory(...)`` call node against the real Arkane ``CompositeLevelOfTheory`` constructor schema (see @@ -198,6 +203,11 @@ def _validate_composite_level_of_theory_call(call: ast.Call, value: str) -> None Args: call (ast.Call): The ``CompositeLevelOfTheory(...)`` call node to validate. value (str): The original ``model_chemistry`` string (used in error messages). + field_name (str): The name of the field ``value`` came from, as the caller knows it + (e.g. ``QMEnergySettings.model_chemistry`` when validating settings, or + ``modelChemistry`` when validating a directive in an Arkane source). + Reported verbatim in error messages, so a failure names the field the + caller actually has rather than one from an unrelated context. Raises: ValueError: If ``call`` has positional args, ``**kwargs``, a duplicate keyword, a keyword @@ -205,25 +215,25 @@ def _validate_composite_level_of_theory_call(call: ast.Call, value: str) -> None ``LevelOfTheory(...)`` call, or is missing ``freq`` and/or ``energy``. """ if call.args: - raise ValueError(f"QMEnergySettings.model_chemistry's CompositeLevelOfTheory(...) call must not have " + raise ValueError(f"{field_name}'s CompositeLevelOfTheory(...) call must not have " f"positional arguments, got {value!r}.") seen_keywords = set() for kw in call.keywords: if kw.arg not in _COMPOSITE_LEVEL_OF_THEORY_REQUIRED_FIELDS: - raise ValueError(f"QMEnergySettings.model_chemistry's CompositeLevelOfTheory(...) call only accepts " + raise ValueError(f"{field_name}'s CompositeLevelOfTheory(...) call only accepts " f"'freq'/'energy' keywords, got {kw.arg!r} in {value!r}.") if kw.arg in seen_keywords: - raise ValueError(f"QMEnergySettings.model_chemistry's CompositeLevelOfTheory(...) call must not " + raise ValueError(f"{field_name}'s CompositeLevelOfTheory(...) call must not " f"repeat keyword {kw.arg!r}, got {value!r}.") seen_keywords.add(kw.arg) if not (isinstance(kw.value, ast.Call) and isinstance(kw.value.func, ast.Name) and kw.value.func.id == 'LevelOfTheory'): - raise ValueError(f"QMEnergySettings.model_chemistry's CompositeLevelOfTheory(...) keyword {kw.arg!r} " + raise ValueError(f"{field_name}'s CompositeLevelOfTheory(...) keyword {kw.arg!r} " f"must be a LevelOfTheory(...) call, got {value!r}.") - _validate_level_of_theory_call(kw.value, value) + _validate_level_of_theory_call(kw.value, value, field_name) missing = [field for field in _COMPOSITE_LEVEL_OF_THEORY_REQUIRED_FIELDS if field not in seen_keywords] if missing: - raise ValueError(f"QMEnergySettings.model_chemistry's CompositeLevelOfTheory(...) call is missing " + raise ValueError(f"{field_name}'s CompositeLevelOfTheory(...) call is missing " f"required keyword(s) {missing!r}, got {value!r}.") @@ -251,9 +261,9 @@ def _validate_model_chemistry_expression(field_name: str, value: str) -> None: _validate_no_injection_chars(field_name, value) return if call.func.id == 'LevelOfTheory': - _validate_level_of_theory_call(call, value) + _validate_level_of_theory_call(call, value, field_name) else: - _validate_composite_level_of_theory_call(call, value) + _validate_composite_level_of_theory_call(call, value, field_name) # Every QMEnergySettings field carries its own expected-type metadata (see the ``field(...)`` @@ -543,7 +553,7 @@ def write_hybrid_network_input_file(source_path: str, raise ValueError("QMEnergySettings.model_chemistry is required and must not be blank: without it, Arkane " "cannot apply atom energy corrections, so a QM'd transition state's E0 would not be on " "the same energy reference scale as the RMG wells around it.") - _validate_model_chemistry_expression('model_chemistry', energy_settings.model_chemistry) + _validate_model_chemistry_expression('QMEnergySettings.model_chemistry', energy_settings.model_chemistry) if not energy_settings.use_atom_corrections: raise ValueError("QMEnergySettings.use_atom_corrections is False: this directive silently disables " "Arkane's atom energy corrections, so a QM'd transition state's E0 would not be on the " diff --git a/tests/test_pdep/test_explorer_input_file.py b/tests/test_pdep/test_explorer_input_file.py index 522529da..b6bb37a3 100644 --- a/tests/test_pdep/test_explorer_input_file.py +++ b/tests/test_pdep/test_explorer_input_file.py @@ -1094,6 +1094,71 @@ def test_numeric_arithmetic_is_accepted(self, source): _validate_source_statements(ast.parse(source), source, '/nonexistent/source.py') +class TestModelChemistryCallFormIsAccepted: + """ + A real ARC/hybrid network source expresses ``modelChemistry`` as a bare + ``LevelOfTheory(...)``/``CompositeLevelOfTheory(...)`` call, which Arkane execs at load time. + That is not ``ast.literal_eval``-able, so the literal-assignment rule refused it. This branch is + now bridged to ``t3.pdep.hybrid._validate_model_chemistry_expression`` -- the exact checker T3 + uses when it EMITS this directive -- so the two paths agree and no second allowlist is grown. + The exception is keyed strictly to the ``modelChemistry`` target and to a genuine call node; any + other target, and any non-call value, still falls through to the existing refusal. + """ + + @staticmethod + def _refuses(source: str) -> str: + with pytest.raises(ValueError) as exc_info: + _validate_source_statements(ast.parse(source), source, '/nonexistent/source.py') + return str(exc_info.value) + + @pytest.mark.parametrize('source', [ + "modelChemistry = LevelOfTheory(method='wb97xd', basis='def2tzvp')", + "modelChemistry = LevelOfTheory(method='wb97xd', basis='def2tzvp', software='qchem')", + # Explicit `+`, not adjacent-literal concatenation: inside a list of parametrize cases + # the implicit form is indistinguishable from a missing comma, which would silently turn + # this one case into three malformed ones. + ("modelChemistry = CompositeLevelOfTheory(" + + "freq=LevelOfTheory(method='wb97xd', basis='def2tzvp'), " + + "energy=LevelOfTheory(method='dlpno-ccsd(t)', basis='cc-pvtz'))"), + ]) + def test_the_call_form_for_model_chemistry_is_accepted(self, source): + """The bare-call form real ARC/hybrid sources use must load, not be refused as non-literal.""" + _validate_source_statements(ast.parse(source), source, '/nonexistent/source.py') + + @pytest.mark.parametrize('source', [ + # A positional arg -- the checker refuses these; the refusal must surface as a source refusal. + "modelChemistry = LevelOfTheory('wb97xd')", + # An unknown keyword. + "modelChemistry = LevelOfTheory(method='wb97xd', bogus='x')", + # A CompositeLevelOfTheory missing a required field. + "modelChemistry = CompositeLevelOfTheory(freq=LevelOfTheory(method='wb97xd'))", + # A non-literal keyword value smuggled into the call. + "modelChemistry = LevelOfTheory(method=().__class__)", + ]) + def test_a_malformed_call_form_for_model_chemistry_is_still_refused(self, source): + assert 'source.py' in self._refuses(source) + + def test_the_refusal_names_the_directive_this_source_actually_has(self): + """The structural checker is shared with the settings path, whose messages name + ``QMEnergySettings.model_chemistry`` -- a field an Arkane source file does not have. A + reader looking at a refused source must be told about ``modelChemistry``, the thing in + front of them, not about a settings attribute from an unrelated context.""" + message = self._refuses("modelChemistry = LevelOfTheory(method='wb97xd', bogus='x')") + assert 'modelChemistry' in message + assert 'QMEnergySettings' not in message + + @pytest.mark.parametrize('target', ['title', 'basis', 'level_of_theory']) + def test_the_call_form_under_any_other_target_is_still_refused(self, target): + """The exception is keyed to ``modelChemistry`` alone; the call form elsewhere is non-literal.""" + assert 'source.py' in self._refuses(f"{target} = LevelOfTheory(method='wb97xd')") + + @pytest.mark.parametrize('value', ['1 + 2', '().__class__', 'foo("x")']) + def test_a_computed_non_call_model_chemistry_value_is_still_refused(self, value): + """Guards the trap: the checker treats any non-call string as a plain label, so the branch + must gate on a real call node -- a computed ``modelChemistry`` value stays refused.""" + assert 'source.py' in self._refuses(f'modelChemistry = {value}') + + class TestSourceIsNarrowedToNetworkSourceSyntax: """ Codex's round-29 P1 B and P1/P2 D: being a name Arkane defines is not a reason to splice a call