mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
fix(wrap): stop the Serena pre-index stalling the launch path for 300s (#2945)
## 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 941c25d3 plus this branch, uvx resolving `serena-agent` from PyPI,
throwaway project with bash + TypeScript + Python sources and no
`.serena/`
- Exact command / steps: spawned the real `uvx --from serena-agent
serena project index` against that project twice — once with stdin
readable and never closed (a parent-held pipe, the faithful stand-in for
the idle terminal the old code inherited; `communicate()` cannot be used
here because with `input=None` it closes the child's stdin immediately
and hands it the EOF the real bug never delivers), once with
`stdin=subprocess.DEVNULL`. Then spawned it twice more, stopping it with
a plain `proc.kill()` (the old timeout behaviour) versus the new
`_kill_serena_index_tree`, counting survivors with `Get-CimInstance
Win32_Process` filtered on the command line.
- Observed result: stall reproduced and fixed — `A/open-stdin: STILL
BLOCKED after 30.0s (timeout hit)` versus `B/DEVNULL: exit=1
elapsed=1.1s` with stderr tail `Project configuration auto-generation
failed after 0.000 seconds / Error: EOF when reading a line`. Leak
reproduced and fixed — after spawning a 5-process tree, `after
proc.kill(): still alive: [34468, 35044, 35808]` versus `after
_kill_serena_index_tree: still alive: []`. The 30-second bound in the
first experiment stands in for the shipped 300; the point is that the
child never returns on its own.
- Not tested: the POSIX `killpg`/`start_new_session` branch end-to-end
(no Linux or macOS host available, so it is unit-tested only — and it is
the branch the issue reporter observed failing); a full `headroom wrap
opencode` launch end-to-end, since this machine has no working local
proxy (`tests/test_cli/test_wrap_opencode.py` fails identically with and
without this branch for that reason); and the second-wrap pre-index
success path against a live Serena run, which is covered by unit test
instead.
## Review Readiness
- [x] I have performed a self-review
- [x] This PR is ready for human review
## Checklist
- [x] My code follows the project's style guidelines
- [x] I have performed a self-review of my code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] New and existing unit tests pass locally with my changes
- [x] I did **not** edit `CHANGELOG.md` — it is generated by
release-please from my Conventional Commit PR title (a CI guard enforces
this)
## Additional Notes
- The documentation checkbox is unchecked because no user-facing doc
describes the pre-index; the behaviour is explained in source
docstrings, which this PR rewrites.
- `_kill_serena_index_tree` is deliberately total: every step is wrapped
so cleaning up an already-dead child cannot turn a timeout into a crash
on the launch path. There is a test for that.
- #2754 (use a pre-installed Serena) would remove the `uvx` layer this
leak depends on, but not the prompt itself — the stdin guard here is
still needed after that lands.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: JD Davis <mxjerrett@gmail.com>
This commit is contained in:
parent
41dab2d099
commit
6147883d5e
2 changed files with 377 additions and 38 deletions
|
|
@ -1715,6 +1715,18 @@ def _serena_project_skip_reason(root: Path) -> str | None:
|
|||
Serena's own ``~/.serena`` config directory. A linked git worktree (its
|
||||
top-level ``.git`` is a file, not a directory) is an ephemeral checkout that
|
||||
would pay for its own index at a path that soon disappears.
|
||||
|
||||
A project with no ``.serena/project.yml`` is skipped because the pre-index
|
||||
cannot succeed there (#2938). ``serena project index`` auto-creates the file
|
||||
when it is missing, and that auto-creation calls
|
||||
``ProjectConfig.autogenerate(interactive=True)``, which asks one ``[y/N]``
|
||||
question per additionally-detected language server. The CLI has no
|
||||
non-interactive switch; the only way to reach the silent branch is to pass
|
||||
``--ls/--language`` explicitly, which means Headroom guessing the project's
|
||||
languages again — exactly the hand-maintained map removed below. Serena's
|
||||
MCP server generates that file itself (non-interactively) on first start and
|
||||
indexes lazily on demand, so the pre-index simply resumes from the next
|
||||
wrap onwards.
|
||||
"""
|
||||
try:
|
||||
resolved = root.resolve()
|
||||
|
|
@ -1725,24 +1737,108 @@ def _serena_project_skip_reason(root: Path) -> str | None:
|
|||
return "$HOME is not a project"
|
||||
if (resolved / ".git").is_file():
|
||||
return "linked git worktree"
|
||||
if not (resolved / ".serena" / "project.yml").is_file():
|
||||
return "no .serena/project.yml yet — Serena will create it and index on demand"
|
||||
return None
|
||||
|
||||
|
||||
#: Upper bound on the synchronous pre-index. The agent does not launch until
|
||||
#: this call returns, so the number is a stall budget, not just a safety net.
|
||||
_SERENA_INDEX_TIMEOUT = 300
|
||||
|
||||
|
||||
def _kill_serena_index_tree(proc: subprocess.Popen) -> None:
|
||||
"""Kill *proc* and everything it spawned (best-effort, never raises).
|
||||
|
||||
``uvx`` is a launcher: it resolves the environment and then runs the real
|
||||
``serena`` executable as a grandchild. Killing only the direct child leaves
|
||||
that grandchild alive and reparented to PID 1, so every timed-out pre-index
|
||||
leaked one process that never exits (#2938 — the same failure mode as #615
|
||||
and #880). The child is started in its own process group precisely so the
|
||||
whole tree can be signalled here.
|
||||
"""
|
||||
if sys.platform == "win32":
|
||||
# Windows has no process groups to signal for an already-wedged child;
|
||||
# ``taskkill /T`` walks the tree by parent PID instead. ``/F`` because a
|
||||
# process blocked in a read will not act on a graceful close request.
|
||||
try:
|
||||
subprocess.run(
|
||||
["taskkill", "/F", "/T", "/PID", str(proc.pid)],
|
||||
capture_output=True,
|
||||
timeout=10,
|
||||
check=False,
|
||||
)
|
||||
except Exception:
|
||||
pass
|
||||
else:
|
||||
try:
|
||||
os.killpg(os.getpgid(proc.pid), signal.SIGKILL)
|
||||
except Exception:
|
||||
pass
|
||||
# Backstop: if the tree kill above did not land, at least the direct child
|
||||
# goes. Then reap so the parent does not leave a zombie behind, and close
|
||||
# the capture pipes we opened so the wrap does not carry stray fds into the
|
||||
# agent it is about to exec.
|
||||
try:
|
||||
proc.kill()
|
||||
except Exception:
|
||||
pass
|
||||
try:
|
||||
proc.wait(timeout=10)
|
||||
except Exception:
|
||||
pass
|
||||
for stream in (proc.stdout, proc.stderr, proc.stdin):
|
||||
try:
|
||||
if stream is not None:
|
||||
stream.close()
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
|
||||
def _index_serena_project(*, verbose: bool = False) -> None:
|
||||
"""Warm Serena's symbol cache for the current project (non-fatal).
|
||||
|
||||
Runs ``serena project index`` (the same ``uvx --from git+…`` launch used to
|
||||
start the MCP server) in the project directory so the first symbol query is
|
||||
not paying for a cold index. Timeout-guarded and best-effort: Serena also
|
||||
indexes lazily on demand, so a failure or timeout here never blocks the
|
||||
wrap.
|
||||
Runs ``serena project index`` (the same ``uvx --from serena-agent`` launch
|
||||
used to start the MCP server) in the project directory so the first symbol
|
||||
query is not paying for a cold index. Serena also indexes lazily on demand,
|
||||
so any failure here is survivable.
|
||||
|
||||
This runs on the launch path, synchronously: the agent starts only once it
|
||||
returns, so the timeout below is time the user spends staring at nothing.
|
||||
Two guards keep that bounded (#2938):
|
||||
|
||||
* ``stdin`` is ``DEVNULL``. Serena prompts when it has to auto-create
|
||||
``project.yml``, and because stdout is captured the question never
|
||||
reaches the terminal — an inherited stdin turned that into a silent,
|
||||
full-timeout hang. EOF makes it fail in about a second instead.
|
||||
``_serena_project_skip_reason`` already keeps us out of that state; this
|
||||
is the belt-and-braces half, and it covers any future Serena prompt too.
|
||||
* The child gets its own process group so ``_kill_serena_index_tree`` can
|
||||
take out the ``uvx`` grandchild on timeout rather than orphaning it.
|
||||
"""
|
||||
if shutil.which("uvx") is None:
|
||||
if verbose:
|
||||
click.echo(" Serena: uvx not found — skipping pre-index")
|
||||
return
|
||||
|
||||
popen_kwargs: dict[str, Any] = {
|
||||
"stdout": subprocess.PIPE,
|
||||
"stderr": subprocess.PIPE,
|
||||
"stdin": subprocess.DEVNULL,
|
||||
"text": True,
|
||||
# ``subprocess.Popen`` directly, so the encoding defaults that
|
||||
# ``headroom._subprocess.run`` applies have to be repeated here.
|
||||
"encoding": "utf-8",
|
||||
"errors": "replace",
|
||||
"cwd": str(Path.cwd()),
|
||||
}
|
||||
if sys.platform == "win32":
|
||||
popen_kwargs["creationflags"] = subprocess.CREATE_NEW_PROCESS_GROUP
|
||||
else:
|
||||
popen_kwargs["start_new_session"] = True
|
||||
|
||||
try:
|
||||
result = run(
|
||||
proc = subprocess.Popen(
|
||||
[
|
||||
"uvx",
|
||||
# PyPI (prebuilt wheels), not the git source that fails to build
|
||||
|
|
@ -1753,20 +1849,32 @@ def _index_serena_project(*, verbose: bool = False) -> None:
|
|||
"project",
|
||||
"index",
|
||||
],
|
||||
capture_output=True,
|
||||
text=True,
|
||||
timeout=300,
|
||||
cwd=str(Path.cwd()),
|
||||
**popen_kwargs,
|
||||
)
|
||||
if result.returncode == 0:
|
||||
click.echo(" Serena: project pre-indexed (symbol cache warmed)")
|
||||
elif verbose:
|
||||
click.echo(f" Serena: pre-index failed ({(result.stderr or '')[:100]})")
|
||||
except subprocess.TimeoutExpired:
|
||||
click.echo(" Serena: pre-index timed out (will index on demand)")
|
||||
except Exception as e:
|
||||
if verbose:
|
||||
click.echo(f" Serena: pre-index skipped ({e})")
|
||||
return
|
||||
|
||||
# Announce the wait. Indexing a large repo legitimately takes minutes and
|
||||
# the output is captured, so without this line the wrap looks hung.
|
||||
click.echo(" Serena: pre-indexing project (first run can take a while)…")
|
||||
try:
|
||||
_stdout, stderr = proc.communicate(timeout=_SERENA_INDEX_TIMEOUT)
|
||||
except subprocess.TimeoutExpired:
|
||||
_kill_serena_index_tree(proc)
|
||||
click.echo(" Serena: pre-index timed out (will index on demand)")
|
||||
return
|
||||
except Exception as e:
|
||||
_kill_serena_index_tree(proc)
|
||||
if verbose:
|
||||
click.echo(f" Serena: pre-index skipped ({e})")
|
||||
return
|
||||
|
||||
if proc.returncode == 0:
|
||||
click.echo(" Serena: project pre-indexed (symbol cache warmed)")
|
||||
elif verbose:
|
||||
click.echo(f" Serena: pre-index failed ({(stderr or '')[:100]})")
|
||||
|
||||
|
||||
def _setup_serena_mcp(
|
||||
|
|
@ -1833,7 +1941,9 @@ def _setup_serena_mcp(
|
|||
|
||||
# Serena is the active engine here (we passed the detect/uvx guards): steer
|
||||
# the agent toward symbol-level tools, then warm the symbol cache. Both are
|
||||
# best-effort and non-fatal — neither blocks the wrap.
|
||||
# best-effort and non-fatal, but the pre-index is *synchronous* — the agent
|
||||
# does not launch until it returns or hits ``_SERENA_INDEX_TIMEOUT``. See
|
||||
# ``_index_serena_project`` for how that wait is kept bounded and visible.
|
||||
#
|
||||
# Headroom no longer writes ``.serena/project.yml`` language scoping. Serena
|
||||
# determines the project's languages itself during
|
||||
|
|
|
|||
|
|
@ -107,18 +107,54 @@ def _stub_uvx(monkeypatch: pytest.MonkeyPatch, present: bool = True) -> None:
|
|||
)
|
||||
|
||||
|
||||
class _FakeProc:
|
||||
"""Minimal stand-in for the ``serena project index`` child process."""
|
||||
|
||||
def __init__(
|
||||
self,
|
||||
*,
|
||||
returncode: int = 0,
|
||||
stderr: str = "",
|
||||
communicate_error: BaseException | None = None,
|
||||
) -> None:
|
||||
self.pid = 4242
|
||||
self.returncode = returncode
|
||||
self.stdin = None
|
||||
self.stdout = None
|
||||
self.stderr = None
|
||||
self._stderr = stderr
|
||||
self._communicate_error = communicate_error
|
||||
self.killed = False
|
||||
self.waited = False
|
||||
|
||||
def communicate(self, timeout: float | None = None) -> tuple[str, str]:
|
||||
if self._communicate_error is not None:
|
||||
raise self._communicate_error
|
||||
return "", self._stderr
|
||||
|
||||
def kill(self) -> None:
|
||||
self.killed = True
|
||||
|
||||
def wait(self, timeout: float | None = None) -> int:
|
||||
self.waited = True
|
||||
return self.returncode
|
||||
|
||||
|
||||
def _stub_popen(monkeypatch: pytest.MonkeyPatch, proc: _FakeProc) -> Mock:
|
||||
mock_popen = Mock(return_value=proc)
|
||||
monkeypatch.setattr(wrap_cli.subprocess, "Popen", mock_popen)
|
||||
return mock_popen
|
||||
|
||||
|
||||
def test_preindex_runs_serena_in_cwd(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
monkeypatch.chdir(tmp_path)
|
||||
_stub_uvx(monkeypatch)
|
||||
mock_run = Mock(
|
||||
return_value=subprocess.CompletedProcess(args=[], returncode=0, stdout="", stderr="")
|
||||
)
|
||||
monkeypatch.setattr(wrap_cli, "run", mock_run)
|
||||
mock_popen = _stub_popen(monkeypatch, _FakeProc())
|
||||
|
||||
wrap_cli._index_serena_project()
|
||||
|
||||
mock_run.assert_called_once()
|
||||
args, kwargs = mock_run.call_args
|
||||
mock_popen.assert_called_once()
|
||||
args, kwargs = mock_popen.call_args
|
||||
cmd = args[0]
|
||||
assert cmd[0] == "uvx"
|
||||
assert cmd[-3:] == ["serena", "project", "index"]
|
||||
|
|
@ -126,63 +162,256 @@ def test_preindex_runs_serena_in_cwd(tmp_path: Path, monkeypatch: pytest.MonkeyP
|
|||
assert "serena-agent" in cmd
|
||||
assert "git+https://github.com/oraios/serena" not in cmd
|
||||
assert kwargs["cwd"] == str(tmp_path) # invoked in the project cwd
|
||||
assert "timeout" in kwargs # timeout-guarded
|
||||
|
||||
|
||||
def test_preindex_is_timeout_guarded(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
monkeypatch.chdir(tmp_path)
|
||||
_stub_uvx(monkeypatch)
|
||||
proc = _FakeProc()
|
||||
_stub_popen(monkeypatch, proc)
|
||||
seen: list[float | None] = []
|
||||
monkeypatch.setattr(
|
||||
proc, "communicate", lambda timeout=None: (seen.append(timeout), ("", ""))[1]
|
||||
)
|
||||
|
||||
wrap_cli._index_serena_project()
|
||||
|
||||
assert seen == [wrap_cli._SERENA_INDEX_TIMEOUT]
|
||||
|
||||
|
||||
def test_preindex_never_inherits_stdin(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""Serena prompts ``[y/N]`` behind a captured stdout; stdin must be EOF (#2938)."""
|
||||
monkeypatch.chdir(tmp_path)
|
||||
_stub_uvx(monkeypatch)
|
||||
mock_popen = _stub_popen(monkeypatch, _FakeProc())
|
||||
|
||||
wrap_cli._index_serena_project()
|
||||
|
||||
assert mock_popen.call_args.kwargs["stdin"] == subprocess.DEVNULL
|
||||
|
||||
|
||||
def test_preindex_child_gets_its_own_process_group(
|
||||
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
"""Without this the ``uvx`` grandchild survives the timeout kill (#2938)."""
|
||||
monkeypatch.chdir(tmp_path)
|
||||
_stub_uvx(monkeypatch)
|
||||
mock_popen = _stub_popen(monkeypatch, _FakeProc())
|
||||
|
||||
wrap_cli._index_serena_project()
|
||||
|
||||
kwargs = mock_popen.call_args.kwargs
|
||||
if wrap_cli.sys.platform == "win32":
|
||||
assert kwargs["creationflags"] & subprocess.CREATE_NEW_PROCESS_GROUP
|
||||
else:
|
||||
assert kwargs["start_new_session"] is True
|
||||
|
||||
|
||||
def test_preindex_skips_without_uvx(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
_stub_uvx(monkeypatch, present=False)
|
||||
mock_run = Mock(side_effect=AssertionError("run must not be called without uvx"))
|
||||
monkeypatch.setattr(wrap_cli, "run", mock_run)
|
||||
mock_popen = Mock(side_effect=AssertionError("Popen must not be called without uvx"))
|
||||
monkeypatch.setattr(wrap_cli.subprocess, "Popen", mock_popen)
|
||||
|
||||
wrap_cli._index_serena_project() # no exception
|
||||
|
||||
mock_run.assert_not_called()
|
||||
mock_popen.assert_not_called()
|
||||
|
||||
|
||||
def test_preindex_timeout_is_non_fatal(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
def test_preindex_timeout_kills_the_tree_and_is_non_fatal(
|
||||
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
monkeypatch.chdir(tmp_path)
|
||||
_stub_uvx(monkeypatch)
|
||||
proc = _FakeProc(communicate_error=subprocess.TimeoutExpired(cmd="serena", timeout=1))
|
||||
_stub_popen(monkeypatch, proc)
|
||||
killed: list[object] = []
|
||||
monkeypatch.setattr(wrap_cli, "_kill_serena_index_tree", killed.append)
|
||||
|
||||
wrap_cli._index_serena_project(verbose=True) # must not propagate
|
||||
|
||||
assert killed == [proc]
|
||||
|
||||
|
||||
def test_preindex_generic_error_kills_the_tree_and_is_non_fatal(
|
||||
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
monkeypatch.chdir(tmp_path)
|
||||
_stub_uvx(monkeypatch)
|
||||
proc = _FakeProc(communicate_error=RuntimeError("boom"))
|
||||
_stub_popen(monkeypatch, proc)
|
||||
killed: list[object] = []
|
||||
monkeypatch.setattr(wrap_cli, "_kill_serena_index_tree", killed.append)
|
||||
|
||||
wrap_cli._index_serena_project(verbose=True) # must not propagate
|
||||
|
||||
assert killed == [proc]
|
||||
|
||||
|
||||
def test_preindex_spawn_failure_is_non_fatal(
|
||||
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
monkeypatch.chdir(tmp_path)
|
||||
_stub_uvx(monkeypatch)
|
||||
monkeypatch.setattr(wrap_cli.subprocess, "Popen", Mock(side_effect=OSError("no exec")))
|
||||
|
||||
wrap_cli._index_serena_project(verbose=True) # must not propagate
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _kill_serena_index_tree — no orphaned `serena project index` on timeout
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_kill_tree_signals_the_group_on_posix(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
if wrap_cli.sys.platform == "win32":
|
||||
pytest.skip("POSIX process groups")
|
||||
proc = _FakeProc()
|
||||
signalled: list[tuple[int, int]] = []
|
||||
monkeypatch.setattr(wrap_cli.os, "getpgid", lambda pid: pid)
|
||||
monkeypatch.setattr(wrap_cli.os, "killpg", lambda pgid, sig: signalled.append((pgid, sig)))
|
||||
|
||||
wrap_cli._kill_serena_index_tree(proc) # type: ignore[arg-type]
|
||||
|
||||
assert signalled == [(proc.pid, wrap_cli.signal.SIGKILL)]
|
||||
assert proc.killed and proc.waited # backstop still runs
|
||||
|
||||
|
||||
def test_kill_tree_walks_the_tree_on_windows(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
if wrap_cli.sys.platform != "win32":
|
||||
pytest.skip("Windows taskkill")
|
||||
proc = _FakeProc()
|
||||
calls: list[list[str]] = []
|
||||
monkeypatch.setattr(
|
||||
wrap_cli,
|
||||
wrap_cli.subprocess,
|
||||
"run",
|
||||
Mock(side_effect=subprocess.TimeoutExpired(cmd="serena", timeout=1)),
|
||||
lambda cmd, **kw: calls.append(cmd),
|
||||
)
|
||||
# Must not propagate.
|
||||
wrap_cli._index_serena_project(verbose=True)
|
||||
|
||||
wrap_cli._kill_serena_index_tree(proc) # type: ignore[arg-type]
|
||||
|
||||
assert calls == [["taskkill", "/F", "/T", "/PID", str(proc.pid)]]
|
||||
assert proc.killed and proc.waited
|
||||
|
||||
|
||||
def test_preindex_generic_error_is_non_fatal(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
_stub_uvx(monkeypatch)
|
||||
monkeypatch.setattr(wrap_cli, "run", Mock(side_effect=RuntimeError("boom")))
|
||||
# Must not propagate.
|
||||
wrap_cli._index_serena_project(verbose=True)
|
||||
def test_kill_tree_survives_a_dead_child(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""Cleanup is best-effort: a child that already exited must not raise."""
|
||||
proc = _FakeProc()
|
||||
monkeypatch.setattr(proc, "kill", Mock(side_effect=ProcessLookupError()))
|
||||
if wrap_cli.sys.platform == "win32":
|
||||
monkeypatch.setattr(wrap_cli.subprocess, "run", Mock(side_effect=OSError("gone")))
|
||||
else:
|
||||
monkeypatch.setattr(wrap_cli.os, "getpgid", Mock(side_effect=ProcessLookupError()))
|
||||
|
||||
wrap_cli._kill_serena_index_tree(proc) # type: ignore[arg-type]
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _serena_project_skip_reason — keep per-project setup off non-project roots
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
_NO_PROJECT_YML = "no .serena/project.yml yet — Serena will create it and index on demand"
|
||||
|
||||
|
||||
def _with_project_yml(root: Path) -> Path:
|
||||
"""Give *root* the ``.serena/project.yml`` Serena writes on first MCP start."""
|
||||
(root / ".serena").mkdir(parents=True, exist_ok=True)
|
||||
(root / ".serena" / "project.yml").write_text("project_name: demo\n")
|
||||
return root
|
||||
|
||||
|
||||
def test_skip_reason_none_for_ordinary_project(tmp_path: Path) -> None:
|
||||
_with_project_yml(tmp_path)
|
||||
|
||||
assert wrap_cli._serena_project_skip_reason(tmp_path) is None
|
||||
|
||||
|
||||
def test_skip_reason_none_for_normal_checkout(tmp_path: Path) -> None:
|
||||
(tmp_path / ".git").mkdir() # real checkout: .git is a directory
|
||||
_with_project_yml(tmp_path)
|
||||
|
||||
assert wrap_cli._serena_project_skip_reason(tmp_path) is None
|
||||
|
||||
|
||||
def test_skip_reason_flags_home(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
monkeypatch.setattr(Path, "home", classmethod(lambda cls: tmp_path))
|
||||
_with_project_yml(tmp_path)
|
||||
|
||||
assert wrap_cli._serena_project_skip_reason(tmp_path) == "$HOME is not a project"
|
||||
|
||||
|
||||
def test_skip_reason_flags_linked_worktree(tmp_path: Path) -> None:
|
||||
(tmp_path / ".git").write_text("gitdir: /repo/.git/worktrees/wt\n")
|
||||
_with_project_yml(tmp_path)
|
||||
|
||||
assert wrap_cli._serena_project_skip_reason(tmp_path) == "linked git worktree"
|
||||
|
||||
|
||||
def test_skip_reason_flags_missing_project_yml(tmp_path: Path) -> None:
|
||||
"""The pre-index cannot succeed here — Serena would stop on a hidden prompt (#2938)."""
|
||||
assert wrap_cli._serena_project_skip_reason(tmp_path) == _NO_PROJECT_YML
|
||||
|
||||
|
||||
def test_skip_reason_flags_serena_dir_without_project_yml(tmp_path: Path) -> None:
|
||||
(tmp_path / ".serena").mkdir() # cache dir exists, config does not
|
||||
|
||||
assert wrap_cli._serena_project_skip_reason(tmp_path) == _NO_PROJECT_YML
|
||||
|
||||
|
||||
def test_skip_reason_survives_unresolvable_root(tmp_path: Path) -> None:
|
||||
assert wrap_cli._serena_project_skip_reason(tmp_path / "gone") is None
|
||||
# Missing directory: resolves fine (non-strict), no config, no exception.
|
||||
assert wrap_cli._serena_project_skip_reason(tmp_path / "gone") == _NO_PROJECT_YML
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# _setup_serena_mcp wiring — the launch path must not wait on a doomed index
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class _FakeRegistrar:
|
||||
"""Just enough registrar for the post-registration branch of the setup."""
|
||||
|
||||
name = "claude"
|
||||
display_name = "Claude"
|
||||
|
||||
def detect(self) -> bool:
|
||||
return True
|
||||
|
||||
def get_server(self, server_name: str) -> None:
|
||||
return None
|
||||
|
||||
def register_server(self, spec: object, *, force: bool = False) -> object:
|
||||
from headroom.mcp_registry.base import RegisterResult, RegisterStatus
|
||||
|
||||
return RegisterResult(RegisterStatus.REGISTERED, "registered")
|
||||
|
||||
|
||||
def _drive_setup(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> list[str]:
|
||||
"""Run ``_setup_serena_mcp`` in *tmp_path*, returning pre-index call markers."""
|
||||
monkeypatch.setenv("HEADROOM_WORKSPACE_DIR", str(tmp_path / ".headroom"))
|
||||
monkeypatch.chdir(tmp_path)
|
||||
_stub_uvx(monkeypatch)
|
||||
monkeypatch.setattr(wrap_cli, "_inject_serena_instructions", lambda *a, **k: True)
|
||||
calls: list[str] = []
|
||||
monkeypatch.setattr(wrap_cli, "_index_serena_project", lambda **k: calls.append("indexed"))
|
||||
|
||||
wrap_cli._setup_serena_mcp(_FakeRegistrar(), context="claude-code", verbose=True)
|
||||
return calls
|
||||
|
||||
|
||||
def test_setup_does_not_preindex_a_project_without_serena_config(
|
||||
tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str]
|
||||
) -> None:
|
||||
"""First wrap of a fresh project: launch immediately, do not wait out the timeout."""
|
||||
calls = _drive_setup(tmp_path, monkeypatch)
|
||||
|
||||
assert calls == []
|
||||
assert "skipping pre-index" in capsys.readouterr().out
|
||||
|
||||
|
||||
def test_setup_preindexes_once_serena_config_exists(
|
||||
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
|
||||
) -> None:
|
||||
"""Second wrap: Serena's MCP server has written project.yml, so the index can run."""
|
||||
_with_project_yml(tmp_path)
|
||||
|
||||
assert _drive_setup(tmp_path, monkeypatch) == ["indexed"]
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue