Skip to content

fix: share one HTTP retry loop and validate CLI/query input - #75

Merged
tomharris merged 1 commit into
mainfrom
fix/audit-http-retries-and-input-validation
Jul 30, 2026
Merged

fix: share one HTTP retry loop and validate CLI/query input#75
tomharris merged 1 commit into
mainfrom
fix/audit-http-retries-and-input-validation

Conversation

@tomharris

@tomharris tomharris commented Jul 30, 2026

Copy link
Copy Markdown
Owner

Addresses the five audit-tagged correctness issues.

Closes #67
Closes #68
Closes #69
Closes #70
Closes #71

Retries — #68, #70, #71

All three issues pointed at the same root cause, so this fixes it once. GitHubClient, JiraClient, SlackClient and SliteClient each carried their own retry loop, and the copies had drifted apart — Jira retried once, GitHub retried once and only on primary rate limits, Slack and Slite were byte-identical duplicates. Per-client fixes never propagated.

request_with_retries() in devrag/utils/http.py is now the single implementation. It owns the mechanics (bounded attempts, sleep placement, transport-error handling); per-service policy is injected as a retry_delay(resp, attempt) callback returning seconds-to-wait or None to accept the response.

  • httpx.TransportError, not httpx.TimeoutException. ConnectError (DNS failure, connection refused), ReadError/WriteError (connection reset) and RemoteProtocolError are siblings of TimeoutException under TransportError, not subclasses — so the narrow catch let the most common real-world network faults abort a sync on first occurrence.
  • Tolerant header parsing via safe_int() / parse_retry_after(). Rate-limit headers are server-controlled strings; a bare int() on one raises ValueError that propagates through pagination and kills the whole sync. parse_retry_after handles both RFC 9110 forms, so the HTTP-date that made int() a reachable crash now works.
  • GitHub gets _rate_limit_retry_delay, covering all three shapes it actually uses: primary limit (403 + x-ratelimit-remaining: 0, wait until reset), secondary limit (429 + Retry-After with a non-zero remaining, which a remaining == 0 test can never fire for), and ordinary 5xx. A 403 with quota remaining is an authorization failure and is deliberately not retried, so "token lacks this scope" surfaces immediately rather than stalling.
  • Slack passes before_attempt=self._throttle, keeping retries inside its global rate cap instead of answering a 429 storm with an unthrottled burst.
  • The helper returns the final response rather than raising, so each client keeps its own raise_for_status() and body-level error handling (Slack's ok: false → SlackAuthError).

embed_query() — #67

Raises ValueError on blank input instead of an IndexError from subscripting embed()'s empty list.

The issue offered two contracts; this takes the raise branch. The zero-vector alternative is actively dangerous: a zero dense vector has undefined cosine similarity against every chunk, so a blank query would return arbitrary results that look like genuine hits. Batch embed() keeps substituting zero vectors — there, position must be preserved so results line up with inputs, and one empty file shouldn't abort a 10k-file index run. Same blank input, different right answer, because the callers differ.

Per the issue's note to check callers: devrag search now rejects a blank query as a usage error.

--since — #69

Six copies of int(since.rstrip("d")) replaced with a shared _parse_since() raising typer.BadParameter (usage error, exit 2). The old form crashed with a raw traceback on 90days / 3w / abc, and silently accepted 90dd as 90.

Each command now calls it as its first statement. Previously the parse ran after the Qdrant store and embedding models were built, so a typo cost full startup before crashing:

$ devrag index prs acme/repo --since 3w
Usage: devrag index prs [OPTIONS] REPO
Error: invalid --since value '3w'; expected a day count like '90d'

Notes on scope

  • MCP search() left unguarded. A blank query there raises the ValueError, whose message already names the cause and which FastMCP surfaces as a tool error. Guarding it would need new test infrastructure (there is no test_mcp_server.py) for marginal gain.
  • No YAML knob for GitHub/Jira max_retries. Both take a constructor arg defaulting to 5 (up from an effective 1). slack.max_retries and slite.max_retries already had config surface; adding it for the other two wasn't in scope of any issue.

Testing

Written test-first, each watched fail before implementing. 60 new tests: 18 for the retry helper, 9 GitHub, 4 Slack, 6 Jira (new tests/test_jira_client.py), 3 embedder, 20 CLI.

426 passed, 2 skipped

One pre-existing test changed: tests/test_slite_client.py patched slite_client.time.sleep, and sleeping moved into the shared helper.

CLAUDE.md and README.md updated; the two max_retries comments in config.py now say "transport-error" rather than "timeout", matching what they mean.

@tomharris
fix: share one HTTP retry loop and validate CLI/query input
c6f6457 
Addresses five audit-tagged correctness issues.

Retries (#68, #70, #71) — GitHubClient, JiraClient, SlackClient and
SliteClient each carried their own retry loop and the copies had drifted:
Jira retried once, GitHub retried once and only on primary rate limits,
Slack and Slite were byte-identical duplicates. Extract one
request_with_retries() into devrag/utils/http.py and move all four onto
it. The loop owns the mechanics; per-service policy is injected as a
retry_delay(resp, attempt) callback.

- Catch httpx.TransportError, not httpx.TimeoutException. ConnectError,
  ReadError, WriteError and RemoteProtocolError are siblings of
  TimeoutException rather than subclasses, so the narrow catch let the
  most common network faults abort a sync on first occurrence.
- Parse rate-limit headers via safe_int()/parse_retry_after() instead of
  bare int(). Those headers are server-controlled strings, and RFC 9110
  allows Retry-After to be an HTTP-date, so int() was a reachable crash
  that propagated through pagination and killed the whole sync.
- GitHub gets _rate_limit_retry_delay, covering all three shapes it
  actually uses: primary limit (403 + x-ratelimit-remaining: 0),
  secondary limit (429 + Retry-After with non-zero remaining, which a
  remaining == 0 test can never catch), and 5xx. A 403 with quota
  remaining is an authorization failure and stays unretried so a missing
  token scope surfaces immediately.
- SlackClient passes before_attempt=self._throttle so retries stay inside
  its global rate cap instead of answering a 429 storm with a burst.
- The helper returns the final response rather than raising, so each
  client keeps its own raise_for_status() and body-level handling
  (Slack's ok: false -> SlackAuthError).

embed_query (#67) — raise ValueError on blank input instead of an
IndexError from subscripting embed()'s empty list. A zero vector has
undefined cosine similarity against every chunk, so a blank query would
return arbitrary results that look like genuine hits. Batch embed()
keeps substituting zero vectors: there position must be preserved and one
empty file must not abort an index run. devrag search guards blank
queries as a usage error.

--since (#69) — replace six copies of int(since.rstrip("d")) with a
shared _parse_since() raising typer.BadParameter. The old form crashed
with a raw traceback on 90days/3w/abc and silently accepted 90dd as 90.
Each command now calls it as its first statement, so a typo fails
instantly instead of after the vector store and embedding models load.

Tests: 60 new across the retry helper, all four clients, the embedder and
the CLI. Suite at 426 passed, 2 skipped.
@tomharris
tomharris merged commit 31d738b into main Jul 30, 2026
@tomharris
tomharris deleted the fix/audit-http-retries-and-input-validation branch July 30, 2026 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment