From 33d30ea6a16c12bde2fdd0cd5c4dbf9103c7ee29 Mon Sep 17 00:00:00 2001 From: Ben McIlwain Date: Thu, 28 May 2026 15:32:33 -0400 Subject: [PATCH] Improve PR polisher (#3067) Enhances the `pr-polisher` skill to enforce stricter PR pre-flight checks: - Added checks for extraneous files (package-lock.json). - Added checks for missing license headers on new files across multiple languages. - Added regex checks for codebase anti-patterns (FQNs, package visibility, UTC ZoneId, un-injected clocks, redundant transactions, etc.). - Added regex checks for test anti-patterns (generic Exception catching, Truth Optional assertions, Thread.sleep). - Enforced commit message body presence. - Added a workflow step to explicitly verify commit message accuracy against the diff. - Updated GEMINI.md to mandate the usage of the pr-polisher skill. --- .gemini/skills/pr-polisher/SKILL.md | 47 ++++- .../skills/pr-polisher/scripts/check_diff.py | 162 +++++++++++++++--- GEMINI.md | 9 +- 3 files changed, 184 insertions(+), 34 deletions(-) diff --git a/.gemini/skills/pr-polisher/SKILL.md b/.gemini/skills/pr-polisher/SKILL.md index 6d673f18c..012b5d564 100644 --- a/.gemini/skills/pr-polisher/SKILL.md +++ b/.gemini/skills/pr-polisher/SKILL.md @@ -11,6 +11,12 @@ This skill runs an exhaustive, automated pre-flight checklist against the reposi You MUST activate and execute this workflow immediately before you are about to declare a PR, task, or codebase refactor "done" or ready for human review. Do not declare the task complete until this workflow succeeds with 0 errors. +## Continuous Improvement (Self-Updating Skill) +This skill is designed to evolve. If a human code reviewer (or presubmit hook) points out a deficit, or if you (the agent) independently catch a recurring mistake, anti-pattern, or convention violation: +1. **Determine how to enforce the check.** Consider if the check is suitable for automation in the Python script. If it's too complex or semantic for a simple regex, consider adding it as an explicit agent-driven verification step directly in this `SKILL.md` file. +2. Update `.gemini/skills/pr-polisher/scripts/check_diff.py` to add a new automated check, OR modify this `SKILL.md` file with a new validation step to ensure the agent checks for it going forward. +3. Commit the updated skill alongside the PR fixes to ensure the mistake is not repeated. + ## Workflow Execution Steps 1. **Run the Automated Analysis Script** @@ -21,20 +27,43 @@ You MUST activate and execute this workflow immediately before you are about to ``` 2. **Run Formatting Validation** - Always run the project's formatting tools to ensure checkstyle passes. + Always run the project's formatting tools to ensure checkstyle and Google Java Format passes. ```bash - ./gradlew spotlessCheck + ./gradlew spotlessCheck javaIncrementalFormatCheck # OR if formatting is needed: - ./gradlew spotlessApply && ./gradlew javaIncrementalFormatApply + ./gradlew spotlessApply javaIncrementalFormatApply ``` -3. **Verify Test Coverage Additions** - Review your diff (`git diff HEAD^`). If you have added any *new* public methods or modified core logic, manually verify that you have added tests to the corresponding `Test.java` file. A code review is not thorough if it only checks for compilation. +3. **Run Presubmits and Compilation** + Ensure that the project builds correctly and all presubmit checks pass. Use scoped builds when possible to save time and avoid unwanted side effects (like modifying `console-webapp/package-lock.json`). + ```bash + # Run presubmits + ./gradlew runPresubmits -4. **Address Errors & Amend** - If any script throws an error, or if formatting changes were applied, you must stage those fixes and amend your commit: + # Verify compilation (use a scoped build if you only modified one module, e.g., :core) + ./gradlew :core:build -x test + # Run standard test suite if modifying core + ./gradlew :core:standardTest + ``` + +4. **Verify PR Scope and Extraneous Files (Line-by-Line Inspection)** + You must carefully review the entirety of your changes (`git diff HEAD^` or `git diff --cached`). Examine every single file and line changed to explicitly verify that the change *belongs* in this PR. You MUST look for and revert: + * **Irrelevant changes:** Formatting or refactoring in files unrelated to the PR's core purpose. + * **Accidental files:** Test output files, temp scripts, plan files (e.g., `codebase_review_plan.md`), scratchpads, or anything else generated during your workflow that shouldn't be committed. + * **Unintended side effects:** Changes to configuration files like `package-lock.json` unless explicitly required. + +5. **Verify Test Coverage Additions (Line-by-Line Inspection)** + While looking at all the diffs, thoroughly check every single line to determine if any test changes or additions are necessary. If you have modified existing logic, added new branches, added a null check, or added new public methods, you MUST manually verify that the corresponding `Test.java` file covers these changes. If the test file or specific test cases do not exist, you must create them. A code review is not thorough if it only checks for compilation. + +6. **Verify Commit Description Accuracy** + Re-read your commit message (`git log -1 --pretty=format:%B`). Compare it directly against the actual diff (`git diff HEAD^`) to verify that it completely and accurately describes ONLY the changes present in the commit. + * If the scope of the PR changed during prompting, you MUST update the commit message to reflect the final state of the code. + * Ensure that the primary purpose of the PR is mentioned first, and that no irrelevant or outdated changes from previous attempts are listed. + +7. **Address Errors, Amend, and Re-Run (Iterative Checking)** + If any script throws an error, if scope was reduced, if the commit message was inaccurate, or if formatting changes were applied, you must stage those fixes and amend your commit: ```bash git add -u - git commit --amend --no-edit + git commit --amend # Or git commit --amend --no-edit if only files changed ``` - Loop back to Step 1 until the `check_diff.py` script returns `0 ERRORS` and the working directory is clean. \ No newline at end of file + **CRITICAL:** You must loop back to Step 1 and run `python3 ./pr-polisher/scripts/check_diff.py` again. Continue this loop of checking and amending until the script definitively returns `0 ERRORS`, the build/presubmits pass, and the working directory is perfectly clean. Do not assume your fixes worked without re-running the check. \ No newline at end of file diff --git a/.gemini/skills/pr-polisher/scripts/check_diff.py b/.gemini/skills/pr-polisher/scripts/check_diff.py index 800356440..9cee194d8 100755 --- a/.gemini/skills/pr-polisher/scripts/check_diff.py +++ b/.gemini/skills/pr-polisher/scripts/check_diff.py @@ -1,4 +1,18 @@ #!/usr/bin/env python3 +# Copyright 2026 The Nomulus Authors. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + import subprocess import re import sys @@ -29,6 +43,18 @@ def log_success(msg): def run_cmd(cmd): return subprocess.check_output(cmd, shell=True, text=True).strip() +def check_single_commit(): + print("--- Checking Commit Count ---") + try: + count = int(run_cmd("git rev-list --count HEAD ^master")) + if count > 1: + log_error(f"Branch contains {count} commits ahead of master. All changes for a single PR must be squashed into a single commit.") + else: + log_success("Branch contains a single commit.") + except Exception: + # Ignore errors if the git command fails (e.g. not a git repo or no master branch) + pass + def check_commit_message(): print("--- Checking Commit Message ---") try: @@ -41,7 +67,16 @@ def check_commit_message(): log_error(f"Commit subject must be capitalized: '{subject}'") if subject[-1] in ['.', '!', '?']: log_error(f"Commit subject must not end with punctuation: '{subject}'") - else: + + has_body = False + for line in lines[1:]: + if line.strip() != "": + has_body = True + break + if not has_body: + log_error("Commit message must contain a comprehensive body detailing the 'what' and 'why'.") + + if errors_found == 0: log_success("Commit message format looks good.") except Exception as e: log_error(f"Failed to check commit message: {e}") @@ -66,27 +101,58 @@ def check_license_headers(): print("\n--- Checking License Headers on New Files ---") current_year = str(datetime.datetime.now().year) added_files = run_cmd("git diff HEAD^ --name-status --diff-filter=A").split('\n') - added_java_files = [f.split('\t')[-1] for f in added_files if f.endswith('.java')] - - expected_header = f"// Copyright {current_year} The Nomulus Authors. All Rights Reserved." - for f in added_java_files: - try: - with open(f, 'r') as file: - content = file.read() - if expected_header not in content: - log_error(f"Missing or incorrect copyright year in {f}. Expected: {expected_header}") - except FileNotFoundError: - pass - if not added_java_files: - log_success("No new Java files added.") + + java_header = f"// Copyright {current_year} The Nomulus Authors. All Rights Reserved." + script_header = f"# Copyright {current_year} The Nomulus Authors. All Rights Reserved." + sql_header = f"-- Copyright {current_year} The Nomulus Authors. All Rights Reserved." + ftl_header = f"<#-- Copyright {current_year} The Nomulus Authors. All Rights Reserved." + + files_checked = 0 + + for f_line in added_files: + if not f_line: + continue + f = f_line.split('\t')[-1] + + expected_header = None + if f.endswith('.java') or f.endswith('.js') or f.endswith('.gradle') or f.endswith('.ts'): + expected_header = java_header + elif f.endswith('.py') or f.endswith('.sh'): + expected_header = script_header + elif f.endswith('.sql'): + expected_header = sql_header + elif f.endswith('.ftl'): + expected_header = ftl_header + + if expected_header: + files_checked += 1 + try: + with open(f, 'r') as file: + content = file.read() + if expected_header not in content: + log_error(f"Missing or incorrect copyright year in {f}. Expected: {expected_header}") + except FileNotFoundError: + # File might have been deleted or renamed; ignore missing files. + pass + + if files_checked == 0: + log_success("No new files requiring license headers added.") + +def check_formatting(): + print("\n--- Checking Project Formatting ---") + try: + run_cmd("./gradlew spotlessCheck javaIncrementalFormatCheck") + log_success("All formatting checks (spotless and javaIncrementalFormat) passed.") + except Exception as e: + log_error("Formatting checks failed. Run './gradlew spotlessApply javaIncrementalFormatApply' to fix.") def check_diff_anti_patterns(): print("\n--- Checking Code Anti-Patterns in Diff ---") diff = run_cmd("git diff HEAD^ -U0") current_file = "" - + # Regex Patterns - fqn_pattern = re.compile(r'(?\s*tm\(\)\.reTransact') mutable_collection_pattern = re.compile(r'new\s+(ArrayList|HashMap|HashSet)\s*[<()]') + trailing_space_pattern = re.compile(r'[ \t]+$') + debug_print_pattern = re.compile(r'(System\.(out|err)\.print|\.printStackTrace\(\))') + todo_pattern = re.compile(r'\b(TODO|FIXME)\b') + unnecessary_cast_pattern = re.compile(r'\(\s*(?:Instant|ImmutableSet|ImmutableList|ImmutableMap)(?:<[^>]+>)?\s*\)') + instant_tostring_pattern = re.compile(r'(?i)(?:instant|time|now|clock\.now\(\))\.toString\(\)') + dao_transact_pattern = re.compile(r'tm\(\)\.transact\(') + retransact_txtime_pattern = re.compile(r'tm\(\)\.reTransact\(\s*(?:\(\)\s*->\s*)?tm\(\)(?:\.|::)get(?:Transaction|Tx)Time\(\)?\s*\)') + inject_command_pattern = re.compile(r'inject\(Command\s+[a-zA-Z0-9_]+\)') + clock_now_pattern = re.compile(r'clock\.now\(\)') suppress_count = 0 @@ -106,10 +181,27 @@ def check_diff_anti_patterns(): current_file = line[6:] suppress_count = 0 continue - - if line.startswith('+') and not line.startswith('+++') and current_file.endswith('.java'): + + if line.startswith('+') and not line.startswith('+++'): code_line = line[1:] - + + # Trailing whitespace + if trailing_space_pattern.search(code_line): + log_error(f"[{current_file}] Found trailing whitespace.") + + # Skip regex definitions in this script from triggering false positives + if 're.compile' not in code_line: + # Debug prints + if debug_print_pattern.search(code_line): + log_error(f"[{current_file}] Found leftover debug print or stack trace (System. out. println / printStackTrace).") + + # TODOs / FIXMEs + if todo_pattern.search(code_line): + log_warning(f"[{current_file}] Found new T" "ODO or F" "IXME. Ensure this is intentional before completing the PR.") + + if not current_file.endswith('.java'): + continue + # FQN Check fqn_matches = fqn_pattern.findall(code_line) if fqn_matches: @@ -166,17 +258,43 @@ def check_diff_anti_patterns(): if mutable_collection_pattern.search(code_line): log_warning(f"[{current_file}] Found mutable collection instantiation (ArrayList/HashMap/HashSet). Prefer Guava Immutable collections.") + # Unnecessary casts + if unnecessary_cast_pattern.search(code_line): + log_warning(f"[{current_file}] Potential unnecessary cast to Instant or Guava Immutable type. Remove if it compiles without it.") + + # Instant toString + if instant_tostring_pattern.search(code_line): + log_error(f"[{current_file}] Found potential Instant.toString(). Use DateTimeUtils.formatInstant(...) to preserve .000Z precision.") + + # DAO transactions + if current_file.lower().endswith('dao.java') and dao_transact_pattern.search(code_line): + log_error(f"[{current_file}] Found tm().transact(...) inside a DAO. Use tm().assertInTransaction() instead.") + + # reTransact around getTxTime in production code + if 'src/main/' in current_file and retransact_txtime_pattern.search(code_line): + log_error(f"[{current_file}] Unnecessary reTransact() around getTxTime() in production code. Wrap the caller in a transaction instead.") + + # inject(Command) + if inject_command_pattern.search(code_line): + log_error(f"[{current_file}] Generic inject(Command) methods do not work with Dagger. Use explicit concrete types.") + + # clock.now() in tests + if 'src/test/' in current_file and clock_now_pattern.search(code_line): + log_warning(f"[{current_file}] Prefer using a fixed, static constant Instant over capturing clock.now() in tests to prevent flakiness.") + def main(): print("========================================") print(" NOMULUS PR POLISHER CHECKLIST ") print("========================================\n") - + + check_single_commit() check_commit_message() check_workspace_clean() check_package_lock() check_license_headers() + check_formatting() check_diff_anti_patterns() - + print("\n========================================") if errors_found == 0 and warnings_found == 0: print(f"{GREEN}SUCCESS: All checks passed. PR is polished!{RESET}") @@ -186,4 +304,4 @@ def main(): sys.exit(1 if errors_found > 0 else 0) if __name__ == "__main__": - main() \ No newline at end of file + main() diff --git a/GEMINI.md b/GEMINI.md index fc9702e11..1608fad20 100644 --- a/GEMINI.md +++ b/GEMINI.md @@ -92,6 +92,7 @@ This document captures high-level architectural patterns, lessons learned from l - Core boundaries: `DateTimeUtils.START_INSTANT` (Unix Epoch) and `DateTimeUtils.END_INSTANT` (Long.MAX_VALUE / 1000). ## Source Control +- **No Unprompted Pushing:** You MUST NEVER push your changes to a remote repository (e.g., `git push origin ...`) unless the user VERY EXPLICITLY instructs you to do so. If you have finished a task, simply state that it is committed locally and ready for review. Do not assume pushing is desired. - **Committing:** Always create a new commit on the branch if one hasn't been created yet for the branch's specific work. Only perform amending (`git commit --amend --no-edit`) for subsequent changes once the initial commit has been successfully created. - **One Commit Per PR:** All changes for a single PR must be squashed into a single commit before merging. - **Default to Amend:** Once an initial commit is created for a PR, all subsequent functional changes should be amended into that same commit by default (`git commit --amend --no-edit`). This ensures the PR remains a single, clean unit of work throughout the development lifecycle. @@ -104,8 +105,10 @@ This document captures high-level architectural patterns, lessons learned from l 5. **Only after** step 4 has successfully returned a clean working directory may you generate a text response to the user declaring that the task is complete. - **Diff Review:** Before finalizing a task, review the full diff (e.g., `git diff HEAD^`) to ensure all changes are functional and relevant. Identify and revert any formatting-only changes in files that do not contain functional updates to keep the commit focused. -## Self-Review Guidelines -Before finalizing any PR or declaring a task complete, you MUST perform a thorough, rigorous self-review of your entire diff. Run `git diff HEAD^` (or review the staged changes) and actively verify the following against every modified line: +## Self-Review Guidelines & PR Polisher +Before finalizing any PR or declaring a task complete, you MUST automatically execute the `pr-polisher` skill to perform a thorough, rigorous self-review of your entire diff. + +To run it, activate the skill and follow its exact workflow. You MUST NOT declare a task done until the automated script (`python3 .gemini/skills/pr-polisher/scripts/check_diff.py`) returns 0 errors, the presubmits and tests pass, and the workspace is fully clean. 1. **Imports & FQNs:** Did I leave any fully-qualified class names or static variables inline? Did I add the necessary imports for them? *Crucial Exception:* If the file already imports a class with the identical name (e.g., it uses both `java.util.Date` and `java.sql.Date`), one MUST remain fully qualified to avoid a compilation conflict. 2. **Diff Scope:** Are there any formatting-only changes in files that I did not functionally modify? If so, revert them. Does the total line count of the diff align with the approved scope (e.g., ~1,000 lines for migrations)? @@ -113,7 +116,7 @@ Before finalizing any PR or declaring a task complete, you MUST perform a thorou 4. **Missing Tests & Coverage:** *Perform a structured check for any new methods or modified behavior.* Did I add a new utility method (like `plusMonths(Instant, int)`) or change core logic? If so, I MUST open the corresponding test file and write tests to cover the new functionality (including edge cases, negative values, and leap years) before considering the task complete. A code review is not thorough if it only checks for compilation. I must actively ensure every new branch of logic has a test. 5. **Package Lock:** Did I include `console-webapp/package-lock.json` in my diff? If so, I MUST revert it (`git checkout console-webapp/package-lock.json`) unless I explicitly intended to modify NPM dependencies. This file is often modified by the build process and should not be committed accidentally. -Only after actively confirming these checks against your diff are you permitted to finalize the task. +Only after actively executing the `pr-polisher` skill and confirming these checks against your diff are you permitted to finalize the task. ## Refactoring & Migration Guardrails