Review fixes: tooling, enums, util module, mypy, docs (#2, #3, #5, #6, #7, #8)#13
Open
kamil-duszynski-dna wants to merge 9 commits into
Open
Review fixes: tooling, enums, util module, mypy, docs (#2, #3, #5, #6, #7, #8)#13kamil-duszynski-dna wants to merge 9 commits into
kamil-duszynski-dna wants to merge 9 commits into
Conversation
Address reviewer comments on the core breaker: - Split CircuitBreaker into a base + per-mode strategies (_BudgetGuardBreaker / _LoopKillerBreaker); the public constructor dispatches on `mode`, so each strategy is free of mode branching and the `is None` limit checks disappear. - Make Mode an Enum; replace the hand-rolled mode assertion with coerce_mode (accepts a Mode member or its string value). - Move numeric guards to core/_numbers.py: is_positive_integer (tri-state) and is_non_negative_finite; drop the _describe helper. - Remove the dead `return` after _trip (which is NoReturn). - Rename set_token_snapshot -> set_tokens and TokenMetrics/ExtractedTokens fields input/output -> input_tokens/output_tokens. Tests, README and AGENTS.md updated. mypy --strict clean.
- Clarify the type-check statement: the package (src/) is mypy --strict clean via `python -m mypy`; the test suite isn't held to strict typing, so bare `mypy --strict .` reports errors. - Sort the repository-layout tree (dirs before files, core first) and add the previously-missing core/_numbers.py entry.
Drop the four identically-named with_circuit_breaker functions in favour of
per-adapter OOP runners (the word "with" collided with the keyword and the
duplicated names were a code smell):
- Add CircuitBreakerRunner base (_runner.py): captures the breaker config,
mints a fresh breaker per run, gates the preflight estimator, resolves
on_trip, and implements the context-manager protocol.
- Each adapter exposes a distinct subclass with a unified run() method:
OpenAIAgentsCircuitBreakerRunner, LangChainCircuitBreakerRunner,
ClaudeAgentCircuitBreakerRunner, LangGraphCircuitBreakerRunner.
- Canonical usage is a context manager:
with OpenAIAgentsCircuitBreakerRunner(agent) as safe:
await safe.run(...)
Update adapter exports, tests (incl. context-manager coverage), README and
AGENTS.md. mypy --strict clean.
Pins code layout so a contributor's editor settings can't reshape the code. black (line length 88) owns formatting; flake8 catches unused imports / undefined names and defers line length to black. Both ship in the dev extra. Reformats the existing code to the committed config and drops now-unused imports in the langchain wrapper.
Mirror Mode: StopReason is now an Enum (MAX_INPUT_TOKENS, MAX_OUTPUT_TOKENS, MAX_RETRIES, REPEATED_STATE) instead of a string Literal, so the trip reason is self-documenting in types and at call sites. Core trips and comparisons use the members; tests and the README compare against them.
- Rename the module to match its BreakerConfig class and the config vocabulary (was _options.py). - Drop the 'knobs' term everywhere in favour of 'config' (code + AGENTS.md). - Rewrite the BreakerConfig docstring: 'core-breaker knobs, captured...' was unclear; say 'circuit-breaker configuration' plainly. Mode is already an Enum (#9), covering the #6 Enum point.
…tests - Group the internal helpers under monetise_circuit_breaker/util/: core/_numbers.py -> util/numbers.py and _config.py -> util/config.py (drop the underscores; the package marks the whole submodule internal). - Type the numeric guards as Any instead of object. - Simplify finite_number to reuse is_number (keeps the deliberate bool rejection that the reviewer's snippet would have dropped). - Add tests/util/test_numbers.py covering every guard, incl. bool / NaN / inf / wrong-type edge cases.
mypy now checks both src/ and tests/ in one run (files + mypy_path), so the plain 'mypy' / 'python -m mypy' the reviewer ran no longer reports errors. src/ stays on the full strict config; tests get a relaxed tests.* override (loosely-typed fakes, callbacks asserted to return None) disabling a few test-only error codes. Fixed one genuine test typing gap (TripContext). AGENTS.md updated to describe this accurately.
- Add a 'Development setup' section: clone, venv, pip install -e '.[dev]', and the pytest / mypy / black / flake8 commands. Note that dev pulls in the adapter frameworks (so the whole suite + mypy run) and give a lighter single-adapter install for when a heavy framework (openai-agents) won't resolve. - Show RetryEvent / StopEvent as frozen dataclasses, like TripContext. - Clarify that the on_trip callback (not the runner) may be sync or async. - Name the LangChain on_llm_start / on_chat_model_start / on_llm_end hooks as BaseCallbackHandler methods, distinct from on_trip / on_event; qualify the LangGraph stream-event names too. - 'swallowed' -> 'consumed' in the LangChain callback-dispatch note. - Sort the options-reference table by mode (both, budget-guard, loop-killer, wrappers).
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
Addresses a batch of reviewer comments (#2, #3, #5, #6, #7, #8). Each item is a
separate commit. Based on the #9–11 branch.
Changes
pyproject.toml) and flake8 (.flake8); both ship in thedevextra.Format the codebase and drop now-unused imports.
StopReasonenum. ConvertStopReasonfrom a stringLiteralto anEnum(matchingMode), so the trip reason is self-documenting in types._options.py→_config.pyto match theBreakerConfigclass and drop the "knobs" wording everywhere.utilsubmodule. Move the internal helpers undermonetise_circuit_breaker/util/(numbers.py,config.py), type the numericguards as
Any, simplifyfinite_number, and add tests for the guards.python -m mypyrun now type-checks bothsrc/(strict) and
tests/(relaxed override) and passes — the plainmypyinvocation no longer reports errors.
RetryEvent/StopEventas dataclasses, clarify theon_tripsync/async wording, name theLangChain callback hooks, and sort the options table by mode.
Notes
Modewas already anEnum, so the enum-related parts of _options.py issues #6/core/types.py issues #7 were partlycovered by earlier work (src/breaker.py issues #9).
finite_numberkeeps its deliberateboolrejection (the reviewer'sexact snippet would have dropped it) by reusing
is_number.Checks
black --check,flake8,python -m mypy, andpytest(116 passed) are allgreen.