morluto/rea

Fork 0

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

[PR #2302] [CLOSED] fix(electron): point scenario path errors at the field that failed #5148

New issue

Closed

opened 2026-10-31 01:04:21 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:04:21 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2302
Author: @harshdazz
Created: 10/12/2026
Status: ❌ Closed

Base: main ← Head: fix/electron-scenario-path-issues

📝 Commits (1)

7d7e49f fix(electron): point scenario path errors at the field that failed

📊 Changes

2 files changed (+206 additions, -56 deletions)

View changed files

📝 src/application/javascript/ElectronActiveObservationService.ts (+74 -24)
📝 tests/boundary/mcp/electronMcp.test.ts (+132 -32)

📄 Description

Summary

Fixes #2272. When one of the capture_electron_scenario paths doesn't exist, the error now names that path. Before, every failure pointed at application_path with invalid_format, so an agent following issues[].path would go and fix the wrong argument.

I said I'd pick this up on the issue before starting. #2279 came in a few minutes after that with a similar approach, so happy for you to go with whichever fits better.

Problem and expected behavior

canonicalizeInput ran all three realpath calls inside one try, and the catch returned a single hard-coded issue: application_path, invalid_format, "Electron executable, application, and root must resolve to existing paths". A missing executable or a missing application_root got blamed on application_path, and the errno was dropped.

Expected: one issue for each path that doesn't resolve, on its own field, with the error code.

Change and scope

Each path is resolved on its own (resolveInputPath). A failure becomes { path: [field], reason: "invalid_value", message: "<label> must resolve to an existing path (<code>)" }.
All failing paths are reported together and the provider is still only called once everything resolves. The causes stay on the error (AggregateError when there's more than one).
I used the existing invalid_value rather than adding a not_found reason, since the path is well-formed and AnalysisInputIssue reasons are shared by every tool.
In the test file I moved the provider and server setup from the existing active-scenario test into two small helpers so the new test doesn't copy it. The jscpd clone count is the same as main.
Not included: the launch crash in #2269.

Follow the contribution scope.
Unresolved scope, authority, side effects, or required verification block
integration. Link the required maintainer scope decision for new providers or
capabilities involving target execution, injection, mutation, or protection
recovery; ordinary fixes within existing boundaries may state "not applicable".
Draft PRs may leave the scope decision pending and the final review checklist
unchecked. Maintainers can help resolve open questions.

Contract and boundary impact

Semantic owner and earliest changed stage: ElectronActiveObservationService.canonicalizeInput, request validation before provider admission.
Contribution scope fit and linked maintainer decision: routine fix within existing boundaries, not applicable.
CLI and MCP/tool-catalog contract: schemas unchanged. For this validation error, issues[].path now names the failing field, reason is invalid_value instead of invalid_format, and there's one issue per failing path.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: none. Successful requests canonicalize the same way, and application_root still defaults to the application's directory.
Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable, nothing is launched on this path.
Generated metadata (docs/public/product-catalog.json), package, or installation impact (CI generates and validates ignored catalogs and skills/; edit authored instructions in .agents/skills/reverse-engineer-anything/ and never commit build outputs): none.

Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.

Read existing files or captured data.
Launch or attach to a process.
Inject instrumentation or run scripts/code.
Change process memory, registers, files, or configuration.
Contact network services.
Recover protected code or work around protections.
Documentation or usability only; no target interaction.
Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

Tests added or updated: new MCP boundary case in tests/boundary/mcp/electronMcp.test.ts covering a missing executable, a missing application_root, and executable plus application both missing. It checks the exact issues and that the provider is never called.
Base reproduction or other evidence: the new test on main's source (aeee5074) fails with:

-           "message": "Electron executable must resolve to an existing path (ENOENT)",
+           "message": "Electron executable, application, and root must resolve to existing paths",
            "path": [
-             "executable_path",
+             "application_path",
            ],
-           "reason": "invalid_value",
+           "reason": "invalid_format",

User-visible CLI/MCP output (if applicable): the issue's cases through node scripts/rea.mjs capture-electron-scenario <json> --json after npm run build:cached. On main, all four print the same issue:

[{"path":["application_path"],"reason":"invalid_format","message":"Electron executable, application, and root must resolve to existing paths"}]

With this change:

=== A missing executable  exit=1
[{"path":["executable_path"],"reason":"invalid_value","message":"Electron executable must resolve to an existing path (ENOENT)"}]
=== B missing application  exit=1
[{"path":["application_path"],"reason":"invalid_value","message":"Electron application must resolve to an existing path (ENOENT)"}]
=== C missing root  exit=1
[{"path":["application_root"],"reason":"invalid_value","message":"Electron application root must resolve to an existing path (ENOENT)"}]
=== D executable + application missing  exit=1
[{"path":["executable_path"],"reason":"invalid_value","message":"Electron executable must resolve to an existing path (ENOENT)"},{"path":["application_path"],"reason":"invalid_value","message":"Electron application must resolve to an existing path (ENOENT)"}]

Remaining proof gaps: I ran everything on Windows 11 only, so Linux is down to CI. 4 other tests in electronMcp.test.ts fail on Windows on main too (root.split("/"), symlink(..., "dir") needing admin, analyze_javascript_application erroring), including the existing active-scenario test. I ran that one with a local-only patch (split on both separators, junction instead of symlink) and it passed with this change. That patch isn't in the PR.

For evidence-bearing changes:

Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Not an evidence-bearing change, so these are left unticked.

Validation performed

All on Windows 11 x64, Node 22.19.0, base aeee5074.

npx vitest run --project mcp-boundary tests/boundary/mcp/electronMcp.test.ts: new test passes. 4 failed | 2 passed, and the same 4 fail on main (4 failed | 1 passed).
npm run check: passes, Lint: 2644 files, 0 errors, 931 warnings., none in the changed files.
npm run jscpd: 2228 files, 1309 clones, 2.82% duplicated lines., same as main.
npx tsc --noEmit: passes.
npx oxfmt --check on the two files: passes.
CLI repro above, on main and on this branch.

I ran vitest directly because npm run test:focused fails on Windows with 'C:\Program' is not recognized. That's a separate bug in scripts/run-exclusive-command.mjs and I'll open an issue for it.

Compatibility, safety, and release

Breaking changes or migration steps: none. Only the issue path, reason and message of this one validation error change, and the old values were wrong.
Real Hopper/Ghidra, browser, or OS coverage: no provider involved. Windows 11 locally.
Package or release metadata impact: none.
Security, privacy, process, or containment review: the message only has the field label and the error code, not the path.

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.

_{🔄 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/2302 **Author:** [@harshdazz](https://github.com/harshdazz) **Created:** 10/12/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/electron-scenario-path-issues` --- ### 📝 Commits (1) - [`7d7e49f`](https://github.com/morluto/rea/commit/7d7e49f749d07ab1dd8aa144ba15ad02b513873d) fix(electron): point scenario path errors at the field that failed ### 📊 Changes **2 files changed** (+206 additions, -56 deletions) <details> <summary>View changed files</summary> 📝 `src/application/javascript/ElectronActiveObservationService.ts` (+74 -24) 📝 `tests/boundary/mcp/electronMcp.test.ts` (+132 -32) </details> ### 📄 Description ## Summary Fixes #2272. When one of the `capture_electron_scenario` paths doesn't exist, the error now names that path. Before, every failure pointed at `application_path` with `invalid_format`, so an agent following `issues[].path` would go and fix the wrong argument. I said I'd pick this up on the issue before starting. #2279 came in a few minutes after that with a similar approach, so happy for you to go with whichever fits better. ## Problem and expected behavior `canonicalizeInput` ran all three `realpath` calls inside one `try`, and the `catch` returned a single hard-coded issue: `application_path`, `invalid_format`, "Electron executable, application, and root must resolve to existing paths". A missing executable or a missing `application_root` got blamed on `application_path`, and the errno was dropped. Expected: one issue for each path that doesn't resolve, on its own field, with the error code. ## Change and scope - Each path is resolved on its own (`resolveInputPath`). A failure becomes `{ path: [field], reason: "invalid_value", message: "<label> must resolve to an existing path (<code>)" }`. - All failing paths are reported together and the provider is still only called once everything resolves. The causes stay on the error (`AggregateError` when there's more than one). - I used the existing `invalid_value` rather than adding a `not_found` reason, since the path is well-formed and `AnalysisInputIssue` reasons are shared by every tool. - In the test file I moved the provider and server setup from the existing active-scenario test into two small helpers so the new test doesn't copy it. The jscpd clone count is the same as `main`. - Not included: the launch crash in #2269. Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). Unresolved scope, authority, side effects, or required verification block integration. Link the required maintainer scope decision for new providers or capabilities involving target execution, injection, mutation, or protection recovery; ordinary fixes within existing boundaries may state "not applicable". Draft PRs may leave the scope decision pending and the final review checklist unchecked. Maintainers can help resolve open questions. ## Contract and boundary impact - Semantic owner and earliest changed stage: `ElectronActiveObservationService.canonicalizeInput`, request validation before provider admission. - Contribution scope fit and linked maintainer decision: routine fix within existing boundaries, not applicable. - CLI and MCP/tool-catalog contract: schemas unchanged. For this validation error, `issues[].path` now names the failing field, `reason` is `invalid_value` instead of `invalid_format`, and there's one issue per failing path. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: none. Successful requests canonicalize the same way, and `application_root` still defaults to the application's directory. - Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable, nothing is launched on this path. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact (CI generates and validates ignored catalogs and `skills/`; edit authored instructions in `.agents/skills/reverse-engineer-anything/` and never commit build outputs): none. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [x] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added or updated: new MCP boundary case in `tests/boundary/mcp/electronMcp.test.ts` covering a missing executable, a missing `application_root`, and executable plus application both missing. It checks the exact issues and that the provider is never called. - Base reproduction or other evidence: the new test on `main`'s source (`aeee5074`) fails with: ``` - "message": "Electron executable must resolve to an existing path (ENOENT)", + "message": "Electron executable, application, and root must resolve to existing paths", "path": [ - "executable_path", + "application_path", ], - "reason": "invalid_value", + "reason": "invalid_format", ``` - User-visible CLI/MCP output (if applicable): the issue's cases through `node scripts/rea.mjs capture-electron-scenario <json> --json` after `npm run build:cached`. On `main`, all four print the same issue: ``` [{"path":["application_path"],"reason":"invalid_format","message":"Electron executable, application, and root must resolve to existing paths"}] ``` With this change: ``` === A missing executable exit=1 [{"path":["executable_path"],"reason":"invalid_value","message":"Electron executable must resolve to an existing path (ENOENT)"}] === B missing application exit=1 [{"path":["application_path"],"reason":"invalid_value","message":"Electron application must resolve to an existing path (ENOENT)"}] === C missing root exit=1 [{"path":["application_root"],"reason":"invalid_value","message":"Electron application root must resolve to an existing path (ENOENT)"}] === D executable + application missing exit=1 [{"path":["executable_path"],"reason":"invalid_value","message":"Electron executable must resolve to an existing path (ENOENT)"},{"path":["application_path"],"reason":"invalid_value","message":"Electron application must resolve to an existing path (ENOENT)"}] ``` - Remaining proof gaps: I ran everything on Windows 11 only, so Linux is down to CI. 4 other tests in `electronMcp.test.ts` fail on Windows on `main` too (`root.split("/")`, `symlink(..., "dir")` needing admin, `analyze_javascript_application` erroring), including the existing active-scenario test. I ran that one with a local-only patch (split on both separators, junction instead of symlink) and it passed with this change. That patch isn't in the PR. For evidence-bearing changes: - [ ] Observed, derived, and inferred claims remain distinguishable. - [ ] Artifact identity, source provenance, and failed attempts remain preserved. - [ ] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. Not an evidence-bearing change, so these are left unticked. ## Validation performed All on Windows 11 x64, Node 22.19.0, base `aeee5074`. - `npx vitest run --project mcp-boundary tests/boundary/mcp/electronMcp.test.ts`: new test passes. `4 failed | 2 passed`, and the same 4 fail on `main` (`4 failed | 1 passed`). - `npm run check`: passes, `Lint: 2644 files, 0 errors, 931 warnings.`, none in the changed files. - `npm run jscpd`: `2228 files, 1309 clones, 2.82% duplicated lines.`, same as `main`. - `npx tsc --noEmit`: passes. - `npx oxfmt --check` on the two files: passes. - CLI repro above, on `main` and on this branch. I ran vitest directly because `npm run test:focused` fails on Windows with `'C:\Program' is not recognized`. That's a separate bug in `scripts/run-exclusive-command.mjs` and I'll open an issue for it. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Only the issue path, reason and message of this one validation error change, and the old values were wrong. - Real Hopper/Ghidra, browser, or OS coverage: no provider involved. Windows 11 locally. - Package or release metadata impact: none. - Security, privacy, process, or containment review: the message only has the field label and the error code, not the path. ## 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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:04:21 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:43:06 +00:00

[PR #5167] [MERGED] feat(cli): summarize managed members with durable Evidence paging #6796

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#5148

No description provided.

Rows
Columns