build(macos): 校验打包产物声明语音识别用途#825
Conversation
PR Reviewer Guide 🔍(Review updated until commit 68edf79)Here are some key observations to aid the review process:
|
appergb
left a comment
There was a problem hiding this comment.
Senior review — exact head 99edf67f77ca9184884348c8c0f97ba35a3b7df5
Strengths
- 改动很聚焦:相对 base
c645b16851727bb09d3a5274854bc1277da4bd9e只有openless-all/app/scripts/build-mac.sh一行新增,提交历史是0 behind / 1 ahead,无前端、Rust、配置或 UI 改动。 $INFO明确指向src-tauri/target/release/bundle/macos/OpenLess.app/Contents/Info.plist;断言发生在npm run tauri -- build之后,因此检查的是 Tauri 生成的 bundle,而不是源码src-tauri/Info.plist。- 该命令是
set -euo pipefail下的独立简单命令:我在 detached worktree 上执行 exact-head 原始断言,合法字符串返回0且无输出;缺 key 和缺 plist 都返回1,会在签名/安装前中止。bash -n与git diff --check也通过。 - exact head 上 5 个 check runs 均为 success:Android cargo check、macOS checks、Windows checks、Linux checks、PR-Agent。
Critical
- None.
Important
- 当前断言会放过类型错误或空的用途说明。 Apple 将
NSSpeechRecognitionUsageDescription定义为string,它应是展示给用户的用途说明(https://developer.apple.com/documentation/bundleresources/information-property-list/nsspeechrecognitionusagedescription)。`PlistBuddy Print只证明 key 可读取:我的 contract fixture 证明bool true和空字符串都返回0。因此这项“发布产物声明有效语音识别用途”的护栏仍有真实假阳性。请同时断言值为 string 且非空;macOS 12+ 可用plutil -extract NSSpeechRecognitionUsageDescription raw -expect string` 后检查非空,且保持成功路径静默。 - PR 没有提交能证明正/反路径的测试,现有 5 个绿色 checks 也没有运行这条断言。
.github/workflows/ci.yml明确不跑build-mac.sh;PR 描述中的反例只是在已安装 app 上查询另一个不存在的NSCameraUsageDescription,没有删除目标 key、没有执行 exact-head 校验块,也没有证明脚本在后续签名/安装前停止。请增加一个 macOS hermetic contract test:对临时 plist 覆盖合法字符串、缺 key、错误类型/空值,并把它接入macOS checks。
Minor
- #823 的“缺失时不产出可安装
.app”按字面并不成立:检查位于完整tauri build之后,所以失败时 target 中的.app/DMG 已生成,只是脚本非零退出、不会安装或由 release job 上传。建议修正文案,或在断言失败时清理无效产物,避免失败构建留下可误用的 stale artifact。 - 该行检查的是最终 macOS
.appbundle;它不直接挂载并复查 DMG 内副本。不过这符合 #823 指定的.app/Contents/Info.plist范围。
Ready to merge verdict
COMMENTED — logical REQUEST_CHANGES. 方向正确、缺 key 的主要故障已被拦住、无 UI 回归面;但在“不会误过”与可重复测试证据两个核心门槛上仍未闭环。补齐 string/non-empty 断言与接入 CI 的正反 contract test 后可重新审核。
|
Persistent review updated to latest commit fd103da |
appergb
left a comment
There was a problem hiding this comment.
Reviewed exact head fd103da7476896a8eefdbe3216136f884092202f against base c645b16851727bb09d3a5274854bc1277da4bd9e, including issue #823 and the prior exact-head review 4700966101.
Strengths
- The two previous Important findings are substantially addressed:
/usr/bin/plutil -expect stringrejects non-string values, and the real helper is now exercised by a macOS-only hermetic contract covering a valid string, missing key, empty string, ASCII whitespace-only string, bool, and missing file. build-mac.shcalls that same helper against the final bundle plist aftertauri build; the valid path is silent, the plist path is quoted and separated with--, and an injection-shaped filename with spaces/metacharacters remained safe in my direct test.- Package and CI wiring is focused and correct. The exact-head macOS job executed the new npm contract successfully. The release workflow's two macOS matrix entries both pass through the shared
Build (macOS)step andINSTALL=0 bash scripts/build-mac.sh. - Scope stayed limited to five build/test/CI files. There are no React/TypeScript UI, Rust runtime, dependency, or application configuration changes.
Critical
None.
Important
-
Blocking — whitespace validation is locale-dependent and still accepts whitespace-only strings under a valid build locale. At
openless-all/app/scripts/check-macos-speech-usage-description.sh:17, Bash[:space:]followsLC_CTYPE. With the same plist string, I measured:- U+00A0 NBSP:
LC_ALL=C→ exit 0;LC_ALL=en_US.UTF-8→ exit 1 - U+2003 EM SPACE:
LC_ALL=C→ exit 0; UTF-8 locale → exit 1 - U+3000 IDEOGRAPHIC SPACE:
LC_ALL=C→ exit 0; UTF-8 locale → exit 1
These are visually blank descriptions, so the promised “trim then require non-empty” invariant is not deterministic. The current contract only uses ASCII spaces/newline/tab and itself passes under
LC_ALL=C, so it misses this false-positive path. Please make trimming/checking locale-independent and add a contract case that runs underLC_ALL=Cwith Unicode whitespace-only input. - U+00A0 NBSP:
Minor
- The PR body should scope its release-failure wording more precisely. The helper runs after
npm run tauri -- build, so Tauri may already have built and signed/notarized the app/DMG; it stops the later codesign verification, local installation, and macOS upload/release steps. Also,release-tauri.ymlusesfail-fast: false, so Windows/Linux matrix jobs may still publish a partial release even when both macOS jobs fail. The invalid macOS artifact is protected from upload, but the overall release workflow does not completely stop.
Verification
npm run check:macos-speech-usage-descriptionunder default,LC_ALL=C, andLC_ALL=zh_CN.UTF-8— existing contract passed- Direct source plist check — exit 0 with 0 output bytes
- Injection-shaped/space-containing plist path — exit 0 with 0 output bytes and no side effect
bash -nfor both shell scripts;node --checkfor the contract — passed- Package script assertion,
plutil -lint, YAML syntax parse for CI/release workflows, andgit diff --check— passed - Exact-head checks at submission: Android, macOS, Linux, and PR-Agent succeeded; Windows was still in progress
Ready verdict
Not ready; logical REQUEST_CHANGES. The prior type/coverage blockers are fixed, but the locale-sensitive trim still leaves a real whitespace-only false positive. Re-review after the checker and contract are deterministic across locales. Do not merge on this head.
build-mac.sh 已在校验 NSMicrophoneUsageDescription 与 audio-input entitlement,但 Apple Speech provider 引入的 NSSpeechRecognitionUsage- Description 没有对应护栏。该 key 一旦缺失,构建静默成功,而选择 apple-speech 的用户会在调用 Speech.framework 时被 macOS TCC 以 SIGABRT 直接杀掉进程(崩溃报告 termination namespace = TCC)。 对打包产物 .app/Contents/Info.plist(而非源码 Info.plist)新增一行 PlistBuddy 校验:脚本开头的 set -euo pipefail 会让 key 缺失时非零退出、 当场中止构建。key 存在时输出被丢弃,行为与现在完全一致,产物逐字节不变。 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fd103da to
68edf79
Compare
|
Persistent review updated to latest commit 68edf79 |
appergb
left a comment
There was a problem hiding this comment.
Logical verdict: APPROVE
Re-reviewed exact head 68edf799d74f77efab0ce802a7632ceec74156c1 against live remote beta 07b2e6a141d0f110954b1f79d58dd79c7cc500e7.
Findings:
- Critical: none.
- Important: none.
- Minor: none.
The prior Unicode-whitespace blocker is closed. The checker now uses ECMAScript String.prototype.trim(), whose whitespace semantics are locale-independent, instead of Bash [:space:]. An independent matrix produced these exact outcomes:
- normal non-empty string: exit 0, stdout 0 bytes, stderr 0 bytes;
- missing key, empty string, bool/wrong type, ASCII whitespace-only, and missing plist: nonzero;
- NBSP U+00A0, EM SPACE U+2003, and IDEOGRAPHIC SPACE U+3000 under
LC_ALL=C LANG=C: all nonzero.
/usr/bin/plutil -expect string remains the type gate, so non-string values fail before the trim check.
Packaged-app and portability review:
build-mac.shruns the real checker againstOpenLess.app/Contents/Info.plistimmediately afternpm run tauri -- buildand before codesign verification, quarantine handling, or local installation, underset -euo pipefail.- An instrumented execution of the exact script with a generated final-bundle plist containing U+2003 exited 1 and never reached a codesign sentinel, directly proving fail-fast control flow.
- Node is not a new deployment/runtime dependency: the same build script already requires Node through the earlier npm/Tauri command, CI installs Node 22, and release installs Node 20 before both macOS matrix entries call
INSTALL=0 bash scripts/build-mac.sh. If Node is unexpectedly unavailable, the checker fails closed before codesign/install. - Both release macOS architectures use the shared build script. The updated PR wording correctly limits the guarantee to the macOS artifact path and acknowledges
fail-fast: falsefor other platforms.
Current-beta merge and verification:
- The GitHub PR API was still showing the prior cached base OID at submission, so I used the authoritative live remote
refs/heads/betaand independently recomputed the merge tree. - Clean live-beta merge tree:
2128febfd6e05149929c3dd2bb3a45f4eb07219e. - Relative to live beta, it changes exactly the five reviewed CI/build/test files. It preserves the merged provider-redaction backend and PIN persistence contract; no React/TypeScript UI, Rust runtime, lockfile, dependency, or application configuration path changes.
- On the synthetic live-beta merge, the C-locale speech contract, PIN persistence contract, macOS capsule contract, and production frontend build all passed.
npm audit --audit-level=highreports 0 vulnerabilities.cargo auditreports 0 vulnerabilities with the repository existing 18 allowed warnings. Shell/Node syntax, plist lint, CI/release YAML parsing, exact and mergedgit diff --check, and gitleaks all pass.- All five head-tied checks are green: PR Agent, Android, macOS, Windows, and Linux. The macOS job executes the new speech contract; all three desktop platforms execute the merged PIN contract.
No blocking or non-blocking correctness finding remains.
User description
摘要
Fixes #823。
macOS 最终
.app/Contents/Info.plist中的NSSpeechRecognitionUsageDescription现在必须是 string,且按 ECMAScript Unicode whitespace 规则 trim 后非空;否则build-mac.sh非零退出,阻止后续 codesign 验证、本地安装以及该 macOS artifact 的上传/发布。修复内容
scripts/check-macos-speech-usage-description.sh:使用系统/usr/bin/plutil -extract ... -expect string保留 plist 类型校验,并用 locale-independent 的 ECMAScripttrim()拒绝空字符串或仅含 Unicode 空白的字符串。scripts/build-mac.sh在 Tauri 完成 bundle 后,对最终OpenLess.app/Contents/Info.plist调用真实 checker;合法路径不产生额外输出。LC_ALL=C下仅含 NBSP(U+00A0)、EM SPACE(U+2003)或 IDEOGRAPHIC SPACE(U+3000)的 string;macOS checks;测试不依赖完整 Tauri 打包。Release 路径复核
release-tauri.yml当前并非另一条独立 macOS 打包路径:两种 macOS runner 的Build (macOS)step 都以INSTALL=0调用bash scripts/build-mac.sh。因此本次 checker 同时保护本地打包和 macOS release workflow。行为与范围
tauri build之后:失败时 target 目录中可能已经存在未验证的.app/DMG,但脚本会在后续 codesign 验证和本地安装前非零退出;对应 macOS matrix 不会上传/发布该 artifact。fail-fast: false下其他平台 matrix 可继续,整体 workflow 不保证完全停止。这里不再声称“失败时完全不生成 bundle”。验证
TDD RED:在旧 checker 上先加入 C locale Unicode fixtures;NBSP-only fixture 明确出现
expected non-zero exit,证明原来的 Bash whitespace 判断可被绕过。GREEN / 回归检查:
LC_ALL=C LANG=C npm run check:macos-speech-usage-description— 通过,覆盖 U+00A0/U+2003/U+3000;npm run check:macos-capsule-spaces— 通过;bash -n scripts/build-mac.sh scripts/check-macos-speech-usage-description.sh— 通过;node --check scripts/macos-speech-usage-description-contract.test.mjs— 通过;src-tauri/Info.plist— 退出 0 且静默;git diff --check— 通过;release-tauri.ymlmacOS step 调用build-mac.sh— 已核对。完整
INSTALL=0 bash scripts/build-mac.sh已验证通过,真实执行 checker、生成.app/DMG,并通过 codesign 与 quarantine 检查;contract test 仍保持 hermetic,不依赖打包产物。最新beta精确基线上另通过全部合约、生产构建及 687 个 Rust lib tests。PR Type
Bug fix, Enhancement, Tests
Description
Add checker for NSSpeechRecognitionUsageDescription in build-mac.sh
Contract tests for valid/invalid plist variants
Add CI step for macOS speech usage description check
Diagram Walkthrough
File Walkthrough
build-mac.sh
Integrate speech usage description checkeropenless-all/app/scripts/build-mac.sh
check-macos-speech-usage-description.shafter Tauribuild
$INFOvariable pathcheck-macos-speech-usage-description.sh
Add speech usage description validation scriptopenless-all/app/scripts/check-macos-speech-usage-description.sh
macos-speech-usage-description-contract.test.mjs
Contract tests for speech description checkeropenless-all/app/scripts/macos-speech-usage-description-contract.test.mjs
package.json
Add npm script for contract testopenless-all/app/package.json
ci.yml
Add macOS speech check to CI.github/workflows/ci.yml