diff --git a/.github/workflows/codeboarding-sync.yml b/.github/workflows/codeboarding-sync.yml index 2700bc0..2660515 100644 --- a/.github/workflows/codeboarding-sync.yml +++ b/.github/workflows/codeboarding-sync.yml @@ -141,6 +141,9 @@ jobs: - uses: ./ with: mode: sync + # 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 16330cc..5bdfb56 100644 --- a/.github/workflows/codeboarding.yml +++ b/.github/workflows/codeboarding.yml @@ -111,4 +111,10 @@ 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. CodeBoarding's own repositories run on the CodeBoarding plan. + llm: openrouter + openrouter_api_key: ${{ secrets.OPENROUTER_API_KEY }} + 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/.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..fd9fbbd 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/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. + +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..ea4a95f 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,96 @@ 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". **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 + +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_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` | +| `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` | + +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/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. + +### 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` (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 + comment with the fix, so the person who has to add the secret sees it where they are. ## Model selection @@ -189,9 +258,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 +279,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 +296,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_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`. | | `parsing_model` | both | empty | Parsing-only override for `model`. | @@ -237,12 +310,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. 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. | | `truncated` | review | Whether the graph was reduced to fit GitHub limits. | diff --git a/action.yml b/action.yml index 220246e..c651c20 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_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.' 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_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_bedrock_region: + description: 'AWS Bedrock region, used when llm is aws_bedrock. 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,91 @@ 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 + # 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_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 }} + 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/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. + - 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.details }} + + 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 }} + + # 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 != '' || steps.llm.outcome != 'success') + shell: bash + env: + CB_ERROR: ${{ steps.llm.outputs.error }} + run: | + 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 if: steps.guard.outputs.skip != 'true' && steps.guard.outputs.mode == 'review' continue-on-error: true @@ -156,12 +342,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 @@ -188,9 +368,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 +380,8 @@ 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 }} + BACKEND_ID: ${{ steps.llm.outputs.backend_id }} MODEL: ${{ inputs.model }} AGENT_MODEL_INPUT: ${{ inputs.agent_model }} PARSING_MODEL_INPUT: ${{ inputs.parsing_model }} @@ -449,8 +627,25 @@ 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 + # 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/scripts/action/configure-auth.sh b/scripts/action/configure-auth.sh index 0a5beda..6abcc9d 100755 --- a/scripts/action/configure-auth.sh +++ b/scripts/action/configure-auth.sh @@ -1,44 +1,32 @@ #!/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 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 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 -fi -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 + +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 + 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 +47,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/credential_check.py b/scripts/action/credential_check.py new file mode 100755 index 0000000..8bd1949 --- /dev/null +++ b/scripts/action/credential_check.py @@ -0,0 +1,496 @@ +#!/usr/bin/env python3 +"""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 -- 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. + +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 +from urllib.parse import urlsplit, urlunsplit + +DOCS = "https://github.com/CodeBoarding/CodeBoarding-action#authentication-and-providers" +SETTINGS_HINT = "Settings -> Secrets and variables -> Actions" +TABLE = Path(__file__).resolve().parent / "supported-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. + + 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: + return json.loads(path.read_text(encoding="utf-8")) + + +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()}", "") + 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 + + +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: + # 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", + 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], environ: 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 `{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 + + +def resolve(table: dict, environ: dict[str, str]) -> dict: + """The whole contract. Returns a plan; raises ConfigError with the reason otherwise.""" + llm = 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"], "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.", + "\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 { + "tier": "license", + "provider": table["hosted_provider"], + "license": license_key, + "env": {}, + } + + name = 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, 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. + return { + "tier": "byok+license" if license_key else "byok", + "provider": name, + "env": env, + } + + +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 _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. + + 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 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)}." + 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. + + "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 = 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( + ( + "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"`{_safe_endpoint(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) + 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 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() + for var in list(provider["selection_envs"]) + list(provider["inputs"].values()) + } + # 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: + 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, + "details": error.details, + }, + sys.stdout, + ) + print() + return 1 + + if args.auth_dir: + write_auth_dir(table, plan, args.auth_dir) + json.dump( + { + "ok": True, + "error": "", + "message": "", + "details": "", + "tier": plan["tier"], + "provider": reported_provider(plan), + "summary": "\n".join(f"| {k} | {v} |" for k, v in plan_summary(table, plan)), + "headline": plan_headline(table, plan), + }, + sys.stdout, + ) + print() + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/scripts/action/state-names.sh b/scripts/action/state-names.sh index a6379c7..e91f3a0 100755 --- a/scripts/action/state-names.sh +++ b/scripts/action/state-names.sh @@ -30,8 +30,11 @@ fi ignore_digest=none ignore_file="$CHECKOUT_DIR/.codeboarding/.codeboardingignore" [ ! -f "$ignore_file" ] || ignore_digest="$(digest < "$ignore_file")" -model_digest="$(printf '%s\n%s\n%s\n%s\n' \ - "${LLM_PROVIDER:-}" "${MODEL:-}" "${AGENT_MODEL_INPUT:-}" "${PARSING_MODEL_INPUT:-}" | digest)" +# BACKEND_ID covers the tier and any endpoint or region the run was pointed at, so moving +# `openai_base_url` to another gateway invalidates a warm start the way changing the model +# does. It carries no key: rotating a secret must not throw away reusable analysis. +model_digest="$(printf '%s\n%s\n%s\n%s\n%s\n' \ + "${LLM_PROVIDER:-}" "${BACKEND_ID:-}" "${MODEL:-}" "${AGENT_MODEL_INPUT:-}" "${PARSING_MODEL_INPUT:-}" | digest)" cfg="$(printf '%s\n%s\n%s\n%s\n' \ "$STATE_SCHEMA" "$engine_version" "$ignore_digest" "$model_digest" | digest)" diff --git a/scripts/action/supported-providers.json b/scripts/action/supported-providers.json new file mode 100644 index 0000000..213cebd --- /dev/null +++ b/scripts/action/supported-providers.json @@ -0,0 +1,157 @@ +{ + "_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" + } + } + } +} diff --git a/scripts/action/verify-credentials.sh b/scripts/action/verify-credentials.sh new file mode 100755 index 0000000..6963142 --- /dev/null +++ b/scripts/action/verify-credentials.sh @@ -0,0 +1,92 @@ +#!/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/credential_check.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)" +details="$(field details)" + +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<> "$GITHUB_OUTPUT" + +if [ "$resolved" -ne 0 ]; then + echo "::error title=CodeBoarding LLM configuration::${message}" + { + echo "### CodeBoarding could not start" + echo + echo "$details" + 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")" + +# 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" + echo + echo "| | |" + echo "|---|---|" + field summary +} >> "${GITHUB_STEP_SUMMARY:-/dev/null}" diff --git a/scripts/action/with-auth.sh b/scripts/action/with-auth.sh index f1dc636..1b4f1ad 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,30 @@ 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 + # `|| [ -n "$selector" ]` so a file whose last line is unterminated still strips that + # entry: `read` reports failure at EOF even when it read a partial line, and losing the + # last variable silently is exactly how a second provider stays configured. + while IFS= read -r selector || [ -n "$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..1cf2568 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,253 @@ import unittest from pathlib import Path - ROOT = Path(__file__).resolve().parent.parent +PREFLIGHT = ROOT / "scripts" / "action" / "verify-credentials.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_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. - 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") + 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"], "") + + 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) - def test_normalizes_pasted_provider_key_wrappers(self) -> None: - result, auth_dir = self._configure("openrouter", " 'OPENROUTER_API_KEY=\"fake-key\"' \n") + 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_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: + """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")) - def test_keyless_direct_provider_does_not_fall_back_to_hosted_openrouter(self) -> None: - result, auth_dir = self._configure("ollama", "") + 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) - self.assertEqual((auth_dir / "provider-env").read_text(), "OLLAMA_API_KEY") - self.assertFalse((auth_dir / "provider-key").exists()) + 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_hosted_auth_relays_to_aws_proxy_instead_of_openrouter(self) -> None: + 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) + # 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.""" + 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(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) + + # -- 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 +276,125 @@ 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_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() - 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) + 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 = self._configure( - "openrouter", - "", + 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", - LICENSE_KEY="license", - RELAY_PID=str(relay_pid), - PATH=f"{fake_bin}:{os.environ['PATH']}", ) + 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()) - 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) + 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") - 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) + 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) - self.assertFalse(auth_dir.exists()) - 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) + 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) - scoped = subprocess.run( - [str(WITH_AUTH), "true"], - env={"PATH": os.environ["PATH"], "RUNNER_TEMP": str(auth_dir.parent)}, + 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() + 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) + + 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..ec5d246 --- /dev/null +++ b/tests/test_action_inputs.py @@ -0,0 +1,126 @@ +"""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" / "supported-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_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, + 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()`. + + 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") + 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..71d5e9a --- /dev/null +++ b/tests/test_llm_contract.py @@ -0,0 +1,354 @@ +"""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("credential_check", ROOT / "scripts" / "action" / "credential_check.py") +credential_check = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(credential_check) + +# 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): + def setUp(self) -> None: + self.table = credential_check.load_table() + + def resolve(self, **environ: str) -> dict: + return credential_check.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 --------------------------------------------------- + + 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_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_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") + 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_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") + 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_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_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. + + 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.""" + 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): + 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_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 + 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(credential_check.ERROR_CODES)) + + 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..13d0a0b --- /dev/null +++ b/tests/test_provider_table_drift.py @@ -0,0 +1,111 @@ +"""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 importlib import metadata +from pathlib import Path + +ROOT = Path(__file__).resolve().parent.parent +TABLE = json.loads((ROOT / "scripts" / "action" / "supported-providers.json").read_text(encoding="utf-8")) + + +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() + + +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: + 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), + "supported-providers.json records a different release than action.yml installs", + ) + + def test_the_same_providers_exist_on_both_sides(self) -> None: + self.assertEqual( + sorted(core_name(n) for n in 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[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[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): + 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_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(core_name(TABLE["hosted_provider"]), CORE_PROVIDERS) + + +if __name__ == "__main__": + unittest.main()