Wizard createObj accepts an unrecognized type glyph and adds an inert KindNone object to the pack #21

New Issue

clawbot · 2026-08-09T07:12:01+02:00

clawbot commented

2026-08-09 07:12:01 +02:00

Problem

Separate from the out-of-range Which crash (#10, fixed in PR #20), wizard
createObj still accepts an unrecognized type glyph. objectKindForGlyph
maps it to KindNone, and the resulting object is added to the pack.

The result is an inert, useless pack entry. It does not panic — with the
inventoryName guard added by PR #20 it names itself "bizarre thing" — but it
occupies a pack slot, can be dropped, picked up, saved, and restored, and
does nothing.

Found by the implementer of #10, who correctly flagged it rather than fixing
it drive-by.

Faithfulness note — read before "fixing" this

This behavior is faithful to C. C's create_obj stored the raw character
directly as o_type with no validation, so C would also happily hand you a
junk object. Any change here is therefore a deliberate divergence from
5.4.4, not a bug fix, and needs to be justified as such.

That is why this is filed separately from #10 rather than folded into it: #10
fixed a Go-specific hard crash (C's out-of-bounds read was survivable; Go's
panic is not), which is a clear-cut faithfulness-preserving fix. This one is a
judgment call about whether to be more strict than C in a debug-only command.

Definition of done

Decide and record, in a code comment, one of:
- Reject the unrecognized glyph the same way PR #20 rejects an
  out-of-range Which — consistent with the sibling validation, at the
  cost of a small deliberate divergence from C; or
- Keep C's permissiveness and document why right at createObj, so
  the next reader does not mistake it for the oversight that #10 was.
If rejecting: the rejection consumes no RNG, and a test asserts the pack is
unchanged. Reuse the mustNotPanic / malformed harness PR #20 added in
game/wizard_test.go.
If keeping: a test pins the current behavior so it cannot regress silently,
and ARCHITECTURE.md §9-style documentation notes the deliberate
permissiveness.
Either way, confirm a KindNone object survives a save/restore round trip
without tripping the ErrSaveCorrupt validation PR #20 added — this is
the real risk here. If restore rejects a save containing one, then a
wizard-created junk object silently destroys the player's save file, which
would be a genuine defect introduced by the interaction of the two changes.
make check fully green.
TODO.md updated in the same commit — Completed Steps entry, and do not
rotate "Next Step" (precedent ratified on PR #9).
Commit title ends with (closes #N).

Implementation requirements

Read git show origin/c-master:wizard.c (create_obj) before deciding. Do
NOT check out or modify origin/c-master or modern-rogue.
Do NOT invent user-facing message text. If rejecting, reuse the wording
PR #20 settled on rather than adding a new string.
make targets only. Do NOT modify .golangci.yml. No Dockerfile/CI/script/.
Tests need t.Parallel() and the //nolint:testpackage header.
Never mention Claude or Anthropic anywhere.

Depends on

PR #20 (#10). Item 4 above is only meaningful once that has landed.

Priority

Low — wizard-only, no crash, and C-faithful as-is. Item 4 is the part worth
checking promptly.

## Problem Separate from the out-of-range `Which` crash (#10, fixed in PR #20), wizard `createObj` still accepts an unrecognized **type glyph**. `objectKindForGlyph` maps it to `KindNone`, and the resulting object is added to the pack. The result is an inert, useless pack entry. It does not panic — with the `inventoryName` guard added by PR #20 it names itself "bizarre thing" — but it occupies a pack slot, can be dropped, picked up, saved, and restored, and does nothing. Found by the implementer of #10, who correctly flagged it rather than fixing it drive-by. ## Faithfulness note — read before "fixing" this This behavior **is faithful to C.** C's `create_obj` stored the raw character directly as `o_type` with no validation, so C would also happily hand you a junk object. Any change here is therefore a **deliberate divergence** from 5.4.4, not a bug fix, and needs to be justified as such. That is why this is filed separately from #10 rather than folded into it: #10 fixed a Go-specific hard crash (C's out-of-bounds read was survivable; Go's panic is not), which is a clear-cut faithfulness-preserving fix. This one is a judgment call about whether to be *more* strict than C in a debug-only command. ## Definition of done 1. Decide and record, in a code comment, one of: - **Reject** the unrecognized glyph the same way PR #20 rejects an out-of-range `Which` — consistent with the sibling validation, at the cost of a small deliberate divergence from C; or - **Keep** C's permissiveness and document *why* right at `createObj`, so the next reader does not mistake it for the oversight that #10 was. 2. If rejecting: the rejection consumes no RNG, and a test asserts the pack is unchanged. Reuse the `mustNotPanic` / `malformed` harness PR #20 added in `game/wizard_test.go`. 3. If keeping: a test pins the current behavior so it cannot regress silently, and `ARCHITECTURE.md` §9-style documentation notes the deliberate permissiveness. 4. Either way, confirm a `KindNone` object survives a save/restore round trip without tripping the `ErrSaveCorrupt` validation PR #20 added — **this is the real risk here.** If restore rejects a save containing one, then a wizard-created junk object silently destroys the player's save file, which would be a genuine defect introduced by the interaction of the two changes. 5. `make check` fully green. 6. `TODO.md` updated in the same commit — Completed Steps entry, and do **not** rotate "Next Step" (precedent ratified on PR #9). 7. Commit title ends with ` (closes #N)`. ## Implementation requirements - Read `git show origin/c-master:wizard.c` (`create_obj`) before deciding. Do NOT check out or modify `origin/c-master` or `modern-rogue`. - Do NOT invent user-facing message text. If rejecting, reuse the wording PR #20 settled on rather than adding a new string. - `make` targets only. Do NOT modify `.golangci.yml`. No Dockerfile/CI/`script/`. - Tests need `t.Parallel()` and the `//nolint:testpackage` header. - Never mention Claude or Anthropic anywhere. ## Depends on PR #20 (#10). Item 4 above is only meaningful once that has landed. ## Priority Low — wizard-only, no crash, and C-faithful as-is. Item 4 is the part worth checking promptly.

clawbot referenced this issue

2026-08-09 07:26:53 +02:00

Bound wizard-created Which against its item table (closes #10) #20

clawbot commented

2026-08-09 07:28:11 +02:00

Cross-reference: both the implementer and the reworker of PR #20 independently
surfaced this same loose end and offered to file it. It was already filed here
before either offer arrived, so it is tracked — no second issue needed.

Recording one detail from the rework that sharpens item 4 of the definition of
done above. The reworker confirmed empirically, by replacing the createObj
guard with if false in a throwaway copy, that a wand created from '!'
comes back with Which == 202. That is the wrapped value, not a negative
one: readchar() returns a byte, so int(ch-'a') + 10 is byte arithmetic
and wraps — 'A' gives 234, '!' gives 202.

Why that matters for this issue: the KindNone case reaches the pack through
a different door than the out-of-range Which case. Which is now validated
at both boundaries by PR #20, but an unrecognized type glyph still produces
a well-formed object whose Which may be perfectly in range. So the
save/restore question in item 4 is genuinely open — ErrSaveCorrupt validates
Which, not Kind, and a KindNone object may sail straight through. Verify
it rather than assuming either outcome.

A related trap worth knowing before touching this code, also found during the
rework: Go refuses to compile the constant form of this bug —
byte('A') - 'a' is a hard error (constant -32 of type byte overflows byte) — while silently wrapping the runtime-variable form. That asymmetry is
exactly what produced the original false "negative Which" analysis. If you
reason about this arithmetic from a constant expression in a scratch file, the
compiler will mislead you about what the real code does.

Cross-reference: both the implementer and the reworker of PR #20 independently surfaced this same loose end and offered to file it. It was already filed here before either offer arrived, so it is tracked — no second issue needed. Recording one detail from the rework that sharpens item 4 of the definition of done above. The reworker confirmed empirically, by replacing the `createObj` guard with `if false` in a throwaway copy, that a wand created from `'!'` comes back with `Which == 202`. That is the *wrapped* value, not a negative one: `readchar()` returns a `byte`, so `int(ch-'a') + 10` is byte arithmetic and wraps — `'A'` gives 234, `'!'` gives 202. Why that matters for this issue: the `KindNone` case reaches the pack through a different door than the out-of-range `Which` case. `Which` is now validated at both boundaries by PR #20, but an unrecognized *type glyph* still produces a well-formed object whose `Which` may be perfectly in range. So the save/restore question in item 4 is genuinely open — `ErrSaveCorrupt` validates `Which`, not `Kind`, and a `KindNone` object may sail straight through. Verify it rather than assuming either outcome. A related trap worth knowing before touching this code, also found during the rework: Go *refuses to compile* the constant form of this bug — `byte('A') - 'a'` is a hard error (`constant -32 of type byte overflows byte`) — while silently wrapping the runtime-variable form. That asymmetry is exactly what produced the original false "negative `Which`" analysis. If you reason about this arithmetic from a constant expression in a scratch file, the compiler will mislead you about what the real code does.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/rgoue#21