Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 9, 2025

Fixed critical path errors when $CMDER_ROOT is unset and silent cp failures when default template is missing. Optimized both shell scripts with proper validation, documentation, and bash-idiomatic conventions. Added automatic CMDER_ROOT detection from script location for CI environments.

Core Fixes

  • Empty CMDER_ROOT handling: Exit gracefully with warning instead of attempting operations on invalid paths like /config/user_profile.sh
  • Missing template check: Only copy user_profile.sh.default when it exists, preventing silent cp failures
  • Path construction safety: Defer cmder_user_profile_path assignment until after CMDER_ROOT validation
  • Automatic CMDER_ROOT detection: Added fallback to detect CMDER_ROOT from script location (vendor directory → parent) when environment variables are not set, similar to PowerShell profile behavior
# Before: Could set invalid paths
CmderUserProfilePath="${CMDER_ROOT}/config/user_profile.sh"  # CMDER_ROOT might be empty

# After: Validate first, then assign
if [ -z "$CMDER_ROOT" ]; then
    echo "Warning: CMDER_ROOT is not set. Cmder integration skipped."
    return 2>/dev/null || exit 0
fi
# ... later, after validation
cmder_user_profile_path="${CMDER_ROOT}/config/user_profile.sh"

CMDER_ROOT Detection Hierarchy

Scripts now use a three-tier detection approach:

  1. Use explicitly set $CMDER_ROOT environment variable
  2. Derive from $ConEmuDir (when running in ConEmu/Cmder)
  3. Fallback to script location using ${BASH_SOURCE[0]} (vendor directory → parent = root)

This ensures scripts work in CI environments (like GitHub Actions) without requiring explicit CMDER_ROOT configuration in .yml files.

Code Quality

  • Function documentation: Added descriptive comments for run_profile_d() and major sections
  • Naming conventions: runProfiledrun_profile_d, CmderUserProfilePathcmder_user_profile_path
  • Optimization: Replaced sed invocation with bash parameter expansion (${CMDER_ROOT%/})
  • Shellcheck compliance: Added shebangs, fixed warnings, added appropriate disable directives

Why tests.yml Doesn't Fail on cp Errors

Bash continues after command failures without set -e. Exit code reflects the last command (a successful if statement), not intermediate failures. Our validation prevents the error condition entirely.

Files changed:

  • vendor/cmder.sh: 100 → 136 lines
  • vendor/cmder_exinit: 114 → 153 lines
Original prompt

Please read, optimise, and fix https://github.com/cmderdev/cmder/blob/master/vendor/cmder.sh and https://github.com/cmderdev/cmder/blob/master/vendor/cmder_exinit

Also, apply correct naming conventions, and modify any scripts that might depend on what you modified so you don't break anything.

Considerations while you do this:

  • $CMDER_ROOT must not be empty when declaring CmderUserProfilePath otherwise it will point to a non-existant path on the root
  • Creating user startup file from user_profile.sh.default should only be done if the .default file exist
  • Some/many bash function require some one-liner comment that explain that function. It is missing for many

Currently, the tests.yml doesn't fail when cp fails in that script, i.e.

cp: cannot stat '/vendor/user_profile.sh.default': No such file or directory

Doesn't fail the tests.yml GitHub action, can you explain why is that?


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@DRSDavidSoft
Copy link
Contributor

@copilot If CMDER_ROOT is not set in the .sh script, and we weren't able to find it using ConEmu, we can find the root based on the parent directory of the script (which is likely in vendor), like how PowerShell's profile does it? Not sure about the cmder_exinit `

This is important because in an GitHub actions CI environment, the CMDER_ROOT variable might not be defined unless we explicitly do so in the .yml configuration, which we don't (and prefer not too)

The .sh scripts must be able to follow other shell profiles and find the CMDER_ROOT the same way they do

Copilot AI changed the title [WIP] Optimize and fix cmder.sh and cmder_exinit scripts Fix CMDER_ROOT validation and optimize cmder.sh/cmder_exinit Nov 9, 2025
Copilot AI requested a review from DRSDavidSoft November 9, 2025 01:36
Copilot finished work on behalf of DRSDavidSoft November 9, 2025 01:36
Copy link
Contributor Author

Copilot AI commented Nov 9, 2025

@copilot If CMDER_ROOT is not set in the .sh script, and we weren't able to find it using ConEmu, we can find the root based on the parent directory of the script (which is likely in `vendor), like ho...

Added fallback to detect CMDER_ROOT from script location (vendor directory → parent) in commit b254bad, similar to how PowerShell profile does it.

Detection hierarchy:

  1. Explicitly set $CMDER_ROOT
  2. Derive from $ConEmuDir
  3. Derive from script location using ${BASH_SOURCE[0]}

For cmder_exinit, added validation to ensure it's in the vendor directory before deriving CMDER_ROOT, since this file may be copied to external locations like /etc/profile.d/.

This now works in GitHub Actions CI without explicit CMDER_ROOT configuration.

Copilot finished work on behalf of DRSDavidSoft November 9, 2025 01:41
@DRSDavidSoft DRSDavidSoft marked this pull request as ready for review November 9, 2025 05:39
@DRSDavidSoft DRSDavidSoft marked this pull request as draft November 9, 2025 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants