Skip to content

Fix doctor bundled runtime dependency ordering#69896

Closed
gumadeiras wants to merge 3 commits into
mainfrom
codex/fix-doctor-runtime-deps-order
Closed

Fix doctor bundled runtime dependency ordering#69896
gumadeiras wants to merge 3 commits into
mainfrom
codex/fix-doctor-runtime-deps-order

Conversation

@gumadeiras
Copy link
Copy Markdown
Member

Summary

  • keep configured channel doctor checks on read-only/setup channel adapters instead of full bundled channel runtimes
  • make setup/read-only doctor adapter lookup failures non-fatal so bundled runtime-dep repair can still run
  • document the doctor ordering invariant and add a changelog entry

Tests

  • pnpm test src/commands/doctor/shared/channel-doctor.test.ts
  • pnpm format:check src/commands/doctor/shared/channel-doctor.ts src/commands/doctor/shared/channel-doctor.test.ts docs/gateway/doctor.md
  • pnpm check:changed
  • scripts/committer 'fix doctor channel runtime dep ordering' ... reran staged pnpm check:changed --staged

Copilot AI review requested due to automatic review settings April 22, 2026 00:45
@aisle-research-bot
Copy link
Copy Markdown

aisle-research-bot Bot commented Apr 22, 2026

🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

# Severity Title
1 🔵 Low Channel doctor silently skips configured plugins when plugin discovery throws (broad exception swallowing)
1. 🔵 Channel doctor silently skips configured plugins when plugin discovery throws (broad exception swallowing)
Property Value
Severity Low
CWE CWE-391
Location src/commands/doctor/shared/channel-doctor.ts:33-72

Description

Several safe* helper functions in channel-doctor.ts catch all exceptions from plugin discovery/loading and then return undefined/[]. This can cause the doctor to run with an incomplete set of channel doctor adapters without informing the user.

Impact:

  • If a configured channel plugin (loaded, bundled setup, or read-only resolution) throws during resolution (e.g., missing dependency, module init error, or intentionally throwing), the doctor will silently omit that plugin.
  • As a result, important maintenance/security-relevant checks (compatibility normalization, stale-config cleanup, warnings) for that channel may not execute, leaving potentially unsafe or outdated configuration in place without any warning.

Vulnerable code:

function safeGetLoadedChannelPlugin(id: string) {
  try {
    return getLoadedChannelPlugin(id);
  } catch {
    return undefined;
  }
}

function safeListReadOnlyChannelPlugins(cfg: OpenClawConfig) {
  try {
    return resolveReadOnlyChannelPluginsForConfig(cfg, {
      includePersistedAuthState: false,
    }).plugins;
  } catch {
    return [];
  }
}

Recommendation

Do not silently ignore plugin discovery failures during doctor runs.

Options (in increasing strictness):

  1. Log and surface a warning that doctor coverage is incomplete for specific channel IDs when an exception occurs.
  2. Collect errors and return them as part of the doctor result so the CLI can show actionable messages.
  3. For doctor --fix/repair modes, fail closed (abort) if configured channel plugins cannot be resolved.

Example (log + propagate warning):

function safeGetLoadedChannelPlugin(id: string, warnings: string[]) {
  try {
    return getLoadedChannelPlugin(id);
  } catch (err) {
    warnings.push(`Failed to load channel plugin '${id}' for doctor: ${String(err)}`);
    return undefined;
  }
}

Also consider catching only expected error types and rethrowing unknown ones.


Analyzed PR: #69896 at commit 086d58a

Last updated on: 2026-04-22T01:22:53Z

@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation gateway Gateway runtime commands Command implementations size: S maintainer Maintainer-authored PR labels Apr 22, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 22, 2026

Greptile Summary

This PR fixes the ordering of doctor checks so that configured channel doctor logic runs against lightweight read-only/setup adapters rather than full bundled channel runtimes, allowing bundled runtime-dep repair to execute before any full runtime import is attempted. The fallback chain (resolveReadOnlyChannelPluginsForConfiggetLoadedChannelPlugingetBundledChannelSetupPlugin) is properly wrapped in safe helpers so lookup failures are non-fatal, and the new test case covers the error path.

Confidence Score: 5/5

Safe to merge — the ordering fix is well-scoped, correctly tested, and all call sites are updated consistently.

All findings are P2 or lower. The logic correctly threads the new read-only-first path through every caller of listChannelDoctorEntries that has access to a config, and the two allowlist helpers that lack a config still avoid full-runtime loading via the setup-plugin fallback.

No files require special attention.

Reviews (1): Last reviewed commit: "document doctor runtime dep fix" | Re-trigger Greptile

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the doctor channel adapter selection to avoid importing full bundled channel runtimes before bundled dependency repair can run, while documenting the ordering invariant.

Changes:

  • Switch configured-channel doctor checks to prefer read-only/setup channel adapters (and make adapter lookup failures non-fatal).
  • Update channel-doctor compatibility mutation tests to cover the new adapter resolution behavior and failure mode.
  • Document the invariant in the doctor docs and add a changelog entry.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/commands/doctor/shared/channel-doctor.ts Uses read-only/setup adapters for configured channel doctor checks and makes adapter lookup failures non-fatal.
src/commands/doctor/shared/channel-doctor.test.ts Adjusts mocks and adds coverage for read-only adapter usage and non-fatal lookup failures.
docs/gateway/doctor.md Documents the doctor ordering invariant (setup/read-only adapters before bundled runtime dependency checks).
CHANGELOG.md Adds an entry describing the doctor/plugins ordering fix.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 66bca9d060

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/commands/doctor/shared/channel-doctor.ts Outdated
@gumadeiras gumadeiras self-assigned this Apr 22, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 086d58aff3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +99 to +100
const bundledSetupPlugin = safeGetBundledChannelSetupPlugin(id);
return bundledSetupPlugin ? [bundledSetupPlugin] : [];
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Fall back to runtime doctor when setup adapter has no doctor

The selected-channel fallback now stops at getBundledChannelSetupPlugin(id), which means channels whose setup plugin omits doctor silently lose all doctor hooks when they are not already loaded. This is observable for bundled channels like extensions/bluebubbles/src/channel.setup.ts and extensions/mattermost/src/channel.setup.ts (no doctor) while their runtime plugins still provide doctor adapters (extensions/bluebubbles/src/channel.ts:100, extensions/mattermost/src/channel.ts:288). In fresh packaged installs (where runtime loading may fail before dependency repair), channel compatibility and warning/repair logic for those channels is skipped instead of running.

Useful? React with 👍 / 👎.

@gumadeiras gumadeiras closed this Apr 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commands Command implementations docs Improvements or additions to documentation gateway Gateway runtime maintainer Maintainer-authored PR size: M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants