fix(analytics): reject invalid KPI numeric evidence on current main - #421
fix(analytics): reject invalid KPI numeric evidence on current main#421seonghobae wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 32 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughKPI ledger 파싱이 비율의 유한성 및 ChangesKPI ledger 검증
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
src/test/java/com/clearfolio/viewer/analytics/KpiSnapshotLedgerTest.java (1)
89-98: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win허용 경계값 테스트를 추가하세요.
현재 테스트는
0.5만 유효한 비율로 사용하고-0.01,1.01만 거부합니다.rate의 허용 범위는[0.0, 1.0]이므로0.0과1.0을 성공적으로 로드하는 사례를 추가하세요.KpiSnapshotResponse.from은p95TimeToPreviewMs를0L로 생성할 수 있으므로,0도 성공 사례로 확인하세요. 이 테스트가 없으면 경계 조건이 잘못되어도 회귀 테스트가 통과할 수 있습니다.🤖 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 `@src/test/java/com/clearfolio/viewer/analytics/KpiSnapshotLedgerTest.java` around lines 89 - 98, The rejectsNonFiniteOrOutOfRangePersistedKpiEvidence test currently lacks valid boundary cases. Add successful-load assertions covering rate values 0.0 and 1.0, plus p95TimeToPreviewMs equal to 0, using the existing valid-ledger assertion/helper flow.
🤖 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.
Nitpick comments:
In `@src/test/java/com/clearfolio/viewer/analytics/KpiSnapshotLedgerTest.java`:
- Around line 89-98: The rejectsNonFiniteOrOutOfRangePersistedKpiEvidence test
currently lacks valid boundary cases. Add successful-load assertions covering
rate values 0.0 and 1.0, plus p95TimeToPreviewMs equal to 0, using the existing
valid-ledger assertion/helper flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f190d4fb-dd21-4208-b2f8-37939bb89dee
📒 Files selected for processing (2)
src/main/java/com/clearfolio/viewer/analytics/KpiSnapshotLedger.javasrc/test/java/com/clearfolio/viewer/analytics/KpiSnapshotLedgerTest.java
|
Exact-current-head read-only review request for @opencode-agent review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head_commit="2c7d16752eddd675593c760d3da00d781b7b74f4"
base_commit="961f44a19c4ced87f8062b3ab9842a9a08d05235"
echo "== Repository state =="
git rev-parse HEAD
git cat-file -t "$head_commit"
git cat-file -t "$base_commit"
git merge-base --is-ancestor "$base_commit" "$head_commit" && echo "BASE_IS_ANCESTOR=true" || echo "BASE_IS_ANCESTOR=false"
echo "== Requested diff =="
git diff --check "$base_commit" "$head_commit"
git diff --find-renames --unified=80 "$base_commit" "$head_commit" -- \
src/main/java/com/clearfolio/viewer/analytics/KpiSnapshotLedger.java \
src/test/java/com/clearfolio/viewer/analytics/KpiSnapshotLedgerTest.java
echo "== Production file outline =="
ast-grep outline src/main/java/com/clearfolio/viewer/analytics/KpiSnapshotLedger.java --items all --view expanded
echo "== Test file outline =="
ast-grep outline src/test/java/com/clearfolio/viewer/analytics/KpiSnapshotLedgerTest.java --items all --view expanded
echo "== Validation and replay call sites =="
rg -n -C 5 -P '\b(KpiSnapshotLedger|Double\.parseDouble|Double\.isFinite|nullableLong|invalid line|read|parse|replay)\b' \
src/main/java src/test/javaLength of output: 101296
No findings.
The regression tests cover the invalid values and the inclusive valid boundaries. The diff has no whitespace errors. This review did not modify the branch.
|
|
@opencode-agent review Review exact head Review read-only; preserve independent reviewer identity. Do not mutate the branch, synthesize author approval, weaken protection, or merge. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
head_commit="2c7d16752eddd675593c760d3da00d781b7b74f4"
base_commit="961f44a19c4ced87f8062b3ab9842a9a08d05235"
prod="src/main/java/com/clearfolio/viewer/analytics/KpiSnapshotLedger.java"
test="src/test/java/com/clearfolio/viewer/analytics/KpiSnapshotLedgerTest.java"
echo "== Exact commit verification =="
printf 'HEAD='
git rev-parse HEAD
printf 'REQUESTED_HEAD='
git rev-parse "$head_commit"
printf 'BASE='
git rev-parse "$base_commit"
git merge-base --is-ancestor "$base_commit" "$head_commit" && echo "BASE_IS_ANCESTOR=true"
git diff --check "$base_commit" "$head_commit"
echo "== Changed paths =="
git diff --name-status "$base_commit" "$head_commit"
echo "== Requested diff =="
git diff --find-renames --unified=70 "$base_commit" "$head_commit" -- "$prod" "$test"
echo "== Production outline =="
ast-grep outline "$prod" --items all --view expanded
echo "== Test outline =="
ast-grep outline "$test" --items all --view expanded
echo "== Production validation and parse boundary =="
rg -n -C 12 -P '\b(rate|nullableLong|Double\.parseDouble|Double\.isFinite|invalidLine|invalid line|read|parse)\b' "$prod"
echo "== Regression tests for numeric replay data =="
rg -n -C 10 -P '\b(NaN|Infinity|POSITIVE_INFINITY|NEGATIVE_INFINITY|rate|p95|invalidLine|invalid line|replay|ledger)\b' "$test"
echo "== KPI denominator and tenant-scope references in changed production file =="
rg -n -C 8 -P '\b(denominator|tenant|Tenant|query|rate|snapshot)\b' "$prod"Length of output: 33247
No findings.
The replay field mapping remains unchanged. KPI denominator values and tenant filtering remain unchanged. The regression tests cover invalid replay values and valid inclusive boundaries. The review was read-only. I did not mutate the branch, approve, merge, or change repository protections. I did not execute tests in this inspection; I considered the reported terminal-success checks as supplied context.
|
|
Superseded by current-main PR #437. The exact #421 numeric-integrity delta has been test-first reconciled into #437's versioned terminal-outcomes ledger on current protected base: persisted NaN, infinities, rates outside #437 now owns the combined denominator-version migration and domain-valid durable numeric contract. No check, review, or approval evidence from #421 transfers to #437's new head. |
Pull request was closed
1 participant
Objective
Reject impossible numeric KPI evidence during durable snapshot replay.
Exact current-head evidence
961f44a19c4ced87f8062b3ab9842a9a08d05235;6610529acb9b6bdf41c1f7d5d491dbf00fdfef7e;2c7d16752eddd675593c760d3da00d781b7b74f4;Contract
Persisted NaN, positive/negative infinity, rates outside
[0,1], and negative p95 preview latency fail closed through the existing controlled invalid-line error instead of entering buyer-visible analytics.Scope boundary
KPI replay numeric integrity only. Denominator semantics, tenant query isolation, API shape, and canonical documentation are unchanged.
Merge gate
Keep this exact head unchanged. Auto-merge may act only after live checks remain successful, zero valid unresolved findings remain, and a qualifying independent non-author approval is attached to this exact head. Automated evidence and predecessor reviews do not transfer.