Phase 1# of postponed issues reprocessing - #746
Conversation
PR Summary by QodoAdd categorized postponed resolutions and sweep jobs to reprocess blocked issues
AI Description
Diagram
High-Level Assessment
Files changed (40)
|
Code Review by Qodo
1.
|
![qodo-for-packit[bot]](https://avatars.githubusercontent.com/in/4066991?s=60&v=4){kind=small}
3f80e7a to
a15d4fa
Compare
b558595 to
041c13b
Compare
lbarcziova
left a comment
lbarcziova
left a comment
There was a problem hiding this comment.
I see some great improvements here, I really like the prompt changes; just few (hopefully last) notes, and something raised by Claude:
1. GitLab Bearer token sent to any gitlab.* host (security, low-severity)
_GITLAB_MR_RE = re.compile(r"https://(gitlab\.[^/]+)/...") still accepts any gitlab.* domain. The extracted host is passed to gitlab_api_get(gitlab_url=f"https://{host}"), which appends the Bearer token to
the request. A crafted blocker_reference pointing to a rogue gitlab.attacker.com host would exfiltrate the token. jpodivin deflected to the supervisor layer but the risk sits in pr_pending.py — the regex
is where the host is extracted and trusted. Since blocker_reference values come from Jira comments written by the LLM (not from a curated allowlist), this is exploitable if a Jira comment is compromised or
the LLM hallucinates a hostile URL.
We could have a whitelist for this. But it will add to the maintenance burden. |
633f2a3 to
f47352b
Compare
majamassarini
left a comment
majamassarini
left a comment
There was a problem hiding this comment.
Beside the comment below and this small finding from Claude, this LGTM.
PostponedData.blocker_references semantically required but schema-optional (ymir/common/models.py:1167)
Field is list[str] | None with default=None, but PRPendingSweep returns an error if None for postponed_pr_pending issues. The schema doesn't enforce the semantic requirement.
Fix: Either make the field required in the schema or handle None gracefully in PRPendingSweep.
| @@ -0,0 +1,9 @@ | |||
| # Required: Jira instance URL | |||
There was a problem hiding this comment.
I am wondering if, instead of creating this new secret file, we should reuse the jira-issue-fetcher.env and potentially rename it - if clearer. This would limit the places that need to be updated.
There was a problem hiding this comment.
But some of the vars in that file, like QUERY, have nothing to do in sweep jobs.
Using a single file for both, would lead to confusion regarding what the cronjobs need. Not just for people, agents too.
Having a single file for two services, would also mean that variables for issue fetcher, would get in sweep containers. Finally, while there is already divergence today, there is no guarantee that it won't be even greater in the future. And then we would have to create a new file anyway.
That is not entirely correct. The |
6ccf63f to
052aab9
Compare
lbarcziova
left a comment
lbarcziova
left a comment
There was a problem hiding this comment.
I think we are close to merging this, thanks for all the changes! Few last things to consider:
- The sweep JQL only filters by label, I think we should we add a status gate to skip issues that are already Closed too (to prevent scenarios where maintainer fixes the Jira before our sweep gets to it)
- Can we adjust the triage comment for postponed resolutions to mention that the issue will be picked up for reprocessing periodically by the sweep? (we can also communicate this on rhel-devel after tested in prod)
- Do you still plan to re-label and adjust the comments on existing postponed issues with ymir_triaged_postponed? What's the timeline for this and merging this PR
- the no_patch path will still post a new Jira comment on every sweep cycle iiuc when the agent still finds no patch. Could we add a guard to check if there is the Ymir comment with the same resolution already and skip commenting for that case?
- Task.from_issue(issue_key) in on_unblock() always uses the default user_triggered=False. Is this intentional? If a maintainer originally triggered triage via
ymir_todoand the issue ends up postponed and later re-queued by the sweep, they won't get a comment update when triage completes. Seems probably minor, but we could eventually store theuser_triggered
In order:
|
|
thanks, sounds good, as for 2, I overlooked that, the message looks sufficient |
Signed-off-by: Jiri Podivin <jpodivin@redhat.com>
Signed-off-by: Jiri Podivin <jpodivin@redhat.com>
Add ymir_abandoned label Signed-off-by: Jiri Podivin <jpodivin@redhat.com>
Sweep cronjob is using a container image, similar to the one for supervisor. Signed-off-by: Jiri Podivin <jpodivin@redhat.com> Assisted-by: Claude Opus 4.6 via Claude Code
Signed-off-by: Jiri Podivin <jpodivin@redhat.com> Assisted-by: Claude Sonnet 4.6 via Claude Code
Signed-off-by: Jiri Podivin <jpodivin@redhat.com>
Assisted-by: Claude Sonnet 4.6 via Claude Code Signed-off-by: Jiri Podivin <jpodivin@redhat.com>
Signed-off-by: Jiri Podivin <jpodivin@redhat.com>
Assisted-by: Claude Sonnet 4.6 via Claude Code Signed-off-by: Jiri Podivin <jpodivin@redhat.com>
lbarcziova
left a comment
lbarcziova
left a comment
There was a problem hiding this comment.
LGTM! 🚀
Given the scope of this change, we should coordinate the deployment and monitoring with Skald. Do you think it's safe to deploy this week, given the long weekend - will we have time to catch and fix any follow-up issues? If you see meaningful risk here, I'd lean toward deploying Monday instead.
| return Workflow.END | ||
| # skip re-posting a comment when this issue already carries | ||
| # a Ymir comment for the same postponement resolution. | ||
| # User-triggered runs always get fresh confirmation, so they bypass this guard. |
There was a problem hiding this comment.
haven't we discussed yesterday that the user_triggered will actually not be stored/will be set to False?
There was a problem hiding this comment.
Yes. The sweep has it set to False for now, because it is conceived as a cronjob. But it is conceivable that we would want to see how postponed issues are handled on demand. This way we can bypass the check and see what happens, otherwise we wouldn't have that option.
If I am to err, I would like to err on the side of caution. The Monday deployment is preferable, in my opinion. |
3 participants
Implementing design from research by @majamassarini .
Postponed issues are now split into several categories, depending on the reason for postponement. In each of these cases, a comment, in a well defined format, is posted under the given JIRA item. This comment is later processed by the
comment_parserand used to determine if the postponement label can be removed, and processing resumed.Sweep is executed using one of 4 strategies:
check_build_in_buildroot.ymir.supervisor.gitlab_utils.gitlab_api_getto check state of MRs. If they are merged, the issue is pushed back into triage queue.ymir_postponed_no_patchback into triage queue, subject to limit of 20 per run.check_build_in_buildrootAll with their own cronjobs.
Notes:
gitlab_api_getfunction, so it can be used by sweep jobs.ymir_abandonedlabel has been implemented as part of this PR, although it is going to be used only in phase 2#.