fix: Busy responses now show as temporary status messages#1227
Merged
Conversation
Busy states are temporary command rejections, not severe Unity failures. Emit a lightweight Status/Message JSON for server_busy RPC responses while keeping the internal retryable classification for CLI handling.
Contributor
📝 WalkthroughWalkthroughAdds a "Unity is busy" status envelope with RPC-informed messaging for ChangesBusy Status Error Handling
Sequence DiagramsequenceDiagram
participant ErrorHandler as classifyError
participant ServerBusyErr as unityServerBusyError
participant MessageBuild as unityServerBusyMessage
participant ErrorWriter as writeErrorEnvelope
participant BusyWriter as writeBusyStatusEnvelope
participant Output as JSON Output
ErrorHandler->>ServerBusyErr: route server_busy RPC error + decodedData
ServerBusyErr->>MessageBuild: build message from RPC data + context
MessageBuild->>ServerBusyErr: return formatted retry message
ServerBusyErr->>ErrorHandler: return classified error
ErrorHandler->>ErrorWriter: pass error with message
ErrorWriter->>BusyWriter: dispatch UNITY_SERVER_BUSY error
BusyWriter->>Output: write cliStatusEnvelope as JSON
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Packages/src/Cli~/internal/cli/error_envelope_test.go (1)
240-266: ⚡ Quick winAdd a fallback-path test for missing tool names in
server_busydata.This block covers the fully populated payload path, but not the branch that falls back to
rpcErr.MessagewhenrunningToolName/requestedToolNameare absent.Proposed test addition
func TestWriteClassifiedServerBusyRPCErrorWritesBusyStatus(t *testing.T) { @@ if bytes.Contains(stderr.Bytes(), []byte("Success")) || bytes.Contains(stderr.Bytes(), []byte("errorCode")) { t.Fatalf("busy output should not include error envelope fields: %s", stderr.String()) } } + +func TestWriteClassifiedServerBusyRPCErrorFallsBackToRPCMessage(t *testing.T) { + rpcErr := &unityipc.RPCError{ + Code: -32603, + Message: "Unity is busy. Retry after the running tool completes.", + Data: json.RawMessage(`{"type":"server_busy"}`), + } + var stderr bytes.Buffer + + writeClassifiedError(&stderr, rpcErr, errorContext{projectRoot: "/tmp/MyProject", command: "get-logs"}) + + var envelope cliStatusEnvelope + if err := json.Unmarshal(stderr.Bytes(), &envelope); err != nil { + t.Fatalf("stderr is not valid JSON: %v\n%s", err, stderr.String()) + } + if envelope.Status != cliStatusBusy { + t.Fatalf("status mismatch: %#v", envelope) + } + if envelope.Message != rpcErr.Message { + t.Fatalf("fallback message mismatch: %#v", envelope) + } +}🤖 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 `@Packages/src/Cli`~/internal/cli/error_envelope_test.go around lines 240 - 266, Add a new unit test that exercises the fallback branch when server_busy data omits tool names: create an RPCError (unityipc.RPCError) whose Data either lacks runningToolName/requestedToolName or is empty but whose Message contains the fallback text, call writeClassifiedError with the same errorContext used in TestWriteClassifiedServerBusyRPCErrorWritesBusyStatus, unmarshal stderr into cliStatusEnvelope and assert envelope.Status == cliStatusBusy and envelope.Message == rpcErr.Message (the fallback), and finally assert the output does not contain full envelope fields like "Success" or "errorCode"; mirror naming and structure of TestWriteClassifiedServerBusyRPCErrorWritesBusyStatus for consistency.
🤖 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 `@Packages/src/Cli`~/internal/cli/error_envelope_test.go:
- Around line 240-266: Add a new unit test that exercises the fallback branch
when server_busy data omits tool names: create an RPCError (unityipc.RPCError)
whose Data either lacks runningToolName/requestedToolName or is empty but whose
Message contains the fallback text, call writeClassifiedError with the same
errorContext used in TestWriteClassifiedServerBusyRPCErrorWritesBusyStatus,
unmarshal stderr into cliStatusEnvelope and assert envelope.Status ==
cliStatusBusy and envelope.Message == rpcErr.Message (the fallback), and finally
assert the output does not contain full envelope fields like "Success" or
"errorCode"; mirror naming and structure of
TestWriteClassifiedServerBusyRPCErrorWritesBusyStatus for consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d46ba688-da46-470d-a95f-674b507a90a2
⛔ Files ignored due to path filters (3)
Packages/src/Cli~/dist/darwin-amd64/uloopis excluded by!**/dist/**and included by nonePackages/src/Cli~/dist/darwin-arm64/uloopis excluded by!**/dist/**and included by nonePackages/src/Cli~/dist/windows-amd64/uloop.exeis excluded by!**/dist/**,!**/*.exeand included by none
📒 Files selected for processing (4)
Packages/src/Cli~/internal/cli/busy_status.goPackages/src/Cli~/internal/cli/error_envelope.goPackages/src/Cli~/internal/cli/error_envelope_test.goPackages/src/Cli~/internal/cli/string_helpers.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
User Impact
UNITY_SERVER_BUSY,errorCode, and retry details, which made a temporary busy state look like a severe Unity failure.Status: "Busy"and a clear message explaining that they can retry after the running command completes.Changes
server_busyRPC failures.Verification
go test ./internal/cli -run ServerBusygo test ./internal/cliscripts/check-go-cli-source.shscripts/check-go-cli.sh