Skip to content

fix(plugins): keep the hook launcher's Python fallback off the project dir - #4842

Open
chopratejas wants to merge 6 commits into
mainfrom
fix/plugin-launcher-isolated-python
Open

chopratejas wants to merge 6 commits into
mainfrom
fix/plugin-launcher-isolated-python

Conversation

@chopratejas

@chopratejas chopratejas commented Nov 13, 2026 •

Copy link
Copy Markdown
Collaborator

Description

plugins/headroom-agent-hooks/bin/headroom-hook.sh looks for a headroom executable and, if none is found, falls back to "$python" -c 'import headroom' followed by exec "$python" -m headroom.cli init hook ensure. Both forms put the current directory first on sys.path, and the hook runs with the opened project as its working directory, so a headroom/ directory in that project was imported instead of the installed package.

Both calls now pass -P (Python 3.11+: do not prepend the cwd/script dir to sys.path). -P was chosen over -I so PYTHONPATH and user site-packages installs keep working. Interpreters older than 3.11 reject -P, so the import check fails and they are skipped; headroom-ai requires 3.11+ so they could not have a real install anyway.

The same fallback pattern in the OpenClaw plugin (plugins/openclaw/src/proxy-manager.ts) is fixed in a separate PR because that plugin has its own test suite.

Follow-up from review: the processes Headroom itself relaunches with python -m headroom.cli (resolve_headroom_command() for the detached agent and MCP registration, build_runtime_command() for the python-runtime proxy, and the macOS malloc re-exec in headroom/cli/proxy.py) now also pass -P. Registered MCP server args therefore become -P -m headroom.cli mcp serve when no headroom executable is on PATH.

Type of Change

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

Changes Made

  • plugins/headroom-agent-hooks/bin/headroom-hook.sh: -P on the import check and on -m headroom.cli.
  • tests/test_plugin_hook_launcher.py: updated the fallback receipt; new test running a real interpreter (site-packages disabled) from a project dir that contains a headroom/ package and asserting it is never imported.

Testing

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

Test Output

# new/updated tests on origin/main (fix stashed)
$ pytest -q tests/test_plugin_hook_launcher.py
FAILED tests/test_plugin_hook_launcher.py::test_launcher_uses_importable_python_module
FAILED tests/test_plugin_hook_launcher.py::test_launcher_python_fallback_does_not_import_from_the_working_directory
E   AssertionError: assert not True   (marker written by the project's headroom/__init__.py)
========================= 2 failed, 9 passed in 2.19s ==========================

# with the fix
$ pytest -q tests/test_plugin_manifests.py tests/test_plugin_hook_launcher.py
============================== 21 passed in 2.08s ==============================

$ ruff check / ruff format --check (0.17.3) tests/test_plugin_hook_launcher.py
All checks passed!
1 file already formatted

Real Behavior Proof

  • Environment: macOS, /bin/sh, a Python 3.12 venv with headroom-ai installed, PATH=/usr/bin:/bin (no headroom executable), clean HOME.
  • Exact command / steps: in a directory containing headroom/__init__.py and headroom/cli.py that each print a line, run env -i HOME=... PATH=/usr/bin:/bin HEADROOM_PYTHON=<venv python> sh plugins/headroom-agent-hooks/bin/headroom-hook.sh.
  • Observed result:
    • main: planted __init__ module ran / planted cli module ran (the project's package executed).
    • this branch: the installed headroom init hook ensure ran (its normal output), nothing from the project dir.
  • Not tested: Windows Git Bash.

Runtime Rollout Safety

  • Rollout-managed feature(s): none
  • Minimum rollout channel: n/a
  • Stable/default behavior changed: the Python fallback only uses an installed headroom; a source checkout reachable only via the cwd is no longer picked up (set HEADROOM_BIN or PYTHONPATH for that).
  • Kill switch / disable path: HEADROOM_BIN / a headroom executable on PATH bypass the fallback entirely.
  • 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 uses -P instead of -I to keep PYTHONPATH/user-site installs working).

🤖 Generated with Claude Code

Reported by @JoaoMarcos44. Thank you for your contribution! 🩵

…t dir


When no headroom executable is found, headroom-hook.sh falls back to
`python -c 'import headroom'` and `python -m headroom.cli`, both of
which put the current directory (the opened project) first on sys.path.
A headroom/ directory in the project was imported instead of the
installed package. Both calls now pass -P (Python 3.11+), so only an
installed headroom is used; older interpreters reject -P and are skipped.

Assay-Kind: bugfix
Assay-Ask: The agent-hooks launcher's Python fallback imported a headroom package from the opened
 project's directory instead of the installed 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. risk: low Scope estimate: documentation/tests-only change; verification still required. area: integrations Touches integrations files; triage routing, not a CODEOWNERS assignment. 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.

@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

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@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 initial launcher regression is fixed: 21 focused launcher/manifest tests pass on Python 3.11, and the new isolation regression fails against the target launcher and passes here. However, the unresolved detached-startup finding remains reproducible. With no headroom executable on PATH, resolve_headroom_command returns [sys.executable, "-m", "headroom.cli"]; start_detached_agent runs it from the project directory. A benign project headroom/cli.py that writes a marker executes through this helper on this head (also on main). Expected: startup continues using the installed package without executing the project package. Please propagate safe import behavior through detached startup and the subsequent Python proxy child, and add a regression with an installed package plus a planted project package. The current no-installed-package test does not cover that startup path. This confirms the existing review thread rather than introducing a new issue.

The hook launcher now uses -P, but the processes it starts did not:
resolve_headroom_command() (detached agent, MCP registration),
build_runtime_command() (python-runtime proxy) and the macOS malloc
re-exec all ran `python -m headroom.cli` with the inherited working
directory first on sys.path. They now pass -P too; sys.executable is
always 3.11+ here since headroom-ai requires it.

Assay-Kind: bugfix
Assay-Ask: Processes Headroom relaunches via python -m headroom.cli could import a headroom package
 from the working directory.

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

@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 previously requested isolation changes are now present and pass targeted validation. With an installed fixture package and a planted project headroom package, detached startup, the Python proxy child and malloc re-execution all load the installed fixture without executing project code; the same reproduction executes project code on the target and recorded main. Also passed: 21 launcher/manifest tests and nine guard tests against extracted functions on Python 3.11. This validates the prior finding as addressed, with no new code defect found. Full installed deployment was not exercised. Qualification remains pending: the staged Init E2E run 43514136611 failed and other checks were still running; its failure log is needed to determine cause. The separate inherited workflow provenance hold remains unchanged.

@github-actions github-actions Bot added the status: ci failing Required or reported CI checks are failing label Nov 13, 2026
Assay-Kind: tests
Assay-Ask: The init e2e expected the old `python -m headroom.cli mcp serve` registration without -P.

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
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>

@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 prior detached-startup isolation finding remains addressed. On this head, 21 launcher/manifest tests and nine extracted-function guard tests pass. A real-interpreter reproduction with an installed fixture and a planted project headroom package confirms detached startup, the Python proxy child and malloc replacement load the installed fixture without executing project code; the same cases execute project code on the target and recorded main. The updated Init E2E expected-command helper also passes both command-resolution cases, and the staged Init E2E run 43518941565 succeeds. Full installed application integration and native macOS execution were not tested locally. Qualification remains pending: please provide the failure details for commitlint job 132208116600 (run 43518941742); its staged output contains no diagnostic. Test shard 3 and docker-native-e2e were still running in the evidence snapshot. The inherited shared workflow provenance hold remains separate from this PR; no new code defect was found.

@chopratejas

Copy link
Copy Markdown
Collaborator Author

Thanks Jerrett.

Source: no code defect, per your review. The detached-start, proxy-child and re-exec paths are in 4667d10, and the e2e expectation is in c756344.

commitlint: the only failing commit is the branch's chore: merge main into ... commit (c2b3cd78). Its body mentions a PR number, so the conventional-commits parser treats the trailer block as a footer, and the Assay-Ask: line there is over 100 characters (footer-max-line-length). Job log:

⧗   input: chore: merge main into fix/plugin-launcher-isolated-python
✖   footer's lines must not be longer than 100 characters [footer-max-line-length]
✖   found 1 problems, 0 warnings

Every other commit on the branch passes the repo's .commitlintrc.json when checked locally (@commitlint/cli@19 + config-conventional@19):

0ea08cfc fix(plugins): keep the hook launcher's Python fallback off the project dir => ok
4667d104 fix(install): pass -P wherever Headroom relaunches itself with python -m => ok
c7563442 test(e2e): expect -P in the python-module MCP registration => ok
c2b3cd78 chore: merge main into fix/plugin-launcher-isolated-python => footer-max-line-length

This can't be cleared without rewriting branch history. The PR's own changes don't cause it, and a squash merge replaces that commit. If you prefer a clean check, I can reword that one commit with a force-push, but only with a maintainer's OK.

Shard 3 and docker-native-e2e had not finished in your snapshot; I'll check them on the next run.

JerrettDavis added a commit that referenced this pull request Nov 14, 2026

@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 prior isolation findings remain addressed. The outstanding CI failure is now diagnosed: run 43547220242, commitlint job 132301234057 rejects commit c2b3cd7 with footer-max-line-length. Its Assay-Ask footer is 116 characters; the limit is 100. Please reword that commit by wrapping the footer, preserving its content, and obtain a passing commitlint check. Adding a later commit will not fix the older message. This confirms the failure already documented in the discussion. No new code defect was found. Earlier-head passing tests were retained as context, not counted as current-head passes; several current checks remain in progress in the staged evidence. The inherited shared workflow provenance hold is separate from this PR-specific history failure.

@JerrettDavis JerrettDavis added area: cli Touches cli files; triage routing, not a CODEOWNERS assignment. area: packaging Touches packaging files; triage routing, not a CODEOWNERS assignment. status: ci failing Required or reported CI checks are failing status: code changes requested labels Nov 14, 2026
@chopratejas

Copy link
Copy Markdown
Collaborator Author

Jerrett, the commitlint blocker is cleared. The footer-length rule fix (#4868) is merged into main, and I merged main into this branch, so commitlint now passes on this head. No code changes in this update.

@JerrettDavis
JerrettDavis force-pushed the fix/plugin-launcher-isolated-python branch from 523361a to 8b971f9 Compare November 14, 2026 05:21
@github-actions github-actions Bot removed the status: ci failing Required or reported CI checks are failing label Nov 14, 2026
@JerrettDavis

Copy link
Copy Markdown
Collaborator

Applied the requested mechanical fix: rewrote the PR-range commits to wrap the oversized Assay-Ask footer(s) to under the 100-character commitlint limit (content preserved, wrapped at word boundaries). Branch force-pushed; commitlint and CI are re-running at the new head.

@github-actions github-actions Bot added the status: ci failing Required or reported CI checks are failing label Nov 14, 2026

This branch was successfully deployed

1 active deployment
Preview — 8b971f97 Deployed Nov 14, 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. area: integrations Touches integrations files; triage routing, not a CODEOWNERS assignment. area: packaging Touches packaging 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: ci failing Required or reported CI checks are failing status: code changes requested 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