Skip to content

refactor(grpc): reduce proto-change friction in the Go bindings - #1800

Merged
slin1237 merged 2 commits into
mainfrom
chore/grpc-bindings-proto-friction
Jun 22, 2026
Merged

slin1237 merged 2 commits into
mainfrom
chore/grpc-bindings-proto-friction

Conversation

@slin1237

@slin1237 slin1237 commented Jun 22, 2026 •

Copy link
Copy Markdown
Member

Summary

Two small, targeted fixes that reduce the per-proto-change cost in the gRPC bindings — the recurring friction discussed around the #1747 reasoning-token change. No behavior change.

Changes

  • Harden Go proto reconstruction. bindings/golang/src/proto_parse.rs listed every field of GenerateStreamChunk/GenerateComplete explicitly, so adding any field to the generate protos broke the Go binding's compile (this is why feat(grpc): add sglang reasoning token usage #1747 had to touch it). Use ..Default::default() for the fields the bindings don't surface, so new proto fields no longer require a change here.

  • De-duplicate reasoning detection. The Go bindings reimplemented the gateway's thinking-toggle logic (should_mark_reasoning_started + extract_thinking_from_kwargs), which could drift. Expose the gateway helpers as pub and have the bindings call them — consistent with how the bindings already reuse smg::routers::grpc::utils::process_chat_messages. The helpers stay in the gateway (request-interpretation lives there); the tokenizer crate is untouched.

Verification

  • cargo +nightly fmt --all clean
  • cargo check --workspace --all-targets clean
  • cargo clippy -p smg -p smg-golang --all-targets -- -D warnings clean
  • cargo test -p smg --lib — 1092 passed, 0 failed

Summary by CodeRabbit

  • Refactor
    • Centralized reasoning token detection logic for improved code reuse across the gateway and Go language bindings.
    • Simplified proto field initialization to rely on default values, reducing code maintenance overhead.

slin1237 added 2 commits June 21, 2026 23:07
…ction

parse_chunk/parse_complete listed every proto field explicitly, so adding
any field to the generate protos broke the Go binding's compile. Use
..Default::default() for the fields the bindings do not surface, so new
proto fields no longer require a change here.

Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
The Go bindings reimplemented the gateway's thinking-toggle detection
(should_mark_reasoning_started + extract_thinking_from_kwargs), which can
drift from the gateway. Expose the gateway helpers as pub and have the
bindings call them, matching how they already reuse process_chat_messages.

Signed-off-by: Simo Lin <25425177+slin1237@users.noreply.github.com>
@github-actions github-actions Bot added grpc gRPC client and router changes model-gateway Model gateway crate changes labels Jun 22, 2026
@coderabbitai

coderabbitai Bot commented Jun 22, 2026 •

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: d5df9ab6-d04c-414e-b80f-f97e6d5e8ae7

📥 Commits

Reviewing files that changed from the base of the PR and between 7cb64bf and e21a443.

📒 Files selected for processing (4)
  • bindings/golang/src/proto_parse.rs
  • bindings/golang/src/utils.rs
  • model_gateway/src/routers/grpc/utils/mod.rs
  • model_gateway/src/routers/grpc/utils/parsers.rs

📝 Walkthrough

Walkthrough

Two gateway functions (should_mark_reasoning_started, extract_thinking_from_kwargs) are promoted from pub(crate) to pub and re-exported publicly. The Go bindings then import and call these shared helpers in chat_requires_reasoning, replacing inline logic. Proto struct builders in parse_chunk and parse_complete are updated to use ..Default::default().

Reasoning detection and proto initialization

Layer / File(s) Summary
Promote reasoning helpers to public API
model_gateway/src/routers/grpc/utils/parsers.rs, model_gateway/src/routers/grpc/utils/mod.rs
should_mark_reasoning_started and extract_thinking_from_kwargs are changed from pub(crate) to pub in parsers.rs, and mod.rs splits their re-exports into a separate pub use block with a comment noting they are intended for Go bindings.
Go bindings delegate to shared helpers and use ..Default::default()
bindings/golang/src/utils.rs, bindings/golang/src/proto_parse.rs
chat_requires_reasoning removes its inline extraction and branching and instead calls should_mark_reasoning_started(extract_thinking_from_kwargs(...), tokenizer). Proto builders in parse_chunk and parse_complete replace explicit zero/None field initializations with ..Default::default() to avoid breakage when proto structs gain new fields.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • lightseekorg/smg#1031: Introduced the same extract_thinking_from_kwargs/should_mark_reasoning_started helpers for reasoning parser state overriding, which this PR now exposes publicly.
  • lightseekorg/smg#1747: Added the require_reasoning and reasoning_tokens proto fields across the gRPC stack that the Go bindings proto-parse changes here directly relate to.

Suggested labels

grpc, model-gateway

Suggested reviewers

  • key4ng

Poem

🐇 Hop, hop! The helpers now shine in the light,
No more inline logic tucked out of sight.
pub is the word, shared across the crate,
Default::default() fills fields — isn't that great?
The bindings and gateway now speak the same tongue,
A tidy refactor, a new song sung! 🎵

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title directly and accurately describes the main refactoring goal: reducing friction from proto changes in Go bindings through better use of defaults and shared utilities.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/grpc-bindings-proto-friction

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

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request refactors the Go bindings and model gateway to reuse reasoning detection logic instead of duplicating it. Specifically, it exposes extract_thinking_from_kwargs and should_mark_reasoning_started as public functions from the gateway and imports them in the Go bindings. Additionally, it updates protobuf parsing in the Go bindings to use ..Default::default() to avoid future breakage when new fields are added. There are no review comments to address, and I have no additional feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@claude

claude Bot commented Jun 22, 2026

Copy link
Copy Markdown

👋 The PR description doesn't fully follow
PULL_REQUEST_TEMPLATE.md:

  • Missing header: ## Description
  • Missing header: ### Problem
  • Missing header: ### Solution
  • Missing header: ## Test Plan

Please update the PR description so reviewers have the context they need.

@claude claude 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.

Clean refactoring — no issues found. The ..Default::default() change is idiomatic and eliminates the proto-field maintenance burden. The visibility promotion to pub follows the existing pattern used by process_chat_messages and create_stop_decoder, and the de-duplicated reasoning detection keeps the two code paths in sync.

@slin1237
slin1237 merged commit b2dddad into main Jun 22, 2026
39 of 59 checks passed
@slin1237
slin1237 deleted the chore/grpc-bindings-proto-friction branch June 22, 2026 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

grpc gRPC client and router changes model-gateway Model gateway crate changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant