Story for milestone M5. Read docs/STORY-WORKFLOW.md first — it defines the process, gates, and patterns; this issue defines only the specific work.
The nuncio.v1.Accounts service (crates/nuncio-proto/proto/nuncio/v1/nuncio.proto:139-147) currently exposes only AddAccount and ListAccounts. There is no way to edit, remove, or test a configured account over gRPC, and AccountConfig.imap_tls_mode/smtp_tls_mode (added to the wire contract and to nuncio_core::AccountConfig) are validated on AddAccount but silently discarded: DatabaseEngine::save_account (crates/nuncio-store/src/db.rs:615-643) writes to an accounts table that has no imap_tls_mode/smtp_tls_mode columns (crates/nuncio-store/src/db.rs:374-386), and DatabaseEngine::list_accounts (crates/nuncio-store/src/db.rs:645-717) hardcodes both back to TlsMode::ImplicitTls regardless of what was configured (crates/nuncio-store/src/db.rs:709-710). Neither nuncio-mail's ImapEngine nor its SmtpTransportEngine ever consult TlsMode at all — ImapEngine::new/with_credentials (crates/nuncio-mail/src/imap.rs:173,183-196) and SmtpTransportEngine::new (crates/nuncio-mail/src/smtp.rs:46) take no TLS-mode parameter, and IMAP's connect_tls_stream (crates/nuncio-mail/src/imap.rs:69-80) always dials implicit TLS immediately — there is no STARTTLS or plaintext code path in this crate at all (grep -r "TlsMode" crates/nuncio-mail/src returns nothing outside test literals). On the CLI side, nuncio-cli already has AccountSubcommand::Show/Edit/Delete/Test defined (crates/nuncio-cli/src/args.rs:209-247), but their handlers in runner.rs (crates/nuncio-cli/src/runner.rs:215-253) are fabricated theater: Show reads self.db — the CLI's own ephemeral, per-process, throwawayDatabaseEngine (crates/nuncio-cli/src/runner.rs:132-172, doc comment explicitly calls it "thrown away when the process exits") rather than the daemon's real store, and Edit/Delete/Test never call the daemon at all — they just print "Account '{id}' updated successfully." / "removed." / "✓ ... connection test OK (24ms latency)." unconditionally, which is exactly the anti-pattern docs/STORY-WORKFLOW.md §2.3 forbids. Fixes#182.
Goal
Add UpdateAccount, RemoveAccount, and TestAccountConnection to the nuncio.v1.Accounts service; make imap_tls_mode/smtp_tls_mode actually persist through a schema migration and actually influence the real IMAP/SMTP connection (implicit TLS / STARTTLS / plaintext, chosen per mode rather than always implicit); and replace the CLI's fabricated account show/edit/delete/test with real thin gRPC clients of the daemon. This must NOT fake anything: TestAccountConnection must report a genuine dial/handshake/auth attempt result (success or the real error), never a canned "OK (24ms latency)"; UpdateAccount/RemoveAccount must genuinely mutate the daemon's persisted store, never just echo the request back.
Files to touch / patterns to mirror
crates/nuncio-proto/proto/nuncio/v1/nuncio.proto — add to service Accounts (~line 139-147): rpc UpdateAccount(UpdateAccountRequest) returns (UpdateAccountResponse), rpc RemoveAccount(RemoveAccountRequest) returns (RemoveAccountResponse), rpc TestAccountConnection(TestAccountConnectionRequest) returns (TestAccountConnectionResponse). UpdateAccountRequest{AccountConfig config} (password optional — omit to keep the existing keyring credential; optional string password = 2), UpdateAccountResponse{} (or the updated AccountConfig, minus password, mirroring AddAccountResponse's discipline); RemoveAccountRequest{string id}, RemoveAccountResponse{}; TestAccountConnectionRequest{string id}, TestAccountConnectionResponse{bool imap_ok, bool smtp_ok, optional string imap_error, optional string smtp_error} — never a boolean-only "ok" with no detail on failure. Regen the descriptor golden per the recipe in crates/nuncio-proto/src/lib.rs.
crates/nuncio-store/src/db.rs — extend the accounts table (migrate, ~line 374-386) with imap_tls_mode TEXT NOT NULL DEFAULT 'implicit_tls' and smtp_tls_mode TEXT NOT NULL DEFAULT 'implicit_tls' columns, using the same backfill-safe ALTER TABLE ... ADD COLUMN migration pattern already used for smtp_host/smtp_port (see Self::ensure_accounts_smtp_columns, referenced in the comment at line 688-695 — find and mirror that function). Update save_account (line 615-643) to bind and persist both modes; update list_accounts (line 645-717) to read them back from the row instead of hardcoding TlsMode::ImplicitTls at line 709-710. Add get_account(&self, id: &str) -> Result<Option<AccountConfig>, DatabaseError> and delete_account(&self, id: &str) -> Result<(), DatabaseError> (mirror the shape of save_email/existing single-row helpers) — today neither exists in this crate at all; do not assume otherwise.
crates/nuncio-mail/src/imap.rs — thread a TlsMode through ImapDualSocketManager/ImapEngine::with_credentials (currently crates/nuncio-mail/src/imap.rs:173-196 has no such parameter) so connect_tls_stream (line 69-80) is only used for TlsMode::ImplicitTls; add a STARTTLS path (connect plain TCP, issue IMAP STARTTLS, then upgrade via tokio_rustls) for TlsMode::StartTls, and a genuinely plaintext path for TlsMode::Plain. Mirror the existing build_tls_connector/build_tls_config helpers for the TLS-upgrade step.
crates/nuncio-mail/src/smtp.rs — thread TlsMode into SmtpTransportEngine::new (line 46) so it selects lettre's implicit-TLS relay vs. STARTTLS vs. plaintext builder accordingly (check AsyncSmtpTransport's builder API for the STARTTLS/plaintext variants already available in the lettre version this crate depends on).
crates/nunciod/src/grpc.rs — add update_account/remove_account/test_account_connection to impl Accounts for AccountsGrpcService (~line 306-357), mirroring add_account's (line 308-340) shape: validate, then call the new store methods. test_account_connection resolves the account, reads its keyring password via self.secrets, and attempts a real (but bounded — do not hang the RPC) connect/auth against the configured IMAP and SMTP endpoints; report genuine per-protocol success/failure, never a fabricated result. Do not persist the orphaned-secret bug pattern from add_account (line 325-337) into update_account if it also needs to touch the keyring — see M5-S4, which fixes that exact bug in add_account; keep update_account's keyring-then-store ordering consistent with whatever M5-S4 lands (check if M5-S4 is merged first; if not, apply the same rollback-on-failure discipline here too).
crates/nuncio-cli/src/args.rs — AccountSubcommand::Show/Edit/Delete/Test already exist (line 209-247); no proto-shape changes needed there, but Edit's fields (imap_host/imap_port/smtp_host/smtp_port, no TLS-mode flags today) should gain imap_mode/smtp_modeOption<String> flags mirroring Add's imap_mode/smtp_mode (line 196-201), reusing the existing parse_tls_mode helper (referenced at crates/nuncio-cli/src/runner.rs:1413-1420).
crates/nuncio-cli/src/runner.rs — replace the fabricated Show/Edit/Delete/Test handlers (line 215-253) with real calls through connect_accounts_client() (line 1552-1564, already used by handle_add_account/handle_accounts_list) to the new RPCs. Show must call ListAccounts (there is no single-account GetAccount RPC in scope here; filter client-side, or — if simpler — skip adding one and just filter the ListAccounts result) against the daemon, not self.db.
Implementation steps
Add the three RPCs + messages to nuncio.proto; rebuild -p nuncio-proto; copy the new nuncio_v1_descriptor.bin over the committed golden; confirm contract_stability::descriptor_matches_committed_golden passes.
Add imap_tls_mode/smtp_tls_mode columns via a backfill-safe migration in nuncio-store; add get_account/delete_account; fix save_account/list_accounts to round-trip the real TLS modes (write a store-level test that saves an account with TlsMode::StartTls, re-reads it, and asserts the mode survived — this is the regression test for the current silent-discard bug).
Thread TlsMode into ImapEngine/SmtpTransportEngine construction sites in nuncio-mail; add the STARTTLS and plaintext code paths; unit-test each mode's connection-setup logic against a local mock/loopback listener (no real server) — mirror the existing tls_config_building/imap_dual_socket_manager_lifecycle tests in crates/nuncio-mail/src/imap.rs.
Implement update_account/remove_account/test_account_connection in AccountsGrpcService; wire the daemon to actually apply the configured imap_tls_mode/smtp_tls_mode when building backends in crates/nunciod/src/sync.rs's build_mail_backend (line 126-137, currently ignores TLS mode entirely) and crates/nunciod/src/send.rs's build_smtp_sender (line 109-120, same gap).
Update nuncio-cli's Show/Edit/Delete/Test handlers to be real gRPC clients; add/adjust CLI-level tests mirroring the existing handle_add_account tests (crates/nuncio-cli/src/runner.rs test module, e.g. around line 1700-1760, 2030-2090).
Add a full-daemon E2E (mirror crates/nunciod/tests/spine_e2e_test.rs) that adds an account with TlsMode::StartTls, updates it, tests the connection against an injected/mock backend (not a live server), lists it back and asserts the mode round-tripped, then removes it and asserts it is gone. Include an unauthenticated-call-rejected assertion for each new RPC (mirror crates/nunciod/tests/grpc_auth_test.rs).
Acceptance criteria
UpdateAccount, RemoveAccount, TestAccountConnection RPCs added to nuncio.v1.Accounts; contract golden regenerated; nuncio-proto's contract_stability test passes.
AccountsGrpcService continues to be mounted behind its own BearerAuthInterceptor; an offline E2E asserts an unauthenticated call to each of the three new RPCs is rejected.
imap_tls_mode/smtp_tls_mode persist across a save/list round-trip in nuncio-store (new regression test proving today's hardcoded-ImplicitTls bug is fixed).
ImapEngine and SmtpTransportEngine genuinely branch their connection setup on TlsMode::{ImplicitTls,StartTls,Plain} — not just validate-and-discard.
nunciod::sync::build_mail_backend and nunciod::send::build_smtp_sender pass the account's real configured TLS mode into the engine constructors.
CLI account show/edit/delete/test are real thin gRPC clients of the running daemon (no reads from the CLI's own ephemeral self.db, no hardcoded success strings).
TestAccountConnection reports a genuine per-protocol success/failure (via a mock backend in tests — never a live server), never a fabricated "OK" with fake latency.
Offline E2E (wiremock / Mock*Backend) proves the capability end to end; no live network.
Follow docs/STORY-WORKFLOW.md: no fabricated data; comment convention (intent-only, no story/GH/phase refs in .rs/.proto); mock-only tests. Branch story/m5-s1-account-lifecycle off dev; PR to dev with the gate output pasted as evidence; the PO reviews before the issue is closed.
Dependencies / notes
[PROTO] — touches nuncio.proto. Confirm with the PO that no other proto-touching story (e.g. M5-S3) is in flight before starting; coordinate ordering if both are queued.
Prior art (reference only, DO NOT trust): branch wip/unreviewed-account-tls-sync, commit 73518f8 ("feat(accounts): real edit/delete/test-connection, fix TLS-mode gaps"), contains an unreviewed implementation of much of this story produced by a runaway agent that connected to a live server during development. You MAY read it for ideas (git show 73518f8 or git diff 703b700..origin/wip/unreviewed-account-tls-sync), but you MUST NOT cherry-pick it wholesale, MUST NOT connect to any live server at any point, and MUST re-implement with mock-only tests reviewed against this issue's acceptance criteria from scratch. Its existence does not reduce this story's scope or review bar.
## Context
Story for milestone M5. **Read [`docs/STORY-WORKFLOW.md`](https://github.com/KofTwentyTwo/nuncio/blob/dev/docs/STORY-WORKFLOW.md) first** — it defines the process, gates, and patterns; this issue defines only the specific work.
The `nuncio.v1.Accounts` service (`crates/nuncio-proto/proto/nuncio/v1/nuncio.proto:139-147`) currently exposes only `AddAccount` and `ListAccounts`. There is no way to edit, remove, or test a configured account over gRPC, and `AccountConfig.imap_tls_mode`/`smtp_tls_mode` (added to the wire contract and to `nuncio_core::AccountConfig`) are validated on `AddAccount` but **silently discarded**: `DatabaseEngine::save_account` (`crates/nuncio-store/src/db.rs:615-643`) writes to an `accounts` table that has no `imap_tls_mode`/`smtp_tls_mode` columns (`crates/nuncio-store/src/db.rs:374-386`), and `DatabaseEngine::list_accounts` (`crates/nuncio-store/src/db.rs:645-717`) **hardcodes both back to `TlsMode::ImplicitTls`** regardless of what was configured (`crates/nuncio-store/src/db.rs:709-710`). Neither `nuncio-mail`'s `ImapEngine` nor its `SmtpTransportEngine` ever consult `TlsMode` at all — `ImapEngine::new`/`with_credentials` (`crates/nuncio-mail/src/imap.rs:173,183-196`) and `SmtpTransportEngine::new` (`crates/nuncio-mail/src/smtp.rs:46`) take no TLS-mode parameter, and IMAP's `connect_tls_stream` (`crates/nuncio-mail/src/imap.rs:69-80`) always dials implicit TLS immediately — there is no STARTTLS or plaintext code path in this crate at all (`grep -r "TlsMode" crates/nuncio-mail/src` returns nothing outside test literals). On the CLI side, `nuncio-cli` already has `AccountSubcommand::Show`/`Edit`/`Delete`/`Test` defined (`crates/nuncio-cli/src/args.rs:209-247`), but their handlers in `runner.rs` (`crates/nuncio-cli/src/runner.rs:215-253`) are fabricated theater: `Show` reads `self.db` — the CLI's own **ephemeral, per-process, throwaway** `DatabaseEngine` (`crates/nuncio-cli/src/runner.rs:132-172`, doc comment explicitly calls it "thrown away when the process exits") rather than the daemon's real store, and `Edit`/`Delete`/`Test` never call the daemon at all — they just print `"Account '{id}' updated successfully."` / `"removed."` / `"✓ ... connection test OK (24ms latency)."` unconditionally, which is exactly the anti-pattern `docs/STORY-WORKFLOW.md` §2.3 forbids. Fixes #182.
## Goal
Add `UpdateAccount`, `RemoveAccount`, and `TestAccountConnection` to the `nuncio.v1.Accounts` service; make `imap_tls_mode`/`smtp_tls_mode` actually persist through a schema migration and actually influence the real IMAP/SMTP connection (implicit TLS / STARTTLS / plaintext, chosen per mode rather than always implicit); and replace the CLI's fabricated `account show`/`edit`/`delete`/`test` with real thin gRPC clients of the daemon. This must NOT fake anything: `TestAccountConnection` must report a genuine dial/handshake/auth attempt result (success or the real error), never a canned `"OK (24ms latency)"`; `UpdateAccount`/`RemoveAccount` must genuinely mutate the daemon's persisted store, never just echo the request back.
## Files to touch / patterns to mirror
- `crates/nuncio-proto/proto/nuncio/v1/nuncio.proto` — add to `service Accounts` (~line 139-147): `rpc UpdateAccount(UpdateAccountRequest) returns (UpdateAccountResponse)`, `rpc RemoveAccount(RemoveAccountRequest) returns (RemoveAccountResponse)`, `rpc TestAccountConnection(TestAccountConnectionRequest) returns (TestAccountConnectionResponse)`. `UpdateAccountRequest{AccountConfig config}` (password optional — omit to keep the existing keyring credential; `optional string password = 2`), `UpdateAccountResponse{}` (or the updated `AccountConfig`, minus password, mirroring `AddAccountResponse`'s discipline); `RemoveAccountRequest{string id}`, `RemoveAccountResponse{}`; `TestAccountConnectionRequest{string id}`, `TestAccountConnectionResponse{bool imap_ok, bool smtp_ok, optional string imap_error, optional string smtp_error}` — never a boolean-only "ok" with no detail on failure. Regen the descriptor golden per the recipe in `crates/nuncio-proto/src/lib.rs`.
- `crates/nuncio-store/src/db.rs` — extend the `accounts` table (`migrate`, ~line 374-386) with `imap_tls_mode TEXT NOT NULL DEFAULT 'implicit_tls'` and `smtp_tls_mode TEXT NOT NULL DEFAULT 'implicit_tls'` columns, using the same backfill-safe `ALTER TABLE ... ADD COLUMN` migration pattern already used for `smtp_host`/`smtp_port` (see `Self::ensure_accounts_smtp_columns`, referenced in the comment at line 688-695 — find and mirror that function). Update `save_account` (line 615-643) to bind and persist both modes; update `list_accounts` (line 645-717) to read them back from the row instead of hardcoding `TlsMode::ImplicitTls` at line 709-710. Add `get_account(&self, id: &str) -> Result<Option<AccountConfig>, DatabaseError>` and `delete_account(&self, id: &str) -> Result<(), DatabaseError>` (mirror the shape of `save_email`/existing single-row helpers) — **today neither exists in this crate at all**; do not assume otherwise.
- `crates/nuncio-mail/src/imap.rs` — thread a `TlsMode` through `ImapDualSocketManager`/`ImapEngine::with_credentials` (currently `crates/nuncio-mail/src/imap.rs:173-196` has no such parameter) so `connect_tls_stream` (line 69-80) is only used for `TlsMode::ImplicitTls`; add a STARTTLS path (connect plain TCP, issue IMAP `STARTTLS`, then upgrade via `tokio_rustls`) for `TlsMode::StartTls`, and a genuinely plaintext path for `TlsMode::Plain`. Mirror the existing `build_tls_connector`/`build_tls_config` helpers for the TLS-upgrade step.
- `crates/nuncio-mail/src/smtp.rs` — thread `TlsMode` into `SmtpTransportEngine::new` (line 46) so it selects `lettre`'s implicit-TLS relay vs. STARTTLS vs. plaintext builder accordingly (check `AsyncSmtpTransport`'s builder API for the STARTTLS/plaintext variants already available in the `lettre` version this crate depends on).
- `crates/nunciod/src/grpc.rs` — add `update_account`/`remove_account`/`test_account_connection` to `impl Accounts for AccountsGrpcService` (~line 306-357), mirroring `add_account`'s (line 308-340) shape: validate, then call the new store methods. `test_account_connection` resolves the account, reads its keyring password via `self.secrets`, and attempts a real (but bounded — do not hang the RPC) connect/auth against the configured IMAP and SMTP endpoints; report genuine per-protocol success/failure, never a fabricated result. **Do not persist the orphaned-secret bug pattern from `add_account`** (line 325-337) into `update_account` if it also needs to touch the keyring — see M5-S4, which fixes that exact bug in `add_account`; keep `update_account`'s keyring-then-store ordering consistent with whatever M5-S4 lands (check if M5-S4 is merged first; if not, apply the same rollback-on-failure discipline here too).
- `crates/nuncio-cli/src/args.rs` — `AccountSubcommand::Show`/`Edit`/`Delete`/`Test` already exist (line 209-247); no proto-shape changes needed there, but `Edit`'s fields (`imap_host`/`imap_port`/`smtp_host`/`smtp_port`, no TLS-mode flags today) should gain `imap_mode`/`smtp_mode` `Option<String>` flags mirroring `Add`'s `imap_mode`/`smtp_mode` (line 196-201), reusing the existing `parse_tls_mode` helper (referenced at `crates/nuncio-cli/src/runner.rs:1413-1420`).
- `crates/nuncio-cli/src/runner.rs` — replace the fabricated `Show`/`Edit`/`Delete`/`Test` handlers (line 215-253) with real calls through `connect_accounts_client()` (line 1552-1564, already used by `handle_add_account`/`handle_accounts_list`) to the new RPCs. `Show` must call `ListAccounts` (there is no single-account `GetAccount` RPC in scope here; filter client-side, or — if simpler — skip adding one and just filter the `ListAccounts` result) against the **daemon**, not `self.db`.
## Implementation steps
1. Add the three RPCs + messages to `nuncio.proto`; rebuild `-p nuncio-proto`; copy the new `nuncio_v1_descriptor.bin` over the committed golden; confirm `contract_stability::descriptor_matches_committed_golden` passes.
2. Add `imap_tls_mode`/`smtp_tls_mode` columns via a backfill-safe migration in `nuncio-store`; add `get_account`/`delete_account`; fix `save_account`/`list_accounts` to round-trip the real TLS modes (write a store-level test that saves an account with `TlsMode::StartTls`, re-reads it, and asserts the mode survived — this is the regression test for the current silent-discard bug).
3. Thread `TlsMode` into `ImapEngine`/`SmtpTransportEngine` construction sites in `nuncio-mail`; add the STARTTLS and plaintext code paths; unit-test each mode's connection-setup logic against a local mock/loopback listener (no real server) — mirror the existing `tls_config_building`/`imap_dual_socket_manager_lifecycle` tests in `crates/nuncio-mail/src/imap.rs`.
4. Implement `update_account`/`remove_account`/`test_account_connection` in `AccountsGrpcService`; wire the daemon to actually apply the configured `imap_tls_mode`/`smtp_tls_mode` when building backends in `crates/nunciod/src/sync.rs`'s `build_mail_backend` (line 126-137, currently ignores TLS mode entirely) and `crates/nunciod/src/send.rs`'s `build_smtp_sender` (line 109-120, same gap).
5. Update `nuncio-cli`'s `Show`/`Edit`/`Delete`/`Test` handlers to be real gRPC clients; add/adjust CLI-level tests mirroring the existing `handle_add_account` tests (`crates/nuncio-cli/src/runner.rs` test module, e.g. around line 1700-1760, 2030-2090).
6. Add a full-daemon E2E (mirror `crates/nunciod/tests/spine_e2e_test.rs`) that adds an account with `TlsMode::StartTls`, updates it, tests the connection against an injected/mock backend (not a live server), lists it back and asserts the mode round-tripped, then removes it and asserts it is gone. Include an unauthenticated-call-rejected assertion for each new RPC (mirror `crates/nunciod/tests/grpc_auth_test.rs`).
## Acceptance criteria
- [ ] `UpdateAccount`, `RemoveAccount`, `TestAccountConnection` RPCs added to `nuncio.v1.Accounts`; contract golden regenerated; `nuncio-proto`'s `contract_stability` test passes.
- [ ] `AccountsGrpcService` continues to be mounted behind its own `BearerAuthInterceptor`; an offline E2E asserts an unauthenticated call to each of the three new RPCs is rejected.
- [ ] `imap_tls_mode`/`smtp_tls_mode` persist across a save/list round-trip in `nuncio-store` (new regression test proving today's hardcoded-`ImplicitTls` bug is fixed).
- [ ] `ImapEngine` and `SmtpTransportEngine` genuinely branch their connection setup on `TlsMode::{ImplicitTls,StartTls,Plain}` — not just validate-and-discard.
- [ ] `nunciod::sync::build_mail_backend` and `nunciod::send::build_smtp_sender` pass the account's real configured TLS mode into the engine constructors.
- [ ] CLI `account show`/`edit`/`delete`/`test` are real thin gRPC clients of the running daemon (no reads from the CLI's own ephemeral `self.db`, no hardcoded success strings).
- [ ] `TestAccountConnection` reports a genuine per-protocol success/failure (via a mock backend in tests — never a live server), never a fabricated "OK" with fake latency.
- [ ] Offline E2E (wiremock / Mock*Backend) proves the capability end to end; no live network.
- [ ] `cargo verify` green (fmt + clippy `-D warnings` all-targets + test-all), 0 ignored.
## Definition of done & process
Follow [`docs/STORY-WORKFLOW.md`](https://github.com/KofTwentyTwo/nuncio/blob/dev/docs/STORY-WORKFLOW.md): no fabricated data; comment convention (intent-only, no story/GH/phase refs in `.rs`/`.proto`); mock-only tests. Branch `story/m5-s1-account-lifecycle` off `dev`; PR to `dev` with the gate output pasted as evidence; the PO reviews before the issue is closed.
## Dependencies / notes
**[PROTO]** — touches `nuncio.proto`. Confirm with the PO that no other proto-touching story (e.g. M5-S3) is in flight before starting; coordinate ordering if both are queued.
**Prior art (reference only, DO NOT trust):** branch `wip/unreviewed-account-tls-sync`, commit `73518f8` ("feat(accounts): real edit/delete/test-connection, fix TLS-mode gaps"), contains an **unreviewed** implementation of much of this story produced by a runaway agent that connected to a live server during development. You MAY read it for ideas (`git show 73518f8` or `git diff 703b700..origin/wip/unreviewed-account-tls-sync`), but you MUST NOT cherry-pick it wholesale, MUST NOT connect to any live server at any point, and MUST re-implement with mock-only tests reviewed against this issue's acceptance criteria from scratch. Its existence does not reduce this story's scope or review bar.
Fixes #182. Rough size: L.
Delivered via PR #230 (squash-merged to dev). Adds UpdateAccount/RemoveAccount/TestAccountConnection (additive); persists imap/smtp TLS modes via a backfill-safe migration and actually branches the IMAP (implicit/STARTTLS/plain) and SMTP connection on them; STARTTLS is a real verified upgrade (checks tagged OK before wrapping, genuine cert validation, no plaintext fallback); replaces the fabricated CLI show/edit/delete/test with real gRPC clients. Opus adversarial review verified the whole dangerous surface + NO live network in tests. Two Important findings fixed before merge: update_account rollback now restores the prior credential (no data loss), and TestAccountConnection's SMTP leg is honestly scoped to transport reachability (not auth) since lettre 0.11 has no clean auth-only probe. Fixes#182.
Delivered via PR #230 (squash-merged to dev). Adds UpdateAccount/RemoveAccount/TestAccountConnection (additive); persists imap/smtp TLS modes via a backfill-safe migration and actually branches the IMAP (implicit/STARTTLS/plain) and SMTP connection on them; STARTTLS is a real verified upgrade (checks tagged OK before wrapping, genuine cert validation, no plaintext fallback); replaces the fabricated CLI show/edit/delete/test with real gRPC clients. Opus adversarial review verified the whole dangerous surface + NO live network in tests. Two Important findings fixed before merge: update_account rollback now restores the prior credential (no data loss), and TestAccountConnection's SMTP leg is honestly scoped to transport reachability (not auth) since lettre 0.11 has no clean auth-only probe. Fixes #182.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Context
Story for milestone M5. Read
docs/STORY-WORKFLOW.mdfirst — it defines the process, gates, and patterns; this issue defines only the specific work.The
nuncio.v1.Accountsservice (crates/nuncio-proto/proto/nuncio/v1/nuncio.proto:139-147) currently exposes onlyAddAccountandListAccounts. There is no way to edit, remove, or test a configured account over gRPC, andAccountConfig.imap_tls_mode/smtp_tls_mode(added to the wire contract and tonuncio_core::AccountConfig) are validated onAddAccountbut silently discarded:DatabaseEngine::save_account(crates/nuncio-store/src/db.rs:615-643) writes to anaccountstable that has noimap_tls_mode/smtp_tls_modecolumns (crates/nuncio-store/src/db.rs:374-386), andDatabaseEngine::list_accounts(crates/nuncio-store/src/db.rs:645-717) hardcodes both back toTlsMode::ImplicitTlsregardless of what was configured (crates/nuncio-store/src/db.rs:709-710). Neithernuncio-mail'sImapEnginenor itsSmtpTransportEngineever consultTlsModeat all —ImapEngine::new/with_credentials(crates/nuncio-mail/src/imap.rs:173,183-196) andSmtpTransportEngine::new(crates/nuncio-mail/src/smtp.rs:46) take no TLS-mode parameter, and IMAP'sconnect_tls_stream(crates/nuncio-mail/src/imap.rs:69-80) always dials implicit TLS immediately — there is no STARTTLS or plaintext code path in this crate at all (grep -r "TlsMode" crates/nuncio-mail/srcreturns nothing outside test literals). On the CLI side,nuncio-clialready hasAccountSubcommand::Show/Edit/Delete/Testdefined (crates/nuncio-cli/src/args.rs:209-247), but their handlers inrunner.rs(crates/nuncio-cli/src/runner.rs:215-253) are fabricated theater:Showreadsself.db— the CLI's own ephemeral, per-process, throwawayDatabaseEngine(crates/nuncio-cli/src/runner.rs:132-172, doc comment explicitly calls it "thrown away when the process exits") rather than the daemon's real store, andEdit/Delete/Testnever call the daemon at all — they just print"Account '{id}' updated successfully."/"removed."/"✓ ... connection test OK (24ms latency)."unconditionally, which is exactly the anti-patterndocs/STORY-WORKFLOW.md§2.3 forbids. Fixes #182.Goal
Add
UpdateAccount,RemoveAccount, andTestAccountConnectionto thenuncio.v1.Accountsservice; makeimap_tls_mode/smtp_tls_modeactually persist through a schema migration and actually influence the real IMAP/SMTP connection (implicit TLS / STARTTLS / plaintext, chosen per mode rather than always implicit); and replace the CLI's fabricatedaccount show/edit/delete/testwith real thin gRPC clients of the daemon. This must NOT fake anything:TestAccountConnectionmust report a genuine dial/handshake/auth attempt result (success or the real error), never a canned"OK (24ms latency)";UpdateAccount/RemoveAccountmust genuinely mutate the daemon's persisted store, never just echo the request back.Files to touch / patterns to mirror
crates/nuncio-proto/proto/nuncio/v1/nuncio.proto— add toservice Accounts(~line 139-147):rpc UpdateAccount(UpdateAccountRequest) returns (UpdateAccountResponse),rpc RemoveAccount(RemoveAccountRequest) returns (RemoveAccountResponse),rpc TestAccountConnection(TestAccountConnectionRequest) returns (TestAccountConnectionResponse).UpdateAccountRequest{AccountConfig config}(password optional — omit to keep the existing keyring credential;optional string password = 2),UpdateAccountResponse{}(or the updatedAccountConfig, minus password, mirroringAddAccountResponse's discipline);RemoveAccountRequest{string id},RemoveAccountResponse{};TestAccountConnectionRequest{string id},TestAccountConnectionResponse{bool imap_ok, bool smtp_ok, optional string imap_error, optional string smtp_error}— never a boolean-only "ok" with no detail on failure. Regen the descriptor golden per the recipe incrates/nuncio-proto/src/lib.rs.crates/nuncio-store/src/db.rs— extend theaccountstable (migrate, ~line 374-386) withimap_tls_mode TEXT NOT NULL DEFAULT 'implicit_tls'andsmtp_tls_mode TEXT NOT NULL DEFAULT 'implicit_tls'columns, using the same backfill-safeALTER TABLE ... ADD COLUMNmigration pattern already used forsmtp_host/smtp_port(seeSelf::ensure_accounts_smtp_columns, referenced in the comment at line 688-695 — find and mirror that function). Updatesave_account(line 615-643) to bind and persist both modes; updatelist_accounts(line 645-717) to read them back from the row instead of hardcodingTlsMode::ImplicitTlsat line 709-710. Addget_account(&self, id: &str) -> Result<Option<AccountConfig>, DatabaseError>anddelete_account(&self, id: &str) -> Result<(), DatabaseError>(mirror the shape ofsave_email/existing single-row helpers) — today neither exists in this crate at all; do not assume otherwise.crates/nuncio-mail/src/imap.rs— thread aTlsModethroughImapDualSocketManager/ImapEngine::with_credentials(currentlycrates/nuncio-mail/src/imap.rs:173-196has no such parameter) soconnect_tls_stream(line 69-80) is only used forTlsMode::ImplicitTls; add a STARTTLS path (connect plain TCP, issue IMAPSTARTTLS, then upgrade viatokio_rustls) forTlsMode::StartTls, and a genuinely plaintext path forTlsMode::Plain. Mirror the existingbuild_tls_connector/build_tls_confighelpers for the TLS-upgrade step.crates/nuncio-mail/src/smtp.rs— threadTlsModeintoSmtpTransportEngine::new(line 46) so it selectslettre's implicit-TLS relay vs. STARTTLS vs. plaintext builder accordingly (checkAsyncSmtpTransport's builder API for the STARTTLS/plaintext variants already available in thelettreversion this crate depends on).crates/nunciod/src/grpc.rs— addupdate_account/remove_account/test_account_connectiontoimpl Accounts for AccountsGrpcService(~line 306-357), mirroringadd_account's (line 308-340) shape: validate, then call the new store methods.test_account_connectionresolves the account, reads its keyring password viaself.secrets, and attempts a real (but bounded — do not hang the RPC) connect/auth against the configured IMAP and SMTP endpoints; report genuine per-protocol success/failure, never a fabricated result. Do not persist the orphaned-secret bug pattern fromadd_account(line 325-337) intoupdate_accountif it also needs to touch the keyring — see M5-S4, which fixes that exact bug inadd_account; keepupdate_account's keyring-then-store ordering consistent with whatever M5-S4 lands (check if M5-S4 is merged first; if not, apply the same rollback-on-failure discipline here too).crates/nuncio-cli/src/args.rs—AccountSubcommand::Show/Edit/Delete/Testalready exist (line 209-247); no proto-shape changes needed there, butEdit's fields (imap_host/imap_port/smtp_host/smtp_port, no TLS-mode flags today) should gainimap_mode/smtp_modeOption<String>flags mirroringAdd'simap_mode/smtp_mode(line 196-201), reusing the existingparse_tls_modehelper (referenced atcrates/nuncio-cli/src/runner.rs:1413-1420).crates/nuncio-cli/src/runner.rs— replace the fabricatedShow/Edit/Delete/Testhandlers (line 215-253) with real calls throughconnect_accounts_client()(line 1552-1564, already used byhandle_add_account/handle_accounts_list) to the new RPCs.Showmust callListAccounts(there is no single-accountGetAccountRPC in scope here; filter client-side, or — if simpler — skip adding one and just filter theListAccountsresult) against the daemon, notself.db.Implementation steps
nuncio.proto; rebuild-p nuncio-proto; copy the newnuncio_v1_descriptor.binover the committed golden; confirmcontract_stability::descriptor_matches_committed_goldenpasses.imap_tls_mode/smtp_tls_modecolumns via a backfill-safe migration innuncio-store; addget_account/delete_account; fixsave_account/list_accountsto round-trip the real TLS modes (write a store-level test that saves an account withTlsMode::StartTls, re-reads it, and asserts the mode survived — this is the regression test for the current silent-discard bug).TlsModeintoImapEngine/SmtpTransportEngineconstruction sites innuncio-mail; add the STARTTLS and plaintext code paths; unit-test each mode's connection-setup logic against a local mock/loopback listener (no real server) — mirror the existingtls_config_building/imap_dual_socket_manager_lifecycletests incrates/nuncio-mail/src/imap.rs.update_account/remove_account/test_account_connectioninAccountsGrpcService; wire the daemon to actually apply the configuredimap_tls_mode/smtp_tls_modewhen building backends incrates/nunciod/src/sync.rs'sbuild_mail_backend(line 126-137, currently ignores TLS mode entirely) andcrates/nunciod/src/send.rs'sbuild_smtp_sender(line 109-120, same gap).nuncio-cli'sShow/Edit/Delete/Testhandlers to be real gRPC clients; add/adjust CLI-level tests mirroring the existinghandle_add_accounttests (crates/nuncio-cli/src/runner.rstest module, e.g. around line 1700-1760, 2030-2090).crates/nunciod/tests/spine_e2e_test.rs) that adds an account withTlsMode::StartTls, updates it, tests the connection against an injected/mock backend (not a live server), lists it back and asserts the mode round-tripped, then removes it and asserts it is gone. Include an unauthenticated-call-rejected assertion for each new RPC (mirrorcrates/nunciod/tests/grpc_auth_test.rs).Acceptance criteria
UpdateAccount,RemoveAccount,TestAccountConnectionRPCs added tonuncio.v1.Accounts; contract golden regenerated;nuncio-proto'scontract_stabilitytest passes.AccountsGrpcServicecontinues to be mounted behind its ownBearerAuthInterceptor; an offline E2E asserts an unauthenticated call to each of the three new RPCs is rejected.imap_tls_mode/smtp_tls_modepersist across a save/list round-trip innuncio-store(new regression test proving today's hardcoded-ImplicitTlsbug is fixed).ImapEngineandSmtpTransportEnginegenuinely branch their connection setup onTlsMode::{ImplicitTls,StartTls,Plain}— not just validate-and-discard.nunciod::sync::build_mail_backendandnunciod::send::build_smtp_senderpass the account's real configured TLS mode into the engine constructors.account show/edit/delete/testare real thin gRPC clients of the running daemon (no reads from the CLI's own ephemeralself.db, no hardcoded success strings).TestAccountConnectionreports a genuine per-protocol success/failure (via a mock backend in tests — never a live server), never a fabricated "OK" with fake latency.cargo verifygreen (fmt + clippy-D warningsall-targets + test-all), 0 ignored.Definition of done & process
Follow
docs/STORY-WORKFLOW.md: no fabricated data; comment convention (intent-only, no story/GH/phase refs in.rs/.proto); mock-only tests. Branchstory/m5-s1-account-lifecycleoffdev; PR todevwith the gate output pasted as evidence; the PO reviews before the issue is closed.Dependencies / notes
[PROTO] — touches
nuncio.proto. Confirm with the PO that no other proto-touching story (e.g. M5-S3) is in flight before starting; coordinate ordering if both are queued.Prior art (reference only, DO NOT trust): branch
wip/unreviewed-account-tls-sync, commit73518f8("feat(accounts): real edit/delete/test-connection, fix TLS-mode gaps"), contains an unreviewed implementation of much of this story produced by a runaway agent that connected to a live server during development. You MAY read it for ideas (git show 73518f8orgit diff 703b700..origin/wip/unreviewed-account-tls-sync), but you MUST NOT cherry-pick it wholesale, MUST NOT connect to any live server at any point, and MUST re-implement with mock-only tests reviewed against this issue's acceptance criteria from scratch. Its existence does not reduce this story's scope or review bar.Fixes #182. Rough size: L.
Delivered via PR #230 (squash-merged to dev). Adds UpdateAccount/RemoveAccount/TestAccountConnection (additive); persists imap/smtp TLS modes via a backfill-safe migration and actually branches the IMAP (implicit/STARTTLS/plain) and SMTP connection on them; STARTTLS is a real verified upgrade (checks tagged OK before wrapping, genuine cert validation, no plaintext fallback); replaces the fabricated CLI show/edit/delete/test with real gRPC clients. Opus adversarial review verified the whole dangerous surface + NO live network in tests. Two Important findings fixed before merge: update_account rollback now restores the prior credential (no data loss), and TestAccountConnection's SMTP leg is honestly scoped to transport reachability (not auth) since lettre 0.11 has no clean auth-only probe. Fixes #182.