feat(action): named models per comment trigger and provider credential docs (plan 125) - #37
Conversation
…l docs (plan 125) Adds a `models` Action input: a flat `alias: provider/model[:reasoning]` list. `model` stays the default (an alias or a full spec) for automatic reviews and a bare trigger; `codegenie review <alias>` runs one review with a listed model. The alias is a closed-set lookup resolved only after the live permission check; an unlisted name gets a fixed reply and no review. Without `models`, behavior is unchanged. `llm-api-key`, when set, is now the only model key: it clears competing credential env vars for its provider and overrides native vars, all configured models must share one provider, and the Action refuses to run when a stored login on a self-hosted runner would override it. Model resolution failures now say unknown model, deprecated model, or missing credentials (naming the env var) instead of one generic message; the Action shows these in the failure comment, which is now sanitized. Syncs the provider env-var table with pi-ai 0.87.1 (qwen-token-plan*, baseten, meta, radius), adds a registry coverage test, generates a Credentials table in models.md (also catching up the stale registry listing), merges the example workflows into one, and updates the README. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxBabX2aBd6eRLJryVci78
🧞 Codegenie ReviewWarning Review completed with unresolved questions. 9 question(s) remain unresolved; absence of a confirmed finding does not establish safety. 0 confirmed findings retained from completed work. Unresolved questions require attention. CoverageReviewed 55/99 hunks.
🙋 Needs Human Attention
Additional unresolved notes suppressed: 4. Stats
No confirmed findingsNo confirmed findings were retained. The limitations above prevent a clean conclusion. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧞 Codegenie Review
Warning
Review completed with unresolved questions. 10 question(s) remain unresolved; absence of a confirmed finding does not establish safety.
1 confirmed finding retained from completed work. Unresolved questions require attention.
Reviewed 55/96 hunks.
Excluded by configuration/planning: 41 hunks.
Coverage disclosure:
- models.md: generated file
- scripts/write-models-md.mjs: generated file
- specs/plans/125-issue-125-github-model-aliases-and-provider-credentials.md: configured skip rule
- specs/plans/README.md: configured skip rule
🙋 Needs human attention:
- Does the README 'Grammar' bullet (first word after trigger phrase, case-insensitive lookup, no model spec accepted from comment text) exactly match the comment parsing implementation in src/github-action/models.ts?; With
modelsconfigured, any trailing word after the trigger phrase (e.g. "codegenie review please look at auth") now resolves to unknown_alias, posting a reply and skipping the review instead of running the default model — confirm this rejection-over-default behavior is the intended plan-125 contract.; Withmodelsconfigured, any authorized comment of the form "codegenie review " (e.g. "codegenie review this PR") now resolves the first token as an alias and skips the run with an unknown-alias reply instead of reviewing. Is this rejection-over-default behavior documented for users upgrading from the pre-plan-125 "trailing text is ignored" contract?; Does tests/github-action.test.ts still cover the credential wiring boundary previously exercised via applyGenericApiKey (env var precedence, provider-specific key selection) now that the import is renamed to applyLlmApiKey?; Are there added tests asserting requestedAliasFromComment behavior (first word on trigger line, unknown alias handling) in this PR?; Is the positive end-to-end path (llm-api-key actually written into the spawned review's env for a single-provider alias set) asserted anywhere, or only the two failure paths at tests/github-action.test.ts:1389 and :1404?; Does any other suite (e.g. a models.ts-level test) assert that the llm-api-key stored-login guard only triggers for the routed provider? - Should action.yml's
modelsdescription state that settingmodelsmakesmodelrequired, givenmodel's input default is ""?; Does parseModelAliases("") handle the always-present empty --models value (unset action input) without error, returning an empty alias map?; Does parsingmodelsat argv-parse time (before decideTrigger) make every event on a workflow with a malformedmodelsblock fail the job, where previously only--modelspecs were validated there? Confirm this hard-fail-on-all-events posture is intended.; Does yaml's parseDocument silently drop content after a---document separator in themodelsinput, so aliases listed after such a line are ignored without an error instead of being rejected by parseModelAliases? - Does createFileAuthStorage(getCodegeniePaths(undefined, env)).get(provider) tolerate a missing/unreadable auth file on hosted runners without throwing, given the new unconditional call whenever LLM_API_KEY routes to a provider?; Is the stored-login / applyLlmApiKey credential validation intentionally skipped in preflight-only mode (it sits after the inputs.preflightOnly early return at entrypoint.ts:170-186), so misconfigured runners only fail in the main review job?; Do the newly added API_KEY_ENV_VARS entries (baseten→BASETEN_API_KEY, meta→META_API_KEY, radius→RADIUS_API_KEY, qwen-token-plan* →QWEN_TOKEN_PLAN[_CN]_API_KEY) exactly match pi-ai's internal env-api-keys table?
- Is ModelResolutionDetail.kind written verbatim into the world-readable failure JSON without scrubGitHubSecrets/boundedPublishedText, and should the guard narrow to the known kinds to keep future producers from publishing unbounded/unscrubbed text?; Does any caller or test assert on the previous literal message "no usable LLM model could be resolved..." for cases that now return a specific message (unknown_model/deprecated_model/missing_credentials)?; Is labeling an unknown provider plus model as "unknown model /" (rather than an unknown-provider message) acceptable product wording, given tests/model-resolution.test.ts asserts it explicitly?
- Does any non-test PiAiAdapter implementation other than createRealPiAiAdapter need explainUnresolvedModel to avoid always reporting the generic unresolved message?; Does PiAiAdapter declare explainUnresolvedModel as optional with return type ModelResolutionFailure, so resolution.failure also type-checks and adapter stubs in tests remain compatible?
- Additional unresolved notes suppressed: 5
— codegenie v0.6.3 (8cbe1ce9ff) · View Workflow Job
| // A stored login for another provider, or no llm-api-key, is unaffected. | ||
| const other = await runWithModels(pullRequestPayload(), "pull_request", ["--model", "anthropic/claude-opus-5"], { |
There was a problem hiding this comment.
Impact: The authStorage guard in src/github-action/entrypoint.ts is deliberately provider-scoped: after applyLlmApiKey returns keyProvider, the run is refused only when storage.get(keyProvider) finds a stored codegenie login for that same provider. The second half of the test claims to prove that a stored login for a different provider does not block llm-api-key, but it executes through runWithModels, which injects authStorage: { get: () => undefined }. No stored login exists for any provider in that run, so the provider comparison is never discriminated. If the guard regressed to be provider-agnostic — checking a fixed provider such as storage.get("openrouter"), or any stored credential at all — this case would still pass, and the only other authStorage test (lines 1404-1418) asserts the throwing case, which such a regression also satisfies.
With that regression unguarded, the action would hard-fail with invalid_args before the status comment is claimed on any self-hosted runner holding an unrelated stored codegenie login, blocking all reviews for a valid llm-api-key configuration with no test signalling the break.
Verification and uncertainty: Mutating the guard to storage.get("openrouter") or to a [...].some(...) check over all providers leaves tests at lines 1404-1425 green, because the negative case's stub returns undefined for every provider. The provider-scoped predicate itself is confirmed in src/github-action/entrypoint.ts:178-185; a path-scoped search of the suite found only two authStorage injections (tests/github-action.test.ts:1308 in runWithModels and :1411 with provider === "openrouter" ? stored : undefined), and the guard message appears only at entrypoint.ts:183 and test line 1417, so no other test file covers this branch.
The coverage conclusion is bounded to the inspected revision and to tests/**: it remains unverified whether a non-test consumer, such as an e2e or workflow check outside that path, exercises the provider-scoped branch. That assumption is non-essential — it would add coverage elsewhere without changing the fact that this assertion does not discriminate the guard's scoping.
Suggested test: Add a case that makes the provider comparison live, driven through executeGitHubActionCommand directly rather than runWithModels (whose extra parameter carries only env/comments and offers no authStorage override):
it("allows llm-api-key when the stored login belongs to another provider", async () => {
// authStorage: { get: (provider) => (provider === "openrouter" ? stored : undefined) }
// run with ["--model", "anthropic/claude-opus-5"] and LLM_API_KEY="sk-ant-explicit"
// expect: no throw, env.ANTHROPIC_API_KEY === "sk-ant-explicit", runReview called
});The injected foreign-provider entry means the case cannot pass vacuously, and a provider-agnostic guard throws invalid_args, failing all three expectations. Keep the existing throwing assertion at lines 1404-1418 unchanged.
Evidence:
tests/github-action.test.ts (source)
// A stored login for another provider, or no llm-api-key, is unaffected.
const other = await runWithModels(pullRequestPayload(), "pull_request", ["--model", "anthropic/claude-opus-5"], {
env: { LLM_API_KEY: "sk-ant-explicit" }
});
expect(other.env.ANTHROPIC_API_KEY).toBe("sk-ant-explicit");src/github-action/entrypoint.ts (source)
const keyProvider = applyLlmApiKey(env, inputs.models);
if (keyProvider !== undefined) {
const storage = opts.authStorage ?? createFileAuthStorage(getCodegeniePaths(undefined, env));
if (storage.get(keyProvider) !== undefined) {
throw new CodegenieError(
"invalid_args",
`a stored codegenie login for ${keyProvider} on this runner would override llm-api-key; ...`
);
}
}The guard is deliberately scoped to the single provider llm-api-key was routed to; only that scoping decides whether an unrelated stored login blocks a valid run.
tests/github-action.test.ts (source)
authStorage: { get: () => undefined }, // runWithModels, line 1308
...
authStorage: { get: (provider) => (provider === "openrouter" ? stored : undefined) }, // line 1411The only two authStorage injections in this suite: the failing case injects an openrouter login, while the "unaffected" case goes through runWithModels, whose stub reports no stored login for any provider.
Original assessments and supporting evidence (may overlap or disagree)
Original source material is retained for audit. The current conclusion is above; superseded assessments are labeled where supplied. Attribution does not prove semantic equivalence.
Original candidate metadata:
f1792113-f1: severity low, confidence medium
Original impact: f1792113-f1/failureMode
The second half of the test claims to prove that a stored login for a *different* provider does not block llm-api-key, but it runs through runWithModels, which injects `authStorage: { get: () => undefined }`. No stored login exists in that run, so the provider comparison in `storage.get(keyProvider)` is never discriminated. If the guard regressed to be provider-agnostic (e.g. checking a fixed provider or any stored credential), this test would still pass, and the only other authStorage test (line 1404-1418) asserts the throwing case, which such a regression also satisfies.
Original impact: f1792113-f1/whyThisMatters
A provider-agnostic regression would make the action hard-fail (throw invalid_args before the status comment is claimed) on self-hosted runners that hold any unrelated stored codegenie login, blocking all reviews for a valid llm-api-key configuration, with no test signalling the break.
Original verification: f1792113-f1/verification
Re-run the suite with the guard mutated to `storage.get("openrouter")` or to `[...].some(...)` over all providers: tests at lines 1404-1425 still pass because the negative case's stub returns undefined for every provider.
Original fix: f1792113-f1/suggestedFix
Drive the "unaffected" case through executeGitHubActionCommand with the same provider-scoped stub, e.g. pass `authStorage: { get: (provider) => (provider === "openrouter" ? stored : undefined) }` while selecting `--model anthropic/claude-opus-5` and LLM_API_KEY set, and assert it resolves (ANTHROPIC_API_KEY applied, review argv produced).
Suggestion assessment: supported
The fix keeps the original requirement intact rather than weakening it: it injects a stored openrouter login while routing llm-api-key to anthropic, so storage.get(keyProvider) must return undefined and the run must proceed. It does not relax the throwing case at 1404-1418 (that assertion stays), and it cannot be satisfied by a provider-agnostic guard. runWithModels does not accept an authStorage override (its extra parameter only carries env/comments), so routing the case through executeGitHubActionCommand directly, as proposed, is the only way to supply the provider-scoped stub.
Behavioral requirement (established): llm-api-key must be refused only when a stored codegenie login exists for the provider llm-api-key was routed to; a stored login for any other provider must leave the run working (env key applied, review argv produced).
src/github-action/entrypoint.ts
const keyProvider = applyLlmApiKey(env, inputs.models);
if (keyProvider !== undefined) {
const storage = opts.authStorage ?? createFileAuthStorage(getCodegeniePaths(undefined, env));
if (storage.get(keyProvider) !== undefined) { throw new CodegenieError("invalid_args", ...) }
Establishes the provider-scoped requirement the fix must exercise.
tests/github-action.test.ts
1295-1319 runWithModels(..., extra: { env?; comments? }) { ... authStorage: { get: () => undefined }, ... }
Shows the helper hard-codes an empty storage and offers no override, so the fix must call executeGitHubActionCommand directly.
Original test: f1792113-f1/suggestedTest
it("allows llm-api-key when the stored login belongs to another provider") — inject the openrouter `stored` entry, run with `--model anthropic/claude-opus-5` and LLM_API_KEY=sk-ant-explicit, expect no throw, expect env.ANTHROPIC_API_KEY === "sk-ant-explicit", and expect runReview to have been called.
Suggestion assessment: supported
The proposed assertions discriminate the three cases correctly. Against the observed gap: with the openrouter stored entry present and --model anthropic/claude-opus-5 + LLM_API_KEY, the provider comparison is finally live. Against a symptom-hiding remedy (deleting or softening the misleading comment, or widening the stub to return undefined again), the test still requires a real stored entry for a foreign provider, so it would not pass vacuously. Against a provider-agnostic regression (storage.get("openrouter") or any-credential check) the call throws invalid_args and the expectations on no-throw, ANTHROPIC_API_KEY and runReview having been called all fail. The correct provider-scoped implementation passes: keyProvider is anthropic, the stub returns undefined for anthropic, and the existing line 1424 assertion already demonstrates ANTHROPIC_API_KEY is set to the llm-api-key value on this path. It does not over-constrain (no exact argv/ordering demands beyond runReview being invoked).
... (truncated)
- llm-api-key's single-key rule now requires one API-key env var rather than one provider id, so provider pairs that share a var (moonshotai / moonshotai-cn, opencode / opencode-go, cloudflare-*) work with one key; competing vars are cleared and the stored-login guard runs per provider. - Validate model/models after the trigger gate, so a broken block fails real triggers but unrelated comments skip instead of going red. - Restore discriminated decision-record variants. - Reuse providerKnown for alias provider validation. - Normalize the requested alias only in the event gate, stripping surrounding quotes/backticks/brackets and trailing punctuation; alias names must start and end with a letter or digit so all stay reachable. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxBabX2aBd6eRLJryVci78
Bump the package to 0.7.0 and pin the documented action references to v0.7.0, the first release with the `models` input. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NxBabX2aBd6eRLJryVci78
1 participant
Implements plan 125.
What changes
Named models, picked per comment
codegenie review opusruns one review with theopusentry. The first word on the trigger phrase's line is looked up in the workflow's list, and nothing else in the comment is read: no model specs, reasoning levels or flags from comment text.models, behavior is unchanged, including trailing comment text being ignored. A regression test pins the simplemodel+llm-api-keyform.llm-api-keyis the only key when setANTHROPIC_AUTH_TOKEN) and overrides native env vars. This reverses the previous precedence: a workflow that sets both, with different values, now usesllm-api-key.codegenie provider loginfor that provider, the Action refuses to run instead of silently ignoringllm-api-key. The CLI is unchanged.Clearer model errors
config_error.Credentials docs
qwen-token-plan*,baseten,meta,radiuswere missing), with a registry coverage test that fails when a provider is added upstream without a mapping.models.mdgains a generated Credentials table. Most of its diff is regeneration catching up with the installed registry (1103 → 1490 models); only the Credentials section is new logic.examples/workflows/codegenie-review.yml, which serves both triggers with the same trust model as the dogfood workflow.Review
An independent adversarial review found three issues: the stored-login bypass (fixed by the owner-chosen guard), a nonexistent provider labeled "missing credentials", and numeric alias names being rejected. All three are fixed with tests. It found the trust boundary, secret handling on every published surface, backward compatibility and error labeling sound. Mutation checks confirm the sanitizer and credential-clearing tests fail when those fixes are removed.
Validation
pnpm test: 1,583 tests across 69 files, including actionlint on the merged example.make evals: 39 passed.git diff --checkpass;models.mdregenerates identically.Not done yet
@v0.7.0(the first release with themodelsinput). Publish npm before pushing thev0.7.0tag.🤖 Generated with Claude Code
https://claude.ai/code/session_01NxBabX2aBd6eRLJryVci78