feat(modkit-contract): add PoC contract stack for REST and gRPC

[MikeFalcon77]

Introduce the first PoC of the ModKit contract system.

• add modkit-contract, modkit-contract-macros, and modkit-contract-protogen
• add contract descriptors, IR validation, OpenAPI generation, and runtime support
• add REST and gRPC contract/codegen support with proto lockfile handling
• extend canonical error wire-compatibility support
• add macro UI tests and contract integration tests
• add examples/modkit/api-contracts sample module and SDK

Summary by CodeRabbit

• New Features

  • Added contract and API binding framework supporting REST and gRPC transports with automatic client/server code generation
  • Introduced payment service example demonstrating contract framework usage
  • Added REST endpoint discovery to the directory service for dynamic service location

• Documentation

  • Added architecture decision records documenting contract binding design, server-side route generation, and consumer wiring patterns
  • Published typed-error wire format compatibility guide

• Improvements

  • Enhanced error handling with typed error categories and RFC 9457 problem format support
  • Improved protocol buffer generation with stable field-number assignment via lockfile mechanism

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 96712811-84d7-4e91-b271-7b0b71d2f653

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

📝 Walkthrough

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 3

Note

Due to the large number of review comments, Critical severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

gears/system/gear-orchestrator/src/server.rs (1)

89-93: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Inconsistent error handling in list_instances.

list_instances uses Status::internal(e.to_string()) which leaks the raw error message to clients, while all other methods use the new map_directory_error helper that sanitizes errors to avoid exposing internal details (DB connection strings, file paths, etc.).

Proposed fix

         let instances = self
             .api
             .list_instances(&gear_name)
             .await
-            .map_err(|e| Status::internal(e.to_string()))?;
+            .map_err(|e| map_directory_error(&e))?;

🤖 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 `@gears/system/gear-orchestrator/src/server.rs` around lines 89 - 93, The
list_instances call currently maps errors with Status::internal(e.to_string())
and may leak internal details; change the error mapping to use the existing
map_directory_error helper so errors are sanitized consistently—update the
.map_err on self.api.list_instances(&gear_name).await to call
map_directory_error(e) (or the appropriate signature) so list_instances uses the
same sanitized error path as other methods.

libs/toolkit-canonical-errors/src/problem.rs (1)

599-643: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Expose error_code and error_domain in the OpenAPI schema.

Problem now serializes these two discriminator fields, but the utoipa::PartialSchema implementation still omits them. That leaves generated OpenAPI docs/clients out of sync with the actual wire payload for typed contract errors.

Suggested schema patch

         ObjectBuilder::new()
             .property(
                 "type",
                 ObjectBuilder::new().schema_type(SchemaType::Type(Type::String)),
             )
@@
             .property(
                 "trace_id",
                 ObjectBuilder::new().schema_type(SchemaType::Type(Type::String)),
             )
+            .property(
+                "error_code",
+                ObjectBuilder::new().schema_type(SchemaType::Type(Type::String)),
+            )
+            .property(
+                "error_domain",
+                ObjectBuilder::new().schema_type(SchemaType::Type(Type::String)),
+            )
             .property(
                 "context",
                 ObjectBuilder::new().schema_type(SchemaType::Type(Type::Object)),
             )
             .required("context")
             .description(Some(
-                "RFC 9457 problem+json. `context` varies by error category.",
+                "RFC 9457 problem+json. `context` varies by error category; typed contract errors may also include `error_code` and `error_domain`.",
             ))
             .into()

🤖 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 `@libs/toolkit-canonical-errors/src/problem.rs` around lines 599 - 643, The
PartialSchema for Problem is missing the serialized discriminator fields
error_code and error_domain; update the impl utoipa::PartialSchema for Problem
(the schema builder chain) to add .property("error_code",
ObjectBuilder::new().schema_type(SchemaType::Type(Type::String))).required("error_code")
and .property("error_domain",
ObjectBuilder::new().schema_type(SchemaType::Type(Type::String))).required("error_domain")
(or mark as optional if your serde exposes them as optional) so the generated
OpenAPI matches the actual Problem wire payload produced by the Problem type.

🟠 Major comments (29)

examples/toolkit/api-contracts/api-contracts/src/module.rs-63-70 (1)

63-70: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Bind the REST server to a local implementation, not the hub-selected client.

Line 69 resolves PaymentApi through ClientHub, but this module’s own docs say that wiring is transport-selected. If config picks rest, these server handlers can end up calling the REST client instead of the local service, which turns the endpoint into a proxy and can recurse back into itself. register_rest should attach a local implementation for serving requests, independent of consumer-side transport selection.

🤖 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 `@examples/toolkit/api-contracts/api-contracts/src/module.rs` around lines 63 -
70, register_rest erroneously binds the server handlers to the client selected
via ctx.client_hub().get::<dyn PaymentApi>(), which can make the server proxy to
its own REST client; instead obtain or instantiate the module's local server
implementation and pass that into routes::register_routes. Replace the call to
ctx.client_hub().get::<dyn PaymentApi>() with the local service (e.g.,
instantiate LocalPaymentApi / PaymentApiImpl or retrieve via a local-service
accessor like ctx.local_service::<dyn PaymentApi>()), then pass that
concrete/local impl to routes::register_routes(router, openapi, local_impl)
ensuring types/lifetimes match so the server endpoints call the local
implementation rather than the hub-selected client.

examples/toolkit/api-contracts/api-contracts/src/api/grpc.rs-140-145 (1)

140-145: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject missing credentials here to match the REST transport contract.

Line 144 turns a missing authorization header into SecurityContext::anonymous(), but the REST surface registers the same operations as authenticated in examples/toolkit/api-contracts/api-contracts/src/api/rest/routes.rs. That makes gRPC the weaker path and can permit unauthenticated calls unless every domain entrypoint re-checks auth itself. Please reject missing credentials here the same way malformed credentials are rejected, or make anonymous access an explicit per-method policy instead of the default.

🤖 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 `@examples/toolkit/api-contracts/api-contracts/src/api/grpc.rs` around lines
140 - 145, In require_security_context, do not map AuthOutcome::Anonymous to
SecurityContext::anonymous(); instead treat missing credentials as an
authentication failure like malformed credentials: change the
AuthOutcome::Anonymous match arm to return an Err(Status::unauthenticated(...))
(or equivalent gRPC unauthenticated Status) with a clear message so gRPC rejects
missing Authorization headers to match the REST contract; keep
AuthOutcome::Authenticated(ctx) returning Ok(ctx) and ensure classify_auth is
still used to distinguish malformed vs missing credentials if you want different
error messages.

examples/toolkit/api-contracts/api-contracts/src/client/local.rs-86-94 (1)

86-94: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Don't bypass the policy stack for list_payments.

This method returns the raw domain stream without invoking self.policies, so streaming calls skip the same tracing/metrics/policy hooks that unary methods go through. That makes PaymentLocalClient observably inconsistent across methods.

🤖 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 `@examples/toolkit/api-contracts/api-contracts/src/client/local.rs` around
lines 86 - 94, list_payments in PaymentLocalClient currently returns the raw
stream from self.service.list_payments, bypassing self.policies and therefore
skipping tracing/metrics/policy hooks; change it to construct the stream via
self.service.list_payments(&ctx, &filter) and then pass that stream through the
policy stack (e.g., call the policies wrapper used by other methods such as
self.policies.apply_stream or the equivalent method your PolicyStack exposes) so
the returned PaymentStream<PaymentSummary> has the same per-call hooks applied;
ensure you use the same SecurityContext and return the wrapped stream instead of
the raw one.

libs/toolkit-contract-protogen/src/lib.rs-1166-1176 (1)

1166-1176: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Detect oneof field-name collisions after snake_case normalization.

Two externally-tagged variants whose discriminator keys normalize to the same proto field name will both be emitted into the same oneof with the same identifier. That produces invalid .proto and can also alias lockfile assignments across distinct variants.

🤖 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 `@libs/toolkit-contract-protogen/src/lib.rs` around lines 1166 - 1176, The code
currently converts discriminator keys to snake_case (snake_variant_name) and
blindly assigns a field number, which lets two distinct keys that normalize to
the same snake name collide; update the logic before calling
lock.messages.entry(...).assign(...) to detect duplicates by checking whether
the target schema's assigned names already contain snake_variant_name (you can
inspect the existing OneofVariant entries in variants for the same schema or add
a lookup on lock.messages for assigned names), and if a duplicate is found, fail
fast with a clear error (or return a Result::Err) identifying the original key
and the colliding key; keep the symbols to change: snake_variant_name, key,
field_number, lock.messages, schema_name, variants, and OneofVariant.

libs/toolkit-contract-protogen/src/lib.rs-895-917 (1)

895-917: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject heterogeneous anyOf instead of treating it as nullable $ref.

This branch accepts any anyOf that contains one $ref, even if there are additional non-null alternatives. For example, [$ref, null, string] currently collapses to optional <ref>, which drops part of the schema instead of raising ProtoGenFeature::AnyOf.

🤖 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 `@libs/toolkit-contract-protogen/src/lib.rs` around lines 895 - 917, The anyOf
handling in lib.rs currently treats any anyOf with a $ref and a null as an
optional ref, which silently drops other alternatives; modify the branch that
processes schema.get("anyOf") so it only collapses to an optional $ref when
every non-null entry is a $ref and there is exactly one distinct $ref target;
otherwise return Err(ProtoGenFeature::AnyOf). Concretely, in the loop that
inspects entries, track the count of non-null non-$ref alternatives (and
multiple distinct $ref targets) and if you encounter any non-$ref non-null type
or more than one distinct $ref, short-circuit to Err(ProtoGenFeature::AnyOf)
instead of mapping to ProtoFieldShape; keep the existing use of ref_to_name,
queue.push(target), and the returned ProtoFieldShape when the strict
single-$ref-or-null condition is satisfied.

libs/toolkit-contract-protogen/src/lockfile.rs-74-79 (1)

74-79: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject unsupported lockfile versions on load.

version is documented as the incompatible-format guard, but load() never checks it. A future lockfile with a different shape will deserialize as far as it can and then be used to assign wire numbers, which can silently corrupt reservations instead of failing fast.

Suggested fix

 pub fn load(path: &Path) -> Result<Self, LockfileError> {
     match std::fs::read_to_string(path) {
-        Ok(text) => toml::from_str(&text).map_err(LockfileError::Parse),
+        Ok(text) => {
+            let lock: Self = toml::from_str(&text).map_err(LockfileError::Parse)?;
+            if lock.version != LOCKFILE_VERSION {
+                return Err(LockfileError::UnsupportedVersion {
+                    found: lock.version,
+                    expected: LOCKFILE_VERSION,
+                });
+            }
+            Ok(lock)
+        }
         Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(Self::empty()),
         Err(e) => Err(LockfileError::Io(e)),
     }
 }

🤖 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 `@libs/toolkit-contract-protogen/src/lockfile.rs` around lines 74 - 79, The
load function currently deserializes without validating the lockfile version;
update load to parse into Self (as now), then check the deserialized object's
version field against the expected/current version constant and return an
Err(LockfileError::...) for mismatches instead of proceeding (preserve the
existing NotFound handling that returns Self::empty()); if LockfileError lacks
an appropriate variant add one (e.g., UnsupportedVersion) to surface the
rejection.

libs/toolkit-contract-protogen/src/lib.rs-705-715 (1)

705-715: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Persist the synthetic entries field through the lockfile.

The top-level additionalProperties wrapper hard-codes entries = 1 instead of allocating through lock.messages[schema_name]. If that schema later evolves from a map wrapper into a normal object, field 1 is immediately reusable because entries was never recorded or tombstoned, which breaks the wire-compat guarantee this crate is introducing.

Suggested fix

+            let field_number = lock
+                .messages
+                .entry(schema_name.to_owned())
+                .or_default()
+                .assign("entries");
             messages.insert(
                 schema_name.to_owned(),
                 MessageDef {
                     fields: vec![MessageField {
                         name: "entries".to_owned(),
                         proto_type: format!("map<string, {}>", value_shape.proto_type),
                         repeated: false,
                         optional: false,
                         is_map: true,
-                        field_number: 1,
+                        field_number,
                     }],
                     oneof: None,
                     ..MessageDef::default()
                 },
             );

🤖 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 `@libs/toolkit-contract-protogen/src/lib.rs` around lines 705 - 715, The
synthetic top-level map wrapper is being created with a hard-coded field_number:
1 for the MessageField "entries" (in the messages.insert branch that constructs
MessageDef/MessageField), which must instead be allocated and persisted via the
lock (lock.messages[schema_name]) so the field is tombstoned/recorded; change
the code that builds MessageDef for schema_name to consult or allocate the next
available field id in lock.messages[schema_name] (or record the newly assigned
id into lock.messages[schema_name]) and use that value for
MessageField.field_number rather than the constant 1, ensuring subsequent schema
changes reuse or respect the recorded/tombstoned id.

examples/toolkit/api-contracts/api-contracts-sdk/src/models.rs-72-91 (1)

72-91: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add a DTO variant for the proto zero enum value.

The committed .proto includes PAYMENT_STATUS_UNSPECIFIED = 0, but this SDK enum cannot represent it. That makes the gRPC surface lossy for ChargeResponse, Invoice, and PaymentSummary: an omitted/default enum value from a peer decodes as 0 on the wire, yet there is no corresponding Rust variant to bridge to. Please add an Unspecified/Unknown variant here, or make the wire fields optional before shipping the gRPC contract.

🤖 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 `@examples/toolkit/api-contracts/api-contracts-sdk/src/models.rs` around lines
72 - 91, The enum PaymentStatus cannot represent the proto zero value
(PAYMENT_STATUS_UNSPECIFIED = 0); add an explicit Unspecified (or Unknown)
variant to PaymentStatus (e.g., Unspecified) and make it the #[default] so zero
maps correctly, keeping serde(rename_all = "snake_case") and proto bridge
attributes intact; update any references in ChargeResponse, Invoice, and
PaymentSummary to accept the new variant (no field type changes needed) so gRPC
enum values decode losslessly.

examples/toolkit/api-contracts/api-contracts-sdk/examples/gen_grpc_proto.rs-8-8 (1)

8-8: ⚠️ Potential issue | 🟠 Major

Feature-gate gen_grpc_proto docs/imports with grpc-client.

gen_grpc_proto.rs imports cf_api_contracts_sdk::grpc::payment_api_grpc_binding unconditionally, but cf-api-contracts-sdk only exposes pub mod grpc / payment_api_grpc_binding under #[cfg(feature = "grpc-client")], while examples/toolkit/api-contracts/api-contracts-sdk/Cargo.toml sets default = [] and contains no [[example]]/required-features for gen_grpc_proto. Result: the documented command (cargo run --example gen_grpc_proto -p cf-api-contracts-sdk) won’t compile unless grpc-client is enabled.

Update Line 8’s run command to include --features grpc-client (or declare required-features = ["grpc-client"] for the gen_grpc_proto example, or enable grpc-client by default).

🤖 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 `@examples/toolkit/api-contracts/api-contracts-sdk/examples/gen_grpc_proto.rs`
at line 8, The example `gen_grpc_proto.rs` imports
`cf_api_contracts_sdk::grpc::payment_api_grpc_binding` which is only compiled
under the `grpc-client` feature; update the example to be feature-gated by
either (A) change the run instruction on the doc comment (the `cargo run
--example gen_grpc_proto -p cf-api-contracts-sdk`) to include `--features
grpc-client`, or (B) declare `required-features = ["grpc-client"]` for the
`gen_grpc_proto` example in the
examples/toolkit/api-contracts/api-contracts-sdk/Cargo.toml, or (C) enable the
`grpc-client` feature by default—pick one approach and make the corresponding
change so the import of `cf_api_contracts_sdk::grpc::payment_api_grpc_binding`
is only used when `grpc-client` is enabled.

libs/toolkit-contract/src/openapi/generator.rs-117-131 (1)

117-131: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Derive OpenAPI required from the field metadata instead of hardcoding it.

The generator currently marks every query/header parameter optional, and it marks the whole requestBody optional unless all body fields are required. That understates the contract for common cases like a required query/header field or a body with one required and one optional property. The requiredness here should follow field_is_optional(...) for each binding and treat the body as required whenever at least one bound body field is mandatory.

Also applies to: 238-246

🤖 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 `@libs/toolkit-contract/src/openapi/generator.rs` around lines 117 - 131, The
code currently hardcodes required = false for HttpFieldBinding::Query and
::Header; change both places to compute required using the existing
field_is_optional(method, field) helper (i.e., required =
!field_is_optional(method, field)) when calling parameter_object (symbols:
HttpFieldBinding::Query, HttpFieldBinding::Header, parameter_object,
field_is_optional, field_schema). Also update the requestBody generation (the
block around the requestBody creation referenced at lines ~238-246) to mark the
whole requestBody as required whenever at least one bound body field is
mandatory by scanning body bindings and setting request_body_required =
body_fields.iter().any(|f| !field_is_optional(method, f)); pass that boolean
into the requestBody construction instead of always making it optional.

libs/toolkit-contract/src/runtime/canonical.rs-92-93 (1)

92-93: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid byte-slicing the response body here.

&body[..200] will panic whenever the 200th byte lands in the middle of a UTF-8 code point. Since this path processes arbitrary remote error text, a long non-ASCII body can crash canonicalization instead of returning a fallback error. Truncate on character boundaries instead.

🤖 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 `@libs/toolkit-contract/src/runtime/canonical.rs` around lines 92 - 93, The
code in http_status_to_canonical currently slices the UTF-8 string with
&body[..200], which can panic on multibyte characters; change the truncation to
operate on character boundaries instead (e.g., build a truncated string using
chars().take(200) or find the nth char byte index via char_indices) and use that
safe substring as preview so arbitrary non-ASCII response bodies cannot cause a
panic in http_status_to_canonical.

libs/toolkit-transport-grpc/src/lib.rs-25-31 (1)

25-31: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use an encoded-size budget here, not an 8 KiB raw JSON budget.

The truncation guard currently allows up to 8192 raw bytes, but the comment just above correctly notes that -bin metadata is base64-expanded on the wire. That means a trailer can pass this check and still exceed the effective metadata budget, so the "truncate if too large" path won't trigger when it should. Either reduce the raw cap to the pre-base64 size you actually want, or enforce against the encoded size instead.

Also applies to: 155-157

🤖 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 `@libs/toolkit-transport-grpc/src/lib.rs` around lines 25 - 31, The constant
MAX_PROBLEM_TRAILER_BYTES currently represents a raw JSON byte cap but the
trailer is base64-encoded on the wire, so update the implementation to enforce
the post-base64 (encoded) size budget instead of the raw size: either change the
truncation/validation logic wherever MAX_PROBLEM_TRAILER_BYTES is used to
compute the base64-encoded length (e.g., base64 length <=
MAX_PROBLEM_TRAILER_BYTES) and keep the constant meaning as “encoded bytes”, or
reduce the raw-byte constant to the pre-encoding budget (8192 * 3/4 ≈ 6144) so
raw JSON + base64 expansion fits the 8 KiB wire limit; apply the same change to
the other identical trailer-cap constant referenced later in the file.

libs/toolkit-contract/src/policy.rs-119-123 (1)

119-123: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Continue unwinding on_response hooks after the first failure.

Lines 119-123 return immediately on the first on_response error, so earlier policies never see completion even though their on_request succeeded. That breaks the stack symmetry you already preserve on the request-error path and can skip per-call cleanup/bookkeeping in outer policies.

Suggested fix

-        for policy in self.policies.iter().rev() {
-            if let Err(e) = policy.on_response(ctx, success).await {
-                return Err(map_policy_err(e));
-            }
-        }
-
-        result
+        let mut response_err = None;
+        for policy in self.policies.iter().rev() {
+            if let Err(e) = policy.on_response(ctx, success).await {
+                response_err.get_or_insert(e);
+            }
+        }
+
+        if let Some(e) = response_err {
+            Err(map_policy_err(e))
+        } else {
+            result
+        }

🤖 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 `@libs/toolkit-contract/src/policy.rs` around lines 119 - 123, The on_response
loop currently returns on the first error, preventing later policies from
running their completion hooks; change the logic in the reverse iteration over
self.policies in on_response to call every policy.on_response(ctx,
success).await, record the first Err(e) you encounter (but do not return
immediately), continue invoking the remaining policies, and after the loop
return Err(map_policy_err(first_error)) if any error was recorded, otherwise
return Ok(...). Keep references to the existing symbols self.policies,
on_response, and map_policy_err so the change is localized to that loop.

libs/toolkit-contract/src/ir/validation.rs-195-223 (1)

195-223: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject Path bindings whose {param} is missing from the template.

Lines 213-221 only validate template → binding coverage. They never validate binding → template coverage, so a binding like Path { param: "id", ... } on "/things" passes validation even though no generated client/server path can materialize it.

Suggested fix

 fn validate_path_params(
     method_binding: &HttpMethodBindingIr,
     method_loc: &str,
     errors: &mut Vec<ValidationError>,
 ) {
     let template_params = extract_path_params(&method_binding.path_template);
+    let template_param_set: HashSet<&str> =
+        template_params.iter().map(String::as_str).collect();
     let path_binding_params: HashSet<&str> = method_binding
         .field_bindings
         .iter()
         .filter_map(|fb| {
             if let HttpFieldBinding::Path { param, .. } = fb {
@@
     for param in &template_params {
         if !path_binding_params.contains(param.as_str()) {
             errors.push(ValidationError {
                 location: method_loc.to_owned(),
                 message: format!(
                     "path template parameter '{{{param}}}' has no corresponding Path field binding"
                 ),
             });
         }
     }
+
+    for &param in &path_binding_params {
+        if !template_param_set.contains(param) {
+            errors.push(ValidationError {
+                location: method_loc.to_owned(),
+                message: format!(
+                    "Path field binding references '{{{param}}}', but the path template does not declare that parameter"
+                ),
+            });
+        }
+    }
 }

🤖 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 `@libs/toolkit-contract/src/ir/validation.rs` around lines 195 - 223,
validate_path_params currently only checks that each template parameter has a
Path binding; add the inverse check to reject Path bindings whose param is not
present in the template: after building template_params (from
extract_path_params(&method_binding.path_template)), iterate
method_binding.field_bindings, filter for HttpFieldBinding::Path variants (same
pattern used earlier), and for each Path { param, .. } verify template_params
contains param.as_str(); if not, push a ValidationError (using the same
method_loc and a clear message like "Path field binding param '{param}' is not
present in path template"). This uses the existing symbols validate_path_params,
method_binding.field_bindings, HttpFieldBinding::Path, and extract_path_params.

libs/toolkit-contract/src/ir/validation.rs-239-260 (1)

239-260: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve FieldRole when validating HTTP-exposed fields.

Line 239 collapses the method input to HashSet<&str>, so SecurityContext fields are treated the same as normal wire fields. That lets Query/Header/Path bindings expose server-injected context across HTTP, even though FieldRole explicitly says those fields must not cross the transport boundary.

Suggested fix

-use std::collections::HashSet;
+use std::collections::{HashMap, HashSet};
@@
-    let input_field_names: HashSet<&str> = contract_method
+    let input_fields: HashMap<&str, crate::ir::contract::FieldRole> = contract_method
         .input
         .fields
         .iter()
-        .map(|f| f.name.as_str())
+        .map(|f| (f.name.as_str(), f.role))
         .collect();
@@
-        if !input_field_names.contains(field.as_str()) {
-            errors.push(ValidationError {
-                location: method_loc.to_owned(),
-                message: format!(
-                    "{kind} binding references field '{field}' not found in contract method input"
-                ),
-            });
+        match input_fields.get(field.as_str()).copied() {
+            None => {
+                errors.push(ValidationError {
+                    location: method_loc.to_owned(),
+                    message: format!(
+                        "{kind} binding references field '{field}' not found in contract method input"
+                    ),
+                });
+            }
+            Some(crate::ir::contract::FieldRole::SecurityContext) => {
+                errors.push(ValidationError {
+                    location: method_loc.to_owned(),
+                    message: format!(
+                        "{kind} binding must not reference security-context field '{field}'"
+                    ),
+                });
+            }
+            Some(crate::ir::contract::FieldRole::Wire) => {}
         }
     }
 }

🤖 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 `@libs/toolkit-contract/src/ir/validation.rs` around lines 239 - 260, The
current validation collapses contract_method.input.fields into a HashSet<&str>
(input_field_names) losing each field's FieldRole, which allows server-only
fields (e.g., SecurityContext) to be bound to
HttpFieldBinding::Path/Query/Header; update the logic to preserve each field's
role (e.g., build a map from field.name -> field.role or lookup the field by
name on contract_method.input.fields) and then: 1) if a binding references a
non-existent field, push the existing ValidationError, and 2) if the field
exists but its FieldRole disallows transport exposure (e.g., SecurityContext),
push a ValidationError explaining that the specific HttpFieldBinding
(Path/Query/Header) may not bind server-injected fields; adjust references to
input_field_names, contract_method, method_binding, and HttpFieldBinding
accordingly.

libs/toolkit-contract/src/runtime/http.rs-179-196 (1)

179-196: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Structured query flattening drops the declared binding name.

Once recursion enters an object, it stops using the bound param and emits each child key directly. For example, Query { field: "filter", param: "q" } plus { "status": "paid" } becomes status=paid, not something rooted at q. That changes the on-the-wire contract and can collide with sibling query bindings.

🤖 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 `@libs/toolkit-contract/src/runtime/http.rs` around lines 179 - 196, The
flatten_query_value function currently drops the parent binding when recursing
into serde_json::Value::Object by using the child key directly; update the
Object branch to preserve the parent param by composing a nested key (for
example use format!("{}[{}]", param, k) or another agreed separator) and pass
that composed key into flatten_query_value so emitted pairs remain rooted at the
original binding (i.e., change flatten_query_value(k, v, out) to
flatten_query_value(&format!("{}[{}]", param, k), v, out) and keep the existing
v.is_null() check).

libs/toolkit-contract/src/http/dispatch.rs-67-69 (1)

67-69: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Keep query-binding semantics identical to the runtime URL builder.

HttpFieldBinding::Query goes through field_as_string here, so objects/arrays are rejected, but libs/toolkit-contract/src/runtime/http.rs accepts structured query values and even tests object flattening. The same HttpMethodBindingIr can therefore succeed in the generated runtime client and fail in dispatch/OpenAPI paths. Please centralize this logic or enforce one query-shape policy across both helpers.

Also applies to: 105-121

🤖 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 `@libs/toolkit-contract/src/http/dispatch.rs` around lines 67 - 69, The
dispatch code for HttpFieldBinding::Query currently calls field_as_string
(rejecting arrays/objects) which diverges from the runtime URL builder's
handling (which accepts structured values and flattens objects); fix this by
centralizing query-shaping logic: replace the direct field_as_string call in the
HttpFieldBinding::Query handling (and the similar block around the other
occurrences) with the shared helper used by
libs/toolkit-contract/src/runtime/http.rs (or extract a new common function
named something like field_to_query_pairs / flatten_field_query) so both the
dispatch/OpenAPI path and runtime URL builder use the same function to produce
query pairs from a field, ensuring objects/arrays are flattened/serialized
identically. Ensure you reference and reuse HttpMethodBindingIr’s existing
query-flattening behavior and update tests if necessary.

libs/toolkit-contract/src/runtime/http.rs-151-154 (1)

151-154: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Normalize parsed Problem.status against the wire status.

TransportError::is_transient() uses the embedded Problem.status, so a mismatched envelope here can change retry behavior and diagnostics away from the actual HTTP status that triggered this path. The response status is authoritative on the wire; validate or overwrite the parsed payload before returning it.

Suggested fix

 pub fn map_http_error(status: u16, body: String) -> TransportError {
-    if let Ok(problem) = serde_json::from_str::<Problem>(&body) {
+    if let Ok(mut problem) = serde_json::from_str::<Problem>(&body) {
+        problem.status = status;
         return TransportError::Problem(problem);
     }
     TransportError::HttpStatus {
         status,
         body: truncate(body, 256),

🤖 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 `@libs/toolkit-contract/src/runtime/http.rs` around lines 151 - 154, The parsed
Problem from serde_json::from_str in map_http_error may contain a different
Problem.status than the actual HTTP status; update map_http_error so that after
successfully parsing a Problem you normalize or overwrite problem.status with
the authoritative wire status (convert the u16 status to the Problem.status type
or set the numeric field) before returning TransportError::Problem(problem) so
TransportError::is_transient() and diagnostics reflect the real HTTP response
code.

libs/toolkit-contract/src/runtime/http.rs-204-208 (1)

204-208: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid panicking while truncating UTF-8 response bodies.

String::truncate requires a char boundary. If byte 256 lands in the middle of a multibyte character, error mapping panics and turns a normal HTTP failure into a client crash.

Suggested fix

 fn truncate(mut s: String, max: usize) -> String {
     if s.len() > max {
-        s.truncate(max);
+        let cut = s
+            .char_indices()
+            .map(|(idx, _)| idx)
+            .take_while(|idx| *idx <= max)
+            .last()
+            .unwrap_or(0);
+        s.truncate(cut);
         s.push('\u{2026}');
     }
     s
 }

🤖 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 `@libs/toolkit-contract/src/runtime/http.rs` around lines 204 - 208, The
truncate function can panic because String::truncate requires a char boundary;
update truncate(s: String, max: usize) to compute a safe boundary before
truncating: when s.len() > max, find a mutable index m = max and decrement m
while m > 0 and !s.is_char_boundary(m), then call s.truncate(m) and push the
ellipsis; reference the truncate function and parameters s and max to locate the
code and ensure you use the safe boundary (e.g., s.is_char_boundary) instead of
calling s.truncate(max) directly.

libs/toolkit-contract/src/runtime/client.rs-169-179 (1)

169-179: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Retry transient HTTP statuses during SSE reconnects.

This branch returns immediately on any non-success response, so 503, 429, 408, etc. end the stream even when reconnect.max_attempts > 0. That bypasses the transient classification already encoded in TransportError::is_transient() and makes overload/rolling-restart scenarios look permanent.

Suggested fix

             if !status.is_success() {
                 let status_code = status.as_u16();
                 let bytes = response.bytes().await.map_err(TransportError::network)?;
                 let body = String::from_utf8_lossy(&bytes).into_owned();
                 let err = map_http_error(status_code, body);
-                // Non-success responses are typically domain errors —
-                // bubble straight through without reconnect (server
-                // explicitly told us "no").
+                if attempt < reconnect.max_attempts && err.is_transient() {
+                    attempt += 1;
+                    sleep_backoff(&reconnect, attempt).await;
+                    continue;
+                }
                 Err(err)?;
                 return;
             }

🤖 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 `@libs/toolkit-contract/src/runtime/client.rs` around lines 169 - 179, The code
currently treats any non-success HTTP response as a terminal error; instead,
after mapping the response to an error via map_http_error you should check
TransportError::is_transient() (or equivalent transient classification) and the
reconnect policy (reconnect.max_attempts) and, if the error is transient and
reconnect.max_attempts > 0, propagate it in a way that triggers the SSE
reconnect/retry logic rather than immediately bailing out; for
non-transient/domain errors keep the existing immediate return behavior. Locate
the logic around response.status(), map_http_error, and the Err(err)? return and
branch based on err.is_transient() + reconnect.max_attempts to drive retries.

libs/toolkit-contract-macros/src/codegen.rs-76-88 (1)

76-88: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Descriptor input_type currently tracks the last parameter, not the request shape.

input_type is derived from m.params.last(), but the parser already distinguishes ParamRole::SecurityContext and the IR keeps every parameter. A method like fn charge(req: Charge, #[secctx] ctx: SecurityContext) will advertise SecurityContext as its request type, and multi-wire methods advertise only their last field. Filter to wire params here and either reject multi-wire signatures earlier or encode the full input shape in the descriptor.

🤖 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 `@libs/toolkit-contract-macros/src/codegen.rs` around lines 76 - 88, The
descriptor currently sets input_type from m.params.last(), which incorrectly
picks SecurityContext or only the last field; update the logic in the
method_descriptors construction to filter m.params by wire parameters (exclude
ParamRole::SecurityContext) before computing input_type_str via type_name_str,
and then: if the filtered list has zero params use Default/Unit, if it has one
use that param's type, and if it has multiple either return an error earlier
(reject multi-wire signatures) or encode the full input shape (e.g., synthesize
a tuple/struct type name) — adjust callers like
method_kind_tokens/idempotency_tokens usages accordingly so the descriptor
reflects the true request shape.

libs/toolkit-contract-macros/src/support.rs-14-17 (1)

14-17: ⚠️ Potential issue | 🟠 Major

Fix FoundCrate::Itself path resolution in contract_support_path()

cf-gears-toolkit-contract’s library name is toolkit_contract, but contract_support_path() maps proc_macro_crate::FoundCrate::Itself to ::toolkit_contract in libs/toolkit-contract-macros/src/support.rs, and there is no extern crate self as toolkit_contract anywhere in the repo. This can break macro expansion when used from within the crate (unit/in-crate usage), even though current macro usage appears only in libs/toolkit-contract/tests/* (integration tests). Emit crate for the Itself case instead of ::toolkit_contract.

🤖 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 `@libs/toolkit-contract-macros/src/support.rs` around lines 14 - 17, In
contract_support_path(), fix the FoundCrate::Itself mapping so generated code
refers to the local crate instead of a hardcoded absolute path: replace the
branch that returns quote!(::toolkit_contract) for
proc_macro_crate::FoundCrate::Itself with a return that emits the local crate
token (e.g., quote!(crate)) so macros expand correctly when used inside the
crate; update the match arm in contract_support_path() that currently handles
FoundCrate::Itself to emit the crate token.

libs/toolkit-contract-macros/src/codegen.rs-35-38 (1)

35-38: ⚠️ Potential issue | 🟠 Major

Stop hard-coding #[::async_trait::async_trait] in generated contract traits.

• libs/toolkit-contract-macros/src/codegen.rs’s generate_trait injects #[::async_trait::async_trait] directly, instead of using the crate path resolved by contract_support_path() (or an equivalent guaranteed re-export). That makes async-trait a hidden dependency of every macro consumer.
• The descriptor metadata in libs/toolkit-contract-macros/src/codegen.rs’s generate_descriptor uses m.params.last() for MethodDescriptor.input_type. Since params includes security-context params (and can include multiple wire params), the recorded input_type can diverge from the actual wire/body param selection logic used by GRPC/REST macros.

Implement async-trait via the resolved support path (or ensure both toolkit_contract and toolkit::contract_support re-export it at a stable location) and compute input_type from the same wire/body parameter rule used for GRPC/REST instead of params.last().

🤖 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 `@libs/toolkit-contract-macros/src/codegen.rs` around lines 35 - 38, In
generate_trait in libs/toolkit-contract-macros/src/codegen.rs, stop injecting
the hard-coded attribute #[::async_trait::async_trait] and instead apply the
async-trait attribute via the resolved support path (use the crate path returned
by contract_support_path() or the stable re-export in
toolkit_contract/toolkit::contract_support) so consumers don't get a hidden
dependency; and in generate_descriptor compute MethodDescriptor.input_type using
the same wire/body parameter selection logic used by the GRPC/REST macros (i.e.,
replicate or call the function that filters out security-context params and
picks the actual wire/body param) instead of using params.last(), ensuring the
recorded input_type matches the real wire/body param selection.

libs/toolkit-contract-macros/src/rest_contract_parse.rs-165-175 (1)

165-175: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Base-trait resolution should ignore marker supertraits in both parsers.

libs/toolkit-contract-macros/src/rest_contract_parse.rs and libs/toolkit-contract-macros/src/grpc_contract_parse.rs both pick the first trait bound as the base contract. That breaks valid declarations like trait FooRest: Send + Sync + FooApi / trait FooGrpc: Send + Sync + FooApi by resolving the base to Send. The shared fix is to skip marker supertraits and select the first non-marker contract bound in both files.

🤖 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 `@libs/toolkit-contract-macros/src/rest_contract_parse.rs` around lines 165 -
175, The base-trait resolution currently returns the first trait bound from
ItemTrait::supertraits (see extract_base_trait) which picks marker traits like
Send/Sync; change extract_base_trait to iterate supertraits and skip known
marker/auto traits (e.g., Send, Sync, Sized, Unpin, 'static, etc.) by inspecting
each syn::TypeParamBound::Trait(t) -> t.path and returning the first non-marker
path, and apply the same update to the equivalent resolver in the gRPC parser so
both parsers select the first non-marker contract bound as the base contract.

libs/toolkit-contract-macros/src/rest_contract.rs-263-280 (1)

263-280: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Do not default generated clients to plaintext HTTP.

new() builds its default client with AllowInsecureHttp, and the generated request path adds bearer tokens when a SecurityContext is present. That makes the out-of-the-box path capable of sending credentials over cleartext http:// URLs.

Please make insecure transport an explicit opt-in on with_http_client or a dedicated constructor, not the default constructor path.

🤖 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 `@libs/toolkit-contract-macros/src/rest_contract.rs` around lines 263 - 280,
The default HTTP client currently allows plaintext via
TransportSecurity::AllowInsecureHttp inside build_default_http_client(), which
enables sending bearer tokens over cleartext; change build_default_http_client()
to require TLS (use TransportSecurity::RequireTls or equivalent) so
new()/try_new() default to secure transport, remove AllowInsecureHttp from the
default path, and add an explicit opt-in API (e.g., a new constructor or a
with_insecure_http()/with_http_client parameter) that documents and sets
TransportSecurity::AllowInsecureHttp for callers that intentionally need
plaintext; update relevant docs/comments for build_default_http_client(), new(),
try_new(), and with_http_client to reflect the new default and opt-in behavior.

libs/toolkit-contract-macros/src/rest_contract.rs-329-338 (1)

329-338: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Streaming REST methods drop the request body on POST/PUT.

The unary path threads body_capture through with_json_body(), but the streaming path never receives or applies it. A #[streaming] #[post(...)] or #[put(...)] method will therefore generate a request without the payload even though the binding model marks one parameter as Body.

Either thread the captured body into __factory and apply with_json_body() there, or reject body-carrying verbs for streaming methods during parse so this fails at macro expansion instead of changing request semantics silently.

Also applies to: 414-504

🤖 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 `@libs/toolkit-contract-macros/src/rest_contract.rs` around lines 329 - 338,
The streaming branch (when method.streaming is true) never applies the captured
body (body_capture) into the generated request, so POST/PUT streaming methods
drop the payload; update generate_streaming_method_body to accept the
body_capture (or thread it into __factory) and call with_json_body(...) on the
request factory before sending, or alternatively add a parse-time validation in
the method parsing code to reject streaming methods that declare a Body
parameter (i.e., detect Body in the binding model and error) so we don't
silently drop the body; reference generate_streaming_method_body, body_capture,
with_json_body, and __factory when making the change.

libs/toolkit-contract-macros/src/rest_contract_parse.rs-185-200 (1)

185-200: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject multiple HTTP verb attributes on the same method.

A method annotated with both #[get(...)] and #[post(...)] is accepted today because each branch overwrites http. The last attribute wins instead of producing a diagnostic, which makes the generated binding depend on attribute order.

Suggested fix

     for attr in &method.attrs {
         let path = attr.path();
         if path.is_ident("get") {
-            http = Some((HttpVerb::Get, parse_path_lit(attr)?, attr.span()));
+            if http.is_some() {
+                return Err(syn::Error::new(attr.span(), "rest_contract methods may declare exactly one HTTP verb attribute"));
+            }
+            http = Some((HttpVerb::Get, parse_path_lit(attr)?, attr.span()));
         } else if path.is_ident("post") {
-            http = Some((HttpVerb::Post, parse_path_lit(attr)?, attr.span()));
+            if http.is_some() {
+                return Err(syn::Error::new(attr.span(), "rest_contract methods may declare exactly one HTTP verb attribute"));
+            }
+            http = Some((HttpVerb::Post, parse_path_lit(attr)?, attr.span()));
         } else if path.is_ident("put") {
-            http = Some((HttpVerb::Put, parse_path_lit(attr)?, attr.span()));
+            if http.is_some() {
+                return Err(syn::Error::new(attr.span(), "rest_contract methods may declare exactly one HTTP verb attribute"));
+            }
+            http = Some((HttpVerb::Put, parse_path_lit(attr)?, attr.span()));
         } else if path.is_ident("delete") {
-            http = Some((HttpVerb::Delete, parse_path_lit(attr)?, attr.span()));
+            if http.is_some() {
+                return Err(syn::Error::new(attr.span(), "rest_contract methods may declare exactly one HTTP verb attribute"));
+            }
+            http = Some((HttpVerb::Delete, parse_path_lit(attr)?, attr.span()));
         } else if path.is_ident("retryable") {

🤖 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 `@libs/toolkit-contract-macros/src/rest_contract_parse.rs` around lines 185 -
200, The loop over method.attrs currently lets multiple HTTP verb attributes
overwrite the http variable; update the code in the method.attrs handling (the
block that sets http via HttpVerb::Get/Post/Put/Delete using parse_path_lit and
attr.span()) to detect if http.is_some() before assigning and produce a
diagnostic instead of overwriting: if http.is_some() return
Err(syn::Error::new_spanned(attr, "multiple HTTP verb attributes on the same
method are not allowed")) (use attr.span() or attr for span), otherwise set http
as before; do this check for each branch that assigns http so the first verb
wins or an error is emitted.

libs/toolkit-contract-macros-tests/tests/ui/fail/grpc_fake_secctx.stderr-1-17 (1)

1-17: ⚠️ Potential issue | 🟠 Major

grpc_contract expands #[cfg(feature="grpc-client")] into the caller and triggers unexpected_cfgs

libs/toolkit-contract-macros-tests/Cargo.toml builds the UI test crate without a grpc-client feature (only toolkit-contract is enabled with features = ["rest-client"]), yet the grpc_contract attribute macro still emits #[cfg(feature = "grpc-client")], producing an unexpected cfg condition value: grpc-client warning in tests/ui/fail/grpc_fake_secctx.rs (and the same warning appears in grpc_unsupported_param_type.stderr) before the intended type-check failure. Crates with -D warnings can fail on this warning instead of the semantic error.

Adjust the macro expansion to avoid emitting an unexpected_cfgs-triggering cfg(feature="grpc-client") in callers without that feature (e.g., gate via type-level errors rather than a feature cfg, or suppress unexpected_cfgs locally around the generated cfg usage) so the UI goldens only bless the intended failure.

🤖 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 `@libs/toolkit-contract-macros-tests/tests/ui/fail/grpc_fake_secctx.stderr`
around lines 1 - 17, The macro grpc_contract is emitting #[cfg(feature =
"grpc-client")] into the caller which triggers unexpected_cfgs when the caller
crate doesn't have that feature; change the macro expansion so it does not emit
that caller-level cfg: either (preferred) replace the feature-gating with a
type-level or compile_error! based gate inside generated items (e.g., generate a
phantom trait/type or a compile_error when the runtime symbols are used) instead
of #[cfg(feature="grpc-client")], or (quick/acceptable) wrap the emitted cfg
usage with a local suppression by emitting #[allow(unexpected_cfgs)] on the
generated items that contain #[cfg(feature = "grpc-client")]; update the code
paths in the grpc_contract macro where it currently inserts "cfg(feature =
\"grpc-client\")" to use one of these approaches so tests like the expansion in
grpc_fake_secctx.rs no longer produce the unexpected_cfgs warning.

libs/toolkit-contract/tests/contract_error_derive.rs-9-10 (1)

9-10: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Run rustfmt on this import block.

Line 9 and Line 10 are out of rustfmt order, and the Fmt / 0_cargo fmt check is already failing on this file.

Suggested fix

-use toolkit_contract::{ContractError, Problem};
 use serde::{Deserialize, Serialize};
+use toolkit_contract::{ContractError, Problem};

🤖 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 `@libs/toolkit-contract/tests/contract_error_derive.rs` around lines 9 - 10,
Reorder and format the import block (or run rustfmt) so the two use statements
are in rustfmt's canonical order: place use serde::{Deserialize, Serialize};
before use toolkit_contract::{ContractError, Problem}; (or simply run cargo fmt
/ rustfmt on the file) to fix the Fmt / 0_cargo fmt failure referencing the
import lines with use serde::{Deserialize, Serialize} and use
toolkit_contract::{ContractError, Problem}.

Source: Pipeline failures

[coderabbitai]

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Tests reference undefined types ModuleManager and ModuleInstance.

The tests use ModuleManager::new() and ModuleInstance::new() but this file imports GearManager and GearInstance from crate::runtime. These types don't exist and the tests won't compile.

Proposed fix for all affected tests

 #[tokio::test]
 async fn test_resolve_rest_service_not_found() {
-    let dir = Arc::new(ModuleManager::new());
+    let dir = Arc::new(GearManager::new());
     let api = LocalDirectoryClient::new(dir);

And similarly for lines 242, 247, 263, 293, 298, 320, 324 — replace ModuleManager with GearManager and ModuleInstance with GearInstance.

🤖 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 `@libs/toolkit/src/directory.rs` around lines 231 - 238, Tests are failing to
compile because they reference non-existent types ModuleManager and
ModuleInstance; replace those with the actual types exported from crate::runtime
(GearManager and GearInstance) wherever used (e.g., in
test_resolve_rest_service_not_found and the other tests that call
LocalDirectoryClient, resolve_rest_service, resolve_rest_instance, etc.), update
the constructors to Arc::new(GearManager::new()) and GearInstance::new() (or the
appropriate constructor signatures), and adjust any imports/uses to reference
GearManager and GearInstance instead of ModuleManager/ModuleInstance across the
affected tests.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Tests in libs/toolkit/src/directory.rs and libs/toolkit/src/runtime/gear_manager.rs reference undefined types.

Both files' new tests use ModuleManager, ModuleInstance, and pick_rest_module which don't exist in the codebase. The correct types defined in gear_manager.rs are GearManager, GearInstance, and pick_rest_gear. Additionally, RegisterInstanceInfo uses the field gear, not module.

This appears to be a naming inconsistency—possibly from an earlier iteration or copy-paste from a different module naming convention. All affected tests will fail to compile.

🤖 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 `@libs/toolkit/src/directory.rs` around lines 231 - 337, Replace the incorrect
module-oriented types/fields in the tests with the gear-oriented names used in
the codebase: change ModuleManager to GearManager, ModuleInstance to
GearInstance, pick_rest_module to pick_rest_gear, and use the
RegisterInstanceInfo field gear (not module); update any related
constructors/usages (e.g., ModuleInstance::new(...) -> GearInstance::new(...),
ModuleManager::new() -> GearManager::new(), and any uses of module/instance_id
fields) so the test references match GearManager, GearInstance, pick_rest_gear,
and RegisterInstanceInfo.gear.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Test uses wrong field name .module instead of .gear.

RegisterInstanceInfo has a gear field (as seen in line 94's usage: info.gear.clone()), not a module field. This won't compile.

Proposed fix

         let register_info = RegisterInstanceInfo {
-            module: "billing".to_owned(),
+            gear: "billing".to_owned(),
             instance_id: instance_id.to_string(),

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	let register_info = RegisterInstanceInfo {
	module: "billing".to_owned(),
	instance_id: instance_id.to_string(),
	let register_info = RegisterInstanceInfo {
	gear: "billing".to_owned(),
	instance_id: instance_id.to_string(),

🤖 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 `@libs/toolkit/src/directory.rs` around lines 267 - 269, The code is
constructing a RegisterInstanceInfo with a non-existent .module field; change
the struct literal in the creation of register_info to use the correct field
name .gear (e.g., gear: "billing".to_owned()) and keep instance_id:
instance_id.to_string() so it matches the RegisterInstanceInfo definition used
elsewhere (see RegisterInstanceInfo and register_info).