fix: add proper preflight check for Docker sandbox plugin availability

[yazan-albaiz]

1. Added spawnSync import from node:child_process
2. Added checkDockerSandbox() function that:
   • First checks if Docker is running via docker info
   • Then checks if the sandbox plugin exists via docker sandbox ls
   • Returns a fatal error with a clear message if the plugin is not found
3. Updated runPreflightChecks() to conditionally run the Docker sandbox check only when config.sandbox is true

[yazan-albaiz]

@claude review the pr.

[claude]

Claude finished @yazan-albaiz's task in 1m 4s —— View job

---

Code Review

Tasks

• Read repository guidelines (CLAUDE.md)
• Review changed files
• Analyze implementation quality
• Provide feedback

---

Review Summary

This PR adds proper preflight validation for Docker sandbox mode. The implementation is solid and follows the codebase patterns well. I found a few issues to address:

Issues Found

1. ⚠️ Unused async keyword (src/lib/preflight.ts:202)

The checkDockerSandbox() function is declared as async but doesn't use any await expressions. This should either be made synchronous or the spawnSync calls should be replaced with async alternatives.

async function checkDockerSandbox(): Promise<PreflightCheck> {
  // ... uses spawnSync (synchronous) throughout
}

Recommendation: Remove the async keyword since the function uses only synchronous operations:

function checkDockerSandbox(): Promise<PreflightCheck> {

Or alternatively, use the async exec pattern already used elsewhere in the file for consistency with other checks.

Fix this →

2. 💡 Inconsistent error handling pattern (src/lib/preflight.ts:254)

The catch block catches all errors but the function uses synchronous spawnSync calls that don't throw - they return error codes. This catch block may only catch runtime errors (like missing spawnSync import), making it less useful than intended.

} catch (error) {
  return {
    name: 'Docker Sandbox',
    passed: false,
    message: `Failed to check Docker sandbox: ${error}`,
    fatal: true,
  };
}

Recommendation: The current implementation is acceptable, but consider clarifying what errors this catches (primarily system-level failures like permission issues or missing spawn capability).

3. ✅ Edge case not handled: exit code vs stderr check

When sandboxCheck.status !== 0 on line 227, the code only checks stderr for specific error messages. However, if the sandbox plugin is installed but fails for a different reason (e.g., permission denied, network timeout), it falls through the if statement at line 230 with no return value, eventually returning success at line 248.

Current behavior: Non-zero exit + non-matching stderr = passes check (potentially incorrect)

Example scenario:

// If 'docker sandbox ls' exits with code 1 due to permissions issue
// stderr might be "permission denied" (doesn't match the checks)
// Falls through to line 248 and returns passed: true

Recommendation: Add an else case or default handling for unexpected errors:

if (sandboxCheck.status !== 0) {
  const stderr = sandboxCheck.stderr || '';

  if (
    stderr.includes('is not a docker command') ||
    stderr.includes('unknown command') ||
    stderr.includes('Unknown command')
  ) {
    return {
      name: 'Docker Sandbox',
      passed: false,
      message: 'Docker sandbox plugin not found...',
      fatal: true,
    };
  }

  // Handle other non-zero exit codes
  return {
    name: 'Docker Sandbox',
    passed: false,
    message: `Docker sandbox check failed: ${stderr.trim() || 'Unknown error'}`,
    fatal: true,
  };
}

Fix this →

Positive Observations

1. ✅ Good separation of concerns - Docker check only runs when config.sandbox is true (lines 299-301)
2. ✅ Clear error messages - The fatal error message properly explains Docker Desktop 4.50+ requirement and mentions alternatives won't work
3. ✅ Proper timeout handling - Both spawnSync calls use 10-second timeouts to prevent hanging
4. ✅ Two-stage validation - Checks Docker is running first, then validates sandbox plugin exists
5. ✅ Consistent with codebase patterns - Follows the existing PreflightCheck interface and return structure

Minor Suggestions

1. Line 220 comment: "This is better than --help" - This is good context, but docker sandbox --help typically returns 0 even without the plugin installed, so the comment is valuable.

2. Consider adding to tests: The new checkDockerSandbox() function should have unit tests to cover:

   • Docker not running scenario
   • Plugin not installed scenario
   • Plugin installed and working scenario
   • Timeout scenarios

Verdict

Approve with minor fixes recommended. The core logic is sound and addresses the issue well. The main concern is the edge case in issue #3 where unexpected errors could be incorrectly treated as success. Issues #1 and #2 are style/consistency improvements.

---

[yazan-albaiz]

@claude re-review please.

[claude]

Claude finished @yazan-albaiz's task in 42s —— View job

---

Re-review Complete

Tasks

• Read changed files
• Analyze changes against previous review feedback
• Check for new issues
• Provide comprehensive review feedback

---

Review Summary

Excellent work! All three issues from the previous review have been addressed properly. The implementation is now solid and production-ready.

Previous Issues - All Resolved ✅

1. ✅ Unused async keyword - FIXED

The async keyword has been removed from checkDockerSandbox() on line 202. The function now correctly returns PreflightCheck directly since it uses only synchronous operations (spawnSync).

2. ✅ Edge case handling - FIXED

The edge case where unexpected errors from docker sandbox ls could fall through has been properly addressed on lines 244-251. Now any non-zero exit code that doesn't match the "plugin not found" patterns returns a proper error with helpful diagnostic information including the exit code.

// Handle other non-zero exit codes (permissions, network, etc.)
const errorDetail = stderr.trim() || `Unknown error (exit code: ${sandboxCheck.status})`;
return {
  name: 'Docker Sandbox',
  passed: false,
  message: `Docker sandbox check failed: ${errorDetail}`,
  fatal: true,
};

This ensures no scenario incorrectly passes the check.

3. ✅ Tests added

Comprehensive tests have been added in tests/preflight.test.ts (lines 186-210) covering:

• Skipping check when sandbox mode is disabled
• Running check when sandbox mode is enabled
• Proper error handling and formatting

Code Quality

The implementation follows all Ralph codebase patterns:

1. Consistent error handling - Uses synchronous spawnSync with proper timeout (10s)
2. Clear error messages - User-friendly messages that explain what's wrong and how to fix it
3. Proper conditionals - Docker sandbox check only runs when config.sandbox is true (lines 305-307)
4. Two-stage validation - First checks Docker is running, then validates sandbox plugin
5. Type safety - Properly typed with PreflightCheck interface

No New Issues Found

The code is clean with no new bugs, security issues, or performance problems identified.

Verdict

Approved ✅ - All previous issues have been properly addressed. This PR is ready to merge.

---