fix: macOS foreground execution — liveness fail-closed, evidence pack included - #3
Conversation
c846222 to
c415205
Compare
Xuan0629
left a comment
There was a problem hiding this comment.
Request changes — current macOS liveness implementation is fail-open
这次 PR 已明显改善:CI 已绿,补了 launcher / Darwin identity / liveness 的测试,并增加了 MACOS_FIX.md。不过以下 P0 安全问题会让不可验证状态被错误判为 dead,因此目前不能合并,也不能作为 v1.1.0 的 macOS release gate 证据。
P0 — ps 失败/超时时会错误允许 resume
当前链路:
# src/unison/foreground.py:140-155
_darwin_process_group_alive()
# catches OSError/SubprocessError and returns False
# src/unison/foreground.py:196
return "dead" if not _darwin_process_group_alive(group_id) else "live"因此 ps 无法执行、超时、权限/解析错误时,False 同时表示“明确没有 group members”和“根本没有可靠查询结果”,随后被解释为 dead。但 Orchestrator.load_resume_state() 只接受 dead,这会错误放行 replacement/resume。该项目的合同要求不可验证必须是 unknown 并 fail closed。
我按真实函数路径复现了:
subprocess.run(... ps ...) -> OSError
_darwin_process_group_alive(...) -> False
foreground_child_and_group_status(...) -> dead # incorrect
Required implementation change
请不要使用 bool 表示该查询结果。让 helper 返回明确三态(例如 "live" | "dead" | "unknown",或在异常/不可解析时返回 None),并让调用方只在 成功完成的 ps 查询、且所有有效输出均证明 group 无成员 时返回 dead。
至少下列情况必须返回 unknown:
subprocess.run的OSError/TimeoutExpired/SubprocessError;ps非零退出;- 不可解析或意外输出;
- 无法建立可靠的 process-group 查询结果。
unknown 不得被转换为 dead;它必须继续阻止 duplicate dispatch 和 resume。保留现有 Linux 语义不变。
Required tests
现有 test_unknown_when_identity_unverifiable / test_unknown_blocks_dispatch_not_dead mock 的是 helper 向上抛异常;但生产 helper 会吞掉异常并返回 False,所以没有覆盖真实故障路径。请新增真实路径测试,mock subprocess.run 本身:
- Darwin
psOSError→ group statusunknown; TimeoutExpired/SubprocessError→unknown;- non-zero return code 与不可解析输出 →
unknown; - 有效、成功的空 group 输出 → 唯一允许
dead的路径; - 状态为
unknown时,覆盖Orchestrator.load_resume_state()(或现有 resume dispatch 路径)确认 replacement/resume 被拒绝; - 继续保留“child gone but verified group member exists → live”。
Required scope cleanup — remove unrelated provider override
请从本 PR 移除 src/unison/foreground.py:55-58 的 CODEX_MODEL_PROVIDER / -c model_provider=... 改动。它与 macOS foreground execution 无关,却改变了 Codex interactive runtime configuration;应单独开 PR,附独立测试和审查,不要混入 release gate 修复。
Required evidence correction
PR body 声称 All 10 cases PASS 并引用:
unison-foreground-evidence/20260720-172302/final-report.md
但该 evidence pack 不在 PR tree/附件中,当前无法审计;MACOS_FIX.md 也只是摘要。请在 PR 提交脱敏、可审计的 evidence pack,或使用可稳定访问的 PR attachment/link,并依照 docs/foreground-execution-macos-validation.md 提供:每例命令、版本、git SHA/clean status、Terminal.app-visible结果、artifact JSON、state/checkpoint/notifications、exit status、脱敏 transcript。
此外,请覆盖两种 runtime 的完整 pack;PR 当前表中 terminal-close、reconcile、resume 只记录了 Codex,不能代表 Claude 已完成相同合同验证。若某一项只在一个 runtime 适用,请在 evidence 中说明该合同映射,而不是声称两者都已完成。
Re-review gate
请在新 head 上完成:
PYTHONPATH=$PWD:$PWD/src .venv/bin/python -m pytest -q
PYTHONPATH=$PWD:$PWD/src .venv/bin/python -m pytest -q -W error::DeprecationWarning并等待 GitHub CI green。复审条件是:ps 查询不确定性严格 fail-closed、真实故障路径有回归测试、无关 provider 改动已移出、macOS evidence pack 可审计,且仍不注入终端输入、不自动审批、不回退 headless、不重试、serial-only。
bbc4627 to
bc88907
Compare
Xuan0629
left a comment
There was a problem hiding this comment.
Final required corrections — implementation is close; evidence and fail-closed boundary need tightening
The P0 ps failure issue is substantially fixed in this revision: the three-state result is the right design, CI is green, the unrelated CODEX_MODEL_PROVIDER change has been removed, and the new real-path failure tests are a material improvement. These final items are not caused by the recent RunLifecyclePersistence / cohesion refactor: this PR is based on master@33e0805, does not modify run_lifecycle.py, orchestrator.py, or WebUI, and the issues are confined to the new macOS foreground implementation/evidence.
Required 1 — regenerate the macOS evidence pack for the actual review HEAD
The committed evidence reports commit bc1af54 and old test totals (69 passed; 1903 passed, 2 failed):
docs/evidence/foreground-macos-v1.1/final-report.md:2
**Commit**: `bc1af54`
docs/evidence/foreground-macos-v1.1/env-info.txt:2
bc1af54...
The review HEAD is now bc889074, which contains the P0 three-state liveness fix. Old artifacts can support earlier launcher behavior, but cannot demonstrate that the fixed fail-closed behavior was validated on a real macOS host.
Please re-run the validation pack on macOS from the exact new head (or its successor) and update the committed evidence/PR body with:
git rev-parse HEADmatching the reviewed commit andgit status --shortbefore each case;- macOS/Python/Claude/Codex versions and the exact redacted commands;
- updated test totals actually produced by that checkout;
- retained redacted invocation artifacts, state/checkpoint/notification results, exit status, and visible Terminal.app outcome as required by
docs/foreground-execution-macos-validation.md; - a focused P0 proof on macOS: make
psunavailable/fail (or use a controlled equivalent) and show the resulting status isunknown, then show resume/replacement is refused; show a successful verified-empty group query is the only route todead.
Do not rewrite old evidence as if it were new. If some prior files remain useful, label them explicitly as historical evidence for bc1af54 and add a separate bc889074 evidence directory/report.
Required 2 — make malformed non-empty ps output fail closed
_darwin_process_group_alive() currently returns dead when its output has at least one parseable unrelated PGID plus one malformed non-empty line. This is encoded by the any_parseable behavior in src/unison/foreground.py:154-168 and the associated mixed-output test.
The contract and docstring say unparseable output is unknown. For a safety-critical resume gate, partial parseability must not be treated as proof that the process group is absent. Please change the rule to:
successful exit-0 ps output AND every non-empty line parseable AND no matching member -> dead
any malformed/unexpected non-empty line -> unknown
matching member -> live
empty output -> unknown
ps execution/timeout/non-zero -> unknown
Update tests accordingly: replace the current mixed-garbage expectation of dead with unknown; retain tests for valid no-match output -> dead, valid matching member -> live, and all existing exception/non-zero/empty-output -> unknown cases. Add/retain an integration-level assertion that unknown prevents load_resume_state() / replacement resume.
Re-review gate
After these two changes, run and paste results from the exact new head:
PYTHONPATH=$PWD:$PWD/src .venv/bin/python -m pytest -q
PYTHONPATH=$PWD:$PWD/src .venv/bin/python -m pytest -q -W error::DeprecationWarningWait for GitHub CI on that new SHA to be green, then request review again. No version/tag/release changes are part of this PR.
bc88907 to
01be2d3
Compare
Xuan0629
left a comment
There was a problem hiding this comment.
Re-verified on current PR head 01be2d3: two required corrections remain
Per maintainer direction, Python 3.10 in the macOS evidence is not a blocking issue and does not need to be changed for this PR. The two items below were re-checked against the current remote PR head 01be2d340c0803e133b38d603941c4e8c72efe12; both remain real.
Required 1 — current-head evidence provenance/artifacts are still not auditable
The PR claims docs/evidence/foreground-macos-v1.1/be523b3/ is current-head evidence, but the checked-in files still state:
HEAD: be523b3baeb3c8b9f8da79b0abd15d52515c2139
That is neither the review head (01be2d3...) nor a resolvable commit in the GitHub repository (GitHub commit lookup returns No commit found for SHA, HTTP 422). env-info.txt also records an untracked evidence directory plus untracked pipeline.yaml and test.txt, rather than the clean baseline required by docs/foreground-execution-macos-validation.md.
Further, the alleged current-head directory contains only:
env-info.txt
final-report.md
It does not contain the current-head case commands, invocation artifacts (request.json, child.json, heartbeat.json, result.json), resulting state/checkpoint/notification artifacts, exit statuses, or redacted transcripts required by the validation pack. It merely points back to historical bc1af54 artifacts, which predate the current three-state fix.
Please replace this with one self-consistent evidence directory for the actual PR head (or the next pushed head):
- Record a real, repository-resolvable
git rev-parse HEADmatching that pushed SHA; - Record
git status --shortbefore each case; use a clean disposable checkout/baseline (the evidence directory should be outside that checkout, as the validation document already specifies); - Include/redact and commit the required current-head case artifacts and exact commands, not only a narrative summary;
- Label prior
bc1af54material explicitly as historical launcher evidence, never as proof of current three-state code; - Keep the focused P0 proof, but include the concrete command/output/artifact showing
unknownblocks resume and verified-emptypsis the onlydeadroute.
Required 2 — liveness still violates its own strict contract when a match precedes garbage
I re-read the current implementation at src/unison/foreground.py:136-174. The docstring promises every non-empty line must be parseable for either live or dead, but the loop returns live immediately on a matching line:
if int(stripped) == group_id:
return "live"So a successful ps output such as:
9999
not-a-pgid
returns live, while reversing the two lines returns unknown. The current test only covers garbage before a match (tests/test_interactive_execution.py:1214-1222), so this order-dependent case is still untested.
This does not recreate the former fail-open resume bug (live and unknown both block resume), but it does violate the advertised strict contract and makes status depend on output order. Please parse/validate every non-empty line before deciding status, e.g. collect validated PGIDs first; any malformed line must return unknown; only after successful full validation return live if group_id is present, otherwise dead.
Add the missing regression test:
stdout = "9999\\nnot-a-pgid\\n" -> "unknown"
Keep the existing valid-output tests (dead for all parseable non-matching PGIDs; live for all parseable output containing the matching PGID).
Re-review gate
After these two corrections, push a new head, update the PR evidence reference to that exact resolvable commit, run the normal and -W error::DeprecationWarning suites, wait for green CI, then request review. No Python-version change, version bump, tag, or release action is requested here.
7c6b27c to
eead5ee
Compare
8ba45d0 to
40766e5
Compare
Xuan0629
left a comment
There was a problem hiding this comment.
Approve — final macOS foreground gate review
Re-reviewed exact head 40766e5. The two-pass liveness contract is order-independent and fail-closed; focused strict tests and the full strict-deprecation suite pass (1933 passed), and GitHub CI run 29742070689 is green.
Evidence provenance is now internally consistent enough for this merge: the real evidence commit da1822c is repository-resolvable, and its production/test blobs (foreground.py, test_interactive_execution.py, test_orchestrator.py) are byte-identical to this reviewed head; changes from that evidence commit to the final head are evidence/description metadata only. Historical bc1af54 launcher artifacts are explicitly labeled historical, while current P0 artifacts are separated. No automatic approval, terminal input injection, bypass fallback, or unrelated provider configuration changes were introduced.
Approved for merge.
Summary
macOS foreground execution support with strict fail-closed three-state process-group liveness.
Core Changes
src/unison/foreground.py: macOS foreground execution implementation_darwin_process_group_alive(): Two-pass strict validation — parse ALL lines first, then check membership"live"/"dead"/"unknown"three-state contract"9999\nnot-a-pgid\n"returns"unknown"regardless of position.ps -o pgid= -axrun_foreground_wrapper(): manages child lifecycle + result recordingTests
tests/test_interactive_execution.py(5 skipped = interactive-only)TestDarwinProcessGroupAliveRealPathcovering OSError, Timeout, SubprocessError, nonzero exit, unparseable output, empty output, valid dead, valid live, mixed garbage+match (both orders), garbage+no-matchtest_match_before_garbage_returns_unknown— ensures order-independenceEvidence Pack
docs/evidence/foreground-macos-v1.1/current-head-evidence/— current HEAD evidencep0-proof/liveness-proof-table.md— 11-condition proof tablep0-proof/request.json,child.json,heartbeat.json,result.json— wrapper artifactsp0-proof/resume-block-proof.json— unknown blocks resume proofgit rev-parse HEADat this commit, thenpython -m pytest tests/test_interactive_execution.py -q -W error::DeprecationWarningLiveness Contract
"unknown""unknown""unknown""unknown""live""dead"Both
"live"and"unknown"block resume — fail-closed. Only verified-empty ps returns"dead".Addresses Review
"9999\nnot-a-pgid\n"→"unknown"