cpex prompt hooks - #81
Conversation
Signed-off-by: Madhu Mohan Jaishankar <madhu.mohan.jaishankar@ibm.com>
Signed-off-by: Madhu Mohan Jaishankar <madhu.mohan.jaishankar@ibm.com>
Signed-off-by: Madhu Mohan Jaishankar <madhu.mohan.jaishankar@ibm.com>
Signed-off-by: Madhu Mohan Jaishankar <madhu.mohan.jaishankar@ibm.com>
lucarlig
left a comment
lucarlig
left a comment
There was a problem hiding this comment.
Please fix the prompt hook registration and CMF result mapping, then add the end-to-end coverage and documentation required by #5258. Prompt deny/error attribution should also say prompt rather than tool call.
…ompt messages through CMF Signed-off-by: Madhu Mohan Jaishankar <madhu.mohan.jaishankar@ibm.com>
| content: PromptRequest { | ||
| prompt_request_id: prompt_request_id.to_owned(), | ||
| name: prompt_name.to_owned(), | ||
| arguments: request.arguments.clone().unwrap_or_default().into_iter().collect(), |
There was a problem hiding this comment.
nit:
change to
arguments: request.arguments.clone().map(HashMap::from_iter).unwrap_or_default(),
There was a problem hiding this comment.
changed.
| } | ||
| } | ||
|
|
||
| pub(crate) fn prompt_request_arguments(payload: &MessagePayload) -> Option<Map<String, Value>> { |
There was a problem hiding this comment.
question, why one is using Map and the other HashMap? do we need to make this conversion??
There was a problem hiding this comment.
yes, rmcp has Map and cpex-core has HashMap.
| payload: &MessagePayload, | ||
| ) -> Option<GetPromptResult> { | ||
| let results = payload.message.get_prompt_results(); | ||
| let result = results.first()?; |
There was a problem hiding this comment.
This integration feels strange to be honest.. so perhaps we need better documentation here.
- Why do we check only first result here?
- And it looks like we are ignoring "result.is_error" flag?
- Are we always guaranteed, that CPEX will return ALL messages.
There was a problem hiding this comment.
- fixed.
- fixed -
is_errornow fails the call with the plugin'serror_message. - cpex doesn't guarantee the return of all messages. so we check the count.
|
|
||
| for (message, edited) in original.messages.iter_mut().zip(&result.messages) { | ||
| let projected = cmf_prompt_message(message, &result.prompt_request_id); | ||
| if serde_json::to_value(&projected).ok()? == serde_json::to_value(edited).ok()? { |
There was a problem hiding this comment.
Is this check, just to compare if CPEX has updated the message?
There was a problem hiding this comment.
yes. created contextforge-org/cpex#163 to address this in cpex. if the round trip becomes lossless, this check goes away.
dawid-nowak
left a comment
dawid-nowak
left a comment
There was a problem hiding this comment.
Minor nits and some questions :)
… prompt hooks Signed-off-by: Madhu Mohan Jaishankar <madhu.mohan.jaishankar@ibm.com>
Signed-off-by: Madhu Mohan Jaishankar <madhu.mohan.jaishankar@ibm.com>
3 participants
closes #5258 partially
pre and post hooks for Prompts