-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Enable load nested hint files #4002
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
Conversation
| let hints_filenames: Vec<String> = std::env::var("CONTEXT_FILE_NAMES") | ||
| .ok() | ||
| .and_then(|s| serde_json::from_str(&s).ok()) | ||
| .unwrap_or_else(|| vec![".goosehints".to_string()]); |
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.
Extracted the hints logic to goose_hints.rs
* main: feat: add @-mention file reference expansion to .goosehints (#3873) feat(cli): Add --name/-n to session remove and --id/-i alias for session export (#3941) Docs: provider and model run options (#4013) To-Do Tools (#3902) ci: correctly match doc only changes (#4009) Remove PR trigger for Linux build workflow (#4008) docs: update release docs with an additional step needed + adjust list formatting (#4005) chore(release): release version 1.3.0 (#3921) docs: MCP-ui blog content (#3996) feat: Add `GOOSE_TERMINAL` env variable to spawned terminals (#3911) add missing dependencies for developer setup (#3930)
| let _ = cliclack::log::info("Notice: GOOSE_NESTED_HINTS environment variable is set and will override the configuration here."); | ||
| } | ||
|
|
||
| let current_enabled: bool = config.get_param("NESTED_GOOSE_HINTS").unwrap_or(false); |
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.
I actually think enabled could be a better default here, and people would only need to seek out the config option if they didn't want it for some reason.
| .expect("Invalid file reference regex pattern") | ||
| }); | ||
|
|
||
| const MAX_DEPTH: usize = 3; |
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.
Why three? I'd think there would be realistic repo scenarios of wanting four or five perhaps
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.
This is copied from existing behaviour during refactoring.
This max depth is more about reference depth. For example,
file 1
@file2.md
file2
@file3.md
file3
@file4.md
We can increase if we would like to have more chain of references
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.
oh i see
| if name == "developer" { | ||
| let config = crate::config::Config::global(); | ||
| if let Ok(nested_enabled) = config.get_param::<bool>("NESTED_GOOSE_HINTS") { | ||
| command.env("NESTED_GOOSE_HINTS", nested_enabled.to_string()); |
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.
do we not already forward env vars to mcp server subprocesses?
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.
Here we get the NESTED_GOOSE_HINTS value from config, and pass it to the mcp as environment variable as it is not good for mcp server to know the config.
We could simplify this without make this behaviour configuration as suggested in the comments below
| }; | ||
| use rmcp::object; | ||
|
|
||
| use crate::developer::goose_hints::load_hints::{load_hint_files, GOOSE_HINTS_FILENAME}; |
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.
Good to see developer get smaller and this to be moved out!
I do wonder if we will need tighter integration again soon though. It could be nice to have it discover .goosehints in subdirs when working with files in those subdirs. And feels like the depth limit could be more relaxed for this case...
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.
I feel this is a slightly different requirements
In the implementation of this PR, while working on the current directly, it inherits all the .goosehints from the parent directory up to the project root.
It could be nice to have it discover .goosehints in subdirs when working with files in those subdirs. And feels like the depth limit could be more relaxed for this case...
This would be also useful. This also aligns the glob features in cursor, and Claude code has something similar. We can work on this separately
|
Tried it locally and asked: "What .goosehints do you already know about?" from cwd of it picked up the one from I wonder if this should work, or indicates an issue with the upward traversal |
|
I think consider default on for this, if it isn't ready for that, then it needs more work IMO. This would use the nearest hints - is that right? or does it keep loading them all the way up? |
|
Yeah I agree with @michaelneale above! I think this would be good just "default to on" behavior. I also don't know that it needs to be configureable tbh. The more options we add to settings the more conceptual overhead there is for the user. People could configure it by modifying/adjusting/removing the content of the goosehints files it finds and uses. |
* main: (108 commits) Remove unused game (#4226) fix issue where app redirects to home after initialization but user has already started a chat (#4260) Feat: Let providers configure a fast model for summarization (#4228) docs: update tool selection strategy (#4258) feat: upgrade `@mcp-ui/client` package and improve UI message handling (#4164) stop replacing chat window when changing working directory (#4200) Only fetch session tokens when chat state is idle to avoid resetting during streaming (#4104) bump timeouts for e2e tests (#4251) docs: custom context files improvements (#4096) chore: upgrade rmcp to 0.6.0 (#4243) doc: uvx not npx (#4240) Add PKCE support for Tetrate Agent Router Service (#4165) Read AGENTS.md by default (#4232) docs: configure provider and model (#4235) docs: add figma tutorial (#4231) Add Nix flake for reproducible builds (#4213) Enhanced onboarding page visual design (#4156) feat: adds mtls to all providers (#2794) (#2799) Don't show a confirm dialog for quitting (#4225) Fix: Missing smart_approve in CLI /mode help text and error message (#4132) ...
👍
It will keep loading them all the way up until they see the directory with .git (eg project root). Otherwise, it uses the |
👍 This totally makes sense! |
Hi @alexhancock Thank you so much for testing this! Really appreciate it! I tried on local, it seems Goose picked up the
|
|
Hi @alexhancock @michaelneale, Thank you so much for your time to review this PR. I've replied to your comments and removed the NESTED_GOOSE_HINTS configuration as suggested. I added a bit clarification on MAX_DEPTH, happy to increase if you think 4/5 depth will be helpful. Thank you! |
alexhancock
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.
So much simpler without the config options. This makes sense as default behavior to me. Nice!
Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
Signed-off-by: Dorien Koelemeijer <dkoelemeijer@squareup.com>

Implement the feature for #3724
What
Next (not in this PR)
It would be good to show loaded hints in CLI and Desktop.