perf(gateway): propagate config context in model normalization to avoid stale policy warning#86372
Closed
medns wants to merge 1 commit into
Closed
perf(gateway): propagate config context in model normalization to avoid stale policy warning#86372medns wants to merge 1 commit into
medns wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR propagates the effective user configuration context into static model ID normalization/selection helpers so plugin metadata snapshot policy hashing is computed consistently, avoiding persistent “stale policy” warnings and unnecessary derived plugin index rebuilds.
Changes:
- Thread
cfg/config(and in some pathsworkspaceDir) into model ID normalization calls (normalizeStaticProviderModelId,normalizeConfiguredProviderCatalogModelId). - Extend normalization context types to carry
configthroughmodel-selection-*utilities. - Adjust
pi-embedded-runnerresolution flow to compute and passworkspaceDirearlier for normalization.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/config/io.write-prepare.ts | Passes config context into provider catalog model ID normalization during config write preparation. |
| src/config/defaults.ts | Passes cfg + manifest plugin context into configured provider catalog normalization while applying defaults. |
| src/agents/pi-embedded-runner/model.ts | Threads cfg/workspaceDir into static model ID normalization in multiple resolution paths. |
| src/agents/model-selection-shared.ts | Propagates cfg into model ref parsing/normalization and catalog building helpers. |
| src/agents/model-selection-normalize.ts | Extends normalization context type to include config, and forwards it into static normalization. |
| src/agents/model-ref-shared.ts | Extends normalization options to include config/env/workspaceDir and forwards them into manifest-based normalization. |
Comments suppressed due to low confidence (2)
src/agents/pi-embedded-runner/model.ts:542
- This normalizeStaticProviderModelId call also omits
workspaceDir. To keep key comparisons consistent with the rest of the model resolution path (and avoid workspace-dependent normalization mismatches), pass the same workspaceDir used for the primary normalizedModelId computation.
normalizeProviderId(candidateProvider) === normalizedProvider &&
normalizeStaticProviderModelId(normalizedProvider, candidateModelId, {
config: params.cfg,
})
.trim()
.toLowerCase() === normalizedModelId
src/agents/pi-embedded-runner/model.ts:1065
- resolveModelWithRegistry() passes
params.workspaceDirinto normalizeStaticProviderModelId, but then later derives an effective workspaceDir fromcfg?.agents?.defaults?.workspace. If callers omit workspaceDir but cfg has a default, modelId normalization happens without the effective workspaceDir, which can reintroduce workspace-scoped manifest/policy mismatches. Consider computing the effective workspaceDir first and using that value for normalization here (similar to resolveModel/resolveModelAsync).
const normalizedRef = {
provider: params.provider,
model: normalizeStaticProviderModelId(normalizeProviderId(params.provider), params.modelId, {
config: params.cfg,
workspaceDir: params.workspaceDir,
}),
Contributor
|
Codex review: needs maintainer review before merge. Reviewed May 28, 2026, 5:42 AM ET / 09:42 UTC. Summary PR surface: Source +47. Total +47 across 6 files. Reproducibility: no. independent high-confidence reproduction was run in this read-only review. The source path is clear, and the PR body supplies after-fix Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land a version that preserves existing config semantics while passing the same config/workspace snapshot through all manifest model-normalization paths, with targeted warm-launch and upgrade proof. Do we have a high-confidence way to reproduce the issue? No independent high-confidence reproduction was run in this read-only review. The source path is clear, and the PR body supplies after-fix Is this the best way to solve the issue? Yes, this is a plausible narrow fix, but the best path requires maintainer upgrade review because the patch deliberately changes config-sensitive normalization during defaults/write and agent model resolution. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against f9aec04167b3. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +47. Total +47 across 6 files. View PR surface stats
What I checked:
Likely related people:
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. How this review workflow works
|
Contributor
|
ClawSweeper PR egg: ✨ hatched 🌱 uncommon Velvet Review Wisp. Rarity: 🌱 uncommon. Trait: collects tiny proofs. DetailsShare on X: post this hatch
About:
|
66ea014 to
c62ecfd
Compare
5f03519 to
7e9701f
Compare
Contributor
Author
|
@clawsweeper review |
Contributor
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
9941cc4 to
b0f7d0b
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/agents/pi-embedded-runner/model.ts:545
- Same formatting issue as above: the call to
normalizeStaticProviderModelId(...)is closed on its own line and then chained with.trim()on the next line. This is likely to be rewritten byoxfmtand may fail formatting checks; please format the chain consistently (e.g.,}).trim().toLowerCase()).
normalizeProviderId(candidateProvider) === normalizedProvider &&
normalizeStaticProviderModelId(normalizedProvider, candidateModelId, {
config: params.cfg,
workspaceDir: params.workspaceDir,
})
.trim()
.toLowerCase() === normalizedModelId
This comment was marked as spam.
This comment was marked as spam.
…minate stale policy registry warnings
b0f7d0b to
e87f856
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
What problem does this PR solve?
resolveInstalledPluginIndexPolicyHash/persisted-registry-stale-policy) by propagating the user configuration context (config/cfg) down to all mid-to-lower level static model normalization and selection helpers.Why does this matter now?
{}defaulted config parameters caused the registry snapshot policy check to compute a mismatched policy hash, triggering an unnecessary fallback to a slow derived plugin index rebuild and filling terminal logs with stale-policy warnings.What is the intended outcome?
normalizeStaticProviderModelId), leading to a 100% cache hit rate for the process-stable plugin metadata snapshots on warm reloads/launches.What is intentionally out of scope?
What does success look like?
openclaw models statusor launching an agent does not output any registry policy stale warnings, and directly loads from the memoized snapshot in memory, resulting in ~70% faster CLI command response.What should reviewers focus on?
Check the parameter threading across
model-ref-shared.ts,model-selection-shared.ts, andpi-embedded-runner/model.ts.Note: Mark as AI-assisted in the PR title or description. Prompts or session logs were reviewed and verified by a human operator.
Linked context
Which issue does this close?
Closes #
Which issues, PRs, or discussions are related?
Related to stale policy registry warnings and cache misses.
Was this requested by a maintainer or owner?
431467405486ad0cde9a739997c4e17924deccf5("perf: reuse plugin metadata snapshots").Real behavior proof (required for external PRs)
Run the model status self-test command:
pnpm test src/agents/model-ref-shared.test.ts src/config/defaults.test.tsThe model status self-test command completed successfully with 0 diagnostics/warnings of type
persisted-registry-stale-policy. Below is the terminal capture showing warm launch completion:The
openclaw models statuscommand launched successfully, correctly compared the policy hash againstpersistedIndex.policyHashwith a 100% cache hit, and executed without throwing any warning or initiating a slow derived rebuild, completing in ~1.1s (a ~70% reduction in CLI latency).Did not test against external OAuth login callbacks as those are managed by separate provider plugins.
Persisted plugin registry policy does not match current config; using derived plugin index...on startup.Tests and validation
Which commands did you run?
What regression coverage was added or updated?
model-ref-shared.test.tsandplugin-metadata-snapshot.memo.test.tsverify that correct option and workspace parameters are loaded and matched correctly.What failed before this fix, if known?
{}defaulted config parameters caused the registry snapshot policy check to compute a mismatched policy hash, triggering an unnecessary fallback to a slow derived plugin index rebuild and throwing stale policy warnings.Risk checklist
Did user-visible behavior change? (
No)Did config, environment, or migration behavior change? (
No)Did security, auth, secrets, network, or tool execution behavior change? (
No)What is the highest-risk area?
How is that risk mitigated?
Current review state
What is the next action?
What is still waiting on author, maintainer, CI, or external proof?
Which bot or reviewer comments were addressed?
perf: reuse plugin metadata snapshotscommit.