Skip to content

Polish: log_format validation, clearer URL errors, status-drop logging - #18

Merged
CryptoFewka merged 1 commit into
mainfrom
fix/review-low-minor
Jun 25, 2026
Merged

Polish: log_format validation, clearer URL errors, status-drop logging#18
CryptoFewka merged 1 commit into
mainfrom
fix/review-low-minor

Conversation

@CryptoFewka

@CryptoFewka CryptoFewka commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

What

The Low/minor findings from the full-main review (polish; no behavior-critical bugs).

  • config: reject an unknown KEYSYNC_LOG_FORMAT. The --log-format flag is enum-restricted by click, but the env-var path wasn't, so KEYSYNC_LOG_FORMAT=yaml silently rendered console output. Now a field_validator fails config (EXIT_CONFIG) for anything but console/json.
  • config: clearer schemeless-URL error. A bare host like console.getoptimum.io (no scheme) was rejected with the generic "must use https"; it now reports the missing scheme specifically.
  • beacon: log known-but-excluded validators. A validator the beacon knows but whose status is outside the owned set (e.g. withdrawal_done) was filtered out with zero diagnostic, so a real exit dropping out of the desired set was invisible. A shared _included helper now logs the count + statuses at info level. (Behavior unchanged; existing status-filter test still passes.)

Intentionally not included (noted as deferred in the review): _read_pubkeys deferring format validation to the beacon (intended design), the theoretical operator_id == 0 falsy case (real operator ids aren't falsy), the internal-only logging.configure(level=...) strictness (no user path sets it), and the dead min_length guard on URL fields (harmless).

Tests

  • test_unknown_log_format_rejected, test_schemeless_url_rejected_with_scheme_message.
  • pytest: 88 passed; ruff + mypy: clean.

Written with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved validator filtering visibility by logging when excluded items are dropped.
    • Added stricter startup validation for API URLs without a scheme, with clearer error messages.
    • Rejected invalid log format values during configuration loading, including environment-based settings.
  • Tests

    • Added coverage for scheme-less URL rejection and invalid log format validation.

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: getoptimum/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4e463468-3849-4190-be45-0b12408524ca

📥 Commits

Reviewing files that changed from the base of the PR and between 9e7993b and b393c65.

📒 Files selected for processing (3)
  • src/optimum_keysync/beacon_client.py
  • src/optimum_keysync/config.py
  • tests/test_config.py

📝 Walkthrough

Walkthrough

BeaconClient now uses a shared helper to filter included validator statuses and logs excluded candidates. Config loading now rejects API URLs without a scheme and validates log_format values against console and json. Tests cover both config validation cases.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Comment @coderabbitai help to get the list of available commands.

…p log

Low/minor findings from the main review:
- config: reject an unknown KEYSYNC_LOG_FORMAT (the --log-format flag is
  enum-restricted by click, but the env path wasn't, so a typo silently
  rendered console output).
- config: a bare host with no scheme now errors with a scheme-specific message
  instead of the generic 'must use https'.
- beacon: log the count + statuses of known-but-excluded validators (e.g.
  withdrawal_done) so a real exit dropping out of the desired set leaves a
  trail instead of vanishing silently (via a shared _included helper).

Adds config tests. 88 passed, ruff and mypy clean.
@CryptoFewka
CryptoFewka force-pushed the fix/review-low-minor branch from 39c4ea6 to b393c65 Compare June 25, 2026 18:32
@CryptoFewka
CryptoFewka merged commit 429a3de into main Jun 25, 2026
2 of 3 checks passed
@CryptoFewka
CryptoFewka deleted the fix/review-low-minor branch June 25, 2026 18:33
@CryptoFewka CryptoFewka mentioned this pull request Jun 25, 2026
CryptoFewka added a commit that referenced this pull request Jun 25, 2026
Rolls up #16 (HTTP client lifecycle), #17 (input/network hardening + beacon
chain_id cross-check), #18 (log_format validation, clearer URL errors,
status-drop logging) for the v1.1.0 release.
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