Skip to content

Fix ai-gate cardgen cache inputs#5469

Merged
matthewevans merged 2 commits into
phase-rs:mainfrom
mike-theDude:ci/issue-5456-ai-gate-cache
Jul 10, 2026
Merged

Fix ai-gate cardgen cache inputs#5469
matthewevans merged 2 commits into
phase-rs:mainfrom
mike-theDude:ci/issue-5456-ai-gate-cache

Conversation

@mike-theDude

Copy link
Copy Markdown
Collaborator

Summary

Fixes the ai-gate cardgen cache ordering for issue #5456.

Closes #5456.

Files changed

  • .github/actions/ai-card-data-cache/action.yml
  • .github/workflows/ai-gate.yml
  • .github/workflows/ci.yml

CR references

None.

Track

Developer

LLM

Model: codex-5
Thinking: medium
Tier: Standard

Anchored on

  • .github/workflows/ci.yml:153 — existing weekly mtgjson-atomic-* cache key setup before generated card-data cache evaluation.
  • .github/workflows/ci.yml:174 — existing file-existence-gated MTGJSON download that self-heals missing or partial cache restores.
  • .github/workflows/ci.yml:188 — existing generated card-data cache key that hashes AtomicCards.json, engine source, engine embedded data, build script, and lockfile.
  • .github/actions/create-auto-merge-pr/action.yml:24 — existing local composite action structure using runs: using: composite.

Gate A

./scripts/check-parser-combinators.sh — clean, exit 0, no output.

Verification

  • cargo fmt --all -- --check — clean
  • ./scripts/check-parser-combinators.sh — clean
  • /tmp/actionlint-bin/actionlint .github/workflows/ai-gate.yml .github/workflows/ci.yml — clean
  • python3 -c 'import yaml, sys; [yaml.safe_load(open(f)) for f in sys.argv[1:]]; print("ok")' .github/workflows/ai-gate.yml .github/workflows/ci.yml .github/actions/ai-card-data-cache/action.yml — clean
  • Composite action metadata shape check — clean
  • Personal doc-guardian — clean
  • Personal code-reviewer — approved
  • Personal qa-tester — READY FOR PR

Scope Expansion

None.

Validation Failures

None.

CI Failures

None.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new GitHub composite action, AI card data cache, designed to cache and restore MTGJSON and generated card data for AI gate jobs. The feedback suggests optimizing the MTGJSON download step by replacing the shell-level file existence check with a step-level if conditional that checks the cache-hit output of the preceding cache step, which is more idiomatic and efficient.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +24 to +31
- name: Download MTGJSON data
shell: bash
run: |
if [ ! -f data/mtgjson/AtomicCards.json ]; then
mkdir -p data/mtgjson
source scripts/lib/mtgjson-fetch.sh
mtgjson_download AtomicCards.json data/mtgjson/AtomicCards.json
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

This step uses a shell if to check for the existence of the cached file. A more idiomatic approach in GitHub Actions is to use a step-level if conditional with the cache-hit output from the preceding cache step. This makes the workflow's control flow more explicit and avoids executing the shell: bash step entirely when the cache is hit, which is slightly more efficient. This pattern is also flagged by actionlint as shell-if.

    - name: Download MTGJSON data
      if: steps.mtgjson-cache.outputs.cache-hit != 'true'
      shell: bash
      run: |
        mkdir -p data/mtgjson
        source scripts/lib/mtgjson-fetch.sh
        mtgjson_download AtomicCards.json data/mtgjson/AtomicCards.json

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Not applying this suggestion.

This step intentionally gates on file existence rather than steps.mtgjson-cache.outputs.cache-hit, matching .github/workflows/ci.yml. The CI workflow documents the failure mode we are preserving against: cache restore can report a hit for a poisoned/partial entry while data/mtgjson/AtomicCards.json is absent, and a cache-hit step condition would then skip the download and break oracle-gen. The shell step is cheap on a warm cache and self-heals missing-file restores.

@matthewevans

Copy link
Copy Markdown
Member

Thanks for this — the direction is right, and the cache key correctly picks up crates/engine/data/** + crates/engine/build.rs (the same fingerprint family that #5461 just landed in engine-source-hash.sh).

However this PR is a hard stop for the automated contributor-review path: every file it touches is on the repo's hard-stop list.

  • .github/workflows/ai-gate.yml
  • .github/workflows/ci.yml
  • .github/actions/ai-card-data-cache/action.yml (new composite action under .github/)

Changes to CI workflows — and especially a new composite action that populates the cardgen- cache the AI gate then consumes — can weaken the very gates that review incoming PRs, so they can't be approved or enqueued through the normal contributor flow. This is a process constraint, not a judgment on the code.

This needs direct maintainer handling. Two specific things a maintainer will need to check before it can land:

  1. Cache-writer ownership. The gate that consumes cardgen-* should remain the sole writer of that key. Hoisting the restore into a composite action shared across four jobs makes it easier for a job that skips regeneration to publish a partially-populated cache entry.
  2. Overlap with fix(ci): hash crates/engine/data and build.rs in engine-source-hash.sh #5461. Confirm the key here does not diverge from scripts/engine-source-hash.sh now that both hash the embedded engine data. Two independent definitions of the same fingerprint will drift.

Leaving this open for a maintainer to pick up directly. Not enqueuing.

@mike-theDude mike-theDude added ai-contribution PR opened via docs/AI-CONTRIBUTOR.md flow needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps) labels Jul 10, 2026
@matthewevans matthewevans self-assigned this Jul 10, 2026
@matthewevans

Copy link
Copy Markdown
Member

Follow-up to my earlier comment — I've now done the full review, and both concerns I raised there are resolved. Correcting the record:

1. Cache-writer ownership — not a problem, by a design that predates this PR.
I worried that hoisting the restore into a shared action would let ai-gate (which never runs card-data-validate) publish a cardgen- entry that suppresses validation. It can't. ci.yml gates the validate/coverage skip on a separate cardgen-gates-v1- key, and actions/cache only saves on job success. Worst case traced: ai-gate generates output that would fail validate → ai-gate succeeds and saves cardgen- → the next card-data-gate restores it, skips generation, misses cardgen-gates-v1-, and runs validate → red. No false green. The existing ci.yml comment anticipated exactly this scenario, and your rewrite of it preserves the reasoning correctly.

2. Key drift vs engine-source-hash.sh — my error.
That script is the Tilt/CI engine-source fingerprint, unrelated to these cache keys. Your composite action's expression matches ci.yml's cardgen-cache character for character. Disregard that concern.

Verification that the fix actually works. Green CI alone couldn't prove this: if hashFiles had returned empty inside the composite action, the key would degenerate to a constant cardgen- and still pass every check while being silently broken. So I read the raw job logs from this PR's own run:

  • ai-gate (job 86260181281): key: mtgjson-atomic-2026-W28 → hit → then key: cardgen-ade68e79558858bd…
  • ci.yml card-data (job 86260181470): key: cardgen-ade68e79558858bd…

Same digest. MTGJSON restores before the key is evaluated, hashFiles sees the gitignored AtomicCards.json, and the two workflows share a real entry for the first time. That is the issue #5456 fix, demonstrated end to end.

Also confirmed: both workflows run the byte-identical oracle-gen command over the same cached paths (deterministic output, so a shared entry can't diverge); the download stays existence-gated rather than cache-hit-gated, so a poisoned cache still self-heals; and no new third-party actions, secrets, pull_request_target, or permissions changes are introduced.

One non-blocking follow-up. The cardgen- key expression now lives in three places that must stay byte-identical or sharing silently degrades into two disjoint namespaces: the composite action, ci.yml's cardgen-cache, and ci.yml's gates-cache. That's down from four, so this is a net improvement — but ci.yml is still unconverted. Converting it is more invasive (its cardgen-cache step id is referenced downstream, and gates-cache needs the same expression), so I'm taking this as-is. Worth consolidating next time someone touches that file.

Approving and enqueuing. Note for the record: this PR touches .github/**, which is a hard-stop path for the automated contributor-review path — it was reviewed and authorized directly by the maintainer, not merged through that path.

Nice diagnosis on the hashFiles-omits-gitignored-paths behavior. That's a genuinely subtle failure mode: it fails silently, produces a plausible-looking key, and passes CI indefinitely.

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approved. Fixes a real, silent CI defect (#5456), verified end-to-end against the run logs: ai-gate and ci.yml now evaluate the identical cardgen-ade68e79… digest, so cross-workflow cache sharing genuinely works and the key invalidates on the weekly MTGJSON refresh. Validate/coverage remain gated on the separate cardgen-gates-v1- key, so a shared cardgen entry cannot license a false green. Details in the comment above.

@matthewevans matthewevans added the bug Bug fix label Jul 10, 2026
@matthewevans
matthewevans added this pull request to the merge queue Jul 10, 2026
@matthewevans matthewevans removed the needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps) label Jul 10, 2026
@matthewevans matthewevans removed their assignment Jul 10, 2026
Merged via the queue into phase-rs:main with commit 14ef07f Jul 10, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-contribution PR opened via docs/AI-CONTRIBUTOR.md flow bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ai-gate.yml cardgen cache key never sees AtomicCards.json (restores before download): stale card-data + phantom sharing with ci.yml

3 participants