Fix false test skip from empty impl_file_content and remove dead read_file_content#2328
Merged
hiroshinishio merged 2 commits intomainfrom Feb 26, 2026
Merged
Fix false test skip from empty impl_file_content and remove dead read_file_content#2328hiroshinishio merged 2 commits intomainfrom
hiroshinishio merged 2 commits intomainfrom
Conversation
…e_content with read_local_file - Fix bug where should_skip_test received empty impl_file_content because get_raw_content was skipped when file was in reference_file_paths. Now reads from clone_dir via read_local_file after prepare_repo_for_work. - Add format_content_with_line_numbers before sending impl_file_content to agent. - Replace reference_file_paths (set) + reference_contents (list) with reference_files (dict) for cleaner data structure. - Delete read_file_content.py (dead code) and replace all usages with read_local_file since clone is always done before these reads. - Simplify detect_package_manager, ensure_node_packages, ensure_php_packages signatures by removing unused owner/repo/branch/token params. - Update all callers and tests for the simplified signatures.
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
1 participant
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
should_skip_testwas receiving emptyimpl_file_contentbecauseget_raw_contentwas conditionally skipped when the file was already inreference_file_paths. This caused GitAuto to incorrectly close PRs saying "no testable code". Now reads fromclone_dirviaread_local_fileafterprepare_repo_for_work.read_file_content.py(wrapper aroundread_local_filewith GitHub API fallback) since clone is always done before these reads. Replaced all 5 production callers withread_local_file.owner/repo/branch/tokenparams fromdetect_package_manager,ensure_node_packages, andensure_php_packages. Updated all callers and tests.format_content_with_line_numbersbefore sendingimpl_file_contentto the agent.reference_file_paths(set) +reference_contents(list) withreference_files(dict).Social Media Post (GitAuto)
A customer's PR was closed with "calculator.py has no testable code" - but the file had testable code. The bug: when the implementation file was already in the reference file list, the content fetch was skipped, leaving an empty string. Empty string passed to the skip-test checker, which correctly said "nothing to test here." Fixed by reading from the local clone after checkout instead of relying on a conditional API fetch.
Social Media Post (Wes)
Found a bug where GitAuto closed a perfectly valid PR because it thought calculator.py had no testable code. Traced through CloudWatch logs to the skip check receiving an empty string. The content fetch had a conditional path - if the file was already referenced elsewhere, it skipped fetching entirely. Empty string goes to the "is this testable?" check, answer is no, PR gets closed. While fixing that, noticed the GitHub API wrapper (read_file_content) was dead code since we always clone first now. Removed it and simplified three function signatures that were passing around params nobody used.