Repository navigation
Code defects surfaced by the docstring pass (PRs #87 to #99) #100
Description
The docstring pass in PRs #87 through #99 documented what the code does rather than changing it. Along the way the coders and reviewers found code that disagrees with its own intent. This issue collects those findings so none is lost. The docstrings in each PR already describe the current behaviour; the fix for each item is a code change plus the docstring update that follows from it.
Each entry names the PR that reported it and the file. A comment is added for each PR as it merges, repeating the defects that PR names.
| PR | Where | Defect |
|---|---|---|
| #87 | crawl/exploration.py, check_fatigue |
The guard is any(...) over living members, so a character who joins the party partway through an unrested stretch is never marked fatigued until the next Rest resets the counter. Documented as the current behaviour; probably unintended. |
| #88 | crawl/battle.py, _party_movement |
BattleDeclaration.move accepts withdraw, but the resolver branches only on close, fighting_withdrawal, and retreat. A withdraw declaration passes validation and moves nobody. |
| #89 | core/spells.py, cast_from_scroll |
Validation runs with the raw reader and resolution with the scroll's caster level. A 6th-level magic-user reading a magic missile scroll must supply three targets and hits all three, and the two spells with a per-level range are range-checked at the reader's level. Fix shape: build the _ScrollReader before validate_cast. |
| #91 | core/treasure.py, generate_treasure, generate_treasure_entries, generate_unguarded_treasure |
The tier argument is validated only when a roll reaches a magic item, so tier="bogus" silently yields a coins-only hoard. generate_magic_item shows the pre-check shape. |
| #93 | core/character.py, Character.literacy |
Nothing in osrlib reads the field. The docstring now says so; either a rule should consume it or the field is dead. |
| #97 | crawl/dungeon.py, DungeonState.discovered_features |
Declared and never written or read. Secret doors record discovery on DoorState.discovered; features go to found_traps and found_tricks. Removing it is a schema decision. |
| #99 | crawl/battle.py, crawl/encounter.py, EncounterGroup.surrendered |
The surrender path is declared end to end (message template, battle.side.surrendered code, a dozen reads of the flag, the surrendered defeat outcome in end_encounter), but no code path ever sets the flag. |
PRs #90, #92, #94, #95, #96, and #98 reported no code defect.
Activity
mmacy commentedon Sep 14, 2026
Merged #87. Code defects it names:
check_fatigueincrawl/exploration.pyguards onany(...)over living members, so a character who joins mid-stretch is never marked fatigued until the nextRestresets the counter. Documented as current behaviour; probably unintended.mmacy commentedon Sep 14, 2026
Merged #88. Code defects it names:
BattleDeclaration.moveacceptswithdraw, but_party_movementincrawl/battle.pyresolves onlyclose,fighting_withdrawal, andretreat, so awithdrawdeclaration passes validation and moves nobody.mmacy commentedon Sep 14, 2026
Merged #89. Code defects it names:
cast_from_scrollincore/spells.pyvalidates with the raw reader and resolves at the scroll's caster level: a 6th-level magic-user reading a magic missile scroll must supply three targets and hits all three, and the two per-level-range spells are range-checked at the reader's level.mmacy commentedon Sep 14, 2026
Merged #90. Code defects it names: This PR names no code defect.
mmacy commentedon Sep 14, 2026
Merged #91. Code defects it names:
generate_treasure,generate_treasure_entries, andgenerate_unguarded_treasureincore/treasure.pyvalidatetieronly when a roll reaches a magic item, sotier="bogus"silently yields a coins-only hoard.mmacy commentedon Sep 14, 2026
Merged #92. Code defects it names: This PR names no code defect.
mmacy commentedon Sep 14, 2026
Merged #93. Code defects it names:
Character.literacyincore/character.pyhas no consumer anywhere in osrlib; the docstring now says no rule reads it.mmacy commentedon Sep 14, 2026
Merged #94. Code defects it names: This PR names no code defect.
mmacy commentedon Sep 14, 2026
Merged #95. Code defects it names: This PR names no code defect.
mmacy commentedon Sep 14, 2026
Merged #96. Code defects it names: This PR names no code defect.
mmacy commentedon Sep 14, 2026
Merged #97. Code defects it names:
DungeonState.discovered_featuresincrawl/dungeon.pyis declared and never written or read; secret doors record onDoorState.discoveredand features go tofound_trapsandfound_tricks.mmacy commentedon Sep 14, 2026
Merged #98. Code defects it names: This PR names no code defect.
mmacy commentedon Sep 14, 2026
Merged #99. Code defects it names:
EncounterGroup.surrenderedis declared end to end (message template,battle.side.surrenderedcode, reads acrosscrawl/battle.pyandcrawl/encounter.py, thesurrendereddefeat outcome inend_encounter), but no code path ever sets the flag.mmacy commentedon Sep 14, 2026
Two more declared-but-unwired surfaces from the pass, in the same family as
surrendered,discovered_features, andliteracy. The rule from AGENTS.md about code kept "just in case" applies to all of them: wire each or delete it._break_party_wardsincrawl/battle.pyskips any ward whose effect params carryunbreakable. Nothing in the shipped spell data sets that param; the only occurrence in the checkout is the read site (Docstrings: crawl battle and encounter #90).GameClock.advancereturns the list ofBoundaryCrossingvalues it crossed. Nothing in the library reads them:EffectsLedger.advancecallsclock.advance, discards the result, and resolves every round itself (Docstrings: core basics #96). Either the ledger should consume the crossings or the return value is for callers only and the type could be simpler.Delete four surfaces the engine declared and never set (#100)
mmacy commentedon Sep 15, 2026
Every row is resolved: check_fatigue (#124), withdraw (#118), cast_from_scroll and the treasure tier (#117), and literacy, discovered_features, the surrender path, and the unbreakable parameter (#127). GameClock.advance keeps returning the crossings it made, since the return value is public API for callers.
Delete four surfaces the engine declared and never set (#127)