Skip to content

Defer status-menu quit during shutdown - #1354

Merged
steipete merged 1 commit into
steipete:mainfrom
jskoiz:fix/defer-status-menu-quit
Jun 8, 2026
Merged

Defer status-menu quit during shutdown#1354
steipete merged 1 commit into
steipete:mainfrom
jskoiz:fix/defer-status-menu-quit

Conversation

@jskoiz

@jskoiz jskoiz commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Summary

  • defer the status-menu Quit path by one main-loop turn after canceling active menu tracking
  • run existing shutdown cleanup before terminating from that deferred Quit path
  • add a regression test that Quit schedules cleanup/termination instead of terminating synchronously

Fixes #1353. Follow-up to #1174.

Verification

  • swift test --filter 'AppDelegateTests|StatusItemControllerShutdownTests'
  • git diff --check

Affected-machine Proof

  • Installed the Dock-only PR branch 7f4bac64 over /Applications/CodexBar.app.
  • Verified the installed binary SHA-256 matches the repo-packaged app binary: 6a9999f82f284b3b4d69fd644a6c9e141edf63b3792b12fda7c037593bb6c30b.
  • Invoked Quit via CodexBar's status-menu Quit item on the affected Mac.
  • Confirmed CodexBar exited, Dock autohide remained enabled, Dock PID stayed unchanged, and Dock accessibility still responded after moving the pointer to the Dock edge.
  • Full terminal transcript: Defer status-menu quit during shutdown #1354 (comment)

Notes/Risks

  • This keeps the existing prepareForAppShutdown() cleanup from 589b77ca; it only adds the status-menu/AppKit ordering piece that was missing from the Quit action.
  • The affected-machine proof from Quitting CodexBar can leave Dock autohide stuck until Dock is restarted #1174 was: cleanup-only still reproduced, cleanup plus deferred status-menu Quit fixed Dock autohide.
  • I also checked swift test --filter StatusMenuTests; current main in this checkout has an unrelated failure in StatusMenuClosedPreparationTests.swift:48 (controller.menuVersions[key] is 2 instead of nil). The targeted shutdown tests for this patch pass.

@clawsweeper

clawsweeper Bot commented Jun 7, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed June 7, 2026, 5:57 PM ET / 21:57 UTC.

Summary
The branch changes the status-menu Quit action to cancel active menu tracking, defer cleanup and termination by one main-loop turn, and adds a regression test for that deferred shutdown path.

Reproducibility: yes. for the reporter's affected setup: the linked issue describes the current status-menu Quit wedge, and the PR comment provides installed-build terminal proof that the branch avoids it. I did not run live macOS validation in this read-only review.

Review metrics: 2 noteworthy metrics.

  • Changed surface: 3 files, +78/-1. The patch is narrowly scoped to the status-menu Quit path, one injectable scheduling seam, and one focused shutdown test.
  • Runtime proof: 1 affected-machine terminal transcript. The new proof addresses the prior real-behavior gate for the Dock autohide regression.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Risk before merge

  • [P1] This changes app termination ordering, and the Dock/AppKit side effect is not fully proven by unit tests; the provided affected-machine transcript is the main runtime evidence.

Maintainer options:

  1. Land with the affected-machine proof (recommended)
    Accept the small shutdown-ordering risk after preserving the provided installed-build transcript and targeted test coverage in the PR record.
  2. Ask for visual confirmation
    If maintainers want stronger confidence, request a short redacted recording showing status-menu Quit followed by Dock autohide reveal without a Dock restart.

Next step before merge

  • No ClawSweeper repair lane is needed; the remaining action is normal maintainer merge review with the updated runtime proof.

Security
Cleared: The diff only changes local AppKit shutdown control flow and tests; it does not touch dependencies, credentials, CI, release scripts, or other supply-chain-sensitive surfaces.

Review details

Best possible solution:

Land the narrow deferred status-menu Quit path with the regression test and keep the linked issue open until the PR is merged or rejected.

Do we have a high-confidence way to reproduce the issue?

Yes for the reporter's affected setup: the linked issue describes the current status-menu Quit wedge, and the PR comment provides installed-build terminal proof that the branch avoids it. I did not run live macOS validation in this read-only review.

Is this the best way to solve the issue?

Yes. The PR is a narrow fix that preserves the existing shutdown cleanup and adds only the missing status-menu/AppKit deferral, with a state-seam regression test aligned with repository guidance.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against db184430bc4b.

Label changes

Label changes:

  • add merge-risk: 🚨 availability: The PR changes how the app exits from the status menu, and CI cannot fully prove the Dock/AppKit runtime side effect.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR now includes sufficient terminal proof from the affected installed build showing the exact status-menu Quit path and Dock behavior after the fix.
  • add rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR now includes sufficient terminal proof from the affected installed build showing the exact status-menu Quit path and Dock behavior after the fix.
  • remove status: 📣 needs proof: Current PR status label is status: 👀 ready for maintainer look.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🦞 diamond lobster, so this older rating label is no longer current.

Label justifications:

  • P2: This is a normal-priority user-facing macOS shutdown bug with limited blast radius around status-menu Quit.
  • merge-risk: 🚨 availability: The PR changes how the app exits from the status menu, and CI cannot fully prove the Dock/AppKit runtime side effect.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR now includes sufficient terminal proof from the affected installed build showing the exact status-menu Quit path and Dock behavior after the fix.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR now includes sufficient terminal proof from the affected installed build showing the exact status-menu Quit path and Dock behavior after the fix.
Evidence reviewed

What I checked:

  • Current main quit path: Current main still calls NSApp.terminate(nil) directly from StatusItemController.quit(), so the deferred status-menu ordering is not already implemented on main. (Sources/CodexBar/StatusItemController+Actions.swift:330, db184430bc4b)
  • PR shutdown ordering: The PR cancels currently tracked menus, schedules a deferred main-actor operation, then runs prepareForAppShutdown() before terminateApplicationForQuit(). (Sources/CodexBar/StatusItemController+Actions.swift:331, 7f4bac64e623)
  • PR test coverage: The new regression test asserts Quit schedules termination without immediate cleanup, then the scheduled operation prepares shutdown, clears the status item menu, and terminates. (Tests/CodexBarTests/StatusItemControllerShutdownTests.swift:61, 7f4bac64e623)
  • Existing cleanup reused: Current main's prepareForAppShutdown() is idempotent and already cancels menu tracking, clears menu state, removes status items, and closes the credits window; the PR reuses that path instead of duplicating cleanup. (Sources/CodexBar/StatusItemController+Shutdown.swift:4, db184430bc4b)
  • Whitespace check: git diff --check between current main and the PR head produced no output. (7f4bac64e623)
  • Affected-machine proof: The PR body and linked comment include a terminal transcript from the affected installed build showing matching binary SHA-256, codesign verification, status-menu Quit invocation, CodexBar exit, unchanged Dock PID, and Dock accessibility response after pointer movement. (7f4bac64e623)

Likely related people:

  • Peter Steinberger: git show shows 589b77c introduced prepareForAppShutdown() and applicationWillTerminate cleanup, and blame ties the current quit/shutdown snapshot in this checkout to the v0.32.4 history. (role: shutdown cleanup feature history; confidence: high; commits: 589b77ca83a3, 723734ef3422; files: Sources/CodexBar/StatusItemController+Shutdown.swift, Sources/CodexBar/CodexbarApp.swift, Sources/CodexBar/StatusItemController+Actions.swift)
  • Larry Hao: git blame shows 65e39f4 recently touched adjacent menu deferral and shutdown menu-state cleanup fields in the same controller area. (role: recent adjacent menu-state contributor; confidence: medium; commits: 65e39f4dcb3a; files: Sources/CodexBar/StatusItemController.swift, Sources/CodexBar/StatusItemController+Shutdown.swift)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. labels Jun 7, 2026
@jskoiz

jskoiz commented Jun 7, 2026

Copy link
Copy Markdown
Contributor Author

Installed-build validation from the affected machine:

  • Built this PR branch at 7f4bac64.
  • Installed it over /Applications/CodexBar.app after backing up the prior app to /Applications/CodexBar.app.preinstall.20260607110429.
  • Verified the installed bundle with codesign --verify --deep --strict.
  • Launched installed version 0.32.5 build 80 from /Applications/CodexBar.app.
  • Quit CodexBar from the installed PR build and confirmed Dock autohide/reveal remained functional without needing killall Dock.

This is the affected-machine manual check that the cleanup-only path was missing.

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

Copy link
Copy Markdown

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: 8d1cde897b

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

let total = models.values.reduce(0) { partial, packed in
partial + (packed[safe: 0] ?? 0) + (packed[safe: 2] ?? 0)
}
return total > Self.codexMaxTokensPerFileDay ? day : nil

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Avoid dropping high-volume valid Codex days

This cap is applied unconditionally in both full and incremental Codex scans, so any real session file whose input+output exceeds 50M tokens in a local day (including non-forked sessions or forks with a resolved parent) is removed from days and all matching rows; the usage report then silently shows no tokens/cost for that valid work. The anomalous replay case should be gated on the unresolved-fork/replay context or another targeted signal rather than a blanket per-file token ceiling.

Useful? React with 👍 / 👎.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jun 7, 2026
@jskoiz
jskoiz force-pushed the fix/defer-status-menu-quit branch from 8d1cde8 to 7f4bac6 Compare June 7, 2026 21:40
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jun 7, 2026
@jskoiz

jskoiz commented Jun 7, 2026

Copy link
Copy Markdown
Contributor Author

Affected-machine proof for the Dock/status-menu Quit fix only, from the installed PR build at 7f4bac64:

Affected-machine Dock proof for CodexBar PR #1354
timestamp=2026-06-07T21:51:53Z
pwd=/Users/jk/Documents/Codex/2026-06-07/files-mentioned-by-the-user-screenshot/work/CodexBar
repo_root=/Users/jk/Documents/Codex/2026-06-07/files-mentioned-by-the-user-screenshot/work/CodexBar
branch=fix/defer-status-menu-quit
head=7f4bac64e623
status=0 changed paths
installed_bundle=/Applications/CodexBar.app
repo_packaged_sha256=6a9999f82f284b3b4d69fd644a6c9e141edf63b3792b12fda7c037593bb6c30b
installed_sha256=6a9999f82f284b3b4d69fd644a6c9e141edf63b3792b12fda7c037593bb6c30b
codesign_verify=pass
dock_autohide=1
dock_pid_before=91114
codexbar_pid_before=40689
status_menu_quit_invocation=System Events click menu item Quit of CodexBar status menu
codexbar_exit_after_status_menu_quit=true
dock_pid_after_quit=91114
dock_restarted=false
dock_accessibility_after_quit_pointer_away=children=22 pos=2811169 size=123873
dock_accessibility_after_quit_pointer_at_edge=children=22 pos=2811155 size=123873
result=pass

This verifies the installed app binary matches the repo-packaged build, the status-menu Quit item exits CodexBar, and Dock autohide remains active without a Dock restart. The Dock accessibility position changes after moving to the Dock edge, while the Dock PID remains unchanged.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 7, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 7, 2026
@steipete
steipete merged commit 7b6b02d into steipete:main Jun 8, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dock autohide can still wedge after status-menu Quit

2 participants