Skip to content

fix(binaries): ignore tool binaries found via the current directory - #4838

Open
chopratejas wants to merge 5 commits into
mainfrom
fix/binaries-skip-cwd
Open

chopratejas wants to merge 5 commits into
mainfrom
fix/binaries-skip-cwd

Conversation

@chopratejas

@chopratejas chopratejas commented Nov 13, 2026 •

Copy link
Copy Markdown
Collaborator

Description

headroom.binaries._path_lookup() (used by which(), resolve() and ensure_tools()) and status() returned shutil.which(name) before looking at the SHA-256-pinned cache. On Windows shutil.which searches the current directory before PATH (unless NoDefaultCurrentDirectoryInExePath is set), and on any OS a relative PATH entry (., an empty entry, bin) resolves against the cwd. The cwd is usually the project being worked on, so a file there named ast-grep, difft, scc, ... was run in place of the real tool, and the cache's pin never applied to it.

A PATH hit is now ignored when it comes from a relative PATH entry or lives in the current directory; lookup then falls back to the interpreter's Scripts/bin dir and the verified cache (or a pinned fetch in resolve()). status() (headroom tools doctor) uses the same rule so it no longer reports such a file as on-path.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Changes Made

  • headroom/binaries.py: new _which_outside_cwd(); _path_lookup() and status() use it. A warning names the ignored file.
  • tests/test_binaries.py: real shutil.which with PATH=. and a tool in the cwd for which(), resolve() and status(), plus a test that a normal PATH hit outside the cwd is still used.

Testing

  • Unit tests pass (pytest)
  • Linting passes (ruff check .)
  • Type checking passes (mypy headroom)
  • New tests added for new functionality
  • Manual testing performed

Test Output

# new tests on origin/main (fix stashed)
$ pytest -q tests/test_binaries.py
FAILED tests/test_binaries.py::test_which_ignores_a_tool_in_the_current_directory
FAILED tests/test_binaries.py::test_resolve_ignores_a_tool_in_the_current_directory
FAILED tests/test_binaries.py::test_status_does_not_report_a_cwd_tool_as_on_path
========================= 3 failed, 45 passed in 3.19s =========================

# with the fix
$ pytest -q tests/test_binaries.py tests/test_binaries_refusal_reaches_callers.py tests/test_bundled_tools_savings.py tests/test_cli_tools.py tests/test_offline_egress_chokepoint.py tests/test_ssl_context.py
================== 286 passed, 4 skipped in 156.19s (0:02:36) ==================

$ ruff check / ruff format --check (0.17.3) on changed files
All checks passed!
2 files already formatted
$ mypy headroom/binaries.py --ignore-missing-imports
Success: no issues found in 1 source file

Real Behavior Proof

  • Environment: macOS, Python 3.12 venv; empty HEADROOM_BINARIES_CACHE, HEADROOM_BINARIES_OFFLINE=1.
  • Exact command / steps: in a directory containing an executable difft, run with PATH=.: python -c 'from headroom import binaries as b; print(b.which("difft")); print([(r["state"], r["path"]) for r in b.status() if r["tool"]=="difft"])'.
  • Observed result:
    • main: which: difft, status: [('on-path', './difft')]
    • this branch: Ignoring difft found via the current directory (.../proj/difft); using the interpreter's Scripts dir or the Headroom cache instead, which: None, status: [('missing', None)]
  • Not tested: on a Windows host (the Windows case is the same code path: shutil.which returns a relative .\name hit there).

Runtime Rollout Safety

  • Rollout-managed feature(s): none
  • Minimum rollout channel: n/a
  • Stable/default behavior changed: a tool binary that is only reachable through the current directory or a relative PATH entry is no longer used; Headroom uses its verified cached copy instead. Running Headroom from inside a directory that is itself listed on PATH also falls back to the cache for tools in that directory.
  • Kill switch / disable path: put the tool in a directory listed on PATH by absolute path.
  • Unsafe override required: no
  • Qualification impact: none
  • Rollback path: revert this PR

Review Readiness

  • I have performed a self-review
  • This PR is ready for human review

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I did not edit CHANGELOG.md

Additional Notes

No docs change needed. Fix adapted from a proposal by HetCreep (this version also ignores hits from relative PATH entries other than .).

🤖 Generated with Claude Code


Devin Review

which(), resolve() and status() took shutil.which()'s first hit. On
Windows that searches the current directory before PATH, and a relative
PATH entry such as "." does the same elsewhere, so a same-named file in
the project being worked on was used instead of the real or cached tool.
Such hits are now ignored and lookup falls back to the interpreter's
Scripts dir and the SHA-256-verified cache.

Assay-Kind: bugfix
Assay-Ask: Headroom's tool lookup could pick up an ast-grep/difft/scc binary from the current working directory before the pinned cached one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@chopratejas chopratejas added bug Something isn't working size: small At most 100 added/deleted lines. labels Nov 13, 2026
@chopratejas chopratejas added risk: low Scope estimate: documentation/tests-only change; verification still required. area: cli Touches cli files; triage routing, not a CODEOWNERS assignment. bug Something isn't working size: small At most 100 added/deleted lines. labels Nov 13, 2026
@github-actions

Copy link
Copy Markdown
Contributor

PR governance

This PR does not yet satisfy the required template fields:

  • Fill in Real Behavior Proof → Observed result.

Please update the PR body, or move the PR back to draft while it is still in progress.

@github-actions github-actions Bot added status: needs author action Pull request body or readiness checklist still needs author updates status: ci failing Required or reported CI checks are failing labels Nov 13, 2026
devin-ai-integration[bot]

This comment was marked as resolved.

resolve() runs for every tool result in the proxy, so the warning about
an ignored tool in the current directory repeated on every request.

Assay-Kind: bugfix
Assay-Ask: The warning about ignoring a tool binary in the current directory repeated on every proxied tool result.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot removed the status: ci failing Required or reported CI checks are failing label Nov 13, 2026
@codecov-commenter

codecov-commenter commented Nov 13, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 89.65517% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
headroom/binaries.py 89.65% 2 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

… hit

shutil.which() returns only the first match, so skipping a hit in the
current directory hid a tool installed in a later PATH entry. The lookup
now continues through the absolute PATH entries. status() uses the same
_path_lookup() as which()/resolve(), so doctor also sees tools in the
interpreter's Scripts dir.

Assay-Kind: bugfix
Assay-Ask: Skipping a current-directory tool hit hid the same tool installed later on PATH, and doctor ignored the interpreter's Scripts dir.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Picks up the dependency updates from #4835 so the audit checks pass.

Assay-Kind: deps
Assay-Ask: The dependency audit checks failed on this PR because the branch predates the dependency updates on main.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
devin-ai-integration[bot]

This comment was marked as resolved.

…Scripts dir

The Scripts-dir fallback in _path_lookup checked only that the path
existed, so which(), resolve() and status() accepted a non-executable
file or a directory there. It now requires a regular file with execute
permission, the same test shutil.which applies.

Assay-Kind: bugfix
Assay-Ask: The interpreter Scripts-dir fallback accepted a non-executable file as an installed tool.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@JerrettDavis JerrettDavis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The PATH-continuation fix still fails on Windows: shutil.which(name, path=entry) can prepend the current directory again. With project/difft.exe and an installed difft.exe in an absolute PATH directory, each retry returns the project executable and is discarded; with an empty cache/interpreter directory, offline resolve fails and doctor reports missing instead of using the installed tool. A CPython 3.11 Windows-branch probe reproduced helper=None with the shadow present and the installed path after removing it (simulated Windows branch on Linux, not a Windows-host test). Please search explicit absolute candidates without the implicit cwd lookup, preserve PATHEXT handling, and add a Windows regression for this case. The earlier three discussion findings are resolved; this is materially new Windows evidence for the continuation case. Hosted CI additionally reports commitlint failure; please resolve that required check. Inherited workflow provenance concerns are tracked separately and are not attributed to this PR.

This branch was successfully deployed

1 active deployment
Preview — 615d7f89 Deployed Nov 13, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: cli Touches cli files; triage routing, not a CODEOWNERS assignment. bug Something isn't working risk: low Scope estimate: documentation/tests-only change; verification still required. size: small At most 100 added/deleted lines. status: needs author action Pull request body or readiness checklist still needs author updates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants