fix: address unresolved Copilot review feedback from PRs #3, #5, #6 - #8
Merged
Conversation
All three PRs merged with Copilot review comments still outstanding. Fixing each finding: PR #3 (fluid/energy storage): - ArchieEnergyStorage.readSnapshot and its Serializer.deserialize now clamp capacity to non-negative and amount to 0..capacity. A malformed/stale snapshot (e.g. from before a capacity change) could otherwise leave the storage over-capacity or negative, wedging insert()/extract() permanently. - gui.md's ProgressBar example: `value ?: 0 / smeltTicks.toFloat()` divides by 0 due to Elvis's low precedence, so the raw observed value passed straight through undivided instead of becoming a 0..1 fraction. Parenthesized. - Added BlockEntityNBTHolderTests.testItemFluidAndEnergyFieldsPersistMutations covering itemField/fluidField/energyField save/load round-tripping, the one field-kind trio the existing suite didn't touch. PR #5 (docs-sync / GUI-networking KDoc): - GuiGraphics.pose/scissor claimed to always restore pose/scissor state even if the block throws, but used no try/finally - an exception left the pose stack or scissor rect unbalanced. Wrapped both (plus the IntRect scissor overload, now just delegating to the primary one instead of duplicating the logic). - Text's KDoc says "ARGB text color" but the renderer passed color.rgb, silently dropping any alpha the caller set (vanilla's drawString forces alpha=0 to opaque, so this always rendered fully opaque regardless of intent). Switched to color.argb so alpha is actually honored. - NetworkChannel.configServerbound's KDoc said persistence happens in decodeDispatchData - it doesn't; decodeDispatchData only does the permission check, in-memory decode, and broadcast/reject. The handler registered by configServerbound is the only place that calls ConfigSpec.save(). Corrected. PR #6 (changelog automation): - release-notes.yaml's base-branch detection used `git branch -r --contains`, which matches any branch the tag is merely reachable from, not just one it's the tip of - non-deterministic if a tag is reachable from several branches. Switched to `--points-at`. - pr-title-lint.yml used pull_request_target unnecessarily (no checkout/exec of fork code happens, so the base-repo context it grants isn't needed and only widens blast radius for future changes) - switched to pull_request, and secrets.GITHUB_TOKEN to the intended github.token context. (One PR #4 finding - a comment about GITHUB_ACTOR injection - was already corrected before merge; left alone.) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
KP2048
marked this pull request as ready for review
August 6, 2026 21:41
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
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
You flagged that Copilot left review comments on recent PRs that never got addressed before merge. Checked #3, #4, #5, #6 — all four had Copilot reviews; #4's finding was already fixed before merge, the rest weren't. This PR fixes all of them.
PR #3 (fluid/energy storage)
ArchieEnergyStorage.readSnapshotand itsSerializer.deserializenow clampcapacityto non-negative andamountto0..capacity. A malformed/stale snapshot could otherwise leave the storage over-capacity or negative, wedginginsert()/extract()permanently.gui.md'sProgressBarexample:value ?: 0 / smeltTicks.toFloat()— Elvis's low precedence means this divides0bysmeltTicks, not the observed value, so a real value passes through undivided instead of becoming a0..1fraction. Parenthesized.BlockEntityNBTHolderTests.testItemFluidAndEnergyFieldsPersistMutationscoveringitemField/fluidField/energyFieldsave/load round-tripping — the one field-kind trio the existing suite didn't touch.PR #5 (docs-sync / GUI+networking KDoc)
GuiGraphics.pose/scissorclaimed to always restore pose/scissor state even if the block throws, but used notry/finally— an exception left the pose stack or scissor rect unbalanced, breaking subsequent rendering. Wrapped both (theIntRectscissor overload now just delegates to the primary one instead of duplicating the logic).Text's KDoc says "ARGB text color" but the renderer passedcolor.rgb, silently dropping any alpha the caller set (vanilla'sdrawStringforces alpha=0 to opaque, so this always rendered fully opaque regardless of intent). Switched tocolor.argbso alpha is actually honored.NetworkChannel.configServerbound's KDoc said persistence happens indecodeDispatchData— it doesn't; that function only does the permission check, in-memory decode, and broadcast/reject. The handler registered byconfigServerboundis the only place that callsConfigSpec.save(). Corrected.PR #6 (changelog automation)
release-notes.yaml's base-branch detection usedgit branch -r --contains, which matches any branch the tag is merely reachable from, not just one it's the tip of — non-deterministic if reachable from several. Switched to--points-at.pr-title-lint.ymlusedpull_request_targetunnecessarily (no checkout/exec of fork code happens, so the base-repo context it grants isn't needed and only widens blast radius for future changes) — switched topull_request, andsecrets.GITHUB_TOKENto the intendedgithub.tokencontext.Test plan
:common:compileKotlinclean (covers the gametest source set too — new test compiles)🤖 Generated with Claude Code