Skip to content

0020: app: electron: Report rejected command exits - #7294

Draft
illume wants to merge 3 commits into
kubernetes-sigs:mainfrom
illume:upstream-0020-invalid-command-exit
Draft

0020: app: electron: Report rejected command exits#7294
illume wants to merge 3 commits into
kubernetes-sigs:mainfrom
illume:upstream-0020-invalid-command-exit

Conversation

@illume

@illume illume commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Summary

Report command-exit events when desktop command execution is rejected before
spawning a child process:

  • -1 for invalid command data
  • -2 for rejected permission secrets
  • -3 for denied command consent

This prevents renderer-side command objects from remaining pending when the
main process rejects a request.

Source

This upstreams the command-rejection portion of:

The change was later rebased in the downstream branch as:

Patch 0020 was extracted while preparing the Headlamp source package in:

Improvements over the existing changes

  • Adds focused unit coverage before the behavior change and raises
    app/electron/runCmd.ts branch coverage to 93.45%. This protects the wider
    command lifecycle rather than only the newly added lines.
  • Adds an Electron end-to-end test for the renderer-to-main IPC path. This
    verifies that invalid commands are rejected by the real desktop bridge and
    that the renderer receives exit code -1.
  • Only reports invalid-data exits when the untrusted payload contains a string
    command ID. This avoids reflecting malformed identifiers back over IPC.
  • Keeps the three rejection causes distinct so callers can diagnose why a
    command did not start.

Backwards compatibility

For users of the downstream patch code, rejected commands now complete with a
negative command-exit code instead of remaining pending. Callers that used a
timeout or the absence of an exit event to infer rejection should migrate.
Callers should handle rejection codes -1, -2, and -3 explicitly. Existing
listeners already accept numeric exit codes, so no API-shape change is required.
Command IDs must be non-empty strings; malformed IDs are rejected without being
reflected over IPC.

For users of Headlamp main, valid commands are unaffected. Command validation,
permission checks, consent prompts, process spawning, output forwarding, and
real child-process exit codes retain their existing behavior. The change is
additive. It only affects requests that the main process rejects before spawn.
Those requests now terminate predictably instead of remaining unresolved.
Choosing Deny on the first consent prompt now rejects immediately, matching
the behavior of a denial already stored in settings.

Testing

  • npm --prefix app run tsc
  • ./node_modules/.bin/vitest run electron/runCmd.test.ts (48 tests)
  • Focused Istanbul coverage for app/electron/runCmd.ts (93.45% branches)
  • npm --prefix app/e2e-tests run test-app -- tests/runCommand.spec.ts

The behavior is not visual, so screenshots are not applicable.

Assisted by copilot.

@kubernetes-prow kubernetes-prow Bot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 16, 2026
@kubernetes-prow
kubernetes-prow Bot requested review from kahirokunn and sniok August 16, 2026 15:55
@kubernetes-prow kubernetes-prow Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 16, 2026
@kubernetes-prow

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: illume

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubernetes-prow kubernetes-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 16, 2026
@illume
illume force-pushed the upstream-0020-invalid-command-exit branch from b71b98b to 9459619 Compare August 16, 2026 15:58
@illume
illume requested a balanced review from Copilot August 16, 2026 19:03
@illume
illume marked this pull request as draft August 16, 2026 19:03
@kubernetes-prow kubernetes-prow Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 16, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds Electron IPC exit reporting for commands rejected before process creation.

Changes:

  • Reports distinct exit codes for validation, permission, and consent failures.
  • Expands command lifecycle unit coverage.
  • Adds an Electron IPC end-to-end rejection test.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
app/electron/runCmd.ts Emits rejection exit events.
app/electron/runCmd.test.ts Adds lifecycle and rejection tests.
app/e2e-tests/tests/runCommand.spec.ts Tests renderer-to-main invalid-command handling.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread app/electron/runCmd.ts
Comment thread app/electron/runCmd.ts
Comment thread app/electron/runCmd.test.ts Outdated
@illume
illume force-pushed the upstream-0020-invalid-command-exit branch from 9459619 to aaa2cd2 Compare August 16, 2026 19:26
illume and others added 3 commits August 16, 2026 21:34
Cover output forwarding and child-process exit handling before changing early
rejection behavior. The shared handler serves both paths, so this baseline
separates regressions in existing command execution from the rejection fix and
shows that successful commands keep their current IPC contract.
Exercise invalid command rejection across the real renderer, preload, and main
process boundary. Unit mocks cannot prove that the desktop bridge delivers the
exit event. A missing event at this boundary leaves plugin callers pending. Use
the minimal app startup path so unrelated backend services cannot make this
focused contract test flaky.
Send an exit event when validation, permission, or consent rejects a command.
Renderer callers rely on that event to settle command state and release IPC
listeners, so returning silently leaves rejected commands pending indefinitely.
Distinct negative codes preserve normal child-process exits while identifying
which pre-spawn gate rejected the request.

Co-authored-by: René Dudfield <renedudfield@microsoft.com>
@illume
illume force-pushed the upstream-0020-invalid-command-exit branch from aaa2cd2 to 4a86998 Compare August 16, 2026 20:04
@illume
illume requested a balanced review from Copilot August 16, 2026 20:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

app/electron/runCmd.ts:283

  • validateCommandData rejects an empty ID, but this branch still reflects it because it checks only the type. That contradicts the stated non-empty-ID contract and sends command-exit for a malformed identifier. Require a non-empty string here (and cover the empty-string handler case), as is already done by validation.
    if (typeof eventData?.id === 'string') {

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants