Merged
Conversation
Contributor
WalkthroughThis PR adds end-to-end AI tool support for entity properties. Frontend: new Properties.tsx defines renderers for GetEntityProperties and SetEntityProperty and handler mapping entries are added. Backend: a new properties crate with domain models, repository/service methods, outbound queries, inbound AsyncTools (get/set), toolset context, SQLx query metadata, and tests. Tool contexts and API/server wiring are extended to include a properties tool context; several Cargo.toml files updated to wire the feature. Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@rust/cloud-storage/macro_db_client/.sqlx/query-cf71680da3444f573f4cddf66b6269bfe31aeee4bbec34b7e6067a855e28f2b7.json`:
- Around line 1-15: The UPDATE currently targets entity_properties by only
entity_id and will update multiple rows; modify the SQL in the query (hash
cf71680da3444f573f4cddf66b6269bfe31aeee4bbec34b7e6067a855e28f2b7) to include
entity_type and property_definition_id in the WHERE clause (e.g. WHERE entity_id
= $1 AND entity_type = $2 AND property_definition_id = $3) and adjust the
parameter list/types so the values parameter becomes the last placeholder (shift
existing placeholders accordingly) to match the schema's composite unique
constraint.
In `@rust/cloud-storage/properties/src/inbound/toolset/get_entity_properties.rs`:
- Around line 104-121: The read path ignores RequestContext so caller identity
isn't enforced; update the get-entity read flow to thread the caller/user id
from RequestContext into the service and query layers: modify the call method in
get_entity_properties.rs to extract the caller (e.g., user_id) from
RequestContext and pass it into service.get_entity_properties, update
PropertiesService::get_entity_properties signature in domain/service.rs to
accept the caller id (or Option<UserId>), and propagate that parameter into
outbound/entity_properties_get_query.rs so the SQL/filtering logic includes
per-user access checks (similar to SetEntityProperty's write path). Ensure all
call sites are updated to compile and preserve existing behavior when caller id
is absent.
- Around line 81-83: The options field currently has #[serde(skip_serializing_if
= "Vec::is_empty")] which omits options for non-select properties and breaks the
generated client schema; remove that skip_serializing_if attribute on the
options field (and add #[serde(default)] if not already present) so the options:
Vec<ToolPropertyOption> is always serialized (as an empty array when there are
no options) to satisfy the generated schema and maintain a stable response
contract.
In `@rust/cloud-storage/properties/src/inbound/toolset/mod.rs`:
- Around line 45-46: The GetEntityProperties tool is exposed without any caller
auth; update the read path so the service requires caller context and enforces
permissions: change the signature of get_entity_properties in domain::service
(and its implementation in domain::service_impl) to accept an actor/request
context (e.g. RequestContext or ActorId), thread that context through
PropertiesToolContext<T> used by the tool registration in inbound::toolset::mod
(where .add_tool::<GetEntityProperties, PropertiesToolContext<T>>() is added),
and call PermissionServiceImpl (e.g. PermissionServiceImpl::check_permission or
equivalent) before forwarding to the repository in
service_impl::get_entity_properties; ensure the tool registration no longer
allows invocation without a populated caller context so guesses of entity_id
cannot bypass auth.
In `@rust/cloud-storage/properties/src/inbound/toolset/set_entity_property.rs`:
- Around line 111-159: to_set_property_value currently returns the first
populated field or None, which allows ambiguous or empty payloads; change it to
validate that exactly one value field is present and return a ToolCallError on
ambiguous or missing values. In practice: in to_set_property_value, count how
many of boolean_value, date_value, number_value, string_value, option_id,
option_ids, entity_ref, entity_refs, link_url, link_urls are Some (treat
string/vec refs as Some when non-empty if applicable), if count == 1 return
Ok(the matched SetPropertyValue) otherwise return
Err(ToolCallError::InvalidArguments("must provide exactly one value field")) (or
the project’s equivalent ToolCallError variant); update the function signature
from Option<SetPropertyValue> to Result<SetPropertyValue, ToolCallError> and
adjust all callers of to_set_property_value (e.g., any call sites that unwrap or
match Option) to handle the new error result.
In `@rust/cloud-storage/properties/src/inbound/toolset/test.rs`:
- Around line 35-63: The documented cargo test commands for the schema-printer
tests (print_get_input_schema, print_set_input_schema, print_get_output_schema,
print_set_output_schema) are missing the feature gate; update the commands to
include --features ai_tools so the tests that use generate_tool_input_schema!
and ai::tool::minimized_output_schema_generator will build/run with the ai_tools
feature enabled. Ensure every printed command example shown for those test
functions appends --features ai_tools (e.g., cargo test ... -- --nocapture
--include-ignored should become cargo test --features ai_tools ... --
--nocapture --include-ignored).
In `@rust/cloud-storage/properties/src/outbound/entity_properties_get_query.rs`:
- Around line 32-37: The tracing span on get_entity_properties does not record
returned errors; update the attribute on the function (get_entity_properties) to
include error capture by changing the existing
#[tracing::instrument(skip(pool))] to #[tracing::instrument(skip(pool), err)] so
that failures returned as anyhow::Result are recorded while still skipping the
DB pool in spans.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: eb33fc7e-0aa1-4a7b-9357-ed70d7529ea2
⛔ Files ignored due to path filters (4)
js/app/packages/service-clients/service-cognition/generated/tools/schemas.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/tools/tool.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/tools/types.tsis excluded by!**/generated/**rust/cloud-storage/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
js/app/packages/core/component/AI/component/tool/Properties.tsxjs/app/packages/core/component/AI/component/tool/handler.tsxrust/cloud-storage/ai_tools/Cargo.tomlrust/cloud-storage/ai_tools/src/lib.rsrust/cloud-storage/ai_tools/src/tool_context.rsrust/cloud-storage/document_cognition_service/Cargo.tomlrust/cloud-storage/document_cognition_service/src/api/chats/tool/post.rsrust/cloud-storage/document_cognition_service/src/api/context/mod.rsrust/cloud-storage/document_cognition_service/src/api/context/test.rsrust/cloud-storage/document_cognition_service/src/api/stream/chat_message.rsrust/cloud-storage/document_cognition_service/src/bin/mcp_server.rsrust/cloud-storage/document_cognition_service/src/main.rsrust/cloud-storage/macro_db_client/.sqlx/query-1435acbba365796ddcb6ddbef86f4ebf664cffc44082996f6b60a60373c28f75.jsonrust/cloud-storage/macro_db_client/.sqlx/query-63492b018803ce26db5891964a0f52f37d2f54bd8243265c6561f4423af8617c.jsonrust/cloud-storage/macro_db_client/.sqlx/query-9cd40fbb1ade51e9e0d49792839d1a363cada017252a6f653c2272608189bcc4.jsonrust/cloud-storage/macro_db_client/.sqlx/query-cf71680da3444f573f4cddf66b6269bfe31aeee4bbec34b7e6067a855e28f2b7.jsonrust/cloud-storage/macro_db_client/.sqlx/query-f2d5d7aea9918978cd69c14f8afe24156c2fa387e001ee8eaf57e4a70a106069.jsonrust/cloud-storage/properties/.sqlx/query-4eef29ad204776f70531f685f709f4e18f2b4f992d73ad2b608b77869a7856a9.jsonrust/cloud-storage/properties/.sqlx/query-8d5ffbdd5be9b1031c7da47bc2d4bcbee35802a575387ef081a09031b42f7de9.jsonrust/cloud-storage/properties/Cargo.tomlrust/cloud-storage/properties/src/domain.rsrust/cloud-storage/properties/src/domain/model.rsrust/cloud-storage/properties/src/domain/ports.rsrust/cloud-storage/properties/src/domain/service.rsrust/cloud-storage/properties/src/domain/service_impl/mod.rsrust/cloud-storage/properties/src/inbound.rsrust/cloud-storage/properties/src/inbound/toolset/get_entity_properties.rsrust/cloud-storage/properties/src/inbound/toolset/mod.rsrust/cloud-storage/properties/src/inbound/toolset/set_entity_property.rsrust/cloud-storage/properties/src/inbound/toolset/test.rsrust/cloud-storage/properties/src/lib.rsrust/cloud-storage/properties/src/outbound.rsrust/cloud-storage/properties/src/outbound/entity_properties_get_query.rsrust/cloud-storage/properties/src/outbound/properties_pg_repo.rs
Comment on lines
+1
to
+15
| { | ||
| "db_name": "PostgreSQL", | ||
| "query": "\n UPDATE entity_properties\n SET values = $2\n WHERE entity_id = $1;\n ", | ||
| "describe": { | ||
| "columns": [], | ||
| "parameters": { | ||
| "Left": [ | ||
| "Text", | ||
| "Jsonb" | ||
| ] | ||
| }, | ||
| "nullable": [] | ||
| }, | ||
| "hash": "cf71680da3444f573f4cddf66b6269bfe31aeee4bbec34b7e6067a855e28f2b7" | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the entity_properties table schema and constraints to check if entity_id is unique alone
# Search for entity_properties table definition and constraints in migrations
fd -e sql . rust/cloud-storage/macro_db_client/migrations --exec grep -l "entity_properties" {} \; | xargs -I {} sh -c 'echo "=== {} ===" && cat {}'
# Also check the Rust code that calls this query to understand the context
rg -n "UPDATE entity_properties" --type rust -C 5Repository: macro-inc/macro
Length of output: 11877
Add entity_type and property_definition_id to the WHERE clause.
The UPDATE query matches only entity_id, but the schema defines a composite unique constraint on (entity_id, entity_type, property_definition_id). A single entity_id can have multiple rows across different entity types and property definitions. This query would unintentionally update all matching rows. Other UPDATE queries in the codebase (e.g., properties/src/outbound/task_property_queries.rs) correctly use all three columns in the WHERE clause.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@rust/cloud-storage/macro_db_client/.sqlx/query-cf71680da3444f573f4cddf66b6269bfe31aeee4bbec34b7e6067a855e28f2b7.json`
around lines 1 - 15, The UPDATE currently targets entity_properties by only
entity_id and will update multiple rows; modify the SQL in the query (hash
cf71680da3444f573f4cddf66b6269bfe31aeee4bbec34b7e6067a855e28f2b7) to include
entity_type and property_definition_id in the WHERE clause (e.g. WHERE entity_id
= $1 AND entity_type = $2 AND property_definition_id = $3) and adjust the
parameter list/types so the values parameter becomes the last placeholder (shift
existing placeholders accordingly) to match the schema's composite unique
constraint.
rust/cloud-storage/properties/src/inbound/toolset/get_entity_properties.rs
Show resolved
Hide resolved
Comment on lines
+104
to
+121
| async fn call( | ||
| &self, | ||
| service_context: ServiceContext<PropertiesToolContext<T>>, | ||
| request_context: RequestContext, | ||
| ) -> ToolResult<Self::Output> { | ||
| tracing::info!(params=?self, "Get entity properties"); | ||
| let _ = &request_context; | ||
|
|
||
| let entity_type = EntityType::from(self.entity_type); | ||
|
|
||
| let props = service_context | ||
| .service | ||
| .get_entity_properties(&self.entity_id, entity_type) | ||
| .await | ||
| .map_err(|e| ToolCallError { | ||
| description: format!("Failed to get entity properties: {e}"), | ||
| internal_error: e.into(), | ||
| })?; |
Contributor
There was a problem hiding this comment.
Thread the caller identity through the read path.
request_context is effectively ignored here, and the provided rust/cloud-storage/properties/src/domain/service.rs:49-53 / rust/cloud-storage/properties/src/outbound/entity_properties_get_query.rs:32-140 path only filters by entity_id and entity_type. That gives this tool no place to enforce per-user access before returning property values, unlike SetEntityProperty, which passes user_id on writes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/properties/src/inbound/toolset/get_entity_properties.rs`
around lines 104 - 121, The read path ignores RequestContext so caller identity
isn't enforced; update the get-entity read flow to thread the caller/user id
from RequestContext into the service and query layers: modify the call method in
get_entity_properties.rs to extract the caller (e.g., user_id) from
RequestContext and pass it into service.get_entity_properties, update
PropertiesService::get_entity_properties signature in domain/service.rs to
accept the caller id (or Option<UserId>), and propagate that parameter into
outbound/entity_properties_get_query.rs so the SQL/filtering logic includes
per-user access checks (similar to SetEntityProperty's write path). Ensure all
call sites are updated to compile and preserve existing behavior when caller id
is absent.
Comment on lines
+45
to
+46
| .add_tool::<GetEntityProperties, PropertiesToolContext<T>>() | ||
| .expect("failed to add GetEntityProperties tool") |
Contributor
There was a problem hiding this comment.
Do not expose GetEntityProperties without caller authorization.
Line 45 registers an AI read tool, but rust/cloud-storage/properties/src/domain/service.rs around Lines 49-53 gives get_entity_properties no actor/request parameter, and rust/cloud-storage/properties/src/domain/service_impl/mod.rs around Lines 147-157 forwards straight to the repository. That makes this tool an authorization bypass for a tool caller who can guess an entity_id. Thread requester context into this read path and enforce PermissionServiceImpl before the repo call.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/properties/src/inbound/toolset/mod.rs` around lines 45 -
46, The GetEntityProperties tool is exposed without any caller auth; update the
read path so the service requires caller context and enforces permissions:
change the signature of get_entity_properties in domain::service (and its
implementation in domain::service_impl) to accept an actor/request context (e.g.
RequestContext or ActorId), thread that context through PropertiesToolContext<T>
used by the tool registration in inbound::toolset::mod (where
.add_tool::<GetEntityProperties, PropertiesToolContext<T>>() is added), and call
PermissionServiceImpl (e.g. PermissionServiceImpl::check_permission or
equivalent) before forwarding to the repository in
service_impl::get_entity_properties; ensure the tool registration no longer
allows invocation without a populated caller context so guesses of entity_id
cannot bypass auth.
Comment on lines
+111
to
+159
| fn to_set_property_value(&self) -> Option<SetPropertyValue> { | ||
| if let Some(v) = self.boolean_value { | ||
| return Some(SetPropertyValue::Boolean { value: v }); | ||
| } | ||
| if let Some(v) = self.date_value { | ||
| return Some(SetPropertyValue::Date { value: v }); | ||
| } | ||
| if let Some(v) = self.number_value { | ||
| return Some(SetPropertyValue::Number { value: v }); | ||
| } | ||
| if let Some(v) = &self.string_value { | ||
| return Some(SetPropertyValue::String { value: v.clone() }); | ||
| } | ||
| if let Some(v) = self.option_id { | ||
| return Some(SetPropertyValue::SelectOption { option_id: v }); | ||
| } | ||
| if let Some(v) = &self.option_ids { | ||
| return Some(SetPropertyValue::MultiSelectOption { | ||
| option_ids: v.clone(), | ||
| }); | ||
| } | ||
| if let Some(v) = &self.entity_ref { | ||
| return Some(SetPropertyValue::EntityReference { | ||
| reference: EntityReference { | ||
| entity_type: EntityType::from(v.entity_type), | ||
| entity_id: v.entity_id.clone(), | ||
| specific_message_id: None, | ||
| }, | ||
| }); | ||
| } | ||
| if let Some(v) = &self.entity_refs { | ||
| return Some(SetPropertyValue::MultiEntityReference { | ||
| references: v | ||
| .iter() | ||
| .map(|r| EntityReference { | ||
| entity_type: EntityType::from(r.entity_type), | ||
| entity_id: r.entity_id.clone(), | ||
| specific_message_id: None, | ||
| }) | ||
| .collect(), | ||
| }); | ||
| } | ||
| if let Some(v) = &self.link_url { | ||
| return Some(SetPropertyValue::Link { url: v.clone() }); | ||
| } | ||
| if let Some(v) = &self.link_urls { | ||
| return Some(SetPropertyValue::MultiLink { urls: v.clone() }); | ||
| } | ||
| None |
Contributor
There was a problem hiding this comment.
Reject ambiguous value payloads instead of taking the first match.
to_set_property_value() silently returns the first populated field. A request that includes both string_value and option_id will ignore the later field, and an all-empty payload falls through to None with whatever downstream meaning that has. The tool contract says callers must provide exactly one value field, so this path should validate that explicitly and return a ToolCallError when the request is ambiguous.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/properties/src/inbound/toolset/set_entity_property.rs`
around lines 111 - 159, to_set_property_value currently returns the first
populated field or None, which allows ambiguous or empty payloads; change it to
validate that exactly one value field is present and return a ToolCallError on
ambiguous or missing values. In practice: in to_set_property_value, count how
many of boolean_value, date_value, number_value, string_value, option_id,
option_ids, entity_ref, entity_refs, link_url, link_urls are Some (treat
string/vec refs as Some when non-empty if applicable), if count == 1 return
Ok(the matched SetPropertyValue) otherwise return
Err(ToolCallError::InvalidArguments("must provide exactly one value field")) (or
the project’s equivalent ToolCallError variant); update the function signature
from Option<SetPropertyValue> to Result<SetPropertyValue, ToolCallError> and
adjust all callers of to_set_property_value (e.g., any call sites that unwrap or
match Option) to handle the new error result.
Comment on lines
+35
to
+63
| // run `cargo test -p properties inbound::toolset::test::print_get_input_schema -- --nocapture --include-ignored` | ||
| #[test] | ||
| #[ignore = "prints the input schema"] | ||
| fn print_get_input_schema() { | ||
| let schema = generate_tool_input_schema!(GetEntityProperties); | ||
| println!("{}", serde_json::to_string_pretty(&schema).unwrap()); | ||
| } | ||
|
|
||
| // run `cargo test -p properties inbound::toolset::test::print_set_input_schema -- --nocapture --include-ignored` | ||
| #[test] | ||
| #[ignore = "prints the input schema"] | ||
| fn print_set_input_schema() { | ||
| let schema = generate_tool_input_schema!(SetEntityProperty); | ||
| println!("{}", serde_json::to_string_pretty(&schema).unwrap()); | ||
| } | ||
|
|
||
| // run `cargo test -p properties inbound::toolset::test::print_get_output_schema -- --nocapture --include-ignored` | ||
| #[test] | ||
| #[ignore = "prints the output schema"] | ||
| fn print_get_output_schema() { | ||
| let generator = ai::tool::minimized_output_schema_generator(); | ||
| let schema = generator.into_root_schema_for::<GetEntityPropertiesResponse>(); | ||
| println!("{}", serde_json::to_string_pretty(&schema).unwrap()); | ||
| } | ||
|
|
||
| // run `cargo test -p properties inbound::toolset::test::print_set_output_schema -- --nocapture --include-ignored` | ||
| #[test] | ||
| #[ignore = "prints the output schema"] | ||
| fn print_set_output_schema() { |
Contributor
There was a problem hiding this comment.
Add --features ai_tools to the documented test commands.
Lines 35, 43, 51, and 60 omit the feature gate introduced in rust/cloud-storage/properties/Cargo.toml on Lines 7-13. As written, these commands won't build/run the ignored schema-printer tests in a default invocation.
Suggested update
-// run `cargo test -p properties inbound::toolset::test::print_get_input_schema -- --nocapture --include-ignored`
+// run `cargo test -p properties --features ai_tools inbound::toolset::test::print_get_input_schema -- --nocapture --include-ignored`
-// run `cargo test -p properties inbound::toolset::test::print_set_input_schema -- --nocapture --include-ignored`
+// run `cargo test -p properties --features ai_tools inbound::toolset::test::print_set_input_schema -- --nocapture --include-ignored`
-// run `cargo test -p properties inbound::toolset::test::print_get_output_schema -- --nocapture --include-ignored`
+// run `cargo test -p properties --features ai_tools inbound::toolset::test::print_get_output_schema -- --nocapture --include-ignored`
-// run `cargo test -p properties inbound::toolset::test::print_set_output_schema -- --nocapture --include-ignored`
+// run `cargo test -p properties --features ai_tools inbound::toolset::test::print_set_output_schema -- --nocapture --include-ignored`📝 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
| // run `cargo test -p properties inbound::toolset::test::print_get_input_schema -- --nocapture --include-ignored` | |
| #[test] | |
| #[ignore = "prints the input schema"] | |
| fn print_get_input_schema() { | |
| let schema = generate_tool_input_schema!(GetEntityProperties); | |
| println!("{}", serde_json::to_string_pretty(&schema).unwrap()); | |
| } | |
| // run `cargo test -p properties inbound::toolset::test::print_set_input_schema -- --nocapture --include-ignored` | |
| #[test] | |
| #[ignore = "prints the input schema"] | |
| fn print_set_input_schema() { | |
| let schema = generate_tool_input_schema!(SetEntityProperty); | |
| println!("{}", serde_json::to_string_pretty(&schema).unwrap()); | |
| } | |
| // run `cargo test -p properties inbound::toolset::test::print_get_output_schema -- --nocapture --include-ignored` | |
| #[test] | |
| #[ignore = "prints the output schema"] | |
| fn print_get_output_schema() { | |
| let generator = ai::tool::minimized_output_schema_generator(); | |
| let schema = generator.into_root_schema_for::<GetEntityPropertiesResponse>(); | |
| println!("{}", serde_json::to_string_pretty(&schema).unwrap()); | |
| } | |
| // run `cargo test -p properties inbound::toolset::test::print_set_output_schema -- --nocapture --include-ignored` | |
| #[test] | |
| #[ignore = "prints the output schema"] | |
| fn print_set_output_schema() { | |
| // run `cargo test -p properties --features ai_tools inbound::toolset::test::print_get_input_schema -- --nocapture --include-ignored` | |
| #[test] | |
| #[ignore = "prints the input schema"] | |
| fn print_get_input_schema() { | |
| let schema = generate_tool_input_schema!(GetEntityProperties); | |
| println!("{}", serde_json::to_string_pretty(&schema).unwrap()); | |
| } | |
| // run `cargo test -p properties --features ai_tools inbound::toolset::test::print_set_input_schema -- --nocapture --include-ignored` | |
| #[test] | |
| #[ignore = "prints the input schema"] | |
| fn print_set_input_schema() { | |
| let schema = generate_tool_input_schema!(SetEntityProperty); | |
| println!("{}", serde_json::to_string_pretty(&schema).unwrap()); | |
| } | |
| // run `cargo test -p properties --features ai_tools inbound::toolset::test::print_get_output_schema -- --nocapture --include-ignored` | |
| #[test] | |
| #[ignore = "prints the output schema"] | |
| fn print_get_output_schema() { | |
| let generator = ai::tool::minimized_output_schema_generator(); | |
| let schema = generator.into_root_schema_for::<GetEntityPropertiesResponse>(); | |
| println!("{}", serde_json::to_string_pretty(&schema).unwrap()); | |
| } | |
| // run `cargo test -p properties --features ai_tools inbound::toolset::test::print_set_output_schema -- --nocapture --include-ignored` | |
| #[test] | |
| #[ignore = "prints the output schema"] | |
| fn print_set_output_schema() { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/properties/src/inbound/toolset/test.rs` around lines 35 -
63, The documented cargo test commands for the schema-printer tests
(print_get_input_schema, print_set_input_schema, print_get_output_schema,
print_set_output_schema) are missing the feature gate; update the commands to
include --features ai_tools so the tests that use generate_tool_input_schema!
and ai::tool::minimized_output_schema_generator will build/run with the ai_tools
feature enabled. Ensure every printed command example shown for those test
functions appends --features ai_tools (e.g., cargo test ... -- --nocapture
--include-ignored should become cargo test --features ai_tools ... --
--nocapture --include-ignored).
Comment on lines
+32
to
+37
| #[tracing::instrument(skip(pool))] | ||
| pub async fn get_entity_properties( | ||
| pool: &Pool<Postgres>, | ||
| entity_id: &str, | ||
| entity_type: EntityType, | ||
| ) -> anyhow::Result<Vec<EntityPropertyInfo>> { |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cat -n rust/cloud-storage/properties/src/outbound/entity_properties_get_query.rs | head -50Repository: macro-inc/macro
Length of output: 2019
Include err on this tracing span.
This function returns anyhow::Result, so the current attribute on line 32 does not record the error on failed executions. Switch it to #[tracing::instrument(skip(pool), err)].
Suggested change
-#[tracing::instrument(skip(pool))]
+#[tracing::instrument(skip(pool), err)]📝 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
| #[tracing::instrument(skip(pool))] | |
| pub async fn get_entity_properties( | |
| pool: &Pool<Postgres>, | |
| entity_id: &str, | |
| entity_type: EntityType, | |
| ) -> anyhow::Result<Vec<EntityPropertyInfo>> { | |
| #[tracing::instrument(skip(pool), err)] | |
| pub async fn get_entity_properties( | |
| pool: &Pool<Postgres>, | |
| entity_id: &str, | |
| entity_type: EntityType, | |
| ) -> anyhow::Result<Vec<EntityPropertyInfo>> { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/properties/src/outbound/entity_properties_get_query.rs`
around lines 32 - 37, The tracing span on get_entity_properties does not record
returned errors; update the attribute on the function (get_entity_properties) to
include error capture by changing the existing
#[tracing::instrument(skip(pool))] to #[tracing::instrument(skip(pool), err)] so
that failures returned as anyhow::Result are recorded while still skipping the
DB pool in spans.
ehayes2000
force-pushed
the
properties-tool
branch
2 times, most recently
from
f3ee754 to
5ff58ae
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust/cloud-storage/document_cognition_service/src/api/context/test.rs (1)
255-261:⚠️ Potential issue | 🔴 CriticalBlocker:
ToolServiceContextliteral is missing the requiredproperties_tool_contextfield.The struct literal at lines 255-261 omits
properties_tool_context, which will fail compilation once the field is added to the struct definition. Initializeproperties_tool_contextbefore thememory_tool_contextliteral and include it in the struct.Proposed fix
+ let properties_service = properties::PropertiesServiceImpl::new( + properties::PropertiesPgRepo::new(pool.clone()), + Some(properties::PermissionServiceImpl::new(pool.clone())), + Some(ai_tools::NoOpNotificationService), + ); + let properties_tool_context = + properties::inbound::toolset::PropertiesToolContext::new(properties_service); + let memory_tool_context = ai_tools::ToolServiceContext { search_service_client: search_service_client.clone(), email_service_client: email_service_client_external.clone(), scribe: scribe.clone(), soup_service: soup_service.clone(), document_tool_context: document_tool_context.clone(), + properties_tool_context: properties_tool_context.clone(), }; - // Build properties tool context - let properties_service = properties::PropertiesServiceImpl::new( - properties::PropertiesPgRepo::new(pool.clone()), - Some(properties::PermissionServiceImpl::new(pool.clone())), - Some(ai_tools::NoOpNotificationService), - ); - let properties_tool_context = - properties::inbound::toolset::PropertiesToolContext::new(properties_service);Also applies to:
rust/cloud-storage/document_cognition_service/src/main.rs:273andrust/cloud-storage/memory/src/context.rs:146.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/document_cognition_service/src/api/context/test.rs` around lines 255 - 261, The ToolServiceContext struct literal for memory_tool_context is missing the newly required properties_tool_context field; create and initialize a suitable PropertiesToolContext value (or reuse an existing properties context variable) before building memory_tool_context and add properties_tool_context: properties_tool_context into the ToolServiceContext literal (referencing the ToolServiceContext type and the memory_tool_context and document_tool_context variables), and apply the same addition in the other occurrences noted (main.rs and memory/src/context.rs).rust/cloud-storage/document_cognition_service/src/main.rs (1)
273-279:⚠️ Potential issue | 🔴 CriticalAdd missing
properties_tool_contextfield toToolServiceContextinitialization.The
memory_tool_contextinitialization at lines 273–279 is missing the requiredproperties_tool_contextfield. TheToolServiceContextstruct requires this field (type:ToolPropertiesToolContext), and it is not optional. The field is created separately at line 296–297 but must be included in the struct initialization:Add to memory_tool_context initialization
let memory_tool_context = ai_tools::ToolServiceContext { search_service_client: search_service_client.clone(), email_service_client: email_service_client_external.clone(), scribe: scribe.clone(), soup_service: soup_service.clone(), document_tool_context: document_tool_context.clone(), properties_tool_context: properties_tool_context.clone(), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/document_cognition_service/src/main.rs` around lines 273 - 279, The ToolServiceContext initialization for memory_tool_context is missing the required properties_tool_context field; update the ai_tools::ToolServiceContext literal where memory_tool_context is created to include properties_tool_context: properties_tool_context.clone(), so the struct satisfies the required ToolPropertiesToolContext field (refer to memory_tool_context and ai_tools::ToolServiceContext and the separately created properties_tool_context variable).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/document_cognition_service/Cargo.toml`:
- Around line 82-84: Reorder the dependency entries in Cargo.toml so they are
alphabetical by crate name: move the properties = { path = "../properties",
features = ["ai_tools"] } line to appear after macro_middleware = { ... } and
before notification = { path = "../notification" }; ensure no other ordering or
formatting changes are introduced and keep the macro_middleware default-features
and features block intact.
---
Outside diff comments:
In `@rust/cloud-storage/document_cognition_service/src/api/context/test.rs`:
- Around line 255-261: The ToolServiceContext struct literal for
memory_tool_context is missing the newly required properties_tool_context field;
create and initialize a suitable PropertiesToolContext value (or reuse an
existing properties context variable) before building memory_tool_context and
add properties_tool_context: properties_tool_context into the ToolServiceContext
literal (referencing the ToolServiceContext type and the memory_tool_context and
document_tool_context variables), and apply the same addition in the other
occurrences noted (main.rs and memory/src/context.rs).
In `@rust/cloud-storage/document_cognition_service/src/main.rs`:
- Around line 273-279: The ToolServiceContext initialization for
memory_tool_context is missing the required properties_tool_context field;
update the ai_tools::ToolServiceContext literal where memory_tool_context is
created to include properties_tool_context: properties_tool_context.clone(), so
the struct satisfies the required ToolPropertiesToolContext field (refer to
memory_tool_context and ai_tools::ToolServiceContext and the separately created
properties_tool_context variable).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8bab2ca1-2a5c-42b5-8d74-498ce194213a
⛔ Files ignored due to path filters (4)
js/app/packages/service-clients/service-cognition/generated/tools/schemas.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/tools/tool.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/tools/types.tsis excluded by!**/generated/**rust/cloud-storage/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
js/app/packages/core/component/AI/component/tool/Properties.tsxjs/app/packages/core/component/AI/component/tool/handler.tsxrust/cloud-storage/ai_tools/Cargo.tomlrust/cloud-storage/ai_tools/src/lib.rsrust/cloud-storage/ai_tools/src/tool_context.rsrust/cloud-storage/document_cognition_service/Cargo.tomlrust/cloud-storage/document_cognition_service/src/api/chats/tool/post.rsrust/cloud-storage/document_cognition_service/src/api/context/mod.rsrust/cloud-storage/document_cognition_service/src/api/context/test.rsrust/cloud-storage/document_cognition_service/src/api/stream/chat_message.rsrust/cloud-storage/document_cognition_service/src/bin/mcp_server.rsrust/cloud-storage/document_cognition_service/src/main.rs
Comment on lines
82
to
84
| notification = { path = "../notification" } | ||
| properties = { path = "../properties", features = ["ai_tools"] } | ||
| macro_middleware = { path = "../macro_middleware", default-features = false, features = [ |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider alphabetical ordering of dependencies.
The properties dependency is inserted between notification and macro_middleware, but alphabetically it should come after macro_middleware (m < n < p). While this doesn't affect functionality, maintaining alphabetical order improves maintainability.
📝 Suggested reordering
notification = { path = "../notification" }
-properties = { path = "../properties", features = ["ai_tools"] }
macro_middleware = { path = "../macro_middleware", default-features = false, features = [
"auth",
"cloud_storage",
"user_permissions",
] }
macro_project_utils = { path = "../macro_project_utils" }
macro_share_permissions = { path = "../macro_share_permissions" }
macro_user_id = { path = "../macro_user_id" }
macro_uuid = { path = "../macro_uuid" }
+properties = { path = "../properties", features = ["ai_tools"] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/document_cognition_service/Cargo.toml` around lines 82 -
84, Reorder the dependency entries in Cargo.toml so they are alphabetical by
crate name: move the properties = { path = "../properties", features =
["ai_tools"] } line to appear after macro_middleware = { ... } and before
notification = { path = "../notification" }; ensure no other ordering or
formatting changes are introduced and keep the macro_middleware default-features
and features block intact.
5ff58ae to
4b1846d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/cloud-storage/properties/src/lib.rs (1)
1-24: 🛠️ Refactor suggestion | 🟠 MajorAdd
#![deny(missing_docs)]to enforce documentation on public items.The crate-level
#![deny(missing_docs)]directive is missing. This should be added to ensure all public items (including the newly addedinboundmodule and re-exported types) are properly documented.📝 Suggested addition
//! Properties crate. //! //! Provides domain logic for property operations following hexagonal architecture. //! //! # Architecture //! //! This crate follows hexagonal architecture: //! - `domain::ports` - Port definitions (traits/interfaces) //! - `domain::service` - Service trait //! - `domain::service_impl` - Service implementation //! - `outbound` - Outbound adapters (e.g., PostgreSQL implementation) + +#![deny(missing_docs)] pub mod domain;As per coding guidelines: "Add
#![deny(missing_docs)]tolib.rsin new crates to enforce documentation on all public items (functions, structs, enums, and modules)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/properties/src/lib.rs` around lines 1 - 24, Add the crate-level attribute `#![deny(missing_docs)]` at the top of lib.rs to enforce documentation for all public items; place it before the module declarations and re-exports so it applies to `pub mod domain`, `pub mod inbound`, `pub mod outbound` and the public re-exports (`PropertiesErr`, `EntityPropertyInfo`, `PropertyOptionInfo`, `NotificationService`, `PermissionService`, `PropertiesRepo`, `PropertiesService`, `PropertiesServiceImpl`, `NotificationServiceImpl`, `PermissionServiceImpl`, `PropertiesPgRepo`), and then add missing doc comments for any public types or modules flagged by the compiler.
♻️ Duplicate comments (1)
rust/cloud-storage/properties/src/inbound/toolset/get_entity_properties.rs (1)
81-84:⚠️ Potential issue | 🟡 MinorAlways serialize
optionsto maintain a consistent response schema.The
#[serde(skip_serializing_if = "Vec::is_empty")]attribute onoptionswill omit the field for non-select properties. If the generated client schema expectsoptionsto always be present, this will cause validation/parsing issues. Consider removing this attribute to always serialize an empty array.Suggested fix
/// Available options for select-type properties. - #[serde(skip_serializing_if = "Vec::is_empty")] pub options: Vec<ToolPropertyOption>,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/properties/src/inbound/toolset/get_entity_properties.rs` around lines 81 - 84, The options field is currently annotated with #[serde(skip_serializing_if = "Vec::is_empty")] which omits the field for non-select properties and breaks the generated client schema; remove that attribute from the options declaration and ensure the field always serializes (and add #[serde(default)] on the same field or the containing struct so missing input deserializes to an empty Vec<ToolPropertyOption>), locating the change on the options: Vec<ToolPropertyOption> declaration in the struct in get_entity_properties.rs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/document_cognition_service/src/api/context/test.rs`:
- Around line 270-277: Build the properties_tool_context before constructing the
memory path and include it in the ToolServiceContext so both tool
initializations get the new properties tools: create PropertiesServiceImpl and
PropertiesToolContext (using PropertiesPgRepo and PermissionServiceImpl) prior
to creating the memory_service, then pass that properties_tool_context into the
ai_tools::all_tools()/memory_service initialization and into the
ToolServiceContext construction so ToolServiceContext includes the new
properties_tool_context rather than omitting it.
In `@rust/cloud-storage/memory/Cargo.toml`:
- Line 44: The memory crate depends on the properties crate but uses
PropertiesToolContext unconditionally (used in context.rs), while that type is
behind the ai_tools feature in properties; update the properties dependency
declaration in memory's Cargo.toml to include features = ["ai_tools"] so the
ai_tools feature is enabled when building memory and PropertiesToolContext is
compiled. Ensure you modify the properties dependency entry (the one currently
listing path = "../properties") to add the features array.
In `@rust/cloud-storage/properties/src/domain/service_impl/mod.rs`:
- Around line 147-158: The toolset path calls service.get_entity_properties(...)
without enforcing authorization (it ignores the request_context) allowing
unauthorized reads; either perform the same check_entity_view_permission(...)
used by the HTTP API inside the toolset inbound handler
(properties::inbound::toolset::get_entity_properties) using the request_context
before calling service.get_entity_properties, or change the service API to
accept caller identity (e.g., user_id) and enforce permissions inside the
service implementation (mod.rs get_entity_properties) so all callers must be
pre-authorized; update the inbound handler to propagate the request_context or
user_id accordingly and return a permission error when
check_entity_view_permission fails.
In `@rust/cloud-storage/properties/src/inbound/toolset/set_entity_property.rs`:
- Around line 17-22: The ToolEntityRef currently cannot represent users because
ToolEntityType lacks a User variant; add a User variant to the ToolEntityType
enum and update the From<ToolEntityType> for EntityType implementation to map
ToolEntityType::User to EntityType::User so that entity_type='user' is supported
(update any serde renaming or docs if present and ensure pattern matches in
places that construct or match on ToolEntityType and in conversions to
EntityType).
---
Outside diff comments:
In `@rust/cloud-storage/properties/src/lib.rs`:
- Around line 1-24: Add the crate-level attribute `#![deny(missing_docs)]` at
the top of lib.rs to enforce documentation for all public items; place it before
the module declarations and re-exports so it applies to `pub mod domain`, `pub
mod inbound`, `pub mod outbound` and the public re-exports (`PropertiesErr`,
`EntityPropertyInfo`, `PropertyOptionInfo`, `NotificationService`,
`PermissionService`, `PropertiesRepo`, `PropertiesService`,
`PropertiesServiceImpl`, `NotificationServiceImpl`, `PermissionServiceImpl`,
`PropertiesPgRepo`), and then add missing doc comments for any public types or
modules flagged by the compiler.
---
Duplicate comments:
In `@rust/cloud-storage/properties/src/inbound/toolset/get_entity_properties.rs`:
- Around line 81-84: The options field is currently annotated with
#[serde(skip_serializing_if = "Vec::is_empty")] which omits the field for
non-select properties and breaks the generated client schema; remove that
attribute from the options declaration and ensure the field always serializes
(and add #[serde(default)] on the same field or the containing struct so missing
input deserializes to an empty Vec<ToolPropertyOption>), locating the change on
the options: Vec<ToolPropertyOption> declaration in the struct in
get_entity_properties.rs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4329346d-958e-4afb-886f-4193d8660ebf
⛔ Files ignored due to path filters (4)
js/app/packages/service-clients/service-cognition/generated/tools/schemas.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/tools/tool.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/tools/types.tsis excluded by!**/generated/**rust/cloud-storage/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
js/app/packages/core/component/AI/component/tool/Properties.tsxjs/app/packages/core/component/AI/component/tool/handler.tsxrust/cloud-storage/ai_tools/Cargo.tomlrust/cloud-storage/ai_tools/src/lib.rsrust/cloud-storage/ai_tools/src/tool_context.rsrust/cloud-storage/document_cognition_service/Cargo.tomlrust/cloud-storage/document_cognition_service/src/api/chats/tool/post.rsrust/cloud-storage/document_cognition_service/src/api/context/mod.rsrust/cloud-storage/document_cognition_service/src/api/context/test.rsrust/cloud-storage/document_cognition_service/src/api/stream/chat_message.rsrust/cloud-storage/document_cognition_service/src/bin/mcp_server.rsrust/cloud-storage/document_cognition_service/src/main.rsrust/cloud-storage/macro_db_client/.sqlx/query-cf71680da3444f573f4cddf66b6269bfe31aeee4bbec34b7e6067a855e28f2b7.jsonrust/cloud-storage/memory/Cargo.tomlrust/cloud-storage/memory/src/context.rsrust/cloud-storage/properties/.sqlx/query-4eef29ad204776f70531f685f709f4e18f2b4f992d73ad2b608b77869a7856a9.jsonrust/cloud-storage/properties/.sqlx/query-8d5ffbdd5be9b1031c7da47bc2d4bcbee35802a575387ef081a09031b42f7de9.jsonrust/cloud-storage/properties/Cargo.tomlrust/cloud-storage/properties/src/domain.rsrust/cloud-storage/properties/src/domain/model.rsrust/cloud-storage/properties/src/domain/ports.rsrust/cloud-storage/properties/src/domain/service.rsrust/cloud-storage/properties/src/domain/service_impl/mod.rsrust/cloud-storage/properties/src/inbound.rsrust/cloud-storage/properties/src/inbound/toolset/get_entity_properties.rsrust/cloud-storage/properties/src/inbound/toolset/mod.rsrust/cloud-storage/properties/src/inbound/toolset/set_entity_property.rsrust/cloud-storage/properties/src/inbound/toolset/test.rsrust/cloud-storage/properties/src/lib.rsrust/cloud-storage/properties/src/outbound.rsrust/cloud-storage/properties/src/outbound/entity_properties_get_query.rsrust/cloud-storage/properties/src/outbound/properties_pg_repo.rs
rust/cloud-storage/document_cognition_service/src/api/context/test.rs
Outdated
Show resolved
Hide resolved
| static_file_service_client = { path = "../static_file_service_client" } | ||
| sync_service_client = { path = "../sync_service_client" } | ||
| soup = { path = "../soup", features = ["all"] } | ||
| properties = { path = "../properties" } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== properties crate manifest =="
fd '^Cargo.toml$' rust/cloud-storage/properties -x sh -c 'echo "--- $1 ---"; sed -n "1,220p" "$1"' sh {}
echo
echo "== ai_tools feature gating / toolset exports in properties crate =="
rg -n --type=rust 'cfg\(feature\s*=\s*"ai_tools"\)|pub mod inbound|pub mod toolset|PropertiesToolContext' rust/cloud-storage/properties
echo
echo "== memory references to properties tool context =="
rg -n --type=rust 'PropertiesToolContext|properties::inbound::toolset|ToolServiceContext' rust/cloud-storage/memoryRepository: macro-inc/macro
Length of output: 4426
🏁 Script executed:
cat -n rust/cloud-storage/memory/Cargo.tomlRepository: macro-inc/macro
Length of output: 2109
Add features = ["ai_tools"] to properties dependency.
The memory crate unconditionally uses properties::inbound::toolset::PropertiesToolContext in src/context.rs:155, but that module is only compiled when the ai_tools feature is enabled in the properties crate.
Fix
-properties = { path = "../properties" }
+properties = { path = "../properties", features = ["ai_tools"] }📝 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
| properties = { path = "../properties" } | |
| properties = { path = "../properties", features = ["ai_tools"] } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/memory/Cargo.toml` at line 44, The memory crate depends on
the properties crate but uses PropertiesToolContext unconditionally (used in
context.rs), while that type is behind the ai_tools feature in properties;
update the properties dependency declaration in memory's Cargo.toml to include
features = ["ai_tools"] so the ai_tools feature is enabled when building memory
and PropertiesToolContext is compiled. Ensure you modify the properties
dependency entry (the one currently listing path = "../properties") to add the
features array.
Comment on lines
+147
to
+158
| #[tracing::instrument(skip(self), fields(entity_id = %entity_id, entity_type = ?entity_type), err)] | ||
| async fn get_entity_properties( | ||
| &self, | ||
| entity_id: &str, | ||
| entity_type: EntityType, | ||
| ) -> Result<Vec<EntityPropertyInfo>, PropertiesErr> { | ||
| Ok(self | ||
| .repository | ||
| .get_entity_properties(entity_id, entity_type) | ||
| .await | ||
| .map_err(anyhow::Error::from)?) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and find the relevant lines
wc -l rust/cloud-storage/properties/src/domain/service_impl/mod.rsRepository: macro-inc/macro
Length of output: 123
🏁 Script executed:
# Read the entire service_impl/mod.rs file to understand context
cat -n rust/cloud-storage/properties/src/domain/service_impl/mod.rsRepository: macro-inc/macro
Length of output: 12730
🏁 Script executed:
# Search for callers of get_entity_properties
rg "get_entity_properties" --type rust -B 2 -A 2Repository: macro-inc/macro
Length of output: 36441
🏁 Script executed:
# Check how set_entity_property uses permission_service
rg "permission_service" rust/cloud-storage/properties/src/ --type rust -B 2 -A 2Repository: macro-inc/macro
Length of output: 10938
🏁 Script executed:
# Look for authorization checks near the AI tool integration
rg "AI\|ai\|read.*permission\|authorize.*read" rust/cloud-storage/properties/src/ --type rust -iRepository: macro-inc/macro
Length of output: 41
🏁 Script executed:
# Read the HTTP API endpoint that calls get_entity_properties
cat -n rust/cloud-storage/properties_service/src/api/properties/entities/get.rs | head -100Repository: macro-inc/macro
Length of output: 4578
🏁 Script executed:
# Look at the toolset integration that calls get_entity_properties
cat -n rust/cloud-storage/properties/src/inbound/toolset/get_entity_properties.rsRepository: macro-inc/macro
Length of output: 8060
🏁 Script executed:
# Check the toolset initialization to understand authorization context
cat -n rust/cloud-storage/properties/src/inbound/toolset/mod.rsRepository: macro-inc/macro
Length of output: 1761
🏁 Script executed:
# Search for where the PropertiesService is instantiated with or without permission_service
rg "PropertiesServiceImpl::new" -B 5 -A 5 --type rustRepository: macro-inc/macro
Length of output: 28418
Add permission checks in the AI toolset before reading properties.
The HTTP API endpoint enforces check_entity_view_permission() before calling the service (lines 87-92 of properties_service/src/api/properties/entities/get.rs). However, the AI toolset integration in get_entity_properties (lines 103-121 of properties/src/inbound/toolset/get_entity_properties.rs) explicitly ignores the request_context at line 110 and calls service.get_entity_properties() without any authorization check.
Since the service method has no user_id parameter and only the write path enforces permission checks, unauthorized read access is possible through the toolset. Either add authorization enforcement in the toolset or require callers to pre-authorize at the framework level.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/properties/src/domain/service_impl/mod.rs` around lines
147 - 158, The toolset path calls service.get_entity_properties(...) without
enforcing authorization (it ignores the request_context) allowing unauthorized
reads; either perform the same check_entity_view_permission(...) used by the
HTTP API inside the toolset inbound handler
(properties::inbound::toolset::get_entity_properties) using the request_context
before calling service.get_entity_properties, or change the service API to
accept caller identity (e.g., user_id) and enforce permissions inside the
service implementation (mod.rs get_entity_properties) so all callers must be
pre-authorized; update the inbound handler to propagate the request_context or
user_id accordingly and return a permission error when
check_entity_view_permission fails.
Comment on lines
+17
to
+22
| #[derive(Debug, Clone, Deserialize, JsonSchema)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct ToolEntityRef { | ||
| pub entity_type: ToolEntityType, | ||
| pub entity_id: String, | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if EntityType has a User variant that ToolEntityType should mirror
ast-grep --pattern 'enum EntityType {
$$$
}'Repository: macro-inc/macro
Length of output: 41
🏁 Script executed:
# First, let's look at the file structure and read the relevant lines
cd rust/cloud-storage/properties && head -50 src/inbound/toolset/set_entity_property.rs | cat -nRepository: macro-inc/macro
Length of output: 3143
🏁 Script executed:
# Find where ToolEntityType is defined
fd -e rs -x grep -l "enum ToolEntityType" {} \;Repository: macro-inc/macro
Length of output: 135
🏁 Script executed:
# Also search for any EntityType enum definition
rg "enum EntityType" -A 10 --type rustRepository: macro-inc/macro
Length of output: 1643
🏁 Script executed:
# Look at the ToolEntityType definition in get_entity_properties.rs
cd rust/cloud-storage/properties && sed -n '1,100p' src/inbound/toolset/get_entity_properties.rs | grep -A 10 "enum ToolEntityType"Repository: macro-inc/macro
Length of output: 243
🏁 Script executed:
# More thorough search for ToolEntityType
rg "enum ToolEntityType" -A 15 rust/cloud-storage/properties/src/Repository: macro-inc/macro
Length of output: 1691
🏁 Script executed:
# Check if EntityType::User exists and see the full From impl
cd rust/cloud-storage/models_properties && cat -n src/shared/entity_type.rs | head -30Repository: macro-inc/macro
Length of output: 1126
🏁 Script executed:
# Check the full From impl for ToolEntityType
rg "impl From<ToolEntityType> for EntityType" -A 20 rust/cloud-storage/properties/src/inbound/toolset/get_entity_properties.rsRepository: macro-inc/macro
Length of output: 1205
ToolEntityRef cannot represent user assignees.
ToolEntityType only has variants: Document, Task, Project, Chat, Thread, Channel—no User variant. The tool description (line 42) documents setting assignees using entity_type='user', but this variant doesn't exist, making task assignee updates via the documented API impossible.
Suggested approach
Add a User variant to ToolEntityType and update the From<ToolEntityType> for EntityType implementation to map it to EntityType::User.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/properties/src/inbound/toolset/set_entity_property.rs`
around lines 17 - 22, The ToolEntityRef currently cannot represent users because
ToolEntityType lacks a User variant; add a User variant to the ToolEntityType
enum and update the From<ToolEntityType> for EntityType implementation to map
ToolEntityType::User to EntityType::User so that entity_type='user' is supported
(update any serde renaming or docs if present and ensure pattern matches in
places that construct or match on ToolEntityType and in conversions to
EntityType).
ehayes2000
force-pushed
the
properties-tool
branch
from
4b1846d to
9ece1a7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/cloud-storage/properties/src/lib.rs (1)
1-1: 🧹 Nitpick | 🔵 TrivialConsider adding
#![deny(missing_docs)]to enforce documentation.Per coding guidelines, new crates should have
#![deny(missing_docs)]inlib.rsto enforce documentation on all public items. This helps maintain documentation quality as the crate grows.♻️ Proposed fix
//! Properties crate. +#![deny(missing_docs)] //! //! Provides domain logic for property operations following hexagonal architecture.As per coding guidelines: "Add
#![deny(missing_docs)]tolib.rsin new crates to enforce documentation on all public items (functions, structs, enums, and modules)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/properties/src/lib.rs` at line 1, Add a crate-level denial for undocumented public items by inserting #![deny(missing_docs)] at the top of the crate root (lib.rs) right after the //! Properties crate. doc comment so the compiler enforces documentation on all public functions, structs, enums and modules in this crate.
♻️ Duplicate comments (8)
rust/cloud-storage/memory/Cargo.toml (1)
44-44:⚠️ Potential issue | 🔴 CriticalMissing
ai_toolsfeature onpropertiesdependency.The
memorycrate usesproperties::inbound::toolset::PropertiesToolContext(per the AI summary), but that module is only compiled when theai_toolsfeature is enabled in thepropertiescrate. This will cause a compilation error.🔧 Proposed fix
-properties = { path = "../properties" } +properties = { path = "../properties", features = ["ai_tools"] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/memory/Cargo.toml` at line 44, The memory crate depends on the properties crate but doesn’t enable the ai_tools feature required for properties::inbound::toolset::PropertiesToolContext; update the dependency entry for properties in the memory crate's Cargo.toml (the properties = { path = "../properties" } line) to enable the ai_tools feature so the PropertiesToolContext module is compiled (e.g., add the appropriate features = ["ai_tools"] attribute to that dependency entry).rust/cloud-storage/macro_db_client/.sqlx/query-cf71680da3444f573f4cddf66b6269bfe31aeee4bbec34b7e6067a855e28f2b7.json (1)
1-15:⚠️ Potential issue | 🔴 CriticalCritical: UPDATE query may affect multiple rows due to incomplete WHERE clause.
The UPDATE statement filters only by
entity_id, but theentity_propertiestable has a composite unique constraint on(entity_id, entity_type, property_definition_id). This means a singleentity_idcan have multiple rows for different entity types and property definitions, and this query would unintentionally update all of them.The WHERE clause needs to include
entity_typeandproperty_definition_idto target the correct single row.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/macro_db_client/.sqlx/query-cf71680da3444f573f4cddf66b6269bfe31aeee4bbec34b7e6067a855e28f2b7.json` around lines 1 - 15, The UPDATE on table entity_properties (query in the JSON with hash cf71680...) currently only filters by entity_id and can update multiple rows; modify the WHERE clause to also filter by entity_type and property_definition_id (the composite key) so the UPDATE targets the single intended row, and update the query parameters accordingly (add the additional parameter placeholders and ensure the calling code that executes this query supplies entity_type and property_definition_id).rust/cloud-storage/properties/src/inbound/toolset/get_entity_properties.rs (2)
81-83:⚠️ Potential issue | 🟠 MajorAlways serialize
optionsto keep the response contract valid.The generated client schema in
js/app/packages/service-clients/service-cognition/generated/tools/schemas.tsrequiresoptionson every property item. Omitting it for non-select properties will make otherwise valid tool responses fail client parsing.Suggested change
- #[serde(skip_serializing_if = "Vec::is_empty")] pub options: Vec<ToolPropertyOption>,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/properties/src/inbound/toolset/get_entity_properties.rs` around lines 81 - 83, The field options on the ToolProperty struct is currently annotated with #[serde(skip_serializing_if = "Vec::is_empty")], causing it to be omitted when empty and breaking the generated client schema; remove that serde attribute so the options: Vec<ToolPropertyOption> field is always serialized (i.e., delete the skip_serializing_if on the options field in get_entity_properties.rs) to preserve the response contract expected by the client.
103-117:⚠️ Potential issue | 🔴 CriticalDo not ignore
RequestContexton the read path.
request_contextis discarded on Line 110, and the current service/repository path only takesentity_idplusentity_type. That leavesGetEntityPropertieswith no authorization hook, so a caller who can guess IDs can read another entity's properties. Thread caller identity intoPropertiesService::get_entity_propertiesand enforce access before the repository call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/properties/src/inbound/toolset/get_entity_properties.rs` around lines 103 - 117, The RequestContext is currently ignored in GetEntityProperties::call which allows unauthorized reads; modify call to pass the request_context (or the relevant caller identity from it) into PropertiesService::get_entity_properties instead of calling get_entity_properties(&self.entity_id, entity_type) and update PropertiesService::get_entity_properties signature to accept RequestContext (or a user identifier) and enforce authorization inside that method before touching the repository; ensure tracing still records request_context.user_id in the instrument fields and propagate the context through ServiceContext<PropertiesToolContext<T>> to perform the access check.rust/cloud-storage/properties/src/inbound/toolset/test.rs (1)
35-35:⚠️ Potential issue | 🟡 MinorAdd
--features ai_toolsto the schema-printer commands.These helpers live under the feature-gated properties toolset, so the documented commands on Lines 35, 43, 51, and 60 will not build/run as written.
Suggested change
-// run `cargo test -p properties inbound::toolset::test::print_get_input_schema -- --nocapture --include-ignored` +// run `cargo test -p properties --features ai_tools inbound::toolset::test::print_get_input_schema -- --nocapture --include-ignored` -// run `cargo test -p properties inbound::toolset::test::print_set_input_schema -- --nocapture --include-ignored` +// run `cargo test -p properties --features ai_tools inbound::toolset::test::print_set_input_schema -- --nocapture --include-ignored` -// run `cargo test -p properties inbound::toolset::test::print_get_output_schema -- --nocapture --include-ignored` +// run `cargo test -p properties --features ai_tools inbound::toolset::test::print_get_output_schema -- --nocapture --include-ignored` -// run `cargo test -p properties inbound::toolset::test::print_set_output_schema -- --nocapture --include-ignored` +// run `cargo test -p properties --features ai_tools inbound::toolset::test::print_set_output_schema -- --nocapture --include-ignored`Also applies to: 43-43, 51-51, 60-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/properties/src/inbound/toolset/test.rs` at line 35, The documented cargo test invocations for the schema-printer tests (e.g., the command for inbound::toolset::test::print_get_input_schema) are missing the feature flag that enables the properties toolset; update each printed command (lines showing the run commands for print_get_input_schema and the other three schema-printer tests) to include --features ai_tools immediately after cargo (or after -p properties) so the feature-gated helpers build and run correctly.rust/cloud-storage/properties/src/inbound/toolset/set_entity_property.rs (2)
17-22:⚠️ Potential issue | 🟠 MajorThe documented assignee payload is unrepresentable.
ToolEntityRef.entity_typereusesToolEntityType, but that enum does not includeUsereven though the schema description tells callers to useentity_type='user'for task assignees. Add aUservariant and map it through toEntityType::User.Also applies to: 41-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/properties/src/inbound/toolset/set_entity_property.rs` around lines 17 - 22, The ToolEntityRef schema is unusable for assignees because ToolEntityType lacks a User variant; add a User variant to the ToolEntityType enum (with appropriate serde casing to match existing camelCase usage) and update the conversion/mapping where ToolEntityType is translated to the internal EntityType to return EntityType::User for the new variant (ensure the From/Into or match arm handling in the conversion function that maps ToolEntityType -> EntityType includes User). Ensure the new variant is covered wherever ToolEntityType is matched/serialized so assignees can be represented.
110-160:⚠️ Potential issue | 🟠 MajorReject ambiguous or empty value payloads instead of taking the first field.
to_set_property_value()silently picks the first populated field and returnsNonewhen nothing is set, even though this tool promises exactly one matching value field. That can ignore caller input or turn an invalid request into an unintended attach/clear operation. Validate the field count and return aToolCallErrorwhen it is not exactly one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/properties/src/inbound/toolset/set_entity_property.rs` around lines 110 - 160, The method SetEntityProperty::to_set_property_value currently returns the first populated field or None, which is ambiguous; change its signature to return Result<SetPropertyValue, ToolCallError> and validate that exactly one of the input fields (boolean_value, date_value, number_value, string_value, option_id, option_ids, entity_ref, entity_refs, link_url, link_urls) is set. Implement logic to count populated fields up front, and if the count != 1 return a ToolCallError with a clear message (e.g., "expected exactly one value field, got N") instead of silently returning None; otherwise construct and return the appropriate SetPropertyValue variant exactly as you already do. Ensure references to types/constructors use SetPropertyValue, EntityReference, EntityType, and ToolCallError so callers can handle the error.rust/cloud-storage/properties/src/outbound/entity_properties_get_query.rs (1)
32-37: 🛠️ Refactor suggestion | 🟠 MajorRecord returned errors on this tracing span.
This function returns
anyhow::Result<_>, but the current attribute will not capture failed outcomes in the span metadata.Suggested change
-#[tracing::instrument(skip(pool))] +#[tracing::instrument(skip(pool), err)]As per coding guidelines, "Include
errwhen adding thetracing::instrumentattribute to functions that returnResult. Do not includeerron functions that returnOption,(), or other non-Resulttypes. Never includelevel = \"info\"".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/cloud-storage/properties/src/outbound/entity_properties_get_query.rs` around lines 32 - 37, The tracing span on get_entity_properties is not recording errors; update the #[tracing::instrument(skip(pool))] attribute to include err (e.g., #[tracing::instrument(skip(pool), err)]) so that failures in get_entity_properties returning anyhow::Result are recorded on the span, and do not add a level parameter (do not set level = "info"); keep skip(pool) and reference the get_entity_properties function and its signature when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/cloud-storage/document_cognition_service/src/main.rs`:
- Around line 273-277: The properties service is being constructed with
Some(NoOpNotificationService) which silently drops task-assignee notifications;
change the wiring so it either supplies the real NotificationService
implementation or passes None to let the early-return debug path run.
Specifically, in the PropertiesServiceImpl::new call (currently creating
PropertiesPgRepo and PermissionServiceImpl alongside
Some(NoOpNotificationService)), replace the NoOp with the real
NotificationServiceImpl (constructed the same way other services are) or use
None; this will ensure handle_task_assignees_property's logic either uses the
real send_notification implementation or returns early rather than silently
dropping notifications.
---
Outside diff comments:
In `@rust/cloud-storage/properties/src/lib.rs`:
- Line 1: Add a crate-level denial for undocumented public items by inserting
#![deny(missing_docs)] at the top of the crate root (lib.rs) right after the //!
Properties crate. doc comment so the compiler enforces documentation on all
public functions, structs, enums and modules in this crate.
---
Duplicate comments:
In
`@rust/cloud-storage/macro_db_client/.sqlx/query-cf71680da3444f573f4cddf66b6269bfe31aeee4bbec34b7e6067a855e28f2b7.json`:
- Around line 1-15: The UPDATE on table entity_properties (query in the JSON
with hash cf71680...) currently only filters by entity_id and can update
multiple rows; modify the WHERE clause to also filter by entity_type and
property_definition_id (the composite key) so the UPDATE targets the single
intended row, and update the query parameters accordingly (add the additional
parameter placeholders and ensure the calling code that executes this query
supplies entity_type and property_definition_id).
In `@rust/cloud-storage/memory/Cargo.toml`:
- Line 44: The memory crate depends on the properties crate but doesn’t enable
the ai_tools feature required for
properties::inbound::toolset::PropertiesToolContext; update the dependency entry
for properties in the memory crate's Cargo.toml (the properties = { path =
"../properties" } line) to enable the ai_tools feature so the
PropertiesToolContext module is compiled (e.g., add the appropriate features =
["ai_tools"] attribute to that dependency entry).
In `@rust/cloud-storage/properties/src/inbound/toolset/get_entity_properties.rs`:
- Around line 81-83: The field options on the ToolProperty struct is currently
annotated with #[serde(skip_serializing_if = "Vec::is_empty")], causing it to be
omitted when empty and breaking the generated client schema; remove that serde
attribute so the options: Vec<ToolPropertyOption> field is always serialized
(i.e., delete the skip_serializing_if on the options field in
get_entity_properties.rs) to preserve the response contract expected by the
client.
- Around line 103-117: The RequestContext is currently ignored in
GetEntityProperties::call which allows unauthorized reads; modify call to pass
the request_context (or the relevant caller identity from it) into
PropertiesService::get_entity_properties instead of calling
get_entity_properties(&self.entity_id, entity_type) and update
PropertiesService::get_entity_properties signature to accept RequestContext (or
a user identifier) and enforce authorization inside that method before touching
the repository; ensure tracing still records request_context.user_id in the
instrument fields and propagate the context through
ServiceContext<PropertiesToolContext<T>> to perform the access check.
In `@rust/cloud-storage/properties/src/inbound/toolset/set_entity_property.rs`:
- Around line 17-22: The ToolEntityRef schema is unusable for assignees because
ToolEntityType lacks a User variant; add a User variant to the ToolEntityType
enum (with appropriate serde casing to match existing camelCase usage) and
update the conversion/mapping where ToolEntityType is translated to the internal
EntityType to return EntityType::User for the new variant (ensure the From/Into
or match arm handling in the conversion function that maps ToolEntityType ->
EntityType includes User). Ensure the new variant is covered wherever
ToolEntityType is matched/serialized so assignees can be represented.
- Around line 110-160: The method SetEntityProperty::to_set_property_value
currently returns the first populated field or None, which is ambiguous; change
its signature to return Result<SetPropertyValue, ToolCallError> and validate
that exactly one of the input fields (boolean_value, date_value, number_value,
string_value, option_id, option_ids, entity_ref, entity_refs, link_url,
link_urls) is set. Implement logic to count populated fields up front, and if
the count != 1 return a ToolCallError with a clear message (e.g., "expected
exactly one value field, got N") instead of silently returning None; otherwise
construct and return the appropriate SetPropertyValue variant exactly as you
already do. Ensure references to types/constructors use SetPropertyValue,
EntityReference, EntityType, and ToolCallError so callers can handle the error.
In `@rust/cloud-storage/properties/src/inbound/toolset/test.rs`:
- Line 35: The documented cargo test invocations for the schema-printer tests
(e.g., the command for inbound::toolset::test::print_get_input_schema) are
missing the feature flag that enables the properties toolset; update each
printed command (lines showing the run commands for print_get_input_schema and
the other three schema-printer tests) to include --features ai_tools immediately
after cargo (or after -p properties) so the feature-gated helpers build and run
correctly.
In `@rust/cloud-storage/properties/src/outbound/entity_properties_get_query.rs`:
- Around line 32-37: The tracing span on get_entity_properties is not recording
errors; update the #[tracing::instrument(skip(pool))] attribute to include err
(e.g., #[tracing::instrument(skip(pool), err)]) so that failures in
get_entity_properties returning anyhow::Result are recorded on the span, and do
not add a level parameter (do not set level = "info"); keep skip(pool) and
reference the get_entity_properties function and its signature when making this
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6890965f-cf01-439a-b805-2ced59f96462
⛔ Files ignored due to path filters (4)
js/app/packages/service-clients/service-cognition/generated/tools/schemas.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/tools/tool.tsis excluded by!**/generated/**js/app/packages/service-clients/service-cognition/generated/tools/types.tsis excluded by!**/generated/**rust/cloud-storage/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
js/app/packages/core/component/AI/component/tool/Properties.tsxjs/app/packages/core/component/AI/component/tool/handler.tsxrust/cloud-storage/ai_tools/Cargo.tomlrust/cloud-storage/ai_tools/src/lib.rsrust/cloud-storage/ai_tools/src/tool_context.rsrust/cloud-storage/document_cognition_service/Cargo.tomlrust/cloud-storage/document_cognition_service/src/api/chats/tool/post.rsrust/cloud-storage/document_cognition_service/src/api/context/mod.rsrust/cloud-storage/document_cognition_service/src/api/context/test.rsrust/cloud-storage/document_cognition_service/src/api/stream/chat_message.rsrust/cloud-storage/document_cognition_service/src/bin/mcp_server.rsrust/cloud-storage/document_cognition_service/src/main.rsrust/cloud-storage/macro_db_client/.sqlx/query-cf71680da3444f573f4cddf66b6269bfe31aeee4bbec34b7e6067a855e28f2b7.jsonrust/cloud-storage/memory/Cargo.tomlrust/cloud-storage/memory/src/context.rsrust/cloud-storage/properties/.sqlx/query-4eef29ad204776f70531f685f709f4e18f2b4f992d73ad2b608b77869a7856a9.jsonrust/cloud-storage/properties/.sqlx/query-8d5ffbdd5be9b1031c7da47bc2d4bcbee35802a575387ef081a09031b42f7de9.jsonrust/cloud-storage/properties/Cargo.tomlrust/cloud-storage/properties/src/domain.rsrust/cloud-storage/properties/src/domain/model.rsrust/cloud-storage/properties/src/domain/ports.rsrust/cloud-storage/properties/src/domain/service.rsrust/cloud-storage/properties/src/domain/service_impl/mod.rsrust/cloud-storage/properties/src/inbound.rsrust/cloud-storage/properties/src/inbound/toolset/get_entity_properties.rsrust/cloud-storage/properties/src/inbound/toolset/mod.rsrust/cloud-storage/properties/src/inbound/toolset/set_entity_property.rsrust/cloud-storage/properties/src/inbound/toolset/test.rsrust/cloud-storage/properties/src/lib.rsrust/cloud-storage/properties/src/outbound.rsrust/cloud-storage/properties/src/outbound/entity_properties_get_query.rsrust/cloud-storage/properties/src/outbound/properties_pg_repo.rs
Comment on lines
+273
to
+277
| let properties_service = properties::PropertiesServiceImpl::new( | ||
| properties::PropertiesPgRepo::new(db.clone()), | ||
| Some(properties::PermissionServiceImpl::new(db.clone())), | ||
| Some(NoOpNotificationService), | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Assignee handling inside properties service ==="
rg -n -C4 'ASSIGNEES_UUID|handle_task_assignees_property|notification_service|send_notification|TaskAssignedMetadata' \
rust/cloud-storage/properties/src --type rust
echo
echo "=== NotificationService implementations in the repo ==="
rg -n -C3 'impl .*NotificationService for' rust/cloud-storage --type rust
echo
echo "=== Tool entrypoints wiring NoOpNotificationService ==="
rg -n -C2 'PropertiesServiceImpl::new|NoOpNotificationService' \
rust/cloud-storage/document_cognition_service/src/main.rs \
rust/cloud-storage/document_cognition_service/src/bin/mcp_server.rs \
rust/cloud-storage/memory/src/context.rsRepository: macro-inc/macro
Length of output: 30365
Don't silently drop task-assignment notifications in the tool path.
PropertiesServiceImpl requires a NotificationService because assignee updates trigger notifications. Wiring Some(NoOpNotificationService) bypasses actual notification delivery while allowing the assignee update to succeed. The same pattern appears in three locations:
rust/cloud-storage/document_cognition_service/src/main.rslines 273–276rust/cloud-storage/document_cognition_service/src/bin/mcp_server.rslines 240–243rust/cloud-storage/memory/src/context.rslines 149–152
The handle_task_assignees_property method (lines 142–146) treats None and Some(NoOp) differently: None returns early with a debug log; Some(NoOp) proceeds to call send_notification on a silent drop. Since a real NotificationServiceImpl exists, either pass it here or pass None and let the debug log surface the limitation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/cloud-storage/document_cognition_service/src/main.rs` around lines 273 -
277, The properties service is being constructed with
Some(NoOpNotificationService) which silently drops task-assignee notifications;
change the wiring so it either supplies the real NotificationService
implementation or passes None to let the early-return debug path run.
Specifically, in the PropertiesServiceImpl::new call (currently creating
PropertiesPgRepo and PermissionServiceImpl alongside
Some(NoOpNotificationService)), replace the NoOp with the real
NotificationServiceImpl (constructed the same way other services are) or use
None; this will ensure handle_task_assignees_property's logic either uses the
real send_notification implementation or returns early rather than silently
dropping notifications.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.