docs: rewrite review-campaign comment narration as plain invariants - #873
Merged
Conversation
A quality audit found comments across the intent-context/stop pipelines
narrating the review campaign that produced them ("Round 5 (C1)",
"Wave-3 CONFIRMED (round 4)", "attack5.py", "regression guard (commit
eec4ae0)") instead of stating the constraint being protected. Rewrite
each site to say what must hold, not the story of how it was found.
Consolidates the session-fold invariant into one canonical statement in
_registry_session_for_context_write's docstring (SESSION-2 §2.6), with
every other call site shrunk to a one-line pointer back to it. Folds
_was_active_pre_drain and _utt_state_pre_drain (StopService) into a single
PreDrainSnapshot NamedTuple dict, keyed and popped together, as the one
mechanical refactor in scope; no other behavior changes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe change clarifies session-folding and context-expiry rules, consolidates targeted-stop pre-drain state into ChangesSession semantics and stop lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant StopService
participant IntentService
participant StopResponse
StopService->>StopService: Capture PreDrainSnapshot
StopService->>IntentService: Drain session copy
StopResponse->>StopService: Return targeted stop response
StopService->>StopService: Consume snapshot and resolve lifecycle
Possibly related PRs
Suggested labels: ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Standard verification protocol finished. 📋I've aggregated the results of the automated checks for this PR below. 📚 DocsA quick update on the progress of your PR checks. 📈 ✅ All required documentation files present. ✅ 📋 Repo HealthEnsuring the repo's joints are well-oiled (aka CI/CD). ⚙️ ✅ All required files present. Latest Version: ✅ 🏷️ Release PreviewI've checked the 'Legal' section for the release. ⚖️ Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
🌍 Locale BuildThe automated pipeline is running smoothly. 🚂 ✅ Locale properly configured (64 files, 17 languages) Locale directories found:
Localization coverage:
pyproject.toml: ✅
Build manifest: ✅ 31 locale files included in package 📊 CoverageA forensic analysis of your test coverage. 🔍 Files below 80% coverage (8 files)
Full report: download the 🔒 Security (pip-audit)Ensuring our cookies are secure and fresh. 🍪 ✅ No known vulnerabilities found (113 packages scanned). 🔎 Type CheckThe results have been compiled and are ready for review. 📑 ❌ mypy: 300 error(s) found
Errors (showing first 10/300)🔌 Plugin DetectionChecking for any potential plugin conflicts. ⚔️ ❌ Plugin Status: ERRORS (1) Plugin Info:
OPM Detection:
Entry Point Validation:
⊘ No Issues:
🔌 Skill Tests (ovoscope)Testing the skill's 'vocabulary' across different languages. 🌍 ✅ 13/37 passed ❌ **TestAdaptIntent** — 0/4
❌ **TestCancelIntentMidSentence** — 0/1
❌ **TestConverse** — 0/1
❌ **TestDeactivate** — 2/3
❌ **TestFallback** — 0/1
❌ **TestIntentPipelineRouting** — 0/4
❌ **TestLangDisambiguation** — 0/4
❌ **TestLegacyIntentIdBackCompat** — 0/2
❌ **TestNoSkills** — 0/2
❌ **TestPadatiousIntent** — 0/4
🚌 Bus CoverageEnsuring no message type is left behind! 🏃 🔴 Coverage Summary
📊 Per-Skill Breakdown
🔍 Detailed Message Type Breakdown
|
| Python | Build | Install | Tests |
|---|---|---|---|
| 3.10 | ✅ | ✅ | ✅ |
| 3.11 | ✅ | ✅ | ✅ |
| 3.12 | ✅ | ✅ | ✅ |
| 3.13 | ✅ | ✅ | ✅ |
| 3.14 | ✅ | ✅ | ✅ |
Standard Automated Signature v2.0 🏷️
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.
A quality audit found comments in the intent-context and stop pipelines that narrate the review campaign that produced them — "Round 5 (C1)", "Wave-3 CONFIRMED (round 4)", "F2 (round-3 adversarial re-review of 6e8c816)", "attack5.py", "regression guard (commit eec4ae0)" — instead of stating the constraint the code protects. A new contributor can't decode any of that. This rewrites each site as the plain invariant it enforces, keeping spec anchors (§ citations) since those are real constraints, not campaign trivia.
The session-fold rule ("a message's session snapshot folds onto the live registry session at lifecycle entry only; incidental messages must never fold") was previously re-explained at five separate sites in
service.pyandconverse_service.py. It's now stated once, in_registry_session_for_context_write's docstring (SESSION-2 §2.6), with every other site shrunk to a one-line pointer back to it.handle_add_context's multi-round narration collapses into one comment block describing the two-dialect write it performs.stop_service.py's F2/CONFIRMED-N/attack comments become plain statements of the pop-before-branch and pre-drain-vs-live-read invariants, and its two oversized docstrings are trimmed to the essentials (the data-vs-context rule now lives once, indispatcher.py's_pop, and is referenced from here).The one non-comment change:
StopService._was_active_pre_drainand_utt_state_pre_drain— two parallel dicts always written and popped together — are merged into a single_pre_draindict of aPreDrainSnapshotNamedTuple, with one comment stating the drain-ordering invariant. Everything else is comments/docstrings only. Fulltest/unittestssuite: 423 passed, 6 xfailed, identical toorigin/dev's baseline.Summary by CodeRabbit
Bug Fixes
Tests