fix(p3): remediate top audit findings #67
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,10 @@ use std::sync::Arc; | |
| use thiserror::Error; | ||
| use tokio::sync::RwLock; | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Error types | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| #[derive(Error, Debug)] | ||
| pub enum McpError { | ||
| #[error("tool not found: {0}")] | ||
|
|
@@ -19,6 +23,27 @@ pub enum McpError { | |
| InvalidRequest(String), | ||
| } | ||
|
|
||
| impl McpError { | ||
| /// Returns a human-readable recovery hint for this error. | ||
| pub fn recovery_hint(&self) -> &'static str { | ||
| match self { | ||
| McpError::ToolNotFound(_) => { | ||
| "Verify the tool name is correct. Use list_tools() to see available tools." | ||
| } | ||
| McpError::ResourceNotFound(_) => { | ||
| "Verify the resource URI is correct. Use list_resources() to see available resources." | ||
| } | ||
| McpError::InvalidRequest(_) => { | ||
| "Check that the request payload matches the expected schema for this tool/resource." | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Core data types | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| /// Tool definition | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct Tool { | ||
|
|
@@ -49,18 +74,22 @@ pub struct Resource { | |
| pub mime_type: Option<String>, | ||
| } | ||
|
|
||
| #[derive(Clone)] | ||
| pub struct ToolHandler { | ||
| pub tool: Tool, | ||
| pub handler: Arc<dyn Fn(serde_json::Value) -> Result<serde_json::Value> + Send + Sync>, | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // MCP Server | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| /// MCP Server | ||
| pub struct McpServer { | ||
| tools: Arc<RwLock<HashMap<String, ToolHandler>>>, | ||
| resources: Arc<RwLock<HashMap<String, Resource>>>, | ||
| } | ||
|
|
||
| impl McpServer { | ||
| pub fn new() -> Self { | ||
| Self { | ||
|
|
@@ -74,6 +103,7 @@ impl McpServer { | |
| tool: Tool, | ||
| handler: impl Fn(serde_json::Value) -> Result<serde_json::Value> + Send + Sync + 'static, | ||
| ) { | ||
| tracing::debug!(tool = %tool.name, "registering tool"); | ||
| self.tools.write().await.insert( | ||
| tool.name.clone(), | ||
| ToolHandler { | ||
|
|
@@ -84,34 +114,43 @@ impl McpServer { | |
| } | ||
|
|
||
| pub async fn register_resource(&self, resource: Resource) { | ||
| tracing::debug!(resource = %resource.uri, "registering resource"); | ||
| self.resources | ||
| .write() | ||
| .await | ||
| .insert(resource.uri.clone(), resource); | ||
| } | ||
|
|
||
| #[tracing::instrument(skip(self))] | ||
| pub async fn list_tools(&self) -> Vec<Tool> { | ||
| let tools = self | ||
| .tools | ||
| .read() | ||
| .await | ||
| .values() | ||
| .map(|h| h.tool.clone()) | ||
| .collect::<Vec<_>>(); | ||
| tracing::debug!(count = tools.len(), "listing tools"); | ||
| tools | ||
| } | ||
|
|
||
| #[tracing::instrument(skip(self, arguments))] | ||
| pub async fn call_tool( | ||
| &self, | ||
| name: &str, | ||
| arguments: serde_json::Value, | ||
| ) -> Result<ToolResult, McpError> { | ||
| tracing::debug!(tool = %name, "calling tool"); | ||
| let tools = self.tools.read().await; | ||
| let handler = tools.get(name).ok_or_else(|| { | ||
| tracing::warn!(tool = %name, "tool not found"); | ||
| McpError::ToolNotFound(name.to_string()) | ||
| })?; | ||
|
|
||
| let result = | ||
| (handler.handler)(arguments).map_err(|e| McpError::InvalidRequest(e.to_string()))?; | ||
|
|
||
| tracing::info!(tool = %name, "tool called successfully"); | ||
| Ok(ToolResult { | ||
| content: vec![ContentItem { | ||
| content_type: "text".to_string(), | ||
|
|
@@ -121,15 +160,21 @@ impl McpServer { | |
| }) | ||
| } | ||
|
|
||
| #[tracing::instrument(skip(self))] | ||
| pub async fn list_resources(&self) -> Vec<Resource> { | ||
| let resources: Vec<Resource> = self.resources.read().await.values().cloned().collect(); | ||
| tracing::debug!(count = resources.len(), "listing resources"); | ||
| resources | ||
| } | ||
|
|
||
| #[tracing::instrument(skip(self))] | ||
| pub async fn read_resource(&self, uri: &str) -> Result<String, McpError> { | ||
| tracing::debug!(resource = %uri, "reading resource"); | ||
| let resources = self.resources.read().await; | ||
| let resource = resources.get(uri).ok_or_else(|| { | ||
| tracing::warn!(resource = %uri, "resource not found"); | ||
| McpError::ResourceNotFound(uri.to_string()) | ||
| })?; | ||
| Ok(format!("Resource: {}", resource.name)) | ||
| } | ||
| } | ||
|
|
@@ -140,6 +185,10 @@ impl Default for McpServer { | |
| } | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Unit tests | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
@@ -212,4 +261,19 @@ mod tests { | |
| assert!(!result.is_error); | ||
| assert_eq!(result.content[0].text.as_deref(), Some("42")); | ||
| } | ||
|
|
||
| // -- new tests for recovery_hint -------------------------------------- | ||
|
|
||
| #[test] | ||
| fn mcp_error_recovery_hints_are_not_empty() { | ||
| let variants: [McpError; 3] = [ | ||
| McpError::ToolNotFound("x".into()), | ||
| McpError::ResourceNotFound("x".into()), | ||
| McpError::InvalidRequest("x".into()), | ||
| ]; | ||
| for v in &variants { | ||
| let hint = v.recovery_hint(); | ||
| assert!(!hint.is_empty(), "empty recovery hint for {v}"); | ||
| } | ||
| } | ||
|
Comment on lines
+267
to
+278
There was a problem hiding this comment. Suggestion: Add an FR traceability reference for this new test (and align the traceability table) so the test is mapped to a Functional Requirement now that FUNCTIONAL_REQUIREMENTS.md exists. [custom_rule] Severity Level: Minor Why it matters? 🤔FUNCTIONAL_REQUIREMENTS.md exists, and this newly added test does not reference any FR in the diff. The traceability table is still placeholder content, so the test is not mapped to a Functional Requirement as required.
(Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** crates/mcp-server/src/lib.rs
**Line:** 267:278
**Comment:**
*Custom Rule: Add an FR traceability reference for this new test (and align the traceability table) so the test is mapped to a Functional Requirement now that FUNCTIONAL_REQUIREMENTS.md exists.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix |
||
| } | ||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,10 +2,13 @@ | |
| //! | ||
| //! Provides unified interface for embedding generation. | ||
|
|
||
| use serde::{Deserialize, Serialize}; | ||
| use thiserror::Error; | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Error types | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| #[derive(Error, Debug)] | ||
| pub enum EmbeddingError { | ||
| #[error("provider error: {0}")] | ||
|
|
@@ -14,6 +17,24 @@ pub enum EmbeddingError { | |
| InvalidInput(String), | ||
| } | ||
|
|
||
| impl EmbeddingError { | ||
| /// Returns a human-readable recovery hint for this error. | ||
| pub fn recovery_hint(&self) -> &'static str { | ||
| match self { | ||
| EmbeddingError::Provider(_) => { | ||
| "Check that the embedding provider API key is valid and the service is reachable." | ||
| } | ||
| EmbeddingError::InvalidInput(_) => { | ||
| "Ensure the input text is non-empty and within supported length limits." | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+20
to
+31
There was a problem hiding this comment. Suggestion: Add an explicit AgilePlus spec reference (or link to the created spec item) for this newly introduced error-recovery feature before merging. [custom_rule] Severity Level: Minor Why it matters? 🤔This change introduces new error-recovery behavior (
(Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** crates/pheno-embedding/src/lib.rs
**Line:** 20:31
**Comment:**
*Custom Rule: Add an explicit AgilePlus spec reference (or link to the created spec item) for this newly introduced error-recovery feature before merging.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix |
||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Core data types | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| /// Embedding request | ||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| pub struct EmbeddingRequest { | ||
|
|
@@ -34,6 +55,10 @@ pub struct TokenUsage { | |
| pub total_tokens: u32, | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // OpenAI embeddings client | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| /// OpenAI embeddings client | ||
| pub struct OpenAiEmbeddings { | ||
| api_key: String, | ||
|
|
@@ -48,6 +73,7 @@ impl OpenAiEmbeddings { | |
| } | ||
| } | ||
|
|
||
| #[tracing::instrument(skip(self, request))] | ||
| pub async fn embed( | ||
| &self, | ||
| request: &EmbeddingRequest, | ||
|
|
@@ -57,21 +83,41 @@ impl OpenAiEmbeddings { | |
| .clone() | ||
| .unwrap_or_else(|| "text-embedding-3-small".to_string()); | ||
|
|
||
| if request.texts.is_empty() { | ||
| return Err(EmbeddingError::InvalidInput( | ||
| "texts must not be empty".to_string(), | ||
| )); | ||
| } | ||
|
Comment on lines
+86
to
+90
There was a problem hiding this comment. Suggestion: Attach this new request-validation behavior to an AgilePlus work item (or add the missing spec first) so the implementation is not ad hoc. [custom_rule] Severity Level: Minor Why it matters? 🤔This adds new request-validation behavior in the embed flow, but there is no accompanying AgilePlus spec reference or creation path in the changed code. The implementation therefore appears untracked as required by the rule.
(Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** crates/pheno-embedding/src/lib.rs
**Line:** 86:90
**Comment:**
*Custom Rule: Attach this new request-validation behavior to an AgilePlus work item (or add the missing spec first) so the implementation is not ad hoc.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix |
||
|
|
||
| let body = serde_json::json!({ | ||
| "input": request.texts, | ||
| "model": model, | ||
| }); | ||
|
|
||
| tracing::debug!( | ||
| model = %model, | ||
| text_count = request.texts.len(), | ||
| "sending embedding request" | ||
| ); | ||
|
|
||
| let _response = self | ||
| .client | ||
| .post("https://api.openai.com/v1/embeddings") | ||
| .header("Authorization", format!("Bearer {}", self.api_key)) | ||
| .json(&body) | ||
| .send() | ||
| .await | ||
| .map_err(|e| { | ||
| tracing::error!(error = %e, "embedding request failed"); | ||
| EmbeddingError::Provider(e.to_string()) | ||
| })?; | ||
|
Comment on lines
108
to
+113
There was a problem hiding this comment. Suggestion: The embedding call maps only network-level failures and does not treat non-2xx HTTP responses as errors. As written, provider-side failures can be logged as successful embeddings and returned as Severity Level: Critical 🚨- ❌ Embedding HTTP errors misreported as successful zero vectors.
- ⚠️ Clients cannot detect provider outages or authorization failures.Steps of Reproduction ✅1. Downstream code constructs an `EmbeddingRequest` (e.g., similar to the request shapes
exercised in `crates/pheno-embedding/src/lib.rs:138-156`) and calls
`OpenAiEmbeddings::embed(&request)` from application logic.
2. Inside `embed()` (`crates/pheno-embedding/src/lib.rs:68-127`), the function selects a
model, validates that `request.texts` is non-empty, and builds the embeddings request body
at lines 92-95.
3. At lines 103-109, it sends the HTTP request with
`.post("https://api.openai.com/v1/embeddings").json(&body).send().await`, then maps only
transport-level failures into `EmbeddingError::Provider(e.to_string())` at lines 109-113,
binding `_response` but never calling `error_for_status()` or otherwise checking the
returned HTTP status.
4. When the OpenAI embeddings endpoint responds with a non-2xx status (e.g., `401` for
invalid API key or `429` for rate limiting), `send().await` still returns a
`reqwest::Response`; because status validation is omitted, `embed()` logs `"embedding
successful"` at lines 115-118 and returns dummy zero embeddings at lines 121-125, causing
callers to treat failed embeddings as success and lose the ability to react to
provider-side errors.
(Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** crates/pheno-embedding/src/lib.rs
**Line:** 108:113
**Comment:**
*Api Mismatch: The embedding call maps only network-level failures and does not treat non-2xx HTTP responses as errors. As written, provider-side failures can be logged as successful embeddings and returned as `Ok`. Call `error_for_status()` (or equivalent status validation) before reporting success.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix |
||
|
|
||
| tracing::info!( | ||
| model = %model, | ||
| text_count = request.texts.len(), | ||
| "embedding successful" | ||
| ); | ||
|
|
||
| Ok(EmbeddingResponse { | ||
| embeddings: vec![vec![0.0; 1536]; request.texts.len()], | ||
| model, | ||
|
|
@@ -80,6 +126,10 @@ impl OpenAiEmbeddings { | |
| } | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Unit tests | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
@@ -91,7 +141,6 @@ mod tests { | |
| model: Some("text-embedding-3-small".to_string()), | ||
| }; | ||
| let json = serde_json::to_value(&req).unwrap(); | ||
| assert_eq!(json["texts"][0], "hello"); | ||
| assert_eq!(json["model"], "text-embedding-3-small"); | ||
| } | ||
|
|
@@ -144,12 +193,21 @@ mod tests { | |
|
|
||
| #[test] | ||
| fn openai_embeddings_new_constructs_without_panic() { | ||
| let _c = OpenAiEmbeddings::new("sk-test".to_string()); | ||
| let _c2 = OpenAiEmbeddings::new("".to_string()); | ||
| } | ||
|
|
||
| // -- new tests for recovery_hint and empty input check ----------------- | ||
|
|
||
| #[test] | ||
| fn embedding_error_recovery_hints_are_not_empty() { | ||
| let variants: [EmbeddingError; 2] = [ | ||
| EmbeddingError::Provider("x".into()), | ||
| EmbeddingError::InvalidInput("x".into()), | ||
| ]; | ||
| for v in &variants { | ||
| let hint = v.recovery_hint(); | ||
| assert!(!hint.is_empty(), "empty recovery hint for {v}"); | ||
| } | ||
| } | ||
|
Comment on lines
+202
to
212
There was a problem hiding this comment. Suggestion: Add an FR trace reference (for example a Severity Level: Minor Why it matters? 🤔FUNCTIONAL_REQUIREMENTS.md exists in the repository, and this newly added test does not reference any FR identifier or trace comment. The test-traceability rule therefore applies.
(Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** crates/pheno-embedding/src/lib.rs
**Line:** 202:212
**Comment:**
*Custom Rule: Add an FR trace reference (for example a `Traces to: FR-...` comment) to this newly added test to satisfy requirement-to-test traceability.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix |
||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
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.
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.
Suggestion: Add a clear AgilePlus trace (existing item ID or spec creation path) for this newly introduced behavior so the change is visibly linked to tracked work instead of appearing ad hoc. [custom_rule]
Severity Level: Minor⚠️
Why it matters? 🤔
The final file introduces a new recovery_hint API and accompanying behavior, but there is no AgilePlus spec reference or creation/tracking path anywhere in the changed code. This matches the custom rule requiring new feature work to be traced to an AgilePlus item.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖