Skip to content

Reviewer's guide — xsofy, Monday 27 July #182

Closed

Description

@mparrett

Six PRs are ready for review, all green and mergeable. Three more are parked and want no attention yet. None of the six depend on each other, so they can go in any order and in any combination.

Sizes are small: four of the six are under 70 lines.

Why clear these now

Individually none of them is urgent. Together they are the repairs standing between us and the two things we actually want to be working on.

Three are foundations #40 will stand on. The world, bestiary, and depth progression framework needs run-scoped state, percept-driven creatures, and a status system that behaves. #178 restores :title and :quest-item across floor transitions, and :quest-item has no reader yet, so today it loses its value in silence and hands the bill to the first feature that wants it. #179 stops entity-fov returning an empty set as though it were a computed result, which is the first wall a percept-driven creature AI walks into. #166 fixes the double status tick and introduces a :no-decay contract that every new status will use. None of this is visible in play today. All of it is in the way of an ecology.

Two clear the shell ahead of #181. The UI design first pass surveys tools/xsofy-shell.html, and findings 4 and 7 put the touch pane in scope.
#167 and #173 both edit that file. Landing them first means the epic works
against merged state instead of reconciling with two open PRs.

One compounds, and is already reviewed. #163 caches the lg binary and Playwright's chromium, so every PR after it is cheaper than every PR before it. It needs an approval, not a review pass. See the note under Suggested order.

Running any of these

The same commands CI runs, so results should match:

let-go xsofy/test/run.lg                      # the test suite
let-go -b /tmp/xsofy main.lg                  # compile smoke
LG=let-go python3 xsofy/test/check_smoke.py   # layer-1 runtime smoke

For the two shell PRs, load the built page with ?mode=dev. Both changes sit behind the dev switcher, which players never see.

Suggested order

#163 is not in the list below, because it needs a click rather than a read. @nnunley reviewed it on 20 July, the GOPATH and cache-path questions were answered on the thread with output from the actual runs, and he closed with "I guess it's fine as it is." It still reads as REVIEW_REQUIRED only because a commenting review does not satisfy the one-approval rule. Approving it costs nothing and lands the change that makes every later PR cheaper.

For the rest: correctness first, then the shell polish. If you only have time for one, make it #178, which closes a filed issue and fixes something a player can see.

  1. fix(world): carry run-scoped fields across floor transitions #178: world fields dropped on floor change (closes World fields set after make-world are dropped on floor transition (:title, :quest-item) #177)
  2. fix(percept): treat an empty :fov as uncomputed, not as a result #179: FOV sentinel (sibling of fix(world): carry run-scoped fields across floor transitions #178; see the merge note below)
  3. fix(status): stop generic decay from racing the web struggle branch #166: spider webs untrampled (closes Spider webs no longer trampled #161)
  4. shell: add 'off' to the force-touch cycle (auto/force/off) #167 and feat(shell): mobile keypad rearrangement + font touch-ups #173: shell, independent of everything, quick

The six

#178: carry run-scoped fields across floor transitions (+139/-46)

Closes #177, follows #176. main.lg assocs :title, :scheme, and :quest-item onto the world after make-world; descending drops two of the three, so a player who dies below depth 1 gets a tombstone with no game title.

The cause matters more than the symptom here: two places hand-listed what survives a transition (change-floor's carry-over assoc and restore-floor's 21-key world literal), and only :scheme made it onto both.

Look at: whether the new mechanism actually removes the two-list problem or just corrects today's entries. That is the difference between fixing this bug and fixing the class.

Verify: the PR body has a before/after table across d1 → d2 → d1-revisit.

#179: treat an empty :fov as uncomputed, not as a result (+57/-1)

Follows #176, independent of #177. entity-fov decided the FOV was cached by asking whether the key existed. #176 seeded :fov to #{} in empty-world, so a skeleton-built world now carries the key with nothing in it, and every "can the player see X" question answers no.

Look at: the choice of sentinel. Reading a container's shape as a signal is what broke the first time.

Note: nothing reaches this today. percept is required only by balance.lg, which runs on already-FOV'd output. This is a trap laid for whoever writes the first percept-based property test, so judge it on the invariant rather than on observable breakage.

#166: stop generic decay racing the web struggle branch (+52/-3)

Closes #161. :webbed ttl was decremented twice per turn: once by the movement struggle branches, which are the only place that tramples, and once by the generic status tick. The status always expired inside the tick, so the trample branch was unreachable. #71's ttl reduction from 3 to 2 exposed it on the player path; the monster path had never trampled since webs landed.

Look at: the :no-decay opt-out. It is a new contract for status definitions, so it is worth agreeing on before more statuses adopt it.

Verify: xsofy/test/status_test.lg. The PR body has a screenshot.

#167: add 'off' to the force-touch cycle (+32/-10)

The ▦ dev toggle was boolean: force or auto. auto is not off: portrait's media query and a coarse pointer both reveal the touch UI anyway, so there was no way to suppress it on a phone with a Bluetooth keyboard or a narrow desktop window. Adds a third state.

Look at: body.no-touch beats both reveal paths on plain specificity, with no !important. If that holds, the mechanism is clean; if it needed !important, it would be worth a second look.

Verify: load with ?mode=dev, cycle ▦ three times, confirm off releases the portrait 55dvh terminal cap and the grid refits.

#173: mobile keypad rearrangement + font touch-ups (+13/-9)

Sliced out of the #168 spike because it is independent of it. WAIT and ENTER swap so the high-frequency key is on the top row, y/n swap to match the landscape variant, a ` tile fills an empty cell so the dev console is reachable without a physical keyboard, and (/) join the a–z pane.

Look at: nothing structural; this is layout. The question is whether the new positions are right, which is a taste call best made on a phone.

#163: cache the lg binary and Playwright browsers (+64/-7)

Follow-through from the let-go CI audit. xsofy has no go.sum, so setup-go's dependency cache never engaged and every run compiled let-go from source. The binary is now cached on the resolved let-go version, with the latest lane resolving its moving tag to a concrete version first so it caches on the same footing.

Already reviewed. @nnunley raised the cache keys on 20 July, specifically ~/go against GOPATH across all three workflows and whether ~/.cache is the right shared directory. Both were answered on the thread against real run output (on Unix Go's default GOPATH is $HOME/go, actions/cache expands ~, and the run resolved ~/go/bin/let-go to /home/runner/go/bin/let-go), and the follow-up landed in edf27f8. He closed with "I guess it's fine as it is."

So this one wants an approval rather than another pass. If you would rather re-check the keys anyway, the property is that a version bump misses once and rebuilds, and nothing can serve a stale binary under a key that no longer describes it.

Merge-order notes

#178 and #179 conflict, in one line. Both add a namespace to the same require block in xsofy/test/run.lg: [xsofy.test.world-shape-test] and [xsofy.test.percept-test]. Git cannot auto-merge adjacent additions, so whichever lands second needs a one-line resolution: keep both. Nothing else about them interacts.

#167 and #173 both edit tools/xsofy-shell.html but test-merge clean, so they can go in either order without coordination.

Parked

No attention needed on these yet.

Activity

  1. mparrett commented on Sep 27, 2026

    @mparrett
    CollaboratorAuthor

    Closing: all six PRs this guide covered have merged (#163, #166, #167, #173, #178, #179).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions