diff --git a/CLAUDE.md b/CLAUDE.md index ea94d46..685838d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -102,12 +102,12 @@ experiments/ ## Testing ```bash -pytest # 553 unit/integration tests (no network) +pytest # 569 unit/integration tests (no network) pytest -m live # 6 live tests (hits jsonplaceholder + httpbin) pytest -v # Verbose output ``` -**Total: 559 tests (553 + 6 live), all passing in <8s.** +**Total: 575 tests (569 offline + 6 live; live tests require network access and are reported separately).** **Test files:** - `test_analyzer.py` — Traffic analysis & grouping (23 tests) @@ -150,7 +150,6 @@ pytest -v # Verbose output - `test_monitor.py` — Diff computation, watcher, webhook, registry CRUD (41 tests) - `test_screenshot.py` — Screenshot model, CLI help, formats (8 tests) -**Total: 500 tests (494 + 6 live), all passing.** ## Live Validation (8 Experiments) diff --git a/VALIDATION-REPORT.md b/VALIDATION-REPORT.md new file mode 100644 index 0000000..f6f8503 --- /dev/null +++ b/VALIDATION-REPORT.md @@ -0,0 +1,168 @@ +# site2cli v0.7.0 — Independent Validation Report + +**Date:** 2026-07-17 +**Method:** Fresh install in a clean Linux container (Python 3.11), full test suite, and a +live end-to-end exercise of the entire CLI surface against a purpose-built local test +website (HTML storefront backed by a JSON XHR API, multi-page site, robots.txt, mutable +content page) using a real headless Chromium via Playwright. + +The sandbox's network policy blocks arbitrary external hosts, so the 8 live experiments +(`experiments/`) and the 6 `@pytest.mark.live` tests could not reach their targets +(jsonplaceholder, httpbin, etc.). Everything else was validated for real — including full +browser-based CDP capture — against localhost. + +--- + +## Verdict + +**The core claim is real.** `site2cli discover` drove a real browser, captured the site's +XHR traffic over CDP, and produced — with zero LLM involvement (`--no-enhance`) — a valid +OpenAPI 3.1 spec with correctly templatized path parameters (`/api/users/{id}`), inferred +query and JSON-body parameters, a working Python client, a working zero-dep JS ES-module +client, an HTML coverage report with gap candidates, and a replayable trace. `site2cli run` +then executed those endpoints as direct HTTP calls (Tier 3) with no browser. The +progressive-formalization architecture is not vaporware. + +**But it's an alpha with real rough edges.** One headline command (`screenshot`) is +completely broken, generated MCP tool names violate the MCP spec, robots.txt handling +fails open, and several of the README's quantified claims are simulations or estimates, +not measurements. + +--- + +## What was verified working (against a live local site) + +| Area | Result | +|---|---| +| Unit tests | **553 passed, 1 skipped** (claim: 553) in 16.5s (claim: <8s; machine-dependent) | +| ruff lint | clean (`src/` + `tests/`) | +| `discover` (real browser, CDP capture) | 4/4 XHR endpoints captured; GET query params, POST JSON body params, path-ID templatization all correct | +| OpenAPI 3.1 output | validates with `openapi-spec-validator`; correct `servers` URL incl. port | +| Generated Python client | all 4 methods executed successfully against the live API (after working around naming bug #1) | +| Generated JS client (`client.mjs`) | loads as ES module in Node, exports `createClient` | +| HTML coverage report | contains endpoint table + gap candidates | +| Trace save/replay | `--save-trace` then `--replay` re-generates all artifacts fully offline | +| `run` (Tier 3 direct API) | path substitution, POST bodies, `--json/--compact/--keys-only/--grep/--limit` all work; success counters increment | +| `scrape` | correct main-content markdown extraction | +| `crawl` | BFS crawled all 6 pages, correct per-page markdown files, `--sitemap` mode works | +| `monitor` | baseline snapshot → content change → clean unified diff + watch registry. Works exactly as advertised | +| `chunk` | heading-strategy chunking correct | +| `mcp generate` + MCP protocol | spoke JSON-RPC to the generated server: `initialize`, `tools/list`, `tools/call` all execute real API calls end-to-end | +| `init` | valid Claude MCP config JSON | +| `community export` | bundle produced | +| `daemon` | JSON-RPC over Unix socket: status + graceful stop work | +| `sites list/show`, `config show`, `version` | work | + +Not testable in this environment: `search` (DuckDuckGo blocked), `extract` (needs +`ANTHROPIC_API_KEY`), LLM-enhanced discovery, auth/OAuth device flow against real +providers, experiments 8–15 (network policy). + +--- + +## Bugs found (all reproduced, with locations) + +1. **Generated Python client is syntactically invalid for digit-leading hosts** — + `src/site2cli/discovery/client_generator.py:50`. `_sanitize_name()` prefixes `_` for + names starting with a digit, but the following `.replace("_", "")` strips it back off: + `127.0.0.1` → `class 127001Client:` → `SyntaxError`. Affects any digit-leading domain + (e.g. `9gag.com`). Tier 3 then silently skips the broken client. Ironically, CLAUDE.md + documents a previous naming bug fix in this same function. + +2. **Registry stores the user's relative `-o` path as `openapi_spec_path`** — after + `discover ... -o spec.json`, running `site2cli mcp generate ` from any other + directory crashes with `FileNotFoundError: 'spec.json'`, and Tier 3 spec loading + breaks the same way. The spec path should be resolved to absolute (or the registry + copy under `~/.site2cli/specs/` should be authoritative). + +3. **Action-name mismatch + silent browser fallback** — actions are registered as + `GET_/api/products`, but `discover` prints `GET /api/products` and the spec/client use + `operationId: get_api_products`. Running the natural name doesn't error — the router + silently falls back to Tier 1 and navigates to `https://{domain}/` (dropping the + registered `base_url`'s scheme *and* port), surfacing as a confusing Playwright + `ERR_CONNECTION_REFUSED` traceback. Unknown actions should fail fast with the list of + valid names; the browser fallback should honor `base_url`. + +4. **robots.txt compliance fails open on non-standard ports** — + `src/site2cli/crawl/crawler.py:41` uses `parsed.hostname` (port stripped), so + `RobotsChecker` fetches `http://host/robots.txt` instead of `http://host:8777/robots.txt`; + the fetch fails and the checker allows everything (`load()` swallows all exceptions). + Verified: the crawler happily fetched `/private/secret.html` despite + `Disallow: /private/`. Standard-port sites are unaffected, but fail-open is the wrong + default for a compliance feature. + +5. **`screenshot` is completely broken in v0.7.0** — `src/site2cli/screenshot/capture.py:60` + calls `create_browser_context(proxy=..., profile=..., session=...)` but the factory + (`src/site2cli/browser/context.py:21`) accepts neither `proxy` nor `session` → + instant `TypeError` on every invocation. The 8 screenshot tests cover the model/help/ + formats but never invoke the capture path. + +6. **Generated MCP tool names violate the MCP spec** — `tools/list` returns names like + `127_get_api_users_{id}`. Tool names must match `^[a-zA-Z0-9_-]+$`; real clients + (Claude Desktop, Claude API) will reject these tools. Path-parameter braces must be + sanitized. Also, parameter typing is inconsistent for agents: query params are typed + `string` while JSON-body params are typed `integer` (e.g. `limit` vs `max_results`), + producing validation errors for the obvious call. + +7. **`daemon start` doesn't daemonize** — `cli.py:1852` runs the server in the foreground + despite the help text "Start the *background* browser daemon". + +8. **Health checks give false negatives** — `src/site2cli/health/monitor.py:63` probes GET + endpoints with `HEAD` (others with `OPTIONS`). Servers that don't implement HEAD + (common; Python's stdlib server returns 501) are marked BROKEN even when every real + call succeeds — observed here: all 4 endpoints FAIL while Tier 3 `run` works perfectly. + +--- + +## Claims audit + +| Claim | Verdict | +|---|---| +| "553 unit tests, all passing" | **True** (1 skipped) | +| Discover → spec → clients → MCP pipeline works | **True**, verified end-to-end with a real browser | +| "Four artifacts every run" | **True** (spec, .py client, .mjs client, HTML report) | +| "<1s per action (API)" after discovery | **True** for Tier 3 (~100ms locally) | +| "74% cheaper than browser-use" | **Simulation, not a measurement** — experiment 11 uses hardcoded cost constants ($0.05/run browser-use vs $0.0001 Tier 3, "based on industry data") | +| "2M x faster than manual" | **Arithmetic on an assumption** — experiment 10 divides an assumed 40 hours/API of manual effort by measured runtime | +| "Reliability ~15-35% on benchmarks vs >95%" | **Uncited** in README | +| "100% health check accuracy" | Measured against the experiment's own scenario set; real-world HEAD-based probes false-negative (bug #8) | +| "62% coverage of known unofficial APIs" | Plausible methodology (compares against documented endpoint lists) but not reproducible here (network) | +| CLAUDE.md internal consistency | Contradicts itself: "559 tests" in one place, "500 tests" in another | + +--- + +## Update: all 8 bugs fixed + +All bugs below were fixed on this branch and re-verified live against the same +local test site (plus 16 new regression tests; suite now 569 passing): + +1. Digit-leading hosts now generate `class Site127001Client:` (valid Python). +2. `discover` stores absolute spec/client paths; `mcp generate` also falls back + to `~/.site2cli/specs/` for legacy relative entries. +3. Action names are now canonical operationIds (`get_api_products`) shared by + the spec, registry, clients, and MCP tools; `discover` prints the runnable + name per endpoint; unknown actions fail fast listing valid names (with + normalized matching for legacy `GET_/path` registries). +4. robots.txt is fetched from the full `host:port` netloc; the crawler no + longer fetches `Disallow`ed paths on non-standard ports. +5. `screenshot` works (context factory gained a `proxy` override; capture now + passes `session_manager`/`session_name`). +6. MCP tool names are sanitized to `^[a-zA-Z0-9_-]+$`, and query params are + type-inferred (`limit=5` → integer) so tool schemas are consistent. +7. `daemon start` detaches by default (logs to `~/.site2cli/daemon.log`); + `--foreground` preserves the old behavior. +8. Health checks fall back to GET when a server answers 405/501 to + HEAD/OPTIONS — no more BROKEN reports for healthy endpoints. + +## How big a deal is it? + +The differentiated core — watch a site's own frontend traffic, formalize it into a typed, +agent-ready API surface (CLI + OpenAPI + clients + MCP), then never pay browser/LLM cost +again for those actions — **works as described and is genuinely useful**. That loop, plus +offline trace replay, is something Firecrawl-style scrapers don't do, and the local-first, +no-API-key posture is real. + +Tempering that: it's `0.7.0` alpha-quality at the edges (one command dead on arrival, MCP +names that real clients would reject, fail-open robots), the headline benchmark numbers +are projections rather than measurements, and discovery inherently only finds what the +frontend happens to call while you browse. Treat the engineering as solid and promising, +and the marketing numbers as directional. diff --git a/src/site2cli/__main__.py b/src/site2cli/__main__.py new file mode 100644 index 0000000..7aa7f8a --- /dev/null +++ b/src/site2cli/__main__.py @@ -0,0 +1,6 @@ +"""Allow running as ``python -m site2cli``.""" + +from site2cli.cli import app + +if __name__ == "__main__": + app(prog_name="site2cli") diff --git a/src/site2cli/browser/context.py b/src/site2cli/browser/context.py index 0207cf5..69e916b 100644 --- a/src/site2cli/browser/context.py +++ b/src/site2cli/browser/context.py @@ -24,6 +24,7 @@ async def create_browser_context( session_manager: Any | None = None, session_name: str | None = None, inject_cookies_for: str | None = None, + proxy: dict | None = None, ) -> AsyncIterator[tuple[Browser, BrowserContext, Page]]: """Create a browser context with cookies, profile, and session support. @@ -34,6 +35,7 @@ async def create_browser_context( session_manager: Optional SessionManager to reuse sessions. session_name: Session name for session_manager lookup. inject_cookies_for: Domain to inject stored cookies for. + proxy: Playwright proxy dict; overrides the configured proxy. Yields: Tuple of (browser, context, page). @@ -56,8 +58,8 @@ async def create_browser_context( browser: Browser | None = None try: - # Resolve proxy settings - proxy_config = config.proxy.get_playwright_proxy() + # Resolve proxy settings (explicit argument wins over config) + proxy_config = proxy if proxy is not None else config.proxy.get_playwright_proxy() if profile: # Profile mode: launch_persistent_context with user data dir diff --git a/src/site2cli/cli.py b/src/site2cli/cli.py index 6bb5fa5..6744051 100644 --- a/src/site2cli/cli.py +++ b/src/site2cli/cli.py @@ -99,7 +99,11 @@ def discover( generate_js_client, save_js_client, ) - from site2cli.discovery.spec_generator import generate_openapi_spec, save_spec + from site2cli.discovery.spec_generator import ( + generate_openapi_spec, + make_unique_operation_ids, + save_spec, + ) from site2cli.discovery.trace import load_trace, save_trace from site2cli.models import ( DiscoveredAPI, @@ -223,18 +227,15 @@ async def do_capture(): # Step 6: Register site registry = _get_registry() + action_names = make_unique_operation_ids(endpoints) actions = [ SiteAction( - name=( - ep.description.replace(" ", "_").lower() - if ep.description - else f"{ep.method}_{ep.path_pattern}" - ), + name=name, description=ep.description, tier=Tier.API, endpoint=ep, ) - for ep in endpoints + for ep, name in zip(endpoints, action_names) ] site = SiteEntry( domain=domain, @@ -242,8 +243,8 @@ async def do_capture(): description=api.description, actions=actions, auth_type=auth_type, - openapi_spec_path=str(spec_path), - client_module_path=str(client_path), + openapi_spec_path=str(spec_path.resolve()), + client_module_path=str(client_path.resolve()), ) registry.add_site(site) @@ -253,10 +254,10 @@ async def do_capture(): f"[bold green]Discovered {len(endpoints)}" f" capabilities for {domain}:[/bold green]" ) - for ep in endpoints: + for ep, action_name in zip(endpoints, action_names): name = ep.description or f"{ep.method} {ep.path_pattern}" params = ", ".join(p.name for p in ep.parameters[:5]) - console.print(f" - {name} ({params})") + console.print(f" - {name} ({params}) [dim]run: {action_name}[/dim]") @app.command() @@ -306,6 +307,13 @@ def run( with console.status(f"[bold green]Executing {action} on {domain}..."): result = _run_async(router.execute(domain, action, param_dict)) + if isinstance(result, dict) and "error" in result and "available_actions" in result: + console.print(f"[red]{result['error']}[/red]") + console.print("Available actions:") + for name in result["available_actions"]: + console.print(f" - {name}") + raise typer.Exit(1) + # Apply output filters if grep or limit is not None or keys_only: from site2cli.output_filter import filter_result @@ -1495,7 +1503,22 @@ def mcp_generate( console.print("[red]No OpenAPI spec found for this site.[/red]") raise typer.Exit(1) - spec = load_spec(Path(site.openapi_spec_path)) + spec_path = Path(site.openapi_spec_path) + if not spec_path.exists(): + # Legacy registries stored relative paths; fall back to the specs dir + for ext in ("json", "yaml"): + candidate = config.specs_dir / f"{domain}.{ext}" + if candidate.exists(): + spec_path = candidate + break + else: + console.print( + f"[red]Spec file not found: {site.openapi_spec_path}[/red]" + f" Re-run `site2cli discover {domain}` to regenerate it." + ) + raise typer.Exit(1) + + spec = load_spec(spec_path) code = generate_mcp_server_code(site, spec) output_path = ( @@ -1849,16 +1872,61 @@ def init( @daemon_app.command("start") -def daemon_start() -> None: +def daemon_start( + foreground: bool = typer.Option( + False, "--foreground", "-f", help="Run in the foreground instead of detaching" + ), +) -> None: """Start the background browser daemon.""" - from site2cli.daemon.server import DaemonServer - config = get_config() - server = DaemonServer(config.daemon_socket_path) - console.print( - f"[green]Starting daemon on {config.daemon_socket_path}...[/green]" - ) - _run_async(server.run()) + + if foreground: + from site2cli.daemon.server import DaemonServer + + server = DaemonServer(config.daemon_socket_path) + console.print( + f"[green]Starting daemon on {config.daemon_socket_path}...[/green]" + ) + _run_async(server.run()) + return + + if config.daemon_socket_path.exists(): + from site2cli.daemon.client import DaemonClient + + client = DaemonClient(config.daemon_socket_path) + try: + _run_async(client.send("list_sessions", {})) + console.print("[yellow]Daemon is already running.[/yellow]") + return + except Exception: + # Stale socket from a previous run + config.daemon_socket_path.unlink() + + import subprocess + import sys + import time + + log_path = config.data_dir / "daemon.log" + with open(log_path, "ab") as log: + subprocess.Popen( + [sys.executable, "-m", "site2cli", "daemon", "start", "--foreground"], + stdout=log, + stderr=log, + stdin=subprocess.DEVNULL, + start_new_session=True, + ) + + for _ in range(40): + if config.daemon_socket_path.exists(): + console.print( + f"[green]Daemon started on {config.daemon_socket_path}[/green]" + f" (log: {log_path})" + ) + return + time.sleep(0.25) + + console.print(f"[red]Daemon did not start within 10s; see {log_path}[/red]") + raise typer.Exit(1) @daemon_app.command("stop") diff --git a/src/site2cli/crawl/crawler.py b/src/site2cli/crawl/crawler.py index b55764e..d43a7ff 100644 --- a/src/site2cli/crawl/crawler.py +++ b/src/site2cli/crawl/crawler.py @@ -39,6 +39,8 @@ def __init__( self.start_url = normalize_url(start_url) or start_url parsed = urlparse(self.start_url) self.domain = parsed.hostname or "" + # netloc keeps the port (host:8080) — required for robots.txt fetching + self.netloc = parsed.netloc or self.domain self.scheme = parsed.scheme or "https" self.max_depth = max_depth self.max_pages = max_pages @@ -74,7 +76,7 @@ async def crawl(self) -> AsyncIterator[CrawlPage]: """Crawl the site using BFS, yielding pages as they're crawled.""" # Load robots.txt if self.respect_robots: - self._robots = RobotsChecker(self.domain, self.scheme) + self._robots = RobotsChecker(self.netloc, self.scheme) await self._robots.load() # BFS queue: (url, depth) diff --git a/src/site2cli/crawl/robots.py b/src/site2cli/crawl/robots.py index 96700e8..676b52b 100644 --- a/src/site2cli/crawl/robots.py +++ b/src/site2cli/crawl/robots.py @@ -19,9 +19,14 @@ def __init__(self, domain: str, scheme: str = "https") -> None: self._sitemaps: list[str] = [] self._loaded = False + @property + def robots_url(self) -> str: + """URL robots.txt is fetched from (domain may include a port).""" + return f"{self._scheme}://{self._domain}/robots.txt" + async def load(self) -> None: """Fetch and parse robots.txt from domain.""" - url = f"{self._scheme}://{self._domain}/robots.txt" + url = self.robots_url try: async with httpx.AsyncClient(follow_redirects=True, timeout=10) as client: resp = await client.get(url) diff --git a/src/site2cli/discovery/analyzer.py b/src/site2cli/discovery/analyzer.py index a9feb84..76db7f0 100644 --- a/src/site2cli/discovery/analyzer.py +++ b/src/site2cli/discovery/analyzer.py @@ -29,6 +29,24 @@ def _normalize_path(path: str) -> str: return path +def _infer_query_param_type(value: str | None) -> str: + """Infer a JSON Schema type from a query-string example value. + + Query values are always strings on the wire, but typing "limit=5" as + integer keeps generated clients and MCP tool schemas consistent with + body parameters (which are inferred from JSON). + """ + if value is None or value == "": + return "string" + if value.lower() in ("true", "false"): + return "boolean" + if re.fullmatch(r"-?\d+", value): + return "integer" + if re.fullmatch(r"-?\d+\.\d+", value): + return "number" + return "string" + + def _infer_json_schema(data: object) -> dict: """Infer a JSON Schema from a sample value.""" if data is None: @@ -112,7 +130,7 @@ def extract_endpoints(self) -> list[EndpointInfo]: ParameterInfo( name=param_name, location="query", - param_type="string", + param_type=_infer_query_param_type(example_val), required=False, example=example_val, ) diff --git a/src/site2cli/discovery/client_generator.py b/src/site2cli/discovery/client_generator.py index f005fc4..343bee2 100644 --- a/src/site2cli/discovery/client_generator.py +++ b/src/site2cli/discovery/client_generator.py @@ -48,6 +48,9 @@ def generate_client_code(spec: dict, class_name: str | None = None) -> str: title = info.get("title", "API") if not class_name: class_name = _sanitize_name(title.replace("API", "").strip()).title().replace("_", "") + if class_name and class_name[0].isdigit(): + # Identifiers can't start with a digit (e.g. "127.0.0.1" or "9gag.com" hosts) + class_name = f"Site{class_name}" class_name = f"{class_name}Client" if class_name else "APIClient" servers = spec.get("servers", []) diff --git a/src/site2cli/discovery/spec_generator.py b/src/site2cli/discovery/spec_generator.py index 9cc64c8..6065d02 100644 --- a/src/site2cli/discovery/spec_generator.py +++ b/src/site2cli/discovery/spec_generator.py @@ -3,11 +3,42 @@ from __future__ import annotations import json +import re from pathlib import Path from site2cli.models import DiscoveredAPI, EndpointInfo, ParameterInfo +def make_operation_id(endpoint: EndpointInfo) -> str: + """Build the canonical operation/action ID for an endpoint. + + Produces identifier-safe names like ``get_api_users_id`` so the same ID + works as an OpenAPI operationId, a registry action name, a generated + client method, and an MCP tool-name component. + """ + raw = endpoint.description or f"{endpoint.method} {endpoint.path_pattern}" + op_id = re.sub(r"[^a-zA-Z0-9]+", "_", raw.lower()).strip("_") + return op_id or "operation" + + +def make_unique_operation_ids(endpoints: list[EndpointInfo]) -> list[str]: + """Build collision-free operation IDs for a list of endpoints. + + Normalization is lossy (``/a-b`` and ``/a_b`` collapse to the same ID), + so duplicates get a deterministic ``_2``, ``_3``, ... suffix in endpoint + order. Callers that iterate the same endpoint list (spec generation and + registry action naming) therefore agree on every ID. + """ + seen: dict[str, int] = {} + ids = [] + for ep in endpoints: + base = make_operation_id(ep) + count = seen.get(base, 0) + 1 + seen[base] = count + ids.append(base if count == 1 else f"{base}_{count}") + return ids + + def _param_to_openapi(param: ParameterInfo) -> dict: """Convert a ParameterInfo to OpenAPI parameter or schema property.""" type_map = { @@ -35,12 +66,10 @@ def _param_to_openapi(param: ParameterInfo) -> dict: return schema -def _endpoint_to_openapi_path(endpoint: EndpointInfo) -> dict: +def _endpoint_to_openapi_path(endpoint: EndpointInfo, operation_id: str | None = None) -> dict: """Convert an EndpointInfo to an OpenAPI path item operation.""" operation: dict = { - "operationId": endpoint.description.replace(" ", "_").lower() - if endpoint.description - else f"{endpoint.method.lower()}_{endpoint.path_pattern.replace('/', '_').strip('_')}", + "operationId": operation_id or make_operation_id(endpoint), "summary": endpoint.description or f"{endpoint.method} {endpoint.path_pattern}", } @@ -106,12 +135,13 @@ def generate_openapi_spec(api: DiscoveredAPI) -> dict: } # Group endpoints by path - for endpoint in api.endpoints: + op_ids = make_unique_operation_ids(api.endpoints) + for endpoint, op_id in zip(api.endpoints, op_ids): path = endpoint.path_pattern if path not in spec["paths"]: spec["paths"][path] = {} method = endpoint.method.lower() - spec["paths"][path][method] = _endpoint_to_openapi_path(endpoint) + spec["paths"][path][method] = _endpoint_to_openapi_path(endpoint, op_id) # Add security schemes if any endpoint requires auth if any(ep.auth_required for ep in api.endpoints): diff --git a/src/site2cli/generators/mcp_gen.py b/src/site2cli/generators/mcp_gen.py index 5b32ae2..bdd30af 100644 --- a/src/site2cli/generators/mcp_gen.py +++ b/src/site2cli/generators/mcp_gen.py @@ -9,10 +9,21 @@ from site2cli.models import MCPToolSchema, SiteEntry, Tier +def _sanitize_tool_name(name: str) -> str: + """Sanitize to the MCP tool-name charset (``^[a-zA-Z0-9_-]+$``). + + OperationIds derived from paths can contain ``{`` ``}`` ``/`` etc., + which MCP clients reject. + """ + name = re.sub(r"[^a-zA-Z0-9_-]", "_", name) + return re.sub(r"_+", "_", name).strip("_") + + def _spec_to_mcp_tools(site: SiteEntry, spec: dict) -> list[MCPToolSchema]: """Convert OpenAPI spec operations to MCP tool schemas.""" tools = [] domain_prefix = re.sub(r"[^a-z0-9]", "_", site.domain.split(".")[0]) + seen_names: dict[str, int] = {} for path, path_item in spec.get("paths", {}).items(): for http_method, operation in path_item.items(): @@ -20,7 +31,13 @@ def _spec_to_mcp_tools(site: SiteEntry, spec: dict) -> list[MCPToolSchema]: continue op_id = operation.get("operationId", f"{http_method}_{path}") - tool_name = f"{domain_prefix}_{op_id}" + tool_name = _sanitize_tool_name(f"{domain_prefix}_{op_id}") + # Sanitization is lossy — suffix deterministically on collision + # so no tool handler shadows another. + count = seen_names.get(tool_name, 0) + 1 + seen_names[tool_name] = count + if count > 1: + tool_name = f"{tool_name}_{count}" description = operation.get("summary", op_id) # Build input schema from parameters + request body @@ -58,10 +75,12 @@ def _spec_to_mcp_tools(site: SiteEntry, spec: dict) -> list[MCPToolSchema]: "required": required, } - # Determine tier from site actions + # Determine tier from site actions (normalized: legacy registries + # may store "GET_/api/products" instead of "get_api_products") tier = Tier.API + wanted = _sanitize_tool_name(op_id).lower() for action in site.actions: - if action.name == op_id: + if _sanitize_tool_name(action.name).lower() == wanted: tier = action.tier break diff --git a/src/site2cli/health/monitor.py b/src/site2cli/health/monitor.py index 8909fc8..38a643b 100644 --- a/src/site2cli/health/monitor.py +++ b/src/site2cli/health/monitor.py @@ -66,6 +66,11 @@ async def _check_endpoint( else: response = await client.options(url) + # Many servers don't implement HEAD/OPTIONS (405/501) even + # though the endpoint itself is healthy — probe with GET. + if response.status_code in (405, 501): + response = await client.get(url) + if response.status_code < 400: return HealthStatus.HEALTHY elif response.status_code < 500: diff --git a/src/site2cli/router.py b/src/site2cli/router.py index a1cd7d6..39d52a8 100644 --- a/src/site2cli/router.py +++ b/src/site2cli/router.py @@ -2,6 +2,8 @@ from __future__ import annotations +import re + from site2cli.config import get_config from site2cli.models import SiteAction, SiteEntry, Tier from site2cli.registry import SiteRegistry @@ -37,7 +39,12 @@ async def execute( action = _find_action(site, action_name) if not action: - return await self._fallback_browser(domain, action_name, params) + # A known site with an unknown action is a user error: fail fast + # with the valid names instead of silently launching a browser. + return { + "error": f"Unknown action '{action_name}' for {domain}", + "available_actions": [a.name for a in site.actions], + } # Try the action's current tier, then fall back tiers_to_try = _tier_fallback_order(action.tier) @@ -45,11 +52,11 @@ async def execute( for tier in tiers_to_try: try: result = await self._execute_tier(site, action, tier, params) - self._registry.record_action_result(domain, action_name, success=True) + self._registry.record_action_result(domain, action.name, success=True) self._maybe_promote(domain, action) return result except Exception: - self._registry.record_action_result(domain, action_name, success=False) + self._registry.record_action_result(domain, action.name, success=False) continue return {"error": f"All tiers failed for {domain}/{action_name}"} @@ -103,11 +110,27 @@ def _maybe_promote(self, domain: str, action: SiteAction) -> None: self._registry.update_action_tier(domain, action.name, new_tier) +def _normalize_action_name(name: str) -> str: + """Normalize an action name for tolerant matching.""" + return re.sub(r"[^a-z0-9]+", "_", name.lower()).strip("_") + + def _find_action(site: SiteEntry, action_name: str) -> SiteAction | None: - """Find an action by name in a site entry.""" + """Find an action by name in a site entry. + + Matches exactly first, then falls back to normalized comparison so + operationId-style names (``get_api_products``) resolve against legacy + registry entries (``GET_/api/products``) and vice versa. + """ for action in site.actions: if action.name == action_name: return action + wanted = _normalize_action_name(action_name) + matches = [a for a in site.actions if _normalize_action_name(a.name) == wanted] + if len(matches) == 1: + return matches[0] + # Zero matches, or ambiguous (several actions normalize to the same + # name): let the caller report the available actions. return None diff --git a/src/site2cli/screenshot/capture.py b/src/site2cli/screenshot/capture.py index 7ed6190..c2f648a 100644 --- a/src/site2cli/screenshot/capture.py +++ b/src/site2cli/screenshot/capture.py @@ -57,8 +57,17 @@ async def take_screenshot( if fmt == "jpeg" and quality is not None: screenshot_kwargs["quality"] = quality + session_manager = None + if session: + from site2cli.browser.session import get_session_manager + + session_manager = get_session_manager() + async with create_browser_context( - proxy=proxy, profile=profile, session=session + proxy=proxy, + profile=profile, + session_manager=session_manager, + session_name=session, ) as (browser, context, page): page.set_default_timeout(30_000) await page.set_viewport_size({"width": width, "height": height}) diff --git a/tests/test_analyzer.py b/tests/test_analyzer.py index f4b8943..a94f66d 100644 --- a/tests/test_analyzer.py +++ b/tests/test_analyzer.py @@ -253,3 +253,32 @@ def test_group_multiple_methods_same_path(): assert "GET /items" in groups assert "POST /items" in groups assert "DELETE /items/{id}" in groups + + +def test_infer_query_param_type(): + from site2cli.discovery.analyzer import _infer_query_param_type + + assert _infer_query_param_type("5") == "integer" + assert _infer_query_param_type("-12") == "integer" + assert _infer_query_param_type("3.14") == "number" + assert _infer_query_param_type("true") == "boolean" + assert _infer_query_param_type("False") == "boolean" + assert _infer_query_param_type("widget") == "string" + assert _infer_query_param_type("") == "string" + assert _infer_query_param_type(None) == "string" + + +def test_query_params_typed_from_examples(): + """Numeric query params should be typed integer, consistent with body params.""" + exchanges = [ + _make_exchange( + "GET", + "https://api.example.com/search?q=test&limit=10", + resp_body='{"results": []}', + ), + ] + analyzer = TrafficAnalyzer(exchanges) + endpoints = analyzer.extract_endpoints() + params = {p.name: p.param_type for p in endpoints[0].parameters} + assert params["limit"] == "integer" + assert params["q"] == "string" diff --git a/tests/test_cli.py b/tests/test_cli.py index bdd8824..0e77a39 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -139,3 +139,51 @@ def test_run_help(): result = runner.invoke(app, ["run", "--help"]) assert result.exit_code == 0 assert "domain" in result.output.lower() + + +def test_daemon_start_has_foreground_flag(): + import re + + result = runner.invoke(app, ["daemon", "start", "--help"]) + assert result.exit_code == 0 + # Typer forces ANSI colors under GITHUB_ACTIONS, splitting "--" with + # escape codes — strip them before substring assertions. + plain = re.sub(r"\x1b\[[0-9;]*m", "", result.output) + assert "--foreground" in plain + + +def test_run_unknown_action_lists_available(tmp_path, monkeypatch): + """run with a bad action name fails fast and prints valid action names.""" + monkeypatch.setenv("XDG_DATA_HOME", str(tmp_path)) + from site2cli.config import get_config, reset_config + reset_config() + + try: + from site2cli.models import EndpointInfo, SiteAction, SiteEntry, Tier + from site2cli.registry import SiteRegistry + + config = get_config() + config.ensure_dirs() + reg = SiteRegistry(config.db_path) + reg.add_site( + SiteEntry( + domain="api.example.com", + base_url="https://api.example.com", + actions=[ + SiteAction( + name="get_items", + tier=Tier.API, + endpoint=EndpointInfo(method="GET", path_pattern="/items"), + ) + ], + ) + ) + reg.close() + + result = runner.invoke(app, ["run", "api.example.com", "totally_wrong_action"]) + assert result.exit_code == 1 + assert "Unknown action" in result.output + assert "get_items" in result.output + finally: + # Don't leak the tmp_path-bound config into subsequent tests + reset_config() diff --git a/tests/test_client_generator.py b/tests/test_client_generator.py index 7235aa8..1ff4cf7 100644 --- a/tests/test_client_generator.py +++ b/tests/test_client_generator.py @@ -95,3 +95,20 @@ def test_generate_client_custom_class_name(): } code = generate_client_code(spec, class_name="MyCustomClient") assert "class MyCustomClient" in code + + +def test_generate_client_digit_leading_title(): + """Hosts starting with a digit (IPs, 9gag.com) must yield valid identifiers.""" + spec = { + "openapi": "3.1.0", + "info": {"title": "127.0.0.1 API", "version": "1.0.0"}, + "servers": [{"url": "http://127.0.0.1:8777"}], + "paths": { + "/api/items": { + "get": {"operationId": "get_api_items", "parameters": []}, + }, + }, + } + code = generate_client_code(spec) + compile(code, "", "exec") # raises SyntaxError if broken + assert "class Site127001Client:" in code diff --git a/tests/test_crawl_robots.py b/tests/test_crawl_robots.py index c0e5eaa..07a28ab 100644 --- a/tests/test_crawl_robots.py +++ b/tests/test_crawl_robots.py @@ -93,3 +93,20 @@ async def test_load_nonexistent(self): await checker.load() assert checker.is_allowed("https://nonexistent-domain-12345.com/anything") is True + + +class TestRobotsURL: + def test_robots_url_includes_port(self): + rc = RobotsChecker("example.com:8080", "http") + assert rc.robots_url == "http://example.com:8080/robots.txt" + + def test_robots_url_standard(self): + rc = RobotsChecker("example.com", "https") + assert rc.robots_url == "https://example.com/robots.txt" + + def test_crawler_passes_netloc_with_port(self): + from site2cli.crawl.crawler import SiteCrawler + + crawler = SiteCrawler("http://127.0.0.1:8777/") + assert crawler.netloc == "127.0.0.1:8777" + assert crawler.domain == "127.0.0.1" diff --git a/tests/test_generated_code.py b/tests/test_generated_code.py index 3af4515..d61f12f 100644 --- a/tests/test_generated_code.py +++ b/tests/test_generated_code.py @@ -224,3 +224,57 @@ def test_mcp_server_includes_correct_server_name(): site = _make_site(domain="widgets.io", base_url="https://api.widgets.io") code = generate_mcp_server_code(site, _make_spec()) assert "widgets.io-site2cli" in code + + +def test_mcp_tool_names_are_spec_valid(): + """MCP tool names must match ^[a-zA-Z0-9_-]+$ — braces from path + + parameters (e.g. get_api_users_{id}) are rejected by real MCP clients. + """ + import re + + from site2cli.generators.mcp_gen import _spec_to_mcp_tools + + spec = _make_spec( + { + "/api/users/{id}": { + "get": { + "operationId": "get_api_users_{id}", + "parameters": [ + { + "name": "id", + "in": "path", + "required": True, + "schema": {"type": "string"}, + } + ], + "responses": {"200": {"description": "OK"}}, + } + } + } + ) + site = _make_site(domain="127.0.0.1", base_url="http://127.0.0.1:8777") + tools = _spec_to_mcp_tools(site, spec) + assert tools + for tool in tools: + assert re.fullmatch(r"[a-zA-Z0-9_-]+", tool.name), tool.name + + +def test_mcp_tool_names_deduplicated_on_sanitization_collision(): + """Sanitized tool names that collide get deterministic suffixes.""" + from site2cli.generators.mcp_gen import _spec_to_mcp_tools + + spec = _make_spec( + { + "/a-b": { + "get": {"operationId": "get_a-b", "responses": {}}, + }, + "/a_b": { + "get": {"operationId": "get_a_b", "responses": {}}, + }, + } + ) + site = _make_site(domain="example.com", base_url="https://api.example.com") + tools = _spec_to_mcp_tools(site, spec) + names = [t.name for t in tools] + assert len(set(names)) == len(names), names diff --git a/tests/test_health.py b/tests/test_health.py index db1f277..df1056c 100644 --- a/tests/test_health.py +++ b/tests/test_health.py @@ -242,3 +242,54 @@ async def test_action_without_endpoint_gets_unknown_status(registry, monitor): result = await monitor.check_site("browser.example.com") assert result["do_something"] == HealthStatus.UNKNOWN + + +class MockAsyncClientNoHead: + """Server that rejects HEAD/OPTIONS (501/405) but serves GET fine.""" + + def __init__(self, probe_status: int = 501, get_status: int = 200): + self.probe_status = probe_status + self.get_status = get_status + + async def __aenter__(self): + return self + + async def __aexit__(self, *args): + pass + + async def head(self, url, **kwargs): + return MockResponse(self.probe_status) + + async def options(self, url, **kwargs): + return MockResponse(self.probe_status) + + async def get(self, url, **kwargs): + return MockResponse(self.get_status) + + +async def test_check_endpoint_falls_back_to_get_on_501( + registry, monitor, site_with_get_endpoint, monkeypatch, +): + """Servers without HEAD support must not be reported BROKEN.""" + registry.add_site(site_with_get_endpoint) + + monkeypatch.setattr( + httpx, "AsyncClient", lambda **kwargs: MockAsyncClientNoHead(501, 200) + ) + + result = await monitor.check_site("api.example.com") + assert result["list_items"] == HealthStatus.HEALTHY + + +async def test_check_endpoint_falls_back_to_get_on_405_for_post( + registry, monitor, site_with_post_endpoint, monkeypatch, +): + """OPTIONS-rejecting servers get a GET probe instead of BROKEN.""" + registry.add_site(site_with_post_endpoint) + + monkeypatch.setattr( + httpx, "AsyncClient", lambda **kwargs: MockAsyncClientNoHead(405, 200) + ) + + result = await monitor.check_site("api.example.com") + assert result["create_item"] == HealthStatus.HEALTHY diff --git a/tests/test_router.py b/tests/test_router.py index e1be085..9edcc6a 100644 --- a/tests/test_router.py +++ b/tests/test_router.py @@ -108,21 +108,48 @@ async def mock_execute_action(url, action, params): class TestExecuteUnknownAction: - """execute() should fall back to browser when action is not registered.""" + """execute() should fail fast with valid names for unknown actions.""" @pytest.mark.asyncio - async def test_unknown_action_calls_fallback_browser(self, registry): + async def test_unknown_action_returns_error_with_available_actions(self, registry): site = _make_api_site() registry.add_site(site) router = Router(registry) - expected = {"fallback": True} async def mock_execute_action(url, action, params): - return expected + raise AssertionError("browser fallback must not run for unknown actions") router._browser.execute_action = mock_execute_action result = await router.execute("api.example.com", "nonexistent_action", {}) + assert "error" in result + assert "nonexistent_action" in result["error"] + assert result["available_actions"] == ["get_data"] + + @pytest.mark.asyncio + async def test_action_name_matches_normalized_legacy_style(self, registry): + """operationId-style names resolve legacy 'GET_/path' registry entries.""" + site = SiteEntry( + domain="legacy.example.com", + base_url="https://legacy.example.com", + actions=[ + SiteAction( + name="GET_/api/products", + tier=Tier.API, + endpoint=EndpointInfo(method="GET", path_pattern="/api/products"), + ) + ], + ) + registry.add_site(site) + router = Router(registry) + expected = {"status_code": 200, "data": []} + + async def mock_api_execute(site_arg, endpoint, params): + return expected + + router._direct_api.execute = mock_api_execute + + result = await router.execute("legacy.example.com", "get_api_products", {}) assert result == expected @@ -372,3 +399,61 @@ async def mock_execute_tier(site_entry, action, tier, params): await router.execute("api.example.com", "get_data", {}) assert promote_calls == [] + + +class TestAmbiguousActionNames: + @pytest.mark.asyncio + async def test_ambiguous_normalized_match_is_rejected(self, registry): + """Two legacy actions normalizing to the same name must not resolve.""" + site = SiteEntry( + domain="dupes.example.com", + base_url="https://dupes.example.com", + actions=[ + SiteAction( + name="GET_/a-b", + tier=Tier.API, + endpoint=EndpointInfo(method="GET", path_pattern="/a-b"), + ), + SiteAction( + name="GET_/a_b", + tier=Tier.API, + endpoint=EndpointInfo(method="GET", path_pattern="/a_b"), + ), + ], + ) + registry.add_site(site) + router = Router(registry) + + result = await router.execute("dupes.example.com", "get_a_b", {}) + assert "error" in result + assert sorted(result["available_actions"]) == ["GET_/a-b", "GET_/a_b"] + + @pytest.mark.asyncio + async def test_exact_match_wins_over_ambiguity(self, registry): + site = SiteEntry( + domain="exact.example.com", + base_url="https://exact.example.com", + actions=[ + SiteAction( + name="get_a_b", + tier=Tier.API, + endpoint=EndpointInfo(method="GET", path_pattern="/a_b"), + ), + SiteAction( + name="GET_/a-b", + tier=Tier.API, + endpoint=EndpointInfo(method="GET", path_pattern="/a-b"), + ), + ], + ) + registry.add_site(site) + router = Router(registry) + expected = {"ok": True} + + async def mock_api_execute(site_arg, endpoint, params): + return expected + + router._direct_api.execute = mock_api_execute + + result = await router.execute("exact.example.com", "get_a_b", {}) + assert result == expected diff --git a/tests/test_screenshot.py b/tests/test_screenshot.py index dfd867d..ea8ae7c 100644 --- a/tests/test_screenshot.py +++ b/tests/test_screenshot.py @@ -72,3 +72,56 @@ def test_cli_missing_url(self): runner = CliRunner() result = runner.invoke(app, ["screenshot"]) assert result.exit_code != 0 + + +class TestCaptureKwargs: + def test_capture_forwards_valid_context_kwargs(self): + """take_screenshot's kwargs must bind to create_browser_context's signature. + + Regression test: v0.7.0 passed proxy=/session= which the factory + rejected, making every `site2cli screenshot` invocation crash. + """ + import inspect + + from site2cli.browser.context import create_browser_context + + sig = inspect.signature(create_browser_context) + # These are exactly the kwargs screenshot/capture.py forwards. + sig.bind(proxy=None, profile=None, session_manager=None, session_name=None) + + async def test_take_screenshot_with_stubbed_browser(self, tmp_path, monkeypatch): + """End-to-end through take_screenshot with a stub context factory.""" + import inspect + from contextlib import asynccontextmanager + + from site2cli.browser import context as ctx_mod + from site2cli.screenshot.capture import take_screenshot + + real_sig = inspect.signature(ctx_mod.create_browser_context) + out_file = tmp_path / "shot.png" + + class FakePage: + def set_default_timeout(self, ms): + pass + + async def set_viewport_size(self, size): + pass + + async def goto(self, url, **kwargs): + pass + + async def screenshot(self, **kwargs): + from pathlib import Path + + Path(kwargs["path"]).write_bytes(b"\x89PNG fake") + + @asynccontextmanager + async def fake_factory(*args, **kwargs): + real_sig.bind(*args, **kwargs) # reject kwargs the real factory rejects + yield None, None, FakePage() + + monkeypatch.setattr(ctx_mod, "create_browser_context", fake_factory) + + result = await take_screenshot("http://example.com", output=str(out_file)) + assert out_file.exists() + assert result.format == "png" diff --git a/tests/test_search.py b/tests/test_search.py index dc1828c..17400e9 100644 --- a/tests/test_search.py +++ b/tests/test_search.py @@ -85,6 +85,8 @@ async def test_returns_results(self): class TestSearchCLI: def test_cli_help(self): + import re + from typer.testing import CliRunner from site2cli.cli import app @@ -92,9 +94,12 @@ def test_cli_help(self): runner = CliRunner() result = runner.invoke(app, ["search", "--help"]) assert result.exit_code == 0 - assert "search" in result.output.lower() - assert "--scrape" in result.output - assert "--extract" in result.output + # Typer forces ANSI colors under GITHUB_ACTIONS, splitting "--" with + # escape codes — strip them before substring assertions. + plain = re.sub(r"\x1b\[[0-9;]*m", "", result.output) + assert "search" in plain.lower() + assert "--scrape" in plain + assert "--extract" in plain def test_cli_missing_query(self): from typer.testing import CliRunner diff --git a/tests/test_spec_generator.py b/tests/test_spec_generator.py index 8bc5a38..2529499 100644 --- a/tests/test_spec_generator.py +++ b/tests/test_spec_generator.py @@ -116,3 +116,65 @@ def test_save_and_load_spec(tmp_path): loaded = load_spec(path) assert loaded["openapi"] == "3.1.0" assert loaded["paths"] == spec["paths"] + + +def test_make_operation_id_sanitizes_path_params(): + from site2cli.discovery.spec_generator import make_operation_id + + ep = EndpointInfo(method="GET", path_pattern="/api/users/{id}") + assert make_operation_id(ep) == "get_api_users_id" + + ep = EndpointInfo(method="POST", path_pattern="/api/search") + assert make_operation_id(ep) == "post_api_search" + + ep = EndpointInfo(method="GET", path_pattern="/v2/items", description="List all items!") + assert make_operation_id(ep) == "list_all_items" + + +def test_operation_ids_are_identifier_safe(): + """Generated operationIds must not contain braces or slashes.""" + import re + + api = DiscoveredAPI( + site_url="example.com", + base_url="https://api.example.com", + endpoints=[EndpointInfo(method="GET", path_pattern="/api/users/{id}")], + auth_type=AuthType.NONE, + ) + spec = generate_openapi_spec(api) + op_id = spec["paths"]["/api/users/{id}"]["get"]["operationId"] + assert re.fullmatch(r"[a-zA-Z0-9_]+", op_id), op_id + + +def test_make_unique_operation_ids_deduplicates_collisions(): + """Lossy normalization collisions get deterministic suffixes.""" + from site2cli.discovery.spec_generator import make_unique_operation_ids + + endpoints = [ + EndpointInfo(method="GET", path_pattern="/a-b"), + EndpointInfo(method="GET", path_pattern="/a_b"), + EndpointInfo(method="GET", path_pattern="/a.b"), + EndpointInfo(method="POST", path_pattern="/a-b"), + ] + ids = make_unique_operation_ids(endpoints) + assert ids == ["get_a_b", "get_a_b_2", "get_a_b_3", "post_a_b"] + assert len(set(ids)) == len(ids) + + +def test_generated_spec_operation_ids_unique_on_collision(): + api = DiscoveredAPI( + site_url="example.com", + base_url="https://api.example.com", + endpoints=[ + EndpointInfo(method="GET", path_pattern="/a-b"), + EndpointInfo(method="GET", path_pattern="/a_b"), + ], + auth_type=AuthType.NONE, + ) + spec = generate_openapi_spec(api) + op_ids = [ + op["operationId"] + for path_item in spec["paths"].values() + for op in path_item.values() + ] + assert len(set(op_ids)) == len(op_ids)