[dayfine]

Depends on #264 (base bdsp/test-portability, which targets #226); the diff shows only this change. Companion: dayfine/PokemonSimulation#75.

Checkpoint 01.03 (Oreburgh Mine to Roark) now loads its 15-event plan from an authored JSON file instead of a C++ list, with route, poses, save policy and executor behavior unchanged.

• StoryData/bdsp-mine-01.03.plan.json is the source of truth; bdsp-mine-01.03.md beside it keeps the lab evidence behind the route. PokemonSimulation keeps only a pinned regression snapshot.
• PokemonBDSP_AutoStory_StoryData.* reads the shared envelope and checks BDSP's save policy; oreburgh_mine_to_roark() becomes one loader call. git diff --color-moved=dimmed-zebra --color-moved-ws=allow-indentation-change origin/bdsp/test-portability...bdsp/checkpoint-json shows the removed C++ list apart.
• CI replays this checkout's JSON through nine pinned simulator scenarios (pin cf365e17); there is no byte-equality check between the repos.
• CMake rejects a story JSON with a syntax error at configure time; schema and semantics are checked only by the tests. Moving that check into pa_embed_text_files so map files get it too is a follow-up (TODO in CMakeLists.txt).

Testing

• PokemonBDSP::StoryData suite passes on macOS with the headless runner (6 cases: registration, event order, save policy and map-derived anchors, plus five malformed-input rejections); it also passed on Mac and Windows CI earlier.
• The CI replay step passes all nine scenarios (run 42540127290).
• Inherited from the base, not from this PR: the Linux optional<TilePos> abort and two Windows Grid sim: 01.03 failures.
• Not checked: detector or controller behavior on a console. No map, profile or save changed.

🤖 Generated with Claude Code

https://claude.ai/code/session_013VUubhHeYSMGSSDXZkKZss

[dayfine]

Reviewed SHA: 4e734d9

Verdict

NEEDS_REWORK

The 01.03 data itself is correct: I found no behavior drift from the sequence the console passed. The two blockers are about the CI pin and about the cost of iterating on 01.03 while it is still being fixed on the console.

What I checked

• Merge into current #226 (b7b74431c): git merge-tree against origin/bdsp/autostory-mine-floaroma-wip is clean. #226's commits since 0738cd55b (d71d6b50f..b7b74431c) touch Bag.cpp, Events_Oreburgh.cpp, Talks.cpp, RouteRecords.cpp (01.04 rows only) and StoryMaps_Tests.cpp. They do not touch StoryTable.cpp or Pokemon/AutoStory/, so 01.03 at #226's tip is exactly the 0738cd55b row.

• 01.03, event by event: I compared the JSON against the removed C++ (row/route/step_into/legs/to_walls/anchored/talk/scene/heal helpers) and against the pinned map resource 4de7742a (pokemon_center_doorstep = (46, 21), OreburghMine.roark = (18, 28)). All of these match:

  • the start pose ({47,21},{46,21} S, set);
  • all 15 ids and kinds, in order;
  • every why string (checked by script);
  • the town/to_wall/to_wall/counted_wall variants;
  • both leg lists (9 legs in and 10 out, tile for tile);
  • every declared_after map, tile, facing and certainty, including below_roark = (18, 29) N and the out-of-mine set {46,59},{46,58};
  • talk with no pose, and the scene with an empty button;
  • RELOAD_AND_RETRY, with saves_in "Oreburgh City" now set from JSON instead of start_save_cards().

  The explicit "direction": "north" on the north steps reads back as the default. No drift from the 14:17-14:23 sequence.

• Executor path: the two new sim cases run make_story_segment(story_checkpoints(), FakeGame, …) and run_segment. That is the same table, segment builder and engine PokemonBDSP_AutoStory.cpp:237 uses on the console, with the FakeGame executor that every existing story sim uses. It is not a parallel path, and it still does not prove the real BDSP executor (as the README says).

• CI at this SHA (dispatched runs):

  • Mac and Windows Clang: both new sim cases and all six StoryData cases pass.
  • Ubuntu: the authored replay step passes 9/9. The test binary then aborts (std::optional<TilePos> assert, exit 134) before the new sim cases start.
  • The remaining failures are the same as on the base: the abort and the two Grid sim: 01.03 … failures also fail on #264 at 504bb704f (runs 42534193326 and 42534193150).

• Code health against origin/bdsp/test-portability: 0 failures (advisories only).

• headless runner: git merge-tree with origin/upstream/headless-tests is clean. That branch only adds CMakeLists lines, and nothing here flows into it. The pokesim CI steps are fork-only, as #226's map-pack step already is.

Blockers

1. .github/workflows/cpp-ci-serial-programs-base.yml:104: the pinned PokemonSimulation SHA 678ee80 is not on any ref. dayfine/PokemonSimulation#74 was rewritten: the same subject is now a1a3d0d, and 678ee80 is not an ancestor of refs/pull/74/head (90fed78). #74 was then squash-merged to main as cf365e1. CI can still fetch the dangling commit today, but GitHub can garbage-collect it at any time, and then the ubuntu test job fails at checkout. The JSON at cf365e1:data/story/bdsp-mine-pilot/bdsp-mine-01.03.plan.json is byte-identical, so repin to cf365e1 (on main) and re-run the step.

2. cpp-ci-serial-programs-base.yml:107-127 and StoryData/bdsp-mine-01.03.plan.json: moving 01.03 now makes the active console loop slower and more fragile. 01.03 is the checkpoint being fixed on lab-B. After this PR, a one-leg or one-tile fix touches:

   • the JSON here;
   • the byte-identical copy in PokemonSimulation data/story/bdsp-mine-pilot/, plus its cases.json when counts change (the 37/38 wall counts are asserted there);
   • a new PokemonSimulation commit on a ref;
   • the SHA pin in this workflow.

   That is two repos in lockstep, or Ubuntu CI goes red with a byte-mismatch assert that names no field. With the C++ rows, the same fix was one edit in one file. The byte check also makes line endings matter: the JSON is LF in a CRLF codebase, and an agent following the "keep CRLF" rule breaks the gate.

   Two smaller costs come with the move:

   • Map-derived poses become literals. The C++ derived below_roark from map_point(MINE, "roark") and the start and heal poses from pokemon_center_doorstep. Now (18, 29) and (46, 21) are hard-coded (JSON lines 10, 98, 113, 168, 190), so a map fix no longer reaches 01.03 by itself.
   • A JSON typo fails at a different point. It used to be a compile error. Now it is a ParseException thrown from the static story_checkpoints() the first time the program panel builds its checkpoint list. That breaks every checkpoint, not just 01.03. Test mode catches it; a lab build that skips tests does not.

   Fix (pick one, then say which in the PR):

   • (a) Keep 01.03 in C++ until it passes 3/3 on the console, and pilot the JSON path on a stable row (e.g. 01.02b).
   • (b) Make one repo the source of truth. Arduino-Source's JSON is authoritative, and PokemonSimulation pins an Arduino-Source SHA and reads the file from there. Drop the byte-equality assert from this workflow so a lab fix lands in one PR.

   The lab build itself is fine: pa_embed_text_files globs StoryData/*.json with CONFIGURE_DEPENDS, so an edit re-runs CMake on the next ninja build and the JSON is embedded. No deploy copy is needed.

Major

3. The removed lab evidence has no home. StoryTable.cpp loses the roughly 45-line 01.03 rationale and the per-step comments: the warp-column mapping, the hall walked on counted taps, lab-B 2026-11-08 09:46/10:39/11:55/12:03/13:50, the column-46 choice and the Hiker zone. StoryData/README.md:13 says "the source #226 comments record the … evidence", but after this PR those comments exist only in git history. JSON cannot carry them. For the checkpoint being iterated, this is the context the next fix needs. Move it into a per-checkpoint sidecar (e.g. StoryData/bdsp-mine-01.03.md, step by step) or into the README.

4. PokemonBDSP_AutoStory_StoryData_Tests.cpp:43-62: "authored mine row replaces the table entry" compares the JSON with itself. story_checkpoints() now builds 01.03 from the same embedded file (StoryTable.cpp:697), so story_checkpoint_json(*row) != story_checkpoint_json(loaded) can never fail. Together with the hard-coded 15 ids, it checks only that the file parses and has those ids. It would not have caught a wrong tile or leg count against #226. Rename it to what it checks, or, while the old row still exists, add a one-off check of the JSON against the old C++ row. The README's "common-record round trip" and "Data tests check … policy/card fields" overstate this test the same way.

Minor / style

5. PokemonBDSP_AutoStory_StoryData.cpp:20: read_failure(const std::string& value) should take std::string_view by value, per the project's string_view rule. (The + value in the message then needs std::string(value).)
6. PokemonBDSP_AutoStory_StoryTable_Tests.cpp:277: the StoryData tests are registered from inside add_tests_AutoStory_StoryTable. Register them in PokemonBDSP_Tests.cpp next to the other add_tests_AutoStory_* calls, so the table tests' header does not pull in another suite.
7. The file headers in StoryData.{h,cpp} and StoryData_Tests.cpp are collapsed to two lines, and StoryData_Tests.h has no From: line. Use the repo's 5-line /* Title\n *\n * From: …\n *\n */. These files also have no blank lines between includes, namespaces and functions, unlike their neighbours.
8. StorySimulation_Tests.cpp:513 ("Data migration of #226 at 0738cd5") and StoryData/README.md:3 name a PR and a SHA in source; both go stale on the next 01.03 fix. Say what the test checks instead.
9. SerialPrograms/CMakeLists.txt:245: the new block leaves three blank lines before # Create binary target.
10. SourceFiles.cmake: splitting the CoordEvents.h … Maps.cpp line is a harmless fix outside this PR's scope. Mention it in the description or drop it.
11. The PR body is stale: "Full native CI is pending" and "Python CI is running". Native CI ran on dispatch: the new cases pass on Mac and Windows Clang, and the failures are the base's. Update it, and note the inherited Ubuntu abort and Grid sim: 01.03 failures as pre-existing on #264.

Findings: 2 blockers, 2 major, 7 minor.

[dayfine]

Reviewed SHA: 3aad410

Verdict

APPROVED

Rework iteration 2. Delta reviewed: 146ef0d, dbdfbcc, 973d6ed, 3aad410 (the base merge aabb574 is excluded). The JSON plan itself is byte-unchanged since 4e734d9, so the event-by-event check against the old C++ row from the prior review still holds. Both blockers are resolved. What is left is minor.

Prior findings

1. Dangling PokemonSimulation pin: fixed. .github/workflows/cpp-ci-serial-programs-base.yml:104 now pins cf365e1722de…. The compare API reports cf365e1...main = ahead/behind_by 0, so the pin is an ancestor of main.
2. Two-repo lockstep for 01.03: fixed (option b). The byte-equality assert is gone. The replay step (base.yml:107-137) stages this checkout's JSON with its own sha256 into a temporary binding and runs the nine pinned scenarios against it. StoryData/README.md names Arduino-Source as the authoritative source. The .gitattributes change was added and then reverted in 3aad410, so the net change is nothing and line endings no longer matter to the gate. On the remaining cost: a route edit that changes a scenario expectation (e.g. the 37/38 wall counts in cases.json) still needs a PokemonSimulation commit and a repin. The README says so, and that is now real regression feedback rather than mirroring. The step passed at 973d6ed (run 42540127290, "Replay the authoritative Arduino mine plan": success).
   • Map-derived literals: addressed by check_map_anchors (PokemonBDSP_AutoStory_StoryData_Tests.cpp:77-97). It checks the start, below_roark, roark_smashes_the_rock and heal_oreburgh tiles against map_point(...). I confirmed that the JSON's tiles satisfy it: start/heal {47,21},{46,21}, Roark (18,29).
   • JSON typo failing at static init: mitigated by the configure-time syntax preflight (SerialPrograms/CMakeLists.txt:243-252). pa_embed_text_files adds each JSON to CMAKE_CONFIGURE_DEPENDS (cmake/EmbedTextFiles.cmake:28), so a content edit re-runs the preflight on the next ninja build. Schema errors are still runtime-only, and the README says to run tests first.
3. Lab evidence lost: fixed. StoryData/bdsp-mine-01.03.md keeps the warp-column mapping, counted hall taps, the lab-B timestamps (11-07 20:18; 11-08 09:46/10:39/11:55/12:03/13:50), the column-46 choice and the Hiker zone, step by step.
4. Self-comparing test: fixed. The case is renamed to "mine resource is registered with ordered events and save policy". The comparison is labelled a wiring check (:102-103), and the map-anchor check adds a real check that does not depend on the JSON. The README no longer claims a round trip.
5. read_failure(std::string_view): fixed (StoryData.cpp:20).
6. Registration moved to PokemonBDSP_Tests.cpp:38: fixed.
7. 5-line file headers and blank-line spacing: fixed in all four files.
8. PR/SHA references in source: fixed (StorySimulation_Tests.cpp:513, README).
9. Extra blank line in CMakeLists: fixed.
10. Out-of-scope SourceFiles.cmake split: dropped. The net diff against the base adds only the four StoryData lines.
11. PR body: partly fixed. It now records option (b), the new pin and the inherited base failures. It still reads as a progress log ("Fresh Linux CI is pending", "Windows Clang CI is compiling"), and a detached paragraph about 3aad410 is appended at the end. Once CI settles, trim it to the final state. It is also well past the 2-5 sentence house style.

New code

Correctness: no issues found. apply_execution's "save card exactly when reload-and-retry" check and the five rejection cases are consistent. The staging path fix (.resolve()) is needed because relative_to(source) is called on a path under a resolved tempfile dir.

Style: all touched C++/CMake/YAML files are fully CRLF (0 bare LF). There is no std::filesystem, and new control flow uses if (...){ with braced multi-line bodies. Code health against origin/bdsp/test-portability: 0 failures, advisories only (check_registration is 27 lines). Note: my agents checkout could not fast-forward, so the linter may be slightly stale.

Minor:

• M1. PokemonBDSP_AutoStory_StoryData_Tests.cpp:82,92,105,109,122: the failure messages say what was expected but not what was read. The repo convention asks for both, e.g. print the actual tiles from tiles_of(...) for the anchor mismatches and the actual id list for the order check.
• M2. SerialPrograms/CMakeLists.txt:243-252: the JSON syntax preflight is inlined for the BDSP story folder only. It belongs in pa_embed_text_files, which already reads every file and would then cover the map resources too, without a second read. This is optional and fine as a follow-up.

CI at this SHA

Fork checks that ran on the push: Code Health, Tabs, Python bindings and BDSP map resources pass. Story data codec and Runtime map reader are queued. Dispatched native runs: Ubuntu at 973d6ed (42540127290) passed the replay and is building; Windows Clang at 146ef0d (42539856876) is building. The commits after those two SHAs change only whitespace, the replay path, .gitattributes and the README. Inherited failures (the Ubuntu optional<TilePos> abort and the two Windows Grid sim: 01.03 cases) are confirmed on the base, per the prior review.

Findings: 0 blockers, 0 major, 3 minor (M1, M2, and the PR body from #11).