Skip to content

feat: v2 BYOK + multi-tenant credential foundations - #7

Open
rekpero wants to merge 9 commits into
mainfrom
feat/v2-byok-foundations
Open

rekpero wants to merge 9 commits into
mainfrom
feat/v2-byok-foundations

Conversation

@rekpero

@rekpero rekpero commented Jun 22, 2026

Copy link
Copy Markdown
Owner

Summary

Lays the foundations for bring-your-own-key (BYOK) credentials and multi-tenant
deployment. Single-tenant self-host keeps working exactly as before — the new
behavior is gated behind DEPLOYMENT_MODE.

  • Deployment mode: new DEPLOYMENT_MODE (single_tenant default / multi_tenant), documented in .env.example alongside Postgres + Fernet encryption setup.
  • Per-org credentials: replaces the shared CLAUDE_CODE_OAUTH_TOKEN with a CredentialProvider that resolves each org's Anthropic credentials, injected through a DI container.
  • Call-site wiring: agent_pool, planner, and rate_limit_watcher now resolve credentials per org and surface MissingCredentialError rather than spawning agents that would immediately fail.
  • New modules: settings (pydantic-settings), tenancy (resolve_org_id), credentials, secrets, infra/crypto, models, repositories.
  • Persistence: Alembic migrations + config for the Postgres system of record.
  • Docs & tests: v2 planning/strategy/system-design docs and a tests/ suite.

Test plan

  • Single-tenant: existing env-based credentials run agents unchanged.
  • Multi-tenant: alembic upgrade head, connect a per-org key, confirm agents/planner/probe use it.
  • Missing credential surfaces MissingCredentialError instead of a failed agent spawn.

🤖 Generated with Claude Code

Introduce the foundations for bring-your-own-key (BYOK) credentials and
multi-tenant deployment, while keeping single-tenant self-host working unchanged.

- Add DEPLOYMENT_MODE (single_tenant default / multi_tenant) and document the
  per-org credential, Postgres, and Fernet encryption setup in .env.example.
- Replace the shared CLAUDE_CODE_OAUTH_TOKEN with a CredentialProvider that
  resolves each org's Anthropic credentials, injected via a DI container.
- Wire per-org credential resolution into agent_pool, planner, and
  rate_limit_watcher; surface MissingCredentialError instead of spawning
  agents that would immediately fail.
- Add settings (pydantic-settings), tenancy (resolve_org_id), credentials,
  secrets, infra/crypto, models, and repositories modules.
- Add Alembic migrations and config for the Postgres system of record.
- Add tests and v2 planning/strategy/system-design docs.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@rekpero rekpero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Claude BugBot Analysis

Found 4 potential bugs in this PR.

medium: 3 | low: 1

The most significant bugs are a thread-unsafe lazy-init race in the SQLAlchemy engine/session-factory helpers, a non-atomic get-or-create pattern for the bootstrap org that will throw an IntegrityError under concurrent startup in multi-tenant mode, and credential-validation logic that conflates rate-limiting with an invalid key and persists the wrong status. There is also a test-isolation gap where the get_container lru_cache is never cleared between tests.

Comment thread orchestrator/infra/db.py
Comment thread orchestrator/repositories/organization_repo.py
Comment thread orchestrator/credentials/api_key_provider.py Outdated
Comment thread tests/conftest.py
- orchestrator/infra/db.py: guard engine/session-factory lazy init with
  threading.Lock (double-checked locking) to prevent races in multi-threaded
  FastAPI/AgentPool environments
- orchestrator/repositories/organization_repo.py: make get_or_create_default
  atomic by flushing after INSERT and catching IntegrityError with a retry
  SELECT to handle concurrent bootstrap-org creation
- orchestrator/credentials/api_key_provider.py: change _probe() to return a
  tri-state (True/False/None) so 429 and 5xx transient responses no longer
  mark a valid credential as STATUS_INVALID; only 401/403 set STATUS_INVALID
- tests/conftest.py: clear get_container lru_cache alongside get_settings in
  the autouse _clear_settings_cache fixture so container state does not leak
  between tests

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@rekpero rekpero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Claude BugBot Analysis

Found 1 potential bug in this PR.

medium: 1

All four previously-reported bugs are fixed in this PR (double-checked locking in db.py, IntegrityError catch-and-retry in organization_repo, tri-state _probe in api_key_provider, and get_container cache clear in conftest). One new race condition remains: FernetSecretStore._get_or_create_dek lacks the same IntegrityError catch-and-retry that was applied to get_or_create_default().

Comment thread orchestrator/secrets/fernet_store.py
Wrap _get_or_create_dek's INSERT in try/except IntegrityError so
concurrent callers racing to create the same org's DEK don't surface
an unhandled exception — the loser rolls back and re-reads the row the
winner committed, matching the pattern in OrganizationRepository.get_or_create_default().

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@rekpero rekpero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Claude BugBot Analysis

Found 1 potential bug in this PR.

medium: 1

The previously reported race condition in fernet_store.py (_get_or_create_dek with no IntegrityError handling) has been correctly fixed with a try/except IntegrityError block that rolls back and re-reads the winning caller's committed DEK. One new bug was found: the test written to validate this exact fix does not exercise the IntegrityError path at all because the winning DEK is pre-committed before put() is called, causing _get_or_create_dek to short-circuit on the first get_for_org() read.

Comment thread tests/test_fernet_store.py
Mock get_for_org to return None on the first call so that _get_or_create_dek
actually reaches create(), triggering the IntegrityError catch-and-re-read
path that the race condition test was intended to exercise.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@rekpero rekpero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Claude BugBot Analysis

No new bugs found. The previously reported race condition test (PRRT_kwDORZvToM6LYex4) has been properly fixed: get_for_org is now mocked to return None on the first call and delegate to the real implementation on subsequent calls, correctly simulating the race-loser scenario and exercising the IntegrityError catch-and-retry path end-to-end.

No bugs were detected in this PR.

# Conflicts:
#	orchestrator/rate_limit_watcher.py

@rekpero rekpero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Claude BugBot Analysis

Found 3 potential bugs in this PR.

high: 1 | medium: 2

Three issues found: a likely NameError in planner.py where workspace_id is used in _planning_credential_env() but the enclosing function only receives workspace: dict; unhandled MissingCredentialError propagation in the agent spawn and resume paths (unlike the probe path which correctly catches it); and a fragile manual session.rollback() inside session_scope context managers in FernetSecretStore and OrganizationRepository, which works under SQLAlchemy 2.0 autobegin but risks identity-map corruption if ORM objects are referenced after the rollback.

Comment thread orchestrator/planner.py Outdated
Comment thread orchestrator/agent_pool.py Outdated
Comment thread orchestrator/secrets/fernet_store.py
- agent_pool.py: catch MissingCredentialError around per-org credential
  resolution in _spawn_agent and resume_rate_limited_agent so a missing
  BYOK credential fails cleanly (clear log, agent marked failed, issue
  reset to pending) instead of risking an unhandled exception escaping
  the rate-limit watcher loop.
- planner.py: same MissingCredentialError handling in
  _run_planning_agent_impl, mirroring the existing Popen-failure path
  (marks the planning session status=error instead of leaving it stuck
  on 'generating').
- fernet_store.py / organization_repo.py: replace the manual
  session.rollback() on the DEK/default-org INSERT race with a
  session.begin_nested() SAVEPOINT, so only the failed INSERT rolls
  back instead of the entire session_scope() transaction (which could
  silently discard other work already done on that session).
- Add regression tests covering the missing-credential and
  savepoint-vs-full-rollback cases.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@rekpero rekpero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Claude BugBot Analysis

The PR correctly replaces the shared CLAUDE_CODE_OAUTH_TOKEN with a per-org credential provider. The three key improvements are: (1) resume_rate_limited_agent and _run_planning_agent_impl now fully handle MissingCredentialError with graceful cleanup; (2) the manual session.rollback() anti-pattern is replaced with session.begin_nested() SAVEPOINT in both fernet_store.py and organization_repo.py; and (3) single-tenant mode correctly bypasses all DB/crypto infrastructure. No new bugs were found in the added code.

No bugs were detected in this PR.

Verified the two outstanding bugbot threads are already handled by the
existing MissingCredentialError catches in agent_pool.py (_spawn_agent,
resume_rate_limited_agent) and planner.py (_run_planning_agent_impl) — the
planner's `workspace_id` is assigned from `workspace["id"]` before use, and
the credential lookups already fail cleanly instead of raising.

Add regression tests confirming resume_rate_limited_agent and
_run_planning_agent_impl never call Popen and fail cleanly (agent marked
failed / session marked error) when an org has no connected credential,
mirroring the existing _spawn_agent coverage.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@rekpero

rekpero commented Jun 25, 2026

Copy link
Copy Markdown
Owner Author

Re: both outstanding threads (orchestrator/planner.py undefined workspace_id, orchestrator/agent_pool.py uncaught MissingCredentialError) — these were anchored to commit b4ff031. The fix landed one commit later in b3ba0da (before this run started):

  • planner.py: workspace_id = workspace["id"] is assigned at the top of _run_planning_agent_impl (line 230) before the _planning_credential_env(workspace_id) call at line 240 — no NameError. _run_planning_agent_impl also already catches MissingCredentialError and marks the planning session status="error".
  • agent_pool.py: _spawn_agent and resume_rate_limited_agent both already catch MissingCredentialError — resume_rate_limited_agent marks the agent failed, resets the issue to pending, and cleans up the worktree; _spawn_agent's re-raise is caught by every call site (dispatch_implement/dispatch_fix_review/dispatch_resolve_conflict), which already wrap the call in except Exception and clean up.

Added regression tests in 3d36847 (tests/test_agent_pool_credentials.py::test_resume_rate_limited_agent_missing_credential_fails_cleanly, tests/test_planner_credentials.py) covering the resume and planner paths, confirming subprocess.Popen is never invoked and state is reset cleanly. Full suite (25 tests) passes. Resolved both threads.

@rekpero rekpero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Claude BugBot Analysis

Found 1 potential bug in this PR.

high: 1

One logic bug found: in the new MissingCredentialError handler for planning session startup, a cancelled session that also hit a missing credential is incorrectly reset to 'active' status instead of 'error', which could cause a silent infinite retry loop whenever credentials are absent.

Comment thread orchestrator/planner.py Outdated

@rekpero rekpero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Claude BugBot Analysis

Found 1 potential bug in this PR.

medium: 1

One new medium-severity race condition: CredentialRepository.upsert() lacks the SAVEPOINT + IntegrityError guard that two analogous methods in the same PR apply, leaving concurrent first-time credential inserts unprotected. The previously reported HIGH bug (planning session incorrectly set to 'active' on MissingCredentialError + cancelled) has been fixed — the handler now unconditionally sets status='error', and the new test covers the cancelled+missing-credential path explicitly.

Comment thread orchestrator/repositories/credential_repo.py

@rekpero rekpero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Claude BugBot Analysis

No new bugs found in the changed lines. The previously reported race-condition bug in upsert() has been fixed: the INSERT is now wrapped in self.session.begin_nested() (SAVEPOINT) and an IntegrityError is caught to re-read the winner's row, matching the same pattern already used in _get_or_create_dek and OrganizationRepository.get_or_create_default. Corresponding tests have been added in test_fernet_store.py to exercise both concurrent DEK creation and concurrent credential upsert races.

No bugs were detected in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant