Skip to content

Harden teardown guards and keychain no-UI query handling#381

Open
artuskg wants to merge 10 commits intosteipete:mainfrom
artuskg:codex/fix-menu-refresh-teardown-warning
Open

Harden teardown guards and keychain no-UI query handling#381
artuskg wants to merge 10 commits intosteipete:mainfrom
artuskg:codex/fix-menu-refresh-teardown-warning

Conversation

@artuskg
Copy link

@artuskg artuskg commented Feb 16, 2026

Summary

This PR hardens teardown and refresh behavior around StatusItemController and fixes keychain no-UI query handling that could regress into interactive prompt behavior.

Errors Found and How to Reproduce / Confirm Independently

1) Keychain UI-fail policy value mismatch

Issue:

  • A raw literal string ("kSecUseAuthenticationUIFail") was used instead of Security’s actual constant value.
  • This can cause kSecUseAuthenticationUI to be set incorrectly and invalidate the non-interactive policy.

Independent confirmation:

  • Run swift -e 'import Security; print(kSecUseAuthenticationUIFail)' and compare output to the literal string.
  • Run swift test --filter KeychainNoUIQueryTests.

2) Linux build break from unconditional Darwin import

Issue:

  • import Darwin was unconditional in CodexBarCore keychain helper code.
  • Linux CI that builds CLI/Core can fail with module import errors.

Independent confirmation:

  • Build Core/CLI on Linux (or Linux CI path) and confirm compilation fails without the guard.

3) Post-teardown UI mutation race in tests

Issue:

  • After releaseStatusItemsForTesting(), observer/task paths could still execute UI-mutating code paths (handleSettingsChange, animation/blink/menu refresh paths).
  • This can recreate/remove status items or mutate menus/images after teardown.

Independent confirmation:

  • Run status/menu/animation suites with real .system status bar (StatusMenuTests, StatusItemAnimationTests, BatteryDrainDiagnosticTests) and observe teardown-sensitive behavior.

What Changed and Why

Keychain / no-UI safety

  • Restored strict no-UI query semantics in preflight.
  • Added makeGenericPasswordPreflightQuery(service:account:) to centralize and test query construction.
  • Switched UI-fail policy resolution to runtime Security symbol lookup.
  • Guarded Darwin import for non-macOS builds.
  • Added/expanded KeychainNoUIQueryTests for structural and behavior-level verification.

Why:

  • Ensure preflight queries stay non-interactive and avoid accidental UI prompts.
  • Preserve Linux build compatibility while keeping macOS behavior correct.

Status menu/animation teardown hardening

  • Added deterministic test teardown hook (releaseStatusItemsForTesting()) and cancellation/cleanup of tasks/state.
  • Added release guard (isReleasedForTesting) and applied it to observer/animation/menu mutation entry points.
  • Hardened menu refresh gating and delayed refresh checks when refresh is disabled.
  • Serialized AppKit status-bar-sensitive suites and standardized .system status bar usage in those tests.
  • Added maintainer note documenting runtime toggle semantics for menuRefreshEnabled.

Why:

  • Prevent post-teardown mutations and racey AppKit interactions.
  • Reduce flaky behavior in status item/menu tests while preserving production behavior.

Validation Run

  • pnpm check
  • ./Scripts/compile_and_run.sh
  • swift test

All passed on this branch.

Review / Tooling Provenance

  • Review and fix cycle used gpt-5.3-codex high and RepoPrompt Review.
  • Changes were iterated with targeted review findings and then validated with full lint/build/test execution to reach minimum quality before PR.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 71451a3e94

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +499 to +500
self.blinkTask?.cancel()
self.loginTask?.cancel()

Choose a reason for hiding this comment

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

P2 Badge Block login task callbacks after test teardown

releaseStatusItemsForTesting() only cancels loginTask, but an in-flight switch-account task still runs its defer (activeLoginProvider = nil, loginTask = nil), and those property observers call refreshMenusForLoginStateChange() which still reaches attachMenus() without an isReleasedForTesting guard. In tests that tear down while login is in progress, this can recreate/mutate status items after teardown and reintroduce the post-release AppKit mutation race this change is trying to prevent.

Useful? React with 👍 / 👎.

Choose a reason for hiding this comment

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

Addressed in 428fd68.

What changed:

  • Added a DEBUG release guard in refreshMenusForLoginStateChange() so login-task/property-observer callbacks no-op after releaseStatusItemsForTesting().

Added regression test:

  • loginStateCallbacksDoNotAttachMenusAfterRelease() in Tests/CodexBarTests/StatusMenuTests.swift to verify login-state callbacks after release do not reattach menus or recreate status items.

Validation rerun:

  • pnpm check
  • ./Scripts/compile_and_run.sh
    Both passed.

Copy link

Open questions to align on before/while merging:

  1. menuRefreshEnabled contract
    Should /Sources/CodexBar/StatusItemController.swift treat menuRefreshEnabled as strictly test-only, or support runtime toggling in production too?
    If runtime toggling is supported, should we backfill tracking/scheduling when it is re-enabled while a menu is already open?

  2. Observer teardown model
    Keep current NotificationCenter removal only in deinit (lint-compliant), or refactor to token-based observers so test teardown can fully detach observer callbacks explicitly?

  3. Release-guard scope policy
    Should isReleasedForTesting be a hard kill-switch for all controller mutation paths, or only selected high-risk entry points?

  4. Keychain fallback observability
    In /Sources/CodexBarCore/KeychainNoUIQuery.swift, do we want debug logging when dlopen/dlsym fallback ("u_AuthUIF") is used, to improve field diagnostics?

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

Comments