feat(tunnel-agent): add protocol v1 contract - #74
mozharovsky wants to merge 13 commits into
Conversation
Advertise compatible protocol versions, provide stable structured failures, and allow supervisors to cancel in-flight requests without breaking legacy reply fields.
|
@coderabbitai review |
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe tunnel serving protocol now supports version negotiation, capability metadata, stable structured errors, request cancellation, duplicate-ID rejection, and in-flight task cleanup. The CLI reports serving metadata, and documentation and tests cover the new protocol behavior. ChangesTunnel protocol lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant TunnelAgentIPC
participant InFlightRequestRegistry
participant TunnelAgentProtocol
Client->>TunnelAgentIPC: Send request with protocolVersion
TunnelAgentIPC->>TunnelAgentProtocol: Validate version and normalize failures
TunnelAgentIPC->>InFlightRequestRegistry: Register request
Client->>TunnelAgentIPC: Send cancel request
TunnelAgentIPC->>InFlightRequestRegistry: Cancel target request
InFlightRequestRegistry-->>TunnelAgentIPC: Return cancellation outcome
TunnelAgentIPC-->>Client: Send structured terminal reply
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (3)
Sources/RorkDeviceCLI/RorkDeviceCommand.swift (1)
1801-1805: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winClarify that
protocolVersionis advertised, not negotiated.
TunnelReadyEventis emitted before the supervisor sends a versioned request, and this initializer always assignsTunnelAgentProtocol.currentVersion. This field describes the agent's implemented protocol version. Rename the comment to “Current agent protocol version” or define a per-session selected version before calling it negotiated. This avoids ambiguity if the agent later supports multiple protocol versions.🤖 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 `@Sources/RorkDeviceCLI/RorkDeviceCommand.swift` around lines 1801 - 1805, Update the documentation for TunnelReadyEvent’s protocolVersion property to describe it as the current or advertised agent protocol version, not a negotiated version; retain supportedProtocolVersions as the negotiation capability field unless a per-session selected version is actually introduced.Sources/RorkDevice/CoreDevice/TunnelAgentProtocol.swift (1)
79-111: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlign the
targetIDwire key with the request-sidetargetIdspelling.The cancel request reads
targetId(seeCancelParametersinSources/RorkDevice/CoreDevice/TunnelAgentIPC.swiftlines 322-324), but the failure details emittargetID. The same concept then has two spellings on the wire. This contract is being frozen as protocol v1, so the mismatch is cheap to fix now and expensive later.♻️ Proposed fix to unify the wire key
struct TunnelAgentErrorDetails: Encodable, Sendable { let requestedVersion: Int? let supportedVersions: [Int]? let operation: String? let targetID: String? @@ + private enum CodingKeys: String, CodingKey { + case requestedVersion + case supportedVersions + case operation + case targetID = "targetId" + case streamIdentifier + case protocolErrorCode + case afcStatus + case misagentStatus + case reason + }Update
Tests/RorkDeviceTests/TunnelAgentIPCTests.swiftline 408 accordingly if you apply this.🤖 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 `@Sources/RorkDevice/CoreDevice/TunnelAgentProtocol.swift` around lines 79 - 111, Rename the encoded failure-detail property in TunnelAgentErrorDetails from targetID to targetId so its wire key matches CancelParameters and the protocol v1 contract. Update all references and the corresponding expectation in TunnelAgentIPCTests to use targetId, preserving the existing value and behavior.Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift (1)
442-478: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConfirm the reply-ordering guarantee for a request that finishes before
startreturns.
startcreates the task at Line 469 and inserts the entry at Line 473. Actor isolation prevents the task from callingfinishbeforestartreturns, soentries[id]is always present. The current code is correct.The correctness depends on
startcontaining no suspension point between the two statements. A futureawaitadded between Lines 469 and 473 would letfinishrun first, hit theguardat Line 511, and drop the terminal reply silently. Insert the entry before creating the task to remove the ordering dependency.♻️ Proposed refactor to remove the ordering dependency
- let task = Task { [weak self] in - let reply = await operation() - await self?.finish(id: id, proposedReply: reply) - } - entries[id] = Entry( - task: task, - cancellationRequested: false - ) - return true + let task = Task { [weak self] in + let reply = await operation() + await self?.finish(id: id, proposedReply: reply) + } + // The entry must be recorded before any suspension point, so `finish` + // never runs against a missing entry. + entries[id] = Entry(task: task, cancellationRequested: false) + return trueA stronger form assigns a placeholder entry first, then replaces its
taskfield.🤖 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 `@Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift` around lines 442 - 478, Refactor InFlightRequestRegistry.start so the identifier is reserved in entries before creating the Task, eliminating the ordering dependency on the absence of suspension points. Adjust Entry or use a placeholder-and-replacement approach so the task can be assigned after reservation, while preserving duplicate rejection and finish(id:proposedReply:) terminal-reply handling.
🤖 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 `@Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift`:
- Around line 186-194: Update Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift
lines 186-194 to emit duplicate_request_id as event "op-error" rather than the
default "op-result", and make the same change in the !started rejection path at
lines 247-254. In the cancel handling around lines 212-219, reserve the cancel
request’s own id in InFlightRequestRegistry before routing so repeated cancel
ids use the duplicate-request rejection path.
In `@Tests/RorkDeviceTests/TunnelAgentIPCTests.swift`:
- Around line 375-378: In Tests/RorkDeviceTests/TunnelAgentIPCTests.swift at
lines 375-378, close stdin and await serving.value before asserting that
replies.replies(id: "slow") contains exactly one reply. At lines 445-450,
likewise await serving shutdown before asserting that "same" has exactly two
replies, then validate their error codes.
---
Nitpick comments:
In `@Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift`:
- Around line 442-478: Refactor InFlightRequestRegistry.start so the identifier
is reserved in entries before creating the Task, eliminating the ordering
dependency on the absence of suspension points. Adjust Entry or use a
placeholder-and-replacement approach so the task can be assigned after
reservation, while preserving duplicate rejection and finish(id:proposedReply:)
terminal-reply handling.
In `@Sources/RorkDevice/CoreDevice/TunnelAgentProtocol.swift`:
- Around line 79-111: Rename the encoded failure-detail property in
TunnelAgentErrorDetails from targetID to targetId so its wire key matches
CancelParameters and the protocol v1 contract. Update all references and the
corresponding expectation in TunnelAgentIPCTests to use targetId, preserving the
existing value and behavior.
In `@Sources/RorkDeviceCLI/RorkDeviceCommand.swift`:
- Around line 1801-1805: Update the documentation for TunnelReadyEvent’s
protocolVersion property to describe it as the current or advertised agent
protocol version, not a negotiated version; retain supportedProtocolVersions as
the negotiation capability field unless a per-session selected version is
actually introduced.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 027c5f4d-a386-4b1c-931a-4926b4e0576a
📒 Files selected for processing (7)
Docs/Roadmap.mdREADME.mdSources/RorkDevice/CoreDevice/TunnelAgentIPC.swiftSources/RorkDevice/CoreDevice/TunnelAgentProtocol.swiftSources/RorkDeviceCLI/RorkDeviceCommand.swiftTests/RorkDeviceCLITests/RorkDeviceCLITests.swiftTests/RorkDeviceTests/TunnelAgentIPCTests.swift
Reserve every in-flight protocol request, distinguish duplicate envelope errors, and make exact reply-count tests wait for complete shutdown.
|
@coderabbitai review |
|
Use an open raw-value type so protocol-v1 clients can decode additive error codes introduced by newer agents.
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 1 minute. |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 29 seconds. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift (1)
399-413: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider an enum for
eventsookcannot silently disappear.
failurederivesokfrom a string comparison against"op-result". A caller that passes any other event string produces a reply with nookfield. Today only"op-result"and"op-error"are used, so the wire output is correct. A typedeventwould make the wrong combination unrepresentable.♻️ Sketch
+private enum ReplyEvent: String, Encodable, Sendable { + case result = "op-result" + case error = "op-error" +} + static func failure( - event: String = "op-result", + event: ReplyEvent = .result, id: String?, failure: TunnelAgentFailure ) -> Reply { Reply( - event: event, + event: event.rawValue, id: id, - ok: event == "op-result" ? false : nil, + ok: event == .result ? false : nil,🤖 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 `@Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift` around lines 399 - 413, Update the failure reply API around static failure to use a typed event enum covering the supported "op-result" and "op-error" values, and derive the ok field from that enum instead of comparing arbitrary strings. Update callers and serialization so the existing wire event values and expected ok behavior remain unchanged while invalid event combinations become unrepresentable.
🤖 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 `@Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift`:
- Around line 160-163: Bound the shutdown join in the serve cleanup flow around
inFlightRequests.cancelAll so cancellation cannot indefinitely block agent exit.
Track completion explicitly while awaiting cancelled task values, race that join
against a shutdown deadline, and ensure an already-completed join wins over the
deadline; when the deadline expires, emit terminal replies for every unfinished
request before returning.
- Around line 533-555: Add a structured cancellation-race detail field to
TunnelAgentErrorDetails in TunnelAgentProtocol.swift, then populate it in finish
when entry.cancellationRequested overrides proposedReply, indicating that the
operation may have completed before cancellation was observed. Preserve the
existing deterministic cancelled reply and ensure supervisors can reconcile
potentially applied side effects from the structured detail.
---
Nitpick comments:
In `@Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift`:
- Around line 399-413: Update the failure reply API around static failure to use
a typed event enum covering the supported "op-result" and "op-error" values, and
derive the ok field from that enum instead of comparing arbitrary strings.
Update callers and serialization so the existing wire event values and expected
ok behavior remain unchanged while invalid event combinations become
unrepresentable.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4522130e-60d1-4fce-9f82-3e0febceabb8
📒 Files selected for processing (7)
Docs/Roadmap.mdREADME.mdSources/RorkDevice/CoreDevice/TunnelAgentIPC.swiftSources/RorkDevice/CoreDevice/TunnelAgentProtocol.swiftSources/RorkDeviceCLI/RorkDeviceCommand.swiftTests/RorkDeviceCLITests/RorkDeviceCLITests.swiftTests/RorkDeviceTests/TunnelAgentIPCTests.swift
Emit deterministic terminal replies after a finite cancellation grace and report when accepted cancellation may race completed side effects.
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 22 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift (2)
118-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the advertisement list against dispatcher drift.
builtInOperationNamesand the dispatch table are two separate sources. If a name is added tobuiltInOperationNameswithout a matching entry inbuiltInHandlersor a branch indispatch, the agent advertises an operation that answersunknown_operation.TunnelAgentOperationsavoids this with one table (Sources/RorkDeviceCLI/TunnelAgentOperations.swift, lines 31-35).A single assertion keeps the two lists aligned without restructuring the API.
♻️ Suggested guard test
func testEveryBuiltInOperationIsServed() { let handled = Set( TunnelAgentIPC.builtInHandlers(capabilities: []).keys ).union(["cancel"]) XCTAssertEqual( Set(TunnelAgentIPC.builtInOperationNames), handled ) }🤖 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 `@Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift` around lines 118 - 128, Update builtInHandlers to assert that its handler keys, combined with the separately served "cancel" operation, exactly match TunnelAgentIPC.builtInOperationNames. Keep the existing handler construction and public API unchanged, and add the suggested alignment test if the project’s test structure supports it.
566-594: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
operationMayHaveCompletedis a constant, so it carries no information.
cancelledReplyalways setsoperationMayHaveCompletedtotrue. Every cancelled reply therefore reports the same value, and a supervisor cannot separate a request cancelled before any work landed from a request that finished during the cancellation race.In
finish, the registry already holdsproposedReply, so it can report the race outcome precisely.finishRemainingAsCancelledhas no proposed reply and correctly stays conservative.If the conservative constant is intentional for protocol v1, this is fine as written. The README already documents the rule at lines 408-412.
♻️ Optional precision for the `finish` path
private func finish( id: String, proposedReply: Reply ) { guard let entry = entries.removeValue(forKey: id) else { return } if entry.cancellationRequested { - writer.write(cancelledReply(id: id)) + writer.write( + cancelledReply( + id: id, + mayHaveCompleted: proposedReply.ok == true + ) + ) } else { writer.write(proposedReply) } } - private func cancelledReply(id: String) -> Reply { + private func cancelledReply( + id: String, + mayHaveCompleted: Bool = true + ) -> Reply { Reply.failure( id: id, failure: TunnelAgentFailure( code: .cancelled, message: "The request was cancelled.", details: TunnelAgentErrorDetails( - operationMayHaveCompleted: true + operationMayHaveCompleted: mayHaveCompleted ) ) ) }
testCancelsAnInFlightRequestExactlyOncewould then need a handler that completes during the race to keep assertingtrue.🤖 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 `@Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift` around lines 566 - 594, Make cancelledReply report whether the operation may have completed based on the proposedReply supplied to finish, rather than always using true. Update finish and cancelledReply to propagate the race outcome, while leaving finishRemainingAsCancelled’s conservative behavior 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 `@README.md`:
- Around line 364-374: Update the cancelled run reply in the README protocol
example to include the emitted errorDetails.operationMayHaveCompleted field,
matching the payload generated by cancelledReply and the documented rule below;
leave the surrounding cancellation sequence unchanged.
---
Nitpick comments:
In `@Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift`:
- Around line 118-128: Update builtInHandlers to assert that its handler keys,
combined with the separately served "cancel" operation, exactly match
TunnelAgentIPC.builtInOperationNames. Keep the existing handler construction and
public API unchanged, and add the suggested alignment test if the project’s test
structure supports it.
- Around line 566-594: Make cancelledReply report whether the operation may have
completed based on the proposedReply supplied to finish, rather than always
using true. Update finish and cancelledReply to propagate the race outcome,
while leaving finishRemainingAsCancelled’s conservative behavior unchanged.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eb6e053e-df0c-4a5c-a6cc-95d5ba9dafe4
📒 Files selected for processing (7)
Docs/Roadmap.mdREADME.mdSources/RorkDevice/CoreDevice/TunnelAgentIPC.swiftSources/RorkDevice/CoreDevice/TunnelAgentProtocol.swiftSources/RorkDeviceCLI/RorkDeviceCommand.swiftTests/RorkDeviceCLITests/RorkDeviceCLITests.swiftTests/RorkDeviceTests/TunnelAgentIPCTests.swift
Differentiate cooperative cancellation from a concurrent successful completion and lock built-in capability advertisements to served operations.
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 11 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift (1)
203-212: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
containspre-check duplicates the atomic reservation instart.
start(id:operation:)already rejects a reserved identifier and returnsfalse, and both rejection paths write the sameduplicate_request_idfailure. This pre-check adds one actor hop per request and a second place to keep in sync. It changes behavior only forcanceland unknown operations, where a duplicate id is reported before the operation is examined.If that ordering is intentional, keep the check and record the reason in a comment. Otherwise remove it and rely on
start.🤖 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 `@Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift` around lines 203 - 212, Remove the inFlightRequests.contains pre-check in the request handling flow and rely on start(id:operation:) for atomic duplicate-ID reservation and rejection. Preserve the existing duplicate_request_id failure handling returned from start, allowing cancel and unknown operations to be validated according to their normal operation handling.Tests/RorkDeviceTests/TunnelAgentIPCTests.swift (1)
94-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe hardcoded
"cancel"weakens the assertion.The test claims that every advertised built-in operation is served. It adds
"cancel"to the handled set by hand, so the assertion stays green even if the serving loop stops routingcancel. Consider exercising acancelrequest throughservein this test, or add a comment that states the serving loop ownscanceland thattestRejectsCancellationForAnUnknownRequestcovers the routing.🤖 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 `@Tests/RorkDeviceTests/TunnelAgentIPCTests.swift` around lines 94 - 103, The testEveryAdvertisedBuiltInOperationIsServed assertion should not rely on the hardcoded "cancel" entry. Exercise a cancellation request through serve so the test verifies the serving loop routes cancel, or document that serve owns this operation and explicitly rely on testRejectsCancellationForAnUnknownRequest for coverage.
🤖 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 `@README.md`:
- Around line 405-416: The cancellation contract must distinguish whether the
operation may have completed. Update README.md lines 405-416 to state that
cancelled replies include operationMayHaveCompleted, with false for cancellation
observed without success and true for concurrent successful completion or
expired shutdown grace; update TunnelAgentFailure.normalize in
Sources/RorkDevice/CoreDevice/TunnelAgentProtocol.swift lines 191-196 to attach
TunnelAgentErrorDetails(operationMayHaveCompleted: true) to handler-thrown
CancellationError failures.
---
Nitpick comments:
In `@Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift`:
- Around line 203-212: Remove the inFlightRequests.contains pre-check in the
request handling flow and rely on start(id:operation:) for atomic duplicate-ID
reservation and rejection. Preserve the existing duplicate_request_id failure
handling returned from start, allowing cancel and unknown operations to be
validated according to their normal operation handling.
In `@Tests/RorkDeviceTests/TunnelAgentIPCTests.swift`:
- Around line 94-103: The testEveryAdvertisedBuiltInOperationIsServed assertion
should not rely on the hardcoded "cancel" entry. Exercise a cancellation request
through serve so the test verifies the serving loop routes cancel, or document
that serve owns this operation and explicitly rely on
testRejectsCancellationForAnUnknownRequest for coverage.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6009fb6d-c483-4bbe-accc-69f8ac1b993c
📒 Files selected for processing (7)
Docs/Roadmap.mdREADME.mdSources/RorkDevice/CoreDevice/TunnelAgentIPC.swiftSources/RorkDevice/CoreDevice/TunnelAgentProtocol.swiftSources/RorkDeviceCLI/RorkDeviceCommand.swiftTests/RorkDeviceCLITests/RorkDeviceCLITests.swiftTests/RorkDeviceTests/TunnelAgentIPCTests.swift
Keep built-in capability sources synchronized and distinguish observed cancellation from a successful completion race in structured replies.
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 2 minutes. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Tests/RorkDeviceTests/TunnelAgentIPCTests.swift (1)
416-457: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTeardown runs only on the success path in two gated tests. Both tests place their cleanup statements after the assertions. A throwing assertion such as
XCTUnwrapreturns early and skips that cleanup, which leaves a serving task or a suspendedCheckedContinuationalive for the rest of the test process.
Tests/RorkDeviceTests/TunnelAgentIPCTests.swift#L416-L457: adddefer { serving.cancel() }after the serving task is created, matching the other tests in this file.Tests/RorkDeviceTests/TunnelAgentIPCTests.swift#L599-L634: moveawait gate.open()to immediately afterawait serving.value, before the assertions.🤖 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 `@Tests/RorkDeviceTests/TunnelAgentIPCTests.swift` around lines 416 - 457, Update testCancellationReportsAConcurrentSuccessfulCompletion in Tests/RorkDeviceTests/TunnelAgentIPCTests.swift (lines 416-457) to add defer { serving.cancel() } immediately after creating serving. In the second gated test at Tests/RorkDeviceTests/TunnelAgentIPCTests.swift (lines 599-634), move await gate.open() to immediately after await serving.value and before assertions; no other cleanup changes are needed.
🤖 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.
Nitpick comments:
In `@Tests/RorkDeviceTests/TunnelAgentIPCTests.swift`:
- Around line 416-457: Update
testCancellationReportsAConcurrentSuccessfulCompletion in
Tests/RorkDeviceTests/TunnelAgentIPCTests.swift (lines 416-457) to add defer {
serving.cancel() } immediately after creating serving. In the second gated test
at Tests/RorkDeviceTests/TunnelAgentIPCTests.swift (lines 599-634), move await
gate.open() to immediately after await serving.value and before assertions; no
other cleanup changes are needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 39df497e-0a81-47c4-ac1f-2879b6048279
📒 Files selected for processing (7)
Docs/Roadmap.mdREADME.mdSources/RorkDevice/CoreDevice/TunnelAgentIPC.swiftSources/RorkDevice/CoreDevice/TunnelAgentProtocol.swiftSources/RorkDeviceCLI/RorkDeviceCommand.swiftTests/RorkDeviceCLITests/RorkDeviceCLITests.swiftTests/RorkDeviceTests/TunnelAgentIPCTests.swift
Normalize cancellation metadata through one constructor, protect active identifiers from malformed reuse, and make gated test teardown unconditional.
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 18 minutes. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift (1)
82-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider one spelling for the supported-version list before v1 freezes.
CapabilitiesPayloadpublishes the list assupportedProtocolVersions. The rejection payload publishes the same list assupportedVersions, throughTunnelAgentErrorDetails.supportedVersionsinSources/RorkDevice/CoreDevice/TunnelAgentProtocol.swiftat Lines 122 and 134. The test atTests/RorkDeviceTests/TunnelAgentIPCTests.swiftLines 226-253 asserts both spellings.A client must then learn two key names for one concept. Aligning them is cheap now. After v1 freezes, the rename needs a protocol revision.
Keep the current keys if the asymmetry is deliberate, for example if
errorDetailsuses short names by design. Record the rule in the protocol contract.🤖 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 `@Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift` around lines 82 - 85, Align the supported-version field naming across CapabilitiesPayload and TunnelAgentErrorDetails so both payloads use one consistent key, updating TunnelAgentProtocol definitions and TunnelAgentIPCTests assertions accordingly. If the differing names are intentional, retain them and document the naming rule in the protocol contract.
🤖 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.
Nitpick comments:
In `@Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift`:
- Around line 82-85: Align the supported-version field naming across
CapabilitiesPayload and TunnelAgentErrorDetails so both payloads use one
consistent key, updating TunnelAgentProtocol definitions and TunnelAgentIPCTests
assertions accordingly. If the differing names are intentional, retain them and
document the naming rule in the protocol contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 772f2d99-dd6b-47a9-9a61-e1ff8a68a257
📒 Files selected for processing (7)
Docs/Roadmap.mdREADME.mdSources/RorkDevice/CoreDevice/TunnelAgentIPC.swiftSources/RorkDevice/CoreDevice/TunnelAgentProtocol.swiftSources/RorkDeviceCLI/RorkDeviceCommand.swiftTests/RorkDeviceCLITests/RorkDeviceCLITests.swiftTests/RorkDeviceTests/TunnelAgentIPCTests.swift
Use supportedProtocolVersions consistently across capability and error-detail payloads before freezing protocol v1.
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 8 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift (1)
400-419: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRecord
errorCodeanderrorDetailsas reserved top-level keys.
encodemerges the payload into the same top-level object at Line 462. The reserved key set grew fromevent,id,ok,errorto includeerrorCodeanderrorDetails. If a handler payload ever carries one of those names,JSONEncoderwrites the key twice and the payload value silently replaces the envelope value. No error is raised.Name the full reserved set in the
payloaddoc comment so handler authors avoid the collision.📝 Proposed doc change
/// Operation-specific fields flattened into the reply, or nil when the - /// envelope says everything. + /// envelope says everything. The envelope reserves the top-level keys + /// `event`, `id`, `ok`, `error`, `errorCode`, and `errorDetails`. A + /// payload that reuses one of those names silently replaces the + /// envelope's value. let payload: (any Encodable & Sendable)?🤖 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 `@Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift` around lines 400 - 419, Update the payload documentation in the reply type to explicitly list all reserved top-level keys: event, id, ok, error, errorCode, and errorDetails. Keep the existing encoding behavior unchanged and clarify that handler payload fields must not use these names.
🧹 Nitpick comments (2)
Tests/RorkDeviceTests/TunnelAgentIPCTests.swift (2)
450-457: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the terminal outcome before reading
errorDetails.The test only inspects
operationMayHaveCompleted. If the registry returned the handler's success reply instead of the cancelled reply,XCTUnwrapreports a generic nil-unwrap failure. Explicit assertions onokanderrorCodename the actual regression and pin the terminal code for this race.♻️ Proposed additional assertions
let reply = try await replies.waitForReply(id: "work") + XCTAssertEqual(reply["ok"] as? Bool, false) + XCTAssertEqual(reply["errorCode"] as? String, "cancelled") let details = try XCTUnwrap( reply["errorDetails"] as? [String: Any] )🤖 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 `@Tests/RorkDeviceTests/TunnelAgentIPCTests.swift` around lines 450 - 457, Update the test around replies.waitForReply(id: "work") to first assert the terminal reply has ok set to false and the expected cancellation errorCode, then unwrap errorDetails and continue validating operationMayHaveCompleted. This makes an unexpected success reply fail with explicit outcome assertions rather than a generic nil-unwrap.
701-730: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider one generic polling helper.
waitForReply,waitForReplies, andwaitForEventnow repeat the same 400-iteration, 5 ms poll loop. A single private helper that takes a predicate and a description removes the duplication and keeps the timeout budget in one place.♻️ Proposed consolidation
+ private func poll<T>( + _ description: `@autoclosure` () -> String, + until produce: () -> T? + ) async throws -> T { + for _ in 0..<400 { + if let value = produce() { + return value + } + try await Task.sleep(for: .milliseconds(5)) + } + throw RorkDeviceError.transport(description()) + } + func waitForReplies( id: String, count: Int ) async throws -> [[String: Any]] { - for _ in 0..<400 { - let matches = replies(id: id) - if matches.count >= count { - return matches - } - try await Task.sleep(for: .milliseconds(5)) - } - throw RorkDeviceError.transport( - "Expected \(count) replies for request \(id)." - ) + try await poll("Expected \(count) replies for request \(id).") { + let matches = replies(id: id) + return matches.count >= count ? matches : nil + } }🤖 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 `@Tests/RorkDeviceTests/TunnelAgentIPCTests.swift` around lines 701 - 730, Consolidate the repeated polling logic in waitForReply, waitForReplies, and waitForEvent into one private generic helper that accepts a matching predicate and timeout description. Keep the existing 400-iteration, 5 ms polling budget and preserve each method’s current return values and timeout error messages by adapting their specific matching logic to the helper.
🤖 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 `@Docs/Roadmap.md`:
- Around line 220-224: Update the Node client startup requirement in the roadmap
to validate that the server’s protocolVersion is included in
supportedProtocolVersions and that all required capability operation names are
present, without referring to capability versions.
In `@Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift`:
- Around line 581-599: Update finish so cancellation replies set
operationMayHaveCompleted to false only when proposedReply represents a
cancellation-normalized result; treat ordinary handler failures as potentially
having completed and preserve their failure details for supervisor reporting.
Use the existing Reply cancellation/error representation and keep non-cancelled
replies unchanged.
---
Outside diff comments:
In `@Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift`:
- Around line 400-419: Update the payload documentation in the reply type to
explicitly list all reserved top-level keys: event, id, ok, error, errorCode,
and errorDetails. Keep the existing encoding behavior unchanged and clarify that
handler payload fields must not use these names.
---
Nitpick comments:
In `@Tests/RorkDeviceTests/TunnelAgentIPCTests.swift`:
- Around line 450-457: Update the test around replies.waitForReply(id: "work")
to first assert the terminal reply has ok set to false and the expected
cancellation errorCode, then unwrap errorDetails and continue validating
operationMayHaveCompleted. This makes an unexpected success reply fail with
explicit outcome assertions rather than a generic nil-unwrap.
- Around line 701-730: Consolidate the repeated polling logic in waitForReply,
waitForReplies, and waitForEvent into one private generic helper that accepts a
matching predicate and timeout description. Keep the existing 400-iteration, 5
ms polling budget and preserve each method’s current return values and timeout
error messages by adapting their specific matching logic to the helper.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9051cbb3-4304-40d3-81db-b4fd7a84d841
📒 Files selected for processing (7)
Docs/Roadmap.mdREADME.mdSources/RorkDevice/CoreDevice/TunnelAgentIPC.swiftSources/RorkDevice/CoreDevice/TunnelAgentProtocol.swiftSources/RorkDeviceCLI/RorkDeviceCommand.swiftTests/RorkDeviceCLITests/RorkDeviceCLITests.swiftTests/RorkDeviceTests/TunnelAgentIPCTests.swift
Document concrete startup negotiation, retain concurrent operation failures inside cancelled replies, and consolidate protocol polling tests.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Tests/RorkDeviceTests/TunnelAgentIPCTests.swift (1)
18-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the force unwrap with
XCTUnwrap.Line 20 force-unwraps
String(data:encoding:). If the value isnil, the test process traps instead of reporting one failed test. The function already declaresthrows, soXCTUnwrapreports a normal failure.♻️ Proposed refactor
XCTAssertNil(request.protocolVersion) // Handlers decode operation-specific fields from the retained line. - XCTAssertTrue(String(data: request.line, encoding: .utf8)!.contains(#""type":"all""#)) + let body = try XCTUnwrap(String(data: request.line, encoding: .utf8)) + XCTAssertTrue(body.contains(#""type":"all""#))🤖 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 `@Tests/RorkDeviceTests/TunnelAgentIPCTests.swift` around lines 18 - 21, Update the test assertion after `request.line` decoding to use `XCTUnwrap` instead of force-unwrapping `String(data:encoding:)`, preserving the existing containment check and leveraging the test method’s `throws` declaration.
🤖 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.
Nitpick comments:
In `@Tests/RorkDeviceTests/TunnelAgentIPCTests.swift`:
- Around line 18-21: Update the test assertion after `request.line` decoding to
use `XCTUnwrap` instead of force-unwrapping `String(data:encoding:)`, preserving
the existing containment check and leveraging the test method’s `throws`
declaration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bac76bf4-d33e-40c1-b935-aaee12ac0b70
📒 Files selected for processing (7)
Docs/Roadmap.mdREADME.mdSources/RorkDevice/CoreDevice/TunnelAgentIPC.swiftSources/RorkDevice/CoreDevice/TunnelAgentProtocol.swiftSources/RorkDeviceCLI/RorkDeviceCommand.swiftTests/RorkDeviceCLITests/RorkDeviceCLITests.swiftTests/RorkDeviceTests/TunnelAgentIPCTests.swift
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 11 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Document every protocol declaration and make the wire-version and cancellation guarantees explicit for supervisors.
Make every introduced declaration readable as a complete contract and align private identifier naming with Swift conventions.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@README.md`:
- Around line 377-383: Update the identifier-lifecycle wording in the README
protocol description to say that an identifier remains reserved while its
request is active and may be reused only after the request’s terminal op-result
or op-error. Preserve the existing duplicate in-flight behavior, including
returning op-error without terminating the original operation.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ed5f07b-dacc-4d79-a32b-0b42e64c1444
📒 Files selected for processing (6)
Docs/Roadmap.mdREADME.mdSources/RorkDevice/CoreDevice/TunnelAgentIPC.swiftSources/RorkDevice/CoreDevice/TunnelAgentProtocol.swiftSources/RorkDeviceCLI/RorkDeviceCommand.swiftTests/RorkDeviceTests/TunnelAgentIPCTests.swift
🚧 Files skipped from review as they are similar to previous changes (4)
- Docs/Roadmap.md
- Sources/RorkDeviceCLI/RorkDeviceCommand.swift
- Tests/RorkDeviceTests/TunnelAgentIPCTests.swift
- Sources/RorkDevice/CoreDevice/TunnelAgentIPC.swift
| New supervisors should request `capabilities`, choose a value present in | ||
| `supportedProtocolVersions`, and send that value as `protocolVersion` on later | ||
| requests. The agent validates the requested version rather than negotiating | ||
| one. The serving `ready` event carries the same protocol and agent version | ||
| fields beside its capability list. Request identifiers must remain unique until | ||
| their terminal `op-result`. They may be reused afterward. A duplicate in-flight | ||
| identifier receives an `op-error` without terminating the original operation. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Define identifier reuse against the terminal reply.
TunnelAgentIPC.dispatch sends op-error for malformed input with a salvaged id, while valid requests normally receive op-result. Therefore, “until their terminal op-result” does not cover every terminal response. State that an identifier remains reserved while the request is active and may be reused after its terminal op-result or op-error.
Proposed wording
-Request identifiers must remain unique until their terminal `op-result`. They may be reused afterward.
+A request identifier must remain unique while its request is active. It may be reused after the terminal `op-result` or `op-error`.📝 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.
| New supervisors should request `capabilities`, choose a value present in | |
| `supportedProtocolVersions`, and send that value as `protocolVersion` on later | |
| requests. The agent validates the requested version rather than negotiating | |
| one. The serving `ready` event carries the same protocol and agent version | |
| fields beside its capability list. Request identifiers must remain unique until | |
| their terminal `op-result`. They may be reused afterward. A duplicate in-flight | |
| identifier receives an `op-error` without terminating the original operation. | |
| New supervisors should request `capabilities`, choose a value present in | |
| `supportedProtocolVersions`, and send that value as `protocolVersion` on later | |
| requests. The agent validates the requested version rather than negotiating | |
| one. The serving `ready` event carries the same protocol and agent version | |
| fields beside its capability list. A request identifier must remain unique while its request is active. It may be reused after the terminal `op-result` or `op-error`. |
🤖 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 `@README.md` around lines 377 - 383, Update the identifier-lifecycle wording in
the README protocol description to say that an identifier remains reserved while
its request is active and may be reused only after the request’s terminal
op-result or op-error. Preserve the existing duplicate in-flight behavior,
including returning op-error without terminating the original operation.
1 participant
Summary
Testing
swift test(574 tests passed, 1 skipped)Summary by CodeRabbit
New Features
Documentation
Tests