TEMPORARY: check remedy rendering (close me) - #106
Closed
Svilen-Stefanov wants to merge 10 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>
Contributor
CodeBoarding review - not configured
1. Add a repository secret named 2. In with:
llm: anthropic
anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }}No analysis ran, and no CodeBoarding hosted usage was consumed. run 33101401710 - attempt 1 |
![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: 5f983af14e
ℹ️ 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".
| # 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: anthropic |
There was a problem hiding this comment.
In the inspected codeboarding.yml review job, selecting anthropic without passing anthropic_api_key makes the new credential preflight return missing_provider_key, after which the “Stop on LLM configuration failure” step fails the run. This affects every same-repository PR and authorized /codeboarding invocation, so restore llm: license with CODEBOARDING_LICENSE as used by the sync workflow, or wire an Anthropic secret.
AGENTS.md reference: AGENTS.md:L51-L56
Useful? React with 👍 / 👎.
Comment on lines
+12
to
+14
| 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 |
There was a problem hiding this comment.
The reviewed commit is titled TEMPORARY: check remedy rendering (close me), but it changes the public action authentication contract and inputs. Release-please will not treat that prefix as feat: or fix:, so merging this exact commit will not propose the release needed to move the consumer-facing v1 tag; use the repository's intended feat: title for this change.
AGENTS.md reference: AGENTS.md:L88-L93
Useful? React with 👍 / 👎.
| for _ in range(2): | ||
| value = re.sub(r'^"(.*)"$', r"\1", value) | ||
| value = re.sub(r"^'(.*)'$", r"\1", value) | ||
| value = re.sub(r"^[A-Z0-9_]+=", "", value) |
There was a problem hiding this comment.
When an actual provider key consists of uppercase letters/digits followed by =, this generic prefix removal treats the key itself as a pasted environment assignment: for example, _clean_key("ABC123=") returns an empty string and the action incorrectly reports missing_provider_key, while ABC123== is silently changed to =. API keys are opaque values, so only the exact environment-variable prefix associated with the selected input should be stripped.
Useful? React with 👍 / 👎.
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. Verifying the failure comment renders its link and YAML snippet.