diff --git a/README.md b/README.md index 4080460..e7fd673 100644 --- a/README.md +++ b/README.md @@ -164,16 +164,38 @@ The same rule makes the combinations explicit rather than order-dependent: | `llm: hosted` + any provider key | refused: pick one | | `llm: hosted` + `license_key` | refused: use `llm: license` | | `llm: license` without `license_key` | refused: names the secret to add | +| `llm: hosted` or `license` + `model`, `agent_model` or `parsing_model` | refused: CodeBoarding picks the model on its own key; bring your own key to choose one | | `llm: anthropic` + `anthropic_api_key` | Anthropic, directly | | `llm: anthropic`, key empty or absent | refused: names the input and the secret | | `llm: anthropic` + `openai_api_key` | refused: a second provider's key | | `llm: anthropic` + key + `license_key` | Anthropic, on a CodeBoarding plan | A licence alongside your own key is deliberately allowed: it says "my CodeBoarding plan, -my own tokens". **Your key always wins.** A direct provider call never reaches -CodeBoarding, so the licence is recorded and reported but not spent, and nothing meters -that combination today. The job summary says so on every run, rather than leaving you to -infer it from the tier name. +my own tokens". **Your key always wins.** The analysis calls your provider directly, so +the licence is recorded and reported but not spent, and nothing meters that combination +today. The job summary says so on every run, rather than leaving you to infer it from the +tier name. + +The review guide and architecture checks are the exception to "directly": their code +runs on CodeBoarding's service, so your key travels with that request, is used for that run +and is neither stored nor logged. A provider the service cannot call, such as a custom base +URL, LiteLLM or a self-hosted Ollama, keeps its key on the runner: those features make no +model call, and the run never falls back to CodeBoarding's model. + +The action independently calls `POST /v1/review-guide` and `POST /v1/architecture` on the +same `review_guide_url` base URL. There is no discovery request. Both receive the same +required analysis inputs. The guide endpoint returns `{guide, usage, timingsMs}`; the action +saves the bare guide as `review_guide.json`. Architecture returns `{envelope, usage}`; its +envelope is saved unchanged as `closed/architecture.json`, including partial, skipped and +failed results. Failure of either request does not prevent the other or fail the review. + +Only architecture receives optional `source.json`: tracked Python, JavaScript/TypeScript +and C# files (`.py`, `.ts`, `.tsx`, `.js`, `.jsx`, `.mjs`, `.cjs`, `.cs`, `.csproj`), excluding +dependency/build directories, docs/public/fixtures, minified JS and TypeScript declarations. +Symlinks are not read. Individual files are capped at 300 KB and source at 16 MB. If adding +source exceeds the 4.3 MB compressed request limit, it is omitted with a reason; architecture +still runs with the required inputs. `closed_features_source: false` also omits source. +An unsupported provider skips the guide, but architecture can still return measured findings. ### Providers @@ -230,7 +252,9 @@ a CodeBoarding service issue. Own-provider quota failures retain their provider ## Model selection -All model inputs are optional and are passed directly to Core without action-side validation: +All model inputs are optional and are passed directly to Core. The action checks one thing: with +`llm: hosted` or `license` it refuses them, because CodeBoarding picks the model on its own key. +Bring your own key to choose one: ```yaml with: @@ -347,7 +371,8 @@ The first sync after the merge creates `codeboarding/baseline`, catching up from | `github_token` | both | `${{ github.token }}` | Token for comments and sync delivery. | | `codeboarding_analysis_location` | both | `codeboarding_branch` | Where the synced branch's analysis is stored: `codeboarding_branch` (on `codeboarding/baseline`, for the default branch; the code is never written) or `in_place` (committed as `.codeboarding/` on the synced branch). Sync saves it there and review reads it from there. | | `warmstart_retention_days` | review | `1` | Days to keep the reusable analysis. Only the next run reads it. | -| `review_guide_url` | review | `https://jxivqwx4or6oip3krgdmdaiunm0iaybf.lambda-url.us-east-1.on.aws/` | Experimental. Production service base URL for the pull request's review guide. The run sends it this analysis and stores `review_guide.json` in the review artifact. Set to `''` to disable. Never fails the run. | +| `review_guide_url` | review | `https://jxivqwx4or6oip3krgdmdaiunm0iaybf.lambda-url.us-east-1.on.aws/` | Experimental. Shared base URL for `/v1/review-guide` and `/v1/architecture`. Saves `review_guide.json` and `closed/architecture.json` in the review artifact. Set to `''` to disable both. Never fails the run. | +| `closed_features_source` | review | `true` | Whether architecture checks may read this repository's source. Only the architecture request carries it, capped in size, and the service keeps nothing. The model sees code around a finding, never the tree. With `false`, source-dependent checks and model review do not run, and the result says so. | `target_branch` and `sync_strategy` are deprecated, and read as the `codeboarding_analysis_location` they stand for, with a warning: `sync_strategy: push` as `in_place`, `sync_strategy: pull_request` as `codeboarding_branch`. `target_branch` is accepted only when it names the branch the run is on: the push trigger picks the branch. `force_full: true` is ignored with a warning. Anything the new input cannot express fails, linking to [moving an existing setup](#moving-an-existing-setup). diff --git a/action.yml b/action.yml index 00cb335..3077b84 100644 --- a/action.yml +++ b/action.yml @@ -145,12 +145,19 @@ inputs: default: '2' review_guide_url: description: >- - Experimental. The CodeBoarding service that writes a pull request's review guide (main - changes, each change as numbered steps on the map). Defaults to the production service. - A review run sends it this run's analysis and stores the guide in the review artifact for - the platform to show. Set to an empty string to disable. + Experimental. Service base URL for POST /v1/review-guide and POST /v1/architecture. + Defaults to production. Both receive this run's analysis independently; results are stored + as review_guide.json and closed/architecture.json. Set to an empty string to disable both. required: false default: 'https://jxivqwx4or6oip3krgdmdaiunm0iaybf.lambda-url.us-east-1.on.aws/' + closed_features_source: + description: >- + Whether architecture checks may read this repository's source, for checks that + need the whole tree and to verify a finding. Only the architecture request carries it, capped in + size, and the service keeps nothing. The model sees the code around a finding, never the tree. + With 'false' those checks and the model review do not run, and the result says so. + required: false + default: 'true' github_token: description: 'Token used for comments and sync delivery.' required: false @@ -301,6 +308,9 @@ runs: ACTION_PATH: ${{ github.action_path }} CB_IN_LLM: ${{ inputs.llm }} CB_IN_LICENSE_KEY: ${{ inputs.license_key }} + CB_IN_MODEL: ${{ inputs.model }} + CB_IN_AGENT_MODEL: ${{ inputs.agent_model }} + CB_IN_PARSING_MODEL: ${{ inputs.parsing_model }} CB_IN_ANTHROPIC_API_KEY: ${{ inputs.anthropic_api_key }} CB_IN_AWS_BEDROCK_API_KEY: ${{ inputs.aws_bedrock_api_key }} CB_IN_AWS_BEDROCK_REGION: ${{ inputs.aws_bedrock_region }} @@ -694,15 +704,16 @@ runs: ANALYSED_FILES_CHANGED: ${{ steps.review_render.outputs.analysed_files_changed }} run: "$GITHUB_ACTION_PATH/scripts/action/build-review-artifact.sh" - # After the artifact holds the analysis and before it is uploaded: the guide is one more file - # in it. Uses production by default; an empty URL disables it. It never fails the run. - - name: Build review guide - id: review_guide + # After the artifact holds the analysis and before it is uploaded: each feature is one more + # file in it. Uses production by default; an empty URL disables it. It never fails the run. + - name: Run closed features + id: closed_features if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && inputs.review_guide_url != '' continue-on-error: true shell: bash env: - REVIEW_GUIDE_URL: ${{ inputs.review_guide_url }} + CLOSED_FUNCTIONS_URL: ${{ inputs.review_guide_url }} + CLOSED_FEATURES_SOURCE: ${{ inputs.closed_features_source }} REPOSITORY: ${{ github.repository }} PR_NUMBER: ${{ steps.guard.outputs.pr_number }} ARTIFACT_DIR: ${{ steps.review_artifact.outputs.artifact_dir }} @@ -711,10 +722,32 @@ runs: CHECKOUT_DIR: ${{ github.workspace }}/.codeboarding-target GITHUB_TOKEN: ${{ inputs.github_token }} LICENSE_KEY: ${{ inputs.license_key }} + LLM: ${{ inputs.llm }} + MODEL: ${{ inputs.model }} + AGENT_MODEL: ${{ inputs.agent_model }} + ANTHROPIC_API_KEY: ${{ inputs.anthropic_api_key }} + AWS_BEDROCK_API_KEY: ${{ inputs.aws_bedrock_api_key }} + AWS_BEDROCK_REGION: ${{ inputs.aws_bedrock_region }} + CEREBRAS_API_KEY: ${{ inputs.cerebras_api_key }} + DEEPSEEK_API_KEY: ${{ inputs.deepseek_api_key }} + DEEPSEEK_BASE_URL: ${{ inputs.deepseek_base_url }} + GLM_API_KEY: ${{ inputs.glm_api_key }} + GLM_BASE_URL: ${{ inputs.glm_base_url }} + GOOGLE_API_KEY: ${{ inputs.google_api_key }} + KIMI_API_KEY: ${{ inputs.kimi_api_key }} + KIMI_BASE_URL: ${{ inputs.kimi_base_url }} OLLAMA_API_KEY: ${{ inputs.ollama_api_key }} - OPENROUTER_API_KEY: ${{ inputs.openrouter_api_key }} + OLLAMA_BASE_URL: ${{ inputs.ollama_base_url }} OPENAI_API_KEY: ${{ inputs.openai_api_key }} - run: python3 "$GITHUB_ACTION_PATH/scripts/review_guide.py" + OPENAI_BASE_URL: ${{ inputs.openai_base_url }} + OPENROUTER_API_KEY: ${{ inputs.openrouter_api_key }} + OPPER_API_KEY: ${{ inputs.opper_api_key }} + ORCAROUTER_API_KEY: ${{ inputs.orcarouter_api_key }} + REQUESTY_API_KEY: ${{ inputs.requesty_api_key }} + REQUESTY_BASE_URL: ${{ inputs.requesty_base_url }} + VERCEL_API_KEY: ${{ inputs.vercel_api_key }} + VERCEL_BASE_URL: ${{ inputs.vercel_base_url }} + run: python3 "$GITHUB_ACTION_PATH/scripts/closed_features.py" - name: Upload review artifact id: upload_review_artifact_dotcom diff --git a/scripts/action/credential_check.py b/scripts/action/credential_check.py index 8bd1949..5498e1f 100755 --- a/scripts/action/credential_check.py +++ b/scripts/action/credential_check.py @@ -49,6 +49,8 @@ "hosted_with_provider_key", "hosted_with_license", "license_with_provider_key", + "hosted_with_model", + "license_with_model", "foreign_provider_key", } ) @@ -174,6 +176,19 @@ def _reject_provider_inputs(table: dict, given: dict[str, str], llm: str, tier: ) +def _reject_model(llm: str, tier: str, environ: dict[str, str]) -> None: + """On CodeBoarding's key, CodeBoarding picks the model and its settings. Choosing one takes a + key of your own, and a workflow that names one here would otherwise run on another without + saying so.""" + for name in ("model", "agent_model", "parsing_model"): + if environ.get(f"CB_IN_{name.upper()}", "").strip(): + raise ConfigError( + f"{tier}_with_model", + f"`llm: {llm}` runs on CodeBoarding's model, so `{name}` cannot be set. " + "Remove it, or set `llm` to a provider and bring your own key to choose a model.", + ) + + def _require_id_token(llm: str, environ: dict[str, str]) -> None: # Both, because the relay needs both (oidc_relay.py refuses to start without either) # and a runner can expose one without the other. Checking only the URL let that case @@ -259,6 +274,7 @@ def resolve(table: dict, environ: dict[str, str]) -> dict: if llm == "hosted": _reject_provider_inputs(table, given, llm, "hosted") + _reject_model(llm, "hosted", environ) if license_key: raise ConfigError( "hosted_with_license", @@ -270,6 +286,7 @@ def resolve(table: dict, environ: dict[str, str]) -> dict: if llm == "license": _reject_provider_inputs(table, given, llm, "license") + _reject_model(llm, "license", environ) if not license_key: raise ConfigError( "missing_license_key", diff --git a/scripts/closed_features.py b/scripts/closed_features.py new file mode 100644 index 0000000..43b7e86 --- /dev/null +++ b/scripts/closed_features.py @@ -0,0 +1,418 @@ +#!/usr/bin/env python3 +"""Call the review-guide and architecture endpoints independently after the review analysis. + +Both requests carry the same analyses, call graphs, PR metadata and changed files. Only architecture +carries optional source. Store the legacy guide in review_guide.json and the unchanged architecture +envelope in closed/architecture.json, including partial, skipped and failed results. + +Credentials follow the analysis: a run that chose its own provider uses its own key and model for +every feature, and a hosted or licensed run uses CodeBoarding's hosted model. A run whose provider the +service cannot call (a self-hosted endpoint) makes no model call at all: its results report +what they can without one, and it never falls back to the hosted model. Either way the service needs +to know the caller is this repository's run: a fresh GitHub Actions OIDC token (plus the license, the +same bearer the hosted proxy gets), or the run's GITHUB_TOKEN when the workflow cannot mint one. +Nothing here fails the run. + +Environment: CLOSED_FUNCTIONS_URL, REPOSITORY, PR_NUMBER, ARTIFACT_DIR, HEAD_ANALYSIS_PATH, +BASE_ANALYSIS_PATH, CHECKOUT_DIR, GITHUB_TOKEN, GITHUB_API_URL, and optionally LICENSE_KEY, +CLOSED_FEATURES_SOURCE, LLM, MODEL, AGENT_MODEL, _API_KEY, _BASE_URL, +ACTIONS_ID_TOKEN_REQUEST_URL/_TOKEN. +""" + +from __future__ import annotations + +import gzip +import importlib.util +import json +import os +import pickle +import re +import subprocess +import sys +from concurrent.futures import ThreadPoolExecutor +from pathlib import Path +from urllib.error import HTTPError +from urllib.parse import quote, urlparse +from urllib.request import Request, urlopen + +# A Function URL takes 6 MB, measured after base64, and a gzipped body goes through base64 on the way +# in: 4.5 MB of bundle arrives, and 5 MB is refused with a 413. +MAX_BUNDLE_BYTES = 4_300_000 +# Changed files whose contents are sent at both commits; past this a feature reads diffs only. +MAX_FILES = 300 +# Fixed hosts matching closed_functions/llm.py; never supplied by a remote service. +PROVIDER_HOSTS = { + "ollama": "ollama.com", + "openrouter": "openrouter.ai", + "openai": "api.openai.com", + "anthropic": "api.anthropic.com", + "aws_bedrock": "bedrock-runtime.{region}.amazonaws.com", + "google": "generativelanguage.googleapis.com", + "cerebras": "api.cerebras.ai", + "deepseek": "api.deepseek.com", + "glm": "open.bigmodel.cn", + "kimi": "api.moonshot.cn", + "vercel": "ai-gateway.vercel.sh", + "orcarouter": "api.orcarouter.ai", + "requesty": "router.requesty.ai", + "opper": "api.opper.ai", +} +SOURCE_EXTENSIONS = (".py", ".ts", ".tsx", ".js", ".jsx", ".mjs", ".cjs", ".cs", ".csproj") +SOURCE_EXCLUDE = re.compile(r"(^|/)(node_modules|dist|build|vendor|__fixtures__|docs|public)/|\.min\.js$|\.d\.ts$") +MAX_SOURCE_FILE_BYTES = 300_000 +MAX_SOURCE_BYTES = 16_000_000 + + +class Skip(Exception): + """A step that did not happen, for a reason worth one line in the log.""" + + +def _github(path: str, accept: str = "application/vnd.github+json") -> bytes: + api = os.environ.get("GITHUB_API_URL", "https://api.github.com").rstrip("/") + req = Request( + f"{api}{path}", + headers={ + "Authorization": f"Bearer {os.environ['GITHUB_TOKEN']}", + "Accept": accept, + "User-Agent": "codeboarding-action", + }, + ) + with urlopen(req, timeout=30) as res: # nosec B310 - GitHub API URL + return res.read() + + +def _github_json(path: str): + return json.loads(_github(path)) + + +def call_graph(pkl: Path) -> dict: + """A static_analysis.pkl's call graph as the service reads it: nodes and edges with call sites. + The pickle holds CodeBoarding's own classes, which the installed engine provides.""" + with pkl.open("rb") as f: + results = pickle.load(f) # nosec B301 - written by this run's own analysis + nodes, edges = {}, [] + for lang in results.results.values(): + graph = lang.cfg.graph + for name, n in graph.nodes.items(): + nodes[name] = { + "file": n.file_path, + "start": n.line_start, + "end": n.line_end, + "callable": bool(n.is_callable()), + "cb": bool(n.is_callback_or_anonymous()), + } + for e in graph.edges: + sites = [] + for s in vars(e).get("_call_sites") or []: + v = s if isinstance(s, (tuple, list, dict)) else vars(s) if hasattr(s, "__dict__") else str(s) + sites.append(list(v) if isinstance(v, tuple) else v) + edges.append({"s": e.get_source(), "d": e.get_destination(), "sites": sites}) + return {"nodes": nodes, "edges": edges} + + +def _diff(repo: str, base: str, head: str) -> list[dict]: + """The files changed between the two analysed commits, in the shape of a pull request's files + (the compare API returns the same fields), so a feature reads exactly what was analysed.""" + out = _github_json(f"/repos/{repo}/compare/{base}...{head}") + return [ + { + k: f.get(k) + for k in ("filename", "status", "additions", "deletions", "changes", "patch", "previous_filename") + if k in f + } + for f in out.get("files") or [] + ] + + +def _contents(repo: str, sha: str, path: str) -> str | None: + try: + raw = _github(f"/repos/{repo}/contents/{quote(path)}?ref={sha}", accept="application/vnd.github.raw") + except HTTPError: + return None + try: + return raw.decode("utf-8") + except UnicodeDecodeError: + return None + + +def collect(env: dict) -> dict: + """The required inputs shared by both endpoints.""" + repo, pr = env["REPOSITORY"], int(env["PR_NUMBER"]) + artifact = Path(env["ARTIFACT_DIR"]) + head_dir, base_dir = Path(env["HEAD_ANALYSIS_PATH"]).parent, Path(env["BASE_ANALYSIS_PATH"]).parent + meta = json.loads((artifact / "metadata.json").read_text()) + head_sha = meta["head_sha"] + # The analysis compared against the merge base, so that is the base every feature reads too. + base_sha = meta.get("merge_base_sha") or meta.get("base_sha") + for p in (head_dir / "static_analysis.pkl", base_dir / "static_analysis.pkl"): + if not p.is_file(): + raise Skip(f"no call graph at {p}") + + pull = _github_json(f"/repos/{repo}/pulls/{pr}") + commits = _github_json(f"/repos/{repo}/pulls/{pr}/commits?per_page=100") + pr_files = _diff(repo, base_sha, head_sha) + checkout = Path(env["CHECKOUT_DIR"]) + files: dict[str, str] = {} + for f in pr_files[:MAX_FILES]: + if f.get("status") != "removed": + try: + p = _checkout_file(checkout, f["filename"]) + files[f"{head_sha}:{f['filename']}"] = p.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + pass + if f.get("status") != "added": + was = f.get("previous_filename") or f["filename"] + text = _contents(repo, base_sha, was) + if text is not None: + files[f"{base_sha}:{was}"] = text + + bundle_meta = {**meta, "base_sha": base_sha, "head_sha": head_sha, "pr_number": str(pr)} + inputs = { + "analysis.json": Path(env["HEAD_ANALYSIS_PATH"]).read_text(), + "base/analysis.json": Path(env["BASE_ANALYSIS_PATH"]).read_text(), + "metadata.json": json.dumps(bundle_meta), + "pr.json": json.dumps( + { + "title": pull.get("title") or "", + "body": pull.get("body") or "", + "headRefOid": head_sha, + "commits": [(c.get("commit") or {}).get("message", "").split("\n")[0] for c in commits], + } + ), + "pr-files.json": json.dumps(pr_files), + "head-cg.json": json.dumps(call_graph(head_dir / "static_analysis.pkl")), + "base-cg.json": json.dumps(call_graph(base_dir / "static_analysis.pkl")), + "files.json": json.dumps(files), + } + return inputs + + +def _load(name: str, path: Path): + spec = importlib.util.spec_from_file_location(name, path) + assert spec and spec.loader + mod = importlib.util.module_from_spec(spec) + sys.modules.setdefault(spec.name, mod) # its dataclass looks its own module up while it is defined + spec.loader.exec_module(sys.modules[spec.name]) + return sys.modules[spec.name] + + +def _relay(): + return _load("oidc_relay", Path(__file__).with_name("oidc_relay.py")) + + +def _api_key(env: dict, llm: str) -> str: + """The key as the analysis read it, so a pasted `VAR="..."` that worked there works here too.""" + check = _load("credential_check", Path(__file__).parent / "action" / "credential_check.py") + inputs = check.load_table()["providers"].get(llm, {}).get("inputs", {}) + env_var = inputs.get(f"{llm}_api_key", f"{llm.upper()}_API_KEY") + return check._clean_key(env.get(f"{llm.upper()}_API_KEY", ""), env_var) + + +def model_headers(env: dict) -> dict[str, str]: + """What tells the service which model this run uses. Nothing for a hosted or licensed run, which + use CodeBoarding's model and settings as they stand. A run's own provider sends its key and model; one the service cannot reach sends its name alone, which + the service reads as "no model here", so a run that chose its own provider is never sent to ours. + A base URL elsewhere is a provider the service cannot reach: it calls fixed hosts only.""" + llm = env.get("LLM", "").strip().lower() + if llm in ("", "hosted", "license"): + return {} + key = _api_key(env, llm) + base = env.get(f"{llm.upper()}_BASE_URL", "").strip() + host = PROVIDER_HOSTS.get(llm) + if host and llm == "aws_bedrock": + host = host.format(region=env.get("AWS_BEDROCK_REGION", "").strip()) + if not key or not host or (base and urlparse(base).hostname != host): + return {"x-llm-provider": llm} + out = {"x-llm-api-key": key, "x-llm-provider": llm} + if env.get(f"{llm.upper()}_REGION", "").strip(): + out["x-llm-region"] = env[f"{llm.upper()}_REGION"].strip() + # The analysis's own order, so a run's closed features use its analysis model. + model = env.get("AGENT_MODEL", "").strip() or env.get("MODEL", "").strip() + if model: + out["x-llm-model"] = model + return out + + +def _bearer(env: dict) -> dict[str, str]: + url, token = env.get("ACTIONS_ID_TOKEN_REQUEST_URL", ""), env.get("ACTIONS_ID_TOKEN_REQUEST_TOKEN", "") + if not url or not token: + # Without `id-token: write` the run's own token proves the same thing: it can read this + # repository. It carries no license, so the owner's free bucket pays. + return {"Authorization": f"Bearer {env['GITHUB_TOKEN']}"} + relay = _relay() + license_file = None + if env.get("LICENSE_KEY", "").strip(): + license_file = Path(env.get("RUNNER_TEMP", "/tmp")) / "codeboarding-guide-license.txt" + license_file.write_text(env["LICENSE_KEY"].strip()) + try: + config = relay.RelayConfig( + upstream_base_url="", id_token_request_url=url, id_token_request_token=token, license_file=license_file + ) + return {"Authorization": relay._authorization(config)} + finally: + if license_file is not None: + license_file.unlink(missing_ok=True) + + +def _checkout_file(checkout: Path, name: str) -> Path: + """Reject traversal and symlinks (including parent directories) before reading source.""" + relative = Path(name) + if relative.is_absolute() or ".." in relative.parts: + raise OSError("path outside checkout") + path = checkout + for part in relative.parts: + path = path / part + if path.is_symlink(): + raise OSError("symlink in checkout") + return path + + +def collect_source(env: dict) -> tuple[str | None, str | None]: + """Tracked architecture source, within fixed size and extension limits, or an omission reason.""" + if env.get("CLOSED_FEATURES_SOURCE", "true").strip().lower() == "false": + return None, "turned off by the workflow" + checkout = Path(env["CHECKOUT_DIR"]) + listed = subprocess.run(["git", "-C", str(checkout), "ls-files", "-z"], capture_output=True, check=True).stdout + total = 0 + files: dict[str, str] = {} + for name in listed.decode("utf-8", "replace").split("\0"): + if not name or not name.endswith(SOURCE_EXTENSIONS) or SOURCE_EXCLUDE.search(name): + continue + try: + path = _checkout_file(checkout, name) + size = path.stat().st_size + if size > MAX_SOURCE_FILE_BYTES or not path.is_file(): + continue + files[name] = path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + continue + total += size + if total > MAX_SOURCE_BYTES: + return None, f"its source is over the {MAX_SOURCE_BYTES / 1e6:.0f} MB limit" + return json.dumps(files), None + + +def _no_architecture(ctx: dict, status: str, reason: str) -> dict: + """Preserve the architecture envelope even when the request cannot complete.""" + return { + "feature": "architecture", + "schema": 1, + "status": status, + "repo": ctx["repo"], + "pr": ctx["pr"], + "headSha": ctx["head"], + "generator": None, + "coverage": None, + "data": None, + "reason": reason, + } + + +def _post(url: str, body: bytes, headers: dict[str, str]) -> dict: + req = Request( + url, + data=body, + headers={**headers, "Content-Type": "application/gzip", "User-Agent": "codeboarding-action"}, + method="POST", + ) + try: + with urlopen(req, timeout=600) as res: # nosec B310 - service URL from workflow configuration + return json.loads(res.read()) + except HTTPError as e: + try: + message = json.loads(e.read()).get("message") or e.reason + except (ValueError, AttributeError): + message = e.reason + reason = f"the service answered {e.code}: {message}" + if e.code == 402: + raise Skip(reason) from e + raise RuntimeError(reason) from e + + +def _pack(ctx: dict, files: dict[str, str], omitted: dict[str, str]) -> bytes: + body = {"version": 1, "repo": ctx["repo"], "pr": ctx["pr"], "files": files} + if omitted: + body["omitted"] = omitted + return gzip.compress(json.dumps(body).encode(), compresslevel=9) + + +def _check_size(data: bytes) -> None: + if len(data) > MAX_BUNDLE_BYTES: + raise Skip(f"its bundle is {len(data) / 1e6:.1f} MB, over the service's limit") + + +def run_review_guide(base: str, ctx: dict, inputs: dict, headers: dict, artifact: Path) -> None: + try: + if "x-llm-provider" in headers and "x-llm-api-key" not in headers: + raise Skip("the service cannot call this run's provider") + data = _pack(ctx, inputs, {}) + _check_size(data) + answer = _post(f"{base.rstrip('/')}/v1/review-guide", data, headers) + guide = answer.get("guide") + if not isinstance(guide, dict): + raise ValueError("the service returned no guide") + (artifact / "review_guide.json").write_text(json.dumps(guide, ensure_ascii=False, indent=1)) + except Exception as e: + print(f"::notice::review-guide {'skipped' if isinstance(e, Skip) else 'failed'}: {e}") + + +def run_architecture(base: str, ctx: dict, inputs: dict, headers: dict, env: dict) -> dict: + try: + try: + source, why = collect_source(env) + except Exception as e: + source, why = None, f"reading it failed: {type(e).__name__}" + files, omitted = dict(inputs), {} + if source is None: + omitted["source.json"] = why or "the source was not collected" + else: + files["source.json"] = source + data = _pack(ctx, files, omitted) + if "source.json" in files and len(data) > MAX_BUNDLE_BYTES: + del files["source.json"] + omitted["source.json"] = "the repository is too large to send" + data = _pack(ctx, files, omitted) + _check_size(data) + answer = _post(f"{base.rstrip('/')}/v1/architecture", data, headers) + envelope = answer.get("envelope") + if not isinstance(envelope, dict) or envelope.get("status") not in ("ok", "partial", "skipped", "failed"): + raise ValueError("the service returned no architecture result") + return envelope + except Exception as e: + return _no_architecture(ctx, "skipped" if isinstance(e, Skip) else "failed", str(e)) + + +def main(env: dict | None = None) -> int: + env = dict(os.environ if env is None else env) + try: + artifact = Path(env["ARTIFACT_DIR"]) + meta = json.loads((artifact / "metadata.json").read_text()) + ctx = {"repo": env["REPOSITORY"], "pr": int(env["PR_NUMBER"]), "head": meta.get("head_sha")} + base = env["CLOSED_FUNCTIONS_URL"] + if not base.strip(): + return 0 + try: + headers = {**_bearer(env), **model_headers(env)} + inputs = collect(env) + except Exception as e: + reason = f"collecting inputs or credentials failed: {type(e).__name__}: {e}" + print(f"::notice::review-guide failed: {reason}") + architecture = _no_architecture(ctx, "failed", reason) + else: + with ThreadPoolExecutor(max_workers=2) as pool: + guide = pool.submit(run_review_guide, base, ctx, inputs, headers, artifact) + result = pool.submit(run_architecture, base, ctx, inputs, headers, env) + architecture = result.result() + guide.result() + path = artifact / "closed" / "architecture.json" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps(architecture, ensure_ascii=False, indent=1)) + if architecture["status"] not in ("ok", "partial"): + print(f"::notice::architecture {architecture['status']}: {architecture.get('reason')}") + except Exception as e: # never fail the review over a closed feature + print(f"::warning::Running the closed features failed: {type(e).__name__}: {e}") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/review_guide.py b/scripts/review_guide.py deleted file mode 100644 index 8698c35..0000000 --- a/scripts/review_guide.py +++ /dev/null @@ -1,252 +0,0 @@ -#!/usr/bin/env python3 -"""Ask CodeBoarding's closed functions for this pull request's review guide, and keep it in the -review artifact as ``review_guide.json``. - -Runs after the review analysis, from what the run already has: both analyses and both call graphs -(the ``static_analysis.pkl`` beside each ``analysis.json``), the head checkout, and the diff between -the two analysed commits. It packs them into one gzipped bundle, posts it, and writes the guide the -service returns next to ``analysis.json``, where the webview reads it. Nothing here builds the guide -itself; that code is CodeBoarding's and stays on its side. - -Credentials follow the analysis: the run's own Ollama, OpenRouter or OpenAI key when it has one, -otherwise CodeBoarding's hosted model. Either way the service needs to know the caller is this -repository's run: a fresh GitHub Actions OIDC token (plus the license, the same bearer the hosted -proxy gets), or the run's GITHUB_TOKEN when the workflow cannot mint one. A failure leaves the -artifact without a guide and never fails the run. - -Environment: REVIEW_GUIDE_URL, REPOSITORY, PR_NUMBER, ARTIFACT_DIR, HEAD_ANALYSIS_PATH, -BASE_ANALYSIS_PATH, CHECKOUT_DIR, GITHUB_TOKEN, GITHUB_API_URL, and optionally LICENSE_KEY, -OLLAMA_API_KEY, OPENROUTER_API_KEY, OPENAI_API_KEY, ACTIONS_ID_TOKEN_REQUEST_URL/_TOKEN. -""" - -from __future__ import annotations - -import gzip -import importlib.util -import json -import os -import pickle -import sys -from pathlib import Path -from urllib.error import HTTPError, URLError -from urllib.parse import quote -from urllib.request import Request, urlopen - -# The service's Function URL takes at most 6 MB; the bundle is gzipped JSON, sent as is. -MAX_BUNDLE_BYTES = 5_800_000 -# Changed files whose contents are sent at both commits; past this the guide reads diffs only. -MAX_FILES = 300 -OWN_KEYS = (("OLLAMA_API_KEY", "ollama"), ("OPENROUTER_API_KEY", "openrouter"), ("OPENAI_API_KEY", "openai")) - - -class Skip(Exception): - """This run gets no guide, for a reason worth one line in the log.""" - - -def _github(path: str, accept: str = "application/vnd.github+json") -> bytes: - api = os.environ.get("GITHUB_API_URL", "https://api.github.com").rstrip("/") - req = Request( - f"{api}{path}", - headers={ - "Authorization": f"Bearer {os.environ['GITHUB_TOKEN']}", - "Accept": accept, - "User-Agent": "codeboarding-action", - }, - ) - with urlopen(req, timeout=30) as res: # nosec B310 - GitHub API URL - return res.read() - - -def _github_json(path: str): - return json.loads(_github(path)) - - -def call_graph(pkl: Path) -> dict: - """A static_analysis.pkl's call graph as the service reads it: nodes and edges with call sites. - The pickle holds CodeBoarding's own classes, which the installed engine provides.""" - with pkl.open("rb") as f: - results = pickle.load(f) # nosec B301 - written by this run's own analysis - nodes, edges = {}, [] - for lang in results.results.values(): - graph = lang.cfg.graph - for name, n in graph.nodes.items(): - nodes[name] = { - "file": n.file_path, - "start": n.line_start, - "end": n.line_end, - "callable": bool(n.is_callable()), - "cb": bool(n.is_callback_or_anonymous()), - } - for e in graph.edges: - sites = [] - for s in vars(e).get("_call_sites") or []: - v = s if isinstance(s, (tuple, list, dict)) else vars(s) if hasattr(s, "__dict__") else str(s) - sites.append(list(v) if isinstance(v, tuple) else v) - edges.append({"s": e.get_source(), "d": e.get_destination(), "sites": sites}) - return {"nodes": nodes, "edges": edges} - - -def _diff(repo: str, base: str, head: str) -> list[dict]: - """The files changed between the two analysed commits, in the shape of a pull request's files - (the compare API returns the same fields), so the guide reads exactly what was analysed.""" - out = _github_json(f"/repos/{repo}/compare/{base}...{head}") - return [ - { - k: f.get(k) - for k in ("filename", "status", "additions", "deletions", "changes", "patch", "previous_filename") - if k in f - } - for f in out.get("files") or [] - ] - - -def _contents(repo: str, sha: str, path: str) -> str | None: - try: - raw = _github(f"/repos/{repo}/contents/{quote(path)}?ref={sha}", accept="application/vnd.github.raw") - except HTTPError: - return None - try: - return raw.decode("utf-8") - except UnicodeDecodeError: - return None - - -def bundle(env: dict) -> dict: - """Everything the guide is built from, as the service's bundle format names it.""" - repo, pr = env["REPOSITORY"], int(env["PR_NUMBER"]) - artifact = Path(env["ARTIFACT_DIR"]) - head_dir, base_dir = Path(env["HEAD_ANALYSIS_PATH"]).parent, Path(env["BASE_ANALYSIS_PATH"]).parent - meta = json.loads((artifact / "metadata.json").read_text()) - head_sha = meta["head_sha"] - # The analysis compared against the merge base, so that is the base the guide reads too. - base_sha = meta.get("merge_base_sha") or meta.get("base_sha") - for p in (head_dir / "static_analysis.pkl", base_dir / "static_analysis.pkl"): - if not p.is_file(): - raise Skip(f"no call graph at {p}") - - pull = _github_json(f"/repos/{repo}/pulls/{pr}") - commits = _github_json(f"/repos/{repo}/pulls/{pr}/commits?per_page=100") - pr_files = _diff(repo, base_sha, head_sha) - checkout = Path(env["CHECKOUT_DIR"]) - files: dict[str, str] = {} - for f in pr_files[:MAX_FILES]: - if f.get("status") != "removed": - p = checkout / f["filename"] - try: - files[f"{head_sha}:{f['filename']}"] = p.read_text(encoding="utf-8") - except (OSError, UnicodeDecodeError): - pass - if f.get("status") != "added": - was = f.get("previous_filename") or f["filename"] - text = _contents(repo, base_sha, was) - if text is not None: - files[f"{base_sha}:{was}"] = text - - bundle_meta = {**meta, "base_sha": base_sha, "head_sha": head_sha, "pr_number": str(pr)} - inputs = { - "analysis.json": Path(env["HEAD_ANALYSIS_PATH"]).read_text(), - "base/analysis.json": Path(env["BASE_ANALYSIS_PATH"]).read_text(), - "metadata.json": json.dumps(bundle_meta), - "pr.json": json.dumps( - { - "title": pull.get("title") or "", - "body": pull.get("body") or "", - "headRefOid": head_sha, - "commits": [(c.get("commit") or {}).get("message", "").split("\n")[0] for c in commits], - } - ), - "pr-files.json": json.dumps(pr_files), - "head-cg.json": json.dumps(call_graph(head_dir / "static_analysis.pkl")), - "base-cg.json": json.dumps(call_graph(base_dir / "static_analysis.pkl")), - "files.json": json.dumps(files), - } - return {"version": 1, "repo": repo, "pr": pr, "files": inputs} - - -def _relay(): - spec = importlib.util.spec_from_file_location("oidc_relay", Path(__file__).with_name("oidc_relay.py")) - assert spec and spec.loader - mod = importlib.util.module_from_spec(spec) - sys.modules.setdefault(spec.name, mod) # its dataclass looks its own module up while it is defined - spec.loader.exec_module(sys.modules[spec.name]) - return sys.modules[spec.name] - - -def credentials(env: dict) -> dict[str, str]: - """The request headers: the run's own key for a provider the service speaks, else the hosted - model's bearer, the same one the hosted proxy gets (a fresh OIDC token, plus the license).""" - for name, provider in OWN_KEYS: - if env.get(name, "").strip(): - # The service still checks the caller is this repository's run. - return {**_bearer(env), "x-llm-api-key": env[name].strip(), "x-llm-provider": provider} - return _bearer(env) - - -def _bearer(env: dict) -> dict[str, str]: - url, token = env.get("ACTIONS_ID_TOKEN_REQUEST_URL", ""), env.get("ACTIONS_ID_TOKEN_REQUEST_TOKEN", "") - if not url or not token: - # Without `id-token: write` the run's own token proves the same thing: it can read this - # repository. It carries no license, so the owner's free bucket pays. - return {"Authorization": f"Bearer {env['GITHUB_TOKEN']}"} - relay = _relay() - license_file = None - if env.get("LICENSE_KEY", "").strip(): - license_file = Path(env.get("RUNNER_TEMP", "/tmp")) / "codeboarding-guide-license.txt" - license_file.write_text(env["LICENSE_KEY"].strip()) - try: - config = relay.RelayConfig( - upstream_base_url="", id_token_request_url=url, id_token_request_token=token, license_file=license_file - ) - return {"Authorization": relay._authorization(config)} - finally: - if license_file is not None: - license_file.unlink(missing_ok=True) - - -def request_guide(url: str, body: bytes, headers: dict[str, str]) -> dict: - req = Request( - f"{url.rstrip('/')}/v1/review-guide", - data=body, - headers={**headers, "Content-Type": "application/gzip", "User-Agent": "codeboarding-action"}, - method="POST", - ) - try: - with urlopen(req, timeout=600) as res: # nosec B310 - the service URL the workflow names - return json.loads(res.read()) - except HTTPError as e: - try: - message = json.loads(e.read()).get("message") or e.reason - except ValueError: - message = e.reason - raise Skip(f"the guide service answered {e.code}: {message}") from e - except (URLError, TimeoutError) as e: - raise Skip(f"the guide service did not answer: {type(e).__name__}") from e - - -def main(env: dict | None = None) -> int: - env = dict(os.environ if env is None else env) - try: - data = gzip.compress(json.dumps(bundle(env)).encode(), compresslevel=9) - if len(data) > MAX_BUNDLE_BYTES: - raise Skip(f"the bundle is {len(data) / 1e6:.1f} MB, over the service's limit") - answer = request_guide(env["REVIEW_GUIDE_URL"], data, credentials(env)) - guide = answer.get("guide") - if not isinstance(guide, dict): - raise Skip("the guide service returned no guide") - out = Path(env["ARTIFACT_DIR"]) / "review_guide.json" - out.write_text(json.dumps(guide, ensure_ascii=False, indent=1)) - usage = answer.get("usage") or {} - print( - f"Review guide: {len(guide.get('changes') or [])} changes in {len(guide.get('themes') or [])} main changes; " - f"{usage.get('provider')} {usage.get('model')}, {usage.get('promptTokens', 0) + usage.get('outputTokens', 0)} tokens " - f"({'hosted' if usage.get('hosted') else 'own key'}); bundle {len(data) / 1e6:.2f} MB" - ) - except Skip as e: - print(f"::notice::No review guide for this run: {e}") - except Exception as e: # never fail the review over its guide - print(f"::warning::Building the review guide failed: {type(e).__name__}: {e}") - return 0 - - -if __name__ == "__main__": - sys.exit(main()) diff --git a/tests/test_closed_features.py b/tests/test_closed_features.py new file mode 100644 index 0000000..8ba89b1 --- /dev/null +++ b/tests/test_closed_features.py @@ -0,0 +1,410 @@ +"""Direct endpoint, source and provider contracts for the optional review requests.""" + +import gzip +import importlib.util +import json +import os +import subprocess +import sys +import tempfile +import threading +import unittest +from contextlib import nullcontext +from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer +from pathlib import Path +from unittest import mock + +_SCRIPT = Path(__file__).resolve().parent.parent / "scripts" / "closed_features.py" +_SPEC = importlib.util.spec_from_file_location("closed_features", _SCRIPT) +closed_features = importlib.util.module_from_spec(_SPEC) +sys.modules[_SPEC.name] = closed_features +_SPEC.loader.exec_module(closed_features) + +INPUTS = { + "analysis.json", + "base/analysis.json", + "metadata.json", + "pr.json", + "pr-files.json", + "head-cg.json", + "base-cg.json", + "files.json", +} +GUIDE = {"version": 1, "changes": [{"id": "u1"}]} +ENVELOPE = { + "feature": "architecture", + "schema": 1, + "status": "partial", + "repo": "o/r", + "pr": 7, + "headSha": "H", + "generator": {"model": "m", "hosted": True, "version": "v"}, + "coverage": {"languages": ["Python"], "files": {"seen": 2, "total": 2}}, + "data": {"findings": []}, + "reason": "source omitted", +} + + +class _Server: + def __init__(self, respond): + outer = self + self.requests = [] + + class Handler(BaseHTTPRequestHandler): + def log_message(self, *_args): + pass + + def _any(self): + body = self.rfile.read(int(self.headers.get("Content-Length") or 0)) + outer.requests.append( + {"path": self.path, "method": self.command, "headers": dict(self.headers), "body": body} + ) + status, payload = respond(self.path, body) + data = json.dumps(payload).encode() + self.send_response(status) + self.send_header("Content-Length", str(len(data))) + self.end_headers() + self.wfile.write(data) + + do_GET = do_POST = _any + + self.server = ThreadingHTTPServer(("127.0.0.1", 0), Handler) + self.thread = threading.Thread(target=self.server.serve_forever, daemon=True) + self.thread.start() + + @property + def url(self): + return f"http://127.0.0.1:{self.server.server_port}" + + def close(self): + self.server.shutdown() + self.thread.join() + self.server.server_close() + + +def _workspace(tmp): + artifact, head, base, checkout = (tmp / n for n in ("artifact", "head", "base", "checkout")) + for directory in (artifact, head, base, checkout): + directory.mkdir() + (artifact / "metadata.json").write_text(json.dumps({"head_sha": "H", "base_sha": "TIP", "merge_base_sha": "B"})) + for directory in (head, base): + (directory / "analysis.json").write_text('{"components": []}') + (directory / "static_analysis.pkl").touch() + (checkout / "app.py").write_text("head content") + subprocess.run(["git", "init", "-q", str(checkout)], check=True) + subprocess.run(["git", "-C", str(checkout), "add", "app.py"], check=True) + return { + "CLOSED_FUNCTIONS_URL": "http://127.0.0.1:9", + "REPOSITORY": "o/r", + "PR_NUMBER": "7", + "ARTIFACT_DIR": str(artifact), + "HEAD_ANALYSIS_PATH": str(head / "analysis.json"), + "BASE_ANALYSIS_PATH": str(base / "analysis.json"), + "CHECKOUT_DIR": str(checkout), + "GITHUB_TOKEN": "ghs_run", + "RUNNER_TEMP": str(tmp), + } + + +def _github_json(path): + if "/compare/" in path: + return {"files": [{"filename": "app.py", "status": "modified"}, {"filename": "new.py", "status": "added"}]} + if "/commits?" in path: + return [{"commit": {"message": "Return two\n\nbody"}}] + return {"title": "Two", "body": "Returns two"} + + +def _read(env, name): + return json.loads((Path(env["ARTIFACT_DIR"]) / name).read_text()) + + +class BundleTests(unittest.TestCase): + def test_analysed_commits_and_both_sides(self): + with tempfile.TemporaryDirectory() as tmp: + env = _workspace(Path(tmp)) + with ( + mock.patch.object(closed_features, "_github_json", side_effect=_github_json) as gh, + mock.patch.object(closed_features, "_contents", return_value="base content") as base, + mock.patch.object(closed_features, "call_graph", return_value={"nodes": {}, "edges": []}), + ): + bundle = closed_features.collect(env) + self.assertIn("/repos/o/r/compare/B...H", [c.args[0] for c in gh.call_args_list]) + self.assertEqual(json.loads(bundle["files.json"]), {"H:app.py": "head content", "B:app.py": "base content"}) + base.assert_called_once_with("o/r", "B", "app.py") + self.assertEqual(json.loads(bundle["metadata.json"])["base_sha"], "B") + self.assertEqual(json.loads(bundle["pr.json"])["commits"], ["Return two"]) + self.assertEqual(set(bundle), INPUTS) + + def test_changed_file_symlink_is_not_read(self): + with tempfile.TemporaryDirectory() as tmp: + env = _workspace(Path(tmp)) + path = Path(env["CHECKOUT_DIR"]) / "app.py" + path.unlink() + path.symlink_to(Path(env["ARTIFACT_DIR"]) / "metadata.json") + with ( + mock.patch.object(closed_features, "_github_json", side_effect=_github_json), + mock.patch.object(closed_features, "_contents", return_value=None), + mock.patch.object(closed_features, "call_graph", return_value={}), + ): + self.assertEqual(json.loads(closed_features.collect(env)["files.json"]), {}) + + +class CredentialTests(unittest.TestCase): + def test_without_oidc_the_runs_own_token_proves_the_repository(self): + self.assertEqual(closed_features._bearer({"GITHUB_TOKEN": "ghs_run"}), {"Authorization": "Bearer ghs_run"}) + + def test_oidc_bearer_includes_license_and_removes_temporary_file(self): + mint = _Server(lambda p, b: (200, {"value": "jwt.token.sig"})) + self.addCleanup(mint.close) + with tempfile.TemporaryDirectory() as tmp: + headers = closed_features._bearer( + { + "ACTIONS_ID_TOKEN_REQUEST_URL": f"{mint.url}/token?x=1", + "ACTIONS_ID_TOKEN_REQUEST_TOKEN": "req", + "LICENSE_KEY": "LIC", + "RUNNER_TEMP": tmp, + } + ) + self.assertEqual(list(Path(tmp).iterdir()), []) + self.assertEqual(headers, {"Authorization": "Bearer jwt.token.sig~codeboarding-license~LIC"}) + self.assertIn("audience=codeboarding-proxy", mint.requests[0]["path"]) + + def test_hosted_and_license_send_no_model(self): + for llm in ("", "hosted", "license"): + self.assertEqual(closed_features.model_headers({"LLM": llm, "OPENAI_API_KEY": "leftover"}), {}) + + def test_each_fixed_provider_and_analysis_model(self): + for provider, host in closed_features.PROVIDER_HOSTS.items(): + with self.subTest(provider=provider): + env = { + "LLM": provider, + f"{provider.upper()}_API_KEY": " k ", + "MODEL": "general", + "AGENT_MODEL": "analysis", + } + if provider == "aws_bedrock": + env["AWS_BEDROCK_REGION"] = "eu-west-1" + host = host.format(region="eu-west-1") + env[f"{provider.upper()}_BASE_URL"] = f"https://{host}/v1" + headers = closed_features.model_headers(env) + self.assertEqual(headers["x-llm-api-key"], "k") + self.assertEqual(headers["x-llm-model"], "analysis") + if provider == "aws_bedrock": + self.assertEqual(headers["x-llm-region"], "eu-west-1") + + def test_key_is_unwrapped_the_way_the_analysis_reads_it(self): + bedrock = {"LLM": "aws_bedrock", "AWS_BEDROCK_REGION": "us-east-1"} + for env, want in ( + ({"LLM": "anthropic", "ANTHROPIC_API_KEY": ' ANTHROPIC_API_KEY="sk-ant-test"\n'}, "sk-ant-test"), + ({"LLM": "openai", "OPENAI_API_KEY": "'sk-test'"}, "sk-test"), + ({**bedrock, "AWS_BEDROCK_API_KEY": "AWS_BEARER_TOKEN_BEDROCK=abc="}, "abc="), + ): + with self.subTest(llm=env["LLM"]): + self.assertEqual(closed_features.model_headers(env)["x-llm-api-key"], want) + + def test_unsupported_self_hosted_and_missing_key_never_fall_back(self): + for env in ( + {"LLM": "litellm", "LITELLM_API_KEY": "k"}, + {"LLM": "ollama", "OLLAMA_API_KEY": "k", "OLLAMA_BASE_URL": "http://10.0.0.5:11434"}, + {"LLM": "openai", "OPENAI_API_KEY": "k", "OPENAI_BASE_URL": "https://gateway.corp/v1"}, + {"LLM": "anthropic"}, + ): + self.assertEqual(closed_features.model_headers(env), {"x-llm-provider": env["LLM"]}) + + +class EndpointTests(unittest.TestCase): + def setUp(self): + tmp = tempfile.TemporaryDirectory() + self.addCleanup(tmp.cleanup) + self.env = _workspace(Path(tmp.name)) + for name, kwargs in ( + ("_github_json", {"side_effect": _github_json}), + ("_contents", {"return_value": "base"}), + ("call_graph", {"return_value": {"nodes": {}, "edges": []}}), + ): + patch = mock.patch.object(closed_features, name, **kwargs) + patch.start() + self.addCleanup(patch.stop) + + def run_service(self, respond=None): + service = _Server( + respond + or ( + lambda p, b: ( + 200, + ( + {"guide": GUIDE, "usage": {}, "timingsMs": {}} + if p == "/v1/review-guide" + else {"envelope": ENVELOPE, "usage": {}} + ), + ) + ) + ) + self.addCleanup(service.close) + self.env["CLOSED_FUNCTIONS_URL"] = service.url + "/" + with mock.patch("builtins.print"): + self.assertEqual(closed_features.main(self.env), 0) + return service + + def test_exactly_two_posts_shared_inputs_and_fixed_outputs(self): + service = self.run_service() + self.assertEqual(_read(self.env, "review_guide.json"), GUIDE) + self.assertEqual(_read(self.env, "closed/architecture.json"), ENVELOPE) + self.assertEqual( + sorted(p.name for p in (Path(self.env["ARTIFACT_DIR"]) / "closed").iterdir()), ["architecture.json"] + ) + self.assertEqual(sorted(r["path"] for r in service.requests), ["/v1/architecture", "/v1/review-guide"]) + bodies = {} + for request in service.requests: + self.assertEqual(request["method"], "POST") + self.assertEqual(request["headers"]["Authorization"], "Bearer ghs_run") + bodies[request["path"]] = json.loads(gzip.decompress(request["body"])) + guide, arch = bodies["/v1/review-guide"], bodies["/v1/architecture"] + self.assertEqual(set(guide["files"]), INPUTS) + self.assertEqual(set(arch["files"]), INPUTS | {"source.json"}) + self.assertEqual({k: arch["files"][k] for k in INPUTS}, guide["files"]) + self.assertEqual((arch["version"], arch["repo"], arch["pr"]), (1, "o/r", 7)) + + def test_http_failures_isolated_in_both_directions(self): + self.run_service( + lambda p, b: (500, {"message": "broken"}) if p.endswith("architecture") else (200, {"guide": GUIDE}) + ) + self.assertEqual(_read(self.env, "review_guide.json"), GUIDE) + self.assertEqual(_read(self.env, "closed/architecture.json")["status"], "failed") + (Path(self.env["ARTIFACT_DIR"]) / "review_guide.json").unlink() + self.run_service(lambda p, b: (500, {}) if p.endswith("review-guide") else (200, {"envelope": ENVELOPE})) + self.assertEqual(_read(self.env, "closed/architecture.json"), ENVELOPE) + self.assertFalse((Path(self.env["ARTIFACT_DIR"]) / "review_guide.json").exists()) + + def test_malformed_responses_do_not_block_other_call(self): + self.run_service(lambda p, b: (200, []) if p.endswith("architecture") else (200, {"guide": GUIDE})) + self.assertEqual(_read(self.env, "review_guide.json"), GUIDE) + self.assertEqual(_read(self.env, "closed/architecture.json")["status"], "failed") + + def test_quota_is_skipped(self): + self.run_service(lambda p, b: (402, {"message": "token limit"})) + self.assertEqual(_read(self.env, "closed/architecture.json")["status"], "skipped") + + def test_all_architecture_statuses_preserved(self): + for status in ("ok", "partial", "skipped", "failed"): + envelope = {**ENVELOPE, "status": status} + self.run_service(lambda p, b: (200, {"guide": GUIDE, "envelope": envelope})) + self.assertEqual(_read(self.env, "closed/architecture.json"), envelope) + + def test_unsupported_provider_still_gets_measured_architecture(self): + self.env.update(LLM="litellm", LITELLM_API_KEY="secret") + service = self.run_service() + self.assertEqual([r["path"] for r in service.requests], ["/v1/architecture"]) + headers = {k.lower(): v for k, v in service.requests[0]["headers"].items()} + self.assertEqual(headers["x-llm-provider"], "litellm") + self.assertNotIn("x-llm-api-key", headers) + self.assertFalse((Path(self.env["ARTIFACT_DIR"]) / "review_guide.json").exists()) + + def test_missing_shared_input_records_failure_without_requests(self): + (Path(self.env["HEAD_ANALYSIS_PATH"]).parent / "static_analysis.pkl").unlink() + self.assertEqual(self.run_service().requests, []) + self.assertEqual(_read(self.env, "closed/architecture.json")["status"], "failed") + + def test_both_requests_can_run_concurrently(self): + barrier = threading.Barrier(2) + + def respond(path, body): + barrier.wait(timeout=5) + return 200, {"guide": GUIDE, "envelope": ENVELOPE} + + self.run_service(respond) + self.assertEqual(_read(self.env, "review_guide.json"), GUIDE) + self.assertEqual(_read(self.env, "closed/architecture.json"), ENVELOPE) + + def test_empty_url_disables_both(self): + self.env["CLOSED_FUNCTIONS_URL"] = "" + with mock.patch.object(closed_features, "_post") as post: + self.assertEqual(closed_features.main(self.env), 0) + post.assert_not_called() + self.assertFalse((Path(self.env["ARTIFACT_DIR"]) / "closed").exists()) + + def test_source_toggle_and_read_failure_only_omit_architecture_source(self): + for failure in (False, True): + self.env["CLOSED_FEATURES_SOURCE"] = "false" + with ( + mock.patch.object(closed_features, "collect_source", side_effect=OSError()) + if failure + else nullcontext() + ): + service = self.run_service() + for request in service.requests: + body = json.loads(gzip.decompress(request["body"])) + self.assertNotIn("source.json", body["files"]) + self.assertEqual("omitted" in body, request["path"] == "/v1/architecture") + + def test_bundle_limit_omits_source_then_skips_if_required_inputs_too_large(self): + with ( + mock.patch.object(closed_features, "MAX_BUNDLE_BYTES", 1000), + mock.patch.object(closed_features, "collect_source", return_value=(os.urandom(2000).hex(), None)), + ): + service = self.run_service() + arch = next(r for r in service.requests if r["path"] == "/v1/architecture") + body = json.loads(gzip.decompress(arch["body"])) + self.assertNotIn("source.json", body["files"]) + self.assertIn("too large", body["omitted"]["source.json"]) + with mock.patch.object(closed_features, "MAX_BUNDLE_BYTES", 1): + self.assertEqual(self.run_service().requests, []) + self.assertEqual(_read(self.env, "closed/architecture.json")["status"], "skipped") + + +class SourceTests(unittest.TestCase): + def test_tracked_extensions_exclusions_limits_and_symlinks(self): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + subprocess.run(["git", "init", "-q", tmp], check=True) + names = [ + "a.py", + "a.ts", + "a.tsx", + "a.js", + "a.jsx", + "a.mjs", + "a.cjs", + "a.cs", + "a.csproj", + "b.txt", + "docs/d.py", + "node_modules/n.js", + "a.min.js", + "a.d.ts", + "binary.py", + "big.py", + ] + for name in names: + path = root / name + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text("x") + (root / "binary.py").write_bytes(b"\xff") + (root / "big.py").write_text("x" * 300_001) + (root / "link.py").symlink_to(root / "a.py") + subprocess.run(["git", "-C", tmp, "add", "."], check=True) + (root / "untracked.py").write_text("not tracked") + text, why = closed_features.collect_source({"CHECKOUT_DIR": tmp}) + self.assertEqual(set(json.loads(text)), set(names[:9])) + self.assertIsNone(why) + with mock.patch.object(closed_features, "MAX_SOURCE_BYTES", 1): + text, why = closed_features.collect_source({"CHECKOUT_DIR": tmp}) + self.assertIsNone(text) + self.assertIn("limit", why) + + def test_symlink_parent_and_traversal_rejected(self): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + (root / "link").symlink_to(root, target_is_directory=True) + for name in ("link/a.py", "../a.py", "/tmp/a.py"): + with self.assertRaises(OSError): + closed_features._checkout_file(root, name) + + def test_disabled_source_does_not_read_checkout(self): + self.assertEqual( + closed_features.collect_source({"CLOSED_FEATURES_SOURCE": "false"}), (None, "turned off by the workflow") + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_llm_contract.py b/tests/test_llm_contract.py index b96f148..621153e 100644 --- a/tests/test_llm_contract.py +++ b/tests/test_llm_contract.py @@ -164,6 +164,20 @@ def test_hosted_refuses_a_licence_it_would_not_spend(self) -> None: self.assertEqual(error.code, "hosted_with_license") self.assertIn("llm: license", error.message) + def test_hosted_and_license_refuse_a_model_of_your_choosing(self) -> None: + """Choosing a model takes your own key. Each of the three inputs names one.""" + for name in ("CB_IN_MODEL", "CB_IN_AGENT_MODEL", "CB_IN_PARSING_MODEL"): + with self.subTest(input=name): + error = self.refuse(CB_IN_LLM="hosted", **OIDC, **{name: "gpt-x"}) + self.assertEqual(error.code, "hosted_with_model") + self.assertIn(name.removeprefix("CB_IN_").lower(), error.message) + error = self.refuse(CB_IN_LLM="license", CB_IN_LICENSE_KEY="lic", **OIDC, **{name: "gpt-x"}) + self.assertEqual(error.code, "license_with_model") + + def test_a_provider_of_your_own_can_run_any_model(self) -> None: + plan = self.resolve(CB_IN_LLM="anthropic", CB_IN_ANTHROPIC_API_KEY="k", CB_IN_MODEL="any-model") + self.assertEqual(plan["provider"], "anthropic") + def test_license_without_a_licence_key_is_refused(self) -> None: error = self.refuse(CB_IN_LLM="license", **OIDC) self.assertEqual(error.code, "missing_license_key") @@ -342,6 +356,8 @@ def test_every_declared_refusal_is_exercised_by_this_file(self) -> None: {"CB_IN_LLM": "hosted", "CB_IN_ANTHROPIC_API_KEY": "k", **OIDC}, {"CB_IN_LLM": "hosted", "CB_IN_LICENSE_KEY": "lic", **OIDC}, {"CB_IN_LLM": "license", "CB_IN_LICENSE_KEY": "lic", "CB_IN_ANTHROPIC_API_KEY": "k", **OIDC}, + {"CB_IN_LLM": "hosted", "CB_IN_MODEL": "x", **OIDC}, + {"CB_IN_LLM": "license", "CB_IN_LICENSE_KEY": "lic", "CB_IN_AGENT_MODEL": "x", **OIDC}, {"CB_IN_LLM": "anthropic", "CB_IN_ANTHROPIC_API_KEY": "k", "CB_IN_OPENAI_API_KEY": "o"}, ] seen = {self.refuse(**case).code for case in cases} diff --git a/tests/test_review_guide.py b/tests/test_review_guide.py deleted file mode 100644 index 1c0507c..0000000 --- a/tests/test_review_guide.py +++ /dev/null @@ -1,220 +0,0 @@ -"""Tests for the review-guide step: the bundle it sends, the credentials, and that it never fails a run.""" - -from __future__ import annotations - -import gzip -import importlib.util -import json -import sys -import tempfile -import threading -import unittest -from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer -from pathlib import Path -from unittest import mock - -_SCRIPT = Path(__file__).resolve().parent.parent / "scripts" / "review_guide.py" -_SPEC = importlib.util.spec_from_file_location("review_guide", _SCRIPT) -assert _SPEC and _SPEC.loader -review_guide = importlib.util.module_from_spec(_SPEC) -sys.modules[_SPEC.name] = review_guide -_SPEC.loader.exec_module(review_guide) - - -class _Server: - """A local HTTP server answering every request with one handler.""" - - def __init__(self, respond): - outer = self - self.requests: list[dict] = [] - - class Handler(BaseHTTPRequestHandler): - def log_message(self, *_a): - pass - - def _any(self): - body = self.rfile.read(int(self.headers.get("Content-Length") or 0)) - outer.requests.append({"path": self.path, "headers": dict(self.headers), "body": body}) - status, payload = respond(self.path, body) - data = json.dumps(payload).encode() - self.send_response(status) - self.send_header("Content-Type", "application/json") - self.send_header("Content-Length", str(len(data))) - self.end_headers() - self.wfile.write(data) - - do_GET = do_POST = _any - - self.server = ThreadingHTTPServer(("127.0.0.1", 0), Handler) - threading.Thread(target=self.server.serve_forever, daemon=True).start() - - @property - def url(self) -> str: - return f"http://127.0.0.1:{self.server.server_port}" - - def close(self) -> None: - self.server.shutdown() - - -def _workspace(tmp: Path) -> dict: - """A run's state as the step finds it: the artifact folder, both analyses, a head checkout.""" - artifact, head, base, checkout = (tmp / n for n in ("artifact", "head", "base", "checkout")) - for d in (artifact, head, base, checkout): - d.mkdir() - (artifact / "metadata.json").write_text( - json.dumps({"kind": "review", "head_sha": "H", "base_sha": "TIP", "merge_base_sha": "B", "pr_number": "7"}) - ) - for d in (head, base): - (d / "analysis.json").write_text(json.dumps({"components": [], "methods_index": {}})) - (d / "static_analysis.pkl").write_bytes(b"") - (checkout / "app.py").write_text("def f():\n return 2\n") - return { - "REVIEW_GUIDE_URL": "http://unused", - "REPOSITORY": "o/r", - "PR_NUMBER": "7", - "ARTIFACT_DIR": str(artifact), - "HEAD_ANALYSIS_PATH": str(head / "analysis.json"), - "BASE_ANALYSIS_PATH": str(base / "analysis.json"), - "CHECKOUT_DIR": str(checkout), - "GITHUB_TOKEN": "ghs_run", - "RUNNER_TEMP": str(tmp), - } - - -_COMPARE = { - "files": [ - {"filename": "app.py", "status": "modified", "additions": 1, "deletions": 1, "patch": "@@ -1,2 +1,2 @@"}, - {"filename": "new.py", "status": "added", "additions": 1, "deletions": 0, "patch": "@@ -0,0 +1 @@"}, - ] -} - - -def _github_json(path: str): - if path.startswith("/repos/o/r/compare/"): - return _COMPARE - if path.endswith("/commits?per_page=100"): - return [{"commit": {"message": "Return two\n\nbody"}}] - return {"title": "Two", "body": "Returns two"} - - -class BundleTests(unittest.TestCase): - def test_the_bundle_reads_the_analysed_commits_and_both_sides_of_each_file(self): - with tempfile.TemporaryDirectory() as t: - env = _workspace(Path(t)) - with ( - mock.patch.object(review_guide, "_github_json", side_effect=_github_json) as gh, - mock.patch.object(review_guide, "_contents", return_value="def f():\n return 1\n") as base, - mock.patch.object(review_guide, "call_graph", return_value={"nodes": {}, "edges": []}), - ): - b = review_guide.bundle(env) - # the diff is between the two analysed commits, the merge base not the branch tip - self.assertIn("/repos/o/r/compare/B...H", [c.args[0] for c in gh.call_args_list]) - files = json.loads(b["files"]["files.json"]) - self.assertEqual(files["H:app.py"], "def f():\n return 2\n") - self.assertEqual(files["B:app.py"], "def f():\n return 1\n") - self.assertNotIn("B:new.py", files) # an added file has no base side to fetch - base.assert_called_once_with("o/r", "B", "app.py") - meta = json.loads(b["files"]["metadata.json"]) - self.assertEqual((meta["base_sha"], meta["head_sha"]), ("B", "H")) - self.assertEqual(json.loads(b["files"]["pr.json"])["commits"], ["Return two"]) - self.assertEqual(sorted(b["files"]), sorted(review_guide_inputs())) - - def test_a_missing_call_graph_skips_the_guide_without_failing(self): - with tempfile.TemporaryDirectory() as t: - env = _workspace(Path(t)) - (Path(env["HEAD_ANALYSIS_PATH"]).parent / "static_analysis.pkl").unlink() - with mock.patch.object(review_guide, "_github_json", side_effect=_github_json): - with mock.patch("builtins.print") as out: - self.assertEqual(review_guide.main(env), 0) - self.assertIn("No review guide", out.call_args.args[0]) - self.assertFalse((Path(env["ARTIFACT_DIR"]) / "review_guide.json").exists()) - - -def review_guide_inputs(): - return [ - "analysis.json", - "base/analysis.json", - "metadata.json", - "pr.json", - "pr-files.json", - "head-cg.json", - "base-cg.json", - "files.json", - ] - - -class CredentialTests(unittest.TestCase): - def test_without_oidc_the_runs_own_token_proves_the_repository(self): - self.assertEqual(review_guide.credentials({"GITHUB_TOKEN": "ghs_run"}), {"Authorization": "Bearer ghs_run"}) - - def test_with_oidc_the_bearer_is_the_proxys_own_with_the_license(self): - mint = _Server(lambda path, body: (200, {"value": "jwt.token.sig"})) - try: - env = { - "ACTIONS_ID_TOKEN_REQUEST_URL": f"{mint.url}/token?x=1", - "ACTIONS_ID_TOKEN_REQUEST_TOKEN": "req", - "LICENSE_KEY": "LIC", - "GITHUB_TOKEN": "ghs_run", - } - with tempfile.TemporaryDirectory() as t: - env["RUNNER_TEMP"] = t - headers = review_guide.credentials(env) - self.assertEqual(list(Path(t).iterdir()), []) # the license file does not outlive the call - self.assertEqual(headers, {"Authorization": "Bearer jwt.token.sig~codeboarding-license~LIC"}) - self.assertIn("audience=codeboarding-proxy", mint.requests[0]["path"]) - finally: - mint.close() - - def test_the_runs_own_provider_key_goes_along_and_the_caller_is_still_proved(self): - headers = review_guide.credentials({"GITHUB_TOKEN": "ghs_run", "OPENROUTER_API_KEY": " sk-or "}) - self.assertEqual( - headers, - {"Authorization": "Bearer ghs_run", "x-llm-api-key": "sk-or", "x-llm-provider": "openrouter"}, - ) - - -class RequestTests(unittest.TestCase): - def test_the_guide_lands_in_the_artifact_beside_the_analysis(self): - guide = {"version": 1, "repo": "o/r", "pr": 7, "themes": [], "changes": [{"id": "u1"}]} - service = _Server(lambda path, body: (200, {"guide": guide, "usage": {"provider": "ollama", "hosted": True}})) - try: - with tempfile.TemporaryDirectory() as t: - env = {**_workspace(Path(t)), "REVIEW_GUIDE_URL": service.url} - with ( - mock.patch.object(review_guide, "_github_json", side_effect=_github_json), - mock.patch.object(review_guide, "_contents", return_value="x"), - mock.patch.object(review_guide, "call_graph", return_value={"nodes": {}, "edges": []}), - ): - self.assertEqual(review_guide.main(env), 0) - written = json.loads((Path(env["ARTIFACT_DIR"]) / "review_guide.json").read_text()) - self.assertEqual(written, guide) - sent = service.requests[0] - self.assertEqual(sent["path"], "/v1/review-guide") - self.assertEqual(sent["headers"]["Authorization"], "Bearer ghs_run") - self.assertEqual(json.loads(gzip.decompress(sent["body"]))["repo"], "o/r") - finally: - service.close() - - def test_a_refusal_is_a_notice_and_the_run_goes_on(self): - service = _Server( - lambda path, body: (402, {"error": "quota", "message": "Resource exhausted: token limit reached"}) - ) - try: - with tempfile.TemporaryDirectory() as t: - env = {**_workspace(Path(t)), "REVIEW_GUIDE_URL": service.url} - with ( - mock.patch.object(review_guide, "_github_json", side_effect=_github_json), - mock.patch.object(review_guide, "_contents", return_value="x"), - mock.patch.object(review_guide, "call_graph", return_value={"nodes": {}, "edges": []}), - mock.patch("builtins.print") as out, - ): - self.assertEqual(review_guide.main(env), 0) - self.assertFalse((Path(env["ARTIFACT_DIR"]) / "review_guide.json").exists()) - self.assertIn("402", out.call_args.args[0]) - self.assertIn("token limit", out.call_args.args[0]) - finally: - service.close() - - -if __name__ == "__main__": - unittest.main()