Correct targets for pushing db artifact

[lipoja]

Source artifact should not be overwritten, we have to push target database since target is the index image that is changed when overwrite_target_index_token is set.

[qodo-for-releng]

PR Summary by Qodo

Fix index.db artifact push to target index during merge overwrite
🐞 Bug fix 🕐 10-20 Minutes

Description

• Push updated index.db to the target index image when overwrite is enabled.
• Prevent overwriting the source index.db artifact in the registry.

Diagram

graph TD
  S["source_from_index"] --> M["Merge task"] --> P["push_index_db_artifact"] --> R[("Registry")]
  T["target_index"] --> M
  M --> DB[["target index.db"]] --> P
  subgraph Legend
    direction LR
    _svc["Task"] ~~~ _art[["Artifact"]] ~~~ _reg[("Registry")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Add regression test for overwrite_target_index_token artifact push

• ➕ Prevents future regressions around which index/db gets pushed
• ➕ Codifies expected behavior for overwrite flows
• ➖ Requires mocking artifact fetch/push and registry-token context
• ➖ Adds some CI/runtime and test maintenance overhead

2. Encapsulate source/target artifact selection into a helper

• ➕ Reduces risk of parameter mixups (index vs db_path) across tasks
• ➕ Improves readability by making overwrite semantics explicit
• ➖ More refactor than necessary for a localized bug fix
• ➖ May expand PR scope by touching additional code paths

Recommendation: The PR’s minimal fix is appropriate and low-risk: overwrite flows should push the target index’s updated index.db, not the source. If this area is prone to regressions, a small follow-up test covering overwrite_target_index_token behavior would provide long-term safety.

Files changed (1) +2 / -2

Bug fix (1) +2 / -2

build_containerized_merge.pyPush index.db artifact using target index/path for overwrite merges +2/-2

Push index.db artifact using target index/path for overwrite merges

• Switches the push_index_db_artifact call to use target_index and target_index_db_path. This ensures the correct (overwritten) index image’s database artifact is pushed and avoids overwriting the source artifact.

iib/workers/tasks/build_containerized_merge.py

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:36 PM UTC · Completed 12:45 PM UTC
Commit: 8207ad4 · View workflow run →

[qodo-for-releng]

Code Review by Qodo

🐞 Bugs (4) 📘 Rule violations (0) 📜 Skill insights (0)

1. API allows missing target_index 🐞 Bug ≡ Correctness ⭐ New

Description

The merge-index-image request model treats target_index as optional, but
handle_containerized_merge_request now raises IIBError when target_index is missing, so a request
can be accepted/persisted and then deterministically fail when executed.

Code

iib/workers/tasks/build_containerized_merge.py[R114-115]

+    if not target_index:
+        raise IIBError('target_index is required.')

Relevance

⭐⭐⭐ High

Team previously tightened API validation to prevent downstream worker failures (PR #1137).

PR-#1137

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The worker now hard-requires target_index, but the API model only validates it when present, so
missing target_index can pass API validation and later fail in the worker.

iib/workers/tasks/build_containerized_merge.py[111-119]
iib/web/models.py[1615-1630]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`handle_containerized_merge_request()` now requires `target_index`, but the `/builds/merge-index-image` request model (`RequestMergeIndexImage.from_json`) still allows `target_index` to be omitted. This causes the API to accept and store requests that will always fail later in the worker.

### Issue Context
The worker now raises immediately when `target_index` is falsy, so the correct place to enforce this requirement is the API validation layer (return 4xx), not after request creation.

### Fix Focus Areas
- iib/web/models.py[1615-1630]
- iib/workers/tasks/build_containerized_merge.py[114-116]

### Suggested fix
1. In `RequestMergeIndexImage.from_json`, make `target_index` required (raise `ValidationError` if missing/empty).
2. Keep the worker-side guard if desired as defense-in-depth, but ensure the API cannot enqueue invalid requests.
3. Update any API schema/docs/tests that assume `target_index` is optional.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Overwrites wrong index image 🐞 Bug ≡ Correctness ⭐ New

Description

When overwrite_target_index=True, the merge task still calls
_update_index_image_pull_spec(from_index=source_from_index,
overwrite_from_index=overwrite_target_index), so the overwrite operation targets source_from_index
rather than target_index. This PR simultaneously changes index.db artifact pushing/rollback to use
target_index, so the image overwrite target can diverge from the artifact overwrite target.

Code

iib/workers/tasks/build_containerized_merge.py[R350-356]

            original_index_db_digest = push_index_db_artifact(
                request_id=request_id,
-                from_index=source_from_index,
-                index_db_path=source_index_db_path,
+                from_index=str(target_index),
+                index_db_path=str(target_index_db_path),
                operators=operators_in_db,
                overwrite_from_index=overwrite_target_index,
                request_type='merge',

Relevance

⭐⭐⭐ High

Overwrite-path correctness is actively maintained; fixes for overwrite/rollback consistency were
merged (PRs #995, #1075).

PR-#995
PR-#1075

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The merge task documents overwriting target_index, but passes source_from_index into the overwrite
helper; the helper overwrites the image designated by its from_index parameter, proving the wrong
image would be overwritten when overwrite_target_index=True.

iib/workers/tasks/build_containerized_merge.py[81-96]
iib/workers/tasks/build_containerized_merge.py[329-357]
iib/workers/tasks/build.py[285-300]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The merge request flag is named `overwrite_target_index` and is documented to overwrite `target_index`, but the code path that actually overwrites the index image uses `_update_index_image_pull_spec(from_index=source_from_index, overwrite_from_index=overwrite_target_index, ...)`. After this PR, index.db artifact push/rollback is switched to `target_index`, making it possible to overwrite different targets for the image vs the index.db artifact.

### Issue Context
`_update_index_image_pull_spec()` overwrites the image specified by its `from_index` argument when `overwrite_from_index=True`. In this merge task, `overwrite_target_index_token` is also the credential intended for `target_index`, not necessarily `source_from_index`.

### Fix Focus Areas
- iib/workers/tasks/build_containerized_merge.py[329-357]
- iib/workers/tasks/build.py[286-299]

### Suggested fix
1. Change the `_update_index_image_pull_spec(...)` call in the merge task to pass `from_index=target_index` (and `resolved_prebuild_from_index=target_index_resolved`) when `overwrite_target_index` is True.
2. Ensure any overwrite-token usage aligns with the same index (`target_index`).
3. Add/adjust unit tests to assert that overwrite operations target `target_index` (both image overwrite and index.db artifact overwrite).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Rollback restores wrong artifact 🐞 Bug ☼ Reliability

Description

After this change, push_index_db_artifact captures the original digest for target_index's v4.x
artifact, but the failure cleanup still calls cleanup_on_failure(from_index=source_from_index), so
restoration targets a different artifact tag than the one whose digest was captured.

Code

iib/workers/tasks/build_containerized_merge.py[R349-350]

+                from_index=target_index,
+                index_db_path=target_index_db_path,

Relevance

⭐⭐⭐ High

Team previously tightened rollback to match overwritten artifact keyed by from_index in overwrite
flow (PR #995).

PR-#995

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The pushed artifact digest capture and the restore logic are both keyed off from_index; the PR
changes the push to use target_index but leaves cleanup using source_from_index, so rollback
would not restore the same artifact that was overwritten.

iib/workers/tasks/build_containerized_merge.py[343-377]
iib/workers/tasks/containerized_utils.py[379-395]
iib/workers/tasks/containerized_utils.py[468-488]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`push_index_db_artifact(from_index=target_index, overwrite_from_index=True)` captures the original digest for the v4.x artifact derived from `target_index`. On exception, `cleanup_on_failure` is still invoked with `from_index=source_from_index`, and `cleanup_on_failure` uses `from_index` to compute the artifact pullspec to restore. This mismatch can prevent correct rollback of the overwritten v4.x artifact.

### Issue Context
- `push_index_db_artifact` uses `from_index` to compute `get_indexdb_artifact_pullspec(from_index)` for digest capture.
- `cleanup_on_failure` uses `get_indexdb_artifact_pullspec(from_index)` for restoration.
- The merge task currently passes different values to those two paths.

### Fix Focus Areas
- iib/workers/tasks/build_containerized_merge.py[343-377]
- iib/workers/tasks/containerized_utils.py[379-395]
- iib/workers/tasks/containerized_utils.py[468-488]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (1)

4. Stale index.db uploaded 🐞 Bug ≡ Correctness

Description

handle_containerized_merge_request now pushes target_index_db_path even though the merge/deprecation
steps mutate source_index_db_path, so the published index.db artifact can be out of sync with the
merged index image content.

Code

iib/workers/tasks/build_containerized_merge.py[R349-350]

+                from_index=target_index,
+                index_db_path=target_index_db_path,

Relevance

⭐⭐ Medium

No direct historical evidence on index.db path; team often fixes similar state-sync bugs (PRs 1075,
1070).

PR-#1075
PR-#1070

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code mutates and reads the merged state from source_index_db_path but pushes
target_index_db_path; push_index_db_artifact pushes the file at index_db_path, so the wrong DB
file would be uploaded.

iib/workers/tasks/build_containerized_merge.py[160-165]
iib/workers/tasks/build_containerized_merge.py[211-244]
iib/workers/tasks/build_containerized_merge.py[343-354]
iib/workers/tasks/containerized_utils.py[344-414]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`push_index_db_artifact()` is called with `index_db_path=target_index_db_path`, but the merge logic modifies `source_index_db_path` (adds missing bundles, deprecates bundles, and extracts `operators_in_db` from it). This causes the artifact upload to potentially publish an unmodified/stale DB that doesn’t reflect the merged result.

### Issue Context
- `source_index_db_path` is the intermediary DB being updated.
- `target_index_db_path` is only fetched for comparison; it is not modified.
- `push_index_db_artifact()` uploads whatever file path is provided.

### Fix Focus Areas
- iib/workers/tasks/build_containerized_merge.py[343-354]
- iib/workers/tasks/build_containerized_merge.py[160-165]
- iib/workers/tasks/build_containerized_merge.py[211-244]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit 02e03f1

Results up to commit b90ab3f

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Rollback restores wrong artifact 🐞 Bug ☼ Reliability

Description

After this change, push_index_db_artifact captures the original digest for target_index's v4.x
artifact, but the failure cleanup still calls cleanup_on_failure(from_index=source_from_index), so
restoration targets a different artifact tag than the one whose digest was captured.

Code

iib/workers/tasks/build_containerized_merge.py[R349-350]

+                from_index=target_index,
+                index_db_path=target_index_db_path,

Relevance

⭐⭐⭐ High

Team previously tightened rollback to match overwritten artifact keyed by from_index in overwrite
flow (PR #995).

PR-#995

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The pushed artifact digest capture and the restore logic are both keyed off from_index; the PR
changes the push to use target_index but leaves cleanup using source_from_index, so rollback
would not restore the same artifact that was overwritten.

iib/workers/tasks/build_containerized_merge.py[343-377]
iib/workers/tasks/containerized_utils.py[379-395]
iib/workers/tasks/containerized_utils.py[468-488]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`push_index_db_artifact(from_index=target_index, overwrite_from_index=True)` captures the original digest for the v4.x artifact derived from `target_index`. On exception, `cleanup_on_failure` is still invoked with `from_index=source_from_index`, and `cleanup_on_failure` uses `from_index` to compute the artifact pullspec to restore. This mismatch can prevent correct rollback of the overwritten v4.x artifact.

### Issue Context
- `push_index_db_artifact` uses `from_index` to compute `get_indexdb_artifact_pullspec(from_index)` for digest capture.
- `cleanup_on_failure` uses `get_indexdb_artifact_pullspec(from_index)` for restoration.
- The merge task currently passes different values to those two paths.

### Fix Focus Areas
- iib/workers/tasks/build_containerized_merge.py[343-377]
- iib/workers/tasks/containerized_utils.py[379-395]
- iib/workers/tasks/containerized_utils.py[468-488]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Stale index.db uploaded 🐞 Bug ≡ Correctness

Description

handle_containerized_merge_request now pushes target_index_db_path even though the merge/deprecation
steps mutate source_index_db_path, so the published index.db artifact can be out of sync with the
merged index image content.

Code

iib/workers/tasks/build_containerized_merge.py[R349-350]

+                from_index=target_index,
+                index_db_path=target_index_db_path,

Relevance

⭐⭐ Medium

No direct historical evidence on index.db path; team often fixes similar state-sync bugs (PRs 1075,
1070).

PR-#1075
PR-#1070

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code mutates and reads the merged state from source_index_db_path but pushes
target_index_db_path; push_index_db_artifact pushes the file at index_db_path, so the wrong DB
file would be uploaded.

iib/workers/tasks/build_containerized_merge.py[160-165]
iib/workers/tasks/build_containerized_merge.py[211-244]
iib/workers/tasks/build_containerized_merge.py[343-354]
iib/workers/tasks/containerized_utils.py[344-414]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`push_index_db_artifact()` is called with `index_db_path=target_index_db_path`, but the merge logic modifies `source_index_db_path` (adds missing bundles, deprecates bundles, and extracts `operators_in_db` from it). This causes the artifact upload to potentially publish an unmodified/stale DB that doesn’t reflect the merged result.

### Issue Context
- `source_index_db_path` is the intermediary DB being updated.
- `target_index_db_path` is only fetched for comparison; it is not modified.
- `push_index_db_artifact()` uploads whatever file path is provided.

### Fix Focus Areas
- iib/workers/tasks/build_containerized_merge.py[343-354]
- iib/workers/tasks/build_containerized_merge.py[160-165]
- iib/workers/tasks/build_containerized_merge.py[211-244]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[fullsend-ai-review]

Looks good to me.

The bug fix correctly introduces effective_index_image, effective_index_image_resolved, and effective_ocp_version variables to route operations to the target index (when provided) instead of always using the source index. The input validation guard preventing overwrite_target_index/overwrite_target_index_token without target_index is appropriate defense-in-depth. Test coverage is thorough — both the target-present and target-absent paths are exercised, including the error/cleanup path.

Low

• [edge-case] iib/workers/tasks/build_containerized_merge.py:113 — The validation guard also rejects overwrite_target_index_token without target_index even when overwrite_target_index=False. This case is already rejected by the web layer (iib/web/models.py), so it is defense-in-depth rather than a bug.
• [missing-authorization] iib/workers/tasks/build_containerized_merge.py — No linked issue for this bug fix. The PR description and code change clearly describe the problem and fix, but linking to a tracking issue would improve traceability.

Previous run

Review

Findings

Low

• [test-inadequate] tests/test_workers/test_tasks/test_build_containerized_merge.py:1978 — The test_handle_containerized_merge_request_without_target_index test sets target_ocp_version and source_ocp_version to the same value 'v4.14', so assertions like gmbfts_call_args[1]['ocp_version'] == prebuild_info['source_ocp_version'] would pass even if the code incorrectly used target_ocp_version. Using distinct values (e.g., 'v4.14' vs 'v4.15') would make the assertions truly discriminating.
  Remediation: Set target_ocp_version and source_ocp_version to different values in the prebuild_info mock.

• [missing-authorization] No linked issue. The change is clearly a well-scoped bug fix with a descriptive title and body, but linking to an issue would improve traceability for this non-trivial change.

Previous run (2)

Review

Findings

Medium

• [scope-coherence] iib/workers/tasks/build_containerized_merge.py:114 — The new validation guard (raising IIBError when overwrite parameters are set without target_index) and the effective_index_image fallback logic both handle target_index being None, but for different scenarios: the validation blocks the semantically invalid case of overwriting without a target, while the fallback handles the legitimate case of no target and no overwrite. These are complementary, not contradictory, but a brief code comment explaining the relationship between the two would improve maintainability.

Low

• [missing-authorization] iib/workers/tasks/build_containerized_merge.py — This PR makes non-trivial changes but has no linked issue. The PR body clearly explains the bug being fixed, and the ready-for-merge label indicates maintainer review has occurred. Consider linking to an issue for traceability.

• [api-consistency] iib/workers/tasks/build_containerized_merge.py:114 — The existing merge endpoint in build_merge_index_image.py validates overwrite parameters at the API layer (models.py) but does not validate that target_index must be set when overwrite is used. This PR adds worker-level validation, which is a valid defense-in-depth approach. Consider adding equivalent validation to the API layer for consistency.

• [naming-convention] iib/workers/tasks/build_containerized_merge.py:138 — The effective_ prefix on effective_index_image, effective_index_image_resolved, and effective_ocp_version is novel to this codebase. The naming is clear and communicates intent well, but differs from existing patterns which use inline conditionals or simpler variable names.

• [error-handling-idiom] iib/workers/tasks/build_containerized_merge.py:115 — The error message uses implicit string concatenation across lines. This is standard Python and functional, though the codebase has varying styles for error messages.

---

Labels: PR fixes incorrect index image selection for db artifact pushes, which is a bug fix.

Previous run (3)

Looks good to me

The core logic change is correct: when target_index is provided, use it (and its resolved/ocp_version equivalents) instead of source_from_index for OPM version detection, git repository preparation, index image pull spec updates, index.db pushes, and failure cleanup. When target_index is absent, fall back to source_from_index. The new input validation guard and comprehensive tests strengthen the change.

A few minor items for follow-up:

Low

• [error-handling] iib/workers/tasks/build_containerized_merge.py:113 — The error message says "when overwrite_target_index is provided" but the guard also fires when only overwrite_target_index_token is set. Consider: "target_index must be set when overwrite_target_index or overwrite_target_index_token is provided"
• [error-message-style] iib/workers/tasks/build_containerized_merge.py:115 — The "Parameter target_index..." capitalization diverges from the codebase pattern of using lowercase quoted parameter names in error messages.
• [test-inadequate] tests/test_workers/test_tasks/test_build_containerized_merge.py:1978 — In test_handle_containerized_merge_request_without_target_index, both source_ocp_version and target_ocp_version are 'v4.14'. Using distinct values would make the test meaningfully verify which branch of the ternary is taken.
• [missing-doc] iib/web/static/api_v1.yaml:1336 — The new validation requiring target_index when overwrite_target_index or overwrite_target_index_token is set could be explicitly documented in the API spec (the existing descriptions already imply this dependency).

Previous run (4)

Review

Findings

High

• [logic-error] iib/workers/tasks/build_containerized_merge.py:137 — Opm.set_opm_version is called with effective_index_image (an unresolved tag-based reference) instead of effective_index_image_resolved. Every other caller of set_opm_version in the codebase (build.py, build_add_deprecations.py, build_create_empty_index.py, build_fbc_operations.py, build_merge_index_image.py) passes a resolved image reference. set_opm_version calls get_image_label on the passed image — using an unresolved tag risks reading labels from a different image if the tag was updated between resolution and this call.
  Remediation: Change to Opm.set_opm_version(effective_index_image_resolved) to be consistent with all other callers and ensure the label is read from the exact resolved image.

Medium

• [test-inadequate] tests/test_workers/test_tasks/test_build_containerized_merge.py:1964 — The test for the without-target-index case adds source_ocp_version to the prebuild_info mock but does not verify the new behavioral changes: (1) no assertion that Opm.set_opm_version is called with the correct image, (2) no assertion that effective_ocp_version resolves to source_ocp_version, (3) no verification that downstream calls (prepare_git_repository_for_build, write_build_metadata, push_index_db_artifact, _update_index_image_pull_spec) receive the correct effective values. Also missing: a test for the new IIBError validation.

Low

• [edge-case] iib/workers/tasks/build_containerized_merge.py:113 — The validation condition fires when only overwrite_target_index_token is provided without overwrite_target_index=True and without target_index. The error message says "when overwrite_target_index is provided" which is misleading in the token-only case. Providing the token without the flag is largely harmless (unused).

• [logic-error] iib/workers/tasks/build_containerized_merge.py:134 — When target_index is None, effective_index_image_resolved falls back to source_from_index_resolved, changing what gets passed as resolved_prebuild_from_index to _update_index_image_pull_spec. This is intentional new behavior for the no-target-index case (since target_index_resolved would be None), but should be confirmed against the _update_index_image_pull_spec implementation.

• [error-handling-idiom] iib/workers/tasks/build_containerized_merge.py:114 — Error message uses "Parameter X must be set when" pattern, but the codebase uses "The X value is required when" patterns (e.g., in models.py).

• [code-organization] iib/workers/tasks/build_containerized_merge.py:114 — Validation logic is placed after reset_docker_config() and set_request_state() calls. Moving validation before state changes would fail fast on invalid inputs.

Previous run (5)

Review

Reason: stale-head

The review agent reviewed commit 622527292e4ed6fc7130f8275cf34ae37fdeaeec but the PR HEAD is now 99fdb0533bd3ed09d9666487159ca238134f403e. This review was discarded to avoid approving unreviewed code.

Previous run (6)

Review

Findings

Medium

• [logic-error] iib/workers/tasks/build_containerized_merge.py:148 — Opm.set_opm_version is still called with target_index_resolved, but when target_index is None, target_index_resolved will also be None, causing OPM to fall back to the default version rather than using the version appropriate for the source index image. The PR introduces main_index_image_resolved to handle this case at all other call sites but missed this one.
  Remediation: Change Opm.set_opm_version(target_index_resolved) to Opm.set_opm_version(main_index_image_resolved).

Low

• [error-message-accuracy] iib/workers/tasks/build_containerized_merge.py:114 — The error message says "when overwrite_target_index is provided" but the condition also triggers when only overwrite_target_index_token is set. Additionally, the message format deviates from the codebase convention (e.g., models.py uses 'The "X" parameter is required when "Y" is used').
  Remediation: Update to 'The "target_index" parameter is required when "overwrite_target_index" or "overwrite_target_index_token" is used'.

• [test-adequacy] tests/test_workers/test_tasks/test_build_containerized_merge.py:1964 — The test only covers the target_index=None path. There is no corresponding test verifying that when target_index IS set, the code correctly selects target_index, target_index_resolved, and target_ocp_version for the new main_index_image variables.
  Remediation: Add or update a test case where target_index is provided and assert that downstream calls receive the target-index-based values.

Info

• [variable-naming] iib/workers/tasks/build_containerized_merge.py:137 — The main_ variable prefix is novel in this codebase. Consider names like selected_index_image or active_index_image for consistency with existing naming patterns.

• [parameter-validation-placement] iib/workers/tasks/build_containerized_merge.py:114 — The validation check is placed after set_request_state rather than before any state changes. This is consistent with the existing handle_merge_request pattern, so no action needed.

Previous run (7)

Review

Findings

High

• [api-contract] iib/workers/tasks/build_containerized_merge.py:335 — The _update_index_image_pull_spec call has a mismatch between from_index=source_from_index (unchanged) and resolved_prebuild_from_index=target_index_resolved (changed). The _overwrite_from_index function calls _verify_index_image(resolved_prebuild_from_index, from_index, ...) which re-resolves from_index and compares it to resolved_prebuild_from_index. Since source_from_index and target_index are different images, this verification will always fail with "The supplied from_index image changed during the IIB request", making the overwrite path broken when overwrite_target_index is True. The existing handle_merge_request in build_merge_index_image.py correctly pairs from_index=target_index with resolved_prebuild_from_index=target_index_resolved.
  Remediation: Change from_index=source_from_index to from_index=target_index (or from_index=main_index_image for consistency with other changes in this PR) in the _update_index_image_pull_spec call.

Medium

• [test-inadequate] tests/test_workers/test_tasks/test_build_containerized_merge.py:1964 — The test test_handle_containerized_merge_request_without_target_index adds source_ocp_version to prebuild_info but does not verify the behavioral changes introduced by this PR. The test does not assert that _update_index_image_pull_spec is called with resolved_prebuild_from_index=target_index_resolved, or that push_index_db_artifact and _rollback_opm_catalog receive main_index_image instead of source_from_index. The test also does not cover the case where target_index is provided, which is the primary scenario where the from_index/resolved_prebuild_from_index mismatch would cause a runtime failure.
  Remediation: Add assertions verifying the arguments passed to _update_index_image_pull_spec, push_index_db_artifact, and _rollback_opm_catalog. Add a test case with target_index provided to verify the overwrite path works correctly.

Low

• [naming-clarity] iib/workers/tasks/build_containerized_merge.py:132 — The variable names main_ocp_version and main_index_image introduce ambiguity — "main" could mean "primary", "master", or "currently selected". Consider more descriptive names like effective_index_image and effective_ocp_version.

Previous run (8)

Review

Reason: stale-head

The review agent reviewed commit bffb3c780e0c4af1747d41aad25212b3e142451b but the PR HEAD is now d1849d16cd7d993059499f1fa03d12f543bae1e2. This review was discarded to avoid approving unreviewed code.

Previous run (9)

Looks good to me

Low

• [unnecessary-type-coercion] iib/workers/tasks/build_containerized_merge.py:349 — str(target_index) is redundant at runtime since the if not target_index guard at line 113 ensures it is a non-empty string. However, it serves as a type-narrowing hint for static type checkers (mypy) narrowing Optional[str] to str. The inconsistency with line 373 (where target_index is passed bare to the same from_index parameter in cleanup_on_failure) is cosmetic but worth noting.

Info

• [consistency] iib/workers/tasks/build_containerized_merge.py:373 — from_index=str(target_index) at line 349 vs from_index=target_index at line 373 — inconsistent str() wrapping for the same parameter type across two call sites.
• [scope-creep] iib/workers/tasks/build_containerized_merge.py:114 — The PR combines a new validation guard (if not target_index) with the variable reference fix. This is reasonable since target_index is Optional[str] and the fix relies on it being non-None.
• [error-message-format] iib/workers/tasks/build_containerized_merge.py:115 — Error message 'target_index is required.' ends with a period; the codebase has mixed conventions on this.
• [naming-consistency] iib/workers/tasks/build_containerized_merge.py — File naming differs from build_merge_index_image.py (lacks _image suffix). Pre-existing, not introduced by this PR.

Previous run (10)

Review

Findings

Critical

• [logic-error] iib/workers/tasks/build_containerized_merge.py:349 — The PR changes index_db_path from source_index_db_path to target_index_db_path, but target_index_db_path is the unmodified database fetched from the target index. All mutations (bundle additions via _opm_registry_add at line 217, deprecations via deprecate_bundles_db at line 238) are applied to source_index_db_path. The FBC is also migrated from source_index_db_path (line 252). By pushing target_index_db_path, the artifact pushed to the registry will contain the original, unmodified target database — discarding all the merge work. The index.db artifact will be out of sync with the actual FBC catalog image built by the pipeline.
  Remediation: The from_index parameter change to target_index may be correct (to push the artifact to the tag location corresponding to the target index), but the index_db_path must remain source_index_db_path since that is the file containing the merged result. Change line 349 to: index_db_path=source_index_db_path,

High

• [error-handling] iib/workers/tasks/build_containerized_merge.py:361 — After the PR, push_index_db_artifact pushes to a tag derived from target_index, and original_index_db_digest captures the pre-existing digest at that target location. However, cleanup_on_failure (line 361) still passes from_index=source_from_index. Inside cleanup_on_failure, when original_index_db_digest is set, it calls get_indexdb_artifact_pullspec(from_index) to derive the v4.x artifact reference for rollback. Since from_index is source_from_index but the artifact was pushed to a tag derived from target_index, the rollback will attempt to restore the wrong registry tag.
  Remediation: Change cleanup_on_failure call to pass from_index=target_index so that rollback correctly targets the same registry location that push_index_db_artifact modified. Note that cleanup_on_failure also uses from_index for revert_last_commit (where source_from_index is correct), so consider adding a separate parameter for the index.db rollback pullspec.

Low

• [nil-deref] iib/workers/tasks/build_containerized_merge.py:348 — target_index is Optional[str] (default None), and target_index_db_path is None unless target_index is truthy. The PR passes both to push_index_db_artifact without an explicit guard. The function handles this safely (its guard short-circuits on None), but an explicit guard would improve readability.

Info

• [missing-authorization] iib/workers/tasks/build_containerized_merge.py — PR Correct targets for pushing db artifact #1355 is a bug fix but has no linked issue. Bug fixes should trace to an issue documenting the observed failure and authorization for the fix.

[fullsend-ai-review]

See the review comment for full details.

[lipoja]

/agentic_review

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 3:20 PM UTC · Ended 3:29 PM UTC
Commit: 8207ad4 · View workflow run →

[qodo-for-releng]

Code review by qodo was updated up to the latest commit 02e03f1

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:20 PM UTC · Completed 3:29 PM UTC
Commit: 8207ad4 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:20 AM UTC · Completed 6:31 AM UTC
Commit: 8207ad4 · View workflow run →

[fullsend-ai-review]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:34 AM UTC · Completed 6:46 AM UTC
Commit: 8207ad4 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:34 AM UTC · Completed 11:45 AM UTC
Commit: 8207ad4 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:13 PM UTC · Completed 12:27 PM UTC
Commit: 8207ad4 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:29 PM UTC · Completed 12:42 PM UTC
Commit: 8207ad4 · View workflow run →

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 12:55 PM UTC · Completed 1:07 PM UTC
Commit: 8207ad4 · View workflow run →

[lipoja]

/agentic_review

[fullsend-ai-review]

🤖 Review · ❌ Terminated · Started 1:47 PM UTC · Ended 1:59 PM UTC
Commit: 8207ad4 · View workflow run →

[qodo-for-releng]

Code Review by Qodo

Sorry, something went wrong

We weren't able to complete the code review on our side. Please try again

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:47 PM UTC · Completed 1:59 PM UTC
Commit: 8207ad4 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:12 PM UTC · Completed 2:24 PM UTC
Commit: 8207ad4 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:49 PM UTC · Completed 3:01 PM UTC
Commit: 8207ad4 · View workflow run →