fix(acp): reset is_running when prompt() raises before the executor guard

[israellot]

What & why

prompt() in acp_adapter/server.py sets state.is_running = True under the runtime lock, but its exception protection only begins at the executor call much later. Everything between the flip and the executor try — callback-factory construction (make_tool_progress_cb / make_message_cb / …), the edit-approval requester wiring, agent callback assignment — is an unprotected raise window.

If anything in that setup region raises, the exception escapes with is_running still True and the session is permanently bricked: every subsequent user message hits the busy guard and is answered with Queued for the next turn. (N queued), and no turn ever drains the queue. The only recovery is restarting the editor / agent process.

Fix at the root cause: open one outer try: immediately after the runtime-lock block that sets is_running=True, enclosing the setup region, the executor call, and the existing post-turn tail. It closes with except BaseException: that resets is_running / current_prompt_text under the runtime lock and re-raises. The existing inner executor except (which also resets and shapes the error response) is kept unchanged — the outer reset is idempotent with it. The busy-branch early returns happen before is_running is set and stay outside the guard.

The diff is mostly re-indentation of the guarded block; git diff -w shows the real change is ~18 added lines.

How to test

bash scripts/run_tests.sh tests/acp/test_server.py -q
bash scripts/run_tests.sh tests/acp/ -q

New test: TestPrompt::test_prompt_setup_exception_still_releases_session — patches acp_adapter.server.make_message_cb (a setup-region callback factory) to raise, calls prompt(), and asserts:

1. the exception propagates (behavior unchanged for the caller),
2. state.is_running is False afterwards and current_prompt_text is cleared,
3. a follow-up prompt() on the same session runs (run_conversation mocked) instead of queueing into a dead session, and queued_prompts stays empty.

Before the fix, step 2 fails: AssertionError: setup exception left is_running=True — session bricked.

Verification on this branch:

• bash scripts/run_tests.sh tests/acp/test_server.py -q → 87 passed, 0 failed
• bash scripts/run_tests.sh tests/acp/ -q → 311 passed, 0 failed (13 files)
• scripts/check-windows-footguns.py acp_adapter/server.py tests/acp/test_server.py → no footguns

Reproduction sketch

Any transient failure in the setup region reproduces it, e.g. a raise inside a callback factory or the requester wiring:

# after session/new, make any setup-region call raise once:
with patch("acp_adapter.server.make_message_cb", side_effect=RuntimeError("boom")):
    await agent.prompt(prompt=[TextContentBlock(type="text", text="hi")], session_id=sid)
# session is now bricked without this fix:
await agent.prompt(...)  # -> "Queued for the next turn. (1 queued)" forever

Platforms tested

Linux fully verified; Windows/macOS not manually tested (change is pure Python control flow, no platform-specific I/O; footguns check clean).

Related — independent

• ACP drops background completion notifications after prompt returns #62548 (ACP drops background completion notifications after prompt returns) — this bug was found while building on that area; not fixed by this PR.
• fix(acp): cancelled turn with final_response=None permanently bricks the session (every prompt queued forever) #64471 (cancelled turn with final_response=None bricks the session) — same brick symptom, different raise window (post-turn tail vs. this PR's pre-executor setup region); independent, they compose.
• fix(acp): don't lose queued prompts when the drain echo hits a dead connection #71012 (queued prompts lost when the drain echo hits a dead connection) — touches the same file from the same base; independent.

Prior art: this fix has been running in a public fork — YallaPlay/hermes-agent@6942586

[teknium1]

Thanks — the reported failure window is present on current main: prompt() sets state.is_running = True at acp_adapter/server.py:1652, calls callback factories at acp_adapter/server.py:1686-1696, and does not enter its existing executor guard until acp_adapter/server.py:1843. A setup exception can therefore bypass the resets at acp_adapter/server.py:1857-1859; later input follows the busy queue path at acp_adapter/server.py:1627-1668.

Problems

• GitHub reports this branch as conflicting. Since its base (7cd48733) current main added the session-cwd binding at acp_adapter/server.py:1764-1773; resolving the broad reindent mechanically risks losing that newer behavior.

Suggested changes

• Salvage the outer reset guard onto current prompt() while preserving the cwd binding and existing executor-specific error response.
• Move the regression test into the current TestPrompt section at tests/acp/test_server.py:404, because the historical test context was pruned on main.

This is an automated hermes-sweeper review.

[israellot]

Both points addressed in 92f53b665 (branch rebuilt directly on current main at 14db1a99e, so the conflict is gone — GitHub now reports mergeable: MERGEABLE).

1. Conflicting branch / risk of losing the newer session-cwd binding. Rather than resolving the old reindent mechanically, I reset the branch to current main and re-applied only the guard. The cwd binding is preserved verbatim — set_session_vars(session_key=session_id, cwd=state.cwd) and its explanatory comment are still there (now at acp_adapter/server.py:1770-1784, just one indent level deeper inside the guard). The executor-specific except that shapes the error response is likewise untouched; the new outer except BaseException only resets is_running / current_prompt_text under state.runtime_lock and re-raises, so it is idempotent with the inner resets. The busy-queue early returns still sit before the flip and stay outside the guard.

git diff -w upstream/main..HEAD → acp_adapter/server.py | 18 ++++++++++++++++++ — the whole functional change is the 11-line comment + try: at acp_adapter/server.py:1670-1680 and the 7-line except BaseException: at acp_adapter/server.py:1998-2004; everything else in the raw diff is re-indentation of the guarded block.

2. Test moved into the current TestPrompt section. test_prompt_setup_exception_still_releases_session now lives at tests/acp/test_server.py:413, immediately after test_prompt_returns_refusal_for_unknown_session in the surviving TestPrompt class at tests/acp/test_server.py:404. It uses only fixtures that exist on current main (agent, MagicMock(spec=acp.Client)), patches acp_adapter.server.make_message_cb to raise inside the setup region, and asserts (a) the exception still propagates, (b) is_running is False and current_prompt_text is cleared, (c) a follow-up prompt() actually runs instead of queueing, with queued_prompts == [].

Verification on 92f53b665:

• bash scripts/run_tests.sh tests/acp/test_server.py -q → 30 tests passed, 0 failed
• bash scripts/run_tests.sh tests/acp/ -q → 14 files, 126 tests passed, 0 failed
• python3 scripts/check-windows-footguns.py acp_adapter/server.py tests/acp/test_server.py → no footguns
• Red-before-green confirmed on current main: with the server.py change stashed and only the test applied, pytest tests/acp/test_server.py -k setup_exception fails with AssertionError: setup exception left is_running=True — session bricked.

[israellot]

CI failure on this PR is unrelated to the change — flagging it rather than working around it, since the flake is in main and worth a proper fix by whoever owns #74487.

The failure

Python tests / Run tests slice 8/8 — one test:

tests/hermes_cli/test_update_eol_churn.py::test_churn_across_more_files_than_fit_in_one_argv
>       assert len(_dirty(repo)) == len(files)
E       AssertionError: assert 77 == 1200
tests/hermes_cli/test_update_eol_churn.py:188: AssertionError

This PR touches only acp_adapter/server.py and tests/acp/test_server.py (git diff --name-only upstream/main..HEAD) — nothing under hermes_cli/, and nothing that test imports. The file arrived on main ~an hour before this run in #74487 (5e807390f, 2026-07-30T04:53Z); its first green run on main at 14db1a99e (job 90852740902) shows tests/hermes_cli/test_update_eol_churn.py (9✓, 2.4s), and the same file in the same slice at the same commit failed on this PR's run (job 90853900700). Same code, same slice, different outcome — it's flaky, not caused by this branch.

Why it flakes (mechanism, not a guess)

The failing line is the fixture precondition, before _normalize_managed_eol is even called. _managed_repo builds the broken state, then asserts all 1200 files read dirty under -c core.autocrlf=false. Whether they do depends on git's racy-index heuristic:

• _managed_repo commits LF content, sets autocrlf=true, deletes the files and runs git checkout -- ., so the worktree gets CRLF and the index records the CRLF stat (git ls-files --debug shows size: 11 for an on-disk b'VALUE = 0\r\n', also 11 bytes — the stat cache says "clean").
• The only reason the CRLF churn is visible to the probe at all is that the checkout wrote those files in the same mtime second as the index write, so git considers each entry racy and re-reads content instead of trusting the cache.
• The moment the index write lands in a later second than some of the files, git trusts the stat cache for those and reports them clean. Locally I can flip the assertion between 1200 and 0 purely by advancing the index mtime — no content change:

dirty right after fixture:                          1200
dirty with index mtime +5s (no racy re-check):         0
dirty after `autocrlf=true git update-index --refresh`: 0

A 1200-file checkout on a loaded 8-worker runner takes long enough to straddle a second boundary, so a subset of files falls outside the racy window — hence 77, and hence the 1200-file test being the one that fails first. The suite's own repro line is order-independent (python -m pytest tests/hermes_cli/test_update_eol_churn.py), so nothing else in the slice is involved.

Suggested fix for the fixture: make the precondition independent of mtime granularity — either drop the == len(files) count assertion (it isn't what the test is about; the argv-length behaviour is), or force the racy state explicitly before asserting, e.g. backdate .git/index (os.utime) or git update-index --really-refresh under autocrlf=true so the cache state is deterministic rather than clock-dependent.

One thing worth a second look beyond the test

The same stat-cache behaviour reaches the product code, so this may be more than a flaky assertion. _normalize_managed_eol computes eol_only = _dirty() - _dirty("--ignore-cr-at-eol"), and on a settled checkout — one where any later git command has refreshed the index, which is the normal state of a real long-lived Windows install — the probe reports the CRLF tree as clean, eol_only is empty, the restore is skipped, and core.autocrlf false is written anyway:

A) FRESH checkout (racy window open)     before: crlf=3/3 autocrlf=true  probe_dirty=3
                                          after: crlf=3/3 autocrlf=false probe_dirty=3   <- repaired
B) SETTLED checkout (index refreshed)    before: crlf=3/3 autocrlf=true  probe_dirty=0
                                          after: crlf=3/3 autocrlf=false probe_dirty=0   <- restore skipped, pin written

Case B is exactly the outcome the docstring says the coupling exists to prevent ("pinning alone would expose every text file as modified and hand the update an autostash of the whole tree"). If that's right, the repair wants a forced refresh (git update-index --really-refresh, or comparing against git diff-index output that ignores the stat cache) before deciding eol_only is empty. Flagging for #74487's author rather than changing it here — out of scope for this PR, and I can't reproduce the Windows side.

(Environment note: the CI runner is git 2.54.0 on runner image 20260720.247.2; the numbers above are from git 2.43.0 locally, where 5 of the 9 tests in this file fail for the same stat-cache reason. The mechanism is version-independent; how often it bites is not.)

Happy to open a separate issue or PR for the fixture fix if that's useful — just say which you'd prefer. Nothing on this branch needs to change for it, so this PR is ready as-is apart from the red check.

[israellot]

Rebuilt on current main at 2c36f0fa476f4ef46698e54b413bda207c3cf296. Both review findings and the red CI slice are addressed.

1. "GitHub reports this branch as conflicting; resolving the reindent mechanically risks losing newer main behavior"

I did not resolve the conflict. I reset the branch to current main and re-applied the semantic change, because the previous diff was ~570 lines of re-indentation over a stale base, and that is exactly where a newer upstream line gets dropped silently.

The re-indent was applied programmatically on asserted block boundaries (first line of the guarded region and the final return PromptResponse(...)), not by hand. The receipt:

$ git diff -w --stat upstream/main..HEAD -- acp_adapter/server.py
 acp_adapter/server.py | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

18 lines added, 0 removed, with whitespace ignored. That is the whole functional change: the outer try at acp_adapter/server.py:1923 and the except BaseException: reset at acp_adapter/server.py:2231.

The specific newer behavior you flagged survives, one indent level deeper inside the guard:

$ grep -n 'set_session_vars(' acp_adapter/server.py
2040:                    session_tokens = set_session_vars(

mergeable is now MERGEABLE per gh pr view 71028 --json mergeable.

2. "Move the regression test into the current TestPrompt section; the historical test context was pruned on main"

Done. The test now lives at tests/acp/test_server.py:450, inside class TestPrompt (tests/acp/test_server.py:404), immediately after test_prompt_binds_session_id_into_subprocess_env. It uses only the agent and mock_manager fixtures that exist on current main; I re-added no pruned fixtures.

3. The red Python tests / Run tests slice 8/8

Not ours, and already fixed on main. The failing test was tests/hermes_cli/test_update_eol_churn.py::test_churn_across_more_files_than_fit_in_one_argv (1 failed, 8 passed) — a file this PR does not touch and does not import:

$ git diff --name-only upstream/main..HEAD
acp_adapter/server.py
tests/acp/test_server.py

It was a git stat-cache flake: the fixture's dirtiness precondition depended on worktree-vs-index mtime. main fixed it in fe497d8722 ("test(update): make EOL-churn dirtiness deterministic under racy-git stat caching", 2026-08-02) by bumping every worktree mtime past the index write — dated after the run that failed here. Rebuilding on current main picks that up, and the file passes on this branch:

$ bash scripts/run_tests.sh tests/hermes_cli/test_update_eol_churn.py -q
=== Summary: 1 files, 9 tests passed, 0 failed (100% complete) in 1.4s

Red-before-green

With only the source file stashed and the new test left in place, the test fails on the exact assertion it is meant to pin:

$ git stash push -- acp_adapter/server.py
$ bash scripts/run_tests.sh tests/acp/test_server.py -q
FAILED tests/acp/test_server.py::TestPrompt::test_prompt_setup_exception_still_releases_session
1 failed, 30 passed in 5.67s

E       AssertionError: setup exception left is_running=True — session bricked
E       assert True is False
tests/acp/test_server.py:480: AssertionError

The test drives the real prompt() path with make_message_cb (a setup-region callback factory, called after the is_running = True flip at acp_adapter/server.py:1908) raising, then asserts a follow-up prompt on the same session actually runs instead of queueing into a session no turn will drain.

Verification

$ bash scripts/run_tests.sh tests/acp/test_server.py -q
=== Summary: 1 files, 31 tests passed, 0 failed in 6.2s

$ bash scripts/run_tests.sh tests/acp/ -q
=== Summary: 14 files, 136 tests passed, 0 failed in 9.5s

$ python -m ruff check acp_adapter/server.py tests/acp/test_server.py
All checks passed!

$ python scripts/check-windows-footguns.py acp_adapter/server.py tests/acp/test_server.py
✓ No Windows footguns found (2 file(s) scanned).