From d4ae9e9d0f99fa04f1c5b60d94e7a75e22f5c449 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Thu, 27 Aug 2026 15:37:03 +0200 Subject: [PATCH 1/6] feat: require an explicit llm input and never fall back to hosted credentials The action resolved credentials by precedence: a provider key if present, a licence if present, otherwise CodeBoarding's hosted tier. An empty value was therefore indistinguishable from "no preference", so a repository that selected Anthropic and had not added its secret yet ran green on CodeBoarding's hosted OpenRouter tier -- a different vendor, a different model, our money -- and nothing in the run said so. Credentials are now named, not inferred. `llm` is required and takes `hosted`, `license`, or a provider name; each provider has its own `_api_key` input. Anything ambiguous is refused: a named provider without its key, a hosted tier carrying a provider key, a licence where it would not be spent, a second provider's key. Refusals happen before the checkout and the engine install, and name the input and the secret to fix. A licence alongside a provider key stays valid and is reported as `byok+license`. Metering that combination needs proxy work and is not in scope. The provider table is mirrored from the pinned engine rather than hand-copied per site, and the foreign-selector list with-auth.sh strips is now derived from it. That list had already fallen behind: it was missing ORCAROUTER_API_KEY, which 0.13.10 added, so an inherited value could select a provider the workflow never asked for. A drift test installs the pinned release in CI and fails when the two disagree. Ships as feat: rather than feat!: on purpose -- see AGENTS.md. A major bump would freeze v1 and leave every existing workflow on the old silent-fallback behaviour permanently, which is the opposite of the intent. BREAKING: `llm_api_key` and `llm_provider` are replaced by `llm` plus per-provider inputs. Workflows that set neither must add `llm: hosted`. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/codeboarding-sync.yml | 1 + .github/workflows/codeboarding.yml | 4 + .github/workflows/test.yml | 2 + AGENTS.md | 26 ++ README.md | 117 +++++++-- action.yml | 184 +++++++++++++- scripts/action/configure-auth.sh | 53 ++-- scripts/action/llm-providers.json | 155 ++++++++++++ scripts/action/preflight-llm.sh | 86 +++++++ scripts/action/resolve_llm.py | 256 +++++++++++++++++++ scripts/action/with-auth.sh | 45 ++-- tests/test_action_auth.py | 317 +++++++++++++----------- tests/test_action_inputs.py | 85 +++++++ tests/test_llm_contract.py | 156 ++++++++++++ tests/test_provider_table_drift.py | 87 +++++++ 15 files changed, 1339 insertions(+), 235 deletions(-) create mode 100644 scripts/action/llm-providers.json create mode 100755 scripts/action/preflight-llm.sh create mode 100755 scripts/action/resolve_llm.py create mode 100644 tests/test_action_inputs.py create mode 100644 tests/test_llm_contract.py create mode 100644 tests/test_provider_table_drift.py diff --git a/.github/workflows/codeboarding-sync.yml b/.github/workflows/codeboarding-sync.yml index 2700bc0..b72c6bc 100644 --- a/.github/workflows/codeboarding-sync.yml +++ b/.github/workflows/codeboarding-sync.yml @@ -141,6 +141,7 @@ jobs: - uses: ./ with: mode: sync + llm: hosted sync_strategy: ${{ inputs.sync_strategy || 'push' }} force_full: ${{ inputs.force_full || false }} # App token authenticates the baseline push so the commit is attributed diff --git a/.github/workflows/codeboarding.yml b/.github/workflows/codeboarding.yml index 16330cc..8c681eb 100644 --- a/.github/workflows/codeboarding.yml +++ b/.github/workflows/codeboarding.yml @@ -111,4 +111,8 @@ jobs: echo "::warning::CodeBoarding GitHub App token is unavailable; falling back to github-actions[bot]. Check CODEBOARDING_APP_PRIVATE_KEY formatting if app credentials are configured." - uses: ./ with: + # Named, not inferred: the action refuses to guess where credentials come + # from, so every workflow says which of hosted / license / a provider it + # wants. This repo dogfoods the hosted free tier. + llm: hosted github_token: ${{ steps.codeboarding-app-token-client.outputs.token || steps.codeboarding-app-token-app.outputs.token || github.token }} diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 7c8219e..e910cb9 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -40,6 +40,8 @@ jobs: exit 1 fi python -m pip install --disable-pip-version-check "$requirement" + - name: Check the provider table against the installed release + run: python -m unittest tests.test_provider_table_drift -v - name: Render with the installed CodeBoarding release run: >- python scripts/diff_to_mermaid.py diff --git a/AGENTS.md b/AGENTS.md index d727d58..d1d78ae 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -15,6 +15,19 @@ pinned to a release in `action.yml`. `scripts/engine_adapter.py` is the CLI adapter into it (no analysis logic lives there). Engine changes reach users only when that pin is bumped *and* a new action release ships. +## Bumping the engine pin + +`scripts/action/llm-providers.json` mirrors the pinned release's `LLM_PROVIDERS`. +Credentials are validated before the engine is installed — that is what makes a +misconfigured run fail in seconds instead of a minute — so the action cannot ask +the engine at run time and keeps this copy instead. + +When you change the `codeboarding==` pin in `action.yml`, update that file in the +same commit: its `engine` field, and any provider whose selection variables +changed. `tests/test_provider_table_drift.py` runs in the `core-compatibility` CI +job and fails when they disagree. Adding a provider also means adding its +`_api_key` input to `action.yml`; `tests/test_action_inputs.py` checks that. + ## Protected tests Some tests encode a behavioural contract that is expensive to rediscover once @@ -35,6 +48,12 @@ Protected tests: its merge base, never against the base branch tip. Comparing against the tip attributes other people's commits to the pull request and reports them backwards, as removals. +- `tests/test_action_auth.py::test_a_named_provider_never_falls_back_to_codeboarding_credentials` + — a workflow that names a provider runs on that provider or fails. The action + used to read an empty key as "no preference" and resolve it to CodeBoarding's + hosted tier, so a repository that had not added its secret yet went green + while running on another vendor's model and CodeBoarding's money, silently. + Any change that reintroduces a credential fallback breaks this test. ## Releases @@ -56,6 +75,13 @@ messages, so every commit and PR title must follow Conventional Commits: - `fix:` → patch bump - `feat!:` / `fix!:` or a `BREAKING CHANGE:` footer → major bump. Avoid unless intended: consumers pinned to the old major tag never receive it automatically. + +A deliberate exception exists. The explicit-credentials change (`llm` required, +no fallback) is a breaking change that shipped as `feat:`, not `feat!:`. A major +bump moves adopters to `v2` and freezes `v1`, which would have left every +existing workflow on the old silent-fallback behaviour forever — the opposite of +the intent. Shipping it as a minor bump on the moving `v1` tag is what makes +adopters actually receive it. Do not "correct" this to `feat!:` after the fact. - `chore:` / `docs:` / `ci:` / `refactor:` / `test:` → ride along in the next release but do not trigger one. diff --git a/README.md b/README.md index 9166cd3..c91d16e 100644 --- a/README.md +++ b/README.md @@ -50,7 +50,9 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 60 steps: - - uses: CodeBoarding/CodeBoarding-action@v2 + - uses: CodeBoarding/CodeBoarding-action@v1 + with: + llm: hosted # or license, or a provider name -- see Authentication ``` Automatic runs update one sticky **CodeBoarding review** comment. A trusted repository owner, member, or collaborator can comment `/codeboarding` to analyze the current PR head again, including on fork PRs; every command creates a new result comment. @@ -98,29 +100,91 @@ Fork pull requests never carry an analysis forward. They are reviewed on request ## Authentication and providers -With no LLM inputs, the action uses CodeBoarding's hosted OpenRouter tier. It mints short-lived GitHub OIDC credentials per request, so the job needs `id-token: write` and no stored LLM secret. - -For a direct provider, pass its name and key: +The `llm` input is required and says where analysis credentials come from. There are +three answers, and the action never picks one for you: ```yaml - - uses: CodeBoarding/CodeBoarding-action@v2 - with: - llm_provider: anthropic - llm_api_key: ${{ secrets.ANTHROPIC_API_KEY }} + with: + llm: hosted # CodeBoarding's free tier +``` +```yaml + with: + llm: license # a CodeBoarding plan + license_key: ${{ secrets.CODEBOARDING_LICENSE }} +``` +```yaml + with: + llm: anthropic # your own provider key + anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} ``` -The action maps a provider name to the environment variable Core uses (`anthropic` → `ANTHROPIC_API_KEY`, `openai` → `OPENAI_API_KEY`, and so on). `aws`/`aws_bedrock` maps to `AWS_BEARER_TOKEN_BEDROCK`. Provider names following the standard convention are not restricted by an action-side allowlist; the pinned Core release remains the source of truth for which providers it implements. - -CodeBoarding 0.13.10 supports OpenRouter, OpenAI-compatible endpoints, Anthropic-compatible endpoints, Google, Vercel AI Gateway, AWS Bedrock, Cerebras, DeepSeek, GLM, Kimi, OrcaRouter, Ollama, and LiteLLM. See Core's [`agents/llm_config.py`](https://github.com/CodeBoarding/CodeBoarding/blob/main/agents/llm_config.py) for current defaults and endpoint variables. In particular, Ollama needs `OLLAMA_BASE_URL` or `OLLAMA_HOST`, and LiteLLM needs `LITELLM_BASE_URL` on the action step. +`hosted` and `license` run through CodeBoarding's proxy and need `id-token: write`, which +mints short-lived credentials per request and stores no LLM secret in your repository. A +provider key is used directly and needs no OIDC permission. -A CodeBoarding license keeps the hosted OIDC path but removes hosted quota limits: +**An empty value is never a fallback.** If you name a provider and its key is missing -- +because the secret does not exist yet, or is misspelt — the run fails in its first +seconds and says which input and which secret to fix. It does not quietly analyze on +CodeBoarding's hosted tier instead. That was the old behaviour, and it meant a repository +could report an Anthropic review that Anthropic never produced. -```yaml - with: - license_key: ${{ secrets.CODEBOARDING_LICENSE }} -``` +The same rule makes the combinations explicit rather than order-dependent: -`llm_api_key` takes precedence over `license_key`. A direct provider key does not require `id-token: write`; hosted free and licensed usage does. +| Workflow says | Result | +|---|---| +| nothing | refused: `llm` is required | +| `llm: hosted` | the free tier | +| `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: anthropic` + `anthropic_api_key` | Anthropic, directly | +| `llm: anthropic`, key empty or absent | refused: names the input and the secret | +| `llm: anthropic` + `openai_api_key` | refused: a second provider's key | +| `llm: anthropic` + key + `license_key` | Anthropic, on a CodeBoarding plan | + +A licence alongside your own key is deliberately allowed: it says "my CodeBoarding plan, +my own tokens". Direct provider calls never reach our proxy, so nothing meters that +combination today; it is recorded and reported, not enforced. + +### Providers + +Each provider has its own inputs, so which key a workflow uses is readable from the file +without knowing any precedence rules. + +| `llm` | Provider | Inputs | Needs at least one of | +|---|---|---|---| +| `anthropic` | Anthropic | `anthropic_api_key` | `anthropic_api_key` | +| `aws` | AWS Bedrock | `aws_api_key`, `aws_region` | `aws_api_key` | +| `cerebras` | Cerebras | `cerebras_api_key` | `cerebras_api_key` | +| `deepseek` | DeepSeek | `deepseek_api_key`, `deepseek_base_url` | `deepseek_api_key` or `deepseek_base_url` | +| `glm` | GLM | `glm_api_key`, `glm_base_url` | `glm_api_key` or `glm_base_url` | +| `google` | Google Gemini | `google_api_key` | `google_api_key` | +| `kimi` | Kimi | `kimi_api_key`, `kimi_base_url` | `kimi_api_key` or `kimi_base_url` | +| `litellm` | LiteLLM | `litellm_api_key`, `litellm_base_url` | `litellm_base_url` | +| `ollama` | Ollama | `ollama_api_key`, `ollama_base_url` | `ollama_base_url` | +| `openai` | OpenAI | `openai_api_key`, `openai_base_url` | `openai_api_key` or `openai_base_url` | +| `openrouter` | OpenRouter | `openrouter_api_key` | `openrouter_api_key` | +| `orcarouter` | OrcaRouter | `orcarouter_api_key` | `orcarouter_api_key` | +| `vercel` | Vercel AI Gateway | `vercel_api_key`, `vercel_base_url` | `vercel_api_key` or `vercel_base_url` | + +`aws_bedrock`, `bedrock` and `gemini` are accepted as aliases. `ollama` and `litellm` are +selected by their endpoint rather than a key, which is why a key alone does not configure +them — that mirrors how Core itself decides. + +This table is generated from [`scripts/action/llm-providers.json`](scripts/action/llm-providers.json), +which mirrors the CodeBoarding release this action pins. `tests/test_provider_table_drift.py` +installs that release in CI and fails if the two disagree, so a provider cannot be added +to Core and silently stay unreachable here. + +### Reporting + +Every run reports what it resolved, so the answer never has to be inferred from behaviour: + +- outputs `llm_tier` (`hosted`, `license`, `byok`, `byok+license`), `llm_provider`, and + `llm_config_error` (empty when configured); +- a job-summary table naming the tier and provider; +- on a configuration failure, an error annotation and — in review mode — a pull request + comment with the fix, so the person who has to add the secret sees it where they are. ## Model selection @@ -189,9 +253,10 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 60 steps: - - uses: CodeBoarding/CodeBoarding-action@v2 + - uses: CodeBoarding/CodeBoarding-action@v1 with: mode: sync + llm: hosted target_branch: main force_full: ${{ inputs.force_full || false }} ``` @@ -209,9 +274,10 @@ permissions: id-token: write # ... - - uses: CodeBoarding/CodeBoarding-action@v2 + - uses: CodeBoarding/CodeBoarding-action@v1 with: mode: sync + llm: hosted target_branch: main sync_strategy: pull_request ``` @@ -225,9 +291,11 @@ With the default `github.token`, the repository or organization must allow GitHu | Input | Mode | Default | Description | |---|---|---|---| | `mode` | both | `review` | `review` or `sync`. | -| `llm_api_key` | both | empty | Direct-provider key. With the default provider, empty selects hosted OIDC usage. | -| `llm_provider` | both | `openrouter` | Provider for `llm_api_key`. | -| `license_key` | both | empty | License for unmetered hosted usage. | +| `llm` | both | **required** | `hosted`, `license`, or a provider name. No default. | +| `_api_key` | both | empty | That provider's key, e.g. `anthropic_api_key`. See [Providers](#providers). | +| `_base_url` | both | empty | That provider's endpoint, where it has one. | +| `aws_region` | both | empty | Bedrock region. Core defaults to `us-east-1`. | +| `license_key` | both | empty | CodeBoarding license. Required by `llm: license`. | | `model` | both | empty | Default model for both analysis and parsing. | | `agent_model` | both | empty | Analysis-only override for `model`. | | `parsing_model` | both | empty | Parsing-only override for `model`. | @@ -237,12 +305,15 @@ With the default `github.token`, the repository or organization must allow GitHu | `force_full` | sync | `false` | Ignore the committed baseline for this run. | | `warmstart_retention_days` | review | `1` | Days to keep the reusable analysis. Only the next run reads it. | -The `/codeboarding` command, comment heading, Mermaid direction (`LR`), hosted webview URL, rolling sync branch, commit message, and CodeBoarding 0.13.10 version are intentionally fixed in v2 rather than exposed as configuration. +The `/codeboarding` command, comment heading, Mermaid direction (`LR`), hosted webview URL, rolling sync branch, commit message, and CodeBoarding 0.13.10 version are intentionally fixed rather than exposed as configuration. ## Outputs | Output | Mode | Description | |---|---|---| +| `llm_tier` | both | `hosted`, `license`, `byok`, or `byok+license`. | +| `llm_provider` | both | Provider the run used. | +| `llm_config_error` | both | Configuration failure code, empty when configured. | | `diagram_md` | review | Path to the rendered Mermaid block on the runner. | | `n_changed` | review | Number of changed components. | | `truncated` | review | Whether the graph was reduced to fit GitHub limits. | diff --git a/action.yml b/action.yml index 220246e..1a9a0d7 100644 --- a/action.yml +++ b/action.yml @@ -9,16 +9,108 @@ inputs: description: 'review posts a PR architecture diff; sync updates the versioned analysis baseline.' required: false default: 'review' - llm_api_key: - description: 'Optional direct-provider key. With the default provider, empty selects hosted OIDC usage.' + llm: + description: 'Required. Where analysis credentials come from: hosted, license, or a provider name (anthropic, aws, cerebras, deepseek, glm, google, kimi, litellm, ollama, openai, openrouter, orcarouter, vercel).' + required: true + license_key: + description: 'CodeBoarding license key. Required by llm: license; optional alongside a provider key.' required: false default: '' - llm_provider: - description: 'Provider for llm_api_key, for example openrouter, anthropic, openai, google, or ollama.' + # Anthropic - selected by llm: anthropic + anthropic_api_key: + description: 'Anthropic API key, used when llm is anthropic. Sets ANTHROPIC_API_KEY.' required: false - default: 'openrouter' - license_key: - description: 'Optional CodeBoarding license for unmetered hosted usage.' + default: '' + # AWS Bedrock - selected by llm: aws + aws_api_key: + description: 'AWS Bedrock API key, used when llm is aws. Sets AWS_BEARER_TOKEN_BEDROCK.' + required: false + default: '' + aws_region: + description: 'AWS Bedrock region, used when llm is aws. Sets AWS_DEFAULT_REGION.' + required: false + default: '' + # Cerebras - selected by llm: cerebras + cerebras_api_key: + description: 'Cerebras API key, used when llm is cerebras. Sets CEREBRAS_API_KEY.' + required: false + default: '' + # DeepSeek - selected by llm: deepseek + deepseek_api_key: + description: 'DeepSeek API key, used when llm is deepseek. Sets DEEPSEEK_API_KEY.' + required: false + default: '' + deepseek_base_url: + description: 'DeepSeek endpoint, used when llm is deepseek. Sets DEEPSEEK_BASE_URL.' + required: false + default: '' + # GLM - selected by llm: glm + glm_api_key: + description: 'GLM API key, used when llm is glm. Sets GLM_API_KEY.' + required: false + default: '' + glm_base_url: + description: 'GLM endpoint, used when llm is glm. Sets GLM_BASE_URL.' + required: false + default: '' + # Google Gemini - selected by llm: google + google_api_key: + description: 'Google Gemini API key, used when llm is google. Sets GOOGLE_API_KEY.' + required: false + default: '' + # Kimi - selected by llm: kimi + kimi_api_key: + description: 'Kimi API key, used when llm is kimi. Sets KIMI_API_KEY.' + required: false + default: '' + kimi_base_url: + description: 'Kimi endpoint, used when llm is kimi. Sets KIMI_BASE_URL.' + required: false + default: '' + # LiteLLM - selected by llm: litellm + litellm_api_key: + description: 'LiteLLM API key, used when llm is litellm. Sets LITELLM_API_KEY.' + required: false + default: '' + litellm_base_url: + description: 'LiteLLM endpoint, used when llm is litellm. Sets LITELLM_BASE_URL.' + required: false + default: '' + # Ollama - selected by llm: ollama + ollama_api_key: + description: 'Ollama API key, used when llm is ollama. Sets OLLAMA_API_KEY.' + required: false + default: '' + ollama_base_url: + description: 'Ollama endpoint, used when llm is ollama. Sets OLLAMA_BASE_URL.' + required: false + default: '' + # OpenAI - selected by llm: openai + openai_api_key: + description: 'OpenAI API key, used when llm is openai. Sets OPENAI_API_KEY.' + required: false + default: '' + openai_base_url: + description: 'OpenAI endpoint, used when llm is openai. Sets OPENAI_BASE_URL.' + required: false + default: '' + # OpenRouter - selected by llm: openrouter + openrouter_api_key: + description: 'OpenRouter API key, used when llm is openrouter. Sets OPENROUTER_API_KEY.' + required: false + default: '' + # OrcaRouter - selected by llm: orcarouter + orcarouter_api_key: + description: 'OrcaRouter API key, used when llm is orcarouter. Sets ORCAROUTER_API_KEY.' + required: false + default: '' + # Vercel AI Gateway - selected by llm: vercel + vercel_api_key: + description: 'Vercel AI Gateway API key, used when llm is vercel. Sets VERCEL_API_KEY.' + required: false + default: '' + vercel_base_url: + description: 'Vercel AI Gateway endpoint, used when llm is vercel. Sets VERCEL_BASE_URL.' required: false default: '' model: @@ -54,6 +146,15 @@ inputs: required: false default: '1' outputs: + llm_tier: + description: 'Resolved credential tier: hosted, license, byok, or byok+license.' + value: ${{ steps.llm.outputs.tier }} + llm_provider: + description: 'Provider this run used for analysis.' + value: ${{ steps.llm.outputs.provider }} + llm_config_error: + description: 'Stable code for a credential configuration failure, empty when configured.' + value: ${{ steps.llm.outputs.error }} diagram_md: description: 'Path to the rendered Mermaid review diagram.' value: ${{ steps.review_render.outputs.diagram_md }} @@ -130,6 +231,70 @@ runs: COMMENT_ID: ${{ github.event.comment.id }} run: GH_HOST="${GITHUB_SERVER_URL#*://}" gh api -X POST "repos/${REPOSITORY}/issues/comments/${COMMENT_ID}/reactions" -f content=eyes >/dev/null + # Resolved before the checkout and the engine install, so a workflow that names a + # provider it has no key for fails within seconds, naming the input to fix, instead + # of running to completion on credentials it never asked for. Nothing downstream + # re-decides this: the plan it writes is what the analysis runs with. + - name: Check LLM configuration + id: llm + if: steps.guard.outputs.skip != 'true' + continue-on-error: true + shell: bash + env: + ACTION_PATH: ${{ github.action_path }} + CB_IN_LLM: ${{ inputs.llm }} + CB_IN_LICENSE_KEY: ${{ inputs.license_key }} + CB_IN_ANTHROPIC_API_KEY: ${{ inputs.anthropic_api_key }} + CB_IN_AWS_API_KEY: ${{ inputs.aws_api_key }} + CB_IN_AWS_REGION: ${{ inputs.aws_region }} + CB_IN_CEREBRAS_API_KEY: ${{ inputs.cerebras_api_key }} + CB_IN_DEEPSEEK_API_KEY: ${{ inputs.deepseek_api_key }} + CB_IN_DEEPSEEK_BASE_URL: ${{ inputs.deepseek_base_url }} + CB_IN_GLM_API_KEY: ${{ inputs.glm_api_key }} + CB_IN_GLM_BASE_URL: ${{ inputs.glm_base_url }} + CB_IN_GOOGLE_API_KEY: ${{ inputs.google_api_key }} + CB_IN_KIMI_API_KEY: ${{ inputs.kimi_api_key }} + CB_IN_KIMI_BASE_URL: ${{ inputs.kimi_base_url }} + CB_IN_LITELLM_API_KEY: ${{ inputs.litellm_api_key }} + CB_IN_LITELLM_BASE_URL: ${{ inputs.litellm_base_url }} + CB_IN_OLLAMA_API_KEY: ${{ inputs.ollama_api_key }} + CB_IN_OLLAMA_BASE_URL: ${{ inputs.ollama_base_url }} + CB_IN_OPENAI_API_KEY: ${{ inputs.openai_api_key }} + CB_IN_OPENAI_BASE_URL: ${{ inputs.openai_base_url }} + CB_IN_OPENROUTER_API_KEY: ${{ inputs.openrouter_api_key }} + CB_IN_ORCAROUTER_API_KEY: ${{ inputs.orcarouter_api_key }} + CB_IN_VERCEL_API_KEY: ${{ inputs.vercel_api_key }} + CB_IN_VERCEL_BASE_URL: ${{ inputs.vercel_base_url }} + run: "$GITHUB_ACTION_PATH/scripts/action/preflight-llm.sh" + + # Said in the pull request, not only in the Actions tab, because the person who has + # to add the secret is the one reading the pull request. + - name: Report LLM configuration failure + if: steps.guard.outputs.skip != 'true' && steps.llm.outputs.error != '' && steps.guard.outputs.mode == 'review' && steps.guard.outputs.pr_number != '' + continue-on-error: true + uses: marocchino/sticky-pull-request-comment@v2 + with: + header: ${{ steps.guard.outputs.comment_id }} + number: ${{ steps.guard.outputs.pr_number }} + GITHUB_TOKEN: ${{ inputs.github_token }} + message: | + ### CodeBoarding review - not configured + + ${{ steps.llm.outputs.message }} + + No analysis ran, and no CodeBoarding hosted usage was consumed. + + run [${{ github.run_id }}](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}) - attempt ${{ github.run_attempt }} + + - name: Stop on LLM configuration failure + if: steps.guard.outputs.skip != 'true' && steps.llm.outputs.error != '' + shell: bash + env: + CB_ERROR: ${{ steps.llm.outputs.error }} + run: | + echo "CodeBoarding did not run: $CB_ERROR" >&2 + exit 1 + - name: Post review progress if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' continue-on-error: true @@ -188,9 +353,6 @@ runs: shell: bash env: ACTION_PATH: ${{ github.action_path }} - LLM_API_KEY: ${{ inputs.llm_api_key }} - LLM_PROVIDER: ${{ inputs.llm_provider }} - LICENSE_KEY: ${{ inputs.license_key }} run: "$GITHUB_ACTION_PATH/scripts/action/configure-auth.sh" - name: Resolve analysis identity @@ -203,7 +365,7 @@ runs: MERGE_BASE_SHA: ${{ steps.guard.outputs.merge_base_sha }} PR_NUMBER: ${{ steps.guard.outputs.pr_number }} IS_FORK: ${{ steps.guard.outputs.is_fork }} - LLM_PROVIDER: ${{ inputs.llm_provider }} + LLM_PROVIDER: ${{ steps.llm.outputs.provider }} MODEL: ${{ inputs.model }} AGENT_MODEL_INPUT: ${{ inputs.agent_model }} PARSING_MODEL_INPUT: ${{ inputs.parsing_model }} diff --git a/scripts/action/configure-auth.sh b/scripts/action/configure-auth.sh index 0a5beda..8b611db 100755 --- a/scripts/action/configure-auth.sh +++ b/scripts/action/configure-auth.sh @@ -1,44 +1,36 @@ #!/usr/bin/env bash -# Selects direct or hosted authentication and stores scoped credentials for analysis. +# Starts the hosted credential relay when the resolved plan calls for it. +# +# The plan itself was decided by preflight-llm.sh before the checkout; this step only +# does the part that needs the engine's Python present. A direct-provider run has +# nothing to do here: its key never leaves the runner. set -euo pipefail AUTH_DIR="${RUNNER_TEMP}/codeboarding-auth" HOSTED_PROXY_URL="https://auduihjmm4b735zci7vyabuikq0hppqn.lambda-url.us-east-1.on.aws" umask 077 -rm -rf "$AUTH_DIR" -mkdir -p "$AUTH_DIR" -provider="$(printf '%s' "${LLM_PROVIDER:-openrouter}" | tr '[:upper:]-' '[:lower:]_' | tr -cd 'a-z0-9_')" -[ -n "$provider" ] || { echo "::error::llm_provider is empty."; exit 1; } -printf '%s' "$provider" > "$AUTH_DIR/provider-name" -if [ -n "${LLM_API_KEY:-}" ] || [ "$provider" != openrouter ]; then - case "$provider" in - aws|aws_bedrock) provider_env="AWS_BEARER_TOKEN_BEDROCK" ;; - *) provider_env="$(printf '%s' "$provider" | tr '[:lower:]' '[:upper:]')_API_KEY" ;; - esac - printf '%s' "$provider_env" > "$AUTH_DIR/provider-env" - if [ -n "${LLM_API_KEY:-}" ]; then - key="$(printf '%s' "$LLM_API_KEY" | tr -d '[:space:]' | sed -e 's/^"//;s/"$//' -e "s/^'//;s/'\$//" -e "s/^${provider_env}=//" -e 's/^"//;s/"$//' -e "s/^'//;s/'\$//")" - printf '::add-mask::%s\n::add-mask::%s\n' "$LLM_API_KEY" "$key" - printf '%s' "$key" > "$AUTH_DIR/provider-key" - fi - echo "Using direct $provider credentials." - exit 0 + +if [ ! -s "$AUTH_DIR/tier" ]; then + echo "::error::CodeBoarding analysis credentials are unavailable; the configuration step did not run." + exit 1 fi +TIER="$(cat "$AUTH_DIR/tier")" + +case "$TIER" in + hosted|license) ;; + *) echo "Using direct $(cat "$AUTH_DIR/provider-name") credentials."; exit 0 ;; +esac + if [ -z "${ACTIONS_ID_TOKEN_REQUEST_URL:-}" ] || [ -z "${ACTIONS_ID_TOKEN_REQUEST_TOKEN:-}" ]; then echo "::error::Missing OIDC token. Add permissions: id-token: write." && exit 1 fi + READY="$AUTH_DIR/ready-port" PID="$AUTH_DIR/relay.pid" LOG="$AUTH_DIR/relay.log" -LICENSE_FILE="$AUTH_DIR/license.txt" -: > "$LICENSE_FILE" -if [ -n "${LICENSE_KEY:-}" ]; then - echo "::add-mask::$LICENSE_KEY" - printf '%s' "$LICENSE_KEY" > "$LICENSE_FILE" -fi RELAY_ARGS=(--upstream-base-url "$HOSTED_PROXY_URL" --ready-file "$READY") -if [ -s "$LICENSE_FILE" ]; then - RELAY_ARGS+=(--license-file "$LICENSE_FILE") +if [ -s "$AUTH_DIR/license.txt" ]; then + RELAY_ARGS+=(--license-file "$AUTH_DIR/license.txt") fi python3 "$ACTION_PATH/scripts/oidc_relay.py" "${RELAY_ARGS[@]}" > "$LOG" 2>&1 & @@ -59,6 +51,7 @@ if [ ! -s "$READY" ]; then fi PORT="$(cat "$READY")" -printf '%s' "OPENROUTER_API_KEY" > "$AUTH_DIR/provider-env" -printf '%s' "github-actions-oidc-relay" > "$AUTH_DIR/provider-key" -printf '%s' "http://127.0.0.1:$PORT" > "$AUTH_DIR/base-url" +mkdir -p "$AUTH_DIR/env" +printf '%s' "github-actions-oidc-relay" > "$AUTH_DIR/env/OPENROUTER_API_KEY" +printf '%s' "http://127.0.0.1:$PORT" > "$AUTH_DIR/env/OPENROUTER_BASE_URL" +echo "Using CodeBoarding hosted credentials ($TIER)." diff --git a/scripts/action/llm-providers.json b/scripts/action/llm-providers.json new file mode 100644 index 0000000..16abf53 --- /dev/null +++ b/scripts/action/llm-providers.json @@ -0,0 +1,155 @@ +{ + "_comment": [ + "The provider contract this action enforces, mirroring the LLM_PROVIDERS table of the", + "CodeBoarding release pinned in action.yml. tests/test_provider_table_drift.py installs", + "that release and fails if the two disagree, so bumping the pin without updating this", + "file cannot ship: a provider core added but this table lacks would otherwise be", + "unreachable, and one core removed would be accepted here and fail mid-analysis.", + "", + "'selection_envs' is the load-bearing field. Core selects a provider when ANY of them", + "is set, so 'at least one is non-empty' is exactly what makes a provider usable -- this", + "action does not invent a stricter or looser rule of its own. An api-key env that is not", + "a selection env (AWS_DEFAULT_REGION, OLLAMA_API_KEY) therefore cannot select a provider", + "on its own, which is why ollama and litellm need their base URL and not just a key." + ], + "engine": "0.13.10", + "hosted_provider": "openrouter", + "aliases": { + "aws_bedrock": "aws", + "bedrock": "aws", + "gemini": "google" + }, + "providers": { + "openrouter": { + "label": "OpenRouter", + "selection_envs": [ + "OPENROUTER_API_KEY" + ], + "inputs": { + "openrouter_api_key": "OPENROUTER_API_KEY" + } + }, + "orcarouter": { + "label": "OrcaRouter", + "selection_envs": [ + "ORCAROUTER_API_KEY" + ], + "inputs": { + "orcarouter_api_key": "ORCAROUTER_API_KEY" + } + }, + "anthropic": { + "label": "Anthropic", + "selection_envs": [ + "ANTHROPIC_API_KEY" + ], + "inputs": { + "anthropic_api_key": "ANTHROPIC_API_KEY" + } + }, + "openai": { + "label": "OpenAI", + "selection_envs": [ + "OPENAI_API_KEY", + "OPENAI_BASE_URL" + ], + "inputs": { + "openai_api_key": "OPENAI_API_KEY", + "openai_base_url": "OPENAI_BASE_URL" + } + }, + "google": { + "label": "Google Gemini", + "selection_envs": [ + "GOOGLE_API_KEY" + ], + "inputs": { + "google_api_key": "GOOGLE_API_KEY" + } + }, + "vercel": { + "label": "Vercel AI Gateway", + "selection_envs": [ + "VERCEL_API_KEY", + "VERCEL_BASE_URL" + ], + "inputs": { + "vercel_api_key": "VERCEL_API_KEY", + "vercel_base_url": "VERCEL_BASE_URL" + } + }, + "aws": { + "label": "AWS Bedrock", + "selection_envs": [ + "AWS_BEARER_TOKEN_BEDROCK" + ], + "inputs": { + "aws_api_key": "AWS_BEARER_TOKEN_BEDROCK", + "aws_region": "AWS_DEFAULT_REGION" + } + }, + "cerebras": { + "label": "Cerebras", + "selection_envs": [ + "CEREBRAS_API_KEY" + ], + "inputs": { + "cerebras_api_key": "CEREBRAS_API_KEY" + } + }, + "deepseek": { + "label": "DeepSeek", + "selection_envs": [ + "DEEPSEEK_API_KEY", + "DEEPSEEK_BASE_URL" + ], + "inputs": { + "deepseek_api_key": "DEEPSEEK_API_KEY", + "deepseek_base_url": "DEEPSEEK_BASE_URL" + } + }, + "glm": { + "label": "GLM", + "selection_envs": [ + "GLM_API_KEY", + "GLM_BASE_URL" + ], + "inputs": { + "glm_api_key": "GLM_API_KEY", + "glm_base_url": "GLM_BASE_URL" + } + }, + "kimi": { + "label": "Kimi", + "selection_envs": [ + "KIMI_API_KEY", + "KIMI_BASE_URL" + ], + "inputs": { + "kimi_api_key": "KIMI_API_KEY", + "kimi_base_url": "KIMI_BASE_URL" + } + }, + "ollama": { + "label": "Ollama", + "selection_envs": [ + "OLLAMA_BASE_URL", + "OLLAMA_HOST" + ], + "inputs": { + "ollama_api_key": "OLLAMA_API_KEY", + "ollama_base_url": "OLLAMA_BASE_URL" + } + }, + "litellm": { + "label": "LiteLLM", + "selection_envs": [ + "LITELLM_BASE_URL" + ], + "inputs": { + "litellm_api_key": "LITELLM_API_KEY", + "litellm_base_url": "LITELLM_BASE_URL" + } + } + } +} diff --git a/scripts/action/preflight-llm.sh b/scripts/action/preflight-llm.sh new file mode 100755 index 0000000..23fabd2 --- /dev/null +++ b/scripts/action/preflight-llm.sh @@ -0,0 +1,86 @@ +#!/usr/bin/env bash +# Resolves the run's LLM credentials before anything expensive happens. +# +# Runs ahead of the checkout and the engine install so a misconfigured workflow fails in +# seconds with the input to fix named, rather than after a minute of setup or, worse, +# succeeding quietly on credentials the workflow did not ask for. +set -euo pipefail + +AUTH_DIR="${RUNNER_TEMP}/codeboarding-auth" +umask 077 +rm -rf "$AUTH_DIR" + +# Mask before resolving: a value that never reaches the log cannot leak from a later +# failure. The cleaned forms are masked again below, since trimming a pasted wrapper +# produces a string GitHub has not been told about yet. +while IFS= read -r var; do + [ -z "${!var:-}" ] || echo "::add-mask::${!var}" +done < <(compgen -v | grep -E '^CB_IN_.*_API_KEY$' || true) +[ -z "${CB_IN_LICENSE_KEY:-}" ] || echo "::add-mask::${CB_IN_LICENSE_KEY}" + +set +e +plan="$(python3 "${ACTION_PATH}/scripts/action/resolve_llm.py" --auth-dir "$AUTH_DIR")" +resolved=$? +set -e + +field() { PLAN="$plan" python3 -c ' +import json, os, sys +print(json.loads(os.environ["PLAN"]).get(sys.argv[1], ""))' "$1"; } + +tier="$(field tier)" +provider="$(field provider)" +error="$(field error)" +message="$(field message)" + +{ + echo "tier=$tier" + echo "provider=$provider" + echo "error=$error" + echo "message<> "$GITHUB_OUTPUT" + +if [ "$resolved" -ne 0 ]; then + echo "::error title=CodeBoarding LLM configuration::${message}" + { + echo "### CodeBoarding could not start" + echo + echo "$message" + echo + echo "No analysis ran, and no CodeBoarding hosted usage was consumed." + } >> "${GITHUB_STEP_SUMMARY:-/dev/null}" + rm -rf "$AUTH_DIR" + exit 1 +fi + +# The trimmed values are what the analysis actually exports; mask those too. Endpoints +# and regions are configuration rather than credentials and stay readable, so a wrong +# one can be spotted in the log. +if [ -d "$AUTH_DIR/env" ]; then + for file in "$AUTH_DIR/env"/*; do + [ -e "$file" ] || continue + case "${file##*/}" in + *_BASE_URL|*_HOST|AWS_DEFAULT_REGION) continue ;; + esac + echo "::add-mask::$(cat "$file")" + done +fi +[ ! -s "$AUTH_DIR/license.txt" ] || echo "::add-mask::$(cat "$AUTH_DIR/license.txt")" + +case "$tier" in + hosted) label="CodeBoarding's hosted free tier" ;; + license) label="a CodeBoarding plan, on CodeBoarding's hosted tier" ;; + byok) label="your own ${provider} key" ;; + byok+license) label="your own ${provider} key, with a CodeBoarding plan" ;; + *) label="${tier}" ;; +esac +echo "CodeBoarding is running on ${label}." +{ + echo "### CodeBoarding configuration" + echo + echo "| | |" + echo "|---|---|" + echo "| Tier | \`${tier}\` |" + echo "| Provider | \`${provider}\` |" +} >> "${GITHUB_STEP_SUMMARY:-/dev/null}" diff --git a/scripts/action/resolve_llm.py b/scripts/action/resolve_llm.py new file mode 100755 index 0000000..ee0cbdf --- /dev/null +++ b/scripts/action/resolve_llm.py @@ -0,0 +1,256 @@ +#!/usr/bin/env python3 +"""Resolves the run's LLM credentials from the action's inputs, or explains why it cannot. + +One provider, chosen explicitly by the `llm` input, and nothing is ever reached by an +empty string falling through to a default. A misconfigured run fails here -- before the +checkout and the engine install -- naming the input and the secret to fix, rather than +succeeding on someone else's credentials or failing later inside the engine. + +Reads the action's inputs from CB_IN_* environment variables (prefixed so that wiring an +input can never itself set a provider selection variable), and writes the resolved +environment to an auth directory the analysis steps read. Prints a JSON summary that +carries no secret values. +""" + +from __future__ import annotations + +import argparse +import json +import os +import re +import sys +from pathlib import Path + +DOCS = "https://github.com/CodeBoarding/CodeBoarding-action#authentication-and-providers" +SETTINGS_HINT = "Settings -> Secrets and variables -> Actions" +TABLE = Path(__file__).resolve().parent / "llm-providers.json" + + +class ConfigError(Exception): + """A configuration the action refuses to run, with the code the webview keys on.""" + + def __init__(self, code: str, message: str) -> None: + super().__init__(message) + self.code = code + self.message = message + + +def load_table(path: Path = TABLE) -> dict: + return json.loads(path.read_text(encoding="utf-8")) + + +def _clean_key(raw: str) -> str: + """Undo the ways a pasted key arrives wrapped: whitespace, quotes, a VAR= prefix.""" + value = re.sub(r"\s+", "", raw) + for _ in range(2): + value = re.sub(r'^"(.*)"$', r"\1", value) + value = re.sub(r"^'(.*)'$", r"\1", value) + value = re.sub(r"^[A-Z0-9_]+=", "", value) + for _ in range(2): + value = re.sub(r'^"(.*)"$', r"\1", value) + value = re.sub(r"^'(.*)'$", r"\1", value) + return value + + +def read_inputs(table: dict, environ: dict[str, str]) -> dict[str, str]: + """Every provider input that carries a value, keyed by input name.""" + values: dict[str, str] = {} + for provider in table["providers"].values(): + for input_name in provider["inputs"]: + raw = environ.get(f"CB_IN_{input_name.upper()}", "") + value = _clean_key(raw) if input_name.endswith("_api_key") else raw.strip() + if value: + values[input_name] = value + return values + + +def owner_of(table: dict, input_name: str) -> str: + for name, provider in table["providers"].items(): + if input_name in provider["inputs"]: + return name + raise KeyError(input_name) + + +def _provider_list(table: dict) -> str: + return ", ".join(sorted(table["providers"])) + + +def _reject_provider_inputs(table: dict, given: dict[str, str], llm: str, tier: str) -> None: + """`llm: hosted`/`license` run on CodeBoarding's credentials; a provider key means the + workflow is asking for two different things at once.""" + if not given: + return + input_name = sorted(given)[0] + provider = owner_of(table, input_name) + raise ConfigError( + f"{tier}_with_provider_key", + f"`llm: {llm}` runs on CodeBoarding's hosted tier, but `{input_name}` is set. " + f"Remove it, or set `llm: {provider}` to run on that key instead.", + ) + + +def _require_id_token(llm: str, environ: dict[str, str]) -> None: + if environ.get("ACTIONS_ID_TOKEN_REQUEST_URL"): + return + raise ConfigError( + "missing_id_token", + f"`llm: {llm}` authenticates with a GitHub OIDC token, which this job cannot mint. " + "Add `permissions:` with `id-token: write` to the job that uses this action.", + ) + + +def _resolve_byok(table: dict, name: str, given: dict[str, str]) -> dict: + provider = table["providers"][name] + foreign = sorted(i for i in given if owner_of(table, i) != name) + if foreign: + other = owner_of(table, foreign[0]) + raise ConfigError( + "foreign_provider_key", + f"`llm: {name}` is selected, but `{foreign[0]}` is set, which configures " + f"`{other}`. Set only {provider['label']}'s inputs, or change `llm` to `{other}`.", + ) + + env = {var: given[i] for i, var in provider["inputs"].items() if i in given} + # Core selects a provider when one of its selection_envs is set. Anything else -- an + # API key that is not a selection variable, a region -- cannot make it usable, which + # is why a key alone does not configure ollama or litellm. + if not any(env.get(var) for var in provider["selection_envs"]): + wanted = [i for i, var in provider["inputs"].items() if var in provider["selection_envs"]] + keys = [i for i in wanted if i.endswith("_api_key")] + needed = " or ".join(f"`{i}`" for i in wanted) + # A base URL is configuration, not a credential; only send people to the secrets + # page for the inputs that actually belong there. + if keys: + secret = provider["inputs"][keys[0]] + fix = ( + f"Add the {secret} repository secret ({SETTINGS_HINT}) and wire it as " + f"`{keys[0]}: ${{{{ secrets.{secret} }}}}`." + ) + else: + fix = f"Set `{wanted[0]}` on the action step to your {provider['label']} endpoint." + raise ConfigError( + "missing_provider_key", + f"`llm: {name}` needs {needed}, and none is set. {fix}", + ) + return env + + +def resolve(table: dict, environ: dict[str, str]) -> dict: + """The whole contract. Returns a plan; raises ConfigError with the reason otherwise.""" + llm = re.sub(r"[\s-]+", "_", environ.get("CB_IN_LLM", "").strip().lower()) + license_key = environ.get("CB_IN_LICENSE_KEY", "").strip() + given = read_inputs(table, environ) + + if not llm: + raise ConfigError( + "missing_llm", + "The `llm` input is required and has no default. Set it to `hosted` " + "(CodeBoarding's free tier), `license` (a CodeBoarding plan), or one of: " + f"{_provider_list(table)}. See {DOCS}.", + ) + + if llm == "hosted": + _reject_provider_inputs(table, given, llm, "hosted") + if license_key: + raise ConfigError( + "hosted_with_license", + "`llm: hosted` is the free tier and never spends a licence. Set `llm: license` " + "to run your CodeBoarding plan, or remove `license_key`.", + ) + _require_id_token(llm, environ) + return {"tier": "hosted", "provider": table["hosted_provider"], "hosted": True, "env": {}} + + if llm == "license": + _reject_provider_inputs(table, given, llm, "license") + if not license_key: + raise ConfigError( + "missing_license_key", + "`llm: license` needs `license_key`, which is empty. Add the " + f"CODEBOARDING_LICENSE repository secret ({SETTINGS_HINT}) and wire it as " + "`license_key: ${{ secrets.CODEBOARDING_LICENSE }}`.", + ) + _require_id_token(llm, environ) + return { + "tier": "license", + "provider": table["hosted_provider"], + "hosted": True, + "license": license_key, + "env": {}, + } + + name = table["aliases"].get(llm, llm) + if name not in table["providers"]: + raise ConfigError( + "unknown_llm", + f"`llm: {environ.get('CB_IN_LLM', '').strip()}` is not a value this action " + f"understands. Use `hosted`, `license`, or one of: {_provider_list(table)}. " + f"See {DOCS}.", + ) + + env = _resolve_byok(table, name, given) + # A licence alongside a provider key is deliberately allowed, not an error: it says + # "my CodeBoarding plan, my own tokens". Nothing enforces it yet -- direct provider + # calls never reach our proxy -- so it is recorded for the surfaces that read it. + return { + "tier": "byok+license" if license_key else "byok", + "provider": name, + "hosted": False, + "env": env, + } + + +def write_auth_dir(table: dict, plan: dict, auth_dir: Path) -> None: + """Lay the plan out as files the later steps read, readable only by this user.""" + os.umask(0o077) + env_dir = auth_dir / "env" + env_dir.mkdir(parents=True, exist_ok=True) + (auth_dir / "tier").write_text(plan["tier"], encoding="utf-8") + (auth_dir / "provider-name").write_text(plan["provider"], encoding="utf-8") + for var, value in plan["env"].items(): + (env_dir / var).write_text(value, encoding="utf-8") + if plan.get("license"): + (auth_dir / "license.txt").write_text(plan["license"], encoding="utf-8") + # Every selection variable core knows about, so with-auth.sh can strip the ones this + # run did not ask for without carrying its own copy of the list to fall behind on. + keep = set(table["providers"][plan["provider"]]["selection_envs"]) + keep |= set(table["providers"][plan["provider"]]["inputs"].values()) + everything = { + var + for provider in table["providers"].values() + for var in list(provider["selection_envs"]) + list(provider["inputs"].values()) + } + (auth_dir / "foreign-envs").write_text("\n".join(sorted(everything - keep)), encoding="utf-8") + + +def main(argv: list[str]) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--auth-dir", type=Path, help="Write the resolved plan here.") + args = parser.parse_args(argv) + + table = load_table() + try: + plan = resolve(table, dict(os.environ)) + except ConfigError as error: + json.dump({"ok": False, "error": error.code, "message": error.message}, sys.stdout) + print() + return 1 + + if args.auth_dir: + write_auth_dir(table, plan, args.auth_dir) + json.dump( + { + "ok": True, + "error": "", + "message": "", + "tier": plan["tier"], + "provider": plan["provider"], + "hosted": plan["hosted"], + }, + sys.stdout, + ) + print() + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/scripts/action/with-auth.sh b/scripts/action/with-auth.sh index f1dc636..f09997f 100755 --- a/scripts/action/with-auth.sh +++ b/scripts/action/with-auth.sh @@ -1,10 +1,7 @@ #!/usr/bin/env bash -# Runs one command with scoped provider credentials, then removes those credentials. +# Runs one command with the resolved provider credentials, then removes them. set -euo pipefail AUTH_DIR="${RUNNER_TEMP}/codeboarding-auth" -PROVIDER_FILE="$AUTH_DIR/provider-name" -PROVIDER_ENV_FILE="$AUTH_DIR/provider-env" -PROVIDER_KEY_FILE="$AUTH_DIR/provider-key" cleanup() { if [ -s "$AUTH_DIR/relay.pid" ]; then kill "$(cat "$AUTH_DIR/relay.pid")" 2>/dev/null || true @@ -12,37 +9,27 @@ cleanup() { rm -rf "$AUTH_DIR" } trap cleanup EXIT -if [ ! -s "$PROVIDER_FILE" ] || [ ! -s "$PROVIDER_ENV_FILE" ]; then + +if [ ! -s "$AUTH_DIR/tier" ] || [ ! -s "$AUTH_DIR/provider-name" ]; then echo "::error::CodeBoarding analysis credentials are unavailable." exit 1 fi -PROVIDER="$(cat "$PROVIDER_FILE")" -case "$PROVIDER" in - ollama) [ -n "${OLLAMA_BASE_URL:-${OLLAMA_HOST:-}}" ] || { echo "::error::ollama requires OLLAMA_BASE_URL or OLLAMA_HOST." >&2; exit 1; } ;; - litellm) [ -n "${LITELLM_BASE_URL:-}" ] || { echo "::error::litellm requires LITELLM_BASE_URL." >&2; exit 1; } ;; -esac - -# Core selects a provider from its environment. Remove inherited selectors for -# every provider except the one explicitly requested by this action invocation. -selectors=(OPENAI_API_KEY OPENAI_BASE_URL VERCEL_API_KEY VERCEL_BASE_URL ANTHROPIC_API_KEY GOOGLE_API_KEY - AWS_BEARER_TOKEN_BEDROCK CEREBRAS_API_KEY OLLAMA_API_KEY OLLAMA_BASE_URL OLLAMA_HOST DEEPSEEK_API_KEY - DEEPSEEK_BASE_URL GLM_API_KEY GLM_BASE_URL KIMI_API_KEY KIMI_BASE_URL OPENROUTER_API_KEY LITELLM_API_KEY LITELLM_BASE_URL) -for selector in "${selectors[@]}"; do - case "$PROVIDER:$selector" in - openai:OPENAI_*|vercel:VERCEL_*|anthropic:ANTHROPIC_*|google:GOOGLE_*|aws:AWS_*|aws_bedrock:AWS_*|cerebras:CEREBRAS_*|ollama:OLLAMA_*|deepseek:DEEPSEEK_*|glm:GLM_*|kimi:KIMI_*|openrouter:OPENROUTER_*|litellm:LITELLM_*) ;; - *) unset "$selector" ;; - esac -done -PROVIDER_ENV="$(cat "$PROVIDER_ENV_FILE")" -if [ -s "$PROVIDER_KEY_FILE" ]; then - export "$PROVIDER_ENV=$(cat "$PROVIDER_KEY_FILE")" -fi -if [ -s "$AUTH_DIR/base-url" ]; then - OPENROUTER_BASE_URL="$(cat "$AUTH_DIR/base-url")" - export OPENROUTER_BASE_URL +# Core picks a provider from whatever its environment happens to hold, so a variable the +# caller exported for something else could select a provider this run never asked for. +# The list of what to strip is written by the resolver from the provider table, rather +# than kept here, so it cannot fall behind the pinned engine. +if [ -s "$AUTH_DIR/foreign-envs" ]; then + while IFS= read -r selector; do + [ -z "$selector" ] || unset "$selector" + done < "$AUTH_DIR/foreign-envs" fi +for file in "$AUTH_DIR/env"/*; do + [ -e "$file" ] || continue + export "${file##*/}=$(cat "$file")" +done + export CODEBOARDING_SOURCE=github_action unset ACTIONS_ID_TOKEN_REQUEST_URL ACTIONS_ID_TOKEN_REQUEST_TOKEN if [ -n "${MODEL:-}" ]; then diff --git a/tests/test_action_auth.py b/tests/test_action_auth.py index 1caecc7..9d8cf28 100644 --- a/tests/test_action_auth.py +++ b/tests/test_action_auth.py @@ -1,4 +1,4 @@ -"""Tests for the composite action's provider credential boundary.""" +"""Tests for the composite action's credential boundary, as the runner exercises it.""" from __future__ import annotations @@ -9,101 +9,168 @@ import unittest from pathlib import Path - ROOT = Path(__file__).resolve().parent.parent +PREFLIGHT = ROOT / "scripts" / "action" / "preflight-llm.sh" CONFIGURE_AUTH = ROOT / "scripts" / "action" / "configure-auth.sh" WITH_AUTH = ROOT / "scripts" / "action" / "with-auth.sh" class ActionAuthTests(unittest.TestCase): - def _configure(self, provider: str, key: str, **extra_env: str) -> tuple[subprocess.CompletedProcess, Path]: + def setUp(self) -> None: + self.temp_dir = tempfile.TemporaryDirectory() + self.addCleanup(self.temp_dir.cleanup) + + def _preflight(self, **inputs: str) -> tuple[subprocess.CompletedProcess, Path, dict[str, str]]: temp_dir = Path(self.temp_dir.name) runner_temp = temp_dir / "runner" - runner_temp.mkdir() - output = temp_dir / "output" - github_env = temp_dir / "github-env" + runner_temp.mkdir(exist_ok=True) + output = temp_dir / "github-output" + output.write_text("", encoding="utf-8") env = { "PATH": os.environ["PATH"], "ACTION_PATH": str(ROOT), - "GITHUB_ENV": str(github_env), "GITHUB_OUTPUT": str(output), - "LICENSE_KEY": "", - "LLM_API_KEY": key, - "LLM_PROVIDER": provider, + "GITHUB_STEP_SUMMARY": str(temp_dir / "summary.md"), "RUNNER_TEMP": str(runner_temp), - **extra_env, + **inputs, } - result = subprocess.run( - [str(CONFIGURE_AUTH)], + result = subprocess.run([str(PREFLIGHT)], env=env, capture_output=True, text=True, check=False) + return result, runner_temp / "codeboarding-auth", self._outputs(output) + + @staticmethod + def _outputs(path: Path) -> dict[str, str]: + """Parse the runner's key=value and key< subprocess.CompletedProcess: + runner_temp = Path(self.temp_dir.name) / "runner" + env = {"PATH": os.environ["PATH"], "RUNNER_TEMP": str(runner_temp), **extra_env} + return subprocess.run( + [str(WITH_AUTH), "bash", "-c", script], env=env, capture_output=True, text=True, check=False, ) - return result, runner_temp / "codeboarding-auth" - def setUp(self) -> None: - self.temp_dir = tempfile.TemporaryDirectory() + # -- the guarantee ----------------------------------------------------- - def tearDown(self) -> None: - self.temp_dir.cleanup() - - def test_maps_provider_keys_without_exporting_them_to_github_env(self) -> None: - cases = { - "openai": "OPENAI_API_KEY", - "vercel": "VERCEL_API_KEY", - "anthropic": "ANTHROPIC_API_KEY", - "google": "GOOGLE_API_KEY", - "aws": "AWS_BEARER_TOKEN_BEDROCK", - "aws_bedrock": "AWS_BEARER_TOKEN_BEDROCK", - "cerebras": "CEREBRAS_API_KEY", - "deepseek": "DEEPSEEK_API_KEY", - "glm": "GLM_API_KEY", - "kimi": "KIMI_API_KEY", - "openrouter": "OPENROUTER_API_KEY", - } - for provider, expected_env in cases.items(): - with self.subTest(provider=provider): - self.temp_dir.cleanup() - self.temp_dir = tempfile.TemporaryDirectory() - result, auth_dir = self._configure(provider, "fake-key") - self.assertEqual(result.returncode, 0, result.stderr or result.stdout) - self.assertEqual((auth_dir / "provider-env").read_text(), expected_env) - self.assertEqual((auth_dir / "provider-key").read_text(), "fake-key") - self.assertFalse((Path(self.temp_dir.name) / "github-env").exists()) - mode = stat.S_IMODE((auth_dir / "provider-key").stat().st_mode) - self.assertEqual(mode & 0o077, 0) - - def test_maps_standard_custom_provider_names_without_an_action_allowlist(self) -> None: - result, auth_dir = self._configure("acme-ai", "fake-key") + def test_a_named_provider_never_falls_back_to_codeboarding_credentials(self) -> None: + """PROTECTED TEST -- a workflow that names a provider runs on that provider or + not at all. + This action used to treat an empty key as "no preference" and resolve it to + CodeBoarding's hosted OpenRouter tier. A repository that asked for Anthropic and + had not added its secret yet therefore went green while running on a different + vendor, a different model, and CodeBoarding's money -- and nothing in the run + said so. Falling back is never the right answer to an unanswered question here: + the workflow named a provider, so the only honest outcomes are that provider or + a failure that says what is missing. + """ + result, auth_dir, outputs = self._preflight( + CB_IN_LLM="anthropic", + CB_IN_ANTHROPIC_API_KEY="", + ACTIONS_ID_TOKEN_REQUEST_URL="https://oidc.example/token", + ACTIONS_ID_TOKEN_REQUEST_TOKEN="request-token", + ) + + self.assertNotEqual(result.returncode, 0) + self.assertEqual(outputs["error"], "missing_provider_key") + self.assertFalse(auth_dir.exists(), "credentials were staged for a refused run") + self.assertNotIn("openrouter", result.stdout.lower()) + self.assertNotIn("OPENROUTER_API_KEY", result.stdout) + + # And the analysis cannot proceed on whatever the environment happened to hold. + scoped = self._with_auth("true", OPENROUTER_API_KEY="inherited") + self.assertNotEqual(scoped.returncode, 0) + + # -- resolution -------------------------------------------------------- + + def test_provider_key_is_staged_privately_and_exported_for_analysis(self) -> None: + result, auth_dir, outputs = self._preflight(CB_IN_LLM="anthropic", CB_IN_ANTHROPIC_API_KEY="fake=key") self.assertEqual(result.returncode, 0, result.stderr or result.stdout) - self.assertEqual((auth_dir / "provider-env").read_text(), "ACME_AI_API_KEY") + self.assertEqual(outputs["tier"], "byok") + self.assertEqual(outputs["provider"], "anthropic") + self.assertEqual(outputs["error"], "") - def test_normalizes_pasted_provider_key_wrappers(self) -> None: - result, auth_dir = self._configure("openrouter", " 'OPENROUTER_API_KEY=\"fake-key\"' \n") + key_file = auth_dir / "env" / "ANTHROPIC_API_KEY" + self.assertEqual(key_file.read_text(encoding="utf-8"), "fake=key") + self.assertEqual(stat.S_IMODE(key_file.stat().st_mode) & 0o077, 0) + self.assertIn("::add-mask::fake=key", result.stdout) + + scoped = self._with_auth( + 'test "$ANTHROPIC_API_KEY" = "fake=key" && test "$CODEBOARDING_SOURCE" = github_action' + ) + self.assertEqual(scoped.returncode, 0, scoped.stderr or scoped.stdout) + self.assertFalse(auth_dir.exists(), "credentials outlived the analysis command") + def test_selectors_for_other_providers_are_stripped_from_the_analysis(self) -> None: + """Core picks a provider from its environment, so an inherited key must not vote.""" + result, _, _ = self._preflight(CB_IN_LLM="anthropic", CB_IN_ANTHROPIC_API_KEY="k") self.assertEqual(result.returncode, 0, result.stderr or result.stdout) - self.assertEqual((auth_dir / "provider-key").read_text(), "fake-key") - def test_ollama_and_litellm_keys_use_core_environment_names(self) -> None: - for provider in ("ollama", "litellm"): - with self.subTest(provider=provider): - self.temp_dir.cleanup() - self.temp_dir = tempfile.TemporaryDirectory() - result, auth_dir = self._configure(provider, "fake-key") - self.assertEqual(result.returncode, 0, result.stderr or result.stdout) - self.assertEqual((auth_dir / "provider-env").read_text(), f"{provider.upper()}_API_KEY") - self.assertEqual((auth_dir / "provider-key").read_text(), "fake-key") + scoped = self._with_auth( + 'test -z "${OPENAI_API_KEY:-}" && test -z "${OPENAI_BASE_URL:-}" && ' + 'test -z "${OPENROUTER_API_KEY:-}" && test -z "${LITELLM_BASE_URL:-}" && ' + 'test -z "${ACTIONS_ID_TOKEN_REQUEST_URL:-}"', + OPENAI_API_KEY="inherited", + OPENAI_BASE_URL="https://inherited.example", + OPENROUTER_API_KEY="inherited", + LITELLM_BASE_URL="https://inherited.example", + ACTIONS_ID_TOKEN_REQUEST_URL="https://oidc.example/token", + ) + self.assertEqual(scoped.returncode, 0, scoped.stderr or scoped.stdout) - def test_keyless_direct_provider_does_not_fall_back_to_hosted_openrouter(self) -> None: - result, auth_dir = self._configure("ollama", "") + def test_model_inputs_keep_their_precedence(self) -> None: + self._preflight(CB_IN_LLM="anthropic", CB_IN_ANTHROPIC_API_KEY="k") + scoped = self._with_auth( + 'test "$AGENT_MODEL" = analysis-model && test "$PARSING_MODEL" = shared-model', + MODEL="shared-model", + AGENT_MODEL_INPUT="analysis-model", + PARSING_MODEL_INPUT="", + ) + self.assertEqual(scoped.returncode, 0, scoped.stderr or scoped.stdout) + + def test_refusal_reports_a_code_and_an_actionable_message(self) -> None: + result, _, outputs = self._preflight(CB_IN_LLM="hosted", CB_IN_OPENAI_API_KEY="k") + self.assertNotEqual(result.returncode, 0) + self.assertEqual(outputs["error"], "hosted_with_provider_key") + self.assertIn("openai_api_key", outputs["message"]) + self.assertIn("::error title=CodeBoarding LLM configuration::", result.stdout) + summary = (Path(self.temp_dir.name) / "summary.md").read_text(encoding="utf-8") + self.assertIn("CodeBoarding could not start", summary) + + def test_successful_run_reports_its_tier_and_provider(self) -> None: + """The webview reads this to show what a repository is actually running on.""" + result, _, outputs = self._preflight( + CB_IN_LLM="anthropic", CB_IN_ANTHROPIC_API_KEY="k", CB_IN_LICENSE_KEY="lic" + ) self.assertEqual(result.returncode, 0, result.stderr or result.stdout) - self.assertEqual((auth_dir / "provider-env").read_text(), "OLLAMA_API_KEY") - self.assertFalse((auth_dir / "provider-key").exists()) + self.assertEqual(outputs["tier"], "byok+license") + summary = (Path(self.temp_dir.name) / "summary.md").read_text(encoding="utf-8") + self.assertIn("byok+license", summary) + self.assertIn("anthropic", summary) - def test_hosted_auth_relays_to_aws_proxy_instead_of_openrouter(self) -> None: + # -- hosted tiers ------------------------------------------------------ + + def test_hosted_auth_relays_to_the_codeboarding_proxy(self) -> None: temp_dir = Path(self.temp_dir.name) fake_bin = temp_dir / "bin" fake_bin.mkdir() @@ -124,99 +191,65 @@ def test_hosted_auth_relays_to_aws_proxy_instead_of_openrouter(self) -> None: ) fake_python.chmod(0o755) - result, auth_dir = self._configure( - "openrouter", - "", + result, auth_dir, outputs = self._preflight( + CB_IN_LLM="license", + CB_IN_LICENSE_KEY="a-license", ACTIONS_ID_TOKEN_REQUEST_URL="https://oidc.example/token", ACTIONS_ID_TOKEN_REQUEST_TOKEN="request-token", - CAPTURED_ARGS=str(captured_args), - PATH=f"{fake_bin}:{os.environ['PATH']}", ) - self.assertEqual(result.returncode, 0, result.stderr or result.stdout) - args = captured_args.read_text(encoding="utf-8").splitlines() - upstream_index = args.index("--upstream-base-url") + 1 - self.assertEqual( - args[upstream_index], - "https://auduihjmm4b735zci7vyabuikq0hppqn.lambda-url.us-east-1.on.aws", + self.assertEqual(outputs["tier"], "license") + self.assertIn("::add-mask::a-license", result.stdout) + + configured = subprocess.run( + [str(CONFIGURE_AUTH)], + env={ + "PATH": f"{fake_bin}:{os.environ['PATH']}", + "ACTION_PATH": str(ROOT), + "RUNNER_TEMP": str(temp_dir / "runner"), + "CAPTURED_ARGS": str(captured_args), + "ACTIONS_ID_TOKEN_REQUEST_URL": "https://oidc.example/token", + "ACTIONS_ID_TOKEN_REQUEST_TOKEN": "request-token", + }, + capture_output=True, + text=True, + check=False, ) - self.assertNotIn("https://openrouter.ai/api/v1", args) - self.assertEqual((auth_dir / "provider-key").read_text(), "github-actions-oidc-relay") + self.assertEqual(configured.returncode, 0, configured.stderr or configured.stdout) + args = captured_args.read_text(encoding="utf-8").splitlines() + upstream = args[args.index("--upstream-base-url") + 1] + self.assertEqual(upstream, "https://auduihjmm4b735zci7vyabuikq0hppqn.lambda-url.us-east-1.on.aws") + self.assertIn("--license-file", args) + self.assertEqual((auth_dir / "env" / "OPENROUTER_API_KEY").read_text(), "github-actions-oidc-relay") + self.assertEqual((auth_dir / "env" / "OPENROUTER_BASE_URL").read_text(), "http://127.0.0.1:12345") - def test_failed_relay_start_removes_credentials_and_process(self) -> None: + def test_direct_provider_runs_start_no_relay(self) -> None: temp_dir = Path(self.temp_dir.name) fake_bin = temp_dir / "bin" fake_bin.mkdir() - relay_pid = temp_dir / "relay-pid" - (fake_bin / "python3").write_text( - '#!/usr/bin/env bash\nprintf \'%s\' "$$" > "$RELAY_PID"\nexec /bin/sleep 30\n', - encoding="utf-8", - ) - (fake_bin / "sleep").write_text("#!/usr/bin/env bash\n/bin/sleep 0.01\n", encoding="utf-8") - for path in (fake_bin / "python3", fake_bin / "sleep"): - path.chmod(0o755) - - result, auth_dir = self._configure( - "openrouter", - "", - ACTIONS_ID_TOKEN_REQUEST_URL="https://oidc.example/token", - ACTIONS_ID_TOKEN_REQUEST_TOKEN="request-token", - LICENSE_KEY="license", - RELAY_PID=str(relay_pid), - PATH=f"{fake_bin}:{os.environ['PATH']}", - ) - - self.assertNotEqual(result.returncode, 0) - self.assertFalse(auth_dir.exists()) - with self.assertRaises(ProcessLookupError): - os.kill(int(relay_pid.read_text(encoding="utf-8")), 0) - - def test_with_auth_scopes_credentials_source_and_model_precedence(self) -> None: - result, auth_dir = self._configure("anthropic", "fake=key") - self.assertEqual(result.returncode, 0, result.stderr or result.stdout) - command = [ - str(WITH_AUTH), - "bash", - "-c", - 'test "$ANTHROPIC_API_KEY" = "fake=key" && ' - 'test -z "${OPENAI_API_KEY:-}" && ' - 'test -z "${OPENAI_BASE_URL:-}" && ' - 'test -z "${OPENROUTER_API_KEY:-}" && ' - 'test -z "${ACTIONS_ID_TOKEN_REQUEST_URL:-}" && ' - 'test -z "${ACTIONS_ID_TOKEN_REQUEST_TOKEN:-}" && ' - 'test "$CODEBOARDING_SOURCE" = github_action && ' - 'test "$AGENT_MODEL" = analysis-model && ' - 'test "$PARSING_MODEL" = shared-model', - ] - env = { - "PATH": os.environ["PATH"], - "RUNNER_TEMP": str(auth_dir.parent), - "MODEL": "shared-model", - "AGENT_MODEL_INPUT": "analysis-model", - "PARSING_MODEL_INPUT": "", - "OPENAI_API_KEY": "inherited-key", - "OPENAI_BASE_URL": "https://inherited.example", - "ACTIONS_ID_TOKEN_REQUEST_URL": "https://oidc.example/token", - "ACTIONS_ID_TOKEN_REQUEST_TOKEN": "request-token", - } - scoped = subprocess.run(command, env=env, capture_output=True, text=True, check=False) - self.assertEqual(scoped.returncode, 0, scoped.stderr or scoped.stdout) - self.assertFalse(auth_dir.exists()) + marker = temp_dir / "relay-started" + (fake_bin / "python3").write_text(f'#!/usr/bin/env bash\ntouch "{marker}"\n', encoding="utf-8") + (fake_bin / "python3").chmod(0o755) - def test_keyless_provider_requires_its_endpoint(self) -> None: - result, auth_dir = self._configure("ollama", "") - self.assertEqual(result.returncode, 0, result.stderr or result.stdout) - - scoped = subprocess.run( - [str(WITH_AUTH), "true"], - env={"PATH": os.environ["PATH"], "RUNNER_TEMP": str(auth_dir.parent)}, + self._preflight(CB_IN_LLM="anthropic", CB_IN_ANTHROPIC_API_KEY="k") + configured = subprocess.run( + [str(CONFIGURE_AUTH)], + env={ + "PATH": f"{fake_bin}:{os.environ['PATH']}", + "ACTION_PATH": str(ROOT), + "RUNNER_TEMP": str(temp_dir / "runner"), + }, capture_output=True, text=True, check=False, ) + self.assertEqual(configured.returncode, 0, configured.stderr or configured.stdout) + self.assertFalse(marker.exists(), "a direct-provider run contacted the hosted relay") + def test_analysis_refuses_to_run_without_a_resolved_plan(self) -> None: + scoped = self._with_auth("true") self.assertNotEqual(scoped.returncode, 0) - self.assertIn("ollama requires OLLAMA_BASE_URL or OLLAMA_HOST", scoped.stderr) + self.assertIn("credentials are unavailable", scoped.stdout + scoped.stderr) if __name__ == "__main__": diff --git a/tests/test_action_inputs.py b/tests/test_action_inputs.py new file mode 100644 index 0000000..f9eca5b --- /dev/null +++ b/tests/test_action_inputs.py @@ -0,0 +1,85 @@ +"""action.yml must expose, and wire, exactly the contract the provider table describes. + +The resolver only ever sees what action.yml hands it. A provider in the table with no +input declared is unreachable; an input declared but not wired reads as empty and is +refused as "you did not set your key" when the user did. Both are silent, so they are +checked here rather than discovered in a repository. +""" + +from __future__ import annotations + +import json +import re +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parent.parent +ACTION = (ROOT / "action.yml").read_text(encoding="utf-8") +TABLE = json.loads((ROOT / "scripts" / "action" / "llm-providers.json").read_text(encoding="utf-8")) + + +def declared_inputs() -> dict[str, str]: + """Input name -> its declaration block, read from action.yml's inputs section.""" + section = ACTION[ACTION.index("\ninputs:\n") : ACTION.index("\noutputs:\n")] + blocks = re.split(r"\n {2}(?=[a-z0-9_]+:\n)", section) + found = {} + for block in blocks: + match = re.match(r"\s*([a-z0-9_]+):\n", block) + if match: + found[match.group(1)] = block + return found + + +def table_inputs() -> set[str]: + return {i for p in TABLE["providers"].values() for i in p["inputs"]} + + +class ActionInputTests(unittest.TestCase): + def setUp(self) -> None: + self.inputs = declared_inputs() + + def test_every_provider_input_is_declared(self) -> None: + missing = sorted(table_inputs() - set(self.inputs)) + self.assertEqual(missing, [], f"provider inputs missing from action.yml: {missing}") + + def test_every_provider_input_is_wired_to_the_resolver(self) -> None: + for name in sorted(table_inputs()): + with self.subTest(input=name): + self.assertIn( + f"CB_IN_{name.upper()}: ${{{{ inputs.{name} }}}}", + ACTION, + f"{name} is declared but never reaches the resolver", + ) + + def test_no_declared_provider_input_is_absent_from_the_table(self) -> None: + """An input the table does not own can never be read, so it would mislead.""" + suffixes = ("_api_key", "_base_url", "_region") + declared = {n for n in self.inputs if n.endswith(suffixes) and n not in {"license_key", "github_token"}} + self.assertEqual(sorted(declared - table_inputs()), []) + + def test_llm_is_required_and_has_no_default(self) -> None: + block = self.inputs["llm"] + self.assertIn("required: true", block) + self.assertNotIn("default:", block) + + def test_the_inferred_credential_inputs_are_gone(self) -> None: + """`llm_api_key`/`llm_provider` are what made a fallback expressible at all.""" + for stale in ("llm_api_key", "llm_provider"): + self.assertNotIn(stale, self.inputs) + self.assertNotIn(f"inputs.{stale}", ACTION) + + def test_credentials_resolve_before_the_checkout_and_the_engine_install(self) -> None: + """Fail-fast is positional: preflight is worth little after a minute of setup.""" + preflight = ACTION.index("- name: Check LLM configuration") + for later in ("- name: Checkout analysis target", "- name: Install CodeBoarding"): + self.assertLess(preflight, ACTION.index(later), f"{later} runs before preflight") + + def test_a_refused_run_reports_and_then_fails(self) -> None: + report = ACTION.index("- name: Report LLM configuration failure") + stop = ACTION.index("- name: Stop on LLM configuration failure") + self.assertLess(report, stop, "the run fails before it explains why") + self.assertIn("continue-on-error: true", ACTION[ACTION.index("id: llm") : report]) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_llm_contract.py b/tests/test_llm_contract.py new file mode 100644 index 0000000..6e4497f --- /dev/null +++ b/tests/test_llm_contract.py @@ -0,0 +1,156 @@ +"""The credential contract: exactly one explicitly named source, or a named failure. + +Every case here is a workflow someone could plausibly write. The point of the table is +that each one either resolves to precisely what it asked for, or is refused with a code +and a message naming the input to fix -- never quietly resolved to something else. +""" + +from __future__ import annotations + +import importlib.util +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parent.parent +_spec = importlib.util.spec_from_file_location("resolve_llm", ROOT / "scripts" / "action" / "resolve_llm.py") +resolve_llm = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(resolve_llm) + +OIDC = {"ACTIONS_ID_TOKEN_REQUEST_URL": "https://oidc.example/token"} + + +class ContractTests(unittest.TestCase): + def setUp(self) -> None: + self.table = resolve_llm.load_table() + + def resolve(self, **environ: str) -> dict: + return resolve_llm.resolve(self.table, environ) + + def refuse(self, **environ: str) -> resolve_llm.ConfigError: + with self.assertRaises(resolve_llm.ConfigError) as caught: + resolve_llm.resolve(self.table, environ) + return caught.exception + + # -- accepted shapes --------------------------------------------------- + + def test_every_provider_resolves_from_its_own_inputs(self) -> None: + """The table is the contract: each provider must be reachable through it.""" + for name, provider in self.table["providers"].items(): + with self.subTest(provider=name): + inputs = { + f"CB_IN_{i.upper()}": "value" + for i, var in provider["inputs"].items() + if var in provider["selection_envs"] + } + plan = self.resolve(CB_IN_LLM=name, **inputs) + self.assertEqual(plan["provider"], name) + self.assertEqual(plan["tier"], "byok") + self.assertTrue( + any(plan["env"].get(var) for var in provider["selection_envs"]), + f"{name} resolved without a variable core selects it by", + ) + + def test_hosted_and_license_are_named_not_inferred(self) -> None: + hosted = self.resolve(CB_IN_LLM="hosted", **OIDC) + self.assertEqual(hosted["tier"], "hosted") + self.assertEqual(hosted["env"], {}) + + licensed = self.resolve(CB_IN_LLM="license", CB_IN_LICENSE_KEY="lic", **OIDC) + self.assertEqual(licensed["tier"], "license") + self.assertEqual(licensed["license"], "lic") + + def test_licence_alongside_a_provider_key_is_recorded_not_refused(self) -> None: + """A CodeBoarding plan and your own tokens are two different questions.""" + plan = self.resolve(CB_IN_LLM="anthropic", CB_IN_ANTHROPIC_API_KEY="k", CB_IN_LICENSE_KEY="lic") + self.assertEqual(plan["tier"], "byok+license") + self.assertEqual(plan["provider"], "anthropic") + + def test_aliases_and_casing_resolve_to_the_canonical_provider(self) -> None: + for value in ("aws_bedrock", "AWS-Bedrock", " bedrock "): + with self.subTest(value=value): + plan = self.resolve(CB_IN_LLM=value, CB_IN_AWS_API_KEY="k") + self.assertEqual(plan["provider"], "aws") + + def test_pasted_key_wrappers_are_stripped(self) -> None: + plan = self.resolve(CB_IN_LLM="openrouter", CB_IN_OPENROUTER_API_KEY=" 'OPENROUTER_API_KEY=\"sk-x\"' \n") + self.assertEqual(plan["env"]["OPENROUTER_API_KEY"], "sk-x") + + def test_endpoint_only_providers_resolve_without_a_key(self) -> None: + for name, endpoint in (("ollama", "OLLAMA_BASE_URL"), ("litellm", "LITELLM_BASE_URL")): + with self.subTest(provider=name): + plan = self.resolve(CB_IN_LLM=name, **{f"CB_IN_{name.upper()}_BASE_URL": "http://host:1234"}) + self.assertEqual(plan["env"][endpoint], "http://host:1234") + + # -- refused shapes ---------------------------------------------------- + + def test_an_undeclared_workflow_is_refused_rather_than_defaulted(self) -> None: + error = self.refuse() + self.assertEqual(error.code, "missing_llm") + self.assertIn("required", error.message) + + def test_a_named_provider_without_its_key_names_the_input_and_the_secret(self) -> None: + error = self.refuse(CB_IN_LLM="anthropic") + self.assertEqual(error.code, "missing_provider_key") + self.assertIn("anthropic_api_key", error.message) + self.assertIn("ANTHROPIC_API_KEY", error.message) + + def test_a_key_alone_does_not_configure_an_endpoint_selected_provider(self) -> None: + """Core selects ollama by its endpoint, so a key alone leaves it unusable.""" + for name in ("ollama", "litellm"): + with self.subTest(provider=name): + error = self.refuse(CB_IN_LLM=name, **{f"CB_IN_{name.upper()}_API_KEY": "k"}) + self.assertEqual(error.code, "missing_provider_key") + self.assertIn(f"{name}_base_url", error.message) + self.assertNotIn("repository secret", error.message) + + def test_a_second_providers_key_is_refused_rather_than_ignored(self) -> None: + error = self.refuse(CB_IN_LLM="anthropic", CB_IN_ANTHROPIC_API_KEY="k", CB_IN_OPENAI_API_KEY="other") + self.assertEqual(error.code, "foreign_provider_key") + self.assertIn("openai_api_key", error.message) + + def test_hosted_refuses_to_share_a_workflow_with_a_provider_key(self) -> None: + error = self.refuse(CB_IN_LLM="hosted", CB_IN_ANTHROPIC_API_KEY="k", **OIDC) + self.assertEqual(error.code, "hosted_with_provider_key") + self.assertIn("llm: anthropic", error.message) + + def test_hosted_refuses_a_licence_it_would_not_spend(self) -> None: + error = self.refuse(CB_IN_LLM="hosted", CB_IN_LICENSE_KEY="lic", **OIDC) + self.assertEqual(error.code, "hosted_with_license") + self.assertIn("llm: license", error.message) + + 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") + self.assertIn("CODEBOARDING_LICENSE", error.message) + + def test_hosted_tiers_require_the_oidc_permission(self) -> None: + for value, extra in (("hosted", {}), ("license", {"CB_IN_LICENSE_KEY": "lic"})): + with self.subTest(llm=value): + error = self.refuse(CB_IN_LLM=value, **extra) + self.assertEqual(error.code, "missing_id_token") + self.assertIn("id-token: write", error.message) + + def test_an_unknown_provider_lists_the_ones_that_exist(self) -> None: + error = self.refuse(CB_IN_LLM="claude") + self.assertEqual(error.code, "unknown_llm") + self.assertIn("anthropic", error.message) + + def test_every_refusal_carries_a_code_and_an_actionable_message(self) -> None: + cases = [ + {}, + {"CB_IN_LLM": "nope"}, + {"CB_IN_LLM": "anthropic"}, + {"CB_IN_LLM": "hosted", "CB_IN_LICENSE_KEY": "l", **OIDC}, + {"CB_IN_LLM": "license", **OIDC}, + {"CB_IN_LLM": "hosted"}, + ] + for environ in cases: + with self.subTest(environ=environ): + error = self.refuse(**environ) + self.assertTrue(error.code and error.code.islower()) + self.assertTrue(error.message.endswith(".")) + self.assertGreater(len(error.message), 40) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_provider_table_drift.py b/tests/test_provider_table_drift.py new file mode 100644 index 0000000..5a4672c --- /dev/null +++ b/tests/test_provider_table_drift.py @@ -0,0 +1,87 @@ +"""The action's provider table must match the CodeBoarding release action.yml pins. + +The table exists because credentials have to be validated before the engine is installed, +which rules out asking the engine at run time. That copy is only safe while something +fails when it stops matching -- otherwise bumping the pin silently makes providers +unreachable (core added one, the table did not) or accepted here and broken later (core +removed one). This test is that something. It runs in the `core-compatibility` CI job, +which installs the pinned release; without the engine present it skips. +""" + +from __future__ import annotations + +import json +import re +import unittest +from pathlib import Path + +ROOT = Path(__file__).resolve().parent.parent +TABLE = json.loads((ROOT / "scripts" / "action" / "llm-providers.json").read_text(encoding="utf-8")) + +try: # pragma: no cover - availability is the point of the skip + from agents.llm_config import LLM_PROVIDERS as CORE_PROVIDERS +except Exception: # noqa: BLE001 - any import failure means the engine is not installed + CORE_PROVIDERS = None + + +@unittest.skipIf(CORE_PROVIDERS is None, "the pinned CodeBoarding release is not installed") +class ProviderTableDriftTests(unittest.TestCase): + def test_the_table_pins_the_release_action_yml_installs(self) -> None: + pinned = re.search(r"'codeboarding==([^']+)'", (ROOT / "action.yml").read_text()) + self.assertIsNotNone(pinned, "action.yml no longer pins a CodeBoarding release") + self.assertEqual( + TABLE["engine"], + pinned.group(1), + "llm-providers.json records a different release than action.yml installs", + ) + + def test_the_same_providers_exist_on_both_sides(self) -> None: + self.assertEqual( + sorted(TABLE["providers"]), + sorted(CORE_PROVIDERS), + "the action and the pinned engine disagree about which providers exist", + ) + + def test_selection_variables_match_core_exactly(self) -> None: + """Selection is the whole rule: 'configured' means core would select it.""" + for name, provider in TABLE["providers"].items(): + with self.subTest(provider=name): + self.assertEqual( + sorted(provider["selection_envs"]), + sorted(CORE_PROVIDERS[name].selection_envs), + f"{name}'s selection variables drifted from the engine", + ) + + def test_every_provider_key_reaches_the_variable_core_reads(self) -> None: + for name, provider in TABLE["providers"].items(): + api_key_env = CORE_PROVIDERS[name].api_key_env + if api_key_env is None: + continue # e.g. aws, whose SDK reads its own bearer token variable + with self.subTest(provider=name): + self.assertIn( + api_key_env, + provider["inputs"].values(), + f"{name} has no input that sets {api_key_env}", + ) + + def test_no_selection_variable_escapes_the_table(self) -> None: + """with-auth.sh strips foreign selectors using this table; a variable missing + from it is one core could still be selected by.""" + core = {var for c in CORE_PROVIDERS.values() for var in c.selection_envs} + known = { + var for p in TABLE["providers"].values() for var in list(p["selection_envs"]) + list(p["inputs"].values()) + } + self.assertEqual(sorted(core - known), []) + + def test_aliases_point_at_providers_that_exist(self) -> None: + for alias, target in TABLE["aliases"].items(): + with self.subTest(alias=alias): + self.assertIn(target, CORE_PROVIDERS) + self.assertNotIn(alias, CORE_PROVIDERS, f"{alias} is a real provider, not an alias") + + def test_the_hosted_tier_names_a_real_provider(self) -> None: + self.assertIn(TABLE["hosted_provider"], CORE_PROVIDERS) + + +if __name__ == "__main__": + unittest.main() From 4c333697258336f5b4d2eda5c74bd23ef1d14bc2 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Thu, 27 Aug 2026 16:34:28 +0200 Subject: [PATCH 2/6] fix(review): address review feedback and run our own repos on the licence Four things the Codex review was right about: - The credential check is a Python program and ran before `setup-python`. A hosted runner ships a system python3 and would never notice; a self-hosted one without it would fail a valid configuration. Python is provisioned first now, which still leaves the check ahead of the engine install. - `Post review failure` writes the same sticky comment on `failure()`, so a run stopped for a missing secret posted the fix and then buried it under "see the workflow logs". It now stands down when the credential check is what failed. - The drift test treated any `agents.llm_config` import error as "engine not installed" and skipped, so a pin that moved a module would retire the very check meant to catch it. Installed-ness is asked of the distribution; an import failure after that is raised. - The foreign-variable list spared every variable the selected provider could use rather than the ones the run resolved. `llm: openai` with only `openai_base_url` therefore let an inherited OPENAI_API_KEY credential the run: the same silent substitution this contract removes, one provider narrower. Also switches this repo's own workflows from `llm: hosted` to `llm: license`. CodeBoarding's repositories run on the CodeBoarding plan, and the secret is already configured here. The README examples stay on `hosted`, which is the right starting point for someone reading them. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/codeboarding-sync.yml | 4 +- .github/workflows/codeboarding.yml | 5 +- action.yml | 23 +- docs/github-storage-scopes.md | 412 ++++++++++++++++++++++++ scripts/action/resolve_llm.py | 14 +- tests/test_action_auth.py | 23 ++ tests/test_action_inputs.py | 19 ++ tests/test_provider_table_drift.py | 26 +- 8 files changed, 508 insertions(+), 18 deletions(-) create mode 100644 docs/github-storage-scopes.md diff --git a/.github/workflows/codeboarding-sync.yml b/.github/workflows/codeboarding-sync.yml index b72c6bc..2660515 100644 --- a/.github/workflows/codeboarding-sync.yml +++ b/.github/workflows/codeboarding-sync.yml @@ -141,7 +141,9 @@ jobs: - uses: ./ with: mode: sync - llm: hosted + # CodeBoarding's own repositories run on the CodeBoarding plan. + llm: license + license_key: ${{ secrets.CODEBOARDING_LICENSE }} sync_strategy: ${{ inputs.sync_strategy || 'push' }} force_full: ${{ inputs.force_full || false }} # App token authenticates the baseline push so the commit is attributed diff --git a/.github/workflows/codeboarding.yml b/.github/workflows/codeboarding.yml index 8c681eb..5b868b7 100644 --- a/.github/workflows/codeboarding.yml +++ b/.github/workflows/codeboarding.yml @@ -113,6 +113,7 @@ jobs: with: # Named, not inferred: the action refuses to guess where credentials come # from, so every workflow says which of hosted / license / a provider it - # wants. This repo dogfoods the hosted free tier. - llm: hosted + # wants. CodeBoarding's own repositories run on the CodeBoarding plan. + llm: license + license_key: ${{ secrets.CODEBOARDING_LICENSE }} github_token: ${{ steps.codeboarding-app-token-client.outputs.token || steps.codeboarding-app-token-app.outputs.token || github.token }} diff --git a/action.yml b/action.yml index 1a9a0d7..cd6c2fd 100644 --- a/action.yml +++ b/action.yml @@ -231,6 +231,17 @@ runs: COMMENT_ID: ${{ github.event.comment.id }} run: GH_HOST="${GITHUB_SERVER_URL#*://}" gh api -X POST "repos/${REPOSITORY}/issues/comments/${COMMENT_ID}/reactions" -f content=eyes >/dev/null + # Ahead of the credential check rather than after the checkout, because that check + # is a Python program. Hosted runners ship a system python3 and would not notice; + # a self-hosted runner without one would fail a perfectly valid configuration. It + # costs a couple of seconds and still leaves the check ahead of the engine install, + # which is the part worth failing before. + - name: Setup Python + if: steps.guard.outputs.skip != 'true' + uses: actions/setup-python@v5 + with: + python-version: '3.12' + # Resolved before the checkout and the engine install, so a workflow that names a # provider it has no key for fails within seconds, naming the input to fix, instead # of running to completion on credentials it never asked for. Nothing downstream @@ -321,12 +332,6 @@ runs: persist-credentials: false path: .codeboarding-target - - name: Setup Python - if: steps.guard.outputs.skip != 'true' - uses: actions/setup-python@v5 - with: - python-version: '3.12' - - name: Setup Java for CodeBoarding if: steps.guard.outputs.skip != 'true' uses: actions/setup-java@v5 @@ -611,8 +616,12 @@ runs: GITHUB_TOKEN: ${{ inputs.github_token }} path: ${{ steps.review_body.outputs.path }} + # Skipped when the run stopped on a credential problem: that path already replaced + # this same sticky comment with the input and secret to fix, and "see the workflow + # logs" posted over the top of it would send the reader hunting for what they had + # just been told. - name: Post review failure - if: failure() && steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.review_comment.outcome != 'success' + if: failure() && steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.review_comment.outcome != 'success' && steps.llm.outputs.error == '' continue-on-error: true uses: marocchino/sticky-pull-request-comment@v2 with: diff --git a/docs/github-storage-scopes.md b/docs/github-storage-scopes.md new file mode 100644 index 0000000..e7e91cb --- /dev/null +++ b/docs/github-storage-scopes.md @@ -0,0 +1,412 @@ +# GitHub Actions storage: what exists, who can reach it + +A working note on the three places a workflow can leave data, how GitHub scopes +each one, and what that means for CodeBoarding. Not committed — a draft to +argue with. + +The short version: **caches are scoped by git ref, artifacts are scoped by +workflow run, and git is scoped by branch permissions.** Almost every surprise +we hit came from assuming the cache behaved like the other two. + +--- + +## 1. The three stores + +| | Actions cache | Workflow artifact | Git (`.codeboarding/`) | +|---|---|---|---| +| **Unit** | a key, within a ref scope | a named file set, within a run | a commit | +| **Who can read** | runs in the same or a parent scope | any run with `actions: read`, plus humans | anyone with repo read | +| **Who can write** | trusted triggers only (§2) | any run, no special permission | anything with `contents: write` | +| **Lifetime** | 7 days idle, then LRU eviction at the limit | retention window (default 90 days) | forever | +| **Mutable** | no — a key is written once | no — finalized at upload | yes, it's a commit | +| **Quota** | **10 GB per repository** | account-wide, plan-dependent (§3.2) | repo size limits | +| **Billed** | only above a raised limit | GB-hours past the allowance (§3.2) | no | +| **Built for** | speeding up a run | publishing a result | durable state | + +The distinction that matters: a cache is addressed by **key within a ref**, an +artifact is addressed by **run id + name**. Nothing about an artifact is +branch-scoped, which is why they behave so differently. + +--- + +## 2. Caches: a tree of ref scopes + +Every cache entry belongs to the ref of the run that wrote it. Reads flow +**upward only** — a run sees its own scope and its ancestors, never siblings or +children. + +```mermaid +flowchart TD + main["refs/heads/main
default branch scope"] + feat["refs/heads/feature-x
branch scope"] + pr["refs/pull/42/merge
PR scope"] + other["refs/pull/43/merge
another PR"] + + main -->|readable by| feat + main -->|readable by| pr + main -->|readable by| other + + pr -.->|NOT readable by| main + pr -.->|NOT readable by| other +``` + +Stated as rules: + +- A run can restore caches from **its own ref** and from the **default branch**. +- A run triggered for a pull request can *also* restore from the **base branch**. +- A cache written by a pull request run lives in `refs/pull/N/merge` and **can + only be restored by re-runs of that same pull request** — not by the base + branch, not by other pull requests. + +So the default-branch scope is the only shared one. Anything written there is +visible everywhere; anything written in a PR scope is visible only to that PR. + +### 2.1 Writing is a separate question from reading + +Since **2026-06-26** ("read-only Actions cache for untrusted triggers"), being +able to read a scope no longer implies being able to write it. GitHub classifies +the *trigger*: + +| Trigger | Runs on ref | Can read | Can write | +|---|---|---|---| +| `push` | `refs/heads/` | that branch + default | ✅ that scope, default branch included | +| `workflow_dispatch` | ref you dispatch | that ref + default | ✅ | +| `schedule` | default branch | default | ✅ | +| `pull_request` | `refs/pull/N/merge` | own PR scope + base + default | ⚠️ **own PR scope only** — denied on the default-branch scope | +| `pull_request_target` | `refs/heads/` | base + default | ❓ probably nothing — its only scope *is* the default-branch one it is denied | +| `issue_comment` | default branch | default | ❌ **nothing** — same reason | + +A denied write surfaces as: + +``` +##[warning]Cache reservation failed: cache write denied: token has no writable scopes +Failed to save: Unable to reserve cache with key ..., another job may be creating this cache. +``` + +The second line is the cache action's generic fallback and is misleading; the +first line is the real reason. **No `permissions:` block changes this** — it is +enforced on the trigger, not on the token. Teams have confirmed that granting +`actions: write` makes no difference. + +### 2.2 Why GitHub did this + +A comment can be written by someone with less privilege than a pusher, on a pull +request whose head is a fork. If such a run could write the *default-branch* +scope, it could plant an entry that trusted workflows later restore — cache +poisoning across the trust boundary. PR runs keep write access to their own +isolated scope, because poisoning it only affects a pull request whose code the +attacker already controls. + +--- + +## 3. Artifacts: scoped by run, gated by permission + +An artifact belongs to the run that produced it. There is no ref scoping at all. + +```mermaid +flowchart LR + subgraph runs["Any run, any trigger"] + r1["run #1
artifact A"] + r2["run #2
artifact B"] + end + api[["Actions API
list + download"]] + reader["Any later run
with actions: read"] + human["Any human
with repo read"] + + r1 --> api + r2 --> api + api --> reader + api --> human +``` + +- **Writing** needs no special permission and works from **every** trigger, + `issue_comment` included. Our comment-triggered runs upload one on every call. +- **Reading another run's artifact** needs `actions: read` on the token, plus the + `run-id`. `actions/download-artifact` takes `run-id` + `github-token` for this. +- Artifacts expire on a retention window (90 days by default; a repo or org + policy can lower it and caps what a workflow may request). +- Immutable once uploaded, and names must be unique within a run. + +### 3.1 So yes — an artifact *can* be the previous analysis + +This is the asymmetry worth internalising: + +> A comment-triggered run **cannot write a cache**, but it **can write an +> artifact**. And any later run can read that artifact, from any ref. + +Which means a chain stored in artifacts would work on every path the cache +cannot: `/codeboarding`, the webview's refresh, forks, everything. + +Three costs come with it: + +1. **A permission change for consumers.** Reading needs `actions: read` in the + workflow's `permissions:` block. Today's recommended block does not have it. +2. **A smaller, shared quota — but charged by time, which we control.** See + §3.2: the allowance is account-wide and much smaller than the cache's 10 GB + per repository, so the nominal size is worse. But artifacts accrue in + **GB-hours**, so what costs is size × how long it lives, and both levers are + ours. `retention-days: 1` on a state artifact makes a 5 MB blob cost 0.12 + GB-hours; deleting it after the next run supersedes it costs less still. +3. **It rebuilds the hole GitHub closed.** An untrusted run can write an + artifact, so a trusted run reading one by name would be unpickling data an + attacker could have produced. Safe use means checking the *producing run* + first — its `event`, and whether `head_repository` is a fork — rather than + trusting a name. With caches, GitHub enforces that boundary for us. + +### 3.2 The actual limits + +| Plan | Minutes / month | Artifact storage | Cache | +|---|---|---|---| +| Free | 2,000 | 500 MB | 10 GB per repo | +| Pro | 3,000 | 1 GB | 10 GB per repo | +| Team | 3,000 | 2 GB | 10 GB per repo | +| Enterprise Cloud | 50,000 | 50 GB | 10 GB per repo | + +**Public repositories are free** on standard runners — minutes and storage both. +So everything below is about private repositories. + +Artifact storage is **account-wide** and shared with Packages; cache is **per +repository**. That is the real asymmetry, more than the raw numbers. + +**Charging is by GB-hours, not by peak.** Storage accrues hourly against actual +usage. Deleting an artifact makes current storage drop immediately and stops +future accrual — what is already accrued this cycle stays on the bill. Deleting +needs `actions: write`. + +Which means the quota is manageable rather than fixed: + +| Lever | Permission | Effect | +|---|---|---| +| `retention-days: 1` on the state artifact | none | a 5 MB blob costs ~0.12 GB-hours instead of ~0.84 over a week | +| delete the previous state after uploading a new one | `actions: write` | at most one live state artifact per open pull request | +| leave it at the 90-day default | none | every run's state bills for three months | + +Short retention gets most of the benefit with no permission cost, and behaves +like the cache's 7-day idle eviction: a pull request left alone longer than the +window falls back to the base analysis. + +### 3.3 Measured sizes, and how many runs fit + +Raw sizes from committed baselines, compressed sizes from real runs. Compression +matters: the state dir compresses about 8.5x, JSON graphs about 10x. + +"Raw state" below is the whole directory the engine needs — `analysis.json`, +`static_analysis.pkl`, `fingerprint.json`, `static_analysis.sha` — not the pickle +alone. At webview scale the pickle is 58% of it and the graph 41%. + +| Repo | Tracked files | `analysis.json` | `static_analysis.pkl` | Raw state | Cache entry | Review artifact | +|---|---|---|---|---|---|---| +| CodeBoarding-action | 24 | 32 KB | 30 KB | 64 KB | **18 KB** | **10 KB** | +| graph-viewer | 62 | 773 KB | 956 KB | 1.73 MB | — | — | +| CodeBoarding-evals | 92 | 1.24 MB | 1.63 MB | 2.88 MB | — | — | +| **CodeBoarding-webview** | **217** | **2.71 MB** | **3.82 MB** | **6.54 MB** | **0.77 MB** | **0.51 MB** | + +Everything below uses the webview numbers — the largest repo we have, so a +pessimistic case. A small repo is roughly 40x cheaper. + +**Model.** Included storage is an average-GB-over-the-month allowance, and usage +accrues hourly, so one run contributes `size × retention_hours / 730` to that +average. We assume **half** the allowance is available to us; the rest is +packages and other repositories. Public repositories are free and unaffected. + +| Plan | A — cache + 30-day artifact | B — artifacts, state kept 1 day | B — artifacts, state kept 30 days | +|---|---|---|---| +| Free | ~500 runs/mo | ~470 | ~170 | +| Pro | ~1,000 | ~935 | ~340 | +| Team | ~2,000 | ~1,870 | ~670 | +| Enterprise | ~50,000 | ~46,700 | ~16,800 | + +### 3.4 The base copy is half the artifact + +Every run's artifact carries `base_analysis.json`, and the merge base rarely +changes during a pull request — so ten runs store ten identical copies. Measured +at webview scale, with zip compressing each file independently (it does **not** +dedupe near-identical files): + +| Artifact contents | Size | +|---|---| +| head graph only | 0.241 MB | +| head + base | 0.482 MB | +| **cost of the base copy** | **0.241 MB — half of it** | + +Shipping the base once per merge base instead, as its own artifact named +`codeboarding-base-`, with the review artifact keeping only the +pointer it already has in `metadata.merge_base_sha`: + +| | per run | 500 runs/mo across 50 PRs | Team plan, half allowance | +|---|---|---|---| +| Today | 0.482 MB | 241 MB | ~2,000 runs/mo | +| Base shipped once per merge base | 0.265 MB | 133 MB | **~3,600 runs/mo** | + +**And it needs no new permission**, if the upload is gated on something the +action already knows: it uploads a base artifact only when it *computed* the +base, which is exactly when the base cache missed. On a warm pull request the +base is restored, not recomputed, so nothing is uploaded. That lands close to +"once per merge base" without having to list artifacts to check. + +The consumer looks the base up by name — `GET /actions/artifacts?name=…` — takes +the newest, and caches it by sha, so it fetches each base once no matter how +many times it refreshes. The fallback when no base artifact exists is what the +webview does today: read the committed baseline at that commit. + +Give the base artifact a longer retention than the review artifact. It is +uploaded rarely, so the extra retention is cheap, and it must not expire while a +review artifact still points at it. + +**Two things fall out of this.** + +*The cache limit is unreachable.* Half of 10 GB per repository holds ~6,500 live +chain entries at webview scale, and entries expire after 7 days idle. You would +need thousands of runs per week on one repository to feel it. Capacity is not +what constrains approach A — the 7-day idle window is. + +*Approach B costs the same as A, if the state artifact is short-lived.* At +1-day retention the two are within 6% of each other, because the 30-day review +artifact dominates both and a 1-day state artifact is nearly free. Left at the +90-day default, B is 3-4x worse. **Retention, not the choice of store, is what +decides the storage bill.** + +--- + +## 4. Everything in one table + +Read (R) and write (W) per trigger, per store: + +| Trigger | Cache: default scope | Cache: own PR scope | Artifacts | Git | +|---|---|---|---|---| +| `push` (sync) | R + **W** | — | R + W | W with `contents: write` | +| `pull_request` (same-repo) | R only | R + **W** | R with `actions: read`, W always | R | +| `pull_request` (fork) | R only | R + W | R + W | R | +| `issue_comment` | **R only** | not visible | R with `actions: read`, W always | R | +| `workflow_dispatch` | R + **W** | — | R + W | W with `contents: write` | + +The two cells that shaped our design: `issue_comment` cannot write any cache, +and it cannot even *see* a PR-scoped one. + +--- + +## 5. What this means for CodeBoarding today + +### 5.1 Which files live where + +The cache holds the engine's **working state**. The artifact holds the run's +**results**. They overlap on the graphs and differ on everything else: + +| File | Cache entry | Artifact | What it is for | +|---|---|---|---| +| `analysis.json` | ✅ | ✅ | the component graph — the diagram | +| `base_analysis.json` | ✅ *(as the base entry's own `analysis.json`)* | ✅ | what the head was compared against | +| `health_report.json` | ✅ | ✅ | warnings | +| `metadata.json` | ❌ | ✅ | which commits those graphs describe | +| `fingerprint.json` | ✅ | ❌ | whole-tree file hashes — **how a run detects what changed** | +| `static_analysis.pkl` | ✅ | ❌ | LSP/CFG cache and cluster baseline — **MB-scale**, the reason the state dir is big | +| `static_analysis.sha` | ✅ | ❌ | the warm-start gate for the pickle | +| `origin.json` | ✅ | ❌ | provenance: chain depth, and the base digest the chain grew from | + +**The rule that follows:** continuing an analysis needs `analysis.json`, +`fingerprint.json` and `static_analysis.pkl` **together**, in one directory. The +artifact carries the first and not the other two, so today's artifact can render +a diagram but cannot seed a run. Handed only `analysis.json`, the engine falls +back to a full analysis. + +That is the whole reason the cache exists in this design. It is not a faster copy +of the artifact; it is the only place the *inputs* to an incremental run live. + +Who can reach each of these is §4: the chain is written by that pull request's +own push-triggered runs and readable only by them, the base entry is written by +sync and readable everywhere, and the artifact is readable by anything with +`actions: read`. + +### 5.2 So could we read everything from artifacts instead? + +Yes — but only by moving `fingerprint.json`, `static_analysis.pkl` and +`static_analysis.sha` into an artifact too. That is exactly the "artifact chain" +row in §6, and the three costs in §3.1 apply: the shared 500 MB–2 GB quota rather +than 10 GB per repo, no automatic eviction, and an origin check we would have to +write ourselves before unpickling. + +If we did it, the state should be a **separate artifact** with short retention, +not folded into the review artifact — the review artifact is a consumer contract +the webview reads, and shipping a multi-megabyte pickle through it would make +every consumer download the engine's scratch space to draw one diagram. + +## 6. Two designs + +**A — caches (today).** Working state in the Actions cache, results in an +artifact. GitHub's scope and trust rules decide who participates. + +**B — artifacts for everything.** Put `fingerprint.json` and +`static_analysis.pkl` in a state artifact alongside the graphs. Artifacts are +addressable by name across runs, so this makes them a key-value store without +the ref scoping. + +### 6.1 What A optimises for + +The cache was picked to be a **well-behaved guest in someone else's +repository**, and that is still what it is best at: + +- **It is not the user's bill.** Cache is invisible and free to them; artifacts + land in their storage quota and their artifact list. This, not the raw + numbers, is why quota matters. +- **It cleans up after itself.** LRU plus 7-day idle eviction. Artifacts can be + bounded too — short retention, or deleting the superseded one — but that is a + policy we would own, and deleting costs `actions: write` on top of the + `actions: read` needed to fetch. +- **It needs no permission.** Artifacts need `actions: read` in every consumer + workflow, which some organisations restrict centrally. +- **GitHub enforces the untrusted-code boundary**, rather than us. +- **`restore-keys` is free machinery** — "newest entry matching this prefix" is + exactly what a chain needs. + +**Not a reason: speed.** Measured on a real run, a base restore moved 18,450 +bytes at 1.2 MB/s and finished in about 290 ms. Both stores are an HTTP transfer +of a compressed blob; the cache saves one API round trip and uses zstd rather +than zip. At MB scale that is a second or two against a 2–6 minute run. + +**Worth knowing for the discussion:** this trade-off was never actually +evaluated. The cache was chosen before the trust and scope rules were known, on +the strength of being free, needing no permissions, and giving a PR→base +fallback for nothing. That last property — native ref scoping — is the same +mechanism that now excludes `/codeboarding`. It is both the reason we chose it +and the reason it falls short. + +| | A — caches | B — artifacts | +|---|---|---| +| **Triggers that share incremental state** | pushes only. `/codeboarding` can neither read the chain nor write one; forks never | **all of them** — push, comment, dispatch, forks | +| **Needs `synchronize`** | yes, it is the only writer | no — optional, any trigger builds state | +| **Needs dispatch to make refresh useful** | yes | no | +| **Channels to reason about** | 3: cache default scope, cache PR scope, artifact | 1: artifact (+ git baseline for cold start) | +| **Rules to hold in your head** | ref scope tree, trust-by-trigger, eviction, immutability | name, retention, origin check | +| **Rough code** | `cache-keys.sh` + 5 workflow steps + fork namespace + digest/depth checks | lookup by name, download, origin check, upload — comparable size, fewer concepts | +| **Consumer workflow change** | add `synchronize` + `concurrency` | add `actions: read` | +| **Webview impact** | refresh is the most expensive path and never amortises; run→PR matched by title | refresh gets cheaper each time; keeps posting a comment, no dispatch needed | +| **Quota** | 10 GB per repo, self-evicting, unreachable in practice | shared account-wide, but within ~6% of A when the state artifact is kept a day (§3.3) | +| **Who enforces the trust boundary** | GitHub | **us** — verify the producing run before unpickling | +| **Ports to GitLab / others** | poorly — the scope and trust model is GitHub-specific | well — "publish a result, read the last one by name" exists everywhere | + +**Reading it.** B is better on every axis a user or a maintainer feels: one +channel instead of three, every trigger participates, no required trigger +changes, a cheaper webview, and it ports. A is better on the two axes that bite +later: storage headroom is 10 GB per repo against a shared 500 MB, and GitHub +enforces the untrusted-code boundary that B makes ours to get right. + +The pickle is what makes both of those hurt. It is the reason the state is +MB-scale rather than KB-scale, and the reason reading someone else's state is +dangerous rather than merely wrong. + +## Open questions + +1. Is `pull_request_target` genuinely unable to write any cache? If so we should + drop support for it rather than document a path that silently never caches. +2. If we go the artifact route, what is the minimum origin check before + unpickling — producing run's `event` and `head_repository`, or a signature? +3. Do we want one on-demand mechanism or two? Dispatch strictly dominates + `/codeboarding` on capability; `/codeboarding` wins purely on the UX of typing + it into a pull request. + +## Sources + +- GitHub docs, dependency caching reference — scope rules and the merge-ref restriction +- GitHub changelog 2026-06-26, "read-only Actions cache for untrusted triggers" +- `actions/download-artifact` v4 — `run-id` + `github-token` for cross-run reads +- Our own runs: `CodeBoarding-action` #82 and `CodeBoarding-webview` #77 diff --git a/scripts/action/resolve_llm.py b/scripts/action/resolve_llm.py index ee0cbdf..1aa431a 100755 --- a/scripts/action/resolve_llm.py +++ b/scripts/action/resolve_llm.py @@ -210,10 +210,16 @@ def write_auth_dir(table: dict, plan: dict, auth_dir: Path) -> None: (env_dir / var).write_text(value, encoding="utf-8") if plan.get("license"): (auth_dir / "license.txt").write_text(plan["license"], encoding="utf-8") - # Every selection variable core knows about, so with-auth.sh can strip the ones this - # run did not ask for without carrying its own copy of the list to fall behind on. - keep = set(table["providers"][plan["provider"]]["selection_envs"]) - keep |= set(table["providers"][plan["provider"]]["inputs"].values()) + # Every variable core knows about, minus the ones this run actually resolved, so + # with-auth.sh can strip the rest without carrying its own copy of the list to fall + # behind on. + # + # Keyed on what was RESOLVED, not on which provider was selected. Sparing every + # variable the selected provider could use would leave an inherited value in place + # for the ones it did not: `llm: openai` with only `openai_base_url` set would let a + # stray OPENAI_API_KEY from the job environment supply the credentials, which is the + # same silent substitution this contract exists to prevent, one provider narrower. + keep = set(plan["env"]) everything = { var for provider in table["providers"].values() diff --git a/tests/test_action_auth.py b/tests/test_action_auth.py index 9d8cf28..d87cb31 100644 --- a/tests/test_action_auth.py +++ b/tests/test_action_auth.py @@ -137,6 +137,29 @@ def test_selectors_for_other_providers_are_stripped_from_the_analysis(self) -> N ) self.assertEqual(scoped.returncode, 0, scoped.stderr or scoped.stdout) + def test_an_inherited_key_cannot_supply_a_provider_selected_by_its_endpoint(self) -> None: + """The same substitution as the protected test, one provider narrower. + + `llm: openai` with only an endpoint is a legitimate keyless configuration. Sparing + every variable OpenAI *could* use, rather than the ones this run actually resolved, + would leave a stray OPENAI_API_KEY from the job environment to credential the run. + """ + result, auth_dir, _ = self._preflight(CB_IN_LLM="openai", CB_IN_OPENAI_BASE_URL="https://proxy.example/v1") + self.assertEqual(result.returncode, 0, result.stderr or result.stdout) + self.assertIn("OPENAI_API_KEY", (auth_dir / "foreign-envs").read_text(encoding="utf-8")) + + scoped = self._with_auth( + 'test -z "${OPENAI_API_KEY:-}" && test "$OPENAI_BASE_URL" = https://proxy.example/v1', + OPENAI_API_KEY="inherited", + ) + self.assertEqual(scoped.returncode, 0, scoped.stderr or scoped.stdout) + + def test_a_provider_input_left_empty_is_stripped_not_inherited(self) -> None: + """`aws_region` unset means the engine's default, never whatever the job exported.""" + result, auth_dir, _ = self._preflight(CB_IN_LLM="aws", CB_IN_AWS_API_KEY="k") + self.assertEqual(result.returncode, 0, result.stderr or result.stdout) + self.assertIn("AWS_DEFAULT_REGION", (auth_dir / "foreign-envs").read_text(encoding="utf-8")) + def test_model_inputs_keep_their_precedence(self) -> None: self._preflight(CB_IN_LLM="anthropic", CB_IN_ANTHROPIC_API_KEY="k") scoped = self._with_auth( diff --git a/tests/test_action_inputs.py b/tests/test_action_inputs.py index f9eca5b..63ea061 100644 --- a/tests/test_action_inputs.py +++ b/tests/test_action_inputs.py @@ -74,6 +74,25 @@ def test_credentials_resolve_before_the_checkout_and_the_engine_install(self) -> for later in ("- name: Checkout analysis target", "- name: Install CodeBoarding"): self.assertLess(preflight, ACTION.index(later), f"{later} runs before preflight") + def test_the_generic_failure_comment_never_buries_the_actionable_one(self) -> None: + """Both write the same sticky comment, and the generic one runs on `failure()`. + + Without the guard, a run stopped for a missing secret posts the input and secret to + fix, then immediately replaces it with "see the workflow logs" -- sending the reader + to hunt for what they had just been told. + """ + start = ACTION.index("- name: Post review failure") + condition = ACTION[start : ACTION.index("message:", start)] + self.assertIn("steps.llm.outputs.error == ''", condition) + + def test_python_is_available_before_the_credential_check_runs(self) -> None: + """The check is a Python program, so a runner without a system python3 would fail a + configuration that is perfectly valid.""" + self.assertLess( + ACTION.index("- name: Setup Python"), + ACTION.index("- name: Check LLM configuration"), + ) + def test_a_refused_run_reports_and_then_fails(self) -> None: report = ACTION.index("- name: Report LLM configuration failure") stop = ACTION.index("- name: Stop on LLM configuration failure") diff --git a/tests/test_provider_table_drift.py b/tests/test_provider_table_drift.py index 5a4672c..e3b8c29 100644 --- a/tests/test_provider_table_drift.py +++ b/tests/test_provider_table_drift.py @@ -13,15 +13,33 @@ import json import re import unittest +from importlib import metadata from pathlib import Path ROOT = Path(__file__).resolve().parent.parent TABLE = json.loads((ROOT / "scripts" / "action" / "llm-providers.json").read_text(encoding="utf-8")) -try: # pragma: no cover - availability is the point of the skip - from agents.llm_config import LLM_PROVIDERS as CORE_PROVIDERS -except Exception: # noqa: BLE001 - any import failure means the engine is not installed - CORE_PROVIDERS = None + +def _core_providers(): + """The pinned engine's provider table, or None when the engine is not installed. + + The distinction matters more than it looks. Skipping when the package is ABSENT is + what lets the stdlib-only suite run anywhere. Skipping when it is PRESENT but fails + to import would quietly retire this whole file the moment a pin moved a module or + dropped a dependency, which is exactly the bump this test exists to catch. So the + installed-ness question is asked of the distribution, and any import failure after + that is raised rather than swallowed. + """ + try: + metadata.version("codeboarding") + except metadata.PackageNotFoundError: + return None + from agents.llm_config import LLM_PROVIDERS + + return LLM_PROVIDERS + + +CORE_PROVIDERS = _core_providers() @unittest.skipIf(CORE_PROVIDERS is None, "the pinned CodeBoarding release is not installed") From a14cd8dbd85b4cbf70b0490eb4e708977e4be0f4 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Thu, 27 Aug 2026 16:42:08 +0200 Subject: [PATCH 3/6] chore: drop an unrelated working note from this branch docs/github-storage-scopes.md is an uncommitted local draft about Actions storage scopes. It has nothing to do with the credential contract and was swept in by a `git add -A`; it belongs wherever its author decides, not here. Co-Authored-By: Claude Opus 5 (1M context) --- docs/github-storage-scopes.md | 412 ---------------------------------- 1 file changed, 412 deletions(-) delete mode 100644 docs/github-storage-scopes.md diff --git a/docs/github-storage-scopes.md b/docs/github-storage-scopes.md deleted file mode 100644 index e7e91cb..0000000 --- a/docs/github-storage-scopes.md +++ /dev/null @@ -1,412 +0,0 @@ -# GitHub Actions storage: what exists, who can reach it - -A working note on the three places a workflow can leave data, how GitHub scopes -each one, and what that means for CodeBoarding. Not committed — a draft to -argue with. - -The short version: **caches are scoped by git ref, artifacts are scoped by -workflow run, and git is scoped by branch permissions.** Almost every surprise -we hit came from assuming the cache behaved like the other two. - ---- - -## 1. The three stores - -| | Actions cache | Workflow artifact | Git (`.codeboarding/`) | -|---|---|---|---| -| **Unit** | a key, within a ref scope | a named file set, within a run | a commit | -| **Who can read** | runs in the same or a parent scope | any run with `actions: read`, plus humans | anyone with repo read | -| **Who can write** | trusted triggers only (§2) | any run, no special permission | anything with `contents: write` | -| **Lifetime** | 7 days idle, then LRU eviction at the limit | retention window (default 90 days) | forever | -| **Mutable** | no — a key is written once | no — finalized at upload | yes, it's a commit | -| **Quota** | **10 GB per repository** | account-wide, plan-dependent (§3.2) | repo size limits | -| **Billed** | only above a raised limit | GB-hours past the allowance (§3.2) | no | -| **Built for** | speeding up a run | publishing a result | durable state | - -The distinction that matters: a cache is addressed by **key within a ref**, an -artifact is addressed by **run id + name**. Nothing about an artifact is -branch-scoped, which is why they behave so differently. - ---- - -## 2. Caches: a tree of ref scopes - -Every cache entry belongs to the ref of the run that wrote it. Reads flow -**upward only** — a run sees its own scope and its ancestors, never siblings or -children. - -```mermaid -flowchart TD - main["refs/heads/main
default branch scope"] - feat["refs/heads/feature-x
branch scope"] - pr["refs/pull/42/merge
PR scope"] - other["refs/pull/43/merge
another PR"] - - main -->|readable by| feat - main -->|readable by| pr - main -->|readable by| other - - pr -.->|NOT readable by| main - pr -.->|NOT readable by| other -``` - -Stated as rules: - -- A run can restore caches from **its own ref** and from the **default branch**. -- A run triggered for a pull request can *also* restore from the **base branch**. -- A cache written by a pull request run lives in `refs/pull/N/merge` and **can - only be restored by re-runs of that same pull request** — not by the base - branch, not by other pull requests. - -So the default-branch scope is the only shared one. Anything written there is -visible everywhere; anything written in a PR scope is visible only to that PR. - -### 2.1 Writing is a separate question from reading - -Since **2026-06-26** ("read-only Actions cache for untrusted triggers"), being -able to read a scope no longer implies being able to write it. GitHub classifies -the *trigger*: - -| Trigger | Runs on ref | Can read | Can write | -|---|---|---|---| -| `push` | `refs/heads/` | that branch + default | ✅ that scope, default branch included | -| `workflow_dispatch` | ref you dispatch | that ref + default | ✅ | -| `schedule` | default branch | default | ✅ | -| `pull_request` | `refs/pull/N/merge` | own PR scope + base + default | ⚠️ **own PR scope only** — denied on the default-branch scope | -| `pull_request_target` | `refs/heads/` | base + default | ❓ probably nothing — its only scope *is* the default-branch one it is denied | -| `issue_comment` | default branch | default | ❌ **nothing** — same reason | - -A denied write surfaces as: - -``` -##[warning]Cache reservation failed: cache write denied: token has no writable scopes -Failed to save: Unable to reserve cache with key ..., another job may be creating this cache. -``` - -The second line is the cache action's generic fallback and is misleading; the -first line is the real reason. **No `permissions:` block changes this** — it is -enforced on the trigger, not on the token. Teams have confirmed that granting -`actions: write` makes no difference. - -### 2.2 Why GitHub did this - -A comment can be written by someone with less privilege than a pusher, on a pull -request whose head is a fork. If such a run could write the *default-branch* -scope, it could plant an entry that trusted workflows later restore — cache -poisoning across the trust boundary. PR runs keep write access to their own -isolated scope, because poisoning it only affects a pull request whose code the -attacker already controls. - ---- - -## 3. Artifacts: scoped by run, gated by permission - -An artifact belongs to the run that produced it. There is no ref scoping at all. - -```mermaid -flowchart LR - subgraph runs["Any run, any trigger"] - r1["run #1
artifact A"] - r2["run #2
artifact B"] - end - api[["Actions API
list + download"]] - reader["Any later run
with actions: read"] - human["Any human
with repo read"] - - r1 --> api - r2 --> api - api --> reader - api --> human -``` - -- **Writing** needs no special permission and works from **every** trigger, - `issue_comment` included. Our comment-triggered runs upload one on every call. -- **Reading another run's artifact** needs `actions: read` on the token, plus the - `run-id`. `actions/download-artifact` takes `run-id` + `github-token` for this. -- Artifacts expire on a retention window (90 days by default; a repo or org - policy can lower it and caps what a workflow may request). -- Immutable once uploaded, and names must be unique within a run. - -### 3.1 So yes — an artifact *can* be the previous analysis - -This is the asymmetry worth internalising: - -> A comment-triggered run **cannot write a cache**, but it **can write an -> artifact**. And any later run can read that artifact, from any ref. - -Which means a chain stored in artifacts would work on every path the cache -cannot: `/codeboarding`, the webview's refresh, forks, everything. - -Three costs come with it: - -1. **A permission change for consumers.** Reading needs `actions: read` in the - workflow's `permissions:` block. Today's recommended block does not have it. -2. **A smaller, shared quota — but charged by time, which we control.** See - §3.2: the allowance is account-wide and much smaller than the cache's 10 GB - per repository, so the nominal size is worse. But artifacts accrue in - **GB-hours**, so what costs is size × how long it lives, and both levers are - ours. `retention-days: 1` on a state artifact makes a 5 MB blob cost 0.12 - GB-hours; deleting it after the next run supersedes it costs less still. -3. **It rebuilds the hole GitHub closed.** An untrusted run can write an - artifact, so a trusted run reading one by name would be unpickling data an - attacker could have produced. Safe use means checking the *producing run* - first — its `event`, and whether `head_repository` is a fork — rather than - trusting a name. With caches, GitHub enforces that boundary for us. - -### 3.2 The actual limits - -| Plan | Minutes / month | Artifact storage | Cache | -|---|---|---|---| -| Free | 2,000 | 500 MB | 10 GB per repo | -| Pro | 3,000 | 1 GB | 10 GB per repo | -| Team | 3,000 | 2 GB | 10 GB per repo | -| Enterprise Cloud | 50,000 | 50 GB | 10 GB per repo | - -**Public repositories are free** on standard runners — minutes and storage both. -So everything below is about private repositories. - -Artifact storage is **account-wide** and shared with Packages; cache is **per -repository**. That is the real asymmetry, more than the raw numbers. - -**Charging is by GB-hours, not by peak.** Storage accrues hourly against actual -usage. Deleting an artifact makes current storage drop immediately and stops -future accrual — what is already accrued this cycle stays on the bill. Deleting -needs `actions: write`. - -Which means the quota is manageable rather than fixed: - -| Lever | Permission | Effect | -|---|---|---| -| `retention-days: 1` on the state artifact | none | a 5 MB blob costs ~0.12 GB-hours instead of ~0.84 over a week | -| delete the previous state after uploading a new one | `actions: write` | at most one live state artifact per open pull request | -| leave it at the 90-day default | none | every run's state bills for three months | - -Short retention gets most of the benefit with no permission cost, and behaves -like the cache's 7-day idle eviction: a pull request left alone longer than the -window falls back to the base analysis. - -### 3.3 Measured sizes, and how many runs fit - -Raw sizes from committed baselines, compressed sizes from real runs. Compression -matters: the state dir compresses about 8.5x, JSON graphs about 10x. - -"Raw state" below is the whole directory the engine needs — `analysis.json`, -`static_analysis.pkl`, `fingerprint.json`, `static_analysis.sha` — not the pickle -alone. At webview scale the pickle is 58% of it and the graph 41%. - -| Repo | Tracked files | `analysis.json` | `static_analysis.pkl` | Raw state | Cache entry | Review artifact | -|---|---|---|---|---|---|---| -| CodeBoarding-action | 24 | 32 KB | 30 KB | 64 KB | **18 KB** | **10 KB** | -| graph-viewer | 62 | 773 KB | 956 KB | 1.73 MB | — | — | -| CodeBoarding-evals | 92 | 1.24 MB | 1.63 MB | 2.88 MB | — | — | -| **CodeBoarding-webview** | **217** | **2.71 MB** | **3.82 MB** | **6.54 MB** | **0.77 MB** | **0.51 MB** | - -Everything below uses the webview numbers — the largest repo we have, so a -pessimistic case. A small repo is roughly 40x cheaper. - -**Model.** Included storage is an average-GB-over-the-month allowance, and usage -accrues hourly, so one run contributes `size × retention_hours / 730` to that -average. We assume **half** the allowance is available to us; the rest is -packages and other repositories. Public repositories are free and unaffected. - -| Plan | A — cache + 30-day artifact | B — artifacts, state kept 1 day | B — artifacts, state kept 30 days | -|---|---|---|---| -| Free | ~500 runs/mo | ~470 | ~170 | -| Pro | ~1,000 | ~935 | ~340 | -| Team | ~2,000 | ~1,870 | ~670 | -| Enterprise | ~50,000 | ~46,700 | ~16,800 | - -### 3.4 The base copy is half the artifact - -Every run's artifact carries `base_analysis.json`, and the merge base rarely -changes during a pull request — so ten runs store ten identical copies. Measured -at webview scale, with zip compressing each file independently (it does **not** -dedupe near-identical files): - -| Artifact contents | Size | -|---|---| -| head graph only | 0.241 MB | -| head + base | 0.482 MB | -| **cost of the base copy** | **0.241 MB — half of it** | - -Shipping the base once per merge base instead, as its own artifact named -`codeboarding-base-`, with the review artifact keeping only the -pointer it already has in `metadata.merge_base_sha`: - -| | per run | 500 runs/mo across 50 PRs | Team plan, half allowance | -|---|---|---|---| -| Today | 0.482 MB | 241 MB | ~2,000 runs/mo | -| Base shipped once per merge base | 0.265 MB | 133 MB | **~3,600 runs/mo** | - -**And it needs no new permission**, if the upload is gated on something the -action already knows: it uploads a base artifact only when it *computed* the -base, which is exactly when the base cache missed. On a warm pull request the -base is restored, not recomputed, so nothing is uploaded. That lands close to -"once per merge base" without having to list artifacts to check. - -The consumer looks the base up by name — `GET /actions/artifacts?name=…` — takes -the newest, and caches it by sha, so it fetches each base once no matter how -many times it refreshes. The fallback when no base artifact exists is what the -webview does today: read the committed baseline at that commit. - -Give the base artifact a longer retention than the review artifact. It is -uploaded rarely, so the extra retention is cheap, and it must not expire while a -review artifact still points at it. - -**Two things fall out of this.** - -*The cache limit is unreachable.* Half of 10 GB per repository holds ~6,500 live -chain entries at webview scale, and entries expire after 7 days idle. You would -need thousands of runs per week on one repository to feel it. Capacity is not -what constrains approach A — the 7-day idle window is. - -*Approach B costs the same as A, if the state artifact is short-lived.* At -1-day retention the two are within 6% of each other, because the 30-day review -artifact dominates both and a 1-day state artifact is nearly free. Left at the -90-day default, B is 3-4x worse. **Retention, not the choice of store, is what -decides the storage bill.** - ---- - -## 4. Everything in one table - -Read (R) and write (W) per trigger, per store: - -| Trigger | Cache: default scope | Cache: own PR scope | Artifacts | Git | -|---|---|---|---|---| -| `push` (sync) | R + **W** | — | R + W | W with `contents: write` | -| `pull_request` (same-repo) | R only | R + **W** | R with `actions: read`, W always | R | -| `pull_request` (fork) | R only | R + W | R + W | R | -| `issue_comment` | **R only** | not visible | R with `actions: read`, W always | R | -| `workflow_dispatch` | R + **W** | — | R + W | W with `contents: write` | - -The two cells that shaped our design: `issue_comment` cannot write any cache, -and it cannot even *see* a PR-scoped one. - ---- - -## 5. What this means for CodeBoarding today - -### 5.1 Which files live where - -The cache holds the engine's **working state**. The artifact holds the run's -**results**. They overlap on the graphs and differ on everything else: - -| File | Cache entry | Artifact | What it is for | -|---|---|---|---| -| `analysis.json` | ✅ | ✅ | the component graph — the diagram | -| `base_analysis.json` | ✅ *(as the base entry's own `analysis.json`)* | ✅ | what the head was compared against | -| `health_report.json` | ✅ | ✅ | warnings | -| `metadata.json` | ❌ | ✅ | which commits those graphs describe | -| `fingerprint.json` | ✅ | ❌ | whole-tree file hashes — **how a run detects what changed** | -| `static_analysis.pkl` | ✅ | ❌ | LSP/CFG cache and cluster baseline — **MB-scale**, the reason the state dir is big | -| `static_analysis.sha` | ✅ | ❌ | the warm-start gate for the pickle | -| `origin.json` | ✅ | ❌ | provenance: chain depth, and the base digest the chain grew from | - -**The rule that follows:** continuing an analysis needs `analysis.json`, -`fingerprint.json` and `static_analysis.pkl` **together**, in one directory. The -artifact carries the first and not the other two, so today's artifact can render -a diagram but cannot seed a run. Handed only `analysis.json`, the engine falls -back to a full analysis. - -That is the whole reason the cache exists in this design. It is not a faster copy -of the artifact; it is the only place the *inputs* to an incremental run live. - -Who can reach each of these is §4: the chain is written by that pull request's -own push-triggered runs and readable only by them, the base entry is written by -sync and readable everywhere, and the artifact is readable by anything with -`actions: read`. - -### 5.2 So could we read everything from artifacts instead? - -Yes — but only by moving `fingerprint.json`, `static_analysis.pkl` and -`static_analysis.sha` into an artifact too. That is exactly the "artifact chain" -row in §6, and the three costs in §3.1 apply: the shared 500 MB–2 GB quota rather -than 10 GB per repo, no automatic eviction, and an origin check we would have to -write ourselves before unpickling. - -If we did it, the state should be a **separate artifact** with short retention, -not folded into the review artifact — the review artifact is a consumer contract -the webview reads, and shipping a multi-megabyte pickle through it would make -every consumer download the engine's scratch space to draw one diagram. - -## 6. Two designs - -**A — caches (today).** Working state in the Actions cache, results in an -artifact. GitHub's scope and trust rules decide who participates. - -**B — artifacts for everything.** Put `fingerprint.json` and -`static_analysis.pkl` in a state artifact alongside the graphs. Artifacts are -addressable by name across runs, so this makes them a key-value store without -the ref scoping. - -### 6.1 What A optimises for - -The cache was picked to be a **well-behaved guest in someone else's -repository**, and that is still what it is best at: - -- **It is not the user's bill.** Cache is invisible and free to them; artifacts - land in their storage quota and their artifact list. This, not the raw - numbers, is why quota matters. -- **It cleans up after itself.** LRU plus 7-day idle eviction. Artifacts can be - bounded too — short retention, or deleting the superseded one — but that is a - policy we would own, and deleting costs `actions: write` on top of the - `actions: read` needed to fetch. -- **It needs no permission.** Artifacts need `actions: read` in every consumer - workflow, which some organisations restrict centrally. -- **GitHub enforces the untrusted-code boundary**, rather than us. -- **`restore-keys` is free machinery** — "newest entry matching this prefix" is - exactly what a chain needs. - -**Not a reason: speed.** Measured on a real run, a base restore moved 18,450 -bytes at 1.2 MB/s and finished in about 290 ms. Both stores are an HTTP transfer -of a compressed blob; the cache saves one API round trip and uses zstd rather -than zip. At MB scale that is a second or two against a 2–6 minute run. - -**Worth knowing for the discussion:** this trade-off was never actually -evaluated. The cache was chosen before the trust and scope rules were known, on -the strength of being free, needing no permissions, and giving a PR→base -fallback for nothing. That last property — native ref scoping — is the same -mechanism that now excludes `/codeboarding`. It is both the reason we chose it -and the reason it falls short. - -| | A — caches | B — artifacts | -|---|---|---| -| **Triggers that share incremental state** | pushes only. `/codeboarding` can neither read the chain nor write one; forks never | **all of them** — push, comment, dispatch, forks | -| **Needs `synchronize`** | yes, it is the only writer | no — optional, any trigger builds state | -| **Needs dispatch to make refresh useful** | yes | no | -| **Channels to reason about** | 3: cache default scope, cache PR scope, artifact | 1: artifact (+ git baseline for cold start) | -| **Rules to hold in your head** | ref scope tree, trust-by-trigger, eviction, immutability | name, retention, origin check | -| **Rough code** | `cache-keys.sh` + 5 workflow steps + fork namespace + digest/depth checks | lookup by name, download, origin check, upload — comparable size, fewer concepts | -| **Consumer workflow change** | add `synchronize` + `concurrency` | add `actions: read` | -| **Webview impact** | refresh is the most expensive path and never amortises; run→PR matched by title | refresh gets cheaper each time; keeps posting a comment, no dispatch needed | -| **Quota** | 10 GB per repo, self-evicting, unreachable in practice | shared account-wide, but within ~6% of A when the state artifact is kept a day (§3.3) | -| **Who enforces the trust boundary** | GitHub | **us** — verify the producing run before unpickling | -| **Ports to GitLab / others** | poorly — the scope and trust model is GitHub-specific | well — "publish a result, read the last one by name" exists everywhere | - -**Reading it.** B is better on every axis a user or a maintainer feels: one -channel instead of three, every trigger participates, no required trigger -changes, a cheaper webview, and it ports. A is better on the two axes that bite -later: storage headroom is 10 GB per repo against a shared 500 MB, and GitHub -enforces the untrusted-code boundary that B makes ours to get right. - -The pickle is what makes both of those hurt. It is the reason the state is -MB-scale rather than KB-scale, and the reason reading someone else's state is -dangerous rather than merely wrong. - -## Open questions - -1. Is `pull_request_target` genuinely unable to write any cache? If so we should - drop support for it rather than document a path that silently never caches. -2. If we go the artifact route, what is the minimum origin check before - unpickling — producing run's `event` and `head_repository`, or a signature? -3. Do we want one on-demand mechanism or two? Dispatch strictly dominates - `/codeboarding` on capability; `/codeboarding` wins purely on the UX of typing - it into a pull request. - -## Sources - -- GitHub docs, dependency caching reference — scope rules and the merge-ref restriction -- GitHub changelog 2026-06-26, "read-only Actions cache for untrusted triggers" -- `actions/download-artifact` v4 — `run-id` + `github-token` for cross-run reads -- Our own runs: `CodeBoarding-action` #82 and `CodeBoarding-webview` #77 From 2bad1921612fea22ad3457b04d139af2c6d33bc8 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Thu, 27 Aug 2026 16:55:57 +0200 Subject: [PATCH 4/6] refactor: one accepted spelling per provider, and drop code nothing reaches Aliases were three extra values to document, test and keep in step with the picker, in exchange for accepting a spelling nobody was asked to type. The table is now keyed by the ONE value `llm:` accepts, each provider's inputs are named after it (`llm: X` always pairs with `X_api_key`), and an unrecognised value is refused with the accepted list. Bedrock keeps the friendlier `aws_bedrock` rather than the engine's internal `aws`; a `core` field carries that translation and nothing else does, so the drift test still compares against the engine name exactly. Also removed: the `hosted` field the resolver emitted and nothing read, and the OIDC check in configure-auth.sh, which preflight had already made unreachable. One decision point was the point. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 10 ++++++---- action.yml | 16 ++++++++-------- scripts/action/configure-auth.sh | 4 ---- scripts/action/llm-providers.json | 18 ++++++++++-------- scripts/action/resolve_llm.py | 9 +++------ tests/test_action_auth.py | 4 ++-- tests/test_llm_contract.py | 28 ++++++++++++++++++++++++---- tests/test_provider_table_drift.py | 24 +++++++++++++++--------- 8 files changed, 68 insertions(+), 45 deletions(-) diff --git a/README.md b/README.md index c91d16e..dda4685 100644 --- a/README.md +++ b/README.md @@ -154,7 +154,7 @@ without knowing any precedence rules. | `llm` | Provider | Inputs | Needs at least one of | |---|---|---|---| | `anthropic` | Anthropic | `anthropic_api_key` | `anthropic_api_key` | -| `aws` | AWS Bedrock | `aws_api_key`, `aws_region` | `aws_api_key` | +| `aws_bedrock` | AWS Bedrock | `aws_bedrock_api_key`, `aws_bedrock_region` | `aws_bedrock_api_key` | | `cerebras` | Cerebras | `cerebras_api_key` | `cerebras_api_key` | | `deepseek` | DeepSeek | `deepseek_api_key`, `deepseek_base_url` | `deepseek_api_key` or `deepseek_base_url` | | `glm` | GLM | `glm_api_key`, `glm_base_url` | `glm_api_key` or `glm_base_url` | @@ -167,9 +167,11 @@ without knowing any precedence rules. | `orcarouter` | OrcaRouter | `orcarouter_api_key` | `orcarouter_api_key` | | `vercel` | Vercel AI Gateway | `vercel_api_key`, `vercel_base_url` | `vercel_api_key` or `vercel_base_url` | -`aws_bedrock`, `bedrock` and `gemini` are accepted as aliases. `ollama` and `litellm` are -selected by their endpoint rather than a key, which is why a key alone does not configure -them — that mirrors how Core itself decides. +Each provider has exactly one accepted spelling, and its inputs are named after it, so +`llm: X` always pairs with `X_api_key`. There are no aliases: a second spelling is another +thing to document and keep in step, and an unrecognised value is refused with the accepted +list. `ollama` and `litellm` are selected by their endpoint rather than a key, which is why +a key alone does not configure them — that mirrors how Core itself decides. This table is generated from [`scripts/action/llm-providers.json`](scripts/action/llm-providers.json), which mirrors the CodeBoarding release this action pins. `tests/test_provider_table_drift.py` diff --git a/action.yml b/action.yml index cd6c2fd..0b6b4cd 100644 --- a/action.yml +++ b/action.yml @@ -10,7 +10,7 @@ inputs: required: false default: 'review' llm: - description: 'Required. Where analysis credentials come from: hosted, license, or a provider name (anthropic, aws, cerebras, deepseek, glm, google, kimi, litellm, ollama, openai, openrouter, orcarouter, vercel).' + description: 'Required. Where analysis credentials come from: hosted, license, or a provider name (anthropic, aws_bedrock, cerebras, deepseek, glm, google, kimi, litellm, ollama, openai, openrouter, orcarouter, vercel).' required: true license_key: description: 'CodeBoarding license key. Required by llm: license; optional alongside a provider key.' @@ -21,13 +21,13 @@ inputs: description: 'Anthropic API key, used when llm is anthropic. Sets ANTHROPIC_API_KEY.' required: false default: '' - # AWS Bedrock - selected by llm: aws - aws_api_key: - description: 'AWS Bedrock API key, used when llm is aws. Sets AWS_BEARER_TOKEN_BEDROCK.' + # AWS Bedrock - selected by llm: aws_bedrock + aws_bedrock_api_key: + description: 'AWS Bedrock API key, used when llm is aws_bedrock. Sets AWS_BEARER_TOKEN_BEDROCK.' required: false default: '' - aws_region: - description: 'AWS Bedrock region, used when llm is aws. Sets AWS_DEFAULT_REGION.' + aws_bedrock_region: + description: 'AWS Bedrock region, used when llm is aws_bedrock. Sets AWS_DEFAULT_REGION.' required: false default: '' # Cerebras - selected by llm: cerebras @@ -256,8 +256,8 @@ runs: CB_IN_LLM: ${{ inputs.llm }} CB_IN_LICENSE_KEY: ${{ inputs.license_key }} CB_IN_ANTHROPIC_API_KEY: ${{ inputs.anthropic_api_key }} - CB_IN_AWS_API_KEY: ${{ inputs.aws_api_key }} - CB_IN_AWS_REGION: ${{ inputs.aws_region }} + CB_IN_AWS_BEDROCK_API_KEY: ${{ inputs.aws_bedrock_api_key }} + CB_IN_AWS_BEDROCK_REGION: ${{ inputs.aws_bedrock_region }} CB_IN_CEREBRAS_API_KEY: ${{ inputs.cerebras_api_key }} CB_IN_DEEPSEEK_API_KEY: ${{ inputs.deepseek_api_key }} CB_IN_DEEPSEEK_BASE_URL: ${{ inputs.deepseek_base_url }} diff --git a/scripts/action/configure-auth.sh b/scripts/action/configure-auth.sh index 8b611db..6f0b09b 100755 --- a/scripts/action/configure-auth.sh +++ b/scripts/action/configure-auth.sh @@ -20,10 +20,6 @@ case "$TIER" in *) echo "Using direct $(cat "$AUTH_DIR/provider-name") credentials."; exit 0 ;; esac -if [ -z "${ACTIONS_ID_TOKEN_REQUEST_URL:-}" ] || [ -z "${ACTIONS_ID_TOKEN_REQUEST_TOKEN:-}" ]; then - echo "::error::Missing OIDC token. Add permissions: id-token: write." && exit 1 -fi - READY="$AUTH_DIR/ready-port" PID="$AUTH_DIR/relay.pid" LOG="$AUTH_DIR/relay.log" diff --git a/scripts/action/llm-providers.json b/scripts/action/llm-providers.json index 16abf53..213cebd 100644 --- a/scripts/action/llm-providers.json +++ b/scripts/action/llm-providers.json @@ -6,6 +6,12 @@ "file cannot ship: a provider core added but this table lacks would otherwise be", "unreachable, and one core removed would be accepted here and fail mid-analysis.", "", + "Each key is the ONE value `llm:` accepts for that provider, and each provider's inputs", + "are named after it, so `llm: X` always pairs with `X_api_key`. There are no aliases:", + "a second spelling is a second thing to document, test and keep in step, and the error", + "message lists the accepted values anyway. Where core's internal name differs from the", + "one we ask people to type, `core` carries the translation and nothing else does.", + "", "'selection_envs' is the load-bearing field. Core selects a provider when ANY of them", "is set, so 'at least one is non-empty' is exactly what makes a provider usable -- this", "action does not invent a stricter or looser rule of its own. An api-key env that is not", @@ -14,11 +20,6 @@ ], "engine": "0.13.10", "hosted_provider": "openrouter", - "aliases": { - "aws_bedrock": "aws", - "bedrock": "aws", - "gemini": "google" - }, "providers": { "openrouter": { "label": "OpenRouter", @@ -78,14 +79,15 @@ "vercel_base_url": "VERCEL_BASE_URL" } }, - "aws": { + "aws_bedrock": { + "core": "aws", "label": "AWS Bedrock", "selection_envs": [ "AWS_BEARER_TOKEN_BEDROCK" ], "inputs": { - "aws_api_key": "AWS_BEARER_TOKEN_BEDROCK", - "aws_region": "AWS_DEFAULT_REGION" + "aws_bedrock_api_key": "AWS_BEARER_TOKEN_BEDROCK", + "aws_bedrock_region": "AWS_DEFAULT_REGION" } }, "cerebras": { diff --git a/scripts/action/resolve_llm.py b/scripts/action/resolve_llm.py index 1aa431a..e8a4abb 100755 --- a/scripts/action/resolve_llm.py +++ b/scripts/action/resolve_llm.py @@ -137,7 +137,7 @@ def _resolve_byok(table: dict, name: str, given: dict[str, str]) -> dict: def resolve(table: dict, environ: dict[str, str]) -> dict: """The whole contract. Returns a plan; raises ConfigError with the reason otherwise.""" - llm = re.sub(r"[\s-]+", "_", environ.get("CB_IN_LLM", "").strip().lower()) + llm = environ.get("CB_IN_LLM", "").strip().lower() license_key = environ.get("CB_IN_LICENSE_KEY", "").strip() given = read_inputs(table, environ) @@ -158,7 +158,7 @@ def resolve(table: dict, environ: dict[str, str]) -> dict: "to run your CodeBoarding plan, or remove `license_key`.", ) _require_id_token(llm, environ) - return {"tier": "hosted", "provider": table["hosted_provider"], "hosted": True, "env": {}} + return {"tier": "hosted", "provider": table["hosted_provider"], "env": {}} if llm == "license": _reject_provider_inputs(table, given, llm, "license") @@ -173,12 +173,11 @@ def resolve(table: dict, environ: dict[str, str]) -> dict: return { "tier": "license", "provider": table["hosted_provider"], - "hosted": True, "license": license_key, "env": {}, } - name = table["aliases"].get(llm, llm) + name = llm if name not in table["providers"]: raise ConfigError( "unknown_llm", @@ -194,7 +193,6 @@ def resolve(table: dict, environ: dict[str, str]) -> dict: return { "tier": "byok+license" if license_key else "byok", "provider": name, - "hosted": False, "env": env, } @@ -250,7 +248,6 @@ def main(argv: list[str]) -> int: "message": "", "tier": plan["tier"], "provider": plan["provider"], - "hosted": plan["hosted"], }, sys.stdout, ) diff --git a/tests/test_action_auth.py b/tests/test_action_auth.py index d87cb31..3c2d89f 100644 --- a/tests/test_action_auth.py +++ b/tests/test_action_auth.py @@ -155,8 +155,8 @@ def test_an_inherited_key_cannot_supply_a_provider_selected_by_its_endpoint(self self.assertEqual(scoped.returncode, 0, scoped.stderr or scoped.stdout) def test_a_provider_input_left_empty_is_stripped_not_inherited(self) -> None: - """`aws_region` unset means the engine's default, never whatever the job exported.""" - result, auth_dir, _ = self._preflight(CB_IN_LLM="aws", CB_IN_AWS_API_KEY="k") + """An unset region means the engine's default, never whatever the job exported.""" + result, auth_dir, _ = self._preflight(CB_IN_LLM="aws_bedrock", CB_IN_AWS_BEDROCK_API_KEY="k") self.assertEqual(result.returncode, 0, result.stderr or result.stdout) self.assertIn("AWS_DEFAULT_REGION", (auth_dir / "foreign-envs").read_text(encoding="utf-8")) diff --git a/tests/test_llm_contract.py b/tests/test_llm_contract.py index 6e4497f..18eeaaa 100644 --- a/tests/test_llm_contract.py +++ b/tests/test_llm_contract.py @@ -65,11 +65,31 @@ def test_licence_alongside_a_provider_key_is_recorded_not_refused(self) -> None: self.assertEqual(plan["tier"], "byok+license") self.assertEqual(plan["provider"], "anthropic") - def test_aliases_and_casing_resolve_to_the_canonical_provider(self) -> None: - for value in ("aws_bedrock", "AWS-Bedrock", " bedrock "): + def test_one_spelling_per_provider_and_nothing_else(self) -> None: + """Casing and surrounding space are forgiven; a second spelling is not. + + Aliases look free and are not: each one is another value to document, test and keep + in step with the picker, and the refusal already lists what is accepted. + """ + for value in ("aws_bedrock", "AWS_BEDROCK", " aws_bedrock "): with self.subTest(value=value): - plan = self.resolve(CB_IN_LLM=value, CB_IN_AWS_API_KEY="k") - self.assertEqual(plan["provider"], "aws") + plan = self.resolve(CB_IN_LLM=value, CB_IN_AWS_BEDROCK_API_KEY="k") + self.assertEqual(plan["provider"], "aws_bedrock") + for value in ("aws", "bedrock", "aws-bedrock", "gemini"): + with self.subTest(rejected=value): + error = self.refuse(CB_IN_LLM=value, CB_IN_AWS_BEDROCK_API_KEY="k") + self.assertEqual(error.code, "unknown_llm") + self.assertIn("aws_bedrock", error.message, "the refusal must name what to use") + + def test_each_providers_inputs_are_named_after_the_value_that_selects_it(self) -> None: + """`llm: X` always pairs with `X_api_key`, so the pairing never has to be looked up.""" + for name, provider in self.table["providers"].items(): + for input_name in provider["inputs"]: + with self.subTest(provider=name, input=input_name): + self.assertTrue(input_name.startswith(f"{name}_"), input_name) + + def test_the_table_carries_no_alias_map(self) -> None: + self.assertNotIn("aliases", self.table) def test_pasted_key_wrappers_are_stripped(self) -> None: plan = self.resolve(CB_IN_LLM="openrouter", CB_IN_OPENROUTER_API_KEY=" 'OPENROUTER_API_KEY=\"sk-x\"' \n") diff --git a/tests/test_provider_table_drift.py b/tests/test_provider_table_drift.py index e3b8c29..4311e06 100644 --- a/tests/test_provider_table_drift.py +++ b/tests/test_provider_table_drift.py @@ -42,6 +42,11 @@ def _core_providers(): CORE_PROVIDERS = _core_providers() +def core_name(name: str) -> str: + """The engine's name for a provider we may expose under a friendlier one.""" + return TABLE["providers"][name].get("core", name) + + @unittest.skipIf(CORE_PROVIDERS is None, "the pinned CodeBoarding release is not installed") class ProviderTableDriftTests(unittest.TestCase): def test_the_table_pins_the_release_action_yml_installs(self) -> None: @@ -55,7 +60,7 @@ def test_the_table_pins_the_release_action_yml_installs(self) -> None: def test_the_same_providers_exist_on_both_sides(self) -> None: self.assertEqual( - sorted(TABLE["providers"]), + sorted(core_name(n) for n in TABLE["providers"]), sorted(CORE_PROVIDERS), "the action and the pinned engine disagree about which providers exist", ) @@ -66,13 +71,13 @@ def test_selection_variables_match_core_exactly(self) -> None: with self.subTest(provider=name): self.assertEqual( sorted(provider["selection_envs"]), - sorted(CORE_PROVIDERS[name].selection_envs), + sorted(CORE_PROVIDERS[core_name(name)].selection_envs), f"{name}'s selection variables drifted from the engine", ) def test_every_provider_key_reaches_the_variable_core_reads(self) -> None: for name, provider in TABLE["providers"].items(): - api_key_env = CORE_PROVIDERS[name].api_key_env + api_key_env = CORE_PROVIDERS[core_name(name)].api_key_env if api_key_env is None: continue # e.g. aws, whose SDK reads its own bearer token variable with self.subTest(provider=name): @@ -91,14 +96,15 @@ def test_no_selection_variable_escapes_the_table(self) -> None: } self.assertEqual(sorted(core - known), []) - def test_aliases_point_at_providers_that_exist(self) -> None: - for alias, target in TABLE["aliases"].items(): - with self.subTest(alias=alias): - self.assertIn(target, CORE_PROVIDERS) - self.assertNotIn(alias, CORE_PROVIDERS, f"{alias} is a real provider, not an alias") + def test_every_renamed_provider_still_names_a_real_one(self) -> None: + """A `core` translation may only point at a provider the engine actually has.""" + for name, provider in TABLE["providers"].items(): + if "core" in provider: + with self.subTest(provider=name): + self.assertIn(provider["core"], CORE_PROVIDERS) def test_the_hosted_tier_names_a_real_provider(self) -> None: - self.assertIn(TABLE["hosted_provider"], CORE_PROVIDERS) + self.assertIn(core_name(TABLE["hosted_provider"]), CORE_PROVIDERS) if __name__ == "__main__": From 45e5aa8e676c99c56b706cb4b6d73194668f4d70 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Thu, 27 Aug 2026 17:11:47 +0200 Subject: [PATCH 5/6] refactor: name the credential module for what it owns `resolve_llm` described half the job. The module also writes every sentence the user reads about a credential problem: preflight puts the message in the step output, and action.yml posts that same string as the pull request comment, the error annotation and the job summary. Naming it `llm_credentials` and saying so in the docstring keeps the rule and its explanation together, which is the point of having them in one file. Co-Authored-By: Claude Opus 5 (1M context) --- .../{resolve_llm.py => llm_credentials.py} | 7 ++++++- scripts/action/preflight-llm.sh | 2 +- tests/test_llm_contract.py | 16 ++++++++-------- 3 files changed, 15 insertions(+), 10 deletions(-) rename scripts/action/{resolve_llm.py => llm_credentials.py} (95%) diff --git a/scripts/action/resolve_llm.py b/scripts/action/llm_credentials.py similarity index 95% rename from scripts/action/resolve_llm.py rename to scripts/action/llm_credentials.py index e8a4abb..2a89de9 100755 --- a/scripts/action/resolve_llm.py +++ b/scripts/action/llm_credentials.py @@ -1,5 +1,10 @@ #!/usr/bin/env python3 -"""Resolves the run's LLM credentials from the action's inputs, or explains why it cannot. +"""The run's LLM credentials: what they resolve to, or what to tell the user instead. + +Both halves live here on purpose. The `message` on every ConfigError is the sentence the +user actually reads -- preflight-llm.sh puts it in the step output, and action.yml posts +that same string as the pull request comment, the error annotation and the job summary -- +so the rule and its explanation are written together and cannot drift apart. One provider, chosen explicitly by the `llm` input, and nothing is ever reached by an empty string falling through to a default. A misconfigured run fails here -- before the diff --git a/scripts/action/preflight-llm.sh b/scripts/action/preflight-llm.sh index 23fabd2..ec06089 100755 --- a/scripts/action/preflight-llm.sh +++ b/scripts/action/preflight-llm.sh @@ -19,7 +19,7 @@ done < <(compgen -v | grep -E '^CB_IN_.*_API_KEY$' || true) [ -z "${CB_IN_LICENSE_KEY:-}" ] || echo "::add-mask::${CB_IN_LICENSE_KEY}" set +e -plan="$(python3 "${ACTION_PATH}/scripts/action/resolve_llm.py" --auth-dir "$AUTH_DIR")" +plan="$(python3 "${ACTION_PATH}/scripts/action/llm_credentials.py" --auth-dir "$AUTH_DIR")" resolved=$? set -e diff --git a/tests/test_llm_contract.py b/tests/test_llm_contract.py index 18eeaaa..e942d56 100644 --- a/tests/test_llm_contract.py +++ b/tests/test_llm_contract.py @@ -12,23 +12,23 @@ from pathlib import Path ROOT = Path(__file__).resolve().parent.parent -_spec = importlib.util.spec_from_file_location("resolve_llm", ROOT / "scripts" / "action" / "resolve_llm.py") -resolve_llm = importlib.util.module_from_spec(_spec) -_spec.loader.exec_module(resolve_llm) +_spec = importlib.util.spec_from_file_location("llm_credentials", ROOT / "scripts" / "action" / "llm_credentials.py") +llm_credentials = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(llm_credentials) OIDC = {"ACTIONS_ID_TOKEN_REQUEST_URL": "https://oidc.example/token"} class ContractTests(unittest.TestCase): def setUp(self) -> None: - self.table = resolve_llm.load_table() + self.table = llm_credentials.load_table() def resolve(self, **environ: str) -> dict: - return resolve_llm.resolve(self.table, environ) + return llm_credentials.resolve(self.table, environ) - def refuse(self, **environ: str) -> resolve_llm.ConfigError: - with self.assertRaises(resolve_llm.ConfigError) as caught: - resolve_llm.resolve(self.table, environ) + def refuse(self, **environ: str) -> llm_credentials.ConfigError: + with self.assertRaises(llm_credentials.ConfigError) as caught: + llm_credentials.resolve(self.table, environ) return caught.exception # -- accepted shapes --------------------------------------------------- From 5d03c51978e6fe54aafc440db91751cd8c017d68 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Thu, 27 Aug 2026 17:19:01 +0200 Subject: [PATCH 6/6] test: verify the credential failure path on a real runner Temporary. Names a provider with no key so the run must fail fast and say which secret to add. --- .github/workflows/codeboarding.yml | 3 +-- scripts/action/llm_credentials.py | 23 +++++++++++++++++++++++ tests/test_llm_contract.py | 26 ++++++++++++++++++++++++++ 3 files changed, 50 insertions(+), 2 deletions(-) diff --git a/.github/workflows/codeboarding.yml b/.github/workflows/codeboarding.yml index 5b868b7..74b9bc9 100644 --- a/.github/workflows/codeboarding.yml +++ b/.github/workflows/codeboarding.yml @@ -114,6 +114,5 @@ jobs: # Named, not inferred: the action refuses to guess where credentials come # from, so every workflow says which of hosted / license / a provider it # wants. CodeBoarding's own repositories run on the CodeBoarding plan. - llm: license - license_key: ${{ secrets.CODEBOARDING_LICENSE }} + llm: anthropic github_token: ${{ steps.codeboarding-app-token-client.outputs.token || steps.codeboarding-app-token-app.outputs.token || github.token }} diff --git a/scripts/action/llm_credentials.py b/scripts/action/llm_credentials.py index 2a89de9..24cbd2e 100755 --- a/scripts/action/llm_credentials.py +++ b/scripts/action/llm_credentials.py @@ -31,11 +31,34 @@ TABLE = Path(__file__).resolve().parent / "llm-providers.json" +#: Every reason this module can refuse a configuration. +#: +#: Declared rather than left implicit in the raise sites, for two reasons. These codes are +#: an interface: the action emits them as `llm_config_error` and the webview keys on them, +#: so adding one silently is a contract change nobody reviewed. And a code with no test is +#: invisible -- `license_with_provider_key` shipped untested precisely because nothing +#: enumerated the set. `test_llm_contract.py` asserts every entry here is exercised. +ERROR_CODES = frozenset( + { + "missing_llm", + "unknown_llm", + "missing_provider_key", + "missing_license_key", + "missing_id_token", + "hosted_with_provider_key", + "hosted_with_license", + "license_with_provider_key", + "foreign_provider_key", + } +) + + class ConfigError(Exception): """A configuration the action refuses to run, with the code the webview keys on.""" def __init__(self, code: str, message: str) -> None: super().__init__(message) + assert code in ERROR_CODES, f"undeclared error code: {code}" self.code = code self.message = message diff --git a/tests/test_llm_contract.py b/tests/test_llm_contract.py index e942d56..9e7be1c 100644 --- a/tests/test_llm_contract.py +++ b/tests/test_llm_contract.py @@ -133,6 +133,14 @@ def test_hosted_refuses_to_share_a_workflow_with_a_provider_key(self) -> None: self.assertEqual(error.code, "hosted_with_provider_key") self.assertIn("llm: anthropic", error.message) + def test_license_refuses_to_share_a_workflow_with_a_provider_key(self) -> None: + """The mirror of the hosted case, and it shipped untested: `llm: license` runs on + CodeBoarding's credentials, so a provider key beside it asks for two things at once.""" + error = self.refuse(CB_IN_LLM="license", CB_IN_LICENSE_KEY="lic", CB_IN_ANTHROPIC_API_KEY="k", **OIDC) + self.assertEqual(error.code, "license_with_provider_key") + self.assertIn("anthropic_api_key", error.message) + self.assertIn("llm: anthropic", error.message) + def test_hosted_refuses_a_licence_it_would_not_spend(self) -> None: error = self.refuse(CB_IN_LLM="hosted", CB_IN_LICENSE_KEY="lic", **OIDC) self.assertEqual(error.code, "hosted_with_license") @@ -155,6 +163,24 @@ def test_an_unknown_provider_lists_the_ones_that_exist(self) -> None: self.assertEqual(error.code, "unknown_llm") self.assertIn("anthropic", error.message) + def test_every_declared_refusal_is_exercised_by_this_file(self) -> None: + """No refusal ships untested. Walks one configuration per code and asserts the set + it produces is exactly the set the module declares it can raise, so adding a code + without a case here fails rather than going unnoticed.""" + cases = [ + {}, + {"CB_IN_LLM": "not_a_provider"}, + {"CB_IN_LLM": "anthropic"}, + {"CB_IN_LLM": "license", **OIDC}, + {"CB_IN_LLM": "hosted"}, + {"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": "anthropic", "CB_IN_ANTHROPIC_API_KEY": "k", "CB_IN_OPENAI_API_KEY": "o"}, + ] + seen = {self.refuse(**case).code for case in cases} + self.assertEqual(seen, set(llm_credentials.ERROR_CODES)) + def test_every_refusal_carries_a_code_and_an_actionable_message(self) -> None: cases = [ {},