From a024310c0d7f9670fc1b5f6ee8aa94ccaa6a05cb Mon Sep 17 00:00:00 2001 From: Pablo Ontiveros Date: Wed, 7 Oct 2026 10:02:44 -0600 Subject: [PATCH 1/2] refactor(services): share query validation --- AGENTS.md | 2 +- cmd/backscroll/context.go | 41 +++++------ cmd/backscroll/context_test.go | 26 +++++++ cmd/backscroll/search.go | 39 +++++------ cmd/backscroll/search_test.go | 33 +++++++++ internal/services/query.go | 67 ++++++++++++++++++ internal/services/query_test.go | 120 ++++++++++++++++++++++++++++++++ 7 files changed, 280 insertions(+), 48 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8b430e99..00a9efc3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -159,7 +159,7 @@ External knowledge sources are configured with active `*.inputs.toml` manifests - **B2 project fallback identity + historical re-resolution**: when the global registry (`~/.config/backscroll/projects.toml`) does not match a session's cwd, `projects.Identify()` derives a sanitized fallback id from the cwd basename (lowercase, `[a-z0-9-_]`; registry always wins when it matches). `rebuild` runs a re-resolution pass: `ReresolveProjects` decodes Claude's session-dir encoding (dashes-for-slashes; if the decoded path exists on disk its basename wins, else the last dash segment — the encoding is lossy and this heuristic is documented as ambiguous for dir names containing dashes) and relabels rows stuck at `project='unknown'`, returning distinct files resolved. Fallback labels are not revisited once set — followed by registry-aware re-resolution below. - **B2.1 registry-aware re-resolution**: `projects.Identify()` now carries a `FromRegistry` flag to distinguish registry matches from fallback labels. `rebuild` includes a second phase that loads `~/.config/backscroll/projects.toml` and re-resolves historical sessions labeled with fallback IDs. For each session path, it decodes the cwd, applies cross-host normalization, and calls `Identify` with the registry. If a registry entry matches (FromRegistry=true) and differs from the stored fallback, the rows are updated. Only registry matches count — fallback-only paths are not re-labeled (no churn). This allows a future registry entry to correct historical misattribution without touching already-correct sessions. Wired into rebuild via `ReresolveProjectsWithRegistry` (queries.go). - **B3 retroactive mining over stored text**: for sessions that expire from disk before Template/Correction/Sequence mining runs, `BackfillDerived()` recovers templates, correction signals, and lossy tool_events from stored text in search_items. Stale template re-mining (F2a) is integrated: BackfillDerived first discovers stale paths via `StaleTemplatePaths(CurrentNormalizationVersion)` and re-mines templates under v2 heuristics, updating existing templates to normalization_version=2. Template mining reuses `internal/templates/Miner` over tool-text rows. Correction detection filters input to prose only (role='user' AND content_type IN ('text','code')) to avoid tool_result false positives; lexicon, denial, and rephrase detectors run on prose; interrupt detector runs on all user messages. Lossy tool_events reverse-parse toolfmt input serialization ( ...; heuristic: first token has no '=', at least one subsequent has '=') to extract tool_name and command_head; outputs unattributable without tool_use_id so skipped; extraction_version=0 marks lossy rows (inputs only, unrecoverable once disk source expires). `rebuild` command now runs after mutation-class startup sync and then: (1) re-derives FTS from DB, (2) BackfillDerived (stale-path discovery + re-mining + expired-file discovery + batch mining + progress reporting), (3) re-resolves projects, and (4) applies registry-aware re-resolution. It performs no second sync. The recover path retains the remediation lock through post-install sync. -- **Early input validation**: CLI commands validate positional arguments and semantic flag values through Cobra `Args`, which runs before the root `PersistentPreRunE`; the root also checks required flags and flag groups before invoking the startup policy for the command class. Invalid invocations therefore fail before opening, creating, synchronizing, or recovering the database. Keep pure validators reusable from direct `run*` callers so tests and non-Cobra entry points preserve the same boundary. +- **Early input validation**: CLI commands validate positional arguments and semantic flag values through Cobra `Args`, which runs before the root `PersistentPreRunE`; the root also checks required flags and flag groups before invoking the startup policy for the command class. Invalid invocations therefore fail before opening, creating, synchronizing, or recovering the database. `internal/services` owns pure query validation as the shared CLI/MCP boundary. Direct service calls validate before database access. - **Coverage gate**: CI enforces ≥85% aggregate statement coverage via `go test ./... -race -coverprofile`. Local check: `bash scripts/check-coverage.sh`. Tests that depend on local machine state (e.g. `~/.config/backscroll/projects.toml`) must use `t.Setenv("HOME", tempDir)` to stay reproducible on CI. Likewise, `InputsDir` branches requiring `BACKSCROLL_CONFIG_DIR` to be unset must set it to `""` via `t.Setenv`. To test the `Validate` orphan path, insert directly into `search_items` without a matching `indexed_files` row. - **Zero-result guidance**: when `search`/`list` return no rows, actionable suggestions (`--all-projects`, `--content-type tool`, `backscroll status`) are printed to STDERR — never STDOUT, so `--json` stays a clean empty payload. - **Search robot output contract**: robot mode on search emits `result_N_field=value` lines exactly once-wrapped (the robot path writes lines directly; passing pre-formatted lines through the picokit formatter double-wraps them as `result_N=result_N_field=...`). Search robot string values escape backslash as `\\`, carriage return as `\r`, and newline as `\n`. Minimal fields use bounded snippets; full fields retain indexed content. Positive `--max-tokens` budgets count the complete escaped payload with one Picokit estimate, including omission metadata; never sum independently rounded line/result estimates. Truncation removes whole trailing results, and emits no output when even the omission record cannot fit. diff --git a/cmd/backscroll/context.go b/cmd/backscroll/context.go index 776df005..718ad594 100644 --- a/cmd/backscroll/context.go +++ b/cmd/backscroll/context.go @@ -20,7 +20,6 @@ import ( const ( contextDefaultWindow = 5 - contextMaxWindow = 50 contextMaxTextRunes = 4000 contextDefaultMaxTokens = 2000 contextMinMaxTokens = 64 @@ -121,22 +120,8 @@ func (opts *contextCommandOptions) captureSelectorFlags(cmd *cobra.Command) { } func validateContextRequest(opts contextCommandOptions) error { - uuidSelector := opts.uuidSet && !opts.sourcePathSet && !opts.ordinalSet - pathOrdinalSelector := !opts.uuidSet && opts.sourcePathSet && opts.ordinalSet - if !uuidSelector && !pathOrdinalSelector { - return fmt.Errorf("provide exactly one anchor selector: --uuid or --source-path with --ordinal") - } - if uuidSelector && opts.uuid == "" { - return fmt.Errorf("--uuid must not be empty") - } - if pathOrdinalSelector && opts.sourcePath == "" { - return fmt.Errorf("--source-path must not be empty") - } - if opts.before < 0 || opts.before > contextMaxWindow { - return fmt.Errorf("--before must be between 0 and %d", contextMaxWindow) - } - if opts.after < 0 || opts.after > contextMaxWindow { - return fmt.Errorf("--after must be between 0 and %d", contextMaxWindow) + if err := services.ValidateContextRequest(contextServiceRequest(opts)); err != nil { + return err } if opts.maxTokens < contextMinMaxTokens || opts.maxTokens > contextMaxMaxTokens { return fmt.Errorf("--max-tokens must be between %d and %d", contextMinMaxTokens, contextMaxMaxTokens) @@ -147,6 +132,20 @@ func validateContextRequest(opts contextCommandOptions) error { return nil } +func contextServiceRequest(opts contextCommandOptions) services.ContextRequest { + request := services.ContextRequest{Before: opts.before, After: opts.after} + if opts.uuidSet { + request.UUID = &opts.uuid + } + if opts.sourcePathSet { + request.SourcePath = &opts.sourcePath + } + if opts.ordinalSet { + request.Ordinal = &opts.ordinal + } + return request +} + func runContextCommand(ctx context.Context, stdout, stderr io.Writer, cfg *config.Config, opts contextCommandOptions) (retErr error) { if err := validateContextRequest(opts); err != nil { return err @@ -161,13 +160,7 @@ func runContextCommand(ctx context.Context, stdout, stderr io.Writer, cfg *confi } defer func() { retErr = closeIndexDB(db, retErr) }() - request := services.ContextRequest{Before: opts.before, After: opts.after} - if opts.uuidSet { - request.UUID = &opts.uuid - } else { - request.SourcePath = &opts.sourcePath - request.Ordinal = &opts.ordinal - } + request := contextServiceRequest(opts) queryService := services.QueryService{DB: db} response, err := queryService.Context(ctx, request) if err != nil { diff --git a/cmd/backscroll/context_test.go b/cmd/backscroll/context_test.go index b326ccab..69b4963d 100644 --- a/cmd/backscroll/context_test.go +++ b/cmd/backscroll/context_test.go @@ -12,6 +12,7 @@ import ( "github.com/pablontiv/backscroll/internal/config" "github.com/pablontiv/backscroll/internal/models" + "github.com/pablontiv/backscroll/internal/services" "github.com/pablontiv/backscroll/internal/storage" ) @@ -278,6 +279,31 @@ func TestContextTextAndRobotSuccessfulPayloadBudgetBoundaries(t *testing.T) { } } +func TestContextValidationMatchesServices(t *testing.T) { + tests := []struct { + name string + opts contextCommandOptions + }{ + {name: "no selector", opts: contextCommandOptions{maxTokens: contextDefaultMaxTokens}}, + {name: "both selectors", opts: contextCommandOptions{uuid: "u", uuidSet: true, sourcePath: "/x", sourcePathSet: true, ordinalSet: true, maxTokens: contextDefaultMaxTokens}}, + {name: "empty UUID", opts: contextCommandOptions{uuidSet: true, maxTokens: contextDefaultMaxTokens}}, + {name: "before high", opts: contextCommandOptions{uuid: "u", uuidSet: true, before: 51, maxTokens: contextDefaultMaxTokens}}, + {name: "valid", opts: contextCommandOptions{uuid: "u", uuidSet: true, before: 0, after: 50, maxTokens: contextDefaultMaxTokens}}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + cliErr := validateContextRequest(test.opts) + serviceErr := services.ValidateContextRequest(contextServiceRequest(test.opts)) + if (cliErr == nil) != (serviceErr == nil) { + t.Fatalf("CLI error = %v, service error = %v", cliErr, serviceErr) + } + if cliErr != nil && cliErr.Error() != serviceErr.Error() { + t.Fatalf("CLI error = %q, service error = %q", cliErr, serviceErr) + } + }) + } +} + func TestContextValidationLimitsAndSelectors(t *testing.T) { validUUID := contextCommandOptions{uuid: "opaque", uuidSet: true, before: 0, after: 50, maxTokens: 64} if err := validateContextRequest(validUUID); err != nil { diff --git a/cmd/backscroll/search.go b/cmd/backscroll/search.go index d8371381..711edb37 100644 --- a/cmd/backscroll/search.go +++ b/cmd/backscroll/search.go @@ -66,15 +66,10 @@ protected. All scope filters remain fixed; no OR or semantic expansion is used.` Args: func(cmd *cobra.Command, args []string) error { return validateCommandBeforeStartup(cmd, args, cobra.MaximumNArgs(1), func() error { query := searchQuery(text, args) - _, _, err := validateAndParseSearchRequest(query, fields, contentType, after, before) + _, _, err := validateAndParseSearchRequest(query, fields, contentType, after, before, relax) if err != nil { return err } - if relax { - if err := storage.ValidateRelaxationQuery(query); err != nil { - return err - } - } resolved, err := effectiveProject(project, allProjects) if err == nil { project = resolved @@ -124,22 +119,18 @@ func searchQuery(text string, args []string) string { return text } -func validateAndParseSearchRequest(query, fields, contentType, after, before string) (*time.Time, *time.Time, error) { - if query == "" { - return nil, nil, fmt.Errorf("search query required (use --text or positional argument)") +func validateAndParseSearchRequest(query, fields, contentType, after, before string, relax bool) (*time.Time, *time.Time, error) { + request := services.SearchRequest{Query: query} + if err := services.ValidateSearchRequest(request); err != nil { + return nil, nil, err } if fields != "minimal" && fields != "full" { return nil, nil, fmt.Errorf("invalid --fields value %q: must be minimal or full", fields) } - validContentTypes := map[string]bool{ - "text": true, - "code": true, - "tool": true, - "reasoning": true, - } - if contentType != "" && !validContentTypes[contentType] { - return nil, nil, fmt.Errorf("invalid --content-type %q; must be one of: text, code, tool, reasoning", contentType) + request.Options.ContentType = contentType + if err := services.ValidateSearchRequest(request); err != nil { + return nil, nil, err } var afterTime, beforeTime *time.Time @@ -157,6 +148,13 @@ func validateAndParseSearchRequest(query, fields, contentType, after, before str } beforeTime = &parsed } + + request.Options.After = afterTime + request.Options.Before = beforeTime + request.Relax = relax + if err := services.ValidateSearchRequest(request); err != nil { + return nil, nil, err + } return afterTime, beforeTime, nil } @@ -168,15 +166,10 @@ func runSearch(ctx context.Context, stdout, stderr io.Writer, cfg *config.Config fields string, maxTokens int, lexicalOnly bool, similarityThreshold float64, relax bool) (retErr error) { - afterTime, beforeTime, err := validateAndParseSearchRequest(query, fields, contentType, after, before) + afterTime, beforeTime, err := validateAndParseSearchRequest(query, fields, contentType, after, before, relax) if err != nil { return err } - if relax { - if err := storage.ValidateRelaxationQuery(query); err != nil { - return err - } - } project, err = effectiveProject(project, allProjects) if err != nil { diff --git a/cmd/backscroll/search_test.go b/cmd/backscroll/search_test.go index 4be1c018..233d1062 100644 --- a/cmd/backscroll/search_test.go +++ b/cmd/backscroll/search_test.go @@ -11,8 +11,41 @@ import ( picokitoutput "github.com/pablontiv/picokit/output" "github.com/pablontiv/backscroll/internal/models" + "github.com/pablontiv/backscroll/internal/services" ) +func TestSearchValidationMatchesServices(t *testing.T) { + tests := []struct { + name string + query string + contentType string + relax bool + }{ + {name: "missing query"}, + {name: "invalid content type", query: "needle", contentType: "audio"}, + {name: "invalid relaxation query", query: `"unclosed`, relax: true}, + {name: "valid", query: "needle", contentType: "text"}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + _, _, cliErr := validateAndParseSearchRequest(test.query, "minimal", test.contentType, "", "", test.relax) + serviceErr := services.ValidateSearchRequest(services.SearchRequest{ + Query: test.query, + Options: models.SearchOptions{ + ContentType: test.contentType, + }, + Relax: test.relax, + }) + if (cliErr == nil) != (serviceErr == nil) { + t.Fatalf("CLI error = %v, service error = %v", cliErr, serviceErr) + } + if cliErr != nil && cliErr.Error() != serviceErr.Error() { + t.Fatalf("CLI error = %q, service error = %q", cliErr, serviceErr) + } + }) + } +} + func TestSearchOutputFormatText(t *testing.T) { _, cleanup := testEnv(t) defer cleanup() diff --git a/internal/services/query.go b/internal/services/query.go index edf53103..dbe66d5d 100644 --- a/internal/services/query.go +++ b/internal/services/query.go @@ -3,11 +3,14 @@ package services import ( "context" + "fmt" "github.com/pablontiv/backscroll/internal/models" "github.com/pablontiv/backscroll/internal/storage" ) +const maxContextWindow = 50 + // QueryService coordinates read-only queries against the perennial index. type QueryService struct { DB *storage.Database @@ -31,8 +34,43 @@ type SearchResponse struct { Provenance SearchProvenance } +// ValidateSearchRequest validates retrieval semantics without accessing storage. +func ValidateSearchRequest(request SearchRequest) error { + if request.Query == "" { + return fmt.Errorf("search query required (use --text or positional argument)") + } + + switch request.Options.ContentType { + case "", "text", "code", "tool", "reasoning": + default: + return fmt.Errorf("invalid --content-type %q; must be one of: text, code, tool, reasoning", request.Options.ContentType) + } + + if request.Relax { + return storage.ValidateRelaxationQuery(request.Query) + } + return nil +} + +// ValidateExplicitSearchScope requires one project or all projects. +// Remote consumers use this check in addition to ValidateSearchRequest. +func ValidateExplicitSearchScope(request SearchRequest) error { + hasProject := request.Options.Project != "" + if hasProject == request.Options.AllProjects { + return fmt.Errorf("provide exactly one project scope: project or all_projects=true") + } + return nil +} + // Search runs either ordinary hybrid retrieval or opt-in lexical relaxation. func (s QueryService) Search(ctx context.Context, request SearchRequest) (SearchResponse, error) { + if err := ctx.Err(); err != nil { + return SearchResponse{}, err + } + if err := ValidateSearchRequest(request); err != nil { + return SearchResponse{}, err + } + if request.Relax { results, stages, err := s.DB.SearchRelaxedContext(ctx, request.Query, request.Options) return SearchResponse{ @@ -61,8 +99,37 @@ type ContextResponse struct { Records []storage.ContextRecord } +// ValidateContextRequest validates selector and window semantics without accessing storage. +func ValidateContextRequest(request ContextRequest) error { + uuidSelector := request.UUID != nil && request.SourcePath == nil && request.Ordinal == nil + pathOrdinalSelector := request.UUID == nil && request.SourcePath != nil && request.Ordinal != nil + if !uuidSelector && !pathOrdinalSelector { + return fmt.Errorf("provide exactly one anchor selector: --uuid or --source-path with --ordinal") + } + if uuidSelector && *request.UUID == "" { + return fmt.Errorf("--uuid must not be empty") + } + if pathOrdinalSelector && *request.SourcePath == "" { + return fmt.Errorf("--source-path must not be empty") + } + if request.Before < 0 || request.Before > maxContextWindow { + return fmt.Errorf("--before must be between 0 and %d", maxContextWindow) + } + if request.After < 0 || request.After > maxContextWindow { + return fmt.Errorf("--after must be between 0 and %d", maxContextWindow) + } + return nil +} + // Context retrieves an exact indexed neighborhood without consulting source files. func (s QueryService) Context(ctx context.Context, request ContextRequest) (ContextResponse, error) { + if err := ctx.Err(); err != nil { + return ContextResponse{}, err + } + if err := ValidateContextRequest(request); err != nil { + return ContextResponse{}, err + } + records, err := s.DB.QueryContextRecords(ctx, storage.ContextRecordQuery{ UUID: request.UUID, SourcePath: request.SourcePath, diff --git a/internal/services/query_test.go b/internal/services/query_test.go index ff1c97b2..81ea656e 100644 --- a/internal/services/query_test.go +++ b/internal/services/query_test.go @@ -5,6 +5,7 @@ import ( "errors" "path/filepath" "reflect" + "strings" "testing" "github.com/pablontiv/backscroll/internal/models" @@ -93,6 +94,125 @@ func TestQueryServiceContext(t *testing.T) { }) } +func TestValidateSearchRequestAndExplicitScope(t *testing.T) { + for _, contentType := range []string{"", "text", "code", "tool", "reasoning"} { + request := SearchRequest{Query: "needle", Options: models.SearchOptions{ContentType: contentType}} + if err := ValidateSearchRequest(request); err != nil { + t.Errorf("content type %q: %v", contentType, err) + } + } + + tests := []struct { + name string + request SearchRequest + want string + }{ + { + name: "missing query", + want: "search query required (use --text or positional argument)", + }, + { + name: "invalid content type", + request: SearchRequest{Query: "needle", Options: models.SearchOptions{ContentType: "audio"}}, + want: "invalid --content-type \"audio\"; must be one of: text, code, tool, reasoning", + }, + { + name: "invalid relaxation query", + request: SearchRequest{Query: `"unclosed`, Relax: true}, + want: "invalid --relax query", + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + err := ValidateSearchRequest(test.request) + if err == nil || !strings.Contains(err.Error(), test.want) { + t.Fatalf("error = %v, want substring %q", err, test.want) + } + }) + } + + for _, request := range []SearchRequest{ + {Options: models.SearchOptions{Project: "alpha"}}, + {Options: models.SearchOptions{AllProjects: true}}, + } { + if err := ValidateExplicitSearchScope(request); err != nil { + t.Errorf("explicit scope %+v: %v", request.Options, err) + } + } + for _, request := range []SearchRequest{ + {}, + {Options: models.SearchOptions{Project: "alpha", AllProjects: true}}, + } { + if err := ValidateExplicitSearchScope(request); err == nil { + t.Errorf("ambiguous or missing scope %+v was accepted", request.Options) + } + } +} + +func TestValidateContextRequestSelectorsAndLimits(t *testing.T) { + uuid := "opaque" + path := "/session" + ordinal := int64(-9) + for _, request := range []ContextRequest{ + {UUID: &uuid, Before: 0, After: 50}, + {SourcePath: &path, Ordinal: &ordinal, Before: 50, After: 0}, + } { + if err := ValidateContextRequest(request); err != nil { + t.Errorf("valid request %+v: %v", request, err) + } + } + + empty := "" + tests := []struct { + name string + request ContextRequest + want string + }{ + {name: "no selector", want: "provide exactly one anchor selector"}, + {name: "both selectors", request: ContextRequest{UUID: &uuid, SourcePath: &path, Ordinal: &ordinal}, want: "provide exactly one anchor selector"}, + {name: "path without ordinal", request: ContextRequest{SourcePath: &path}, want: "provide exactly one anchor selector"}, + {name: "ordinal without path", request: ContextRequest{Ordinal: &ordinal}, want: "provide exactly one anchor selector"}, + {name: "empty UUID", request: ContextRequest{UUID: &empty}, want: "--uuid must not be empty"}, + {name: "empty path", request: ContextRequest{SourcePath: &empty, Ordinal: &ordinal}, want: "--source-path must not be empty"}, + {name: "before low", request: ContextRequest{UUID: &uuid, Before: -1}, want: "--before must be between 0 and 50"}, + {name: "before high", request: ContextRequest{UUID: &uuid, Before: 51}, want: "--before must be between 0 and 50"}, + {name: "after low", request: ContextRequest{UUID: &uuid, After: -1}, want: "--after must be between 0 and 50"}, + {name: "after high", request: ContextRequest{UUID: &uuid, After: 51}, want: "--after must be between 0 and 50"}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + err := ValidateContextRequest(test.request) + if err == nil || err.Error() != test.want && !strings.Contains(err.Error(), test.want) { + t.Fatalf("error = %v, want %q", err, test.want) + } + }) + } +} + +func TestQueryServiceRejectsInvalidRequestsWithoutDatabaseAccess(t *testing.T) { + service := QueryService{} + if _, err := service.Search(context.Background(), SearchRequest{}); err == nil || !strings.Contains(err.Error(), "search query required") { + t.Fatalf("search error = %v", err) + } + if _, err := service.Context(context.Background(), ContextRequest{}); err == nil || !strings.Contains(err.Error(), "exactly one anchor selector") { + t.Fatalf("context error = %v", err) + } +} + +func TestQueryServiceChecksCancellationWithoutDatabaseAccess(t *testing.T) { + service := QueryService{} + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + if _, err := service.Search(ctx, SearchRequest{Query: "needle"}); !errors.Is(err, context.Canceled) { + t.Fatalf("search error = %v, want context.Canceled", err) + } + uuid := "opaque" + if _, err := service.Context(ctx, ContextRequest{UUID: &uuid}); !errors.Is(err, context.Canceled) { + t.Fatalf("context error = %v, want context.Canceled", err) + } +} + func TestQueryServiceStatus(t *testing.T) { service := newQueryServiceTestFixture(t) response, err := service.Status(context.Background(), StatusRequest{}) From 452606391d07f83a1d0a8a146b5d98d29b88b682 Mon Sep 17 00:00:00 2001 From: Pablo Ontiveros Date: Wed, 7 Oct 2026 10:18:20 -0600 Subject: [PATCH 2/2] fix(services): validate query bounds --- AGENTS.md | 2 +- cmd/backscroll/context.go | 15 +- cmd/backscroll/context_startup_test.go | 3 +- cmd/backscroll/context_test.go | 71 ++++-- cmd/backscroll/search.go | 8 +- cmd/backscroll/search_test.go | 156 ++++++++++++-- internal/services/query.go | 33 ++- internal/services/query_test.go | 287 +++++++++++++++++++------ 8 files changed, 457 insertions(+), 118 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 00a9efc3..a647cf6d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -159,7 +159,7 @@ External knowledge sources are configured with active `*.inputs.toml` manifests - **B2 project fallback identity + historical re-resolution**: when the global registry (`~/.config/backscroll/projects.toml`) does not match a session's cwd, `projects.Identify()` derives a sanitized fallback id from the cwd basename (lowercase, `[a-z0-9-_]`; registry always wins when it matches). `rebuild` runs a re-resolution pass: `ReresolveProjects` decodes Claude's session-dir encoding (dashes-for-slashes; if the decoded path exists on disk its basename wins, else the last dash segment — the encoding is lossy and this heuristic is documented as ambiguous for dir names containing dashes) and relabels rows stuck at `project='unknown'`, returning distinct files resolved. Fallback labels are not revisited once set — followed by registry-aware re-resolution below. - **B2.1 registry-aware re-resolution**: `projects.Identify()` now carries a `FromRegistry` flag to distinguish registry matches from fallback labels. `rebuild` includes a second phase that loads `~/.config/backscroll/projects.toml` and re-resolves historical sessions labeled with fallback IDs. For each session path, it decodes the cwd, applies cross-host normalization, and calls `Identify` with the registry. If a registry entry matches (FromRegistry=true) and differs from the stored fallback, the rows are updated. Only registry matches count — fallback-only paths are not re-labeled (no churn). This allows a future registry entry to correct historical misattribution without touching already-correct sessions. Wired into rebuild via `ReresolveProjectsWithRegistry` (queries.go). - **B3 retroactive mining over stored text**: for sessions that expire from disk before Template/Correction/Sequence mining runs, `BackfillDerived()` recovers templates, correction signals, and lossy tool_events from stored text in search_items. Stale template re-mining (F2a) is integrated: BackfillDerived first discovers stale paths via `StaleTemplatePaths(CurrentNormalizationVersion)` and re-mines templates under v2 heuristics, updating existing templates to normalization_version=2. Template mining reuses `internal/templates/Miner` over tool-text rows. Correction detection filters input to prose only (role='user' AND content_type IN ('text','code')) to avoid tool_result false positives; lexicon, denial, and rephrase detectors run on prose; interrupt detector runs on all user messages. Lossy tool_events reverse-parse toolfmt input serialization ( ...; heuristic: first token has no '=', at least one subsequent has '=') to extract tool_name and command_head; outputs unattributable without tool_use_id so skipped; extraction_version=0 marks lossy rows (inputs only, unrecoverable once disk source expires). `rebuild` command now runs after mutation-class startup sync and then: (1) re-derives FTS from DB, (2) BackfillDerived (stale-path discovery + re-mining + expired-file discovery + batch mining + progress reporting), (3) re-resolves projects, and (4) applies registry-aware re-resolution. It performs no second sync. The recover path retains the remediation lock through post-install sync. -- **Early input validation**: CLI commands validate positional arguments and semantic flag values through Cobra `Args`, which runs before the root `PersistentPreRunE`; the root also checks required flags and flag groups before invoking the startup policy for the command class. Invalid invocations therefore fail before opening, creating, synchronizing, or recovering the database. `internal/services` owns pure query validation as the shared CLI/MCP boundary. Direct service calls validate before database access. +- **Early input validation**: CLI commands validate positional arguments and semantic flag values through Cobra `Args`, which runs before the root `PersistentPreRunE`; the root also checks required flags and flag groups before invoking the startup policy for the command class. Invalid invocations therefore fail before opening, creating, synchronizing, or recovering the database. `internal/services` owns pure query validation as the shared CLI/MCP boundary. Direct service calls validate before database access. Search accepts limit 0 for the historical internal default of 100, or an explicit limit from 1 through 300. Context accepts max_tokens from 64 through 16384 and uses 2000 by default. - **Coverage gate**: CI enforces ≥85% aggregate statement coverage via `go test ./... -race -coverprofile`. Local check: `bash scripts/check-coverage.sh`. Tests that depend on local machine state (e.g. `~/.config/backscroll/projects.toml`) must use `t.Setenv("HOME", tempDir)` to stay reproducible on CI. Likewise, `InputsDir` branches requiring `BACKSCROLL_CONFIG_DIR` to be unset must set it to `""` via `t.Setenv`. To test the `Validate` orphan path, insert directly into `search_items` without a matching `indexed_files` row. - **Zero-result guidance**: when `search`/`list` return no rows, actionable suggestions (`--all-projects`, `--content-type tool`, `backscroll status`) are printed to STDERR — never STDOUT, so `--json` stays a clean empty payload. - **Search robot output contract**: robot mode on search emits `result_N_field=value` lines exactly once-wrapped (the robot path writes lines directly; passing pre-formatted lines through the picokit formatter double-wraps them as `result_N=result_N_field=...`). Search robot string values escape backslash as `\\`, carriage return as `\r`, and newline as `\n`. Minimal fields use bounded snippets; full fields retain indexed content. Positive `--max-tokens` budgets count the complete escaped payload with one Picokit estimate, including omission metadata; never sum independently rounded line/result estimates. Truncation removes whole trailing results, and emits no output when even the omission record cannot fit. diff --git a/cmd/backscroll/context.go b/cmd/backscroll/context.go index 718ad594..d37d778b 100644 --- a/cmd/backscroll/context.go +++ b/cmd/backscroll/context.go @@ -21,9 +21,9 @@ import ( const ( contextDefaultWindow = 5 contextMaxTextRunes = 4000 - contextDefaultMaxTokens = 2000 - contextMinMaxTokens = 64 - contextMaxMaxTokens = 16384 + contextDefaultMaxTokens = services.DefaultContextMaxTokens + contextMinMaxTokens = services.MinContextMaxTokens + contextMaxMaxTokens = services.MaxContextMaxTokens ) type contextCommandOptions struct { @@ -123,9 +123,6 @@ func validateContextRequest(opts contextCommandOptions) error { if err := services.ValidateContextRequest(contextServiceRequest(opts)); err != nil { return err } - if opts.maxTokens < contextMinMaxTokens || opts.maxTokens > contextMaxMaxTokens { - return fmt.Errorf("--max-tokens must be between %d and %d", contextMinMaxTokens, contextMaxMaxTokens) - } if opts.jsonFormat && opts.robotFormat { return fmt.Errorf("--json and --robot are mutually exclusive") } @@ -133,7 +130,7 @@ func validateContextRequest(opts contextCommandOptions) error { } func contextServiceRequest(opts contextCommandOptions) services.ContextRequest { - request := services.ContextRequest{Before: opts.before, After: opts.after} + request := services.ContextRequest{Before: opts.before, After: opts.after, MaxTokens: opts.maxTokens} if opts.uuidSet { request.UUID = &opts.uuid } @@ -180,13 +177,13 @@ func runContextCommand(ctx context.Context, stdout, stderr io.Writer, cfg *confi } else if opts.robotFormat { format = contextRobotFormat } - payload, successfulPayloadFits, err := contextSuccessfulPayloadWithinBudget(response.Records, format, opts.maxTokens) + payload, successfulPayloadFits, err := contextSuccessfulPayloadWithinBudget(response.Records, format, request.MaxTokens) if err != nil { return fmt.Errorf("format context records: %w", err) } if !successfulPayloadFits { return writeContextDiagnostic(stdout, stderr, "context_budget_too_small", - fmt.Sprintf("--max-tokens %d cannot fit the anchor and required metadata", opts.maxTokens), opts) + fmt.Sprintf("--max-tokens %d cannot fit the anchor and required metadata", request.MaxTokens), opts) } if _, err := stdout.Write(payload); err != nil { return fmt.Errorf("write context records: %w", err) diff --git a/cmd/backscroll/context_startup_test.go b/cmd/backscroll/context_startup_test.go index 15c33f7b..8c7df813 100644 --- a/cmd/backscroll/context_startup_test.go +++ b/cmd/backscroll/context_startup_test.go @@ -212,7 +212,8 @@ func TestInvalidRootContextDoesNotCreateDatabaseOrStartupSidecar(t *testing.T) { {name: "empty path", args: []string{"context", "--source-path=", "--ordinal", "1"}}, {name: "invalid before", args: []string{"context", "--uuid", "u", "--before", "-1"}}, {name: "invalid after", args: []string{"context", "--uuid", "u", "--after", "51"}}, - {name: "invalid token budget", args: []string{"context", "--uuid", "u", "--max-tokens", "63"}}, + {name: "token budget below minimum", args: []string{"context", "--uuid", "u", "--max-tokens", "63"}}, + {name: "token budget above maximum", args: []string{"context", "--uuid", "u", "--max-tokens", "16385"}}, {name: "conflicting formats", args: []string{"context", "--uuid", "u", "--json", "--robot"}}, } diff --git a/cmd/backscroll/context_test.go b/cmd/backscroll/context_test.go index 69b4963d..ec6e2ade 100644 --- a/cmd/backscroll/context_test.go +++ b/cmd/backscroll/context_test.go @@ -12,7 +12,6 @@ import ( "github.com/pablontiv/backscroll/internal/config" "github.com/pablontiv/backscroll/internal/models" - "github.com/pablontiv/backscroll/internal/services" "github.com/pablontiv/backscroll/internal/storage" ) @@ -279,27 +278,69 @@ func TestContextTextAndRobotSuccessfulPayloadBudgetBoundaries(t *testing.T) { } } -func TestContextValidationMatchesServices(t *testing.T) { +func TestContextValidationGoldenErrorsAndOrder(t *testing.T) { tests := []struct { name string opts contextCommandOptions + want string }{ - {name: "no selector", opts: contextCommandOptions{maxTokens: contextDefaultMaxTokens}}, - {name: "both selectors", opts: contextCommandOptions{uuid: "u", uuidSet: true, sourcePath: "/x", sourcePathSet: true, ordinalSet: true, maxTokens: contextDefaultMaxTokens}}, - {name: "empty UUID", opts: contextCommandOptions{uuidSet: true, maxTokens: contextDefaultMaxTokens}}, - {name: "before high", opts: contextCommandOptions{uuid: "u", uuidSet: true, before: 51, maxTokens: contextDefaultMaxTokens}}, - {name: "valid", opts: contextCommandOptions{uuid: "u", uuidSet: true, before: 0, after: 50, maxTokens: contextDefaultMaxTokens}}, + { + name: "selector wins", + opts: contextCommandOptions{before: -1, after: -1, maxTokens: 63}, + want: "provide exactly one anchor selector: --uuid or --source-path with --ordinal", + }, + { + name: "ambiguous selector", + opts: contextCommandOptions{uuid: "u", uuidSet: true, sourcePath: "/x", sourcePathSet: true, ordinalSet: true, maxTokens: contextDefaultMaxTokens}, + want: "provide exactly one anchor selector: --uuid or --source-path with --ordinal", + }, + { + name: "empty UUID wins", + opts: contextCommandOptions{uuidSet: true, before: -1, after: -1, maxTokens: 63}, + want: "--uuid must not be empty", + }, + { + name: "empty path wins", + opts: contextCommandOptions{sourcePathSet: true, ordinalSet: true, before: -1, after: -1, maxTokens: 63}, + want: "--source-path must not be empty", + }, + { + name: "before wins", + opts: contextCommandOptions{uuid: "u", uuidSet: true, before: 51, after: -1, maxTokens: 63}, + want: "--before must be between 0 and 50", + }, + { + name: "after wins", + opts: contextCommandOptions{uuid: "u", uuidSet: true, after: 51, maxTokens: 63}, + want: "--after must be between 0 and 50", + }, + { + name: "max tokens low", + opts: contextCommandOptions{uuid: "u", uuidSet: true, maxTokens: 63}, + want: "--max-tokens must be between 64 and 16384", + }, + { + name: "max tokens high", + opts: contextCommandOptions{uuid: "u", uuidSet: true, maxTokens: 16385}, + want: "--max-tokens must be between 64 and 16384", + }, + { + name: "format conflict follows shared validation", + opts: contextCommandOptions{uuid: "u", uuidSet: true, maxTokens: contextDefaultMaxTokens, jsonFormat: true, robotFormat: true}, + want: "--json and --robot are mutually exclusive", + }, + { + name: "minimum bounds", + opts: contextCommandOptions{uuid: "u", uuidSet: true, before: 0, after: 0, maxTokens: 64}, + }, + { + name: "maximum bounds", + opts: contextCommandOptions{sourcePath: "/x", sourcePathSet: true, ordinalSet: true, before: 50, after: 50, maxTokens: 16384}, + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - cliErr := validateContextRequest(test.opts) - serviceErr := services.ValidateContextRequest(contextServiceRequest(test.opts)) - if (cliErr == nil) != (serviceErr == nil) { - t.Fatalf("CLI error = %v, service error = %v", cliErr, serviceErr) - } - if cliErr != nil && cliErr.Error() != serviceErr.Error() { - t.Fatalf("CLI error = %q, service error = %q", cliErr, serviceErr) - } + assertCLIValidationError(t, validateContextRequest(test.opts), test.want) }) } } diff --git a/cmd/backscroll/search.go b/cmd/backscroll/search.go index 711edb37..e1fa1acf 100644 --- a/cmd/backscroll/search.go +++ b/cmd/backscroll/search.go @@ -66,7 +66,7 @@ protected. All scope filters remain fixed; no OR or semantic expansion is used.` Args: func(cmd *cobra.Command, args []string) error { return validateCommandBeforeStartup(cmd, args, cobra.MaximumNArgs(1), func() error { query := searchQuery(text, args) - _, _, err := validateAndParseSearchRequest(query, fields, contentType, after, before, relax) + _, _, err := validateAndParseSearchRequest(query, fields, contentType, after, before, limit, offset, relax) if err != nil { return err } @@ -119,7 +119,7 @@ func searchQuery(text string, args []string) string { return text } -func validateAndParseSearchRequest(query, fields, contentType, after, before string, relax bool) (*time.Time, *time.Time, error) { +func validateAndParseSearchRequest(query, fields, contentType, after, before string, limit, offset int, relax bool) (*time.Time, *time.Time, error) { request := services.SearchRequest{Query: query} if err := services.ValidateSearchRequest(request); err != nil { return nil, nil, err @@ -151,6 +151,8 @@ func validateAndParseSearchRequest(query, fields, contentType, after, before str request.Options.After = afterTime request.Options.Before = beforeTime + request.Options.Limit = limit + request.Options.Offset = offset request.Relax = relax if err := services.ValidateSearchRequest(request); err != nil { return nil, nil, err @@ -166,7 +168,7 @@ func runSearch(ctx context.Context, stdout, stderr io.Writer, cfg *config.Config fields string, maxTokens int, lexicalOnly bool, similarityThreshold float64, relax bool) (retErr error) { - afterTime, beforeTime, err := validateAndParseSearchRequest(query, fields, contentType, after, before, relax) + afterTime, beforeTime, err := validateAndParseSearchRequest(query, fields, contentType, after, before, limit, offset, relax) if err != nil { return err } diff --git a/cmd/backscroll/search_test.go b/cmd/backscroll/search_test.go index 233d1062..a31c99e4 100644 --- a/cmd/backscroll/search_test.go +++ b/cmd/backscroll/search_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "os" "path/filepath" "strings" "testing" @@ -11,41 +12,158 @@ import ( picokitoutput "github.com/pablontiv/picokit/output" "github.com/pablontiv/backscroll/internal/models" - "github.com/pablontiv/backscroll/internal/services" ) -func TestSearchValidationMatchesServices(t *testing.T) { +func TestSearchValidationGoldenErrorsAndOrder(t *testing.T) { tests := []struct { name string query string + fields string contentType string + after string + before string + limit int + offset int relax bool + want string }{ - {name: "missing query"}, - {name: "invalid content type", query: "needle", contentType: "audio"}, - {name: "invalid relaxation query", query: `"unclosed`, relax: true}, - {name: "valid", query: "needle", contentType: "text"}, + { + name: "missing query wins", + fields: "invalid", + limit: -1, + want: "search query required (use --text or positional argument)", + }, + { + name: "fields win", + query: "needle", + fields: "invalid", + contentType: "audio", + limit: -1, + want: "invalid --fields value \"invalid\": must be minimal or full", + }, + { + name: "content type wins", + query: "needle", + fields: "minimal", + contentType: "audio", + after: "bad", + limit: -1, + want: "invalid --content-type \"audio\"; must be one of: text, code, tool, reasoning", + }, + { + name: "after date wins", + query: "needle", + fields: "minimal", + after: "bad", + limit: -1, + want: "parse --after date: parsing time \"bad\" as \"2006-01-02\": cannot parse \"bad\" as \"2006\"", + }, + { + name: "before date wins", + query: "needle", + fields: "minimal", + before: "bad", + limit: -1, + want: "parse --before date: parsing time \"bad\" as \"2006-01-02\": cannot parse \"bad\" as \"2006\"", + }, + { + name: "negative limit wins", + query: `"unclosed`, + fields: "minimal", + limit: -1, + offset: -1, + relax: true, + want: "--limit must be between 0 and 300", + }, + { + name: "limit over maximum", + query: "needle", + fields: "minimal", + limit: 301, + want: "--limit must be between 0 and 300", + }, + { + name: "negative offset wins", + query: `"unclosed`, + fields: "minimal", + limit: 20, + offset: -1, + relax: true, + want: "--offset must be >= 0", + }, + { + name: "relaxation syntax", + query: `"unclosed`, + fields: "minimal", + limit: 20, + relax: true, + want: "invalid --relax query: quoted phrases must be closed and whitespace-delimited", + }, + { + name: "zero limit keeps internal default", + query: "needle", + fields: "minimal", + }, + { + name: "maximum limit", + query: "needle", + fields: "full", + limit: 300, + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - _, _, cliErr := validateAndParseSearchRequest(test.query, "minimal", test.contentType, "", "", test.relax) - serviceErr := services.ValidateSearchRequest(services.SearchRequest{ - Query: test.query, - Options: models.SearchOptions{ - ContentType: test.contentType, - }, - Relax: test.relax, - }) - if (cliErr == nil) != (serviceErr == nil) { - t.Fatalf("CLI error = %v, service error = %v", cliErr, serviceErr) - } - if cliErr != nil && cliErr.Error() != serviceErr.Error() { - t.Fatalf("CLI error = %q, service error = %q", cliErr, serviceErr) + _, _, err := validateAndParseSearchRequest( + test.query, test.fields, test.contentType, test.after, test.before, + test.limit, test.offset, test.relax, + ) + assertCLIValidationError(t, err, test.want) + }) + } +} + +func TestSearchInvalidBoundsDoNotCreateDatabaseOrStartupSidecar(t *testing.T) { + tests := []struct { + name string + args []string + want string + }{ + {name: "negative limit", args: []string{"search", "needle", "--all-projects", "--limit", "-1"}, want: "--limit must be between 0 and 300"}, + {name: "limit over maximum", args: []string{"search", "needle", "--all-projects", "--limit", "301"}, want: "--limit must be between 0 and 300"}, + {name: "negative offset", args: []string{"search", "needle", "--all-projects", "--offset", "-1"}, want: "--offset must be >= 0"}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + testEnv(t) + dbPath := filepath.Join(t.TempDir(), "must-not-exist.db") + t.Setenv("BACKSCROLL_DATABASE_PATH", dbPath) + _, _, err := runCmd(test.args...) + assertCLIValidationError(t, err, test.want) + for _, path := range []string{dbPath, dbPath + ".startup-sync.lock"} { + if _, err := os.Lstat(path); !os.IsNotExist(err) { + t.Fatalf("invalid search created %s: stat error=%v", path, err) + } } }) } } +func assertCLIValidationError(t *testing.T, err error, want string) { + t.Helper() + if want == "" { + if err != nil { + t.Fatalf("error = %q, want nil", err) + } + return + } + if err == nil { + t.Fatalf("error = nil, want %q", want) + } + if err.Error() != want { + t.Fatalf("error = %q, want %q", err, want) + } +} + func TestSearchOutputFormatText(t *testing.T) { _, cleanup := testEnv(t) defer cleanup() diff --git a/internal/services/query.go b/internal/services/query.go index dbe66d5d..e4122000 100644 --- a/internal/services/query.go +++ b/internal/services/query.go @@ -9,7 +9,18 @@ import ( "github.com/pablontiv/backscroll/internal/storage" ) -const maxContextWindow = 50 +const ( + maxSearchLimit = 300 + + maxContextWindow = 50 + + // DefaultContextMaxTokens is the current context payload default. + DefaultContextMaxTokens = 2000 + // MinContextMaxTokens is the smallest accepted context payload budget. + MinContextMaxTokens = 64 + // MaxContextMaxTokens is the largest accepted context payload budget. + MaxContextMaxTokens = 16384 +) // QueryService coordinates read-only queries against the perennial index. type QueryService struct { @@ -46,15 +57,27 @@ func ValidateSearchRequest(request SearchRequest) error { return fmt.Errorf("invalid --content-type %q; must be one of: text, code, tool, reasoning", request.Options.ContentType) } + // Limit 0 keeps the storage layer's historical internal default of 100. + // The CLI supplies its separate historical default of 20. + if request.Options.Limit < 0 || request.Options.Limit > maxSearchLimit { + return fmt.Errorf("--limit must be between 0 and %d", maxSearchLimit) + } + if request.Options.Offset < 0 { + return fmt.Errorf("--offset must be >= 0") + } + if request.Relax { return storage.ValidateRelaxationQuery(request.Query) } return nil } -// ValidateExplicitSearchScope requires one project or all projects. -// Remote consumers use this check in addition to ValidateSearchRequest. +// ValidateExplicitSearchScope validates the base request and then requires one project scope. +// Remote consumers use this composed validator. The CLI keeps project inference before service use. func ValidateExplicitSearchScope(request SearchRequest) error { + if err := ValidateSearchRequest(request); err != nil { + return err + } hasProject := request.Options.Project != "" if hasProject == request.Options.AllProjects { return fmt.Errorf("provide exactly one project scope: project or all_projects=true") @@ -92,6 +115,7 @@ type ContextRequest struct { Ordinal *int64 Before int After int + MaxTokens int } // ContextResponse contains the anchor-relative indexed record window. @@ -118,6 +142,9 @@ func ValidateContextRequest(request ContextRequest) error { if request.After < 0 || request.After > maxContextWindow { return fmt.Errorf("--after must be between 0 and %d", maxContextWindow) } + if request.MaxTokens < MinContextMaxTokens || request.MaxTokens > MaxContextMaxTokens { + return fmt.Errorf("--max-tokens must be between %d and %d", MinContextMaxTokens, MaxContextMaxTokens) + } return nil } diff --git a/internal/services/query_test.go b/internal/services/query_test.go index 81ea656e..c177615e 100644 --- a/internal/services/query_test.go +++ b/internal/services/query_test.go @@ -5,7 +5,6 @@ import ( "errors" "path/filepath" "reflect" - "strings" "testing" "github.com/pablontiv/backscroll/internal/models" @@ -68,9 +67,10 @@ func TestQueryServiceContext(t *testing.T) { t.Run("UUID selector", func(t *testing.T) { uuid := "anchor" response, err := service.Context(context.Background(), ContextRequest{ - UUID: &uuid, - Before: 1, - After: 1, + UUID: &uuid, + Before: 1, + After: 1, + MaxTokens: DefaultContextMaxTokens, }) if err != nil { t.Fatal(err) @@ -86,6 +86,7 @@ func TestQueryServiceContext(t *testing.T) { Ordinal: &ordinal, Before: 1, After: 1, + MaxTokens: DefaultContextMaxTokens, }) if err != nil { t.Fatal(err) @@ -94,108 +95,260 @@ func TestQueryServiceContext(t *testing.T) { }) } -func TestValidateSearchRequestAndExplicitScope(t *testing.T) { - for _, contentType := range []string{"", "text", "code", "tool", "reasoning"} { - request := SearchRequest{Query: "needle", Options: models.SearchOptions{ContentType: contentType}} - if err := ValidateSearchRequest(request); err != nil { - t.Errorf("content type %q: %v", contentType, err) - } - } - +func TestValidateSearchRequestExactErrorsAndOrder(t *testing.T) { tests := []struct { name string request SearchRequest want string }{ { - name: "missing query", + name: "zero limit keeps internal default", + request: SearchRequest{ + Query: "needle", + }, + }, + { + name: "maximum limit", + request: SearchRequest{ + Query: "needle", + Options: models.SearchOptions{Limit: 300}, + }, + }, + { + name: "missing query wins", + request: SearchRequest{ + Options: models.SearchOptions{ContentType: "audio", Limit: -1, Offset: -1}, + Relax: true, + }, want: "search query required (use --text or positional argument)", }, { - name: "invalid content type", - request: SearchRequest{Query: "needle", Options: models.SearchOptions{ContentType: "audio"}}, - want: "invalid --content-type \"audio\"; must be one of: text, code, tool, reasoning", + name: "content type wins", + request: SearchRequest{ + Query: "needle", + Options: models.SearchOptions{ContentType: "audio", Limit: -1, Offset: -1}, + }, + want: "invalid --content-type \"audio\"; must be one of: text, code, tool, reasoning", + }, + { + name: "negative limit wins", + request: SearchRequest{Query: `"unclosed`, Options: models.SearchOptions{Limit: -1, Offset: -1}, Relax: true}, + want: "--limit must be between 0 and 300", + }, + { + name: "limit over maximum wins", + request: SearchRequest{Query: "needle", Options: models.SearchOptions{Limit: 301, Offset: -1}}, + want: "--limit must be between 0 and 300", + }, + { + name: "negative offset wins", + request: SearchRequest{Query: `"unclosed`, Options: models.SearchOptions{Limit: 20, Offset: -1}, Relax: true}, + want: "--offset must be >= 0", }, { - name: "invalid relaxation query", - request: SearchRequest{Query: `"unclosed`, Relax: true}, - want: "invalid --relax query", + name: "relaxation syntax", + request: SearchRequest{Query: `"unclosed`, Options: models.SearchOptions{Limit: 20}, Relax: true}, + want: "invalid --relax query: quoted phrases must be closed and whitespace-delimited", }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - err := ValidateSearchRequest(test.request) - if err == nil || !strings.Contains(err.Error(), test.want) { - t.Fatalf("error = %v, want substring %q", err, test.want) - } + assertExactValidationError(t, ValidateSearchRequest(test.request), test.want) }) } +} - for _, request := range []SearchRequest{ - {Options: models.SearchOptions{Project: "alpha"}}, - {Options: models.SearchOptions{AllProjects: true}}, - } { - if err := ValidateExplicitSearchScope(request); err != nil { - t.Errorf("explicit scope %+v: %v", request.Options, err) - } +func TestValidateExplicitSearchScopeComposesBaseValidation(t *testing.T) { + tests := []struct { + name string + request SearchRequest + want string + }{ + { + name: "project", + request: SearchRequest{ + Query: "needle", + Options: models.SearchOptions{Project: "alpha", Limit: 20}, + }, + }, + { + name: "all projects", + request: SearchRequest{ + Query: "needle", + Options: models.SearchOptions{AllProjects: true, Limit: 20}, + }, + }, + { + name: "base validation first", + request: SearchRequest{ + Options: models.SearchOptions{Limit: -1}, + }, + want: "search query required (use --text or positional argument)", + }, + { + name: "base bounds before scope", + request: SearchRequest{ + Query: "needle", + Options: models.SearchOptions{Limit: 301}, + }, + want: "--limit must be between 0 and 300", + }, + { + name: "missing scope", + request: SearchRequest{Query: "needle", Options: models.SearchOptions{Limit: 20}}, + want: "provide exactly one project scope: project or all_projects=true", + }, + { + name: "ambiguous scope", + request: SearchRequest{ + Query: "needle", + Options: models.SearchOptions{Project: "alpha", AllProjects: true, Limit: 20}, + }, + want: "provide exactly one project scope: project or all_projects=true", + }, } - for _, request := range []SearchRequest{ - {}, - {Options: models.SearchOptions{Project: "alpha", AllProjects: true}}, - } { - if err := ValidateExplicitSearchScope(request); err == nil { - t.Errorf("ambiguous or missing scope %+v was accepted", request.Options) - } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + assertExactValidationError(t, ValidateExplicitSearchScope(test.request), test.want) + }) } } -func TestValidateContextRequestSelectorsAndLimits(t *testing.T) { +func TestValidateContextRequestExactErrorsAndOrder(t *testing.T) { + if DefaultContextMaxTokens != 2000 { + t.Fatalf("DefaultContextMaxTokens = %d, want 2000", DefaultContextMaxTokens) + } + uuid := "opaque" path := "/session" ordinal := int64(-9) - for _, request := range []ContextRequest{ - {UUID: &uuid, Before: 0, After: 50}, - {SourcePath: &path, Ordinal: &ordinal, Before: 50, After: 0}, - } { - if err := ValidateContextRequest(request); err != nil { - t.Errorf("valid request %+v: %v", request, err) - } - } - empty := "" tests := []struct { name string request ContextRequest want string }{ - {name: "no selector", want: "provide exactly one anchor selector"}, - {name: "both selectors", request: ContextRequest{UUID: &uuid, SourcePath: &path, Ordinal: &ordinal}, want: "provide exactly one anchor selector"}, - {name: "path without ordinal", request: ContextRequest{SourcePath: &path}, want: "provide exactly one anchor selector"}, - {name: "ordinal without path", request: ContextRequest{Ordinal: &ordinal}, want: "provide exactly one anchor selector"}, - {name: "empty UUID", request: ContextRequest{UUID: &empty}, want: "--uuid must not be empty"}, - {name: "empty path", request: ContextRequest{SourcePath: &empty, Ordinal: &ordinal}, want: "--source-path must not be empty"}, - {name: "before low", request: ContextRequest{UUID: &uuid, Before: -1}, want: "--before must be between 0 and 50"}, - {name: "before high", request: ContextRequest{UUID: &uuid, Before: 51}, want: "--before must be between 0 and 50"}, - {name: "after low", request: ContextRequest{UUID: &uuid, After: -1}, want: "--after must be between 0 and 50"}, - {name: "after high", request: ContextRequest{UUID: &uuid, After: 51}, want: "--after must be between 0 and 50"}, + { + name: "current default", + request: ContextRequest{UUID: &uuid, MaxTokens: DefaultContextMaxTokens}, + }, + { + name: "minimum bounds", + request: ContextRequest{UUID: &uuid, Before: 0, After: 0, MaxTokens: 64}, + }, + { + name: "maximum bounds", + request: ContextRequest{SourcePath: &path, Ordinal: &ordinal, Before: 50, After: 50, MaxTokens: 16384}, + }, + { + name: "selector wins", + request: ContextRequest{Before: -1, After: -1, MaxTokens: 63}, + want: "provide exactly one anchor selector: --uuid or --source-path with --ordinal", + }, + { + name: "ambiguous selector", + request: ContextRequest{UUID: &uuid, SourcePath: &path, Ordinal: &ordinal, MaxTokens: DefaultContextMaxTokens}, + want: "provide exactly one anchor selector: --uuid or --source-path with --ordinal", + }, + { + name: "empty UUID wins", + request: ContextRequest{UUID: &empty, Before: -1, After: -1, MaxTokens: 63}, + want: "--uuid must not be empty", + }, + { + name: "empty path wins", + request: ContextRequest{SourcePath: &empty, Ordinal: &ordinal, Before: -1, After: -1, MaxTokens: 63}, + want: "--source-path must not be empty", + }, + { + name: "before wins", + request: ContextRequest{UUID: &uuid, Before: 51, After: -1, MaxTokens: 63}, + want: "--before must be between 0 and 50", + }, + { + name: "after wins", + request: ContextRequest{UUID: &uuid, After: 51, MaxTokens: 63}, + want: "--after must be between 0 and 50", + }, + { + name: "max tokens low", + request: ContextRequest{UUID: &uuid, MaxTokens: 63}, + want: "--max-tokens must be between 64 and 16384", + }, + { + name: "max tokens high", + request: ContextRequest{UUID: &uuid, MaxTokens: 16385}, + want: "--max-tokens must be between 64 and 16384", + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - err := ValidateContextRequest(test.request) - if err == nil || err.Error() != test.want && !strings.Contains(err.Error(), test.want) { - t.Fatalf("error = %v, want %q", err, test.want) - } + assertExactValidationError(t, ValidateContextRequest(test.request), test.want) }) } } func TestQueryServiceRejectsInvalidRequestsWithoutDatabaseAccess(t *testing.T) { service := QueryService{} - if _, err := service.Search(context.Background(), SearchRequest{}); err == nil || !strings.Contains(err.Error(), "search query required") { - t.Fatalf("search error = %v", err) + uuid := "opaque" + tests := []struct { + name string + run func() error + want string + }{ + { + name: "search limit", + run: func() error { + _, err := service.Search(context.Background(), SearchRequest{Query: "needle", Options: models.SearchOptions{Limit: -1}}) + return err + }, + want: "--limit must be between 0 and 300", + }, + { + name: "search limit over maximum", + run: func() error { + _, err := service.Search(context.Background(), SearchRequest{Query: "needle", Options: models.SearchOptions{Limit: 301}}) + return err + }, + want: "--limit must be between 0 and 300", + }, + { + name: "search offset", + run: func() error { + _, err := service.Search(context.Background(), SearchRequest{Query: "needle", Options: models.SearchOptions{Offset: -1}}) + return err + }, + want: "--offset must be >= 0", + }, + { + name: "context max tokens", + run: func() error { + _, err := service.Context(context.Background(), ContextRequest{UUID: &uuid, MaxTokens: 63}) + return err + }, + want: "--max-tokens must be between 64 and 16384", + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + assertExactValidationError(t, test.run(), test.want) + }) + } +} + +func assertExactValidationError(t *testing.T, err error, want string) { + t.Helper() + if want == "" { + if err != nil { + t.Fatalf("error = %q, want nil", err) + } + return + } + if err == nil { + t.Fatalf("error = nil, want %q", want) } - if _, err := service.Context(context.Background(), ContextRequest{}); err == nil || !strings.Contains(err.Error(), "exactly one anchor selector") { - t.Fatalf("context error = %v", err) + if err.Error() != want { + t.Fatalf("error = %q, want %q", err, want) } } @@ -208,7 +361,7 @@ func TestQueryServiceChecksCancellationWithoutDatabaseAccess(t *testing.T) { t.Fatalf("search error = %v, want context.Canceled", err) } uuid := "opaque" - if _, err := service.Context(ctx, ContextRequest{UUID: &uuid}); !errors.Is(err, context.Canceled) { + if _, err := service.Context(ctx, ContextRequest{UUID: &uuid, MaxTokens: DefaultContextMaxTokens}); !errors.Is(err, context.Canceled) { t.Fatalf("context error = %v, want context.Canceled", err) } } @@ -254,7 +407,7 @@ func TestQueryServiceCancellation(t *testing.T) { name: "context", run: func() error { uuid := "anchor" - _, err := service.Context(ctx, ContextRequest{UUID: &uuid}) + _, err := service.Context(ctx, ContextRequest{UUID: &uuid, MaxTokens: DefaultContextMaxTokens}) return err }, },