-
Notifications
You must be signed in to change notification settings - Fork 21
Our agents can handle multiple patches for a single Jira #115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -86,7 +86,7 @@ def get_instructions() -> str: | |||||
| return """ | ||||||
| You are an expert on backporting upstream patches to packages in RHEL ecosystem. | ||||||
|
|
||||||
| To backport upstream fix <UPSTREAM_FIX> to package <PACKAGE> in dist-git branch <DIST_GIT_BRANCH>, do the following: | ||||||
| To backport upstream patches <UPSTREAM_PATCHES> to package <PACKAGE> in dist-git branch <DIST_GIT_BRANCH>, do the following: | ||||||
|
|
||||||
| CRITICAL: Do NOT modify, delete, or touch any existing patches in the dist-git repository. | ||||||
| Only add new patches for the current backport. Existing patches are there for a reason | ||||||
|
|
@@ -97,7 +97,7 @@ def get_instructions() -> str: | |||||
| end the process with `success=True` and `status="Backport already applied"`. | ||||||
|
|
||||||
| 2. Use the `git_prepare_package_sources` tool to prepare package sources in directory <UNPACKED_SOURCES> | ||||||
| for application of the upstream fix. | ||||||
| for application of the upstream patch. | ||||||
|
|
||||||
| 3. Determine which backport approach to use: | ||||||
|
|
||||||
|
|
@@ -108,7 +108,7 @@ def get_instructions() -> str: | |||||
| - <UPSTREAM_REPO>: A temporary upstream repository clone (created in step 3c with -upstream suffix) | ||||||
|
|
||||||
| When to use this workflow: | ||||||
| - <UPSTREAM_FIX> is a commit or pull request URL | ||||||
| - <UPSTREAM_PATCHES> is a list of commit or pull request URLs | ||||||
| - This includes URLs with .patch suffix (e.g., https://github.com/.../commit/abc123.patch) | ||||||
| - If URL extraction fails, fall back to approach B | ||||||
|
|
||||||
|
|
@@ -218,22 +218,26 @@ def get_instructions() -> str: | |||||
|
|
||||||
| B. GIT AM WORKFLOW (Fallback approach): | ||||||
|
|
||||||
| Note: For this workflow, use the pre-downloaded patch file at {{local_clone}}/{{jira_issue}}.patch | ||||||
| Note: For this workflow, use the pre-downloaded patch files in the current working directory. | ||||||
| They are called `<JIRA_ISSUE>-<N>.patch` where <N> is a 0-based index. For example, | ||||||
| for a `RHEL-12345` Jira issue the first patch would be called `RHEL-12345-0.patch`. | ||||||
|
|
||||||
| 3a. Backport the patch: | ||||||
| - Use the `git_patch_apply` tool with the patch file: {{local_clone}}/{{jira_issue}}.patch | ||||||
| Backport all patches individually using the steps 3a and 3b below. | ||||||
|
|
||||||
| 3a. Backport one patch at a time using the following steps: | ||||||
| - Use the `git_patch_apply` tool with the patch file: <JIRA_ISSUE>-<N>.patch | ||||||
| - Resolve all conflicts and leave the repository in a dirty state. Delete all *.rej files. | ||||||
| - Use the `git_apply_finish` tool to finish the patch application. | ||||||
|
|
||||||
| 3b. Once there are no more conflicts, use the `git_patch_create` tool with the patch file path | ||||||
| {{local_clone}}/{{jira_issue}}.patch to update the patch file. | ||||||
| <JIRA_ISSUE>-<N>.patch to update the patch file. | ||||||
|
|
||||||
| 4. Update the spec file. Add a new `Patch` tag pointing to the <UPSTREAM_FIX> patch file. | ||||||
| 4. Update the spec file. Add a new `Patch` tag for every patch in <UPSTREAM_PATCHES>. | ||||||
| Add the new `Patch` tag after all existing `Patch` tags and, if `Patch` tags are numbered, | ||||||
| make sure it has the highest number. Make sure the patch is applied in the "%prep" section | ||||||
| and the `-p` argument is correct. Add an upstream URL as a comment above | ||||||
| the `Patch:` tag - this URL references the related upstream commit or a pull/merge request. | ||||||
| Default to <UPSTREAM_FIX> if it is an URL. | ||||||
| Include every patch defined in <UPSTREAM_PATCHES> list. | ||||||
| IMPORTANT: Only ADD new patches. Do NOT modify existing Patch tags or their order. | ||||||
|
|
||||||
| 5. Run `centpkg --name=<PACKAGE> --namespace=rpms --release=<DIST_GIT_BRANCH> prep` to see if the new patch | ||||||
|
|
@@ -269,7 +273,10 @@ def get_prompt() -> str: | |||||
| {{dist_git_branch}} dist-git branch has been checked out. You are working on Jira issue {{jira_issue}} | ||||||
| {{#cve_id}}(a.k.a. {{.}}){{/cve_id}}. | ||||||
| {{^build_error}} | ||||||
| Backport upstream fix {{upstream_fix}}. | ||||||
| Backport upstream patches: | ||||||
| {{#upstream_patches}} | ||||||
| - {{.}} | ||||||
| {{/upstream_patches}} | ||||||
| Unpacked upstream sources are in {{unpacked_sources}}. | ||||||
| {{/build_error}} | ||||||
| {{#build_error}} | ||||||
|
|
@@ -289,7 +296,12 @@ def get_fix_build_error_prompt() -> str: | |||||
| {{dist_git_branch}} dist-git branch has been checked out. You are working on Jira issue {{jira_issue}} | ||||||
| {{#cve_id}}(a.k.a. {{.}}){{/cve_id}}. | ||||||
|
|
||||||
| The backport of {{upstream_fix}} was initially successful using the cherry-pick workflow, | ||||||
| Upstream patches that were backported: | ||||||
| {{#upstream_patches}} | ||||||
| - {{.}} | ||||||
| {{/upstream_patches}} | ||||||
|
|
||||||
| The backport of upstream patches was initially successful using the cherry-pick workflow, | ||||||
| but the build failed with the following error: | ||||||
|
|
||||||
| {{build_error}} | ||||||
|
|
@@ -492,7 +504,7 @@ async def main() -> None: | |||||
| local_tool_options = {"working_directory": None} | ||||||
|
|
||||||
| class State(PackageUpdateState): | ||||||
| upstream_fix: str | ||||||
| upstream_patches: list[str] | ||||||
| cve_id: str | None | ||||||
| unpacked_sources: Path | None = Field(default=None) | ||||||
| backport_log: list[str] = Field(default=[]) | ||||||
|
|
@@ -502,7 +514,7 @@ class State(PackageUpdateState): | |||||
| incremental_fix_attempts: int = Field(default=0) # Track how many times we tried incremental fix | ||||||
|
|
||||||
| async def run_workflow( | ||||||
| package, dist_git_branch, upstream_fix, jira_issue, cve_id, redis_conn=None | ||||||
| package, dist_git_branch, upstream_patches, jira_issue, cve_id, redis_conn=None | ||||||
| ): | ||||||
| local_tool_options["working_directory"] = None | ||||||
|
|
||||||
|
|
@@ -545,11 +557,14 @@ async def fork_and_prepare_dist_git(state): | |||||
| state.unpacked_sources = get_unpacked_sources(state.local_clone, state.package) | ||||||
| timeout = aiohttp.ClientTimeout(total=30) | ||||||
| async with aiohttp.ClientSession(timeout=timeout) as session: | ||||||
| async with session.get(state.upstream_fix) as response: | ||||||
| if response.status < 400: | ||||||
| (state.local_clone / f"{state.jira_issue}.patch").write_text(await response.text()) | ||||||
| else: | ||||||
| raise ValueError(f"Failed to fetch upstream fix: {response.status}") | ||||||
| for idx, upstream_patch in enumerate(state.upstream_patches): | ||||||
| # should we guess the patch name with log agent? | ||||||
| patch_name = f"{state.jira_issue}-{idx}.patch" | ||||||
| async with session.get(upstream_patch) as response: | ||||||
| if response.status < 400: | ||||||
| (state.local_clone / patch_name).write_text(await response.text()) | ||||||
| else: | ||||||
| raise ValueError(f"Failed to fetch upstream patch: {response.status}") | ||||||
| return "run_backport_agent" | ||||||
|
|
||||||
| async def run_backport_agent(state): | ||||||
|
|
@@ -563,7 +578,7 @@ async def run_backport_agent(state): | |||||
| dist_git_branch=state.dist_git_branch, | ||||||
| jira_issue=state.jira_issue, | ||||||
| cve_id=state.cve_id, | ||||||
| upstream_fix=state.upstream_fix, | ||||||
| upstream_patches=state.upstream_patches, | ||||||
| build_error=state.build_error, | ||||||
| ), | ||||||
| ), | ||||||
|
|
@@ -624,7 +639,7 @@ async def fix_build_error(state): | |||||
| dist_git_branch=state.dist_git_branch, | ||||||
| jira_issue=state.jira_issue, | ||||||
| cve_id=state.cve_id, | ||||||
| upstream_fix=state.upstream_fix, | ||||||
| upstream_patches=state.upstream_patches, | ||||||
| build_error=state.build_error, | ||||||
| ), | ||||||
| ), | ||||||
|
|
@@ -774,21 +789,22 @@ async def run_log_agent(state): | |||||
| log_output=log_output, | ||||||
| operation_type="backport", | ||||||
| package=state.package, | ||||||
| details=state.upstream_fix, | ||||||
| details=str(state.upstream_patches), | ||||||
| ) | ||||||
| state.log_result = log_output | ||||||
|
|
||||||
| return "stage_changes" | ||||||
|
|
||||||
| async def commit_push_and_open_mr(state): | ||||||
| try: | ||||||
| formatted_patches = "\n".join(f" - {p}" for p in state.upstream_patches) | ||||||
| state.merge_request_url = await tasks.commit_push_and_open_mr( | ||||||
| local_clone=state.local_clone, | ||||||
| commit_message=( | ||||||
| f"{state.log_result.title}\n\n" | ||||||
| f"{state.log_result.description}\n\n" | ||||||
| + (f"CVE: {state.cve_id}\n" if state.cve_id else "") | ||||||
| + f"Upstream fix: {state.upstream_fix}\n" | ||||||
| + "Upstream patches:\n" + formatted_patches + "\n" | ||||||
| + f"Resolves: {state.jira_issue}\n\n" | ||||||
| f"This commit was backported {I_AM_JOTNAR}\n\n" | ||||||
| "Assisted-by: Jotnar\n" | ||||||
|
|
@@ -801,7 +817,7 @@ async def commit_push_and_open_mr(state): | |||||
| f"This merge request was created {I_AM_JOTNAR}\n" | ||||||
| f"{CAREFULLY_REVIEW_CHANGES}\n\n" | ||||||
| f"{state.log_result.description}\n\n" | ||||||
| f"Upstream patch: {state.upstream_fix}\n\n" | ||||||
| + "Upstream patches:\n" + formatted_patches + "\n" | ||||||
| f"Resolves: {state.jira_issue}\n\n" | ||||||
| f"Backporting steps:\n\n{state.backport_log[-1]}" | ||||||
| ), | ||||||
|
|
@@ -849,7 +865,7 @@ async def comment_in_jira(state): | |||||
| State( | ||||||
| package=package, | ||||||
| dist_git_branch=dist_git_branch, | ||||||
| upstream_fix=upstream_fix, | ||||||
| upstream_patches=upstream_patches, | ||||||
| jira_issue=jira_issue, | ||||||
| cve_id=cve_id, | ||||||
| ), | ||||||
|
|
@@ -859,14 +875,15 @@ async def comment_in_jira(state): | |||||
| if ( | ||||||
| (package := os.getenv("PACKAGE", None)) | ||||||
| and (branch := os.getenv("BRANCH", None)) | ||||||
| and (upstream_fix := os.getenv("UPSTREAM_FIX", None)) | ||||||
| and (upstream_patches_raw := os.getenv("UPSTREAM_PATCHES", None)) | ||||||
| and (jira_issue := os.getenv("JIRA_ISSUE", None)) | ||||||
| ): | ||||||
| upstream_patches = upstream_patches_raw.split(",") | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it can't be empty mate |
||||||
| logger.info("Running in direct mode with environment variables") | ||||||
| state = await run_workflow( | ||||||
| package=package, | ||||||
| dist_git_branch=branch, | ||||||
| upstream_fix=upstream_fix, | ||||||
| upstream_patches=upstream_patches, | ||||||
| jira_issue=jira_issue, | ||||||
| cve_id=os.getenv("CVE_ID", None), | ||||||
| redis_conn=None, | ||||||
|
|
@@ -928,7 +945,7 @@ async def retry(task, error): | |||||
| state = await run_workflow( | ||||||
| package=backport_data.package, | ||||||
| dist_git_branch=dist_git_branch, | ||||||
| upstream_fix=backport_data.patch_url, | ||||||
| upstream_patches=backport_data.patch_urls, | ||||||
| jira_issue=backport_data.jira_issue, | ||||||
| cve_id=backport_data.cve_id, | ||||||
| redis_conn=redis, | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -95,7 +95,8 @@ class BackportInputSchema(BaseModel): | |||||||||||
| dist_git_branch: str = Field(description="Git branch in dist-git to be updated") | ||||||||||||
| jira_issue: str = Field(description="Jira issue to reference as resolved") | ||||||||||||
| cve_id: str | None = Field(default=None, description="CVE ID if the jira issue is a CVE") | ||||||||||||
| upstream_fix: str = Field(description="URL to the upstream fix (commit URL or patch URL)") | ||||||||||||
| upstream_patches: list[str] = Field( | ||||||||||||
| description="List of URLs to upstream patches that were validated using the PatchValidator tool") | ||||||||||||
| build_error: str | None = Field(description="Error encountered during package build") | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
|
|
@@ -131,7 +132,8 @@ class RebaseData(BaseModel): | |||||||||||
| class BackportData(BaseModel): | ||||||||||||
| """Data for backport resolution.""" | ||||||||||||
| package: str = Field(description="Package name") | ||||||||||||
| patch_url: str = Field(description="URL to the source of the fix that was validated using PatchValidator tool") | ||||||||||||
| patch_urls: list[str] = Field( | ||||||||||||
| description="A list of URLs to the sources of the fixes that were validated using the PatchValidator tool") | ||||||||||||
|
Comment on lines
+135
to
+136
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change from a single For example, Additionally, to improve robustness, you should ensure that
Suggested change
Comment on lines
+135
to
+136
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay, I didn't know we are following some syntax 🤦
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought you meant the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added the list syntax.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No, that's just a way how to express variables/placeholders, it's not defined anywhere. |
||||||||||||
| justification: str = Field(description="Clear explanation of why this patch fixes the issue, linking it to the root cause") | ||||||||||||
| jira_issue: str = Field(description="Jira issue identifier") | ||||||||||||
| cve_id: str | None = Field(description="CVE identifier", default=None) | ||||||||||||
|
|
@@ -188,9 +190,10 @@ def format_for_comment(self) -> str: | |||||||||||
| case BackportData(): | ||||||||||||
| fix_version_text = f"\n*Fix Version*: {self.data.fix_version}" if self.data.fix_version else "" | ||||||||||||
|
|
||||||||||||
| patch_urls_text = "\n".join([f"*Patch URL {i+1}*: {url}" for i, url in enumerate(self.data.patch_urls)]) | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For better readability in the generated Jira comment, it would be good to handle the case of a single patch URL differently. When there's only one URL, using
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unlike Gemini, I would rather suggest a formatted list: (or whatever Jira supports). |
||||||||||||
| return ( | ||||||||||||
| f"{resolution}" | ||||||||||||
| f"*Patch URL*: {self.data.patch_url}\n" | ||||||||||||
| f"{patch_urls_text}\n" | ||||||||||||
| f"*Justification*: {self.data.justification}" | ||||||||||||
| f"{fix_version_text}" | ||||||||||||
| ) | ||||||||||||
|
|
@@ -271,4 +274,4 @@ class CachedMRMetadata(BaseModel): | |||||||||||
| operation_type: str = Field(description="Type of operation (backport or rebase)") | ||||||||||||
| title: str = Field(description="Merge request title") | ||||||||||||
| package: str = Field(description="Package name") | ||||||||||||
| details: str = Field(description="Operation-specific identifier (upstream_fix URL for backport, version for rebase)") | ||||||||||||
| details: str = Field(description="Operation-specific identifier (list of upstream patch URLs for backport, version for rebase)") | ||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example should point out that the patches are supposed to be comma-delimited.