Validate card-script API names and sub-ability refs against the engine

Add a pure-advisory test (CardScriptApiTest) that checks each card against
the engine's own definitions and writes findings for the review workflow
to post, never failing the build:

  - ability/trigger/replacement API names via ApiType, TriggerType and
    ReplacementType (the same smartValueOf the engine runs at card load),
    catching a typo'd API such as DB$ DealDamge that the linter cannot;
  - sub-ability params in AbilityFactory.additionalAbilityKeys must point
    at a defined SVar, closing a gap where the linter only knew a handful
    of these keys. cardlint's REF_KEYS drops the now-engine-checked keys.

The analysis runs inside the build (mvn test). A pull_request build has a
read-only token on fork PRs and so cannot comment; the review workflow is
therefore reworked from pull_request_target into a workflow_run poster.
test-build uploads the findings and the PR number as an artifact, and the
poster downloads it, runs the linter and Scryfall frame check, and posts
the merged, deduped comments on the changed lines.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: MostCromulent

.github/scripts/card_script_review.py

@@ -17,6 +17,11 @@
      When the card is not on Scryfall (e.g. an unreleased card) it stays silent;
      see scryfall_facts().
 
+  3. Engine API findings (optional, --java-findings) — ability/trigger/replacement
+     API names validated by the build against the engine's own enums, the same
+     check the engine runs at card load. This is the one engine-coupled input, and
+     only to the stable API vocabulary; it is produced by the build, not here.
+
 This file never interprets ability semantics. It only (a) relays the linter's
 structured findings and (b) diffs frame fields — both decoupled from how any
 individual card is scripted, which is what keeps it stable across engine changes.
@@ -118,11 +123,15 @@ def grab(pat):
     elif code == "LEX-PIPE":
         return "add a space around the `|` separator"
     elif code == "LEX-CURLY":
-        return "curly apostrophe `’` " + ARROW + " ASCII `'`"
+        return f"curly apostrophe `’` {ARROW} ASCII `'`"
     elif code == "MANA":
         t = grab(r"token '([^']+)'")
         if t:
             return f"illegal mana token `{t}`"
+    elif code == "API-UNKNOWN":
+        m = re.match(r"(\w+\$) '([^']+)' is not a known (.+)", msg)
+        if m:
+            return f"`{m.group(1)} {m.group(2)}` {ARROW} unknown {m.group(3)}"
     # default: the linter message is already human-readable
     return msg
 
@@ -288,31 +297,57 @@ def scryfall_facts(path):
 # --------------------------------------------------------------------------- #
 # main
 # --------------------------------------------------------------------------- #
+def java_findings_comments(path, changed):
+    """Load engine-validated API findings emitted by the build, scoped to changed cards.
+
+    The build's Java test validates every card's ability/trigger/replacement API
+    names against the engine's own enums and writes {path, line, code, body}. We
+    only keep findings on cards this PR changed; the workflow further scopes them
+    to the diff lines before posting.
+    """
+    try:
+        items = json.load(open(path, encoding="utf-8"))
+    except Exception as e:
+        print(f"no java findings ({e})", file=sys.stderr)
+        return []
+    out = []
+    for f in items:
+        p = f.get("path", "").replace("\\", "/")
+        if p in changed:
+            out.append({"path": p, "line": f["line"],
+                        "body": terse_comment(f.get("code", ""), f.get("body", ""))})
+    return out
+
+
 def main():
-    if len(sys.argv) < 2:
-        print("usage: card_script_review.py <changed_files.txt>", file=sys.stderr)
+    args = sys.argv[1:]
+    java_path = None
+    if "--java-findings" in args:
+        i = args.index("--java-findings")
+        java_path = args[i + 1]
+        del args[i:i + 2]
+    if not args:
+        print("usage: card_script_review.py [--java-findings <file>] <changed_files.txt>",
+              file=sys.stderr)
         return 2
-    paths = [l.strip() for l in open(sys.argv[1], encoding="utf-8") if l.strip()]
-    cards = [p for p in paths if p.endswith(".txt") and "cardsfolder" in p.replace("\\", "/")]
+    paths = [l.strip() for l in open(args[0], encoding="utf-8") if l.strip()]
+    cards = [p for p in paths if p.endswith(".txt")
+             and "cardsfolder" in p.replace("\\", "/") and os.path.exists(p)]
 
     # The workflow materializes the PR's cards into the corpus, so a freshly
     # computed key_freq counts them too. Subtract each reviewed card's own param
     # counts so a typo'd or made-up param present only in the reviewed card isn't
     # self-counted as "known" (which would silence KEY-TYPO / UNKNOWN-KEY).
     freq = dict(cardlint.key_freq(cardlint.find_corpus()) or {})
     for path in cards:
-        if not os.path.exists(path):
-            continue
         text = open(path, encoding="utf-8", errors="ignore").read()
-        for k, n in collections.Counter(re.findall(r"([A-Za-z][A-Za-z0-9]*)\$", text)).items():
+        for k, n in collections.Counter(cardlint.KEY_TOKEN.findall(text)).items():
             if k in freq:
                 freq[k] -= n
                 if freq[k] <= 0:
                     del freq[k]
     comments = []
     for path in cards:
-        if not os.path.exists(path):                   # deleted in the PR
-            continue
         found = []
         try:
             found += lint_comments(path, freq)
@@ -325,6 +360,9 @@ def main():
         for line, body in found:
             comments.append({"path": path.replace("\\", "/"), "line": line, "body": body})
 
+    if java_path:
+        comments += java_findings_comments(java_path, set(c.replace("\\", "/") for c in cards))
+
     json.dump(comments, sys.stdout, ensure_ascii=False, indent=2)
     print(f"\n{len(comments)} comment(s) across {len(cards)} card(s)", file=sys.stderr)
     return 0

.github/scripts/cardlint.py

@@ -14,14 +14,17 @@
         forge-gui/res/cardsfolder near cwd; skipped if not found).
 Exit:   1 if any findings, else 0.
 """
-import re, sys, os, json, tempfile, hashlib
+import re, sys, os, json, time, tempfile, hashlib
+
+KEY_TOKEN = re.compile(r"([A-Za-z][A-Za-z0-9]*)\$")  # a `key$` param token
 
 LINE_PREFIXES = {"Name","ManaCost","Types","PT","Loyalty","Defense","Colors","Text",
     "Oracle","K","A","T","S","R","SVar","AI","DeckHints","DeckNeeds","DeckHas",
     "AlternateMode","Variant","ALTERNATE","SetColor"}
 PFX_LOWER = {p.lower():p for p in LINE_PREFIXES}
-REF_KEYS = {"Execute","SubAbility","TrueSubAbility","FalseSubAbility","AbilityX",
-            "RepeatSubAbility","ChosenSubAbility"}
+# The sub-ability keys in the engine's additionalAbilityKeys are validated against
+# defined SVars by the build's Java test; only the keys outside that list stay here.
+REF_KEYS = {"Execute","SubAbility","AbilityX","ChosenSubAbility"}
 LIST_REF_KEYS = {"Choices"}
 AMP_LIST_KEYS = {"AddTypes","AddKeyword","AddKeywords","RemoveKeywords",
                  "AddTrigger","AddStatic","AddReplacement","Triggers"}
@@ -80,7 +83,7 @@ def key_freq(corpus):
     cache=os.path.join(tempfile.gettempdir(),
                        "cardlint_keyfreq_"+hashlib.md5(corpus.encode()).hexdigest()[:8]+".json")
     try:
-        if os.path.exists(cache) and (os.path.getmtime(cache) > __import__("time").time()-7*86400):
+        if os.path.exists(cache) and (os.path.getmtime(cache) > time.time()-7*86400):
             return json.load(open(cache,encoding="utf-8"))
     except Exception: pass
     freq={}
@@ -89,7 +92,7 @@ def key_freq(corpus):
             if not fn.endswith(".txt"): continue
             try: txt=open(os.path.join(root,fn),encoding="utf-8",errors="ignore").read()
             except Exception: continue
-            for k in re.findall(r"([A-Za-z][A-Za-z0-9]*)\$",txt):
+            for k in KEY_TOKEN.findall(txt):
                 freq[k]=freq.get(k,0)+1
     try: json.dump(freq,open(cache,"w",encoding="utf-8"))
     except Exception: pass

.github/workflows/card-script-review.yml

@@ -1,113 +1,134 @@
-# On a PR that touches card scripts, runs a deterministic linter + a Scryfall
-# frame fact-check and posts terse inline comments. It is purely ADVISORY: it
-# never fails the check, it only comments. A reviewer still decides what to act on.
+# Posts terse, advisory inline comments on PRs that touch card scripts: a
+# deterministic DSL linter, a Scryfall frame fact-check, and engine-validated API
+# names. It only comments; it never fails a check. A reviewer decides what to act on.
 #
-# Trigger: pull_request_target, because almost all PRs come from forks and a plain
-# pull_request token is read-only (can't comment). pull_request_target runs in the
-# base repo with a write token, so the hard rule is: NEVER execute PR code.
-# This workflow upholds that — it runs only the BASE repo's scripts (checked out
-# from the target branch) and pulls in just the PR's `cardsfolder/*.txt` files as
-# DATA to lint. The changed-file filter excludes `.github/**`, so a PR cannot swap
-# in its own linter, and nothing from the PR is ever built or executed.
+# Why this is a separate workflow from the build. The analysis runs inside the
+# build (the Card-script review test in `mvn test` + the linter here), but a
+# `pull_request` build has a READ-ONLY token on fork PRs — almost all card PRs —
+# so it cannot post comments. `workflow_run` runs after the build completes in the
+# base-repo context with a write token, which is GitHub's supported pattern for
+# "untrusted build produces findings, a privileged job posts them". The build
+# itself stays on the safe read-only token; only this poster can write.
 #
-# Stable across engine changes: the linter derives "known params" from the corpus
-# itself, and the Scryfall side only compares stable frame fields (name/type/P-T/
-# cost/loyalty). Neither is coupled to how a card ability is scripted.
+# Trust boundary: the build that produced the artifact ran PR code, so the
+# downloaded files are treated as DATA only (parsed, never executed). The linter
+# and corpus are the BASE repo's, checked out fresh here; the PR's card files are
+# pulled in as data to lint, never run.
 
 name: Card-script review
 
 on:
-  pull_request_target:
-    paths:
-      - 'forge-gui/res/cardsfolder/**'
+  workflow_run:
+    workflows: ["Test build"]
+    types: [completed]
 
 permissions:
   contents: read
+  actions: read
   pull-requests: write
 
 concurrency:
-  group: card-script-review-${{ github.event.pull_request.number }}
+  group: card-script-review-${{ github.event.workflow_run.head_branch }}
   cancel-in-progress: true
 
 jobs:
   review:
     runs-on: ubuntu-latest
+    # Only PR builds carry review data; push builds have nothing to comment on.
+    if: github.event.workflow_run.event == 'pull_request'
     steps:
-      # Trusted base checkout: the scripts we run and the card corpus the linter
-      # compares against. NOT the PR's code.
-      - name: Check out base repo
-        uses: actions/checkout@v4
-
-      - name: Set up Python
-        uses: actions/setup-python@v5
+      - name: Download review data from the build
+        uses: actions/download-artifact@v4
         with:
-          python-version: '3.11'
+          name: card-script-review
+          run-id: ${{ github.event.workflow_run.id }}
+          github-token: ${{ secrets.GITHUB_TOKEN }}
+          path: review-data
+        continue-on-error: true   # no artifact (non-card PR) -> nothing to do
 
-      - name: List changed card files
-        uses: actions/github-script@v7
+      - name: Resolve PR and changed cards
+        id: pr
+        uses: actions/github-script@v8
         with:
           script: |
             const fs = require('fs');
+            let meta;
+            try { meta = JSON.parse(fs.readFileSync('review-data/card-script-pr-meta.json', 'utf8')); }
+            catch (e) { core.info('no PR metadata — nothing to review'); return; }
+            const pull_number = meta.number;
             const files = await github.paginate(github.rest.pulls.listFiles, {
-              owner: context.repo.owner,
-              repo: context.repo.repo,
-              pull_number: context.issue.number,
-            });
-            // Only cardsfolder .txt files — this also guarantees we never pull a
-            // PR's version of .github/** (incl. the scripts we're about to run).
+              owner: context.repo.owner, repo: context.repo.repo, pull_number });
             const cards = files.filter(f => f.status !== 'removed'
                         && f.filename.startsWith('forge-gui/res/cardsfolder/')
                         && f.filename.endsWith('.txt'));
+            if (!cards.length) { core.info('no card files changed'); return; }
+            core.setOutput('pull_number', String(pull_number));
+            core.setOutput('head_sha', meta.head_sha);
             fs.writeFileSync('changed_files.txt', cards.map(f => f.filename).join('\n'));
 
-            // For each card, the set of NEW-file line numbers it actually adds or
-            // changes (the '+' lines of the diff). We only comment on these — a
-            // finding on a line the PR didn't touch is pre-existing and out of scope.
-            // For an added file the whole file is '+' lines, so nothing is lost.
+            // For each card, the NEW-file line numbers it adds or changes (the '+'
+            // lines). We only comment on these — a finding on an untouched line of an
+            // edited file is pre-existing and out of scope.
             const addedLines = {};
             for (const f of cards) {
-              const lines = [];
-              let newLine = 0;
+              const lines = []; let newLine = 0;
               for (const ln of (f.patch || '').split('\n')) {
                 const h = ln.match(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/);
                 if (h) { newLine = parseInt(h[1], 10); continue; }
                 if (ln.startsWith('\\')) continue;           // "\ No newline at end of file"
                 if (ln.startsWith('+')) { lines.push(newLine); newLine++; }
-                else if (ln.startsWith('-')) { /* old-side only: no new line */ }
+                else if (ln.startsWith('-')) { /* old-side only */ }
                 else { newLine++; }                          // context line
               }
               addedLines[f.filename] = lines;
             }
             fs.writeFileSync('diff_lines.json', JSON.stringify(addedLines));
             core.info(`${cards.length} changed card file(s)`);
 
-      # Bring in the PR's versions of those card files as plain data, overwriting
+      - name: Check out base scripts and corpus
+        if: steps.pr.outputs.pull_number != ''
+        uses: actions/checkout@v5
+
+      - name: Set up Python
+        if: steps.pr.outputs.pull_number != ''
+        uses: actions/setup-python@v6
+        with:
+          python-version: '3.11'
+
+      # Bring in the PR's versions of the changed cards as plain data, overwriting
       # the base copies in place. We only read them; we never run them.
       - name: Materialize PR card files (data only)
+        if: steps.pr.outputs.pull_number != ''
         env:
-          PR_NUMBER: ${{ github.event.pull_request.number }}
+          PR_NUMBER: ${{ steps.pr.outputs.pull_number }}
         run: |
           [ -s changed_files.txt ] || { echo "no card files changed"; exit 0; }
           git fetch --no-tags --depth=1 origin "+refs/pull/${PR_NUMBER}/head:refs/remotes/pr/head"
-          # `|| [ -n "$f" ]` processes the final line even when the file has no
-          # trailing newline (changed_files.txt is written with join('\n')).
+          # `|| [ -n "$f" ]` processes the final line even without a trailing newline.
           while IFS= read -r f || [ -n "$f" ]; do
             [ -n "$f" ] || continue
             mkdir -p "$(dirname "$f")"
             git show "refs/remotes/pr/head:$f" > "$f" || echo "skip $f"
           done < changed_files.txt
 
       - name: Run review checks
+        if: steps.pr.outputs.pull_number != ''
         continue-on-error: true   # advisory: a script hiccup must never fail the PR
         env:
-          PYTHONIOENCODING: utf-8   # the comments contain '→'; don't depend on runner locale
+          PYTHONIOENCODING: utf-8   # comments contain '→'; don't depend on runner locale
         run: |
-          python .github/scripts/card_script_review.py changed_files.txt > comments.json
+          JF=review-data/card-script-findings.json
+          ARGS=""; [ -f "$JF" ] && ARGS="--java-findings $JF"
+          python .github/scripts/card_script_review.py $ARGS changed_files.txt > comments.json
           echo "---- comments ----"
           cat comments.json
 
       - name: Post inline comments
-        uses: actions/github-script@v7
+        if: steps.pr.outputs.pull_number != ''
+        uses: actions/github-script@v8
+        env:
+          PULL_NUMBER: ${{ steps.pr.outputs.pull_number }}
+          HEAD_SHA: ${{ steps.pr.outputs.head_sha }}
         with:
           script: |
             const fs = require('fs');
@@ -119,12 +140,10 @@ jobs:
             if (!comments.length) { core.info('no findings'); return; }
 
             const { owner, repo } = context.repo;
-            const pull_number = context.issue.number;
-            const commit_id = context.payload.pull_request.head.sha;
+            const pull_number = Number(process.env.PULL_NUMBER);
+            const commit_id = process.env.HEAD_SHA;
 
-            // Diff-lines-only: comment only on lines the PR added or changed. A
-            // finding on an untouched line of an edited file is pre-existing and
-            // out of scope, so we skip it (no summary comment).
+            // Diff-lines-only: comment only on lines the PR added or changed.
             const inDiff = (c) => (diffLines[c.path] || []).includes(c.line);
 
             // De-dupe against what the bot already said on this PR (re-runs on push).

.github/workflows/test-build.yaml

@@ -27,3 +27,28 @@ jobs:
           export DISPLAY=":1"
           Xvfb :1 -screen 0 800x600x8 &
           mvn -U -B clean test
+
+      # The card-script review test writes findings here; hand them, plus the PR
+      # number, to the Card-script review workflow. A pull_request build has a
+      # read-only token on fork PRs and cannot comment, so posting happens there.
+      # One matrix leg only, and always() so a test failure still ships findings.
+      - name: Stage card-script review data
+        if: ${{ always() && github.event_name == 'pull_request' && matrix.java == '17' }}
+        run: |
+          printf '{"number": %s, "head_sha": "%s"}\n' \
+            "${{ github.event.pull_request.number }}" \
+            "${{ github.event.pull_request.head.sha }}" > card-script-pr-meta.json
+          # Stage at repo root so both files sit at the artifact's top level.
+          [ -f forge-game/target/card-script-findings.json ] \
+            && cp forge-game/target/card-script-findings.json card-script-findings.json || true
+
+      - name: Upload card-script review data
+        if: ${{ always() && github.event_name == 'pull_request' && matrix.java == '17' }}
+        uses: actions/upload-artifact@v4
+        with:
+          name: card-script-review
+          path: |
+            card-script-findings.json
+            card-script-pr-meta.json
+          if-no-files-found: ignore
+          retention-days: 1   # only needed until the poster runs minutes later