feat: Hot reload status now reports how often each added member has been called - #3036
Merged
Merged
Conversation
--status reports InvocationCount 0 for every Added row because only
Harmony-patched methods carry the registry increment, and an added member
has no original method to patch. The worker now gives every added-member
shim a public static long <shimMethodName>__uloopCalls field and increments
it with Interlocked at the top of the shim body, after the null-receiver
check, so a call the check refuses is not counted.
- One function (PrependAddedMemberPreamble, generalized from
GuardAddedMemberReceiver) prepends the receiver check and the increment.
All three added-member emit paths reach it through
ShimTypeBuilder.AddAddedMemberMethod, so no added-member shim can be
emitted without its counter. Static expression bodies now become blocks
too, and a shim with no body stops with an explicit exception (body-less
members are skipped earlier, so this is not reachable).
- Auto-property accessors now get the receiver check as well. The store
already threw the same bare NullReferenceException, so only the throwing
frame moves, and the check keeps null-receiver calls out of the count.
- The next commit mirrors the suffix constant on the Editor side, which
reads the counter.
- Five test helpers that located a shim method by the first occurrence of
its name now search for the name followed by '(', because the counter
field's name starts with it.
Verification: the new worker test for the counter and its position passes
9/9 (red before the change); worker and related suites 131/131, 230/230
and 87/87; null-receiver E2E 10/10. The mutations "increment before the
check plus no increment for static members" and "accessor shims without
the check" each failed the expected cases.
`uloop hot-reload --status` reported InvocationCount 0 on every Added row and told the reader the member was not instrumented, so an agent could not tell whether a hot-reloaded caller ever reached a member it had added. The worker now declares a static counter beside every added-member shim; the Editor reads it. - Entry resolution looks up `<shim>__uloopCalls` beside the shim. A missing counter, or one that is not a static long, fails the whole file and the Failed row names the counter; registration refuses an added method without a readable counter, so no Added row exists without a count. - `--status` Added rows show the live count. Reason explains why the member has not run only while the count is 0, and the Message aggregate counts those rows together with never-invoked Active rows. - An unchanged reload's AlreadyActive row for an added member now carries the member and reads its counter when the response is built. It used to read the patch ledger, which after a return-type change holds the old patch's count rather than the added member's. - Re-applying a member emits a new shim whose counter starts at zero; an unchanged reload keeps the member and its count. Verification: new E2E class (methods, expression bodies, static members, bodied and auto properties, throw expressions, iterators, async, re-apply and sibling re-apply resets, return-type change after a body patch) 16/16; added-member E2E 254/254, other hot reload E2E 97/97, related unit suites 243/243. Mutations of the worker increment and of each Editor read failed the expected tests.
The skill told agents that Added rows always show InvocationCount 0 because added members are not instrumented. They are now counted, so the docs explain what the count means and when it stays 0: compiled code cannot call an added member, so only a hot-reloaded caller, or the Play Mode proxy for a forwarded Unity message, can run it. - output.md and troubleshooting.md describe the Added-row count, its reset on re-apply, AlreadyActive rows keeping it, and that an added iterator counts when its enumeration starts; the example status row now shows a counted added member. - SKILL.md and mechanism-and-lifecycle.md cover added members wherever they described the live InvocationCount; SKILL.md stays at 7,999 bytes. - The .claude and .agents copies are regenerated from the sources.
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: hatayama/unity-cli-loop/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughAdded-member shims now track calls with generated counters. Hot-reload registration and responses carry those counters, and status reports added-member invocation counts, zero-count reasons, and aggregate counts. Tests and hot-reload skill documentation cover the updated behavior. ChangesAdded-member invocation counting
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature · Severity of issue fixed: Low Sequence Diagram(s)sequenceDiagram
participant AddedMemberShim
participant InvocationCounterField
participant HotReloadStatusExecutor
participant HotReloadAddedMemberInfo
AddedMemberShim->>InvocationCounterField: Increment before the body runs
HotReloadStatusExecutor->>HotReloadAddedMemberInfo: Read InvocationCount for the status row
HotReloadAddedMemberInfo->>InvocationCounterField: Read the current long value
Merge Risk: ⚪ Minimal · up to Added-member invocation counts flow through status and apply results, with no concrete merge-blocking issue identified. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The inspected counter flow does not establish a new execution or privilege boundary. Remaining uncertainty concerns interrupted reload recovery and compatibility for external consumers, rather than a demonstrated new security vulnerability. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 49.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 117 functions across 34 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
uloop hot-reload --statusnow reportsInvocationCountfor members that hot reload added (new methods, property accessors, auto-properties), not only for patched methods.User Impact
Addedrow showedInvocationCount0 with a "not instrumented" reason, so there was no way to tell whether a hot-reloaded caller ever reached a newly added member.Addedrows count calls into the added member since it was applied.Reasonexplains why the member has not run only while the count is 0, and the statusMessagecounts those rows together with never-invokedActiverows.AlreadyActiverow for an added member carries that member's count.Changes
public static long <shimMethodName>__uloopCallsfield, incremented withInterlocked.Incrementat the top of the shim body after the null-receiver check, so a refused call is not counted. All three added-member emit paths go through one preamble function. Auto-property accessors now get the receiver check too (the store already threw the sameNullReferenceException; only the throwing frame moves).Addedrow exists without a count.--statusreads the counter forAddedrows..claude/.agentscopies are regenerated.Reasontext change.Verification
HotReloadAddedMemberInvocationCountE2ETests16/16: methods, expression bodies, static members, bodied and auto properties, throw expressions, iterators, async, re-apply and sibling re-apply resets, and a return-type change after a body patch.check-skill-size,sync-tool-docs --check, CA1502 complexity (max 15) and file length (max 500 SLOC) pass.Closes #2869