cli/internal/plugin/builtin/oidc/oidc_plugin_test.go (1)

43-44: Consider using constants for default values in test assertions.

The test hardcodes the expected defaults ("https://oauth2.sigstore.dev/auth", "sigstore") rather than referencing oidcflow.DefaultIssuer and oidcflow.DefaultClientID. If the defaults change in the future, this test would silently pass with stale values.

♻️ Proposed fix

+import "ocm.software/open-component-model/cli/internal/oidcflow"
+
 func Test_OIDCPlugin_GetConsumerIdentity_Defaults(t *testing.T) {
 	// ...
-	r.Equal("https://oauth2.sigstore.dev/auth", id[configKeyIssuer])
-	r.Equal("sigstore", id[configKeyClientID])
+	r.Equal(oidcflow.DefaultIssuer, id[configKeyIssuer])
+	r.Equal(oidcflow.DefaultClientID, id[configKeyClientID])
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/plugin/builtin/oidc/oidc_plugin_test.go` around lines 43 - 44,
The test assertions use hardcoded strings for issuer and client ID; change them
to reference the package constants oidcflow.DefaultIssuer and
oidcflow.DefaultClientID (instead of the literal
"https://oauth2.sigstore.dev/auth" and "sigstore") in the assertions that check
id[configKeyIssuer] and id[configKeyClientID] so the test stays correct if
defaults change.

cli/internal/oidcflow/oidcflow_test.go (1)

28-43: Test validates PKCE struct but not the newPKCE constructor.

This test manually constructs a pkce struct and validates its methods, but doesn't test newPKCE() which includes the provider claims check and verifier generation. Consider adding a test that exercises newPKCE with a mock provider if S256 support validation is critical.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/oidcflow/oidcflow_test.go` around lines 28 - 43, Add a test that
calls newPKCE rather than constructing pkce directly: create a mock OIDC
provider (or stub its provider.Claims) that advertises "S256" in the
"code_challenge_methods_supported" claim, call newPKCE(ctx, provider) and assert
it returns a non-nil *pkce with a generated verifier (non-empty and expected
length), verify the challenge equals
base64.RawURLEncoding.EncodeToString(sha256.Sum256(verifier)[:]) and that
authURLOpts() and tokenURLOpts() return the expected counts; also add a negative
case where the provider does not list "S256" and assert newPKCE returns an
error.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

cli/integration/Taskfile.yml (1)

20-20: Quote SIGSTORE_ENV_FILE in shell expressions.

Line 20 and Line 27 use an unquoted path. Quoting avoids word-splitting/globbing edge cases and makes the task more robust.

♻️ Proposed fix

-      - sh: '[ -f {{.SIGSTORE_ENV_FILE}} ]'
+      - sh: '[ -f "{{.SIGSTORE_ENV_FILE}}" ]'
@@
-        . {{.SIGSTORE_ENV_FILE}}
+        . "{{.SIGSTORE_ENV_FILE}}"

Also applies to: 27-27

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/integration/Taskfile.yml` at line 20, The shell checks using the
SIGSTORE_ENV_FILE template variable are unquoted which can break on paths with
spaces or glob characters; update the sh expressions that use SIGSTORE_ENV_FILE
(the two occurrences in Taskfile.yml) to wrap the expanded value in quotes so
the test uses "[ -f \"{{.SIGSTORE_ENV_FILE}}\" ]" (i.e., quote the template
variable in both the check at the top and the second occurrence on line 27) to
prevent word-splitting/globbing issues.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

cli/internal/oidcflow/oidcflow_test.go (1)

31-46: Auth URL test manually constructs parameters instead of using the same production code path

The test at line 245 manually assembles URL query parameters, whereas the production code at oidcflow.go:128-132 calls oauth2.Config.AuthCodeURL() with options from p.authURLOpts(). This means the test can pass even if the real auth URL construction path changes incorrectly. Refactor the test to use the same code path: call cfg.AuthCodeURL(state, append(p.authURLOpts(), oauth2.AccessTypeOnline, oidc.Nonce("test-nonce"))...) instead of manually building the query parameters.

♻️ Suggested refactor

 import (
 	"context"
 	"crypto/sha256"
 	"encoding/base64"
 	"errors"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"testing"
 
 	"github.com/coreos/go-oidc/v3/oidc"
+	"golang.org/x/oauth2"
 	"github.com/stretchr/testify/require"
 )
@@
 func TestAuthURL_ContainsPKCEAndState(t *testing.T) {
 	t.Parallel()
 	r := require.New(t)
 
-	challenge := "test-challenge"
-	state := "random-state-value"
-	redirectURL := "http://127.0.0.1:12345/auth/callback"
-
-	params := url.Values{
-		"client_id":             {"sigstore"},
-		"redirect_uri":         {redirectURL},
-		"response_type":        {"code"},
-		"scope":                {"openid email"},
-		"state":                {state},
-		"code_challenge_method": {"S256"},
-		"code_challenge":       {challenge},
-		"access_type":          {"online"},
-		"nonce":                {"test-nonce"},
-	}
-	authURL := "https://issuer.example.com/auth?" + params.Encode()
+	state := "random-state-value"
+	redirectURL := "http://127.0.0.1:12345/auth/callback"
+	p := &pkce{challenge: "test-challenge", verifier: "test-verifier"}
+	cfg := oauth2.Config{
+		ClientID:    "sigstore",
+		RedirectURL: redirectURL,
+		Scopes:      []string{oidc.ScopeOpenID, "email"},
+		Endpoint: oauth2.Endpoint{
+			AuthURL: "https://issuer.example.com/auth",
+		},
+	}
+	authURL := cfg.AuthCodeURL(
+		state,
+		append(p.authURLOpts(), oauth2.AccessTypeOnline, oidc.Nonce("test-nonce"))...,
+	)
 
 	parsed, err := url.Parse(authURL)
 	r.NoError(err)
@@
-	r.Equal(challenge, q.Get("code_challenge"))
+	r.Equal(p.challenge, q.Get("code_challenge"))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/internal/oidcflow/oidcflow_test.go` around lines 31 - 46, The
TestPKCEChallenge currently builds auth URL params manually; change it to
exercise the real production path by creating an oauth2.Config and calling
cfg.AuthCodeURL(state, append(p.authURLOpts(), oauth2.AccessTypeOnline,
oidc.Nonce("test-nonce"))...), then parse the returned URL and assert the
expected PKCE query parameters (code_challenge, code_challenge_method, etc.) and
that tokenURLOpts still returns the expected token options via p.tokenURLOpts();
reference TestPKCEChallenge, pkce.authURLOpts, oauth2.Config.AuthCodeURL and
p.tokenURLOpts when making the change.

cli/cmd/setup/setup.go (1)

117-124: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Add a typed.GetType() == nil guard before dereference.

At Line 121, typed.GetType().GetName() can panic when typed is non-nil but missing type metadata.

Suggested fix

 			func(_ context.Context, typed runtime.Typed) (credentials.CredentialPlugin, error) {
-				if typed == nil {
+				if typed == nil || typed.GetType() == nil {
 					return nil, fmt.Errorf("no credential plugin found: missing type metadata")
 				}
-				if typed.GetType().GetName() == oidc.OIDCPluginType {
+				if typed.GetType().GetName() == oidc.OIDCPluginType {
 					return &oidc.OIDCPlugin{}, nil
 				}
 				return nil, fmt.Errorf("no credential plugin found for type %s", typed.GetType())
 			},

Verify API contract and nilability with this read-only check (expected: either explicit non-nil guarantee, or evidence GetType() may be nil, confirming this guard is required):

#!/bin/bash
set -euo pipefail

echo "== runtime.Typed contract and GetType() declarations =="
rg -n -C3 --type=go 'type\s+Typed\s+interface|GetType\(\)' bindings/go/runtime

echo
echo "== Call sites that dereference GetType() in setup path =="
rg -n -C2 --type=go 'typed\.GetType\(\)\.GetName\(\)|typed\.GetType\(\)' cli/cmd/setup/setup.go

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cli/cmd/setup/setup.go` around lines 117 - 124, The anonymous credential
plugin factory currently dereferences typed.GetType() without checking it; add a
nil guard for typed.GetType() before calling GetName() in the func(_
context.Context, typed runtime.Typed) (credentials.CredentialPlugin, error)
block so it returns a clear error when type metadata is missing (e.g., check if
typed.GetType() == nil and return fmt.Errorf("no credential plugin found:
missing type metadata") or similar), then only call typed.GetType().GetName() to
compare against oidc.OIDCPluginType and construct the existing error path that
includes the type when present.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes