fix: implement _get_changed_files for FilePatternCondition#71
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🛡️ Watchflow Governance ChecksStatus: ❌ 1 Violations Found 🟡 Medium Severity (1)Ensures PRs that modify source code also include a CHANGELOG or .changeset addition.Source code was modified without a corresponding CHANGELOG update. 💡 Reply with Thanks for using Watchflow! It's completely free for OSS and private repositories. You can also self-host it easily. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🧹 Nitpick comments (1)
tests/unit/rules/conditions/test_filesystem.py (1)
99-166: Add one malformed-payload regression for_get_changed_filesNice coverage on happy paths and dedup. Please add one error-path test with mixed invalid entries (e.g., missing filename,
None, non-list commit keys) and assert it returns only valid string paths without exceptions.As per coding guidelines: "Write unit tests for deterministic rule evaluation (pass/warn/block), model validation, and error paths" and "Write regression tests for every bug fix; keep CI coverage thresholds green".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/rules/conditions/test_filesystem.py` around lines 99 - 166, Add a regression test that supplies a mixed malformed payload to FilePatternCondition._get_changed_files (e.g., changed_files containing a dict missing "filename", a None, a non-string value, and commits with non-list values or non-string entries) and assert the method returns only the valid string file paths and does not raise an exception; implement this as a new test function (e.g., test_get_changed_files_with_malformed_payload) mirroring existing tests' style and using condition._get_changed_files to validate the filtered output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/rules/conditions/filesystem.py`:
- Around line 120-135: The _get_changed_files parsing trusts payload shapes;
update the logic that reads event.get("changed_files") and event.get("commits")
to validate types: only accept changed_files when it's a list and each entry is
either a non-empty str or a dict with a non-empty str "filename"; for commits,
skip non-dict commit entries, ensure added/modified/removed are lists, and only
add items that are non-empty str paths to the seen set before returning
sorted(seen); reference the variables/functionality around changed_files,
commits, the keys ("added","modified","removed"), and the seen set when applying
the guards.
---
Nitpick comments:
In `@tests/unit/rules/conditions/test_filesystem.py`:
- Around line 99-166: Add a regression test that supplies a mixed malformed
payload to FilePatternCondition._get_changed_files (e.g., changed_files
containing a dict missing "filename", a None, a non-string value, and commits
with non-list values or non-string entries) and assert the method returns only
the valid string file paths and does not raise an exception; implement this as a
new test function (e.g., test_get_changed_files_with_malformed_payload)
mirroring existing tests' style and using condition._get_changed_files to
validate the filtered output.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 192a864b-5e0c-4a8c-a536-c17cff74ed2e
📒 Files selected for processing (2)
src/rules/conditions/filesystem.pytests/unit/rules/conditions/test_filesystem.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
**/*.py: Use modern typing only:dict[str, Any],list[str],str | None(noDict,List,Optional)
GitHub/HTTP/DB calls must beasync def; avoid blocking calls (time.sleep, sync HTTP) in async paths
All agent outputs and external payloads must use validatedBaseModelfrom Pydantic
Use dataclasses for internal immutable state where appropriate
Use structured logging at boundaries with fields:operation,subject_ids,decision,latency_ms
Implement Agent pattern: single-responsibility agents with typed inputs/outputs
Use Decorator pattern for retries, metrics, caching as cross-cutting concerns
Agent outputs must include:decision,confidence (0..1), shortreasoning,recommendations,strategy_used
Implement confidence policy: reject or route to human-in-the-loop when confidence < 0.5
Use minimal, step-driven prompts; provide Chain-of-Thought only for complexity > 0.7 or ambiguity > 0.6
Strip secrets/PII from agent prompts; scope tools; keep raw reasoning out of logs (store summaries only)
Cache idempotent lookups; lazy-import heavy dependencies; bound fan-out withasyncio.Semaphore
Avoid redundant LLM calls; memoize per event when safe
Use domain errors (e.g.,AgentError) witherror_type,message,context,timestamp,retry_count
Use exponential backoff for transient failures; circuit-break noisy integrations when needed
Fail closed for risky decisions; provide actionable remediation in error paths
Validate all external inputs; verify webhook signatures
Implement prompt-injection hardening; sanitize repository content passed to LLMs
Performance targets: Static validation ~<100ms typical, hybrid decisions sub-second when cache warm, budget LLM paths thoughtfully
Reject old typing syntax (Dict,List,Optional) in code review
Reject blocking calls in async code; reject bareexcept:clauses; reject swallowed errors
Reject LLM calls for trivial/deterministic checks
Reject unvalidated agent outputs and missing confidenc...
Files:
src/rules/conditions/filesystem.pytests/unit/rules/conditions/test_filesystem.py
tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/guidelines.mdc)
tests/**/*.py: Write unit tests for deterministic rule evaluation (pass/warn/block), model validation, and error paths
Write integration tests for webhook parsing, idempotency, multi-agent coordination, and state persistence
Usepytest.mark.asynciofor async tests; avoid live network calls; freeze time and seed randomness
Write regression tests for every bug fix; keep CI coverage thresholds green
Files:
tests/unit/rules/conditions/test_filesystem.py
🧠 Learnings (2)
📚 Learning: 2026-03-27T12:52:44.067Z
Learnt from: oleksii-quinta
Repo: warestack/watchflow PR: 67
File: src/webhooks/handlers/issue_comment.py:153-159
Timestamp: 2026-03-27T12:52:44.067Z
Learning: When enqueuing processor tasks using `task_queue`, follow the documented pre-built-task pattern: (1) build the task with `pre_built_task = task_queue.build_task(event_type, payload, processor.process, delivery_id=...)`; (2) call `task_queue.enqueue(processor.process, event_type, payload, pre_built_task, delivery_id=...)` by passing the pre-built task as a single positional `*args` element; (3) ensure the worker ultimately calls `await processor.process(pre_built_task)` (i.e., the processor `process(self, task: Task)` receives the `Task` instance). This matches the expectation that `enqueue` stores the pre-built task in the wrapper Task’s `args` as described by `build_task`’s docstring (“pass as single arg to enqueue”).
Applied to files:
src/rules/conditions/filesystem.py
📚 Learning: 2026-01-31T19:35:22.504Z
Learnt from: CR
Repo: warestack/watchflow PR: 0
File: .cursor/rules/guidelines.mdc:0-0
Timestamp: 2026-01-31T19:35:22.504Z
Learning: Applies to tests/**/*.py : Write unit tests for deterministic rule evaluation (pass/warn/block), model validation, and error paths
Applied to files:
tests/unit/rules/conditions/test_filesystem.py
|
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (73.0%) is below the target coverage (80.0%). You can increase the head coverage or adjust the target coverage. @@ Coverage Diff @@
## main #71 +/- ##
=======================================
+ Coverage 72.8% 73.0% +0.1%
=======================================
Files 181 181
Lines 13398 13456 +58
=======================================
+ Hits 9762 9828 +66
+ Misses 3636 3628 -8 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
dkargatzis
left a comment
dkargatzis
left a comment
There was a problem hiding this comment.
The implementation is robust with proper type guards and covers both happy and error paths. Tests are also thorough.
🛡️ Watchflow Governance ChecksStatus: ❌ 1 Violations Found 🟠 High Severity (1)When a PR modifies paths that have owners defined in CODEOWNERS, the corresponding code owners must be added as reviewersCode owners for modified paths must be added as reviewers: dkargatzis 💡 Reply with Thanks for using Watchflow! It's completely free for OSS and private repositories. You can also self-host it easily. |
3 participants
Summary
FilePatternCondition._get_changed_files()was a TODO stub that returned[]for both PR and push events, making thefiles_match_patterncondition entirely non-functionalchanged_fileslist (populated by the PR enricher) for pull request eventsadded,modified, andremovedarraysCloses #70
Summary by CodeRabbit
Bug Fixes
Tests