Skip to content

ci(python): add advisory ty type checking with SARIF reporting - #3752

Draft
mcosgriff wants to merge 2 commits into
mainfrom
add-python-type-checking-astral-ty
Draft

ci(python): add advisory ty type checking with SARIF reporting#3752
mcosgriff wants to merge 2 commits into
mainfrom
add-python-type-checking-astral-ty

Conversation

@mcosgriff

Copy link
Copy Markdown
Contributor

Why

The Python library has no static type checking today — only ruff, which catches style and a narrow band of correctness issues. Real type errors reach runtime, and in a command-and-control system that means they surface during an operation rather than in CI.

The obvious approach — turn ty on and gate PRs on it — doesn't work here. A bare ty check reports 816 diagnostics on the current tree, so a blocking check would fail every PR on day one and get disabled within a week. This PR is the groundwork that makes ty usable at all:

  • Advisory first, blocking later. The check runs with --exit-zero so it reports without gating. That gets ty in front of reviewers on real PRs while the backlog is worked down, instead of holding the whole thing until the tree is clean. Flipping it to blocking is deleting one flag.
  • 816 diagnostics are unreadable; the reporting has to be good or nobody looks. Step output is ephemeral and 816 lines deep. SARIF puts findings in the Security tab where they're filterable by rule and dismissible, and — critically — annotated on the diff lines a reviewer is already reading.
  • 18% of the noise was one false pattern. 149 of the 816 were Logger.info("msg") reported as a bad self argument, because the metaclass forwarding is invisible to type checkers. Leaving that in place means the real findings drown in it, and the first person to read the report concludes ty doesn't understand the codebase and stops reading.

Summary

  • Adds .github/workflows/python_type_check.yml alongside python_lint.yml, running astral-sh/ty over openc3/python/openc3 and openc3-cosmos-script-runner-api/scripts, advisory via --exit-zero.
  • Publishes SARIF to Security → Code scanning, plus a rule-count job summary and the raw SARIF as an artifact.
  • Adds tools/ty_report.py to convert ty's GitLab Code Quality JSON into SARIF 2.1.0. Why a converter: ty can't emit SARIF (full, concise, gitlab, github, junit), and its github annotation format emits absolute paths, so GitHub silently fails to attach annotations to files. The converter rewrites every path repo-root-relative, which is what GitHub matches against. It also carries ty's fingerprint into partialFingerprints so a finding keeps its identity across runs — otherwise every run re-reports everything as new and dismissals don't stick.
  • Adds tools/generate_singleton_stubs.py and the five .pyi files it generates, taking diagnostics 816 → 675. Why generated rather than hand-written: the first version of logger.pyi was hand-written, which means it silently rots the moment someone adds a method to Logger. just stubs-check runs in CI to catch that drift. Why not just stubgen: run bare it emits def info(self, ...), faithfully reproducing the bug, so an AST pass promotes the metaclass-forwarded methods to classmethods.
  • Hardens every uv invocation with --no-build (plus --no-install-project on the sync). Why: building a source distribution executes that package's build backend, i.e. arbitrary third-party code in CI. No locked dependency is sdist-only, so the flag costs nothing today and fails loudly if a future dependency bump introduces one. --no-install-project is what makes --no-build usable at all — the editable openc3 install was the only thing requiring a build, and ty resolves first-party imports from the source tree without it.
  • Adds a ty dev dependency — why a dependency rather than uvx ty: it pins in uv.lock for reproducible runs and lets dependabot bump it like ruff. Plus just recipes: typecheck, typecheck-strict, typecheck-report, stubs, stubs-check, stubs-diff.

Deliberately out of scope: fixing the remaining 675. Roughly half trace to two declaration patterns rather than 675 distinct problems — singleton class attributes declared instance = None and never reset (91 in running_script.py alone, all downstream of one untyped declaration), and instance attributes assigned None then immediately reassigned. Those are source changes with real review cost and belong in follow-ups; this PR establishes the check and the reporting so those follow-ups have a baseline to measure against.

Worth noting the check already found at least one genuine bug it wasn't looking for: running_script.py:1136 guards on not RunningScript.instance, which is always false since instance is set in init and never reset — so that output-thread cleanup never runs.

Test plan

  • just stubs-check passes on a clean checkout (the one blocking step in the workflow)
  • just stubs is idempotent — regenerating twice produces no diff
  • just typecheck reports 675 diagnostics
  • just typecheck-report writes ty.sarif and ty.md; SARIF validates against the 2.1.0 schema and contains no absolute paths
  • uv run --frozen pytest passes (2820 tests) — stubs must not affect runtime
  • uv run --frozen ruff check openc3 and ruff format --check openc3 pass, including the new .pyi files
  • On the PR, confirm results land in Security → Code scanning under category ty, with annotations pointing at the right files and lines
  • Confirm the job summary renders the rule table and the collapsible file table
  • Confirm the ty steps do not fail the build when diagnostics exist
  • Verify the workflow's paths filter triggers on .py/.pyi/pyproject.toml/uv.lock/tools/** changes and skips unrelated PRs

Note: upload-sarif needs a write token, so the code-scanning upload is expected to fail on fork PRs. It's marked continue-on-error: true; the job summary and SARIF artifact still work there.

- Add python_type_check.yml alongside python_lint, running ty with
  --exit-zero so it annotates but never fails the build yet
- Add tools/ty_report.py to convert ty's GitLab JSON into SARIF for
  code scanning, since ty cannot emit SARIF and its paths need
  rewriting to be repo-root relative
- Add tools/generate_singleton_stubs.py and the five .pyi it emits, so
  metaclass-forwarded calls like Logger.info("msg") check correctly;
  drops diagnostics from 816 to 675
- Pass --no-build and --no-install-project to every uv step, so no
  build backend executes in CI
- Add ty dev dependency plus just stubs and typecheck recipes

Co-Authored-By: Claude noreply@anthropic.com
@mcosgriff mcosgriff self-assigned this Aug 20, 2026
@socket-security

socket-security Bot commented Aug 20, 2026

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedpypi/​ty@​0.0.73100100100100100

View full report

@github-advanced-security

Copy link
Copy Markdown
Contributor

You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool.

What Enabling Code Scanning Means:

  • The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
  • Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
  • You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.04%. Comparing base (dbff03d) to head (1478f93).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3752      +/-   ##
==========================================
+ Coverage   80.01%   80.04%   +0.03%     
==========================================
  Files         885      885              
  Lines       65382    65382              
  Branches     2543     2543              
==========================================
+ Hits        52313    52336      +23     
+ Misses      12407    12387      -20     
+ Partials      662      659       -3     
Flag Coverage Δ
frontend 66.14% <ø> (+0.06%) ⬆️
python 81.87% <ø> (+<0.01%) ⬆️
ruby-api 82.45% <ø> (+0.27%) ⬆️
ruby-backend 84.43% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

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

- Lock the PEP 723 dependencies of both tools scripts and pass --locked
  everywhere, so a new mypy or ruff release cannot silently change what
  runs or make stubs-check report spurious drift
- Validate --input, --sarif and --markdown in ty_report.py before
  touching the file system, confining them to the repo, cwd, temp dir
  and the paths the runner declares via GITHUB_STEP_SUMMARY and friends
- Reject traversing arguments, symlinks, directories and missing
  parents with a clear message and exit 1 instead of writing

Co-Authored-By: Claude noreply@anthropic.com
@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
MAJOR Vulnerabilities Severity on New Code (required < MINOR)

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants