Make Wave bench launches responsive - #8
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
Author
|
@codex review |
Reviewer's GuideAligns Wave container shell launcher with durable installer by improving startup UX, centralizing container history handling, and making AI profile launcher installation idempotent and extended to Pi profiles. Sequence diagram for responsive Wave bench launch and idempotent AI profile installationsequenceDiagram
actor User
participant WaveLauncher
participant wave_container_shell_sh
participant Docker
participant Container
User->>WaveLauncher: launch_bench
WaveLauncher->>wave_container_shell_sh: invoke_with_block_title
alt check_only != true
wave_container_shell_sh->>WaveLauncher: printf_title_and_opening
end
wave_container_shell_sh->>Docker: docker_start(container)
wave_container_shell_sh->>Docker: ensure_container_history
Docker->>Container: mkdir_workbenches_history
Docker->>Container: touch_zsh_history
wave_container_shell_sh->>Docker: install_ai_profile_launchers
Docker->>Container: read_profile_launchers_sha256
alt installed_hash == bundle_hash
wave_container_shell_sh->>Docker: skip_docker_cp
else installed_hash != bundle_hash
Docker->>Container: docker_cp_claude_profile
Docker->>Container: docker_cp_codex_profile
Docker->>Container: docker_cp_provider_profile
Docker->>Container: docker_cp_pi_profile
Docker->>Container: write_profile_launchers_sha256
end
wave_container_shell_sh->>Docker: docker_exec_shell_with_HISTFILE
Docker->>Container: start_shell_with_history
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![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 1 issue, and left some high level feedback:
- Switching the history volume from a single file to a directory (and updating mounts and HISTFILE accordingly) is a behavioral change; double-check that any host-side logic that initializes
history_volumenow expects and creates a directory rather than just a file to avoid mismatches or stale paths. - The
install_ai_profile_launchersfunction runs several separatedocker cpanddocker execcommands; consider grouping related operations into fewerdocker execcalls to reduce overhead and make the launcher installation flow easier to follow.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Switching the history volume from a single file to a directory (and updating mounts and HISTFILE accordingly) is a behavioral change; double-check that any host-side logic that initializes `history_volume` now expects and creates a directory rather than just a file to avoid mismatches or stale paths.
- The `install_ai_profile_launchers` function runs several separate `docker cp` and `docker exec` commands; consider grouping related operations into fewer `docker exec` calls to reduce overhead and make the launcher installation flow easier to follow.
## Individual Comments
### Comment 1
<location path="scripts/wave-container-shell.sh" line_range="305-309" />
<code_context>
+ "mkdir -p '$container_history_dir' && touch '$container_history_file' && chown -R '${container_user}:${container_user}' '$container_history_dir'"
+}
+
install_ai_profile_launchers() {
local claude_launcher="$workbenches_root/base-image/files/claude-profile"
local codex_launcher="$workbenches_root/base-image/files/codex-profile"
local provider_launcher="$workbenches_root/base-image/files/provider-profile"
+ local pi_launcher="$workbenches_root/base-image/files/pi-profile"
[[ -f "$claude_launcher" ]] || return 0
</code_context>
<issue_to_address>
**issue:** Pi launcher installation is gated on the Claude launcher existing.
Because of `[[ -f "$claude_launcher" ]] || return 0`, the function stops if the Claude launcher is missing, even when `pi-profile` exists. This prevents installing Pi tooling (`pi-profile`, `ppi` symlink) unless Claude is present. If Pi should be installable independently, adjust the guard so it only wraps Claude-specific logic or add separate presence checks per launcher to avoid this coupling.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
This PR updates the Wave Terminal container-shell launcher script to make bench launches feel responsive and to better align host/container state (history + AI profile launchers), while reducing repeated work during startup.
Changes:
- Show immediate terminal title + “Opening …” message before container startup work begins.
- Persist shell history under a dedicated container directory (
~/.workbenches-history) and exportHISTFILEinto the exec/check paths. - Add
.pi-profilesmount support and introduce a cached hash marker to avoid redundantdocker cpfor AI profile launchers.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
368
to
371
| if [[ "$check_only" == true ]]; then | ||
| docker exec --user "$container_user" --workdir "$workdir" "$container" "$shell_path" -lc \ | ||
| 'printf "%s\n" "wave-container-shell-ok"; whoami; pwd; command -v claude-profile; command -v pclaude; command -v codex-profile; command -v pcodex; command -v pgemini; command -v pgrok; command -v pglm; test -d "$HOME/.claude-profiles"; test -d "$HOME/.chatgpt-profiles"; test -d "$HOME/.gemini-profiles"; test -d "$HOME/.grok-profiles"; test -d "$HOME/.glm-profiles"' | ||
| docker exec --user "$container_user" --env "HISTFILE=$container_history_file" --workdir "$workdir" "$container" "$shell_path" -lc \ | ||
| 'printf "%s\n" "wave-container-shell-ok"; whoami; pwd; test "$HISTFILE" = "$HOME/.workbenches-history/.zsh_history"; command -v claude-profile; command -v pclaude; command -v codex-profile; command -v pcodex; command -v ppi; command -v pgemini; command -v pgrok; command -v pglm; test -d "$HOME/.claude-profiles"; test -d "$HOME/.chatgpt-profiles"; test -d "$HOME/.pi-profiles"; test -d "$HOME/.gemini-profiles"; test -d "$HOME/.grok-profiles"; test -d "$HOME/.glm-profiles"' | ||
| exit 0 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
scripts/wave-container-shell.sh:371
- The
--checkcommand sequence doesn’t reliably fail when a required command/test is missing because it’s a;-separated list and the overall exit status becomes the lasttest -d .... This makes--checkreport success even when earliercommand -v ...checks fail, and it also makes the newppicheck ambiguous (sincepi-profilemay not be installed). Consider enablingset -einside the shell and only requiringppiwhenpi-profileis present.
"mkdir -p '/home/${container_user}/.local/bin' && chown '${container_user}:${container_user}' '/home/${container_user}/.local' '/home/${container_user}/.local/bin'"
docker exec --user "$container_user" "$container" sh -c \
'ln -sfn /usr/local/bin/claude "$HOME/.local/bin/claude"'
Contributor
Author
|
@codex review |
Contributor
Author
|
Addressed the review on the current head:
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
Author
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
scripts/wave-container-shell.sh:317
pi_launcherpoints at$workbenches_root/base-image/files/pi-profile, but that file does not exist in the repository (base-image/files currently contains claude-profile, codex-profile, and provider-profile only). As a result,ppiwill never be installed even though the script now advertises support for it.
Please add the missing base-image/files/pi-profile to this PR (and ensure it’s copied/linked appropriately), or remove the pi-profile branch from this script if it’s not intended to ship yet.
local claude_launcher="$workbenches_root/base-image/files/claude-profile"
local codex_launcher="$workbenches_root/base-image/files/codex-profile"
local provider_launcher="$workbenches_root/base-image/files/provider-profile"
local pi_launcher="$workbenches_root/base-image/files/pi-profile"
if [[ ! -f "$claude_launcher" \
&& ! -f "$codex_launcher" \
&& ! -f "$provider_launcher" \
&& ! -f "$pi_launcher" ]]; then
return 0
scripts/wave-container-shell.sh:381
--checkcurrently always runscommand -v ppi, which fails when the optional pi-profile launcher isn’t present/installed. This makes--checkbrittle on checkouts that don’t shipbase-image/files/pi-profileyet.
Make the ppi check conditional on the launcher file existing (or otherwise treat ppi as optional) so --check only validates what can actually be installed from the current checkout.
if [[ "$check_only" == true ]]; then
docker exec --user "$container_user" --env "HISTFILE=$container_history_file" --workdir "$workdir" "$container" "$shell_path" -lc \
'printf "%s\n" "wave-container-shell-ok"; whoami; pwd; test "$HISTFILE" = "$HOME/.workbenches-history/.zsh_history"; command -v claude-profile; command -v pclaude; command -v codex-profile; command -v pcodex; command -v ppi; command -v pgemini; command -v pgrok; command -v pglm; test -d "$HOME/.claude-profiles"; test -d "$HOME/.chatgpt-profiles"; test -d "$HOME/.pi-profiles"; test -d "$HOME/.gemini-profiles"; test -d "$HOME/.grok-profiles"; test -d "$HOME/.glm-profiles"'
exit 0
Comment on lines
+370
to
+371
| docker exec --user "$container_user" "$container" sh -c \ | ||
| 'ln -sfn /usr/local/bin/claude "$HOME/.local/bin/claude"' |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
scripts/wave-container-shell.sh:380
--checkcurrently runs a long command list wherecommand -v ...failures don’t affect the exit status, so the script can report success even when required launchers aren’t installed. If--checkis meant to validate the install, make the check command fail on missing required commands (while keepingppioptional untilpi-profileexists).
docker exec --user "$container_user" --env "HISTFILE=$container_history_file" --workdir "$workdir" "$container" "$shell_path" -lc \
'printf "%s\n" "wave-container-shell-ok"; whoami; pwd; test "$HISTFILE" = "$HOME/.workbenches-history/.zsh_history"; command -v claude-profile; command -v pclaude; command -v codex-profile; command -v pcodex; command -v ppi; command -v pgemini; command -v pgrok; command -v pglm; test -d "$HOME/.claude-profiles"; test -d "$HOME/.chatgpt-profiles"; test -d "$HOME/.pi-profiles"; test -d "$HOME/.gemini-profiles"; test -d "$HOME/.grok-profiles"; test -d "$HOME/.glm-profiles"'
scripts/wave-container-shell.sh:372
ln -sfnwill overwrite any existing$HOME/.local/bin/claude(including a real file), which can clobber user-installed tooling inside the container. Prefer only forcing the symlink when the destination is missing or already a symlink.
if [[ -f "$claude_launcher" ]]; then
docker exec --user "$container_user" "$container" sh -c \
'ln -sfn /usr/local/bin/claude "$HOME/.local/bin/claude"'
fi
Contributor
Author
|
@codex review |
Contributor
Author
|
The current head also makes |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Comment on lines
+321
to
+336
| local launchers=( | ||
| "$claude_launcher" | ||
| "$codex_launcher" | ||
| "$provider_launcher" | ||
| "$pi_launcher" | ||
| ) | ||
| local bundle_hash | ||
| bundle_hash="$( | ||
| for launcher in "${launchers[@]}"; do | ||
| if [[ -f "$launcher" ]]; then | ||
| sha256sum "$launcher" | awk '{print $1}' | ||
| else | ||
| printf '%s\n' missing | ||
| fi | ||
| done | sha256sum | awk '{print $1}' | ||
| )" |
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
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
Validation
rustBenchthrough Wave’s own widget-launch RPC/workspacein about 4.6 secondsOpeningimmediately and performs no redundant copiesSummary by Sourcery
Improve Wave container shell responsiveness, history handling, and AI profile launcher installation while keeping the host and container behavior in sync.
Enhancements: