From 1144d2093d281dfab7f44893a01591a39afa6ddd Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Sun, 1 Nov 2026 01:04:44 +0100 Subject: [PATCH 1/6] feat: run every closed feature the service lists The review guide step becomes a generic runner: it reads GET /v1/features, collects the bundle once, posts one request per feature in parallel with only the inputs that feature names, and writes each answer to the artifact. A feature that never answered is written as skipped or failed, so a reader can tell not checked from nothing found. It computes nothing and names no feature, except the fallback to the guide alone for a service with no manifest. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- action.yml | 20 +- scripts/closed_features.py | 386 ++++++++++++++++++++++++++++++++++ scripts/review_guide.py | 252 ---------------------- tests/test_closed_features.py | 324 ++++++++++++++++++++++++++++ tests/test_review_guide.py | 220 ------------------- 6 files changed, 721 insertions(+), 483 deletions(-) create mode 100644 scripts/closed_features.py delete mode 100644 scripts/review_guide.py create mode 100644 tests/test_closed_features.py delete mode 100644 tests/test_review_guide.py diff --git a/README.md b/README.md index 4080460..1e9d9ae 100644 --- a/README.md +++ b/README.md @@ -347,7 +347,7 @@ 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. Production service base URL for the closed features, such as the pull request's review guide. The run sends it this analysis and stores one file per feature in the review artifact (`closed/.json`, plus `review_guide.json`). Set to `''` to disable. Never fails the run. | `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..3a19a18 100644 --- a/action.yml +++ b/action.yml @@ -145,10 +145,10 @@ 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. The CodeBoarding service behind the closed features, such as 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 each feature's + answer in the review artifact for the platform to show. Set to an empty string to disable. required: false default: 'https://jxivqwx4or6oip3krgdmdaiunm0iaybf.lambda-url.us-east-1.on.aws/' github_token: @@ -694,15 +694,15 @@ 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 }} REPOSITORY: ${{ github.repository }} PR_NUMBER: ${{ steps.guard.outputs.pr_number }} ARTIFACT_DIR: ${{ steps.review_artifact.outputs.artifact_dir }} @@ -714,7 +714,7 @@ runs: OLLAMA_API_KEY: ${{ inputs.ollama_api_key }} OPENROUTER_API_KEY: ${{ inputs.openrouter_api_key }} OPENAI_API_KEY: ${{ inputs.openai_api_key }} - run: python3 "$GITHUB_ACTION_PATH/scripts/review_guide.py" + run: python3 "$GITHUB_ACTION_PATH/scripts/closed_features.py" - name: Upload review artifact id: upload_review_artifact_dotcom diff --git a/scripts/closed_features.py b/scripts/closed_features.py new file mode 100644 index 0000000..22f625e --- /dev/null +++ b/scripts/closed_features.py @@ -0,0 +1,386 @@ +#!/usr/bin/env python3 +"""Run CodeBoarding's closed features on this pull request and keep what each one answers in the +review artifact. + +Runs after the review analysis. It asks the service which features exist (``GET /v1/features``), +packs what the run already has once (both analyses, both call graphs from the ``static_analysis.pkl`` +beside each ``analysis.json``, the head checkout, the diff between the two analysed commits), and +posts one request per feature in parallel, each with the inputs that feature names. Every feature +gets a file in the artifact whatever happens to it: its answer, or an envelope saying it was skipped +or failed, so a reader can tell "not checked" from "nothing found". + +Nothing here knows what a feature does. A feature's code is CodeBoarding's and stays on its side, and +a new one needs no release of this Action unless it asks for an input this script does not collect. +The one exception is ``FALLBACK``: a service that cannot name its features still gets asked for the +review guide, because that is what every service since this Action's first release can answer. + +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. 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, +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 concurrent.futures import ThreadPoolExecutor +from pathlib import Path +from urllib.error import HTTPError, URLError +from urllib.parse import quote, urljoin +from urllib.request import Request, urlopen + +# A Function URL takes at most 6 MB; each feature's bundle is gzipped JSON, sent as is. +MAX_BUNDLE_BYTES = 5_800_000 +# Changed files whose contents are sent at both commits; past this a feature reads diffs only. +MAX_FILES = 300 +OWN_KEYS = (("OLLAMA_API_KEY", "ollama"), ("OPENROUTER_API_KEY", "openrouter"), ("OPENAI_API_KEY", "openai")) +# What the review guide has always been: asked for when the service lists no features. +FALLBACK = { + "name": "review-guide", + "schema": 1, + "url": "/v1/review-guide", + "needs": [ + "analysis.json", + "base/analysis.json", + "metadata.json", + "pr.json", + "pr-files.json", + "head-cg.json", + "base-cg.json", + "files.json", + ], + "output": "closed/review-guide.json", + "dataOutput": "review_guide.json", + "enabled": True, +} + + +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: + """Everything a feature can ask for, 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 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": + 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 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 read_manifest(url: str) -> list[dict]: + """What the service runs, or the guide alone when it cannot say. A feature that cannot be read + is left out rather than guessed at; a service that names none is treated as one that failed.""" + try: + with urlopen( # nosec B310 - the service URL the workflow names + Request(f"{url.rstrip('/')}/v1/features", headers={"User-Agent": "codeboarding-action"}), timeout=15 + ) as res: + features = json.loads(res.read()).get("features") + except (OSError, ValueError, AttributeError) as e: + print(f"::notice::The service listed no features ({type(e).__name__}); asking for the review guide alone.") + return [FALLBACK] + usable = [ + f + for f in features or [] + if isinstance(f, dict) and isinstance(f.get("name"), str) and isinstance(f.get("output"), str) + ] + return usable or [FALLBACK] + + +def _in_artifact(artifact: Path, relative: str) -> Path: + """A path the service named, held inside the artifact.""" + path = (artifact / relative).resolve() + if not path.is_relative_to(artifact.resolve()): + raise Skip(f"the service named a path outside the artifact: {relative}") + return path + + +def _write(artifact: Path, feature: dict, env_out: dict) -> None: + path = _in_artifact(artifact, feature["output"]) + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps(env_out, ensure_ascii=False, indent=1)) + # A reader from before the envelope takes the bare data, and only a good result. + plain = feature.get("dataOutput") + if plain and env_out.get("status") in ("ok", "partial") and env_out.get("data") is not None: + _in_artifact(artifact, plain).write_text(json.dumps(env_out["data"], ensure_ascii=False, indent=1)) + + +def _no_result(feature: dict, ctx: dict, status: str, reason: str) -> dict: + """The envelope of a feature that never answered.""" + return { + "feature": feature["name"], + "schema": feature.get("schema"), + "status": status, + "repo": ctx["repo"], + "pr": ctx["pr"], + "headSha": ctx["head"], + "generator": None, + "coverage": None, + "data": None, + "reason": reason, + } + + +def _answered(feature: dict, ctx: dict, answer: dict) -> dict: + env_out = answer.get("envelope") + if isinstance(env_out, dict) and env_out.get("status"): + return env_out + # A service from before the manifest answers the bare guide. + usage = answer.get("usage") or {} + if isinstance(answer.get("guide"), dict): + return { + **_no_result(feature, ctx, "ok", ""), + "generator": {"model": usage.get("model"), "hosted": bool(usage.get("hosted")), "version": None}, + "data": answer["guide"], + } + return _no_result(feature, ctx, "failed", "the service returned no result") + + +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", + ) + with urlopen(req, timeout=600) as res: # nosec B310 - a URL the workflow's service named + return json.loads(res.read()) + + +def run_feature(feature: dict, base: str, ctx: dict, inputs: dict[str, str], headers: dict[str, str]) -> dict: + """One feature, start to finish: never raises, always an envelope.""" + needs = feature.get("needs") or [] + missing = [n for n in needs if n not in inputs] + if missing: + return _no_result(feature, ctx, "skipped", f"this Action does not collect {', '.join(missing)}") + url = urljoin(f"{base.rstrip('/')}/", (feature.get("url") or "").lstrip("/")) + if not url.startswith((base.rstrip("/"), "https://")): + return _no_result(feature, ctx, "failed", "the service named an address that is not https") + data = gzip.compress( + json.dumps( + {"version": 1, "repo": ctx["repo"], "pr": ctx["pr"], "files": {n: inputs[n] for n in needs}} + ).encode(), + compresslevel=9, + ) + if len(data) > MAX_BUNDLE_BYTES: + return _no_result(feature, ctx, "skipped", f"its bundle is {len(data) / 1e6:.1f} MB, over the service's limit") + try: + answer = _post(url, data, headers) + except HTTPError as e: + try: + message = json.loads(e.read()).get("message") or e.reason + except ValueError: + message = e.reason + # 402 is the owner's weekly bucket: nothing went wrong, and nothing was checked. + return _no_result( + feature, ctx, "skipped" if e.code == 402 else "failed", f"the service answered {e.code}: {message}" + ) + except (OSError, ValueError) as e: + return _no_result(feature, ctx, "failed", f"the service did not answer: {type(e).__name__}") + out = _answered(feature, ctx, answer) + usage = answer.get("usage") or {} + print( + f"{feature['name']}: {out.get('status')}; {usage.get('provider')} {usage.get('model')}, " + f"{usage.get('promptTokens', 0) + usage.get('outputTokens', 0)} tokens " + f"({'hosted' if usage.get('hosted') else 'own key'}); bundle {len(data) / 1e6:.2f} MB" + ) + return out + + +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"] + features = read_manifest(base) + results: dict[str, dict] = { + f["name"]: _no_result(f, ctx, "skipped", "turned off by the service") + for f in features + if not f.get("enabled") or not f.get("url") + } + enabled = [f for f in features if f["name"] not in results] + if enabled: + try: + inputs = collect(env) + headers = credentials(env) + except Exception as e: # every feature reads this, so every feature is told why + reason = str(e) if isinstance(e, Skip) else f"collecting its inputs failed: {type(e).__name__}: {e}" + results.update({f["name"]: _no_result(f, ctx, "failed", reason) for f in enabled}) + enabled = [] + with ThreadPoolExecutor(max_workers=max(1, len(enabled))) as pool: + done = pool.map(lambda f: run_feature(f, base, ctx, inputs, headers), enabled) + results.update({f["name"]: r for f, r in zip(enabled, done)}) + for f in features: + out = results[f["name"]] + _write(artifact, f, out) + if out["status"] not in ("ok", "partial"): + print(f"::notice::{f['name']} {out['status']}: {out.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..a2d8bcc --- /dev/null +++ b/tests/test_closed_features.py @@ -0,0 +1,324 @@ +"""Tests for the closed-features step: the bundle it sends, the credentials, one result per feature, 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" / "closed_features.py" +_SPEC = importlib.util.spec_from_file_location("closed_features", _SCRIPT) +assert _SPEC and _SPEC.loader +closed_features = importlib.util.module_from_spec(_SPEC) +sys.modules[_SPEC.name] = closed_features +_SPEC.loader.exec_module(closed_features) + + +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 { + "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), + } + + +_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(closed_features, "_github_json", side_effect=_github_json) as gh, + mock.patch.object(closed_features, "_contents", return_value="def f():\n return 1\n") as base, + mock.patch.object(closed_features, "call_graph", return_value={"nodes": {}, "edges": []}), + ): + b = closed_features.collect(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.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["metadata.json"]) + self.assertEqual((meta["base_sha"], meta["head_sha"]), ("B", "H")) + self.assertEqual(json.loads(b["pr.json"])["commits"], ["Return two"]) + self.assertEqual(sorted(b), sorted(closed_features.FALLBACK["needs"])) + + +class CredentialTests(unittest.TestCase): + def test_without_oidc_the_runs_own_token_proves_the_repository(self): + self.assertEqual(closed_features.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 = closed_features.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 = closed_features.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"}, + ) + + +GUIDE = { + "name": "review-guide", + "schema": 1, + "url": "/v1/features/review-guide", + "needs": ["pr.json"], + "output": "closed/review-guide.json", + "dataOutput": "review_guide.json", + "enabled": True, +} +OTHER = { + "name": "other", + "schema": 2, + "url": "/v1/features/other", + "needs": ["analysis.json", "pr-files.json"], + "output": "closed/other.json", + "dataOutput": None, + "enabled": True, +} + + +def _envelope(name, data, status="ok"): + return { + "feature": name, + "schema": 1, + "status": status, + "repo": "o/r", + "pr": 7, + "headSha": "H", + "generator": {"model": "m", "hosted": True, "version": "v"}, + "coverage": {"languages": ["Python"], "files": {"seen": 2, "total": 2}}, + "data": data, + } + + +def _run(env): + with ( + mock.patch.object(closed_features, "_github_json", side_effect=_github_json), + mock.patch.object(closed_features, "_contents", return_value="x"), + mock.patch.object(closed_features, "call_graph", return_value={"nodes": {}, "edges": []}), + mock.patch("builtins.print") as out, + ): + code = closed_features.main(env) + return code, [" ".join(map(str, c.args)) for c in out.call_args_list] + + +def _read(env, name): + return json.loads((Path(env["ARTIFACT_DIR"]) / name).read_text()) + + +class RunnerTests(unittest.TestCase): + def _serve(self, features, answer): + def respond(path, body): + if path == "/v1/features": + return features if isinstance(features, tuple) else (200, {"version": 1, "features": features}) + return answer(path, body) + + service = _Server(respond) + self.addCleanup(service.close) + return service + + def test_each_feature_gets_its_own_request_with_only_the_inputs_it_names(self): + guide = {"version": 1, "changes": [{"id": "u1"}]} + + def answer(path, body): + name = path.rsplit("/", 1)[-1] + data = guide if name == "review-guide" else {"findings": []} + return 200, {"envelope": _envelope(name, data), "usage": {"model": "m"}} + + service = self._serve([GUIDE, OTHER], answer) + with tempfile.TemporaryDirectory() as t: + env = {**_workspace(Path(t)), "CLOSED_FUNCTIONS_URL": service.url} + self.assertEqual(_run(env)[0], 0) + self.assertEqual(_read(env, "review_guide.json"), guide) + self.assertEqual(_read(env, "closed/review-guide.json")["data"], guide) + self.assertEqual(_read(env, "closed/other.json")["status"], "ok") + self.assertFalse((Path(env["ARTIFACT_DIR"]) / "other.json").exists()) + posts = {r["path"]: json.loads(gzip.decompress(r["body"])) for r in service.requests if r["body"]} + self.assertEqual(sorted(posts["/v1/features/review-guide"]["files"]), ["pr.json"]) + self.assertEqual(sorted(posts["/v1/features/other"]["files"]), ["analysis.json", "pr-files.json"]) + self.assertEqual(posts["/v1/features/other"]["pr"], 7) + + def test_a_feature_that_fails_is_written_as_failed_and_costs_its_neighbour_nothing(self): + guide = {"version": 1, "changes": []} + + def answer(path, body): + if path.endswith("/other"): + return 500, {"error": "failed", "message": "KeyError: x"} + return 200, {"envelope": _envelope("review-guide", guide), "usage": {}} + + service = self._serve([GUIDE, OTHER], answer) + with tempfile.TemporaryDirectory() as t: + env = {**_workspace(Path(t)), "CLOSED_FUNCTIONS_URL": service.url} + code, printed = _run(env) + self.assertEqual(code, 0) + self.assertEqual(_read(env, "review_guide.json"), guide) + failed = _read(env, "closed/other.json") + self.assertEqual((failed["status"], failed["data"], failed["headSha"]), ("failed", None, "H")) + self.assertIn("500", failed["reason"]) + self.assertTrue(any("other failed" in line for line in printed)) + + def test_a_spent_bucket_is_skipped_not_failed(self): + service = self._serve( + [OTHER], lambda p, b: (402, {"error": "quota", "message": "Resource exhausted: token limit reached"}) + ) + with tempfile.TemporaryDirectory() as t: + env = {**_workspace(Path(t)), "CLOSED_FUNCTIONS_URL": service.url} + _run(env) + out = _read(env, "closed/other.json") + self.assertEqual(out["status"], "skipped") + self.assertIn("token limit", out["reason"]) + + def test_a_turned_off_feature_is_skipped_and_never_requested(self): + service = self._serve([{**OTHER, "enabled": False}], lambda p, b: (500, {})) + with tempfile.TemporaryDirectory() as t: + env = {**_workspace(Path(t)), "CLOSED_FUNCTIONS_URL": service.url} + _run(env) + self.assertEqual(_read(env, "closed/other.json")["status"], "skipped") + self.assertEqual([r["path"] for r in service.requests], ["/v1/features"]) + + def test_a_feature_needing_what_this_action_does_not_collect_is_skipped(self): + wants = {**OTHER, "needs": ["whole-repo.tar"]} + service = self._serve([wants], lambda p, b: (500, {})) + with tempfile.TemporaryDirectory() as t: + env = {**_workspace(Path(t)), "CLOSED_FUNCTIONS_URL": service.url} + _run(env) + out = _read(env, "closed/other.json") + self.assertEqual(out["status"], "skipped") + self.assertIn("whole-repo.tar", out["reason"]) + + def test_a_service_with_no_manifest_still_gets_asked_for_the_guide_alone(self): + guide = {"version": 1, "repo": "o/r", "pr": 7, "themes": [], "changes": [{"id": "u1"}]} + usage = {"provider": "ollama", "model": "m", "hosted": True} + service = self._serve((404, {"error": "not_found"}), lambda p, b: (200, {"guide": guide, "usage": usage})) + with tempfile.TemporaryDirectory() as t: + env = {**_workspace(Path(t)), "CLOSED_FUNCTIONS_URL": service.url} + _run(env) + written = (Path(env["ARTIFACT_DIR"]) / "review_guide.json").read_text() + wrapped = _read(env, "closed/review-guide.json") + # byte for byte what the step has always written + self.assertEqual(written, json.dumps(guide, ensure_ascii=False, indent=1)) + self.assertEqual((wrapped["status"], wrapped["data"]), ("ok", guide)) + sent = service.requests[-1] + self.assertEqual(sent["path"], "/v1/review-guide") + self.assertEqual(sent["headers"]["Authorization"], "Bearer ghs_run") + self.assertEqual(sorted(json.loads(gzip.decompress(sent["body"]))["files"]), sorted(GUIDE_INPUTS)) + + def test_a_missing_call_graph_is_recorded_for_every_feature_and_never_fails_the_run(self): + service = self._serve([GUIDE, OTHER], lambda p, b: (500, {})) + with tempfile.TemporaryDirectory() as t: + env = {**_workspace(Path(t)), "CLOSED_FUNCTIONS_URL": service.url} + (Path(env["HEAD_ANALYSIS_PATH"]).parent / "static_analysis.pkl").unlink() + self.assertEqual(_run(env)[0], 0) + self.assertFalse((Path(env["ARTIFACT_DIR"]) / "review_guide.json").exists()) + for name in ("closed/review-guide.json", "closed/other.json"): + self.assertEqual(_read(env, name)["status"], "failed") + + def test_a_path_outside_the_artifact_is_refused(self): + escape = {**OTHER, "output": "../outside.json"} + answer = lambda p, b: (200, {"envelope": _envelope("other", {}), "usage": {}}) # noqa: E731 + service = self._serve([escape], answer) + with tempfile.TemporaryDirectory() as t: + env = {**_workspace(Path(t)), "CLOSED_FUNCTIONS_URL": service.url} + self.assertEqual(_run(env)[0], 0) + self.assertFalse((Path(t) / "outside.json").exists()) + + +GUIDE_INPUTS = closed_features.FALLBACK["needs"] + + +if __name__ == "__main__": + unittest.main() 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() From 445bcefed745d9d0cf770a358be92d0e4b573ab3 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Sun, 1 Nov 2026 01:21:35 +0100 Subject: [PATCH 2/6] feat: send a repository's source to the feature that asks for it Only that feature's request carries it, within the size the service names. Past that, or with closed_features_source: false, the request goes without it and says why, so the feature reports what it could not check. Also lowers the bundle limit to what a Function URL really takes: base64 counts, so 5 MB was refused with a 413. Co-Authored-By: Claude Opus 5.5 --- README.md | 1 + action.yml | 8 ++++ scripts/closed_features.py | 88 ++++++++++++++++++++++++++++++----- tests/test_closed_features.py | 72 ++++++++++++++++++++++++++++ 4 files changed, 158 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 1e9d9ae..21b1b45 100644 --- a/README.md +++ b/README.md @@ -348,6 +348,7 @@ The first sync after the merge creates `codeboarding/baseline`, catching up from | `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 closed features, such as the pull request's review guide. The run sends it this analysis and stores one file per feature in the review artifact (`closed/.json`, plus `review_guide.json`). Set to `''` to disable. Never fails the run. | +| `closed_features_source` | review | `true` | Whether a closed feature that asks for it may read this repository's source, for checks that need the whole tree. Only that feature's request carries it, capped in size, and the service keeps nothing. With `false` those checks 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 3a19a18..94a0246 100644 --- a/action.yml +++ b/action.yml @@ -151,6 +151,13 @@ inputs: answer in the review artifact for the platform to show. Set to an empty string to disable. required: false default: 'https://jxivqwx4or6oip3krgdmdaiunm0iaybf.lambda-url.us-east-1.on.aws/' + closed_features_source: + description: >- + Whether a closed feature that asks for it may read this repository's source, for checks that + need the whole tree. Only that feature's request carries it, capped in size, and the service + keeps nothing. With 'false' those checks do not run and the result says so. + required: false + default: 'true' github_token: description: 'Token used for comments and sync delivery.' required: false @@ -703,6 +710,7 @@ runs: shell: bash env: 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 }} diff --git a/scripts/closed_features.py b/scripts/closed_features.py index 22f625e..b18984b 100644 --- a/scripts/closed_features.py +++ b/scripts/closed_features.py @@ -14,6 +14,11 @@ The one exception is ``FALLBACK``: a service that cannot name its features still gets asked for the review guide, because that is what every service since this Action's first release can answer. +A feature can ask for the repository's source. Only that feature's request carries it, and past the +size the service names, or with ``closed_features_source: false`` in the workflow, the request goes +without it and says why, so the feature reports what it could not check. This script reads the files +and sends them; it computes nothing from them. + 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 @@ -21,7 +26,7 @@ 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, -OLLAMA_API_KEY, OPENROUTER_API_KEY, OPENAI_API_KEY, ACTIONS_ID_TOKEN_REQUEST_URL/_TOKEN. +CLOSED_FEATURES_SOURCE, OLLAMA_API_KEY, OPENROUTER_API_KEY, OPENAI_API_KEY, ACTIONS_ID_TOKEN_REQUEST_URL/_TOKEN. """ from __future__ import annotations @@ -31,6 +36,8 @@ import json import os import pickle +import re +import subprocess import sys from concurrent.futures import ThreadPoolExecutor from pathlib import Path @@ -38,8 +45,9 @@ from urllib.parse import quote, urljoin from urllib.request import Request, urlopen -# A Function URL takes at most 6 MB; each feature's bundle is gzipped JSON, sent as is. -MAX_BUNDLE_BYTES = 5_800_000 +# 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 OWN_KEYS = (("OLLAMA_API_KEY", "ollama"), ("OPENROUTER_API_KEY", "openrouter"), ("OPENAI_API_KEY", "openai")) @@ -228,6 +236,33 @@ def _bearer(env: dict) -> dict[str, str]: license_file.unlink(missing_ok=True) +def collect_source(spec: dict, env: dict) -> tuple[str | None, str | None]: + """The tracked files a feature asked for, as one JSON document, or why there is none.""" + 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 + extensions = tuple(spec.get("extensions") or ()) + exclude = re.compile(spec["exclude"]) if spec.get("exclude") else None + limit, total = spec.get("maxBytes") or 0, 0 + files: dict[str, str] = {} + for name in listed.decode("utf-8", "replace").split("\0"): + if not name or not name.endswith(extensions) or (exclude and exclude.search(name)): + continue + path = checkout / name + try: + size = path.stat().st_size + if size > (spec.get("maxFileBytes") or size) or not path.is_file(): + continue + files[name] = path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + continue + total += size + if limit and total > limit: + return None, f"its source is over the {limit / 1e6:.0f} MB limit" + return json.dumps(files), None + + def read_manifest(url: str) -> list[dict]: """What the service runs, or the guide alone when it cannot say. A feature that cannot be read is left out rather than guessed at; a service that names none is treated as one that failed.""" @@ -307,7 +342,21 @@ def _post(url: str, body: bytes, headers: dict[str, str]) -> dict: return json.loads(res.read()) -def run_feature(feature: dict, base: str, ctx: dict, inputs: dict[str, str], headers: dict[str, str]) -> dict: +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 run_feature( + feature: dict, + base: str, + ctx: dict, + inputs: dict[str, str], + headers: dict[str, str], + source: tuple[str | None, str | None] | None = None, +) -> dict: """One feature, start to finish: never raises, always an envelope.""" needs = feature.get("needs") or [] missing = [n for n in needs if n not in inputs] @@ -316,12 +365,21 @@ def run_feature(feature: dict, base: str, ctx: dict, inputs: dict[str, str], hea url = urljoin(f"{base.rstrip('/')}/", (feature.get("url") or "").lstrip("/")) if not url.startswith((base.rstrip("/"), "https://")): return _no_result(feature, ctx, "failed", "the service named an address that is not https") - data = gzip.compress( - json.dumps( - {"version": 1, "repo": ctx["repo"], "pr": ctx["pr"], "files": {n: inputs[n] for n in needs}} - ).encode(), - compresslevel=9, - ) + files = {n: inputs[n] for n in needs} + spec = feature.get("source") or None + omitted: dict[str, str] = {} + if spec: + text, why = source if source else (None, "the source was not collected") + if text is None: + omitted[spec["input"]] = why or "the source was not collected" + else: + files[spec["input"]] = text + data = _pack(ctx, files, omitted) + if spec and spec["input"] in files and len(data) > MAX_BUNDLE_BYTES: + # The source is the one optional part: past the limit the feature runs without it. + del files[spec["input"]] + omitted[spec["input"]] = "the repository is too large to send" + data = _pack(ctx, files, omitted) if len(data) > MAX_BUNDLE_BYTES: return _no_result(feature, ctx, "skipped", f"its bundle is {len(data) / 1e6:.1f} MB, over the service's limit") try: @@ -347,6 +405,13 @@ def run_feature(feature: dict, base: str, ctx: dict, inputs: dict[str, str], hea return out +def _source_of(feature: dict, env: dict) -> tuple[str | None, str | None]: + try: + return collect_source(feature["source"], env) + except (OSError, ValueError, subprocess.SubprocessError) as e: + return None, f"reading it failed: {type(e).__name__}" + + def main(env: dict | None = None) -> int: env = dict(os.environ if env is None else env) try: @@ -369,8 +434,9 @@ def main(env: dict | None = None) -> int: reason = str(e) if isinstance(e, Skip) else f"collecting its inputs failed: {type(e).__name__}: {e}" results.update({f["name"]: _no_result(f, ctx, "failed", reason) for f in enabled}) enabled = [] + sources = {f["name"]: _source_of(f, env) for f in enabled if f.get("source")} with ThreadPoolExecutor(max_workers=max(1, len(enabled))) as pool: - done = pool.map(lambda f: run_feature(f, base, ctx, inputs, headers), enabled) + done = pool.map(lambda f: run_feature(f, base, ctx, inputs, headers, sources.get(f["name"])), enabled) results.update({f["name"]: r for f, r in zip(enabled, done)}) for f in features: out = results[f["name"]] diff --git a/tests/test_closed_features.py b/tests/test_closed_features.py index a2d8bcc..ceaa354 100644 --- a/tests/test_closed_features.py +++ b/tests/test_closed_features.py @@ -6,6 +6,7 @@ import gzip import importlib.util import json +import subprocess import sys import tempfile import threading @@ -317,6 +318,77 @@ def test_a_path_outside_the_artifact_is_refused(self): self.assertFalse((Path(t) / "outside.json").exists()) +class SourceTests(unittest.TestCase): + SPEC = { + "input": "source.json", + "extensions": [".py"], + "exclude": r"(^|/)tests/", + "maxFileBytes": 1000, + "maxBytes": 5000, + } + + def _repo(self, tmp: Path) -> Path: + (tmp / "tests").mkdir() + (tmp / "a.py").write_text("x = 1\n") + (tmp / "big.py").write_text("x" * 2000) + (tmp / "b.txt").write_text("not asked for") + (tmp / "tests" / "t.py").write_text("excluded") + (tmp / "untracked.py").write_text("not tracked") + subprocess.run(["git", "init", "-q", str(tmp)], check=True) + subprocess.run(["git", "-C", str(tmp), "add", "a.py", "big.py", "b.txt", "tests"], check=True) + return tmp + + def test_only_the_tracked_files_a_feature_asked_for_are_sent(self): + with tempfile.TemporaryDirectory() as t: + text, why = closed_features.collect_source(self.SPEC, {"CHECKOUT_DIR": str(self._repo(Path(t)))}) + self.assertEqual((json.loads(text), why), ({"a.py": "x = 1\n"}, None)) + + def test_the_workflow_can_turn_it_off(self): + out = closed_features.collect_source(self.SPEC, {"CLOSED_FEATURES_SOURCE": "false", "CHECKOUT_DIR": "/nowhere"}) + self.assertEqual(out, (None, "turned off by the workflow")) + + def test_past_the_cap_nothing_is_sent_and_it_says_why(self): + with tempfile.TemporaryDirectory() as t: + text, why = closed_features.collect_source( + {**self.SPEC, "maxBytes": 3}, {"CHECKOUT_DIR": str(self._repo(Path(t)))} + ) + self.assertIsNone(text) + self.assertIn("limit", why) + + def _sent(self, source, inputs=None): + feature = {**OTHER, "source": self.SPEC, "needs": ["pr.json"]} + service = _Server(lambda p, b: (200, {"envelope": _envelope("other", {}), "usage": {}})) + self.addCleanup(service.close) + ctx = {"repo": "o/r", "pr": 7, "head": "H"} + with mock.patch("builtins.print"): + closed_features.run_feature(feature, service.url, ctx, inputs or {"pr.json": "{}"}, {}, source) + return json.loads(gzip.decompress(service.requests[0]["body"])) + + def test_the_source_goes_only_in_the_request_of_the_feature_that_asked(self): + sent = self._sent(('{"a.py": "x"}', None)) + self.assertEqual(sent["files"]["source.json"], '{"a.py": "x"}') + service = _Server(lambda p, b: (200, {"envelope": _envelope("other", {}), "usage": {}})) + self.addCleanup(service.close) + with mock.patch("builtins.print"): + closed_features.run_feature( + {**GUIDE, "url": "/g"}, service.url, {"repo": "o/r", "pr": 7, "head": "H"}, {"pr.json": "{}"}, {}, None + ) + self.assertNotIn("source.json", json.loads(gzip.decompress(service.requests[0]["body"]))["files"]) + + def test_a_feature_without_its_source_is_told_why(self): + sent = self._sent((None, "turned off by the workflow")) + self.assertNotIn("source.json", sent["files"]) + self.assertEqual(sent["omitted"], {"source.json": "turned off by the workflow"}) + + def test_a_bundle_too_large_to_send_drops_the_source_and_keeps_the_feature(self): + import os + + huge = os.urandom(5_000_000).hex() + sent = self._sent((json.dumps({"a.py": huge}), None)) + self.assertNotIn("source.json", sent["files"]) + self.assertIn("too large", sent["omitted"]["source.json"]) + + GUIDE_INPUTS = closed_features.FALLBACK["needs"] From 20c302ff062f76bb1bba5f290da7565c3cbe83b7 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Sun, 1 Nov 2026 01:37:16 +0100 Subject: [PATCH 3/6] feat: use a run's own provider for every closed feature The run's llm, key and model now reach every feature, for each provider the service can call. A run on a provider it cannot reach makes no model call and is never sent to the hosted model. The manifest is read with the run's credentials. Co-Authored-By: Claude Opus 5.5 --- action.yml | 21 ++++++++- scripts/closed_features.py | 83 ++++++++++++++++++++++++----------- tests/test_closed_features.py | 67 ++++++++++++++++++++++++---- 3 files changed, 136 insertions(+), 35 deletions(-) diff --git a/action.yml b/action.yml index 94a0246..a5407cc 100644 --- a/action.yml +++ b/action.yml @@ -719,9 +719,28 @@ runs: CHECKOUT_DIR: ${{ github.workspace }}/.codeboarding-target GITHUB_TOKEN: ${{ inputs.github_token }} LICENSE_KEY: ${{ inputs.license_key }} + LLM: ${{ inputs.llm }} + MODEL: ${{ inputs.model }} + ANTHROPIC_API_KEY: ${{ inputs.anthropic_api_key }} + 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 }} + 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 diff --git a/scripts/closed_features.py b/scripts/closed_features.py index b18984b..c26ebdc 100644 --- a/scripts/closed_features.py +++ b/scripts/closed_features.py @@ -19,14 +19,18 @@ without it and says why, so the feature reports what it could not check. This script reads the files and sends them; it computes nothing from them. -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. Nothing here fails the run. +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, Bedrock) makes no model call at all: its features 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, OLLAMA_API_KEY, OPENROUTER_API_KEY, OPENAI_API_KEY, ACTIONS_ID_TOKEN_REQUEST_URL/_TOKEN. +CLOSED_FEATURES_SOURCE, LLM, MODEL, _API_KEY, _BASE_URL, +ACTIONS_ID_TOKEN_REQUEST_URL/_TOKEN. """ from __future__ import annotations @@ -42,7 +46,7 @@ from concurrent.futures import ThreadPoolExecutor from pathlib import Path from urllib.error import HTTPError, URLError -from urllib.parse import quote, urljoin +from urllib.parse import quote, urljoin, 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 @@ -50,7 +54,8 @@ 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 -OWN_KEYS = (("OLLAMA_API_KEY", "ollama"), ("OPENROUTER_API_KEY", "openrouter"), ("OPENAI_API_KEY", "openai")) +# The providers a service from before the manifest can call, each with the host it calls. +FALLBACK_HOSTS = {"ollama": "ollama.com", "openrouter": "openrouter.ai", "openai": "api.openai.com"} # What the review guide has always been: asked for when the service lists no features. FALLBACK = { "name": "review-guide", @@ -205,14 +210,23 @@ def _relay(): 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 model_headers(env: dict, hosts: dict[str, str]) -> dict[str, str]: + """What tells the service which model this run uses. Nothing for a hosted or licensed run. 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 = env.get(f"{llm.upper()}_API_KEY", "").strip() + base = env.get(f"{llm.upper()}_BASE_URL", "").strip() + host = hosts.get(llm) + 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("MODEL", "").strip(): + out["x-llm-model"] = env["MODEL"].strip() + return out def _bearer(env: dict) -> dict[str, str]: @@ -263,23 +277,26 @@ def collect_source(spec: dict, env: dict) -> tuple[str | None, str | None]: return json.dumps(files), None -def read_manifest(url: str) -> list[dict]: - """What the service runs, or the guide alone when it cannot say. A feature that cannot be read - is left out rather than guessed at; a service that names none is treated as one that failed.""" +def read_manifest(url: str, headers: dict[str, str], repo: str) -> tuple[list[dict], dict[str, str]]: + """What the service runs and which providers it can call, or the guide alone when it cannot say. + A feature that cannot be read is left out rather than guessed at; a service that names none is + treated as one that failed.""" + address = f"{url.rstrip('/')}/v1/features?repo={quote(repo, safe='/')}" try: - with urlopen( # nosec B310 - the service URL the workflow names - Request(f"{url.rstrip('/')}/v1/features", headers={"User-Agent": "codeboarding-action"}), timeout=15 - ) as res: - features = json.loads(res.read()).get("features") + req = Request(address, headers={**headers, "User-Agent": "codeboarding-action"}) + with urlopen(req, timeout=15) as res: # nosec B310 - the service URL the workflow names + manifest = json.loads(res.read()) + features = manifest.get("features") except (OSError, ValueError, AttributeError) as e: print(f"::notice::The service listed no features ({type(e).__name__}); asking for the review guide alone.") - return [FALLBACK] + return [FALLBACK], FALLBACK_HOSTS usable = [ f for f in features or [] if isinstance(f, dict) and isinstance(f.get("name"), str) and isinstance(f.get("output"), str) ] - return usable or [FALLBACK] + hosts = manifest.get("providers") + return (usable, hosts if isinstance(hosts, dict) else {}) if usable else ([FALLBACK], FALLBACK_HOSTS) def _in_artifact(artifact: Path, relative: str) -> Path: @@ -358,6 +375,8 @@ def run_feature( source: tuple[str | None, str | None] | None = None, ) -> dict: """One feature, start to finish: never raises, always an envelope.""" + if "x-llm-provider" in headers and "x-llm-api-key" not in headers and feature.get("needsModel", True): + return _no_result(feature, ctx, "skipped", "it needs a model, and the service cannot call this run's provider") needs = feature.get("needs") or [] missing = [n for n in needs if n not in inputs] if missing: @@ -419,7 +438,12 @@ def main(env: dict | None = None) -> int: 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"] - features = read_manifest(base) + try: + bearer = _bearer(env) + features, hosts = read_manifest(base, bearer, ctx["repo"]) + except Exception as e: # the caller could not be proved, so nothing can be asked for + print(f"::notice::Could not identify this run to the service: {type(e).__name__}") + bearer, features, hosts = None, [FALLBACK], FALLBACK_HOSTS results: dict[str, dict] = { f["name"]: _no_result(f, ctx, "skipped", "turned off by the service") for f in features @@ -428,8 +452,15 @@ def main(env: dict | None = None) -> int: enabled = [f for f in features if f["name"] not in results] if enabled: try: + if bearer is None: + raise Skip("this run could not be identified to the service") inputs = collect(env) - headers = credentials(env) + headers = {**bearer, **model_headers(env, hosts)} + if "x-llm-provider" in headers and "x-llm-api-key" not in headers: + print( + f"::notice::The service cannot call this run's provider ({headers['x-llm-provider']}), " + "so it makes no model call: measured findings only, and no review guide." + ) except Exception as e: # every feature reads this, so every feature is told why reason = str(e) if isinstance(e, Skip) else f"collecting its inputs failed: {type(e).__name__}: {e}" results.update({f["name"]: _no_result(f, ctx, "failed", reason) for f in enabled}) diff --git a/tests/test_closed_features.py b/tests/test_closed_features.py index ceaa354..059d0a5 100644 --- a/tests/test_closed_features.py +++ b/tests/test_closed_features.py @@ -123,8 +123,10 @@ def test_the_bundle_reads_the_analysed_commits_and_both_sides_of_each_file(self) class CredentialTests(unittest.TestCase): + HOSTS = {"openrouter": "openrouter.ai", "ollama": "ollama.com", "anthropic": "api.anthropic.com"} + def test_without_oidc_the_runs_own_token_proves_the_repository(self): - self.assertEqual(closed_features.credentials({"GITHUB_TOKEN": "ghs_run"}), {"Authorization": "Bearer ghs_run"}) + self.assertEqual(closed_features._bearer({"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"})) @@ -137,20 +139,35 @@ def test_with_oidc_the_bearer_is_the_proxys_own_with_the_license(self): } with tempfile.TemporaryDirectory() as t: env["RUNNER_TEMP"] = t - headers = closed_features.credentials(env) + headers = closed_features._bearer(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 = closed_features.credentials({"GITHUB_TOKEN": "ghs_run", "OPENROUTER_API_KEY": " sk-or "}) + def test_a_hosted_or_licensed_run_sends_no_model_at_all(self): + for llm in ("hosted", "license", ""): + env = {"LLM": llm, "OPENROUTER_API_KEY": "left over"} + self.assertEqual(closed_features.model_headers(env, self.HOSTS), {}) + + def test_a_runs_own_provider_key_and_model_are_used(self): + env = {"LLM": "anthropic", "ANTHROPIC_API_KEY": " sk-a ", "MODEL": "claude-sonnet-5"} self.assertEqual( - headers, - {"Authorization": "Bearer ghs_run", "x-llm-api-key": "sk-or", "x-llm-provider": "openrouter"}, + closed_features.model_headers(env, self.HOSTS), + {"x-llm-api-key": "sk-a", "x-llm-provider": "anthropic", "x-llm-model": "claude-sonnet-5"}, ) + def test_ollama_on_its_cloud_is_reachable_and_a_self_hosted_one_is_not(self): + cloud = {"LLM": "ollama", "OLLAMA_API_KEY": "k", "OLLAMA_BASE_URL": "https://ollama.com"} + self.assertEqual(closed_features.model_headers(cloud, self.HOSTS)["x-llm-api-key"], "k") + own = {**cloud, "OLLAMA_BASE_URL": "http://10.0.0.5:11434"} + self.assertEqual(closed_features.model_headers(own, self.HOSTS), {"x-llm-provider": "ollama"}) + + def test_a_provider_the_service_cannot_call_never_sends_its_key(self): + env = {"LLM": "aws_bedrock", "AWS_BEDROCK_API_KEY": "k"} + self.assertEqual(closed_features.model_headers(env, self.HOSTS), {"x-llm-provider": "aws_bedrock"}) + GUIDE = { "name": "review-guide", @@ -204,7 +221,7 @@ def _read(env, name): class RunnerTests(unittest.TestCase): def _serve(self, features, answer): def respond(path, body): - if path == "/v1/features": + if path.startswith("/v1/features?"): return features if isinstance(features, tuple) else (200, {"version": 1, "features": features}) return answer(path, body) @@ -269,7 +286,8 @@ def test_a_turned_off_feature_is_skipped_and_never_requested(self): env = {**_workspace(Path(t)), "CLOSED_FUNCTIONS_URL": service.url} _run(env) self.assertEqual(_read(env, "closed/other.json")["status"], "skipped") - self.assertEqual([r["path"] for r in service.requests], ["/v1/features"]) + self.assertEqual([r["path"] for r in service.requests], ["/v1/features?repo=o/r"]) + self.assertEqual(service.requests[0]["headers"]["Authorization"], "Bearer ghs_run") def test_a_feature_needing_what_this_action_does_not_collect_is_skipped(self): wants = {**OTHER, "needs": ["whole-repo.tar"]} @@ -318,6 +336,39 @@ def test_a_path_outside_the_artifact_is_refused(self): self.assertFalse((Path(t) / "outside.json").exists()) +class OwnProviderTests(RunnerTests): + def test_a_provider_the_service_cannot_call_gets_no_guide_and_no_hosted_model(self): + env_keys = {"LLM": "litellm", "LITELLM_API_KEY": "k", "LITELLM_BASE_URL": "http://proxy:4000"} + guide = {**GUIDE, "needsModel": True} + other = {**OTHER, "needsModel": False} + service = self._serve([guide, other], lambda p, b: (200, {"envelope": _envelope("other", {}), "usage": {}})) + with tempfile.TemporaryDirectory() as t: + env = {**_workspace(Path(t)), **env_keys, "CLOSED_FUNCTIONS_URL": service.url} + _, printed = _run(env) + self.assertEqual(_read(env, "closed/review-guide.json")["status"], "skipped") + self.assertFalse((Path(env["ARTIFACT_DIR"]) / "review_guide.json").exists()) + self.assertEqual(_read(env, "closed/other.json")["status"], "ok") + posts = [r for r in service.requests if r["body"]] + self.assertEqual([r["path"] for r in posts], ["/v1/features/other"]) + lower = {k.lower(): v for k, v in posts[0]["headers"].items()} + self.assertEqual(lower["x-llm-provider"], "litellm") + self.assertNotIn("x-llm-api-key", lower) + self.assertEqual(sum("cannot call this run's provider" in line for line in printed), 2) + + def test_with_no_manifest_a_run_on_its_own_provider_still_never_reaches_the_hosted_model(self): + service = self._serve((404, {}), lambda p, b: (200, {"guide": {"version": 1}, "usage": {}})) + with tempfile.TemporaryDirectory() as t: + env = { + **_workspace(Path(t)), + "LLM": "anthropic", + "ANTHROPIC_API_KEY": "k", + "CLOSED_FUNCTIONS_URL": service.url, + } + _run(env) + self.assertEqual(_read(env, "closed/review-guide.json")["status"], "skipped") + self.assertEqual([r for r in service.requests if r["body"]], []) + + class SourceTests(unittest.TestCase): SPEC = { "input": "source.json", From 48d0c311c9eba20af17bfa64538b166c8e3bab17 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Sun, 1 Nov 2026 01:39:51 +0100 Subject: [PATCH 4/6] feat: Bedrock's region, and say what really reaches CodeBoarding Co-Authored-By: Claude Opus 5.5 --- README.md | 14 ++++++++++---- action.yml | 2 ++ scripts/closed_features.py | 2 ++ tests/test_closed_features.py | 17 +++++++++++++++-- 4 files changed, 29 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 21b1b45..ac757db 100644 --- a/README.md +++ b/README.md @@ -170,10 +170,16 @@ The same rule makes the combinations explicit rather than order-dependent: | `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 closed features, such as the review guide, 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. ### Providers diff --git a/action.yml b/action.yml index a5407cc..7587f03 100644 --- a/action.yml +++ b/action.yml @@ -722,6 +722,8 @@ runs: LLM: ${{ inputs.llm }} MODEL: ${{ inputs.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 }} diff --git a/scripts/closed_features.py b/scripts/closed_features.py index c26ebdc..8c7d61e 100644 --- a/scripts/closed_features.py +++ b/scripts/closed_features.py @@ -224,6 +224,8 @@ def model_headers(env: dict, hosts: dict[str, str]) -> dict[str, str]: 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() if env.get("MODEL", "").strip(): out["x-llm-model"] = env["MODEL"].strip() return out diff --git a/tests/test_closed_features.py b/tests/test_closed_features.py index 059d0a5..69ff463 100644 --- a/tests/test_closed_features.py +++ b/tests/test_closed_features.py @@ -165,8 +165,21 @@ def test_ollama_on_its_cloud_is_reachable_and_a_self_hosted_one_is_not(self): self.assertEqual(closed_features.model_headers(own, self.HOSTS), {"x-llm-provider": "ollama"}) def test_a_provider_the_service_cannot_call_never_sends_its_key(self): - env = {"LLM": "aws_bedrock", "AWS_BEDROCK_API_KEY": "k"} - self.assertEqual(closed_features.model_headers(env, self.HOSTS), {"x-llm-provider": "aws_bedrock"}) + env = {"LLM": "litellm", "LITELLM_API_KEY": "k", "LITELLM_BASE_URL": "https://proxy.internal"} + self.assertEqual(closed_features.model_headers(env, self.HOSTS), {"x-llm-provider": "litellm"}) + + def test_a_base_url_elsewhere_keeps_the_key_at_home(self): + env = {"LLM": "openai", "OPENAI_API_KEY": "k", "OPENAI_BASE_URL": "https://gateway.corp/v1"} + hosts = {"openai": "api.openai.com"} + self.assertEqual(closed_features.model_headers(env, hosts), {"x-llm-provider": "openai"}) + + def test_bedrock_sends_its_region(self): + env = {"LLM": "aws_bedrock", "AWS_BEDROCK_API_KEY": "k", "AWS_BEDROCK_REGION": "eu-west-1"} + hosts = {"aws_bedrock": "bedrock-runtime.{region}.amazonaws.com"} + self.assertEqual( + closed_features.model_headers(env, hosts), + {"x-llm-api-key": "k", "x-llm-provider": "aws_bedrock", "x-llm-region": "eu-west-1"}, + ) GUIDE = { From cc15df4803302f9e78e87ea21b9246d3ce5d7c47 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Sun, 1 Nov 2026 01:45:40 +0100 Subject: [PATCH 5/6] feat: hosted runs use CodeBoarding's model, and only an own key chooses llm: hosted and license now refuse model, agent_model and parsing_model, with hosted_with_model and license_with_model. A run's own key uses agent_model, then model, as its analysis does. Co-Authored-By: Claude Opus 5.5 --- README.md | 1 + action.yml | 4 ++++ scripts/action/credential_check.py | 17 +++++++++++++++++ scripts/closed_features.py | 12 +++++++----- tests/test_closed_features.py | 4 ++++ tests/test_llm_contract.py | 16 ++++++++++++++++ 6 files changed, 49 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index ac757db..abc3077 100644 --- a/README.md +++ b/README.md @@ -164,6 +164,7 @@ 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 | diff --git a/action.yml b/action.yml index 7587f03..e8a6b14 100644 --- a/action.yml +++ b/action.yml @@ -308,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 }} @@ -721,6 +724,7 @@ runs: 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 }} 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 index 8c7d61e..a4a36c0 100644 --- a/scripts/closed_features.py +++ b/scripts/closed_features.py @@ -29,7 +29,7 @@ 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, _API_KEY, _BASE_URL, +CLOSED_FEATURES_SOURCE, LLM, MODEL, AGENT_MODEL, _API_KEY, _BASE_URL, ACTIONS_ID_TOKEN_REQUEST_URL/_TOKEN. """ @@ -211,8 +211,8 @@ def _relay(): def model_headers(env: dict, hosts: dict[str, str]) -> dict[str, str]: - """What tells the service which model this run uses. Nothing for a hosted or licensed run. A run's - own provider sends its key and model; one the service cannot reach sends its name alone, which + """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() @@ -226,8 +226,10 @@ def model_headers(env: dict, hosts: dict[str, str]) -> dict[str, str]: 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() - if env.get("MODEL", "").strip(): - out["x-llm-model"] = env["MODEL"].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 diff --git a/tests/test_closed_features.py b/tests/test_closed_features.py index 69ff463..1db360e 100644 --- a/tests/test_closed_features.py +++ b/tests/test_closed_features.py @@ -158,6 +158,10 @@ def test_a_runs_own_provider_key_and_model_are_used(self): {"x-llm-api-key": "sk-a", "x-llm-provider": "anthropic", "x-llm-model": "claude-sonnet-5"}, ) + def test_the_analysis_model_wins_as_it_does_in_the_analysis(self): + env = {"LLM": "anthropic", "ANTHROPIC_API_KEY": "k", "MODEL": "general", "AGENT_MODEL": "analysis"} + self.assertEqual(closed_features.model_headers(env, self.HOSTS)["x-llm-model"], "analysis") + def test_ollama_on_its_cloud_is_reachable_and_a_self_hosted_one_is_not(self): cloud = {"LLM": "ollama", "OLLAMA_API_KEY": "k", "OLLAMA_BASE_URL": "https://ollama.com"} self.assertEqual(closed_features.model_headers(cloud, self.HOSTS)["x-llm-api-key"], "k") 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} From 071e7e9dfbd4e56fcdadabcf49dd4e95563cffb0 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Sun, 1 Nov 2026 13:54:19 +0100 Subject: [PATCH 6/6] docs: say what the model sees of a repository's source Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- action.yml | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index abc3077..59257e3 100644 --- a/README.md +++ b/README.md @@ -355,7 +355,7 @@ The first sync after the merge creates `codeboarding/baseline`, catching up from | `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 closed features, such as the pull request's review guide. The run sends it this analysis and stores one file per feature in the review artifact (`closed/.json`, plus `review_guide.json`). Set to `''` to disable. Never fails the run. | -| `closed_features_source` | review | `true` | Whether a closed feature that asks for it may read this repository's source, for checks that need the whole tree. Only that feature's request carries it, capped in size, and the service keeps nothing. With `false` those checks do not run and the result says so. | +| `closed_features_source` | review | `true` | Whether a closed feature that asks for it may read this repository's source, for checks that need the whole tree and to verify a finding. Only that feature's 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. | `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 e8a6b14..294e799 100644 --- a/action.yml +++ b/action.yml @@ -154,8 +154,9 @@ inputs: closed_features_source: description: >- Whether a closed feature that asks for it may read this repository's source, for checks that - need the whole tree. Only that feature's request carries it, capped in size, and the service - keeps nothing. With 'false' those checks do not run and the result says so. + need the whole tree and to verify a finding. Only that feature's 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: