fix: write to real file if config.yaml is symlink#7669
Conversation
Signed-off-by: lsytj0413 <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2a58ce465
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut path = self.config_path.clone(); | ||
|
|
||
| // Follow symlinks so we update the target file without replacing the link itself. | ||
| const MAX_SYMLINK_HOPS: usize = 1; |
There was a problem hiding this comment.
Resolve full symlink chains before writing config
Limiting symlink resolution to a single hop causes save_values to fail for valid, non-cyclic setups where config.yaml points to another symlink (for example, when dotfile managers or environment overlays create a 2+ link chain). In that case set_param now returns "Too many symlink levels" instead of updating the real file, which is a functional regression for those users; this should follow the chain until a real file (or a detected cycle) rather than hard-capping at one hop.
Useful? React with 👍 / 👎.
| load_init_config_from_workspace() | ||
| } | ||
|
|
||
| fn config_write_target_path(&self) -> Result<PathBuf, ConfigError> { |
There was a problem hiding this comment.
The goal makes sense to me, but why not use fs::canonicalize?
There was a problem hiding this comment.
Since the config_path may not exist, we should return it as the target_path in this scenario.
There was a problem hiding this comment.
That makes sense. It just seems like symlink resolution is something we should let the OS do, not on our own in a loop. But I'm not sure there's a better way if we also want to preserve the creating of parent directories.
| load_init_config_from_workspace() | ||
| } | ||
|
|
||
| fn config_write_target_path(&self) -> Result<PathBuf, ConfigError> { |
There was a problem hiding this comment.
That makes sense. It just seems like symlink resolution is something we should let the OS do, not on our own in a loop. But I'm not sure there's a better way if we also want to preserve the creating of parent directories.
* origin/main: (21 commits) fix(ui-desktop): unify path resolution around GOOSE_PATH_ROOT (#7335) fix: pass OAuth scopes to DCR and extract granted_scopes from token response (#7571) fix: write to real file if config.yaml is symlink (#7669) fix: preserve pairings when stopping gateway (#7733) fix: reduce server log verbosity — skip session in instrument, defaul… (#7729) fix: provider test infrastructure (#7738) fix: sanitize streamable HTTP extension names derived from URLs (#7740) refactor: derive GooseMode string conversions with strum (#7706) docs: Add Spraay Batch Payments MCP Extension Tutorial (#7525) fix: flake.nix (#7224) delete goose web (#7696) Add @angiejones as CODEOWNER for documentation (#7711) Add MLflow integration guide (#7563) docs: LM Studio availability (#7698) feat: add Avian as an LLM provider (#7561) Adds `linux-mcp-server` to the goose registry (#6979) fix: add #[serde(default)] to description field on 4 ExtensionConfig variants (#7708) feat: combine TUI UX from alexhancock/tui-goodness with publishing config from jackamadeo/package-tui (#7683) chore: cleanup old sandbox (#7700) Correct windows artifact (#7699) ...
* origin/main: (21 commits) fix(ui-desktop): unify path resolution around GOOSE_PATH_ROOT (#7335) fix: pass OAuth scopes to DCR and extract granted_scopes from token response (#7571) fix: write to real file if config.yaml is symlink (#7669) fix: preserve pairings when stopping gateway (#7733) fix: reduce server log verbosity — skip session in instrument, defaul… (#7729) fix: provider test infrastructure (#7738) fix: sanitize streamable HTTP extension names derived from URLs (#7740) refactor: derive GooseMode string conversions with strum (#7706) docs: Add Spraay Batch Payments MCP Extension Tutorial (#7525) fix: flake.nix (#7224) delete goose web (#7696) Add @angiejones as CODEOWNER for documentation (#7711) Add MLflow integration guide (#7563) docs: LM Studio availability (#7698) feat: add Avian as an LLM provider (#7561) Adds `linux-mcp-server` to the goose registry (#6979) fix: add #[serde(default)] to description field on 4 ExtensionConfig variants (#7708) feat: combine TUI UX from alexhancock/tui-goodness with publishing config from jackamadeo/package-tui (#7683) chore: cleanup old sandbox (#7700) Correct windows artifact (#7699) ...
…e-issue * origin/main: fix(ui-desktop): unify path resolution around GOOSE_PATH_ROOT (#7335) fix: pass OAuth scopes to DCR and extract granted_scopes from token response (#7571) fix: write to real file if config.yaml is symlink (#7669) fix: preserve pairings when stopping gateway (#7733) fix: reduce server log verbosity — skip session in instrument, defaul… (#7729) fix: provider test infrastructure (#7738) fix: sanitize streamable HTTP extension names derived from URLs (#7740) refactor: derive GooseMode string conversions with strum (#7706) docs: Add Spraay Batch Payments MCP Extension Tutorial (#7525) fix: flake.nix (#7224) delete goose web (#7696) Add @angiejones as CODEOWNER for documentation (#7711) Add MLflow integration guide (#7563) docs: LM Studio availability (#7698) feat: add Avian as an LLM provider (#7561) Adds `linux-mcp-server` to the goose registry (#6979) fix: add #[serde(default)] to description field on 4 ExtensionConfig variants (#7708)
* main: (45 commits)
fix: resolve {{ recipe_dir }} in nested sub-recipe paths during secret discovery (#7797)
Add @DOsinga as CODEOWNER for documentation (#7799)
feat: Add summarize tool for deterministic reads (#7054)
fix(api): use camelCase in CallToolResponse and add type discriminators to ContentBlock (#7487)
feat: ACP providers for claude code and codex (#6605)
chore(deps): bump express-rate-limit from 8.2.1 to 8.3.0 in /evals/open-model-gym/mcp-harness (#7703)
feat(openai): capture reasoning summaries from responses API (#7375)
Fix some dependencies (#7794)
fix: improve keyring availability error detection (#7766)
feat: add MiniMax provider with Anthropic-compatible API (#7640)
feat: add Tensorix as a declarative provider (#7712)
fix(security): remove insecure default secret from GOOSE_EXTERNAL_BACKEND (#7783)
refactor: Convert Tanzu provider to declarative JSON config (#7124)
replaces https://github.com/block/goose/pull/7340/changes (#7786)
feat(summon): make skill supporting files individually loadable via load() (#7583)
Keep toast open on failed extension (#7771)
fix(ui-desktop): unify path resolution around GOOSE_PATH_ROOT (#7335)
fix: pass OAuth scopes to DCR and extract granted_scopes from token response (#7571)
fix: write to real file if config.yaml is symlink (#7669)
fix: preserve pairings when stopping gateway (#7733)
...
* main: (69 commits)
fix: resolve {{ recipe_dir }} in nested sub-recipe paths during secret discovery (#7797)
Add @DOsinga as CODEOWNER for documentation (#7799)
feat: Add summarize tool for deterministic reads (#7054)
fix(api): use camelCase in CallToolResponse and add type discriminators to ContentBlock (#7487)
feat: ACP providers for claude code and codex (#6605)
chore(deps): bump express-rate-limit from 8.2.1 to 8.3.0 in /evals/open-model-gym/mcp-harness (#7703)
feat(openai): capture reasoning summaries from responses API (#7375)
Fix some dependencies (#7794)
fix: improve keyring availability error detection (#7766)
feat: add MiniMax provider with Anthropic-compatible API (#7640)
feat: add Tensorix as a declarative provider (#7712)
fix(security): remove insecure default secret from GOOSE_EXTERNAL_BACKEND (#7783)
refactor: Convert Tanzu provider to declarative JSON config (#7124)
replaces https://github.com/block/goose/pull/7340/changes (#7786)
feat(summon): make skill supporting files individually loadable via load() (#7583)
Keep toast open on failed extension (#7771)
fix(ui-desktop): unify path resolution around GOOSE_PATH_ROOT (#7335)
fix: pass OAuth scopes to DCR and extract granted_scopes from token response (#7571)
fix: write to real file if config.yaml is symlink (#7669)
fix: preserve pairings when stopping gateway (#7733)
...
Summary
If config.yaml is a symlink, rename to the real file
Type of Change
AI Assistance
Testing
Related Issues
Relates to #ISSUE_ID
Discussion: LINK (if any)
Fix: #6847
Screenshots/Demos (for UX changes)
Before:
After: