Skip to content

fix(electron): reject non-directory scenario roots before launch - #8949

Open
ShuaixinHuang wants to merge 1 commit into
morluto:mainfrom
ShuaixinHuang:codex/fix-electron-root-directory
Open

ShuaixinHuang wants to merge 1 commit into
morluto:mainfrom
ShuaixinHuang:codex/fix-electron-root-directory

Conversation

@ShuaixinHuang

@ShuaixinHuang ShuaixinHuang commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Problem and outcome

Fixes #8946. An existing regular file supplied as application_root currently passes existence checks and reaches the Electron provider, which can fail with spawn ENOTDIR and provider-oriented recovery. Reject that selection as invalid_request with an issue at ["application_root"] before provider capture.

Change and boundaries

The shared Electron path-admission owner now checks the existing stat result for directory type when admitting application_root. Preserve aggregate invalid-input reporting, EACCES/EPERM host-access handling, canonical paths, explicit directory selection and omitted-root inference. application_path remains file-or-directory compatible; no new launch authority or schema change.

Verification

  • Regression before fix: 1 failed / 1 passed / 7 existing platform skips; the recording provider was reached and returned capability_unavailable instead of the expected input issue.
  • Final focused selection: 6 passed / 9 existing Windows platform skips in two complete files (22.67 s), including real compiled CLI and stdio MCP invalid-root requests. The service regression asserts the recording provider is not called; explicit-directory and omitted-root controls are retained.
  • check:fast: typecheck and lint passed, 2 tasks, 56.168 s.
  • Formatting and diff checks passed; all three remote integrated payloads pass the explicit repository formatter.
  • Full check stopped at knip (four unused exports, two unused types, two duplicate exports), matching the previously reproduced local baseline reports. Subsequent architecture/production-reachability/typed-lint tasks were not reached; full check is not claimed passed.

Scope limits: Windows Node22.20.0/npm10.9.3 with existing dependencies and verified native addon junction; no dependencies installed. CLI/MCP rejected inputs were exercised without launching real Electron. The service uses a recording provider, so this is not a real Electron/provider or Linux/macOS permission verification. Existing permission and symlink skips remain unchanged. No new timeout, resource limit or skip.

Remote commit is based on main 71f680d, with only three files applied using exact-context patches. The complete remote main integration tree was not locally rerun and requires this new PR's own CI. Local isolated worktree c03935f9b preserved the checked bytes after commit.

Compatibility

Corrects input classification and recovery for an invalid application_root. No runtime behavior change for admitted paths, target execution, package metadata, generated contracts or release migration.

Author readiness

Human owner: @ShuaixinHuang

  • 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. The known early-global-call P1 below still needs its published regression/fix.
  • 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.

Current-head P1 correction

Commit 9611e5cbe8d0d3639f87df529d2989e69a63d1f2 covers the classic-script global activation case. Focused domain and CLI/stdio MCP validation passed 67/67 tests; check:fast passed 2/2. The local regression covers early and late globalThis/window/self/this activations without expanding call-graph or authority semantics.

This replacement preserves the prior PR discussion and validation history at #8331. It reuses the verified published head and requires its own hosted CI.

Supersedes #8749.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ⚠️ Failed 2026-11-05T01:57:17.922416Z
We couldn’t complete this review. Comment @codex review to try again.
340de50 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Repo admins can enable using credits for code reviews in their settings.

@greptile-apps

greptile-apps Bot commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The code appears safe to merge; the directory check fixes input classification without changing admitted-path behavior.

Summary

Rejects an existing non-directory application_root in the shared application service before provider capture.

  • Electron captures reject a file selected as the application root.

Reviews (1) · Last reviewed commit: "fix(electron): reject non-directory scen..." · Reviewed by Greptile

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] capture-electron-scenario treats a regular-file application_root as a provider failure

1 participant