# Phase A — Progression domain Replace the current state layer with the model in [`CONTEXT.md`](../../CONTEXT.md), governed by [ADR-0001..0004](../adr/). **No new player-facing features.** Save files are wiped once; the game is unreleased so this costs nothing (ADR context). Phase B — slot picker UI, level select, Replay surfacing, best times out of the ghost files — is out of scope here. Phase A does put the *persistence format* in place for slots, so Phase B is UI work rather than another model change. ## Two kinds of slice | | verified by | `/tdd` | |---|---|---| | **Slices 1–6** — pure domain, no Godot | `dotnet test` | yes — red/green/refactor throughout | | **Slices 7–13** — adapters and wiring | compiler + playing the game | no — write the test where one is possible, otherwise verify by hand | Don't try to TDD slices 7–13. You can't red-green an autoload swap or a `.tscn` connection deletion, and forcing it produces tests that assert the mock rather than the behaviour. The value of slices 1–6 is that they absorb *all* the logic worth testing, so the untestable slices become mechanical. Each slice ends with the build green and the game runnable. Don't start the next one until it is. --- ## Slice 0 — scaffolding No tests. Pure setup. - `progression/Mr.BrickAdventures.Progression.csproj` — plain `net8.0` library, **no Godot reference**, root namespace `Mr.BrickAdventures.Progression`. - `progression.tests/Mr.BrickAdventures.Progression.Tests.csproj` — xunit (NUnit is equally fine), references the domain library **only**. - Add both to `Mr. Brick Adventures.sln`. Add a `ProjectReference` from the game project to the domain library. - Set `true` in the domain library too, not just the game project — Godot needs it on referenced assemblies for hot reload, and omitting it produces confusing load failures. **Done when:** `dotnet test` runs and reports zero tests; Godot still builds; the game still launches and plays. **Guard rail for the whole phase:** ```sh # must print nothing — anchored to real using-directives grep -rnE "^\s*(global\s+)?using\s+Godot" progression progression.adapters progression.tests --include=*.cs # stronger, and cannot be fooled by wording: no project may resolve the engine assembly at all grep -c GodotSharp progression/obj/project.assets.json progression.adapters/obj/project.assets.json ``` That check is what ADR-0004 buys you. Match using-directives **anchored to the start of a line**: a bare `grep -r "using Godot"` also hits `.csproj` comments and doc comments that legitimately mention the engine in prose, and reports a false violation. The `project.assets.json` check is the one that cannot be fooled by wording. --- ## Slice 1 — value objects · TDD `Coins`, `Lives`, `LevelId`, `SkillId`, `SkillLevel`, `StatName`, `LevelTime`, `PlaythroughId`. Invariants to drive out with tests: - `Coins` cannot be negative. Subtraction that would go negative is a failure the caller must handle, not a silent clamp — the current `Mathf.Max(0, ...)` clamp in one path and absence of it in another is exactly the bug this removes. - `Lives` floors at zero, and reaching zero is observable. - `LevelId` is a non-empty stable string. Not an int, not an index. - `SkillLevel` starts at 1 and has no zero value — "unowned" is represented by *absence*, never by level 0. See the `SkillData.Level = 1` ambiguity this replaces. - `LevelTime` is positive; comparison is what "best" means. - `StatName` is a closed set (the five in `StatNames` today), so a typo cannot invent a statistic. **Done when:** no value object can hold invalid state, and every clamp/guard currently scattered across `GameStateStore` and `ConsoleManager` exists in exactly one place. --- ## Slice 2 — Attempt · TDD The ledger from ADR-0002: `CoinsFound`, `CoinsSpent`, `SkillsBought`, plus an outcome. - `Attempt(LevelId level)` and `LevelId Level` — the Attempt knows which Level it is an attempt at, so `Commit` decides First Clear versus Replay from the Attempt rather than from a level the caller passes alongside - `Outcome` — `InProgress | Died | Cleared` - `void RecordCoinsFound(Coins)` — silently ignores late coins once ended - `bool TryRecordPurchase(SkillId, SkillLevel resulting, Coins cost)` — records where the skill ended up, since one Attempt can both unlock and upgrade the same skill - `bool TryEndInDeath()` / `bool TryEndInCompletion()` — return false if the Attempt already ended - No `Discard` method: discarding is throwing the object away. The recording methods are deliberately asymmetric. A coin pickup firing after death is a real event — the player node survives for two seconds before `DeathScreen` reloads — and its caller has nothing useful to do with a `false`, so it returns nothing. A purchase's caller does care. `CoinsFound` and `CoinsSpent` are both non-negative. There is no signed net value — settlement is `bank.Add(found).TrySubtract(spent)` in `Playthrough.Commit`, which in that order cannot go negative given affordability was checked at purchase time. > Corrects an earlier draft of this plan, which had `NetCoins` "may be negative". That would have forced `Coins` > to permit negatives and defeated the point of the type. **The outcome is the idempotency guard, and it is the reason this slice matters.** An Attempt is the thing that dies or is cleared, and it can only do either once. `Lives` flooring at zero does not solve this: losing two lives from a single death takes you from 3 to 1, which no clamp catches. Tests worth writing: - **An Attempt cannot end twice.** Second `TryEndInDeath` returns false and changes nothing. - **An Attempt that died cannot then be cleared**, and vice versa. - Spending more than found is representable — the shortfall is settled at Commit, not rejected here. - Recording anything after the Attempt ended is refused. Two real call sites motivate this. Death: overlapping hazards in a kill zone, or a hazard re-triggering during the two seconds before `DeathScreen` reloads the scene — the player node still exists with zero health. Completion: `ExitDoorComponent:46` and `ConsoleManager.GoToNextLevelCommand:99` both call `OnLevelComplete()`. `HealthComponent.ApplyHealthChange` currently absorbs repeat deaths via its `delta == 0` early return, but that is incidental and `Health` is a public `[Export]` setter that bypasses it. Do not rely on the engine side. --- ## Slice 3 — Playthrough · TDD Aggregate root. `PlaythroughId`, `Lives`, `Bank`, `OwnedSkills : SkillId → SkillLevel`, `CurrentLevel : LevelId`, `ClearedLevels`. Operations: `SettleDeath(Attempt)`, `SettleCompletion(Attempt, Campaign)`, `CanAfford(Attempt, Coins)`, `HasCleared(LevelId)`, `ReachableLevels(Campaign)`, `IsRunOver`. **There is no `LoseLife`** — see ADR-0005. Each settle operation calls the Attempt's `TryEnd…` internally and does nothing when it returns false, so it is atomic and idempotent, and no bare mutator exists to be called from a second place. **Two settle operations, not three.** An earlier draft had `Commit` and `SettleReplay` as separate calls, which made the *caller* decide whether a completion was a First Clear or a Replay — precisely the ADR-0003 regression. `SettleCompletion` branches on `HasCleared(attempt.Level)` itself, so it cannot be selected wrongly. **`Campaign`** is a domain value object holding the authored ordering of `LevelId`s (`First`, `Next`, `Contains`). `ILevelCatalogue` stays on the adapter side resolving a `LevelId` to a scene handle and display name; the ordering is pure data handed in as a `Campaign`, so the aggregate never touches a port. **`Commit` does not validate skill levels against the upgrade cap.** Max level comes from `SkillData.Upgrades.Count`, which is authored data the domain cannot see. The cap is a purchase-time rule enforced in slice 6's `UpgradeSkill`, which is also where `TryUpgradeSkill`'s current `Upgrades[skill.Level]` overrun gets a test. `Commit` applies what the ledger recorded. Events leave via `IReadOnlyList DrainEvents()`. The composition root drains once after every use case and republishes (ADR-0004), so operations stay `void` and the "forgot to drain" risk collapses to one line in one place. Raises: `LifeLost(remaining)`, `RunEnded`, `BankChanged(total)`, `SkillSetChanged`. This is where the report's bugs become tests. Write these first, watch them fail: - **Losing the last life sets lives to zero *and* raises `RunEnded` in one operation.** This is the last-life soft-lock — today two screens read `Lives` before the handler writes it. Make it one atomic operation and the race cannot be expressed. - **Two deaths in one Attempt cost one life.** Pairs with slice 2's outcome guard; assert it here at the Playthrough level too, because this is the invariant a player would actually notice. - **`Commit` on an already-cleared level does not advance and does not bank** (ADR-0003). - **`Commit` settles as `bank.Add(found).TrySubtract(spent)`**, in that order, so an Attempt that overspent reduces the Bank correctly and can never drive it negative. A discarded Attempt leaves the Bank untouched — the "buy mid-level, die, lose coins *and* skill" bug. - **`ReachableLevels` is computed** from `ClearedLevels` plus level order, never stored. `UnlockedLevels` and `IsLevelUnlocked` are deleted, not ported. - **`Bank` cannot go negative** even through Commit. --- ## Slice 4 — Profile · TDD Aggregate root, one per installation. `Achievements`, `BestTimes : LevelId → LevelTime`, `Statistics`. - `RecordTime(LevelId, LevelTime)` — only replaces a slower time. Recording a worse time is a no-op, not an error. - `UnlockAchievement(id)` — idempotent, raises `AchievementUnlocked` only on the transition. - `Record(StatName, int)`. - `RelockAchievement(id)` exists for the console only, and is documented as **not** reaching Steam — that asymmetry is unavoidable (Steam achievements cannot be un-earned) so make it explicit rather than surprising. --- ## Slice 5 — ports and in-memory adapters · TDD Interfaces in the domain: `IPlaythroughStore`, `IProfileStore`, `ILevelCatalogue`, `IGhostStore`. In-memory implementations live in the **test project**. That's what makes the seam real rather than hypothetical — two adapters, one of which ships and one of which doesn't. - Round-trip: save a Playthrough, load it, assert equality. Same for Profile. - Loading an absent Playthrough and loading a *corrupt* one are different outcomes. Today both surface as `false` from `LoadGame`, which is why a version bump silently overwrites saves. - `ILevelCatalogue` returns ordered `LevelId`s and resolves one to a scene reference — as an opaque handle, since the domain can't name `PackedScene`. --- ## Slice 6 — use cases · TDD `StartPlaythrough`, `LoadPlaythrough`, `EnterLevel`, `FinishLevel`, `HandlePlayerDeath`, `PurchaseSkill`, `UpgradeSkill`. Four of these carry the orchestration that is currently spread across `GameManager`, the `Events/` handlers and the UI. Each of the remaining report bugs gets a test here: - **`EnterLevel` always announces `LevelStarted`.** Today only `TryToGoToNextLevel` emits it, so ghost recording never starts on the first level or after Continue, and a continued run records completion times of zero. - **`FinishLevel` branches First Clear vs Replay** and a Replay never advances `CurrentLevel` (ADR-0003 flags this as the obvious regression). - **`HandlePlayerDeath` loses a life and discards the Attempt as one operation.** - **`PurchaseSkill` writes only the Attempt**, checks affordability across Attempt + Bank, and rejects rather than throwing when short. - **`UpgradeSkill` reads the level from the Playthrough**, and cannot index past the upgrade table. **Done when:** every bug listed in the architecture review has a test that failed before the fix and passes after. That is the deliverable of Phase A — the point where "hidden bugs" stops being a category. --- > Everything above is pure C# with no Godot. Everything below touches the engine. Switch verification mode here. --- ## Slice 7 — level catalogue adapter `resources/levels/` already holds six `LevelResource` files (`village_1..5`, `forest_1`) but there are eight level scenes and only two scenes reference a resource at all. Complete and adopt them. - Add `LevelId` and explicit order to `LevelResource`; create the two missing entries (`tutorial_1`, `village_6`). - A catalogue resource lists them in order. Adapter implements `ILevelCatalogue`. - Delete `GameManager.LevelScenes` and all array-index addressing. - `LevelResource.ScenePath` is currently never read — either it becomes how the catalogue resolves scenes, or it goes. Don't leave it. **Verify:** game boots into the first level; order matches the catalogue; `DeathScreen`'s level-name label still populates. ## Slice 8 — JSON store adapters - `user://profile.json` and `user://playthrough_{id}.json`. The slot format lands now even though the UI only ever creates one; Phase B then adds a picker and nothing about the format changes. - Write to a temp file and rename, so a crash mid-save can't corrupt a slot. With N slots this stops being theoretical. - Keep a version field, but replace the equality check with a migration hook that can return "unmigratable" — distinct from "absent". **Verify:** play, quit, continue. Then hand-corrupt a file and confirm you get an error rather than a silent new game. ## Slice 9 — composition root and republisher - New autoload owns the `Profile` and the active `Playthrough`. It is the only `Node` in this area. - Republisher subscribes to domain events and emits the existing `EventBus` signals (ADR-0004), so `Hud`, `DeathScreen` and the rest keep working unchanged. - Delete `GameStateStore`. Register the new autoload in `project.godot` in its place. **Verify:** HUD shows coins and lives; a coin pickup updates them. ## Slice 10 — swap the call sites Mechanical and compiler-driven — sealing the aggregates turns every old write into a compile error, which is the cheapest way to find them all. - `Events/` handlers become thin adapters that call use cases. `CoinStateHandler` already has this shape. - `ConsoleManager` calls use cases instead of writing fields and hand-re-emitting signals. - `AchievementManager` reads and writes the `Profile` instead of reaching into `PlayerState`. - `SkillManager` reconciles `ActiveComponents` from `SkillSetChanged` — `ApplyUnlockedSkills` becomes the only path, so the four two-call pairs collapse to one call each and a skill can no longer end up owned but inert. - Delete `SaveSystem`, `PlayerState`, `SessionState`, `SaveDataDto`. **Verify:** compiler first, then play one level start to finish including a purchase. ## Slice 11 — skill level off the asset - `ISkill.Initialize(Node owner, SkillData data, int level)`. Update `BrickThrowComponent:90`, `MagneticSkillComponent:97`, `BrickArmorSkillComponent:25` to use the passed level rather than `Data.Level`. `BrickArmorSkillComponent` indexes `Upgrades[Level - 1]` unguarded — fix while you're there. - Delete `SkillData.Level`. `SkillData` becomes immutable authored data (CONTEXT.md). - `MarketplaceButton` and `Marketplace` read the level from the Playthrough. **Verify:** buy a skill, upgrade it, quit, continue — level persists. Game over, New Game — level is gone. Both are currently wrong. **Landmine to note:** five skills (`brick_armor`, `brick_shield`, `double_jump`, `ground_pound_skill`, `wall_jump`) have no `upgrades/` entries, so `Upgrades.Count == 0`. `TryUnlockSkill` would throw on `Upgrades[0].Cost`; it's currently unreachable only because `MarketplaceButton` hides zero-upgrade skills. Decide whether those skills are purchasable at all and make the domain say so. ## Slice 12 — one death path - Delete the 18 `Death` → `OnPlayerDeath` connections across `objects/level/base_level.tscn` and the eight level scenes. - `DeathScreen` and `GameOverScreen` subscribe to republished `LivesChanged` / `RunEnded`. - `DeathScreen` currently reads `Player.Lives` directly *and* subscribes to `LivesChanged` — it keeps only the subscription. **Verify:** die with lives remaining → scene reloads. Die on the last life → game over appears. The second case does not work today. ## Slice 13 — delete the dead surface - The four never-emitted signals (`LevelRestarted`, `EnemyDamaged`, `SkillActivated`, `SkillDeactivated`). - Of the eleven emitted-but-unheard: keep the ones the republisher now feeds, delete the speculative ones. - Rename `EventBus.SkillUnlocked` — it collides with `SkillUnlockerComponent.SkillUnlocked`, and every apparent subscriber is bound to the component's. - `SaveSystem.DeleteSave`, `SpeedRunManager.StopTimer`, `AchievementManager.IsAchievementUnlocked`, `Constants.CoinsGroup` — zero callers each. - `SpeedRunManager`: `StopTimer` carries a "save personal best" comment for logic that never existed. Best times are the Profile's job now; delete the comment and the dead method. --- ## Order dependencies ``` 0 ──> 1 ──> 2 ──┐ 3 ──┼──> 5 ──> 6 ──> 9 ──> 10 ──> 11 ──> 12 ──> 13 4 ──┘ ↑ 7 ───┘ 8 ───┘ ``` 2, 3 and 4 are independent of each other after 1. 7 and 8 can be done any time after 5, and both must precede 9. 11, 12 and 13 each require 10.