Repository navigation
Fix set_registry_token call for token usa - #1370
Conversation
|
🤖 Finished Review · ✅ Success · Started 6:30 AM UTC · Completed 6:50 AM UTC |
1bc0ed6 to
ea42f2c
Compare
PR Summary by QodoScope overwrite_from_index_token to same-registry images during resolution
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
ReviewFindingsHigh
Medium
Low
Next steps:
Previous runReviewReason: stale-head The review agent reviewed commit Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsMedium
Low
Labels: PR fixes token scoping behavior in registry authentication logic — a bug fix Next steps:
Previous run (4)ReviewReason: stale-head The review agent reviewed commit |
|
/fs-review |
Code Review by Qodo
1.
|
|
🤖 Finished Review · ✅ Success · Started 6:50 AM UTC · Completed 7:07 AM UTC |
lipoja
left a comment
There was a problem hiding this comment.
Once you address the [medium] logic-error you can merge it.
LGTM.
ea42f2c to
f0936e7
Compare
|
🤖 Finished Review · ✅ Success · Started 1:35 PM UTC · Completed 2:16 PM UTC Commit: |
Superseded by updated review
|
LGTM, lets fix the linting errors and remove the unused code. |
|
Apart from the comments above LGTM. Once we migrate to the Pods, would it make sense, to craft the credentials file (docker config) per command? That way, we would have better control over the credentials. |
|
Hello - wondering how long before this gets merged? I've been waiting for this to be done so I can once again do 'manual' IIB job submissions (I have the relevent credentials) to remove or add bundles in the Certified, Marketplace, or Community bundle indexes. |
yashvardhannanavati
left a comment
There was a problem hiding this comment.
@chandwanitulsi could you please fix the tests too?
f0936e7 to
7468a9b
Compare
|
🤖 Finished Review · ✅ Success · Started 1:34 AM UTC · Completed 2:02 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $9.63 |
…h-scoped Docker auth Fixes CLOUDDST-32419 and CLOUDDST-32824 by applying the overwrite token only where worker Docker config cannot already pull the image, and by avoiding blanket token stamping that broke unrelated private fragments and namespace template credentials. Co-authored-by: Cursor <cursoragent@cursor.com>
7468a9b to
906a20c
Compare
|
Risk Assessment: moderate (2/5) DetailsRe-review anchoring applied: Tier 1 signals are unchanged from the prior assessment (large blast radius offset by zero protected/security paths, 0.43 test ratio, established non-bot author), and Tier 2 shows negligible 30-day churn and no recent regression activity, providing no articulable reason to deviate from the prior score of 2. Previous runRisk Assessment: moderate (2/5) DetailsModerate risk: the large blast radius (core worker task files touching build, opm, and utility layers) elevates Tier 1, but strong test coverage (0.43 ratio), no protected or security-sensitive paths, zero regression history, and an established author keep the composite score at 2. |
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 2:03 AM UTC · Completed 2:31 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $9.72 |
Reviewed manually
bc84f69
into
release-engineering:master
|
🤖 Finished Retro · ✅ Success · Started 9:43 AM UTC · Completed 9:55 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $3.01 |
Retro: PR #1370 — Scope overwrite_from_index_tokenTimeline
Review QualityThe agent produced ~22 findings across 3 review rounds (~50% actionable). Human reviewers produced 6 findings (~100% actionable). Agent strengths: Caught undisclosed Agent weaknesses: Missed the deepest architectural finding — yashvardhannanavati identified Existing Issue Evidence
Autonomy ReadinessThe agent is not ready for increased autonomy on complex auth/credential changes in this repo — it missed the deepest architectural finding and inflated severity on intentional design decisions. However, its strength at catching undisclosed behavioral changes (append=True) suggests it adds value as a complementary reviewer when combined with human domain experts. Proposals skipped (target repo not allowed)File manually or update
|
* Scope overwrite_from_index_token to same-namespace images without path-scoped Docker auth (#1370) Fixes CLOUDDST-32419 and CLOUDDST-32824 by applying the overwrite token only where worker Docker config cannot already pull the image, and by avoiding blanket token stamping that broke unrelated private fragments and namespace template credentials. Co-authored-by: Tulsi Chandwani <tchandwa@tchandwa.users.ipa.redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> * Scope overwrite_from_index_token at the containerized handler call sites cca5101 (#1370) landed the scoping primitives -- docker_config_has_path_auth_for_image, get_images_needing_overwrite_token and the list-aware set_registry_token -- and converted the old worker architecture to use them. The containerized handlers were left on the pre-fix pattern, so CLOUDDST-32419 and CLOUDDST-32824 still reproduce on every request routed to build_containerized_*. Port the call sites, applying the token only where worker Docker config cannot already pull the image: - build_containerized_add: the bundle resolve stamped from_index rather than the bundles it actually pulls, so a private same-namespace bundle failed while an unrelated namespace lost its template credentials. Scope it to the bundles instead. _opm_registry_add pulls every bundle again after that window has closed and was entirely unguarded; give it the same scoped window over the digest-resolved specs. - build_containerized_fbc_operations: collapse the per-fragment loop of unfiltered context managers into one scoped window, and pass from_index down to opm_registry_add_fbc_fragment_containerized. - opm_operations.opm_registry_add_fbc_fragment_containerized: takes from_index and scopes the extract loop, which pulls each fragment. set_registry_token no-ops on an empty list, so one window covers both the "some fragments need the token" and "none do" cases. The utils import stays function-local -- utils imports get_list_bundles from this module, so a top-level import would be circular. - Opm.set_opm_version reads a label off from_index via skopeo inspect and was unguarded in add, rm and fbc-operations; a private index failed there before reaching any of the above. Wrap all three. from_index auth is deliberately not stamped at the bundle and fragment resolve steps: those calls do not pull the index. prepare_request_for_build and the later from_index accessors carry the token for the index itself. utils.set_registry_auths gains a docstring note that it restores the iib_docker_config_template baseline on exit rather than the configuration in place on entry, so nesting it inside another credential window drops that outer window's auths. No caller does this today; the note is to keep it that way. Tests seed _load_docker_config_auths with a registry-only key to confirm that registry-level auth does not count as path-scoped coverage. The two rm tests that reach the credential path now mock set_registry_token -- previously they called through to the real set_registry_auths, which writes to the developer's ~/.docker/config.json. build_containerized_merge is intentionally left for a follow-up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yashvardhan Nanavati <yashn@bu.edu> --------- Signed-off-by: Yashvardhan Nanavati <yashn@bu.edu> Co-authored-by: Tulsi Chandwani <17147736+chandwanitulsi@users.noreply.github.com> Co-authored-by: Tulsi Chandwani <tchandwa@tchandwa.users.ipa.redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Assisted-by: Cursor/Gemini