Skip to content

Conversation

@huerni
Copy link
Collaborator

@huerni huerni commented Oct 21, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Backend-provided error reasons are now surfaced in batch operation responses when available, improving error clarity.
  • New Features

    • Submit-batch responses include an explicit reason field to convey backend explanations.
    • Added a specific error code/message for Lua script validation failures ("Lua script validation failed").

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 21, 2025

Walkthrough

Added enum value ERR_LUA_FAILED, added a reason string to SubmitBatchTaskReply, mapped the new error code to a message, and updated SendRequest to return the backend-provided reason as a CraneError message when present for non-OK replies.

Changes

Cohort / File(s) Summary
Protobuf definitions
protos/PublicDefs.proto, protos/Crane.proto
Added ErrCode member ERR_LUA_FAILED = 85 and added string reason = 4; to SubmitBatchTaskReply.
Go error mapping
internal/util/err.go
Added mapping for protos.ErrCode_ERR_LUA_FAILED → "Lua script validation failed". Other error lookup behavior unchanged.
Request handling
internal/cbatch/cbatch.go
In SendRequest, for non-OK backend replies, if reply.GetReason() is non-empty return CraneError{Code: ErrorBackend, Message: reply.GetReason()}; otherwise preserve existing behavior of deriving message from reply code.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Verify protobuf field number and regenerate stubs.
  • Inspect SendRequest branching for unintended bypass of existing error handling or JSON paths.
  • Confirm new error-message mapping and search for assumptions about unknown error formatting.

Suggested reviewers

  • NamelessOIer
  • Nativu5

Poem

🐇 I found a reason on the trail,
A tiny string with quite a tale.
Lua tripped and left its sign,
Now errors speak a clearer line.
✨🐰

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title 'feat: lua' is extremely vague and does not clearly convey what changes were made. While it references Lua, it provides no meaningful context about the feature being added or the purpose of the changes. Revise the title to be more descriptive, such as 'feat: add Lua script validation error handling' or 'feat: implement Lua error code support in batch submission', to clearly communicate the main change to reviewers.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dev/jobsubmit_lua

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c1e465a and f146556.

📒 Files selected for processing (4)
  • internal/cbatch/cbatch.go (1 hunks)
  • internal/util/err.go (1 hunks)
  • protos/Crane.proto (1 hunks)
  • protos/PublicDefs.proto (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • internal/cbatch/cbatch.go
  • internal/util/err.go
  • protos/Crane.proto
  • protos/PublicDefs.proto

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 and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fe6476a and 4f1a929.

📒 Files selected for processing (4)
  • internal/cbatch/cbatch.go (1 hunks)
  • internal/util/err.go (1 hunks)
  • protos/Crane.proto (1 hunks)
  • protos/PublicDefs.proto (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/cbatch/cbatch.go
🧰 Additional context used
🪛 GitHub Actions: Go Code Quality Check
internal/util/err.go

[error] 1-1: The following files are not properly formatted: internal/util/err.go. Please run 'gofmt -w .' to fix the formatting.

🔇 Additional comments (3)
internal/util/err.go (1)

199-199: LGTM!

The error mapping for ERR_LUA_FAILED is correctly added with a clear, user-friendly message that follows the existing conventions in the error map.

protos/PublicDefs.proto (1)

682-682: LGTM!

The new error code ERR_LUA_FAILED = 78 is correctly added to the ErrCode enum with proper numbering and placement in the sequence.

protos/Crane.proto (1)

89-89: LGTM!

The reason field is correctly added outside the oneof payload block, allowing it to be populated regardless of whether the reply indicates success (with task_id) or failure (with code). This design enables the backend to provide detailed error messages, particularly useful for Lua validation failures.

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.

2 participants