fix: remove double shell-wrapping for aliases in vt script #132
Conversation
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
- Remove -- separator from resolve_command function calls in vt script - Fix regression that prevented aliases from working properly - Add comprehensive test for ProcessUtils.resolveCommand behavior - Update fwd.ts to properly handle -- as an argument separator - Document vt CLI wrapper technical details in development guide The -- separator was causing ProcessUtils.resolveCommand to misinterpret the command array, with '--' being treated as the command name instead of a separator. This led to fallback shell execution with incorrect parameters and the 'undefined' alias error. Test demonstrates the issue and verifies correct behavior with and without the -- separator. Fixes amantus-ai#101
- Added test-pr132-fix.sh to specifically test the core fix (removing -- separator) - Added comprehensive integration test scripts for alias/function resolution - Updated CI workflow to run integration tests after unit tests - Tests verify that shell commands work correctly without the -- separator - Tests also verify that fwd.ts handles -- correctly when present
Collaborator
|
Thanks @chrisbyboston! This is super helpful! |
The integration test step was failing because it references scripts that don't exist in the base branch. The test scripts are still available for local testing but won't run in CI until they're merged to main.
This reverts commit 2b4ef5d.
The CI environment uses bash as the default shell, while local development might use zsh. Updated the tests to accept either shell to ensure they pass in all environments.
ProcessUtils.resolveCommand adds -i -l flags to interactive shell commands and preserves the command string after -c flag. Updated tests to match this actual behavior rather than testing incorrect expectations.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The tests were creating temporary files with random names but shells expect specific filenames (.bashrc, .zshrc). Now creating proper directories with correctly named config files so shells can find and load them. Also fixed grep command to handle -- properly in expected output.
The tests were expecting the wrong values. ProcessUtils correctly preserves just the command string after -c flag, not the full command array. Updated tests to expect the actual behavior where 'echo "hello"' and 'claude --dangerously-skip-permissions' are preserved as-is.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Collaborator
CI Status UpdateThe PR has been updated to fix the test failures. Current status: ✅ All web tests are passing:
❌ Mac CI: Failed (this might be due to permissions/security restrictions for external contributor PRs) Changes made to fix tests:
The core fix (removing |
ferg-cod3s
pushed a commit
to ferg-cod3s/tunnelforge
that referenced
this pull request
Sep 23, 2025
ferg-cod3s
pushed a commit
to ferg-cod3s/tunnelforge
that referenced
this pull request
Oct 26, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Fixes #101 - Restores alias functionality in the vt command by removing double shell-wrapping
Changes
Testing
pnpm run test)pnpm run lint && pnpm run typecheck)Details
The -- separator was causing ProcessUtils.resolveCommand to misinterpret the command array, with '--' being treated as the command name instead of a separator. This led to fallback shell execution with incorrect parameters and the 'undefined' alias error.
Test demonstrates the issue and verifies correct behavior with and without the -- separator.