-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: support global .gooseignore and negation patterns #6157
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
fix: support global .gooseignore and negation patterns #6157
Conversation
d73626a to
3e06718
Compare
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.
Pull request overview
This PR fixes issue #6155 by adding support for global .gooseignore files and fixing negation patterns to allow overriding default ignore patterns. Previously, the global .gooseignore file was never loaded, and default patterns couldn't be negated to allow access to files like .env.example.
Key changes:
- Modified pattern loading to always apply defaults first, then global, then local patterns (following gitignore semantics)
- Added global
.gooseignoresupport from~/.config/goose/.gooseignore - Enabled negation patterns (prefixed with
!) to override earlier patterns
| if let Some(ref global_path) = global_ignore_path { | ||
| if has_global_ignore { | ||
| let _ = builder.add(global_path); | ||
| } |
Copilot
AI
Dec 17, 2025
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.
The nested check is redundant. If has_global_ignore is true, then global_ignore_path must be Some, since has_global_ignore is computed from global_ignore_path.as_ref().map(|p| p.is_file()).unwrap_or(false). You can simplify this to just check if has_global_ignore and unwrap global_path directly since we know it must be Some.
| if let Some(ref global_path) = global_ignore_path { | |
| if has_global_ignore { | |
| let _ = builder.add(global_path); | |
| } | |
| if has_global_ignore { | |
| let global_path = global_ignore_path | |
| .as_ref() | |
| .expect("global_ignore_path must be Some when has_global_ignore is true"); | |
| let _ = builder.add(global_path); |
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.
+1
3e06718 to
8a33ac2
Compare
8c2f72b to
2f18caf
Compare
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.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
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.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
| .join(".gooseignore"), | ||
| ) | ||
| } else { | ||
| etcetera::choose_app_strategy(etcetera::AppStrategyArgs { |
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.
What's going on here? Don't want anything block specific. Can we mirror this:
goose/crates/goose-mcp/src/memory/mod.rs
Line 185 in bf95995
| let local_memory_dir = std::env::var("GOOSE_WORKING_DIR") |
|
|
||
| // 1. Always add default patterns first (lowest priority) | ||
| // This allows them to be negated by global or local .gooseignore files | ||
| let _ = builder.add_line(None, "**/.env"); |
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.
Old code only used these when we didn't explicitly have an ignore file; I think that makes sense.
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.
Let's only do these defaults if we don't have a local/global ignore file.
|
|
||
| #[tokio::test] | ||
| #[serial] | ||
| async fn test_global_gooseignore_negation_pattern() { |
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.
These tests are a bit longwinded.
Can we have a single parameterized test with inputs of:
global .gooseignore contents.
local .gooseignore contents.
expected output
| if let Some(ref global_path) = global_ignore_path { | ||
| if has_global_ignore { | ||
| let _ = builder.add(global_path); | ||
| } |
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.
+1
|
@katzdave I've addressed all the PR review comments:
Ready for another review when you get a chance! |
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.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
| // Only use default patterns if no ignore file exists | ||
| // This maintains backward compatibility with the original behavior | ||
| if !has_local_ignore && !has_global_ignore { | ||
| let _ = builder.add_line(None, "**/.env"); | ||
| let _ = builder.add_line(None, "**/.env.*"); | ||
| let _ = builder.add_line(None, "**/secrets.*"); | ||
| } else { | ||
| // When ignore files exist, add defaults first (lowest priority) | ||
| // to allow them to be negated by global or local .gooseignore files | ||
| let _ = builder.add_line(None, "**/.env"); | ||
| let _ = builder.add_line(None, "**/.env.*"); | ||
| let _ = builder.add_line(None, "**/secrets.*"); | ||
|
|
||
| // Add global .gooseignore patterns (medium priority) | ||
| if has_global_ignore { | ||
| let global_path = global_ignore_path | ||
| .as_ref() | ||
| .expect("global_ignore_path must be Some when has_global_ignore is true"); | ||
| let _ = builder.add(global_path); | ||
| } | ||
|
|
||
| // Add local .gooseignore patterns (highest priority) | ||
| if has_local_ignore { | ||
| let _ = builder.add(&local_ignore_path); | ||
| } | ||
| } |
Copilot
AI
Dec 22, 2025
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.
The comment "Only use default patterns if no ignore file exists" is misleading since default patterns are actually added in both branches. The code structure duplicates the default pattern additions unnecessarily. Simplify by removing the if-else and always adding defaults first, then conditionally adding global and local patterns.
| } | ||
|
|
||
| // Override HOME to use temp_dir so etcetera finds our test config | ||
| std::env::set_var("HOME", temp_dir.path()); |
Copilot
AI
Dec 22, 2025
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.
Setting the HOME environment variable is Unix-specific and won't work correctly on Windows, where etcetera uses APPDATA/LOCALAPPDATA instead. Since the actual code should work cross-platform, consider either marking this test as Unix-only with #[cfg(unix)] or making it cross-platform by setting the appropriate environment variables for each OS.
This fix addresses issue block#6155 where global .gooseignore negation patterns were not working to allow access to files like .env.example. Changes: - Always apply default patterns first (lowest priority) so they can be negated - Load global .gooseignore from ~/.config/goose/.gooseignore (medium priority) - Load local .gooseignore last (highest priority) - Import AppStrategy trait for etcetera config_dir() method The priority order is now: 1. Default patterns (**/.env, **/.env.*, **/secrets.*) - lowest 2. Global .gooseignore patterns - medium 3. Local .gooseignore patterns - highest This allows users to add negation patterns like '!.env.example' in their global or local .gooseignore files to allow access to specific files that would otherwise be blocked by the default patterns. Added tests: - test_global_gooseignore_negation_pattern - test_local_gooseignore_overrides_global - test_local_gooseignore_negation_overrides_defaults Fixes block#6155 Signed-off-by: leocavalcante <leonardo.cavalcante@picpay.com>
Signed-off-by: leocavalcante <leonardo.cavalcante@picpay.com>
…user config Add GOOSE_PATH_ROOT env var to prevent the test from loading the user's real global .gooseignore file, which could cause flaky test failures. This matches the pattern used by the other two gooseignore tests. Signed-off-by: leocavalcante <leonardo.cavalcante@picpay.com>
- Use choose_app_strategy(APP_STRATEGY) pattern from memory/mod.rs - Simplify redundant nested check for has_global_ignore - Convert three verbose tests to single parameterized test - Use HOME env var for test isolation instead of GOOSE_PATH_ROOT Signed-off-by: leocavalcante <leonardo.cavalcante@picpay.com>
56fe0f5 to
ccae925
Compare
- Remove duplicated default pattern code in if-else branches - Always add defaults first, then conditionally add global/local patterns - Add #[cfg(unix)] to test since HOME env var is Unix-specific Signed-off-by: leocavalcante <leonardo.cavalcante@picpay.com>
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.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Set HOME env var before calling etcetera to get the platform-specific config directory path, rather than hardcoding .config/goose which only works on Linux. Signed-off-by: leocavalcante <leonardo.cavalcante@picpay.com>
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.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
| // Override HOME to use temp_dir so etcetera finds our test config | ||
| std::env::set_var("HOME", temp_dir.path()); | ||
|
|
||
| // Set up global .gooseignore if specified | ||
| // Use etcetera to get the actual config dir path for this platform | ||
| if let Some(global_content) = test_case.global_ignore { | ||
| let global_config_dir = etcetera::choose_app_strategy(crate::APP_STRATEGY.clone()) | ||
| .map(|strategy| strategy.config_dir()) | ||
| .expect("Failed to get config dir"); | ||
| fs::create_dir_all(&global_config_dir).unwrap(); | ||
| fs::write(global_config_dir.join(".gooseignore"), global_content).unwrap(); | ||
| } | ||
|
|
||
| // Set up local .gooseignore if specified | ||
| if let Some(local_content) = test_case.local_ignore { | ||
| fs::write(".gooseignore", local_content).unwrap(); | ||
| } | ||
|
|
||
| let server = create_test_server(); | ||
|
|
||
| for (path, expected_ignored) in test_case.expected { | ||
| assert_eq!( | ||
| server.is_ignored(Path::new(path)), | ||
| *expected_ignored, | ||
| "Test '{}': expected '{}' ignored={}, got ignored={}", | ||
| test_case.name, | ||
| path, | ||
| expected_ignored, | ||
| server.is_ignored(Path::new(path)) | ||
| ); | ||
| } | ||
|
|
||
| // Clean up env var | ||
| std::env::remove_var("HOME"); |
Copilot
AI
Dec 22, 2025
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.
The test modifies the HOME environment variable but doesn't restore its original value after cleanup. This could cause issues if HOME was previously set, as it will be completely removed rather than restored. Consider saving the original HOME value before the loop and restoring it after, or use a scope guard pattern to ensure proper cleanup even if the test panics.
| let _ = builder.add(local_ignore_path); | ||
| has_ignore_file = true; | ||
| // Get the global config directory for .gooseignore | ||
| // Uses the same pattern as memory/mod.rs for consistency |
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.
rm comments. And below
|
|
||
| // 1. Always add default patterns first (lowest priority) | ||
| // This allows them to be negated by global or local .gooseignore files | ||
| let _ = builder.add_line(None, "**/.env"); |
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.
Let's only do these defaults if we don't have a local/global ignore file.
| ); | ||
| } | ||
|
|
||
| /// Test case for gooseignore patterns |
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.
Sorry, this still ended up being pretty complicated; I was hoping we could have something that just tested some of the diff cases without actually interacting with the filesystem.
If that's not possible, let's just get rid of the test completely because the logic in build_ignore_patterns isn't too heavy.
Signed-off-by: leocavalcante <leonardo.cavalcante@picpay.com>
Signed-off-by: leocavalcante <leonardo.cavalcante@picpay.com>
Signed-off-by: leocavalcante <leonardo.cavalcante@picpay.com>
katzdave
left a comment
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.
Nice job!
* main: fix: require auth when running goose on non loopback address (#6478) chore(deps): bump hono from 4.11.3 to 4.11.4 in /ui/desktop (#6485) feat(cli): graceful fallback for keyring failures (#5808) fix: support global .gooseignore and negation patterns (#6157) docs: manual config for jetbrains (#6490) fix: Recipe slash command doesn't work with single optional parameter (#6235) fix(openrouter): Handle Gemini thoughtSignature for tool calls (#6370) docs: fix extensions page (#6484) Allow customizing the new line keybinding in the CLI (#5956) Ask for permission in the CLI (#6475) docs: add Ralph Loop tutorial for multi-model iterative development (#6455) Remove gitignore fallback from gooseignore docs (#6480) fix: clean up result recording for code mode (#6343) fix(code_execution): handle model quirks with tool calls (#6352) feat(ui): support prefersBorder option for MCP Apps (#6465) fixed line breaks (#6459) Use Intl.NumberFormat for token formatting in SessionsInsights (#6466) feat(ui): format large and small token counts for readability (#6449) fix: apply subrecipes when using slash commands (#6460)
Signed-off-by: leocavalcante <leonardo.cavalcante@picpay.com> Signed-off-by: ThanhNguyxn <thanhnguyentuan2007@gmail.com>
…ased * 'main' of github.com:block/goose: fix(code_execution): serialize record_result output as JSON (#6495) perf(google): avoid accumulating thoughtSignatures across conversation history (#6462) fix(openai): make tool_call arguments optional and fix silent stream termination (#6309) fix: Improve error messages for invalid tool calls (#6483) fix: require auth when running goose on non loopback address (#6478) chore(deps): bump hono from 4.11.3 to 4.11.4 in /ui/desktop (#6485) feat(cli): graceful fallback for keyring failures (#5808) fix: support global .gooseignore and negation patterns (#6157) docs: manual config for jetbrains (#6490) fix: Recipe slash command doesn't work with single optional parameter (#6235) fix(openrouter): Handle Gemini thoughtSignature for tool calls (#6370) docs: fix extensions page (#6484) Allow customizing the new line keybinding in the CLI (#5956) Ask for permission in the CLI (#6475) docs: add Ralph Loop tutorial for multi-model iterative development (#6455) Remove gitignore fallback from gooseignore docs (#6480) fix: clean up result recording for code mode (#6343) fix(code_execution): handle model quirks with tool calls (#6352) feat(ui): support prefersBorder option for MCP Apps (#6465) fixed line breaks (#6459)
Signed-off-by: leocavalcante <leonardo.cavalcante@picpay.com> Signed-off-by: fbalicchia <fbalicchia@cuebiq.com>
Summary
This PR fixes issue #6155 where global
.gooseignorenegation patterns were not working to allow access to files like.env.example.Problem
The previous implementation had two issues:
.gooseignorefile at~/.config/goose/.gooseignorewas never being loaded.gooseignoreexisted, making it impossible to negate themSolution
Changed
build_ignore_patterns()to:**/.env,**/.env.*,**/secrets.*) - lowest priority.gooseignorefrom~/.config/goose/.gooseignore- medium priority.gooseignore- highest priorityThis follows the standard gitignore pattern matching semantics where later patterns can override earlier ones, and negation patterns (prefixed with
!) can allow access to files that would otherwise be ignored.Example
With this fix, users can now add
!.env.exampleto their global or local.gooseignoreto allow access to.env.examplefiles while still blocking.envand.env.local.Testing
Added 3 new tests:
test_global_gooseignore_negation_pattern- verifies global negation patterns worktest_local_gooseignore_overrides_global- verifies local patterns override globaltest_local_gooseignore_negation_overrides_defaults- verifies local negation patterns override defaultsAll existing ignore-related tests continue to pass.
Fixes #6155