Refactor set_registry_token to support repository-specific auth

[chandwanitulsi]

Updated the set_registry_token function to allow for appending tokens specific to a repository while leaving registry-level credentials intact. Adjusted related tests to verify the new behavior, ensuring proper handling of authentication entries for both repository and registry levels.

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 2:28 AM UTC · Completed 2:40 AM UTC
Commit: e686f8c · View workflow run →

[fullsend-ai-review]

Review

Findings

Low

• [missing-authorization] N/A — This PR lacks a linked issue. The PR modifies authentication behavior which is a non-trivial functional change. Consider linking to an issue that authorizes the change.

• [scope-tier-mismatch] N/A — The PR title claims "Refactor" but introduces new functional behavior: repository-specific auth keys, a new function, and changed authentication scope. The PR body already describes the behavioral change, but the title could more accurately reflect the nature of the change.

• [test adequacy] tests/test_workers/test_tasks/test_build_fbc_operations.py:360 — test_handle_fbc_operation_request_with_build_tags and other tests still use unqualified pull specs like from_index='from-index:latest'. These are safe because overwrite_from_index_token is None (making set_registry_token a no-op), but would fail if the test is extended with a token.

• [test adequacy] tests/test_workers/test_tasks/test_build.py:747 — Multiple tests in test_build.py still use unqualified pull specs. These pass because set_registry_token is mocked at the build module level, but could be updated for consistency.

• [architectural-coherence] iib/workers/tasks/utils.py — The new _docker_auth_key_for_image introduces multi-tier authentication key selection. The function's docstring explains the strategy well, but broader project documentation could benefit from describing this policy.

• [documentation-staleness] README.md:319 — The README update changes "registry" to "specific repository" but doesn't explain the conditions under which repository-specific vs registry-level auth is used.

• [edge case handling] iib/workers/tasks/utils.py:590 — The function relies on ImageName.parse for deeply nested paths (e.g. registry.example.com/a/b/c:tag). Behavior should work correctly but the docstring does not describe this edge case.

• [logging-format-consistency] iib/workers/tasks/utils.py:665 — The old log message had a trailing space before the closing quote (likely unintentional). The new message correctly removes it.

Info

• [Principle of Least Privilege] iib/workers/tasks/utils.py:577 — The new _docker_auth_key_for_image function narrows Docker auth credentials from registry-level to repository-level when namespace is present. Positive security improvement with fail-closed error handling.

• [Authentication Scope] iib/workers/tasks/utils.py:665 — The auth key update correctly writes the scoped token without overwriting broader credentials. When append=True, existing config is preserved.

• [code-organization] iib/workers/tasks/utils.py:577 — The new helper function is well-placed immediately before its caller, following established codebase patterns.

• [docstring-completeness] iib/workers/tasks/utils.py:577 — The new function has a well-structured docstring matching codebase conventions.

Previous run

Review

Findings

Medium

• [error handling / behavior change] iib/workers/tasks/utils.py:593 — _docker_auth_key_for_image raises IIBError for pull specifications that ImageName.parse cannot resolve to a registry (e.g., short names like ubuntu:latest). The old code silently used whatever ImageName.parse(...).registry returned as the Docker auth key. This is a runtime behavior change: callers that previously succeeded (albeit with a wrong/empty auth key) will now get an IIBError. There is no input validation at the API layer that rejects short-name pull specifications for from_index or fbc_fragments, so a user-supplied short name will now cause a hard failure instead of a silent misconfiguration.
  Remediation: Consider adding a user-facing validation in the API layer that rejects pull specifications without a resolvable registry, so users get a clear 400 error rather than a worker-side IIBError during processing.

Low

• [edge case] iib/workers/tasks/utils.py:586 — _docker_auth_key_for_image constructs the auth key as f'{registry}/{image_name.namespace}/{image_name.repo}' when namespace is present. If ImageName.parse ever produces a state where namespace is truthy but repo is empty/None, the resulting auth key would have a trailing slash. This is theoretical since ImageName.parse should not produce that state for realistic input.

• [authentication] iib/workers/tasks/utils.py:588 — The auth key changes from registry-level to registry/namespace/repo-level when a namespace is present. The non-append call site (get_index_image_info) previously set a registry-wide token but will now set a more narrowly-scoped key. If a container runtime uses a different matching algorithm, the narrower key might not be matched. This is a fail-closed outcome (availability risk, not confidentiality).
  Remediation: Verify that all container runtimes in use (Docker, Podman, Buildah) support registry/namespace/repo-level auth key matching.

• [docstring-convention] iib/workers/tasks/utils.py:577 — _docker_auth_key_for_image is missing :return: and :rtype: docstring tags. Other private helper functions in this file consistently include both.
  Remediation: Add :return: and :rtype: str to the docstring.

• [log-message-consistency] iib/workers/tasks/utils.py:665 — The updated log message 'Setting the override token for the image %s' logs auth_key, which may be a registry-level key or a repository-level key. Calling it "image" is imprecise.
  Remediation: Consider 'Setting the override token using auth key %s'.

Info

• [test adequacy] tests/test_workers/test_tasks/test_utils.py — New tests cover namespaced images, registry-only images, short names, namespace-without-registry edge case, and append-mode overwrite. Coverage is adequate for the new behavior.

• [docstring-wording] iib/workers/tasks/utils.py:579 — The docstring summary uses "Return" as the verb; the codebase also uses "Get" in similar helpers. Both are acceptable per PEP 257.

• [variable-naming] iib/workers/tasks/utils.py:648 — auth_entry is a single-use intermediate variable that could be inlined into the update call.

• [naming-alignment] iib/workers/tasks/utils.py — _docker_auth_key_for_image returns a key scoped to either registry or repository depending on input. The name is reasonable but does not communicate the conditional scope.

Previous run (2)

Review

Findings

Medium

• [stale-doc] README.md:318 — Documentation states that IIB "will use this file as a base and set the overwrite_from_index_token for the registry of the from_index container image." This is now inaccurate because the auth token is set for the repository-specific key (registry/namespace/repo) rather than just the registry.
  Remediation: Update to clarify that the token is set for the specific repository rather than the registry.

Low

• [edge-case] iib/workers/tasks/utils.py:589 — _docker_auth_key_for_image does not verify that registry is non-empty when namespace is truthy. If ImageName.parse ever returns a truthy namespace with a falsy registry, the key would be /namespace/repo. The risk is theoretical given standard image reference parsing, but a defensive guard would be prudent.
• [authentication-scope-reduction] iib/workers/tasks/utils.py:577 — With append=True, the auth token is now scoped to registry/namespace/repo instead of registry. Operations inside the context manager that pull sibling repos from the same registry will not be authenticated by this token. The template config likely carries registry-level credentials, but this should be verified for all callsites (especially deprecate_bundles in build.py).
• [test-inadequate] tests/test_workers/test_tasks/test_utils.py:123 — No test case for a digest-referenced image without a namespace (e.g., localhost:5000/myimage@sha256:abc), which is a common pattern for from_index_resolved values.
• [test-inadequate] tests/test_workers/test_tasks/test_utils.py:58 — No test for the append=False path with a namespace-less image, which would exercise the registry-only auth key fallback through the full set_registry_token → set_registry_auths flow.
• [scope-alignment] iib/workers/tasks/utils.py:574 — The PR title says "Refactor" but the change modifies behavior (auth key structure). The PR description is transparent about this, but the title could be more precise.
• [architectural-coherence] iib/workers/tasks/utils.py:574 — The approach of using registry/namespace/repo as Docker auth keys is valid and supported by Docker/podman, but documenting evidence of compatibility testing with the container tools IIB depends on would increase confidence.
• [docstring-consistency] iib/workers/tasks/utils.py:577 — The new helper uses backticks around technical terms in its docstring summary while similar helpers (e.g., _get_container_image_name) use simpler phrasing.
• [error-message-consistency] iib/workers/tasks/utils.py:594 — The error message phrasing pattern (action + "for" + purpose) is not commonly used elsewhere in this codebase.
• [missing-doc] README.md:172 — The "Registry Authentication" section does not document that auth can now be configured at the repository level.

Info

• [credential-scoping] iib/workers/tasks/utils.py:577 — The change narrows Docker auth credentials from registry-level to repository-level scope when namespace is present. This is a security improvement.
• [integration-impact] iib/workers/tasks/utils.py:625 — The change affects all callsites using append=True, including critical paths in build_merge_index_image.py and build.py (~12 callsites).
• [log-message-consistency] iib/workers/tasks/utils.py:641 — The log message says "image" but logs the auth_key value, which is a derived key (registry or registry/namespace/repo), not an image reference.

Previous run (3)

Review

Findings

Medium

• [behavioral-change] iib/workers/tasks/utils.py — Several callers (e.g., build.py line 839) use set_registry_token with from_index and append=True to authenticate operations that pull different images (bundles) from the same registry. Previously, the token was set at the registry level which matched all repos. Now it is set at the repo level (registry/namespace/repo), so the token will not match bundle pulls on different repo paths unless existing broader config entries already cover them. With append=True the existing config is preserved, mitigating this for most deployments — but if any deployment relies solely on overwrite_from_index_token without broader template credentials, those pulls will fail.
  Remediation: Verify that deployment configurations have adequate registry-level credentials in the docker config template (iib_docker_config_template).

• [edge-case] tests/test_workers/test_tasks/test_utils.py — The test_docker_auth_key_for_image parametrized cases do not include images with a dotted registry and a single repo component (e.g., registry.example.com/repo:latest). Depending on how ImageName.parse classifies this input, it could reveal unexpected behavior in the registry-only fallback.
  Remediation: Add a test case such as ('registry.example.com/repo:latest', 'registry.example.com') to confirm the fallback.

• [scope-alignment] iib/workers/tasks/utils.py — The PR body states it allows "appending tokens specific to a repository while leaving registry-level credentials intact." This is accurate for the append=True path (verified by test_set_registry_token_append_overwrites_repo_auth), but when append=False, only the repo-specific key is written — a behavioral change from the old registry-level key.
  Remediation: Clarify in the PR description that append=False now writes a repo-specific key instead of a registry key.

• [test-coverage-gap] tests/test_workers/test_tasks/test_utils.py — No test covers append=True when the image has no namespace (auth key falls back to registry-only), leaving that interaction path unverified.
  Remediation: Add a test for append=True with a no-namespace image (e.g., localhost:5000/myimage:tag).

Low

• [missing-authorization] iib/workers/tasks/utils.py — Non-trivial change (117 lines) lacks a linked issue. The PR has the ready-for-merge label indicating organizational review, but linking a tracking issue would improve traceability.

• [log-message-precision] iib/workers/tasks/utils.py:665 — The log message says "Setting the override token for the image %s" but auth_key can be either a bare registry (e.g., localhost:5000) or registry/namespace/repo. Calling it "the image" is slightly misleading for debug logs.

• [documentation-completeness] iib/workers/tasks/utils.py:577 — _docker_auth_key_for_image() is missing :return: and :rtype: in its docstring. Minor nit — the function has Python type annotations (-> str) which serve the same purpose.

• [architecture-coherence] iib/workers/tasks/utils.py — The helper function's docstring clearly explains when each auth key format is used; consider adding concrete examples for additional clarity.

Previous run (4)

Review

Findings

Low

• [edge-case] iib/workers/tasks/utils.py:591 — The fallback path return container_image in _docker_auth_key_for_image returns the raw pull specification including the tag or digest (e.g., myimage:tag). Docker config auths keys should not include tags or digests. While this path is unreachable with current callers (all pass fully-qualified images with registries), it would produce an invalid auth key if hit.
  Remediation: Strip the tag/digest from container_image before the final return, or raise an error if no registry can be determined.

• [test-inadequate] tests/test_workers/test_tasks/test_utils.py:120 — The test_docker_auth_key_for_image parametrized test does not cover the fallback path where ImageName.parse yields no registry and no namespace. Additionally, it does not test images with digests (e.g., registry.redhat.io/ns/repo@sha256:abc123) to verify that tags/digests are not included in the auth key.

• [api-contract] iib/workers/tasks/utils.py:655 — This PR changes the auth key from registry-level (registry.redhat.io) to repo-level (registry.redhat.io/ns/repo) for images with a namespace. Non-append callers produce a docker config with only the repo-scoped key plus any template entries. Docker, Podman, and Buildah all support repo-level auth key matching, and the template merge (via set_registry_auths) preserves broader registry-level credentials when configured.
  Remediation: Verify that the target container runtimes used in the deployment environment reliably match repo-level auth keys.

Info

• [authentication scope change] iib/workers/tasks/utils.py:577 — The new _docker_auth_key_for_image function narrows credential scope from registry-level to repo-level when the image includes a namespace. All call sites target the same image used to derive the auth key. No fail-open path was identified.

• [credential-handling] iib/workers/tasks/utils.py:590 — The fallback path returns container_image verbatim as a JSON object key. No injection risk; Docker daemon will not match the credential, resulting in fail-closed behavior.

• [secrets-handling] iib/workers/tasks/utils.py:643 — The log.debug call logs the auth_key (public image reference), not the credential. No credential leakage.

• [sub-agent-failure] N/A — The style-conventions, intent-coherence, and docs-currency sub-agents did not return findings: model claude-sonnet-4-5@20250929 is not available on the deployment. These are non-critical dimensions; correctness and security reviews completed successfully.

Previous run (5)

Review

Findings

Medium

• [edge-case] iib/workers/tasks/utils.py:587 — _docker_auth_key_for_image checks if image_name.namespace before verifying that registry is truthy. If ImageName.parse returns a result where namespace is set but registry is empty (e.g., Docker Hub shorthand like library/ubuntu parses as namespace='library', repo='ubuntu', registry=''), the function returns '/{namespace}/{repo}' with a leading slash — an invalid auths key that no container runtime would match, causing silent authentication failure.
  Remediation: Guard the namespace branch to also require registry: if image_name.namespace and registry:, and fall through to subsequent branches otherwise.

Low

• [test-inadequate] tests/test_workers/test_tasks/test_utils.py — The test_docker_auth_key_for_image parametrized tests do not cover digest-based references (e.g., registry.redhat.io/ns/repo@sha256:abcdef) or Docker Hub canonical references (e.g., docker.io/library/ubuntu:latest). Since _docker_auth_key_for_image is now the sole determinant of which auths key receives credentials, these common pull spec formats should be verified.

Info

• [sub-agent-failure] N/A — The style-conventions, intent-coherence, and docs-currency sub-agents did not return findings due to model unavailability (claude-sonnet-4-5@20250929 not available on deployment). These dimensions were not evaluated in this review run.

Previous run (6)

Review

Findings

Medium

• [edge-case] iib/workers/tasks/build_merge_index_image.py:193 — The auth key is now scoped to repository level instead of registry level. In build_merge_index_image.py:193, set_registry_token is called with target_index as the container_image, but operations inside the context manager access source_from_index (e.g., is_image_fbc(source_from_index), opm_registry_add_fbc with from_index=source_from_index). If source_from_index is on the same registry but a different repo, the new repo-scoped auth key will not cover it. With append=True the existing docker config is preserved, so if registry-level credentials exist in the template they would still work, but callers that relied solely on this token for registry-wide access would break.
  Remediation: Audit all call sites where operations inside the set_registry_token context manager access images on the same registry but different repo paths (especially build_merge_index_image.py:193-209) and ensure those images have separate authentication configured.

• [architectural-coherence] iib/workers/tasks/utils.py:609 — Behavioral change not fully reflected in function contract. The diff updates the append parameter docstring but the opening docstring still says "Configure authentication to the registry" (line 582) and "the registry this token is for" (lines 588-589). The append parameter description should also be expanded to clarify the new repository-level scoping behavior. See also: [doc-style] and [scope-alignment] findings at this location.
  Remediation: Update the full docstring to reflect repository-specific auth behavior and clarify implications for existing callers.

Low

• [test-inadequate] tests/test_workers/test_tasks/test_utils.py:79 — The code has three branches for constructing auth_key (namespace+repo, repo-only, registry-only) but only the namespace+repo path is tested. The elif (repo without namespace) and else (registry-only, no repo) branches have no test coverage.

• [credential-scope] iib/workers/tasks/utils.py:614 — When ImageName.parse() returns empty namespace but non-empty repo for an ambiguous pull spec, auth_key could be constructed as registry/repo — a path that Docker credential matching may or may not resolve depending on the container runtime. This is not a security issue (fail-closed, not fail-open) but could cause unexpected auth failures.

Info

• [credential-handling] iib/workers/tasks/utils.py:633 — The added registry_auths.setdefault('auths', {}) is useful defensive coding: when append=True and the loaded JSON config lacks an auths key, the initial {'auths': {}} dict is replaced by json.load(), so this setdefault prevents a KeyError.

Previous run (7)

Review

Reason: stale-head

The review agent reviewed commit 052ecd80c3913f93c8e2c11c9c9eac65658687ab but the PR HEAD is now 96f55ee57ef7c8f4d3ec11e748818ccecf7c856f. This review was discarded to avoid approving unreviewed code.

Previous run (8)

Review

Reason: stale-head

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

[fullsend-ai-review]

🤖 Finished Review · ❌ Failure · Started 2:41 AM UTC · Completed 2:55 AM UTC
Commit: e686f8c · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:57 AM UTC · Completed 3:07 AM UTC
Commit: e686f8c · View workflow run →

[fullsend-ai-review]

🤖 Review · Started 3:04 AM UTC
Commit: e686f8c · View workflow run →

[chandwanitulsi]

Fixed both the low and [architectural-coherence] medium findings by full-send, the merge-index-image finding is a partially valid case as we don't use overwrite_from_index token for source_image and that seems to be separate issue

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:04 AM UTC · Completed 3:21 AM UTC
Commit: e686f8c · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 5:44 PM UTC · Completed 5:56 PM UTC
Commit: e686f8c · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:28 PM UTC · Completed 6:41 PM UTC
Commit: e686f8c · View workflow run →

[yashvardhannanavati]

@chandwanitulsi the CI seems to be failing

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:45 AM UTC · Completed 3:00 AM UTC
Commit: e686f8c · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:50 AM UTC · Completed 4:05 AM UTC
Commit: e686f8c · View workflow run →

[yashvardhannanavati]

one minor comment. otherwise, LGTM

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:36 PM UTC · Completed 2:50 PM UTC
Commit: e686f8c · View workflow run →