fix(ipi): reject unknown atom names

[njzjz]

Summary

• look up coordinate-file atom names without std::map::operator[];
• reject names missing from atom_type instead of inserting them as DeePMD type 0;
• catch conversion/configuration errors locally before model loading, print a stable diagnostic, and return exit code 1;
• add a backend-independent CLI regression proving validation occurs before model loading.

Impact

Type 0 is a valid model type. The previous operator[] lookup silently inserted every unknown name with value 0, so a typo or incomplete atom_type mapping could produce plausible-looking but scientifically incorrect energies and forces.

Why existing tests missed this

The existing i-PI tests use complete O/H mappings and exercise successful socket and inference workflows. None supplies an unknown coordinate-file name, so the default-insertion behavior was never observed.

The new test uses atom Xx with only O configured and deliberately points graph_file at a missing model. It requires the unknown atom diagnostic and exact exit code before any model-loading error can occur.

Exception boundary and CodeQL

The first version wrapped the entire driver in a helper called from main. That preserved runtime behavior but caused CodeQL to treat the pre-existing command-line configuration-file access as a newly introduced uncontrolled path flow. The revised implementation keeps file parsing directly in main and limits the exception boundary to Convert construction, which is the operation that can reject an unknown atom name. This avoids suppressing the security scan while retaining the intended stable diagnostic.

Validation

• full C++ build completed successfully and installed the updated dp_ipi;
• incremental dp_ipi target build succeeded after the localized exception-boundary revision;
• backend-independent unknown-name regression passed after the revision;
• complete i-PI test suite previously passed: 7 tests across TensorFlow, PyTorch, and the new validation case;
• Convert compiled separately under C++11 with -Wall -Wextra -Werror;
• both C API and C++ API driver conditional paths passed syntax checks;
• clang-format check passed;
• ruff format .;
• ruff check .;
• git diff --check.

Closes #5648.

Coding agent: Codex
Codex version: codex-cli 0.144.1
Model: gpt-5.6-sol
Reasoning effort: xhigh

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[coderabbitai]

📝 Walkthrough

Walkthrough

Changes

i-PI validation flow

Layer / File(s)	Summary
Atom-name lookup validation source/ipi/src/Convert.cc	Unknown atom names now raise std::invalid_argument instead of defaulting to type 0.
Driver error propagation and regression coverage source/ipi/driver.cc, source/ipi/tests/test_driver_input_validation.py	main catches conversion-construction exceptions, reports the diagnostic, and an end-to-end test verifies failure before model loading.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant dp_ipi
  participant main
  participant Convert
  dp_ipi->>main: start with coordinate and config files
  main->>Convert: construct atom conversion
  Convert-->>main: throw std::invalid_argument for unknown atom
  main-->>dp_ipi: print error and return 1

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR replaces silent map insertion with validation, errors on unknown atom names, exits before inference, and adds a regression test.
Out of Scope Changes check	✅ Passed	The changes stay focused on input validation and the associated regression test, with no clear unrelated additions.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and accurately summarizes the main change: rejecting unknown atom names in ipi.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

source/ipi/driver.cc (1)

52-220: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Add a return statement at the end of run_driver.

Although the while (true) loop makes the end of the function statically unreachable, run_driver is declared to return an int. Unlike main, regular functions do not implicitly return 0. Adding a return 0; at the very end ensures the code complies with C++ standards and avoids potential -Wreturn-type compiler warnings.

Additionally, because there is no break in the while (true) loop, the cleanup code at the end of the function (lines 217-220) is dead code and will never be executed. If an exception is thrown, the process will exit and the OS will reclaim the memory, so the leak is benign, but cleaning it up or using a smart pointer like std::unique_ptr<double[]> would be more robust.

♻️ Proposed refactor

   if (msg_buff != NULL) {
     delete[] msg_buff;
   }
+  return 0;
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@source/ipi/driver.cc` around lines 52 - 220, Add an explicit return 0 at the
end of run_driver after the msg_buff cleanup block to satisfy the function’s int
return contract. Since the while loop has no break and makes that cleanup
unreachable, either remove the dead manual cleanup or replace msg_buff with
ownership-managed storage such as std::unique_ptr<double[]> while preserving its
existing buffer usage.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@source/ipi/driver.cc`:
- Around line 52-220: Add an explicit return 0 at the end of run_driver after
the msg_buff cleanup block to satisfy the function’s int return contract. Since
the while loop has no break and makes that cleanup unreachable, either remove
the dead manual cleanup or replace msg_buff with ownership-managed storage such
as std::unique_ptr<double[]> while preserving its existing buffer usage.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 97b1e83a-8abc-4ca4-b7f0-cdbd13da42f7

📥 Commits

Reviewing files that changed from the base of the PR and between 7d5ad38 and 0c09a60.

📒 Files selected for processing (3)

• source/ipi/driver.cc
• source/ipi/src/Convert.cc
• source/ipi/tests/test_driver_input_validation.py

[codecov]

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 79.57%. Comparing base (7d5ad38) to head (d895434).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
source/ipi/src/Convert.cc	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5793      +/-   ##
==========================================
- Coverage   79.85%   79.57%   -0.28%     
==========================================
  Files        1022     1029       +7     
  Lines      117351   117582     +231     
  Branches     4313     4310       -3     
==========================================
- Hits        93706    93570     -136     
- Misses      22101    22464     +363     
- Partials     1544     1548       +4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[wanghan-iapcm]

Correct fix. Convert used name_type_map[atomname] (std::map::operator[]), which inserts a value-initialized 0 for a missing key -- and type 0 is a valid DeePMD type, so an unknown or mistyped atom name silently produced wrong-but-plausible energies/forces. Switching to .find() + std::invalid_argument, caught in main to return exit 1 with a stable diagnostic before model loading, is the right fix. The regression genuinely fails pre-fix: with a deliberately missing graph_file, the old code reaches model loading (a different error / non-1 exit) instead of the exact 'Unknown atom name' message, which nicely proves validation happens before model init. Keeping file parsing in main to avoid a spurious CodeQL uncontrolled-path finding is a thoughtful touch. LGTM.

Dismissing to re-review through the /code-review skill per process.

[njzjz]

Follow-up on the run_driver nit: I did not add an unreachable return 0;. The function’s while (true) loop has no break, and every explicit termination path returns from inside the loop, so control cannot flow off the end of the non-main function and the current build emits no -Wreturn-type warning. Adding a statement after that loop would not change behavior or make the existing cleanup reachable. Converting the long-lived buffers to RAII would be a separate lifecycle refactor rather than part of the unknown-atom validation fix.

Coding agent: Codex
Codex version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning effort: xhigh

[wanghan-iapcm]

Re-reviewed via the code-review skill. Correct fix: Convert's operator[] silently inserted unknown atom names as the valid type 0; switching to .find()+std::invalid_argument, caught in main around only the Convert construction (file parsing kept in main to avoid the CodeQL uncontrolled-path false positive), returning exit 1 with a stable diagnostic before model loading. All cvt. call sites were converted to cvt->, the unique_ptr+raw-new is the correct C++11 pattern, and the try/catch scope doesn't swallow model-load errors. A reviewer executed the regression and confirmed it genuinely fails pre-fix (exit 134 / unrelated message) and passes post-fix (exit 1 / exact message); both accept and reject branches are covered. One non-blocking, pre-existing note: a missing atom_type key in the config still aborts via an uncaught nlohmann type_error at driver.cc:76 (before the try), so the PR body's broader 'configuration errors' phrasing slightly overclaims -- worth a follow-up, out of scope here. LGTM.