fix(flare-docs): caller-vs-service error mapping, search limit cap, non-blocking-fetch test

[getappz]

Addresses the actionable subset of the six Minor findings from the PR #316 whole-branch review (agentflare item #327).

Four findings needed code. Two were already fixed by later work and are reported below rather than re-done.

1. A 404 was reported as internal_error instead of invalid_params

Root cause was structural: ureq::Error::Status was flattened into a string, so blocking_fetch had no way to tell "you asked for a package that does not exist" from "the registry is down".

FetchError gains a structured variant carrying the code:

/// A response arrived carrying a non-2xx status. Split out from
/// [`Self::Http`] so callers can tell "the server said this does not
/// exist" from "the request never got an answer" ...
#[error("http status {0}")]
Status(u16),

and a ClientError trait lets the MCP layer discriminate, implemented per error type because only the error knows which of its variants are caller-caused:

.map_err(|e| {
    let msg = format!("{e} — {}", eco.other_ecosystem_hint(package));
    if e.is_client_error() { ErrorData::invalid_params(msg, None) }
    else { ErrorData::internal_error(msg, None) }
})

4xx and npm's NoTypes are caller-caused; 5xx, transport errors, timeouts and task panics stay internal_error.

This also subsumes the FetchError::NotFound variant added during #342's review — one variant carrying the status code preserves that timeout-vs-absence distinction and adds 4xx-vs-5xx, so the npm @types fallback now matches FetchError::Status(404).

2. limit was unbounded

Capped at 50 in DocsStore::search rather than at each caller, since the MCP tool and the CLI both route through it and two separate guards would drift:

pub const MAX_SEARCH_LIMIT: usize = 50;

pub fn search(&self, query: &str, limit: usize) -> Result<Vec<DocMatch>, Error> {
    let limit = limit.min(MAX_SEARCH_LIMIT);
    ...
}

Schema description and CLI --limit help updated to state the cap.

5. The non-2xx re-check in UreqFetcher::fetch is not dead

The review suggested removing it or documenting it. It turns out not to be dead: ureq only auto-errors on status >= 400, so a 1xx/3xx response (redirect budget exhausted, or an agent configured not to follow redirects) still arrives as Ok. Documented as deliberate and switched to return the same structured Status variant.

6. No committed regression test for the spawn_blocking fix

The "a slow fetch does not freeze the MCP server" property (fixed in 83f76ad) was previously only proven by an ad-hoc uncommitted script. Now covered by a committed test that exercises the property directly through blocking_fetch, so no Fetcher-injection refactor was needed.

The test was verified to have teeth: temporarily making the fetch run inline fails it with runtime was blocked during the fetch: only 0 ticks elapsed. The mutation was then reverted.

Findings that needed no change

• Bump actions/checkout from 4 to 7 #3 (no zstd output cap) — already landed: MAX_COMPRESSED_BYTES / MAX_DECOMPRESSED_BYTES + read_capped, with three tests.
• Bump dirs from 5.0.1 to 6.0.0 #4 (CLI get --help overpromises caching) — now stale: Get has its own cache-checking arm via store.get_by_path, so "Fetch (or read from cache)" is accurate.

Verification

• cargo fmt --all --check clean
• cargo test --workspace: 821 passed, 8 failed

Those 8 failures are pre-existing and environmental, not from this change. They reproduce identically on pristine origin/master with the same command in the same setup (819 passed / 8 failed — the 2-test difference is exactly the two tests this PR adds). All 8 are tests that shell out to git, failing with:

flare-git-shim: recursion guard tripped (depth 3) -- refusing to spawn further.
This should never happen; please report it.

They only trip under the parallelism of a full --workspace run; a filtered run of the same tests passes. Since the shim itself asks for it to be reported, that is worth a separate issue — it is unrelated to this PR.

Summary by CodeRabbit

• New Features
  • Search and list operations now cap returned results at 50 across supported interfaces.
  • Improved fetch error classification to better distinguish invalid requests from service failures.
• Bug Fixes
  • Slow document fetching no longer blocks other server activity.
  • Error responses now map 404-style failures to invalid-parameter errors, while 5xx remain internal errors.
• Documentation
  • Updated CLI help and request schema text to reflect the 50-result maximum behavior.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Fetch failures now preserve HTTP status codes and classify 4xx errors for MCP responses. npm fallback handling uses status 404, while documentation searches and MCP lists cap results at 50 with updated CLI and schema metadata.

Changes

Fetch error classification

Layer / File(s)	Summary
Status-based fetch error contract crates/flare-docs/src/fetch.rs, crates/flare-docs/src/lib.rs	FetchError::Status(u16) replaces NotFound; ClientError classifies applicable 4xx statuses, and ureq status handling preserves HTTP codes.
npm error propagation crates/flare-docs/src/npm/mod.rs	NpmError implements client-error classification and maps DefinitelyTyped 404 responses to missing types.
MCP error mapping and execution validation src/mcp_server/flare_docs.rs	Client errors map to invalid parameters, other failures map to internal errors, and tests cover blocking execution, status mapping, and list limits.

Search result cap

Layer / File(s)	Summary
Search limit enforcement and contract documentation crates/flare-docs/src/store.rs, src/cli/docs.rs, src/mcp_server/flare_docs.rs, src/mcp_server/types.rs	Search and list limits are capped at 50, with tests and CLI/MCP descriptions documenting the behavior.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

• getappz/agentflare#342: Introduced the earlier 404-based fetch error handling that this change replaces with status-based classification.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main changes: error classification, search limit capping, and the blocking-fetch test.
Description check	✅ Passed	The description explains the changes, rationale, and verification, though it doesn't follow the template sections exactly.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch task/327

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@src/mcp_server/flare_docs.rs`:
- Around line 175-180: Update the error-message construction in the visible
error-handling branch so eco.other_ecosystem_hint(package) is not appended for
NpmError::Npm(NpmFetchError::NoTypes(_)). Restrict the cross-ecosystem hint to
genuine package-not-found errors, while preserving the existing
invalid_params/internal_error classification and messages for other failures.

In `@src/mcp_server/types.rs`:
- Around line 445-447: The limit field description incorrectly claims
enforcement for list operations. Update the schemars description on the limit
field in the request type to document only the search behavior unless the list
handler is also changed to enforce the limit; preserve the existing default and
maximum wording.

🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 52b49445-476c-4ba8-ac18-e5d11a810911

📥 Commits

Reviewing files that changed from the base of the PR and between b21047a and 4073d0c.

📒 Files selected for processing (7)

• crates/flare-docs/src/fetch.rs
• crates/flare-docs/src/lib.rs
• crates/flare-docs/src/npm/mod.rs
• crates/flare-docs/src/store.rs
• src/cli/docs.rs
• src/mcp_server/flare_docs.rs
• src/mcp_server/types.rs