Add global config switch to disable automatic session naming#7052
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a global configuration switch to disable the LLM-powered automatic session naming, intended to reduce unnecessary background LLM calls (tokens/latency) in CI/headless usage.
Changes:
- Introduces
GOOSE_DISABLE_SESSION_NAMINGas a new config/env value. - Short-circuits
SessionManager::maybe_update_namewhen the flag is enabled.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| crates/goose/src/session/session_manager.rs | Adds an early-return gate in maybe_update_name based on the new global config flag. |
| crates/goose/src/config/base.rs | Registers GOOSE_DISABLE_SESSION_NAMING via config_value! to generate get/set accessors. |
| if Config::global() | ||
| .get_goose_disable_session_naming() | ||
| .unwrap_or(false) | ||
| { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
Calling Config::global().get_goose_disable_session_naming() here will read (and potentially create/migrate) the config file on every maybe_update_name invocation, which is likely on every agent reply; consider checking an env var first and/or caching this flag (or moving the check to the caller) to avoid repeated disk I/O and latency.
| if Config::global() | ||
| .get_goose_disable_session_naming() | ||
| .unwrap_or(false) | ||
| { |
There was a problem hiding this comment.
Because this flag is defined as a bool, common CI-style values like GOOSE_DISABLE_SESSION_NAMING=1 will fail to deserialize and then be silently treated as false due to unwrap_or(false); consider treating this as a presence-based flag (like GOOSE_DISABLE_KEYRING) or otherwise accepting "1"/"0" to avoid a confusing no-op configuration.
| pub async fn maybe_update_name(&self, id: &str, provider: Arc<dyn Provider>) -> Result<()> { | ||
| if Config::global() | ||
| .get_goose_disable_session_naming() | ||
| .unwrap_or(false) | ||
| { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
The new disable-switch behavior in maybe_update_name isn’t covered by tests in this file; adding a unit/integration test that sets the flag and asserts generate_session_name is not invoked would prevent regressions (especially since this runs in a background task).
| config_value!(GOOSE_MODEL, String); | ||
| config_value!(GOOSE_PROMPT_EDITOR, Option<String>); | ||
| config_value!(GOOSE_MAX_ACTIVE_AGENTS, usize); | ||
| config_value!(GOOSE_DISABLE_SESSION_NAMING, bool); |
There was a problem hiding this comment.
This new config/env key should be added to the user-facing configuration docs (both the environment variable list and the config.yaml settings list), otherwise users won’t discover it or may assume unsupported values (e.g., "1") work.
| config_value!(GOOSE_MODEL, String); | ||
| config_value!(GOOSE_PROMPT_EDITOR, Option<String>); | ||
| config_value!(GOOSE_MAX_ACTIVE_AGENTS, usize); | ||
| config_value!(GOOSE_DISABLE_SESSION_NAMING, bool); |
There was a problem hiding this comment.
Declaring GOOSE_DISABLE_SESSION_NAMING as bool means env-var values like "1"/"yes" won’t deserialize to bool via get_param, which is inconsistent with other presence-based disable flags (e.g., GOOSE_DISABLE_KEYRING); consider using a presence-based string flag or broadening accepted boolean-like values.
| config_value!(GOOSE_DISABLE_SESSION_NAMING, bool); | |
| config_value!(GOOSE_DISABLE_SESSION_NAMING, String); |
DOsinga
left a comment
There was a problem hiding this comment.
Shouldn't we be doing this at the calling side of things in agent:reply_internal? there we spawn to do this on the side, maybe best to not do that at all?
| let skip_git_check = config | ||
| .get_codex_skip_git_check() | ||
| .map(|s| s.to_lowercase() == "true") | ||
| .map(|s| is_truthy(&s)) | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
Because get_codex_skip_git_check() is backed by a String-valued config key, common env values like CODEX_SKIP_GIT_CHECK=true/false/1/0 will be parsed as JSON primitives and fail to deserialize into String, causing this to fall back to false and effectively ignore the setting; consider making the config key typed as bool (and read it directly) so env/config-file booleans work as expected.
crates/goose/src/config/base.rs
Outdated
| config_value!(GOOSE_MODEL, String); | ||
| config_value!(GOOSE_PROMPT_EDITOR, Option<String>); | ||
| config_value!(GOOSE_MAX_ACTIVE_AGENTS, usize); | ||
| config_value!(GOOSE_DISABLE_SESSION_NAMING, String, "false"); |
There was a problem hiding this comment.
GOOSE_DISABLE_SESSION_NAMING is defined as a String with a default, but Config::get_param parses env/config-file true/false as booleans, so setting GOOSE_DISABLE_SESSION_NAMING=true (or false) will fail to deserialize into String and the flag won’t work; define this config value as a bool (and default via unwrap_or(false) on NotFound) or adjust the config parsing so string-typed keys can still accept boolean literals.
| config_value!(GOOSE_DISABLE_SESSION_NAMING, String, "false"); | |
| config_value!(GOOSE_DISABLE_SESSION_NAMING, bool, false); |
crates/goose/src/config/base.rs
Outdated
| /// Interpret a string value as a boolean in a permissive, presence-based way. | ||
| /// Returns true for values like "true", "1", "yes", "on" (case-insensitive). | ||
| /// Returns false for "false", "0", "no", "off", or empty strings. | ||
| pub fn is_truthy(value: &str) -> bool { | ||
| !matches!( | ||
| value.trim().to_lowercase().as_str(), | ||
| "false" | "0" | "no" | "off" | "" | ||
| ) | ||
| } |
There was a problem hiding this comment.
is_truthy claims to accept values like "true" and "1", but for env/config entries those values are currently parsed into boolean/number types before deserialization, so string-backed config keys will never see them and the behavior will be surprising; either scope this helper to raw strings that are known to stay strings (e.g., "yes"/"on"), or switch the relevant config keys to typed bool so "true"/"1" work as documented.
crates/goose/src/agents/agent.rs
Outdated
| .get_goose_disable_session_naming() | ||
| .map(|v| is_truthy(&v)) |
There was a problem hiding this comment.
This uses a String-backed config value plus is_truthy, but with the current env/config parsing GOOSE_DISABLE_SESSION_NAMING=true/false will deserialize as a bool and error, so unwrap_or(false) will ignore the user’s intent; prefer a bool-typed config getter here (e.g., get_param::<bool> or a config_value! bool) so the switch behaves predictably.
| .get_goose_disable_session_naming() | |
| .map(|v| is_truthy(&v)) | |
| .get_param::<bool>("goose_disable_session_naming") |
codefromthecrypt
left a comment
There was a problem hiding this comment.
since we can't predict the order of the session request, this is the single complicating thing of VCR style replay tests. Also this complicates the interim solution. I'd like to thread a config on ACP side to not require the Config::global() singleton to accomplish this.
| if !naming_disabled { | ||
| let manager_for_spawn = session_manager.clone(); | ||
| tokio::spawn(async move { | ||
| if let Err(e) = manager_for_spawn | ||
| .maybe_update_name(&session_id, provider) | ||
| .await | ||
| { | ||
| warn!("Failed to generate session description: {}", e); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
This new config-gated branch changes whether a background naming task is spawned, but there’s no test coverage validating that session naming is skipped when the flag is enabled; add a unit/integration test that sets GOOSE_DISABLE_SESSION_NAMING and asserts maybe_update_name is not invoked.
|
#7062 follow-up so it is used in acp and scenario tests |
* 'main' of github.com:block/goose: refactor: move disable_session_naming into AgentConfig (#7062) Add global config switch to disable automatic session naming (#7052) docs: add blog post - 8 Things You Didn't Know About Code Mode (#7059) fix: ensure animated elements are visible when prefers-reduced-motion is enabled (#7047) Show recommended model on failture (#7040) feat(ui): add session content search via API (#7050) docs: fix img url (#7053) Desktop UI for deleting custom providers (#7042) Add blog post: How I Used RPI to Build an OpenClaw Alternative (#7051)
Adds a
GOOSE_DISABLE_SESSION_NAMINGconfiguration flag that lets users turn off the LLM-powered automatic session naming feature. This is useful in headless/CI environments or for users who find the background naming calls unnecessary, reducing token usage and latency.