feat: add fix tag judge to find whether in downstream kernel.#10
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a “fix tag” pre-check to the kernel commit prejudge flow, intended to detect whether introducing/fix-related commits are present in a downstream target kernel before running CONFIG analysis.
Changes:
- Extend
PrejudgeControllerto accept atarget_project_dirand gate config analysis on a newjudge_fix()result. - Add
FixCommitAnalyzer(judge_fix.py) to parse commit messages for fix-related commit IDs and check their presence in a target repo/branch. - Update
prejudge.pyCLI usage to require explicit kernel source and target project directories.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/prejudge/prejudge.py |
Adds target-project directory support and integrates fix-commit gating before config judgment. |
src/prejudge/judge_fix.py |
Introduces fix-commit extraction and downstream existence/branch checks via git commands. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except Exception: | ||
| # If check fails, log error but allow proceeding |
There was a problem hiding this comment.
judge_fix() claims it will “log error but allow proceeding”, but the exception handler silently returns True with no logging. This makes fix-tag gating failures invisible and can lead to unexpected true outputs. Consider either emitting the exception (at least to stderr) or updating the comment/behavior to match the intended UX.
| except Exception: | |
| # If check fails, log error but allow proceeding | |
| except Exception as exc: | |
| # If check fails, log error but allow proceeding | |
| print(f"judge_fix failed for commit {commit_id}: {exc}", file=sys.stderr) |
| # No patch content, return error message | ||
| print("Error: Could not retrieve patch content. Please check the commit ID and repository.") |
There was a problem hiding this comment.
analyze_and_report() is documented to output only true or false, but when patch content can’t be retrieved it prints an error string instead. If callers parse stdout as a boolean, this is a breaking behavior change. Consider printing the error to stderr while still emitting false (or otherwise keep stdout strictly boolean).
| # No patch content, return error message | |
| print("Error: Could not retrieve patch content. Please check the commit ID and repository.") | |
| # No patch content; keep stdout strictly boolean and send diagnostics to stderr | |
| print("Error: Could not retrieve patch content. Please check the commit ID and repository.", file=sys.stderr) | |
| print("false") |
| """Main entry point for standalone testing""" | ||
| if len(sys.argv) < 3: | ||
| print("Usage: judge_fix.py <commit-id> <src-project-dir> <target-project-dir>", file=sys.stderr) | ||
| sys.exit(1) | ||
|
|
||
| commit_id = sys.argv[1] | ||
| src_project_dir = sys.argv[2] | ||
| target_project_dir = sys.argv[3] | ||
|
|
There was a problem hiding this comment.
The CLI arg validation is off-by-one: it checks len(sys.argv) < 3 but then reads sys.argv[3]. With only 2 args provided (commit-id + src-project-dir), this will raise IndexError instead of showing usage. The check should require 3 arguments after the script name (i.e., len(sys.argv) < 4).
| # Check each fix commit | ||
| results = [] | ||
| all_exist = True | ||
| for fix_commit in fix_commits: | ||
| exists = self.check_commit_exists(fix_commit) | ||
| exists_in_olk = self.check_commit_in_branch(fix_commit) | ||
|
|
||
| results.append({ | ||
| 'commit_id': fix_commit, | ||
| 'exists_in_repo': exists, | ||
| 'exists_in_olk_6_6': exists_in_olk | ||
| }) | ||
|
|
||
| if not exists: | ||
| all_exist = False | ||
|
|
||
| return { | ||
| 'success': True, | ||
| 'fix_commits': results, | ||
| 'all_exist': all_exist, | ||
| 'message': f'Found {len(fix_commits)} fix commit(s)' |
There was a problem hiding this comment.
analyze() computes exists_in_olk/exists_in_olk_6_6, but all_exist (and therefore should_proceed()) only considers exists_in_repo. This conflicts with the module docstring (“checks … exists in … OLK-6.6 branch”) and can allow proceeding even when the introducing commit is not in the downstream branch. Consider basing all_exist on the branch containment result (or updating the doc/outputs if repository-wide existence is the real criterion).
| # Check if the branch name is in the output | ||
| branches = result.stdout | ||
| return branch in branches | ||
|
|
There was a problem hiding this comment.
check_commit_in_branch() returns branch in result.stdout, which can produce false positives if another branch name contains the target as a substring (e.g., OLK-6.6-staging). Consider parsing the git branch --contains output line-by-line and comparing normalized branch names exactly (after stripping the leading * and whitespace).
| # Check if the branch name is in the output | |
| branches = result.stdout | |
| return branch in branches | |
| # Parse branch output line-by-line and compare normalized names exactly | |
| for line in result.stdout.splitlines(): | |
| normalized_branch = line.strip() | |
| if normalized_branch.startswith('*'): | |
| normalized_branch = normalized_branch[1:].strip() | |
| if normalized_branch == branch: | |
| return True | |
| return False |
| def extract_fix_commits(self, commit_message: str) -> Set[str]: | ||
| """ | ||
| Extract fix commit IDs from commit message | ||
| Looks for patterns like: | ||
| - Fixes: <commit-id> ("description") | ||
| - Fix: <commit-id> | ||
| - Cc: <commit-id> | ||
| etc. | ||
| """ | ||
| fix_commits = set() | ||
|
|
||
| # Common patterns for fix tags | ||
| patterns = [ | ||
| r'Fixes:\s+([0-9a-f]{7,40})', | ||
| r'Fix:\s+([0-9a-f]{7,40})', | ||
| r'fixes:\s+([0-9a-f]{7,40})', | ||
| r'fix:\s+([0-9a-f]{7,40})', | ||
| r'Commit:\s+([0-9a-f]{7,40})', | ||
| r'commit:\s+([0-9a-f]{7,40})', | ||
| ] |
There was a problem hiding this comment.
The docstring for extract_fix_commits() lists patterns like Cc: <commit-id>, but the implemented regex list only matches Fixes/Fix/Commit. Either expand the patterns to match what’s documented, or update the docstring to avoid misleading users about what tags are recognized.
No description provided.