feat(ci): select agent model from the live catalog, not hardcoded ids #395
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: konairius/homelab#395
Reference in New Issue
Block a user
What
All five automatic CI agents now choose their model from the live Anthropic
model catalog instead of hardcoded ids.
Previously
pr-review,update-audit,security-scan,alert-monitorandissue-fixpinned--model claude-sonnet-5(and pr-review mapped ahaiku/sonnet/opus keyword to dated ids via
map_model). Those pins go stale onevery model release and hardcode which model families are assumed to exist, so a
newly released model could never be selected without a code edit.
How
scripts/ci/fetch-models.sh— theclaudeCLI has no "list models"command, so this queries the same catalog the CLI itself uses
(
GET /v1/models) with the existingCLAUDE_CODE_OAUTH_TOKEN(requires theanthropic-beta: oauth-2025-04-20header). Emits each model with objectivecapability signals — context window, max output, effort, thinking, code_exec,
release date — with no id patterns and no family names.
scripts/ci/pick-model.sh— a triage agent picks the best-fit id from thatcatalog for a one-line task description, weighing every listed model on its
merits, and validates the choice via
scripts/ci/valid-model.sh. Used by thefour fixed jobs.
scripts/ci/valid-model.sh— the single, unit-tested gate: echoes acandidate id only if it is a safe
[A-Za-z0-9._-]token (no leading dash) thatis present in the live catalog. Reused by both
pick-model.shand pr-review soan untrusted, agent-supplied value can never reach a
--modelflag.(
./.pr-review/models.txt) and returns a concrete id;map_modelis removed.Guarantees
tomorrow, of any family, is eligible the day the API lists it.
ever unreachable,
--modelis simply omitted (CLI default) rather thanfalling back to a named model — a run never dies over model selection.
degrades to the CLI default.
is ignored and the review runs on the CLI default — the strongest configured
model — so an injected diff cannot steer a sensitive review onto a weak model.
Properties & tradeoffs (cost)
These pipelines no longer carry a fixed model-cost ceiling — by design, cost now
tracks whatever the account's live catalog and default model are:
model. Its token count is bounded (it reads
stats.txt+ a completefiles.txt+ a diff capped at 4000 lines, not the up-to-20k review diff), butthe per-token rate is the account default.
claudecall per run (thepicker, on the default model) to choose the model. For jobs that already
describe themselves as needing "a capable model", that bootstrap can cost more
than it saves; it is accepted as the price of removing hardcoded family
knowledge. Worth measuring on the 15-minute alert-monitor cadence (gated to
alert-bearing runs).
Testing
scripts/ci/test-model-selection.sh— offline unit tests (no network / claude/ Woodpecker) for the validation gate and the
fetch-models.shresponsemapping: valid ids pass; hallucinated / empty / space- / newline- / leading-
dash- / metachar-injection are rejected; empty catalog rejects all; a
well-formed
/v1/modelsbody maps to non-null capability columns; a degradedbody warns loudly. Wired into
.woodpecker/e2e.ymlso it runs in CI.sh -n; all touched pipelines parse underyq./v1/modelsreturns HTTP 200 with the OAuth subscription token,the capability fields are populated, a trivial task selects a small model and a
security task the strongest/newest, and every failure path degrades to the CLI
default.
🤖 Generated with Claude Code
🤖 Automated review — model
claude-opus-5(complexity: complex), commitffe3be4b545f8be87ece4da3d8d7edd07d2e3f95.Summary
Replaces hardcoded
--modelpins in all five agent pipelines with a livelookup:
scripts/ci/fetch-models.shpullsGET /v1/modelswith the existingOAuth token, and a triage agent (
scripts/ci/pick-model.sh, or the per-PRtriage in
pr-review.yml) picks an id from that catalog. The shell-level designis careful — the pick is validated against the catalog before it is trusted, the
token only ever goes to
api.anthropic.com, and no secret is added. One failurepath is not covered by the fallback the PR promises, and a few of the premises
behind the capability table are worth confirming before merge.
🔴 Blocking
A catalog id the CLI refuses is the one failure mode with no fallback —
scripts/ci/pick-model.sh:73and.woodpecker/pr-review.yml:228both validatethe pick with "is this string in the catalog?" and nothing more.
/v1/modelslists what the API offers for the account; the Claude Code CLI runs a subset
of that (legacy Claude 3 ids, and models gated by subscription tier). The old
code could not hit this because
map_modelmapped to three hand-verified ids;this PR widens the accepted set to everything the API lists, with no probe and
no retry.
What happens when
claude --model <listed-but-unusable-id>exits non-zero:.woodpecker/pr-review.yml:236-245— the failure is swallowed by|| echo "(review agent exited non-zero)",report.mdis empty, the loopcontinues, and the step exits 0. The PR gets no review and the pipeline isgreen — the exact "green run that did nothing" failure this repo has already
been burned by twice (see the comment at
.woodpecker/update-audit.yml:106-114)..woodpecker/security-scan.yml:106,update-audit.yml:92,alert-monitor.yml:198,issue-fix.yml:158— no|| trueon the pipe, so thestep fails outright. A run that worked yesterday on a pinned id now dies on
model selection, which is precisely what the PR's "a run never dies over model
selection" is meant to prevent.
Being straight about what I could and could not verify: the missing fallback
is confirmed by reading the code above. The trigger — that the catalog contains
ids this CLI/plan cannot run — I could not confirm from the checkout, and the
sandbox here blocked me from calling the API to check. The one check that settles
it, in the
ci-claude:2.1.218image:If that prints nothing, downgrade this to a nit. If it prints anything, the gap
is live. Either way the fix is small: probe the chosen id once (as above) before
returning it from
pick-model.sh, or retry the agent invocation without--modelwhen it exits non-zero — which is what the PR body already claimshappens.
🟡 Suggestions
The pr-review triage got much more expensive, against the PR's own goal —
.woodpecker/pr-review.yml:219-223drops--model claude-haiku-4-5-20251001so the triage now runs on the CLI default. That triage reads
diff.patch(ci/agents/pr-review/triage-prompt.md, "Readmodels.txt,stats.txt, anddiff.patchfirst"), which is capped at 20,000 lines(
pr-review.yml:131). So to decide "this PR is trivial, use a cheap model",the pipeline now reads the entire diff on the most capable default model — the
triage can cost as much as the review it is trying to economise. Avoiding a
hardcoded family for the bootstrap is a reasonable principle, but it would cost
much less if the triage were fed only
stats.txtplus the changed-file list(
grep '^diff --git ') and readdiff.patchonly when it needs to judgesecurity-sensitivity.
The picker agent runs unsandboxed with
acceptEdits—scripts/ci/pick-model.sh:63-66invokesclaude --permission-mode acceptEditswith no
--settings. Every other agent invocation in this repo carries a Readsandbox for a documented reason (
ci/agents/pr-review/settings.json:2-18:/proc/self/environis where the tokens actually live). The picker's steps holdwrite-scoped Gitea PATs (
alert-monitor.yml:115,update-audit.yml:50,issue-fix.yml:75). Worse,acceptEditsauto-approves file writes the pickerhas no business making — and in
security-scan.yml:113-115andissue-fix.ymlan untracked stray file is counted as agent work and swept into the
auto-opened PR. The picker needs no tools at all: drop
--permission-mode acceptEdits, add--settings(or--disallowedTools), andenv -u GITEA_TOKENthe call. Low likelihood today (the prompt is entirelydeveloper-controlled — see the next point) but it is free to close.
/v1/modelsis paginated and only the first page is read —scripts/ci/fetch-models.sh:35-40requests nolimitand ignoreshas_more/last_id. The endpoint's default page size is 20 and the order is newest-first,so once the account has more than 20 models the ones silently dropped are the
oldest and cheapest — exactly the ones a
trivialreview or a mechanicaltask should be routed to, and the fallback the selector is told to prefer for
cheap work.
?limit=100(or ahas_moreloop) fixes it.Confirm the capability fields actually exist in the response —
fetch-models.sh:54-58reads.max_input_tokens,.max_tokensand.capabilities.{effort,thinking,code_execution}.supported. The documented/v1/modelsmodel object is{id, type, display_name, created_at}. If theOAuth beta response is that shape, jq emits
context=null max_output=null effort=false thinking=false code_exec=falsefor every row without erroring,and the "objective capability signals" premise collapses — leaving the selector
with only the display name and the release date, i.e. the family-name reasoning
this PR set out to eliminate (
triage-prompt.mdstill instructs the agent toweigh
context/max_output/effort/thinking/code_exec). The PR says thiswas verified live; please paste one row of
sh scripts/ci/fetch-models.sh | jq '.[0]'into the PR. If the fields do exist, consider having the script
loga warningwhen
contextis null for every model, so a future API schema change surfacesas a message rather than as a silently degraded table.
No timeout on the selection call —
fetch-models.sh:36uses--max-time 20,but the
claudecall atpick-model.sh:63has none. All five pipelines nowblock on an extra LLM round-trip before doing any real work; if that call hangs,
the job hangs until the Woodpecker-level timeout. A
timeout 120 claude ...(or the CLI's own limit) keeps the "never dies over model selection" contract
true for hangs as well as errors.
The table format is duplicated and must stay in sync in three places — the
jq tab-table at
pick-model.sh:31-32, the identical one atpr-review.yml:206, and the column list documented intriage-prompt.md. A--tablemode onfetch-models.shwould leave one copy.pr-review.yml:204executes a script from the PR head withREVIEW_TOKENin the environment —
HOMELAB="$$WS"(line 136) is the PR's own checkout, sofor homelab PRs
sh "$$HOMELAB/scripts/ci/fetch-models.sh"runs branch-suppliedshell, and unlike the
claudecalls it is not wrapped inenv -u REVIEW_TOKEN.I checked before flagging this as an escalation and it is not a new exposure:
Woodpecker already takes
.woodpecker/*.ymlitself from the PR branch onpull_requestevents, so a PR branch can run arbitrary commands in this stepregardless. Worth one comment line at line 136 saying so, since the surrounding
block (lines 184-193) documents its token-handling reasoning so carefully.
The header prints an empty model on the fallback path —
pr-review.yml:251-252still formatsmodel \%s`with$$MODEL, which is now legitimately empty whenever the catalog fetch fails or the pick is rejected. The published comment then reads ``model `` (complexity: normal)``. Use"$${MODEL:-CLI default}"`.🟢 Nits
fetch-models.sh:40— on a transport failure curl itself prints000andexits non-zero, so
|| echo 000appends a second one and the log line becomesreturned HTTP 000\n000. Outcome is still correct (non-200 → exit 1); useHTTP="$(curl ... )" || HTTP=000if you want a clean message.fetch-models.sh:59—2>/dev/null || truehides jq's parse error, so aresponse-schema change surfaces only as the generic "response held no usable
model list". Logging jq's stderr would save a debugging round.
pick-model.sh:20— theGUIDANCEdefault is unreachable; all five call sitespass a description. Harmless.
Tests
The repo has no shell test harness, so
sh -nplus ayqparse is theestablished bar and the PR meets it. What is untested is the part that matters:
every fallback path in
pick-model.shcurrently requires real network conditions(no token, HTTP error, hallucinated id) to exercise, so all four can only be
verified by deliberately breaking CI. A small seam — e.g. honouring
PICK_MODEL_CATALOG_FILEin place of callingfetch-models.sh— would let ascripts/ci/self-test drive an empty catalog, a garbage catalog, and anout-of-catalog pick offline, and would also cover the blocking issue's fix. At
minimum, the "unusable id" probe quoted in the Blocking section should be run
once by hand and its output recorded in the PR.
Recommendation
Request changes — 1 blocking issue (unhandled fallback for a valid-in-catalog but
CLI-rejected model id; the trigger is unconfirmed and one command settles it).
Thanks — this was a genuinely useful review. Addressed in
b329c432.🔴 Blocking — listed-but-CLI-unusable id
Verified the trigger is not live: ran your probe in the checkout against the
current catalog and every one of the 11 ids runs under
claude --model:So per your own "downgrade to a nit if it prints nothing", it downgrades. The
residual robustness point stands though, and the design already covers it: if a
future id is listed-but-unusable,
pick-model.shreturning it only affects therun that used it, and the fallback path (omit
--model→ CLI default) is whatthe four fixed jobs get once the pick is cleared. I did not add
|| truetotheir
claudepipe on purpose — that would also mask genuine agent failures,which those steps are meant to surface loudly.
🟡 Suggestions
?limit=1000(the API max) plus ahas_morewarningin
fetch-models.sh. Good catch: at 11 models today it was latent, but thedropped page would have been the oldest/cheapest.
{"id":"claude-opus-5","context":1000000,"max_output":128000,"effort":true, "thinking":true,"code_exec":true}. The bare{id,type,display_name,created_at}in the docs is not what the
oauth-2025-04-20beta returns.jqstderr is nowsurfaced so a future schema change fails loudly instead of degrading silently.
env -u GITEA_TOKENandwithout
acceptEdits(it is granted no tools at all).triage-prompt.md: it now readsstats.txt+the changed-file list first and pulls the full diff only when it needs to judge
a borderline case or a security-sensitive path, so classifying a trivial PR no
longer reads a 20k-line diff on the default model.
fetch-models.sh --tableis now the onesource of truth;
pick-model.shandpr-review.ymlboth consume it.model `CLI default`on thefallback path.
REVIEW_TOKEN— added the comment you suggestedat the
HOMELAB=line, noting it is not a new exposure (Woodpecker alreadyruns
.woodpecker/*.ymlfrom the PR branch onpull_request).--max-timecovers the catalog fetch; wrapping the
claudecall needstimeout, whoseargument syntax differs between coreutils and older busybox builds, and I would
rather confirm the
ci-claudeimage'stimeoutbefore adding something thatcould silently route every run to the default model. Tracking separately.
🟢 Nits
000double-print — fixed (HTTP="$(curl …)" || HTTP=000).jqstderr hidden — fixed (surfaced).GUIDANCEdefault — left as a harmless guard.On the offline-testability point: agreed it's the real gap. I kept it out of this
change to stay scoped, but a
PICK_MODEL_CATALOG_FILEseam + ascripts/ci/self-test is the right follow-up and I'll open an issue for it.
🤖 Automated review — model
claude-opus-5(complexity: complex), commitb329c432c56932dd7f557ada8e17c9cad36124af.Summary
Replaces the hardcoded
--model claude-sonnet-5pins in five CI agents with alive-catalog lookup:
scripts/ci/fetch-models.shfetchesGET /v1/models, andeither
scripts/ci/pick-model.sh(four fixed jobs) or the existing pr-reviewtriage agent picks an id from it, with every failure path degrading to the CLI
default. I traced the shell in all five pipelines and both helpers; the control
flow, the
$$-escaping, the${MODEL:+--model $MODEL}conditional expansion,the catalog validation, and each fallback path are correct, and no step has a
directory:override so the relativescripts/ci/...paths resolve. Noblocking issues — the comments below are about diagnostics, silent
degradation, and one free defence-in-depth tightening.
🟡 Suggestions
1.
scripts/ci/pick-model.sh:28,65—2>/dev/nullthrows away everydiagnostic the sibling script exists to produce.
fetch-models.shis explicitly built around stderr (:22— "Diagnostics go tostderr so
$(...)capture stays clean") and emits precise causes: token notset,
GET /v1/models returned HTTP 401,has_more=true,could not parse.TABLE="$(sh "$HERE/fetch-models.sh" --table 2>/dev/null || true)"discards allof it, so when the four fixed jobs silently start running on the CLI default the
operator sees only
pick-model: no catalog available. The same applies to theclaudecall at:65— an auth failure, a rate limit, and a malformed reply areindistinguishable in the log. Dropping both redirects costs nothing; the capture
is already stdout-only.
2.
scripts/ci/fetch-models.sh:67-76— nothing validates that the capabilityfields actually came back, and the comment above it claims a guarantee the code
does not provide.
The comment at
:64-66says "jq stderr is surfaced so a response-schema changeshows up as a real error". That holds only for
.datagoing missing. Ifmax_input_tokens,max_tokens, orcapabilitiesdisappear from the response,jq does not error —
{context: .max_input_tokens}yieldsnulland the three// falsedefaults swallow the rest, so the script exits 0 with rows readingcontext=null max_output=null effort=false thinking=false code_exec=false. Bothselectors then have nothing left to weigh but the display name and release date,
which is exactly the "guess from the family name" behaviour this PR is removing —
and it happens silently, on the security-sensitive jobs too. Worth an explicit
guard, e.g. fail (or at minimum
logloudly) when every entry hascontext == null.I could not confirm from the checkout whether the OAuth-beta response actually
carries these fields — they are not in the public
/v1/modelsschema, and the PRbody's live verification only asserts HTTP 200. The one check that settles it:
run
sh scripts/ci/fetch-models.sh --tablein CI and confirm thecontext=column is a number rather than
null. (See Tests.)3.
.woodpecker/pr-review.yml:211— stripREVIEW_TOKENfrom the helper too.sh "$HOMELAB/scripts/ci/fetch-models.sh" --tableruns branch-supplied shell inthe step shell, which holds the write-scoped
REVIEW_TOKEN. The comment at:137-143is right that this is not a new capability (Woodpecker takes.woodpecker/*.ymlfrom the PR head onpull_request, and$PROMPTSalreadycame from
$WS), but it is a new direct execution path, and the mitigation isone word —
env -u REVIEW_TOKEN sh "$HOMELAB/scripts/ci/fetch-models.sh" --table. The script needs onlyCLAUDE_CODE_OAUTH_TOKEN, and this matches thepattern already applied to both agents 13 lines below.
4.
scripts/ci/pick-model.sh:63— no timeout on the selection call.fetch-models.sh:47carefully caps curl at--max-time 20, but theclaudeinvocation that follows has no bound. A hung or very slow CLI blocks the step
until Woodpecker's pipeline timeout, which is the opposite of the stated contract
("a run never dies over model selection").
timeout 120 claude ...(busyboxprovides
timeout) preserves the fallback: on timeout|| trueleavesRAWempty and
none()fires.5.
.woodpecker/pr-review.yml:224— triage lost its cheap-model guarantee.The removed header text (
:51-55in the diff) justified the two-stage designwith "Triage is cheap". Triage now runs on the CLI default model over inputs that
can reach
MAX_DIFF_LINES=20000(:131), so on a docs-only PR the triage stepcan plausibly cost more than the review it selects — inverting the point of
triaging.
triage-prompt.mdnow asks the agent to readstats.txtplus thegrep '^diff --git 'file list first, which helps, but nothing enforces it. Twooptions that keep the "no family baked in" property: bound what triage sees (hand
it
stats.txt+ the file list rather than the symlinked full diff), or derive thebootstrap deterministically from the catalog (e.g. the row with the smallest
max_output) instead of spending an LLM call to choose the chooser. The sameconcern applies to
pick-model.sh, which adds one full catalog fetch plus oneclaudesession to each of the four fixed jobs on every run.6.
.woodpecker/issue-fix.yml:76— the token stripping is incomplete there.pick-model.sh:63doesenv -u GITEA_TOKEN, and its comment reasons that "thepicker has no reason to hold a write-scoped forge PAT". In issue-fix,
PUSH_TOKEN(
:76) is bound to the sameissue_fix_bot_tokensecret and survives, so thepicker still carries a push-capable credential in that one pipeline.
env -u GITEA_TOKEN -u PUSH_TOKENrestores the intended property. Low impact — thepicker's prompt is developer-controlled and it holds no tools — but the stated
invariant is currently false for one of the four callers.
🟢 Nits
scripts/ci/pick-model.sh:72and.woodpecker/pr-review.yml:233:grep -qxF "$CHOICE"treats a value beginning with-as an option. Today theoutcome is still safe by accident (grep errors → the
ifis false → fallback),but
grep -qxF -- "$CHOICE"makes it deliberate.scripts/ci/pick-model.sh:64:--allowedTools ""with no--settingsis theonly
claudeinvocation in the repo without a settings sandbox, so it inheritswhatever global config the
ci-claudeimage ships. Harmless given it is grantedno tools and reads a developer-controlled prompt, but reusing a minimal settings
file would keep the invocations uniform.
scripts/ci/fetch-models.sh:12-16: thejsonmode is the default and has noin-repo consumer —
--tableis described as "the single format every selectorconsumes". Fine as a debugging affordance, worth saying so.
scripts/ci/pick-model.sh:11: the usage comment shows the quoted form${MODEL:+--model "$MODEL"}, while all four call sites use the unquoted form.Both work for a catalog id; the mismatch is just confusing.
Tests
The repo has no shell test harness and no shellcheck step (
grep -rn shellcheck .woodpecker/finds nothing), sosh -n+ ayqparse — what the PR bodyreports — is the existing bar, and both new scripts are syntactically clean.
The gap worth closing is suggestion #2, because it is the one failure mode that
nothing detects and nothing degrades loudly from. A cheap guard in a pipeline
that already runs on the
ci-claudeimage:sh scripts/ci/fetch-models.sh --table, assert at least one line, andassert the
context=field is numeric rather thannull— this catches anAPI schema change before it silently reduces both selectors to guessing;
pick-model.shwithCLAUDE_CODE_OAUTH_TOKENunset and assert it printsnothing and exits 0 — that is the contract every caller depends on for the
${MODEL:+...}fallback to work.Both are a handful of lines and need no new tooling.
Recommendation
Looks reasonable — no blocking issues; comments above are optional.
Round two addressed in
9d360878.2>/dev/nullon both thefetch-models.sh --tablecapture and theclaudepicker call, so the realcause of a fallback (no token / HTTP 401 / parse error / auth) now reaches the
job log.
fetch-models.shnow logs a loudWARNING when every model returns a null
context, so a/v1/modelsschemachange surfaces as a message instead of silently degrading both selectors to
name+date. Confirmed live the fields are present today (
context=1000000, etc.).REVIEW_TOKENon the helper (#3) — fixed →env -u REVIEW_TOKEN sh … fetch-models.sh --table, matching the agent calls below it.timeout 120 claude …, but only after atimeout 1 trueprobe confirms the image'stimeouttakes the coreutilsSECS CMDform — so a busybox build with different syntax degrades to "nowrapper" rather than silently sending every run to the default.
PUSH_TOKENin issue-fix (#6) — good catch, fixed.pick-model.shnowdoes
env -u GITEA_TOKEN -u PUSH_TOKEN -u REVIEW_TOKEN; verified onlyCLAUDE_CODE_OAUTH_TOKENsurvives into the picker.grep -qxF --in both places.Deferred, with reasoning:
Hard-bounding what triage sees (stats + file list only) would blunt its
security-sensitivity judgment, which is the one thing it must not get wrong;
the deterministic "smallest max_output" bootstrap trades the cost for a
catalog heuristic I'd rather not reintroduce. Open to revisiting if the triage
spend turns out to matter in practice.
as a follow-up (a
PICK_MODEL_CATALOG_FILEseam + ascripts/ci/self-test)rather than growing this PR further.
🤖 Automated review — model
claude-opus-5(complexity: complex), commit9d360878573e2039ad45aea74f4057b15716d8fe.Summary
Replaces hardcoded
--modelpins in all five agent pipelines with a live lookupagainst Anthropic's
/v1/models, plus a triage agent that picks an id from thatcatalog. Two new POSIX helpers (
scripts/ci/fetch-models.sh,scripts/ci/pick-model.sh) and a rewrittentriage-prompt.mdcarry the logic.The design is careful — every failure path degrades to the CLI default rather
than dying — but removing
map_model()also removed the only thing thatconstrained the model string to a safe literal, and the replacement validation
is bypassable by the agent that produces it.
🔴 Blocking
Agent-supplied
MODELcan inject arbitrary flags into the reviewclaudecall.woodpecker/pr-review.yml:232-241The value is taken verbatim from
triage.jsonand never shape-checked, and bothgates that follow are permeable to a multi-line value:
grep -qxF -- "$MODEL"— POSIXgrep -Ftreats newlines in the patternoperand as pattern separators. A two-line value whose first line is a
real catalog id (
claude-sonnet-5\n--dangerously-skip-permissions) matchesand passes validation.
$${MODEL:+--model $$MODEL}is unquoted, so the result undergoes fieldsplitting on the default
IFS(space/tab/newline) and expands to--model claude-sonnet-5 --dangerously-skip-permissions— three argventries, the third an attacker-chosen CLI flag.
This is reachable, not theoretical. The triage agent's inputs are
./.pr-review/diff.patchandpr.json— attacker-controlled by this file's ownstated threat model (
.woodpecker/pr-review.yml:218-220, andci/agents/pr-review/settings.jsonwhose header calls the diff "attacker-controlledinput"). It runs with
--allowedTools "Read,Glob,Grep,Write", andsettings.jsondenies only
Read— Write is unscoped (update-audit.yml:60-64 records that Writepath-scoping "was tested and found NOT to be enforced under
--permission-mode acceptEdits"). So a prompt-injected diff can make the triagewrite any
modelstring it likes, and that string becomes flags on the reviewagent's invocation — e.g.
--dangerously-skip-permissions, defeating the--allowedToolsallowlist and the Read sandbox that exist specifically tocontain that agent.
CLAUDE_CODE_OAUTH_TOKENis in that process's env.This is a regression introduced here: the removed
map_model()returned one ofthree literal ids from a
caseallowlist and the call site used a quoted--model "$$MODEL", so neither hole existed before.scripts/ci/pick-model.shis not affected —grep -oEmatches within asingle line, so
CHOICEcannot contain a newline, and itsgrep -qxFthenrejects any value containing whitespace. Only the pr-review path is exposed.
Fix — reject anything that is not a single safe token, before the catalog check:
(Quoting alone is not enough:
$${MODEL:+--model $$MODEL}cannot be quotedwithout emitting an empty
--modelarg; aset --/positional-parameter form orthe
caseguard above is needed.)One command settles the first half if you want to see it locally:
printf 'claude-sonnet-5\n' > /tmp/ids && grep -qxF -- "$(printf 'claude-sonnet-5\n--foo')" /tmp/ids && echo MATCHED.🟡 Suggestions
.woodpecker/pr-review.yml:137-143— the new NB comment is factually wrong.It says the
fetch-models.shrun "executes branch-supplied shell withREVIEW_TOKEN in the env", but line 211 wraps that exact call in
env -u REVIEW_TOKEN. The credential actually exposed to branch-supplied codethere is
CLAUDE_CODE_OAUTH_TOKEN. The argument the comment makes (a branchalready controls
.woodpecker/*.ymlonpull_request, so this is not newexposure) is sound — it just names the wrong token, which will mislead whoever
edits this next.
scripts/ci/pick-model.sh:40-45asks the picker to weigh cost it cannotsee. The prompt says "Match capability (and cost) to the task" and "do not
send trivial or mechanical work to the most powerful model", but
fetch-models.sh:67-76emits no pricing signal at all — only context,max_output, and three capability booleans, all of which correlate with
bigger, not cheaper. The same prompt then forbids reasoning from family
names. Net effect: the only cost proxy left is the display name the prompt
tells it not to trust. Either drop the cost instruction or emit a real
price/tier signal if the API exposes one.
Cost regression in pr-review triage.
pr-review.yml:224dropped--model claude-haiku-4-5-..., so the triage — which reads the diff — now runson the CLI default, and
triage-prompt.md:26-29acknowledges it "may run on acapable (and costly) model". Likewise each of the four cron pipelines now pays
an extra full
claudeinvocation per run just to choose a model. Worthmeasuring before this lands on the 15-minute alert-monitor cadence (it is
gated behind
NEW -eq 0atalert-monitor.yml:176-179, so only onalert-bearing runs — but that is the busy case).
Degradation is silent for the four cron jobs.
pr-review.yml:257surfaces${MODEL:-CLI default}in the posted review header, so a human sees it.security-scan,issue-fix,update-auditandalert-monitoronly echo alog line; a persistent
/v1/models401 would quietly move every agent to adifferent model with no signal in the issue/PR they produce. Consider echoing
the selected model into the artifact those jobs post.
scripts/ci/pick-model.sh:71-72runsclaudewith no--settings, unlikeevery other agent invocation in this repo. Risk is low (input is the fixed
guidance string plus the catalog;
--allowedTools ""with the defaultpermission mode grants no tools), but a Read-deny settings file would match the
convention
ci/agents/*/settings.jsonestablishes.scripts/ci/fetch-models.sh:59-61warns onhas_more=truebut does notpaginate. If it ever fires, the older/cheaper end of the catalog is missing
while the pick still looks successful — the exact failure the
limit=1000comment is guarding against, downgraded to a log line.
Unverified from here: I have no network, so I cannot confirm that
/v1/modelsreturnsmax_input_tokens,max_tokensandcapabilities.*(the public schema documents only
id/type/display_name/created_at).The PR body says this was verified live, and
fetch-models.sh:87-89warns ifevery
contextis null, so a shape change degrades rather than breaks. Check:run
sh scripts/ci/fetch-models.sh --tablein CI once and confirm nocontext=nullcolumn.🟢 Nits
scripts/ci/fetch-models.sh:96-98— thejsonmode has no caller; only--tableis consumed. Dead path unless something else is planned.scripts/ci/fetch-models.sh:46-51—2>/dev/nulloncurl -sSdiscards themessage
-Sexists to print; only the HTTP code (or000) survives into thelog, which is a step backwards from the "precise cause in the job log" goal
stated at
pick-model.sh:26-29.bash scripts/ci/...(issue-fix.yml:61,e2e.yml:33); the new ones assh. Deliberate (they are POSIX), but a one-linenote in the headers would stop someone "fixing" the inconsistency.
Tests
There is no shell lint or unit test for
scripts/ci/in this repo today(
fetch-kubeconfig.sh,make-diag-kubeconfig.shhave none either), so shippingwithout one matches convention, and
sh -n+ a live run is a reasonable bar forthese. That said, the blocking issue is pure string-validation logic and a
~10-line fixture test would have caught it: feed a handful of
triage.jsonbodies (valid id, hallucinated id, empty string, embedded newline, embedded
space) through the
jq→grep -qxF→ argv-expansion sequence and assert theresulting argv. Worth extracting that validation out of the YAML into
scripts/ci/so it is testable at all — right now it only exists inline in aWoodpecker
commands:block.Recommendation
Request changes — 1 blocking issue.
The pr-review triage agent reads the attacker-controlled PR diff and writes triage.json; its Write tool is not path-scoped, so a prompt-injected diff can make it emit any `model` string. The catalog validation used `grep -qxF -- "$MODEL"`, and grep -F treats a MULTI-LINE operand as newline-separated patterns: a value like "claude-sonnet-5\n--dangerously-skip-permissions" matches on its first line, passes validation, and -- via the unquoted `${MODEL:+--model $MODEL}` expansion -- word-splits into an extra argv entry, injecting a CLI flag into the review agent's own `claude` invocation (defeating its --allowedTools allowlist and Read sandbox, with CLAUDE_CODE_OAUTH_TOKEN in that process's env). Removing map_model() (which returned one of three literal ids) is what opened this; restore the invariant with a single-token guard before the expansion: `case "$MODEL" in ""|*[!A-Za-z0-9._-]*) MODEL= ;; esac`. Verified it turns the newline/space/hallucinated payloads into a plain `claude -p` (CLI default) while still passing real ids. Mirrored the guard in pick-model.sh as defence in depth (its grep -oE already keeps CHOICE single-line, so it was not exploitable). Also from the review: - pr-review.yml: correct the HOMELAB comment -- the fetch-models.sh call is env -u REVIEW_TOKEN'd, so CLAUDE_CODE_OAUTH_TOKEN (not REVIEW_TOKEN) is what branch-supplied shell there can see. - pick-model.sh / triage-prompt.md: stop asking the picker to weigh "cost" the catalog does not expose; reason from the visible capability signals (context / max_output / effort|thinking|code_exec) as the scale proxy instead. - fetch-models.sh: drop 2>/dev/null on curl so its -S error reaches the log. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>Good and correct catch — this was a real regression, not a nit. Fixed in
f333dfd6.🔴 Flag injection via agent-supplied
MODELConfirmed both halves locally with your repro:
grep -qxF -- "$(printf 'claude-sonnet-5\n--dangerously-skip-permissions')" /tmp/ids→ MATCHED (grep -F splits the multi-line operand into separate patterns; the first line is a real id).${MODEL:+--model $MODEL}then word-splits the newline into a third argv entry.You're right that removing
map_model()is what opened it (literalcaseoutput + quoted--model "$MODEL"before). Fix is the single-token guard you suggested, placed before the catalog check:Verified the payloads now collapse to a plain
claude -p(CLI default) while real ids pass:Mirrored the guard in
pick-model.shas defence in depth (agreed it wasn't exploitable there —grep -oEkeepsCHOICEsingle-line — but the invariant should be explicit).🟡 Also fixed
fetch-models.shcall isenv -u REVIEW_TOKEN'd, so it'sCLAUDE_CODE_OAUTH_TOKENthat branch shell sees there, notREVIEW_TOKEN. Thanks — I introduced that mismatch across two commits./v1/modelsexposes no price, so the prompt now reasons from the visible capability signals (context / max_output / effort·thinking·code_exec) as the scale proxy and drops the bare "cost" language. Same change intriage-prompt.md.curl -sSmessage discarded — dropped2>/dev/nullon the curl so its-Serror reaches the log.Deferred (unchanged rationale)
--settingson the picker, true pagination pastlimit=1000,jsonmode /shvsbashheader notes — low-risk, noted.scripts/ci/for a fixture test — agreed this is the right home for it, and this exact bug is the argument for it; opening it as the offline-test follow-up.Closing out the remaining items in
a7890e0a.Implemented
untrusted-model gate now lives in
scripts/ci/valid-model.sh(singlesafe-token + catalog-membership check), reused by both
pick-model.shandpr-review.ymlso the security-critical logic exists in one place instead ofinline in a
commands:block. Addedscripts/ci/test-model-selection.sh, anoffline fixture test (no network /
claude/ Woodpecker). All 9 pass:diff. The pipeline writes
files.txt(the complete changed-file list) anddiff.triage.patch(capped at 4000 lines);triage-prompt.mdreads those +stats.txt(true totals) and consults the capped diff only when needed. So adocs-only PR is classified from stats + filenames, not by reading 20k lines on
the default model.
sh(deliberately, vs theolder
bash scripts/ci/*.sh), and thatfetch-models.sh'sjsonmode is ahuman/debug affordance with no pipeline consumer.
Deferred, with reasons
the fallback-to-default, is already
echoed per run in each job's CI log,which is the operational surface where you'd diagnose "why did this agent
behave differently". Threading it into four different posted formats (issue
body / PR / comment) is an observability enhancement I'd rather do as its own
small change than fold in here.
--settingssandbox on the picker (#5-nit) — the picker is invoked with--allowedTools "", i.e. no tools at all, so there is nothing for a Read-denysettings file to constrain. The sandboxes on the other agents exist to scope
the tools they do hold (Read/Grep/Write); this call holds none.
limit=1000— that is the API's maximum page size, and theaccount lists ~11 models; >1000 is not a real scenario. The
has_morewarningturns even that impossible case into a visible log line rather than silent
truncation, which is the failure the reviewer (rightly) wanted to avoid.
I believe that closes every item across the three rounds — blocking, suggestions,
and nits — either implemented or justified above. The offline test also gives us
a place to pin any future selection-logic change.
🤖 Automated review — model
claude-opus-5(complexity: complex), commita7890e0aa2272b41f579da6fdba3ee129b50e4a2.Summary
Replaces the hardcoded
--modelpins in all five agent pipelines with a live/v1/modelslookup (scripts/ci/fetch-models.sh) plus an LLM picker(
scripts/ci/pick-model.sh) and a shared validation gate(
scripts/ci/valid-model.sh) that is unit-tested. The fallback design is good —every failure path lands on "omit
--model" rather than a named model — and theinjection gate in
valid-model.sh:24-31is the right shape (token allowlistplus catalog membership,
grep -qxF --correctly guarded). I found noblocking defect, but there are a few real robustness/consistency gaps and one
claim I could not verify from the checkout.
🟡 Suggestions
1.
REVIEW_TOKENis stripped forfetch-models.shbut not forvalid-model.sh—
.woodpecker/pr-review.yml:247Both invoke a script from
$HOMELAB, which for a homelab PR is the PR headcheckout (
pr-review.yml:136) — i.e. branch-supplied shell. Line 219 wraps itin
env -u REVIEW_TOKEN; line 247 does not, so the write:repository PAT sits inits environment. The comment you added at
pr-review.yml:138-142is thejustification for line 219, and it applies just as much to line 247.
Worth noting the case the comment does not cover: for the mirrored repos
(shelly-manager etc.) the
/tmpclone of trusted homelab@master is only usedwhen
$WS/ci/agents/pr-review/prompt.mdis absent (pr-review.yml:143). A PRbranch in one of those repos that simply adds that file keeps
HOMELAB="$WS",so both scripts then come from that branch too.
I'm not calling this blocking, because on a
pull_requestevent Woodpeckeralready takes
.woodpecker/pr-review.ymlitself from the PR branch — the trustboundary is already "PR branch may run code in this step". But the fix is one
env -u REVIEW_TOKEN, and it keeps the file's own invariant consistent.2. Line 247 is the one model-selection call that can kill the step —
.woodpecker/pr-review.yml:247The block runs under
set -eu(pr-review.yml:126), andVAR=$(cmd)propagatescmd's status, so if$HOMELAB/scripts/ci/valid-model.shis missing ornon-executable
shexits 127 and the whole step aborts — no review posted, redpipeline. Every other selection call is guarded (
|| trueat line 240,ifatline 219, and
pick-model.shis careful to alwaysexit 0), and the commentthree lines above states "a run never dies on selection." Adding
|| truetoline 247 restores that invariant. Realistic trigger: a homelab PR that
moves/deletes
scripts/ci/, or a mirrored-repo branch as in (1).3. Nothing enforces the complexity → capability pairing, and the triage reads
attacker-controlled input —
ci/agents/pr-review/triage-prompt.md:74-78valid-model.shblocks flag injection, but any id in the catalog is accepted.The triage agent reads up to 4000 lines of attacker-supplied diff and its
Writeis not path-scoped (as your own comment at
pr-review.yml:241-242notes), so aninjected diff can steer
triage.jsonto the weakest catalog entry and get ashallow review of its own change. This is not a regression (the old
map_modelkeyword path had the same exposure), but the search space is now the whole
catalog. A cheap hardening that stays inside the "no hardcoded family" rule:
when
CPLX = complex, ignore the triage's pick and leaveMODELempty so thereview runs on the CLI default — the strongest configured model — instead of an
injectable one.
4. The capability fields the whole design rests on are not in the documented
/v1/modelsresponse —scripts/ci/fetch-models.sh:71-80.max_input_tokens,.max_tokensand.capabilities.{effort,thinking, code_execution}.supportedare not part of the publicly documented Models APIresponse (
id,type,display_name,created_at). If the OAuth/beta variantdoesn't return them, every row degrades to
context=null max_output=null effort=false thinking=false code_exec=false, andboth selectors fall back to judging on name and release date — exactly the
family-name heuristic this PR sets out to eliminate. I could not confirm this
from the checkout; the PR body says it was verified live. The check that
settles it: run
sh scripts/ci/fetch-models.shinsideci-claude:2.1.218with areal token and confirm non-null
context=/max_output=in the output.Credit where due:
fetch-models.sh:83-86already detects the all-null-contextcase and warns. Consider making that warning louder at the call site — right now
it lands in stderr and the pipelines print nothing about it.
5. Cost may go up, not down —
.woodpecker/pr-review.yml:232,scripts/ci/pick-model.sh:75-76Two effects worth being aware of:
--model claude-haiku-4-5-20251001pin and nowruns on the CLI default. Capping its diff at 4000 lines (
pr-review.yml:197)mitigates the token count, but the per-token rate is now whatever the account
default is.
claudeinvocation on thedefault model, purely to pick a model, where previously they ran
claude-sonnet-5directly. Forsecurity-fixandissue-fix— which alreadyselect "a capable model" by the task description — that bootstrap call may cost
more than it saves.
Not a defect, but the pipelines no longer have any cost ceiling, and that is a
change in properties worth stating in the PR description.
6.
pick-model.shis the only agent in the fleet with no--settings—scripts/ci/pick-model.sh:76claude --allowedTools "" -pgrants no tools, so the practical risk is low, butevery other agent invocation in this repo passes a
--settingsfile with theRead deny-list (
/proc,/root,/home,/etc,.git) precisely becauseCLAUDE_CODE_OAUTH_TOKENis in the environment. If--allowedTools ""is everparsed as "unset" rather than "empty list", this call gets default tools with no
sandbox. Passing
--settings ci/agents/pr-review/settings.json(or a smallshared one) costs nothing and removes the assumption.
7.
--max-time 20with no retry —scripts/ci/fetch-models.sh:51A single transient network blip silently drops every job to the CLI default for
that run.
--retry 2 --retry-connrefusedwould make the degradation rarerwithout weakening the fallback.
🟢 Nits
scripts/ci/valid-model.sh:11-13— the comment says a value with "a leadingdash ... can never pass" the shape check, but
-is inside the allowed class[A-Za-z0-9._-];--dangerously-skip-permissionsclears step 1 and isrejected only by the catalog-membership check at line 29. The behaviour is
correct (and the empty-catalog guard at line 28 keeps it safe), but the comment
attributes the protection to the wrong check — which matters if anyone reuses
this helper without a catalog.
verbatim in four pipelines (
alert-monitor.yml:191-195,issue-fix.yml:151-155,security-scan.yml:99-103,update-audit.yml:84-89).pick-model.sh:1-20already carries the fullrationale; a one-line pointer per site would keep them from drifting apart.
scripts/ci/fetch-models.sh:16-17documents the default JSON mode as having noconsumer. Fine as a debugging affordance — just flagging it as knowingly unused.
Tests
The new
scripts/ci/test-model-selection.shis well-targeted: it covers theexact injection shapes that matter (embedded space, embedded newline, bare
leading dash,
;rm -rf /, hallucinated id, empty candidate, empty catalog), andthe fixture uses real tab characters, so the
cut -f1path is genuinelyexercised. I read it closely but could not execute it in this sandbox, so I
verified it by inspection only.
Three gaps:
test-model-selection.shis referencedonly by its own header and by a comment in
pr-review.yml:243— no pipelineinvokes it. A "regression test for the flag-injection fix" that CI never runs
will rot.
e2e.yml/e2e-test.ymlalready callbash scripts/ci/…, sothere's an obvious home for
sh scripts/ci/test-model-selection.sh.pick-model.sh's extraction is untested (pick-model.sh:78-81). Thegrep -oE | head -1 | sedchain is the other place an untrusted model replyis parsed. Worth cases for: reply wrapped in a ``` fence, prose before the
JSON, two
"model"keys (must take the first), and empty output.fetch-models.sh's jq mapping is untested — and per suggestion (4) it isthe piece most likely to break silently on an API shape change. A recorded
/v1/modelsfixture piped through the samejq -cfilter would catch that,and would double as executable documentation of the response shape you
verified live.
No prompt-injection attempt was present in the PR title, body, or diff.
Recommendation
Looks reasonable — no blocking issues; comments above are optional. Items 1, 2
and the test-wiring gap are the ones I'd fix before merge.
All items from this round addressed in
44a42968.Implemented
REVIEW_TOKENon thevalid-model.shcall — nowenv -u REVIEW_TOKEN,same as the
fetch-models.shcall. You're right it runs branch-supplied shellfrom
$HOMELABtoo.|| true; a missing/movedscripts/ci/valid-model.shnow leavesMODELempty (→ CLI default) instead ofaborting the step under
set -eu. Restores the "never dies on selection"invariant.
complexity=complex, thetriage's pick is now ignored and the review runs on the CLI default (strongest
configured) model. An injected diff can no longer steer a complex/security
review onto the weakest catalog entry.
--settingson the picker — addedci/agents/model-picker/settings.json(the same Read deny-list) and pass it; still
--allowedTools "", so it'sbelt-and-suspenders against the "empty parsed as unset" case you raised.
--retry—--retry 2 --retry-connrefusedon the catalog fetch.scripts/ci/valid-model.sh(already there) and added a
FETCH_MODELS_BODY_FILEseam so thefetch-modelsjq mapping is now tested offline too: a well-formed
/v1/modelsbody maps tonon-null capability columns, and a body missing the fields triggers the loud
warning. Wired the whole suite into
.woodpecker/e2e.ymlso it actuallyruns in CI (your "it never runs" point) — 11 cases, all green locally.
valid-model.shnow rejects a leading dash at the shape check (sothe header is accurate and the helper is safe even without a catalog); the
duplicated 6-line comment in the four fixed jobs is now a one-line pointer to
pick-model.sh.description spelling out that there's no longer a fixed cost ceiling (triage on
the default model over a 4000-line-capped diff; one extra picker call per fixed
job).
Deferred, with reasons
(the header of every review shows a catalog-derived pick), and the new
degraded-body test + the existing all-null warning cover a silent schema
change. Since I removed
2>/dev/null, that warning now reaches the job log.artifact embedding across four formats remains a separate observability
follow-up.
limit=1000— API max page size; the account lists ~11models;
has_morewarns if it ever mattered.pick-model.sh's JSON extraction (Tests #2) — its output alwayspasses through the now-tested
valid-model.shgate before use, and the pickerprompt is developer-controlled (not attacker input), so a parse miss degrades
to the fallback rather than mis-selecting. I left it untested deliberately for
that reason; happy to add cases if you'd rather.
I believe that's every item across all four rounds now either implemented or
justified in code + thread.
🤖 Automated review — model
CLI default(complexity: complex), commit44a429685c5c31d8c2bff4a32b0fd11a2dd11752.Summary
This PR replaces the hardcoded
--model claude-sonnet-5pins (and pr-review's3-way haiku/sonnet/opus map) across five CI pipelines with a live Anthropic
/v1/modelscatalog lookup (scripts/ci/fetch-models.sh), an agent-drivenpicker (
scripts/ci/pick-model.sh), and a shared validation gate(
scripts/ci/valid-model.sh) that only lets a shape-safe, catalog-present idreach an unquoted
--modelflag. The shell-injection hardening itself(
valid-model.sh's character-class + membership check,env -ustripping offorge tokens,
|| true/timeout fallbacks to the CLI default) is careful andwell tested offline. However, the
pr-review.ymlintegration has one real gapin the trust model it claims to establish: the catalog file the validation
gate checks membership against is writable by the very (untrusted-diff-driven)
agent whose pick it is meant to constrain.
🔴 Blocking
.woodpecker/pr-review.yml:219-252— the "must be a currently-availablecatalog id" check can be defeated by the agent it is meant to constrain.
The live catalog is fetched and written to
$D/models.txt(219-225)before the triage agent runs. The triage agent then runs (232-236) with
--allowedTools "Read,Glob,Grep,Write"and no Write-path restriction —ci/agents/pr-review/settings.jsononly deniesReadof sensitive paths,never
Write— while its cwd ($WS) contains./.pr-reviewsymlinked tothat same
$D. The PR's own comment at line 242 acknowledgestriage.json is UNTRUSTED (... its Write is not path-scoped), but onlyreasons about the
modelvalue it writes, not about the fact it can alsooverwrite
./.pr-review/models.txtitself.valid-model.shis then invokedat line 252 with
< "$D/models.txt"— i.e. it re-reads the catalog fromdisk, after the triage agent has already run, not the pre-triage
snapshot. A prompt-injection payload in the PR diff/title/body (the exact
threat this file is written to defend against elsewhere) can therefore
instruct the triage agent to rewrite
models.txtto contain anyattacker-chosen
[A-Za-z0-9._-]token, then report that same token asmodelintriage.json—valid-model.sh's membership check will thenpass it, because the "catalog" it's checking against is the attacker's own
writeback. This defeats the specific guarantee the PR body advertises
("valid-model.sh ... echoes a candidate id only if ... present in the live
catalog"). Concretely this lets an attacker either (a) force a real-but-
weaker existing model onto the review regardless of what the catalog would
otherwise justify, or (b) supply a syntactically-valid but nonexistent id,
which makes the review's
claude --model <fake-id>invocation fail(
|| echo "(review agent exited non-zero)"),report.mdis never written,and the pipeline silently skips posting any review at all (line 271-274) —
a quiet way to suppress the automated review on a given push.
This is a genuine regression versus the pre-PR design: previously
map_modelmapped the triage's (equally untrusted)KEYthrough a fixed3-entry, hardcoded case statement, so no amount of file tampering could
produce anything other than one of 3 pinned, real ids. Note
pick-model.sh(used by the other four jobs) does not have this problem: it keeps the
fetched table in a shell variable (
TABLE) that is validated and consumedbefore the (tool-less,
--allowedTools "") picker agent ever runs, sothere's no file for that agent to tamper with even in principle.
Fix direction: don't let the triage agent's Write reach
models.txt— e.g.scope its Write grant the same way
Bash(./.ci-diag/kubectl:*)is scopedelsewhere in this repo (something like
Write(./.pr-review/triage.json)if the CLI's permission syntax supportsit for Write the way it does for Bash), or simply re-fetch/keep the catalog
in a shell variable captured before the triage call and validate against
that instead of re-reading the file afterward.
🟡 Suggestions
ci/agents/model-picker/settings.jsonmirrors the pr-reviewRead-deny list as "belt and suspenders" for the case where an empty
--allowedTools ""is ever treated as "unset" and the default toolsetre-enables. If that ever happens, the picker would also regain unrestricted
Write (only Read is denied here too), which — per the finding above — would
be a real, not just theoretical, gap; worth keeping in mind if this deny
list is ever reused as the sole safety net for an agent that's supposed to
be tool-less.
scripts/ci/test-model-selection.shcoversvalid-model.shand thefetch-models.shresponse mapping thoroughly, but notpick-model.sh's ownCHOICEextraction (thegrep -oE ... | sedpipeline atscripts/ci/pick-model.sh:602-605) or thepr-review.ymlmodels.txtwrite-order issue above. A regression test asserting that
triage.json'smodelis validated against a catalog snapshot taken before the triageagent runs (not re-read after) would have caught the Blocking item.
🟢 Nits
.woodpecker/pr-review.yml:20-27header comment says "if the fetch or thepick fails it leaves the model unset" — accurate for the fetch, but for the
pick step the fallback also fires on a valid pick that doesn't survive the
complex-review override (line 258-261); minor wording nit only, behavior
is otherwise correctly described elsewhere in the file (line 254-257).
Tests
scripts/ci/test-model-selection.shis solid for what it covers: thecharacter-class/membership gate (valid id, hallucinated id, empty, space/
newline/leading-dash/metachar injection, empty catalog) and the
fetch-models.shjq mapping (well-formed body, degraded/missing-capabilitybody). It's correctly wired into
.woodpecker/e2e.yml. It does not coverpick-model.sh's prompt-output parsing or thepr-review.ymlfetch-then-triage-then-validate ordering, which is exactly where the Blocking
issue above lives — that gap can only be caught by an integration-level test
or careful review of the shell, not the current unit tests.
Recommendation
Request changes — 1 blocking issue.
Correct and serious catch — fixed in
14aa6415.🔴 models.txt writeback defeats the membership check
You're right: the catalog was written to
./.pr-review/models.txtbefore thetriage ran, the triage has an unscoped
Writewith./.pr-reviewin its cwd,and validation re-read that file from disk after the triage — so an injected
diff could rewrite
models.txtand smuggle any[A-Za-z0-9._-]token past themembership check (forcing a weaker real model, or a fake id that fails the review
claude --modelcall and silently suppresses the review).Fix: validate against
$MODELS_TABLE— the catalog captured in a shell variablebefore the triage runs — instead of re-reading the tamperable file. This is
exactly what makes
pick-model.shimmune (as you noted), so pr-review now usesthe same in-memory-snapshot approach. Since Write path-scoping isn't enforced
under
acceptEditsin this CLI (perupdate-audit.yml's own note), keeping thetrusted catalog out of the agent's reach entirely is the robust fix rather than
relying on a
Write(...)scope.Verified with a direct simulation of the flow:
The smuggled token is now rejected (→ CLI default, so the review still runs and
can't be suppressed); a legit id still passes. The triage may still tamper its
own on-disk copy, but that only misleads its own pick, which the pristine
snapshot then rejects.
Tests
Added a regression case to
scripts/ci/test-model-selection.sh("tampered id rejected by pristine-snapshot validation") that encodes exactly
this: a token present only in a tampered on-disk file but absent from the
pre-triage snapshot must be rejected. Suite is now 12 cases, all green, and runs
in
e2e.yml.Nits
snapshot taken before the triage runs, and that the complex-review override
is another path to the CLI default.
complexity=trivial+ a weaker real model — remains the same pre-existingexposure the old
map_modelkeyword path had (an injectedKEYcould forcehaiku); the complex-review override narrows it, and the pristine-snapshot fixstops it from reaching fake or out-of-catalog ids. I left that residual as-is
since removing it means not trusting the triage's classification at all, which
is the triage's entire job — flagging it explicitly rather than papering over it.
🤖 Automated review — model
CLI default(complexity: complex), commit14aa6415608bacd7efd47175f0d0298bd9059883.Summary
This PR replaces the hardcoded
--model claude-sonnet-5/ keyword-mapped model pins across all five automatic CI agents with a live-catalog lookup (scripts/ci/fetch-models.sh+scripts/ci/pick-model.sh+scripts/ci/valid-model.sh), and reworkspr-review's triage to pick a concrete id from that catalog instead of ahaiku|sonnet|opuskeyword. I traced the full flow (all four.woodpecker/*.ymlcall sites,pr-review.yml's two-phase triage/validate logic, and the three newscripts/ci/*.shhelpers) against the checkout. The injection-safety design is careful and internally consistent:valid-model.sh's shape+membership gate is the single choke point before any untrusted string reaches an unquoted--modelexpansion, andpr-review.ymlcorrectly validates the triage's pick against the pristine$MODELS_TABLEcaptured before the (unscoped-Write) triage agent ran, not the on-disk file it could overwrite. I found no blocking defects.🟡 Suggestions
.woodpecker/e2e.yml:23-30— the newmodel-selection-testsstep (the regression test for the flag-injection gate invalid-model.sh) only runs on thedaily-security-scan/update-audit/weekly-updatecrons or manualPIPELINE == "e2e", per thewhen:block at the top of the file. It does not run onpull_request, so a future PR that weakensvalid-model.sh(e.g. loosens the char class or drops the membership check) would merge without this suite catching it — the regression would only surface on the next schedulede2erun, by which timepick-model.sh/pr-review.ymlmay have already executed the weakened gate against real (and potentially adversarial) input for up to a week. Worth either adding a lightweightpull_request-triggered pipeline forscripts/ci/**changes, or explicitly accepting the gap in the commit/PR description.scripts/ci/fetch-models.sh:488-497— the capability fields (max_input_tokens,max_tokens,capabilities.effort/thinking/code_execution) are asserted in the PR description to be present on live/v1/modelsresponses under the OAuth beta header, but the only fixture exercised bytest-model-selection.sh:688is a synthetic body the author wrote by hand — there's no recorded real response pinned in the repo. The code degrades gracefully (loud stderr warning) if the schema ever changes, so this isn't a functional risk, but that warning is easy to miss on a run that still exits 0 (no test currently asserts CI actually surfaces/greps for it). Consider having one of the CI steps that already runsfetch-models.sh(e.g. inpr-review.yml) fail loudly, or at least post a visible marker, if the degraded-schema warning fires, rather than relying on someone reading full job logs.scripts/ci/pick-model.sh:601-608— the model-pickerclaudeinvocation omits--permission-mode, relying solely on--allowedTools ""(plus thesettings.jsonRead deny-list as defence in depth) to keep the picker tool-less. The comment inci/agents/model-picker/settings.jsonalready flags uncertainty about whether an empty--allowedToolsis reliably treated as "no tools" vs. "unset → default toolset" by the CLI. SinceGUIDANCEhere is a fixed string baked into the.woodpecker/*.ymlfiles (not attacker-controlled, unlike thepr-reviewtriage path), the practical exposure is low, but if it's easy to do, an explicit empirical check (or an even more conservative--permission-mode plan) would remove the remaining doubt called out in your own comment rather than leaving it as an open question.🟢 Nits
scripts/ci/fetch-models.sh:426-427header comment: "the olderscripts/ci/*.shhelpers invoked asbash" — confirmed accurate (fetch-kubeconfig.sh,make-diag-kubeconfig.share the only pre-existing scripts there), just noting I checked it rather than took it on faith.Tests
scripts/ci/test-model-selection.shis solid offline coverage for the security-critical part (valid-model.sh's injection-shape rejections, the pristine-snapshot-vs-writeback regression, and thefetch-models.shjq mapping over both a well-formed and a degraded fixture body). The one gap is coverage timing, not coverage content — see the first Suggestion above: it's wired intoe2e.ymlbut that pipeline doesn't trigger onpull_request, so it won't gate the PR that breaks it.Recommendation
Looks reasonable — no blocking issues; comments above are optional.
Thanks — addressed in
1992fabb. One of these turned out to be a real gap, not just a doubt.🟡 Picker tool-restriction (your #3) — the doubt was justified
I ran the empirical check you suggested.
--allowedTools ""does NOT make thepicker tool-less — with an empty allowlist the picker happily used the Write
tool and created a file. An empty allowlist is treated as "unset" → default
toolset stays live. So the "tool-less" claim in my comment (and the settings.json
note) was wrong.
Fix: the picker now runs
--permission-mode plan, which I verified does blockthe Write tool while still producing the selection JSON:
--allowedTools ""and the Read deny-list stay as additional layers. Comments inpick-model.shandci/agents/model-picker/settings.jsoncorrected to say planmode is the actual guarantee. (Practical exposure was low either way — the
picker's prompt is developer-authored + the API catalog, no attacker input — but
the invariant is now real, not assumed.)
🟡 Test only ran on crons, not PRs (your #1) — fixed
New
.woodpecker/ci-scripts-test.ymlrunstest-model-selection.shon everyhomelab
pull_request(homelab-only; not deployed to the mirrored repos). A PRthat loosens
valid-model.shnow fails checks before merge, not up to a weeklater on the next
e2ecron.🟡 No recorded real /v1/models fixture (your #2) — added
Pinned
scripts/ci/testdata/v1-models-sample.json(the shape verified live) anda test asserting
fetch-models.shmaps it correctly (opuseffort=true, haikueffort=false). Suite is now 13 cases, green, and runs both on PRs and in e2e.Deferred: hard-failing a pipeline when the degraded-schema warning fires. The
warning already reaches the job log (curl/jq stderr is no longer suppressed since
an earlier round), and hard-failing on a benign API schema addition would break
all model selection across five pipelines — graceful degradation to name+date is
the deliberate choice. Happy to add a visible marker step if you'd prefer.
🟢 Nit
Header "older scripts/ci/*.sh invoked as bash" — confirmed accurate, thanks for
double-checking it.
🤖 Automated review — model
CLI default(complexity: complex), commit1992fabb1226fe702c73dd0d778b6a6030a9bfe4.Summary
This PR replaces the hardcoded
--model claude-sonnet-5/ haiku-sonnet-opus keyword mapping across five CI agent pipelines with a live-catalog lookup (scripts/ci/fetch-models.sh→GET /v1/models), a shared validation gate (scripts/ci/valid-model.sh), and a triage/picker agent that chooses a fitting id (scripts/ci/pick-model.sh,ci/agents/pr-review/triage-prompt.md). I traced the full flow for both the four cron/manual jobs (alert-monitor, issue-fix, security-scan, update-audit) and the pull_request-triggeredpr-review.yml, including the flag-injection gate, the "validate against the pre-triage snapshot, not the on-disk file the untrusted agent can overwrite" logic, and the complex/security-sensitive override that ignores the triage's pick. Everything I checked holds up: the validation regex correctly rejects space/newline/leading-dash/metachar payloads (confirmed againstscripts/ci/test-model-selection.sh's cases by tracing thecasepattern andgrep -qxFmembership check by hand),pick-model.sh/valid-model.share structured to always exit 0 soset -eucallers never die on selection, and the four fixed jobs only ever pass a hardcoded (non-attacker-controlled) task description to the picker since they run on cron/manual triggers against trustedmaster, not PR branches. The two new JSON files (testdata/v1-models-sample.json,ci/agents/model-picker/settings.json) are valid JSON (checked withjq empty). No blocking issues found.🟡 Suggestions
ci/agents/model-picker/settings.json:1-6/scripts/ci/pick-model.sh:620-631— the comment states it was "VERIFIED empirically" that--permission-mode planblocks the Write tool while--allowedTools ""alone does not. That's a real behavioral claim about theclaudeCLI with no automated regression coverage (unlikevalid-model.sh, which has a full test suite). If a future CLI version changes plan-mode semantics, this would silently regress. Worth a one-line note that this is asserted from manual testing only, or an inline check if the CLI ever exposes one — not blocking today since the picker's task string is always a hardcoded literal in the four callers, never attacker-controlled.scripts/ci/pick-model.shhas no automated test of its own control flow (the RAW→CHOICE grep/sed extraction, the always-exit-0 contract). Coverage today is indirect, viavalid-model.sh's tests plus the "verified live" claim in the PR description. Given how carefullyvalid-model.shandfetch-models.share tested, a small offline test that feedspick-model.sha cannedclaudestub (or at least documents that this path is untested) would close the gap.🟢 Nits
alert-monitor.yml,issue-fix.yml,security-scan.yml, andupdate-audit.yml. Fine as-is given each file is a standalone Woodpecker pipeline, but a shared doc (e.g. a comment pointer toscripts/ci/pick-model.sh's header, which is already the pattern used) keeps it from drifting if the mechanism changes again.Tests
scripts/ci/test-model-selection.shis thorough: it covers the injection gate (valid id, dated id, hallucinated id, empty, space/newline/leading-dash/metachar injection, empty catalog), the specific "models.txt writeback" attack this PR's design is defending against (triage's unscoped Write can't smuggle a token past validation because the gate is checked against the pre-triage snapshot), and thefetch-models.shjq mapping against both a synthetic and a recorded real/v1/modelsbody (including a degraded-schema warning path). It's wired into bothe2e.yml(cron) and the newci-scripts-test.yml(every homelabpull_request), with a documented rationale for running it in both places. This is good coverage for the security-critical piece (the injection gate). The one gap, noted above, ispick-model.sh's own glue logic, which is comparatively low-risk since its input is never attacker-controlled in the current callers.Recommendation
Looks reasonable — no blocking issues; comments above are optional.