Skip to content

ci: test latest upstream dependencies periodically - #1230

Closed
floze-the-genius wants to merge 9 commits into
anam-org:mainfrom
floze-the-genius:ci/upstream-compatibility-1229
Closed

ci: test latest upstream dependencies periodically#1230
floze-the-genius wants to merge 9 commits into
anam-org:mainfrom
floze-the-genius:ci/upstream-compatibility-1229

Conversation

@floze-the-genius

@floze-the-genius floze-the-genius commented Jul 19, 2026

Copy link
Copy Markdown

Summary

  • add weekly and manually dispatchable upstream compatibility checks
  • test newest allowed dependency versions across supported Python, Linux, Windows, macOS, and dependency matrices
  • reuse the existing QA workflow without duplicate pull-request CI
  • isolate service credentials from dependency resolution and ordinary integration tests
  • upload the resolved uv.lock diff as a run artifact
  • create or update a labeled scheduled incident with failed job names, then comment and close it after recovery

Resolves #1229.

Security

  • latest-dependency callers and test jobs use read-only GitHub permissions
  • integration checkout does not persist GitHub credentials
  • latest-dependency jobs receive no Codecov token or unrelated credentials
  • ordinary integration tests run without the MotherDuck token
  • the MotherDuck token is injected only into a dedicated live-test step after dependency setup
  • the live-test step uses uv run --no-sync, preventing dependency resolution while the secret is present
  • issue-write and actions-read permissions remain isolated to the scheduled reporting job
  • manual dispatches never mutate scheduled incident state

Test Plan

  • actionlint on the reusable QA, pull-request, and upstream compatibility workflows, excluding only the repository custom-runner label warning
  • YAML parsing for all three changed workflows
  • GitHub Script syntax compilation
  • mocked incident lifecycle scenarios: first failure, repeated failure, recovery, and success without an open incident
  • uv lock --upgrade --dry-run resolved 349 packages and reported the pending updates
  • uv run --frozen --no-sync pytest -p no:pytest_postgresql tests/ext/duckdb/test_motherduck.py -q without service credentials (3 skipped)
  • git diff --check
  • no end-to-end scheduled run is claimed before the workflow is merged

AI Assistance

Codex assisted with implementation and validation. I reviewed the workflow behavior, maintainer feedback, identity, rendered description, and final diff.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 12469f2323

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread .github/workflows/upstream-compatibility.yml Outdated
Comment thread .github/workflows/_qa.yml Outdated
Comment thread .github/workflows/upstream-compatibility.yml Outdated
Comment thread .github/workflows/upstream-compatibility.yml Outdated
Comment thread .github/workflows/upstream-compatibility.yml

@geoHeil geoHeil left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please set the changelog entry in the PR correctly

further @floze-the-genius please comunicate like a human if even the communicaiton for humans is AI/slop it does not neccessarily speed up a review process.

Comment thread .github/actions/setup-devenv/action.yml Outdated
@codecov

codecov Bot commented Jul 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@geoHeil

geoHeil commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator

Thanks @floze-the-genius for picking up #1229 — reusing the existing _qa.yml matrix instead of building a parallel pipeline is the right instinct, and the EXTRAS_ARGS string→array conversion is a genuinely nice drive-by fix (correct quoting-safe idiom, and the new UPGRADE_ARGS follows it). All six setup-devenv call sites in _qa.yml are threaded correctly, and uv sync --upgrade has exactly the right semantics for "do the latest allowed versions still work". The direction is good. But there is one blocking bug and a set of design problems, all verified against the actual runs and sources.

Blocking: the workflow is dead on arrival

The Upstream compatibility run triggered by this very PR failed with startup_failure after 1 second, zero jobs, and had already been retried once when I looked (attempt 2 — it fails deterministically): https://github.com/anam-org/metaxy/actions/runs/29669012865

Root cause, by elimination and consistent with the standard GitHub error signature: the new workflow grants only permissions: contents: read, but _qa.yml's test-core job requests pull-requests/issues/checks: write. A called reusable workflow cannot elevate the caller's token permissions, and GitHub rejects the entire run at startup ("is requesting 'X: write', but is only allowed 'X: none'"). Both existing callers grant these writes (main.yml at job level, Pull Request.yml at workflow level); this caller is the only one that doesn't. The error banner on the run page should confirm the exact message.

This kills every trigger — pull_request, schedule, and workflow_dispatch alike. Merged as-is, the weekly early-warning run would never execute. Worse: startup failures create no check runs (gh pr checks doesn't even list it), so nothing goes red. A workflow built to prevent silent absence of signal ships with exactly that failure mode.

Fix: copy the permissions: block from main.yml's qa job onto this one. That also gives paths-filter the pull-requests: read it needs on PR events (Codex's P2 comment).

And the uncomfortable part: the test plan lists actionlint, YAML parsing, bash -n, and uv --dry-run — all syntax-level checks — while the one end-to-end signal that existed, the run on this PR, was red the whole time. For a CI change, looking at the triggered run has to be part of the validation story.

Design issues

1. The pull_request trigger doubles CI and will red-X unrelated PRs. Its paths fully overlap _qa.yml's own test filter (.github/workflows/**), so any PR touching _qa.yml runs the normal full QA plus this entire matrix with run-all-checks + dependency-matrix — roughly double cost per push. There is no concurrency group (contrast Pull Request.yml), so rapid pushes stack uncancelled full-matrix runs. And once an upstream genuinely breaks, every CI-touching PR goes red for reasons outside its author's control — inverting the early-warning intent. Suggest: validate via workflow_dispatch only (or a single canary call without run-all-checks/dependency-matrix), and add a concurrency group with cancel-in-progress for PR events.

2. The mechanism works by side effect, not by design. The nox-based jobs (integrations, compat) run tests in venvs that nox creates after setup-devenv--upgrade on the outer .venv never touches those envs. They pick up latest versions only because uv sync --upgrade rewrites uv.lock in the workspace, and nox_uv (uv_sync_locked=False) then syncs the session venv from the mutated lock. I verified this works today (nox_uv 0.7.1 emits uv sync with neither --locked nor --frozen; confirmed uv sync --upgrade --dry-run reports "Would update lockfile"). But it's an undocumented mutation two layers away from the flag — in the same action that carefully restores devenv.lock "to prevent lock file modifications". Any future hygiene change (--locked, uv_sync_locked=True, or restoring uv.lock likewise) silently turns the weekly run into a green locked-deps no-op — the exact false-green this workflow exists to prevent. Minimum: a loud comment in action.yml. Better: make lock regeneration an explicit first-class step.

3. Coverage gaps. The weekly run is Linux + Python 3.10 only. test-windows/test-macos bypass setup-devenv (raw uv sync), so the flag structurally cannot reach them even if enabled — and the most recent real upstream breakage (#1224) was macOS-specific. Upstream releases also tend to break the newest Python first, and this caller doesn't set python-versions. Codex flagged both points; I agree with both.

4. Nobody will see it fail. Failed scheduled runs notify only the user who last modified the cron syntax, filtered through their personal notification settings. For an early-warning system, add an if: failure() job on the schedule event that opens/updates an issue (needs issues: write — folds into the fix above), matching how upstream breakage is already tracked here (#952, #1218).

Minor: secrets: inherit over-grants relative to the explicit two-secret pattern both existing callers use (Codex's P1 — agree). Codecov uploads from latest-deps runs merge indistinguishably into the same Python3.10 flag as locked-deps coverage (Codecov merges all uploads per commit). Each job re-resolves --upgrade independently, so two jobs in one run can test different versions of the same package, and a red run leaves no record of which resolution broke — consider one uv lock --upgrade job that uploads the lock as an artifact. Under upgrade mode the ibis(12) compat session becomes redundant with the main run (the upgraded lock already resolves latest ibis 12.x).

A simpler design worth considering

What this PR actually implements — via the side effect in point 2 — is "regenerate uv.lock at latest allowed versions, then run normal CI", performed invisibly and independently in ~18 jobs. Doing it once, explicitly, is simpler and strictly more capable:

A small scheduled workflow (or Renovate lockFileMaintenance — the repo has neither renovate nor dependabot config today) runs uv lock --upgrade and opens/refreshes a bot PR labeled tests:full. Then:

  • Pull Request.yml already runs the full matrix on that label — all 5 Pythons, Windows and macOS (they sync from uv.lock, closing gap 3), plus the dependency matrix — with zero changes to _qa.yml or setup-devenv.
  • The lockfile diff is a per-package record of exactly what was tested (fixes the forensics problem).
  • A red PR is an unmissable early warning (fixes gap 4); a green PR is a mergeable auto-upgrade — the "autofix" idea [ci] refine CI upstream testing #1229 muses about, for free.

Honest cost: PRs created with the default GITHUB_TOKEN don't trigger CI, so this needs a fine-grained PAT or GitHub App token (Renovate avoids that entirely by triggering CI natively). If managing a token is unacceptable, the current approach with fixes 1–4 is workable.

On the AI-assisted parts

The disclosure that Codex helped is appreciated — that's the right thing to do. But several parts shipped without the human pass that disclosure implies:

  • The PR body degenerates into literal \n escape sequences from the second Test Plan bullet onward — pasted from a tool's escaped string without checking the rendered result.
  • "YAML parsing with Ruby Psych" as validation in a Python/uv/Nix repo with no Ruby toolchain is a fingerprint of the assistant's sandbox, not meaningful validation.
  • The commit is authored as air <air@MacBook-Air-Air.local> — an unconfigured git identity GitHub can't associate with your account, and no Co-Authored-By trailer despite the disclosed assistance.
  • Per the blocking section: the stated validation was syntax-only while the actual end-to-end run sat red and unnoticed.

To be fair: the code itself shows none of this — the bash is correct, the wiring is complete, nothing is cargo-culted. The slop is confined to exactly the parts a human was supposed to check. Please fix the body rendering, amend the commit identity, and restate what was actually run.


Summary: right goal, right building blocks, one startup-killing permissions bug, and a core mechanism that works by accident. My preference would be the lockfile-PR design — it deletes most of this diff and covers more — but if you'd rather land this approach, the fixes in points 1–4 plus the permissions block make it mergeable. Thanks again for tackling #1229!

@floze-the-genius
floze-the-genius force-pushed the ci/upstream-compatibility-1229 branch 2 times, most recently from 6d6819d to f5876a6 Compare July 19, 2026 04:02
@floze-the-genius

Copy link
Copy Markdown
Author

Thanks for the detailed review. I reworked this around your points 1-4: removed the duplicate PR trigger, copied the required QA permissions, enabled the full Python/Windows/macOS matrix, documented the lockfile side effect, narrowed secret forwarding, and added a scheduled-failure issue reporter. I also fixed the commit identity and rewrote the rendered description with only the checks actually run. The latest commit is f5876a69.

@floze-the-genius
floze-the-genius force-pushed the ci/upstream-compatibility-1229 branch from f5876a6 to 48366f1 Compare July 20, 2026 11:22
@floze-the-genius

floze-the-genius commented Jul 20, 2026

Copy link
Copy Markdown
Author

Made the caller workflow’s pull-requests: read permission explicit, so dorny/paths-filter can inspect PR metadata while the reusable QA job retains only its declared job-level permissions. The changed workflows pass actionlint after suppressing only the expected unknown-label diagnostic for the repository’s existing custom depot-ubuntu-latest runner, and git diff --check passes. A real workflow_dispatch run cannot be started from this PR branch because GitHub only registers manually dispatched workflows once the workflow file exists on the fork’s default branch; the PR checks are rerunning on the new head.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 48366f1b10

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread .github/workflows/upstream-compatibility.yml
@floze-the-genius
floze-the-genius force-pushed the ci/upstream-compatibility-1229 branch from 48366f1 to 0142863 Compare July 20, 2026 12:35

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0142863946

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread .github/workflows/upstream-compatibility.yml
@floze-the-genius
floze-the-genius force-pushed the ci/upstream-compatibility-1229 branch from 146f9de to abe09fc Compare July 21, 2026 07:46
@floze-the-genius

Copy link
Copy Markdown
Author

@geoHeil I found one thing I had previously claimed fixed but hadn't: the first commit still had the local air@... author. That's actually corrected now on all three commits (head abe09fc). The old review threads are addressed and resolved, and the changelog section remains omitted because this is CI-only. Could you take another look when you have time?

@floze-the-genius
floze-the-genius requested a review from geoHeil July 21, 2026 07:56
@danielgafni danielgafni added the tests:latest-deps Run tests against the latest version of dependencies label Jul 21, 2026
@danielgafni

Copy link
Copy Markdown
Collaborator

I added a new label tests:latest-deps to this PR, can we update the standard workflow to spawn an additional test matrix against the latest deps in this case? I'd like to keep both standard tests and also have latest deps jobs additionally

@geoHeil

geoHeil commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

@geoHeil I found one thing I had previously claimed fixed but hadn't: the first commit still had the local air@... author. That's actually corrected now on all three commits (head abe09fc). The old review threads are addressed and resolved, and the changelog section remains omitted because this is CI-only. Could you take another look when you have time?

changelog empty is fine but it should be chore(ci): <<title>>

@danielgafni

Copy link
Copy Markdown
Collaborator

I think the current title is ok

Copy link
Copy Markdown
Author

Implemented the labeled dual-matrix flow in dfbee2f:

  • the existing qa job remains unchanged and continues to test the locked dependencies;
  • tests:latest-deps now adds a separate qa-latest-deps reusable-workflow job with test-latest-dependencies: true;
  • if tests:full is also present, both jobs expand to the same full Python/Windows/macOS/dependency matrix.

git diff --check passes, and actionlint reports no new workflow errors (only the repository's existing unknown-label diagnostic for depot-ubuntu-latest). The new fork run is registered but awaiting maintainer approval: https://github.com/anam-org/metaxy/actions/runs/29860032335

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dfbee2f7da

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread .github/workflows/_qa.yml
@floze-the-genius

floze-the-genius commented Jul 22, 2026

Copy link
Copy Markdown
Author

The latest-deps matrix exposed a few real upstream compatibility breaks, and I pushed fixes in 9534bf2:

I verified the changes in a clean upgraded lock resolution (pytest 9.0.3, pytest-cases 3.10.1, polars 1.42.1, sqlmodel 0.0.39). ty check, the two affected SQLModel tests, uv lock --check, and git diff --check all pass locally.

The new fork workflow run is awaiting maintainer approval rather than failing: https://github.com/anam-org/metaxy/actions/runs/29963293026

@geoHeil

geoHeil commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Thanks for following up. The Exclusions are not the intended end state here. a) we want to be notified and b) eventually get the compatibility fixed - not just excluded.

@floze-the-genius, please attempt durable fixes for the Polars 1.43 and pytest 9.1 / pytest-cases failures rather than only locking them out.

If a failure is conclusively upstream-only and cannot be worked around safely, a narrow temporary exclusion may be necessary, but it should be the last resort and must include a tracked upstream issue plus an explicit removal condition.

@geoHeil

geoHeil commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Further:

  • The generated incident has no lifecycle: report-failure only creates or comments on an open issue. It never closes that issue after recovery, matches by title within only the first 100 open issues, and records only the run URL. Once the first failure occurs, this becomes a permanently stale incident with weekly comments rather than actionable monitoring.

@floze-the-genius, please make the notification stateful:

  • give generated incidents a dedicated label and locate them by that label using pagination or the search API;
  • include the failed job names and a reproducible dependency manifest / upgraded lock artifact in the report; (diff only)
  • on the next successful scheduled run, comment with the recovery run and close the open incident;
  • keep manual dispatches from mutating scheduled-incident state.

A single always-running reporting job that inspects needs.qa.result can handle both failure and recovery.

@geoHeil

geoHeil commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

And one more thing: We have to fix the secrets handling.
For testing purposes, we do need to retain connections to the data sources i.e. Motherduck and in the future Bigquery. An other secrets or permission should be redacted from the dependency jobs.

@floze-the-genius, please structure it so that:

  • all ordinary latest-dependency integration jobs run only with the MOTHERDUCK_TOKEN or other service secrets;
  • that job has read-only GitHub permissions and uses actions/checkout with persist-credentials: false;
  • dependency resolution and installation happen before the secret is introduced;
  • MOTHERDUCK_TOKEN is injected only into the live-test step, not at reusable-workflow or matrix-job scope;
  • CODECOV_TOKEN and unrelated credentials are not exposed to this job.
  • comments write also not, that is only reserved for a reporting job

Signed-off-by: Floze <88098863+floze-the-genius@users.noreply.github.com>
@floze-the-genius
floze-the-genius force-pushed the ci/upstream-compatibility-1229 branch from 9534bf2 to 8978fac Compare July 23, 2026 07:10
@floze-the-genius

floze-the-genius commented Jul 23, 2026

Copy link
Copy Markdown
Author

Addressed the secrets-handling feedback in 2116499.

  • both latest-dependency callers now run with read-only GitHub permissions;
  • the reusable core and integration test jobs are also explicitly read-only;
  • issue-write permission exists only on the scheduled failure-reporting job;
  • CODECOV_TOKEN is no longer passed to latest-dependency runs, and their Codecov upload steps are disabled;
  • the integration checkout uses persist-credentials: false;
  • ordinary integration tests run without MOTHERDUCK_TOKEN, so the live tests skip there;
  • a dedicated DuckDB live-test step receives only MOTHERDUCK_TOKEN and MOTHERDUCK_REGION, after checkout and dependency setup;
  • that live step uses uv run --no-sync, so dependency resolution or installation cannot occur while the token is present.

Validation: actionlint passes (excluding the repository's known custom-runner label warning), all changed workflows parse as YAML, the no-token MotherDuck run reports 3 skipped, and git diff --check passes.

@floze-the-genius
floze-the-genius force-pushed the ci/upstream-compatibility-1229 branch 2 times, most recently from b04e3a4 to 2116499 Compare July 23, 2026 07:21
Signed-off-by: Floze <88098863+floze-the-genius@users.noreply.github.com>
@floze-the-genius

Copy link
Copy Markdown
Author

Addressed the scheduled-incident lifecycle feedback in e0cd67c2.

  • a dedicated dependency-manifest job uploads the upgraded uv.lock diff for each run;
  • the scheduled reporter now always inspects both QA and manifest results, lists failed jobs, and links the artifact;
  • incidents use a dedicated upstream-compatibility label and paginated lookup;
  • repeated failures comment on the open incident, while the next successful scheduled run comments with the recovery run and closes it;
  • manual dispatches do not mutate incident state;
  • only the reporting job has issues: write and actions: read.

Validation: actionlint passes apart from the repository custom-runner label warning, all changed workflows parse as YAML, the GitHub Script compiles, four mocked lifecycle scenarios pass, uv lock --upgrade --dry-run resolves 349 packages, and git diff --check passes.

This commit addresses the monitoring lifecycle request only; I am not claiming the separate Polars 1.43 / pytest 9.1 compatibility concerns are resolved by it.

Signed-off-by: Floze <88098863+floze-the-genius@users.noreply.github.com>
@floze-the-genius

Copy link
Copy Markdown
Author

Addressed the durable latest-dependency compatibility request in aab24fd7.

Validation on that exact dependency set: full ty check, repository-wide Ruff and format checks, 165 Delta tests, 163 Iceberg tests, 368 core/versioning tests, and 43 SQLModel tests all pass. I also added focused coverage for the version-specific Polars optimizer behavior and preservation of explicit options.

I am not claiming the GitHub matrix is green until the new fork workflow is approved and runs.

@danielgafni

Copy link
Copy Markdown
Collaborator

Alright these is too much going on here. Given CI/CD is a particularly sensitive part of the codebase I'd rather implement this myself later. I have plans for redoing CI/CD in Dagger anyway. @floze-the-genius thank you for your work but it's better to pick a more trivial issue if you still want to help

@floze-the-genius

Copy link
Copy Markdown
Author

Understood, thanks for the direction. I won't reopen this, and I'll keep any future contributions here smaller and outside CI/CD.

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

Labels

tests:latest-deps Run tests against the latest version of dependencies

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ci] refine CI upstream testing

3 participants