Fix email report forecast visibility split - #18
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe report now separates private and public Actions minutes. It reuses consumer rows when available and otherwise fetches repository Actions data. Repository loading occurs when Actions reporting is enabled. Tests cover fallback, filtering, and disabled Actions behavior. ChangesActions visibility reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ReportData
participant Consumers
participant ActionsTable
participant VisibilitySplit
ReportData->>Consumers: collect consumer rows
alt Consumer rows available
Consumers-->>ReportData: return raw repository rows
else Consumer retrieval fails or is disabled
ReportData->>ActionsTable: fetch repository Actions rows
ActionsTable-->>ReportData: return Actions data
end
ReportData->>VisibilitySplit: attach private/public minute totals
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 review |
✅ Action performedReview 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: 4
🧹 Nitpick comments (1)
tests/test_report_data.py (1)
513-531: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest filtering with mixed repository input.
The fixture at Line 524 contains only a public repository. The mocked fallback also returns only a public row. This test passes if the Actions path stops filtering private repositories before the fallback fetch. Use
_TWO_REPOSand assert that the fallback receives no private repository whenonly_public=True.🤖 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 `@tests/test_report_data.py` around lines 513 - 531, Update the test around build_report_data to use the mixed-repository _TWO_REPOS fixture instead of only _PUBLIC_REPO, while keeping only_public=True. Assert that the mocked fetch_repo_actions_table fallback receives only the public repository and excludes the private one, ensuring filtering occurs before the fallback fetch.
🤖 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
`@docs/superpowers/plans/archived/2026-08-10-fix-email-forecast-visibility-split.md`:
- Line 47: Update build_report_data() so the errors returned by the fallback
fetch_repo_actions_table() call are merged into report["errors"] before
attach_actions_visibility_split() runs. Replace the discarded _errors binding
with propagation of those errors, while preserving the existing rows and
visibility-split behavior.
In `@src/github_usage/report_data.py`:
- Around line 407-410: Update the caller around fetch_repo_actions_table so its
returned _errors are recorded through the report error contract, and ensure
attach_actions_visibility_split marks the result incomplete or skips the
private-minute forecast when failures exist. Add a regression test covering a
failed repository Actions billing fetch and the resulting partial-fallback
behavior.
- Around line 339-341: Update estimate_api_request_count() to include one
fallback Actions request per repository when include_actions is enabled,
matching the fetch_repo_actions_table(api, repos) path used by
build_report_data() when repo_consumers data is unavailable. Add a low-quota
test covering Actions-only reporting and verify the quota check accounts for
these requests.
In `@src/github_usage/report_optional.py`:
- Around line 56-58: Normalize the per-repository rows before attaching the
Actions visibility split so storage is attributed correctly. In
src/github_usage/report_optional.py lines 56-58, update the _raw_rows data
passed from get_repo_consumers() to retain or derive storage_gb_hours from
storage_avg_mb. In src/github_usage/report_data.py lines 399-405, ensure the
attachment call consumes rows containing storage_gb_hours rather than
storage_avg_mb alone, preserving existing behavior for other row fields.
---
Nitpick comments:
In `@tests/test_report_data.py`:
- Around line 513-531: Update the test around build_report_data to use the
mixed-repository _TWO_REPOS fixture instead of only _PUBLIC_REPO, while keeping
only_public=True. Assert that the mocked fetch_repo_actions_table fallback
receives only the public repository and excludes the private one, ensuring
filtering occurs before the fallback fetch.
🪄 Autofix
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: d9491270-c178-44d9-bf87-3a31ec982836
📒 Files selected for processing (7)
AGENTS.mdCHANGELOG.mdTO_DO.mddocs/superpowers/plans/archived/2026-08-10-fix-email-forecast-visibility-split.mdsrc/github_usage/report_data.pysrc/github_usage/report_optional.pytests/test_report_data.py
|
|
||
| 1. **`include_consumers=True`** — `get_repo_consumers()` already calls `get_actions_per_repo()` for every repo (`report_optional.py:35`) and builds rows with `repo`, `minutes`, `storage_avg_mb`, and `visibility` — the same shape `attach_actions_visibility_split()` needs. Extend `get_repo_consumers()` to return its raw rows under a `_raw_rows` key (or a dedicated public key), then pass them directly to `attach_actions_visibility_split()`. This avoids duplicating every per-repo API call. | ||
|
|
||
| 2. **`include_consumers=False`** — `get_repo_consumers()` is not called, so no rows are available. Call `fetch_repo_actions_table(api, repos)` from `report_actions.py:172`, which returns `(rows, errors)` — the same function the legacy path uses. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Propagate fallback fetch errors before finalizing the visibility split.
fetch_repo_actions_table() returns (rows, errors), but build_report_data() currently binds the second value to _errors and drops it. A failed repository request then produces a partial visibility split without an entry in report["errors"]. The email can undercount private minutes and the forecast.
Merge the fallback errors before calling attach_actions_visibility_split().
Proposed fix
- rows, _errors = fetch_repo_actions_table(api, repos)
+ rows, fetch_errors = fetch_repo_actions_table(api, repos)
+ errors.update(fetch_errors)🤖 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
`@docs/superpowers/plans/archived/2026-08-10-fix-email-forecast-visibility-split.md`
at line 47, Update build_report_data() so the errors returned by the fallback
fetch_repo_actions_table() call are merged into report["errors"] before
attach_actions_visibility_split() runs. Replace the discarded _errors binding
with propagation of those errors, while preserving the existing rows and
visibility-split behavior.
| needs_repos = ( | ||
| include_actions or include_consumers or include_artifact_storage or include_release_assets | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the estimator definition and all call sites.
ast-grep outline src/github_usage/report_data.py --items all --type function
rg -n -A100 -B5 '^def estimate_api_request_count\(' src/github_usage/report_data.py
rg -n -C5 'estimate_api_request_count\(' src tests
# Inspect Actions-only coverage and quota-related tests.
rg -n -C5 'include_actions|core_remaining|estimated_incremental_requests|quota' tests/test_report_data.pyRepository: kgrizz-git/github-usage
Length of output: 1261
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- report_data.py relevant sections ---'
cat -n src/github_usage/report_data.py | sed -n '230,430p'
printf '%s\n' '--- estimator definitions and call sites ---'
rg -n -C8 'estimate_api_request_count|estimated_incremental_requests|core_remaining' . \
-g '*.py' -g '*.toml' -g '*.yaml' -g '*.yml'
printf '%s\n' '--- report-data test files ---'
git ls-files 'tests/*' | sortRepository: kgrizz-git/github-usage
Length of output: 30109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Actions fallback implementation ---'
rg -n -A100 -B10 'def fetch_repo_actions_table|def get_actions_usage' src/github_usage/report_actions.py src/github_usage/report_data.py
printf '%s\n' '--- Actions request tests and report-data fixtures ---'
rg -n -C12 'fetch_repo_actions_table|include_actions|build_report_data|rate_limit|quota' \
tests/test_report_actions.py tests/test_report_data.py tests/_fakes.py
printf '%s\n' '--- optional estimator implementation and constants ---'
cat -n src/github_usage/report_optional.py | sed -n '1,180p'Repository: kgrizz-git/github-usage
Length of output: 45996
Include Actions fallback requests in the quota estimate.
When include_actions is enabled without usable repo_consumers rows, build_report_data() calls fetch_repo_actions_table(api, repos) after the quota check. estimate_api_request_count() does not count these per-repository requests. Add the Actions request count to the estimate and add an Actions-only low-quota test.
🤖 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/github_usage/report_data.py` around lines 339 - 341, Update
estimate_api_request_count() to include one fallback Actions request per
repository when include_actions is enabled, matching the
fetch_repo_actions_table(api, repos) path used by build_report_data() when
repo_consumers data is unavailable. Add a low-quota test covering Actions-only
reporting and verify the quota check accounts for these requests.
| rows, _errors = fetch_repo_actions_table(api, repos) | ||
| attach_actions_visibility_split( | ||
| report, rows, only_public=only_public, only_private=only_private | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve failed repository Actions fetches.
fetch_repo_actions_table() omits a repository row when its billing request fails and returns that failure in its second result. Line 407 discards those failures. The visibility split can then understate private minutes while the report shows no incomplete-data error. Record these failures through the report error contract and mark the split incomplete, or skip the private-minute forecast. Add a partial-fallback regression test.
🤖 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/github_usage/report_data.py` around lines 407 - 410, Update the caller
around fetch_repo_actions_table so its returned _errors are recorded through the
report error contract, and ensure attach_actions_visibility_split marks the
result incomplete or skips the private-minute forecast when failures exist. Add
a regression test covering a failed repository Actions billing fetch and the
resulting partial-fallback behavior.
| # Used by report_data.build_report_data() to pass per-repo rows to | ||
| # attach_actions_visibility_split() without duplicate API calls. | ||
| "_raw_rows": rows, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the split helper defaults and row-field access.
ast-grep outline src/github_usage/usage_split.py --items all --type function
rg -n -A120 -B5 '^def split_rows_by_visibility\(' src/github_usage/usage_split.py
rg -n -A80 -B5 '^def finalize_actions_split\(' src/github_usage/usage_split.py
# Compare fallback and consumer row construction.
rg -n -A45 -B5 '^def fetch_repo_actions_table\(' src/github_usage/report_actions.py
rg -n -A45 -B5 '^def get_repo_consumers\(' src/github_usage/report_optional.pyRepository: kgrizz-git/github-usage
Length of output: 14870
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the accumulator and the complete attachment path.
sed -n '100,205p' src/github_usage/usage_split.py
sed -n '282,345p' src/github_usage/usage_split.py
sed -n '370,425p' src/github_usage/report_data.py
# Find all consumers of the reused rows and all storage_avg_mb/storage_gb_hours
# transformations to determine whether the schema mismatch affects output.
rg -n -A20 -B10 'storage_avg_mb|storage_gb_hours|_raw_rows|attach_actions_visibility_split' src tests 2>/dev/null || trueRepository: kgrizz-git/github-usage
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '100,205p' src/github_usage/usage_split.py
sed -n '282,345p' src/github_usage/usage_split.py
sed -n '370,425p' src/github_usage/report_data.py
rg -n -A20 -B10 'storage_avg_mb|storage_gb_hours|_raw_rows|attach_actions_visibility_split' src tests 2>/dev/null || trueRepository: kgrizz-git/github-usage
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- usage_split accumulator ---'
sed -n '145,200p' src/github_usage/usage_split.py
printf '%s\n' '--- attach_actions_visibility_split ---'
sed -n '282,345p' src/github_usage/usage_split.py
printf '%s\n' '--- report_data attachment call ---'
sed -n '390,415p' src/github_usage/report_data.pyRepository: kgrizz-git/github-usage
Length of output: 4160
Normalize consumer rows before attaching the Actions split.
attach_actions_visibility_split() reads storage_gb_hours by default. _raw_rows contains only storage_avg_mb, so reused rows contribute zero visibility storage and place the full account storage in unattributed_storage_gb_hours. Retain storage_gb_hours in get_repo_consumers() rows, or normalize the rows before the attachment call.
📍 Affects 2 files
src/github_usage/report_optional.py#L56-L58(this comment)src/github_usage/report_data.py#L399-L405
🤖 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/github_usage/report_optional.py` around lines 56 - 58, Normalize the
per-repository rows before attaching the Actions visibility split so storage is
attributed correctly. In src/github_usage/report_optional.py lines 56-58, update
the _raw_rows data passed from get_repo_consumers() to retain or derive
storage_gb_hours from storage_avg_mb. In src/github_usage/report_data.py lines
399-405, ensure the attachment call consumes rows containing storage_gb_hours
rather than storage_avg_mb alone, preserving existing behavior for other row
fields.
|
1 participant
The email report forecast was counting total Actions minutes (private + public) against the 2,000-minute free-tier limit. Only private repos consume quota—public repos are free. This fix ensures the email report now correctly uses private-only minutes, matching the legacy terminal report behavior.
Changes
attach_actions_visibility_split()call tobuild_report_data()to compute private/public split on account-level Actions minutesneeds_reposto includeinclude_actionsso repos are always fetched when needed for the splitget_repo_consumers()to expose raw per-repo rows (_raw_rowskey) for reuse by the visibility splitfetch_repo_actions_table()when consumers aren't enabledImpact
Tests
Added 4 new test cases:
fetch_repo_actions_table()fallback is usedfetch_repo_actions_table()Summary by CodeRabbit