-
Notifications
You must be signed in to change notification settings - Fork 146
feat: add firebender checkpoint support #917
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
9382cf8
65d2ab4
524945f
b3ad021
dd50abe
05232ab
624051b
f8f8ada
c4548fd
b8bbdb6
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 | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,6 +6,7 @@ use crate::{ | |||||||||||||||||||
| error::GitAiError, | ||||||||||||||||||||
| git::repository::find_repository_for_file, | ||||||||||||||||||||
| observability::log_error, | ||||||||||||||||||||
| utils::normalize_to_posix, | ||||||||||||||||||||
| }; | ||||||||||||||||||||
| use chrono::{TimeZone, Utc}; | ||||||||||||||||||||
| use dirs; | ||||||||||||||||||||
|
|
@@ -3648,3 +3649,214 @@ impl AgentCheckpointPreset for AiTabPreset { | |||||||||||||||||||
| }) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Firebender to checkpoint preset | ||||||||||||||||||||
| pub struct FirebenderPreset; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| #[derive(Debug, Deserialize)] | ||||||||||||||||||||
| struct FirebenderHookInput { | ||||||||||||||||||||
| hook_event_name: String, | ||||||||||||||||||||
| model: String, | ||||||||||||||||||||
| repo_working_dir: Option<String>, | ||||||||||||||||||||
| workspace_roots: Option<Vec<String>>, | ||||||||||||||||||||
| tool_name: Option<String>, | ||||||||||||||||||||
| tool_input: Option<serde_json::Value>, | ||||||||||||||||||||
| completion_id: Option<String>, | ||||||||||||||||||||
| dirty_files: Option<HashMap<String, String>>, | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| impl FirebenderPreset { | ||||||||||||||||||||
| fn push_unique_path(paths: &mut Vec<String>, candidate: &str) { | ||||||||||||||||||||
| let trimmed = candidate.trim(); | ||||||||||||||||||||
| if !trimmed.is_empty() && !paths.iter().any(|path| path == trimmed) { | ||||||||||||||||||||
| paths.push(trimmed.to_string()); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| fn normalize_hook_path(raw_path: &str, cwd: &str) -> Option<String> { | ||||||||||||||||||||
| let trimmed = raw_path.trim(); | ||||||||||||||||||||
| if trimmed.is_empty() { | ||||||||||||||||||||
| return None; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let normalized_path = normalize_to_posix(trimmed); | ||||||||||||||||||||
| let normalized_cwd = normalize_to_posix(cwd.trim()) | ||||||||||||||||||||
| .trim_end_matches('/') | ||||||||||||||||||||
| .to_string(); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if normalized_cwd.is_empty() { | ||||||||||||||||||||
| return Some(normalized_path); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let relative = if normalized_path == normalized_cwd { | ||||||||||||||||||||
| String::new() | ||||||||||||||||||||
| } else if let Some(stripped) = normalized_path.strip_prefix(&(normalized_cwd.clone() + "/")) | ||||||||||||||||||||
| { | ||||||||||||||||||||
| stripped.to_string() | ||||||||||||||||||||
| } else { | ||||||||||||||||||||
| normalized_path | ||||||||||||||||||||
| }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Some(relative) | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| fn extract_patch_paths(patch: &str) -> Vec<String> { | ||||||||||||||||||||
| let mut paths = Vec::new(); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| for line in patch.lines() { | ||||||||||||||||||||
| for prefix in [ | ||||||||||||||||||||
| "*** Add File: ", | ||||||||||||||||||||
| "*** Update File: ", | ||||||||||||||||||||
| "*** Delete File: ", | ||||||||||||||||||||
| "*** Move to: ", | ||||||||||||||||||||
| ] { | ||||||||||||||||||||
| if let Some(path) = line.strip_prefix(prefix) { | ||||||||||||||||||||
| Self::push_unique_path(&mut paths, path); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| paths | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Firebender emits multiple real tool_input shapes across editing flows. | ||||||||||||||||||||
| // Normalize direct file fields, structured patch payloads, and raw apply-patch | ||||||||||||||||||||
| // text into a single edited-file list for checkpointing. | ||||||||||||||||||||
| fn extract_file_paths(tool_input: &serde_json::Value) -> Option<Vec<String>> { | ||||||||||||||||||||
| let mut paths = Vec::new(); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| match tool_input { | ||||||||||||||||||||
| serde_json::Value::Object(_) => { | ||||||||||||||||||||
| for key in [ | ||||||||||||||||||||
| "file_path", | ||||||||||||||||||||
| "target_file", | ||||||||||||||||||||
| "relative_workspace_path", | ||||||||||||||||||||
| "path", | ||||||||||||||||||||
| ] { | ||||||||||||||||||||
| if let Some(path) = tool_input.get(key).and_then(|v| v.as_str()) { | ||||||||||||||||||||
| Self::push_unique_path(&mut paths, path); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if let Some(patch) = tool_input.get("patch").and_then(|v| v.as_str()) { | ||||||||||||||||||||
| for path in Self::extract_patch_paths(patch) { | ||||||||||||||||||||
| Self::push_unique_path(&mut paths, &path); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| serde_json::Value::String(raw_patch) => { | ||||||||||||||||||||
| for path in Self::extract_patch_paths(raw_patch) { | ||||||||||||||||||||
| Self::push_unique_path(&mut paths, &path); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| _ => {} | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if paths.is_empty() { None } else { Some(paths) } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| impl AgentCheckpointPreset for FirebenderPreset { | ||||||||||||||||||||
| fn run(&self, flags: AgentCheckpointFlags) -> Result<AgentRunResult, GitAiError> { | ||||||||||||||||||||
| let hook_input_json = flags.hook_input.ok_or_else(|| { | ||||||||||||||||||||
| GitAiError::PresetError("hook_input is required for firebender preset".to_string()) | ||||||||||||||||||||
| })?; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let hook_input: FirebenderHookInput = serde_json::from_str(&hook_input_json) | ||||||||||||||||||||
| .map_err(|e| GitAiError::PresetError(format!("Invalid JSON in hook_input: {}", e)))?; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let FirebenderHookInput { | ||||||||||||||||||||
| hook_event_name, | ||||||||||||||||||||
| model, | ||||||||||||||||||||
| repo_working_dir, | ||||||||||||||||||||
| workspace_roots, | ||||||||||||||||||||
| tool_name, | ||||||||||||||||||||
| tool_input, | ||||||||||||||||||||
| completion_id, | ||||||||||||||||||||
| dirty_files, | ||||||||||||||||||||
| } = hook_input; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if hook_event_name == "beforeSubmitPrompt" || hook_event_name == "afterFileEdit" { | ||||||||||||||||||||
| std::process::exit(0); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
Comment on lines
+3779
to
+3781
Contributor
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. 🔴
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||||||||||||
|
|
||||||||||||||||||||
| if hook_event_name != "preToolUse" && hook_event_name != "postToolUse" { | ||||||||||||||||||||
| return Err(GitAiError::PresetError(format!( | ||||||||||||||||||||
| "Invalid hook_event_name: {}. Expected 'preToolUse' or 'postToolUse'", | ||||||||||||||||||||
| hook_event_name | ||||||||||||||||||||
| ))); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let tool_name = tool_name.unwrap_or_default(); | ||||||||||||||||||||
| // Firebender hooks emit canonical hook tool names rather than raw function names. | ||||||||||||||||||||
| // For example, `apply_patch` and `local_search_replace` both come through as `Edit`. | ||||||||||||||||||||
| if !matches!( | ||||||||||||||||||||
| tool_name.as_str(), | ||||||||||||||||||||
| "Write" | "Edit" | "Delete" | "RenameSymbol" | "DeleteSymbol" | ||||||||||||||||||||
| ) { | ||||||||||||||||||||
| return Err(GitAiError::PresetError(format!( | ||||||||||||||||||||
| "Skipping Firebender hook for non-edit tool_name '{}'.", | ||||||||||||||||||||
| tool_name | ||||||||||||||||||||
| ))); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let repo_working_dir = repo_working_dir | ||||||||||||||||||||
| .map(|s| s.trim().to_string()) | ||||||||||||||||||||
| .filter(|s| !s.is_empty()) | ||||||||||||||||||||
| .or_else(|| workspace_roots.and_then(|roots| roots.into_iter().next())); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let tool_input = tool_input.unwrap_or(serde_json::Value::Null); | ||||||||||||||||||||
| let file_paths = Self::extract_file_paths(&tool_input).map(|paths| { | ||||||||||||||||||||
| if let Some(cwd) = repo_working_dir.as_deref() { | ||||||||||||||||||||
| paths | ||||||||||||||||||||
| .into_iter() | ||||||||||||||||||||
| .filter_map(|path| Self::normalize_hook_path(&path, cwd)) | ||||||||||||||||||||
| .collect::<Vec<String>>() | ||||||||||||||||||||
| } else { | ||||||||||||||||||||
| paths | ||||||||||||||||||||
| } | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let model = { | ||||||||||||||||||||
| let m = model.trim().to_string(); | ||||||||||||||||||||
| if m.is_empty() { | ||||||||||||||||||||
| "unknown".to_string() | ||||||||||||||||||||
| } else { | ||||||||||||||||||||
| m | ||||||||||||||||||||
| } | ||||||||||||||||||||
| }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let agent_id = AgentId { | ||||||||||||||||||||
| tool: "firebender".to_string(), | ||||||||||||||||||||
| id: format!( | ||||||||||||||||||||
| "firebender-{}", | ||||||||||||||||||||
| completion_id.unwrap_or_else(|| Utc::now().timestamp_millis().to_string()) | ||||||||||||||||||||
| ), | ||||||||||||||||||||
| model, | ||||||||||||||||||||
| }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if hook_event_name == "preToolUse" { | ||||||||||||||||||||
| return Ok(AgentRunResult { | ||||||||||||||||||||
| agent_id, | ||||||||||||||||||||
| agent_metadata: None, | ||||||||||||||||||||
| checkpoint_kind: CheckpointKind::Human, | ||||||||||||||||||||
| transcript: None, | ||||||||||||||||||||
| repo_working_dir, | ||||||||||||||||||||
| edited_filepaths: None, | ||||||||||||||||||||
| will_edit_filepaths: file_paths.clone(), | ||||||||||||||||||||
| dirty_files, | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Ok(AgentRunResult { | ||||||||||||||||||||
| agent_id, | ||||||||||||||||||||
| agent_metadata: None, | ||||||||||||||||||||
| checkpoint_kind: CheckpointKind::AiAgent, | ||||||||||||||||||||
| transcript: None, | ||||||||||||||||||||
| repo_working_dir, | ||||||||||||||||||||
| edited_filepaths: file_paths, | ||||||||||||||||||||
| will_edit_filepaths: None, | ||||||||||||||||||||
| dirty_files, | ||||||||||||||||||||
| }) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
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.
🟡
normalize_hook_pathreturnsSome("")(empty string path) when file path equals repo working dirWhen
normalized_path == normalized_cwd(i.e., the tool reports the repo directory itself as a file path),normalize_hook_pathreturnsSome(String::new())at line 3691-3692. This empty string passes through thefilter_mapatagent_presets.rs:3813and ends up inedited_filepathsorwill_edit_filepaths. Downstream consumers of these path lists would encounter an empty string as a file path, which is semantically invalid. The function should returnNonefor this case, similar to how it handles emptytrimmedinput at line 3678-3680.Was this helpful? React with 👍 or 👎 to provide feedback.