Repair base-set boundaries and playable table contracts - #3
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| self.send_header("content-type", "image/png") | ||
| self.end_headers() | ||
| self.wfile.write(open(png, "rb").read()) | ||
| with open(png, "rb") as fh: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
https://github.com/The-Interdependency/scared-sacred/blob/1da762feabee3eaeb69a65cecfcd0d4ac7d78fd2/engine/arcana_agendas_v1.py#L196-L197
The card is defined as granting immunity for one round (base-game/arcana_v02.md:15-16), but adding players charges makes immunity last until that many secret effects happen. If the following round contains fewer secret effects, the shield persists into later rounds; conversely, multiple delayed effects becoming eligible together can consume several charges in one beat. Track the protected machine beats or expire the shield at the rotation boundary instead of counting effects.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| working-directory: render | ||
| env: | ||
| PYTHONPATH: ../engine | ||
| POLITICS_BOTS: null |
There was a problem hiding this comment.
In YAML, this unquoted value is a null scalar rather than the literal string "null", while Table selects NullPlayer only when os.environ.get("POLITICS_BOTS") == "null". Consequently the table job either receives an empty value and runs the noisy-bot branch or fails workflow validation, so it does not exercise the intended deterministic null-bot context.
Useful? React with 👍 / 👎.
| if self.path == "/react": | ||
| ok, choice = reaction_from_request(TABLE, req) | ||
| if not ok: | ||
| self._json({"ok": False}, 400 if TABLE.human.reaction else 409) | ||
| return | ||
| TABLE.human.reaction_inbox.put(choice) |
There was a problem hiding this comment.
Because this is served by ThreadingHTTPServer, two rapid or concurrent /react requests can both observe the same non-None reaction and enqueue choices before the worker clears it. The first choice resolves the current window, but the second remains in reaction_inbox and is consumed automatically by the next reaction window, potentially passing or playing and burning cards without a new user decision. Claim or clear the pending window atomically before enqueueing.
Useful? React with 👍 / 👎.
| burns = [hand[i] for i in burn_indexes] | ||
| plays = pr.TurnPlays(discard_cards=burns) | ||
| if kind == "static": | ||
| card["side"] = "player" | ||
| plays.static = card | ||
| elif kind == "action": | ||
| card["declared_target"] = "shared" | ||
| plays.actions = [card] |
There was a problem hiding this comment.
The request records physical burn cards but never assigns their count to the selected action's burn field, which is the only value RulesV1.legal uses to lower activation energy. Since the browser submits at most one action, these burns cannot be tempo payments either; every card marked through the UI is therefore discarded without affecting legality, and an action that required the advertised reach discount is still rejected.
Useful? React with 👍 / 👎.
| if ev[0] == "action": | ||
| r_by_pid[ev[1]] = r_by_pid.get(ev[1], 0) + 1 | ||
| resolved_r = ev[4] if len(ev) > 4 else 0 | ||
| r_by_pid[ev[1]] = r_by_pid.get(ev[1], 0) + resolved_r |
There was a problem hiding this comment.
The newly playable reflex path contributes reflex_card["r"] to the engine but logs a reflex event rather than an action event (politics_runner.py:244-247). This verifier scans only actions, so a player holding THE ORGANIZER receives no credit for repair supplied through reflexes and can incorrectly lose the agenda despite having the greatest attributed R.
Useful? React with 👍 / 👎.
| # id: agendas_organizer_counts_r | ||
| # behavior: the organizer agenda holds only for the player with the | ||
| # strictly greatest attributed r tally | ||
| # behavior: organizer uses resolved attributed R, not action count | ||
| # id: arcana_dealt_ownership | ||
| # behavior: a player may resolve only the arcanum dealt to that seat |
There was a problem hiding this comment.
This commit adds and changes module-local contracts and checks, but the committed canonical collection point scared-sacred_msdmd.ts was not regenerated: it omits arcana_dealt_ownership, cards_se_shield_consumed, runner_attributed_r_log, the new renderer contracts and their checks, and still contains the previous Organizer behavior. Running the repository's msdmd.collect generator shows those missing declarations and edges, so collection consumers and visualizers receive stale contract coverage.
Useful? React with 👍 / 👎.
| candidates = [i for i, card in enumerate(hand) | ||
| if card.get("reflex") | ||
| and self.rules.reflex_legal(state, card, mcard)] |
There was a problem hiding this comment.
The new reaction UI offers WELS' SPEECH on M15, but WeimarMachine.next_card evaluates the M15 hinge before the runner opens _reaction_window, and that window only adds the card's R without applying the m_bonus handled by RulesV1.interference. Consequently the card's documented M+2 is omitted and the hinge threshold has already been decided, so a human reflex cannot provide its intended last-moment effect on the Enabling Act.
Useful? React with 👍 / 👎.
| burn_indexes = req.get("burns", []) | ||
| if not isinstance(burn_indexes, list): | ||
| return None | ||
| if len(set(burn_indexes)) != len(burn_indexes): | ||
| return None | ||
| if any(not isinstance(i, int) or not 0 <= i < len(hand) | ||
| for i in burn_indexes): |
There was a problem hiding this comment.
The /act payload is untrusted JSON, but set(burn_indexes) runs before verifying that every entry is an integer. A request such as {"burns":[[]]} therefore raises TypeError in the request thread instead of returning the contract's 400 response for an invalid burn index. Check element types first or deduplicate only after validation.
Useful? React with 👍 / 👎.
2 participants
Achieved
expansions/scared-sacred/intobase-game/sets/fifty-three-days/without rewriting their blobsexpansions/scared-sacred/for the actual SCARED SACRED expansion__pycache__bytecode and ignore generated Python stateRemaining hmmm
noop_extra_card, M35noop_window, Camarilla rotation buff, and Book Fires choice targeting remain explicitly incomplete in existing canon/code; this PR does not invent their missing rulingsawaitingGates
CI runs source compilation plus all engine and table unittest suites on Python 3.12.