mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
6147883d5e
## Description
`headroom wrap <agent>` could sit silently for a full 300 seconds before
the agent launched, and leaked one orphaned process every time it did.
`_setup_serena_mcp` runs `serena project index` synchronously on the
launch path, with `capture_output=True`, an inherited stdin and
`timeout=300`. When a project has no `.serena/project.yml`, Serena
auto-creates one — and that auto-creation asks one `[y/N]` question per
additionally-detected language server. Three things then combine:
1. stdin was inherited, so Serena believed it could prompt.
2. stdout was captured, so the question never reached the terminal.
3. the call was synchronous, so the agent waited out the entire timeout.
The user saw no prompt, no progress and no error — only a wrapper that
appeared to hang. The pre-index could never succeed in that state, so
the 300 seconds bought nothing.
On top of that, `subprocess.run` kills only its direct child on timeout.
`uvx` is a launcher that execs the real `serena` executable as a
grandchild, which was never signalled: it reparented to PID 1 and
survived indefinitely. Same class of bug as #615 and #880.
Closes #2938
## Type of Change
- [x] Bug fix (non-breaking change that fixes an issue)
## Changes Made
- `_serena_project_skip_reason` (`headroom/cli/wrap.py`) now returns a
skip reason when `.serena/project.yml` is absent, so the pre-index does
not run in the one state where it cannot succeed.
- `_index_serena_project` passes `stdin=subprocess.DEVNULL`, so a
subprocess that decides to prompt gets EOF and exits in about a second
instead of blocking behind a captured pipe. This is deliberately kept as
a second line of defence even though the skip above already avoids the
known prompt.
- `_index_serena_project` now spawns via `subprocess.Popen` in its own
process group (`start_new_session=True` on POSIX,
`CREATE_NEW_PROCESS_GROUP` on Windows) instead of `run(...)`, so the
whole tree can be signalled.
- New `_kill_serena_index_tree` helper kills that tree on timeout —
`killpg(..., SIGKILL)` on POSIX, `taskkill /F /T /PID` on Windows — then
reaps the child and closes the capture pipes. Best-effort throughout; it
never raises.
- Corrected two comments that asserted the opposite of the observed
behaviour ("a failure or timeout here never blocks the wrap", "neither
blocks the wrap"). Both were accurate about intent and wrong about
effect.
- Added `_SERENA_INDEX_TIMEOUT` (still 300) and a line announcing the
pre-index, so a legitimately long index no longer looks like a hang.
- Tests in `tests/test_cli/test_wrap_serena_boost.py` rewritten for the
`Popen` path and extended to cover the DEVNULL stdin, the process-group
flag, the timeout tree-kill, the new skip reason, and the
`_setup_serena_mcp` wiring on both a fresh project and one that already
has `project.yml`.
### Behaviour change worth a reviewer's attention
**On a project with no `.serena/project.yml`, the pre-index no longer
runs at all.** That is the first wrap of any project, so this is the
common case.
I went this way rather than fixing the prompt because there is no way to
fix it from Headroom's side without re-introducing something the project
deliberately removed. Serena's `project index` command has no
non-interactive switch: `ProjectCommands._create_project` calls
`ProjectConfig.autogenerate(..., interactive=True)` with `interactive`
hardcoded. The only path that skips the prompt is passing
`--ls/--language` explicitly, which means Headroom guessing the
project's languages again — exactly the hand-maintained
extension-to-language map that was removed in #2674, with a comment in
this same function explaining why Serena should own that job.
The cost of skipping is small and self-correcting. Serena's MCP server
(`serena start-mcp-server --project-from-cwd`) generates `project.yml`
itself, non-interactively, on first start, and indexes lazily on demand
— which is the fallback the existing docstring already relied on. So the
first wrap now launches immediately with lazy indexing, and every wrap
after that pre-indexes for real. Previously the first wrap cost 300
seconds *and* still produced no index, so nothing of value is lost.
Happy to switch to passing `--ls` instead if maintainers would rather
keep the pre-index on the first wrap and accept a language map; the
other two changes stand either way.
## Testing
- [x] Unit tests pass (`pytest`)
- [x] Linting passes (`ruff check .`)
- [x] Type checking passes (`mypy headroom`)
- [x] New tests added for new functionality
- [x] Manual testing performed
### Test Output
```text
$ python -m pytest tests/test_cli/test_wrap_serena_boost.py tests/test_cli/test_serena_migrate.py -q
collected 29 items
tests\test_cli\test_wrap_serena_boost.py .............s........... [ 86%]
tests\test_cli\test_serena_migrate.py .... [100%]
======================== 28 passed, 1 skipped in 0.54s ========================
$ python -m ruff check headroom/cli/wrap.py tests/test_cli/test_wrap_serena_boost.py
All checks passed!
$ python -m ruff format --check headroom/cli/wrap.py tests/test_cli/test_wrap_serena_boost.py
2 files already formatted
$ python -m mypy headroom/cli/wrap.py --ignore-missing-imports --python-version 3.13 --follow-imports=silent
Success: no issues found in 1 source file
```
The single skip is `test_kill_tree_signals_the_group_on_posix`, which is
platform-gated; the Windows counterpart ran. I develop on Windows, so
the POSIX `killpg` branch is covered by unit test only — the end-to-end
tree-kill proof below is the Windows `taskkill` branch.
## Real Behavior Proof
- Environment: Windows 11 Pro 26200, Python 3.13.11, headroom checkout
at
|
||
|---|---|---|
| .. | ||
| conftest.py | ||
| test_copilot_auth.py | ||
| test_init_cli.py | ||
| test_init_enable_tool_search.py | ||
| test_install_cli.py | ||
| test_main_help_version.py | ||
| test_mcp.py | ||
| test_mcp_status.py | ||
| test_recover_codex.py | ||
| test_serena_disable.py | ||
| test_serena_migrate.py | ||
| test_subprocess_utf8_encoding.py | ||
| test_unwrap_claude.py | ||
| test_wrap_aider.py | ||
| test_wrap_bridge.py | ||
| test_wrap_claude.py | ||
| test_wrap_claude_base_url.py | ||
| test_wrap_claude_finally_unbound.py | ||
| test_wrap_claude_vertex_proxy_env.py | ||
| test_wrap_codex.py | ||
| test_wrap_copilot.py | ||
| test_wrap_dead_marker_selfheal.py | ||
| test_wrap_encoding.py | ||
| test_wrap_goose.py | ||
| test_wrap_grok.py | ||
| test_wrap_helpers.py | ||
| test_wrap_kimi.py | ||
| test_wrap_omp.py | ||
| test_wrap_openclaude.py | ||
| test_wrap_openclaw.py | ||
| test_wrap_opencode.py | ||
| test_wrap_openhands.py | ||
| test_wrap_persistent.py | ||
| test_wrap_proxy_detach.py | ||
| test_wrap_serena_boost.py | ||
| test_wrap_stale_marker.py | ||
| test_wrap_vibe.py | ||
| test_wrap_vscode.py | ||
| test_wrap_vscode_claude.py | ||
| test_wrap_zcode.py | ||