Track 15 Micro-PRs#147
Conversation
- Add SAMOT5Summarizer class with SAMO optimizations - Add SAMO-specific configuration (configs/samo_t5_config.yaml) - Optimize parameters for journal entry summarization - Add emotional keyword extraction - Include comprehensive error handling and validation - Add standalone test script for validation Files changed: 3 Lines changed: ~200 Scope: T5 model implementation only This is a focused micro-PR that adds only the T5 summarization model without any API integration or other features.
- Add comprehensive master plan for breaking down PR #145 - Add surgical breakdown executor script for automation - Add prevention strategy to avoid future monster PRs - Include automated PR size guards and validation - Add monitoring and metrics for PR health This is the 16th overarching PR that tracks all 15 micro-PRs and provides comprehensive prevention measures.
Reviewer's GuideThis PR introduces a surgical breakdown and tracking system to split a previously unmanageable monolithic PR into 15 focused micro-PRs, adds comprehensive PR prevention and enforcement tools (size guards, branch naming rules, pre-commit hooks, GitHub Actions), delivers the first micro-PR implementation of a SAMO-optimized T5 summarization model with its configuration and standalone tests, and provides automation scripts to create, validate, and monitor those micro-PRs. Class diagram for SurgicalBreakdownExecutor automation scriptclassDiagram
class SurgicalBreakdownExecutor {
- project_root: Path
- master_plan_file: Path
- progress_file: Path
- progress: Dict
+ __init__()
+ load_progress()
+ save_progress()
+ create_pr(pr_number: int, description: str, files: List[str], estimated_lines: int, phase: int) bool
+ validate_pr(branch_name: str) Dict[str, any]
+ status()
+ next_pr()
+ get_pr_info(pr_number: int) Dict
}
Flow diagram for surgical breakdown execution and trackingflowchart TD
A["Start Surgical Breakdown"] --> B["Create micro-PR branch"]
B --> C["Add relevant files"]
C --> D["Commit with single-purpose message"]
D --> E["Run pre-commit checks"]
E --> F["Push branch to remote"]
F --> G["GitHub Actions validate PR size and rules"]
G --> H["Update SURGICAL_BREAKDOWN_MASTER_PLAN.md"]
H --> I["Update .surgical_breakdown_progress.json"]
I --> J["Monitor progress via dashboard"]
J --> K["Repeat for next micro-PR"]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Warning Rate limit exceeded@uelkerd has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds two new documentation plans for PR size prevention and surgical breakdown, a T5 summarization config, a Python CLI to orchestrate micro-PRs, a SAMO-optimized T5 summarizer module, and a standalone test harness validating loading, inference, batching, and input checks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as SurgicalBreakdownExecutor (CLI)
participant Git as git
participant Store as .surgical_breakdown_progress.json
User->>CLI: create-pr --pr-number N --files ...
CLI->>Store: load_progress()
CLI->>Git: checkout -b feat/dl-pr-N
CLI->>Git: add <files>\ncommit -m "PR-N ..."
CLI->>Store: save_progress({status: created})
CLI-->>User: result (success/failure)
User->>CLI: validate-pr --branch feat/dl-pr-N
CLI->>Git: diff --name-only main..branch
CLI->>Git: diff --numstat main..branch
CLI->>Git: rev-list --count main..branch
CLI-->>User: {file_count, lines_changed, commit_count, overall_ok}
User->>CLI: status / next-pr
CLI->>Store: load_progress()
CLI-->>User: overview or recommendation
sequenceDiagram
autonumber
participant App
participant Summ as SAMOT5Summarizer
participant HF as T5 Model/Tokenizer
participant YAML as Config (YAML)
App->>Summ: __init__(config_path)
Summ->>YAML: load/merge config
Summ->>HF: load tokenizer & model (device: cuda/mps/cpu)
Summ-->>App: ready
App->>Summ: generate_summary(text)
Summ->>Summ: _validate_input()
alt valid
Summ->>Summ: _extract_emotional_keywords()
Summ->>HF: tokenize("summarize: ...")
Summ->>HF: model.generate(params)
HF-->>Summ: ids
Summ-->>App: {summary, metrics, emotional_keywords, success:true}
else invalid
Summ-->>App: {error, success:false}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches
🧪 Generate unit tests
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 |
|
Here's the code health analysis summary for commits Analysis Summary
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @uelkerd, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request serves as a meta-PR to manage the surgical breakdown of an excessively large and unmanageable pull request (#145) into 15 smaller, focused micro-PRs. Its primary purpose is to establish and enforce new organizational and technical guidelines for pull request creation, ensuring future changes are atomic, reviewable, and maintainable. This initiative aims to significantly improve code quality, accelerate review cycles, and prevent the recurrence of "monster PRs" by integrating automated checks and revised development workflows.
Highlights
- PR Breakdown Strategy: Implemented a comprehensive plan to break down the massive PR #145 into 15 smaller, focused micro-PRs to improve manageability and reviewability.
- New PR Prevention Measures: Introduced automated size guards (files < 25, lines < 500, commits < 5), pre-commit hooks, and branch naming enforcement to prevent future large PRs.
- Enhanced Development Workflow: Defined new daily PR planning templates and micro-PR templates to guide developers in creating small, single-purpose pull requests.
- Technical Enforcement: Added .pre-commit-config.yaml and GitHub Actions workflows (pr-guardian.yml, pr-validation.yml) to enforce PR rules and automate testing, coverage checks, and security scans.
- PR Health Monitoring: Established a framework for monitoring PR health with a dashboard and weekly reports, tracking metrics like average files/lines/commits per PR and rule violations.
- T5 Summarization Model (PR-1): Completed the first micro-PR, which adds a SAMO-optimized T5 text summarization model, its configuration, and standalone tests.
- Surgical Breakdown Executor Script: Introduced a Python script (scripts/surgical_breakdown_executor.py) to assist in managing the micro-PR creation, validation, and tracking process.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR implements a surgical breakdown strategy to address the critical issue of PR #145 becoming unmanageable with 53,340 lines, 399 files, and 103 commits. The solution creates a tracking system for breaking the massive PR into 15 focused micro-PRs, implements the first micro-PR (T5 summarization model), and establishes prevention measures to avoid similar issues in the future.
Key changes include:
- Implementation of T5 summarization model as the first focused micro-PR
- Automated surgical breakdown executor for managing the 15-PR sequence
- Comprehensive prevention strategy with automated guards and monitoring
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test_samo_t5_standalone.py | Standalone test script for T5 summarization model validation |
| src/models/summarization/samo_t5_summarizer.py | Core T5 summarization model implementation with SAMO optimizations |
| scripts/surgical_breakdown_executor.py | Automation tool for managing the surgical breakdown process |
| configs/samo_t5_config.yaml | Configuration file for T5 model parameters and SAMO optimizations |
| SURGICAL_BREAKDOWN_MASTER_PLAN.md | Master tracking document for the 15-PR breakdown strategy |
| PR_PREVENTION_STRATEGY.md | Comprehensive prevention measures and automated guards |
Comments suppressed due to low confidence (1)
scripts/surgical_breakdown_executor.py:1
- Hardcoded placeholder token in example code could be accidentally committed with real credentials. This should use environment variables or configuration files for token management.
#!/usr/bin/env python3
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if summary.startswith("summarize:"): | ||
| summary = summary[10:].strip() |
There was a problem hiding this comment.
The prefix removal logic assumes the exact prefix "summarize:" but the tokenizer may add spaces or formatting. This could fail to remove the prefix correctly in some cases, leading to malformed summaries.
| ["git", "diff", "--numstat", "main", branch_name], | ||
| capture_output=True, text=True, check=True | ||
| ) | ||
| lines_changed = sum(int(line.split('\t')[0]) for line in result.stdout.strip().split('\n') if line) |
There was a problem hiding this comment.
This line assumes the first column in git diff --numstat output is always a valid integer, but it can be '-' for binary files, which will cause a ValueError when passed to int().
| print(f" ⚠️ Warning: File {file_path} not found") | ||
|
|
||
| # Commit | ||
| commit_message = f"feat: {description.lower()}\n\n- PR-{pr_number} of surgical breakdown\n- Phase {phase}\n- Files: {len(files)}\n- Estimated lines: {estimated_lines}\n\nThis is a focused micro-PR that addresses exactly one concern\nas part of the surgical breakdown of PR #145." |
There was a problem hiding this comment.
The commit message is hardcoded with a very long multi-line string. This should be extracted to a template or configuration to improve maintainability and allow customization.
| "", # Empty text | ||
| "Short", # Too short | ||
| "A" * 2000, # Too long | ||
| 123 # Wrong type |
There was a problem hiding this comment.
[nitpick] Using a magic number (123) without explanation in test data makes the test less readable. Consider using a more descriptive constant or adding a comment explaining why this specific value is used for testing type validation.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/models/summarization/samo_t5_summarizer.py:87` </location>
<code_context>
+ }
+ }
+
+ if config_path and os.path.exists(config_path):
+ try:
+ with open(config_path, 'r', encoding='utf-8') as f:
+ config = yaml.safe_load(f)
+ # Merge with defaults
+ for key, value in default_config.items():
+ if key not in config:
+ config[key] = value
</code_context>
<issue_to_address>
Config merging may not handle nested dictionaries as expected.
The merge only adds missing top-level keys, so nested defaults may be lost if the config partially overrides them. A recursive merge would preserve all default values.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if config_path and os.path.exists(config_path):
try:
with open(config_path, 'r', encoding='utf-8') as f:
config = yaml.safe_load(f)
# Merge with defaults
for key, value in default_config.items():
if key not in config:
config[key] = value
return config
except Exception as e:
logger.warning("Failed to load config from %s: %s", config_path, e)
return default_config
=======
def recursive_merge_dicts(default, override):
"""Recursively merge two dictionaries, with values from override taking precedence."""
result = default.copy()
for key, value in override.items():
if (
key in result
and isinstance(result[key], dict)
and isinstance(value, dict)
):
result[key] = recursive_merge_dicts(result[key], value)
else:
result[key] = value
return result
if config_path and os.path.exists(config_path):
try:
with open(config_path, 'r', encoding='utf-8') as f:
config = yaml.safe_load(f)
# Recursively merge with defaults
merged_config = recursive_merge_dicts(default_config, config or {})
return merged_config
except Exception as e:
logger.warning("Failed to load config from %s: %s", config_path, e)
return default_config
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `src/models/summarization/samo_t5_summarizer.py:103` </location>
<code_context>
+
+ def _get_device(self) -> str:
+ """Get the best available device."""
+ if torch.cuda.is_available():
+ return "cuda"
+ elif hasattr(torch.backends, 'mps') and torch.backends.mps.is_available():
+ return "mps"
+ else:
</code_context>
<issue_to_address>
Device selection logic may not work as intended on some platforms.
Use 'torch.backends.mps.is_available()' directly to check for MPS support, as 'torch.backends.mps' may exist even when MPS is unavailable. Ensure this logic works reliably on all platforms.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if torch.cuda.is_available():
return "cuda"
elif hasattr(torch.backends, 'mps') and torch.backends.mps.is_available():
return "mps"
else:
return "cpu"
=======
if torch.cuda.is_available():
return "cuda"
elif torch.backends.mps.is_available():
return "mps"
else:
return "cpu"
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `src/models/summarization/samo_t5_summarizer.py:208` </location>
<code_context>
+ input_text = f"summarize: {text}"
+
+ # Tokenize
+ inputs = self.tokenizer(
+ input_text,
+ return_tensors="pt",
+ max_length=512,
+ truncation=True,
+ padding=True
+ ).to(self.device)
+
</code_context>
<issue_to_address>
Tokenizer padding argument may not behave as expected.
Since only a single input is provided, 'padding=True' may not pad as expected. For consistent input shapes, use 'padding="max_length"' and set 'max_length' explicitly if batching is planned.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
inputs = self.tokenizer(
input_text,
return_tensors="pt",
max_length=512,
truncation=True,
padding=True
).to(self.device)
=======
inputs = self.tokenizer(
input_text,
return_tensors="pt",
max_length=512,
truncation=True,
padding="max_length"
).to(self.device)
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `src/models/summarization/samo_t5_summarizer.py:231` </location>
<code_context>
+ )
+
+ # Decode output
+ summary = self.tokenizer.decode(outputs[0], skip_special_tokens=True)
+
+ # Remove "summarize:" prefix if present
</code_context>
<issue_to_address>
Decoding may not handle multiple outputs in batch mode.
Currently, only the first output is decoded. Please update the method to process all outputs in batch mode, or document that it only supports single-input usage.
Suggested implementation:
```python
# Decode all outputs in batch mode
summaries = [
self.tokenizer.decode(output, skip_special_tokens=True)
for output in outputs
]
# Remove "summarize:" prefix if present in each summary
summaries = [
summary[10:].strip() if summary.startswith("summarize:") else summary
for summary in summaries
]
# Calculate metrics for each summary
if isinstance(text, list):
original_lengths = [len(t.split()) for t in text]
else:
original_lengths = [len(text.split())] * len(summaries)
summary_lengths = [len(s.split()) for s in summaries]
compression_ratios = [
sl / ol if ol > 0 else 0
for sl, ol in zip(summary_lengths, original_lengths)
]
processing_time = time.time() - start_time
return {
"summaries": summaries,
"original_lengths": original_lengths,
"summary_lengths": summary_lengths,
"compression_ratios": compression_ratios,
"processing_time": processing_time,
}
```
- If your method is only intended to support single-input usage, you should add a docstring or comment to clarify this and revert to decoding only the first output.
- If you have downstream code expecting a single summary, you may need to update it to handle a list of summaries and associated metrics.
- If you want to keep backward compatibility, you could return a single summary if only one input is provided.
</issue_to_address>
### Comment 5
<location> `src/models/summarization/samo_t5_summarizer.py:234` </location>
<code_context>
+ summary = self.tokenizer.decode(outputs[0], skip_special_tokens=True)
+
+ # Remove "summarize:" prefix if present
+ if summary.startswith("summarize:"):
+ summary = summary[10:].strip()
+
</code_context>
<issue_to_address>
Removing 'summarize:' prefix from output may be unnecessary.
This check appears redundant unless there are known cases where the prefix is generated. If so, please document the reason for its inclusion.
</issue_to_address>
### Comment 6
<location> `src/models/summarization/samo_t5_summarizer.py:263` </location>
<code_context>
+ "processing_time": time.time() - start_time
+ }
+
+ def generate_batch_summaries(self, texts: List[str]) -> List[Dict[str, Any]]:
+ """
+ Generate summaries for multiple texts.
+
+ Args:
+ texts: List of input texts
+
+ Returns:
+ List of summary dictionaries
+ """
+ results = []
+
+ for text in texts:
+ result = self.generate_summary(text)
+ results.append(result)
</code_context>
<issue_to_address>
Batch summary generation does not leverage model batching.
Batching the input texts and passing them to the model together can significantly improve throughput and reduce processing time.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def generate_batch_summaries(self, texts: List[str]) -> List[Dict[str, Any]]:
"""
Generate summaries for multiple texts.
Args:
texts: List of input texts
Returns:
List of summary dictionaries
"""
results = []
for text in texts:
result = self.generate_summary(text)
results.append(result)
return results
=======
def generate_batch_summaries(self, texts: List[str]) -> List[Dict[str, Any]]:
"""
Generate summaries for multiple texts using model batching.
Args:
texts: List of input texts
Returns:
List of summary dictionaries
"""
start_time = time.time()
results = []
try:
# Tokenize all texts together
inputs = self.tokenizer(
texts,
max_length=self.config["model"]["max_input_length"],
truncation=True,
padding=True,
return_tensors="pt"
).to(self.device)
# Generate summaries in batch
summary_ids = self.model.generate(
input_ids=inputs.input_ids,
attention_mask=inputs.attention_mask,
max_length=self.config["model"]["max_output_length"],
num_beams=self.config["model"].get("num_beams", 4),
early_stopping=True
)
# Decode summaries
summaries = self.tokenizer.batch_decode(
summary_ids,
skip_special_tokens=True,
clean_up_tokenization_spaces=True
)
# Build result dicts
for summary in summaries:
results.append({
"summary": summary,
"error": None,
"success": True,
"processing_time": time.time() - start_time
})
except Exception as e:
logger.error("Batch summarization failed: %s", e)
for _ in texts:
results.append({
"summary": "",
"error": str(e),
"success": False,
"processing_time": time.time() - start_time
})
return results
>>>>>>> REPLACE
</suggested_fix>
### Comment 7
<location> `scripts/surgical_breakdown_executor.py:28` </location>
<code_context>
+ """Executes the surgical breakdown plan for PR #145."""
+
+ def __init__(self):
+ self.project_root = Path(__file__).parent.parent
+ self.master_plan_file = self.project_root / "SURGICAL_BREAKDOWN_MASTER_PLAN.md"
+ self.progress_file = self.project_root / ".surgical_breakdown_progress.json"
</code_context>
<issue_to_address>
Project root calculation may fail in some execution contexts.
This approach may not work if the script is run from a different directory or as a module. Please use a method that reliably locates the project root in all contexts.
Suggested implementation:
```python
def __init__(self):
self.project_root = self.find_project_root()
self.master_plan_file = self.project_root / "SURGICAL_BREAKDOWN_MASTER_PLAN.md"
self.progress_file = self.project_root / ".surgical_breakdown_progress.json"
self.load_progress()
```
```python
class SurgicalBreakdownExecutor:
"""Executes the surgical breakdown plan for PR #145."""
def find_project_root(self) -> Path:
"""
Reliably locate the project root by searching upwards for a known marker file.
Adjust the marker file as needed for your project (e.g., '.git', 'pyproject.toml').
"""
current = Path(__file__).resolve().parent
while current != current.parent:
if (current / ".git").exists() or (current / "SURGICAL_BREAKDOWN_MASTER_PLAN.md").exists():
return current
current = current.parent
raise RuntimeError("Project root not found. Please ensure a marker file exists.")
def __init__(self):
self.project_root = self.find_project_root()
self.master_plan_file = self.project_root / "SURGICAL_BREAKDOWN_MASTER_PLAN.md"
self.progress_file = self.project_root / ".surgical_breakdown_progress.json"
self.load_progress()
def load_progress(self):
"""Load progress tracking data."""
if self.progress_file.exists():
with open(self.progress_file, 'r') as f:
self.progress = json.load(f)
else:
self.progress = {
"created_at": datetime.now().isoformat(),
```
- If your project uses a different marker file to identify the root (e.g., `pyproject.toml`, `setup.py`), update the `find_project_root` method accordingly.
- If you want to support more marker files, add them to the condition in the `while` loop.
</issue_to_address>
### Comment 8
<location> `scripts/surgical_breakdown_executor.py:63` </location>
<code_context>
+
+ try:
+ # Create branch
+ subprocess.run(["git", "checkout", "-b", branch_name], check=True)
+
+ # Add files
</code_context>
<issue_to_address>
Branch creation does not handle existing branches gracefully.
Check if the branch exists before attempting creation, and handle the error to improve user feedback.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
try:
# Create branch
subprocess.run(["git", "checkout", "-b", branch_name], check=True)
# Add files
=======
try:
# Check if branch exists
result = subprocess.run(
["git", "branch", "--list", branch_name],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
text=True
)
if result.stdout.strip():
print(f" ℹ️ Branch '{branch_name}' already exists. Checking out existing branch.")
subprocess.run(["git", "checkout", branch_name], check=True)
else:
print(f" 🏗️ Creating new branch '{branch_name}'.")
subprocess.run(["git", "checkout", "-b", branch_name], check=True)
# Add files
>>>>>>> REPLACE
</suggested_fix>
### Comment 9
<location> `scripts/surgical_breakdown_executor.py:163` </location>
<code_context>
+ print(f"Status: {completed} completed, {in_progress} in progress, {created} created")
+ print()
+
+ for pr_num in range(1, total_prs + 1):
+ pr_key = str(pr_num)
+ if pr_key in self.progress["prs"]:
+ pr = self.progress["prs"][pr_key]
+ status_icon = {"completed": "✅", "in_progress": "🚧", "created": "📝"}.get(pr["status"], "⏳")
+ print(f"PR-{pr_num:02d}: {status_icon} {pr['description']}")
+ print(f" Branch: {pr['branch']}")
+ print(f" Files: {len(pr['files'])}")
+ print(f" Phase: {pr['phase']}")
+ print()
+
</code_context>
<issue_to_address>
Status output does not show PRs that have not been created.
Consider updating the status report to list all PRs (1-15), marking those not yet created, for a more complete overview.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
for pr_num in range(1, total_prs + 1):
pr_key = str(pr_num)
if pr_key in self.progress["prs"]:
pr = self.progress["prs"][pr_key]
status_icon = {"completed": "✅", "in_progress": "🚧", "created": "📝"}.get(pr["status"], "⏳")
print(f"PR-{pr_num:02d}: {status_icon} {pr['description']}")
print(f" Branch: {pr['branch']}")
print(f" Files: {len(pr['files'])}")
print(f" Phase: {pr['phase']}")
print()
=======
for pr_num in range(1, total_prs + 1):
pr_key = str(pr_num)
if pr_key in self.progress["prs"]:
pr = self.progress["prs"][pr_key]
status_icon = {"completed": "✅", "in_progress": "🚧", "created": "📝"}.get(pr["status"], "⏳")
print(f"PR-{pr_num:02d}: {status_icon} {pr['description']}")
print(f" Branch: {pr['branch']}")
print(f" Files: {len(pr['files'])}")
print(f" Phase: {pr['phase']}")
print()
else:
# PR not yet created
print(f"PR-{pr_num:02d}: ❌ Not created")
print(f" Branch: -")
print(f" Files: -")
print(f" Phase: -")
print()
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if config_path and os.path.exists(config_path): | ||
| try: | ||
| with open(config_path, 'r', encoding='utf-8') as f: | ||
| config = yaml.safe_load(f) | ||
| # Merge with defaults | ||
| for key, value in default_config.items(): | ||
| if key not in config: | ||
| config[key] = value | ||
| return config | ||
| except Exception as e: | ||
| logger.warning("Failed to load config from %s: %s", config_path, e) | ||
|
|
||
| return default_config |
There was a problem hiding this comment.
suggestion: Config merging may not handle nested dictionaries as expected.
The merge only adds missing top-level keys, so nested defaults may be lost if the config partially overrides them. A recursive merge would preserve all default values.
| if config_path and os.path.exists(config_path): | |
| try: | |
| with open(config_path, 'r', encoding='utf-8') as f: | |
| config = yaml.safe_load(f) | |
| # Merge with defaults | |
| for key, value in default_config.items(): | |
| if key not in config: | |
| config[key] = value | |
| return config | |
| except Exception as e: | |
| logger.warning("Failed to load config from %s: %s", config_path, e) | |
| return default_config | |
| def recursive_merge_dicts(default, override): | |
| """Recursively merge two dictionaries, with values from override taking precedence.""" | |
| result = default.copy() | |
| for key, value in override.items(): | |
| if ( | |
| key in result | |
| and isinstance(result[key], dict) | |
| and isinstance(value, dict) | |
| ): | |
| result[key] = recursive_merge_dicts(result[key], value) | |
| else: | |
| result[key] = value | |
| return result | |
| if config_path and os.path.exists(config_path): | |
| try: | |
| with open(config_path, 'r', encoding='utf-8') as f: | |
| config = yaml.safe_load(f) | |
| # Recursively merge with defaults | |
| merged_config = recursive_merge_dicts(default_config, config or {}) | |
| return merged_config | |
| except Exception as e: | |
| logger.warning("Failed to load config from %s: %s", config_path, e) | |
| return default_config |
| if torch.cuda.is_available(): | ||
| return "cuda" | ||
| elif hasattr(torch.backends, 'mps') and torch.backends.mps.is_available(): | ||
| return "mps" | ||
| else: | ||
| return "cpu" |
There was a problem hiding this comment.
suggestion (bug_risk): Device selection logic may not work as intended on some platforms.
Use 'torch.backends.mps.is_available()' directly to check for MPS support, as 'torch.backends.mps' may exist even when MPS is unavailable. Ensure this logic works reliably on all platforms.
| if torch.cuda.is_available(): | |
| return "cuda" | |
| elif hasattr(torch.backends, 'mps') and torch.backends.mps.is_available(): | |
| return "mps" | |
| else: | |
| return "cpu" | |
| if torch.cuda.is_available(): | |
| return "cuda" | |
| elif torch.backends.mps.is_available(): | |
| return "mps" | |
| else: | |
| return "cpu" |
| inputs = self.tokenizer( | ||
| input_text, | ||
| return_tensors="pt", | ||
| max_length=512, | ||
| truncation=True, | ||
| padding=True | ||
| ).to(self.device) |
There was a problem hiding this comment.
suggestion: Tokenizer padding argument may not behave as expected.
Since only a single input is provided, 'padding=True' may not pad as expected. For consistent input shapes, use 'padding="max_length"' and set 'max_length' explicitly if batching is planned.
| inputs = self.tokenizer( | |
| input_text, | |
| return_tensors="pt", | |
| max_length=512, | |
| truncation=True, | |
| padding=True | |
| ).to(self.device) | |
| inputs = self.tokenizer( | |
| input_text, | |
| return_tensors="pt", | |
| max_length=512, | |
| truncation=True, | |
| padding="max_length" | |
| ).to(self.device) |
| ) | ||
|
|
||
| # Decode output | ||
| summary = self.tokenizer.decode(outputs[0], skip_special_tokens=True) |
There was a problem hiding this comment.
suggestion: Decoding may not handle multiple outputs in batch mode.
Currently, only the first output is decoded. Please update the method to process all outputs in batch mode, or document that it only supports single-input usage.
Suggested implementation:
# Decode all outputs in batch mode
summaries = [
self.tokenizer.decode(output, skip_special_tokens=True)
for output in outputs
]
# Remove "summarize:" prefix if present in each summary
summaries = [
summary[10:].strip() if summary.startswith("summarize:") else summary
for summary in summaries
]
# Calculate metrics for each summary
if isinstance(text, list):
original_lengths = [len(t.split()) for t in text]
else:
original_lengths = [len(text.split())] * len(summaries)
summary_lengths = [len(s.split()) for s in summaries]
compression_ratios = [
sl / ol if ol > 0 else 0
for sl, ol in zip(summary_lengths, original_lengths)
]
processing_time = time.time() - start_time
return {
"summaries": summaries,
"original_lengths": original_lengths,
"summary_lengths": summary_lengths,
"compression_ratios": compression_ratios,
"processing_time": processing_time,
}- If your method is only intended to support single-input usage, you should add a docstring or comment to clarify this and revert to decoding only the first output.
- If you have downstream code expecting a single summary, you may need to update it to handle a list of summaries and associated metrics.
- If you want to keep backward compatibility, you could return a single summary if only one input is provided.
| summary = self.tokenizer.decode(outputs[0], skip_special_tokens=True) | ||
|
|
||
| # Remove "summarize:" prefix if present | ||
| if summary.startswith("summarize:"): |
There was a problem hiding this comment.
nitpick: Removing 'summarize:' prefix from output may be unnecessary.
This check appears redundant unless there are known cases where the prefix is generated. If so, please document the reason for its inclusion.
| pr_info = executor.get_pr_info(args.pr_number) | ||
| description = args.description or pr_info["description"] | ||
| files = args.files or pr_info["files"] | ||
| lines = args.lines or pr_info["estimated_lines"] | ||
| phase = args.phase or pr_info["phase"] | ||
|
|
||
| executor.create_pr(args.pr_number, description, files, lines, phase) |
There was a problem hiding this comment.
issue (code-quality): Extract code out into function (extract-method)
|
|
||
| def _load_model(self) -> None: | ||
| """Load the T5 model and tokenizer.""" | ||
| try: |
There was a problem hiding this comment.
issue (code-quality): Explicitly raise from a previous error (raise-from-previous-error)
| max_words = self.config["validation"]["max_words"] | ||
|
|
||
| if word_count < min_words: | ||
| return False, f"Text too short (minimum {min_words} words)" | ||
|
|
||
| if word_count > max_words: | ||
| return False, f"Text too long (maximum {max_words} words)" |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Move assignments closer to their usage (
move-assign) - Lift code into else after jump in control flow [×2] (
reintroduce-else) - Remove unnecessary else after guard condition (
remove-unnecessary-else) - Replace if statement with if expression (
assign-if-exp)
|
|
||
| try: | ||
| # Extract emotional keywords for SAMO optimization | ||
| emotional_keywords = self._extract_emotional_keywords(text) |
There was a problem hiding this comment.
issue (code-quality): Extract code out into method (extract-method)
|
|
||
| try: | ||
| # Initialize summarizer | ||
| print("1. Initializing SAMO T5 Summarizer...") |
There was a problem hiding this comment.
issue (code-quality): Extract code out into function [×2] (extract-method)
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive strategy and initial implementation for breaking down a large, unmanageable pull request into smaller, focused micro-PRs. The changes include documentation for the breakdown plan, a prevention strategy with automated checks, and the first functional piece: a T5 summarization model with its configuration and tests. While the overall approach is excellent, several of the automation scripts and implementation details contain critical flaws that need to be addressed. My review focuses on correcting security vulnerabilities, fixing logical errors in the automation scripts, improving performance in the model implementation, and strengthening the test suite.
| COMMIT_MSG=$(git log -1 --pretty=%B) | ||
| if [[ $COMMIT_MSG == *" and "* ]] || [[ $COMMIT_MSG == *" also "* ]]; then | ||
| echo "❌ Commit message suggests multiple changes" | ||
| echo "💡 Use 'feat:', 'fix:', or 'refactor:' with single purpose" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
This check for the commit message format is implemented in a pre-commit hook, which is incorrect. At the pre-commit stage, the commit message has not been created yet. git log -1 will fetch the previous commit's message.
This check should be moved to a commit-msg hook, which runs after the message is written and receives the path to the message file as its first argument. The current implementation is non-functional for its intended purpose.
| files = pr.get("changed_files", 0) | ||
| additions = pr.get("additions", 0) | ||
| deletions = pr.get("deletions", 0) | ||
| commits = pr.get("commits", 0) |
There was a problem hiding this comment.
The script attempts to get changed_files, additions, and deletions from the PR objects returned by the pulls list endpoint. However, this endpoint returns a summary and does not include these details. To get this information, you must make a separate API request to the url of each individual pull request. As a result, the current metrics for files and lines changed will always be 0, making the dashboard inaccurate.
| dashboard = PRHealthDashboard("uelkerd", "SAMO--DL", "your_token_here") | ||
| dashboard.generate_report() |
There was a problem hiding this comment.
A GitHub token is hardcoded in the example usage. This is a significant security risk. Secrets like API tokens should never be committed to the repository. Instead, load it from an environment variable.
| dashboard = PRHealthDashboard("uelkerd", "SAMO--DL", "your_token_here") | |
| dashboard.generate_report() | |
| token = os.environ.get("GITHUB_TOKEN") | |
| if not token: | |
| raise ValueError("GITHUB_TOKEN environment variable not set") | |
| dashboard = PRHealthDashboard("uelkerd", "SAMO--DL", token) | |
| dashboard.generate_report() |
|
|
||
| try: | ||
| # Create branch | ||
| subprocess.run(["git", "checkout", "-b", branch_name], check=True) |
There was a problem hiding this comment.
The script creates a new branch from the current branch. This is risky, as it could be a feature branch, leading to incorrect history. Before creating a new branch for a micro-PR, the script should ensure it is branching from an up-to-date main branch.
| subprocess.run(["git", "checkout", "-b", branch_name], check=True) | |
| subprocess.run(["git", "checkout", "main"], check=True) | |
| subprocess.run(["git", "pull"], check=True) | |
| subprocess.run(["git", "checkout", "-b", branch_name], check=True) |
| # Merge with defaults | ||
| for key, value in default_config.items(): | ||
| if key not in config: | ||
| config[key] = value | ||
| return config |
There was a problem hiding this comment.
The configuration loading logic performs a shallow merge of the user-provided config with the default config. If a user overrides a nested dictionary like generation with only a subset of keys, the other default keys within that dictionary will be lost. This can lead to unexpected behavior or errors. A deep merge is required to handle nested configurations correctly.
| const { data: pr } = await github.rest.pulls.get({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| pull_number: context.issue.number | ||
| }); | ||
|
|
||
| const { data: files } = await github.rest.pulls.listFiles({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| pull_number: context.issue.number | ||
| }); | ||
|
|
||
| const linesChanged = pr.additions + pr.deletions; | ||
| const filesChanged = files.length; | ||
| const commits = pr.commits; | ||
|
|
||
| // Hard limits | ||
| if (filesChanged > 25) { | ||
| await github.rest.issues.createComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: context.issue.number, | ||
| body: `🚨 **PR REJECTED**: Too many files changed (${filesChanged}/25)` | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The GitHub Action script correctly identifies when PR limits are exceeded, but it only posts a comment. This does not prevent the PR from being merged. To enforce the limits, the workflow step should be marked as failed. This can be done using core.setFailed().
| const { data: pr } = await github.rest.pulls.get({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| pull_number: context.issue.number | |
| }); | |
| const { data: files } = await github.rest.pulls.listFiles({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| pull_number: context.issue.number | |
| }); | |
| const linesChanged = pr.additions + pr.deletions; | |
| const filesChanged = files.length; | |
| const commits = pr.commits; | |
| // Hard limits | |
| if (filesChanged > 25) { | |
| await github.rest.issues.createComment({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| issue_number: context.issue.number, | |
| body: `🚨 **PR REJECTED**: Too many files changed (${filesChanged}/25)` | |
| }); | |
| return; | |
| } | |
| if (filesChanged > 25) { | |
| core.setFailed(`🚨 **PR REJECTED**: Too many files changed (${filesChanged}/25)`); | |
| return; | |
| } |
| const { data: files } = await github.rest.pulls.listFiles({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| pull_number: context.issue.number | ||
| }); |
There was a problem hiding this comment.
The script makes an unnecessary API call to pulls.listFiles to get the number of changed files. The pr object, fetched via pulls.get, already contains the changed_files count. Using pr.changed_files is more efficient and avoids potential pagination issues with listFiles on very large PRs.
| const { data: files } = await github.rest.pulls.listFiles({ | |
| owner: context.repo.owner, | |
| repo: context.repo.repo, | |
| pull_number: context.issue.number | |
| }); | |
| const filesChanged = pr.changed_files; |
| - **Phase 1**: 3/3 PRs completed | ||
| - **Phase 2**: 0/4 PRs completed | ||
| - **Phase 3**: 0/5 PRs completed | ||
| - **Phase 4**: 0/3 PRs completed | ||
| - **Overall**: 1/15 PRs completed (6.7%) |
There was a problem hiding this comment.
There's a contradiction in the progress tracking. This line states that Phase 1 is 3/3 PRs completed, but the lists of PRs in the MICRO-PR TRACKING section show that only PR-1 is complete, while PR-2 and PR-3 are still pending. The Overall progress also states 1/15 PRs completed. Please update this for consistency.
|
|
||
| import torch | ||
| from transformers import T5ForConditionalGeneration, T5Tokenizer | ||
| import numpy as np |
There was a problem hiding this comment.
The numpy and pathlib modules are imported but never used in this file. Unused imports should be removed to keep the code clean.
| def test_samo_t5_summarizer(): | ||
| """Test the SAMO T5 summarizer functionality.""" |
There was a problem hiding this comment.
This test script is implemented as a standalone executable with print statements for reporting. While functional for a quick check, it's not a scalable or standard way to write tests. It would be better to structure these tests using a framework like pytest. This would make them auto-discoverable, provide clearer pass/fail reporting, and allow for more powerful features like fixtures and assertions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (23)
SURGICAL_BREAKDOWN_MASTER_PLAN.md (3)
34-39: Add blank lines around tables to satisfy MD058.-### **Phase 1: Core Models (3 PRs)** -| PR | Branch | Files | Lines | Purpose | +### **Phase 1: Core Models (3 PRs)** + +| PR | Branch | Files | Lines | Purpose | |----|--------|-------|-------|---------| ... -| **PR-3** | `feat/dl-add-emotion-detection-enhancements` | 6 | ~150 | Enhance existing emotion detection | +| **PR-3** | `feat/dl-add-emotion-detection-enhancements` | 6 | ~150 | Enhance existing emotion detection | + -### **Phase 2: API Infrastructure (4 PRs)** -| PR | Branch | Files | Lines | Purpose | +### **Phase 2: API Infrastructure (4 PRs)** + +| PR | Branch | Files | Lines | Purpose | ... -### **Phase 3: API Endpoints (5 PRs)** -| PR | Branch | Files | Lines | Purpose | +### **Phase 3: API Endpoints (5 PRs)** + +| PR | Branch | Files | Lines | Purpose | ... -### **Phase 4: Testing & Quality (3 PRs)** -| PR | Branch | Files | Lines | Purpose | +### **Phase 4: Testing & Quality (3 PRs)** + +| PR | Branch | Files | Lines | Purpose |Also applies to: 41-47, 49-56, 58-63
69-89: Replace empty “Link to PR” placeholders with real links or plain text.MD042 flags “(#)” as empty links. Use actual PR URLs once created or remove link markup.
-- [x] **PR-1**: `feat/dl-add-t5-summarization-model` - [Link to PR](#) - ✅ **READY** +- [x] **PR-1**: `feat/dl-add-t5-summarization-model` — PR link: TBD — ✅ **READY** ... -- [ ] **PR-2**: `feat/dl-add-whisper-transcription-model` - [Link to PR](#) - 🚧 **NEXT** +- [ ] **PR-2**: `feat/dl-add-whisper-transcription-model` — PR link: TBD — 🚧 **NEXT**
121-127: Revisit “100% test coverage” gate (risk of perfidious incentives).Consider ≥85–90% for new code and critical-path 100% if you must enforce it; otherwise teams game the metric.
PR_PREVENTION_STRATEGY.md (4)
223-244: Code fence language mislabeled; use yaml not python.This block is YAML for pre-commit; fix fence for readability.
-```python +```yaml # .pre-commit-config.yaml repos: - repo: local
280-312: check_dependencies robustness (main vs origin/main, safety presence).Handle missing upstream, and install safety or fail with actionable message.
- result = subprocess.run(['git', 'diff', '--name-only', 'main'], + base = 'origin/main' + probe = subprocess.run(['git', 'rev-parse', '--verify', base], capture_output=True) + if probe.returncode != 0: + base = 'main' + result = subprocess.run(['git', 'diff', '--name-only', base], capture_output=True, text=True) ... - result = subprocess.run(['safety', 'check', '--file', req_file], + result = subprocess.run(['safety', 'check', '--file', req_file, '--full-report'], capture_output=True, text=True) if result.returncode != 0:
417-443: Weekly report relies on gh and jq; document prerequisites.Add a short note that gh and jq must be installed or gate the commands.
-# scripts/weekly_health_report.sh +# scripts/weekly_health_report.sh +# Requires: gh CLI and jq on PATH
1-6: One more nit: standardize on “safety” vs “safety-mcp”.Docs mention “safety-mcp” elsewhere; keep one name to avoid confusion.
src/models/summarization/samo_t5_summarizer.py (8)
27-30: Avoid configuring global logging in a library module.Remove
basicConfig; respect log level from config later.-# Configure logging -logging.basicConfig(level=logging.INFO) -logger = logging.getLogger(__name__) +# Module logger only; configure in app or based on config after load +logger = logging.getLogger(__name__)
21-26: Remove unused imports.
Pathandnumpyaren’t used.-from pathlib import Path +from pathlib import Path # unused ... -import numpy as np +import numpy as np # unusedConsider deleting both lines.
101-109: Respect configured device fallback.Use config["model"]["device"] when set; else auto-detect.
def _get_device(self) -> str: """Get the best available device.""" - if torch.cuda.is_available(): + preferred = (self.config.get("model", {}) or {}).get("device") + if preferred in {"cuda", "mps", "cpu"}: + return preferred + if torch.cuda.is_available(): return "cuda" elif hasattr(torch.backends, 'mps') and torch.backends.mps.is_available(): return "mps" else: return "cpu"
110-129: Exception handling: use logger.exception and preserve traceback.Also small nit: prefer AutoTokenizer for flexibility (optional).
def _load_model(self) -> None: """Load the T5 model and tokenizer.""" try: model_name = self.config["model"]["name"] logger.info("Loading T5 model: %s", model_name) # Load tokenizer - self.tokenizer = T5Tokenizer.from_pretrained(model_name) + self.tokenizer = T5Tokenizer.from_pretrained(model_name) # Load model self.model = T5ForConditionalGeneration.from_pretrained(model_name) self.model.to(self.device) self.model.eval() logger.info("T5 model loaded successfully on %s", self.device) except Exception as e: - logger.error("Failed to load T5 model: %s", e) - raise RuntimeError(f"Model loading failed: {e}") + logger.exception("Failed to load T5 model") + raise RuntimeError("Model loading failed") from e
237-244: Compression ratio is inverted.Use original/summary to match common “X:1” semantics and the doc’s example.
- original_length = len(text.split()) - summary_length = len(summary.split()) - compression_ratio = summary_length / original_length if original_length > 0 else 0 + original_length = len(text.split()) + summary_length = len(summary.split()) + compression_ratio = ( + (original_length / summary_length) if summary_length > 0 else 0.0 + )
254-261: Use logger.exception for runtime failures.Keeps stacktrace for debugging.
- except Exception as e: - logger.error("Summarization failed: %s", e) + except Exception as e: + logger.exception("Summarization failed") return { "summary": "", "error": str(e), "success": False, "processing_time": time.time() - start_time }
16-26: Minor: drop unused imports and keep this module import-light.Good practice for model modules.
305-322: Consider moving the example to examples/ or a CLI.Keeping heavy models out of library modules helps packaging; optional.
scripts/surgical_breakdown_executor.py (3)
139-142: Remove unnecessary f-strings.No interpolations; drop the
fprefix.- print(f" ✅ PR validation passed") + print(" ✅ PR validation passed") else: - print(f" ❌ PR validation failed") + print(" ❌ PR validation failed")
250-257: Validate--pr-numberrange (1–15).Prevents accidental out-of-range branches.
if args.command == "create-pr": - pr_info = executor.get_pr_info(args.pr_number) + if not 1 <= args.pr_number <= 15: + print("❌ --pr-number must be between 1 and 15") + sys.exit(2) + pr_info = executor.get_pr_info(args.pr_number)
1-1: Shebang vs. execution bit.Either make the file executable (
chmod +x) or remove the shebang to silence EXE001.configs/samo_t5_config.yaml (2)
8-8: Remove trailing spaces flagged by yamllint.Whitespace-only lines contain trailing spaces. Clean them to pass lint.
- + @@ - + @@ - + @@ - + @@ - + @@ - + @@ - + @@ - + @@ - +Also applies to: 14-14, 18-18, 22-22, 26-26, 31-31, 36-36, 42-42, 46-46
23-26:temperatureis unused withdo_sample: false.You can drop it to avoid confusion, or keep it with a comment.
do_sample: false # Use beam search for consistency - temperature: 1.0 # Deterministic generation + # temperature is ignored when do_sample is falsetest_samo_t5_standalone.py (3)
1-1: Shebang vs. execution bit.Script isn’t executable; remove shebang or
chmod +x.
39-45: Dedent and trim the multiline sample to avoid accidental leading spaces.- test_text = """ - Today I had an amazing experience at the conference. I learned so much about AI and machine learning. - The speakers were incredibly knowledgeable and the networking opportunities were fantastic. I met - several people who share my passion for deep learning and we exchanged contact information. I'm - feeling really excited about the future possibilities and can't wait to implement some of the - techniques I learned. This has been one of the most productive days I've had in months. - """ + test_text = textwrap.dedent(""" + Today I had an amazing experience at the conference. I learned so much about AI and machine learning. + The speakers were incredibly knowledgeable and the networking opportunities were fantastic. I met + several people who share my passion for deep learning and we exchanged contact information. I'm + feeling really excited about the future possibilities and can't wait to implement some of the + techniques I learned. This has been one of the most productive days I've had in months. + """).strip()
93-97: Avoid catching broadExceptionin tests unless necessary.If feasible, narrow to expected exceptions to improve signal; otherwise, add a comment justifying broad catch.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
PR_PREVENTION_STRATEGY.md(1 hunks)SURGICAL_BREAKDOWN_MASTER_PLAN.md(1 hunks)configs/samo_t5_config.yaml(1 hunks)scripts/surgical_breakdown_executor.py(1 hunks)src/models/summarization/samo_t5_summarizer.py(1 hunks)test_samo_t5_standalone.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test_samo_t5_standalone.py (1)
src/models/summarization/samo_t5_summarizer.py (4)
create_samo_t5_summarizer(292-302)get_model_info(281-289)generate_summary(178-261)generate_batch_summaries(263-279)
🪛 Ruff (0.12.2)
src/models/summarization/samo_t5_summarizer.py
1-1: Shebang is present but file is not executable
(EXE001)
95-95: Consider moving this statement to an else block
(TRY300)
96-96: Do not catch blind exception: Exception
(BLE001)
126-126: Do not catch blind exception: Exception
(BLE001)
127-127: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
128-128: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
128-128: Avoid specifying long messages outside the exception class
(TRY003)
243-252: Consider moving this statement to an else block
(TRY300)
254-254: Do not catch blind exception: Exception
(BLE001)
255-255: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
scripts/surgical_breakdown_executor.py
1-1: Shebang is present but file is not executable
(EXE001)
63-63: subprocess call: check for execution of untrusted input
(S603)
63-63: Starting a process with a partial executable path
(S607)
68-68: subprocess call: check for execution of untrusted input
(S603)
68-68: Starting a process with a partial executable path
(S607)
75-75: subprocess call: check for execution of untrusted input
(S603)
75-75: Starting a process with a partial executable path
(S607)
90-90: Consider moving this statement to an else block
(TRY300)
102-102: subprocess call: check for execution of untrusted input
(S603)
103-103: Starting a process with a partial executable path
(S607)
110-110: subprocess call: check for execution of untrusted input
(S603)
111-111: Starting a process with a partial executable path
(S607)
117-117: subprocess call: check for execution of untrusted input
(S603)
118-118: Starting a process with a partial executable path
(S607)
139-139: f-string without any placeholders
Remove extraneous f prefix
(F541)
141-141: f-string without any placeholders
Remove extraneous f prefix
(F541)
143-143: Consider moving this statement to an else block
(TRY300)
test_samo_t5_standalone.py
1-1: Shebang is present but file is not executable
(EXE001)
91-91: Consider moving this statement to an else block
(TRY300)
93-93: Do not catch blind exception: Exception
(BLE001)
🪛 markdownlint-cli2 (0.17.2)
SURGICAL_BREAKDOWN_MASTER_PLAN.md
34-34: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
41-41: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
49-49: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
58-58: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
69-69: No empty links
(MD042, no-empty-links)
74-74: No empty links
(MD042, no-empty-links)
75-75: No empty links
(MD042, no-empty-links)
78-78: No empty links
(MD042, no-empty-links)
79-79: No empty links
(MD042, no-empty-links)
80-80: No empty links
(MD042, no-empty-links)
81-81: No empty links
(MD042, no-empty-links)
82-82: No empty links
(MD042, no-empty-links)
83-83: No empty links
(MD042, no-empty-links)
84-84: No empty links
(MD042, no-empty-links)
85-85: No empty links
(MD042, no-empty-links)
86-86: No empty links
(MD042, no-empty-links)
87-87: No empty links
(MD042, no-empty-links)
88-88: No empty links
(MD042, no-empty-links)
89-89: No empty links
(MD042, no-empty-links)
🪛 YAMLlint (1.37.1)
configs/samo_t5_config.yaml
[error] 8-8: trailing spaces
(trailing-spaces)
[error] 14-14: trailing spaces
(trailing-spaces)
[error] 18-18: trailing spaces
(trailing-spaces)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 26-26: trailing spaces
(trailing-spaces)
[error] 31-31: trailing spaces
(trailing-spaces)
[error] 36-36: trailing spaces
(trailing-spaces)
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
🔇 Additional comments (6)
SURGICAL_BREAKDOWN_MASTER_PLAN.md (1)
155-157: Last-updated timestamp is stale relative to this PR’s creation (Jan 09, 2025 vs Sep 09, 2025).Update “Last Updated” and re-affirm the ETA to avoid confusion.
-**Last Updated**: 2025-01-09 -**Status**: 🚧 **IN PROGRESS** - PR-1 completed, PR-2 next -**Estimated Completion**: 2-3 days with focused effort +**Last Updated**: 2025-09-09 +**Status**: 🚧 **IN PROGRESS** - PR-1 completed, PR-2 next +**Estimated Completion**: 2–3 days from Last UpdatedPR_PREVENTION_STRATEGY.md (4)
1-6: Title and scope are solid.Clear problem/solution framing and actionable controls. Nice.
447-456: Target metrics look reasonable.Good, measurable thresholds; align with the guardian limits.
469-492: Timeline is practical.Phased rollout makes sense.
151-216: Great templates.Daily/Micro-PR templates will help keep PRs small and focused.
src/models/summarization/samo_t5_summarizer.py (1)
178-199: Good: input validation short-circuits early with clear errors.
| ```yaml | ||
| # .github/workflows/pr-guardian.yml | ||
| name: PR Guardian | ||
| on: | ||
| pull_request: | ||
| types: [opened, synchronize] | ||
|
|
||
| jobs: | ||
| pr-guardian: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Check PR Size | ||
| uses: actions/github-script@v6 | ||
| with: | ||
| script: | | ||
| const { data: pr } = await github.rest.pulls.get({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| pull_number: context.issue.number | ||
| }); | ||
|
|
||
| const { data: files } = await github.rest.pulls.listFiles({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| pull_number: context.issue.number | ||
| }); | ||
|
|
||
| const linesChanged = pr.additions + pr.deletions; | ||
| const filesChanged = files.length; | ||
| const commits = pr.commits; | ||
|
|
||
| // Hard limits | ||
| if (filesChanged > 25) { | ||
| await github.rest.issues.createComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: context.issue.number, | ||
| body: `🚨 **PR REJECTED**: Too many files changed (${filesChanged}/25)` | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (linesChanged > 500) { | ||
| await github.rest.issues.createComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: context.issue.number, | ||
| body: `🚨 **PR REJECTED**: Too many lines changed (${linesChanged}/500)` | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (commits > 5) { | ||
| await github.rest.issues.createComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: context.issue.number, | ||
| body: `🚨 **PR REJECTED**: Too many commits (${commits}/5)` | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| // Warning thresholds | ||
| if (filesChanged > 15) { | ||
| await github.rest.issues.createComment({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| issue_number: context.issue.number, | ||
| body: `⚠️ **WARNING**: Approaching file limit (${filesChanged}/25)` | ||
| }); | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
PR Guardian doesn’t block merges; add failure + pagination.
github-script must call core.setFailed() to fail the job; listFiles needs pagination for >300 files.
- - name: Check PR Size
- uses: actions/github-script@v6
+ - name: Check PR Size
+ uses: actions/github-script@v7
with:
script: |
const { data: pr } = await github.rest.pulls.get({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: context.issue.number
});
-
- const { data: files } = await github.rest.pulls.listFiles({
- owner: context.repo.owner,
- repo: context.repo.repo,
- pull_number: context.issue.number
- });
+ // Paginate changed files
+ const files = await github.paginate(github.rest.pulls.listFiles, {
+ owner: context.repo.owner,
+ repo: context.repo.repo,
+ pull_number: context.issue.number,
+ per_page: 100
+ });
const linesChanged = pr.additions + pr.deletions;
const filesChanged = files.length;
const commits = pr.commits;
// Hard limits
if (filesChanged > 25) {
await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: context.issue.number,
body: `🚨 **PR REJECTED**: Too many files changed (${filesChanged}/25)`
});
- return;
+ core.setFailed('Too many files changed');
+ return;
}
if (linesChanged > 500) {
await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: context.issue.number,
body: `🚨 **PR REJECTED**: Too many lines changed (${linesChanged}/500)`
});
- return;
+ core.setFailed('Too many lines changed');
+ return;
}
if (commits > 5) {
await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: context.issue.number,
body: `🚨 **PR REJECTED**: Too many commits (${commits}/5)`
});
- return;
+ core.setFailed('Too many commits');
+ return;
}🤖 Prompt for AI Agents
In PR_PREVENTION_STRATEGY.md around lines 12 to 83, the github-script currently
only posts comments and doesn't fail the workflow nor handle pull request file
pagination; update the script to use @actions/core (or context.github) to call
core.setFailed() (or throw an error) when hard limits are exceeded so the job
fails and blocks merges, and replace the single github.rest.pulls.listFiles call
with a paginated approach (either github.paginate or loop over listFiles with
page/per_page until all files are retrieved) to correctly count files for PRs
with >300 files; ensure the rest of the logic uses the aggregated files length
and that warnings still only comment without failing.
| ```bash | ||
| #!/bin/bash | ||
| # .git/hooks/pre-commit | ||
|
|
||
| # Check file count | ||
| CHANGED_FILES=$(git diff --cached --name-only | wc -l) | ||
| if [ $CHANGED_FILES -gt 25 ]; then | ||
| echo "❌ Too many files changed: $CHANGED_FILES (max 25)" | ||
| echo "💡 Consider splitting this into multiple PRs" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Check line count | ||
| LINES_CHANGED=$(git diff --cached --numstat | awk '{sum += $1 + $2} END {print sum}') | ||
| if [ $LINES_CHANGED -gt 500 ]; then | ||
| echo "❌ Too many lines changed: $LINES_CHANGED (max 500)" | ||
| echo "💡 Consider splitting this into multiple PRs" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Check commit message format | ||
| COMMIT_MSG=$(git log -1 --pretty=%B) | ||
| if [[ $COMMIT_MSG == *" and "* ]] || [[ $COMMIT_MSG == *" also "* ]]; then | ||
| echo "❌ Commit message suggests multiple changes" | ||
| echo "💡 Use 'feat:', 'fix:', or 'refactor:' with single purpose" | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "✅ Pre-commit checks passed" | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pre-commit script: wrong stage for commit message, and brittle shell.
Move commit-message checks to commit-msg; keep pre-commit strictly for staged diff; add strict bash flags.
-#!/bin/bash
-# .git/hooks/pre-commit
+#!/usr/bin/env bash
+# .git/hooks/pre-commit
+set -euo pipefail
+IFS=$'\n\t'
...
-# Check commit message format
-COMMIT_MSG=$(git log -1 --pretty=%B)
-if [[ $COMMIT_MSG == *" and "* ]] || [[ $COMMIT_MSG == *" also "* ]]; then
- echo "❌ Commit message suggests multiple changes"
- echo "💡 Use 'feat:', 'fix:', or 'refactor:' with single purpose"
- exit 1
-fi
-
echo "✅ Pre-commit checks passed"And ensure commit message checks live in a commit-msg hook (already configured via check_commit_message.py).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```bash | |
| #!/bin/bash | |
| # .git/hooks/pre-commit | |
| # Check file count | |
| CHANGED_FILES=$(git diff --cached --name-only | wc -l) | |
| if [ $CHANGED_FILES -gt 25 ]; then | |
| echo "❌ Too many files changed: $CHANGED_FILES (max 25)" | |
| echo "💡 Consider splitting this into multiple PRs" | |
| exit 1 | |
| fi | |
| # Check line count | |
| LINES_CHANGED=$(git diff --cached --numstat | awk '{sum += $1 + $2} END {print sum}') | |
| if [ $LINES_CHANGED -gt 500 ]; then | |
| echo "❌ Too many lines changed: $LINES_CHANGED (max 500)" | |
| echo "💡 Consider splitting this into multiple PRs" | |
| exit 1 | |
| fi | |
| # Check commit message format | |
| COMMIT_MSG=$(git log -1 --pretty=%B) | |
| if [[ $COMMIT_MSG == *" and "* ]] || [[ $COMMIT_MSG == *" also "* ]]; then | |
| echo "❌ Commit message suggests multiple changes" | |
| echo "💡 Use 'feat:', 'fix:', or 'refactor:' with single purpose" | |
| exit 1 | |
| fi | |
| echo "✅ Pre-commit checks passed" | |
| ``` | |
| #!/usr/bin/env bash | |
| # .git/hooks/pre-commit | |
| set -euo pipefail | |
| IFS=$'\n\t' | |
| # Check file count | |
| CHANGED_FILES=$(git diff --cached --name-only | wc -l) | |
| if [ "$CHANGED_FILES" -gt 25 ]; then | |
| echo "❌ Too many files changed: $CHANGED_FILES (max 25)" | |
| echo "💡 Consider splitting this into multiple PRs" | |
| exit 1 | |
| fi | |
| # Check line count | |
| LINES_CHANGED=$(git diff --cached --numstat | awk '{sum += $1 + $2} END {print sum}') | |
| if [ "$LINES_CHANGED" -gt 500 ]; then | |
| echo "❌ Too many lines changed: $LINES_CHANGED (max 500)" | |
| echo "💡 Consider splitting this into multiple PRs" | |
| exit 1 | |
| fi | |
| echo "✅ Pre-commit checks passed" |
🤖 Prompt for AI Agents
In PR_PREVENTION_STRATEGY.md around lines 86 to 115, the pre-commit hook shown
performs commit-message validation and lacks strict shell flags; move the commit
message checks out of the pre-commit hook and into the commit-msg hook (the
repository already has check_commit_message.py), keep the pre-commit hook
focused only on staged-diff checks (file and line counts), and harden the script
by adding strict bash options (set -euo pipefail and IFS) and robust variable
quoting and numeric tests so it fails safely and handles edge cases.
| ```python | ||
| # scripts/validate_branch_name.py | ||
| import re | ||
| import sys | ||
|
|
||
| def validate_branch_name(branch_name): | ||
| """Validate branch name follows our conventions.""" | ||
| pattern = r'^(feat|fix|refactor|chore|docs)/dl-[a-z0-9-]+$' | ||
|
|
||
| if not re.match(pattern, branch_name): | ||
| print(f"❌ Invalid branch name: {branch_name}") | ||
| print("✅ Valid format: feat/dl-add-user-authentication") | ||
| print("✅ Valid format: fix/dl-memory-leak") | ||
| print("✅ Valid format: refactor/dl-training-loop") | ||
| return False | ||
|
|
||
| # Check for multiple purposes | ||
| if ' and ' in branch_name or ' also ' in branch_name: | ||
| print(f"❌ Branch name suggests multiple purposes: {branch_name}") | ||
| print("💡 Split into multiple branches") | ||
| return False | ||
|
|
||
| print(f"✅ Valid branch name: {branch_name}") | ||
| return True | ||
|
|
||
| if __name__ == "__main__": | ||
| branch_name = sys.argv[1] if len(sys.argv) > 1 else "" | ||
| if not validate_branch_name(branch_name): | ||
| sys.exit(1) | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Branch-name validator: auto-detect current branch when no argv is passed.
Pre-commit won’t pass args; resolve branch via git to avoid false failures.
-if __name__ == "__main__":
- branch_name = sys.argv[1] if len(sys.argv) > 1 else ""
+if __name__ == "__main__":
+ if len(sys.argv) > 1:
+ branch_name = sys.argv[1]
+ else:
+ import subprocess
+ branch_name = subprocess.run(
+ ["git", "rev-parse", "--abbrev-ref", "HEAD"],
+ check=False, capture_output=True, text=True
+ ).stdout.strip()
if not validate_branch_name(branch_name):
sys.exit(1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```python | |
| # scripts/validate_branch_name.py | |
| import re | |
| import sys | |
| def validate_branch_name(branch_name): | |
| """Validate branch name follows our conventions.""" | |
| pattern = r'^(feat|fix|refactor|chore|docs)/dl-[a-z0-9-]+$' | |
| if not re.match(pattern, branch_name): | |
| print(f"❌ Invalid branch name: {branch_name}") | |
| print("✅ Valid format: feat/dl-add-user-authentication") | |
| print("✅ Valid format: fix/dl-memory-leak") | |
| print("✅ Valid format: refactor/dl-training-loop") | |
| return False | |
| # Check for multiple purposes | |
| if ' and ' in branch_name or ' also ' in branch_name: | |
| print(f"❌ Branch name suggests multiple purposes: {branch_name}") | |
| print("💡 Split into multiple branches") | |
| return False | |
| print(f"✅ Valid branch name: {branch_name}") | |
| return True | |
| if __name__ == "__main__": | |
| branch_name = sys.argv[1] if len(sys.argv) > 1 else "" | |
| if not validate_branch_name(branch_name): | |
| sys.exit(1) | |
| ``` | |
| # scripts/validate_branch_name.py | |
| import re | |
| import sys | |
| def validate_branch_name(branch_name): | |
| """Validate branch name follows our conventions.""" | |
| pattern = r'^(feat|fix|refactor|chore|docs)/dl-[a-z0-9-]+$' | |
| if not re.match(pattern, branch_name): | |
| print(f"❌ Invalid branch name: {branch_name}") | |
| print("✅ Valid format: feat/dl-add-user-authentication") | |
| print("✅ Valid format: fix/dl-memory-leak") | |
| print("✅ Valid format: refactor/dl-training-loop") | |
| return False | |
| # Check for multiple purposes | |
| if ' and ' in branch_name or ' also ' in branch_name: | |
| print(f"❌ Branch name suggests multiple purposes: {branch_name}") | |
| print("💡 Split into multiple branches") | |
| return False | |
| print(f"✅ Valid branch name: {branch_name}") | |
| return True | |
| if __name__ == "__main__": | |
| if len(sys.argv) > 1: | |
| branch_name = sys.argv[1] | |
| else: | |
| import subprocess | |
| branch_name = subprocess.run( | |
| ["git", "rev-parse", "--abbrev-ref", "HEAD"], | |
| check=False, capture_output=True, text=True | |
| ).stdout.strip() | |
| if not validate_branch_name(branch_name): | |
| sys.exit(1) |
🤖 Prompt for AI Agents
In PR_PREVENTION_STRATEGY.md around lines 118 to 147, the branch-name validator
fails when no argv is passed (pre-commit runs) because it doesn't auto-detect
the current git branch; update the script so when sys.argv has no branch
argument it runs a git command (e.g., git rev-parse --abbrev-ref HEAD or git
symbolic-ref --short HEAD) to obtain the current branch, handle subprocess
failures gracefully (print a clear error and return False / exit non-zero), and
ensure the detected branch is stripped before passing to validate_branch_name.
| # scripts/pr_health_dashboard.py | ||
| import json | ||
| import requests | ||
| from datetime import datetime, timedelta | ||
|
|
||
| class PRHealthDashboard: | ||
| def __init__(self, repo_owner, repo_name, token): | ||
| self.repo_owner = repo_owner | ||
| self.repo_name = repo_name | ||
| self.token = token | ||
| self.headers = {"Authorization": f"token {token}"} | ||
|
|
||
| def get_pr_metrics(self): | ||
| """Get PR metrics for the last 30 days.""" | ||
| url = f"https://api.github.com/repos/{self.repo_owner}/{self.repo_name}/pulls" | ||
| params = { | ||
| "state": "all", | ||
| "since": (datetime.now() - timedelta(days=30)).isoformat() | ||
| } | ||
|
|
||
| response = requests.get(url, headers=self.headers, params=params) | ||
| prs = response.json() | ||
|
|
||
| metrics = { | ||
| "total_prs": len(prs), | ||
| "avg_files_changed": 0, | ||
| "avg_lines_changed": 0, | ||
| "avg_commits": 0, | ||
| "rule_violations": 0, | ||
| "avg_review_time": 0 | ||
| } | ||
|
|
||
| # Calculate metrics | ||
| total_files = 0 | ||
| total_lines = 0 | ||
| total_commits = 0 | ||
| violations = 0 | ||
|
|
||
| for pr in prs: | ||
| files = pr.get("changed_files", 0) | ||
| additions = pr.get("additions", 0) | ||
| deletions = pr.get("deletions", 0) | ||
| commits = pr.get("commits", 0) | ||
|
|
||
| total_files += files | ||
| total_lines += additions + deletions | ||
| total_commits += commits | ||
|
|
||
| # Check for violations | ||
| if files > 25 or (additions + deletions) > 500 or commits > 5: | ||
| violations += 1 | ||
|
|
||
| if prs: | ||
| metrics["avg_files_changed"] = total_files / len(prs) | ||
| metrics["avg_lines_changed"] = total_lines / len(prs) | ||
| metrics["avg_commits"] = total_commits / len(prs) | ||
| metrics["rule_violations"] = violations | ||
|
|
||
| return metrics | ||
|
|
||
| def generate_report(self): | ||
| """Generate a health report.""" | ||
| metrics = self.get_pr_metrics() | ||
|
|
||
| print("📊 PR HEALTH DASHBOARD") | ||
| print("=" * 30) | ||
| print(f"Total PRs (30 days): {metrics['total_prs']}") | ||
| print(f"Avg files changed: {metrics['avg_files_changed']:.1f}") | ||
| print(f"Avg lines changed: {metrics['avg_lines_changed']:.1f}") | ||
| print(f"Avg commits: {metrics['avg_commits']:.1f}") | ||
| print(f"Rule violations: {metrics['rule_violations']}") | ||
|
|
||
| # Health score | ||
| health_score = 100 | ||
| if metrics['avg_files_changed'] > 15: | ||
| health_score -= 20 | ||
| if metrics['avg_lines_changed'] > 300: | ||
| health_score -= 20 | ||
| if metrics['rule_violations'] > 0: | ||
| health_score -= 30 | ||
|
|
||
| print(f"Health Score: {health_score}/100") | ||
|
|
||
| if health_score < 70: | ||
| print("🚨 ATTENTION: PR health is declining") | ||
| elif health_score < 90: | ||
| print("⚠️ WARNING: PR health needs improvement") | ||
| else: | ||
| print("✅ PR health is excellent") | ||
|
|
||
| if __name__ == "__main__": | ||
| dashboard = PRHealthDashboard("uelkerd", "SAMO--DL", "your_token_here") | ||
| dashboard.generate_report() | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
PR health dashboard: REST parameters are invalid for PRs; use GraphQL or per-PR detail calls.
GET /pulls ignores since; fields like changed_files aren’t present in list responses. Prefer GraphQL to fetch metrics in one request.
-import requests
+import requests
...
- url = f"https://api.github.com/repos/{self.repo_owner}/{self.repo_name}/pulls"
- params = {
- "state": "all",
- "since": (datetime.now() - timedelta(days=30)).isoformat()
- }
-
- response = requests.get(url, headers=self.headers, params=params)
- prs = response.json()
+ # Query GraphQL for last 30 days of updated PRs with metrics
+ q = """
+ query($owner:String!, $name:String!) {
+ repository(owner:$owner, name:$name) {
+ pullRequests(first:100, orderBy:{field:UPDATED_AT, direction:DESC}, states:[OPEN, MERGED, CLOSED]) {
+ nodes {
+ number
+ title
+ updatedAt
+ additions
+ deletions
+ changedFiles
+ commits { totalCount }
+ }
+ }
+ }
+ }
+ """
+ response = requests.post(
+ "https://api.github.com/graphql",
+ headers={**self.headers, "Content-Type": "application/json"},
+ json={"query": q, "variables": {"owner": self.repo_owner, "name": self.repo_name}},
+ timeout=30
+ )
+ data = response.json()
+ prs = data["data"]["repository"]["pullRequests"]["nodes"]Also read the token from env (GITHUB_TOKEN) instead of a placeholder.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In PR_PREVENTION_STRATEGY.md around lines 320 to 413, the script uses the REST
/pulls list endpoint with a non-supported "since" param and expects per-PR
fields (changed_files, additions, deletions, commits) that are not returned in
list responses; also it embeds a placeholder token. Fix by reading the token
from the GITHUB_TOKEN env var, fetch PRs created in the last 30 days either by
switching to the GitHub GraphQL API to request changedFiles, additions,
deletions, commits and createdAt in a single paginated query, or call the REST
list to get PR numbers then fetch per-PR details from GET
/repos/{owner}/{repo}/pulls/{number} (handling pagination and rate limits) to
compute metrics; ensure you filter by created_at within 30 days, handle
pagination, and surface errors when requests fail.
| try: | ||
| # Create branch | ||
| subprocess.run(["git", "checkout", "-b", branch_name], check=True) | ||
|
|
||
| # Add files | ||
| for file_path in files: | ||
| if Path(file_path).exists(): | ||
| subprocess.run(["git", "add", file_path], check=True) | ||
| else: | ||
| print(f" ⚠️ Warning: File {file_path} not found") | ||
|
|
||
| # Commit | ||
| commit_message = f"feat: {description.lower()}\n\n- PR-{pr_number} of surgical breakdown\n- Phase {phase}\n- Files: {len(files)}\n- Estimated lines: {estimated_lines}\n\nThis is a focused micro-PR that addresses exactly one concern\nas part of the surgical breakdown of PR #145." | ||
|
|
||
| subprocess.run(["git", "commit", "-m", commit_message], check=True) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Run git commands in repo root; fix path handling and add -- to git add.
Without cwd, calls depend on the current working directory; existence checks also don’t use project_root. Add cwd=self.project_root to all git calls and guard git add with -- to avoid option-like filenames.
- subprocess.run(["git", "checkout", "-b", branch_name], check=True)
+ subprocess.run(["git", "checkout", "-b", branch_name], check=True, cwd=self.project_root)
@@
- for file_path in files:
- if Path(file_path).exists():
- subprocess.run(["git", "add", file_path], check=True)
+ for file_path in files:
+ abs_path = self.project_root / file_path
+ if abs_path.exists():
+ subprocess.run(["git", "add", "--", file_path], check=True, cwd=self.project_root)
else:
print(f" ⚠️ Warning: File {file_path} not found")
@@
- subprocess.run(["git", "commit", "-m", commit_message], check=True)
+ subprocess.run(["git", "commit", "-m", commit_message], check=True, cwd=self.project_root)
@@
- result = subprocess.run(
+ result = subprocess.run(
["git", "diff", "--name-only", "main", branch_name],
- capture_output=True, text=True, check=True
+ capture_output=True, text=True, check=True, cwd=self.project_root
)
@@
- result = subprocess.run(
+ result = subprocess.run(
["git", "diff", "--numstat", "main", branch_name],
- capture_output=True, text=True, check=True
+ capture_output=True, text=True, check=True, cwd=self.project_root
)
@@
- result = subprocess.run(
+ result = subprocess.run(
["git", "rev-list", "--count", f"main..{branch_name}"],
- capture_output=True, text=True, check=True
+ capture_output=True, text=True, check=True, cwd=self.project_root
)Also applies to: 100-121
🧰 Tools
🪛 Ruff (0.12.2)
63-63: subprocess call: check for execution of untrusted input
(S603)
63-63: Starting a process with a partial executable path
(S607)
68-68: subprocess call: check for execution of untrusted input
(S603)
68-68: Starting a process with a partial executable path
(S607)
75-75: subprocess call: check for execution of untrusted input
(S603)
75-75: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
In scripts/surgical_breakdown_executor.py around lines 61 to 76 (and likewise
apply the same changes to lines 100-121), git subprocess calls and file
existence checks are using the current working directory instead of the
repository root; update checks to resolve paths against self.project_root (e.g.,
Path(self.project_root) / file_path) and add cwd=self.project_root to all
subprocess.run invocations (git checkout, git add, git commit) so commands run
in the repo root; also protect filenames that may look like options by using the
git add -- <file> form (i.e., include "--" before the filename argument) when
invoking git add.
| # Commit | ||
| commit_message = f"feat: {description.lower()}\n\n- PR-{pr_number} of surgical breakdown\n- Phase {phase}\n- Files: {len(files)}\n- Estimated lines: {estimated_lines}\n\nThis is a focused micro-PR that addresses exactly one concern\nas part of the surgical breakdown of PR #145." | ||
|
|
||
| subprocess.run(["git", "commit", "-m", commit_message], check=True) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid failing commit when nothing is staged.
Explicitly check for staged files and abort early with a clear message.
- # Commit
+ # Commit
+ # Ensure there is something to commit
+ staged = subprocess.run(
+ ["git", "diff", "--cached", "--name-only"],
+ capture_output=True, text=True, check=True, cwd=self.project_root
+ ).stdout.strip()
+ if not staged:
+ print(" ❌ No files staged for commit. Aborting.")
+ return False
commit_message = f"feat: {description.lower()}\n\n- PR-{pr_number} of surgical breakdown\n- Phase {phase}\n- Files: {len(files)}\n- Estimated lines: {estimated_lines}\n\nThis is a focused micro-PR that addresses exactly one concern\nas part of the surgical breakdown of PR #145."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Commit | |
| commit_message = f"feat: {description.lower()}\n\n- PR-{pr_number} of surgical breakdown\n- Phase {phase}\n- Files: {len(files)}\n- Estimated lines: {estimated_lines}\n\nThis is a focused micro-PR that addresses exactly one concern\nas part of the surgical breakdown of PR #145." | |
| subprocess.run(["git", "commit", "-m", commit_message], check=True) | |
| # Commit | |
| # Ensure there is something to commit | |
| staged = subprocess.run( | |
| ["git", "diff", "--cached", "--name-only"], | |
| capture_output=True, text=True, check=True, cwd=self.project_root | |
| ).stdout.strip() | |
| if not staged: | |
| print(" ❌ No files staged for commit. Aborting.") | |
| return False | |
| commit_message = f"feat: {description.lower()}\n\n- PR-{pr_number} of surgical breakdown\n- Phase {phase}\n- Files: {len(files)}\n- Estimated lines: {estimated_lines}\n\nThis is a focused micro-PR that addresses exactly one concern\nas part of the surgical breakdown of PR #145." | |
| subprocess.run(["git", "commit", "-m", commit_message], check=True) |
🧰 Tools
🪛 Ruff (0.12.2)
75-75: subprocess call: check for execution of untrusted input
(S603)
75-75: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
In scripts/surgical_breakdown_executor.py around lines 72 to 76, the script
currently runs git commit unconditionally which will fail (and raise) when
nothing is staged; before attempting the commit, run a check for staged files
(e.g., invoke git diff --cached --name-only or git diff --staged --name-only via
subprocess and examine output), and if the output is empty, print/log a clear
message and exit/return early without calling subprocess.run for commit; if
there are staged files proceed to build the commit message and call
subprocess.run as before, and handle possible subprocess exceptions for
robustness.
| # Check line count | ||
| result = subprocess.run( | ||
| ["git", "diff", "--numstat", "main", branch_name], | ||
| capture_output=True, text=True, check=True | ||
| ) | ||
| lines_changed = sum(int(line.split('\t')[0]) for line in result.stdout.strip().split('\n') if line) | ||
|
|
There was a problem hiding this comment.
Fix lines-changed calculation (handles binary diffs and deletions).
Current code counts only insertions and will crash on binary diffs (-). Sum insertions + deletions and treat - as 0.
- lines_changed = sum(int(line.split('\t')[0]) for line in result.stdout.strip().split('\n') if line)
+ lines_changed = 0
+ for row in result.stdout.strip().splitlines():
+ if not row:
+ continue
+ parts = row.split('\t')
+ if len(parts) < 3:
+ continue
+ ins, dels = parts[0], parts[1]
+ ins_n = int(ins) if ins.isdigit() else 0
+ dels_n = int(dels) if dels.isdigit() else 0
+ lines_changed += ins_n + dels_n📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Check line count | |
| result = subprocess.run( | |
| ["git", "diff", "--numstat", "main", branch_name], | |
| capture_output=True, text=True, check=True | |
| ) | |
| lines_changed = sum(int(line.split('\t')[0]) for line in result.stdout.strip().split('\n') if line) | |
| # Check line count | |
| result = subprocess.run( | |
| ["git", "diff", "--numstat", "main", branch_name], | |
| capture_output=True, text=True, check=True | |
| ) | |
| lines_changed = 0 | |
| for row in result.stdout.strip().splitlines(): | |
| if not row: | |
| continue | |
| parts = row.split('\t') | |
| if len(parts) < 3: | |
| continue | |
| ins, dels = parts[0], parts[1] | |
| ins_n = int(ins) if ins.isdigit() else 0 | |
| dels_n = int(dels) if dels.isdigit() else 0 | |
| lines_changed += ins_n + dels_n |
🧰 Tools
🪛 Ruff (0.12.2)
110-110: subprocess call: check for execution of untrusted input
(S603)
111-111: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
In scripts/surgical_breakdown_executor.py around lines 109 to 115, the
lines-changed calculation only converts the insertions field to int and will
crash or miscount for binary diffs (represented as '-') and deletions; update
the parsing to split each numstat line with split('\t', 2), extract added and
deleted fields, convert '-' or non-numeric values to 0 (e.g. if field == '-' or
not field.isdigit(): value = 0 else value = int(field)), sum added+deleted for
each line and then sum across all lines to produce lines_changed so both
insertions and deletions are counted and binary diffs are treated as 0.
| def _load_config(self, config_path: Optional[str]) -> Dict[str, Any]: | ||
| """Load configuration from YAML file.""" | ||
| default_config = { | ||
| "model": { | ||
| "name": "t5-small", | ||
| "device": None | ||
| }, | ||
| "generation": { | ||
| "max_length": 100, | ||
| "min_length": 20, | ||
| "num_beams": 4, | ||
| "early_stopping": True, | ||
| "repetition_penalty": 1.2, | ||
| "length_penalty": 1.0, | ||
| "do_sample": False, | ||
| "temperature": 1.0 | ||
| }, | ||
| "validation": { | ||
| "min_words": 20, | ||
| "max_words": 1000 | ||
| }, | ||
| "performance": { | ||
| "batch_size": 4, | ||
| "timeout_seconds": 30 | ||
| }, | ||
| "samo_optimizations": { | ||
| "emotional_context": True, | ||
| "preserve_tone": True, | ||
| "journal_mode": True, | ||
| "extract_key_emotions": True, | ||
| "sanitize_input": True, | ||
| "log_level": "INFO" | ||
| } | ||
| } | ||
|
|
||
| if config_path and os.path.exists(config_path): | ||
| try: | ||
| with open(config_path, 'r', encoding='utf-8') as f: | ||
| config = yaml.safe_load(f) | ||
| # Merge with defaults | ||
| for key, value in default_config.items(): | ||
| if key not in config: | ||
| config[key] = value | ||
| return config | ||
| except Exception as e: | ||
| logger.warning("Failed to load config from %s: %s", config_path, e) | ||
|
|
||
| return default_config | ||
|
|
||
| def _get_device(self) -> str: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Deep-merge config and honor nested defaults.
Current merge only fills missing top-level keys; nested keys under “generation”, “validation”, etc. may go missing, causing KeyErrors. Also apply log level from config.
def _load_config(self, config_path: Optional[str]) -> Dict[str, Any]:
"""Load configuration from YAML file."""
default_config = {
"model": {
"name": "t5-small",
- "device": None
+ "device": None
},
"generation": {
"max_length": 100,
"min_length": 20,
"num_beams": 4,
"early_stopping": True,
"repetition_penalty": 1.2,
"length_penalty": 1.0,
"do_sample": False,
"temperature": 1.0
},
"validation": {
"min_words": 20,
"max_words": 1000
},
"performance": {
"batch_size": 4,
"timeout_seconds": 30
},
"samo_optimizations": {
"emotional_context": True,
"preserve_tone": True,
"journal_mode": True,
"extract_key_emotions": True,
"sanitize_input": True,
"log_level": "INFO"
}
}
-
- if config_path and os.path.exists(config_path):
- try:
- with open(config_path, 'r', encoding='utf-8') as f:
- config = yaml.safe_load(f)
- # Merge with defaults
- for key, value in default_config.items():
- if key not in config:
- config[key] = value
- return config
- except Exception as e:
- logger.warning("Failed to load config from %s: %s", config_path, e)
-
- return default_config
+ def deep_merge(base: Dict[str, Any], override: Dict[str, Any]) -> Dict[str, Any]:
+ for k, v in override.items():
+ if isinstance(v, dict) and isinstance(base.get(k), dict):
+ base[k] = deep_merge(base[k], v)
+ else:
+ base[k] = v
+ return base
+
+ config = default_config
+ if config_path and os.path.exists(config_path):
+ try:
+ with open(config_path, 'r', encoding='utf-8') as f:
+ loaded = yaml.safe_load(f) or {}
+ config = deep_merge(default_config.copy(), loaded)
+ except Exception as e:
+ logger.warning("Failed to load config from %s: %s", config_path, e)
+
+ # Apply log level if provided
+ lvl = (config.get("samo_optimizations", {}).get("log_level") or "INFO").upper()
+ logger.setLevel(getattr(logging, lvl, logging.INFO))
+ return config📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _load_config(self, config_path: Optional[str]) -> Dict[str, Any]: | |
| """Load configuration from YAML file.""" | |
| default_config = { | |
| "model": { | |
| "name": "t5-small", | |
| "device": None | |
| }, | |
| "generation": { | |
| "max_length": 100, | |
| "min_length": 20, | |
| "num_beams": 4, | |
| "early_stopping": True, | |
| "repetition_penalty": 1.2, | |
| "length_penalty": 1.0, | |
| "do_sample": False, | |
| "temperature": 1.0 | |
| }, | |
| "validation": { | |
| "min_words": 20, | |
| "max_words": 1000 | |
| }, | |
| "performance": { | |
| "batch_size": 4, | |
| "timeout_seconds": 30 | |
| }, | |
| "samo_optimizations": { | |
| "emotional_context": True, | |
| "preserve_tone": True, | |
| "journal_mode": True, | |
| "extract_key_emotions": True, | |
| "sanitize_input": True, | |
| "log_level": "INFO" | |
| } | |
| } | |
| if config_path and os.path.exists(config_path): | |
| try: | |
| with open(config_path, 'r', encoding='utf-8') as f: | |
| config = yaml.safe_load(f) | |
| # Merge with defaults | |
| for key, value in default_config.items(): | |
| if key not in config: | |
| config[key] = value | |
| return config | |
| except Exception as e: | |
| logger.warning("Failed to load config from %s: %s", config_path, e) | |
| return default_config | |
| def _get_device(self) -> str: | |
| def _load_config(self, config_path: Optional[str]) -> Dict[str, Any]: | |
| """Load configuration from YAML file.""" | |
| default_config = { | |
| "model": { | |
| "name": "t5-small", | |
| "device": None | |
| }, | |
| "generation": { | |
| "max_length": 100, | |
| "min_length": 20, | |
| "num_beams": 4, | |
| "early_stopping": True, | |
| "repetition_penalty": 1.2, | |
| "length_penalty": 1.0, | |
| "do_sample": False, | |
| "temperature": 1.0 | |
| }, | |
| "validation": { | |
| "min_words": 20, | |
| "max_words": 1000 | |
| }, | |
| "performance": { | |
| "batch_size": 4, | |
| "timeout_seconds": 30 | |
| }, | |
| "samo_optimizations": { | |
| "emotional_context": True, | |
| "preserve_tone": True, | |
| "journal_mode": True, | |
| "extract_key_emotions": True, | |
| "sanitize_input": True, | |
| "log_level": "INFO" | |
| } | |
| } | |
| def deep_merge(base: Dict[str, Any], override: Dict[str, Any]) -> Dict[str, Any]: | |
| for k, v in override.items(): | |
| if isinstance(v, dict) and isinstance(base.get(k), dict): | |
| base[k] = deep_merge(base[k], v) | |
| else: | |
| base[k] = v | |
| return base | |
| config = default_config | |
| if config_path and os.path.exists(config_path): | |
| try: | |
| with open(config_path, 'r', encoding='utf-8') as f: | |
| loaded = yaml.safe_load(f) or {} | |
| config = deep_merge(default_config.copy(), loaded) | |
| except Exception as e: | |
| logger.warning("Failed to load config from %s: %s", config_path, e) | |
| # Apply log level if provided | |
| lvl = (config.get("samo_optimizations", {}).get("log_level") or "INFO").upper() | |
| logger.setLevel(getattr(logging, lvl, logging.INFO)) | |
| return config |
🧰 Tools
🪛 Ruff (0.12.2)
95-95: Consider moving this statement to an else block
(TRY300)
96-96: Do not catch blind exception: Exception
(BLE001)
| import sys | ||
| import os | ||
| from pathlib import Path | ||
|
|
||
| # Add src to path | ||
| sys.path.insert(0, str(Path(__file__).parent / "src")) | ||
|
|
||
| from models.summarization.samo_t5_summarizer import create_samo_t5_summarizer |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make paths robust and remove dependency on CWD.
Resolve repo root and build an absolute config path. Also dedent the test text.
-import sys
-import os
-from pathlib import Path
+import sys
+from pathlib import Path
+import textwrap
@@
-# Add src to path
-sys.path.insert(0, str(Path(__file__).parent / "src"))
+# Add src to path
+REPO_ROOT = Path(__file__).parent.resolve()
+sys.path.insert(0, str(REPO_ROOT / "src"))
@@
- summarizer = create_samo_t5_summarizer("configs/samo_t5_config.yaml")
+ config_path = REPO_ROOT / "configs" / "samo_t5_config.yaml"
+ summarizer = create_samo_t5_summarizer(str(config_path))Also applies to: 26-27
🤖 Prompt for AI Agents
In test_samo_t5_standalone.py around lines 9 to 16 (and similarly lines 26-27),
avoid relying on the current working directory: compute the repository root from
Path(__file__).resolve().parents[...] and use that to construct an absolute path
to the src package before inserting into sys.path, build the absolute config
path from that same repo-root base, and replace any inline multi-line test text
with a dedented string (e.g., using textwrap.dedent) so the test is independent
of CWD and indentation.
- Mark PR-1 (T5 model) as completed with performance metrics - Mark PR-2 (Whisper model) as completed with performance metrics - Update progress tracking: 2/15 PRs completed (13.3%) - Update Phase 1 progress: 2/3 PRs completed (66.7%) - Set PR-3 (emotion detection) as next priority - Update estimated completion time Both core models are now working and tested: - T5: 0.823 confidence, 0.28:1 compression, MPS device - Whisper: 0.732 avg confidence, 7.3x real-time, multi-format
- Add .surgical_breakdown_progress.json with PR-1 and PR-2 completion - Track performance metrics for both completed PRs - Enable executor script to show accurate status - PR-1: T5 model (0.823 confidence, 0.28 compression, MPS device) - PR-2: Whisper model (0.732 avg confidence, 7.3x real-time) - Next: PR-3 (emotion detection enhancements)
Breaking Down PR #145 (53,340 lines) into 15 Micro-PRs
Status: 🚨 CRITICAL - PR #145 has become unmanageable and must be surgically broken down
Original PR: #145 - Complete AI API with T5 and Whisper
Problem: 53,340 lines changed, 399 files modified, 103 commits - violates all PR rules
Solution: 15 focused micro-PRs + 1 master tracking PR (this one)
📊 ROOT CAUSE ANALYSIS
What Went Wrong:
Prevention Measures:
🎯 SURGICAL BREAKDOWN PLAN
PHASE 1: Core Models (3 PRs) - COMPLETED ✅
✅ PR-1:
feat/dl-add-t5-summarization-model-only- COMPLETED & MERGED✅ PR-2:
feat/dl-add-whisper-transcription-model-only- COMPLETED & MERGED🔄 PR-3:
feat/dl-add-emotion-detection-enhancements- NEXTPHASE 2: Data Pipeline (3 PRs)
feat/dl-add-audio-preprocessing-pipelinefeat/dl-add-text-preprocessing-pipelinefeat/dl-add-batch-processing-utilitiesPHASE 3: API Layer (4 PRs)
feat/dl-add-unified-ai-api-endpointsfeat/dl-add-api-rate-limitingfeat/dl-add-api-documentationfeat/dl-add-api-error-handlingPHASE 4: Infrastructure (3 PRs)
feat/dl-add-docker-optimizationfeat/dl-add-monitoring-loggingfeat/dl-add-security-headersPHASE 5: Testing & Deployment (2 PRs)
feat/dl-add-comprehensive-testingfeat/dl-add-deployment-scripts📈 PROGRESS TRACKING
Completed PRs: 2/15 (13.3%)
Current Status:
Quality Metrics:
🚨 ENFORCEMENT RULES
HARD LIMITS (AUTOMATED)
SCOPE DECLARATION (MANDATORY)
Every PR must include:
FORBIDDEN COMBINATIONS
🎉 SUCCESS METRICS
Before (PR #145):
After (Micro-PRs):
Time Savings:
🔥 NUCLEAR OPTION
If ANY rule is violated:
git stashgit checkout -b feat/dl-[single-thing]git stash popthen selective addNO EXCEPTIONS. NO EXCUSES. NO "JUST THIS ONCE".
Remember: Small PRs = Fast reviews = Quick merges = Happy developers = Successful project
Next Action: Start PR-3 (Emotion Detection Enhancements) 🚀