FastMCP alignment: new tools, prompts, and middleware#15
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #15 +/- ##
==========================================
+ Coverage 82.92% 86.90% +3.97%
==========================================
Files 17 38 +21
Lines 984 1710 +726
Branches 110 201 +91
==========================================
+ Hits 816 1486 +670
- Misses 122 165 +43
- Partials 46 59 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
76bec04 to
f9d5388
Compare
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
…thread around capture_pane why: In commit 67c3fc8 the wait tools became `async def` so that ctx.report_progress could be awaited during long polls. FastMCP direct-awaits async tools on the main event loop (`fastmcp/tools/function_tool.py:277-278`) — it only uses a threadpool for *sync* tools via `call_sync_fn_in_threadpool`. That means every blocking `pane.capture_pane()` subprocess.run inside the poll loop starves every other concurrent MCP request for the duration of the tmux roundtrip (~10s of ms per tick), for the full configured timeout. A 60-second `wait_for_text` call would pin the entire server for 60 seconds, delaying list_sessions / capture_pane / anything else. Loom code review surfaced this with 100% confidence (Gemini Skeptic pass on PR #15); verified against FastMCP source. what: - Wrap the three blocking `pane.capture_pane(...)` calls in src/libtmux_mcp/tools/pane_tools/wait.py with `asyncio.to_thread`: * `wait_for_text` polling call. * `wait_for_content_change` initial snapshot. * `wait_for_content_change` polling call. - Inline comment at the first site explains why (future maintainers will otherwise see "sync call inside async fn" as a bug and try to "fix" it by removing `async def`, reintroducing the blocking path). - No docstring changes — the `async def` signature already promises non-blocking semantics; this commit actually delivers on it. - Add `test_wait_tools_do_not_block_event_loop` in tests/test_pane_tools.py that runs `wait_for_text` against a never-matching pattern inside an asyncio.gather with a ticker coroutine. Asserts the ticker counter >= 5 over the 300 ms wait window — a blocking capture would leave it at 0 or 1. Deliberately a generous lower bound to stay robust on slow CI.
…_wait
why: The ``run_and_wait`` prompt hardcoded ``mcp_done`` as the tmux
wait-for channel name. tmux channels are server-global, so two
concurrent agents (or parallel prompt renderings from one agent)
would race: one agent's ``tmux wait-for -S mcp_done`` would
unblock another's pending ``wait_for_channel("mcp_done")``,
producing a false positive completion signal.
Flagged as Critical by GPT Builder in the Loom review of PR #15.
what:
- Import ``uuid`` in src/libtmux_mcp/prompts/recipes.py.
- Generate a fresh channel name at each call: ``libtmux_mcp_wait_<uuid4hex[:8]>``.
* Prefix aligns with the buffer-tools namespace (libtmux_mcp_) so a
future ``list_buffers(prefix="libtmux_mcp_")``-style operator sees
every MCP-owned tmux artifact uniformly.
* 8 hex characters (~32 bits) is safe against collision inside a
single tmux server's concurrent-agent population.
- Interpolate the same channel name into both the send_keys payload
and the wait_for_channel call so one prompt rendering remains
internally consistent.
- Update the prompt docstring to note the per-invocation scope.
- tests/test_prompts.py:
* Update ``test_run_and_wait_returns_string_template`` to pin the
new prefix.
* Add ``test_run_and_wait_channel_is_uuid_scoped`` asserting
(a) channel matches ``libtmux_mcp_wait_[0-9a-f]{8}``,
(b) send_keys and wait_for_channel use the same name within one
rendering,
(c) two separate renderings emit different channel names.
why: ``show_hook`` swallowed three tmux error substrings into an empty
result: ``"too many arguments"`` (correctly — that's how tmux
reports an unset hook), but also ``"unknown hook"`` and
``"invalid option"``. The latter two fire for *typos* and
*wrong-scope* mistakes, not "hook is unset". Agents sending
``show_hook("pane-esited")`` (typo) or
``show_hook("pane-exited", scope="server")`` (wrong scope) got
back an empty list indistinguishable from a correctly-named but
unset hook — masking the real input error.
Flagged as Important by both Gemini (Skeptic) and GPT (Builder)
in the Loom review of PR #15. This commit also lands the upstream
TODO comment for Suggestion #15 (libtmux's scope-kwarg argv bug).
what:
- Narrow the ``except libtmux_exc.OptionError`` branch in
src/libtmux_mcp/tools/hook_tools.py::show_hook to only match
``"too many arguments"`` — the single substring all tmux builds
supported by this project use for an unset hook. Any other
OptionError (``"unknown hook"``, ``"invalid option"``, new future
substrings) re-raises so ``handle_tool_errors`` surfaces it.
- Replace the block's comment with the reasoning so a future
maintainer knows not to re-broaden the catch.
- Add a TODO(libtmux upstream) comment in ``_resolve_hook_target``
explaining why the scope-nulling workaround exists. The fix
belongs in libtmux's argv-assembly path, not here.
- tests/test_hook_tools.py:
* Remove ``test_show_hook_missing_returns_empty`` (its use of
``"after-nonexistent-hook-cxyz"`` was actually exercising the
now-removed broad catch).
* Add ``test_show_hook_unset_known_hook_returns_empty`` that sets
and then unsets a real hook (``pane-exited``) before calling
``show_hook`` — exercises the narrow "too many arguments" path.
* Add ``test_show_hook_unknown_name_raises`` asserting that a
bogus hook name now surfaces as ``ToolError`` matching
``r"invalid option|unknown hook"`` (regression guard against
re-broadening the swallow).
…tmcp
why: ``fastmcp_model_classes`` in docs/conf.py is the allow-list that
``sphinx-autodoc-fastmcp`` uses to decide which Pydantic models
get rendered in the generated schema docs. After PR #15 added
seven new models, the tuple still listed only the original ten —
so the new schemas (``SearchPanesResult``, ``PaneSnapshot``,
``ContentChangeResult``, ``HookEntry``, ``HookListResult``,
``BufferRef``, ``BufferContent``) were invisible in the built
API docs, even though they're part of every tool surface that
was documented.
Flagged as Important by Gemini (Skeptic) in the Loom review.
what:
- Add the seven new model names to ``conf["fastmcp_model_classes"]``
in docs/conf.py. Keeps the existing order for the old entries;
new entries are inserted at semantically adjacent positions
(e.g. ``SearchPanesResult`` next to ``PaneContentMatch``,
``ContentChangeResult`` next to ``WaitForTextResult``,
``HookEntry`` / ``HookListResult`` grouped, ``BufferRef`` /
``BufferContent`` grouped).
- Verified post-build: all seven names appear in the generated
reference/api/models/index.html.
…ult shape why: PR #15 wrapped ``search_panes`` output in a ``SearchPanesResult`` model (matches + pagination fields) but the docs/tools/panes.md example still showed the pre-wrapper bare-array response shape. Three reviewers converged on this (Claude + Gemini + GPT in the Loom review, 3-way consensus). what: - Rewrite the response block in docs/tools/panes.md from: [ {pane_id: ..., matched_lines: [...]} ] to the actual wrapper: { matches: [...], truncated, truncated_panes, total_panes_matched, offset, limit } - Add a one-paragraph pagination hint above the example explaining how to iterate larger result sets via ``offset += len(matches)`` until ``truncated == false`` and ``truncated_panes == []``.
why: AGENTS.md §255-287 requires doctests on pure helpers. Most of
the pure helpers introduced by PR #15 have them
(``_validate_channel_name``, ``_validate_logical_name``,
``_validate_buffer_name``, ``_truncate_lines_tail``,
``_tmux_argv``, ``_pane_id_sort_key``). The critic pass surfaced
``_allocate_buffer_name`` as the lone remaining pure helper
without a doctest — an oversight when commit 272396d introduced
the module.
Loom review Suggestion #16 (finalized).
what:
- Expand the docstring on
src/libtmux_mcp/tools/buffer_tools.py::_allocate_buffer_name with
a NumPy-style ``Examples`` block covering:
* prefix contract (``"libtmux_mcp_"``),
* logical-name suffix preservation,
* 32-hex-character uuid nonce length,
* empty-string and ``None`` both collapsing to the ``"buf"``
fallback.
- The docstring also expands the rationale for why the name has the
exact shape it does (privacy prefix + collision-resistant nonce
+ logical suffix), so future maintainers don't reduce the
structure and accidentally re-introduce either the OS-clipboard
read risk or the cross-agent collision risk that commit 272396d
fixed.
65971d8 to
c714044
Compare
…thread around capture_pane why: In commit 67c3fc8 the wait tools became `async def` so that ctx.report_progress could be awaited during long polls. FastMCP direct-awaits async tools on the main event loop (`fastmcp/tools/function_tool.py:277-278`) — it only uses a threadpool for *sync* tools via `call_sync_fn_in_threadpool`. That means every blocking `pane.capture_pane()` subprocess.run inside the poll loop starves every other concurrent MCP request for the duration of the tmux roundtrip (~10s of ms per tick), for the full configured timeout. A 60-second `wait_for_text` call would pin the entire server for 60 seconds, delaying list_sessions / capture_pane / anything else. Loom code review surfaced this with 100% confidence (Gemini Skeptic pass on PR #15); verified against FastMCP source. what: - Wrap the three blocking `pane.capture_pane(...)` calls in src/libtmux_mcp/tools/pane_tools/wait.py with `asyncio.to_thread`: * `wait_for_text` polling call. * `wait_for_content_change` initial snapshot. * `wait_for_content_change` polling call. - Inline comment at the first site explains why (future maintainers will otherwise see "sync call inside async fn" as a bug and try to "fix" it by removing `async def`, reintroducing the blocking path). - No docstring changes — the `async def` signature already promises non-blocking semantics; this commit actually delivers on it. - Add `test_wait_tools_do_not_block_event_loop` in tests/test_pane_tools.py that runs `wait_for_text` against a never-matching pattern inside an asyncio.gather with a ticker coroutine. Asserts the ticker counter >= 5 over the 300 ms wait window — a blocking capture would leave it at 0 or 1. Deliberately a generous lower bound to stay robust on slow CI.
…_wait
why: The ``run_and_wait`` prompt hardcoded ``mcp_done`` as the tmux
wait-for channel name. tmux channels are server-global, so two
concurrent agents (or parallel prompt renderings from one agent)
would race: one agent's ``tmux wait-for -S mcp_done`` would
unblock another's pending ``wait_for_channel("mcp_done")``,
producing a false positive completion signal.
Flagged as Critical by GPT Builder in the Loom review of PR #15.
what:
- Import ``uuid`` in src/libtmux_mcp/prompts/recipes.py.
- Generate a fresh channel name at each call: ``libtmux_mcp_wait_<uuid4hex[:8]>``.
* Prefix aligns with the buffer-tools namespace (libtmux_mcp_) so a
future ``list_buffers(prefix="libtmux_mcp_")``-style operator sees
every MCP-owned tmux artifact uniformly.
* 8 hex characters (~32 bits) is safe against collision inside a
single tmux server's concurrent-agent population.
- Interpolate the same channel name into both the send_keys payload
and the wait_for_channel call so one prompt rendering remains
internally consistent.
- Update the prompt docstring to note the per-invocation scope.
- tests/test_prompts.py:
* Update ``test_run_and_wait_returns_string_template`` to pin the
new prefix.
* Add ``test_run_and_wait_channel_is_uuid_scoped`` asserting
(a) channel matches ``libtmux_mcp_wait_[0-9a-f]{8}``,
(b) send_keys and wait_for_channel use the same name within one
rendering,
(c) two separate renderings emit different channel names.
94c4044 to
bc4c2ed
Compare
why: ``show_hook`` swallowed three tmux error substrings into an empty
result: ``"too many arguments"`` (correctly — that's how tmux
reports an unset hook), but also ``"unknown hook"`` and
``"invalid option"``. The latter two fire for *typos* and
*wrong-scope* mistakes, not "hook is unset". Agents sending
``show_hook("pane-esited")`` (typo) or
``show_hook("pane-exited", scope="server")`` (wrong scope) got
back an empty list indistinguishable from a correctly-named but
unset hook — masking the real input error.
Flagged as Important by both Gemini (Skeptic) and GPT (Builder)
in the Loom review of PR #15. This commit also lands the upstream
TODO comment for Suggestion #15 (libtmux's scope-kwarg argv bug).
what:
- Narrow the ``except libtmux_exc.OptionError`` branch in
src/libtmux_mcp/tools/hook_tools.py::show_hook to only match
``"too many arguments"`` — the single substring all tmux builds
supported by this project use for an unset hook. Any other
OptionError (``"unknown hook"``, ``"invalid option"``, new future
substrings) re-raises so ``handle_tool_errors`` surfaces it.
- Replace the block's comment with the reasoning so a future
maintainer knows not to re-broaden the catch.
- Add a TODO(libtmux upstream) comment in ``_resolve_hook_target``
explaining why the scope-nulling workaround exists. The fix
belongs in libtmux's argv-assembly path, not here.
- tests/test_hook_tools.py:
* Remove ``test_show_hook_missing_returns_empty`` (its use of
``"after-nonexistent-hook-cxyz"`` was actually exercising the
now-removed broad catch).
* Add ``test_show_hook_unset_known_hook_returns_empty`` that sets
and then unsets a real hook (``pane-exited``) before calling
``show_hook`` — exercises the narrow "too many arguments" path.
* Add ``test_show_hook_unknown_name_raises`` asserting that a
bogus hook name now surfaces as ``ToolError`` matching
``r"invalid option|unknown hook"`` (regression guard against
re-broadening the swallow).
…tmcp
why: ``fastmcp_model_classes`` in docs/conf.py is the allow-list that
``sphinx-autodoc-fastmcp`` uses to decide which Pydantic models
get rendered in the generated schema docs. After PR #15 added
seven new models, the tuple still listed only the original ten —
so the new schemas (``SearchPanesResult``, ``PaneSnapshot``,
``ContentChangeResult``, ``HookEntry``, ``HookListResult``,
``BufferRef``, ``BufferContent``) were invisible in the built
API docs, even though they're part of every tool surface that
was documented.
Flagged as Important by Gemini (Skeptic) in the Loom review.
what:
- Add the seven new model names to ``conf["fastmcp_model_classes"]``
in docs/conf.py. Keeps the existing order for the old entries;
new entries are inserted at semantically adjacent positions
(e.g. ``SearchPanesResult`` next to ``PaneContentMatch``,
``ContentChangeResult`` next to ``WaitForTextResult``,
``HookEntry`` / ``HookListResult`` grouped, ``BufferRef`` /
``BufferContent`` grouped).
- Verified post-build: all seven names appear in the generated
reference/api/models/index.html.
…ult shape why: PR #15 wrapped ``search_panes`` output in a ``SearchPanesResult`` model (matches + pagination fields) but the docs/tools/panes.md example still showed the pre-wrapper bare-array response shape. Three reviewers converged on this (Claude + Gemini + GPT in the Loom review, 3-way consensus). what: - Rewrite the response block in docs/tools/panes.md from: [ {pane_id: ..., matched_lines: [...]} ] to the actual wrapper: { matches: [...], truncated, truncated_panes, total_panes_matched, offset, limit } - Add a one-paragraph pagination hint above the example explaining how to iterate larger result sets via ``offset += len(matches)`` until ``truncated == false`` and ``truncated_panes == []``.
why: AGENTS.md §255-287 requires doctests on pure helpers. Most of
the pure helpers introduced by PR #15 have them
(``_validate_channel_name``, ``_validate_logical_name``,
``_validate_buffer_name``, ``_truncate_lines_tail``,
``_tmux_argv``, ``_pane_id_sort_key``). The critic pass surfaced
``_allocate_buffer_name`` as the lone remaining pure helper
without a doctest — an oversight when commit 272396d introduced
the module.
Loom review Suggestion #16 (finalized).
what:
- Expand the docstring on
src/libtmux_mcp/tools/buffer_tools.py::_allocate_buffer_name with
a NumPy-style ``Examples`` block covering:
* prefix contract (``"libtmux_mcp_"``),
* logical-name suffix preservation,
* 32-hex-character uuid nonce length,
* empty-string and ``None`` both collapsing to the ``"buf"``
fallback.
- The docstring also expands the rationale for why the name has the
exact shape it does (privacy prefix + collision-resistant nonce
+ logical suffix), so future maintainers don't reduce the
structure and accidentally re-introduce either the OS-clipboard
read risk or the cross-agent collision risk that commit 272396d
fixed.
…thread around capture_pane why: In commit 67c3fc8 the wait tools became `async def` so that ctx.report_progress could be awaited during long polls. FastMCP direct-awaits async tools on the main event loop (`fastmcp/tools/function_tool.py:277-278`) — it only uses a threadpool for *sync* tools via `call_sync_fn_in_threadpool`. That means every blocking `pane.capture_pane()` subprocess.run inside the poll loop starves every other concurrent MCP request for the duration of the tmux roundtrip (~10s of ms per tick), for the full configured timeout. A 60-second `wait_for_text` call would pin the entire server for 60 seconds, delaying list_sessions / capture_pane / anything else. Loom code review surfaced this with 100% confidence (Gemini Skeptic pass on PR #15); verified against FastMCP source. what: - Wrap the three blocking `pane.capture_pane(...)` calls in src/libtmux_mcp/tools/pane_tools/wait.py with `asyncio.to_thread`: * `wait_for_text` polling call. * `wait_for_content_change` initial snapshot. * `wait_for_content_change` polling call. - Inline comment at the first site explains why (future maintainers will otherwise see "sync call inside async fn" as a bug and try to "fix" it by removing `async def`, reintroducing the blocking path). - No docstring changes — the `async def` signature already promises non-blocking semantics; this commit actually delivers on it. - Add `test_wait_tools_do_not_block_event_loop` in tests/test_pane_tools.py that runs `wait_for_text` against a never-matching pattern inside an asyncio.gather with a ticker coroutine. Asserts the ticker counter >= 5 over the 300 ms wait window — a blocking capture would leave it at 0 or 1. Deliberately a generous lower bound to stay robust on slow CI.
bc4c2ed to
8c76b87
Compare
…_wait
why: The ``run_and_wait`` prompt hardcoded ``mcp_done`` as the tmux
wait-for channel name. tmux channels are server-global, so two
concurrent agents (or parallel prompt renderings from one agent)
would race: one agent's ``tmux wait-for -S mcp_done`` would
unblock another's pending ``wait_for_channel("mcp_done")``,
producing a false positive completion signal.
Flagged as Critical by GPT Builder in the Loom review of PR #15.
what:
- Import ``uuid`` in src/libtmux_mcp/prompts/recipes.py.
- Generate a fresh channel name at each call: ``libtmux_mcp_wait_<uuid4hex[:8]>``.
* Prefix aligns with the buffer-tools namespace (libtmux_mcp_) so a
future ``list_buffers(prefix="libtmux_mcp_")``-style operator sees
every MCP-owned tmux artifact uniformly.
* 8 hex characters (~32 bits) is safe against collision inside a
single tmux server's concurrent-agent population.
- Interpolate the same channel name into both the send_keys payload
and the wait_for_channel call so one prompt rendering remains
internally consistent.
- Update the prompt docstring to note the per-invocation scope.
- tests/test_prompts.py:
* Update ``test_run_and_wait_returns_string_template`` to pin the
new prefix.
* Add ``test_run_and_wait_channel_is_uuid_scoped`` asserting
(a) channel matches ``libtmux_mcp_wait_[0-9a-f]{8}``,
(b) send_keys and wait_for_channel use the same name within one
rendering,
(c) two separate renderings emit different channel names.
why: ``show_hook`` swallowed three tmux error substrings into an empty
result: ``"too many arguments"`` (correctly — that's how tmux
reports an unset hook), but also ``"unknown hook"`` and
``"invalid option"``. The latter two fire for *typos* and
*wrong-scope* mistakes, not "hook is unset". Agents sending
``show_hook("pane-esited")`` (typo) or
``show_hook("pane-exited", scope="server")`` (wrong scope) got
back an empty list indistinguishable from a correctly-named but
unset hook — masking the real input error.
Flagged as Important by both Gemini (Skeptic) and GPT (Builder)
in the Loom review of PR #15. This commit also lands the upstream
TODO comment for Suggestion #15 (libtmux's scope-kwarg argv bug).
what:
- Narrow the ``except libtmux_exc.OptionError`` branch in
src/libtmux_mcp/tools/hook_tools.py::show_hook to only match
``"too many arguments"`` — the single substring all tmux builds
supported by this project use for an unset hook. Any other
OptionError (``"unknown hook"``, ``"invalid option"``, new future
substrings) re-raises so ``handle_tool_errors`` surfaces it.
- Replace the block's comment with the reasoning so a future
maintainer knows not to re-broaden the catch.
- Add a TODO(libtmux upstream) comment in ``_resolve_hook_target``
explaining why the scope-nulling workaround exists. The fix
belongs in libtmux's argv-assembly path, not here.
- tests/test_hook_tools.py:
* Remove ``test_show_hook_missing_returns_empty`` (its use of
``"after-nonexistent-hook-cxyz"`` was actually exercising the
now-removed broad catch).
* Add ``test_show_hook_unset_known_hook_returns_empty`` that sets
and then unsets a real hook (``pane-exited``) before calling
``show_hook`` — exercises the narrow "too many arguments" path.
* Add ``test_show_hook_unknown_name_raises`` asserting that a
bogus hook name now surfaces as ``ToolError`` matching
``r"invalid option|unknown hook"`` (regression guard against
re-broadening the swallow).
…tmcp
why: ``fastmcp_model_classes`` in docs/conf.py is the allow-list that
``sphinx-autodoc-fastmcp`` uses to decide which Pydantic models
get rendered in the generated schema docs. After PR #15 added
seven new models, the tuple still listed only the original ten —
so the new schemas (``SearchPanesResult``, ``PaneSnapshot``,
``ContentChangeResult``, ``HookEntry``, ``HookListResult``,
``BufferRef``, ``BufferContent``) were invisible in the built
API docs, even though they're part of every tool surface that
was documented.
Flagged as Important by Gemini (Skeptic) in the Loom review.
what:
- Add the seven new model names to ``conf["fastmcp_model_classes"]``
in docs/conf.py. Keeps the existing order for the old entries;
new entries are inserted at semantically adjacent positions
(e.g. ``SearchPanesResult`` next to ``PaneContentMatch``,
``ContentChangeResult`` next to ``WaitForTextResult``,
``HookEntry`` / ``HookListResult`` grouped, ``BufferRef`` /
``BufferContent`` grouped).
- Verified post-build: all seven names appear in the generated
reference/api/models/index.html.
…ult shape why: PR #15 wrapped ``search_panes`` output in a ``SearchPanesResult`` model (matches + pagination fields) but the docs/tools/panes.md example still showed the pre-wrapper bare-array response shape. Three reviewers converged on this (Claude + Gemini + GPT in the Loom review, 3-way consensus). what: - Rewrite the response block in docs/tools/panes.md from: [ {pane_id: ..., matched_lines: [...]} ] to the actual wrapper: { matches: [...], truncated, truncated_panes, total_panes_matched, offset, limit } - Add a one-paragraph pagination hint above the example explaining how to iterate larger result sets via ``offset += len(matches)`` until ``truncated == false`` and ``truncated_panes == []``.
why: AGENTS.md §255-287 requires doctests on pure helpers. Most of
the pure helpers introduced by PR #15 have them
(``_validate_channel_name``, ``_validate_logical_name``,
``_validate_buffer_name``, ``_truncate_lines_tail``,
``_tmux_argv``, ``_pane_id_sort_key``). The critic pass surfaced
``_allocate_buffer_name`` as the lone remaining pure helper
without a doctest — an oversight when commit 272396d introduced
the module.
Loom review Suggestion #16 (finalized).
what:
- Expand the docstring on
src/libtmux_mcp/tools/buffer_tools.py::_allocate_buffer_name with
a NumPy-style ``Examples`` block covering:
* prefix contract (``"libtmux_mcp_"``),
* logical-name suffix preservation,
* 32-hex-character uuid nonce length,
* empty-string and ``None`` both collapsing to the ``"buf"``
fallback.
- The docstring also expands the rationale for why the name has the
exact shape it does (privacy prefix + collision-resistant nonce
+ logical suffix), so future maintainers don't reduce the
structure and accidentally re-introduce either the OS-clipboard
read risk or the cross-agent collision risk that commit 272396d
fixed.
…thread around capture_pane why: In commit 67c3fc8 the wait tools became `async def` so that ctx.report_progress could be awaited during long polls. FastMCP direct-awaits async tools on the main event loop (`fastmcp/tools/function_tool.py:277-278`) — it only uses a threadpool for *sync* tools via `call_sync_fn_in_threadpool`. That means every blocking `pane.capture_pane()` subprocess.run inside the poll loop starves every other concurrent MCP request for the duration of the tmux roundtrip (~10s of ms per tick), for the full configured timeout. A 60-second `wait_for_text` call would pin the entire server for 60 seconds, delaying list_sessions / capture_pane / anything else. Loom code review surfaced this with 100% confidence (Gemini Skeptic pass on PR #15); verified against FastMCP source. what: - Wrap the three blocking `pane.capture_pane(...)` calls in src/libtmux_mcp/tools/pane_tools/wait.py with `asyncio.to_thread`: * `wait_for_text` polling call. * `wait_for_content_change` initial snapshot. * `wait_for_content_change` polling call. - Inline comment at the first site explains why (future maintainers will otherwise see "sync call inside async fn" as a bug and try to "fix" it by removing `async def`, reintroducing the blocking path). - No docstring changes — the `async def` signature already promises non-blocking semantics; this commit actually delivers on it. - Add `test_wait_tools_do_not_block_event_loop` in tests/test_pane_tools.py that runs `wait_for_text` against a never-matching pattern inside an asyncio.gather with a ticker coroutine. Asserts the ticker counter >= 5 over the 300 ms wait window — a blocking capture would leave it at 0 or 1. Deliberately a generous lower bound to stay robust on slow CI.
…_wait
why: The ``run_and_wait`` prompt hardcoded ``mcp_done`` as the tmux
wait-for channel name. tmux channels are server-global, so two
concurrent agents (or parallel prompt renderings from one agent)
would race: one agent's ``tmux wait-for -S mcp_done`` would
unblock another's pending ``wait_for_channel("mcp_done")``,
producing a false positive completion signal.
Flagged as Critical by GPT Builder in the Loom review of PR #15.
what:
- Import ``uuid`` in src/libtmux_mcp/prompts/recipes.py.
- Generate a fresh channel name at each call: ``libtmux_mcp_wait_<uuid4hex[:8]>``.
* Prefix aligns with the buffer-tools namespace (libtmux_mcp_) so a
future ``list_buffers(prefix="libtmux_mcp_")``-style operator sees
every MCP-owned tmux artifact uniformly.
* 8 hex characters (~32 bits) is safe against collision inside a
single tmux server's concurrent-agent population.
- Interpolate the same channel name into both the send_keys payload
and the wait_for_channel call so one prompt rendering remains
internally consistent.
- Update the prompt docstring to note the per-invocation scope.
- tests/test_prompts.py:
* Update ``test_run_and_wait_returns_string_template`` to pin the
new prefix.
* Add ``test_run_and_wait_channel_is_uuid_scoped`` asserting
(a) channel matches ``libtmux_mcp_wait_[0-9a-f]{8}``,
(b) send_keys and wait_for_channel use the same name within one
rendering,
(c) two separate renderings emit different channel names.
SearchPanesResult, PaneContentMatch, SessionInfo.active_pane_id, and the four project-local middleware classes (AuditMiddleware, SafetyMiddleware, ReadonlyRetryMiddleware, TailPreservingResponseLimitingMiddleware) now resolve to the API reference pages instead of rendering as bare code. TimingMiddleware and ErrorHandlingMiddleware stay bare — they come from fastmcp and the project has no intersphinx inventory for it.
…es it now The shim was a project-local workaround for two gp-sphinx bugs in ``spa-nav.js::addCopyButtons``: clone-based template failed on pages without code blocks, and only ``div.highlight pre`` was iterated (not the full configured ``copybutton_selector``, so prompt blocks went buttonless after SPA swap). Both are fixed upstream in gp-sphinx (#20): inline HTML template and ``window.GP_SPHINX_COPYBUTTON_SELECTOR`` bridge. The shim's responsibilities collapse into the theme's own code, so remove it from conf.py's ``app.add_js_file`` list and delete the file. ``prompt-copy.js`` stays — it's unrelated: it implements Markdown- preserving copy (backtick-wrapping ``<code>`` children) for prompt admonitions, which sphinx-copybutton's ``innerText``-based copy flattens. That behavior has nothing to do with SPA navigation and uses document-level event delegation to survive DOM swaps.
…d resources
Switch ``docs/tools/prompts.md`` and ``docs/reference/api/resources.md``
from hand-written signatures / ``automodule`` to the new
``{fastmcp-prompt}`` / ``{fastmcp-prompt-input}`` and
``{fastmcp-resource-template}`` directives from gp-sphinx. Each recipe
now has a live-introspected card (name, description, tags, type
badge) and an auto-generated arguments table, while the editorial
prose (``**Use when**``, ``**Why use this**``, ``**Sample render**``)
stays hand-written around the directive blocks.
``docs/conf.py`` now sets ``fastmcp_server_module = "libtmux_mcp.server:mcp"``
so the collector can enumerate the live FastMCP instance. Because
``server.py`` defers registration to ``run_server()``, the collector
invokes ``_register_all()`` internally to populate the component
registry before reading.
Also add three short ``docs/topics/`` pages for the MCP protocol
utilities that map conceptually rather than as first-class autodoc:
- ``completion.md`` — what FastMCP derives automatically from prompt
args and resource-template parameters; notes that live tmux state
is not yet wired in as a completion source.
- ``logging.md`` — the ``libtmux_mcp.*`` logger hierarchy and how
FastMCP forwards records to clients via the MCP logging capability.
- ``pagination.md`` — contrasts MCP protocol-level cursors (automatic,
server-owned) with tool-level ``offset`` / ``limit`` on
``search_panes`` (agent-controlled bounded cost).
Topics index grows a new "MCP protocol utilities" grid row linking
to them.
The fastmcp-prompt directive now creates its own section with a hidden
title (for TOC / {ref} resolution) and a visible card header. The outer
## `run_and_wait` / ## `diagnose_failing_pane` etc. headings caused the
prompt name to appear twice — once as the section heading immediately
above the card, and once in the card's own signature line.
With the headings removed, each prompt name appears exactly once: in the
card header, with the new ¶ permalink from the directive-created section.
The intro bullet {ref} cross-refs continue to resolve correctly.
…e backticks in prompt text - Change all sample render fences from text to markdown for syntax highlighting - Add python language hint to the send_keys/wait_for_channel code block - Convert RST double backticks to single markdown backticks in prompt text (recipes.py) and matching sample renders (prompts.md) — the output is markdown sent to LLMs, not RST
Prompts and Resources are peer MCP concepts alongside Tools, not subtypes — so tools/prompts.md and reference/api/resources.md both read wrong by path. Move them to docs/prompts.md and docs/resources.md, sitting beside tools/index in the "Use it" nav section. rediraffe covers the old URLs.
Following the nav lift, the front-page grid now advertises all three MCP capabilities (Tools, Prompts, Resources) side-by-side instead of burying Prompts/Resources one click deeper.
… hints Quickstart now picks the MCP client (Claude Code / Claude Desktop / Codex / Gemini / Cursor) up front, then nests install-method tabs (uvx / pipx / pip install) inside — so every client gets a copy-pasteable first command without bouncing to clients.md. uvx and pipx tabs lead with "With [uv]/[pipx] installed:" and link to the respective tool docs; pip install tabs show the `pip install --user --upgrade libtmux libtmux-mcp` prereq before the per-client registration step. clients.md mirrors the same structure so the two pages don't drift.
85f20cb to
9353282
Compare
Code reviewFound 1 issue:
Representative examples (each touches libtmux-mcp/docs/_ext/widgets/_base.py Lines 4 to 9 in 34a7f3c libtmux-mcp/docs/_ext/widgets/mcp_install.py Lines 4 to 9 in 34a7f3c libtmux-mcp/tests/docs/test_widgets.py Lines 3 to 11 in 34a7f3c Suggested mechanical fix: 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
34a7f3c to
6ac122c
Compare
…cker
New in-tree Sphinx extension at docs/_ext/widgets/ that autodiscovers
BaseWidget subclasses and renders each via a Jinja2 template shipped
at docs/_widgets/<name>/widget.{html,js,css}. Assets are copied into
_static/widgets/<name>/ at build time and registered globally via
app.add_{css,js}_file.
First concrete widget is MCPInstallWidget (`{mcp-install}` directive).
It replaces the ~200-line nested sphinx-inline-tabs block in
quickstart.md with a single 5×3 client/method picker, and drops a
compact variant above the index.md grid. Tab state syncs across
widgets on the same page via CustomEvent, persists per-user via
localStorage, and survives gp-sphinx SPA navigation via document-
level event delegation + gp-sphinx:navigated listener.
Tests at tests/docs/test_widgets.py cover autodiscovery, the client×
method matrix, rendering, asset copy, missing-template handling,
option validation, and env.note_dependency tracking. Root conftest
registers sphinx.testing.fixtures so the Sphinx build fixtures are
available to pytest.
tests/docs imports the widget framework as ``from widgets ...`` (the same way docs/conf.py loads it, via a sys.path insertion). Under mypy strict that raises "Cannot find implementation or library stub for module named 'widgets'". Listing ``docs/_ext`` under ``mypy_path`` alone -- rather than adding it to ``files`` -- avoids the reciprocal "source file found twice under different module names" error that would fire if mypy discovered the package via both its parent path and as a top-level module. ``exclude = ["^docs/"]`` is also required so that the CI invocation ``uv run mypy .`` -- which traverses every path under the current directory -- doesn't rediscover files in ``docs/_ext/widgets/`` under their ``docs._ext.`` prefix and error with the same "found twice" message. The widgets package is still fully type-checked via the ``mypy_path`` entry; the exclude only affects discovery-by-traversal. Both settings are needed from the moment the widgets package was introduced (17a406a); the ``mypy .`` failure was only surfaced by CI after tests/docs landed.
syrupy provides the ``snapshot`` assertion fixture used by the forthcoming widget highlight-parity tests to record byte-level HTML for regression catch. Chosen to match gp-sphinx's own docs tests (which use the same version) so the snapshot-fixture pattern ports over verbatim.
… parity Code blocks inside the mcp-install widget now go through Sphinx's PygmentsBridge via a new ``highlight`` Jinja filter (registered in BaseWidget.render). Output is byte-identical to Sphinx's native visit_literal_block, so the theme's highlighting CSS and sphinx-copybutton's default selector + gp-sphinx's prompt-strip regex apply automatically — "$ " is tagged ``<span class="gp">`` and dropped from the copied text. The widget data renames the CLI language from "shell" to "console" (ShellSessionLexer) and prepends "$ " to each CLI body; the prereq ``pip install`` block and the client-config JSON now both render through the same filter path. Tests gain a HighlightCase NamedTuple + three parametrized tests (``console-claude-code-uvx``, ``console-pip-prereq``, ``json-mcp-config-uvx``) that build a real ``.. code-block::`` via SphinxTestApp, extract the rendered block, and assert byte equality with the widget filter output. Syrupy captures snapshots of the shared HTML for regression catch. Helpers ported from gp-sphinx's ``tests/_snapshots.py`` as ``tests/docs/_snapshots.py`` with a Protocol-typed fixture.
… list sphinx-autodoc-fastmcp registers section labels as ``fastmcp-<kind>-<slug>`` and only tools keep the bare slug as an alias (gp-sphinx/sphinx_autodoc_fastmcp/_directives.py:82-104). The bullet-list refs in docs/prompts.md were using bare slugs (``run-and-wait`` etc.) that were never registered for prompts, producing four ``undefined label`` warnings on every build. Rename each ref to its canonical namespaced form.
6ac122c to
906dc40
Compare
Code reviewNo issues found. Checked for bugs and AGENTS.md compliance against the atomized commits (6b46a51..906dc40). The namespace-imports violation from the prior review is resolved. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
``make_highlight_filter`` dereferenced ``env.app.builder.highlighter``
at filter-registration time, but that attribute only exists on
``StandaloneHTMLBuilder`` and its subclasses. Running
``sphinx-build -b text|linkcheck|gettext|man`` on a page that uses the
``{mcp-install}`` directive crashed with ``AttributeError: 'TextBuilder'
object has no attribute 'highlighter'`` because ``SphinxDirective.run()``
executes during doctree construction for every builder.
The prior comment ("Callers are guaranteed an HTML builder by the
``builder.format == 'html'`` guard in ``install_widget_assets``") was
wrong -- that guard protects only the asset-copy hook, not the render
path.
Fix: narrow via ``isinstance(builder, StandaloneHTMLBuilder)`` (which
also covers DirectoryHTMLBuilder + SingleFileHTMLBuilder). For non-HTML
builders, fall back to an HTML-escaped ``<pre>`` block. The isinstance
narrow eliminates the ``# type: ignore[attr-defined]`` that previously
covered the bare attribute access.
Adds ``test_widget_renders_with_text_builder`` which drives the text
builder end-to-end and would AttributeError before this fix.
``get_lexer_by_name("console")`` returns ``BashSessionLexer``
(``pygments/lexers/shell.py:223``, alias registration at line 230), which
extends ``ShellSessionBaseLexer``. There is no class literally named
``ShellSessionLexer``. The behavioural claim in the comment -- that the
lexer tags ``$ `` as ``Generic.Prompt`` and emits ``<span class="gp">`` --
is unchanged and still correct.
The comment in ``_sphinx_native_html`` cited ``writers/html5.py:626-629`` which drifted relative to the installed Sphinx 8.2.3 (actual: ``visit_literal_block`` at 589, ``highlight_block`` call at 603, ``'</div>\n'`` append at 614). Drop the line-number suffix and keep the class-plus-method anchor -- the latter is a stable API reference that won't rot. (``_base.py``'s matching reference was updated in the non-HTML-builder commit.)
why: The `2026-04-follow-ups` branch (FastMCP alignment: new tools, prompts, and middleware, #15) has merged and the accumulated unreleased entries — new tools, prompts, full middleware stack, bounded outputs, plus the search_panes / fastmcp minimum-version breaking changes — constitute a shippable alpha point release. The previous published version was 0.1.0a1 on 2026-04-13; everything between v0.1.0a1 and main now belongs under a finalized release block. what: - pyproject.toml: version 0.1.0a1 -> 0.1.0a2 - src/libtmux_mcp/__about__.py: __version__ 0.1.0a1 -> 0.1.0a2 - uv.lock: regenerated via `uv sync` (libtmux-mcp entry updated) - CHANGES: introduce a `## libtmux-mcp 0.1.0a2 (2026-04-19)` heading between the rolling `0.1.x (unreleased)` placeholder and the existing `_FastMCP alignment..._` tagline. The placeholder block stays at the top for the next iteration; the Breaking changes, What's new, Fixes, and Documentation entries that had accumulated under "unreleased" are now the content of the 0.1.0a2 release block. No bullet text changed.
why: The `2026-04-follow-ups` branch (FastMCP alignment: new tools, prompts, and middleware, #15) has merged and the accumulated unreleased entries — new tools, prompts, full middleware stack, bounded outputs, plus the search_panes / fastmcp minimum-version breaking changes — constitute a shippable alpha point release. The previous published version was 0.1.0a1 on 2026-04-13; everything between v0.1.0a1 and main now belongs under a finalized release block. what: - pyproject.toml: version 0.1.0a1 -> 0.1.0a2 - src/libtmux_mcp/__about__.py: __version__ 0.1.0a1 -> 0.1.0a2 - uv.lock: regenerated via `uv sync` (libtmux-mcp entry updated) - CHANGES: introduce a `## libtmux-mcp 0.1.0a2 (2026-04-19)` heading between the rolling `0.1.x (unreleased)` placeholder and the existing `_FastMCP alignment..._` tagline. The placeholder block stays at the top for the next iteration; the Breaking changes, What's new, Fixes, and Documentation entries that had accumulated under "unreleased" are now the content of the 0.1.0a2 release block. No bullet text changed.
why: The `2026-04-follow-ups` branch (FastMCP alignment: new tools, prompts, and middleware, #15) has merged and the accumulated unreleased entries — new tools, prompts, full middleware stack, bounded outputs, plus the search_panes / fastmcp minimum-version breaking changes — constitute a shippable alpha point release. The previous published version was 0.1.0a1 on 2026-04-13; everything between v0.1.0a1 and main now belongs under a finalized release block. what: - pyproject.toml: version 0.1.0a1 -> 0.1.0a2 - src/libtmux_mcp/__about__.py: __version__ 0.1.0a1 -> 0.1.0a2 - uv.lock: regenerated via `uv sync` (libtmux-mcp entry updated) - CHANGES: introduce a `## libtmux-mcp 0.1.0a2 (2026-04-19)` heading between the rolling `0.1.x (unreleased)` placeholder and the existing `_FastMCP alignment..._` tagline. The placeholder block stays at the top for the next iteration; the Breaking changes, What's new, Fixes, and Documentation entries that had accumulated under "unreleased" are now the content of the 0.1.0a2 release block. No bullet text changed.
Summary
FastMCP alignment for libtmux-mcp: new tool families, prompt recipes, middleware stack, bounded outputs, and correctness fixes.
Breaking changes
search_panesreturnsSearchPanesResult(waslist[PaneContentMatch]). Matches move to.matches; new pagination fields. Migration:for m in search_panes(...).matches.fastmcp>=3.2.4.New tools
list_servers.wait_for_text,wait_for_content_change,wait_for_channel,signal_channel. Bounded, cancellable, emitctx.report_progress/ctx.warning.load_buffer,paste_buffer,show_buffer,delete_buffer. UUID-namespaced; leaked buffers GC'd on shutdown.show_hook,show_hooks.snapshot_pane,pipe_pane,display_message,paste_text,select_pane,swap_pane,select_window,move_window,enter_copy_mode,exit_copy_mode.New prompts
Four recipes:
run_and_wait,diagnose_failing_pane,build_dev_workspace,interrupt_gracefully. Expose as tools withLIBTMUX_MCP_PROMPTS_AS_TOOLS=1.Middleware
TimingMiddleware,ErrorHandlingMiddleware,AuditMiddleware,SafetyMiddleware,ReadonlyRetryMiddleware,TailPreservingResponseLimitingMiddleware.Bounded outputs
capture_pane,snapshot_pane,show_buffertakemax_lines(default 500) with tail-preserving truncation. Passmax_lines=Noneto opt out.Fixes
search_panes— neutralize tmux format-string injection.TMUX_TMPDIRself-kill guard — resolve socket viadisplay-messagebefore env fallback.build_dev_workspaceprompt — real parameter names, drop post-launch prompt waits, OS-neutrallog_command.Test plan
uv run ruff check . && uv run ruff format --check .uv run mypyuv run py.test --reruns 0— 276 tests passjust build-docstmuxrenamed onPATH→ cleanRuntimeErrorfrom lifespan probecapture_paneon a >50 KB scrollback pane withmax_lines=None→ head trimmed, tail preservedsearch_panespagination viaoffset/limitwait_for_channel+signal_channelround-tripLIBTMUX_MCP_PROMPTS_AS_TOOLS=1→ prompts in tool list