Make Wave bench launches responsive - #2
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reviewer's GuideMakes Wave bench container launches more responsive by printing immediate startup status and avoiding redundant AI profile launcher copies via content hashing, while documenting the behavior in README and workbench widget docs. Sequence diagram for responsive Wave bench container launch and hashed AI profile synchronizationsequenceDiagram
actor User
participant wave_container_shell_sh as wave-container-shell.sh
participant Docker
User->>wave_container_shell_sh: launch bench (check_only != true)
wave_container_shell_sh->>User: printf OSC title with block_title
wave_container_shell_sh->>User: echo Opening block_title container shell...
User->>wave_container_shell_sh: install_ai_profile_launchers
wave_container_shell_sh->>wave_container_shell_sh: compute bundle_hash from launchers
wave_container_shell_sh->>Docker: docker exec cat /usr/local/share/workbenches/profile-launchers.sha256
Docker-->>wave_container_shell_sh: installed_hash
alt installed_hash == bundle_hash
wave_container_shell_sh->>User: skip docker cp profile launchers
else installed_hash != bundle_hash
wave_container_shell_sh->>Docker: docker cp claude_launcher ...
wave_container_shell_sh->>Docker: docker cp codex_launcher ...
wave_container_shell_sh->>Docker: docker cp provider_launcher ...
wave_container_shell_sh->>Docker: docker cp pi_launcher ...
wave_container_shell_sh->>Docker: docker exec write bundle_hash to marker
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey - I've found 1 issue, and left some high level feedback:
- Consider guarding the OSC title and "Opening" status print with a TTY check so non-interactive or redirected runs of
wave-container-shell.sharen’t polluted with escape sequences and status text. - The profile-launchers hash marker path (
/usr/local/share/workbenches/profile-launchers.sha256) is embedded inline; factoring it into a named constant at the top of the script would make future changes and reuse clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider guarding the OSC title and "Opening" status print with a TTY check so non-interactive or redirected runs of `wave-container-shell.sh` aren’t polluted with escape sequences and status text.
- The profile-launchers hash marker path (`/usr/local/share/workbenches/profile-launchers.sha256`) is embedded inline; factoring it into a named constant at the top of the script would make future changes and reuse clearer.
## Individual Comments
### Comment 1
<location path="bin/wave-container-shell.sh" line_range="332-333" />
<code_context>
+
+ local installed_hash
+ installed_hash="$(docker exec --user root "$container" sh -c "cat '$marker' 2>/dev/null" || true)"
+ if [[ "$installed_hash" == "$bundle_hash" ]]; then
+ return 0
+ fi
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Skipping the whole installer when hashes match may leave user-level symlinks out of sync.
The early return on matching `installed_hash` prevents creation of the `$HOME/.local/bin/claude` symlink. If the marker exists and binaries are unchanged but the user-level symlink was removed or corrupted, the installer will now exit without restoring it.
To avoid this drift, keep the hash check but limit the skip to the heavy `docker cp`/chmod/ln steps, and always run the user-level symlink creation so the CLI entry point is consistently present.
Suggested implementation:
```
local installed_hash
installed_hash="$(docker exec --user root "$container" sh -c "cat '$marker' 2>/dev/null" || true)"
# If the bundle hash matches the hash recorded in the container, we can skip
# re-copying binaries into the container, but we still want to ensure that
# user-level symlinks are present and correct.
local skip_container_install=false
if [[ "$installed_hash" == "$bundle_hash" ]]; then
skip_container_install=true
fi
```
You will also need to:
1. Wrap the heavy container install steps (the `docker cp` of the bundle into the container, any `chmod` calls in the container, and container-level `ln` commands) in a conditional `if [[ "$skip_container_install" != true ]]; then ... fi` so they are skipped when hashes match.
2. Ensure the logic that creates or refreshes the user-level symlink (e.g. `ln -sf "$claude_launcher" "$HOME/.local/bin/claude"` or similar) is *not* inside the `skip_container_install` guard, so it always runs regardless of the hash comparison.
3. If any existing early `return` was used elsewhere to skip both container install and symlink creation on matching hashes, remove or adjust it so that only the container work is skipped while the user-level symlink creation still executes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR improves perceived responsiveness when launching Wave workBench containers by printing startup status immediately and avoiding redundant profile-launcher synchronization when nothing has changed.
Changes:
- Print an immediate
Opening ...status (and set the terminal title) when starting a bench shell. - Cache a combined SHA-256 hash of AI profile launcher scripts inside the container to skip repeated
docker cpoperations when unchanged. - Document the new startup/status and synchronization behavior in the README and workbenches widget docs.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| README.md | Documents immediate startup status + hash-based skipping of profile-launcher copies. |
| docs/workbenches-widgets.md | Documents Opening ... status and avoids repeated docker cp via combined content hash. |
| bin/wave-container-shell.sh | Adds immediate “Opening …” feedback and introduces a marker-based hash check to skip redundant launcher sync. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
|
@codex review |
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
Author
|
Addressed the review on the updated head:
Validation deliberately replaced that symlink with a broken target, ran the cached path, and confirmed it was repaired without any |
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
wsh launchpath/workspacein about 4.6 secondsOpeningimmediately and performed no repeateddocker cpoperationsSummary by Sourcery
Improve Wave bench launcher responsiveness by providing immediate startup feedback and avoiding redundant profile launcher synchronization.
New Features:
Enhancements: