Skip to content

keychain CI: an unusable Keychain may refuse before the state folder exists - #456

Merged
plonkus merged 1 commit into
mainfrom
keychain-check-no-folder
Nov 9, 2026
Merged

plonkus merged 1 commit into
mainfrom
keychain-check-no-folder

Conversation

@plonkus

@plonkus plonkus commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

Goal: The keychain workflow on main passes again. Its "An unusable Keychain fails clearly" check has failed on every run since 2026-10-30.

Outside users today: None. This changes CI only.

Minimal solution for that user count: List the state folder only if it exists. The check that matters, that no key file was written, stays as it was.

Complexity added: One shell conditional in the workflow.

Size: Expected net LOC: +1. Actual: +1 net (+2/−1).

Why

The step runs a signed latch as a LaunchDaemon, with no usable Keychain, and expects keychain_failed and no key file. latch still does exactly that.

Since #332 (login on a logged-in folder, login --again), login reads the saved Plow token first. Without a usable Keychain it now refuses at that read, before it creates the state folder:

  • before: the Keychain refused to save …/daemon-state/key (status -25291), and the folder existed;
  • now: the Keychain refused to read …/daemon-state/plow-token (status -25291), and the folder doesn't exist.

The step then ran sudo ls -la "$state" unconditionally, and under bash -e that ls failed the step: ls: …/daemon-state: No such file or directory, run 42681375631. The keychain_failed grep before it had passed.

What changed

  • .github/workflows/keychain.yml: list the state folder only when it exists. The keychain_failed grep and the "a key file was written" check are unchanged.

What it doesn't do

It doesn't change latch. Refusing earlier, at the token read, is still a clear failure that writes no files.

The workflow runs only on main, in the release environment, so the real signed run happens after merge. Before that, the step's logic was checked locally for an absent folder, an empty folder and a folder holding a key, and actionlint is clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01RFixJTbcqzMj4yVHDwaQCw

…exists

Since login reads the saved token first, a signed latch with no usable
Keychain refuses on that read, before it makes the state folder. The
check listed the folder unconditionally, so `ls` failed the step even
though latch had failed clearly and written no key. List it only when it
is there; the no-key-file check is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RFixJTbcqzMj4yVHDwaQCw
@srosro

srosro commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

📋 First review of this PR — reviewed 7bcd271. 🧪 Tests not run. ✅ Prior-art (KID) checked. 🔍 8 sibling repos not searched (8 allowlist; names withheld on a public repo). ⚡ Small-diff lane (3 lines): skipped security, data-integrity, architecture-refined, tests, consumers — screened by aggregator. 📐 Complexity budget unchecked — the PR body is missing Goal, Outside users today, Minimal solution for that user count, Complexity added, Size: Expected net LOC; fill in those PR-template fields so the next review can check the budget.

This PR has no inferable end-user-facing intent — it appears to be a CI-only fix in .github/workflows/keychain.yml, allowing the unusable-Keychain check to pass when login correctly refuses before creating the state folder — reviewing against that goal.

Overview — The PR makes the diagnostic directory listing conditional while preserving both substantive assertions: keychain_failed must appear, and no key file may exist. The change matches the current login path: state.load_token() at latch/src/login.rs:94 can fail before state.create() at line 151. One local conditional fits the repository’s operating point: a pre-PMF internal tool with one engineer and a handful of hosts.

Strengths — The fix accommodates an earlier valid refusal without creating artificial state or suppressing listing errors with || true. Both security assertions remain outside the new conditional, so an absent directory does not bypass them.

Probes

None.

Security — None.

Test coverage — The staged just test attempt stopped with exit 127 because cargo-nextest is missing; this is a reviewer-environment limitation, not evidence of a PR regression. The author reports local checks for absent, empty, and key-containing directories plus clean actionlint; the signed macOS workflow remains unverified here and runs on main after merge.

For AI authors — (Codex, Claude Code, Cursor, etc. reading this PR): The Probes above are load-bearing. Treat each [open] probe as a hard requirement — answer it directly in your reply or revisit the structural decision. Do NOT silently absorb a probe by adding more code; that path inverts the cost stance the probe is encoding. This repo's operating point (see its REVIEW.md) prefers cutting LOC over adding it; an [open] [simplification] probe whose answer is "no, this complexity isn't needed" should land as a deletion in your next push, not a new defensive guard.


How to use: auto-reviews every new PR and re-reviews after a period of inactivity. Trigger an incremental re-review with /srosro-update-review, or a whole-PR re-review with /srosro-review.

For humans only: push-access collaborators can post:

  • /srosro-approve — APPROVE the PR.
  • /srosro-props [from: <specialist>] — give props to a specialist's contribution.
  • /srosro-critique [from: <specialist>] — flag a specialist's contribution as a misread.
  • /srosro-memorize <feedback> — teach a calibration lesson (learn-from-replies updates COMMENT_REVIEW_MISTAKES.md from your body, sentiment-aware via LLM).

Props: /srosro-props [from: shape] caught a real layering bug we'd have shipped.
Critique: /srosro-critique [from: architecture-refined] DRY suggestion misread distinct seams.
Calibration: /srosro-memorize the architecture-refined DRY finding was a misread; those helpers serve different contracts.

AI agents must not use /srosro-memorize, /srosro-props, or /srosro-critique — those signals tune shared global state.

Generated by sam's ai review bot.

@srosro srosro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving per automated review above.

@plonkus
plonkus merged commit 0954b4b into main Nov 9, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants