ci: encode the definition of done as CodeRabbit review gates - #146
Merged
Conversation
Add `.coderabbit.yaml` so the automated review enforces the rules from AGENTS.md that a generic Python reviewer cannot infer from a diff. - Eleven `path_instructions` covering the zero-dependency core, the `_notify.py` sanctioned swallow, the engine lock scope, `Clock`-only time, the test naming convention and the SHA-pinning rule for workflows. - Five custom pre-merge checks mirroring the definition of done: zero-dependency core (error), changelog entry, docs plus the `llms-full.txt` mirror, tests alongside behaviour changes and public API surface. - Disable pylint and markdownlint: ruff and pymarkdown already own those files, and a second unconfigured rule set is pure noise. - Keep `auto_apply_labels` off. The `breaking-change` label disables the griffe API gate, so it stays a human decision. - Point `code_guidelines.filePatterns` at AGENTS.md and CONTRIBUTING.md; the default patterns look for CLAUDE.md files, which are gitignored. `base_branches` names `main` explicitly: right after installation the bot skipped PR #129 as targeting a non-default branch even though its base is `main`.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
WalkthroughAdded a complete ChangesCodeRabbit configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 9✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 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 @.coderabbit.yaml:
- Around line 116-118: Align the public API definitions in the two review rules:
update the rule around the interlock/__init__.py guidance and the rule around
interlock/pipeline.py so both recognize the same exported symbols and apply
identical __all__ and docstring requirements. Preserve the existing Python 3.11
compatibility rule.
- Around line 122-126: Update the critical-section guidance in the
.coderabbit.yaml comment to say that holding the threading.Lock across the
protected call can deadlock, rather than asserting it always deadlocks; retain
the existing warning about serialization and throughput impact.
🪄 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: 7bfa1ded-e09b-4755-8dc6-01c39913e39c
📒 Files selected for processing (1)
.coderabbit.yaml
Comment on lines
+116
to
+118
| (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. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Resolve the public API definition conflict.
Lines 116-118 define only interlock/__init__.py exports as public. Lines 286-292 also treat interlock/pipeline.py symbols as public. A valid interlock.pipeline export can therefore receive incompatible review feedback. Define the same public API surface in both rules.
🤖 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 @.coderabbit.yaml around lines 116 - 118, Align the public API definitions in
the two review rules: update the rule around the interlock/__init__.py guidance
and the rule around interlock/pipeline.py so both recognize the same exported
symbols and apply identical __all__ and docstring requirements. Preserve the
existing Python 3.11 compatibility rule.
Comment on lines
+122
to
+126
| The critical section. Verify: the state machine stays I/O-free and | ||
| unaware of sync vs async; the threading.Lock covers only the await-free | ||
| acquire and record sections and is never held across the protected call | ||
| (a call under the lock is a deadlock and a throughput bug); HALF_OPEN | ||
| still bounds concurrent probes. Ask whether `uv run mutmut run` was run — |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import threading
lock = threading.Lock()
assert lock.acquire()
try:
assert lock.acquire(blocking=False) is False
finally:
lock.release()
PYRepository: bagowix/interlock
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- .coderabbit.yaml ---'
sed -n '118,130p' .coderabbit.yaml
printf '%s\n' '--- AGENTS.md files ---'
find .. -name AGENTS.md -print
python3 - <<'PY'
import threading
# A call that does not touch the lock completes while the lock is held.
lock = threading.Lock()
completed = []
def independent_call():
completed.append("done")
with lock:
independent_call()
assert completed == ["done"]
# A same-thread re-entry cannot acquire a non-reentrant Lock.
reentrant = threading.Lock()
with reentrant:
assert reentrant.acquire(blocking=False) is False
# A second caller waits, but does not itself create a deadlock.
serialized = threading.Lock()
started = threading.Event()
finished = threading.Event()
def caller():
started.set()
with serialized:
finished.set()
with serialized:
thread = threading.Thread(target=caller)
thread.start()
assert started.wait(1)
assert not finished.is_set()
serialized.release()
thread.join(1)
assert finished.is_set() and not thread.is_alive()
PYRepository: bagowix/interlock
Length of output: 1267
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import threading
# Holding the lock does not deadlock an independent call.
lock = threading.Lock()
completed = []
def independent_call():
completed.append("done")
lock.acquire()
try:
independent_call()
finally:
lock.release()
assert completed == ["done"]
# A same-thread re-entry cannot acquire a non-reentrant Lock.
lock.acquire()
try:
assert lock.acquire(blocking=False) is False
finally:
lock.release()
# A second caller waits and then proceeds after the lock is released.
lock.acquire()
started = threading.Event()
finished = threading.Event()
def caller():
started.set()
with lock:
finished.set()
thread = threading.Thread(target=caller)
thread.start()
assert started.wait(1)
assert not finished.is_set()
lock.release()
thread.join(1)
assert finished.is_set() and not thread.is_alive()
PYRepository: bagowix/interlock
Length of output: 155
Change “is a deadlock” to “can deadlock.” Holding threading.Lock across a call serializes callers. It deadlocks only if the call re-enters the lock or waits for work that requires it.
🤖 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 @.coderabbit.yaml around lines 122 - 126, Update the critical-section
guidance in the .coderabbit.yaml comment to say that holding the threading.Lock
across the protected call can deadlock, rather than asserting it always
deadlocks; retain the existing warning about serialization and throughput
impact.
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
Add a
.coderabbit.yamlso the automated PR review enforces this project'srules instead of generic Python advice.
AGENTS.mdthat areinvisible in a diff: stdlib-only core outside
interlock/integrations/,time only through the injected
Clock, the lock in_engine.pynever heldacross the protected call,
X | NoneoverOptional[X], keyword argumentsat 3+ parameters, the
test__unit__state__expectednaming convention, andSHA-pinned actions under
.github/workflows/. The sanctioned swallow in_notify.pyis called out explicitly so the bot stops trying to "fix" it.zero-dependency core (
error),[Unreleased]changelog entry, docs plus aregenerated
docs/llms-full.txt, tests alongside behaviour changes, andpublic API surface (requires the
breaking-changelabel plus a migrationnote, matching the
griffe checkgate). Title check enforces ConventionalCommits, since PRs are squash-merged.
pylintandmarkdownlintare off — ruff and pymarkdownalready gate those files, and a second unconfigured rule set only produces
contradictory nits. Docstring coverage is
off: public API carriesdocstrings, private helpers deliberately do not.
auto_apply_labelsstaysfalseandbreaking-changeis absentfrom the labeling instructions, because that label switches off the griffe
API-compatibility gate — not something a bot should do unattended.
filePatternspoints atAGENTS.mdandCONTRIBUTING.md; the defaults look forCLAUDE.mdfiles, which aregitignored here and would never be visible.
docs/llms-full.txtis deliberately not inpath_filters: those patternsalso drive sparse-checkout, and excluding the file would blind the check that
verifies the mirror was regenerated. A path instruction tells the reviewer to
confirm its regeneration without reviewing its contents.
reviews.auto_review.base_branchesnamesmainexplicitly. On installationthe bot skipped PR #129 with "auto reviews are disabled on base/target branches
other than the default branch", although that PR targets
mainand the basewas never changed. The explicit entry removes the dependency on how CodeRabbit
resolves the default branch.
Validated against
https://coderabbit.ai/integrations/schema.v2.json— noerrors. Note that CodeRabbit reads the file from the branch under review, so
this PR is its own smoke test: the Run configuration block should report
Configuration used: .coderabbit.yamlinstead ofdefaults.Checklist
code changed; the suite is untouched
uv run ruff format --checkanduv run ruff checkpassuv run mypy,uv run pyrightanduv run pyrefly checkpassdocs/) for user-facing changes — n/a, tooling onlyCHANGELOG.md[Unreleased]updated — n/a, no user-visible changeRelated issues
Added
AGENTS.mdandCONTRIBUTING.md.Changed
mainas the review base branch.pylint,markdownlint, docstring coverage, and automatic label application.