Demo: card-script review (new poster) — do not merge#24
Open
MostCromulent wants to merge 8 commits into
Open
Conversation
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) <noreply@anthropic.com>
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).
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.
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.
Add a pure-advisory test (CardScriptApiTest) that checks each card against
the engine's own definitions and writes findings for the review workflow
to post, never failing the build:
- ability/trigger/replacement API names via ApiType, TriggerType and
ReplacementType (the same smartValueOf the engine runs at card load),
catching a typo'd API such as DB$ DealDamge that the linter cannot;
- sub-ability params in AbilityFactory.additionalAbilityKeys must point
at a defined SVar, closing a gap where the linter only knew a handful
of these keys. cardlint's REF_KEYS drops the now-engine-checked keys.
The analysis runs inside the build (mvn test). A pull_request build has a
read-only token on fork PRs and so cannot comment; the review workflow is
therefore reworked from pull_request_target into a workflow_run poster.
test-build uploads the findings and the PR number as an artifact, and the
poster downloads it, runs the linter and Scryfall frame check, and posts
the merged, deduped comments on the changed lines.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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).
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -1,5 +1,5 @@ | |||
| Name:Lightning Bolt | |||
| ManaCost:R | |||
| ManaCost:1 R | |||
| Name:Zzz Review Demo | ||
| ManaCost:R | ||
| Types:Instant | ||
| A:SP$ DealDamge | ValidTgts$ Any | NumDmg$ 3 | NumDmg$ 4 | SpellDescription$ Deal damage. |
There was a problem hiding this comment.
duplicate NumDmg$ → engine keeps only the last
| Name:Zzz Review Demo | ||
| ManaCost:R | ||
| Types:Instant | ||
| A:SP$ DealDamge | ValidTgts$ Any | NumDmg$ 3 | NumDmg$ 4 | SpellDescription$ Deal damage. |
There was a problem hiding this comment.
SP$ DealDamge → unknown API
| ManaCost:R | ||
| Types:Instant | ||
| A:SP$ DealDamge | ValidTgts$ Any | NumDmg$ 3 | NumDmg$ 4 | SpellDescription$ Deal damage. | ||
| T:Mode$ Bogus | Execute$ TrigGo | TriggerDescription$ test |
There was a problem hiding this comment.
Mode$ Bogus → unknown trigger type
| 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 |
There was a problem hiding this comment.
WinSubAbility$ NopeAbility → undefined SVar (clause dropped)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Throwaway demo to validate the workflow_run poster end-to-end. Do not merge.