Skip to content

Fix-forward 1/4: real bugs — Exa contents, explicit-engine errors, 4xx classification#27

Open
ronaldmannak wants to merge 1 commit into
mainfrom
claude/sx-fix-1-real-bugs
Open

Fix-forward 1/4: real bugs — Exa contents, explicit-engine errors, 4xx classification#27
ronaldmannak wants to merge 1 commit into
mainfrom
claude/sx-fix-1-real-bugs

Conversation

@ronaldmannak

Copy link
Copy Markdown
Contributor

First of four stacked fix-forward PRs addressing the genuine review findings tracked in #26 (deferred while the original sx stack merged as-is). This one fixes the three real bugs.

Fixes

1. Exa API returns empty snippets (ExaBackend)
The /search request omitted a contents object, so Exa returned only metadata (title/url) — no text/summary — yet the mapper reads exactly those fields. Every API-mode snippet came back empty. Now requests contents: { text: true }.

2. Explicit --engine errors bypass the contract (SearchManager.searchExplicit)
The explicit path only caught CancellationError / SxError / BackendError; any other error (a raw URLSession/decoder error that escaped a backend's wrapping) propagated uncaught and skipped the sx: message + stable exit-code contract. Added a catch-all mapping it to SxError(.general, …) — exit 1, mirroring how the fallback search() path (which already has a bare catch) treats a non-BackendError.

3. 4xx misclassified as network → exit 7

  • TavilyBackend: HTTP 400.invalidResponse and 432/433.rateLimit (both exit 1), instead of falling through to default → .network (exit 7, "escalate / don't retry").
  • MCPHTTPClient: 401/403.auth and 429.rateLimit, instead of mapping every non-2xx to .network. (Matches what ExaBackend's API path already does.)

No new BackendErrorCode case — reuses existing exit-1 codes, consistent with the existing pageNo > 1 → .invalidResponse precedent.

Tests

  • Exa: asserts the request body requests contents.text.
  • Exa MCP: 401/403 → .auth, 429 → .rateLimit, 500 → .network (regression guard).
  • Tavily: 400 → .invalidResponse, 432/433 → .rateLimit, each asserting sxExitCode == .general (i.e. not exit 7).
  • SearchManager: a non-BackendError thrown from the explicit path maps to SxError(.general).

Notes


Generated by Claude Code

First batch of the fix-forward backlog (#26):

- ExaBackend (API mode): request `contents: { text: true }`. Without a
  `contents` object the /search endpoint returns only metadata, so every
  snippet came back empty even though the mapper reads `text`/`summary`.
- SearchManager.searchExplicit: add a catch-all mapping any non-BackendError
  (e.g. a raw URLSession/decoder error) to SxError(.general, …), so the
  explicit --engine path carries the same sx:/exit-code contract as the
  fallback path (which already had a bare catch).
- TavilyBackend: classify HTTP 400 -> .invalidResponse and 432/433 ->
  .rateLimit (both exit 1) instead of falling through to .network (exit 7).
- MCPHTTPClient: classify 401/403 -> .auth and 429 -> .rateLimit instead of
  mapping every non-2xx to .network.

Tests added: Exa contents body, MCP 401/403/429/500, Tavily 400/432/433,
and a non-BackendError explicit-path mapping.

https://claude.ai/code/session_01NAsKiDMs1XHKYV22umnV5A

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request improves error handling and API request payloads across multiple search backends. Specifically, it updates ExaBackend to request page text contents, refines HTTP status code mappings (including auth, rate limits, and bad requests) for both MCP and Tavily clients, and introduces a catch-all error mapper in SearchManager to ensure unexpected errors map to a stable exit code. Comprehensive unit tests were added to verify these behaviors. The reviewer noted that the tests for Tavily status codes 432 and 433 do not explicitly assert that an error is thrown, which could lead to silent passes, and suggested using #expect(throws:) to ensure robust test coverage.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +525 to +531
do {
_ = try await backend.search(SearchOptions(query: "test"))
} catch let error as BackendError {
#expect(error.code == .rateLimit)
#expect(error.code.sxExitCode == .general)
#expect(error.message.contains("432"))
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This test does not assert that an error is actually thrown. If backend.search succeeds without throwing, the test will silently pass. Use #expect(throws:) to ensure the expected error is thrown, matching the pattern used in other tests.

Suggested change
do {
_ = try await backend.search(SearchOptions(query: "test"))
} catch let error as BackendError {
#expect(error.code == .rateLimit)
#expect(error.code.sxExitCode == .general)
#expect(error.message.contains("432"))
}
await #expect(throws: BackendError.self) {
_ = try await backend.search(SearchOptions(query: "test"))
}
do {
_ = try await backend.search(SearchOptions(query: "test"))
} catch let error as BackendError {
#expect(error.code == .rateLimit)
#expect(error.code.sxExitCode == .general)
#expect(error.message.contains("432"))
}

Comment on lines +538 to +544
do {
_ = try await backend.search(SearchOptions(query: "test"))
} catch let error as BackendError {
#expect(error.code == .rateLimit)
#expect(error.code.sxExitCode == .general)
#expect(error.message.contains("433"))
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This test does not assert that an error is actually thrown. If backend.search succeeds without throwing, the test will silently pass. Use #expect(throws:) to ensure the expected error is thrown, matching the pattern used in other tests.

        await #expect(throws: BackendError.self) {
            _ = try await backend.search(SearchOptions(query: "test"))
        }

        do {
            _ = try await backend.search(SearchOptions(query: "test"))
        } catch let error as BackendError {
            #expect(error.code == .rateLimit)
            #expect(error.code.sxExitCode == .general)
            #expect(error.message.contains("433"))
        }

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8806d871cf

ℹ️ 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".

// network/infra failure — don't escalate it to fail-closed (exit 7).
throw BackendError(
backend: "tavily",
code: .invalidResponse,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Map Tavily 400s to usage errors

For explicit --engine tavily calls, this code: .invalidResponse becomes SxExitCode.general via BackendErrorCode.sxExitCode, even though the new message classifies HTTP 400 as a bad query/options request. That violates the repo's stable exit-code contract in AGENTS.md (bad input should exit 2 so agents know to fix the command/config), so a rejected Tavily request with invalid parameters is surfaced as an unclassified runtime failure instead of an actionable usage error.

Useful? React with 👍 / 👎.

}

let requestBody = ExaAPIRequest(query: queryString, numResults: n)
let requestBody = ExaAPIRequest(query: queryString, numResults: n, contents: .init(text: true))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Bound Exa content retrieval for snippets

This makes every Exa API search request full page text for every result, but the JSON/clean renderers emit SearchResult.content verbatim rather than applying the plain renderer's snippet cap. For normal sx --engine exa --json searches against long pages (or larger --count values), this can turn a lightweight search into megabytes of output and unnecessary content-fetch latency/cost; request a bounded text object or highlights instead of unbounded text: true.

Useful? React with 👍 / 👎.

// failure. Map to the transient (exit 1) class, not fail-closed.
throw BackendError(
backend: "tavily",
code: .rateLimit,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Treat Tavily credit exhaustion as fail-closed

When Tavily returns 432/433 for plan or credit exhaustion, classifying it as .rateLimit makes explicit searches exit 1 and lets aggregate fallback handling treat the failure as transient/retryable, even though the message tells the agent to fix the plan or credits. For users whose Tavily account is out of quota, this should be a fail-closed/escalate condition (exit 7) rather than the same class as a temporary 429 backoff.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants