Remove quarantine attributes from App Store bundles

[delano]

Summary

• Remove com.apple.quarantine from the assembled App Store app bundle after embedding the provisioning profile, without modifying the source profile or following symbolic links out of the bundle.
• Refuse App Store packaging when quarantine attributes remain in the signed bundle, printing only the quarantined paths. App Store Connect rejects such an upload with ITMS-91109; Apple's wording, as quoted on the Apple Developer Forums, is cited in docs/development/testflight-distribution.md.
• Add macOS integration coverage for cleanup, verification, repeatability, symbolic links, source-profile preservation, and command failures, plus layout tests (also run on Linux) pinning cleanup ordering and App Store lane scoping.

Also in this branch

• shell/Sources/CompanionKit/TimeRailView.swift: marks width and experimentalWidth nonisolated so nonisolated static func width(experimental:) can read them. Unrelated to quarantine; CI on main passes without it. Its commit (72a355a) has a malformed subject: "Fix", with the intended subject on the second line.

Review guide

• Start with scripts/package-app.sh: cleanup is limited to the App Store lane and runs after the final profile copy; the prepackage check sits with the other bundle content checks, before the signature checks.
• Review scripts/tests/test_app_store_quarantine.py for real-xattr coverage and the layout tests.

Validation

• python3 -m unittest discover -s scripts/tests (65 tests)

[claude]

Claude finished @delano's task —— View job

---

Code review: remove quarantine attributes from App Store bundles

• Read repo conventions (AGENTS.md) and CI wiring
• Review scripts/package-app.sh (cleanup + prepackage check)
• Review scripts/tests/test_app_store_quarantine.py
• Review docs/development/testflight-distribution.md
• Check reachability/ordering invariants against the rest of the script
• Post review

Verdict: approve-with-nits. The change is correct, tightly scoped, and the ordering claims in the PR description hold up against the script. No blocking issues. Notes below are mostly about the durability of the tests and two stale items in the PR description.

---

What I verified

1. The cleanup is always reached in the App Store lane. It sits inside the elif [[ -n "$PROVISIONING_PROFILE" ]] arm (scripts/package-app.sh:519), and validate_signing_configuration already refuses App Store mode without CODESIGN_IDENTITY (scripts/package-app.sh:132), without PROVISIONING_PROFILE (scripts/package-app.sh:148), and without a readable TEAM_ID (scripts/package-app.sh:158). So the three arms that would skip cleanup are unreachable with --app-store, and the prepackage check cannot fail for lack of a cleanup run.
2. Ordering is right in both directions. Cleanup (scripts/package-app.sh:534) runs after the final profile copy (:524) and before codesign (:556). That matters: stripping xattrs after signing is the version of this fix that produces mysterious signature-validity questions later. It is also after the pre-signature digest (:490), which hashes file content only, so the digest is unaffected.
3. Dangling and outbound symlinks are handled. xattr -drs acting on links themselves is the detail that makes this safe, and test_symbolic_links_are_cleaned_but_never_followed (scripts/tests/test_app_store_quarantine.py:135) covers both the outbound link (target attribute preserved) and a dangling link (recursion does not abort). That is the failure mode most likely to bite in a real bundle.
4. Fail-closed on read errors. APP_XATTRS="$(xattr -rs "$APP")" (:590) is a bare assignment, so its substitution's exit status propagates under set -euo pipefail (:52); test_prepackage_attribute_read_failure_stops_packaging pins that. The check also sits before productbuild (:648), so a rejection cannot leave a stale signed .pkg behind.
5. Diagnostics are scoped. The grep -E ': com\.apple\.quarantine$' re-run prints only offending paths, and test_prepackage_check_rejects_quarantine_anywhere_in_bundle asserts an unrelated attribute is not echoed. Good instinct — this output goes into release logs.
6. CI really runs the macOS half. Discovery runs in both the ubuntu logic job (.github/workflows/ci.yml:33) and the release packaging (macos) job (:190), so the seven skipUnless(darwin) tests are not developer-machine-only. The temp directory prefix with spaces (test_app_store_quarantine.py:67) also quietly exercises quoting.

Nits and suggestions (non-blocking)

1. Two test names promise more than they check.
   • test_nothing_writes_into_the_bundle_after_cleanup (:47) relies on BUNDLE_WRITE (:17), which matches a fixed command list (cp|ditto|install|ln|mv|rsync|tee) and only arguments written literally as "$APP/… or "$APP". A later plutil -replace … "$APP/Contents/Info.plist", a PlistBuddy edit, or a copy through an intermediate variable (DEST="$APP/Contents"; cp x "$DEST") all pass silently. None of those import a downloader's attribute, so the guard still covers the real hazard, but consider adding plutil|PlistBuddy to the alternation or renaming to something like test_no_file_copies_into_the_bundle_after_cleanup.
   • test_prepackage_check_applies_to_the_app_store_lane_only (:54) matches only column-zero if|elif|else|fi, so it proves the check is inside the top-level if ((APP_STORE_MODE)) block but cannot tell whether it is further nested in an indented conditional inside it. Fine as a smoke test; the name reads as a stronger guarantee.
2. The fragment anchors are invisible from the script side. The tests slice package-app.sh on exact text including two-space indentation (EMBED, CHECK, " SIGNED_BUNDLE_ID=", "codesign --force --options runtime", "productbuild --component") and then execute the EMBED → SIGNED_BUNDLE_ID range under bash -euc. The failure mode is loud (locate raises on a changed or duplicated marker), which is the right trade, but someone reformatting that region gets a puzzling AssertionError from a file they did not touch — and any security/network call added into that range would start being executed by the suite. A one-line comment near :524 and :590 saying these boundaries are anchors for scripts/tests/test_app_store_quarantine.py would pay for itself.
3. Scope of the attribute set. Only com.apple.quarantine is removed and checked, which matches the cited ITMS-91109 wording exactly — correct discipline, and I would not broaden it speculatively. Worth knowing that resource-fork/com.apple.FinderInfo rejections are a separate class the manual xattr -rs listing in the docs will now surface anyway.
4. Optional: the other lanes. Leaving dev/local copies untouched is defensible (nothing uploads them), and the comment says so. If a quarantined embedded profile ever shows up as a Gatekeeper oddity in a local install, the same two lines would move up one level.
5. Docs read well and respect AGENTS.md. The forum citation is labelled as quoted-on-the-forums rather than attributed directly to Apple, and the man cp -X reading is presented as the mechanism rather than as Apple's wording. The closing point — that removing attributes after the fact does not update an already-built .pkg — is the part a reader will actually need at 2am.

Two stale items in the PR description

• The "Also in this branch" section describes shell/Sources/CompanionKit/TimeRailView.swift, but d3982cd/029cc9a have since landed on main via Fix time rail actor isolation and release build stripping #239, and the PR's file list is now three files. The Swift change is no longer a net diff here.
• The malformed subject on 72a355a ("Fix fix(companion-kit): mark time rail widths nonisolated") still exists in this branch's history. A squash merge keeps it out of main's log; a merge commit does not.

I did not run the suite — python3 -m unittest discover -s scripts/tests is outside my allowed tools in this job, and the seven integration tests need macOS regardless. The release packaging (macos) CI job covers both halves.
· Branch: fix/app-store-quarantine

[greptile-apps]

Confidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable issues were found.

_{Reviews (1) · Last reviewed commit: "docs(testflight): cite the ITMS-91109 wo..." · Reviewed by Greptile}

[Copilot]

🟢 Approval recommended

The cleanup is correctly scoped and ordered, with comprehensive automated coverage.

0 open findings

What changed in this PR

Removes quarantine attributes from App Store bundles and verifies they are absent before packaging.

Changes:

• Adds scoped quarantine cleanup and validation.
• Adds macOS integration and layout tests.
• Fixes TimeRailView static property isolation.

File	Description
scripts/​package-app.sh	Cleans and validates App Store bundles.
scripts/​tests/​test_app_store_quarantine.py	Tests cleanup, failures, symlinks, and ordering.
docs/​development/​testflight-distribution.md	Documents quarantine handling.
shell/​Sources/​CompanionKit/​TimeRailView.swift	Marks width constants nonisolated.

🧠 Review effort: Balanced

---

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