feat(scripts): rebuild_log audit script (#288 phase-1c) - #350
Merged
Conversation
Reads per-session rebuild_log JSONL files (output of phase-1a) and prints a summary: pack-rate distribution (mean / p50 / p90), drop- reason histogram bucketed by prefix (so below_floor:0.40 and below_floor:0.41 collapse), and the rank distribution of packed rows. Reads only — never modifies inputs. Tolerant of malformed JSONL lines (the writer is fail-soft so partial last lines are expected). Closes the phase-1 implementation tracker for #288. Phase 2 (fixed- corpus precision harness) blocks on a week of operator-collected logs per the ratified spec.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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. Review rate limit: 0/1 reviews remaining, refill in 39 minutes and 6 seconds.Comment |
Reviewer's GuideAdds a standalone audit script and its tests for summarising rebuild_log JSONL files, including pack-rate stats, drop-reason bucketing, packed-rank distribution, truncated-session counting, and a small CLI wrapper with robust file handling and malformed-line tolerance. Sequence diagram for audit_rebuild_log CLI execution flowsequenceDiagram
actor Developer
participant audit_rebuild_log_py as audit_rebuild_log_py
participant argparse_module as argparse
participant file_system as filesystem
participant summariser as summariser
Developer->>audit_rebuild_log_py: invoke with paths
audit_rebuild_log_py->>argparse_module: parse_args(argv)
argparse_module-->>audit_rebuild_log_py: args.paths
audit_rebuild_log_py->>file_system: _collect_paths(paths)
file_system-->>audit_rebuild_log_py: list_of_jsonl_paths
alt no_paths
audit_rebuild_log_py-->>Developer: exit code 1 (no readable input)
else have_paths
loop for each path
audit_rebuild_log_py->>file_system: _iter_records(path)
file_system-->>audit_rebuild_log_py: decoded_records (malformed lines skipped)
end
audit_rebuild_log_py->>summariser: _summarise(all_records)
summariser-->>audit_rebuild_log_py: summary_dict
audit_rebuild_log_py->>audit_rebuild_log_py: _print_report(summary_dict, paths_read)
audit_rebuild_log_py-->>Developer: exit code 0 (summary printed)
end
Class diagram for audit_rebuild_log script structureclassDiagram
class AuditRebuildLogScript {
+_iter_records(path: Path) Iterable_dict
+_collect_paths(targets: list_Path) list_Path
+_percentile(values: list_float, p: float) float
+_summarise(records: list_dict) dict
+_bucketise_drop_reasons(reasons: Counter_str) Counter_str
+_print_report(summary: dict, paths_read: int) void
+main(argv: list_str) int
}
class Path
class Counter_str {
+__getitem__(key: str) int
+items() Iterable_tuple_str_int
+most_common() list_tuple_str_int
}
AuditRebuildLogScript --> Path : uses
AuditRebuildLogScript --> Counter_str : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
robotrocketscience
added
attn:review
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| from __future__ import annotations | ||
|
|
||
| import importlib.util | ||
| import io |
| import importlib.util | ||
| import io | ||
| import json | ||
| import sys |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Both
_iter_recordsandmaincurrently materialize entire files / record sets into memory; consider iterating over file handles line-by-line and streaming records into_summarise(or making_summariseaccept an iterator) to keep memory usage bounded for large rebuild logs. - _iter_records
usesPath.read_text()andsplitlines()which lose line-offset context and load the entire file; switching towith path.open() as fand iteratingfor line in f` would be more efficient and avoid holding large JSONL files in memory.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Both `_iter_records` and `main` currently materialize entire files / record sets into memory; consider iterating over file handles line-by-line and streaming records into `_summarise` (or making `_summarise` accept an iterator) to keep memory usage bounded for large rebuild logs.
- _iter_records` uses `Path.read_text()` and `splitlines()` which lose line-offset context and load the entire file; switching to `with path.open() as f` and iterating `for line in f` would be more efficient and avoid holding large JSONL files in memory.
## Individual Comments
### Comment 1
<location path="scripts/audit_rebuild_log.py" line_range="73" />
<code_context>
+ return out
+
+
+def _percentile(values: list[float], p: float) -> float:
+ """Nearest-rank percentile. Empty -> 0.0."""
+ if not values:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Percentile implementation doesn’t match standard nearest-rank definition and may behave unexpectedly at extremes.
The implementation uses `int(round((p/100) * (n-1)))`, which is a rounded, linearly-scaled index rather than conventional nearest-rank. Standard nearest-rank for p∈(0,100] is based on `ceil(p/100 * n)` (index `ceil(p/100 * n) - 1`). If percentile behavior is important for diagnostics, consider switching to the standard formula and explicitly defining behavior for p=0 and empty input to avoid surprising results.
Suggested implementation:
```python
def _percentile(values: list[float], p: float) -> float:
"""Nearest-rank percentile.
Uses the standard nearest-rank definition for 0 < p <= 100:
rank = ceil(p / 100 * n), index = rank - 1
Special cases:
- Empty input -> 0.0
- p <= 0 -> minimum value
- p >= 100 -> maximum value
"""
if not values:
return 0.0
s = sorted(values)
n = len(s)
if p <= 0:
return s[0]
if p >= 100:
return s[-1]
rank = math.ceil((p / 100.0) * n)
index = min(max(rank - 1, 0), n - 1)
return s[index]
```
The updated implementation uses `math.ceil`, so ensure `math` is imported at the top of `scripts/audit_rebuild_log.py`:
- Add `import math` alongside other imports (or reuse an existing `math` import if already present).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return out | ||
|
|
||
|
|
||
| def _percentile(values: list[float], p: float) -> float: |
There was a problem hiding this comment.
suggestion (bug_risk): Percentile implementation doesn’t match standard nearest-rank definition and may behave unexpectedly at extremes.
The implementation uses int(round((p/100) * (n-1))), which is a rounded, linearly-scaled index rather than conventional nearest-rank. Standard nearest-rank for p∈(0,100] is based on ceil(p/100 * n) (index ceil(p/100 * n) - 1). If percentile behavior is important for diagnostics, consider switching to the standard formula and explicitly defining behavior for p=0 and empty input to avoid surprising results.
Suggested implementation:
def _percentile(values: list[float], p: float) -> float:
"""Nearest-rank percentile.
Uses the standard nearest-rank definition for 0 < p <= 100:
rank = ceil(p / 100 * n), index = rank - 1
Special cases:
- Empty input -> 0.0
- p <= 0 -> minimum value
- p >= 100 -> maximum value
"""
if not values:
return 0.0
s = sorted(values)
n = len(s)
if p <= 0:
return s[0]
if p >= 100:
return s[-1]
rank = math.ceil((p / 100.0) * n)
index = min(max(rank - 1, 0), n - 1)
return s[index]The updated implementation uses math.ceil, so ensure math is imported at the top of scripts/audit_rebuild_log.py:
- Add
import mathalongside other imports (or reuse an existingmathimport if already present).
Owner
Author
|
[claim:review:Toug:2026-05-02T19:40:28Z] |
Owner
Author
|
[release:review:Toug:2026-05-02T19:41:10Z] |
robotrocketscience
removed
the
attn:review
Owner
Author
|
[claim:review:Kulili:2026-05-02T19:41:27Z] |
Owner
Author
|
[release:review:Kulili:2026-05-02T19:42:14Z] |
This was referenced May 2, 2026
Closed
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
2 participants
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.
Phase-1c companion to the merged phase-1a rebuild diagnostic log (#288).
scripts/audit_rebuild_log.pyreads per-session rebuild_log JSONL files and prints:n_packed / n_candidatesbelow_floor:0.40,below_floor:0.41collapse tobelow_floor: N)Reads only — never modifies inputs. Tolerant of malformed JSONL lines (the writer is fail-soft so a partial last line is expected on a crash). Accepts files or directories of
*.jsonl.Tests cover: summary maths (records, pack-rate, packed ranks, drop-reason counts), truncated-marker accounting, missing pack_summary tolerance, drop-reason prefix bucketing, malformed-line skipping, directory expansion, percentile maths, and CLI exit codes (0 on summary, 1 on no-input).
Closes the phase-1 implementation tracker for #288. Phase 2 (fixed-corpus precision harness) blocks on a week of operator-collected logs per the ratified spec at
docs/rebuild_eval_harness.md.Summary by Sourcery
Add a CLI script to audit rebuild_log JSONL session files and report aggregate packing and drop statistics.
New Features:
Tests: