Skip to content

fix(shell): retire approvals for disabled components - #1023

Open
hezhizhen wants to merge 2 commits into
OmniNull:mainfrom
hezhizhen:codex/fix-disabled-component-approvals
Open

hezhizhen wants to merge 2 commits into
OmniNull:mainfrom
hezhizhen:codex/fix-disabled-component-approvals

Conversation

@hezhizhen

@hezhizhen hezhizhen commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Disabling the last enabled instance of a component leaves its pending permission prompt open, even after the runtime has disabled the instance. The host still counts declarations retained for the disabled component.

 permission declarations
- all component descriptors and configured instance overrides
+ descriptors with enabled instances, and those instances' overrides

Use the same enabled-instance declarations when admitting and pruning HTTP and native-program approvals. Keep shared prompts while another instance remains enabled, and retire network hosts added only by a disabled instance. Late deliveries and canceled prompt callbacks are covered.

Evidence

  • Before: a temporary component loaded through the real source loader and Luau HTTP path left enabled=0, pending=1, visible windows=1 after disabling. Deleting the instance dismissed the prompt.
    After: the same native-panel reproduction, linked against the fix, produced enabled=0, pending=0, visible windows=0 in two consecutive runs.
  • Four new loader/package regression tests: 20 failing assertions before the fix → all pass. Cover HTTP disable/re-enable, program trust prompts, enabled siblings, instance-specific hosts, late approvals, and stale answers. Two existing registry fixtures now configure the enabled instances their descriptors represent.
  • 159 related approval, HTTP, and provider tests pass.
  • Full serial swift test: 7,329 tests, 58 skipped, 5 failing cases / 30 assertions in focus transfer, island input, the Lake reference fixture, and window-close focus. Rebuilding the same worktree with the byte-identical upstream permission implementation reproduces the same 5 cases and 30 assertions in a 39-test control run. These failures also occur without this fix.
  • make format passes. make verify passes formatting and lint (18 existing warnings), then stops at the existing GhosttyKit digest mismatch: checkout expects e248aca3…, installed archive is dd856d4d…. Tests build and run with that existing local framework.

Merge Danger

Door: two-way
Blast Radius: component permission prompts

Summary by CodeRabbit

  • Bug Fixes
    • Network permission prompts now reflect enabled components. Disabling a component removes permission requests and approvals it no longer needs, while prompts remain available when an enabled sibling still requires the shared permission. This keeps network access decisions aligned with the components currently active in your island.

Limit component permission declarations to enabled instances when admitting and pruning HTTP and program approvals. Cover real component loading, disable/re-enable, shared instances, per-instance hosts, late deliveries, and canceled answers.
@coderabbitai

coderabbitai Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 82e3a5ba-d87b-46cd-9cfa-876f69bf9d2a
Commits

Reviewing files that changed from the base of the PR and between d738784 and c705b01.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 9a3a4945-ca09-4352-84dd-31969bd9adc1
Commits

Reviewing files that changed from the base of the PR and between ea87db9 and d738784.

Files selected for processing: 4
  • Sources/OmniWM/Core/Shell/ShellWidgetHost+Permissions.swift
  • Tests/OmniWMTests/ShellDisabledComponentApprovalTests.swift
  • Tests/OmniWMTests/ShellNetworkPermissionTests.swift
  • Tests/OmniWMTests/ShellPermissionPromptTests.swift

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review.

Walkthrough

Approval declarations now include network capabilities from enabled island components that match a widget descriptor. Tests cover prompt behavior when components are disabled or re-enabled, and update host island setup in existing permission tests.

Changes: Component Approval Declarations

Layer / File(s) Summary
Filter declarations by enabled components
Sources/OmniWM/Core/Shell/ShellWidgetHost+Permissions.swift, Tests/OmniWMTests/ShellDisabledComponentApprovalTests.swift, Tests/OmniWMTests/ShellNetworkPermissionTests.swift, Tests/OmniWMTests/ShellPermissionPromptTests.swift
approvalDeclarations() excludes network capabilities from disabled matching components. New tests cover prompt dismissal, late approval callbacks, re-enabling, shared permissions, and extra hosts. Existing host-registry tests configure island components.

Priority: Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: barutsrb

Merge Risk: Minimal · previous review d7387

The approval behavior appears consistent with enabled component instances. No merge-blocking issue was identified; merge after normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes retiring approvals for disabled shell components, which is the main change.
Description check Passed The description provides the problem, user-facing result, verification evidence, test coverage, and known unrelated failures. It uses different headings from the template, but it contains the required…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description provides the problem, user-facing result, verification evidence, test coverage, and known unrelated failures. It uses different headings from the template, but it contains the required information and is mostly complete.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 4 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches: 💡 1
🧪 Generate unit tests (beta)
  • Create a new PR
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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.

2 participants