Greptile Summary

This PR introduces a sandbox bin-probing layer so that skill eligibility checks for requires.bins, requires.anyBins, and os are evaluated against the Docker sandbox container/image rather than the gateway host. The new skills-sandbox.ts module mirrors the existing remote-node pattern: it probes the configured sandbox image for available binaries, caches the result, and exposes it via getSandboxSkillEligibility(). All call sites that previously only threaded remote eligibility now also pass sandbox eligibility.

Key findings:

• Logic bug in getSandboxSkillEligibility() — the guard !cachedBins || cachedBins.size === 0 returns undefined when the cache has been populated but is empty (either because no skills declare any requires.bins, or because all probes found nothing). This discards the sandbox's "linux" platform entirely, so a skill specifying only os: ["linux"] with no binary requirements is still excluded on a macOS/Windows gateway despite a Linux sandbox being configured. The guard should be cachedBins === null ("not yet probed") rather than also gating on size.
• containerName fast-path is dead code — probeSandboxBins accepts an optional containerName to attempt a cheaper docker exec against a running container, but refreshSandboxBinsCache never resolves or passes the container name. Every probe therefore spins up a throwaway container via docker run --rm. The parameter should either be wired in or removed.
• The evaluateRuntimeRequires / evaluateRuntimeEligibility changes in config-eval.ts are clean and well-tested.
• Note: per a previous review thread, refreshSandboxBinsCache is exported but is not yet called from gateway startup or on skills change, meaning the cache is never populated in production. That wiring is a prerequisite for the entire feature to function.

Confidence Score: 2/5

• Not safe to merge — the sandbox platform is silently dropped for OS-only skill constraints, and the cache is never populated at runtime.
• Two correctness issues need to be resolved before the feature works as intended: the cachedBins.size === 0 guard in getSandboxSkillEligibility() defeats OS-only eligibility for the sandbox, and (per a prior thread) refreshSandboxBinsCache is never invoked so the cache stays null in production. The underlying eval logic and tests are solid, but the feature is effectively a no-op until both issues are addressed.
• src/infra/skills-sandbox.ts requires the most attention — specifically the getSandboxSkillEligibility return guard and the missing refreshSandboxBinsCache call-site wiring.

_{Last reviewed commit: 4a0b36b}

Thank you @chatgpt-codex-connector and @greptile-apps for the thorough multi-round reviews. Here is a summary of all fixes incorporated based on your feedback, as of the latest commit f441706:

Fixes from review feedback

1. Cache refresh wired into lifecycle — refreshSandboxBinsCache is now called at gateway startup, on skills/config file changes, and on config hot-reload (server.impl.ts). This was the critical missing piece that prevented the entire feature from working.
2. Correct null vs empty-set sentinel — getSandboxSkillEligibility() now uses cachedBins === null (not probed yet) instead of cachedBins.size === 0 (probed, nothing found), so OS-only skills like os: ["linux"] are correctly included when the sandbox is running.
3. Session-aware eligibility injection — All call sites now gate sandbox eligibility behind sandboxRuntime.sandboxed, preventing sandbox-only skills from being advertised in non-sandboxed sessions (e.g., main session under mode: "non-main").
4. Container image verification — resolveRunningContainerName now accepts an expectedImage parameter and skips containers whose image does not match, preventing probing of stale or unrelated containers.
5. Concurrency guard — Added a refreshGeneration counter to prevent race conditions where a slow, stale refresh overwrites a newer result.
6. Correct session key for sandbox status — Fixed commands-system-prompt.ts to use params.sessionKey (target session) instead of params.ctx.SessionKey (source session) when resolving sandbox runtime status.
7. CI test fixes — Added mocks for agents/sandbox.js and infra/skills-sandbox.js in run.skill-filter.test.ts to resolve all 10 test failures.

Known limitations (deferred to follow-up)

• Per-agent sandbox image overrides share a single global cache.
• openclaw agent --local CLI path does not trigger cache refresh.
• Sandbox container creation via docker.setupCommand does not trigger a cache refresh.

I've also updated the PR description to reflect the full scope of changes (18 files) and the robustness improvements made during review.

To use Codex here, create a Codex account and connect to github.

Your summary is spot-on. I've verified all the key fixes in commit f441706:

✓ Core functionality enabled - refreshSandboxBinsCache is correctly called on:

• Startup: server.impl.ts:644
• Skills/config changes: server.impl.ts:651-664 (debounced)
• Hot-reload: server.impl.ts:872

✓ Correct eligibility logic - The cachedBins === null check in getSandboxSkillEligibility() properly distinguishes "not probed" from "probed but empty", ensuring OS-only skills work correctly.

✓ Session-aware eligibility - Both skill-commands.ts:33 and commands-system-prompt.ts:32 correctly gate sandbox eligibility with sandboxRuntime.sandboxed.

✓ Robust container matching - resolveRunningContainerName() at skills-sandbox.ts:146-149 verifies the container image matches before probing.

✓ Concurrency safety - The generation counter pattern prevents stale refreshes from overwriting newer results.

✓ Correct session context - commands-system-prompt.ts:28-31 uses the correct params.sessionKey.

✓ CI test mocks - Proper stubs added for skills-sandbox.js, skills-remote.js, and sandbox.js.

This is solid work. The feature is now properly gated, session-aware, and resilient to edge cases.

Great work on the sandbox eligibility probing — this is a much-needed improvement for sandbox-enabled deployments.

I wanted to flag a related but orthogonal issue in the existing parseBinProbePayload() function that affects non-sandbox remote nodes (e.g. Docker gateway with a Mac host connected via openclaw node run).

The parsing bug: system.which returns a Record<string, string> ({ "git": "/usr/bin/git", ... }), but parseBinProbePayload only handles string[] and raw stdout formats. Array.isArray() evaluates to false for plain objects, so the Record-typed response is never parsed — the function returns [], and all remote-bin-dependent skills are incorrectly gated as blocked.

I have a fix in #28406 that adds Object.keys() extraction for the Record format, along with unit tests covering all three response shapes. The change is minimal and scoped entirely to the parsing layer — it should not conflict with the sandbox probing logic introduced here.

Since this parsing bug exists independently of sandbox configuration, would it make sense to land #28406 separately, or would you prefer to incorporate the Record-format handling into this PR?

Thanks for the PR! Multiple PRs address issue #29254. Keeping #29303 as the earliest submission. Closing to reduce noise. This is an AI-assisted triage review. If we got this wrong, feel free to reopen — happy to revisit.