feat(backend): resolve team/org credentials on the read/use path - #13532
feat(backend): resolve team/org credentials on the read/use path#13532ntindle wants to merge 4 commits into
Conversation
…RT-2452) Why: IntegrationCredential already supported TEAM/ORG-owned credentials and backend.integrations.scoped_credentials implemented USER→TEAM→ORG resolution, but that resolver had zero callers — the live path (get_user_credentials) only ever returned the user's own USER-scoped rows. Team-owned "shared" credentials were therefore invisible to the credential picker and unusable by executions, so TeamAction.USE_CREDENTIALS was enforced nowhere. What: wire the dormant resolution into the single read/fetch chokepoint so a user resolves their own USER creds plus TEAM creds (teams they are an ACTIVE member of) plus ORG creds (orgs they are an ACTIVE member of). Credential LISTING (picker) and execution FETCH now both see shared creds; the write path is untouched, so personal USER-cred CRUD is byte-for-byte unchanged. How: - data/user.py: add get_accessible_credentials(user_id) — USER + active-team + active-org decrypted Credentials, USER>TEAM>ORG precedence, corrupt-row skip. get_user_credentials stays USER-only (the write path read-modify-writes it, so it must never widen). Shared decrypt logic extracted to a helper. - db_manager.py: expose get_accessible_credentials over RPC so the executor (which runs without an HTTP/request context) resolves from the credential owner's perspective. - credentials_store.py: _get_all_creds_unlocked now resolves the accessible set via a new _get_accessible_creds_unlocked; write helpers keep using _get_persisted_user_creds_unlocked (USER-only). Because get_creds_by_id filters get_all_creds, the executor's fetch-by-id inherits the access check: a credential id only resolves when the caller is a live member of the owning team/org — so a run scheduled by a user who later left the team stops resolving that team's creds at execution time (conservative: active membership re-checked on every fetch). Read-side only: create/update/delete of team/org creds (MANAGE_CREDENTIALS) and the credential-assignment UI are later slices. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jm3mCG9okfdGtAXtFaDF9A
github-project-automation
Bot
moved this to 🆕 Needs initial review
in AutoGPT development kanban
WalkthroughCredential reads now decrypt through a shared helper and resolve active USER, TEAM, and ORG credentials. The database RPC and integration store use the access-aware path, with tests covering membership filtering, precedence, deduplication, listing, and retrieval. ChangesAccessible credential resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User as Requesting user
participant Store as IntegrationCredentialsStore
participant RPC as DatabaseManagerAsyncClient
participant DB as Credential and membership data
User->>Store: Request credentials
Store->>RPC: get_accessible_credentials(user_id)
RPC->>DB: Query active memberships and credential rows
DB-->>RPC: USER, TEAM, and ORG candidates
RPC-->>Store: Decrypted accessible credentials
Store-->>User: Filtered credential results
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 PR Overlap DetectionThis check compares your PR against all other open PRs targeting the same branch to detect potential merge conflicts early. 🔴 Merge Conflicts DetectedThe following PRs have been tested and will have merge conflicts if merged after this PR. Consider coordinating with the authors.
🟢 Low Risk — File Overlap OnlyThese PRs touch the same files but different sections (click to expand)
Summary: 1 conflict(s), 0 medium risk, 3 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}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b2bedfe. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
autogpt_platform/backend/backend/data/user_test.py (1)
261-276: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer top-level imports over per-method local imports.
JSONCryptor,SecretStr,APIKeyCredentials, andget_accessible_credentialsare imported locally inside_rowand each test method.mocker.patch("backend.data.user.prisma", ...)patches the module attribute regardless of when the function is imported, so the deferral isn't required here — hoisting these to module scope removes the repetition.As per coding guidelines: "Use top-level imports only — avoid local/inner imports except for lazy imports of heavy optional dependencies like
openpyxl".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@autogpt_platform/backend/backend/data/user_test.py` around lines 261 - 276, Move the local imports of JSONCryptor in _row and SecretStr, APIKeyCredentials, and get_accessible_credentials in the test methods to module-level imports, removing the now-redundant per-method imports while preserving existing test behavior.Source: Coding guidelines
autogpt_platform/backend/backend/data/user.py (1)
287-308: 🩺 Stability & Availability | 🔵 TrivialConsider emitting a metric when rows are skipped, not just an error log.
The catch-all skip is fine for one-off corruption, but a systemic failure (e.g. an encryption-key rotation/mismatch) would make every row "corrupt," silently returning an empty credential set. On this path that surfaces downstream as agents losing all credentials with no distinct signal beyond scattered
logger.errorlines. A counter/alert on skip volume would let you distinguish a single bad row from a platform-wide decryption regression.🤖 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/data/user.py` around lines 287 - 308, The _decrypt_credential_rows function currently logs and skips failed rows without emitting a measurable signal. Add a counter metric for each decryption/validation failure in its exception handler, using the project’s existing metrics instrumentation and including appropriate context such as user or failure type, while retaining the existing error log and skip behavior.
🤖 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.
Nitpick comments:
In `@autogpt_platform/backend/backend/data/user_test.py`:
- Around line 261-276: Move the local imports of JSONCryptor in _row and
SecretStr, APIKeyCredentials, and get_accessible_credentials in the test methods
to module-level imports, removing the now-redundant per-method imports while
preserving existing test behavior.
In `@autogpt_platform/backend/backend/data/user.py`:
- Around line 287-308: The _decrypt_credential_rows function currently logs and
skips failed rows without emitting a measurable signal. Add a counter metric for
each decryption/validation failure in its exception handler, using the project’s
existing metrics instrumentation and including appropriate context such as user
or failure type, while retaining the existing error log and skip behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c66d2e9d-160c-4cc5-a111-80e490313187
📒 Files selected for processing (5)
autogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.pyautogpt_platform/backend/backend/data/user_test.pyautogpt_platform/backend/backend/integrations/credentials_store.pyautogpt_platform/backend/backend/integrations/credentials_store_test.py
📜 Review details
⏰ Context from checks skipped due to timeout. (11)
- GitHub Check: check API types
- GitHub Check: type-check (3.11)
- GitHub Check: end-to-end tests
- GitHub Check: Check PR Status
- GitHub Check: type-check (3.12)
- GitHub Check: type-check (3.13)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (typescript)
🧰 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/data/db_manager.pyautogpt_platform/backend/backend/integrations/credentials_store.pyautogpt_platform/backend/backend/integrations/credentials_store_test.pyautogpt_platform/backend/backend/data/user.pyautogpt_platform/backend/backend/data/user_test.py
autogpt_platform/backend/backend/data/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All data access in backend requires user ID checks; verify this for any 'data/*.py' changes
Files:
autogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.pyautogpt_platform/backend/backend/data/user_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/data/db_manager.pyautogpt_platform/backend/backend/integrations/credentials_store.pyautogpt_platform/backend/backend/integrations/credentials_store_test.pyautogpt_platform/backend/backend/data/user.pyautogpt_platform/backend/backend/data/user_test.py
autogpt_platform/**/data/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
For changes touching
data/*.py, validate user ID checks or explain why not needed
Files:
autogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.pyautogpt_platform/backend/backend/data/user_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/credentials_store_test.pyautogpt_platform/backend/backend/data/user_test.py
🧠 Learnings (13)
📚 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/data/db_manager.pyautogpt_platform/backend/backend/integrations/credentials_store.pyautogpt_platform/backend/backend/integrations/credentials_store_test.pyautogpt_platform/backend/backend/data/user.pyautogpt_platform/backend/backend/data/user_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/data/db_manager.pyautogpt_platform/backend/backend/integrations/credentials_store.pyautogpt_platform/backend/backend/integrations/credentials_store_test.pyautogpt_platform/backend/backend/data/user.pyautogpt_platform/backend/backend/data/user_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/data/db_manager.pyautogpt_platform/backend/backend/integrations/credentials_store.pyautogpt_platform/backend/backend/integrations/credentials_store_test.pyautogpt_platform/backend/backend/data/user.pyautogpt_platform/backend/backend/data/user_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/data/db_manager.pyautogpt_platform/backend/backend/integrations/credentials_store.pyautogpt_platform/backend/backend/integrations/credentials_store_test.pyautogpt_platform/backend/backend/data/user.pyautogpt_platform/backend/backend/data/user_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/data/db_manager.pyautogpt_platform/backend/backend/integrations/credentials_store.pyautogpt_platform/backend/backend/integrations/credentials_store_test.pyautogpt_platform/backend/backend/data/user.pyautogpt_platform/backend/backend/data/user_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/data/db_manager.pyautogpt_platform/backend/backend/integrations/credentials_store.pyautogpt_platform/backend/backend/integrations/credentials_store_test.pyautogpt_platform/backend/backend/data/user.pyautogpt_platform/backend/backend/data/user_test.py
📚 Learning: 2026-04-21T04:35:34.710Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12865
File: autogpt_platform/backend/backend/data/credit.py:1584-1584
Timestamp: 2026-04-21T04:35:34.710Z
Learning: When reviewing this codebase, don’t flag snake_case attribute names (e.g., `subscription_tier`, `stripe_customer_id`, `top_up_config`) on the app-layer Pydantic `User` model as “wrong” field names. These are correct for the app-layer model and are expected to be mapped from the Prisma-layer camelCase fields (e.g., `subscriptionTier`, `stripeCustomerId`) inside methods like `User.from_db()`. Only Prisma-returned/raw objects would use camelCase, but functions like `get_user_by_id(user_id: str)` are expected to return the Pydantic app-layer model.
Applied to files:
autogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.pyautogpt_platform/backend/backend/data/user_test.py
📚 Learning: 2026-05-07T15:32:39.703Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 13033
File: autogpt_platform/backend/backend/data/generate_data.py:111-117
Timestamp: 2026-05-07T15:32:39.703Z
Learning: When reviewing the Python data-generation layer, do not treat missing `user_id`/user filtering in calls to graph-metadata resolvers as a security issue if the `graph_id` inputs are already guaranteed to be user-scoped by earlier upstream SQL (e.g., `WHERE "userId" = ...`). In particular, `_resolve_agent_name(graph_id)` in `generate_data.py` correctly calls `get_graph_metadata(graph_id=graph_id)` without a `user_id` parameter by design, because name resolution must also work for user-executed shared/marketplace agents that the user may not own.
Applied to files:
autogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.pyautogpt_platform/backend/backend/data/user_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/data/db_manager.pyautogpt_platform/backend/backend/integrations/credentials_store.pyautogpt_platform/backend/backend/integrations/credentials_store_test.pyautogpt_platform/backend/backend/data/user.pyautogpt_platform/backend/backend/data/user_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/data/db_manager.pyautogpt_platform/backend/backend/integrations/credentials_store.pyautogpt_platform/backend/backend/integrations/credentials_store_test.pyautogpt_platform/backend/backend/data/user.pyautogpt_platform/backend/backend/data/user_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/data/db_manager.pyautogpt_platform/backend/backend/integrations/credentials_store.pyautogpt_platform/backend/backend/integrations/credentials_store_test.pyautogpt_platform/backend/backend/data/user.pyautogpt_platform/backend/backend/data/user_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/data/db_manager.pyautogpt_platform/backend/backend/integrations/credentials_store.pyautogpt_platform/backend/backend/integrations/credentials_store_test.pyautogpt_platform/backend/backend/data/user.pyautogpt_platform/backend/backend/data/user_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/data/db_manager.pyautogpt_platform/backend/backend/integrations/credentials_store.pyautogpt_platform/backend/backend/integrations/credentials_store_test.pyautogpt_platform/backend/backend/data/user.pyautogpt_platform/backend/backend/data/user_test.py
🔇 Additional comments (5)
autogpt_platform/backend/backend/data/user.py (1)
311-325: LGTM!Also applies to: 328-390
autogpt_platform/backend/backend/data/user_test.py (1)
288-311: LGTM!Also applies to: 313-341, 342-375, 376-407
autogpt_platform/backend/backend/data/db_manager.py (1)
131-131: LGTM!Also applies to: 310-310, 591-591
autogpt_platform/backend/backend/integrations/credentials_store.py (1)
331-354: LGTM!Also applies to: 378-389
autogpt_platform/backend/backend/integrations/credentials_store_test.py (1)
1-92: LGTM!
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #13532 +/- ##
==========================================
- Coverage 76.92% 75.94% -0.99%
==========================================
Files 2761 2793 +32
Lines 209521 210553 +1032
Branches 20077 20231 +154
==========================================
- Hits 161171 159898 -1273
- Misses 43981 46152 +2171
- Partials 4369 4503 +134
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/review |
|
/batch |
ntindle
added
the
batch
|
🤖 Batch command failed: Command failed: gh api -X PATCH repos/Significant-Gravitas/AutoGPT/git/refs/heads/batch/rollup -f sha=d153a652d452dd4bc1176444b048a31858249e74 -F force=true |
|
…fix add-then-list race (#13536) Replaces #13534 (auto-repointed to dev by the base-branch enforcer — the branch needed a `hotfix/*` name to target master). Same commit. Dev forward-port: #13535. ### Why / What / How **Why:** The batch bot's first live runs failed at the final step both times it got there: 1. `buildRollup` merges member branches into a **local** commit in the runner, then points the remote `batch/rollup` ref at that SHA via the git refs API — which 422s ("Object does not exist") because the server never received the merge objects. Hard blocker; the whole /batch flow dies here (live evidence: run 29135797524). 2. The first `/batch` on a PR reports "Current batch (0): none" — `cmdBatch` adds the label then immediately lists members, but GitHub's label index lags `pr edit --add-label` (live evidence: run 29135796249). **What/How:** - Replace the refs-API PATCH/POST dance with a single `git push --force origin HEAD:refs/heads/batch/rollup` — uploads objects and creates/force-moves the branch in one step under the bot's checkout token (array-arg `execFileSync`, no shell). - `rebuildAndReport` takes an optional `ensureNumber`: the just-labeled PR is fetched directly and unioned into the member list so the first /batch builds immediately. Targets `master` because issue_comment/repository_dispatch workflows execute the default branch's copy of the bot — a dev-only fix wouldn't take effect until the next release merge. ### Changes 🏗️ - `.github/batch-bot/batch.mjs`: force-push rollup branch; union just-added PR into the rebuild ### Checklist 📋 #### For code changes: - [x] I have clearly listed my changes in the PR description - [x] I have made a test plan - [x] I have tested my changes according to the test plan: - [x] `node --check` clean; both failure modes reproduced from live run logs - [ ] Real e2e: re-trigger `/batch` after merge (batch labels already on #13533/#13526/#13530/#13532) 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Jm3mCG9okfdGtAXtFaDF9A <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes how the bot-owned rollup branch is updated (force-push) and batch membership is assembled; limited to CI/batch-bot infrastructure but affects preview deploy and batch-merge flows. > > **Overview** > Fixes two production failures in the batch bot’s rollup publish and member listing. > > **Rollup publish:** After local merges, the bot no longer updates `batch/rollup` via the Git refs API (which 422’d because the server never had the merge objects). It now **`git push --force`** to `HEAD:refs/heads/batch/rollup`, uploading objects and moving the branch in one step under the checkout token. > > **First `/batch` race:** `rebuildAndReport` accepts an optional **`ensureNumber`**. When `/batch` adds a label, the just-labeled PR is fetched with `pr view` and merged into the member list if GitHub’s label index hasn’t caught up yet, so the first add doesn’t report “batch (0): none”. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 0f357bf. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…dev-merge union fixup) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
/batch orgs |
|
🤖 Added #13532 to batch Deploying the combined preview (#13651); |
…edentials Clean merge. credentials_store_test (team/org creds read path) 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.
🧹 Nitpick comments (1)
autogpt_platform/backend/backend/data/user.py (1)
379-441: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit the access resolver into named helpers.
This function exceeds the backend ~40-line limit and combines membership lookup, query construction, and precedence deduplication. Extract those responsibilities so the authorization path remains easier to audit and test. As per coding guidelines, “Keep functions under ~40 lines; extract named helpers when a function grows longer.”
🤖 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/data/user.py` around lines 379 - 441, Refactor get_accessible_credentials into named helpers for active team/org membership lookup, credential owner-clause construction/querying, and precedence-based deduplication. Keep the existing USER > TEAM > ORG ordering, active-status filtering, decryption, and authorization behavior unchanged, while reducing the top-level function below the ~40-line guideline.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.
Nitpick comments:
In `@autogpt_platform/backend/backend/data/user.py`:
- Around line 379-441: Refactor get_accessible_credentials into named helpers
for active team/org membership lookup, credential owner-clause
construction/querying, and precedence-based deduplication. Keep the existing
USER > TEAM > ORG ordering, active-status filtering, decryption, and
authorization behavior unchanged, while reducing the top-level function below
the ~40-line guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e33fe5f1-f0a7-4b22-9de0-113b12061be2
📒 Files selected for processing (3)
autogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.pyautogpt_platform/backend/backend/data/user_test.py
📜 Review details
⏰ Context from checks skipped due to timeout. (13)
- GitHub Check: check API types
- GitHub Check: Seer Code Review
- GitHub Check: end-to-end tests
- GitHub Check: type-check (3.13)
- GitHub Check: type-check (3.11)
- GitHub Check: type-check (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: Analyze (typescript)
- GitHub Check: Analyze (python)
- GitHub Check: Check PR Status
- GitHub Check: copilot-setup-steps
🧰 Additional context used
📓 Path-based instructions (5)
autogpt_platform/backend/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
autogpt_platform/backend/**/*.py: Use Python 3.11 (required; managed by Poetry via pyproject.toml) for backend development
Always run 'poetry run format' (Black + isort) before linting in backend development
Always run 'poetry run lint' (ruff) after formatting in backend development
autogpt_platform/backend/**/*.py: Usepoetry run ...command for executing Python package dependencies
Use top-level imports only — avoid local/inner imports except for lazy imports of heavy optional dependencies likeopenpyxl
Use absolute imports withfrom backend.module import ...for cross-package imports; single-dot relative imports are acceptable for sibling modules within the same package; avoid double-dot relative imports
Do not use duck typing — avoidhasattr/getattr/isinstancefor type dispatch; use typed interfaces/unions/protocols instead
Use Pydantic models over dataclass/namedtuple/dict for structured data
Do not use linter suppressors — no# type: ignore,# noqa,# pyright: ignore; fix the type/code instead
Prefer list comprehensions over manual loop-and-append patterns
Use early return with guard clauses first to avoid deep nesting
Use%sfor deferred interpolation indebuglog statements for efficiency; use f-strings elsewhere for readability (e.g.,logger.debug("Processing %s items", count)vslogger.info(f"Processing {count} items"))
Sanitize error paths by usingos.path.basename()in error messages to avoid leaking directory structure
Be aware of TOCTOU (Time-Of-Check-Time-Of-Use) issues — avoid check-then-act patterns for file access and credit charging
Usetransaction=Truefor Redis pipelines to ensure atomicity on multi-step operations
Usemax(0, value)guards for computed values that should never be negative
Keep files under ~300 lines; if a file grows beyond this, split by responsibility (extract helpers, models, or a sub-module into a new file)
Keep functions under ~40 lines; extract named helpers when a function grows longer
...
Files:
autogpt_platform/backend/backend/data/user_test.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.py
autogpt_platform/backend/backend/data/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All data access in backend requires user ID checks; verify this for any 'data/*.py' changes
Files:
autogpt_platform/backend/backend/data/user_test.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.py
autogpt_platform/{backend,autogpt_libs}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Format Python code with
poetry run format
Files:
autogpt_platform/backend/backend/data/user_test.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.py
autogpt_platform/**/data/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
For changes touching
data/*.py, validate user ID checks or explain why not needed
Files:
autogpt_platform/backend/backend/data/user_test.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.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/data/user_test.py
🧠 Learnings (13)
📚 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/data/user_test.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.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/data/user_test.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.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/data/user_test.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.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/data/user_test.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.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/data/user_test.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.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/data/user_test.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.py
📚 Learning: 2026-04-21T04:35:34.710Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 12865
File: autogpt_platform/backend/backend/data/credit.py:1584-1584
Timestamp: 2026-04-21T04:35:34.710Z
Learning: When reviewing this codebase, don’t flag snake_case attribute names (e.g., `subscription_tier`, `stripe_customer_id`, `top_up_config`) on the app-layer Pydantic `User` model as “wrong” field names. These are correct for the app-layer model and are expected to be mapped from the Prisma-layer camelCase fields (e.g., `subscriptionTier`, `stripeCustomerId`) inside methods like `User.from_db()`. Only Prisma-returned/raw objects would use camelCase, but functions like `get_user_by_id(user_id: str)` are expected to return the Pydantic app-layer model.
Applied to files:
autogpt_platform/backend/backend/data/user_test.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.py
📚 Learning: 2026-05-07T15:32:39.703Z
Learnt from: majdyz
Repo: Significant-Gravitas/AutoGPT PR: 13033
File: autogpt_platform/backend/backend/data/generate_data.py:111-117
Timestamp: 2026-05-07T15:32:39.703Z
Learning: When reviewing the Python data-generation layer, do not treat missing `user_id`/user filtering in calls to graph-metadata resolvers as a security issue if the `graph_id` inputs are already guaranteed to be user-scoped by earlier upstream SQL (e.g., `WHERE "userId" = ...`). In particular, `_resolve_agent_name(graph_id)` in `generate_data.py` correctly calls `get_graph_metadata(graph_id=graph_id)` without a `user_id` parameter by design, because name resolution must also work for user-executed shared/marketplace agents that the user may not own.
Applied to files:
autogpt_platform/backend/backend/data/user_test.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.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/data/user_test.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.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/data/user_test.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.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/data/user_test.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.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/data/user_test.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.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/data/user_test.pyautogpt_platform/backend/backend/data/db_manager.pyautogpt_platform/backend/backend/data/user.py
🔇 Additional comments (4)
autogpt_platform/backend/backend/data/user.py (2)
220-248: LGTM!
338-358: LGTM!autogpt_platform/backend/backend/data/user_test.py (1)
658-686: LGTM!autogpt_platform/backend/backend/data/db_manager.py (1)
136-136: LGTM!Also applies to: 311-317, 613-617
| **Caller must already hold ``locked_user_integrations(user_id)``.** | ||
| """ | ||
| all_credentials = await self._get_persisted_user_creds_unlocked(user_id) | ||
| all_credentials = await self._get_accessible_creds_unlocked(user_id) |
There was a problem hiding this comment.
🤖 🔴 Blocker: Widening the reader here makes OAuth refresh of a TEAM/ORG credential destroy the shared token — the PR description's "no corruption possible" doesn't hold. In creds_manager._refresh_locked the provider call comes first: fresh = await oauth_handler.refresh_tokens(credentials) rotates (and invalidates) the old refresh token upstream, and only then does store.update_creds → _get_persisted_user_creds_unlocked (USER-only) fail to find the id and raise ValueError. The new tokens are discarded while the old refresh token is already dead at the provider, so the team credential is bricked for every member with no API path to recover it.
It also fails open, not closed: executor/manager.py catches that ValueError from creds_manager.acquire and, when the field has a default, sets input_data[field_name] = None and runs the block with no credentials.
(Separate failure mode from the already-reported delete/revoke issue on this line — same root cause, but this one loses data rather than no-op'ing.) (flagged by: Claude + Codex)
| seen_ids.add(row.id) | ||
| deduped.append(row) | ||
|
|
||
| return _decrypt_credential_rows(deduped, user_id) |
There was a problem hiding this comment.
🤖 🟠 Should Fix: The returned Credentials carry the payload's id, not the DB row id, so TEAM/ORG rows are unreachable by the id-based lookup this PR is wiring up. set_user_credentials writes "id": cred.id explicitly, so USER rows happen to agree — but scoped_credentials.create_credential (the only TEAM/ORG create path) lets Prisma generate id via @default(uuid()) and stores the caller's payload unchanged. get_creds_by_id(user_id, <row id handed out at creation>) then returns None. Worse, _BaseCredentials.id is default_factory=lambda: str(uuid4()), so a payload without an id yields a different id on every read — no node's stored credentials_meta.id can ever match.
The dedup just above compounds it: it keys on row.id while every consumer matches on the payload id, so the two are guarding different keys. Stamp the row id onto the validated credential (or make creation write the same id into both). (flagged by: Codex)
| if org_ids: | ||
| owner_clauses.append({"ownerType": "ORG", "ownerId": {"in": org_ids}}) | ||
|
|
||
| rows = await prisma.integrationcredential.find_many( |
There was a problem hiding this comment.
🤖 🟠 Should Fix: Resolution is org-context-free — it returns credentials for every org/team the user is an ACTIVE member of, ignoring the active RequestContext.org_id/team_id that scopes every other resource, and that the sibling integrations/scoped_credentials.py explicitly enforces (organizationId + active team_id).
A user who belongs to orgs X and Y, working in org X, sees org Y's TEAM/ORG credentials in the picker and can bind one into an org-X graph — org Y's secret is then exercised, logged and billed inside a tenant whose admins were never granted it. Membership-derived resolution is the right call for the executor (no request context), but the listing surface has context available and should narrow to it. (flagged by: Claude)
|
|
||
| assert [c.id for c in result] == ["u"] | ||
| assert ( | ||
| mock_prisma.teammember.find_many.call_args.kwargs["where"]["status"] |
There was a problem hiding this comment.
🤖 🟠 Should Fix: These tests never assert the authorization predicate they exist to protect. The membership where is only checked for ["status"] == "ACTIVE" — nothing asserts ["userId"] == user_id. Delete "userId": user_id from teammember.find_many/orgmember.find_many and all four tests still pass, even though that change resolves every ACTIVE membership platform-wide and hands every user every team's credentials.
credentials_store_test.py mocks get_accessible_credentials wholesale, so it covers wiring only — nothing in the suite exercises the predicate itself. Add assert where["userId"] == "u1" on both membership queries. (flagged by: Claude)
| The WRITE path (``set_user_credentials``) stays USER-only, so team/org | ||
| rows are never mutated through this read path. | ||
| """ | ||
| team_ids = [ |
There was a problem hiding this comment.
🤖 🟡 Nice to Have: This triples the query count on the hottest credential path. get_creds_by_id → get_all_creds → get_accessible_credentials now issues 3 queries (teammember, orgmember, integrationcredential) where it previously issued 1, and the executor calls it once per credentialed node per execution with no memoization across nodes. Consider resolving membership once per execution, or short-circuiting the two membership queries for users with no team/org rows. (flagged by: Claude)
| Team/org rows resolved here are never written back — the write | ||
| helpers use ``_get_persisted_user_creds_unlocked`` (USER-only). | ||
| """ | ||
| return list(await self.db_manager.get_accessible_credentials(user_id=user_id)) |
There was a problem hiding this comment.
🤖 🟠 Should Fix: The shared-credential delete problem has a second endpoint that the existing Bugbot thread on line 381 doesn't name: the external API. api/external/v1/integrations.py delete_credential resolves via get_creds_by_id(auth.user_id, cred_id) — which this line now widens — so a TEAM/ORG id clears the 404 gate, creds_manager.delete → delete_creds_by_id filters a USER-only list and writes back an unchanged set (silent no-op), and the route unconditionally returns DeleteCredentialResponse(deleted=True, credentials_id=cred_id).
Any API key holding DELETE_INTEGRATIONS gets a false success while the row stays status="active". No provider-side revocation happens here (unlike the main route), so the damage is a lying response rather than a destroyed grant — but both endpoints need the same fix: gate mutation/delete flows on an owner-only lookup rather than the accessible set. (flagged by: Claude)
|
/review |
1 similar comment
|
/review |
|
Queued a review for PR #13532 at 88b8dcc. |
![autogpt-pr-reviewer[bot]](https://avatars.githubusercontent.com/u/130738209?s=60&v=4){kind=small}
There was a problem hiding this comment.
❓ INCONCLUSIVE
You've hit your session limit · resets 9am (UTC)
Risk level: medium | Human review: recommended | Duration: 106s | Reviewed: 88b8dccc
Specialist Reports
| Specialist | Status | Summary |
|---|---|---|
| security | You've hit your session limit · resets 9am (UTC) | |
| architect | You've hit your session limit · resets 9am (UTC) | |
| performance | You've hit your session limit · resets 9am (UTC) | |
| testing | You've hit your session limit · resets 9am (UTC) | |
| quality | You've hit your session limit · resets 9am (UTC) | |
| product | You've hit your session limit · resets 9am (UTC) | |
| discussion | You've hit your session limit · resets 9am (UTC) | |
| ui-reviewer (local) | You've hit your session limit · resets 9am (UTC) | |
| ui-reviewer (hosted) | You've hit your session limit · resets 9am (UTC) |
Quality Checks
- ✅ lint: cd autogpt_platform/frontend && pnpm lint:
cd autogpt_platform/frontend && pnpm lint(0s) - ✅ lint: cd autogpt_platform/backend && poetry run lint:
cd autogpt_platform/backend && poetry run lint(96s) - ✅ typecheck: cd autogpt_platform/frontend && pnpm types:
cd autogpt_platform/frontend && pnpm types(0s) - ✅ test: cd autogpt_platform/frontend && mv .env /tmp/qa-env-stash 2>/dev/null; pnpm test:unit; rc=$?; [ -f /tmp/qa-env-stash ] && mv /tmp/qa-env-stash .env; exit $rc:
cd autogpt_platform/frontend && mv .env /tmp/qa-env-stash 2>/dev/null; pnpm test:unit; rc=$?; [ -f /tmp/qa-env-stash ] && mv /tmp/qa-env-stash .env; exit $rc(0s) - ✅ build: cd autogpt_platform/frontend && pnpm build:
cd autogpt_platform/frontend && pnpm build(0s)
|
/review |
|
Queued a review for PR #13532 at 88b8dcc. |
2 participants
Why / What / How
Why:
IntegrationCredentialhas supportedTEAM/ORGownership since PR1 andbackend.integrations.scoped_credentialsimplemented USER→TEAM→ORG resolution — but the resolver was dead code. The live path (get_user_credentials) only ever returned USER rows, so shared team credentials were invisible to the picker and unusable by executions.What: Read-side resolution only. A user now resolves their own USER creds + TEAM creds (teams where they're an ACTIVE member) + ORG creds (active org membership) for credential listing and execution fetch. Write paths are untouched — personal USER-cred CRUD is byte-for-byte unchanged, and team/org rows can never be mutated through the read path.
How:
data/user.py: newget_accessible_credentials(user_id)— resolves from the user's own memberships (no request context needed, safe for the executor), USER>TEAM>ORG precedence, corrupt-row skip.get_user_credentialsdeliberately stays USER-only: it's the seam the write helpers read-modify-write, so widening it would let a full-list replace clobber shared rows.db_manager.py:get_accessible_credentialsexposed over RPC for the executor.credentials_store.py:_get_all_creds_unlockednow resolves the accessible set; sinceget_creds_by_idfilters it, the executor's fetch-by-id inherits the access check for free — live membership is re-checked on every fetch, so a run scheduled by someone who later left the team stops resolving that team's creds at execution time (conservative policy, flagged for product review).Known gap (deliberate, currently unreachable): an OAuth2 TEAM/ORG credential would fail at token-refresh write-back (
update_credsraises on non-USER ids rather than cloning — no corruption possible). No create path for TEAM/ORG credentials exists yet, so this cannot trigger; the MANAGE_CREDENTIALS slice that adds team-cred creation MUST include refresh write-back for shared creds. Tracked on SECRT-2452.Linear: SECRT-2452 (read half)
Changes 🏗️
backend/data/user.py:get_accessible_credentials+ shared decrypt helper;get_user_credentialsdocstring hardened to explain the USER-only invariantbackend/data/db_manager.py: RPC exposurebackend/integrations/credentials_store.py: read surface resolves accessible set; write helpers explicitly pinned to the USER-only readerChecklist 📋
For code changes:
poetry run pytest backend/data/user_test.py backend/integrations/credentials_store_test.py -q— 21 passedpoetry run formatcleanFor changes touching
data/*.py:get_accessible_credentialstakesuser_idand resolves strictly from that user's ACTIVE team/org memberships — access is derived from membership rows, never from caller-supplied org/team ids.For configuration changes: n/a
🤖 Generated with Claude Code
https://claude.ai/code/session_01Jm3mCG9okfdGtAXtFaDF9A
Note
Medium Risk
Changes credential access for listing and executor fetch-by-id based on membership; scope is read-only but affects security-sensitive integration secrets and execution behavior.
Overview
Team and org integration credentials now show up in listing and can be used at execution time, while personal credential CRUD stays USER-scoped only.
Adds
get_accessible_credentials(user_id), which loads USER credentials plus TEAM/ORG rows from live ACTIVE team/org memberships (no request context), with USER > TEAM > ORG dedup and shared decrypt logic that skips corrupt rows.get_user_credentialsis unchanged in scope and remains the write-path seam.IntegrationCredentialsStore._get_all_creds_unlockedswitches from USER-only reads to the accessible set, soget_creds_by_id/ listing inherit membership checks (including revoking access after someone leaves a team). Exposed onDatabaseManagerRPC for executor workers. Tests cover resolution, non-member isolation, departed members, and store wiring.Reviewed by Cursor Bugbot for commit 88b8dcc. Bugbot is set up for automated code reviews on this repo. Configure here.