#171 - Introduce new records ID #173
Conversation
- Code migrated to work with string record ids. - Added logic for detection of cross repo issues and their appending to records.
|
Warning Rate limit exceeded@miroslavpojer has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 21 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 (3)
WalkthroughRecord IDs moved from numeric to repository-scoped strings ("owner/repo#number"); MinedData now requires a Repository and tracks commits/since; DataMiner adds repository existence check and raises on missing repo; factories, models, chapters, utils, generator, filter, and tests updated for string IDs and repository context. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Generator
participant Miner
participant GitHub as GitHub API
participant Factory
participant Builder
participant Chapters
Caller->>Generator: generate()
Generator->>Miner: check_repository_exists()
Miner->>GitHub: safe_call(getRepo)
GitHub-->>Miner: Repo / None
alt Repo not found
Miner-->>Generator: False
Generator-->>Caller: None
else Repo found
Miner-->>Generator: True
Generator->>Miner: mine_data()
Miner->>GitHub: fetch issues/PRs/commits
Miner-->>Generator: MinedData(repo)
Generator->>Factory: generate(MinedData(repo))
Factory-->>Generator: dict[str, Record] ("owner/repo#N")
Generator->>Builder: ReleaseNotesBuilder(records, ...)
Builder->>Chapters: populate(records)
Chapters-->>Builder: done
Builder-->>Generator: notes
Generator-->>Caller: notes
end
sequenceDiagram
autonumber
participant Factory
participant PRUtils
participant Repo as Repository
Factory->>PRUtils: extract_issue_numbers_from_body(pr, Repo)
PRUtils-->>Factory: set[str] e.g. "owner/name#123"
Factory->>Factory: get_id(obj) -> "owner/name#N"
Note right of Factory: Records stored and linked using string IDs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (4 warnings)
✅ Passed checks (1 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
release_notes_generator/record/factory/default_record_factory.py (1)
90-143: Remove unused parameterskip_recThe
skip_recparameter in the nestedregister_pull_requestfunction is never used.Apply this diff to remove the unused parameter:
- def register_pull_request(pull: PullRequest, skip_rec: bool) -> None: + def register_pull_request(pull: PullRequest) -> None:And update the call site:
- register_pull_request(pull, skip_record) + register_pull_request(pull)release_notes_generator/model/mined_data.py (1)
41-48: Use timezone-aware epoch forsince.Naive datetime can cause comparison issues vs. GitHub timestamps. Prefer UTC-aware default.
Apply this diff:
- self.since = datetime(1970, 1, 1) # Default to epoch start + from datetime import timezone + self.since = datetime(1970, 1, 1, tzinfo=timezone.utc) # Default to epoch start (UTC)release_notes_generator/utils/pull_request_utils.py (2)
32-53: Body parsing misses cross-repo references (owner/repo#123).This blocks the PR objective (#172). Support optional
owner/repobefore#numberand construct IDs accordingly.Apply this diff:
-def extract_issue_numbers_from_body(pr: PullRequest, repository: Repository) -> set[str]: +def extract_issue_numbers_from_body(pr: PullRequest, repository: Repository) -> set[str]: @@ - # Regex pattern to match issue numbers following keywords like "Close", "Fix", "Resolve" - regex_pattern = re.compile(r"([Cc]los(e|es|ed)|[Ff]ix(es|ed)?|[Rr]esolv(e|es|ed))\s*#\s*([0-9]+)") + # Matches: "close(s|d)?|fix(es|ed)?|resolve(s|d)?" followed by optional "owner/repo" and "#<num>" + # Examples: "closes #123", "Fixes org/repo#456" + regex_pattern = re.compile( + r"(?i)\b(?:close[sd]?|fix(?:es|ed)?|resolve[sd]?)\s+" + r"(?:(?P<owner>[A-Za-z0-9_.-]+)/(?P<name>[A-Za-z0-9_.-]+)\s*)?" + r"#\s*(?P<num>\d+)" + ) @@ - issue_matches = regex_pattern.findall(pr.body if pr.body else "") - - # Extract the issue numbers from the matches - issue_numbers = {int(match[-1]) for match in issue_matches} - issue_ids = {f"{repository.full_name}#{number}" for number in issue_numbers} - - return issue_ids + text = pr.body or "" + issue_ids: set[str] = set() + for m in regex_pattern.finditer(text): + owner = m.group("owner") + name = m.group("name") + num = m.group("num") + if owner and name: + issue_ids.add(f"{owner}/{name}#{num}") + else: + issue_ids.add(f"{repository.full_name}#{num}") + return issue_idsPlease add tests for cross-repo mentions like "Fixes other/repo#42".
72-82: Avoid disabling TLS verification in requests.
verify=Falseweakens security. Remove it; default verification should be used. Update tests accordingly.Apply this diff:
- response = requests.post(github_api_url, json={"query": query}, headers=headers, verify=False, timeout=10) + response = requests.post(github_api_url, json={"query": query}, headers=headers, timeout=10)
🧹 Nitpick comments (8)
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (1)
73-77: External sub-issue processing needs clarificationThe TODO comment indicates uncertainty about whether external sub-issues should be treated as issues or sub-issues. This design decision should be finalized.
The TODO suggests this is an unresolved design decision. Would you like me to help analyze the implications of each approach and suggest a solution, or open an issue to track this decision?
release_notes_generator/model/mined_data.py (1)
23-23:@dataclasswithout fields is misleading; remove or define fields.You define all attributes in a custom init, so the decorator brings little value and can confuse readers/tools.
Apply this diff:
-from dataclasses import dataclass-@dataclass class MinedData:Alternatively, convert attributes to real dataclass fields and let dataclass generate init.
Also applies to: 37-41
release_notes_generator/miner.py (3)
50-62: Avoid duplicate ActionInputs calls; include repo in logs.Minor cleanup for clarity and fewer lookups.
Apply this diff:
- def check_repository_exists(self) -> bool: + def check_repository_exists(self) -> bool: """ Checks if the specified GitHub repository exists. Returns: bool: True if the repository exists, False otherwise. """ - repo: Repository = self._safe_call(self.github_instance.get_repo)(ActionInputs.get_github_repository()) + repo_name = ActionInputs.get_github_repository() + repo: Repository = self._safe_call(self.github_instance.get_repo)(repo_name) if repo is None: - logger.error("Repository not found: %s", ActionInputs.get_github_repository()) + logger.error("Repository not found: %s", repo_name) return False return True
68-74: Raising on missing repo is good; include repository in the exception.This aids debugging. Also ensure callers now handle the ValueError (behavior change).
Apply this diff:
- repo: Repository = self._safe_call(self.github_instance.get_repo)(ActionInputs.get_github_repository()) - if repo is None: - logger.error("Repository not found: %s", ActionInputs.get_github_repository()) - raise ValueError("Repository not found") + repo_name = ActionInputs.get_github_repository() + repo: Repository = self._safe_call(self.github_instance.get_repo)(repo_name) + if repo is None: + logger.error("Repository not found: %s", repo_name) + raise ValueError(f"Repository not found: {repo_name}")Please verify all call sites/tests expect and handle this exception.
78-81: Filter commits by release timestamp to avoid huge fetch.Fetching all commits is expensive; when release exists, pass since=data.since.
Apply this diff:
- # pulls and commits, and then reduce them by the latest release since time + # pulls and commits; filter commits by the latest release timestamp when available data.pull_requests = list(self._safe_call(data.repository.get_pulls)(state=PullRequestRecord.PR_STATE_CLOSED)) - data.commits = list(self._safe_call(data.repository.get_commits)()) + if data.release is not None and data.since: + data.commits = list(self._safe_call(data.repository.get_commits)(since=data.since)) + else: + data.commits = list(self._safe_call(data.repository.get_commits)())tests/release_notes/record/factory/test_default_record_factory.py (1)
340-341: Fix linter warning and type for unused argument.Use int type and underscore to signal it’s intentionally unused.
Apply this diff:
-def mock_get_issues_for_pr_no_issues(pull_number: str) -> list[str]: - return [] +def mock_get_issues_for_pr_no_issues(_pull_number: int) -> list[str]: + return []tests/utils/test_pull_request_utils.py (2)
126-134: Update test: stop assertingverify=False.After removing
verify=False, this assertion should be dropped.Apply this diff:
- assert captured["verify"] is False
60-65: Add a test for cross-repo mentions in PR body.To cover the new parsing, include an assertion for "owner/repo#num".
Add this test:
def test_extract_issue_numbers_from_body_cross_repo(mocker, mock_repo): mock_pr = mocker.Mock(spec=PullRequest) mock_pr.body = "This PR closes other/repo#42 and fixes #7." issue_ids = extract_issue_numbers_from_body(mock_pr, mock_repo) assert issue_ids == {'other/repo#42', 'org/repo#7'}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
release_notes_generator/builder/builder.py(1 hunks)release_notes_generator/chapters/base_chapters.py(1 hunks)release_notes_generator/chapters/custom_chapters.py(1 hunks)release_notes_generator/chapters/service_chapters.py(1 hunks)release_notes_generator/filter.py(1 hunks)release_notes_generator/generator.py(2 hunks)release_notes_generator/miner.py(1 hunks)release_notes_generator/model/hierarchy_issue_record.py(1 hunks)release_notes_generator/model/mined_data.py(2 hunks)release_notes_generator/record/factory/default_record_factory.py(4 hunks)release_notes_generator/record/factory/issue_hierarchy_record_factory.py(6 hunks)release_notes_generator/record/factory/record_factory.py(1 hunks)release_notes_generator/utils/pull_request_utils.py(3 hunks)tests/conftest.py(15 hunks)tests/release_notes/record/factory/test_default_record_factory.py(17 hunks)tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py(5 hunks)tests/test_miner.py(1 hunks)tests/utils/test_pull_request_utils.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (17)
release_notes_generator/builder/builder.py (2)
release_notes_generator/model/record.py (1)
Record(29-190)tests/release_notes/chapters/test_base_chapters.py (1)
Record(24-25)
release_notes_generator/record/factory/record_factory.py (3)
release_notes_generator/record/factory/default_record_factory.py (1)
generate(81-155)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (1)
generate(57-110)release_notes_generator/generator.py (1)
generate(69-108)
tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py (3)
tests/conftest.py (1)
mock_repo(94-97)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (1)
IssueHierarchyRecordFactory(44-299)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)
release_notes_generator/chapters/base_chapters.py (4)
release_notes_generator/chapters/custom_chapters.py (1)
populate(42-73)release_notes_generator/chapters/service_chapters.py (1)
populate(96-141)tests/release_notes/chapters/test_base_chapters.py (2)
populate(30-31)Record(24-25)release_notes_generator/model/record.py (1)
Record(29-190)
tests/test_miner.py (1)
tests/conftest.py (1)
mock_repo(94-97)
release_notes_generator/utils/pull_request_utils.py (1)
release_notes_generator/action_inputs.py (3)
ActionInputs(64-513)get_github_owner(81-94)get_github_repo_name(97-110)
tests/conftest.py (1)
release_notes_generator/model/mined_data.py (1)
MinedData(38-56)
release_notes_generator/filter.py (1)
release_notes_generator/model/mined_data.py (1)
MinedData(38-56)
release_notes_generator/chapters/service_chapters.py (4)
release_notes_generator/chapters/base_chapters.py (1)
populate(107-112)release_notes_generator/chapters/custom_chapters.py (1)
populate(42-73)tests/release_notes/chapters/test_base_chapters.py (2)
populate(30-31)Record(24-25)release_notes_generator/model/record.py (1)
Record(29-190)
release_notes_generator/miner.py (2)
release_notes_generator/action_inputs.py (1)
get_github_repository(113-117)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)
release_notes_generator/generator.py (6)
release_notes_generator/miner.py (2)
check_repository_exists(50-61)mine_data(63-88)release_notes_generator/model/mined_data.py (1)
is_empty(49-56)release_notes_generator/model/record.py (1)
Record(29-190)release_notes_generator/record/factory/default_record_factory.py (1)
generate(81-155)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (1)
generate(57-110)release_notes_generator/record/factory/record_factory.py (1)
generate(35-42)
tests/utils/test_pull_request_utils.py (2)
tests/conftest.py (1)
mock_repo(94-97)release_notes_generator/utils/pull_request_utils.py (1)
extract_issue_numbers_from_body(32-52)
tests/release_notes/record/factory/test_default_record_factory.py (3)
tests/conftest.py (3)
mock_get_issues_for_pr(57-60)mock_repo(94-97)wrapper(50-53)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/record/factory/default_record_factory.py (2)
DefaultRecordFactory(45-199)generate(81-155)
release_notes_generator/chapters/custom_chapters.py (4)
release_notes_generator/chapters/base_chapters.py (1)
populate(107-112)release_notes_generator/chapters/service_chapters.py (1)
populate(96-141)tests/release_notes/chapters/test_base_chapters.py (2)
populate(30-31)Record(24-25)release_notes_generator/model/record.py (1)
Record(29-190)
release_notes_generator/model/hierarchy_issue_record.py (1)
release_notes_generator/model/sub_issue_record.py (1)
SubIssueRecord(15-36)
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (6)
release_notes_generator/record/factory/default_record_factory.py (2)
generate(81-155)get_id(57-67)release_notes_generator/record/factory/record_factory.py (1)
generate(35-42)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/model/sub_issue_record.py (2)
issue(27-34)SubIssueRecord(15-36)release_notes_generator/utils/pull_request_utils.py (1)
extract_issue_numbers_from_body(32-52)release_notes_generator/model/hierarchy_issue_record.py (3)
HierarchyIssueRecord(16-154)sub_issues(47-51)sub_hierarchy_issues(54-58)
release_notes_generator/record/factory/default_record_factory.py (6)
release_notes_generator/model/issue_record.py (4)
issue(63-68)register_pull_request(192-199)IssueRecord(16-328)get_labels(81-83)release_notes_generator/model/pull_request_record.py (3)
pull_request(70-75)get_labels(107-109)PullRequestRecord(15-290)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (2)
generate(57-110)_create_record_for_issue(234-239)release_notes_generator/record/factory/record_factory.py (1)
generate(35-42)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/utils/pull_request_utils.py (2)
extract_issue_numbers_from_body(32-52)get_issues_for_pr(56-81)
🪛 Ruff (0.13.1)
release_notes_generator/miner.py
71-71: Avoid specifying long messages outside the exception class
(TRY003)
tests/release_notes/record/factory/test_default_record_factory.py
340-340: Unused function argument: pull_number
(ARG001)
release_notes_generator/record/factory/default_record_factory.py
90-90: Unused function argument: skip_rec
(ARG001)
🔇 Additional comments (18)
release_notes_generator/chapters/service_chapters.py (1)
96-96: Good type narrowing for record keysThe signature change from
dict[int | str, Record]todict[str, Record]aligns well with the project's migration to string-based IDs.release_notes_generator/model/hierarchy_issue_record.py (1)
26-27: Consistent migration to string-based keysThe internal dictionary types have been correctly updated to use string keys, aligning with the project-wide shift to repository-scoped IDs.
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (3)
71-72: Good addition of temporal filtering for sub-issuesThe optional
sinceparameter allows proper filtering of sub-issues based on the release timeframe, preventing inclusion of issues closed in previous releases.
164-193: Excellent cross-repo sub-issue detection and handlingThe logic correctly:
- Detects sub-issues from different repositories
- Registers them for later processing
- Filters out sub-issues closed before the release date
- Skips open sub-issues
This aligns perfectly with the PR objectives for detecting and appending cross-repo issues.
278-283: Good defensive programming for hierarchy buildingThe error handling for unexpected record types in the hierarchy and the prevention of infinite recursion by removing unresolved mappings shows good defensive programming practices.
tests/test_miner.py (1)
42-42: LGTM - Correct repository context integration.The change from
super().__init__()tosuper().__init__(mock_repo)correctly aligns with the updatedMinedDataconstructor that now requires a repository parameter. This ensures proper initialization with repository context.release_notes_generator/generator.py (2)
79-81: LGTM - Proper early validation with repository existence check.Adding the repository existence check early in the flow prevents unnecessary processing when the repository cannot be found, improving both performance and error handling. The early return of
Noneprovides a clear indication of repository unavailability.
100-100: LGTM - Type narrowing aligns with codebase-wide string key migration.The type annotation change from
dict[int | str, Record]todict[str, Record]correctly reflects the migration to repository-context record IDs (e.g., "org/repo#123"). This follows modern Python type annotation practices wheredict[str, Record]is preferred overtyping.Dictand ensures type consistency across the record generation pipeline.release_notes_generator/chapters/custom_chapters.py (1)
42-42: LGTM - Type signature consistency maintained.The parameter type change from
dict[int | str, Record]todict[str, Record]maintains consistency with the broader codebase migration to string-based record keys. The method implementation remains unchanged, ensuring functional compatibility while improving type safety.release_notes_generator/builder/builder.py (1)
41-41: LGTM - Type consistency across builder initialization.The narrowed type annotation from
dict[int | str, Record]todict[str, Record]aligns with the project-wide standardization on string-based record keys. This ensures type consistency between the builder and the record factories that generate these collections.release_notes_generator/filter.py (1)
64-64: LGTM - Proper repository-aware MinedData construction.The change from creating an empty
MinedData()and assigning the repository afterward to constructingMinedData(data.repository)directly aligns with the updated constructor signature. This ensures proper initialization with repository context from the start.release_notes_generator/record/factory/record_factory.py (1)
35-35: LGTM - Abstract contract updated for string-based record IDs.The changes to the abstract method signature and docstring correctly reflect the migration to string-based record keys (e.g., "org_name/repo_name#number"). This provides clear guidance to implementers and ensures type consistency across all record factory implementations.
Also applies to: 41-41
tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py (2)
32-32: LGTM - Test updated for repository-context initialization.The addition of
mock_repoparameter andMinedData(mock_repo)construction aligns with the updatedMinedDataconstructor requirements. This ensures tests properly initialize data with repository context.Also applies to: 35-35
62-293: LGTM - Comprehensive test updates for string-based record keys.The extensive updates throughout the test file correctly reflect the migration from numeric keys to string-based repository-context keys (e.g.,
'org/repo#121'instead of121). All assertions, dictionary access patterns, and sub-structure references have been consistently updated while maintaining the same test logic and coverage.Key observations:
- All record dictionary keys updated to format
'org/repo#<number>'- Sub-issue and hierarchy access patterns updated accordingly
- PR record keys remain as simple string numbers (
'123','124')- Commit records continue using SHA keys
- Test assertions verify the same functional behavior with new key format
The changes maintain test integrity while supporting the new record ID format.
tests/conftest.py (3)
149-149: LGTM - Repository context added to issue fixtures.Adding
issue.repository.full_name = "org/repo"andissue.closed_at = datetime.now()to the issue fixtures provides proper repository context and timestamp information required by the updated record generation logic. This ensures tests have access to repository metadata needed for string-based ID generation.Also applies to: 168-168, 187-188, 206-207, 224-225, 246-247, 265-266, 284-285, 305-306, 330-331, 354-355, 375-376, 399-400
424-425: LGTM - Pull request fixtures updated with proper closure timestamps.Adding
pull.closed_at = datetime.now()to pull request fixtures ensures consistency with the expected data structure for release note generation. This provides proper temporal context for filtering and processing operations.Also applies to: 445-446, 468-469, 488-489, 510-511, 530-531, 552-553
609-609: LGTM - Mined data fixtures updated for repository-context initialization.The addition of
mock_repoparameter andMinedData(mock_repo)construction ensures test data is properly initialized with repository context. This change supports the updatedMinedDataconstructor that requires repository information for proper record ID generation.Also applies to: 619-619
tests/release_notes/record/factory/test_default_record_factory.py (1)
170-175: LGTM: tests updated to string-based issue IDs.Switch to "org/repo#N" aligns with new key format.
release_notes_generator/record/factory/default_record_factory.py
Outdated
Show resolved
Hide resolved
| # dev note: here we expect that PR links to an issue in the same repository !!! | ||
| parent_issue_number = int(parent_issue_id.split("#")[1]) | ||
| parent_issue = ( |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify cross-repo issue handling logic
The comment indicates an expectation that PRs link to issues in the same repository, but the code attempts to handle cross-repo references. This could fail if the repository context differs.
Based on the PR objectives, this code should support cross-repo issues. Consider updating the comment to reflect the actual behavior or add proper handling for cross-repo scenarios.
🏁 Script executed:
#!/bin/bash
# Check how cross-repo issues are handled in the codebase
echo "Searching for cross-repo issue handling patterns..."
# Look for patterns that handle repository-scoped IDs
rg -n "repository\.full_name.*#" --type py
# Check for any validation or error handling for cross-repo references
ast-grep --pattern 'if $_.repository.full_name != $_.repository.full_name'Length of output: 317
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' release_notes_generator/record/factory/default_record_factory.pyLength of output: 8566
Support cross-repository issue references Refactor the logic at release_notes_generator/record/factory/default_record_factory.py:107–109 to parse the owner/repo prefix from parent_issue_id (e.g. split on “#” and then “/”) and fetch the issue via the corresponding repository’s get_issue call instead of always using data.repository.
There was a problem hiding this comment.
Check validity of this review note.
There was a problem hiding this comment.
🧩 Analysis chain
@miroslavpojer The CodeRabbit review comment is valid and identifies a real bug.
Here's my analysis:
The Problem:
The current code at lines 107-109 assumes all linked issues are in the same repository (data.repository), but the new ID format supports cross-repository references in the form "owner/repo#number". When a PR links to an issue from a different repository, the code will:
- Extract only the number part:
int(parent_issue_id.split("#")[1]) - Try to fetch it from the wrong repository:
data.repository.get_issue(parent_issue_number)
The Solution:
CodeRabbit's suggestion is correct. The code should:
- Parse the repository context from
parent_issue_id(e.g.,"owner/repo#123"→"owner/repo") - Get the appropriate repository instance
- Fetch the issue from the correct repository
Example Fix:
# Parse repository and issue number from cross-repo ID
if "#" in parent_issue_id:
repo_part, issue_num = parent_issue_id.split("#")
parent_issue_number = int(issue_num)
if "/" in repo_part: # Cross-repo reference
owner, repo_name = repo_part.split("/", 1)
target_repo = github_instance.get_repo(f"{owner}/{repo_name}")
parent_issue = self._safe_call(target_repo.get_issue)(parent_issue_number)
else: # Same repo reference
parent_issue = self._safe_call(data.repository.get_issue)(parent_issue_number)This aligns with the PR objectives to support cross-repository issue collection (#172).
🏁 Script executed:
#!/bin/bash
# Let's examine how parent_issue_id is formatted by looking at the get_id implementations
echo "=== Checking get_id implementations ==="
ast-grep --pattern 'def _(self, issue: Issue) -> str:
$$$'
echo -e "\n=== Checking how extract_issue_numbers_from_body works ==="
rg -n -A 10 -B 5 "def extract_issue_numbers_from_body" --type py
echo -e "\n=== Checking get_issues_for_pr return format ==="
rg -n -A 10 -B 5 "def get_issues_for_pr" --type py
echo -e "\n=== Looking for cross-repo handling patterns ==="
rg -n "repository\.full_name" --type py -C 3
Length of output: 15987
Based on my analysis of the code execution results, I can now provide a complete assessment of the CodeRabbit review comment's validity.
@miroslavpojer The CodeRabbit review comment is completely valid and correctly identifies a critical bug in the cross-repository issue handling logic.
Evidence from the Code Analysis:
1. ID Format Confirmation:
get_idfor issues returns:f"{issue.repository.full_name}#{issue.number}"(Line 74)extract_issue_numbers_from_bodyreturns:f"{repository.full_name}#{number}"(Line 50 in pull_request_utils.py)- So
parent_issue_idwill be in format"owner/repo#123"
2. The Bug:
The current code at lines 107-109:
parent_issue_number = int(parent_issue_id.split("#")[1])
parent_issue = self._safe_call(data.repository.get_issue)(parent_issue_number)- Extracts only
"123"from"owner/repo#123" - Always fetches from
data.repository(home repository) - Will fail for cross-repo references - tries to find issue #58 - Design PR template proposal #123 in wrong repository
3. Missing GitHub Instance:
The code lacks access to a GitHub instance needed to fetch repositories. The constructor only stores self._home_repository but would need the GitHub client to call github.get_repo("owner/repo").
Required Fix:
The code needs both:
- Repository parsing logic as CodeRabbit suggested
- GitHub instance access to fetch cross-repo repositories
# Need to store github instance in constructor
def __init__(self, github: Github, home_repository: Repository) -> None:
# ... existing code ...
self._github = github # Add this
# Then in the problematic section:
if "#" in parent_issue_id:
repo_part, issue_num = parent_issue_id.split("#")
parent_issue_number = int(issue_num)
if "/" in repo_part: # Cross-repo reference
target_repo = self._safe_call(self._github.get_repo)(repo_part)
parent_issue = self._safe_call(target_repo.get_issue)(parent_issue_number)
else: # Same repo reference
parent_issue = self._safe_call(data.repository.get_issue)(parent_issue_number)The CodeRabbit suggestion correctly identifies both the parsing logic needed and the fundamental architectural issue preventing cross-repo support.
🏁 Script executed:
#!/bin/bash
# Let's check the actual content of extract_issue_numbers_from_body and get_issues_for_pr to see the return format
echo "=== Full extract_issue_numbers_from_body implementation ==="
sed -n '32,60p' release_notes_generator/utils/pull_request_utils.py
echo -e "\n=== Full get_issues_for_pr implementation ==="
sed -n '56,85p' release_notes_generator/utils/pull_request_utils.py
echo -e "\n=== Check if there's any Github instance access in DefaultRecordFactory ==="
rg -n "_github|github" release_notes_generator/record/factory/default_record_factory.py -C 2
Length of output: 3272
Perfect! Now I have the complete picture. Let me provide the final assessment:
@miroslavpojer The CodeRabbit review comment is absolutely valid and identifies a critical bug, but there's an important nuance about the GitHub instance availability.
Complete Analysis:
1. The Bug is Real:
- Both
extract_issue_numbers_from_bodyandget_issues_for_prreturn IDs in format"owner/repo#123" - Current code extracts only
"123"and fetches from wrong repository - This will definitely fail for cross-repository references
2. GitHub Instance Access:
Looking at the constructor, I can see that while the Github instance is passed in, it's not stored as a class attribute:
def __init__(self, github: Github, home_repository: Repository) -> None:
rate_limiter = GithubRateLimiter(github)
self._safe_call = safe_call_decorator(rate_limiter) # Only used for rate limiting
self._home_repository = home_repository
# github instance is NOT stored!3. The Fix Needs Two Parts:
Part A - Store Github instance:
def __init__(self, github: Github, home_repository: Repository) -> None:
rate_limiter = GithubRateLimiter(github)
self._safe_call = safe_call_decorator(rate_limiter)
self._home_repository = home_repository
self._github = github # Add this linePart B - Implement CodeRabbit's parsing suggestion:
# Replace lines 107-109 with:
if "#" in parent_issue_id:
repo_part, issue_num = parent_issue_id.split("#")
parent_issue_number = int(issue_num)
if "/" in repo_part: # Cross-repo reference "owner/repo#123"
target_repo = self._safe_call(self._github.get_repo)(repo_part)
parent_issue = self._safe_call(target_repo.get_issue)(parent_issue_number)
else: # Same repo reference (shouldn't happen with current ID format, but defensive)
parent_issue = self._safe_call(data.repository.get_issue)(parent_issue_number)Verdict: CodeRabbit's review is spot-on - it correctly identifies the parsing issue that will cause the feature to fail. The implementation currently cannot handle the cross-repo IDs that the rest of the system is designed to produce.
🏁 Script executed:
#!/bin/bash
# Let's check if the Github instance is stored in the DefaultRecordFactory
echo "=== Check the full constructor and any github storage ==="
sed -n '51,60p' release_notes_generator/record/factory/default_record_factory.py
echo -e "\n=== Check if there's any self._github usage in the class ==="
rg -n "self\._github" release_notes_generator/record/factory/default_record_factory.py
echo -e "\n=== Let's see what's available in the current implementation around line 120-130 ==="
sed -n '115,135p' release_notes_generator/record/factory/default_record_factory.py
Length of output: 1696
Perfect! The script output confirms the exact issue CodeRabbit identified.
@miroslavpojer The CodeRabbit review comment is 100% valid and correctly identifies a critical bug.
Confirmed Evidence:
The Exact Problematic Code (Lines 120-125):
# dev note: here we expect that PR links to an issue in the same repository !!!
parent_issue_number = int(parent_issue_id.split("#")[1]) # ❌ Only extracts "123" from "owner/repo#123"
parent_issue = (
self._safe_call(data.repository.get_issue)(parent_issue_number) if data.repository else None # ❌ Always uses home repository
)The Problems:
- Line 121: Strips repository context, only keeping the number
- Line 122-123: Always fetches from
data.repository(home repo) - Missing Infrastructure: No stored
Githubinstance to fetch other repositories - Developer Acknowledgment: The comment on line 120 shows this limitation is known
CodeRabbit's Suggested Fix is Exactly Right:
The suggested parsing of owner/repo prefix and using the appropriate repository's get_issue method is the correct solution. However, it also requires:
- Store Github instance in constructor:
def __init__(self, github: Github, home_repository: Repository) -> None:
# ... existing code ...
self._github = github # Add this- Implement the parsing logic CodeRabbit suggested:
# Parse repository and issue number from cross-repo ID
repo_part, issue_num = parent_issue_id.split("#")
parent_issue_number = int(issue_num)
if "/" in repo_part: # Cross-repo reference
target_repo = self._safe_call(self._github.get_repo)(repo_part)
parent_issue = self._safe_call(target_repo.get_issue)(parent_issue_number)
else: # Home repo reference
parent_issue = self._safe_call(data.repository.get_issue)(parent_issue_number)Conclusion: CodeRabbit's review is spot-on and identifies a feature-breaking bug that prevents the cross-repo functionality from working as designed.
✏️ Learnings added
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#173
File: release_notes_generator/record/factory/default_record_factory.py:115-117
Timestamp: 2025-09-28T13:20:42.353Z
Learning: In the DefaultRecordFactory class, the Github instance passed to __init__ is not stored as a class attribute, only used to create a rate limiter. For cross-repo issue handling, the Github instance needs to be stored to enable fetching issues from different repositories via github.get_repo().
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@miroslavpojer I'll add a TODO comment to address the cross-repo issue handling bug identified by CodeRabbit.
# TODO: Fix cross-repo issue handling bug (Issue #172)
# Currently this code assumes all linked issues are in the same repository, but parent_issue_id
# can contain cross-repo references in format "owner/repo#123". The current implementation:
# 1. Only extracts the number part (123) via split("#")[1]
# 2. Always fetches from data.repository (home repository)
# This fails for cross-repo references. Need to:
# 1. Store Github instance in constructor: self._github = github
# 2. Parse repository from parent_issue_id: repo_part, issue_num = parent_issue_id.split("#")
# 3. Use self._github.get_repo(repo_part) for cross-repo cases
# 4. Fetch from appropriate repository instead of always using data.repository
# dev note: here we expect that PR links to an issue in the same repository !!!
parent_issue_number = int(parent_issue_id.split("#")[1])
parent_issue = (
self._safe_call(data.repository.get_issue)(parent_issue_number) if data.repository else None
)This TODO comment should be placed right before line 120 where the problematic code begins, clearly documenting the cross-repo issue handling bug and the required fix.
🧠 Learnings used
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#173
File: release_notes_generator/record/factory/default_record_factory.py:109-126
Timestamp: 2025-09-28T14:13:38.954Z
Learning: In release_notes_generator/record/factory/default_record_factory.py, there is a cross-repo fetch bug where parent_issue_id in format "owner/repo#number" is fetched from the wrong repository because the code only queries data.repository instead of parsing the owner/repo from the ID and using the GitHub client to fetch from the correct repository.
release_notes_generator/record/factory/issue_hierarchy_record_factory.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
release_notes_generator/model/pull_request_record.py (1)
33-34: Record IDs must carry repository contextLine 33 still returns the bare PR number, but the PR objective requires repository-scoped IDs (
org/repo#number). Without that, cross-repo deduping and lookups will fail. Please include the repo full name (falling back safely if the API doesn’t provide it).@property def record_id(self) -> int | str: - return self._pull_request.number + repo_full_name = getattr(getattr(self._pull_request.base, "repo", None), "full_name", None) + if not repo_full_name: + repo_full_name = getattr(getattr(self._pull_request.head, "repo", None), "full_name", None) + if not repo_full_name: + return f"#{self._pull_request.number}" + return f"{repo_full_name}#{self._pull_request.number}"release_notes_generator/record/factory/default_record_factory.py (1)
132-149: Replace remaining directpull.numberkey usage withget_id(pull)
release_notes_generator/record/factory/issue_hierarchy_record_factory.py:178
– Changeself._records[str(pull.number)] = pr_recto
self._records[self.get_id(pull)] = pr_recAlso update the corresponding
logger.debugto use the new ID placeholder.
🧹 Nitpick comments (8)
release_notes_generator/chapters/custom_chapters.py (1)
46-47: Bring the docstring in sync with the new key type.The signature now mandates
dict[str, Record], but the docstring still states the keys are integers. Please update the comment so the public contract is accurate.release_notes_generator/record/factory/default_record_factory.py (4)
69-76: Ensure get_id also handles SubIssue to avoid NotImplementedError at call sites.
IssueHierarchyRecordFactorycallsself.get_id(si)forSubIssue, which may not be a subclass ofIssue. Add a SubIssue registration.Apply this diff, and add the import shown below:
@get_id.register def _(self, issue: Issue) -> str: # delegate to a cached, hashable-only helper return self._issue_id(issue.repository.full_name, issue.number) +@get_id.register +def _(self, sub_issue: "SubIssue") -> str: + return self._issue_id(sub_issue.repository.full_name, sub_issue.number)And add this import near the other GitHub imports:
from github.Issue import SubIssue # at top with other importsIf
SubIssueis a subclass ofIssue, this is redundant but harmless. Otherwise it prevents runtime errors.
To confirm inheritance, I can query the web for PyGithub’sSubIssueclass details if you want.
87-94: Docstring: clarify key format in returned records.Keys are repo-scoped IDs (and commit SHAs), not “issue or pull request number”.
Apply this diff:
- Returns: - dict[str, Record]: A dictionary of records where the key is the issue or pull request number. + Returns: + dict[str, Record]: A dictionary keyed by 'owner/repo#number' for issues/PRs and commit SHA for commits.
96-96: Remove unused parameter ‘skip_rec’.It’s dead and flagged by Ruff (ARG001).
Apply this diff:
- def register_pull_request(pull: PullRequest, skip_rec: bool) -> None: + def register_pull_request(pull: PullRequest) -> None:
149-149: Update callsite after removing unused param.Apply this diff:
- register_pull_request(pull, skip_record) + register_pull_request(pull)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (3)
64-65: Docstring: align described key format with repo-scoped IDs and SHAs.Apply this diff:
- Returns: - dict[str, Record]: A dictionary of records where the key is the issue id or pull request number. + Returns: + dict[str, Record]: Records keyed by 'owner/repo#number' for issues/PRs and commit SHA for commits.
81-81: Remove unnecessary f-string.No placeholders used.
Apply this diff:
- logger.debug(f"Looking for hierarchical issue among sub-issues...") + logger.debug("Looking for hierarchical issue among sub-issues...")
181-187: Wrap API call to get sub-issues with the rate-limiter.Consistent with other API calls and reduces risk of rate limit errors.
Apply this diff:
- sub_issues = list(issue.get_sub_issues()) + sub_issues = list(self._safe_call(issue.get_sub_issues)())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
release_notes_generator/chapters/custom_chapters.py(2 hunks)release_notes_generator/filter.py(2 hunks)release_notes_generator/model/commit_record.py(1 hunks)release_notes_generator/model/hierarchy_issue_record.py(3 hunks)release_notes_generator/model/issue_record.py(1 hunks)release_notes_generator/model/pull_request_record.py(1 hunks)release_notes_generator/model/record.py(1 hunks)release_notes_generator/record/factory/default_record_factory.py(4 hunks)release_notes_generator/record/factory/issue_hierarchy_record_factory.py(6 hunks)tests/conftest.py(16 hunks)tests/release_notes/builder/test_release_notes_builder.py(0 hunks)tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py(5 hunks)
💤 Files with no reviewable changes (1)
- tests/release_notes/builder/test_release_notes_builder.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/conftest.py
🧰 Additional context used
🧬 Code graph analysis (10)
release_notes_generator/model/record.py (3)
release_notes_generator/model/commit_record.py (1)
contains_change_increment(68-69)release_notes_generator/model/issue_record.py (1)
contains_change_increment(125-126)release_notes_generator/model/pull_request_record.py (1)
contains_change_increment(131-132)
release_notes_generator/model/commit_record.py (3)
release_notes_generator/model/issue_record.py (1)
contains_change_increment(125-126)release_notes_generator/model/pull_request_record.py (1)
contains_change_increment(131-132)release_notes_generator/model/record.py (1)
contains_change_increment(121-127)
release_notes_generator/model/pull_request_record.py (3)
release_notes_generator/model/commit_record.py (1)
contains_change_increment(68-69)release_notes_generator/model/issue_record.py (1)
contains_change_increment(125-126)release_notes_generator/model/record.py (1)
contains_change_increment(121-127)
release_notes_generator/model/issue_record.py (4)
release_notes_generator/model/commit_record.py (1)
contains_change_increment(68-69)release_notes_generator/model/pull_request_record.py (1)
contains_change_increment(131-132)release_notes_generator/model/record.py (1)
contains_change_increment(121-127)release_notes_generator/model/hierarchy_issue_record.py (1)
pull_requests_count(60-69)
release_notes_generator/record/factory/default_record_factory.py (7)
release_notes_generator/model/record.py (3)
Record(29-199)get_labels(130-135)skip(54-56)release_notes_generator/model/issue_record.py (4)
issue(63-68)register_pull_request(195-202)IssueRecord(16-331)get_labels(81-83)release_notes_generator/model/pull_request_record.py (3)
pull_request(70-75)get_labels(107-109)PullRequestRecord(15-293)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (2)
generate(58-134)_create_record_for_issue(273-278)release_notes_generator/record/factory/record_factory.py (1)
generate(35-42)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/utils/pull_request_utils.py (1)
extract_issue_numbers_from_body(32-52)
release_notes_generator/chapters/custom_chapters.py (8)
release_notes_generator/chapters/base_chapters.py (1)
populate(107-112)release_notes_generator/chapters/service_chapters.py (1)
populate(96-141)tests/release_notes/chapters/test_base_chapters.py (2)
populate(30-31)Record(24-25)release_notes_generator/model/record.py (4)
Record(29-199)record_id(72-77)is_present_in_chapters(45-51)contains_change_increment(121-127)release_notes_generator/model/commit_record.py (2)
record_id(27-28)contains_change_increment(68-69)release_notes_generator/model/issue_record.py (2)
record_id(43-44)contains_change_increment(125-126)release_notes_generator/model/pull_request_record.py (2)
record_id(33-34)contains_change_increment(131-132)tests/release_notes/model/test_record.py (1)
record_id(30-31)
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (3)
release_notes_generator/record/factory/default_record_factory.py (2)
generate(87-161)get_id(57-67)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/utils/pull_request_utils.py (1)
extract_issue_numbers_from_body(32-52)
release_notes_generator/filter.py (1)
release_notes_generator/model/mined_data.py (1)
MinedData(38-56)
release_notes_generator/model/hierarchy_issue_record.py (5)
release_notes_generator/model/sub_issue_record.py (1)
SubIssueRecord(15-36)release_notes_generator/model/commit_record.py (2)
contains_change_increment(68-69)to_chapter_row(54-66)release_notes_generator/model/issue_record.py (2)
contains_change_increment(125-126)to_chapter_row(100-123)release_notes_generator/model/pull_request_record.py (2)
contains_change_increment(131-132)to_chapter_row(111-129)release_notes_generator/model/record.py (2)
contains_change_increment(121-127)to_chapter_row(109-118)
tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py (2)
tests/conftest.py (1)
mock_repo(94-97)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)
🪛 Ruff (0.13.1)
release_notes_generator/record/factory/default_record_factory.py
96-96: Unused function argument: skip_rec
(ARG001)
release_notes_generator/record/factory/issue_hierarchy_record_factory.py
81-81: f-string without any placeholders
Remove extraneous f prefix
(F541)
🪛 GitHub Actions: Build and Test
release_notes_generator/chapters/custom_chapters.py
[error] 1-1: Black formatting would reformat this file. Run 'black' to reformat.
release_notes_generator/record/factory/issue_hierarchy_record_factory.py
[error] 1-1: Black formatting would reformat this file. Run 'black' to reformat.
release_notes_generator/filter.py
[error] 1-1: Black formatting would reformat this file. Run 'black' to reformat.
tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py
[error] 1-1: Hierarchy record factory produced fewer sub-issues than expected (expected 2, got 1).
🔇 Additional comments (6)
release_notes_generator/model/issue_record.py (1)
125-126: Good guard on IssueRecord change detection.The predicate lines up with the new contract on
Record.contains_change_incrementand keeps IssueRecords consistent with the rest of the model layer.release_notes_generator/model/commit_record.py (1)
68-69: Commit records always represent change increments.Returning
Truehere keepsCommitRecordcompliant with the base-class contract without adding unnecessary conditionals.release_notes_generator/record/factory/issue_hierarchy_record_factory.py (3)
95-101: get_id(SubIssue) must resolve properly here.This relies on
DefaultRecordFactory.get_idhandlingSubIssue. Ensure the registration exists per suggested change in DefaultRecordFactory.
174-179: Store standalone PR records under repo-scoped ID, not just the number.This avoids key collisions and aligns with the new ID scheme.
Apply this diff:
- if not attached_any: - pr_rec = PullRequestRecord(pull, pull_labels, skip_record) + if not attached_any: + pr_rec = PullRequestRecord(pull, pull_labels, skip_record) for c in related_commits: # register commits to the PR record pr_rec.register_commit(c) - self._records[str(pull.number)] = pr_rec - logger.debug("Created record for PR %d: %s", pull.number, pull.title) + pid = self.get_id(pull) + self._records[pid] = pr_rec + logger.debug("Created record for PR %s: %s", pid, pull.title)
1-341: Formatting: Black reformat required by pipeline.
Black isn’t available in this environment; please install Black locally and run:black release_notes_generator/record/factory/issue_hierarchy_record_factory.pyrelease_notes_generator/record/factory/default_record_factory.py (1)
79-82: PR IDs must be repository-scoped ("owner/repo#number"), not just the number.This inconsistency will cause collisions and failed lookups when issues and PRs share numbers or when multiple repos are processed.
Apply this diff:
- def _(self, pull_request: PullRequest) -> str: - return f"{pull_request.number}" + def _(self, pull_request: PullRequest) -> str: + # Prefer the PR's base repo (where the PR lives) + return f"{pull_request.base.repo.full_name}#{pull_request.number}"
| if not sub_issue.contains_change_increment(): | ||
| continue # skip sub-issues without change increment |
There was a problem hiding this comment.
Don’t drop cross-repo sub-issues when rendering.
With the new cross-repo detection, sub-issues from external repositories generally won’t carry local PRs or commits, so contains_change_increment() will return False. This guard stops those entries from ever reaching the release notes, defeating the stated objective of surfacing cross-repo sub-issues. Please relax or rethink this filter so externally mined sub-issues still render, even when they lack locally tracked PRs.
🤖 Prompt for AI Agents
In release_notes_generator/model/hierarchy_issue_record.py around lines 133 to
134: the current guard skips any sub-issue that returns False for
contains_change_increment(), which unintentionally filters out cross-repo
sub-issues that have no local PRs/commits. Update the conditional to allow
cross-repo sub-issues to pass through (e.g., only continue when
contains_change_increment() is False AND the sub_issue is not cross-repo or not
externally sourced), i.e., relax the filter to let sub_issue instances flagged
as cross-repo/external render even if they lack local change increments; add a
unit test asserting external sub-issues are included and adjust any
docstring/comments to reflect the new behavior.
There was a problem hiding this comment.
Check validity of this review note.
There was a problem hiding this comment.
🧩 Analysis chain
Based on my analysis of the codebase, I can now provide a comprehensive response about the validity of CodeRabbit's review comment.
@miroslavpojer The CodeRabbit review comment appears to be valid and identifies a legitimate issue. Here's my analysis:
Validity Assessment: ✅ VALID
Key findings:
-
SubIssueRecord.contains_change_increment() logic: The method returns
self.pull_requests_count() > 0, meaning it only returns True if the sub-issue has associated pull requests. -
Cross-repo sub-issues behavior: From the factory code, external/cross-repo sub-issues are created as
SubIssueRecordinstances but they likely won't have local pull requests associated with them since they're from external repositories. -
Filter consequence: The new guard
if not sub_issue.contains_change_increment(): continuewill indeed skip any sub-issue without local PRs, which would include cross-repo sub-issues that are the main objective of this PR (Add collection of cross repo sub-issues #172). -
Conflicts with PR objective: PR objective Add collection of cross repo sub-issues #172 explicitly states "Recognize and collect cross-repo sub-issues that are linked as sub-issues" and "Add logic to detect and save these cross-repo sub-issues so they can be appended to records." The current filter defeats this purpose.
Recommendation
The review suggestion is sound. You should modify the conditional to allow cross-repo sub-issues to render even when they lack local change increments. Consider one of these approaches:
- Add a flag to identify cross-repo sub-issues and exempt them from the
contains_change_increment()check - Modify the contains_change_increment() logic for cross-repo sub-issues to return True by default
- Add repository context to the filtering logic to distinguish between local and cross-repo sub-issues
This is a critical issue that would prevent the core functionality introduced in this PR from working as intended.
🏁 Script executed:
#!/bin/bash
# First, let's examine the contains_change_increment method implementations across different record types
echo "=== Examining contains_change_increment implementations ==="
ast-grep --pattern 'def contains_change_increment($$$) -> bool:
$$$'
echo -e "\n=== Looking for SubIssueRecord implementation specifically ==="
fd -t f -n sub_issue_record.py --exec cat {}
echo -e "\n=== Checking how cross-repo sub-issues are handled in factories ==="
rg -nP -C5 'external.*sub.*issue|cross.*repo.*sub'
echo -e "\n=== Looking for SubIssue type usage ==="
rg -nP -C3 'SubIssue.*import|from.*SubIssue'
Length of output: 10207
🏁 Script executed:
#!/bin/bash
# Let's examine the SubIssueRecord class implementation
echo "=== Full SubIssueRecord class ==="
fd -t f -n sub_issue_record.py --exec cat {}
echo -e "\n=== Looking at how sub-issues are created in factories ==="
rg -nP -C8 'SubIssueRecord.*\('
echo -e "\n=== Checking factory logic for external sub-issues ==="
rg -nP -C10 'external_sub_issues|__external_sub_issues'
Length of output: 13624
🏁 Script executed:
#!/bin/bash
# Let's look at the issue hierarchy factory implementation for external sub-issues
echo "=== External sub-issues handling in IssueHierarchyRecordFactory ==="
rg -nP -A15 -B5 'external_sub_issues.*append|SubIssue.*external'
echo -e "\n=== How are SubIssueRecord instances created? ==="
ast-grep --pattern 'SubIssueRecord($$$)'
echo -e "\n=== Let's see the SubIssue import and usage ==="
rg -nP -C5 'from github.*SubIssue'
Length of output: 5242
🏁 Script executed:
#!/bin/bash
# Let's examine the complete SubIssueRecord class to understand contains_change_increment
cat release_notes_generator/model/sub_issue_record.py
echo -e "\n=== Now let's look at how external sub-issues are registered ==="
rg -nP -A20 'register.*external.*sub|external.*sub.*register' release_notes_generator/record/factory/issue_hierarchy_record_factory.py
Length of output: 2085
🏁 Script executed:
#!/bin/bash
# Let's look at the specific method that processes external sub-issues
echo "=== Looking at _register_external_sub_issues method ==="
ast-grep --pattern '_register_external_sub_issues($$$) {
$$$
}'
echo -e "\n=== Let's examine the complete flow in issue_hierarchy_record_factory ==="
rg -nP -A10 -B5 'def.*register.*external' release_notes_generator/record/factory/issue_hierarchy_record_factory.py
echo -e "\n=== Check what SubIssue type is from PyGithub ==="
rg -nP -C3 'SubIssue.*from.*github'
Length of output: 181
🏁 Script executed:
#!/bin/bash
# Let's look for any existing tests about cross-repo sub-issues to understand expected behavior
echo "=== Looking for tests related to cross-repo or external sub-issues ==="
rg -nP -C5 'external.*sub|cross.*repo' tests/
echo -e "\n=== Let's check if there are any TODO comments about this behavior ==="
rg -nP 'TODO.*cross.*repo|TODO.*external'
echo -e "\n=== Let's see how SubIssueRecord constructor works ==="
cat release_notes_generator/model/sub_issue_record.py | head -50
Length of output: 1247
@miroslavpojer The CodeRabbit review comment is ✅ VALID and identifies a critical issue that would prevent the core functionality of this PR from working correctly.
Analysis Summary
The Problem:
-
SubIssueRecord inheritance:
SubIssueRecordinherits fromIssueRecord, which implementscontains_change_increment()asreturn self.pull_requests_count() > 0 -
External sub-issues behavior: External/cross-repo sub-issues are created as
SubIssueRecordinstances but come from different repositories, so they likely won't have local pull requests associated with them -
Filter consequence: The new guard
if not sub_issue.contains_change_increment(): continuewill skip any sub-issue wherepull_requests_count() == 0, which includes cross-repo sub-issues -
Objective conflict: This directly contradicts PR objective Add collection of cross repo sub-issues #172: "Add logic to detect and save these cross-repo sub-issues so they can be appended to records"
Evidence from Code Analysis
From the factory code (lines 98-103 in issue_hierarchy_record_factory.py):
# Third register all external sub-issues
for ext_sub_issue in self.__external_sub_issues:
if self.get_id(ext_sub_issue) in self.__registered_issues:
continue
self._create_record_for_sub_issue(ext_sub_issue) # Creates SubIssueRecordThese external sub-issues won't have local PRs, so contains_change_increment() returns False, causing them to be filtered out in to_chapter_row().
Recommendation
You need to modify the filtering logic to allow cross-repo sub-issues to render. Consider:
- Add a repository context check to distinguish local vs. cross-repo sub-issues
- Override
contains_change_increment()inSubIssueRecordto handle cross-repo cases - Add a flag to
SubIssueRecordto identify cross-repo sub-issues and exempt them from the filter
This is a high-priority issue that would make the cross-repo sub-issue feature non-functional.
There was a problem hiding this comment.
I see it will be solved in next issue, see new TODO.
There was a problem hiding this comment.
🧩 Analysis chain
@miroslavpojer I understand you're planning to address this in a future issue as indicated by the TODO. This is a reasonable approach given that the cross-repo sub-issue filtering issue is a significant design consideration that might require broader changes.
The CodeRabbit review correctly identified that the current contains_change_increment() filter would prevent cross-repo sub-issues from appearing in release notes, but deferring this to a dedicated issue allows for a more thoughtful solution that considers the broader implications of how external sub-issues should be handled and rendered.
🐰 (・ω・)
🏁 Script executed:
#!/bin/bash
# Search for TODO comments related to cross-repo, external sub-issues, or change increment
echo "=== Searching for TODO comments about cross-repo or external sub-issues ==="
rg -nP -C3 'TODO.*cross.*repo|TODO.*external.*sub|TODO.*change.*increment'
echo -e "\n=== Searching for any TODO in the hierarchy_issue_record.py file ==="
rg -nP -C5 'TODO' release_notes_generator/model/hierarchy_issue_record.py
echo -e "\n=== Searching for recent TODO comments in factory files ==="
rg -nP -C3 'TODO' release_notes_generator/record/factory/
Length of output: 206
🏁 Script executed:
#!/bin/bash
# Search for ALL TODO comments in the codebase
echo "=== All TODO comments in the codebase ==="
rg -nP 'TODO'
echo -e "\n=== Let's also check for any comments mentioning 'next issue' or 'future issue' ==="
rg -nP -i 'next issue|future issue|later issue'
echo -e "\n=== Search for any comments about filtering or rendering cross-repo ==="
rg -nP -C2 'filter.*cross|render.*cross|cross.*filter|cross.*render'
Length of output: 2019
@miroslavpojer I understand your approach to defer this cross-repo sub-issue filtering issue to a future iteration. While I wasn't able to locate the specific TODO you're referencing in the current codebase, I trust that you have a clear plan for addressing this in a subsequent issue.
This is actually a wise approach given the complexity involved - properly handling cross-repo sub-issues in the rendering pipeline requires careful consideration of:
- How to distinguish between local and external sub-issues
- Whether external sub-issues should have different rendering rules
- How the
contains_change_increment()logic should work for cross-repo cases
The CodeRabbit review correctly identified the issue, but addressing it properly likely requires broader architectural decisions that are better suited for dedicated issue planning rather than a quick fix in this PR.
Your current implementation successfully introduces the core cross-repo detection and collection functionality (the main objectives of issues #171 and #172), and the rendering refinement can be addressed in the follow-up work.
🐰 (・ω・)
🧠 Learnings used
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#173
File: release_notes_generator/record/factory/default_record_factory.py:109-126
Timestamp: 2025-09-28T14:13:38.954Z
Learning: In release_notes_generator/record/factory/default_record_factory.py, there is a cross-repo fetch bug where parent_issue_id in format "owner/repo#number" is fetched from the wrong repository because the code only queries data.repository instead of parsing the owner/repo from the ID and using the GitHub client to fetch from the correct repository.
| for parent_issue_id in detected_issues: | ||
| # create an issue record if not present for PR parent | ||
| if parent_issue_number not in self._records: | ||
| if parent_issue_id not in self._records: | ||
| logger.warning( | ||
| "Detected PR %d linked to issue %d which is not in the list of received issues. " | ||
| "Detected PR %d linked to issue %s which is not in the list of received issues. " | ||
| "Fetching ...", | ||
| pull.number, | ||
| parent_issue_number, | ||
| parent_issue_id, | ||
| ) | ||
| # dev note: here we expect that PR links to an issue in the same repository !!! | ||
| parent_issue_number = int(parent_issue_id.split("#")[1]) | ||
| parent_issue = ( | ||
| self._safe_call(data.repository.get_issue)(parent_issue_number) if data.repository else None | ||
| ) | ||
| if parent_issue is not None: | ||
| self._create_record_for_issue(parent_issue) | ||
|
|
||
| if parent_issue_number in self._records: | ||
| cast(IssueRecord, self._records[parent_issue_number]).register_pull_request(pull) | ||
| logger.debug("Registering PR %d: %s to Issue %d", pull.number, pull.title, parent_issue_number) | ||
| if parent_issue_id in self._records: |
There was a problem hiding this comment.
Cross-repo fetch bug: fetching parent issue only from data.repository.
When parent_issue_id points to another repo, the code incorrectly queries the current repo.
Apply this diff and store the GitHub client in __init__ as shown below:
- # dev note: here we expect that PR links to an issue in the same repository !!!
- parent_issue_number = int(parent_issue_id.split("#")[1])
- parent_issue = (
- self._safe_call(data.repository.get_issue)(parent_issue_number) if data.repository else None
- )
+ # Parse 'owner/repo#number' and fetch from the correct repository.
+ owner_repo, num = parent_issue_id.split("#", 1)
+ parent_issue_number = int(num)
+ parent_repo = self._safe_call(self._github.get_repo)(owner_repo) if hasattr(self, "_github") else None
+ parent_issue = (
+ self._safe_call(parent_repo.get_issue)(parent_issue_number) if parent_repo else None
+ )Add storing the client in the constructor:
# in __init__
self._github = github🤖 Prompt for AI Agents
In release_notes_generator/record/factory/default_record_factory.py around lines
104 to 121, the code only queries data.repository for parent issues so
cross-repo parent_issue_id values are fetched from the wrong repo; store the
passed GitHub client on the instance (add self._github = github in __init__) and
when resolving parent_issue_id parse the owner/repo and issue number from the
id, use the stored client to get the correct repository and fetch that issue
(falling back to data.repository only if owner/repo matches or client call
fails), then create the record if the fetched issue is not None.
There was a problem hiding this comment.
Check validity of this review note.
There was a problem hiding this comment.
🧩 Analysis chain
Let me analyze the validity of this review note by examining the implementation details.
@miroslavpojer The CodeRabbit review note appears to be valid and identifies a legitimate bug.
Here's my analysis:
The Issue:
- The new ID format is
"owner/repo#number"(confirmed by the_issue_idmethod returningf"{repo_full_name.lower()}#{number}") - The problematic code parses
parent_issue_idwithint(parent_issue_id.split("#")[1]), extracting only the number part - It then queries
data.repository.get_issue(parent_issue_number), which would fetch from the wrong repository for cross-repo issues
Example of the Bug:
- If
parent_issue_id = "external-org/external-repo#123" - Current code extracts
parent_issue_number = 123 - Then queries
data.repository(e.g., "current-org/current-repo") for issue #58 - Design PR template proposal #123 - This fetches issue #58 - Design PR template proposal #123 from the wrong repository
Validation:
- The PR objectives explicitly mention "cross-repo issues" as a feature
- The
extract_issue_numbers_from_bodyandget_issues_for_prfunctions now return IDs in the"owner/repo#number"format - The constructor doesn't currently store the GitHub client, making cross-repo fetching impossible
CodeRabbit's Solution:
The suggested fix is correct:
- Parse both repository and issue number from
parent_issue_id - Use the stored GitHub client to get the correct repository
- Fetch the issue from that repository
The review note accurately identifies both the problem and provides a viable solution. You should implement the suggested fix to properly support cross-repository issue fetching.
🏁 Script executed:
#!/bin/bash
# Let's examine the current implementation in the file to understand the issue better
echo "=== Examining the specific code section flagged by CodeRabbit ==="
sed -n '100,125p' release_notes_generator/record/factory/default_record_factory.py
echo -e "\n=== Looking for other similar patterns in the file ==="
rg -n "parent_issue" release_notes_generator/record/factory/default_record_factory.py
echo -e "\n=== Checking how IDs are generated to confirm the format ==="
rg -n -A5 -B5 "def.*get_id" release_notes_generator/record/factory/default_record_factory.py
echo -e "\n=== Looking at the constructor to see if GitHub client is stored ==="
rg -n -A10 "__init__" release_notes_generator/record/factory/default_record_factory.py
echo -e "\n=== Examining the extract_issue_numbers_from_body function signature changes ==="
rg -n -A5 -B5 "extract_issue_numbers_from_body" release_notes_generator/utils/pull_request_utils.py
echo -e "\n=== Checking what get_issues_for_pr returns now ==="
rg -n -A10 -B5 "def get_issues_for_pr" release_notes_generator/utils/pull_request_utils.py
Length of output: 4490
🏁 Script executed:
#!/bin/bash
# Let's look at how the ID generation works for cross-repo scenarios
echo "=== Checking ID generation methods ==="
rg -n -A3 -B1 "_issue_id" release_notes_generator/record/factory/default_record_factory.py
echo -e "\n=== Looking at how pull request IDs are generated ==="
rg -n -A5 "get_id.*pull_request" release_notes_generator/record/factory/default_record_factory.py
echo -e "\n=== Examining the home_repository usage ==="
rg -n "home_repository" release_notes_generator/record/factory/default_record_factory.py
echo -e "\n=== Checking what extract_issue_numbers_from_body returns ==="
cat release_notes_generator/utils/pull_request_utils.py | head -50
echo -e "\n=== Looking at the test cases to understand expected behavior ==="
rg -n -A5 -B5 "org/repo#" tests/release_notes/record/factory/test_default_record_factory.py
Length of output: 10195
🏁 Script executed:
#!/bin/bash
# Let's examine the pull request utilities to understand the ID format
echo "=== Full extract_issue_numbers_from_body function ==="
ast-grep --pattern $'def extract_issue_numbers_from_body($$$) {
$$$
}'
echo -e "\n=== Full get_issues_for_pr function ==="
ast-grep --pattern $'def get_issues_for_pr($$$) {
$$$
}'
echo -e "\n=== Looking at test expectations for cross-repo scenarios ==="
rg -n -C3 "org/repo#" tests/
echo -e "\n=== Checking if there are any references to cross-repo in the codebase ==="
rg -n -i "cross.repo" .
echo -e "\n=== Examining the PR objectives more closely ==="
rg -n -C2 "cross-repo" README.md || echo "No cross-repo mentions in README"
Length of output: 38644
Based on my comprehensive analysis of the code and evidence, I can confirm that CodeRabbit's review note is completely valid and identifies a serious bug.
Evidence Supporting the Bug Report:
1. ID Format Confirmation:
- The
_issue_idmethod returnsf"{repo_full_name}#{number}"(Line 79) - Tests consistently expect
"org/repo#123"format throughout the codebase - Pull request IDs use
f"{self._home_repository.full_name}#{pull_request.number}"(Line 83)
2. Problematic Code Pattern:
# Line 116: Only extracts the number part
parent_issue_number = int(parent_issue_id.split("#")[1])
# Line 118: Always queries the current repository
parent_issue = self._safe_call(data.repository.get_issue)(parent_issue_number)3. Cross-Repo Bug Scenario:
- Input:
parent_issue_id = "external-org/external-repo#123" - Parsed:
parent_issue_number = 123 - Fetched from:
data.repository(wrong repo!) - Should fetch from:
external-org/external-repo
4. Missing Infrastructure:
- The constructor doesn't store the GitHub client (
self._github = githubis missing) - Cannot fetch from external repositories without the client
5. Contradictory Evidence:
- The comment says "here we expect that PR links to an issue in the same repository !!!"
- But the PR objectives explicitly mention cross-repo sub-issues as a feature
- The new ID format supports cross-repo references
Conclusion:
The bug is real and CodeRabbit's suggested fix is correct. The implementation needs to:
- Store the GitHub client in
__init__ - Parse both repository and issue number from
parent_issue_id - Use the client to fetch from the correct repository
This is a critical bug that would cause incorrect issue fetching in cross-repository scenarios.
There was a problem hiding this comment.
@miroslavpojer Thank you for adding the TODO to address the cross-repo fetch bug!
Would you like me to create a GitHub issue to track this cross-repository issue fetching bug? I can include the technical details and suggested fix for future reference.
(\ /)
( .. )
o(")(") thoughtful rabbit pondering cross-repo complexities
✏️ Learnings added
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#173
File: release_notes_generator/record/factory/default_record_factory.py:109-126
Timestamp: 2025-09-28T14:13:38.954Z
Learning: In release_notes_generator/record/factory/default_record_factory.py, there is a cross-repo fetch bug where parent_issue_id in format "owner/repo#number" is fetched from the wrong repository because the code only queries data.repository instead of parsing the owner/repo from the ID and using the GitHub client to fetch from the correct repository.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| linked_from_api = self._safe_call(get_issues_for_pr)(pull_number=pull.number) or set() | ||
| linked_from_body = extract_issue_numbers_from_body(pull) | ||
| pull_issues: list[int] = list(linked_from_api.union(linked_from_body)) | ||
| linked_from_body = extract_issue_numbers_from_body(pull, data.repository) | ||
| pull_issues: list[str] = list(linked_from_api.union(linked_from_body)) | ||
| attached_any = False | ||
| if len(pull_issues) > 0: | ||
| for issue_number in pull_issues: | ||
| if issue_number not in self._records: | ||
| for issue_id in pull_issues: | ||
| if issue_id not in self._records: | ||
| logger.warning( | ||
| "Detected PR %d linked to issue %d which is not in the list of received issues. " | ||
| "Detected PR %d linked to issue %s which is not in the list of received issues. " | ||
| "Fetching ...", | ||
| pull.number, | ||
| issue_number, | ||
| issue_id, | ||
| ) | ||
| # dev note: here we expect that PR links to an issue in the same repository !!! | ||
| issue_number = int(issue_id.split("#")[1]) | ||
| parent_issue = self._safe_call(data.repository.get_issue)(issue_number) if data.repository else None | ||
| if parent_issue is not None: | ||
| self._create_issue_record_using_sub_issues_existence(parent_issue) | ||
| self._create_issue_record_using_sub_issues_existence(parent_issue, data) | ||
|
|
There was a problem hiding this comment.
Cross-repo fetch bug when attaching PRs to issues.
issue_id is repo-scoped, but the code always queries data.repository.
Apply this diff:
- # dev note: here we expect that PR links to an issue in the same repository !!!
- issue_number = int(issue_id.split("#")[1])
- parent_issue = self._safe_call(data.repository.get_issue)(issue_number) if data.repository else None
+ # Fetch from the repository encoded in 'owner/repo#number'
+ owner_repo, num = issue_id.split("#", 1)
+ issue_number = int(num)
+ parent_repo = self._safe_call(self._github.get_repo)(owner_repo) if hasattr(self, "_github") else None
+ parent_issue = self._safe_call(parent_repo.get_issue)(issue_number) if parent_repo else NoneNote: Ensure self._github is stored in the base class __init__ (see prior comment).
📝 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.
| linked_from_api = self._safe_call(get_issues_for_pr)(pull_number=pull.number) or set() | |
| linked_from_body = extract_issue_numbers_from_body(pull) | |
| pull_issues: list[int] = list(linked_from_api.union(linked_from_body)) | |
| linked_from_body = extract_issue_numbers_from_body(pull, data.repository) | |
| pull_issues: list[str] = list(linked_from_api.union(linked_from_body)) | |
| attached_any = False | |
| if len(pull_issues) > 0: | |
| for issue_number in pull_issues: | |
| if issue_number not in self._records: | |
| for issue_id in pull_issues: | |
| if issue_id not in self._records: | |
| logger.warning( | |
| "Detected PR %d linked to issue %d which is not in the list of received issues. " | |
| "Detected PR %d linked to issue %s which is not in the list of received issues. " | |
| "Fetching ...", | |
| pull.number, | |
| issue_number, | |
| issue_id, | |
| ) | |
| # dev note: here we expect that PR links to an issue in the same repository !!! | |
| issue_number = int(issue_id.split("#")[1]) | |
| parent_issue = self._safe_call(data.repository.get_issue)(issue_number) if data.repository else None | |
| if parent_issue is not None: | |
| self._create_issue_record_using_sub_issues_existence(parent_issue) | |
| self._create_issue_record_using_sub_issues_existence(parent_issue, data) | |
| linked_from_api = self._safe_call(get_issues_for_pr)(pull_number=pull.number) or set() | |
| linked_from_body = extract_issue_numbers_from_body(pull, data.repository) | |
| pull_issues: list[str] = list(linked_from_api.union(linked_from_body)) | |
| attached_any = False | |
| if len(pull_issues) > 0: | |
| for issue_id in pull_issues: | |
| if issue_id not in self._records: | |
| logger.warning( | |
| "Detected PR %d linked to issue %s which is not in the list of received issues. " | |
| "Fetching ...", | |
| pull.number, | |
| issue_id, | |
| ) | |
| # Fetch from the repository encoded in 'owner/repo#number' | |
| owner_repo, num = issue_id.split("#", 1) | |
| issue_number = int(num) | |
| parent_repo = self._safe_call(self._github.get_repo)(owner_repo) if hasattr(self, "_github") else None | |
| parent_issue = self._safe_call(parent_repo.get_issue)(issue_number) if parent_repo else None | |
| if parent_issue is not None: | |
| self._create_issue_record_using_sub_issues_existence(parent_issue, data) |
There was a problem hiding this comment.
Check this is if it still valid.
There was a problem hiding this comment.
🧩 Analysis chain
@miroslavpojer Yes, my previous comment is still valid. The cross-repo fetch bug remains in the current code.
The Issue:
The code now uses string issue IDs in format "owner/repo#number", but when fetching a missing issue, it still incorrectly queries the home repository (data.repository) instead of the repository specified in the issue_id.
Current problematic code (lines 155-157):
# dev note: here we expect that PR links to an issue in the same repository !!!
issue_number = int(issue_id.split("#")[1])
parent_issue = self._safe_call(data.repository.get_issue)(issue_number) if data.repository else NoneThe Problem:
issue_idcould be"other-org/other-repo#123"- Code extracts
123but queriesdata.repository(your home repo) - Should query
other-org/other-repoinstead
My suggested fix is still correct and needed:
# Fetch from the repository encoded in 'owner/repo#number'
owner_repo, num = issue_id.split("#", 1)
issue_number = int(num)
parent_repo = self._safe_call(self._github.get_repo)(owner_repo) if hasattr(self, "_github") else None
parent_issue = self._safe_call(parent_repo.get_issue)(issue_number) if parent_repo else NoneThis bug could cause:
- Fetching wrong issues from the home repo
- Missing actual cross-repo issues
- Incorrect record associations
The fix is essential for proper cross-repo issue handling as intended by PR objectives #171 and #172.
🏁 Script executed:
# Let's examine the specific section of code around the issue
rg -n -A 10 -B 5 "here we expect that PR links to an issue in the same repository" release_notes_generator/record/factory/issue_hierarchy_record_factory.py
# Also check how issue_id is used in this context
rg -n -A 15 -B 5 "issue_id not in self._records" release_notes_generator/record/factory/issue_hierarchy_record_factory.py
Length of output: 2312
@miroslavpojer Yes, the issue is definitely still valid.
The script output confirms the exact problem I identified:
Lines 156-158 show the bug:
# dev note: here we expect that PR links to an issue in the same repository !!!
issue_number = int(issue_id.split("#")[1])
parent_issue = self._safe_call(data.repository.get_issue)(issue_number) if data.repository else NoneThe contradiction:
- Your PR introduces cross-repo issue IDs in format
"owner/repo#number" - But this code comment says it "expects issues in the same repository"
- The code ignores the
owner/repopart and always queriesdata.repository
This breaks cross-repo functionality - when a PR links to "external-org/external-repo#123", your code tries to fetch issue #123 from your home repository instead of external-org/external-repo.
My suggested fix is still needed:
# Fetch from the repository encoded in 'owner/repo#number'
owner_repo, num = issue_id.split("#", 1)
issue_number = int(num)
parent_repo = self._safe_call(self._github.get_repo)(owner_repo) if hasattr(self, "_github") else None
parent_issue = self._safe_call(parent_repo.get_issue)(issue_number) if parent_repo else NoneThis bug directly contradicts your PR objectives #171 and #172 for cross-repo issue support.
There was a problem hiding this comment.
@miroslavpojer Thank you for adding the TODO! That's a great way to acknowledge the cross-repo fetch bug while keeping it out of scope for this PR. The TODO will ensure this issue gets proper attention in future work.
This addresses my concern - having it documented prevents it from being forgotten and makes the current limitation clear to other developers.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
release_notes_generator/record/factory/default_record_factory.py (1)
50-53: Store the GitHub client for cross-repo fetches.Needed to resolve parent issues from other repositories.
Apply:
def __init__(self, github: Github) -> None: - rate_limiter = GithubRateLimiter(github) + self._github = github + rate_limiter = GithubRateLimiter(github)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (1)
174-179: Standalone PRs keyed by str(pull.number); make repo-scoped.Prevents collisions and aligns with the new ID scheme.
Apply:
if not attached_any: pr_rec = PullRequestRecord(pull, pull_labels, skip_record) for c in related_commits: # register commits to the PR record pr_rec.register_commit(c) - self._records[str(pull.number)] = pr_rec - logger.debug("Created record for PR %d: %s", pull.number, pull.title) + pid = self.get_id(pull) + self._records[pid] = pr_rec + logger.debug("Created record for PR %s: %s", pid, pull.title)
🧹 Nitpick comments (3)
.pylintrc (1)
307-307: Prefer refactoring over loosening the branch thresholdBumping
max-branchesto 15 relaxes the guardrail for every function in the project. Before adopting the higher ceiling, can we refactor the hot spots that now breach the old limit (e.g., extract helpers, collapse conditionals) or confine the exception to the specific module? That keeps the global lint budget tight and prevents future creep in cyclomatic complexity.release_notes_generator/chapters/custom_chapters.py (1)
65-71: Simplify: rely on contains_change_increment(); drop pulls_count.Avoid duplicate logic and extra API calls.
Apply:
- pulls_count = 1 - if isinstance(records[record_id], (HierarchyIssueRecord, IssueRecord, SubIssueRecord)): - pulls_count = cast(IssueRecord, records[record_id]).pull_requests_count() - - for record_label in records[record_id].labels: # iterate all labels of the record (issue, or 1st PR) - if record_label in ch.labels and pulls_count > 0: + for record_label in records[record_id].labels: + if record_label in ch.labels and records[record_id].contains_change_increment():release_notes_generator/record/factory/issue_hierarchy_record_factory.py (1)
190-197: Confirm singledispatch covers SubIssue.get_id(si) assumes SubIssue dispatches to Issue handler. If SubIssue isn’t a subclass of Issue in your PyGithub version, add an explicit @get_id.register for SubIssue.
Option if needed:
+ @get_id.register + def _(self, sub_issue: SubIssue) -> str: + return self._issue_id(sub_issue.repository.full_name, sub_issue.number)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.pylintrc(1 hunks)release_notes_generator/chapters/custom_chapters.py(2 hunks)release_notes_generator/filter.py(1 hunks)release_notes_generator/record/factory/default_record_factory.py(4 hunks)release_notes_generator/record/factory/issue_hierarchy_record_factory.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
release_notes_generator/chapters/custom_chapters.py (6)
release_notes_generator/chapters/base_chapters.py (1)
populate(107-112)release_notes_generator/chapters/service_chapters.py (1)
populate(96-141)release_notes_generator/model/record.py (4)
Record(29-199)record_id(72-77)is_present_in_chapters(45-51)contains_change_increment(121-127)release_notes_generator/model/commit_record.py (2)
record_id(27-28)contains_change_increment(68-69)release_notes_generator/model/issue_record.py (2)
record_id(43-44)contains_change_increment(125-126)release_notes_generator/model/pull_request_record.py (2)
record_id(33-34)contains_change_increment(131-132)
release_notes_generator/record/factory/default_record_factory.py (6)
release_notes_generator/model/issue_record.py (4)
issue(63-68)register_pull_request(195-202)IssueRecord(16-331)get_labels(81-83)release_notes_generator/model/pull_request_record.py (3)
pull_request(70-75)get_labels(107-109)PullRequestRecord(15-293)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (1)
generate(58-134)release_notes_generator/record/factory/record_factory.py (1)
generate(35-42)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/utils/pull_request_utils.py (2)
extract_issue_numbers_from_body(32-52)get_issues_for_pr(56-81)
release_notes_generator/filter.py (1)
release_notes_generator/model/mined_data.py (1)
MinedData(38-56)
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (6)
release_notes_generator/record/factory/default_record_factory.py (2)
generate(87-161)get_id(57-67)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/model/issue_record.py (2)
issue(63-68)IssueRecord(16-331)release_notes_generator/model/sub_issue_record.py (2)
issue(27-34)SubIssueRecord(15-36)release_notes_generator/utils/pull_request_utils.py (1)
extract_issue_numbers_from_body(32-52)release_notes_generator/model/hierarchy_issue_record.py (3)
HierarchyIssueRecord(16-158)sub_issues(47-51)sub_hierarchy_issues(54-58)
🪛 Ruff (0.13.1)
release_notes_generator/record/factory/default_record_factory.py
96-96: Unused function argument: skip_rec
(ARG001)
🔇 Additional comments (7)
release_notes_generator/filter.py (1)
64-64: Constructing MinedData with repository preserves contextPassing
data.repositoryinto the new constructor aligns with the updatedMinedDataAPI and keeps the downstream filter logic unchanged. Looks good.release_notes_generator/record/factory/issue_hierarchy_record_factory.py (3)
209-214: Boundary check: since vs closed_at uses “>” (excludes equals).If “closed exactly at since” should be excluded, keep as is; otherwise use “>=”.
77-101: LGTM: multi-pass hierarchy build and re-registration.Loop with progress check and bottom-up re-parenting looks solid.
Also applies to: 295-346
147-160: Cross-repo fetch bug when attaching PRs to issues.Always fetching via data.repository breaks for cross-repo links.
Apply:
- # dev note: here we expect that PR links to an issue in the same repository !!! - issue_number = int(issue_id.split("#")[1]) - parent_issue = self._safe_call(data.repository.get_issue)(issue_number) if data.repository else None + owner_repo, num = issue_id.split("#", 1) + issue_number = int(num) + parent_repo = self._safe_call(self._github.get_repo)(owner_repo) if hasattr(self, "_github") else None + parent_issue = self._safe_call(parent_repo.get_issue)(issue_number) if parent_repo else Nonerelease_notes_generator/record/factory/default_record_factory.py (3)
79-81: PR IDs must be repo-scoped for consistency and to avoid collisions.Currently returns only the number.
Apply:
@get_id.register def _(self, pull_request: PullRequest) -> str: - return f"{pull_request.number}" + return f"{pull_request.repository.full_name}#{pull_request.number}"
74-77: Do not lowercase repo in IDs; breaks lookups and joins.Lowercasing causes mismatches with emitters that keep original casing.
Apply:
@lru_cache(maxsize=2048) def _issue_id(repo_full_name: str, number: int) -> str: - return f"{repo_full_name.lower()}#{number}" + return f"{repo_full_name}#{number}"
104-117: Cross-repo parent issue fetch queries the wrong repo.Parse owner/repo from the ID and fetch via the correct repository.
Apply:
- # dev note: here we expect that PR links to an issue in the same repository !!! - parent_issue_number = int(parent_issue_id.split("#")[1]) - parent_issue = ( - self._safe_call(data.repository.get_issue)(parent_issue_number) if data.repository else None - ) + owner_repo, num = parent_issue_id.split("#", 1) + parent_issue_number = int(num) + parent_repo = self._safe_call(self._github.get_repo)(owner_repo) if hasattr(self, "_github") else None + parent_issue = ( + self._safe_call(parent_repo.get_issue)(parent_issue_number) if parent_repo else None + )
release_notes_generator/record/factory/default_record_factory.py
Outdated
Show resolved
Hide resolved
| dict[str, Record]: A dictionary of records where the key is the issue or pull request number. | ||
| """ | ||
|
|
||
| def register_pull_request(pull: PullRequest, skip_rec: bool) -> None: |
There was a problem hiding this comment.
Remove unused parameter skip_rec.
Flagged by Ruff and unused.
Apply:
- def register_pull_request(pull: PullRequest, skip_rec: bool) -> None:
+ def register_pull_request(pull: PullRequest) -> None:- register_pull_request(pull, skip_record)
+ register_pull_request(pull)Also applies to: 149-149
🧰 Tools
🪛 Ruff (0.13.1)
96-96: Unused function argument: skip_rec
(ARG001)
🤖 Prompt for AI Agents
In release_notes_generator/record/factory/default_record_factory.py around line
96 (and similarly at line 149), the register_pull_request signature includes an
unused parameter skip_rec flagged by Ruff; remove skip_rec from the function
definitions, update any local type hints or declarations accordingly, and search
for and update all call sites to stop passing that argument (or refactor callers
to no longer supply it); run linters/tests to ensure no remaining references and
commit the cleaned-up signatures.
There was a problem hiding this comment.
Kept for later usage. I know it is not good.
There was a problem hiding this comment.
@miroslavpojer Thanks for clarifying! I understand you're keeping it for future functionality.
If you'd like to keep the parameter for later use, you might consider adding a brief comment to document the intended future usage, which can help other developers understand the design decision and prevent similar linting flags in the future.
I'll respect your architectural choice here.
✏️ Learnings added
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#173
File: release_notes_generator/record/factory/default_record_factory.py:0-0
Timestamp: 2025-09-28T13:07:31.335Z
Learning: In release_notes_generator/record/factory/default_record_factory.py, the user miroslavpojer prefers to keep unused parameters like `skip_rec` in the `register_pull_request` function for future usage, even when flagged by linters as unused.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| dict[str, Record]: A dictionary of records where the key is the issue id or pull request number. | ||
| """ |
There was a problem hiding this comment.
Docstring: fix key description.
Align with repo-scoped string ID format.
Apply:
- dict[str, Record]: A dictionary of records where the key is the issue id or pull request number.
+ dict[str, Record]: A dictionary of records keyed by 'owner/repo#number' (or commit SHA).📝 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.
| dict[str, Record]: A dictionary of records where the key is the issue id or pull request number. | |
| """ | |
| dict[str, Record]: A dictionary of records keyed by 'owner/repo#number' (or commit SHA). | |
| """ |
🤖 Prompt for AI Agents
In release_notes_generator/record/factory/issue_hierarchy_record_factory.py
around lines 64 to 65, the docstring currently says the dict keys are "issue id
or pull request number"; update this to state keys are repo-scoped string IDs
(for example "owner/repo#<number>") to align with the repository-wide ID format
used across the codebase, replacing the old description accordingly.
There was a problem hiding this comment.
@miroslavpojer Thank you for fixing the docstring! The documentation now properly reflects that the dictionary keys use the repository-scoped string ID format ('owner/repo#number') instead of the previous description. This aligns well with the new ID system implemented throughout the codebase.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
@miroslavpojer Perfect! Thank you for resolving the docstring issue. The documentation now accurately reflects the repository-scoped string ID format used throughout the codebase.
/| /|
( :v: )
|(_)|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (12)
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (3)
143-146: Type safety: ensure set operations don’t blow up
get_issues_for_prmay return list in tests/mocks; calling.union(...)on a list crashes. Normalize to sets.- linked_from_api = self._safe_call(get_issues_for_pr)(pull_number=pull.number) or set() - linked_from_body = extract_issue_numbers_from_body(pull, data.repository) - pull_issues: list[str] = list(linked_from_api.union(linked_from_body)) + linked_from_api_raw = self._safe_call(get_issues_for_pr)(pull_number=pull.number) + linked_from_api = set(linked_from_api_raw or []) + linked_from_body = set(extract_issue_numbers_from_body(pull, data.repository)) + pull_issues: list[str] = list(linked_from_api | linked_from_body)
243-244: Minor: avoid.keys()membership + drop pylint disableUse direct dict membership.
- if self.get_id(issue) in self.__sub_issue_parents.keys(): # pylint: disable=consider-iterating-dictionary + if self.get_id(issue) in self.__sub_issue_parents:
191-227: Avoid repeated API calls to fetch sub‑issuesCompute “has children” once per sub‑issue instead of calling
list(si.get_sub_issues())multiple times.for si in sub_issues: siid = self.get_id(si) # check if sub-issue is from current repository if si.repository.full_name != issue.repository.full_name: @@ else: use_issue = False + children = si.get_sub_issues() + has_children = bool(getattr(children, "totalCount", 0)) or any(True for _ in children) if ( data.since and si.state == IssueRecord.ISSUE_STATE_CLOSED and si.closed_at and data.since > si.closed_at ): logger.debug("Detected sub-issue %d closed in previous release.", si.number) - if len(list(si.get_sub_issues())) > 0: + if has_children: use_issue = True else: self.__registered_issues.add(siid) elif si.state == IssueRecord.ISSUE_STATE_OPEN: logger.debug("Detected sub-issue %d is still open.", si.number) - if len(list(si.get_sub_issues())) > 0: + if has_children: use_issue = True else: self.__registered_issues.add(siid)release_notes_generator/chapters/base_chapters.py (2)
37-44: Tighten types to string IDsRecords now use string IDs; align internal storage and API.
- self._populated_record_numbers: list[int | str] = [] + self._populated_record_numbers: list[str] = [] @@ - def populated_record_numbers_list(self) -> list[int | str]: + def populated_record_numbers_list(self) -> list[str]:
67-79: Accept string record IDs inadd_rowThe “number” is now a string record ID. Adjust signature and doc.
- def add_row(self, chapter_key: str, number: int, row: str) -> None: + def add_row(self, chapter_key: str, number: str, row: str) -> None: @@ - @param number: The number of the row. + @param number: The record ID (string).tests/release_notes/record/factory/test_default_record_factory.py (5)
172-177: Normalize mocked return type toset[str]Production expects a set-like; keeps contract consistent.
-def mock_get_issues_for_pr(pull_number: int) -> list[str]: +def mock_get_issues_for_pr(pull_number: int) -> set[str]: if pull_number == 101: - return ['org/repo#1'] + return {'org/repo#1'} elif pull_number == 102: - return ['org/repo#2'] - return [] + return {'org/repo#2'} + return set()
182-182: Silence unused unpack warnings (RUF059)Prefix unused with underscores.
- issue1, issue2, pr1, pr2, commit1, commit2 = setup_issues_pulls_commits(mocker, mock_repo) + issue1, _issue2, pr1, _pr2, commit1, _commit2 = setup_issues_pulls_commits(mocker, mock_repo)
273-274: Silence unused unpack warnings (RUF059)Only
issue1,pr1,issue2used.- issue1, issue2, pr1, pr2, commit1, commit2 = setup_issues_pulls_commits(mocker, mock_repo) + issue1, issue2, pr1, _pr2, _commit1, _commit2 = setup_issues_pulls_commits(mocker, mock_repo)
306-307: Silence unused unpack warnings (RUF059)Only
issue1,issue2,pr1used.- issue1, issue2, pr1, pr2, commit1, commit2 = setup_issues_pulls_commits(mocker, mock_repo) + issue1, issue2, pr1, _pr2, _commit1, _commit2 = setup_issues_pulls_commits(mocker, mock_repo)
343-345: Type + unused arg in mockMatch production signature and silence ARG001.
-def mock_get_issues_for_pr_no_issues(pull_number: str) -> list[str]: - return [] +def mock_get_issues_for_pr_no_issues(_pull_number: int) -> list[str]: + return []release_notes_generator/record/factory/default_record_factory.py (2)
95-95: Document the new record key formatThe return docstring still mentions “issue or pull request number,” but we now key everything by
owner/repo#number(and commits by repo@sha). Please update the wording so future readers aren’t misled.- dict[str, Record]: A dictionary of records where the key is the issue or pull request number. + dict[str, Record]: A dictionary keyed by `owner/repo#number` (or `<owner/repo>@<sha>` for commits).
98-151: Drop the unusedskip_recparameter
skip_recis never read insideregister_pull_request, and Ruff already flags it. Removing the argument and the corresponding call-site parameter keeps the code clean and quiets the warning.- def register_pull_request(pr: PullRequest, skip_rec: bool) -> None: + def register_pull_request(pr: PullRequest) -> None: @@ - register_pull_request(pull, skip_record) + register_pull_request(pull)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
release_notes_generator/chapters/base_chapters.py(1 hunks)release_notes_generator/generator.py(3 hunks)release_notes_generator/record/factory/default_record_factory.py(4 hunks)release_notes_generator/record/factory/issue_hierarchy_record_factory.py(6 hunks)tests/release_notes/builder/test_release_notes_builder.py(18 hunks)tests/release_notes/record/factory/test_default_record_factory.py(16 hunks)tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py
- tests/release_notes/builder/test_release_notes_builder.py
🧰 Additional context used
🧬 Code graph analysis (5)
release_notes_generator/chapters/base_chapters.py (4)
release_notes_generator/chapters/custom_chapters.py (1)
populate(42-76)release_notes_generator/chapters/service_chapters.py (1)
populate(96-141)tests/release_notes/chapters/test_base_chapters.py (2)
populate(30-31)Record(24-25)release_notes_generator/model/record.py (1)
Record(29-199)
release_notes_generator/generator.py (3)
release_notes_generator/miner.py (2)
check_repository_exists(50-61)mine_data(63-88)release_notes_generator/record/factory/default_record_factory.py (2)
generate(89-163)DefaultRecordFactory(46-207)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (2)
generate(59-135)IssueHierarchyRecordFactory(44-346)
release_notes_generator/record/factory/default_record_factory.py (5)
release_notes_generator/utils/decorators.py (1)
safe_call_decorator(51-82)release_notes_generator/model/issue_record.py (2)
issue(63-68)register_pull_request(195-202)release_notes_generator/record/factory/record_factory.py (1)
generate(35-42)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/utils/pull_request_utils.py (2)
extract_issue_numbers_from_body(32-52)get_issues_for_pr(56-81)
tests/release_notes/record/factory/test_default_record_factory.py (3)
tests/conftest.py (4)
mock_repo(94-97)mock_get_issues_for_pr(57-60)mock_safe_call_decorator(49-54)wrapper(50-53)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/record/factory/default_record_factory.py (2)
DefaultRecordFactory(46-207)generate(89-163)
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (8)
release_notes_generator/generator.py (1)
generate(70-110)release_notes_generator/record/factory/default_record_factory.py (2)
generate(89-163)get_id(59-69)release_notes_generator/record/factory/record_factory.py (1)
generate(35-42)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/model/issue_record.py (2)
issue(63-68)IssueRecord(16-331)release_notes_generator/model/sub_issue_record.py (2)
issue(27-34)SubIssueRecord(15-36)release_notes_generator/utils/pull_request_utils.py (1)
extract_issue_numbers_from_body(32-52)release_notes_generator/model/hierarchy_issue_record.py (3)
HierarchyIssueRecord(16-158)sub_issues(47-51)sub_hierarchy_issues(54-58)
🪛 Ruff (0.13.1)
release_notes_generator/record/factory/default_record_factory.py
98-98: Unused function argument: skip_rec
(ARG001)
tests/release_notes/record/factory/test_default_record_factory.py
182-182: Unpacked variable issue2 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
182-182: Unpacked variable pr2 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
273-273: Unpacked variable pr2 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
273-273: Unpacked variable commit1 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
273-273: Unpacked variable commit2 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
306-306: Unpacked variable pr2 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
306-306: Unpacked variable commit1 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
306-306: Unpacked variable commit2 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
343-343: Unused function argument: pull_number
(ARG001)
🔇 Additional comments (4)
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (3)
65-66: Docstring: reflect repo‑scoped keysKeys aren’t plain numbers anymore.
- dict[str, Record]: A dictionary of records where the key is the issue id or pull request number. + dict[str, Record]: Records keyed by 'owner/repo#number' (issues/PRs) and by commit SHA for direct commits.
179-179: PR record keyed by plain number; use repo‑scoped IDStore standalone PR records under the same repo‑scoped key format as elsewhere.
Apply:
- self._records[str(pull.number)] = pr_rec + pid = self.get_id(pull) + self._records[pid] = pr_rec
156-161: Cross‑repo fetch when attaching PRs to issuesDon’t assume same repo. Parse
owner/repo#numberand fetch from that repo. Requires a_githubhandle (see supporting change below).Apply:
- # dev note: here we expect that PR links to an issue in the same repository !!! - issue_number = int(issue_id.split("#")[1]) - parent_issue = self._safe_call(data.repository.get_issue)(issue_number) if data.repository else None + # Fetch from the repository encoded in 'owner/repo#number' + owner_repo, num = issue_id.split("#", 1) + issue_number = int(num) + parent_repo = ( + data.repository + if owner_repo.lower() == data.repository.full_name.lower() + else self._safe_call(self._github.get_repo)(owner_repo) + ) + parent_issue = self._safe_call(parent_repo.get_issue)(issue_number) if parent_repo else NoneSupporting change outside this file (DefaultRecordFactory.init):
class DefaultRecordFactory(RecordFactory): @@ - def __init__(self, github: Github, home_repository: Repository) -> None: + def __init__(self, github: Github, home_repository: Repository) -> None: rate_limiter = GithubRateLimiter(github) self._safe_call = safe_call_decorator(rate_limiter) self._home_repository = home_repository + self._github = githubRun to confirm all occurrences fixed:
#!/bin/bash rg -n "self\._records\[str\(pull\.number\)\]" -S rg -n "get_issue\)\(" release_notes_generator/record/factory/issue_hierarchy_record_factory.py -n -C2 rg -n "self\._github" release_notes_generator/record/factory -nrelease_notes_generator/record/factory/default_record_factory.py (1)
98-132: Resolve parent issues from their actual repositoryThe new string IDs already carry the
{owner/repo#number}context, but this block still assumes everything lives indata.repository. For true cross-repo sub-issues (PR objective #172) we must parse the owner/repo prefix and fetch the issue from that repository; otherwise the lookup keeps failing and we never materialize those parent records. Persist the GitHub client and use it to resolve the correct repo before callingget_issue.def __init__(self, github: Github, home_repository: Repository) -> None: rate_limiter = GithubRateLimiter(github) self._safe_call = safe_call_decorator(rate_limiter) self._home_repository = home_repository + self._github = github self._records: dict[str, Record] = {} @@ - # dev note: here we expect that PR links to an issue in the same repository !!! - parent_issue_number = int(parent_issue_id.split("#")[1]) - parent_issue = ( - self._safe_call(data.repository.get_issue)(parent_issue_number) if data.repository else None - ) + owner_repo, issue_fragment = parent_issue_id.split("#", 1) + parent_issue_number = int(issue_fragment) + parent_repo = self._safe_call(self._github.get_repo)(owner_repo) + parent_issue = ( + self._safe_call(parent_repo.get_issue)(parent_issue_number) if parent_repo else None + )
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (1)
177-183: Use normalized repo-scoped ID for PR record keys
Keying bystr(pull.number)risks collisions across repos; replace withpid = self.get_id(pull)and use that as the dict key.- if not attached_any: - pr_rec = PullRequestRecord(pull, pull_labels, skip_record) - for c in related_commits: # register commits to the PR record - pr_rec.register_commit(c) - self._records[str(pull.number)] = pr_rec - logger.debug("Created record for PR %d: %s", pull.number, pull.title) + if not attached_any: + pr_rec = PullRequestRecord(pull, pull_labels, skip_record) + for c in related_commits: # register commits to the PR record + pr_rec.register_commit(c) + pid = self.get_id(pull) + self._records[pid] = pr_rec + logger.debug("Created record for PR %s: %s", pid, pull.title)
🧹 Nitpick comments (5)
release_notes_generator/record/factory/default_record_factory.py (2)
138-152: Avoid duplicate API calls for linked issues.
get_issues_for_prandextract_issue_numbers_from_bodyare called here and again insideregister_pull_request. Compute once and pass the set down.- for pull in data.pull_requests: + for pull in data.pull_requests: pid = self.get_id(pull) pull_labels = [label.name for label in pull.get_labels()] skip_record: bool = any(item in pull_labels for item in ActionInputs.get_skip_release_notes_labels()) - linked_from_api = self._safe_call(get_issues_for_pr)(pull_number=pull.number) or set() - linked_from_body = extract_issue_numbers_from_body(pull, data.repository) - if not linked_from_api and not linked_from_body: + detected_issues = (self._safe_call(get_issues_for_pr)(pull_number=pull.number) or set()).union( + extract_issue_numbers_from_body(pull, data.repository) + ) + if not detected_issues: self._records[pid] = PullRequestRecord(pull, pull_labels, skip=skip_record) logger.debug("Created record for PR %s: %s", pid, pull.title) else: - logger.debug("Registering pull number: %s, title : %s", pid, pull.title) - register_pull_request(pull, skip_record) + logger.debug("Registering PR %s: %s", pid, pull.title) + # Pass precomputed detections (adjust nested fn signature accordingly) — or keep inner detection removed. + register_pull_request(pull, skip_record)
150-151: Nit: log message says “pull number” but logs an ID.Use “PR”/“ID” to avoid confusion.
- logger.debug("Registering pull number: %s, title : %s", pid, pull.title) + logger.debug("Registering PR %s: %s", pid, pull.title)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (3)
119-123: Commit keying: consider consistent ID scheme.You key commits by
shahere; ensure all factories use the same convention (either plainshaorowner/repo@sha).- self._records[commit.sha] = CommitRecord(commit) + self._records[commit.sha] = CommitRecord(commit) # OK if plain SHA is the chosen convention +# Or: +# self._records[self.get_id(commit)] = CommitRecord(commit)
190-241: Avoid repeated sub-issue fetches per item.
list(si.get_sub_issues())is called multiple times persi. Cache once to reduce API calls.- for si in sub_issues: + for si in sub_issues: siid = self.get_id(si) - # check if sub-issue is from current repository + # check if sub-issue is from current repository if si.repository.full_name != issue.repository.full_name: @@ - else: + else: + si_children = list(si.get_sub_issues()) use_issue = False if ( data.since and si.state == IssueRecord.ISSUE_STATE_CLOSED and si.closed_at and data.since > si.closed_at ): logger.debug("Detected sub-issue %d closed in previous release.", si.number) - if len(list(si.get_sub_issues())) > 0: + if len(si_children) > 0: use_issue = True else: self.__registered_issues.add(siid) elif si.state == IssueRecord.ISSUE_STATE_OPEN: logger.debug("Detected sub-issue %d is still open.", si.number) - if len(list(si.get_sub_issues())) > 0: + if len(si_children) > 0: use_issue = True else: self.__registered_issues.add(siid)
139-151: Cross-repo detection from PR body likely misses owner/repo#N format.
extract_issue_numbers_from_body(pull, data.repository)only emits same-repo IDs. Consider enhancing the helper to parseowner/repo#123and full URLs.Would you like a focused patch for
release_notes_generator/utils/pull_request_utils.pyto support cross-repo patterns? Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
release_notes_generator/chapters/custom_chapters.py(2 hunks)release_notes_generator/record/factory/default_record_factory.py(4 hunks)release_notes_generator/record/factory/issue_hierarchy_record_factory.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- release_notes_generator/chapters/custom_chapters.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-28T13:20:42.353Z
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#173
File: release_notes_generator/record/factory/default_record_factory.py:115-117
Timestamp: 2025-09-28T13:20:42.353Z
Learning: In the DefaultRecordFactory class, the Github instance passed to __init__ is not stored as a class attribute, only used to create a rate limiter. For cross-repo issue handling, the Github instance needs to be stored to enable fetching issues from different repositories via github.get_repo().
Applied to files:
release_notes_generator/record/factory/default_record_factory.py
📚 Learning: 2025-09-28T13:07:31.335Z
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#173
File: release_notes_generator/record/factory/default_record_factory.py:0-0
Timestamp: 2025-09-28T13:07:31.335Z
Learning: In release_notes_generator/record/factory/default_record_factory.py, the user miroslavpojer prefers to keep unused parameters like `skip_rec` in the `register_pull_request` function for future usage, even when flagged by linters as unused.
Applied to files:
release_notes_generator/record/factory/default_record_factory.py
🧬 Code graph analysis (2)
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (5)
release_notes_generator/record/factory/default_record_factory.py (2)
generate(89-163)get_id(59-69)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/model/sub_issue_record.py (2)
issue(27-34)SubIssueRecord(15-36)release_notes_generator/utils/pull_request_utils.py (1)
extract_issue_numbers_from_body(32-52)release_notes_generator/model/hierarchy_issue_record.py (3)
HierarchyIssueRecord(16-158)sub_issues(47-51)sub_hierarchy_issues(54-58)
release_notes_generator/record/factory/default_record_factory.py (4)
release_notes_generator/generator.py (2)
rate_limiter(66-68)generate(70-110)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (1)
generate(59-137)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/utils/pull_request_utils.py (2)
extract_issue_numbers_from_body(32-52)get_issues_for_pr(56-81)
🪛 Ruff (0.13.1)
release_notes_generator/record/factory/default_record_factory.py
98-98: Unused function argument: skip_rec
(ARG001)
🔇 Additional comments (2)
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (1)
158-163: Cross-repo fetch is broken; fetch the parent issue from the repo encoded in the ID.
issue_idis repo-scoped but the code always queriesdata.repository. Useself._github.get_repo(owner/repo)and thenget_issue(number).- # dev note: here we expect that PR links to an issue in the same repository !!! - issue_number = int(issue_id.split("#")[1]) - parent_issue = self._safe_call(data.repository.get_issue)(issue_number) if data.repository else None + # Fetch from repository encoded in 'owner/repo#number' + owner_repo, num = issue_id.split("#", 1) + issue_number = int(num) + parent_repo = self._safe_call(self._github.get_repo)(owner_repo) if hasattr(self, "_github") else None + parent_issue = self._safe_call(parent_repo.get_issue)(issue_number) if parent_repo else Nonerelease_notes_generator/record/factory/default_record_factory.py (1)
51-57: Cross-repo fetch is broken; store Github client and fetch by owner/repo from the ID.Parent issue IDs are repo-scoped (owner/repo#number) but the code always queries
data.repository. Also, the passedGithubinstance isn’t stored, making cross-repo fetch impossible. Fix both.Apply:
def __init__(self, github: Github, home_repository: Repository) -> None: - rate_limiter = GithubRateLimiter(github) + self._github = github + rate_limiter = GithubRateLimiter(github) self._safe_call = safe_call_decorator(rate_limiter) self._home_repository = home_repository- # dev note: here we expect that PR links to an issue in the same repository !!! - parent_issue_number = int(parent_issue_id.split("#")[1]) - parent_issue = ( - self._safe_call(data.repository.get_issue)(parent_issue_number) if data.repository else None - ) + # Fetch issue from the repository encoded in 'owner/repo#number' + owner_repo, num = parent_issue_id.split("#", 1) + parent_issue_number = int(num) + parent_repo = self._safe_call(self._github.get_repo)(owner_repo) if hasattr(self, "_github") else None + parent_issue = ( + self._safe_call(parent_repo.get_issue)(parent_issue_number) if parent_repo else None + )Also applies to: 106-121
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (4)
56-57: Use a set for membership checks of processed sub-issues.
__local_sub_issues_checkedis a list but used for membership; switch to a set for O(1).- self.__local_sub_issues_checked: list[str] = [] + self.__local_sub_issues_checked: set[str] = set()- self.__local_sub_issues_checked.append(iid) + self.__local_sub_issues_checked.add(iid)Also applies to: 92-95
246-246: Minor: membership on dict keys.No need to call
.keys().- if self.get_id(issue) in self.__sub_issue_parents.keys(): # pylint: disable=consider-iterating-dictionary + if self.get_id(issue) in self.__sub_issue_parents: # pylint: disable=consider-iterating-dictionary
289-289: Type hint accepts SubIssue at call sites; widen annotation.
_create_record_for_sub_issueis called withSubIssue. Reflect that in the signature.- def _create_record_for_sub_issue(self, issue: Issue, issue_labels: Optional[list[str]] = None) -> None: + def _create_record_for_sub_issue(self, issue: Issue | SubIssue, issue_labels: Optional[list[str]] = None) -> None:
66-68: Clarify return-key format in docstring.Be explicit about the repo-scoped IDs to avoid misuse.
- dict[str, Record]: A dictionary of records indexed by their IDs. + dict[str, Record]: A dictionary of records keyed by 'owner/repo#number' (or commit SHA for commits).tests/release_notes/builder/test_release_notes_builder.py (1)
1588-1607: Add a test for cross-repo PR→Issue linking.To prevent regressions in #172, add a case where
get_issues_for_prreturns an issue from another repo and assert it gets fetched/registered.I can help draft a focused unit test that stubs:
get_issues_for_pr→ {"external/org#123"}self._github.get_repo("external/org").get_issue(123)→ mocked IssueWould you like me to generate this test scaffold?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
release_notes_generator/chapters/custom_chapters.py(2 hunks)release_notes_generator/record/factory/default_record_factory.py(4 hunks)release_notes_generator/record/factory/issue_hierarchy_record_factory.py(7 hunks)tests/conftest.py(17 hunks)tests/release_notes/builder/test_release_notes_builder.py(18 hunks)tests/release_notes/record/factory/test_default_record_factory.py(16 hunks)tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py(9 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-28T13:20:42.353Z
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#173
File: release_notes_generator/record/factory/default_record_factory.py:115-117
Timestamp: 2025-09-28T13:20:42.353Z
Learning: In the DefaultRecordFactory class, the Github instance passed to __init__ is not stored as a class attribute, only used to create a rate limiter. For cross-repo issue handling, the Github instance needs to be stored to enable fetching issues from different repositories via github.get_repo().
Applied to files:
release_notes_generator/record/factory/default_record_factory.pytests/release_notes/builder/test_release_notes_builder.py
📚 Learning: 2025-09-28T13:07:31.335Z
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#173
File: release_notes_generator/record/factory/default_record_factory.py:0-0
Timestamp: 2025-09-28T13:07:31.335Z
Learning: In release_notes_generator/record/factory/default_record_factory.py, the user miroslavpojer prefers to keep unused parameters like `skip_rec` in the `register_pull_request` function for future usage, even when flagged by linters as unused.
Applied to files:
release_notes_generator/record/factory/default_record_factory.py
🧬 Code graph analysis (7)
tests/conftest.py (1)
release_notes_generator/model/mined_data.py (1)
MinedData(38-56)
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (5)
release_notes_generator/record/factory/default_record_factory.py (2)
generate(89-171)get_id(59-69)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/model/sub_issue_record.py (2)
issue(27-34)SubIssueRecord(15-36)release_notes_generator/utils/pull_request_utils.py (1)
extract_issue_numbers_from_body(32-52)release_notes_generator/model/hierarchy_issue_record.py (3)
HierarchyIssueRecord(16-158)sub_issues(47-51)sub_hierarchy_issues(54-58)
release_notes_generator/record/factory/default_record_factory.py (5)
release_notes_generator/generator.py (2)
rate_limiter(66-68)generate(70-110)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (1)
generate(59-137)release_notes_generator/record/factory/record_factory.py (1)
generate(35-42)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/utils/pull_request_utils.py (2)
extract_issue_numbers_from_body(32-52)get_issues_for_pr(56-81)
tests/release_notes/builder/test_release_notes_builder.py (3)
tests/conftest.py (5)
mock_repo(95-98)custom_chapters_not_print_empty_chapters(76-84)mined_data_isolated_record_types_no_labels_no_type_defined(610-735)mined_data_isolated_record_types_with_labels_no_type_defined(739-774)mined_data_isolated_record_types_no_labels_with_type_defined(778-829)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (1)
IssueHierarchyRecordFactory(44-349)release_notes_generator/record/factory/default_record_factory.py (1)
DefaultRecordFactory(46-215)
tests/release_notes/record/factory/test_default_record_factory.py (3)
tests/conftest.py (4)
mock_repo(95-98)mock_get_issues_for_pr(58-61)mock_safe_call_decorator(50-55)wrapper(51-54)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/record/factory/default_record_factory.py (2)
DefaultRecordFactory(46-215)generate(89-171)
release_notes_generator/chapters/custom_chapters.py (4)
release_notes_generator/chapters/base_chapters.py (3)
populate(107-116)add_row(67-78)populated_record_numbers_list(43-49)release_notes_generator/model/record.py (5)
Record(29-199)record_id(72-77)is_present_in_chapters(45-51)contains_change_increment(121-127)to_chapter_row(109-118)release_notes_generator/action_inputs.py (1)
get_duplicity_scope(180-191)release_notes_generator/utils/enums.py (1)
DuplicityScopeEnum(24-32)
tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py (4)
tests/conftest.py (1)
mock_repo(95-98)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (2)
IssueHierarchyRecordFactory(44-349)generate(59-137)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/record/factory/record_factory.py (1)
generate(35-42)
🪛 Ruff (0.13.1)
tests/release_notes/record/factory/test_default_record_factory.py
182-182: Unpacked variable issue2 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
182-182: Unpacked variable pr2 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
274-274: Unpacked variable pr2 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
274-274: Unpacked variable commit1 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
274-274: Unpacked variable commit2 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
307-307: Unpacked variable pr2 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
307-307: Unpacked variable commit1 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
307-307: Unpacked variable commit2 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
344-344: Unused function argument: pull_number
(ARG001)
🪛 GitHub Actions: Build and Test
tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py
[error] 1-260: Test failures in IssueHierarchyRecordFactory: 4 assertions failed due to mismatch in expected keys. The tests expect keys including plain '123' and '124', as well as 'org/repo#121', 'org/repo#301'-'org/repo#304', and 'org/repo@merge_commit_sha_direct', but the produced keys are 'org/repo#121', 'org/repo#123', 'org/repo#124', 'org/repo#301'...'org/repo#304', 'org/repo@merge_commit_sha_direct'. Adjust test expectations or the key generation logic to align.
[error] 1-1: Command failed with exit code 1. Pytest run returned non-zero status due to failing tests.
🔇 Additional comments (22)
release_notes_generator/chapters/custom_chapters.py (2)
46-48: Docstring aligned with string keys. LGTM.Matches repo-scoped ID semantics.
74-86: Duplicity scope still blocked by outer guard.The outer
if not is_present_in_chaptersprevents duplicates even whenCUSTOM/BOTHis set. Merge the conditions once.- if ( - not records[record_id].is_present_in_chapters - and records[record_id].contains_change_increment() - ): - allow_dup = ActionInputs.get_duplicity_scope() in ( - DuplicityScopeEnum.CUSTOM, - DuplicityScopeEnum.BOTH, - ) - if (allow_dup or not records[record_id].is_present_in_chapters) and records[ - record_id - ].contains_change_increment(): - ch.add_row(record_id, records[record_id].to_chapter_row(True)) - self.populated_record_numbers_list.append(record_id) + allow_dup = ActionInputs.get_duplicity_scope() in ( + DuplicityScopeEnum.CUSTOM, + DuplicityScopeEnum.BOTH, + ) + if (allow_dup or not records[record_id].is_present_in_chapters) and \ + records[record_id].contains_change_increment(): + ch.add_row(record_id, records[record_id].to_chapter_row(True)) + self.populated_record_numbers_list.append(record_id)tests/release_notes/builder/test_release_notes_builder.py (1)
1588-1607: Tests updated to pass home_repository into factories.Constructor changes are correctly reflected; this safeguards ID generation consistency in tests.
Also applies to: 1621-1639, 1653-1671, 1684-1702, 1716-1734, 1747-1764, 1778-1796, 1810-1828
release_notes_generator/record/factory/issue_hierarchy_record_factory.py (1)
158-163: Cross-repo fetch uses home repo; breaks #172.
issue_idis repo-scoped, but the code fetches fromdata.repository. Use the repository encoded in the ID. Requires having aself._githubhandle (see additional snippet below).Apply:
- # dev note: here we expect that PR links to an issue in the same repository !!! - issue_number = int(issue_id.split("#")[1]) - parent_issue = self._safe_call(data.repository.get_issue)(issue_number) if data.repository else None + # Fetch from repository encoded in 'owner/repo#number' + owner_repo, num = issue_id.split("#", 1) + issue_number = int(num) + parent_repo = self._safe_call(self._github.get_repo)(owner_repo) if hasattr(self, "_github") else None + parent_issue = self._safe_call(parent_repo.get_issue)(issue_number) if parent_repo else NoneAdditionally (outside this file), store the Github client so the above works:
# release_notes_generator/record/factory/default_record_factory.py class DefaultRecordFactory(RecordFactory): def __init__(self, github: Github, home_repository: Repository) -> None: - rate_limiter = GithubRateLimiter(github) + self._github = github + rate_limiter = GithubRateLimiter(github) self._safe_call = safe_call_decorator(rate_limiter) self._home_repository = home_repositoryBased on learnings.
tests/conftest.py (4)
23-23: LGTM! Correct import for Commit type.The addition of
Committo the imports is appropriate for the new mock commit fixtures that propagate repository context.
150-151: Repository context propagation looks good.The consistent addition of
issue.repository.full_nameandissue.closed_atacross all mock fixtures appropriately supports the new repository-scoped ID system.Also applies to: 169-170, 189-190, 207-209, 225-227, 247-249, 267-268, 286-287, 306-308, 331-333, 355-357, 376-378, 400-402
601-606: Verify repository context for mock commits.The mock commit fixture now includes
commit.repository.full_name, aligning with the repository-scoped commit ID format.
610-611: MinedData initialization properly updated.The fixture now correctly passes
mock_repotoMinedDataconstructor, consistent with the repository requirement changes.Also applies to: 621-621
tests/release_notes/record/factory/test_default_record_factory.py (7)
86-87: Repository context properly added to mock issues.Mock issues now include
repository.full_name = "org/repo"to support the new string-based ID format.Also applies to: 96-97, 111-112, 121-122
155-156: Mock commits properly initialized with repository context.Mock commits now have the repository reference required for the new commit ID format.
Also applies to: 161-162
172-177: Test helper functions updated for string IDs.The
mock_get_issues_for_prnow returns string-based IDs in the format'org/repo#number', consistent with the new ID system.
189-190: MinedData and factory initialization properly updated.Tests correctly pass
mock_repoto bothMinedDataandDefaultRecordFactoryconstructors.Also applies to: 197-198
202-207: Test assertions properly verify string-based record keys.The test now correctly verifies that records are keyed by string IDs like
'org/repo#1'and casts to appropriate record types.
267-267: Commit record access properly uses string ID.The test correctly accesses the commit record using the string ID format.
348-358: Test properly updated for PR-only scenarios with string keys.Tests correctly use string-based record keys like
'org/repo#101'for pull request records.Also applies to: 363-365
tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py (1)
32-35: Factory and MinedData initialization properly updated.Tests correctly pass
mock_repoto bothIssueHierarchyRecordFactoryandMinedDataconstructors.Also applies to: 48-49, 58-58
release_notes_generator/record/factory/default_record_factory.py (6)
22-22: Good use of singledispatchmethod for type-based ID generation.The addition of
singledispatchmethodandlru_cacheprovides a clean, extensible pattern for generating IDs based on object type with efficient caching.
51-54: Constructor properly updated for repository context.The addition of
home_repositoryparameter aligns with the PR objectives for repository-scoped ID support.
58-88: Well-designed ID generation system.The
singledispatchmethodimplementation provides clear, type-safe ID generation with appropriate caching for performance. The ID formats (owner/repo#numberfor issues/PRs andowner/repo@shafor commits) properly support the cross-repository feature objectives.
195-195: Commit record properly uses get_id method.Good consistency in using the unified ID format for commit records.
98-141: Good fallback logic for unattached PRs.The addition of
attached_anytracking and fallback PR record creation ensures that PRs with unresolvable links aren't lost.
118-122: Fix cross-repository issue fetching.The current implementation always fetches issues from
data.repository, which will fail for cross-repository references. The comment acknowledges this limitation but doesn't address it.Based on learnings
You need to store the GitHub instance and parse the repository from the ID:
def __init__(self, github: Github, home_repository: Repository) -> None: rate_limiter = GithubRateLimiter(github) self._safe_call = safe_call_decorator(rate_limiter) self._home_repository = home_repository + self._github = github self._records: dict[str, Record] = {}Then update the issue fetching logic:
- # dev note: here we expect that PR links to an issue in the same repository !!! - parent_issue_number = int(parent_issue_id.split("#")[1]) - parent_issue = ( - self._safe_call(data.repository.get_issue)(parent_issue_number) if data.repository else None - ) + # Parse repository and issue number from cross-repo ID + repo_part, issue_num = parent_issue_id.split("#", 1) + parent_issue_number = int(issue_num) + + # Get the appropriate repository + if "/" in repo_part and repo_part != data.repository.full_name: + # Cross-repo reference + target_repo = self._safe_call(self._github.get_repo)(repo_part) + parent_issue = self._safe_call(target_repo.get_issue)(parent_issue_number) if target_repo else None + else: + # Same repo reference + parent_issue = self._safe_call(data.repository.get_issue)(parent_issue_number) if data.repository else None
release_notes_generator/record/factory/issue_hierarchy_record_factory.py
Outdated
Show resolved
Hide resolved
| mocker.patch("release_notes_generator.record.factory.default_record_factory.get_issues_for_pr", return_value=['org/repo#2']) | ||
| records = DefaultRecordFactory(mock_github_client, mock_repo).generate(data) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Test properly mocks cross-repository issue fetching.
The patch correctly mocks get_issues_for_pr to return string-based IDs.
However, the test doesn't actually verify the cross-repository behavior mentioned in the AI summary. Consider adding a test case that explicitly tests cross-repo issue references.
Also applies to: 319-320
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for TODO comments in the test file and related files
echo "=== Searching for TODO comments in the test file ==="
rg -n "TODO" tests/release_notes/record/factory/test_default_record_factory.py
echo -e "\n=== Searching for recent TODO additions in test files ==="
rg -n -C2 "TODO.*cross.*repo|TODO.*cross-repo|TODO.*repository" tests/
echo -e "\n=== General TODO search in test factory files ==="
rg -n "TODO" tests/release_notes/record/factory/
Length of output: 162
🏁 Script executed:
#!/bin/bash
# Search for all TODO comments in the codebase
echo "=== All TODO comments in the codebase ==="
rg -n "TODO" --type py
echo -e "\n=== Recent git log to see if TODO was added in recent commits ==="
git log --oneline -10 --grep="TODO"
echo -e "\n=== Check if there are any uncommitted changes with TODO ==="
git status --porcelain
echo -e "\n=== Search for any TODO in git diff (staged and unstaged) ==="
git diff HEAD -- "*.py" | grep -n "TODO" || echo "No TODO found in git diff"
Length of output: 1983
@miroslavpojer, I don't see any new TODO comment added to the codebase. My search across all Python files shows existing TODOs, but none related to cross-repository testing or in the test file we were discussing.
Could you clarify where you added the TODO? If you haven't committed the change yet, you might want to:
- Add a TODO comment in the test file
tests/release_notes/record/factory/test_default_record_factory.py - Or let me know if you added it in a different location
For reference, the TODO could be something like:
# TODO: Add test case for cross-repository issue references to verify behavior mentioned in PR objectivesThis would help future developers understand that cross-repo functionality needs more comprehensive test coverage.
| def mock_get_issues_for_pr_no_issues(pull_number: str) -> list[str]: | ||
| return [] |
There was a problem hiding this comment.
Fix unused parameter in helper function.
The pull_number parameter is not used in the function body.
Apply this diff to fix the unused parameter:
-def mock_get_issues_for_pr_no_issues(pull_number: str) -> list[str]:
+def mock_get_issues_for_pr_no_issues(pull_number: int) -> list[str]:Note: The parameter should be int (not str) to match the actual function signature, even though it's unused.
📝 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 mock_get_issues_for_pr_no_issues(pull_number: str) -> list[str]: | |
| return [] | |
| def mock_get_issues_for_pr_no_issues(pull_number: int) -> list[str]: | |
| return [] |
🧰 Tools
🪛 Ruff (0.13.1)
344-344: Unused function argument: pull_number
(ARG001)
🤖 Prompt for AI Agents
In tests/release_notes/record/factory/test_default_record_factory.py around
lines 344-345, the helper's parameter is unused and typed as str; update the
function signature to accept an int (matching the real signature) and mark it as
intentionally unused by renaming the parameter to start with an underscore
(e.g., _pull_number) so linters won't flag it, leaving the body returning an
empty list.
There was a problem hiding this comment.
@miroslavpojer Thank you for fixing the unused parameter issue! I appreciate you addressing the linter warning.
🧠 Learnings used
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#173
File: release_notes_generator/record/factory/default_record_factory.py:0-0
Timestamp: 2025-09-28T13:07:31.335Z
Learning: In release_notes_generator/record/factory/default_record_factory.py, the user miroslavpojer prefers to keep unused parameters like `skip_rec` in the `register_pull_request` function for future usage, even when flagged by linters as unused.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
I have returned this back, as this introduce failing unit tests. Resolved for me.
There was a problem hiding this comment.
@miroslavpojer Thank you for the feedback! I understand that the suggested change caused test failures, and I appreciate you reverting it to maintain the working test suite.
It's completely reasonable to keep the current implementation if changing the parameter type breaks the tests - the functionality and test reliability take priority over linter warnings. I respect your decision to resolve it this way.
✏️ Learnings added
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#173
File: tests/release_notes/record/factory/test_default_record_factory.py:0-0
Timestamp: 2025-09-28T14:09:54.315Z
Learning: In tests/release_notes/record/factory/test_default_record_factory.py, the mock_get_issues_for_pr_no_issues helper function parameter should remain as `str` type (not changed to `int`) because changing it to `int` causes test failures, even though the parameter is unused and flagged by linters.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: miroslavpojer
PR: AbsaOSS/generate-release-notes#173
File: release_notes_generator/record/factory/default_record_factory.py:0-0
Timestamp: 2025-09-28T13:07:31.335Z
Learning: In release_notes_generator/record/factory/default_record_factory.py, the user miroslavpojer prefers to keep unused parameters like `skip_rec` in the `register_pull_request` function for future usage, even when flagged by linters as unused.
tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py
Outdated
Show resolved
Hide resolved
tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py
Outdated
Show resolved
Hide resolved
tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py
Outdated
Show resolved
Hide resolved
tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py (6)
32-41: Constructor update to pass home_repository looks correct; tweak assertion style.Use len(result) instead of len(result.values()).
- assert 0 == len(result.values()) + assert len(result) == 0
48-59: Repeatable setup: extract a fixture for rate‑limit + decorator patching.The GitHub client/rate‑limit wiring and safe_call_decorator patch repeat across tests; move into a pytest fixture to DRY.
Example (add to tests/conftest.py):
@pytest.fixture def github_with_rate_limit(mocker): gh = mocker.Mock(spec=Github) rl = mocker.Mock() rl.rate.remaining = 10 rl.rate.reset.timestamp.return_value = time.time() + 3600 gh.get_rate_limit.return_value = rl mocker.patch( "release_notes_generator.record.factory.default_record_factory.safe_call_decorator", side_effect=mock_safe_call_decorator, ) return ghThen in tests: use github_with_rate_limit and drop per‑test patching.
62-73: Assert exact key set instead of subset+len combo.Direct equality is clearer and fails with a precise diff.
- assert 8 == len(result) - assert {'org/repo#121', 'org/repo#301', 'org/repo#302', 'org/repo#303', 'org/repo#304', 'org/repo#123', 'org/repo#124', "org/repo@merge_commit_sha_direct"}.issubset(result.keys()) + expected_keys = { + 'org/repo#121', 'org/repo#301', 'org/repo#302', 'org/repo#303', + 'org/repo#304', 'org/repo#123', 'org/repo#124', 'org/repo@merge_commit_sha_direct' + } + assert set(result.keys()) == expected_keysApply similarly to other tests below.
84-90: Top‑level level() consistency.Optional: also assert rec_hi_2.level == 0 for parity with other top‑level hierarchy issues.
rec_hi_2 = cast(HierarchyIssueRecord, result['org/repo#302']) assert 1 == rec_hi_2.pull_requests_count() assert 0 == len(rec_hi_2.sub_hierarchy_issues.values()) assert 1 == len(rec_hi_2.sub_issues.values()) assert 1 == rec_hi_2.sub_issues['org/repo#451'].pull_requests_count() + assert 0 == rec_hi_2.level
99-109: Remove redundant variable rec_hi_5.Reuse rec_hi_4 for the nested level assertion.
- rec_hi_5 = cast(HierarchyIssueRecord, result['org/repo#304']) - assert 1 == rec_hi_5.sub_hierarchy_issues['org/repo#350'].level + assert 1 == rec_hi_4.sub_hierarchy_issues['org/repo#350'].level
32-298: Add explicit cross‑repo sub‑issue coverage.Given objective #172, add a test that injects a cross‑repo sub‑issue (e.g., 'other/repo#999') and asserts it’s appended under the correct parent with the fully‑qualified ID.
I can sketch a minimal pytest test + helper mocks to simulate a cross‑repo sub‑issue linkage if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py (4)
tests/conftest.py (5)
mock_repo(95-98)mined_data_isolated_record_types_no_labels_no_type_defined(610-735)mined_data_isolated_record_types_with_labels_no_type_defined(739-774)mined_data_isolated_record_types_no_labels_with_type_defined(778-829)mined_data_isolated_record_types_with_labels_with_type_defined(833-863)release_notes_generator/record/factory/issue_hierarchy_record_factory.py (2)
IssueHierarchyRecordFactory(44-349)generate(59-137)release_notes_generator/model/mined_data.py (1)
MinedData(38-56)release_notes_generator/record/factory/default_record_factory.py (1)
generate(89-171)
🔇 Additional comments (8)
tests/release_notes/record/factory/test_issue_hierarchy_record_factory.py (8)
74-83: Hierarchy 301 expectations aligned (1 sub‑issue).This addresses the earlier failure (“expected 2, got 1”). Current assertions for org/repo#450 and level 0 look good.
91-97: Commit association on sub‑issue 452 is validated.Good coverage tying PR 151’s merge commit to the sub‑issue.
118-129: Same setup duplication as above.Apply the shared fixture here as well.
132-143: Use exact key equality here too.Mirror the earlier assertion refactor for clarity.
181-206: Third scenario: mirror the key‑equality assertion + shared setup.Consistent patterns improve readability and failure diffs.
244-269: Fourth scenario: same refactors apply.Adopt the shared fixture and exact key‑set assertion.
270-298: Deeply nested commit linkage validated.The check for PR 152’s merge commit under sub‑hierarchy 350 → sub‑issue 453 is good.
32-298: Sanity checks passed: no numeric-only record keys or missinghome_repositorywiring detected.
1 participant
Release Notes:
Summary by CodeRabbit
New Features
Improvements
Fixes #171
Fixes #172