From b3d85125abc09c9d18a8ebfa442a338bc6374410 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Tue, 13 Oct 2026 21:30:24 +0400 Subject: [PATCH 01/10] Fix ruff LOG and G findings Pass logging arguments instead of f-strings (G004); each message renders to the same text. Name the economy-comparison router's logger after its module rather than its file path (LOG002). Log the gateway's auth misconfiguration with logger.exception and a fixed message (G201): the traceback already ends with the exception and its message, which the old call also repeated in the message. Co-Authored-By: Claude Opus 5.5 --- .../src/policyengine_fastapi/auth/jwt_decoder.py | 6 +++--- .../src/policyengine_fastapi/opentelemetry/gcp.py | 3 ++- .../src/policyengine_simulation_executor/simulation.py | 2 +- .../policyengine_simulation_executor/simulation_runtime.py | 4 ++-- .../src/policyengine_simulation_gateway/auth.py | 4 ++-- 5 files changed, 10 insertions(+), 9 deletions(-) diff --git a/libs/policyengine-fastapi/src/policyengine_fastapi/auth/jwt_decoder.py b/libs/policyengine-fastapi/src/policyengine_fastapi/auth/jwt_decoder.py index cdc6ce37c..a1356412c 100644 --- a/libs/policyengine-fastapi/src/policyengine_fastapi/auth/jwt_decoder.py +++ b/libs/policyengine-fastapi/src/policyengine_fastapi/auth/jwt_decoder.py @@ -63,10 +63,10 @@ def _validate(self, token: HTTPAuthorizationCredentials | None) -> dict[str, str token.credentials ).key except jwt.exceptions.PyJWKClientError as error: - LOG.error(f"Unable to get public key for JWT validation: {error}") # noqa: TRY400 # callers control the token, so a traceback per bad token would let them flood the logs + LOG.error("Unable to get public key for JWT validation: %s", error) # noqa: TRY400 # callers control the token, so a traceback per bad token would let them flood the logs raise HTTPException(status_code=status.HTTP_403_FORBIDDEN) except jwt.exceptions.DecodeError as error: - LOG.error(f"Unable to get public key for JWT validation: {error}") # noqa: TRY400 # callers control the token, so a traceback per bad token would let them flood the logs + LOG.error("Unable to get public key for JWT validation: %s", error) # noqa: TRY400 # callers control the token, so a traceback per bad token would let them flood the logs raise HTTPException(status_code=status.HTTP_403_FORBIDDEN) try: @@ -78,6 +78,6 @@ def _validate(self, token: HTTPAuthorizationCredentials | None) -> dict[str, str issuer=self.issuer, ) except Exception as error: # noqa: BLE001 # any decode or claim-validation failure must become a 403, never a 500 - LOG.info(f"Invalid bearer token: {error}") + LOG.info("Invalid bearer token: %s", error) raise HTTPException(status_code=status.HTTP_403_FORBIDDEN) return payload diff --git a/libs/policyengine-fastapi/src/policyengine_fastapi/opentelemetry/gcp.py b/libs/policyengine-fastapi/src/policyengine_fastapi/opentelemetry/gcp.py index d2312e5a3..b96ad4f4d 100644 --- a/libs/policyengine-fastapi/src/policyengine_fastapi/opentelemetry/gcp.py +++ b/libs/policyengine-fastapi/src/policyengine_fastapi/opentelemetry/gcp.py @@ -58,7 +58,8 @@ def _get_project_id_from_metadata() -> str | None: return request.urlopen(req).read().decode() except Exception as err: # noqa: BLE001 # best-effort probe: any failure falls back to GOOGLE_CLOUD_PROJECT log.info( - f"Unable to get the google project id from a local metadata service: {err}" + "Unable to get the google project id from a local metadata service: %s", + err, ) return None diff --git a/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/simulation.py b/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/simulation.py index 57203d8fb..d42e5da1c 100644 --- a/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/simulation.py +++ b/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/simulation.py @@ -13,7 +13,7 @@ ) from policyengine_simulation_executor.simulation_runtime import run_simulation_impl -logger = logging.getLogger(__file__) +logger = logging.getLogger(__name__) def create_router(runtime: ObservabilityRuntime) -> APIRouter: diff --git a/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/simulation_runtime.py b/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/simulation_runtime.py index b205d8d70..9cdf0dfc7 100644 --- a/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/simulation_runtime.py +++ b/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/simulation_runtime.py @@ -121,7 +121,7 @@ def setup_gcp_credentials( for k, v in os.environ.items() if "GOOGLE" in k or "GCP" in k or "CREDENTIAL" in k } - logger.info(f"GCP-related env vars: {list(gcp_vars.keys())}") + logger.info("GCP-related env vars: %s", list(gcp_vars.keys())) if previous: logger.info("GOOGLE_APPLICATION_CREDENTIALS already set") @@ -149,7 +149,7 @@ def setup_gcp_credentials( creds_file.write(normalized) creds_file.flush() os.environ["GOOGLE_APPLICATION_CREDENTIALS"] = creds_file.name - logger.info(f"GCP credentials written to {creds_file.name}") + logger.info("GCP credentials written to %s", creds_file.name) yield finally: diff --git a/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/auth.py b/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/auth.py index d7f942bf1..f0ed3fb03 100644 --- a/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/auth.py +++ b/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/auth.py @@ -264,8 +264,8 @@ def require_auth( try: decoder = _get_decoder() - except RuntimeError as exc: - logger.error("Gateway auth misconfigured: %s", exc, exc_info=True) + except RuntimeError: + logger.exception("Gateway auth misconfigured") raise HTTPException( status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail="Gateway authentication is not configured.", From 37c0d141983594134307943c646153825f920bef Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Tue, 13 Oct 2026 21:34:05 +0400 Subject: [PATCH 02/10] Keep naive policy dates explicit for ruff DTZ policyengine.py parses policy period keys with strptime and no zone, so its ParameterValue dates are naive, and the executor's period parsing and the tests that compare against those dates must stay naive too. Mark each kept naive datetime with a reasoned DTZ suppression, and route the policy tests' expected dates through one helper. Co-Authored-By: Claude Opus 5.5 --- .../simulation_runtime.py | 2 +- .../tests/test_simulation_runtime_policy.py | 33 +++++++++++++------ .../tests/test_stage12_runtime.py | 2 +- 3 files changed, 25 insertions(+), 12 deletions(-) diff --git a/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/simulation_runtime.py b/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/simulation_runtime.py index 9cdf0dfc7..09739c4aa 100644 --- a/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/simulation_runtime.py +++ b/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/simulation_runtime.py @@ -212,7 +212,7 @@ def _parse_policy_period_dates(text: str) -> tuple[datetime, datetime | None]: raise ValueError # The wrapper has always parsed dates with strptime, which also takes # unpadded months and days ("2027-4-6"); accept what it accepted. - dates = [datetime.strptime(part, "%Y-%m-%d") for part in parts] + dates = [datetime.strptime(part, "%Y-%m-%d") for part in parts] # noqa: DTZ007 # naive like policyengine.py's own period parsing; its ParameterValue dates are naive start = dates[0] stop = dates[1] if len(dates) == 2 else None if stop is not None and stop < start: diff --git a/projects/policyengine-simulation-executor/tests/test_simulation_runtime_policy.py b/projects/policyengine-simulation-executor/tests/test_simulation_runtime_policy.py index 1c2ee7fb7..6845e540d 100644 --- a/projects/policyengine-simulation-executor/tests/test_simulation_runtime_policy.py +++ b/projects/policyengine-simulation-executor/tests/test_simulation_runtime_policy.py @@ -21,6 +21,15 @@ US_SALT_CAP = "gov.irs.deductions.itemized.salt_and_real_estate.cap.JOINT" +def _naive_policy_date(year: int, month: int, day: int) -> datetime: + """Return midnight on a date as a naive datetime. + + policyengine.py parses policy period keys with ``strptime`` and no zone, so + its ``ParameterValue`` dates are naive; an aware value never compares equal. + """ + return datetime(year, month, day) # noqa: DTZ001 # naive like the dates under test + + @pytest.fixture(scope="module") def uk(): country = _country_module("uk") @@ -59,8 +68,8 @@ def test_uk_interval_policy_preserves_inclusive_stop_date(uk, stop): assert value.parameter.name == UK_RATE assert value.parameter.tax_benefit_model_version is uk.model assert value.parameter.data_type is float - assert value.start_date == datetime(2027, 4, 6) - assert value.end_date == datetime.strptime(stop, "%Y-%m-%d") + assert value.start_date == _naive_policy_date(2027, 4, 6) + assert value.end_date == datetime.strptime(stop, "%Y-%m-%d") # noqa: DTZ007 # policyengine.py's ParameterValue dates are naive assert value.value == 0.21 assert reform_dict_from_parameter_values(policy.parameter_values) == raw @@ -83,9 +92,9 @@ def test_mixed_effective_dates_and_intervals_are_chronological(uk, reverse): (value.start_date, value.end_date, value.value) for value in policy.parameter_values ] == [ - (datetime(2026, 4, 6), None, 0.20), - (datetime(2027, 4, 6), datetime(2028, 4, 5), 0.21), - (datetime(2029, 4, 6), None, 0.22), + (_naive_policy_date(2026, 4, 6), None, 0.20), + (_naive_policy_date(2027, 4, 6), _naive_policy_date(2028, 4, 5), 0.21), + (_naive_policy_date(2029, 4, 6), None, 0.22), ] assert reform_dict_from_parameter_values(policy.parameter_values) == { UK_RATE: { @@ -165,7 +174,7 @@ def test_scalar_in_interval_policy_uses_request_year(uk, period, year): for value in simulation.policy.parameter_values if value.parameter.name == UK_ALLOWANCE ) - assert scalar.start_date == datetime(year, 1, 1) + assert scalar.start_date == _naive_policy_date(year, 1, 1) assert scalar.end_date is None assert scalar.value == 13000 assert scalar.parameter.data_type is int @@ -188,8 +197,8 @@ def test_us_interval_policy_uses_same_public_parameter_values(): assert value.parameter.name == US_SALT_CAP assert value.parameter.tax_benefit_model_version is us.model assert value.parameter.data_type is int - assert value.start_date == datetime(2027, 1, 1) - assert value.end_date == datetime(2027, 12, 31) + assert value.start_date == _naive_policy_date(2027, 1, 1) + assert value.end_date == _naive_policy_date(2027, 12, 31) assert value.value == 50000 assert reform_dict_from_parameter_values(policy.parameter_values) == raw @@ -237,8 +246,12 @@ def recording_compiler(reform, *, year, model_version): @pytest.mark.parametrize( "key, start, stop", [ - ("2027-4-6", datetime(2027, 4, 6), None), - ("2027-4-6.2028-4-5", datetime(2027, 4, 6), datetime(2028, 4, 5)), + ("2027-4-6", _naive_policy_date(2027, 4, 6), None), + ( + "2027-4-6.2028-4-5", + _naive_policy_date(2027, 4, 6), + _naive_policy_date(2028, 4, 5), + ), ], ) def test_unpadded_dates_are_accepted_as_the_wrapper_always_accepted_them( diff --git a/projects/policyengine-simulation-executor/tests/test_stage12_runtime.py b/projects/policyengine-simulation-executor/tests/test_stage12_runtime.py index 2bf926f6e..62e4dac53 100644 --- a/projects/policyengine-simulation-executor/tests/test_stage12_runtime.py +++ b/projects/policyengine-simulation-executor/tests/test_stage12_runtime.py @@ -1002,7 +1002,7 @@ def serialize(self): for role, stop in (("baseline", "2026-12-31"), ("reform", "2027-12-31")): (value,) = observed[role].policy.parameter_values - assert value.end_date == datetime.strptime(stop, "%Y-%m-%d"), role + assert value.end_date == datetime.strptime(stop, "%Y-%m-%d"), role # noqa: DTZ007 # policyengine.py's ParameterValue dates are naive def _uk_report() -> ReportExecutionInput: From d0d66f614686531ce9b7c951fe2c63defb108ebc Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Tue, 13 Oct 2026 21:35:01 +0400 Subject: [PATCH 03/10] Replace asserts outside tests for ruff S101 The deployed-service test suite in authenticated_tests is a pytest suite that the tests glob misses, so give it the same test exemptions. Elsewhere, drop the asserts that only narrowed types: the SPM selection check now returns the hash and scenario it has already proved present, and the qualification runner closes over a non-optional runtime. The gateway image smoke raises RuntimeError when its report is incomplete, as the executor's static-file smoke does, and the fake model output raises AssertionError explicitly when the adapter asks for a mode other than JSON. Co-Authored-By: Claude Opus 5.5 --- .../policyengine_simulation_contract/spm.py | 20 +++++++++---------- .../test_simulation_output_builder.py | 4 +++- .../stage12_qualification.py | 6 +++--- .../smoke_app.py | 8 ++++++-- ruff.toml | 3 +++ 5 files changed, 24 insertions(+), 17 deletions(-) diff --git a/libs/policyengine-simulation-contract/src/policyengine_simulation_contract/spm.py b/libs/policyengine-simulation-contract/src/policyengine_simulation_contract/spm.py index 450950d69..bf47aadc3 100644 --- a/libs/policyengine-simulation-contract/src/policyengine_simulation_contract/spm.py +++ b/libs/policyengine-simulation-contract/src/policyengine_simulation_contract/spm.py @@ -229,7 +229,8 @@ class SPMRuntimeVersions(BaseModel): spm_calculator: str = Field(min_length=1, alias="spm-calculator") -def _require_complete_selection(selection: SPMSelection) -> None: +def _require_complete_selection(selection: SPMSelection) -> tuple[str, str]: + """Return a complete selection's artifact hash and scenario, else raise ValueError.""" required = set(SPMSelection.model_fields) missing = required.difference(selection.model_fields_set) if missing: @@ -242,18 +243,17 @@ def _require_complete_selection(selection: SPMSelection) -> None: if selection.scenario is None: msg = "Resolved SPM selection has no scenario" raise ValueError(msg) + return selection.forecast_content_sha256, selection.scenario def _resolved_spm_configuration( selection: SPMSelection, ) -> SPMResolvedConfiguration: """Convert a resolved request selection to the completed-result contract.""" - _require_complete_selection(selection) - assert selection.forecast_content_sha256 is not None - assert selection.scenario is not None + forecast_content_sha256, scenario = _require_complete_selection(selection) return SPMResolvedConfiguration( - forecast_content_sha256=selection.forecast_content_sha256, - scenario=selection.scenario, + forecast_content_sha256=forecast_content_sha256, + scenario=scenario, geography_kind=selection.geography_kind, geography_id=selection.geography_id, county_vintage=selection.county_vintage, @@ -284,17 +284,15 @@ def build_spm_provenance( runtime_versions: SPMRuntimeVersions, ) -> SPMProvenance: """Build one compact receipt from resolved calculation inputs.""" - _require_complete_selection(selection) - assert selection.forecast_content_sha256 is not None - assert selection.scenario is not None - if forecast_sha256 != selection.forecast_content_sha256: + forecast_content_sha256, scenario = _require_complete_selection(selection) + if forecast_sha256 != forecast_content_sha256: msg = "SPM receipt artifact hash differs from the selection" raise ValueError(msg) return SPMProvenance( schema_version="canonical-spm-provenance-v2", forecast_id=forecast_id, forecast_sha256=forecast_sha256, - scenario=selection.scenario, + scenario=scenario, geography_kind=selection.geography_kind, geography_id=selection.geography_id, county_vintage=selection.county_vintage, diff --git a/projects/policyengine-simulation-executor/fixtures/test_simulation_output_builder.py b/projects/policyengine-simulation-executor/fixtures/test_simulation_output_builder.py index 2d091ee87..14f6c642a 100644 --- a/projects/policyengine-simulation-executor/fixtures/test_simulation_output_builder.py +++ b/projects/policyengine-simulation-executor/fixtures/test_simulation_output_builder.py @@ -24,7 +24,9 @@ def __init__(self, payload: dict[str, object]) -> None: def model_dump(self, *, mode: str) -> dict[str, object]: """Return the payload; fails unless the adapter asks for JSON mode.""" - assert mode == "json" + if mode != "json": + msg = f"The adapter must dump outputs in JSON mode, not {mode!r}" + raise AssertionError(msg) return self.payload diff --git a/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/stage12_qualification.py b/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/stage12_qualification.py index 0522f525c..34473ae93 100644 --- a/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/stage12_qualification.py +++ b/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/stage12_qualification.py @@ -241,11 +241,11 @@ def qualify_report_parity( run_simulation_impl, ) - local_runtime = _local_qualification_runtime() + qualification_runtime = _local_qualification_runtime() + local_runtime = qualification_runtime def run_existing(request: dict[str, Any]) -> Mapping[str, Any]: - assert local_runtime is not None - return run_simulation_impl(request, runtime=local_runtime) + return run_simulation_impl(request, runtime=qualification_runtime) existing_runner = run_existing if single_simulation_runner is None: diff --git a/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/smoke_app.py b/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/smoke_app.py index 6b53b8312..dcd62c39f 100644 --- a/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/smoke_app.py +++ b/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/smoke_app.py @@ -70,6 +70,10 @@ def main() -> None: """Run the in-image smoke remotely; fail unless modules and /health loaded.""" report = smoke_import_gateway.remote() print(report) - assert report["modules_imported"] > 0 - assert any("/health" in route for route in report["routes"]) + if report["modules_imported"] <= 0: + msg = f"Gateway image smoke imported no modules: {report!r}" + raise RuntimeError(msg) + if not any("/health" in route for route in report["routes"]): + msg = f"Gateway image smoke found no /health route: {report!r}" + raise RuntimeError(msg) print("gateway image smoke OK") diff --git a/ruff.toml b/ruff.toml index 670a22e79..9349cafaf 100644 --- a/ruff.toml +++ b/ruff.toml @@ -28,6 +28,9 @@ ignore = ["D203", "D213"] # Tests may use pytest's bare assert, and need no docstrings, annotations or # named constants for magic numbers. Every other rule applies to tests. "**/tests/**" = ["ANN", "D1", "PLR2004", "S101"] +# The deployed-service test suite (pytest, run against a live candidate) gets +# the same test exemptions. +"**/authenticated_tests/**" = ["ANN", "D1", "PLR2004", "S101"] [lint.flake8-type-checking] # Classes whose annotations Pydantic evaluates at runtime, so their imports must From 84438a52ab6fb395f978628addd90ed2b5735ab0 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Tue, 13 Oct 2026 21:36:14 +0400 Subject: [PATCH 04/10] Keep command-line output for ruff T20 Every print the rule finds belongs to a command-line script or a Modal local entrypoint, whose stdout is its output, so keep each print. Files with one to three prints mark each with a reasoned suppression; the routing-state publisher and the policyengine package updater send their output through one small helper that holds the only print. Co-Authored-By: Claude Opus 5.5 --- .../scripts/update_policyengine_package.py | 13 +++-- .../generate_openapi.py | 2 +- .../stage12_infrastructure.py | 2 +- .../scripts/dump_package_version.py | 2 +- .../src/modal/smoke_app.py | 4 +- .../modal/utils/extract_bundle_versions.py | 4 +- .../src/modal/utils/extract_stage12_bundle.py | 4 +- .../src/modal/utils/qualify_stage12_parity.py | 2 +- .../src/modal/utils/record_deployment.py | 2 +- .../modal/utils/update_v2_version_manifest.py | 2 +- .../modal/utils/update_version_registry.py | 49 ++++++++++--------- .../src/modal/utils/validate_hf_access.py | 2 +- .../modal/utils/validate_v2_worker_release.py | 2 +- .../modal/v2_static_runtime_files_smoke.py | 2 +- .../generate_openapi.py | 2 +- .../generate_openapi.py | 2 +- .../smoke_app.py | 4 +- scripts/run_quality_guards.py | 6 +-- 18 files changed, 58 insertions(+), 48 deletions(-) diff --git a/.github/scripts/update_policyengine_package.py b/.github/scripts/update_policyengine_package.py index f570b34ec..fc8d3936b 100644 --- a/.github/scripts/update_policyengine_package.py +++ b/.github/scripts/update_policyengine_package.py @@ -32,6 +32,11 @@ def _json_from_stdin() -> object: return json.load(sys.stdin) +def _write_result(result: object) -> None: + """Print one result line for update-policyengine-package.sh to capture.""" + print(result) # noqa: T201 # command-line script: the shell script reads stdout + + def _requirement_name(requirement: str) -> str: return re.split(r"[\s\[<>=!~;@]", requirement, maxsplit=1)[0] @@ -155,7 +160,7 @@ def find_issue(title: str) -> None: and isinstance(item.get("number"), int) ) if matches: - print(matches[0]) + _write_result(matches[0]) def verify_issue(expected_number: int, expected_title: str) -> None: @@ -191,7 +196,7 @@ def latest_version(package: str) -> None: if not isinstance(version, str) or not version: msg = f"PyPI response has no latest version for {package}" raise SystemExit(msg) - print(version) + _write_result(version) def bundle_versions() -> None: @@ -264,7 +269,7 @@ def certificate_sha256(country: str) -> str: "uk_certificate_sha256": certificate_sha256("uk"), } for name, version in outputs.items(): - print(f"{name}={version}") + _write_result(f"{name}={version}") #: Release-dispatch fields compared with the installed bundle: (payload key @@ -398,7 +403,7 @@ def main() -> None: """ args = _parser().parse_args() if args.command == "current-version": - print(current_version(args.pyproject, args.package)) + _write_result(current_version(args.pyproject, args.package)) elif args.command == "update-requirements": update_requirements(args.pyproject, args.package, args.current, args.latest) elif args.command == "find-issue": diff --git a/projects/policyengine-simulation-entry/src/policyengine_simulation_entry/generate_openapi.py b/projects/policyengine-simulation-entry/src/policyengine_simulation_entry/generate_openapi.py index 70f1e9066..3e985410c 100644 --- a/projects/policyengine-simulation-entry/src/policyengine_simulation_entry/generate_openapi.py +++ b/projects/policyengine-simulation-entry/src/policyengine_simulation_entry/generate_openapi.py @@ -20,7 +20,7 @@ def main() -> None: json.dumps(create_app().openapi(), indent=2) + "\n", encoding="utf-8", ) - print(f"OpenAPI spec written to {output}") + print(f"OpenAPI spec written to {output}") # noqa: T201 # command-line script: reports where it wrote the spec if __name__ == "__main__": diff --git a/projects/policyengine-simulation-entry/src/policyengine_simulation_entry/stage12_infrastructure.py b/projects/policyengine-simulation-entry/src/policyengine_simulation_entry/stage12_infrastructure.py index ccae557c9..09a36cd8b 100644 --- a/projects/policyengine-simulation-entry/src/policyengine_simulation_entry/stage12_infrastructure.py +++ b/projects/policyengine-simulation-entry/src/policyengine_simulation_entry/stage12_infrastructure.py @@ -36,7 +36,7 @@ def main() -> None: ) except (Stage12RuntimeAccessError, ValueError) as error: raise SystemExit(str(error)) from None - print( + print( # noqa: T201 # command-line check: stdout reports the verified access "Shared API v2 runtime database access verified for Stage 12: " f"environment={args.environment}, role={args.expected_role}" ) diff --git a/projects/policyengine-simulation-executor/scripts/dump_package_version.py b/projects/policyengine-simulation-executor/scripts/dump_package_version.py index 32732e013..943fe1a8c 100644 --- a/projects/policyengine-simulation-executor/scripts/dump_package_version.py +++ b/projects/policyengine-simulation-executor/scripts/dump_package_version.py @@ -34,5 +34,5 @@ def get_package_version_from_lock( if version is None: sys.exit(1) -print(version) +print(version) # noqa: T201 # command-line script: the version on stdout is its output sys.exit(0) diff --git a/projects/policyengine-simulation-executor/src/modal/smoke_app.py b/projects/policyengine-simulation-executor/src/modal/smoke_app.py index 65ad946e8..40320881c 100644 --- a/projects/policyengine-simulation-executor/src/modal/smoke_app.py +++ b/projects/policyengine-simulation-executor/src/modal/smoke_app.py @@ -169,5 +169,5 @@ def smoke_import_executor() -> dict: def main() -> None: """Run the image smoke remotely and print its report.""" report = smoke_import_executor.remote() - print(report) - print("executor image smoke OK") + print(report) # noqa: T201 # Modal local entrypoint: runs locally, and stdout is the smoke's report + print("executor image smoke OK") # noqa: T201 # Modal local entrypoint: runs locally, and stdout is the smoke's report diff --git a/projects/policyengine-simulation-executor/src/modal/utils/extract_bundle_versions.py b/projects/policyengine-simulation-executor/src/modal/utils/extract_bundle_versions.py index 4e41068fa..3d5c263e8 100644 --- a/projects/policyengine-simulation-executor/src/modal/utils/extract_bundle_versions.py +++ b/projects/policyengine-simulation-executor/src/modal/utils/extract_bundle_versions.py @@ -39,7 +39,7 @@ def main() -> None: if "--shell" in sys.argv[1:]: for key, value in outputs.items(): - print(f"{key}={value}") + print(f"{key}={value}") # noqa: T201 # command-line script: deployment scripts read stdout return github_output = os.environ.get("GITHUB_OUTPUT") @@ -49,7 +49,7 @@ def main() -> None: for key, value in outputs.items(): file.write(f"{key}={value}\n") - print( + print( # noqa: T201 # command-line script: deployment scripts read stdout "Deploying with policyengine.py bundle " f"{outputs['policyengine_version']}: " f"policyengine-core={outputs['policyengine_core_version']}, " diff --git a/projects/policyengine-simulation-executor/src/modal/utils/extract_stage12_bundle.py b/projects/policyengine-simulation-executor/src/modal/utils/extract_stage12_bundle.py index 3850ed768..11cd6dbaa 100644 --- a/projects/policyengine-simulation-executor/src/modal/utils/extract_stage12_bundle.py +++ b/projects/policyengine-simulation-executor/src/modal/utils/extract_stage12_bundle.py @@ -66,10 +66,10 @@ def main() -> None: + "\n" ) if args.output is None: - print(rendered, end="") + print(rendered, end="") # noqa: T201 # command-line script: stdout is its output else: args.output.write_text(rendered, encoding="utf-8") - print(f"bundle_manifest_sha256={resolved.bundle_manifest_sha256}") + print(f"bundle_manifest_sha256={resolved.bundle_manifest_sha256}") # noqa: T201 # command-line script: stdout is its output if __name__ == "__main__": diff --git a/projects/policyengine-simulation-executor/src/modal/utils/qualify_stage12_parity.py b/projects/policyengine-simulation-executor/src/modal/utils/qualify_stage12_parity.py index 994750336..42c5864f3 100644 --- a/projects/policyengine-simulation-executor/src/modal/utils/qualify_stage12_parity.py +++ b/projects/policyengine-simulation-executor/src/modal/utils/qualify_stage12_parity.py @@ -61,7 +61,7 @@ def main() -> None: simulation_tolerances=_tolerances(arguments.simulation_tolerances), aggregate_tolerances=_tolerances(arguments.aggregate_tolerances), ) - print(receipt.model_dump_json()) + print(receipt.model_dump_json()) # noqa: T201 # command-line script: the receipt JSON on stdout is its output if __name__ == "__main__": diff --git a/projects/policyengine-simulation-executor/src/modal/utils/record_deployment.py b/projects/policyengine-simulation-executor/src/modal/utils/record_deployment.py index 3b0426453..9efd1beb4 100644 --- a/projects/policyengine-simulation-executor/src/modal/utils/record_deployment.py +++ b/projects/policyengine-simulation-executor/src/modal/utils/record_deployment.py @@ -64,7 +64,7 @@ def main() -> None: # The store speaks JSON-able mappings; models dump at the call site # (same discipline as publish_manifest_impl). ArtifactStore().write_deployed_marker(args.environment, marker.model_dump()) - print( + print( # noqa: T201 # command-line script: reports the recorded marker f"Recorded deployed/{args.environment}.json for manifest {args.manifest_digest}" ) diff --git a/projects/policyengine-simulation-executor/src/modal/utils/update_v2_version_manifest.py b/projects/policyengine-simulation-executor/src/modal/utils/update_v2_version_manifest.py index c695073e7..632e8e87b 100644 --- a/projects/policyengine-simulation-executor/src/modal/utils/update_v2_version_manifest.py +++ b/projects/policyengine-simulation-executor/src/modal/utils/update_v2_version_manifest.py @@ -81,7 +81,7 @@ def main() -> None: worker=worker, force_default=args.force_default, ) - print( + print( # noqa: T201 # command-line script: the manifest JSON on stdout is its output json.dumps( manifest.model_dump(mode="json"), sort_keys=True, diff --git a/projects/policyengine-simulation-executor/src/modal/utils/update_version_registry.py b/projects/policyengine-simulation-executor/src/modal/utils/update_version_registry.py index 2d320a2bc..fdc71be2c 100644 --- a/projects/policyengine-simulation-executor/src/modal/utils/update_version_registry.py +++ b/projects/policyengine-simulation-executor/src/modal/utils/update_version_registry.py @@ -1078,6 +1078,11 @@ def restore_routing_state( return next_state +def _report(line: str = "") -> None: + """Print one line of the command's progress report.""" + print(line) # noqa: T201 # command-line script: stdout is its progress report + + def main() -> None: """Publish a deployed app's routes, or seed the state from legacy Dicts.""" parser = argparse.ArgumentParser( @@ -1348,19 +1353,19 @@ def active_state() -> object: parser.error("--expected-latest needs --restore-latest") if args.seed_active_from_legacy: - print( + _report( "Seeding active routing state from legacy Modal dicts in " f"environment: {args.environment}" ) state = seed_active_routing_state_from_legacy(environment=args.environment) - print() - print(f" {ROUTING_STATE_DICT_NAME}[{ROUTING_STATE_ACTIVE_KEY}]: updated") - print(f" policyengine routes: {len(state['routes']['policyengine'])}") - print(f" US routes: {len(state['routes']['us'])}") - print(f" UK routes: {len(state['routes']['uk'])}") - print(f" bundle manifests: {len(state['bundles'])}") - print() - print("Legacy routing state seed completed successfully.") + _report() + _report(f" {ROUTING_STATE_DICT_NAME}[{ROUTING_STATE_ACTIVE_KEY}]: updated") + _report(f" policyengine routes: {len(state['routes']['policyengine'])}") + _report(f" US routes: {len(state['routes']['us'])}") + _report(f" UK routes: {len(state['routes']['uk'])}") + _report(f" bundle manifests: {len(state['bundles'])}") + _report() + _report("Legacy routing state seed completed successfully.") return required_args = { @@ -1391,12 +1396,12 @@ def active_state() -> object: print(f"advances_latest={'true' if plan['advances_latest'] else 'false'}") return - print(f"Publishing routing state in Modal environment: {args.environment}") - print(f" App name: {args.app_name}") - print(f" policyengine.py version: {args.policyengine_version}") - print(f" US version: {args.us_version}") - print(f" UK version: {args.uk_version}") - print() + _report(f"Publishing routing state in Modal environment: {args.environment}") + _report(f" App name: {args.app_name}") + _report(f" policyengine.py version: {args.policyengine_version}") + _report(f" US version: {args.us_version}") + _report(f" UK version: {args.uk_version}") + _report() state = publish_routing_state( environment=args.environment, @@ -1406,13 +1411,13 @@ def active_state() -> object: uk_version=args.uk_version, force_latest=args.force_latest, ) - print() - print(f" {ROUTING_STATE_DICT_NAME}[{ROUTING_STATE_ACTIVE_KEY}]: updated") - print(f" latest.policyengine: {state['latest']['policyengine']}") - print(f" latest.us: {state['latest']['us']}") - print(f" latest.uk: {state['latest']['uk']}") - print() - print("Routing state published successfully.") + _report() + _report(f" {ROUTING_STATE_DICT_NAME}[{ROUTING_STATE_ACTIVE_KEY}]: updated") + _report(f" latest.policyengine: {state['latest']['policyengine']}") + _report(f" latest.us: {state['latest']['us']}") + _report(f" latest.uk: {state['latest']['uk']}") + _report() + _report("Routing state published successfully.") if __name__ == "__main__": diff --git a/projects/policyengine-simulation-executor/src/modal/utils/validate_hf_access.py b/projects/policyengine-simulation-executor/src/modal/utils/validate_hf_access.py index 895fc9549..27ebf3062 100644 --- a/projects/policyengine-simulation-executor/src/modal/utils/validate_hf_access.py +++ b/projects/policyengine-simulation-executor/src/modal/utils/validate_hf_access.py @@ -15,7 +15,7 @@ def validate_local_access() -> HFCredentialAudit: def main() -> None: """Validate the CI credential and print its audit record as JSON.""" - print(validate_local_access().model_dump_json()) + print(validate_local_access().model_dump_json()) # noqa: T201 # command-line script: the audit record on stdout is its output if __name__ == "__main__": diff --git a/projects/policyengine-simulation-executor/src/modal/utils/validate_v2_worker_release.py b/projects/policyengine-simulation-executor/src/modal/utils/validate_v2_worker_release.py index 0192c50f4..579799482 100644 --- a/projects/policyengine-simulation-executor/src/modal/utils/validate_v2_worker_release.py +++ b/projects/policyengine-simulation-executor/src/modal/utils/validate_v2_worker_release.py @@ -108,7 +108,7 @@ def main() -> None: + "\n", encoding="utf-8", ) - print(validation.validation_invocation_id) + print(validation.validation_invocation_id) # noqa: T201 # command-line script: logs the validation id for CI if __name__ == "__main__": diff --git a/projects/policyengine-simulation-executor/src/modal/v2_static_runtime_files_smoke.py b/projects/policyengine-simulation-executor/src/modal/v2_static_runtime_files_smoke.py index 47c15c8b9..3c022295a 100644 --- a/projects/policyengine-simulation-executor/src/modal/v2_static_runtime_files_smoke.py +++ b/projects/policyengine-simulation-executor/src/modal/v2_static_runtime_files_smoke.py @@ -71,4 +71,4 @@ def main() -> None: f"UK dataset boundary version: {observed!r}" ) raise RuntimeError(msg) - print("Stage 12 static runtime file smoke OK") + print("Stage 12 static runtime file smoke OK") # noqa: T201 # Modal local entrypoint: runs locally, and stdout is the smoke's report diff --git a/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/generate_openapi.py b/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/generate_openapi.py index 3bf906d7e..f7b8b069b 100644 --- a/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/generate_openapi.py +++ b/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/generate_openapi.py @@ -13,4 +13,4 @@ with open(output_path, "w") as f: json.dump(openapi_spec, f, indent=4) -print(f"OpenAPI spec written to {output_path}") +print(f"OpenAPI spec written to {output_path}") # noqa: T201 # command-line script: reports where it wrote the spec diff --git a/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/generate_openapi.py b/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/generate_openapi.py index fea8f2a3b..5df2740f3 100644 --- a/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/generate_openapi.py +++ b/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/generate_openapi.py @@ -150,7 +150,7 @@ def main() -> None: with open(output_path, "w") as f: json.dump(openapi_spec, f, indent=2) - print(f"OpenAPI spec written to {output_path}") + print(f"OpenAPI spec written to {output_path}") # noqa: T201 # command-line script: reports where it wrote the spec if __name__ == "__main__": diff --git a/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/smoke_app.py b/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/smoke_app.py index dcd62c39f..647afeb65 100644 --- a/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/smoke_app.py +++ b/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/smoke_app.py @@ -69,11 +69,11 @@ def smoke_import_gateway() -> dict: def main() -> None: """Run the in-image smoke remotely; fail unless modules and /health loaded.""" report = smoke_import_gateway.remote() - print(report) + print(report) # noqa: T201 # Modal local entrypoint: runs locally, and stdout is the smoke's report if report["modules_imported"] <= 0: msg = f"Gateway image smoke imported no modules: {report!r}" raise RuntimeError(msg) if not any("/health" in route for route in report["routes"]): msg = f"Gateway image smoke found no /health route: {report!r}" raise RuntimeError(msg) - print("gateway image smoke OK") + print("gateway image smoke OK") # noqa: T201 # Modal local entrypoint: runs locally, and stdout is the smoke's report diff --git a/scripts/run_quality_guards.py b/scripts/run_quality_guards.py index c6d7f4cc1..919857305 100644 --- a/scripts/run_quality_guards.py +++ b/scripts/run_quality_guards.py @@ -30,13 +30,13 @@ def main() -> int: for name, check in GUARDS: violations = check() if not violations: - print(f"{name}: passed") + print(f"{name}: passed") # noqa: T201 # command-line guard runner: stdout is its report continue failed = True - print(f"{name}: failed") + print(f"{name}: failed") # noqa: T201 # command-line guard runner: stdout is its report for violation in violations: - print(f" - {violation}") + print(f" - {violation}") # noqa: T201 # command-line guard runner: stdout is its report return 1 if failed else 0 From 2d1fd8dacaa3e98053cecac61351fb9a12b216b3 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Tue, 13 Oct 2026 21:39:48 +0400 Subject: [PATCH 05/10] Route test subprocess calls through helpers for ruff S603 and S607 Each test module that runs repository scripts now does so through one helper that holds the only subprocess call, with reasoned suppressions: the scripts are repository-owned, their arguments are built by the tests, and bash and uv come from PATH as they do in the workflows. Resolving the executables in Python instead could pick a different binary, because subprocess searches the child's PATH, and some tests run scripts with an empty environment. Co-Authored-By: Claude Opus 5.5 --- .../tests/test_cloud_run_revision_cleanup.py | 23 +- .../tests/test_deployment_assets.py | 93 +++---- .../tests/test_stage12_infrastructure.py | 37 +-- .../tests/workflow_script_support.py | 2 +- ...est_policyengine_package_update_scripts.py | 4 +- .../integration/test_image_smoke_modal.py | 36 +-- .../tests/test_ci_complete_gate.py | 31 ++- .../tests/test_modal_scripts.py | 258 +++++------------- .../test_policyengine_dependency_source.py | 29 +- ...est_policyengine_package_update_scripts.py | 6 +- .../integration/test_image_smoke_modal.py | 4 +- 11 files changed, 192 insertions(+), 331 deletions(-) diff --git a/projects/policyengine-simulation-entry/tests/test_cloud_run_revision_cleanup.py b/projects/policyengine-simulation-entry/tests/test_cloud_run_revision_cleanup.py index 80cfecf78..2f9bc38fd 100644 --- a/projects/policyengine-simulation-entry/tests/test_cloud_run_revision_cleanup.py +++ b/projects/policyengine-simulation-entry/tests/test_cloud_run_revision_cleanup.py @@ -27,6 +27,22 @@ ) +def _run_bash( + *arguments: str | Path, + check: bool, + capture_output: bool = False, + env: dict[str, str] | None = None, +) -> subprocess.CompletedProcess[str]: + """Run bash on a repository deployment script, with text output.""" + return subprocess.run( # noqa: S603 # runs repository scripts with test-built arguments + ["bash", *arguments], # noqa: S607 # bash from PATH, as the workflows run these scripts + check=check, + capture_output=capture_output, + text=True, + env=env, + ) + + def _revision(name: str, created_at: str, *, ready: bool = True) -> dict: return { "metadata": { @@ -96,11 +112,10 @@ def _run_cleanup( ) revisions_json.write_text(json.dumps(revisions), encoding="utf-8") - return subprocess.run( - ["bash", CLEANUP_SCRIPT], + return _run_bash( + CLEANUP_SCRIPT, check=False, capture_output=True, - text=True, env=os.environ | { "GCLOUD_BIN": str(_write_fake_gcloud(tmp_path)), @@ -321,5 +336,5 @@ def test_revision_cleanup_runs_after_promoted_deployment_completion(): def test_revision_cleanup_script_has_valid_shell_syntax_and_is_executable(): - subprocess.run(["bash", "-n", CLEANUP_SCRIPT], check=True) + _run_bash("-n", CLEANUP_SCRIPT, check=True) assert os.access(CLEANUP_SCRIPT, os.X_OK) diff --git a/projects/policyengine-simulation-entry/tests/test_deployment_assets.py b/projects/policyengine-simulation-entry/tests/test_deployment_assets.py index d978edba6..ad4f6d433 100644 --- a/projects/policyengine-simulation-entry/tests/test_deployment_assets.py +++ b/projects/policyengine-simulation-entry/tests/test_deployment_assets.py @@ -58,10 +58,26 @@ ) +def _run_bash( + *arguments: str | Path, + check: bool, + capture_output: bool = False, + env: dict[str, str] | None = None, +) -> subprocess.CompletedProcess[str]: + """Run bash on a repository deployment script, with text output.""" + return subprocess.run( # noqa: S603 # runs repository scripts with test-built arguments + ["bash", *arguments], # noqa: S607 # bash from PATH, as the workflows run these scripts + check=check, + capture_output=capture_output, + text=True, + env=env, + ) + + def test_deployment_scripts_have_valid_shell_syntax(): - subprocess.run(["bash", "-n", TRAFFIC_SCRIPT], check=True) - subprocess.run(["bash", "-n", SMOKE_SCRIPT], check=True) - subprocess.run(["bash", "-n", CLOUD_RUN_DEPLOY_SCRIPT], check=True) + _run_bash("-n", TRAFFIC_SCRIPT, check=True) + _run_bash("-n", SMOKE_SCRIPT, check=True) + _run_bash("-n", CLOUD_RUN_DEPLOY_SCRIPT, check=True) assert os.access(SMOKE_SCRIPT, os.X_OK) assert os.access(CLOUD_RUN_DEPLOY_SCRIPT, os.X_OK) @@ -167,17 +183,13 @@ def test_stable_smoke_retries_the_previous_revision_until_target_is_served( [old_revision, old_revision, target_revision], ) - subprocess.run( - [ - "bash", - SMOKE_SCRIPT, - "https://simulation.example.test", - target_revision, - old_revision, - ], + _run_bash( + SMOKE_SCRIPT, + "https://simulation.example.test", + target_revision, + old_revision, check=True, capture_output=True, - text=True, env=_smoke_test_env(tmp_path, state_file, revisions_file), ) @@ -193,17 +205,13 @@ def test_stable_smoke_rejects_an_unexpected_revision_without_retrying(tmp_path: [unexpected_revision, target_revision], ) - result = subprocess.run( - [ - "bash", - SMOKE_SCRIPT, - "https://simulation.example.test", - target_revision, - old_revision, - ], + result = _run_bash( + SMOKE_SCRIPT, + "https://simulation.example.test", + target_revision, + old_revision, check=False, capture_output=True, - text=True, env=_smoke_test_env(tmp_path, state_file, revisions_file), ) @@ -220,16 +228,12 @@ def test_candidate_smoke_rejects_a_revision_mismatch_without_retrying(tmp_path: [old_revision, target_revision], ) - result = subprocess.run( - [ - "bash", - SMOKE_SCRIPT, - "https://candidate.example.test", - target_revision, - ], + result = _run_bash( + SMOKE_SCRIPT, + "https://candidate.example.test", + target_revision, check=False, capture_output=True, - text=True, env=_smoke_test_env(tmp_path, state_file, revisions_file), ) @@ -248,17 +252,13 @@ def test_stable_smoke_fails_when_the_previous_revision_does_not_converge( [old_revision, old_revision, old_revision], ) - result = subprocess.run( - [ - "bash", - SMOKE_SCRIPT, - "https://simulation.example.test", - target_revision, - old_revision, - ], + result = _run_bash( + SMOKE_SCRIPT, + "https://simulation.example.test", + target_revision, + old_revision, check=False, capture_output=True, - text=True, env=_smoke_test_env(tmp_path, state_file, revisions_file), ) @@ -483,7 +483,7 @@ def test_cloud_run_deployment_escapes_environment_and_pins_secret_versions(tmp_p "MODAL_TOKEN_SECRET_SECRET_NAME": "modal-token-secret", } - subprocess.run( + subprocess.run( # noqa: S603 # runs the repository deploy script directly, as the workflow does [CLOUD_RUN_DEPLOY_SCRIPT], check=True, env=command_environment, @@ -1604,11 +1604,10 @@ def test_traffic_changes_are_exact_revision_validated_and_reversible(tmp_path): "SIMULATION_ENTRYPOINT_SERVICE": "policyengine-simulation-entry", } - subprocess.run( - ["bash", TRAFFIC_SCRIPT], + _run_bash( + TRAFFIC_SCRIPT, check=True, capture_output=True, - text=True, env={ **base_env, "SIMULATION_ENTRYPOINT_TARGET_REVISION": "policyengine-simulation-entry-00002-new", @@ -1620,11 +1619,10 @@ def test_traffic_changes_are_exact_revision_validated_and_reversible(tmp_path): == "policyengine-simulation-entry-00002-new" ) - subprocess.run( - ["bash", TRAFFIC_SCRIPT], + _run_bash( + TRAFFIC_SCRIPT, check=True, capture_output=True, - text=True, env={ **base_env, "SIMULATION_ENTRYPOINT_TARGET_REVISION": "policyengine-simulation-entry-00001-old", @@ -1655,11 +1653,10 @@ def test_traffic_change_refuses_an_intervening_promotion(tmp_path): ) fake_gcloud.chmod(0o755) - result = subprocess.run( - ["bash", TRAFFIC_SCRIPT], + result = _run_bash( + TRAFFIC_SCRIPT, check=False, capture_output=True, - text=True, env={ **os.environ, "GCLOUD_BIN": str(fake_gcloud), diff --git a/projects/policyengine-simulation-entry/tests/test_stage12_infrastructure.py b/projects/policyengine-simulation-entry/tests/test_stage12_infrastructure.py index 4eb899072..6833cad03 100644 --- a/projects/policyengine-simulation-entry/tests/test_stage12_infrastructure.py +++ b/projects/policyengine-simulation-entry/tests/test_stage12_infrastructure.py @@ -11,20 +11,23 @@ DEPLOY_WORKFLOW = REPO_ROOT / ".github/workflows/simulation-deploy.reusable.yml" -def test_infrastructure_validation_is_bounded_and_valid_shell() -> None: - result = subprocess.run( - ["bash", "-n", str(SCRIPT)], +def _run_bash( + *arguments: str, env: dict[str, str] | None = None +) -> subprocess.CompletedProcess[str]: + """Run bash on a repository script, capturing text output; a failure is returned.""" + return subprocess.run( # noqa: S603 # runs repository scripts with test-built arguments + ["bash", *arguments], # noqa: S607 # bash from PATH, as the workflows run these scripts capture_output=True, text=True, + env=env, check=False, ) + + +def test_infrastructure_validation_is_bounded_and_valid_shell() -> None: + result = _run_bash("-n", str(SCRIPT)) assert result.returncode == 0, result.stderr - sync_result = subprocess.run( - ["bash", "-n", str(SYNC_SCRIPT)], - capture_output=True, - text=True, - check=False, - ) + sync_result = _run_bash("-n", str(SYNC_SCRIPT)) assert sync_result.returncode == 0, sync_result.stderr source = SCRIPT.read_text(encoding="utf-8") assert "STAGE12_ENVIRONMENT" in source @@ -64,13 +67,7 @@ def test_infrastructure_validation_is_bounded_and_valid_shell() -> None: def test_infrastructure_validation_rejects_missing_configuration() -> None: - result = subprocess.run( - ["bash", str(SCRIPT)], - capture_output=True, - text=True, - env={}, - check=False, - ) + result = _run_bash(str(SCRIPT), env={}) assert result.returncode != 0 assert "STAGE12_ENVIRONMENT is required" in result.stderr @@ -98,13 +95,7 @@ def test_storage_validation_uses_only_the_modal_worker_credentials() -> None: def test_modal_secret_sync_rejects_missing_configuration() -> None: - result = subprocess.run( - ["bash", str(SYNC_SCRIPT), "staging"], - capture_output=True, - text=True, - env={}, - check=False, - ) + result = _run_bash(str(SYNC_SCRIPT), "staging", env={}) assert result.returncode != 0 assert "STAGE12_GCP_PROJECT_ID is required" in result.stderr diff --git a/projects/policyengine-simulation-entry/tests/workflow_script_support.py b/projects/policyengine-simulation-entry/tests/workflow_script_support.py index f9111816e..f6aa5141f 100644 --- a/projects/policyengine-simulation-entry/tests/workflow_script_support.py +++ b/projects/policyengine-simulation-entry/tests/workflow_script_support.py @@ -73,7 +73,7 @@ def run_script( cwd: Path | None = None, ) -> subprocess.CompletedProcess[str]: """Run ``.github/scripts/`` directly, as a workflow step does.""" - return subprocess.run( + return subprocess.run( # noqa: S603 # runs a repository script with test-built arguments [str(SCRIPTS_DIR / name), *args], capture_output=True, text=True, diff --git a/projects/policyengine-simulation-executor/fixtures/test_policyengine_package_update_scripts.py b/projects/policyengine-simulation-executor/fixtures/test_policyengine_package_update_scripts.py index 84b849f4c..edc93e3fc 100644 --- a/projects/policyengine-simulation-executor/fixtures/test_policyengine_package_update_scripts.py +++ b/projects/policyengine-simulation-executor/fixtures/test_policyengine_package_update_scripts.py @@ -250,8 +250,8 @@ def updater_env(fake_bin: Path, **extra: str) -> dict[str, str]: def run_updater(*args: str, env: dict[str, str]) -> subprocess.CompletedProcess[str]: """Run the updater from the repo root; a non-zero exit is returned, not raised.""" - return subprocess.run( - ["bash", str(SCRIPT), *args], + return subprocess.run( # noqa: S603 # runs the repository updater script with test-built arguments + ["bash", str(SCRIPT), *args], # noqa: S607 # bash from PATH, as the workflow runs the script cwd=REPO_ROOT, env=env, capture_output=True, diff --git a/projects/policyengine-simulation-executor/tests/integration/test_image_smoke_modal.py b/projects/policyengine-simulation-executor/tests/integration/test_image_smoke_modal.py index cf7919c9f..4437595ce 100644 --- a/projects/policyengine-simulation-executor/tests/integration/test_image_smoke_modal.py +++ b/projects/policyengine-simulation-executor/tests/integration/test_image_smoke_modal.py @@ -13,43 +13,27 @@ PROJECT_ROOT = Path(__file__).resolve().parents[2] -@pytest.mark.integration -def test_executor_image_smoke(): +def _run_modal_smoke(app_path: str) -> subprocess.CompletedProcess[str]: + """Run one smoke app with ``modal run`` in the ``MODAL_SMOKE_ENV`` environment.""" env_name = os.environ.get("MODAL_SMOKE_ENV", "staging") - result = subprocess.run( - [ - "uv", - "run", - "modal", - "run", - f"--env={env_name}", - "src/modal/smoke_app.py", - ], + return subprocess.run( # noqa: S603 # runs a repository smoke app through uv and Modal + ["uv", "run", "modal", "run", f"--env={env_name}", app_path], # noqa: S607 # uv from PATH, as CI runs it cwd=PROJECT_ROOT, capture_output=True, text=True, timeout=2400, ) + + +@pytest.mark.integration +def test_executor_image_smoke(): + result = _run_modal_smoke("src/modal/smoke_app.py") assert result.returncode == 0, result.stdout + result.stderr assert "executor image smoke OK" in result.stdout @pytest.mark.integration def test_stage12_static_runtime_file_image_smoke(): - env_name = os.environ.get("MODAL_SMOKE_ENV", "staging") - result = subprocess.run( - [ - "uv", - "run", - "modal", - "run", - f"--env={env_name}", - "src/modal/v2_static_runtime_files_smoke.py", - ], - cwd=PROJECT_ROOT, - capture_output=True, - text=True, - timeout=2400, - ) + result = _run_modal_smoke("src/modal/v2_static_runtime_files_smoke.py") assert result.returncode == 0, result.stdout + result.stderr assert "Stage 12 static runtime file smoke OK" in result.stdout diff --git a/projects/policyengine-simulation-executor/tests/test_ci_complete_gate.py b/projects/policyengine-simulation-executor/tests/test_ci_complete_gate.py index 798704db2..52b39b385 100644 --- a/projects/policyengine-simulation-executor/tests/test_ci_complete_gate.py +++ b/projects/policyengine-simulation-executor/tests/test_ci_complete_gate.py @@ -103,16 +103,25 @@ def test_rule_proof_gate_check() -> None: assert gate_violations(good) == [] +def _run_gate_script(env: dict[str, str]) -> subprocess.CompletedProcess[str]: + return subprocess.run( # noqa: S603 # runs the repository gate script with test-built arguments + ["bash", GATE_SCRIPT], # noqa: S607 # bash from PATH, as the workflow runs the script + cwd=REPO, + env=env, + text=True, + capture_output=True, + check=False, + ) + + def _run_gate( job_results: dict[str, str], *, pull_request_base_ref: str = "main", stack_base_ref: str = "", ) -> subprocess.CompletedProcess[str]: - return subprocess.run( - ["bash", GATE_SCRIPT], - cwd=REPO, - env={ + return _run_gate_script( + { "HOME": os.environ.get("HOME", ""), "PATH": os.environ["PATH"], "JOB_RESULTS": json.dumps( @@ -123,10 +132,7 @@ def _run_gate( ), "PULL_REQUEST_BASE_REF": pull_request_base_ref, "STACK_BASE_REF": stack_base_ref, - }, - text=True, - capture_output=True, - check=False, + } ) @@ -179,13 +185,8 @@ def test_gate_fails_when_a_job_was_skipped_on_full_ci() -> None: def test_gate_requires_the_job_results() -> None: - result = subprocess.run( - ["bash", GATE_SCRIPT], - cwd=REPO, - env={"PATH": os.environ["PATH"], "PULL_REQUEST_BASE_REF": "main"}, - text=True, - capture_output=True, - check=False, + result = _run_gate_script( + {"PATH": os.environ["PATH"], "PULL_REQUEST_BASE_REF": "main"} ) assert result.returncode != 0 diff --git a/projects/policyengine-simulation-executor/tests/test_modal_scripts.py b/projects/policyengine-simulation-executor/tests/test_modal_scripts.py index 3b9b9f45f..753a4a80a 100644 --- a/projects/policyengine-simulation-executor/tests/test_modal_scripts.py +++ b/projects/policyengine-simulation-executor/tests/test_modal_scripts.py @@ -6,6 +6,7 @@ import os import subprocess +from pathlib import Path import pytest from fixtures.test_modal_scripts import REPO_ROOT, SCRIPTS_DIR @@ -13,6 +14,22 @@ pytest_plugins = ("fixtures.test_modal_scripts",) +def _run_bash( + *arguments: str, + env: dict[str, str] | None = None, + cwd: Path | None = None, +) -> subprocess.CompletedProcess[str]: + """Run bash on a repository script, capturing text output; a failure is returned.""" + return subprocess.run( # noqa: S603 # runs repository scripts with test-built arguments + ["bash", *arguments], # noqa: S607 # bash from PATH, as the workflows run these scripts + capture_output=True, + text=True, + env=env, + cwd=cwd, + check=False, + ) + + class TestModalExtractVersions: """Tests for modal-extract-versions.sh.""" @@ -24,11 +41,7 @@ def test_script_exists(self): def test_script_is_executable_or_can_be_run_with_bash(self): """Script should be runnable with bash.""" - result = subprocess.run( - ["bash", "-n", str(self.script)], - capture_output=True, - text=True, - ) + result = _run_bash("-n", str(self.script)) assert result.returncode == 0, f"Syntax error in script: {result.stderr}" def test_extracts_versions_from_policyengine_bundle(self, temp_github_output): @@ -41,12 +54,7 @@ def test_extracts_versions_from_policyengine_bundle(self, temp_github_output): env = os.environ.copy() env["GITHUB_OUTPUT"] = temp_github_output - result = subprocess.run( - ["bash", str(self.script), str(project_dir)], - capture_output=True, - text=True, - env=env, - ) + result = _run_bash(str(self.script), str(project_dir), env=env) assert result.returncode == 0, f"Script failed: {result.stderr}" @@ -101,28 +109,18 @@ def test_script_exists(self): def test_script_syntax(self): """Script should have valid bash syntax.""" - result = subprocess.run( - ["bash", "-n", str(self.script)], - capture_output=True, - text=True, - ) + result = _run_bash("-n", str(self.script)) assert result.returncode == 0, f"Syntax error: {result.stderr}" def test_requires_base_url_argument(self): """Should fail when no URL is provided.""" - result = subprocess.run( - ["bash", str(self.script)], - capture_output=True, - text=True, - ) + result = _run_bash(str(self.script)) assert result.returncode != 0, "Should fail without URL argument" def test_fails_on_unreachable_url(self): """Should fail when URL is unreachable.""" - result = subprocess.run( - ["bash", str(self.script), "http://localhost:99999/nonexistent", "1", "1"], - capture_output=True, - text=True, + result = _run_bash( + str(self.script), "http://localhost:99999/nonexistent", "1", "1" ) assert result.returncode != 0, "Should fail on unreachable URL" @@ -150,13 +148,7 @@ def test_requires_hf_token_before_running_modal(self, tmp_path): ) env.pop("PE_UK_PRIVATE_HF_READ_TOKEN", None) - result = subprocess.run( - ["bash", str(self.script), "staging"], - capture_output=True, - text=True, - env=env, - check=False, - ) + result = _run_bash(str(self.script), "staging", env=env) assert result.returncode != 0 assert "PE_UK_PRIVATE_HF_READ_TOKEN is required" in result.stderr @@ -180,13 +172,7 @@ def test_syncs_expected_hf_key_before_image_smokes(self, tmp_path): } ) - result = subprocess.run( - ["bash", str(self.script), "staging"], - capture_output=True, - text=True, - env=env, - check=False, - ) + result = _run_bash(str(self.script), "staging", env=env) assert result.returncode == 0, result.stderr calls = uv_calls_log.read_text(encoding="utf-8").splitlines() @@ -268,11 +254,7 @@ def test_script_exists(self): def test_script_syntax(self): """Script should have valid bash syntax.""" - result = subprocess.run( - ["bash", "-n", str(self.script)], - capture_output=True, - text=True, - ) + result = _run_bash("-n", str(self.script)) assert result.returncode == 0, f"Syntax error: {result.stderr}" def test_generates_success_summary(self, temp_github_step_summary): @@ -280,17 +262,12 @@ def test_generates_success_summary(self, temp_github_step_summary): env = os.environ.copy() env["GITHUB_STEP_SUMMARY"] = temp_github_step_summary - result = subprocess.run( - [ - "bash", - str(self.script), - "success", - "https://beta.example.com", - "success", - "https://prod.example.com", - ], - capture_output=True, - text=True, + result = _run_bash( + str(self.script), + "success", + "https://beta.example.com", + "success", + "https://prod.example.com", env=env, ) @@ -312,17 +289,12 @@ def test_generates_skipped_summary(self, temp_github_step_summary): env = os.environ.copy() env["GITHUB_STEP_SUMMARY"] = temp_github_step_summary - result = subprocess.run( - [ - "bash", - str(self.script), - "skipped", - "", - "success", - "https://prod.example.com", - ], - capture_output=True, - text=True, + result = _run_bash( + str(self.script), + "skipped", + "", + "success", + "https://prod.example.com", env=env, ) @@ -345,20 +317,12 @@ def test_script_exists(self): def test_script_syntax(self): """Script should have valid bash syntax.""" - result = subprocess.run( - ["bash", "-n", str(self.script)], - capture_output=True, - text=True, - ) + result = _run_bash("-n", str(self.script)) assert result.returncode == 0, f"Syntax error: {result.stderr}" def test_requires_modal_environment_argument(self): """Should fail when no modal environment is provided.""" - result = subprocess.run( - ["bash", str(self.script)], - capture_output=True, - text=True, - ) + result = _run_bash(str(self.script)) assert result.returncode != 0, "Should fail without modal environment" def test_fails_when_gateway_auth_config_is_partial(self): @@ -371,12 +335,7 @@ def test_fails_when_gateway_auth_config_is_partial(self): env.pop("GATEWAY_AUTH_CLIENT_ID", None) env.pop("GATEWAY_AUTH_CLIENT_SECRET", None) - result = subprocess.run( - ["bash", str(self.script), "staging"], - capture_output=True, - text=True, - env=env, - ) + result = _run_bash(str(self.script), "staging", env=env) assert result.returncode != 0 assert "Gateway auth config is partial." in result.stderr @@ -395,12 +354,7 @@ def test_fails_when_auth_required_but_gateway_auth_vars_missing(self): ): env.pop(key, None) - result = subprocess.run( - ["bash", str(self.script), "staging"], - capture_output=True, - text=True, - env=env, - ) + result = _run_bash(str(self.script), "staging", env=env) assert result.returncode != 0 assert "GATEWAY_AUTH_REQUIRED is enabled" in result.stderr @@ -411,13 +365,7 @@ def test_requires_gcp_credentials(self): env["PE_UK_PRIVATE_HF_READ_TOKEN"] = "hf_test" env.pop("GCP_CREDENTIALS_JSON", None) - result = subprocess.run( - ["bash", str(self.script), "staging"], - capture_output=True, - text=True, - env=env, - check=False, - ) + result = _run_bash(str(self.script), "staging", env=env) assert result.returncode != 0 assert "GCP_CREDENTIALS_JSON is required" in result.stderr @@ -446,12 +394,7 @@ def test_creates_gateway_secret_with_normalized_issuer(self, tmp_path): } ) - result = subprocess.run( - ["bash", str(self.script), "staging"], - capture_output=True, - text=True, - env=env, - ) + result = _run_bash(str(self.script), "staging", env=env) assert result.returncode == 0, result.stderr calls = uv_calls_log.read_text() @@ -498,13 +441,7 @@ def test_gcp_secret_sync_failure_stops_deployment(self, tmp_path): ): env.pop(key, None) - result = subprocess.run( - ["bash", str(self.script), "staging"], - capture_output=True, - text=True, - env=env, - check=False, - ) + result = _run_bash(str(self.script), "staging", env=env) assert result.returncode != 0 calls = uv_calls_log.read_text() @@ -555,12 +492,7 @@ def _run_with_fake_uv( else: env["POLICYENGINE_ARTIFACT_BUCKET"] = bucket - result = subprocess.run( - ["bash", str(self.script), *args], - capture_output=True, - text=True, - env=env, - ) + result = _run_bash(str(self.script), *args, env=env) calls = ( log_path.read_text(encoding="utf-8").splitlines() if log_path.exists() @@ -577,20 +509,12 @@ def test_script_exists(self): def test_script_syntax(self): """Script should have valid bash syntax.""" - result = subprocess.run( - ["bash", "-n", str(self.script)], - capture_output=True, - text=True, - ) + result = _run_bash("-n", str(self.script)) assert result.returncode == 0, f"Syntax error: {result.stderr}" def test_requires_modal_environment_argument(self): """Should fail when no modal environment is provided.""" - result = subprocess.run( - ["bash", str(self.script)], - capture_output=True, - text=True, - ) + result = _run_bash(str(self.script)) assert result.returncode != 0, "Should fail without modal environment" def test_requires_artifact_bucket(self, tmp_path): @@ -784,12 +708,7 @@ def _run_with_fake_uv(self, tmp_path, *args, bucket="policyengine-sim-artifacts" else: env["POLICYENGINE_ARTIFACT_BUCKET"] = bucket - result = subprocess.run( - ["bash", str(self.script), *args], - capture_output=True, - text=True, - env=env, - ) + result = _run_bash(str(self.script), *args, env=env) calls = ( log_path.read_text(encoding="utf-8").splitlines() if log_path.exists() @@ -803,11 +722,7 @@ def test_script_exists(self): def test_requires_environment_argument(self): """Should fail with no arguments.""" - result = subprocess.run( - ["bash", str(self.script)], - capture_output=True, - text=True, - ) + result = _run_bash(str(self.script)) assert result.returncode != 0 def test_requires_manifest_digest_argument(self, tmp_path): @@ -879,20 +794,12 @@ def test_script_exists(self): def test_script_syntax(self): """Script should have valid bash syntax.""" - result = subprocess.run( - ["bash", "-n", str(self.script)], - capture_output=True, - text=True, - ) + result = _run_bash("-n", str(self.script)) assert result.returncode == 0, f"Syntax error: {result.stderr}" def test_requires_modal_environment_argument(self): """Should fail when no modal environment is provided.""" - result = subprocess.run( - ["bash", str(self.script)], - capture_output=True, - text=True, - ) + result = _run_bash(str(self.script)) assert result.returncode != 0, "Should fail without modal environment" @@ -907,11 +814,7 @@ def test_script_exists(self): def test_script_syntax(self): """Script should have valid bash syntax.""" - result = subprocess.run( - ["bash", "-n", str(self.script)], - capture_output=True, - text=True, - ) + result = _run_bash("-n", str(self.script)) assert result.returncode == 0, f"Syntax error: {result.stderr}" @@ -926,29 +829,17 @@ def test_script_exists(self): def test_script_syntax(self): """Script should have valid bash syntax.""" - result = subprocess.run( - ["bash", "-n", str(self.script)], - capture_output=True, - text=True, - ) + result = _run_bash("-n", str(self.script)) assert result.returncode == 0, f"Syntax error: {result.stderr}" def test_requires_environment_argument(self): """Should fail when no environment is provided.""" - result = subprocess.run( - ["bash", str(self.script)], - capture_output=True, - text=True, - ) + result = _run_bash(str(self.script)) assert result.returncode != 0, "Should fail without environment" def test_requires_base_url_argument(self): """Should fail when no base URL is provided.""" - result = subprocess.run( - ["bash", str(self.script), "beta"], - capture_output=True, - text=True, - ) + result = _run_bash(str(self.script), "beta") assert result.returncode != 0, "Should fail without base URL" def test_fails_when_auth_config_is_partial(self): @@ -959,12 +850,7 @@ def test_fails_when_auth_config_is_partial(self): env.pop("SIMULATION_TEST_AUTH_CLIENT_ID", None) env.pop("SIMULATION_TEST_AUTH_CLIENT_SECRET", None) - result = subprocess.run( - ["bash", str(self.script), "beta", "https://example.com"], - capture_output=True, - text=True, - env=env, - ) + result = _run_bash(str(self.script), "beta", "https://example.com", env=env) assert result.returncode != 0 assert "Simulation integration-test auth config is partial." in result.stderr @@ -981,12 +867,7 @@ def test_fails_when_auth_required_but_auth_vars_missing(self): ): env.pop(key, None) - result = subprocess.run( - ["bash", str(self.script), "beta", "https://example.com"], - capture_output=True, - text=True, - env=env, - ) + result = _run_bash(str(self.script), "beta", "https://example.com", env=env) assert result.returncode != 0 assert "SIMULATION_TEST_AUTH_REQUIRED is enabled" in result.stderr @@ -1018,17 +899,12 @@ def test_exports_us_and_uk_model_versions_to_integration_tests(self, tmp_path): ): env.pop(key, None) - result = subprocess.run( - [ - "bash", - str(self.script), - "prod", - "https://example.com", - "1.690.7", - "2.88.20", - ], - capture_output=True, - text=True, + result = _run_bash( + str(self.script), + "prod", + "https://example.com", + "1.690.7", + "2.88.20", env=env, cwd=REPO_ROOT, ) @@ -1060,11 +936,7 @@ def test_all_scripts_have_strict_mode(self, all_modal_scripts): def test_all_scripts_have_valid_syntax(self, all_modal_scripts): """All scripts should pass bash syntax check.""" for script in all_modal_scripts: - result = subprocess.run( - ["bash", "-n", str(script)], - capture_output=True, - text=True, - ) + result = _run_bash("-n", str(script)) assert result.returncode == 0, ( f"{script.name} has syntax errors: {result.stderr}" ) diff --git a/projects/policyengine-simulation-executor/tests/test_policyengine_dependency_source.py b/projects/policyengine-simulation-executor/tests/test_policyengine_dependency_source.py index 1450fe5b7..7cafeec5c 100644 --- a/projects/policyengine-simulation-executor/tests/test_policyengine_dependency_source.py +++ b/projects/policyengine-simulation-executor/tests/test_policyengine_dependency_source.py @@ -96,6 +96,19 @@ def _modal_import_env() -> dict[str, str]: return env +def _run_python_snippet( + code: str, env: dict[str, str] +) -> subprocess.CompletedProcess[str]: + """Run ``code`` in a fresh interpreter from the project root, capturing text output.""" + return subprocess.run( # noqa: S603 # runs a test-owned snippet with this interpreter + [sys.executable, "-c", code], + cwd=REPO_ROOT, + env=env, + capture_output=True, + text=True, + ) + + def test_modal_app_remote_import_uses_image_version_env(): code = """ import json @@ -124,13 +137,7 @@ def fail(message): """ env = _modal_import_env() env.update(VERSION_ENV) - result = subprocess.run( - [sys.executable, "-c", code], - cwd=REPO_ROOT, - env=env, - capture_output=True, - text=True, - ) + result = _run_python_snippet(code, env) assert result.returncode == 0, result.stderr output = result.stdout.strip() @@ -147,13 +154,7 @@ def test_modal_app_remote_import_fails_clearly_without_version_env(): import src.modal.app """ - result = subprocess.run( - [sys.executable, "-c", code], - cwd=REPO_ROOT, - env=_modal_import_env(), - capture_output=True, - text=True, - ) + result = _run_python_snippet(code, _modal_import_env()) assert result.returncode != 0 assert "POLICYENGINE_VERSION must be set" in result.stderr diff --git a/projects/policyengine-simulation-executor/tests/test_policyengine_package_update_scripts.py b/projects/policyengine-simulation-executor/tests/test_policyengine_package_update_scripts.py index a627dd5a8..21971edfc 100644 --- a/projects/policyengine-simulation-executor/tests/test_policyengine_package_update_scripts.py +++ b/projects/policyengine-simulation-executor/tests/test_policyengine_package_update_scripts.py @@ -23,8 +23,8 @@ def test_update_policyengine_package_script_has_valid_bash_syntax() -> None: - result = subprocess.run( - ["bash", "-n", str(SCRIPT)], + result = subprocess.run( # noqa: S603 # checks the syntax of a repository script + ["bash", "-n", str(SCRIPT)], # noqa: S607 # bash from PATH, as the workflow runs the script capture_output=True, text=True, check=False, @@ -41,7 +41,7 @@ def test_update_policyengine_package_uses_a_separate_python_helper() -> None: assert "python3 -c" not in source assert "python -c" not in source assert "<<'PY'" not in source - result = subprocess.run( + result = subprocess.run( # noqa: S603 # runs the repository helper script with this interpreter [sys.executable, str(helper), "--help"], capture_output=True, text=True, diff --git a/projects/policyengine-simulation-gateway/tests/integration/test_image_smoke_modal.py b/projects/policyengine-simulation-gateway/tests/integration/test_image_smoke_modal.py index f2ceb0be2..ae88bb2bc 100644 --- a/projects/policyengine-simulation-gateway/tests/integration/test_image_smoke_modal.py +++ b/projects/policyengine-simulation-gateway/tests/integration/test_image_smoke_modal.py @@ -16,8 +16,8 @@ @pytest.mark.integration def test_gateway_image_smoke(): env_name = os.environ.get("MODAL_SMOKE_ENV", "staging") - result = subprocess.run( - [ + result = subprocess.run( # noqa: S603 # runs the repository smoke app through uv and Modal + [ # noqa: S607 # uv from PATH, as CI runs it "uv", "run", "modal", From a18a95dacfccc0fd175319bb0f81cc321063ca91 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Tue, 13 Oct 2026 21:42:49 +0400 Subject: [PATCH 06/10] Fix the remaining ruff S findings Build the GCP metadata request inline from its constant http URL, with the same Metadata-Flavor header, so S310 can see the scheme. Keep every other finding with a reasoned suppression: - S105/S106: fake tokens in tests (one module constant per test module where a value repeats), and names or fingerprints that only look like secrets. - S108: the executor's fixed data directories inside the Modal container, and a lookup path the tests' fakes only pass along. - S110: add S110 to the existing reasoned BLE001 suppressions. - S102: the image tests execute repository-owned app source as Modal's /root/app.py. - S301: the SPM tests round-trip errors they built or raised. - S310: the Hub API request URL is built from the HF_ENDPOINT base and quoted path parts. Co-Authored-By: Claude Opus 5.5 --- .../policyengine_fastapi/opentelemetry/gcp.py | 14 ++++++++++---- .../hf_dataset.py | 4 ++-- .../tests/test_hf_dataset.py | 6 ++++-- .../errors.py | 2 +- .../src/modal/app.py | 2 +- .../src/modal/segmented_national.py | 2 +- .../hf_access_validation.py | 4 ++-- .../release_bundle.py | 2 +- .../simulation_runtime.py | 4 ++-- .../tests/test_canonical_spm.py | 4 ++-- .../tests/test_hf_access_validation.py | 2 +- .../tests/test_modal_bundle_image.py | 2 +- .../tests/test_modal_scripts.py | 16 +++++++++------- .../tests/test_simulation_output_builder.py | 6 ++++-- .../tests/test_simulation_output_geographic.py | 6 ++++-- .../tests/test_validate_hf_access.py | 4 ++-- .../src/policyengine_simulation_gateway/auth.py | 2 +- .../tests/test_modal_gateway_image.py | 2 +- 18 files changed, 49 insertions(+), 35 deletions(-) diff --git a/libs/policyengine-fastapi/src/policyengine_fastapi/opentelemetry/gcp.py b/libs/policyengine-fastapi/src/policyengine_fastapi/opentelemetry/gcp.py index b96ad4f4d..fc2b13c21 100644 --- a/libs/policyengine-fastapi/src/policyengine_fastapi/opentelemetry/gcp.py +++ b/libs/policyengine-fastapi/src/policyengine_fastapi/opentelemetry/gcp.py @@ -51,11 +51,17 @@ def _get_project_id() -> str: def _get_project_id_from_metadata() -> str | None: from urllib import request - url = "http://metadata.google.internal/computeMetadata/v1/project/project-id" - req = request.Request(url) - req.add_header("Metadata-Flavor", "Google") try: - return request.urlopen(req).read().decode() + return ( + request.urlopen( + request.Request( + "http://metadata.google.internal/computeMetadata/v1/project/project-id", + headers={"Metadata-Flavor": "Google"}, + ) + ) + .read() + .decode() + ) except Exception as err: # noqa: BLE001 # best-effort probe: any failure falls back to GOOGLE_CLOUD_PROJECT log.info( "Unable to get the google project id from a local metadata service: %s", diff --git a/libs/policyengine-simulation-contract/src/policyengine_simulation_contract/hf_dataset.py b/libs/policyengine-simulation-contract/src/policyengine_simulation_contract/hf_dataset.py index d76e67da7..49b053282 100644 --- a/libs/policyengine-simulation-contract/src/policyengine_simulation_contract/hf_dataset.py +++ b/libs/policyengine-simulation-contract/src/policyengine_simulation_contract/hf_dataset.py @@ -83,9 +83,9 @@ def _fetch_hf_dataset_revision( if token is not None: headers["Authorization"] = f"Bearer {token}" - request = Request(url, headers=headers) + request = Request(url, headers=headers) # noqa: S310 # Hub API URL: the HF_ENDPOINT base (https by default) and quoted path parts try: - with urlopen(request, timeout=HF_REQUEST_TIMEOUT_SECONDS) as response: + with urlopen(request, timeout=HF_REQUEST_TIMEOUT_SECONDS) as response: # noqa: S310 # the Hub API request built above return json.loads(response.read().decode("utf-8")) except HTTPError as exc: detail = exc.reason or f"HTTP {exc.code}" diff --git a/libs/policyengine-simulation-contract/tests/test_hf_dataset.py b/libs/policyengine-simulation-contract/tests/test_hf_dataset.py index ab2473134..336d229db 100644 --- a/libs/policyengine-simulation-contract/tests/test_hf_dataset.py +++ b/libs/policyengine-simulation-contract/tests/test_hf_dataset.py @@ -13,6 +13,8 @@ with_hf_revision, ) +RUNTIME_TOKEN = "runtime-token" # noqa: S105 # fake token; the Hub fetch is stubbed + class _FakeResponse: def __init__(self, payload: dict): @@ -75,12 +77,12 @@ def fake_fetch(repo_id, revision, token): return {"siblings": [{"rfilename": "dataset.h5"}]} monkeypatch.setattr(hf_dataset, "_fetch_hf_dataset_revision", fake_fetch) - monkeypatch.setenv("HUGGING_FACE_TOKEN", "runtime-token") + monkeypatch.setenv("HUGGING_FACE_TOKEN", RUNTIME_TOKEN) monkeypatch.setenv("PE_UK_PRIVATE_HF_READ_TOKEN", "deployment-token") monkeypatch.setenv("HF_TOKEN", "legacy-token") assert validate_hf_dataset_uri(dataset_uri) == dataset_uri - assert seen["token"] == "runtime-token" + assert seen["token"] == RUNTIME_TOKEN def test_validate_hf_dataset_uri_ignores_non_runtime_credential_names(monkeypatch): diff --git a/libs/policyengine-simulation-observability/src/policyengine_simulation_observability/errors.py b/libs/policyengine-simulation-observability/src/policyengine_simulation_observability/errors.py index dc3a4f9d1..119c3b3f5 100644 --- a/libs/policyengine-simulation-observability/src/policyengine_simulation_observability/errors.py +++ b/libs/policyengine-simulation-observability/src/policyengine_simulation_observability/errors.py @@ -91,7 +91,7 @@ def log_and_redact_exception( "gateway_error_redacted", attributes=payload, ) - except Exception: # noqa: BLE001 # pragma: no cover - defensive, never raise from logger + except Exception: # noqa: BLE001, S110 # pragma: no cover - defensive, never raise from logger pass return f"{GENERIC_JOB_FAILURE_MESSAGE} (correlation_id={correlation_id})" diff --git a/projects/policyengine-simulation-executor/src/modal/app.py b/projects/policyengine-simulation-executor/src/modal/app.py index 5ae7de06c..00f5f2ace 100644 --- a/projects/policyengine-simulation-executor/src/modal/app.py +++ b/projects/policyengine-simulation-executor/src/modal/app.py @@ -258,7 +258,7 @@ def _configure_modal_observability( def _set_modal_call_attributes(runtime: "ObservabilityRuntime") -> None: try: runtime.set_context(function_call_id=modal.current_function_call_id()) - except Exception: # noqa: BLE001 # telemetry is non-fatal: a failed attribute must never fail the Modal call + except Exception: # noqa: BLE001, S110 # telemetry is non-fatal: a failed attribute must never fail the Modal call pass diff --git a/projects/policyengine-simulation-executor/src/modal/segmented_national.py b/projects/policyengine-simulation-executor/src/modal/segmented_national.py index 875b82d6c..17594a173 100644 --- a/projects/policyengine-simulation-executor/src/modal/segmented_national.py +++ b/projects/policyengine-simulation-executor/src/modal/segmented_national.py @@ -320,7 +320,7 @@ def _cancel_all(self, handles: list[tuple[list[str], Any]]) -> None: for _, call in handles: try: call.cancel() - except Exception: # noqa: BLE001 # best-effort cleanup: a failed cancel must not mask the child failure being raised + except Exception: # noqa: BLE001, S110 # best-effort cleanup: a failed cancel must not mask the child failure being raised pass def _reduce( diff --git a/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/hf_access_validation.py b/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/hf_access_validation.py index 30358bb9b..d08f89188 100644 --- a/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/hf_access_validation.py +++ b/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/hf_access_validation.py @@ -16,8 +16,8 @@ ) HF_RUNTIME_ENV_NAME = "HUGGING_FACE_TOKEN" -HF_MODAL_SECRET_NAME = "pe-uk-private-hf-read-token" -EXPECTED_HF_TOKEN_DISPLAY_NAME = "pe-uk-private-hf-read-token" +HF_MODAL_SECRET_NAME = "pe-uk-private-hf-read-token" # noqa: S105 # the Modal secret's name, not its value +EXPECTED_HF_TOKEN_DISPLAY_NAME = "pe-uk-private-hf-read-token" # noqa: S105 # the token's Hugging Face display name, not its value class HFAccessTarget(BaseModel): diff --git a/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/release_bundle.py b/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/release_bundle.py index feed3409a..2c2aadf7c 100644 --- a/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/release_bundle.py +++ b/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/release_bundle.py @@ -446,7 +446,7 @@ def _receipt_path() -> Path: explicit = os.environ.get("POLICYENGINE_BUNDLE_RECEIPT") if explicit: return Path(explicit) - data_folder = os.environ.get("POLICYENGINE_DATA_FOLDER", "/tmp/policyengine-data") + data_folder = os.environ.get("POLICYENGINE_DATA_FOLDER", "/tmp/policyengine-data") # noqa: S108 # fixed data directory inside the Modal container return Path(data_folder) / BUNDLE_RECEIPT_FILENAME diff --git a/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/simulation_runtime.py b/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/simulation_runtime.py index 09739c4aa..4ddad5656 100644 --- a/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/simulation_runtime.py +++ b/projects/policyengine-simulation-executor/src/policyengine_simulation_executor/simulation_runtime.py @@ -637,13 +637,13 @@ def resolve_data_folder() -> str: this folder for ``ensure_datasets``/``Simulation.ensure()`` to find, so any divergence from ``_load_dataset``'s resolution strands them. """ - return os.environ.get("POLICYENGINE_DATA_FOLDER", "/tmp/policyengine-data") + return os.environ.get("POLICYENGINE_DATA_FOLDER", "/tmp/policyengine-data") # noqa: S108 # fixed data directory inside the Modal container def _nondefault_data_folder(country: str, name: str, uri: str) -> str: """Separate managed datasets even if their source filenames have the same stem.""" identity = hashlib.sha256(f"{country}:{name}:{uri}".encode()).hexdigest() - return f"/tmp/policyengine-alternate-data/{identity}" + return f"/tmp/policyengine-alternate-data/{identity}" # noqa: S108 # fixed data directory inside the Modal container def _resolve_dataset_selection( diff --git a/projects/policyengine-simulation-executor/tests/test_canonical_spm.py b/projects/policyengine-simulation-executor/tests/test_canonical_spm.py index 899b4e52d..afd643638 100644 --- a/projects/policyengine-simulation-executor/tests/test_canonical_spm.py +++ b/projects/policyengine-simulation-executor/tests/test_canonical_spm.py @@ -516,7 +516,7 @@ def test_an_unavailable_capability_is_rederived_every_time(self, monkeypatch): ) def test_actual_http_formula_error_contract(monkeypatch, code, observability_runtime): error = SPMInputError(code, "Explicit input is required") - assert pickle.loads(pickle.dumps(error)).to_dict() == error.to_dict() + assert pickle.loads(pickle.dumps(error)).to_dict() == error.to_dict() # noqa: S301 # round-trips an object the test built monkeypatch.setattr( "policyengine_simulation_executor.simulation.run_simulation_impl", Mock(side_effect=error), @@ -816,7 +816,7 @@ def __init__(self, code, message): simulation_runtime.run_simulation_impl( {"country": "us"}, runtime=observability_runtime ) - transported = pickle.loads(pickle.dumps(caught.value)) + transported = pickle.loads(pickle.dumps(caught.value)) # noqa: S301 # round-trips an object the test raised assert transported.to_dict() == {"code": code, "message": "Observed input required"} diff --git a/projects/policyengine-simulation-executor/tests/test_hf_access_validation.py b/projects/policyengine-simulation-executor/tests/test_hf_access_validation.py index 0ef6f8b92..a24940b1f 100644 --- a/projects/policyengine-simulation-executor/tests/test_hf_access_validation.py +++ b/projects/policyengine-simulation-executor/tests/test_hf_access_validation.py @@ -54,7 +54,7 @@ def test_bundle_access_plan_covers_both_private_repositories() -> None: def test_validation_returns_only_safe_identity_and_artifact_metadata() -> None: - token = "private-test-token" + token = "private-test-token" # noqa: S105 # fake token; the loaders are stubbed target = _target() result = validate_uk_private_hf_access( diff --git a/projects/policyengine-simulation-executor/tests/test_modal_bundle_image.py b/projects/policyengine-simulation-executor/tests/test_modal_bundle_image.py index 8d57ad6d6..af5682dd1 100644 --- a/projects/policyengine-simulation-executor/tests/test_modal_bundle_image.py +++ b/projects/policyengine-simulation-executor/tests/test_modal_bundle_image.py @@ -301,7 +301,7 @@ def test_app_module_imports_at_container_entrypoint_path(monkeypatch): ) module = importlib.util.module_from_spec(spec) module.__file__ = "/root/app.py" - exec(code, module.__dict__) + exec(code, module.__dict__) # noqa: S102 # executes repository-owned app source as Modal's /root/app.py assert module.APP_NAME.startswith("policyengine-simulation-py") fetch_calls = [ diff --git a/projects/policyengine-simulation-executor/tests/test_modal_scripts.py b/projects/policyengine-simulation-executor/tests/test_modal_scripts.py index 753a4a80a..9600d5af5 100644 --- a/projects/policyengine-simulation-executor/tests/test_modal_scripts.py +++ b/projects/policyengine-simulation-executor/tests/test_modal_scripts.py @@ -13,6 +13,8 @@ pytest_plugins = ("fixtures.test_modal_scripts",) +FAKE_HF_TOKEN = "hf_test" # noqa: S105 # fake token; the scripts under test run stubbed commands + def _run_bash( *arguments: str, @@ -168,7 +170,7 @@ def test_syncs_expected_hf_key_before_image_smokes(self, tmp_path): { "PATH": f"{fake_bin}:{env['PATH']}", "UV_CALLS_LOG": str(uv_calls_log), - "PE_UK_PRIVATE_HF_READ_TOKEN": "hf_test", + "PE_UK_PRIVATE_HF_READ_TOKEN": FAKE_HF_TOKEN, } ) @@ -178,7 +180,7 @@ def test_syncs_expected_hf_key_before_image_smokes(self, tmp_path): calls = uv_calls_log.read_text(encoding="utf-8").splitlines() assert calls[0] == ( "run modal secret create pe-uk-private-hf-read-token " - "HUGGING_FACE_TOKEN=hf_test --env=staging --force" + f"HUGGING_FACE_TOKEN={FAKE_HF_TOKEN} --env=staging --force" ) assert len(calls) == 4 @@ -328,7 +330,7 @@ def test_requires_modal_environment_argument(self): def test_fails_when_gateway_auth_config_is_partial(self): """Should fail before touching Modal when auth config is partial.""" env = os.environ.copy() - env["PE_UK_PRIVATE_HF_READ_TOKEN"] = "hf_test" + env["PE_UK_PRIVATE_HF_READ_TOKEN"] = FAKE_HF_TOKEN env["GCP_CREDENTIALS_JSON"] = '{"type":"service_account"}' env["GATEWAY_AUTH_ISSUER"] = "https://tenant.auth0.com" env.pop("GATEWAY_AUTH_AUDIENCE", None) @@ -343,7 +345,7 @@ def test_fails_when_gateway_auth_config_is_partial(self): def test_fails_when_auth_required_but_gateway_auth_vars_missing(self): """Required auth must refuse deploy when the GitHub secrets are absent.""" env = os.environ.copy() - env["PE_UK_PRIVATE_HF_READ_TOKEN"] = "hf_test" + env["PE_UK_PRIVATE_HF_READ_TOKEN"] = FAKE_HF_TOKEN env["GCP_CREDENTIALS_JSON"] = '{"type":"service_account"}' env["GATEWAY_AUTH_REQUIRED"] = "1" for key in ( @@ -362,7 +364,7 @@ def test_fails_when_auth_required_but_gateway_auth_vars_missing(self): def test_requires_gcp_credentials(self): """Artifact credential synchronization must fail before deployment.""" env = os.environ.copy() - env["PE_UK_PRIVATE_HF_READ_TOKEN"] = "hf_test" + env["PE_UK_PRIVATE_HF_READ_TOKEN"] = FAKE_HF_TOKEN env.pop("GCP_CREDENTIALS_JSON", None) result = _run_bash(str(self.script), "staging", env=env) @@ -384,7 +386,7 @@ def test_creates_gateway_secret_with_normalized_issuer(self, tmp_path): { "PATH": f"{fake_bin}:{env['PATH']}", "UV_CALLS_LOG": str(uv_calls_log), - "PE_UK_PRIVATE_HF_READ_TOKEN": "hf_test", + "PE_UK_PRIVATE_HF_READ_TOKEN": FAKE_HF_TOKEN, "GCP_CREDENTIALS_JSON": '{"type":"service_account"}', "GATEWAY_AUTH_ISSUER": "https://tenant.auth0.com", "GATEWAY_AUTH_AUDIENCE": "https://simulation-api-beta.policyengine.org", @@ -428,7 +430,7 @@ def test_gcp_secret_sync_failure_stops_deployment(self, tmp_path): { "PATH": f"{fake_bin}:{env['PATH']}", "UV_CALLS_LOG": str(uv_calls_log), - "PE_UK_PRIVATE_HF_READ_TOKEN": "hf_test", + "PE_UK_PRIVATE_HF_READ_TOKEN": FAKE_HF_TOKEN, "GCP_CREDENTIALS_JSON": '{"type":"service_account"}', } ) diff --git a/projects/policyengine-simulation-executor/tests/test_simulation_output_builder.py b/projects/policyengine-simulation-executor/tests/test_simulation_output_builder.py index 37125d961..77e1ccf79 100644 --- a/projects/policyengine-simulation-executor/tests/test_simulation_output_builder.py +++ b/projects/policyengine-simulation-executor/tests/test_simulation_output_builder.py @@ -79,6 +79,8 @@ ) from policyengine_simulation_observability.stages import Stage +CONSTITUENCY_CSV_PATH = "/tmp/constituencies_2024.csv" # noqa: S108 # a path string the fakes pass along; nothing opens it + def _params_with_spm(params, selection): """Return request parameters after the runtime resolves SPM settings.""" @@ -1771,7 +1773,7 @@ def compute(baseline_simulation, reform_simulation, **kwargs): assert baseline_simulation is baseline assert reform_simulation is reform assert kwargs == { - "constituency_csv_path": "/tmp/constituencies_2024.csv", + "constituency_csv_path": CONSTITUENCY_CSV_PATH, "download_missing_assets": False, } return SimpleNamespace(constituency_results=expected) @@ -1785,7 +1787,7 @@ def compute(baseline_simulation, reform_simulation, **kwargs): monkeypatch.setattr( "policyengine_simulation_executor.simulation_output_geographic." "_required_uk_geography_lookup_csv_path", - lambda spec: "/tmp/constituencies_2024.csv", + lambda spec: CONSTITUENCY_CSV_PATH, ) result = _simulation_output_builder( diff --git a/projects/policyengine-simulation-executor/tests/test_simulation_output_geographic.py b/projects/policyengine-simulation-executor/tests/test_simulation_output_geographic.py index 74b05af23..18d1442f7 100644 --- a/projects/policyengine-simulation-executor/tests/test_simulation_output_geographic.py +++ b/projects/policyengine-simulation-executor/tests/test_simulation_output_geographic.py @@ -14,6 +14,8 @@ ) from policyengine_simulation_executor import simulation_output_geographic +CONSTITUENCY_CSV_PATH = "/tmp/constituencies_2024.csv" # noqa: S108 # a path string the fakes pass along; nothing opens it + def _complete_constituency_record() -> dict[str, object]: return { @@ -54,7 +56,7 @@ def resolve(spec, **kwargs): assert spec is CONSTITUENCY_ASSET_SPEC assert kwargs == {"download_missing_assets": True} observed.append("lookup-resolved") - return "/tmp/constituencies_2024.csv" + return CONSTITUENCY_CSV_PATH monkeypatch.setattr( "policyengine_simulation_executor.simulation_runtime.setup_gcp_credentials", @@ -69,7 +71,7 @@ def resolve(spec, **kwargs): CONSTITUENCY_ASSET_SPEC ) - assert result == "/tmp/constituencies_2024.csv" + assert result == CONSTITUENCY_CSV_PATH assert observed == ["credentials-entered", "lookup-resolved"] diff --git a/projects/policyengine-simulation-executor/tests/test_validate_hf_access.py b/projects/policyengine-simulation-executor/tests/test_validate_hf_access.py index e74a50439..30a980436 100644 --- a/projects/policyengine-simulation-executor/tests/test_validate_hf_access.py +++ b/projects/policyengine-simulation-executor/tests/test_validate_hf_access.py @@ -12,9 +12,9 @@ def _audit() -> HFCredentialAudit: return HFCredentialAudit( - token_fingerprint="abc123def456", + token_fingerprint="abc123def456", # noqa: S106 # a fake hash prefix, not a token account="policyengine-runtime-data-reader", - token_display_name="pe-uk-private-hf-read-token", + token_display_name="pe-uk-private-hf-read-token", # noqa: S106 # a display name, not a token artifacts=(), ) diff --git a/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/auth.py b/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/auth.py index f0ed3fb03..e4d042b1f 100644 --- a/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/auth.py +++ b/projects/policyengine-simulation-gateway/src/policyengine_simulation_gateway/auth.py @@ -186,7 +186,7 @@ def enforce_production_auth_guard(runtime: ObservabilityRuntime) -> None: "ack_value_present": True, }, ) - except Exception: # noqa: BLE001 # pragma: no cover - observability must never block startup + except Exception: # noqa: BLE001, S110 # pragma: no cover - observability must never block startup pass diff --git a/projects/policyengine-simulation-gateway/tests/test_modal_gateway_image.py b/projects/policyengine-simulation-gateway/tests/test_modal_gateway_image.py index 907630ee0..5d8ac0a84 100644 --- a/projects/policyengine-simulation-gateway/tests/test_modal_gateway_image.py +++ b/projects/policyengine-simulation-gateway/tests/test_modal_gateway_image.py @@ -166,6 +166,6 @@ def test_app_module_imports_at_container_entrypoint_path(monkeypatch): ) module = importlib.util.module_from_spec(spec) module.__file__ = "/root/app.py" - exec(code, module.__dict__) + exec(code, module.__dict__) # noqa: S102 # executes repository-owned app source as Modal's /root/app.py assert module.app.name == "policyengine-simulation-gateway" From 8ddd2b72f4e578b13d2c0820b42e6f44669feee3 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Tue, 13 Oct 2026 21:43:03 +0400 Subject: [PATCH 07/10] Enable ruff group 6 (security, logging and debugging leftovers) Select DTZ, ERA, EXE, FIX, G, LOG, S, T20 and TD. The earlier commits on this branch fixed or suppressed, with a reason, every finding outside the shared scripts/rules tool. Co-Authored-By: Claude Opus 5.5 --- ruff.toml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ruff.toml b/ruff.toml index 9349cafaf..275be0394 100644 --- a/ruff.toml +++ b/ruff.toml @@ -19,6 +19,8 @@ select = [ "D", # Group 5: exceptions and messages. "BLE", "EM", "TRY", + # Group 6: security, logging and debugging leftovers. + "DTZ", "ERA", "EXE", "FIX", "G", "LOG", "S", "T20", "TD", ] # Mutually exclusive pydocstyle pairs: keep D211 (no blank line before a class # docstring) and D212 (summary on the first line). From 1a49d13772cdcf4bd3a4435018f3b82aedad8796 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Tue, 13 Oct 2026 21:46:05 +0400 Subject: [PATCH 08/10] Register group 6 and the deployed-test ruff exemption sim.quality.ruff-lint now lists DTZ, ERA, EXE, FIX, G, LOG, S, T20 and TD. The authenticated_tests exemption gets its own rule, since a ruff per-file-ignore rule names one pattern. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 3 ++- changelog_entry.yaml | 1 + docs/generated/rules.md | 19 ++++++++++++------- rules/registry.toml | 21 ++++++++++++++++++++- 4 files changed, 35 insertions(+), 9 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 3efdaa8a1..154b9a427 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -135,10 +135,11 @@ To add or change a rule, read `docs/engineering/skills/rules-registry.md`. ### Code quality and tests +- `sim.quality.deployed-tests-ruff-exemptions`: The deployed-service test suite (projects/*/authenticated_tests, pytest run against a live candidate) gets the same ruff exemptions as unit tests (S101, D1, ANN, PLR2004); every other ruff rule applies to it. (blocking: ruff per-file ignore of ANN, D1, PLR2004, S101 for `**/authenticated_tests/**`) - `sim.quality.every-project-tested`: Every project and library with unit tests runs them in the PR test matrix. (doc-only, not checked: follow it yourself) - `sim.quality.ruff-format-src`: Source code under projects/*/src and libs/*/src is formatted with ruff format. (blocking: ruff format) - `sim.quality.ruff-format-tests`: Test code is formatted with ruff format too; CI currently checks only src directories. (doc-only, not checked: follow it yourself) -- `sim.quality.ruff-lint`: All Python code passes ruff check with every rule family selected in ruff.toml, with no ratchet; families are added one themed group at a time until the selection is ALL. (blocking: ruff check (ANN, BLE, D, E4, E7, E9, EM, F, FLY, FURB, I, PIE, PYI, RET, RSE, TC, TRY, UP, W)) +- `sim.quality.ruff-lint`: All Python code passes ruff check with every rule family selected in ruff.toml, with no ratchet; families are added one themed group at a time until the selection is ALL. (blocking: ruff check (ANN, BLE, D, DTZ, E4, E7, E9, EM, ERA, EXE, F, FIX, FLY, FURB, G, I, LOG, PIE, PYI, RET, RSE, S, T20, TC, TD, TRY, UP, W)) - `sim.quality.tests-pass`: Each project's unit tests pass in its own locked environment. (blocking: CI job `test`) - `sim.quality.tests-ruff-exemptions`: Test files may use pytest's bare assert and need no docstrings, annotations or named constants for magic numbers (S101, D1, ANN, PLR2004); every other ruff rule applies to tests. (blocking: ruff per-file ignore of ANN, D1, PLR2004, S101 for `**/tests/**`) diff --git a/changelog_entry.yaml b/changelog_entry.yaml index 4c5fd5f51..fb45e4266 100644 --- a/changelog_entry.yaml +++ b/changelog_entry.yaml @@ -4,6 +4,7 @@ - Made CLAUDE.md and the Copilot instructions thin pointers to AGENTS.md, required workflow logic longer than one line to live in tested and shellchecked scripts, and registered the tracking-issue rule - Added a rules registry (rules/registry.toml) with a standard-library tool that validates it, checks every rule is enforced where it claims, and renders docs/generated/rules.md and the Rules block in AGENTS.md changed: + - Enabled ruff's security, logging and debugging rules (S, T20, LOG, G, DTZ, ERA, TD, FIX, EXE); asserts outside tests became explicit checks, command-line scripts keep their printed output, test subprocess calls go through helpers, and every deliberate exception carries a reasoned suppression; the gateway's auth misconfiguration log no longer repeats its message, and the deployed-service tests share the unit-test ruff exemptions - Enabled ruff's exception rules (TRY, EM, BLE); exception messages are built before the raise, raises inside try blocks moved into helpers, and every deliberate catch-all or traceback-free error log carries a reasoned suppression; message text is unchanged, and error types change only in an operator script's two type checks - Enabled ruff's docstring rules (D) and documented every module, class and function outside tests; published contract models and routes now carry these descriptions in their JSON schema and OpenAPI, with no structural schema change - Enabled ruff's annotation rules (ANN) and annotated every function outside tests; test files are exempt from ANN, missing docstrings, magic numbers and bare-assert rules diff --git a/docs/generated/rules.md b/docs/generated/rules.md index ad5698ba4..5e0c58d19 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -8,11 +8,11 @@ Every rule that governs `policyengine-sim-api` is listed here with how it is enf | Tier | enforced | ratcheted | advisory | doc-only | Total | | --- | ---: | ---: | ---: | ---: | ---: | -| mechanical | 33 | 1 | 0 | 14 | 48 | +| mechanical | 34 | 1 | 0 | 14 | 49 | | metric | 0 | 0 | 0 | 0 | 0 | | judgment | 0 | 0 | 0 | 0 | 0 | -**Blocking:** 34 of 48 rules. +**Blocking:** 35 of 49 rules. ## Gaps @@ -242,34 +242,39 @@ flowchart LR | Rule | Statement | Tier | Status | Blocking | Enforced by | CI | Proof | | --- | --- | --- | --- | --- | --- | --- | --- | +| `sim.quality.deployed-tests-ruff-exemptions` | The deployed-service test suite (projects/*/authenticated_tests, pytest run against a live candidate) gets the same ruff exemptions as unit tests (S101, D1, ANN, PLR2004); every other ruff rule applies to it. | mechanical | enforced | yes | ruff per-file ignore of ANN, D1, PLR2004, S101 for `**/authenticated_tests/**` | `pr.yml:lint` | — | | `sim.quality.every-project-tested` | Every project and library with unit tests runs them in the PR test matrix. | mechanical | doc-only | no | nothing | — | — | | `sim.quality.ruff-format-src` | Source code under projects/*/src and libs/*/src is formatted with ruff format. | mechanical | enforced | yes | ruff format | `pr.yml:lint` | — | | `sim.quality.ruff-format-tests` | Test code is formatted with ruff format too; CI currently checks only src directories. | mechanical | doc-only | no | nothing | — | — | -| `sim.quality.ruff-lint` | All Python code passes ruff check with every rule family selected in ruff.toml, with no ratchet; families are added one themed group at a time until the selection is ALL. | mechanical | enforced | yes | ruff check (ANN, BLE, D, E4, E7, E9, EM, F, FLY, FURB, I, PIE, PYI, RET, RSE, TC, TRY, UP, W) | `pr.yml:lint` | — | +| `sim.quality.ruff-lint` | All Python code passes ruff check with every rule family selected in ruff.toml, with no ratchet; families are added one themed group at a time until the selection is ALL. | mechanical | enforced | yes | ruff check (ANN, BLE, D, DTZ, E4, E7, E9, EM, ERA, EXE, F, FIX, FLY, FURB, G, I, LOG, PIE, PYI, RET, RSE, S, T20, TC, TD, TRY, UP, W) | `pr.yml:lint` | — | | `sim.quality.tests-pass` | Each project's unit tests pass in its own locked environment. | mechanical | enforced | yes | CI job `test` | `pr.yml:test` | — | | `sim.quality.tests-ruff-exemptions` | Test files may use pytest's bare assert and need no docstrings, annotations or named constants for magic numbers (S101, D1, ANN, PLR2004); every other ruff rule applies to tests. | mechanical | enforced | yes | ruff per-file ignore of ANN, D1, PLR2004, S101 for `**/tests/**` | `pr.yml:lint` | — | ```mermaid flowchart LR + r_sim_quality_deployed_tests_ruff_exemptions("sim.quality.deployed-tests-ruff-exemptions (enforced)") + e_ruff_per_file_ignore_of_ANN__D1__PLR2004__S101_for____authenticated_tests___["ruff per-file ignore of ANN, D1, PLR2004, S101 for **/authenticated_tests/**"] + j_CI_pr_yml_lint["CI pr.yml:lint"] r_sim_quality_every_project_tested("sim.quality.every-project-tested (doc-only)") gap_unenforced>"not enforced"] r_sim_quality_ruff_format_src("sim.quality.ruff-format-src (enforced)") e_ruff_format["ruff format"] - j_CI_pr_yml_lint["CI pr.yml:lint"] r_sim_quality_ruff_format_tests("sim.quality.ruff-format-tests (doc-only)") r_sim_quality_ruff_lint("sim.quality.ruff-lint (enforced)") - e_ruff_check__ANN__BLE__D__E4__E7__E9__EM__F__FLY__FURB__I__PIE__PYI__RET__RSE__TC__TRY__UP__W_["ruff check (ANN, BLE, D, E4, E7, E9, EM, F, FLY, FURB, I, PIE, PYI, RET, RSE, TC, TRY, UP, W)"] + e_ruff_check__ANN__BLE__D__DTZ__E4__E7__E9__EM__ERA__EXE__F__FIX__FLY__FURB__G__I__LOG__PIE__PYI__RET__RSE__S__T20__TC__TD__TRY__UP__W_["ruff check (ANN, BLE, D, DTZ, E4, E7, E9, EM, ERA, EXE, F, FIX, FLY, FURB, G, I, LOG, PIE, PYI, RET, RSE, S, T20, TC, TD, TRY, UP, W)"] r_sim_quality_tests_pass("sim.quality.tests-pass (enforced)") e_CI_job_test["CI job test"] j_CI_pr_yml_test["CI pr.yml:test"] r_sim_quality_tests_ruff_exemptions("sim.quality.tests-ruff-exemptions (enforced)") e_ruff_per_file_ignore_of_ANN__D1__PLR2004__S101_for____tests___["ruff per-file ignore of ANN, D1, PLR2004, S101 for **/tests/**"] + r_sim_quality_deployed_tests_ruff_exemptions --> e_ruff_per_file_ignore_of_ANN__D1__PLR2004__S101_for____authenticated_tests___ + e_ruff_per_file_ignore_of_ANN__D1__PLR2004__S101_for____authenticated_tests___ --> j_CI_pr_yml_lint r_sim_quality_every_project_tested -.-> gap_unenforced r_sim_quality_ruff_format_src --> e_ruff_format e_ruff_format --> j_CI_pr_yml_lint r_sim_quality_ruff_format_tests -.-> gap_unenforced - r_sim_quality_ruff_lint --> e_ruff_check__ANN__BLE__D__E4__E7__E9__EM__F__FLY__FURB__I__PIE__PYI__RET__RSE__TC__TRY__UP__W_ - e_ruff_check__ANN__BLE__D__E4__E7__E9__EM__F__FLY__FURB__I__PIE__PYI__RET__RSE__TC__TRY__UP__W_ --> j_CI_pr_yml_lint + r_sim_quality_ruff_lint --> e_ruff_check__ANN__BLE__D__DTZ__E4__E7__E9__EM__ERA__EXE__F__FIX__FLY__FURB__G__I__LOG__PIE__PYI__RET__RSE__S__T20__TC__TD__TRY__UP__W_ + e_ruff_check__ANN__BLE__D__DTZ__E4__E7__E9__EM__ERA__EXE__F__FIX__FLY__FURB__G__I__LOG__PIE__PYI__RET__RSE__S__T20__TC__TD__TRY__UP__W_ --> j_CI_pr_yml_lint r_sim_quality_tests_pass --> e_CI_job_test e_CI_job_test --> j_CI_pr_yml_test r_sim_quality_tests_ruff_exemptions --> e_ruff_per_file_ignore_of_ANN__D1__PLR2004__S101_for____tests___ diff --git a/rules/registry.toml b/rules/registry.toml index b5c3233e0..ddf1f3c34 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -710,7 +710,7 @@ autofix = true [rule.enforcer] kind = "ruff" mode = "lint" -codes = ["ANN", "BLE", "D", "E4", "E7", "E9", "EM", "F", "FLY", "FURB", "I", "PIE", "PYI", "RET", "RSE", "TC", "TRY", "UP", "W"] +codes = ["ANN", "BLE", "D", "DTZ", "E4", "E7", "E9", "EM", "ERA", "EXE", "F", "FIX", "FLY", "FURB", "G", "I", "LOG", "PIE", "PYI", "RET", "RSE", "S", "T20", "TC", "TD", "TRY", "UP", "W"] [[rule.ci]] workflow = ".github/workflows/pr.yml" job = "lint" @@ -735,6 +735,25 @@ workflow = ".github/workflows/pr.yml" job = "lint" invocation = "ruff check ." +[[rule]] +id = "sim.quality.deployed-tests-ruff-exemptions" +category = "quality" +statement = "The deployed-service test suite (projects/*/authenticated_tests, pytest run against a live candidate) gets the same ruff exemptions as unit tests (S101, D1, ANN, PLR2004); every other ruff rule applies to it." +rationale = "docs/engineering/skills/rules-registry.md#baselines" +scope = ["projects/*/authenticated_tests/**/*.py"] +tier = "mechanical" +status = "enforced" +autofix = false +[rule.enforcer] +kind = "ruff" +mode = "per-file-ignore" +pattern = "**/authenticated_tests/**" +codes = ["ANN", "D1", "PLR2004", "S101"] +[[rule.ci]] +workflow = ".github/workflows/pr.yml" +job = "lint" +invocation = "ruff check ." + [[rule]] id = "sim.quality.tests-pass" category = "quality" From c34653481be058dd79557a7d9971bdb2dc1c7548 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Tue, 13 Oct 2026 21:51:14 +0400 Subject: [PATCH 09/10] Sync the rules tool from the Public API's group 6 The shared scripts/rules tool takes the Public API's group 6 fixes: explicit checks instead of asserts, and reasoned suppressions for its git and shellcheck subprocess calls, prints and calendar dates. The rendered rules page picks up the tool's new hash. Co-Authored-By: Claude Opus 5.5 --- scripts/rules/baseline.py | 8 ++++---- scripts/rules/ci_scripts.py | 12 ++++++++---- scripts/rules/cli.py | 19 ++++++++++++------- scripts/rules/guard.py | 2 +- scripts/rules/tests/test_guard.py | 6 +++--- 5 files changed, 28 insertions(+), 19 deletions(-) diff --git a/scripts/rules/baseline.py b/scripts/rules/baseline.py index 5bc3feb90..4ebe33fac 100644 --- a/scripts/rules/baseline.py +++ b/scripts/rules/baseline.py @@ -28,16 +28,16 @@ def load_base_registry(root: Path, ref: str) -> Registry | None: Raise BaseUnavailable when ``ref`` does not name a commit. """ - verify = subprocess.run( - ["git", "rev-parse", "--verify", "--quiet", f"{ref}^{{commit}}"], + verify = subprocess.run( # noqa: S603 # runs git with a fixed command and no shell + ["git", "rev-parse", "--verify", "--quiet", f"{ref}^{{commit}}"], # noqa: S607 # git comes from PATH, as in any checkout the tool runs in cwd=root, capture_output=True, text=True, ) if verify.returncode != 0: raise BaseUnavailable(ref) - shown = subprocess.run( - ["git", "show", f"{ref}:{REGISTRY_PATH.as_posix()}"], + shown = subprocess.run( # noqa: S603 # runs git with a fixed command and no shell + ["git", "show", f"{ref}:{REGISTRY_PATH.as_posix()}"], # noqa: S607 # git comes from PATH, as in any checkout the tool runs in cwd=root, capture_output=True, text=True, diff --git a/scripts/rules/ci_scripts.py b/scripts/rules/ci_scripts.py index 0a0edecd2..e252cea64 100644 --- a/scripts/rules/ci_scripts.py +++ b/scripts/rules/ci_scripts.py @@ -96,7 +96,7 @@ def shellcheck_failures(config: CiScripts, root: Path) -> set[str] | None: ) if not scripts: return set() - result = subprocess.run( + result = subprocess.run( # noqa: S603 # runs the resolved shellcheck on this repository's scripts [shellcheck, "--format=gcc", *scripts], cwd=root, capture_output=True, @@ -149,7 +149,9 @@ def compare(registry: Registry, findings: Findings, root: Path) -> list[Violatio A check that could not run is skipped rather than reported. """ config = registry.ci_scripts - assert config is not None + if config is None: + msg = "the registry has no [ci_scripts] table" + raise TypeError(msg) checks = ( ( "multiline", @@ -197,7 +199,9 @@ def compare(registry: Registry, findings: Findings, root: Path) -> list[Violatio def write_baselines(registry: Registry, findings: Findings, root: Path) -> None: """Record today's findings as the baselines (used once, when adopting the rule).""" config = registry.ci_scripts - assert config is not None + if config is None: + msg = "the registry has no [ci_scripts] table" + raise TypeError(msg) for found, path in ( (findings.multiline, config.multiline_baseline), (findings.untested, config.untested_baseline), @@ -243,5 +247,5 @@ def check() -> list[str]: """Adapter for scripts/run_quality_guards.py.""" violations, notes = run_check(REPO_ROOT) for note in notes: - print(f"note: {note}") + print(f"note: {note}") # noqa: T201 # the guard adapter prints its notes return [str(violation) for violation in violations] diff --git a/scripts/rules/cli.py b/scripts/rules/cli.py index a4134cacb..9df65da56 100644 --- a/scripts/rules/cli.py +++ b/scripts/rules/cli.py @@ -14,12 +14,17 @@ from scripts.rules.validate import validate +def _report(message: str) -> None: + """Print one line of the command's output.""" + print(message) # noqa: T201 # reporting the result is this command's job + + def _print(violations: list, notes: list[str] | None = None) -> int: for note in notes or []: - print(f"note: {note}") + _report(f"note: {note}") for violation in violations: - print(f" - {violation}") - print("rules: failed" if violations else "rules: passed") + _report(f" - {violation}") + _report("rules: failed" if violations else "rules: passed") return 1 if violations else 0 @@ -55,7 +60,7 @@ def main(argv: list[str] | None = None) -> int: ci_scripts.write_baselines( registry, ci_scripts.find(registry.ci_scripts, root), root ) - print("wrote ci_scripts baselines; update the rules' baseline counts") + _report("wrote ci_scripts baselines; update the rules' baseline counts") return 0 violations, notes = ci_scripts.run_check(root) return _print(violations, notes) @@ -68,12 +73,12 @@ def main(argv: list[str] | None = None) -> int: registry, violations = load_registry(root) if registry is None: return _print(violations) - violations += validate(registry, root, date.today()) + violations += validate(registry, root, date.today()) # noqa: DTZ011 # review and expiry dates are calendar days, checked against the local date if args.command == "render": if violations: - print("render refused: fix the registry first") + _report("render refused: fix the registry first") return _print(violations) for path in write_outputs(registry, root): - print(f"wrote {path.relative_to(root)}") + _report(f"wrote {path.relative_to(root)}") return 0 return _print(violations) diff --git a/scripts/rules/guard.py b/scripts/rules/guard.py index 3efa3d4f5..092425003 100644 --- a/scripts/rules/guard.py +++ b/scripts/rules/guard.py @@ -89,7 +89,7 @@ def run_guard( if registry is None: return result - result.violations += validate(registry, root, today or date.today()) + result.violations += validate(registry, root, today or date.today()) # noqa: DTZ011 # review and expiry dates are calendar days, checked against the local date for rule in registry.rules: result.violations += resolve_enforcer(rule, root) result.violations += resolve_ci(rule, root) diff --git a/scripts/rules/tests/test_guard.py b/scripts/rules/tests/test_guard.py index 400a68b7e..731f65d49 100644 --- a/scripts/rules/tests/test_guard.py +++ b/scripts/rules/tests/test_guard.py @@ -589,9 +589,9 @@ def test_compares_against_git_base_ref(self) -> None: repo.write("rules/baselines/ratcheted.txt", "pkg/routes.py\n") repo.render() git = ["git", "-c", "user.email=t@example.com", "-c", "user.name=t"] - subprocess.run(["git", "init", "-q"], cwd=repo.root, check=True) - subprocess.run([*git, "add", "-A"], cwd=repo.root, check=True) - subprocess.run([*git, "commit", "-qm", "base"], cwd=repo.root, check=True) + subprocess.run(["git", "init", "-q"], cwd=repo.root, check=True) # noqa: S607 # git comes from PATH in the temporary test repository + subprocess.run([*git, "add", "-A"], cwd=repo.root, check=True) # noqa: S603 # a fixed git command in the temporary test repository + subprocess.run([*git, "commit", "-qm", "base"], cwd=repo.root, check=True) # noqa: S603 # a fixed git command in the temporary test repository repo.edit("rules/registry.toml", "count = 1", "count = 2") repo.write("rules/baselines/ratcheted.txt", "pkg/routes.py\npkg/other.py\n") repo.render() From 150c339732d481ece20fd4bcc3bcee36bdde334f Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Wed, 14 Oct 2026 20:44:30 +0400 Subject: [PATCH 10/10] Fix the ruff group 6 findings in the code #743 and #744 added #743 and #744 landed on main after this stack branched. Group 6, which this PR enables, flags their new code; each fix follows this PR's conventions: - T201: the routing publisher's new commands report through its _report helper, the package updater's dispatch check through _write_result, and the sentinel CLI gets one _report helper that holds its only print. - S603/S607: the deployment-asset tests route #744's new bash calls through _run_bash, which now takes cwd; the rollback tests and the package-update helper tests get one helper each that holds the only subprocess call, with reasoned suppressions. - S310: the sentinel's HTTP helper opens the deployed entrypoint or the test auth issuer URL that the workflow passes; reasoned suppressions. - S106: a fake bearer token in the sentinel tests. Co-Authored-By: Claude Opus 5.5 --- .../scripts/update_policyengine_package.py | 6 +- .../tests/test_deployment_assets.py | 55 ++++++++------ .../tests/test_simulation_rollback.py | 31 ++++++-- .../src/modal/utils/release_sentinel.py | 19 +++-- .../modal/utils/update_version_registry.py | 36 ++++----- ...est_policyengine_package_update_scripts.py | 75 +++++++++---------- .../tests/test_release_sentinel.py | 2 +- 7 files changed, 126 insertions(+), 98 deletions(-) diff --git a/.github/scripts/update_policyengine_package.py b/.github/scripts/update_policyengine_package.py index fc8d3936b..785d0e96b 100644 --- a/.github/scripts/update_policyengine_package.py +++ b/.github/scripts/update_policyengine_package.py @@ -358,9 +358,9 @@ def verify_dispatch() -> None: ): msg = f"The release dispatch says the UK pair was not promotable ({verdict})" raise SystemExit(msg) - print(f"dispatch_verified={'true' if compared else 'false'}") - print(f"uk_qualification_verdict={verdict}") - print(f"uk_qualification_receipt_sha256={receipt_sha256}") + _write_result(f"dispatch_verified={'true' if compared else 'false'}") + _write_result(f"uk_qualification_verdict={verdict}") + _write_result(f"uk_qualification_receipt_sha256={receipt_sha256}") def _parser() -> argparse.ArgumentParser: diff --git a/projects/policyengine-simulation-entry/tests/test_deployment_assets.py b/projects/policyengine-simulation-entry/tests/test_deployment_assets.py index ad4f6d433..bcb880623 100644 --- a/projects/policyengine-simulation-entry/tests/test_deployment_assets.py +++ b/projects/policyengine-simulation-entry/tests/test_deployment_assets.py @@ -63,6 +63,7 @@ def _run_bash( check: bool, capture_output: bool = False, env: dict[str, str] | None = None, + cwd: Path | None = None, ) -> subprocess.CompletedProcess[str]: """Run bash on a repository deployment script, with text output.""" return subprocess.run( # noqa: S603 # runs repository scripts with test-built arguments @@ -71,6 +72,7 @@ def _run_bash( capture_output=capture_output, text=True, env=env, + cwd=cwd, ) @@ -1133,12 +1135,16 @@ def test_a_failing_sentinel_fails_its_step(tmp_path, job): script = step["run"].replace( "${{ needs.deploy_entrypoint.outputs.candidate_url }}", "https://candidate" ) - result = subprocess.run( + result = _run_bash( # GitHub runs `run` blocks with bash -e -o pipefail. - ["bash", "--noprofile", "--norc", "-eo", "pipefail", "-c", script], + "--noprofile", + "--norc", + "-eo", + "pipefail", + "-c", + script, check=False, capture_output=True, - text=True, env={ "PATH": f"{bin_dir}{os.pathsep}{os.environ['PATH']}", "RUNNER_TEMP": str(tmp_path), @@ -1207,11 +1213,15 @@ def _run_routing_step_with_a_failing_command(tmp_path, step, failed_command, **e plan = runner_temp / "routing-plans" / "routing-plan-prod-1" / "plan.json" plan.parent.mkdir(parents=True) plan.write_text("{}") - result = subprocess.run( - ["bash", "--noprofile", "--norc", "-eo", "pipefail", "-c", step["run"]], + result = _run_bash( + "--noprofile", + "--norc", + "-eo", + "pipefail", + "-c", + step["run"], check=False, capture_output=True, - text=True, env={ "PATH": f"{bin_dir}{os.pathsep}{os.environ['PATH']}", "RUNNER_TEMP": str(runner_temp), @@ -1461,19 +1471,15 @@ def test_failed_release_gate_cannot_report_a_successful_release(tmp_path, failed } env[failed_gate] = "failure" # A later step cannot run after this gate under Actions' bash -e behavior. - result = subprocess.run( - [ - "bash", - "-eo", - "pipefail", - "-c", - gate["run"] + '\ntouch "$1"', - "gate", - str(marker), - ], + result = _run_bash( + "-eo", + "pipefail", + "-c", + gate["run"] + '\ntouch "$1"', + "gate", + str(marker), check=False, capture_output=True, - text=True, env=env, cwd=REPOSITORY_ROOT, ) @@ -1504,11 +1510,13 @@ def test_release_success_requires_completed_finalization_for_published_routing( ] if step.get("name") == "Check deployment result" ) - completed = subprocess.run( - ["bash", "-eo", "pipefail", "-c", gate["run"]], + completed = _run_bash( + "-eo", + "pipefail", + "-c", + gate["run"], check=False, capture_output=True, - text=True, cwd=REPOSITORY_ROOT, env={ "PATH": os.environ["PATH"], @@ -1735,11 +1743,12 @@ def _run_identity_smoke(tmp_path: Path, versions: dict, **expected: str): "FAKE_CURL_VERSIONS": str(versions_file), **expected, } - return subprocess.run( - ["bash", SMOKE_SCRIPT, "https://simulation.example.test", revision], + return _run_bash( + SMOKE_SCRIPT, + "https://simulation.example.test", + revision, check=False, capture_output=True, - text=True, env=env, ) diff --git a/projects/policyengine-simulation-entry/tests/test_simulation_rollback.py b/projects/policyengine-simulation-entry/tests/test_simulation_rollback.py index 972e43b3d..ddb099729 100644 --- a/projects/policyengine-simulation-entry/tests/test_simulation_rollback.py +++ b/projects/policyengine-simulation-entry/tests/test_simulation_rollback.py @@ -30,12 +30,27 @@ ) +def _run_bash( + *arguments: str | Path, + check: bool, + capture_output: bool = False, + env: dict[str, str] | None = None, +) -> subprocess.CompletedProcess[str]: + """Run bash on a repository rollback script or step, with text output.""" + return subprocess.run( # noqa: S603 # runs repository scripts with test-built arguments + ["bash", *arguments], # noqa: S607 # bash from PATH, as the workflows run these scripts + check=check, + capture_output=capture_output, + text=True, + env=env, + ) + + def _validate(**env: str) -> subprocess.CompletedProcess[str]: - return subprocess.run( - ["bash", VALIDATOR], + return _run_bash( + VALIDATOR, check=False, capture_output=True, - text=True, env={"PATH": os.environ["PATH"], **env}, ) @@ -306,11 +321,13 @@ def _run_restore( bin_dir = tmp_path / "bin" bin_dir.mkdir() _fake_gh(bin_dir, attempts, kept, inventory) - return subprocess.run( - ["bash", "-eo", "pipefail", "-c", _restore_script()], + return _run_bash( + "-eo", + "pipefail", + "-c", + _restore_script(), check=False, capture_output=True, - text=True, env={ "PATH": f"{bin_dir}{os.pathsep}{os.environ['PATH']}", "RUNNER_TEMP": str(runner_temp), @@ -400,5 +417,5 @@ def test_rollbacks_share_the_deployment_concurrency_group(): def test_validator_has_valid_shell_syntax_and_is_executable(): - subprocess.run(["bash", "-n", VALIDATOR], check=True) + _run_bash("-n", VALIDATOR, check=True) assert os.access(VALIDATOR, os.X_OK) diff --git a/projects/policyengine-simulation-executor/src/modal/utils/release_sentinel.py b/projects/policyengine-simulation-executor/src/modal/utils/release_sentinel.py index ea776a9ad..9563b0107 100644 --- a/projects/policyengine-simulation-executor/src/modal/utils/release_sentinel.py +++ b/projects/policyengine-simulation-executor/src/modal/utils/release_sentinel.py @@ -184,14 +184,14 @@ def _http( timeout: float = 60, ) -> tuple[int, Mapping[str, str], Any]: data = json.dumps(body).encode() if body is not None else None - request = urllib.request.Request(url, data=data, method=method) + request = urllib.request.Request(url, data=data, method=method) # noqa: S310 # the deployed entrypoint or the test auth issuer, from the workflow request.add_header("Accept", "application/json") if data is not None: request.add_header("Content-Type", "application/json") if token: request.add_header("Authorization", f"Bearer {token}") try: - with urllib.request.urlopen(request, timeout=timeout) as response: + with urllib.request.urlopen(request, timeout=timeout) as response: # noqa: S310 # the request built above status, headers, raw = response.status, response.headers, response.read() except urllib.error.HTTPError as error: status, headers, raw = error.code, error.headers, error.read() @@ -323,6 +323,11 @@ def check_deployment( return problems +def _report(line: str, *, error: bool = False) -> None: + """Print one line of the command's report, to stderr for a failure.""" + print(line, file=sys.stderr if error else sys.stdout) # noqa: T201 # command-line script: stdout and stderr are its report + + def main(argv: list[str] | None = None) -> int: """Run the ``expected`` or ``check`` command and return its exit status.""" parser = argparse.ArgumentParser(description=__doc__.split("\n\n")[0]) @@ -340,7 +345,7 @@ def main(argv: list[str] | None = None) -> int: with open(args.output, "w") as handle: json.dump(expected, handle, indent=2, sort_keys=True) for case in expected["cases"]: - print(f"{case['case_id']} ({case['seconds']}s): {case['values']}") + _report(f"{case['case_id']} ({case['seconds']}s): {case['values']}") return 0 with open(args.expected) as handle: @@ -352,13 +357,11 @@ def main(argv: list[str] | None = None) -> int: timeout_seconds=args.timeout_seconds, ) if problems: - print( - "The deployed API does not reproduce the bundle's numbers:", file=sys.stderr - ) + _report("The deployed API does not reproduce the bundle's numbers:", error=True) for problem in problems: - print(f" {problem}", file=sys.stderr) + _report(f" {problem}", error=True) return 1 - print( + _report( f"{args.base_url} reproduces {len(expected['cases'])} sentinel case(s) for " f"policyengine {expected['policyengine_version']}, UK dataset " f"{expected['uk']['dataset_sha256']}" diff --git a/projects/policyengine-simulation-executor/src/modal/utils/update_version_registry.py b/projects/policyengine-simulation-executor/src/modal/utils/update_version_registry.py index fdc71be2c..aaa05ee34 100644 --- a/projects/policyengine-simulation-executor/src/modal/utils/update_version_registry.py +++ b/projects/policyengine-simulation-executor/src/modal/utils/update_version_registry.py @@ -1238,17 +1238,17 @@ def active_state() -> object: return snapshot.get(ROUTING_STATE_ACTIVE_KEY) if args.print_latest: - print(read_latest_policyengine_version(environment=args.environment) or "") + _report(read_latest_policyengine_version(environment=args.environment) or "") return if args.print_identity: for key, value in latest_release_identity(active_state()).items(): - print(f"{key}={value}") + _report(f"{key}={value}") return if args.print_pending_plan: store = _routing_store(args.environment, create_if_missing=False) - print(json.dumps(_unfinished_publication(store), indent=2, sort_keys=True)) + _report(json.dumps(_unfinished_publication(store), indent=2, sort_keys=True)) return writes_routing = ( @@ -1277,10 +1277,10 @@ def active_state() -> object: store = _routing_store(args.environment, create_if_missing=False) pending = _unfinished_publication(store) if pending is None: - print("No pending routing publication; routing is unchanged.") + _report("No pending routing publication; routing is unchanged.") else: - print("Pending routing publication:") - print( + _report("Pending routing publication:") + _report( json.dumps(store.get(ROUTING_PUBLICATION_KEY), indent=2, sort_keys=True) ) outcome = finalize_routing_publication( @@ -1290,10 +1290,10 @@ def active_state() -> object: publication_run=args.publication_run, ) if outcome == "foreign-publication": - print( + _report( "Pending routing publication belongs to another run or environment; routing is unchanged." ) - print(f"Routing publication finalization: {outcome}") + _report(f"Routing publication finalization: {outcome}") if args.github_output: with open(args.github_output, "a") as handle: handle.write(f"outcome={outcome}\n") @@ -1304,7 +1304,7 @@ def active_state() -> object: plan = json.load(handle) operation = record_routing_plan if args.record_plan else complete_routing_plan operation(environment=args.environment, plan=plan) - print( + _report( f"Routing publication {plan['publication_id']}: " f"{'pending' if args.record_plan else 'completed'}" ) @@ -1317,7 +1317,7 @@ def active_state() -> object: environment=args.environment, plan=plan, ) - print( + _report( f"Published policyengine {plan['policyengine_version']} routing " f"({plan['after_sha256']}) in Modal environment {args.environment}" ) @@ -1332,7 +1332,7 @@ def active_state() -> object: environment=args.environment, plans=plans, ) - print( + _report( f"Routing for policyengine {plans[-1]['policyengine_version']}: " f"{outcome} in Modal environment {args.environment}" ) @@ -1344,10 +1344,10 @@ def active_state() -> object: policyengine_version=args.restore_latest, expected_latest=args.expected_latest, ) - print(f"Restored routing state in Modal environment: {args.environment}") - print(f" latest.policyengine: {state['latest']['policyengine']}") - print(f" latest.us: {state['latest']['us']}") - print(f" latest.uk: {state['latest']['uk']}") + _report(f"Restored routing state in Modal environment: {args.environment}") + _report(f" latest.policyengine: {state['latest']['policyengine']}") + _report(f" latest.us: {state['latest']['us']}") + _report(f" latest.uk: {state['latest']['uk']}") return if args.expected_latest: parser.error("--expected-latest needs --restore-latest") @@ -1391,9 +1391,9 @@ def active_state() -> object: if args.plan_out: with open(args.plan_out, "w") as handle: json.dump(plan, handle, indent=2, sort_keys=True) - print(f"before_sha256={plan['before_sha256']}") - print(f"after_sha256={plan['after_sha256']}") - print(f"advances_latest={'true' if plan['advances_latest'] else 'false'}") + _report(f"before_sha256={plan['before_sha256']}") + _report(f"after_sha256={plan['after_sha256']}") + _report(f"advances_latest={'true' if plan['advances_latest'] else 'false'}") return _report(f"Publishing routing state in Modal environment: {args.environment}") diff --git a/projects/policyengine-simulation-executor/tests/test_policyengine_package_update_scripts.py b/projects/policyengine-simulation-executor/tests/test_policyengine_package_update_scripts.py index 21971edfc..835239f39 100644 --- a/projects/policyengine-simulation-executor/tests/test_policyengine_package_update_scripts.py +++ b/projects/policyengine-simulation-executor/tests/test_policyengine_package_update_scripts.py @@ -403,15 +403,30 @@ def test_update_policyengine_package_rejects_direct_component_requirements( ) +def _run_helper( + command: str, + *, + check: bool = False, + input_text: str | None = None, + env: dict[str, str] | None = None, +) -> subprocess.CompletedProcess[str]: + """Run one command of the repository's Python update helper.""" + return subprocess.run( # noqa: S603 # runs the repository helper script with this interpreter + [sys.executable, str(HELPER), command], + input=input_text, + capture_output=True, + text=True, + check=check, + env=env, + ) + + def _verify(payload: str) -> subprocess.CompletedProcess[str]: import os - return subprocess.run( - [sys.executable, str(HELPER), "verify-dispatch"], - input=INSTALLED, - capture_output=True, - text=True, - check=False, + return _run_helper( + "verify-dispatch", + input_text=INSTALLED, env={**os.environ, "RELEASE_PAYLOAD": payload}, ) @@ -476,12 +491,9 @@ def test_verify_dispatch_compares_the_certificate_when_installed_reports_one() - ) def verify(payload: str) -> subprocess.CompletedProcess[str]: - return subprocess.run( - [sys.executable, str(HELPER), "verify-dispatch"], - input=installed, - capture_output=True, - text=True, - check=False, + return _run_helper( + "verify-dispatch", + input_text=installed, env={**os.environ, "RELEASE_PAYLOAD": payload}, ) @@ -496,12 +508,9 @@ def test_verify_dispatch_fails_closed_when_the_installed_bundle_cannot_answer() import os installed = INSTALLED.replace("uk_dataset_sha256=" + "a" * 64, "uk_dataset_sha256=") - result = subprocess.run( - [sys.executable, str(HELPER), "verify-dispatch"], - input=installed, - capture_output=True, - text=True, - check=False, + result = _run_helper( + "verify-dispatch", + input_text=installed, env={**os.environ, "RELEASE_PAYLOAD": _payload()}, ) assert result.returncode != 0 @@ -589,10 +598,8 @@ def test_bundle_versions_matches_policyengine_release_identity() -> None: bundle = get_current_bundle() record = bundle["data_releases"]["uk"] - output = subprocess.run( - [sys.executable, str(HELPER), "bundle-versions"], - capture_output=True, - text=True, + output = _run_helper( + "bundle-versions", check=True, ).stdout installed = dict(line.split("=", 1) for line in output.splitlines() if "=" in line) @@ -608,12 +615,9 @@ def test_bundle_versions_matches_policyengine_release_identity() -> None: } }, } - result = subprocess.run( - [sys.executable, str(HELPER), "verify-dispatch"], - input=output, - capture_output=True, - text=True, - check=False, + result = _run_helper( + "verify-dispatch", + input_text=output, env={**os.environ, "RELEASE_PAYLOAD": json.dumps(payload)}, ) assert result.returncode == 0, result.stderr @@ -641,10 +645,8 @@ def test_bundle_versions_reports_the_receipt_certificate_digest(tmp_path) -> Non "sys.modules['policyengine.provenance.receipt'] = module\n" ) env = {**os.environ, "PYTHONPATH": str(stand_in)} - output = subprocess.run( - [sys.executable, str(HELPER), "bundle-versions"], - capture_output=True, - text=True, + output = _run_helper( + "bundle-versions", check=True, env=env, ).stdout @@ -657,12 +659,9 @@ def test_bundle_versions_reports_the_receipt_certificate_digest(tmp_path) -> Non record = get_current_bundle()["data_releases"]["uk"] expected = hashlib.sha256(json.dumps(record, sort_keys=True).encode()).hexdigest() assert installed["uk_certificate_sha256"] == expected - mismatch = subprocess.run( - [sys.executable, str(HELPER), "verify-dispatch"], - input=output, - capture_output=True, - text=True, - check=False, + mismatch = _run_helper( + "verify-dispatch", + input_text=output, env={ **os.environ, "RELEASE_PAYLOAD": json.dumps( diff --git a/projects/policyengine-simulation-executor/tests/test_release_sentinel.py b/projects/policyengine-simulation-executor/tests/test_release_sentinel.py index 3d0c125a5..bf52aa4da 100644 --- a/projects/policyengine-simulation-executor/tests/test_release_sentinel.py +++ b/projects/policyengine-simulation-executor/tests/test_release_sentinel.py @@ -187,7 +187,7 @@ def _check(api, **kwargs): problems = sentinel.check_deployment( "https://candidate.example/", expected, - token="t0ken", + token="t0ken", # noqa: S106 # fake bearer token; the HTTP calls are stubbed http=api, sleep=sleeps.append, **kwargs,