refactor: build paths with pathlib instead of os.path by adamsrnmsu · Pull Request #153 · sandialabs/sceptre-phenix-apps

adamsrnmsu · 2026-10-01T15:11:58Z

Description

A mechanical os.path → pathlib conversion, limited to files that need no other change: the otsim config writer, the logger file sink, the SCORCH base class, the caldera, hoststats, iperf, kafka listener, opcexport, snort, trafficgen and vmstats components, and the wind_turbine app.

The SCORCH base class now keeps root_dir, files_dir and base_dir as Path objects. Components join onto them directly and convert to str only where minimega or a file-transfer helper needs one. The two components that built paths by string concatenation on base_dir (cc recv and ssh SFTP downloads) now join with pathlib. Those lines are written identically to #154's versions, so the two PRs still merge cleanly.

No behavior change. common/utils.py, SunSpec loading and the files whose path handling also gains validation are converted in #154 instead, next to the validation, because safe_join() returns a Path. That keeps the PRs from overlapping.

Related Issues/PRs

One of four independent PRs: #152, #154, #155. Each is a single commit on main and they can merge in any order.

Type of Change

Refactor (refactor)

Checklist

This PR conforms to the process detailed in the Contributing Guide.
I have included no proprietary/sensitive information in my code or the PR.
I have commented my code, particularly in hard-to-understand areas.
I have made corresponding changes to the documentation.
I have tested my code (describe below).

Testing

make check and make test in src/python on Python 3.12: 697 passed, the same as main. I also merged all five open PRs (#152–#156) together: make check is clean and 744 tests pass.

Additional Notes

The vendored sunspec/models/smdx/manifest.py, which ruff excludes, is deliberately left alone.

🤖 Generated with Claude Code

https://claude.ai/code/session_016KAfcDUSerQCCWwBM9BxAo

adamsrnmsu · 2026-10-01T15:19:43Z

This one is dedicated to you @GhostofGoes

GhostofGoes · 2026-10-02T01:43:07Z

+            cmd_src = str(Path(self.root_dir) / self.exp_name / cmd_file)
+            cmd_dst = str(PurePath("/tmp/miniccc/files") / self.exp_name / cmd_file)


Why not make these Path objects? And then convert to strings in the places they need to be strings? Right now they're becoming strings, then being converted to Paths in a bunch of places.

Done. cmd_src and cmd_dst stay Path/PurePath, and str() only happens at the minimega call.

GhostofGoes · 2026-10-02T01:44:06Z

-                        dst=os.path.join(
-                            self.base_dir, os.path.basename(client["client_log_path"])
+                        dst=str(
+                            Path(self.base_dir)


can self.base_dir be made into a Path?

Done. self.base_dir is a Path in ComponentBase, so the wrappers are gone.

GhostofGoes · 2026-10-02T01:44:55Z

+        e_dir = Path(self.exp_dir) / "opcexport"
+        if not e_dir.exists():
+            e_dir.mkdir()
+        opc_variables_file = str(e_dir / "opc_variables.json")


make it a Path object?

Done. e_dir and opc_variables_file are Paths, and exp_dir is now a Path too.

GhostofGoes · 2026-10-02T01:45:47Z

            script = config_snort["script"]
            executor = config_snort["executor"]

-            logger.info(f"copying {os.path.basename(script)} to {hostname}")


the conversion once, store in a variable, instead of doing 4 times

Done. script_name is computed once and reused.

GhostofGoes · 2026-10-02T01:46:19Z

            script = scripts["trafficServer"]

-            logger.info(f"copying {os.path.basename(script)} to {hostname}")
+            logger.info(f"copying {PurePath(script).name} to {hostname}")


use a variable

Done. script_name is a variable here too.

GhostofGoes · 2026-10-02T01:46:41Z

        self.metadata: Box | None = self.extract_metadata()

-        self.root_dir: str = os.path.join(PHENIX_DIR, "images")
+        self.root_dir: str = str(Path(PHENIX_DIR) / "images")


Make this a Path object

Done. root_dir, files_dir and base_dir are all Paths.

GhostofGoes · 2026-10-02T01:46:58Z

-        self.root_dir: str = os.path.join(PHENIX_DIR, "images")
+        self.root_dir: str = str(Path(PHENIX_DIR) / "images")
        self.files_dir: str = os.getenv(
-            "PHENIX_FILES_DIR", os.path.join(self.root_dir, self.exp_name, "files")


make all of these path objects yeah?

exp_dir is a Path now as well. I also dropped the leftover Path(self.base_dir) wraps in cc and ssh.

adamsrnmsu · 2026-10-02T02:14:24Z

@GhostofGoes Thanks, all addressed in 468a5f7:

ComponentBase.root_dir, files_dir and base_dir are now Path objects, so components join onto them directly and only convert to str where minimega or a transfer helper needs a string (mm_cc_send_wait, mm_send, mm_recv).
caldera keeps cmd_src / cmd_dst as paths, iperf and opcexport use Path throughout, and snort and trafficgen compute each script name once into a variable.
cc recv and ssh SFTP downloads did self.base_dir + "/...", which would break with a Path, so they join with pathlib now. Those lines match fix: validate hostnames and paths built from scenario metadata #154 exactly, so the PRs stay independent.

Still one commit, still independent of the others. All five open PRs merged together pass make check and 744 tests.

GhostofGoes · 2026-10-02T17:03:55Z

+            utils.mm_recv(mm, hostname, f"/var/log/snort/{log}", str(log_path))

-            if log == "snort.stats" and os.path.exists(f"{self.base_dir}/{log}"):
+            if log == "snort.stats" and log_path.exists():


Use 'is_file()' for file existence checks

Mechanical os.path -> pathlib conversion for files that need no other change: the otsim config writer, the logger file sink, the SCORCH base class and the caldera, hoststats, iperf, kafka listener, opcexport, snort, trafficgen and vmstats components, and the wind_turbine app. The SCORCH base class now keeps root_dir, files_dir and base_dir as Path objects, so components join onto them directly and convert to str only at the minimega and file-transfer boundaries. The two components that built paths by string concatenation on base_dir (cc recv and ssh SFTP downloads) now join with pathlib instead. No behavior change. common/utils.py and the files whose path handling also gains validation are converted in the hardening PRs, next to that validation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016KAfcDUSerQCCWwBM9BxAo

GhostofGoes · 2026-10-05T00:24:03Z

@adamsrnmsu @nblair2 @glattercj to make sure internal apps are updated to use pathlib after this one is merged

nblair2 · 2026-10-08T15:42:02Z

It looks like this is using the syntactic surgar Path(foo) / "bar" / "baz.txt" . Should we update the ignition app to also use that syntax instead of Path(foo, "bar", "bas.txt") ?

This was referenced Oct 1, 2026

fix: validate hostnames and paths built from scenario metadata #154

Open

fix: stop scenario metadata from reaching shells and host commands #155

Open

fix(deps): raise Mako and requests floors past known advisories #152

Merged

adamsrnmsu requested review from cmulk and mgaliar October 1, 2026 15:19

adamsrnmsu requested a review from glattercj October 1, 2026 15:20

GhostofGoes suggested changes Oct 2, 2026

View reviewed changes

adamsrnmsu force-pushed the refactor-pathlib branch from 1df233e to b887c56 Compare October 2, 2026 01:51

adamsrnmsu mentioned this pull request Oct 2, 2026

chore(sceptre): remove the unused SunSpec protocol #156

Merged

7 tasks done

adamsrnmsu force-pushed the refactor-pathlib branch from b887c56 to 468a5f7 Compare October 2, 2026 02:14

GhostofGoes suggested changes Oct 2, 2026

View reviewed changes

adamsrnmsu force-pushed the refactor-pathlib branch from 468a5f7 to 2d49208 Compare October 4, 2026 23:53

GhostofGoes approved these changes Oct 5, 2026

View reviewed changes

cmulk merged commit c5c523a into sandialabs:main Oct 10, 2026
3 checks passed

		cmd_src = str(Path(self.root_dir) / self.exp_name / cmd_file)
		cmd_dst = str(PurePath("/tmp/miniccc/files") / self.exp_name / cmd_file)

Conversation

adamsrnmsu commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues/PRs

Type of Change

Checklist

Testing

Additional Notes

Uh oh!

adamsrnmsu commented Oct 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsrnmsu commented Oct 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GhostofGoes commented Oct 5, 2026

Uh oh!

nblair2 commented Oct 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adamsrnmsu commented Oct 1, 2026 •

edited

Loading