feat(context): Add background task support for Context (SEP-1686)#2905
Conversation
marvin-context-protocol
Bot
added
enhancement
Test Failure AnalysisSummary: The static analysis workflow failed because the code doesn't pass Ruff linting and formatting checks. Root Cause: The newly added code in
Suggested Solution: Run
Detailed AnalysisPrek OutputSpecific Changes# src/fastmcp/server/dependencies.py
- __slots__ = ("_task_id", "_session_id")
+ __slots__ = ("_session_id", "_task_id")
# Import ordering fixes (two locations):
- import anyio
-
- import mcp.shared.exceptions
+ import anyio
+ import mcp.shared.exceptions
import mcp.shared.message
import mcp.types
+Related Files
This analysis was automatically generated by the FastMCP test failure triage bot |
WalkthroughAdds a TaskContext subsystem for background tasks: introduces TaskContextInfo dataclass, TaskContext class with elicit() and sample() methods, a CurrentTaskContext dependency, get_task_context/get_task_session/register_task_session APIs, and an in-process weakref session registry. Registers task sessions during background task submission and adds send_input_required_notification to emit SEP-1686 input-required notifications. Documentation files updated with TaskContext usage guidance for background tasks. No existing public signatures were removed. Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/fastmcp/server/dependencies.py (2)
124-157: Session registry lacks cleanup mechanism for stale entries.The
_task_sessionsdictionary stores weak references to sessions, but entries are only cleaned up whenget_task_sessionis called for that specificsession_id. Sessions that are never retrieved again will leave staleNone-returning weakrefs in the dict indefinitely.Consider periodic cleanup or using
WeakValueDictionary(though that requires the session to be the value, not a weakref wrapper).♻️ Suggested improvement
+def _cleanup_stale_sessions() -> None: + """Remove entries for garbage-collected sessions.""" + stale_keys = [k for k, ref in _task_sessions.items() if ref() is None] + for k in stale_keys: + _task_sessions.pop(k, None) + + def register_task_session(session_id: str, session: ServerSession) -> None: """Register a session for TaskContext access in background tasks. ... """ + # Opportunistic cleanup of stale entries + _cleanup_stale_sessions() _task_sessions[session_id] = weakref.ref(session)
927-927: Minor:__slots__could be sorted alphabetically.Static analysis suggests sorting slots for consistency.
♻️ Proposed fix
- __slots__ = ("_task_id", "_session_id") + __slots__ = ("_session_id", "_task_id")src/fastmcp/server/tasks/subscriptions.py (1)
215-263: Consider extracting shared notification-building logic.The new
send_input_required_notificationfunction duplicates Redis lookup and notification building logic from_send_status_notificationand_send_progress_notification. While this is acceptable for now, consider extracting the common pattern into a helper if more notification types are added.The implementation is otherwise correct and follows the established patterns.
docs/servers/tasks.mdx (3)
306-312: Use second person + active voice for the 4-step sequence (and considerSteps).
Line 306 reads passive; consider “When you callelicit()orsample(),TaskContextautomatically…” and render the four steps with the Steps component for sequential guidance.As per coding guidelines, include second person and active voice for instructions and use Steps for sequential instructions.
277-383: Add error handling and expected outputs to the TaskContext examples.
The examples are clear but don’t show error/edge handling (e.g., declined input, missing session, timeout) or expected output for verification. Consider adding a small try/except or explicit failure branch plus a short “Expected result” block below each example so users can confirm behavior.As per coding guidelines, MDX examples should be runnable, show error handling, and include expected outputs.
389-396: Add a short troubleshooting/next-steps block for distributed-worker failures.
This section explains the limitation but doesn’t tell users what error they’ll see or what to do next. Consider a brief “Troubleshooting” note (expected error message + fix) and a “Next steps” link to distributed-worker guidance.As per coding guidelines, include troubleshooting and end sections with next steps/related information.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e4fae4fb7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # Register session for TaskContext access in background workers (SEP-1686) | ||
| # This enables elicitation/sampling from background tasks via weakref | ||
| from fastmcp.server.dependencies import register_task_session | ||
|
|
||
| register_task_session(session_id, ctx.session) |
There was a problem hiding this comment.
When a task is created outside an MCP request (the code already falls back to session_id = "internal" for this case), ctx.session raises because no request context exists. The new unconditional call to register_task_session(session_id, ctx.session) therefore raises and prevents programmatic/background task submission that previously worked. This only happens for internal/programmatic calls (no session), but those now hard-fail before the task is queued; consider skipping registration or handling the missing session in that case.
Useful? React with 👍 / 👎.
gfortaine
force-pushed
the
feature/input-required-task-status
branch
from
070f226 to
6e4fae4
Compare
Test Failure AnalysisSummary: Static analysis (prek) failed because the code wasn't formatted according to the project's linting rules. Root Cause: The PR code has minor formatting violations that ruff automatically fixed:
Suggested Solution: Apply the auto-formatting changes and commit them. Run locally: uv run prek run --all-files
git add -u
git commit -m "Apply ruff formatting"Or simply commit the changes that the CI already made (the diff is shown in the workflow logs). Detailed AnalysisThe prek pre-commit hooks found and auto-fixed 4 style violations:
These are purely cosmetic changes enforced by ruff's formatting rules. Workflow Log Excerpt: Related Files
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/fastmcp/server/dependencies.py (3)
124-157: Minor: Stale weakref entries accumulate until accessed.The
_task_sessionsdict cleans up dead weakrefs only whenget_task_session()is called for that specific session. If sessions disconnect without their tasks completing (e.g., client crashes), the stale entries remain indefinitely. This is acceptable for typical usage patterns since background tasks will access their sessions, but consider adding periodic cleanup for long-running servers with high session churn.
1104-1112: Return type annotation is overly broad.The
sample()method returnsAny, but per the docstring it returnsCreateMessageResult. Similarly,elicit()returns a union of elicitation result types. Consider using more specific return type annotations for better IDE support and type safety.♻️ Suggested type annotations
+from typing import Union +# In TYPE_CHECKING block: +from fastmcp.server.elicitation import ( + AcceptedElicitation, + CancelledElicitation, + DeclinedElicitation, +) async def elicit( self, message: str, response_type: type | None = None, -) -> Any: +) -> AcceptedElicitation[Any] | DeclinedElicitation | CancelledElicitation:For
sample(), consider returningmcp.types.CreateMessageResultexplicitly.
1161-1173: Message conversion handles basic cases; consider validating role.The dict-to-
SamplingMessageconversion accepts anyrolevalue without validation. Per MCP spec, role should typically be"user"or"assistant". Invalid roles may cause downstream errors. Consider validating or documenting accepted values.
| async def elicit( | ||
| self, | ||
| message: str, | ||
| response_type: type | None = None, | ||
| ) -> Any: | ||
| """Request user input, updating task status to input_required. | ||
|
|
||
| This method implements the SEP-1686 ``input_required`` flow: | ||
|
|
||
| 1. Updates task status to ``input_required`` | ||
| 2. Sends elicitation request with ``related-task`` metadata | ||
| 3. Waits for user response | ||
| 4. Updates task status back to ``working`` | ||
| 5. Returns the parsed result | ||
|
|
||
| Args: | ||
| message: The message to display to the user | ||
| response_type: The expected response type. Can be: | ||
|
|
||
| - A dataclass or Pydantic model for structured input | ||
| - A primitive type (str, int, bool, float) for simple input | ||
| - None for freeform text input | ||
|
|
||
| Returns: | ||
| - ``AcceptedElicitation[T]`` if user accepted (data contains response) | ||
| - ``DeclinedElicitation`` if user declined | ||
| - ``CancelledElicitation`` if user cancelled | ||
|
|
||
| Raises: | ||
| RuntimeError: If session is no longer available | ||
| McpError: If client doesn't support elicitation | ||
|
|
||
| Example: | ||
| .. code-block:: python | ||
|
|
||
| @dataclass | ||
| class UserInfo: | ||
| name: str | ||
| age: int | ||
|
|
||
| @mcp.tool(task=True) | ||
| async def get_info(task_ctx: TaskContext = CurrentTaskContext()): | ||
| result = await task_ctx.elicit( | ||
| message="Please provide your information", | ||
| response_type=UserInfo, | ||
| ) | ||
| if result.action == "accept": | ||
| return f"{result.data.name} is {result.data.age} years old" | ||
| return "No info provided" | ||
| """ | ||
| import anyio | ||
| import mcp.shared.exceptions | ||
| import mcp.shared.message | ||
| import mcp.types | ||
|
|
||
| from fastmcp.server.elicitation import ( | ||
| CancelledElicitation, | ||
| DeclinedElicitation, | ||
| handle_elicit_accept, | ||
| parse_elicit_response_type, | ||
| ) | ||
| from fastmcp.server.tasks.subscriptions import send_input_required_notification | ||
|
|
||
| session = self._get_session() | ||
| docket = self._get_docket() | ||
| config = parse_elicit_response_type(response_type) | ||
|
|
||
| try: | ||
| # Update status to input_required | ||
| await send_input_required_notification( | ||
| session=session, | ||
| task_id=self._task_id, | ||
| session_id=self._session_id, | ||
| docket=docket, | ||
| status="input_required", | ||
| ) | ||
|
|
||
| # Build the elicitation request with related-task metadata | ||
| request = session._build_elicit_form_request( # pyright: ignore[reportPrivateUsage] | ||
| message=message, | ||
| requestedSchema=config.schema, | ||
| related_task_id=self._task_id, | ||
| ) | ||
|
|
||
| # Send the request and wait for response | ||
| response_stream, response_stream_reader = anyio.create_memory_object_stream[ | ||
| mcp.types.JSONRPCResponse | mcp.types.JSONRPCError | ||
| ](1) | ||
| request_id = request.id | ||
| session._response_streams[request_id] = response_stream # pyright: ignore[reportPrivateUsage] | ||
|
|
||
| try: | ||
| await session._write_stream.send( # pyright: ignore[reportPrivateUsage] | ||
| mcp.shared.message.SessionMessage( | ||
| message=mcp.types.JSONRPCMessage(request) | ||
| ) | ||
| ) | ||
|
|
||
| response_or_error = await response_stream_reader.receive() | ||
|
|
||
| if isinstance(response_or_error, mcp.types.JSONRPCError): | ||
| raise mcp.shared.exceptions.McpError(response_or_error.error) | ||
| else: | ||
| result = mcp.types.ElicitResult.model_validate( | ||
| response_or_error.result | ||
| ) | ||
| finally: | ||
| session._response_streams.pop(request_id, None) # pyright: ignore[reportPrivateUsage] | ||
| await response_stream.aclose() | ||
| await response_stream_reader.aclose() | ||
|
|
||
| # Parse and return result | ||
| if result.action == "accept": | ||
| return handle_elicit_accept(config, result.content) | ||
| elif result.action == "decline": | ||
| return DeclinedElicitation() | ||
| elif result.action == "cancel": | ||
| return CancelledElicitation() | ||
| else: | ||
| raise ValueError(f"Unexpected elicitation action: {result.action}") | ||
|
|
||
| finally: | ||
| # Always restore status to working | ||
| await send_input_required_notification( | ||
| session=session, | ||
| task_id=self._task_id, | ||
| session_id=self._session_id, | ||
| docket=docket, | ||
| status="working", | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
mcp python sdk ServerSession public API elicitation
💡 Result:
Relevant public API for ServerSession elicitation (MCP Python SDK):
- ServerSession.elicit(...) — send a generic elicitation request to the client. [1]
- ServerSession.elicit_form(...) — send a form-mode elicitation (structured schema). [1]
- ServerSession.elicit_url(...) — send a URL-mode elicitation (redirects user to external flow). [1]
- ServerSession.send_elicit_complete(...) — notify the client that an elicitation has completed. [1]
Related server-side helpers / errors:
- UrlElicitationRequiredError — raise this from a tool to signal a URL elicitation is required (SDK converts this into the appropriate MCP error/elicitation flow). [1][2]
- elicitations module / ElicitRequestURLParams and ElicitationResult types — types used for building elicitation requests and interpreting results. [1]
Examples:
- Elicitation examples (form and URL modes) and a pattern showing raising UrlElicitationRequiredError are in the SDK examples/documentation. [2]
Sources: API reference and SDK examples/documentation. [1][2]
Use the public ServerSession.elicit_form() API instead of private implementation details.
The MCP SDK provides a public ServerSession.elicit_form() API designed for exactly this purpose. The current implementation bypasses these public APIs in favor of private _build_elicit_form_request, _response_streams, and _write_stream methods, creating unnecessary fragility. Refactor to use the public API, which handles request/response patterns and cleanup internally.
🧰 Tools
🪛 Ruff (0.14.13)
1092-1092: Avoid specifying long messages outside the exception class
(TRY003)
Test Failure AnalysisSummary: Static analysis failed due to 8 unused Root Cause: The test file Suggested Solution: Remove all 8 unused File to modify: Remove
Detailed AnalysisThe These comments were likely added when the mocks were first created, but the type checker now properly infers that The fix is straightforward: simply remove the You can apply the fix automatically by running: uv run prek run --all-filesThis will fix the issues and any other formatting problems automatically. Related Files
Updated: 2026-02-01 18:15 UTC - Refreshed analysis for latest workflow run |
chrisguidry
left a comment
chrisguidry
left a comment
There was a problem hiding this comment.
@gfortaine Thank you for this! I like your approach and I think I can see a path to adding the distributed version later. I love how you worked with the dependency system and you covered giving good errors for the distributed case.
I do have some changes to request, though:
-
Could we remove the v3-notes/distributed-task-context-design.md from this PR? I think there's a simpler possible implementation for the distributed case that we could discuss in a Github issue instead. I think we'd rather not have a file with a speculative design in the repo
-
What do you think about making
Contextitself task-aware rather than introducing a separateTaskContext? The idea would be that users always declarectx: Context = CurrentContext()as their dependency, and it just works regardless of whether the tool is running in foreground or as a background task.- For foreground calls, it behaves as it does today. For background tasks,
elicit()andsample()would route through the session registry (embedded) or eventually via Redis (distributed). Properties that don't make sense in a task context (like request_id) could raise RuntimeError with a clear message if accessed. - This would be simpler for users - one context type, one dependency - and aligns well with SEP-1686 where the same tool may be called either way. Thoughts?
- For foreground calls, it behaves as it does today. For background tasks,
gfortaine
force-pushed
the
feature/input-required-task-status
branch
from
c6dc7e0 to
d81ba79
Compare
gfortaine
changed the title
|
Thanks for the detailed feedback @chrisguidry. Implemented the unified Context approach you suggested - refactored to enhance the existing Key changes:
This follows the v3.0 pattern where a single interface adapts to its execution context (similar to how Ready for another review when you have a moment. |
gfortaine
force-pushed
the
feature/input-required-task-status
branch
from
d81ba79 to
a62ecec
Compare
chrisguidry
left a comment
chrisguidry
left a comment
There was a problem hiding this comment.
This is looking really good! Could we get at least one test showing the end-to-end of it working, with a background task that's eliciting input? This will help with what the client-side sees when this happens.
|
@chrisguidry sounds like you wanted clarity on the client-server dance during elicitation. the new test in client receives:
client responds:
tool gets back:
the test explicitly asserts on the notification structure so you can see exactly what the client sees: meta = notification._meta
related_task = meta["modelcontextprotocol.io/related-task"]
assert related_task["taskId"] == "test-task-456"
assert related_task["status"] == "input_required"
assert related_task["statusMessage"] == "What is your name?"
assert "requestedSchema" in related_task["elicitation"]if you want me to add assertions on the redis key structure or anything else lmk. |
Test Failure AnalysisSummary: The static analysis job () failed due to formatting issues and unused comments in the test file. Root Cause: The type checker and formatter detected several fixable issues in :
Suggested Solution: Run
Then commit the changes: uv run prek run --all-files
git add tests/server/tasks/test_context_background_task.py
git commit -m "Fix formatting and remove unused type ignores"Detailed AnalysisThe Unused type ignores (8 instances): # Lines 246, 283, 286, 328, 331
fastmcp=mock_fastmcp, # type: ignore[arg-type] # ← Remove this comment
session=mock_session, # type: ignore[arg-type] # ← Remove this commentMissing newline at EOF: # Line 657 (last line)
assert result.data == "Alice" # The value from content["value"]
\ No newline at end of file # ← Need to add newlineLong assertion that needs reformatting (line 522): # Before:
assert captured_is_background is True, "Context.is_background_task should be True"
# After (prettier format):
assert captured_is_background is True, (
"Context.is_background_task should be True"
)Ternary expression simplification (lines 559-561): # Before:
redis_storage[key] = (
value.encode() if isinstance(value, str) else value
)
# After:
redis_storage[key] = value.encode() if isinstance(value, str) else valueMissing blank line (after line 337): Related Files
|
gfortaine
force-pushed
the
feature/input-required-task-status
branch
5 times, most recently
from
cffffc9 to
32928b9
Compare
Test Failure AnalysisSummary: A single test () in is timing out on Windows Python 3.10, but passing on all other platforms (Ubuntu Python 3.10, 3.13, Windows lowest deps, integration tests). Root Cause: This is not related to the Context changes in this PR. The failing test doesn't use Context at all - it simply instantiates objects with different values and verifies endpoint configuration. The timeout appears to be a Windows-specific performance issue or transient CI slowness, not a code defect. Evidence:
Suggested Solution: Re-run the failed Windows job. This looks like a transient timeout issue rather than a code problem. If it continues to fail:
The PR changes are safe - they only modify Detailed AnalysisTest Log Excerpttest_special_tenant_values (lines 105-129)def test_special_tenant_values(self):
"""Test that special tenant values are accepted."""
# Test with "organizations"
provider1 = AzureProvider( # ← Times out here on Windows
client_id="test_client",
client_secret="test_secret",
tenant_id="organizations",
base_url="https://myserver.com",
required_scopes=["read"],
jwt_signing_key="test-secret",
)
parsed = urlparse(provider1._upstream_authorization_endpoint)
assert "/organizations/" in parsed.path
# ... rest of testWhy This Isn't Related to the PRThe PR changes signature: # Before
def __init__(self, fastmcp: FastMCP, session: ServerSession | None = None):
# After
def __init__(self, fastmcp: FastMCP, session: ServerSession | None = None, *, task_id: str | None = None):But (line 107 in azure.py) calls which goes to , not . There's no connection between OAuth providers and the Context class. CI Job Results
Related Files
|
Test Failure AnalysisSummary: A single test (test_special_tenant_values) in tests/server/auth/providers/test_azure.py is timing out on Windows Python 3.10, but passing on all other platforms. Root Cause: This is NOT related to the Context changes in this PR. The failing test does not use Context at all - it simply instantiates AzureProvider objects with different tenant_id values and verifies endpoint configuration. The timeout appears to be a Windows-specific performance issue or transient CI slowness, not a code defect. Evidence:
Suggested Solution: Re-run the failed Windows job. This looks like a transient timeout issue rather than a code problem. If it continues to fail:
The PR changes are safe - they only modify Context.init() to add an optional task_id parameter (backward compatible), and update elicit() behavior. These changes do not affect OAuth provider initialization at all. Details: The test times out when creating an AzureProvider instance, which goes through OAuthProxy.init(), not Context.init(). There is no code path connecting OAuth providers to the Context class. All other CI jobs passed, including Ubuntu 3.10, Ubuntu 3.13, lowest dependencies, and integration tests. |
chrisguidry
left a comment
chrisguidry
left a comment
There was a problem hiding this comment.
Thanks @gfortaine, looking good! I think I'd have gone a little less mock-y with these because we do have the memory:// redis broker available during tests. Glad to see the extra end-to-end tests here, thank you!
|
🙏 thanks chris! good point on the memory:// broker – added a test using the real docket backend. ready for merge whenever you are. |
Implements unified Context approach for background tasks running in Docket workers. Based on code review feedback from @chrisguidry to use existing Context class rather than introducing a separate TaskContext. Key changes: - Add task_id parameter to Context.__init__() - Add is_background_task and task_id properties - Wire Context creation in _CurrentContext for Docket workers - Add session registry for background task access - Add elicitation module for Redis-based coordination - Remove outdated warning about Context unavailability in workers The elicitation flow: 1. Tool calls ctx.elicit() in background task 2. Request stored in Redis, sends input_required notification 3. Client responds via handle_task_input() 4. Tool receives response and resumes Addresses PrefectHQ#2568, fixes PrefectHQ#2877
gfortaine
force-pushed
the
feature/input-required-task-status
branch
from
5c0a52a to
5d7ea90
Compare
- 21 tests covering Context.is_background_task, session property, elicit() in background task mode, and documentation - E2E tests for elicit_for_task() and handle_task_input() with mocked Redis - Full flow tests: task blocks on elicit(), client sends input, task resumes
gfortaine
force-pushed
the
feature/input-required-task-status
branch
from
5d7ea90 to
16656ee
Compare
Test Failure AnalysisSummary: The static analysis job failed due to type checking warnings in the new background task test file. The type checker found 11 warnings: 8 unused comments and 3 warnings. Root Cause:
Suggested Solution:
Detailed AnalysisType Checker OutputThe Unused Type Ignore CommentsThe type checker reports that 8 Possibly Missing AttributeThe more critical issue is the assert result.action == "accept"
assert isinstance(result.data, UserInfo) # Warning: .data may not exist
assert result.data.name == "Alice" # Warning: .data may not exist
assert result.data.age == 30 # Warning: .data may not existThe type checker correctly identifies that checking Related Files
|
gfortaine
force-pushed
the
feature/input-required-task-status
branch
from
16656ee to
521a9de
Compare
jlowin
left a comment
jlowin
left a comment
There was a problem hiding this comment.
@gfortaine this looks awesome! At one point @chrisguidry and I thought this wasn't possible, thanks for proving us wrong :)
I ran through one of my FastMCP reviewbots and have a few relatively small comments for you. Please feel free to consider, some (like this first one) we could defer or @chrisguidry may take up as some future work.
On elicitation.py — the polling loop (~line 574)
The polling approach works for now, but it's worth noting this will make up to 7,200 Redis round-trips over the 1-hour TTL (every 0.5s), each going through async with docket.redis(). For a future iteration, Redis BLPOP or pub/sub would be a much better fit here — it's the exact use case they're designed for. Not necessarily a blocker but something to keep in mind for production workloads.
On elicitation.py — contextlib.suppress(Exception) (~line 566)
This notification is the only way the client learns the task needs input. If it fails silently, the task will poll Redis indefinitely waiting for a response that never comes. Could we at least log the exception here? Something like logger.warning("Failed to send input_required notification for task %s: %s", task_id, e) would make debugging much easier.
On elicitation.py — session._fastmcp_state_prefix access (~line 511)
Context.session_id already handles this extraction (with the same fallback logic). Since _elicit_for_task is called from Context.elicit(), could we just pass self.session_id through as a parameter instead of re-deriving it from the session's private attribute here? Or load from the (Python contextvar) context instead?
On elicitation.py — import contextlib inside function body (~line 564)
Nit: this import should be at module level per project conventions.
On context.py — is_background_task docstring example (~line 32)
The example shows an if/else where both branches do the same thing (ctx.elicit("Need input", str)) — which actually makes the point that it's transparent, but reads oddly as example code. Maybe just show the simple case without the branch? The transparency is the whole point.
On test_context_background_task.py — general
The mock-heavy tests verify the mechanics, but the test_context_elicit_with_real_docket_memory_backend test at the end is doing the most work in terms of actual confidence. If there's appetite, leaning more on the real memory:// backend (as Chris mentioned) would strengthen the suite — mocks can pass even when the real integration has issues.
On context.py — session property fallback chain (~line 74)
The new fallback to self._session (line 83) is more permissive than before — previously this only returned from request_context.session. Just want to make sure the "fallback to stored session" path is intentional and won't mask cases where the session wasn't properly set up through the normal path.
Test Failure AnalysisSummary: Static analysis (prek/ruff) failed due to 4 unused Root Cause: The test file contains Suggested Solution: Remove the four unused type ignore comments:
Detailed AnalysisThe prek static analysis tool (combining Ruff + Prettier + ty) reports: And three similar warnings at lines 283, 286, and 328. Each suggests removing the suppression comment and shows the exact diff needed. The type checker no longer reports type errors for these mock objects being passed to the functions, so the suppressions are unnecessary and should be removed. Related Files
|
Per reviewer feedback (Chris Guidry), adds test demonstrating elicitation flow with Docket's real memory:// backend instead of mocking Redis. This complements the existing mock-based tests.
gfortaine
force-pushed
the
feature/input-required-task-status
branch
from
521a9de to
0847145
Compare
Test Failure AnalysisSummary: Static type checking () is failing with 12 type warnings in the new test file. Root Cause: Two categories of type issues in
Suggested Solution: Fix 1: Remove Unused Type IgnoresRemove the
Fix 2: Add Type Narrowing for Attribute AccessFor lines 432-434 and 816, add type narrowing before accessing # Before (line 432-434)
assert result.action == "accept"
assert isinstance(result.data, UserInfo)
assert result.data.name == "Alice"
# After
assert result.action == "accept"
assert isinstance(result, AcceptedElicitation) # Type narrowing
assert isinstance(result.data, UserInfo)
assert result.data.name == "Alice"# Before (line 816)
if result.action == "accept":
return f"Hello, {result.data}!"
# After
if isinstance(result, AcceptedElicitation): # Type narrowing
return f"Hello, {result.data}!"Detailed AnalysisThe type checker correctly identifies that the union type Relevant log excerpts: Related Files
|
- Log notification failures instead of silently suppressing (elicitation.py) - Simplify is_background_task docstring example (context.py) - Remove unused type: ignore comments (test file) - Add proper type narrowing with isinstance(AcceptedElicitation) (test file)
|
Thanks @gfortaine! |
3 participants
Summary
This PR implements background task support for the
Contextclass following the unified interface pattern suggested by @chrisguidry in code review.Key Changes:
task_idparameter toContext.__init__()is_background_taskandtask_idpropertieselicit()to work transparently in background task modetasks/elicitation.pymodule for Redis-based coordinationHow It Works
When running as a background task:
input_requiredin Redisnotifications/tasks/updatedis sent with elicitation metadatatasks/sendInputArchitecture
Approach Change
Originally proposed
TaskContextas a separate dependency class. Based on review feedback, refactored to enhance the existingContextclass instead, following the v3.0 principle of unified interfaces.Tests
Related
input_requiredstatus)input_requiredtask status for elicitation and sampling during background tasks [SEP-1686] #2568, fixes Passing context to Background Task #2877Breaking Changes
None. The new
task_idparameter is optional and defaults toNone.