From d4ae9e9d0f99fa04f1c5b60d94e7a75e22f5c449 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Thu, 27 Aug 2026 15:37:03 +0200 Subject: [PATCH 01/15] 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 02/15] 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 03/15] 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 04/15] 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 05/15] 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 c39d5b01af258c1517f7ddb79acd51fbacf7f4fc Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Thu, 27 Aug 2026 17:23:02 +0200 Subject: [PATCH 06/15] fix(review): strip the last foreign selector, and hash the backend Three more from the Codex review, all real: - `"\n".join(...)` left foreign-envs unterminated, and `read` reports failure on an unterminated final line, so `while read` never ran its body for that record. VERCEL_BASE_URL sorts last, so an Anthropic run inherited it and core saw two providers configured. The list is terminated and the loop now handles a partial final line, either of which alone would fix it. The existing stripping test passed throughout because none of the variables it named was last, so the new one asserts the last entry specifically. - Endpoint and region are first-class inputs now, but the reusable-analysis name hashed only provider and model, so pointing `openai_base_url` at another gateway could restore a warm start built against the old one. A backend id covering the tier, provider, endpoints and region feeds the state identity. It carries no key: rotating a secret must not throw away reusable analysis, and an artifact name must never be built from one. - The README's input table still named `aws_region` after the rename. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 2 +- action.yml | 1 + scripts/action/llm_credentials.py | 21 ++++++++++++++- scripts/action/preflight-llm.sh | 4 +++ scripts/action/state-names.sh | 7 +++-- scripts/action/with-auth.sh | 5 +++- tests/test_action_auth.py | 44 +++++++++++++++++++++++++++++++ 7 files changed, 79 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index dda4685..4f73941 100644 --- a/README.md +++ b/README.md @@ -296,7 +296,7 @@ With the default `github.token`, the repository or organization must allow GitHu | `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`. | +| `aws_bedrock_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`. | diff --git a/action.yml b/action.yml index 0b6b4cd..b9b9c22 100644 --- a/action.yml +++ b/action.yml @@ -371,6 +371,7 @@ runs: PR_NUMBER: ${{ steps.guard.outputs.pr_number }} IS_FORK: ${{ steps.guard.outputs.is_fork }} LLM_PROVIDER: ${{ steps.llm.outputs.provider }} + BACKEND_ID: ${{ steps.llm.outputs.backend_id }} MODEL: ${{ inputs.model }} AGENT_MODEL_INPUT: ${{ inputs.agent_model }} PARSING_MODEL_INPUT: ${{ inputs.parsing_model }} diff --git a/scripts/action/llm_credentials.py b/scripts/action/llm_credentials.py index 2a89de9..0ec2f3c 100755 --- a/scripts/action/llm_credentials.py +++ b/scripts/action/llm_credentials.py @@ -202,6 +202,11 @@ def resolve(table: dict, environ: dict[str, str]) -> dict: } +def _is_endpoint(var: str) -> bool: + """Configuration rather than credential: safe to name in an artifact, worth hashing.""" + return var.endswith(("_BASE_URL", "_HOST")) or var == "AWS_DEFAULT_REGION" + + 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) @@ -228,7 +233,21 @@ def write_auth_dir(table: dict, plan: dict, auth_dir: Path) -> None: 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") + # Trailing newline, because `read` returns non-zero on an unterminated final line and + # a `while read` loop therefore skips it. Without it the alphabetically last variable + # was never unset: an inherited VERCEL_BASE_URL survived an Anthropic run and let core + # see two providers configured. with-auth.sh guards the same case from its side. + # What the run talks to, minus anything secret. The reusable-analysis name is built + # from this: pointing `openai_base_url` at a different backend, or moving Bedrock to + # another region, produces different analysis, so it must not restore a bundle built + # against the old one. Keys are deliberately excluded -- they do not change what the + # model says, and rotating one should not throw away a warm start. + backend = [f"{plan['tier']}:{plan['provider']}"] + backend += [f"{var}={value}" for var, value in sorted(plan["env"].items()) if _is_endpoint(var)] + (auth_dir / "backend-id").write_text("\n".join(backend), encoding="utf-8") + + foreign = sorted(everything - keep) + (auth_dir / "foreign-envs").write_text("".join(f"{var}\n" for var in foreign), encoding="utf-8") def main(argv: list[str]) -> int: diff --git a/scripts/action/preflight-llm.sh b/scripts/action/preflight-llm.sh index ec06089..d5f1fe0 100755 --- a/scripts/action/preflight-llm.sh +++ b/scripts/action/preflight-llm.sh @@ -32,8 +32,12 @@ provider="$(field provider)" error="$(field error)" message="$(field message)" +backend_id="" +[ ! -s "$AUTH_DIR/backend-id" ] || backend_id="$(cksum < "$AUTH_DIR/backend-id" | cut -d' ' -f1)" + { echo "tier=$tier" + echo "backend_id=$backend_id" echo "provider=$provider" echo "error=$error" echo "message< None: 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_the_last_foreign_selector_is_stripped_like_every_other(self) -> None: + """The alphabetically last entry is the one a `while read` loop drops. + + `"\\n".join(...)` left the file unterminated, so `read` failed at EOF and never ran + the body for that record. VERCEL_BASE_URL sorts last, so an Anthropic run inherited + it and core saw two providers configured. The earlier stripping test passed + throughout, because none of the variables it named was last. + """ + result, auth_dir, _ = self._preflight(CB_IN_LLM="anthropic", CB_IN_ANTHROPIC_API_KEY="k") + self.assertEqual(result.returncode, 0, result.stderr or result.stdout) + listed = (auth_dir / "foreign-envs").read_text(encoding="utf-8") + self.assertTrue(listed.endswith("\n"), "an unterminated final record is silently skipped") + last = listed.strip().splitlines()[-1] + + scoped = self._with_auth(f'test -z "${{{last}:-}}"', **{last: "inherited"}) + self.assertEqual(scoped.returncode, 0, f"{last} survived into the analysis") + + def test_endpoint_and_region_changes_reach_the_reusable_analysis_name(self) -> None: + """Two runs that talk to different backends must not share a warm start.""" + secret = "sk-distinctive-value" + first, auth_dir, outputs = self._preflight( + CB_IN_LLM="openai", CB_IN_OPENAI_API_KEY=secret, CB_IN_OPENAI_BASE_URL="https://a.example/v1" + ) + self.assertEqual(first.returncode, 0, first.stderr or first.stdout) + recorded = (auth_dir / "backend-id").read_text(encoding="utf-8") + self.assertIn("https://a.example/v1", recorded) + self.assertNotIn(secret, recorded, "an artifact name must never be built from a key") + moved = outputs["backend_id"] + + self.temp_dir.cleanup() + self.temp_dir = tempfile.TemporaryDirectory() + _, _, other = self._preflight( + CB_IN_LLM="openai", CB_IN_OPENAI_API_KEY=secret, CB_IN_OPENAI_BASE_URL="https://b.example/v1" + ) + self.assertNotEqual(moved, other["backend_id"], "a different endpoint must not reuse analysis") + + def test_rotating_a_key_does_not_throw_away_reusable_analysis(self) -> None: + """The backend id names what the run talks to, never the secret it talks with.""" + _, _, first = self._preflight(CB_IN_LLM="anthropic", CB_IN_ANTHROPIC_API_KEY="old-key") + self.temp_dir.cleanup() + self.temp_dir = tempfile.TemporaryDirectory() + _, _, second = self._preflight(CB_IN_LLM="anthropic", CB_IN_ANTHROPIC_API_KEY="new-key") + self.assertEqual(first["backend_id"], second["backend_id"]) + 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( From f0cc70fc5befef81d9b10325f38ca4eb3a7b786c Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Thu, 27 Aug 2026 17:27:16 +0200 Subject: [PATCH 07/15] test: declare the refusal codes, and cover the one that shipped untested `license_with_provider_key` could be raised and nothing asserted it, which is what happens when a failure surface is only implicit in its raise sites. The codes are declared in one frozenset now, ConfigError asserts membership, and a test walks one configuration per code and asserts the set produced is exactly the set declared, so adding a code without a case fails the build. These codes are an interface: the action emits them as `llm_config_error` and the webview keys on them, so adding one silently was a contract change nobody reviewed. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/action/llm_credentials.py | 23 +++++++++++++++++++++++ tests/test_llm_contract.py | 26 ++++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/scripts/action/llm_credentials.py b/scripts/action/llm_credentials.py index 0ec2f3c..db40645 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 = [ {}, From e4c801fb7320596a69d3ba6c5b816f14f05e1ef0 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Thu, 27 Aug 2026 17:52:45 +0200 Subject: [PATCH 08/15] refactor: name the credential files for what they do Agreed naming: - supported-providers.json says what the list IS, which is the same set the refusal message recites back when a value is not recognised. - credential_check.py leads with the verb, because refusing with a reason is the primary job. `resolve_llm` named only the resolving half, and the module also writes every sentence the user reads about a credential problem. - verify-credentials.sh matches the verb-noun shape of its siblings (configure-auth, fetch-state, deliver-sync, render-review) rather than being the one noun-first script in the directory. configure-auth.sh keeps its name for now, though after this change it only starts the hosted relay. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 2 +- README.md | 2 +- action.yml | 2 +- scripts/action/configure-auth.sh | 2 +- ...{llm_credentials.py => credential_check.py} | 4 ++-- ...providers.json => supported-providers.json} | 0 ...{preflight-llm.sh => verify-credentials.sh} | 2 +- tests/test_action_auth.py | 2 +- tests/test_action_inputs.py | 2 +- tests/test_llm_contract.py | 18 +++++++++--------- tests/test_provider_table_drift.py | 4 ++-- 11 files changed, 20 insertions(+), 20 deletions(-) rename scripts/action/{llm_credentials.py => credential_check.py} (98%) rename scripts/action/{llm-providers.json => supported-providers.json} (100%) rename scripts/action/{preflight-llm.sh => verify-credentials.sh} (96%) diff --git a/AGENTS.md b/AGENTS.md index d1d78ae..fd9fbbd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -17,7 +17,7 @@ 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`. +`scripts/action/supported-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. diff --git a/README.md b/README.md index 4f73941..22827d3 100644 --- a/README.md +++ b/README.md @@ -173,7 +173,7 @@ thing to document and keep in step, and an unrecognised value is refused with th 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), +This table is generated from [`scripts/action/supported-providers.json`](scripts/action/supported-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. diff --git a/action.yml b/action.yml index b9b9c22..6d0c2f1 100644 --- a/action.yml +++ b/action.yml @@ -276,7 +276,7 @@ runs: 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" + run: "$GITHUB_ACTION_PATH/scripts/action/verify-credentials.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. diff --git a/scripts/action/configure-auth.sh b/scripts/action/configure-auth.sh index 6f0b09b..6abcc9d 100755 --- a/scripts/action/configure-auth.sh +++ b/scripts/action/configure-auth.sh @@ -1,7 +1,7 @@ #!/usr/bin/env bash # 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 +# The plan itself was decided by verify-credentials.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 diff --git a/scripts/action/llm_credentials.py b/scripts/action/credential_check.py similarity index 98% rename from scripts/action/llm_credentials.py rename to scripts/action/credential_check.py index db40645..c0bcf37 100755 --- a/scripts/action/llm_credentials.py +++ b/scripts/action/credential_check.py @@ -2,7 +2,7 @@ """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 +user actually reads -- verify-credentials.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. @@ -28,7 +28,7 @@ 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" +TABLE = Path(__file__).resolve().parent / "supported-providers.json" #: Every reason this module can refuse a configuration. diff --git a/scripts/action/llm-providers.json b/scripts/action/supported-providers.json similarity index 100% rename from scripts/action/llm-providers.json rename to scripts/action/supported-providers.json diff --git a/scripts/action/preflight-llm.sh b/scripts/action/verify-credentials.sh similarity index 96% rename from scripts/action/preflight-llm.sh rename to scripts/action/verify-credentials.sh index d5f1fe0..e613d87 100755 --- a/scripts/action/preflight-llm.sh +++ b/scripts/action/verify-credentials.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/llm_credentials.py" --auth-dir "$AUTH_DIR")" +plan="$(python3 "${ACTION_PATH}/scripts/action/credential_check.py" --auth-dir "$AUTH_DIR")" resolved=$? set -e diff --git a/tests/test_action_auth.py b/tests/test_action_auth.py index 04dd4ed..ab5fdca 100644 --- a/tests/test_action_auth.py +++ b/tests/test_action_auth.py @@ -10,7 +10,7 @@ from pathlib import Path ROOT = Path(__file__).resolve().parent.parent -PREFLIGHT = ROOT / "scripts" / "action" / "preflight-llm.sh" +PREFLIGHT = ROOT / "scripts" / "action" / "verify-credentials.sh" CONFIGURE_AUTH = ROOT / "scripts" / "action" / "configure-auth.sh" WITH_AUTH = ROOT / "scripts" / "action" / "with-auth.sh" diff --git a/tests/test_action_inputs.py b/tests/test_action_inputs.py index 63ea061..bf8aeef 100644 --- a/tests/test_action_inputs.py +++ b/tests/test_action_inputs.py @@ -15,7 +15,7 @@ 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")) +TABLE = json.loads((ROOT / "scripts" / "action" / "supported-providers.json").read_text(encoding="utf-8")) def declared_inputs() -> dict[str, str]: diff --git a/tests/test_llm_contract.py b/tests/test_llm_contract.py index 9e7be1c..9b875b0 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("llm_credentials", ROOT / "scripts" / "action" / "llm_credentials.py") -llm_credentials = importlib.util.module_from_spec(_spec) -_spec.loader.exec_module(llm_credentials) +_spec = importlib.util.spec_from_file_location("credential_check", ROOT / "scripts" / "action" / "credential_check.py") +credential_check = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(credential_check) OIDC = {"ACTIONS_ID_TOKEN_REQUEST_URL": "https://oidc.example/token"} class ContractTests(unittest.TestCase): def setUp(self) -> None: - self.table = llm_credentials.load_table() + self.table = credential_check.load_table() def resolve(self, **environ: str) -> dict: - return llm_credentials.resolve(self.table, environ) + return credential_check.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) + def refuse(self, **environ: str) -> credential_check.ConfigError: + with self.assertRaises(credential_check.ConfigError) as caught: + credential_check.resolve(self.table, environ) return caught.exception # -- accepted shapes --------------------------------------------------- @@ -179,7 +179,7 @@ def test_every_declared_refusal_is_exercised_by_this_file(self) -> None: {"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)) + self.assertEqual(seen, set(credential_check.ERROR_CODES)) def test_every_refusal_carries_a_code_and_an_actionable_message(self) -> None: cases = [ diff --git a/tests/test_provider_table_drift.py b/tests/test_provider_table_drift.py index 4311e06..13d0a0b 100644 --- a/tests/test_provider_table_drift.py +++ b/tests/test_provider_table_drift.py @@ -17,7 +17,7 @@ from pathlib import Path ROOT = Path(__file__).resolve().parent.parent -TABLE = json.loads((ROOT / "scripts" / "action" / "llm-providers.json").read_text(encoding="utf-8")) +TABLE = json.loads((ROOT / "scripts" / "action" / "supported-providers.json").read_text(encoding="utf-8")) def _core_providers(): @@ -55,7 +55,7 @@ def test_the_table_pins_the_release_action_yml_installs(self) -> None: self.assertEqual( TABLE["engine"], pinned.group(1), - "llm-providers.json records a different release than action.yml installs", + "supported-providers.json records a different release than action.yml installs", ) def test_the_same_providers_exist_on_both_sides(self) -> None: From e3f476ac16c65d613e487148f2abaff845665d7d Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Thu, 27 Aug 2026 20:01:10 +0200 Subject: [PATCH 09/15] feat(review): give a refused run the page to click and the line to copy "Add the secret and wire it as X" describes the fix rather than being it. The reader is often someone who has never edited a workflow, so a refusal now names the exact secrets page as a link, the exact workflow file, and the YAML to paste, indented as it will sit in the `with:` block. That needs two renderings, because the surfaces differ: `::error::` annotations cannot carry newlines, so `message` stays one plain line for the annotation while `details` carries the markdown for the pull request comment and the job summary. Only the refusals with one exact answer get a snippet. "Remove one of these" has two valid fixes, so picking one would be guessing, and it stays prose. The repository and workflow file come from GITHUB_REPOSITORY and GITHUB_WORKFLOW_REF, so off a runner the remedy degrades to prose rather than emitting a half-built link. Co-Authored-By: Claude Opus 5 (1M context) --- action.yml | 2 +- scripts/action/credential_check.py | 100 ++++++++++++++++++++++++--- scripts/action/verify-credentials.sh | 8 ++- tests/test_action_auth.py | 18 +++++ tests/test_llm_contract.py | 47 +++++++++++++ 5 files changed, 163 insertions(+), 12 deletions(-) diff --git a/action.yml b/action.yml index 6d0c2f1..e09a074 100644 --- a/action.yml +++ b/action.yml @@ -291,7 +291,7 @@ runs: message: | ### CodeBoarding review - not configured - ${{ steps.llm.outputs.message }} + ${{ steps.llm.outputs.details }} No analysis ran, and no CodeBoarding hosted usage was consumed. diff --git a/scripts/action/credential_check.py b/scripts/action/credential_check.py index c0bcf37..bfe6054 100755 --- a/scripts/action/credential_check.py +++ b/scripts/action/credential_check.py @@ -54,13 +54,59 @@ class ConfigError(Exception): - """A configuration the action refuses to run, with the code the webview keys on.""" + """A configuration the action refuses to run, with the code the webview keys on. - def __init__(self, code: str, message: str) -> None: + Two renderings, because they go to surfaces with different rules. ``message`` is one + plain line, for the ``::error::`` annotation, which cannot carry newlines. ``details`` + is markdown for the pull request comment and the job summary, where a link and a + snippet the reader can copy are worth far more than a sentence describing them. + """ + + def __init__(self, code: str, message: str, details: str | None = None) -> None: super().__init__(message) assert code in ERROR_CODES, f"undeclared error code: {code}" self.code = code self.message = message + self.details = details or message + + +def secrets_url(environ: dict[str, str]) -> str | None: + """This repository's "new secret" page, when the runner tells us which repo we are in.""" + repo = environ.get("GITHUB_REPOSITORY", "") + if not repo: + return None + server = environ.get("GITHUB_SERVER_URL", "https://github.com").rstrip("/") + return f"{server}/{repo}/settings/secrets/actions/new" + + +def workflow_path(environ: dict[str, str]) -> str: + """The workflow file to edit, named rather than left for the reader to find. + + GITHUB_WORKFLOW_REF is ``owner/repo/.github/workflows/x.yml@refs/...``; a repository + can have several workflows calling this action, so "add it to your workflow" is not + an instruction someone can follow without guessing. + """ + ref = environ.get("GITHUB_WORKFLOW_REF", "") + path = ref.split("@", 1)[0] + _, _, tail = path.partition("/") + _, _, tail = tail.partition("/") + return tail or "your CodeBoarding workflow" + + +def _add_secret(environ: dict[str, str], secret: str, what: str) -> str: + """Step one of every credential remedy: put the value in the repository.""" + url = secrets_url(environ) + where = f"[Add a repository secret]({url})" if url else "Add a repository secret" + return f"{where} named `{secret}`, with {what} as the value." + + +def _wire_it(environ: dict[str, str], llm: str, lines: list[str]) -> str: + """Step two: the exact YAML, in the exact file, indented as it will sit there.""" + body = "\n".join(f" {line}" for line in lines) + return ( + f"In `{workflow_path(environ)}`, the CodeBoarding step's `with:` block " + f"needs to read:\n\n```yaml\n with:\n llm: {llm}\n{body}\n```" + ) def load_table(path: Path = TABLE) -> dict: @@ -124,10 +170,18 @@ def _require_id_token(llm: str, environ: dict[str, str]) -> None: "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.", + "\n\n".join( + [ + f"`llm: {llm}` authenticates with a GitHub OIDC token, which this job cannot mint.", + f"In `{workflow_path(environ)}`, the job running this action needs:", + "```yaml\n permissions:\n id-token: write\n```", + "No secret is involved: the token is minted per request and never stored.", + ] + ), ) -def _resolve_byok(table: dict, name: str, given: dict[str, str]) -> dict: +def _resolve_byok(table: dict, name: str, given: dict[str, str], environ: dict[str, str]) -> dict: provider = table["providers"][name] foreign = sorted(i for i in given if owner_of(table, i) != name) if foreign: @@ -150,15 +204,26 @@ def _resolve_byok(table: dict, name: str, given: dict[str, str]) -> dict: # 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} }}}}`." + fix = f"Add the {secret} repository secret ({SETTINGS_HINT}) and wire it as `{keys[0]}`." + details = "\n\n".join( + [ + f"`llm: {name}` needs {needed}, and none is set.", + "**1.** " + _add_secret(environ, secret, f"your {provider['label']} API key"), + "**2.** " + _wire_it(environ, name, [f"{keys[0]}: ${{{{ secrets.{secret} }}}}"]), + ] ) else: fix = f"Set `{wanted[0]}` on the action step to your {provider['label']} endpoint." + details = "\n\n".join( + [ + f"`llm: {name}` needs {needed}, and none is set.", + _wire_it(environ, name, [f"{wanted[0]}: https://your-{name}-host"]), + ] + ) raise ConfigError( "missing_provider_key", f"`llm: {name}` needs {needed}, and none is set. {fix}", + details, ) return env @@ -194,8 +259,14 @@ def resolve(table: dict, environ: dict[str, str]) -> dict: 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 }}`.", + f"CODEBOARDING_LICENSE repository secret ({SETTINGS_HINT}) and wire it.", + "\n\n".join( + [ + "`llm: license` needs `license_key`, which is empty.", + "**1.** " + _add_secret(environ, "CODEBOARDING_LICENSE", "your CodeBoarding licence key"), + "**2.** " + _wire_it(environ, "license", ["license_key: ${{ secrets.CODEBOARDING_LICENSE }}"]), + ] + ), ) _require_id_token(llm, environ) return { @@ -214,7 +285,7 @@ def resolve(table: dict, environ: dict[str, str]) -> dict: f"See {DOCS}.", ) - env = _resolve_byok(table, name, given) + env = _resolve_byok(table, name, given, environ) # 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. @@ -282,7 +353,15 @@ def main(argv: list[str]) -> int: try: plan = resolve(table, dict(os.environ)) except ConfigError as error: - json.dump({"ok": False, "error": error.code, "message": error.message}, sys.stdout) + json.dump( + { + "ok": False, + "error": error.code, + "message": error.message, + "details": error.details, + }, + sys.stdout, + ) print() return 1 @@ -293,6 +372,7 @@ def main(argv: list[str]) -> int: "ok": True, "error": "", "message": "", + "details": "", "tier": plan["tier"], "provider": plan["provider"], }, diff --git a/scripts/action/verify-credentials.sh b/scripts/action/verify-credentials.sh index e613d87..47fcd73 100755 --- a/scripts/action/verify-credentials.sh +++ b/scripts/action/verify-credentials.sh @@ -31,6 +31,7 @@ tier="$(field tier)" provider="$(field provider)" error="$(field error)" message="$(field message)" +details="$(field details)" backend_id="" [ ! -s "$AUTH_DIR/backend-id" ] || backend_id="$(cksum < "$AUTH_DIR/backend-id" | cut -d' ' -f1)" @@ -43,6 +44,11 @@ backend_id="" echo "message<> "$GITHUB_OUTPUT" if [ "$resolved" -ne 0 ]; then @@ -50,7 +56,7 @@ if [ "$resolved" -ne 0 ]; then { echo "### CodeBoarding could not start" echo - echo "$message" + echo "$details" echo echo "No analysis ran, and no CodeBoarding hosted usage was consumed." } >> "${GITHUB_STEP_SUMMARY:-/dev/null}" diff --git a/tests/test_action_auth.py b/tests/test_action_auth.py index ab5fdca..57f4909 100644 --- a/tests/test_action_auth.py +++ b/tests/test_action_auth.py @@ -221,8 +221,26 @@ def test_refusal_reports_a_code_and_an_actionable_message(self) -> None: self.assertEqual(outputs["error"], "hosted_with_provider_key") self.assertIn("openai_api_key", outputs["message"]) self.assertIn("::error title=CodeBoarding LLM configuration::", result.stdout) + # The annotation carries the one-line form; the summary carries the copyable one. + annotation = next(line for line in result.stdout.splitlines() if line.startswith("::error title=")) + self.assertNotIn("```", annotation) summary = (Path(self.temp_dir.name) / "summary.md").read_text(encoding="utf-8") self.assertIn("CodeBoarding could not start", summary) + # "Remove one of these" has two valid fixes, so it stays prose; the remedies that + # have one exact answer carry a snippet, which the next test covers. + self.assertIn("openai_api_key", outputs["details"]) + + def test_a_refusal_with_one_exact_fix_carries_it_into_the_summary(self) -> None: + result, _, outputs = self._preflight( + CB_IN_LLM="anthropic", + GITHUB_REPOSITORY="acme/widgets", + GITHUB_SERVER_URL="https://github.com", + ) + self.assertNotEqual(result.returncode, 0) + summary = (Path(self.temp_dir.name) / "summary.md").read_text(encoding="utf-8") + self.assertIn("```yaml", summary, "the summary should show the line to add") + self.assertIn("secrets/actions/new", summary, "and the page to click") + self.assertIn("anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }}", outputs["details"]) def test_successful_run_reports_its_tier_and_provider(self) -> None: """The webview reads this to show what a repository is actually running on.""" diff --git a/tests/test_llm_contract.py b/tests/test_llm_contract.py index 9b875b0..6f9eb86 100644 --- a/tests/test_llm_contract.py +++ b/tests/test_llm_contract.py @@ -163,6 +163,53 @@ 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_a_remedy_links_the_secrets_page_and_shows_the_line_to_add(self) -> None: + """ "Add the secret and wire it" is a description of the fix, not the fix. + + The reader is someone who has never edited a workflow. They get the page to click + and the exact YAML, in the file it belongs in, indented as it will sit there. + """ + runner = { + "GITHUB_REPOSITORY": "acme/widgets", + "GITHUB_SERVER_URL": "https://github.com", + "GITHUB_WORKFLOW_REF": "acme/widgets/.github/workflows/codeboarding.yml@refs/pull/7/merge", + } + error = self.refuse(CB_IN_LLM="anthropic", **runner) + self.assertIn("https://github.com/acme/widgets/settings/secrets/actions/new", error.details) + self.assertIn(".github/workflows/codeboarding.yml", error.details) + self.assertIn("```yaml", error.details) + self.assertIn("anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }}", error.details) + self.assertIn("llm: anthropic", error.details) + + def test_a_remedy_degrades_to_prose_off_a_runner(self) -> None: + """No GITHUB_REPOSITORY means no link to build, so say it without one.""" + error = self.refuse(CB_IN_LLM="anthropic") + self.assertNotIn("](", error.details, "a half-built link is worse than none") + self.assertIn("Add a repository secret", error.details) + self.assertIn("your CodeBoarding workflow", error.details) + + def test_the_licence_and_permission_remedies_are_copyable_too(self) -> None: + licence = self.refuse(CB_IN_LLM="license", GITHUB_REPOSITORY="acme/widgets", **OIDC) + self.assertIn("secrets/actions/new", licence.details) + self.assertIn("license_key: ${{ secrets.CODEBOARDING_LICENSE }}", licence.details) + + oidc = self.refuse(CB_IN_LLM="hosted") + self.assertIn("permissions:", oidc.details) + self.assertIn("id-token: write", oidc.details) + self.assertIn("```yaml", oidc.details) + + def test_the_annotation_stays_one_line_however_rich_the_remedy(self) -> None: + """`::error::` cannot carry newlines, so the two renderings must stay separate.""" + for environ in ( + {"CB_IN_LLM": "anthropic", "GITHUB_REPOSITORY": "acme/widgets"}, + {"CB_IN_LLM": "license", **OIDC}, + {"CB_IN_LLM": "hosted"}, + {}, + ): + with self.subTest(environ=environ): + error = self.refuse(**environ) + self.assertNotIn("\n", 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 From 17c0e8cc5df7da5516024efe4889d28ff386b31a Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Thu, 27 Aug 2026 20:13:52 +0200 Subject: [PATCH 10/15] feat(review): say which credential actually pays, and stop when the check crashes Three things, all from questions the summary could not answer: - `byok+license` runs on YOUR key, always. A direct provider call never reaches CodeBoarding, so there is nothing for a licence to pay for there; it is recorded and not spent. That was already the behaviour and nothing pinned it, so a test now asserts the provider key is exported, no licence is staged, and the relay is never started. - "Tier: byok+license" names the configuration without answering the question a summary is read to answer. The summary now states which credential pays, says outright that a wired licence is not spent on a direct call, and names a non-default endpoint or region, which is the setting most likely to be wrong and least likely to be noticed. - The credential check is `continue-on-error` so a refusal can be reported before the job dies. A crash inside it -- an unreadable table, a missing python3 -- therefore left `error` empty and let the run reach the checkout and the engine install. The stop step watches the step outcome as well, so "credentials are decided before anything expensive happens" holds when the deciding breaks. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 6 ++- action.yml | 14 ++++++- scripts/action/credential_check.py | 33 +++++++++++++++ scripts/action/verify-credentials.sh | 8 ++-- tests/test_action_auth.py | 60 ++++++++++++++++++++++++++++ tests/test_action_inputs.py | 10 +++++ 6 files changed, 124 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 22827d3..854ff0d 100644 --- a/README.md +++ b/README.md @@ -143,8 +143,10 @@ The same rule makes the combinations explicit rather than order-dependent: | `llm: anthropic` + key + `license_key` | Anthropic, on a CodeBoarding plan | A licence alongside your own key is deliberately allowed: it says "my CodeBoarding plan, -my own tokens". Direct provider calls never reach our proxy, so nothing meters that -combination today; it is recorded and reported, not enforced. +my own tokens". **Your key always wins.** A direct provider call never reaches +CodeBoarding, so the licence is recorded and reported but not spent, and nothing meters +that combination today. The job summary says so on every run, rather than leaving you to +infer it from the tier name. ### Providers diff --git a/action.yml b/action.yml index e09a074..f9f32ce 100644 --- a/action.yml +++ b/action.yml @@ -297,13 +297,23 @@ runs: run [${{ github.run_id }}](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}) - attempt ${{ github.run_attempt }} + # Fires on a refusal AND on the check failing to reach a verdict at all. The step + # above is `continue-on-error` so the refusal can be reported before the job dies, + # which also means a crash in it -- an unreadable provider table, a missing python3 -- + # would leave `error` empty and let the run carry on to the checkout and the engine + # install. Checking the outcome as well keeps "credentials are decided before anything + # expensive happens" true when the deciding itself breaks. - name: Stop on LLM configuration failure - if: steps.guard.outputs.skip != 'true' && steps.llm.outputs.error != '' + if: steps.guard.outputs.skip != 'true' && (steps.llm.outputs.error != '' || steps.llm.outcome != 'success') shell: bash env: CB_ERROR: ${{ steps.llm.outputs.error }} run: | - echo "CodeBoarding did not run: $CB_ERROR" >&2 + if [ -n "$CB_ERROR" ]; then + echo "CodeBoarding did not run: $CB_ERROR" >&2 + else + echo "::error title=CodeBoarding LLM configuration::The credential check did not complete, so no analysis was attempted. See the step above." + fi exit 1 - name: Post review progress diff --git a/scripts/action/credential_check.py b/scripts/action/credential_check.py index bfe6054..683939e 100755 --- a/scripts/action/credential_check.py +++ b/scripts/action/credential_check.py @@ -301,6 +301,38 @@ def _is_endpoint(var: str) -> bool: return var.endswith(("_BASE_URL", "_HOST")) or var == "AWS_DEFAULT_REGION" +def plan_summary(table: dict, plan: dict) -> list[tuple[str, str]]: + """What this run is actually about to do, for the job summary. + + "Tier: byok+license" names the configuration without answering the question someone + reads a summary to answer, which is *which credential pays*. A direct provider call + never reaches CodeBoarding, so a licence wired beside your own key is recorded and + not spent; saying only "byok+license" leaves that ambiguous, so it is spelled out. + """ + tier, provider = plan["tier"], plan["provider"] + label = table["providers"].get(provider, {}).get("label", provider) + rows = [("Tier", f"`{tier}`"), ("Provider", f"`{provider}`")] + if tier == "hosted": + rows.append(("Credentials", "CodeBoarding's hosted free tier")) + elif tier == "license": + rows.append(("Credentials", "CodeBoarding's hosted tier, on your plan")) + else: + rows.append(("Credentials", f"your own {label} key, called directly")) + if tier == "byok+license": + rows.append( + ( + "Licence", + "wired, and not spent: a direct provider call never reaches CodeBoarding", + ) + ) + # Only where the run was pointed somewhere other than the default, since that is the + # setting most likely to be wrong and least likely to be noticed. + for var, value in sorted(plan["env"].items()): + if _is_endpoint(var): + rows.append((f"`{var}`", f"`{value}`")) + return rows + + 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) @@ -375,6 +407,7 @@ def main(argv: list[str]) -> int: "details": "", "tier": plan["tier"], "provider": plan["provider"], + "summary": "\n".join(f"| {k} | {v} |" for k, v in plan_summary(table, plan)), }, sys.stdout, ) diff --git a/scripts/action/verify-credentials.sh b/scripts/action/verify-credentials.sh index 47fcd73..68ec5ae 100755 --- a/scripts/action/verify-credentials.sh +++ b/scripts/action/verify-credentials.sh @@ -82,15 +82,17 @@ 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" ;; + # Named precisely: the plan is wired but a direct provider call never reaches us, so + # "with a CodeBoarding plan" would imply the plan is paying for something here. + byok+license) label="your own ${provider} key (the wired CodeBoarding plan is not spent on a direct call)" ;; *) label="${tier}" ;; esac echo "CodeBoarding is running on ${label}." +# The rows come from the check, which is the only thing that knows what it resolved. { echo "### CodeBoarding configuration" echo echo "| | |" echo "|---|---|" - echo "| Tier | \`${tier}\` |" - echo "| Provider | \`${provider}\` |" + field summary } >> "${GITHUB_STEP_SUMMARY:-/dev/null}" diff --git a/tests/test_action_auth.py b/tests/test_action_auth.py index 57f4909..1cf2568 100644 --- a/tests/test_action_auth.py +++ b/tests/test_action_auth.py @@ -308,6 +308,66 @@ def test_hosted_auth_relays_to_the_codeboarding_proxy(self) -> None: 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_your_own_key_wins_outright_when_a_licence_is_wired_beside_it(self) -> None: + """`byok+license` runs on YOUR key. The licence is recorded and never spent. + + A direct provider call does not go through CodeBoarding, so there is nothing for a + licence to pay for. The tier label alone leaves that ambiguous, so the guarantee is + pinned here: the provider key is exported, no licence is staged for the relay, and + the relay is never started. + """ + temp_dir = Path(self.temp_dir.name) + fake_bin = temp_dir / "bin" + fake_bin.mkdir() + 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) + + result, auth_dir, outputs = self._preflight( + CB_IN_LLM="anthropic", + CB_IN_ANTHROPIC_API_KEY="my-own-key", + CB_IN_LICENSE_KEY="a-licence", + ACTIONS_ID_TOKEN_REQUEST_URL="https://oidc.example/token", + ACTIONS_ID_TOKEN_REQUEST_TOKEN="request-token", + ) + self.assertEqual(result.returncode, 0, result.stderr or result.stdout) + self.assertEqual(outputs["tier"], "byok+license") + self.assertEqual((auth_dir / "env" / "ANTHROPIC_API_KEY").read_text(), "my-own-key") + self.assertFalse((auth_dir / "license.txt").exists(), "no licence is staged for the relay") + self.assertNotIn("a-licence", (auth_dir / "env" / "ANTHROPIC_API_KEY").read_text()) + + configured = subprocess.run( + [str(CONFIGURE_AUTH)], + env={ + "PATH": f"{fake_bin}:{os.environ['PATH']}", + "ACTION_PATH": str(ROOT), + "RUNNER_TEMP": str(temp_dir / "runner"), + "ACTIONS_ID_TOKEN_REQUEST_URL": "https://oidc.example/token", + "ACTIONS_ID_TOKEN_REQUEST_TOKEN": "request-token", + }, + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(configured.returncode, 0, configured.stderr or configured.stdout) + self.assertFalse(marker.exists(), "a licensed BYOK run must not contact the relay") + + scoped = self._with_auth('test "$ANTHROPIC_API_KEY" = "my-own-key" && test -z "${OPENROUTER_API_KEY:-}"') + self.assertEqual(scoped.returncode, 0, scoped.stderr or scoped.stdout) + + def test_the_summary_says_which_credential_actually_pays(self) -> None: + """A tier name is not an answer to "whose key is this run spending?".""" + self._preflight(CB_IN_LLM="anthropic", CB_IN_ANTHROPIC_API_KEY="k", CB_IN_LICENSE_KEY="lic") + summary = (Path(self.temp_dir.name) / "summary.md").read_text(encoding="utf-8") + self.assertIn("your own Anthropic key, called directly", summary) + self.assertIn("not spent", summary, "the licence's status must be stated, not implied") + + def test_the_summary_names_a_non_default_endpoint(self) -> None: + """The setting most likely to be wrong and least likely to be noticed.""" + self._preflight(CB_IN_LLM="openai", CB_IN_OPENAI_API_KEY="k", CB_IN_OPENAI_BASE_URL="https://gw.example/v1") + summary = (Path(self.temp_dir.name) / "summary.md").read_text(encoding="utf-8") + self.assertIn("https://gw.example/v1", summary) + def test_direct_provider_runs_start_no_relay(self) -> None: temp_dir = Path(self.temp_dir.name) fake_bin = temp_dir / "bin" diff --git a/tests/test_action_inputs.py b/tests/test_action_inputs.py index bf8aeef..6a3bb50 100644 --- a/tests/test_action_inputs.py +++ b/tests/test_action_inputs.py @@ -74,6 +74,16 @@ 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_a_crashed_credential_check_still_stops_the_run(self) -> None: + """The check is `continue-on-error` so a refusal can be reported before the job + dies. That same flag would let a crash in it through: no `error` output written, + so a condition keyed only on the code is false and the run reaches the checkout + and the engine install. The stop must also watch the step's outcome.""" + start = ACTION.index("- name: Stop on LLM configuration failure") + condition = ACTION[start : ACTION.index("run:", start)] + self.assertIn("steps.llm.outputs.error != ''", condition) + self.assertIn("steps.llm.outcome != 'success'", condition) + 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()`. From 0b8f29ffa30441ed4311479e6ceff2af3605f596 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Thu, 27 Aug 2026 20:29:54 +0200 Subject: [PATCH 11/15] feat(review): put the review on the run page as well as the pull request The comment is the product, but it is not always reachable. A manual dispatch has no pull request to comment on, and a token without `pull-requests: write` cannot write one, so the diagram had nowhere to go on either. The same body now also goes to the job summary, which before this change was blank for every review run. It is free in both senses that matter: GitHub excludes logs and job summaries from the artifact storage allowance outright, and the 1MiB per-step cap is measured against diagrams of a few kilobytes (1.4KB on this repository, 2.6KB on Core's five-component graph). `continue-on-error`, because a summary that fails to write must never fail a review that succeeded. Co-Authored-By: Claude Opus 5 (1M context) --- action.yml | 13 +++++++++++++ tests/test_action_inputs.py | 12 ++++++++++++ 2 files changed, 25 insertions(+) diff --git a/action.yml b/action.yml index f9f32ce..c651c20 100644 --- a/action.yml +++ b/action.yml @@ -627,6 +627,19 @@ runs: GITHUB_TOKEN: ${{ inputs.github_token }} path: ${{ steps.review_body.outputs.path }} + # The same body, on the run page. Job summaries and logs are explicitly excluded from + # the artifact storage allowance, and the cap is 1MiB per step against a diagram + # measured in single-digit kilobytes, so this is free in both senses. It earns its + # place on the triggers where the comment cannot be the record: a manual dispatch, or + # any run whose comment write fails on a token without `pull-requests: write`. + - name: Add the review to the job summary + if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' && steps.review_body.outputs.path != '' + continue-on-error: true + shell: bash + env: + BODY: ${{ steps.review_body.outputs.path }} + run: cat "$BODY" >> "$GITHUB_STEP_SUMMARY" + # 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 diff --git a/tests/test_action_inputs.py b/tests/test_action_inputs.py index 6a3bb50..ec5d246 100644 --- a/tests/test_action_inputs.py +++ b/tests/test_action_inputs.py @@ -74,6 +74,18 @@ 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_review_reaches_the_job_summary_as_well_as_the_comment(self) -> None: + """The comment is the product, but it is not always reachable: a manual dispatch has + no pull request, and a token without `pull-requests: write` cannot write one. Job + summaries are excluded from the artifact storage allowance, so the record is free.""" + start = ACTION.index("- name: Add the review to the job summary") + block = ACTION[start : ACTION.index("- name: Post review failure", start)] + self.assertIn("GITHUB_STEP_SUMMARY", block) + self.assertIn("steps.review_body.outputs.path", block) + # Never fail a good review because the summary write did not work. + self.assertIn("continue-on-error: true", block) + self.assertLess(ACTION.index("- name: Post review comment"), start, "comment first") + def test_a_crashed_credential_check_still_stops_the_run(self) -> None: """The check is `continue-on-error` so a refusal can be reported before the job dies. That same flag would let a crash in it through: no `error` output written, From c668bf896883e5d5c4ccab149254c3dd28ab6bfb Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Thu, 27 Aug 2026 20:38:43 +0200 Subject: [PATCH 12/15] fix(review): require both OIDC variables, and stop claiming a key that is not there Two from the review, both mine: - `_require_id_token` checked only ACTIONS_ID_TOKEN_REQUEST_URL while the relay refuses to start without the token as well, and a runner can expose one alone. I made this reachable by removing the equivalent check from configure-auth.sh as unreachable: it was not, it covered exactly this case. So a half-configured runner passed preflight and failed generically after the engine install, which is what the check exists to prevent. - The new credential reporting said "your own OpenAI key" for endpoint-only runs that resolve no key at all: `llm: openai` with just a base URL, and every keyless ollama or litellm run. with-auth.sh strips any inherited key, so the phrase named a credential that was not there. The headline is now written once, by the check, and the shell echoes it instead of restating it. That duplication is how the two came to disagree; a test asserts the summary's phrase appears in the headline. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/action/credential_check.py | 44 +++++++++++++++++++++++----- scripts/action/verify-credentials.sh | 14 +++------ tests/test_llm_contract.py | 41 +++++++++++++++++++++++++- 3 files changed, 80 insertions(+), 19 deletions(-) diff --git a/scripts/action/credential_check.py b/scripts/action/credential_check.py index 683939e..d3793b6 100755 --- a/scripts/action/credential_check.py +++ b/scripts/action/credential_check.py @@ -164,7 +164,11 @@ def _reject_provider_inputs(table: dict, given: dict[str, str], llm: str, tier: def _require_id_token(llm: str, environ: dict[str, str]) -> None: - if environ.get("ACTIONS_ID_TOKEN_REQUEST_URL"): + # Both, because the relay needs both (oidc_relay.py refuses to start without either) + # and a runner can expose one without the other. Checking only the URL let that case + # through preflight and turned it into a generic failure after the engine install, + # which is the whole thing this check exists to prevent. + if environ.get("ACTIONS_ID_TOKEN_REQUEST_URL") and environ.get("ACTIONS_ID_TOKEN_REQUEST_TOKEN"): return raise ConfigError( "missing_id_token", @@ -301,6 +305,35 @@ def _is_endpoint(var: str) -> bool: return var.endswith(("_BASE_URL", "_HOST")) or var == "AWS_DEFAULT_REGION" +def _pays(table: dict, plan: dict) -> str: + """What is actually paying for this run's tokens, in one phrase. + + Endpoint-only runs are the reason this is not just the tier name. `llm: openai` with a + base URL and no key, and every keyless ollama or litellm run, resolve with no API key + at all -- with-auth.sh strips any inherited one -- so "your own OpenAI key" would name + a credential that is not there. + """ + tier, provider = plan["tier"], plan["provider"] + if tier == "hosted": + return "CodeBoarding's hosted free tier" + if tier == "license": + return "CodeBoarding's hosted tier, on your plan" + entry = table["providers"].get(provider, {}) + label = entry.get("label", provider) + key_envs = {var for i, var in entry.get("inputs", {}).items() if i.endswith("_api_key")} + if any(plan["env"].get(var) for var in key_envs): + return f"your own {label} key, called directly" + return f"your own {label} endpoint, called directly with no API key" + + +def plan_headline(table: dict, plan: dict) -> str: + """The one line the log opens with. Same source as the summary, so they cannot drift.""" + sentence = f"CodeBoarding is running on {_pays(table, plan)}." + if plan["tier"] == "byok+license": + return sentence + " The wired CodeBoarding plan is not spent on a direct call." + return sentence + + def plan_summary(table: dict, plan: dict) -> list[tuple[str, str]]: """What this run is actually about to do, for the job summary. @@ -310,14 +343,8 @@ def plan_summary(table: dict, plan: dict) -> list[tuple[str, str]]: not spent; saying only "byok+license" leaves that ambiguous, so it is spelled out. """ tier, provider = plan["tier"], plan["provider"] - label = table["providers"].get(provider, {}).get("label", provider) rows = [("Tier", f"`{tier}`"), ("Provider", f"`{provider}`")] - if tier == "hosted": - rows.append(("Credentials", "CodeBoarding's hosted free tier")) - elif tier == "license": - rows.append(("Credentials", "CodeBoarding's hosted tier, on your plan")) - else: - rows.append(("Credentials", f"your own {label} key, called directly")) + rows.append(("Credentials", _pays(table, plan))) if tier == "byok+license": rows.append( ( @@ -408,6 +435,7 @@ def main(argv: list[str]) -> int: "tier": plan["tier"], "provider": plan["provider"], "summary": "\n".join(f"| {k} | {v} |" for k, v in plan_summary(table, plan)), + "headline": plan_headline(table, plan), }, sys.stdout, ) diff --git a/scripts/action/verify-credentials.sh b/scripts/action/verify-credentials.sh index 68ec5ae..6963142 100755 --- a/scripts/action/verify-credentials.sh +++ b/scripts/action/verify-credentials.sh @@ -78,16 +78,10 @@ if [ -d "$AUTH_DIR/env" ]; then 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" ;; - # Named precisely: the plan is wired but a direct provider call never reaches us, so - # "with a CodeBoarding plan" would imply the plan is paying for something here. - byok+license) label="your own ${provider} key (the wired CodeBoarding plan is not spent on a direct call)" ;; - *) label="${tier}" ;; -esac -echo "CodeBoarding is running on ${label}." +# Written by the check, not restated here. The same phrase feeds the summary, so the log +# and the summary cannot end up claiming different things -- which they did: this said +# "your own openai key" for an endpoint-only run that resolved no key at all. +field headline # The rows come from the check, which is the only thing that knows what it resolved. { echo "### CodeBoarding configuration" diff --git a/tests/test_llm_contract.py b/tests/test_llm_contract.py index 6f9eb86..de85b87 100644 --- a/tests/test_llm_contract.py +++ b/tests/test_llm_contract.py @@ -16,7 +16,11 @@ credential_check = importlib.util.module_from_spec(_spec) _spec.loader.exec_module(credential_check) -OIDC = {"ACTIONS_ID_TOKEN_REQUEST_URL": "https://oidc.example/token"} +# Both, since the relay needs both and the check now says so. +OIDC = { + "ACTIONS_ID_TOKEN_REQUEST_URL": "https://oidc.example/token", + "ACTIONS_ID_TOKEN_REQUEST_TOKEN": "request-token", +} class ContractTests(unittest.TestCase): @@ -151,6 +155,41 @@ def test_license_without_a_licence_key_is_refused(self) -> None: self.assertEqual(error.code, "missing_license_key") self.assertIn("CODEBOARDING_LICENSE", error.message) + def test_hosted_tiers_require_both_oidc_variables(self) -> None: + """The relay refuses to start without either, and a runner can expose one alone. + + Checking only the URL let that through preflight and turned a detectable + configuration error into a generic failure after the engine install. + """ + for extra in ({}, {"ACTIONS_ID_TOKEN_REQUEST_URL": "https://oidc.example/token"}): + with self.subTest(present=sorted(extra)): + error = self.refuse(CB_IN_LLM="hosted", **extra) + self.assertEqual(error.code, "missing_id_token") + # Both present is the only accepted shape. + plan = self.resolve( + CB_IN_LLM="hosted", + ACTIONS_ID_TOKEN_REQUEST_URL="https://oidc.example/token", + ACTIONS_ID_TOKEN_REQUEST_TOKEN="request-token", + ) + self.assertEqual(plan["tier"], "hosted") + + def test_an_endpoint_only_run_is_not_described_as_using_a_key(self) -> None: + """`llm: openai` with only a base URL resolves no key, and with-auth.sh strips any + inherited one, so naming a key would name a credential that is not there.""" + table = self.table + keyless = self.resolve(CB_IN_LLM="openai", CB_IN_OPENAI_BASE_URL="https://gw.example/v1") + self.assertIn("no API key", credential_check.plan_headline(table, keyless)) + self.assertNotIn("key, called directly", credential_check.plan_headline(table, keyless)) + + keyed = self.resolve(CB_IN_LLM="openai", CB_IN_OPENAI_API_KEY="sk-x") + self.assertIn("your own OpenAI key, called directly", credential_check.plan_headline(table, keyed)) + + def test_the_headline_and_the_summary_cannot_disagree(self) -> None: + """Both render from the same phrase; they were separately written and drifted.""" + plan = self.resolve(CB_IN_LLM="ollama", CB_IN_OLLAMA_BASE_URL="http://localhost:11434") + rows = dict(credential_check.plan_summary(self.table, plan)) + self.assertIn(rows["Credentials"], credential_check.plan_headline(self.table, plan)) + 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): From 107f35bfc2486e6f24cca6a2c8ebc7dec486864e Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Thu, 27 Aug 2026 20:44:18 +0200 Subject: [PATCH 13/15] fix(review): do not tell hosted users which upstream we route them to A licensed run reported `Provider: openrouter`, which is CodeBoarding's routing decision rather than the user's configuration. It invites "why does my CodeBoarding plan say openrouter?", and it implies a commitment we have not made: the proxy's upstream can change without notice. The provider row and the `llm_provider` output are now empty on `hosted` and `license`, and populated whenever the key is the user's own, where it is their configuration and the first thing they would check. Only the reporting is withheld. The plan still resolves a provider, because the analysis has to be pointed somewhere, and the backend id still separates hosted from licensed from bring-your-own-OpenRouter, so a reusable analysis is never restored across them. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 5 +++-- scripts/action/credential_check.py | 23 ++++++++++++++++++++--- tests/test_llm_contract.py | 27 +++++++++++++++++++++++++++ 3 files changed, 50 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 854ff0d..ea4a95f 100644 --- a/README.md +++ b/README.md @@ -184,7 +184,8 @@ to Core and silently stay unreachable here. 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 +- outputs `llm_tier` (`hosted`, `license`, `byok`, `byok+license`), `llm_provider` (empty + on the hosted tiers, where the upstream is ours rather than yours), 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 @@ -316,7 +317,7 @@ The `/codeboarding` command, comment heading, Mermaid direction (`LR`), hosted w | Output | Mode | Description | |---|---|---| | `llm_tier` | both | `hosted`, `license`, `byok`, or `byok+license`. | -| `llm_provider` | both | Provider the run used. | +| `llm_provider` | both | Provider the run used. Empty on `hosted` and `license`: which upstream the proxy routes to is CodeBoarding's decision, not your configuration. | | `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. | diff --git a/scripts/action/credential_check.py b/scripts/action/credential_check.py index d3793b6..a6e918a 100755 --- a/scripts/action/credential_check.py +++ b/scripts/action/credential_check.py @@ -326,6 +326,20 @@ def _pays(table: dict, plan: dict) -> str: return f"your own {label} endpoint, called directly with no API key" +def reported_provider(plan: dict) -> str: + """The provider to tell the user about, which is none on the hosted tiers. + + Hosted and licensed runs go through CodeBoarding's proxy, and which upstream sits + behind it is our routing decision, not the user's configuration. Naming it invites + "why does my CodeBoarding plan say openrouter?", and worse, implies a commitment we + have not made: we can change what the proxy routes to without telling anyone. + + It stays in the plan, because the analysis still has to be pointed somewhere and the + reusable-analysis identity has to separate the tiers. Only the reporting is withheld. + """ + return "" if plan["tier"] in ("hosted", "license") else plan["provider"] + + def plan_headline(table: dict, plan: dict) -> str: """The one line the log opens with. Same source as the summary, so they cannot drift.""" sentence = f"CodeBoarding is running on {_pays(table, plan)}." @@ -342,8 +356,11 @@ def plan_summary(table: dict, plan: dict) -> list[tuple[str, str]]: never reaches CodeBoarding, so a licence wired beside your own key is recorded and not spent; saying only "byok+license" leaves that ambiguous, so it is spelled out. """ - tier, provider = plan["tier"], plan["provider"] - rows = [("Tier", f"`{tier}`"), ("Provider", f"`{provider}`")] + tier = plan["tier"] + rows = [("Tier", f"`{tier}`")] + shown = reported_provider(plan) + if shown: + rows.append(("Provider", f"`{shown}`")) rows.append(("Credentials", _pays(table, plan))) if tier == "byok+license": rows.append( @@ -433,7 +450,7 @@ def main(argv: list[str]) -> int: "message": "", "details": "", "tier": plan["tier"], - "provider": plan["provider"], + "provider": reported_provider(plan), "summary": "\n".join(f"| {k} | {v} |" for k, v in plan_summary(table, plan)), "headline": plan_headline(table, plan), }, diff --git a/tests/test_llm_contract.py b/tests/test_llm_contract.py index de85b87..8fff35e 100644 --- a/tests/test_llm_contract.py +++ b/tests/test_llm_contract.py @@ -173,6 +173,33 @@ def test_hosted_tiers_require_both_oidc_variables(self) -> None: ) self.assertEqual(plan["tier"], "hosted") + def test_a_hosted_run_does_not_name_the_upstream_it_is_routed_to(self) -> None: + """Which provider sits behind CodeBoarding's proxy is our routing decision. + + Reporting it invites "why does my plan say openrouter?", and implies a commitment + we have not made: the proxy's upstream can change without notice. The plan keeps + it, because the analysis still has to be pointed somewhere; only the reporting + withholds it. + """ + for environ in ( + {"CB_IN_LLM": "hosted", **OIDC}, + {"CB_IN_LLM": "license", "CB_IN_LICENSE_KEY": "lic", **OIDC}, + ): + plan = self.resolve(**environ) + with self.subTest(tier=plan["tier"]): + self.assertEqual(credential_check.reported_provider(plan), "") + rendered = dict(credential_check.plan_summary(self.table, plan)) + self.assertNotIn("Provider", rendered) + self.assertNotIn("openrouter", credential_check.plan_headline(self.table, plan)) + # Still resolved internally: the run has to be pointed somewhere. + self.assertEqual(plan["provider"], "openrouter") + + def test_your_own_provider_is_always_named(self) -> None: + """It is your configuration, and it is the thing you would check first.""" + plan = self.resolve(CB_IN_LLM="anthropic", CB_IN_ANTHROPIC_API_KEY="k") + self.assertEqual(credential_check.reported_provider(plan), "anthropic") + self.assertEqual(dict(credential_check.plan_summary(self.table, plan))["Provider"], "`anthropic`") + def test_an_endpoint_only_run_is_not_described_as_using_a_key(self) -> None: """`llm: openai` with only a base URL resolves no key, and with-auth.sh strips any inherited one, so naming a key would name a credential that is not there.""" From 4a5209b10aebbe58c359fe57469fd3f5b0a3a069 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Thu, 27 Aug 2026 21:02:17 +0200 Subject: [PATCH 14/15] fix(review): stop corrupting keys that contain '=', and never publish an endpoint's credentials Two from the review, both reachable: - `_clean_key` stripped any leading `[A-Z0-9_]+=`, meant for a key pasted as `ANTHROPIC_API_KEY=sk-...`. It also ate the start of real credentials: `ABC=DEF` became `DEF`, and the base64 token `AWSKEY123=` became the empty string, which preflight then reported as a key the user had not set. That is the misleading failure this whole contract exists to remove, and Bedrock bearer tokens are base64, so it was reachable. Only the input's own variable name is stripped now. - The summary published endpoint values verbatim, and endpoints are deliberately not masked because a wrong one is the thing you most want to see. A custom endpoint is user-supplied text that can carry authentication in userinfo or a query, and the job summary renders on the run page, which is public for a public repository. Published endpoints keep their scheme, host and path, and lose anything that could be a credential. The value the analysis uses is untouched; only the reporting is trimmed. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/action/credential_check.py | 50 ++++++++++++++++++++++++------ tests/test_llm_contract.py | 39 +++++++++++++++++++++++ 2 files changed, 80 insertions(+), 9 deletions(-) diff --git a/scripts/action/credential_check.py b/scripts/action/credential_check.py index a6e918a..8bd1949 100755 --- a/scripts/action/credential_check.py +++ b/scripts/action/credential_check.py @@ -25,6 +25,7 @@ import re import sys from pathlib import Path +from urllib.parse import urlsplit, urlunsplit DOCS = "https://github.com/CodeBoarding/CodeBoarding-action#authentication-and-providers" SETTINGS_HINT = "Settings -> Secrets and variables -> Actions" @@ -113,26 +114,36 @@ 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) +def _unquote(value: str) -> str: for _ in range(2): value = re.sub(r'^"(.*)"$', r"\1", value) value = re.sub(r"^'(.*)'$", r"\1", value) return value +def _clean_key(raw: str, env_var: str) -> str: + """Undo the ways a pasted key arrives wrapped: whitespace, quotes, a `VAR=` prefix. + + Only THIS input's own variable name is stripped. Matching any `[A-Z0-9_]+=` prefix + corrupted real credentials: a base64 token like `AWSKEY123=` was reduced to the empty + string and then reported as a key the user had not set, which is precisely the + misleading failure this contract exists to remove. Bedrock bearer tokens are base64, + so that was reachable, not theoretical. + """ + value = _unquote(re.sub(r"\s+", "", raw)) + if value.startswith(f"{env_var}="): + value = _unquote(value[len(env_var) + 1 :]) + 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() + env_var = provider["inputs"][input_name] + value = _clean_key(raw, env_var) if input_name.endswith("_api_key") else raw.strip() if value: values[input_name] = value return values @@ -305,6 +316,27 @@ def _is_endpoint(var: str) -> bool: return var.endswith(("_BASE_URL", "_HOST")) or var == "AWS_DEFAULT_REGION" +def _safe_endpoint(value: str) -> str: + """An endpoint with any embedded credential removed, for publishing. + + The job summary renders on the run page, which is public for a public repository, and + an endpoint is user-supplied text that can carry authentication: `https://u:tok@host` + or `?api-key=...`. Endpoints are not masked, deliberately, because a wrong one is the + thing you most want to see. So publish the part that helps you spot a mistake, the + scheme, host and path, and drop the parts that only carry secrets. + """ + split = urlsplit(value) + if not split.scheme or not split.netloc: + # Not a URL (OLLAMA_HOST is often `host:11434`), so it carries no userinfo unless + # someone wrote one; if they did, say nothing rather than guess where it ends. + return "(hidden)" if "@" in value else value + host = split.netloc.rsplit("@", 1)[-1] + redacted = urlunsplit((split.scheme, host, split.path, "", "")) + if split.username or split.query: + redacted += " (credentials removed)" + return redacted + + def _pays(table: dict, plan: dict) -> str: """What is actually paying for this run's tokens, in one phrase. @@ -373,7 +405,7 @@ def plan_summary(table: dict, plan: dict) -> list[tuple[str, str]]: # setting most likely to be wrong and least likely to be noticed. for var, value in sorted(plan["env"].items()): if _is_endpoint(var): - rows.append((f"`{var}`", f"`{value}`")) + rows.append((f"`{var}`", f"`{_safe_endpoint(value)}`")) return rows diff --git a/tests/test_llm_contract.py b/tests/test_llm_contract.py index 8fff35e..71d5e9a 100644 --- a/tests/test_llm_contract.py +++ b/tests/test_llm_contract.py @@ -173,6 +173,45 @@ def test_hosted_tiers_require_both_oidc_variables(self) -> None: ) self.assertEqual(plan["tier"], "hosted") + def test_a_key_containing_an_equals_sign_survives_intact(self) -> None: + """Stripping any `[A-Z0-9_]+=` prefix corrupted real credentials. + + `AWSKEY123=` became the empty string and was then reported as a key the user had + not set, which is exactly the misleading failure this contract removes. Bedrock + bearer tokens are base64, so it was reachable rather than theoretical. + """ + for raw in ("QUJDRA==", "ABC=DEF", "AWSKEY123=", "sk-ant-abc123"): + with self.subTest(key=raw): + plan = self.resolve(CB_IN_LLM="anthropic", CB_IN_ANTHROPIC_API_KEY=raw) + self.assertEqual(plan["env"]["ANTHROPIC_API_KEY"], raw) + # The paste it was written for still works: this input's own variable, and no other. + pasted = self.resolve(CB_IN_LLM="anthropic", CB_IN_ANTHROPIC_API_KEY="ANTHROPIC_API_KEY=sk-real") + self.assertEqual(pasted["env"]["ANTHROPIC_API_KEY"], "sk-real") + other = self.resolve(CB_IN_LLM="anthropic", CB_IN_ANTHROPIC_API_KEY="OPENAI_API_KEY=sk-x") + self.assertEqual(other["env"]["ANTHROPIC_API_KEY"], "OPENAI_API_KEY=sk-x") + + def test_a_published_endpoint_never_carries_its_credentials(self) -> None: + """The job summary renders on the run page, public for a public repository, and an + endpoint is user-supplied text that can carry authentication. Endpoints are not + masked on purpose, so what is published has to be safe by construction.""" + for value in ( + "https://user:s3cret@gw.example/v1", + "https://gw.example/v1?api-key=s3cret", + ): + with self.subTest(endpoint=value): + plan = self.resolve(CB_IN_LLM="openai", CB_IN_OPENAI_API_KEY="k", CB_IN_OPENAI_BASE_URL=value) + rendered = "\n".join(v for _, v in credential_check.plan_summary(self.table, plan)) + self.assertNotIn("s3cret", rendered) + self.assertIn("gw.example", rendered, "the host still has to be checkable") + # The value the analysis uses is untouched; only the reporting is trimmed. + self.assertEqual(plan["env"]["OPENAI_BASE_URL"], value) + + def test_an_ordinary_endpoint_is_published_unchanged(self) -> None: + """Redaction that mangles a correct endpoint would defeat the point of showing it.""" + plan = self.resolve(CB_IN_LLM="ollama", CB_IN_OLLAMA_BASE_URL="http://localhost:11434") + rendered = dict(credential_check.plan_summary(self.table, plan)) + self.assertEqual(rendered["`OLLAMA_BASE_URL`"], "`http://localhost:11434`") + def test_a_hosted_run_does_not_name_the_upstream_it_is_routed_to(self) -> None: """Which provider sits behind CodeBoarding's proxy is our routing decision. From 7460bec385c2e4814cab96caca7ec7eeeff03597 Mon Sep 17 00:00:00 2001 From: Svilen Stefanov Date: Thu, 27 Aug 2026 21:02:52 +0200 Subject: [PATCH 15/15] test: crashed check (temporary) --- scripts/action/supported-providers.json | 157 ------------------------ 1 file changed, 157 deletions(-) delete mode 100644 scripts/action/supported-providers.json diff --git a/scripts/action/supported-providers.json b/scripts/action/supported-providers.json deleted file mode 100644 index 213cebd..0000000 --- a/scripts/action/supported-providers.json +++ /dev/null @@ -1,157 +0,0 @@ -{ - "_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.", - "", - "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", - "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", - "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_bedrock": { - "core": "aws", - "label": "AWS Bedrock", - "selection_envs": [ - "AWS_BEARER_TOKEN_BEDROCK" - ], - "inputs": { - "aws_bedrock_api_key": "AWS_BEARER_TOKEN_BEDROCK", - "aws_bedrock_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" - } - } - } -}