feat(backend): accept explicit team_id on create/save flows - #13540
feat(backend): accept explicit team_id on create/save flows#13540ntindle wants to merge 2 commits into
Conversation
The frontend retired the active-team header (X-Team-Id is always null),
so builder save, schedule create and API key create silently stamped
every new resource org-home. Add an explicit, membership-validated
team_id to those create/save surfaces:
- POST /graphs and PUT /graphs/{id}: team_id in the CreateGraph body /
query param. On a new version, inherit the agent's existing team when
omitted so re-saving no longer moves a team agent back to org-home.
- POST /graphs/{id}/schedules: team_id in ScheduleCreationRequest,
inheriting the scheduled agent's team when omitted.
- POST /api-keys: team_id in CreateAPIKeyRequest, mapped to
teamIdRestriction (the enforced team scope for a key).
Creating into a team requires ACTIVE membership (org admins must join
first); an invalid or cross-org team returns 400. Regenerated openapi.json.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jm3mCG9okfdGtAXtFaDF9A
github-project-automation
Bot
moved this to 🆕 Needs initial review
in AutoGPT development kanban
github-actions
Bot
added
platform/frontend
WalkthroughOptional ChangesTeam-scoped writes
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant MembershipValidation
participant GraphOrScheduler
Client->>API: submit write with optional team_id
API->>MembershipValidation: validate active membership
MembershipValidation-->>API: resolved team scope
API->>GraphOrScheduler: persist or schedule with resolved scope
GraphOrScheduler-->>Client: response with operation status
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 |
🔍 PR Overlap DetectionThis check compares your PR against all other open PRs targeting the same branch to detect potential merge conflicts early. 🟢 Low Risk — File Overlap OnlyThese PRs touch the same files but different sections (click to expand)
Summary: 0 conflict(s), 0 medium risk, 4 low risk (out of 4 PRs with file overlap) Auto-generated on push. Ignores: |
![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
🤖 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/v1_test.py`:
- Around line 1020-1280: Add coverage for schedule creation in the existing
team-resolution tests: add tests for the POST /graphs/{id}/schedules flow
verifying that a membership-validated explicit team_id is forwarded to
add_execution_schedule, an omitted team_id inherits graph.team_id, and an
invalid team_id returns 400 without invoking the scheduler. Reuse the existing
get_user_team_ids mocking and schedule request fixtures/helpers, targeting the
schedule creation handler and add_execution_schedule mock.
🪄 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: cd26a34e-4764-4dc6-9aa8-346281565ce6
📒 Files selected for processing (4)
autogpt_platform/backend/backend/api/features/v1.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/model.pyautogpt_platform/frontend/src/app/api/openapi.json
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: check API types
- GitHub Check: lint
- GitHub Check: integration_test
- GitHub Check: end-to-end tests
- GitHub Check: type-check (3.13)
- GitHub Check: Check PR Status
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
- GitHub Check: type-check (3.11)
- GitHub Check: test (3.11)
- GitHub Check: type-check (3.12)
🧰 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/api/model.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/model.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/model.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/api/features/v1_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/api/model.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/model.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/model.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/model.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/model.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/model.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/model.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/model.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/model.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/model.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/model.pyautogpt_platform/backend/backend/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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
🔇 Additional comments (4)
autogpt_platform/backend/backend/api/features/v1.py (1)
1683-1712: LGTM!Also applies to: 1724-1751, 1791-1839, 2450-2511, 2844-2857
autogpt_platform/backend/backend/api/model.py (1)
46-52: LGTM!Also applies to: 59-66
autogpt_platform/backend/backend/api/features/v1_test.py (1)
1020-1062: LGTM!Also applies to: 1064-1145, 1147-1207, 1209-1280
autogpt_platform/frontend/src/app/api/openapi.json (1)
5562-5572: LGTM!Also applies to: 15125-15129, 15168-15172, 20954-20958
| # --------------------------------------------------------------------------- | ||
| # Team-picker on create/save flows (SECRT-2462) | ||
| # | ||
| # The frontend retired the active-team header (X-Team-Id is now always null), | ||
| # so create/save endpoints must accept an explicit team_id in the request | ||
| # instead of silently stamping from the header. These tests lock in the | ||
| # resolution: an explicit, membership-validated team wins; an invalid team is | ||
| # rejected; and omitting it preserves each surface's safe default. | ||
| # --------------------------------------------------------------------------- | ||
| TEAM_ID = "team-abc" | ||
| _GRAPH_BODY = {"name": "Test Graph", "description": "Test", "nodes": [], "links": []} | ||
|
|
||
|
|
||
| def _saved_graph_model(user_id: str) -> GraphModel: | ||
| return GraphModel( | ||
| id="graph-123", | ||
| version=1, | ||
| is_active=True, | ||
| name="Test Graph", | ||
| description="A test graph", | ||
| user_id=user_id, | ||
| created_at=datetime(2025, 9, 4, 13, 37), | ||
| ) | ||
|
|
||
|
|
||
| def _mock_graph_create_pipeline(mocker: pytest_mock.MockFixture, saved: GraphModel): | ||
| """Stub the create pipeline so only team stamping is under test.""" | ||
| mocker.patch( | ||
| "backend.api.features.v1.graph_db.make_graph_model", return_value=Mock() | ||
| ) | ||
| mocker.patch( | ||
| "backend.api.features.v1.before_graph_activate", | ||
| new=AsyncMock(return_value=saved), | ||
| ) | ||
| create_graph_mock = mocker.patch( | ||
| "backend.api.features.v1.graph_db.create_graph", | ||
| new=AsyncMock(return_value=saved), | ||
| ) | ||
| create_lib_agent_mock = mocker.patch( | ||
| "backend.api.features.v1.library_db.create_library_agent", new=AsyncMock() | ||
| ) | ||
| return create_graph_mock, create_lib_agent_mock | ||
|
|
||
|
|
||
| def test_create_new_graph_stamps_explicit_team( | ||
| mocker: pytest_mock.MockFixture, test_user_id: str | ||
| ) -> None: | ||
| """A valid team_id in the body lands the agent (and its library entry) | ||
| under that team.""" | ||
| create_graph_mock, create_lib_agent_mock = _mock_graph_create_pipeline( | ||
| mocker, _saved_graph_model(test_user_id) | ||
| ) | ||
| mocker.patch( | ||
| "backend.api.features.v1.get_user_team_ids", | ||
| new=AsyncMock(return_value=[TEAM_ID]), | ||
| ) | ||
|
|
||
| response = client.post("/graphs", json={"graph": _GRAPH_BODY, "team_id": TEAM_ID}) | ||
|
|
||
| assert response.status_code == 200 | ||
| assert create_graph_mock.await_args.kwargs["team_id"] == TEAM_ID | ||
| assert create_lib_agent_mock.await_args.kwargs["team_id"] == TEAM_ID | ||
|
|
||
|
|
||
| def test_create_new_graph_omitted_team_lands_org_home( | ||
| mocker: pytest_mock.MockFixture, test_user_id: str | ||
| ) -> None: | ||
| """No team_id -> org-home (teamId NULL), and no membership lookup happens.""" | ||
| create_graph_mock, create_lib_agent_mock = _mock_graph_create_pipeline( | ||
| mocker, _saved_graph_model(test_user_id) | ||
| ) | ||
| get_teams = mocker.patch( | ||
| "backend.api.features.v1.get_user_team_ids", | ||
| new=AsyncMock(return_value=[TEAM_ID]), | ||
| ) | ||
|
|
||
| response = client.post("/graphs", json={"graph": _GRAPH_BODY}) | ||
|
|
||
| assert response.status_code == 200 | ||
| assert create_graph_mock.await_args.kwargs["team_id"] is None | ||
| assert create_lib_agent_mock.await_args.kwargs["team_id"] is None | ||
| get_teams.assert_not_awaited() | ||
|
|
||
|
|
||
| def test_create_new_graph_rejects_non_member_team( | ||
| mocker: pytest_mock.MockFixture, test_user_id: str | ||
| ) -> None: | ||
| """A team the caller isn't an active member of -> 400, nothing persisted.""" | ||
| create_graph_mock, _ = _mock_graph_create_pipeline( | ||
| mocker, _saved_graph_model(test_user_id) | ||
| ) | ||
| mocker.patch( | ||
| "backend.api.features.v1.get_user_team_ids", | ||
| new=AsyncMock(return_value=[]), | ||
| ) | ||
|
|
||
| response = client.post("/graphs", json={"graph": _GRAPH_BODY, "team_id": TEAM_ID}) | ||
|
|
||
| assert response.status_code == 400 | ||
| assert "active member" in response.json()["detail"] | ||
| create_graph_mock.assert_not_awaited() | ||
|
|
||
|
|
||
| def test_create_new_graph_rejects_cross_org_team( | ||
| mocker: pytest_mock.MockFixture, test_user_id: str | ||
| ) -> None: | ||
| """A team in another org never appears in the caller's team list -> 400. | ||
|
|
||
| get_user_team_ids only returns teams within the caller's org, so a | ||
| cross-org team is indistinguishable from a non-member team here — both | ||
| reject. We assert the org-scoped list can't be bypassed by a foreign id.""" | ||
| create_graph_mock, _ = _mock_graph_create_pipeline( | ||
| mocker, _saved_graph_model(test_user_id) | ||
| ) | ||
| mocker.patch( | ||
| "backend.api.features.v1.get_user_team_ids", | ||
| new=AsyncMock(return_value=["team-in-my-org"]), | ||
| ) | ||
|
|
||
| response = client.post( | ||
| "/graphs", json={"graph": _GRAPH_BODY, "team_id": "team-in-another-org"} | ||
| ) | ||
|
|
||
| assert response.status_code == 400 | ||
| create_graph_mock.assert_not_awaited() | ||
|
|
||
|
|
||
| def _mock_update_graph_pipeline( | ||
| mocker: pytest_mock.MockFixture, existing: Mock, saved: GraphModel | ||
| ): | ||
| mocker.patch( | ||
| "backend.api.features.v1.graph_db.get_graph_all_versions", | ||
| new=AsyncMock(return_value=[existing]), | ||
| ) | ||
| mocker.patch( | ||
| "backend.api.features.v1.graph_db.make_graph_model", | ||
| return_value=Mock(is_active=False), | ||
| ) | ||
| create_graph_mock = mocker.patch( | ||
| "backend.api.features.v1.graph_db.create_graph", | ||
| new=AsyncMock( | ||
| return_value=Mock(version=2, is_active=False, webhook_input_node=None) | ||
| ), | ||
| ) | ||
| mocker.patch( | ||
| "backend.api.features.v1.graph_db.get_graph", | ||
| new=AsyncMock(return_value=saved), | ||
| ) | ||
| return create_graph_mock | ||
|
|
||
|
|
||
| def test_update_graph_inherits_existing_team_when_omitted( | ||
| mocker: pytest_mock.MockFixture, test_user_id: str | ||
| ) -> None: | ||
| """Regression (SECRT-2462): re-saving a team agent must keep its team, not | ||
| reset it to org-home now that the active-team header is always null.""" | ||
| existing = Mock(version=1, is_active=True, team_id="team-existing") | ||
| create_graph_mock = _mock_update_graph_pipeline( | ||
| mocker, existing, _saved_graph_model(test_user_id) | ||
| ) | ||
|
|
||
| response = client.put("/graphs/graph-123", json={"id": "graph-123", **_GRAPH_BODY}) | ||
|
|
||
| assert response.status_code == 200 | ||
| assert create_graph_mock.await_args.kwargs["team_id"] == "team-existing" | ||
|
|
||
|
|
||
| def test_update_graph_moves_to_explicit_team( | ||
| mocker: pytest_mock.MockFixture, test_user_id: str | ||
| ) -> None: | ||
| """An explicit team_id query param saves the new version into that team.""" | ||
| existing = Mock(version=1, is_active=True, team_id="team-existing") | ||
| create_graph_mock = _mock_update_graph_pipeline( | ||
| mocker, existing, _saved_graph_model(test_user_id) | ||
| ) | ||
| mocker.patch( | ||
| "backend.api.features.v1.get_user_team_ids", | ||
| new=AsyncMock(return_value=["team-new"]), | ||
| ) | ||
|
|
||
| response = client.put( | ||
| "/graphs/graph-123?team_id=team-new", | ||
| json={"id": "graph-123", **_GRAPH_BODY}, | ||
| ) | ||
|
|
||
| assert response.status_code == 200 | ||
| assert create_graph_mock.await_args.kwargs["team_id"] == "team-new" | ||
|
|
||
|
|
||
| def _api_key_info(user_id: str): | ||
| from prisma.enums import APIKeyStatus | ||
|
|
||
| from backend.data.auth.api_key import APIKeyInfo | ||
|
|
||
| return APIKeyInfo( | ||
| id="key-1", | ||
| name="k", | ||
| head="agpt_xxxx", | ||
| tail="yyyy", | ||
| status=APIKeyStatus.ACTIVE, | ||
| scopes=[], | ||
| user_id=user_id, | ||
| created_at=datetime(2025, 9, 4, 13, 37), | ||
| ) | ||
|
|
||
|
|
||
| def test_create_api_key_stamps_team_restriction( | ||
| mocker: pytest_mock.MockFixture, test_user_id: str | ||
| ) -> None: | ||
| """A valid team_id pins the new key to that team (teamIdRestriction).""" | ||
| create_mock = mocker.patch( | ||
| "backend.api.features.v1.api_key_db.create_api_key", | ||
| new=AsyncMock(return_value=(_api_key_info(test_user_id), "agpt_plaintext")), | ||
| ) | ||
| mocker.patch( | ||
| "backend.api.features.v1.get_user_team_ids", | ||
| new=AsyncMock(return_value=[TEAM_ID]), | ||
| ) | ||
|
|
||
| response = client.post( | ||
| "/api-keys", json={"name": "k", "permissions": [], "team_id": TEAM_ID} | ||
| ) | ||
|
|
||
| assert response.status_code == 200 | ||
| assert create_mock.await_args.kwargs["team_id_restriction"] == TEAM_ID | ||
|
|
||
|
|
||
| def test_create_api_key_omitted_team_is_unrestricted( | ||
| mocker: pytest_mock.MockFixture, test_user_id: str | ||
| ) -> None: | ||
| """No team_id -> an org-wide (unrestricted) key, today's behavior.""" | ||
| create_mock = mocker.patch( | ||
| "backend.api.features.v1.api_key_db.create_api_key", | ||
| new=AsyncMock(return_value=(_api_key_info(test_user_id), "agpt_plaintext")), | ||
| ) | ||
|
|
||
| response = client.post("/api-keys", json={"name": "k", "permissions": []}) | ||
|
|
||
| assert response.status_code == 200 | ||
| assert create_mock.await_args.kwargs["team_id_restriction"] is None | ||
|
|
||
|
|
||
| def test_create_api_key_rejects_non_member_team( | ||
| mocker: pytest_mock.MockFixture, | ||
| ) -> None: | ||
| """Scoping a key to a team the caller isn't in -> 400, no key minted.""" | ||
| create_mock = mocker.patch( | ||
| "backend.api.features.v1.api_key_db.create_api_key", new=AsyncMock() | ||
| ) | ||
| mocker.patch( | ||
| "backend.api.features.v1.get_user_team_ids", | ||
| new=AsyncMock(return_value=[]), | ||
| ) | ||
|
|
||
| response = client.post( | ||
| "/api-keys", json={"name": "k", "permissions": [], "team_id": TEAM_ID} | ||
| ) | ||
|
|
||
| assert response.status_code == 400 | ||
| create_mock.assert_not_awaited() | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add tests for schedule creation team resolution.
The schedule creation flow (POST /graphs/{id}/schedules) accepts team_id in ScheduleCreationRequest and resolves it via the same _resolve_write_team_id helper (v1.py lines 2483-2493), but has zero test coverage. The inheritance path (resolved_team_id = graph.team_id) is unique to schedules and differs from the graph update inheritance path. Since schedules auto-execute graphs under the resolved team, this is security-sensitive.
Suggested tests:
- Explicit valid
team_id→ forwarded toadd_execution_scheduleasteam_id. - Omitted
team_id→ inheritsgraph.team_id. - Invalid
team_id→ returns 400, scheduler not called.
Would you like me to generate these tests?
🤖 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/v1_test.py` around lines 1020 -
1280, Add coverage for schedule creation in the existing team-resolution tests:
add tests for the POST /graphs/{id}/schedules flow verifying that a
membership-validated explicit team_id is forwarded to add_execution_schedule, an
omitted team_id inherits graph.team_id, and an invalid team_id returns 400
without invoking the scheduler. Reuse the existing get_user_team_ids mocking and
schedule request fixtures/helpers, targeting the schedule creation handler and
add_execution_schedule mock.
|
/batch |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #13540 +/- ##
==========================================
- Coverage 76.92% 76.92% -0.01%
==========================================
Files 2761 2761
Lines 209521 209619 +98
Branches 20077 20173 +96
==========================================
+ Hits 161171 161239 +68
+ Misses 43981 43940 -41
- Partials 4369 4440 +71
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ntindle
added
the
batch
…ent — union: explicit body param wins, ctx.team_id fallback Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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. |
…cker Resolve v1.py update_graph: keep team_id Query param + adopt dev's UpdateGraphResponse return type (same as team-field-alignment). v1_test + orgs routes pass against Better Auth. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NBwmh7CkHiF8vKLBf2GWTU
|
Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly. |
github-actions
Bot
removed
the
conflicts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
autogpt_platform/backend/backend/api/features/v1.py (1)
1859-1864: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not use the legacy context team when
team_idis omitted.This branch silently changes an existing graph’s tenant whenever
ctx.team_idis set, and bypasses_resolve_write_team_id. Omittedteam_idmust inherit the active/existing version’steam_id.Proposed fix
if team_id is not None: resolved_team_id = await _resolve_write_team_id(user_id, ctx.org_id, team_id) -elif ctx.team_id is not None: - resolved_team_id = ctx.team_id else: resolved_team_id = (current_active_version or existing_versions[0]).team_id🤖 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/v1.py` around lines 1859 - 1864, Update the team ID selection logic around _resolve_write_team_id so an omitted team_id never falls back to ctx.team_id. When team_id is absent, always inherit the team_id from current_active_version or existing_versions[0], preserving explicit team_id resolution through _resolve_write_team_id.
🤖 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.
Outside diff comments:
In `@autogpt_platform/backend/backend/api/features/v1.py`:
- Around line 1859-1864: Update the team ID selection logic around
_resolve_write_team_id so an omitted team_id never falls back to ctx.team_id.
When team_id is absent, always inherit the team_id from current_active_version
or existing_versions[0], preserving explicit team_id resolution through
_resolve_write_team_id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3eb73a8c-9806-44ef-a1a7-69f7133a14d1
📒 Files selected for processing (2)
autogpt_platform/backend/backend/api/features/v1.pyautogpt_platform/backend/backend/api/features/v1_test.py
📜 Review details
⏰ Context from checks skipped due to timeout. (15)
- GitHub Check: integration_test
- GitHub Check: lint
- GitHub Check: check API types
- GitHub Check: Seer Code Review
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: type-check (3.13)
- GitHub Check: Check PR Status
- GitHub Check: type-check (3.11)
- GitHub Check: test (3.13)
- GitHub Check: end-to-end tests
- GitHub Check: type-check (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (typescript)
- GitHub Check: copilot-setup-steps
🧰 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/api/features/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/api/features/v1_test.py
🧠 Learnings (11)
📚 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/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/v1_test.pyautogpt_platform/backend/backend/api/features/v1.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/v1_test.pyautogpt_platform/backend/backend/api/features/v1.py
🪛 Ruff (0.16.0)
autogpt_platform/backend/backend/api/features/v1.py
[warning] 210-210: Do not perform function call Security in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (1)
autogpt_platform/backend/backend/api/features/v1_test.py (1)
120-139: 📐 Maintainability & Code Quality | ⚡ Quick winSnapshot the created-user response body.
This path only checks the header; add a response snapshot to cover the created-user payload as well. As per coding guidelines, “Use pytest with snapshot testing for API responses.”
[ suggest_recommended_refactor ]
Source: Coding guidelines
|
/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 #13540
PR #13540 — feat(backend): accept explicit team_id on create/save flows
Author: ntindle | Files: 4
🎯 Verdict: APPROVE
PR Description Quality
✅ Has Why + What + How — clearly frames the re-save bug (SECRT-2462), the four write surfaces touched, and the validation rule. Author's checklist is fully filled with a concrete test plan.
What This PR Does
Previously, saving an agent from the builder (which no longer sends team context) silently reset a team-owned agent back to org-home, because the write path defaulted a missing team_id to NULL. This PR lets all four create/save surfaces (POST /graphs, PUT /graphs/{id}, POST /graphs/{id}/schedules, POST /api-keys) accept an explicit team_id, validates it through a single shared helper (_resolve_write_team_id) that requires the caller to be an ACTIVE member of a team in their org, and — on update/schedule — inherits the existing tenant when team_id is omitted so a re-save no longer drops the agent to org-home.
Specialist Findings
🛡️ Security ✅ — Traced the full trust chain. _resolve_write_team_id delegates to get_user_team_ids, which filters on both status: "ACTIVE" and Team.orgId == organization_id, so non-member, stale, and cross-org teams all fail closed with a 400 before any persistence. Net security improvement over the retired header. One 🟡 defense-in-depth note: the inherit path (v1.py:1864, :2543) doesn't re-check the inherited team_id against the current org, so a multi-org user switching X-Org-Id between saves could produce a version whose organization_id and team_id belong to different orgs. Self-owned resource only, no cross-user exposure.
🏗️ Architecture ✅ — Clean, tightly-scoped. Single enforcement point reused across all four surfaces; data layer untouched, so blast radius is just route stamping. 🔵 Notes: team_id is a query param on PUT /graphs/{id} but a body field elsewhere (v1.py:1833); comment at v1.py:1854 uses change-relative "the key fix / now always absent" language that won't age well post-merge.
⚡ Performance ✅ — Adds exactly one bounded, index-backed membership query per write, and only when a client supplies team_id (the common omitted-path skips it entirely, verified by test). O(1) extra round trips, no N+1. 🟡 Optional micro-opt (tenancy.py:26): a targeted find_first existence check would avoid fetching the full membership set, immaterial at real team cardinality.
🧪 Testing create_graph_execution_schedule gained new team-resolution logic with zero tests (v1.py:2534), and update_graph lacks a non-member 400 test on its query-param wiring plus a no-active-version fallback test (v1.py:1863). (Flagged by: testing, discussion — 2 specialists.)
📖 Quality ✅ — Grade A: clear naming, good docstrings, tests carry intent docstrings. 🔵 The explicit→context→inherit ladder is copy-pasted across three sites (v1.py:1857, ~2534, ~1766) and could drift; str | None vs Optional[str] style is mixed within the PR.
📦 Product ✅ — Backend slice matches requirements exactly. 🟡 Forward-looking gaps for the follow-up frontend picker: team_id=None is overloaded to mean "inherit" on update, so there's no wire value meaning "move a team agent back to org-home" (v1.py:1858); and the query-vs-body inconsistency will force the picker to special-case PUT.
📬 Discussion 737cdc3; no merge conflicts (MERGEABLE). Two open CodeRabbit (Major) threads with no author reply: the missing schedule-flow team tests, and the elif ctx.team_id is not None fallback (documented by the author as dead "now always-null" back-compat). No human approval yet; author self-triggered a fresh /review on 2026-08-06 that was still pending.
🔎 QA ✅ — Exercised all four surfaces live against the running stack with before/after DB verification. 12/12 scenarios Actual = Expected: explicit team stamped on both AgentGraph and LibraryAgent; invalid/non-member/cross-org → 400 with nothing persisted; unauthenticated → 401. The headline re-save fix was confirmed against live DB — re-saving with no team context produced v2.teamId = 8f7842f5… (inherited) instead of resetting to NULL.
🟠 Should Fix
- Add schedule-endpoint team-resolution tests (
v1.py:2534) — the schedule surface gained explicit-validate / omit-inherit / reject logic with no tests; this is the exact silent-misplacement class of bug the PR fixes for graphs. Add: explicit-valid stamp, omitted→graph.team_id, non-member→400. (Flagged by: testing, discussion — 2 specialists.) - Add
update_graphrejection + fallback tests (v1.py:1833,:1863) — no test covers a non-member?team_id=returning 400 on the query-param path, and theexisting_versions[0](no-active-version) inherit branch is never exercised. (Flagged by: testing.)
🟡 Nice to Have
- Re-validate inherited
team_idagainst current org on inherit path (v1.py:1864,:2543) — defense-in-depth against org/team mismatch for multi-org users. (security) - Resolve the "move back to org-home" gap and query-vs-body shape before the frontend picker locks the contract (
v1.py:1858,:1833) — cheaper to decide now than a breaking change later. (product, architect — 2 specialists) - Targeted membership existence check (
tenancy.py:26) instead of fetch-all-and-scan. (performance)
🔵 Nits
- Non-durable diff-relative comment (
v1.py:1854) — rewrite as a standing invariant. (architect) - Mixed
str | NonevsOptional[str]within the PR (v1.py:1725). (quality) - Collapse the duplicated resolution ladder into the helper (
v1.py:1766,1857,2534). (quality, architect — 2 specialists)
QA Screenshots
| Screenshot | Description |
|---|---|
 |
Authenticated app renders after test run ✅ (backend-only PR; no UI in this diff) |
Human Review Needed
YES — this modifies an authorization/tenancy trust boundary (which teams a caller may stamp resources into); a maintainer familiar with the org/team model should eyeball the inherit-path org-consistency note before merge.
Risk Assessment
Merge risk: LOW | Rollback: EASY — additive, backward-compatible (all new fields Optional = None), data layer untouched, fail-closed on invalid input.
CI Status
GitHub CI: per the discussion specialist, 47/47 required checks were green at head 737cdc3 (not independently re-fetched this run). Local harness: backend lint ✅, frontend lint ✅, typecheck ✅, build ✅; the frontend pnpm test:unit suite failed in the sandbox — this is a frontend suite unrelated to a backend-only diff and reflects environment skew, not a regression from this PR.
UI Testing — Variant Results
✅ local: All four create/save surfaces correctly validate and stamp explicit team_id, reject invalid/cross-org teams with 400, and the PUT re-save bug fix (inherit existing tenant) is verified against live DB state.
✅ hosted: All four team_id surfaces (graph create, graph save, schedule, api-key) resolve tenancy correctly and reject non-member teams; the re-save inheritance regression fix is confirmed live in the DB.
| elif ctx.team_id is not None: | ||
| resolved_team_id = ctx.team_id | ||
| else: | ||
| resolved_team_id = (current_active_version or existing_versions[0]).team_id |
There was a problem hiding this comment.
🤖 🟢 low (security/tenancy-consistency)
When team_id is omitted and no header team context is set, update_graph inherits the prior version's team_id without verifying it belongs to ctx.org_id. A multi-org user who switches X-Org-Id between saves can create a version whose organization_id and team_id belong to different orgs, which could confuse team-keyed access checks.
Suggestion: Validate the inherited team_id is in get_user_team_ids(user_id, ctx.org_id); if it isn't (org changed), clear to org-home (None) or reject, so organization_id and team_id always stay in the same org.
| elif ctx.team_id is not None: | ||
| resolved_team_id = ctx.team_id | ||
| else: | ||
| resolved_team_id = graph.team_id |
There was a problem hiding this comment.
🤖 🟢 low (security/tenancy-consistency)
create_graph_execution_schedule inherits graph.team_id with no check that the team belongs to the current ctx.org_id, so a schedule can be stamped with a team from a different org than the request's org context for a multi-org user.
Suggestion: Apply the same org-consistency check to the inherited graph.team_id as used on the explicit path (membership within ctx.org_id), else fall back to org-home.
| graph.version = max(g.version for g in existing_versions) + 1 | ||
| current_active_version = next((v for v in existing_versions if v.is_active), None) | ||
|
|
||
| # Team resolution for the new version: an explicit picker choice wins, |
There was a problem hiding this comment.
🤖 🟢 low (architect/comment-durability)
The update-graph resolution comment is diff-justification with change-relative language ('the key fix for the observed bug', 'team_id now always absent'). After merge it documents a bug that no longer exists and depends on knowing when the diff landed.
Suggestion: Rewrite to state the standing invariant only: explicit choice wins, then active-team context, else inherit the existing tenant so a team-owned agent stays in its team on re-save.
| graph: graph_db.Graph, | ||
| user_id: Annotated[str, Security(get_user_id)], | ||
| ctx: Annotated[RequestContext, Security(get_request_context)], | ||
| team_id: Annotated[ |
There was a problem hiding this comment.
🤖 🟢 low (architect/api-consistency)
team_id is a query param on PUT /graphs/{id} but a body field on create, schedule, and api-key surfaces. The frontend picker will have to special-case this one surface.
Suggestion: Prefer a consistent placement across the four write surfaces, or document the intentional divergence for the follow-up frontend slice.
| # otherwise inherit the agent's existing tenant. Inheriting is the key | ||
| # fix for the observed bug — without it, re-saving from the builder | ||
| # (team_id now always absent) silently moves a team agent to org-home. | ||
| if team_id is not None: |
There was a problem hiding this comment.
🤖 🟢 low (architect/design-asymmetry)
On create, omitted team_id means org-home; on update, omitted means inherit existing tenant. This correctly fixes the re-save bug but leaves no way to move a team-owned graph back to org-home via the API.
Suggestion: If demote-to-org-home is a needed action, plan a sentinel value or dedicated path; otherwise note the deferral explicitly.
| detail=f"Graph #{graph_id} v{schedule_params.graph_version} not found.", | ||
| ) | ||
|
|
||
| # A schedule is bound to a graph, so it inherits the graph's tenant unless |
There was a problem hiding this comment.
🤖 🟡 medium (product/ux-gap)
Schedule creation inherits the graph's team when team_id is omitted, with no way to create a schedule in org-home for a team-owned graph — same sentinel-overload limitation as PUT /graphs.
Suggestion: Apply the same explicit org-home sentinel decision here for consistency across surfaces.
| graph: graph_db.Graph, | ||
| user_id: Annotated[str, Security(get_user_id)], | ||
| ctx: Annotated[RequestContext, Security(get_request_context)], | ||
| team_id: Annotated[ |
There was a problem hiding this comment.
🤖 🟡 medium (product/api-consistency)
team_id is a query parameter on PUT /graphs/{id} but a request-body field on POST /graphs, POST /schedules, and POST /api-keys. The shared frontend picker must special-case this one surface.
Suggestion: Move team_id into the update request body for uniformity, or document explicitly why PUT must use a query param.
| # then the (validated) active-team context if a legacy header set it, | ||
| # otherwise inherit the agent's existing tenant. Inheriting is the key | ||
| # fix for the observed bug — without it, re-saving from the builder | ||
| # (team_id now always absent) silently moves a team agent to org-home. |
There was a problem hiding this comment.
🤖 🟢 low (product/backward-compat / docs)
The corrected PUT /graphs inherit-on-omit behavior is an observable API contract change for existing consumers that relied on the old reset-to-org-home behavior, but there is no changelog/migration note.
Suggestion: Add a release-note/changelog line documenting that re-saving a team-owned agent now preserves its team.
| resolved_team_id = await _resolve_write_team_id( | ||
| user_id, ctx.org_id, schedule_params.team_id | ||
| ) | ||
| elif ctx.team_id is not None: |
There was a problem hiding this comment.
🤖 🟡 medium (discussion/unaddressed-reviewer-concern / test-coverage)
CodeRabbit (Major) flagged that the POST /graphs/{id}/schedules team resolution has zero test coverage. The schedule-specific inheritance path (resolved_team_id = graph.team_id) is security-sensitive since schedules auto-execute under the resolved team, yet no test asserts explicit-team forwarding, omitted->graph.team_id inheritance, or 400 on invalid team. The PR description implies schedule scenarios are tested, but the diff only covers create/update/api-key. No author response.
Suggestion: Add the three suggested tests for the schedule flow (explicit valid team_id forwarded to add_execution_schedule, omitted inherits graph.team_id, invalid returns 400 without calling scheduler), or reply on the thread explaining the gap.
| # fix for the observed bug — without it, re-saving from the builder | ||
| # (team_id now always absent) silently moves a team agent to org-home. | ||
| if team_id is not None: | ||
| resolved_team_id = await _resolve_write_team_id(user_id, ctx.org_id, team_id) |
There was a problem hiding this comment.
🤖 🟡 medium (discussion/unaddressed-reviewer-concern)
CodeRabbit (Major, outside-diff) flagged that when team_id is omitted the code falls back to ctx.team_id (elif ctx.team_id is not None) before inheriting the existing/active version's team, bypassing _resolve_write_team_id validation. The same pattern appears in the schedule and create paths. The author's inline comment states ctx.team_id is 'now always-null' back-compat, but the reviewer's point that this branch is unvalidated and could silently re-stamp tenant was never acknowledged or resolved.
Suggestion: Either remove the dead ctx.team_id fallback so omitted team_id always inherits the existing tenant, or reply on the thread confirming why the back-compat branch is safe to keep.
github-actions
Bot
added
cla: signed
|
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
1 participant
Why / What / How
Why: The badge/filter model retired the active-team header, so every created resource now lands org-home — the builder gives no way to choose a team (observed on the rollup preview, SECRT-2462). Worse, a latent bug:
teamIdis stored per graph version, so re-saving a team-owned agent with no team context silently moved it back to org-home.What: Create/save endpoints accept an explicit
team_id, validated as "caller is an ACTIVE member of a team in their org" (org admins are not exempt — creating into a team requires membership; join first):POST /graphs—team_idonCreateGraphPUT /graphs/{id}—team_idquery param; when omitted, the graph inherits its existing tenant instead of resetting to org-home (the re-save bug fix, with regression test)POST /graphs/{id}/schedules—team_idon the request; omitted → inherits the graph's tenantPOST /api-keys—team_id→teamIdRestriction(the enforced team dimension for keys; the ownershipteamIdcolumn is dead for user-owned keys)How: One shared
_resolve_write_team_idhelper over the existingget_user_team_ids(membership implies org-belonging in a single check; None → org-home; invalid/non-member/cross-org → 400). Explicit value wins over the (now always-null, back-compat) header context. Data layer unchanged — it already took the ids as params; only route stamping changed. OpenAPI regenerated.Frontend picker (shared component + builder save integration) is the follow-up slice on the org-UI chain.
Linear: SECRT-2462 (backend half)
Changes 🏗️
backend/api/model.py,backend/api/features/v1.py:team_idon the four surfaces +_resolve_write_team_idbackend/api/features/v1_test.py: +9 scenario tests (explicit stamp, omitted→NULL, omitted-on-update→inherit, non-member 400, cross-org 400, api-key restriction)frontend/src/app/api/openapi.json: regeneratedChecklist 📋
For code changes:
poetry run pytest backend/api/features/v1_test.py -q— 58 passed; orgs suite 233 passedpoetry run formatcleanFor changes touching
data/*.py: n/a — data layer untouched; team validation derives strictly from the caller's own ACTIVE memberships.For configuration changes: n/a
🤖 Generated with Claude Code
https://claude.ai/code/session_01Jm3mCG9okfdGtAXtFaDF9A
Note
Medium Risk
Changes tenancy stamping on graph versions, schedules, and API keys; wrong resolution could misplace resources, but membership validation and regression tests reduce that risk.
Overview
Adds explicit
team_idon graph create, graph save, schedule create, and API key create so tenancy no longer depends on the retiredX-Team-Idheader. A shared_resolve_write_team_idhelper checks org context and active team membership (org admins must still be members); invalid or cross-org teams return 400.PUT /graphs/{id}now inherits the agent’s existing team whenteam_idis omitted, fixing re-saves that silently moved team-owned agents to org-home. Create and schedule flows use explicitteam_idwhen provided, else legacy header context, else org-home or the graph’s team. API keys map optionalteam_idtoteamIdRestriction. OpenAPI and nine route tests cover stamping, inheritance, and rejection paths.Reviewed by Cursor Bugbot for commit 737cdc3. Bugbot is set up for automated code reviews on this repo. Configure here.