Open
Conversation
Signed-off-by: Aloui-Ikram <ikramaloui145@example.com> Signed-off-by: bupd <bupdprasanth@gmail.com>
- Added `InheritedAccessories` to the Artifact model to prevent mixing inherited parent signatures with direct artifact accessories. - Updated `populateAccessories` to store parent signatures in the new field to maintain data integrity. - Fixed `ListAccessories` handler to properly parse multi-type UI queries using `strings.Contains` and serve inherited signatures when requested. Signed-off-by: Aloui-Ikram <ikramaloui145@example.com> Signed-off-by: bupd <bupdprasanth@gmail.com>
The new populateAccessories logic calls artMgr.ListReferences whenever an artifact has no direct cosign signature. Several existing tests (TestList, TestListWithLatest, TestDeleteDeeply, TestWalk) mock accMgr.List to return empty accessories but did not set up a ListReferences expectation, causing the mock to panic. Add the missing mock expectations returning an empty reference slice so the new code path exits cleanly. Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
Signed-off-by: bupd <bupdprasanth@gmail.com>
📝 WalkthroughWalkthroughThis change introduces a pluggable manifest abstraction registry system to extract metadata from different OCI manifest types (v1, v2, index). It adds a new BuildKit attestation accessory type detected from OCI index manifests. Legacy BuildKit attestation references are filtered during artifact assembly and synthesized as accessories during population. The artifact model gains fields for accessory candidates and inherited accessories. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as ListAccessories Handler
participant Controller as Artifact Controller
participant Registry as Manifest Abstractor<br/>Registry
participant Abstractor as Index Manifest<br/>Abstractor
participant Accessory as Accessory Model
Client->>Handler: List Accessories
Handler->>Controller: GetByDigest with Accessory Details
Controller->>Controller: AbstractMetadata
Controller->>Registry: getManifestAbstractor(mediaType)
Registry-->>Controller: indexManifestAbstractor
Controller->>Abstractor: AbstractMetadata(context, artifact, content)
Abstractor->>Abstractor: Parse OCI Index Manifest
loop For Each Manifest in Index
Abstractor->>Abstractor: Check if BuildKit Attestation<br/>(via descriptor annotations)
alt Is BuildKit Attestation
Abstractor->>Accessory: Create AccessoryCandidate<br/>(type: attestation.buildkit)
Accessory-->>Abstractor: AccessoryCandidate
Abstractor->>Abstractor: Aggregate Size & Add to Candidates
else Regular Child
Abstractor->>Abstractor: Fetch Child Artifact<br/>Create Reference
Abstractor->>Abstractor: Add Reference
end
end
Abstractor-->>Controller: Artifact with Candidates & References
Controller->>Controller: Ensure Accessory Candidates<br/>in Transaction
Controller-->>Handler: Artifact with Accessories
Handler-->>Client: Accessory List
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Important Merge conflicts detected (Beta)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
component/core
component/jobservice
component/portal
tests
labels
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Preview images for this PR are available in
Verify a preview image: Verify SBOM attestation: |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/controller/artifact/manifest_abstractor_v1.go">
<violation number="1" location="src/controller/artifact/manifest_abstractor_v1.go:62">
P1: Register `schema1.MediaTypeManifest` here too; otherwise unsigned Docker v1 manifests remain unsupported after the refactor.</violation>
</file>
<file name="src/controller/artifact/controller.go">
<violation number="1" location="src/controller/artifact/controller.go:810">
P2: This inherited-signature lookup adds extra per-artifact queries to the default artifact GET/LIST path, but `InheritedAccessories` are not returned there. It creates an avoidable N+1 regression for artifact listing/detail responses.</violation>
</file>
<file name="src/controller/artifact/manifest_abstractor.go">
<violation number="1" location="src/controller/artifact/manifest_abstractor.go:41">
P2: Make alias registration atomic; a duplicate later in `mediaTypes` currently leaves earlier aliases registered and can cause some manifest media types to fail with `unsupported manifest media type`.</violation>
</file>
<file name="src/server/v2.0/handler/artifact.go">
<violation number="1" location="src/server/v2.0/handler/artifact.go:392">
P2: Checking inheritance only when `len(accs) == 0` misses inherited cosign signatures whenever the same query also returns a direct SBOM accessory.</violation>
</file>
<file name="src/controller/artifact/manifest_abstractor_index.go">
<violation number="1" location="src/controller/artifact/manifest_abstractor_index.go:134">
P2: Propagate attestation-inspection errors instead of silently downgrading the manifest to a normal child reference.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
|
|
||
| func init() { | ||
| mediaTypes := []string{"", "application/json", schema1.MediaTypeSignedManifest} |
There was a problem hiding this comment.
P1: Register schema1.MediaTypeManifest here too; otherwise unsigned Docker v1 manifests remain unsupported after the refactor.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/controller/artifact/manifest_abstractor_v1.go, line 62:
<comment>Register `schema1.MediaTypeManifest` here too; otherwise unsigned Docker v1 manifests remain unsupported after the refactor.</comment>
<file context>
@@ -0,0 +1,68 @@
+}
+
+func init() {
+ mediaTypes := []string{"", "application/json", schema1.MediaTypeSignedManifest}
+ if err := registerManifestAbstractor(func(a *abstractor) manifestAbstractor {
+ return &manifestV1Abstractor{blobMgr: a.blobMgr}
</file context>
| manifestAbstractorLock.Lock() | ||
| defer manifestAbstractorLock.Unlock() | ||
|
|
||
| for _, mediaType := range mediaTypes { |
There was a problem hiding this comment.
P2: Make alias registration atomic; a duplicate later in mediaTypes currently leaves earlier aliases registered and can cause some manifest media types to fail with unsupported manifest media type.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/controller/artifact/manifest_abstractor.go, line 41:
<comment>Make alias registration atomic; a duplicate later in `mediaTypes` currently leaves earlier aliases registered and can cause some manifest media types to fail with `unsupported manifest media type`.</comment>
<file context>
@@ -0,0 +1,61 @@
+ manifestAbstractorLock.Lock()
+ defer manifestAbstractorLock.Unlock()
+
+ for _, mediaType := range mediaTypes {
+ if _, ok := manifestAbstractorFactories[mediaType]; ok {
+ return fmt.Errorf("manifest abstractor for media type %s already exists", mediaType)
</file context>
| } | ||
|
|
||
| // If no direct signature found, check for Cosign signatures inherited from a parent OCI index. | ||
| if len(accs) == 0 { |
There was a problem hiding this comment.
P2: Checking inheritance only when len(accs) == 0 misses inherited cosign signatures whenever the same query also returns a direct SBOM accessory.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/server/v2.0/handler/artifact.go, line 392:
<comment>Checking inheritance only when `len(accs) == 0` misses inherited cosign signatures whenever the same query also returns a direct SBOM accessory.</comment>
<file context>
@@ -386,6 +388,27 @@ func (a *artifactAPI) ListAccessories(ctx context.Context, params operation.List
}
+ // If no direct signature found, check for Cosign signatures inherited from a parent OCI index.
+ if len(accs) == 0 {
+ // Use strings.Contains because the UI sends a list of types (e.g. type={signature.cosign ...})
+ if strings.Contains(lib.StringValue(params.Q), "signature.cosign") {
</file context>
| } | ||
|
|
||
| func (m *indexManifestAbstractor) isBuildKitAttestationManifest(repository, digest string) bool { | ||
| manifest, _, err := m.regCli.PullManifest(repository, digest) |
There was a problem hiding this comment.
P2: Propagate attestation-inspection errors instead of silently downgrading the manifest to a normal child reference.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/controller/artifact/manifest_abstractor_index.go, line 134:
<comment>Propagate attestation-inspection errors instead of silently downgrading the manifest to a normal child reference.</comment>
<file context>
@@ -0,0 +1,168 @@
+}
+
+func (m *indexManifestAbstractor) isBuildKitAttestationManifest(repository, digest string) bool {
+ manifest, _, err := m.regCli.PullManifest(repository, digest)
+ if err != nil {
+ return false
</file context>
Signed-off-by: bupd <bupdprasanth@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/pkg/accessory/model/attestation/attestation_test.go (1)
22-27: Fail fast on accessory construction errors inSetupSuite.Discarding the
model.Newerror here turns a registration/constructor regression into a nil-interface panic across the suite, which makes the failure much harder to diagnose.💡 Proposed change
func (suite *AttestationTestSuite) SetupSuite() { suite.digest = suite.DigestString() suite.subDigest = suite.DigestString() - suite.accessory, _ = model.New(model.TypeBuildKitAttestation, model.AccessoryData{ + var err error + suite.accessory, err = model.New(model.TypeBuildKitAttestation, model.AccessoryData{ ArtifactID: 1, SubArtifactDigest: suite.subDigest, Size: 1234, Digest: suite.digest, }) + suite.Require().NoError(err) + suite.Require().NotNil(suite.accessory) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pkg/accessory/model/attestation/attestation_test.go` around lines 22 - 27, In SetupSuite, don't ignore the error returned by model.New; capture the returned error from model.New(model.TypeBuildKitAttestation, ...) and fail fast when it is non-nil (e.g., using suite.Require().NoError(err) or suite.T().Fatalf) before assigning suite.accessory, so a constructor/registration regression surfaces immediately instead of causing nil-interface panics later; update the SetupSuite code around the model.New call to check err and handle it accordingly.src/controller/artifact/controller.go (1)
817-841: Consider handling multiple parent Cosign signatures.The current implementation returns immediately after finding the first Cosign signature from any parent (Line 837). If a child artifact is referenced by multiple parent indexes, each potentially having different Cosign signatures, only the first one found is added to
InheritedAccessories.This may be intentional to avoid UI clutter, but if completeness is desired, consider collecting all parent Cosign signatures:
♻️ Optional: Collect all inherited Cosign signatures
if !hasCosign { parents, err := c.artMgr.ListReferences(ctx, &q.Query{ Keywords: map[string]any{"ChildID": art.ID}, }) if err != nil { log.Errorf("failed to list parent references of artifact %d: %v", art.ID, err) return } if len(parents) == 0 { return } + seen := map[int64]struct{}{} for _, p := range parents { parentAccs, err := c.accessoryMgr.List(ctx, q.New(q.KeyWords{"SubjectArtifactID": p.ParentID})) if err != nil { continue } for _, pAcc := range parentAccs { if pAcc.GetData().Type == accessorymodel.TypeCosignSignature { + if _, ok := seen[pAcc.GetData().ArtifactID]; ok { + continue + } + seen[pAcc.GetData().ArtifactID] = struct{}{} art.InheritedAccessories = append(art.InheritedAccessories, pAcc) - return } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/controller/artifact/controller.go` around lines 817 - 841, The code returns after adding the first parent cosign signature, so other parent signatures may be skipped; modify the block where hasCosign is false (using c.artMgr.ListReferences and c.accessoryMgr.List) to continue scanning all parents instead of returning after the first match: for each parent (from ListReferences) iterate parentAccs and for every accessory whose GetData().Type == accessorymodel.TypeCosignSignature append it to art.InheritedAccessories, and only return (or exit) after all parents have been processed; ensure you remove the early return inside the inner loop so multiple cosign signatures from different parents are collected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/controller/artifact/manifest_abstractor_index.go`:
- Around line 96-110: The code silently treats transient probe/pull/JSON errors
from isBuildKitAttestationManifest as "false" and falls back to normal
child-reference handling; change isBuildKitAttestationManifest to surface errors
(e.g., return (bool, error) or add a variant that returns an error) and update
callers (including toBuildKitAttestationCandidate and the other call site around
lines 133-159) to check the error and return it (instead of returning nil,nil
and downgrading the descriptor) so probe failures are propagated rather than
reclassifying attestations.
In `@src/server/v2.0/handler/artifact.go`:
- Around line 391-410: The current logic gates inherited Cosign lookup on
len(accs) == 0 which incorrectly prevents adding an inherited Cosign when other
direct accessories exist; change the condition to check whether a direct Cosign
is already present (e.g., scan accs for any acc.GetData().Type ==
accessorymodel.TypeCosignSignature) and only skip inheritance if a direct Cosign
exists; if no direct Cosign is found, call a.artCtl.Get(ctx, art.ID,
&artifact.Option{WithAccessory: true}) to retrieve artWithAccs, scan
artWithAccs.Accessories and artWithAccs.InheritedAccessories for
TypeCosignSignature, append any inherited Cosign to accs, and update total
appropriately (and ensure this merge happens before final pagination/counting so
the inherited item is included in the paged response).
---
Nitpick comments:
In `@src/controller/artifact/controller.go`:
- Around line 817-841: The code returns after adding the first parent cosign
signature, so other parent signatures may be skipped; modify the block where
hasCosign is false (using c.artMgr.ListReferences and c.accessoryMgr.List) to
continue scanning all parents instead of returning after the first match: for
each parent (from ListReferences) iterate parentAccs and for every accessory
whose GetData().Type == accessorymodel.TypeCosignSignature append it to
art.InheritedAccessories, and only return (or exit) after all parents have been
processed; ensure you remove the early return inside the inner loop so multiple
cosign signatures from different parents are collected.
In `@src/pkg/accessory/model/attestation/attestation_test.go`:
- Around line 22-27: In SetupSuite, don't ignore the error returned by
model.New; capture the returned error from
model.New(model.TypeBuildKitAttestation, ...) and fail fast when it is non-nil
(e.g., using suite.Require().NoError(err) or suite.T().Fatalf) before assigning
suite.accessory, so a constructor/registration regression surfaces immediately
instead of causing nil-interface panics later; update the SetupSuite code around
the model.New call to check err and handle it accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de7c1f62-173c-49d7-90bf-c51d1a450e6d
📒 Files selected for processing (17)
src/controller/artifact/abstractor.gosrc/controller/artifact/abstractor_test.gosrc/controller/artifact/controller.gosrc/controller/artifact/controller_test.gosrc/controller/artifact/manifest_abstractor.gosrc/controller/artifact/manifest_abstractor_index.gosrc/controller/artifact/manifest_abstractor_v1.gosrc/controller/artifact/manifest_abstractor_v2.gosrc/controller/artifact/model.gosrc/core/main.gosrc/jobservice/main.gosrc/pkg/accessory/model/accessory.gosrc/pkg/accessory/model/attestation/attestation.gosrc/pkg/accessory/model/attestation/attestation_test.gosrc/pkg/artifact/model.gosrc/portal/src/app/base/project/repository/artifact/artifact.tssrc/server/v2.0/handler/artifact.go
Comment on lines
+96
to
+110
| func (m *indexManifestAbstractor) toBuildKitAttestationCandidate(ctx context.Context, repository string, descriptor v1.Descriptor) (*pkgartifact.AccessoryCandidate, error) { | ||
| if descriptor.MediaType != v1.MediaTypeImageManifest && descriptor.MediaType != schema2.MediaTypeManifest { | ||
| return nil, nil | ||
| } | ||
| if descriptor.Annotations[buildKitReferenceTypeAnnotation] != buildKitAttestationManifestType { | ||
| return nil, nil | ||
| } | ||
|
|
||
| targetDigest := descriptor.Annotations[buildKitReferenceDigestAnnotation] | ||
| if targetDigest == "" { | ||
| return nil, nil | ||
| } | ||
|
|
||
| if !m.isBuildKitAttestationManifest(repository, descriptor.Digest.String()) { | ||
| return nil, nil |
There was a problem hiding this comment.
Don't silently downgrade attestation descriptors on probe failures.
isBuildKitAttestationManifest currently returns false for pull/payload/JSON errors, and the caller then falls back to the normal child-reference path. A transient registry failure will therefore reclassify the attestation as a regular child instead of surfacing the failure, which makes this fix nondeterministic.
💡 Suggested change
- if !m.isBuildKitAttestationManifest(repository, descriptor.Digest.String()) {
- return nil, nil
- }
+ isAttestation, err := m.isBuildKitAttestationManifest(repository, descriptor.Digest.String())
+ if err != nil {
+ return nil, err
+ }
+ if !isAttestation {
+ return nil, nil
+ }
@@
-func (m *indexManifestAbstractor) isBuildKitAttestationManifest(repository, digest string) bool {
+func (m *indexManifestAbstractor) isBuildKitAttestationManifest(repository, digest string) (bool, error) {
manifest, _, err := m.regCli.PullManifest(repository, digest)
if err != nil {
- return false
+ return false, err
}
mediaType, content, err := manifest.Payload()
if err != nil {
- return false
+ return false, err
}
if mediaType != v1.MediaTypeImageManifest && mediaType != schema2.MediaTypeManifest {
- return false
+ return false, nil
}
imageManifest := &v1.Manifest{}
if err := json.Unmarshal(content, imageManifest); err != nil {
- return false
+ return false, err
}
for _, layer := range imageManifest.Layers {
if layer.MediaType == inTotoLayerMediaType {
- return true
+ return true, nil
}
}
- return false
+ return false, nil
}Also applies to: 133-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/controller/artifact/manifest_abstractor_index.go` around lines 96 - 110,
The code silently treats transient probe/pull/JSON errors from
isBuildKitAttestationManifest as "false" and falls back to normal
child-reference handling; change isBuildKitAttestationManifest to surface errors
(e.g., return (bool, error) or add a variant that returns an error) and update
callers (including toBuildKitAttestationCandidate and the other call site around
lines 133-159) to check the error and return it (instead of returning nil,nil
and downgrading the descriptor) so probe failures are propagated rather than
reclassifying attestations.
Comment on lines
+391
to
+410
| // If no direct signature found, check for Cosign signatures inherited from a parent OCI index. | ||
| if len(accs) == 0 { | ||
| // Use strings.Contains because the UI sends a list of types (e.g. type={signature.cosign ...}) | ||
| if strings.Contains(lib.StringValue(params.Q), "signature.cosign") { | ||
| artWithAccs, err := a.artCtl.Get(ctx, art.ID, &artifact.Option{WithAccessory: true}) | ||
| if err != nil { | ||
| log.Warningf("failed to get artifact %d with accessories for inheritance check: %v", art.ID, err) | ||
| } else if artWithAccs != nil { | ||
| // Combine direct and inherited accessories | ||
| allAccs := append(artWithAccs.Accessories, artWithAccs.InheritedAccessories...) | ||
| for _, acc := range allAccs { | ||
| if acc.GetData().Type == accessorymodel.TypeCosignSignature { | ||
| accs = append(accs, acc) | ||
| total = 1 | ||
| break | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Don't gate inherited Cosign lookup on len(accs) == 0.
The controller only suppresses inherited Cosign when a direct Cosign exists, not when any direct accessory exists. With a multi-type query like type={attestation.buildkit,signature.cosign}, direct BuildKit accessories make accs non-empty, so the inherited Cosign never gets returned. Because the inherited entry is appended after Count/List, this path also bypasses the final result's count/paging semantics. Please key this off “has direct Cosign already” and merge the inherited item before finalizing the paged response.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/v2.0/handler/artifact.go` around lines 391 - 410, The current
logic gates inherited Cosign lookup on len(accs) == 0 which incorrectly prevents
adding an inherited Cosign when other direct accessories exist; change the
condition to check whether a direct Cosign is already present (e.g., scan accs
for any acc.GetData().Type == accessorymodel.TypeCosignSignature) and only skip
inheritance if a direct Cosign exists; if no direct Cosign is found, call
a.artCtl.Get(ctx, art.ID, &artifact.Option{WithAccessory: true}) to retrieve
artWithAccs, scan artWithAccs.Accessories and artWithAccs.InheritedAccessories
for TypeCosignSignature, append any inherited Cosign to accs, and update total
appropriately (and ensure this merge happens before final pagination/counting so
the inherited item is included in the paged response).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
attestation.buildkitaccessories instead ofunknown/unknownindex childrenRelated Issues
Type of Change
fix:)feat:)feat!:/fix!:)docs:)refactor:)ci:/build:)chore:)test:)Release Notes
unknown/unknownchild rows.Testing
Checklist
git commit -s)Summary by CodeRabbit
Release Notes
New Features
Tests