Repository navigation
fix(agent): aggregator retrieval runs in the request's workspace - #409
Merged
Merged
Conversation
With AGGREGATOR_AGENT_ENABLED=true every local_docs job raised ScopeUnavailableError and came back empty. Two causes, both needed: - The chat route bound the request scope only inside the SSE generator, after retrieval had already run. It now binds it around retrieval too. - AggregatorAgent submitted jobs to a ThreadPoolExecutor, which does not carry contextvars. Each job now runs in a copy of the caller's context. Failed closed (no other workspace reachable); flag off by default. Each half is pinned by its own test and fails with its fix reverted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resolves against P2-1b-ii (#406): retrieval now takes scope=, so the route binds request_scope(scope) around the call and the test spies record the scope kwarg. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The repaired aggregator path still ignores source, document, and additional-workspace retrieval constraints.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Propagates authorized workspace scope through chat retrieval and aggregator worker threads.
Changes:
- Binds scope during route-level retrieval.
- Copies context into each aggregator worker.
- Adds regression tests and documentation.
| File | Description |
|---|---|
src/routes_fastapi/api_routes.py |
Binds request scope around retrieval. |
src/agent/aggregator.py |
Copies context into worker threads. |
tests/unit/test_aggregator_carries_request_scope.py |
Tests scope propagation. |
CHANGELOG.md |
Documents the fix. |
.claude/rules/file-map.md |
Registers the new test. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+352
to
+354
| plan=plan, scope=scope, | ||
| additional_workspace_ids=fields.get("additional_workspace_ids") or None, | ||
| source_ids=fields.get("active_source_ids") or None, |
Comment on lines
+93
to
+95
| `ThreadPoolExecutor`, which does not carry contextvars. Every `local_docs` job raised | ||
| `ScopeUnavailableError` in its worker and came back empty, marked partial. It failed closed — | ||
| no other workspace's data was reachable — and the flag is off by default. The route now |
Comment on lines
+5
to
+7
| plain `pool.submit`, so with `AGGREGATOR_AGENT_ENABLED=true` every local-docs job raised | ||
| `ScopeUnavailableError` in its thread: fail-closed, nothing leaked, but retrieval came back | ||
| empty and marked partial on every request. |
…t-scope # Conflicts: # CHANGELOG.md
|
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
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.
With
AGGREGATOR_AGENT_ENABLED=true, chat retrieved nothing from the workspace's documents. Everylocal_docsjob raisedScopeUnavailableErrorin its worker and came back empty, markedpartial. It failed closed, so no other workspace's data was reachable, and the flag is off by default. I noted this in #405 and it's fixed here.Two causes, and both were needed
api_chatbound the request scope only inside_generate_sse, afterretrieve_contextshad run. It now also binds it around retrieval, using the sameworkspace_id or ALL_WORKSPACESthe stream already uses.run_in_threadpoolcopies the context into its thread.AggregatorAgentused a plainpool.submit, and contextvars don't follow it. Each job now runs incontextvars.copy_context().run.Tests: each half is pinned separately
tests/unit/test_aggregator_carries_request_scope.py:POST /api/chatwith the aggregator on retrieves in the caller's workspace.Reproduced before the fix: retrieval was never reached (
seen == []). With only the route fix reverted, the end-to-end test fails. With only the thread fix reverted, all three fail.Verification
ruff,mypy,bandit: clean. Fast suite: 0 failed.Merge note
This touches the same
retrieve_contextscall inapi_routes.pyas #406 (workspace_id=→scope=). Whichever lands second needs a one-line conflict resolution, and I'll do it when I rebase #406.🤖 Generated with Claude Code