Better tracking of shells and terminal colors, and other internal improvements #970
Conversation
📝 WalkthroughWalkthroughThis pull request updates multiple aspects of the system. It adjusts configuration and ignore files to toggle summary generation and refine file exclusion rules. Documentation and release notes are revised with updates, additions, and deprecations of various environment variables and mount options for Geodesic v4. A Debian Dockerfile is enhanced with a new Rust build stage for installing terminal-related tools. Several shell and environment scripts have been reworked to standardize variable names and improve terminal theme detection and shell tracking. Additionally, a new Rust package for terminal theme detection and its associated license file are introduced in the colorsaurus component. Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (18)
rootfs/templates/wrapper-body.sh (4)
255-258: Array Population in_running_shell_count()
The function collects PIDs via command substitution into an array using syntax like:local count=($(_running_shell_pids || true))While this works in many cases, it can be fragile when output contains whitespace. Consider using a more robust approach (e.g. using
mapfile -tif available) to ensure safe splitting.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 256-256: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
318-360: Robust Array Processing inrun_exit_hooks()
The function uses command substitution to populate arrays (e.g. at lines 319 and 330) which can be error prone when outputs contain whitespace or special characters. It is recommended to use techniques such asmapfile -t(if using Bash ≥ 4) or proper IFS handling to avoid potential splitting issues.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 318-318: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 319-319: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 330-330: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
400-404: Use of Positional Parameters indocker execCommands
In the branches where the script callsdocker exec, the unquoted use of$*is observed. To preserve argument boundaries and protect against word splitting, it is recommended to replace$*with"$@". For example:- docker exec --env G_HOST_PID=$WRAPPER_PID --detach-keys "ctrl-^,ctrl-[,ctrl-@" "${DOCKER_EXEC_ARGS[@]}" "${DOCKER_NAME}" $* + docker exec --env G_HOST_PID="$WRAPPER_PID" --detach-keys "ctrl-^,ctrl-[,ctrl-@" "${DOCKER_EXEC_ARGS[@]}" "${DOCKER_NAME}" "$@"Apply a similar change in the other branch (around line 607).
Also applies to: 607-611
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 403-403: Use "$@" (with quotes) to prevent whitespace problems.
(SC2048)
256-610: General Shellcheck and Quoting Improvements
Several static analysis warnings suggest improvements that would increase robustness:
- Word Splitting & Array Handling: Use constructs like
mapfile -torread -a(instead of bare command substitution) when populating arrays (e.g. in lines 256, 319, and 330).- Brace Expansion with Variables: Avoid using variable expansions in brace ranges (line 276) and switch to arithmetic for loops as suggested above.
- Quoting Variables: Ensure variables (e.g. in docker exec calls and environment assignments) are consistently quoted to prevent unwanted word splitting or globbing.
These changes will help mitigate potential runtime issues across different environments and Bash versions.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 256-256: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 276-276: Bash doesn't support variables in brace range expansions.
(SC2051)
[warning] 318-318: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 319-319: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 330-330: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 384-384: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 403-403: Use "$@" (with quotes) to prevent whitespace problems.
(SC2048)
[warning] 421-421: docker_install_prompt is referenced but not assigned.
(SC2154)
[warning] 486-486: Expanding an array without an index only gives the first element.
(SC2128)
[warning] 582-582: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 582-582: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 584-584: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 595-595: Use "$@" (with quotes) to prevent whitespace problems.
(SC2048)
[warning] 610-610: Use "$@" (with quotes) to prevent whitespace problems.
(SC2048)
rootfs/usr/local/sbin/shell-monitor (5)
5-5: Leverage local arrays if scope is limited
wrapper_pidsis declared globally. If you do not require it elsewhere, consider declaring it withlocalwithin the functions to limit scope.
10-12: Consider returning instead of echoing
count_shells()echoes the count, which you parse externally. Returning the count directly, or setting a global or environment variable, could simplify usage.
31-54: Consolidate repeated kill logic
Thekill_shells()function uses multiple loops to send different signals. This is valid, but you could reduce repetition by iterating once over each signal:for signal in HUP TERM KILL; do ....
61-69: Make timeout configurable
You have a hard-coded 60-second timeout before exiting if no shells connect. Consider using an environment variable to allow users or administrators to adjust the waiting period.
86-90: Reassess the 0.67s sleep interval
Sleeping for 0.67 seconds is precise but may be confusing for future maintainers. If sub-second responsiveness is desired, consider using 0.5 for consistency with the earlier wait logic.src/colorsaurus/lib/display.rs (1)
11-15: Be aware of potential differences between Display and Debug
Here,Debugformatting is delegated directly to the type’sDisplayimplementation. This is safe if you want consistent textual representations, but you may miss typical “debug info” often included inDebug. Ensure this is intentional.src/colorsaurus/src/main.rs (1)
17-21: Consider using standard 2-digit hex format for RGB values.The current implementation uses 4 digits per color component (
:04x). Standard RGB hex format typically uses 2 digits per component.Apply this diff to use standard RGB hex format:
- "{:04x}/{:04x}/{:04x};{:04x}/{:04x}/{:04x}", + "{:02x}/{:02x}/{:02x};{:02x}/{:02x}/{:02x}",rootfs/etc/profile.d/_07-term-mode.sh (2)
82-114: Terminal luminance retrieval function.
The_get_terminal_luminance()function now correctly handles forced modes (“light” or “dark”) by returning fixed luminance values. For dynamic detection, it callsterminal-theme-detectorwith a timeout, then computes luminance using the standard sRGB formula.A minor nitpick: a brief inline comment on why the constant
1000000000is used (for scaling to integer precision) would aid future maintainers.🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 93-93: Argument mixes string and array. Use * or separate argument.
(SC2145)
231-301: Retain legacy OSC query function as reference.
The_raw_query_term_mode()function is preserved as a reference for OSC queries. Although it is no longer used in primary detection (thanks to the new terminal-colorsaurus library), leaving it here helps future developers understand the evolution of the implementation. Consider marking it as deprecated in an inline comment to avoid confusion.ReleaseNotes-v4.md (5)
247-256: Clarify and polish the TF_PLUGIN_CACHE_DIR documentation.
The new documentation clearly explains that settingTF_PLUGIN_CACHE_DIRto an empty string,"false", or"disabled"will unset the variable for users who wish to opt out of the plugin cache. However, the phrase “valuable saver of time and bandwidth” is somewhat awkward. Consider rewording it to something like “this saves time and bandwidth” for improved clarity and professionalism.
541-552: Enhance clarity in the new customization command-line options section.
The section describing new command-line options (such as--no-custom,--trace, and--dark/--light) is informative. However, the description for--trace="custom terminal histappears to be truncated. Please review and complete the description to ensure it fully conveys its intent. Additionally, providing a brief example or further clarification for these options could help users understand their usage better.🧰 Tools
🪛 LanguageTool
[style] ~548-~548: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...zations, terminal configuration (mainly with respect to light and dark mode support), and deter...(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
594-595: Rephrase repetitive sentences in the theme setting instructions.
The two sentences—“If you provide an argument, it will set the terminal theme to that argument. If you do not provide an argument, it will attempt to determine the terminal theme itself.”—start similarly and sound repetitive. Consider rewording one (or both) to vary the sentence structure and improve readability.
580-587: Review grammar in the experimental theme detection section.
Static analysis flagged a grammar issue around this segment (line ~580) regarding the use of articles before a countable noun. Please review the sentences in this experimental theme detection block to ensure that a proper article or possessive pronoun is present where needed for clarity.🧰 Tools
🪛 LanguageTool
[grammar] ~580-~580: Before the countable noun ‘in’ an article or a possessive pronoun is necessary.
Context: ...ns.sh` file. #### Detecting changes in theme while running (experimental) You may h...(IN_NN_CC_VBG)
723-725: Correct a minor typo in environment variable descriptions.
In the descriptions forGEODESIC_TERM_THEMEandGEODESIC_TERM_THEME_AUTO, the phrase “light/dark them detection” should be corrected to “light/dark theme detection.” This small update will improve the clarity and professionalism of the release notes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/colorsaurus/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.coderabbit.yaml(1 hunks).dockerignore(2 hunks).gitignore(2 hunks)ReleaseNotes-v4.md(6 hunks)docs/environment.md(2 hunks)os/debian/Dockerfile.debian(2 hunks)rootfs/etc/profile.d/_07-term-mode.sh(3 hunks)rootfs/etc/profile.d/_10-colors.sh(7 hunks)rootfs/etc/profile.d/_60-register-stray-shells.sh(1 hunks)rootfs/etc/profile.d/prompt.sh(1 hunks)rootfs/etc/profile.d/terraform.sh(1 hunks)rootfs/templates/wrapper-body.sh(12 hunks)rootfs/usr/local/bin/terminal-colorsaurus-license.txt(1 hunks)rootfs/usr/local/sbin/list-wrapper-shells(1 hunks)rootfs/usr/local/sbin/shell-monitor(1 hunks)src/colorsaurus/Cargo.toml(1 hunks)src/colorsaurus/lib/display.rs(1 hunks)src/colorsaurus/src/main.rs(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- rootfs/usr/local/bin/terminal-colorsaurus-license.txt
- .gitignore
- src/colorsaurus/Cargo.toml
🧰 Additional context used
🪛 Shellcheck (0.10.0)
rootfs/etc/profile.d/_60-register-stray-shells.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
rootfs/etc/profile.d/_07-term-mode.sh
[error] 93-93: Argument mixes string and array. Use * or separate argument.
(SC2145)
[warning] 122-122: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
rootfs/templates/wrapper-body.sh
[warning] 256-256: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 276-276: Bash doesn't support variables in brace range expansions.
(SC2051)
[warning] 318-318: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 319-319: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 330-330: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 384-384: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 403-403: Use "$@" (with quotes) to prevent whitespace problems.
(SC2048)
[warning] 486-486: Expanding an array without an index only gives the first element.
(SC2128)
[warning] 582-582: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 582-582: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 584-584: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 595-595: Use "$@" (with quotes) to prevent whitespace problems.
(SC2048)
[warning] 610-610: Use "$@" (with quotes) to prevent whitespace problems.
(SC2048)
🪛 LanguageTool
docs/environment.md
[style] ~39-~39: To form a complete sentence, be sure to include a subject.
Context: ... dark themes. - GEODESIC_MOTD_ENABLED can be set to "false" to disable printing t...
(MISSING_IT_THERE)
ReleaseNotes-v4.md
[grammar] ~580-~580: Before the countable noun ‘in’ an article or a possessive pronoun is necessary.
Context: ...ns.sh` file. #### Detecting changes in theme while running (experimental) You may h...
(IN_NN_CC_VBG)
[style] ~596-~596: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...et the terminal theme to that argument. If you do not provide an argument, it will...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-push (debian)
🔇 Additional comments (29)
.dockerignore (2)
27-28: New entry for terminal colorsaurus target directory added.
The new linesrc/colorsaurus/target/**ensures that compiled/generated files in the specified directory are excluded from the Docker image. This change maintains consistency with the similar update in the.gitignorefile.
46-47: $RECYCLE.BIN entry correction.
The adjustment to the$RECYCLE.BINignore pattern ensures that this directory is consistently ignored in Docker builds, aligning with the corrections made in the.gitignorefile.rootfs/templates/wrapper-body.sh (4)
120-125: Add Terminal Theme Options (--darkand--light)
The new options in theparse_args()function correctly exportGEODESIC_TERM_THEMEas "dark" or "light". This change clearly documents the user’s intent to override automatic terminal color detection. Consider adding tests or updating the documentation accordingly.
182-192: Update Help Message for Terminal Theme Settings
The enhancements to thehelp()function now include clear instructions for the--darkand--lightoptions. This makes the UI more informative and aligns well with the new flag handling in argument parsing.
247-249: Introduce_running_shell_pids()Function
The new helper function usingdocker execto list running shell PIDs is straightforward. Ensure that the external command (list-wrapper-shells) reliably returns the expected PID list.
251-253: Introduce_our_shell_pid()Function
This function neatly wraps a call todocker execfor retrieving the current shell’s PID. The inclusion of|| truehelps guard against command failures.rootfs/usr/local/sbin/shell-monitor (3)
3-3: Ensure the source path is valid
Sourcinglist-wrapper-shellshere is fine, but consider verifying error handling or fallback if the script is missing or unreadable.
14-28: Validate the exit-checking logic
Theshell_has_exited()function immediately returns once it finds any PID that is missing from/proc. Ensure this is your intended behavior if multiple shells may exit at various times. Consider updatingwrapper_pidsto remove exited shells to maintain accurate state.
74-83: Helpful inline documentation
These detailed comments about exit timing are insightful and clarify the script's logic. Thank you for including them.src/colorsaurus/lib/display.rs (1)
3-9: Neat newtype for bridging Display and Debug
Wrapping types asDisplayAsDebug<T>is a creative way to unifyDisplayoutput with a debug interface. This approach can reduce duplication for simple types.rootfs/usr/local/sbin/list-wrapper-shells (2)
6-15: Validate approach to parsing /proc
This method of filtering processes by scanning/proc/$pid/environis effective in many environments. However, in heavily locked-down or containerized situations,/proc/$pid/environmay be inaccessible. Confirm this approach covers edge cases where advanced security restrictions might be in place.
19-19: Graceful handling when sourcing
Allowing this script to be either sourced or executed is a good design approach. This dual usage avoids redundant calls yet remains user-friendly when used as an independent tool.src/colorsaurus/src/main.rs (1)
7-12: Verify if the commented code should be removed.The code contains commented-out logic for color scheme detection. If this functionality is no longer needed, consider removing it to improve code clarity.
rootfs/etc/profile.d/_60-register-stray-shells.sh (1)
20-23: LGTM!The shell tracking logic is well-implemented:
- Correctly identifies shells launched by
docker execusing PPID check.- Properly handles the initial shell case.
- Uses clear and descriptive output message.
rootfs/etc/profile.d/terraform.sh (1)
48-56: LGTM!The implementation of the opt-out functionality for
TF_PLUGIN_CACHE_DIRis well-designed:
- Handles various opt-out values: empty string, "false", "disabled".
- Properly unsets the variable when opted out.
- Gracefully handles directory creation failure.
.coderabbit.yaml (1)
8-8: LGTM!The configuration changes are valid and align with the schema:
- Disabling high-level summary in PR description.
- Enabling high-level summary in walkthrough.
Also applies to: 10-10
rootfs/etc/profile.d/prompt.sh (1)
54-60: Refactor terminal update logic to use theme detection instead of color mode.
The previous variable used for color updates has been replaced by a new check that looks at whether automatic terminal theme detection is enabled. Setting[[ ${GEODESIC_TERM_THEME_AUTO} == "enabled" ]] && export GEODESIC_TERM_THEME_UPDATING="needed"and then later calling
auto-update-terminal-themeis clear and concise. Please verify that all corresponding calls in other scripts have been updated accordingly.docs/environment.md (1)
37-38: Document new terminal theme environment variables.
The new entries forGEODESIC_TERM_THEMEandGEODESIC_TERM_THEME_AUTOare clearly explained and align with the updated detection logic. Ensure that their intended defaults (i.e. defaulting to light when not explicitly set) are consistent with the behavior in the shell scripts.rootfs/etc/profile.d/_07-term-mode.sh (3)
50-67: Robust integration of terminal-theme-detector in query verification.
Within the_verify_terminal_queries_are_supported()function, you now:
- Use
tput colorsto determine terminal capabilities.- Fall back by exporting
GEODESIC_TERM_THEME_AUTO=unsupportedwhen either (a) the terminal does not support colors or (b) the external binary (terminal-theme-detector) fails.This enhances robustness by clearly flagging unsupported environments. One suggestion is to consider caching the outcome if repeated calls are expected.
80-80: Conditional startup verification.
By checking[[ "${GEODESIC_TERM_THEME}" == "light" ]] || [[ "${GEODESIC_TERM_THEME}" == "dark" ]] || _verify_terminal_queries_are_supportedyou ensure that the automatic detection is only attempted when needed. Please verify that cases where GEODESIC_TERM_THEME is not set are handled as intended.
121-163: Determine terminal theme mode based on luminance.
The_is_term_dark_mode()function clearly compares the foreground and background luminance values and maps them to “dark”, “light”, or “unknown”. Defaulting to “unknown” (which later falls back to light mode) preserves historical behavior and avoids abrupt changes.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 122-122: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
rootfs/etc/profile.d/_10-colors.sh (6)
13-32: Rename and update terminal theme update function.
The function formerly known asupdate-terminal-color-mode()is nowupdate-terminal-theme(). The handling of explicit modes (“dark” or “light”) and the fallback when no valid mode is provided is clear. The updated usage message helps users understand the expected arguments.🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 19-19: Couldn't parse this case item. Fix to allow more checks.
(SC1073)
[error] 23-23: Expected a command. Fix any mentioned problems and try again.
(SC1072)
67-76: Introduce set-terminal-theme for manual overrides.
The newset-terminal-theme()function neatly encapsulates user-initiated theme changes. By checking for valid arguments ("dark" or "light") and otherwise unsetting the override to fall back on automatic detection, this design upholds the separation of manual and auto-detection logic.
78-80: Provide a getter for the current terminal theme.
Theget-terminal-theme()function returns the cached theme state (defaulting to “light” if not explicitly set). This helper is consistent with the overall design and makes theme state queries straightforward.
322-333: Automatic terminal theme update handling is solid.
Inauto-update-terminal-theme(), the function checks that automatic updates are enabled and temporarily locks the update state withGEODESIC_TERM_THEME_UPDATING. In the event of a failure (detected by the exit code 9), it marks the automatic mechanism as failed. This prevents repeated, futile update attempts.
341-345: Improved SIGWINCH handler for theme updates.
The_update-terminal-theme-sigwinch()function replaces its color-mode predecessor, simply settingGEODESIC_TERM_THEME_UPDATINGto “needed”. This streamlined approach avoids running full updates inside the signal handler and defers them to the prompt hook.
347-362: Integrate new SIGWINCH trap with any existing handlers.
The updated trap setup preserves any preexisting SIGWINCH traps while appending the new theme update call. This careful combination helps prevent conflicts with other signal-dependent functionality.os/debian/Dockerfile.debian (2)
25-29: Introduce multi-stage Rust build for terminal-theme-detector.
A new build stage using a Rust base image is added to compile theterminal-theme-detectorbinary. Copying the/src/colorsaurusdirectory, setting the working directory, and runningcargo installto place the binary in/usr/localis a clean approach. Please verify that dependencies (including theterminal-colorsauruslibrary) are correctly specified in Cargo.toml.
165-167: Copy terminal-theme-detector binary into the final image.
The commandCOPY --from=rust /usr/local/bin/terminal-theme-detector /usr/local/bin/terminal-theme-detectorensures that the detector is present in the final Debian image. This integration is essential for the new automatic terminal theme detection and looks correct.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
rootfs/etc/profile.d/_07-term-mode.sh (3)
95-98: Improve Logging of Function Stack
In the block where the terminal mode is forced, the trace message:
_terminal_trace "Function stack is ${FUNCNAME[@]}."
may trigger a shellcheck warning due to mixing string and array. Consider using"${FUNCNAME[*]}"instead to safely expand the array.🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 98-98: Argument mixes string and array. Use * or separate argument.
(SC2145)
127-127: Safer Array Splitting for Luminance Values
At the start of the_is_term_dark_mode()function, the command
local lum=($(_get_terminal_luminance))
relies on word splitting which can be fragile. It would be preferable to use a more robust approach (e.g.,read -a lum <<< "$(_get_terminal_luminance)") to ensure correct parsing of the output.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 127-127: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
235-306: _raw_query_term_mode() as a Reference Implementation
The new helper function_raw_query_term_mode()is well annotated and serves as a useful reference for OSC-based color queries. If this function is not actively used in the new detection logic, consider deprecating it (or clearly marking it as legacy) in future releases to reduce maintenance overhead.ReleaseNotes-v4.md (2)
541-549: Enhanced Documentation for New Command-Line Options
The added list of new customization options is very informative. As a minor stylistic suggestion, consider rephrasing “with respect to light and dark mode support” to a more concise form (e.g. “for light/dark mode support”) to improve readability.🧰 Tools
🪛 LanguageTool
[style] ~548-~548: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...zations, terminal configuration (mainly with respect to light and dark mode support), and the d...(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
605-606: Minor Grammatical Improvement in Signal Description
In the sentence “Geodesic also sends this signal to its shell when it detects change in the size of the window,” consider adding an article for clarity (e.g. “when it detects a change in the size of the window”).🧰 Tools
🪛 LanguageTool
[uncategorized] ~606-~606: Possible missing article found.
Context: ...his signal to its shell when it detects change in the size of the window. You can set ...(AI_HYDRA_LEO_MISSING_A)
rootfs/templates/wrapper-body.sh (2)
256-257: Refine Shell Count Retrieval in_running_shell_countCurrently, the count is built via a command substitution that may be prone to word-splitting issues. For improved robustness, consider using
readarray(or its synonymmapfile) to capture the output as an array. For example:- local count=($(_running_shell_pids || true)) - echo "${#count[@]}" + readarray -t count < <(_running_shell_pids || true) + echo "${#count[@]}"This change improves readability and maintains predictable behavior in diverse shell environments.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 256-256: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
318-320: Capture Running Shell PIDs inrun_exit_hooksWithin
run_exit_hooks(), the code assigns the output of_our_shell_pidand_running_shell_pidsto variables. As with the_running_shell_count()function, using simple command substitution for array creation may be fragile when handling whitespace.A suggestion would be to utilize
readarrayto robustly populate theshell_pidsarray. For example:- local shell_pids=($(_running_shell_pids)) + readarray -t shell_pids < <(_running_shell_pids)This change helps avoid potential splitting issues.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 318-318: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 319-319: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignore(2 hunks)ReleaseNotes-v4.md(5 hunks)rootfs/etc/profile.d/_07-term-mode.sh(3 hunks)rootfs/templates/wrapper-body.sh(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🪛 Shellcheck (0.10.0)
rootfs/templates/wrapper-body.sh
[warning] 256-256: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 318-318: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 319-319: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 330-330: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 384-384: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 403-403: Use "$@" (with quotes) to prevent whitespace problems.
(SC2048)
[warning] 486-486: Expanding an array without an index only gives the first element.
(SC2128)
[warning] 582-582: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 582-582: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 584-584: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 595-595: Use "$@" (with quotes) to prevent whitespace problems.
(SC2048)
[warning] 610-610: Use "$@" (with quotes) to prevent whitespace problems.
(SC2048)
rootfs/etc/profile.d/_07-term-mode.sh
[error] 98-98: Argument mixes string and array. Use * or separate argument.
(SC2145)
[warning] 127-127: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
🪛 LanguageTool
ReleaseNotes-v4.md
[style] ~548-~548: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...zations, terminal configuration (mainly with respect to light and dark mode support), and the d...
(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
[grammar] ~580-~580: Before the countable noun ‘in’ an article or a possessive pronoun is necessary.
Context: ...ns.sh` file. #### Detecting changes in theme while running (experimental) You may h...
(IN_NN_CC_VBG)
[style] ~596-~596: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...et the terminal theme to that argument. If you do not provide an argument, it will...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~606-~606: Possible missing article found.
Context: ...his signal to its shell when it detects change in the size of the window. You can set ...
(AI_HYDRA_LEO_MISSING_A)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-push (debian)
🔇 Additional comments (20)
rootfs/etc/profile.d/_07-term-mode.sh (5)
9-38: Detailed Documentation on Theme Detection Modes
The new comment block clearly explains the two different auto‐detection approaches (initial startup vs. runtime changes) with ample detail. This helps future readers understand the rationale and configuration options.
52-53: Clarification ontputLimitations
The added comments explain that even if the terminal supports color,tputmight not, and thus a fallback to a monochrome mode is needed. This extra context is useful.
59-59: Explicit Fallback When Color Detection Fails
SettingGEODESIC_TERM_THEME_AUTO=unsupportedimmediately when the terminal isn’t supported is a clear and appropriate fallback mechanism.
63-67: Robust Check with External Detector
The new conditional block using/usr/local/bin/terminal-theme-detectorenhances reliability by verifying if the external utility works. Consider, as a follow‐up, logging any error details returned by the detector for easier debugging.
80-80: Concise Conditional Invocation for Terminal Verification
The compound condition checking for a forced theme ("light"or"dark") before invoking_verify_terminal_queries_are_supportedis both concise and effective.ReleaseNotes-v4.md (4)
247-256: Clear Explanation of TF_PLUGIN_CACHE_DIR Opt-Out Mechanism
The revised section explains how Geodesic now handles the Terraform Provider Plugin Cache, including the opt-out mechanism by unsetting the variable. The description is clear and aligns well with the PR objectives.
592-597: Clear Usage Instructions for Setting Terminal Theme
The instructions on how to use theset-terminal-theme [light|dark]command are clear and direct. They effectively communicate both the behavior when an argument is provided and when it is not.🧰 Tools
🪛 LanguageTool
[style] ~596-~596: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...et the terminal theme to that argument. If you do not provide an argument, it will...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
601-603: Effective Query Mechanism for Cached Terminal Theme
The snippet showing how to useget-terminal-themeto query the cached theme setting is straightforward and informative.
723-725: Documentation of New Environment Variables for Theme Configuration
The update clearly documents the use and expected behavior ofGEODESIC_TERM_THEMEandGEODESIC_TERM_THEME_AUTO. This ensures users understand how to disable automatic theme detection or enable experimental auto-detection.rootfs/templates/wrapper-body.sh (11)
120-125: Add New Terminal Theme Options in Argument ParsingThe new
--darkand--lightoptions are cleanly implemented by setting theGEODESIC_TERM_THEMEenvironment variable accordingly. This enhancement aligns the CLI with the updated documentation and provides users with manual control of the terminal theme.
181-193: Enhance Help Message with Theme OptionsThe help output has been updated to list the new terminal theme options (
--darkand--light) along with their descriptions. This improvement improves discoverability and user guidance.
247-249: Introduce_running_shell_pidsHelper FunctionThe new
_running_shell_pids()function encapsulates the logic for retrieving running shell PIDs via a Docker exec call. Its minimal design is appropriate for its purpose.
251-253: Introduce_our_shell_pidHelper FunctionThe newly added
_our_shell_pid()function effectively retrieves the PID corresponding to our shell by using Docker exec with the$WRAPPER_PIDvariable. The fallback (|| true) ensures that errors during retrieval are safely suppressed.
271-304: Implement Robust Container Exit Waiting LogicThe new
wait_for_container_exit()function uses a C-style loop (for (( i=0; i<=n; i++ ))) that replaces the problematic brace expansion, which is a more portable and reliable construct. The loop checks for the disappearance of the container and handles cases where other shells continue running.A couple of points to consider:
- Verify that the condition combining
[_our_shell_pid]with_running_shell_counttruly captures the intended exit scenarios.- The sleep interval (
0.4seconds per iteration) is hardcoded; if shell exit latency might vary, consider parameterizing the wait time for greater flexibility.
343-359: Wait for Shell Exit with Timeout inrun_exit_hooksThe logic that waits for our shell to finish exiting is clear: it loops while
_our_shell_pidreturns a value, with a timeout of 15 iterations and an initial delay. This approach provides feedback to the user (via a debug message) and eventually callswait_for_container_exit().Consider logging additional context if timeouts occur frequently, although the current implementation is functionally sound.
366-367: Store the Wrapper PID for Later UseStoring the current shell’s PID in
WRAPPER_PIDusing$$is correct and makes the later Docker exec calls traceable to the proper shell process.
400-400: Conditionally Add Interactive Flag for Docker ExecThe check
[ -t 0 ] && DOCKER_EXEC_ARGS+=(-it)is a nice touch to ensure that interactive terminals get the appropriate flag.
543-549: Consolidate Workspace Mounting in Docker Launch ArgumentsThe block that builds the
DOCKER_LAUNCH_ARGSfor mounting the workspace is well structured. The concatenation ofmount_dirwith the host directory is straightforward, but please verify thatmount_diris always set appropriately (even if empty) so that the constructed container paths are as intended.
575-575: Include Host Mounts Environment VariableAppending the
--env GEODESIC_HOST_MOUNTSflag toDOCKER_LAUNCH_ARGSensures that the container is aware of what host mounts are configured. This is a useful addition for later processing inside the container.
580-588: Refine Docker Launch Argument CompositionThe addition of privileges, port mappings, and environment variables in the
DOCKER_LAUNCH_ARGSblock looks comprehensive. One minor note is to ensure that the use of parameter substitution (e.g."${DOCKER_IMAGE%:*}") aligns with the intended semantics of stripping the image tag.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 582-582: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 582-582: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 584-584: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
rootfs/templates/wrapper-body.sh
Outdated
| ) | ||
|
|
||
| if [ "$ONE_SHELL" = "true" ]; then | ||
| DOCKER_NAME="${DOCKER_NAME}-$(date +%d%H%M%S)" | ||
| echo "# Starting single shell ${DOCKER_NAME} session from ${DOCKER_IMAGE}" | ||
| echo "# Exposing port ${GEODESIC_PORT}" | ||
| [ -z "${GEODESIC_DOCKER_EXTRA_ARGS}" ] || echo "# Launching with extra Docker args: ${GEODESIC_DOCKER_EXTRA_ARGS}" | ||
| docker run --name "${DOCKER_NAME}" "${DOCKER_ARGS[@]}" ${GEODESIC_DOCKER_EXTRA_ARGS} ${DOCKER_IMAGE} -l $* | ||
| docker run --name "${DOCKER_NAME}" "${DOCKER_LAUNCH_ARGS[@]}" "${DOCKER_EXEC_ARGS[@]}" ${GEODESIC_DOCKER_EXTRA_ARGS} ${DOCKER_IMAGE} -l $* |
There was a problem hiding this comment.
🛠️ Refactor suggestion
(Repeated) Ensure Proper Quoting in ONE_SHELL Branch
As mentioned earlier, please change unquoted $* in the ONE_SHELL branch command invocation to "$@" to prevent issues with argument splitting.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 595-595: Use "$@" (with quotes) to prevent whitespace problems.
(SC2048)
🛠️ Refactor suggestion
Use Quoted Argument Handling in Docker Run Command
In the ONE_SHELL branch, the Docker run command currently expands additional arguments via $*. This can lead to word-splitting issues when arguments contain spaces. A more robust solution is to use "$@" to preserve argument boundaries. For example:
- docker run --name "${DOCKER_NAME}" "${DOCKER_LAUNCH_ARGS[@]}" "${DOCKER_EXEC_ARGS[@]}" ${GEODESIC_DOCKER_EXTRA_ARGS} ${DOCKER_IMAGE} -l $*
+ docker run --name "${DOCKER_NAME}" "${DOCKER_LAUNCH_ARGS[@]}" "${DOCKER_EXEC_ARGS[@]}" ${GEODESIC_DOCKER_EXTRA_ARGS} ${DOCKER_IMAGE} -l "$@"This change ensures that each argument is correctly passed to the container.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| docker run --name "${DOCKER_NAME}" "${DOCKER_LAUNCH_ARGS[@]}" "${DOCKER_EXEC_ARGS[@]}" ${GEODESIC_DOCKER_EXTRA_ARGS} ${DOCKER_IMAGE} -l $* | |
| docker run --name "${DOCKER_NAME}" "${DOCKER_LAUNCH_ARGS[@]}" "${DOCKER_EXEC_ARGS[@]}" ${GEODESIC_DOCKER_EXTRA_ARGS} ${DOCKER_IMAGE} -l "$@" |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 595-595: Use "$@" (with quotes) to prevent whitespace problems.
(SC2048)
There was a problem hiding this comment.
Allowing word splitting is intentional
There was a problem hiding this comment.
@Nuru Understood, I'll make note of this intentional design choice. Thank you for the clarification.
✏️ Learnings added
Learnt from: Nuru
PR: cloudposse/geodesic#970
File: rootfs/templates/wrapper-body.sh:595-595
Timestamp: 2025-02-20T06:20:41.294Z
Learning: In the Geodesic wrapper script's ONE_SHELL branch, word splitting with `$*` in the Docker run command is intentional and should not be changed to `"$@"`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
rootfs/templates/wrapper-body.sh
Outdated
| [ -t 0 ] && DOCKER_EXEC_ARGS+=(-it) | ||
| # We set unusual detach keys because (a) the default first char is ctrl-p, which is used for command history, | ||
| # and (b) if you detach from the shell, there is no way to reattach to it, so we want to effectively disable detach. | ||
| docker exec --env G_HOST_PID=$$ --detach-keys "ctrl-^,ctrl-[,ctrl-@" "${DOCKER_EXEC_ARGS[@]}" "${DOCKER_NAME}" $* |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure Robust Argument Handling in Docker Exec
Similarly, in the docker exec command within the non-ONE_SHELL branch, replace $* with "$@" to preserve argument boundaries and avoid potential splitting issues.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 610-610: Use "$@" (with quotes) to prevent whitespace problems.
(SC2048)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
rootfs/templates/wrapper-body.sh (4)
256-258: Consider Robust Splitting in Shell Count Calculation
In_running_shell_count(), the linelocal count=($(_running_shell_pids || true))splits the output into an array. This construction can be fragile if the output contains unexpected whitespace. Consider using a more robust method (e.g. using
mapfile -torread -a) if your Bash version permits.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 256-256: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
272-304: Review of wait_for_container_exit() Logic
The newwait_for_container_exit()function implements a looping mechanism to periodically check for container and shell status before taking action. The logic is generally sound; however, note that resetting the loop counter (i=0) upon detecting container exit may be non-obvious to future maintainers. Consider adding a comment explaining this design choice or using a dedicated flag variable for clarity.
318-320: Array Splitting in run_exit_hooks(): Shell PID Retrieval & Filtering
Withinrun_exit_hooks(), the assignment to local variables using:local our_shell_pid=$(_our_shell_pid) local shell_pids=($(_running_shell_pids))and the subsequent filtering via:
shell_pids=($(printf "%s\n" "${shell_pids[@]}" | grep -v "^$our_shell_pid\$"))relies on word splitting that can be error‐prone if any output contains spaces or unusual characters. For improved robustness, consider using
mapfile -tor a similar mechanism to split the output based on newline delimiters.Also applies to: 327-330
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 318-318: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 319-319: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
343-356: Waiting Loop for Our Shell Exit
The loop that waits for the current shell to finish exiting (using a counter and sleep intervals) is clear and achieves its purpose. However, the repeated “wait” pattern in both this block and inwait_for_container_exit()could be refactored into a helper to avoid duplication and improve maintainability.ReleaseNotes-v4.md (5)
541-553: Customization Command-Line Options Clarity & Wordiness
This added block detailing new command-line options (such as--no-custom,--trace,--dark, and--light) is generally clear. However, note that in list item 3 the phrase “with respect to light/dark mode support” (around line 549) might be a bit wordy. Consider replacing it with a shorter alternative (for example, “regarding light/dark support”). Also, review the flow in these bullet points to ensure varied sentence beginnings, as starting consecutive sentences with “If you…” may sound repetitive.🧰 Tools
🪛 LanguageTool
[style] ~548-~548: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...zations, terminal configuration (mainly with respect to light/dark mode support), and the deter...(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
580-588: Experimental Detection of Theme Changes While Running
This section properly documents the experimental nature of automatic theme change detection and its limitations. It provides useful context about why the feature is disabled by default. Please verify the minor grammatical construction (e.g. ensuring that countable nouns following “in” are correctly preceded by an article where needed).🧰 Tools
🪛 LanguageTool
[grammar] ~580-~580: Before the countable noun ‘in’ an article or a possessive pronoun is necessary.
Context: ...ns.sh` file. #### Detecting changes in theme while running (experimental) You may h...(IN_NN_CC_VBG)
596-603: Terminal Theme Setting Behavior and Query Instructions
This segment explains what happens when an argument is provided versus when none is given, and it guides the user on how to query the cached theme withget-terminal-theme. While the explanation is clear, consider varying the sentence starters if three successive sentences begin the same way.🧰 Tools
🪛 LanguageTool
[style] ~596-~596: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...et the terminal theme to that argument. If you do not provide an argument, it will...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
614-617: Disclaimer on Theme Update Limitations
The disclaimer on the limitations and the disabled-by-default status of the automatic theme change detection is appropriately cautious. It might be beneficial to slightly rephrase for clarity, but the current wording effectively sets user expectations.
676-677: Clarification on Yellow/Magenta Usage in Light Mode
The note on how yellow is too light in light mode and is therefore replaced with magenta is informative. However, it may seem slightly contradictory to discourage using magenta when it is the substitute. A brief clarification—explaining that while the substitution happens automatically, users should avoid explicitly using magenta to prevent visual ambiguity—might improve the clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ReleaseNotes-v4.md(5 hunks)rootfs/templates/wrapper-body.sh(12 hunks)
🧰 Additional context used
🧠 Learnings (1)
rootfs/templates/wrapper-body.sh (3)
Learnt from: Nuru
PR: cloudposse/geodesic#970
File: rootfs/templates/wrapper-body.sh:595-595
Timestamp: 2025-02-20T06:20:41.753Z
Learning: In the Geodesic wrapper script's ONE_SHELL branch, word splitting with `$*` in the Docker run command is intentional and should not be changed to `"$@"`.
Learnt from: Nuru
PR: cloudposse/geodesic#961
File: rootfs/templates/wrapper-body.sh:304-304
Timestamp: 2025-01-28T06:17:36.167Z
Learning: In the Geodesic codebase, the use of `$*` instead of `"$@"` in Docker commands is intentional, particularly in wrapper scripts that pass arguments to `docker exec` and `docker run` commands.
Learnt from: Nuru
PR: cloudposse/geodesic#970
File: rootfs/templates/wrapper-body.sh:603-607
Timestamp: 2025-02-20T06:20:15.610Z
Learning: In the Geodesic wrapper script's Docker exec command, using unquoted `$*` for argument expansion is intentional to allow word splitting, rather than using `"$@"` which would preserve argument boundaries.
🪛 Shellcheck (0.10.0)
rootfs/templates/wrapper-body.sh
[warning] 256-256: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 318-318: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 319-319: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 330-330: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 384-384: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 486-486: Expanding an array without an index only gives the first element.
(SC2128)
[warning] 582-582: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 582-582: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
[warning] 584-584: Quote to prevent word splitting/globbing, or split robustly with mapfile or read -a.
(SC2206)
🪛 LanguageTool
ReleaseNotes-v4.md
[style] ~548-~548: ‘with respect to’ might be wordy. Consider a shorter alternative.
Context: ...zations, terminal configuration (mainly with respect to light/dark mode support), and the deter...
(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
[grammar] ~580-~580: Before the countable noun ‘in’ an article or a possessive pronoun is necessary.
Context: ...ns.sh` file. #### Detecting changes in theme while running (experimental) You may h...
(IN_NN_CC_VBG)
[style] ~596-~596: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...et the terminal theme to that argument. If you do not provide an argument, it will...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-push (debian)
🔇 Additional comments (15)
rootfs/templates/wrapper-body.sh (8)
120-125: Terminal Theme Options Added
The addition of the--darkand--lightoptions in theparse_args()function is clear and aligns with the PR objectives to offer manual control over terminal colors. This implementation correctly sets theGEODESIC_TERM_THEMEvariable based on user selection.
182-193: Updated Help Text for New Options
The help function now reflects the new terminal theme options (and other updates) with accurate usage instructions. This helps users understand how to set the terminal theme explicitly.
247-249: New Helper Function: _running_shell_pids()
The newly introduced_running_shell_pids()function encapsulates the logic for fetching running shell PIDs via adocker execcall. This improves the modularity of the shell management code.
251-253: New Helper Function: _our_shell_pid()
The_our_shell_pid()function is added to isolate the logic needed to retrieve the current shell’s PID. The use of a fallback with|| trueis acceptable here since the intended behavior is to avoid unexpected exits.
366-373: Setting Wrapper PID and Docker Exec Arguments
Assigning the current shell’s PID toWRAPPER_PIDand initializingDOCKER_EXEC_ARGSis straightforward. This setup is important for correlating executed shells with their wrapper process and looks correct.
591-597: Docker Run Command in ONE_SHELL Branch
In the ONE_SHELL branch, the code modifiesDOCKER_NAMEby appending a timestamp and then launches a container viadocker run. Note that whileGEODESIC_DOCKER_EXTRA_ARGSis intentionally left unquoted (as documented) to allow it to represent a list of arguments, the final command correctly uses"$@"for passing the remaining parameters. This aligns with the PR objectives and the intentional design choices communicated via learnings.
608-612: Docker Exec Command for Existing Containers
For the non-ONE_SHELL branch, the script uses a docker exec call to attach to an already running container. The construction (including the use of unusual detach keys and conditionally appending-it) is appropriate and consistent with the project’s design decisions.
543-549: Workspace Mounting and DOCKER_LAUNCH_ARGS Setup
The block that appends workspace mounting configurations toDOCKER_LAUNCH_ARGSclearly constructs the necessary docker run options for host directory mounts. The use of unquoted variables in this context (for variables likemount_dir) is acceptable given the controlled input paths. Overall, this segment is well-structured and aligns with the intended shell-to-container mapping improvements.ReleaseNotes-v4.md (7)
247-256: TF_PLUGIN_CACHE_DIR Opt‐Out Documentation
This section clearly explains the new behavior for the Terraform Provider Plugin Cache. The explanation of how Geodesic now “unsets” the variable when it is empty,"false", or"disabled"is concise and informs users how to opt out. Please double‑check that the conditions documented here match the implementation in the code.
561-571: Section on Switching Between Light and Dark Themes
The new “Switching between light and dark themes” section succinctly describes Geodesic’s basic support for terminal themes. The explanation of why different colors are used (e.g. “black in light mode and white in dark mode”) is clear. Minor stylistic improvements are possible, but overall this segment is informative and well‑structured.
572-579: Initial Terminal Theme Detection Details
This hunk explains the critical nature of detecting the terminal theme at startup and specifies how users can override the automatic detection. The instructions are straightforward.
590-595: Manual Notification of Theme Changes
The instructions for manually notifying Geodesic of a theme change using theset-terminal-theme [light|dark]command are clear and are well‑demonstrated with a formatted code block.
605-613: SIGWINCH and Automatic Theme Update Behavior
This section gives a detailed account of how Geodesic leverages theSIGWINCHsignal for detecting terminal changes and outlines the role of theGEODESIC_TERM_THEME_AUTOvariable. The explanation is thorough and should help users troubleshoot if the terminal does not respond promptly.
623-623: Named Text Color Helpers Explanation
By adding details about the provided named text color helpers and the reasoning behind avoiding certain colors, this segment is very helpful. The text clearly explains that colors like blue and magenta are avoided due to potential issues in dark mode and that yellow is substituted in light mode. This guidance should help users choose colors appropriately in their customizations.Also applies to: 635-640
723-724: Environment Variable Changes for Terminal Theme Settings
This part documents the changes toGEODESIC_TERM_THEMEandGEODESIC_TERM_THEME_AUTOcomprehensively. It clearly instructs users on when and how to set these environment variables. The clear technical specification aligns well with the update objectives.
what
--lightand--darkcommand line options to manually set terminal themeTF_PLUGIN_CACHE_DIRwhy
TERM). Previously, Geodesic mainly limited configuration to the launch of the container, and forced later shells to inherit that launch configuration. Now, to the extent practical, each shell can have different settings.docker execand the shells launch by other shells, and therefore it could not reliably report on the status of the container. Now each shell is matched with its wrapper process, and the shell monitor tracks shells according to whether or not they were launched bydocker exec, making the whole process much more reliable and efficient.TF_PLUGIN_CACHE_DIRenvironment variable to a location on the host filesystem since the feature first became available. However, the cache does not have any protection against parallel modifications, so it is not always appropriate. Previously, Geodesic did not provide a good way to opt out of having the cache configured. Now you can setTF_PLUGIN_CACHE_DIRto "false" or "disabled" and Geodesic will honor that by unsetting the variable (leaving Terraform with its default non-caching behavior) rather than forcing you into using a cache.references