feat(cli): lazy-resolve operator_id via auth GET /api/v1/me - #4
Conversation
KEYSYNC_OPERATOR_ID is now optional. When omitted, the CLI calls
GET /api/v1/me on KEYSYNC_AUTH_URL (the new required env var) and uses
the operator_id from the response. Explicit env or flag wins, so an
already-configured deployment keeps working unchanged.
The auth Worker validates ovi_* directly against the same
internal.operator_api_keys table billing reads, so /me is a clean
identity-resolution surface that doesn't require any change to how
keysync presents the bearer to billing on subsequent calls.
- New IdentityClient lives in identity_client.py (separate module
from billing_client so the host targeting is obvious at the call
site). Same tenacity/401 retry semantics.
- cli._build_auth_and_billing wraps IdentityClient.get_me when
operator_id is missing. Mapped to EXIT_AUTH on terminal failure.
- config.py: new auth_url field (required, HTTPS-validated +
no-userinfo like billing_url); operator_id: str | None = None.
- Doc fixes: drop "operator UUID" wording (it's a TEXT
auto-increment per billing's migration 000011, not a UUID);
pyproject description rewritten (no more JWT-exchange reference);
quickstart / config table updated; k8s example comment updated.
Depends on getoptimum/optimum-auth/pull/27 deploying first so that
auth.getoptimum.io/api/v1/me exists. CI is unaffected (respx stubs
both endpoints in test_cli_lazy_me.py + test_identity_client.py).
📝 WalkthroughWalkthroughThis PR implements lazy operator_id resolution for the keysync CLI. When Sequence Diagram(s)sequenceDiagram
participant CLI
participant IdentityClient
participant AuthAPI
participant BillingService
CLI->>IdentityClient: get_me(bearer=cfg.api_key)
IdentityClient->>AuthAPI: GET /api/v1/me (Authorization: Bearer ...)
AuthAPI-->>IdentityClient: 200 JSON { operator_id, role }
IdentityClient-->>CLI: MeResponse(operator_id, role)
CLI->>BillingService: request validators for operator_id
BillingService-->>CLI: validators list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/optimum_keysync/cli.py (1)
143-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMap config validation failures to
EXIT_CONFIG.Line 143 builds config outside any CLI error mapping. Now that
auth_urlis required/validated, a missing or invalid--auth-urlwill bubble out as a raw config exception instead of the documented config exit path. Wrap_build_config()in a shared helper that catches the config-layer validation exception and exits withEXIT_CONFIG. Also at:src/optimum_keysync/cli.py:179,src/optimum_keysync/cli.py:219.As per coding guidelines "Validate inputs at boundaries; avoid bare
except:and catch specific exceptions with clear error messages."
🤖 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.
Inline comments:
In `@examples/kubernetes/cronjob.yaml`:
- Line 26: The inline comment next to the ConfigMap name
"optimum-keysync-config" references a wrong env var key ("AUTH_URL"); update
that comment to the correct key name "KEYSYNC_AUTH_URL" and ensure the comment
lists the exact env keys used (e.g., KEYSYNC_BILLING_URL, KEYSYNC_AUTH_URL,
NETWORK, CHAIN_ID, OWNER_NAME, BEACON_URL) so operators copying it aren’t
misled.
In `@src/optimum_keysync/config.py`:
- Around line 77-80: The operator_id field currently allows None but also
accepts an empty string; update the declaration of operator_id in
src/optimum_keysync/config.py to use Pydantic's Field with a default of None and
min_length=1 so that when a string is provided it cannot be empty (i.e., change
operator_id: str | None = None to use Field(default=None, min_length=1) so the
min_length constraint applies to the str branch while still permitting None).
In `@src/optimum_keysync/identity_client.py`:
- Around line 50-58: The constructor for IdentityClient creates an httpx.Client
when client is None but never closes it, leaking sockets; modify IdentityClient
to own and manage the lifecycle of the client: add an ownership flag (e.g.,
self._owns_client = client is None), implement context manager methods __enter__
and __exit__ (or async equivalents if used async) that call a new close() method
which closes the internal httpx.Client only when self._owns_client is True, and
ensure any existing call sites can use "with IdentityClient(...)" or call
close() manually; update __init__, add close(), __enter__, __exit__, and ensure
methods that use self._client remain unchanged.
In `@tests/test_cli_lazy_me.py`:
- Around line 22-36: The tests use a shared hardcoded /tmp indices file via
_BASE_OPTS and _write_empty_indices which causes cross-test interference; change
these to be per-test by making _write_empty_indices accept a Path (no default)
and have tests build their option list from a tmp_path (or
CliRunner.isolated_filesystem()) so each test writes its own indices file and
passes that path into the options instead of the global
"/tmp/keysync-test-indices.json"; update all references to _BASE_OPTS in
tests/test_cli_lazy_me.py (and the calls at the noted locations) to construct
options inside each test using the fixture-provided temp path and call the
revised _write_empty_indices(temp_path) before invoking the CLI.
In `@tests/test_identity_client.py`:
- Around line 1-62: Add unit tests in tests/test_identity_client.py covering the
remaining error paths for IdentityClient.get_me: (1) assert that other 4xx
responses (e.g., 400/403/404) raise IdentityError without retries; (2) simulate
httpx.TransportError (e.g., httpx.ConnectError) and verify it retries like 5xx
and then raises an IdentityError (check route.call_count == 3); (3) mock a 200
JSON response missing the "operator_id" key and assert IdentityError/behavior
when body.get("operator_id") is absent; and (4) mock a non-JSON response (make
resp.json() raise) and assert IdentityClient.get_me handles it by raising
IdentityError; use the same respx.mock / AUTH_URL + "/api/v1/me" route and
existing IdentityError/MeResponse classes to locate where to add each test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: getoptimum/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Lite
Run ID: 1ded1283-cf71-4695-ace4-4222f5b00970
⛔ Files ignored due to path filters (1)
pyproject.tomlis excluded by none and included by none
📒 Files selected for processing (8)
README.mdexamples/kubernetes/cronjob.yamlsrc/optimum_keysync/cli.pysrc/optimum_keysync/config.pysrc/optimum_keysync/identity_client.pytests/test_cli_lazy_me.pytests/test_config.pytests/test_identity_client.py
| envFrom: | ||
| - configMapRef: | ||
| name: optimum-keysync-config # KEYSYNC_BILLING_URL, OPERATOR_ID, NETWORK, CHAIN_ID, OWNER_NAME, BEACON_URL | ||
| name: optimum-keysync-config # KEYSYNC_BILLING_URL, AUTH_URL, NETWORK, CHAIN_ID, OWNER_NAME, BEACON_URL (OPERATOR_ID auto-resolved via /me) |
There was a problem hiding this comment.
Fix env var name in inline ConfigMap comment.
Line 26 uses AUTH_URL, but the actual key is KEYSYNC_AUTH_URL. This comment is likely to be copied and can mislead operators.
Suggested fix
- name: optimum-keysync-config # KEYSYNC_BILLING_URL, AUTH_URL, NETWORK, CHAIN_ID, OWNER_NAME, BEACON_URL (OPERATOR_ID auto-resolved via /me)
+ name: optimum-keysync-config # KEYSYNC_BILLING_URL, KEYSYNC_AUTH_URL, KEYSYNC_NETWORK, KEYSYNC_CHAIN_ID, KEYSYNC_OWNER_NAME, KEYSYNC_BEACON_URL (KEYSYNC_OPERATOR_ID auto-resolved via /me)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: optimum-keysync-config # KEYSYNC_BILLING_URL, AUTH_URL, NETWORK, CHAIN_ID, OWNER_NAME, BEACON_URL (OPERATOR_ID auto-resolved via /me) | |
| name: optimum-keysync-config # KEYSYNC_BILLING_URL, KEYSYNC_AUTH_URL, KEYSYNC_NETWORK, KEYSYNC_CHAIN_ID, KEYSYNC_OWNER_NAME, KEYSYNC_BEACON_URL (KEYSYNC_OPERATOR_ID auto-resolved via /me) |
🤖 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 `@examples/kubernetes/cronjob.yaml` at line 26, The inline comment next to the
ConfigMap name "optimum-keysync-config" references a wrong env var key
("AUTH_URL"); update that comment to the correct key name "KEYSYNC_AUTH_URL" and
ensure the comment lists the exact env keys used (e.g., KEYSYNC_BILLING_URL,
KEYSYNC_AUTH_URL, NETWORK, CHAIN_ID, OWNER_NAME, BEACON_URL) so operators
copying it aren’t misled.
| def __init__( | ||
| self, | ||
| auth_url: str, | ||
| *, | ||
| client: httpx.Client | None = None, | ||
| timeout_seconds: float = 15.0, | ||
| ) -> None: | ||
| self._auth_url = auth_url.rstrip("/") | ||
| self._client = client or httpx.Client(timeout=timeout_seconds) |
There was a problem hiding this comment.
Critical: Resource leak—httpx.Client is never closed.
When client=None, line 58 creates a new httpx.Client that maintains connection pools and must be explicitly closed to release sockets and threads. Per coding guidelines, network resources require context managers.
Impact: Long-running processes or repeated instantiation will leak connections and file descriptors, potentially exhausting system resources.
🔒 Proposed fix: make IdentityClient a context manager
class IdentityClient:
"""Thin client over `auth_url/api/v1/me`."""
def __init__(
self,
auth_url: str,
*,
client: httpx.Client | None = None,
timeout_seconds: float = 15.0,
) -> None:
self._auth_url = auth_url.rstrip("/")
- self._client = client or httpx.Client(timeout=timeout_seconds)
+ self._owns_client = client is None
+ self._client = client or httpx.Client(timeout=timeout_seconds)
+
+ def __enter__(self) -> IdentityClient:
+ return self
+
+ def __exit__(self, *_: object) -> None:
+ if self._owns_client:
+ self._client.close()Then update call sites to use with IdentityClient(...) as client:.
🤖 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 `@src/optimum_keysync/identity_client.py` around lines 50 - 58, The constructor
for IdentityClient creates an httpx.Client when client is None but never closes
it, leaking sockets; modify IdentityClient to own and manage the lifecycle of
the client: add an ownership flag (e.g., self._owns_client = client is None),
implement context manager methods __enter__ and __exit__ (or async equivalents
if used async) that call a new close() method which closes the internal
httpx.Client only when self._owns_client is True, and ensure any existing call
sites can use "with IdentityClient(...)" or call close() manually; update
__init__, add close(), __enter__, __exit__, and ensure methods that use
self._client remain unchanged.
Per CodeRabbit review on #4: - cli._build_config_or_exit wraps pydantic.ValidationError and bare ValueError as EXIT_CONFIG (4) rather than letting the traceback exit 1. A bad --auth-url / missing required flag now matches the documented exit-code contract. - config: empty-string operator_id (e.g. KEYSYNC_OPERATOR_ID="" in a k8s ConfigMap with the entry present but blank) coerces to None via a field_validator. Keeps the CLI's `if not operator_id` lazy- lookup branch reachable for typo'd configs. - test_cli_lazy_me: drop the shared /tmp/keysync-test-indices.json, use a per-test tmp_path fixture so pytest-xdist runs don't collide. - test_identity_client: cover the four branches that weren't exercised — non-401 4xx (no retry), httpx.TransportError (retried), 200 with missing operator_id key, 200 with non-JSON body. - test_config: pin the empty-string + whitespace-only operator_id coercion to None. Skipped (with reasons in the PR thread): - cronjob.yaml comment "AUTH_URL" → "KEYSYNC_AUTH_URL": the existing convention drops the KEYSYNC_ prefix for the whole list; the comment matches its neighbours. - IdentityClient socket leak (close + context-manager): BillingClient and BeaconClient have the same pattern, asymmetric fix would be weird. Filed as a coordinated follow-up.
|
Thanks @coderabbitai. Addressed in dd0b123 (4 of 6, with explicit skip reasons for the other two): Applied:
Skipped with reasons:
Full suite green: 57 tests, ruff + mypy clean. |
|
Tip For best results, initiate chat on the files or code changes.
The four applied items are exactly what was needed:
57/57 green, ruff + mypy clean — LGTM. 🐇✓ ✏️ Learnings added
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_identity_client.py (1)
120-127:⚠️ Potential issue | 🟠 MajorEnforce
IdentityErroron non-JSON/meresponses
tests/test_identity_client.py(lines 120-127) currently allowsc.get_me()to raisejson.JSONDecodeError/ValueErrorviapytest.raises(Exception), so it won’t catch the CLI-breaking behavior.IdentityClient.get_me()callsresp.json()without converting parse failures toIdentityError, and the CLI only handlesIdentityError/AuthError—a non-JSON/mebody can leak a raw JSON exception and crash the command.Suggested test change (after mapping JSON parse failures in
IdentityClient.get_metoIdentityError)- with pytest.raises(Exception) as exc_info: - c.get_me("ovi_live_test") - # If this fires as a raw JSONDecodeError, IdentityClient should be - # tightened to catch + re-raise as IdentityError. Pinning current - # behavior so a future fix flips this assertion deliberately. - import json as _json - assert isinstance(exc_info.value, (IdentityError, _json.JSONDecodeError, ValueError)) + with pytest.raises(IdentityError): + c.get_me("ovi_live_test")🤖 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 `@tests/test_identity_client.py` around lines 120 - 127, IdentityClient.get_me currently calls resp.json() and lets json.JSONDecodeError/ValueError bubble up; update the get_me method to catch JSON parsing exceptions (json.JSONDecodeError and ValueError) around resp.json() and re-raise them as IdentityError (preserving the original exception message/str as context), so callers that only handle IdentityError/AuthError (e.g., the CLI and tests) won't receive raw JSON exceptions; reference the IdentityClient.get_me function, resp.json() call site, and the IdentityError class when making this change.tests/test_cli_lazy_me.py (1)
48-102: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winCover the
_compute_planlazy-lookup path too.These tests only exercise
show, but the PR also changed lazyoperator_idresolution fordiffandsyncthrough_compute_plan. A regression there would slip through becauseshowbypasses that helper. Add onedifforsync --dry-runcase that asserts/meis used whenoperator_idis omitted and that a/me401 still maps toEXIT_AUTH. Also at:src/optimum_keysync/cli.py:191-197,src/optimum_keysync/cli.py:226-236,src/optimum_keysync/cli.py:284-316.🤖 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 `@tests/test_cli_lazy_me.py` around lines 48 - 102, Add a test that exercises the lazy operator_id resolution path in _compute_plan by invoking either the CLI "diff" command or "sync --dry-run" (instead of "show") so the helper is used; stub https://auth.test/api/v1/me and the corresponding billing route (e.g., /operators/42/validators) the same way as the existing tests in tests/test_cli_lazy_me.py, assert that when operator_id is omitted /me is called once and the billing route is hit, and also add a case where /me returns 401 and assert the CLI exits with EXIT_AUTH (exit code 2); this ensures the code paths around _compute_plan in src/optimum_keysync/cli.py (around the referenced blocks) are covered.
🤖 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.
Outside diff comments:
In `@tests/test_cli_lazy_me.py`:
- Around line 48-102: Add a test that exercises the lazy operator_id resolution
path in _compute_plan by invoking either the CLI "diff" command or "sync
--dry-run" (instead of "show") so the helper is used; stub
https://auth.test/api/v1/me and the corresponding billing route (e.g.,
/operators/42/validators) the same way as the existing tests in
tests/test_cli_lazy_me.py, assert that when operator_id is omitted /me is called
once and the billing route is hit, and also add a case where /me returns 401 and
assert the CLI exits with EXIT_AUTH (exit code 2); this ensures the code paths
around _compute_plan in src/optimum_keysync/cli.py (around the referenced
blocks) are covered.
In `@tests/test_identity_client.py`:
- Around line 120-127: IdentityClient.get_me currently calls resp.json() and
lets json.JSONDecodeError/ValueError bubble up; update the get_me method to
catch JSON parsing exceptions (json.JSONDecodeError and ValueError) around
resp.json() and re-raise them as IdentityError (preserving the original
exception message/str as context), so callers that only handle
IdentityError/AuthError (e.g., the CLI and tests) won't receive raw JSON
exceptions; reference the IdentityClient.get_me function, resp.json() call site,
and the IdentityError class when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: getoptimum/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Lite
Run ID: d1da5707-e631-47a5-9fbb-3c26253861ff
📒 Files selected for processing (5)
src/optimum_keysync/cli.pysrc/optimum_keysync/config.pytests/test_cli_lazy_me.pytests/test_config.pytests/test_identity_client.py
|
Tip For best results, initiate chat on the files or code changes.
|
Per CodeRabbit re-review on #4 (post-dd0b123): - IdentityClient.get_me wraps resp.json() in try/except ValueError → IdentityError. A 200 with a non-JSON body (proxy injecting an HTML error page, truncated upstream response) no longer escapes as a raw json.JSONDecodeError past the CLI's except mapping. Test flipped from "any of these exception types" to pytest.raises(IdentityError, match="non-JSON body"). - test_cli_lazy_me adds two diff-path tests so the _compute_plan helper is exercised: happy lazy-/me + 401 → EXIT_AUTH. show bypasses _compute_plan, so a regression there would have slipped through the existing show-only coverage.
|
Thanks @coderabbitai — both findings from the re-review addressed in c0b8b6e:
Full suite green: 59 tests, ruff + mypy clean. |
Per CodeRabbit re-review on #4: the constructor created an httpx.Client when none was injected but never closed it, leaking sockets through process exit. - Track ownership via self._owns_client (True iff we built it). - Add close(): closes the internal client only when owned, so a caller-injected client keeps its own lifecycle. - Add __enter__/__exit__ so `with IdentityClient(...)` works. - Wire cli._build_auth_and_billing to use `with`, which is the only production call site; the /me lookup is one-shot per invocation so the client doesn't need to outlive that block. - Pin the ownership semantics in test_close_closes_internal_client _but_not_injected_one. BillingClient and BeaconClient still have the same pattern; the follow-up at #5 now covers just those two.
#1 desired_state._read_indices: reject a bool/float/negative/string validator_index instead of letting int() silently coerce it to a wrong index (the JSON path is the no-beacon input we steer customers to). #2 api_client.batch_register_keys: wrap int(added/skipped) so a null or non-numeric count surfaces as ApiError, not a raw exception escaping the public boundary. #3/#4 chain_id cross-check: BeaconClient.get_chain_id() reads /eth/v1/config/deposit_contract; the CLI verifies it matches the configured chain_id before computing a plan. Catches a testnet beacon paired with the mainnet default, and a wrong beacon_url (which would otherwise resolve an empty set and look like a mass exit). Keeps the convenience defaults. #5 partial-progress: batch_register_keys takes an on_progress callback invoked per committed chunk, so a mid-register failure logs what actually landed instead of 0. Adds tests for each. 107 passed, ruff and mypy clean.
1 participant
Summary
KEYSYNC_OPERATOR_IDbecomes optional. When omitted, the CLI callsGET /api/v1/meon the new requiredKEYSYNC_AUTH_URLand uses the returnedoperator_idfor subsequent billing URL paths. Explicit env or flag still wins.IdentityClient(src/optimum_keysync/identity_client.py) targets auth.getoptimum.io, separate module frombilling_clientso the host targeting is obvious at the call site. Same tenacity / 401-mapping semantics.Why through auth, not billing
Per the design discussion: this is the first piece of migrating operator-credential validation out of billing into the auth Worker. Billing stays the source of truth for the
internal.operator_api_keystable; the auth Worker now reads it via a newSECURITY DEFINERPG function (getoptimum/optimum-billing/pull/72). Future steps could move more of the credential surface to auth without churning keysync further.Dependencies
public.operator_auth_lookup, rotates ovi_* hashing to peppered HMAC./mehandler that calls the function.KEYSYNC_AUTH_URLdefaults: prodhttps://auth.getoptimum.io, devhttps://dev-auth.getoptimum.io.CI is unaffected — both endpoints are respx-stubbed in the test suite.
Test plan
tests/test_config.py—KEYSYNC_AUTH_URLrequired + HTTPS-validated; missing operator_id accepted; explicit operator_id preservedtests/test_identity_client.py—get_mehappy path + bearer forwarding, empty-operator →IdentityError, 401 →IdentityError, 5xx retries then raisestests/test_cli_lazy_me.py— end-to-end CliRunner:keysync showwithout--operator-idhits auth /me once then billing list; explicit--operator-idskips /me; auth /me 401 →EXIT_AUTH(2)ruff checkclean,mypy src/cleanunset KEYSYNC_OPERATOR_ID && KEYSYNC_AUTH_URL=https://dev-auth.getoptimum.io keysync showlogs "operator_id not configured; resolving via auth /me" then prints the assigned validatorsWritten with Claude Code
Summary by CodeRabbit
New Features
Configuration
Documentation
Tests