morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #6023] [MERGED] fix(electron): record actions skipped after a failed action #7128

New issue

Closed

opened 2026-11-01 21:45:36 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:45:36 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6023
Author: @kaoru0822-kitauji
Created: 10/21/2026
Status: ✅ Merged
Merged: 10/21/2026
Merged by: @kaoru0822-kitauji

Base: main ← Head: fix/5943-electron-unexecuted-actions

📝 Commits (1)

3b48be7 fix(electron): record actions skipped after a failed action

📊 Changes

8 files changed (+186 additions, -18 deletions)

View changed files

📝 docs/browser-scenario-contract.md (+3 -1)
📝 docs/electron-observation.md (+3 -1)
📝 src/browser/PlaywrightBrowserScenarioProvider.test.ts (+13 -2)
📝 src/browser/PlaywrightBrowserScenarioProvider.ts (+15 -11)
📝 src/browser/PlaywrightElectronActiveActions.test.ts (+40 -0)
📝 src/browser/PlaywrightElectronActiveActions.ts (+24 -3)
📝 src/browser/PlaywrightElectronActiveProvider.test.ts (+74 -0)
➕ src/browser/ScenarioActionSequence.ts (+14 -0)

📄 Description

What broke: capture_electron_scenario returned no record for declared actions after the first failed or cancelled action, so normalized_result.actions could not account for the whole scenario.
Fix: The Electron action runner now records each remaining action as cancelled with a not-executed error. This is the representation browser scenarios already use, and both runners now share one helper that produces that error.
Risk: Low. No schema change: cancelled with an error string is already in the Electron action contract. Callers now get extra records with elapsed_ms: 0, and a browser step that was not executed after cancellation now says "cancellation" instead of "failure".

Summary

Fixes #5943.

Before this change, an Electron scenario stopped at the first failed or cancelled action. Every action after it was left out of normalized_result.actions, while parameters.actions still listed it. Now every declared action has exactly one result record, and an action that never ran is recorded explicitly.

Problem and expected behavior

Smallest trigger: actions [click #duplicate (rejects), wait 0]. The capture succeeds with actions: [{ step_id: "fail", status: "failed" }], wait is never called, and nothing records settle. This breaks the per-item outcome invariant in docs/tool-design.md, and docs/electron-observation.md says the result records action status. The expected behavior is one record per declared action, and actions after the stop are marked as not executed.

Change and scope

src/browser/ScenarioActionSequence.ts (new): notExecutedActionError(stoppedBy) is the single owner of the not-executed explanation. It returns "not executed after an earlier action failure" or "not executed after scenario request cancellation".
runElectronActions keeps going through the declared actions after a stop but does not run them. Each remaining action is recorded with status: "cancelled", target: null, elapsed_ms: 0, its declared window_index, and the shared error.
The browser scenario runner now uses the same helper in place of its inline string. Its priorFailure boolean is replaced by the status of the action that stopped the sequence, so steps skipped after cancellation now name cancellation instead of failure.
Docs: the action-status sentences in docs/electron-observation.md and docs/browser-scenario-contract.md were updated.
Not included: the remaining actions are still not run after a failure, and no new status value was added.

Contribution scope: not applicable. This is an ordinary fix within existing boundaries.

Contract and boundary impact

Semantic owner and earliest changed stage: the scenario action runners (runElectronActions and the browser runScenario/executeStep), plus the shared helper ScenarioActionSequence.ts.
Contribution scope fit and linked maintainer decision: not applicable (ordinary fix).
CLI and MCP/tool-catalog contract: no schema change. actions[] now has one entry per declared action.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: Evidence now accounts for the whole declared action sequence. For browser steps, the reason for unavailable artifacts now matches the step error.
Target selection, authority, upstream behavior, and cleanup details: unchanged. Actions after the stop are still not run.
Generated metadata, package, or installation impact: none. docs:check passes.
Launch or attach to a process.
Inject instrumentation or run scripts/code.

(These describe what the existing tools already do. This PR adds no new effects.)

Evidence and regression coverage

Tests added or updated:
- PlaywrightElectronActiveProvider.test.ts: provider capture with the real action runner. A failing click followed by wait and deep-link records both later actions as not executed after a failure. The test also checks that wait is never called and the app is closed.
- PlaywrightElectronActiveActions.test.ts: when cancellation arrives during the first action, the following renderer-reload is recorded as not executed after cancellation and reload is not called.
- PlaywrightBrowserScenarioProvider.test.ts: checks the not-executed error after a failure, and a scenario cancelled mid-action records its third step as not executed after cancellation.
Base reproduction: the issue's MCP repro test was run on base 6d356b843 and reproduced the bug: returnedActions: [fail/failed], waitCalls: 0, isError: false. With the fix the same test returns [fail/failed, settle/cancelled "not executed after an earlier action failure"]. The temporary test is not committed. With only the two source files reverted, the three new or updated assertions fail (Electron provider, Electron actions, browser cancellation).

User-visible MCP output (simulated app, issue repro):

{"returnedActions":[{"step_id":"fail","status":"failed","error":"simulated strict mode violation"},{"step_id":"settle","status":"cancelled","error":"not executed after an earlier action failure"}],"waitCalls":0,"coverage":"partial_attach"}

Remaining proof gaps: no real Electron runtime was launched. Action failures were simulated with test doubles.
Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

Issue MCP repro (vitest run, temporary file): on base it reproduced the missing record; with the fix it passes with both actions accounted for.
npm run test:local -- src/browser/*.test.ts (42 files): 514 passed.
npm run test:focused -- tests/boundary/mcp/electronMcp.test.ts: 8 passed.
npm run check:fast: passed.
npm run format:check: passed.
npm run docs:check: passed.

Compatibility, safety, and release

Breaking changes or migration steps: none. The extra cancelled records use statuses the contract already defines.
Real Hopper/Ghidra, browser, or OS coverage: none. Only simulated Electron and Playwright doubles were used.
Package or release metadata impact: none.
Security, privacy, process, or containment review: no new process interaction. Actions after a stop are still not run; only their records are added.

Author readiness

Human owner: @kaoru0822-kitauji

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare this PR ready for initial maintainer review.

Before merge

This PR addresses a concrete problem or an agreed enhancement.
Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
The PR has one focused outcome and the title follows type(scope): outcome.
Related issue is linked, or the reason for not linking one is stated above.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed.
User-visible CLI/MCP changes include representative output.
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🤖 Generated with Claude Code

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/6023 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/21/2026 **Status:** ✅ Merged **Merged:** 10/21/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/5943-electron-unexecuted-actions` --- ### 📝 Commits (1) - [`3b48be7`](https://github.com/morluto/rea/commit/3b48be727b3f73f963cce6f94b3d4ffdeb97387c) fix(electron): record actions skipped after a failed action ### 📊 Changes **8 files changed** (+186 additions, -18 deletions) <details> <summary>View changed files</summary> 📝 `docs/browser-scenario-contract.md` (+3 -1) 📝 `docs/electron-observation.md` (+3 -1) 📝 `src/browser/PlaywrightBrowserScenarioProvider.test.ts` (+13 -2) 📝 `src/browser/PlaywrightBrowserScenarioProvider.ts` (+15 -11) 📝 `src/browser/PlaywrightElectronActiveActions.test.ts` (+40 -0) 📝 `src/browser/PlaywrightElectronActiveActions.ts` (+24 -3) 📝 `src/browser/PlaywrightElectronActiveProvider.test.ts` (+74 -0) ➕ `src/browser/ScenarioActionSequence.ts` (+14 -0) </details> ### 📄 Description **What broke:** `capture_electron_scenario` returned no record for declared actions after the first failed or cancelled action, so `normalized_result.actions` could not account for the whole scenario. **Fix:** The Electron action runner now records each remaining action as `cancelled` with a not-executed error. This is the representation browser scenarios already use, and both runners now share one helper that produces that error. **Risk:** Low. No schema change: `cancelled` with an error string is already in the Electron action contract. Callers now get extra records with `elapsed_ms: 0`, and a browser step that was not executed after cancellation now says "cancellation" instead of "failure". ## Summary Fixes #5943. Before this change, an Electron scenario stopped at the first failed or cancelled action. Every action after it was left out of `normalized_result.actions`, while `parameters.actions` still listed it. Now every declared action has exactly one result record, and an action that never ran is recorded explicitly. ## Problem and expected behavior Smallest trigger: actions `[click #duplicate (rejects), wait 0]`. The capture succeeds with `actions: [{ step_id: "fail", status: "failed" }]`, `wait` is never called, and nothing records `settle`. This breaks the per-item outcome invariant in docs/tool-design.md, and docs/electron-observation.md says the result records action status. The expected behavior is one record per declared action, and actions after the stop are marked as not executed. ## Change and scope - `src/browser/ScenarioActionSequence.ts` (new): `notExecutedActionError(stoppedBy)` is the single owner of the not-executed explanation. It returns "not executed after an earlier action failure" or "not executed after scenario request cancellation". - `runElectronActions` keeps going through the declared actions after a stop but does not run them. Each remaining action is recorded with `status: "cancelled"`, `target: null`, `elapsed_ms: 0`, its declared `window_index`, and the shared error. - The browser scenario runner now uses the same helper in place of its inline string. Its `priorFailure` boolean is replaced by the status of the action that stopped the sequence, so steps skipped after cancellation now name cancellation instead of failure. - Docs: the action-status sentences in `docs/electron-observation.md` and `docs/browser-scenario-contract.md` were updated. - Not included: the remaining actions are still not run after a failure, and no new status value was added. Contribution scope: not applicable. This is an ordinary fix within existing boundaries. ## Contract and boundary impact - Semantic owner and earliest changed stage: the scenario action runners (`runElectronActions` and the browser `runScenario`/`executeStep`), plus the shared helper `ScenarioActionSequence.ts`. - Contribution scope fit and linked maintainer decision: not applicable (ordinary fix). - CLI and MCP/tool-catalog contract: no schema change. `actions[]` now has one entry per declared action. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: Evidence now accounts for the whole declared action sequence. For browser steps, the reason for unavailable artifacts now matches the step error. - Target selection, authority, upstream behavior, and cleanup details: unchanged. Actions after the stop are still not run. - Generated metadata, package, or installation impact: none. `docs:check` passes. - [x] Launch or attach to a process. - [x] Inject instrumentation or run scripts/code. (These describe what the existing tools already do. This PR adds no new effects.) ## Evidence and regression coverage - Tests added or updated: - `PlaywrightElectronActiveProvider.test.ts`: provider capture with the real action runner. A failing click followed by `wait` and `deep-link` records both later actions as not executed after a failure. The test also checks that `wait` is never called and the app is closed. - `PlaywrightElectronActiveActions.test.ts`: when cancellation arrives during the first action, the following `renderer-reload` is recorded as not executed after cancellation and `reload` is not called. - `PlaywrightBrowserScenarioProvider.test.ts`: checks the not-executed error after a failure, and a scenario cancelled mid-action records its third step as not executed after cancellation. - Base reproduction: the issue's MCP repro test was run on base `6d356b843` and reproduced the bug: `returnedActions: [fail/failed]`, `waitCalls: 0`, `isError: false`. With the fix the same test returns `[fail/failed, settle/cancelled "not executed after an earlier action failure"]`. The temporary test is not committed. With only the two source files reverted, the three new or updated assertions fail (Electron provider, Electron actions, browser cancellation). - User-visible MCP output (simulated app, issue repro): ```json {"returnedActions":[{"step_id":"fail","status":"failed","error":"simulated strict mode violation"},{"step_id":"settle","status":"cancelled","error":"not executed after an earlier action failure"}],"waitCalls":0,"coverage":"partial_attach"} ``` - Remaining proof gaps: no real Electron runtime was launched. Action failures were simulated with test doubles. - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - Issue MCP repro (`vitest run`, temporary file): on base it reproduced the missing record; with the fix it passes with both actions accounted for. - `npm run test:local -- src/browser/*.test.ts` (42 files): 514 passed. - `npm run test:focused -- tests/boundary/mcp/electronMcp.test.ts`: 8 passed. - `npm run check:fast`: passed. - `npm run format:check`: passed. - `npm run docs:check`: passed. ## Compatibility, safety, and release - Breaking changes or migration steps: none. The extra `cancelled` records use statuses the contract already defines. - Real Hopper/Ghidra, browser, or OS coverage: none. Only simulated Electron and Playwright doubles were used. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new process interaction. Actions after a stop are still not run; only their records are added. ## Author readiness Human owner: @kaoru0822-kitauji - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-11-01 21:45:36 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-03 13:39:27 +00:00

[PR #7128] [MERGED] fix(javascript): stop recording replaced slot values as store receivers #7661

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#7128

No description provided.

Rows
Columns