Skip to content

Commit 638de88

Browse files
author
Mateusz
committed
fix: address model catalog PR review
1 parent 47b6965 commit 638de88

12 files changed

Lines changed: 87 additions & 15 deletions

File tree

‎internal/core/config/effective_secure_session.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ func EffectiveSecureSessionSQLQueryCache(ss SecureSessionConfig) (ttl time.Durat
1919
return 0, 0, false
2020
}
2121
me := ss.SQLQueryCacheMaxEntries
22-
if me == 0 {
22+
if me <= 0 {
2323
me = defaultSecureSessionSQLQueryCacheMaxEntries
2424
}
2525
return d, uint64(me), true

‎internal/core/config/model_catalog_test.go‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -342,6 +342,49 @@ model_catalog:
342342
}
343343
}
344344

345+
func TestLoadFile_modelCatalog_rejectsBackendModelOverrideEmptyModel(t *testing.T) {
346+
t.Parallel()
347+
cache := yamlPath(filepath.Join(t.TempDir(), "c.json"))
348+
p := filepath.Join(t.TempDir(), "cfg.yaml")
349+
body := minimalLoadableYAML + `
350+
model_catalog:
351+
enabled: true
352+
cache_path: "` + cache + `"
353+
backend_model_overrides:
354+
- backend: "b1"
355+
model: ""
356+
`
357+
if err := os.WriteFile(p, []byte(body), 0o600); err != nil {
358+
t.Fatal(err)
359+
}
360+
_, err := config.LoadFile(p)
361+
if err == nil || !strings.Contains(err.Error(), "model_catalog.backend_model_overrides") {
362+
t.Fatalf("want backend_model_overrides error, got %v", err)
363+
}
364+
}
365+
366+
func TestLoadFile_modelCatalog_rejectsBackendOverrideNonPositiveContextLimit(t *testing.T) {
367+
t.Parallel()
368+
cache := yamlPath(filepath.Join(t.TempDir(), "c.json"))
369+
p := filepath.Join(t.TempDir(), "cfg.yaml")
370+
body := minimalLoadableYAML + `
371+
model_catalog:
372+
enabled: true
373+
cache_path: "` + cache + `"
374+
backend_model_overrides:
375+
- backend: "b1"
376+
model: "m1"
377+
context_limit_tokens: 0
378+
`
379+
if err := os.WriteFile(p, []byte(body), 0o600); err != nil {
380+
t.Fatal(err)
381+
}
382+
_, err := config.LoadFile(p)
383+
if err == nil || !strings.Contains(err.Error(), "context_limit_tokens") {
384+
t.Fatalf("want context_limit_tokens error, got %v", err)
385+
}
386+
}
387+
345388
func TestLoadFile_modelCatalog_omittedIsDisabled(t *testing.T) {
346389
t.Parallel()
347390
p := filepath.Join(t.TempDir(), "cfg.yaml")

‎internal/core/config/validate.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -674,6 +674,12 @@ func validateModelCatalog(cfg *Config) error {
674674
if strings.TrimSpace(row.Backend) == "" {
675675
return fmt.Errorf("model_catalog.backend_model_overrides[%d].backend: required", i)
676676
}
677+
if strings.TrimSpace(row.Model) == "" {
678+
return fmt.Errorf("model_catalog.backend_model_overrides[%d].model: required", i)
679+
}
680+
if err := posLimit("context_limit_tokens", row.ContextLimitTokens); err != nil {
681+
return err
682+
}
677683
if err := posLimit("input_limit_tokens", row.InputLimitTokens); err != nil {
678684
return err
679685
}

‎internal/core/modelcatalog/diagnostics.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,8 @@ func RedactSourceURL(raw string) string {
6969
return "redacted:invalid"
7070
}
7171
u.User = nil
72+
u.RawQuery = ""
73+
u.Fragment = ""
7274
return strings.TrimSpace(u.String())
7375
}
7476

‎internal/core/modelcatalog/diagnostics_test.go‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,14 @@ func TestRedactSourceURL_stripsUserinfo(t *testing.T) {
1919
}
2020
}
2121

22+
func TestRedactSourceURL_stripsQueryFragmentAndUserinfo(t *testing.T) {
23+
t.Parallel()
24+
got := RedactSourceURL("https://user:secret@example.com/models.json?api_key=hidden#token")
25+
if got != "https://example.com/models.json" {
26+
t.Fatalf("redacted URL = %q", got)
27+
}
28+
}
29+
2230
func TestBuildCatalogDiagnosticsJSON_usageDisabled(t *testing.T) {
2331
t.Parallel()
2432
v := BuildCatalogDiagnosticsJSON(CatalogStatusHandlerConfig{UsageEnabled: false})

‎internal/core/modelcatalog/eligibility.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,10 @@ import (
99

1010
// EligibilityDecision is the routing-time outcome for context limits before backend open.
1111
type EligibilityDecision struct {
12-
IsEligible bool `json:"eligible,omitempty"`
13-
Reason EligibilityReason
14-
Facts EffectiveFacts
15-
Estimate SizeEstimate
12+
IsEligible bool `json:"eligible"`
13+
Reason EligibilityReason `json:"reason,omitempty"`
14+
Facts EffectiveFacts `json:"facts,omitempty"`
15+
Estimate SizeEstimate `json:"estimate,omitempty"`
1616
}
1717

1818
// EligibilityResolverImpl decides context-limit eligibility from already-resolved [EffectiveFacts]

‎internal/core/modelcatalog/matcher.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ var _ Matcher = DefaultMatcher{}
2828
// Match implements [Matcher].
2929
func (DefaultMatcher) Match(candidate routing.AttemptCandidate, index *SnapshotIndex) MatchResult {
3030
input := strings.TrimSpace(candidate.Primary.Model)
31-
if index == nil {
31+
if index == nil || input == "" {
3232
return MatchResult{Kind: MatchNoMatch, InputModel: input}
3333
}
3434
if _, ok := index.byCatalogModelID[input]; ok {

‎internal/core/modelcatalog/matcher_test.go‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,3 +121,14 @@ func TestMatcher_Match_nilIndex(t *testing.T) {
121121
t.Fatal(got.Kind)
122122
}
123123
}
124+
125+
func TestDefaultMatcher_emptyInputNoMatch(t *testing.T) {
126+
t.Parallel()
127+
idx := modelcatalog.NewSnapshotIndex(map[string]modelcatalog.ModelFacts{
128+
"openai/": {Tools: modelcatalog.CapabilitySupported},
129+
})
130+
got := modelcatalog.DefaultMatcher{}.Match(routing.AttemptCandidate{Primary: routing.Primary{Model: " "}}, idx)
131+
if got.Kind != modelcatalog.MatchNoMatch {
132+
t.Fatalf("kind = %v", got.Kind)
133+
}
134+
}

‎internal/core/securesession/adapters/bunstore/store_test.go‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ func newTestStoreWithOpts(t *testing.T, opts Options) (*Store, func()) {
181181
func TestBunStore_sqlMetaCache_appendTranscriptStaleUntilTTL(t *testing.T) {
182182
t.Parallel()
183183
ctx := context.Background()
184-
st, cleanup := newTestStoreWithOpts(t, Options{SQLQueryCacheTTL: 120 * time.Millisecond, SQLQueryCacheMaxEntries: 64})
184+
st, cleanup := newTestStoreWithOpts(t, Options{SQLQueryCacheTTL: 50 * time.Millisecond, SQLQueryCacheMaxEntries: 64})
185185
defer cleanup()
186186
fp := domain.TokenFingerprint{}
187187
fp[0] = 1
@@ -211,18 +211,19 @@ func TestBunStore_sqlMetaCache_appendTranscriptStaleUntilTTL(t *testing.T) {
211211
}); err != nil {
212212
t.Fatalf("expected cached policy before TTL: %v", err)
213213
}
214-
time.Sleep(150 * time.Millisecond)
215-
if err := st.AppendTranscript(ctx, domain.TranscriptItem{
214+
time.Sleep(500 * time.Millisecond)
215+
err := st.AppendTranscript(ctx, domain.TranscriptItem{
216216
SessionID: cr.SessionID, TurnID: "t1", EventKind: "e3", PayloadRef: "p3", CreatedAt: time.Unix(4, 0),
217-
}); err != domain.ErrTranscriptDisabled {
217+
})
218+
if !errors.Is(err, domain.ErrTranscriptDisabled) {
218219
t.Fatalf("want ErrTranscriptDisabled after TTL got %v", err)
219220
}
220221
}
221222

222223
func TestBunStore_sqlMetaCache_transcriptObservesStalePolicyUntilTTL(t *testing.T) {
223224
t.Parallel()
224225
ctx := context.Background()
225-
st, cleanup := newTestStoreWithOpts(t, Options{SQLQueryCacheTTL: 120 * time.Millisecond, SQLQueryCacheMaxEntries: 64})
226+
st, cleanup := newTestStoreWithOpts(t, Options{SQLQueryCacheTTL: 50 * time.Millisecond, SQLQueryCacheMaxEntries: 64})
226227
defer cleanup()
227228

228229
fp := domain.TokenFingerprint{}
@@ -258,7 +259,7 @@ func TestBunStore_sqlMetaCache_transcriptObservesStalePolicyUntilTTL(t *testing.
258259
if len(items) != 1 {
259260
t.Fatalf("want stale read to still return transcript rows got len=%d", len(items))
260261
}
261-
time.Sleep(150 * time.Millisecond)
262+
time.Sleep(500 * time.Millisecond)
262263
items2, err := st.Transcript(ctx, cr.SessionID, domain.ReadOptions{})
263264
if err != nil {
264265
t.Fatal(err)

‎internal/infra/modelcatalog/modelsdev/normalize.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ func limitFromNumber(n json.Number) modelcatalog.LimitFact {
123123
v, err := n.Int64()
124124
if err != nil {
125125
f, ferr := n.Float64()
126-
if ferr != nil || f <= 0 || math.IsNaN(f) || math.IsInf(f, 0) || f > float64(math.MaxInt64) {
126+
if ferr != nil || f <= 0 || math.IsNaN(f) || math.IsInf(f, 0) || f >= float64(math.MaxInt64) {
127127
return modelcatalog.LimitFact{State: modelcatalog.LimitUnknown}
128128
}
129129
v = int64(f)

0 commit comments

Comments
 (0)