fix(backend): harden Copilot session tenancy - #13650
Conversation
…-2489) The persisted ChatSession org/team was trusted for every turn, but membership is only validated at session creation. A user removed or suspended from an org afterward kept acting under the stale org through existing sessions — spending credits and running agents under a tenancy they no longer belonged to. Re-verify ACTIVE membership at the turn-dispatch choke points: - New backend/copilot/session_tenancy.py::verify_session_org_membership runs up to two indexed lookups on the membership unique constraints (OrgMember@@unique(orgId,userId); TeamMember@@unique(teamId,userId)), mirroring get_request_context's checks for the header org. - HTTP /stream (stream_chat_post): no ACTIVE OrgMember for the session's org → 403 (honest failure, not silent degradation to personal). A stale team on a still-valid org is stripped to org-home. - Queue promotion (dispatch_next_for_user): re-check before promoting a turn that was queued while the user was a member; on revocation drop the session out of the queue (queued → idle) so it neither runs under the revoked org nor blocks the user's other queued turns. Personal/ctx-fallback sessions (organization_id None) are unaffected. Co-Authored-By: Claude Opus <noreply@anthropic.com>
github-project-automation
Bot
moved this to 🆕 Needs initial review
in AutoGPT development kanban
|
/batch |
WalkthroughChat routes, execution paths, scheduled turns, and queued turns now revalidate organization and team membership. Revoked organization access blocks dispatch. Stale team access falls back to the organization context. Tenancy metadata propagates through streaming, scheduling, recovery, and sub-session flows. ChangesSession tenancy revalidation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ChatClient
participant ChatRoutes
participant resolve_session_tenancy
participant TurnQueue
participant Scheduler
ChatClient->>ChatRoutes: submit stream or pending-message request
ChatRoutes->>resolve_session_tenancy: revalidate session tenancy
resolve_session_tenancy-->>ChatRoutes: resolved tenancy or access error
ChatRoutes-->>ChatClient: continue or return documented error
TurnQueue->>resolve_session_tenancy: revalidate queued session
resolve_session_tenancy-->>TurnQueue: resolved team or revoked organization
TurnQueue->>TurnQueue: reset revoked session or select next valid session
Scheduler->>resolve_session_tenancy: resolve scheduled session tenancy
resolve_session_tenancy-->>Scheduler: resolved organization and team
Scheduler->>TurnQueue: schedule turn with resolved tenancy
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![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 (1)
autogpt_platform/backend/backend/copilot/session_tenancy_test.py (1)
20-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exact Prisma lookup keys.
The mocks return fixed values regardless of their arguments, so a regression using the wrong
(orgId, userId)or(teamId, userId)pair—or skipping the active-team lookup—would still pass. Addassert_awaited_once_with(...)checks for the unique lookup payloads, especially in the active-team test.Proposed test strengthening
- _patch_prisma( + prisma = _patch_prisma( mocker, org_member=_member(OrgMemberStatus.ACTIVE), team_member=_member(OrgMemberStatus.ACTIVE), ) result = await verify_session_org_membership( user_id="u1", organization_id="org-1", team_id="team-1" ) assert result == "team-1" + prisma.orgmember.find_unique.assert_awaited_once_with( + where={"orgId_userId": {"orgId": "org-1", "userId": "u1"}} + ) + prisma.teammember.find_unique.assert_awaited_once_with( + where={"teamId_userId": {"teamId": "team-1", "userId": "u1"}} + )Also applies to: 32-114
🤖 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 `@autogpt_platform/backend/backend/copilot/session_tenancy_test.py` around lines 20 - 24, Strengthen the Prisma mocks in the session tenancy tests by asserting each orgmember.find_unique and teammember.find_unique AsyncMock was awaited exactly once with the expected (orgId, userId) or (teamId, userId) lookup payload. Apply these assertions across the affected tests, with particular attention to the active-team scenario to verify the active-team lookup is performed.
🤖 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 `@autogpt_platform/backend/backend/copilot/turn_queue.py`:
- Around line 307-325: In dispatch_next_for_user, after dropping the revoked
head session to idle and invalidating its cache, invoke
dispatch_next_for_user(user_id) again so the next queued session can be
promoted. Replace the immediate False return while preserving the existing
status update and cache invalidation.
---
Nitpick comments:
In `@autogpt_platform/backend/backend/copilot/session_tenancy_test.py`:
- Around line 20-24: Strengthen the Prisma mocks in the session tenancy tests by
asserting each orgmember.find_unique and teammember.find_unique AsyncMock was
awaited exactly once with the expected (orgId, userId) or (teamId, userId)
lookup payload. Apply these assertions across the affected tests, with
particular attention to the active-team scenario to verify the active-team
lookup is performed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3bdd838e-c0fc-4059-a348-ff59a8c921cd
📒 Files selected for processing (6)
autogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/api/features/chat/routes_test.pyautogpt_platform/backend/backend/copilot/session_tenancy.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/copilot/turn_queue_test.py
📜 Review details
⏰ Context from checks skipped due to timeout. (13)
- GitHub Check: check API types
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
- GitHub Check: type-check (3.12)
- GitHub Check: type-check (3.11)
- GitHub Check: type-check (3.13)
- GitHub Check: lint
- GitHub Check: types
- GitHub Check: lint
- GitHub Check: Analyze (typescript)
- GitHub Check: Check PR Status
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (5)
autogpt_platform/backend/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
autogpt_platform/backend/**/*.py: Use Python 3.11 (required; managed by Poetry via pyproject.toml) for backend development
Always run 'poetry run format' (Black + isort) before linting in backend development
Always run 'poetry run lint' (ruff) after formatting in backend development
autogpt_platform/backend/**/*.py: Usepoetry run ...command for executing Python package dependencies
Use top-level imports only — avoid local/inner imports except for lazy imports of heavy optional dependencies likeopenpyxl
Use absolute imports withfrom backend.module import ...for cross-package imports; single-dot relative imports are acceptable for sibling modules within the same package; avoid double-dot relative imports
Do not use duck typing — avoidhasattr/getattr/isinstancefor type dispatch; use typed interfaces/unions/protocols instead
Use Pydantic models over dataclass/namedtuple/dict for structured data
Do not use linter suppressors — no# type: ignore,# noqa,# pyright: ignore; fix the type/code instead
Prefer list comprehensions over manual loop-and-append patterns
Use early return with guard clauses first to avoid deep nesting
Use%sfor deferred interpolation indebuglog statements for efficiency; use f-strings elsewhere for readability (e.g.,logger.debug("Processing %s items", count)vslogger.info(f"Processing {count} items"))
Sanitize error paths by usingos.path.basename()in error messages to avoid leaking directory structure
Be aware of TOCTOU (Time-Of-Check-Time-Of-Use) issues — avoid check-then-act patterns for file access and credit charging
Usetransaction=Truefor Redis pipelines to ensure atomicity on multi-step operations
Usemax(0, value)guards for computed values that should never be negative
Keep files under ~300 lines; if a file grows beyond this, split by responsibility (extract helpers, models, or a sub-module into a new file)
Keep functions under ~40 lines; extract named helpers when a function grows longer
...
Files:
autogpt_platform/backend/backend/copilot/session_tenancy.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/api/features/chat/routes_test.py
autogpt_platform/{backend,autogpt_libs}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Format Python code with
poetry run format
Files:
autogpt_platform/backend/backend/copilot/session_tenancy.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/api/features/chat/routes_test.py
autogpt_platform/backend/backend/api/features/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Update routes in '/backend/backend/api/features/' and add/update Pydantic models in the same directory for API development
Files:
autogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/api/features/chat/routes_test.py
autogpt_platform/backend/**/api/**/*.py
📄 CodeRabbit inference engine (autogpt_platform/backend/AGENTS.md)
autogpt_platform/backend/**/api/**/*.py: UseSecurity()instead ofDepends()for authentication dependencies to get proper OpenAPI security specification
Follow SSE (Server-Sent Events) protocol: usedata:lines for frontend-parsed events (must match Zod schema) and: commentlines for heartbeats/status
Files:
autogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/api/features/chat/routes_test.py
autogpt_platform/backend/**/*_test.py
📄 CodeRabbit inference engine (autogpt_platform/backend/AGENTS.md)
autogpt_platform/backend/**/*_test.py: Use pytest with snapshot testing for API responses
Colocate test files with source files using*_test.pynaming convention
Mock at boundaries — mock where the symbol is used, not where it's defined; after refactoring, update mock targets to match new module paths
UseAsyncMockfromunittest.mockfor async functions in tests
When writing tests, use Test-Driven Development (TDD): write failing tests marked with@pytest.mark.xfailbefore implementation, then remove the marker once the implementation is complete
When creating snapshots in tests, usepoetry run pytest path/to/test.py --snapshot-update; always review snapshot changes withgit diffbefore committing
Files:
autogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/api/features/chat/routes_test.py
🧠 Learnings (14)
📚 Learning: 2026-02-26T17:02:22.448Z
Learnt from: Pwuts
Repo: Significant-Gravitas/AutoGPT PR: 12211
File: .pre-commit-config.yaml:160-179
Timestamp: 2026-02-26T17:02:22.448Z
Learning: Keep the pre-commit hook pattern broad for autogpt_platform/backend to ensure OpenAPI schema changes are captured. Do not narrow to backend/api/ alone, since the generated schema depends on Pydantic models across multiple directories (backend/data/, backend/blocks/, backend/copilot/, backend/integrations/, backend/util/). Narrowing could miss schema changes and cause frontend type desynchronization.
Applied to files:
autogpt_platform/backend/backend/copilot/session_tenancy.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/api/features/chat/routes_test.py
📚 Learning: 2026-03-04T08:04:35.881Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12273
File: autogpt_platform/backend/backend/copilot/tools/workspace_files.py:216-220
Timestamp: 2026-03-04T08:04:35.881Z
Learning: In the AutoGPT Copilot backend, ensure that SVG images are not treated as vision image types by excluding 'image/svg+xml' from INLINEABLE_MIME_TYPES and MULTIMODAL_TYPES in tool_adapter.py; the Claude API supports PNG, JPEG, GIF, and WebP for vision. SVGs (XML text) should be handled via the text path instead, not the vision path.
Applied to files:
autogpt_platform/backend/backend/copilot/session_tenancy.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.py
📚 Learning: 2026-04-01T04:17:41.600Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12632
File: autogpt_platform/backend/backend/copilot/tools/workspace_files.py:0-0
Timestamp: 2026-04-01T04:17:41.600Z
Learning: When reviewing AutoGPT Copilot tool implementations, accept that `readOnlyHint=True` (provided via `ToolAnnotations`) may be applied unconditionally to *all* tools—even tools that have side effects (e.g., `bash_exec`, `write_workspace_file`, or other write/save operations). Do **not** flag these tools for having `readOnlyHint=True`; this is intentional to enable fully-parallel dispatch by the Anthropic SDK/CLI and has been E2E validated. Only flag `readOnlyHint` issues if they conflict with the established `ToolAnnotations` behavior (e.g., missing/incorrect propagation relative to the intended annotation mechanism).
Applied to files:
autogpt_platform/backend/backend/copilot/session_tenancy.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.py
📚 Learning: 2026-06-06T12:22:37.648Z
Learnt from: anvyle
Repo: Significant-Gravitas/AutoGPT PR: 13302
File: autogpt_platform/backend/backend/copilot/tools/agent_search.py:579-583
Timestamp: 2026-06-06T12:22:37.648Z
Learning: When writing LLM-facing instruction strings that trigger tool calls in this AutoGPT codebase, use the exact registered tool name `view_agent_output` (as defined in `backend/copilot/tools/agent_output.py` via its `name` property and exported via `TOOL_REGISTRY`). Do not reference the bare name `agent_output`, since it is not a valid tool name and will cause tool invocation to fail.
Applied to files:
autogpt_platform/backend/backend/copilot/session_tenancy.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.py
📚 Learning: 2026-03-05T15:42:08.207Z
Learnt from: ntindle
Repo: Significant-Gravitas/AutoGPT PR: 12297
File: .claude/skills/backend-check/SKILL.md:14-16
Timestamp: 2026-03-05T15:42:08.207Z
Learning: In Python files under autogpt_platform/backend (recursively), rely on poetry run format to perform formatting (Black + isort) and linting (ruff). Do not run poetry run lint as a separate step after poetry run format, since format already includes linting checks.
Applied to files:
autogpt_platform/backend/backend/copilot/session_tenancy.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/api/features/chat/routes_test.py
📚 Learning: 2026-03-16T16:35:40.236Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12440
File: autogpt_platform/backend/backend/api/features/workflow_import.py:54-63
Timestamp: 2026-03-16T16:35:40.236Z
Learning: Avoid using the word 'competitor' in public-facing identifiers and text. Use neutral naming for API paths, model names, function names, and UI text. Examples: rename 'CompetitorFormat' to 'SourcePlatform', 'convert_competitor_workflow' to 'convert_workflow', '/competitor-workflow' to '/workflow'. Apply this guideline to files under autogpt_platform/backend and autogpt_platform/frontend.
Applied to files:
autogpt_platform/backend/backend/copilot/session_tenancy.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/api/features/chat/routes_test.py
📚 Learning: 2026-03-31T15:37:38.626Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12623
File: autogpt_platform/backend/backend/copilot/tools/agent_generator/fixer.py:37-47
Timestamp: 2026-03-31T15:37:38.626Z
Learning: When validating/constructing Anthropic API model IDs in Significant-Gravitas/AutoGPT, allow the hyphen-separated Claude Opus 4.6 model ID `claude-opus-4-6` (it corresponds to `LlmModel.CLAUDE_4_6_OPUS` in `autogpt_platform/backend/backend/blocks/llm.py`). Do NOT require the dot-separated form in Anthropic contexts. Only OpenRouter routing variants should use the dot separator (e.g., `anthropic/claude-opus-4.6`); `claude-opus-4-6` should be treated as correct when passed to Anthropic, and flagged only if it’s used in the OpenRouter path where the dot form is expected.
Applied to files:
autogpt_platform/backend/backend/copilot/session_tenancy.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/api/features/chat/routes_test.py
📚 Learning: 2026-04-15T02:43:36.890Z
Learnt from: ntindle
Repo: Significant-Gravitas/AutoGPT PR: 12780
File: autogpt_platform/backend/backend/copilot/tools/workspace_files.py:0-0
Timestamp: 2026-04-15T02:43:36.890Z
Learning: When reviewing Python exception handlers, do not flag `isinstance(e, X)` checks as dead/unreachable if the caught exception `X` is a subclass of the exception type being handled. For example, if `X` (e.g., `VirusScanError`) inherits from `ValueError` (directly or via an intermediate class) and it can be raised within an `except ValueError:` block, then `isinstance(e, X)` inside that handler is reachable and should not be treated as dead code.
Applied to files:
autogpt_platform/backend/backend/copilot/session_tenancy.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/api/features/chat/routes_test.py
📚 Learning: 2026-05-23T05:29:43.085Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 13200
File: autogpt_platform/backend/backend/executor/scheduler.py:590-593
Timestamp: 2026-05-23T05:29:43.085Z
Learning: When reviewing Python code that uses Pydantic discriminated/tagged unions (e.g., `Annotated[Union[...], Field(discriminator="kind")]`), recognize that using `isinstance(x, SomeVariantInfo)` to narrow the union is an intentional and correct runtime guard and should also enable static type narrowing in tools like Pyright. Do not recommend replacing such `isinstance`-based narrowing with `cast(...)` when the check already proves the variant at runtime.
Applied to files:
autogpt_platform/backend/backend/copilot/session_tenancy.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/api/features/chat/routes_test.py
📚 Learning: 2026-04-22T11:46:04.431Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12881
File: autogpt_platform/backend/backend/copilot/config.py:0-0
Timestamp: 2026-04-22T11:46:04.431Z
Learning: Do not flag the Claude Sonnet 4.6 model ID as incorrect when it uses the project’s established hyphenated convention: `anthropic/claude-sonnet-4-6`. This hyphen form is the intentional, production convention and should be treated as valid (including in files like llm.py, blocks tests, reasoning.py, `_is_anthropic_model` tests, and config defaults). Note that OpenRouter also accepts the dot variant `anthropic/claude-sonnet-4.6`, so either form may be tolerated, but `anthropic/claude-sonnet-4-6` should be considered the standard to match project usage.
Applied to files:
autogpt_platform/backend/backend/copilot/session_tenancy.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/api/features/chat/routes_test.py
📚 Learning: 2026-04-22T11:46:12.892Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12881
File: autogpt_platform/backend/backend/copilot/baseline/service.py:322-332
Timestamp: 2026-04-22T11:46:12.892Z
Learning: In this codebase (Significant-Gravitas/AutoGPT), OpenRouter-routed Anthropic model IDs should use the hyphen-separated convention (e.g., `anthropic/claude-sonnet-4-6`, `anthropic/claude-opus-4-6`). Although OpenRouter may accept both hyphen and dot variants, treat the hyphen-separated form as the intended, correct codebase-wide convention and do not flag it as an error. Only flag the dot-separated variant (e.g., `anthropic/claude-sonnet-4.6`) as incorrect when reviewing/validating model ID strings for OpenRouter-routed Anthropic models.
Applied to files:
autogpt_platform/backend/backend/copilot/session_tenancy.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/api/features/chat/routes_test.py
📚 Learning: 2026-05-07T18:48:14.242Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 13040
File: autogpt_platform/backend/backend/blocks/llm.py:0-0
Timestamp: 2026-05-07T18:48:14.242Z
Learning: In this repository, isort may split imports from the same module into separate blocks when some imports are aliased (e.g., `from module import X as Y`) and others are not. Preserve the two-block layout when it results from isort (such as keeping `from openai.types.chat import ChatCompletion as OpenAIChatCompletion` separate from non-aliased imports from `openai.types.chat`). Do not treat that split as a style issue during review; merging them into a single block can fail CI with `Imports are incorrectly sorted and/or formatted`.
Applied to files:
autogpt_platform/backend/backend/copilot/session_tenancy.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/api/features/chat/routes_test.py
📚 Learning: 2026-05-26T14:24:34.866Z
Learnt from: Abhi1992002
Repo: Significant-Gravitas/AutoGPT PR: 13217
File: autogpt_platform/backend/backend/api/features/search/service.py:137-137
Timestamp: 2026-05-26T14:24:34.866Z
Learning: In the Significant-Gravitas/AutoGPT backend, treat `user_id` (an opaque UUID used only for correlation/tracing) as non-PII. Do not flag direct logging of `user_id` in `logger.warning`/`logger.info` statements as a PII exposure issue, as the established convention is to log `user_id` for tracing while reserving PII for fields like email or display name.
Applied to files:
autogpt_platform/backend/backend/copilot/session_tenancy.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/api/features/chat/routes_test.py
📚 Learning: 2026-06-11T19:39:10.493Z
Learnt from: ntindle
Repo: Significant-Gravitas/AutoGPT PR: 13337
File: autogpt_platform/backend/backend/copilot/graphiti/reranker.py:0-0
Timestamp: 2026-06-11T19:39:10.493Z
Learning: In the Significant-Gravitas/AutoGPT Python backend, when calling the OpenAI Python client `chat.completions.create`, construct the `messages` payload using the concrete typed-dict variants from `openai.types.chat` (e.g., `ChatCompletionSystemMessageParam`, `ChatCompletionUserMessageParam`, etc.) rather than trying to instantiate `ChatCompletionMessageParam` directly. `ChatCompletionMessageParam` is a `Union` alias and is not constructible, so `ChatCompletionMessageParam(role=..., content=...)` should fail type checking. Build each message element with the appropriate concrete typed dict and then annotate the resulting list as `list[ChatCompletionMessageParam]` (e.g., `messages: list[ChatCompletionMessageParam] = [ChatCompletionSystemMessageParam(...), ...]`).
Applied to files:
autogpt_platform/backend/backend/copilot/session_tenancy.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/api/features/chat/routes_test.py
🔇 Additional comments (8)
autogpt_platform/backend/backend/api/features/chat/routes.py (1)
73-76: LGTM!Also applies to: 1169-1190
autogpt_platform/backend/backend/copilot/session_tenancy.py (1)
1-85: LGTM!autogpt_platform/backend/backend/api/features/chat/routes_test.py (2)
369-574: LGTM!
1001-1005: LGTM!autogpt_platform/backend/backend/copilot/turn_queue.py (2)
52-55: LGTM!
299-306: LGTM!Also applies to: 376-380
autogpt_platform/backend/backend/copilot/turn_queue_test.py (2)
44-62: LGTM!
374-454: LGTM!
|
/batch orgs |
… close parity gaps Addresses the review round on #13650 (SECRT-2489). - verify_session_org_membership called the global Prisma client, but dispatch_next_for_user runs inside the CoPilot executor where Prisma is not connected, so org-tagged queued turns raised into stream_registry's broad except and were never promoted. The two membership reads move into orgs.db.get_session_tenancy_membership, registered on DatabaseManager / DatabaseManagerAsyncClient, and are reached via db_accessors.orgs_db(). - Gate soft-deleted orgs (include Org, require deletedAt is None) and require Team.orgId == organization_id before honouring a team, matching get_request_context. - Re-verify tenancy on POST /sessions/{id}/messages/pending too: pending messages drain straight into the running turn loop. - Extract _resolve_promotable_head so a revoked queue head no longer wastes the freed promotion slot; the next valid queued session is promoted in the same tick. - Rename verify_session_org_membership -> resolve_session_tenancy; collapse the triplicated rationale comment onto the module docstring. - Tests: soft-deleted org, missing Org relation, team outside the session org, the pending-message gate, promote-next-after-revoked-head, the lost-CAS race, plus exact unique-lookup and helper-call-arg assertions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
cla: pending
|
👋 Friendly reminder: This PR is waiting on a signed CLA. All contributors need to sign our Contributor License Agreement before we can merge this PR. Why do we need a CLA?The CLA protects both you and the project by clarifying the terms under which your contribution is made. It's a one-time process — once signed, it covers all your future contributions. Common issues
If you have questions, just ask! 🙂 |
github-actions
Bot
removed
the
cla: pending
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@autogpt_platform/backend/backend/api/features/chat/routes.py`:
- Around line 1207-1220: Update queue_pending_for_http call handling in
autogpt_platform/backend/backend/api/features/chat/routes.py at lines 1207-1220
to preserve and pass the resolved turn_team_id for the /stream continuation
path. Also update lines 1547-1558 to retain the return value from
resolve_session_tenancy and propagate that resolved team context into
pending-message execution, ensuring both paths avoid stale team context.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d254d33-3b2a-4b41-a278-63a3b5c13456
📒 Files selected for processing (10)
autogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/api/features/chat/routes_test.pyautogpt_platform/backend/backend/api/features/orgs/db.pyautogpt_platform/backend/backend/api/features/orgs/model.pyautogpt_platform/backend/backend/copilot/session_tenancy.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/frontend/src/app/api/openapi.json
🚧 Files skipped from review as they are similar to previous changes (1)
- autogpt_platform/backend/backend/api/features/chat/routes_test.py
📜 Review details
⏰ Context from checks skipped due to timeout. (18)
- GitHub Check: lint
- GitHub Check: integration_test
- GitHub Check: check API types
- GitHub Check: Cursor Bugbot
- GitHub Check: Seer Code Review
- GitHub Check: lint
- GitHub Check: types
- GitHub Check: end-to-end tests
- GitHub Check: test (3.11)
- GitHub Check: type-check (3.13)
- GitHub Check: type-check (3.12)
- GitHub Check: type-check (3.11)
- GitHub Check: test (3.12)
- GitHub Check: lint
- GitHub Check: test (3.13)
- GitHub Check: Check PR Status
- GitHub Check: Analyze (typescript)
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (7)
autogpt_platform/backend/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
autogpt_platform/backend/**/*.py: Use Python 3.11 (required; managed by Poetry via pyproject.toml) for backend development
Always run 'poetry run format' (Black + isort) before linting in backend development
Always run 'poetry run lint' (ruff) after formatting in backend development
autogpt_platform/backend/**/*.py: Usepoetry run ...command for executing Python package dependencies
Use top-level imports only — avoid local/inner imports except for lazy imports of heavy optional dependencies likeopenpyxl
Use absolute imports withfrom backend.module import ...for cross-package imports; single-dot relative imports are acceptable for sibling modules within the same package; avoid double-dot relative imports
Do not use duck typing — avoidhasattr/getattr/isinstancefor type dispatch; use typed interfaces/unions/protocols instead
Use Pydantic models over dataclass/namedtuple/dict for structured data
Do not use linter suppressors — no# type: ignore,# noqa,# pyright: ignore; fix the type/code instead
Prefer list comprehensions over manual loop-and-append patterns
Use early return with guard clauses first to avoid deep nesting
Use%sfor deferred interpolation indebuglog statements for efficiency; use f-strings elsewhere for readability (e.g.,logger.debug("Processing %s items", count)vslogger.info(f"Processing {count} items"))
Sanitize error paths by usingos.path.basename()in error messages to avoid leaking directory structure
Be aware of TOCTOU (Time-Of-Check-Time-Of-Use) issues — avoid check-then-act patterns for file access and credit charging
Usetransaction=Truefor Redis pipelines to ensure atomicity on multi-step operations
Usemax(0, value)guards for computed values that should never be negative
Keep files under ~300 lines; if a file grows beyond this, split by responsibility (extract helpers, models, or a sub-module into a new file)
Keep functions under ~40 lines; extract named helpers when a function grows longer
...
Files:
autogpt_platform/backend/backend/api/features/orgs/model.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/api/features/orgs/db.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/session_tenancy.py
autogpt_platform/backend/backend/api/features/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Update routes in '/backend/backend/api/features/' and add/update Pydantic models in the same directory for API development
Files:
autogpt_platform/backend/backend/api/features/orgs/model.pyautogpt_platform/backend/backend/api/features/orgs/db.pyautogpt_platform/backend/backend/api/features/chat/routes.py
autogpt_platform/{backend,autogpt_libs}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Format Python code with
poetry run format
Files:
autogpt_platform/backend/backend/api/features/orgs/model.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/api/features/orgs/db.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/session_tenancy.py
autogpt_platform/backend/**/api/**/*.py
📄 CodeRabbit inference engine (autogpt_platform/backend/AGENTS.md)
autogpt_platform/backend/**/api/**/*.py: UseSecurity()instead ofDepends()for authentication dependencies to get proper OpenAPI security specification
Follow SSE (Server-Sent Events) protocol: usedata:lines for frontend-parsed events (must match Zod schema) and: commentlines for heartbeats/status
Files:
autogpt_platform/backend/backend/api/features/orgs/model.pyautogpt_platform/backend/backend/api/features/orgs/db.pyautogpt_platform/backend/backend/api/features/chat/routes.py
autogpt_platform/backend/backend/data/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All data access in backend requires user ID checks; verify this for any 'data/*.py' changes
Files:
autogpt_platform/backend/backend/data/db_manager.py
autogpt_platform/**/data/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
For changes touching
data/*.py, validate user ID checks or explain why not needed
Files:
autogpt_platform/backend/backend/data/db_manager.py
autogpt_platform/backend/**/*_test.py
📄 CodeRabbit inference engine (autogpt_platform/backend/AGENTS.md)
autogpt_platform/backend/**/*_test.py: Use pytest with snapshot testing for API responses
Colocate test files with source files using*_test.pynaming convention
Mock at boundaries — mock where the symbol is used, not where it's defined; after refactoring, update mock targets to match new module paths
UseAsyncMockfromunittest.mockfor async functions in tests
When writing tests, use Test-Driven Development (TDD): write failing tests marked with@pytest.mark.xfailbefore implementation, then remove the marker once the implementation is complete
When creating snapshots in tests, usepoetry run pytest path/to/test.py --snapshot-update; always review snapshot changes withgit diffbefore committing
Files:
autogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/copilot/turn_queue_test.py
🧠 Learnings (19)
📚 Learning: 2026-02-26T17:02:22.448Z
Learnt from: Pwuts
Repo: Significant-Gravitas/AutoGPT PR: 12211
File: .pre-commit-config.yaml:160-179
Timestamp: 2026-02-26T17:02:22.448Z
Learning: Keep the pre-commit hook pattern broad for autogpt_platform/backend to ensure OpenAPI schema changes are captured. Do not narrow to backend/api/ alone, since the generated schema depends on Pydantic models across multiple directories (backend/data/, backend/blocks/, backend/copilot/, backend/integrations/, backend/util/). Narrowing could miss schema changes and cause frontend type desynchronization.
Applied to files:
autogpt_platform/backend/backend/api/features/orgs/model.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/api/features/orgs/db.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/session_tenancy.py
📚 Learning: 2026-03-05T15:42:08.207Z
Learnt from: ntindle
Repo: Significant-Gravitas/AutoGPT PR: 12297
File: .claude/skills/backend-check/SKILL.md:14-16
Timestamp: 2026-03-05T15:42:08.207Z
Learning: In Python files under autogpt_platform/backend (recursively), rely on poetry run format to perform formatting (Black + isort) and linting (ruff). Do not run poetry run lint as a separate step after poetry run format, since format already includes linting checks.
Applied to files:
autogpt_platform/backend/backend/api/features/orgs/model.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/api/features/orgs/db.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/session_tenancy.py
📚 Learning: 2026-03-16T16:35:40.236Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12440
File: autogpt_platform/backend/backend/api/features/workflow_import.py:54-63
Timestamp: 2026-03-16T16:35:40.236Z
Learning: Avoid using the word 'competitor' in public-facing identifiers and text. Use neutral naming for API paths, model names, function names, and UI text. Examples: rename 'CompetitorFormat' to 'SourcePlatform', 'convert_competitor_workflow' to 'convert_workflow', '/competitor-workflow' to '/workflow'. Apply this guideline to files under autogpt_platform/backend and autogpt_platform/frontend.
Applied to files:
autogpt_platform/backend/backend/api/features/orgs/model.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/api/features/orgs/db.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/session_tenancy.py
📚 Learning: 2026-03-31T15:37:38.626Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12623
File: autogpt_platform/backend/backend/copilot/tools/agent_generator/fixer.py:37-47
Timestamp: 2026-03-31T15:37:38.626Z
Learning: When validating/constructing Anthropic API model IDs in Significant-Gravitas/AutoGPT, allow the hyphen-separated Claude Opus 4.6 model ID `claude-opus-4-6` (it corresponds to `LlmModel.CLAUDE_4_6_OPUS` in `autogpt_platform/backend/backend/blocks/llm.py`). Do NOT require the dot-separated form in Anthropic contexts. Only OpenRouter routing variants should use the dot separator (e.g., `anthropic/claude-opus-4.6`); `claude-opus-4-6` should be treated as correct when passed to Anthropic, and flagged only if it’s used in the OpenRouter path where the dot form is expected.
Applied to files:
autogpt_platform/backend/backend/api/features/orgs/model.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/api/features/orgs/db.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/session_tenancy.py
📚 Learning: 2026-04-15T02:43:36.890Z
Learnt from: ntindle
Repo: Significant-Gravitas/AutoGPT PR: 12780
File: autogpt_platform/backend/backend/copilot/tools/workspace_files.py:0-0
Timestamp: 2026-04-15T02:43:36.890Z
Learning: When reviewing Python exception handlers, do not flag `isinstance(e, X)` checks as dead/unreachable if the caught exception `X` is a subclass of the exception type being handled. For example, if `X` (e.g., `VirusScanError`) inherits from `ValueError` (directly or via an intermediate class) and it can be raised within an `except ValueError:` block, then `isinstance(e, X)` inside that handler is reachable and should not be treated as dead code.
Applied to files:
autogpt_platform/backend/backend/api/features/orgs/model.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/api/features/orgs/db.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/session_tenancy.py
📚 Learning: 2026-05-23T05:29:43.085Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 13200
File: autogpt_platform/backend/backend/executor/scheduler.py:590-593
Timestamp: 2026-05-23T05:29:43.085Z
Learning: When reviewing Python code that uses Pydantic discriminated/tagged unions (e.g., `Annotated[Union[...], Field(discriminator="kind")]`), recognize that using `isinstance(x, SomeVariantInfo)` to narrow the union is an intentional and correct runtime guard and should also enable static type narrowing in tools like Pyright. Do not recommend replacing such `isinstance`-based narrowing with `cast(...)` when the check already proves the variant at runtime.
Applied to files:
autogpt_platform/backend/backend/api/features/orgs/model.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/api/features/orgs/db.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/session_tenancy.py
📚 Learning: 2026-04-22T11:46:04.431Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12881
File: autogpt_platform/backend/backend/copilot/config.py:0-0
Timestamp: 2026-04-22T11:46:04.431Z
Learning: Do not flag the Claude Sonnet 4.6 model ID as incorrect when it uses the project’s established hyphenated convention: `anthropic/claude-sonnet-4-6`. This hyphen form is the intentional, production convention and should be treated as valid (including in files like llm.py, blocks tests, reasoning.py, `_is_anthropic_model` tests, and config defaults). Note that OpenRouter also accepts the dot variant `anthropic/claude-sonnet-4.6`, so either form may be tolerated, but `anthropic/claude-sonnet-4-6` should be considered the standard to match project usage.
Applied to files:
autogpt_platform/backend/backend/api/features/orgs/model.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/api/features/orgs/db.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/session_tenancy.py
📚 Learning: 2026-04-22T11:46:12.892Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12881
File: autogpt_platform/backend/backend/copilot/baseline/service.py:322-332
Timestamp: 2026-04-22T11:46:12.892Z
Learning: In this codebase (Significant-Gravitas/AutoGPT), OpenRouter-routed Anthropic model IDs should use the hyphen-separated convention (e.g., `anthropic/claude-sonnet-4-6`, `anthropic/claude-opus-4-6`). Although OpenRouter may accept both hyphen and dot variants, treat the hyphen-separated form as the intended, correct codebase-wide convention and do not flag it as an error. Only flag the dot-separated variant (e.g., `anthropic/claude-sonnet-4.6`) as incorrect when reviewing/validating model ID strings for OpenRouter-routed Anthropic models.
Applied to files:
autogpt_platform/backend/backend/api/features/orgs/model.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/api/features/orgs/db.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/session_tenancy.py
📚 Learning: 2026-05-07T18:48:14.242Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 13040
File: autogpt_platform/backend/backend/blocks/llm.py:0-0
Timestamp: 2026-05-07T18:48:14.242Z
Learning: In this repository, isort may split imports from the same module into separate blocks when some imports are aliased (e.g., `from module import X as Y`) and others are not. Preserve the two-block layout when it results from isort (such as keeping `from openai.types.chat import ChatCompletion as OpenAIChatCompletion` separate from non-aliased imports from `openai.types.chat`). Do not treat that split as a style issue during review; merging them into a single block can fail CI with `Imports are incorrectly sorted and/or formatted`.
Applied to files:
autogpt_platform/backend/backend/api/features/orgs/model.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/api/features/orgs/db.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/session_tenancy.py
📚 Learning: 2026-05-26T14:24:34.866Z
Learnt from: Abhi1992002
Repo: Significant-Gravitas/AutoGPT PR: 13217
File: autogpt_platform/backend/backend/api/features/search/service.py:137-137
Timestamp: 2026-05-26T14:24:34.866Z
Learning: In the Significant-Gravitas/AutoGPT backend, treat `user_id` (an opaque UUID used only for correlation/tracing) as non-PII. Do not flag direct logging of `user_id` in `logger.warning`/`logger.info` statements as a PII exposure issue, as the established convention is to log `user_id` for tracing while reserving PII for fields like email or display name.
Applied to files:
autogpt_platform/backend/backend/api/features/orgs/model.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/api/features/orgs/db.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/session_tenancy.py
📚 Learning: 2026-06-11T19:39:10.493Z
Learnt from: ntindle
Repo: Significant-Gravitas/AutoGPT PR: 13337
File: autogpt_platform/backend/backend/copilot/graphiti/reranker.py:0-0
Timestamp: 2026-06-11T19:39:10.493Z
Learning: In the Significant-Gravitas/AutoGPT Python backend, when calling the OpenAI Python client `chat.completions.create`, construct the `messages` payload using the concrete typed-dict variants from `openai.types.chat` (e.g., `ChatCompletionSystemMessageParam`, `ChatCompletionUserMessageParam`, etc.) rather than trying to instantiate `ChatCompletionMessageParam` directly. `ChatCompletionMessageParam` is a `Union` alias and is not constructible, so `ChatCompletionMessageParam(role=..., content=...)` should fail type checking. Build each message element with the appropriate concrete typed dict and then annotate the resulting list as `list[ChatCompletionMessageParam]` (e.g., `messages: list[ChatCompletionMessageParam] = [ChatCompletionSystemMessageParam(...), ...]`).
Applied to files:
autogpt_platform/backend/backend/api/features/orgs/model.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/api/features/orgs/db.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/copilot/session_tenancy.py
📚 Learning: 2026-03-01T07:58:56.207Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12213
File: autogpt_platform/frontend/src/app/api/openapi.json:10030-10037
Timestamp: 2026-03-01T07:58:56.207Z
Learning: When a backend field represents sensitive data, use a secret type (e.g., Pydantic SecretStr with length constraints) so OpenAPI marks it as a password/writeOnly field. Apply this pattern to similar sensitive request fields across API schemas so generated TypeScript clients and docs treat them as secrets and do not mishandle sensitivity. Review all openapi.jsons where sensitive inputs are defined and replace plain strings with SecretStr-like semantics with appropriate minLength constraints.
Applied to files:
autogpt_platform/frontend/src/app/api/openapi.json
📚 Learning: 2026-04-14T06:39:49.111Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12773
File: autogpt_platform/frontend/src/app/api/openapi.json:12803-12806
Timestamp: 2026-04-14T06:39:49.111Z
Learning: In OpenAPI specs, ensure the schema/message length caps for the StreamChatRequest.message and QueuePendingMessageRequest.message fields are set to the intended values: StreamChatRequest.message maxLength must be 64000 and QueuePendingMessageRequest.message maxLength must be 32000. Keep QueuePendingMessageRequest.message consistent with PendingMessage.content, and ensure the pending (queue) ceiling never exceeds the stream ceiling because both ultimately feed the same LLM context window. Update any legacy smaller limits (e.g., 4000/16000) to these newer ceilings.
Applied to files:
autogpt_platform/frontend/src/app/api/openapi.json
📚 Learning: 2026-03-07T07:43:09.871Z
Learnt from: kcze
Repo: Significant-Gravitas/AutoGPT PR: 12328
File: autogpt_platform/frontend/src/app/api/openapi.json:1116-1118
Timestamp: 2026-03-07T07:43:09.871Z
Learning: For autogpt_platform/frontend/src/app/api/openapi.json, preserve the existing behavior: HTTPBearerJWT is declared at the router level with Depends(auth.get_user_id) returning None for unauthenticated users; treat as optional auth. Do not change per-operation security descriptions unless you plan a repo-wide OpenAPI update. If you change this file, prefer clarifying operation descriptions rather than altering security requirements.
Applied to files:
autogpt_platform/frontend/src/app/api/openapi.json
📚 Learning: 2026-04-21T04:35:34.710Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12865
File: autogpt_platform/backend/backend/data/credit.py:1584-1584
Timestamp: 2026-04-21T04:35:34.710Z
Learning: When reviewing this codebase, don’t flag snake_case attribute names (e.g., `subscription_tier`, `stripe_customer_id`, `top_up_config`) on the app-layer Pydantic `User` model as “wrong” field names. These are correct for the app-layer model and are expected to be mapped from the Prisma-layer camelCase fields (e.g., `subscriptionTier`, `stripeCustomerId`) inside methods like `User.from_db()`. Only Prisma-returned/raw objects would use camelCase, but functions like `get_user_by_id(user_id: str)` are expected to return the Pydantic app-layer model.
Applied to files:
autogpt_platform/backend/backend/data/db_manager.py
📚 Learning: 2026-05-07T15:32:39.703Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 13033
File: autogpt_platform/backend/backend/data/generate_data.py:111-117
Timestamp: 2026-05-07T15:32:39.703Z
Learning: When reviewing the Python data-generation layer, do not treat missing `user_id`/user filtering in calls to graph-metadata resolvers as a security issue if the `graph_id` inputs are already guaranteed to be user-scoped by earlier upstream SQL (e.g., `WHERE "userId" = ...`). In particular, `_resolve_agent_name(graph_id)` in `generate_data.py` correctly calls `get_graph_metadata(graph_id=graph_id)` without a `user_id` parameter by design, because name resolution must also work for user-executed shared/marketplace agents that the user may not own.
Applied to files:
autogpt_platform/backend/backend/data/db_manager.py
📚 Learning: 2026-03-04T08:04:35.881Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12273
File: autogpt_platform/backend/backend/copilot/tools/workspace_files.py:216-220
Timestamp: 2026-03-04T08:04:35.881Z
Learning: In the AutoGPT Copilot backend, ensure that SVG images are not treated as vision image types by excluding 'image/svg+xml' from INLINEABLE_MIME_TYPES and MULTIMODAL_TYPES in tool_adapter.py; the Claude API supports PNG, JPEG, GIF, and WebP for vision. SVGs (XML text) should be handled via the text path instead, not the vision path.
Applied to files:
autogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/copilot/session_tenancy.py
📚 Learning: 2026-04-01T04:17:41.600Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12632
File: autogpt_platform/backend/backend/copilot/tools/workspace_files.py:0-0
Timestamp: 2026-04-01T04:17:41.600Z
Learning: When reviewing AutoGPT Copilot tool implementations, accept that `readOnlyHint=True` (provided via `ToolAnnotations`) may be applied unconditionally to *all* tools—even tools that have side effects (e.g., `bash_exec`, `write_workspace_file`, or other write/save operations). Do **not** flag these tools for having `readOnlyHint=True`; this is intentional to enable fully-parallel dispatch by the Anthropic SDK/CLI and has been E2E validated. Only flag `readOnlyHint` issues if they conflict with the established `ToolAnnotations` behavior (e.g., missing/incorrect propagation relative to the intended annotation mechanism).
Applied to files:
autogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/copilot/session_tenancy.py
📚 Learning: 2026-06-06T12:22:37.648Z
Learnt from: anvyle
Repo: Significant-Gravitas/AutoGPT PR: 13302
File: autogpt_platform/backend/backend/copilot/tools/agent_search.py:579-583
Timestamp: 2026-06-06T12:22:37.648Z
Learning: When writing LLM-facing instruction strings that trigger tool calls in this AutoGPT codebase, use the exact registered tool name `view_agent_output` (as defined in `backend/copilot/tools/agent_output.py` via its `name` property and exported via `TOOL_REGISTRY`). Do not reference the bare name `agent_output`, since it is not a valid tool name and will cause tool invocation to fail.
Applied to files:
autogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/copilot/session_tenancy.py
🔇 Additional comments (12)
autogpt_platform/backend/backend/api/features/orgs/model.py (1)
35-43: LGTM!autogpt_platform/backend/backend/api/features/orgs/db.py (1)
7-7: LGTM!Also applies to: 18-24, 183-230
autogpt_platform/backend/backend/data/db_manager.py (1)
34-37: LGTM!Also applies to: 447-450, 742-742
autogpt_platform/backend/backend/copilot/session_tenancy.py (1)
3-35: LGTM!Also applies to: 54-90
autogpt_platform/backend/backend/copilot/session_tenancy_test.py (1)
3-199: LGTM!autogpt_platform/backend/backend/api/features/chat/routes.py (1)
76-76: LGTM!Also applies to: 130-132, 1144-1144, 1524-1524
autogpt_platform/frontend/src/app/api/openapi.json (1)
3704-3706: LGTM!Also applies to: 3992-3994
autogpt_platform/backend/backend/copilot/turn_queue.py (3)
42-42: LGTM!Also applies to: 55-55
295-302: LGTM!Also applies to: 353-357
393-435: LGTM!autogpt_platform/backend/backend/copilot/turn_queue_test.py (2)
10-18: LGTM!Also applies to: 201-220
410-432: LGTM!Also applies to: 435-477, 480-506, 518-535
|
🤖 Addressed the review round in 132df49fb. Summary of what changed and what I pushed back on — inline replies are on each thread. Blockers1. Prisma called from the Prisma-less executor (@kcze, Codex, Cursor Bugbot, reviewer) — real, fixed. The two membership reads moved into 2. Soft-deleted org not blocked — real, fixed. The org gate now uses Should-Fix
NitsRenamed Pushed back onNice-to-Have 1 — skip re-verification when the session tenancy already matches Nice-to-Have 2 — factor the shared membership primitives out of TestsNew coverage: soft-deleted org, missing |
|
/review |
![autogpt-pr-reviewer[bot]](https://avatars.githubusercontent.com/u/130738209?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Automated Review — PR #13650
PR #13650 — fix(backend): re-verify org/team membership on every chat turn
Author: ntindle | Files: 10
🎯 Verdict: APPROVE
PR Description Quality
✅ Has Why (closes SECRT-2489 privilege-persistence gap) + What (per-turn tenancy re-verification) + How (gate at /stream, /messages/pending, queue promotion; hard-403 stale org, soft-strip stale team, skip untagged sessions). The description also honestly enumerates the dispatch paths it does not cover (scheduled-followup, engine-switch, sub-session waiter), tracked separately.
What This PR Does
Previously a chat session persisted its organization_id/team_id and trusted that tenancy on every turn, while membership was only checked once at session creation — so a user removed or suspended from an org could keep running agents and spending credits under that org indefinitely. This PR re-verifies org/team membership on every turn dispatch: a revoked org membership hard-403s, a stale team is silently stripped back to the org home, personal/untagged sessions are left untouched, and the queued-then-revoked promotion race is closed.
Specialist Findings
🛡️ Security ✅ — Traced every branch of the new gate; it fails closed everywhere that matters (revoked → SessionOrgMembershipRevoked → 403), tenancy comes from the server-persisted session row (not client-controllable), and both lookups are parameterized find_unique calls. The two prior blockers (soft-deleted-org bypass, executor Prisma access) are confirmed fixed. Only residual risk is the acknowledged uncovered dispatch paths, tracked on SECRT-2489.
🏗️ Architecture ✅ — Dependency direction is correct (call sites → session_tenancy → orgs_db() RPC accessor, avoiding the global Prisma client in the executor). 🟠 Notes the membership policy is now duplicated between get_session_tenancy_membership (orgs/db.py:183) and get_request_context, kept in sync by docstring/tests only, and the gate lives at N entry points rather than the execution choke point.
⚡ Performance ✅ — Adds ≤2 indexed, short-circuiting unique-key reads per turn (O(1)); dwarfed by the LLM round-trip that follows. On the executor path each check is a sequential RPC round-trip, bounded by queue depth. A short-TTL membership cache is the suggested scaling lever, not a blocker.
🧪 Testing ✅ — ~90% coverage, TDD-first; the core policy chain runs for real with only the two Prisma reads stubbed, so route/wiring regressions are actually caught. Negative cases (revoked, suspended, soft-deleted, missing Org relation, stale team, cross-org team) all present, plus the CAS-race drop. 🟠 Minor gaps: no untagged-skip test on the pending endpoint, no transient-DB-failure test.
📖 Quality ✅ — Naming is precise, and the "policy lives in the module docstring" single-source pattern is exemplary. 🟠 The 403 gate block is copy-pasted across stream_chat_post and queue_pending_message; worth extracting to a shared helper.
📦 Product [code:...] marker, so the frontend stream handler falls through to a generic banner likely offering a "Try Again" that can only re-403; and a queued turn dropped on revocation fails silently with only a server log. Both are follow-up UX gaps, not defects in the security fix.
📬 Discussion ✅ — All 30 review threads resolved, each with an author response; the two author pushbacks are reasoned and defensible. GitHub CI reported 43/43 green. Note: a stale CHANGES_REQUESTED bot review predating the fix commit (132df49fb) still governs reviewDecision and needs dismissal/re-review to unblock merge.
🔎 QA ✅ — Exercised live against a running stack across 11 scenarios: happy path (200), revoked org via a distinct second org isolating the new code path (403 with exact new detail), pending-message choke point (403), soft-deleted org (403), stale-team strip (200), untagged legacy session (200), no-auth (401), and session-stays-idle-after-403. Independently re-ran the suite: 49 associated tests pass. Queue-promotion path covered by unit tests only (can't be raced live).
🟠 Should Fix
- Duplicated 403 tenancy gate across two handlers (
routes.py:1207&~1546) — extract_enforce_session_tenancy(session, user_id) -> str | Noneso both choke points (and future ones) stay in sync. (Flagged by: architect, quality — 2 specialists) - New 403 has no curated frontend handling (
routes.py:130) — plain-string detail falls throughcopilotStreamErrorHandlers.tsto a generic banner that likely offers a non-working "Try Again"; emit a[code:org_membership_revoked]marker or add an explicit non-retryable case with recovery copy. (Flagged by: product) - Queued-turn revocation drop is silent (
turn_queue.py:~421) — a droppedqueued→idleturn gives the user no signal; persist a retryable/terminal marker so the UI can explain it. (Flagged by: product) - Policy fork with
get_request_context(orgs/db.py:183) — membership predicate is re-implemented and guarded only by prose/tests; extract a shared predicate so a future org-status field can't silently drift the per-turn gate. (Flagged by: architect, security — 2 specialists) - Missing pending-path untagged-skip and transient-failure tests (
routes.py:1537,session_tenancy.py:71) — add anorganization_id=Noneskip test mirroring the stream path, and one test pinning the intended behavior when the membership read raises a non-revocation error. (Flagged by: testing)
🟡 Nice to Have
- Short-TTL membership cache (
session_tenancy.py) — a 5–15s Redis cache keyed on(user_id, org_id, team_id)across the three choke points would eliminate steady-state read load as chat volume scales, keeping the revocation window tight. (performance) - Skip-next on transient promotion error (
turn_queue.py:~410) — a non-revocation error currently aborts the whole dispatch tick; skip that head and continue so one membership-read hiccup doesn't stall the user's queue. (security, performance)
🔵 Nits
turn_team_idcomputed then overwritten (routes.py:1204) — the org-branch of the first assignment is dead whenorganization_idis set; a one-line comment would clarify. (quality)- Shared test-stub helpers (
session_tenancy_test.py:28,routes_test.py:~382) — near-duplicate_org_member/_team_memberfixtures could live in a shared conftest. (quality)
QA Screenshots
| Screenshot | Description |
|---|---|
 |
Copilot page loads authenticated, full stack up ✅ |
 |
Message sent through UI, reply renders end-to-end ✅ |
Human Review Needed
YES — This change modifies an authorization/trust boundary (per-turn tenancy enforcement between users and orgs/teams). A maintainer re-review is warranted to clear the stale CHANGES_REQUESTED decision, even though the code is sound and QA-proven.
Risk Assessment
Merge risk: LOW | Rollback: EASY (isolated, additive gate; revertable without schema/data migration)
CI Status
GitHub CI: reported 43/43 required checks green on the fix commit (132df49fb) per the discussion review; no merge conflicts. Local harness: backend lint ✅, frontend lint/typecheck/build ✅; the frontend pnpm test:unit suite failed in the sandbox — this is an unrelated frontend suite for a backend-only change and is environment skew against the green GitHub run, not a blocker. QA independently re-ran the 49 backend tests associated with this PR: all pass.
UI Testing — Variant Results
✅ local: Per-turn org/team re-verification works correctly at every dispatch choke point — revoked/suspended/soft-deleted org returns 403, stale team soft-strips and proceeds, legacy sessions unaffected, all 49 associated tests pass on independent run.
✅ hosted: Per-turn org/team re-verification works live: non-member org yields 403 at both /stream and /messages/pending with no dispatch, active member and restored membership return 200, negative auth returns 401, and both new unit suites pass (25 tests).
| sessions keep their persisted user message; a re-send from a valid | ||
| context re-dispatches it. | ||
| """ | ||
| for session in queued: |
There was a problem hiding this comment.
🤖 🟢 low (security/availability / error handling)
The promotion loop only catches SessionOrgMembershipRevoked. A transient error from resolve_session_tenancy (RPC timeout, Prisma error) on one queued head propagates out of dispatch_next_for_user and aborts the whole tick, so a still-valid queued session behind it is not promoted. Fails closed (no wrong-tenant run) but couples one membership-read hiccup to the user's entire queue availability.
Suggestion: Catch non-revoked exceptions per-session and continue to the next queued head (logging distinctly), rather than letting them abort the whole dispatch tick.
| *team_id* is ``None`` or the org already failed. | ||
|
|
||
| Returns plain booleans rather than Prisma rows so Prisma-less processes | ||
| (the CoPilot executor) can call this over the DatabaseManager RPC. |
There was a problem hiding this comment.
🤖 🟢 low (security/auth-parity verification)
get_session_tenancy_membership gates the org on ACTIVE OrgMember + Org.deletedAt is None and claims exact parity with get_request_context. If get_request_context also inspects any org-level suspension/status column beyond deletedAt, this per-turn re-check would be weaker than the header path.
Suggestion: Confirm get_request_context checks no additional Organization-level status field; if it does, mirror that predicate here so the re-check cannot pass where a normal request would 403.
| return org_id, ws_id | ||
|
|
||
|
|
||
| async def get_session_tenancy_membership( |
There was a problem hiding this comment.
🤖 🟡 medium (architect/policy-duplication)
get_session_tenancy_membership re-implements the ACTIVE-member + Org.deletedAt + Team.orgId membership policy that get_request_context enforces. The equivalence is guaranteed only by docstring and tests, so a future change to get_request_context (new OrgMemberStatus, billing suspension, invite-pending) silently drifts this copy and the per-turn gate diverges from session-creation gating.
Suggestion: Extract the shared membership predicate (e.g. membership_is_active(org_member) / team_is_valid(team_member, org_id)) into one function consumed by both get_request_context and get_session_tenancy_membership so the policy has a single definition.
| @@ -1195,6 +1204,21 @@ async def stream_chat_post( | |||
| turn_org_id = session.organization_id or ctx.org_id | |||
| turn_team_id = session.team_id if session.organization_id else ctx.team_id | |||
|
|
|||
There was a problem hiding this comment.
🤖 🟢 low (architect/duplication)
The tenancy gate (if session.organization_id is not None -> try resolve_session_tenancy -> except SessionOrgMembershipRevoked -> raise HTTPException 403) is copy-pasted between stream_chat_post and queue_pending_message (~line 1543). Each new HTTP dispatch entry point will duplicate it again.
Suggestion: Factor the gate into a shared helper or FastAPI dependency (e.g. _enforce_session_tenancy(session, user_id) returning the resolved team_id) and call it from both handlers.
| @@ -0,0 +1,90 @@ | |||
| """Per-turn re-verification of a chat session's persisted org/team. | |||
There was a problem hiding this comment.
🤖 🟡 medium (architect/enforcement-placement)
Tenancy is re-verified at each dispatch entry point (/stream, /messages/pending, queue promotion) rather than at the single execution point where organization_id/team_id are consumed for billing and tool execution. The PR's own listed-not-covered paths (scheduled-followup dispatcher, engine-switch continuation, sub-session waiter) demonstrate that the invariant is only as strong as each author remembering to add the gate.
Suggestion: Track a follow-up to enforce (or duplicate-guard) tenancy at the turn-execution choke point (dispatch_turn/turn loop) so bypass is structurally impossible and entry-point checks become fast-fail optimizations.
| @@ -1195,6 +1204,21 @@ async def stream_chat_post( | |||
| turn_org_id = session.organization_id or ctx.org_id | |||
There was a problem hiding this comment.
🤖 🟢 low (quality/readability / structure)
turn_team_id is computed from session/ctx, then immediately overwritten by resolve_session_tenancy whenever organization_id is not None, making the org-branch of the first assignment dead in that path. Momentarily confusing to a maintainer.
Suggestion: Add a brief inline comment clarifying the first assignment only applies to untagged sessions, or restructure so the org-tagged branch computes turn_team_id in a single place.
| def _patch_membership_reads(mocker, *, org_member, team_member): | ||
| prisma = MagicMock() | ||
| prisma.orgmember.find_unique = AsyncMock(return_value=org_member) | ||
| prisma.teammember.find_unique = AsyncMock(return_value=team_member) |
There was a problem hiding this comment.
🤖 🟢 low (quality/test duplication)
_patch_membership_reads, _org_member, and _team_member helpers are near-duplicates of the same helpers in routes_test.py (~line 382), risking drift if the Prisma row shape changes.
Suggestion: Consider sharing these row-stub/patch helpers via a conftest fixture or a small shared test-support module.
|
|
||
| config = ChatConfig() | ||
|
|
||
| # 403 detail shared by every turn-dispatch choke point that re-verifies a |
There was a problem hiding this comment.
🤖 🟡 medium (product/error-ux)
The new 403 detail is a plain string with no [code:...] prefix, so the frontend stream handler (copilotStreamErrorHandlers.ts) falls through to generic/inline handling. Removed-from-org is terminal (retry always re-403s) but the UI gives no distinct, non-retryable treatment and likely still offers a 'Try Again' affordance that loops forever.
Suggestion: Emit this 403 with a [code:org_membership_revoked] prefix (or add an explicit case in handleStreamError) so the UI can show clear, non-retryable copy with recovery guidance (e.g. switch to personal workspace / contact org admin).
| organization_id=session.organization_id, | ||
| team_id=session.team_id, | ||
| ) | ||
| except SessionOrgMembershipRevoked: |
There was a problem hiding this comment.
🤖 🟡 medium (product/silent-failure)
_resolve_promotable_head drops a revoked queued session queued→idle with only a server-side logger.warning. The user who queued a follow-up gets no error, notification, or completion — the message silently stops progressing, and they have no way to know a re-send is required.
Suggestion: Persist a retryable/terminal-error marker or notification on the dropped session so the chat UI can explain the membership revocation, consistent with how other terminal turn failures surface to the user.
|
|
||
| config = ChatConfig() | ||
|
|
||
| # 403 detail shared by every turn-dispatch choke point that re-verifies a |
There was a problem hiding this comment.
🤖 🟢 low (product/error-copy)
The 403 message states the user was removed from the org but offers no next step, leaving a mid-conversation user with no clear path forward.
Suggestion: Add recovery guidance to the detail string (e.g. suggest starting a new session in their personal workspace or contacting an org admin).
Superseded by a newer automated review.
github-actions
Bot
added
the
conflicts
|
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
# Conflicts: # autogpt_platform/backend/backend/api/features/chat/routes.py # autogpt_platform/backend/backend/api/features/chat/routes_test.py # autogpt_platform/backend/backend/api/features/orgs/db.py # autogpt_platform/backend/backend/data/db_manager.py
github-actions
Bot
added
cla: pending
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly. |
github-actions
Bot
removed
the
conflicts
ntindle
changed the title
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| if persisted_session.organization_id is None: | ||
| raise SessionAdmissionError("session_tenancy_unverifiable") |
There was a problem hiding this comment.
Bug: The tenancy check for in-flight turns incorrectly rejects requests for legacy sessions even when a valid organization_id is provided by the caller.
Severity: MEDIUM
Suggested Fix
Update the conditional check for in-flight turns to be consistent with the non-in-flight path. The check should only raise an error if both persisted_session.organization_id and the caller-provided organization_id are None.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: autogpt_platform/backend/backend/copilot/sdk/session_waiter.py#L199-L200
Potential issue: In `run_copilot_turn_via_queue`, the logic for handling in-flight turns
has an inconsistent tenancy check for legacy sessions. When a turn is in-flight for a
session where `persisted_session.organization_id` is `None`, the code immediately raises
a `SessionAdmissionError`. This occurs even if the caller provides a valid
`organization_id`. This behavior is inconsistent with the non-in-flight path, which
correctly allows the request by only raising an error if both the persisted and the
provided `organization_id` are `None`. This blocks legitimate requests for legacy
sessions.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
autogpt_platform/backend/backend/executor/scheduler_unit_test.py (1)
336-410: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the fail-closed reason, not only the absence of calls.
These tests assert only that mocks were not awaited. They pass if
_execute_copilot_turnfails earlier for an unrelated reason, because every assertion is negative. Add one positive assertion, for example acaplogcheck for the swallowed authorization or lookup failure, so a regression that breaks the function before the tenancy gate is still detected.Also applies to: 473-510
🤖 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 `@autogpt_platform/backend/backend/executor/scheduler_unit_test.py` around lines 336 - 410, Strengthen both revoked-session tests around _execute_copilot_turn by adding a positive caplog assertion that confirms the expected authorization or tenancy-resolution failure was logged or swallowed. Keep the existing not-awaited assertions, and ensure the assertion distinguishes the SessionOrgMembershipRevoked failure from unrelated early exits.autogpt_platform/backend/backend/executor/scheduler.py (1)
256-316: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the tenancy resolution into a named helper.
_execute_copilot_turnnow spans about 105 lines and mixes session lookup, ownership validation, tenancy resolution, session creation, and dispatch. Extract the block that produces(organization_id, team_id)into a helper, for example_resolve_scheduled_tenancy(args, target_session). The coding guidelines require functions under about 40 lines: "Keep functions under ~40 lines; extract named helpers when a function grows longer".As per coding guidelines.
🤖 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 `@autogpt_platform/backend/backend/executor/scheduler.py` around lines 256 - 316, Extract the organization_id/team_id derivation and resolve_session_tenancy call from _execute_copilot_turn into a named helper such as _resolve_scheduled_tenancy(args, target_session). Have it return the resolved tenancy tuple while preserving existing session metadata, fallback, and no-organization behavior; replace the inline block with the helper call so _execute_copilot_turn remains under the function-length guideline.Source: Coding guidelines
autogpt_platform/backend/backend/blocks/autopilot.py (2)
542-543: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a reason comment for this local import.
The repository guideline requires top-level imports and allows local imports only for lazy loading of heavy optional dependencies. The sibling lazy import at Lines 382-384 carries the comment
# avoid circular import, which documents the exception. This new import ofSessionAdmissionErrorhas no such comment, so a later reader may hoist it and reintroduce the cycle.📝 Proposed comment
- from backend.copilot.sdk.session_waiter import SessionAdmissionError + from backend.copilot.sdk.session_waiter import ( + SessionAdmissionError, # avoid circular import + )As per coding guidelines: "Use top-level imports only — avoid local/inner imports except for lazy imports of heavy optional dependencies like
openpyxl".🤖 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 `@autogpt_platform/backend/backend/blocks/autopilot.py` around lines 542 - 543, Add a concise reason comment directly above the local import of SessionAdmissionError in the relevant autopilot flow, documenting that it must remain local to avoid a circular import. Keep the existing import placement and behavior unchanged.Source: Coding guidelines
357-358: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the new parameters in the docstring.
execute_copilotnow acceptsorganization_idandteam_id. TheArgs:block at Lines 371-377 listsprompt,system_context,session_id,max_recursion_depth,user_id, andpermissions, but omits the two tenancy parameters.📝 Proposed docstring addition
user_id: Authenticated user ID. + organization_id: Organization that owns the turn, or None. + team_id: Team scope within the organization, or None. permissions: Optional capability filter restricting tools/blocks.🤖 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 `@autogpt_platform/backend/backend/blocks/autopilot.py` around lines 357 - 358, Update the Args section of execute_copilot’s docstring to document the new organization_id and team_id parameters, including their tenancy-related purpose and optional nature, while leaving the existing parameter descriptions unchanged.autogpt_platform/backend/backend/copilot/tools/schedule_followup_test.py (1)
165-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssertions are correct. Add coverage for the fresh-chat fallback branch.
These lines cover the explicit-target branch of
schedule_followup.py. The other branch at Lines 256 and 261 ofschedule_followup.pyfalls back to the scheduling session's organization and team whensession_idis omitted. No test asserts that fallback, so a regression that sendsNonefor the fresh-chat sentinel would pass.The existing test that calls
_execute(..., session_id=None)can assert the same two kwargs against the current session's tenancy.Also applies to: 187-188
🤖 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 `@autogpt_platform/backend/backend/copilot/tools/schedule_followup_test.py` around lines 165 - 166, Add coverage for the fresh-chat fallback in the existing `_execute(..., session_id=None)` test: assert that the scheduling call receives the current session’s organization and team IDs rather than `None`. Keep the explicit-target assertions unchanged, and verify both tenancy kwargs passed through the fallback branch.autogpt_platform/backend/backend/copilot/tools/sub_session_test.py (1)
246-258: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssertions are correct. The resume branch stays uncovered.
This test exercises the fresh-sub-session branch, where the inner session inherits the parent tenancy. The resume branch (
sub_autopilot_session_idset) also forwards the parent tenancy while the inner session row may carry different values. Add a test that resumes an owned session with a different organization and asserts which tenancy reachesrun_copilot_turn_via_queue.🤖 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 `@autogpt_platform/backend/backend/copilot/tools/sub_session_test.py` around lines 246 - 258, Add a test alongside the existing RunSubSessionTool tenancy test that sets sub_autopilot_session_id to resume an owned inner session with organization_id differing from the parent, then assert run_copilot_turn_via_queue receives the parent organization and team tenancy. Keep the existing fresh-sub-session assertions unchanged and specifically cover the resume branch.
🤖 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 `@autogpt_platform/backend/backend/api/features/chat/routes.py`:
- Around line 1451-1459: Update both pending-message checks in chat routes.py at
lines 1451-1459 and 1808-1814 to remove the session.organization_id is None
condition, so untagged and personal sessions are not rejected. Retain the 409
response only when an organization-scoped session has a persisted team while
turn_team_id is missing, and add regression tests covering both endpoints.
---
Nitpick comments:
In `@autogpt_platform/backend/backend/blocks/autopilot.py`:
- Around line 542-543: Add a concise reason comment directly above the local
import of SessionAdmissionError in the relevant autopilot flow, documenting that
it must remain local to avoid a circular import. Keep the existing import
placement and behavior unchanged.
- Around line 357-358: Update the Args section of execute_copilot’s docstring to
document the new organization_id and team_id parameters, including their
tenancy-related purpose and optional nature, while leaving the existing
parameter descriptions unchanged.
In `@autogpt_platform/backend/backend/copilot/tools/schedule_followup_test.py`:
- Around line 165-166: Add coverage for the fresh-chat fallback in the existing
`_execute(..., session_id=None)` test: assert that the scheduling call receives
the current session’s organization and team IDs rather than `None`. Keep the
explicit-target assertions unchanged, and verify both tenancy kwargs passed
through the fallback branch.
In `@autogpt_platform/backend/backend/copilot/tools/sub_session_test.py`:
- Around line 246-258: Add a test alongside the existing RunSubSessionTool
tenancy test that sets sub_autopilot_session_id to resume an owned inner session
with organization_id differing from the parent, then assert
run_copilot_turn_via_queue receives the parent organization and team tenancy.
Keep the existing fresh-sub-session assertions unchanged and specifically cover
the resume branch.
In `@autogpt_platform/backend/backend/executor/scheduler_unit_test.py`:
- Around line 336-410: Strengthen both revoked-session tests around
_execute_copilot_turn by adding a positive caplog assertion that confirms the
expected authorization or tenancy-resolution failure was logged or swallowed.
Keep the existing not-awaited assertions, and ensure the assertion distinguishes
the SessionOrgMembershipRevoked failure from unrelated early exits.
In `@autogpt_platform/backend/backend/executor/scheduler.py`:
- Around line 256-316: Extract the organization_id/team_id derivation and
resolve_session_tenancy call from _execute_copilot_turn into a named helper such
as _resolve_scheduled_tenancy(args, target_session). Have it return the resolved
tenancy tuple while preserving existing session metadata, fallback, and
no-organization behavior; replace the inline block with the helper call so
_execute_copilot_turn remains under the function-length guideline.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 33f9858d-37c2-49cd-ac43-5ce03d1a4bac
📒 Files selected for processing (23)
autogpt_platform/backend/backend/api/features/chat/routes.pyautogpt_platform/backend/backend/api/features/chat/routes_test.pyautogpt_platform/backend/backend/api/features/orgs/db.pyautogpt_platform/backend/backend/api/features/orgs/model.pyautogpt_platform/backend/backend/blocks/autopilot.pyautogpt_platform/backend/backend/blocks/autopilot_permissions_test.pyautogpt_platform/backend/backend/blocks/test/test_autopilot.pyautogpt_platform/backend/backend/copilot/executor/processor.pyautogpt_platform/backend/backend/copilot/executor/processor_test.pyautogpt_platform/backend/backend/copilot/sdk/session_waiter.pyautogpt_platform/backend/backend/copilot/sdk/session_waiter_test.pyautogpt_platform/backend/backend/copilot/session_tenancy.pyautogpt_platform/backend/backend/copilot/session_tenancy_test.pyautogpt_platform/backend/backend/copilot/tools/run_sub_session.pyautogpt_platform/backend/backend/copilot/tools/schedule_followup.pyautogpt_platform/backend/backend/copilot/tools/schedule_followup_test.pyautogpt_platform/backend/backend/copilot/tools/sub_session_test.pyautogpt_platform/backend/backend/copilot/turn_queue.pyautogpt_platform/backend/backend/copilot/turn_queue_test.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/executor/scheduler.pyautogpt_platform/backend/backend/executor/scheduler_unit_test.pyautogpt_platform/frontend/src/app/api/openapi.json
🚧 Files skipped from review as they are similar to previous changes (4)
- autogpt_platform/backend/backend/copilot/session_tenancy_test.py
- autogpt_platform/backend/backend/api/features/orgs/model.py
- autogpt_platform/backend/backend/data/db_manager.py
- autogpt_platform/backend/backend/copilot/session_tenancy.py
| if turn_in_flight and ( | ||
| session.organization_id is None | ||
| or (session.team_id is not None and turn_team_id is None) | ||
| ): | ||
| raise HTTPException( | ||
| status_code=409, | ||
| detail=SESSION_TEAM_REVOKED_PENDING_DETAIL, | ||
| ) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not reject pending messages for untagged sessions.
Line 1452 and Line 1808 return 409 for every active session where organization_id is None. This blocks pending follow-ups for legacy untagged and personal sessions. Keep the 409 only for an organization-scoped session whose persisted team is stale. Add regression tests for both endpoints.
autogpt_platform/backend/backend/api/features/chat/routes.py#L1451-L1459: remove thesession.organization_id is Nonecondition.autogpt_platform/backend/backend/api/features/chat/routes.py#L1808-L1814: remove the equivalentsession.organization_id is Nonecondition.
Proposed fix
- if turn_in_flight and (
- session.organization_id is None
- or (session.team_id is not None and turn_team_id is None)
- ):
+ if (
+ turn_in_flight
+ and session.organization_id is not None
+ and session.team_id is not None
+ and turn_team_id is None
+ ):- if session.organization_id is None or (
- session.team_id is not None and pending_team_id is None
- ):
+ if (
+ session.organization_id is not None
+ and session.team_id is not None
+ and pending_team_id is None
+ ):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if turn_in_flight and ( | |
| session.organization_id is None | |
| or (session.team_id is not None and turn_team_id is None) | |
| ): | |
| raise HTTPException( | |
| status_code=409, | |
| detail=SESSION_TEAM_REVOKED_PENDING_DETAIL, | |
| ) | |
| if ( | |
| turn_in_flight | |
| and session.organization_id is not None | |
| and session.team_id is not None | |
| and turn_team_id is None | |
| ): | |
| raise HTTPException( | |
| status_code=409, | |
| detail=SESSION_TEAM_REVOKED_PENDING_DETAIL, | |
| ) |
| if turn_in_flight and ( | |
| session.organization_id is None | |
| or (session.team_id is not None and turn_team_id is None) | |
| ): | |
| raise HTTPException( | |
| status_code=409, | |
| detail=SESSION_TEAM_REVOKED_PENDING_DETAIL, | |
| ) | |
| if ( | |
| session.organization_id is not None | |
| and session.team_id is not None | |
| and pending_team_id is None | |
| ): | |
| raise HTTPException( | |
| status_code=409, | |
| detail=SESSION_TEAM_REVOKED_PENDING_DETAIL, | |
| ) |
📍 Affects 1 file
autogpt_platform/backend/backend/api/features/chat/routes.py#L1451-L1459(this comment)autogpt_platform/backend/backend/api/features/chat/routes.py#L1808-L1814
🤖 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 `@autogpt_platform/backend/backend/api/features/chat/routes.py` around lines
1451 - 1459, Update both pending-message checks in chat routes.py at lines
1451-1459 and 1808-1814 to remove the session.organization_id is None condition,
so untagged and personal sessions are not rejected. Retain the 409 response only
when an organization-scoped session has a persisted team while turn_team_id is
missing, and add regression tests covering both endpoints.
| # A legacy row has no persisted tenant anchor. Its server-produced caller | ||
| # context is required so the processor can revalidate it before execution. | ||
| if persisted_session.organization_id is None and organization_id is None: | ||
| raise SessionAdmissionError("session_tenancy_unverifiable") | ||
| if organization_id is None: | ||
| team_id = None |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Queued turn admission treats caller-supplied tenancy as authoritative instead of the persisted session row. The in-flight branch resolves tenancy from persisted_session, but the idle branch forwards the caller's organization_id and team_id straight to schedule_turn. Callers that pass a different organization, or None, therefore override the session's own tenancy.
autogpt_platform/backend/backend/copilot/sdk/session_waiter.py#L253-L258: anchor onpersisted_session.organization_idandpersisted_session.team_idwhen they are set, and raisesession_tenancy_unverifiableonly when both the persisted row and the caller supply no organization.autogpt_platform/backend/backend/copilot/tools/run_sub_session.py#L186-L187: for the resume branch, pass the tenancy of the resumedownedsession rather than the parentsession, or rely on the corrected anchoring inrun_copilot_turn_via_queue.
If CoPilotProcessor._execute_async already re-derives tenancy from the persisted row before execution, document that contract at both sites instead of changing behavior.
📍 Affects 2 files
autogpt_platform/backend/backend/copilot/sdk/session_waiter.py#L253-L258(this comment)autogpt_platform/backend/backend/copilot/tools/run_sub_session.py#L186-L187
2 participants
Why / What / How
A Copilot session persists its organization and team, but membership can change after session creation. Without a final DB-backed check at every admission and execution boundary, an offboarded user can keep using queued, resumed, scheduled, or internally injected turns under stale organization billing and permissions.
This PR makes the persisted session organization authoritative and revalidates active membership before work can reach either Copilot engine. Organization revocation hard-denies. Team removal within an active organization rehomes new turns to organization-home; it does not silently move already-running work whose capabilities may have been established under the former team.
Changes 🏗️
run_sub_sessionand AutoPilot pending-message injection with the existing turn-in-flight compare-and-set boundary.Product behavior:
Checklist 📋
For code changes:
Example test plan
For configuration changes:
.env.defaultis updated or already compatible with my changesdocker-compose.ymlis updated or already compatible with my changesExamples of configuration changes
Note
High Risk
Touches authorization and tenancy across chat dispatch, queue promotion, executor admission, and scheduled turns—security-critical paths that control org-scoped spend and agent execution.
Overview
Closes SECRT-2489 by re-verifying a chat session's persisted org/team on every turn, instead of trusting membership only at session creation.
Adds shared
resolve_session_tenancypolicy: org revocation is a hard deny (HTTP 403 / fail-closed elsewhere); stale team membership is soft-stripped to org-home. Wires that check across HTTP/streamand pending-message handlers, queue promotion, the CoPilot processor (authoritative DB metadata before execution), sub-session/AutoPilot admission, and scheduled followups.Also propagates resolved
organization_id/team_idthrough turn enqueue/recovery paths, rejects unverifiable pending injection into in-flight turns (409), and drops revoked queued sessions so they cannot block later valid work.Reviewed by Cursor Bugbot for commit d1632a8. Bugbot is set up for automated code reviews on this repo. Configure here.