fix(skills): keep blank env requirements unsatisfied by wahaha1223 · Pull Request #108848 · openclaw/openclaw

wahaha1223 · 2026-07-16T09:33:48Z

What Problem This Solves

Fixes an issue where users with whitespace-only skill credential environment values could have those skills loaded and reported as eligible even though no usable credential was available.

Why This Change Was Made

Skill loading and skill status now share one non-blank requirement check for host environment values, per-skill environment overrides, and primary API keys. The change preserves configured SecretRef eligibility and does not alter always-on, bundled allowlist, remote platform, or binary requirement behavior.

User Impact

Skills that require credentials are no longer exposed to agents or reported as ready when their configured value contains only whitespace. Valid credentials continue to work as before.

Evidence

AI-assisted.

Real production-path proof was run on a real Linux x86_64 host (Linux 6.8.0-134-generic, Node v24.18.0) by directly invoking the exported production loading and status functions. No mocks or network substitutes were used.

Exact harness command (run once in a detached latest-main worktree and once on this branch):

node --import tsx --input-type=module - <<'EOF'
import { shouldIncludeSkill } from "./src/skills/loading/config.ts";
import { buildWorkspaceSkillStatus } from "./src/skills/discovery/status.ts";
const key = "HUNT_BLANK_ENV";
const entry = {
  skill: {
    name: "proof-skill", description: "proof",
    filePath: "/tmp/proof-skill/SKILL.md", baseDir: "/tmp/proof-skill",
    source: "test",
    sourceInfo: { path: "/tmp/proof-skill/SKILL.md", source: "test", scope: "temporary", origin: "top-level" },
    disableModelInvocation: false,
  },
  frontmatter: {},
  metadata: { primaryEnv: key, requires: { env: [key] } },
};
for (const [label, value] of [["missing", undefined], ["blank", "   "], ["valid", " token "]]) {
  if (value === undefined) delete process.env[key]; else process.env[key] = value;
  const included = shouldIncludeSkill({ entry, bundledAllowlist: undefined });
  const status = buildWorkspaceSkillStatus("/tmp/proof-workspace", { entries: [entry] }).skills[0];
  console.log(JSON.stringify({ label, included, eligible: status?.eligible, missingEnv: status?.missing.env }));
}
delete process.env[key];
EOF

Before, on latest upstream/main 57ae43a89:

{"label":"missing","included":false,"eligible":false,"missingEnv":["HUNT_BLANK_ENV"]}
{"label":"blank","included":true,"eligible":true,"missingEnv":[]}
{"label":"valid","included":true,"eligible":true,"missingEnv":[]}

Premise: the shared normalization contract turns whitespace-only strings into no value while trimming valid padded strings:

node --import tsx --input-type=module -e 'import { normalizeOptionalString } from "@openclaw/normalization-core/string-coerce"; console.log(JSON.stringify({ blank: normalizeOptionalString("   "), padded: normalizeOptionalString(" token ") }));'

{"padded":"token"}

(blank is omitted by JSON.stringify because it is undefined.)

After, on this branch at bc66063d5:

{"label":"missing","included":false,"eligible":false,"missingEnv":["HUNT_BLANK_ENV"]}
{"label":"blank","included":false,"eligible":false,"missingEnv":["HUNT_BLANK_ENV"]}
{"label":"valid","included":true,"eligible":true,"missingEnv":[]}

The missing and valid cases are negative controls: missing remains blocked, while a valid padded value remains eligible. Focused regression coverage also exercises blank and valid per-skill env values, blank and valid primary API keys, status reporting, always-on behavior, and the existing SecretRef path.

node scripts/run-vitest.mjs src/skills/loading/skills.test.ts src/skills/discovery/status.test.ts

Test Files  2 passed (2)
Tests       40 passed (40)

Final structured review:

autoreview clean: no accepted/actionable findings reported
overall: patch is correct (0.96)

Not tested: no external skill execution was attempted because this change is entirely in pre-execution eligibility and status projection; the proof drives both exported production decision paths directly.

clawsweeper · 2026-07-16T11:03:05Z

Codex review: needs maintainer review before merge. Reviewed July 16, 2026, 6:02 AM ET / 10:02 UTC.

Summary
The branch adds a shared non-blank skill environment-requirement check and regression coverage for loading and status eligibility.

PR surface: Source +17, Tests +80. Total +97 across 4 files.

Reproducibility: yes. with high confidence from source and supplied live proof: the exact production loading and status paths accept a whitespace-only value on the baseline, and current main retains that same truthiness logic.

Review metrics: none identified.

Stored data model
Persistent data-model change detected: vector/embedding metadata: src/skills/discovery/status.test.ts, vector/embedding metadata: src/skills/discovery/status.ts, vector/embedding metadata: src/skills/loading/config.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

[P1] The branch is behind current main, so maintainers should refresh the merge-base review and required checks on the final head before landing.

Maintainer options:

Decide the mitigation before merge
Land one shared non-blank credential-presence predicate used by both skill loading and status, retaining SecretRef eligibility and the existing always-on and platform semantics.
Pause or close
Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

No automated repair is needed; this correct, proven patch needs ordinary final-head maintainer review and merge gating.

Security
Cleared: The patch adds no dependency, workflow, permission, execution, or secret-disclosure surface and instead prevents unusable blank credential values from passing eligibility checks.

Review details

Best possible solution:

Land one shared non-blank credential-presence predicate used by both skill loading and status, retaining SecretRef eligibility and the existing always-on and platform semantics.

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

Yes, with high confidence from source and supplied live proof: the exact production loading and status paths accept a whitespace-only value on the baseline, and current main retains that same truthiness logic.

Is this the best way to solve the issue?

Yes. Centralizing the non-blank presence rule at the shared loading-config boundary is narrower and safer than independently patching loading and status, and the tests cover the important sibling cases.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 8f1760b667ff.

Label changes

Label changes:

add P2: Whitespace-only credentials can incorrectly expose unusable skills, but the defect has limited blast radius and a narrow, well-proven fix.
add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix Linux output from both exported production decision paths, with before output and missing/valid controls; no external skill execution is needed to prove this pre-execution eligibility change.
add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes after-fix Linux output from both exported production decision paths, with before output and missing/valid controls; no external skill execution is needed to prove this pre-execution eligibility change.

Label justifications:

P2: Whitespace-only credentials can incorrectly expose unusable skills, but the defect has limited blast radius and a narrow, well-proven fix.
rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes after-fix Linux output from both exported production decision paths, with before output and missing/valid controls; no external skill execution is needed to prove this pre-execution eligibility change.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix Linux output from both exported production decision paths, with before output and missing/valid controls; no external skill execution is needed to prove this pre-execution eligibility change.

Evidence reviewed

PR surface:

Source +17, Tests +80. Total +97 across 4 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	2	27	10	+17
Tests	2	80	0	+80
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	4	107	10	+97

What I checked:

Current-main bug remains: Current main uses Boolean truthiness over host environment values, per-skill env overrides, and apiKey values, so a whitespace-only string is accepted as satisfying the requirement. (src/skills/loading/config.ts:97, 8f1760b667ff)
Status path has the same defect: The status projection independently repeats the same truthiness check, allowing loading and reported eligibility to agree on the wrong result. (src/skills/discovery/status.ts:270, 8f1760b667ff)
Shared fix preserves SecretRefs: The new helper rejects blank strings while continuing to treat a configured non-string SecretRef apiKey as satisfying the primary environment requirement. (src/skills/loading/config.ts:59, bc66063d564c)
Both decision surfaces use one invariant: The loading and status paths now call the same helper, avoiding future drift between agent exposure and user-facing status. (src/skills/discovery/status.ts:270, bc66063d564c)
Regression coverage: Focused tests cover blank and padded host env values, configured env values, primary apiKey strings, SecretRefs, always-on behavior, and status missing-env reporting. (src/skills/loading/skills.test.ts:506, bc66063d564c)
Real production-path proof: The PR body provides before-and-after Linux output from the exported production loading and status functions: blank values change from eligible to ineligible while missing and valid padded values remain correct. (src/skills/loading/config.ts:59, bc66063d564c)

Likely related people:

KimGLee: Authored the merged work that unified skill runtime inclusion with available-skills exposure, directly adjacent to this eligibility invariant. (role: feature-history contributor; confidence: high; commits: 0c91bda, fd71bc0; files: src/skills/loading/config.ts, src/skills/loading/workspace.ts)
altaywtf: Merged and iterated on the prior skill-exposure policy change that established the current owner boundary for inclusion decisions. (role: merger and recent feature reviewer; confidence: high; commits: fd71bc0, a02ceb3, 2b48b3a; files: src/skills/loading/config.ts, src/skills/loading/workspace.ts)
Peter Steinberger: Early history connects this contributor to skill onboarding and status presentation, including callers of buildWorkspaceSkillStatus. (role: original status-surface contributor; confidence: medium; commits: 62112d9978, f717a13039; files: src/skills/discovery/status.ts, src/commands/onboard-skills.ts)

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

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

ClawSweeper keeps one durable marker-backed review comment per issue or PR.
Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
Maintainers can also comment @clawsweeper review to request a fresh review only.
Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

steipete · 2026-07-16T15:37:40Z

Merged via squash.

Prepared head SHA: a7b637b4c32c01a21952489d1d3dda6a29b26e97
Landed commit: d1e2fbd6823eb44d7f9b8f631d068786b36ce484

* fix(skills): keep blank env requirements unsatisfied * refactor(skills): reuse secret input presence check --------- Co-authored-by: Peter Steinberger <[email protected]>

openclaw-barnacle Bot added the size: S label Jul 16, 2026

wahaha1223 force-pushed the fix/skills-blank-env-requirements branch from fc56c20 to bc66063 Compare July 16, 2026 09:55

steipete self-assigned this Jul 16, 2026

wahaha1223 and others added 2 commits July 16, 2026 16:11

fix(skills): keep blank env requirements unsatisfied

060ac29

refactor(skills): reuse secret input presence check

a7b637b

steipete force-pushed the fix/skills-blank-env-requirements branch from bc66063 to a7b637b Compare July 16, 2026 15:26

steipete merged commit d1e2fbd into openclaw:main Jul 16, 2026
118 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(skills): keep blank env requirements unsatisfied#108848

fix(skills): keep blank env requirements unsatisfied#108848
steipete merged 2 commits into
openclaw:mainfrom
wahaha1223:fix/skills-blank-env-requirements

wahaha1223 commented Jul 16, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented Jul 16, 2026

Uh oh!

Uh oh!

steipete commented Jul 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

wahaha1223 commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What Problem This Solves

Why This Change Was Made

User Impact

Evidence

Uh oh!

clawsweeper Bot commented Jul 16, 2026

Uh oh!

Uh oh!

steipete commented Jul 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wahaha1223 commented Jul 16, 2026 •

edited

Loading