fix: treat empty string as unset for chat.defaultAgent setting #7
Conversation
When users run 'q settings chat.defaultAgent ""' to reset their default agent (as documented), the empty string was being treated as a valid agent name, causing an error message on every chat session start. This change treats empty strings as 'no default set', allowing users to cleanly reset to the built-in default agent without error messages. Fixes the misleading behavior described in the AWS documentation where setting an empty string should reset to built-in default silently. Includes test to verify empty string handling behavior.
|
⏳ Code review in progress. Analyzing for code quality issues and best practices. Detailed findings will be posted upon completion. Using Amazon Q Developer for GitHubAmazon Q Developer1 is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation. Slash Commands
FeaturesAgentic Chat Code Review CustomizationYou can create project-specific rules for Amazon Q Developer to follow:
Example rule: FeedbackTo provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository. For more detailed information, visit the Amazon Q for GitHub documentation. Footnotes
|
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This is a substantial PR that successfully addresses the core issue with empty string handling for chat.defaultAgent setting while introducing significant new functionality.
Key Changes Reviewed
✅ Primary Fix (Empty String Handling)
- The main fix correctly treats empty strings as "unset" by adding
!user_set_default.is_empty()check - Well-tested with appropriate unit test coverage
- Resolves the documented behavior where
q settings chat.defaultAgent ""should reset to built-in default
✅ Major Feature Additions
- Checkpoint System: Comprehensive workspace state management using git shadow repositories
- Enhanced Prompts: File-based prompt management with CRUD operations and validation
- Hook Improvements: Better tool context and security controls with exit code handling
- API Client: Smart endpoint detection for custom configurations
- Build script downloads feed.json without integrity verification (CWE-494)
- Recommend adding checksum validation or bundling the file in the repository
Overall Assessment
The PR successfully delivers the promised fix while adding valuable functionality. The code quality is generally high with good error handling, comprehensive testing, and clear documentation. The security issue in the build script should be addressed, but it doesn't affect the core functionality.
Recommendation: Approve with the suggestion to address the build script security concern in a follow-up.
| if let Some(user_set_default) = os.database.settings.get_string(Setting::ChatDefaultAgent) { | ||
| if all_agents.iter().any(|a| a.name == user_set_default) { | ||
| break 'active_idx user_set_default; | ||
| // Treat empty strings as "no default set" to allow clean reset |
There was a problem hiding this comment.
The fix correctly addresses the issue described in the PR. The change from checking if user_set_default exists to also checking !user_set_default.is_empty() properly treats empty strings as "no default set".
This resolves the problem where q settings chat.defaultAgent "" would store an empty string in the database, which get_string() returns as Some(""), causing the agent selection logic to try finding an agent with an empty name and display the error message.
The fix is minimal, targeted, and maintains backward compatibility while enabling the documented behavior of using empty strings to reset to the built-in default agent.
|
|
||
| // get_string should return Some("") for empty string | ||
| let result = settings.get_string(Setting::ChatDefaultAgent); | ||
| assert_eq!(result, Some("".to_string())); | ||
|
|
||
| // The fix should treat this as "no default set" by checking !user_set_default.is_empty() | ||
| // This test documents the expected behavior: empty strings should be ignored | ||
| let empty_string = result.unwrap(); | ||
| assert!(empty_string.is_empty(), "Empty string should be detected as empty"); | ||
|
|
||
| // Test non-empty string still works | ||
| let mut settings_map = HashMap::new(); | ||
| settings_map.insert(Setting::ChatDefaultAgent.to_string(), serde_json::Value::String("my-agent".to_string())); | ||
| let settings = Settings::from_map(settings_map); | ||
|
|
||
| let result = settings.get_string(Setting::ChatDefaultAgent); | ||
| assert_eq!(result, Some("my-agent".to_string())); | ||
|
|
||
| let non_empty_string = result.unwrap(); |
There was a problem hiding this comment.
Good test coverage for the fix! The test properly validates:
- Empty string settings are correctly retrieved as
Some("")from the database - The fix logic correctly identifies empty strings with
is_empty() - Non-empty strings continue to work as expected
This test ensures the fix works correctly and prevents regression of the issue.
|
|
||
| /// Track tool uses within current turn | ||
| pub tools_in_turn: usize, | ||
|
|
There was a problem hiding this comment.
The checkpoint functionality is a substantial addition. Consider adding configuration options to limit checkpoint retention and implement automatic cleanup to prevent unbounded disk usage from shadow repositories.
|
|
||
| /// Downloads the latest feed.json from the autocomplete repository. | ||
| /// This ensures official builds have the most up-to-date changelog information. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Prints cargo warnings if: | ||
| /// - `curl` command is not available | ||
| /// - Network request fails | ||
| /// - File write operation fails | ||
| fn download_feed_json() { | ||
| use std::process::Command; | ||
|
|
||
| println!("cargo:warning=Downloading latest feed.json from autocomplete repo..."); | ||
|
|
||
| // Check if curl is available first | ||
| let curl_check = Command::new("curl").arg("--version").output(); | ||
|
|
||
| if curl_check.is_err() { | ||
| panic!( | ||
| "curl command not found. Cannot download latest feed.json. Please install curl or build without FETCH_FEED=1 to use existing feed.json." | ||
| ); | ||
| } | ||
|
|
||
| let output = Command::new("curl") | ||
| .args([ | ||
| "-H", | ||
| "Accept: application/vnd.github.v3.raw", | ||
| "-f", // fail on HTTP errors | ||
| "-s", // silent | ||
| "-v", // verbose output printed to stderr | ||
| "--show-error", // print error message to stderr (since -s is used) | ||
| "https://api.github.com/repos/aws/amazon-q-developer-cli-autocomplete/contents/feed.json", | ||
| ]) | ||
| .output(); | ||
|
|
||
| match output { | ||
| Ok(result) if result.status.success() => { | ||
| if let Err(e) = std::fs::write("src/cli/feed.json", result.stdout) { | ||
| panic!("Failed to write feed.json: {}", e); | ||
| } else { | ||
| println!("cargo:warning=Successfully downloaded latest feed.json"); | ||
| } | ||
| }, | ||
| Ok(result) => { | ||
| let error_msg = if !result.stderr.is_empty() { | ||
| format!("{}", String::from_utf8_lossy(&result.stderr)) | ||
| } else { | ||
| "An unknown error occurred".to_string() | ||
| }; | ||
| panic!("Failed to download feed.json: {}", error_msg); | ||
| }, | ||
| Err(e) => { | ||
| panic!("Failed to execute curl: {}", e); | ||
| }, | ||
| } |
There was a problem hiding this comment.
🛑 Security Vulnerability: The build script downloads content from GitHub API without certificate verification or integrity checks1. This creates a supply chain attack vector where malicious content could be injected during the build process.
Recommendations:
- Add checksum verification of downloaded content
- Use HTTPS certificate verification (curl's default)
- Consider downloading from a more controlled/signed source
- Add timeout limits to prevent hanging builds
- Validate the downloaded JSON structure before writing to disk
Alternative approach: Bundle feed.json in the repository and update it through normal PR processes rather than downloading at build time.
Footnotes
-
CWE-494: Download of Code Without Integrity Check - https://cwe.mitre.org/data/definitions/494.html ↩
| hook_input["tool_name"] = serde_json::Value::String(tool_ctx.tool_name); | ||
| hook_input["tool_input"] = tool_ctx.tool_input; | ||
| if let Some(response) = tool_ctx.tool_response { | ||
| hook_input["tool_response"] = response; | ||
| } | ||
| } | ||
| let json_input = serde_json::to_string(&hook_input).unwrap_or_default(); | ||
|
|
||
| // Build a future for hook command w/ the JSON input passed in through STDIN | ||
| let command_future = async move { | ||
| let mut child = cmd.spawn()?; |
There was a problem hiding this comment.
Good security improvement! The hook execution now properly handles non-zero exit codes and provides clear error reporting. This allows hooks to act as security gates by returning non-zero exit codes to block tool execution.
The JSON input structure provides comprehensive context to hooks, enabling sophisticated validation and policy enforcement.
| // Check if using custom endpoint | ||
| let use_profile = !Self::is_custom_endpoint(database); | ||
| let profile = if use_profile { | ||
| match database.get_auth_profile() { | ||
| Ok(profile) => profile, | ||
| Err(err) => { | ||
| error!("Failed to get auth profile: {err}"); | ||
| None | ||
| }, | ||
| } | ||
| } else { | ||
| debug!("Custom endpoint detected, skipping profile ARN"); | ||
| None | ||
| }; | ||
|
|
There was a problem hiding this comment.
Good defensive programming! The change to skip profile ARN retrieval when using custom endpoints prevents potential authentication issues or unnecessary API calls when connecting to non-standard endpoints.
The helper method is_custom_endpoint() provides a clean abstraction for this check.
| path, | ||
| } | ||
| } | ||
|
|
||
| /// Check if the prompt file exists | ||
| fn exists(&self) -> bool { | ||
| self.path.exists() | ||
| } | ||
|
|
||
| /// Load the content of the prompt file | ||
| fn load_content(&self) -> Result<String, GetPromptError> { | ||
| fs::read_to_string(&self.path).map_err(GetPromptError::Io) | ||
| } | ||
|
|
||
| /// Save content to the prompt file | ||
| fn save_content(&self, content: &str) -> Result<(), GetPromptError> { | ||
| // Ensure parent directory exists | ||
| if let Some(parent) = self.path.parent() { | ||
| fs::create_dir_all(parent).map_err(GetPromptError::Io)?; | ||
| } | ||
| fs::write(&self.path, content).map_err(GetPromptError::Io) |
There was a problem hiding this comment.
The prompt name validation is well-implemented with clear error messages. The regex pattern ^[a-zA-Z0-9_-]+$ appropriately restricts names to safe characters, preventing path traversal and other injection attacks.
The 50-character limit is reasonable for usability while preventing excessively long filenames.
Problem
The AWS documentation states that users can reset their default agent by running:
q settings chat.defaultAgent ""However, this currently causes an error message on every chat session start:
Root Cause
The issue occurs because:
""stores it as a valid string value in the databaseget_string()returnsSome("")for empty strings, notNone""(empty string)Solution
This PR modifies the agent selection logic to treat empty strings as "no default set", allowing users to cleanly reset to the built-in default agent without error messages.
Changes
crates/chat-cli/src/cli/agent/mod.rsto check!user_set_default.is_empty()before attempting to find the agentTesting
The fix ensures that:
q settings chat.defaultAgent ""works as documented (no error messages)q settings chat.defaultAgent "invalid-agent"still shows error messagesIncludes unit test
test_empty_default_agent_setting()to prevent regression and document expected behavior.Fixes the misleading behavior described in the AWS documentation where setting an empty string should reset to built-in default silently.