[feat] Seed starter credits at signup via budget-capped proxy keys (EE) - #6138
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe pull request adds a configurable starter-credit bridge. It provides an authenticated proxy client, guarded seeding during signup, reconciliation for partial states, and an admin endpoint for manual reconciliation. Starter credits bridge
Estimated code review effort: 5 (Critical) | ~90 minutes Merge Risk: 🟠 High · up to This PR adds automatic starter provider credentials during signup, but unresolved issues could expose virtual keys in logs, modify credentials the system does not own, misapply rollout decisions, and materially delay or disrupt signup. Merge should be blocked until the security, ownership, and signup-path risks are addressed. Sequence Diagram(s)sequenceDiagram
participant SignupService
participant StarterCreditsBridgeService
participant StarterCreditsProxyClient
participant VaultService
SignupService->>StarterCreditsBridgeService: seed_starter_credits_bridge_safely(organization_id, organization_email)
StarterCreditsBridgeService->>StarterCreditsProxyClient: verify team and mint or recover key
StarterCreditsBridgeService->>VaultService: create or repair custom-provider secret
StarterCreditsBridgeService->>StarterCreditsProxyClient: update metadata or block exhausted key
🚥 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 |
mmabrouk
changed the title
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (9)
api/oss/src/utils/env.py (1)
1615-1621: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWarn when
enabledis true but the bridge stays disarmed.If an operator sets
AGENTA_STARTER_CREDITS_BRIDGE_ENABLED=trueand omitsPROXY_PUBLIC_URL,MASTER_KEY, orTEAM_ID,armedreturnsFalse.seed_starter_credits_bridgethen returns at line 93 inapi/ee/src/core/starter_credits_bridge/service.pywithout a log line. The feature is silently inert and the misconfiguration is invisible. Other configs in this file emit a startupwarnings.warnfor surprising states (seeServicesCodeConfig._warn_sandbox_runner_mode, line 380).♻️ Proposed startup warning
model_config = ConfigDict(extra="ignore") + `@model_validator`(mode="after") + def _warn_enabled_but_disarmed(self) -> "StarterCreditsBridgeConfig": + if self.enabled and not self.armed: + warnings.warn( + "AGENTA_STARTER_CREDITS_BRIDGE_ENABLED is true but PROXY_PUBLIC_URL, " + "MASTER_KEY, or TEAM_ID is missing; starter-credit seeding stays off.", + stacklevel=2, + ) + return self + `@property` def armed(self) -> bool:api/ee/tests/pytest/unit/test_starter_credits_bridge_client.py (1)
216-238: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a case for the non-JSON response branch.
StarterCreditsProxyClient._requestmaps a 2xx response with a non-JSON body toProxyRequestError(client.py, lines 140-146). No test covers that branch. A proxy that returns an HTML error page behind a gateway hits it.💚 Proposed test
async def test_non_json_success_body_raises(self): def handler(request: httpx.Request) -> httpx.Response: return httpx.Response(200, text="<html>gateway</html>") client = _client_with_handler(handler) with pytest.raises(ProxyRequestError) as excinfo: await client.get_team_info(team_id="team-1") assert excinfo.value.status_code == 200api/ee/src/core/starter_credits_bridge/service.py (2)
51-54: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winGive the verified-team cache a TTL.
_verified_team_idscaches positive verification for the process lifetime and is never invalidated. If an operator lowers the program team'smax_budgetor adds abudget_durationafter the first successful check, every running API process keeps minting until it restarts._team_ceiling_verifieddescribes the team ceiling as the always-on bound on total exposure, so an unbounded positive cache weakens that guarantee.Store the verification in the shared cache with a short TTL, or record a timestamp per team id and re-check after a few minutes.
102-120: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCheck the existing vault row before the proxy round trip.
_team_ceiling_verifiedperforms a network call to the proxy at line 103. The already-seeded check happens later at line 115. On the first call for a process, an already-seeded organization therefore pays a full proxy round trip inside the signup timeout before returning at line 120. Move the default-project lookup and theget_secret_by_slugcheck above the team-ceiling verification. Both are local and cheap, and neither depends on the proxy.api/ee/tests/pytest/unit/test_starter_credits_bridge_seeding.py (2)
896-903: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExercise the route through the HTTP layer.
The test calls
instance.reconciledirectly. That skips FastAPI request validation, the mounted prefix, and any authorization dependency. The test therefore passes whether or not the endpoint is protected. Usefastapi.testclient.TestClientagainst an app that includesinstance.admin_router, and add a case that asserts an unauthenticated request is rejected.
55-57: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlign
FakeVaultServicewithVaultServicesignatures.
create_secretomitsorganization_idand makesproject_idrequired.delete_secretmakessecret_idkeyword-only. Match the production signatures so the fake detects contract changes.api/ee/src/apis/fastapi/starter_credits_bridge/router.py (1)
19-20: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winType
organization_idasUUID.
organization_idis declared asstr.reconcile_starter_credits_bridgeforwards it todb_manager.get_default_project_by_organization_idand toclient.list_keysas the key alias. A malformed value reaches the database layer before it is rejected. AUUIDfield makes FastAPI return 422 for bad input.♻️ Proposed change
+from uuid import UUID + class StarterCreditsReconcileRequest(BaseModel): - organization_id: str + organization_id: UUIDThe service takes
organization_id: str, so passstr(request_body.organization_id)at the call site.api/ee/src/core/starter_credits_bridge/types.py (1)
44-46: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winProtect
MintedKey.keywithSecretStr.Change the field to
SecretStr. Passminted.key.get_secret_value()at the two current use sites inservice.py.api/ee/src/core/starter_credits_bridge/client.py (1)
66-75: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPin the LiteLLM proxy contract.
Record the deployed LiteLLM version and validate the
key,keys, andteam_inforesponse shapes. The client has no version or schema pin, so proxy upgrades can change these cross-service contracts. Missingkeyfails explicitly, but the seeding service then degrades to no starter credits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: d08dff55-d1aa-499a-bc5e-aa9918fcb8e1
📒 Files selected for processing (11)
api/ee/src/apis/fastapi/starter_credits_bridge/__init__.pyapi/ee/src/apis/fastapi/starter_credits_bridge/router.pyapi/ee/src/core/organizations/service.pyapi/ee/src/core/starter_credits_bridge/__init__.pyapi/ee/src/core/starter_credits_bridge/client.pyapi/ee/src/core/starter_credits_bridge/service.pyapi/ee/src/core/starter_credits_bridge/types.pyapi/ee/src/main.pyapi/ee/tests/pytest/unit/test_starter_credits_bridge_client.pyapi/ee/tests/pytest/unit/test_starter_credits_bridge_seeding.pyapi/oss/src/utils/env.py
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
|
Review requested by @mmabrouk. Generated by OpenAI Codex CLI (gpt-5.6-sol, xhigh reasoning) with full cross-PR context; posted by the orchestrating agent. Findings are being addressed on this branch. Codex review (gpt-5.6-sol, xhigh)Do not merge yet. The implementation has a direct full-refill path after partial spend and a concurrency race that can permanently pair a dead vault credential with a live proxy orphan. The signup hook generally contains asynchronous failures, but policy, ownership, and team checks do not meet the stated fail-closed and exact-identity contracts. Findings
Test gaps
Nits
The bridge-specific module, environment, and origin names correctly avoid the permanent wallet, signup-grant, and builtin namespaces. |
|
Codex review findings addressed in commit 9cd9e0e (see the "Review response" section of the PR body for the finding-by-finding map). The headline change: provisioning is now a row-first protocol — the vault row is created before every mint and carries an HMAC-signed record of the authorized remaining, so the never-increase-the-grant invariant survives every failure boundary (including the delete-then-mint crash and the row-without-key state, which is now assumed spent). "Healthy" requires the exact secret_id pairing recorded at mint; reconcile never creates a first grant from an empty state; the flag outage-cache is per organization; grant and per-key limits moved into the policy payload with strict parsing and fail-closed malformed handling; team verification re-runs on a 10-minute TTL; the PostHog calls run in a worker thread inside the real 10s bound; the alias-conflict match is narrowed to the measured wording; error details are redacted of key material. CodeRabbit majors and nits folded in (origin-checked key actions, velocity slot handback, raise_for_status on the alert webhook, UUID-typed reconcile route with operation_id, unused client methods dropped). 65 unit tests cover the review's test-gap list. |
9cd9e0e to
051a9f3
Compare
d4ebe51 to
227c827
Compare
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
Reviewer guide for this PR, written against the current head - the repair/reconcile machinery was removed in the last commit, so seeding is now a single bounded mint-then-write with no repair path and no admin reconcile route. That deletion is what makes this reviewable: the module is a straight line of gates followed by two writes. The inline notes follow that line in order - where it hooks into signup, each gate and how it fails, the mint call, the row write, and what happens when either half fails.
5224149 to
784e94e
Compare
227c827 to
9dd2c03
Compare
784e94e to
aa04102
Compare
9dd2c03 to
bba94f5
Compare
bba94f5 to
de49aa4
Compare
0575a64 to
818d5a4
Compare
f8d4288 to
7749491
Compare
818d5a4 to
5215139
Compare
7749491 to
4ebe820
Compare
5215139 to
74298e4
Compare
4ebe820 to
4cdc3dd
Compare
74298e4 to
a84533a
Compare
a84533a to
e5d0e74
Compare
4cdc3dd to
3cd3c5a
Compare
3cd3c5a to
d0717cd
Compare
e5d0e74 to
a3fa961
Compare
d0717cd to
f134b5b
Compare
a3fa961 to
ac58713
Compare
f134b5b to
e12188b
Compare
ac58713 to
dfd33cc
Compare
e12188b to
362860c
Compare
dfd33cc to
ab97118
Compare
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
Review round 3 (final heads)
Head dc92975a4d, base managed-secrets at 6c70e7b3c9.
The lane content is byte-identical to round 2 (ab97118eb4). The whole delta since then is the rebase onto the new base. So this round re-checks the properties against the final head and looks at the two remaining gaps.
Tests run read-only: pytest ee/tests/pytest/unit/test_starter_credits_bridge_client.py ee/tests/pytest/unit/test_starter_credits_bridge_seeding.py -q → 77 passed.
Verified addressed
Every CodeRabbit thread on this PR is resolved and the resolutions hold in the code at this head. Spot-checked all of them:
- Key material out of
detail.client.py:107-108scrubssk-patterns from the response body before it becomes an exception message, and truncates. Transport failures carry only the exception class name. - Ownership before destructive proxy actions. Every key the module touches goes through the origin plus organization check on the key's master-key-only metadata. A key without the marker is refused rather than deleted or blocked.
- Ownership no longer reads
header.description._row_originis gone. The description is display only. - PostHog calls run in a worker thread, so the 10 second signup bound can actually preempt them.
- The flag outage cache is keyed per organization.
- Velocity slots are released when an attempt funds no mint.
- The grant env parsing rejects unparseable, non-finite, and non-positive values at import time.
- The Codex review findings are addressed by the row-first protocol. I re-traced the three crash windows below and they hold.
The admin reconcile router that CodeRabbit reviewed is no longer in the diff, which removes that whole HTTP surface and the comments attached to it.
Disclosure check
Clean. I grepped the diff and the whole branch for the program's name, for dollar amounts, and for IP addresses. Nothing. The only hostnames in the diff are fictional test values. No cap number, grant size, or budget lives in source: every policy value arrives in the flag payload. The one exception is the development policy used when the deployment configured no PostHog of its own, which is the right place for a number.
Confirmed properties
- Managed and write-only, both explicit. Single creation site at
service.py:324-342:create_managed_secretwithwrite_only=Trueandmanagement=SecretManagementDTO(manager=STARTER_CREDITS_BRIDGE, policy=MANAGER_ONLY). - Display name.
STARTER_CREDITS_NAME = "Agenta"(service.py:58), used for both the header name andprovider_slug, which matters because the slug is the namespace half of every model key the connection publishes. - Policy source and dev fallback. The condition is exactly
if not env.posthog.api_key_configured(service.py:496), andapi_key_configuredis a fresh field that is true only when this deployment supplied the key (env.py:1363). Using it rather thanposthog.enabledis correct, since the built-in fallback key makesenabledtrue in every checkout. - Skipped cleanly when not armed.
if not config.armed: returnatservice.py:130. OSS never imports the module at all; the import sits behind the edition check incommoners.py. - Nothing can reach signup.
seed_starter_credits_bridge_safelyhas no await outside itstry, wraps everything inasyncio.timeoutplus a bareexcept Exception, and the alert it fires is itself guarded. This matters because the signup path deletes the new user when setup raises. - One grant per organization. Two independent guards, either sufficient: the proxy
key_aliasis the organization id, so a duplicate mint conflicts and is treated as already-seeded without re-minting; and the vault unique index on(project_id, slug)rejects a second row, after which the just-minted key is blocked. Crash after mint and before the row write blocks the key. Crash after the row write and before the mint is unreachable, since the mint strictly precedes the write. - Marker stability.
"starter-credits-bridge"is consistent between the vault manager enum and the proxy origin marker.
Should
1. A deleted policy payload does not fail closed while the cache is warm. service.py:513-530. When PostHog is reachable and the payload has been removed, raw is None, so payload stays None and live_malformed stays False. The next branch then reads the Redis-cached payload and seeds on it. The comment directly above says the opposite, and env.py documents clearing the payload as the redeploy-free kill switch. The window is bounded at the cache TTL of five minutes and get_cache does not renew it, so this is not unbounded. But an operator who pulls the payload to stop seeding will watch it keep seeding, which is the moment they most need it to stop. A malformed payload does fail closed and does ignore the cache, so the fix is small: treat a live None the same way, or set a flag that skips the cache read. No test covers it, because the fixture resets the cache between tests.
2. The alert webhook URL reaches the logs. service.py:690-696. response.raise_for_status() raises httpx.HTTPStatusError, whose message embeds the full request URL, and the handler logs it with exc_info=True. An incoming-webhook URL is a bearer credential, and a revoked webhook returning 404 is the ordinary case, not an edge one. Log the status code and the exception class rather than the exception.
Nit
3. A DB error on the row insert can render the minted key into the log. service.py:107-111 logs the seeding exception with exc_info=True. The vault insert encrypts through a SQLAlchemy bind expression, so the plaintext payload and the crypt key are ordinary bind parameters, and StatementError appends [SQL: ...] [parameters: ...] to its string. Any DBAPI error on that insert, such as a connection drop or a statement timeout, puts both in the log line. This is a pre-existing class rather than something this PR invents, and the same shape already exists for user-created secrets. It is worth noting here only because this path handles a freshly minted funded key. Setting hide_parameters on the engine would close it everywhere at once.
4. A timeout between the mint and the row write skips the block. asyncio.timeout raises CancelledError, which the except Exception at service.py:222 does not catch, so _block_key is skipped. Financially harmless, since the key never left the backend and nobody can spend it. The cost is that it permanently consumes the organization's key_alias, so no later repair could mint for that organization.
5. _release_velocity_slots gives up on the first Redis error. service.py:663-669 uses return inside the loop rather than continue, so a transient failure on the first counter leaves the remaining slots consumed.
6. Velocity keys are built from two separate clock reads. service.py:569-570 calls now() more than once, and again on release. An allow and release pair that straddles an hour boundary decrements a different key than it incremented.
Cross-lane note, not a defect
The seeded model is stored as the slug vertex_ai/gemini-3.6-flash. There is no model name field to put a human name in: CustomModelSettingsDTO carries slug and extras only. The string "Gemini 3.6 Flash" is produced client-side, by stripping the connection namespace off the model key and looking the bare id up in the curated catalog. That lookup lives in the frontend lane, not here. So a user sees the friendly name only once that lane is deployed, and sees the raw slug before it.
Verdict
Safe to merge in order. Findings 1 and 2 are worth fixing, but neither can overspend and neither can break a signup. Finding 1 delays a kill switch by up to five minutes; the enabled flag remains an immediate switch. Finding 2 is a credential in a log rather than a credential on the wire.
1 participant
Context
New eligible EE projects receive a funded provider connection. That credential is issued and maintained by Agenta, so users must be able to run with it without reading, replacing, renaming, probing, or deleting it.
Changes
The starter-credits bridge now creates the row through the typed managed-secret boundary:
The bridge chooses lifecycle ownership and value visibility independently. It keeps a separate proxy-origin identifier for audit metadata and uses user-facing copy for the Vault header.
Vault invalidation now lives at the mutation boundary, so this internal create invalidates the same cached list as public create, update, and delete. The bridge refuses to mint or seed when
AGENTA_SERVICES_INTERNAL_KEYis absent, blank, or a known placeholder.No repair, owner-update, release, delete, or universal bypass path is introduced. This release creates one bounded row once.
Tests / notes
What to QA
manager_only, andwrite_only=True.Depends on #6165 through the
managed-secretsbase.