Replace helper library with argparse CLI and pydantic config models#56
Merged
Conversation
Remove the `helper` dependency entirely. Add `rejected/config.py` with pydantic v2 models (Config, ConsumerConfig, ConnectionConfig, StatsConfig, StatsdConfig, InfluxDBConfig, ConnectionRef, Settings) and a `load()` function that reads YAML/TOML files. Rewrite `controller.py` with a standalone argparse CLI and signal-handling Controller class. Update `mcp.py` and `process.py` to consume pydantic Config objects instead of helper's dict-like Data objects. Update `testing.py` to use the new Settings class. Update tests to build pydantic Config fixtures directly. Remove the Daemon section from example.yaml. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Removes the hardcoded version string; the installed package version is read at import time. Falls back to 'unknown' if the package metadata is not available (e.g. running directly from source without install). Also fixes two pre-existing lint issues in config.py introduced by the remove-helper commit: removes the obsolete Python version guard around tomllib (always available in 3.11+) and adds `from None` to the AttributeError raise in Settings.__getattr__. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
sprockets_influxdb was an optional dependency that added complexity without being widely used. Removing it simplifies the instrumentation path to statsd-only. - Remove InfluxDBConfig model from config.py - Remove influxdb_measurement from ConsumerConfig - Remove sprockets_influxdb import from process.py - Remove self.influxdb, setup_influxdb(), submit_influxdb_measurement() - Simplify setup_instrumentation() to statsd only - Clean up example.yaml and test fixtures Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 20 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughReplaces the external Changes
Sequence DiagramsequenceDiagram
participant User
participant Main as rejected.controller.main()
participant Parser as ArgumentParser
participant Loader as rejected.config.load()
participant Validator as Config.model_validate()
participant Controller as rejected.controller.Controller
participant MCP as MasterControlProgram
participant Process as Process
User->>Main: invoke CLI
Main->>Parser: parse args
Parser-->>Main: args
Main->>Loader: load(args.config)
Loader->>Loader: read YAML/TOML
Loader->>Validator: validate -> Config instance
Validator-->>Loader: validated Config
Loader-->>Main: Config
Main->>Controller: Controller(args, Config)
Controller->>MCP: instantiate/run MCP
MCP->>MCP: install signal handlers
MCP->>Process: new_process(typed Config, consumer)
Process->>Process: access config.connections / config.consumers / config.stats
Process->>MCP: start child process
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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)
rejected/controller.py (1)
58-69:⚠️ Potential issue | 🟠 MajorStop spawned workers in a
finallyblock.If
self._mcp.run()raises unexpectedly, Line 67 re-raises before the cleanup at Line 68 runs. That can leave consumer subprocesses alive after the controller exits.💡 Suggested fix
try: self._mcp.run() except KeyboardInterrupt: LOGGER.info('Caught CTRL-C, shutting down') except Exception: exc_info = sys.exc_info() if self._sentry_client: LOGGER.debug('Sending exception to sentry') sentry_sdk.capture_exception(exc_info) raise - if self._mcp and self._mcp.is_running: - self._mcp.stop_processes() + finally: + if self._mcp and self._mcp.is_running: + self._mcp.stop_processes()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rejected/controller.py` around lines 58 - 69, Move the worker-cleanup into a finally block so spawned consumers are always stopped even if self._mcp.run() raises; specifically, wrap the run/exception handling (self._mcp.run(), the KeyboardInterrupt except, and the generic except that calls sentry_sdk.capture_exception(exc_info)) in a try...finally and in the finally call self._mcp.stop_processes() only if self._mcp and self._mcp.is_running are true; ensure you still re-raise the original exception after sending to Sentry (capture exc_info with sentry_sdk.capture_exception) before the finally completes.
🧹 Nitpick comments (2)
tests/test_process.py (2)
167-170: Avoid callingget_consumertwice in the same assertion path.Lines 168-169 invoke the method twice; one call is enough and keeps this test
more deterministic.Suggested diff
new_process = self.new_process() - new_process.get_consumer(cfg) - self.assertIsNone(new_process.get_consumer(cfg)) + result = new_process.get_consumer(cfg) + self.assertIsNone(result)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_process.py` around lines 167 - 170, The test calls new_process.get_consumer(cfg) twice; instead call it once, store the result in a local variable (e.g., consumer = new_process.get_consumer(cfg)) and assertIsNone(consumer) to avoid repeated invocation and make the test deterministic; update the test around self.new_process() / get_consumer(cfg) accordingly.
79-81: Use an explicitNonecheck in_make_config.Line 81 currently treats empty dict input like “no input”. Prefer explicit
raw is Noneto avoid accidental fallback behavior.Suggested diff
def _make_config(raw=None) -> config_module.Config: """Build a Config pydantic object from the raw dict.""" - return config_module.Config.model_validate(raw or _CONFIG_RAW) + source = _CONFIG_RAW if raw is None else raw + return config_module.Config.model_validate(source)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_process.py` around lines 79 - 81, The helper _make_config treats any falsy raw (e.g., empty dict) as missing input by using "raw or _CONFIG_RAW"; change this to check explicitly for None so empty dicts are validated as provided. Update the return in _make_config to call Config.model_validate with raw when raw is not None, otherwise pass _CONFIG_RAW (i.e., use an explicit "raw is None" conditional), referencing the _make_config function and Config.model_validate to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rejected/config.py`:
- Around line 157-158: The code assumes `raw` is a mapping and directly calls
`.get()` which will raise AttributeError for non-mapping YAML results; update
the start of the config parsing to validate that `raw` is a mapping (e.g.,
isinstance(raw, Mapping)) and if not raise a ValueError with a clear message (so
controller.main() handles it), then safely compute app_raw and logging_raw from
`raw` using `.get()`; reference the variables `raw`, `app_raw`, and
`logging_raw` in config.py and perform the type check before any `.get()` calls.
In `@rejected/controller.py`:
- Around line 143-156: Validate args.only and args.qty before constructing the
Controller/MCP: check that if args.only is provided its value maps to a known
consumer set (reject unknown values) and that args.qty is not provided without
args.only; on invalid cases call parser.error(...) with a clear message so the
program exits immediately instead of creating Controller(args, cfg) and idling;
add these checks just after config loading and logging setup and before
instantiating Controller (referencing args, args.only, args.qty, parser.error,
and Controller).
In `@rejected/process.py`:
- Around line 1282-1283: The queue_name() method incorrectly falls back to
self.name (the per-worker name) causing each worker to use a different queue;
change the fallback to use the logical consumer identifier by returning
self.consumer_config.queue or self.consumer_name instead of self.name (update
the queue_name method accordingly so it references self.consumer_name as the
implicit queue).
- Around line 604-615: The code currently passes a plain dict as settings
(settings = dict(cfg.config)) but consumers expect a rejected.config.Settings
instance; import Settings from rejected.config and instantiate it from
cfg.config (e.g. settings = Settings(cfg.config) or Settings(**cfg.config)
depending on constructor), then set settings._import_module =
'.'.join(cfg.consumer.split('.')[0:-1]) and pass that Settings instance in
kwargs (the variable names to update are settings, cfg, and kwargs in this
block). Ensure the change mirrors how rejected/testing.py constructs consumers
so attribute-style access works in production and tests.
In `@tests/test_process.py`:
- Around line 48-50: The test fixture's ssl_options dict contains a typo key
'prototcol' which should be 'protocol'; update the ssl_options fixture in
tests/test_process.py (the dict literal containing 'ssl_options') to use the
correct key name 'protocol' so SSL option mapping tests exercise the real option
and don't mask mapping bugs.
---
Outside diff comments:
In `@rejected/controller.py`:
- Around line 58-69: Move the worker-cleanup into a finally block so spawned
consumers are always stopped even if self._mcp.run() raises; specifically, wrap
the run/exception handling (self._mcp.run(), the KeyboardInterrupt except, and
the generic except that calls sentry_sdk.capture_exception(exc_info)) in a
try...finally and in the finally call self._mcp.stop_processes() only if
self._mcp and self._mcp.is_running are true; ensure you still re-raise the
original exception after sending to Sentry (capture exc_info with
sentry_sdk.capture_exception) before the finally completes.
---
Nitpick comments:
In `@tests/test_process.py`:
- Around line 167-170: The test calls new_process.get_consumer(cfg) twice;
instead call it once, store the result in a local variable (e.g., consumer =
new_process.get_consumer(cfg)) and assertIsNone(consumer) to avoid repeated
invocation and make the test deterministic; update the test around
self.new_process() / get_consumer(cfg) accordingly.
- Around line 79-81: The helper _make_config treats any falsy raw (e.g., empty
dict) as missing input by using "raw or _CONFIG_RAW"; change this to check
explicitly for None so empty dicts are validated as provided. Update the return
in _make_config to call Config.model_validate with raw when raw is not None,
otherwise pass _CONFIG_RAW (i.e., use an explicit "raw is None" conditional),
referencing the _make_config function and Config.model_validate to locate the
change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5f00da6-c383-4fd4-816b-858a915ece5d
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
example.yamlpyproject.tomlrejected/__init__.pyrejected/config.pyrejected/controller.pyrejected/data.pyrejected/mcp.pyrejected/process.pyrejected/testing.pytests/test_mcp.pytests/test_process.py
- config.py: raise ValueError when YAML root is not a mapping, preventing AttributeError from bypassing controller error handling - controller.py: validate --only and --qty before constructing the MCP, so unknown consumer names and --qty-without-only fail immediately - process.py: pass config_module.Settings instance to consumers instead of a plain dict, matching the contract documented in config.py and testing.py - process.py: queue_name falls back to self.consumer_name (logical name) not self.name (per-worker name) to avoid each worker consuming from a different queue - tests/test_process.py: fix typo 'prototcol' -> 'protocol' in SSL fixture Co-Authored-By: Gavin M. Roy <gavin@aweber.com>
Owner
Author
|
🤖 This comment was posted by Claude on behalf of @gmr PR Monitor SummaryAll CI checks were passing. Addressed 5 CodeRabbit review comments (commit 91a19ca): Fixes applied
All 225 tests pass locally. All 5 review threads resolved. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
rejected/config.py (1)
149-161:⚠️ Potential issue | 🟠 MajorThe
or {}coercions still let malformed config values through.Line 151 turns falsy YAML roots like
[]orfalseinto{}, so the root-mapping guard never runs. Lines 160-161 do the same forApplicationandLogging, which means values likeApplication: []are silently treated as empty config instead of failing fast. Treat onlyNoneas “empty” and reject any present non-mapping section.Possible fix
elif path.suffix in ('.yaml', '.yml'): with open(path) as f: - raw = yaml.safe_load(f) or {} + raw = yaml.safe_load(f) + if raw is None: + raw = {} @@ - app_raw = raw.get('Application', raw.get('application', {})) or {} - logging_raw = raw.get('Logging', raw.get('logging', {})) or {} + app_raw = raw.get('Application', raw.get('application')) + logging_raw = raw.get('Logging', raw.get('logging')) + + if app_raw is None: + app_raw = {} + elif not isinstance(app_raw, dict): + raise ValueError('Application section must be a mapping') + + if logging_raw is None: + logging_raw = {} + elif not isinstance(logging_raw, dict): + raise ValueError('Logging section must be a mapping')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rejected/config.py` around lines 149 - 161, The current code coerces any falsy YAML values into {} which hides malformed configs; change the logic so that raw = yaml.safe_load(f) (leave it as-is), then set raw = {} only if raw is None, and keep the existing check raise ValueError('Configuration root must be a mapping') if raw is not a dict; similarly, for Application and Logging use raw.get('Application', raw.get('application', None')) and treat None as empty (app_raw = {} only when the section is absent or None) but raise ValueError if the section is present and not a mapping (i.e. if app_raw is not None and not isinstance(app_raw, dict) raise ValueError), and do the same for logging_raw—this preserves path.suffix-based parsing while ensuring app_raw and logging_raw are validated rather than silently coerced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rejected/controller.py`:
- Around line 153-159: The logging.dictConfig call can raise uncaught
exceptions; wrap the cfg.logging branch so dictConfig failures are caught and
routed through the same CLI error path used for config_module.load() in main():
surround logging.config.dictConfig(cfg.logging) with a try/except Exception as
e, and on exception invoke the same error handling/exit logic (same error
message format "Error: ...", same exception/exit code) that main() uses for
config_module.load() failures so malformed logging dicts produce the normalized
CLI error instead of a raw traceback.
---
Duplicate comments:
In `@rejected/config.py`:
- Around line 149-161: The current code coerces any falsy YAML values into {}
which hides malformed configs; change the logic so that raw = yaml.safe_load(f)
(leave it as-is), then set raw = {} only if raw is None, and keep the existing
check raise ValueError('Configuration root must be a mapping') if raw is not a
dict; similarly, for Application and Logging use raw.get('Application',
raw.get('application', None')) and treat None as empty (app_raw = {} only when
the section is absent or None) but raise ValueError if the section is present
and not a mapping (i.e. if app_raw is not None and not isinstance(app_raw, dict)
raise ValueError), and do the same for logging_raw—this preserves
path.suffix-based parsing while ensuring app_raw and logging_raw are validated
rather than silently coerced.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd81742e-599f-4bc7-853b-ee509000546f
📒 Files selected for processing (4)
rejected/config.pyrejected/controller.pyrejected/process.pytests/test_process.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_process.py
- rejected/process.py
A malformed logging dict in the config previously escaped as a raw traceback. Catch ValueError, TypeError, AttributeError, and ImportError from logging.config.dictConfig and route them through sys.exit with the same "Error: ..." format used for config_module.load() failures. Co-Authored-By: Gavin M. Roy <gavin@aweber.com>
Owner
Author
|
🤖 This comment was posted by Claude on behalf of @gmr PR Monitor Update (cycle 2)CodeRabbit reviewed the new HEAD and left one additional comment. Fixed and resolved:
All 225 tests still pass. Waiting for CI and CodeRabbit to review the new HEAD. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rejected/controller.py (1)
58-69:⚠️ Potential issue | 🟠 MajorRun
stop_processes()in afinallyblock.Line 67 re-raises before the cleanup block on Lines 68-69, so an unexpected failure in
self._mcp.run()can leave active consumer processes running without their controller.💡 Suggested fix
try: self._mcp.run() except KeyboardInterrupt: LOGGER.info('Caught CTRL-C, shutting down') except Exception: exc_info = sys.exc_info() if self._sentry_client: LOGGER.debug('Sending exception to sentry') sentry_sdk.capture_exception(exc_info) raise - if self._mcp and self._mcp.is_running: - self._mcp.stop_processes() + finally: + if self._mcp and self._mcp.is_running: + self._mcp.stop_processes()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rejected/controller.py` around lines 58 - 69, The current try/except re-raises exceptions before calling self._mcp.stop_processes(), risking leftover processes; move the cleanup into a finally block so stop_processes() always runs. Specifically, wrap the run/exception handling around self._mcp.run() (keep the KeyboardInterrupt handling and sentry capture using self._sentry_client and sentry_sdk.capture_exception(exc_info) in the except clauses), then in a finally block check if self._mcp and self._mcp.is_running and call self._mcp.stop_processes(); re-raise the exception after ensuring the finally block runs (i.e., don’t rely on code after the try/except for cleanup).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rejected/controller.py`:
- Around line 27-31: The startup window can ignore a SIGTERM because the handler
is installed while self._mcp is still being constructed; modify the init/startup
flow so SIGTERM is honored: either install the SIGTERM handler before
constructing self._mcp in __init__ or make the handler (e.g., _sigterm_handler)
only set a durable flag/attribute like self._terminate_requested (instead of
just logging) and then immediately check that flag right after constructing
self._mcp (and before proceeding) to abort startup/clean up; ensure any places
that continue boot (the code that currently follows creation of self._mcp and
use of self._sentry_client) check self._terminate_requested and perform an
orderly shutdown.
---
Outside diff comments:
In `@rejected/controller.py`:
- Around line 58-69: The current try/except re-raises exceptions before calling
self._mcp.stop_processes(), risking leftover processes; move the cleanup into a
finally block so stop_processes() always runs. Specifically, wrap the
run/exception handling around self._mcp.run() (keep the KeyboardInterrupt
handling and sentry capture using self._sentry_client and
sentry_sdk.capture_exception(exc_info) in the except clauses), then in a finally
block check if self._mcp and self._mcp.is_running and call
self._mcp.stop_processes(); re-raise the exception after ensuring the finally
block runs (i.e., don’t rely on code after the try/except for cleanup).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
A SIGTERM arriving between _setup_signals() and the end of MCP construction was only logged and then ignored, allowing the process to continue booting after a shutdown request. Add a _shutdown_requested flag set by _on_sigterm, and check it at each startup checkpoint in run(): after signal setup and after MCP construction, returning immediately if shutdown was requested. Co-Authored-By: Gavin M. Roy <gavin@aweber.com>
Owner
Author
|
🤖 This comment was posted by Claude on behalf of @gmr PR Monitor Update (cycle 3)CodeRabbit reviewed HEAD e4fad2b and left one additional comment. Fixed and resolved:
All 225 tests still pass. Waiting for CI and CodeRabbit to review the new HEAD. |
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.
Summary
Removes the
helperlibrary dependency and replaces it with native Python tooling:argparsefor the CLI andpydanticfor configuration validation. Also removes the InfluxDB integration (sprockets_influxdb) and switches__version__to useimportlib.metadata.Problem
The
helperlibrary was a decade-old internal utility that provided daemonization, config loading, and a base controller class. It carried a lot of daemon-era baggage (pidfiles, user/group dropping, foreground flags) that no longer fits how rejected is deployed (containers, systemd, always foreground). It also made the config structure opaque — a dict-likeDataobject with no schema.sprockets_influxdbwas an optional instrumentation backend that added significant code complexity for a feature not widely used.Solution
rejected/config.py(new) — Pydantic v2 models for the full config schema:Config— top-level application config (connections, consumers, stats, logging)ConnectionConfig— RabbitMQ connection params (validatespassalias →password)ConsumerConfig— per-consumer settings with typed fieldsStatsdConfig— statsd client settingsStatsConfig— stats aggregatorConnectionRef— typed connection reference in consumer connection listsSettings— dict wrapper for consumer-facing settings (preserves.get()/ attribute access API)load(path)— loads YAML or TOML, validates with pydantic, returns aConfigrejected/controller.py(rewritten) — standaloneControllerclass with:argparseCLI:-c/--config,-o/--only,-q/--qty,-P/--profile,-p/--prepend-path,--version-f/--foregroundand all daemonization removed (always runs in foreground)logging.config.dictConfigfrom the config filerejected/process.py— removedhelper_config.LoggingConfig; subprocess logging now callsdictConfigdirectly. Connection and consumer config access updated to use pydantic model attributes.rejected/mcp.py— updated to work withConfigpydantic model directly instead ofhelper.config.Config.rejected/testing.py—config.Data(...)→config.Settings(...).__version__— replaced hardcoded string withimportlib.metadata.version('rejected').InfluxDB removed —
sprockets_influxdbimport,InfluxDBConfig,setup_influxdb(),submit_influxdb_measurement(), andinfluxdb_measurementconsumer config key all removed. Instrumentation is now statsd-only.Impact
Breaking changes:
-f/--foregroundflag removed — rejected always runs in the foreground nowDaemon:config section ignored (no longer parsed)influxdb_measurementconsumer config key removedhelper.config.Datano longer available; consumers usingself.settingsare unaffected (theSettingswrapper preserves the same interface)sprockets_influxdbno longer installed as a side-effectConfig files require no changes beyond removing the
Daemon:section and anyinfluxdb_measurementkeys. TheApplication:,Connections:,Consumers:, andLogging:structure is unchanged.All 225 tests pass.
Summary by CodeRabbit
New Features
Removals
Chores
Bug Fixes