fix(para): stop suggest_archive dividing by a non-positive inactive_days#1679
Conversation
PARAFileMover.suggest_archive computed confidence as min(0.95, 0.5 + days_inactive / (inactive_days * 3)), which raised ZeroDivisionError for inactive_days=0 -- a reasonable caller intent meaning "archive everything regardless of age". Negative thresholds were broken too: every file passes the age check, then the negative divisor drives confidence below zero and MoveSuggestion.__post_init__ rejects it with ValueError. Saturate confidence at 0.95 when inactive_days <= 0 instead. A non-positive threshold selects every file, so the age ratio carries no signal, and one branch covers both failure modes without adding a new error path. Rejecting 0 with ValueError was the alternative, but an existing test already calls suggest_archive(inactive_days=0) and only escapes the crash by monkeypatching stat. Adds regression tests for the zero and negative thresholds; both were confirmed failing against the previous code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
ChangesArchive threshold handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
The diff-coverage gate failed at 66.7%: line 346 (the saturated confidence branch) was uncovered. CI generates coverage.xml from `pytest tests/ -m "ci and not benchmark"`, and the two new regression tests carried only the class-level `unit` marker, so they never ran in that selection. The positive-threshold branch was covered incidentally by the ci-marked tests/integration/test_para_file_mover.py. Add `@pytest.mark.ci` to both regression tests. Verified with the gate's own command against a ci-selected coverage run: 3 changed lines, 0 missing, 100%. Both tests are Windows-safe (tmp_path + os.utime only), which matters now that `ci` also pulls them into test-windows (-m "ci or smoke"). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The per-file integration coverage floor failed: file_mover.py dropped to 84.7%, below its 85% floor. The floor is measured over `pytest tests/ -m "integration or conformance"`, and the fix added two statements plus two branch arcs there, of which the saturated-confidence branch was unreachable from that pool -- the unit-suite regressions do not run in it. Add the zero-threshold case to tests/integration/test_para_file_mover.py (module-marked integration + ci), which covers all four added units. Verified with the gate's own arithmetic (covered_lines + covered_branches) / (num_statements + num_branches): 84.69% -> 85.71%. The 84.69% reproduces CI's reported 84.7% exactly, so the local measurement tracks the gate. Coverage over the full pool is monotonically >= this subset. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![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 `@src/file_organizer/methodologies/para/ai/file_mover.py`:
- Around line 314-316: Update the file-selection predicate in the method
handling inactive-file suggestions so inactive_days <= 0 bypasses the
days_inactive threshold and includes every eligible file, while preserving the
existing age check for positive thresholds. Add a regression test covering a
future modification time with a non-positive inactive_days value and verify it
is selected.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 962b1c5c-13d9-4f7a-a35b-1ef5eca384da
📒 Files selected for processing (3)
src/file_organizer/methodologies/para/ai/file_mover.pytests/integration/test_para_file_mover.pytests/methodologies/para/test_file_mover.py
…nfidence CodeRabbit review on #1679: the docstring promises inactive_days <= 0 archives every file, but the selection predicate still read `days_inactive >= inactive_days`. A file whose mtime is in the future -- clock skew, or an archive extracted with a bogus timestamp -- has a negative days_inactive and was silently excluded. Verified: a file dated 30 days ahead returned 0 suggestions at inactive_days=0. Short-circuit the predicate on the non-positive threshold so selection matches the documented contract and the confidence branch beside it. Also clamp the reasoning day count at 0. That string is now reachable with a negative days_inactive, and "has not been modified in -30 days" is nonsense; positive-threshold output is unchanged. Regression tests in both gate pools: parametrized (0, -1) in the unit suite, plus an integration case. Both guards mutation-tested -- reverting either the predicate or the clamp fails all three tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Description
PARAFileMover.suggest_archivecomputed archive confidence as:inactive_daysis an unvalidatedint, andinactive_days=0is a reasonablecaller intent — "archive everything regardless of age". It raised
ZeroDivisionError: float division by zero.Negative thresholds were broken too, in a second way:
days_inactive >= inactive_dayspasses for every file, then the negative divisor drives confidence below zero and
MoveSuggestion.__post_init__rejects it(
ValueError: confidence must be between 0.0 and 1.0, got -99.5for a 300-day-oldfile at
inactive_days=-1).Fix: saturate confidence at 0.95 when
inactive_days <= 0. A non-positivethreshold selects every file, so the age ratio carries no signal; one branch covers
both failure modes without adding a new error path. The docstring now states that
0or less means "archive everything regardless of age" at maximum confidence.Why not reject
inactive_days < 1withValueError: an existing test alreadycalls
suggest_archive(src, inactive_days=0)(
tests/methodologies/para/test_file_mover.py,test_file_stat_error) and onlyescapes the crash because it monkeypatches
statto raise. The suite already treats0as a legitimate call, and saturating preserves the "archive everything" intent.Fixes # (issue)
Type of change
How Has This Been Tested?
TestSuggestArchiveintests/methodologies/para/test_file_mover.py:test_zero_inactive_days_archives_everythingandtest_negative_inactive_days_archives_everything. Both were written first andconfirmed failing against the previous code with exactly the two errors above.
pytest tests/methodologies/para/ tests/integration/test_para_file_mover.py— 702 passed.
ruff check,ruff format --check,mypyclean on both changed files.pre-commit run --files <both files>— all hooks pass.diff-coverwasskipped locally (it runs the full suite single-threaded and times out); both
new branches are directly exercised by the tests above, and CI runs the gate.
Notes for the reviewer
suggest_archiveis referenced only inside its ownmodule and in tests (grepped
.py/.md/.json/.yaml/.tomlrepo-wide), sonothing currently passes a computed or user-supplied threshold. This is a public
library API whose contract is now safe across the full
intrange.safe_walkmigration has not landed for this file. The scan on line ~323is still
directory.rglob("*")onmain(f72ffd74);core.path_guard.safe_walkadoption for the PARA movers is in flight in security(traversal): migrate 13 rglob call sites to safe_walk #1676. This change is independent of
which scan API is used and should not conflict beyond trivial context.
File has not been modified in 0 days (threshold: 0 days)at the saturated threshold, which seemed accurate enough to leave as-is.
Checklist:
Args)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests