don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-06-07-NEO-128.md

6.1 KiB
Raw Blame History

Code review — NEO-128 (E7M2-05)

Date: 2026-06-07
Scope: Branch NEO-128-e7m2-quest-complete-reward-wiring vs origin/main — commits 7abe7dc … da99769
Base: origin/main

Verdict

Approve with nits — deliver-then-mark wiring in QuestStateOperations.TryMarkComplete, idempotent skip, deny passthrough, and dependency threading through objective wiring and upstream ops match the adopted plan and NEO-105 precedent; tests cover the primary AC paths; risk is low (server-internal until NEO-129 HTTP projection).

Summary

NEO-128 wires RewardRouterOperations.TryDeliverQuestCompletion into QuestStateOperations.TryMarkComplete on the first-time active→completed path: bundle delivery runs before IPlayerQuestStateStore.TryMarkComplete, idempotent completed replay returns early without calling the router, and delivery denies passthrough router reasonCode values while leaving the quest active. Reward-router dependencies are threaded through QuestObjectiveWiring, GatherOperations, CraftOperations, EncounterCompletionOperations, QuestProgressApi, QuestAcceptApi, AbilityCastApi, and related call sites so wiring-driven completion exercises the same path. Five new/extended AAA integration tests lock gather-intro bundle delivery, idempotent replay (direct and wiring), and inventory-full deny on operator-chain setup; compile-fix updates propagate the expanded signatures across quest/gather/craft/encounter test harnesses. Docs (plan reconciliation, E7.M2 anchor, alignment register, backlog AC, server/README.md) are updated. A minimal Bruno health smoke was added (host boot check only).

Documentation checked

Path Result
docs/plans/NEO-128-implementation-plan.md Matches — kickoff deliver-then-mark and idempotent skip adopted; reconciliation accurate; AC checked.
docs/plans/E7M2-prototype-backlog.md (E7M2-05) Matches — AC checkboxes complete; Bruno listed as optional in backlog (health-only smoke landed).
docs/decomposition/modules/E7_M2_RewardAndUnlockRouter.md Matches — NEO-128 implementation anchor added.
docs/decomposition/modules/documentation_and_implementation_alignment.md Matches — E7.M2 row notes E7M2-05 quest-state wiring; row correctly stays Planned until NEO-132 capstone.
docs/decomposition/modules/module_dependency_register.md Matches — no register change required.
server/README.md Matches — TryMarkComplete table row and reward-router cross-ref document deliver-then-mark, idempotent skip, deny passthrough.
Full-stack epic decomposition N/A — E7M2-05 is server engine; client counterpart NEO-131 correctly deferred (blocked by NEO-129).

Register / tracking: E7.M2 correctly remains Planned until capstone NEO-132; alignment update is appropriate.

Blocking issues

None.

Suggestions

  1. Operator-chain happy-path delivery assertions — The plan lists integration coverage for “operator-chain → item + skill grants.” TryMarkComplete_ShouldDenyAndStayActive_WhenInventoryFull exercises chain quest at the operations layer (deny only). ChainQuest_ShouldAdvanceThroughMixedObjectives_WhenPrerequisitesMet completes via wiring but does not assert IRewardDeliveryStore, survey_drone_kit, or salvage +50. NEO-127 covers router apply in isolation; extending the wiring or operations happy-path test would lock end-to-end TryMarkComplete + item grant for the only bundle with itemGrants. Done. Added TryMarkComplete_ShouldDeliverOperatorChainBundle_WhenFirstCompletion; extended ChainQuest_ShouldAdvanceThroughMixedObjectives_WhenPrerequisitesMet with store, item, and skill XP asserts.

  2. Inventory-full via wiring path — Deny is tested via direct TryMarkComplete after manual chain setup. Optional: one wiring-driven test (e.g. fill bag then complete terminal inventory_has_item step) to prove QuestObjectiveWiring best-effort ignore does not mask the deny for primary ops while quest stays active for retry. Done. Added ChainQuest_ShouldStayActive_WhenBagFullOnTerminalWiringComplete (terminal advance via TryAdvanceStep while bag full; quest stays active, no store record).

Nits

  • Nit: Kickoff adopted “skip Bruno” for bundle verification, but a GET /health smoke was added under bruno/neon-sprawl-server/quest-progress/ with docs pointing to dotnet test for bundle asserts. Harmless and consistent with backlog “optional” wording; consider a one-line plan note if authors want kickoff vs backlog Bruno policy explicit. Done. Plan kickoff clarifications note health-only Bruno for pre-commit hook; functional verification via dotnet test.

  • Nit: TryCompleteStepIfSatisfied discards TryMarkComplete return value (by design per plan — wiring best-effort). Inventory-full deny therefore does not surface on gather/craft/encounter envelopes; acceptable for prototype, worth remembering for NEO-131 client surfacing.

  • Nit: Delivery succeeds then TryMarkComplete store write fails — orphan store record self-heals via already_delivered on retry (plan deferred). No action required for prototype.

  • Nit: Expanded parameter lists on TryAccept / TryAdvanceStep mirror NEO-118 threading precedent; a future facade/DI record could reduce call-site churn but is explicitly out of scope.

Verification

dotnet test server/NeonSprawl.Server.Tests --filter "QuestStateOperationsTests|QuestObjectiveWiringTests"
dotnet test server/NeonSprawl.Server.Tests --filter "FullyQualifiedName~Quests|FullyQualifiedName~GatherOperations|FullyQualifiedName~CraftOperations|FullyQualifiedName~EncounterCompletion|FullyQualifiedName~EncounterCombatWiring"

All 32 quest-state/wiring tests and 141 related tests pass (verified 2026-06-07; +2 after review follow-up).

Optional before merge:

  • Boot dev server; run Bruno GET health (quest complete reward wiring NEO-128) or curl /health — no new DI registration beyond existing reward stores.