chore: address non-blocking review nits from #27 and #30#32
chore: address non-blocking review nits from #27 and #30#32smartwatermelon merged 1 commit intomainfrom
Conversation
- prep-airdrop.sh: only echo "Provisioning..." when token is present (previously printed before the nil-check, creating misleading output when the dev Keychain lacks the token) — closes #28 - first-boot.sh: clarify -u flag semantics for security set-keychain-settings — the no-timeout behavior comes from omitting -t, not from -u alone — closes #31 - CLAUDE.md: note that opp is a local shell function from dotfiles, not a standard macOS/Homebrew tool — closes #29 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
PR Review: chore: address non-blocking review nits from #27 and #30Three focused changes, all low-risk. Reviewed against the diff only. prep-airdrop.sh — echo moved inside nil-check (line 671) Improvement, no regression. Moving the echo inside the if block means the message is now printed only when provisioning actually occurs. Previously it printed unconditionally before the nil-check, so operators running on a machine where the dev Keychain item was absent would see the message followed by silence — suggesting work happened when it didn't. The fix makes the log truthful. Logic is unchanged. scripts/server/first-boot.sh — inline comment on security set-keychain-settings flags (after line 491) Documentation only. The added comment correctly explains why -l -u without -t N produces lock-on-sleep with no idle timeout: -u enables the idle-lock feature but the timeout is left unset, which security treats as disabled. This matches the existing show_log message. No code touched. CLAUDE.md — opp provenance note (after line 36) Documentation only. Clarifies that opp is a local shell function from dotfiles (~/.config/bash/1password.sh), not a Homebrew or macOS tool. Important context for anyone running dev commands cold. Non-blocking observations: None. All three changes are tight and correct. VERDICT: PASS |
Summary
Wraps up the three non-blocking issues filed by the pre-merge analyzer on the recent keychain PRs:
Closes #28, closes #29, closes #31.
Test plan
🤖 Generated with Claude Code