feat(org): shared LLM provider connections (cloud) - #219
Conversation
Adds org-level shared LLM provider connections so multiple LLM profiles can
reference a single credential (api_key + optional base_url) by id. Rotating the
shared key in one place updates every profile that points at it.
Backend (enterprise app-server), mirroring the existing org LLM-profiles design:
- Store connections as an EncryptedJSON blob on the org row
(org.provider_connections), NOT a relational table. The column is the at-rest
encryption boundary; the api_key rides in cleartext inside the encrypted
envelope, same contract as llm_profiles.
- ProviderConnections container model with create/update/delete/list, per-org
limit, id validation, and secret-safe summaries.
- CRUD router (/api/organizations/{org_id}/provider-connections) mounted in
saas_server. Mutations serialize via SELECT ... FOR UPDATE on the org row.
CRUD requires EDIT_ORG_SETTINGS; listing requires VIEW_ORG_SETTINGS.
- Referential integrity enforced in code: deleting a connection still
referenced by a profile returns 409; both collections share the org-row lock
so the check+delete is atomic.
- Resolution wired at the profile-activation choke point: a linked profile's
provider_connection_id is resolved into concrete credentials before the key
is masked/snapshotted. Dangling reference returns 422. Read-at-use, so key is masked/snapshotted. Dangling reference returnsation 150 adds the org.provider_connections column (no backfill; NULL
reads back as emp reads back as emp reads back as emp reads back as emp reads back as to the
unreleased commit that adds LLM.provider_connection_id
(software-agent-sdk#4492, 73fabfd), since no published release contains it yet.
Pinned in both uv ([tool.uv.sources]) and poetry, including direct deps in
enterprise/pyproject.toml because Poetry resolves the openhands-ai path
dependency's PEP 621 pins. Bumped pillow 12.2.0 -> 12.3.0 to satisfy the
unreleased SDK. Revert to a normal vunreleased SDK. Revert to a normal vunreleased SDK. Revert to a norm munreleased SDK. Revert to a normal vunreleased SDK. Revert to a normal vunrelnd resolution-at-activation (incl. 422
on dangling reference).
Co-authored-by: openhands <openhands@all-hands.dev>
|
|
The Docker image build ran 'poetry install --no-root' against the root pyproject.toml + poetry.lock and failed with 'pyproject.toml changed significantly since poetry.lock was last generated'. The SDK deps were switched to a git rev in pyproject.toml (and uv.lock / enterprise/poetry.lock were regenerated) but the root poetry.lock was not, so the enterprise-server image never built. Regenerate it so the image builds and publishes its sha- tag. Co-authored-by: openhands <openhands@all-hands.dev>
The enterprise stage runs 'poetry export --only main' (with hashes) then 'pip install -r requirements.txt'. With the SDK deps pinned to a git rev, pip aborts: 'Can't verify hashes for these requirements because we don't have a way to hash version control repositories'. openhands-sdk/agent-server/tools are already installed in the base venv from the root 'poetry install', so strip their software-agent-sdk git lines from requirements.txt, exactly as the openhands-ai local path dep is already stripped. Co-authored-by: openhands <openhands@all-hands.dev>
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
juanmichelini
left a comment
juanmichelini
left a comment
There was a problem hiding this comment.
Taste Rating: 🟡 Acceptable — the core design has good taste (mirroring llm_profiles instead of inventing new storage machinery), but there is one deploy-breaking bug and a pile of self-admitted temporary pins sitting between this PR and the merge button.
[CRITICAL ISSUES] (Must fix)
- [enterprise/migrations/versions/150_add_provider_connections_to_org.py, Line 26] Migration revision collision:
mainalready ships150_add_daily_conversation_limit.pywithrevision: '150'. This PR adds a second revision150, also withdown_revision: '149'. Alembic will explode with a duplicate-revision / branched-head error the moment this rolls to any environment. Renumber to151withdown_revision: '150'. The one CI job that would have caught this — "Apply migrations on test DB" — was skipped on this PR, which is exactly how this slipped through 4 failing checks. - [openhands/app_server/settings/provider_connections.py, Line 198] CI is red and mypy found a real bug:
ProviderConnections.list()(line 195) shadows the builtin, so the-> list[...]annotations inside the class now refer to the method (Function ... not valid as a type). Renamelist()to something likeall()/values(), or you will keep tripping over builtins for the life of this module.Lint python,Lint enterprise python,check-package-versions, andcheck-syncare all failing — nothing with four red checks should describe itself as ready. - [pyproject.toml, enterprise/pyproject.toml, uv.lock, poetry.lock] Git-pinned to an unreleased SDK commit: the PR pins
openhands-sdk/openhands-agent-server/openhands-toolsto commit73fabfd(merged 2026-08-19, ~2 days ago) in four manifests, plus a forcedpillow 12.2.0 → 12.3.0bump. Your own description says "before this can ship: swap the git pins back to a normal==<version>pin." Correct. This PR is not mergeable until an SDK release containing #4492 is published and pinned properly. An unreleased commit ~2 days old fails any reasonable supply-chain bar for a credentials-handling feature, even if it is a sibling repo.
[IMPROVEMENT OPPORTUNITIES] (Should fix)
- [enterprise/server/routes/org_profiles.py, Line 151] Bogus lazy import: the comment claims the import of
_load_connectionsis lazy "to keep this module importable if the settings package layout shifts" and to avoid "a hard import cost on the hot non-linked path." There is no circular import here (org_provider_connectionsnever importsorg_profiles), and both routers are imported at server startup anyway. This is a comment justifying a workaround for a problem that doesn't exist — move it to the top of the file and delete the comment. - [enterprise/server/routes/org_provider_connections.py, Line 220] Function-level import:
import uuid as _uuidinside the handler for no reason. Top of file. One line. - [openhands/app_server/settings/provider_connections.py, Line 70] Import-time configuration:
MAX_CONNECTIONS_PER_ORGis frozen at module import, soMAX_PROVIDER_CONNECTIONS_PER_ORGset after startup is silently ignored, and tests must patch a module constant instead of flipping an env var. Read the env at call time, or accept that it's a constant and drop the env-var fiction. - [enterprise/server/routes/org_provider_connections.py, Line 46] Docstring lies about GET: the module docstring says "List/Get: VIEW_ORG_SETTINGS" but there is no single-connection GET endpoint. Either add the endpoint or fix the docstring — comments that describe non-behavior drift and mislead.
[TESTING GAPS]
- The 19 unit tests are genuinely good — real async sessions, real HTTPX round trips, encryption-on-persist verification, the 409 ref-guard, the dangling-to-422 resolution. That's real coverage, not mock theater. But there is no migration test, and the migration is the thing that's broken. Back-to-back deployments (main's
150+ this150) will fail atalembic upgrade— add an integration test that runs the migration chain head-to-head, or at minimum un-skip the "Apply migrations on test DB" job for PRs touchingenterprise/migrations/.
[PR TEMPLATE] Per the repo's review guidelines (custom-codereview-guide.md): this PR does not follow .github/pull_request_template.md — missing Why, Issue Number, How to Test, Video/Screenshots, and Type. (An attempt to convert to draft was not permitted, so this is left as a comment per the guide.) For a backend PR publishing a GHCR image, How to Test with the exact curl commands against the published image would be genuinely useful, not bureaucratic.
[RISK ASSESSMENT]
⚠️ Risk Assessment: 🔴 HIGH
This PR (a) introduces a new at-rest store for API credentials (correctly usingEncryptedJSONand org-rowFOR UPDATElocking — good), (b) contains a migration that will break the next deploy via the duplicate alembic revision, and (c) temporarily depends on an unreleased, 2-day-old SDK commit pinned by SHA across four manifests.
Recommendation: Do not auto-merge. Fix the migration revision, wait for an SDK release containing OpenHands/software-agent-sdk#4492, and get human sign-off on the credential-storage boundary (summaries/response models stay secret-free;expose_secretsonly on the persistence path — verified, keep it that way).
VERDICT:
❌ Needs rework: the design is sound and the tests are good, but the alembic collision is a deploy-breaker, CI is red from a real mypy bug, and the whole thing rides on a dependency the PR itself says must be replaced before merge.
KEY INSIGHT: Elegant design executed one revision number and one release too early — renumber the migration to 151, wait for an SDK release, and this becomes mergeable; as it stands, alembic upgrade fails on any database that has run main's revision 150.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
This review was prepared by an AI agent (OpenHands) on behalf of the user.
|
@OpenHands please address the reviewer concerns |
|
I'm on it! juanmichelini can track my progress at all-hands.dev |
- Migration: renumber to 151 (main already owns revision 150), fixing the duplicate alembic revision that would break deploys; add a unit test pinning revision/down_revision plus an upgrade/downgrade op test, and assert the checked-in versions dir passes the integrity script from the root unit suite. - Pins: replace the temporary git pin (73fabfd) with the published SDK 1.43.0 release (contains software-agent-sdk#4492) in pyproject, uv.lock, and both poetry.lock files; keep pillow 12.3.0 (SDK 1.43.0 requires pillow>=12.3.0 at runtime). - Model: rename ProviderConnections.list() to all() to stop shadowing the builtin and unblock mypy. - Limit: read MAX_PROVIDER_CONNECTIONS_PER_ORG at call time instead of freezing a module-level constant at import. - Router: move the lazy _load_connections import in org_profiles and the function-level uuid import in org_provider_connections to the top of each module; fix the module docstring to stop claiming a single-object GET endpoint exists. Co-authored-by: openhands <openhands@all-hands.dev>
…nto feat/org-provider-connections
|
@juanmichelini Thanks for the review — all concerns addressed in PR TEMPLATE
CRITICAL
SHOULD FIX
Verification
This comment was created by an AI agent (OpenHands) on behalf of the user. |
|
All of juanmichelini's review concerns on PR #219 have been addressed, committed ( Critical items:
Should-fix items: env-var limit now read at call time (with tests for custom/default/invalid values); router docstring fixed (removed the nonexistent single-object GET claim; documented List=VIEW / CRUD=EDIT); lazy imports in Verification: 97 targeted enterprise tests pass (provider connections, migration 151, org profiles, agent profiles), plus full One caveat: the provided PR: #219 |
|
🟢 Good taste — Approve with one cleanup. This is well-built. It doesn't invent a new pattern; it copies the proven Linus's Three Questions:
[IMPROVEMENT OPPORTUNITIES]
[STYLE NOTES]
[TESTING GAPS] None. The suite exercises real code paths (persist→decrypt round-trip, activation resolution, dangling→422, delete→409) and asserts on state, not mocked calls. The 64-iteration default-limit test is a touch slow but proves the real ceiling. [DEPENDENCY CHANGES]
[RISK ASSESSMENT]
VERDICT: KEY INSIGHT:
This review was created by an AI agent (OpenHands) on behalf of the user. |
Co-authored-by: openhands <openhands@all-hands.dev>
2 participants
HUMAN:
Evidence:
To test, go to
https://pr-795.staging.all-hands.dev/canvas/settings/llm
Then click add connection
Then create an LLM and select that provider connection.
AGENT:
Why
Org-level LLM profiles on cloud/SaaS currently carry credential material inline. Rotating a shared key means editing every profile that uses it. A provider connection is an org-level, named bundle of that credential material — an
api_keyplus an optionalbase_url— that multiple LLM profiles can reference by id, so rotating a shared key in one place updates every profile that points at it.This is the cloud/enterprise counterpart to:
LLM.provider_connection_id+ local resolution)Both of those gated the feature off on cloud. This PR implements the cloud backend.
Summary
openhands/app_server/settings/provider_connections.pycontainer model +org.provider_connectionsEncryptedJSONcolumn, migration151), exposed over a CRUD router at/api/organizations/{org_id}/provider-connectionswithSELECT ... FOR UPDATEserialization,EDIT_ORG_SETTINGS/VIEW_ORG_SETTINGSpermissions, a 409 delete-ref guard, and read-at-use credential resolution at the profile-activation choke point (dangling reference → 422).Issue Number
N/A — feature work; no tracking issue. Counterparts: OpenHands/software-agent-sdk#4492, OpenHands/OpenHands#16616.
Design
Mirrors the existing org LLM-profiles design rather than inventing a new one:
EncryptedJSONblob on theorgrow (org.provider_connections), not a relational table. The column is the at-rest encryption boundary, so each connection'sapi_keyrides in cleartext inside the encrypted envelope — the same contractllm_profilesalready uses. Envelope shape:{connections: {<id>: ProviderConnection}}.SELECT ... FOR UPDATEon the org row, so concurrent writes serialize instead of racing.EDIT_ORG_SETTINGS; listing requiresVIEW_ORG_SETTINGS.409. Both collections live on the same org row, so theFOR UPDATElock makes the referrer check and the delete atomic (no TOCTOU window).provider_connection_idis resolved into concrete credentials before the key is masked/snapshotted into the member's settings. A dangling reference returns422. Resolution is read-at-use, so rotating a shared key takes effect the next time a linked profile is activated — nothing is pushed retroactively into running conversations.What's included
openhands/app_server/settings/provider_connections.py—ProviderConnectionscontainer model (create/update/delete/list, per-org limit, id validation, secret-safe summaries).enterprise/server/routes/org_provider_connections.py— CRUD router at/api/organizations/{org_id}/provider-connections, mounted insaas_server.py.enterprise/storage/org.py— newprovider_connectionsEncryptedJSONcolumn.enterprise/migrations/versions/151_add_provider_connections_to_org.py— additive migration, no backfill (NULL reads back as empty). Chains off main's revision150.enterprise/server/routes/org_profiles.py—_resolve_provider_connectionwired intoactivate_profile.How to Test
Unit/integration tests (SQLite in-memory, real async sessions, encryption-on-persist verification):
Migration chain (requires a Postgres DSN in env; CI runs
alembic upgrade head+ downgrade/upgrade round-trip on theApply migrations on test DBjob):Manual API exercise against the published enterprise image (
ghcr.io/openhands/enterprise-server:sha-<this pr>), with an org-admin session:Video/Screenshots
Backend-only change (no UI); behavior is covered by the automated tests above.
Type
Notes
openhands-sdk/openhands-agent-server/openhands-toolsto the unreleased merge commit of software-agent-sdk#4492 (73fabfd). That feature has shipped in SDK 1.43.0, and this PR now pins==1.43.0inpyproject.toml+uv.lock+ bothpoetry.lockfiles.pillowstays at12.3.0because SDK 1.43.0 requirespillow>=12.3.0at runtime.151(chains off main's150). TheApply migrations on test DBCI job runs the full chain on both DB drivers.This PR was created by an AI agent (OpenHands) on behalf of the user.
Enterprise server image for this PR: