docs: warn about registry-owned classification in the transport docstrings - #154
Conversation
…rings A caller-owned Registry owns the failure policy, so passing a bare Registry() to the httpx2/httpx transports, the aiohttp middleware or the requests adapter silently drops HTTP status classification: a returned 503 becomes a success with no error raised anywhere. The integration guides already said so, but the argument docstring — what a reader actually sees while writing the call — did not. Add the warning to all six registry arguments.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (12)
|
| Layer / File(s) | Summary |
|---|---|
Document registry failure policy interlock/integrations/*.py |
Integration docstrings state that caller-provided registries own failure classification. Bare registries classify exceptions only. HttpStatusClassifier() classifies returned 503 responses as failures. |
Document HTTP classifier scope docs/integrations/*, docs/llms-full.txt |
Documentation warns that HttpStatusClassifier can raise AttributeError for non-HTTP results without the required status attribute. |
Clarify registry conflict guidance interlock/integrations/_registry.py, tests/test_integration_registry.py, CHANGELOG.md |
The conflict ValueError and its test identify registry-owned options. The changelog records classifier and configuration pitfalls. |
Estimated code review effort: 1 (Trivial) | ~2 minutes
Possibly related PRs
- bagowix/interlock#149: Establishes the shared-registry behavior clarified by this pull request.
🚥 Pre-merge checks | ✅ 8 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | The title uses a valid Conventional Commit type and describes the changes, but it is 74 characters and exceeds the 72-character limit. | Shorten the summary to 72 characters or fewer while preserving the main change. |
✅ Passed checks (8 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Zero-Dependency Core | ✅ Passed | PR diff shows no changed core files outside interlock/integrations; pyproject.toml keeps [project] dependencies = []; interlock/init.py has no integrations re-export. |
| Changelog Entry | ✅ Passed | CHANGELOG.md contains three bullets under ## [Unreleased], including the user-visible effect that a bare Registry treats returned 503 responses as successes. |
| Docs And Llm Mirror | ✅ Passed | The PR changes docstrings and one ValueError message, with no API or behavioral logic change; all four integration pages and docs/llms-full.txt are updated. |
| Tests Accompany Behaviour Change | ✅ Passed | The only non-documentation production edit changes ValueError wording in _registry.py; tests/test_integration_registry.py was changed in the same PR to assert that wording. |
| Public Api Surface | ✅ Passed | The PR diff from origin/main changes neither interlock/init.py nor interlock/pipeline.py; exported names and public signatures remain unchanged. |
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
- Create stacked PR
- Commit on current branch
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
docs/registry-classifier-docstring
Comment @coderabbitai help to get the list of available commands.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@CHANGELOG.md`:
- Around line 9-18: Rewrite the changelog entry to focus on user impact: explain
that configuring a caller-owned registry previously caused returned HTTP 503
responses to be treated as circuit-breaker successes, while users can now see
guidance during registry configuration and provide HttpStatusClassifier() to
classify those responses. Remove claims that the 503 is hidden or that the
change is about moving warnings between documentation locations.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fa5dfc9f-817c-4be0-aae0-0757e6e971c4
📒 Files selected for processing (5)
CHANGELOG.mdinterlock/integrations/aiohttp.pyinterlock/integrations/httpx.pyinterlock/integrations/httpx2.pyinterlock/integrations/requests.py
📜 Review details
⏰ Context from checks skipped due to timeout. (15)
- GitHub Check: github-advanced-security
- GitHub Check: quality (3.13)
- GitHub Check: quality (3.14)
- GitHub Check: Platform smoke (macos-latest, Python 3.14)
- GitHub Check: Platform smoke (windows-latest, Python 3.14)
- GitHub Check: quality (3.12)
- GitHub Check: Run benchmarks
- GitHub Check: quality (3.14t)
- GitHub Check: quality (3.11)
- GitHub Check: Platform smoke (macos-latest, Python 3.11)
- GitHub Check: Extras (minimum versions)
- GitHub Check: Platform smoke (windows-latest, Python 3.11)
- GitHub Check: Analyze (actions)
- GitHub Check: Coverage
- GitHub Check: Analyze (python)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Support Python 3.11 and newer; use Python 3.11+ features where required.
Keep the core zero-dependency and use only the standard library; external dependencies must be isolated behind optional integrations.
Use injectedClockinstances for all time access; do not calltime.monotonic()orsleep()directly in logic.
Implement the core as an I/O-free state machine with a singlethreading.Lockaround the await-free critical section, never held across the protected call.
UseProtocols for extension points:Clock,SlidingWindow,Storage,FailureClassifier, andEventListener; do not inherit from internal classes.
Expose one publicCircuitBreakerclass for sync and async operation, with separate internal paths selected by coroutine detection.
Expose the public API through the package__init__.py; keep helpers underscore-prefixed and hidden.
Use absolute imports, placed at the top of the file, ordered as standard library, third-party, then local imports with blank lines between groups.
Use a maximum line length of 100 characters, single-quoted strings, f-strings, andpathlib.Pathinstead ofos.path.
Annotate every parameter and return value; use modern generic syntax andX | Noneinstead ofOptional[X].
UseStrEnumor module-level constants instead of magic constants.
When a constructor or function has three or more arguments, pass them by keyword.
Keep functions focused on one job, generally no longer than 20–30 lines, with minimal side effects and extracted repeated loop logic.
Useasync/awaitfor I/O-bound work,asyncio.TaskGroupinstead ofasyncio.gather, andasyncio.to_threadorProcessPoolExecutorfor CPU-bound work.
Do not mix sync and async in one function; never await a sync callable or block on an async callable.
Fail fast on invalid input or state by raising immediately; do not continue with partial results or invented defaults.
Catch only expected exceptions, log them with context, and re-raise; do not use ...
Files:
interlock/integrations/httpx.pyinterlock/integrations/aiohttp.pyinterlock/integrations/requests.pyinterlock/integrations/httpx2.py
{interlock/**/*.py,docs/**/*.md,docs/llms-full.txt,docs/llms.txt}
📄 CodeRabbit inference engine (Custom checks)
When a change affects user-facing behaviour through the public API, integrations, or configuration options, update the relevant page under
docs/and regeneratedocs/llms-full.txt; when adding a new documentation page, list it under## Docsindocs/llms.txt.
Files:
interlock/integrations/httpx.pyinterlock/integrations/aiohttp.pyinterlock/integrations/requests.pyinterlock/integrations/httpx2.py
interlock/**/*.py
📄 CodeRabbit inference engine (Custom checks)
Every production behaviour change in
interlock/must be accompanied by a change undertests/; changes limited to docstrings, comments, or type annotations are exempt. Bug fixes must include at least one regression test that fails without the production fix.Keep the core dependency-free; external dependencies must belong to extras and be imported lazily.
Files:
interlock/integrations/httpx.pyinterlock/integrations/aiohttp.pyinterlock/integrations/requests.pyinterlock/integrations/httpx2.py
⚙️ CodeRabbit configuration file
Core rules (AGENTS.md is authoritative): (1) Zero-dependency core — anything under interlock/ except interlock/integrations/ may import stdlib only. Flag every third-party import as a blocking issue. (2) No fallbacks, no silent excepts, no
a or b or cfor required config or data, no hidden retries. Invalid input or state raises immediately. interlock/_notify.py is the one sanctioned swallow (listener hooks are observability, logged with traceback, BaseException still propagates) — do not suggest generalising or "fixing" it. (3) Time comes only from the injected Clock protocol; direct time.monotonic()/time.sleep() in library logic is a bug. (4) Style: 100-char lines, single quotes, f-strings, pathlib, full annotations,X | NoneneverOptional[X], keyword arguments for calls with 3+ arguments, no magic constants (StrEnum or module constants), functions under ~30 lines. (5) Extension points are Protocols (Clock, SlidingWindow, Storage, FailureClassifier, EventListener) — do not propose inheriting internal classes. (6) Sync and async live in one CircuitBreaker with separate internal paths; never propose Sync*/Async* twins and never mix the two paths in one function. (7) Public API is exported from interlock/init.py; everything else is underscore-prefixed. New public symbols need__all__and a docstring. (8) Python 3.11 is the floor — no 3.12+ syntax or stdlib.
Files:
interlock/integrations/httpx.pyinterlock/integrations/aiohttp.pyinterlock/integrations/requests.pyinterlock/integrations/httpx2.py
interlock/integrations/**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Integration dependencies must be optional extras and imported lazily.
Files:
interlock/integrations/httpx.pyinterlock/integrations/aiohttp.pyinterlock/integrations/requests.pyinterlock/integrations/httpx2.py
⚙️ CodeRabbit configuration file
Optional extras. The third-party import must stay inside this package, must never be re-exported from interlock/init.py, and a missing extra must fail with a clear install hint rather than a fallback. Wrap the dependency behind the project's own types so its objects do not leak into core signatures. Check that the extra is declared in pyproject.toml
[project.optional-dependencies]and documented under docs/integrations/.
Files:
interlock/integrations/httpx.pyinterlock/integrations/aiohttp.pyinterlock/integrations/requests.pyinterlock/integrations/httpx2.py
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Document user-facing changes in English Markdown documentation and keep generated documentation mirrors synchronized.
Files:
CHANGELOG.md
CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Add every change to the
[Unreleased]section underAdded,Fixed, orChanged, explaining user impact rather than only symbol movement.User-facing changes must update the
[Unreleased]section.
Files:
CHANGELOG.md
⚙️ CodeRabbit configuration file
Keep a Changelog format. New entries go under
## [Unreleased]in Added / Fixed / Changed. An entry describes what a user could not do before and can now, not which symbol moved. Only the release commit dates a section and updates the link references.
Files:
CHANGELOG.md
🔇 Additional comments (4)
interlock/integrations/aiohttp.py (1)
124-127: LGTM!interlock/integrations/httpx.py (1)
122-125: LGTM!Also applies to: 216-219
interlock/integrations/httpx2.py (1)
122-125: LGTM!Also applies to: 217-220
interlock/integrations/requests.py (1)
130-133: LGTM!
HttpStatusClassifier reads the status off every result it records, so a
registry configured with it is only safe for breakers that guard HTTP
calls. Borrowing one from the same registry for other work — a
registry.get('db') next to the transports — raises AttributeError on the
first returned value. The shared-registry guides recommended the
classifier without naming that limit; all four now do.
Also spell out the conflict rejected by @reject_registry_options: the
error named the offending options and stopped, leaving the reader to
infer that the injected registry is where they belong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@interlock/integrations/_registry.py`:
- Around line 30-33: Add a regression test under tests/ covering an injected
registry combined with a conflicting option. Assert that the raised ValueError
identifies the conflicting option and instructs callers to configure it on the
registry, preserving the error contract introduced in the registry validation
path.
🪄 Autofix
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 791f6d40-862d-45d8-b043-a1be7ac3c19f
📒 Files selected for processing (7)
CHANGELOG.mddocs/integrations/aiohttp.mddocs/integrations/httpx.mddocs/integrations/httpx2.mddocs/integrations/requests.mddocs/llms-full.txtinterlock/integrations/_registry.py
📜 Review details
⏰ Context from checks skipped due to timeout. (7)
- GitHub Check: Run benchmarks
- GitHub Check: quality (3.14t)
- GitHub Check: Platform smoke (macos-latest, Python 3.11)
- GitHub Check: quality (3.11)
- GitHub Check: Platform smoke (windows-latest, Python 3.14)
- GitHub Check: Platform smoke (macos-latest, Python 3.14)
- GitHub Check: Coverage
⚠️ CI failures not shown inline (2)
GitHub Actions: Code scanning AI findings on PR #154 / 0_github-advanced-security.txt: Code scanning AI findings on PR #154
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mecho "RUNNER_TEMP=$RUNNER_TEMP"�[0m
�[36;1mfind "$RUNNER_TEMP" -maxdepth 1 -type f -name 'git-credentials-*.config' -print -delete�[0m
�[36;1m�[0m
�[36;1m# Generate a unique token and stop processing workflow commands to prevent the runtime from injecting commands�[0m
�[36;1mSTOP_***REDACTED_SECRET_ASSIGNMENT*** /proc/sys/kernel/random/uuid)�[0m
�[36;1m�[0m
�[36;1m# Use a trap to ensure we always resume command processing and check for�[0m
�[36;1m# fallback error annotations, even if the runtime exits with a non-zero code�[0m
�[36;1m# (which would otherwise cause set -e to abort the shell before we get here).�[0m
�[36;1m# The trap preserves the original exit code.�[0m
�[36;1mcopilot_cleanup() {�[0m
�[36;1m �[0m
�[36;1m if [ -n "${GIT_PROXY_PID:-}" ] && kill -0 "$GIT_PROXY_PID" 2>/dev/null; then�[0m
�[36;1m echo "Stopping git-proxy (pid=$GIT_PROXY_PID)..."�[0m
�[36;1m kill "$GIT_PROXY_PID" 2>/dev/null || true�[0m
�[36;1m for _ in {1..25}; do�[0m
�[36;1m if ! kill -0 "$GIT_PROXY_PID" 2>/dev/null; then break; fi�[0m
�[36;1m sleep 0.2�[0m
�[36;1m done�[0m
�[36;1m if kill -0 "$GIT_PROXY_PID" 2>/dev/null; then�[0m
�[36;1m echo "git-proxy did not stop gracefully; forcing termination."�[0m
�[36;1m kill -KILL "$GIT_PROXY_PID" 2>/dev/null || true�[0m
�[36;1m fi�[0m
�[36;1m wait "$GIT_PROXY_PID" 2>/dev/null || true�[0m
�[36;1m fi�[0m
�[36;1m �[0m
�[36;1m echo "::$STOP_***REDACTED_SECRET_ASSIGNMENT***
�[36;1m FALLBACK_FILE="${RUNNER_TEMP}/copilot-fallback-error.txt"�[0m
�[36;1m if [ -f "$FALLBACK_FILE" ]; then�[0m
�[36;1m FALLBACK_MSG=$(head -c 500 "$FALLBACK_FILE" | tr -d '\n\r')�[0m
�[36;1m echo "::error title=Copilot Error::${FALLBACK_MSG}"�[0m
GitHub Actions: Code scanning AI findings on PR #154 / github-advanced-security: Code scanning AI findings on PR #154
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1mecho "RUNNER_TEMP=$RUNNER_TEMP"�[0m
�[36;1mfind "$RUNNER_TEMP" -maxdepth 1 -type f -name 'git-credentials-*.config' -print -delete�[0m
�[36;1m�[0m
�[36;1m# Generate a unique token and stop processing workflow commands to prevent the runtime from injecting commands�[0m
�[36;1mSTOP_***REDACTED_SECRET_ASSIGNMENT*** /proc/sys/kernel/random/uuid)�[0m
�[36;1m�[0m
�[36;1m# Use a trap to ensure we always resume command processing and check for�[0m
�[36;1m# fallback error annotations, even if the runtime exits with a non-zero code�[0m
�[36;1m# (which would otherwise cause set -e to abort the shell before we get here).�[0m
�[36;1m# The trap preserves the original exit code.�[0m
�[36;1mcopilot_cleanup() {�[0m
�[36;1m �[0m
�[36;1m if [ -n "${GIT_PROXY_PID:-}" ] && kill -0 "$GIT_PROXY_PID" 2>/dev/null; then�[0m
�[36;1m echo "Stopping git-proxy (pid=$GIT_PROXY_PID)..."�[0m
�[36;1m kill "$GIT_PROXY_PID" 2>/dev/null || true�[0m
�[36;1m for _ in {1..25}; do�[0m
�[36;1m if ! kill -0 "$GIT_PROXY_PID" 2>/dev/null; then break; fi�[0m
�[36;1m sleep 0.2�[0m
�[36;1m done�[0m
�[36;1m if kill -0 "$GIT_PROXY_PID" 2>/dev/null; then�[0m
�[36;1m echo "git-proxy did not stop gracefully; forcing termination."�[0m
�[36;1m kill -KILL "$GIT_PROXY_PID" 2>/dev/null || true�[0m
�[36;1m fi�[0m
�[36;1m wait "$GIT_PROXY_PID" 2>/dev/null || true�[0m
�[36;1m fi�[0m
�[36;1m �[0m
�[36;1m echo "::$STOP_***REDACTED_SECRET_ASSIGNMENT***
�[36;1m FALLBACK_FILE="${RUNNER_TEMP}/copilot-fallback-error.txt"�[0m
�[36;1m if [ -f "$FALLBACK_FILE" ]; then�[0m
�[36;1m FALLBACK_MSG=$(head -c 500 "$FALLBACK_FILE" | tr -d '\n\r')�[0m
�[36;1m echo "::error title=Copilot Error::${FALLBACK_MSG}"�[0m
🧰 Additional context used
📓 Path-based instructions (9)
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Document user-facing changes in English Markdown documentation and keep generated documentation mirrors synchronized.
Files:
docs/integrations/httpx2.mddocs/integrations/httpx.mddocs/integrations/requests.mdCHANGELOG.mddocs/integrations/aiohttp.md
{interlock/**/*.py,docs/**/*.md,docs/llms-full.txt,docs/llms.txt}
📄 CodeRabbit inference engine (Custom checks)
When a change affects user-facing behaviour through the public API, integrations, or configuration options, update the relevant page under
docs/and regeneratedocs/llms-full.txt; when adding a new documentation page, list it under## Docsindocs/llms.txt.
Files:
docs/integrations/httpx2.mddocs/integrations/httpx.mddocs/integrations/requests.mddocs/llms-full.txtdocs/integrations/aiohttp.mdinterlock/integrations/_registry.py
docs/**/*.{md,mdx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
User-facing changes must update the relevant documentation page under
docs/.
Files:
docs/integrations/httpx2.mddocs/integrations/httpx.mddocs/integrations/requests.mddocs/integrations/aiohttp.md
docs/**/*.md
⚙️ CodeRabbit configuration file
User-facing documentation. Check that code samples match the current public API and would actually run. A new page must also be listed in docs/llms.txt under
## Docs. Keep the existing voice: short sentences, no marketing.
Files:
docs/integrations/httpx2.mddocs/integrations/httpx.mddocs/integrations/requests.mddocs/integrations/aiohttp.md
docs/llms-full.txt
⚙️ CodeRabbit configuration file
Generated artefact — produced by
uv run python scripts/build_llms_full.py. Do not review its content or suggest edits; only confirm it was regenerated together with the docs/ changes in the same PR.
Files:
docs/llms-full.txt
CHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Add every change to the
[Unreleased]section underAdded,Fixed, orChanged, explaining user impact rather than only symbol movement.User-facing changes must update the
[Unreleased]section.
Files:
CHANGELOG.md
⚙️ CodeRabbit configuration file
Keep a Changelog format. New entries go under
## [Unreleased]in Added / Fixed / Changed. An entry describes what a user could not do before and can now, not which symbol moved. Only the release commit dates a section and updates the link references.
Files:
CHANGELOG.md
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Support Python 3.11 and newer; use Python 3.11+ features where required.
Keep the core zero-dependency and use only the standard library; external dependencies must be isolated behind optional integrations.
Use injectedClockinstances for all time access; do not calltime.monotonic()orsleep()directly in logic.
Implement the core as an I/O-free state machine with a singlethreading.Lockaround the await-free critical section, never held across the protected call.
UseProtocols for extension points:Clock,SlidingWindow,Storage,FailureClassifier, andEventListener; do not inherit from internal classes.
Expose one publicCircuitBreakerclass for sync and async operation, with separate internal paths selected by coroutine detection.
Expose the public API through the package__init__.py; keep helpers underscore-prefixed and hidden.
Use absolute imports, placed at the top of the file, ordered as standard library, third-party, then local imports with blank lines between groups.
Use a maximum line length of 100 characters, single-quoted strings, f-strings, andpathlib.Pathinstead ofos.path.
Annotate every parameter and return value; use modern generic syntax andX | Noneinstead ofOptional[X].
UseStrEnumor module-level constants instead of magic constants.
When a constructor or function has three or more arguments, pass them by keyword.
Keep functions focused on one job, generally no longer than 20–30 lines, with minimal side effects and extracted repeated loop logic.
Useasync/awaitfor I/O-bound work,asyncio.TaskGroupinstead ofasyncio.gather, andasyncio.to_threadorProcessPoolExecutorfor CPU-bound work.
Do not mix sync and async in one function; never await a sync callable or block on an async callable.
Fail fast on invalid input or state by raising immediately; do not continue with partial results or invented defaults.
Catch only expected exceptions, log them with context, and re-raise; do not use ...
Files:
interlock/integrations/_registry.py
interlock/**/*.py
📄 CodeRabbit inference engine (Custom checks)
Every production behaviour change in
interlock/must be accompanied by a change undertests/; changes limited to docstrings, comments, or type annotations are exempt. Bug fixes must include at least one regression test that fails without the production fix.Keep the core dependency-free; external dependencies must belong to extras and be imported lazily.
Files:
interlock/integrations/_registry.py
⚙️ CodeRabbit configuration file
Core rules (AGENTS.md is authoritative): (1) Zero-dependency core — anything under interlock/ except interlock/integrations/ may import stdlib only. Flag every third-party import as a blocking issue. (2) No fallbacks, no silent excepts, no
a or b or cfor required config or data, no hidden retries. Invalid input or state raises immediately. interlock/_notify.py is the one sanctioned swallow (listener hooks are observability, logged with traceback, BaseException still propagates) — do not suggest generalising or "fixing" it. (3) Time comes only from the injected Clock protocol; direct time.monotonic()/time.sleep() in library logic is a bug. (4) Style: 100-char lines, single quotes, f-strings, pathlib, full annotations,X | NoneneverOptional[X], keyword arguments for calls with 3+ arguments, no magic constants (StrEnum or module constants), functions under ~30 lines. (5) Extension points are Protocols (Clock, SlidingWindow, Storage, FailureClassifier, EventListener) — do not propose inheriting internal classes. (6) Sync and async live in one CircuitBreaker with separate internal paths; never propose Sync*/Async* twins and never mix the two paths in one function. (7) Public API is exported from interlock/init.py; everything else is underscore-prefixed. New public symbols need__all__and a docstring. (8) Python 3.11 is the floor — no 3.12+ syntax or stdlib.
Files:
interlock/integrations/_registry.py
interlock/integrations/**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Integration dependencies must be optional extras and imported lazily.
Files:
interlock/integrations/_registry.py
⚙️ CodeRabbit configuration file
Optional extras. The third-party import must stay inside this package, must never be re-exported from interlock/init.py, and a missing extra must fail with a clear install hint rather than a fallback. Wrap the dependency behind the project's own types so its objects do not leak into core signatures. Check that the extra is declared in pyproject.toml
[project.optional-dependencies]and documented under docs/integrations/.
Files:
interlock/integrations/_registry.py
🔇 Additional comments (6)
docs/integrations/aiohttp.md (1)
102-105: 📐 Maintainability & Code QualityConfirm the required
CHANGELOG.mdentry.This is a user-facing documentation change. Verify that
CHANGELOG.mdcontains an entry under## [Unreleased]inAdded,Fixed, orChanged. The supplied files do not include that file.As per coding guidelines and path instructions, user-facing changes must update
CHANGELOG.mdunder## [Unreleased].Sources: Coding guidelines, Path instructions
docs/integrations/httpx.md (1)
149-152: LGTM!docs/integrations/httpx2.md (1)
155-158: LGTM!docs/integrations/requests.md (1)
105-108: LGTM!docs/llms-full.txt (1)
2874-2877: LGTM!Also applies to: 3068-3071, 3226-3229, 3392-3395
CHANGELOG.md (1)
18-25: Rewrite these entries around user impact.State that users can avoid
AttributeErrorby restrictingHttpStatusClassifierregistries to HTTP clients, and can resolve conflicting options by configuring them on the injectedRegistry. Do not describe only where the warning or message was added.As per path instructions,
CHANGELOG.mdentries must describe what users could not do before and can do now.Source: Path instructions
| raise ValueError( | ||
| f'The injected registry already owns these options: {joined}. ' | ||
| 'Configure them on the registry instead.' | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a regression test for the expanded ValueError.
Test an injected registry combined with a conflicting option. Assert that the error names the conflict and directs callers to configure the option on the registry. Without this test, the new user-facing error contract can regress.
As per coding guidelines, every production behaviour change in interlock/ must be accompanied by a change under tests/.
🤖 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 `@interlock/integrations/_registry.py` around lines 30 - 33, Add a regression
test under tests/ covering an injected registry combined with a conflicting
option. Assert that the raised ValueError identifies the conflicting option and
instructs callers to configure it on the registry, preserving the error contract
introduced in the registry validation path.
Source: Coding guidelines
Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
The entry described where the warning moved rather than what a user gets wrong: handing a caller-owned Registry to an HTTP integration makes the registry the source of failure policy, so a returned 503 is a success until the registry itself carries HttpStatusClassifier(). Also fold the auto-generated assertion on the rejection message into the existing conflict test — it re-ran the same call, and its bare pytest.raises(ValueError) failed ruff's PT011.
1 participant
Summary
A caller-owned
Registryowns the failure policy for every breaker it creates. Passing a bareRegistry()to the httpx2/httpx transports, the aiohttp middleware or the requests adaptertherefore downgrades them to exception-only classification: a returned
503counts as a success,and nothing anywhere reports it. The integration guides already carry that warning, but the
registryargument docstring — what the reader actually sees in the editor while writing the call— did not. This adds it to all six
registryarguments.Docstrings only; no behaviour change, no signature change.
Two neighbouring sharp edges were considered and deliberately left alone:
reading
Registry._classifier(or growing a public property for it), and exception-onlyclassification is a legitimate choice for a caller who raises on status upstream. An
unsuppressable warning on a legitimate configuration is noise.
reject_registry_optionsrejecting on key presence rather than on a non-default value.Kept as is: "you passed both" is predictable and value-independent, whereas comparing against
defaults would make
initial_state=State.CLOSEDpass andState.METRICS_ONLYfail, and wouldneed
classifier=Noneto be distinguishable from "not passed".Checklist
behaviour to cover;
uv run pytest --covstays at 100.00% (714 passed, 2 skipped)uv run ruff format --checkanduv run ruff checkpassuv run mypy,uv run pyrightanduv run pyrefly checkpassdocs/) for user-facing changes —docs/integrations/*.mdalready documentthis; no page changed, so
llms-full.txtneeds no regenerationCHANGELOG.md[Unreleased]updatedRelated issues
None.
Changed
httpxandhttpx2transports: Document bare caller-ownedRegistrybehavior. Returned503responses count as successes.aiohttpCircuitBreakerMiddleware: Document that an explicit classifier is required to classify response statuses such as503as failures.requestsCircuitBreakerAdapter: Document the failure policy of a caller-ownedRegistry.HttpStatusClassifier: Document that they are limited to HTTP clients. Non-HTTP results can raiseAttributeError.Registryoptions: Clarify that conflicting options raiseValueErrorand must be configured on theRegistry.