[AGE-3945] fix(api): dedupe provider_key secrets on create and delete#5270
Open
sridhar852002 wants to merge 2 commits into
Open
[AGE-3945] fix(api): dedupe provider_key secrets on create and delete#5270sridhar852002 wants to merge 2 commits into
sridhar852002 wants to merge 2 commits into
Conversation
Prevent duplicate standard provider vault rows from leaving agent credential resolution ambiguous after UI delete (Agenta-AI#5255 / AGE-3945). Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
✅ Thanks @sridhar852002! This PR now meets the contribution requirements and has been reopened. A maintainer will review it soon. |
dosubot
Bot
added
the
size:M
github-actions
Bot
added
the
incomplete-pr
|
@sridhar-codes is attempting to deploy a commit to the agenta projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughProvider-key creation now updates matching secrets instead of creating duplicates. Frontend deletion removes all vault rows matching a provider name, with provider ID fallback behavior. ChangesProvider key lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
removed
the
incomplete-pr
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/packages/agenta-entities/src/secret/state/atoms.ts (1)
288-289: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider continuing deletion after individual failures.
The sequential
awaitloop stops on the first error, leaving remaining duplicate IDs undeleted. If one secret was already removed by another path, the loop aborts and the user must retry to clean up the rest. Consider collecting errors and attempting all deletions before throwing.♻️ Proposed refactor: attempt all deletions, then throw aggregated error
- for (const secret_id of secretIds) { - await deleteMutation.mutateAsync({projectId, secret_id}) + const errors: unknown[] = [] + for (const secret_id of secretIds) { + try { + await deleteMutation.mutateAsync({projectId, secret_id}) + } catch (error) { + errors.push(error) + } + } + if (errors.length > 0) { + throw new Error( + `[vault] Failed to delete ${errors.length} of ${secretIds.length} secret(s)` + ) }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8f4cce40-8f88-4e2c-ba4a-d741e0aabc2f
⛔ Files ignored due to path filters (1)
docs/assets/demos/age-3945-provider-key-dedupe.pngis excluded by!**/*.png
📒 Files selected for processing (3)
api/oss/src/core/secrets/services.pyapi/oss/tests/pytest/unit/secrets/test_provider_key_upsert.pyweb/packages/agenta-entities/src/secret/state/atoms.ts
Comment on lines
+28
to
+47
| if create_secret_dto.secret.kind == SecretKind.PROVIDER_KEY: | ||
| existing = await self.secrets_dao.list( | ||
| project_id=project_id, | ||
| organization_id=organization_id, | ||
| ) | ||
| provider_kind = create_secret_dto.secret.data.kind | ||
| for secret in existing: | ||
| if secret.kind != SecretKind.PROVIDER_KEY: | ||
| continue | ||
| existing_kind = getattr(secret.data, "kind", None) | ||
| if existing_kind == provider_kind and secret.id is not None: | ||
| return await self.secrets_dao.update( | ||
| secret_id=secret.id, | ||
| update_secret_dto=UpdateSecretDTO( | ||
| header=create_secret_dto.header, | ||
| secret=create_secret_dto.secret, | ||
| ), | ||
| project_id=project_id, | ||
| organization_id=organization_id, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for uniqueness constraints on secrets model/table
rg -n "UniqueConstraint\|unique_together\|unique=" api/oss/src/core/secrets --type=py | head -20
# Check the real SecretsDAO update method signature and return type
rg -n "async def update" api/oss/src/core/secrets --type=py -A 15 | head -40
# Check for any existing migrations adding uniqueness on secrets
fd -e py . api/oss/src --exec rg -l "secrets" {} \; | head -10Repository: Agenta-AI/agenta
Length of output: 2908
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== secrets core files =="
git ls-files api/oss/src/core/secrets api/oss/src | sed -n '1,200p'
echo
echo "== inspect secrets DTOs / interfaces / services / DAO implementation =="
for f in \
api/oss/src/core/secrets/dtos.py \
api/oss/src/core/secrets/interfaces.py \
api/oss/src/core/secrets/services.py \
api/oss/src/core/secrets/*.py \
api/oss/src/core/secrets/**/*dao*.py \
api/oss/src/core/secrets/**/*repository*.py
do
[ -f "$f" ] && { echo "--- $f"; wc -l "$f"; }
done
echo
echo "== search for uniqueness / locking / transaction hints in secrets core =="
rg -n "UniqueConstraint|unique_together|unique=|upsert|ON CONFLICT|for update|SELECT .*FOR UPDATE|transaction|lock" api/oss/src/core/secrets api/oss/src -g '*.py'
echo
echo "== find secret model definitions and migrations mentioning secrets =="
rg -n "class .*Secret|secrets table|SecretKind|provider_kind|kind" api/oss/src -g '*.py'
fd -a -t f . api/oss/src/migrations api/oss/migrations . | rg 'secrets|secret|migration|alembic|versions'
echo
echo "== read the relevant service file section =="
sed -n '1,180p' api/oss/src/core/secrets/services.py
echo
echo "== read interfaces =="
sed -n '1,200p' api/oss/src/core/secrets/interfaces.py
echo
echo "== read DTOs =="
sed -n '1,240p' api/oss/src/core/secrets/dtos.pyRepository: Agenta-AI/agenta
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== secrets package outline =="
ast-grep outline api/oss/src/core/secrets/services.py --view expanded || true
ast-grep outline api/oss/src/core/secrets/interfaces.py --view expanded || true
ast-grep outline api/oss/src/core/secrets/dtos.py --view expanded || true
ast-grep outline api/oss/src/core/secrets/enums.py --view expanded || true
echo
echo "== locate secrets DAO / model implementations =="
rg -n "class .*Secrets?DAO|class .*Secret.*DAO|def list\\(|def create\\(|def update\\(" api/oss/src -g '*.py' | sed -n '1,200p'
echo
echo "== search for secrets table/model definitions =="
rg -n "SecretKind|provider_kind|secrets? table|class .*Secret|__tablename__.*secret|__tablename__.*secrets" api/oss/src -g '*.py' | sed -n '1,240p'
echo
echo "== search for migrations mentioning secret(s) =="
fd -a -t f api/oss/src | rg '/migrations/|/versions/|alembic|migration|secret'
rg -n "secret|secrets|provider_kind|SecretKind" api/oss/src/migrations api/oss/src -g '*.py' | sed -n '1,240p'
echo
echo "== read services.py relevant section =="
sed -n '1,140p' api/oss/src/core/secrets/services.py
echo
echo "== read interfaces.py =="
sed -n '1,120p' api/oss/src/core/secrets/interfaces.pyRepository: Agenta-AI/agenta
Length of output: 9710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== secrets postgres model =="
sed -n '1,220p' api/oss/src/dbs/postgres/secrets/dbes.py
echo
echo "== secrets postgres DBA =="
sed -n '1,220p' api/oss/src/dbs/postgres/secrets/dbas.py
echo
echo "== secrets postgres DAO =="
sed -n '1,220p' api/oss/src/dbs/postgres/secrets/dao.py
echo
echo "== secrets mappings =="
sed -n '1,180p' api/oss/src/dbs/postgres/secrets/mappings.py
echo
echo "== secrets migrations / alembic mentions =="
fd -a -t f . api/oss/src/dbs api/oss/migrations . | rg 'secrets|secret|alembic|versions|migration' | sed -n '1,200p'
echo
echo "== search for constraints in secrets postgres package =="
rg -n "UniqueConstraint|unique=True|index=True|primary_key=True|ForeignKey|Constraint" api/oss/src/dbs/postgres/secrets api/oss/src/dbs/postgres -g '*.py' | sed -n '1,240p'
echo
echo "== search for transaction / lock usage in secrets DAO =="
rg -n "transaction|begin\\(|for update|with_for_update|lock|acquire|commit|rollback|session" api/oss/src/dbs/postgres/secrets api/oss/src/dbs/postgres -g '*.py' | sed -n '1,240p'Repository: Agenta-AI/agenta
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== service file =="
sed -n '1,140p' api/oss/src/core/secrets/services.py
echo
echo "== provider-key upsert test =="
sed -n '1,240p' api/oss/tests/pytest/unit/secrets/test_provider_key_upsert.py
echo
echo "== any secrets uniqueness tests =="
rg -n "provider_key|upsert|duplicate|unique" api/oss/tests/pytest/unit/secrets -g '*.py' | sed -n '1,200p'Repository: Agenta-AI/agenta
Length of output: 9100
Make the provider-key upsert atomic. SecretsDBE has no uniqueness constraint for (scope, provider_kind), and create_secret() does a separate list() then update()/create() in different sessions. Two concurrent calls can still insert duplicate provider keys. Add a scoped unique constraint or serialize this path with a transaction/row lock.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #5255 / AGE-3945.
When users removed Anthropic connections in the UI,
GET /api/secrets/could still return duplicateprovider_keyrows. The settings UI hides duplicates (standardSecretsAtomuses.find()), so delete only removed one row. Agent runs withconnection.mode: "agenta"then failed with:Changes
VaultService.create_secret): upsert standardprovider_keysecrets by provider kind so duplicates cannot accumulate.deleteSecretAtom): delete all vault secrets matching the provider name, not only the single visible row id.Test plan
cd api && uv run --python 3.13 pytest oss/tests/pytest/unit/secrets/ -v(7 passed, including new upsert tests)ruff format+ruff check --fixon changed Python filesprovider_keysecrets for one projectGET /api/secrets/returns 2 Anthropic rowsGET /api/secrets/returns 0 Anthropicprovider_keyrowsconnection.mode: "agenta"and run — no ambiguous-connection errorDemo
Before/after of the duplicate
provider_keybug and fix:provider_keyrecords after delete → agent run fails as ambiguous.