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-27-NEO-149.md

4.9 KiB
Raw Blame History

Code review — NEO-149 contract completion encounter wiring

Date: 2026-06-27
Scope: Branch NEO-149-e7m4-06-contract-completion-encounter-wiring — commits 8d9d02f..bcf6fd2 vs origin/main
Base: origin/main

Verdict

Approve with nits

Summary

NEO-149 adds ContractCompletionOperations.TryCompleteOnEncounterClear — deliver-then-mark contract completion on encounter clear, following the NEO-128 quest precedent and NEO-148 router. The orchestrator resolves the active instance, matches encounterTemplateId, calls TryDeliverContractCompletion, marks the instance complete, and appends an outcome audit row. Encounter wiring invokes the hook after encounter loot and quest objective wiring; contract payout failure does not block encounter success (result discarded). Contract store/registry dependencies are plumbed through EncounterCombatWiring and AbilityCastApi. QuestFixtureRequest.resetContractInstanceIds clears instance, delivery, and outcome rows for Bruno/dev reset loops. Five unit tests, one DI integration test, and one encounter-completion wiring test cover match/mismatch, delivery deny, idempotent replay, and combined encounter + contract loot. Implementation plan reconciliation, server/README.md, and E7.M4 module anchor are updated. Server-only slice; client work explicitly deferred to NEO-153/NEO-154 after NEO-151 HTTP. Full suite: 904 tests passed locally.

Documentation checked

Path Result
docs/plans/NEO-149-implementation-plan.md Matches — AC checklist complete; shipped reconciliation aligns with diff
docs/plans/E7M4-pre-production-backlog.md (E7M4-06) Matches — orchestrator + encounter wiring in scope; HTTP Bruno loop deferred to NEO-151
docs/decomposition/modules/E7_M4_ContractMissionGenerator.md Matches — NEO-149 completion wiring anchor added
docs/decomposition/modules/documentation_and_implementation_alignment.md Partially matches — E7.M4 row includes NEO-144–NEO-148 but not E7M4-06 / NEO-149 yet
server/README.md Matches — contract completion operations section, wiring order, reason codes, fixture reset
Client counterpart (NEO-153, NEO-154) N/A — explicitly out of scope; no prototype-complete claim

Register/tracking table should be updated for E7.M4 (NEO-149) after merge.

Blocking issues

None.

Suggestions

  1. Update implementation alignment register — Add E7M4-06 / NEO-149 landed to the E7.M4 row in docs/decomposition/modules/documentation_and_implementation_alignment.md (orchestrator + encounter wiring + quest-fixture reset), consistent with NEO-144–NEO-148 entries.

  2. Quest-fixture test for contract reset — QuestFixtureOperations now clears contract instance, delivery, and outcome rows via resetContractInstanceIds, but QuestFixtureApiTests has no parallel to PostQuestFixture_ShouldClearRewardDeliveryAndAudit_WhenResetQuestIdsProvided. A test that issues a contract, completes it, resets via fixture, and asserts stores are empty would lock the NEO-148 follow-on reset path.

  3. TryMarkComplete failure reason code — When delivery succeeds but TryMarkComplete fails without leaving a completed snapshot, the orchestrator returns invalid_ids (line 122 in ContractCompletionOperations.cs). QuestStateOperations.TryMarkComplete uses richer fallback codes (NotActive, UnknownPlayer). Consider aligning deny semantics in a follow-on if store-failure paths become testable; not blocking for prototype (same deliver-then-mark stuck-state risk as quest path when mark loses after delivery).

Nits

  • Nit: Idempotent replay after successful completion returns no_active_contract (instance no longer active) rather than hitting the defensive completed snapshot branch at lines 59–61. Behavior is correct — no double grant — but the reason code differs from the plan’s “completed replay” wording.
  • Nit: TryCompleteOnEncounterClear_ShouldBeIdempotent_WhenReplayed performs success assertions on the first completion inside Arrange; prefer moving first-call verification to Assert after Act for strict AAA, or extract a shared setup helper.
  • Nit: Plan lists optional EncounterCombatWiring end-to-end issue → cast → contract completed; only compile-fix call-site updates landed. Acceptable given integration + encounter completion tests cover the path.
  • Nit: Bruno Reset contract instance via quest fixture.bru uses placeholder ci_000… id — fine until NEO-151 wires issue → clear → GET; helper resetContractInstances in bruno-dev-fixture-helper.js is the useful piece for that story.

Verification

dotnet test NeonSprawl.sln
dotnet test NeonSprawl.sln --filter "FullyQualifiedName~ContractCompletionOperations|FullyQualifiedName~EncounterCompletionOperationsTests.TryCompleteAndGrant_ShouldApplyContractBundle"

Manual Godot QA: none (server engine per plan). Bruno issue → clear → GET loop: NEO-151.