Skip to content

fix: Busy responses now use the standard error envelope#1491

Merged
hatayama merged 2 commits into
v3-betafrom
fix/busy-error-envelope-schema
Jul 4, 2026
Merged

fix: Busy responses now use the standard error envelope#1491
hatayama merged 2 commits into
v3-betafrom
fix/busy-error-envelope-schema

Conversation

@hatayama

@hatayama hatayama commented Jul 4, 2026

Copy link
Copy Markdown
Owner

Summary

  • Server busy failures now use the same Success/Error JSON envelope as other CLI errors.
  • The old busy-only Status schema is removed from WriteErrorEnvelope output.

User Impact

  • JSON consumers can parse busy failures with the same error envelope shape as other failures.
  • Busy details such as runningToolName, requestedToolName, isPlaying, and isPaused remain available under Error.Details.Data.

Changes

  • Remove the busy-specific status envelope branch from WriteErrorEnvelope.
  • Update busy tests to assert the unified CLIErrorEnvelope output.
  • Delete old status-schema helpers that are no longer used.

Verification

  • cd cli/common && go test ./errors ./clicore
  • cd cli/dispatcher && go test ./internal/dispatcher
  • cd cli/project-runner && go test ./internal/projectrunner
  • cd cli/release-automation && go test ./internal/automation -run TestReleaseTriggerGuardCommonPackageWhitelistsMatchGoDependencies -count=1
  • scripts/check-go-cli.sh
  • cd cli/release-automation && go run ./cmd/check-release-triggers --base origin/v3-beta --head HEAD

Review in cubic

Return server_busy failures through the same Success/Error JSON shape as other CLI errors so consumers can parse one error schema consistently.
@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@hatayama, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 11 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 279124e0-b27e-499f-81d6-39fa45b0ee01

📥 Commits

Reviewing files that changed from the base of the PR and between 338a06c and 9b52405.

📒 Files selected for processing (1)
  • cli/common/errors/error_envelope_test.go
📝 Walkthrough

Walkthrough

This PR removes the specialized "busy status" error envelope path in the CLI error handling. The rpcBoolData helper is deleted, WriteErrorEnvelope no longer special-cases UNITY_SERVER_BUSY, and tests are updated to validate the unified CLIErrorEnvelope output. Shared-inputs hash stamps are also updated.

Changes

Unified error envelope for server busy errors

Layer / File(s) Summary
Remove busy-status envelope special-casing
cli/common/errors/busy_status.go, cli/common/errors/error_envelope.go
The rpcBoolData helper and its import are removed, and WriteErrorEnvelope no longer special-cases UNITY_SERVER_BUSY, always encoding the standard CLIErrorEnvelope JSON.
Update tests to validate unified envelope output
cli/common/errors/error_envelope_test.go
Tests are renamed and rewritten to unmarshal server_busy failures into CLIErrorEnvelope, checking Success, ErrorCode, Message, busy tool fields, and absence of the legacy Status field.
Refresh shared-inputs hash stamps
cli/dispatcher/shared-inputs-stamp.json, cli/project-runner/shared-inputs-stamp.json
sharedInputsHash values are updated to new hashes.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • hatayama/unity-cli-loop#1215: Both PRs modify the busy error-envelope path, updating expectations for running/requested tool names.
  • hatayama/unity-cli-loop#1227: Both PRs modify UNITY_SERVER_BUSY error-envelope special-casing, in opposite directions (removal vs. addition) with corresponding test changes.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: busy responses now use the standard error envelope.
Description check ✅ Passed The description matches the changeset and accurately describes the unified error envelope behavior and test updates.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/busy-error-envelope-schema

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
cli/common/errors/error_envelope_test.go (1)

328-330: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Misleading failure message.

The t.Fatalf message says "tool names mismatch" but this assertion checks envelope.Success, not tool names. Copy-paste artifact that will confuse debugging on failure.

🐛 Proposed fix
 	if envelope.Success {
-		t.Fatalf("tool names mismatch: %#v", envelope)
+		t.Fatalf("busy envelope reported success: %#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 `@cli/common/errors/error_envelope_test.go` around lines 328 - 330, The failure
message in the `TestErrorEnvelope` assertion is misleading because it checks
`envelope.Success` but reports a "tool names mismatch". Update the `t.Fatalf`
message in this test to describe the actual `envelope.Success` expectation,
using the surrounding `TestErrorEnvelope`/`envelope.Success` check so failures
clearly reflect the condition being validated.
🤖 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 `@cli/common/errors/error_envelope_test.go`:
- Around line 328-330: The failure message in the `TestErrorEnvelope` assertion
is misleading because it checks `envelope.Success` but reports a "tool names
mismatch". Update the `t.Fatalf` message in this test to describe the actual
`envelope.Success` expectation, using the surrounding
`TestErrorEnvelope`/`envelope.Success` check so failures clearly reflect the
condition being validated.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 78245d1c-3259-400a-b916-fbfef96fae58

📥 Commits

Reviewing files that changed from the base of the PR and between 0e821cc and 338a06c.

📒 Files selected for processing (5)
  • cli/common/errors/busy_status.go
  • cli/common/errors/error_envelope.go
  • cli/common/errors/error_envelope_test.go
  • cli/dispatcher/shared-inputs-stamp.json
  • cli/project-runner/shared-inputs-stamp.json
💤 Files with no reviewable changes (2)
  • cli/common/errors/error_envelope.go
  • cli/common/errors/busy_status.go

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 5 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread cli/common/errors/error_envelope_test.go
Align the failure message with the assertion so future test failures point at the actual unified-envelope check.
@hatayama
hatayama merged commit 0aa2e7e into v3-beta Jul 4, 2026
10 checks passed
@hatayama
hatayama deleted the fix/busy-error-envelope-schema branch July 4, 2026 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant