test: harden the suite against tests that cannot fail

A mutation audit of all 120 tests this branch adds — each one probed by
breaking the source it covers and re-running — found fifteen that passed
against the very regression they named. Every fix below was verified by
re-running the mutation that exposed it.

One defect shape dominated: an assertion loose enough to admit the broken
value. `pytest.raises(BaseException)` also catches the TimeoutError from a
hang. `all(size <= 101)` is satisfied by the -1 that an unbounded read()
records. `pytest.raises(RequestError)` accepts every JSON-RPC error,
including the -32603 that one of these tests exists specifically to rule
out. `assert not any(...)` is satisfied by a handler that never ran.

Four tests hung rather than failed. A busy-turn regression wedged the
suite indefinitely; two subprocess prompts did the same; a slash command
that stopped reaching the host loop blocked forever. There is no
pytest-timeout plugin, so each of those stalls CI instead of reporting —
and a hung job reads as "still running", not "broken". All four are now
bounded and fail in seconds.

Coverage gaps closed: the protection guard had no negative control, so
deleting its opt-in — which turns a warning into a hard error for every
ordinary skill — left all 23 tests green. The tool_call_timeout regression
covered the new factory but not create_from_server, where the bug actually
lived. Transport validation, header copying, the legacy TOML guard, the
env-override branch, registry sorting, duplicate rejection and
case-insensitive lookup had no coverage at all.

Adapters are now closed by an autouse fixture. Forty-five were built and
thirty-eight closed, every close a last statement, so a failing assertion
abandoned the pump task, agent and SQLite handle on the shared loop — one
failure was observed cascading into unrelated failures later.

Rebased onto main, which had moved 48 commits. RELEASING.md took main's
rewritten text with the ACP facts re-applied, and picked up two package
counts main still had at four; make_release.py kept both smoke checks.

Signed-off-by: Paul Furgale <pfurgale@nvidia.com>

Author: furgalep

packages/nooa-acp/tests/test_event_bridge.py

@@ -216,6 +216,19 @@ async def test_bridge_omits_usage_when_context_window_is_unknown(tmp_path):
     await bridge.close()
     await agent.close()
 
+    # Paired positive: the same event with a known context window must emit a
+    # UsageUpdate. Without this, `return` at the top of _on_llm_complete passes
+    # both halves — an AgentMessageChunk control comes from a different handler
+    # and cannot tell "the guard works" from "usage never fires".
+    sized = CodingAgent(llm=FakeLLMClient(), cwd=tmp_path)
+    sized_client = _RecordingClient()
+    sized_bridge = ACPEventBridge(sized, sized_client, "session-2")  # type: ignore[arg-type]
+    sized.event_manager.add(LLMComplete(prompt_tokens=40, completion_tokens=10, cost_usd=0.25))
+    await sized_bridge.flush()
+    assert any(isinstance(update, UsageUpdate) for _, update in sized_client.updates)
+    await sized_bridge.close()
+    await sized.close()
+
 
 async def test_bridge_emits_structured_file_edit(tmp_path):
     agent = CodingAgent(llm=FakeLLMClient(), cwd=tmp_path)
@@ -389,6 +402,9 @@ async def test_a_cancelled_command_reads_as_cancellation_not_a_crash(tmp_path):
     rendered = str(finished)
     assert "Cancelled by user." in rendered
     assert "CancelledError" not in rendered
+    # Status too: dropping `or event.cancelled` renders a cancelled command as a
+    # green completed card while the reason text still reads correctly.
+    assert finished.status == "failed"
     await bridge.close()
 
 
@@ -417,7 +433,7 @@ async def test_bare_expression_result_is_shown_not_reported_as_no_output(tmp_pat
     await bridge.flush()
 
     rendered = "".join(str(update) for _, update in client.updates)
-    assert "42" in rendered
+    assert "Out[3]: 42" in rendered, rendered
     assert "Completed." not in rendered
     await bridge.close()
 
@@ -461,6 +477,15 @@ async def test_an_unfinished_tool_call_does_not_leak_for_the_session(tmp_path):
     await bridge.close()
     assert bridge._open_tools == set()
     assert bridge._python_source == {}
+    # And the card was actually closed out for the client: clearing the private
+    # state alone leaves it spinning, which is what the docstring forbids.
+    closing = [
+        update
+        for _, update in client.updates
+        if isinstance(update, ToolCallProgress) and update.tool_call_id == "t3"
+    ]
+    assert closing and closing[-1].status == "failed", client.updates
+    assert closing[-1].title == "Unfinished"
 
 
 async def test_a_cancelled_tool_card_is_titled_cancelled(tmp_path):

packages/nooa-acp/tests/test_protocol.py

@@ -6,6 +6,7 @@
 import sys
 from pathlib import Path
 
+import pytest
 from acp import PROTOCOL_VERSION, spawn_agent_process, text_block
 from acp.connection import StreamDirection
 from acp.schema import (
@@ -88,7 +89,10 @@ async def test_acp_subprocess_transcript(tmp_path, monkeypatch):
         initialized = await connection.initialize(PROTOCOL_VERSION)
         session = await connection.new_session(str(tmp_path))
         await asyncio.wait_for(client.commands_updated.wait(), timeout=5)
-        response = await connection.prompt(session.session_id, [text_block("run smoke test")])
+        response = await asyncio.wait_for(
+            connection.prompt(session.session_id, [text_block("run smoke test")]),
+            timeout=_HANG_TIMEOUT,
+        )
 
     assert initialized.agent_info is not None
     assert initialized.agent_info.name == "nooa-acp"
@@ -156,9 +160,12 @@ async def test_acp_subprocess_dispatches_advertised_slash_command(tmp_path, monk
     ) as (connection, _process):
         await connection.initialize(PROTOCOL_VERSION)
         session = await connection.new_session(str(tmp_path))
-        response = await connection.prompt(
-            session.session_id,
-            [text_block("/protocol-check ready")],
+        response = await asyncio.wait_for(
+            connection.prompt(
+                session.session_id,
+                [text_block("/protocol-check ready")],
+            ),
+            timeout=_HANG_TIMEOUT,
         )
 
     assert response.stop_reason == "end_turn"
@@ -219,6 +226,14 @@ async def test_acp_subprocess_closes_a_session_over_the_wire(tmp_path):
         session = await connection.new_session(str(tmp_path))
         await connection.close_session(session.session_id)
 
+        # Routable is only half of it: a no-op handler leaks the runtime for the
+        # process lifetime. Prompting a closed session must now be rejected.
+        with pytest.raises(Exception, match="(?i)not found|no such|unknown"):
+            await asyncio.wait_for(
+                connection.prompt(session.session_id, [text_block("still there?")]),
+                timeout=_HANG_TIMEOUT,
+            )
+
     capabilities = initialized.agent_capabilities.session_capabilities
     assert capabilities is not None and capabilities.close is not None
 

packages/nooa-acp/tests/test_runtime.py

@@ -14,6 +14,9 @@
     SessionRuntimePool,
 )
 
+# Bounds a hang, not the expected duration.
+_HANG_TIMEOUT = 30
+
 
 class _RuntimeValue:
     def __init__(self) -> None:
@@ -25,11 +28,17 @@ async def close(self) -> None:
 
 async def test_same_session_rejects_a_second_foreground_turn():
     runtime = SessionRuntime("one", object())
+
+    async def _claim_again() -> None:
+        async with runtime.turn():
+            pass
+
     async with runtime.turn():
         assert runtime.busy is True
+        # Bounded: if turns start queueing instead of failing fast — the exact
+        # regression — this wedges on the inner lock and hangs the suite.
         with pytest.raises(SessionBusyError):
-            async with runtime.turn():
-                pass
+            await asyncio.wait_for(_claim_again(), timeout=_HANG_TIMEOUT)
 
 
 async def test_simultaneous_turn_claims_do_not_queue():
@@ -95,7 +104,10 @@ async def active_turn() -> None:
     turn_task = asyncio.create_task(active_turn())
     await turn_started.wait()
     close_task = asyncio.create_task(runtime.close())
-    await asyncio.sleep(0)
+    # A single yield is satisfied by scheduling latency — _close_once has not
+    # even started — so it passes with the turn lock removed entirely.
+    for _ in range(20):
+        await asyncio.sleep(0)
     assert close_task.done() is False
     assert value.close_calls == 0
     release_turn.set()

packages/nooa-acp/tests/test_server.py

@@ -6,6 +6,7 @@
 import os
 import sys
 import threading
+from contextlib import suppress
 from typing import Literal
 from unittest.mock import AsyncMock, call, patch
 
@@ -54,6 +55,37 @@ def isolated_user_config(tmp_path_factory, monkeypatch):
     return home
 
 
+# JSON-RPC code for resource_not_found; asserting it distinguishes a typed
+# protocol error from the generic -32603 internal_error.
+_RESOURCE_NOT_FOUND = -32002
+
+
+@pytest.fixture(autouse=True)
+async def close_every_adapter(monkeypatch):
+    """Guarantee teardown for every adapter a test builds.
+
+    Each test closes its adapter as the last statement, so a failing assertion
+    leaks the pump task, bootstrap task, agent, MCP tools and SQLite handle onto
+    the shared event loop for the rest of the module — which has been observed
+    turning one failure into unrelated intermittent failures later. Closing is
+    idempotent, so tests keep their explicit close and this only covers the
+    paths that do not reach it.
+    """
+    built: list[CodingACPAdapter] = []
+    original = CodingACPAdapter.__init__
+
+    def tracking_init(self, *args, **kwargs):
+        original(self, *args, **kwargs)
+        built.append(self)
+
+    monkeypatch.setattr(CodingACPAdapter, "__init__", tracking_init)
+    yield built
+
+    for adapter in built:
+        with suppress(Exception):
+            await adapter.close()
+
+
 def _completed_llm() -> FakeLLMClient:
     return FakeLLMClient.with_tool_call(
         "execute_python",
@@ -1267,8 +1299,11 @@ async def test_prompt_on_a_closing_session_is_a_clean_protocol_error(tmp_path):
     runtime = await adapter._sessions.get(session.session_id)
     await runtime.close()
 
-    with pytest.raises(RequestError):
+    # The code, not merely "some RequestError": a bare raises() accepts the
+    # -32603 internal_error this test exists to rule out.
+    with pytest.raises(RequestError) as raised:
         await adapter.prompt(session.session_id, [text_block("do the work")])
+    assert raised.value.code == _RESOURCE_NOT_FOUND, raised.value.code
     await adapter.close()
 
 

packages/nooa-cli/tests/test_coding_activity.py

@@ -3,6 +3,7 @@
 """Semantic activity emitted by the shared interactive coding tools."""
 
 import asyncio
+from pathlib import Path
 
 import nooa_cli.coding.activity as activity
 import pytest
@@ -314,3 +315,56 @@ async def test_overwriting_an_empty_file_reports_no_original_lines(tmp_path):
     edit = next(event for event in events if isinstance(event, FileEdit))
     assert edit.operation == "update"
     assert (edit.start_line, edit.end_line) == (None, None)
+
+
+async def test_a_path_outside_the_workspace_is_made_relative(tmp_path):
+    """The fallback branch was unreachable from the other tests.
+
+    Every fixture file lives under tmp_path, which is the shell cwd, so
+    relative_to() always succeeds and the except branch never ran — leaving
+    `assert "a//" not in edit.diff` unable to fail.
+    """
+    shell, _ = _observed_shell(tmp_path)
+    try:
+        assert shell._diff_path(Path("/etc/hosts")) == "etc/hosts"
+        inside = tmp_path / "kept.txt"
+        assert shell._diff_path(inside) == "kept.txt"
+    finally:
+        await shell.close()
+
+
+async def test_overwrite_reads_the_previous_content_boundedly(tmp_path):
+    """The bound must be on the read, not only on what is emitted.
+
+    The other activity tests assert on the emitted event, which is produced by
+    pformat/TruncatingStringIO *after* the read — so reverting to an unbounded
+    read_text() of a workspace-controlled file is invisible to them.
+    """
+    reads: list[int] = []
+    real_open = Path.open
+
+    def spying_open(self, *args, **kwargs):
+        stream = real_open(self, *args, **kwargs)
+        real_read = stream.read
+
+        def read(size=-1):
+            reads.append(size)
+            return real_read(size)
+
+        stream.read = read  # type: ignore[method-assign]
+        return stream
+
+    target = tmp_path / "big.txt"
+    target.write_text("x" * 5000)
+    shell, _ = _observed_shell(tmp_path)
+    try:
+        with pytest.MonkeyPatch.context() as patcher:
+            patcher.setattr(Path, "open", spying_open)
+            await shell.write_file("big.txt", "replacement\n")
+    finally:
+        await shell.close()
+
+    # Positive sizes only: an unbounded read records -1, which satisfies any
+    # "<= limit" assertion.
+    assert reads, "the previous content was never read"
+    assert all(0 < size <= activity._MAX_DIFF_INPUT_CHARS + 1 for size in reads), reads

packages/nooa-cli/tests/test_coding_agent.py

@@ -178,7 +178,10 @@ def read(size=-1):
 
     rendered = instructions.render_agent_instructions(tmp_path)
 
-    assert reads and all(size is not None and size <= 101 for size in reads), reads
+    # Positive sizes only: an unbounded .read() records -1, which satisfies
+    # any `<= limit` assertion and made this test pass against the very
+    # regression it names.
+    assert reads == [101], reads
     assert "[... truncated ...]" in rendered
     assert len(rendered) < 1_000
 

packages/nooa-cli/tests/test_coding_settings.py

@@ -188,3 +188,42 @@ def test_a_non_utf8_settings_file_does_not_abort_discovery(tmp_path, monkeypatch
     (config_dir / "settings.yaml").write_bytes(b"\xff\xfe coding:\n")
 
     assert load_coding_skills_dirs(workspace) == [conventional.resolve()]
+
+
+def test_a_non_utf8_legacy_config_does_not_abort_discovery(tmp_path, monkeypatch):
+    """The legacy TOML reader needs the same UnicodeError guard as the YAML one.
+
+    The sibling test only feeds a bad settings.yaml, so dropping UnicodeError
+    from the config.toml handler went unnoticed.
+    """
+    workspace = tmp_path / "workspace"
+    conventional = workspace / ".agents" / "skills"
+    conventional.mkdir(parents=True)
+    monkeypatch.delenv("NEMO_OO_SETTINGS", raising=False)
+
+    config_dir = workspace / ".nooa"
+    config_dir.mkdir()
+    (config_dir / "config.toml").write_bytes(b"\xff\xfe [tui]\n")
+
+    assert load_coding_skills_dirs(workspace) == [conventional.resolve()]
+
+
+def test_an_env_override_suppresses_a_legacy_only_workspace(tmp_path, monkeypatch):
+    """The env half of the legacy guard had no test.
+
+    Every existing case also has a modern settings.yaml, so `modern_key_set`
+    short-circuits and the NEMO_OO_SETTINGS check is never exercised.
+    """
+    workspace = tmp_path / "workspace"
+    legacy = tmp_path / "legacy-skills"
+    override = tmp_path / "override.yaml"
+    workspace.mkdir()
+    legacy.mkdir()
+    override.write_text("coding:\n  additional_skills_dirs: []\n")
+    monkeypatch.setenv("NEMO_OO_SETTINGS", str(override))
+
+    config_dir = workspace / ".nooa"
+    config_dir.mkdir()
+    (config_dir / "config.toml").write_text(f'[tui]\nlibs_dirs = ["{legacy}"]\n')
+
+    assert load_coding_skills_dirs(workspace) == []

packages/nooa-cli/tests/test_coding_slash_commands.py

@@ -121,11 +121,57 @@ async def wait(self) -> str:
     registry = CodingSlashCommandRegistry(agent)
     try:
         invocation = asyncio.create_task(registry.invoke("wait", ""))
-        await started.wait()
+        # Bounded: a regression that stops the coroutine reaching the host loop
+        # would otherwise wedge here forever, so CI stalls instead of going red.
+        await asyncio.wait_for(started.wait(), timeout=30)
         invocation.cancel()
 
         with pytest.raises(asyncio.CancelledError):
-            await invocation
+            await asyncio.wait_for(invocation, timeout=30)
+    finally:
+        registry.close()
+        await agent.close()
+
+
+async def test_commands_are_sorted_deduplicated_and_case_insensitive(tmp_path):
+    """Three registry behaviours that no test exercised.
+
+    Every existing case registers one command and reads it back in canonical
+    case, so unsorted output, a dropped duplicate guard, and lost case
+    normalisation were all invisible.
+    """
+
+    class _Beta(Skill):
+        @slash_command("beta")
+        def beta(self, args: str) -> str:
+            """Beta."""
+            return "beta"
+
+    class _Alpha(Skill):
+        @slash_command("alpha")
+        def alpha(self, args: str) -> str:
+            """Alpha."""
+            return "alpha"
+
+    class _AlphaAgain(Skill):
+        @slash_command("alpha")
+        def alpha(self, args: str) -> str:
+            """Duplicate."""
+            return "duplicate"
+
+    agent = CodingAgent(llm=FakeLLMClient(), cwd=tmp_path)
+    # Registered out of order, and with a colliding name.
+    # Registry names sort opposite to the command names they provide, so
+    # _commands is built in [beta, alpha] order and sorting is observable.
+    agent.skills.register("test.aaa", _Beta())
+    agent.skills.register("test.zzz", _Alpha())
+    agent.skills.register("test.zzzz", _AlphaAgain())
+    registry = CodingSlashCommandRegistry(agent)
+    try:
+        names = [command.name for command in registry.commands()]
+        assert names == sorted(names), names
+        assert names.count("alpha") == 1, names
+        assert registry.get("ALPHA") is not None, "lookup is not case-insensitive"
     finally:
         registry.close()
         await agent.close()