fix(quest): close the defects the loyalty gating deep review found - #57
Merged
josephjang merged 2 commits intoSep 10, 2026
Merged
Conversation
A deep review of PR #55 across ten angles. Forty findings fixed, four refuted on re-verification, six put back as steering decisions and answered. Build clean in Debug and Release; 1653 tests pass, from a 1563 baseline. The defects share a shape: one rule written down in more than one place, with nothing tying the copies together. Every fix collapses the copies rather than correcting them one by one. - **The badge re-walked the engine's precedence by hand.** It re-derived which gate was holding a quest, in an order copied from GetStatus with nothing connecting the two. Reordering the engine would have left every test green while the badge named a requirement the player did not have to clear next. GetStatus now reports the gate it stopped at as a QuestGate, and the badge is a switch over that answer. A coupling test walks all ten gates. - **The edition gate collapsed two rules into one bool**, so a quest the player was barred from by the edition it excludes showed the name of the edition it requires. The walk now reports which of the two failed. - **Trader data was never loaded on a normal launch.** Nothing else loads the Traders table, and DatabaseUpdated only fires after an actual download, so every trader name would have read as the English nickname for a whole session and then changed under the player when an update landed. The quest load now loads it, before the rows that reference it. - **The badge ordering rule existed three times.** It is applied once now, at load, by QuestDbService.SortIntoBadgeOrder, and each row carries the NormalizedName it sorts by. FirstUnmetTraderLoyalty is "the first unmet entry", and the detail pane is a plain projection. - **A blank TraderName dropped the row**, which fails OPEN: a publish with an empty nickname would have silently un-gated the quest and hidden the trader from the drawer. It is a display value, so it falls back. A published level above the app's ceiling is kept and warned about, for the same reason. - **TraderLoyaltyLevels.Entries handed out its backing store.** One cast in any caller could add or clear entries in place, and on the shared Empty instance that would corrupt every snapshot built from Defaults for the life of the process. It returns a copy, and the clamp moved into the value type so no door into it can leave a level out of range. - **The roster folded trader ids case-insensitively** while the entered levels are keyed ordinally, so two ids differing only in case would have built one button whose level the gate could not read back. - **The Requirements section was three mechanisms** (two TextBlocks and an ItemsControl, two bools and a three-term OR). One list over one view model now, and the level line's third copy of the met rule routes through QuestProgressService. - **The drawer's loyalty controls left MainWindow** as a passive TraderLoyaltyPanel; the window keeps the subscriptions so the teardown guard still applies, and its two loyalty handlers share one coalescer, collapsing an eight-repaint fan-out to one. - **Documentation accuracy**: stale gate and event counts in four source files, the code guide's "eight events", and two spec claims the shipped code contradicts. Two decisions traded a mechanical guarantee for a process one, and both are recorded with what they leave open: the loyalty ceiling stays an app constant, so a level-5 publish reaches older builds as a permanently locked quest with CI on the publish PR as the only guard; and the six-way TableExistsAsync duplication is tracked by #56 rather than refactored, cross-referenced from a comment at the duplication site.
…view Part 2 of the trader loyalty guides, pairing with the code guide the same review corrected. Eight chapters, three labs and a comprehension gate. The chapters follow the shape the findings share: one rule written down in more than one place, with nothing holding the copies together. The badge that re-walked the engine's precedence, the one bool covering two edition rules, the Traders table nothing loaded, the guard that failed open on a blank nickname, the ordering rule in three places, and the value type that handed out its own store. The last chapter is what stayed open: the four refuted findings, and the two decisions that chose a process guarantee over a mechanical one. The labs run the real decision logic. Lab A drives a quest that fails more than one gate at once and compares the old hand-copied badge with the one that reads the reported gate. Lab B is the loader's keep-or-drop policy and which way each choice fails. Lab C is the ordering rule, in three places and then in one, with the arrival order as the control. Verified: both checkers pass (the STE writing rules, not the full dictionary), a node driver over the DOM shim walks every control combination and asserts 1434 outcomes against the shipped C# rather than against the lab, and a headless render pass covers 1440 wide and 390.
josephjang
added a commit
that referenced
this pull request
Sep 11, 2026
The skill shipped without the `.claude/commands/deliver.md` that makes a repository skill reachable from Claude Code, so `/deliver` did not exist while CLAUDE.md told readers to invoke it. The rest are gaps the review of the same PR found. - Add `.claude/commands/deliver.md`, in the shape of the existing three - Gitignore `/PLAN-*.md` at the repo root. The per-run plan file otherwise keeps `git status` dirty for the whole run, which stops `/create-pr` on its uncommitted-changes prompt and contradicts the plan's own exit criterion that the working tree is clean - Open PR A as a draft at the end of step 1, so the guide in step 2 cites a real number instead of a guess or three later amendments. This is what the PR-numbers trap was warning about, left unresolved - Define the path where the deep review finds nothing: no part 2 guide and no PR B, with the review's outcome recorded in PR A's body instead - Point the body models at PRs #55 and #57, whose sections are the ones the checklists list, and ask PR A's body to name its guide file the way PR B's already does - Add the trap about telling a subagent to wait, and say that the deep review is the expensive step, not only the slow one Build 0 warnings, 0 errors; non-E2E suite 1606 passed.
josephjang
added a commit
that referenced
this pull request
Sep 11, 2026
Phase 4 of the 1.1 roadmap (PRs #55 and #57) ran a sequence that worked and was not written down anywhere: plan file, slices, code guide, PR A, a deep review on a stacked branch, its own guide, PR B. This makes it repeatable. The skill does not make product decisions. It starts from an approved PRD and spec pair and sequences the work that implements them, so /design-product-spec still owns everything before it. Three rules carry most of the value, and each was learned by not doing it: - Record the build and test baseline before the first edit. Every count reported later is a delta from a number nobody wrote down otherwise. - Write the failing test before the fix, for the defect the phase exists to remove, and put the red-then-green fact in the commit body. - Run the deep review in its own session and worktree, on a branch stacked on PR A. A session that just wrote the code cannot supply the one thing the review is for. The Traps section is the part that will age best: PR numbers that guides cite before the PR exists, blaming your own diff for a failure that also fails on main, flake hunting with filtered output, leading slashes mangled through a terminal bridge, session limits on long runs, and manual verification steps the data makes unreachable. Each cost time once. - .agents/skills/deliver/SKILL.md and references/workflow.md - references/plan-template.md, the plan file the workflow produces - CLAUDE.md points at it from the decision-docs section
josephjang
added a commit
that referenced
this pull request
Sep 11, 2026
The skill shipped without the `.claude/commands/deliver.md` that makes a repository skill reachable from Claude Code, so `/deliver` did not exist while CLAUDE.md told readers to invoke it. The rest are gaps the review of the same PR found. - Add `.claude/commands/deliver.md`, in the shape of the existing three - Gitignore `/PLAN-*.md` at the repo root. The per-run plan file otherwise keeps `git status` dirty for the whole run, which stops `/create-pr` on its uncommitted-changes prompt and contradicts the plan's own exit criterion that the working tree is clean - Open PR A as a draft at the end of step 1, so the guide in step 2 cites a real number instead of a guess or three later amendments. This is what the PR-numbers trap was warning about, left unresolved - Define the path where the deep review finds nothing: no part 2 guide and no PR B, with the review's outcome recorded in PR A's body instead - Point the body models at PRs #55 and #57, whose sections are the ones the checklists list, and ask PR A's body to name its guide file the way PR B's already does - Add the trap about telling a subagent to wait, and say that the deep review is the expensive step, not only the slow one Build 0 warnings, 0 errors; non-E2E suite 1606 passed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #55. Base is
feat/quest-loyalty-gating, so the diff here is onlythe review fixes.
A
/deep-reviewof #55 across ten angles. Forty findings fixed, four refutedon re-verification, six returned as steering decisions and answered. The run
was killed twice by session limits and resumed from its checkpoint both times.
Interactive code guide:
docs/2026-09-quest-loyalty-gating-deep-review-guide.html(8 chapters, 3 labs, comprehension gate). It pairs with the part 1 guide that
#55 adds, and which this PR also corrects.
The shape they share
Every defect is one rule written down in more than one place, with nothing
holding the copies together. A test over one copy passes. A test over the other
copy passes. Nothing tests that the two say the same thing, because in the
source they are two unrelated pieces of code that happen to encode one rule.
So most of these fixes are deletions.
The badge re-walked the engine's precedence
QuestStatuscollapses causes on purpose: level, karma and loyalty all answerLevelLocked, and three gates all answerUnavailable. The badge existed tosay which, and it did so by asking the same predicates again in an order copied
by hand.
Reorder the engine and every test stays green. The engine tests assert statuses,
which do not move; the badge tests chose settings where one gate was unmet.
Neither asked whether the badge names the gate the walk stopped at.
GetStatusnow reports aQuestGate, and the badge is a switch over it. Theprecedence exists once. A coupling test walks all ten gates and fails if the
badge is reverted to its own rule.
One bool for two edition rules
IsEditionRequirementMetfolded "requires an edition you lack" and "bars anedition you own" into one false. The badge then looked for a required edition
and printed its name, so a player barred by the excluded edition read
EOD-an edition they already owned. The walk now reports which rule failed.
The Traders table nothing loaded
TraderDbServicereloads onDatabaseUpdated, which fires only after an actualdownload, and the only other caller is a dialog a player may never open. So
every trader name read as the English nickname for a whole session, then
changed under the player when an hourly update landed. Korean and Japanese users
saw English throughout. The quest load now loads it, before the rows that
reference it.
No unit test found this, because every test that needed trader data loaded it
itself.
Dropping a row failed open
A requirement row with a blank nickname was dropped. The gate compares the
trader id and the nickname is a display value, so the row was usable - and
dropping it removed a gate the game still applies, making the quest available to
everybody. It is kept now, with a fallback for the name.
A published level above the app's ceiling is the same question with the opposite
answer: kept, because the gate is right and the input has fallen behind, and
warned about so the publish is visible in the log.
One ordering rule, in three places
FirstUnmetTraderLoyaltyranked as it scanned, the detail pane sorted its owncopy, and the loader's
ORDER BYwas a fourth weaker version. Three copies isthree chances for the badge to name one trader while the list under it puts
another first.
SortIntoBadgeOrderruns once at load; every consumer is a plainwalk over that order.
The value type that handed out its store
TraderLoyaltyLevels.Entriesreturned its ownSortedDictionarybehind aread-only interface. One cast could mutate it - and on the shared
Emptyinstance that corrupts every snapshot built from
Defaultsfor the life of theprocess. It returns a copy now, and the clamp moved into the type so
WithandFromare the only doors in.Also
keyed ordinally, so two ids differing only in case would have built one button
whose level the gate could not read back.
TextBlocks and anItemsControl, two bools and a three-term OR). One list over one view modelnow, and the level line's third copy of the met rule routes through
QuestProgressService.MainWindowas a passiveTraderLoyaltyPanel; the window keeps the subscriptions so the teardown guardstill applies, and its two loyalty handlers share one coalescer, collapsing an
eight-repaint fan-out to one.
part 1 guide's "eight events", and two spec claims the shipped code
contradicts.
Tests
1563 before, 1653 after (+90): 1606 non-E2E and 47 E2E.
New suites:
ProfileDrawerFitTests,MainWindowTeardownTests,QuestRequirementLinesTests,TraderLoyaltyPanelTests, plus sharedLoyaltyFixtures.Tests proven inert and tightened: the badge suite no longer asserts a precedence
of its own (that would be a second copy of the rule) and instead runs the walk
and asserts the badge against the gate it reports; the loader suite gained the
keep-or-drop cases in both directions.
Verification
Both guides re-verified after the review edited them: checkers pass, and their
node drivers assert 2331 and 1434 outcomes against the shipped C#.
Residuals
Two decisions traded a mechanical guarantee for a process one. Both are
recorded in the spec with what they leave open, and both are worth a second
opinion at merge:
reaches installed builds without an app release, and those builds show the
quest permanently locked behind a stepper that stops at 4. A content test
fails on the publish PR, so the guarantee is that CI runs and somebody reads
it. The loader now warns at load as a second signal.
TableExistsAsynchas six copies (andColumnExistsAsyncthree), trackedby refactor(db): collapse the duplicated TableExistsAsync and ColumnExistsAsync helpers #56 rather than refactored, cross-referenced from a comment at the
duplication site. The seventh copy gets written by whoever adds the next
*DbService.Carried forward from #55: the seasonal quests stay ungated until upstream
publishes their loyalty rows, and the Collector page is phase 5.
Read closely: the
TraderLoyaltyPanelextraction is a plain class ratherthan a
.xamlpair, becauseProfileDrawerFitTestsparses the drawer as looseXAML and a
UserControlcannot resolve there. That was the fixer's judgementcall and it is flagged as such.
Merge order
#55 first. GitHub retargets this to
mainwhenfeat/quest-loyalty-gatingis deleted; confirm the base readsmainbeforemerging.