Build comment from fields of Koji build target

[jpodivin]

RELEASE NOTES BEGIN

Packit will supply Log Detective with additional information about executed Koji build. Such as duration, version and architecture.

RELEASE NOTES END

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:26 PM UTC · Completed 3:39 PM UTC

Commit: efcb19d · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.97

[centosinfra-prod-github-app]

Build failed.
https://gateway-cloud-softwarefactory.apps.ocp.cloud.ci.centos.org/zuul/t/packit-service/buildset/ae6f346edd0447f8b48cda5d1cec48c8

✔️ pre-commit SUCCESS in 1m 56s
❌ packit-service-tests FAILURE in 3m 24s

[nforro]

LGTM. Not sure what triggered the test failures though.

[fullsend-ai-review]

Review

Findings

Medium

• [mocking library consistency] tests/unit/test_logdetective_koji_helper.py:7 — The PR introduces from unittest.mock import MagicMock to create mock objects that pass isinstance() checks. However, every other test file in the repository (64 files) uses flexmock exclusively and none import unittest.mock. Mixing two mocking libraries creates maintenance friction. flexmock can achieve isinstance() compatibility via flexmock(PullRequestModel()).
  Remediation: Replace MagicMock(spec=PullRequestModel) with a flexmock-based approach such as flexmock(PullRequestModel()). Apply the same change for GitBranchModel and ProjectReleaseModel. Remove the from unittest.mock import MagicMock import.

Low

• [test adequacy] tests/integration/test_logdetective_koji.py:76 — The integration test sets nvr/scratch/sidetag/build_submission_stdout on the koji_build_pr_downstream fixture, but the fixture's db_project_object is a bare flexmock, not a PullRequestModel instance. Since _build_commentary uses isinstance(db_project_object, PullRequestModel), the PR-build branch will never be exercised in the integration test.

• [edge case handling] packit_service/worker/helpers/logdetective.py:107 — build.build_submission_stdout is appended to the commentary string without truncation. If Koji build submission produced unusually large stdout, the Log Detective API request payload could become unexpectedly large. In practice Koji submission stdout is typically short, so the risk is low.

• [naming-coherence] packit_service/worker/helpers/logdetective.py — The method name _build_commentary uses 'build' as a verb (meaning 'construct'), but in this codebase 'build' overwhelmingly refers to a Koji/Copr build noun. A name like _compose_commentary or _generate_commentary would avoid the collision with the domain term.

Previous run

Review

Findings

Low

• [edge-case] packit_service/worker/helpers/logdetective.py:82 — _build_commentary accesses self.koji_event.build_model and immediately dereferences build.nvr without a None guard. If build_model were None, this would raise AttributeError. However, this mirrors the existing pattern in trigger_log_detective_analysis_for_arch (which also dereferences build_model without checking) and is not practically reachable — the handler's _run() method returns early if the build model is absent before this code path executes.

• [test-inadequate] tests/integration/test_logdetective_koji.py:76 — The integration test sets new attributes (build_submission_stdout, nvr, scratch, sidetag) on koji_build_pr_downstream but the requests.post mock accepts any arguments, so the dynamic commentary payload is never validated end-to-end. Additionally, db_project_object resolves to a flexmock that will fail the isinstance(db_project_object, PullRequestModel) check (flexmock objects are not instances of the real model class), causing the PR-specific commentary line to be silently skipped. Consider adding a payload assertion or using flexmock(PullRequestModel) to ensure isinstance works correctly.

Previous run (2)

Review

Findings

Low

• [test-inadequate] tests/unit/test_logdetective_koji_helper.py:55 — The test fixture only exercises one path through _build_commentary: db_project_object=None, scratch=False, sidetag=None, build_submission_stdout=None, and valid numeric timestamps. None of the new conditional branches are tested: PullRequestModel/GitBranchModel/ProjectReleaseModel project types, scratch builds, sidetag builds, non-None build_submission_stdout, or the error/edge-case paths in _format_duration (None timestamps, negative duration, non-numeric string timestamps). The method has six independent conditional branches; a single happy-path case leaves most of them unverified.

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 7:54 AM UTC · Ended 8:11 AM UTC

Commit: 2e9d82f · View workflow run →

[centosinfra-prod-github-app]

Build failed.
https://gateway-cloud-softwarefactory.apps.ocp.cloud.ci.centos.org/zuul/t/packit-service/buildset/fe04ecc34dd446a6b9282a8af0bbd79d

✔️ pre-commit SUCCESS in 1m 57s
❌ packit-service-tests FAILURE in 3m 04s

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:54 AM UTC · Completed 8:11 AM UTC

Commit: 2e9d82f · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.46

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 10:08 AM UTC · Ended 10:24 AM UTC

Commit: 83d4b46 · View workflow run →

[centosinfra-prod-github-app]

Build failed.
https://gateway-cloud-softwarefactory.apps.ocp.cloud.ci.centos.org/zuul/t/packit-service/buildset/ad7881b8e4db4ab3be8b5afcb0f5459e

✔️ pre-commit SUCCESS in 2m 05s
❌ packit-service-tests FAILURE in 3m 19s

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:08 AM UTC · Completed 10:24 AM UTC

Commit: 83d4b46 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.50