feat: providers model - #118

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

43-43: ⚡ Quick win

Use t.Run("Should ...") naming for the new subtest.

Line 43 should follow the repo’s test-case naming convention for consistency.

Proposed tweak

-	t.Run("validate create session runtime overrides", func(t *testing.T) {
+	t.Run("Should validate create session runtime overrides", func(t *testing.T) {

As per coding guidelines: "Use t.Run('Should ...') pattern for Go test subtests instead of flat test structures".

🤖 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/session_workspace_internal_test.go` at line 43, Update the
subtest name to follow the repo convention by changing the t.Run invocation
string in the subtest for session workspace validation; locate the t.Run call
(the subtest inside session_workspace_internal_test.go) currently using
"validate create session runtime overrides" and prefix it with "Should " so the
test reads "Should validate create session runtime overrides" to conform with
the t.Run("Should ...") pattern.

internal/acp/types_test.go (1)

47-75: ⚡ Quick win

TestAgentProcessCapsSnapshotClonesConfigOptions should use t.Run("Should ...") subtests.

The test covers two distinct behaviors — snapshot isolation from mutation, and setConfigOptions update correctness — but they are merged into a flat function body. Per the coding guidelines, each scenario should be a named subtest with its own t.Parallel().

♻️ Proposed refactor

-func TestAgentProcessCapsSnapshotClonesConfigOptions(t *testing.T) {
-	t.Parallel()
-
-	proc := &AgentProcess{}
-	proc.setCaps(Caps{
-		ConfigOptions: []SessionConfigOption{
-			{
-				ID:      "model",
-				Kind:    SessionConfigOptionKindSelect,
-				Current: "model-a",
-				Values:  []SessionConfigOptionValue{{Value: "model-a"}},
-			},
-		},
-	})
-
-	first := proc.CapsSnapshot()
-	first.ConfigOptions[0].Current = "mutated"
-	first.ConfigOptions[0].Values[0].Value = "mutated"
-
-	second := proc.CapsSnapshot()
-	if second.ConfigOptions[0].Current != "model-a" || second.ConfigOptions[0].Values[0].Value != "model-a" {
-		t.Fatalf("CapsSnapshot() leaked mutable config options: %#v", second.ConfigOptions)
-	}
-	proc.setConfigOptions([]SessionConfigOption{{ID: "reasoning_effort", Kind: SessionConfigOptionKindSelect}})
-	updated := proc.CapsSnapshot()
-	if len(updated.ConfigOptions) != 1 || updated.ConfigOptions[0].ID != "reasoning_effort" {
-		t.Fatalf("setConfigOptions() = %#v", updated.ConfigOptions)
-	}
-}
+func TestAgentProcessCapsSnapshotClonesConfigOptions(t *testing.T) {
+	t.Parallel()
+
+	t.Run("Should deep-clone config options to prevent mutation leakage", func(t *testing.T) {
+		t.Parallel()
+
+		proc := &AgentProcess{}
+		proc.setCaps(Caps{
+			ConfigOptions: []SessionConfigOption{
+				{
+					ID:      "model",
+					Kind:    SessionConfigOptionKindSelect,
+					Current: "model-a",
+					Values:  []SessionConfigOptionValue{{Value: "model-a"}},
+				},
+			},
+		})
+
+		first := proc.CapsSnapshot()
+		first.ConfigOptions[0].Current = "mutated"
+		first.ConfigOptions[0].Values[0].Value = "mutated"
+
+		second := proc.CapsSnapshot()
+		if second.ConfigOptions[0].Current != "model-a" || second.ConfigOptions[0].Values[0].Value != "model-a" {
+			t.Fatalf("CapsSnapshot() leaked mutable config options: %#v", second.ConfigOptions)
+		}
+	})
+
+	t.Run("Should reflect updated config options after setConfigOptions", func(t *testing.T) {
+		t.Parallel()
+
+		proc := &AgentProcess{}
+		proc.setCaps(Caps{
+			ConfigOptions: []SessionConfigOption{
+				{ID: "model", Kind: SessionConfigOptionKindSelect, Current: "model-a"},
+			},
+		})
+		proc.setConfigOptions([]SessionConfigOption{{ID: "reasoning_effort", Kind: SessionConfigOptionKindSelect}})
+		updated := proc.CapsSnapshot()
+		if len(updated.ConfigOptions) != 1 || updated.ConfigOptions[0].ID != "reasoning_effort" {
+			t.Fatalf("setConfigOptions() = %#v", updated.ConfigOptions)
+		}
+	})
+}

As per coding guidelines: Use t.Run("Should ...") subtests with t.Parallel as default.

🤖 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/acp/types_test.go` around lines 47 - 75, Split
TestAgentProcessCapsSnapshotClonesConfigOptions into two named subtests using
t.Run and call t.Parallel inside each subtest: one subtest (e.g., "Should clone
config options to prevent mutation") should set up proc with setCaps, call
CapsSnapshot(), mutate the first snapshot and assert the second snapshot remains
unchanged; the other subtest (e.g., "Should update config options via
setConfigOptions") should call proc.setConfigOptions, then CapsSnapshot() and
assert the updated ConfigOptions contains the expected ID; keep the existing
assertions and use the same symbols
(TestAgentProcessCapsSnapshotClonesConfigOptions, proc.setCaps,
proc.CapsSnapshot, proc.setConfigOptions) so each scenario runs in isolation and
parallel.

internal/modelcatalog/redact.go (1)

8-17: 💤 Low value

Pattern coverage looks correct for its purpose; consider noting the github_pat_ gap.

Modern GitHub fine-grained PATs use the github_pat_ prefix (not covered by gh[pousr]_). For a best-effort catalog-error redactor this is acceptable, but worth a comment explaining the intended scope so future contributors know to extend the list as new token formats appear.

🤖 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/modelcatalog/redact.go` around lines 8 - 17, The secretPatterns list
currently omits modern GitHub fine-grained PATs (e.g., "github_pat_...") because
the gh[pousr]_* regex only matches legacy prefixes; update
internal/modelcatalog/redact.go by adding a clarifying comment above the
secretPatterns variable (and optionally next to the gh[pousr]_[A-Za-z0-9_]{8,}
entry) that documents the intended scope and known gaps (specifically noting
`github_pat_` tokens) so future contributors know to extend secretPatterns or
add a new regexp to cover fine-grained PATs; reference the secretPatterns
variable and the gh[pousr]_ regex when making this comment.

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

53-55: ⚡ Quick win

Make runtime-override errors always carry session.ErrInvalidRuntimeOverride

statusForSessionError now maps session.ErrInvalidRuntimeOverride to 400, but prefixedRuntimeOverrideErr does not guarantee that sentinel is attached. Wrapping it there makes status mapping robust and deterministic.

💡 Proposed fix

 func prefixedRuntimeOverrideErr(prefix string, err error) error {
+	if err == nil {
+		return nil
+	}
+	wrapped := fmt.Errorf("%w: %w", session.ErrInvalidRuntimeOverride, err)
 	label := strings.TrimSpace(prefix)
 	if label == "" {
-		return err
+		return wrapped
 	}
-	return fmt.Errorf("%s: %w", label, err)
+	return fmt.Errorf("%s: %w", label, wrapped)
 }

Also applies to: 156-157, 184-190

🤖 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/session_workspace.go` around lines 53 - 55, The
prefixedRuntimeOverrideErr call should always wrap the underlying validation
error with the sentinel session.ErrInvalidRuntimeOverride so
statusForSessionError can deterministically map it to 400; update each place
that returns prefixedRuntimeOverrideErr (e.g., where
session.ValidateReasoningEffort is used) to wrap the returned err with
session.ErrInvalidRuntimeOverride (using error wrapping like fmt.Errorf("%w:
%v", session.ErrInvalidRuntimeOverride, err) or errors.Join) before passing it
into prefixedRuntimeOverrideErr, and apply the same change to the other
prefixedRuntimeOverrideErr call sites in this file.

internal/api/spec/spec_test.go (1)

108-120: ⚡ Quick win

Assert error response schemas for declared non-2xx statuses

This case currently verifies that 403/503 responses exist, but not their JSON schema shape. Add schema assertions for those statuses (for example, required error envelope fields) so spec regressions are caught earlier.

As per coding guidelines "Always assert both HTTP status code AND response body in tests; status-code-only assertions are insufficient".

🤖 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/spec/spec_test.go` around lines 108 - 120, The tests only assert
403/503 response presence via assertResponseStatus but not their JSON shapes;
update the spec tests to fetch the response schemas for those non-2xx cases (use
jsonResponseSchema with the same operation variables like openAI and status for
the 403 and 503 codes) and assert the expected error envelope fields via
assertRequired (e.g. require the canonical error fields such as "error" and
"message" or whichever envelope keys your project uses) so the 403/503 responses
are validated for both status and body.

internal/acp/types.go (1)

486-486: ⚖️ Poor tradeoff

Public Caps field may still allow unsynchronized access.

While CapsSnapshot(), setCaps(), and setConfigOptions() properly synchronize via capsMu, the Caps field on AgentProcess remains public. Direct access to p.Caps bypasses the mutex, potentially causing data races. Consider making Caps unexported to enforce access through the synchronized methods.

Also applies to: 598-624

🤖 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/acp/types.go` at line 486, The public Caps field on AgentProcess
allows unsynchronized access and should be made unexported (rename Caps to caps)
so all reads/writes go through the existing synchronized methods; update the
AgentProcess struct field name and any direct references to p.Caps across the
codebase to use the new unexported caps field or to call the synchronized
accessors (CapsSnapshot(), setCaps(), setConfigOptions()), and ensure capsMu
continues to protect all mutations and snapshots.

internal/session/manager_start.go (1)

387-404: 💤 Low value

Unused parameter in validateRuntimeOverrides.

The aghconfig.ResolvedAgent parameter is accepted but explicitly ignored (named _). If it's not needed for validation, consider removing it entirely to simplify the signature. If it's reserved for future validation logic, a brief comment would clarify intent.

🤖 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/session/manager_start.go` around lines 387 - 404, The
validateRuntimeOverrides function currently accepts an unused
aghconfig.ResolvedAgent parameter; either remove that parameter from the
signature of validateRuntimeOverrides and update any callers to match, or keep
it if planned for future use but stop using the blank identifier: rename the
parameter to something like resolvedAgent or add a short comment above the
function explaining it’s reserved for future validation so it’s not confusing to
reviewers; ensure you update all call sites of validateRuntimeOverrides (or
their invocations) to match the new signature if you remove the parameter.

internal/cli/provider_models.go (1)

118-140: 💤 Low value

Consider extracting duplicate row builder.

The text row builder (lines 118-128) and JSON row builder (lines 129-139) are identical. If they're always the same, a single function could be passed to listBundle twice, reducing duplication.

♻️ Proposed simplification

 func providerModelListBundle(record ProviderModelListRecord) outputBundle {
+	rowBuilder := func(model ProviderModelRecord) []string {
+		return []string{
+			model.ProviderID,
+			model.ModelID,
+			providerModelNullableBoolString(model.Available),
+			model.AvailabilityState,
+			providerModelBoolString(model.Stale),
+			providerModelSourceSummary(model.Sources),
+			model.RefreshedAt,
+		}
+	}
 	return listBundle(
 		record,
 		record.Models,
 		"Provider Models",
 		[]string{"Provider", "Model", "Available", "State", "Stale", "Sources", "Refreshed"},
 		"provider_models",
 		[]string{"provider_id", "model_id", "available", "availability_state", "stale", "sources", "refreshed_at"},
-		func(model ProviderModelRecord) []string {
-			return []string{
-				model.ProviderID,
-				model.ModelID,
-				providerModelNullableBoolString(model.Available),
-				model.AvailabilityState,
-				providerModelBoolString(model.Stale),
-				providerModelSourceSummary(model.Sources),
-				model.RefreshedAt,
-			}
-		},
-		func(model ProviderModelRecord) []string {
-			return []string{
-				model.ProviderID,
-				model.ModelID,
-				providerModelNullableBoolString(model.Available),
-				model.AvailabilityState,
-				providerModelBoolString(model.Stale),
-				providerModelSourceSummary(model.Sources),
-				model.RefreshedAt,
-			}
-		},
+		rowBuilder,
+		rowBuilder,
 	)
 }

🤖 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/cli/provider_models.go` around lines 118 - 140, The two identical
anonymous row-builder functions for ProviderModelRecord (used as the text and
JSON row builders) should be consolidated into a single reusable function:
extract a named function (e.g., buildProviderModelRow) that returns []string and
calls providerModelNullableBoolString, providerModelBoolString,
providerModelSourceSummary and accesses ProviderID, ModelID, AvailabilityState,
RefreshedAt on ProviderModelRecord; then pass that single buildProviderModelRow
function to listBundle twice instead of the duplicated lambdas.

internal/settings/service_test.go (1)

846-864: ⚡ Quick win

Assert that the empty providers.custom.models table is removed too.

This only proves the nested keys disappeared. If the writer still leaves [providers.custom.models] behind, the clear-path regression still passes while an empty overlay remains in the file. Add a negative check for the table header itself.

As per coding guidelines, "Check tests can fail when business logic changes".

🤖 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/service_test.go` around lines 846 - 864, After clearing
provider models in the test that calls service.PutCollectionItem
(CollectionItemPutRequest with ProviderSettings), add an assertion that the raw
config payload (configPayload read from homePaths.ConfigFile) does not contain
the empty table header for providers.custom.models; e.g. check that neither
`[[providers.custom.models]]` nor `[providers.custom.models]` appears and fail
the test if either is found so we ensure the writer removes the empty table
itself in addition to nested keys.

internal/extension/model_source_test.go (1)

250-345: ⚡ Quick win

Assert the specific validation failure for each row case.

Every case here passes on any non-nil error, so the table will miss regressions that reject a row for the wrong reason. Add an expected sentinel or substring per case and assert with errors.Is/ErrorContains.

As per coding guidelines, "MUST have specific error assertions (ErrorContains, ErrorAs)".

🤖 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/extension/model_source_test.go` around lines 250 - 345, The
table-driven tests currently only check for any non-nil error from
source.ListModels, which masks wrong rejection reasons; add an expected error
sentinel or substring field to the test cases (e.g., expectedErr error or
expectedErrSubstring string) and replace the generic nil check with a specific
assertion using errors.Is or testutil.ErrorContains/ErrorContains(t, err,
"<substring>") against that field; update the tests slice entries to specify the
correct expected error for each ModelSourceRow case so each branch (e.g.,
provider filter, negative ContextWindow, negative cost, duplicate reasoning
efforts, default effort not in list) is asserted explicitly when calling
ListModels.

internal/config/provider.go (1)

1028-1031: 💤 Low value

Misleading error message for whitespace-only default.

The condition strings.TrimSpace(m.Default) == "" && m.Default != "" correctly catches whitespace-only defaults, but the error message "is required" is confusing—it implies the field is missing rather than malformed.

Suggested clarification

 func (m ProviderModelsConfig) Validate(path string) error {
 	if strings.TrimSpace(m.Default) == "" && m.Default != "" {
-		return fmt.Errorf("%s.default is required", path)
+		return fmt.Errorf("%s.default cannot be whitespace-only", path)
 	}

🤖 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.go` around lines 1028 - 1031, The Validate method on
ProviderModelsConfig currently detects whitespace-only m.Default but returns a
misleading "is required" message; update the error returned in
ProviderModelsConfig.Validate (when strings.TrimSpace(m.Default) == "" &&
m.Default != "") to a clearer message such as "%s.default must not be empty or
whitespace" (or similar) so it accurately reflects that the value is present but
invalid (whitespace-only).

Fix all unresolved CodeRabbit comments on this PR:

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

internal/store/globaldb/global_db_network_conversations_test.go-221-230 (1)

221-230: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add explicit length check before comparison loop.

The sibling subtest at lines 267-269 guards the comparison loop with an explicit length check, but this new subtest does not. If secondRecords is shorter than firstRecords due to a regression, the test would panic on index out of bounds instead of producing a clear assertion failure.

🛡️ Proposed fix

 			}
+			if got, want := len(secondRecords), len(firstRecords); got != want {
+				t.Fatalf("len(secondRecords) = %d, want %d", got, want)
+			}
 			for index, firstRecord := range firstRecords {

🤖 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_network_conversations_test.go` around lines
221 - 230, Add an explicit length assertion before iterating over firstRecords
to compare AppliedAt; check len(secondRecords) == len(firstRecords) and call
t.Fatalf with both lengths if they differ, then proceed with the existing loop
that compares firstRecords[index].AppliedAt to secondRecords[index].AppliedAt
(symbols: firstRecords, secondRecords, AppliedAt).

internal/modelcatalog/types.go-156-156 (1)

156-156: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use the declared enum types for state fields instead of raw strings.

Model.AvailabilityState and SourceStatus.RefreshState are currently string, which bypasses the type guarantees provided by AvailabilityState and RefreshState.

Suggested fix

 type Model struct {
@@
-	AvailabilityState      string
+	AvailabilityState      AvailabilityState
@@
 }

 type SourceStatus struct {
@@
-	RefreshState string
+	RefreshState RefreshState
@@
 }

Also applies to: 181-181

🤖 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/modelcatalog/types.go` at line 156, The fields currently declared as
raw strings should use the declared enum types: change the Model struct's
AvailabilityState field from string to the AvailabilityState type and change
SourceStatus.RefreshState from string to the RefreshState type (preserving
existing JSON tags and zero-value handling); update any constructors,
unmarshalling or assignments that treat these fields as plain strings to use or
convert to the enum types (e.g., use AvailabilityState("...") or a helper parse
function where needed) so the compiler enforces valid enum values for
Model.AvailabilityState and SourceStatus.RefreshState.

internal/modelcatalog/source_id.go-12-23 (1)

12-23: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Reject whitespace-padded source IDs.

ValidateSourceID trims before validating, so values like " builtin " or "extension:demo " pass even though callers still retain the padded string. Since source IDs are used as exact identities, that lets a non-canonical value validate and then miss equality/map lookups later. Reject surrounding whitespace here, or expose a normalize-and-validate helper instead. Based on learnings "User-visible runtime capabilities in Go backend must expose stable machine-readable control surfaces."

Suggested fix

 func ValidateSourceID(sourceID string) error {
 	trimmed := strings.TrimSpace(sourceID)
+	if trimmed != sourceID {
+		return fmt.Errorf("model catalog source id %q must not contain surrounding whitespace", sourceID)
+	}
 	if trimmed == "" {
 		return fmt.Errorf("model catalog source id is required")
 	}

🤖 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/modelcatalog/source_id.go` around lines 12 - 23, ValidateSourceID
currently trims input which allows whitespace-padded IDs to validate; change it
to reject surrounding whitespace instead: in ValidateSourceID compare the
original sourceID to strings.TrimSpace(sourceID) and if they differ return an
error like "model catalog source id must not contain leading or trailing
whitespace"; then continue validation using the trimmed value (and keep the
existing staticSourceKind(trimmed) and strings.Cut(trimmed, ":") checks). This
preserves exact identity semantics while still validating normalized content.

internal/api/core/conversions_parsers_test.go-34-35 (1)

34-35: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Exercise normalization, not just pass-through.

The new assertions use already-trimmed "gpt-test" / "high" values, so they won't fail if SessionPayloadFromInfo stops trimming these fields. Seed whitespace here and keep the current expected assertions to cover the conversion behavior end-to-end.

Suggested test tweak

-		Model:           "gpt-test",
-		ReasoningEffort: "high",
+		Model:           " gpt-test ",
+		ReasoningEffort: " high ",

🤖 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/conversions_parsers_test.go` around lines 34 - 35, The test
currently uses already-trimmed values for Model and ReasoningEffort so it
doesn't verify normalization; update the test vector passed into
SessionPayloadFromInfo to include leading/trailing whitespace for the Model and
ReasoningEffort fields (e.g. " gpt-test " and " high ") while keeping the
expected assertions as the trimmed values ("gpt-test", "high") so the test
exercises trimming/normalization behavior of SessionPayloadFromInfo.

internal/api/contract/contract_test.go-107-117 (1)

107-117: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert the serialized config option values too.

The new JSON-shape check only verifies id, kind, and current. If config_options[0].values is dropped or mis-encoded, this test still passes even though the selector becomes unusable to clients.

Suggested assertion

 		if configOption["id"] != "model" || configOption["kind"] != "select" || configOption["current"] != "gpt-test" {
 			t.Fatalf("config option JSON = %#v", configOption)
 		}
+		values, ok := configOption["values"].([]any)
+		if !ok || len(values) != 1 {
+			t.Fatalf("config option values = %#v", configOption["values"])
+		}

🤖 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/contract/contract_test.go` around lines 107 - 117, The test
currently checks acpCaps -> config_options -> configOption for id/kind/current
but doesn't assert configOption["values"]; update the test (around
configOptions/configOption) to assert that configOption["values"] exists, is a
[]any, has at least one element, and that at least one element (each element
cast to map[string]any) contains the expected option fields (e.g., "id" and
"name") and that one element's "id" equals "gpt-test" so the selector values are
present and correctly serialized.

internal/api/udsapi/model_catalog_test.go-64-67 (1)

64-67: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert 404 response body, not only status code.

Line 64-Line 67 validates only the code; add a body assertion to make the route contract explicit.

Proposed test hardening

 		recorder := performRequest(t, engine, http.MethodGet, "/api/openai/v1/models", nil)
 		if recorder.Code != http.StatusNotFound {
 			t.Fatalf("status = %d, want 404; body=%s", recorder.Code, recorder.Body.String())
 		}
+		if body := recorder.Body.String(); body == "" {
+			t.Fatal("body = empty, want 404 response body")
+		}
 	})
 }

As per coding guidelines: "`**/*_test.go`: Always assert both HTTP status code AND response body in tests; status-code-only assertions are insufficient."

🤖 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/udsapi/model_catalog_test.go` around lines 64 - 67, The test
only checks the status code from performRequest(t, engine, http.MethodGet,
"/api/openai/v1/models")—add an assertion on recorder.Body to verify the
response body content as well (for example check JSON error shape or exact error
message expected by the route) so the route contract is explicit; update the
test after the existing StatusNotFound check to decode/inspect
recorder.Body.String() (or unmarshal if JSON) and assert it matches the expected
error payload/message.

internal/modelcatalog/modelsdev_test.go-173-195 (1)

173-195: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert the timeout failure itself, not just “some fast error.”

Any early failure would pass this test today, including validation or parsing bugs before the request is even made. Please verify that the returned error is actually a timeout.

Suggested change

 		if err == nil {
 			t.Fatal("ListModels(timeout) error = nil, want timeout error")
 		}
+		var timeoutErr interface{ Timeout() bool }
+		if !errors.As(err, &timeoutErr) || !timeoutErr.Timeout() {
+			t.Fatalf("ListModels(timeout) error = %v, want timeout error", err)
+		}
 		if elapsed >= 150*time.Millisecond {
 			t.Fatalf("elapsed = %s, want timeout before server sleep completes", elapsed)
 		}

As per coding guidelines: MUST have specific error assertions (ErrorContains, ErrorAs).

🤖 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/modelcatalog/modelsdev_test.go` around lines 173 - 195, The test
currently only checks that an error occurred; instead assert it's a timeout by
inspecting the returned error from source.ListModels: replace the generic
nil-check with a targeted assertion that the error is a timeout (e.g. use
errors.Is(err, context.DeadlineExceeded) or testutil.ErrorContains(t, err,
"Client.Timeout") / testutil.ErrorAs(t, &context.DeadlineExceeded, err)
depending on available helpers). Keep the rest of the timing assertion but
ensure the error check explicitly verifies a timeout came from the HTTP request
made by ListModels (reference the test's call to source.ListModels, the
ListOptions passed, and the surrounding test "Should apply explicit HTTP
timeout").

internal/cli/client_provider_models.go-55-77 (1)

55-77: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Reject blank provider IDs on provider-scoped model endpoints.

RefreshProviderModels and ProviderModelStatus currently turn an empty providerID into /api/providers/models/refresh or /api/providers/models/status. That converts a caller bug into a misleading route failure instead of returning a clear client-side error.

Suggested fix

 package cli
 
 import (
 	"context"
+	"errors"
 	"net/http"
 	"net/url"
 	"strings"
@@
 func (c *unixSocketClient) RefreshProviderModels(
 	ctx context.Context,
 	providerID string,
 	request ProviderModelRefreshRequest,
 ) (ProviderModelRefreshRecord, error) {
+	if strings.TrimSpace(providerID) == "" {
+		return ProviderModelRefreshRecord{}, errors.New("cli: provider id is required")
+	}
 	path := providerModelsPath(providerID, "refresh")
 	var response ProviderModelRefreshRecord
 	if err := c.doJSON(ctx, http.MethodPost, path, nil, request, &response); err != nil {
 		return ProviderModelRefreshRecord{}, err
 	}
@@
 func (c *unixSocketClient) ProviderModelStatus(
 	ctx context.Context,
 	providerID string,
 ) (ProviderModelStatusRecord, error) {
+	if strings.TrimSpace(providerID) == "" {
+		return ProviderModelStatusRecord{}, errors.New("cli: provider id is required")
+	}
 	path := providerModelsPath(providerID, "status")
 	var response ProviderModelStatusRecord
 	if err := c.doJSON(ctx, http.MethodGet, path, nil, nil, &response); err != nil {
 		return ProviderModelStatusRecord{}, 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/cli/client_provider_models.go` around lines 55 - 77, Validate that
providerID is non-empty in RefreshProviderModels and ProviderModelStatus before
calling providerModelsPath or c.doJSON; if providerID is empty or only
whitespace return a clear client-side error (e.g., fmt.Errorf("providerID
required")) so the method fails fast and does not construct routes like
"/api/providers/models/refresh"/"status"; update both
unixSocketClient.RefreshProviderModels and unixSocketClient.ProviderModelStatus
to perform this check (use strings.TrimSpace(providerID) to catch
whitespace-only values) and return the error instead of proceeding to
providerModelsPath/doJSON.

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

43-44: ⚡ Quick win

Rename subtest to the required Should ... form.

Line 43 currently uses "validate create session runtime overrides"; please rename to Should ... to match enforced test naming conventions.

Proposed change

-	t.Run("validate create session runtime overrides", func(t *testing.T) {
+	t.Run("Should validate create session runtime overrides", func(t *testing.T) {

As per coding guidelines **/*_test.go: Use t.Run('Should ...') pattern for Go test subtests instead of flat test structures.

🤖 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/session_workspace_internal_test.go` around lines 43 - 44,
Rename the subtest name passed to t.Run from "validate create session runtime
overrides" to follow the required "Should ..." pattern (for example "Should
validate create session runtime overrides") so update the t.Run(...) string in
the test (the subtest created in the Test function that calls t.Run and
t.Parallel) accordingly to match the enforced naming convention.

internal/extension/manager_model_source_test.go (1)

72-74: ⚡ Quick win

Assert the denied error explicitly.

err == nil is too broad and can hide wrong failure paths. Assert the expected denied error text/sentinel.

Proposed test tightening

 import (
+	"strings"
 	"testing"
@@
 		_, err := manager.ListModelSourceRows(
 			testutil.Context(t),
 			"ext-no-models",
 			extensioncontract.ModelSourceListParams{ProviderID: "codex"},
 		)
-		if err == nil {
-			t.Fatal("ListModelSourceRows() error = nil, want denied service method")
+		if err == nil || !strings.Contains(err.Error(), "denied") {
+			t.Fatalf("ListModelSourceRows() error = %v, want denied service method", err)
 		}
 	})
 }

As per coding guidelines "MUST have specific error assertions (ErrorContains, ErrorAs)".

🤖 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/extension/manager_model_source_test.go` around lines 72 - 74, The
test currently only checks err == nil after calling ListModelSourceRows(), which
is too broad; replace that check with a specific assertion that the error is the
expected "denied" error: call ListModelSourceRows() and assert err is not nil
and either errors.Is(err, ErrDeniedServiceMethod) (if you have a sentinel
ErrDeniedServiceMethod) or assert that err.Error() contains "denied service
method" (or use require.ErrorContains/Assert.ErrorContains from your test
helper) so the test explicitly verifies the denied-service error path.

internal/extension/capability_models_test.go (1)

86-95: ⚡ Quick win

Split per-method denials into subtests for clearer failures.

Line 86-Line 95 currently checks three methods in one loop. Converting each iteration into a t.Run("Should ...") gives isolated failures and aligns with your test-case convention.

Proposed refactor

-		for _, method := range []string{"models/list", "models/refresh", "models/status"} {
-			err := checker.CheckHostAPI("ext", method)
-			if err == nil {
-				t.Fatalf("CheckHostAPI(%q) error = nil, want capability denied", method)
-			}
-			var denied *ErrCapabilityDenied
-			if !errors.As(err, &denied) {
-				t.Fatalf("CheckHostAPI(%q) error = %T, want *ErrCapabilityDenied", method, err)
-			}
-		}
+		for _, method := range []string{"models/list", "models/refresh", "models/status"} {
+			method := method
+			t.Run("Should deny "+method+" for marketplace tier", func(t *testing.T) {
+				t.Parallel()
+				err := checker.CheckHostAPI("ext", method)
+				if err == nil {
+					t.Fatalf("CheckHostAPI(%q) error = nil, want capability denied", method)
+				}
+				var denied *ErrCapabilityDenied
+				if !errors.As(err, &denied) {
+					t.Fatalf("CheckHostAPI(%q) error = %T, want *ErrCapabilityDenied", method, err)
+				}
+			})
+		}

As per coding guidelines: "`**/*_test.go`: Use `t.Run('Should ...')` pattern for Go test subtests instead of flat test structures."

🤖 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/extension/capability_models_test.go` around lines 86 - 95, Refactor
the loop that asserts denial for "models/list", "models/refresh",
"models/status" into individual subtests using t.Run so failures are isolated;
for each method call checker.CheckHostAPI("ext", method) create a
t.Run(fmt.Sprintf("Should deny %s", method), func(t *testing.T) { ... }) body
that asserts err is non-nil and that errors.As(err, &denied) yields a
*ErrCapabilityDenied (use the same checker.CheckHostAPI and ErrCapabilityDenied
symbols), keeping the existing assertion logic inside each subtest.

internal/session/manager_start.go (1)

387-404: 💤 Low value

Consider removing the unused resolved parameter.

The validateRuntimeOverrides method accepts resolved aghconfig.ResolvedAgent but uses _ to ignore it. The validation operates solely on sessionStartSpec fields. If this parameter isn't needed for future validation logic, removing it would simplify the signature.

Optional: Remove unused parameter

-func (s *sessionStartSpec) validateRuntimeOverrides(_ aghconfig.ResolvedAgent) error {
+func (s *sessionStartSpec) validateRuntimeOverrides() error {

And at the call site (line 371):

-	if err := spec.validateRuntimeOverrides(resolved); err != nil {
+	if err := spec.validateRuntimeOverrides(); err != nil {

🤖 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/session/manager_start.go` around lines 387 - 404, The function
validateRuntimeOverrides currently accepts an unused parameter resolved of type
aghconfig.ResolvedAgent; remove that parameter from the validateRuntimeOverrides
signature and from all call sites (e.g., where validateRuntimeOverrides is
invoked around the former line 371) so the method only uses s
(sessionStartSpec); update any tests or callers to stop passing the resolved
argument and ensure compilation by adjusting imports/signatures accordingly.

internal/acp/config_options_test.go (1)

85-139: ⚡ Quick win

Wrap these cases in t.Run("Should ...") subtests.

The file switches back to flat top-level assertions here, which makes failures coarser than the repo’s test convention. Breaking the matching and legacy-state checks into named subtests will keep failures targeted and consistent with the rest of the suite.

As per coding guidelines: MUST use t.Run("Should...") pattern for ALL test cases.

🤖 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/acp/config_options_test.go` around lines 85 - 139, Tests in
TestConfigOptionMatching and TestLegacyModelStateAllows use flat assertions;
wrap each logical assertion group into t.Run("Should ...") subtests to follow
the repo convention. For TestConfigOptionMatching, create named subtests around
the model/effort discovery checks (calling findModelConfigOption and
findReasoningConfigOption) and the configOptionAllowsValue assertions
(advertised model, unadvertised model, boolean-as-select rejection). For
TestLegacyModelStateAllows, wrap the legacyModelStateAllows positive/negative
checks and the CloneCaps equality check into separate t.Run("Should ...")
subtests. Move the existing assertions into those subtests and leave the test
helper calls and variables (options, caps) in the parent test as needed.

internal/modelcatalog/sources.go (1)

160-166: ⚡ Quick win

Deep-copy the curated model data here.

maps.Copy only duplicates the map header. ProviderConfig.Models.Curated still shares the caller's backing array, so post-construction config edits can bleed into ListModels() and make this source stop behaving like a stable snapshot.

Based on learnings: Keep execution paths deterministic and observable in Go backend.

🤖 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/modelcatalog/sources.go` around lines 160 - 166, The
cloneConfigProviders function currently uses maps.Copy which leaves slice
backing arrays shared; replace it with an explicit loop over src (in
cloneConfigProviders) that copies each aghconfig.ProviderConfig value into the
new map and, for each config, deep-copy the Models.Curated slice (e.g., allocate
a new slice and copy elements) so the cloned ProviderConfig.Models.Curated does
not share the caller's backing array; ensure you still handle nil src by
returning an empty map.

internal/config/provider.go (2)

1163-1174: 💤 Low value

Consider using explicit scheme constants for clarity.

Using string(MCPServerTransportHTTP) as the HTTP scheme works because the constant happens to equal "http", but this couples URL validation to MCP transport terminology. Consider using explicit "http" and "https" strings or defining separate URL scheme constants.

Suggested fix

+const (
+	urlSchemeHTTP  = "http"
+	urlSchemeHTTPS = "https"
+)
+
 func validateAbsoluteHTTPURL(path string, raw string) error {
 	parsed, err := url.Parse(strings.TrimSpace(raw))
 	if err != nil || parsed.Scheme == "" || parsed.Host == "" {
 		return fmt.Errorf("%s must be an absolute HTTP(S) URL", path)
 	}
 	switch parsed.Scheme {
-	case string(MCPServerTransportHTTP), urlSchemeHTTPS:
+	case urlSchemeHTTP, urlSchemeHTTPS:
 		return nil
 	default:
 		return fmt.Errorf("%s must be an absolute HTTP(S) URL", path)
 	}
 }

🤖 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.go` around lines 1163 - 1174, The URL validation in
validateAbsoluteHTTPURL currently uses string(MCPServerTransportHTTP) which
couples URL schemes to MCP transport names; change the comparison to use
explicit HTTP scheme constants (either replace string(MCPServerTransportHTTP)
with the literal "http" or define a new constant like urlSchemeHTTP = "http")
and update the switch (which already references urlSchemeHTTPS) to use
urlSchemeHTTP and urlSchemeHTTPS so the function validates against standard URL
scheme strings rather than transport-specific symbols.

---

1152-1161: 💤 Low value

Consider including the parse error in the validation message for debugging.

When time.ParseDuration fails, the original error contains useful information about what was wrong with the input. Wrapping it with %w would help operators debug malformed duration strings.

Suggested fix

 func validatePositiveDuration(path string, raw string) error {
 	duration, err := time.ParseDuration(strings.TrimSpace(raw))
 	if err != nil {
-		return fmt.Errorf("%s must be a positive duration", path)
+		return fmt.Errorf("%s must be a positive duration: %w", path, err)
 	}
 	if duration <= 0 {
 		return fmt.Errorf("%s must be a positive duration", path)
 	}
 	return nil
 }

🤖 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.go` around lines 1152 - 1161, The
validatePositiveDuration function returns a generic error when
time.ParseDuration fails; modify the error path in validatePositiveDuration to
wrap the original parse error (from time.ParseDuration) using fmt.Errorf with %w
so the returned error includes the underlying parse error details (retain the
existing "%s must be a positive duration" message while wrapping err), and
ensure other logic (duration <= 0) remains unchanged.

internal/extension/model_source.go (1)

220-222: 💤 Low value

Calling .UTC() on potentially zero ExpiresAt may produce misleading timestamps.

When row.ExpiresAt is zero (i.e., the extension didn't provide an expiration), calling .UTC() on it returns 0001-01-01 00:00:00 +0000 UTC rather than preserving zero-value semantics. Consider guarding this similar to the RefreshedAt handling at lines 208-211.

Suggested fix

 	refreshedAt := row.RefreshedAt
 	if refreshedAt.IsZero() {
 		refreshedAt = now
 	}
+	expiresAt := row.ExpiresAt
+	if !expiresAt.IsZero() {
+		expiresAt = expiresAt.UTC()
+	}
 	modelRow := modelcatalog.ModelRow{
 		...
-		ExpiresAt:              row.ExpiresAt.UTC(),
+		ExpiresAt:              expiresAt,
 		...
 	}

🤖 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/extension/model_source.go` around lines 220 - 222, The code
currently calls row.ExpiresAt.UTC() which converts a zero time to the 0001-01-01
UTC timestamp; instead, mirror the RefreshedAt handling and guard ExpiresAt: if
row.ExpiresAt.IsZero() set the ExpiresAt field to the zero time value, otherwise
set it to row.ExpiresAt.UTC(); update the struct assignment that sets ExpiresAt
(near Stale/RefreshedAt/ExpiresAt) to perform this conditional check so zero
semantics are preserved.

Fix all unresolved CodeRabbit comments on this PR:

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