Skip to content

fix(btw): honour every stop signal a work run can carry - #189

Merged
YUZHEthefool merged 1 commit into
fix/btw-stop-third-party-work-157from
fix/btw-stop-honour-all-signals-157
Sep 14, 2026
Merged

fix(btw): honour every stop signal a work run can carry#189
YUZHEthefool merged 1 commit into
fix/btw-stop-third-party-work-157from
fix/btw-stop-honour-all-signals-157

Conversation

@YUZHEthefool

Copy link
Copy Markdown
Member

Summary

#157 made an in-flight third-party work run honour a stop, but it read only the
event's own stop flag. /task stop sets that flag for a third-party runner and
the agent stop request for a local one, so both paths were left half-covered:

Separately, #170 records a local err response as a failure, but run_agent
skips the rest of a response once a stop is pending and the marker was written
after that gate. An error arriving together with a stop request was never
recorded, so a failed run read as cancelled.

Related issues

Fixes #157
Fixes #170

Root cause

Three signals all mean "this run was stopped", and no single one covers every
path:

  • event.is_stopped() — set by stop_all, the third-party path;
  • agent_stop_requested — set by request_agent_stop_all, the local path;
  • agent_user_aborted — set by run_agent when it reports an abort, which also
    clears agent_stop_requested.

WorkLoop._execute collapsed these into a local expression for the terminal
status but still tested only is_stopped() for the queued guard and
_run_detached. #173 made the third-party consumer poll event.is_stopped, so
the agent-stop path never reached it. And in run_agent, the err marker sat
behind if _should_stop_agent(astr_event): continue.

Reproduction

  1. /work <task> on a local (non-streaming) Agent, then /task stop
    mid-run → /work status reported completed.
  2. Same with a third-party runner whose stub keeps producing → the runner was
    drained to the last response, and the session ended failed.
  3. A runner emitting err while the stop request is already pending → the run
    ended cancelled with no failure marker.

Implementation notes

  • The three-signal predicate moves to btw/types.py as stop_requested, beside
    the existing work markers, and gets one definition. The work loop's terminal
    status, its queued and pre-delivery stop checks, and both third-party handlers
    read the same function, so the two stop paths cannot be handled by only one of
    them again.
  • run_agent records the err marker before the stop gate. The gate still
    controls delivery; only the bookkeeping moved.
  • Behavior change: a local work run stopped mid-flight is cancelled instead of
    completed; a third-party run stopped via the agent request stops consuming
    immediately and is cancelled instead of failed. No user-visible text
    changes, so the /task stop and /work status docs already describe this.

Validation

.venv/Scripts/python.exe -m pytest tests/unit -q
  5236 passed, 6 skipped

.venv/Scripts/python.exe -m pytest --test-profile blocking -q
  5357 passed, 6 skipped, 1 deselected

.venv/Scripts/python.exe -m ruff format --check <touched files>   # clean
.venv/Scripts/python.exe -m ruff check <touched files>            # clean
.venv/Scripts/python.exe -m pyright --project pyrightconfig.quality.json <touched source files>
  0 errors

Each of the three new regression tests was run against the unmodified tree
first, where it fails for the reported reason: completed instead of
cancelled, a fully drained runner, and a missing failure marker. make check
and make quality were not run locally on this Windows checkout.

Compatibility and risk

  • No route, schema, configuration, or dependency change. BTW stays off by default.
  • The stop predicate is now shared by a module that already imported
    btw.types; no new import cycle.
  • Residual risk: the stop is still observed between responses, so a runner
    blocked inside one long network call is not interrupted until it returns.
    Neither the local nor the remote task's cancellation is claimed beyond the
    local wait.

Checklist

  • The change is focused and does not include unrelated refactoring.
  • I added or updated a regression test, or explained why a test is not practical.
  • I ran the relevant formatting, lint, build, and test commands.
  • User-visible behavior updates both docs/zh/ and docs/en/ when needed. (No user-visible text changed.)
  • OpenAPI, generated client, docs/public/openapi.json, and tests change together when routes or schemas change. (No route or schema change.)
  • No secrets committed. Runtime Python deps update pyproject.toml, requirements.txt, and uv.lock together. (No dependency change.)
  • I did not restore legacy shims, Python <3.14 fallbacks, or upstream publish/docs URLs as fork artifacts.
  • Breaking API or behavior changes use ! and a BREAKING CHANGE: footer. (None; the status change is the fix.)
  • I will not merge this PR myself. Merge needs a human maintainer review plus a separate AI-assisted review (AI_POLICY.md).
  • AI use follows AI_POLICY.md. Keep exactly one author note below. Do not fabricate the other.

Agent note

Goal: close the stop paths #157 and #170 left uncovered, keeping BTW off by
default and preserving authorization and request identity.

Paths touched: astrbot/core/agent/btw/types.py,
astrbot/core/agent/btw/work_loop.py,
astrbot/core/astr_agent_run_util.py,
astrbot/core/pipeline/process_stage/method/agent_sub_stages/third_party.py,
and tests/unit/test_btw_delivery.py + tests/unit/test_btw_work_loop.py.

Checks run: the commands under Validation, plus each new test against the
unmodified tree. An audit of the #168#173 stack produced the findings; a
candidate around aclosing failing to propagate an athrown exception to
_execute was rejected after confirming _run_detached already records the
failure and redacts provider details.

Residual risk: stated under Compatibility and risk.

Tools used: Claude Code (Opus 5) with the repository's AGENTS.md and AI_POLICY.md.

This PR is stacked on #173 (fix/btw-stop-third-party-work-157); merge that first.

#157 stopped a third-party work run that `/task stop` had stopped, but it read
only the event's own stop flag.  `/task stop` sets that flag for a third-party
runner and the agent stop request for a local one, so the local path was still
uncovered: a local non-streaming run consumed every response, kept `produced`
true, and ended as completed.  A third-party run stopped through the agent
request was uncovered too -- it kept draining its runner and ended as failed.

#170 recorded a local `err` response as a failure, but `run_agent` skips the
rest of a response once a stop is pending, and the marker was written after
that gate.  An error that arrived with a stop request was never recorded, so a
failed run read as cancelled.

Move the three-signal predicate next to the other work markers and give it one
definition.  The work loop's terminal status, its queued and pre-delivery stop
checks, and both third-party handlers now read the same function, so the two
stop paths cannot be handled by only one of them again.  Record the `err`
marker before the stop gate.

Each new regression test fails on the unmodified tree: completed instead of
cancelled, a fully drained runner, and a missing failure marker.

Fixes #157
Fixes #170
AI-Generated: true
@YUZHEthefool
YUZHEthefool added this pull request to stack #174 September 14, 2026 14:24
@YUZHEthefool
YUZHEthefool merged commit 393fd93 into master Sep 14, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] BTW 停止第三方后台工作未中断本地执行,结束后仍标记完成

1 participant