web/src/systems/settings/stores/mcp-authorize-store.ts-244-248 (1)

244-248: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Invalid redirect URL is a silent no-op.

When an operator pastes something that isn't an absolute MCP redirect URL, the reducer returns without transitioning and without recording an error, so the dialog appears frozen. Consider transitioning to the completion-failure state (or having the caller validate and surface a message) so the error state is explicit.

As per coding guidelines, "Handle loading, error, and empty states explicitly".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/src/systems/settings/stores/mcp-authorize-store.ts` around lines 244 -
248, The manualRedirectSubmitted handler silently returns for invalid redirect
URLs, leaving the dialog in its current phase. Update this handler to explicitly
transition to the completion-failure/error state and record the invalid URL
error, while preserving the existing exchange flow for valid absolute MCP
redirect URLs.

Source: Coding guidelines

web/src/systems/settings/stores/mcp-authorize-store.ts-191-214 (1)

191-214: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

authorizeRequested forwards un-normalized approved scopes.

scopeEscalationRequested stores normalizeMCPAuthScopes(event.approvedScopes) (Line 160), but here event.scopeApproval is validated through hasExplicitScopeApproval — which normalizes only for the length check — and then stored and sent to begin as-is. The two entry points therefore put differently-shaped scope lists on the wire.

🐛 Normalize once before storing and forwarding

       authorizeRequested: (context, event, enqueue) => {
         if (context.phase !== "idle") return;
         if (event.scopeApproval && !hasExplicitScopeApproval(event.scopeApproval)) return;
+        const scopeApproval = event.scopeApproval
+          ? {
+              approveScopeEscalation: event.scopeApproval.approveScopeEscalation,
+              approvedScopes: normalizeMCPAuthScopes(event.scopeApproval.approvedScopes),
+            }
+          : undefined;
         const attemptId = context.attemptId + 1;
         enqueueBegin(
           event.begin,
           enqueue,
           attemptId,
           event.server,
           event.filter,
           event.mode,
-          event.scopeApproval
+          scopeApproval
         );
         return {
           phase: "beginning",
           attemptId,
           dismissOnConfirmation: event.dismissOnConfirmation,
           server: event.server,
           filter: event.filter,
           prior: event.prior,
           mode: event.mode,
-          scopeApproval: event.scopeApproval,
+          scopeApproval,
         };
       },

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/src/systems/settings/stores/mcp-authorize-store.ts` around lines 191 -
214, Update authorizeRequested to normalize event.scopeApproval once after
validation, then use the normalized value for both enqueueBegin and the returned
state’s scopeApproval field. Preserve undefined when no scope approval is
provided and keep the existing idle and validation guards unchanged.

internal/marketplace/source_test.go-290-294 (1)

290-294: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Verify the diagnostic for an older manifest.

A manifest_version: 1 feed is asserted to report client too old, which inverts the actual situation (the feed is older than the client). If UnsupportedManifestVersionError emits one message for both directions, operators reading logs for a stale feed will be told to upgrade the client.

#!/bin/bash
# Inspect the unsupported-version error construction and its wording.
rg -nP --type=go -C8 'UnsupportedManifestVersionError' internal/marketplace

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/marketplace/source_test.go` around lines 290 - 294, Correct the
expected diagnostic in the “Should reject a v1 manifest” test to describe an
older feed rather than an outdated client, and inspect
UnsupportedManifestVersionError to ensure its wording distinguishes manifest
versions older than the client from versions newer than the client.

internal/api/core/settings_mcp_install.go-62-62 (1)

62-62: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Confirm the settings service resolves an omitted scope.

scope became optional and the required-scope check was dropped, so MCPCatalogInstallRequest.Scope can now be "" while workspace_id may still be present. Unless InstallMCPCatalog defaults and re-validates the scope/workspace pairing, an install can land in an unclassified scope — the changed tests always send an explicit scope, so this path is uncovered.

#!/bin/bash
# Verify scope defaulting/validation for catalog installs.
rg -nP --type=go -C10 'func .*InstallMCPCatalog\(' internal/settings internal/daemon
rg -nP --type=go -C4 'Scope\s*==\s*""' internal/settings

As per coding guidelines, "workspace data must be classified by scope and checked for cross-workspace leakage".

Also applies to: 76-80, 97-103

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/api/core/settings_mcp_install.go` at line 62, Update
InstallMCPCatalog and its MCPCatalogInstallRequest handling to resolve an
omitted Scope before installation. Default the scope using the existing settings
behavior, then validate the scope/workspace_id pairing so workspace data is
classified and cross-workspace installs are rejected; preserve explicit-scope
behavior and apply the same handling to the related request construction and
validation paths.

internal/mcp/executor.go-156-190 (1)

156-190: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Authorization errors are masked once the call deadline elapses.

Lines 157-166 overwrite err with ctx.Err()/DeadlineExceeded before the scope/authorization checks, so a 401 or insufficient_scope that arrives as the deadline expires is reported as a timeout and ReasonMCPAuthRequired never reaches the caller — the re-authorize path is lost. Classify auth first, then fall back to context normalization.

🐛 Proposed fix

 func normalizeMCPErrorWithContext(ctx context.Context, id toolspkg.ToolID, err error, discovery bool) error {
+	var scopeErr *InsufficientScopeError
+	if errors.As(err, &scopeErr) {
+		return toolspkg.NewToolError(
+			toolspkg.ErrorCodeUnavailable,
+			id,
+			scopeErr.Error(),
+			scopeErr,
+			toolspkg.ReasonMCPAuthRequired,
+		)
+	}
+	if errors.Is(err, ErrAuthorizationRequired) {
+		return toolspkg.NewToolError(
+			toolspkg.ErrorCodeUnavailable,
+			id,
+			ErrAuthorizationRequired.Error(),
+			ErrAuthorizationRequired,
+			toolspkg.ReasonMCPAuthRequired,
+		)
+	}
 	if ctxErr := ctx.Err(); ctxErr != nil {
 		err = ctxErr
 	} else if deadline, hasDeadline := ctx.Deadline(); hasDeadline && !time.Now().Before(deadline) {
 		err = context.DeadlineExceeded
 	} else {
 		var networkError net.Error
 		if errors.As(err, &networkError) && networkError.Timeout() {
 			err = context.DeadlineExceeded
 		}
 	}
-	var scopeErr *InsufficientScopeError
-	if errors.As(err, &scopeErr) {
-		return toolspkg.NewToolError(
-			toolspkg.ErrorCodeUnavailable,
-			id,
-			scopeErr.Error(),
-			scopeErr,
-			toolspkg.ReasonMCPAuthRequired,
-		)
-	}
-	if errors.Is(err, ErrAuthorizationRequired) {
-		return toolspkg.NewToolError(
-			toolspkg.ErrorCodeUnavailable,
-			id,
-			ErrAuthorizationRequired.Error(),
-			ErrAuthorizationRequired,
-			toolspkg.ReasonMCPAuthRequired,
-		)
-	}
 	if discovery {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/mcp/executor.go` around lines 156 - 190, Update
normalizeMCPErrorWithContext to perform the InsufficientScopeError and
ErrAuthorizationRequired checks before overwriting err with context or timeout
errors, preserving the existing auth-specific ToolError responses; only apply
context/deadline normalization when the original error is not
authorization-related, then retain the existing discovery and general error
handling.

internal/mcp/executor.go-192-216 (1)

192-216: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Bound tools/list pagination and dereference safely.

ListToolsResult has no GetTTLMs() method here, so reading the TTL should go through the SDK’s actual cache/TTL field if required. Pagination also has no cap: keep adding pages until the context deadline if NextCursor never stabilizes or terminates, so enforce a page limit. A nil result guard could avoid a panic if the SDK/transport returns nil before an error.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/mcp/executor.go` around lines 192 - 216, The listMCPTools pagination
flow must use the SDK-supported cache/TTL field instead of calling the
unavailable result.GetTTLMs() method, guard against a nil result before
dereferencing Tools or NextCursor, and enforce a finite maximum page count while
preserving repeated-cursor detection and context/error propagation.

internal/config/provider_test.go-1283-1305 (1)

1283-1305: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Wrap the two validation cases in t.Run("Should ...") subtests.

This test asserts two distinct behaviors (valid pre-registered OAuth passes; missing ClientID fails) sequentially in one function, unlike the table-driven t.Run pattern used by the neighboring tests in this file.

As per path instructions, "MUST use t.Run("Should...") pattern for ALL test cases."

♻️ Suggested refactor

func TestMCPServerValidateSupportsRemotePreRegisteredOAuth(t *testing.T) {
	t.Parallel()

	server := MCPServer{
		Name:      "linear",
		Transport: MCPServerTransportHTTP,
		URL:       "https://mcp.example/mcp",
		Auth: MCPAuthConfig{
			Registration: MCPAuthRegistrationPreRegistered,
			IssuerURL:    "https://auth.example",
			ClientID:     "client-1",
			Scopes:       []string{"read", "write"},
		},
	}

	t.Run("Should validate a pre-registered remote OAuth config", func(t *testing.T) {
		t.Parallel()
		if err := server.Validate("mcp_servers[0]"); err != nil {
			t.Fatalf("Validate(remote OAuth) error = %v", err)
		}
	})

	t.Run("Should reject a missing pre-registered client ID", func(t *testing.T) {
		t.Parallel()
		invalid := server
		invalid.Auth.ClientID = ""
		if err := invalid.Validate("mcp_servers[0]"); err == nil {
			t.Fatal("Validate(missing pre-registered client ID) error = nil, want validation failure")
		}
	})
}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/config/provider_test.go` around lines 1283 - 1305, Refactor
TestMCPServerValidateSupportsRemotePreRegisteredOAuth to place each validation
case in its own t.Run subtest using the “Should ...” naming pattern: one subtest
for accepting valid pre-registered remote OAuth and one for rejecting a missing
ClientID. Keep the cases independent by validating a copied server with the
empty ClientID, and preserve parallel execution where appropriate.

Source: Path instructions

internal/mcp/auth/fingerprint.go-113-121 (1)

113-121: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Trim both sides of the resource URL comparison.

The left side is trimmed but firstNonEmpty(cfg.ResourceURL, cfg.RemoteURL) is compared raw. If the config value carries surrounding whitespace, the binding check fails and forces an unnecessary re-authorization. Please confirm firstNonEmpty trims; otherwise wrap the right side in strings.TrimSpace.

#!/bin/bash
ast-grep run --pattern 'func firstNonEmpty($$$) $_ { $$$ }' --lang go internal/mcp/auth

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/mcp/auth/fingerprint.go` around lines 113 - 121, Update the resource
URL comparison in the registration/token binding logic to trim the result of
firstNonEmpty(cfg.ResourceURL, cfg.RemoteURL) with strings.TrimSpace, unless
firstNonEmpty already guarantees trimming. Preserve the existing fallback
selection and other comparison checks.

internal/mcp/auth/sdk_adapter.go-199-211 (1)

199-211: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Well-known path candidate retains the resource URL's query and fragment.

atEndpoint copies the resource URL wholesale, so https://host/mcp?tenant=a#x yields /.well-known/oauth-protected-resource/mcp?tenant=a#x. The root candidate already clears both; do the same here to build a conformant URL and avoid forwarding resource query params to the metadata endpoint.

🐛 Proposed fix

 	atEndpoint := *resource
 	atEndpoint.Path = "/.well-known/oauth-protected-resource/" + strings.TrimLeft(
 		resource.Path,
 		"/",
 	)
+	atEndpoint.RawQuery = ""
+	atEndpoint.Fragment = ""

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/mcp/auth/sdk_adapter.go` around lines 199 - 211, Update the
atEndpoint construction in the resource URL candidate logic to clear the copied
query and fragment fields before appending its metadata URL. Preserve the
existing host, scheme, and transformed path behavior while ensuring the
well-known candidate never forwards resource query parameters or fragments.

web/src/systems/marketplace/components/marketplace-detail-extension-actions.tsx-185-185 (1)

185-185: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

formatMarketplaceVersion's empty-input contract is unspecified, and one caller assumes undefined. The consent dialog falls back with ??, which only triggers for null/undefined, while the sibling branch at Line 87 of the same file and marketplace-card.tsx treat the result as possibly empty-string. The new test table never pins the empty case, so neither assumption is enforced.

• web/src/systems/marketplace/components/marketplace-detail-extension-actions.tsx#L185-L185: switch the fallback to || so an empty formatted version still renders "the newer release".
• web/src/systems/marketplace/components/__tests__/marketplace-ui.test.ts#L42-L49: add ["", ""] (and a whitespace-only input) to the table so the empty-input return value becomes a pinned contract.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@web/src/systems/marketplace/components/marketplace-detail-extension-actions.tsx`
at line 185, Update marketplace-detail-extension-actions.tsx at lines 185-185 to
use a truthiness fallback for formatMarketplaceVersion so empty results render
“the newer release”; in marketplace-ui.test.ts lines 42-49, add table cases for
empty and whitespace-only inputs, pinning the formatter’s empty-input contract
to return an empty string.

internal/mcp/hosted_proxy_test.go-151-155 (1)

151-155: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Double session.Close() can fail the test spuriously.

The session is closed explicitly at Line 205 and again by the t.Cleanup registered at Line 151, which calls t.Errorf on any error. Rely on the cleanup only (or make the cleanup tolerant of an already-closed session). Same for the provider-protocol test: cancel() at Line 324 is not deferred, so an early t.Fatalf leaks the context.

🧪 Proposed change

 	cancel()
-	if err := session.Close(); err != nil {
-		t.Fatalf("session.Close() error = %v", err)
-	}
 	select {

Also applies to: 204-207

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/mcp/hosted_proxy_test.go` around lines 151 - 155, Remove the
explicit session.Close call in the hosted-proxy test so the t.Cleanup callback
is the sole owner of session cleanup and cannot report a duplicate-close error.
Apply the same cleanup ownership to the provider-protocol test, ensuring
cancel() is deferred immediately after context creation so early t.Fatalf paths
still release the context.

internal/mcp/serve_test.go-68-76 (1)

68-76: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assertion no longer fails on a missing schema.

json.Marshal(nil) returns null, which passes both len(schema) == 0 and json.Valid, so the "schema-backed tool" invariant can no longer fail. Assert presence explicitly.

💚 Proposed fix

 			schema, err := json.Marshal(tool.InputSchema)
 			if err != nil {
 				t.Fatalf("json.Marshal(tool %q input schema) error = %v", tool.Name, err)
 			}
-			if len(schema) == 0 || !json.Valid(schema) {
+			if len(schema) == 0 || string(schema) == "null" || !json.Valid(schema) {
 				t.Fatalf("tool %q schema = %s, want valid JSON", tool.Name, schema)
 			}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/mcp/serve_test.go` around lines 68 - 76, Update the schema
validation loop in the tool test to explicitly reject a missing or nil
tool.InputSchema before marshaling it. Preserve the existing JSON marshaling and
validity checks for present schemas so schema-backed tools must provide valid,
non-null JSON.

internal/settings/mcp_catalog_input.go-185-191 (1)

185-191: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Secret inputs ignore input.Binding.Type.

Both branches bind unconditionally to env (setMCPSecretEnvRef / secrets.SecretEnv). A catalog entry declaring a secret with a url_query (or any non-env) binding is accepted and silently mis-bound, unlike the plain-input path which rejects unsupported binding types. Reject non-env bindings here (or explicitly support them).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/settings/mcp_catalog_input.go` around lines 185 - 191, Update the
secret-input handling around setMCPSecretEnvRef and secrets.SecretEnv so it
validates input.Binding.Type before binding; reject any type other than env,
matching the plain-input path’s unsupported-binding behavior, while preserving
both existing env-reference and literal-secret handling for valid env bindings.

internal/store/globaldb/global_db_mcp_auth_atomic_test.go-136-138 (1)

136-138: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Error-path assertions in the globaldb MCP auth suite only check non-nil. All four sites force a specific failure (SQLite trigger or record validation) but assert only err == nil, so an unrelated error — validation rejecting the record before the trigger fires, a target mismatch, a closed handle — still passes and hides the rollback/validation regression under test. global_db_test.go already asserts the forced message; apply the same discipline here.

• internal/store/globaldb/global_db_mcp_auth_atomic_test.go#L136-L138: assert the error contains forced MCP registration update failure.
• internal/store/globaldb/global_db_mcp_auth_atomic_test.go#L194-L196: assert the error contains forced MCP registration secret cleanup failure.
• internal/store/globaldb/global_db_mcp_auth_atomic_test.go#L235-L237: assert the error contains forced authorization cleanup failure.
• internal/store/globaldb/global_db_mcp_auth_test.go#L417-L432: assert the validation error contains access token and client URI must be set together.

As per path instructions, tests MUST have specific error assertions (ErrorContains, ErrorAs) and must avoid "weak assertions like assert.Error(t, err) without message validation".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/store/globaldb/global_db_mcp_auth_atomic_test.go` around lines 136 -
138, Strengthen the error assertions in
internal/store/globaldb/global_db_mcp_auth_atomic_test.go at lines 136-138,
194-196, and 235-237 by requiring errors containing “forced MCP registration
update failure”, “forced MCP registration secret cleanup failure”, and “forced
authorization cleanup failure” respectively; update
internal/store/globaldb/global_db_mcp_auth_test.go lines 417-432 to require the
validation error containing “access token and client URI must be set together”.
Use specific message validation such as ErrorContains rather than non-nil-only
assertions, preserving the existing test scenarios around
SaveMCPAuthRegistration and authorization cleanup.

Source: Path instructions

internal/testutil/mcpfixture/fixture_test.go-185-204 (1)

185-204: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Strengthen error assertions in TestFixtureValidation.

All three checks only assert err != nil, never validating the error message/content. This can't detect whether the correct validation branch fired (e.g., "unsupported profile" vs. a completely different failure) if the underlying logic regresses.

As per path instructions, tests "MUST have specific error assertions (ErrorContains, ErrorAs)" and must avoid "Weak assertions like assert.Error(t, err) without message validation."

🧪 Proposed fix using message-specific assertions

-		if _, err := New(Profile("unsupported")); err == nil {
-			t.Fatal("New(unsupported profile) error = nil, want validation error")
+		if _, err := New(Profile("unsupported")); err == nil || !strings.Contains(err.Error(), "profile") {
+			t.Fatalf("New(unsupported profile) error = %v, want validation error mentioning profile", err)
 		}
 		fixture := MustNew(ProfileModern2026)
-		if err := fixture.RunStdio(t.Context(), nil, nil); err == nil {
-			t.Fatal("RunStdio(nil input) error = nil, want validation error")
+		if err := fixture.RunStdio(t.Context(), nil, nil); err == nil || !strings.Contains(err.Error(), "input") {
+			t.Fatalf("RunStdio(nil input) error = %v, want validation error mentioning input", err)
 		}
 		input := io.NopCloser(strings.NewReader(""))
-		if err := fixture.RunStdio(t.Context(), input, nil); err == nil {
-			t.Fatal("RunStdio(nil output) error = nil, want validation error")
+		if err := fixture.RunStdio(t.Context(), input, nil); err == nil || !strings.Contains(err.Error(), "output") {
+			t.Fatalf("RunStdio(nil output) error = %v, want validation error mentioning output", err)
 		}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/testutil/mcpfixture/fixture_test.go` around lines 185 - 204,
Strengthen the three error checks in TestFixtureValidation by asserting each
error contains the expected validation message, not merely that an error exists.
Add message-specific assertions for New(Profile("unsupported")), RunStdio with
nil input, and RunStdio with nil output, while preserving the existing
close-error check.

Source: Path instructions

internal/testutil/mcpfixture/oauth_test.go-226-234 (1)

226-234: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the redirect status before trusting Location.

If /authorize rejects the request (e.g. missing resource match), Location is empty and url.Parse("") succeeds, so the assertion fails with a misleading iss mismatch instead of the real cause.

🐛 Proposed fix

 		response := performRequest(t, client, http.MethodGet, authorizeURL.String(), nil)
 		defer closeResponse(t, response)
+		if got, want := response.StatusCode, http.StatusFound; got != want {
+			t.Fatalf("mix-up authorize status = %d, want %d", got, want)
+		}
 		callbackURL, err := url.Parse(response.Header.Get("Location"))

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/testutil/mcpfixture/oauth_test.go` around lines 226 - 234, In the
authorization test around performRequest, assert that the response has the
expected redirect status before parsing or validating its Location header. Keep
the existing callbackURL parsing and issuer assertion afterward so rejected
/authorize responses fail with the actual status-related cause rather than an
empty URL mismatch.

internal/testutil/mcpfixture/oauth.go-326-338 (1)

326-338: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

continue leaves a nil map in the cloned slice.

On a marshal/unmarshal failure the pre-sized element stays nil, so Requests() silently returns a hole instead of the recorded registration, which makes any dependent assertion fail with a confusing nil dereference. Prefer appending only successful clones (or failing the clone loudly).

🐛 Proposed fix

 func cloneRegistrationSlice(values []map[string]any) []map[string]any {
-	cloned := make([]map[string]any, len(values))
-	for index, value := range values {
+	cloned := make([]map[string]any, 0, len(values))
+	for _, value := range values {
 		data, err := json.Marshal(value)
 		if err != nil {
 			continue
 		}
-		if err := json.Unmarshal(data, &cloned[index]); err != nil {
+		var entry map[string]any
+		if err := json.Unmarshal(data, &entry); err != nil {
 			continue
 		}
+		cloned = append(cloned, entry)
 	}
 	return cloned
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/testutil/mcpfixture/oauth.go` around lines 326 - 338, Update
cloneRegistrationSlice so marshal or unmarshal failures do not leave nil entries
in the returned slice; build the result by appending only successfully cloned
maps, or propagate the failure explicitly. Preserve the order of successful
registrations and ensure Requests() cannot receive holes from this helper.

packages/site/components/marketplace/marketplace-entry-card.tsx-79-86 (1)

79-86: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Missing separator between the launch pill and the input summary.

When inputs is present the row renders pill, inputs, separator, scope — the input summary has no separator preceding it, so it visually merges with the pill, while the same markup renders correctly when inputs is null.

🐛 Proposed fix

-      {inputs ? <span>{inputs}</span> : null}
+      {inputs ? (
+        <>
+          <MetaSeparator />
+          <span>{inputs}</span>
+        </>
+      ) : null}
       <MetaSeparator />

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/site/components/marketplace/marketplace-entry-card.tsx` around lines
79 - 86, Update the JSX around the launch type Pill and conditional inputs
summary in the marketplace entry card to render a MetaSeparator between them
when inputs is present. Preserve the existing separator before the default-scope
text and avoid adding an extra separator when inputs is absent.

packages/site/lib/marketplace-catalog.ts-319-325 (1)

319-325: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Message references a launch type that no longer exists.

launch.type is npm | uvx | docker | remote; "stdio" is not one of them, so the validation error is confusing for catalog authors.

🐛 Proposed fix

-          message: "env inputs are only allowed for stdio launch",
+          message: "env inputs are only allowed for local launch (npm, uvx, docker)",

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/site/lib/marketplace-catalog.ts` around lines 319 - 325, Update the
validation message in the env-binding check within the catalog validation flow
to reference the supported launch types accurately instead of “stdio”; preserve
the existing condition and issue path.

web/src/systems/settings/components/mcp-authorize-dialog.tsx-294-294 (1)

294-294: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Validate the trimmed value to match submission behavior.

submitManual accepts padded input (the hook trims before exchanging, per use-mcp-authorize.test.tsx line 243), but the enablement check validates the raw textarea value. A pasted URL with a leading/trailing newline would keep the button disabled with no explanation.

🛠️ Proposed fix

-        disabled={!isAbsoluteMCPRedirectURL(manualValue) || exchanging}
+        disabled={!isAbsoluteMCPRedirectURL(manualValue.trim()) || exchanging}

Please confirm whether isAbsoluteMCPRedirectURL already trims internally; if it does, this is a no-op.

#!/bin/bash
fd -t f 'mcp-authorize-model.ts' web/src --exec cat -n

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/src/systems/settings/components/mcp-authorize-dialog.tsx` at line 294,
Update the manual submission button’s disabled condition near submitManual to
validate the same trimmed manualValue that submission uses, while preserving the
existing exchanging check. First verify whether isAbsoluteMCPRedirectURL trims
internally; if it does, leave this condition unchanged, otherwise pass the
trimmed value for padded URLs to enable correctly.

web/src/systems/settings/stores/mcp-authorize-store.ts (2)

158-176: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Same silent-drop pattern in scopeEscalationRequested.

Shares the same root cause as the authorizeRequested handler above (line 165): an invalid scopeApproval (empty normalized scopes) causes a silent no-op instead of an explicit failure state.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/src/systems/settings/stores/mcp-authorize-store.ts` around lines 158 -
176, The scopeEscalationRequested handler currently drops invalid normalized
approvals silently. Replace the hasExplicitScopeApproval early return with the
same explicit failure-state transition used by authorizeRequested, while
preserving the existing reviewing_scopes transition for valid approvals and the
idle-phase guard.

---

191-220: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Silent no-op when scopeApproval fails hasExplicitScopeApproval.

If event.scopeApproval is provided but invalid (e.g. approveScopeEscalation: false, or all scopes normalize to empty), authorizeRequested returns without any state transition (line 199) — the UI stays in "idle" with no error, unlike the newly added manualRedirectSubmitted handling which surfaces an explicit failed state for an analogous invalid-input case. Worth confirming whether use-mcp-authorize.ts can ever dispatch such a malformed event; if so, consider surfacing a failed transition here too for consistency and debuggability.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@web/src/systems/settings/stores/mcp-authorize-store.ts` around lines 191 -
220, Update authorizeRequested to handle invalid provided scopeApproval instead
of silently returning when hasExplicitScopeApproval fails. Transition to the
existing failed state with an appropriate error, matching the
manualRedirectSubmitted invalid-input handling, while preserving the current
beginning transition for valid approvals and the idle behavior when no
scopeApproval is provided.

internal/settings/mcp_catalog_input.go (1)

155-203: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Missing validation for empty input.Binding.Name in secret-input binding.

bindingName (line 172) is used directly as an env key for setMCPSecretEnvRef (195) and secrets.SecretEnv (201) without checking it's non-empty. A malformed catalog manifest with a blank binding name would silently create an invalid empty-key environment binding, the same class of manifest-defect issue already fixed for url_query on the URL side.

🛡️ Proposed guard

 	bindingName := strings.TrimSpace(input.Binding.Name)
+	if bindingName == "" {
+		return validationError(fmt.Errorf(
+			"settings: catalog secret input %q has an empty binding name",
+			id,
+		))
+	}
 	if !supplied {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/settings/mcp_catalog_input.go` around lines 155 - 203, Validate the
trimmed bindingName in applyCatalogSecretInput before using it as an environment
key. Return the existing validationError pattern with a clear catalog
secret-input binding-name error when it is empty, while preserving the current
vault-reference and SecretEnv assignment flows for valid names.

internal/api/core/settings_mcp_auth.go (1)

126-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider validating the manual redirect URI, not just its presence.

Automatic mode parses the configured redirect URI and enforces scheme/host/loopback, while manual mode returns the raw trimmed value. A malformed configured value (e.g. missing scheme) would flow into the authorization request and fail later with an opaque provider error. Reusing the same parse/validation keeps both modes consistent.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/api/core/settings_mcp_auth.go` around lines 126 - 130, The manual
redirect URI path in the MCP OAuth settings validation must validate the trimmed
value using the same parsing and scheme/host/loopback checks as automatic mode,
rather than returning it immediately. Reuse the existing redirect URI validation
logic and return its validation error before returning the URI.

internal/mcp/auth/manager.go (1)

446-465: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Extract the duplicated revision check.

The lock/check/unlock block appears twice verbatim; a small func (m *Manager) revisionMatches(targetKey string, revision uint64) bool helper would remove the duplication and the manual unlock paths. The final persist relies on the generation fence rather than the released lock, which is consistent with the mutation-race tests.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/mcp/auth/manager.go` around lines 446 - 465, The revision validation
in the surrounding Manager method is duplicated before and after token matching.
Add a Manager.revisionMatches helper that locks internally, compares the target
revision, and returns the result, then replace both manual lock/check/unlock
blocks with this helper while preserving ErrLoginSessionStale handling and the
existing generation-fenced persistReplacementTokenAtGeneration call.

internal/testutil/mcpfixture/oauth.go (1)

326-340: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Silently dropping un-clonable entries can distort snapshot counts.

Tests assert on len(fixture.Requests().Registration); a skipped entry would make the snapshot shorter than what the server actually recorded, turning a fixture bug into a confusing count mismatch. Since these maps come from json.Unmarshal, a failure here is effectively impossible — so preserving a shallow copy (or panicking) is more honest than continue.

♻️ Suggested handling

 func cloneRegistrationSlice(values []map[string]any) []map[string]any {
 	cloned := make([]map[string]any, 0, len(values))
 	for _, value := range values {
 		data, err := json.Marshal(value)
 		if err != nil {
-			continue
+			cloned = append(cloned, value)
+			continue
 		}
 		var entry map[string]any
 		if err := json.Unmarshal(data, &entry); err != nil {
-			continue
+			cloned = append(cloned, value)
+			continue
 		}
 		cloned = append(cloned, entry)
 	}
 	return cloned
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/testutil/mcpfixture/oauth.go` around lines 326 - 340, Update
cloneRegistrationSlice so entries that cannot be JSON-cloned are not silently
omitted, preserving the original slice length and snapshot counts. Replace the
continue-based failure handling in the function with an honest fallback such as
preserving a shallow copy or panicking, while retaining the existing deep-clone
behavior for successful entries.

internal/mcp/executor_test.go (1)

881-921: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Prefer an atomic counter for calls.

calls++ runs on the server goroutine while line 919 reads it from the test goroutine; ordering relies on the transport's happens-before edges rather than explicit synchronization. internal/mcp/auth/sdk_adapter_test.go already uses atomic.Int32 for the same pattern — matching it removes any -race ambiguity for free.

♻️ Suggested change

-		calls := 0
+		var calls atomic.Int32
 		server.AddReceivingMiddleware(func(next mcpsdk.MethodHandler) mcpsdk.MethodHandler {
 			return func(ctx context.Context, method string, request mcpsdk.Request) (mcpsdk.Result, error) {
 				if method == "tools/list" {
-					calls++
-					return &mcpsdk.ListToolsResult{NextCursor: fmt.Sprintf("page-%d", calls)}, nil
+					return &mcpsdk.ListToolsResult{
+						NextCursor: fmt.Sprintf("page-%d", calls.Add(1)),
+					}, nil
 				}
 				return next(ctx, method, request)
 			}
 		})

-		if got, want := calls, maxMCPToolListPages; got != want {
+		if got, want := int(calls.Load()), maxMCPToolListPages; got != want {
 			t.Fatalf("tools/list calls = %d, want %d", got, want)
 		}

As per coding guidelines, "Go tests must use Should … subtest names, default to t.Parallel, ... run with race detection".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/mcp/executor_test.go` around lines 881 - 921, Replace the
unsynchronized calls counter in the paginated tools test with an atomic counter,
following the atomic.Int32 pattern used in
internal/mcp/auth/sdk_adapter_test.go. Update the increment in the tools/list
middleware and the assertion comparing the count with maxMCPToolListPages to use
atomic operations, preserving the existing pagination behavior.

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

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