diff --git a/.github/scripts/card_script_review.py b/.github/scripts/card_script_review.py new file mode 100644 index 000000000000..432d3425491c --- /dev/null +++ b/.github/scripts/card_script_review.py @@ -0,0 +1,372 @@ +#!/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(). + + 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. +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}`" + 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 + + +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 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(): + 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(args[0], encoding="utf-8") if l.strip()] + cards = [p for p in paths if p.endswith(".txt") + and "cardsfolder" in p.replace("\\", "/") and os.path.exists(p)] + + # The workflow materializes the PR's cards into the corpus, so a freshly + # computed key_freq counts them too. Subtract each reviewed card's own param + # counts so a typo'd or made-up param present only in the reviewed card isn't + # self-counted as "known" (which would silence KEY-TYPO / UNKNOWN-KEY). + freq = dict(cardlint.key_freq(cardlint.find_corpus()) or {}) + for path in cards: + 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}) + + 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 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/scripts/cardlint.py b/.github/scripts/cardlint.py new file mode 100644 index 000000000000..c664b916207c --- /dev/null +++ b/.github/scripts/cardlint.py @@ -0,0 +1,231 @@ +#!/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} +# 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"} +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 000000000000..7a387b06e130 --- /dev/null +++ b/.github/workflows/card-script-review.yml @@ -0,0 +1,174 @@ +# 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. +# +# 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. +# +# 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: + workflow_run: + workflows: ["Test build"] + types: [completed] + +permissions: + contents: read + actions: read + pull-requests: write + +concurrency: + 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: + - name: Download review data from the build + uses: actions/download-artifact@v4 + with: + 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: 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 }); + 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 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; + 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 */ } + else { newLine++; } // context line + } + addedLines[f.filename] = lines; + } + fs.writeFileSync('diff_lines.json', JSON.stringify(addedLines)); + core.info(`${cards.length} changed card file(s)`); + + - 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: ${{ 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 without a trailing newline. + while IFS= read -r f || [ -n "$f" ]; do + [ -n "$f" ] || continue + mkdir -p "$(dirname "$f")" + git show "refs/remotes/pr/head:$f" > "$f" || echo "skip $f" + done < changed_files.txt + + - name: Run review checks + if: steps.pr.outputs.pull_number != '' + continue-on-error: true # advisory: a script hiccup must never fail the PR + env: + PYTHONIOENCODING: utf-8 # comments contain '→'; don't depend on runner locale + run: | + 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'); + 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 = Number(process.env.PULL_NUMBER); + const commit_id = process.env.HEAD_SHA; + + // 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). + 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)`); 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")); + } +} 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