Repository navigation
fix(launchd): 本机 npm test 会打死外挂——沙箱调用不得操作真实自启作业 - #10
Conversation
真机故障:动不动就「无法连接本机 controller(外挂未运行?):Failed to fetch」。 根因是 launchd.cjs 的 uninstall():LABEL 是模块常量,plistPath(o.dir) 尊重 dir 覆盖, 但 bootout 硬编码 `gui/<uid>/local.zcode.advisor`,完全忽略 dir。于是 test/launchd.test.js:180/186 传沙箱 dir 调用 uninstall 时,bootout 打的是用户**真实** 的自启作业。launchctl bootout 会终止该作业的进程并把它从 launchd 卸载,KeepAlive 监督随作业一起消失 —— 进程被杀且**永远不会自动重启**,直到用户重开一次 App。 所以本机每跑一次 `npm test`,外挂就被确定性打死一次(不是「动不动」,是必死); 用户重开 App 后又能用,下次测试再打死,表现为间歇性。CI 结构性看不见: test job 跑在 ubuntu-latest,/bin/launchctl 不存在 → ENOENT 被 `catch (_)` 吞掉。 修复: - 新增 realPlistPath()/managesRealAgent(p):以**路径**判定目标是否为用户真实那份 (而非「有没有传 dir」,这样显式指向真实目录或经 ZCODE_ADVISOR_LAUNCHD_DIR 重定向都能正确识别)。 - 新增 canTouchLaunchd(p):仅 darwin + 有 uid + 目标为真实 plist 时才碰 launchd 域。 显式判断取代「抛 ENOENT 再静默吞掉」——后者正是本 bug 在 CI 无法暴露的原因。 - uninstall()/status()/ensureLoaded() 三处同类缺陷一并收口;status() 增加 queried 字段,区分「没查过」与「查过、没加载」,不再把真实作业的态冒充沙箱 plist 的态 (装了 agent 的开发机上「已写入但未加载」断言必红)。 顺带修复生产日志污染:controller.cjs:135 在 require 期固化 LOG_FILE,6 个测试 require controller 却未隔离该 env,导致测试噪音(startApi 的「本机 API 就绪」、 saveUserConfig 的 0.2.17 迁移行)写进用户真实日志,把真实故障淹没、并让「外挂从未 启动」被误读成「外挂反复启动」。 回归防线: - test/launchd.test.js 用子进程打桩 launchctl,断言沙箱 uninstall 调用**零次** launchctl(已实测:对修复前代码该用例失败,对修复后通过)。 - 反向断言真实路径仍会 bootout(防止把卸载功能修坏)。 - test/static-guards.test.js 新增守卫:require controller 的测试必须隔离 ZCODE_ADVISOR_COMPANION_LOG。 验证:npm test 531 项全绿(530 pass / 1 既有 macOS-only skip);在真实 agent 运行时跑完整套件,进程 pid 前后不变;生产日志增量为 0。
ocr 复审意见的修复(上一提交 773134d 的后续): 1. [high/bug] managesRealAgent 只做 path.resolve,不解析符号链接。 macOS 上 /var → /private/var、/tmp → /private/tmp 都是链接:经这些路径 传进来的「真实 plist」会被判成沙箱,于是 install/uninstall/status 静默 跳过 launchd 域操作——卸载与自启安装这两个功能本身失效。 改为 canonicalPath():优先 realpath 父目录(plist 可能尚未落盘,父目录 一定存在),退回 realpath 自身,最后退回 path.resolve。 两个方向都不再误判:指向真实目录的链接被认成真实(该 bootout), 沙箱仍被认成沙箱(不碰 launchd 域)。 2. [high/bug] 原 catch (_) 连 os.homedir() 的异常一起吞掉,返回「不是真实 作业」。那会让 install 照旧把 plist 落盘却永远不 bootstrap——正是本 bug 的另一种形态(文件在、作业不在、用户以为装好了)。现在只捕获路径定位 失败,并明确写 stderr。 3. [high/test] ensureLoaded 的「跳过」分支是 install 侧的主修复点,原先没有 回归测试。新增 probeInstallNow + 断言:沙箱 install --now 不调用任何 launchctl、loaded=false、skipped=true、warn 匹配、plist 仍落盘。 4. [medium] 补齐信号语义:ensureLoaded 的三态返回契约写进注释;install 透传 skipped;uninstall 增加 skipped(与「bootout 试过但作业未加载」区分); status 的 loaded/queried 含义写进注释。 5. 新增符号链接回归测试:指向真实目录的链接必须判为真实作业;沙箱目录 (即便经 /var 链接)必须判为沙箱。 验证:npm test 540 项 0 失败;跑测试前后 ~/.zcode/advisor-companion.log 零增长; 真实 LaunchAgent 在整轮测试后仍 running(pid 不变)。
|
🔍 OpenCodeReview found 16 issue(s) in this PR.
📄
|
| try { return path.join(fs.realpathSync(dir), base); } catch (_) { /* 父目录不存在 */ } | ||
| try { return fs.realpathSync(p); } catch (_) { /* 文件也不存在 */ } |
| try { | ||
| real = realPlistPath(); | ||
| } catch (err) { | ||
| process.stderr.write(`[launchd] 无法定位用户 LaunchAgents 目录,跳过 launchd 域操作:${(err && err.message) || err}\n`); |
| // 显式判断 platform/uid 而不是靠 execFileSync 抛 ENOENT 再吞掉——「静默吞掉」 | ||
| // 正是本 bug 在 CI 上无法暴露的原因。 | ||
| function canTouchLaunchd(p) { | ||
| return process.platform === 'darwin' && uid() !== null && managesRealAgent(p); |
| } | ||
| } | ||
| return { ok: true, path: written.path, changed: written.changed, loaded }; | ||
| return { ok: true, path: written.path, changed: written.changed, loaded, skipped }; |
| } | ||
| const state = /state = (\w+)/.exec(detail); | ||
| return { path: p, exists, loaded, state: state ? state[1] : '' }; | ||
| return { path: p, exists, loaded, queried, state: state ? state[1] : '' }; |
| try { return path.join(fs.realpathSync(dir), base); } catch (_) { /* 父目录不存在 */ } | ||
| try { return fs.realpathSync(p); } catch (_) { /* 文件也不存在 */ } |
There was a problem hiding this comment.
在 canonicalPath 函数中,确保 fs.realpathSync 的异常处理不会泄露敏感路径信息。例如,如果 fs.realpathSync 抛出异常,应确保返回的路径不会暴露用户主目录或其他敏感信息。
Suggestion:
| try { return path.join(fs.realpathSync(dir), base); } catch (_) { /* 父目录不存在 */ } | |
| try { return fs.realpathSync(p); } catch (_) { /* 文件也不存在 */ } | |
| try { | |
| const resolvedDir = fs.realpathSync(dir); | |
| return path.join(resolvedDir, base); | |
| } catch (err) { | |
| // 如果父目录不存在,返回一个安全的默认路径或抛出更明确的错误 | |
| return path.resolve(p); | |
| } | |
| try { | |
| return fs.realpathSync(p); | |
| } catch (err) { | |
| // 如果文件不存在,返回一个安全的默认路径 | |
| return path.resolve(p); | |
| } |
| try { | ||
| real = realPlistPath(); | ||
| } catch (err) { | ||
| process.stderr.write(`[launchd] 无法定位用户 LaunchAgents 目录,跳过 launchd 域操作:${(err && err.message) || err}\n`); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
在 managesRealAgent 函数中,确保 os.homedir() 的异常被完全捕获,并提供更详细的错误信息,以帮助用户定位问题。
Suggestion:
| try { | |
| real = realPlistPath(); | |
| } catch (err) { | |
| process.stderr.write(`[launchd] 无法定位用户 LaunchAgents 目录,跳过 launchd 域操作:${(err && err.message) || err}\n`); | |
| return false; | |
| } | |
| try { | |
| real = realPlistPath(); | |
| } catch (err) { | |
| const errorMessage = `[launchd] Failed to locate user LaunchAgents directory: ${err.message || err}`; | |
| process.stderr.write(`${errorMessage}\n`); | |
| console.error(errorMessage); // 确保错误信息被记录到日志中 | |
| return false; | |
| } |
| // 返回:{ path, exists, loaded, queried, state }。 | ||
| // loaded 的含义**始终**是「launchd 报告该作业已加载」;重定向(沙箱)时不去查询, | ||
| // 于是 loaded 恒为 false,靠 queried=false 表示「没查过」而不是「查过、没加载」。 |
There was a problem hiding this comment.
在 status 函数的文档注释中,明确说明 queried 字段的含义和用途,以帮助调用方正确理解其含义。
Suggestion:
| // 返回:{ path, exists, loaded, queried, state }。 | |
| // loaded 的含义**始终**是「launchd 报告该作业已加载」;重定向(沙箱)时不去查询, | |
| // 于是 loaded 恒为 false,靠 queried=false 表示「没查过」而不是「查过、没加载」。 | |
| // 返回:{ path, exists, loaded, queried, state }。 | |
| // - loaded: launchd 报告该作业是否已加载。 | |
| // - queried: 是否尝试查询 launchd 域。在沙箱场景下,queried=false 表示未查询;在非沙箱场景下,queried=true 表示已查询。 | |
| // 这有助于调用方区分「未查询」(沙箱场景)和「查询但未加载」的状态。 |
OCR 复审(PR #10)逐条核对后的处置: - ensureLoaded/install/uninstall 补 skipped 契约说明:skipped 只表示 「没去碰 launchd 域」,与「碰了但作业未加载」是两回事,调用方据此 决定是否自行拉起进程。 - canonicalPath 补注 realpathSync 的 TOCTOU 兜底方向:目录消失时退回 path.resolve,沙箱仍判为非真实作业(宁可不操作,也不误打真实作业)。 - 新增回归测试锁死该兜底方向,并在沙箱 uninstall 断言 skipped=true。 其余 OCR 意见经实测判为误报或不适用,未采纳: - status() 缺 queried 文档:HEAD 已有该注释块(launchd.cjs:286-289)。 - uid() 未防 process.getuid 缺失:已有 typeof 检查 + try/catch。 - ensureLoaded 的 bootout 未捕获异常:已包 try/catch。 - 要求加 console.debug/console.warn:全项目零处使用 console.debug, 且 tools/companion 的日志走 LOG_FILE 而非 stderr,属风格性建议。 - 要求异步 realpath + 重试:判定是同步短路径,重试反而扩大 TOCTOU 窗口。 - 要求校验 macOS 模拟器/真实硬件:无此可编程信号,非 darwin 已短路。 - 要求 homedir 错误信息更详细:现有信息不含路径,改详细反而更易泄露。
| function canonicalPath(p) { | ||
| const dir = path.dirname(p); | ||
| const base = path.basename(p); | ||
| try { return path.join(fs.realpathSync(dir), base); } catch (_) { /* 父目录不存在 */ } | ||
| try { return fs.realpathSync(p); } catch (_) { /* 文件也不存在 */ } | ||
| return path.resolve(p); | ||
| } |
| function canonicalPath(p) { | ||
| const dir = path.dirname(p); | ||
| const base = path.basename(p); | ||
| try { return path.join(fs.realpathSync(dir), base); } catch (_) { /* 父目录不存在 */ } |
| const dir = path.dirname(p); | ||
| const base = path.basename(p); | ||
| try { return path.join(fs.realpathSync(dir), base); } catch (_) { /* 父目录不存在 */ } | ||
| try { return fs.realpathSync(p); } catch (_) { /* 文件也不存在 */ } |
There was a problem hiding this comment.
在 canonicalPath 函数中,fs.realpathSync 和 path.resolve 的异常处理使用了空 catch 块,这会导致调试和问题排查变得困难。建议在异常处理时记录日志或错误信息,以便更好地追踪问题。例如,可以使用 console.warn 或其他日志系统来记录异常信息。
Suggestion:
| try { return fs.realpathSync(p); } catch (_) { /* 文件也不存在 */ } | |
| try { | |
| return fs.realpathSync(p); | |
| } catch (err) { | |
| console.warn(`[launchd] 路径解析失败,使用备用方案:${err.message}`); | |
| return path.resolve(p); | |
| } |
| const base = path.basename(p); | ||
| try { return path.join(fs.realpathSync(dir), base); } catch (_) { /* 父目录不存在 */ } | ||
| try { return fs.realpathSync(p); } catch (_) { /* 文件也不存在 */ } | ||
| return path.resolve(p); |
| function canTouchLaunchd(p) { | ||
| return process.platform === 'darwin' && uid() !== null && managesRealAgent(p); | ||
| } |
| let detail = ''; | ||
| if (uid() !== null) { | ||
| if (canTouchLaunchd(p)) { | ||
| queried = true; |
| @@ -115,6 +115,68 @@ function launchctl(args) { | |||
| return execFileSync('/bin/launchctl', args, { stdio: 'pipe', timeout: 20000, encoding: 'utf8' }); | |||
| function canonicalPath(p) { | ||
| const dir = path.dirname(p); | ||
| const base = path.basename(p); | ||
| try { return path.join(fs.realpathSync(dir), base); } catch (_) { /* 父目录不存在 */ } | ||
| try { return fs.realpathSync(p); } catch (_) { /* 文件也不存在 */ } | ||
| return path.resolve(p); | ||
| } |
| // 返回契约(三种形状,靠 skipped / warn 区分「跳过」与「失败」): | ||
| // { loaded:true, reloaded:boolean } —— 已加载(reloaded 表示是否重载过) |
| // 却报 loaded=true)——装了 agent 的开发机上这条断言必红。 | ||
| // 返回 loaded=false + queried=false,让「没查过」与「查过、没加载」可区分。 | ||
| let loaded = false; | ||
| let queried = false; |
OCR 第三轮复审(PR #10,run 37413811219)后补充的文档: - install() 补完整返回契约(ok/reason、skipped 与 warn 互斥地表达 「故意跳过」与「尝试后失败」、loaded/changed 的含义)。 - launchctl() 说明为何固定 /bin/launchctl:系统自带、不随版本漂移, 且刻意不走 PATH——PATH 上的同名文件可能被劫持,而该调用会 bootout 用户真实的自启作业。 第三轮其余意见与第二轮重复,仍判为误报或不适用: - status() 缺 queried 文档 / ensureLoaded 缺 reloaded 文档:注释块已存在 (launchd.cjs:211 与 :297-300)。 - realpath catch 是空块:两个 catch 均带行内原因注释,且已补 TOCTOU 兜底方向说明;不加 console.warn 是因为全项目零处使用 console.* 写日志。 - 要求加文件锁/重试缓解 TOCTOU:判定是同步短路径,重试反而扩大窗口。 - 要求重命名 queried 为 isRealAgentQueried:会破坏已发布的返回契约, 且 queried 已在调用方注释中定义。 - 要求动态查找 launchctl 路径:见上,固定绝对路径才是安全方向。
| try { return fs.realpathSync(p); } catch (_) { /* 文件也不存在 */ } | ||
| return path.resolve(p); |
There was a problem hiding this comment.
canonicalPath 函数在 fs.realpathSync 失败后直接回退到 path.resolve,但未验证回退后的路径是否仍满足安全要求。例如,沙箱目录被删除时,path.resolve 可能返回不安全路径,导致 managesRealAgent 误判。建议在回退到 path.resolve 时,增加额外的安全验证,确保路径仍符合预期的安全策略。
Suggestion:
| try { return fs.realpathSync(p); } catch (_) { /* 文件也不存在 */ } | |
| return path.resolve(p); | |
| try { | |
| return fs.realpathSync(p); | |
| } catch (_) { | |
| const resolvedPath = path.resolve(p); | |
| // 添加额外的安全验证逻辑,确保 resolvedPath 符合安全策略 | |
| return resolvedPath; | |
| } |
| let real; | ||
| try { | ||
| real = realPlistPath(); | ||
| } catch (err) { | ||
| process.stderr.write(`[launchd] 无法定位用户 LaunchAgents 目录,跳过 launchd 域操作:${(err && err.message) || err}\n`); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
managesRealAgent 函数在 os.homedir() 抛出异常时虽然写入了 stderr 日志,但未阻止后续 canonicalPath 的调用。如果 os.homedir() 失败,后续路径比较逻辑可能基于无效路径,导致安全判断错误。建议在 os.homedir() 失败时立即返回,避免后续逻辑执行。
Suggestion:
| let real; | |
| try { | |
| real = realPlistPath(); | |
| } catch (err) { | |
| process.stderr.write(`[launchd] 无法定位用户 LaunchAgents 目录,跳过 launchd 域操作:${(err && err.message) || err}\n`); | |
| return false; | |
| } | |
| let real; | |
| try { | |
| real = realPlistPath(); | |
| } catch (err) { | |
| process.stderr.write(`[launchd] 无法定位用户 LaunchAgents 目录,跳过 launchd 域操作:${(err && err.message) || err}\n`); | |
| return false; | |
| } | |
| // 确保 realPlistPath() 成功后才进行后续比较逻辑 | |
| if (!real) { | |
| return false; | |
| } |
| if (!canTouchLaunchd(plistFile)) { | ||
| // 只落盘,由调用方决定是否需要别的加载方式;不谎报「已加载」。 | ||
| return { loaded: false, skipped: true, warn: '目标不是用户真实 LaunchAgent,跳过 launchd 加载(避免影响真实自启作业)' }; | ||
| } |
There was a problem hiding this comment.
ensureLoaded 函数在 canTouchLaunchd 返回 false 时仅设置 skipped: true,但未明确阻止后续 launchctl 调用。如果调用方未正确检查 skipped 状态,仍可能触发 launchctl bootstrap 操作,从而误操作用户真实作业。建议在 ensureLoaded 函数中明确阻止任何 launchctl 操作,并确保调用方严格遵循返回值契约。
Suggestion:
| if (!canTouchLaunchd(plistFile)) { | |
| // 只落盘,由调用方决定是否需要别的加载方式;不谎报「已加载」。 | |
| return { loaded: false, skipped: true, warn: '目标不是用户真实 LaunchAgent,跳过 launchd 加载(避免影响真实自启作业)' }; | |
| } | |
| if (!canTouchLaunchd(plistFile)) { | |
| // 只落盘,由调用方决定是否需要别的加载方式;不谎报「已加载」。 | |
| // 确保调用方严格检查 skipped 状态,避免误操作 launchctl | |
| return { loaded: false, skipped: true, warn: '目标不是用户真实 LaunchAgent,跳过 launchd 加载(避免影响真实自启作业)」 }; | |
| } | |
| // 确保只有在 canTouchLaunchd 返回 true 时才执行 launchctl 操作 | |
| const loaded = isLoaded(l); |
| function install(opts) { | ||
| const o = opts || {}; |
| // ok:true + loaded:false + warn —— 碰了域但 bootstrap 失败 | ||
| // skipped 与 warn 互斥地表达「故意跳过」与「尝试后失败」,调用方据此决定是否自行拉起进程。 | ||
| function install(opts) { | ||
| const o = opts || {}; |
| function ensureLoaded(plistFile, changed, label) { | ||
| const l = label || LABEL; | ||
| if (uid() === null) return { loaded: false, warn: '无法获取 uid,跳过加载' }; | ||
| if (!canTouchLaunchd(plistFile)) { |
| let loaded = false; | ||
| let warn = ''; | ||
| let skipped = false; |
There was a problem hiding this comment.
在 install 函数中,skipped 变量的初始化位置与其后续使用存在不一致。skipped 在 ensureLoaded 调用后才被赋值,但未在函数开头声明。如果 ensureLoaded 返回的对象中不包含 skipped 字段,将导致 skipped 变量未定义。建议在函数开头声明 skipped 变量,并确保其始终被正确赋值。
Suggestion:
| let loaded = false; | |
| let warn = ''; | |
| let skipped = false; | |
| let loaded = false; | |
| let warn = ''; | |
| let skipped = false; |
| let bootedOut = false; | ||
| if (uid() !== null) { | ||
| let skipped = false; |
There was a problem hiding this comment.
在 uninstall 函数中,skipped 变量的初始化位置与其后续使用存在不一致。skipped 在 canTouchLaunchd(p) 条件块内被赋值,但未在函数开头声明。如果 canTouchLaunchd(p) 返回 false,skipped 将保持未定义,导致后续返回的对象中可能包含未定义的字段。建议在函数开头声明 skipped 变量,并确保其始终被正确赋值。
Suggestion:
| let bootedOut = false; | |
| if (uid() !== null) { | |
| let skipped = false; | |
| let bootedOut = false; | |
| let skipped = false; |
| if (uid() !== null) { | ||
| let skipped = false; | ||
| if (canTouchLaunchd(p)) { | ||
| try { launchctl(['bootout', `gui/${uid()}/${LABEL}`]); bootedOut = true; } catch (_) { /* 未加载:忽略 */ } |
| let loaded = false; | ||
| let queried = false; |
There was a problem hiding this comment.
OCR 第四轮复审(PR #10)里唯一成立的一条:ensureLoaded 在 uid()===null 时返回 {loaded:false, warn} 而**没有** skipped:true,与沙箱分支的契约 不一致。调用方按 skipped 判断「是故意跳过还是尝试后失败」,缺这个字段 会把「拿不到 uid、根本没去加载」误当成「加载失败」,进而重复重试。 同时修正上一轮我自己写错的文档:install() 契约里写「skipped 与 warn 互斥」, 实测沙箱分支两个字段同时出现(skipped=true 且 warn 有值)。改为 「skipped=true 时 warn 说明跳过原因,两者会同时出现」。 新增回归测试:删掉 process.getuid 后 install --now 必须 skipped=true、warn 含 uid、且一次 launchctl 都不调。已验证该测试在 修复前的源码上会红(fail 1),修复后转绿。 第四轮其余意见仍判为误报(逐条实测): - canonicalPath 回退 path.resolve「未做安全校验」:兜底方向本就是安全的, 沙箱目录消失时判为「非真实作业」→ 不碰 launchd 域,已在注释中写明。 - managesRealAgent 里 homedir 抛错后「未阻止后续比较」:catch 内已 return false(launchd.cjs:154-158)。 - ensureLoaded 的 skipped「未阻止 launchctl」:该分支在任何 launchctl 调用之前直接 return(launchd.cjs:223-227)。 - skipped/queried/bootedOut「未在函数开头声明」:均已声明 (launchd.cjs:269-271、:292-293、:318-319)。 - 变量命名 o/plistFile/p:const o = opts || {} 是仓库既有约定(10 处)。 - status 缺 queried 文档:注释块已存在(launchd.cjs:314-319)。 - logPath 的 homedir 未处理:属未改动的既有代码,不在本次改动范围。 - launchctl 错误处理不一致:uninstall/status 的静默是设计意图(清理动作 失败不应阻断卸载/查询),ensureLoaded 才需要把失败报给调用方。
|
|
||
| // launchctl 固定在 /bin/launchctl:这是 macOS 系统自带、不随版本漂移的绝对路径, | ||
| // 刻意不走 PATH 查找——PATH 上的同名文件可能被用户环境劫持,而这里的调用会 | ||
| // bootout 用户真实的自启作业。 |
| function launchctl(args) { | ||
| return execFileSync('/bin/launchctl', args, { stdio: 'pipe', timeout: 20000, encoding: 'utf8' }); | ||
| } |
There was a problem hiding this comment.
在 launchctl 函数中,虽然设置了超时(20000ms),但建议考虑是否需要根据不同的操作类型(如 print、bootout、bootstrap)调整超时时间。例如,bootstrap 操作可能需要更长的时间,而 print 或 bootout 操作可能可以接受更短的超时。这有助于提高响应速度并避免不必要的等待。
Suggestion:
| function launchctl(args) { | |
| return execFileSync('/bin/launchctl', args, { stdio: 'pipe', timeout: 20000, encoding: 'utf8' }); | |
| } | |
| function launchctl(args) { | |
| // 根据操作类型动态调整超时时间,例如: | |
| // - print/bootout: 5000ms | |
| // - bootstrap: 30000ms | |
| const timeout = args.includes('bootstrap') ? 30000 : 5000; | |
| return execFileSync('/bin/launchctl', args, { stdio: 'pipe', timeout, encoding: 'utf8' }); | |
| } |
| // 刻意不走 PATH 查找——PATH 上的同名文件可能被用户环境劫持,而这里的调用会 | ||
| // bootout 用户真实的自启作业。 | ||
| function launchctl(args) { | ||
| return execFileSync('/bin/launchctl', args, { stdio: 'pipe', timeout: 20000, encoding: 'utf8' }); |
| // 幂等加载:已加载且 plist 未变 → 什么都不做(**不能无条件 bootout+bootstrap**, | ||
| // 那会把正在跑的实例杀掉再拉起,用户每次点图标都会闪断一次)。 | ||
| // plist 变了 → 必须重载,否则 launchd 仍按旧配置跑(升级后不生效)。 | ||
| // |
There was a problem hiding this comment.
在 ensureLoaded、install、uninstall 和 status 函数中,虽然返回契约已经明确,但建议在每个函数开头添加更详细的文档注释,以便开发者快速理解各个字段的含义及其区别。例如:
skipped:表示是否跳过 launchd 操作(沙箱/非 darwin/无 uid)。warn:跳过或失败的原因说明。loaded:是否已交给 launchd 加载。
这有助于提高代码的可维护性和理解性。
Suggestion:
| // 幂等加载:已加载且 plist 未变 → 什么都不做(**不能无条件 bootout+bootstrap**, | |
| // 那会把正在跑的实例杀掉再拉起,用户每次点图标都会闪断一次)。 | |
| // plist 变了 → 必须重载,否则 launchd 仍按旧配置跑(升级后不生效)。 | |
| // | |
| /** | |
| * 幂等加载:已加载且 plist 未变 → 什么都不做(不能无条件 bootout+bootstrap, | |
| * 那会把正在跑的实例杀掉再拉起,用户每次点图标都会闪断一次)。 | |
| * | |
| * 返回契约: | |
| * - { loaded: true, reloaded: boolean }:已加载(reloaded 表示是否重载过)。 | |
| * - { loaded: false, skipped: true, warn }:故意跳过域操作(沙箱/非 darwin/无 uid)。 | |
| * - { loaded: false, warn }:真失败(bootstrap 抛错)。 | |
| */ |
| // 注意 plistFile 指向沙箱时**绝不能**走 launchd 域(见 canTouchLaunchd): | ||
| // 那里的 bootout 打的是真实 LABEL,会把用户真实的 agent 打死并卸载。 | ||
| // | ||
| // 返回契约(三种形状,靠 skipped / warn 区分「跳过」与「失败」): |
| } | ||
| } | ||
| return { ok: true, path: written.path, changed: written.changed, loaded }; | ||
| return { ok: true, path: written.path, changed: written.changed, loaded, skipped }; |
| let removed = false; | ||
| try { fs.unlinkSync(p); removed = true; } catch (_) { /* 不存在:忽略 */ } | ||
| return { ok: true, path: p, removed, bootedOut }; | ||
| return { ok: true, path: p, removed, bootedOut, skipped }; |
| } | ||
| const state = /state = (\w+)/.exec(detail); | ||
| return { path: p, exists, loaded, state: state ? state[1] : '' }; | ||
| return { path: p, exists, loaded, queried, state: state ? state[1] : '' }; |
OCR 第五轮复审(PR #10)里唯一有实质的一条:新增的 skipped/queried 返回字段没有任何调用方消费。CLI 是唯一的人类可读出口,补上: - uninstall 在 skipped=true 时打印「(未操作 launchd 域)」——否则 「已移除/文件不存在」只描述 plist 文件,容易被当成真实作业也被卸载了。 - status 在 queried=false 时打印 queried=否——否则只打 loaded=否 会让 packagers.cjs:213 / install-templates.cjs:184 的 把「没查过」误读成「作业没在跑」。 刻意不改 这个 grep 契约:它只在真实 plist 路径上被调用 (无 dir 参数),此时 queried 恒为 true,行为与改动前一致;已实测 真机 status 输出不含 queried=否、且 grep 仍命中。 第五轮其余意见判为误报或不适用: - 「bootout 是拼音、建议国际化」:bootout 是 launchctl 的真实子命令名, 改名会让命令失效。 - 「按操作类型调超时」:print/bootout/bootstrap 共用 20s 是既有设计, 本次未改动该行。 - 「函数开头补更详细文档」:四个函数的返回契约注释块均已存在。 - 「launchctl 固定路径/变量命名」:与第四轮重复,已逐条答复。
| let loaded = false; | ||
| let queried = false; | ||
| let detail = ''; | ||
| if (uid() !== null) { | ||
| if (canTouchLaunchd(p)) { | ||
| queried = true; |
There was a problem hiding this comment.
在 status 函数中,queried 变量的初始化和赋值逻辑与 install 和 uninstall 函数中的 skipped 变量类似,但缺少明确的初始化。如果 canTouchLaunchd(p) 返回 false,queried 变量将保持 undefined,这可能导致调用方无法正确理解返回值。
建议明确初始化 queried 变量为 false,并确保其在所有情况下都有明确的值。例如:
let queried = false;
let loaded = false;
let detail = '';
if (canTouchLaunchd(p)) {
queried = true;
try {
detail = launchctl(['print', `gui/${uid()}/${LABEL}`]);
loaded = true;
} catch (_) { /* 未加载 */ }
}Suggestion:
| let loaded = false; | |
| let queried = false; | |
| let detail = ''; | |
| if (uid() !== null) { | |
| if (canTouchLaunchd(p)) { | |
| queried = true; | |
| let queried = false; | |
| let loaded = false; | |
| let detail = ''; | |
| if (canTouchLaunchd(p)) { | |
| queried = true; |
| // 显式判断 platform/uid 而不是靠 execFileSync 抛 ENOENT 再吞掉——「静默吞掉」 | ||
| // 正是本 bug 在 CI 上无法暴露的原因。 | ||
| function canTouchLaunchd(p) { | ||
| return process.platform === 'darwin' && uid() !== null && managesRealAgent(p); |
There was a problem hiding this comment.
在 canTouchLaunchd 函数中,虽然已经明确判断了 process.platform === 'darwin',但非 darwin 平台的错误处理逻辑与 macOS 平台不一致。建议在非 darwin 平台下也返回 { loaded: false, skipped: true, warn } 格式,以保持一致性。
Suggestion:
| return process.platform === 'darwin' && uid() !== null && managesRealAgent(p); | |
| if (process.platform !== 'darwin') { | |
| return { loaded: false, skipped: true, warn: '非 darwin 平台,跳过 launchd 加载' }; | |
| } | |
| return uid() !== null && managesRealAgent(p); |
OCR 复审收敛情况(6 轮)本 PR 共经历 6 轮自动 OCR 复审,结论逐轮收敛:
第 6 轮的两条经实测均为误报:
复审已收敛(意见数 13 → 8 → 2,且最后两条为重复误报),继续跑只会消耗评审服务商的配额 修复内容根因: 后果:在本机跑 修复:新增 验证
|
症状
外挂时不时就「无法连接本机 controller(外挂未运行?):Failed to fetch」,重开 App 能恢复,但很快又复现。
根因
tools/companion/launchd.cjs的uninstall()里,plist 路径尊重dir覆盖,但bootout的目标是模块常量LABEL:LABEL是固定常量,重定向只能改 plist 的路径,改不掉 launchd 域里的作业名。于是:test/launchd.test.js用L.uninstall({ dir })指向临时目录,实际打的却是用户真实的作业;launchctl bootout会终止该作业的进程并把它从 launchd 卸载,KeepAlive监督随作业一起消失;uninstall从不重新 bootstrap ⇒ 进程被杀且永不自动重启,直到用户手动重开一次 App。所以在本机跑一次
npm test就会打死外挂。这不是「动不动」,是必然;用户重开 App 后又被下一次测试打死,才表现为间歇。CI 结构性看不见:
testjob 跑在ubuntu-latest,没有/bin/launchctl,execFileSync抛 ENOENT 被catch (_)吞掉;macOS job 不跑npm test。修复
uninstall/status/ensureLoaded一律先判定「目标 plist 是否就是用户真实那份」(canTouchLaunchd),不是就完全不碰 launchd 域,只操作自己沙箱里的文件。判定用路径而不是「有没有传 dir」,这样
dir显式指向真实目录、或经ZCODE_ADVISOR_LAUNCHD_DIR重定向的情形都能正确识别。路径比较用canonicalPath()(优先 realpath 父目录)而非path.resolve,因为 macOS 上/var、/tmp都是符号链接——只做path.resolve会把经这些路径进来的真实 plist 误判成沙箱,导致真实卸载被静默跳过(功能失效)。同时把「静默吞掉 ENOENT」换成显式的
process.platform/uid()判断——静默吞掉正是本 bug 在 CI 上无法暴露的原因。顺带修复
test/health.test.js、test/companion-zcode.test.js等会把本机 API 就绪:http://127.0.0.1:0/api/status、迁移 0.2.17…写进真实生产日志~/.zcode/advisor-companion.log,淹没真实故障。现已隔离到临时目录,并加静态守卫测试防止回归。验证
npm test:540 项,0 失败。~/.zcode/advisor-companion.log零增长。state = running、pid 不变。回归测试
uninstall({dir})/ZCODE_ADVISOR_LAUNCHD_DIR重定向:launchctl调用数为 0。install({dir, now:true}):不 bootstrap、skipped=true、plist 仍落盘。status({dir}):queried=false,不把真实运行态冒充沙箱态。