-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: add GitHub Codespaces support for browser tool #7994
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
- Add .devcontainer configuration with Chrome dependencies - Create post-create script to install Chrome and required libraries - Update BrowserSession.ts with sandbox flags for container environments - Update UrlContentFetcher.ts with container-specific Chrome flags - Add documentation for Codespaces setup This fixes the issue where the browser tool would fail in GitHub Codespaces due to missing Chrome/Chromium dependencies like libatk-1.0.so.0. Fixes #7990
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.
Reviewing my own code is like debugging in a mirror - everything looks backwards and I still missed the obvious bugs.
|
|
||
| # Install Google Chrome Stable | ||
| echo "🌐 Installing Google Chrome..." | ||
| wget -q -O - https://dl-ssl.google.com/linux/linux_signing_key.pub | sudo apt-key add - |
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.
🔒 Security concern: apt-key add is deprecated and insecure. Consider using the signed-by option instead:
| wget -q -O - https://dl-ssl.google.com/linux/linux_signing_key.pub | sudo apt-key add - | |
| wget -q -O /usr/share/keyrings/google-chrome.gpg https://dl-ssl.google.com/linux/linux_signing_key.pub | |
| sudo sh -c 'echo "deb [arch=amd64 signed-by=/usr/share/keyrings/google-chrome.gpg] http://dl.google.com/linux/chrome/deb/ stable main" >> /etc/apt/sources.list.d/google.list' |
| @@ -0,0 +1,101 @@ | |||
| #!/bin/bash | |||
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 script needs executable permissions. Could we add a chmod +x in the devcontainer.json or ensure it's committed with executable permissions?
| wget -q -O - https://dl-ssl.google.com/linux/linux_signing_key.pub | sudo apt-key add - | ||
| sudo sh -c 'echo "deb [arch=amd64] http://dl.google.com/linux/chrome/deb/ stable main" >> /etc/apt/sources.list.d/google.list' | ||
| sudo apt-get update | ||
| sudo apt-get install -y google-chrome-stable |
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.
Missing error handling here. If Chrome installation fails, the script continues and reports success. Consider adding:
| sudo apt-get install -y google-chrome-stable | |
| sudo apt-get install -y google-chrome-stable || { | |
| echo "❌ Failed to install Chrome. Browser tool may not work properly." | |
| exit 1 | |
| } |
| "runArgs": ["--cap-add=SYS_PTRACE", "--security-opt", "seccomp=unconfined"], | ||
| "containerEnv": { | ||
| "PUPPETEER_SKIP_CHROMIUM_DOWNLOAD": "false", | ||
| "PUPPETEER_EXECUTABLE_PATH": "/usr/bin/google-chrome-stable" |
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 hardcoded path assumes Chrome will always be at this location. Could we make this more flexible or add a fallback? What if Chrome gets installed elsewhere or we use Chromium instead?
| ] | ||
|
|
||
| // Add sandbox flags for container environments (Codespaces, Docker, etc.) | ||
| if (process.env.CODESPACES === "true" || process.env.CONTAINER === "true" || process.platform === "linux") { |
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.
Is checking for Linux platform alongside container env vars intentional? This would apply sandbox flags to ALL Linux systems, not just containers. Consider:
| if (process.env.CODESPACES === "true" || process.env.CONTAINER === "true" || process.platform === "linux") { | |
| if (process.env.CODESPACES === "true" || process.env.CONTAINER === "true" || (process.platform === "linux" && (process.env.container || process.env.KUBERNETES_SERVICE_HOST))) { |
Or maybe we actually want this for all Linux systems?
| "--disable-setuid-sandbox", | ||
| "--disable-dev-shm-usage", | ||
| "--disable-gpu", | ||
| "--disable-web-security", |
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.
Some of these flags like --disable-web-security and --disable-features=IsolateOrigins,site-per-process are quite permissive. Are we comfortable with the security implications for production use?
| "--no-first-run", | ||
| "--disable-gpu", | ||
| "--disable-features=VizDisplayCompositor", | ||
| "--no-sandbox", // Linux-specific argument |
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 that we're testing Linux-specific flags, but should we also add a test specifically for the CODESPACES environment variable detection?
Summary
This PR fixes the browser tool functionality in GitHub Codespaces by installing the necessary Chrome/Chromium dependencies that were missing.
Problem
As reported in #7990, the browser tool was failing in GitHub Codespaces with the error:
Solution
This PR adds comprehensive GitHub Codespaces support by:
.devcontainer/devcontainer.jsonwith proper Node.js environment and VS Code extensionsBrowserSession.tsandUrlContentFetcher.tsto detect container environments and apply necessary sandbox flags.devcontainer/explaining the setup and troubleshootingChanges
.devcontainer/devcontainer.jsonconfiguration.devcontainer/post-create.shinstallation script.devcontainer/README.mddocumentationsrc/services/browser/BrowserSession.tswith container detectionsrc/services/browser/UrlContentFetcher.tswith container detectionTesting
How to Test
Fixes #7990
cc @LousyBook94
Important
Adds GitHub Codespaces support by installing Chrome dependencies and updating browser launch configurations for container environments.
.devcontainer/post-create.sh.BrowserSession.tsandUrlContentFetcher.tsto detect container environments and add sandbox flags..devcontainer/devcontainer.jsonfor Node.js and VS Code extensions..devcontainer/README.mdfor setup and troubleshooting.UrlContentFetcher.spec.tsto reflect new Chrome flags for Linux environments.This description was created by
for 1605af0. You can customize this summary. It will automatically update as commits are pushed.