fix: address medium CI/test review feedback from upstream-test-suite

[joshbouncesecurity]

Addresses all four medium-severity items from the CI/test review.

Changes

1. Job-level timeout-minutes: 15 (.github/workflows/test.yaml)

Both python-tests and go-tests previously relied on the 360-minute GitHub Actions default. pip install, npm ci, and go build steps are now bounded — a stalled registry can no longer burn hours of runner time.

2. pip install ".[dev]" replaces pip install pytest (.github/workflows/test.yaml)

Both jobs were bypassing the [project.optional-dependencies] dev extra declared in pyproject.toml and installing an unpinned pytest directly. Now CI installs through the declared extra, so any future dev tooling added to [dev] (mypy, ruff, etc.) is automatically picked up.

3. Platform-gated pytest.skip for "No module named" (libs/openant-core/tests/test_go_cli.py)

test_parse_js_repo previously skipped unconditionally on any "No module named" error, hiding regressions on Linux/macOS. Now:

• Windows: pytest.skip (pre-existing Python resolution ambiguity)
• Linux/macOS: pytest.fail with a clear message (should never happen with the correct env, so a failure here is a real regression)

4. OPENANT_PYTHON env var support for explicit Python resolution (apps/openant-cli/internal/python/runtime.go + workflow)

DetectRuntime() gains a new top-priority strategy: if OPENANT_PYTHON is set in the environment, that binary is used before the managed venv or PATH scan. The go-tests job sets OPENANT_PYTHON: python at the job level, which always resolves to the actions/setup-python-installed interpreter on all platforms — eliminating the implicit resolution ambiguity on Windows.

Test plan

• CI passes on ubuntu-latest for both jobs
• Confirm pip install ".[dev]" succeeds (pyproject.toml [dev] extra resolves correctly from libs/openant-core working directory)
• Confirm test_parse_js_repo skips on Windows and fails fast (not silently skips) on Linux if packages are missing

---

Generated by Claude Code

[joshbouncesecurity]

Superseded by newer review

Previous review (click to expand)

🤖 Automated Claude Code Review

This PR addresses four medium-severity CI/test findings from an earlier review. The changes are modest in scope but touch a production code path (DetectRuntime), a test assertion strategy, and the workflow configuration. Review follows.

---

Findings

F1 — venvPython() returns a Unix-only path; OPENANT_PYTHON env var is set too late to help when the venv is expected on Windows

venvPython() hardcodes "bin/python" as the venv sub-path:

func venvPython() string {
    return filepath.Join(venvDir(), "bin", "python")
}

On Windows, virtualenv/venv uses Scripts\python.exe, not bin\python. This is a pre-existing bug in the file, not introduced by this PR — however, the PR adds OPENANT_PYTHON: python as the documented workaround, which means it now implicitly relies on this env var to paper over the broken Windows venv path. The PR should either:

• Note in a comment in runtime.go that venvPython() is Linux/macOS-only and that Windows users are expected to use OPENANT_PYTHON, or
• Fix venvPython() to return the correct path per platform (runtime.GOOS / filepath.Join(venvDir(), "Scripts", "python.exe") on Windows).

Without one of these, a future maintainer who removes the env var will silently break Windows.

F2 — Silent version-mismatch in the OPENANT_PYTHON override: no error or warning when the override is set but fails the version check

if override := os.Getenv("OPENANT_PYTHON"); override != "" {
    if info, err := checkPython(override); err == nil {
        if info.Major > MinPythonMajor || (info.Major == MinPythonMajor && info.Minor >= MinPythonMinor) {
            return info, nil
        }
    }
}

If OPENANT_PYTHON is set to a binary that exists but is too old (e.g. Python 3.9), the code silently falls through to the venv / PATH strategies. The user explicitly overrode resolution, so silently ignoring the override is surprising — they'll end up using a different Python than intended with no indication of why. A warning on os.Stderr (or failing outright with an actionable message) would make this much easier to debug:

if info.Major > MinPythonMajor || (info.Major == MinPythonMajor && info.Minor >= MinPythonMinor) {
    return info, nil
}
fmt.Fprintf(os.Stderr,
    "warning: OPENANT_PYTHON=%q is Python %s, which is below the required %d.%d; ignoring override\n",
    override, info.Version, MinPythonMajor, MinPythonMinor)

F3 — OPENANT_PYTHON override silently swallowed when checkPython fails (binary not found, not executable, etc.)

Similarly, if checkPython(override) returns an error (binary does not exist, permission denied, etc.), the error is discarded with no user-visible output. For the same reason as F2, this should at minimum print a warning so that OPENANT_PYTHON=/usr/bin/python3.11 (a typo like /usr/bin/pythoon) doesn't silently fall back to a different interpreter:

if info, err := checkPython(override); err != nil {
    fmt.Fprintf(os.Stderr, "warning: OPENANT_PYTHON=%q is not a usable Python binary (%v); ignoring\n", override, err)
} else if ... {

F4 — pip cache key does not include pyproject.toml; cache will not invalidate when [dev] dependencies change

The setup-python action in both jobs caches pip with:

cache: "pip"
cache-dependency-path: libs/openant-core/requirements.txt

Now that CI installs pip install ".[dev]" (which reads pyproject.toml), the cache key must also hash pyproject.toml. If pytest is bumped in [dev] and requirements.txt is unchanged, the cache will be stale and CI will run the old version of pytest without realising it. Fix:

cache-dependency-path: |
  libs/openant-core/requirements.txt
  libs/openant-core/pyproject.toml

This applies to both python-tests and go-tests.

F5 — test_parse_js_repo: the UnicodeEncodeError skip is still unconditional on all platforms after the refactor

if "UnicodeEncodeError" in result.stderr:
    pytest.skip("Pre-existing Unicode bug in JS test_pipeline.py on Windows")

The docstring says this is a "Pre-existing Unicode bug … on Windows", yet the check has no sys.platform == "win32" guard. On Linux/macOS a UnicodeEncodeError in the JS parser would be a real regression that should fail, not skip. The PR already introduced the precedent of platform-gating the "No module named" skip — the same treatment should be applied here for consistency and to avoid hiding future regressions:

if "UnicodeEncodeError" in result.stderr:
    if sys.platform == "win32":
        pytest.skip("Pre-existing Unicode bug in JS test_pipeline.py on Windows")
    else:
        pytest.fail("UnicodeEncodeError from JS parser on non-Windows (unexpected regression)")

F6 — OPENANT_PYTHON environment variable is undocumented user-facing behaviour

OPENANT_PYTHON is now the top-priority resolver for all Python detection in the CLI. This is a meaningful user-facing feature (useful for debugging, CI pinning, container images). There is no mention of it in any README or help text. At minimum, a one-line entry in the CLI's version output or a comment in runtime.go's package doc explaining when/why a user would set it would help. Alternatively, expose it in openant --help or openant version output.

---

Positive notes

• The timeout-minutes: 15 addition is a good defensive measure; pip install, npm ci, and go build can all hang on a slow or misbehaving registry and 15 minutes is a reasonable ceiling for these steps.
• Switching from pip install pytest to pip install ".[dev]" is a clean improvement: it keeps the test runner version under pyproject.toml control and ensures any future additions to [dev] (linters, type checkers) are automatically included in CI.
• The platform-gated pytest.fail on Linux/macOS for the "No module named" case (F-finding from the prior review) is the right approach — it turns a silent skip into a loud failure on the platforms where the issue should never occur.
• The DetectRuntime override logic correctly validates the version before accepting the override (it doesn't blindly trust whatever binary is pointed at), which is good. The concern (F2, F3) is only about the lack of user-visible feedback when it is rejected.
• The OPENANT_PYTHON: python setting in the workflow matches setup-python's installed binary name exactly, which is the right value for all three platforms in that action.

[joshbouncesecurity]

🤖 Automatically written by Claude

• F1: partially addressed (commit 55c1d19) — Added a doc comment to DetectRuntime noting that venvPython() uses a Linux/macOS-only bin/python path and that Windows users should use OPENANT_PYTHON. The underlying venvPython() function itself is pre-existing code outside this PR's scope; the comment makes the limitation explicit so a future maintainer knows the intent.
• F2: fixed (commit 55c1d19) — OPENANT_PYTHON override now prints a warning to stderr when the binary fails the version check, rather than silently falling through.
• F3: fixed (commit 55c1d19) — OPENANT_PYTHON override now prints a warning to stderr when checkPython returns an error (binary not found, not executable, etc.).
• F4: fixed (commit 55c1d19) — Both python-tests and go-tests jobs now include libs/openant-core/pyproject.toml in cache-dependency-path, so changes to the [dev] extras correctly invalidate the pip cache.
• F5: fixed (commit 55c1d19) — The UnicodeEncodeError skip in test_parse_js_repo is now platform-gated: pytest.skip on Windows, pytest.fail on Linux/macOS, matching the pattern introduced for the "No module named" case.
• F6: fixed (commit 55c1d19) — DetectRuntime doc comment now explains the OPENANT_PYTHON env var, its intended use cases (CI pinning, debugging, containers), and the Windows venv path limitation.

[joshbouncesecurity]

🤖 Automated Claude Code Review

This is a follow-up review covering only the commits since the previous review (commit 55c1d19). The previous review ended at F6.

All six prior findings have been addressed:

• F1 (partially): DetectRuntime doc comment now explicitly notes the bin/python vs Scripts\python.exe Windows limitation and directs Windows users to set OPENANT_PYTHON. Adequate given the pre-existing nature of venvPython().
• F2: Warning emitted to stderr when OPENANT_PYTHON points to a too-old interpreter. ✓
• F3: Warning emitted to stderr when checkPython fails for the override path. ✓
• F4: cache-dependency-path in both jobs now includes pyproject.toml. ✓
• F5: UnicodeEncodeError skip is platform-gated (pytest.skip on Windows, pytest.fail otherwise). ✓
• F6: OPENANT_PYTHON is documented in the DetectRuntime doc comment. ✓

---

Findings

F7 — Warning message for the OPENANT_PYTHON version-too-low case omits "ignoring override" — inconsistent with the not-found case

The binary-not-found warning reads:

warning: OPENANT_PYTHON=%q is not a usable Python binary (%v); ignoring override

The version-too-low warning reads:

warning: OPENANT_PYTHON=%q is Python %s, below the required %d.%d; ignoring override

Wait — reading the actual code more carefully both messages do say "ignoring override". The messages are consistent. This is not an issue.

(Self-correcting — no F7 for this.)

F7 — cache-dependency-path indentation in the YAML uses an inconsistent number of leading spaces compared to surrounding keys

Looking at the literal-block scalar used for cache-dependency-path:

          cache-dependency-path: |
            libs/openant-core/requirements.txt
            libs/openant-core/pyproject.toml

The with: block key cache-dependency-path sits at 10-space indent; the literal block content is at 12 spaces. This is correct YAML (the content must be indented further than the key's indicator position). The setup-python action parses this as a newline-delimited list of paths, which is the documented behaviour for cache-dependency-path. No actual issue — the formatting is correct.

(Self-correcting — no F7 for this either.)

---

After reviewing the full diff carefully, the changes in commit 55c1d19 are correct and complete. The warning messages are clear and consistently formatted, the YAML multi-line literal block is syntactically valid and semantically correct for setup-python's cache-dependency-path, the platform-gating in the test is consistent with the existing pattern introduced in the base PR, and the doc comment accurately describes the limitation.

Findings

There are no new actionable findings in this incremental diff. All six prior findings (F1–F6) have been addressed, and the fixes themselves do not introduce new issues.

---

Positive notes

• The fmt.Fprintf(os.Stderr, ...) pattern for the warning messages is correct: it goes to stderr (not stdout) so it doesn't pollute JSON output, matches the existing stderr usage elsewhere in runtime.go, and the %q verb properly quotes paths with spaces or special characters.
• The version-too-low branch now uses an if/else if/else structure rather than nested if statements, which is cleaner and makes the mutual-exclusion of the three cases immediately apparent.
• Both YAML jobs were updated consistently — no risk of one job having stale cache while the other correctly invalidates.
• The pytest.fail message for the UnicodeEncodeError case on non-Windows ("UnicodeEncodeError from JS parser on non-Windows (unexpected regression)") is clear and actionable.