Skip to content

fix: respect enabled: true as force-enable in shouldIncludeSkill#50978

Open
carrotRakko wants to merge 2 commits intoopenclaw:mainfrom
delight-co:fix/skills-enabled-override
Open

fix: respect enabled: true as force-enable in shouldIncludeSkill#50978
carrotRakko wants to merge 2 commits intoopenclaw:mainfrom
delight-co:fix/skills-enabled-override

Conversation

@carrotRakko
Copy link
Copy Markdown
Contributor

@carrotRakko carrotRakko commented Mar 20, 2026

Summary

  • enabled: false force-disables a skill, but enabled: true does not force-enable — it still checks requires.bins, OS restrictions, etc.
  • This creates an asymmetry: users cannot override runtime eligibility checks to force-include a skill they know works in their environment.
  • Fix: when enabled: true, skip evaluateRuntimeEligibility and return true. The isBundledSkillAllowed check still runs first, so allowBundled restrictions are respected.

Change Type (select all)

  • Bug fix
  • Feature
  • Refactor
  • Docs
  • Security hardening
  • Chore/infra

Scope (select all touched areas)

  • Gateway / orchestration
  • Skills / tool execution
  • Auth / tokens
  • Memory / storage
  • Integrations
  • API / contracts
  • UI / DX
  • CI/CD / infra

Linked Issue/PR

User-visible / Behavior Changes

Setting skills.entries.<key>.enabled: true in config now guarantees the skill is included, regardless of whether required binaries are installed or OS restrictions would normally exclude it.

Security Impact (required)

  • New permissions/capabilities? No
  • Secrets/tokens handling changed? No
  • New/changed network calls? No
  • Command/tool execution surface changed? No (skill was already available, just gated by runtime check)
  • Data access scope changed? No

Repro + Verification

Steps

  1. Configure a skill that requires a binary not present on the system (e.g., requires: { bins: ["nonexistent"] })
  2. Set skills.entries.<skill-name>.enabled: true in config
  3. Check if the skill is included

Expected

  • Skill is included (force-enabled).

Actual

  • Before: Skill is excluded because evaluateRuntimeEligibility fails on the missing binary.
  • After: Skill is included because enabled: true bypasses runtime eligibility.

Evidence

  • Failing test/log before + passing after

New test file src/agents/skills/config.test.ts with 4 test cases covering force-disable, force-enable, fallthrough, and allowBundled interaction.

Human Verification (required)

  • Verified scenarios: Tested on a fork with a skill requiring a missing binary. enabled: true correctly force-enables, enabled: false correctly force-disables.
  • Edge cases checked: allowBundled restrictions are still respected even when enabled: true.
  • What you did not verify: Not every skill entry combination, but the logic is straightforward (3-line change with early return).

Compatibility / Migration

  • Backward compatible? Yes — enabled: undefined (default) behavior is unchanged.
  • Config/env changes? No new config keys.
  • Migration needed? No.

Failure Recovery (if this breaks)

  • How to disable/revert: Revert the 3-line change in config.ts.
  • Known bad symptoms: A skill that should be excluded by runtime checks appearing in the agent's skill list.

Risks and Mitigations

  • Risk: User force-enables a skill whose required binary is truly missing, leading to runtime errors.
    • Mitigation: This is an explicit opt-in (enabled: true). The user is taking responsibility for ensuring the skill works.

✍️ Author: Claude Code with @carrotRakko (AI-written, human-approved)

@carrotRakko
fix: respect enabled: true as force-enable in shouldIncludeSkill
9f7b118 
When a skill has `enabled: true` in config, bypass `evaluateRuntimeEligibility`
(which checks requires.bins, OS, etc.) so the skill is always included.
This makes `enabled: true` symmetric with `enabled: false` (force-disable).

The check runs after `isBundledSkillAllowed` so allowBundled restrictions
are still respected.

✍️ Author: Claude Code with @carrotRakko (AI-written, human-approved)
@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: S labels Mar 20, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 20, 2026

Greptile Summary

This PR fixes an asymmetry in shouldIncludeSkill: previously enabled: false would force-disable a skill while enabled: true had no special effect and still fell through to evaluateRuntimeEligibility (which could reject the skill due to missing binaries or OS restrictions). The fix adds a 3-line early-return for enabled === true, placed deliberately after the isBundledSkillAllowed check so that allowBundled restrictions are still respected.

Key observations:

  • The logic change is minimal and correct; the check ordering (enabled: false → isBundledSkillAllowed → enabled: true → runtime eligibility) accurately captures the intended precedence rules.
  • The new test file covers all four meaningful branches (force-disable, force-enable, default fallthrough, and allowBundled interaction) with a clean mock and factory helper.
  • The deliberate asymmetry between where enabled: false and enabled: true sit relative to the allowBundled guard is non-obvious and should carry a brief inline comment per the repo's own "add comments for tricky logic" guideline (AGENTS.md/CLAUDE.md).

Confidence Score: 5/5

  • This PR is safe to merge — the change is minimal, well-tested, backward-compatible, and carries no security surface changes.
  • The code change is 3 lines with a clear and correct early-return pattern, fully covered by new tests. enabled: undefined behavior is unchanged, allowBundled restrictions are still respected, and no auth/network/execution surfaces are affected.
  • No files require special attention beyond the optional inline comment suggestion on src/agents/skills/config.ts.
Prompt To Fix All With AI 
This is a comment left during a code review.
Path: src/agents/skills/config.ts
Line: 87-89

Comment:
**Missing comment for non-obvious check ordering**

The repo style guide (`AGENTS.md`/`CLAUDE.md`) explicitly calls out: _"Add brief code comments for tricky or non-obvious logic."_

The placement of the `enabled === true` early-return **after** `isBundledSkillAllowed` (while `enabled === false` sits **before** it) is intentionally asymmetric — `allowBundled` should win even over an explicit force-enable. That ordering is easy to miss or accidentally "fix" in a future refactor without understanding the intent. A brief comment would make it self-documenting:

```suggestion
  // enabled: true force-enables, but allowBundled still gates bundled skills
  // (checked above). enabled: false short-circuits before that check since
  // the result is the same regardless.
  if (skillConfig?.enabled === true) {
    return true;
  }
```

**Context Used:** CLAUDE.md ([source](https://app.greptile.com/review/custom-context?memory=fd949e91-5c3a-4ab5-90a1-cbe184fd6ce8))

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: "fix: respect enabled..."

greptile-apps[bot]
greptile-apps Bot reviewed Mar 20, 2026
View reviewed changes
Comment thread src/agents/skills/config.ts
chatgpt-codex-connector[bot]
chatgpt-codex-connector Bot reviewed Mar 20, 2026
View reviewed changes
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: 9f7b118b74

ℹ️ 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/agents/skills/config.ts
@carrotRakko
fix: add comment for check ordering + sync skills-status eligibility
12255f8 
- Add comment explaining why enabled: true is checked after
  isBundledSkillAllowed (Greptile review feedback).
- Sync skills-status.ts eligibility with force-enable: when
  enabled: true, mark the skill as eligible in status reporting
  so CLI output matches runtime behavior (Codex review feedback).

✍️ Author: Claude Code with @carrotRakko (AI-written, human-approved)
@BingqingLyu BingqingLyu mentioned this pull request Apr 27, 2026
Open
15 tasks
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented Apr 28, 2026 •
edited
Loading

Codex review: needs changes before merge.

Summary
The PR changes skill inclusion and skill status logic so skills.entries.<key>.enabled: true force-includes a skill despite runtime eligibility failures while preserving skills.allowBundled gating.

Reproducibility: yes. Source inspection on current main shows enabled === true falls through to evaluateRuntimeEligibility, and evaluateRuntimeRequires rejects missing required binaries, matching the linked missing-host-binary reproduction.

Next step before merge
A repair worker can safely rebase or replace the contributor branch and add the missing docs/changelog coverage without a product or security decision first.

Security
Cleared: The diff is limited to explicit skill inclusion/status behavior and tests, with no workflow, dependency, lockfile, secret-handling, publishing, or artifact execution changes.

Review findings

  • [P2] Document force-enable skill semantics — src/agents/skills/config.ts:89-94
  • [P3] Add the required changelog entry — src/agents/skills/config.ts:89-94
Review details

Best possible solution:

Land this PR or an equivalent narrow replacement after rebasing, documenting the force-enable semantics, adding the required changelog entry, preserving skills.allowBundled precedence, and rerunning focused skills validation plus the changed gate.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection on current main shows enabled === true falls through to evaluateRuntimeEligibility, and evaluateRuntimeRequires rejects missing required binaries, matching the linked missing-host-binary reproduction.

Is this the best way to solve the issue?

No, not as submitted. The code direction is narrow and includes status coverage, but the PR changes a public skills config contract without updating user docs or the required changelog entry.

Full review comments:

  • [P2] Document force-enable skill semantics — src/agents/skills/config.ts:89-94
    This changes skills.entries.<key>.enabled: true into an override that bypasses runtime gates such as bins and OS checks, but the public skills docs still describe enabled only as a disable switch and still say requires.bins is checked on the host at load time. Please update the skills config/reference docs so operators understand the override and that skills.allowBundled still wins.
    Confidence: 0.87
  • [P3] Add the required changelog entry — src/agents/skills/config.ts:89-94
    This is a user-facing skills behavior fix, but the branch does not update CHANGELOG.md. Repo policy requires user-facing fixes to add a single-line entry under the active changelog section with human contributor credit, so please add one before merge.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.86

Acceptance criteria:

  • pnpm test src/agents/skills/config.test.ts src/agents/skills.buildworkspaceskillstatus.test.ts
  • pnpm exec oxfmt --check --threads=1 src/agents/skills/config.ts src/agents/skills-status.ts src/agents/skills/config.test.ts docs/tools/skills.md docs/tools/skills-config.md docs/gateway/configuration-reference.md CHANGELOG.md
  • pnpm check:changed in Testbox after rebase or replacement

What I checked:

Likely related people:

  • Vincent Koc: Local blame for the central shouldIncludeSkill and skill-status code in this shallow checkout attributes the current snapshot to Vincent Koc, and the changelog credits @vincentkoc on several recent skills/config/runtime fixes. (role: recent maintainer; confidence: medium; commits: ea75cd897182; files: src/agents/skills/config.ts, src/agents/skills-status.ts, CHANGELOG.md)
  • @mbelinky: The changelog credits @mbelinky for recent CLI/skills status behavior work, which is adjacent to this PR's buildWorkspaceSkillStatus surface. (role: adjacent skills/status contributor; confidence: low; files: src/agents/skills-status.ts, src/cli/skills-cli.format.ts, CHANGELOG.md)

Remaining risk / open question:

  • The source branch likely needs a rebase or replacement branch because the provided PR metadata shows an older base and earlier ClawSweeper context reported a dirty merge state.
  • The local checkout is partial/shallow, so older feature-introduction provenance is weaker than the current source and changelog evidence.

Codex review notes: model gpt-5.5, reasoning high; reviewed against ea75cd897182.

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

Reviewers

@greptile-apps greptile-apps[bot] greptile-apps[bot] left review comments

@chatgpt-codex-connector chatgpt-codex-connector[bot] chatgpt-codex-connector[bot] left review comments

Assignees

No one assigned

Labels

agents Agent runtime and tooling size: S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: skills.entries.<key>.enabled: true does not override requires.bins check

1 participant

@carrotRakko