[PRE-301] implement agent termination in python sdk - #16
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds PrefactorTerminatedError and an asyncio TerminationMonitor (fast control-signal + periodic polling), extends HTTP models/endpoints for control signals and termination fields, integrates monitor into PrefactorCoreClient with a background sync loop, propagates termination through LangChain middleware, and provides examples and tests. ChangesAgent Instance Termination Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Siutan
marked this pull request as ready for review
May 6, 2026 23:45
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/http/README.md (1)
268-271:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the
AgentStatusdocs snippet to include"terminated".The Types section still shows the old literal set. Since
AgentStatusnow includes"terminated", this snippet is stale and may confuse users.Suggested diff
-# AgentStatus = Literal["pending", "active", "complete", "failed", "cancelled"] +# AgentStatus = Literal["pending", "active", "complete", "failed", "cancelled", "terminated"] # FinishStatus = Literal["complete", "failed", "cancelled"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/http/README.md` around lines 268 - 271, Update the docs snippet showing the type literals so AgentStatus includes "terminated": edit the commented example lines that define AgentStatus to include "terminated" (e.g., change AgentStatus = Literal["pending", "active", "complete", "failed", "cancelled"] to AgentStatus = Literal["pending", "active", "complete", "failed", "cancelled", "terminated"]) and keep FinishStatus as-is; ensure the snippet near the AgentStatus/FinishStatus import reflects the current type definition.packages/core/examples/agent_e2e.py (1)
188-214:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTreat empty
PREFACTOR_AGENT_IDas unset.On Line 188 / Line 200,
is Nonewon’t catchPREFACTOR_AGENT_ID="", which will route to the account-scoped path and send an emptyagent_id.Proposed fix
- agent_id = os.environ.get("PREFACTOR_AGENT_ID") + agent_id = os.environ.get("PREFACTOR_AGENT_ID") or None ... - if agent_id is None: + if agent_id is None: instance = await client.create_agent_instance( agent_version={ "name": "Example Agent", "external_identifier": "v8.0.0", } )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/examples/agent_e2e.py` around lines 188 - 214, The code currently checks only if agent_id is None before deciding which create_agent_instance path to call, which treats an empty PREFACTOR_AGENT_ID="" as set; update the condition around the agent_id variable (used before calling PrefactorCoreClient and the two create_agent_instance calls) to treat empty or whitespace-only strings as unset (e.g., use a truthiness/strip check such as "if not agent_id or agent_id.strip() == ''" or equivalent) so the branch that creates an instance without agent_id is taken when the env var is empty.packages/langchain/src/prefactor_langchain/middleware.py (2)
259-286:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument
environment_idinfrom_config()docstring.
environment_idwas added to the signature at Line 265 but is missing fromArgs:in the public docstring.Suggested fix
Args: api_url: The Prefactor API URL. api_token: The API token for authentication. agent_id: Optional agent identifier for categorization. agent_name: Optional human-readable agent name. + environment_id: Optional environment identifier used when creating + the agent instance. schema_registry: Optional SchemaRegistry for registering span schemas.As per coding guidelines, "All public functions and classes need docstrings in Google style".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/langchain/src/prefactor_langchain/middleware.py` around lines 259 - 286, The docstring for PrefactorMiddleware.from_config is missing documentation for the environment_id parameter; update the Google-style Args section of the from_config docstring to include a one-line description for environment_id (e.g., "environment_id: Optional environment identifier for scoping the agent.") so all public parameters are documented alongside api_url, api_token, agent_id, agent_name, schema_registry, include_langchain_schemas, and tool_schemas.
37-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t swallow
PrefactorTerminatedErrorin agent hooks.
_throw_if_terminated()is called at Line 805 and Line 913, but both methods catchExceptionand returnNone. That suppresses termination inbefore_agent/abefore_agentand weakens fail-fast behavior.Suggested fix
def _raise_if_telemetry_failure(error: BaseException) -> None: """Re-raise latched core telemetry failures through provider hooks.""" - if isinstance(error, PrefactorTelemetryFailureError): + if isinstance(error, (PrefactorTelemetryFailureError, PrefactorTerminatedError)): raise errorAlso applies to: 805-831, 913-955
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/langchain/src/prefactor_langchain/middleware.py` around lines 37 - 40, The current agent hooks swallow PrefactorTerminatedError because the try/except blocks around calls to _throw_if_terminated (invoked from before_agent and abefore_agent) catch bare Exception and return None; change those exception handlers so they do not suppress termination—either remove the broad except or, inside the except Exception as exc block, re-raise if isinstance(exc, PrefactorTerminatedError) (or let BaseException pass through), otherwise handle/log the original exception as before; ensure _throw_if_terminated and its callers (before_agent, abefore_agent) will propagate PrefactorTerminatedError to preserve fail-fast behavior.
🧹 Nitpick comments (8)
packages/core/src/prefactor_core/monitoring/__init__.py (1)
1-3: ⚡ Quick winInclude future annotations import in this new module.
This new Python module should add
from __future__ import annotationsat the top.Suggested diff
+from __future__ import annotations + from prefactor_core.monitoring.termination_monitor import TerminationMonitorAs per coding guidelines,
**/*.py: Use Python 3.11+ withfrom __future__ import annotationsat the top of modules.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/prefactor_core/monitoring/__init__.py` around lines 1 - 3, Add the future annotations import to the module by inserting "from __future__ import annotations" at the very top of this file (before any other imports) so the module that imports TerminationMonitor and defines __all__ uses PEP-compliant postponed evaluation of annotations; ensure the new import appears above the existing "from prefactor_core.monitoring.termination_monitor import TerminationMonitor" line.packages/http/tests/test_models.py (1)
357-383: ⚡ Quick winAdd docstrings to the new public test class/methods.
Line 357 introduces a new public test class, and its public test methods are missing docstrings.
Suggested diff
class TestAgentInstanceTerminatedReason: + """Tests termination_reason behavior on AgentInstance.""" + def _make_instance(self, status="active", **kwargs): from datetime import datetime, timezone @@ def test_termination_reason_defaults_none(self): + """Defaults to None for non-terminated instances.""" instance = self._make_instance() assert instance.termination_reason is None def test_termination_reason_parsed(self): + """Parses termination_reason when status is terminated.""" instance = self._make_instance( status="terminated", termination_reason="admin action" ) assert instance.termination_reason == "admin action"As per coding guidelines,
**/*.py: All public functions and classes need docstrings in Google style.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/http/tests/test_models.py` around lines 357 - 383, Add Google-style docstrings for the new public test class TestAgentInstanceTerminatedReason and its public test methods (test_termination_reason_defaults_none and test_termination_reason_parsed); include a brief one-line class docstring describing the test purpose and for each test method a short docstring describing the scenario being tested (e.g., default termination_reason is None, parsing termination_reason when status is "terminated"), and optionally document the helper _make_instance to describe what it constructs and key parameters.packages/core/src/prefactor_core/exceptions.py (1)
61-71: ⚡ Quick winUse a Google-style docstring for the new public exception.
The new public class is documented, but not in Google style (missing structured
Argscontext forreason).Suggested diff
class PrefactorTerminatedError(PrefactorCoreError): - """Raised when the agent instance has been terminated by p2.""" + """Raised when an agent instance has been terminated. + + Args: + reason: Optional termination reason returned by the backend. + """As per coding guidelines,
**/*.py: All public functions and classes need docstrings in Google style.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/prefactor_core/exceptions.py` around lines 61 - 71, The PrefactorTerminatedError class has a docstring but it must follow Google-style for public classes; update the class docstring for PrefactorTerminatedError to include a short description and an Args section documenting the reason parameter (type and meaning), e.g., describe reason: str | None — optional termination reason, and ensure the docstring format matches other public exceptions (use triple-quoted string immediately under class definition and include the Args block); leave implementation of __init__ and self.reason unchanged.packages/http/src/prefactor_http/models/types.py (1)
1-3: ⚡ Quick winAdd
from __future__ import annotationsat module top.Line 3 imports typing symbols, but this module still misses the required future-annotations import.
Suggested diff
+from __future__ import annotations + from typing import LiteralAs per coding guidelines,
**/*.py: Use Python 3.11+ withfrom __future__ import annotationsat the top of modules.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/http/src/prefactor_http/models/types.py` around lines 1 - 3, This module is missing the required future annotations import; add the statement "from __future__ import annotations" at the very top of the file (above the module docstring and before any other imports) in packages/http/src/prefactor_http/models/types.py so that the typing usages (e.g., Literal imported on line with "from typing import Literal") use postponed evaluation of annotations per the project's Python 3.11+ guideline.packages/http/src/prefactor_http/models/agent_instance.py (1)
229-242: ⚡ Quick winUpdate
AgentInstancedocstring to include new public fields.
agent_deployment_idandtermination_reasonwere added on Line 250 and Line 256, but the class Attributes section doesn’t document them yet.Proposed fix
class AgentInstance(BaseModel): """Agent instance model. @@ agent_version_id: Agent version ID environment_id: Environment ID + agent_deployment_id: Agent deployment ID status: Instance status @@ started_at: When the instance started (null if not started) finished_at: When the instance finished (null if not finished) + termination_reason: Optional reason provided when instance is terminated span_counts: Span counts for this instance """As per coding guidelines, "All public functions and classes need docstrings in Google style".
Also applies to: 250-256
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/http/src/prefactor_http/models/agent_instance.py` around lines 229 - 242, Update the AgentInstance class docstring to document the two new public fields: add entries for agent_deployment_id and termination_reason to the Attributes section (matching the Google style used elsewhere), describing their types and brief meaning; locate the AgentInstance class and update its docstring so the Attributes list includes "agent_deployment_id: Agent deployment ID" and "termination_reason: Reason for termination (null if not terminated)" or equivalent concise descriptions.packages/core/examples/agent_e2e.py (1)
34-38: ⚡ Quick winAdd
from __future__ import annotationsat module top.This module is missing the required future import before standard imports (Line 34 onward).
Proposed fix
+from __future__ import annotations + import asyncio import osAs per coding guidelines, "
**/*.py: Use Python 3.11+ withfrom __future__ import annotationsat the top of modules".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/examples/agent_e2e.py` around lines 34 - 38, Add the future annotations import at the very top of the module before any other imports: insert "from __future__ import annotations" as the first line of the file so it precedes the existing imports (asyncio, os, and the Prefactor imports) to comply with the project guideline for Python 3.11+ modules; ensure no blank lines or other code comes before this import so functions/classes using forward references (e.g., any types in PrefactorCoreClient/PrefactorCoreConfig usage) benefit from postponed evaluation.packages/langchain/examples/termination_demo.py (1)
144-146: 💤 Low value
KeyboardInterruptmay not propagate reliably in async context.Catching
KeyboardInterruptinside an async loop may not behave as expected. When Ctrl+C is pressed, the signal is typically handled by the main thread and converted toasyncio.CancelledErrorfor running tasks. Consider using signal handlers or catchingasyncio.CancelledErrorinstead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/langchain/examples/termination_demo.py` around lines 144 - 146, The except KeyboardInterrupt block in termination_demo.py inside the async loop should also handle asyncio.CancelledError (or use a proper signal handler) because Ctrl+C in async contexts is often delivered as CancelledError; import asyncio and change the handler to catch both exceptions (e.g., except (KeyboardInterrupt, asyncio.CancelledError): logger.info("Stopped by user."); break) or alternatively install a signal handler in the main async entry (using loop.add_signal_handler) that cancels the running task so the async function (e.g., the demo loop) can catch asyncio.CancelledError and shut down cleanly.packages/core/src/prefactor_core/monitoring/termination_monitor.py (1)
112-116: ⚡ Quick winClear subscribers on
destroy()to avoid retaining callback references.
destroy()stops polling but keepsself._callbackspopulated. Clearing them makes teardown final and avoids unnecessary retained references.Suggested fix
def destroy(self) -> None: """Permanently shut down the monitor (no further events will fire).""" self._destroyed = True self._stop_poll() + self._callbacks.clear()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/prefactor_core/monitoring/termination_monitor.py` around lines 112 - 116, The destroy() method currently sets _destroyed and calls _stop_poll() but leaves subscriber callbacks in self._callbacks, retaining references; update destroy() to also clear self._callbacks (e.g., reset to an empty list/dict or call clear() on it) after calling _stop_poll() so all registered callbacks are released and teardown is final; locate the destroy method in termination_monitor.py and modify the logic around _stop_poll() to ensure self._callbacks is cleared.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/superpowers/plans/2026-05-06-agent-termination.md`:
- Around line 16-27: Add blank lines before and after the markdown tables and
declare a fenced-code language for the demo block to satisfy markdownlint:
insert an empty line above and below the table block that lists files (the table
showing packages/core/... and packages/langchain/...) in
docs/superpowers/plans/2026-05-06-agent-termination.md, and change the fenced
code at the demo block (the block around "Run `#1` — Agent instance: <instance-id>
...") to include a language marker (e.g., ```text) so MD058 and MD040 are
resolved.
In `@docs/superpowers/specs/2026-05-06-agent-termination-design.md`:
- Line 27: The doc references a stale field name terminated_reason in the
AgentInstance spec; update all occurrences to the implemented field name
termination_reason to match the Pydantic model and API behavior (e.g., replace
terminated_reason with termination_reason in the GET
`/api/v1/agent_instance/{id}` response and the other mentions). Ensure the
AgentInstance examples, schema sections, and descriptive text (previously at
lines referencing terminated_reason) consistently use termination_reason so the
spec aligns with the code.
- Around line 37-63: The unlabeled fenced code block containing the architecture
flow (lines showing p2 span response →
AgentSpanClient.create/finish(control_signal_callback=...),
_check_control_signal, monitor.detect_termination, asyncio.Event.set(),
PrefactorCoreClient.sync, _throw_if_terminated, AgentInstanceHandle.finish and
client._termination_monitor.reset()) needs a Markdown language tag; update the
opening fence to include a tag such as ```text (or a more specific tag like
```flow) so the block is no longer unlabeled and MD040 is satisfied.
In `@packages/core/src/prefactor_core/monitoring/termination_monitor.py`:
- Around line 65-67: detect_termination currently iterates self._callbacks and
calls each subscriber directly so an exception in one callback can propagate;
change the loop in detect_termination to call each cb inside a try/except
Exception block that catches any exception, logs the error (including the
exception details and which callback failed) and continues to the next callback
so one bad subscriber cannot break signaling; reference the existing
self._callbacks list and the detect_termination() function when making the
change.
In `@packages/core/tests/monitoring/test_termination_monitor.py`:
- Around line 32-63: The async tests in TestPrimaryPath (e.g.,
test_detect_termination_sets_event, test_reason_propagates,
test_null_reason_accepted, test_second_detect_termination_is_idempotent,
test_detect_termination_noop_after_destroy) are missing pytest-asyncio
decorators; add a pytest.mark.asyncio decorator (either on each async test
method or on the TestPrimaryPath class) and ensure pytest is imported at the top
of the file, and apply the same decorator approach to the other async test
classes (TestFallbackPoll, TestReset, TestCallbackLifecycle) so pytest runs the
coroutines correctly (or alternatively rely on pytest-asyncio auto mode per
project config).
In `@packages/http/src/prefactor_http/endpoints/agent_span.py`:
- Around line 27-29: The control payload may be non-dict and calling .get on it
raises AttributeError; update the handling in the agent_span response logic so
you first verify control is a dict (e.g., isinstance(control, dict)) before
calling control.get; only check control.get("terminate") and invoke
callback(control.get("reason")) when control is a dict (and optionally
coerce/validate the reason), adjusting the code around the existing control =
response.get("control") usage to guard the .get() calls.
In `@packages/http/tests/test_endpoints.py`:
- Around line 448-449: The async test method
TestAgentInstanceGet.test_get_returns_agent_instance is missing the pytest event
loop decorator; add `@pytest.mark.asyncio` directly above the async def to ensure
pytest runs the coroutine, and if pytest is not already imported in the test
module add an import pytest at the top; keep the decorator placement immediately
above the method in the TestAgentInstanceGet class.
- Around line 480-481: The async tests in class TestAgentSpanControlSignal are
missing the pytest asyncio decorator so they won't run; add `@pytest.mark.asyncio`
above each async def in that class (e.g.,
test_create_calls_callback_when_control_signal_present and the other three async
test methods) and ensure pytest is imported in the test file if not already
present.
In `@packages/langchain/tests/test_middleware.py`:
- Line 903: Remove the orphaned module-level assertion by deleting the stray
line `assert "properties" in LANGCHAIN_TOOL_SCHEMA` that sits after the
TestMiddlewareThrowIfTerminated class; this duplicate belongs inside the
existing test function `test_langchain_tool_schema` (already asserts this at
line ~822), so eliminating the standalone assertion prevents it running at
import time and keeps validation within the proper test.
- Around line 864-902: Add the missing `@pytest.mark.asyncio` decorators (and
ensure pytest is imported) to the async test functions so they run properly as
coroutines: apply `@pytest.mark.asyncio` above
test_throw_if_terminated_raises_when_event_set,
test_throw_if_terminated_noop_when_not_terminated,
test_throw_if_terminated_noop_when_getter_is_none,
test_awrap_model_call_raises_when_terminated, and
test_awrap_tool_call_raises_when_terminated; no logic changes required in
PrefactorMiddleware or its helpers, just annotate these async test methods so
awaiting inside (e.g., await middleware.awrap_model_call / await
middleware.awrap_tool_call) executes correctly.
---
Outside diff comments:
In `@packages/core/examples/agent_e2e.py`:
- Around line 188-214: The code currently checks only if agent_id is None before
deciding which create_agent_instance path to call, which treats an empty
PREFACTOR_AGENT_ID="" as set; update the condition around the agent_id variable
(used before calling PrefactorCoreClient and the two create_agent_instance
calls) to treat empty or whitespace-only strings as unset (e.g., use a
truthiness/strip check such as "if not agent_id or agent_id.strip() == ''" or
equivalent) so the branch that creates an instance without agent_id is taken
when the env var is empty.
In `@packages/http/README.md`:
- Around line 268-271: Update the docs snippet showing the type literals so
AgentStatus includes "terminated": edit the commented example lines that define
AgentStatus to include "terminated" (e.g., change AgentStatus =
Literal["pending", "active", "complete", "failed", "cancelled"] to AgentStatus =
Literal["pending", "active", "complete", "failed", "cancelled", "terminated"])
and keep FinishStatus as-is; ensure the snippet near the
AgentStatus/FinishStatus import reflects the current type definition.
In `@packages/langchain/src/prefactor_langchain/middleware.py`:
- Around line 259-286: The docstring for PrefactorMiddleware.from_config is
missing documentation for the environment_id parameter; update the Google-style
Args section of the from_config docstring to include a one-line description for
environment_id (e.g., "environment_id: Optional environment identifier for
scoping the agent.") so all public parameters are documented alongside api_url,
api_token, agent_id, agent_name, schema_registry, include_langchain_schemas, and
tool_schemas.
- Around line 37-40: The current agent hooks swallow PrefactorTerminatedError
because the try/except blocks around calls to _throw_if_terminated (invoked from
before_agent and abefore_agent) catch bare Exception and return None; change
those exception handlers so they do not suppress termination—either remove the
broad except or, inside the except Exception as exc block, re-raise if
isinstance(exc, PrefactorTerminatedError) (or let BaseException pass through),
otherwise handle/log the original exception as before; ensure
_throw_if_terminated and its callers (before_agent, abefore_agent) will
propagate PrefactorTerminatedError to preserve fail-fast behavior.
---
Nitpick comments:
In `@packages/core/examples/agent_e2e.py`:
- Around line 34-38: Add the future annotations import at the very top of the
module before any other imports: insert "from __future__ import annotations" as
the first line of the file so it precedes the existing imports (asyncio, os, and
the Prefactor imports) to comply with the project guideline for Python 3.11+
modules; ensure no blank lines or other code comes before this import so
functions/classes using forward references (e.g., any types in
PrefactorCoreClient/PrefactorCoreConfig usage) benefit from postponed
evaluation.
In `@packages/core/src/prefactor_core/exceptions.py`:
- Around line 61-71: The PrefactorTerminatedError class has a docstring but it
must follow Google-style for public classes; update the class docstring for
PrefactorTerminatedError to include a short description and an Args section
documenting the reason parameter (type and meaning), e.g., describe reason: str
| None — optional termination reason, and ensure the docstring format matches
other public exceptions (use triple-quoted string immediately under class
definition and include the Args block); leave implementation of __init__ and
self.reason unchanged.
In `@packages/core/src/prefactor_core/monitoring/__init__.py`:
- Around line 1-3: Add the future annotations import to the module by inserting
"from __future__ import annotations" at the very top of this file (before any
other imports) so the module that imports TerminationMonitor and defines __all__
uses PEP-compliant postponed evaluation of annotations; ensure the new import
appears above the existing "from prefactor_core.monitoring.termination_monitor
import TerminationMonitor" line.
In `@packages/core/src/prefactor_core/monitoring/termination_monitor.py`:
- Around line 112-116: The destroy() method currently sets _destroyed and calls
_stop_poll() but leaves subscriber callbacks in self._callbacks, retaining
references; update destroy() to also clear self._callbacks (e.g., reset to an
empty list/dict or call clear() on it) after calling _stop_poll() so all
registered callbacks are released and teardown is final; locate the destroy
method in termination_monitor.py and modify the logic around _stop_poll() to
ensure self._callbacks is cleared.
In `@packages/http/src/prefactor_http/models/agent_instance.py`:
- Around line 229-242: Update the AgentInstance class docstring to document the
two new public fields: add entries for agent_deployment_id and
termination_reason to the Attributes section (matching the Google style used
elsewhere), describing their types and brief meaning; locate the AgentInstance
class and update its docstring so the Attributes list includes
"agent_deployment_id: Agent deployment ID" and "termination_reason: Reason for
termination (null if not terminated)" or equivalent concise descriptions.
In `@packages/http/src/prefactor_http/models/types.py`:
- Around line 1-3: This module is missing the required future annotations
import; add the statement "from __future__ import annotations" at the very top
of the file (above the module docstring and before any other imports) in
packages/http/src/prefactor_http/models/types.py so that the typing usages
(e.g., Literal imported on line with "from typing import Literal") use postponed
evaluation of annotations per the project's Python 3.11+ guideline.
In `@packages/http/tests/test_models.py`:
- Around line 357-383: Add Google-style docstrings for the new public test class
TestAgentInstanceTerminatedReason and its public test methods
(test_termination_reason_defaults_none and test_termination_reason_parsed);
include a brief one-line class docstring describing the test purpose and for
each test method a short docstring describing the scenario being tested (e.g.,
default termination_reason is None, parsing termination_reason when status is
"terminated"), and optionally document the helper _make_instance to describe
what it constructs and key parameters.
In `@packages/langchain/examples/termination_demo.py`:
- Around line 144-146: The except KeyboardInterrupt block in termination_demo.py
inside the async loop should also handle asyncio.CancelledError (or use a proper
signal handler) because Ctrl+C in async contexts is often delivered as
CancelledError; import asyncio and change the handler to catch both exceptions
(e.g., except (KeyboardInterrupt, asyncio.CancelledError): logger.info("Stopped
by user."); break) or alternatively install a signal handler in the main async
entry (using loop.add_signal_handler) that cancels the running task so the async
function (e.g., the demo loop) can catch asyncio.CancelledError and shut down
cleanly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 901ac5a4-2635-4e1d-aee6-75a274db3250
📒 Files selected for processing (29)
.gitignoredocs/superpowers/plans/2026-05-06-agent-termination.mddocs/superpowers/specs/2026-05-06-agent-termination-design.mdpackages/core/README.mdpackages/core/examples/agent_e2e.pypackages/core/src/prefactor_core/__init__.pypackages/core/src/prefactor_core/client.pypackages/core/src/prefactor_core/exceptions.pypackages/core/src/prefactor_core/managers/agent_instance.pypackages/core/src/prefactor_core/monitoring/__init__.pypackages/core/src/prefactor_core/monitoring/termination_monitor.pypackages/core/tests/monitoring/__init__.pypackages/core/tests/monitoring/test_termination_monitor.pypackages/core/tests/test_agent_instance_finish_status.pypackages/core/tests/test_agent_instance_register.pypackages/http/README.mdpackages/http/src/prefactor_http/endpoints/agent_instance.pypackages/http/src/prefactor_http/endpoints/agent_span.pypackages/http/src/prefactor_http/models/agent_instance.pypackages/http/src/prefactor_http/models/types.pypackages/http/tests/test_endpoints.pypackages/http/tests/test_models.pypackages/langchain/README.mdpackages/langchain/examples/termination_demo.pypackages/langchain/src/prefactor_langchain/middleware.pypackages/langchain/tests/test_middleware.pypackages/livekit/README.mdpackages/livekit/src/prefactor_livekit/session.pypackages/livekit/tests/test_session.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/http/src/prefactor_http/endpoints/agent_span.py (1)
86-86: 💤 Low valueMissing docstring entry for
control_signal_callbackparameter.The
control_signal_callbackparameter is added to bothcreate()andfinish()but not documented in the Args section of their docstrings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/http/src/prefactor_http/endpoints/agent_span.py` at line 86, Add a docstring entry for the control_signal_callback parameter in both create() and finish() of agent_span.py: document that control_signal_callback: Callable[[str | None], None] | None is an optional callback invoked with a control signal string (or None) and returns None; place the description under the Args section alongside the other parameters and mirror the same wording for both methods to keep docs consistent with the signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/http/src/prefactor_http/endpoints/agent_span.py`:
- Line 86: Add a docstring entry for the control_signal_callback parameter in
both create() and finish() of agent_span.py: document that
control_signal_callback: Callable[[str | None], None] | None is an optional
callback invoked with a control signal string (or None) and returns None; place
the description under the Args section alongside the other parameters and mirror
the same wording for both methods to keep docs consistent with the signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad71e645-512c-43e5-8810-a98ce3925614
📒 Files selected for processing (16)
docs/superpowers/plans/2026-05-06-agent-termination.mddocs/superpowers/specs/2026-05-06-agent-termination-design.mdpackages/core/examples/agent_e2e.pypackages/core/src/prefactor_core/exceptions.pypackages/core/src/prefactor_core/monitoring/__init__.pypackages/core/src/prefactor_core/monitoring/termination_monitor.pypackages/core/tests/monitoring/test_termination_monitor.pypackages/http/README.mdpackages/http/src/prefactor_http/endpoints/agent_span.pypackages/http/src/prefactor_http/models/agent_instance.pypackages/http/src/prefactor_http/models/types.pypackages/http/tests/test_endpoints.pypackages/http/tests/test_models.pypackages/langchain/examples/termination_demo.pypackages/langchain/src/prefactor_langchain/middleware.pypackages/langchain/tests/test_middleware.py
✅ Files skipped from review due to trivial changes (4)
- packages/core/src/prefactor_core/monitoring/init.py
- packages/http/src/prefactor_http/models/types.py
- packages/core/src/prefactor_core/exceptions.py
- docs/superpowers/plans/2026-05-06-agent-termination.md
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/core/examples/agent_e2e.py
- packages/http/README.md
- packages/http/src/prefactor_http/models/agent_instance.py
- packages/langchain/tests/test_middleware.py
- packages/langchain/examples/termination_demo.py
- packages/core/src/prefactor_core/monitoring/termination_monitor.py
joshgillies
reviewed
May 14, 2026
| @@ -0,0 +1,1722 @@ | |||
| # Agent Termination Implementation Plan | |||
Collaborator
There was a problem hiding this comment.
Do these need to be commited?
joshgillies
approved these changes
May 14, 2026
…ed, and get() endpoint
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…fore enqueueing Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ation_reason - sync() now skips restart if instance_id unchanged (prevents poll reset every 1s) - Rename terminated_reason → termination_reason to match p2 API response field Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Switch from deprecated create_react_agent to langchain.agents.create_agent which properly wires AgentMiddleware (abefore_agent/aafter_agent hooks) - Add PREFACTOR_BA_TOKEN env var for terminate API (requires BA token) - Fix terminate request body to include required reason field - Log full status+body from terminate response Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add optional environment_id param to PrefactorMiddleware.from_config() so new agents (no existing deployment) can register successfully - Thread environment_id through _ensure_initialized → create_agent_instance - Initialize _environment_id = None in all constructor paths - Demo reads PREFACTOR_ENVIRONMENT_ID env var Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Deleted the agent termination implementation plan and design documents as they are no longer relevant to the current project structure.
Siutan
force-pushed
the
pre-301-implement-agent-termination-in-python-sdk
branch
from
May 18, 2026 02:04
4171c68 to
cef643e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/core/tests/test_agent_instance_finish_status.py (1)
107-109: ⚡ Quick winAsync test methods have inconsistent decorator usage but will execute correctly.
These tests are missing
@pytest.mark.asyncio, but sinceasyncio_mode = "auto"is configured inpyproject.toml, they will run correctly without the decorator. However, consider adding the decorator for consistency with other async tests in the file (e.g., lines 70, 88).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/tests/test_agent_instance_finish_status.py` around lines 107 - 109, The async test method test_409_on_finish_treated_as_success inside class TestFinishAgentInstance409Handling lacks the `@pytest.mark.asyncio` decorator used elsewhere; add `@pytest.mark.asyncio` (either on the test method or the class TestFinishAgentInstance409Handling) to make its async decorator usage consistent with the other async tests (referencing pytest.mark.asyncio, TestFinishAgentInstance409Handling, and test_409_on_finish_treated_as_success).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/prefactor_core/client.py`:
- Around line 183-190: The background sync loop can die silently because
_run_sync_loop lacks an exception guard and close() skips awaiting a completed
task (so exceptions go unobserved); update _run_sync_loop to wrap the loop body
(or each iteration) in a try/except that catches asyncio.CancelledError
(re-raise or break) and catches Exception to log the error and continue,
specifically around the call to self._termination_monitor.sync(); also change
close() to always await self._sync_task if it exists (not only when not done)
and catch asyncio.CancelledError and Exception from awaiting it, logging
exceptions so they are observed and do not vanish (referencing _run_sync_loop,
close, self._termination_monitor.sync, and self._sync_task).
In `@packages/langchain/examples/termination_demo.py`:
- Around line 60-70: The terminate-call currently ignores non-2xx responses;
update the session.post block in termination_demo.py to detect failures by
calling resp.raise_for_status() (or explicitly checking resp.status) after
reading the response (body) and raise an exception when status is not successful
so the demo fails fast; ensure the surrounding code (where exceptions may be
suppressed) no longer swallows this error so that failures from the terminate
request (variables: url, ba_token, resp, body, logger) propagate and are visible
in logs.
- Around line 51-79: Add Google-style docstrings to the public async functions
terminate_after_delay, run_once, and main: for each function, add a
top-of-function docstring that briefly describes what the function does,
documents all parameters with types and meanings (e.g., api_url,
ba_token/api_token, instance_id/agent_id, delay/auto_terminate_delay), states
the return type (None) and notes any exceptions or side-effects (network calls,
task termination). Place the docstrings immediately under each async def and
follow Google style sections: Args, Returns, and Raises where appropriate so
they satisfy the repository’s public-docstring requirement.
---
Nitpick comments:
In `@packages/core/tests/test_agent_instance_finish_status.py`:
- Around line 107-109: The async test method
test_409_on_finish_treated_as_success inside class
TestFinishAgentInstance409Handling lacks the `@pytest.mark.asyncio` decorator used
elsewhere; add `@pytest.mark.asyncio` (either on the test method or the class
TestFinishAgentInstance409Handling) to make its async decorator usage consistent
with the other async tests (referencing pytest.mark.asyncio,
TestFinishAgentInstance409Handling, and test_409_on_finish_treated_as_success).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d671e35-6ed2-476e-90f3-03a882c87a8f
📒 Files selected for processing (20)
packages/core/examples/agent_e2e.pypackages/core/src/prefactor_core/__init__.pypackages/core/src/prefactor_core/client.pypackages/core/src/prefactor_core/exceptions.pypackages/core/src/prefactor_core/managers/agent_instance.pypackages/core/src/prefactor_core/monitoring/__init__.pypackages/core/src/prefactor_core/monitoring/termination_monitor.pypackages/core/tests/monitoring/__init__.pypackages/core/tests/monitoring/test_termination_monitor.pypackages/core/tests/test_agent_instance_finish_status.pypackages/http/README.mdpackages/http/src/prefactor_http/endpoints/agent_instance.pypackages/http/src/prefactor_http/endpoints/agent_span.pypackages/http/src/prefactor_http/models/agent_instance.pypackages/http/src/prefactor_http/models/types.pypackages/http/tests/test_endpoints.pypackages/http/tests/test_models.pypackages/langchain/examples/termination_demo.pypackages/langchain/src/prefactor_langchain/middleware.pypackages/langchain/tests/test_middleware.py
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/http/src/prefactor_http/models/types.py
- packages/core/src/prefactor_core/monitoring/init.py
- packages/core/src/prefactor_core/init.py
- packages/core/src/prefactor_core/monitoring/termination_monitor.py
- packages/http/src/prefactor_http/endpoints/agent_span.py
- packages/core/src/prefactor_core/exceptions.py
- packages/core/tests/monitoring/test_termination_monitor.py
- packages/http/src/prefactor_http/models/agent_instance.py
- packages/langchain/src/prefactor_langchain/middleware.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/tests/test_failure_handling.py (1)
122-125: ⚡ Quick winSimplify the fast_sleep implementation.
The
original_sleepvariable is unnecessary since the patch only affectsprefactor_core.client.asyncio.sleep, not the test module'sasyncio.sleep.♻️ Proposed simplification
- original_sleep = asyncio.sleep - async def fast_sleep(_delay): - await original_sleep(0) + await asyncio.sleep(0)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/tests/test_failure_handling.py` around lines 122 - 125, Remove the unused original_sleep variable and simplify fast_sleep to directly await asyncio.sleep(0); update the monkeypatch that sets prefactor_core.client.asyncio.sleep to use this simplified async def fast_sleep(_delay): await asyncio.sleep(0) so the patch still targets prefactor_core.client.asyncio.sleep but the test module's asyncio.sleep remains untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/tests/test_failure_handling.py`:
- Around line 122-125: Remove the unused original_sleep variable and simplify
fast_sleep to directly await asyncio.sleep(0); update the monkeypatch that sets
prefactor_core.client.asyncio.sleep to use this simplified async def
fast_sleep(_delay): await asyncio.sleep(0) so the patch still targets
prefactor_core.client.asyncio.sleep but the test module's asyncio.sleep remains
untouched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab8f004e-a0c2-4e15-8b06-ca2b9a73346a
📒 Files selected for processing (9)
packages/core/pyproject.tomlpackages/core/src/prefactor_core/_version.pypackages/core/src/prefactor_core/client.pypackages/core/tests/test_failure_handling.pypackages/http/src/prefactor_http/_version.pypackages/langchain/examples/termination_demo.pypackages/langchain/pyproject.tomlpackages/langchain/src/prefactor_langchain/_version.pypackages/langchain/tests/test_termination_demo.py
✅ Files skipped from review due to trivial changes (3)
- packages/core/src/prefactor_core/_version.py
- packages/langchain/src/prefactor_langchain/_version.py
- packages/http/src/prefactor_http/_version.py
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/langchain/examples/termination_demo.py
- packages/core/src/prefactor_core/client.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes Pre 301
Summary by CodeRabbit
New Features
Improvements
Tests / Docs