feat(p1): tutorial → campaign flow + scenario JSONs (acceptance #9, #20)#30
Conversation
Closes audit P1: acceptance #9 ('Tutorial completes <10 min, transitions to long-running campaign') and #20 ('Tutorial→campaign handoff works in same session'). bridge/tutorial.py — state machine + persistent store: - States: not_started → tutorial_active → tutorial_complete → campaign_active. Skip path: not_started → campaign_active. Replay path: tutorial_complete → tutorial_active. - TutorialFlow dataclass tracks state, turns_in_tutorial, started_at, completed_at; transition() raises on illegal moves. - TutorialStore atomic-writes JSON to ./saves/tutorial_state.json (separate from the kingdom save exchange so completion survives close/reopen — PRD §BS-6 no-decay). - scenario_to_load(flow) helper returns the scenario name the Unciv mod should load: 'Tutorial' until completion, then 'LongRunningCampaign'. mod/ClaudeKingdoms/scenarios/Tutorial/: - scenario.json — Tiny map, 1 human civ (The Operators), starting city Camelot, victoryConditions: ['Tutorial Complete'], tutorialFlags: graduateAfterTurn=1, expectedDurationMinutes=10 (PRD acceptance #9 budget). claudeKingdomsMeta.handoffTarget pins LongRunningCampaign as the next scenario. - Map.json — 6×6 grid, single starting position for Camelot. mod/ClaudeKingdoms/scenarios/LongRunningCampaign/: - scenario.json — Standard map, victoryConditions: [] (no forced end per PRD acceptance #18), campaignFlags: noForcedEnd=true, noDecayOnClose=true, supportMultipleSessions=true. - Map.json — 12×8 'Albion' starter map with coast / hill / forest / plains / grassland mix and Camelot positioned away from the Tutorial scenario's location so it's clearly a different world. bridge/tests/test_tutorial_flow.py — 24 tests: - Initial state is not_started; full happy path transitions - Skip path; illegal transition raises - record_turn only counts during TUTORIAL_ACTIVE - Replay tutorial after complete; campaign is terminal - Store round-trip; default filename behavior; explicit path - Missing file / corrupt file → default flow - Reset removes file - Atomic write leaves no temp files - scenario_to_load returns Tutorial / LongRunningCampaign at the right transitions - Both scenarios' .json files parse - Tutorial scenario specifies <=10 minute duration - Tutorial handoffTarget = LongRunningCampaign - Campaign scenario has no forced end + no decay flag - Tutorial map has exactly one city (Camelot) Total: 234 passing (was 210; adds 24). Note on Unciv-side completeness: scenario JSONs follow the documented Unciv format but full in-engine validation requires an Unciv runtime which we don't run in CI. The handoff state machine and contract are testable in isolation; the in-engine wiring is the final M4 milestone. Closes audit P1: acceptance #9 + #20. Refs: PR #22, kanban t_26404be3. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a tutorial-to-campaign state machine and persistence layer, along with the necessary scenario and map configurations for both modes. Feedback focused on improving the robustness of the state loading process, refining path handling logic, clarifying misleading comments about the reset mechanism, and ensuring data durability during atomic writes by incorporating fsync.
| if new_state == CAMPAIGN_ACTIVE and self.completed_at is None: | ||
| # Skipped tutorial — record completion so reset() works cleanly. | ||
| self.completed_at = now |
There was a problem hiding this comment.
The comment mentioning that completed_at is recorded so that reset() works cleanly appears to be misleading. The reset() method (line 154) simply unlinks the state file and does not depend on the completed_at timestamp. If this was intended to support a different mechanism or UI state, the comment should be updated to reflect the actual purpose.
| if p.suffix == "": | ||
| p.mkdir(parents=True, exist_ok=True) | ||
| p = p / DEFAULT_TUTORIAL_FILENAME |
There was a problem hiding this comment.
The heuristic used to distinguish between a directory and a file path (p.suffix == "") is fragile. It will incorrectly treat files without extensions as directories (creating a directory and appending the default filename) and may fail for directories that contain a dot in their name. It is safer to use p.is_dir() for existing paths or to require callers to provide the full file path.
| if p.suffix == "": | |
| p.mkdir(parents=True, exist_ok=True) | |
| p = p / DEFAULT_TUTORIAL_FILENAME | |
| if p.is_dir(): | |
| p = p / DEFAULT_TUTORIAL_FILENAME | |
| else: | |
| p.parent.mkdir(parents=True, exist_ok=True) |
| def load(self) -> TutorialFlow: | ||
| if not self.path.exists(): | ||
| return TutorialFlow() | ||
| try: | ||
| with open(self.path, "r", encoding="utf-8") as f: | ||
| data = json.load(f) | ||
| except (json.JSONDecodeError, OSError): | ||
| return TutorialFlow() | ||
| return TutorialFlow.from_dict(data) |
There was a problem hiding this comment.
The load method is not fully robust against malformed or corrupted state files. TutorialFlow.from_dict is called outside the try...except block, so any ValueError, TypeError, or AttributeError resulting from unexpected JSON types or structure will cause a crash. Additionally, catching all OSError exceptions can hide critical issues like PermissionError. It is recommended to wrap the from_dict call and catch specific schema-related exceptions.
| def load(self) -> TutorialFlow: | |
| if not self.path.exists(): | |
| return TutorialFlow() | |
| try: | |
| with open(self.path, "r", encoding="utf-8") as f: | |
| data = json.load(f) | |
| except (json.JSONDecodeError, OSError): | |
| return TutorialFlow() | |
| return TutorialFlow.from_dict(data) | |
| def load(self) -> TutorialFlow: | |
| if not self.path.exists(): | |
| return TutorialFlow() | |
| try: | |
| with open(self.path, "r", encoding="utf-8") as f: | |
| data = json.load(f) | |
| return TutorialFlow.from_dict(data) | |
| except (json.JSONDecodeError, OSError, ValueError, TypeError, AttributeError): | |
| return TutorialFlow() |
| with os.fdopen(fd, "w", encoding="utf-8") as f: | ||
| f.write(body) | ||
| os.replace(tmp_path, self.path) |
There was a problem hiding this comment.
To fulfill the "atomic write" guarantee described in the class docstring and ensure durability against system crashes or power failures, you should call f.flush() and os.fsync(f.fileno()) before closing the file and performing the os.replace() operation. This ensures the data is actually committed to disk.
try:
with os.fdopen(fd, "w", encoding="utf-8") as f:
f.write(body)
f.flush()
os.fsync(f.fileno())
os.replace(tmp_path, self.path)1 participant
Closes audit P1: acceptance #9 + #20. Tutorial state machine + persistent store; Tutorial + LongRunningCampaign scenario manifests + maps. 24 new tests; 234 total. Refs PR #22, kanban t_26404be3.