Skills: enabled: true overrides requires.bins check#32789
Skills: enabled: true overrides requires.bins check#32789haosenwang1018 wants to merge 4 commits intoopenclaw:mainfrom
Conversation
… instead of cancelling
Greptile SummaryThis PR bundles three independent fixes: (1) the headline fix making Key findings:
Confidence Score: 3/5
Last reviewed commit: d4ce4fa |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (skillConfig?.enabled === true) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
enabled: true silently bypasses isBundledSkillAllowed
The new early return at line 83–85 is placed before the isBundledSkillAllowed check (line 86), so setting enabled: true on a bundled skill now unconditionally includes it even when skills.allowBundled is configured to exclude that skill. The PR description says "the bundled-allowlist check … remain untouched," but it is now skippable for any explicitly-enabled skill.
If this is intentional (enabled: true means "force-include regardless of every guard"), it should be documented and tested explicitly. For example, a test where entry.skill.source === "openclaw-bundled", allowBundled does not list the skill, and enabled: true still returns true would make the invariant explicit. As written, the bundled-allowlist can be silently overridden with no test coverage for that path.
If the intent is narrower—only bypass runtime binary/env checks—then the early return should be moved to after the isBundledSkillAllowed guard:
| if (skillConfig?.enabled === true) { | |
| return true; | |
| } | |
| if (skillConfig?.enabled === false) { | |
| return false; | |
| } | |
| if (!isBundledSkillAllowed(entry, allowBundled)) { | |
| return false; | |
| } | |
| if (skillConfig?.enabled === true) { | |
| return true; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/skills/config.ts
Line: 83-85
Comment:
**`enabled: true` silently bypasses `isBundledSkillAllowed`**
The new early return at line 83–85 is placed *before* the `isBundledSkillAllowed` check (line 86), so setting `enabled: true` on a bundled skill now unconditionally includes it even when `skills.allowBundled` is configured to exclude that skill. The PR description says "the bundled-allowlist check … remain untouched," but it is now skippable for any explicitly-enabled skill.
If this is intentional (`enabled: true` means "force-include regardless of every guard"), it should be documented and tested explicitly. For example, a test where `entry.skill.source === "openclaw-bundled"`, `allowBundled` does **not** list the skill, and `enabled: true` still returns `true` would make the invariant explicit. As written, the bundled-allowlist can be silently overridden with no test coverage for that path.
If the intent is narrower—only bypass runtime binary/env checks—then the early return should be moved to *after* the `isBundledSkillAllowed` guard:
```suggestion
if (skillConfig?.enabled === false) {
return false;
}
if (!isBundledSkillAllowed(entry, allowBundled)) {
return false;
}
if (skillConfig?.enabled === true) {
return true;
}
```
How can I resolve this? If you propose a fix, please make it concise.| }): string | undefined { | ||
| const context = params.toolContext; | ||
| if (!context?.currentThreadTs || !context.currentChannelId) { | ||
| return undefined; |
There was a problem hiding this comment.
Case-insensitive comparison of case-sensitive Matrix room IDs
Matrix room IDs have the form !localpart:server. The server name is a DNS hostname (case-insensitive), but the localpart is an opaque, server-generated string that is case-sensitive per the Matrix specification. Applying .toLowerCase() to the full room ID (including the localpart) can cause !RoomA:example.com and !rooma:example.com to match even though they are different rooms.
Consider only lower-casing the server portion (after the last :) while preserving the localpart's original case for comparison:
function normalizeMatrixRoomId(raw: string): string {
const stripped = stripMatrixRoomPrefix(raw.trim());
const colonIdx = stripped.lastIndexOf(":");
if (colonIdx === -1) return stripped;
return stripped.slice(0, colonIdx) + stripped.slice(colonIdx).toLowerCase();
}Then replace the current comparison:
if (normalizeMatrixRoomId(params.to) !== normalizeMatrixRoomId(context.currentChannelId)) {
return undefined;
}The test "matches case-insensitively" passes !ABC123:Matrix.Org vs !abc123:matrix.org—this passes today but would mask a real mismatch if those were actually two distinct rooms.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/infra/outbound/message-action-params.ts
Line: 100-103
Comment:
**Case-insensitive comparison of case-sensitive Matrix room IDs**
Matrix room IDs have the form `!localpart:server`. The server name is a DNS hostname (case-insensitive), but the `localpart` is an opaque, server-generated string that is **case-sensitive** per the Matrix specification. Applying `.toLowerCase()` to the full room ID (including the localpart) can cause `!RoomA:example.com` and `!rooma:example.com` to match even though they are different rooms.
Consider only lower-casing the server portion (after the last `:`) while preserving the localpart's original case for comparison:
```typescript
function normalizeMatrixRoomId(raw: string): string {
const stripped = stripMatrixRoomPrefix(raw.trim());
const colonIdx = stripped.lastIndexOf(":");
if (colonIdx === -1) return stripped;
return stripped.slice(0, colonIdx) + stripped.slice(colonIdx).toLowerCase();
}
```
Then replace the current comparison:
```typescript
if (normalizeMatrixRoomId(params.to) !== normalizeMatrixRoomId(context.currentChannelId)) {
return undefined;
}
```
The test `"matches case-insensitively"` passes `!ABC123:Matrix.Org` vs `!abc123:matrix.org`—this passes today but would mask a real mismatch if those were actually two distinct rooms.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4ce4fa7fa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (skillConfig?.enabled === true) { | ||
| return true; |
There was a problem hiding this comment.
Returning early on enabled === true now bypasses isBundledSkillAllowed, so a bundled skill blocked by skills.allowBundled becomes eligible as soon as skills.entries.<key>.enabled: true is set. That contradicts the documented allowlist contract and changes behavior beyond the commit’s stated scope (runtime requirements only), so users relying on allowBundled as a hard gate can unintentionally expose blocked bundled skills.
Useful? React with 👍 / 👎.
| ((ctx: ChannelOutboundContext) => { | ||
| log.warn("sendMedia not implemented; degrading to sendText", { | ||
| channel: params.channel, | ||
| }); | ||
| return sendText({ ...ctx, mediaUrl: undefined }); |
There was a problem hiding this comment.
This fallback converts every sendMedia call into sendText, but delivery still invokes sendMedia once per media URL and uses an empty caption after the first attachment. For payloads with multiple media URLs (or media-only payloads), that produces extra blank text messages (or channel errors if empty text is rejected), so the degradation path can spam/fail instead of cleanly downgrading media delivery.
Useful? React with 👍 / 👎.
1 participant
Summary
skills.entries.<key>.enabled: truedoes not force-include a skill. It falls through toevaluateRuntimeEligibility(), which rejects the skill whenrequires.binslists binaries not found on the system.enabled: trueto be the symmetric counterpart ofenabled: false— an unconditional override that forces the skill on regardless of runtime checks.enabled === trueinshouldIncludeSkill(), symmetric with the existingenabled === falsepath, so the skill is included without evaluating runtime requirements.enabled === falsepath, the bundled-allowlist check, and allevaluateRuntimeEligibilitylogic remain untouched.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
Setting
skills.entries.<key>.enabled: truein config now unconditionally includes the skill, bypassingrequires.bins,requires.env, and other runtime eligibility checks.Security Impact (required)
NoNoNoNoNoRepro + Verification
Steps
enabled: trueand arequires.binsdependency that is not installed.Expected
enabled: true, regardless ofrequires.bins.Actual
evaluateRuntimeEligibilityrejected it.Evidence
src/agents/skills/config.test.tscovers:enabled: false→ excludedenabled: truewith unmetrequires.bins→ included (the bug scenario)enabled: truewithout requires → includedenabledset + unmet bins → excluded (falls through to runtime check)Human Verification (required)
src/agents/skills/pass.enabled: undefined(no config entry) still falls through to runtime eligibility.Compatibility / Migration
YesNoNoFailure Recovery (if this breaks)
shouldIncludeSkill().src/agents/skills/config.tsrequires.binsbeing incorrectly included when no explicitenabledis set.Risks and Mitigations
enabled: trueas a no-op annotation may now get a skill that was previously filtered out by runtime checks.enabled: true; users who set it explicitly want the skill included.