Harden inputs and network handling (review findings #1-#5) - #17
Merged
Conversation
#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.
📝 WalkthroughWalkthroughThe PR adds chunk-level progress reporting to key registration, adds beacon-chain ID fetching with retry and unified BeaconError handling, checks configured chain_id against the beacon node before plan computation, tightens validator_index validation for indices files, and updates tests and CLI stubs for the new behavior. Sequence Diagram(s)sequenceDiagram
participant CLI
participant ApiClient
participant BatchRegisterKeysAPI
CLI->>ApiClient: batch_register_keys(..., on_progress)
loop each committed chunk
ApiClient->>BatchRegisterKeysAPI: POST chunk
BatchRegisterKeysAPI-->>ApiClient: added/skipped
ApiClient-->>CLI: on_progress(added, skipped)
end
sequenceDiagram
participant Main
participant ComputePlan
participant BeaconClient
participant BeaconNode
participant DesiredState
Main->>ComputePlan: build plan
ComputePlan->>BeaconClient: get_chain_id()
BeaconClient->>BeaconNode: GET /eth/v1/config/deposit_contract
BeaconNode-->>BeaconClient: chain_id
BeaconClient-->>ComputePlan: chain_id
alt beacon error
ComputePlan-->>Main: EXIT_BEACON
else chain_id mismatch
ComputePlan-->>Main: EXIT_CONFIG
else chain_id match
ComputePlan->>DesiredState: resolve(...)
DesiredState-->>ComputePlan: desired validators
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 3
🤖 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 `@src/optimum_keysync/api_client.py`:
- Around line 161-167: The batch count handling in api_client.py still accepts
coerced or negative values because `ApiClient` adds `body.get("added", 0)` and
`body.get("skipped", 0)` via `int(...)`. Update this logic to validate the
response fields are real non-negative integers before updating the running
totals, and raise `ApiError` for booleans, floats, strings, or any negative
counts. Keep the fix near the existing `added`/`skipped` aggregation and
`on_progress` callback so malformed responses cannot affect either the callback
or the returned totals.
In `@src/optimum_keysync/beacon_client.py`:
- Around line 203-206: Reject non-string chain_id values in the beacon response
parsing path before converting them with int(). In BeaconClient’s chain_id
handling, validate that resp.json()["data"]["chain_id"] is a string matching the
expected decimal format, and raise BeaconError for booleans, numbers, or other
malformed scalars instead of letting int() coerce them. Keep the existing
try/except around the parsing logic, but tighten the type check in the same
block so the deposit_contract lookup cannot falsely succeed on bad payloads.
- Around line 183-188: The beacon chain_id retrieval path in BeaconClient should
also convert malformed URL and payload cases into BeaconError so callers can
handle them uniformly. Update the chain_id flow around _fetch_chain_id() and the
surrounding try/except to catch httpx.InvalidURL in addition to the existing
transport/retryable errors, and wrap it as BeaconError. Also tighten the
chain_id parsing in the response handling so only string values from
resp.json()["data"]["chain_id"] are accepted before conversion, rejecting
booleans/numbers and raising BeaconError for invalid payloads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: getoptimum/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e51433be-2496-4457-8e93-9d0b2551b64f
📒 Files selected for processing (9)
src/optimum_keysync/api_client.pysrc/optimum_keysync/beacon_client.pysrc/optimum_keysync/cli.pysrc/optimum_keysync/desired_state.pytests/test_api_client.pytests/test_beacon_client.pytests/test_cli_apply.pytests/test_cli_chain_id.pytests/test_desired_state.py
Comment on lines
+161
to
+167
| try: | ||
| added += int(body.get("added", 0)) | ||
| skipped += int(body.get("skipped", 0)) | ||
| except (TypeError, ValueError) as e: | ||
| raise ApiError(f"unexpected validator-keys/batch response: {body!r}") from e | ||
| if on_progress is not None: | ||
| on_progress(added, skipped) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject coerced or negative batch counts.
Lines 161-163 still rely on int(...), so True, 1.7, "2", or -1 will be accepted and folded into the running totals. That leaves this hardening incomplete and can skew both on_progress and the returned (added, skipped) on malformed API responses.
Suggested fix
body = self._json_obj(resp)
- try:
- added += int(body.get("added", 0))
- skipped += int(body.get("skipped", 0))
- except (TypeError, ValueError) as e:
- raise ApiError(f"unexpected validator-keys/batch response: {body!r}") from e
+ chunk_added = body.get("added", 0)
+ chunk_skipped = body.get("skipped", 0)
+ if (
+ isinstance(chunk_added, bool)
+ or not isinstance(chunk_added, int)
+ or chunk_added < 0
+ or isinstance(chunk_skipped, bool)
+ or not isinstance(chunk_skipped, int)
+ or chunk_skipped < 0
+ ):
+ raise ApiError(f"unexpected validator-keys/batch response: {body!r}")
+ added += chunk_added
+ skipped += chunk_skipped
if on_progress is not None:
on_progress(added, skipped)📝 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.
Suggested change
| try: | |
| added += int(body.get("added", 0)) | |
| skipped += int(body.get("skipped", 0)) | |
| except (TypeError, ValueError) as e: | |
| raise ApiError(f"unexpected validator-keys/batch response: {body!r}") from e | |
| if on_progress is not None: | |
| on_progress(added, skipped) | |
| body = self._json_obj(resp) | |
| chunk_added = body.get("added", 0) | |
| chunk_skipped = body.get("skipped", 0) | |
| if ( | |
| isinstance(chunk_added, bool) | |
| or not isinstance(chunk_added, int) | |
| or chunk_added < 0 | |
| or isinstance(chunk_skipped, bool) | |
| or not isinstance(chunk_skipped, int) | |
| or chunk_skipped < 0 | |
| ): | |
| raise ApiError(f"unexpected validator-keys/batch response: {body!r}") | |
| added += chunk_added | |
| skipped += chunk_skipped | |
| if on_progress is not None: | |
| on_progress(added, skipped) |
🤖 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/api_client.py` around lines 161 - 167, The batch count
handling in api_client.py still accepts coerced or negative values because
`ApiClient` adds `body.get("added", 0)` and `body.get("skipped", 0)` via
`int(...)`. Update this logic to validate the response fields are real
non-negative integers before updating the running totals, and raise `ApiError`
for booleans, floats, strings, or any negative counts. Keep the fix near the
existing `added`/`skipped` aggregation and `on_progress` callback so malformed
responses cannot affect either the callback or the returned totals.
Comment on lines
+183
to
+188
| try: | ||
| return self._fetch_chain_id() | ||
| except _RetryableBeaconError as e: | ||
| raise BeaconError(f"beacon chain_id failed after retries: {e}") from e | ||
| except httpx.TransportError as e: | ||
| raise BeaconError(f"beacon chain_id transport failed after retries: {e}") from e |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import httpx
print("InvalidURL subclass of TransportError:", issubclass(httpx.InvalidURL, httpx.TransportError))
print("InvalidURL MRO:", [cls.__name__ for cls in httpx.InvalidURL.__mro__])
PY
sed -n '172,208p' src/optimum_keysync/beacon_client.pyRepository: getoptimum/optimum-keysync
Length of output: 2103
🏁 Script executed:
python3 - <<'PY'
import httpx, json
print("InvalidURL subclass of TransportError:", issubclass(httpx.InvalidURL, httpx.TransportError))
print("int(True) ->", int(True))
print("int(False) ->", int(False))
print('json.loads("true") ->', json.loads("true"), type(json.loads("true")).__name__)
PYRepository: getoptimum/optimum-keysync
Length of output: 272
🏁 Script executed:
python3 - <<'PY'
import httpx, json
print("InvalidURL subclass of TransportError:", issubclass(httpx.InvalidURL, httpx.TransportError))
print("int(True) ->", int(True))
print("int(False) ->", int(False))
print('json.loads("true") ->', json.loads("true"), type(json.loads("true")).__name__)
PYRepository: getoptimum/optimum-keysync
Length of output: 272
Wrap InvalidURL and non-string chain_id into BeaconError.
httpx.InvalidURLis not ahttpx.TransportError, so malformedbeacon_urlvalues can still bypassexcept BeaconErrorand crash the CLI instead of exiting viaEXIT_BEACON.int(resp.json()["data"]["chain_id"])also accepts JSON booleans and numbers, so malformeddeposit_contractpayloads can be treated as valid. Reject non-string values before converting.
🤖 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/beacon_client.py` around lines 183 - 188, The beacon
chain_id retrieval path in BeaconClient should also convert malformed URL and
payload cases into BeaconError so callers can handle them uniformly. Update the
chain_id flow around _fetch_chain_id() and the surrounding try/except to catch
httpx.InvalidURL in addition to the existing transport/retryable errors, and
wrap it as BeaconError. Also tighten the chain_id parsing in the response
handling so only string values from resp.json()["data"]["chain_id"] are accepted
before conversion, rejecting booleans/numbers and raising BeaconError for
invalid payloads.
Comment on lines
+203
to
+206
| try: | ||
| # The beacon spec returns chain_id as a decimal string, e.g. "1". | ||
| return int(resp.json()["data"]["chain_id"]) | ||
| except (KeyError, TypeError, ValueError) as e: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject non-string chain_id values before calling int().
int() will silently coerce malformed JSON scalars here (true -> 1, 1.5 -> 1). That turns a bad deposit_contract payload into a valid-looking network match instead of the BeaconError this path is supposed to raise.
Suggested fix
try:
# The beacon spec returns chain_id as a decimal string, e.g. "1".
- return int(resp.json()["data"]["chain_id"])
+ raw_chain_id = resp.json()["data"]["chain_id"]
+ if not isinstance(raw_chain_id, str) or not raw_chain_id.isdecimal():
+ raise ValueError(f"invalid chain_id: {raw_chain_id!r}")
+ return int(raw_chain_id)
except (KeyError, TypeError, ValueError) as e:
raise BeaconError(f"beacon deposit_contract malformed: {resp.text!r}") from e📝 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.
Suggested change
| try: | |
| # The beacon spec returns chain_id as a decimal string, e.g. "1". | |
| return int(resp.json()["data"]["chain_id"]) | |
| except (KeyError, TypeError, ValueError) as e: | |
| try: | |
| # The beacon spec returns chain_id as a decimal string, e.g. "1". | |
| raw_chain_id = resp.json()["data"]["chain_id"] | |
| if not isinstance(raw_chain_id, str) or not raw_chain_id.isdecimal(): | |
| raise ValueError(f"invalid chain_id: {raw_chain_id!r}") | |
| return int(raw_chain_id) | |
| except (KeyError, TypeError, ValueError) as e: |
🤖 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/beacon_client.py` around lines 203 - 206, Reject
non-string chain_id values in the beacon response parsing path before converting
them with int(). In BeaconClient’s chain_id handling, validate that
resp.json()["data"]["chain_id"] is a string matching the expected decimal
format, and raise BeaconError for booleans, numbers, or other malformed scalars
instead of letting int() coerce them. Keep the existing try/except around the
parsing logic, but tighten the type check in the same block so the
deposit_contract lookup cannot falsely succeed on bad payloads.
Merged
CryptoFewka
added a commit
that referenced
this pull request
Jun 25, 2026
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
1 participant
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.
What
Addresses the High/Medium findings from the full-
mainreview.#1 (High) —
desired_state._read_indicessilently accepted bad indices.int(row["validator_index"])coercedtrue→1,3.9→3, and accepted negatives, registering/assigning the wrong validator with no error. Now requires a non-negative JSON integer (bool is rejected explicitly, since it's anintsubclass). This is the input path the dashboard boilerplate now steers customers to, so it matters most.#2 (High) —
api_client.batch_register_keyscould crash past theApiErrorboundary.int(body.get("added", 0))raised a rawTypeError/ValueErroron{"added": null}or a non-numeric value. Wrapped to raiseApiError, matchinglist_validators.#3/#4 (Medium) — wrong-network / wrong-beacon-url footgun. The mainnet
chain_id/networkdefaults are convenient but a testnet beacon + omittedchain_idsilently stamped every record with0x1; and a wrongbeacon_urlresolved an empty set that looks like a mass exit. AddedBeaconClient.get_chain_id()(reads/eth/v1/config/deposit_contract); the CLI verifies it matches the configuredchain_idbefore computing a plan. A mismatch exitsEXIT_CONFIG; a non-beacon URL fails the probe withEXIT_BEACON. The defaults stay; this just makes a mismatch loud. (Only runs on beacon-backed inputs; the indices-file path carries its own per-recordchain_id.)#5 (Medium) — partial-progress log understated a mid-register failure.
batch_register_keysnow takes anon_progresscallback fired per committed chunk;_apply_planrecords it, so a failure on chunk 2 of a >500-key import logsregistered_added=500, not0.Tests
New coverage for all five: indices-file rejection (bool/float/negative/string/null), non-numeric batch count → ApiError, on-progress survives a mid-chunk failure,
get_chain_idparse/404/non-JSON,_chain_id_to_intparsing, and CLI chain-id mismatch→EXIT_CONFIG / match→0 / wrong-beacon→EXIT_BEACON.pytest: 107 passedruff+mypy: cleanWritten with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes