-
Notifications
You must be signed in to change notification settings - Fork 0
fix(clawdbot): add browser executablePath and use configOverrides #553
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,12 +64,18 @@ lib.mkIf (!env.isCI) { | |
|
|
||
| # Browser configuration (headless on Linux, GUI on macOS) | ||
| # Gateway binds to LAN on Linux for k8s ingress access | ||
| config = { | ||
| # NOTE: uses configOverrides because upstream nix-clawdbot doesn't merge `config` into output | ||
| configOverrides = { | ||
| browser = { | ||
| enabled = true; | ||
| headless = pkgs.stdenv.isLinux; | ||
| } | ||
| // lib.optionalAttrs pkgs.stdenv.isLinux { | ||
| executablePath = "${pkgs.chromium}/bin/chromium"; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The executablePath is hardcoded to chromium regardless of platform. While this works currently because the module is disabled in CI and chromium is installed on Linux, it creates a subtle dependency assumption. Consider making this conditional: executablePath = lib.optionalString pkgs.stdenv.isLinux "${pkgs.chromium}/bin/chromium";This would make the browser configuration more robust if the module's guards change in the future, and better documents that chromium is only available on Linux (per packages/default.nix lines 107-114). Prompt for AgentThis would make the browser configuration more robust if the module's guards change in the future, and better documents that chromium is only available on Linux (per packages/default.nix lines 107-114).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: Chromium is referenced unconditionally, breaking macOS builds where Prompt for AI agents✅ Addressed in |
||
| }; | ||
|
Comment on lines
69
to
75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # First, let's look at the full clawdbot module to understand the context
cat home-manager/modules/clawdbot/default.nixRepository: shunkakinoki/dotfiles Length of output: 3752 🏁 Script executed: # Check chromium references in the codebase
rg -n "chromium" --type nix -C 2Repository: shunkakinoki/dotfiles Length of output: 756 🏁 Script executed: # Check the flake.nix to see how packages are defined
head -100 flake.nixRepository: shunkakinoki/dotfiles Length of output: 2646 🏁 Script executed: # Check if there are Darwin-specific handling in the module
rg -n "isDarwin\|isLinux\|stdenv" home-manager/modules/clawdbot/default.nixRepository: shunkakinoki/dotfiles Length of output: 47 🏁 Script executed: # Check if chromium is available on macOS in nixpkgs and if there's a Darwin alternative
rg -n "chromium" home-manager/packages/default.nix -B 5 -A 5Repository: shunkakinoki/dotfiles Length of output: 263 🏁 Script executed: # Check if there's any Darwin-specific browser configuration elsewhere
rg -n "darwin\|isDarwin" home-manager/modules/clawdbot/default.nix -B 2 -A 2Repository: shunkakinoki/dotfiles Length of output: 47 🏁 Script executed: # Check the home-manager/packages/default.nix to understand full context
cat home-manager/packages/default.nixRepository: shunkakinoki/dotfiles Length of output: 1432 Make browser configuration platform-conditional or provide Darwin alternative. The code unconditionally enables the browser and references Either guard the browser configuration with |
||
| gateway = lib.mkIf pkgs.stdenv.isLinux { | ||
| } | ||
| // lib.optionalAttrs pkgs.stdenv.isLinux { | ||
| gateway = { | ||
| bind = "lan"; | ||
| }; | ||
| }; | ||
|
|
||
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
executablePathis unconditionally set to chromium for all platforms, but chromium is only installed on Linux (seehome-manager/packages/default.nixline 108). On macOS, this will cause a build failure sincepkgs.chromiumwon't be available. TheexecutablePathshould only be set for Linux systems, similar to how theheadlessproperty is conditionally set.