nightclub-arcadia/docs/skill-system-refactor-plan.md

# Skill System Refactor Plan — placeholder roster → Skill Bible roster

**Audience:** an implementing agent with no prior context on this repo.
**Repo root:** `/Users/lennart/Dev/nightlcub-arcadia`
**Unity project:** `NightclubArcadia/` — Unity `6000.5.8f1`, Yarn Spinner 3 (`dev.yarnspinner.unity`), TextMeshPro.
**Unity editor binary:** `/Applications/Unity/Hub/Editor/6000.5.8f1/Unity.app/Contents/MacOS/Unity`

Read this whole document before touching a file. The section that matters most is
[§2 The reconciliation principle](#2-the-reconciliation-principle) — it tells you what *not* to build.

---

## 1. What exists today

A working, tested skill system built to a *placeholder* roster. The architecture is sound; only the
content is wrong.

### 1.1 File inventory

```
NightclubArcadia/Assets/Scripts/Skills/          (asmdef: NightclubArcadia.Skills)
  Data/SkillDefinition.cs      ScriptableObject: id, displayName, domain, description,
                               startingRank, accentColor. Has #if UNITY_EDITOR EditorSet(...).
  Data/SkillDatabase.cs        ScriptableObject: List<SkillDefinition> + case-insensitive
                               TryGet lookup + OnValidate duplicate/empty-id checks.
  Data/SkillDomain.cs          enum { Reason, Instinct, Presence, Body }
  Data/DcBand.cs               enum DcBand { Trivial=5, Easy=8, Standard=11, Hard=14,
                               Formidable=17, Legendary=20 } + DcBands.TryParse(string,out int)
                               which accepts band names (case-insensitive) OR raw integers.
  Data/SkillCheckDegree.cs     enum { CriticalFailure, Failure, Success, CriticalSuccess }
  Data/SkillCheckResult.cs     readonly struct: skill id/display, roll, rankMod, situationalMod,
                               stateMod, total, dc, success, degree.
  Runtime/PlayerSkills.cs      Plain C# class. Ranks only, seeded from SkillDatabase.StartingRank.
  Runtime/SkillStateModifiers.cs  Plain C#. Nested dict sourceKey -> (skillId -> delta).
                               Supports skillId "*" as a wildcard. TotalFor(skillId) sums all.
  Runtime/SkillCheckSystem.cs  Plain C#, no UnityEngine types. Resolve(skillId, dc, situationalMod):
                               d20 + rank + situational + state; nat 1 = CriticalFailure,
                               nat 20 = CriticalSuccess, else total >= dc.
  Runtime/IRollSource.cs       Roll(sides) -> [1,sides]
  Runtime/UnityRollSource.cs   Random.Range
  Runtime/SeededRollSource.cs  System.Random, for tests / determinism
  Runtime/SkillRuntime.cs      MonoBehaviour singleton + composition root. Holds Database,
                               DialogueRunner, ResultView, Commentary; constructs PlayerSkills,
                               SkillStateModifiers, SkillCheckSystem, IRollSource in Awake().
  Yarn/YarnSkillCommands.cs    static class. [YarnCommand("check")] Check(skillId, difficulty,
                               modifier=0) -> IEnumerator; [YarnFunction("skill_rank")].
                               Writes $check_result/_roll/_total/_dc/_modifier/_degree/_skill.
  UI/SkillCheckResultView.cs   Fading TMP banner. Prints roll, each of the three mods, total,
                               DC, and outcome word. Tints by SkillDefinition.accentColor.
  Commentary/CommentarySystem.cs   Passive barks. Looks for Yarn node "Commentary_{contextId}_{skillId}";
                               only skills whose node exists AND rank >= minRankToSpeak are
                               candidates. Global cooldown minSecondsBetween (20s), recency-window
                               weight halving, weight = max(1, rank). Public static Pick(...) is
                               unit-tested. Fires exactly one node per request.
  Commentary/CommentaryTrigger.cs, Commentary/CommentaryIdleTimer.cs   context sources.

NightclubArcadia/Assets/Skills/
  SkillDatabase.asset          references the 12 definition assets by GUID
  Definitions/*.asset (+.meta) deduction, streetwise, recall, read_room, paranoia, reflexes,
                               persuasion, intimidation, rapport, endurance, sleight_of_hand, nerve

NightclubArcadia/Assets/Editor/
  SkillSystemSetup.cs          SOURCE OF TRUTH for the roster today: a static SkillSpec[] Roster,
                               a menu item "Tools/Nightclub Arcadia/Set Up Skill System" that
                               regenerates the definition assets + database and rewires
                               Assets/Scenes/DialogueTest.unity, and a [DidReloadScripts] hook
                               (EnsureSkillAssetsOnly) that regenerates the ASSETS ONLY once per
                               editor session. <-- see §7.1, this will overwrite hand-edits.
  SkillSystemTestRunner.cs     menu item to run the EditMode test assembly.

NightclubArcadia/Assets/Tests/EditMode/   (asmdef: NightclubArcadia.Skills.Tests)
  DcBandTests.cs               asserts "standard" -> 11
  SkillCheckSystemTests.cs     modifier math, nat 1/20, wildcard state mods, unknown skill
  CommentaryPickerTests.cs     weighted pick + recency halving

NightclubArcadia/Assets/Dialogue/
  Common.yarn                  declares $check_* variables (never <<set>> by hand)
  Commentary.yarn              Commentary_Entrance_Queue_{paranoia,read_room,streetwise}
  Scenes/Entrance.yarn         Start / Entrance_Inside / Entrance_Outside — no skill references
  Scenes/Debug.yarn            <<check persuasion standard>>, <<check deduction 16 2>>,
                               <<check nosuchskill standard>> (deliberate error-path test)
  Characters/Bouncer.yarn      <<check persuasion standard>>, skill_rank("intimidation") >= 2,
                               <<check intimidation hard -1>>, branches on $check_degree
```

### 1.2 Every existing skill-name reference in the project (the complete change surface)

| File | Reference |
|---|---|
| `Assets/Editor/SkillSystemSetup.cs` | the 12-row `Roster` array |
| `Assets/Skills/Definitions/*.asset` | 12 assets, `id:` + `displayName:` fields |
| `Assets/Skills/SkillDatabase.asset` | 12 GUID references |
| `Assets/Dialogue/Scenes/Debug.yarn` | `persuasion`, `deduction` |
| `Assets/Dialogue/Characters/Bouncer.yarn` | `persuasion` ×1, `intimidation` ×2 |
| `Assets/Dialogue/Commentary.yarn` | node titles for `paranoia`, `read_room`, `streetwise` |
| `Assets/Tests/EditMode/*.cs` | `persuasion`, `reflexes`, `read_room`, `paranoia` as literals |

Nothing else in the project references a skill by name. The scene `DialogueTest.unity` references the
**database asset**, not individual definitions, so replacing definition assets does not break scene
wiring.

---

## 2. The reconciliation principle

The target system is described in two documents:

- **`/Users/lennart/Obsidian-Vaults/obsidian-vault/Projects/Active/NightclubArcadia/Skill System Draft.md`**
  — the Skill Bible. Eleven named voices, four opposed pairs plus three specialists, with per-voice
  prose (domain, blind spot, register, verbal tics, forbidden domains, success/failure/passive voice
  samples), firing-frequency targets, and a trace-channel coverage audit in §6. **Read it.** Its full
  content is not reproduced here; §4 below reproduces only the machine-relevant fields.
- An architecture **skeleton** (a rough C# sketch, reproduced in §3 below), which was handed over as
  "a starting spec, not a finished implementation."

**The player never sees an implementation. They see a name on a banner and a line of prose.**
Therefore:

> Where the existing implementation already produces the target behaviour under a different
> internal name, **rename it. Do not rebuild it.** Only write new code where the target requires
> behaviour that does not exist.

The skeleton in §3 is *less* capable than what is already in the repo in several places (it has no
degrees of success, no seedable roll source, no error handling, no node-existence gate, and it puts
game logic in MonoBehaviours that are currently plain testable C# classes). **Do not regress the
implementation to match the skeleton.** Match its *vocabulary and its data model*, keep the existing
mechanics.

### 2.1 Decision table — what each difference actually costs

| Skeleton / Bible says | Repo has | Verdict |
|---|---|---|
| 11 named voices | 12 placeholder skills | **Data change.** Rewrite the roster; no runtime code touched. §5.2 |
| `DCBand { Trivial=8, Routine=11, Hard=14, Specialist=17, BuildDefining=20 }` | `DcBand { Trivial=5, Easy=8, Standard=11, Hard=14, Formidable=17, Legendary=20 }` | **Rename + drop one.** Same parser, same call convention. §5.3 |
| Axes Reason / Body / Social / Self | `SkillDomain { Reason, Instinct, Presence, Body }` | **Rename enum + add `Specialist`.** §5.4 |
| `targetFiringsPerHour` per skill | single global `minSecondsBetween` | **Real new behaviour.** Small, do it. §5.6 |
| `GetTotalModifier` = rank + situational + state | identical three sources, split across `PlayerSkills` (rank), the `<<check>>` argument (situational) and `SkillStateModifiers` (state) | **Already satisfied.** Keep. Add Yarn commands so writers can drive mods (§5.7). |
| `[YarnFunction("rank")]` | `[YarnFunction("skill_rank")]` | **Keep `skill_rank`.** Not player-visible; `rank` is a likely collision with future generic helpers, and `skill_rank` is already used in `Bouncer.yarn`. Note the deviation in the file header. |
| `<<check empathy 14>>` (raw int DC) | `<<check skill <band-or-int> [mod]>>` | **Already a superset.** Keep. Prefer band names in authored Yarn. |
| `CheckResult { success, roll, total, dc }` | `SkillCheckResult` with degrees + per-source mod breakdown | **Already a superset.** Keep. |
| "show the number, always" | `SkillCheckResultView` already prints roll / rank / situation / state / total / DC / outcome | **Already satisfied.** No change. |
| Bible §7 "Wrong Truth": failure grants a wrong deduction, *no failure signal in the prose*, "roll shown as failed in UI only" | UI shows the roll; prose is authored in Yarn | **Already satisfied — this is a writing rule, not a code feature.** Do not add a "quiet failure" mode. Document the convention in `Commentary.yarn` / a Yarn style comment. |
| Bible §6: documentary routes should be *skill-free*, gated on Act + errand state | n/a | **Authoring guidance, not code.** Record it in the doc header; build nothing. |
| Bible per-skill "Domains it must NOT comment on" | `CommentarySystem` only considers a skill if `Commentary_{context}_{skillId}` exists | **Already enforced structurally** — a voice with no node for a context cannot speak there. Zero code. Say so in the doc so nobody builds a forbidden-domain table. |
| Bible "cost gates, not ability gates" for PLACEMENT | n/a | **Authoring guidance.** Build nothing. |
| Bible trace channels + clue yield (§6 audit table) | n/a | **Data + an editor audit.** Cheap and it protects the coverage rule. §5.5 |
| Opposed / allied pairs | n/a | **Data + a symmetry validation.** §5.5 |

---

## 3. The skeleton, for reference

<details>
<summary>Architecture skeleton as handed over (do not implement literally — see §2)</summary>

```csharp
[CreateAssetMenu(menuName = "Skills/SkillDefinition")]
public class SkillDefinition : ScriptableObject
{
    public string skillId;          // e.g. "confluence" — matches flag naming convention
    public string displayName;      // e.g. "CONFLUENCE"
    public string domain;           // one-line, from the skill bible
    [TextArea] public string notes; // full bible entry, for your reference in-editor
    public float targetFiringsPerHour = 5f;
}

public enum DCBand { Trivial = 8, Routine = 11, Hard = 14, Specialist = 17, BuildDefining = 20 }

public class PlayerSkills : MonoBehaviour
{
    private Dictionary<string, int> ranks = new();
    private Dictionary<string, int> situationalMods = new(); // cleared per-scene
    private Dictionary<string, int> stateMods = new();       // consumables/status effects
    public int GetTotalModifier(string skillId) =>
        GetRank(skillId) + situationalMods.GetValueOrDefault(skillId, 0)
                         + stateMods.GetValueOrDefault(skillId, 0);
}

public struct CheckResult { public bool success; public int roll; public int total; public int dc; }

public class SkillCheckSystem : MonoBehaviour
{
    public CheckResult RollCheck(string skillId, DCBand dc) { /* d20 + mod vs dc */ }
}

public class YarnSkillCommands : MonoBehaviour
{
    [YarnCommand("check")] public void Check(string skillId, int dc) { /* sets $check_result, $check_roll */ }
    [YarnFunction("rank")] public static int Rank(string skillId) { ... }
}

public class CommentarySystem : MonoBehaviour
{
    public void RequestCommentary(string context, List<string> eligibleSkillIds)
    {
        // minInterval = 3600f / def.targetFiringsPerHour; skip if not refreshed;
        // weighted pick; one passive per request
    }
}
```

Two ideas from this skeleton are genuinely new and must be adopted: **`targetFiringsPerHour` as a
per-skill budget** (§5.6) and the **`domain` one-liner + `notes` bible blob** on the definition
asset (§5.5). Everything else is already present in better form.

</details>

---

## 4. The canonical roster

Ids are lowercase `^[a-z][a-z0-9_]*$` — they are used as Yarn command arguments and as suffixes in
Yarn node titles, so they cannot contain spaces.

**Article rule:** drop the leading "THE" from ids, **except `the_float`** — bare `float` is a C#
keyword and a Yarn type name, and would break any future generated id constants.

| # | displayName | id | axis | opposedTo | alliedWith | firings/hr | primary channel | secondary | clue yield |
|---|---|---|---|---|---|---|---|---|---|
| 1 | `PROVENANCE` | `provenance` | Reason | `confluence` | `the_float`, `room_tone` | 7 | Documentary | Physical | High |
| 2 | `CONFLUENCE` | `confluence` | Reason | `provenance` | `undisclosed`, `house_pour` | 9 | Behavioural | Testimonial | Low |
| 3 | `HOUSE POUR` | `house_pour` | Body | `long_shift` | `facework`, `amnesty` | 8 | Behavioural | Physical | Medium |
| 4 | `THE LONG SHIFT` | `long_shift` | Body | `house_pour` | `standing_order`, `room_tone` | 6 | Physical | Behavioural | Medium |
| 5 | `FACEWORK` | `facework` | Social | `placement` | `house_pour`, `room_tone` | 10 | Testimonial | Behavioural | High |
| 6 | `PLACEMENT` | `placement` | Social | `facework` | `the_float`, `confluence` | 6 | Testimonial | None | Medium |
| 7 | `AMNESTY` | `amnesty` | Self | `standing_order` | `house_pour`, `facework` | 4 | Behavioural | None | Low |
| 8 | `STANDING ORDER` | `standing_order` | Self | `amnesty` | `long_shift`, `provenance` | 5 | Testimonial | None | Low |
| 9 | `UNDISCLOSED` | `undisclosed` | Specialist | `provenance` | `confluence`, `placement` | 3 | Physical | Behavioural | Low |
| 10 | `THE FLOAT` | `the_float` | Specialist | `amnesty` | `provenance`, `placement` | 5 | Documentary | Physical | High |
| 11 | `ROOM TONE` | `room_tone` | Specialist | `placement` | `facework`, `long_shift` | 8 | Physical | Testimonial | High |

Sums and tallies that a validation pass must reproduce (Bible §6):

- Firing total = **71 / hour**.
- Primary-channel tally: physical **3**, testimonial **3**, behavioural **3**, documentary **2**.
- `opposedTo` is **symmetric within the four pairs** (1↔2, 3↔4, 5↔6, 7↔8). The three specialists'
  `opposedTo` entries are **deliberately asymmetric** — `undisclosed→provenance`,
  `the_float→amnesty`, `room_tone→placement` are one-directional thematic oppositions stated in the
  Bible, and the targets already oppose their own pair partner. Validation must allow this: assert
  symmetry only when both skills have `axis != Specialist`.
- `startingRank`: **1 for every skill.** The Bible does not assign starting ranks; character
  creation is out of scope. One exception for the demo — see §5.9.
- `accentColor`: pick something plausible per axis (Reason cool blue, Body amber, Social violet,
  Self pale green, Specialists desaturated). Purely cosmetic; the current values are placeholders too.

The one-line `domain` strings and the long-form `notes` blob come verbatim from the Bible's per-skill
`domain:` field and the whole `SKILL` block respectively.

### 4.1 Old → new mapping for the five existing call sites

The roster is a **replacement, not a migration**; there is no 1:1 map for all 12. Only these
call sites need a decision, and these are the decisions:

| Where | Old | New | Why |
|---|---|---|---|
| `Bouncer.yarn` — "I'm on the list" | `persuasion` | `placement` | PLACEMENT's domain is literally "the ask, the favour, the comp, **the door**". |
| `Bouncer.yarn` — "Let me through" (gate + check) | `intimidation` | `undisclosed` | UNDISCLOSED is leverage never spelled out — "He never threatened you." Exactly this beat. |
| `Debug.yarn` | `persuasion` | `placement` | as above |
| `Debug.yarn` | `deduction` | `confluence` | CONFLUENCE is the deduction engine. |
| `Commentary.yarn` — "hasn't looked at his phone once" | `paranoia` | `facework` | Reading one individual. |
| `Commentary.yarn` — "pretending they're not cold" | `read_room` | `long_shift` | Bodies paying a cost. |
| `Commentary.yarn` — "that's a price, not a queue" | `streetwise` | `the_float` | Money in motion. |
| Tests | `persuasion`, `reflexes`, `read_room`, `paranoia` | `placement`, `long_shift`, `facework`, `house_pour` | arbitrary; tests only need valid ids |

`<<check nosuchskill standard>>` in `Debug.yarn` is a **deliberate** bad-id error path. Keep the bad
id; only update its DC band name.

---

## 5. Work items

Do these in order. Each is independently compilable.

### 5.1 Add a JSON bible as the roster source of truth

**Problem:** the roster currently lives as a C# `SkillSpec[]` in an editor script. The new roster
carries ~800 characters of authored prose per voice (`domain`, `notes`). Eleven verbatim string
literals inside an editor script is unmaintainable, and the writer who owns this content should not
have to edit C#.

**Do:** create `NightclubArcadia/Assets/Skills/skill_bible.json` as the machine-readable mirror of
the Bible, and make `SkillSystemSetup` read it instead of a hardcoded array.

```jsonc
{
  "skills": [
    {
      "id": "provenance",
      "displayName": "PROVENANCE",
      "axis": "Reason",
      "domain": "Chains of custody. Paperwork, licences, deeds, freight stamps, serial numbers, rebuilds, previous owners. Reading an object as a descendant of the decisions that moved it.",
      "notes": "<full SKILL block from the Bible, newline-escaped>",
      "targetFiringsPerHour": 7,
      "opposedTo": "confluence",
      "alliedWith": ["the_float", "room_tone"],
      "primaryChannel": "Documentary",
      "secondaryChannel": "Physical",
      "clueYield": "High",
      "startingRank": 1,
      "accentColor": { "r": 0.62, "g": 0.72, "b": 0.95, "a": 1 }
    }
    // ... 10 more
  ]
}
```

Notes:
- `JsonUtility` cannot deserialize a top-level array, hence the `skills` wrapper object. Use a
  `[Serializable]` DTO; enum-valued fields are strings in JSON and parsed with `Enum.TryParse`
  (fail loudly on a typo — this file is content, and a silent default is a content bug).
- Put the file under `Assets/` so it is an addressable project asset; it is editor-time-only input,
  so a `TextAsset` load via `AssetDatabase.LoadAssetAtPath<TextAsset>` is fine.
- Keep the human-facing Bible in the Obsidian vault as the source of the prose; the JSON is a
  derived, checked-in mirror. State this in a comment at the top of `SkillSystemSetup.cs`.

### 5.2 Rewrite `SkillDefinition`

Add fields; keep the existing style (private `[SerializeField]` + public getters + the
`#if UNITY_EDITOR EditorSet(...)` writer).

```
id                     string   (unchanged)
displayName            string   (unchanged)
axis                   SkillAxis         <- renamed from `domain`, see §5.4
domain                 string            <- NEW: the Bible's one-line domain
notes                  string [TextArea] <- NEW: full Bible entry (replaces `description`)
targetFiringsPerHour   float             <- NEW
opposedTo              string            <- NEW (skill id)
alliedWith             string[]          <- NEW (skill ids)
primaryChannel         TraceChannel      <- NEW
secondaryChannel       TraceChannel      <- NEW
clueYield              ClueYield         <- NEW
startingRank           int      (unchanged)
accentColor            Color    (unchanged)
```

- Drop `description`; `notes` supersedes it. The only consumer of `description` is the placeholder
  string in `SkillSystemSetup`.
- The `EditorSet(...)` signature grows past readability. Replace the positional parameter list with a
  single `[Serializable]` DTO parameter (reuse the JSON DTO from §5.1) — this also makes the three
  test files' helpers shorter, not longer.
- New enums, in `Data/`:
  `enum TraceChannel { None, Documentary, Behavioural, Testimonial, Physical }`
  `enum ClueYield { Low, Medium, High }`
  Keep the British `Behavioural` spelling — the Bible uses it and it will be grepped against the
  Bible constantly.

### 5.3 `DcBand` → the five target bands

```csharp
public enum DcBand
{
    Trivial       = 8,
    Routine       = 11,
    Hard          = 14,
    Specialist    = 17,
    BuildDefining = 20,
}
```

- The old `Trivial = 5` is **removed**, and `Trivial` is **reused at 8**. No call site uses
  `trivial` today, so nothing silently changes value — but flag this in the commit message, because
  it is exactly the kind of rename that would silently rebalance an existing script.
- `DcBands.TryParse` keeps accepting raw integers (`<<check confluence 16 2>>` is used in
  `Debug.yarn`, and the skeleton's own example passes a raw int). The Bible says "never use
  in-between values", so **add a `Debug.LogWarning` when a parsed integer is not one of the five band
  values** — a nudge for the author, not an error. Do not make it throw.
- Do **not** rename the C# type to `DCBand`; `DcBand` matches the project's existing casing
  convention and is not player-visible.

### 5.4 `SkillDomain` → `SkillAxis` (a terminology collision worth fixing)

The Bible uses "domain" to mean *the subject matter a voice covers* ("Chains of custody…"), while the
current enum uses it to mean *the axis a skill sits on*. Leaving both named `domain` will cause
persistent confusion for the writer.

```csharp
public enum SkillAxis { Reason, Body, Social, Self, Specialist }
```

- Rename the file `Data/SkillDomain.cs` → `Data/SkillAxis.cs` (and its `.meta`).
- Rename the `SkillDefinition` field `domain` → `axis` and free the name `domain` for the Bible's
  one-liner (§5.2). Because the definition assets are regenerated wholesale in §5.8,
  `[FormerlySerializedAs]` is not needed — but the *serialized value* changes meaning
  (old `domain: 2` meant `Presence`), so the old assets **must be deleted, not edited in place**.

### 5.5 Roster validation in `SkillDatabase.OnValidate`

Extend the existing duplicate/empty-id checks (do not create a new validator class) with:

1. Every `id` matches `^[a-z][a-z0-9_]*$`.
2. `opposedTo` and every `alliedWith` entry resolve to a skill in this database.
3. `opposedTo` is symmetric **when both skills have `axis != Specialist`**.
4. Exactly one pair per axis for `Reason` / `Body` / `Social` / `Self` (2 skills each), and 3
   `Specialist`.
5. `targetFiringsPerHour > 0` for every skill; log the **sum** at info level so drift from the
   Bible's 71 is visible.
6. Log the primary-channel tally so drift from `physical 3 / testimonial 3 / behavioural 3 /
   documentary 2` is visible. Bible §6 flags documentary as the scarce channel and the whole
   coverage rule depends on it.

Rules 5 and 6 are **informational logs, not errors** — they are design budgets the writer is
expected to adjust deliberately.

### 5.6 Per-skill firing budgets in `CommentarySystem`

This is the one genuinely new runtime behaviour. Keep everything else about the class — including
the node-existence gate, the `_recent` recency halving, and the public static `Pick(...)` that the
tests exercise.

Change `RequestCommentary(contextId, repeatable)`:

- Add `readonly Dictionary<string, float> _lastFiredBySkill`.
- A skill is a candidate only if, in addition to the existing gates
  (non-null, non-empty id, `rank >= minRankToSpeak`, node exists):
  `Time.time - _lastFiredBySkill.GetValueOrDefault(id, float.NegativeInfinity) >= 3600f / def.TargetFiringsPerHour`.
- Guard `TargetFiringsPerHour <= 0` (treat as "never fires", and let `OnValidate` complain).
- Keep the existing global `minSecondsBetween` floor. It is the aggregate limiter; 71 firings/hour is
  roughly one per minute, so a 20s global floor and per-skill budgets are complementary, not
  redundant.
- On a successful fire, set `_lastFiredBySkill[picked.Id] = Time.time` alongside the existing
  `_lastFiredAt` / `_firedContexts` / `_recent` bookkeeping.
- Keep `Pick(...)`'s signature and weighting unchanged so `CommentaryPickerTests` stays valid.
  Budget eligibility belongs in the candidate filter, not in the weighting.

**Optional, only if it costs nothing:** an ambient fallback. If no `Commentary_{context}_{id}` node
exists for any eligible skill, look for `Commentary_Ambient_{id}` — the Bible gives every voice one
`PASSIVE VOICE` line that is context-free. Implement only if it does not complicate the candidate
loop; otherwise leave it out and note it as future work.

### 5.7 Yarn commands for situational / state modifiers

The skeleton exposes `SetSituationalMod` / `ClearSituationalMods`. The repo's `SkillStateModifiers`
(source-keyed, wildcard-capable) is strictly more capable, but there is currently **no way for a
writer to set a modifier from Yarn** — the only situational input is the third argument to
`<<check>>`. Close that gap in `YarnSkillCommands`:

```
<<skill_mod scene facework 2>>      // set: source key, skill id (or "*"), delta
<<clear_skill_mods scene>>          // clear one source key
```

Convention (document it in the file header and in `Common.yarn`):
**source key `scene` is the "situational" bucket** and must be cleared on scene exit;
any other source key (`drunk`, `injured`, …) is the "state" bucket. This preserves the
"exactly three modifier sources" rule from the design doc without adding a fourth dictionary.

Both commands must fail soft (log an error, do not throw) when `SkillRuntime.Instance` is null,
matching the existing `<<check>>` error handling.

### 5.8 Regenerate the assets

The definition assets must be **replaced**, not edited — the meaning of the serialized `domain`
integer changes (§5.4) and the roster is not a 1:1 map.

**Preferred path (Unity available):**

1. Delete `NightclubArcadia/Assets/Skills/Definitions/` entirely (all 12 `.asset` + `.meta`).
2. Add a cleanup step to `SkillSystemSetup`: after generating, delete any `.asset` in
   `Definitions/` whose filename is not in the roster. This prevents the exact orphan problem that
   made this step manual.
3. Run the menu item headlessly:
   ```bash
   /Applications/Unity/Hub/Editor/6000.5.8f1/Unity.app/Contents/MacOS/Unity \
     -batchmode -quit -nographics \
     -projectPath /Users/lennart/Dev/nightlcub-arcadia/NightclubArcadia \
     -executeMethod SkillSystemSetup.SetUpSkillSystem \
     -logFile -
   ```
   Note `SetUpSkillSystem` also opens and re-saves `Assets/Scenes/DialogueTest.unity`. That is fine
   and idempotent, but it will produce scene diff noise — check the scene diff before committing and
   revert it if it is pure reserialization.

**Fallback path (Unity unavailable):** hand-author the YAML. Each definition is a single
`MonoBehaviour` asset; copy the shape of an existing file
(`Assets/Skills/Definitions/persuasion.asset`), keep
`m_Script: {fileID: 11500000, guid: 9a26230b6f20436891aa772e8f858f3f, type: 3}` (the
`SkillDefinition` script GUID — verify it against `Scripts/Skills/Data/SkillDefinition.cs.meta`),
and write a fresh 32-hex-char `guid:` into each `.meta`:

```yaml
fileFormatVersion: 2
guid: <32 hex chars>
NativeFormatImporter:
  externalObjects: {}
  mainObjectFileID: 11400000
  userData:
  assetBundleName:
  assetBundleVariant:
```

Then rewrite `Assets/Skills/SkillDatabase.asset`'s `skills:` list to
`- {fileID: 11400000, guid: <that guid>, type: 2}` in roster order. Multi-line `notes` in YAML is
the only fiddly part — use a block scalar or escape newlines as `\n` in a double-quoted scalar,
matching how Unity already escapes the em-dash in `persuasion.asset`.

### 5.9 Update the Yarn call sites

Apply the §4.1 mapping.

- `Debug.yarn`: `<<check placement routine>>`, `<<check confluence 16 2>>`,
  `<<check nosuchskill routine>>` (keep the deliberate bad id).
- `Bouncer.yarn`: `<<check placement routine>>`; `skill_rank("undisclosed") >= 2`;
  `<<check undisclosed hard -1>>`. Give `undisclosed` `startingRank: 2` in the JSON bible so the
  gated option is reachable in the demo — this is the one starting-rank exception, and it replaces
  the identical existing exception for `intimidation`. Comment it in the JSON.
- `Commentary.yarn`: rename the three nodes to `Commentary_Entrance_Queue_{facework,long_shift,the_float}`
  and **rewrite the three lines in each voice's register**. The current lines are placeholders. Use the
  Bible's `PASSIVE VOICE` samples as the model — e.g. ROOM TONE gives direction before content and
  transcribes with the gaps marked; THE FLOAT converts everything to a nightly rate and never
  moralises. Also drop the `"Paranoia: "` / `"Read the Room: "` speaker prefixes if the display name
  is already surfaced elsewhere; if not, use the exact `displayName` (`THE FLOAT:`, `FACEWORK:`).
- `Common.yarn`: no `$check_*` change needed. Add a short comment block recording the two writing
  rules that the code deliberately does not enforce (Bible §7 / §2.1 above):
  *Wrong Truth failures carry no failure signal in the prose — the UI banner is the only tell;*
  *documentary routes are gated on Act and errand state, never on a voice.*

### 5.10 Update the tests, then add three

Existing tests need mechanical fixes:

- `DcBandTests`: `"standard" -> 11` becomes `"routine" -> 11`. Add a case asserting `"trivial" -> 8`
  (not 5) so the reused name is pinned.
- `SkillCheckSystemTests`, `CommentaryPickerTests`: the `EditorSet(...)` helper calls change to the
  DTO form (§5.2), and the literal skill ids change per §4.1. No assertions about check maths should
  change — if one does, you have regressed the implementation. Stop and re-read §2.

New tests worth the keystrokes:

1. **Firing budget gating** (`CommentarySystem`): a skill with `targetFiringsPerHour = 3600` (one per
   second) is eligible again immediately; a skill with `targetFiringsPerHour = 1` is not. This needs
   the budget check to be reachable without a `DialogueRunner` — extract the eligibility predicate
   into a `public static bool IsOffCooldown(float lastFired, float now, float targetPerHour)` and
   test that, rather than trying to instantiate the MonoBehaviour.
2. **Roster shape**: load `Assets/Skills/SkillDatabase.asset` in an EditMode test and assert 11
   skills, id regex, pair symmetry (non-specialists), the axis counts, the 71 firing total, and the
   channel tally. This is the test that catches a JSON typo before it reaches the writer.
3. **Id/display divergence**: assert `displayName` is the Bible's uppercase form and `id` is the
   snake_case form for all 11 — the two are intentionally different and easy to accidentally unify.

Run them:
```bash
/Applications/Unity/Hub/Editor/6000.5.8f1/Unity.app/Contents/MacOS/Unity \
  -batchmode -quit -nographics \
  -projectPath /Users/lennart/Dev/nightlcub-arcadia/NightclubArcadia \
  -runTests -testPlatform EditMode -testFilter "NightclubArcadia.Skills.Tests" \
  -testResults /tmp/skill-tests.xml -logFile -
```

---

## 6. Explicit non-goals

Do not build any of these. Each is either already satisfied or is a writing convention:

- A "quiet failure" / suppressed-failure-signal check mode. The UI banner *is* the tell; the prose
  is the writer's job (Bible §7).
- A forbidden-domains table per skill. Node absence already enforces it (§2.1).
- Clue / deduction / evidence objects (`CL-0xx`, `DED-0xx`, criticality, routes). The Bible §7
  worked example references a clue ledger from a separate handbook. **That system does not exist in
  this repo and is out of scope.** Do not invent a schema for it.
- Act / errand state, or any gating for documentary routes.
- Character creation, rank progression, or skill-point spending.
- Converting `PlayerSkills` / `SkillCheckSystem` into MonoBehaviours to match the skeleton. They are
  plain C# classes precisely so they can be unit-tested; the skeleton is a sketch, not a target.
- Renaming `skill_rank` to `rank`, or `SkillCheckResult` to `CheckResult`.

---

## 7. Traps

### 7.1 `[DidReloadScripts]` will overwrite hand-edited assets

`SkillSystemSetup.AutoSetupAfterReload` calls `EnsureSkillAssetsOnly()` once per editor session,
which regenerates every definition asset and the database from the roster. **If you hand-edit an
`.asset` without updating the roster source (now `skill_bible.json`), your edit will be silently
reverted the next time scripts reload.** The JSON is the source of truth; assets are build output.
Consider adding that sentence as a comment at the top of every generated asset's owning script.

### 7.2 Skill ids are Yarn node-title fragments

`CommentarySystem` builds `Commentary_{contextId}_{skillId}`. Yarn node titles must be valid
identifiers. This is the reason for the id regex in §5.5 — an id with a space or a hyphen produces
a node title that either fails to compile or silently never matches.

### 7.3 Lookups are case-insensitive, node names are not

`SkillDatabase` and `PlayerSkills` use `StringComparer.OrdinalIgnoreCase`; `CommentarySystem`'s
`NodeExists` uses `StringComparison.Ordinal`. So `<<check PLACEMENT routine>>` works but
`Commentary_Entrance_Queue_PLACEMENT` does not. Keep every authored id lowercase.

### 7.4 The scene references the database, not the definitions

`Assets/Scenes/DialogueTest.unity` wires `SkillRuntime.database` → `SkillDatabase.asset`, and
`CommentarySystem.database` likewise. Replacing definition assets does not break the scene, provided
`SkillDatabase.asset` keeps its own GUID. **Do not delete or recreate `SkillDatabase.asset`** —
rewrite its `skills:` list in place.

### 7.5 `float` is a keyword

Covered in §4, restated because it will be tempting: the id for THE FLOAT is `the_float`.

---

## 8. Definition of done

- [ ] `skill_bible.json` contains all 11 voices with every field in §4, prose copied from the Bible.
- [ ] `Assets/Skills/Definitions/` contains exactly 11 assets, named `<id>.asset`; no orphans.
- [ ] `SkillDatabase.asset` keeps its original GUID and lists the 11 in roster order.
- [ ] `DcBand` has exactly the five target bands; a non-band integer DC logs a warning.
- [ ] `SkillAxis` replaces `SkillDomain`; `SkillDefinition.domain` is now the Bible one-liner.
- [ ] `CommentarySystem` enforces a per-skill `3600 / targetFiringsPerHour` budget on top of the
      existing global floor.
- [ ] `<<skill_mod>>` / `<<clear_skill_mods>>` exist and fail soft.
- [ ] No Yarn file, test, or script references any of the 12 old skill names.
- [ ] EditMode tests pass, including the three new ones in §5.10.
- [ ] `grep -ri` for each old id across `NightclubArcadia/Assets` (excluding `Library/`) returns
      nothing.
- [ ] The check maths in `SkillCheckSystem` is byte-for-byte unchanged apart from the DC band
      constants. If it is not, §2 was violated.