Independent v0.7.0 validation + fixes for the 8 bugs it found#26
Merged
Conversation
End-to-end validation against a local test site with real browser CDP capture. Core discover->spec->clients->MCP pipeline confirmed working; 8 reproduced bugs documented with locations; quantified README claims audited (several are simulations/estimates rather than measurements). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T2ZL9mUURihJVhk4Q9zGpN
1. client_generator: digit-leading hosts (127.0.0.1, 9gag.com) produced
syntactically invalid Python (class 127001Client) — now Site127001Client.
2. discover: registry stored relative -o spec paths, breaking run/mcp
generate from other directories — paths now resolved absolute, with a
specs-dir fallback for legacy entries.
3. Action naming unified on operationIds (get_api_products) across spec,
registry, clients, and MCP tools; discover prints runnable names;
unknown actions fail fast with the valid list instead of silently
launching a browser at https://domain/ (dropping base_url's port);
normalized matching keeps legacy GET_/path registries working.
4. crawler: robots.txt was fetched without the port (hostname instead of
netloc), failing open on non-standard ports — Disallow rules were
silently ignored.
5. screenshot: every invocation crashed (proxy=/session= kwargs the
context factory rejects) — factory gained a proxy override, capture
passes session_manager/session_name.
6. mcp_gen: tool names contained { } from path params, violating the MCP
name charset; query params now type-inferred (limit=5 -> integer) for
consistency with body params.
7. daemon start now detaches by default (--foreground to keep old
behavior); added __main__.py for python -m site2cli.
8. health: servers answering 405/501 to HEAD/OPTIONS were marked BROKEN
despite working endpoints — now falls back to a GET probe.
16 regression tests added (suite: 553 -> 569). CLAUDE.md test totals
deduplicated and updated.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T2ZL9mUURihJVhk4Q9zGpN
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe changes normalize generated identifiers and action routing, fix artifact path and browser/session handling, add port-aware robots and health probes, support detached daemon startup, add ChangesCLI validation and runtime fixes
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Typer forces colored output when GITHUB_ACTIONS is set, splitting option names with escape codes (e.g. --foreground) so plain substring asserts fail on CI while passing locally. Affected the new daemon flag test and the pre-existing search help test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T2ZL9mUURihJVhk4Q9zGpN
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/site2cli/browser/context.py (1)
61-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse an explicit
is not Nonecheck for the optional proxy argument.Since
proxyis typed asdict | None, evaluating it for truthiness (proxy or ...) means an explicitly passed empty dictionary ({}) would evaluate to false and fall back to the config instead of overriding it. Usingis not Nonecorrectly handles the distinction between "not provided" and "provided but empty".♻️ Proposed refactor
- # Resolve proxy settings (explicit argument wins over config) - proxy_config = proxy or 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()🤖 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/site2cli/browser/context.py` around lines 61 - 62, Update the proxy selection in the context initialization flow to use an explicit None check for the optional proxy argument, so a provided empty dictionary is preserved instead of falling back to config.proxy.get_playwright_proxy(). Keep the configuration fallback only for proxy values that are actually None.src/site2cli/__main__.py (1)
5-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSet
prog_nameexplicitly to improve CLI help output.When invoking a Typer/Click application via
python -m site2cli, the CLI help text will often default to displayingUsage: __main__.py [OPTIONS]instead of the module name. Passingprog_name="site2cli"ensures consistent and user-friendly usage instructions.♻️ Proposed refactor
if __name__ == "__main__": - app() + app(prog_name="site2cli")🤖 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/site2cli/__main__.py` around lines 5 - 6, Update the __main__ entry-point invocation of app to pass prog_name explicitly as "site2cli", ensuring help output uses the module name instead of __main__.py.
🤖 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 `@CLAUDE.md`:
- Line 110: Update the test-summary statement in CLAUDE.md to explicitly
distinguish the 569 offline tests from the 6 network-dependent live tests, state
that live tests require network access, and avoid implying that all live tests
are always passing. Preserve the separate reporting of offline tests and live
external-API validation.
In `@src/site2cli/discovery/spec_generator.py`:
- Around line 19-21: Make normalized identifiers collision-safe across all three
sites: in src/site2cli/discovery/spec_generator.py lines 19-21, update the
operation-ID generation to assign deterministic unique IDs and persist those
assignments for registry actions; in src/site2cli/router.py lines 128-131,
detect multiple normalized lookup matches and reject them instead of selecting
the first; in src/site2cli/generators/mcp_gen.py lines 32-33, detect sanitized
tool-name collisions and deterministically suffix or reject duplicate names.
In `@tests/test_cli.py`:
- Around line 150-180: Update test_run_unknown_action_lists_available to call
reset_config() after runner.invoke completes, using a fixture or try/finally so
cleanup always runs even if assertions fail. Preserve the existing test setup
and assertions while ensuring the global cached configuration does not leak into
subsequent tests.
---
Nitpick comments:
In `@src/site2cli/__main__.py`:
- Around line 5-6: Update the __main__ entry-point invocation of app to pass
prog_name explicitly as "site2cli", ensuring help output uses the module name
instead of __main__.py.
In `@src/site2cli/browser/context.py`:
- Around line 61-62: Update the proxy selection in the context initialization
flow to use an explicit None check for the optional proxy argument, so a
provided empty dictionary is preserved instead of falling back to
config.proxy.get_playwright_proxy(). Keep the configuration fallback only for
proxy values that are actually None.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64ab3dbb-2b26-4ee5-91a9-74d05a7438a3
📒 Files selected for processing (23)
CLAUDE.mdVALIDATION-REPORT.mdsrc/site2cli/__main__.pysrc/site2cli/browser/context.pysrc/site2cli/cli.pysrc/site2cli/crawl/crawler.pysrc/site2cli/crawl/robots.pysrc/site2cli/discovery/analyzer.pysrc/site2cli/discovery/client_generator.pysrc/site2cli/discovery/spec_generator.pysrc/site2cli/generators/mcp_gen.pysrc/site2cli/health/monitor.pysrc/site2cli/router.pysrc/site2cli/screenshot/capture.pytests/test_analyzer.pytests/test_cli.pytests/test_client_generator.pytests/test_crawl_robots.pytests/test_generated_code.pytests/test_health.pytests/test_router.pytests/test_screenshot.pytests/test_spec_generator.py
Comment on lines
+19
to
+21
| 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" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make normalized identifiers collision-safe across discovery, routing, and MCP generation.
Lossy normalization currently permits distinct endpoints/actions to share one identifier, causing ambiguous routing or shadowed MCP handlers.
src/site2cli/discovery/spec_generator.py#L19-L21: assign deterministic unique operation IDs and persist that assignment for registry actions.src/site2cli/router.py#L128-L131: reject normalized lookups with multiple matches instead of selecting the first.src/site2cli/generators/mcp_gen.py#L32-L33: detect sanitized tool-name collisions and suffix or reject duplicates.
📍 Affects 3 files
src/site2cli/discovery/spec_generator.py#L19-L21(this comment)src/site2cli/router.py#L128-L131src/site2cli/generators/mcp_gen.py#L32-L33
🤖 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/site2cli/discovery/spec_generator.py` around lines 19 - 21, Make
normalized identifiers collision-safe across all three sites: in
src/site2cli/discovery/spec_generator.py lines 19-21, update the operation-ID
generation to assign deterministic unique IDs and persist those assignments for
registry actions; in src/site2cli/router.py lines 128-131, detect multiple
normalized lookup matches and reject them instead of selecting the first; in
src/site2cli/generators/mcp_gen.py lines 32-33, detect sanitized tool-name
collisions and deterministically suffix or reject duplicate names.
…leanups - Operation IDs / action names / MCP tool names are now collision-safe: normalization is lossy (/a-b and /a_b collapse to the same ID), so duplicates get deterministic _2/_3 suffixes shared between the spec and registry (make_unique_operation_ids), and the router rejects ambiguous normalized lookups instead of picking the first match. - CLAUDE.md: clarify offline vs network-dependent live test split. - test_cli: reset cached config in finally so tmp_path doesn't leak. - browser context: explicit 'is not None' for the proxy override. - __main__: pass prog_name='site2cli' for correct help text. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T2ZL9mUURihJVhk4Q9zGpN
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
2 participants
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.
Summary
Full end-to-end validation of v0.7.0 (fresh install, whole CLI surface exercised against a local test site with a real headless Chromium), documented in
VALIDATION-REPORT.md, plus fixes for all 8 bugs the validation reproduced.Fixes
127.0.0.1,9gag.com) generated syntactically invalid Python (class 127001Client:); nowSite127001Client.-ospec paths, breakingrun/mcp generatefrom other directories; paths are now stored absolute, with a specs-dir fallback for legacy entries.get_api_products) across spec, registry, generated clients, and MCP tools;discoverprints the runnable name per endpoint; unknown actions fail fast with the valid names instead of silently launching a browser athttps://domain/(which dropped the port); normalized matching keeps legacyGET_/pathregistries working.Disallowrules on non-standard ports.TypeError(invalidproxy=/session=kwargs); the context factory gained aproxyoverride and capture passessession_manager/session_name.{/}from path params, violating the MCP name charset (^[a-zA-Z0-9_-]+$); query params are now type-inferred (limit=5→ integer) for consistency with body params.~/.site2cli/daemon.log),--foregroundkeeps the old behavior; added__main__.pyforpython -m site2cli.Behavior change
The router no longer falls back to browser exploration when a known site receives an unknown action name — it returns an error listing the valid actions. Browser fallback for entirely unknown domains is unchanged.
Testing
runfrom arbitrary cwd, robots-compliant crawl, working screenshot, MCP JSON-RPC probe (initialize/tools/list/tools/call), daemon detach/status/stop, health checks.CLAUDE.mdtest totals deduplicated (had contradictory 559/500 entries).🤖 Generated with Claude Code
https://claude.ai/code/session_01T2ZL9mUURihJVhk4Q9zGpN
Generated by Claude Code
Summary by CodeRabbit
New Features
python -m site2cli.Bug Fixes