From acc5a7b2a222d00bb8621f9b390a78de303449cb Mon Sep 17 00:00:00 2001 From: Phillip Tarrant Date: Sun, 12 Jul 2026 19:11:40 -0500 Subject: [PATCH] fix(npc): a move applies at most once per reply MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tag extractor returns every occurrence of a move tag and the validator is a pure membership test, so MoveApplier applied duplicates. A model repeating a tag N times therefore picked an amount in unary — the AI choosing a number, which is the §2 breach the bounded vocabulary exists to prevent. The currency fix (8e1238c) closed this for denominations only. Generalizing it to every move that names a thing covers real items too — three give_item(amulet) tags are one amulet, not three — and lets the currency special case be deleted rather than kept alongside a second mechanism. ID_MOVES is reused from MoveValidator instead of restated. Also caps adjust_disposition on the reply's NET swing. MAX_DELTA was clamped per tag, so three +15s moved standing by 45 — precisely the wholesale swing its own comment says it forbids. Applied in emission order, so a later become_hostile still floors what came before it. offer_quest and reveal needed no change: add_quest self-guards on has_quest and mark_revealed is idempotent. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01QYa9u7Kdxv5gX4AnwWexy8 --- client/scripts/npc/move_applier.gd | 34 +++++++---- client/tests/unit/test_move_applier.gd | 61 +++++++++++++++++-- .../2026-07-12-currency-copper-design.md | 12 ++-- 3 files changed, 86 insertions(+), 21 deletions(-) diff --git a/client/scripts/npc/move_applier.gd b/client/scripts/npc/move_applier.gd index d9e87f6..6ab94f1 100644 --- a/client/scripts/npc/move_applier.gd +++ b/client/scripts/npc/move_applier.gd @@ -11,20 +11,24 @@ const HOSTILE_FLOOR := -100 static func apply(moves: Array, game_state, canon_log, content_db, npc_id: String) -> void: - # A currency move (give_item/accept_item on a denomination) applies AT MOST - # ONCE per reply. Without this, a model repeating one tag N times would pick - # an arbitrary amount in unary — the AI choosing a number is a §2 breach. - # Real-item duplicates are unaffected; that stays bounded by gifts_given. - var currency_applied_this_reply := {} # "name(denom)" -> true + # ONE RULE: a move applies AT MOST ONCE PER REPLY. The tag extractor returns + # every occurrence and the validator is a membership test, so without this a + # model repeating a tag N times picks an amount in unary — the AI choosing a + # number, which is the §2 breach the bounded vocabulary exists to prevent. + # It is worth the same for real items: three give_item(amulet) tags are one + # amulet, not three. + var applied := {} # "name(id)" -> true + var dispo_total := 0 # the reply's NET disposition swing, capped at MAX_DELTA for move in moves: var name: String = move.get("name", "") var args: Array = move.get("args", []) - if name in ["give_item", "accept_item"] and not args.is_empty() \ - and Currency.is_currency(str(args[0])): - var currency_key := "%s(%s)" % [name, str(args[0])] - if currency_applied_this_reply.has(currency_key): + # MoveValidator.ID_MOVES is the one list of moves that name a thing — those + # are the ones a (name, id) key can dedupe. Reused, not restated. + if name in MoveValidator.ID_MOVES and not args.is_empty(): + var key := "%s(%s)" % [name, str(args[0])] + if applied.has(key): continue - currency_applied_this_reply[currency_key] = true + applied[key] = true match name: "offer_quest": var q: String = str(args[0]) @@ -40,9 +44,17 @@ static func apply(moves: Array, game_state, canon_log, content_db, npc_id: Strin "accept_item": game_state.take(str(args[0]), 1) "adjust_disposition": + # MAX_DELTA caps the reply's NET swing, not each tag — clamping per + # tag let three +15s move standing by 45, which is exactly the + # "wholesale swing" the constant is here to forbid. Applied inside + # the loop (not batched at the end) so emission order still holds + # and a later become_hostile floors what came before it. var delta: int = clampi(int(str(args[0])), -MAX_DELTA, MAX_DELTA) + var capped: int = clampi(dispo_total + delta, -MAX_DELTA, MAX_DELTA) + var allowed: int = capped - dispo_total + dispo_total = capped var cur: int = int(game_state.npc_dispositions.get(npc_id, 0)) - game_state.set_npc_disposition(npc_id, cur + delta) + game_state.set_npc_disposition(npc_id, cur + allowed) "become_hostile": game_state.set_npc_disposition(npc_id, HOSTILE_FLOOR) _: diff --git a/client/tests/unit/test_move_applier.gd b/client/tests/unit/test_move_applier.gd index 09e2176..d7ff074 100644 --- a/client/tests/unit/test_move_applier.gd +++ b/client/tests/unit/test_move_applier.gd @@ -144,18 +144,67 @@ func test_dedup_key_is_move_name_not_just_denomination(): assert_eq(gs.purse_copper, 0) -func test_repeated_non_currency_give_item_still_applies_each_time(): - # Pins the scope line: real-item duplicate handling is UNCHANGED by this fix - # (it stays bounded only by the cross-reply gifts_given gate, reported separately). +func test_repeated_give_item_applies_once_for_real_items_too(): var gs = GameState.new() MoveApplier.apply([_m("give_item", ["amulet"]), _m("give_item", ["amulet"]), _m("give_item", ["amulet"])], gs, CanonLog.new(), _content(), "fenn") - assert_eq(gs.inventory.get("amulet", 0), 3, "non-currency give_item is out of scope for this fix") + assert_eq(gs.inventory.get("amulet", 0), 1, "three tags, one amulet") -func test_repeated_non_currency_accept_item_still_applies_each_time(): +func test_repeated_accept_item_applies_once_for_real_items_too(): var gs = GameState.new() gs.add_item("worn_shortsword", 5) MoveApplier.apply([_m("accept_item", ["worn_shortsword"]), _m("accept_item", ["worn_shortsword"])], gs, CanonLog.new(), _content(), "fenn") - assert_eq(gs.inventory.get("worn_shortsword", 0), 3, "non-currency accept_item is out of scope for this fix") + assert_eq(gs.inventory.get("worn_shortsword", 0), 4, "two tags, one sword taken") + + +func test_different_real_items_in_one_reply_both_apply(): + var gs = GameState.new() + MoveApplier.apply([_m("give_item", ["amulet"]), _m("give_item", ["ring"])], + gs, CanonLog.new(), _content(), "fenn") + assert_eq(gs.inventory.get("amulet", 0), 1) + assert_eq(gs.inventory.get("ring", 0), 1) + + +func test_repeated_disposition_tags_cannot_exceed_the_per_reply_cap(): + # MAX_DELTA means "one line cannot swing standing wholesale" (§7 spirit). It was + # clamped PER TAG, so three +15 tags moved standing by 45. The cap is per REPLY. + var gs = GameState.new() + MoveApplier.apply([_m("adjust_disposition", ["+15"]), _m("adjust_disposition", ["+15"]), + _m("adjust_disposition", ["+15"])], gs, CanonLog.new(), _content(), "fenn") + assert_eq(gs.npc_dispositions.get("fenn", 0), MoveApplier.MAX_DELTA, "+45 is capped to +15") + + +func test_disposition_cap_applies_to_the_net_swing(): + var gs = GameState.new() + MoveApplier.apply([_m("adjust_disposition", ["+10"]), _m("adjust_disposition", ["+10"])], + gs, CanonLog.new(), _content(), "fenn") + assert_eq(gs.npc_dispositions.get("fenn", 0), 15, "+20 is capped to +15") + + +func test_disposition_tags_of_opposite_sign_net_out(): + # The cap is on the reply's NET swing, not on the tag count — so a walk-back + # inside one reply still lands where the arithmetic says. + var gs = GameState.new() + MoveApplier.apply([_m("adjust_disposition", ["+10"]), _m("adjust_disposition", ["-4"])], + gs, CanonLog.new(), _content(), "fenn") + assert_eq(gs.npc_dispositions.get("fenn", 0), 6) + + +func test_become_hostile_still_wins_after_a_disposition_tag(): + # Moves apply in emission order — the hostile floor must not be undone by + # batching the disposition arithmetic. + var gs = GameState.new() + MoveApplier.apply([_m("adjust_disposition", ["+15"]), _m("become_hostile")], + gs, CanonLog.new(), _content(), "fenn") + assert_eq(gs.npc_dispositions.get("fenn", 0), MoveApplier.HOSTILE_FLOOR) + + +func test_disposition_cap_is_per_reply_not_per_conversation(): + # Two separate replies each get a fresh MAX_DELTA budget — standing still moves + # over a conversation, it just cannot lurch in a single line. + var gs = GameState.new() + MoveApplier.apply([_m("adjust_disposition", ["+15"])], gs, CanonLog.new(), _content(), "fenn") + MoveApplier.apply([_m("adjust_disposition", ["+15"])], gs, CanonLog.new(), _content(), "fenn") + assert_eq(gs.npc_dispositions.get("fenn", 0), 30) diff --git a/docs/superpowers/specs/2026-07-12-currency-copper-design.md b/docs/superpowers/specs/2026-07-12-currency-copper-design.md index 9ce21ea..52dfb90 100644 --- a/docs/superpowers/specs/2026-07-12-currency-copper-design.md +++ b/docs/superpowers/specs/2026-07-12-currency-copper-design.md @@ -210,10 +210,14 @@ model selecting a subset of a closed, six-element, state-gated set — an NPC ha a gold, a silver and a copper is a legal sentence — not inventing an integer. Unary repetition was what made it unbounded, and that is closed. -**The same duplicate-tag path exists for non-currency moves and predates this work** -(three `give_item(amulet)` tags yield three amulets; a repeated `offer_quest` can -double-add a quest). It is bounded *across* replies by `gifts_given` / `has_quest`, but -not *within* one. Deliberately left alone here — it is not a currency bug. **Open.** +**The same duplicate-tag path existed for non-currency moves and predated this work.** +Closed in a follow-up (`fix/duplicate-move-tags`), which generalized the rule — *any* +move that names a thing applies at most once per reply — and so **deleted** the currency +special-case rather than leaving two mechanisms side by side. That follow-up also found +`MAX_DELTA` was clamped **per tag**, so three `[ADJUST_DISPOSITION: +15]` tags moved +standing by 45; the cap is now on the reply's net swing, which is what the constant's own +comment always claimed. (`offer_quest` and `reveal` turned out to be safe already — +`add_quest` self-guards and `mark_revealed` is idempotent.) **`NpcContent.available_moves`:**