feat(backend): team-scoped credential creation + management (team admins) - #13641
feat(backend): team-scoped credential creation + management (team admins)#13641ntindle wants to merge 6 commits into
Conversation
…ins) Co-Authored-By: Claude Opus <noreply@anthropic.com>
github-project-automation
Bot
moved this to 🆕 Needs initial review
in AutoGPT development kanban
|
/batch |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughTeam-scoped credentials now support authorized creation, listing, and deletion. Storage applies organization, team, and active-status filters. Personal credential creation remains unchanged when no team is specified. ChangesTeam credential management
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant IntegrationRouter
participant TeamDb
participant ScopedCredentials
participant Prisma
Client->>IntegrationRouter: Request team credential operation
IntegrationRouter->>TeamDb: Check team membership
TeamDb->>Prisma: Query membership and team
Prisma-->>TeamDb: Authorization data
IntegrationRouter->>ScopedCredentials: Create, list, or revoke credential
ScopedCredentials->>Prisma: Persist or query scoped credential
Prisma-->>ScopedCredentials: Operation result
ScopedCredentials-->>IntegrationRouter: Credential metadata
IntegrationRouter-->>Client: API response
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 |
autogpt-batch-bot
Bot
added
the
batch
🔍 PR Overlap DetectionThis check compares your PR against all other open PRs targeting the same branch to detect potential merge conflicts early. 🔴 Merge Conflicts DetectedThe following PRs have been tested and will have merge conflicts if merged after this PR. Consider coordinating with the authors.
🟡 Medium Risk — Some Line OverlapThese PRs have some overlapping changes:
🟢 Low Risk — File Overlap OnlyThese PRs touch the same files but different sections (click to expand)
Summary: 1 conflict(s), 1 medium risk, 2 low risk (out of 4 PRs with file overlap) Auto-generated on push. Ignores: |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
autogpt_platform/backend/backend/integrations/scoped_credentials.py (1)
127-167: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winGuard the TEAM ownership invariant instead of relying on caller discipline.
The docstring states
owner_type="TEAM"+owner_id=<teamId>+team_id=<teamId>must stay in sync for the read path and theteamIdFK cascade-delete to work, but nothing enforces it. If a future caller passesowner_type="TEAM"withoutteam_id(or with a mismatched value), the row silently loses cascade cleanup and diverges from whatget_scoped_credentials/get_credential_by_idexpect.🛡️ Proposed guard
encrypted = _cryptor.encrypt(payload) + + if owner_type == "TEAM" and team_id != owner_id: + raise ValueError("team_id must equal owner_id for TEAM-owned credentials")🤖 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/integrations/scoped_credentials.py` around lines 127 - 167, Update create_credential to validate the TEAM ownership invariant before creating the credential: when owner_type is "TEAM", require team_id to be present and equal to owner_id, and reject missing or mismatched values. Preserve the existing creation flow for non-TEAM owners and only persist the row after validation succeeds.autogpt_platform/backend/backend/api/features/integrations/router.py (1)
445-470: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTeam-credential path doesn't reject OAuth-shaped credentials, despite OAuth being explicitly deferred for teams.
The PR defers team OAuth support because callback/merge logic is user-scoped, but
create_credentialsdoesn't stop a client from POSTing an OAuth2-typedCredentialsbody withteam_idset — it will be persisted via_create_team_credentialwith no way to ever refresh/merge it correctly.Consider adding a test asserting OAuth-typed bodies are rejected on the team path.🐛 Proposed guard
if team_id is not None: + if credentials.type != "api_key": + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail="Only api_key credentials are supported for team-owned credentials", + ) return await _create_team_credential(user_id, provider, credentials, 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/integrations/router.py` around lines 445 - 470, Update create_credentials so the team_id branch rejects OAuth2-typed credentials before calling _create_team_credential, returning the established client error for unsupported team OAuth credentials. Keep non-OAuth team credentials on the existing _create_team_credential path and leave personal credential handling unchanged; add coverage for an OAuth-shaped body with team_id.
🧹 Nitpick comments (2)
autogpt_platform/backend/backend/api/features/integrations/router.py (2)
606-633: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute handlers placed after the private helpers they depend on.
list_team_credentialsanddelete_team_credentialare the public endpoints in this section, but they're defined after_require_team_admin,_require_team_member, and_team_cred_meta_to_response, which they consume. As per coding guidelines: "Use top-down ordering — define the main/public function or class first, then the helpers it uses below."Move the two
@routerdecorated handlers above their private helpers (or move the helpers below), consistent with the ordering already used elsewhere in this file (e.g._cred_to_metadatainscoped_credentials.pysits after all the public functions that call it).🤖 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/integrations/router.py` around lines 606 - 633, Reorder the credential route section so the public handlers list_team_credentials and delete_team_credential appear before the private helpers they call: _require_team_member, _require_team_admin, and _team_cred_meta_to_response. Preserve each handler’s behavior and keep the helpers defined below the routes.Source: Coding guidelines
501-549: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate membership-lookup logic between
_require_team_adminand_require_team_member.Both functions issue the identical
prisma.teammember.find_unique(..., include={"Team": True})call and identical 404 (not-found/archived) branch, differing only in the extra admin/active gate. Extracting the shared lookup avoids the two authorization paths drifting out of sync.As per coding guidelines: "Prefer list comprehensions over manual loop-and-append patterns" reflects the broader repo emphasis on avoiding duplicated logic in backend Python files.♻️ Proposed refactor
+async def _get_active_team(user_id: str, team_id: str): + member = await prisma.teammember.find_unique( + where={"teamId_userId": {"teamId": team_id, "userId": user_id}}, + include={"Team": True}, + ) + if member is None or member.Team is None or member.Team.archivedAt is not None: + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, detail="Team not found" + ) + return member + + async def _require_team_admin(user_id: str, team_id: str) -> str: - member = await prisma.teammember.find_unique( - where={"teamId_userId": {"teamId": team_id, "userId": user_id}}, - include={"Team": True}, - ) - if member is None or member.Team is None or member.Team.archivedAt is not None: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, detail="Team not found" - ) + member = await _get_active_team(user_id, team_id) if member.status != "ACTIVE" or not member.isAdmin: raise HTTPException( status_code=status.HTTP_403_FORBIDDEN, detail="Team admin access required to manage team credentials", ) return member.Team.orgId async def _require_team_member(user_id: str, team_id: str) -> str: - member = await prisma.teammember.find_unique( - where={"teamId_userId": {"teamId": team_id, "userId": user_id}}, - include={"Team": True}, - ) - if member is None or member.Team is None or member.Team.archivedAt is not None: - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, detail="Team not found" - ) + member = await _get_active_team(user_id, team_id) if member.status != "ACTIVE": raise HTTPException( status_code=status.HTTP_403_FORBIDDEN, detail="Active team membership required", ) return member.Team.orgId🤖 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/integrations/router.py` around lines 501 - 549, Extract the shared Prisma membership lookup and missing/archived-team 404 handling from `_require_team_admin` and `_require_team_member` into a private helper that returns the validated member record. Update both authorization functions to reuse that helper, retaining `_require_team_admin`’s active-admin check and `_require_team_member`’s active-membership check and organization ID return behavior.Source: Coding guidelines
🤖 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/integrations/router.py`:
- Around line 571-603: Update _create_team_credential to wrap the
scoped_credentials.create_credential call in the same try/except pattern used by
the sibling personal-credential path. Log failures with logger.exception and
return the established controlled 500 response, while preserving the existing
success flow that assigns created["id"] and returns to_meta_response.
---
Outside diff comments:
In `@autogpt_platform/backend/backend/api/features/integrations/router.py`:
- Around line 445-470: Update create_credentials so the team_id branch rejects
OAuth2-typed credentials before calling _create_team_credential, returning the
established client error for unsupported team OAuth credentials. Keep non-OAuth
team credentials on the existing _create_team_credential path and leave personal
credential handling unchanged; add coverage for an OAuth-shaped body with
team_id.
In `@autogpt_platform/backend/backend/integrations/scoped_credentials.py`:
- Around line 127-167: Update create_credential to validate the TEAM ownership
invariant before creating the credential: when owner_type is "TEAM", require
team_id to be present and equal to owner_id, and reject missing or mismatched
values. Preserve the existing creation flow for non-TEAM owners and only persist
the row after validation succeeds.
---
Nitpick comments:
In `@autogpt_platform/backend/backend/api/features/integrations/router.py`:
- Around line 606-633: Reorder the credential route section so the public
handlers list_team_credentials and delete_team_credential appear before the
private helpers they call: _require_team_member, _require_team_admin, and
_team_cred_meta_to_response. Preserve each handler’s behavior and keep the
helpers defined below the routes.
- Around line 501-549: Extract the shared Prisma membership lookup and
missing/archived-team 404 handling from `_require_team_admin` and
`_require_team_member` into a private helper that returns the validated member
record. Update both authorization functions to reuse that helper, retaining
`_require_team_admin`’s active-admin check and `_require_team_member`’s
active-membership check and organization ID return behavior.
🪄 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: 1cc99b2b-1dee-4250-abad-b093ea98c85f
📒 Files selected for processing (4)
autogpt_platform/backend/backend/api/features/integrations/router.pyautogpt_platform/backend/backend/api/features/integrations/router_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/integrations/scoped_credentials_test.py
📜 Review details
⏰ Context from checks skipped due to timeout. (15)
- GitHub Check: check API types
- GitHub Check: Cursor Bugbot
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
- GitHub Check: type-check (3.11)
- GitHub Check: type-check (3.12)
- GitHub Check: type-check (3.13)
- GitHub Check: lint
- GitHub Check: types
- GitHub Check: lint
- GitHub Check: end-to-end tests
- GitHub Check: Check PR Status
- GitHub Check: Analyze (typescript)
- 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/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.pyautogpt_platform/backend/backend/api/features/integrations/router_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/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.pyautogpt_platform/backend/backend/api/features/integrations/router_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/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/api/features/integrations/router_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/integrations/router.pyautogpt_platform/backend/backend/api/features/integrations/router_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/integrations/router.pyautogpt_platform/backend/backend/api/features/integrations/router_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/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.pyautogpt_platform/backend/backend/api/features/integrations/router_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/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.pyautogpt_platform/backend/backend/api/features/integrations/router_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/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.pyautogpt_platform/backend/backend/api/features/integrations/router_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/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.pyautogpt_platform/backend/backend/api/features/integrations/router_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/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.pyautogpt_platform/backend/backend/api/features/integrations/router_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/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.pyautogpt_platform/backend/backend/api/features/integrations/router_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/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.pyautogpt_platform/backend/backend/api/features/integrations/router_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/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.pyautogpt_platform/backend/backend/api/features/integrations/router_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/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.pyautogpt_platform/backend/backend/api/features/integrations/router_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/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.pyautogpt_platform/backend/backend/api/features/integrations/router_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/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.pyautogpt_platform/backend/backend/api/features/integrations/router_test.py
🔇 Additional comments (4)
autogpt_platform/backend/backend/integrations/scoped_credentials.py (1)
170-223: LGTM!autogpt_platform/backend/backend/integrations/scoped_credentials_test.py (1)
174-285: LGTM!autogpt_platform/backend/backend/api/features/integrations/router_test.py (1)
854-1107: LGTM!autogpt_platform/backend/backend/api/features/integrations/router.py (1)
594-602: 🗄️ Data Integrity & IntegrationNo stale TEAM credential id issue here. The TEAM read path uses row metadata and does not rebuild a
Credentialsobject from the encrypted payload;CREDENTIALS_ADAPTERis used on USER-scoped rows only.> Likely an incorrect or invalid review comment.
…iant, handle store errors - create_credential now generates the row id up front and stamps it into the encrypted payload, so a decrypted read resolves to the same credential the row represents (was: blob kept the client-supplied id while Prisma assigned a different primary key) - enforce the TEAM ownership invariant (team_id == owner_id) instead of relying on caller discipline; without it the row loses cascade cleanup - wrap the team-credential store call in try/except with logger.exception + 500, matching the personal-credential path's observability Co-Authored-By: Claude Opus <noreply@anthropic.com>
|
Re: the outside-diff nitpick to guard the TEAM ownership invariant in |
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/integrations/scoped_credentials.py (1)
150-169: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject
team_idfor non-TEAM credentials.A USER/ORG call can currently supply
team_id, writing an unrelatedteamIdFK. That credential can then be cascade-deleted with the team despite being resolved as USER/ORG-owned.Proposed fix
- if owner_type == "TEAM" and team_id != owner_id: - # Enforce the invariant the docstring promises: without the matching - # teamId FK, the row loses cascade cleanup and diverges from what the - # read path resolves on. - raise ValueError("team_id must equal owner_id for TEAM-owned credentials") + if owner_type == "TEAM": + if team_id != owner_id: + raise ValueError( + "team_id must equal owner_id for TEAM-owned credentials" + ) + elif team_id is not None: + raise ValueError("team_id is only valid for TEAM-owned credentials")🤖 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/integrations/scoped_credentials.py` around lines 150 - 169, Update the credential creation validation before encryption and persistence to reject any non-TEAM credential with a non-null team_id. Preserve the existing TEAM validation requiring team_id to equal owner_id, and raise a clear ValueError before creating the row when USER- or ORG-owned credentials supply 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/integrations/scoped_credentials.py`:
- Around line 150-169: Update the credential creation validation before
encryption and persistence to reject any non-TEAM credential with a non-null
team_id. Preserve the existing TEAM validation requiring team_id to equal
owner_id, and raise a clear ValueError before creating the row when USER- or
ORG-owned credentials supply team_id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64817499-1c1c-414d-bd47-052a0156d856
📒 Files selected for processing (4)
autogpt_platform/backend/backend/api/features/integrations/router.pyautogpt_platform/backend/backend/api/features/integrations/router_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/integrations/scoped_credentials_test.py
🚧 Files skipped from review as they are similar to previous changes (3)
- autogpt_platform/backend/backend/integrations/scoped_credentials_test.py
- autogpt_platform/backend/backend/api/features/integrations/router_test.py
- autogpt_platform/backend/backend/api/features/integrations/router.py
📜 Review details
⏰ Context from checks skipped due to timeout. (15)
- GitHub Check: check API types
- GitHub Check: Cursor Bugbot
- GitHub Check: end-to-end tests
- GitHub Check: lint
- GitHub Check: types
- GitHub Check: Analyze (python)
- GitHub Check: type-check (3.13)
- GitHub Check: test (3.12)
- GitHub Check: type-check (3.11)
- GitHub Check: type-check (3.12)
- GitHub Check: Analyze (typescript)
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
- GitHub Check: lint
- GitHub Check: Check PR Status
🧰 Additional context used
📓 Path-based instructions (2)
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/integrations/scoped_credentials.py
autogpt_platform/{backend,autogpt_libs}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Format Python code with
poetry run format
Files:
autogpt_platform/backend/backend/integrations/scoped_credentials.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/integrations/scoped_credentials.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/integrations/scoped_credentials.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/integrations/scoped_credentials.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/integrations/scoped_credentials.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/integrations/scoped_credentials.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/integrations/scoped_credentials.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/integrations/scoped_credentials.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/integrations/scoped_credentials.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/integrations/scoped_credentials.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/integrations/scoped_credentials.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/integrations/scoped_credentials.py
🔇 Additional comments (4)
autogpt_platform/backend/backend/integrations/scoped_credentials.py (4)
14-14: LGTM!
156-177: LGTM!
183-203: LGTM!
206-234: LGTM!
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #13641 +/- ##
==========================================
+ Coverage 77.56% 77.60% +0.03%
==========================================
Files 2849 2854 +5
Lines 215522 216108 +586
Branches 20569 20856 +287
==========================================
+ Hits 167179 167718 +539
- Misses 43806 43844 +38
- Partials 4537 4546 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/batch orgs |
Clean merge. scoped_credentials_test (team-scoped credential write) passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NBwmh7CkHiF8vKLBf2GWTU
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bfeb379. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
/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 #13641
PR #13641 — feat(backend): team-scoped credential creation + management (team admins)
Author: ntindle | Files: 4
🎯 Verdict: BLOCK
PR Description Quality
✅ Has Why + What + How — the scope (team admins create, active members list, admins delete, OAuth deferred, personal path unchanged) is clearly stated and matched against actual behavior by reviewers.
What This PR Does
Adds team-scoped credential management to the backend: team admins can create credentials owned by a team (stored in the IntegrationCredential scoped store), active members can list them, and admins can soft-delete (revoke) them. Authorization is derived server-side from team membership and org, and the personal per-user credential path is left untouched. It also makes JSONCryptor lazy so importing the module no longer requires ENCRYPTION_KEY at import time (fixes CI OpenAPI export).
Specialist Findings
🛡️ Security organization_id is server-derived, admin/member gates are explicit, cross-org/cross-team IDs 404, and delete/list are org+owner scoped. Two contained gaps.
🟠 OAuth2 credential types are accepted on the team path despite team OAuth being deferred (router.py:591), storing un-revocable tokens.
🟠 get_credential_by_id ignores soft-delete status (scoped_credentials.py:114), so a revoked cred stays fetchable — latent (no live caller today).
🏗️ Architecture _require_team_admin/_require_team_member (router.py:512, :534), which are ~90% duplicated hand-rolled authz. REST surface is asymmetric (create is a ?team_id= query param; list/delete live under /teams/{team_id}/credentials).
⚡ Performance ✅ — No hot-path concerns; this is a low-traffic admin surface. delete_team_credential uses two DB round-trips (scoped_credentials.py:233) where one scoped update_many would do; list_team_credentials is unbounded but filtered to active rows. Both minor.
🧪 Testing api_key types, OAuth rejection.
📖 Quality "ACTIVE" vs "active"), and stringly-typed dict boundaries between store and router.
📦 Product host/username/scopes to None (router.py:566), making host-scoped and user-password creds indistinguishable in the list — and inconsistent with the create response, which does populate them. Plus the OAuth-accept gap.
📬 Discussion check API types CI is red because openapi.json wasn't regenerated for the new endpoints, and CodeRabbit's Major OAuth-guard concern is unaddressed. Branch is BEHIND dev; test (3.13) timeout looks flaky (3.11/3.12 passed).
🔎 QA 🔴 — Ran all three endpoints against a live DB with a real team/admin. List, delete (with DB status active→revoked), and the full authz matrix (403/404/401 across admin/member/suspended/non-member) all work correctly. But the headline feature — credential creation — returns HTTP 500 and writes 0 rows, reproduced 3×.
🔴 Blockers
-
Team credential creation is broken — HTTP 500, persists nothing (
scoped_credentials.py:174&:186) —prisma.integrationcredential.create()is rejected by the query engine withMissingRequiredValueError: the requiredOrganizationrelation is passed as a raw scalar"organizationId": organization_idinstead of the"Organization": {"connect": {"id": ...}}connect form, and"metadata": metadatapasses a rawdict/Nonefor aJson?field instead ofprisma.Json(...). Reproduced live 3× against a real team+admin;SELECT count(*)confirms 0 rows before and after. This is the PR's core purpose and it does not function. (Flagged by: QA — reproduced end-to-end) -
openapi.jsonnot regenerated →check API typesCI is red (router.py:451) — the newteam_idquery param and/teams/{team_id}/credentialsroutes change the OpenAPI surface, but the generated frontend client wasn't updated. Regenerate and commit (poetry run export-api-schema, prettier,pnpm generate:api). (Flagged by: discussion — GitHub CI)
🟠 Should Fix
- Add a non-mocked integration test for the create path (
scoped_credentials_test.py:180) — all create tests mock Prisma, producing a false green over a create() that fails against the real schema. At least one create→list→delete test against the realIntegrationCredentialtable would have caught blocker #1. (Flagged by: QA, testing — 2 specialists) - Reject unsupported (OAuth2) credential types on the team path (
router.py:591) — team OAuth is explicitly deferred, but the endpoint accepts the fullCredentialsunion; a stored OAuth2 team cred can never be refreshed and its delete returnsrevoked=None. Reject non-{api_key, host_scoped, user_password}with a 400 before storing. (Flagged by: security, architect, testing, product, discussion — 5 specialists) - List response drops
host/username, making creds indistinguishable (router.py:566) — host-scoped/user-password creds show identical rows in the list and disagree with the create response. Surface these fields. (Flagged by: product, quality — 2 specialists) - Honor soft-delete status in
get_credential_by_id(scoped_credentials.py:114) — add astatus == "active"filter so revocation is enforced consistently before the read path is wired into execution. (Flagged by: security) - Route authz off direct Prisma access and de-duplicate the two helpers (
router.py:512,:534) — move the membership lookup behind the data layer and collapse_require_team_admin/_require_team_memberinto one parametrized helper to prevent drift in load-bearing authz. (Flagged by: architect, quality — 2 specialists) - Cover the missing authz branches (
router_test.py:1026,:883) — inactive-member-on-list 403, and host-scoped/user-password create round-trips. (Flagged by: testing)
🟡 Nice to Have
- Single-query scoped delete (
scoped_credentials.py:233) — replace find-then-update with oneupdate_manyscoped by{id, organizationId, ownerType, ownerId, status}; atomic and one round-trip. (performance, architect) - Drop the redundant
team_idparam oncreate_credential(scoped_credentials.py:165) — derive it fromowner_idfor TEAM instead of requiring equality and raising. (architect) - Consider
POST /teams/{team_id}/credentialsfor symmetry with list/delete, or document the query-param form. (architect, product)
🔵 Nits
- Magic status strings with inconsistent casing (
scoped_credentials.py:205) — promote"active"/"revoked"/"TEAM"etc. to aStrEnum. (quality) - Stringly-typed store↔router dict boundary (
scoped_credentials.py:191) — aTypedDictwould type-check the "load-bearing shape". (quality) - Change-relative comment (
router.py:465) — rewrite "legacy per-user store unchanged" to the standing fact. (architect) - Stale
owner_typecomment droppingWORKSPACE(scoped_credentials.py:142) — confirm the enumerated set. (quality)
QA Screenshots
| Screenshot | Description |
|---|---|
 |
Post-login surface for the backend-only PR; captured for evidence. Create endpoint returned HTTP 500 ❌; list/delete/authz verified via API ✅ |
Human Review Needed
YES — This changes how credentials/secrets are stored and adds a team-level authorization boundary; the security-boundary code warrants human eyes, and the create path must be fixed and re-verified against a real DB before merge.
Risk Assessment
Merge risk: HIGH | Rollback: EASY (additive, new endpoints behind team gates; personal path untouched)
CI Status
Local harness (review sandbox): ✅ all 5 checks pass (frontend lint/types/test/build, backend lint). GitHub CI: check API types ❌ (stale openapi.json), test (3.13) ❌ (flaky timeout — 3.11/3.12 passed), aggregate roll-up red; branch is BEHIND dev. Note: the local harness does not exercise the backend Prisma create path, which is why blocker #1 is not reflected in the harness results.
UI Testing — Variant Results
❌ local: Team credential creation endpoint returns HTTP 500 and persists nothing due to an invalid prisma-client-py create input (scalar organizationId instead of Organization relation connect, and unwrapped Json metadata); list/delete/authz all work.
- critical: prisma.integrationcredential.create() is rejected by the Prisma query engine with MissingRequiredValueError:
data.Organization: A value is required but not set. prisma-client-py does not accept the raw scalar FK 'organizationId' for a required relation; it requires the relation connect form. Reproduced live: POST /{provider}/credentials?team_id= returns HTTP 500 and writes 0 rows. - critical: "metadata": metadata passes a raw dict/None for a Json? field; prisma-client-py raises 'metadata should be of type NullableJsonNullValueInput or Json'. Contributes to the same create() 500.
- high: All create tests mock prisma.integrationcredential.create (or scoped_credentials.create_credential), so they assert the kwargs passed but never validate the real Prisma input contract. The create path fails against a real DB while these tests stay green — a false pass.
❌ hosted: Team credential creation (the PR's core feature) returns HTTP 500 on every request because scoped_credentials.create_credential passes a raw dict/None for the Prisma Json metadata field instead of wrapping it; all 20 new tests mock the Prisma boundary and miss it.
- critical: create_credential passes
"metadata": metadataas a rawdict | Nonedirectly into prisma.integrationcredential.create. prisma-client-py rejects this: a None value on the optionalJson?field yieldsMissingRequiredValueError: data.metadata: A value is required but not set, and a dict yieldsmetadata should be of ... NullableJsonNullValueInput, Json. Verified live: POST /openai/credentials?team_id=... returns HTTP 500 'Failed to store credentials' for both metadata-absent and metadata-present requests, and no row is written. This makes the PR's headline capability (creating TEAM-owned credentials) completely non-functional. - high: Every new create test mocks
_cryptorandmock_prisma.integrationcredential.create, so the row-shape assertions run against a MagicMock and never touch a real Prisma engine. This is why the 100%-reproducible HTTP 500 at the Prisma boundary was not caught by the 20 added tests.
github-project-automation
Bot
moved this from 🆕 Needs initial review
to 🚧 Needs work
in AutoGPT development kanban
|
👋 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! 🙂 |
…n the team paths
Team credential creation was reaching Prisma with an input shape the query
engine rejects, so the PR's core endpoint returned HTTP 500 and persisted
nothing. `create_credential()` had no production caller before this branch, so
the bug was latent until the router started using it:
- `Organization` is a *required* relation — the raw `organizationId` scalar is
rejected with `MissingRequiredValueError`; it must be passed in `connect`
form.
- the `teamId` relation is named `Workspace`, and is now derived from
`owner_id` for TEAM rows (and omitted entirely otherwise) so the FK and
`ownerId` cannot desync. This replaces the equality check + `ValueError`.
- `metadata` is a `Json?` column, so it is wrapped in `SafeJson` and the key is
omitted when there is nothing to store.
Adds `scoped_credentials_integration_test.py`, which drives create -> list ->
delete against the real `IntegrationCredential` table — the mocked tests could
only assert the shape of the input, never that the engine accepts it.
Also on the team paths:
- reject non-`{api_key, host_scoped, user_password}` credentials with a 400.
Team OAuth is deferred, and a stored OAuth2 team credential could never be
refreshed and its delete could never revoke provider-side tokens.
- surface `host` in the list response by mirroring it into the row's metadata
at create time, so host-scoped creds stay distinguishable and list agrees
with create.
- default `title` to the provider before responding, so the 201 reports the
`displayName` that was actually persisted instead of `null`.
- honor the soft-delete status in `get_credential_by_id`, so a revoked
credential is not still readable (or decryptable) by id.
- collapse `_require_team_admin`/`_require_team_member` into one parametrized
`_require_team_access`, and move the membership lookup behind
`team_db.get_team_membership` so the router no longer hand-rolls authz
against Prisma.
- make `delete_team_credential` a single scoped `update_many` instead of
find-then-update, so the ownership check and the write are atomic.
- promote the credential status strings to a `CredentialStatus` StrEnum and
type the store<->router boundary with a `CredentialMetadata` TypedDict.
Regenerates `openapi.json` for the new `team_id` query param and the two
`/teams/{team_id}/credentials` routes (`check API types` was red).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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. |
github-actions
Bot
added
cla: signed
|
🤖 Addressed the automated review ( 🔴 Blockers — both fixed1. Create returned 500 / wrote 0 rows. Confirmed against data: IntegrationCredentialCreateInput = {
"id": credential_id,
"Organization": {"connect": {"id": organization_id}}, # required relation, not a scalar
...
}
if owner_type == CredentialOwnerType.TEAM:
data["Workspace"] = {"connect": {"id": owner_id}} # relation is `Workspace`, not `Team`
if metadata is not None:
data["metadata"] = SafeJson(metadata) # `Json?`, and omitted when absentNote the diagnosis was right but the line attribution slightly off: those three lines were unchanged context in the diff. 2. Stale 🟠 Should Fix — all six done
🟡 Nice to Have
🔵 Nits — all four done
📬 Sentry's route-shadowing report — false positiveArgued in-thread with evidence: Local: 55/55 router tests, 16/16 store tests, |
There was a problem hiding this comment.
Actionable comments posted: 1
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/integrations/scoped_credentials.py (1)
162-186: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winConsider excluding archived teams in the cross-team membership check.
get_team_membershipinautogpt_platform/backend/backend/api/features/orgs/team_db.pytreats an archived team as no membership. This path only checks theTeamMemberrow status. A member of an archived team can therefore still read, and withdecrypt=Trueuse, that team's credential by ID. Align the two checks.🛡️ Proposed fix
membership = await prisma.teammember.find_unique( - where={"teamId_userId": {"teamId": cred.ownerId, "userId": user_id}} + where={"teamId_userId": {"teamId": cred.ownerId, "userId": user_id}}, + include={"Team": True}, ) - if membership is None or membership.status != "ACTIVE": + if ( + membership is None + or membership.status != "ACTIVE" + or membership.Team is None + or membership.Team.archivedAt is not None + ): return None🤖 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/integrations/scoped_credentials.py` around lines 162 - 186, Update the cross-team access branch in the credential lookup flow around the TeamMember query to also reject memberships whose owning team is archived, matching get_team_membership behavior. Preserve access for active team memberships and continue returning None before metadata or decryption when the team is archived.
🧹 Nitpick comments (1)
autogpt_platform/backend/backend/api/features/integrations/router.py (1)
500-673: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting the team-credential surface into its own module.
This PR adds about 175 lines to
router.py, which now holds the personal and the team credential paths plus their helpers. The coding guidelines require files under about 300 lines, split by responsibility. MovingTEAM_CREDENTIAL_TYPES,_require_team_access,_team_cred_meta_to_response,_team_row_metadata,_create_team_credential, and the two team routes into a sibling module insidebackend/api/features/integrations/keeps the router focused. Tests patch{ROUTER}.scoped_credentialsand{ROUTER}.get_team_membership, so update those mock targets if you move the code.As per coding guidelines: "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)".
🤖 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/integrations/router.py` around lines 500 - 673, Extract the team-credential surface from the integrations router into a sibling module: move TEAM_CREDENTIAL_TYPES, _require_team_access, _team_cred_meta_to_response, _team_row_metadata, _create_team_credential, list_team_credentials, and delete_team_credential together so the router remains focused and under the file-size guideline. Preserve the existing routes and behavior, wire the new module into the router as needed, and update tests that patch the old router-scoped scoped_credentials or get_team_membership symbols to target their new module locations.Source: Coding guidelines
🤖 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/integrations/router.py`:
- Around line 577-588: Update _team_row_metadata to stop copying the full
client-supplied credentials.metadata; construct the row metadata only from the
known non-secret display field host used by _team_cred_meta_to_response,
preserving None when no host is available.
---
Outside diff comments:
In `@autogpt_platform/backend/backend/integrations/scoped_credentials.py`:
- Around line 162-186: Update the cross-team access branch in the credential
lookup flow around the TeamMember query to also reject memberships whose owning
team is archived, matching get_team_membership behavior. Preserve access for
active team memberships and continue returning None before metadata or
decryption when the team is archived.
---
Nitpick comments:
In `@autogpt_platform/backend/backend/api/features/integrations/router.py`:
- Around line 500-673: Extract the team-credential surface from the integrations
router into a sibling module: move TEAM_CREDENTIAL_TYPES, _require_team_access,
_team_cred_meta_to_response, _team_row_metadata, _create_team_credential,
list_team_credentials, and delete_team_credential together so the router remains
focused and under the file-size guideline. Preserve the existing routes and
behavior, wire the new module into the router as needed, and update tests that
patch the old router-scoped scoped_credentials or get_team_membership symbols to
target their new module locations.
🪄 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: 2cc69eb6-2e94-401c-a74a-5159f938f00f
📒 Files selected for processing (8)
autogpt_platform/backend/backend/api/features/integrations/router.pyautogpt_platform/backend/backend/api/features/integrations/router_test.pyautogpt_platform/backend/backend/api/features/orgs/team_db.pyautogpt_platform/backend/backend/api/features/orgs/team_model.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/integrations/scoped_credentials_integration_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials_test.pyautogpt_platform/frontend/src/app/api/openapi.json
📜 Review details
⏰ Context from checks skipped due to timeout. (15)
- GitHub Check: lint
- GitHub Check: integration_test
- GitHub Check: check API types
- GitHub Check: Seer Code Review
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
- GitHub Check: type-check (3.12)
- GitHub Check: type-check (3.13)
- GitHub Check: test (3.12)
- GitHub Check: type-check (3.11)
- GitHub Check: lint
- GitHub Check: Check PR Status
- GitHub Check: end-to-end tests
- GitHub Check: Analyze (typescript)
- 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/api/features/orgs/team_db.pyautogpt_platform/backend/backend/api/features/orgs/team_model.pyautogpt_platform/backend/backend/integrations/scoped_credentials_integration_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/api/features/integrations/router_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.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/team_db.pyautogpt_platform/backend/backend/api/features/orgs/team_model.pyautogpt_platform/backend/backend/api/features/integrations/router_test.pyautogpt_platform/backend/backend/api/features/integrations/router.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/team_db.pyautogpt_platform/backend/backend/api/features/orgs/team_model.pyautogpt_platform/backend/backend/integrations/scoped_credentials_integration_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/api/features/integrations/router_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.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/team_db.pyautogpt_platform/backend/backend/api/features/orgs/team_model.pyautogpt_platform/backend/backend/api/features/integrations/router_test.pyautogpt_platform/backend/backend/api/features/integrations/router.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/integrations/scoped_credentials_integration_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/api/features/integrations/router_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/features/orgs/team_db.pyautogpt_platform/backend/backend/api/features/orgs/team_model.pyautogpt_platform/backend/backend/integrations/scoped_credentials_integration_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/api/features/integrations/router_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.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/team_db.pyautogpt_platform/backend/backend/api/features/orgs/team_model.pyautogpt_platform/backend/backend/integrations/scoped_credentials_integration_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/api/features/integrations/router_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.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/team_db.pyautogpt_platform/backend/backend/api/features/orgs/team_model.pyautogpt_platform/backend/backend/integrations/scoped_credentials_integration_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/api/features/integrations/router_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.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/team_db.pyautogpt_platform/backend/backend/api/features/orgs/team_model.pyautogpt_platform/backend/backend/integrations/scoped_credentials_integration_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/api/features/integrations/router_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.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/team_db.pyautogpt_platform/backend/backend/api/features/orgs/team_model.pyautogpt_platform/backend/backend/integrations/scoped_credentials_integration_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/api/features/integrations/router_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.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/team_db.pyautogpt_platform/backend/backend/api/features/orgs/team_model.pyautogpt_platform/backend/backend/integrations/scoped_credentials_integration_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/api/features/integrations/router_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.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/team_db.pyautogpt_platform/backend/backend/api/features/orgs/team_model.pyautogpt_platform/backend/backend/integrations/scoped_credentials_integration_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/api/features/integrations/router_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.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/team_db.pyautogpt_platform/backend/backend/api/features/orgs/team_model.pyautogpt_platform/backend/backend/integrations/scoped_credentials_integration_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/api/features/integrations/router_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.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/team_db.pyautogpt_platform/backend/backend/api/features/orgs/team_model.pyautogpt_platform/backend/backend/integrations/scoped_credentials_integration_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/api/features/integrations/router_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.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/team_db.pyautogpt_platform/backend/backend/api/features/orgs/team_model.pyautogpt_platform/backend/backend/integrations/scoped_credentials_integration_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/api/features/integrations/router_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.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/team_db.pyautogpt_platform/backend/backend/api/features/orgs/team_model.pyautogpt_platform/backend/backend/integrations/scoped_credentials_integration_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials_test.pyautogpt_platform/backend/backend/api/features/integrations/router_test.pyautogpt_platform/backend/backend/integrations/scoped_credentials.pyautogpt_platform/backend/backend/api/features/integrations/router.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 (21)
autogpt_platform/backend/backend/integrations/scoped_credentials.py (5)
13-72: LGTM!
75-136: LGTM!
189-244: LGTM!
247-304: LGTM!
326-343: LGTM!autogpt_platform/backend/backend/integrations/scoped_credentials_test.py (2)
10-11: LGTM!Also applies to: 28-41, 108-126
199-400: LGTM!autogpt_platform/backend/backend/integrations/scoped_credentials_integration_test.py (3)
40-69: LGTM!
72-196: LGTM!
32-33: 🩺 Stability & AvailabilityVerify pytest-asyncio behavior for
team_context.
autogpt_platform/backendsetsasyncio_mode = "auto"andasyncio_default_fixture_loop_scope = "session", and the marked tests use@pytest.mark.asyncio(loop_scope="session"). A plain@pytest.fixtureasync generator is awaited under the current configuration/mode; the remaining check is whether the shared Prisma connection survives the session loop in this test suite.autogpt_platform/backend/backend/api/features/orgs/team_model.py (1)
79-90: LGTM!autogpt_platform/backend/backend/api/features/orgs/team_db.py (1)
8-32: LGTM!autogpt_platform/backend/backend/api/features/integrations/router.py (5)
17-23: LGTM!Also applies to: 43-43
452-472: LGTM!
500-546: LGTM!
591-642: LGTM!
645-673: LGTM!autogpt_platform/backend/backend/api/features/integrations/router_test.py (3)
7-14: LGTM!Also applies to: 857-933
936-1159: LGTM!
1162-1295: LGTM!autogpt_platform/frontend/src/app/api/openapi.json (1)
6612-6708: LGTM!Also applies to: 6856-6864
| def _team_row_metadata(credentials: Credentials) -> dict[str, Any] | None: | ||
| """Non-secret display metadata to mirror onto the credential row. | ||
|
|
||
| Listing team credentials must not decrypt payloads, so anything the list | ||
| response needs has to live in queryable columns. ``host`` is the only such | ||
| field today (host-scoped creds are otherwise indistinguishable in a list). | ||
| """ | ||
| row_metadata = dict(credentials.metadata or {}) | ||
| host = CredentialsMetaResponse.get_host(credentials) | ||
| if host is not None: | ||
| row_metadata["host"] = host | ||
| return row_metadata or None |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Mirror only the known display fields into the unencrypted row metadata.
_team_row_metadata copies the whole client-supplied credentials.metadata dict into the row's metadata column. That column is stored unencrypted by scoped_credentials.create_credential, while only payload is encrypted. The docstring states that only non-secret display fields are mirrored, and _team_cred_meta_to_response reads only host. A caller can therefore place arbitrary content, including secret-like values, in metadata and it is persisted in plaintext and returned to every active team member by the list endpoint.
Restrict the mirrored dictionary to the fields the list response needs.
🛡️ Proposed fix
def _team_row_metadata(credentials: Credentials) -> dict[str, Any] | None:
"""Non-secret display metadata to mirror onto the credential row.
Listing team credentials must not decrypt payloads, so anything the list
response needs has to live in queryable columns. ``host`` is the only such
- field today (host-scoped creds are otherwise indistinguishable in a list).
+ field today (host-scoped creds are otherwise indistinguishable in a list).
+ Only that field is mirrored: the column is not encrypted, so arbitrary
+ client-supplied metadata must not be copied into it.
"""
- row_metadata = dict(credentials.metadata or {})
+ row_metadata: dict[str, Any] = {}
host = CredentialsMetaResponse.get_host(credentials)
if host is not None:
row_metadata["host"] = host
return row_metadata or 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.
| def _team_row_metadata(credentials: Credentials) -> dict[str, Any] | None: | |
| """Non-secret display metadata to mirror onto the credential row. | |
| Listing team credentials must not decrypt payloads, so anything the list | |
| response needs has to live in queryable columns. ``host`` is the only such | |
| field today (host-scoped creds are otherwise indistinguishable in a list). | |
| """ | |
| row_metadata = dict(credentials.metadata or {}) | |
| host = CredentialsMetaResponse.get_host(credentials) | |
| if host is not None: | |
| row_metadata["host"] = host | |
| return row_metadata or None | |
| def _team_row_metadata(credentials: Credentials) -> dict[str, Any] | None: | |
| """Non-secret display metadata to mirror onto the credential row. | |
| Listing team credentials must not decrypt payloads, so anything the list | |
| response needs has to live in queryable columns. ``host`` is the only such | |
| field today (host-scoped creds are otherwise indistinguishable in a list). | |
| Only that field is mirrored: the column is not encrypted, so arbitrary | |
| client-supplied metadata must not be copied into it. | |
| """ | |
| row_metadata: dict[str, Any] = {} | |
| host = CredentialsMetaResponse.get_host(credentials) | |
| if host is not None: | |
| row_metadata["host"] = host | |
| return row_metadata or None |
🤖 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/integrations/router.py` around
lines 577 - 588, Update _team_row_metadata to stop copying the full
client-supplied credentials.metadata; construct the row metadata only from the
known non-secret display field host used by _team_cred_meta_to_response,
preserving None when no host is available.
|
/review |
|
|
/review |
There was a problem hiding this comment.
📋 Automated Review — PR #13641
PR #13641 — feat(backend): team-scoped credential creation + management (team admins)
Author: ntindle | Files: 8
🎯 Verdict: REQUEST_CHANGES
PR Description Quality
✅ Has Why + What + How — clearly scoped as the "write half" of team-owned credentials, honestly flags OAuth2 as deferred, and self-discloses the one new pyright diagnostic. Good discipline.
What This PR Does
Adds the ability for team admins to create and manage credentials owned by a team rather than an individual: POST /{provider}/credentials?team_id= to create, and GET/DELETE /teams/{team_id}/credentials[/{id}] to list and revoke. The organizationId is derived server-side from the team, all mutations are gated on MANAGE_CREDENTIALS (team-admin), OAuth2 is explicitly rejected for team ownership, and the personal credential path is unchanged.
Prior BLOCK status: The previous review's critical findings — the create() HTTP 500 from passing a scalar organizationId instead of a relation connect, and unwrapped Json metadata — are now ✅ Addressed. QA reproduced create→list→delete live against a real DB (201, real row written, secret encrypted). The OAuth2-acceptance and revoked-row-fetchable findings are also resolved (OAuth2 rejected at create; status != ACTIVE → None guard added). One new merge-blocking issue has appeared: the PR's own integration test errors in GitHub CI.
Specialist Findings
🛡️ Security ✅ — Authorization ordering, server-derived tenancy (org_id never client-supplied), cross-team/cross-org isolation, and at-rest encryption are all sound and test-covered. One defense-in-depth note below.
🟠 _team_row_metadata mirrors the full client metadata dict into an unencrypted, member-visible column (router.py:584).
🏗️ Architecture ✅ — Well-layered; write path mirrors the read contract, atomic scoped delete (no TOCTOU), enum-over-stringly-typed cleanup. Verdict APPROVE.
🟠 Create is modeled inconsistently with list/delete — overloaded onto POST /{provider}/credentials via a team_id query flag (router.py:466).
⚡ Performance ✅ — Every route is 1–2 indexed queries; new filters land on existing IntegrationCredential indexes. No N+1, no complexity regressions. Only note: team list is unpaginated (low risk — team cred counts are naturally small).
🧪 Testing get_team_membership has no direct test (mocked everywhere), and the router↔store mock accepts any kwargs so signature drift ships green.
🟠 get_team_membership untested (team_db.py:14); 🟠 weak router↔store mock contract (router_test.py:900).
📖 Quality ✅ — Readability grade A; docstrings capture invariants and the OAuth2-deferral rationale. Only minor naming/type-annotation polish (leftover ws_ prefix, loose str param types).
📦 Product ✅ — Matches the stated write-half scope faithfully; authz matrix and secret handling verified. Notes: create can silently fall back to a personal credential if team_id is dropped; list response omits creator/last-used provenance a team admin needs before revoking.
📬 Discussion test (3.11/3.12/3.13) fail because the PR's new integration-test fixture errors with Event loop is closed. The blocking CHANGES_REQUESTED decision is also stale (from pre-fix commit bfeb379; two re-reviews failed on infra, not code). One CodeRabbit metadata-security comment remains unanswered.
🔎 QA ✅ — Exercised all 18 scenarios against the live database: happy path, 4 credential types, full 403/404 authz matrix, cross-team delete escalation (blocked, 404), soft-delete lifecycle, and personal-path regression. Row shape is byte-identical to the read path's contract; secret sk-qa-supersecret → 0 plaintext hits. No defects found in the runtime feature.
🔴 Blockers
- New integration test errors in GitHub CI —
Event loop is closed(autogpt_platform/backend/backend/integrations/scoped_credentials_integration_test.py:32) — theteam_contextfixture is a plain@pytest.fixturewhile the tests use@pytest.mark.asyncio(loop_scope="session"), so fixture setup runs on a loop closed relative to the test loop. This ERRORstest (3.11),test (3.12),test (3.13)andCheck PR Status— the PR cannot merge on a red required suite. Fix: use@pytest_asyncio.fixture(loop_scope="session")and confirm the backend matrix goes green. (Flagged by: discussion — traced to concrete GitHub CI failure)
🟠 Should Fix
- Full client metadata mirrored into an unencrypted, member-visible column (
router.py:584) —_team_row_metadatacopies the entire arbitrarycredentials.metadatadict into the plaintextIntegrationCredential.metadataJSON, readable by any ACTIVE team member via the list endpoint, though onlyhostis ever consumed. Buildrow_metadatafrom an explicit allowlist (start empty, copy onlyhost). This is also the one unanswered CodeRabbit thread. (Flagged by: security, discussion — 2 specialists) get_team_membershipauthorization primitive has no direct test (team_db.py:14) — the function gating create/list/delete (archived-team exclusion, org-id derivation, ACTIVE mapping) is only ever mocked. A regression dropping thearchivedAtcheck would leak cross-org access with no failing test. Add an integration test (harness already exists) covering ACTIVE admin / ACTIVE non-admin / non-ACTIVE / archived. (Flagged by: testing)- Router↔store contract never exercised (
router_test.py:900) —patch(f"{ROUTER}.scoped_credentials")accepts any kwargs, so a renamed/dropped arg between_create_team_credentialandcreate_credentialships green. Useautospec=Trueor one real-store end-to-end test. (Flagged by: testing) - Team create is overloaded via
team_idquery flag (router.py:466) — list/delete live at/teams/{team_id}/credentials, but create is bolted onto the genericPOST /{provider}/credentials. If a client dropsteam_id, the request silently succeeds as a personal credential — the admin believes they shared a key with the team but it stays private. Prefer a symmetricPOST /teams/{team_id}/credentials/{provider}; if reuse is intentional, capture the rationale in the ticket. (Flagged by: architect, product — 2 specialists)
🟡 Nice to Have
- Bound the team-credential list (
scoped_credentials.py:233) — add atakecap / pagination for defense-in-depth. (performance) - Surface shared-credential provenance (
router.py:565) — exposecreatedByUserId/lastUsedAt/createdAtso admins can see who added a key before revoking it. (product) - Expose the
providerfilter on the list route (router.py:565) — the store already accepts it; without it a block picker fetches all team creds and filters client-side. (product) - Type-aware unsupported-type message (
router.py:603) — only append the "Connect OAuth per user" hint whencredentials.type == "oauth2". (product) - Expiry round-trip test (
router.py:620) — no team-path test asserts a non-null expiry survives into the stored payload. (testing)
🔵 Nits
- Leftover
ws_naming (scoped_credentials.py:109) — renamews_where/ws_creds→team_where/team_credsfor consistency with the USER/ORG branches. (architect, quality) - Loose param type (
scoped_credentials.py:194) —credential_type: str→credential_type: CredentialsType. (quality) - Untyped
credparam (scoped_credentials.py:330) — annotatecred: IntegrationCredential. (quality)
QA Screenshots
| Screenshot | Description |
|---|---|
 |
Integrations page renders after the regenerated openapi.json; frontend healthy ✅ |
Human Review Needed
YES — This change adds a new path for how credentials/secrets are stored and a team-scoped permission boundary; a human should confirm the metadata-encryption and authz decisions before merge.
Risk Assessment
Merge risk: MEDIUM | Rollback: EASY (additive endpoints + a store method; the personal path is untouched, revert is isolated)
CI Status
GitHub CI: RED — test (3.11), test (3.12), test (3.13), and Check PR Status fail on this head due to the PR's own new integration-test fixture (Event loop is closed); ~45 other checks green (lint, type-check, CodeQL, e2e, codecov). The blocking CHANGES_REQUESTED decision is stale (pre-fix commit) and needs a clean automated re-review to lift.
Local harness (review sandbox, not repository CI): ✅ frontend lint, ✅ backend lint, ✅ frontend typecheck, ✅ frontend build; ❌ frontend test:unit. The local frontend-test failure is environment skew — the frontend suite is green on GitHub CI — so it is reported as a warning, not a blocker. The backend suite was not run locally; the authoritative backend result is the red GitHub CI above.
UI Testing — Variant Results
✅ local: Team-scoped credential create/list/delete works end-to-end against live DB with correct row shape, encryption, and a fully-verified authz matrix; no defects found.
✅ hosted: Team credential create/list/delete verified live: exact row shape, encrypted secrets, correct 401/403/404 authz matrix, and cross-team delete isolation all confirmed, with the new integration tests passing on the real DB.
| response needs has to live in queryable columns. ``host`` is the only such | ||
| field today (host-scoped creds are otherwise indistinguishable in a list). | ||
| """ | ||
| row_metadata = dict(credentials.metadata or {}) |
There was a problem hiding this comment.
🤖 🟢 low (security/data-exposure)
_team_row_metadata copies the entire client-supplied credentials.metadata dict (arbitrary dict[str, Any]) into the unencrypted IntegrationCredential.metadata column, which is readable by any ACTIVE team member via GET /teams/{team_id}/credentials. This diverges from the personal path, which encrypts the whole credential. Only host is actually consumed by the list response.
Suggestion: Build row_metadata from an explicit allowlist of known non-secret display keys (e.g. {"host": ...}) instead of dict(credentials.metadata or {}), so arbitrary/secret metadata can never land in a plaintext, member-visible column.
| status_code=status.HTTP_403_FORBIDDEN, | ||
| detail="Cannot create credentials with a reserved ID", | ||
| ) | ||
|
|
There was a problem hiding this comment.
🤖 🟡 medium (architect/api-design/resource-modeling)
Team credential creation is POST /{provider}/credentials?team_id= while list/delete live at /teams/{team_id}/credentials — the same logical resource is modeled two inconsistent ways, and the create endpoint is overloaded to produce two ownership models via a query flag.
Suggestion: Consider a uniform collection URL such as POST /teams/{team_id}/credentials/{provider} (provider in path or body) so create/read/delete share one shape; if reusing the provider path is intentional, document the rationale.
| """ | ||
| results: list[dict] = [] | ||
| results: list[CredentialMetadata] = [] | ||
|
|
There was a problem hiding this comment.
🤖 🟢 low (architect/naming-debt)
The WORKSPACE->TEAM rename left the local variables named ws_where/ws_creds in get_scoped_credentials step 2 while all surrounding literals and comments moved to TEAM, inviting confusion over whether 'workspace' and 'team' are distinct concepts.
Suggestion: Rename ws_where/ws_creds to team_where/team_creds for consistency with the rest of the rename.
| "expiresAt": expires_at, | ||
| } | ||
| if owner_type == CredentialOwnerType.TEAM: | ||
| # The teamId relation is named `Workspace` on this model (see its |
There was a problem hiding this comment.
🤖 🟢 low (architect/duplication)
list_team_credentials duplicates the TEAM-branch where clause from get_scoped_credentials; a future change to team visibility must be made in two places.
Suggestion: Extract a shared _team_where(org_id, team_id, provider) helper used by both the read and list paths.
| "createdByUserId": user_id, | ||
| "expiresAt": expires_at, | ||
| } | ||
| if owner_type == CredentialOwnerType.TEAM: |
There was a problem hiding this comment.
🤖 🟢 low (performance/unbounded result set)
list_team_credentials issues a find_many with no take/pagination, returning every active TEAM-owned credential in one response. Low risk given team credential counts are naturally small, but the endpoint has no upper bound if a team accumulates many rows.
Suggestion: Add a take cap (and optional cursor/skip pagination) to bound the result set for defense-in-depth, consistent with other list endpoints.
| ProviderName, Path(title="The provider to create credentials for") | ||
| ], | ||
| credentials: Credentials, | ||
| team_id: Annotated[ |
There was a problem hiding this comment.
🤖 🟡 medium (product/api-consistency / silent-misrouting)
Team credential creation is overloaded onto the generic POST /{provider}/credentials via an optional team_id query param, while list/delete live under /teams/{team_id}/credentials. If a client omits team_id, the request silently creates a personal credential instead of a team-shared one — the admin thinks they shared a key with the team but it stays private.
Suggestion: Add a dedicated POST /teams/{team_id}/credentials/{provider} route symmetric with the list/delete routes, so team creation is explicit and cannot silently fall back to the personal path.
| personal-credential path doesn't surface either. | ||
| """ | ||
| row_metadata = meta.get("metadata") or {} | ||
| return CredentialsMetaResponse( |
There was a problem hiding this comment.
🤖 🟢 low (product/product-completeness)
The team credential list response drops createdByUserId, lastUsedAt, and createdAt (already available in CredentialMetadata). For a shared team credential these are exactly the fields an admin needs to decide who added a key and whether it is still in use before revoking it.
Suggestion: Extend CredentialsMetaResponse (or add a team-specific response) to surface creator and last-used/created timestamps for team-owned credentials.
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail=( | ||
| f"Team-owned '{credentials.type}' credentials are not supported. " |
There was a problem hiding this comment.
🤖 🟢 low (product/error-messaging)
The unsupported-type error message hard-codes 'Connect OAuth integrations per user instead' for any type outside TEAM_CREDENTIAL_TYPES. It reads correctly only because oauth2 is the sole excluded type today; it becomes misleading if the credential-type union grows.
Suggestion: Make the message type-aware, e.g. only append the OAuth guidance when credentials.type == 'oauth2'.
| HOST = "api.example.com" | ||
|
|
||
|
|
||
| @pytest.fixture |
There was a problem hiding this comment.
🤖 🟠 high (discussion/ci-failure)
The new integration test test_team_credential_create_list_delete_round_trip ERRORs at setup with 'RuntimeError: Event loop is closed', failing test (3.11/3.12/3.13) and Check PR Status across CI. The team_context async fixture is a plain @pytest.fixture while the tests use @pytest.mark.asyncio(loop_scope='session'), so the fixture runs on a loop that is closed relative to the session-scoped test loop.
Suggestion: Mark the async fixture with a matching loop scope (e.g. @pytest_asyncio.fixture(loop_scope='session')) so fixture setup and the test share one event loop; re-run the backend test matrix to confirm green.
| host = CredentialsMetaResponse.get_host(credentials) | ||
| if host is not None: | ||
| row_metadata["host"] = host | ||
| return row_metadata or None |
There was a problem hiding this comment.
🤖 🟡 medium (discussion/unaddressed-review-comment)
CodeRabbit's latest (2026-08-06) security comment is unaddressed with no author reply: _team_row_metadata copies the entire client-supplied credentials.metadata dict into the row's metadata column, which scoped_credentials.create_credential stores UNENCRYPTED and the list endpoint returns to every active team member. Only 'host' is ever read back, so arbitrary/secret-like values can be persisted in plaintext.
Suggestion: Restrict the mirrored dict to the known display fields the list needs (start from an empty dict and copy only host), per CodeRabbit's proposed fix; reply on the thread to close it.
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. |
1 participant
Why
SECRT-2452 write-half (decided v1, 2026-07-21): the read path resolves USER → TEAM → ORG credentials, but nothing can create a TEAM-owned credential — team credential sharing was read-only theater.
What
POST /{provider}/credentials?team_id=— creates a TEAM-owned credential (API-key, host-scoped, user-password types); row shape exactly matches the read path's resolution contract (ownerType=TEAM,ownerId=<teamId>,teamIdFK for cascade,organizationIdderived fromTeam.orgId, never client-supplied). Legacy per-user path byte-for-byte unchanged.GET /teams/{team_id}/credentials— metadata list for ACTIVE team members (mirrors read-half visibility).DELETE /teams/{team_id}/credentials/{cred_id}— team admins only; delete is team+org-scoped in the store so a team-A admin can't revoke team-B's credential by id.TeamAction.MANAGE_CREDENTIALS({team_admin}): ACTIVE + isAdmin membership, archived teams excluded, cross-org unreachable by construction (404, unprobeable).Testing
20 new scenario tests (row shape at the Prisma boundary, 403/404 authz matrix, personal-path regression, list/delete scoping); 46 router + 14 scoped-credentials tests green. Formatters clean; pyright: 1 new diagnostic matching the file's pre-existing systemic Prisma-dict idiom (4 identical pre-existing ones untouched — consistency kept over one-file divergence).
Checklist
data/*/ credential paths: all team operations gated by ACTIVE membership checks; org id derived server-side; secrets encrypted via the existing JSONCryptor path🤖 Generated with Claude Code
https://claude.ai/code/session_01Jm3mCG9okfdGtAXtFaDF9A
Note
High Risk
Changes credential storage, encryption, and team authorization boundaries; mistakes could leak secrets or allow cross-team revocation, though OAuth team creds are blocked and delete/list are tightly scoped with extensive tests.
Overview
Adds the write path for team-owned integration credentials so teams can share API keys and similar secrets, aligned with the existing USER → TEAM → ORG read resolution.
API:
POST /{provider}/credentials?team_id=persists TEAM rows inIntegrationCredential(team admins only).GETandDELETEunder/teams/{team_id}/credentialslist metadata and soft-revoke team creds. Personal creation withoutteam_idis unchanged. OAuth2 is rejected for team ownership (refresh/revoke flows stay user-scoped).Authz:
get_team_membershipsupplies org id and admin/active flags; missing/archived/cross-org teams return 404; inactive or non-admin mutators get 403.Store:
scoped_credentialsgainslist_team_credentials/delete_team_credential, fixes Prisma create shape (Organizationconnect,WorkspaceFK for teams,SafeJsonmetadata), server-generated row ids in encrypted payloads, lazy encryptor for key-less CI, and hides revoked rows on id lookup. OpenAPI and broad unit/integration tests cover authz, secret non-leakage, and cross-team delete scoping.Reviewed by Cursor Bugbot for commit 71e691a. Bugbot is set up for automated code reviews on this repo. Configure here.