Skip to content

Commit e5404ed

Browse files
Lykhoydaclaude
andauthored
fix(#249): key the exit-0 Maestro failure guard on status lines, not a FAILED substring (B193) (#254)
The secondary guard after a zero exit scanned combined stdout+stderr with output.includes('FAILED') — app logs (FETCH_FAILED, LOGIN_FAILED) flipped passing flows to failed, triggering pointless auto-repair and corrupting pass/fail telemetry. New shared outputIndicatesFlowFailure in domain/maestro-error-parser matches only Maestro's terminal status lines; maestro-runner itself emits no uppercase FAILED at all (verified against the binary), so substring matches there could only ever be app-log false positives. Wired at all three call sites: maestro_run, maestro_test_all, and the inline maestro-invoke fallback. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 57dc310 commit e5404ed

10 files changed

Lines changed: 93 additions & 28 deletions

File tree

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
"rn-dev-agent-cdp": patch
3+
"rn-dev-agent-plugin": patch
4+
---
5+
6+
fix(#249): Maestro pass detection no longer flips passing flows to failed when app logs contain the substring `FAILED`. The exit-0 secondary guard in `maestro_run`, `maestro_test_all`, and the inline maestro fallback used a bare `output.includes('FAILED')` over combined stdout+stderr — app/console output like a `FETCH_FAILED` Redux action or a `LOGIN_FAILED` analytics event marked a genuinely passing flow as failed and triggered pointless auto-repair. All three call sites now share `outputIndicatesFlowFailure`, which keys on Maestro's own terminal status lines (`Test FAILED` / `Flow FAILED` / a `[FAILED]` step marker / a bare `FAILED` line) instead of a substring.

scripts/cdp-bridge/dist/domain/maestro-error-parser.js

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,3 +128,17 @@ export function parseMaestroFailure(output) {
128128
export function isAutoRepairable(failure) {
129129
return failure.kind === 'SELECTOR_NOT_FOUND';
130130
}
131+
/**
132+
* GH#249/B193: exit-0 secondary guard for "flow failed despite a zero exit".
133+
* The previous bare `output.includes('FAILED')` scanned combined stdout+stderr
134+
* — which carries app/console logs — so a passing flow whose app logged
135+
* FETCH_FAILED (or any *_FAILED token) was flagged failed and triggered
136+
* pointless auto-repair. Key on Maestro's own terminal status LINES instead:
137+
* `Test FAILED` / `Flow FAILED` (JVM Maestro), a `[FAILED]` step marker, or a
138+
* line that IS the bare token. maestro-runner emits no uppercase FAILED at all
139+
* (verified against the binary), so app-log content is the only thing a
140+
* substring match could ever catch there.
141+
*/
142+
export function outputIndicatesFlowFailure(output) {
143+
return /^\s*(?:\[FAILED\]|(?:Test|Flow) FAILED\b|FAILED\s*$)/m.test(output);
144+
}

scripts/cdp-bridge/dist/maestro-invoke.js

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { homedir, tmpdir } from 'node:os';
66
import { resolveBundleId, readExpoSlug } from './project-config.js';
77
import { buildMaestroFlow, parseAndValidateFlow, isValidBundleId, MaestroValidationError, } from './domain/maestro-validator.js';
88
import { chooseMaestroDispatch } from './tools/maestro-dispatch.js';
9+
import { outputIndicatesFlowFailure } from './domain/maestro-error-parser.js';
910
import { resolveAppFileForClearState } from './tools/resolve-ios-app-file.js';
1011
const execFile = promisify(execFileCb);
1112
// Escape a user-supplied string for safe embedding inside a double-quoted YAML scalar.
@@ -96,10 +97,10 @@ export async function runMaestroInline(yaml, opts) {
9697
try {
9798
const { stdout, stderr } = await execFile(dispatch.binPath, dispatch.buildArgs(opts.platform, flowFile, appFileResolution.appFile), { timeout, encoding: 'utf8', maxBuffer: 10 * 1024 * 1024 });
9899
const output = (stdout + '\n' + stderr).trim();
99-
// Runner exited 0 → authoritative pass. Key the secondary scan on maestro's
100-
// `FAILED` token only; a broad `Error:` match false-flagged passing runs
101-
// whose app/console output merely contained "Error:" (mirrors maestro_run).
102-
const passed = !output.includes('FAILED');
100+
// Runner exited 0 → authoritative pass. The secondary scan keys on Maestro's
101+
// own status LINES (GH#249: a bare `FAILED` substring false-flagged passing
102+
// runs whose app logs contained the token; mirrors maestro_run).
103+
const passed = !outputIndicatesFlowFailure(output);
103104
return { passed, output, flowFile };
104105
}
105106
catch (err) {

scripts/cdp-bridge/dist/tools/maestro-run.js

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { resolveBundleId, readExpoSlug } from '../project-config.js';
99
import { chooseMaestroDispatch, shouldWarnFallback } from './maestro-dispatch.js';
1010
import { resolveAppFileForClearState } from './resolve-ios-app-file.js';
1111
import { buildMaestroFlow, parseAndValidateFlow, isValidBundleId, MaestroValidationError, } from '../domain/maestro-validator.js';
12+
import { outputIndicatesFlowFailure } from '../domain/maestro-error-parser.js';
1213
import { stopFastRunner as defaultStopFastRunner } from '../runners/rn-fast-runner-client.js';
1314
import { releaseAndroidInteractionSlot as defaultReleaseAndroidSlot } from '../runners/release-android-slot.js';
1415
import { markCdpStale as defaultMarkCdpStale } from '../cdp/recovery.js';
@@ -165,11 +166,10 @@ export function createMaestroRunHandler() {
165166
const output = (stdout + '\n' + stderr).trim();
166167
// Reaching here means the runner exited 0 — that exit code is the
167168
// authoritative pass signal (a real flow failure exits non-zero and is
168-
// handled in the catch below). The output scan is only a secondary guard;
169-
// it keys on maestro's own `FAILED` status token rather than the previous
170-
// broad `Error:` match, which false-flagged passing runs whose app/console
171-
// logs merely contained "Error:".
172-
const passed = !output.includes('FAILED');
169+
// handled in the catch below). The output scan is only a secondary guard,
170+
// keyed on Maestro's own status LINES (GH#249: the prior bare `FAILED`
171+
// substring false-flagged passing runs whose app logs contained the token).
172+
const passed = !outputIndicatesFlowFailure(output);
173173
const meta = {
174174
passed,
175175
flowFile,

scripts/cdp-bridge/dist/tools/maestro-test-all.js

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { findProjectRoot } from '../nav-graph/storage.js';
99
import { chooseMaestroDispatch, shouldWarnFallback } from './maestro-dispatch.js';
1010
import { buildMaestroFlow, parseAndValidateFlow, MaestroValidationError, } from '../domain/maestro-validator.js';
1111
import { runFlowParked } from './maestro-run.js';
12+
import { outputIndicatesFlowFailure } from '../domain/maestro-error-parser.js';
1213
import { resolveAppFileForClearState } from './resolve-ios-app-file.js';
1314
const execFile = promisify(execFileCb);
1415
function discoverFlows(dir, pattern) {
@@ -113,11 +114,10 @@ export function createMaestroTestAllHandler() {
113114
const { stdout, stderr } = await runFlowParked(() => execFile(dispatch.binPath, dispatch.buildArgs(platform, safeFlowFile, appFile), { timeout, encoding: 'utf8', maxBuffer: 10 * 1024 * 1024 }), { platform, deviceId: getActiveSession()?.deviceId });
114115
const output = (stdout + '\n' + stderr).trim();
115116
// The runner already exited 0 here, so that exit code is the
116-
// authoritative pass signal. Key the secondary scan on maestro's own
117-
// `FAILED` token only — a broad `Error:` match false-flagged passing
118-
// runs whose app/console logs merely contained "Error:" (mirrors the
119-
// maestro_run fix).
120-
const ok = !output.includes('FAILED');
117+
// authoritative pass signal. The secondary scan keys on Maestro's own
118+
// status LINES (GH#249: a bare `FAILED` substring false-flagged passing
119+
// runs whose app logs contained the token; mirrors the maestro_run fix).
120+
const ok = !outputIndicatesFlowFailure(output);
121121
results.push({
122122
name,
123123
passed: ok,

scripts/cdp-bridge/src/domain/maestro-error-parser.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,3 +139,18 @@ export function parseMaestroFailure(output: string): MaestroFailure {
139139
export function isAutoRepairable(failure: MaestroFailure): boolean {
140140
return failure.kind === 'SELECTOR_NOT_FOUND';
141141
}
142+
143+
/**
144+
* GH#249/B193: exit-0 secondary guard for "flow failed despite a zero exit".
145+
* The previous bare `output.includes('FAILED')` scanned combined stdout+stderr
146+
* — which carries app/console logs — so a passing flow whose app logged
147+
* FETCH_FAILED (or any *_FAILED token) was flagged failed and triggered
148+
* pointless auto-repair. Key on Maestro's own terminal status LINES instead:
149+
* `Test FAILED` / `Flow FAILED` (JVM Maestro), a `[FAILED]` step marker, or a
150+
* line that IS the bare token. maestro-runner emits no uppercase FAILED at all
151+
* (verified against the binary), so app-log content is the only thing a
152+
* substring match could ever catch there.
153+
*/
154+
export function outputIndicatesFlowFailure(output: string): boolean {
155+
return /^\s*(?:\[FAILED\]|(?:Test|Flow) FAILED\b|FAILED\s*$)/m.test(output);
156+
}

scripts/cdp-bridge/src/maestro-invoke.ts

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
MaestroValidationError,
1212
} from './domain/maestro-validator.js';
1313
import { chooseMaestroDispatch } from './tools/maestro-dispatch.js';
14+
import { outputIndicatesFlowFailure } from './domain/maestro-error-parser.js';
1415
import { resolveAppFileForClearState } from './tools/resolve-ios-app-file.js';
1516

1617
const execFile = promisify(execFileCb);
@@ -129,10 +130,10 @@ export async function runMaestroInline(
129130
{ timeout, encoding: 'utf8', maxBuffer: 10 * 1024 * 1024 },
130131
);
131132
const output = (stdout + '\n' + stderr).trim();
132-
// Runner exited 0 → authoritative pass. Key the secondary scan on maestro's
133-
// `FAILED` token only; a broad `Error:` match false-flagged passing runs
134-
// whose app/console output merely contained "Error:" (mirrors maestro_run).
135-
const passed = !output.includes('FAILED');
133+
// Runner exited 0 → authoritative pass. The secondary scan keys on Maestro's
134+
// own status LINES (GH#249: a bare `FAILED` substring false-flagged passing
135+
// runs whose app logs contained the token; mirrors maestro_run).
136+
const passed = !outputIndicatesFlowFailure(output);
136137
return { passed, output, flowFile };
137138
} catch (err) {
138139
// execFile errors carry stdout/stderr from the failed child process. When

scripts/cdp-bridge/src/tools/maestro-run.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import {
1515
isValidBundleId,
1616
MaestroValidationError,
1717
} from '../domain/maestro-validator.js';
18+
import { outputIndicatesFlowFailure } from '../domain/maestro-error-parser.js';
1819
import { stopFastRunner as defaultStopFastRunner } from '../runners/rn-fast-runner-client.js';
1920
import { releaseAndroidInteractionSlot as defaultReleaseAndroidSlot } from '../runners/release-android-slot.js';
2021
import { markCdpStale as defaultMarkCdpStale } from '../cdp/recovery.js';
@@ -217,11 +218,10 @@ export function createMaestroRunHandler(): (args: MaestroRunArgs) => Promise<Too
217218
const output = (stdout + '\n' + stderr).trim();
218219
// Reaching here means the runner exited 0 — that exit code is the
219220
// authoritative pass signal (a real flow failure exits non-zero and is
220-
// handled in the catch below). The output scan is only a secondary guard;
221-
// it keys on maestro's own `FAILED` status token rather than the previous
222-
// broad `Error:` match, which false-flagged passing runs whose app/console
223-
// logs merely contained "Error:".
224-
const passed = !output.includes('FAILED');
221+
// handled in the catch below). The output scan is only a secondary guard,
222+
// keyed on Maestro's own status LINES (GH#249: the prior bare `FAILED`
223+
// substring false-flagged passing runs whose app logs contained the token).
224+
const passed = !outputIndicatesFlowFailure(output);
225225
const meta = {
226226
passed,
227227
flowFile,

scripts/cdp-bridge/src/tools/maestro-test-all.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
MaestroValidationError,
1515
} from '../domain/maestro-validator.js';
1616
import { runFlowParked } from './maestro-run.js';
17+
import { outputIndicatesFlowFailure } from '../domain/maestro-error-parser.js';
1718
import { resolveAppFileForClearState } from './resolve-ios-app-file.js';
1819

1920
const execFile = promisify(execFileCb);
@@ -151,11 +152,10 @@ export function createMaestroTestAllHandler(): (args: MaestroTestAllArgs) => Pro
151152
);
152153
const output = (stdout + '\n' + stderr).trim();
153154
// The runner already exited 0 here, so that exit code is the
154-
// authoritative pass signal. Key the secondary scan on maestro's own
155-
// `FAILED` token only — a broad `Error:` match false-flagged passing
156-
// runs whose app/console logs merely contained "Error:" (mirrors the
157-
// maestro_run fix).
158-
const ok = !output.includes('FAILED');
155+
// authoritative pass signal. The secondary scan keys on Maestro's own
156+
// status LINES (GH#249: a bare `FAILED` substring false-flagged passing
157+
// runs whose app logs contained the token; mirrors the maestro_run fix).
158+
const ok = !outputIndicatesFlowFailure(output);
159159

160160
results.push({
161161
name,
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
import { test } from 'node:test';
2+
import assert from 'node:assert/strict';
3+
import { outputIndicatesFlowFailure } from '../../dist/domain/maestro-error-parser.js';
4+
5+
// GH#249/B193: the exit-0 secondary guard was `output.includes('FAILED')` over combined
6+
// stdout+stderr — which contains app/console logs, so a passing flow whose app merely
7+
// logged the substring (FETCH_FAILED, LOGIN_FAILED, ...) was flagged failed and kicked
8+
// off pointless auto-repair. The guard must key on Maestro's own terminal status LINES.
9+
// (maestro-runner emits no uppercase FAILED at all — verified against the binary — so
10+
// the token only ever comes from the JVM Maestro fallback, e.g. the 'Test FAILED' line
11+
// already pinned in maestro-error-parser.test.js.)
12+
13+
test('#249 matches Maestro terminal status lines', () => {
14+
assert.equal(outputIndicatesFlowFailure('Test FAILED'), true);
15+
assert.equal(outputIndicatesFlowFailure('[INFO] Tapping on "ok"\nTest FAILED\n'), true);
16+
assert.equal(outputIndicatesFlowFailure('Flow FAILED'), true);
17+
assert.equal(outputIndicatesFlowFailure('[FAILED] Tap on element with id "submit"'), true);
18+
assert.equal(outputIndicatesFlowFailure(' FAILED '), true); // bare status line, indented
19+
});
20+
21+
test('#249 does NOT match app/console log content that merely contains the substring', () => {
22+
assert.equal(outputIndicatesFlowFailure('[INFO] dispatching action FETCH_FAILED'), false);
23+
assert.equal(outputIndicatesFlowFailure('console.log: USER_REGISTRATION_FAILED event sent'), false);
24+
assert.equal(outputIndicatesFlowFailure('2026-06-10 12:00:01 upload FAILED midway, retrying'), false);
25+
assert.equal(outputIndicatesFlowFailure('FAILED to fetch user profile'), false); // line-leading but prose, not a status line
26+
assert.equal(outputIndicatesFlowFailure('All steps passed'), false);
27+
assert.equal(outputIndicatesFlowFailure(''), false);
28+
});

0 commit comments

Comments
 (0)