diff --git a/AGENTS.md b/AGENTS.md index 18e449d1..0495152a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -128,32 +128,35 @@ The verifier prints JSON `{task_id, pass, reason, evidence[]}` and exits 0/1; th Run all of these before opening a PR. ```bash -# 1. syntax +# 1. registry consistency (no Docker required) +python3 scripts/audit_site_registry.py --strict + +# 2. syntax python3 -m py_compile sites//app.py -# 2. build +# 3. build ./scripts/build.sh webharbor:dev -# 3. run on alt ports (don't collide with anything you already have running) +# 4. run on alt ports (don't collide with anything you already have running) docker run -d --rm --name wh-test \ -p 8201:8101 -p 41000-41030:40000-40030 webharbor:dev -# 4. control plane healthy, all sites alive +# 5. control plane healthy, all sites alive curl -s http://localhost:8201/health | python3 -m json.tool | head -# 5. every site renders 200 +# 6. every site renders 200 for p in $(seq 41000 41030); do curl -so /dev/null -w "$p:%{http_code}\n" http://localhost:$p/ done -# 6. byte-identical reset (the strict invariant) +# 7. byte-identical reset (the strict invariant) curl -X POST http://localhost:8201/reset/ docker exec wh-test md5sum \ /opt/WebSyn//instance/.db \ /opt/WebSyn//instance_seed/.db # the two md5s MUST match — if not, see "Idempotent seeding" -# 7. teardown +# 8. teardown docker stop wh-test ``` diff --git a/README.md b/README.md index f3973fec..5a112a17 100644 --- a/README.md +++ b/README.md @@ -246,6 +246,25 @@ Any other improvement — bug fixes, UI polish, data enrichment, task suggestion | 📊 Contribution Track Sheet | [Google Sheet](https://docs.google.com/spreadsheets/d/1vZsrQjy9nJKze58fx4kbQtFi85NjVXIWCFyu3ShD7gk/edit?gid=0#gid=0) | | 📝 Contribution Request Form | [Google Form](https://forms.gle/ngcD1rzAfUEphNmRA) | +## Site Registry Audit + +Use the repository registry audit to check site registration consistency, port mappings, and task integration before opening a review or PR: + +```bash +python3 scripts/audit_site_registry.py +python3 scripts/audit_site_registry.py --site amazon +python3 scripts/audit_site_registry.py --strict +python3 scripts/audit_site_registry.py --json +``` + +Warnings are informational by default. Use `--strict` in pre-PR checks or CI to make warnings fail the command. + +This audit complements `scripts/check_site_registry.py`, which runs through +`scripts/check_assets.sh` during the build and checks exact task URLs and +referenced verifier files. Keep that build check enabled. The audit adds +structured JSON diagnostics, per-site selection, asset-path coverage, and +runtime-file checks. + ## Citation WebHarbor is initiated by UNC-Chapel Hill and Microsoft, with contributions from the broader community. If you have any questions, please contact us via `webharborcomm at gmail dot com` or `zhaoyang at cs dot unc dot edu`. diff --git a/review-reports/PR-46-REVIEW.md b/review-reports/PR-46-REVIEW.md new file mode 100644 index 00000000..2c208358 --- /dev/null +++ b/review-reports/PR-46-REVIEW.md @@ -0,0 +1,98 @@ +# Review of PR #46: site registry audit + +Original contribution: [aiming-lab/WebHarbor#46](https://github.com/aiming-lab/WebHarbor/pull/46), authored by @Lxr-max / XuanRui LI. + +## Scope and fixed versions + +- Current upstream base: `5d7a4e8c594028b845cbdcc180619e438c3a22ab` +- Integrated and tested implementation: `4610b5df689ef664d49d7ba56bc53d123de28261` +- Original review base: `f20b5ee8377ba31bcb825b4dfe30ad96c416e477` +- Original contribution: `6e5d77b0af6c2b7dfcd82039361df4228f2c3c65` +- Isolated-review fixed point: `80bd5817109563313d1ae30fac684a8b918599f7` +- Reconciled implementation commit: `839c74dc0dd1a2967552b9d6df991701c61aafe6` +- Assets pin inherited from current upstream: `fa1e8a5b9e8e5d0e42764cd658825f4dea088d8f` + +Relative to current upstream, this PR adds repository-level tooling and documentation only. It does not add or modify a mirror site, task set, deterministic task verifier, or Hugging Face asset. The asset pin is identical to upstream; the original review used `ad6f424f72cada9e6f5c09a58093d0ceeab9c52b`. + +## Baseline findings + +The original seven unit tests passed, but the original implementation did not pass its own strict scan on the original review base `f20b5ee`: it reported the valid `osu` / `Ohio State University` name pair as a warning and exited 1. + +Reproducible review fixtures also confirmed that the original implementation: + +1. raised tracebacks for malformed task URL ports and missing repository files; +2. parsed shell comments as site names; +3. rejected valid Docker `EXPOSE .../tcp` syntax while accepting descending ranges; +4. rejected complete explicit `.assetpaths` entries unless wildcard entries also existed; +5. inferred invalid task/site relationships from brand names without a canonical mapping. + +## Reviewer fixes + +- Return structured findings instead of tracebacks for malformed task URLs, invalid UTF-8 task files, missing required files, and malformed registries. +- Parse shell arrays with comment-aware tokenization and ignore commented-out declarations. +- Support protocol-qualified and lowercase Docker `EXPOSE` instructions; reject descending and out-of-range ports. +- Accept either wildcard or complete per-site asset paths. +- Remove the unreliable brand-name/slug heuristic while retaining within-file `web_name` consistency checks. +- Document that pre-PR and CI use should pass `--strict`. +- Report runtime-like regular files as structured warnings instead of raising `NotADirectoryError`. +- Parse only top-level registry declarations, remove unreachable duplicate-generated-port logic, and make the task-port collision test assert the actual diagnostic. +- Add the strict registry audit to the canonical `AGENTS.md` pre-PR checklist. + +## Isolated review reconciliation + +The frozen candidate was independently reviewed at `80bd581`. The reviewer reproduced the four recorded verification commands and returned `CHANGES_REQUIRED` with one P2 and four P3 findings; the review declared no contamination. + +- Accepted and fixed the P2 runtime-file crash, with a red-then-green JSON CLI regression. +- Accepted the P3 findings for top-level registry parsing, unreachable generated-port code / misleading test coverage, and the missing `AGENTS.md` checklist entry. +- Did not broaden `.assetpaths` to accept arbitrary recursive glob spellings. The checked-in file and the actual pack/extract scripts define three canonical managed roots; no repository consumer defines the proposed spellings as equivalent. Certifying them in the audit would accept an unverified asset configuration. Canonical wildcard entries and explicit per-site entries remain supported. + +Affected tests and the full validation set were rerun after reconciliation. A direct regression check also confirmed that restricting Python assignments to module scope does not reject valid indented shell declarations. + +## Upstream synchronization — 2026-09-16 + +Merged upstream `main` at `5d7a4e8`, preserving the original contribution and reviewer commits. The only content conflict was in the `AGENTS.md` pre-PR checklist: the audit had shifted the step numbers while upstream expanded the HTTP sweep. The resolution keeps the audit step, consecutive numbering, and the current `41000–41030` host-port range. + +Upstream now includes `scripts/check_site_registry.py`, called by `scripts/check_assets.sh` during the build. That check verifies exact task URLs and referenced verifier paths. It remains enabled and unchanged. The supplemental audit adds structured JSON diagnostics, per-site selection, asset-path coverage, and runtime-file checks; README now explains the relationship. + +The audit implementation and its 22 tests are byte-for-byte unchanged from the previously reconciled PR head `472863e`. The historical isolated review therefore remains applicable to that code; no new independent review is claimed. Integration checks below were rerun against all 31 current sites. The upstream HF revision was confirmed reachable; no asset archives were downloaded or modified for this tooling update. + +## Validation + +All commands below were run on 2026-09-16 from the tree committed as `4610b5d`; the subsequent report update changes documentation only: + +```bash +python3.12 -m py_compile scripts/audit_site_registry.py scripts/test_audit_site_registry.py +python3.12 -B -m unittest discover -s scripts -p 'test_audit_site_registry.py' -v +python3.12 -B scripts/audit_site_registry.py --strict +python3.12 -B scripts/check_site_registry.py +pyright scripts/audit_site_registry.py scripts/test_audit_site_registry.py +``` + +Results: + +- 22/22 unit and adversarial tests passed. +- Current repository scan covered 31 site directories, 31 registered sites, 31 ports, 31 task files, and 945 tasks. +- Strict scan: 0 errors, 0 warnings, exit 0. +- Upstream registry check: all 31 sites, task URLs, and referenced verifier paths passed; Docker exposure is `8101 40000-40030`. +- Pyright: 0 errors, 0 warnings. +- Python byte-compilation: passed. +- Git whitespace/conflict checks: passed; only the audit, its tests, and documentation differ from current upstream. + +The original 2026-09-13 validation covered 26 sites and 843 tasks. Those historical counts are superseded by the current integration results above. + +The negative fixtures cover missing registrations/directories, duplicate task ports, mismatched ports, malformed JSONL inputs, malformed registries, function-local lookalike declarations, runtime-like regular files, missing core files, invalid Docker ranges/ports, warning/strict exit behavior, and JSON error output. Legal alternatives cover explicit per-site asset paths, brand aliases, shell comments and indentation, and protocol-qualified Docker ports. + +## Applicability and unexecuted checks + +| Check | Result | Reason | +|---|---|---| +| Repository CLI and unit/adversarial tests | PASS | Results above | +| Site UI / original-site visual comparison | N/A | No mirror site differs from current upstream | +| Browser task trajectories and before/after state | N/A | No benchmark task differs from current upstream | +| Deterministic task verifier review | N/A | No task verifier differs from current upstream | +| Hugging Face asset PR | N/A | Current upstream pin retained; no new asset contribution | +| Full Docker image build/smoke | NOT RUN | Not repeated for this tooling/documentation update; Dockerfile, runtime, sites, and assets are identical to current upstream | + +## Current status + +The synchronized candidate passes the repository-tooling checks and is ready for maintainer review. The original full Docker build was not run because the review host lacked safe rebuild headroom; this update does not claim a fresh Docker build or runtime smoke. Final approval and merge remain with the maintainer. diff --git a/scripts/audit_site_registry.py b/scripts/audit_site_registry.py new file mode 100644 index 00000000..95ab461b --- /dev/null +++ b/scripts/audit_site_registry.py @@ -0,0 +1,842 @@ +#!/usr/bin/env python3 +"""Audit WebHarbor site registration consistency and port mappings. + +This script checks repository-level site registration metadata without touching +runtime state or Hugging Face managed assets. +""" + +from __future__ import annotations + +import argparse +import ast +import json +import re +import shlex +import subprocess +import sys +from dataclasses import asdict, dataclass +from pathlib import Path +from typing import Any +from urllib.parse import urlparse + + +RUNTIME_SUBDIRS = ( + "instance", + "scraped_data", + "cache", + "caches", + "log", + "logs", + "screenshots", +) + + +@dataclass +class Finding: + severity: str + message: str + file: str | None = None + site: str | None = None + port: int | None = None + line: int | None = None + task_id: str | None = None + + +@dataclass +class SiteSummary: + site: str + in_sites_dir: bool + in_websyn: bool + in_control: bool + websyn_port: int | None + control_port: int | None + task_file: str | None + task_count: int + task_port: int | None + task_web_name: str | None + has_app: bool + has_seed_data: bool + has_tasks: bool + assetpaths_instance_seed: bool + assetpaths_images: bool + assetpaths_external_cache: bool + warnings: int + errors: int + + +@dataclass +class AuditResult: + root: str + strict: bool + site_directories_found: int + registered_sites_found: int + ports_found: int + task_files_checked: int + task_count: int + errors: list[Finding] + warnings: list[Finding] + sites: list[SiteSummary] + ports: dict[str, Any] + + @property + def exit_code(self) -> int: + if self.errors: + return 1 + if self.strict and self.warnings: + return 1 + return 0 + + def to_json_dict(self) -> dict[str, Any]: + return { + "summary": { + "root": self.root, + "strict": self.strict, + "site_directories_found": self.site_directories_found, + "registered_sites_found": self.registered_sites_found, + "ports_found": self.ports_found, + "task_files_checked": self.task_files_checked, + "task_count": self.task_count, + "errors": len(self.errors), + "warnings": len(self.warnings), + "exit_code": self.exit_code, + }, + "sites": [asdict(site) for site in self.sites], + "ports": self.ports, + "errors": [asdict(finding) for finding in self.errors], + "warnings": [asdict(finding) for finding in self.warnings], + } + + +class FindingCollector: + def __init__(self) -> None: + self.errors: list[Finding] = [] + self.warnings: list[Finding] = [] + + def error( + self, + message: str, + *, + file: str | None = None, + site: str | None = None, + port: int | None = None, + line: int | None = None, + task_id: str | None = None, + ) -> None: + self.errors.append( + Finding( + severity="ERROR", + message=message, + file=file, + site=site, + port=port, + line=line, + task_id=task_id, + ) + ) + + def warn( + self, + message: str, + *, + file: str | None = None, + site: str | None = None, + port: int | None = None, + line: int | None = None, + task_id: str | None = None, + ) -> None: + self.warnings.append( + Finding( + severity="WARN", + message=message, + file=file, + site=site, + port=port, + line=line, + task_id=task_id, + ) + ) + + +def incomplete_audit_result( + root: Path, + *, + strict: bool, + collector: FindingCollector, + site_directories_found: int = 0, +) -> AuditResult: + return AuditResult( + root=str(root), + strict=strict, + site_directories_found=site_directories_found, + registered_sites_found=0, + ports_found=0, + task_files_checked=0, + task_count=0, + errors=collector.errors, + warnings=collector.warnings, + sites=[], + ports={}, + ) + + +def slug_is_valid(slug: str) -> bool: + return bool(re.fullmatch(r"[a-z0-9_]+", slug)) + + +def parse_site_array(text: str, file_label: str) -> tuple[list[str], int]: + declaration_prefix = r"^" if Path(file_label).suffix == ".py" else r"^[ \t]*" + sites_match = re.search( + declaration_prefix + r"SITES\s*=\s*(\(.*?\)|\[.*?\])", + text, + re.DOTALL | re.MULTILINE, + ) + if not sites_match: + raise ValueError(f"Could not parse SITES from {file_label}") + sites_block = sites_match.group(1) + if sites_block.startswith("("): + try: + sites = shlex.split(sites_block[1:-1], comments=True, posix=True) + except ValueError as exc: + raise ValueError(f"Could not parse SITES from {file_label}: {exc}") from exc + else: + sites = ast.literal_eval(sites_block) + if not isinstance(sites, list): + raise ValueError(f"SITES is not a list in {file_label}") + if not all(isinstance(site, str) and site for site in sites): + raise ValueError(f"SITES must contain only non-empty strings in {file_label}") + base_match = re.search( + declaration_prefix + r"BASE_PORT\s*=\s*(\d+)", text, re.MULTILINE + ) + if not base_match: + raise ValueError(f"Could not parse BASE_PORT from {file_label}") + return sites, int(base_match.group(1)) + + +def build_port_map(sites: list[str], base_port: int) -> dict[str, int]: + return {site: base_port + index for index, site in enumerate(sites)} + + +def parse_docker_ports(dockerfile: Path) -> dict[str, Any]: + exposed: set[int] = set() + invalid_expose_tokens: list[str] = [] + text = dockerfile.read_text(encoding="utf-8") + lines = re.sub(r"\\\s*\n", " ", text).splitlines() + for line in lines: + stripped = line.split("#", 1)[0].strip() + tokens = stripped.split() + if not tokens or tokens[0].upper() != "EXPOSE": + continue + for raw_token in tokens[1:]: + port_token, separator, protocol = raw_token.partition("/") + if separator and not protocol: + invalid_expose_tokens.append(f"invalid EXPOSE token '{raw_token}'") + continue + port_match = re.fullmatch(r"(\d+)(?:-(\d+))?", port_token) + if not port_match: + invalid_expose_tokens.append(f"invalid EXPOSE token '{raw_token}'") + continue + start = int(port_match.group(1)) + end = int(port_match.group(2)) if port_match.group(2) else start + if end < start: + invalid_expose_tokens.append(f"descending EXPOSE range '{raw_token}'") + continue + if start < 1 or end > 65535: + invalid_expose_tokens.append(f"out-of-range EXPOSE token '{raw_token}'") + continue + exposed.update(range(start, end + 1)) + return { + "exposed_ports": sorted(exposed), + "invalid_expose_tokens": invalid_expose_tokens, + } + + +def parse_assetpaths(assetpaths_path: Path) -> list[str]: + if not assetpaths_path.exists(): + return [] + patterns: list[str] = [] + for raw_line in assetpaths_path.read_text(encoding="utf-8").splitlines(): + line = raw_line.strip() + if not line or line.startswith("#"): + continue + patterns.append(line.rstrip("/")) + return patterns + + +def pattern_covers_site(patterns: list[str], site: str, suffix: str) -> bool: + suffix = suffix.strip("/").replace("\\", "/") + explicit = f"sites/{site}/{suffix}" + wildcard = f"sites/*/{suffix}" + return explicit in patterns or wildcard in patterns + + +def parse_readme_reset_examples(readme_path: Path) -> set[str]: + if not readme_path.exists(): + return set() + text = readme_path.read_text(encoding="utf-8", errors="replace") + return set(re.findall(r"/reset/([A-Za-z0-9_]+)", text)) + + +def git_tracked_files(root: Path, site: str, relative_dir: str) -> list[str]: + try: + completed = subprocess.run( + ["git", "ls-files", f"sites/{site}/{relative_dir}"], + cwd=root, + check=False, + capture_output=True, + text=True, + ) + except OSError: + return [] + if completed.returncode != 0: + return [] + return [line.strip() for line in completed.stdout.splitlines() if line.strip()] + + +def parse_tasks_jsonl( + tasks_path: Path, collector: FindingCollector, site: str +) -> tuple[int, int | None, str | None]: + if not tasks_path.exists(): + collector.error("registered site is missing tasks.jsonl", file=str(tasks_path), site=site) + return 0, None, None + + task_count = 0 + ports: set[int] = set() + web_names: set[str] = set() + seen_task_objects = False + + try: + task_lines = tasks_path.read_text(encoding="utf-8").splitlines() + except (OSError, UnicodeError) as exc: + collector.error( + f"could not read tasks.jsonl as UTF-8: {exc}", + file=str(tasks_path), + site=site, + ) + return 0, None, None + + for line_number, raw_line in enumerate(task_lines, start=1): + if not raw_line.strip(): + continue + try: + payload = json.loads(raw_line) + except json.JSONDecodeError as exc: + collector.error( + f"invalid JSONL: {exc.msg}", + file=str(tasks_path), + site=site, + line=line_number, + ) + continue + if not isinstance(payload, dict): + collector.error( + "task line is not a JSON object", + file=str(tasks_path), + site=site, + line=line_number, + ) + continue + + seen_task_objects = True + task_count += 1 + web_name = payload.get("web_name") + web_url = payload.get("web") + task_id = payload.get("id") + + if isinstance(web_name, str) and web_name.strip(): + web_names.add(web_name.strip()) + else: + collector.warn( + "task is missing a non-empty web_name field", + file=str(tasks_path), + site=site, + line=line_number, + task_id=str(task_id) if task_id else None, + ) + + if not isinstance(web_url, str) or not web_url.strip(): + collector.warn( + "task is missing a non-empty web field", + file=str(tasks_path), + site=site, + line=line_number, + task_id=str(task_id) if task_id else None, + ) + continue + + try: + parsed = urlparse(web_url) + hostname = parsed.hostname + port = parsed.port + except ValueError as exc: + collector.warn( + f"task web URL has an invalid port or host: {exc}", + file=str(tasks_path), + site=site, + line=line_number, + task_id=str(task_id) if task_id else None, + ) + continue + if parsed.scheme not in {"http", "https"}: + collector.warn( + "task web URL must use http or https", + file=str(tasks_path), + site=site, + line=line_number, + task_id=str(task_id) if task_id else None, + ) + if hostname not in {"localhost", "127.0.0.1"}: + collector.warn( + "task web URL should point to localhost or 127.0.0.1", + file=str(tasks_path), + site=site, + line=line_number, + task_id=str(task_id) if task_id else None, + ) + if port is None: + collector.warn( + "task web URL should include an explicit port", + file=str(tasks_path), + site=site, + line=line_number, + task_id=str(task_id) if task_id else None, + ) + else: + ports.add(port) + + if not seen_task_objects: + collector.error("tasks.jsonl is empty", file=str(tasks_path), site=site) + + if len(web_names) > 1: + collector.warn( + f"task file uses multiple web_name values: {sorted(web_names)}", + file=str(tasks_path), + site=site, + ) + + task_port = next(iter(ports)) if len(ports) == 1 else None + if len(ports) > 1: + collector.warn( + f"task file uses multiple localhost ports: {sorted(ports)}", + file=str(tasks_path), + site=site, + ) + + web_name = next(iter(web_names)) if len(web_names) == 1 else None + return task_count, task_port, web_name + + +def human_status(errors: int, warnings: int) -> str: + if errors: + return "ERROR" + if warnings: + return "WARN" + return "OK" + + +def audit_repository(root: Path, *, site: str | None = None, strict: bool = False) -> AuditResult: + collector = FindingCollector() + + readme_path = root / "README.md" + websyn_path = root / "websyn_start.sh" + control_path = root / "control_server.py" + site_runner_path = root / "site_runner.py" + dockerfile_path = root / "Dockerfile" + assetpaths_path = root / ".assetpaths" + sites_root = root / "sites" + + if not sites_root.exists(): + collector.error("sites directory is missing", file=str(sites_root)) + return incomplete_audit_result( + root, + strict=strict, + collector=collector, + ) + + site_dirs = sorted(path.name for path in sites_root.iterdir() if path.is_dir()) + required_paths = (websyn_path, control_path, site_runner_path, dockerfile_path) + for required_path in required_paths: + if not required_path.is_file(): + collector.error( + f"required repository file is missing: {required_path.name}", + file=str(required_path), + ) + if collector.errors: + return incomplete_audit_result( + root, + strict=strict, + collector=collector, + site_directories_found=len(site_dirs), + ) + + websyn_text = websyn_path.read_text(encoding="utf-8", errors="replace") + control_text = control_path.read_text(encoding="utf-8", errors="replace") + site_runner_text = site_runner_path.read_text(encoding="utf-8", errors="replace") + docker_ports = parse_docker_ports(dockerfile_path) + asset_patterns = parse_assetpaths(assetpaths_path) + readme_reset_sites = parse_readme_reset_examples(readme_path) + + websyn_registry: tuple[list[str], int] | None = None + control_registry: tuple[list[str], int] | None = None + try: + websyn_registry = parse_site_array(websyn_text, str(websyn_path)) + except (SyntaxError, ValueError) as exc: + collector.error(str(exc), file=str(websyn_path)) + try: + control_registry = parse_site_array(control_text, str(control_path)) + except (SyntaxError, ValueError) as exc: + collector.error(str(exc), file=str(control_path)) + if collector.errors: + return incomplete_audit_result( + root, + strict=strict, + collector=collector, + site_directories_found=len(site_dirs), + ) + + assert websyn_registry is not None + assert control_registry is not None + websyn_sites, websyn_base_port = websyn_registry + control_sites, control_base_port = control_registry + websyn_port_map = build_port_map(websyn_sites, websyn_base_port) + control_port_map = build_port_map(control_sites, control_base_port) + exposed_ports = set(docker_ports["exposed_ports"]) + + if site is not None: + all_known_sites = set(site_dirs) | set(websyn_sites) | set(control_sites) + if site not in all_known_sites: + collector.error(f"site '{site}' was not found in sites/ or registry lists", site=site) + sites_to_check = [site] + else: + sites_to_check = sorted(set(site_dirs) | set(websyn_sites) | set(control_sites)) + + if len(websyn_sites) != len(set(websyn_sites)): + duplicates = sorted( + item for item in set(websyn_sites) if websyn_sites.count(item) > 1 + ) + collector.error( + f"websyn_start.sh contains duplicate site slugs: {duplicates}", + file=str(websyn_path), + ) + + if len(control_sites) != len(set(control_sites)): + duplicates = sorted( + item for item in set(control_sites) if control_sites.count(item) > 1 + ) + collector.error( + f"control_server.py contains duplicate site slugs: {duplicates}", + file=str(control_path), + ) + + if websyn_sites != control_sites: + collector.error( + "websyn_start.sh and control_server.py site registration lists do not match exactly", + file=str(websyn_path), + ) + + if websyn_base_port != control_base_port: + collector.error( + "websyn_start.sh and control_server.py use different BASE_PORT values", + file=str(websyn_path), + port=websyn_base_port, + ) + + if 8101 not in exposed_ports: + collector.error("Dockerfile is missing EXPOSE 8101", file=str(dockerfile_path), port=8101) + for invalid_expose_token in docker_ports["invalid_expose_tokens"]: + collector.error(invalid_expose_token, file=str(dockerfile_path)) + + registered_site_ports = { + site_slug: websyn_port_map[site_slug] for site_slug in websyn_sites + } + + if "from app import app" not in site_runner_text: + collector.warn( + "site_runner.py no longer imports app.py directly; app.py entrypoint checks may need review", + file=str(site_runner_path), + ) + + for reset_site in sorted(readme_reset_sites): + if reset_site not in registered_site_ports: + collector.warn( + f"README reset example references unknown site '{reset_site}'", + file=str(readme_path), + site=reset_site, + ) + + if not asset_patterns: + collector.warn(".assetpaths is missing or empty", file=str(assetpaths_path)) + + site_summaries: list[SiteSummary] = [] + total_task_count = 0 + task_files_checked = 0 + task_ports_seen: dict[int, str] = {} + + for site_slug in sites_to_check: + site_dir = sites_root / site_slug + in_sites_dir = site_dir.is_dir() + in_websyn = site_slug in websyn_port_map + in_control = site_slug in control_port_map + websyn_port = websyn_port_map.get(site_slug) + control_port = control_port_map.get(site_slug) + + if not slug_is_valid(site_slug): + collector.warn("site slug contains characters outside [a-z0-9_]", site=site_slug) + + if in_sites_dir and not in_websyn and not in_control: + collector.warn( + "site directory exists but is not registered in websyn_start.sh or control_server.py", + file=str(site_dir), + site=site_slug, + ) + + if (in_websyn or in_control) and not in_sites_dir: + collector.error( + "site is registered but its directory is missing under sites/", + file=str(site_dir), + site=site_slug, + port=websyn_port or control_port, + ) + + has_app = (site_dir / "app.py").exists() if in_sites_dir else False + has_seed_data = (site_dir / "seed_data.py").exists() if in_sites_dir else False + tasks_path = site_dir / "tasks.jsonl" + has_tasks = tasks_path.exists() if in_sites_dir else False + + if in_sites_dir and in_websyn and not has_app: + collector.error( + "registered site is missing app.py required by site_runner.py", + file=str(site_dir / "app.py"), + site=site_slug, + port=websyn_port, + ) + + assetpaths_instance_seed = pattern_covers_site(asset_patterns, site_slug, "instance_seed") + assetpaths_images = pattern_covers_site(asset_patterns, site_slug, "static/images") + assetpaths_external_cache = pattern_covers_site( + asset_patterns, site_slug, "static/external_cache" + ) + + if in_sites_dir and not assetpaths_instance_seed: + collector.warn( + "site is not covered by .assetpaths for instance_seed", + file=str(assetpaths_path), + site=site_slug, + ) + if in_sites_dir and not assetpaths_images: + collector.warn( + "site is not covered by .assetpaths for static/images", + file=str(assetpaths_path), + site=site_slug, + ) + if in_sites_dir and not assetpaths_external_cache: + collector.warn( + "site is not covered by .assetpaths for static/external_cache", + file=str(assetpaths_path), + site=site_slug, + ) + + if in_sites_dir: + for runtime_subdir in RUNTIME_SUBDIRS: + runtime_path = site_dir / runtime_subdir + if runtime_path.is_file(): + collector.warn( + f"runtime-like path is a file: {runtime_subdir}", + file=str(runtime_path), + site=site_slug, + ) + elif runtime_path.is_dir(): + tracked = git_tracked_files(root, site_slug, runtime_subdir) + if tracked: + collector.warn( + f"runtime-like path has tracked files: {runtime_subdir}", + file=str(runtime_path), + site=site_slug, + ) + elif any(runtime_path.iterdir()): + collector.warn( + f"runtime-like path contains files: {runtime_subdir}", + file=str(runtime_path), + site=site_slug, + ) + + task_count = 0 + task_port = None + task_web_name = None + if in_sites_dir: + task_count, task_port, task_web_name = parse_tasks_jsonl(tasks_path, collector, site_slug) + total_task_count += task_count + if tasks_path.exists(): + task_files_checked += 1 + + if ( + task_port is not None + and task_port in task_ports_seen + and task_ports_seen[task_port] != site_slug + ): + collector.error( + f"task web URL port {task_port} is already used by site '{task_ports_seen[task_port]}'", + file=str(tasks_path), + site=site_slug, + port=task_port, + ) + elif task_port is not None: + task_ports_seen[task_port] = site_slug + + expected_port = websyn_port or control_port + if task_port is not None and expected_port is not None and task_port != expected_port: + collector.warn( + f"task web URL port {task_port} does not match registered port {expected_port}", + file=str(tasks_path), + site=site_slug, + port=task_port, + ) + + if expected_port is not None and expected_port not in exposed_ports: + collector.error( + "Dockerfile does not expose the registered site port", + file=str(dockerfile_path), + site=site_slug, + port=expected_port, + ) + + if websyn_port is not None and not (40000 <= websyn_port <= 49999): + collector.warn( + "registered port falls outside the expected 40000+ WebHarbor range", + file=str(websyn_path), + site=site_slug, + port=websyn_port, + ) + + site_errors = 0 + site_warnings = 0 + for finding in collector.errors: + if finding.site == site_slug: + site_errors += 1 + for finding in collector.warnings: + if finding.site == site_slug: + site_warnings += 1 + + site_summaries.append( + SiteSummary( + site=site_slug, + in_sites_dir=in_sites_dir, + in_websyn=in_websyn, + in_control=in_control, + websyn_port=websyn_port, + control_port=control_port, + task_file=str(tasks_path) if tasks_path.exists() else None, + task_count=task_count, + task_port=task_port, + task_web_name=task_web_name, + has_app=has_app, + has_seed_data=has_seed_data, + has_tasks=has_tasks, + assetpaths_instance_seed=assetpaths_instance_seed, + assetpaths_images=assetpaths_images, + assetpaths_external_cache=assetpaths_external_cache, + warnings=site_warnings, + errors=site_errors, + ) + ) + + ports_payload = { + "websyn_base_port": websyn_base_port, + "control_base_port": control_base_port, + "websyn_ports": websyn_port_map, + "control_ports": control_port_map, + "docker_exposed_ports": docker_ports["exposed_ports"], + } + + return AuditResult( + root=str(root), + strict=strict, + site_directories_found=len(site_dirs), + registered_sites_found=len(set(websyn_sites) | set(control_sites)), + ports_found=len(set(registered_site_ports.values())), + task_files_checked=task_files_checked, + task_count=total_task_count, + errors=collector.errors, + warnings=collector.warnings, + sites=site_summaries, + ports=ports_payload, + ) + + +def render_human(result: AuditResult) -> str: + lines = [ + ( + f"Checked {result.site_directories_found} site directorie(s), " + f"{result.registered_sites_found} registered site(s), " + f"{result.ports_found} port(s), " + f"{result.task_files_checked} task file(s), " + f"{result.task_count} task(s)" + ), + f"Errors: {len(result.errors)} Warnings: {len(result.warnings)}", + "", + ] + + for site in result.sites: + status = human_status(site.errors, site.warnings) + port_display = site.websyn_port if site.websyn_port is not None else site.control_port + lines.append( + ( + f"[{status}] {site.site}: " + f"registered={site.in_websyn and site.in_control} " + f"dir={site.in_sites_dir} " + f"port={port_display} " + f"tasks={site.task_count}" + ) + ) + + findings = [*result.errors, *result.warnings] + if findings: + lines.append("") + for finding in findings: + parts = [finding.severity] + if finding.file: + parts.append(f"file={finding.file}") + if finding.site: + parts.append(f"site={finding.site}") + if finding.port is not None: + parts.append(f"port={finding.port}") + if finding.line is not None: + parts.append(f"line={finding.line}") + if finding.task_id: + parts.append(f"task_id={finding.task_id}") + parts.append(finding.message) + lines.append(" | ".join(parts)) + + return "\n".join(lines) + + +def build_arg_parser() -> argparse.ArgumentParser: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--site", help="Audit only one site slug under sites//") + parser.add_argument("--strict", action="store_true", help="Treat warnings as failures") + parser.add_argument("--json", action="store_true", help="Print machine-readable JSON only") + return parser + + +def main( + argv: list[str] | None = None, + *, + root: Path | None = None, + stdout: Any = None, +) -> int: + parser = build_arg_parser() + args = parser.parse_args(argv) + target_root = root or Path(__file__).resolve().parents[1] + result = audit_repository(target_root, site=args.site, strict=args.strict) + stream = stdout if stdout is not None else sys.stdout + + if args.json: + json.dump(result.to_json_dict(), stream, indent=2, sort_keys=True) + stream.write("\n") + else: + stream.write(render_human(result)) + stream.write("\n") + return result.exit_code + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/scripts/test_audit_site_registry.py b/scripts/test_audit_site_registry.py new file mode 100644 index 00000000..1df98de9 --- /dev/null +++ b/scripts/test_audit_site_registry.py @@ -0,0 +1,464 @@ +#!/usr/bin/env python3 +"""Tests for scripts/audit_site_registry.py.""" + +from __future__ import annotations + +import io +import json +import sys +import tempfile +import textwrap +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent)) +import audit_site_registry as audit # noqa: E402 + + +def write(path: Path, content: str) -> None: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(textwrap.dedent(content).lstrip("\n"), encoding="utf-8") + + +def build_repo( + root: Path, + *, + sites: list[str] | None = None, + site_dirs: list[str] | None = None, + task_ports: dict[str, int] | None = None, + docker_expose: str = "EXPOSE 8101 40000-40014", +) -> None: + sites = ["amazon"] if sites is None else sites + site_dirs = list(sites) if site_dirs is None else site_dirs + task_ports = ( + {site: 40000 + index for index, site in enumerate(sites)} + if task_ports is None + else task_ports + ) + for index, site in enumerate(site_dirs): + task_ports.setdefault(site, 41000 + index) + + write( + root / "README.md", + """ + # WebHarbor + + curl -X POST http://localhost:8101/reset/amazon + """, + ) + write( + root / "CONTRIBUTING.md", + """ + # Contributing + """, + ) + write( + root / "websyn_start.sh", + f""" + #!/bin/bash + SITES=({' '.join(sites)}) + BASE_PORT=40000 + """, + ) + quoted_sites = ", ".join(repr(site) for site in sites) + write( + root / "control_server.py", + f""" + SITES = [{quoted_sites}] + BASE_PORT = 40000 + """, + ) + write( + root / "site_runner.py", + """ + from app import app + """, + ) + write( + root / "Dockerfile", + f""" + FROM python:3.12-slim-bookworm + {docker_expose} + """, + ) + write( + root / ".assetpaths", + """ + sites/*/instance_seed/ + sites/*/static/images/ + sites/*/static/external_cache/ + """, + ) + + sites_root = root / "sites" + sites_root.mkdir(parents=True, exist_ok=True) + for site in site_dirs: + site_root = sites_root / site + write(site_root / "app.py", "from flask import Flask\napp = Flask(__name__)\n") + write(site_root / "_health.py", "pass\n") + write(site_root / "templates" / "index.html", "\n") + write( + site_root / "tasks.jsonl", + json.dumps( + { + "web_name": site.replace("_", " ").title(), + "id": f"{site}--0", + "ques": f"Find something on {site}", + "web": f"http://localhost:{task_ports[site]}/", + "upstream_url": f"https://{site}.example.com/", + } + ) + + "\n", + ) + + +class AuditSiteRegistryTests(unittest.TestCase): + def test_valid_minimal_registry_passes(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + build_repo(root) + result = audit.audit_repository(root) + self.assertEqual(result.exit_code, 0) + self.assertEqual(len(result.errors), 0) + + def test_duplicate_task_url_ports_fail(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + build_repo( + root, + sites=["amazon", "apple"], + task_ports={"amazon": 40000, "apple": 40000}, + ) + result = audit.audit_repository(root) + self.assertNotEqual(result.exit_code, 0) + self.assertTrue( + any( + "task web URL port 40000 is already used by site 'amazon'" + in error.message + for error in result.errors + ), + result.errors, + ) + + def test_site_directory_missing_registration_warns(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + build_repo(root, sites=["amazon"], site_dirs=["amazon", "orphan_site"]) + result = audit.audit_repository(root) + messages = [warning.message for warning in result.warnings] + self.assertTrue( + any("not registered" in message for message in messages), + messages, + ) + self.assertEqual(result.exit_code, 0) + + def test_registered_site_missing_directory_fails(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + build_repo(root, sites=["amazon", "apple"], site_dirs=["amazon"]) + result = audit.audit_repository(root) + messages = [error.message for error in result.errors] + self.assertTrue( + any("directory is missing" in message for message in messages), + messages, + ) + + def test_task_url_port_mismatch_warns(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + build_repo(root, task_ports={"amazon": 49999}) + result = audit.audit_repository(root) + messages = [warning.message for warning in result.warnings] + self.assertTrue( + any("does not match registered port" in message for message in messages), + messages, + ) + self.assertEqual(result.exit_code, 0) + + def test_warning_only_exits_zero_but_strict_fails(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + build_repo(root, task_ports={"amazon": 49999}) + normal = audit.audit_repository(root, strict=False) + strict = audit.audit_repository(root, strict=True) + self.assertEqual(normal.exit_code, 0) + self.assertEqual(strict.exit_code, 1) + + def test_json_output_is_valid(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + build_repo(root) + buffer = io.StringIO() + exit_code = audit.main(["--json"], root=root, stdout=buffer) + payload = json.loads(buffer.getvalue()) + self.assertEqual(exit_code, 0) + self.assertIn("summary", payload) + self.assertIn("sites", payload) + self.assertIn("ports", payload) + + def test_invalid_task_port_is_reported_without_traceback(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + build_repo(root) + write( + root / "sites" / "amazon" / "tasks.jsonl", + json.dumps( + { + "web_name": "Amazon", + "id": "amazon--0", + "ques": "Find something", + "web": "http://localhost:not-a-port/", + "upstream_url": "https://amazon.example.com/", + } + ) + + "\n", + ) + + result = audit.audit_repository(root) + + self.assertEqual(len(result.errors), 0) + self.assertTrue( + any("invalid port" in warning.message for warning in result.warnings), + result.warnings, + ) + + def test_invalid_utf8_task_file_is_reported_without_traceback(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + build_repo(root) + (root / "sites" / "amazon" / "tasks.jsonl").write_bytes(b"\xff\xfe") + + result = audit.audit_repository(root) + + self.assertEqual(result.exit_code, 1) + self.assertTrue( + any("UTF-8" in error.message for error in result.errors), + result.errors, + ) + + def test_missing_required_repository_file_is_reported_without_traceback(self) -> None: + required_files = ("websyn_start.sh", "control_server.py", "site_runner.py", "Dockerfile") + for required_file in required_files: + with self.subTest(required_file=required_file): + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + build_repo(root) + (root / required_file).unlink() + + result = audit.audit_repository(root) + + self.assertEqual(result.exit_code, 1) + self.assertTrue( + any(required_file in error.message for error in result.errors), + result.errors, + ) + + def test_shell_site_array_ignores_comments(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + build_repo(root) + write( + root / "websyn_start.sh", + """ + SITES=(amazon # explanatory comment + ) + BASE_PORT=40000 + """, + ) + + result = audit.audit_repository(root) + + self.assertEqual(result.exit_code, 0) + self.assertEqual([site.site for site in result.sites], ["amazon"]) + + def test_indented_shell_registry_declarations_are_supported(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + build_repo(root) + write( + root / "websyn_start.sh", + """ + #!/bin/bash + SITES=(amazon) + BASE_PORT=40000 + """, + ) + + result = audit.audit_repository(root) + + self.assertEqual(result.exit_code, 0) + self.assertEqual([site.site for site in result.sites], ["amazon"]) + + def test_commented_registry_declarations_are_ignored(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + build_repo(root) + write( + root / "websyn_start.sh", + """ + # SITES=(wrong_site) + # BASE_PORT=49999 + SITES=(amazon) + BASE_PORT=40000 + """, + ) + + result = audit.audit_repository(root) + + self.assertEqual(result.exit_code, 0) + self.assertEqual([site.site for site in result.sites], ["amazon"]) + + def test_function_local_registry_declarations_are_ignored(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + build_repo(root) + write( + root / "control_server.py", + """ + def helper(): + SITES = ["wrong_site"] + BASE_PORT = 49999 + + SITES = ["amazon"] + BASE_PORT = 40000 + """, + ) + + result = audit.audit_repository(root) + + self.assertEqual(result.exit_code, 0) + self.assertEqual([site.site for site in result.sites], ["amazon"]) + + def test_malformed_registry_is_reported_without_traceback(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + build_repo(root) + write(root / "control_server.py", "SITES = [unknown_name]\nBASE_PORT = 40000\n") + + result = audit.audit_repository(root) + + self.assertEqual(result.exit_code, 1) + self.assertTrue( + any("control_server.py" in (error.file or "") for error in result.errors), + result.errors, + ) + + def test_json_output_remains_valid_for_repository_errors(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + build_repo(root) + (root / "Dockerfile").unlink() + buffer = io.StringIO() + + exit_code = audit.main(["--json"], root=root, stdout=buffer) + payload = json.loads(buffer.getvalue()) + + self.assertEqual(exit_code, 1) + self.assertEqual(payload["summary"]["errors"], 1) + self.assertIn("Dockerfile", payload["errors"][0]["message"]) + + def test_runtime_like_regular_file_is_reported_as_json(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + build_repo(root) + write(root / "sites" / "amazon" / "logs", "runtime output\n") + buffer = io.StringIO() + + exit_code = audit.main(["--json", "--strict"], root=root, stdout=buffer) + payload = json.loads(buffer.getvalue()) + + self.assertEqual(exit_code, 1) + self.assertTrue( + any( + warning["message"] == "runtime-like path is a file: logs" + for warning in payload["warnings"] + ), + payload["warnings"], + ) + + def test_explicit_assetpaths_cover_site_without_wildcard(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + build_repo(root) + write( + root / ".assetpaths", + """ + sites/amazon/instance_seed/ + sites/amazon/static/images/ + sites/amazon/static/external_cache/ + """, + ) + + result = audit.audit_repository(root, strict=True) + + self.assertEqual(result.exit_code, 0) + self.assertFalse( + any(".assetpaths" in warning.message for warning in result.warnings), + result.warnings, + ) + + def test_brand_web_name_does_not_require_slug_match(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + build_repo(root) + write( + root / "sites" / "amazon" / "tasks.jsonl", + json.dumps( + { + "web_name": "Whole Foods Market", + "id": "amazon--0", + "ques": "Find something", + "web": "http://localhost:40000/", + "upstream_url": "https://amazon.example.com/", + } + ) + + "\n", + ) + + result = audit.audit_repository(root, strict=True) + + self.assertEqual(result.exit_code, 0) + self.assertFalse( + any("does not look related" in warning.message for warning in result.warnings), + result.warnings, + ) + + def test_docker_expose_protocol_suffix_is_supported(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + build_repo(root, docker_expose="expose 8101/tcp 40000/tcp") + + result = audit.audit_repository(root) + + self.assertEqual(result.exit_code, 0) + self.assertEqual(result.ports["docker_exposed_ports"], [8101, 40000]) + + def test_descending_docker_port_range_is_an_error(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + build_repo(root, docker_expose="EXPOSE 8101 40000-39999") + + result = audit.audit_repository(root) + + self.assertEqual(result.exit_code, 1) + self.assertTrue( + any("descending EXPOSE range" in error.message for error in result.errors), + result.errors, + ) + + def test_out_of_range_docker_port_is_an_error(self) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + root = Path(tmpdir) + build_repo(root, docker_expose="EXPOSE 8101 40000 70000") + + result = audit.audit_repository(root) + + self.assertEqual(result.exit_code, 1) + self.assertTrue( + any("out-of-range EXPOSE token" in error.message for error in result.errors), + result.errors, + ) + + +if __name__ == "__main__": + unittest.main()