test(desktop): isolate Rewind owner snapshot authority - #12192
Conversation
Reproduce the revoked process-wide owner state, then establish and restore the test owner through RuntimeOwnerAuthorityTestFixture so suite order cannot turn authenticated capture into anonymous capture. Verification: RewindCaptureExclusionGenerationTests 9/9 passed; deterministic contamination recovery passed 50/50 runs; the repaired test passed inside two 5,769-test process runs (each full run retained one unrelated baseline failure). Failure-Class: FC-hand-listed-test-isolation-membership
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Thanks @aryanorastar — verified end to end against the current authorization implementation; this is a clean fix for the #12039 flake signature. What I checked in
Test-only change, no production behavior touched — leaving for human maintainer review before merge. by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
Git-on-my-level
added
positive-signal
kodjima33
left a comment
kodjima33
left a comment
There was a problem hiding this comment.
Test-isolation fix, low risk (test-file only, zero prod code touched), verified real and not already fixed. Bug-fix fast-lane heuristic scores 2/5 (below the 3/5 owner-override bar) and title doesn't match fix(desktop)/fix(macos)/fix(windows), so holding at approve rather than owner-override merge.
|
@undivisible this one is ready for a merge press — approved by @kodjima33, all checks green, no conflicts. It just needs someone to click, since the bug-fix fast lane scored it 2/5 (below the 3/5 owner-override bar) and held at approve. On the title: Two others in the same neighbourhood while you're here, both green with zero conflicts:
#12288 is the one I'd take first: it's six lines, it's a break on #12259 is worth a glance as a pair with this one — same root cause ( No rush on any of them. |
"merge my shit like a good boy" ahh guy ok i will also idc if you kinda name shit a bit different its obvious that you can write test its all good |
i was trying to reach you on discord wanna learn a lot from you my id is astar6969 @undivisible |
…12650) `SuggestedTasksStoreTests.testNotMineAndAlreadyHandledPersistReasonAndResolveCandidate` is one of the flakes tracked in #12039. It failed in CI on an unrelated backend PR with: ("nil") is not equal to ("Optional(…TaskIntelligenceFeedbackReason.not_mine)") The store resolves two owner authorities. `suppressionStore` was injected, but `feedbackOutboxStore` was left on the default `SuggestedFeedbackOutboxDefaults`, whose owner is process-global: fixedOwnerID ?? defaults.string(forKey: .authUserId) ?? "signed-out" `dismiss` re-checks owner currency *after* awaiting the backend reject. Any other suite in the same binary that writes `auth_userId` while that await is suspended flips the owner, so `dismiss` returns early without recording feedback — the assertion above sees an empty `api.feedback`. 34 other test files touch that key, which is why the failure tracks suite order rather than anything in this suite. 27 of the 37 store constructions in this file were half-isolated this way; the other 10 already injected a `MemoryFeedbackOutboxStore` because they assert on outbox contents. All 37 now pin both owners to the suite, which is the same repair #12192 and #12259 made for the Rewind and FloatingBar owner authorities. `testDismissSurvivesAGlobalAuthOwnerFlipAtTheAwaitedRejectPoint` pins the contract. It drives the flip through the existing `onReject` hook — the exact suspension point — so the hazard is deterministic rather than order-dependent. Verification: 40/40 in this suite; 99/99 with the owner and auth suites that write `auth_userId` in one process. Mutation-checked: dropping the `feedbackOutboxStore` injection from the new test reproduces CI's failure byte for byte, `("nil") is not equal to ("Optional(Omi_Computer.OmiAPI.TaskIntelligenceFeedbackReason.not_mine)")`, and restoring it returns green. This is one leg of #12039. `KernelTurnRecordedProjectionTests` remains open; the chat gesture, Rewind owner, and FloatingBar owner legs landed in #12406, #12192 and #12259. Failure-Class: none
4 participants
What changed and why
Addresses the Rewind/auth occurrence reported in #12039. The regression test changed
auth_userIddirectly even though runtime owner authorization is process-wide and deliberately revokes out-of-band owner changes. That made the result depend on which owner-bound suite ran first and produced the observedanonymousversusauth-leading-…failure.The test now recreates that revoked-authority state deliberately, then establishes and restores its owner through
RuntimeOwnerAuthorityTestFixture. This follows the same serialized transition boundary as production and automatically places the suite in the runner's derived owner-isolation cluster.Product invariants affected
none
How it was verified
xcrun swift test --package-path desktop/macos/Desktop --filter 'RewindCaptureExclusionGenerationTests'— 9/9 passed.--skip-build— 50/50 passed.This is test-harness isolation only; there is no user-facing path or physical-device behavior to exercise.
Tests
RewindCaptureExclusionGenerationTests.testOwnerSnapshotStaysCurrentWhenAuthLeadsUnresolvedRewindDatabasenow starts from the exact revoked authority that previously arrived nondeterministically, then proves the auth-leading owner remains current whileRewindDatabase.currentUserIdis unresolved.Failure class (fixes)
Failure-Class: FC-hand-listed-test-isolation-membership