Improve native wait stability diagnostics#749
Conversation
Constraint: Preserve existing app_wait_for defaults and add only optional diagnostics. Rejected: Introduce a new locator or action retry engine | broader behavior change than the wait-diagnostics issue needs. Confidence: high Scope-risk: narrow Directive: Keep timeout samples bounded so AX dumps do not become noisy or leak-heavy artifacts. Tested: npm test -- --runTestsByPath tests/unit/app-wait-for.test.ts; npm run lint -- --quiet src/tools/app-wait-for.ts tests/unit/app-wait-for.test.ts Not-tested: Live simulator wait timeout evidence.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
opensafari/src/tools/app-wait-for.ts
Lines 235 to 236 in 910d503
Reset stability window after polling errors
When stable_ms is set, a transient bridge.query failure should break the “continuous hold” window, but this catch block just continues and preserves firstMetAtMs from earlier successful polls. In scenarios with intermittent query errors, the next successful poll can incorrectly count the errored interval toward stable_for_ms and return status: "found" even though the condition was not continuously observed, which violates the new stability guarantee and can produce false positives.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Constraint: stable_ms must mean continuously observed condition satisfaction. Rejected: Count bridge query errors as neutral time | that can produce false positive stable waits after transient AX failures. Confidence: high Scope-risk: narrow Directive: Any future wait retry layer must preserve this reset-on-unobserved-poll contract. Tested: npm test -- --runTestsByPath tests/unit/app-wait-for.test.ts tests/unit/app-wait-for-diagnostics.test.ts; npm run lint -- --quiet src/tools/app-wait-for.ts tests/unit/app-wait-for.test.ts tests/unit/app-wait-for-diagnostics.test.ts; npm run build Not-tested: Live simulator intermittent AX query failure.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea43c33b5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| sample_limit: { | ||
| type: 'number', | ||
| description: 'Maximum matching AX nodes to include in timeout diagnostics (default: 5, max: 20).', | ||
| }, |
There was a problem hiding this comment.
The new sample_limit option is wired into polling, but on the timeout path the response still only returns { error, condition, query, timeout, elapsed, polls }, so callers never receive last_matches/stability diagnostics for the failure case this feature targets. In practice, timed-out waits get no extra debugging signal and sample_limit becomes effectively a no-op unless the condition succeeds; please include the tracked lastEvaluation/stability fields in the timeout payload.
Useful? React with 👍 / 👎.
1 participant
Summary
stable_msandsample_limitinputs toapp_wait_for.matching_count,last_matches, stability fields).Closes #745.
Direction/scope review
Verification
npm test -- --runTestsByPath tests/unit/app-wait-for.test.ts tests/unit/app-wait-for-diagnostics.test.tsnpm run lint -- --quiet src/tools/app-wait-for.ts tests/unit/app-wait-for.test.ts tests/unit/app-wait-for-diagnostics.test.tsLive validation after merge
stable_ms=500and confirm success includes stability fields.last_matchesand context metadata.