From b98d63b80506032031009cfb7c8b6fc980a61af5 Mon Sep 17 00:00:00 2001 From: MostCromulent <201167372+MostCromulent@users.noreply.github.com> Date: Sat, 13 Jun 2026 09:37:18 +0930 Subject: [PATCH 1/8] Card-script review (advisory CI) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Runs a deterministic card-script linter plus a Scryfall frame fact-check on PRs that touch cardsfolder, and posts terse inline comments on changed lines. Advisory only — never fails the check. Uses pull_request_target to comment on fork PRs while only ever reading the PR's card .txt files as data; never executes PR code. Corpus-driven and frame-only, so engine/ability refactors don't require maintenance. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/scripts/card_script_review.py | 320 +++++++++++++++++++++++ .github/scripts/cardlint.py | 228 ++++++++++++++++ .github/workflows/card-script-review.yml | 153 +++++++++++ 3 files changed, 701 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 000000000000..38b12e575404 --- /dev/null +++ b/.github/scripts/card_script_review.py @@ -0,0 +1,320 @@ +#!/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, contextlib, 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 "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("\\", "/")] + + freq = cardlint.key_freq(cardlint.find_corpus()) # corpus-derived; see module docstring + comments = [] + for path in cards: + if not os.path.exists(path): # deleted in the PR + continue + 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 000000000000..00d47b780011 --- /dev/null +++ b/.github/scripts/cardlint.py @@ -0,0 +1,228 @@ +#!/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, tempfile, hashlib + +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 __import__("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 re.findall(r"([A-Za-z][A-Za-z0-9]*)\$",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 000000000000..5b3f663b9ca5 --- /dev/null +++ b/.github/workflows/card-script-review.yml @@ -0,0 +1,153 @@ +# 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" + while IFS= read -r 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 2e371339fc9af4700713927efb8e4f7fdb39ed01 Mon Sep 17 00:00:00 2001 From: MostCromulent <201167372+MostCromulent@users.noreply.github.com> Date: Sat, 13 Jun 2026 10:08:51 +0930 Subject: [PATCH 2/8] Exclude reviewed cards from the linter corpus frequency When the workflow materializes a PR's card files into cardsfolder, a freshly computed key_freq counted those files too, so a typo'd or made-up param present only in the reviewed card was self-counted as known and KEY-TYPO / UNKNOWN-KEY never fired. key_freq now takes an exclude set (the cards under review). --- .github/scripts/card_script_review.py | 4 ++- .github/scripts/cardlint.py | 35 ++++++++++++++++++--------- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/.github/scripts/card_script_review.py b/.github/scripts/card_script_review.py index 38b12e575404..cc0364bb31ad 100644 --- a/.github/scripts/card_script_review.py +++ b/.github/scripts/card_script_review.py @@ -294,7 +294,9 @@ def main(): 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("\\", "/")] - freq = cardlint.key_freq(cardlint.find_corpus()) # corpus-derived; see module docstring + # Exclude the cards under review from the reference corpus so their own params + # aren't self-counted as "known" (see cardlint.key_freq). + freq = cardlint.key_freq(cardlint.find_corpus(), exclude=[p for p in cards if os.path.exists(p)]) comments = [] for path in cards: if not os.path.exists(path): # deleted in the PR diff --git a/.github/scripts/cardlint.py b/.github/scripts/cardlint.py index 00d47b780011..5c24e6d95386 100644 --- a/.github/scripts/cardlint.py +++ b/.github/scripts/cardlint.py @@ -74,25 +74,38 @@ def find_corpus(): d=nd return None -def key_freq(corpus): - """Frequency of every `key$` token across the corpus (cached 7 days in temp).""" +def key_freq(corpus, exclude=None): + """Frequency of every `key$` token across the corpus (cached 7 days in temp). + + `exclude` is a set of file paths to leave OUT of the count -- pass the cards + under review so their own params aren't self-counted as "known" (otherwise a + typo'd / made-up param present only in the reviewed card looks legitimate and + KEY-TYPO / UNKNOWN-KEY never fire). The cache is bypassed when excluding, since + the exclusion set varies per run. + """ if not corpus or not os.path.isdir(corpus): return None - 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): - return json.load(open(cache,encoding="utf-8")) - except Exception: pass + exclude=set(os.path.normpath(os.path.abspath(p)) for p in (exclude or [])) + cache=(os.path.join(tempfile.gettempdir(), + "cardlint_keyfreq_"+hashlib.md5(corpus.encode()).hexdigest()[:8]+".json") + if not exclude else None) + if cache: + try: + if os.path.exists(cache) and (os.path.getmtime(cache) > __import__("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() + full=os.path.join(root,fn) + if exclude and os.path.normpath(os.path.abspath(full)) in exclude: continue + try: txt=open(full,encoding="utf-8",errors="ignore").read() except Exception: continue for k in re.findall(r"([A-Za-z][A-Za-z0-9]*)\$",txt): freq[k]=freq.get(k,0)+1 - try: json.dump(freq,open(cache,"w",encoding="utf-8")) - except Exception: pass + if cache: + try: json.dump(freq,open(cache,"w",encoding="utf-8")) + except Exception: pass return freq def lint(path, freq=None): From d810e19ef1272bc0ab898c105de1f2e4f6b7ff4d Mon Sep 17 00:00:00 2001 From: MostCromulent <201167372+MostCromulent@users.noreply.github.com> Date: Sat, 13 Jun 2026 10:24:12 +0930 Subject: [PATCH 3/8] Subtract reviewed cards' params from corpus frequency The previous path-based corpus exclusion didn't match the materialized files in CI, so a reviewed card's own typo'd/made-up params were still self-counted as known and KEY-TYPO / UNKNOWN-KEY stayed silent. Read each reviewed card and subtract its param counts from the full corpus frequency instead -- robust to how paths resolve, since it reuses the same file paths the linter already reads. --- .github/scripts/card_script_review.py | 20 ++++++++++++--- .github/scripts/cardlint.py | 35 +++++++++------------------ 2 files changed, 27 insertions(+), 28 deletions(-) diff --git a/.github/scripts/card_script_review.py b/.github/scripts/card_script_review.py index cc0364bb31ad..43680a11697c 100644 --- a/.github/scripts/card_script_review.py +++ b/.github/scripts/card_script_review.py @@ -28,7 +28,8 @@ (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, contextlib, urllib.parse, urllib.request, urllib.error +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) @@ -294,9 +295,20 @@ def main(): 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("\\", "/")] - # Exclude the cards under review from the reference corpus so their own params - # aren't self-counted as "known" (see cardlint.key_freq). - freq = cardlint.key_freq(cardlint.find_corpus(), exclude=[p for p in cards if 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(): + 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 diff --git a/.github/scripts/cardlint.py b/.github/scripts/cardlint.py index 5c24e6d95386..00d47b780011 100644 --- a/.github/scripts/cardlint.py +++ b/.github/scripts/cardlint.py @@ -74,38 +74,25 @@ def find_corpus(): d=nd return None -def key_freq(corpus, exclude=None): - """Frequency of every `key$` token across the corpus (cached 7 days in temp). - - `exclude` is a set of file paths to leave OUT of the count -- pass the cards - under review so their own params aren't self-counted as "known" (otherwise a - typo'd / made-up param present only in the reviewed card looks legitimate and - KEY-TYPO / UNKNOWN-KEY never fire). The cache is bypassed when excluding, since - the exclusion set varies per run. - """ +def key_freq(corpus): + """Frequency of every `key$` token across the corpus (cached 7 days in temp).""" if not corpus or not os.path.isdir(corpus): return None - exclude=set(os.path.normpath(os.path.abspath(p)) for p in (exclude or [])) - cache=(os.path.join(tempfile.gettempdir(), - "cardlint_keyfreq_"+hashlib.md5(corpus.encode()).hexdigest()[:8]+".json") - if not exclude else None) - if cache: - try: - if os.path.exists(cache) and (os.path.getmtime(cache) > __import__("time").time()-7*86400): - return json.load(open(cache,encoding="utf-8")) - except Exception: pass + 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): + 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 - full=os.path.join(root,fn) - if exclude and os.path.normpath(os.path.abspath(full)) in exclude: continue - try: txt=open(full,encoding="utf-8",errors="ignore").read() + 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): freq[k]=freq.get(k,0)+1 - if cache: - try: json.dump(freq,open(cache,"w",encoding="utf-8")) - except Exception: pass + try: json.dump(freq,open(cache,"w",encoding="utf-8")) + except Exception: pass return freq def lint(path, freq=None): From 31da7fcbf6466729108426d99828886b1bc872f1 Mon Sep 17 00:00:00 2001 From: MostCromulent <201167372+MostCromulent@users.noreply.github.com> Date: Sat, 13 Jun 2026 10:29:26 +0930 Subject: [PATCH 4/8] TEMP diagnostics for freq subtraction (to be reverted) --- .github/scripts/card_script_review.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.github/scripts/card_script_review.py b/.github/scripts/card_script_review.py index 43680a11697c..a7a71b7561a6 100644 --- a/.github/scripts/card_script_review.py +++ b/.github/scripts/card_script_review.py @@ -295,6 +295,13 @@ def main(): 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("\\", "/")] + print(f"[dbg] cwd={os.getcwd()} corpus={cardlint.find_corpus()}", file=sys.stderr) + print(f"[dbg] cards={cards}", file=sys.stderr) + for p in cards: + print(f"[dbg] exists={os.path.exists(p)} {p}", file=sys.stderr) + _f0 = dict(cardlint.key_freq(cardlint.find_corpus()) or {}) + print(f"[dbg] pre-subtract freq ValidTgt={_f0.get('ValidTgt')} Foozle={_f0.get('Foozle')}", file=sys.stderr) + # 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 @@ -309,6 +316,7 @@ def main(): freq[k] -= n if freq[k] <= 0: del freq[k] + print(f"[dbg] post-subtract freq ValidTgt={freq.get('ValidTgt')} Foozle={freq.get('Foozle')}", file=sys.stderr) comments = [] for path in cards: if not os.path.exists(path): # deleted in the PR From 9b8904baaa3a4d13b9e1766a365f945c5998f8f3 Mon Sep 17 00:00:00 2001 From: MostCromulent <201167372+MostCromulent@users.noreply.github.com> Date: Sat, 13 Jun 2026 10:32:15 +0930 Subject: [PATCH 5/8] Materialize the last changed card file (trailing-newline read fix) changed_files.txt is written with join('\n') (no trailing newline), so the materialize loop's 'while read' dropped the final entry -- the alphabetically last card was never written and so never linted. Use 'read -r f || [ -n "$f" ]' so the last line is processed. Also removes temporary diagnostics. --- .github/scripts/card_script_review.py | 8 -------- .github/workflows/card-script-review.yml | 4 +++- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/.github/scripts/card_script_review.py b/.github/scripts/card_script_review.py index a7a71b7561a6..43680a11697c 100644 --- a/.github/scripts/card_script_review.py +++ b/.github/scripts/card_script_review.py @@ -295,13 +295,6 @@ def main(): 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("\\", "/")] - print(f"[dbg] cwd={os.getcwd()} corpus={cardlint.find_corpus()}", file=sys.stderr) - print(f"[dbg] cards={cards}", file=sys.stderr) - for p in cards: - print(f"[dbg] exists={os.path.exists(p)} {p}", file=sys.stderr) - _f0 = dict(cardlint.key_freq(cardlint.find_corpus()) or {}) - print(f"[dbg] pre-subtract freq ValidTgt={_f0.get('ValidTgt')} Foozle={_f0.get('Foozle')}", file=sys.stderr) - # 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 @@ -316,7 +309,6 @@ def main(): freq[k] -= n if freq[k] <= 0: del freq[k] - print(f"[dbg] post-subtract freq ValidTgt={freq.get('ValidTgt')} Foozle={freq.get('Foozle')}", file=sys.stderr) comments = [] for path in cards: if not os.path.exists(path): # deleted in the PR diff --git a/.github/workflows/card-script-review.yml b/.github/workflows/card-script-review.yml index 5b3f663b9ca5..4a650323cf48 100644 --- a/.github/workflows/card-script-review.yml +++ b/.github/workflows/card-script-review.yml @@ -89,7 +89,9 @@ jobs: 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" - while IFS= read -r f; do + # `|| [ -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" From bf13906d93fdb4e9281f9cb31e542eea291fbf20 Mon Sep 17 00:00:00 2001 From: MostCromulent <201167372+MostCromulent@users.noreply.github.com> Date: Sun, 14 Jun 2026 07:11:54 +0930 Subject: [PATCH 6/8] 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) --- .github/scripts/card_script_review.py | 58 +++++- .github/scripts/cardlint.py | 13 +- .github/workflows/card-script-review.yml | 123 +++++++----- .github/workflows/test-build.yaml | 25 +++ .../forge/game/ability/CardScriptApiTest.java | 189 ++++++++++++++++++ 5 files changed, 341 insertions(+), 67 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 43680a11697c..432d3425491c 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. @@ -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,12 +297,42 @@ 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()] - 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 @@ -301,18 +340,14 @@ def main(): # 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 diff --git a/.github/scripts/cardlint.py b/.github/scripts/cardlint.py index 00d47b780011..c664b916207c 100644 --- a/.github/scripts/cardlint.py +++ b/.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 diff --git a/.github/workflows/card-script-review.yml b/.github/workflows/card-script-review.yml index 4a650323cf48..7a387b06e130 100644 --- a/.github/workflows/card-script-review.yml +++ b/.github/workflows/card-script-review.yml @@ -1,79 +1,83 @@ -# 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; @@ -81,16 +85,26 @@ 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: 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")" @@ -98,16 +112,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 - 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). diff --git a/.github/workflows/test-build.yaml b/.github/workflows/test-build.yaml index c293c0741196..7c4420650d78 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 000000000000..5ff37f078d5d --- /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 3d14e4456e44b2a295a0c9b0a5362056b56fc91b Mon Sep 17 00:00:00 2001 From: MostCromulent <201167372+MostCromulent@users.noreply.github.com> Date: Sun, 14 Jun 2026 07:14:47 +0930 Subject: [PATCH 7/8] Demo: deliberately broken cards for card-script review (do not merge) Exercises all three review sources: engine API legality and sub-ability ref checking (zzz_review_demo), the deterministic linter (duplicate param), and the Scryfall frame check (a wrong mana cost on a real card). --- forge-gui/res/cardsfolder/l/lightning_bolt.txt | 2 +- forge-gui/res/cardsfolder/z/zzz_review_demo.txt | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 forge-gui/res/cardsfolder/z/zzz_review_demo.txt diff --git a/forge-gui/res/cardsfolder/l/lightning_bolt.txt b/forge-gui/res/cardsfolder/l/lightning_bolt.txt index dbc619a2f91d..a176ff13920a 100644 --- a/forge-gui/res/cardsfolder/l/lightning_bolt.txt +++ b/forge-gui/res/cardsfolder/l/lightning_bolt.txt @@ -1,5 +1,5 @@ Name:Lightning Bolt -ManaCost:R +ManaCost:1 R Types:Instant A:SP$ DealDamage | ValidTgts$ Any | NumDmg$ 3 | SpellDescription$ CARDNAME deals 3 damage to any target. Oracle:Lightning Bolt deals 3 damage to any target. diff --git a/forge-gui/res/cardsfolder/z/zzz_review_demo.txt b/forge-gui/res/cardsfolder/z/zzz_review_demo.txt new file mode 100644 index 000000000000..169e441d2b24 --- /dev/null +++ b/forge-gui/res/cardsfolder/z/zzz_review_demo.txt @@ -0,0 +1,7 @@ +Name:Zzz Review Demo +ManaCost:R +Types:Instant +A:SP$ DealDamge | ValidTgts$ Any | NumDmg$ 3 | NumDmg$ 4 | SpellDescription$ Deal damage. +T:Mode$ Bogus | Execute$ TrigGo | TriggerDescription$ test +SVar:TrigGo:DB$ Pump | WinSubAbility$ NopeAbility +Oracle:test From 08e04b7acedaaba1c7be05dd5e459ed69ebed25e Mon Sep 17 00:00:00 2001 From: MostCromulent <201167372+MostCromulent@users.noreply.github.com> Date: Sun, 14 Jun 2026 07:27:36 +0930 Subject: [PATCH 8/8] re-trigger CI