fix(menu-bar): validate preserved status item positions

[steipete]

Status-item hide/remove operations could restore a corrupt saved position or retain an invalid replacement written during the operation. Validate the owned position before and after these mutations, restore only a valid prior value, and route split-provider visibility through the same helper. Valid replacement positions remain intact. Reading only the relevant defaults keys removes the old whole-defaults snapshot; the patch reduces production code by two lines.

This is a bounded follow-up to #3355: the original finite-coordinate writer remains unknown and predates the preservation helper. The creation-time repair from #3825 remains unchanged. The new offscreen Icon and Percent tests investigate #2580; they do not establish Sonoma runtime visibility. The distinct macOS-owned Control Center mapping reports in #1711 remain unresolved.

Validation

• Regression red → green: the isolated harness compiles the actual placement sources and actual regression test file, with only logging stubbed and dictionary-backed defaults. Before: 8 tests, 3 failures / 9 assertions. After: all 8 pass. The pre-fix run added only the injectable width parameter, not validation logic.

CODEXBAR_SUPPRESS_TEST_KEYCHAIN_ACCESS=1 swift test \
  --package-path /tmp/statusitem-2-placement-proof \
  --scratch-path .build/placement-proof --build-system native \
  --disable-index-store --jobs 2 \
  --filter MenuBarStatusItemPlacementPreservationTests

• Full repository focused run: 223 tests in 9 suites passed, zero failures, including controller shutdown/recovery, creation ordering, rendering, and the architecture gate.

TMPDIR=/Volumes/CBStatusItem2/tmp \
CODEXBAR_SUPPRESS_TEST_KEYCHAIN_ACCESS=1 \
CODEXBAR_TEST_CODEX_FILE_ISOLATION=1 \
CODEXBAR_TEST_SESSION_FILE_ISOLATION=1 \
CODEXBAR_LAYOUT_VISIBILITY_PROOF_DIR="$PWD/../reports/statusitem-2-proof/renderings" \
swift test --scratch-path /Volumes/CBStatusItem2/build \
  --build-system native --disable-index-store --jobs 2 --no-parallel \
  --filter 'MenuBarStatusItemPlacementPreservationTests|StatusItemCreationOrderingTests|StatusItemControllerShutdownTests|StatusItemControllerSplitLifecycleTests|StatusItemReuseRegressionTests|MenuBarVisibilityWatcherTests|MenuBarLayoutRendererTests|MenuBarLayoutVisibilityTests|ProviderArchitectureGatekeeperTests'

• Synthetic rendering: all 16 captures were inspected. Icon and 50% remain visible at small/regular size, light/dark appearance, tight/regular spacing, and 1×/2× scale. Measured widths are 47–58 points at 22-point height. This ran on macOS 27 using the shared macOS 14-compatible code path, not on Sonoma itself.
• TMPDIR=/Volumes/CBStatusItem2/tmp make check: passed. Process-cleanup checks: 102 tests, one skipped. SwiftFormat: 0/2672 files require formatting, 6 files skipped. SwiftLint: 0 violations, 0 serious in 2671 files.
• Independent Codex review through P2: scoped-clean.
• CI: 36367704061 succeeded for cae4291dc1ddae37bb8197e9f05a626bf0b12bcb.

Two earlier SSD builds were interrupted during filesystem-write stalls before selected tests executed. An unchanged process-cleanup fixture also hit timing bounds on the busy SSD; it and the complete check passed unchanged with RAM-backed temporary storage. No production preferences, live account probes, or running-app relaunches were used.

Refs #3355
Refs #2580
Refs #1711

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: blocked before merge. Reviewed September 27, 2026, 11:34 PM ET / September 28, 2026, 03:34 UTC (Revision 2).

ClawSweeper review

What this changes

The branch validates saved menu-bar positions around status-item visibility and removal, routes split-provider visibility through that check, and adds placement and offscreen rendering tests.

Merge readiness

⛔ Blocked before merge - 3 items remain

Current main still restores a corrupt saved position after a status item is hidden or removed, so this focused repair remains useful. The patch has no identified code defect, but its effect on existing persisted positions needs native upgrade evidence before merge.

Priority: P2
Reviewed head: cae4291dc1ddae37bb8197e9f05a626bf0b12bcb
Owner decision: Required. See Decision needed.

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	The focused patch and regression tests provide useful evidence, while persisted-preference upgrade behavior remains unverified.
Proof confidence	🦐 gold shrimp (3/6)	Not applicable: The owner-authored PR is exempt from contributor live-proof requirements. Its changed preservation helper is reached by native status-item hide and removal; reported isolated macOS tests exercise seeded dictionary defaults after the fix, but no installed-app trace verifies existing persisted positions through upgrade and mutation. Stored-default compatibility evidence remains insufficient.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Not applicable	Not applicable: The owner-authored PR is exempt from contributor live-proof requirements. Its changed preservation helper is reached by native status-item hide and removal; reported isolated macOS tests exercise seeded dictionary defaults after the fix, but no installed-app trace verifies existing persisted positions through upgrade and mutation. Stored-default compatibility evidence remains insufficient.
Evidence reviewed	9 items	Current-main gap: The current helper saves the prior defaults value and restores it when a status-item mutation clears the key, without first validating that value. Introduced repair: The PR validates the owned saved position before and after a mutation and restores only a prior value that passed validation. Native call path: The controller's visibility and removal helpers call the changed preservation helper; the introduced hunk also routes split-provider visibility through it.
Findings	None	None.
Security	None	None.

How this fits together

CodexBar creates macOS menu-bar items for provider usage displays. AppKit saves their positions in app defaults, and the status-item controller uses those values when it shows, hides, removes, or recreates an item.

flowchart LR
 A[Saved menu position] --> C[Position validation]
 B[Attached display widths] --> C
 C --> D[Show hide or remove item]
 D --> E[Validate and restore position]
 E --> A
 D --> F[Menu bar display]

Loading

Decision needed

Question	Recommendation
Should the existing display bound also clear saved positions during runtime visibility and removal, including positions parked by a menu-bar manager or saved before a display change?	Accept after upgrade proof: Keep the runtime bound after a native upgraded-preference trace shows what happens to valid and out-of-bound positions.

Why: The earlier owner decision accepted this bound at item creation, but did not explicitly cover the new runtime clearing points.

Before merge

• Resolve merge risk (P1) - Runtime visibility changes can now clear an existing saved position that exceeds the currently attached display bound. Dictionary-backed tests do not establish the result for persisted positions through an installed-app upgrade or display change.
• Complete next step (P2) - Verify valid and corrupt persisted positions in a freshly built app across upgrade, display change, and hide or removal; complete make test, then resolve the runtime-clearing compatibility choice before merge.
• Resolve maintainer decision - Resolve the maintainer decision shown above before merge.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production and test delta	production −2 lines, tests +149 lines	The production change is small and supported by focused placement and rendering coverage.

Merge-risk options

Maintainer options:

1. Verify existing placements (recommended)
   Capture a redacted installed-app trace across upgrade, display change, and hide or removal for both valid and corrupt saved positions before accepting the runtime clearing rule.
2. Narrow runtime validation
   If parked positions must survive runtime mutations, restrict those mutations to structurally invalid values and leave the display-bound repair at creation.

Technical review

Best possible solution:

Keep the shared validation boundary while demonstrating that valid persisted placements survive upgrade, hide, removal, and display changes, and that only corrupt positions are cleared.

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

Yes at source level: seed an invalid saved position and clear its key during the existing hide or removal helper; current main restores that invalid value. The original finite-coordinate writer has not been identified in a native trace.

Is this the best way to solve the issue?

Yes for the mutation-time defect: reusing the established validator in the shared helper is a narrow repair. Persisted-preference upgrade behavior still needs verification.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 579f68406855.

Labels

Label changes:

No label changes.

Label justifications:

• P2: This is a bounded repair for menu-bar placement failures affecting a limited set of users and display setups.
• merge-risk: 🚨 compatibility: The introduced runtime checks may clear previously saved menu-bar positions before the next item creation, and upgraded-preference behavior remains unverified.
• rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The owner-authored PR is exempt from contributor live-proof requirements. Its changed preservation helper is reached by native status-item hide and removal; reported isolated macOS tests exercise seeded dictionary defaults after the fix, but no installed-app trace verifies existing persisted positions through upgrade and mutation. Stored-default compatibility evidence remains insufficient.

Evidence

What I checked:

• Current-main gap: The current helper saves the prior defaults value and restores it when a status-item mutation clears the key, without first validating that value. (Sources/CodexBar/MenuBarStatusItemPlacementPreservation.swift:20, 579f68406855)
• Introduced repair: The PR validates the owned saved position before and after a mutation and restores only a prior value that passed validation. (Sources/CodexBar/MenuBarStatusItemPlacementPreservation.swift:20, cae4291dc1dd)
• Native call path: The controller's visibility and removal helpers call the changed preservation helper; the introduced hunk also routes split-provider visibility through it. (Sources/CodexBar/StatusItemController+StatusItemVending.swift:51, cae4291dc1dd)
• Regression coverage: The new cases seed valid and invalid positions, check replacement and removal during a mutation, and verify that repair stays scoped to the owned identity. (Tests/CodexBarTests/MenuBarStatusItemPlacementPreservationTests.swift:9, cae4291dc1dd)
• Persisted-default proof boundary: The placement tests use a dictionary-backed defaults substitute that has no persistent writes or Foundation defaults search-domain fallback; the PR body reports isolated and focused test runs, but no installed-app upgrade trace. (Tests/CodexBarTests/TestStores.swift:8, cae4291dc1dd)
• Prior placement policy and remaining issue: In the linked issue, the owner explicitly accepted clearing menu-manager parking positions beyond the bound at creation time and said the writer of recurring finite corruption remains unknown. This PR extends validation to runtime mutations.

Likely related people:

• steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• kratocz: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• Jackson Dimiglio-Wood: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Record redacted installed-app before-and-after evidence for valid and corrupt persisted positions across upgrade and hide or removal.
• Complete make test and report its result; the PR already reports make check passing.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

• 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.

History

Review history (1 earlier review cycle)

• reviewed 2026-09-28T01:56:58.218Z sha cae4291 :: blocked before merge. :: none

[steipete]

Landed on main as 3dad9da via merge train #4096 (one green CI run for the whole train).