feat: CI governance, runtime coverage, and agent LLM policy - #5
Merged
Coldaine merged 9 commits intoMay 20, 2026
Merged
Conversation
Add capability/doc drift enforcement, expand adapter and engine test coverage, implement missing Firecrawl/Exa/Tavily capabilities, and introduce operational tooling (`status`, `--dry-run`, usage logging) with synchronized provider and configuration documentation. Made-with: Cursor
…olicy Drop ClaudeClient and api.anthropic.com usage. Agent mode uses OpenAI only. Record session decisions on routing, config, CI gates, and agent vs default CLI. Co-authored-by: Cursor <cursoragent@cursor.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (31)
📝 WalkthroughWalkthroughThis PR implements ColdSearch's OpenAI-only LLM policy, expands provider capabilities (Exa crawl, Firecrawl search), adds JSONL usage logging with per-provider metrics, introduces CLI dry-run and status commands, and provides comprehensive documentation and test coverage across all changes. ChangesOpenAI-Only LLM Policy and Agent Mode
Expanded Provider Capabilities and Documentation
Usage Logging and Operational Observability
Testing Infrastructure and CI Governance
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI User
participant Config as Config/Logger
participant Engine as FanoutEngine
participant Adapter as Provider Adapter
participant API as Provider API
CLI->>Config: load config, resolve capabilities
Config->>Logger: initialize UsageLogger
CLI->>Engine: instantiate with logger & key pool
CLI->>Engine: search(query) or crawl(url)
Engine->>Engine: resolveCapabilityProviders()
loop For each provider (fanout/sequential)
Engine->>Config: getNextKeyWithRef(provider)
Config-->>Engine: {value, ref} from env:/bws:/literal
Engine->>Engine: record startTime
Engine->>Adapter: search/crawl(query/url, apiKey)
Adapter->>API: POST/GET request
API-->>Adapter: response or error
Adapter-->>Engine: results or exception
Engine->>Logger: write({provider, key, success, duration, error})
Logger->>Logger: append to ~/.config/coldsearch/usage.jsonl
end
Engine-->>CLI: aggregated results + providersUsed
CLI->>CLI: render JSON or text output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
3 tasks
codeant-ai
Bot
added
the
size:XXL
There was a problem hiding this comment.
Pull request overview
Consolidates CI governance, runtime/adapter coverage, and agent LLM policy onto one branch. Removes the Anthropic API path from agent mode (OpenAI-only), adds Firecrawl search, Exa crawl, and Tavily native crawl, introduces usage logging, status and --dry-run CLI features, and adds a large test suite plus docs-sync CI job.
Changes:
- Provider/runtime: implement missing capabilities (Firecrawl
search, Exacrawl, Tavily native/crawl); sharedresolveCapabilityProviders;KeyPoolManagerwith safe key refs;UsageLogger(JSONL) wired intoFanoutEngine. - CLI/agent: new
statuscommand and--dry-runflag; agent--llmrestricted toopenai(Anthropic client removed); SSRF guard handles IPv4-mapped IPv6 and::. - CI/docs/tests: new
docs-syncworkflow job andnpm run test:docs; per-provider adoption plans,CONFIGURATION.md, capability matrix/registry/adapter drift tests, fanout/rerank/agent/CLI integration tests, fetch-mock utility.
Reviewed changes
Copilot reviewed 55 out of 55 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| package.json | Adds test:docs and changes test glob to test/**/*.test.mjs |
| .github/workflows/ci.yml | Adds docs-sync job running npm run test:docs |
| src/adapters/{exa,firecrawl,tavily}.ts | Adds Exa crawl, Firecrawl search, Tavily native crawl |
| src/agent/{agent,context,llm}.ts | Removes Anthropic client; OpenAI-only; SSRF tweaks; sources appended to answer |
| src/cli.ts | Adds status, --dry-run, restricts --llm to openai |
| src/config.ts | Falls back to default path when neither config exists |
| src/engine/{fanout,keypool}.ts | Instance-owned key pool with safe key refs; usage logging per call |
| src/logging/usage.ts | New JSONL usage logger and safe key reference helper |
| src/providers.ts | Updates Exa/Firecrawl capabilities; adds shared resolveCapabilityProviders |
| src/types.ts | Adds optional logging.usage config block; updates source field docs |
| config.example.toml | Documents optional [logging.usage] block |
| docs/** | New CONFIGURATION.md, adoption plan template + 6 plans, decision record, NORTH_STAR/architecture/CAPABILITY_MATRIX/PROGRESS updates, per-provider Capabilities/Configuration sections |
| README.md, SKILL.md, CLAUDE.md | Reflect Exa crawl support and OpenAI-only agent policy |
| test/** | New adapter mocks, fanout/rerank/agent/CLI integration/doc-drift tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "dev": "tsc --watch", | ||
| "typecheck": "tsc --noEmit", | ||
| "test": "npm run build && node --test test/*.test.mjs", | ||
| "test": "npm run build && node --test test/**/*.test.mjs", |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| useAutoprompt: false, | ||
| }), | ||
| }, { | ||
| label: "Exa crawl discover", |
There was a problem hiding this comment.
Suggestion: Use a provider-neutral request label here so runtime error messages shown to the agent do not expose provider names. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The code hard-codes the provider name "Exa" into the request label. If the custom rule is to keep agent-facing runtime labels provider-neutral, this is a real violation because the label is not generic.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/adapters/exa.ts
**Line:** 142:142
**Comment:**
*Custom Rule: Use a provider-neutral request label here so runtime error messages shown to the agent do not expose provider names.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| livecrawl_timeout: 10000, | ||
| }), | ||
| }, { | ||
| label: "Exa crawl contents", |
There was a problem hiding this comment.
Suggestion: Rename this request label to a generic, provider-agnostic phrase to avoid leaking provider identity in agent-facing error text. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The request label explicitly includes the provider name "Exa", so it is not provider-agnostic. This matches the suggested rule violation about leaking provider identity in agent-facing text.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/adapters/exa.ts
**Line:** 173:173
**Comment:**
*Custom Rule: Rename this request label to a generic, provider-agnostic phrase to avoid leaking provider identity in agent-facing error text.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| }, | ||
| }), | ||
| }, | ||
| { label: "Firecrawl search" } |
There was a problem hiding this comment.
Suggestion: Replace the provider-specific request label with a provider-neutral label so provider names are not exposed through surfaced request errors in agent-facing flows. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The existing code uses a provider-specific label string, "Firecrawl search". If the custom rule is to avoid exposing provider names in surfaced request labels or errors, this is a real violation present in the current file.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/adapters/firecrawl.ts
**Line:** 59:59
**Comment:**
*Custom Rule: Replace the provider-specific request label with a provider-neutral label so provider names are not exposed through surfaced request errors in agent-facing flows.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| ); | ||
|
|
||
| if (!data.success || data.error) { | ||
| throw new Error(`Firecrawl error: ${data.error || "Unknown error"}`); |
There was a problem hiding this comment.
Suggestion: Change the thrown error text to a provider-neutral message so agent-facing error content does not include a provider name. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The thrown error explicitly includes the provider name "Firecrawl". That matches the stated concern about provider-specific error content, so the violation is present in the existing code.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/adapters/firecrawl.ts
**Line:** 63:63
**Comment:**
*Custom Rule: Change the thrown error text to a provider-neutral message so agent-facing error content does not include a provider name.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fab306c72
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "dev": "tsc --watch", | ||
| "typecheck": "tsc --noEmit", | ||
| "test": "npm run build && node --test test/*.test.mjs", | ||
| "test": "npm run build && node --test test/**/*.test.mjs", |
There was a problem hiding this comment.
The new npm test command uses an unquoted shell glob (test/**/*.test.mjs), which is expanded by the shell before Node runs; in typical CI shells this matches test/*/*.test.mjs but skips root-level suites like test/cli-integration.test.mjs, test/fanout-engine.test.mjs, and test/agent-mode.test.mjs. Because .github/workflows/ci.yml runs npm test, a large part of the intended regression coverage is silently dropped, allowing breakages to merge undetected.
Useful? React with 👍 / 👎.
Comment on lines
+383
to
+397
| const keyPreview = getKeyReference(pool, provider); | ||
|
|
||
| const warnings = []; | ||
| if (keyCount > 0) { | ||
| const first = pool.keys[0]; | ||
| if (first.startsWith("env:")) { | ||
| const varName = first.slice(4); | ||
| if (!process.env[varName]) warnings.push(`missing env var ${varName}`); | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| provider, | ||
| capability, | ||
| key_pool: { count: keyCount, strategy: keyStrategy, preview: keyPreview }, |
There was a problem hiding this comment.
Suggestion: The dry-run plan exposes key_pool.preview using getKeyReference, which returns the raw first key entry; if operators store literal API keys in config, this prints secrets directly to stdout. Redact preview values (or only show key source type/count) instead of returning actual key references. [security]
Severity Level: Critical 🚨
- ❌ Dry-run command can print literal API keys.
- ⚠️ Secrets may leak via terminal history or logs.
- ⚠️ Operator tooling exposes more secret surfaces than config.Steps of Reproduction ✅
1. Configure a provider key pool with a literal API key value in the config file (schema
defined in `src/types.ts:88-103` where `KeyPool.keys: string[]` is not restricted to
env/BWS refs, so operators can put `keys = ["sk-live-123..."]` directly).
2. Run the CLI in dry-run mode for search, e.g. `coldsearch --dry-run "test query"` which
maps to `runFanoutMode()` in `src/cli.ts:248-257`; since `--dry-run` is set,
`runFanoutMode()` calls `buildExecutionPlan("search", options)` and returns early at
`src/cli.ts:248-253`.
3. Inside `buildExecutionPlan()` at `src/cli.ts:117-141`, the code calls
`getKeyReference(pool, provider)` to compute `keyPreview` and includes it in the response
object as `key_pool.preview`.
4. `getKeyReference()` in `src/logging/usage.ts:33-39` simply returns `keyPool.keys[0]`
(the raw configured string), so when the configured entry is a literal API key, the
dry-run JSON printed to stdout by `console.log(formatOutput(plan, options))` at
`src/cli.ts:249-252` exposes the full secret in `key_pool.preview`.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/cli.ts
**Line:** 383:397
**Comment:**
*Security: The dry-run plan exposes `key_pool.preview` using `getKeyReference`, which returns the raw first key entry; if operators store literal API keys in config, this prints secrets directly to stdout. Redact preview values (or only show key source type/count) instead of returning actual key references.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| : usagePath; | ||
|
|
||
| if (resolved && fs.existsSync(resolved)) { | ||
| const lines = fs.readFileSync(resolved, "utf8").split("\n").filter(Boolean); |
There was a problem hiding this comment.
Suggestion: runStatus reads the entire usage log into memory with readFileSync(...).split(...); since the log grows unbounded over time, this can cause high memory usage or process failure on large files. Stream the file line-by-line or cap parsed bytes/lines. [performance]
Severity Level: Major ⚠️
- ❌ Status command can OOM on large usage logs.
- ⚠️ Running `status` may spike memory unexpectedly.
- ⚠️ Operational observability degraded for long-lived deployments.Steps of Reproduction ✅
1. Run many search/extract/crawl operations so that usage logging is active: each call to
`FanoutEngine.search/extract/crawl` in `src/engine/fanout.ts:93-208` invokes
`this.usageLogger.write(...)`, which appends one JSONL line to the usage log path
configured by `UsageLogger` in `src/logging/usage.ts:16-18,45-60`.
2. Allow the usage log file (default `~/.config/coldsearch/usage.jsonl` from
`defaultUsageLogPath()` in `src/logging/usage.ts:16-18`) to grow very large over time
(hundreds of MB or more).
3. Invoke the status command via CLI, e.g. `coldsearch status`, which flows through
`main()` in `src/cli.ts:221-257`: `parseArgs()` sets `options.status`, and `main()` calls
`runStatus(options)` at `src/cli.ts:232-234`.
4. In `runStatus()` at `src/cli.ts:152-216`, the code resolves `usagePath` and, if the
file exists, executes `fs.readFileSync(resolved, "utf8").split("\n").filter(Boolean);` at
line 438, loading the entire (potentially huge) file into memory at once, which can cause
high memory usage or process failure when the log is large.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/cli.ts
**Line:** 438:438
**Comment:**
*Performance: `runStatus` reads the entire usage log into memory with `readFileSync(...).split(...)`; since the log grows unbounded over time, this can cause high memory usage or process failure on large files. Stream the file line-by-line or cap parsed bytes/lines.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+290
to
+296
| this.usageLogger.write({ | ||
| timestamp: new Date().toISOString(), | ||
| provider, | ||
| capability: "search", | ||
| key: keyRef, | ||
| success: true, | ||
| response_time_ms: Math.round(performance.now() - start), |
There was a problem hiding this comment.
Suggestion: Search fanout now performs synchronous usage-log writes inside async provider execution paths, which blocks the Node.js event loop for each provider result/error and degrades parallel throughput. Use non-blocking logging (async append/queued writer) so provider fanout remains concurrent. [performance]
Severity Level: Major ⚠️
- ⚠️ Each provider result performs synchronous disk writes.
- ⚠️ High-volume searches experience reduced concurrency.
- ⚠️ Agent mode research steps incur extra latency.Steps of Reproduction ✅
1. Execute a normal search through the CLI, e.g. `coldsearch "test query"`, which calls
`main()` in `src/cli.ts:221-257`, then `runFanoutMode()` in `src/cli.ts:248-257`, which
uses `new LocalExecutionBackend(options.config)` from `src/execution/backend.ts:4-10`.
2. `LocalExecutionBackend.search()` in `src/execution/backend.ts:12-14` delegates to
`FanoutEngine.search()` in `src/engine/fanout.ts:93-147`, which runs
`Promise.allSettled(providers.map((provider) => this.searchProvider(provider, query)))`,
invoking `searchProvider()` concurrently for each configured provider.
3. Inside `searchProvider()` at `src/engine/fanout.ts:16-38`, after resolving an API key
and calling the adapter, the code logs usage by calling `this.usageLogger.write({...})` in
both the success path at lines 31-37 and the error path at lines 45-53.
4. `UsageLogger.write()` in `src/logging/usage.ts:53-60` performs `existsSync`,
`mkdirSync`, and `appendFileSync` synchronously on the JSONL log file, so every provider
result/error in the fanout path performs a blocking filesystem write on the Node.js event
loop, which under high query volume or many providers reduces parallel throughput and
increases latency for both CLI searches and agent calls that use `LocalExecutionBackend`
(see `SearchAgent` backend initialization in `src/agent/agent.ts:9-14`).Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/engine/fanout.ts
**Line:** 290:296
**Comment:**
*Performance: Search fanout now performs synchronous usage-log writes inside async provider execution paths, which blocks the Node.js event loop for each provider result/error and degrades parallel throughput. Use non-blocking logging (async append/queued writer) so provider fanout remains concurrent.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
144
to
148
| body: JSON.stringify({ | ||
| urls: uniqueUrls, | ||
| include_images: false, | ||
| url: normalizedUrl, | ||
| limit, | ||
| extract_depth: "basic", | ||
| }), |
There was a problem hiding this comment.
Suggestion: Tavily crawl requests use limit, but the provider contract in this repo documents max_results for /crawl; this causes the requested crawl size to be ignored and can return fewer/more pages than requested. Send the documented field name to preserve caller expectations for result limits. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ Tavily crawl ignores CLI-specified result limits.
- ⚠️ FanoutEngine.crawl returns unpredictable Tavily page counts.Steps of Reproduction ✅
1. Configure Tavily as a crawl provider (capability "crawl" pointing to provider "tavily"
in config) so that `resolveCapabilityProviders()` in `src/providers.ts:96-120` can select
it.
2. Run the CLI crawl mode with a non-default limit against Tavily, e.g. `coldsearch crawl
https://example.com --providers tavily --limit 3`, which routes via `main()` in
`src/cli.ts:201-233` to `runCrawlMode()` in `src/cli.ts:310-44` (lines 31-44 of the
280–539 chunk).
3. `runCrawlMode()` constructs a `LocalExecutionBackend` and calls
`backend.crawl(options.query, { limit: options.limit, ... })` (`src/cli.ts:38-44`), which
forwards to `FanoutEngine.crawl()` in `src/engine/fanout.ts:213-240`, passing the
user-specified `limit` through as `options.limit` into each provider adapter.
4. For Tavily, `FanoutEngine.crawl()` creates `TavilyAdapter` and calls
`TavilyAdapter.crawl()` (`src/adapters/tavily.ts:123-157`), where the numeric limit is
sanitized (line 133) but then sent to Tavily's `/crawl` endpoint as `{ url: normalizedUrl,
limit, extract_depth: "basic" }` (lines 136-148); Tavily's API expects `max_results` for
result count (per provider documentation), so the backend ignores the unrecognized `limit`
field, causing the CLI's `limit` flag and `FanoutOptions.limit` to be silently ignored and
`runCrawlMode()` to report a `total` (`src/cli.ts:46-53`) that does not honor user
expectations.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/adapters/tavily.ts
**Line:** 144:148
**Comment:**
*Api Mismatch: Tavily crawl requests use `limit`, but the provider contract in this repo documents `max_results` for `/crawl`; this causes the requested crawl size to be ignored and can return fewer/more pages than requested. Send the documented field name to preserve caller expectations for result limits.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+68
to
+69
| const match = address.match(/^::ffff:(\d+\.\d+\.\d+\.\d+)$/i); | ||
| return match ? match[1] : null; |
There was a problem hiding this comment.
Suggestion: The IPv4-mapped IPv6 parser only accepts dotted-quad forms (::ffff:127.0.0.1) and misses valid mapped forms like ::ffff:7f00:1, so private/loopback targets can bypass the SSRF block. Normalize and decode all IPv4-mapped IPv6 representations before checking blocked CIDRs. [ssrf]
Severity Level: Critical 🚨
- ❌ Agent SSRF guard bypass via IPv4-mapped IPv6 encoding.
- ⚠️ Internal HTTP services like metadata endpoints exposed.Steps of Reproduction ✅
1. Run the CLI in agent mode (e.g., `coldsearch --agent "probe internal metadata"`), which
routes through `main()` in `src/cli.ts:201-233` to `runAgentMode()` in `src/cli.ts:62-85`;
`runAgentMode()` constructs a `SearchAgent` with a backend and LLM configuration.
2. During `SearchAgent.research()` (`src/agent/agent.ts:158-287`), the LLM can issue tool
calls parsed by `parseAgentPayload()` from `src/agent/tools.ts:132-167`; when the model
emits `{"type":"tool","tool":"fetch","args":["http://[::ffff:7f00:1]/"]}`, the `tools`
registry in `src/agent/tools.ts:98-102` resolves this to `fetchTool`, which calls
`context.fetchFn(url)` in `fetchTool.execute()` (`src/agent/tools.ts:52-55`).
3. `fetchFn` is wired to `SearchAgent.fetchContent()` in the tool context (see
`toolContext` in `src/agent/agent.ts:230-243`), so
`fetchContent("http://[::ffff:7f00:1]/")` runs; `fetchContent()` immediately calls
`this.validateFetchUrl(url)` at `src/agent/agent.ts:293-295` to enforce SSRF protections.
4. Inside `validateFetchUrl()` (`src/agent/agent.ts:308-347`), `parsedUrl.hostname` is
`"::ffff:7f00:1"`. `isBlockedIpAddress(parsedUrl.hostname)` (`src/agent/agent.ts:324`)
sees an IPv6 address and calls `extractIPv4FromMapped()` (`src/agent/agent.ts:66-70`),
which only matches dotted-quad forms like `::ffff:127.0.0.1` and returns `null` for
`::ffff:7f00:1`. The subsequent IPv6 prefix checks (lines 97-107) do not match
`::ffff:7f00:1`, so the function incorrectly returns `false`, treating this IPv4-mapped
loopback as public. The later DNS `lookup()` (`src/agent/agent.ts:328-333`) returns the
same IPv6 address, and the repeated `isBlockedIpAddress(entry.address)` check also passes.
As a result, `fetchValidatedBody()` (`src/agent/agent.ts:350-370`) performs an HTTP GET
directly to this internal-equivalent address, allowing SSRF to 127.0.0.1 (and other
private IPv4 ranges) when encoded as non-dotted IPv4-mapped IPv6, bypassing the intended
CIDR-based blocklist.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/agent/agent.ts
**Line:** 68:69
**Comment:**
*Ssrf: The IPv4-mapped IPv6 parser only accepts dotted-quad forms (`::ffff:127.0.0.1`) and misses valid mapped forms like `::ffff:7f00:1`, so private/loopback targets can bypass the SSRF block. Normalize and decode all IPv4-mapped IPv6 representations before checking blocked CIDRs.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
This was referenced May 19, 2026
…E.md Co-authored-by: Cursor <cursoragent@cursor.com>
Collaborator
Author
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (20+ files)
Reviewed by grok-code-fast-1:optimized:free · 166,665 tokens |
Preserve full conversation outcomes in docs/decisions/; link from README. Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/providers/jina.md (1)
10-19:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMerge duplicate authentication sections.
The document has two "Authentication" headers (lines 10 and 17) with overlapping content. The first explains that the key is optional and shows the export command, while the second explains the bearer token behavior. These should be combined into a single coherent section.
📝 Proposed fix to merge the sections
-## Authentication (Optional for basic usage) - -```bash -# Optional - increases rate limits -export JINA_API_KEY="jina_..." -``` - ## Authentication -If you configure `JINA_API_KEY`, ColdSearch will send it as a bearer token. The current adapter also works without a key for basic Reader extraction. +Jina Reader works without an API key for basic extraction. If you configure `JINA_API_KEY`, ColdSearch will send it as a bearer token to access higher rate limits. + +```bash +# Optional - increases rate limits +export JINA_API_KEY="jina_..." +```🤖 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 `@docs/providers/jina.md` around lines 10 - 19, Merge the two duplicate "Authentication" sections into a single coherent section: keep a concise prose sentence stating that Jina Reader works without an API key for basic extraction and that setting JINA_API_KEY causes ColdSearch to send it as a bearer token to access higher rate limits (mentioning JINA_API_KEY and ColdSearch explicitly), then include the bash example export JINA_API_KEY="jina_..." code block below that sentence; update the header "Authentication" and remove the extra/duplicated header and paragraph so only one combined section remains.
🤖 Prompt for all review comments with 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.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 10-25: Add an explicit least-privilege permissions block for this
workflow: at the top-level of the workflow (above the "docs-sync" job) add a
permissions map that grants only what the docs-sync job needs—for example
"contents: read" (and if your tests post status/checks, add "checks: write"
only) instead of using default token permissions; update the workflow YAML to
include the "permissions:" key so the "docs-sync" job and its steps (checkout,
setup-node, npm ci, npm run test:docs) run with those minimal privileges.
- Around line 13-16: Update both checkout and Node setup steps in the docs-sync
and build-and-test jobs to use immutable commit SHAs instead of mutable tags and
set persist-credentials: false on checkout; specifically replace uses:
actions/checkout@v4 with the corresponding actions/checkout@<commit-sha> and add
persist-credentials: false to that checkout step, and replace uses:
actions/setup-node@v4 with actions/setup-node@<commit-sha>, ensuring you update
both occurrences of each action in the workflow.
In `@docs/providers/searxng.md`:
- Line 41: Remove the extra leading space before the markdown list marker for
the line starting with "- SearXNG gives ColdSearch a self-hosted search
option..." so the hyphen is left-aligned with other list items; edit the entry
in docs/providers/searxng.md (the line containing that exact text) to eliminate
the leading space and ensure consistent list indentation with the surrounding
items.
In `@src/adapters/exa.ts`:
- Around line 122-123: Wrap the call that computes domain (the new
URL(normalizedUrl).hostname expression which assigns to variable domain) in a
try/catch so malformed normalizedUrl values don't throw a raw runtime error; on
catch rethrow a deterministic adapter error (e.g., throw new
AdapterError(`Malformed crawl URL: ${normalizedUrl}`, { cause: err }) or the
project’s standard adapter error type) and ensure you import/use the existing
AdapterError class used by other adapters so CLI/tests observe a stable error
shape.
In `@src/cli.ts`:
- Around line 411-475: The runStatus function is overly complex—extract three
helpers to reduce cognitive load: implement buildCapabilitySummary(config) to
replace the byCapability construction (the Object.fromEntries mapping that
creates capability -> { providers, strategy }), implement
buildKeyPoolSummary(config) to replace the keyPools construction (the
Object.fromEntries mapping that creates provider -> { keys, strategy }), and
implement parseUsageLog(usagePath): Record<string, UsageStats> to encapsulate
the entire usage-file resolution, reading, JSON parsing, cutoff filtering, and
success_rate calculation (the try/catch block that builds usageSummary); then
call these helpers from runStatus and use their return values (capabilities,
key_pools, recent_usage_summary_7d) while preserving existing behavior and
error-eating semantics.
- Around line 421-426: The mapping that builds keyPools reads
cfg.keyPool.keys.length without guarding for a missing keyPool, which can throw;
update the Object.entries(...).map callback (where keyPools is constructed) to
safely handle absent cfg.keyPool by using optional chaining or fallbacks so keys
count defaults to 0 and strategy defaults to "round-robin" (e.g., read const
keysCount = cfg.keyPool?.keys?.length ?? 0 and const strategy =
cfg.keyPool?.strategy ?? "round-robin" inside the map before returning the
[provider, { keys: keysCount, strategy }] tuple).
In `@src/engine/keypool.ts`:
- Around line 71-74: The returned KeyResult currently exposes the raw pool entry
(keyRef) which for literal entries can leak the secret; when constructing the
result in the function that uses pool.keys[keyIndex] and resolveKeyRef(...), do
not return the literal secret as KeyResult.ref. Instead detect literal entries
(e.g., keyRef.type === 'literal' or keyRef.kind === 'literal') and replace ref
with a non-secret identifier such as keyRef.id (if present) or a deterministic
anonymized token (e.g., `pool:${keyIndex}` or a secure hash/fingerprint of the
key) — never include the raw key value; for non-literal refs keep the original
reference object. Ensure resolveKeyRef continues to return the secret value but
KeyResult.ref contains only the non-sensitive identifier.
In `@src/logging/usage.ts`:
- Around line 33-39: The function getKeyReference currently returns the raw
literal key (keyPool.keys[0]); change it to mask any literal key before
returning so secrets aren't exposed: read the first key into a local (e.g. const
raw = keyPool.keys[0]), produce a masked representation that keeps a small
prefix/suffix (for example first 4 and last 4 chars with the middle replaced by
ellipsis or asterisks) for keys longer than a threshold, and return a
provider-scoped safe reference (for example `${provider}:${masked}`) instead of
the raw key; keep the existing `${provider}:keyless` behavior when keyPool is
missing or empty.
In `@test/adapters/_fetch-mock.mjs`:
- Around line 6-9: The URL extraction in the mocked globalThis.fetch handler is
incorrect for Request objects because it uses input.toString(); update the logic
in the fetch stub (the async function assigned to globalThis.fetch) to set url =
typeof input === "string" ? input : input.url (or input.url || input.toString()
as a fallback) so Request instances yield their real URL; keep the existing
method and key construction (method and key variables) but replace the url
assignment to use input.url when input is a Request object.
In `@test/adapters/exa.adapter.test.mjs`:
- Line 75: The assertion uses Array.prototype.sort() with default behavior which
is flagged by the quality gate; update the key-sorting call around
assert.deepEqual(Object.keys(results[0]).sort(), ["content", "title", "url"]) to
use an explicit comparator such as localeCompare (i.e., sort((a,b) =>
a.localeCompare(b))) so the keys are deterministically compared before the
deepEqual assertion on results[0].
In `@test/adapters/tavily.adapter.test.mjs`:
- Line 21: Replace the non-deterministic default sort on the keys in the
assertion with an explicit comparator: when comparing Object.keys(results[0]) in
the test (the assert.deepEqual call that sorts keys of results[0]), call .sort
with a comparator that uses localeCompare (e.g., (a, b) => a.localeCompare(b))
so the key order is deterministic before comparing to the expected array
["score","snippet","source","title","url"].
In `@test/capability-matrix-drift.test.mjs`:
- Around line 102-103: Replace the unstable default string sort with an explicit
locale-aware comparator wherever sorted diagnostics are produced (specifically
in the template expressions that build the diagnostic lines using
[...expected].sort().join(", ") and [...registryCaps].sort().join(", ") and the
similar occurrences at the second pair of lines); modify the sort calls used in
the diagnostic output generation (the expressions that reference expected and
registryCaps) to use .sort((a, b) => a.localeCompare(b)) so ordering is
deterministic and locale-explicit.
- Around line 30-35: The parsing currently splits the entire tail (afterHeader)
into table-like lines, which picks up later tables; limit extraction to the Dual
Matrix block only by first isolating the text between the "## Dual Matrix"
header and the next top-level header (e.g., next line starting with "## ") or
end-of-file, then run the existing .split("\n").filter(...) logic on that
isolated block (use the existing afterHeader variable as the start and compute
the end index before creating rows). Ensure you update the rows computation to
operate on that sliced block rather than the full afterHeader string.
In `@test/fanout-engine.test.mjs`:
- Around line 26-27: Replace the global mutation of Math.random (originalRandom
/ Math.random = () => 0.9) with a scoped mock inside the test using the test
runner's helper: call t.mock.method(Math, "random", () => 0.9) within the test
that asserts the "random" strategy so the random value is isolated to that test,
remove the manual save/restore of originalRandom, and rely on t.mock cleanup to
restore Math.random after the test completes.
In `@test/providers-docs.test.mjs`:
- Around line 191-193: Replace the unspecialized array sorts used in the
assertion comparing "declared" and "expected" with an explicit comparator that
uses localeCompare (e.g., sort((a, b) => a.localeCompare(b))) so ordering is
deterministic and quality-gate compliant; update both occurrences (the assert
using [...declared].sort() / [...expected].sort() and the similar assertion at
the other location referenced) to use localeCompare for both arrays.
- Around line 33-37: The current parser collects every table-like line after
afterHeader, allowing later tables to contaminate matrixSupport; change the
logic that builds rows so you first locate the index of the first contiguous
table block in afterHeader (find the first line matching /^\|\s*[^|]+\s*\|/),
then collect lines from that index forward only while each line matches the
table-row or table-separator pattern (stop on the first non-matching/blank
line), and finally apply the existing filters to that contiguous slice; update
the variable handling around rows/afterHeader and reference the existing regexes
used in the filters to ensure identical row-selection semantics.
In `@test/registry-adapter-drift.test.mjs`:
- Line 35: The code mutates arrays by calling .sort() directly on stored arrays
(e.g., providerRegistry[providerName].capabilities) which leaks state and fails
Sonar's explicit-sort rule; fix by performing a non-mutating, locale-aware sort
when building comparison values—create a shallow copy (e.g., using
[...providerRegistry[providerName].capabilities] or .slice()) and call
.sort((a,b) => a.localeCompare(b)) to produce registryCaps, and apply the same
change to the adapter instance capabilities (e.g., adapter.capabilities) and any
other places that previously called .sort() in place.
---
Outside diff comments:
In `@docs/providers/jina.md`:
- Around line 10-19: Merge the two duplicate "Authentication" sections into a
single coherent section: keep a concise prose sentence stating that Jina Reader
works without an API key for basic extraction and that setting JINA_API_KEY
causes ColdSearch to send it as a bearer token to access higher rate limits
(mentioning JINA_API_KEY and ColdSearch explicitly), then include the bash
example export JINA_API_KEY="jina_..." code block below that sentence; update
the header "Authentication" and remove the extra/duplicated header and paragraph
so only one combined section remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 86232e0d-d223-476f-8d62-610f2925f40a
📒 Files selected for processing (55)
.github/workflows/ci.ymlCLAUDE.mdREADME.mdSKILL.mdconfig.example.tomldocs/CAPABILITY_MATRIX.mddocs/CONFIGURATION.mddocs/NORTH_STAR.mddocs/PROGRESS.mddocs/architecture.mddocs/decisions/2026-05-19-routing-llm-policy-and-shipping.mddocs/plans/TEMPLATE.mddocs/plans/brave.mddocs/plans/exa.mddocs/plans/firecrawl.mddocs/plans/jina.mddocs/plans/serper.mddocs/plans/tavily.mddocs/providers/README.mddocs/providers/brave.mddocs/providers/exa.mddocs/providers/firecrawl.mddocs/providers/jina.mddocs/providers/searxng.mddocs/providers/serper.mddocs/providers/tavily.mdpackage.jsonsrc/adapters/exa.tssrc/adapters/firecrawl.tssrc/adapters/tavily.tssrc/agent/agent.tssrc/agent/context.tssrc/agent/llm.tssrc/cli.tssrc/config.tssrc/engine/fanout.tssrc/engine/keypool.tssrc/logging/usage.tssrc/providers.tssrc/types.tstest/adapters/_fetch-mock.mjstest/adapters/brave.adapter.test.mjstest/adapters/exa.adapter.test.mjstest/adapters/firecrawl.adapter.test.mjstest/adapters/jina.adapter.test.mjstest/adapters/searxng.adapter.test.mjstest/adapters/serper.adapter.test.mjstest/adapters/tavily.adapter.test.mjstest/agent-mode.test.mjstest/capability-matrix-drift.test.mjstest/cli-integration.test.mjstest/fanout-engine.test.mjstest/providers-docs.test.mjstest/registry-adapter-drift.test.mjstest/reranker.test.mjs
Comment on lines
+13
to
+16
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Node.js | ||
| uses: actions/setup-node@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify unpinned actions and checkout credential persistence in workflows
rg -n --glob '.github/workflows/*.yml' 'uses:\s*actions\/(checkout|setup-node)@'
rg -n --glob '.github/workflows/*.yml' 'persist-credentials:\s*false'Repository: Coldaine/ColdSearch
Length of output: 333
🏁 Script executed:
# First, check if the workflows directory exists and read the ci.yml file
cat .github/workflows/ci.ymlRepository: Coldaine/ColdSearch
Length of output: 841
🏁 Script executed:
# Also verify what workflows exist
ls -la .github/workflows/Repository: Coldaine/ColdSearch
Length of output: 227
Pin GitHub Actions by commit SHA and disable credential persistence for checkout.
Both actions/checkout@v4 (lines 13 and 32) and actions/setup-node@v4 (lines 16 and 35) use mutable tag references instead of immutable commit SHAs. Additionally, checkout actions lack persist-credentials: false configuration, persisting credentials unnecessarily across the workflow.
These configurations appear in both the docs-sync and build-and-test jobs.
🛡️ Suggested hardening
- uses: actions/checkout@v4
+ with:
+ persist-credentials: false
- name: Set up Node.js
- uses: actions/setup-node@v4
+ uses: actions/setup-node@<PINNED_COMMIT_SHA>
with:
node-version: 20
cache: npm🧰 Tools
🪛 zizmor (1.25.2)
[warning] 13-13: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 13-13: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 16-16: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/ci.yml around lines 13 - 16, Update both checkout and Node
setup steps in the docs-sync and build-and-test jobs to use immutable commit
SHAs instead of mutable tags and set persist-credentials: false on checkout;
specifically replace uses: actions/checkout@v4 with the corresponding
actions/checkout@<commit-sha> and add persist-credentials: false to that
checkout step, and replace uses: actions/setup-node@v4 with
actions/setup-node@<commit-sha>, ensuring you update both occurrences of each
action in the workflow.
| ## Why It Matters | ||
|
|
||
| SearXNG gives ColdSearch a self-hosted search option that can later fit a hybrid remote execution model where search infrastructure and secrets live centrally. | ||
| - SearXNG gives ColdSearch a self-hosted search option that can later fit a hybrid remote execution model where search infrastructure and secrets live centrally. |
There was a problem hiding this comment.
Fix inconsistent list indentation.
Line 41 has an extra space before the list marker, causing inconsistent indentation. Remove the leading space to align with standard markdown formatting.
📝 Proposed fix
- - SearXNG gives ColdSearch a self-hosted search option that can later fit a hybrid remote execution model where search infrastructure and secrets live centrally.
+- SearXNG gives ColdSearch a self-hosted search option that can later fit a hybrid remote execution model where search infrastructure and secrets live centrally.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - SearXNG gives ColdSearch a self-hosted search option that can later fit a hybrid remote execution model where search infrastructure and secrets live centrally. | |
| - SearXNG gives ColdSearch a self-hosted search option that can later fit a hybrid remote execution model where search infrastructure and secrets live centrally. |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 41-41: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 1
(MD005, list-indent)
🤖 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 `@docs/providers/searxng.md` at line 41, Remove the extra leading space before
the markdown list marker for the line starting with "- SearXNG gives ColdSearch
a self-hosted search option..." so the hyphen is left-aligned with other list
items; edit the entry in docs/providers/searxng.md (the line containing that
exact text) to eliminate the leading space and ensure consistent list
indentation with the surrounding items.
Comment on lines
+411
to
+475
| async function runStatus(options: ExtendedCLIOptions): Promise<void> { | ||
| const config = loadConfig(options.config); | ||
|
|
||
| const byCapability = Object.fromEntries( | ||
| Object.entries(config.capabilities).map(([capability, cfg]) => [ | ||
| capability, | ||
| { providers: cfg.providers, strategy: cfg.strategy || "all" }, | ||
| ]) | ||
| ); | ||
|
|
||
| const keyPools = Object.fromEntries( | ||
| Object.entries(config.providers).map(([provider, cfg]) => [ | ||
| provider, | ||
| { keys: cfg.keyPool.keys.length, strategy: cfg.keyPool.strategy || "round-robin" }, | ||
| ]) | ||
| ); | ||
|
|
||
| const usagePath = config.logging?.usage?.path || "~/.config/coldsearch/usage.jsonl"; | ||
|
|
||
| const usageSummary: Record<string, { calls: number; successes: number; success_rate: number }> = {}; | ||
|
|
||
| try { | ||
| const resolved = usagePath.startsWith("~/") | ||
| ? path.join(os.homedir(), usagePath.slice(2)) | ||
| : usagePath; | ||
|
|
||
| if (resolved && fs.existsSync(resolved)) { | ||
| const lines = fs.readFileSync(resolved, "utf8").split("\n").filter(Boolean); | ||
| const cutoff = Date.now() - 7 * 24 * 60 * 60 * 1000; | ||
|
|
||
| for (const line of lines) { | ||
| let entry; | ||
| try { | ||
| entry = JSON.parse(line); | ||
| } catch { | ||
| continue; | ||
| } | ||
| const ts = typeof entry.timestamp === "string" ? Date.parse(entry.timestamp) : NaN; | ||
| if (!Number.isFinite(ts) || ts < cutoff) continue; | ||
| const provider = entry.provider; | ||
| if (typeof provider !== "string") continue; | ||
| if (!usageSummary[provider]) { | ||
| usageSummary[provider] = { calls: 0, successes: 0, success_rate: 0 }; | ||
| } | ||
| usageSummary[provider].calls += 1; | ||
| if (entry.success === true) usageSummary[provider].successes += 1; | ||
| } | ||
|
|
||
| for (const value of Object.values(usageSummary)) { | ||
| value.success_rate = value.calls > 0 ? value.successes / value.calls : 0; | ||
| } | ||
| } | ||
| } catch { | ||
| // best-effort: ignore usage parsing errors | ||
| } | ||
|
|
||
| const status = { | ||
| capabilities: byCapability, | ||
| key_pools: keyPools, | ||
| usage_log: usagePath, | ||
| recent_usage_summary_7d: Object.keys(usageSummary).length ? usageSummary : undefined, | ||
| }; | ||
|
|
||
| console.log(formatOutput(status, options)); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Refactor runStatus to reduce cognitive complexity.
SonarCloud flags this function at complexity 30 vs the allowed 15. Extract helpers to separate concerns:
buildCapabilitySummary(config)for lines 414-419buildKeyPoolSummary(config)for lines 421-426parseUsageLog(usagePath): Record<string, UsageStats>for lines 432-465
This improves testability and maintainability.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 411-411: Refactor this function to reduce its Cognitive Complexity from 30 to the 15 allowed.
🤖 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 `@src/cli.ts` around lines 411 - 475, The runStatus function is overly
complex—extract three helpers to reduce cognitive load: implement
buildCapabilitySummary(config) to replace the byCapability construction (the
Object.fromEntries mapping that creates capability -> { providers, strategy }),
implement buildKeyPoolSummary(config) to replace the keyPools construction (the
Object.fromEntries mapping that creates provider -> { keys, strategy }), and
implement parseUsageLog(usagePath): Record<string, UsageStats> to encapsulate
the entire usage-file resolution, reading, JSON parsing, cutoff filtering, and
success_rate calculation (the try/catch block that builds usageSummary); then
call these helpers from runStatus and use their return values (capabilities,
key_pools, recent_usage_summary_7d) while preserving existing behavior and
error-eating semantics.
Comment on lines
+26
to
+27
| const originalRandom = Math.random; | ||
| Math.random = () => 0.9; // pick last provider |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether test runner configuration enables concurrency and whether global Math.random
# monkeypatching appears in other tests (risk of cross-test interference).
set -euo pipefail
echo "== package.json test scripts =="
cat package.json | jq -r '.scripts // {}'
echo
echo "== node:test usage with explicit concurrency options =="
rg -n --type=js --type=mjs --type=ts -C2 'test\s*\(.*\{[^)]*concurrency'
echo
echo "== global Math.random monkeypatch patterns in tests =="
rg -n --type=js --type=mjs --type=ts -C2 'Math\.random\s*=' test src || trueRepository: Coldaine/ColdSearch
Length of output: 618
🏁 Script executed:
#!/bin/bash
# Search for Math.random usage and Node version requirements
set -euo pipefail
echo "=== Finding test files ==="
fd -e test.mjs -o -e test.js test/
echo
echo "=== Checking for Math.random in test files ==="
fd -e test.mjs -o -e test.js test/ -x grep -l "Math.random" {} \;
echo
echo "=== Node version requirement ==="
if [ -f .nvmrc ]; then
cat .nvmrc
elif [ -f package.json ]; then
jq -r '.engines.node // "not specified"' package.json
fi
echo
echo "=== Searching for t.mock usage in existing tests ==="
fd -e test.mjs -o -e test.js test/ -x grep -l "t\.mock" {} \; || echo "(no matches found)"
echo
echo "=== Checking fanout-engine.test.mjs for test function signature ==="
head -30 test/fanout-engine.test.mjs | grep -E "^test\("Repository: Coldaine/ColdSearch
Length of output: 225
🌐 Web query:
Node.js test runner concurrency behavior node:test default parallel execution
💡 Result:
The Node.js test runner (node:test) manages concurrency at two distinct levels: the execution of test files and the execution of tests within those files [1][2]. 1. Test File Concurrency (CLI Level) By default, the Node.js test runner uses process-level isolation, where each matching test file is executed in a separate child process [3]. The maximum number of these child processes running simultaneously is controlled by the --test-concurrency CLI flag [3]. This flag specifically manages the parallelism of test files, not the tests contained within them [1][2]. 2. Test Concurrency (Within a File) When a test file is executed, all tests defined within that file run within a single application thread [3][4]. By default, tests within a file are executed sequentially (concurrency of 1) [3][5]. While the node:test module provides a concurrency option for the test and suite functions, this option only manages asynchronous execution within that single thread [3]. - If a number is provided, that many tests will run asynchronously (managed by the event loop) [3]. - If set to true, all scheduled asynchronous tests run concurrently within the thread [3]. - If set to false (the default), only one test runs at a time [3]. It is important to distinguish these two behaviors: the --test-concurrency flag does not affect the execution of individual tests within a file, and the concurrency option within a test file does not affect the parallel execution of multiple test files [1][2]. When process-level isolation is disabled (e.g., via --experimental-test-isolation=none), all test files are imported into the same process and top-level tests are executed with a concurrency of one [3].
Citations:
- 1: --test-concurrency has no effect on single test file nodejs/node#51186
- 2: Support
concurrencywhen--experimental-test-isolationis set to'none'nodejs/node#55939 - 3: https://nodejs.org/api/test.html
- 4: concurrent tests are slow nodejs/node#47365
- 5: https://www.sonarsource.com/blog/node-js-test-runner
🏁 Script executed:
#!/bin/bash
# Read the test file to check for concurrency options and test structure
echo "=== fanout-engine.test.mjs content ==="
cat test/fanout-engine.test.mjs
echo
echo "=== Check if any test uses concurrency option ==="
grep -n "test\(" test/fanout-engine.test.mjs | head -10Repository: Coldaine/ColdSearch
Length of output: 4485
Use scoped mocks instead of global mutations for better test isolation.
Lines 26-27 override Math.random globally. While the Node.js test runner executes tests sequentially within a file by default (not concurrently), global state mutations are still poor practice. Use t.mock.method() instead for cleaner test isolation:
test('strategy: "random" selects exactly one provider', async (t) => {
t.mock.method(Math, "random", () => 0.9); // pick last provider
// ... rest of test
// no manual restore needed; t.mock handles cleanup
});🤖 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 `@test/fanout-engine.test.mjs` around lines 26 - 27, Replace the global
mutation of Math.random (originalRandom / Math.random = () => 0.9) with a scoped
mock inside the test using the test runner's helper: call t.mock.method(Math,
"random", () => 0.9) within the test that asserts the "random" strategy so the
random value is isolated to that test, remove the manual save/restore of
originalRandom, and rely on t.mock cleanup to restore Math.random after the test
completes.
| describe("registry ↔ adapter capability consistency", () => { | ||
| for (const providerName of listRegisteredProviders()) { | ||
| test(`${providerName}: registry capabilities match adapter capabilities`, () => { | ||
| const registryCaps = providerRegistry[providerName].capabilities.sort().join(","); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -i "registry-adapter-drift"Repository: Coldaine/ColdSearch
Length of output: 100
🏁 Script executed:
cat -n test/registry-adapter-drift.test.mjsRepository: Coldaine/ColdSearch
Length of output: 2766
Use non-mutating, locale-aware sorting to prevent state leakage and fix Sonar rule violations.
Lines 35 and 40 call .sort() directly on arrays stored in the registry and adapter instance. This mutates the original data during each loop iteration, causing state to leak between test runs. Additionally, default .sort() without an explicit comparator violates Sonar's quality rule requiring explicit alphabetical sorting. Lines 52–53 have the same Sonar violation.
Proposed fix
- const registryCaps = providerRegistry[providerName].capabilities.sort().join(",");
+ const registryCaps = [...providerRegistry[providerName].capabilities]
+ .sort((a, b) => a.localeCompare(b))
+ .join(",");
@@
- const adapterCaps = instance.capabilities.sort().join(",");
+ const adapterCaps = [...instance.capabilities]
+ .sort((a, b) => a.localeCompare(b))
+ .join(",");
@@
- const registered = listRegisteredProviders().sort();
- const adapterNames = Object.keys(adapterByName).sort();
+ const registered = [...listRegisteredProviders()].sort((a, b) => a.localeCompare(b));
+ const adapterNames = Object.keys(adapterByName).sort((a, b) => a.localeCompare(b));🤖 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 `@test/registry-adapter-drift.test.mjs` at line 35, The code mutates arrays by
calling .sort() directly on stored arrays (e.g.,
providerRegistry[providerName].capabilities) which leaks state and fails Sonar's
explicit-sort rule; fix by performing a non-mutating, locale-aware sort when
building comparison values—create a shallow copy (e.g., using
[...providerRegistry[providerName].capabilities] or .slice()) and call
.sort((a,b) => a.localeCompare(b)) to produce registryCaps, and apply the same
change to the adapter instance capabilities (e.g., adapter.capabilities) and any
other places that previously called .sort() in place.
Remove echo-style and triplicate drift checks; add search/HTTP contract tests. Use dot reporter for quieter CI output. Add docs/contributing/testing.md. Closes part of #7. Co-authored-by: Cursor <cursoragent@cursor.com>
This was referenced May 20, 2026
- Report single-provider mode when config strategy is random or one provider ran - status: show configured vs effective_strategy - OPENAI_BASE_URL env for OpenAI-compatible agent endpoints - Merge CI into one job; align example crawl pool and README first-run
Mask literal API keys in logs/dry-run, cap status usage tail, harden SSRF for IPv4-mapped hex addresses, fix Tavily crawl max_results, scope Dual Matrix parser to first table, explicit test globs and contract mocks.
test: consolidate redundant tests and quiet CI output
Coldaine
merged commit May 20, 2026
bc7e1e8
into
feat/multi-provider-capabilities
2 of 3 checks passed
|
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Summary
Consolidates CI governance, adapter/runtime coverage, and LLM policy onto a single feature branch (supersedes #4).
docs-sync+build-and-test), capability matrix and provider-doc drift tests.statusand--dry-run.ClaudeClient/api.anthropic.com). Agent mode uses OpenAI chat completions only; documented in NORTH_STAR and a session decision doc.docs/decisions/2026-05-19-routing-llm-policy-and-shipping.md— how routing, config, agent mode, and quality gates actually work.Not in this PR
~/.config/coldsearch/config.toml(operators still copyconfig.example.toml).Test plan
npm test(60 tests)CodeAnt-AI Description
Expand provider coverage, add usage/status tools, and make agent mode OpenAI-only
What Changed
search,extract, andcrawlcan now be previewed with--dry-run, andstatusshows configured providers, key pools, and recent usage.Impact
✅ Full crawl coverage for more providers✅ Clearer agent-mode LLM setup✅ Safer provider planning and status checks💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
CI Governance, Runtime Coverage, and Agent LLM Policy
Overview
This PR consolidates three major initiatives: CI governance (GitHub Actions workflows and capability verification), runtime coverage (expanded adapter support for Firecrawl, Exa, and Tavily with provider-specific behaviors), and LLM policy (removing Anthropic, restricting agent mode to OpenAI only).
Key Changes
1. LLM Policy: Anthropic Removal & OpenAI-Only Agent Mode
llmProvidernow accepts only"openai"(removed"anthropic")ClaudeClientremoved;createLLMClientdefaults to and supports only OpenAIANTHROPIC_API_KEYno longer used;OPENAI_API_KEYrequired for agent mode; addedOPENAI_BASE_URLsupportCLAUDE.md,SKILL.mdupdated; new policy documented inNORTH_STAR.mdand decision recordFiles affected:
src/agent/agent.ts,src/agent/llm.ts,src/cli.ts,CLAUDE.md,SKILL.md,docs/NORTH_STAR.md2. Runtime Coverage Expansion
Adapter Capability Matrix
searchextractcrawl/crawlendpoint (simplified from multi-step)/searchsupport; polling-based async crawlFiles affected:
src/adapters/*.ts,docs/CAPABILITY_MATRIX.md,docs/providers/Crawl Implementation Patterns
/crawlPOST call withmax_resultsandextract_depth: "basic"/v2/crawl/job-{id}) → fetch resultssite:{domain}) →/contentsfetch withlivecrawl_timeout: 100003. CLI Enhancements
New Features:
statuscommand: Reports configured capabilities, provider key-pool counts/strategies, and 7-day usage stats from JSONL logs--dry-runflag: Previews execution plan without invoking adapters; shows resolved providers, key counts, and execution strategy--llmto accept onlyopenai; validates against other providers--providers,--rerank,--model,--maxSteps,--maxSourcesOutput Mode Labeling:
"fanout"mode: parallel multi-provider execution forsearch"single-provider"mode: sequential single-provider forextract/crawlor when strategy is randomFiles affected:
src/cli.ts,package.json(test scripts)4. Configuration & Usage Logging
~/.config/coldsearch/config.toml; legacy fallback only if existsenv:VAR_NAME,bws:SECRET_ID, or raw literals; literals masked as${provider}:literalin logs~/.config/coldsearch/usage.jsonl(configurable); entries include timestamp, provider, capability, key ref, success flag, response time, optional errorFiles affected:
config.example.toml,docs/CONFIGURATION.md,src/config.ts,src/logging/usage.ts,src/types.ts5. CI & Testing
Workflow Changes:
ci.ymljob withpermissions: contents: readtest:docsscript for capability matrix and provider docs consistency checksdot(quieter output)New Test Contracts:
http-errors.contract.test.mjs), search normalization (search-normalize.contract.test.mjs)--providersfiltering, missing capability errorsmaxStepsenforcement"random"determinism), fanout/sequential semantics, error handlingFiles affected:
test/,package.json,.github/workflows/ci.yml,docs/contributing/testing.md6. Documentation & Decision Records
New Documentation:
docs/CONFIGURATION.md: TOML config reference with examplesdocs/decisions/2026-05-19-routing-llm-policy-and-shipping.md: Comprehensive decision record covering routing, LLM policy, agent mode, quality gates, and follow-up issues (#6–[long-term] Agent GitHub search: playbook, ranked tools, and reminders #8)docs/decisions/README.md: Index of decision recordsdocs/plans/TEMPLATE.md: Provider adoption plan templatedocs/plans/{brave,exa,firecrawl,jina,serper,tavily}.mdUpdated Documentation:
README.md: Capability matrix expanded, roadmap updatedSKILL.md: Agent mode documentation reflects OpenAI-only policydocs/PROGRESS.md: Crawl providers now include Exadocs/architecture.md: Clarified agent-mode LLM calls use OpenAI onlyFiles affected: 20+ doc files added/updated
7. Provider Routing & Key Management
New Helper Function:
resolveCapabilityProviders(config, capability, options): Centralized provider selection logicoptions.providersor configured providersKey Pool Enhancements:
KeyResultinterface: Returns both resolved value and originating referencegetNextKeyWithRef(): Includesreffield (e.g.,env:OPENAI_API_KEY,${provider}:literal)getNextKeyWithRefOrEmpty(): Returns keyless marker when no keys configuredcreateKeyPoolManager(): Factory for fresh instancesFiles affected:
src/providers.ts,src/engine/keypool.ts,src/engine/fanout.ts8. Agent Mode Behavior
Request Context:
ResearchContexttrackscurrentQueryfor tool referencegenerateResponse()appends formattedSources:citation blockTool Safety:
validateFetchUrl()hardened withnormalizeHostname()(bracket-stripping IPv6 literals)Files affected:
src/agent/agent.ts,src/agent/context.tsTest Coverage Summary
test/adapters/*.adapter.test.mjstest/adapters/*.contract.test.mjstest/fanout-engine.test.mjs,test/reranker.test.mjstest/agent-mode.test.mjstest/agent-llm.test.mjstest/capability-matrix-drift.test.mjs,test/providers-docs.test.mjstest/cli-integration.test.mjsTest Status: 60+ npm tests passing locally; CI green on this branch
Not Included (Follow-ups)
#6~/.config/coldsearch/config.toml→#6#8Files Changed: Quick Reference
.github/workflows/ci.yml,package.jsonsrc/agent/llm.ts,src/agent/agent.ts,CLAUDE.md,SKILL.mdsrc/adapters/{exa,firecrawl,tavily}.ts+ 6 new provider docssrc/providers.ts,src/engine/fanout.ts,src/engine/keypool.tssrc/cli.ts(196 lines added)src/logging/usage.ts(new)config.example.toml,src/config.ts,docs/CONFIGURATION.md(new)Summary Statistics
search, Exacrawl