feat: subscription lifecycle hardening and readiness accessors#63
feat: subscription lifecycle hardening and readiness accessors#63amery wants to merge 11 commits into
Conversation
📝 WalkthroughWalkthroughThis PR clarifies NanoRPC subscription lifecycle semantics and implements corresponding client and server infrastructure. It introduces connection-readiness signaling for client-server coordination, refactors subscription callback dispatch to distinguish acknowledgements from updates, extends the server with readiness APIs, and upgrades the Go toolchain to 1.24 with code hygiene improvements. The protocol documentation, error types, and queue routing logic align to ensure correct request_id-based routing without misrouting between subscription establishment and update delivery phases. ChangesSubscription routing and readiness coordination
Go version upgrade and dependency updates
Code hygiene and tooling improvements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="Running error: context loading failed: no go files to analyze: running 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/nanorpc/server/README.md (1)
107-112:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the DI example to the new server constructor signatures
The “Custom Server with Dependency Injection” snippet still uses outdated call forms (
NewDefaultMessageHandler(),NewDefaultSessionManager(handler),NewServer(listener, sessionManager, handler)). This example should be updated to match current APIs so readers can copy-paste successfully.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/nanorpc/server/README.md` around lines 107 - 112, The DI example uses outdated constructors (NewDefaultMessageHandler, NewDefaultSessionManager, NewServer); update the snippet to call the current constructor APIs used in the package (replace NewDefaultMessageHandler and NewDefaultSessionManager(handler) with the package's current message handler and session manager constructors and update the NewServer call to the new server constructor signature), ensuring the new function names and required parameters/options match the current server package exports so the example compiles and can be copy-pasted.
🧹 Nitpick comments (4)
pkg/nanorpc/client/connected_test.go (1)
25-123: ⚡ Quick winConsider converting these readiness tests to table-driven form.
The scenarios are related and would align better with repository test style (and be easier to extend) as a table-driven suite.
As per coding guidelines, "
**/*_test.go: Use table-driven tests for comprehensive test coverage in Go."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/nanorpc/client/connected_test.go` around lines 25 - 123, Convert the multiple readiness tests into a single table-driven test that iterates over scenarios (initially disconnected, opens then closes, swaps after endSession, already connected fast-path, blocks then succeeds, context cancellation) to match repo style; create a test table with a name, setup steps (using newClientForTest, optional calls to setSession or endSession), expected behavior (IsConnected value, whether Connected channel is closed, WaitConnected result or blocking), and any timeouts, then implement a loop that runs each case as t.Run(name, func(t *testing.T){...}) using the existing helpers (Connected, setSession, endSession, WaitConnected) and assertions, preserving the current assertions and timeouts for each scenario.NANORPC_PROTOCOL.md (1)
283-289: ⚡ Quick winWrap the new prose lines to the 80-column markdownlint limit.
Multiple added lines exceed 80 chars and will trip the markdownlint configuration.
As per coding guidelines, "
**/*.md: Markdown files should follow markdownlint rules with 80-character line limits as configured ininternal/build/markdownlint.json."Also applies to: 323-330, 345-346
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NANORPC_PROTOCOL.md` around lines 283 - 289, The added paragraph about subscription lifetimes and routing exceeds the 80-column markdownlint limit; reflow the prose in NANORPC_PROTOCOL.md so each line is ≤80 chars while preserving wording about TYPE_SUBSCRIBE, TYPE_RESPONSE, TYPE_REQUEST and request_id semantics (first TYPE_RESPONSE = subscription ack, later TYPE_RESPONSE after TYPE_REQUEST with empty data = unsubscribe ack). Apply the same 80-column wrapping to the other affected blocks noted (around the additions at the ranges corresponding to lines 323-330 and 345-346) so the file conforms to the project's markdownlint configuration.pkg/nanorpc/client/subscribe_callback_test.go (1)
41-133: ⚡ Quick winRefactor these callback-path tests into a table-driven suite.
The cases are tightly related and fit naturally into a table-driven matrix (ACK OK, ACK error, update data/no-data/bad-data), which also matches project testing conventions.
As per coding guidelines, "
**/*_test.go: Use table-driven tests for comprehensive test coverage in Go."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/nanorpc/client/subscribe_callback_test.go` around lines 41 - 133, Combine the individual TestSubscribeCallback_* tests into a single table-driven test (e.g., TestSubscribeCallback_Table) that iterates over cases describing inputs and expected outcomes; each case should provide a name, the nanorpc.NanoRPCResponse (use the existing patterns: TYPE_RESPONSE with STATUS_OK, TYPE_RESPONSE with non-OK, TYPE_UPDATE with data, TYPE_UPDATE without data, TYPE_UPDATE with bad data), the expected error predicate (IsSubscriptionEstablished, IsNotFound, IsNoResponse, nil, or generic decode error) and expected out assertions, and call invokeSubscribeCallback for each row using t.Run; preserve existing assertions (core.AssertNoError, core.AssertErrorIs, core.AssertTrue/False, core.AssertNotNil, core.AssertEqual) but execute them per case so behavior for invokeSubscribeCallback, nanorpc.NanoRPCResponse, and the various status checks remains identical while removing the separate TestSubscribeCallback_ACKSurfacesEstablished, TestSubscribeCallback_ACKErrorStatusSurfacesRealError, TestSubscribeCallback_UpdateWithDataIsDelivered, TestSubscribeCallback_UpdateWithoutDataIsErrNoResponse, and TestSubscribeCallback_UpdateWithBadDataSurfacesDecodeError functions.pkg/nanorpc/client/session_test.go (1)
89-116: ⚡ Quick winAdd a regression row for “SUBSCRIBE-only + non-RESPONSE/non-UPDATE”
Please add one case where a SUBSCRIBE-only queue receives
TYPE_PONG(or unknown type) and assert no callback plus unchanged residue. This will lock in the demux contract and prevent accidental re-ack behavior regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/nanorpc/client/session_test.go` around lines 89 - 116, Add a regression row in routingTestCases that covers a SUBSCRIBE-only queue receiving a non-RESPONSE/non-UPDATE (e.g. TYPE_PONG): call newRoutingTestCase with name like "subscribe_only_nonresponse_nonupdate", initial state core.S(sub(9)), id 9, response respPong, expected callback index -1, and expected residue core.S(sub(9)); place this new test alongside the other routingTestCase entries to ensure newRoutingTestCase, routingTestCases, core.S, sub, and respPong are exercised and the subscribe-only residue remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/nanorpc/client/session.go`:
- Around line 130-134: When matching a SUBSCRIBE callback (subIdx) you currently
set cs.cb[subIdx].Acknowledged = true for any response that isn't TYPE_UPDATE,
which can erroneously acknowledge on other types (e.g., TYPE_PONG); change the
logic so you only set Acknowledged and return cs.cb[subIdx].Callback when the
incoming message's Type equals TYPE_RESPONSE—otherwise do not acknowledge and
return nil. Ensure you reference and check the message type constant
(TYPE_RESPONSE) before modifying cs.cb[subIdx].Acknowledged or returning
cs.cb[subIdx].Callback so only true response messages activate the subscription.
---
Outside diff comments:
In `@pkg/nanorpc/server/README.md`:
- Around line 107-112: The DI example uses outdated constructors
(NewDefaultMessageHandler, NewDefaultSessionManager, NewServer); update the
snippet to call the current constructor APIs used in the package (replace
NewDefaultMessageHandler and NewDefaultSessionManager(handler) with the
package's current message handler and session manager constructors and update
the NewServer call to the new server constructor signature), ensuring the new
function names and required parameters/options match the current server package
exports so the example compiles and can be copy-pasted.
---
Nitpick comments:
In `@NANORPC_PROTOCOL.md`:
- Around line 283-289: The added paragraph about subscription lifetimes and
routing exceeds the 80-column markdownlint limit; reflow the prose in
NANORPC_PROTOCOL.md so each line is ≤80 chars while preserving wording about
TYPE_SUBSCRIBE, TYPE_RESPONSE, TYPE_REQUEST and request_id semantics (first
TYPE_RESPONSE = subscription ack, later TYPE_RESPONSE after TYPE_REQUEST with
empty data = unsubscribe ack). Apply the same 80-column wrapping to the other
affected blocks noted (around the additions at the ranges corresponding to lines
323-330 and 345-346) so the file conforms to the project's markdownlint
configuration.
In `@pkg/nanorpc/client/connected_test.go`:
- Around line 25-123: Convert the multiple readiness tests into a single
table-driven test that iterates over scenarios (initially disconnected, opens
then closes, swaps after endSession, already connected fast-path, blocks then
succeeds, context cancellation) to match repo style; create a test table with a
name, setup steps (using newClientForTest, optional calls to setSession or
endSession), expected behavior (IsConnected value, whether Connected channel is
closed, WaitConnected result or blocking), and any timeouts, then implement a
loop that runs each case as t.Run(name, func(t *testing.T){...}) using the
existing helpers (Connected, setSession, endSession, WaitConnected) and
assertions, preserving the current assertions and timeouts for each scenario.
In `@pkg/nanorpc/client/session_test.go`:
- Around line 89-116: Add a regression row in routingTestCases that covers a
SUBSCRIBE-only queue receiving a non-RESPONSE/non-UPDATE (e.g. TYPE_PONG): call
newRoutingTestCase with name like "subscribe_only_nonresponse_nonupdate",
initial state core.S(sub(9)), id 9, response respPong, expected callback index
-1, and expected residue core.S(sub(9)); place this new test alongside the other
routingTestCase entries to ensure newRoutingTestCase, routingTestCases, core.S,
sub, and respPong are exercised and the subscribe-only residue remains
unchanged.
In `@pkg/nanorpc/client/subscribe_callback_test.go`:
- Around line 41-133: Combine the individual TestSubscribeCallback_* tests into
a single table-driven test (e.g., TestSubscribeCallback_Table) that iterates
over cases describing inputs and expected outcomes; each case should provide a
name, the nanorpc.NanoRPCResponse (use the existing patterns: TYPE_RESPONSE with
STATUS_OK, TYPE_RESPONSE with non-OK, TYPE_UPDATE with data, TYPE_UPDATE without
data, TYPE_UPDATE with bad data), the expected error predicate
(IsSubscriptionEstablished, IsNotFound, IsNoResponse, nil, or generic decode
error) and expected out assertions, and call invokeSubscribeCallback for each
row using t.Run; preserve existing assertions (core.AssertNoError,
core.AssertErrorIs, core.AssertTrue/False, core.AssertNotNil, core.AssertEqual)
but execute them per case so behavior for invokeSubscribeCallback,
nanorpc.NanoRPCResponse, and the various status checks remains identical while
removing the separate TestSubscribeCallback_ACKSurfacesEstablished,
TestSubscribeCallback_ACKErrorStatusSurfacesRealError,
TestSubscribeCallback_UpdateWithDataIsDelivered,
TestSubscribeCallback_UpdateWithoutDataIsErrNoResponse, and
TestSubscribeCallback_UpdateWithBadDataSurfacesDecodeError functions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8cf6dae-54c2-406c-946b-e7d60f3984d2
📒 Files selected for processing (14)
NANORPC_PROTOCOL.mdpkg/nanorpc/client/README.mdpkg/nanorpc/client/client.gopkg/nanorpc/client/connected_test.gopkg/nanorpc/client/reconnect.gopkg/nanorpc/client/request.gopkg/nanorpc/client/session.gopkg/nanorpc/client/session_test.gopkg/nanorpc/client/subscribe_callback_test.gopkg/nanorpc/errors.gopkg/nanorpc/server/README.mdpkg/nanorpc/server/doc.gopkg/nanorpc/server/server.gopkg/nanorpc/server/server_test.go
| if subIdx < 0 { | ||
| return nil | ||
| } | ||
| cs.cb[subIdx].Acknowledged = true | ||
| return cs.cb[subIdx].Callback |
There was a problem hiding this comment.
Acknowledge subscriptions only on TYPE_RESPONSE
Acknowledged is currently set for any non-TYPE_UPDATE response when only a SUBSCRIBE entry matches. That can incorrectly activate a subscription on unexpected response types (for example TYPE_PONG with a colliding request_id).
Suggested fix
- if subIdx < 0 {
+ if subIdx < 0 {
return nil
}
+ if respType != nanorpc.NanoRPCResponse_TYPE_RESPONSE {
+ return nil
+ }
cs.cb[subIdx].Acknowledged = true
return cs.cb[subIdx].Callback📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if subIdx < 0 { | |
| return nil | |
| } | |
| cs.cb[subIdx].Acknowledged = true | |
| return cs.cb[subIdx].Callback | |
| if subIdx < 0 { | |
| return nil | |
| } | |
| if respType != nanorpc.NanoRPCResponse_TYPE_RESPONSE { | |
| return nil | |
| } | |
| cs.cb[subIdx].Acknowledged = true | |
| return cs.cb[subIdx].Callback |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/nanorpc/client/session.go` around lines 130 - 134, When matching a
SUBSCRIBE callback (subIdx) you currently set cs.cb[subIdx].Acknowledged = true
for any response that isn't TYPE_UPDATE, which can erroneously acknowledge on
other types (e.g., TYPE_PONG); change the logic so you only set Acknowledged and
return cs.cb[subIdx].Callback when the incoming message's Type equals
TYPE_RESPONSE—otherwise do not acknowledge and return nil. Ensure you reference
and check the message type constant (TYPE_RESPONSE) before modifying
cs.cb[subIdx].Acknowledged or returning cs.cb[subIdx].Callback so only true
response messages activate the subscription.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
The subscribe and unsubscribe acknowledgements share a single request_id, and the original wording in §5.4 and §6.1 left two gaps: it did not say how long the request_id is reserved, and it did not distinguish the subscribe TYPE_RESPONSE from the unsubscribe TYPE_RESPONSE that arrives on the same id. §5.4 now states the reservation window explicitly — from TYPE_SUBSCRIBE until the unsubscribe acknowledgement or session end — and warns that routing keyed only on request_id will misroute one of the two TYPE_RESPONSEs. §6.1 expands the termination bullets, adds a stateDiagram-v2 of the Pending/Active/Unsubscribing/Terminated transitions, and introduces a Phases subsection naming each phase and which message types may arrive in it. §6.3 adds a Termination bullet covering the in-flight TYPE_UPDATE that may still arrive between the unsubscribe request and its acknowledgement. §10.1's ASCII sequence shows the same in-flight TYPE_UPDATE alongside the unsubscribe acknowledgement. §2.2's request-response ASCII diagram picks up a one-space column alignment fix. Signed-off-by: Alejandro Mery <amery@apptly.co>
Replace older loop and formatting patterns with their current stdlib equivalents, in preparation for enabling golangci-lint's modernize analyser: - max() for the zero-skipping clamp in the request counter - range-over-int for C-style index loops in the server and the shared test helpers - maps.Copy for hand-written map-copy loops in the mock field logger - fmt.Appendf in place of []byte(fmt.Sprintf(...)) Every construct is available on the current go 1.23.0 floor (range-over-int since 1.22), so this stands independently of the upcoming Go 1.24 bump. Signed-off-by: Alejandro Mery <amery@apptly.co>
Finish the 1.24 floor that the core v0.19 bump (c3d4153) started: the generator and nanorpc modules already declared go 1.24.0 through that dependency, leaving the root and nanopb modules behind at 1.23.0. Raise both, and align the build configuration with the darvaza.org siblings. - Raise the root and nanopb modules to go 1.24.0, and update the prose that quotes the minimum (README, AGENTS, coverage docs). - Enable the modernize analyser in .golangci.yml, the code having been prepared for it in the preceding refactor, and quote the version field to match core/slog/x. - Add .deepsource.toml, mirroring the siblings with the nanorpc import root. - Drop the duplicate *.test entry from .gitignore and sync the .stdout coverage docs from core. Signed-off-by: Alejandro Mery <amery@apptly.co>
- `claude.yml`: respond to `@claude` mentions in issues and pull requests, gated to the `amery` / `karasz` actors. The `pull_request_review*` branches add a fork-PR guard (`pull_request.head.repo.full_name == github.repository`) as belt-and-braces over the actor gate. - `claude-code-review.yml`: automatic review on non-draft pull requests from the base repo, using the `code-review` plugin from the anthropics/claude-code-plugins marketplace. Renovate bot excluded; fork PRs gated out via the same `head.repo.full_name` guard. Both require the `CLAUDE_CODE_OAUTH_TOKEN` secret on the repo or inherited from the protomcp org — fork PRs can't access it, hence the guards. Signed-off-by: Alejandro Mery <amery@apptly.co>
Move the generator and nanorpc modules from core v0.19.0 to the v0.19.1 patch release, which adds the MustNoError error-precondition helpers. No API used here changes. Signed-off-by: Alejandro Mery <amery@apptly.co>
Expose three readiness primitives on the [Client]: - Connected() returns a channel that closes whilst the client holds an active session. The channel is replaced after each disconnect, so it signals the next readiness edge and callers waiting across a reconnect cycle must re-fetch. - IsConnected() reports the current session state as a point-in-time snapshot. - WaitConnected(ctx) wraps Connected with the caller's context, so consumers can ride out a brief reconnect blip without polling Pong(). Closes the doc/code gap left by the README's "Connection Management" example, which had referenced Connected() and IsConnected() before either existed. The readiness channel is initialised in init(), closed inside setSession after the session pointer is attached, and swapped for a fresh open channel inside endSession when a live session ends. endSession leaves the channel untouched when already disconnected, so waiters holding it are not orphaned. Both transitions happen under the existing mu guarding c.cs. Signed-off-by: Alejandro Mery <amery@apptly.co>
Allow callers to pass an existing *DefaultMessageHandler — and so register paths against it — before [Server.Serve] starts. When the handler is nil, the existing behaviour is preserved: a fresh DefaultMessageHandler with an internal HashCache is built in-place. Update the doc.go and README examples so the call shape matches the new signature, and add an integration test that registers an echo handler, drives a real TCP request through the server, and verifies the payload round-trips. Migrate the rest of server_test.go from t.Fatalf to the darvaza.org/core assertion helpers while touching it. BREAKING CHANGE: NewDefaultServer gains a handler parameter between listener and logger; existing callers must pass nil to retain the previous behaviour. Signed-off-by: Alejandro Mery <amery@apptly.co>
x/sync v0.4.1 closes the workgroup.doCancel race where enrolling the OnCancel handler could Add(1) after the inner WaitGroup had drained, racing a concurrent Wait() return — an ordering Server.Shutdown depends on. Signed-off-by: Alejandro Mery <amery@apptly.co>
Add a `ready chan struct{}` field to [Server] and a public
`Ready() <-chan struct{}` accessor that closes once the accept loop
has started taking connections. The close is performed by an
idempotent `signalReady` helper guarded by `mu`, so repeated Serve
calls do not panic on a double close, and the channel stays closed
across shutdown so late observers never block.
Switch the test harness off the `time.Sleep(50 * time.Millisecond)`
heuristic: a `waitServerReady` helper now blocks on `Ready()` with a
generous 1s ceiling, and a new `TestServer_Ready` confirms the
channel is open before Serve, closes after the loop is reached, and
stays closed across Shutdown.
Signed-off-by: Alejandro Mery <amery@apptly.co>
The server's TYPE_RESPONSE acknowledgement now reaches the typed SubscribeCallback as either ErrSubscriptionEstablished on STATUS_OK or a ResponseError on any other status, via a dispatcher split into isSubscribeACK, subscribeACKErr and decodeSubscribePayload, plus a callNewOut helper that rejects typed-nil factory results through core.IsNil. The newOut factory is now required at the public Subscribe boundary. GetResponse gains the same boundary protection: typed-nil client and out arguments are rejected with core.ErrInvalid, and the wait arm is extracted into waitGetResponse so the function stays under the cognitive-complexity floor. Add ErrSubscriptionEstablished and IsSubscriptionEstablished in the root package so callers can recognise the acknowledgement. Cover the new boundary guards with tests: the callNewOut typed-nil and factory-error paths, and the Subscribe/GetResponse nil rejections. BREAKING CHANGE: Subscribe now rejects a nil newOut factory with core.ErrInvalid instead of substituting a zero-value factory, and GetResponse rejects typed-nil client or out arguments. Callers that relied on the previous nil-tolerant behaviour must supply a factory and non-nil arguments. Signed-off-by: Alejandro Mery <amery@apptly.co>
The dispatcher previously keyed callbacks on request_id alone, so the TYPE_RESPONSE acknowledging an Unsubscribe — which reuses the subscription's request_id — reached the still-registered SubscribeCallback. With subscribe-ACK surfacing in place, this showed up as ErrSubscriptionEstablished firing after the caller had already unsubscribed. popRequestCallback now matches on the (request_id, response_type) pair: unsafeIndexCallbacks indexes the queue by request_id and splits the matches into the SUBSCRIBE entry and the first non-SUBSCRIBE entry, then popRequestCallback routes by response_type. TYPE_UPDATE routes to the SUBSCRIBE entry without removing it; any other response prefers the non-SUBSCRIBE entry and drops both entries when a SUBSCRIBE entry shadowed it; a non-update response that resolves only a SUBSCRIBE entry is steered by the subscription lifecycle in unsafeResolveSubscribeResponse. A Pending subscription takes its one acknowledgement — STATUS_OK moves it to Active (Acknowledged flips, the entry stays queued for updates) and a non-OK ack moves it to Terminated (the entry is dropped so a rejected subscription cannot linger and later satisfy the unsubscribe guard). An Active subscription has no acknowledgement left to take, so a further TYPE_RESPONSE is anomalous and is ignored rather than tearing down the live subscription. Send mirrors the protocol shape by rejecting an unsubscribe-form TYPE_REQUEST (positive request_id on a non-subscribe) whose target subscription is missing or still pending its acknowledgement. The function is factored into validateSendArgs, isUnsubscribeShape, checkUnsubscribeTarget, normaliseRequestID and registerCallback so the pipeline reads in order. Document the new os.ErrInvalid failure modes on Session.Send and on Client.Unsubscribe, Client.UnsubscribeByHash and Client.UnsubscribeWithHash, each cross-linked to its matching Subscribe variant. Cover popRequestCallback's routing matrix and the Send guard with data-driven tests under package client. Signed-off-by: Alejandro Mery <amery@apptly.co>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pkg/nanorpc/client/subscribe_callback_test.go (1)
52-190: 🏗️ Heavy liftConsider converting the scenario set to one table-driven test matrix.
The scenario coverage is solid, but consolidating these cases into table rows (
name/input/expected/wantErr) will make future lifecycle additions cheaper and keep style consistent across test files.As per coding guidelines,
**/*_test.go: "Use table-driven tests for comprehensive test coverage with test cases defined as structs containing name, input, expected, and wantErr fields."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/nanorpc/client/subscribe_callback_test.go` around lines 52 - 190, Convert the multiple individual test functions (TestSubscribeCallback_ACKSurfacesEstablished, TestSubscribeCallback_ACKErrorStatusSurfacesRealError, TestSubscribeCallback_UpdateWithDataIsDelivered, TestSubscribeCallback_UpdateWithoutDataIsErrNoResponse, TestSubscribeCallback_UpdateWithBadDataSurfacesDecodeError, TestSubscribeCallback_NewOutTypedNilRejected, TestSubscribeCallback_NewOutErrorSurfaces, TestSubscribeCallback_UpdateNewOutErrorSurfaces) into a single table-driven test. Define a test case struct containing name, input (NanoRPCResponse), expected output, and wantErr fields, then create a loop that iterates through all test cases and executes the same invokeSubscribeCallback logic with appropriate assertions for each scenario. This consolidation will follow the coding guidelines for comprehensive test coverage while maintaining consistency.Source: Coding guidelines
pkg/nanorpc/client/request_test.go (1)
199-229: ⚡ Quick winPrefer a single table-driven boundary test for these nil-guard cases.
These three tests exercise the same API contract shape; consolidating into a table with
name/input/expected/wantErrwill reduce duplication and keep this file aligned with repo test conventions.As per coding guidelines,
**/*_test.go: "Use table-driven tests for comprehensive test coverage with test cases defined as structs containing name, input, expected, and wantErr fields."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/nanorpc/client/request_test.go` around lines 199 - 229, Consolidate the three separate test functions TestGetResponse_NilClientRejected, TestGetResponse_NilOutRejected, and TestSubscribe_NilNewOutRejected into a single table-driven test. Replace these three functions with one test function that defines a slice of test case structs with fields for test name, input parameters, and expected error, then iterate through the table executing the appropriate GetResponse or Subscribe call for each case and verifying the error matches core.ErrInvalid using core.AssertErrorIs. This reduces duplication and aligns with the repository's test conventions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/claude-code-review.yml:
- Around line 21-33: Replace mutable version tags with immutable commit SHAs for
supply-chain security. For the "Checkout repository" step using
actions/checkout, replace the `@v6` tag with a specific commit SHA. For the "Run
Claude Code Review" step using anthropics/claude-code-action, replace the `@v1`
tag with a specific commit SHA. Additionally, add persist-credentials: false to
the checkout step's with configuration to prevent repository tokens from
persisting in the runner environment.
In @.github/workflows/claude.yml:
- Around line 31-42: The workflow uses mutable action version tags that could be
retargeted to malicious code, and the checkout step does not disable credential
persistence. Replace the mutable tag `actions/checkout@v6` with a pinned SHA
hash, replace `anthropics/claude-code-action@v1` with a pinned SHA hash, and add
`persist-credentials: false` to the checkout step configuration to prevent the
GITHUB_TOKEN from being exposed to subsequent steps.
In `@pkg/nanorpc/client/session.go`:
- Around line 230-231: The validateSendArgs function dereferences
req.RequestType unconditionally without first checking if req is nil, which
causes a panic when Send is called with a nil request. Add a nil guard at the
beginning of the validateSendArgs function that checks if req is nil and returns
an appropriate error (such as an invalid-argument error) before attempting to
access req.RequestType.
---
Nitpick comments:
In `@pkg/nanorpc/client/request_test.go`:
- Around line 199-229: Consolidate the three separate test functions
TestGetResponse_NilClientRejected, TestGetResponse_NilOutRejected, and
TestSubscribe_NilNewOutRejected into a single table-driven test. Replace these
three functions with one test function that defines a slice of test case structs
with fields for test name, input parameters, and expected error, then iterate
through the table executing the appropriate GetResponse or Subscribe call for
each case and verifying the error matches core.ErrInvalid using
core.AssertErrorIs. This reduces duplication and aligns with the repository's
test conventions.
In `@pkg/nanorpc/client/subscribe_callback_test.go`:
- Around line 52-190: Convert the multiple individual test functions
(TestSubscribeCallback_ACKSurfacesEstablished,
TestSubscribeCallback_ACKErrorStatusSurfacesRealError,
TestSubscribeCallback_UpdateWithDataIsDelivered,
TestSubscribeCallback_UpdateWithoutDataIsErrNoResponse,
TestSubscribeCallback_UpdateWithBadDataSurfacesDecodeError,
TestSubscribeCallback_NewOutTypedNilRejected,
TestSubscribeCallback_NewOutErrorSurfaces,
TestSubscribeCallback_UpdateNewOutErrorSurfaces) into a single table-driven
test. Define a test case struct containing name, input (NanoRPCResponse),
expected output, and wantErr fields, then create a loop that iterates through
all test cases and executes the same invokeSubscribeCallback logic with
appropriate assertions for each scenario. This consolidation will follow the
coding guidelines for comprehensive test coverage while maintaining consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd66ae5f-eb21-4e5c-a1a1-8897b5073d03
⛔ Files ignored due to path filters (2)
pkg/generator/go.sumis excluded by!**/*.sumpkg/nanorpc/go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
.deepsource.toml.github/workflows/claude-code-review.yml.github/workflows/claude.yml.gitignore.golangci.ymlAGENTS.mdNANORPC_PROTOCOL.mdREADME.mdgo.modinternal/build/README-coverage.mdpkg/generator/go.modpkg/nanopb/go.modpkg/nanorpc/client/README.mdpkg/nanorpc/client/client.gopkg/nanorpc/client/connected_test.gopkg/nanorpc/client/reconnect.gopkg/nanorpc/client/request.gopkg/nanorpc/client/request_counter.gopkg/nanorpc/client/request_test.gopkg/nanorpc/client/session.gopkg/nanorpc/client/session_test.gopkg/nanorpc/client/subscribe_callback_test.gopkg/nanorpc/errors.gopkg/nanorpc/go.modpkg/nanorpc/server/README.mdpkg/nanorpc/server/doc.gopkg/nanorpc/server/handler_test.gopkg/nanorpc/server/server.gopkg/nanorpc/server/server_test.gopkg/nanorpc/testutils_test.gopkg/nanorpc/utils/slices_test.gopkg/nanorpc/utils/testutils/mockfieldlogger.gopkg/nanorpc/utils/testutils/mockfieldlogger_test.go
💤 Files with no reviewable changes (1)
- .gitignore
✅ Files skipped from review due to trivial changes (11)
- go.mod
- pkg/generator/go.mod
- pkg/nanopb/go.mod
- README.md
- AGENTS.md
- pkg/nanorpc/utils/testutils/mockfieldlogger_test.go
- .golangci.yml
- internal/build/README-coverage.md
- pkg/nanorpc/server/README.md
- pkg/nanorpc/server/doc.go
- pkg/nanorpc/server/handler_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/nanorpc/client/README.md
- pkg/nanorpc/client/connected_test.go
- pkg/nanorpc/client/request.go
- pkg/nanorpc/client/reconnect.go
- pkg/nanorpc/client/client.go
- pkg/nanorpc/errors.go
| - name: Checkout repository | ||
| uses: actions/checkout@v6 | ||
| with: | ||
| fetch-depth: 1 | ||
|
|
||
| - name: Run Claude Code Review | ||
| id: claude-review | ||
| uses: anthropics/claude-code-action@v1 | ||
| with: | ||
| claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} | ||
| plugin_marketplaces: 'https://github.com/anthropics/claude-code.git' | ||
| plugins: 'code-review@claude-code-plugins' | ||
| prompt: '/code-review:code-review ${{ github.repository }}/pull/${{ github.event.pull_request.number }}' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/claude-code-review.ymlRepository: protomcp/nanorpc
Length of output: 1287
🏁 Script executed:
cd .github/workflows && pwd && ls -laRepository: protomcp/nanorpc
Length of output: 639
🌐 Web query:
GitHub Actions security best practices pin commit SHA instead of tag versions
💡 Result:
Pinning GitHub Actions to a full-length commit SHA is a primary security best practice because it provides an immutable reference to the action's code [1][2][3]. Unlike tags or branch names (e.g., v4, main), which are mutable and can be force-updated to point to different, potentially malicious commits, a full-length commit SHA is cryptographically tied to a specific version of the code [4][5][6]. Key security and operational considerations include: 1. Mitigation of Supply Chain Attacks: If an action's repository is compromised, an attacker can modify existing tags to include malicious code [2][7][8]. Workflows referencing these tags will automatically pull the updated, compromised code [7]. Pinning to a SHA prevents this, as the workflow will continue to run the specific, verified version it was configured to use, regardless of what the tag points to later [2][7]. 2. Policy Enforcement: Organizations can enforce this practice at the repository or organization level through GitHub's policy settings [1][9][5]. When enabled, workflows attempting to use tags or branch names will fail, mandating the use of full commit SHAs [5][2][10]. 3. Maintenance with Automation: While SHA pinning is more secure, it requires manual maintenance to update to newer versions. This is best managed using automation tools like Dependabot or Renovate [4][5][2][8]. These tools can open pull requests to update the pinned SHA, ensuring that users receive security patches and feature updates while maintaining the security benefits of immutable pinning [5][2]. 4. Implementation Best Practice: To maintain readability and ease of maintenance, it is common to include the version tag as a comment alongside the commit SHA in the workflow file [2]: - uses: actions/checkout@3df4ab1 # v4.0.0 5. Important Warnings: - Always use the full-length commit SHA (40 characters). Shortened SHAs are insecure because they can be subject to collision attacks in repository networks [3]. - SHA pinning protects against tag-retargeting attacks, but it does not protect against malicious code that is already present in a commit [8]. You should still only use actions from trusted sources [1][3].
Citations:
- 1: https://docs.github.com/en/actions/reference/security/secure-use
- 2: https://architecture.lullabot.com/adr/20260505-pin-github-actions-to-hash/
- 3: https://docs.github.com/en/enterprise-server@2.22/actions/security-guides/security-hardening-for-github-actions
- 4: https://github.com/kubernetes/community/blob/main/github-management/github-actions-policy.md
- 5: https://www.romainlespinasse.dev/posts/github-actions-commit-sha-pinning/
- 6: https://nearform.com/insights/why-you-should-pin-your-github-actions-by-commit-hash/
- 7: https://safeguard.sh/resources/blog/github-actions-pinning-sha-vs-tag
- 8: https://emmer.dev/blog/pin-your-github-actions-to-protect-against-mutability/
- 9: https://github.blog/changelog/2025-08-15-github-actions-policy-now-supports-blocking-and-sha-pinning-actions/
- 10: https://github.com/github/docs/blob/main/content/admin/enforcing-policies/enforcing-policies-for-your-enterprise/enforcing-policies-for-github-actions-in-your-enterprise.md
🌐 Web query:
actions/checkout persist-credentials security implications GitHub Actions
💡 Result:
The persist-credentials input in actions/checkout determines whether the authentication token used to clone the repository is stored in the Git configuration for subsequent use within the job [1][2]. When set to its default value of true, the action configures the runner's Git environment with credentials, enabling scripts in later steps to execute authenticated Git commands (such as fetch or push) without additional configuration [1][3][4]. Security Implications The primary security risk arises because this configuration persists secrets within the runner environment, which can lead to accidental exposure: 1. Credential Leakage via Artifacts: A well-documented risk known as "Artipacked" occurs when a workflow uploads the workspace (containing the Git configuration) as an artifact using actions/upload-artifact [5][6]. If the credentials are persisted, they are bundled into the artifact and potentially exposed to unauthorized users who have access to those artifacts [7][6]. 2. Exposure to Malicious Steps: Persisting credentials makes them accessible to every subsequent step in the job [8][4]. If a workflow runs untrusted third-party actions, those actions could potentially access and misuse the persisted credentials to perform unauthorized API calls or modify repository contents [9][4]. 3. Mitigation in Newer Versions: In actions/checkout v6 and later, credential storage was improved by moving the persisted information from the repository's.git/config file to a separate, isolated file under $RUNNER_TEMP [1][10][5]. While this offers better isolation, the credentials remain present in the runner environment during the job execution [5]. Recommendation Security best practices mandate setting persist-credentials: false unless your workflow explicitly requires the ability to perform authenticated Git operations (e.g., pushing commits back to the repository) [9][4]. Example of secure usage: - uses: actions/checkout@v4 with: persist-credentials: false By disabling this option, you ensure that the checkout step does not introduce persistent credentials into the job's environment, thereby minimizing the risk of accidental leakage [7][4].
Citations:
- 1: https://github.com/actions/checkout
- 2: actions/checkout@c170eef
- 3: https://github.com/marketplace/actions/checkout
- 4: https://actsense.dev/vulnerabilities/unsafe_checkout/
- 5: https://sisaku-security.github.io/lint/docs/rules/artipacked/
- 6: https://sisaku-security.github.io/lint/docs/goat/case06-artipacked/
- 7: https://yossarian.net/til/post/actions-checkout-can-leak-github-credentials/
- 8: Remove
persist-credentialsor change the default tofalseactions/checkout#485 - 9: https://cheatsheetseries.owasp.org/cheatsheets/GitHub_Actions_Security_Cheat_Sheet.html
- 10: Persist creds to a separate file actions/checkout#2286
Pin all third-party actions to immutable commit SHAs and disable checkout credential persistence.
Line 22 uses actions/checkout@v6 and line 28 uses anthropics/claude-code-action@v1 with mutable version tags. Mutable tags can be force-updated to point to malicious code, creating a supply-chain risk. Additionally, the checkout step at line 24 does not disable credential persistence (persist-credentials: false), leaving the repository token in the runner environment for the duration of the job. With this workflow's write permissions, both issues warrant hardening.
🔒 Suggested hardening patch
- name: Checkout repository
- uses: actions/checkout@v6
+ uses: actions/checkout@<full-commit-sha>
with:
fetch-depth: 1
+ persist-credentials: false
- name: Run Claude Code Review
id: claude-review
- uses: anthropics/claude-code-action@v1
+ uses: anthropics/claude-code-action@<full-commit-sha>🧰 Tools
🪛 zizmor (1.25.2)
[warning] 21-24: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 22-22: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 28-28: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/claude-code-review.yml around lines 21 - 33, Replace
mutable version tags with immutable commit SHAs for supply-chain security. For
the "Checkout repository" step using actions/checkout, replace the `@v6` tag with
a specific commit SHA. For the "Run Claude Code Review" step using
anthropics/claude-code-action, replace the `@v1` tag with a specific commit SHA.
Additionally, add persist-credentials: false to the checkout step's with
configuration to prevent repository tokens from persisting in the runner
environment.
Source: Linters/SAST tools
| - name: Checkout repository | ||
| uses: actions/checkout@v6 | ||
| with: | ||
| fetch-depth: 1 | ||
|
|
||
| - name: Run Claude Code | ||
| id: claude | ||
| uses: anthropics/claude-code-action@v1 | ||
| with: | ||
| claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} | ||
| additional_permissions: | | ||
| actions: read |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the Claude workflow file
cat -n .github/workflows/claude.yml | head -50Repository: protomcp/nanorpc
Length of output: 1776
🌐 Web query:
GitHub Actions security best practices pin action version commit SHA vs tag persist-credentials
💡 Result:
To secure GitHub Actions workflows, follow these established best practices regarding action versioning and credential persistence: Pinning Actions: Commit SHA vs. Tag Pinning actions to a full-length commit SHA is the recommended method to ensure immutability and protect against supply chain attacks [1][2][3]. - Commit SHA (Recommended): Because commit SHAs are immutable, pinning an action to a specific SHA guarantees that the exact code you vetted will execute every time [4][3]. This mitigates the risk of "retargeting" attacks, where a malicious actor updates a tag or branch to point to compromised code [5][2]. GitHub allows administrators to enforce this policy at the organization or repository level to mandate full-length commit SHA usage [1][4]. - Tags (Use with Caution): While tags (e.g., @v4) are convenient, they are mutable references that can be moved or deleted by repository maintainers [1][2]. If you choose to use tags, ensure you only trust the action's creator [1]. - Best Practice for Maintenance: To balance security and ease of maintenance, pin to a full-length commit SHA and use automated dependency update tools like Dependabot or Renovate. These tools can automatically create pull requests to update your SHAs, often including the corresponding human-readable version tag in a comment to make the update process transparent [5][2]. Persist-Credentials The persist-credentials parameter in actions/checkout controls whether the Git credential helper is configured with the repository's credentials (typically the GITHUB_TOKEN) [6][7]. - Default Risk: By default, persist-credentials is set to true, which stores the token in the runner's Git configuration [6][7]. This makes the credentials accessible to all subsequent steps and potentially exposes them if the runner is compromised or if the workspace is inadvertently uploaded as an artifact (a vulnerability known as "Artipacked") [8][6][7]. - Security Recommendation: Unless your workflow explicitly requires Git operations that need these credentials (such as pushing commits back to the repository), you should always explicitly set persist-credentials: false [9][6][7]. Summary of Security Configuration For optimal security, use the following patterns: 1. Pin all actions (including sub-actions, if you are an action maintainer) to full-length commit SHAs [1][4]. 2. Set persist-credentials: false for all actions/checkout steps that do not require repository write access [9][6]. 3. Apply the principle of least privilege by configuring minimal permissions for the GITHUB_TOKEN [10][8][6].
Citations:
- 1: https://docs.github.com/en/actions/reference/security/secure-use
- 2: https://nearform.com/insights/why-you-should-pin-your-github-actions-by-commit-hash/
- 3: https://www.stepsecurity.io/blog/pinning-github-actions-for-enhanced-security-a-complete-guide
- 4: https://www.romainlespinasse.dev/posts/github-actions-commit-sha-pinning/
- 5: https://emmer.dev/blog/pin-your-github-actions-to-protect-against-mutability/
- 6: https://actsense.dev/vulnerabilities/unsafe_checkout/
- 7: https://sisaku-security.github.io/lint/docs/rules/artipacked/
- 8: Remove
persist-credentialsor change the default tofalseactions/checkout#485 - 9: https://cheatsheetseries.owasp.org/cheatsheets/GitHub_Actions_Security_Cheat_Sheet.html
- 10: https://github.com/github/docs/blob/main/content/actions/reference/security/secure-use.md
Pin action SHAs and set persist-credentials: false in checkout configuration.
Line 32 uses actions/checkout@v6 and line 38 uses anthropics/claude-code-action@v1—both are mutable tags that can be retargeted to malicious code. Additionally, the checkout step does not disable credential persistence, exposing the GITHUB_TOKEN to all subsequent steps by default.
🔒 Suggested hardening patch
- name: Checkout repository
- uses: actions/checkout@v6
+ uses: actions/checkout@<full-commit-sha>
with:
fetch-depth: 1
+ persist-credentials: false
- name: Run Claude Code
id: claude
- uses: anthropics/claude-code-action@v1
+ uses: anthropics/claude-code-action@<full-commit-sha>🧰 Tools
🪛 zizmor (1.25.2)
[warning] 31-34: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 32-32: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 38-38: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/claude.yml around lines 31 - 42, The workflow uses mutable
action version tags that could be retargeted to malicious code, and the checkout
step does not disable credential persistence. Replace the mutable tag
`actions/checkout@v6` with a pinned SHA hash, replace
`anthropics/claude-code-action@v1` with a pinned SHA hash, and add
`persist-credentials: false` to the checkout step configuration to prevent the
GITHUB_TOKEN from being exposed to subsequent steps.
Source: Linters/SAST tools
| func validateSendArgs(req *nanorpc.NanoRPCRequest, cb RequestCallback) error { | ||
| switch req.RequestType { |
There was a problem hiding this comment.
Guard Send against a nil request before reading RequestType.
validateSendArgs dereferences req.RequestType unconditionally, so Send(nil, ...) panics instead of returning a typed invalid-argument error.
Suggested fix
func validateSendArgs(req *nanorpc.NanoRPCRequest, cb RequestCallback) error {
+ if req == nil {
+ return core.QuietWrap(os.ErrInvalid, "missing request")
+ }
switch req.RequestType {
case nanorpc.NanoRPCRequest_TYPE_PING:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func validateSendArgs(req *nanorpc.NanoRPCRequest, cb RequestCallback) error { | |
| switch req.RequestType { | |
| func validateSendArgs(req *nanorpc.NanoRPCRequest, cb RequestCallback) error { | |
| if req == nil { | |
| return core.QuietWrap(os.ErrInvalid, "missing request") | |
| } | |
| switch req.RequestType { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/nanorpc/client/session.go` around lines 230 - 231, The validateSendArgs
function dereferences req.RequestType unconditionally without first checking if
req is nil, which causes a panic when Send is called with a nil request. Add a
nil guard at the beginning of the validateSendArgs function that checks if req
is nil and returns an appropriate error (such as an invalid-argument error)
before attempting to access req.RequestType.
1 participant
Summary
Subscription lifecycle correctness across the client and server,
pinned by a protocol-document clarification that lands first so the
code can align with the new wording. Adjacent client and server
readiness primitives close doc/code gaps that the README had
referenced ahead of implementation.
Subscription lifecycle
Three commits address the request_id-sharing contract end-to-end.
docs(protocol): clarify subscription request_id lifetime and demux
closes two gaps in NANORPC_PROTOCOL.md §5.4 and §6.1: the
reservation window for a subscription's request_id is named
explicitly (
TYPE_SUBSCRIBEuntil the unsubscribe acknowledgementor session end), and the subscribe
TYPE_RESPONSEis distinguishedfrom the unsubscribe
TYPE_RESPONSEthat arrives on the same id.§6.1 picks up a
stateDiagram-v2of thePending/Active/Unsubscribing/Terminated transitions and a Phases
subsection naming which message types may arrive in each phase.
§6.3 and §10.1 cover the in-flight
TYPE_UPDATEthat may arrivebetween the unsubscribe request and its acknowledgement.
feat(client): surface subscribe ACKs and reject typed nils routes
the subscribe
TYPE_RESPONSEacknowledgement to the typedSubscribeCallbackas eitherErrSubscriptionEstablishedonSTATUS_OKor aResponseErroron any other status, via adispatcher split into
isSubscribeACK/subscribeACKErr/decodeSubscribePayload.callNewOutrejects typed-nil factoryresults through
core.IsNil;GetResponsegains the sameboundary protection on its client and out arguments.
ErrSubscriptionEstablishedandIsSubscriptionEstablishedlandin the root package.
fix(client): demux unsubscribe responses and guard Send fixes
the dispatcher:
popRequestCallbacknow matches on the(request_id, response_type)pair viaunsafeIndexCallbacks, sothe
TYPE_RESPONSEacknowledging an Unsubscribe no longer reachesthe still-registered
SubscribeCallback.Sendrejects anunsubscribe-form
TYPE_REQUESTwhose target subscription ismissing or still pending its acknowledgement, factored into
validateSendArgs/isUnsubscribeShape/checkUnsubscribeTarget/normaliseRequestID/registerCallback. Failure modes documented onSession.Sendand the three
Client.Unsubscribe*variants.popRequestCallback's routing matrix and theSendguard arecovered with data-driven tests.
Client readiness primitives
exposes
Connected(),IsConnected(), andWaitConnected(ctx)onClient.Connected()returns a channelthat closes while the client holds an active session and is
replaced after each disconnect;
IsConnected()is a point-in-timesnapshot;
WaitConnected(ctx)wrapsConnected()with thecaller's context. Closes the doc/code gap left by the
client/README.md"Connection Management" example, which hadreferenced both accessors before either existed.
Server ergonomic improvements
feat(server): accept a handler argument in NewDefaultServer
allows callers to pass an existing
*DefaultMessageHandler— andregister paths against it — before
Server.Servestarts. A nilhandler preserves the existing behaviour (a fresh
DefaultMessageHandlerwith an internalHashCacheis builtin-place). Integration test drives an echo handler through a real
TCP request.
feat(server): expose a Ready channel for the accept loop adds a
Ready() <-chan struct{}accessor that closes once the acceptloop has started taking connections, via an idempotent
signalReadyguarded bymu. The channel stays closed acrossshutdown so late observers never block. The test harness drops
its
time.Sleep(50 * time.Millisecond)heuristic for awaitServerReadyhelper with a 1s ceiling; a newTestServer_Readyconfirms the open-before-Serve /closed-after-loop / stays-closed-across-Shutdown sequence.
Test plan
makeis green — tidy across all four modules, cspell,shellcheck, build.
make testis green acrossgenerator,nanopb, andnanorpc.make raceis green; newTestServer_Readydoes not flake.pkg/nanorpc/client/README.mdandpkg/nanorpc/server/{README.md,doc.go}compile against theupdated API.
Summary by CodeRabbit
Connected(),IsConnected(), andWaitConnected(ctx).Ready().ErrSubscriptionEstablished(andIsSubscriptionEstablished) for subscription acknowledgements.