Skip to content

fix: preserve user-defined blockrun/* allowlist entries across restarts#84

Merged
1bcMax merged 1 commit intomainfrom
fix/preserve-user-allowlist-entries
Mar 8, 2026
Merged

fix: preserve user-defined blockrun/* allowlist entries across restarts#84
1bcMax merged 1 commit intomainfrom
fix/preserve-user-allowlist-entries

Conversation

@1bcMax
Copy link
Copy Markdown
Member

@1bcMax 1bcMax commented Mar 8, 2026

Summary

  • injectModelsConfig() was destructively deleting any blockrun/* entry in agents.defaults.models that wasn't in the curated TOP_MODELS list on every plugin load/gateway restart
  • Users who manually added valid catalog models (e.g. blockrun/openai/gpt-5.4) would see them silently pruned, causing "Model not allowed" errors
  • Switched to additive-only behaviour: TOP_MODELS entries are added if missing, but no user-defined entries are ever deleted

Fixes #82.

Test plan

  • Add a non-TOP_MODELS blockrun/* entry to agents.defaults.models in ~/.openclaw/openclaw.json
  • Restart the gateway (openclaw gateway restart)
  • Verify the entry is still present (openclaw models status --json)
  • Verify the model is callable without "Model not allowed" error
  • All existing tests pass (npm test)

Summary by CodeRabbit

  • Bug Fixes
    • User-defined configuration entries are now preserved while ensuring default models remain active.

injectModelsConfig() was destructively pruning any blockrun/* key in
agents.defaults.models that wasn't in TOP_MODELS, silently deleting
entries users had manually added (e.g. blockrun/openai/gpt-5.4).

Switch to additive-only behaviour: only add missing TOP_MODELS entries,
never remove user-defined keys. Fixes #82.
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 8, 2026

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

The changes replace the plugin's destructive allowlist normalization logic with an additive approach that preserves user-defined blockrun entries while ensuring TOP_MODELS are present. Additionally, test integration setup shifts from static to dynamic worker-scoped port allocation using a computed port value.

Changes

Cohort / File(s) Summary
Allowlist normalization fix
src/index.ts
Replaced destructive cleanup loop that deleted blockrun entries outside TOP_MODELS with an additive merge strategy. User-defined entries are now preserved; TOP_MODELS are added only if missing. Adds entry counting and summary logging when changes occur, making the behavior idempotent for non-TOP_MODELS entries.
Test infrastructure
test/integration/setup.ts
Introduced getTestPort() function that computes dynamic worker-scoped port (8401 + workerId for valid IDs, else 8402) and replaced static TEST_PORT constant usage. Updated integration test header comments to reflect worker-scoped port behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The PR includes an out-of-scope change: modifications to test/integration/setup.ts replacing static port 8402 with dynamic worker-scoped ports are unrelated to the blockrun allowlist preservation objective. Remove the test/integration/setup.ts changes or move them to a separate PR, as they are unrelated to preserving user-defined blockrun/* allowlist entries.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: preserving user-defined blockrun/* allowlist entries across restarts by switching from destructive to additive merge behavior.
Linked Issues check ✅ Passed The PR implements the core fix for issue #82 by making injectModelsConfig() additive-only: TOP_MODELS are added if missing and user-defined blockrun/* entries are never deleted, directly addressing the destructive normalization problem.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/preserve-user-allowlist-entries

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant