backport: typed install_app errors, payload signing, and lair error detail - #147
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
62c2fe5 to
945a9e8
Compare
945a9e8 to
c74fc9b
Compare
Route `Runtime::install_app` through the conductor handle directly (`install_app_bundle`) instead of `AdminInterfaceApi::handle_request`, which flattens a conductor error into a print-only `ExternalApiWireError::InternalError(String)` (holochain TODO B-01506). Going direct preserves the typed `ConductorError`, which the existing `From<ConductorError>` impl surfaces as `RuntimeError::Conductor(..)`, so callers can match on the actual failure, for instance the benign re-install case as `ConductorError::AppAlreadyInstalled`, instead of grepping a Debug string. Previously every install failure arrived as the opaque `RuntimeError::AdminApiBadResponse`. The success path mirrors the admin `InstallApp` handler (`get_dna_definitions` + `AppInfo::from_installed_app`), and `check_running()` is preserved for fast-fail parity with the sibling `req_admin_api` calls. `tauri-plugin-holochain` re-exports `RuntimeError` and `ConductorError` so plugin consumers can destructure the error without depending on the runtime crate directly. Add `test_install_app_already_installed`, covering that the typed conductor error survives the passthrough and that a failed re-install leaves the original app untouched.
The Display impl dropped the wrapped OneErr's message, so every lair failure (locked keystore, missing key, any other cause) rendered as the identical bare string "Lair Error" with no way to tell them apart from the caller side.
Runtime::sign_payload signs whatever bytes the caller supplies with a specific agent key held by the keystore, for protocols beyond zome calls that need proof of control over a Holochain identity (e.g. signing a timestamp for a re-authentication handshake). The signing key is always explicit: this keystore can hold more than one signable identity at once (the device-seed key and, in authenticated mode, a separate hc-auth key), and there is no default, since a signature from the wrong key is still valid, just for the wrong identity. Exposed through the in-process plugin as the sign_payload Tauri command: raw agent-key bytes and payload in, a base64-encoded signature out ready for a JSON body. Malformed key bytes from the webview are rejected via the fallible HoloHash parse rather than the panicking one used elsewhere at the FFI boundary, since this input comes straight off the wire. Invoking the command before the conductor boots returns Error::NotReady rather than panicking the command task. The command stays out of the plugin's `default` permission set. `sign_zome_call` signs the hash of a well-formed `ZomeCallParams`, so what it produces is only usable as the zome call it describes; this signs bytes the caller chose, which carry no such domain separation, so a capability must name `allow-sign-payload` itself.
c74fc9b to
69bcfb9
Compare
Review follow-up: a failed boot burned the full wait and hid the real start() error. The plugin held only `RwLock<Option<Runtime>>`, so a setup error existed solely as the transient `holochain://setup-failed` event. Anything not already listening when it fired lost the cause for good, which is why `try_runtime()` could only ever answer `NotReady`, indistinguishable from a boot still in flight. A host app whose conductor failed to come up had no way to learn why. The runtime slot becomes a `BootState` of `NotStarted`, `Ready(Runtime)` or `Failed(String)` behind the same single lock, so the outcome is one piece of state and cannot disagree with itself. `start_with_config` records the cause of a failed boot and `try_runtime()` reports it as the new `Error::SetupFailed`; `NotReady` now means only that no boot has finished. `holochain://setup-failed` is emitted exactly as before, so existing consumers are unaffected, and the cause is recorded before the emit, so a listener that reacts to the event and then calls `try_runtime()` sees the same string. `wait_for_ready` becomes a plain poll that panics on a reported setup failure, so a test fails on the actual error rather than sitting out `BOOT_TIMEOUT`. Against a conductor whose data root cannot be created, that is 0.2s reporting the lair error in place of 60.0s reporting only "conductor did not become ready". `failed_boot_reports_its_cause_instead_of_timing_out` covers it, and fails both if the plugin stops recording the cause and if the waiter goes back to polling blindly.
Two review follow-ups, neither touching behaviour.
`Error::Serialization` was documented as an App API (de)serialization
failure. That held until this branch added `sign_payload`, which returns it
for a malformed agent key, so the doc now covers a command argument as well
as an App API message.
The lair change earlier on this branch alters `RuntimeError::Lair`'s Display
from "Lair Error" to "Lair Error: {0}", which every FFI consumer sees. It
had no changelog entry while its two neighbours did.
…ome_call It panicked where its sibling sign_payload returns Error::NotReady, and a panic in a command handler does not reject the webview promise.
…red start A deferred start recorded the cause but emitted nothing, so a frontend listening for the event saw no failure. Emitting from start_with_config covers both entry points, and the lock is released first so a listener can call back in.
It reads as a cosmetic improvement to an error string, not a change a consumer needs to act on.
|
✔️ 90f5ad7...4acf0cd - Conventional commits check succeeded. |
Backport of #146 to the 0.6 line.
install_apppreserves the typedConductorErrorinstead of flattening every failure into a string, so a caller can tell an already-installed app from a real fault without matching on text.RuntimeError::Laircarries lair's own error detail, rather than reporting a category with no cause.One adaptation for 0.6.3:
Conductor::get_dna_definitionsis synchronous here andasyncon 0.7, so the call drops its.await. That matches this branch's own adminInstallApphandler, which the change mirrors. The test fixture also drops the 0.7-onlyrestore_from_dhtfield and unnests the assertion, sinceRuntimeError::ConductorholdsConductorErrordirectly on this line rather than boxed.Note that
make staticis already red onmain-0.6before this change: 13cargo fmtviolations and 16 clippy errors (uninlined_format_args,result_large_err), in files this backport does not touch. Verified identical on the base commit, and the fmt regions do not intersect any line this branch adds. Left alone, since the clippy fix means boxingRuntimeErrorvariants, a public API change that is yours to make rather than a backport's to smuggle in.