Skip to content

Support Forgejo events handling by Stevejobs - #3154

Merged
centosinfra-prod-github-app[bot] merged 2 commits into
packit:mainfrom
betulependule:forgejo/steve-jobs
Aug 21, 2026
Merged

Support Forgejo events handling by Stevejobs#3154
centosinfra-prod-github-app[bot] merged 2 commits into
packit:mainfrom
betulependule:forgejo/steve-jobs

Conversation

@betulependule

@betulependule betulependule commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

The logic in SteveJobs has been modified to support proper Forgejo events processing. Forgejo events have also been added in the Allowlist.check_and_report() method as unchecked events.

Related to #2862

@centosinfra-prod-github-app

Copy link
Copy Markdown
Contributor

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 19, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 7:22 AM UTC · Completed 7:42 AM UTC

Commit: 16b9f77 · View workflow run →

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 19, 2026

Copy link
Copy Markdown

Review

Findings

Low

  • [fail-open] packit_service/worker/allowlist.py:508 — All six Forgejo event types bypass allowlist authorization (routed to _check_unchecked_event() which returns True unconditionally). This follows the existing Pagure pattern and is acceptable for the current dist-git-only scope, but no structural mechanism distinguishes dist-git from upstream Forgejo events when upstream support is added. The [XXX] comment acknowledges this.
  • [fail-open] packit_service/worker/jobs.py:967check_explicit_matching() assumes all Forgejo events come from dist-git. The [XXX] comment documents the assumption. Upstream Forgejo PR comments could trigger dist-git-specific jobs (koji_build, bodhi_update) if upstream support is added without refining this logic.
  • [scope-gap] packit_service/worker/jobs.py:359 — The reaction guard in process() excludes only pagure.pr.Comment from add_reaction(). Forgejo PR/issue comment events will trigger reaction addition. This is likely correct (Forgejo supports reactions unlike Pagure), but worth confirming the ogr Forgejo implementation supports it.
  • [scope-gap] packit_service/worker/jobs.py:374 — Forgejo PR events added to Fedora CI processing guard require db_project_object. Verified via inheritance chain (AddPullRequestEventToDb) that this is correctly wired.
  • [scope-alignment] packit_service/worker/jobs.py:965 — The [XXX] comment appropriately flags the dist-git assumption for forgejo.pr.Comment in koji-build/bodhi-update re-trigger matching.
  • [ordering convention] packit_service/worker/jobs.py:378 — Fedora CI isinstance check interleaves Forgejo and Pagure events by subtype rather than grouping by forge as done elsewhere in the file.
  • [ordering convention] packit_service/worker/jobs.py:352 — Help comment isinstance check groups by event subtype with forgejo first alphabetically; internally consistent but departs from forge-grouping elsewhere.
  • [missing_documentation] files/scripts/README.md:24 — Allowlist docs only show GitHub namespace examples. Since Forgejo dist-git events bypass the allowlist entirely (unchecked bucket), a note explaining this behavior would help operators understand that Forgejo namespaces do not need allowlist entries.
Previous run

Review

Findings

Medium

  • [authorization bypass] packit_service/worker/allowlist.py:508 — All six Forgejo event types are added to the unchecked-events tuple in check_and_report(), routing them to _check_unchecked_event() which unconditionally returns True. While this parallels the existing Pagure pattern (which is safe because Pagure is only used as dist-git), there is no mechanism to distinguish whether a Forgejo event originates from a dist-git instance or an upstream Forgejo instance. If upstream Forgejo support is added without updating this code, those events would bypass all allowlist authorization checks. The [XXX] comment acknowledges this limitation.
    Remediation: Consider adding a configuration guard (e.g., trusted Forgejo dist-git hostnames in ServiceConfig) and verify event.project_url against it before granting unchecked access.

Low

  • [fail-open] packit_service/worker/allowlist.py:508 — The unchecked-events authorization bypass is fail-open by design. Any new Forgejo event type added to the unchecked tuple inherits the bypass without independent validation.

  • [logic error] packit_service/worker/jobs.py:963check_explicit_matching() is updated to match forgejo.pr.Comment alongside pagure.pr.Comment for koji_build, bodhi_update, and pull_from_upstream jobs. However, no handler has @reacts_to(event=forgejo.pr.Comment) registered in SUPPORTED_EVENTS_FOR_HANDLER, making the non-help-comment processing paths effectively dead code for Forgejo events. The help comment path works because it dispatches handlers directly.

  • [edge-case] packit_service/worker/jobs.py:1098 — In get_handlers_for_comment_and_rerun_event(), the reaction exclusion guard does not exclude forgejo.pr.Comment. If the ogr Forgejo implementation does not support add_reaction(), this would cause a runtime error. Currently unreachable due to the handler registration gap, but would become live once handlers are registered.

  • [scope gap] packit_service/worker/jobs.py:374 — Forgejo events are added to the Fedora CI gating block, but FedoraCIConfig.is_project_enabled() restricts processing to src.fedoraproject.org URLs. If Forgejo dist-git URLs don't match these patterns, the addition is inert.

  • [test-inadequate] packit_service/worker/jobs.py — This PR modifies process(), check_explicit_matching(), and get_handlers_for_comment_and_rerun_event() but adds no tests for Forgejo event handling.

Previous run (2)

Review

Findings

Medium

  • [authorization-bypass] packit_service/worker/allowlist.py:508 — All six Forgejo event types are added to the unchecked events tuple, which maps to _check_unchecked_event — a method that unconditionally returns True. While this mirrors the existing Pagure dist-git trust model, there is no code-level enforcement of the dist-git-only assumption (no validation of project_url against known dist-git domains). The [XXX] comment acknowledges this risk. The risk is partially mitigated by the fedora-messaging bus being a controlled transport channel.
    Remediation: Add validation of project_url against a configurable list of trusted dist-git Forgejo domains, or at minimum file a tracking issue and reference it from the [XXX] comment.

  • [missing-reaction-guard] packit_service/worker/jobs.py:1096 — In get_handlers_for_comment_and_rerun_event(), add_reaction() is guarded by excluding only pagure.pr.Comment and abstract.comment.Commit. forgejo.pr.Comment events will now reach add_reaction(). The same pattern exists in the help comment path (line 362). If ogr's Forgejo implementation does not support add_reaction on comment objects, this will raise an exception at runtime.
    Remediation: Verify that ogr's Forgejo implementation supports add_reaction(). If unsupported, add forgejo.pr.Comment to the exclusion tuples in both locations.

Low

  • [authorization-bypass] packit_service/worker/jobs.py:968check_explicit_matching() now treats forgejo.pr.Comment identically to pagure.pr.Comment, allowing it to match koji_build and bodhi_update job configs for re-triggering from dist-git PRs. The [XXX] comment acknowledges the dist-git-only assumption. Consistent with the allowlist bypass finding above.
    Remediation: File a tracking issue referenced from the [XXX] comment for forge-origin discrimination when upstream Forgejo support is added.

  • [incomplete-wiring] packit_service/worker/jobs.py:378forgejo.pr.Action and forgejo.pr.Comment are added to the Fedora CI processing gate, but no handler has @reacts_to_as_fedora_ci decorators for these event types. process_fedora_ci_jobs() will return [] (falsy), and execution falls through to process_jobs(). The Fedora CI gate is a no-op for Forgejo events — functionally correct but creates an unnecessary code path detour.
    Remediation: Either remove Forgejo events from the Fedora CI gate isinstance check, or add a comment explaining they are included for future handler registration.

Previous run (3)

Review

Findings

Medium

  • [logic-error] packit_service/worker/jobs.py:972check_explicit_matching now matches forgejo.pr.Comment events and returns koji_build, bodhi_update, and pull_from_upstream job configs. However, none of the downstream handlers (DownstreamKojiBuildHandler, RetriggerBodhiUpdateHandler, PullFromUpstreamHandler) register @reacts_to(event=forgejo.pr.Comment). The handler matching in is_handler_matching_the_event() checks isinstance(self.event, tuple(SUPPORTED_EVENTS_FOR_HANDLER[handler])), which will never match forgejo.pr.Comment since it is not registered. The net effect is that dist-git job retriggering via Forgejo PR comments silently does nothing. This appears to be intentional incremental work (issue Procesing events from Forgejo as a Fedora dist-git #2862 lists "Support packit command extraction from comments" as TODO). Consider adding a comment noting that handler registration is pending, or adding the @reacts_to decorators in this or a follow-up PR.

  • [authorization-bypass] packit_service/worker/allowlist.py:506 — Six Forgejo event types are added to the unchecked-events tuple in check_and_report(), meaning all Forgejo events bypass allowlist authorization entirely (_check_unchecked_event() unconditionally returns True). This follows the existing pattern for Pagure dist-git events, which is appropriate for the current scope (Forgejo as Fedora dist-git only). However, unlike Pagure which is constrained to src.fedoraproject.org, Forgejo is a general-purpose forge. If packit-service ever ingests webhooks from non-dist-git Forgejo instances, any user could trigger jobs without allowlist approval. The [XXX] comment at jobs.py:969 acknowledges this assumption. Consider adding a hostname-based guard or configuration-level restriction to make the dist-git constraint explicit at the allowlist level.

Low

  • [stale-reference] packit_service/worker/allowlist.py:45 — The UncheckedEvent type alias (lines 45–56) was not updated to include the six new Forgejo event types, even though they were added to the unchecked-events tuple in check_and_report(). While this has no runtime effect (dispatch uses isinstance against the CALLBACKS dict), the type annotation for _check_unchecked_event() is now inaccurate.

  • [edge-case] packit_service/worker/jobs.py:1105 — In get_handlers_for_comment_and_rerun_event(), the add_reaction() call excludes pagure.pr.Comment (reactions not supported in Pagure) but does not exclude forgejo.pr.Comment. If the ogr Forgejo backend does not support add_reaction(), this could raise a runtime exception. Verify whether the Forgejo backend implements this method.

  • [edge-case] packit_service/worker/handlers/distgit.py:1439DownstreamKojiBuildHandler.get_branches() checks self.data.event_type in (pagure.pr.Comment.event_type(),). If forgejo.pr.Comment reaches this handler in the future (after @reacts_to registration), the event type string will not match, causing incorrect branch resolution. Currently unreachable but worth noting for when handler registration is added.

  • [ordering-consistency] packit_service/worker/allowlist.py:506 — The Forgejo entries in the CALLBACKS unchecked-events tuple are placed after Pagure entries. While the existing tuple was not strictly alphabetical, consider grouping consistently.

  • [ordering-consistency] packit_service/worker/jobs.py:381 — In the Fedora CI event isinstance check, Forgejo and Pagure entries are interleaved by event type (forgejo.pr.Action, pagure.pr.Action, forgejo.pr.Comment, pagure.pr.Comment) rather than grouped by forge. Consider grouping by forge for consistency with other tuples in this file.


Labels: PR adds Forgejo event handling to SteveJobs and allowlist, directly implementing Forgejo dist-git forge support.

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment area/forgejo Forgejo-forge related labels Aug 19, 2026
@centosinfra-prod-github-app

Copy link
Copy Markdown
Contributor

@betulependule

betulependule commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

[The following is OUTDATED, based on fullsend's first review]

To address fullsend's review:

  1. packit_service/worker/jobs.py:972: [medium] logic-error

    • This will be addressed in a follow-up PR. It is out of scope of the issue this PR addresses.
  2. packit_service/worker/allowlist.py:508: [medium] authorization-bypass

    • Added a comment in the code addressing this. This should be addressed once upstream Forgejo support is added.
  3. packit_service/worker/allowlist.py (file-level): Line 45 · [low] stale-reference

    • Fixed.
  4. packit_service/worker/jobs.py (file-level): Line 1105 · [low] edge-case

    • ogr supports the add_reaction() method in relation to Forgejo comments. No fix needed here.
  5. packit_service/worker/allowlist.py:506: [low] ordering-consistency

    • Fixed, though I don't think it was necessary.
  6. packit_service/worker/jobs.py:381: [low] ordering-consistency

    • I would keep the sorting consistent to what it was previously.

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 19, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 12:35 PM UTC · Completed 1:12 PM UTC

Commit: e837e13 · View workflow run →

@betulependule

betulependule commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

To address fullsend's (second) review:

  1. [authorization-bypass] packit_service/worker/allowlist.py:508

    • For now, this should be ok the way it is because we shouldn't be receiving Forgejo events from anywhere other than from dist-git. It would be best to create a sub-issue for this in: Support Forgejo for upstream CI #2855.
  2. [missing-reaction-guard] packit_service/worker/jobs.py:1096

    • As mentioned already, add_reaction() is implemented for Forgejo. No fix needed.
  3. [authorization-bypass] packit_service/worker/jobs.py:968

  4. [incomplete-wiring] packit_service/worker/jobs.py:378

@fullsend-ai-review

fullsend-ai-review Bot commented Aug 19, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 1:58 PM UTC · Ended 2:16 PM UTC

Commit: f715816 · View workflow run →

@centosinfra-prod-github-app

Copy link
Copy Markdown
Contributor

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 1:58 PM UTC · Completed 2:16 PM UTC

Commit: f715816 · View workflow run →

@betulependule

Copy link
Copy Markdown
Contributor Author

For reviewers taking a look at fullsends's latest review:

  1. packit_service/worker/allowlist.py:508: [medium] authorization bypass

    • An issue should be created to track this (needed once Forgejo upstream support is implemented). I don't think it's needed for dist-git support currently.
  2. packit_service/worker/allowlist.py:508: [low] fail-open

    • Same as above.
  3. packit_service/worker/jobs.py (file-level): Line 963 · [low] logic-error

    • Out of scope for the issue covered by this PR.
  4. packit_service/worker/jobs.py (file-level): Line 1098 · [low] edge-case

    • ogr supports add_reaction() for Forgejo.
  5. packit_service/worker/jobs.py (file-level): Line 374 · [low] scope-gap

    • Dist-git URLs aren't going to change. No fix needed.

The logic in `SteveJobs` has been modified to support Forgejo
events processing.
@fullsend-ai-review

fullsend-ai-review Bot commented Aug 21, 2026

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 6:54 AM UTC · Ended 7:34 AM UTC

Commit: 1b073cd · View workflow run →

@centosinfra-prod-github-app

Copy link
Copy Markdown
Contributor

@betulependule

Copy link
Copy Markdown
Contributor Author

recheck

@fullsend-ai-review fullsend-ai-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: The following inline comments could not be posted on the diff (GitHub returned 422) and are included here instead:

  • packit_service/worker/allowlist.py:508: [low] fail-open

All six Forgejo event types bypass allowlist authorization. This follows the existing Pagure pattern and is acceptable for dist-git, but no structural mechanism prevents upstream Forgejo events from bypassing authorization when upstream support is added.

Suggested fix: Before adding upstream Forgejo support, implement a guard (e.g., checking project_url against configured dist-git domains) to route upstream events through _check_pr_event/_check_issue_comment_event.

  • packit_service/worker/jobs.py:967: [low] fail-open

check_explicit_matching() assumes all Forgejo events come from dist-git. Upstream Forgejo PR comments could trigger dist-git-specific job types (koji_build, bodhi_update) if upstream support is added without refining this logic.

Suggested fix: Add a dist-git vs upstream guard when upstream Forgejo support is implemented.

  • packit_service/worker/jobs.py:359: [low] scope-gap

Reaction guard in process() excludes only pagure.pr.Comment from add_reaction(). Forgejo events will trigger reaction addition. Likely correct (Forgejo supports reactions) but ogr Forgejo implementation should be confirmed.

  • packit_service/worker/jobs.py (file-level): Line 374 · [low] scope-gap

Forgejo PR events added to Fedora CI processing guard require db_project_object. Verified correct via AddPullRequestEventToDb inheritance chain.

  • packit_service/worker/jobs.py:965: [low] scope-alignment

XXX comment appropriately flags the dist-git assumption for forgejo.pr.Comment in koji-build/bodhi-update re-trigger matching. Well-aligned with issue #2862 scope.

  • packit_service/worker/jobs.py:378: [low] ordering convention

Fedora CI isinstance check interleaves Forgejo and Pagure events by subtype rather than grouping by forge as done elsewhere.

Suggested fix: Group by forge: (forgejo.pr.Action, forgejo.pr.Comment, pagure.pr.Action, pagure.pr.Comment, ...)

  • packit_service/worker/jobs.py:352: [low] ordering convention

Help comment isinstance check groups by event subtype with forgejo first alphabetically. Internally consistent but departs from forge-grouping used elsewhere in the file.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed requires-manual-review Review requires human judgment labels Aug 21, 2026
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 6:54 AM UTC · Completed 7:33 AM UTC

Commit: 1b073cd · View workflow run →

@centosinfra-prod-github-app

Copy link
Copy Markdown
Contributor

@betulependule betulependule moved this from New to In review in Packit pull requests Aug 21, 2026
@betulependule betulependule added the mergeit Merge via Zuul label Aug 21, 2026
@centosinfra-prod-github-app

Copy link
Copy Markdown
Contributor

@centosinfra-prod-github-app
centosinfra-prod-github-app Bot merged commit ca574c7 into packit:main Aug 21, 2026
56 checks passed
@github-project-automation github-project-automation Bot moved this from In review to Done in Packit pull requests Aug 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/forgejo Forgejo-forge related mergeit Merge via Zuul ready-for-merge All reviewers approved — ready to merge

Projects

Development

Successfully merging this pull request may close these issues.

3 participants