You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Adds a single CTest entry \ that runs \ against an NCSX equilibrium.\n\n- Base: main\n- Head: feature/ncsx-meiss-ck-test-only\n- Tests: \ (non-regression, excluding golden_record) passes locally.
PR Type
Tests, Enhancement
Description
Adds comparison script for NCSX guiding-center orbits using Cash–Karp integrator
Compares VMEC field-based vs Meiss coil-based trajectory calculations
Automatically downloads NCSX equilibrium and coils files from remote sources
Generates visual comparison plots of s, theta, and phi coordinates over time
Integrates new test into CMake test suite with 600-second timeout
Diagram Walkthrough
flowchart LR
A["NCSX Files<br/>wout + coils"] -->|Download & Convert| B["Test Data<br/>Directory"]
B -->|Link| C["Work Directory"]
C -->|Run SIMPLE| D["VMEC GC<br/>Cash–Karp"]
C -->|Run SIMPLE| E["Meiss GC<br/>Cash–Karp"]
D -->|orbits.nc| F["Plot Comparison<br/>s, theta, phi"]
E -->|orbits.nc| F
F -->|PNG| G["Output Plot"]
H["CMakeLists.txt"] -->|Register Test| I["CTest Suite"]
Loading
File Walkthrough
Relevant files
Tests
compare_ncsx_meiss_vmec_ck.py
NCSX guiding-center trajectory comparison script
examples/compare_ncsx_meiss_vmec_ck.py
New standalone Python script for comparing NCSX guiding-center orbits
Downloads NCSX wout and coils files from remote URLs if not cached
Converts STELLOPT coils format to SIMPLE's coils.simple format
Runs SIMPLE twice with identical parameters except field type (VMEC vs Meiss)
Reads orbits.nc output and generates comparison plots of s, theta mod 2π, phi mod 2π
Below is a summary of compliance checks for this PR:
Security Compliance
⚪
Symlink traversal
Description: The script creates symlinks in the working directory using os.symlink without sanitizing or checking for symlink attacks, allowing a malicious preexisting symlink at the destination to redirect writes and cause unintended file access or overwrite. compare_ncsx_meiss_vmec_ck.py [115-120]
Description: Untrusted external executable invocation: the script runs a repository-local simple.x binary supplied by the environment without validation, which can lead to arbitrary code execution if the binary is tampered with. compare_ncsx_meiss_vmec_ck.py [121-134]
Referred Code
print(f"Running SIMPLE [{tag}] in {run_dir}")
res=subprocess.run(
[simple_exe, cfg_path],
cwd=run_dir,
capture_output=True,
text=True,
timeout=1800,
)
ifres.returncode!=0:
print(f"SIMPLE run '{tag}' failed")
print("STDOUT:", res.stdout[-2000:] iflen(res.stdout) >2000elseres.stdout)
print("STDERR:", res.stderr[-2000:] iflen(res.stderr) >2000elseres.stderr)
raiseRuntimeError(f"SIMPLE run '{tag}' failed with exit code {res.returncode}")
Unsigned downloads
Description: Downloads files over HTTPS and uses them directly without integrity verification (no checksum or signature), risking supply-chain attacks if the remote content is compromised.
Description: The script exits with sys.exit(1) after printing the missing executable path, which may leak repository structure; prefer a generic error message to avoid disclosing internal paths. compare_ncsx_meiss_vmec_ck.py [176-180]
Referred Code
simple_exe=os.path.join(repo_root, "build", "simple.x")
ifnotos.path.exists(simple_exe):
print(f"Error: SIMPLE executable not found at {simple_exe}")
sys.exit(1)
Excessive logging
Description: On subprocess failure, the script prints full stdout/stderr which may include sensitive environment or system details, potentially leaking information in CI logs. compare_ncsx_meiss_vmec_ck.py [129-133]
Referred Code
ifres.returncode!=0:
print(f"SIMPLE run '{tag}' failed")
print("STDOUT:", res.stdout[-2000:] iflen(res.stdout) >2000elseres.stdout)
print("STDERR:", res.stderr[-2000:] iflen(res.stderr) >2000elseres.stderr)
raiseRuntimeError(f"SIMPLE run '{tag}' failed with exit code {res.returncode}")
Ticket Compliance
⚪
🎫 No ticket provided
Create ticket/issue
Codebase Duplication Compliance
⚪
Codebase context is not defined
Follow the guide to enable codebase context checks.
Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code
Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting
Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data.
Status: Unstructured logs: The script prints free-form messages (downloads, warnings, subprocess outputs) rather than structured logs, which may hinder auditing and could inadvertently include sensitive paths or outputs.
Generic: Security-First Input Validation and Data Handling
Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities
Status: External inputs: Remote URLs and coil file parsing are accepted without validation or integrity checks (e.g., checksum), and subprocess outputs are printed without redaction controls.
To make tests reliable and self-contained, vendor the necessary test data files within the repository instead of downloading them from external URLs during test execution. This removes dependencies on network and external services.
# examples/compare_ncsx_meiss_vmec_ck.py# (URLs are removed, files are assumed to be in the repository)defget_ncsx_file_paths(test_data_dir: str):
wout_ncsx=os.path.join(test_data_dir, "wout_ncsx.nc")
coils_simple=os.path.join(test_data_dir, "coils.c09r00.simple")
# The script should now assume the files exist and fail if they don't.ifnotos.path.exists(wout_ncsx) ornotos.path.exists(coils_simple):
raiseFileNotFoundError(
"Required test data files not found in repository."
)
# The logic for converting coils.c09r00 might be moved to a# separate, one-off script for preparing test data.returnwout_ncsx, coils_simple
Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a critical flaw in the test design; relying on external downloads makes the test suite unreliable and non-hermetic, which is a significant issue for CI stability.
High
General
Show full error logs on failure
To aid in debugging, remove the truncation of stdout and stderr when a subprocess fails, ensuring the full error logs are always displayed.
if res.returncode != 0:
print(f"SIMPLE run '{tag}' failed")
- print("STDOUT:", res.stdout[-2000:] if len(res.stdout) > 2000 else res.stdout)- print("STDERR:", res.stderr[-2000:] if len(res.stderr) > 2000 else res.stderr)+ print("STDOUT:", res.stdout)+ print("STDERR:", res.stderr)
raise RuntimeError(f"SIMPLE run '{tag}' failed with exit code {res.returncode}")
Apply / Chat
Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies that truncating stdout and stderr can hide important debugging information. Printing the full output upon failure significantly improves the script's debuggability, which is valuable for a test script.
Low
Possible issue
Improve symlink creation robustness
Replace os.path.islink(dst) or os.path.exists(dst) with the more robust os.path.lexists(dst) to reliably check for pre-existing files or symlinks (including broken ones) before creating a new symlink.
for src, dst in ((wout_ncsx, wout_link), (coils_simple, coils_link)):
- if os.path.islink(dst) or os.path.exists(dst):+ if os.path.lexists(dst):
os.remove(dst)
os.symlink(src, dst)
Apply / Chat
Suggestion importance[1-10]: 5
__
Why: The suggestion correctly points out that using os.path.lexists() is more robust and idiomatic for checking if a path exists before creating a symlink, as it correctly handles broken symlinks. This improves code quality and reliability.
Low
More
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Adds a single CTest entry \ that runs \ against an NCSX equilibrium.\n\n- Base: main\n- Head: feature/ncsx-meiss-ck-test-only\n- Tests: \ (non-regression, excluding golden_record) passes locally.
PR Type
Tests, Enhancement
Description
Adds comparison script for NCSX guiding-center orbits using Cash–Karp integrator
Compares VMEC field-based vs Meiss coil-based trajectory calculations
Automatically downloads NCSX equilibrium and coils files from remote sources
Generates visual comparison plots of s, theta, and phi coordinates over time
Integrates new test into CMake test suite with 600-second timeout
Diagram Walkthrough
File Walkthrough
compare_ncsx_meiss_vmec_ck.py
NCSX guiding-center trajectory comparison scriptexamples/compare_ncsx_meiss_vmec_ck.py
Meiss)
2π, phi mod 2π
CMakeLists.txt
Register NCSX Cash–Karp comparison test in CMaketest/tests/CMakeLists.txt
gc_ck_ncsx_vmec_vs_meissthat executes thecomparison script