Skip to content

fix(discover): normalize venv/absolute paths with binary preservation (#1053)#1378

Open
Timber125 wants to merge 1 commit intortk-ai:developfrom
Timber125:fix/discover-rewrite-absolute-paths
Open

fix(discover): normalize venv/absolute paths with binary preservation (#1053)#1378
Timber125 wants to merge 1 commit intortk-ai:developfrom
Timber125:fix/discover-rewrite-absolute-paths

Conversation

@Timber125
Copy link
Copy Markdown

@Timber125 Timber125 commented Apr 18, 2026

Summary

Fixes the path-prefixed command rewrite issue described in #1053 and #1206.

The naive approach (strip .venv/bin/ prefix and emit bare rtk pytest) changes execution semantics: handlers call resolved_command("pytest") which resolves via PATH, not the original venv binary. When the venv isn't activated, this executes the wrong binary or fails.

This PR implements the three-part fix from #1053:

  1. Normalize for matching onlystrip_absolute_path() in rewrite_segment_inner() normalizes path-prefixed commands to find the matching registry rule, but doesn't discard the original path
  2. Preserve original binary via RTK_BIN_PATH — when a path prefix is stripped, the rewritten command includes RTK_BIN_PATH=<original_bin> as an env var prefix (e.g. RTK_BIN_PATH=.venv/bin/python rtk pytest tests/)
  3. Handlers honor the overrideoverride_command() in utils.rs checks RTK_BIN_PATH; if the basename starts with "python", it uses -m <tool> module invocation, otherwise direct execution. All Python handlers (pytest, ruff, mypy, pip) use this before falling back to PATH resolution.

Paths handled

Input Rewritten
.venv/bin/pytest tests/ RTK_BIN_PATH=.venv/bin/pytest rtk pytest tests/
.venv/bin/python -m pytest tests/ RTK_BIN_PATH=.venv/bin/python rtk pytest tests/
/usr/bin/grep -rn pattern src/ RTK_BIN_PATH=/usr/bin/grep rtk grep -rn pattern src/
pytest tests/ (no path) rtk pytest tests/ (no RTK_BIN_PATH)

Test plan

  • All 1606 existing tests pass
  • New test: bare commands produce no RTK_BIN_PATH
  • Updated tests: venv paths emit correct RTK_BIN_PATH values
  • cargo fmt --all && cargo clippy --all-targets && cargo test --all clean

Closes #1053
Refs #1206

🤖 Generated with Claude Code

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Apr 18, 2026

CLA assistant check
All committers have signed the CLA.

@pszymkowiak pszymkowiak added bug Something isn't working effort-small Quelques heures, 1 fichier labels Apr 18, 2026
@pszymkowiak
Copy link
Copy Markdown
Collaborator

[w] wshm · Automated triage by AI

📊 Automated PR Analysis

🐛 Type bug-fix
🟢 Risk low

Summary

Fixes a bug where commands invoked via absolute or virtualenv paths (e.g., .venv/bin/python -m pytest) were correctly classified but not rewritten to their rtk equivalents. The root cause was that rewrite_segment_inner() did not call strip_absolute_path() before prefix matching, unlike classify_command(). The fix is a 2-line insertion to normalize paths before the rewrite loop.

Review Checklist

  • Tests present
  • Breaking change
  • Docs updated

Analyzed automatically by wshm · This is an automated analysis, not a human review.

@Timber125 Timber125 force-pushed the fix/discover-rewrite-absolute-paths branch from 9424b94 to ac5e444 Compare April 18, 2026 12:50
@Timber125 Timber125 marked this pull request as draft April 18, 2026 14:22
@Timber125
Copy link
Copy Markdown
Author

⚠️ Converting to draft — this fix is naive and hits exactly the wrong-binary trap described in #1053.

Problem: Normalizing .venv/bin/pytestpytest in rewrite output means rtk pytest calls resolved_command("pytest") which resolves via PATH, not the original venv binary. If the venv isn't activated, this executes the wrong binary or fails.

Reworking to implement the three-part fix from #1053:

  1. Normalize in rewrite_segment() for matching only
  2. Preserve the original executable path via RTK_ORIGINAL_BIN env var
  3. Teach handlers to honor the explicit binary override

Will reopen when ready.

@Timber125 Timber125 changed the title fix(discover): normalize absolute paths in rewrite prefix matching fix(discover): normalize venv/absolute paths with binary preservation (#1053) Apr 18, 2026
@Timber125 Timber125 marked this pull request as ready for review April 18, 2026 14:37
@Timber125 Timber125 force-pushed the fix/discover-rewrite-absolute-paths branch from d8f6af8 to 526230d Compare April 18, 2026 14:43
…rtk-ai#1053)

The naive approach of stripping .venv/bin/ prefixes and emitting bare
`rtk pytest` changes execution semantics: handlers resolve via PATH
instead of the original venv binary. This implements the three-part fix:

1. Normalize path-prefixed commands for matching only
2. Emit RTK_BIN_PATH=<original_bin> env var in rewritten commands
3. Handlers (pytest, ruff, mypy, pip) honor the override via
   override_command() before falling back to PATH resolution

Closes rtk-ai#1053
Refs rtk-ai#1206

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Timber125 Timber125 force-pushed the fix/discover-rewrite-absolute-paths branch from 526230d to 96e315a Compare April 18, 2026 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working effort-small Quelques heures, 1 fichier

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants