Conversation
The streaming docs say `rolling_trust_score` is "row-weighted trust over the recent window," but that's not actually what the code does — it's just a plain mean of the last `rolling_trust_window` batch scores. So a 1-row batch ends up counting exactly as much as a 999-row batch, which isn't what "row-weighted" implies. I checked git blame and the doc line and the implementation both came from the same original commit, so this isn't a regression — the docs have just been wrong since day one. The config docstring for `rolling_trust_window` already says "averaged," which matches the code, so this really is just a doc bug. To confirm, I ran the repro from the issue: - 1-row batch (all missing) → 0 trust - 999-row batch (clean) → 100 trust - rolling comes out to 50.0, not ~100 like row-weighting would give `cumulative_trust_score` on the other hand IS genuinely row-weighted, so I left that line alone and only fixed the `rolling_trust_score` row. Changes: - Corrected the docs table row to describe what the metric actually does - Added two tests pinning down the exact numbers from the issue's repro, so this doesn't quietly drift again - Changelog entry No changes to the actual trust-score logic — that'd be a real behavior change and probably deserves its own discussion if it's ever wanted. Fixes FreshCode-Org#349 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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 |
There was a problem hiding this comment.
🔵 Needs a closer look
The key regression test does not currently exercise the stated 1-row scenario.
Pull request overview
Corrects the streaming documentation for rolling_trust_score and adds regression coverage without changing trust-score logic.
Changes:
- Updates the rolling score description.
- Adds weighting behavior tests.
- Records the fix in the changelog.
File summaries
| File | Summary |
|---|---|
tests/test_streaming_state.py |
Adds weighting tests; the 1-row fixture currently drops the empty row and needs adjustment. |
docs/streaming.md |
Corrects the rolling score description. |
CHANGELOG.md |
Documents the fix. |
Review details
Suppressed comments (1)
tests/test_streaming_state.py:114
- This case does not actually reproduce the stated 1-row batch:
StreamingCleanerdefaults todrop_empty_rows=True, and_build_rep_configleaves that setting enabled, so the all-Nonerow is removed beforerows_in_batchandrecord_trustare computed. As a resultrows[0]is 0 rather than 1, and the test would continue to pass without exercising the issue's 1-vs-999 row weighting; disable empty-row dropping for this fixture (or use an equivalent non-dropped 1-row input).
cleaner = StreamingCleaner(verbose=False)
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The end-to-end test claimed to reproduce the issue's 1-row-vs-999-row split, but it did not. `_build_rep_config` clears `drop_empty_columns` and leaves `drop_empty_rows` at its default, so the all-None row was dropped before the batch was scored — `rows_in_batch` came back as 0, and the test was really exercising a 0-vs-999 split where the small batch carries no weight under either rule. Pass `drop_empty_rows=False` so the all-None row survives, keeping the issue's literal reproduction rather than substituting a different input. The weak `rows[0] < rows[1]` assertion (which passed on 0 < 999) is now an exact `rows == [1, 999]`, pinning the thing that regressed, and the docstring records why the flag is needed. Verified by temporarily making the rolling score row-weighted: the test now fails 99.97 != 85.0, against a genuine 1-vs-999 split. Thanks to the automated review on the PR for catching this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The streaming docs say
rolling_trust_scoreis "row-weighted trust over the recent window," but that's not actually what the code does — it's just a plain mean of the lastrolling_trust_windowbatch scores. So a 1-row batch ends up counting exactly as much as a 999-row batch, which isn't what "row-weighted" implies.I checked git blame and the doc line and the implementation both came from the same original commit. So this isn't a regression, the docs have just been wrong since day one. The config docstring for
rolling_trust_windowalready says "averaged," which matches the code, so this really is just a doc bug.To confirm, I ran the repro from the issue:
cumulative_trust_scoreon the other hand IS genuinely row-weighted, so I left that line alone and only fixed therolling_trust_scorerow.Changes:
No changes to the actual trust-score logic. That'd be a real behavior change and probably deserves its own discussion if it's ever wanted.