[fzowl]

Fixes #69 and closes #68 — CI-workflow-only changes.

#69 — fork PRs can never pass required checks

GitHub does not pass repository secrets to pull_request workflows from a fork, so integration-tests (needs VOYAGE_API_KEY) is always red on fork PRs. The legacy required-check bridge tests depended on it, so the required Tests (3.x) contexts were red on every fork PR — permanently BLOCKED.

• tests bridge needs only fork-safe, keyless jobs → required Tests (3.x) can go green on forks.
• integration-tests runs everywhere (no skip); red on fork PRs by design, gated by review (external: merge + revert if the post-merge main run fails; same-repo: don't merge on red). Not a required check (confirmed in the merge UI).

Keyless required coverage

The keyless mock suite (test_unit.py, test_import_order.py) previously ran only inside the non-required integration-tests, so a same-repo PR breaking test_unit.py would pass every required check. Now covered by the required feeder unit-tests-no-local-deps (no API key). pytest -m "not integration" was rejected — it is not keyless (the live-API client/multimodal tests carry no integration marker; 147 fail without a key).

test_video_utils.py is deliberately NOT in the required jobs: its e2e leg calls _get_client_config() → hf_hub_download() with no offline fallback, so it needs the HuggingFace Hub and would make a required check flaky on HF outages. It runs in integration-tests.

Matrix isolation

needs is job-level, so a failing 3.13/3.14 leg of a bridge feeder would fail the whole job and skip the bridge → required contexts unreported → blocked. Bridge feeders are the required set 3.9-3.12; 3.13/3.14 run in separate advisory jobs (*-next): red on failure (visible) but not bridge dependencies, so they never block merges (rerun turns them green).

Dedup

The four test jobs shared an identical install/run block. Extracted into a local composite action (.github/actions/setup-and-test: python-version/extras/pytest-args/install-openblas). Four separate jobs and the needs topology kept intact (no matrix.include+continue-on-error collapse, which would re-couple non-required versions to the required gate).

#68 — needless max-parallel: 1 on unit-tests-no-local-deps

Zero API calls (no key, no local extras) → the TPM rationale copied from integration-tests doesn't apply. Removed; serialization stays only on integration-tests.

Admin follow-up

None. The Tests bridge keeps emitting the legacy required contexts; Integration Tests (3.x) is not required. The bridge can be retired later if protection is switched to require the real job names.

Related: #67 (the fork PR that surfaced this).

[jasonqiu95]

Automated review of the CI changes. Three points on the required-check bridge; all are on the workflow semantics, none block the fork-safe intent conceptually.

One extra note (not inline, pre-existing): local-model-tests is a required bridge dependency but is not hermetic — it downloads voyage-4-nano from HuggingFace with no -m filter, so a Hub outage/anonymous-throttle can red a required check on fork PRs. Same job-level cascade as the matrix note below.

[jasonqiu95]

Re-reviewed after c9f78a7. All three prior points are addressed correctly:

• Coverage gap — fixed. And your pushback is right: contextualized_embed calls _require_api_key() before validation (client.py:129), and AuthenticationError is a VoyageError, not a ValueError — so test_client.py's validation tests would raise AuthenticationError without a key and can't be keyless. No residual gap there; the genuinely-keyless files are the right set.
• Matrix/bridge decoupling — fixed. Feeders and bridge are both 3.9–3.12; 3.13/3.14 moved to non-required *-next jobs. Correct.
• Stale #68 ref — fixed.

Two new points that the fix commit itself introduced (one is a real flakiness risk on a required check):

[fzowl]

Self-review hardening (pre-empting likely comments):

• HF flakiness in the required gate. local-model-tests loads the model via SentenceTransformerBackend(...), which downloads from the HF Hub — same outage exposure as the test_video_utils e2e that was pulled out, but here the fixture deliberately lets download errors surface (documented) rather than skip-masking bugs. Rather than weaken that, added an actions/cache of ~/.cache/huggingface on the model-loading jobs; huggingface_hub falls back to a populated cache when the Hub is unreachable, so routine HF flake no longer reds the required check.
• Action input handling. The composite now passes extras/pytest-args via env: instead of interpolating ${{ inputs.* }} into run: (script-injection pattern actionlint flags).
• CI cost. Added a concurrency group so superseded PR runs cancel; not cancelled on main so each merged commit is still validated post-merge.

Consciously deferred (happy to take if wanted): single-source POETRY_HOME (composite hardcodes /opt/poetry vs the workflow env); coverage --fail-under only runs in the non-required integration-tests so it isn't enforced on fork PRs; pytest-args duplicated across the required/-next pairs.

[jasonqiu95]

Re-reviewed after 43b9d90 + 6d57695. Both round-2 findings are resolved, and I checked the new surface for regressions:

1. HF network in a required check — resolved (and verified deeper than the fix comment claims). test_video_utils.py is out of the fork-safe jobs. Note that test_unit.py's TestVideoUtilsIntegration stays in the required job and still calls optimize_video / _compute_basic_usage_for_path / Video.from_file with model="voyage-multimodal-3.5" unmocked — so it's not "mocked" as the reply says. But it's genuinely safe: _get_video_token_config swallows HF errors → None (video_utils.py:308-312), both optimize_video and _compute_basic_usage_for_path have video_config is None fallbacks, and no test in that class asserts on the config-derived estimated_tokens — they only assert ffmpeg-probe values (num_pixels, num_frames, .optimized). So an HF outage degrades gracefully instead of reding the required check. Confirmed resolved.

2. Job duplication — resolved. The composite action (.github/actions/setup-and-test) collapses the four jobs to ~5 lines each, keeps the needs/job topology intact, and passes inputs via env: (not ${{ }} interpolation) — good injection hygiene. POETRY_HOME is hardcoded in the action (correct — composite actions don't inherit workflow env) and the only workflow-level $POETRY_HOME consumer is the coverage step, which does inherit it. No mismatch.

New additions checked, all correct:

• concurrency: github.ref is refs/pull/<N>/merge (unique per PR), and cancel-in-progress is gated to pull_request so the post-merge main integration gate is never cancelled.
• HF actions/cache: fixed key across parallel legs → save races are non-fatal warnings; warm cache lets huggingface_hub fall back on an outage, a strict improvement over the prior always-live behavior. (Residual: a cold cache + HF outage can still red the required local-model-tests feeder, but that's pre-existing, not introduced here.)
• PYTEST_ARGS word-splitting is safe for all three arg sets (no spaces/globs); SC2086 disable is appropriate. Checkout precedes the local action in every job; all actions SHA-pinned.

No new correctness issues. LGTM from my side.