fix(config): honor workspace shell opt-in#2529
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces user workspace configuration overlays, allowing users to define workspace-specific settings (such as allow_shell) within their global configuration file. It adds logic to merge these configurations, implements OS-specific path comparison, and updates the documentation and tests accordingly. Feedback was provided to improve the robustness of the Windows path comparison logic by handling mismatched path separators and UNC prefixes.
| #[cfg(windows)] | ||
| fn paths_equal_for_config(left: &Path, right: &Path) -> bool { | ||
| left.to_string_lossy() | ||
| .eq_ignore_ascii_case(&right.to_string_lossy()) | ||
| } |
There was a problem hiding this comment.
On Windows, path comparison using to_string_lossy() directly can fail due to mismatched path separators (forward vs backward slashes) or the presence of the UNC prefix (\\?\) on canonicalized paths. Normalizing the slashes and stripping the UNC prefix ensures robust path matching.
#[cfg(windows)]
fn paths_equal_for_config(left: &Path, right: &Path) -> bool {
let left_str = left.to_string_lossy().replace('/', "\\");
let right_str = right.to_string_lossy().replace('/', "\\");
let left_clean = left_str.strip_prefix("\\\\\\\\?\\\\").unwrap_or(left_str.as_str());
let right_clean = right_str.strip_prefix("\\\\\\\\?\\\\").unwrap_or(right_str.as_str());
left_clean.eq_ignore_ascii_case(right_clean)
}|
You should see my comment here #2523 (comment) There are many issues around shell configuration that need to be fixed. |
|
Harvested into #2504 for the v0.8.50 release slice. What landed:
Local verification on the harvest branch:
Live #2504 CI is green across Ubuntu, macOS, Windows, lint, version drift, CodeQL, mobile smoke, npm smoke, and GitGuardian. Thanks @cyq1017 for the focused fix; I kept your authorship on the harvested commits. |
Problem
exec_shelltool remains unavailable despiteallow_shell = true+trusted = true#2523 reports that shell tools stay gated even when the user config has a workspace-specificallow_shell = trueentry.Change
[workspace.'...']/ legacy[projects."..."]entries forallow_shellduring TUI and engine-backed exec startup.allow_shelldefault.Verification
Fixes #2523
Greptile Summary
This PR fixes the user-reported issue where a workspace-scoped
allow_shell = trueentry in the user's global config file was ignored at launch because the TUI only merged top-level config values and project-local overlays.merge_user_workspace_config(called from both the TUI interactive path and theexecpath) that re-reads the user config file as raw TOML and applies theallow_shellvalue from any matching[workspace.'...']or legacy[projects.'...']entry, with guards for managed-config, requirements-path, and theDEEPSEEK_ALLOW_SHELLenvironment variable.resolve_load_config_pathvisibility topub(crate)so the new helper can call it, and adds five unit tests covering path matching, env-var precedence, and managed-config bypass.allow_shelldefault fromtruetofalse.Confidence Score: 5/5
The change is safe to merge — it adds a new opt-in code path guarded by managed-config and env-var precedence checks with no effect unless the user's global config explicitly contains a matching workspace entry.
The new merge function is narrow in scope (only touches
allow_shell), all existing precedence invariants are preserved, and the five new tests cover the main branches. The open gap — no test for the legacy[projects]table — is low-risk given that table follows the same iteration logic.No files require special attention; the logic in
crates/tui/src/main.rsis straightforward and well-tested for the primary[workspace]path.Important Files Changed
resolve_load_config_pathwidened topub(crate)so the new merge helper in main.rs can call it — a minimal, low-risk change.allow_shelldefault tofalse. The section does not note that a project-levelconfig.toml(applied after) can still override the user workspace setting, which could surprise users who rely on this opt-in.merge_user_workspace_config(called from both TUI interactive andexecpaths) with appropriate guards for managed-config, env-var precedence, and Windows path normalisation. Five new unit tests added; the legacy[projects]table branch is not covered by tests.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[launch / exec] --> B[load_config_from_cli\nuser file + env vars] B --> C{managed_config_path\nor requirements_path?} C -- yes --> G[skip workspace overlay] C -- no --> D[merge_user_workspace_config\nre-read raw TOML\nfind matching workspace entry\napply allow_shell] D --> E{DEEPSEEK_ALLOW_SHELL\nenv var set?} E -- yes --> F[restore pre-merge\nallow_shell value\nenv wins] E -- no --> H[keep merged value] F --> I{interactive mode?} H --> I G --> I I -- yes --> J[merge_project_config\n.codewhale/config.toml\napplied last — can override] I -- no / exec --> K[proceed with config] J --> KReviews (2): Last reviewed commit: "fix(config): normalize windows workspace..." | Re-trigger Greptile