kubeflow / kubeflow/sdk

Bugs in OSV-Scanner workflow and helper scripts, found while porting to kubeflow/mcp-server

Open
#748 2 comments 0 reactions 1 assignee Claimed by @Kartikeya-trivedi View on GitHub
Dominant language
Python
Stars
148
Forks
262
Avg merge
1d 2h
Merged PRs (30d)
1

Description

### What happened?

I recently ported this repo's `.github/workflows/osv-scanner.yaml` and its helper scripts into [kubeflow/mcp-server](https://github.com/kubeflow/mcp-server) (kubeflow/mcp-server#30, merged and now running nightly there — its auto-fix pipeline has already landed a real security bump, kubeflow/mcp-server#137). The review rounds on that port surfaced several bugs that all still exist in the SDK's original copy on `main`:

**1. `update_overrides.py` writes invalid TOML when `pyproject.toml` has no pre-existing `[tool.uv]` section**
The creation path appends a `[tool.uv]` + header, and the insertion logic below adds a second header + `override-dependencies` block — producing duplicate keys that `uv lock` rejects (`Cannot overwrite a value`). Repro:

```bash
printf '[project]\nname = "x"\nversion = "0.1.0"\n' > pyproject.toml
python3 .github/scripts/update_overrides.py requests "requests==2.31.0" "2026-01-01" "https://example.com/adv"
python3 -c "import tomllib; tomllib.load(open('pyproject.toml','rb'))" # -> TOMLDecodeError
```

**2. `update_overrides.py` misses an indented `override-dependencies` key (valid TOML), producing a duplicate key**
The removal regexes are anchored at column 0 (`r"^override-dependencies..."`, `r"^# Security overrides..."`), so an indented key survives removal and the rewritten block is appended alongside it → invalid TOML. Fix is `^[ \t]*` (not `^\s*`, which also eats newlines).
Note: #573 touches the same pattern (trailing-comment tolerance) but keeps the `^` anchor, so both changes compose cleanly.

**3. `labels: "area/security"` in the auto-fix PR step creates a label with literal quotes**
`peter-evans/create-pull-request` splits the `labels` input on `[\n,]+` and does **not** strip quote characters, so the YAML literal block's `"area/security"` becomes a malformed label named `"area/security"` (quotes included).

**4. `continue-on-error: true` on the SARIF scan step swallows real scanner crashes**
The step's own error handling re-exits 1 on unexpected codes (e.g. exit 2 = scanner crash), but `continue-on-error` neutralizes it — a genuine crash leaves the job green. Replacing it with `if: always()` on the following JSON-scan step keeps the intended "SARIF failure doesn't block auto-fix" behavior while making crashes fail the job.

**5. Auto-fix PR body renders a broken table**
The PR body declares a two-column markdown table header, but `fix_details` lines are emitted as `- pkg | [id](url)` bullets — bullets don't render as table rows. Emitting `| pkg | [id](url) |` rows fixes the rendering.

**6. (minor) `compare_versions.py` depends on `packaging` being transitively present**
Under `uv run` a missing `packaging` import exits 1, which the fixer loop misreads as "natural upgrade sufficient" — silently skipping a needed override. `uv run --with packaging` makes the dependency explicit. Related: `echo "$TREE_OUTPUT"` is safer as `printf '%s\n' "$TREE_OUTPUT"` for multi-line tree output.

**7. The scanner pin is stale: `2.3.8`, current release is `v2.5.0`**
The pin + `EXPECTED_SHA` pattern is right, but nothing alerts when it goes stale — the file comment says "update version + checksum manually" and that hasn't happened across two releases. In mcp-server this is being addressed with a weekly auto-bump workflow (kubeflow/mcp-server#90) that pulls the new checksum from the release's official `SHA256SUMS`, cross-verifies it against the downloaded binary, and opens a PR for review — happy to contribute that here as a follow-up once it merges there.

### What did you expect to happen?

- `update_overrides.py` always produces valid TOML (both fresh and indented-key cases)
- A scanner crash (exit >= 2) fails the workflow visibly
- The auto-fix PR gets a correctly named `area/security` label and a properly rendered changelog table
- The scanner pin either stays current or something alerts when it doesn't

All of the above are fixed in kubeflow/mcp-server (merge commit kubeflow/mcp-server@e89648a, plus regression tests for the two TOML cases in its `test_scripts.py`). I'm happy to send the mirror PR with the fixes + tests — filing this first for visibility and to coordinate with #573, which touches the same regex in `update_overrides.py`.

### Environment

N/A — CI workflow and repo scripts (`.github/workflows/osv-scanner.yaml`, `.github/scripts/update_overrides.py`), not the SDK runtime.

Kubernetes version: N/A
Kubeflow Trainer version: N/A
Kubeflow Python SDK version: N/A

### Impacted by this bug?

Give it a 👍 We prioritize the issues with most 👍

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.