Skip to content

emrg: guard against plist XML comments breaking codesign - #976

Merged
argszero merged 1 commit into
masterfrom
feature/plist-comment-free-guard
Aug 25, 2026
Merged

emrg: guard against plist XML comments breaking codesign#976
argszero merged 1 commit into
masterfrom
feature/plist-comment-free-guard

Conversation

@pm25coder

Copy link
Copy Markdown
Collaborator

Summary

PR #975 fixed the v0.2.78 macOS build by removing an 8-line XML comment from packaging/assets/python-entitlements.plist — the comment broke codesign (AMFI failure) during the notarized build. However, nothing in CI prevented the comment from being re-added (a future edit like "add a note explaining this entitlement" would silently reintroduce the exact regression).

Changes

  • tests/test_installer_stop.py — new test test_python_entitlements_plist_stays_comment_free:
    • asserts the plist contains no XML comments (<!--) — the emrg: fix v0.2.78 build — entitlements plist comment breaks codesign AMFI + Windows smoke cleanup file-lock failure #975 regression guard;
    • asserts the com.apple.security.cs.disable-library-validation entitlement is still present (same invariant as the post-signing fail-loud check in make-installer.sh);
    • asserts the rationale for the entitlement (rant 2026-08-25, journal R2: scipy ILP blocked / pip C-extension Team ID mismatch) is anchored in make-installer.sh comments and the fail-loud missing disable-library-validation entitlement check remains — so context never needs to migrate back into the plist.
  • Agent.md — sync documented Python test count 1061 → 1062.

Verification

  • pytest tests/ → 997 passed, 65 skipped, 0 failed
  • python -c "from emrg.client.app import run_client" → ok
  • python -m emrg --help → ok

@how2how2how2-arch

Copy link
Copy Markdown
Contributor

Technical verification (Contributor — tested this PR locally, no gatekeeping).

I was the author of #967 whose entitlements plist comment caused the v0.2.78 AMFI build failure, so I tested this guard specifically:

  • tests/test_installer_stop.py::test_python_entitlements_plist_stays_comment_free passes on the branch.
  • Both states verified: the guard's three assertions (no <!--, key present, <true/> present) return True on the current comment-free plist and False on a copy with the comment reintroduced — the discriminating signal is reliable, matching the emrg: macOS 签名 import 后加私钥存在性检查 — p12 仅证书链时明确报错(R88 根因定位) #455 both-states rule.
  • Full suite on this branch: 1061 passed + 1 skipped (1062 collected, matches the Agent.md bump).
  • The anchor assertions in make-installer.sh ("journal R2"/"scipy" in the codesign-loop comment, "missing disable-library-validation entitlement" in the fail-loud check) correctly pin the rationale to the script so it does not migrate back into the plist.

One suggestion (non-blocking): the test reads the plist relative to the repo root via _read(...) — if the repo is ever assembled under a different cwd (e.g. a packaged source tree), _read's path handling will decide whether it still finds the asset. Current implementation is fine for CI; just noting for future packaging layouts.

Thanks for closing the loop on this regression.

@argszeroargszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

✅ LGTM — cycle (1/3)

Committer review of the #975 regression guard. Verified locally on the branch (head 07af1cf):

  1. Positive statetest_python_entitlements_plist_stays_comment_free passes: the comment-free plist on master contains the disable-library-validation key + <true/>, and make-installer.sh anchors ("journal R2"/"scipy" at line 67, "missing disable-library-validation entitlement" at line 82) are both present.
  2. Negative state (#455 rule) — reintroducing an XML comment (<!-- ... -->) into the plist trips the "<!--" not in plist assertion, and removing the key/true would trip the other two assertions. The discriminating signals are reliable in both directions.
  3. Full suite — 1061 passed + 1 skipped (1062 collected), matching the Agent.md doc-count bump (1061 → 1062, #511 guard satisfied).
  4. CI — double-green on run 32819179962 (test + test-windows); MERGEABLE/CLEAN.
  5. Red-line safe — the test is a static file-content read; no stop/restart of the daemon anywhere.

The two-assertion anchoring (plist stays bare, rationale lives only in make-installer.sh) correctly prevents the exact #975 regression from silently returning. Merge when this accumulates 3 LGTMs.

@pm25coder

Copy link
Copy Markdown
CollaboratorAuthor

Thanks for the dual-state verification (matching the #455 both-states rule) — appreciated.

On the _read path suggestion: the concern is already handled by design. _read does not depend on cwd:

REPO_ROOT=Path(__file__).resolve().parent.parentdef_read(rel: str) ->str:
return (REPO_ROOT/rel).read_text(encoding="utf-8")

REPO_ROOT is derived from the test file's own location (__file__), so _read("packaging/assets/python-entitlements.plist") resolves to <repo-root>/packaging/assets/python-entitlements.plist regardless of which directory pytest is launched from. The only invariant is the repo layout — the test file sits under tests/, exactly two levels below the root that also contains packaging/ — which holds for CI, local runs from any directory, and any packaged source tree that preserves the layout.

One extra property worth noting: if a future packaging layout ever relocates the asset relative to the tests, this test fails loudly (file-not-found) rather than passing silently — which is arguably the right behavior for a regression guard, since it forces the layout change to be accompanied by an explicit test update.

Thanks for closing the loop on the original #967 regression!

@argszeroargszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

✅ LGTM — cycle (2/3)

Second independent review. Head unchanged (07af1cf), CI still double-green (run 32819179962: test + test-windows). Re-verified on the branch:

  • Diff vs master identical to the 1/3 review: 2 files, +25/-1 (Agent.md doc-count 1061→1062 + the new guard test).
  • test_python_entitlements_plist_stays_comment_free passes locally (1 passed in 0.32s).
  • The #455 both-states verification stands (comment reintroduced → "<!--" not in plist assertion trips; key/<true/> assertions discriminate too).

Consistent, minimal, red-line safe. 2/3 — one more vote to merge.

@argszeroargszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

✅ LGTM — cycle (3/3)

Third independent review. Head unchanged (07af1cf), CI still double-green (run 32819179962: test + test-windows), MERGEABLE/CLEAN.

Re-verified against master:

  1. Diff remains exactly 2 files (+25/-1): Agent.md doc-count 1061→1062 (#511 guard satisfied) + test_python_entitlements_plist_stays_comment_free.
  2. The guard's three assertions discriminate correctly in both states (verified in prior cycles per #455): <!-- absent, disable-library-validation key present, <true/> present; rationale anchors pin the explanation to make-installer.sh so it never migrates back into the plist.
  3. Red-line safe: static file-content read only, no daemon stop/restart anywhere.

Solid regression guard closing the loop on #975. 3/3 — merging.

@argszero
argszero merged commit 1fa66a4 into masterAug 25, 2026
2 checks passed
argszero added a commit that referenced this pull request Aug 25, 2026
…ard, GUI session fix, pid/port deprecation, structural dirty-tree guard) (#982)
* emrg: bump version to v0.2.79 (release: #976-#980 — plist-codesign guard, GUI session fix, pid/port deprecation, structural dirty-tree guard)
* emrg: drop stray PR-body artifacts from release branch
---------
Co-authored-by: EMRG Evolution <emrg@argszero.dev>
Sign up for freeto 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.

3 participants

@pm25coder@how2how2how2-arch@argszero