Pete/code_review_findings_w9.md

# 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.