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

5.3 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..eb6a4fe vs origin/main
Base: origin/main
Follow-up: Re-review after code-review follow-ups (68ba267–eb6a4fe).

Verdict

Approve

Summary

NEO-149 adds ContractCompletionOperations.TryCompleteOnEncounterClear — deliver-then-mark contract completion on encounter clear, following the NEO-128 quest precedent and NEO-148 router. Encounter wiring invokes the hook after encounter loot and quest objective wiring; contract payout failure does not block encounter success. Contract store/registry dependencies are plumbed through EncounterCombatWiring and AbilityCastApi. QuestFixtureRequest.resetContractInstanceIds clears instance, delivery, and outcome rows for Bruno/dev reset loops.

Follow-up commits address all prior review suggestions: E7.M4 alignment register updated, PostQuestFixture_ShouldClearContractStores_WhenResetContractInstanceIdsProvided added, TryMarkComplete failure path returns player_not_writable / instance_not_active / instance_not_found instead of invalid_ids, and idempotent replay test refactored with IssueAndCompleteOnce. Ancillary analyzer fixes in encounter wiring (debug log guard) and craft API (CraftResult XML docs, MapIoRows return type, deny empty IO arrays test) keep the branch clean; craft changes are orthogonal to contract completion but low risk.

Seven contract-related tests (5 unit + 1 integration + 1 encounter wiring) plus 1 quest-fixture API test cover the story. Full suite: 906 tests passed locally.

Documentation checked

Path Result
docs/plans/NEO-149-implementation-plan.md Matches — AC checklist complete; shipped reconciliation aligns with diff (test count still cites 904 — minor drift)
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 Matches — E7.M4 row includes E7M4-06 / NEO-149
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 updated for E7.M4 (NEO-149).

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. Done. E7.M4 row + plan link updated (68ba267).

  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. Done. PostQuestFixture_ShouldClearContractStores_WhenResetContractInstanceIdsProvided (68ba267).

  3. TryMarkComplete failure reason code — When delivery succeeds but TryMarkComplete fails without leaving a completed snapshot, the orchestrator returns invalid_ids. QuestStateOperations.TryMarkComplete uses richer fallback codes. Consider aligning deny semantics. Done. Final deny uses player_not_writable, instance_not_active, or instance_not_found (68ba267).

Nits

  • Nit: Idempotent replay after successful completion returns no_active_contract (instance no longer active) rather than hitting the defensive completed snapshot branch. Behavior is correct — no double grant — but the reason code differs from the plan’s “completed replay” wording. Deferred — acceptable for prototype.
  • 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. Done. IssueAndCompleteOnce setup helper (68ba267).
  • Nit: Plan lists optional EncounterCombatWiring end-to-end issue → cast → contract completed; only compile-fix call-site updates landed. Deferred — 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.
  • Nit: Follow-up commits include craft API analyzer/doc fixes unrelated to E7M4-06 scope; harmless but could have been a separate chore commit.

Verification

dotnet test NeonSprawl.sln
dotnet test NeonSprawl.sln --filter "FullyQualifiedName~ContractCompletionOperations|FullyQualifiedName~QuestFixtureApiTests.PostQuestFixture_ShouldClearContractStores"

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