I remember that we did a migration between keychains for macOS. It was done in the main project, see https://github.com/cryptomator/cryptomator/blob/a7b8541912adce80744eed1640abd98991c32420/src/main/java/org/cryptomator/ui/preferences/GeneralPreferencesController.java#L103-L121.

Or is there a reason that the migration cannot be done on a higher level?

⚠️ Potential issue | 🟠 Major

Replace assertion with proper error handling.

Using assert for error checking in production code is an anti-pattern because assertions can be disabled via the -da JVM flag. If the replace operation fails and assertions are disabled, execution will continue silently past a critical error.

 					var itemProps = Item.createProperties(label, newAttribs);
 					var replace = collection.createItem(itemProps, secret, true);
-					assert replace.isSuccess() : "Replacing migrated item failed";
+					if (!replace.isSuccess()) {
+						LOG.error("Replacing migrated item {} failed: {}", i.getPath(), replace.error());
+						continue;
+					}
 					item.delete();

In src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java
around line 185, replace the assertion "assert replace.isSuccess() : ..." with
explicit error handling: check if replace.isSuccess() is false, log an error
with context and any available details from the replace result (e.g.
replace.toString() or a getError/getMessage accessor), and then fail-fast by
throwing an appropriate unchecked exception (IllegalStateException or a specific
custom exception) so failure cannot be skipped when assertions are disabled;
ensure any necessary cleanup or rollback is performed before throwing.

@purejava What is the reason not checking the success status of collection.createItem() ?

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

@purejava What is the reason not checking the success status of collection.createItem() ?

Yes, this should be checked, there is no reason to omit such a check. But, probably we do not need SecretServiceKeychainAccess#migrateKDEWalletEntries() at all, when we use GeneralPreferencesController#migrateKeychainEntries() instead, as you noted.

I wasn't aware of this feature and I am about to test, whether migrateKeychainEntries() does work with all keychain backends on Linux, once the method is enabled for Linux as well.

As the bot pointed out: Do we require to disable the tests if no DISPLAY is available?

Hi @infeo, I understand the bot to mean, that DISPLAY is not set for Wayland and the guard only works for x server sessions.

And I understand your question to mean, whether to disable the guard, when DISLPLAY is not set?

To my understanding, the guard already checks that: when DISPLAY contains anything, so an x server is available, the test is run.

The guard checks, whether we are on a system with a display and not on CI, so in the end whether DBus is available, right? So my suggestion is to use @EnabledIfEnvironmentVariable(named = "DBUS_SESSION_BUS_ADDRESS", matches = ".*") instead.

The bot pointed out, that the "is-supported" method (and all other methods), do not depend on the DISPLAY variable.

I don't know Linux good, but to my understanding DBus is not tied to a graphical user interface. It is independent of it. Hence, yes we should check wether dbus is available on the system.

You are right. DBus is independent of a graphical user interface. I wasn't aware of that. Nevertheless, the change mentioned above checks exactly what is needed here. I'll commit it.