diff --git a/crates/forge_app/src/fmt/fmt_input.rs b/crates/forge_app/src/fmt/fmt_input.rs index edc463f3ef..f483f1d57e 100644 --- a/crates/forge_app/src/fmt/fmt_input.rs +++ b/crates/forge_app/src/fmt/fmt_input.rs @@ -12,10 +12,10 @@ impl FormatContent for ToolCatalog { match self { ToolCatalog::Read(input) => { let display_path = display_path_for(&input.file_path); - let is_explicit_range = input.start_line.is_some() || input.end_line.is_some(); + let is_explicit_range = input.range.is_some(); let mut subtitle = display_path; - if is_explicit_range { - match (&input.start_line, &input.end_line) { + if is_explicit_range && let Some(range) = &input.range { + match (range.start_line, range.end_line) { (Some(start), Some(end)) => { subtitle.push_str(&format!(":{start}-{end}")); } diff --git a/crates/forge_app/src/fmt/fmt_output.rs b/crates/forge_app/src/fmt/fmt_output.rs index 63866c3169..c2dc15bfa2 100644 --- a/crates/forge_app/src/fmt/fmt_output.rs +++ b/crates/forge_app/src/fmt/fmt_output.rs @@ -88,8 +88,7 @@ mod tests { let fixture = ToolOperation::FsRead { input: forge_domain::FSRead { file_path: "/home/user/test.txt".to_string(), - start_line: None, - end_line: None, + range: None, show_line_numbers: true, }, output: ReadOutput { @@ -111,8 +110,7 @@ mod tests { let fixture = ToolOperation::FsRead { input: forge_domain::FSRead { file_path: "/home/user/test.txt".to_string(), - start_line: Some(2), - end_line: Some(4), + range: Some(forge_domain::FSReadRange { start_line: Some(2), end_line: Some(4) }), show_line_numbers: true, }, output: ReadOutput { diff --git a/crates/forge_app/src/operation.rs b/crates/forge_app/src/operation.rs index 78c8cc42bb..47cda35742 100644 --- a/crates/forge_app/src/operation.rs +++ b/crates/forge_app/src/operation.rs @@ -741,7 +741,7 @@ mod tests { use std::fmt::Write; use std::path::PathBuf; - use forge_domain::{FSRead, FileInfo, ToolValue}; + use forge_domain::{FSRead, FSReadRange, FileInfo, ToolValue}; use super::*; use crate::{Content, Match, MatchResult}; @@ -861,8 +861,7 @@ mod tests { let fixture = ToolOperation::FsRead { input: FSRead { file_path: "/home/user/test.txt".to_string(), - start_line: None, - end_line: None, + range: None, show_line_numbers: true, }, output: ReadOutput { @@ -892,8 +891,7 @@ mod tests { let fixture = ToolOperation::FsRead { input: FSRead { file_path: "/home/user/test.txt".to_string(), - start_line: None, - end_line: None, + range: None, show_line_numbers: true, }, output: ReadOutput { @@ -922,8 +920,7 @@ mod tests { let fixture = ToolOperation::FsRead { input: FSRead { file_path: "/home/user/test.txt".to_string(), - start_line: Some(2), - end_line: Some(3), + range: Some(FSReadRange { start_line: Some(2), end_line: Some(3) }), show_line_numbers: true, }, output: ReadOutput { @@ -953,8 +950,7 @@ mod tests { let fixture = ToolOperation::FsRead { input: FSRead { file_path: "/home/user/large_file.txt".to_string(), - start_line: None, - end_line: None, + range: None, show_line_numbers: true, }, output: ReadOutput { @@ -2621,8 +2617,7 @@ mod tests { let fixture = ToolOperation::FsRead { input: FSRead { file_path: "/home/user/test.png".to_string(), - start_line: None, - end_line: None, + range: None, show_line_numbers: true, }, output: ReadOutput { diff --git a/crates/forge_app/src/tool_executor.rs b/crates/forge_app/src/tool_executor.rs index fee0c2dcec..e409fb4a2c 100644 --- a/crates/forge_app/src/tool_executor.rs +++ b/crates/forge_app/src/tool_executor.rs @@ -160,8 +160,16 @@ impl< .services .read( normalized_path, - input.start_line.map(|i| i as u64), - input.end_line.map(|i| i as u64), + input + .range + .as_ref() + .and_then(|r| r.start_line) + .map(|i| i as u64), + input + .range + .as_ref() + .and_then(|r| r.end_line) + .map(|i| i as u64), ) .await?; diff --git a/crates/forge_domain/src/tools/catalog.rs b/crates/forge_domain/src/tools/catalog.rs index 5814460281..2849ad91c8 100644 --- a/crates/forge_domain/src/tools/catalog.rs +++ b/crates/forge_domain/src/tools/catalog.rs @@ -188,27 +188,35 @@ impl Todo { } } +/// Optional line range for partial file reads. +#[derive(Default, Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq)] +#[schemars(deny_unknown_fields)] +pub struct FSReadRange { + /// 1-based first line. + #[serde(skip_serializing_if = "Option::is_none")] + pub start_line: Option, + + /// Inclusive 1-based last line. + #[serde(skip_serializing_if = "Option::is_none")] + pub end_line: Option, +} + #[derive(Default, Debug, Clone, Serialize, Deserialize, JsonSchema, ToolDescription, PartialEq)] #[tool_description_file = "crates/forge_domain/src/tools/descriptions/fs_read.md"] +#[schemars(deny_unknown_fields)] pub struct FSRead { - /// The absolute path to the file to read + /// Absolute path to the file to read. #[serde(alias = "path")] pub file_path: String, - /// The line number to start reading from starting from 1 not 0. Only - /// provide if the file is too large to read at once + /// Optional line range for partial reads. #[serde(skip_serializing_if = "Option::is_none")] - pub start_line: Option, + pub range: Option, /// If true, prefixes each line with its line index (starting at 1). /// Defaults to true. #[serde(default = "default_true")] pub show_line_numbers: bool, - - /// The line number to stop reading at (inclusive). Only provide if the file - /// is too large to read at once - #[serde(skip_serializing_if = "Option::is_none")] - pub end_line: Option, } #[derive(Default, Debug, Clone, Serialize, Deserialize, JsonSchema, ToolDescription, PartialEq)] @@ -1261,7 +1269,7 @@ mod tests { name: ToolName::new("read"), call_id: None, arguments: ToolCallArguments::from_json( - r#"{"path": "/test/path.rs", "start_line": "10", "end_line": "20"}"#, + r#"{"path": "/test/path.rs", "range": {"start_line": 10, "end_line": 20}}"#, ), thought_signature: None, }; @@ -1276,8 +1284,8 @@ mod tests { if let Ok(ToolCatalog::Read(fs_read)) = actual { assert_eq!(fs_read.file_path, "/test/path.rs"); - assert_eq!(fs_read.start_line, Some(10)); - assert_eq!(fs_read.end_line, Some(20)); + assert_eq!(fs_read.range.as_ref().and_then(|r| r.start_line), Some(10)); + assert_eq!(fs_read.range.as_ref().and_then(|r| r.end_line), Some(20)); } else { panic!("Expected FSRead variant"); } @@ -1292,7 +1300,7 @@ mod tests { name: ToolName::new("read"), call_id: None, arguments: ToolCallArguments::from_json( - r#"{"path": "/test/path.rs", "start_line": 10, "end_line": 20}"#, + r#"{"path": "/test/path.rs", "range": {"start_line": 10, "end_line": 20}}"#, ), thought_signature: None, }; @@ -1306,8 +1314,8 @@ mod tests { if let Ok(ToolCatalog::Read(fs_read)) = actual { assert_eq!(fs_read.file_path, "/test/path.rs"); - assert_eq!(fs_read.start_line, Some(10)); - assert_eq!(fs_read.end_line, Some(20)); + assert_eq!(fs_read.range.as_ref().and_then(|r| r.start_line), Some(10)); + assert_eq!(fs_read.range.as_ref().and_then(|r| r.end_line), Some(20)); } else { panic!("Expected FSRead variant"); } @@ -1322,7 +1330,7 @@ mod tests { name: ToolName::new("Read"), call_id: None, arguments: ToolCallArguments::from_json( - r#"{"path": "/test/path.rs", "start_line": 10, "end_line": 20}"#, + r#"{"path": "/test/path.rs", "range": {"start_line": 10, "end_line": 20}}"#, ), thought_signature: None, }; @@ -1336,8 +1344,8 @@ mod tests { if let Ok(ToolCatalog::Read(fs_read)) = actual { assert_eq!(fs_read.file_path, "/test/path.rs"); - assert_eq!(fs_read.start_line, Some(10)); - assert_eq!(fs_read.end_line, Some(20)); + assert_eq!(fs_read.range.as_ref().and_then(|r| r.start_line), Some(10)); + assert_eq!(fs_read.range.as_ref().and_then(|r| r.end_line), Some(20)); } else { panic!("Expected FSRead variant"); } diff --git a/crates/forge_domain/src/tools/definition/snapshots/forge_domain__tools__definition__usage__tests__tool_usage.snap b/crates/forge_domain/src/tools/definition/snapshots/forge_domain__tools__definition__usage__tests__tool_usage.snap index ee3ed3a2f6..9192079f13 100644 --- a/crates/forge_domain/src/tools/definition/snapshots/forge_domain__tools__definition__usage__tests__tool_usage.snap +++ b/crates/forge_domain/src/tools/definition/snapshots/forge_domain__tools__definition__usage__tests__tool_usage.snap @@ -2,7 +2,7 @@ source: crates/forge_domain/src/tools/definition/usage.rs expression: prompt --- -{"name":"read","description":"Reads a file from the local filesystem. You can access any file directly by using this tool. Assume this tool is able to read all files on the machine. If the User provides a path to a file assume that path is valid. It is okay to read a file that does not exist; an error will be returned.\n\nUsage:\n- The file_path parameter must be an absolute path, not a relative path\n- By default, it reads up to {{config.maxReadSize}} lines starting from the beginning of the file\n- You can optionally specify a line start_line and end_line (especially handy for long files), but it's recommended to read the whole file by not providing these parameters\n- Any lines longer than {{config.maxLineLength}} characters will be truncated\n- Results are returned using rg \"\" -n format, with line numbers starting at 1\n{{#if (contains model.input_modalities \"image\")}}\n- This tool allows Forge Code to read images (eg PNG, JPG, etc). When reading an image file the contents are presented visually.\n- PDFs, Automatically encoded as base64 and sent as visual content for LLM to analyze pages. Any PDFs larger than {{config.maxImageSize}} bytes will return error\n{{/if}}\n- Jupyter notebooks (.ipynb files) are read as plain JSON text - you can parse the cell structure, outputs, and embedded content directly from the JSON\n- This tool can only read files, not directories. To read a directory, use an ls command via the `{{tool_names.shell}}` tool.\n- You can call multiple tools in a single response. It is always better to speculatively read multiple potentially useful files in parallel.","arguments":{"end_line":{"description":"The line number to stop reading at (inclusive). Only provide if the file\nis too large to read at once","type":"integer","is_required":false},"file_path":{"description":"The absolute path to the file to read","type":"string","is_required":true},"show_line_numbers":{"description":"If true, prefixes each line with its line index (starting at 1).\nDefaults to true.","type":"boolean","is_required":false},"start_line":{"description":"The line number to start reading from starting from 1 not 0. Only\nprovide if the file is too large to read at once","type":"integer","is_required":false}}} +{"name":"read","description":"Reads a file from the local filesystem. You can access any file directly by using this tool. Assume this tool is able to read all files on the machine. If the User provides a path to a file assume that path is valid. It is okay to read a file that does not exist; an error will be returned.\n\nUsage:\n- The file_path parameter must be an absolute path, not a relative path\n- By default, it reads up to {{config.maxReadSize}} lines starting from the beginning of the file\n- You can optionally specify a line start_line and end_line (especially handy for long files), but it's recommended to read the whole file by not providing these parameters\n- Any lines longer than {{config.maxLineLength}} characters will be truncated\n- Results are returned using rg \"\" -n format, with line numbers starting at 1\n{{#if (contains model.input_modalities \"image\")}}\n- This tool allows Forge Code to read images (eg PNG, JPG, etc). When reading an image file the contents are presented visually.\n- PDFs, Automatically encoded as base64 and sent as visual content for LLM to analyze pages. Any PDFs larger than {{config.maxImageSize}} bytes will return error\n{{/if}}\n- Jupyter notebooks (.ipynb files) are read as plain JSON text - you can parse the cell structure, outputs, and embedded content directly from the JSON\n- This tool can only read files, not directories. To read a directory, use an ls command via the `{{tool_names.shell}}` tool.\n- You can call multiple tools in a single response. It is always better to speculatively read multiple potentially useful files in parallel.","arguments":{"file_path":{"description":"Absolute path to the file to read.","type":"string","is_required":true},"range":{"description":"Optional line range for partial reads.","type":"object","is_required":false},"show_line_numbers":{"description":"If true, prefixes each line with its line index (starting at 1).\nDefaults to true.","type":"boolean","is_required":false}}} {"name":"write","description":"Writes a file to the local filesystem.\n\nUsage:\n- This tool will overwrite the existing file if there is one at the provided path.\n- If this is an existing file, you MUST use the {{tool_names.read}} tool first to read the file's contents and use this tool with 'overwrite' as true . This tool will fail if you did not read the file first or don't set overwrite parameter to true.\n- ALWAYS prefer {{tool_names.patch}} on existing files in the codebase. NEVER write new files unless explicitly required.\n- NEVER proactively create documentation files (*.md) or README files. Only create documentation files if explicitly requested by the User.\n- Only use emojis if the user explicitly requests it. Avoid writing emojis to files unless asked.","arguments":{"content":{"description":"The content to write to the file","type":"string","is_required":true},"file_path":{"description":"The absolute path to the file to write (must be absolute, not relative)","type":"string","is_required":true},"overwrite":{"description":"If set to true, existing files will be overwritten. If not set and the\nfile exists, an error will be returned with the content of the\nexisting file.","type":"boolean","is_required":false}}} {"name":"fs_search","description":"A powerful search tool built on ripgrep\n\nUsage:\n- ALWAYS use `{{tool_names.fs_search}}` for search tasks. NEVER invoke `grep` or `rg` as a Bash command. The `{{tool_names.fs_search}}` tool has been optimized for correct permissions and access.\n- Supports full regex syntax (e.g., \"log.*Error\", \"function\\\\s+\\\\w+\")\n- Filter files with glob parameter (e.g., \"*.js\", \"**/*.tsx\") or type parameter (e.g., \"js\", \"py\", \"rust\")\n- Output modes: \"content\" shows matching lines, \"files_with_matches\" shows only file paths (default), \"count\" shows match counts\n- Use Task tool for open-ended searches requiring multiple rounds\n- Pattern syntax: Uses ripgrep (not grep) - literal braces need escaping (use `interface\\\\{\\\\}` to find `interface{}` in Go code)\n- Multiline matching: By default patterns match within single lines only. For cross-line patterns like `struct \\\\{[\\\\s\\\\S]*?field`, use `multiline: true`","arguments":{"-A":{"description":"Number of lines to show after each match (rg -A). Requires output_mode:\n\"content\", ignored otherwise.","type":"integer","is_required":false},"-B":{"description":"Number of lines to show before each match (rg -B). Requires output_mode:\n\"content\", ignored otherwise.","type":"integer","is_required":false},"-C":{"description":"Number of lines to show before and after each match (rg -C). Requires\noutput_mode: \"content\", ignored otherwise.","type":"integer","is_required":false},"-i":{"description":"Case insensitive search (rg -i)","type":"boolean","is_required":false},"-n":{"description":"Show line numbers in output (rg -n). Requires output_mode: \"content\",\nignored otherwise.","type":"boolean","is_required":false},"glob":{"description":"Glob pattern to filter files (e.g. \"*.js\", \"*.{ts,tsx}\") - maps to rg\n--glob","type":"string","is_required":false},"head_limit":{"description":"Limit output to first N lines/entries, equivalent to \"| head -N\". Works\nacross all output modes: content (limits output lines),\nfiles_with_matches (limits file paths), count (limits count entries).\nWhen unspecified, shows all results from ripgrep.","type":"integer","is_required":false},"multiline":{"description":"Enable multiline mode where . matches newlines and patterns can span\nlines (rg -U --multiline-dotall). Default: false.","type":"boolean","is_required":false},"offset":{"description":"Skip first N lines/entries before applying head_limit","type":"integer","is_required":false},"output_mode":{"description":"Output mode: \"content\" shows matching lines (supports -A/-B/-C context,\n-n line numbers, head_limit), \"files_with_matches\" shows file paths\n(supports head_limit), \"count\" shows match counts (supports head_limit).\nDefaults to \"files_with_matches\".","type":"string","is_required":false},"path":{"description":"File or directory to search in (rg PATH). Defaults to current working\ndirectory.","type":"string","is_required":false},"pattern":{"description":"The regular expression pattern to search for in file contents.","type":"string","is_required":true},"type":{"description":"File type to search (rg --type). Common types: js, py, rust, go, java,\netc. More efficient than include for standard file types.","type":"string","is_required":false}}} {"name":"sem_search","description":"AI-powered semantic code search. YOUR DEFAULT TOOL for code discovery and exploration when searching within {{env.cwd}}. Use this when you need to find code locations, understand implementations, discover patterns, or explore unfamiliar code - it works with natural language about behavior and concepts, not just keyword matching.\n\n**WHEN TO USE sem_search:**\n- Finding implementation of specific features or algorithms\n- Understanding how a system works across multiple files\n- Discovering architectural patterns and design approaches\n- Locating test examples or fixtures\n- Finding where specific technologies/libraries are used\n- Exploring unfamiliar codebases to learn structure\n- Finding documentation files (README, guides, API docs)\n\n**WHEN NOT TO USE (use {{tool_names.fs_search}} instead):**\n- Searching for exact strings, TODOs, or specific function names\n- Finding all occurrences of a variable or identifier\n- Searching in specific file paths or with regex patterns\n- When you know the exact text to search for\n\nIMPORTANT: Only searches within {{env.cwd}} and subdirectories. For paths outside this scope, use {{tool_names.fs_search}} with path parameter.\n\n**TIPS FOR SUCCESS:**\n- Use 2-3 varied queries to capture different aspects (e.g., \"OAuth token refresh\", \"JWT expiry handling\", \"authentication middleware\")\n- Balance specificity (focused results) with generality (don't miss relevant code)\n- Avoid overly broad queries like \"authentication\" or \"tools\" - be specific about what aspect you need\n- Keep queries targeted - too many broad queries can cause timeouts\n- **Match your intent**: If seeking documentation, use doc-focused keywords (\"setup guide\", \"configuration README\"); if seeking code, use implementation terms (\"token refresh logic\", \"error handling implementation\")\n\nReturns the topK most relevant file:line locations with code context. Each query is ranked independently, then reranked by relevance to your stated intent.","arguments":{"queries":{"description":"List of search queries to execute in parallel. Using multiple queries\n(2-3) with varied phrasings significantly improves results - each query\ncaptures different aspects of what you're looking for. Each query pairs\na search term with a use_case for reranking. Example: for\nauthentication, try \"user login verification\", \"token generation\",\n\"OAuth flow\".","type":"array","is_required":true}}} diff --git a/crates/forge_domain/src/tools/snapshots/forge_domain__tools__catalog__tests__tool_definition_json.snap b/crates/forge_domain/src/tools/snapshots/forge_domain__tools__catalog__tests__tool_definition_json.snap index ea27d14860..25672f24b3 100644 --- a/crates/forge_domain/src/tools/snapshots/forge_domain__tools__catalog__tests__tool_definition_json.snap +++ b/crates/forge_domain/src/tools/snapshots/forge_domain__tools__catalog__tests__tool_definition_json.snap @@ -6,28 +6,37 @@ expression: tools "title": "FSRead", "type": "object", "properties": { - "end_line": { - "description": "The line number to stop reading at (inclusive). Only provide if the file\nis too large to read at once", - "type": "integer", - "format": "int32", - "nullable": true - }, "file_path": { - "description": "The absolute path to the file to read", + "description": "Absolute path to the file to read.", "type": "string" }, + "range": { + "description": "Optional line range for partial reads.", + "type": "object", + "properties": { + "end_line": { + "description": "Inclusive 1-based last line.", + "type": "integer", + "format": "int32", + "nullable": true + }, + "start_line": { + "description": "1-based first line.", + "type": "integer", + "format": "int32", + "nullable": true + } + }, + "additionalProperties": false, + "nullable": true + }, "show_line_numbers": { "description": "If true, prefixes each line with its line index (starting at 1).\nDefaults to true.", "type": "boolean", "default": true - }, - "start_line": { - "description": "The line number to start reading from starting from 1 not 0. Only\nprovide if the file is too large to read at once", - "type": "integer", - "format": "int32", - "nullable": true } }, + "additionalProperties": false, "required": [ "file_path" ] diff --git a/crates/forge_domain/src/transformer/normalize_tool_args.rs b/crates/forge_domain/src/transformer/normalize_tool_args.rs index 1e5492f79e..fd3aaf25af 100644 --- a/crates/forge_domain/src/transformer/normalize_tool_args.rs +++ b/crates/forge_domain/src/transformer/normalize_tool_args.rs @@ -64,7 +64,7 @@ mod tests { call_id: Some(ToolCallId::new("call_001")), // This is what an old dump would have - stringified JSON arguments: ToolCallArguments::from_json( - r#"{"file_path": "/test/path", "start_line": 1}"#, + r#"{"file_path": "/test/path", "range": {"start_line": 1, "end_line": 10}}"#, ), thought_signature: None, }]), @@ -99,7 +99,7 @@ mod tests { match &tool_call.arguments { ToolCallArguments::Parsed(value) => { assert_eq!(value["file_path"], "/test/path"); - assert_eq!(value["start_line"], 1); + assert_eq!(value["range"]["start_line"], 1); } ToolCallArguments::Unparsed(_) => { panic!("Arguments should be Parsed after normalization") @@ -141,7 +141,7 @@ mod tests { call_id: Some(ToolCallId::new("call_001")), arguments: ToolCallArguments::Parsed(json!({ "file_path": "/test/path", - "start_line": 1 + "range": {"start_line": 1, "end_line": 10} })), thought_signature: None, }]), diff --git a/crates/forge_domain/src/transformer/snapshots/forge_domain__transformer__set_model__tests__set_model_only_affects_user_messages.snap b/crates/forge_domain/src/transformer/snapshots/forge_domain__transformer__set_model__tests__set_model_only_affects_user_messages.snap deleted file mode 100644 index bcbb32f112..0000000000 --- a/crates/forge_domain/src/transformer/snapshots/forge_domain__transformer__set_model__tests__set_model_only_affects_user_messages.snap +++ /dev/null @@ -1,28 +0,0 @@ ---- -source: crates/forge_domain/src/transformer/set_model.rs -expression: snapshot ---- -transformation: SetModel(gpt-4)_user_only -before: - messages: - - text: - role: System - content: System message - - text: - role: Assistant - content: Assistant message - - text: - role: User - content: User message -after: - messages: - - text: - role: System - content: System message - - text: - role: Assistant - content: Assistant message - - text: - role: User - content: User message - model: gpt-4 diff --git a/crates/forge_domain/tests/test_stringified_tool_calls.rs b/crates/forge_domain/tests/test_stringified_tool_calls.rs index 662aa56b6a..fd9d39ab2d 100644 --- a/crates/forge_domain/tests/test_stringified_tool_calls.rs +++ b/crates/forge_domain/tests/test_stringified_tool_calls.rs @@ -38,7 +38,7 @@ fn test_stringified_tool_call_arguments_roundtrip() { { "name": "read", "call_id": "call_001", - "arguments": "{\"file_path\": \"/test/path\", \"start_line\": 1, \"end_line\": 10}" + "arguments": "{\"file_path\": \"/test/path\", \"range\": {\"start_line\": 1, \"end_line\": 10}}" } ] } @@ -72,7 +72,7 @@ fn test_stringified_tool_call_arguments_roundtrip() { // Verify arguments are parsed correctly (can access fields) let parsed_args = tool_call.arguments.parse().expect("Should parse arguments"); assert_eq!(parsed_args["file_path"], "/test/path"); - assert_eq!(parsed_args["start_line"], 1); + assert_eq!(parsed_args["range"]["start_line"], 1); // Now serialize the context back to JSON (as we would send to API) let serialized = serde_json::to_string(&context).expect("Should serialize"); @@ -106,8 +106,8 @@ fn test_stringified_tool_call_arguments_roundtrip() { // Verify the values are preserved correctly assert_eq!(args["file_path"], "/test/path"); - assert_eq!(args["start_line"], 1); - assert_eq!(args["end_line"], 10); + assert_eq!(args["range"]["start_line"], 1); + assert_eq!(args["range"]["end_line"], 10); println!("SUCCESS: Stringified arguments properly converted to JSON object"); } @@ -275,7 +275,7 @@ fn test_regular_json_objects_unchanged() { { "name": "read", "call_id": "call_001", - "arguments": {"file_path": "/test/path", "start_line": 1} + "arguments": {"file_path": "/test/path", "range": {"start_line": 1, "end_line": 10}} } ] } @@ -307,7 +307,7 @@ fn test_regular_json_objects_unchanged() { "Regular JSON objects should remain as objects" ); assert_eq!(args["file_path"], "/test/path"); - assert_eq!(args["start_line"], 1); + assert_eq!(args["range"]["start_line"], 1); println!("SUCCESS: Regular JSON objects work correctly"); } diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index 5b9ad64db2..7089fcd3c8 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -2961,6 +2961,11 @@ impl A + Send + Sync> UI }) .collect::>>()?; + let allows_local_api_key = matches!( + provider_id.as_ref().as_ref(), + "ollama" | "vllm" | "lm_studio" | "llama_cpp" | "jan_ai" + ); + // Check if API key is already provided // For Google ADC, we use a marker to skip prompting // For other providers, we use the existing key as a default value (autofill) @@ -2970,6 +2975,19 @@ impl A + Send + Sync> UI // Skip prompting for markers that indicate non-API-key auth if key_str == "google_adc_marker" || key_str == "aws_profile_marker" { key_str.to_string() + } else if allows_local_api_key { + let input = ForgeWidget::input(format!( + "Enter your {provider_id} API key (press Enter to skip)" + )) + .allow_empty(true); + let api_key = input.prompt()?.context("API key input cancelled")?; + let api_key_str = api_key.trim(); + + if api_key_str.is_empty() { + "local".to_string() + } else { + api_key_str.to_string() + } } else { // For other providers, show the existing key as default (autofill) let input = ForgeWidget::input(format!("Enter your {provider_id} API key")) @@ -2979,6 +2997,19 @@ impl A + Send + Sync> UI anyhow::ensure!(!api_key_str.is_empty(), "API key cannot be empty"); api_key_str.to_string() } + } else if allows_local_api_key { + let input = ForgeWidget::input(format!( + "Enter your {provider_id} API key (press Enter to skip)" + )) + .allow_empty(true); + let api_key = input.prompt()?.context("API key input cancelled")?; + let api_key_str = api_key.trim(); + + if api_key_str.is_empty() { + "local".to_string() + } else { + api_key_str.to_string() + } } else { // Prompt for API key input (no existing key) let input = ForgeWidget::input(format!("Enter your {provider_id} API key")); diff --git a/crates/forge_repo/src/provider/openai_responses/snapshots/forge_repo__provider__openai_responses__request__tests__openai_responses_all_catalog_tools.snap b/crates/forge_repo/src/provider/openai_responses/snapshots/forge_repo__provider__openai_responses__request__tests__openai_responses_all_catalog_tools.snap index 1d944b3324..05b5451959 100644 --- a/crates/forge_repo/src/provider/openai_responses/snapshots/forge_repo__provider__openai_responses__request__tests__openai_responses_all_catalog_tools.snap +++ b/crates/forge_repo/src/provider/openai_responses/snapshots/forge_repo__provider__openai_responses__request__tests__openai_responses_all_catalog_tools.snap @@ -9,43 +9,60 @@ expression: actual.tools "parameters": { "additionalProperties": false, "properties": { - "end_line": { + "file_path": { + "description": "Absolute path to the file to read.", + "type": "string" + }, + "range": { "anyOf": [ { - "type": "integer" + "additionalProperties": false, + "properties": { + "end_line": { + "anyOf": [ + { + "type": "integer" + }, + { + "type": "null" + } + ], + "description": "Inclusive 1-based last line." + }, + "start_line": { + "anyOf": [ + { + "type": "integer" + }, + { + "type": "null" + } + ], + "description": "1-based first line." + } + }, + "required": [ + "end_line", + "start_line" + ], + "type": "object" }, { "type": "null" } ], - "description": "The line number to stop reading at (inclusive). Only provide if the file\nis too large to read at once" - }, - "file_path": { - "description": "The absolute path to the file to read", - "type": "string" + "description": "Optional line range for partial reads." }, "show_line_numbers": { "default": true, "description": "If true, prefixes each line with its line index (starting at 1).\nDefaults to true.", "type": "boolean" - }, - "start_line": { - "anyOf": [ - { - "type": "integer" - }, - { - "type": "null" - } - ], - "description": "The line number to start reading from starting from 1 not 0. Only\nprovide if the file is too large to read at once" } }, "required": [ - "end_line", "file_path", - "show_line_numbers", - "start_line" + "range", + "show_line_numbers" ], "title": "FSRead", "type": "object" diff --git a/crates/forge_repo/src/provider/provider.json b/crates/forge_repo/src/provider/provider.json index b67e1d574a..68dddbf52f 100644 --- a/crates/forge_repo/src/provider/provider.json +++ b/crates/forge_repo/src/provider/provider.json @@ -1212,46 +1212,66 @@ { "id": "llama_cpp", "api_key_vars": "LLAMA_CPP_API_KEY", - "url_param_vars": ["LLAMA_CPP_URL", "LLAMA_CPP_PORT"], + "url_param_vars": [ + {"name": "LLAMA_CPP_SSL_SCHEME", "options": ["http", "https"]}, + "LLAMA_CPP_HOST", + "LLAMA_CPP_PORT" + ], "response_type": "OpenAI", - "url": "{{LLAMA_CPP_URL}}:{{LLAMA_CPP_PORT}}/v1/chat/completions", - "models": "{{LLAMA_CPP_URL}}:{{LLAMA_CPP_PORT}}/v1/models", + "url": "{{LLAMA_CPP_SSL_SCHEME}}://{{LLAMA_CPP_HOST}}:{{LLAMA_CPP_PORT}}/v1/chat/completions", + "models": "{{LLAMA_CPP_SSL_SCHEME}}://{{LLAMA_CPP_HOST}}:{{LLAMA_CPP_PORT}}/v1/models", "auth_methods": ["api_key"] }, { "id": "vllm", "api_key_vars": "VLLM_API_KEY", - "url_param_vars": ["VLLM_URL", "VLLM_PORT"], + "url_param_vars": [ + {"name": "VLLM_SSL_SCHEME", "options": ["http", "https"]}, + "VLLM_HOST", + "VLLM_PORT" + ], "response_type": "OpenAI", - "url": "{{VLLM_URL}}:{{VLLM_PORT}}/v1/chat/completions", - "models": "{{VLLM_URL}}:{{VLLM_PORT}}/v1/models", + "url": "{{VLLM_SSL_SCHEME}}://{{VLLM_HOST}}:{{VLLM_PORT}}/v1/chat/completions", + "models": "{{VLLM_SSL_SCHEME}}://{{VLLM_HOST}}:{{VLLM_PORT}}/v1/models", "auth_methods": ["api_key"] }, { "id": "jan_ai", "api_key_vars": "JAN_AI_API_KEY", - "url_param_vars": ["JAN_AI_URL", "JAN_AI_PORT"], + "url_param_vars": [ + {"name": "JAN_AI_SSL_SCHEME", "options": ["http", "https"]}, + "JAN_AI_HOST", + "JAN_AI_PORT" + ], "response_type": "OpenAI", - "url": "{{JAN_AI_URL}}:{{JAN_AI_PORT}}/v1/chat/completions", - "models": "{{JAN_AI_URL}}:{{JAN_AI_PORT}}/v1/models", + "url": "{{JAN_AI_SSL_SCHEME}}://{{JAN_AI_HOST}}:{{JAN_AI_PORT}}/v1/chat/completions", + "models": "{{JAN_AI_SSL_SCHEME}}://{{JAN_AI_HOST}}:{{JAN_AI_PORT}}/v1/models", "auth_methods": ["api_key"] }, { "id": "ollama", "api_key_vars": "OLLAMA_API_KEY", - "url_param_vars": ["OLLAMA_URL", "OLLAMA_PORT"], + "url_param_vars": [ + {"name": "OLLAMA_SSL_SCHEME", "options": ["http", "https"]}, + "OLLAMA_HOST", + "OLLAMA_PORT" + ], "response_type": "OpenAI", - "url": "{{OLLAMA_URL}}:{{OLLAMA_PORT}}/v1/chat/completions", - "models": "{{OLLAMA_URL}}:{{OLLAMA_PORT}}/v1/models", + "url": "{{OLLAMA_SSL_SCHEME}}://{{OLLAMA_HOST}}:{{OLLAMA_PORT}}/v1/chat/completions", + "models": "{{OLLAMA_SSL_SCHEME}}://{{OLLAMA_HOST}}:{{OLLAMA_PORT}}/v1/models", "auth_methods": ["api_key"] }, { "id": "lm_studio", "api_key_vars": "LM_STUDIO_API_KEY", - "url_param_vars": ["LM_STUDIO_URL", "LM_STUDIO_PORT"], + "url_param_vars": [ + {"name": "LM_STUDIO_SSL_SCHEME", "options": ["http", "https"]}, + "LM_STUDIO_HOST", + "LM_STUDIO_PORT" + ], "response_type": "OpenAIResponses", - "url": "{{LM_STUDIO_URL}}:{{LM_STUDIO_PORT}}/v1/responses", - "models": "{{LM_STUDIO_URL}}:{{LM_STUDIO_PORT}}/v1/models", + "url": "{{LM_STUDIO_SSL_SCHEME}}://{{LM_STUDIO_HOST}}:{{LM_STUDIO_PORT}}/v1/responses", + "models": "{{LM_STUDIO_SSL_SCHEME}}://{{LM_STUDIO_HOST}}:{{LM_STUDIO_PORT}}/v1/models", "auth_methods": ["api_key"] }, { diff --git a/crates/forge_repo/src/provider/provider_repo.rs b/crates/forge_repo/src/provider/provider_repo.rs index 9253f668d6..098ee4ad6c 100644 --- a/crates/forge_repo/src/provider/provider_repo.rs +++ b/crates/forge_repo/src/provider/provider_repo.rs @@ -79,6 +79,33 @@ struct ProviderConfig { custom_headers: Option>, } +/// Maps new environment variable names to their legacy fallback names. +/// This enables backward compatibility when renaming env vars (e.g. OLLAMA_URL +/// → OLLAMA_HOST). +fn legacy_env_var_fallback(new_name: &str) -> Option<&'static str> { + match new_name { + "OLLAMA_HOST" => Some("OLLAMA_URL"), + "VLLM_HOST" => Some("VLLM_URL"), + "LM_STUDIO_HOST" => Some("LM_STUDIO_URL"), + "LLAMA_CPP_HOST" => Some("LLAMA_CPP_URL"), + "JAN_AI_HOST" => Some("JAN_AI_URL"), + _ => None, + } +} + +/// Returns the default value for URL parameters that should be optional during +/// environment migration. +fn default_url_param_value(name: &str) -> Option<&'static str> { + match name { + "OLLAMA_SSL_SCHEME" + | "VLLM_SSL_SCHEME" + | "LM_STUDIO_SSL_SCHEME" + | "LLAMA_CPP_SSL_SCHEME" + | "JAN_AI_SSL_SCHEME" => Some("http"), + _ => None, + } +} + fn overwrite(base: &mut T, other: T) { *base = other; } @@ -351,8 +378,20 @@ impl< for env_var in &config.url_param_vars { let name = env_var.param_name(); - if let Some(value) = self.infra.get_env_var(name) { + let value = self + .infra + .get_env_var(name) + // Fall back to legacy env var name for backward compatibility + .or_else(|| { + legacy_env_var_fallback(name).and_then(|legacy| self.infra.get_env_var(legacy)) + }); + if let Some(value) = value { url_params.insert(URLParam::from(name.to_string()), URLParamValue::from(value)); + } else if let Some(value) = default_url_param_value(name) { + url_params.insert( + URLParam::from(name.to_string()), + URLParamValue::from(value.to_string()), + ); } else { return Err(Error::env_var_not_found(config.id.clone(), name).into()); } @@ -1292,6 +1331,193 @@ mod env_tests { ); } + #[tokio::test] + async fn test_migration_ollama_with_new_env_var() { + // New users use OLLAMA_HOST + let mut env_vars = HashMap::new(); + env_vars.insert("OLLAMA_API_KEY".to_string(), "ollama-key".to_string()); + env_vars.insert("OLLAMA_HOST".to_string(), "http://localhost".to_string()); + env_vars.insert("OLLAMA_PORT".to_string(), "11434".to_string()); + + let infra = Arc::new(MockInfra::new(env_vars)); + let registry = ForgeProviderRepository::new(infra.clone()); + + registry.migrate_env_to_file().await.unwrap(); + + let credentials = infra.credentials.lock().await; + let creds = credentials.as_ref().unwrap(); + + let ollama_id = ProviderId::from("ollama".to_string()); + let ollama_cred = creds.iter().find(|c| c.id == ollama_id).unwrap(); + assert_eq!( + ollama_cred + .url_params + .get(&URLParam::from("OLLAMA_SSL_SCHEME".to_string())) + .map(|v| v.as_str()), + Some("http") + ); + assert_eq!( + ollama_cred + .url_params + .get(&URLParam::from("OLLAMA_HOST".to_string())) + .map(|v| v.as_str()), + Some("http://localhost") + ); + assert_eq!( + ollama_cred + .url_params + .get(&URLParam::from("OLLAMA_PORT".to_string())) + .map(|v| v.as_str()), + Some("11434") + ); + } + + #[tokio::test] + async fn test_migration_ollama_with_legacy_env_var() { + // Old users still use OLLAMA_URL (backward compat) + let mut env_vars = HashMap::new(); + env_vars.insert("OLLAMA_API_KEY".to_string(), "ollama-key".to_string()); + env_vars.insert("OLLAMA_URL".to_string(), "http://localhost".to_string()); + env_vars.insert("OLLAMA_PORT".to_string(), "11434".to_string()); + + let infra = Arc::new(MockInfra::new(env_vars)); + let registry = ForgeProviderRepository::new(infra.clone()); + + registry.migrate_env_to_file().await.unwrap(); + + let credentials = infra.credentials.lock().await; + let creds = credentials.as_ref().unwrap(); + + let ollama_id = ProviderId::from("ollama".to_string()); + let ollama_cred = creds.iter().find(|c| c.id == ollama_id).unwrap(); + // Should still be stored under OLLAMA_HOST key (the new param name) + assert_eq!( + ollama_cred + .url_params + .get(&URLParam::from("OLLAMA_HOST".to_string())) + .map(|v| v.as_str()), + Some("http://localhost") + ); + } + + #[tokio::test] + async fn test_migration_ollama_new_env_var_takes_precedence_over_legacy() { + // If both OLLAMA_HOST and OLLAMA_URL are set, OLLAMA_HOST wins + let mut env_vars = HashMap::new(); + env_vars.insert("OLLAMA_API_KEY".to_string(), "ollama-key".to_string()); + env_vars.insert("OLLAMA_HOST".to_string(), "http://new-host".to_string()); + env_vars.insert("OLLAMA_URL".to_string(), "http://old-host".to_string()); + env_vars.insert("OLLAMA_PORT".to_string(), "11434".to_string()); + + let infra = Arc::new(MockInfra::new(env_vars)); + let registry = ForgeProviderRepository::new(infra.clone()); + + registry.migrate_env_to_file().await.unwrap(); + + let credentials = infra.credentials.lock().await; + let creds = credentials.as_ref().unwrap(); + + let ollama_id = ProviderId::from("ollama".to_string()); + let ollama_cred = creds.iter().find(|c| c.id == ollama_id).unwrap(); + assert_eq!( + ollama_cred + .url_params + .get(&URLParam::from("OLLAMA_HOST".to_string())) + .map(|v| v.as_str()), + Some("http://new-host") + ); + } + + #[tokio::test] + async fn test_migration_vllm_with_legacy_env_var() { + let mut env_vars = HashMap::new(); + env_vars.insert("VLLM_API_KEY".to_string(), "vllm-key".to_string()); + env_vars.insert("VLLM_URL".to_string(), "http://vllm-host".to_string()); + env_vars.insert("VLLM_PORT".to_string(), "8000".to_string()); + + let infra = Arc::new(MockInfra::new(env_vars)); + let registry = ForgeProviderRepository::new(infra.clone()); + + registry.migrate_env_to_file().await.unwrap(); + + let credentials = infra.credentials.lock().await; + let creds = credentials.as_ref().unwrap(); + + let vllm_id = ProviderId::from("vllm".to_string()); + let vllm_cred = creds.iter().find(|c| c.id == vllm_id).unwrap(); + assert_eq!( + vllm_cred + .url_params + .get(&URLParam::from("VLLM_HOST".to_string())) + .map(|v| v.as_str()), + Some("http://vllm-host") + ); + } + + #[tokio::test] + async fn test_migration_lm_studio_with_legacy_env_var() { + let mut env_vars = HashMap::new(); + env_vars.insert("LM_STUDIO_API_KEY".to_string(), "lm-key".to_string()); + env_vars.insert("LM_STUDIO_URL".to_string(), "http://lm-host".to_string()); + env_vars.insert("LM_STUDIO_PORT".to_string(), "1234".to_string()); + + let infra = Arc::new(MockInfra::new(env_vars)); + let registry = ForgeProviderRepository::new(infra.clone()); + + registry.migrate_env_to_file().await.unwrap(); + + let credentials = infra.credentials.lock().await; + let creds = credentials.as_ref().unwrap(); + + let cred_id = ProviderId::from("lm_studio".to_string()); + let cred = creds.iter().find(|c| c.id == cred_id).unwrap(); + assert_eq!( + cred.url_params + .get(&URLParam::from("LM_STUDIO_HOST".to_string())) + .map(|v| v.as_str()), + Some("http://lm-host") + ); + } + + #[tokio::test] + async fn test_ollama_config_uses_new_host_param() { + let configs = get_provider_configs(); + let ollama_id = ProviderId::from("ollama".to_string()); + let config = configs.iter().find(|c| c.id == ollama_id).unwrap(); + assert_eq!( + config + .url_param_vars + .iter() + .map(|v| v.param_name()) + .collect::>(), + vec!["OLLAMA_SSL_SCHEME", "OLLAMA_HOST", "OLLAMA_PORT"] + ); + assert!(config.url.contains("{{OLLAMA_SSL_SCHEME}}://")); + assert!(config.url.contains("{{OLLAMA_HOST}}")); + assert!(!config.url.contains("{{OLLAMA_URL}}")); + } + + #[tokio::test] + async fn test_ollama_ssl_scheme_config_uses_options() { + let configs = get_provider_configs(); + let ollama_id = ProviderId::from("ollama".to_string()); + let config = configs.iter().find(|c| c.id == ollama_id).unwrap(); + let ssl_scheme = config + .url_param_vars + .iter() + .find(|v| v.param_name() == "OLLAMA_SSL_SCHEME") + .unwrap() + .clone() + .into_spec(); + assert_eq!( + ssl_scheme, + URLParamSpec::with_options( + URLParam::from("OLLAMA_SSL_SCHEME".to_string()), + vec!["http".to_string(), "https".to_string()] + ) + ); + } + #[tokio::test] async fn test_merge_base_provider_configs() { use std::io::Write;