-
Notifications
You must be signed in to change notification settings - Fork 12
Apply AI object and context fixes #98
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
Changes from all commits
e8181d1
cb7a426
8656d43
d0b7d31
b09f8d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -44,6 +44,15 @@ pub enum SelectionStrategy { | |||||||
| pub enum ContextItemKind { | ||||||||
| /// A regular file in the repository. | ||||||||
| File, | ||||||||
| /// A URL (web page, API endpoint, etc.). | ||||||||
| Url, | ||||||||
| /// A free-form text snippet (e.g. doc fragment, note). | ||||||||
| Snippet, | ||||||||
| /// Command or terminal output. | ||||||||
| Command, | ||||||||
| /// Image or other binary visual content. | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. API Design: Consider using an enum variant instead of The
Suggested change
Recommendation:
Example: /// Other context item kind with a documented format.
/// Should follow the pattern: "provider:kind" (e.g., "github:issue", "jira:ticket")
Other(String), |
||||||||
| Image, | ||||||||
| Other(String), | ||||||||
| } | ||||||||
|
|
||||||||
| /// Context item describing a single input. | ||||||||
|
|
@@ -52,6 +61,11 @@ pub struct ContextItem { | |||||||
| pub kind: ContextItemKind, | ||||||||
| pub path: String, | ||||||||
| pub content_id: IntegrityHash, | ||||||||
| /// Optional preview/summary of the content (for example, first 200 characters). | ||||||||
| /// Used for display without loading the full content via `content_id`. | ||||||||
| /// Should be kept under 500 characters for performance. | ||||||||
|
||||||||
| /// Should be kept under 500 characters for performance. | |
| /// For performance, it is recommended (but not enforced) to keep this under 500 characters. |
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.
Missing Documentation: What is content_preview for?
The new content_preview field lacks documentation explaining:
- What should be stored here (truncated content? summary?)
- When should it be populated vs left as
None? - What's the expected length/format?
- How does it relate to the actual content referenced by
content_id?
| #[serde(default)] | |
| #[serde(default)] | |
| pub content_preview: Option<String>, |
Recommendation:
/// Optional preview/summary of the content (e.g., first 200 chars).
/// Used for display purposes without loading the full content via content_id.
/// Should be kept under 500 characters for performance.
#[serde(default)]
pub content_preview: Option<String>,| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,169 @@ | ||||||||
| use std::fmt; | ||||||||
|
|
||||||||
| use serde::{Deserialize, Serialize}; | ||||||||
| use uuid::Uuid; | ||||||||
|
|
||||||||
| use crate::{ | ||||||||
| errors::GitError, | ||||||||
| hash::ObjectHash, | ||||||||
| internal::object::{ | ||||||||
| ObjectTrait, | ||||||||
| integrity::IntegrityHash, | ||||||||
| types::{ActorRef, Header, ObjectType}, | ||||||||
| }, | ||||||||
| }; | ||||||||
|
|
||||||||
| #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] | ||||||||
| #[serde(rename_all = "snake_case")] | ||||||||
| pub enum IntentStatus { | ||||||||
| Draft, | ||||||||
| Active, | ||||||||
| Completed, | ||||||||
| Cancelled, | ||||||||
| } | ||||||||
|
|
||||||||
| impl IntentStatus { | ||||||||
| pub fn as_str(&self) -> &'static str { | ||||||||
| match self { | ||||||||
| IntentStatus::Draft => "draft", | ||||||||
| IntentStatus::Active => "active", | ||||||||
| IntentStatus::Completed => "completed", | ||||||||
| IntentStatus::Cancelled => "cancelled", | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| impl fmt::Display for IntentStatus { | ||||||||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||||||||
| write!(f, "{}", self.as_str()) | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||||||||
| pub struct Intent { | ||||||||
| #[serde(flatten)] | ||||||||
| header: Header, | ||||||||
| content: String, | ||||||||
| parent_id: Option<Uuid>, | ||||||||
| root_id: Option<Uuid>, | ||||||||
| task_id: Option<Uuid>, | ||||||||
| result_commit_sha: Option<IntegrityHash>, | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. API Design: Consider builder pattern for complex initialization The let mut intent = Intent::new(repo_id, actor, "content")?;
intent.set_parent_id(Some(parent));
intent.set_task_id(Some(task));
intent.set_status(IntentStatus::Active);Suggestion: Consider a builder pattern for cleaner API: impl Intent {
pub fn builder(repo_id: Uuid, created_by: ActorRef) -> IntentBuilder {
IntentBuilder::new(repo_id, created_by)
}
}
pub struct IntentBuilder { /* ... */ }
impl IntentBuilder {
pub fn content(mut self, content: impl Into<String>) -> Self { /* ... */ }
pub fn parent_id(mut self, id: Uuid) -> Self { /* ... */ }
pub fn task_id(mut self, id: Uuid) -> Self { /* ... */ }
pub fn build(self) -> Result<Intent, String> { /* ... */ }
}
// Usage:
let intent = Intent::builder(repo_id, actor)
.content("Refactor login")
.parent_id(parent_id)
.task_id(task_id)
.build()?;This is optional but improves ergonomics for types with many optional fields. |
||||||||
| status: IntentStatus, | ||||||||
| } | ||||||||
|
|
||||||||
| impl Intent { | ||||||||
| pub fn new( | ||||||||
| repo_id: Uuid, | ||||||||
| created_by: ActorRef, | ||||||||
| content: impl Into<String>, | ||||||||
| ) -> Result<Self, String> { | ||||||||
| Ok(Self { | ||||||||
| header: Header::new(ObjectType::Intent, repo_id, created_by)?, | ||||||||
| content: content.into(), | ||||||||
| parent_id: None, | ||||||||
| root_id: None, | ||||||||
| task_id: None, | ||||||||
| result_commit_sha: None, | ||||||||
| status: IntentStatus::Draft, | ||||||||
| }) | ||||||||
| } | ||||||||
|
|
||||||||
| pub fn header(&self) -> &Header { | ||||||||
| &self.header | ||||||||
| } | ||||||||
|
|
||||||||
| pub fn content(&self) -> &str { | ||||||||
| &self.content | ||||||||
| } | ||||||||
|
|
||||||||
| pub fn parent_id(&self) -> Option<Uuid> { | ||||||||
| self.parent_id | ||||||||
| } | ||||||||
|
|
||||||||
| pub fn root_id(&self) -> Option<Uuid> { | ||||||||
| self.root_id | ||||||||
| } | ||||||||
|
|
||||||||
| pub fn task_id(&self) -> Option<Uuid> { | ||||||||
| self.task_id | ||||||||
| } | ||||||||
|
|
||||||||
| pub fn result_commit_sha(&self) -> Option<&IntegrityHash> { | ||||||||
| self.result_commit_sha.as_ref() | ||||||||
| } | ||||||||
|
|
||||||||
| pub fn status(&self) -> &IntentStatus { | ||||||||
| &self.status | ||||||||
| } | ||||||||
|
|
||||||||
| pub fn set_parent_id(&mut self, parent_id: Option<Uuid>) { | ||||||||
| self.parent_id = parent_id; | ||||||||
| } | ||||||||
|
|
||||||||
| pub fn set_root_id(&mut self, root_id: Option<Uuid>) { | ||||||||
| self.root_id = root_id; | ||||||||
| } | ||||||||
|
|
||||||||
| pub fn set_task_id(&mut self, task_id: Option<Uuid>) { | ||||||||
| self.task_id = task_id; | ||||||||
| } | ||||||||
|
|
||||||||
| pub fn set_result_commit_sha(&mut self, sha: Option<IntegrityHash>) { | ||||||||
| self.result_commit_sha = sha; | ||||||||
| } | ||||||||
|
|
||||||||
| pub fn set_status(&mut self, status: IntentStatus) { | ||||||||
| self.status = status; | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| impl fmt::Display for Intent { | ||||||||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||||||||
| write!(f, "Intent: {}", self.header.object_id()) | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| impl ObjectTrait for Intent { | ||||||||
| fn from_bytes(data: &[u8], _hash: ObjectHash) -> Result<Self, GitError> | ||||||||
| where | ||||||||
| Self: Sized, | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Error Handling: Wrong error variant used The
Suggested change
Recommended fix: serde_json::from_slice(data).map_err(|e| GitError::InvalidIntentObject(e.to_string()))This provides better error specificity and uses the error variant that was specifically added for this type. |
||||||||
| { | ||||||||
| serde_json::from_slice(data).map_err(|e| GitError::InvalidIntentObject(e.to_string())) | ||||||||
| } | ||||||||
|
|
||||||||
| fn get_type(&self) -> ObjectType { | ||||||||
| ObjectType::Intent | ||||||||
| } | ||||||||
|
|
||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security Issue: Avoid The
Suggested change
Recommended fix:
This same issue appears in other object types - consider a holistic fix across the codebase. |
||||||||
| fn get_size(&self) -> usize { | ||||||||
| match serde_json::to_vec(self) { | ||||||||
| Ok(v) => v.len(), | ||||||||
| Err(e) => { | ||||||||
| tracing::warn!("failed to compute Intent size: {}", e); | ||||||||
| 0 | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consistency Issue: Same error handling as The
Suggested change
Recommended fix: serde_json::to_vec(self).map_err(|e| GitError::InvalidIntentObject(e.to_string())) |
||||||||
| fn to_data(&self) -> Result<Vec<u8>, GitError> { | ||||||||
| serde_json::to_vec(self).map_err(|e| GitError::InvalidIntentObject(e.to_string())) | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| #[cfg(test)] | ||||||||
| mod tests { | ||||||||
| use super::*; | ||||||||
|
|
||||||||
| #[test] | ||||||||
| fn test_intent_creation() { | ||||||||
| let repo_id = Uuid::from_u128(0x0123456789abcdef0123456789abcdef); | ||||||||
| let actor = ActorRef::human("jackie").expect("actor"); | ||||||||
| let intent = Intent::new(repo_id, actor, "Refactor login flow").expect("intent"); | ||||||||
|
|
||||||||
| assert_eq!(intent.header().object_type(), &ObjectType::Intent); | ||||||||
| assert_eq!(intent.status(), &IntentStatus::Draft); | ||||||||
| assert!(intent.parent_id().is_none()); | ||||||||
| assert!(intent.root_id().is_none()); | ||||||||
| assert!(intent.task_id().is_none()); | ||||||||
| } | ||||||||
| } | ||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ pub mod context; | |
| pub mod decision; | ||
| pub mod evidence; | ||
| pub mod integrity; | ||
| pub mod intent; | ||
| pub mod note; | ||
| pub mod patchset; | ||
| pub mod plan; | ||
|
|
@@ -58,4 +59,13 @@ pub trait ObjectTrait: Send + Sync + Display { | |
| fn get_size(&self) -> usize; | ||
|
|
||
| fn to_data(&self) -> Result<Vec<u8>, GitError>; | ||
|
|
||
| /// Computes the object hash from serialized data. | ||
| /// | ||
| /// Default implementation serializes the object and computes the hash from that data. | ||
| /// Override only if you need custom hash computation or caching. | ||
| fn object_hash(&self) -> Result<ObjectHash, GitError> { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good Addition: Default This is a nice improvement that reduces boilerplate! All types implementing Minor suggestion: Consider adding a doc comment explaining when this might need to be overridden: /// Computes the object hash from serialized data.
///
/// Default implementation serializes the object and computes hash from the data.
/// Override only if you need custom hash computation or caching.
fn object_hash(&self) -> Result<ObjectHash, GitError> {
let data = self.to_data()?;
Ok(ObjectHash::from_type_and_data(self.get_type(), &data))
} |
||
| let data = self.to_data()?; | ||
| Ok(ObjectHash::from_type_and_data(self.get_type(), &data)) | ||
| } | ||
| } | ||
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.
Test Coverage: Good addition but could be more thorough
The new test validates that
object_hash()matches the storedid, which is good. However, consider testing:Enhancement suggestion: