fix(evolution): account for every generated file - #7
Merged
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Reviewer's GuideUpdates the evolution refresh workflow to count every generated file (including those in previously-collapsed untracked directories and the generated summary itself) when enforcing file/line budgets, and adds a regression test to lock in the new git status behavior. Sequence diagram for updated refresh budget enforcement workflowsequenceDiagram
title run_refresh sequence with remeasurement and budget enforcement
actor Cron
participant run_refresh
participant _write_summary
participant _git_changes
participant _run
participant enforce_change_budget
Cron->>run_refresh: run_refresh(args, root)
alt [not args.dry_run]
run_refresh->>_write_summary: _write_summary(root, run_date, summary)
run_refresh->>_git_changes: _git_changes(root)
_git_changes->>_run: _run(["git","status","--porcelain=v1","--untracked-files=all"], root)
_run-->>_git_changes: output
_git_changes-->>run_refresh: changed_files, changed_lines
run_refresh->>_write_summary: _write_summary(root, run_date, summary)
run_refresh->>_git_changes: _git_changes(root)
_git_changes->>_run: _run(["git","status","--porcelain=v1","--untracked-files=all"], root)
_run-->>_git_changes: output
_git_changes-->>run_refresh: changed_files, changed_lines
run_refresh->>_write_summary: _write_summary(root, run_date, summary)
run_refresh->>enforce_change_budget: enforce_change_budget(changed_files, changed_lines, args.max_files, args.max_lines)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
run_refresh, the sequence of_write_summaryand_git_changescalls is repeated three times; consider refactoring to a single helper or clearly documenting why multiple writes/measurements are required, and whether this can be reduced to the minimal number of passes needed to account for the summary file.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `run_refresh`, the sequence of `_write_summary` and `_git_changes` calls is repeated three times; consider refactoring to a single helper or clearly documenting why multiple writes/measurements are required, and whether this can be reduced to the minimal number of passes needed to account for the summary file.
## Individual Comments
### Comment 1
<location path="scripts/evolve/refresh.py" line_range="209-208" />
<code_context>
}
if not args.dry_run:
_write_summary(root, run_date, summary)
+ # Include the generated summary itself and expand every untracked file
+ # before enforcing/reported budgets. A second measurement is needed
+ # because adding the summary path grows the summary by one list entry.
+ changed_files, changed_lines = _git_changes(root)
+ summary["changed_files"] = changed_files
+ summary["changed_lines"] = changed_lines
+ _write_summary(root, run_date, summary)
+ changed_files, changed_lines = _git_changes(root)
+ summary["changed_files"] = changed_files
</code_context>
<issue_to_address>
**issue (bug_risk):** The summary is written three times and `_git_changes` is called twice; this sequencing is hard to reason about and likely not doing exactly what the comment describes.
Right now the sequence is:
- write summary (v1)
- measure1
- write summary with measure1 (v2)
- measure2
- write summary with measure2 (v3)
- enforce budget based on measure2
That means the budget is enforced on the repo state as of v2, not the final on-disk summary (v3), and v3 itself changes the state after the budget check. This off‑by‑one in what’s being measured adds complexity and extra I/O.
Could we simplify this so that the budget is enforced on the final written summary (e.g., two writes and one/two measurements with a clear invariant about what’s included), or document why three writes/two measurements are strictly required?
</issue_to_address>
### Comment 2
<location path="tests/test_evolution.py" line_range="391" />
<code_context>
def test_refresh_budget_rejects_unreviewable_change_sets():
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test that exercises `run_refresh`’s new behavior of enforcing budgets against the final change set including the generated summary file.
Production now re-runs `_git_changes` and calls `enforce_change_budget` after writing the generated summary so budgets apply to the final change set (including the summary file and fully expanded untracked files). The current tests only cover `_git_changes` directly and the existing `test_refresh_budget_rejects_unreviewable_change_sets()` behavior.
To fully validate this bugfix, add an integration-style test around `run_refresh` that:
- uses a temporary git repo,
- creates a case where the pre-summary measurement is under budget but the post-summary measurement exceeds it (e.g., via `max_files` so the summary file pushes it over), and
- verifies that `enforce_change_budget` is called with the final `changed_files`/`changed_lines` (e.g., via mocking) and that an over-budget scenario is rejected.
This will help prevent regressions where `run_refresh` might again enforce budgets on a pre-summary snapshot.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
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.
Summary
Verification
python3 tests/test_evolution.pypython3 scripts/validate.pygit diff --checkFound while deploying the live cron automation: bootstrap PR #6 contained 13 files while its pre-fix summary reported 2 because Git collapsed
candidates/runs/into one untracked directory entry.Summary by Sourcery
Ensure evolution refresh accounting includes all generated files and enforces budgets against the final change set.
Bug Fixes:
Tests: