TEMP: verify at 4a5209b (v2/crashed-check) - #114
Closed
Svilen-Stefanov wants to merge 15 commits into
Closed
Conversation
…dentials The action resolved credentials by precedence: a provider key if present, a licence if present, otherwise CodeBoarding's hosted tier. An empty value was therefore indistinguishable from "no preference", so a repository that selected Anthropic and had not added its secret yet ran green on CodeBoarding's hosted OpenRouter tier -- a different vendor, a different model, our money -- and nothing in the run said so. Credentials are now named, not inferred. `llm` is required and takes `hosted`, `license`, or a provider name; each provider has its own `<name>_api_key` input. Anything ambiguous is refused: a named provider without its key, a hosted tier carrying a provider key, a licence where it would not be spent, a second provider's key. Refusals happen before the checkout and the engine install, and name the input and the secret to fix. A licence alongside a provider key stays valid and is reported as `byok+license`. Metering that combination needs proxy work and is not in scope. The provider table is mirrored from the pinned engine rather than hand-copied per site, and the foreign-selector list with-auth.sh strips is now derived from it. That list had already fallen behind: it was missing ORCAROUTER_API_KEY, which 0.13.10 added, so an inherited value could select a provider the workflow never asked for. A drift test installs the pinned release in CI and fails when the two disagree. Ships as feat: rather than feat!: on purpose -- see AGENTS.md. A major bump would freeze v1 and leave every existing workflow on the old silent-fallback behaviour permanently, which is the opposite of the intent. BREAKING: `llm_api_key` and `llm_provider` are replaced by `llm` plus per-provider inputs. Workflows that set neither must add `llm: hosted`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ence Four things the Codex review was right about: - The credential check is a Python program and ran before `setup-python`. A hosted runner ships a system python3 and would never notice; a self-hosted one without it would fail a valid configuration. Python is provisioned first now, which still leaves the check ahead of the engine install. - `Post review failure` writes the same sticky comment on `failure()`, so a run stopped for a missing secret posted the fix and then buried it under "see the workflow logs". It now stands down when the credential check is what failed. - The drift test treated any `agents.llm_config` import error as "engine not installed" and skipped, so a pin that moved a module would retire the very check meant to catch it. Installed-ness is asked of the distribution; an import failure after that is raised. - The foreign-variable list spared every variable the selected provider could use rather than the ones the run resolved. `llm: openai` with only `openai_base_url` therefore let an inherited OPENAI_API_KEY credential the run: the same silent substitution this contract removes, one provider narrower. Also switches this repo's own workflows from `llm: hosted` to `llm: license`. CodeBoarding's repositories run on the CodeBoarding plan, and the secret is already configured here. The README examples stay on `hosted`, which is the right starting point for someone reading them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
docs/github-storage-scopes.md is an uncommitted local draft about Actions storage scopes. It has nothing to do with the credential contract and was swept in by a `git add -A`; it belongs wherever its author decides, not here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eaches Aliases were three extra values to document, test and keep in step with the picker, in exchange for accepting a spelling nobody was asked to type. The table is now keyed by the ONE value `llm:` accepts, each provider's inputs are named after it (`llm: X` always pairs with `X_api_key`), and an unrecognised value is refused with the accepted list. Bedrock keeps the friendlier `aws_bedrock` rather than the engine's internal `aws`; a `core` field carries that translation and nothing else does, so the drift test still compares against the engine name exactly. Also removed: the `hosted` field the resolver emitted and nothing read, and the OIDC check in configure-auth.sh, which preflight had already made unreachable. One decision point was the point. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`resolve_llm` described half the job. The module also writes every sentence the user reads about a credential problem: preflight puts the message in the step output, and action.yml posts that same string as the pull request comment, the error annotation and the job summary. Naming it `llm_credentials` and saying so in the docstring keeps the rule and its explanation together, which is the point of having them in one file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three more from the Codex review, all real: - `"\n".join(...)` left foreign-envs unterminated, and `read` reports failure on an unterminated final line, so `while read` never ran its body for that record. VERCEL_BASE_URL sorts last, so an Anthropic run inherited it and core saw two providers configured. The list is terminated and the loop now handles a partial final line, either of which alone would fix it. The existing stripping test passed throughout because none of the variables it named was last, so the new one asserts the last entry specifically. - Endpoint and region are first-class inputs now, but the reusable-analysis name hashed only provider and model, so pointing `openai_base_url` at another gateway could restore a warm start built against the old one. A backend id covering the tier, provider, endpoints and region feeds the state identity. It carries no key: rotating a secret must not throw away reusable analysis, and an artifact name must never be built from one. - The README's input table still named `aws_region` after the rename. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`license_with_provider_key` could be raised and nothing asserted it, which is what happens when a failure surface is only implicit in its raise sites. The codes are declared in one frozenset now, ConfigError asserts membership, and a test walks one configuration per code and asserts the set produced is exactly the set declared, so adding a code without a case fails the build. These codes are an interface: the action emits them as `llm_config_error` and the webview keys on them, so adding one silently was a contract change nobody reviewed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Agreed naming: - supported-providers.json says what the list IS, which is the same set the refusal message recites back when a value is not recognised. - credential_check.py leads with the verb, because refusing with a reason is the primary job. `resolve_llm` named only the resolving half, and the module also writes every sentence the user reads about a credential problem. - verify-credentials.sh matches the verb-noun shape of its siblings (configure-auth, fetch-state, deliver-sync, render-review) rather than being the one noun-first script in the directory. configure-auth.sh keeps its name for now, though after this change it only starts the hosted relay. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"Add the secret and wire it as X" describes the fix rather than being it. The reader is often someone who has never edited a workflow, so a refusal now names the exact secrets page as a link, the exact workflow file, and the YAML to paste, indented as it will sit in the `with:` block. That needs two renderings, because the surfaces differ: `::error::` annotations cannot carry newlines, so `message` stays one plain line for the annotation while `details` carries the markdown for the pull request comment and the job summary. Only the refusals with one exact answer get a snippet. "Remove one of these" has two valid fixes, so picking one would be guessing, and it stays prose. The repository and workflow file come from GITHUB_REPOSITORY and GITHUB_WORKFLOW_REF, so off a runner the remedy degrades to prose rather than emitting a half-built link. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…heck crashes Three things, all from questions the summary could not answer: - `byok+license` runs on YOUR key, always. A direct provider call never reaches CodeBoarding, so there is nothing for a licence to pay for there; it is recorded and not spent. That was already the behaviour and nothing pinned it, so a test now asserts the provider key is exported, no licence is staged, and the relay is never started. - "Tier: byok+license" names the configuration without answering the question a summary is read to answer. The summary now states which credential pays, says outright that a wired licence is not spent on a direct call, and names a non-default endpoint or region, which is the setting most likely to be wrong and least likely to be noticed. - The credential check is `continue-on-error` so a refusal can be reported before the job dies. A crash inside it -- an unreadable table, a missing python3 -- therefore left `error` empty and let the run reach the checkout and the engine install. The stop step watches the step outcome as well, so "credentials are decided before anything expensive happens" holds when the deciding breaks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment is the product, but it is not always reachable. A manual dispatch has no pull request to comment on, and a token without `pull-requests: write` cannot write one, so the diagram had nowhere to go on either. The same body now also goes to the job summary, which before this change was blank for every review run. It is free in both senses that matter: GitHub excludes logs and job summaries from the artifact storage allowance outright, and the 1MiB per-step cap is measured against diagrams of a few kilobytes (1.4KB on this repository, 2.6KB on Core's five-component graph). `continue-on-error`, because a summary that fails to write must never fail a review that succeeded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t is not there Two from the review, both mine: - `_require_id_token` checked only ACTIONS_ID_TOKEN_REQUEST_URL while the relay refuses to start without the token as well, and a runner can expose one alone. I made this reachable by removing the equivalent check from configure-auth.sh as unreachable: it was not, it covered exactly this case. So a half-configured runner passed preflight and failed generically after the engine install, which is what the check exists to prevent. - The new credential reporting said "your own OpenAI key" for endpoint-only runs that resolve no key at all: `llm: openai` with just a base URL, and every keyless ollama or litellm run. with-auth.sh strips any inherited key, so the phrase named a credential that was not there. The headline is now written once, by the check, and the shell echoes it instead of restating it. That duplication is how the two came to disagree; a test asserts the summary's phrase appears in the headline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A licensed run reported `Provider: openrouter`, which is CodeBoarding's routing decision rather than the user's configuration. It invites "why does my CodeBoarding plan say openrouter?", and it implies a commitment we have not made: the proxy's upstream can change without notice. The provider row and the `llm_provider` output are now empty on `hosted` and `license`, and populated whenever the key is the user's own, where it is their configuration and the first thing they would check. Only the reporting is withheld. The plan still resolves a provider, because the analysis has to be pointed somewhere, and the backend id still separates hosted from licensed from bring-your-own-OpenRouter, so a reusable analysis is never restored across them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… an endpoint's credentials Two from the review, both reachable: - `_clean_key` stripped any leading `[A-Z0-9_]+=`, meant for a key pasted as `ANTHROPIC_API_KEY=sk-...`. It also ate the start of real credentials: `ABC=DEF` became `DEF`, and the base64 token `AWSKEY123=` became the empty string, which preflight then reported as a key the user had not set. That is the misleading failure this whole contract exists to remove, and Bedrock bearer tokens are base64, so it was reachable. Only the input's own variable name is stripped now. - The summary published endpoint values verbatim, and endpoints are deliberately not masked because a wrong one is the thing you most want to see. A custom endpoint is user-supplied text that can carry authentication in userinfo or a query, and the job summary renders on the run page, which is public for a public repository. Published endpoints keep their scheme, host and path, and lose anything that could be a credential. The value the analysis uses is untouched; only the reporting is trimmed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
CodeBoarding review · failedSee the workflow logs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7460bec385
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| 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" |
There was a problem hiding this comment.
The new resolver always loads scripts/action/supported-providers.json, but a repo-wide git ls-tree -r --name-only ae3664c confirms that file is absent from this commit. Consequently every non-skipped review and sync run crashes in load_table() before resolving credentials, and the later stop step fails the action regardless of the selected llm; the new unit suite likewise fails at import or preflight time. Include the provider table in this commit.
AGENTS.md reference: AGENTS.md:L20-L23
Useful? React with 👍 / 👎.
Comment on lines
+79
to
+84
| 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. |
There was a problem hiding this comment.
If this commit is merged as TEMP: verify at 4a5209b (v2/crashed-check), release-please skips it as unparseable, so this adopter-facing authentication change will not produce a release or reach consumers of the moving v1 tag. Rename the commit and PR title to the intended feat: form described here.
AGENTS.md reference: AGENTS.md:L90-L93
Useful? React with 👍 / 👎.
💡 Codex Review Add the provider table consumed by the resolver
The new resolver always loads AGENTS.md reference: AGENTS.md:L20-L23 Lines 79 to 84 in 7460bec Use a releasing Conventional Commit prefix
If this commit is merged as AGENTS.md reference: AGENTS.md:L90-L93 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Throwaway.