Skip to content

Fix cd environment variable expansion#56

Merged
AkshajSinghal merged 3 commits into
TruFoundation:mainfrom
BeauDevCode:fix/cd-expandvars-45
Jun 10, 2026
Merged

Fix cd environment variable expansion#56
AkshajSinghal merged 3 commits into
TruFoundation:mainfrom
BeauDevCode:fix/cd-expandvars-45

Conversation

@BeauDevCode

@BeauDevCode BeauDevCode commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Summary:

  • Expands environment variables in the CLI cd handler before changing directories.
  • Strips trailing whitespace before path expansion so CLI cd behavior matches kernel/command dispatch normalization.
  • Keeps CLI cd behavior aligned with the kernel implementation.
  • Adds regression coverage for env-var based cd paths and trailing-whitespace handling.

Validation:

  • python -m pytest tests/test_cli_cd.py -q
  • Result: 3 passed

Related issue:

Risk:

  • Low
  • The change only affects cd path normalization and uses the same expandvars/expanduser order as the kernel.

@AkshajSinghal

Copy link
Copy Markdown
Collaborator

Before I take this out of draft:

  1. Let’s get that Python 3.12 CI check green, looks like a pre-existing Windows issue, so feel free to skip those specific tests for now.
  2. Add a # TODO comment in cli.py noting that we should eventually deprecate this stub in favor of the kernel dispatch to avoid duplicate logic.

Fix the CI status and add that note, and it’s good to go.

@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

CLI cd path normalization now expands environment variables (in addition to ~). Two tests were added: one verifies expansion of env-var targets, the other verifies trailing whitespace after an env-var target is stripped and still resolved.

Changes

cd command environment variable expansion

Layer / File(s) Summary
Environment variable expansion in cd command
trushell/cli.py
Path expansion for cd now runs os.path.expandvars(os.path.expanduser(argument.strip())); a TODO was added noting planned deprecation of the CLI cd stub in favor of kernel dispatch.
Tests for environment-variable expansion and trailing whitespace
tests/test_cli_cd.py
Adds tests: test_handle_cd_command_expands_environment_variables(...) to assert $VAR targets are expanded and test_handle_cd_command_strips_trailing_whitespace(...) to assert trailing whitespace after an env-var target is ignored.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 I sniffed the dollar-sign trail,
A hidden path in green and pale.
Expandvars hopped the gates ajar,
Now cd finds home, both near and far.
A tiny test and one small tweak — hooray, path found, hop away!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: the CLI cd handler now expands environment variables, which aligns with the core modification in the changeset.
Linked Issues check ✅ Passed The PR directly addresses issue #45 by implementing environment variable expansion in cli.py's _handle_cd_command using os.path.expandvars, ensuring consistent behavior with the kernel implementation and preventing silent failures for env-var paths.
Out of Scope Changes check ✅ Passed All changes are scoped to fixing cd environment variable expansion and adding regression tests; no unrelated modifications are present.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@BeauDevCode

Copy link
Copy Markdown
Contributor Author

Follow-up verification:

  • The # TODO note exists directly above _handle_cd_command in trushell/cli.py, calling out eventual deprecation in favor of kernel dispatch to avoid duplicate path logic.
  • python -m pytest tests/test_cli_cd.py -q passes locally: 2 passed.
  • gh pr checks currently reports only the CodeRabbit draft-status check as passing; I do not see a failing Python 3.12 CI check exposed on this PR right now, so I did not make any workflow/test-selection changes.

@BeauDevCode BeauDevCode marked this pull request as ready for review June 9, 2026 04:42

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@trushell/cli.py`:
- Line 287: Trailing whitespace on the parsed argument causes inconsistent path
expansion; update the target computation to strip whitespace from the variable
argument before calling os.path.expanduser/expandvars so it matches kernel and
commands behavior (see _split_command which can leave trailing spaces) — change
the logic that assigns target (currently using argument) to use a stripped
argument, ensuring the existing check that uses argument.strip() remains
consistent with this expansion.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 53ee447b-7803-42d9-bc42-b3c7507c119c

📥 Commits

Reviewing files that changed from the base of the PR and between 8fdfbed and 4e6c584.

📒 Files selected for processing (2)
  • tests/test_cli_cd.py
  • trushell/cli.py

Comment thread trushell/cli.py Outdated
@AkshajSinghal AkshajSinghal merged commit 4c6a359 into TruFoundation:main Jun 10, 2026
4 checks passed
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.

duplicate cd handling, kernel and cli.py both implement cd

2 participants