fix(plugins): dedupe provider-prefixed model refs in runtime LLM allowlist policy#84940
fix(plugins): dedupe provider-prefixed model refs in runtime LLM allowlist policy#84940nxmxbbd wants to merge 1 commit into
Conversation
…wlist policy
The plugin runtime LLM policy gate and the gateway fallback subagent
policy gate each build allowlist keys and resolved-selection keys via
ad-hoc `${normalized.provider}/${normalized.model}` concatenation. When a
provider plugin manifest contributes `prefixWhenBare` (e.g. OpenRouter)
and the resolved selection's `modelId` already carries the provider
segment, the concatenation produces a double-prefixed key
(`openrouter/openrouter/gpt-5.4-mini`). The user-typed allowlist entry
does not carry the duplicate prefix, so `Set.has` misses and the
diagnostic surfaces a confusing `openrouter/openrouter/...` string.
Swap the five concat sites to the existing canonical `modelKey()` helper
(`src/agents/model-ref-shared.ts`), which dedupes when `model` already
starts with `${provider}/`. Both pair sides remain symmetric (allowlist
Set construction + resolved lookup), so the policy gate keeps the same
contract, and the diagnostic now surfaces the deduped, user-recognizable
ref.
Closes openclaw#84887.
|
Codex review: needs maintainer review before merge. Workflow note: Future ClawSweeper reviews update this same comment in place. How this review workflow works
Summary Reproducibility: yes. Source inspection shows current main can normalize an OpenRouter selection to provider PR rating Rank-up moves:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. Real behavior proof Next step before merge Security Review detailsBest possible solution: Land the focused modelKey substitution with its two regression tests after normal CI, leaving broader helper consolidation as a separate cleanup. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main can normalize an OpenRouter selection to provider Is this the best way to solve the issue? Yes. Reusing the existing modelKey contract is the narrow maintainable fix and avoids adding a second runtime-only dedupe rule. Label changes:
Label justifications:
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against bde07ddb1552. |
|
ClawSweeper PR egg ✨ Hatched: 🥚 common Frosted Lint Imp Hatch commandComment Hatchability rules:
Rarity: 🥚 common. What is this egg doing here?
|
|
Closing this as superseded by the narrower maintainer-selected fix in #84946, merged at commit 937a756. That landed the #84887 runtime LLM allowlist diagnostic fix by using |
|
@vincentkoc following up on your closure note here: I opened #85445 as the separate PR for the gateway fallback path. Current state there:
So this closed PR can stay closed; the unresolved gateway fallback path is now tracked in #85445. |
Summary
src/plugins/runtime/runtime-llm.runtime.ts) and the gateway fallback subagent policy gate (src/gateway/server-plugins.ts) each build allowlistSetkeys and resolved-selection keys via ad-hoc${normalized.provider}/${normalized.model}concatenation. When a provider plugin manifest contributesprefixWhenBare(e.g. OpenRouter) and the resolved selection'smodelIdalready carries the provider segment, the concatenation produces a double-prefixed key (openrouter/openrouter/gpt-5.4-mini). The user-typed allowlist entry does not carry the duplicate prefix, soSet.hasmisses and the diagnostic surfaces a confusingopenrouter/openrouter/...string.modelKey()helper atsrc/agents/model-ref-shared.ts:17(already re-exported viasrc/agents/model-selection.ts).modelKeydedupes whenmodelalready starts with${provider}/, so both pair sides remain symmetric: allowlistSetconstruction + resolved-selection lookup produce identical canonical keys, the policy gate keeps the same contract, and the diagnostic surfaces the deduped, user-recognizable ref.modelKey(p, m)swaps + 2 named-import additions acrossruntime-llm.runtime.ts(2 sites) andserver-plugins.ts(3 sites), plus 2 regression tests (one per file) locking in the deduped diagnostic.legacyModelKey(intentional raw concat for legacy lookup), the wider set ofnormalizeModelRefcallers undersrc/agents/**(each requires its own diagnosis), and the cross-filenormalizeAllowedModelRefduplication (known smell, separate refactor).Motivation
#84887 was opened by
100yenadminwho reported their plugin LLM allowlist diagnostic surfacedopenrouter/openrouter/gpt-5.4-minieven though the actionable model ref they had configured wasopenrouter/gpt-5.4-mini. The doubled provider segment confused remediation. The reporter's suggested fix shape ("normalize runtime LLM model refs with a helper that strips only duplicated provider prefixes afternormalizeModelRef()") matches the canonicalmodelKey()helper that already lives insrc/agents/model-ref-shared.ts— the bug here is that two flows didn't use it.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Real behavior proof
Behavior or issue addressed: Plugin LLM runtime allowlist policy and gateway fallback subagent policy both build canonical
provider/modelkeys via ad-hoc concatenation that does not dedupe when the resolvedmodelIdalready carries the provider segment, producing a double-prefixed diagnostic and (in the reporter's environment) a wrongly denied allowlist match.Real environment tested: Local checkout at
/root/repos/openclaw-84887rebased onto currentupstream/main(f39f56a096). Direct production-source invocation vianode_modules/.bin/tsx /tmp/proof-84887.mts, importing the actual productionmodelKeyfromsrc/agents/model-selection.ts(re-exported fromsrc/agents/model-ref-shared.ts). Also focused vitest on the two touched test files.Exact steps or command run after this patch:
Terminal capture from this branch, copied live output of the production
modelKeyinvocation:Focused regression tests on both touched flows, copied live output:
Observed result after fix: The production
modelKeycollapses(provider="openrouter", model="openrouter/gpt-5.4-mini")to the single-prefix canonical key"openrouter/gpt-5.4-mini". TheSet.haslookup that was previously denying legitimate selections now succeeds, and the diagnostic for real deny cases carries the user-recognizable single-prefix ref (matching the reporter's allowlist entry shape). The pre-fix path simulation reproduces the reporter's verbatim diagnostic string character-for-character, confirming the diagnosis end-to-end through the real production helper.What was not tested: No live OpenRouter API call or live Lossless plugin invocation; the bug is host-side normalization, so an end-to-end runtime call is not required to validate the fix. The "allow" case end-to-end through
runtimeContext.llm.completewas not added as a vitest case because in the vitest environment (no OpenRouter plugin manifest loaded) the host'snormalizeProviderModelIdWithRuntimedoes not re-prefix bare model ids, so both Set-construction and resolved-side paths produce symmetric output and the bug becomes invisible at the policy gate (it only surfaces in the diagnostic string). The diagnostic-dedup regression tests in both files cover the same concat logic the allow path depends on.Before evidence (optional but encouraged): The reporter's
[lcm] runtime.llm.complete errorlog in the issue body showsPlugin LLM completion model override "openrouter/openrouter/gpt-5.4-mini" is not allowlisted for plugin "lossless-claw".. The pre-fix path simulation in the proof capture above produces this exact string character-for-character.Root Cause
provider/modelkeys via local ad-hoc${normalized.provider}/${normalized.model}concatenation. The repo already has a canonical helpermodelKey(provider, model)atsrc/agents/model-ref-shared.ts:17that dedupes whenmodelalready starts with${provider}/. The ad-hoc paths were missing the dedup branch. When OpenRouter's manifestprefixWhenBare=openrouterproduces a resolvedselection.modelIdofopenrouter/gpt-5.4-mini, the concatenation generates a doubled key while the user-entered allowlist ref stays single-prefixed; theSet.haslookup misses and the diagnostic surfaces the confusing doubled string.modelIdalready carried the provider segment. Both regression test files in this PR add that coverage.normalizeAllowedModelRef(one inruntime-llm.runtime.ts, one inserver-plugins.ts); both implementations carry the same canonical-key bug. Consolidating them is out of scope for this focused fix and left as a follow-up.Regression Test Plan
src/plugins/runtime/runtime-llm.runtime.test.ts— context-engine LLM deny diagnostic (Runtime LLM allowlist diagnostics can double-prefix provider-qualified model refs #84887)src/gateway/server-plugins.test.ts— fallback subagent override deny diagnostic (Runtime LLM allowlist diagnostics can double-prefix provider-qualified model refs #84887)modelIdarrives with the provider segment already prepended (simulating OpenRouter'sprefixWhenBarebehavior), an allowlist deny must surface the deduped single-prefix ref in the diagnostic and (transitively) construct symmetric canonical Set keys on both sides of the policy check.User-visible / Behavior Changes
Users configuring plugin LLM allowlists or gateway fallback subagent allowlists with single-prefix entries (e.g.
openrouter/gpt-5.4-mini) will now match their selections correctly when the selection'smodelIdarrives provider-prefixed via a plugin'sprefixWhenBarehook. Deny diagnostics will surface the deduped single-prefix ref instead of the previously doubled form. Users who deliberately configured legacy double-prefix entries likeopenrouter/openrouter/hunter-alphawill see those entries collapse to the same canonical key as the deduped resolved ref; the policy gate semantics remain consistent (legacy entries continue to match their intended targets via the deduped canonical key).Security Impact
No)No)No)No)No)This is a focused diagnostic + canonical-key contract fix in two host-side normalization paths. No auth, network, permission, or data-access surface is touched.
Repro + Verification
Environment
Steps
allowedModels: ["openrouter/gpt-5.4-mini"]for a plugin that callsapi.runtime.llm.complete(or use a fallback subagent override targeting OpenRouter).selection.modelIdarrivesopenrouter/gpt-5.4-mini(via OpenRouter's manifestprefixWhenBare=openrouter).model override "openrouter/openrouter/gpt-5.4-mini" is not allowlisted for plugin "X".Expected
Actual (before fix)
Setlookup misses; diagnostic carries the doubled-prefix ref (verbatim match to reporter's log line above).Human Verification
Set.hassucceeds on the rebased branch via production-sourcemodelKeyinvocation; vitest deny-diagnostic regression tests pass in both touched files; fullsrc/plugins/runtime/+src/gateway/vitest scope passed (242 files / 2672 tests) before rebase, focused 2-file scope (64 tests) reran clean post-rebase.pnpm buildcompletes;dist/runtime-llm.runtime-*.jsanddist/server-plugins-*.jscontain themodelKeyreference paths.*allowlist entries short-circuit before the concat path (untouched). Empty provider/model inputs are rejected bynormalizeAllowedModelRefbefore reaching the concat (untouched). Legacy double-prefix allowlist entries collapse to the same canonical key as the deduped resolved ref.legacyModelKey(intentional raw concat for back-compat lookup) is not on the policy-gate path and is untouched.normalizeModelRefcallers undersrc/agents/**outside the two policy-gate flows fixed here (out of scope; each requires its own diagnosis and proof).Compatibility / Migration
Yes)No)No)Users who had configured legacy double-prefix workaround entries see those entries collapse to the same canonical key now used by both the allowlist Set and the resolved-selection key. Single-prefix entries (which the user-facing diagnostics already encouraged) continue to work, now also matching provider-prefixed resolved selections that previously slipped past the policy gate.
Risks and Mitigations
normalizeAllowedModelRef; this PR fixes the bug in both but does not consolidate them.modelKey()helper for key construction, so the contract is consistent across both flows. A separate refactor PR can dedupe the helper body without affecting the canonical-key semantics this PR establishes.normalizeModelRefcallers undersrc/agents/**may have the same ad-hoc concat pattern; this PR does not touch them.#84887