Fix: auto update check error should not fail the running command

[Copilot]

When the background update check hits a network/HTTP error (e.g. 401, 403 rate-limit), CLI_Command::get_updates() calls WP_CLI::error(), which exits the process — silently killing whatever command the user actually ran.

Changes

• Runner::auto_check_update(): Replace the bare ob_start() / WP_CLI::run_command() pattern with WP_CLI::runcommand() using exit_error => false and return => 'all'
  • exit_error => false sets $capture_exit = true so WP_CLI::error() throws ExitException instead of calling exit()
  • return => 'all' swaps in an Execution logger that captures stdout/stderr in memory — the error message is never printed to the terminal
  • On failure, a WP_CLI::debug() message is emitted (visible with --debug) and the check silently returns without affecting the user's command

// Before: any HTTP error in get_updates() would call exit(1)
ob_start();
WP_CLI::run_command( [ 'cli', 'check-update' ], [ 'format' => 'count' ] );
$count = ob_get_clean();

// After: errors are captured, not propagated
$result = WP_CLI::runcommand(
    'cli check-update --format=count',
    [ 'launch' => false, 'exit_error' => false, 'return' => 'all' ]
);
if ( ! is_object( $result ) || $result->return_code ) {
    WP_CLI::debug( 'Background update check failed: ' . ( is_object( $result ) ? trim( $result->stderr ) : '' ), 'auto-update' );
    return;
}

Summary by CodeRabbit

• Bug Fixes
  • Improved update-availability checking to handle network or HTTP failures more gracefully without interrupting normal execution.
  • Added clearer diagnostics when the background update check encounters issues.
  • Enhanced reliability in determining update availability and reporting the correct update count.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
php/WP_CLI/Runner.php	0.00%	4 Missing ⚠️

📢 Thoughts on this report? Let us know!

[schlessera]

I dug further into runcommand(), and I think removing this guard was too optimistic. With the current literal return => 'all', every normal return path constructs the expected object. However, runcommand() has a mixed, unenforced contract: it initializes $retval to null and returns null for unsupported or falsy return modes.

More importantly, this code is meant to fail open. If the result contract ever isn’t met, the direct property accesses emit warnings (or throw through an installed error handler) and can once again interfere with the user’s command.

Please keep defensive validation here, and validate the full result rather than only calling is_object(): return_code, stdout, and stderr should exist and have the expected types. We should also reject unexpected non-numeric STDOUT instead of treating any non-empty output as an update count.

This still won’t protect against exit/die or an uncaught Error inside the in-process command, since runcommand() never returns in those cases. That’s a separate limitation of launch => false.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

php/WP_CLI/Runner.php (1)

2728-2733: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Validate the runcommand() result before reading stdout. is_object() doesn't guarantee return_code, stdout, and stderr exist or have the expected types, so malformed results can still emit notices and a non-numeric stdout can fall through to the update prompt. Reject missing fields and cast a numeric count before continuing.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@php/WP_CLI/Runner.php` around lines 2728 - 2733, Validate the result in the
background update check before accessing its fields: require an object with
valid return_code, stderr, and stdout values, and retain the existing
debug-and-return failure path for malformed results. Convert stdout to a numeric
count before assigning it to $count so non-numeric output cannot reach the
update prompt.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@php/WP_CLI/Runner.php`:
- Around line 2728-2733: Validate the result in the background update check
before accessing its fields: require an object with valid return_code, stderr,
and stdout values, and retain the existing debug-and-return failure path for
malformed results. Convert stdout to a numeric count before assigning it to
$count so non-numeric output cannot reach the update prompt.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f589edd8-9c12-4a9e-b19a-0bd9eaf86df5

📥 Commits

Reviewing files that changed from the base of the PR and between 1fb78a6 and c82caee.

📒 Files selected for processing (1)

• php/WP_CLI/Runner.php

[swissspidy]

I dug further into runcommand(), and I think removing this guard was too optimistic. With the current literal return => 'all', every normal return path constructs the expected object. However, runcommand() has a mixed, unenforced contract: it initializes $retval to null and returns null for unsupported or falsy return modes.

More importantly, this code is meant to fail open. If the result contract ever isn’t met, the direct property accesses emit warnings (or throw through an installed error handler) and can once again interfere with the user’s command.

Please keep defensive validation here, and validate the full result rather than only calling is_object(): return_code, stdout, and stderr should exist and have the expected types. We should also reject unexpected non-numeric STDOUT instead of treating any non-empty output as an update count.

This still won’t protect against exit/die or an uncaught Error inside the in-process command, since runcommand() never returns in those cases. That’s a separate limitation of launch => false.

@schlessera Mind double checking? I don't see how it could never return in those cases. The return values seem pretty clear, and there is WPCliRuncommandDynamicReturnTypeExtension to have proper static analysis for this method as well. The only bug I found in there is wp-cli/wp-cli-tests#339