From c5b8b7a7268906ae7d9d36602588a1907aeaeab7 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sat, 27 Jun 2026 21:58:26 -0400 Subject: [PATCH] NEO-149: re-review after code review follow-ups --- docs/reviews/2026-06-27-NEO-149.md | 34 ++++++++++++++++++------------ 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/docs/reviews/2026-06-27-NEO-149.md b/docs/reviews/2026-06-27-NEO-149.md index 06fd154..d143329 100644 --- a/docs/reviews/2026-06-27-NEO-149.md +++ b/docs/reviews/2026-06-27-NEO-149.md @@ -1,25 +1,30 @@ # 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` +**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 with nits** +**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. 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. +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 | +| `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** (orchestrator + encounter wiring + quest-fixture reset) | +| `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 | @@ -31,24 +36,25 @@ 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. +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`. +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` (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).~~ **Done.** Final deny uses `player_not_writable`, `instance_not_active`, or `instance_not_found` instead of `invalid_ids`. +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 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.~~ **Done.** `IssueAndCompleteOnce` setup helper; replay test Act/Assert only. -- 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. +- 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 ```bash dotnet test NeonSprawl.sln -dotnet test NeonSprawl.sln --filter "FullyQualifiedName~ContractCompletionOperations|FullyQualifiedName~EncounterCompletionOperationsTests.TryCompleteAndGrant_ShouldApplyContractBundle" +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.