code-review

name code-review description Review a PR or working diff against this repo's intent layer (the AGENTS.md hierarchy), toolception pitfalls, and core invariants. Use when reviewing code, auditing a PR, or checking changes before merge in the Financial Modeling Prep MCP server. Triggers on: "review", "code review", "review this PR", "review the diff", "audit", "check this diff", "before merge", "pull request review", "PR review". allowed-tools Read, Grep, Glob, Bash(git:*), Bash(gh pr:*), Bash(gh api:*), Bash(gh search:*), Skill Code Review — FMP MCP Server You are reviewing changes to this repository. This server exposes 253+ read-only financial-data tools over MCP (HTTP/SSE) using toolception + Fastify. Most of the load-bearing rules here are non-obvious and live in the intent layer (the AGENTS.md hierarchy), not in the code. A generic review misses them. Your job is to catch real bugs and any violation of this repo's documented invariants. Work through the steps in order. Do not skip step 1 or step 3 — nor step 4 when the diff touches the toolception integration surface. 1. Load behavioral guidelines Invoke the karpathy-guidelines skill first (via the Skill tool) and review through its four lenses: Surgical changes — every changed line should trace to the PR's stated purpose. Flag drive-by refactors, reformatting, or "improvements" to untouched code. Simplicity — flag speculative abstraction, unused config/flexibility, and error handling for impossible states. Surfaced assumptions — flag silent decisions where multiple interpretations existed. Verifiable success — flag behavior changes that arrive without a test proving them. 2. Scope the diff Determine what changed. Given a PR reference — CI passes it as owner/repo/pull/N . Extract the number N and run gh pr diff N (the repo is inferred from the checkout); a full PR URL also works. Otherwise (local working branch) — diff against main : git diff --merge-base main --stat git diff --merge-base main List the changed files and collapse them to the source directories touched — that set drives step 3. 3. Load the intent layer (live read — do not rely on memory) Always read the root AGENTS.md (it defines the intent layer and its maintenance rules). Then, for each touched directory, read its matching doc. Read the area's FLOW.md too when one exists. Loading is hierarchical and T-shaped: root + the specific node. Changed path Read src/ (startup, index.ts , Fastify wiring) docs/src/AGENTS.md , docs/src/FLOW.md src/api/** docs/src/api/AGENTS.md src/tools/** docs/src/tools/AGENTS.md src/toolception-adapters/** docs/src/toolception-adapters/AGENTS.md src/server-mode-enforcer/** docs/src/server-mode-enforcer/AGENTS.md src/endpoints/** docs/src/endpoints/AGENTS.md , docs/src/endpoints/FLOW.md __tests__/smoke/** docs/tests/smoke/AGENTS.md Adding a tool/module/tool set docs/GUIDE.md (the procedure to follow) Directories without their own doc ( src/constants , src/prompts , src/schemas , src/types , src/utils ) inherit the root AGENTS.md plus the nearest parent that does have one — review them on general dimensions (step 5) and the invariants in scope. The rules you cite in findings come from the docs you just read — quote the actual Key Rule / Anti-pattern / Pitfall , do not paraphrase from this skill. 4. Toolception deep-check (only when the diff touches the integration surface) toolception is the core MCP layer, but node_modules/toolception ships compiled dist/ only — the readable source and the real API contracts live upstream at the public repo code-rabi/toolception . When the diff touches the integration surface, verify it against the actual upstream source , not memory. Trigger when the diff touches any of: src/toolception-adapters/** , src/index.ts (the createMcpServer config), src/endpoints/*.ts ( defineEndpoint ), src/prompts/** (the McpServer.prompt() extension), or bumps toolception in package.json / package-lock.json . Fetch the source with gh , pinned to the installed version: Read the installed version from node_modules/toolception/package.json (the version field) with the Read tool. The matching tag is v<version> (e.g. v0.6.3 ). Pull the relevant source — raw, no base64 decode. Substitute that version into ref : gh api -H "Accept: application/vnd.github.raw" "/repos/code-rabi/toolception/contents/src/index.ts?ref=v<version>" gh api "/repos/code-rabi/toolception/contents/src/<area>?ref=v<version>" --jq '.[].name' # list a dir gh search code --repo code-rabi/toolception "<symbol>" # locate a symbol Offline fallback for the type contracts: node_modules/toolception/dist/index.d.ts . Fetch the area that matches the change: Diff touches Fetch from toolception src/ createMcpServer config ( src/index.ts ) src/index.ts , src/server/** , src/types/** startup mode / toolsets ( ModeConfigMapper ) src/mode/** exposure policy / namespacing / allowlist / maxActiveToolsets src/permissions/** session config / config query param / cache key src/session/** defineEndpoint / custom endpoints / Fastify app src/http/** ModuleLoader / McpToolDefinition shapes src/types/** McpServer.prompt() extension ( src/prompts/** ) src/server/** meta-tools (enable/disable/list toolsets) src/meta/** Verify the diff against the fetched contracts: The config this repo passes to createMcpServer ( catalog , moduleLoaders , startup{mode, toolsets} , context , sessionContext{queryParam} , exposurePolicy{namespaceToolsWithSetKey, maxActiveToolsets?, allowlist?} , createServer , http ) still matches the upstream CreateMcpServerOptions type — flag renamed/removed/ retyped keys. ModuleLoader is still (context?) => McpToolDefinition[] | Promise<McpToolDefinition[]> and the McpToolDefinition shape the adapters / ToolCollector emit still matches upstream. McpServer.prompt() still exists upstream — a runtime extension reached via type cast that silently no-ops if dropped ( list_mcp_assets then vanishes with no error). On a toolception version bump: fetch the new tag's source and re-verify every contract above. Flag any breaking rename/removal/retype (precedent: initialToolsets → toolsets in 0.5.1). Don't restate the intent layer — docs/src/toolception-adapters/AGENTS.md and docs/src/AGENTS.md / FLOW.md already hold this repo's toolception pitfalls; cite them. Upstream toolception ships its own AGENTS.md files too (e.g. src/session/AGENTS.md ) — fetch them for its documented intent when a change is subtle. 5. Review across dimensions For each, cite the specific source ( file:line for code, and the exact rule from the loaded AGENTS.md for invariants). A. General correctness & security. Logic bugs, unhandled edge cases and error paths, secret/token exposure in logs or responses, injection, and obvious performance traps. This makes the skill a complete reviewer, not just an overlay. B. Intent-layer invariants & toolception pitfalls. Validate the diff against the rules in the docs loaded in step 3. The recurring high-cost ones to anchor on: API key travels as the ?apikey= query param — never a header ( docs/src/api/AGENTS.md ). Token precedence is Instance (constructor-injected session token) > Environment; don't invert it, and don't reintroduce a per-request "context" tier. Tools never throw — handlers return { content: [...], isError: true } , message formatted Error: ${message} ( docs/src/tools/AGENTS.md ). Tool names are globally unique across all modules — a duplicate silently overwrites. Toolception integration pitfalls (session cache key, base64 config query param, MODULE_ADAPTERS ↔ TOOL_SETS sync, no session-level toolset config) live in docs/src/toolception-adapters/AGENTS.md — and when the integration surface is touched, run the step 4 deep-check against upstream source. Startup order ( docs/src/AGENTS.md / FLOW.md ): the preHandler hook is registered before createMcpServer() ; a custom Fastify app must call app.listen() . Server-mode enforcer : initialize() precedes getInstance() ; invalid tool sets fail-fast via process.exit(1) . Don't infer request success from HTTP status — FMP returns error bodies with HTTP 200. C. AGENTS.md sync. The intent layer's own maintenance rule (root AGENTS.md ): if the diff changes behavior documented in an AGENTS.md , that file must be updated in the same PR . If a touched area's documented rule no longer matches the code and its AGENTS.md is untouched, flag it. (Also: a new major directory with distinct concerns should add an AGENTS.md wired into the navigation table.) D. TypeScript standards. Invoke the typescript-standards skill (Skill tool) and apply it to the changed .ts files; report what it flags. It covers no- any (prefer unknown ), TSDoc on exports, pure functions / single responsibility, readonly , and preferring interface — defer to the skill for the exact rules rather than restating them here. E. Test expectations. Per docs/tests/smoke/AGENTS.md : smoke tests run dist/ so a build must precede them; responses are SSE ( event: message\ndata: {json} ) — never parsed as raw JSON; resetSession() runs between tests (global clientId / sessionId ). New tools or modules should ship with tests. 6. Output Report high-signal findings only. For each: Severity — Blocker / High / Medium / Low. Location — file:line . Rule — the exact invariant or bug, with its citation (e.g. docs/src/tools/AGENTS.md → never-throw ). Fix — concrete and minimal. Do not raise nits that trace to neither a documented rule nor a real defect. If the diff is clean, say so and name what you checked. Group findings by severity, Blockers first.