fix: treat empty string as unset for chat.defaultAgent setting #5
Conversation
…generate (aws#2741) * fixing bugs * formatting * fix: CTRL+C handling during multi-select, auto completion for /agent generate * set use legacy mcp config to false --------- Co-authored-by: Xian Wu <xianwwu@amazon.com>
- Changed from ~/.q/knowledge_bases/ to ~/.aws/amazonq/knowledge_bases/ - Default agent uses q_cli_default/ (no alphanumeric suffix) - Custom agents use <agent-name>_<alphanumeric-code>/ format Co-authored-by: Kenneth S. <kennvene@amazon.com>
- Add Drop trait to InputSource for automatic history saving - Replace DefaultHistory with FileHistory for persistence - Store history in ~/.aws/amazonq/cli_history - Refactor ChatHinter to use rustyline's built-in history search - Remove manual history tracking in favor of rustyline's implementation - Add history loading on startup with error handling - Clean up unused hinter history update methods
* comment profile set * comment profile in apiclient * add a helper func * fix compile issue * remove dead code tag
* client struct definition * clean up unused code * adds mechanism for checking if server is alive * prefetches prompts if applicable * fixes agent swap fixes agent swap * only applies process group leader promo for unix * removes unused import for windows * renames abstractions for different stages of mcp config
- Add file://AGENTS.md to default resources list alongside AmazonQ.md - Update test to include both AmazonQ.md and AGENTS.md files - Ensures AGENTS.md is included everywhere AmazonQ.md was previously included Co-authored-by: Matt Lee <mr-lee@users.noreply.github.com>
- Add optional 'model' field to Agent struct for specifying model per agent - Update JSON schema and documentation with model field usage - Integrate agent model into model selection priority: 1. CLI argument (--model) 2. Agent's model field (new) 3. User's saved default model 4. System default model - Add proper fallback when agent specifies unavailable model - Extract fallback logic to eliminate code duplication - Include comprehensive unit tests for model field functionality - Maintain backward compatibility with existing agent configurations Co-authored-by: Matt Lee <mr-lee@users.noreply.github.com>
* Properly handle path with trailing slash in file matching Today if a path has a trailing slash, the glob pattern will look like "/path-to-folder//**" (note the double slash). Glob doesn't work with double slash actually (it doesn't match anything). As a result, the permission management for fs_read and fs_write is broken when allowed or denied path has trailing slash. The fix is to just manually remove the trailing slash. * format change
* add a wrapmode in chat args * add a ut for the wrap arg
- Add auto_allow_readonly field to use_aws Settings struct (defaults to false) - Update eval_perm method to use auto_allow_readonly setting instead of hardcoded behavior - Default behavior: all AWS operations require user confirmation (secure by default) - Opt-in behavior: when autoAllowReadonly=true, read-only operations are auto-approved - Add comprehensive tests covering all scenarios - Maintains backward compatibility through configuration 🤖 Assisted by Amazon Q Developer Co-authored-by: Matt Lee <mr-lee@users.noreply.github.com>
…s#2838) Users can now keep the final question and answer from tangent mode by using `/tangent tail` instead of `/tangent`. This preserves the last Q&A pair when returning to the main conversation, making it easy to retain helpful insights discovered during exploration. - `/tangent` - exits tangent mode (existing behavior unchanged) - `/tangent tail` - exits tangent mode but keeps the last Q&A pair This enables users to safely explore topics without losing the final valuable insight that could benefit their main conversation flow.
Tracks daily active users by sending amazonqcli_dailyHeartbeat event once per day. Uses fail-closed logic to prevent spam during database errors.
* docs: fix local agent directory path - Fix local agent path from .aws/amazonq/cli-agents/ to .amazonq/cli-agents/ - Global paths (~/.aws/amazonq/cli-agents/) remain correct - Aligns documentation with source code implementation * fix: correct workspace agent path in /agent help message The help message for the /agent command incorrectly showed the workspace agent path as 'cwd/.aws/amazonq/cli-agents' when it should be 'cwd/.amazonq/cli-agents' (without the .aws directory). This fix aligns the help text with the actual WORKSPACE_AGENT_DIR_RELATIVE constant defined in directories.rs.
…urity (aws#2846) - Change default_allow_read_only() from true to false for secure by default behavior - Default behavior: all bash commands require user confirmation (secure by default) - Opt-in behavior: when autoAllowReadonly=true, read-only commands are auto-approved - Use autoAllowReadonly casing to match use_aws tool pattern - Update documentation to reflect new default value and consistent naming - Add comprehensive tests covering all scenarios - Maintains backward compatibility through configuration - Follows same pattern as use_aws autoAllowReadonly setting 🤖 Assisted by Amazon Q Developer Co-authored-by: Matt Lee <mrlee@amazon.com>
- Add Edit subcommand to AgentSubcommands enum - Implement edit functionality that opens existing agent files in editor - Use Agent::get_agent_by_name to locate and load existing agents - Include post-edit validation to ensure JSON remains valid - Add comprehensive tests for the new edit subcommand - Support both --name and -n flags for agent name specification 🤖 Assisted by Amazon Q Developer Co-authored-by: Matt Lee <mr-lee@users.noreply.github.com>
* Change autocomplete shortcut from ctrl-f to ctrl-g The reason is ctrl-f is the standard shortcut in UNIX for moving cursor forward by 1 character. You can find it being supported everywhere... in your browser, your terminal, etc. * make the autocompletion key configurable
* fix incorrect scope for mcp oauth * reverts custom tool config enum change * fixes display task overriding sign in notice * updates schema
aws#2975) * fix: consolidate tool permission logic for consistent display and execution * fix: centralize tool permission checking logic --------- Co-authored-by: Niraj Chowdhary <chownira@amazon.com>
* feat: expand support for /prompts command * fix: prompts spec * fix: add /prompts delete cmd * fix(prompts): improve validation and user input handling * fix(prompts): manage prompt using structs
Co-authored-by: Kenneth S. <kennvene@amazon.com>
Add experimental feature to show context window usage as a percentage in the chat prompt (e.g., "[rust-agent] 6% >"). The percentage is color-coded: green (<50%), yellow (50-89%), red (90-100%). The feature is disabled by default and can be enabled via /experiment.
* [fix] Fixes issues with Tool Input parsing. * Ocassionally the model will generate a tool use which parameters are not a valid json. When this happens it corrupts the conversation history. * Here we first avoid storing the tool use and add the propert validation logic to the conversation history. * adds validation logic to safety * [fix] Update to use a new RecvErrorKind instead of custom error handling. * [fix] Gives visual hint to the user, that request is being retried. --------- Co-authored-by: Kenneth S. <kennvene@amazon.com>
* (in progress) Implement checkpointing using git CLI commands * feat: Add new checkpointing functionality using git CLI Updates: - Only works if the user has git installed - Supports auto initialization if the user is in a git repo, manual if not - UI ported over from dedicated file tools implementation * feat: Add user message for turn-level checkpoints, clean command Updates: - The clean subcommand will delete the shadow repo - The description for turn-level checkpoints is a truncated version of the user's last message * fix: Fix shadow repo deletion logic Updates: - Running the clean subcommand now properly deletes the entire shadow repo for both automatic and manual modes * chore: Run formatter and fix clippy warnings * feat: Add checkpoint diff Updates: - Users can now view diffs between checkpoints - Fixed tool-level checkpoint display handling * fix: Fix last messsage handling for checkpoints Updates: - Checkpoints now (hopefully) correctly display the correct turn-specific user message - Added slash command auto completion * fix: Fix commit message handling again * chore: Run formatter * Removed old comment * define a global capture dirctory * revise the capture path * fix cpature clean bug * add a clean all flag * add auto drop method for capture feature * support file details when expand * add the file summary when list and expand * revise structure and print no diff msg * delete all flag, add summry when fs read * refactor code * revise ui * add capture into experiement * clippy * rename to checkpoint * reverse false renaming * recover history * disable tangent mode in checkpoint * fix cr * nit: keep checkpoint name * allow both tangent & checkpoint enabled * ci --------- Co-authored-by: kiran-garre <kiranbug@amazon.com>
Co-authored-by: Kenneth S. <kennvene@amazon.com>
Add version note indicating that agent-specific knowledge bases are available in development but not in current releases (v1.14.1 and earlier). Current releases use global storage at ~/.aws/amazonq/knowledge_bases/.
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.
Co-authored-by: amazon-q-developer[bot] <208079219+amazon-q-developer[bot]@users.noreply.github.com>
|
⏳ 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 PR successfully addresses the main issue described - fixing the empty string handling for chat.defaultAgent setting. The core fix in crates/chat-cli/src/cli/agent/mod.rs is well-implemented and maintains backward compatibility.
Key Findings
✅ Main Fix - Correct Implementation
The empty string handling for chat.defaultAgent is properly implemented by checking !user_set_default.is_empty() before attempting agent lookup.
⚠️ Security Concerns
- Critical: Build script contains incomplete download functionality with empty URL
- High: Hook execution system passes unsanitized user data to external commands
- Medium: Checkpoint functionality involves git operations that need input validation
📝 Recommendations
- Add test coverage for the empty string agent default behavior
- Fix or remove the incomplete
download_feed_json()function in build.rs - Implement input sanitization for hook execution system
- Add path validation for checkpoint and prompt file operations
Overall Assessment
While the main fix is solid, the PR introduces significant new functionality (checkpoints, enhanced prompts, hooks) that requires security hardening before production use. The core agent default fix can be safely merged, but the additional features need security review and input validation improvements.
| 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 treats empty strings as "no default set" by adding the condition !user_set_default.is_empty() before attempting to find the agent. This prevents the error message when users run q settings chat.defaultAgent "" to reset their default agent.
The logic is sound:
- If the setting exists but is empty, it skips the agent lookup entirely
- If the setting exists and is non-empty, it performs the lookup as before
- If the lookup fails for a non-empty value, it still shows the appropriate error message
This maintains backward compatibility while fixing the documented behavior.
| 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 | ||
| if !user_set_default.is_empty() { | ||
| if all_agents.iter().any(|a| a.name == user_set_default) { | ||
| break 'active_idx user_set_default; | ||
| } | ||
| let _ = queue!( | ||
| output, | ||
| style::SetForegroundColor(Color::Red), | ||
| style::Print("Error"), | ||
| style::SetForegroundColor(Color::Yellow), | ||
| style::Print(format!( | ||
| ": user defined default {} not found. Falling back to in-memory default", | ||
| user_set_default | ||
| )), | ||
| style::Print("\n"), | ||
| style::SetForegroundColor(Color::Reset) | ||
| ); |
There was a problem hiding this comment.
Consider adding a test case to verify the empty string handling behavior. This would help ensure the fix works correctly and prevent regressions. A test could verify that:
- When
chat.defaultAgentis set to an empty string, no error message is displayed - When
chat.defaultAgentis set to a non-existent agent name, the error message is still shown - The fallback to the built-in default agent works correctly in both cases
| "--show-error", // print error message to stderr (since -s is used) | ||
| "--max-filesize", "1048576", // 1MB limit | ||
| "", | ||
| ]) |
There was a problem hiding this comment.
🛑 Security Vulnerability: The download_feed_json() function contains a hardcoded empty URL string in the curl command, which will cause the command to fail. This appears to be incomplete implementation that could lead to build failures.
Additionally, there are several security concerns with this implementation:
- No URL validation or allowlist of permitted domains
- The curl command uses an empty URL which will fail
- No integrity verification of downloaded content beyond basic JSON validation
Consider implementing proper URL validation and integrity checks if this functionality is needed1.
Footnotes
-
CWE-494: Download of Code Without Integrity Check - https://cwe.mitre.org/data/definitions/494.html ↩
|
|
||
| /// Track tool uses within current turn | ||
| pub tools_in_turn: usize, | ||
|
|
There was a problem hiding this comment.
The checkpoint functionality introduces git operations with user-controlled input. There are several security considerations:
-
Command Injection Risk: The
run_git()function constructs git commands with user-provided arguments. While the current usage appears safe, ensure all inputs are properly validated. -
Path Traversal: The
shadow_repo_pathshould be validated to prevent directory traversal attacks. -
Resource Management: The
Dropimplementation attempts cleanup but may fail silently. Consider logging cleanup failures for debugging.
The implementation looks generally well-structured, but consider adding input validation for security-sensitive operations.
| if let Some(response) = tool_ctx.tool_response { | ||
| hook_input["tool_response"] = response; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛑 Security Vulnerability: The hook execution system passes user-controlled data through environment variables and stdin without proper sanitization. The sanitize_user_prompt() function is called for environment variables, but the JSON input passed to stdin contains unsanitized user data.
Key security concerns:
- Command Injection: User prompts and tool inputs are passed to external commands via stdin as JSON
- Information Disclosure: Sensitive data in prompts/tool inputs could be logged or exposed
- Resource Exhaustion: No limits on hook execution concurrency or resource usage
Consider implementing stricter input validation and sandboxing for hook execution1.
Footnotes
-
CWE-78: OS Command Injection - https://cwe.mitre.org/data/definitions/78.html ↩
| return Err(format!( | ||
| "Prompt name must be {} characters or less. Current length: {} characters.", | ||
| MAX_PROMPT_NAME_LENGTH, | ||
| name.len() |
There was a problem hiding this comment.
The prompt name validation is well-implemented with proper regex validation and length limits. However, consider these improvements:
-
Path Validation: While prompt names are validated, ensure the constructed file paths don't escape the intended directories through symlinks or other means.
-
File Permissions: Consider setting appropriate file permissions when creating prompt files to prevent unauthorized access.
-
Atomic Operations: File operations could benefit from atomic writes to prevent corruption during concurrent access.
The validation regex and length limits are good security practices.
| } | ||
|
|
||
| // Add a helper method to check if using non-default endpoint | ||
| fn is_custom_endpoint(database: &Database) -> bool { |
There was a problem hiding this comment.
Good security practice: The addition of is_custom_endpoint() check to skip profile ARN when using custom endpoints is a sensible security measure. This prevents potential information disclosure when connecting to non-AWS endpoints.
The debug logging for custom endpoints is also helpful for troubleshooting without exposing sensitive information.
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 messagesFixes the misleading behavior described in the AWS documentation where setting an empty string should reset to built-in default silently.
Ready to be submitted as PR to upstream aws/amazon-q-developer-cli.