CI check: a new BaseStore must be wired into the app lifespan (no orphan stores) - #2334
Conversation
- scripts/check_store_wiring.py detects PRs that add a new BaseStore subclass without wiring it into tinyagentos/app.py - Uses name-level check (class name appears in app.py) - Only flags newly added classes; pre-existing orphans are skipped - Store-Unwired-Intentionally: <ClassName>, <why> trailer waives and logs - .github/workflows/store-wiring-gate.yml runs the check on PRs - tests/test_check_store_wiring.py proves FAIL, PASS, existing orphan not flagged, trailer waiver, and transitive subclass detection
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
📝 WalkthroughWalkthroughAdds a pull-request gate that detects newly added ChangesStore wiring gate
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Pull_request
participant Store_wiring_workflow
participant check_store_wiring_py
participant Git_repository
participant tinyagentos_app_py
Pull_request->>Store_wiring_workflow: Provide base reference and PR body
Store_wiring_workflow->>check_store_wiring_py: Run store wiring validation
check_store_wiring_py->>Git_repository: Read changed files and base revisions
check_store_wiring_py->>tinyagentos_app_py: Check subclass name references
tinyagentos_app_py-->>check_store_wiring_py: Return wiring references
check_store_wiring_py-->>Store_wiring_workflow: Return status and diagnostics
Store_wiring_workflow-->>Pull_request: Report gate result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
🤖 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 `@scripts/check_store_wiring.py`:
- Around line 196-202: Update the changed-file filtering loop in
check_store_wiring to handle Git rename and copy statuses (`R` and `C`) instead
of skipping them. Preserve each entry’s preimage path for these statuses, then
evaluate the destination path using the same Python-file and wiring validation
applied to modified files.
- Around line 114-117: Update the class hierarchy collection around the node
base scan to resolve ast.Attribute expressions and imported aliases to the
canonical BaseStore symbol, so declarations such as qualified
base_store.BaseStore are recorded and gated correctly. Preserve existing
ast.Name handling, and add coverage for qualified and aliased BaseStore imports.
- Around line 84-90: Update the class-handling logic in the store-wiring check
so an existing class is skipped only when its base-revision inheritance already
includes BaseStore; continue processing classes whose inheritance changed from
another base to BaseStore. Add a regression test covering this conversion from a
non-BaseStore subclass.
- Around line 170-174: Update the trailer parsing logic around TRAILER so a
waiver is added only when the trailer contains a comma-separated class and a
non-empty reason; reject entries missing the comma or with blank text after it,
while preserving valid waiver handling.
In `@tests/test_check_store_wiring.py`:
- Around line 411-415: Update the test around check_store_wiring to assert that
the returned waived collection is empty, using the existing waived variable
alongside the violation assertions. Keep the existing NewStore violation checks
unchanged.
🪄 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: e6a6bb45-7d78-44eb-8472-e6077b9baf3e
📒 Files selected for processing (4)
.github/workflows/store-wiring-gate.ymlchangelog.d/tsk-n3w5mh-store-wiring-gate.mdscripts/check_store_wiring.pytests/test_check_store_wiring.py
| base_content = _get_file_at_ref(file_path, base_ref, repo_root) | ||
| if base_content is not None: | ||
| try: | ||
| base_tree = ast.parse(base_content) | ||
| for node in ast.walk(base_tree): | ||
| if isinstance(node, ast.ClassDef) and node.name == class_name: | ||
| return False |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Detect classes converted into BaseStore subclasses.
Line 89 returns False when the class name exists in the base revision. If a PR changes class Foo(Other) to class Foo(BaseStore), the guard skips Foo even though it introduces a new store subclass. Compare the base revision inheritance before skipping the class. Add a regression test for this conversion.
🤖 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 `@scripts/check_store_wiring.py` around lines 84 - 90, Update the
class-handling logic in the store-wiring check so an existing class is skipped
only when its base-revision inheritance already includes BaseStore; continue
processing classes whose inheritance changed from another base to BaseStore. Add
a regression test covering this conversion from a non-BaseStore subclass.
| for base in node.bases: | ||
| if isinstance(base, ast.Name): | ||
| bases.add(base.id) | ||
| classes[node.name] = bases |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Resolve qualified BaseStore base expressions.
The hierarchy only records ast.Name bases. A valid declaration such as class Foo(base_store.BaseStore): produces no base entry and bypasses the gate. Resolve ast.Attribute bases and imported aliases to the canonical BaseStore symbol. Add coverage for qualified and aliased imports.
🤖 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 `@scripts/check_store_wiring.py` around lines 114 - 117, Update the class
hierarchy collection around the node base scan to resolve ast.Attribute
expressions and imported aliases to the canonical BaseStore symbol, so
declarations such as qualified base_store.BaseStore are recorded and gated
correctly. Preserve existing ast.Name handling, and add coverage for qualified
and aliased BaseStore imports.
| if line.startswith(TRAILER): | ||
| classes_str = line[len(TRAILER):].strip() | ||
| cls = classes_str.split(",", 1)[0].strip() | ||
| if cls: | ||
| waived.add(cls) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require a non-empty waiver reason.
Store-Unwired-Intentionally: Foo currently waives Foo without the required <why> value. Reject trailers with no comma or an empty reason so the waiver remains auditable.
Proposed fix
- cls = classes_str.split(",", 1)[0].strip()
- if cls:
- waived.add(cls)
+ cls, separator, reason = classes_str.partition(",")
+ if cls.strip() and separator and reason.strip():
+ waived.add(cls.strip())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if line.startswith(TRAILER): | |
| classes_str = line[len(TRAILER):].strip() | |
| cls = classes_str.split(",", 1)[0].strip() | |
| if cls: | |
| waived.add(cls) | |
| if line.startswith(TRAILER): | |
| classes_str = line[len(TRAILER):].strip() | |
| cls, separator, reason = classes_str.partition(",") | |
| if cls.strip() and separator and reason.strip(): | |
| waived.add(cls.strip()) |
🤖 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 `@scripts/check_store_wiring.py` around lines 170 - 174, Update the trailer
parsing logic around TRAILER so a waiver is added only when the trailer contains
a comma-separated class and a non-empty reason; reject entries missing the comma
or with blank text after it, while preserving valid waiver handling.
| for status, file_path in changed: | ||
| if not file_path.startswith("tinyagentos/") or not file_path.endswith(".py"): | ||
| continue | ||
| if status.startswith("D"): | ||
| continue | ||
| if not (status.startswith("A") or status.startswith("M")): | ||
| continue |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Handle renamed and copied Python files.
Git can report a renamed or copied file as R or C. This code skips both statuses. A PR can rename a store module and add an unwired subclass in the same change without validation. Preserve the preimage path for rename or copy entries and evaluate the destination file as a modified file.
🤖 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 `@scripts/check_store_wiring.py` around lines 196 - 202, Update the
changed-file filtering loop in check_store_wiring to handle Git rename and copy
statuses (`R` and `C`) instead of skipping them. Preserve each entry’s preimage
path for these statuses, then evaluate the destination path using the same
Python-file and wiring validation applied to modified files.
| violations, waived = csw.check_store_wiring(base_tip, repo) | ||
|
|
||
| assert len(violations) == 1 | ||
| assert violations[0].class_name == "NewStore" | ||
| assert violations[0].file_path == "tinyagentos/metrics_store.py" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the no-waiver result.
waived is unused, and Ruff reports RUF059. Assert that it is empty. This also verifies that an unwired NewStore does not receive an implicit waiver.
Proposed fix
assert len(violations) == 1
assert violations[0].class_name == "NewStore"
assert violations[0].file_path == "tinyagentos/metrics_store.py"
+ assert waived == set()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| violations, waived = csw.check_store_wiring(base_tip, repo) | |
| assert len(violations) == 1 | |
| assert violations[0].class_name == "NewStore" | |
| assert violations[0].file_path == "tinyagentos/metrics_store.py" | |
| violations, waived = csw.check_store_wiring(base_tip, repo) | |
| assert len(violations) == 1 | |
| assert violations[0].class_name == "NewStore" | |
| assert violations[0].file_path == "tinyagentos/metrics_store.py" | |
| assert waived == set() |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 411-411: Unpacked variable waived is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 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 `@tests/test_check_store_wiring.py` around lines 411 - 415, Update the test
around check_store_wiring to assert that the returned waived collection is
empty, using the existing waived variable alongside the violation assertions.
Keep the existing NewStore violation checks unchanged.
Source: Linters/SAST tools
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| def _run_git(args: list[str], repo_root: Path) -> str: | ||
| result = subprocess.run( | ||
| ["git", *args], cwd=repo_root, capture_output=True, text=True, check=True, |
There was a problem hiding this comment.
WARNING: _run_git raises unhandled CalledProcessError on git failures
_run_git uses check=True but callers don't catch CalledProcessError. If the git command fails (e.g., invalid base ref, network issue during fetch), the script crashes with a raw traceback instead of a user-friendly error, making CI failures hard to diagnose.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| for node in ast.walk(base_tree): | ||
| if isinstance(node, ast.ClassDef) and node.name == class_name: | ||
| return False | ||
| except SyntaxError: |
There was a problem hiding this comment.
WARNING: SyntaxError in base file silently produces false positives
_class_def_in_added_lines catches SyntaxError from ast.parse and falls through to the diff check. If the base file has a syntax error, an existing class can be incorrectly flagged as "new" because its definition appears in the diff. Return False instead of falling through.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| waived.add(class_name) | ||
| continue | ||
|
|
||
| if not re.search(rf"\b{re.escape(class_name)}\b", app_py_content): |
There was a problem hiding this comment.
WARNING: Name-level wiring check matches in comments and string literals
re.search(rf"\b{re.escape(class_name)}\b", app_py_content) matches the class name anywhere in app.py, including comments and docstrings. A new BaseStore subclass could pass the check without being wired if its name merely appears in a comment (e.g., # TODO: wire FooStore). Consider parsing app.py with ast and checking for actual attribute assignments.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit 3c71a7e)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 3c71a7e)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (4 files)
Reviewed by step-3.7-flash · Input: 57.5K · Output: 5K · Cached: 170.2K |
|
nemotron-super review VERDICT: No blocking issues found. Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
…er gate on PR edits
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 @.claude/skills/taos-development-skill/SKILL.md:
- Line 451: Update the waiver example fenced block in SKILL.md to specify the
text language tag, changing the opening fence to ```text while preserving its
Store-Unwired-Intentionally example content.
🪄 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: 833fb110-90b7-49a4-b4de-6bedf3cde33e
📒 Files selected for processing (2)
.claude/skills/taos-development-skill/SKILL.md.github/workflows/store-wiring-gate.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/store-wiring-gate.yml
|
|
||
| For a store genuinely constructed elsewhere (tests, CLI, workers), waive it with a PR-body | ||
| trailer, which is logged by the gate: | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the waiver example.
The fenced block at Line 451 has no language. Add text to satisfy markdownlint rule MD040.
Proposed fix
-```
+```text
Store-Unwired-Intentionally: <ClassName>, <why>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 451-451: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 SkillSpector (2.5.1)
[warning] 506: [RA2] Session Persistence: Skill establishes unauthorized persistence across sessions via cron jobs, startup scripts, or state files. Session persistence allows an attacker to maintain access beyond the current interaction.
Remediation: Remove any persistence mechanisms (cron jobs, startup scripts, state files). Skills should not maintain state across sessions without explicit user consent.
(Rogue Agent (RA2))
🤖 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 @.claude/skills/taos-development-skill/SKILL.md at line 451, Update the
waiver example fenced block in SKILL.md to specify the text language tag,
changing the opening fence to ```text while preserving its
Store-Unwired-Intentionally example content.
Source: Linters/SAST tools
1 participant
CARD TITLE (intent, not commit subject): CI check: a new BaseStore must be wired into the app lifespan (no orphan stores)
Autonomous build of board card tsk-n3w5mh.
subclass without wiring it into tinyagentos/app.py
not flagged, trailer waiver, and transitive subclass detection
Files:
.github/workflows/store-wiring-gate.yml | 43 +++
changelog.d/tsk-n3w5mh-store-wiring-gate.md | 11 +
scripts/check_store_wiring.py | 273 ++++++++++++++++++
tests/test_check_store_wiring.py | 415 ++++++++++++++++++++++++++++
4 files changed, 742 insertions(+)
Summary by CodeRabbit
New Features
Documentation
Tests
RED-proof at the merge ref (gate-PRs-prove-red rule)
Lead-verified, two independent bad cases, both through the exact workflow entry point:
Bad case 1 - the real defect this gate exists for. Merge ref = dev + this PR, then merge #2333 (adds
StrikeStorewithout wiring):Bad case 2 - synthetic unwired store (
RedTestOrphanStoreundertinyagentos/):Green half: the merge ref alone (no new stores) exits 0
store-wiring-guard: clean. Waiver viaPR_BODYenv verified working.