From c40ac1e673107244eb3375b23d7726f32a61a683 Mon Sep 17 00:00:00 2001 From: prosolis <5590409+prosolis@users.noreply.github.com> Date: Fri, 24 Jul 2026 22:36:42 -0700 Subject: [PATCH] adventure: write up the five review findings left for a follow-up Same review, the half not fixed in place: the extract pre-check treating a stale snapshot as the last word, offersToUndo's party guard not covering the case its comment claims, a duplicated boss_id failing the whole war-room replace, an "already out there" that Pete can't actually know, and the siege_join check loading the entire siege to read one flag. Each one has the direction already decided and the edits and tests spelled out, so the follow-up session doesn't have to re-derive any of it. Delete the file when they're done. --- code_review_findings_w9.md | 181 +++++++++++++++++++++++++++++++++++++ 1 file changed, 181 insertions(+) create mode 100644 code_review_findings_w9.md diff --git a/code_review_findings_w9.md b/code_review_findings_w9.md new file mode 100644 index 0000000..186c37c --- /dev/null +++ b/code_review_findings_w9.md @@ -0,0 +1,181 @@ +# Code review findings + fix plan — w9-verbs-and-polish + +From a `/code-review --fix` pass on the branch diff, 2026-07-24. Two findings were +fixed in that session; the five below were left for a follow-up session, with the +direction chosen by the author. Work them in the order given — items 1/2 and 3 +touch different files and are independent, item 4 is the only one with a +gogobee-side half. + +Already fixed on this branch (don't redo): + +- `internal/web/orders.go` verdict handler — non-`ErrNoSuchAdvOrder` storage + failures now return 500 instead of 400, via a new `storage.ErrBadAdvVerdict`. + A 400 makes gogobee park the row for a human, so a transient SQLite blip used + to permanently strand a resolvable order. +- `internal/web/push_adventure.go` — `advStoryURL` / `advRunOrStoryURL` now + path-escape the guid and run id like every other URL builder does. + +--- + +## 1. Drop the extract pre-check — `internal/web/orders.go:129` + +**Why.** The comment above the pre-checks says they read a snapshot up to two +minutes behind and that "neither is allowed to be the last word", and `who.html` +promises the button works even on an idle-looking mark. But +`if haveEntry && entry.Status != "expedition"` returns 409 and no order is ever +created. A player who set out via Matrix during a lagging roster push (staleness +is tolerated up to 12 minutes) gets told they're not on an expedition for a run +gogobee would happily have ended. + +**Decision: drop it.** Same call `abandon` and `leave` already make one function +down — and the reasoning there (an *extracted* expedition is still abandonable +while its owner reads as idle) applies to extract too. Cost accepted: a genuine +mistake now comes back as a verdict rather than an instant 409. + +**Do.** + +- Delete the `case storage.AdvActionExtract:` arm. Replace it with a comment in + the shape of the `AdvActionAbandon, AdvActionLeave` one in + `resolveAdvOrderParams` — say that the snapshot is stale, that a Matrix + departure can outrun the roster push, and that `rejected_not_running` is the + honest answer. +- With extract gone the `switch req.Action` has one arm left; collapse it to an + `if req.Action == storage.AdvActionSiegeJoin` (see item 2, which rewrites the + body anyway). +- `characterName` is still needed for the insert, so keep the `RosterEntryByToken` + lookup and its error path; only the status test goes. +- Nothing to do client-side: `adventure-actions.js:28` already renders + `rejected_not_running` as "couldn't, you weren't on an expedition", which is + the string the 409 was standing in for. + +**Tests.** `TestActionPreChecksAreCourtesyOnly` in `internal/web/orders_test.go` +pins the old behaviour — its first block asserts `extract` while idle is a 409. +Flip it to 200 and rewrite the doc comment, which currently describes an +asymmetry that no longer exists (both halves are now "let it through" except the +one positive siege case). Consider renaming it to match. Its two siege blocks +stay exactly as they are. + +## 2. Make the siege_join pre-check cheap — `internal/web/orders.go:135` + +**Why.** It calls `storage.LoadSiege()`, which loads every defender row and the +whole siege history, to read one `active` flag, on a pool that is +`MaxOpenConns(1)`. Once per click, so not urgent, but it is a one-column read. + +**Keep the check itself.** Unlike a personal status, "is a boss camped outside +town" is a town-wide fact on a day-or-longer clock, so a two-minute-old copy is +almost never wrong about it — the reason item 1 goes the other way doesn't apply. + +**Do.** + +- Add `SiegeIsCamped() (active, known bool, err error)` to + `internal/storage/siege.go`, next to `LoadSiege`. One + `SELECT active FROM adventure_siege WHERE id = 1`; `sql.ErrNoRows` means + known=false. +- The known/active distinction is load-bearing and already pinned by + `TestActionPreChecksAreCourtesyOnly`: no snapshot at all must queue the order + (a fresh deploy must not have a dead button), only a snapshot that positively + says `active=0` refuses. Keep `LoadSiege`'s doc note about that on the new + function. +- Swap the call in `orders.go`. No behaviour change, so no new test beyond the + existing one continuing to pass. + +## 3. Make the war-room history replace collision-proof — `internal/storage/siege.go:116` + +**Why.** `adventure_siege_history` has `boss_id` as PRIMARY KEY and `ReplaceSiege` +uses a bare `INSERT`, inside the transaction that also writes the live boss and +the muster. Two history rows sharing a `boss_id` fail the whole replace, so the +war room freezes on the previous snapshot indefinitely with no partial +degradation. It is genuinely unclear whether `boss_id` is the siege instance or +the boss type — `SiegeBarForBoss` matches history on `boss_name` + nearest +`ended_at` and its comment says "the same boss comes back month after month", +which reads like type. + +**Decision: don't settle the question, make it survivable.** `INSERT OR REPLACE` +can never lose a row that would otherwise have landed; it just keeps the last of +a colliding pair instead of 500ing the ingest. + +**Do.** + +- `hstmt` becomes `INSERT OR REPLACE INTO adventure_siege_history (...)`. +- Comment why, at the statement: the whole table is deleted and rebuilt from + gogobee's list every push, so a duplicate key is a wire quirk and not data + loss, and a frozen war room is a worse failure than a dropped history row. +- Also note the open question in `schema.go` above the `boss_id INTEGER PRIMARY + KEY` line, so the next reader knows the key's meaning was never confirmed. + +**Tests.** New one in `internal/web/siege_test.go` alongside the existing push +tests: post a snapshot whose history carries two rows with the same `boss_id`, +assert the push succeeds and the war room shows the new live boss — i.e. the +ingest degrades to one history row rather than freezing. + +## 4. Close the party-guard hole — `internal/web/who.go:133` (+ gogobee) + +**Why.** `offersToUndo` receives `page.HasDetail`, which only means "the blob +decoded". Its comment claims the guard also catches "a gogobee too old to push +seats", but such a gogobee pushes a *valid* blob with no `party` key: `HasDetail` +is true, `Party` is nil, and the code takes the "solo ⇒ this player is the +leader" branch and shows a party **member** "Call the whole thing off" — the +exact outcome the comment says it prevents. gogobee refuses with +`rejected_not_leader`, so today's cost is a misleading offer, not a lost +expedition. + +A nil `Party` cannot be told apart from a solo run on the current wire, so this +needs a new field. **Decision: spec the wire change.** + +**Note the asymmetry before writing any code:** only the `len(party) == 0` branch +is unsafe. The `len(party) > 0` branch reads the viewer's *own* seat and is +self-evidencing — it cannot mistake a member for a leader. So gate the empty +branch alone, and the rollout costs nothing but a solo leader's party-branch +abandon in the window before gogobee ships (the `self.Resume != nil` branch below +still covers the extracted case). + +**Pete side.** + +- `whoDetail` gains `PartyKnown bool \`json:"party_known"\`` with a comment + saying what nil-vs-empty could not express. +- `offersToUndo`'s third parameter changes from `haveParty` to `partyKnown`; + pass `page.HasDetail && page.Detail.PartyKnown` at the call site (`who.go:405`) + so a blob that failed to decode is still excluded. +- Move the guard: keep `status == rosterStatusExpedition` on the outer `if`, and + put `partyKnown` on the `len(party) == 0` branch only. +- Rewrite the doc comment — it is currently wrong about what the guard buys, and + the "found by getting a fixture wrong" note should stay but now point at the + real signal. + +**gogobee side (separate repo).** Write it up as a contract the way +`adventure_ask7_equipment_mgmt.md` was: the public detail blob on the roster push +sets `"party_known": true` whenever the sheet was built by a gogobee that knows +about party seats — unconditionally, including on a solo run, and including when +the party list is omitted. It is a capability flag about the *sender*, not a fact +about the character, so it must never be conditional on there being a party. + +**Tests.** `internal/web/orders_undo_test.go` already tables this function. Add +cases: `partyKnown=false` + empty party + expedition ⇒ no abandon (the old +gogobee); `partyKnown=true` + empty party ⇒ abandon (genuine solo); +`partyKnown=false` + a party naming the viewer as member ⇒ leave still offered +(the self-evidencing branch must not regress). + +## 5. Stop asserting a fact Pete can't know — `internal/web/orders.go:210` + +**Why.** `resolveAdvOrderParams` treats `len(detail.Zones) == 0` as proof the +player is on an expedition and says "you're already out there", but an empty +offer list is equally what an out-of-date game box sends. Only reachable via a +hand-crafted request today (with no offers the picker doesn't render), so this is +message honesty, not a live bug. + +**Do.** Keep the refusal, reword it so it describes what Pete actually saw — +something like "nowhere is on offer for you right now" — and adjust the comment +above the branch, which states the "already out there" reading as fact. Then fix +the two places that echo it: the doc comment on +`TestNoZoneOffersMeansAlreadyOut` (`orders_test.go:331`) and, if you want them to +match, nothing else — `adventure-actions.js:33`'s `rejected_busy` string is +gogobee's own verdict and is correct as it stands. + +--- + +## Wrap-up + +- `go build ./...`, `go vet ./...`, `go test ./...` after each item. +- `gofmt -l` already flags `internal/storage/orders.go` on this branch (pre-existing + hand-aligned comment blocks); don't let that fool you into reformatting it. +- Delete this file once the five are done.