Feature/claude feature #110
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideAdds a local Dev Container Feature for the Claude Code CLI, including installation logic, configuration directory setup, and extensive docs/troubleshooting for running Claude inside devcontainers, plus minor devcontainer/task wiring changes. Sequence diagram for OAuth authentication flow with Claude in devcontainersequenceDiagram
actor DevUser
participant HostVSCode
participant DevContainer
participant ClaudeCLI
participant OAuthServer as OAuth_provider
participant HostBrowser
participant HostFS
DevUser ->> HostVSCode: Open_repo_and_start_devcontainer
HostVSCode ->> DevContainer: Create_container_with_runArgs_network_host
DevContainer ->> ClaudeCLI: Feature_install.sh_installs_CLI
DevUser ->> ClaudeCLI: Run_claude
ClaudeCLI ->> ClaudeCLI: Start_local_OAuth_callback_server
ClaudeCLI ->> HostBrowser: Print_OAuth_URL
DevUser ->> HostBrowser: Open_URL
HostBrowser ->> OAuthServer: Authorize_request
OAuthServer -->> HostBrowser: Redirect_to_http_localhost_random_port_callback
Note over DevContainer,HostBrowser: With_network_host,_container_port_equals_host_port
HostBrowser ->> ClaudeCLI: HTTP_callback_to_localhost_port
ClaudeCLI ->> HostFS: Write_tokens_to_~/.claude/.credentials.json
ClaudeCLI ->> HostFS: Update_~/.claude/.claude.json_state
ClaudeCLI -->> DevUser: Authenticated_interactive_session
Flow diagram for install.sh Claude Code feature installation logicflowchart TD
A["Start_install.sh"] --> B["Detect_package_manager"]
B --> C["Check_node_and_npm_present"]
C -->|"Missing"| D["install_nodejs(pkg_manager)"]
C -->|"Present"| E["Skip_Node.js_install"]
D --> F{"node_and_npm_installed"}
F -->|"No"| G["print_nodejs_requirement_and_exit"]
F -->|"Yes"| H["Proceed_to_CLI_install"]
E --> H
H --> I{"claude_command_exists"}
I -->|"Yes"| J["Print_existing_version"]
I -->|"No"| K["npm_install_-g_@anthropic-ai/claude-code"]
J --> L["create_claude_directories"]
K --> M{"claude_installed"}
M -->|"No"| N["Exit_with_error"]
M -->|"Yes"| L
L --> O["Determine_target_home_and_user(_REMOTE_USER_or_vscode)"]
O --> P["mkdir_-p_.claude,_agents,_commands,_hooks"]
P --> Q["Ensure_.credentials.json_and_.claude.json_exist_with_600_perms"]
Q --> R["chown_-R_target_user_if_running_as_root"]
R --> S["Print_summary_and_success"]
S --> T["End_install.sh"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
install.sh, theinstall_nodejsfunction assumescurlis available for theyumanddnfpaths but only explicitly installs it forapt; consider installingcurl(and any other prerequisites likeca-certificates) before using it in those branches to avoid failures on minimal images. - When
create_claude_directoriescreates placeholder.credentials.jsonand.claude.jsonfiles with{}content, it does so silently; adding an explicit log message or warning in this code path would make it easier to distinguish between real credentials/state and auto-generated placeholders during debugging.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `install.sh`, the `install_nodejs` function assumes `curl` is available for the `yum` and `dnf` paths but only explicitly installs it for `apt`; consider installing `curl` (and any other prerequisites like `ca-certificates`) before using it in those branches to avoid failures on minimal images.
- When `create_claude_directories` creates placeholder `.credentials.json` and `.claude.json` files with `{}` content, it does so silently; adding an explicit log message or warning in this code path would make it easier to distinguish between real credentials/state and auto-generated placeholders during debugging.
## Individual Comments
### Comment 1
<location> `.devcontainer/claude-code/install.sh:23-47` </location>
<code_context>
+}
+
+# Function to install packages using the appropriate package manager
+install_packages() {
+ local pkg_manager="$1"
+ shift
+ local packages="$@"
+
+ case "$pkg_manager" in
+ apt)
+ apt-get update
+ apt-get install -y $packages
+ ;;
+ apk)
+ apk add --no-cache $packages
+ ;;
+ dnf|yum)
+ $pkg_manager install -y $packages
+ ;;
+ *)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Use "$@" instead of collecting packages into a single unquoted string to preserve arguments correctly.
`install_packages` currently does `local packages="$@"` and later uses `$packages` unquoted, which collapses all arguments into one string and relies on word-splitting. Since you already `shift` the package manager off, you can drop the extra variable and pass `"$@"` directly to the package manager, e.g.:
```sh
a pt-get install -y "$@"
apk add --no-cache "$@"
"$pkg_manager" install -y "$@"
```
This avoids incorrect splitting if arguments contain spaces or glob characters and follows common shell practices.
```suggestion
# Function to install packages using the appropriate package manager
install_packages() {
local pkg_manager="$1"
shift
case "$pkg_manager" in
apt)
apt-get update
apt-get install -y "$@"
;;
apk)
apk add --no-cache "$@"
;;
dnf|yum)
"$pkg_manager" install -y "$@"
;;
*)
echo "WARNING: Unsupported package manager. Cannot install packages:" "$@"
return 1
;;
esac
return 0
}
```
</issue_to_address>
### Comment 2
<location> `.devcontainer/claude-code/install.sh:1-4` </location>
<code_context>
+#!/bin/sh
+set -eu
+
+# Claude Code CLI Local Feature Install Script
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider enabling pipefail to avoid masking errors in piped commands (e.g., curl | bash).
Because this script uses pipelines like `curl -sL https://rpm.nodesource.com/setup_18.x | bash -`, `set -eu` alone won’t fail the script if `curl` fails, which can hide errors and leave partial setup. If `/bin/sh` is actually bash (or if you switch to `#!/bin/bash`), consider adding `set -o pipefail` to ensure the script exits on failure in any part of the pipeline. Alternatively, download the script with `curl` to a file, check `curl`’s exit code, and only then execute it instead of using `curl | bash` directly.
```suggestion
#!/bin/bash
set -euo pipefail
# Claude Code CLI Local Feature Install Script
```
</issue_to_address>
### Comment 3
<location> `.devcontainer/claude-code/install.sh:99` </location>
<code_context>
+ echo "Installing Claude Code CLI globally..."
+
+ # Install with npm
+ npm install -g @anthropic-ai/claude-code
+
+ # Verify installation
</code_context>
<issue_to_address>
**suggestion:** Consider pinning the Claude Code CLI version or allowing it to be configured for reproducible devcontainer builds.
Installing `@anthropic-ai/claude-code` without a version makes devcontainer builds non-deterministic across time and team members. To keep environments reproducible and upgrades controlled, either pin a specific version (e.g. `npm install -g @anthropic-ai/[email protected]`) or parameterize it via an environment variable:
```sh
CLAUDE_CODE_VERSION="${CLAUDE_CODE_VERSION:-latest}"
npm install -g "@anthropic-ai/claude-code@${CLAUDE_CODE_VERSION}"
```
Suggested implementation:
```
# Function to install Claude Code CLI
install_claude_code() {
# Allow version to be configured, defaulting to "latest" for flexibility.
# Example: export CLAUDE_CODE_VERSION="1.2.3" to pin a specific version.
CLAUDE_CODE_VERSION="${CLAUDE_CODE_VERSION:-latest}"
echo "Installing Claude Code CLI globally (version: ${CLAUDE_CODE_VERSION})..."
# Ensure Node.js and npm are available
if ! command -v node >/dev/null || ! command -v npm >/dev/null; then
echo "Node.js and npm are required to install Claude Code CLI but were not found in PATH."
return 1
fi
# Install with npm, using configurable version for reproducible builds
if npm install -g "@anthropic-ai/claude-code@${CLAUDE_CODE_VERSION}"; then
echo "Successfully installed Claude Code CLI (version: ${CLAUDE_CODE_VERSION})"
return 0
else
echo "Failed to install Claude Code CLI (version: ${CLAUDE_CODE_VERSION})"
return 1
fi
}
```
If your devcontainer uses environment files or `devcontainer.json` to set build-time environment variables, you may also want to:
1. Add `CLAUDE_CODE_VERSION` to those configurations to pin a specific version for the team.
2. Document the variable in your project README or contributing guide so developers know how to override it.
</issue_to_address>
### Comment 4
<location> `.devcontainer/claude-code/README.md:446` </location>
<code_context>
+Based on [PR #25](https://github.com/anthropics/devcontainer-features/pull/25):
+
+- **Read-Only Mounts**: Prevents prompt injection attacks that could modify CLAUDE.md or hooks
+- **No Credential Mounts**: `.credentials.json` is NOT mounted to prevent key exfiltration
+- **Isolated Configuration**: Each container uses host config but cannot modify it
+
</code_context>
<issue_to_address>
**🚨 issue (security):** Security notes contradict earlier sections about mounting `.credentials.json` and `.claude.json`.
This contradicts the earlier description (and TROUBLESHOOTING) where `.credentials.json` and `.claude.json` are described as read-write mounts that must be writable. Please align this bullet with the actual behavior—either clarify under what conditions credentials are mounted or update/remove the “No Credential Mounts” claim so the security notes accurately reflect the real mount configuration.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+23
to
+47
| # Function to install packages using the appropriate package manager | ||
| install_packages() { | ||
| local pkg_manager="$1" | ||
| shift | ||
| local packages="$@" | ||
|
|
||
| case "$pkg_manager" in | ||
| apt) | ||
| apt-get update | ||
| apt-get install -y $packages | ||
| ;; | ||
| apk) | ||
| apk add --no-cache $packages | ||
| ;; | ||
| dnf|yum) | ||
| $pkg_manager install -y $packages | ||
| ;; | ||
| *) | ||
| echo "WARNING: Unsupported package manager. Cannot install packages: $packages" | ||
| return 1 | ||
| ;; | ||
| esac | ||
|
|
||
| return 0 | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Use "$@" instead of collecting packages into a single unquoted string to preserve arguments correctly.
install_packages currently does local packages="$@" and later uses $packages unquoted, which collapses all arguments into one string and relies on word-splitting. Since you already shift the package manager off, you can drop the extra variable and pass "$@" directly to the package manager, e.g.:
a pt-get install -y "$@"
apk add --no-cache "$@"
"$pkg_manager" install -y "$@"This avoids incorrect splitting if arguments contain spaces or glob characters and follows common shell practices.
Suggested change
| # Function to install packages using the appropriate package manager | |
| install_packages() { | |
| local pkg_manager="$1" | |
| shift | |
| local packages="$@" | |
| case "$pkg_manager" in | |
| apt) | |
| apt-get update | |
| apt-get install -y $packages | |
| ;; | |
| apk) | |
| apk add --no-cache $packages | |
| ;; | |
| dnf|yum) | |
| $pkg_manager install -y $packages | |
| ;; | |
| *) | |
| echo "WARNING: Unsupported package manager. Cannot install packages: $packages" | |
| return 1 | |
| ;; | |
| esac | |
| return 0 | |
| } | |
| # Function to install packages using the appropriate package manager | |
| install_packages() { | |
| local pkg_manager="$1" | |
| shift | |
| case "$pkg_manager" in | |
| apt) | |
| apt-get update | |
| apt-get install -y "$@" | |
| ;; | |
| apk) | |
| apk add --no-cache "$@" | |
| ;; | |
| dnf|yum) | |
| "$pkg_manager" install -y "$@" | |
| ;; | |
| *) | |
| echo "WARNING: Unsupported package manager. Cannot install packages:" "$@" | |
| return 1 | |
| ;; | |
| esac | |
| return 0 | |
| } |
| echo "Installing Claude Code CLI globally..." | ||
|
|
||
| # Install with npm | ||
| npm install -g @anthropic-ai/claude-code |
There was a problem hiding this comment.
suggestion: Consider pinning the Claude Code CLI version or allowing it to be configured for reproducible devcontainer builds.
Installing @anthropic-ai/claude-code without a version makes devcontainer builds non-deterministic across time and team members. To keep environments reproducible and upgrades controlled, either pin a specific version (e.g. npm install -g @anthropic-ai/[email protected]) or parameterize it via an environment variable:
CLAUDE_CODE_VERSION="${CLAUDE_CODE_VERSION:-latest}"
npm install -g "@anthropic-ai/claude-code@${CLAUDE_CODE_VERSION}"Suggested implementation:
# Function to install Claude Code CLI
install_claude_code() {
# Allow version to be configured, defaulting to "latest" for flexibility.
# Example: export CLAUDE_CODE_VERSION="1.2.3" to pin a specific version.
CLAUDE_CODE_VERSION="${CLAUDE_CODE_VERSION:-latest}"
echo "Installing Claude Code CLI globally (version: ${CLAUDE_CODE_VERSION})..."
# Ensure Node.js and npm are available
if ! command -v node >/dev/null || ! command -v npm >/dev/null; then
echo "Node.js and npm are required to install Claude Code CLI but were not found in PATH."
return 1
fi
# Install with npm, using configurable version for reproducible builds
if npm install -g "@anthropic-ai/claude-code@${CLAUDE_CODE_VERSION}"; then
echo "Successfully installed Claude Code CLI (version: ${CLAUDE_CODE_VERSION})"
return 0
else
echo "Failed to install Claude Code CLI (version: ${CLAUDE_CODE_VERSION})"
return 1
fi
}
If your devcontainer uses environment files or devcontainer.json to set build-time environment variables, you may also want to:
- Add
CLAUDE_CODE_VERSIONto those configurations to pin a specific version for the team. - Document the variable in your project README or contributing guide so developers know how to override it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add a local devcontainer feature for installing and configuring the Claude Code CLI with mounted host configuration.
New Features:
Enhancements: