docs(protocol): P_StatUpdate detail page + fix stale audit comment#299
Merged
Conversation
Fifth per-packet detail page in the docs/protocol/packets/ series. P_StatUpdate is the actor-stat broadcast channel (S->C only, opcode 22) that P_AttackActor's "O" sub-code intentionally relies on for observer HP re-sync. Closes the cross-reference loop opened in PR #288. Three sub-codes documented: "A" -- attribute current value (6 bytes; 1B sub + 2B RuntimeID + 1B Attribute + 2B Value) "M" -- attribute maximum (same shape) "R" -- reputation (5 bytes; no attribute byte, only one reputation per actor) Two emit patterns captured -- the critical design decision in this packet: Important attributes (HealthStat, SpeedStat, EnergyStat) and reputation -- route through Update*/UpdateReputation in Server.bb, which walk FirstInZone for area-wide broadcast (observers need HP / speed updates to render correctly). Unimportant attributes (Strength, Wisdom, Toughness, custom stats) -- direct RCE_Send to target actor only (only the target's own HUD needs these). Five dispatching sites in ScriptingCommands.bb (SET / CHANGE / SETMAX / CHANGEMAX ATTRIBUTE) and ServerNet.bb (DM / setattribute /setattributemax) must stay in lockstep with the Health/Speed/Energy importance list. Validation discipline captured: * Server-side: `Attribute > -1` guard before any array index (BVM_CHANGEMAXATTRIBUTE pre-fix bug: write was outside the guard, typo'd attribute name OOB-wrote Maximum[-1]) * Server-side: Object.AreaInstance Null guard in Update* helpers (mid-warp actors no longer crash the broadcast loop -- PRs #154/ #176/#182-#188) * Client-side: RuntimeIDList Null guard (race on actor spawn) + bounds check Attribute >= 0 And Attribute < 40 (Field[39] = 40 slots) Privilege classification: SETATTRIBUTE/CHANGEATTRIBUTE are gated full- priv (not self-or-priv) because the HealthStat branch falls through to KillActor(Actor, Null) -- see CLAUDE.md "BVM clicker-handle trap" for why self-or-priv would have been wrong. Generator-only: scripts/gen_packet_index.sh re-run; row 22's Detail column now links to the new page (correctly notes C->S = "--", S->C only). Doc-audit-finds-source-bug bonus (the recon pattern from PRs #290/ #291/#297 keeps paying off): MainMenu.bb:1896 had a stale audit comment that referenced "P_StatUpdate 'B1' payload" while sitting inside the P_FetchCharacter "C1" handler. The actual packet/sub-code is P_FetchCharacter "C1" (40-attribute-pairs initial-load block); P_StatUpdate carries no "B" sub-code at all. Corrected to match the enclosing handler. Compile-verified clean across all 5 engine targets. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 675d509384
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| | `"M"` | 6 bytes | `1B sub + 2B RuntimeID + 1B Attribute + 2B Value` | Maximum attribute update — written to `A\Attributes\Maximum[Attribute]`. | | ||
| | `"R"` | 5 bytes | `1B sub + 2B RuntimeID + 2B Value` | Reputation update — written to `A\Reputation`. **No attribute byte** (only one reputation per actor). | | ||
|
|
||
| The 2-byte `Value` is signed and can carry negative attribute values (the wire format does not clamp — the server may send 0 or any in-range int16 for the broadcast). |
There was a problem hiding this comment.
For 2-byte fields, RCE_IntFromStr zeroes the conversion bank before poking the provided bytes, so a value like RCE_StrFromInt$(-1, 2) is decoded by the receiver as 65535, not -1 (see src/Modules/RCEnet.bb:77-82). Documenting these Value fields as signed will make client/proxy implementations diverge specifically when a script or DM command sends a negative attribute or reputation value over P_StatUpdate.
Useful? React with 👍 / 👎.
Pre-commit verification (the same symbolic-refs lesson from PR #297): two of the cited line numbers for the importance check were off: ServerNet.bb:353 should be :356, ServerNet.bb:367 should be :370. Replaced the line-number list with symbolic references (function / case names) per the doc-audit lesson. Also adjusted the prose: noted there are SIX sites (not five) -- I under-counted by lumping the two ServerNet handlers as one. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fifth per-packet detail page in docs/protocol/packets/.
P_StatUpdateis the actor-stat broadcast channel (S→C only, opcode 22) that P_AttackActor's "O" sub-code intentionally relies on for observer HP re-sync — observers receive a swing animation, then re-sync the victim's HP via the nextP_StatUpdate"A" broadcast. Closes the cross-reference loop opened in PR #288.What's covered
HealthStat/SpeedStat/EnergyStat+ reputation) route throughUpdate*helpers in Server.bb:969-1019 for area-wide broadcast viaFirstInZone; unimportant attributes (Strength, Wisdom, custom stats) send direct to the target actor only. Five dispatching sites must stay in lockstep with the importance list — catalogued in the doc.Attribute > -1guard (referencing the pre-fixBVM_CHANGEMAXATTRIBUTEOOB-write bug),AreaInstanceNull guard from PRs #154 / #176 / #182–#188; client-sideRuntimeIDListNull +Attribute < 40Field-bounds.SETATTRIBUTE/CHANGEATTRIBUTEare full-priv (HealthStat branch →KillActor), referencing the CLAUDE.md "BVM clicker-handle trap" note.Doc-audit-finds-source-bug bonus
The recon pattern from #290 / #291 / #297 keeps paying off. Found one this iteration:
MainMenu.bb:1896 had a stale audit comment that referenced "P_StatUpdate 'B1' payload" while sitting inside the
P_FetchCharacter \"C1\"handler. The actual packet+sub-code isP_FetchCharacter \"C1\"(40-attribute-pairs initial-load block);P_StatUpdatecarries no "B" sub-code at all. The comment's claim about the underlying data structure (40-slot Field bounds) was correct; only the packet name was wrong. Corrected to match the enclosing handler.Generator
scripts/gen_packet_index.shre-run; row 22's Detail column now links to the new page (correctly notes C→S = "—", S→C only).Test plan
compile.bat -tclean across all 5 engine targetsP_AttackActor.md,P_ChatMessage.md,encoding.md,handler-conventions.md) resolveServer.bb:969/988/1005,ClientNet.bb:990-1011,ScriptingCommands.bb:2224-2336,ServerNet.bb:353-378🤖 Generated with Claude Code