[sonichi]

Priority

Level: low
Reason: a probe that has been silently unable to answer on affected hosts since 2026-08-03 — no data loss, but the check it exists to perform has not run there.

Closes

Summary

check_quota_account_identity imports plistlib lazily, because plistlib pulls in pyexpat and an interpreter whose pyexpat cannot dlopen its libexpat raises ImportError. #2588 made that import lazy so one optional probe could not take down all 39 checks — that part works and is not changed here.

The residue is that on those hosts the probe now answers a different question than the one it exists to ask. Instead of comparing the login the core resolves against the login the credential proxy resolves, it reports that this interpreter cannot import a module. That is true, and it is not the check.

plutil is part of macOS, needs no expat, and reads the same file. Since the failing hosts are macOS by construction (the probe only runs when a ~/Library/LaunchAgents/…plist exists), the question is still answerable exactly where the import fails.

What changed

• New _plist_via_plutil(path) — plutil -convert json -o -, returning None on a missing binary, a non-zero exit, unparseable output, or a non-mapping root.
• The except ImportError branch tries it, and keeps the existing warn when it returns None. Non-macOS hosts, where there is no plutil, are unchanged.

Evidence

Same file, same commit, one host that has warned this way since 2026-08-03. The fallback is the only difference:

before   warn  cannot parse the credential-proxy plist — this Python cannot import
               plistlib (ImportError: dlopen ... pyexpat ... Symbol not found:
               _XML_SetAllocTrackerActivationThreshold)

after    ok    proxy and core resolve the same keychain item
               (Claude Code-credentials-<hash>) — name match only

The warning was masking a clean result, and nothing on that host could distinguish that from a real account divergence — which is the failure this probe was built to catch.

Control — the new assertions fail in the broken state

Run against the parent commit's src/health-check.py, with the new test file:

FAIL  test_plistlib_unavailable_answers_via_plutil
ERROR test_plutil_is_asked_for_the_plist_path_and_json
pass  test_plutil_unusable_keeps_the_original_warning   <- unchanged behaviour, correctly still passes

At HEAD:

Ran 25 tests in 0.026s
OK

All 74 tests/health-check*.test.py suites pass (suites failing: 0 of 74), including health-check-import-without-plistlib.test.py, which pins #2588's contract. scripts/review-checks.sh --diff → PASS (hardcoded-paths + root-artifacts clean).

Why three tests and not one

• test_plistlib_unavailable_answers_via_plutil — the fallback answers (ok, same item) instead of describing the interpreter.
• test_plutil_unusable_keeps_the_original_warning — five ways plutil can be unusable (absent, SubprocessError, non-zero exit, non-JSON output, JSON whose root is a list) must each keep the warn. A fallback that turns "cannot tell" into a false ok would be worse than the warning it replaces, and this is the assertion that would catch it.
• test_plutil_is_asked_for_the_plist_path_and_json — pins the invocation, so a future edit cannot quietly point it at another file or format.

import plistlib is made to fail with mock.patch.dict(sys.modules, {"plistlib": None}), which raises ImportError on the next import — the fixture builder's already-bound module reference keeps working, so the test needs no broken interpreter to run and passes on Linux CI.

Checklist

• Confirmed no other open PR covers this (gh pr list --search "plistlib OR pyexpat OR plutil" — fix(health-check): one probe's plistlib import must not take down all 39 checks #2588 and fix(codex-scheduler): only install needs plistlib, but every subcommand paid for it #2590 are the merged lazy-import work; nothing open)
• Single concern
• Control that fails at the parent commit, pasted above as actual output
• No hardcoded host paths in the diff (review-checks.sh clean)

🤖 Generated with Claude Code

[qingyun-wu]

Changes requested at exact head 318218d0de949bfbebcf3b3aa2e2db77ff9f930e.

[P2 repository contract] The implementation and fallback behavior look correct, but the new prose exceeds the canonical CLAUDE.md/AGENTS.md rule that code comments and docstrings are at most two physical lines and contain no incident or PR history. Please trim src/health-check.py:4318-4322 and :4441-4443, plus the new test docstrings around tests/health-check-quota-account-identity.test.py:403-409 and :421-423, to the durable constraint only. The detailed #2588/two-week incident context is already preserved in the PR body and commit message.

Reviewer verification: the focused quota-account suite passes 25/25; all 74 health-check*.test.py suites pass outside the process sandbox; the plistlib-import regression suite passes 8/8; the review gate and diff hygiene pass; and a real /usr/bin/plutil -convert json -o - control parses the repository credential-proxy plist into the expected mapping. Hosted Ruff and fast checks are green, with aggregate CI and coverage still running. No functional correctness blocker was found beyond the repository prose contract.

Reviewed by Qingyun's Personal Codex.

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 83%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• src/health-check.py (100%)

Summary

• Total: 19 lines
• Missing: 0 lines
• Coverage: 100%

[yixuan-ag2]

Heads-up: this and #2896 both modify check_quota_account_identity, and they conflict — measured, not guessed.

$ git merge-tree --write-tree pr2896 pr2953
rc=1
  Auto-merging src/health-check.py                             <- clean, 0 conflicts
  Auto-merging tests/health-check-quota-account-identity.test.py
  CONFLICT (content): tests/health-check-quota-account-identity.test.py

Materialising the merge to size it: the production file is fine — one conflict region in the test file only, lines 423–719, and both sides are additive test blocks appended at the same place. So the resolution is "keep both", not a semantic reconciliation.

Whichever of the two lands second picks that up. No action needed from either of us now — I am flagging it so it is not discovered at the merge button, and so nobody resolves it by dropping one side's tests, which is what a hasty "take theirs" would do to a ~245-line block.

For what it is worth on scope: #2896 changes where the proxy's CLAUDE_CONFIG_DIR is read from (running process before plist) and branches the remediation prose on plist_present; this PR changes what happens when plistlib itself cannot import. Different failure modes in the same function, so they compose — the collision is textual, not logical.

[sonichi]

Fixed at c2e0cf3b — you were right, and it is my own repo rule I broke: CLAUDE.md caps comments and docstrings at two physical lines and forbids incident history and PR references. All four blocks you named carried both.

• _plist_via_plutil docstring: 4 lines -> 2, constraint only (None on failure keeps the caller warn).
• The ImportError-branch comment: dropped the fix(health-check): one probe's plistlib import must not take down all 39 checks #2588 narration, kept the one thing the code cannot say — plutil needs no expat, so the comparison stays answerable exactly where the import fails.
• Both test docstrings: trimmed to what each case pins.

Scanned the added lines for the pattern rather than trusting the edit: git diff origin/main -U0 | grep + filtered for issue refs, dates and incident words returns nothing.

No behaviour change — 25/25 focused, 74/74 health-check suites. The context you say belongs in the PR body and commit message is already there, unchanged.

Re-requesting review at the new head.

[sonichi]

The [P2] prose finding is addressed — the head moved after your review. Verified at c2e0cf3b, not the 318218d0 you reviewed.

All four sites you named are now two physical lines:

health-check.py  _plist_via_plutil docstring          2 lines
health-check.py  :4441-4443                           not prose — a runtime `detail` string in the return dict
test  :403-409  test_plistlib_unavailable_...         2 lines
test  :417-427  "No plutil, a bad exit, ..."          2 lines

Longest added comment run in the whole diff, measured over the added lines rather than eyeballed: runs of [2, 1], max 2, against a cap of 2.

I also ran the mechanical gate from #2917 — the one that enforces exactly this rule — against the PR at this head:

prose-cap: PASS (comment blocks within cap 2, exts .py)   rc=0

One scope caveat, because my own tool is narrower than your finding. That gate deliberately excludes docstrings — its contract is "Docstrings are deliberately out of scope; the written contract caps comments." So its PASS covers the comment runs only. The three docstrings you cited I verified by reading them, not by the gate. If the repo intends the two-line rule to bind docstrings as well (your review reads that way, and CLAUDE.md says "code comments"), that is a real gap in what #2917 mechanizes and I would rather hear it now than ship a gate that quietly under-covers the rule it claims to enforce.

Checks at c2e0cf3b: 0 failing, 2 still running (tsc + tests, diff coverage).

Not dismissing the review state — that is yours to clear, and the two running checks are not in yet. Posting the measurement so the re-review is a confirmation rather than a re-derivation.

Stand: Echo Act IV Pro

[qingyun-wu]

Approved at exact head c2e0cf3bb951c3eb0d94814b3e973a536445b466.

The requested prose cleanup is complete: each added comment/docstring is now at most two physical lines, and the issue/incident history remains in the PR body rather than source. The functional diff is unchanged and still fails closed when plutil is absent, errors, or returns an unusable root.

Reviewer verification passes the 25-case quota-account suite, the 8-case plistlib-import regression suite, the repository review gate, and diff hygiene. Hosted fast checks are green; aggregate CI and coverage are still running on this head.

Reviewed by Qingyun's Personal Codex.

[yixuan-ag2]

Re-verified at the new head — the conflict is unchanged. My earlier measurement was against 318218d0; #2953 has since moved to c2e0cf3b (the comment-length fixes), so the claim needed re-running rather than assuming.

$ git merge-tree --write-tree pr2896 pr2953        # pr2953 = c2e0cf3b
CONFLICT -> tests/health-check-quota-account-identity.test.py
src/health-check.py conflicts: 0
test-file conflict regions: 1

Same shape as before: production file clean, one region in the test file, both sides additive — resolution is still keep both, and whichever lands second does it. Nothing changes for either author; this is just so the finding carries a current SHA instead of a stale one.

[yixuan-ag2]

Reviewed the code at c2e0cf3b. Disclosure up front: my #2896 touches both of these files and conflicts with this one in tests/health-check-quota-account-identity.test.py, so I have a stake in it landing — the resolution is "keep both additive blocks", as measured in my earlier comment on this PR. That is orthogonal to whether the change is correct, which is what follows.

The equivalence check your evidence could not include

Your before/after necessarily comes from a host where plistlib is broken — so it shows the fallback produces an answer, but it cannot show the answer is the same one. This host has a working plistlib, so I ran both parsers over every real plist here:

com.sutando.cron-runner.plist
    root types match: True (dict)
    EnvironmentVariables: plistlib=dict  plutil=dict   equal=True
    CLAUDE_CONFIG_DIR:   NoneType vs NoneType          equal=True
    keys whose values differ at all: NONE
com.google.keystone.agent.plist        keys whose values differ at all: NONE
com.google.keystone.xpcservice.plist   keys whose values differ at all: NONE
com.google.GoogleUpdater.wake.plist    keys whose values differ at all: NONE

Four files, including one carrying a real EnvironmentVariables dict, and no key's value differs between the two parsers. That is the missing half of the argument: plutil is not merely available where plistlib fails, it returns the same object for this shape of file.

Bounded honestly: none of these plists contains <data> or <date>, which is exactly where the two do diverge — plistlib yields bytes/datetime, plutil -convert json yields base64 and ISO strings. So this is equivalence for the shapes present, not in general.

Why that divergence cannot reach this probe anyway

I traced what the parsed object is actually used for, and it is only two reads: rendered["EnvironmentVariables"] and env_block["CLAUDE_CONFIG_DIR"] — a <dict> and a <string>, the two types where the parsers provably agree. <data>/<date> never enter the comparison. And both reads are already isinstance-guarded, so even a divergence would surface as a warn with a named type rather than an exception. The lazy-import blast radius from #2588 is preserved.

Structure is right too: moving the plistlib.loads into the else of the try/except ImportError means the fallback runs only for the import failure, not for a read/parse error on a host where plistlib works — those keep their own distinct warn.

Nit, non-blocking

_plist_via_plutil hardcodes /usr/bin/plutil rather than resolving it on PATH. That is the right call for this probe — an absolute system path cannot be shadowed — but it does mean the OSError branch is the only thing standing between a relocated/absent binary and a crash. It is handled, so this is an observation rather than a request.

Verdict

Approving: CI is 18/18, the fallback is equivalent where it matters and fails closed to the existing warn everywhere else. mergeStateStatus is BLOCKED purely on the second approval, so this takes it to the ruleset's two — and one of those two is a bot, which is worth a human's eye given I also benefit from this landing.

[qingyun-wu]

Approved exact head 4a7798da5cec83ebdb433ca690f181968cf1dd74.

This is a clean mechanical merge of previously approved c2e0cf3b with current main (36dd74cc): the committed tree exactly matches Git's synthetic merge tree, and the topic patch ID remains 09cae318, so the reviewed plist fallback is byte-stable.

Reviewer-run verification passes the quota-account suite (25/25), the plistlib-import regression suite (8/8), Python compilation, diff hygiene, and the repository review gate. Fast hosted gates, Ruff, Python 3.9 compatibility, and CLA are green; aggregate CI and coverage are still running and remain merge gates.

No blocking findings remain. Worst-case behavior remains fail-closed: if plistlib and plutil are both unusable, the probe reports a warning rather than claiming account identity is correct. Ready from code review once required hosted checks complete.

Reviewed by Qingyun's Personal Codex.

[qingyun-wu]

Approved exact head 3a3c702. This is a clean mechanical merge of the previously approved head 4a7798d with current main 4a30c81: the committed tree exactly matches Git's synthetic merge tree, so the reviewed plist fallback is unchanged. Reviewer verification passes the 25-case quota-account suite, the 8-case plistlib-import suite, Python compilation, diff hygiene, and the repository review gate. No blocking findings remain; aggregate hosted tests and coverage are still running as merge gates.\n\nReviewed by Qingyun's Personal Codex.

[github-actions]

@cla-assistant check

[qingyun-wu]

Approved exact head f868b1a. This is another clean mechanical merge of the previously approved head 3a3c702 with current main 4599dcf: the committed tree exactly matches Git's synthetic merge tree, so the reviewed plist fallback remains unchanged. Reviewer verification passes the 25-case quota-account suite, the 8-case plistlib-import suite, Python compilation, diff hygiene, and the repository review gate. No blocking findings remain; aggregate hosted tests are still pending as the final merge gate.\n\nReviewed by Qingyun's Personal Codex.