NEO-141: add code review for faction telemetry hook sites
parent
3c1ade4185
commit
cbd8444da3
|
|
@ -0,0 +1,55 @@
|
|||
# Code review — NEO-141 (E7M3-09)
|
||||
|
||||
**Date:** 2026-06-17
|
||||
**Scope:** Branch `NEO-141-faction-telemetry-hooks` — commits `5ab8121` … `3c1ade4` vs `main`
|
||||
**Base:** `main`
|
||||
|
||||
## Verdict
|
||||
|
||||
**Approve with nits**
|
||||
|
||||
## Summary
|
||||
|
||||
This branch lands **E7M3-09**: comment-only telemetry anchor sites for future E9.M1 catalog events **`reputation_delta`** and **`faction_gate_blocked`**, consolidating partial stubs from NEO-136/137/138 into single ops-layer hooks. **`ReputationOperations.TryApplyDelta`** gains a full NEO-121-style block after audit append success; **`FactionGateOperations.TryEvaluate`** adds the missing read-failure deny hook and tightens threshold-branch payload notes; **`RewardRouterOperations`** duplicate **`reputation_delta`** stub is removed. **`server/README.md`** adds a dedicated NEO-141 subsection and cross-links NEO-130. Docs (plan, module, alignment register, backlog, manual QA) are reconciled. The branch also includes **NEO-140 story-end cleanup** (remove `docs/manual-qa/NEO-140.md`, plan/review wording). No runtime, wire, or test behavior changes — **823** server tests pass. Low merge risk.
|
||||
|
||||
## Documentation checked
|
||||
|
||||
| Path | Result |
|
||||
|------|--------|
|
||||
| `docs/plans/NEO-141-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` | **Partially matches** — E7M3-09 AC checked and landed note correct; in-scope bullet still mentions **`TryAccept`** deny path while implementation (correctly) anchors only **`FactionGateOperations`** per adopted plan decision. |
|
||||
| `docs/decomposition/modules/E7_M3_FactionReputationLedger.md` | **Matches** — Status line notes E7M3-09 landed (NEO-141); Responsibilities “Emit telemetry hook sites” satisfied at comment layer. |
|
||||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M3 row updated with NEO-141 telemetry + README link. |
|
||||
| `docs/decomposition/modules/module_dependency_register.md` | **N/A change** — no register row edit required beyond module doc / alignment table. |
|
||||
| `docs/manual-qa/NEO-141.md` | **Matches** — NEO-130-style code-review checklist; Bruno optional sanity steps. |
|
||||
| `docs/plans/NEO-140-implementation-plan.md` | **Matches** — manual QA removal note consistent with story-end convention (bundled in branch). |
|
||||
| `server/README.md` | **Matches** — Faction telemetry hooks (NEO-141) subsection; NEO-130 table cross-link; no duplicate router stub. |
|
||||
|
||||
## Blocking issues
|
||||
|
||||
None.
|
||||
|
||||
## Suggestions
|
||||
|
||||
1. **Align E7M3-09 backlog in-scope bullet with shipped anchor.** [E7M3-pre-production-backlog.md](../plans/E7M3-pre-production-backlog.md) §E7M3-09 still lists “`**TryAccept`** deny path” alongside **`FactionGateOperations`**, but the adopted plan and code place **`faction_gate_blocked`** only in **`FactionGateOperations.TryEvaluate`** (quest layer maps HTTP reason without duplicating the hook). Update the in-scope bullet to ops-only wording so future readers do not expect a **`QuestStateOperations`** comment block.
|
||||
|
||||
## Nits
|
||||
|
||||
- Nit: **`FactionGateOperations`** class summary says “deny return path below” (singular) while two deny branches now carry hooks — consider “deny return paths” for accuracy.
|
||||
- Nit: Branch bundles **NEO-140** manual QA removal (`ef75111`) with **NEO-141** work — acceptable hygiene; PR description should mention both if opened as one branch.
|
||||
|
||||
## Verification
|
||||
|
||||
```bash
|
||||
cd /Users/don/neon-sprawl && dotnet test NeonSprawl.sln
|
||||
```
|
||||
|
||||
Manual (see `docs/manual-qa/NEO-141.md` — code review checklist):
|
||||
|
||||
- Confirm hook comment blocks in `ReputationOperations.cs`, `FactionGateOperations.cs`; no **`reputation_delta`** stub in `RewardRouterOperations.cs`.
|
||||
- Confirm README NEO-141 subsection and NEO-130 cross-link.
|
||||
|
||||
Optional Bruno (unchanged behavior):
|
||||
|
||||
- `quest-progress/Accept grid contract faction gate deny`
|
||||
- `quest-progress/Accept grid contract after operator chain`
|
||||
Loading…
Reference in New Issue