-
Notifications
You must be signed in to change notification settings - Fork 3
Feature/update from template #697
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
base: main
Are you sure you want to change the base?
Conversation
Updates the requirements on [pytest](https://github.com/pytest-dev/pytest), [hypothesis](https://github.com/HypothesisWorks/hypothesis), [ruff](https://github.com/astral-sh/ruff), [coverage](https://github.com/coveragepy/coveragepy) and [pre-commit](https://github.com/pre-commit/pre-commit) to permit the latest version. Updates `pytest` to 9.0.2 - [Release notes](https://github.com/pytest-dev/pytest/releases) - [Changelog](https://github.com/pytest-dev/pytest/blob/main/CHANGELOG.rst) - [Commits](pytest-dev/pytest@7.4.0...9.0.2) Updates `hypothesis` to 6.148.8 - [Release notes](https://github.com/HypothesisWorks/hypothesis/releases) - [Commits](HypothesisWorks/hypothesis@hypothesis-python-6.104.2...hypothesis-python-6.148.8) Updates `ruff` to 0.14.10 - [Release notes](https://github.com/astral-sh/ruff/releases) - [Changelog](https://github.com/astral-sh/ruff/blob/main/CHANGELOG.md) - [Commits](astral-sh/ruff@0.5.0...0.14.10) Updates `coverage` to 7.13.1 - [Release notes](https://github.com/coveragepy/coveragepy/releases) - [Changelog](https://github.com/coveragepy/coveragepy/blob/main/CHANGES.rst) - [Commits](coveragepy/coveragepy@7.5.4...7.13.1) Updates `pre-commit` to 4.5.1 - [Release notes](https://github.com/pre-commit/pre-commit/releases) - [Changelog](https://github.com/pre-commit/pre-commit/blob/main/CHANGELOG.md) - [Commits](pre-commit/pre-commit@v0.1.0...v4.5.1) --- updated-dependencies: - dependency-name: pytest dependency-version: 9.0.2 dependency-type: direct:development dependency-group: dev-dependencies - dependency-name: hypothesis dependency-version: 6.148.8 dependency-type: direct:development dependency-group: dev-dependencies - dependency-name: ruff dependency-version: 0.14.10 dependency-type: direct:development dependency-group: dev-dependencies - dependency-name: coverage dependency-version: 7.13.1 dependency-type: direct:development dependency-group: dev-dependencies - dependency-name: pre-commit dependency-version: 4.5.1 dependency-type: direct:development dependency-group: dev-dependencies ... Signed-off-by: dependabot[bot] <[email protected]>
…ba4d64570 build(deps-dev): bump the dev-dependencies group with 5 updates
address some comments
Reviewer's GuideRefactors the claude-code devcontainer feature to rely on the devcontainer Node.js feature instead of doing in-script package management, hardens home-directory handling, tweaks documentation to match the new behavior, pins the Pixi version, and prepares the devcontainer for SSH config mounts. Sequence diagram for updated claude-code devcontainer feature installationsequenceDiagram
participant DevcontainerEngine
participant NodeFeature as Node_feature
participant ClaudeFeature as Claude_code_feature
participant Shell as install_sh
participant Npm as npm_registry
DevcontainerEngine->>NodeFeature: Build step: install Node_feature
NodeFeature-->>DevcontainerEngine: Node.js and npm installed
DevcontainerEngine->>ClaudeFeature: Build step: activate claude-code feature
ClaudeFeature->>Shell: Execute install.sh main
Shell->>Shell: Print feature activation banner
Shell->>Shell: Check command node
Shell->>Shell: Check command npm
alt Node or npm missing
Shell->>DevcontainerEngine: Print error about missing Node.js feature
Shell-->>DevcontainerEngine: Exit 1
else Node and npm available
Shell->>Shell: Check command claude
alt claude already installed
Shell->>Shell: Print existing version
else claude not installed
Shell->>Npm: npm install -g @anthropic-ai/claude-code
Npm-->>Shell: Package installed globally
end
Shell->>Shell: create_claude_directories
Shell-->>ClaudeFeature: Installation and configuration complete
end
ClaudeFeature-->>DevcontainerEngine: Feature activation finished
Flow diagram for robust target home directory resolutionflowchart TD
Start([Start create_claude_directories]) --> SetUser[Set target_user = _REMOTE_USER or vscode]
SetUser --> SetHome[Set target_home = _REMOTE_USER_HOME or /home/target_user]
SetHome --> CheckHome{Does target_home exist?}
CheckHome -->|Yes| UseTargetHome[Use target_home]
UseTargetHome --> End([Done])
CheckHome -->|No| CheckEnvHome{Is HOME set and exists?}
CheckEnvHome -->|Yes| UseEnvHome[Warn and set target_home = HOME]
UseEnvHome --> End
CheckEnvHome -->|No| CheckDefaultHome{Does /home/target_user exist?}
CheckDefaultHome -->|Yes| UseDefaultHome[Warn and set target_home = /home/target_user]
UseDefaultHome --> End
CheckDefaultHome -->|No| Error[Print error about missing home dirs and exit 1]
Error --> End
Flow diagram for Node.js requirement enforcement in install.shflowchart TD
Start([Start main]) --> Banner[Print feature activation banner]
Banner --> CheckNode{command -v node and npm}
CheckNode -->|Missing| ErrorMsg[Print multi-line error
about required Node.js feature]
ErrorMsg --> ExitError[exit 1]
CheckNode -->|Present| CheckClaude{command -v claude}
CheckClaude -->|Exists| PrintVersion[Print existing Claude CLI version]
PrintVersion --> CreateDirs[Run create_claude_directories]
CreateDirs --> End([Done])
CheckClaude -->|Missing| InstallClaude[npm install -g @anthropic-ai/claude-code]
InstallClaude --> CreateDirs
CreateDirs --> End
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `.devcontainer/claude-code/install.sh:53-64` </location>
<code_context>
+ # Be defensive: if the resolved home does not exist, fall back to $HOME,
</code_context>
<issue_to_address>
**suggestion:** Reconsider the fallback order for `target_home` to avoid surprising use of `$HOME` from a different user context.
The current order `_REMOTE_USER_HOME` → `$HOME` → `/home/${target_user}` means that in common devcontainer setups (script running as root with `HOME=/root` and `target_user=vscode`), `/root` is chosen over `/home/vscode` and then later `chown`ed to `vscode`. This is functional but surprising and can hide `_REMOTE_USER_HOME` misconfigurations. Consider preferring `/home/${target_user}` over `$HOME`, or validating that `$HOME` actually belongs to `target_user` before using it.
```suggestion
local target_home="${_REMOTE_USER_HOME:-/home/${target_user}}"
# Be defensive: if the resolved home does not exist, fall back to /home/${target_user},
# then to $HOME. If neither is available, fail clearly.
if [ ! -d "$target_home" ]; then
if [ -d "/home/${target_user}" ]; then
echo "Warning: target_home '$target_home' does not exist, falling back to /home/${target_user}" >&2
target_home="/home/${target_user}"
elif [ -n "${HOME:-}" ] && [ -d "$HOME" ]; then
echo "Warning: target_home '$target_home' does not exist, falling back to \$HOME: $HOME" >&2
target_home="$HOME"
else
```
</issue_to_address>
### Comment 2
<location> `.devcontainer/claude-code/install.sh:112-115` </location>
<code_context>
-
- # Install Claude Code CLI
- # Check if already installed to make this idempotent
+ # Install Claude Code CLI (or verify it's already installed)
if command -v claude >/dev/null; then
echo "Claude Code CLI is already installed"
claude --version
</code_context>
<issue_to_address>
**suggestion:** It may be useful to log Node.js/npm versions when present for easier debugging of CLI installation issues.
The earlier script printed `node --version` and `npm --version` when available, which is very helpful for diagnosing CLI install/runtime issues from CI or devcontainer logs. Please consider adding those version logs back, either in `main` before calling `install_claude_code` or inside `install_claude_code` itself.
Suggested implementation:
```
# Install Claude Code CLI (or verify it's already installed)
if command -v claude >/dev/null; then
echo "Claude Code CLI is already installed"
claude --version
# Log Node.js and npm versions when available for easier debugging
if command -v node >/dev/null; then
echo "Node.js version: $(node --version)"
else
echo "Node.js is not installed or not on PATH"
fi
if command -v npm >/dev/null; then
echo "npm version: $(npm --version)"
else
echo "npm is not installed or not on PATH"
fi
```
If you also want Node.js/npm versions logged when the CLI is not yet installed (during the install path), mirror this version-logging block at the start of the installation function or in the script's `main` flow before `install_claude_code` is invoked. This ensures consistent diagnostic logging in both the "already installed" and "installing now" scenarios.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Summary by Sourcery
Simplify the Claude Code devcontainer feature by relying on the Node.js feature via installsAfter, improving home directory resolution, and tightening devcontainer setup defaults.
Bug Fixes:
Enhancements: