chore(PR-7320): port random discard cost to current main

Resolve maintainer-caused test-census conflict, correct current review evidence, and retain only the reachable replacement-delivery continuation.

Co-authored-by: Jacob Woodson <38709105+JacobWoodson@users.noreply.github.com>

Author: matthewevans

.claude/skills/engine-implementer/SKILL.md

@@ -70,7 +70,7 @@ Spawn a `general-purpose` agent and instruct it to invoke `/engine-planner`. The
 
 **Spawn inputs:** task description; in-scope file/subsystem hints; any prior reviewer findings (none on first round).
 
-Do not author or edit the plan in this thread. If the returned plan is missing sections or is superficial, send the same inputs plus an explicit "missing sections" note to a **fresh** planning agent — do not patch it yourself.
+Do not author or edit the plan in this thread — surgical-fix mode (below) is the one exception, and only under its three measured conditions. If the returned plan is missing sections or is superficial, send the same inputs plus an explicit "missing sections" note to a **fresh** planning agent — do not patch it yourself.
 
 ### Step 2 — Review the plan until clean (unbounded loop)
 
@@ -88,6 +88,32 @@ If the reviewer returns gaps, spawn a **fresh** planning agent (Step 1 inputs pl
 
 Each review must run in a fresh agent context — never reuse the previous reviewer's context.
 
+#### Surgical-fix mode — when the design is settled and the findings are spot drift
+
+The loop above assumes findings move the **design**. Once they stop doing that, re-running it makes the artifact worse: a fresh planner rewrites prose to absorb each finding, prose is where spot findings live, so every round manufactures the next round's findings.
+
+**When all three hold, switch modes** — measure them, do not judge them:
+
+1. The design is unchanged for ≥2 consecutive rounds (compare the named entries themselves — which steps, sub-steps, enum variants, and call sites each round names, because a 1:1 substitution holds every count constant; **not** a count and **not** line count; an in-place rewrite that preserves every name survives this comparison and is caught only by the whole-artifact re-review below).
+2. The last round's findings are all **spot** — a stale number, a stale coordinate, a claim contradicted by a neighbouring section, a missing restatement of a control the plan already specifies, a sentence never swept. None changes what the implementation does.
+3. Each finding names a coordinate **and** its replacement text. If any finding requires *deciding* something, it is a design finding: stay in the loop.
+
+**Do not add a fourth condition based on falling churn.** Round-over-round churn shrinks while a loop turns unproductive: smaller repairs to a growing record. It measures edit size, not convergence, and gating on it blocks the switch precisely when the switch is warranted.
+
+**The corroborating signal, if you want one, is the fraction of a round's findings whose defect originated in the *previous* round's repairs.** It climbs as the loop starts feeding on itself, but not monotonically — so treat a high fraction as evidence for the switch, never as the trigger.
+
+**In surgical-fix mode the orchestrator applies the findings itself**, as check-and-replace edits — the one narrow exception to "the orchestrator never authors content." It is *applying* adjudicated text, not authoring; the moment a fix needs a decision, dispatch a planner instead. Requirements:
+
+- **Two-sided verification per edit:** before the edit, the quoted old string is present at the finding's named coordinate — a quote that is not there is a stale coordinate, not an applicable fix; after the edit, the text the replacement adds is present exactly once and sits where the old string was, and the old string is absent — except that when the replacement contains the old string, that string survives by construction and the added text is the sole gate; count occurrences, not matching lines, 1:1 per fragment, not a lucky aggregate.
+- **State the sweep's boundary.** A changelog entry that quotes the struck text will match your own grep for it. Population, predicate, scan direction, and whether the matched line counts — write them down; every enumeration defect is an unstated predicate rather than a bad measurement.
+- **Fix the neighbours the fix breaks.** A finding's repair frequently contradicts a section that classified the old form. Sweep by mechanism, not by coordinate.
+- **Then re-review the WHOLE artifact**, fresh context — not just the repaired sections, per `$bug-triage`'s targeted-re-review rule. Repeat apply → whole-artifact re-review until a round returns zero gaps; any finding that requires *deciding* something ends surgical mode and returns to the unbounded loop above. Surgical mode replaces the planner-rewrite rounds, never the final independent check.
+- **Record the mode switch, its three measurements, the spot-vs-design classification of each round's findings, each attempted edit's two-sided verification result (pass or fail) and sweep boundary, and why the mode ends** in `<git-common-dir>/engine-implementer-runs/<run-id>/surgical-mode-switch`, never in the plan text the fresh re-reviewer and the executor read — recording it there hands the one remaining independent check a prior verdict. It is a process record of this loop, not provenance: it carries no candidate identity and duplicates no receipt field, so the canonical-receipt rule above does not reach it. Append one numbered entry per round, never overwrite: ending surgical mode and re-entering it later continues the same numbered sequence, and clobbering an earlier entry loses the exit that entry recorded. Every exit is then auditable rather than asserted. Each entry carries that round's classification, edit results, and sweep boundaries; only a round that enters the mode records the switch and its three measurements, and only a round that ends the mode records why.
+
+**This does not contradict `$bug-triage`'s fixpoint gate.** That gate requires whole-plan re-review because *"revisions routinely INTRODUCE new gaps in untouched-looking areas"* — planner **rewrites** do. A check-and-replace at a named coordinate does not rewrite, which is why it is the safe tool once the design has stopped moving. `$review-engine-plan` ends its loop with *"or the caller stops the process"* and states no criteria; this section is those criteria, and it lives here because the orchestrator is that caller.
+
+This is not a licence for "two rounds and ship". The unbounded loop remains the default and the burden of proof is on leaving it: no measurement, no switch. Surgical mode is scoped to this Step 2 plan-review loop only — Step 6's implementation-review loop never uses it, because there the artifact is a committed candidate that only an executor may edit under the frozen-scope and receipt contract.
+
 ### Step 3 — Dispatch implementation
 
 Spawn the `engine-implementation-executor` agent.
@@ -129,7 +155,7 @@ Every round keeps the original `BASE_SHA` and frozen scope paths. Every checkpoi
 
 ### Step 7 — Final acceptance
 
-Accept only when the plan-review loop is clean, the reviewer emitted a matching external receipt SHA256, final acceptance independently revalidates the receipt and every recorded artifact, parser evidence and completion checks pass, and the fresh implementation review returns zero findings. The receipt validation/hash are acceptance evidence, never completion-check rows or receipt artifacts. Immediately run `git -C "$IMPLEMENTATION_WORKTREE" rev-parse HEAD` and compare its output to `CANDIDATE_SHA`; if it differs, the review is stale and the current head must repeat the checkpoint-to-review sequence. Do not treat review of an ancestor as review of current work.
+Accept only when the plan-review loop is clean, the reviewer emitted a matching external receipt SHA256, final acceptance independently revalidates the receipt and every recorded artifact, parser evidence and completion checks pass, every surgical-fix mode round has the complete numbered entry Step 2 requires, and the fresh implementation review returns zero findings. The receipt validation/hash and those entries are acceptance evidence, never completion-check rows or receipt artifacts. Immediately run `git -C "$IMPLEMENTATION_WORKTREE" rev-parse HEAD` and compare its output to `CANDIDATE_SHA`; if it differs, the review is stale and the current head must repeat the checkpoint-to-review sequence. Do not treat review of an ancestor as review of current work.
 
 ### Post-acceptance PR handoff (non-gating)
 
@@ -152,7 +178,7 @@ If later work invalidates the approved plan or architecture, return to plan revi
 
 Return after final acceptance:
 
-1. Plan-review rounds (count) and final clean result.
+1. Plan-review rounds (count), whether surgical-fix mode was used, and final clean result.
 2. What changed, grouped by subsystem and file.
 3. Key architectural decisions.
 4. `BASE_SHA`, accepted `CANDIDATE_SHA`, frozen scope paths, and run-artifact root.

.gitignore

@@ -56,6 +56,13 @@ client/public/parser-warning-patterns.json
 # Atomic-write staging files from gen-card-data.sh (cleaned on normal exit,
 # but may linger after SIGKILL / power loss).
 client/public/*.tmp
+# The same, for callers that stage through mktemp and so carry a random tail
+# after `.tmp` (gen-scryfall-token-images.sh). The template cannot be
+# `.XXXXXX.tmp` instead: BSD/macOS mktemp rejects anything after the X run.
+client/public/*.tmp.*
+# scryfall_finalize_download stages its rename diagnostic beside the target;
+# gen-scryfall-token-images.sh is the first caller to aim it at a tracked dir.
+client/public/*.mv-error.*
 
 # Coverage
 client/coverage/

Tiltfile

@@ -22,6 +22,26 @@ enabled = config.parse().get('enable', [])
 # restarts the watching resources mid-build.
 TMP_IGNORE = ['**/*.tmp.*']
 
+# probe-pin isolates through `unshare --map-root-user --mount` (util-linux) and runs its target
+# under `timeout` (GNU coreutils). macOS ships neither, and there is no Darwin equivalent of an
+# unprivileged mount namespace to port to — so both probe-pin resources abort on a Darwin host
+# no matter what the tree contains. Gate their auto_init rather than let them boot straight into
+# a permanent red: a gate that is red on every change teaches everyone to stop reading the
+# colour, which costs more than the gate earns. They stay VISIBLE and clickable, like every
+# other opt-in resource here, so the refusal is still one click away when someone wants to see
+# it. `os.name` is consulted FIRST and short-circuits, so a Windows host never reaches `uname`
+# — which it does not ship, and which would fail Tiltfile LOAD rather than one resource.
+IS_LINUX = (
+    os.name == 'posix'
+    and str(local('uname -s', quiet = True, echo_off = True)).strip() == 'Linux'
+)
+
+# auto_init alone would NOT be enough: it governs only the STARTUP run, and the default
+# TRIGGER_MODE_AUTO re-runs a resource whenever its deps change. Both probe-pin resources watch
+# 'crates/probe-pin/', so off Linux the very next edit there would drag them back into the red
+# that auto_init just avoided. Off-Linux they must stop watching too, not merely stop booting.
+PROBE_PIN_TRIGGER = TRIGGER_MODE_AUTO if IS_LINUX else TRIGGER_MODE_MANUAL
+
 # Must stay a SUPERSET of what `scripts/engine-source-hash.sh` hashes as the engine cache
 # key (src + data + build.rs + Cargo.toml). `data/` is `include_str!`d into the binary and
 # `build.rs`/`Cargo.toml` change what gets compiled, so a change to any of them changes the
@@ -272,15 +292,22 @@ local_resource('probe-pin-check',
     # TMP_IGNORE is a FILENAME glob ('**/*.tmp.*') and does not match a tmp/ DIRECTORY, so the
     # Tier-2 tests' scratch writes under tests/fixtures/tmp/ would retrigger this resource.
     ignore = TMP_IGNORE + ['**/tmp/**'],
-    auto_init = 'lint' in enabled,
+    auto_init = 'lint' in enabled and IS_LINUX,
+    trigger_mode = PROBE_PIN_TRIGGER,
     allow_parallel = True,
     labels = ['lint'],
 )
 
 # The Tier-2 suite is #[ignore]d because GitHub's runners deny unprivileged user namespaces
 # (unshare -> /proc/self/uid_map EPERM), so GH CI never executes a real mount. THIS resource is
-# what keeps that honest: the local venue does have the capability, so Tier 2 runs here on every
-# change. Without it, "ignored in CI" quietly becomes "never run anywhere".
+# what keeps that honest: a LINUX local venue does have the capability, so Tier 2 runs there on
+# every change. Without it, "ignored in CI" quietly becomes "never run anywhere".
+#
+# On a non-Linux host, "never run anywhere" is not a hedge — it is the literal state. CI cannot
+# mount, and Darwin has no `unshare` to try, so Tier 2's claims are carried entirely by whatever
+# Linux venue last executed them. IS_LINUX only stops this resource auto-starting into a red it
+# can never clear; it does not make that gap smaller. The tests are deliberately NOT cfg'd out,
+# so a manual run here still prints probe-pin's own named refusal rather than a zero-test green.
 #
 # `-- --ignored` runs ONLY the ignored tests, which is exactly this suite. A non-zero exit turns
 # the resource red like any other gate — a real Tier-2 gate, not a fire-and-forget reporter.
@@ -298,7 +325,8 @@ local_resource('probe-pin-e2e',
            'CARGO_TARGET_DIR=target/probe-pin-e2e cargo test -p probe-pin --test isolation_e2e -- --ignored'],
     deps = ['crates/probe-pin/'],
     ignore = TMP_IGNORE + ['**/tmp/**'],
-    auto_init = 'lint' in enabled,
+    auto_init = 'lint' in enabled and IS_LINUX,
+    trigger_mode = PROBE_PIN_TRIGGER,
     allow_parallel = True,
     labels = ['lint'],
 )

client/src/adapter/__tests__/draftPodAdapter.test.ts

@@ -48,6 +48,7 @@ const mockHostOnEvent = vi.fn((_handler: (event: Record<string, unknown>) => voi
 const mockHostInitialize = vi.fn(async () => {});
 const mockHostStartDraft = vi.fn(async () => {});
 const mockHostSubmitHostPick = vi.fn(async () => mockView("Drafting"));
+const mockHostSubmitHostPickWithDraftEffect = vi.fn(async () => mockView("Drafting"));
 const mockHostSubmitHostDeck = vi.fn(async () => mockView("Deckbuilding"));
 const mockHostGetHostView = vi.fn(async () => mockView("Lobby"));
 const mockHostKickPlayer = vi.fn();
@@ -64,6 +65,7 @@ vi.mock("../p2p-draft-host", () => ({
       initialize: mockHostInitialize,
       startDraft: mockHostStartDraft,
       submitHostPick: mockHostSubmitHostPick,
+      submitHostPickWithDraftEffect: mockHostSubmitHostPickWithDraftEffect,
       submitHostDeck: mockHostSubmitHostDeck,
       getHostView: mockHostGetHostView,
       kickPlayer: mockHostKickPlayer,
@@ -83,6 +85,7 @@ vi.mock("../p2p-draft-host", () => ({
 const mockGuestOnEvent = vi.fn((_handler: (event: Record<string, unknown>) => void) => vi.fn());
 const mockGuestInitialize = vi.fn(async () => {});
 const mockGuestSubmitPick = vi.fn(async () => {});
+const mockGuestSubmitPickWithDraftEffect = vi.fn(async () => {});
 const mockGuestSubmitDeck = vi.fn(async () => {});
 const mockGuestLeave = vi.fn(async () => {});
 
@@ -92,6 +95,7 @@ vi.mock("../p2p-draft-guest", () => ({
       onEvent: mockGuestOnEvent,
       initialize: mockGuestInitialize,
       submitPick: mockGuestSubmitPick,
+      submitPickWithDraftEffect: mockGuestSubmitPickWithDraftEffect,
       submitDeck: mockGuestSubmitDeck,
       leave: mockGuestLeave,
       view: null,
@@ -112,6 +116,7 @@ function mockView(status: string): DraftPlayerView {
     pass_direction: "Left",
     current_pack: null,
     pool: [],
+    draft_effects: [],
     pool_groups: {
       color_groups: [],
       type_groups: [],
@@ -294,6 +299,21 @@ describe("DraftPodHostAdapter", () => {
     expect(view.status).toBe("Drafting");
   });
 
+  it("delegates draft-effect picks and returns view", async () => {
+    await adapter.initialize({
+      poolInput: { type: "Set", data: { set_pool_json: "{}" } },
+      kind: "Premier",
+      podSize: 8,
+      hostDisplayName: "Host",
+      tournamentFormat: "Swiss",
+      podPolicy: "Competitive",
+    });
+
+    const view = await adapter.submitPickWithDraftEffect("cogwork-1", ["card-1", "card-2"]);
+    expect(mockHostSubmitHostPickWithDraftEffect).toHaveBeenCalledWith("cogwork-1", ["card-1", "card-2"]);
+    expect(view.status).toBe("Drafting");
+  });
+
   it("delegates submitDeck and returns view", async () => {
     await adapter.initialize({
       poolInput: { type: "Set", data: { set_pool_json: "{}" } },
@@ -500,6 +520,13 @@ describe("DraftPodGuestAdapter", () => {
     expect(mockGuestSubmitPick).toHaveBeenCalledWith("card-456");
   });
 
+  it("delegates draft-effect picks to P2PDraftGuest", async () => {
+    await adapter.initialize({ roomCode: "ABCDE", displayName: "Alice" });
+
+    await adapter.submitPickWithDraftEffect("cogwork-1", ["card-1", "card-2"]);
+    expect(mockGuestSubmitPickWithDraftEffect).toHaveBeenCalledWith("cogwork-1", ["card-1", "card-2"]);
+  });
+
   it("delegates submitDeck to P2PDraftGuest", async () => {
     await adapter.initialize({ roomCode: "ABCDE", displayName: "Alice" });
 

client/src/adapter/__tests__/p2p-adapter-multiplayer.test.ts

@@ -581,17 +581,34 @@ describe("P2PHostAdapter — 3-4p multiplayer", () => {
     ).toThrow("P2P supports 2-6 players");
   });
 
-  it("enables multiplayer-mode enforcement on the engine at init time", async () => {
-    // P2PHostAdapter owns an authoritative WASM engine locally; flipping
-    // the engine's multiplayer flag during initialize() ensures any stray
-    // restore_game_state call is refused in the Rust layer.
+  it("enables multiplayer-mode enforcement at game start, not at lobby open", async () => {
+    // The engine's multiplayer flag is process-wide and nothing ever clears it,
+    // so an open host lobby must not set it — it is claimed only when the host
+    // actually takes the engine, on the line before `initializeGame`, which is
+    // where `initialize_debug_permissions` reads it.
     const { adapter } = makeHost(2);
     expect(mockSetMultiplayerMode).not.toHaveBeenCalled();
 
     await adapter.initialize();
 
+    expect(mockSetMultiplayerMode).not.toHaveBeenCalled();
+
+    await adapter.applySeatMutation({
+      type: "SetKind",
+      data: {
+        seatIndex: 1,
+        kind: {
+          type: "Ai",
+          data: { difficulty: "Medium", deck: { type: "Random" } },
+        },
+      },
+    });
+    await adapter.initializeGame();
+
     expect(mockSetMultiplayerMode).toHaveBeenCalledTimes(1);
     expect(mockSetMultiplayerMode).toHaveBeenCalledWith(true);
+    expect(mockSetMultiplayerMode.mock.invocationCallOrder[0])
+      .toBeLessThan(mockInitializeGame.mock.invocationCallOrder[0]);
   });
 
   it("does not reinitialize the host during the lobby-to-game handoff", async () => {
@@ -601,7 +618,7 @@ describe("P2PHostAdapter — 3-4p multiplayer", () => {
     await adapter.initialize();
 
     expect(mockInitialize).toHaveBeenCalledTimes(1);
-    expect(mockSetMultiplayerMode).toHaveBeenCalledTimes(1);
+    expect(mockSetMultiplayerMode).not.toHaveBeenCalled();
   });
 
   it("fences a stale host when a same-session resume claims a new incarnation", async () => {