Skip to content

fix: clean limactl entitlement test#823

Open
Nikhil (shadowfax92) wants to merge 2 commits intodevfrom
fix/lima-vm-clii
Open

fix: clean limactl entitlement test#823
Nikhil (shadowfax92) wants to merge 2 commits intodevfrom
fix/lima-vm-clii

Conversation

@shadowfax92
Copy link
Copy Markdown
Contributor

Summary

  • Removes the redundant optional null-guard from the limactl entitlement regression test.
  • Keeps pyright green by using an explicit expected entitlement filename constant.

Design

PR #822 was squash-merged before this review-nit cleanup commit landed on the branch. This follow-up carries only the test cleanup requested by the AI review; the production signing fix is already merged in #822.

Test plan

  • cd packages/browseros && uv run python -m unittest build.common.server_binaries_test
  • cd packages/browseros && uv run python -m unittest discover -s build -p '*_test.py'
  • cd packages/browseros && uv run ruff check build/common/server_binaries.py build/common/server_binaries_test.py
  • cd packages/browseros && uv run pyright build/common/server_binaries.py build/common/server_binaries_test.py
  • git diff --check

@shadowfax92
Copy link
Copy Markdown
Contributor Author

Claude (@claude) review

@shadowfax92
Copy link
Copy Markdown
Contributor Author

Greptile (@greptileai) review

@claude
Copy link
Copy Markdown

claude Bot commented Apr 26, 2026

Claude finished Nikhil (@shadowfax92)'s task in 1m 1s —— View job


Code Review

  • Gather context and read changed files
  • Review changes for correctness, style, and best practices
  • Post review feedback

Overall: LGTM — clean, purposeful cleanup. The two-commit structure tells a clear story: commit 983faf5 lands the production fix (limactl VZ entitlement), commit 239dcec cleans up the test pattern in response to review feedback. Both are correct.

packages/browseros/build/common/server_binaries.py

-    "limactl": SignSpec("limactl", "runtime"),
+    "limactl": SignSpec("limactl", "runtime", "lima-vz-entitlements.plist"),

Straightforward and correct. The test_every_entitlements_plist_exists_on_disk test will now also gate this registration against the presence of the file on disk — good defence-in-depth.

packages/browseros/build/common/server_binaries_test.py

The cleanup is the right call. The old pattern was awkward:

# Old: assert equality against a literal, then re-assign from spec to satisfy pyright
self.assertEqual(spec.entitlements, "lima-vz-entitlements.plist")
entitlements_name = spec.entitlements  # Optional[str]
assert entitlements_name is not None   # pyright guard

The new pattern is strictly better — the constant is defined upfront as str, so pyright is happy and the null-guard disappears:

entitlements_name = "lima-vz-entitlements.plist"   # str, not Optional[str]
...
self.assertEqual(spec.entitlements, entitlements_name)

assertIs(..., True) on line 44 is the right assertion style for a plistlib-parsed boolean (<true/> → Python True singleton), not just assertTrue which would accept any truthy value.

packages/browseros/resources/entitlements/lima-vz-entitlements.plist

Minimal and correct — only com.apple.security.virtualization, which is exactly what limactl needs for Virtualization.framework. No extra entitlements snuck in.


No issues. Ready to merge.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 26, 2026

Greptile Summary

This PR adds lima-vz-entitlements.plist (granting com.apple.security.virtualization) and wires it into the limactl SignSpec, plus a regression test that validates both the spec entry and the plist key. It is a small, well-scoped follow-up to #822.

Confidence Score: 5/5

Safe to merge — minimal, well-tested cleanup with no logic risk.

Three small, coherent changes: a one-line production fix, a new entitlements file, and a targeted regression test. No dead code, no removed guards, no logic branches that could regress. Test plan is complete and pyright is kept green.

No files require special attention.

Important Files Changed

Filename Overview
packages/browseros/build/common/server_binaries.py Single-line change: adds lima-vz-entitlements.plist as the explicit entitlements plist for limactl; correct and matches the new resource file.
packages/browseros/build/common/server_binaries_test.py Adds test_limactl_uses_vz_entitlement which verifies the spec entry and that the plist contains the virtualization key; assertIs(..., True) is correctly strict for a boolean plist value.
packages/browseros/resources/entitlements/lima-vz-entitlements.plist New minimal plist granting only com.apple.security.virtualization; well-formed and correctly scoped.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[macos_sign_spec_for 'limactl'] --> B[SignSpec: identifier='limactl', options='runtime', entitlements='lima-vz-entitlements.plist']
    B --> C[resources/entitlements/lima-vz-entitlements.plist]
    C --> D[com.apple.security.virtualization = true]
    E[test_limactl_uses_vz_entitlement] -->|asserts spec.entitlements| B
    E -->|loads & asserts key| C
Loading

Reviews (1): Last reviewed commit: "fix: address limactl entitlement test re..." | Re-trigger Greptile

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.

1 participant