58 lines
5.1 KiB
Markdown
58 lines
5.1 KiB
Markdown
# Code review — NEO-140 (E7M3-08)
|
|
|
|
**Date:** 2026-06-17
|
|
**Scope:** Branch `NEO-140-e7m3-quest-http-rep-gate-projections` — commits `367f894` … `1dedc5f` vs `main`
|
|
**Base:** `main`
|
|
|
|
**Follow-up:** Suggestions and actionable nits below are **done** (strikethrough + **Done.** / **Addressed.** / **Deferred**).
|
|
|
|
## Verdict
|
|
|
|
**Approve with nits**
|
|
|
|
## Summary
|
|
|
|
This branch lands **E7M3-08**: additive HTTP projections so clients can read faction gate rules on world quest definitions and reputation grant lines on quest-progress `completionRewardSummary`, without local standing math. Implementation mirrors **NEO-129** / **NEO-115** patterns — optional `factionGateRules` on world GET (omitted when empty), required `reputationGrants` array when a completion summary exists (sourced from `RewardDeliveryEvent.GrantedReputation` at commit time). Shared mappers mean **QuestAcceptApi** embedded rows pick up rep lines automatically. Integration tests extend existing quest API suites; Bruno adds operator-chain completion assertions and extends quest-definitions / gather-intro bru. **821** server tests pass locally. Server-only story; Godot HUD remains **NEO-142**. Low merge risk — read-only projections, schema version stays **1**.
|
|
|
|
## Documentation checked
|
|
|
|
| Path | Result |
|
|
|------|--------|
|
|
| `docs/plans/NEO-140-implementation-plan.md` | **Matches** — kickoff decisions, AC checklist, reconciliation section, and file list align with the shipped diff. |
|
|
| `docs/plans/E7M3-pre-production-backlog.md` | **Matches** — E7M3-08 server scope; client counterpart NEO-142 explicitly deferred. |
|
|
| `docs/decomposition/modules/E7_M3_FactionReputationLedger.md` | **Matches** — Status line notes E7M3-08 landed (NEO-140). |
|
|
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M3 row updated with quest HTTP projection links. |
|
|
| `docs/decomposition/modules/module_dependency_register.md` | **N/A change** — no register row edit required beyond module doc / alignment table. |
|
|
| `docs/manual-qa/NEO-140.md` | **Removed post-merge** — server-only story; manual QA reserved for client-facing work (story-end decision). |
|
|
| `server/README.md` | **Matches** — Quest-definitions and quest-progress sections document new fields with NEO-140 cross-links. |
|
|
|
|
## Blocking issues
|
|
|
|
None.
|
|
|
|
## Suggestions
|
|
|
|
1. ~~**Grid-contract completion rep (+10 Rust Collective) still untested end-to-end.** Plan §prototype targets and open-questions table adopt deferring grid-contract `reputationGrants` Bruno until a hand-in flow helper exists; operator-chain coverage is the primary AC and is solid. When grid-contract completion lands in Bruno or integration helpers, add an assertion for `[{ factionId: prototype_faction_rust_collective, amount: 10 }]` so the third freeze row in the plan table is locked like operator-chain.~~ **Done.** `QuestProgressApiTests.GetQuestProgress_ShouldReturnGridContractReputationGrant_WhenAcceptedAfterOperatorChain`; `Accept grid contract after operator chain.bru` asserts embedded `completionRewardSummary.reputationGrants`.
|
|
|
|
2. ~~**Optional wire-level omission test for `factionGateRules`.** `QuestDefinitionsWorldApiTests` asserts deserialized `FactionGateRules` is null on non-gated quests; Bruno asserts JSON `undefined` for gather intro and operator chain. A small integration test that reads raw JSON (or `JsonDocument`) and confirms the property key is absent on non-gated rows would close the loop without Bruno — low priority given Bruno coverage.~~ **Done.** `GetQuestDefinitions_ShouldOmitFactionGateRulesJsonProperty_WhenQuestHasNoGates` uses `JsonDocument` to assert key absence on gather intro and operator chain.
|
|
|
|
## Nits
|
|
|
|
- ~~Nit: `docs/manual-qa/NEO-140.md` links client follow-up as `[NEO-142](NEO-142.md)` — relative path may not resolve from `docs/manual-qa/`; prefer `NEO-142.md` (same folder) or `../manual-qa/NEO-142.md` from other docs.~~ **Done.** Linear issue link until `NEO-142.md` exists.
|
|
- ~~Nit: `Get quest progress after operator chain complete.bru` pre-request performs a quest-progress GET before the named GET (mirrors gather-intro bru idempotency pattern) — fine; seq **12** places it after gather-intro complete (**8**); confirm collection order if Bruno runs fail mid-spine.~~ **Addressed.** Pre-request helpers reset quest/inventory state per bru; seq order does not share mutable spine between these requests.
|
|
- Nit: Client fixture JSON in `client/test/quest_progress_client_test.gd` (`_completed_with_reward_summary_json`) omits `reputationGrants` — acceptable until **NEO-142**; Godot client ignores unknown keys today. **Deferred** to NEO-142.
|
|
|
|
## Verification
|
|
|
|
```bash
|
|
cd /Users/don/neon-sprawl && dotnet test NeonSprawl.sln
|
|
```
|
|
|
|
Manual (Bruno — no separate manual QA doc for this server-only story):
|
|
|
|
- `cd server/NeonSprawl.Server && dotnet run`
|
|
- Bruno: `quest-definitions/Get quest definitions`
|
|
- Bruno: `quest-progress/Get quest progress after gather intro complete`
|
|
- Bruno: `quest-progress/Get quest progress after operator chain complete`
|
|
- Regression: `faction-standing/Get faction standing after operator chain`, `quest-progress/Accept grid contract after operator chain`
|