From 3fff074de06259ce8d36636f2b89841ef7d88ebb Mon Sep 17 00:00:00 2001 From: MostCromulent <201167372+MostCromulent@users.noreply.github.com> Date: Sat, 13 Jun 2026 12:39:01 +0930 Subject: [PATCH 1/5] Add advisory card-script review CI workflow Reviews changed card scripts on a pull request and posts terse inline comments: a deterministic linter (undefined SVar references, duplicate/unknown params, illegal mana tokens, missing ManaCost, loyalty abilities missing Planeswalker) plus a Scryfall frame fact-check (name, type line, P/T, mana cost, loyalty). Advisory only -- it never fails the check, it only comments. Runs on pull_request_target so it can comment on fork PRs; it only reads the PR's cardsfolder/*.txt files as data and runs the base-branch scripts, never executing PR code. The linter derives its known params from the card corpus, so it adapts when the engine adds or renames a param. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/scripts/card_script_review.py | 331 +++++++++++++++++++++++ .github/scripts/cardlint.py | 230 ++++++++++++++++ .github/workflows/card-script-review.yml | 155 +++++++++++ 3 files changed, 716 insertions(+) create mode 100644 .github/scripts/card_script_review.py create mode 100644 .github/scripts/cardlint.py create mode 100644 .github/workflows/card-script-review.yml diff --git a/.github/scripts/card_script_review.py b/.github/scripts/card_script_review.py new file mode 100644 index 00000000000..70722c84247 --- /dev/null +++ b/.github/scripts/card_script_review.py @@ -0,0 +1,331 @@ +#!/usr/bin/env python3 +"""Review changed card scripts on a pull request and emit terse inline comments. + +Runs two independent, low-risk checks on each changed card and prints a JSON list +of `{path, line, body}` comments for the workflow to post. Both are advisory. + + 1. cardlint.py — a deterministic linter for the card-script DSL: undefined SVar + references (a bad `Execute$` means the card fails to load), duplicate or + unknown params, illegal mana tokens, a missing `ManaCost`, and similar + mechanical issues. Its set of "known params" is derived from the card corpus + itself, so when a param is added or renamed the corpus reflects it and the + linter adapts on its own — nothing here is hard-coded against the game engine. + + 2. Scryfall fact-check — compares only the card FRAME (name, type line, P/T, mana + cost, loyalty) against the printed card. Catches transcription slips such as a + missing Legendary supertype, an instant/sorcery swap, or a wrong mana symbol. + When the card is not on Scryfall (e.g. an unreleased card) it stays silent; + see scryfall_facts(). + +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. +If the linter emits a finding code this file doesn't recognise, terse_comment() +falls back to the linter's own message rather than failing. Comments are kept +short, in `wrong -> right` form. + +Usage: card_script_review.py + (one repo-relative card path per line; non-card paths are ignored) +Output: JSON array of {path, line, body} on stdout; a human summary on stderr. +""" +import sys, os, io, re, json, time, collections, contextlib +import urllib.parse, urllib.request, urllib.error + +HERE = os.path.dirname(os.path.abspath(__file__)) +sys.path.insert(0, HERE) +import cardlint # noqa: E402 (sibling module, committed alongside this script) + +ARROW = "→" # → , so the comments read naturally in the GitHub UI +SCRYFALL = "https://api.scryfall.com/cards/named" + +# Scryfall asks for ~100 ms between requests. We pace every call and, on the first +# 429, stop calling for the rest of the run so a big PR can't storm the API and +# silently turn every card into "not found". The skip is logged, not hidden. +_MIN_INTERVAL = 0.1 +_last_call = [0.0] +_rate_limited = [False] + + +def _throttle(): + dt = time.time() - _last_call[0] + if dt < _MIN_INTERVAL: + time.sleep(_MIN_INTERVAL - dt) + _last_call[0] = time.time() + + +# --------------------------------------------------------------------------- # +# 1. Linter findings -> terse comments +# --------------------------------------------------------------------------- # +def terse_comment(code, msg): + """Render one cardlint finding as a terse PR comment. + + Falls back to the linter's own message for any code it doesn't special-case, + so a new check added to the linter never breaks this script — the comment just + reads a little more verbosely until someone adds a template here. + """ + def grab(pat): + m = re.search(pat, msg) + return m.group(1) if m else None + + if code == "REF-UNDEF": + m = re.match(r"(\w+\$) '([^']+)' undefined SVar", msg) + if m: + key, name = m.group(1), m.group(2) + why = "card won't load" if key.startswith("Execute") else "clause dropped" + return f"`{key} {name}` {ARROW} undefined SVar ({why})" + elif code == "KEY-TYPO": + m = re.match(r"'(\w+)\$' not in corpus; did you mean '(\w+)\$'", msg) + if m: + return f"`{m.group(1)}$` {ARROW} `{m.group(2)}$`" + elif code == "UNKNOWN-KEY": + k = grab(r"'(\w+)\$'") + if k: + return f"`{k}$` {ARROW} appears in no other card (unknown or outdated param?)" + elif code == "DUP-PARAM": + k = grab(r"duplicate '(\w+)\$'") + if k: + return f"duplicate `{k}$` {ARROW} engine keeps only the last" + elif code == "NO-MANACOST": + return "missing a `ManaCost` line" + elif code == "LOYALTY": + return "loyalty ability needs `Planeswalker$ True`" + elif code == "TRIG-CTX": + t = grab(r"'(\w+)'") + if t: + return f"`{t}` is a trigger-only token on an `A:` line (no triggering context)" + elif code == "LEX-DELIM": + k = grab(r"(\w+)\$") + if k: + return f"`{k}$` is comma-separated, but the engine splits on ` & `" + elif code == "LEX-FILTER": + t = grab(r"filter '([^']+)'") + if t: + return f"`{t}` has multiple `.` {ARROW} matches nothing" + elif code == "DESC-NAME": + t = grab(r"literal '([^']+)'") + if t: + return f"literal `{t}` in the description {ARROW} use NICKNAME / CARDNAME" + elif code == "DESC-COST": + return "SpellDescription restates the activation cost" + elif code == "ORPHAN": + t = grab(r"SVar '([^']+)'") + if t: + return f"SVar `{t}` is never referenced (clause never fires)" + elif code == "CASE": + m = re.match(r"(\w+)\$ '([^']+)' miscased \(want '([^']+)'", msg) + if m: + return f"`{m.group(1)}$ {m.group(2)}` {ARROW} `{m.group(3)}` (case-sensitive)" + elif code == "LEX-PIPE": + return "add a space around the `|` separator" + elif code == "LEX-CURLY": + return f"curly apostrophe `’` {ARROW} ASCII `'`" + elif code == "MANA": + t = grab(r"token '([^']+)'") + if t: + return f"illegal mana token `{t}`" + # default: the linter message is already human-readable + return msg + + +def lint_comments(path, freq): + """Run the linter on one card and return [(line, body), ...].""" + sink = io.StringIO() + with contextlib.redirect_stdout(sink): # lint() also prints; we want the data + findings = cardlint.lint(path, freq) + out = [] + for line, sev, code, msg in findings: + out.append((line, terse_comment(code, msg))) + return out + + +# --------------------------------------------------------------------------- # +# 2. Scryfall frame fact-check +# --------------------------------------------------------------------------- # +def read_frame(path): + """Pull the front face's frame fields and the line each sits on. + + Stops at the `ALTERNATE` separator so a multi-section file (DFC, meld, flip, + split, adventure) yields only the FRONT face's frame -- otherwise last-wins + parsing builds a frankenframe from both faces and produces false diffs. + """ + frame = {} + for i, raw in enumerate(open(path, encoding="utf-8", errors="ignore").read().split("\n"), 1): + if raw.strip() == "ALTERNATE": + break + for field in ("Name", "ManaCost", "Types", "PT", "Loyalty"): + if raw.startswith(field + ":"): + frame[field] = (i, raw[len(field) + 1:].strip()) + return frame + + +def scryfall_lookup(name): + """Return the Scryfall card dict, or None if the card isn't indexed. + + Tries an EXACT name match first (no false matches). Only if that misses does + it fall back to a fuzzy match, and then accepts the result solely when the + returned name is within a small edit distance of ours — i.e. the same card + with a transcription typo. Anything farther away is treated as "not on + Scryfall" and the check stays silent (e.g. an unreleased card). + """ + def get(params): + if _rate_limited[0]: + return None + _throttle() + url = SCRYFALL + "?" + urllib.parse.urlencode(params) + req = urllib.request.Request(url, headers={ + "User-Agent": "ForgeCardScriptReviewBot/1.0 (+github-actions)", + "Accept": "application/json"}) + try: + with urllib.request.urlopen(req, timeout=20) as r: + return json.load(r) + except urllib.error.HTTPError as e: + if e.code == 429 and not _rate_limited[0]: + _rate_limited[0] = True + print("Scryfall rate-limited (429) — skipping remaining fact-checks " + "this run", file=sys.stderr) + return None # 404 = not found, 422 = ambiguous fuzzy, etc. + except Exception: + return None # network hiccup -> stay silent, never block + + card = get({"exact": name}) + if card: + return card + card = get({"fuzzy": name}) + if card and 0 < _edit_distance(name.lower(), card.get("name", "").lower()) <= 2: + return card # same card, name has a small typo + return None + + +def _edit_distance(a, b, cap=3): + """Levenshtein distance, capped (we only care about 'small').""" + if abs(len(a) - len(b)) > cap: + return cap + 1 + prev = list(range(len(b) + 1)) + for i, ca in enumerate(a, 1): + cur = [i] + for j, cb in enumerate(b, 1): + cur.append(min(prev[j] + 1, cur[-1] + 1, prev[j - 1] + (ca != cb))) + prev = cur + if min(prev) > cap: + return cap + 1 + return prev[-1] + + +def _mana_to_forge(scryfall_cost): + """`{2}{W/U}{R}` -> `2 WU R`; phyrexian `{G/P}` stays `G/P`. For display+compare.""" + out = [] + for sym in re.findall(r"\{([^}]+)\}", scryfall_cost or ""): + if "/" in sym and "P" not in sym.upper(): + out.append(sym.replace("/", "")) # two-colour hybrid: W/U -> WU + else: + out.append(sym) + return " ".join(out) + + +def _norm_mana(s): + """Order-independent, punctuation-independent mana comparison key.""" + return sorted(re.sub(r"[^A-Za-z0-9]", "", t).upper() for t in s.split() if t) + + +def scryfall_facts(path): + """Compare the card frame against Scryfall. Returns [(line, body), ...]. + + Silent (returns []) when the card isn't on Scryfall, e.g. an unreleased card. + Only the stable frame fields are compared, so this never needs to know how an + ability is scripted. + """ + frame = read_frame(path) + if "Name" not in frame: + return [] + name_line, name = frame["Name"] + card = scryfall_lookup(name) + if not card: + return [] + # Multi-faced cards (DFC/MDFC/split/adventure) keep their frame and text under + # `card_faces`; a single Forge frame line can't be compared cleanly, so skip + # them rather than emit false diffs. + if card.get("card_faces"): + return [] + + out = [] + + # Name (only reachable via the fuzzy path, i.e. a real typo) + real_name = card.get("name", "") + if real_name and real_name != name: + out.append((name_line, f"Name `{name}` {ARROW} `{real_name}`")) + + # Type line — compare token SETS so harmless ordering differences don't flag, + # but a missing supertype (e.g. Legendary) or instant/sorcery swap does. + if "Types" in frame: + line, ours = frame["Types"] + theirs = card.get("type_line", "").replace("—", " ") + if theirs and set(ours.split()) != set(theirs.split()): + out.append((line, f"Types `{ours}` {ARROW} `{' '.join(theirs.split())}`")) + + # Power/Toughness — only when both are present and plainly differ. + if "PT" in frame and card.get("power") is not None: + line, ours = frame["PT"] + theirs = f"{card.get('power')}/{card.get('toughness')}" + if "/" in ours and ours != theirs: + out.append((line, f"PT `{ours}` {ARROW} `{theirs}`")) + + # Mana cost — order/punctuation-independent compare; show Forge-style suggestion. + if "ManaCost" in frame and card.get("mana_cost"): + line, ours = frame["ManaCost"] + theirs = _mana_to_forge(card["mana_cost"]) + if ours.lower() not in ("no cost", "") and _norm_mana(ours) != _norm_mana(theirs): + out.append((line, f"ManaCost `{ours}` {ARROW} `{theirs}`")) + + # Loyalty / starting loyalty + if "Loyalty" in frame and card.get("loyalty"): + line, ours = frame["Loyalty"] + if ours != str(card["loyalty"]): + out.append((line, f"Loyalty `{ours}` {ARROW} `{card['loyalty']}`")) + + return out + + +# --------------------------------------------------------------------------- # +# main +# --------------------------------------------------------------------------- # +def main(): + if len(sys.argv) < 2: + print("usage: card_script_review.py ", 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("\\", "/") 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: + text = open(path, encoding="utf-8", errors="ignore").read() + 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: + found = [] + try: + found += lint_comments(path, freq) + except Exception as e: # never let one bad card abort the run + print(f"lint error on {path}: {e}", file=sys.stderr) + try: + found += scryfall_facts(path) + except Exception as e: + print(f"scryfall error on {path}: {e}", file=sys.stderr) + for line, body in found: + comments.append({"path": path.replace("\\", "/"), "line": line, "body": body}) + + 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 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/scripts/cardlint.py b/.github/scripts/cardlint.py new file mode 100644 index 00000000000..4bdfd03ed1d --- /dev/null +++ b/.github/scripts/cardlint.py @@ -0,0 +1,230 @@ +#!/usr/bin/env python3 +"""Deterministic linter for Forge card scripts. + +Checks a card script for mechanical issues: dead references, duplicate params, +orphaned SVars, malformed costs/filters, lexical typos, literal names in +descriptions, loyalty-ability hints, near-miss param-key typos, unknown params +absent from the whole corpus (UNKNOWN-KEY), a missing ManaCost on a non-Land card +(NO-MANACOST), and trigger-only tokens used outside a trigger. It does not check +value accuracy (P/T, cost VALUE, may-vs-mandatory all need the real card) or what +an ability actually does. Output: file:line: [ERROR|WARN] CODE msg. + +Usage: python3 cardlint.py [--corpus ] [more.txt ...] + --corpus enables the KEY near-miss check (defaults to auto-detected + forge-gui/res/cardsfolder near cwd; skipped if not found). +Exit: 1 if any findings, else 0. +""" +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"} +LIST_REF_KEYS = {"Choices"} +AMP_LIST_KEYS = {"AddTypes","AddKeyword","AddKeywords","RemoveKeywords", + "AddTrigger","AddStatic","AddReplacement","Triggers"} +VALID_KEYS = {"ValidTgts","ValidCard","ValidCards","Valid","Affected","ValidSource", + "ValidTarget","ChangeType","ChangeValid","SacValid","ValidCause"} +DESC_KEYS = {"SpellDescription","TriggerDescription","StackDescription","Description"} +SPACED_KEYS = DESC_KEYS | {"Name"} +CANON = {"self":"Self","targeted":"Targeted","remembered":"Remembered", + "imprinted":"Imprinted","battlefield":"Battlefield","exile":"Exile", + "graveyard":"Graveyard","hand":"Hand","library":"Library", + "command":"Command","stack":"Stack"} +TRIG_TOKEN = re.compile(r"\bTriggered[A-Za-z]+\b") # trigger-context-only references + +def split_params(body): + out=[] + for piece in re.split(r"\s*\|\s*", body.strip()): + if not piece: continue + if "$" in piece: + k,v=piece.split("$",1); out.append((k.strip(),v.strip(),piece)) + else: out.append((piece.strip(),None,piece)) + return out + +def check_mana(cost,i,add): + # Lexical only: hybrids (WB, RW, 2R, U4, W/P) are legal and indistinguishable + # from a typo like 'R2' by form -- mana-cost VALUE accuracy needs the real card. + if cost.lower() in ("no cost","nocost",""): return + for tok in cost.split(): + if not re.fullmatch(r"[0-9WUBRGCSXYP/]+",tok): + add(i,"ERROR","MANA",f"illegal character in mana token '{tok}' in ManaCost '{cost}'") + +def edit1(a,b): + """True iff a and b differ by exactly one edit (sub / ins / del).""" + if a==b: return False + la,lb=len(a),len(b) + if abs(la-lb)>1: return False + if la==lb: + return sum(1 for x,y in zip(a,b) if x!=y)==1 + if la>lb: a,b,la,lb=b,a,lb,la # a is the shorter + i=0 + while i time.time()-7*86400): + return json.load(open(cache,encoding="utf-8")) + except Exception: pass + freq={} + for root,_,files in os.walk(corpus): + for fn in files: + 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 KEY_TOKEN.findall(txt): + freq[k]=freq.get(k,0)+1 + try: json.dump(freq,open(cache,"w",encoding="utf-8")) + except Exception: pass + return freq + +def lint(path, freq=None): + F=[]; add=lambda ln,s,c,m:F.append((ln,s,c,m)) + text=open(path,encoding="utf-8").read(); lines=text.split("\n") + nick=""; defined={} + for i,l in enumerate(lines,1): + if l.startswith("Name:"): nick=l[5:].strip().split(",")[0].strip() + if l.startswith("SVar:"): + p=l.split(":",2) + if len(p)==3: defined[p[1]]=(i,p[2]) + def wordcount(w): return len(re.findall(rf"\b{re.escape(w)}\b",text)) + bare=lambda s: re.fullmatch(r"[A-Za-z0-9_]+",s) is not None + # candidate "known" keys to suggest against (frequent in the corpus) + known=[k for k,n in (freq or {}).items() if n>=50] if freq else [] + # case-insensitive set of every corpus key (param map is case-insensitive, + # so a case-only difference like PreCostDesc vs PrecostDesc is harmless) + freq_lower=set(k.lower() for k in (freq or {})) + + for i,l in enumerate(lines,1): + if l in ("","ALTERNATE"): continue + m=re.match(r"([A-Za-z]+)(::?)",l) + if m: + pfx,col=m.group(1),m.group(2) + if col=="::": add(i,"ERROR","LEX-PREFIX",f"double-colon prefix '{pfx}::'") + elif pfx not in LINE_PREFIXES and pfx.lower() in PFX_LOWER: + add(i,"ERROR","LEX-PREFIX",f"miscased prefix '{pfx}:' (want '{PFX_LOWER[pfx.lower()]}:')") + if "’" in l: add(i,"WARN","LEX-CURLY","curly apostrophe U+2019 (use ASCII apostrophe)") + if not re.match(r"(DeckHas|DeckHints|DeckNeeds):",l) and (re.search(r"\S\|",l) or re.search(r"\|\S",l)): + add(i,"WARN","LEX-PIPE","'|' separator missing a surrounding space") + if "$ " in l: add(i,"WARN","LEX-DBLSPACE","double space after '$'") + + if l.startswith("K:Chapter:"): + cps=l.split(":") + if len(cps)>=4: + for nm in cps[3].split(","): + nm=nm.strip() + if nm and nm not in defined: + add(i,"ERROR","REF-UNDEF",f"Chapter ability '{nm}' is not a defined SVar") + + is_a = l.startswith("A:") + body=None + if l.startswith("SVar:"): + p=l.split(":",2); body=p[2] if len(p)==3 else None + elif re.match(r"(A|T|S|R):",l): body=l.split(":",1)[1] + if body is None: + if l.startswith("ManaCost:"): check_mana(l[9:].strip(),i,add) + continue + + seen={}; desc="" + for k,v,raw in split_params(body): + if k in seen: + add(i,"ERROR","DUP-PARAM",f"duplicate '{k}$' (engine keeps last '{v}', drops '{seen[k]}')") + seen[k]=v + # param-key sanity: a key$ token absent from the whole corpus (the param + # map is case-insensitive, so a case-only difference is harmless). A + # near-miss of a frequent key is a typo (ERROR); no near-miss at all is an + # unknown param the engine silently ignores (WARN). Guard on v so a bare + # SVar value token (e.g. `PlayMain1:TRUE`) isn't mistaken for a key. + if (freq is not None and v is not None and bare(k) and len(k)>=4 + and freq.get(k,0)==0 and k.lower() not in freq_lower): + cand=[kk for kk in known if edit1(k,kk)] + if cand: + best=max(cand,key=lambda kk:freq[kk]) + add(i,"ERROR","KEY-TYPO",f"'{k}$' not in corpus; did you mean '{best}$' (freq {freq[best]})?") + else: + add(i,"WARN","UNKNOWN-KEY",f"'{k}$' appears in no other card — engine silently ignores unknown params (typo, outdated, or made-up?). NB: a param valid for a DIFFERENT API still slips past this.") + if v is None: continue + if k in REF_KEYS and bare(v) and v not in defined: + note=("card FAILS TO LOAD - hard RuntimeException at parse (Trigger.ensureAbility)" + if k=="Execute" else "clause silently does nothing") + add(i,"ERROR","REF-UNDEF",f"{k}$ '{v}' undefined SVar - {note}") + if k in LIST_REF_KEYS: + for nm in v.split(","): + nm=nm.strip() + if nm and bare(nm) and nm not in defined: + add(i,"ERROR","REF-UNDEF",f"{k}$ choice '{nm}' undefined") + if k in SPACED_KEYS and raw[len(k)+1:len(k)+2] not in (" ",""): + add(i,"ERROR","LEX-NOSPACE",f"'{k}$' not followed by a space") + if k in AMP_LIST_KEYS and "," in v: + add(i,"ERROR","LEX-DELIM",f"{k}$ uses ',' but engine splits on ' & ' -> becomes one bogus entry") + if k in VALID_KEYS: + for tok in re.split(r"[ ,]",v): + if tok.count(".")>=2: + add(i,"ERROR","LEX-FILTER",f"{k}$ filter '{tok}' has multiple '.' (malformed property; matches nothing)") + head=v.split()[0] if v.split() else "" + if k in ("Defined","Origin","Destination") and head.lower() in CANON and head!=CANON[head.lower()]: + add(i,"ERROR","CASE",f"{k}$ '{head}' miscased (want '{CANON[head.lower()]}', engine is case-sensitive)") + if k in DESC_KEYS: desc+=" "+v + if k=="SpellDescription" and re.match(r"\{[^}]+\}:",v.strip()): + add(i,"WARN","DESC-COST","SpellDescription restates the activation cost ('{..}:')") + if desc and nick and len(nick)>=3 and re.search(rf"\b{re.escape(nick)}\b",desc): + add(i,"WARN","DESC-NAME",f"literal '{nick}' in description (use NICKNAME/CARDNAME)") + if "LOYALTY" in (seen.get("Cost") or "") and seen.get("Planeswalker")!="True": + add(i,"ERROR","LOYALTY","loyalty-cost ability lacks 'Planeswalker$ True'") + # trigger-only tokens (TriggeredSource*, TriggeredTarget*, ...) never resolve + # in a directly-activated A: ability -- they belong in a trigger's SVar. + if is_a: + for tok in sorted(set(TRIG_TOKEN.findall(body))): + add(i,"ERROR","TRIG-CTX",f"trigger-only token '{tok}' on an A: line — belongs in a trigger's SVar (an A: ability has no triggering context)") + # NOTE: a controller<->description target check is intentionally absent -- + # it is too noisy to be a reliable lexical rule. + + # every non-Land card needs a ManaCost (lands have none; DFC/back faces under + # ALTERNATE are exempt -- the front face always carries the cost). 0 FP across + # the corpus. + if "ALTERNATE" not in text: + types=next((l[6:] for l in lines if l.startswith("Types:")),None) + if (types is not None and "Land" not in types + and not any(l.startswith("ManaCost:") for l in lines)): + ln=next((j for j,l in enumerate(lines,1) if l.startswith("Name:")),1) + add(ln,"ERROR","NO-MANACOST","card has no ManaCost line (non-Land cards need one)") + + for nm,(ln,bdy) in defined.items(): + if re.match(r"\s*(DB|AB|SP)\$",bdy) and wordcount(nm)<=1: + add(ln,"WARN","ORPHAN",f"SVar '{nm}' never referenced (clause never fires)") + + base=os.path.basename(path); F.sort() + for ln,s,c,msg in F: print(f"{base}:{ln}: [{s}] {c} {msg}") + return F + +if __name__=="__main__": + args=sys.argv[1:]; corpus=None + if "--corpus" in args: + j=args.index("--corpus"); corpus=args[j+1]; del args[j:j+2] + if corpus is None: corpus=find_corpus() + freq=key_freq(corpus) + total=0 + for f in args: + try: total+=len(lint(f,freq)) + except Exception as e: print(f"{f}: [ERROR] LINT-FAIL {e}") + print(f"\n== {total} finding(s) ==") + sys.exit(1 if total else 0) diff --git a/.github/workflows/card-script-review.yml b/.github/workflows/card-script-review.yml new file mode 100644 index 00000000000..4a650323cf4 --- /dev/null +++ b/.github/workflows/card-script-review.yml @@ -0,0 +1,155 @@ +# 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. +# +# 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. +# +# 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. + +name: Card-script review + +on: + pull_request_target: + paths: + - 'forge-gui/res/cardsfolder/**' + +permissions: + contents: read + pull-requests: write + +concurrency: + group: card-script-review-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + review: + runs-on: ubuntu-latest + 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 + with: + python-version: '3.11' + + - name: List changed card files + uses: actions/github-script@v7 + with: + script: | + const fs = require('fs'); + 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). + const cards = files.filter(f => f.status !== 'removed' + && f.filename.startsWith('forge-gui/res/cardsfolder/') + && f.filename.endsWith('.txt')); + 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. + const addedLines = {}; + for (const f of cards) { + 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 { 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 + # the base copies in place. We only read them; we never run them. + - name: Materialize PR card files (data only) + env: + PR_NUMBER: ${{ github.event.pull_request.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')). + 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 + 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 + run: | + python .github/scripts/card_script_review.py changed_files.txt > comments.json + echo "---- comments ----" + cat comments.json + + - name: Post inline comments + uses: actions/github-script@v7 + with: + script: | + const fs = require('fs'); + let comments = [], diffLines = {}; + try { comments = JSON.parse(fs.readFileSync('comments.json', 'utf8')); } + catch (e) { core.info('no comments.json — nothing to post'); return; } + try { diffLines = JSON.parse(fs.readFileSync('diff_lines.json', 'utf8')); } + catch (e) {} + 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; + + // 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). + const inDiff = (c) => (diffLines[c.path] || []).includes(c.line); + + // De-dupe against what the bot already said on this PR (re-runs on push). + const existing = await github.paginate(github.rest.pulls.listReviewComments, + { owner, repo, pull_number }); + const seen = new Set(existing + .filter(c => c.user.type === 'Bot') + .map(c => `${c.path}|${c.line}|${c.body}`)); + + let posted = 0, skipped = 0; + for (const c of comments) { + if (!inDiff(c)) { skipped++; continue; } + const key = `${c.path}|${c.line}|${c.body}`; + if (seen.has(key)) continue; + try { + await github.rest.pulls.createReviewComment({ + owner, repo, pull_number, commit_id, + path: c.path, line: c.line, side: 'RIGHT', body: c.body, + }); + seen.add(key); + posted++; + } catch (e) { + // Pre-filtered to diff lines, so this is a real API error, not a + // line-position problem. Surface it in the log; don't comment. + core.warning(`failed to comment on ${c.path}:${c.line} — ${e.message}`); + } + } + core.info(`posted ${posted}, skipped ${skipped} out-of-diff finding(s)`); From 5a8f662c6707ae369bf52441606ce457ec65aa9a Mon Sep 17 00:00:00 2001 From: MostCromulent <201167372+MostCromulent@users.noreply.github.com> Date: Sat, 13 Jun 2026 21:42:59 +0930 Subject: [PATCH 2/5] Bump card-script-review workflow actions to Node 24 versions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit checkout v4->v5, setup-python v5->v6, github-script v7->v8 — all runtime-only bumps off the deprecated Node 20. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/card-script-review.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/card-script-review.yml b/.github/workflows/card-script-review.yml index 4a650323cf4..0bb98b4089c 100644 --- a/.github/workflows/card-script-review.yml +++ b/.github/workflows/card-script-review.yml @@ -36,15 +36,15 @@ jobs: # 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 + uses: actions/checkout@v5 - name: Set up Python - uses: actions/setup-python@v5 + uses: actions/setup-python@v6 with: python-version: '3.11' - name: List changed card files - uses: actions/github-script@v7 + uses: actions/github-script@v8 with: script: | const fs = require('fs'); @@ -107,7 +107,7 @@ jobs: cat comments.json - name: Post inline comments - uses: actions/github-script@v7 + uses: actions/github-script@v8 with: script: | const fs = require('fs'); From 497205a10f1ad8662c40d5577a05b1969a177d8b Mon Sep 17 00:00:00 2001 From: MostCromulent <201167372+MostCromulent@users.noreply.github.com> Date: Sun, 14 Jun 2026 07:42:36 +0930 Subject: [PATCH 3/5] Split card-script review into a build check and a workflow_run commenter The card-script review now runs its analysis as part of the normal build and hands the findings to a separate workflow that posts them, replacing the single pull_request_target workflow. The reason for the split: a pull_request build has a read-only token on fork PRs (almost all card PRs), so it can produce findings but cannot comment. A workflow_run commenter runs afterwards in the base repo with a write token and does the posting. The flow: - test-build runs a new pure-advisory test (CardScriptApiTest) and uploads its findings plus the PR number as an artifact (if: always, so a red build still ships findings); - the commenter downloads the artifact, runs the deterministic linter and the Scryfall frame check, and posts the merged, deduped comments on the changed lines only. The new test reuses the engine's own definitions -- the same checks it runs at card load -- rather than re-encoding them: - ability/trigger/replacement API names via ApiType, TriggerType and ReplacementType, catching a typo'd API such as DB$ DealDamge that the corpus-based linter cannot; - sub-ability params in AbilityFactory.additionalAbilityKeys must point at a defined SVar, closing a gap where the linter knew only a handful of these keys. cardlint's REF_KEYS drops the now-engine-checked keys. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/scripts/card_script_review.py | 47 ++++- .github/scripts/cardlint.py | 5 +- .github/workflows/card-script-review.yml | 116 ++++++----- .github/workflows/test-build.yaml | 25 +++ .../forge/game/ability/CardScriptApiTest.java | 189 ++++++++++++++++++ 5 files changed, 329 insertions(+), 53 deletions(-) create mode 100644 forge-game/src/test/java/forge/game/ability/CardScriptApiTest.java diff --git a/.github/scripts/card_script_review.py b/.github/scripts/card_script_review.py index 70722c84247..432d3425491 100644 --- a/.github/scripts/card_script_review.py +++ b/.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. @@ -123,6 +128,10 @@ def grab(pat): 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,11 +297,40 @@ 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 ", 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=sys.stderr) return 2 - paths = [l.strip() for l in open(sys.argv[1], encoding="utf-8") if l.strip()] + 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)] @@ -322,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 diff --git a/.github/scripts/cardlint.py b/.github/scripts/cardlint.py index 4bdfd03ed1d..c664b916207 100644 --- a/.github/scripts/cardlint.py +++ b/.github/scripts/cardlint.py @@ -22,8 +22,9 @@ "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"} diff --git a/.github/workflows/card-script-review.yml b/.github/workflows/card-script-review.yml index 0bb98b4089c..46bf6fb2144 100644 --- a/.github/workflows/card-script-review.yml +++ b/.github/workflows/card-script-review.yml @@ -1,79 +1,88 @@ -# 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 + # Check out first: actions/checkout cleans the workspace, so it must run + # before the artifact download and the files the resolve step writes. + - name: Check out base scripts and corpus uses: actions/checkout@v5 - - name: Set up Python - uses: actions/setup-python@v6 + - 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 + - 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; @@ -81,16 +90,22 @@ jobs: 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: 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")" @@ -98,16 +113,23 @@ jobs: 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 + 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 +141,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). diff --git a/.github/workflows/test-build.yaml b/.github/workflows/test-build.yaml index c293c074119..7c4420650d7 100644 --- a/.github/workflows/test-build.yaml +++ b/.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 diff --git a/forge-game/src/test/java/forge/game/ability/CardScriptApiTest.java b/forge-game/src/test/java/forge/game/ability/CardScriptApiTest.java new file mode 100644 index 00000000000..5ff37f078d5 --- /dev/null +++ b/forge-game/src/test/java/forge/game/ability/CardScriptApiTest.java @@ -0,0 +1,189 @@ +package forge.game.ability; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import java.util.stream.Stream; + +import org.testng.annotations.Test; + +import forge.game.replacement.ReplacementType; +import forge.game.trigger.TriggerType; + +/** + * Advisory: validates each card script against the engine's own definitions and + * writes findings to {@code target/card-script-findings.json} for the review + * workflow to post. Two engine-sourced checks, neither hard-coded here: + *
    + *
  • ability/trigger/replacement API names against {@link ApiType}/ + * {@link TriggerType}/{@link ReplacementType} (the same {@code smartValueOf} + * the engine runs at card load);
    • + *
  • sub-ability params in {@link AbilityFactory#additionalAbilityKeys} must + * reference a defined SVar. {@code Execute} is left to the linter, which + * reports it as a hard load failure.
    • + *
+ * The test never fails, so it stays out of the build's pass/fail signal. + */ +public class CardScriptApiTest { + + private static final Pattern KEY = Pattern.compile("([A-Za-z][A-Za-z0-9]*)\\$(.*)"); + private static final Pattern SVAR_NAME = Pattern.compile("[A-Za-z0-9_]+"); + private static final Set REF_KEYS = refKeys(); + + private static Set refKeys() { + Set keys = new HashSet<>(AbilityFactory.additionalAbilityKeys); + keys.remove("Execute"); // the linter owns Execute (reports a hard load failure) + return keys; + } + + @Test + public void writeApiFindings() throws IOException { + Path root = locateRoot(); + Path corpus = root.resolve("forge-gui/res/cardsfolder"); + List findings = new ArrayList<>(); + try (Stream paths = Files.walk(corpus)) { + paths.filter(p -> p.toString().endsWith(".txt")).sorted().forEach(p -> { + String rel = root.relativize(p).toString().replace('\\', '/'); + try { + lintFile(p, rel, findings); + } catch (IOException e) { + // unreadable card is the build's problem, not the linter's + } + }); + } + Path out = Paths.get("target"); + Files.createDirectories(out); + Files.write(out.resolve("card-script-findings.json"), + toJson(findings).getBytes(StandardCharsets.UTF_8)); + } + + private static void lintFile(Path file, String rel, List findings) throws IOException { + List lines = Files.readAllLines(file, StandardCharsets.UTF_8); + Set svars = new HashSet<>(); + for (String line : lines) { + if (line.startsWith("SVar:")) { + String[] p = line.split(":", 3); + if (p.length == 3) { + svars.add(p[1]); + } + } + } + for (int i = 0; i < lines.size(); i++) { + String line = lines.get(i); + int ln = i + 1; + String body = null; + if (line.startsWith("A:")) { + body = line.substring(2); + checkAbility(body, rel, ln, findings); + } else if (line.startsWith("T:")) { + body = line.substring(2); + checkNamed(body, "Mode", "trigger", rel, ln, findings); + } else if (line.startsWith("R:")) { + body = line.substring(2); + checkNamed(body, "Event", "replacement", rel, ln, findings); + } else if (line.startsWith("S:")) { + body = line.substring(2); + } else if (line.startsWith("SVar:")) { + String[] p = line.split(":", 3); + if (p.length == 3) { + body = p[2]; + checkAbility(body, rel, ln, findings); + } + } + if (body != null) { + checkRefs(body, svars, rel, ln, findings); + } + } + } + + private static void checkAbility(String body, String rel, int ln, List findings) { + Matcher m = KEY.matcher(firstSegment(body)); + if (!m.matches()) { + return; + } + String key = m.group(1); + if (!(key.equals("AB") || key.equals("SP") || key.equals("ST") || key.equals("DB"))) { + return; + } + String api = m.group(2).trim(); + try { + ApiType.smartValueOf(api); + } catch (RuntimeException e) { + findings.add(finding(rel, ln, "API-UNKNOWN", key + "$ '" + api + "' is not a known API")); + } + } + + private static void checkNamed(String body, String param, String kind, String rel, int ln, + List findings) { + for (String seg : body.split("\\|")) { + Matcher m = KEY.matcher(seg.trim()); + if (!m.matches() || !m.group(1).equals(param)) { + continue; + } + String api = m.group(2).trim(); + try { + if (param.equals("Mode")) { + TriggerType.smartValueOf(api); + } else { + ReplacementType.smartValueOf(api); + } + } catch (RuntimeException e) { + findings.add(finding(rel, ln, "API-UNKNOWN", + param + "$ '" + api + "' is not a known " + kind + " type")); + } + return; + } + } + + private static void checkRefs(String body, Set svars, String rel, int ln, + List findings) { + for (String seg : body.split("\\|")) { + int d = seg.indexOf('$'); + if (d < 0) { + continue; + } + String key = seg.substring(0, d).trim(); + String val = seg.substring(d + 1).trim(); + if (REF_KEYS.contains(key) && SVAR_NAME.matcher(val).matches() && !svars.contains(val)) { + findings.add(finding(rel, ln, "REF-UNDEF", key + "$ '" + val + "' undefined SVar")); + } + } + } + + private static String firstSegment(String body) { + int bar = body.indexOf('|'); + return (bar < 0 ? body : body.substring(0, bar)).trim(); + } + + private static String finding(String path, int line, String code, String message) { + return "{\"path\":\"" + path + "\",\"line\":" + line + + ",\"code\":\"" + code + "\",\"body\":\"" + escape(message) + "\"}"; + } + + private static String toJson(List findings) { + return "[" + String.join(",", findings) + "]"; + } + + private static String escape(String s) { + return s.replace("\\", "\\\\").replace("\"", "\\\""); + } + + private static Path locateRoot() { + Path dir = Paths.get(System.getProperty("user.dir")).toAbsolutePath(); + while (dir != null) { + if (Files.isDirectory(dir.resolve("forge-gui/res/cardsfolder"))) { + return dir; + } + dir = dir.getParent(); + } + throw new IllegalStateException("repo root not found from " + System.getProperty("user.dir")); + } +} From 1ba4a76f17f3e5ebca35f00e2459a4fb72f1f29d Mon Sep 17 00:00:00 2001 From: MostCromulent <201167372+MostCromulent@users.noreply.github.com> Date: Sun, 14 Jun 2026 07:53:45 +0930 Subject: [PATCH 4/5] Bump artifact actions to Node 24 (download-artifact v8, upload-artifact v7) Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/card-script-review.yml | 2 +- .github/workflows/test-build.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/card-script-review.yml b/.github/workflows/card-script-review.yml index 46bf6fb2144..c8dabd60e5b 100644 --- a/.github/workflows/card-script-review.yml +++ b/.github/workflows/card-script-review.yml @@ -43,7 +43,7 @@ jobs: uses: actions/checkout@v5 - name: Download review data from the build - uses: actions/download-artifact@v4 + uses: actions/download-artifact@v8 with: name: card-script-review run-id: ${{ github.event.workflow_run.id }} diff --git a/.github/workflows/test-build.yaml b/.github/workflows/test-build.yaml index 7c4420650d7..d74f1202e4b 100644 --- a/.github/workflows/test-build.yaml +++ b/.github/workflows/test-build.yaml @@ -44,7 +44,7 @@ jobs: - name: Upload card-script review data if: ${{ always() && github.event_name == 'pull_request' && matrix.java == '17' }} - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@v7 with: name: card-script-review path: | From b7425b554dd5e37a2237650125cacfb1741d357d Mon Sep 17 00:00:00 2001 From: MostCromulent <201167372+MostCromulent@users.noreply.github.com> Date: Mon, 15 Jun 2026 07:08:09 +0930 Subject: [PATCH 5/5] Clean up card-script linter vocabulary and harden the review workflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Linter: drop reference/list keys that don't exist in the engine (ChosenSubAbility, AbilityX, AddStatic, AddReplacement) and use the engine's real key names. Split Triggers/AddTriggers/AddStaticAbilities on ',' — the engine's actual delimiter — instead of mis-checking them as ' & ' lists. - Guard LEX-DELIM against parameterized keywords whose values legitimately carry internal commas (Protection:..., OnlyUntapChosen:...), removing pre-existing false positives. Zero LEX-DELIM findings across the corpus. - Correct LINE_PREFIXES to match the DSL: drop SetColor, add HandLifeModifier, Draft, CopyFaceFrom, MeldPair, SPECIALIZE. - Resolve the PR from the artifact's number but re-validate it against the build's trusted head SHA before commenting, so a tampered artifact cannot aim the poster at another PR. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/scripts/cardlint.py | 21 +++++++++++++++------ .github/workflows/card-script-review.yml | 17 +++++++++++++++-- .github/workflows/test-build.yaml | 7 +++---- 3 files changed, 33 insertions(+), 12 deletions(-) diff --git a/.github/scripts/cardlint.py b/.github/scripts/cardlint.py index c664b916207..fa5a7dfac1a 100644 --- a/.github/scripts/cardlint.py +++ b/.github/scripts/cardlint.py @@ -20,14 +20,19 @@ 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"} + "AlternateMode","Variant","ALTERNATE","HandLifeModifier","Draft","CopyFaceFrom", + "MeldPair","SPECIALIZE"} PFX_LOWER = {p.lower():p for p in LINE_PREFIXES} -# 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"} +# Execute (which CardScriptApiTest excludes) and SubAbility (not an +# additionalAbilityKeys key) are checked here; the additionalAbilityKeys sub-ability +# keys are validated against defined SVars by CardScriptApiTest instead. +REF_KEYS = {"Execute","SubAbility"} LIST_REF_KEYS = {"Choices"} +# Engine splits these on " & " (a comma there collapses into one bogus entry). AMP_LIST_KEYS = {"AddTypes","AddKeyword","AddKeywords","RemoveKeywords", - "AddTrigger","AddStatic","AddReplacement","Triggers"} + "AddTrigger","AddStaticAbility","AddReplacementEffect"} +# Engine splits these on "," (a " & " there collapses into one bogus entry). +COMMA_LIST_KEYS = {"Triggers","AddTriggers","AddStaticAbilities"} VALID_KEYS = {"ValidTgts","ValidCard","ValidCards","Valid","Affected","ValidSource", "ValidTarget","ChangeType","ChangeValid","SacValid","ValidCause"} DESC_KEYS = {"SpellDescription","TriggerDescription","StackDescription","Description"} @@ -175,8 +180,12 @@ def wordcount(w): return len(re.findall(rf"\b{re.escape(w)}\b",text)) add(i,"ERROR","REF-UNDEF",f"{k}$ choice '{nm}' undefined") if k in SPACED_KEYS and raw[len(k)+1:len(k)+2] not in (" ",""): add(i,"ERROR","LEX-NOSPACE",f"'{k}$' not followed by a space") - if k in AMP_LIST_KEYS and "," in v: + # A ":" means a parameterized keyword (Protection:...,..., OnlyUntapChosen:A,B,C) + # whose value legitimately carries internal commas; only a comma in a bare list is the bug. + if k in AMP_LIST_KEYS and "," in v and ":" not in v: add(i,"ERROR","LEX-DELIM",f"{k}$ uses ',' but engine splits on ' & ' -> becomes one bogus entry") + if k in COMMA_LIST_KEYS and " & " in v: + add(i,"ERROR","LEX-DELIM",f"{k}$ uses ' & ' but engine splits on ',' -> becomes one bogus entry") if k in VALID_KEYS: for tok in re.split(r"[ ,]",v): if tok.count(".")>=2: diff --git a/.github/workflows/card-script-review.yml b/.github/workflows/card-script-review.yml index c8dabd60e5b..f9e0077dedf 100644 --- a/.github/workflows/card-script-review.yml +++ b/.github/workflows/card-script-review.yml @@ -57,10 +57,23 @@ jobs: with: script: | const fs = require('fs'); + const head_sha = context.payload.workflow_run.head_sha; // trusted: from the event + + // The artifact names a PR number, but it came from an untrusted build, so + // treat it as a hint. Confirm the PR's real head SHA equals this build's + // head SHA before commenting; otherwise a tampered artifact could aim the + // poster at someone else's PR. (listPullRequestsAssociatedWithCommit can't + // replace this — it returns nothing for fork PRs, which is almost all of them.) 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 pull_number = Number(meta.number); + const { data: prData } = await github.rest.pulls.get({ + owner: context.repo.owner, repo: context.repo.repo, pull_number }); + if (prData.state !== 'open' || prData.head.sha !== head_sha) { + core.warning('artifact PR does not match the build head — skipping'); return; + } + const files = await github.paginate(github.rest.pulls.listFiles, { owner: context.repo.owner, repo: context.repo.repo, pull_number }); const cards = files.filter(f => f.status !== 'removed' @@ -68,7 +81,7 @@ jobs: && 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); + core.setOutput('head_sha', head_sha); fs.writeFileSync('changed_files.txt', cards.map(f => f.filename).join('\n')); // For each card, the NEW-file line numbers it adds or changes (the '+' diff --git a/.github/workflows/test-build.yaml b/.github/workflows/test-build.yaml index d74f1202e4b..d68b3a29c7a 100644 --- a/.github/workflows/test-build.yaml +++ b/.github/workflows/test-build.yaml @@ -31,14 +31,13 @@ jobs: # 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. + # The number is a hint only: the poster re-checks it against the build's + # trusted head SHA before commenting, so a tampered artifact can't redirect it. # 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. + printf '{"number": %s}\n' "${{ github.event.pull_request.number }}" > card-script-pr-meta.json [ -f forge-game/target/card-script-findings.json ] \ && cp forge-game/target/card-script-findings.json card-script-findings.json || true