fix(planning): "Append to beat notes" replaced them instead (#454)

claude-bot commented

2026-08-31 01:58:53 +00:00

Contributor

Closes #454 (HIGH — active data loss).

The defect

PrepResult.handleAppend composed its append from sessions.find(...)?.beat_notes, read off the campaign session list — which has never carried that field. current was therefore always "", so the "Append" button wrote the generated prep sheet alone, over whatever the GM had written, and then reported "Appended to beat notes."

A GM with an evening of hand-written prep who generates a sheet and clicks Append loses all of it and is told the opposite. There is no undo and no version history for beat_notes.

The same root cause left BeatPlanner's editor opening blank regardless of what was stored, so typing into that empty box and saving replaced the notes too.

Why it could not simply be read

beat_notes appeared in no read schema anywhere — the column was writable and never readable. That is why both call sites invented an empty string: there was no value to fetch.

It is now on SessionResponse, gated on is_gm exactly as transcript already is. That gate is load-bearing: GET /api/sessions/{id} is member-readable, and beat notes are the GM's plan for the evening — a player who can read them has been handed the session's surprises.

Putting beat_notes on SessionListItem was the smaller change and removes a round trip. Rejected: that list is served to every campaign member with no per-field gating, so it would have published the GM's plan to the table. A test pins that decision, so if someone later adds it there for convenience it fails rather than quietly leaking.

I checked every endpoint with response_model=SessionResponse before doing this — all of them are get_session_for_gm or require_gm except the one member-readable detail endpoint, which is the one that applies the gate.

What changed

Both call sites read the real value. A failed read aborts the append with the error rather than composing against "" — a button labelled Append must never be able to replace. BeatPlanner disables its editor and Save while the notes are loading, for the same reason. Replace still replaces, which is what its label says.

Why this survived

There was already a green test for it:

const sessionsWithNotes = [{ ...SESSIONS[0], beat_notes: "Existing notes." }, SESSIONS[1]];
// ...
expect(planningApi.updateBeatNotes).toHaveBeenCalledWith("c1", "s1", "Existing notes." + "\n\n" + formatted);

It handed PrepResult a sessions array with beat_notes on it and asserted the composition. The composition logic was correct — the test simply fabricated a source shape that has never existed, so it passed while the feature was broken in production. This is exactly the pattern #441 ("Audit the test suite for tests that cannot fail") exists to hunt, and worth remembering as a concrete example when that work starts: the test was not weak, it was testing against invented input.

Rewritten against fetchSession, plus a new test asserting a failed read writes nothing at all.

Verification

Mutation-checked: dropping the is_gm gate fails the player-visibility test.

1,453 backend tests pass (was 1,450), 454 frontend (was 452). Lint clean at pinned ruff 0.4.4; eslint clean across src/ apart from one pre-existing warning in a file this PR does not touch.

Also in here

Corrects a stale comment I left in CampaignPlanning.jsx during #403 — it claimed the session list does not expose updated_at and that it is fetched per selection. True of an earlier draft, not of what shipped.

🤖 Generated with Claude Code

Closes #454 (HIGH — active data loss). ## The defect `PrepResult.handleAppend` composed its append from `sessions.find(...)?.beat_notes`, read off the **campaign session list** — which has never carried that field. `current` was therefore always `""`, so the "Append" button wrote the generated prep sheet **alone**, over whatever the GM had written, and then reported *"Appended to beat notes."* A GM with an evening of hand-written prep who generates a sheet and clicks Append loses all of it and is told the opposite. There is no undo and no version history for `beat_notes`. The same root cause left `BeatPlanner`'s editor opening **blank** regardless of what was stored, so typing into that empty box and saving replaced the notes too. ## Why it could not simply be read `beat_notes` appeared in **no read schema anywhere** — the column was writable and never readable. That is why both call sites invented an empty string: there was no value to fetch. It is now on `SessionResponse`, **gated on `is_gm`** exactly as `transcript` already is. That gate is load-bearing: `GET /api/sessions/{id}` is member-readable, and beat notes are the GM's plan for the evening — a player who can read them has been handed the session's surprises. Putting `beat_notes` on `SessionListItem` was the smaller change and removes a round trip. **Rejected**: that list is served to every campaign member with no per-field gating, so it would have published the GM's plan to the table. A test pins that decision, so if someone later adds it there for convenience it fails rather than quietly leaking. I checked every endpoint with `response_model=SessionResponse` before doing this — all of them are `get_session_for_gm` or `require_gm` except the one member-readable detail endpoint, which is the one that applies the gate. ## What changed Both call sites read the real value. A **failed read aborts the append** with the error rather than composing against `""` — a button labelled Append must never be able to replace. `BeatPlanner` disables its editor and Save while the notes are loading, for the same reason. Replace still replaces, which is what its label says. ## Why this survived There was already a green test for it: ```js const sessionsWithNotes = [{ ...SESSIONS[0], beat_notes: "Existing notes." }, SESSIONS[1]]; // ... expect(planningApi.updateBeatNotes).toHaveBeenCalledWith("c1", "s1", "Existing notes." + "\n\n" + formatted); ``` It handed `PrepResult` a sessions array with `beat_notes` on it and asserted the composition. The composition logic was correct — the test simply fabricated a source shape that has never existed, so it passed while the feature was broken in production. This is exactly the pattern **#441** ("Audit the test suite for tests that cannot fail") exists to hunt, and worth remembering as a concrete example when that work starts: the test was not weak, it was testing against invented input. Rewritten against `fetchSession`, plus a new test asserting a failed read writes nothing at all. ## Verification Mutation-checked: dropping the `is_gm` gate fails the player-visibility test. **1,453 backend tests pass** (was 1,450), **454 frontend** (was 452). Lint clean at pinned ruff 0.4.4; eslint clean across `src/` apart from one pre-existing warning in a file this PR does not touch. ## Also in here Corrects a stale comment I left in `CampaignPlanning.jsx` during #403 — it claimed the session list does not expose `updated_at` and that it is fetched per selection. True of an earlier draft, not of what shipped. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot added 1 commit

2026-08-31 01:58:54 +00:00

fix(planning): "Append to beat notes" replaced them instead (#454 )

CI / Backend lint (ruff) (pull_request) Successful in 1m8s

Details

CI / Bot/backend version sync (pull_request) Successful in 54s

Details

CI / Docker image build (pull_request) Successful in 57s

Details

CI / Summarisation accuracy eval harness (stub provider) (pull_request) Successful in 2m15s

Details

CI / Frontend tests, audit, and build (pull_request) Successful in 2m51s

Details

CI / Bot tests and audit (pull_request) Successful in 2m54s

Details

CI / Backend migration, tests, and audit (pull_request) Successful in 10m45s

Details

dd425a9b96

PrepResult composed its append against sessions.find(...)?.beat_notes, read
off the campaign session list — which has never carried that field. current
was therefore always "", so Append wrote the generated sheet alone over the
GM's prep and reported "Appended to beat notes." There is no undo and no
version history for beat_notes.

Same root cause left BeatPlanner's editor opening blank whatever was
stored, so typing there and saving replaced the notes too.

beat_notes was in no read schema at all — the column was writable and never
readable, which is why both call sites invented an empty string. It is now
on SessionResponse, gated on is_gm exactly as transcript is: the session
detail endpoint is member-readable and beat notes are the GM's plan for the
evening. Putting it on SessionListItem would have been the smaller change
and was rejected for that reason; a test pins the decision so nobody adds
it there for convenience later.

Both call sites now read the real value, and a failed read aborts the
append with the error rather than composing against "". Replace still
replaces, which is what its label says.

Why this survived: the existing test handed PrepResult a sessions array
with beat_notes on it and asserted the composition. It was green the whole
time. The logic was right; the fabricated source shape never existed —
exactly the pattern #441 is meant to hunt. Rewritten against fetchSession,
plus a test that a failed read writes nothing.

Mutation-checked: dropping the is_gm gate fails the player-visibility test.
1,453 backend and 454 frontend tests pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

rbrooks added 1 commit

2026-08-31 02:18:22 +00:00

fix: generated content stops overwriting GM authorship (#400 , #401 )

CI / Docker image build (pull_request) Successful in 10s

Details

CI / Bot/backend version sync (pull_request) Successful in 22s

Details

CI / Backend lint (ruff) (pull_request) Successful in 1m2s

Details

CI / Summarisation accuracy eval harness (stub provider) (pull_request) Successful in 1m29s

Details

CI / Frontend tests, audit, and build (pull_request) Successful in 1m58s

Details

CI / Bot tests and audit (pull_request) Successful in 2m10s

Details

CI / Backend migration, tests, and audit (pull_request) Successful in 8m15s

Details

4dfe2730b6

Two paths, one principle, and they compound — a canonical-name pick
regenerated the summary and fired the fan-out that rebuilt the storyline, so
one click could destroy both. Neither keeps history, so the GM's work was
not merely replaced; it was unrecoverable.

#400 — the storyline was rebuilt from raw session summaries on every
transcription, summary edit and canonical-name pick, for any session in the
campaign, and `storyline.body = new_body` was unconditional. Adds
campaign_storylines.manually_edited_at (c1d2e3f4a5b7), set by the manual
edit endpoint; the automatic trigger now leaves an authored body alone.
The GM's explicit regenerate still rebuilds and clears the mark — which is
the first behaviour full_regenerate has ever had, since both of its
branches compute the identical body.

#401 — picking a canonical name re-summarised the session and committed
straight over the GM's corrections, reintroducing the very mistakes they
had just fixed, from an action whose stated purpose is naming. Removed
entirely: the name lands in canonical_names, which every later generation
reads, so nothing is gained by rewriting prose already approved.

Three existing tests asserted the removed behaviour. One required the
summary to be refreshed — the defect written down as the specification —
and is inverted. Two used the canonical-name route as a vehicle to check
system-prompt threading into summarise; their vehicle is gone, and the note
left in their place records that summarise's remaining call site
(process_audio) has no call-site test rather than hiding the gap.

Mutation-checked. The first attempt did not hold: the removed code sat
behind `if llm_cfg is not None`, and the suite configures no LLM, so the
canonical-name tests passed with or without the fix. They now seed a config
and a summariser that would fire, and reverting the fix fails two of them.
The storyline tests likewise needed task_session patched to the test's own
session, or the task found no sessions and returned before reaching the
guard.

1,458 backend tests pass. Migration verified up and down over the full chain.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

rbrooks force-pushed fix/454-beat-notes-append from 4dfe2730b6

CI / Docker image build (pull_request) Successful in 10s

Details

CI / Bot/backend version sync (pull_request) Successful in 22s

Details

CI / Backend lint (ruff) (pull_request) Successful in 1m2s

Details

CI / Summarisation accuracy eval harness (stub provider) (pull_request) Successful in 1m29s

Details

CI / Frontend tests, audit, and build (pull_request) Successful in 1m58s

Details

CI / Bot tests and audit (pull_request) Successful in 2m10s