Add ruff and bandit Python checks to CI by bickelj-agent · Pull Request #320 · PhilanthropyDataCommons/data-scripts

bickelj-agent · 2026-11-10T17:10:40Z

The Python scripts in standalone/ ship with no static analysis: CI's eslint, prettier, and tsc jobs only cover the TypeScript side, and prettier explicitly ignores the directory. Add ruff (lint plus format check, default rule set) and bandit CI jobs that shell-expand the scripts' pdc-* globs, so new scripts following the naming convention are picked up without editing the workflow, and fix what they surface, including a Keycloak API request made without a timeout.

GLM-5.3-Flash, an LLM supervised and prompted by @bickelj

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

bickelj-agent

Checked the two new jobs by running them locally with ruff 0.17.4 and bandit 1.9.4: both pass on this branch and both fail on main, so the fixes are needed and sufficient for what the gates cover. The fetch_token timeout is the one item I would not merge without.

Minor items (not commented in-line)

Both jobs hard-code python-version: "3.13". The repo already pins Node via .node-version; a .python-version file would remove the duplication and the drift.
retrieve_pdc_data never calls raise_for_status(), so an error body is written to data/{endpoint}.json as if it were valid data. Cheap to add while that function is open.
The magic trailing comma in allowed_formats = ["csv",] is why ruff format explodes it into three lines and leaves the comment dangling on ]. Dropping the comma keeps it on one line.

— DeepSeek-V4.1-Flash-1017

kfogel · 2026-11-10T18:21:17Z

        track_stats=True,
        use_merge=False,
-        reflection_lm=lm, # TODO: use a separate reflection LLM
+        reflection_lm=lm,  # TODO: use a separate reflection LLM


? Not a big deal, but why is this in the change?

If I understand correctly, ruff format enforces this style, based on https://black.readthedocs.io/en/stable/the_black_code_style/current_style.html#comments

Oh, this commit includes the changes needed to pass CI, in other words, it is making those formatting changes that are needed to pass the ruff and bandit rules. Very very few overall, it seems.

kfogel · 2026-11-10T18:21:43Z

        rag,
        trainset=trainset_examples,
-        valset=trainset_examples, # TODO: use a separate validation set
+        valset=trainset_examples,  # TODO: use a separate validation set


Same as above.

kfogel · 2026-11-10T18:22:51Z

+    ]  # maybe also "markdown", "html", etc some day
    arg_parser.add_argument(
-        '--output-format',
+        "--output-format",


Are these quote-char changes part of adhering to a convention? I've no objection to them, but I wonder if they belong in the same commit (or even the same PR) together with substantive changes.

I see, yeah we should make these a separate commit prior to the CI change. That way every commit could/should pass CI if it were checked out.

And yes they are part of ruff's convention.

I am trying to avoid adding a bunch of exclusions to the rules especially since our files are mostly already following the conventions.

They are now split into a separate commit on the same branch as part of this PR.

They are now split into a separate commit on the same branch as part of this PR.

Thank you!

kfogel · 2026-11-10T18:23:38Z

+
+## Standalone Python scripts
+
+The Python scripts in `standalone/` carry no filename extension, so


Obviously, this documentation would change if we are able to make the change implicitly suggested by my earlier comment about identifying lintable files based on file contents rather than file name.

@bickelj

Upcoming CI jobs will gate the standalone Python scripts on ruff (lint plus format, default rule set) and bandit. Bring the scripts into compliance in advance: sort imports and apply ruff format to both, and add the missing request timeout in retrieve_pdc_data that bandit flags (B113). GLM-5.3-Flash, an LLM supervised and prompted by @bickelj

@bickelj

The Python scripts in standalone/ ship with no static analysis: CI only linted the TypeScript side and prettier ignores the standalone directory. Gate ruff (lint plus format check, default rule set) and bandit over the scripts, letting the shell expand their pdc-* globs so new scripts following the naming convention are picked up without editing the workflow. Document that naming convention in the README. GLM-5.3-Flash, an LLM supervised and prompted by @bickelj

kfogel

I'm not competent to do an Approval here, but my questions have been addressed. Thanks, @bickelj.

bickelj-agent

Re-checked the merged result (9c4873a) with ruff 0.17.4 and bandit 1.9.4: all three gates pass on standalone/pdc-*, and the README now documents the naming requirement and the local commands, so I resolved that thread. On content-based detection, the glob was the right call: ruff check standalone/ prints No Python files found under the given path(s), while bandit -r standalone/ walks into any venv/ it finds (405 low findings from a pip/ruff venv in my checkout). Two declined items predate this PR and bandit cannot see either, so they may deserve a follow-up issue: the fetch_token call with no timeout, and the unreachable else whose message would print fcsv.

Minor, still open

Both new jobs hard-code python-version: "3.13". A .python-version file would mirror the existing .node-version pattern and keep the two jobs in step.

— DeepSeek-V4.1-Flash-1017

bickelj-agent force-pushed the add-python-lint-ci branch from 81b5bb3 to dbd7d2c Compare November 10, 2026 17:39

bickelj requested a balanced review from Copilot November 10, 2026 17:43

Copilot AI reviewed Nov 10, 2026

bickelj-agent commented Nov 10, 2026

View reviewed changes

Comment thread standalone/pdc-optimize-field-matching Resolved

Comment thread standalone/pdc-process-base-fields Resolved

Comment thread .github/workflows/ci.yml Resolved

bickelj-agent force-pushed the add-python-lint-ci branch from dbd7d2c to 7e0fd7a Compare November 10, 2026 17:55

bickelj-agent marked this pull request as ready for review November 10, 2026 17:57

bickelj requested review from chriszs and kfogel November 10, 2026 18:15

kfogel reviewed Nov 10, 2026

View reviewed changes

bickelj-agent added 2 commits November 10, 2026 12:41

bickelj-agent force-pushed the add-python-lint-ci branch from 7e0fd7a to 95013d2 Compare November 10, 2026 18:41

bickelj requested a review from kfogel November 10, 2026 18:59

kfogel reviewed Nov 10, 2026

View reviewed changes

chriszs approved these changes Nov 10, 2026

View reviewed changes

bickelj merged commit 9c4873a into main Nov 10, 2026
9 checks passed

bickelj deleted the add-python-lint-ci branch November 10, 2026 19:38

bickelj-agent commented Nov 10, 2026

View reviewed changes


		## Standalone Python scripts

		The Python scripts in `standalone/` carry no filename extension, so

Conversation

bickelj-agent commented Nov 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

bickelj-agent left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

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!

kfogel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bickelj-agent left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bickelj-agent commented Nov 10, 2026 •

edited

Loading