Conversation
…ocs`
Adds npm package documentation alongside the existing docs.rs path, indexed
per exported symbol so search returns the one signature an agent needs rather
than a whole README.
The MCP tool is renamed `flare_docs` -> `docs` (`mcp__flare__docs`). Omitting
`ecosystem` still means Rust, so every call written before this keeps working;
scoped names (`@scope/pkg`) infer npm.
Extraction is tree-sitter over the package's TypeScript declaration files, not
deno_doc. Measured, deno_doc + deno_graph costs 100 net-new crates and +4.78 MB
of binary, and still leaves Node module resolution to the caller -- a probe
built from deno's own examples/ddoc panics with
`Resolve("Failed resolving '../../hono'")` on any multi-file package. This route
costs 4 crates (flate2/tar were already vendored). A .d.ts is fully explicit, so
there is no inference a type-checker would add.
Packages are fetched as a whole tarball rather than file-by-file, which turns
`export * from './x'` into a local path probe instead of a network round trip.
Three things real packages forced that synthetic fixtures did not:
- Declaration files spell members `method_signature` / `property_signature` /
`public_field_definition`; a query written against implementation files
(`method_definition`) matches nothing inside a .d.ts class body.
- `statement_block` is the body of both a function and a namespace, so
namespace members need distinguishing by parent kind or they are dropped.
- DefinitelyTyped packages use ambient `declare` plus a trailing `export = x`
with no `export` keyword anywhere, so an export-statement-only reachability
check discards their entire API -- exactly the packages the @types fallback
exists to serve. express went from 1 to 32 indexed items once fixed.
Verified: 1495 workspace tests pass; live fetches index hono per symbol and
resolve express through @types/express.
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds npm package documentation support through manifest and tarball processing, TypeScript API extraction, storage reconciliation, ecosystem-aware cache paths, and CLI/MCP routing while retaining Rust documentation behavior. ChangesNpm documentation integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DocsTool
participant Ecosystem
participant NpmRegistry
participant DocsStore
Client->>DocsTool: Request package documentation
DocsTool->>Ecosystem: Resolve registry
Ecosystem-->>DocsTool: Return Rust or Npm
DocsTool->>DocsStore: Check ecosystem-specific cache path
DocsTool->>NpmRegistry: Fetch manifest and tarball
NpmRegistry-->>DocsTool: Return package metadata and declarations
DocsTool->>DocsStore: Store overview and reconciled API items
DocsStore-->>Client: Return documentation result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
crates/flare-docs/src/npm/mod.rs (1)
1-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment overstates scope: no JSR support exists in this ecosystem.
The header says "npm (and, by extension, JSR) ecosystem support," but
Ecosystem(crates/flare-docs/src/ecosystem.rs) only hasRust/Npmvariants, and the PR objectives explicitly note JSR support isn't included. If the intent is "JSR packages that are also mirrored to npm happen to work via this path," consider wording it that way to avoid implying dedicated JSR handling exists.🤖 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 `@crates/flare-docs/src/npm/mod.rs` around lines 1 - 9, Update the module-level documentation in the npm module to remove the claim of direct JSR ecosystem support. Describe this as npm support only, or clarify that JSR packages may work only when mirrored through npm, while leaving the fetch_package and store_package API description unchanged.src/cli/docs.rs (1)
34-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Refresh's--ecosystemhelp text drops the defaulting explanation.
Get's help text (Lines 22-25) explains "Defaults to rust; scoped names (@scope/pkg) imply npm," butRefresh's doesn't, even though the sameresolve_ecosystemdefaulting logic applies to both. A user running--helponrefreshwon't see this.📝 Proposed fix
/// Registry to look the package up in: rust (docs.rs) or npm. + /// Defaults to rust; scoped names (`@scope/pkg`) imply npm. #[arg(long, short = 'e')] ecosystem: Option<String>,🤖 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/docs.rs` around lines 34 - 36, Update the `Refresh` command’s `ecosystem` argument help text in `src/cli/docs.rs` to match `Get`’s explanation: state that it defaults to rust and that scoped package names (`@scope/pkg`) imply npm, while preserving the existing registry description.src/mcp_server/flare_docs.rs (1)
115-173: 🩺 Stability & Availability | 🔵 TrivialTimed-out blocking fetches keep running in the background.
tokio::time::timeoutaroundspawn_blockingreturns early on timeout, but the underlying blocking task isn't cancelled — a timed-out npm fetch (manifest → possibly@typesmanifest → tarball, all sequential per the docstring) keeps occupying a blocking-pool thread and outbound connection until it eventually finishes or errors on its own. Under repeated timeouts (e.g. a slow/unreachable registry) this can gradually exhaust the blocking thread pool. Worth keeping in mind for the timeout value and blocking-pool sizing; no change needed if this is already an accepted tradeoff.🤖 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/mcp_server/flare_docs.rs` around lines 115 - 173, Update blocking_fetch to account for spawn_blocking tasks continuing after FETCH_TIMEOUT; avoid relying on tokio::time::timeout as cancellation, and ensure timed-out registry work cannot indefinitely occupy the blocking pool or outbound connections. Preserve the existing timeout error and panic/fetch error reporting behavior.
🤖 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 `@crates/flare-docs/src/npm/extract.rs`:
- Around line 297-322: Update relative_imports to parse import/export
declarations across line boundaries instead of requiring the declaration keyword
and from clause on the same line. Preserve detection of quoted relative
specifiers, deduplication, and skipping non-import/export content while handling
formatted multiline named imports and exports.
In `@crates/flare-docs/src/npm/mod.rs`:
- Around line 64-76: Update the DefinitelyTyped fallback in the manifest-loading
flow to preserve the underlying error from fetcher.fetch instead of mapping
every failure to NpmFetchError::NoTypes. Keep genuine missing-types handling
distinct from transport, timeout, DNS, and other fetch failures, while retaining
the existing types_package_name and manifest parsing flow.
---
Nitpick comments:
In `@crates/flare-docs/src/npm/mod.rs`:
- Around line 1-9: Update the module-level documentation in the npm module to
remove the claim of direct JSR ecosystem support. Describe this as npm support
only, or clarify that JSR packages may work only when mirrored through npm,
while leaving the fetch_package and store_package API description unchanged.
In `@src/cli/docs.rs`:
- Around line 34-36: Update the `Refresh` command’s `ecosystem` argument help
text in `src/cli/docs.rs` to match `Get`’s explanation: state that it defaults
to rust and that scoped package names (`@scope/pkg`) imply npm, while preserving
the existing registry description.
In `@src/mcp_server/flare_docs.rs`:
- Around line 115-173: Update blocking_fetch to account for spawn_blocking tasks
continuing after FETCH_TIMEOUT; avoid relying on tokio::time::timeout as
cancellation, and ensure timed-out registry work cannot indefinitely occupy the
blocking pool or outbound connections. Preserve the existing timeout error and
panic/fetch error reporting behavior.
🪄 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: 727d6b1e-579e-42d9-9e8a-a6e03a85a6ce
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
crates/flare-docs/Cargo.tomlcrates/flare-docs/src/ecosystem.rscrates/flare-docs/src/lib.rscrates/flare-docs/src/npm/extract.rscrates/flare-docs/src/npm/fetch.rscrates/flare-docs/src/npm/mod.rscrates/flare-docs/src/rustdoc.rscrates/flare-docs/tests/npm_real_package.rssrc/cli/docs.rssrc/mcp_server.rssrc/mcp_server/flare_docs.rssrc/mcp_server/types.rssrc/rule_text.rs
…ookup Two CodeRabbit findings on #342. The DefinitelyTyped fallback mapped every fetch failure to NoTypes, so a timeout, DNS failure, or rate limit was reported as "this package ships no TypeScript types" -- sending a caller to diagnose the wrong thing on a retryable blip. FetchError gains a NotFound variant, returned only for a 404, and the fallback maps just that to NoTypes while propagating everything else as itself. relative_imports scanned line by line, so a named import whose specifier list wraps -- the shape formatters produce -- put on a line that did not start with import/export and was skipped entirely. Wrapped lines now accumulate into one logical statement, keeping the keyword anchor so prose in doc comments still cannot masquerade as an import. The helper has no production caller yet, so this is a latent bug rather than a live one; fixing beats deleting because whoever wires it up next inherits a correct parser instead of a silently lossy one.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/flare-docs/src/fetch.rs`:
- Around line 86-88: Update the fetch error mapping around the
`call().map_err(...)` expression to detect `ureq::Error::Status(404, _)` and
return `FetchError::NotFound` directly, while preserving existing HTTP error
handling for other failures. Ensure the later status-based `NpmError::NoTypes`
mapping remains reachable for missing packages.
🪄 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: 0c504d70-f7e2-4a1a-9a25-9a3bb806529e
📒 Files selected for processing (3)
crates/flare-docs/src/fetch.rscrates/flare-docs/src/npm/extract.rscrates/flare-docs/src/npm/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/flare-docs/src/npm/extract.rs
- crates/flare-docs/src/npm/mod.rs
Comment on lines
+86
to
+88
| if status == 404 { | ||
| return Err(FetchError::NotFound); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
sed -n '1,180p' crates/flare-docs/src/fetch.rsRepository: getappz/agentflare
Length of output: 4883
🏁 Script executed:
set -euo pipefail
nl -ba crates/flare-docs/src/fetch.rs | sed -n '1,220p'Repository: getappz/agentflare
Length of output: 196
Handle ureq::Error::Status(404, _) in the error mapping
call().map_err(|e| FetchError::Http(e.to_string()))? turns 404s into FetchError::Http before the later status check runs, so missing packages never reach FetchError::NotFound. Match the 404 status in the map_err closure, or otherwise preserve the status for the downstream NpmError::NoTypes mapping.
🤖 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 `@crates/flare-docs/src/fetch.rs` around lines 86 - 88, Update the fetch error
mapping around the `call().map_err(...)` expression to detect
`ureq::Error::Status(404, _)` and return `FetchError::NotFound` directly, while
preserving existing HTTP error handling for other failures. Ensure the later
status-based `NpmError::NoTypes` mapping remains reachable for missing packages.
This was referenced Jul 26, 2026
Merged
This was referenced Jul 27, 2026
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
1 participant
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.
Closes agentflare item #373.
Adds npm package documentation to
flare-docsalongside the existing docs.rs path, and renames the MCP toolflare_docs→docs(mcp__flare__docs).Why
flare_docsonly knew about Rust crates, so any non-Rust project got nothing. npm has no docs.rs equivalent — nobody publishes machine-readable API docs per package — so the API surface has to be derived from what packages do ship: their TypeScript declarations.Why tree-sitter and not
deno_docdeno doc --json npm:<pkg>looked like the obvious answer, and a spike confirmed the output shape is a good match for our existing store. Using thedeno_doccrate is not, and the numbers are why:deno_doc+deno_graphdeno docCLI subprocessThe decisive part is the last column. A probe built from deno's own
examples/ddocshape parses a self-contained.d.tsfine but panics on any multi-file package:Node-style resolution (extension probing,
exportsmaps,node_moduleslayout) lives in the deno CLI, not the crate. Sodeno_doccosts 25× the dependencies and still leaves the hard part to us. Since a.d.tsis fully explicit — every type written out, nothing inferred — a syntactic pass recovers the same surface.flate2/tarwere already vendored, so the tarball path adds nothing.Design notes
Fetching the whole tarball (rather than file-by-file over unpkg) is deliberate: it's one request per package, and it turns
export * from './x'into a local path probe instead of a network round trip.Ecosystemis an enum, not a trait — two variants, dispatch happens once per request, and the fetch protocols differ enough (one zstd JSON document vs. a manifest plus a tarball) that a shared trait would be a lowest-common-denominator abstraction over two genuinely different things.Back-compat: omitting
ecosystemstill means Rust, so every existing call is unaffected. Scoped names (@scope/pkg) infer npm.rule_text::FLARE_DOCS_SUPERSEDEDnow carries the pre-rename rule body soagentflare initreplaces installed rules pointing at the old tool name.Three things real packages forced that fixtures didn't
An integration test runs over real published tarballs (fixtures in gitignored
.refs/npm/; tests skip when absent). It caught all three:.d.tsusesmethod_signature/property_signature/public_field_definition; implementation files usemethod_definition. A query written for.tsmatches nothing inside a.d.tsclass body — every class collapses to one opaque node.statement_blockis the body of both a function and a namespace. Treating it uniformly as an execution scope silently drops all namespace members.exportkeyword at all. They use ambientdeclare function e()/declare namespace e { var json }plus a trailingexport = e. An export-statement-only reachability check discards their entire API — exactly the packages the@typesfallback exists to serve. express went from 1 → 32 indexed items once fixed.Verification
cargo test --workspace— 1495 passed, 0 failedcargo fmt --all— clean--workspace -D warningsfails on master independently of this PR: five files (agents.rs,gateway_secrets.rs,github/identity.rs,mcp_server/tests/asset_tests.rs,paths.rs) carry pre-existingunsafe_codelints..email()), hono >100 members.Not in scope
JSR. The seam is in place and the extractor query already covers
.tsimplementation nodes, so it's an addition rather than a rewrite.Summary by CodeRabbit
.d.tsextraction (plus DefinitelyTyped fallback when bundled types are missing).FetchOutcomeis now surfaced from the crate root for consistent indexing results.