-
Notifications
You must be signed in to change notification settings - Fork 758
fix(routines): resolve message tool channel/target from per-job metadata #708
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
77a4afd
97ffa3f
547d345
0487fcb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -488,18 +488,16 @@ async fn execute_full_job( | |||||||||||||||||
| reason: "scheduler not available".to_string(), | ||||||||||||||||||
| })?; | ||||||||||||||||||
|
|
||||||||||||||||||
| // Set the message tool's default channel/target from the routine's notify config | ||||||||||||||||||
| // so the LLM can send results without triggering cross-channel approval. | ||||||||||||||||||
| // TODO: This mutates shared global state and can race with concurrent jobs. | ||||||||||||||||||
| // Move notify config into JobContext metadata and apply per-job instead. | ||||||||||||||||||
| // Notify config is carried in job metadata (see above) and read by | ||||||||||||||||||
| // MessageTool::execute from JobContext — no global state mutation needed. | ||||||||||||||||||
|
|
||||||||||||||||||
| let mut metadata = serde_json::json!({ "max_iterations": max_iterations }); | ||||||||||||||||||
| // Carry the routine's notify config in job metadata so the message tool | ||||||||||||||||||
| // can resolve channel/target per-job without global state mutation. | ||||||||||||||||||
| if let Some(channel) = &routine.notify.channel { | ||||||||||||||||||
| scheduler | ||||||||||||||||||
| .tools() | ||||||||||||||||||
| .set_message_tool_context(Some(channel.clone()), Some(routine.notify.user.clone())) | ||||||||||||||||||
| .await; | ||||||||||||||||||
| metadata["notify_channel"] = serde_json::json!(channel); | ||||||||||||||||||
|
||||||||||||||||||
| metadata["notify_channel"] = serde_json::json!(channel); | |
| metadata["notify_channel"] = serde_json::json!(channel); | |
| } else { | |
| // When no explicit channel is configured, write an empty string so | |
| // MessageTool::execute can treat this as "no specific channel" (e.g., | |
| // broadcast or default-channel behavior) instead of seeing the key | |
| // as entirely absent and reporting "No channel specified". | |
| metadata["notify_channel"] = serde_json::json!(""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment "Notify config is carried in job metadata (see above) and read by MessageTool::execute from JobContext" references "see above" but there is nothing above this function explaining this mechanism. The metadata population happens immediately below (lines 494-500), not above. The comment is self-contradictory: it says the config is carried in metadata, then immediately populates the metadata. The "see above" reference should be removed or replaced with "see below".