refactor: reorganize crate by responsibility into amqp/service/client modules by doubleailes · Pull Request #159 · doubleailes/girolle

doubleailes · 2026-04-30T05:45:18Z

Splits the previous flat layout (rpc_service / rpc_core / rpc_client /
events / queue / types / nameko_utils / payload) into three scoped
modules plus shared protocol pieces, so each file has one job:

amqp/ — all lapin transport: connection, channel/queue/exchange
declarations, Nameko header utilities, reply publisher
service/ — RpcService surface: register/subscribe/start, dispatch
loop, RpcContext, RpcCaller (handle + core), EventDispatcher
(handle + core), RpcTask, handler/event types, arg binding
client/ — standalone RpcClient + RpcReply/RpcResult
protocol.rs (private) — PayloadResult wire envelope and nameko.*
header keys, shared by amqp/service/client without coupling them
payload.rs — public Payload only (PayloadResult moved out)
error.rs — adds GirolleResult alias next to GirolleError
__macro_support — stable path the #[girolle] macro emits into

The previous types.rs was a junk drawer mixing type aliases, capability
handles, and handler signatures. Each capability handle now lives next
to its core (caller.rs, dispatcher.rs) instead of being split across
unrelated files.

No public API changes: the prelude and top-level re-exports are
preserved. The macro now emits ::girolle::__macro_support::* instead of
::girolle::nameko_utils::*; the bench is updated to the new path.

… modules Splits the previous flat layout (rpc_service / rpc_core / rpc_client / events / queue / types / nameko_utils / payload) into three scoped modules plus shared protocol pieces, so each file has one job: - amqp/ — all lapin transport: connection, channel/queue/exchange declarations, Nameko header utilities, reply publisher - service/ — RpcService surface: register/subscribe/start, dispatch loop, RpcContext, RpcCaller (handle + core), EventDispatcher (handle + core), RpcTask, handler/event types, arg binding - client/ — standalone RpcClient + RpcReply/RpcResult - protocol.rs (private) — PayloadResult wire envelope and nameko.* header keys, shared by amqp/service/client without coupling them - payload.rs — public Payload only (PayloadResult moved out) - error.rs — adds GirolleResult alias next to GirolleError - __macro_support — stable path the #[girolle] macro emits into The previous types.rs was a junk drawer mixing type aliases, capability handles, and handler signatures. Each capability handle now lives next to its core (caller.rs, dispatcher.rs) instead of being split across unrelated files. No public API changes: the prelude and top-level re-exports are preserved. The macro now emits ::girolle::__macro_support::* instead of ::girolle::nameko_utils::*; the bench is updated to the new path.

qodo-code-review · 2026-04-30T05:45:22Z

ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one.

Copilot

Pull request overview

Refactors girolle from a flat module layout into responsibility-scoped amqp/, service/, and client/ modules, while centralizing shared Nameko wire definitions and stabilizing the macro expansion path.

Changes:

Reorganize transport, service-runtime, and standalone client code into new module structure (amqp, service, client) with protocol shared internals.
Replace the previous types.rs “catch-all” with capability handles co-located with their cores and move handler/task types under service::task.
Introduce __macro_support and update #[girolle] macro + bench to reference it.

Reviewed changes

Copilot reviewed 27 out of 27 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
girolle_macro/src/entry.rs	Updates macro expansion to call `::girolle::__macro_support::build_inputs_fn_service`.
girolle/src/types.rs	Removed; previous type aliases/handles relocated into `service/*` and `error`.
girolle/src/service/task.rs	Adds `RpcTask`, handler type aliases, and boxed-future helper under `service`.
girolle/src/service/runtime.rs	Refactors service dispatch loop to use new `amqp/` and `service/` modules.
girolle/src/service/mod.rs	New `RpcService` surface API module and submodule map.
girolle/src/service/dispatcher.rs	Moves event dispatcher capability + core into `service`.
girolle/src/service/context.rs	Moves `RpcContext` into `service`.
girolle/src/service/caller.rs	Refactors in-service RPC caller core + handle, imports shared protocol constants.
girolle/src/service/args.rs	Moves handler argument binding logic (used by macro) into `service`.
girolle/src/rpc_task.rs	Removed; functionality moved to `service/task.rs`.
girolle/src/rpc_client.rs	Removed; functionality moved to `client/mod.rs` + `client/reply.rs`.
girolle/src/protocol.rs	New internal wire protocol module (`PayloadResult`, Nameko header keys).
girolle/src/prelude.rs	Updates prelude re-exports to the new module layout.
girolle/src/payload.rs	Narrows to public request payload only; reply envelope moved to `protocol`.
girolle/src/nameko_utils.rs	Removed; split into `amqp/*`, `service/args.rs`, and `service/runtime.rs`.
girolle/src/lib.rs	Re-exports updated to new modules; introduces `__macro_support`.
girolle/src/events.rs	Removed; functionality moved to `service/dispatcher.rs`.
girolle/src/error.rs	Adds public `GirolleResult` alias next to `GirolleError`.
girolle/src/client/reply.rs	New `RpcReply` / `RpcResult` types extracted from old client module.
girolle/src/client/mod.rs	New standalone `RpcClient` implementation under `client`.
girolle/src/amqp/publish.rs	New AMQP publish helper for RPC replies.
girolle/src/amqp/mod.rs	New private transport-layer module root.
girolle/src/amqp/headers.rs	New Nameko header manipulation helpers (call id stack, properties).
girolle/src/amqp/connection.rs	New AMQP connection helper.
girolle/src/amqp/channel.rs	Refactors channel/queue/exchange declaration helpers; removes connection logic.
girolle/src/__macro_support.rs	New stable macro-emitted symbol path (`::girolle::__macro_support::*`).
girolle/benches/macro.rs	Updates benchmark to use `__macro_support::build_inputs_fn_service`.

Comments suppressed due to low confidence (3)

girolle/src/service/runtime.rs:100

Initializing a global tracing subscriber inside a library entrypoint can panic if another subscriber was already set (common in binaries/tests). Prefer try_init() (and ignore the error) or leave subscriber initialization to the application using the crate.
girolle/src/service/runtime.rs:83
compute_deliver uses expect("Error publishing") on the result of publish(...). Since publish spawns a background task and currently unwraps internally, failures will either panic in the spawned task or be lost, while compute_deliver will keep running as if the reply was sent. Prefer making publish return a meaningful error (and avoid panics/unwraps), then handle the error here (log + decide whether to ack/nack).
girolle/src/service/runtime.rs:55
Typo in identifier: incomming_data should be spelled incoming_data (and similarly for other incomming_* variables) to avoid spreading misspellings across the codebase and public docs/logs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-30T06:03:32Z

+    let call_id_stack_slice = inner_headers
+        .get(NAMEKO_CALL_ID_STACK)
+        .unwrap()
+        .as_array()
+        .unwrap()
+        .as_slice();
+    let mut call_id_stack = call_id_stack_slice.to_vec();


insert_new_id_to_call_id uses unwrap() on the nameko.call_id_stack header and on its type conversions. If a caller sends headers without that key (or with an unexpected type), the service will panic. Consider treating a missing/invalid stack as an empty stack and seeding it instead of panicking.

Suggested change

let call_id_stack_slice = inner_headers

.get(NAMEKO_CALL_ID_STACK)

.unwrap()

.as_array()

.unwrap()

.as_slice();

let mut call_id_stack = call_id_stack_slice.to_vec();

let mut call_id_stack = inner_headers

.get(NAMEKO_CALL_ID_STACK)

.and_then(|value| value.as_array())

.map(|array| array.as_slice().to_vec())

.unwrap_or_default();

Copilot · 2026-04-30T06:03:32Z

+    let rpc_channel_clone = rpc_channel.clone();
+    let rpc_exchange_clone = rpc_exchange.to_string();
+    tokio::spawn(async move {
+        rpc_channel_clone
+            .basic_publish(
+                &rpc_exchange_clone,
+                &reply_to_id,
+                BasicPublishOptions::default(),
+                payload
+                    .to_string()
+                    .expect("can't serialize payload")
+                    .as_bytes(),
+                properties,
+            )
+            .await
+            .unwrap()
+            .await
+            .unwrap();


Using expect("can't serialize payload") inside this library helper will panic the process on serialization failures (e.g., if PayloadResult structure changes). Prefer returning the serialization error as part of the lapin::Result (or mapping it into GirolleError) so callers can handle it.

Suggested change

let rpc_channel_clone = rpc_channel.clone();

let rpc_exchange_clone = rpc_exchange.to_string();

tokio::spawn(async move {

rpc_channel_clone

.basic_publish(

&rpc_exchange_clone,

&reply_to_id,

BasicPublishOptions::default(),

payload

.to_string()

.expect("can't serialize payload")

.as_bytes(),

properties,

)

.await

.unwrap()

.await

.unwrap();

let payload = payload.to_string().map_err(|err| {

lapin::Error::from(std::io::Error::new(

std::io::ErrorKind::InvalidData,

format!("can't serialize payload: {err}"),

))

})?;

let rpc_channel_clone = rpc_channel.clone();

let rpc_exchange_clone = rpc_exchange.to_string();

tokio::spawn(async move {

if let Ok(confirm) = rpc_channel_clone

.basic_publish(

&rpc_exchange_clone,

&reply_to_id,

BasicPublishOptions::default(),

payload.as_bytes(),

properties,

)

.await

{

let _ = confirm.await;

}

Copilot · 2026-04-30T06:03:32Z

+        tokio::spawn(async move {
+            channel_clone
+                .basic_publish(
+                    &exchange_clone,
+                    &routing_key,
+                    BasicPublishOptions {
+                        mandatory: false,
+                        immediate: false,
+                    },
+                    payload.to_string().as_bytes(),
+                    properties,
+                )
+                .await
+                .expect("Failed to publish");
+        });


The spawned publish task uses .expect("Failed to publish"), which will panic in a background task and still let call_async return Ok(...). Prefer handling publish errors explicitly (e.g., log + surface the error back to the caller, or await publisher confirms).

Suggested change

tokio::spawn(async move {

channel_clone

.basic_publish(

&exchange_clone,

&routing_key,

BasicPublishOptions {

mandatory: false,

immediate: false,

},

payload.to_string().as_bytes(),

properties,

)

.await

.expect("Failed to publish");

});

channel_clone

.basic_publish(

&exchange_clone,

&routing_key,

BasicPublishOptions {

mandatory: false,

immediate: false,

},

payload.to_string().as_bytes(),

properties,

)

.await?;

Copilot · 2026-04-30T06:03:33Z

+        Some(id) => id.to_string(),
+        None => {
+            error!("{}: None", id_name);
+            panic!("{}: None", id_name)
+        }


get_id panics when an expected header is missing. In the consumer path this can bring down the whole service on a single malformed message. Prefer returning a Result<String, GirolleError> and letting callers decide how to handle missing fields (log + ack/nack).

Copilot · 2026-04-30T06:03:33Z

+            .await
+            .unwrap()
+            .await
+            .unwrap();


The spawned publish task uses unwrap() on both the initial publish and the broker confirm future. If the broker closes the channel or the confirm is negative, this will panic in a background task and the caller will still get Ok(()). Consider handling these errors explicitly (log + return error or signal failure) instead of panicking.

Copilot · 2026-04-30T06:03:33Z

+        tokio::spawn(async move {
+            channel_clone


call_async is a synchronous method but uses tokio::spawn, which will panic if called when no Tokio runtime is active (e.g., from a purely blocking context). If RpcClient::send is meant to be usable outside a runtime, consider making the publish path blocking (no spawn) or use Handle::try_current() and return a structured error when no runtime is available.

Copilot · 2026-04-30T06:03:33Z

+pub mod config;
+pub mod error;
+pub mod payload;
 pub mod prelude;
-mod events;
-mod rpc_client;
-pub use rpc_client::RpcClient;
-mod rpc_core;
-mod rpc_service;
-pub use rpc_service::RpcService;
-mod rpc_task;
-pub use rpc_task::RpcTask;
-mod payload;
+
+pub mod client;
+pub mod service;
+
+mod amqp;
+mod protocol;
+
+#[doc(hidden)]
+pub mod __macro_support;


The PR description says there are no public API changes, but pub mod types (and the entire types module) has been removed. Any downstream code using paths like girolle::types::RpcContext will break even though top-level/prelude re-exports remain. Consider reintroducing a pub mod types shim that re-exports the moved items (or update the PR description to acknowledge the breaking change).

Copilot · 2026-04-30T06:03:34Z

+        .map(|&arg| {
+            kwargs.get(arg).cloned().ok_or_else(|| {
+                GirolleError::IncorrectSignature("Key is missing in kwargs".to_string())
+            })


The IncorrectSignature error message on missing kwargs is too generic ("Key is missing in kwargs") and makes debugging hard. Include the missing argument name (and ideally the handler signature) in the error so callers can quickly identify what to provide.

doubleailes requested a review from Copilot April 30, 2026 05:57

Copilot started reviewing on behalf of doubleailes April 30, 2026 05:58 View session

Copilot AI reviewed Apr 30, 2026

View reviewed changes

chore: bump version to 2.1.0

7c612e3

doubleailes merged commit 65d94e5 into main Apr 30, 2026
5 checks passed

doubleailes deleted the claude/refactor-module-architecture-Q0z1P branch April 30, 2026 06:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: reorganize crate by responsibility into amqp/service/client modules#159

refactor: reorganize crate by responsibility into amqp/service/client modules#159
doubleailes merged 2 commits into
mainfrom
claude/refactor-module-architecture-Q0z1P

doubleailes commented Apr 30, 2026

Uh oh!

qodo-code-review Bot commented Apr 30, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 30, 2026

Uh oh!

Copilot AI Apr 30, 2026

Uh oh!

Copilot AI Apr 30, 2026

Uh oh!

Copilot AI Apr 30, 2026

Uh oh!

Copilot AI Apr 30, 2026

Uh oh!

Copilot AI Apr 30, 2026

Uh oh!

Copilot AI Apr 30, 2026

Uh oh!

Copilot AI Apr 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

doubleailes commented Apr 30, 2026

Uh oh!

qodo-code-review Bot commented Apr 30, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants