feat(tvc): operator key backup command and login onboarding nudge by richardpringle · Pull Request #236 · tkhq/rust-sdk

richardpringle · 2026-08-10T19:52:06Z

Extracted from #224 (first of the stack).

Typed operator public key

StoredQosOperatorKey.public_key becomes QosOperatorPublicKey, a 130-byte composite (encrypt_public ‖ sign_public) parsed once at the file boundary; hex stays the display and serialization form, so disk and JSON shapes are unchanged. The backup command and login nudge below carry the typed key instead of re-validating strings.

TVC-52 — `tvc keys backup-operator-key`

Copies the org's operator key file byte-for-byte after validating it parses (unknown fields survive — no re-serialize). Interactive runs prompt for the destination and confirm overwrites; non-interactive requires --output/--overwrite. Output includes the private-key warning, storage guidance, and the manual restore path (restore command deferred). Org resolution follows login's find_org (alias or org ID, else the active org). Adopts the Run trait from #223.

TVC-53 — onboarding backup nudge

When login generates a fresh operator key interactively, it explains the key exists only on this machine and offers a backup (default Yes) via the shared core; backup errors degrade to a warning so login never dies. Declining prints the pointer to the standalone command; re-logins with an existing key get a single tip line. A one-shot mock whoami server carries PTY login past credential verification, so the accept/decline/tip paths are all covered end-to-end.

Closes TVC-52. Closes TVC-53.

emostov

dep review only approval

daniilrrr

trying something a little different with this review

I started human-reviewing the PR with Claude Opus 5 locally, and it flagged several edge cases off the bat. I am not very familiar with this code, so I thought the most productive thing would be to demarcate the pure-AGENT comments, and add my own human review reply on each. Feel free to override any of the comments, but have a look.

I did not intend to add slop to your reading workload, but rather use the agent to review a variety of paths and edge cases

(AGENT) acceptance checklist — the five commands that misbehave today

Every one of these was reproduced against 2d1908d with the script at the bottom. Note the framing: two of them must keep exiting non-zero and change only their message, and one must start failing where it currently succeeds — "must not fail" isn't the criterion for all five.

1. No `--output`, stdin is a pipe, `TVC_NON_INTERACTIVE` unset

$ tvc keys backup-operator-key < /dev/null
error: The input device is not a TTY          exit=1

Required: still exit=1, but error: --output is required in non-interactive mode. The exit code is already correct; the message is the defect. Covers the blocking comment on backup_operator_key.rs:42.

2. `--output` already exists, stdin is a pipe, `TVC_NON_INTERACTIVE` unset

$ echo previous > bk.json
$ tvc keys backup-operator-key --output bk.json < /dev/null
error: The input device is not a TTY          exit=1

Required: still exit=1, with error: destination bk.json already exists; pass --overwrite to replace it, and bk.json still containing previous. Covers the blocking comment on backup_operator_key.rs:78.

3. Permissions of a written backup

$ tvc keys backup-operator-key --output bk2.json    # TVC_NON_INTERACTIVE=1
$ ls -l bk2.json
-rw-r--r--   bk2.json

Required: -rw-------, under the default umask 022 this repo's CI and a stock dev shell both use. Covers the blocking comment on backup_operator_key.rs:129. Worth asserting the mode in the test rather than eyeballing it — std::os::unix::fs::PermissionsExt::mode() & 0o777 == 0o600.

4. An operator key whose private key cannot derive its public key

$ cat operator.json    # public_key: 130 valid hex bytes, private_key: "not-even-hex"
$ tvc keys backup-operator-key --output bk3.json
Operator key backed up!                       exit=0

Required: non-zero exit naming the source path, and no bk3.json written. This is the inverse of the others — it currently succeeds and must start failing. Covers the medium comment on backup_operator_key.rs:112. If you decide to keep back_up I/O-only instead, this check comes off the list and the doc comment on line 95 changes instead — but then say so on that thread, because as written the command reports success for a backup that cannot be restored.

5. `login`: cancelling the backup prompt kills a login that already succeeded

$ printf '\x04' | script -q /dev/null tvc login --org alias-a
...
Operator Key Generated!
? Back up your operator key now? <canceled>
error: Operation was canceled by the user     exit=1

with operator.json already on disk and the config already saved. Re-running tvc login immediately afterwards prints Successfully logged in! and exit=0, which is what makes it clear the first run had genuinely succeeded before the prompt killed it.

Required: exit=0 and Successfully logged in! in the output, with a warning line about the skipped backup. Covers the blocking comment on login.rs:616. The probe uses Ctrl-D because it scripts cleanly; Ctrl-C and Esc reach the same InquireError and the same ?, so one PTY test covers all three. Both prompts need it — the destination prompt on line 617 has the identical problem, so please cover cancelling at either one.

Not on this list, deliberately

tvc deploy approve --dangerous-skip-interactive --skip-post --manifest ... against an operator.json with a short public_key currently fails with must be 130 bytes, got 3. That's the subject of the medium comment on qos_operator_key.rs:116, and it's a decision, not a defect — if you accept the compatibility break (which I'd lean towards) it should keep failing, so there's nothing to verify. It only becomes an acceptance criterion if you decide to preserve compatibility for the seed-only paths.

Reproduction script (run from the repo root after cargo build -p tvc)

Needs openssl, python3, and script(1). Builds a throwaway $HOME with a real p256 API key and a one-shot whoami server on port 8791, so login gets past credential verification — same trick as tests/pty.rs::spawn_whoami_server.

#!/usr/bin/env bash
# Acceptance probes for PR #236. Run from the repo root after `cargo build -p tvc`.
set -uo pipefail

TVC=./target/debug/tvc
FIX=$(mktemp -d)
ORG="$FIX/.config/turnkey/orgs/alias-a"
mkdir -p "$ORG"

# A real p256 API key, so `login` gets past credential loading.
openssl ecparam -name prime256v1 -genkey -noout -out "$FIX/k.pem" 2>/dev/null
PRIV=$(openssl ec -in "$FIX/k.pem" -text -noout 2>/dev/null \
  | awk '/priv:/{f=1;next}/pub:/{f=0}f' | tr -d ' :\n')
PUB=$(openssl ec -in "$FIX/k.pem" -pubout -conv_form compressed -outform DER 2>/dev/null \
  | xxd -p | tr -d '\n' | tail -c 66)
python3 - "$ORG" "$PRIV" "$PUB" <<'PY'
import json, os, secrets, sys
org, priv, pub = sys.argv[1], sys.argv[2].rjust(64, "0"), sys.argv[3]
json.dump({"public_key": pub, "private_key": priv, "curve": "p256"},
          open(os.path.join(org, "api_key.json"), "w"))
# A well-formed operator key: 130-byte public, 32-byte seed.
json.dump({"public_key": secrets.token_hex(130), "private_key": secrets.token_hex(32)},
          open(os.path.join(org, "operator.json"), "w"))
PY

cat > "$FIX/.config/turnkey/tvc.config.toml" <<EOF
version = 1
active_org = "alias-a"

[orgs.alias-a]
id = "org-e2e"
api_key_path = "$ORG/api_key.json"
api_base_url = "http://127.0.0.1:8791"
default_operator_kind = "local"

[[orgs.alias-a.operators]]
name = "default"
kind = "local"
key_path = "$ORG/operator.json"
EOF

echo "### 1. no --output, stdin is a pipe, TVC_NON_INTERACTIVE unset"
HOME="$FIX" $TVC keys backup-operator-key < /dev/null 2>&1; echo "   exit=$?"

echo
echo "### 2. --output exists, stdin is a pipe, TVC_NON_INTERACTIVE unset"
echo previous > "$FIX/bk.json"
HOME="$FIX" $TVC keys backup-operator-key --output "$FIX/bk.json" < /dev/null 2>&1; echo "   exit=$?"

echo
echo "### 3. permissions of a written backup"
rm -f "$FIX/bk2.json"
HOME="$FIX" TVC_NON_INTERACTIVE=1 $TVC keys backup-operator-key --output "$FIX/bk2.json" >/dev/null 2>&1
ls -l "$FIX/bk2.json" | awk '{print "   "$1"  "$NF}'

echo
echo "### 4. operator key whose private key cannot derive its public key"
python3 -c "
import json,secrets
json.dump({'public_key':secrets.token_hex(130),'private_key':'not-even-hex'},
          open('$ORG/operator.json','w'))"
rm -f "$FIX/bk3.json"
HOME="$FIX" TVC_NON_INTERACTIVE=1 $TVC keys backup-operator-key --output "$FIX/bk3.json" 2>&1 | head -2
echo "   exit=${PIPESTATUS[0]}"
python3 -c "
import json,secrets
json.dump({'public_key':secrets.token_hex(130),'private_key':secrets.token_hex(32)},
          open('$ORG/operator.json','w'))"

echo
echo "### 5. login: cancel the backup prompt after the key is already saved"
python3 - <<'PY' &
from http.server import BaseHTTPRequestHandler, HTTPServer
B = b'{"organizationId":"org-e2e","organizationName":"E2E Org","userId":"user-1","username":"e2e"}'
class H(BaseHTTPRequestHandler):
    def do_POST(self):
        self.rfile.read(int(self.headers.get("content-length", 0)))
        self.send_response(200)
        self.send_header("Content-Type", "application/json")
        self.send_header("Content-Length", str(len(B)))
        self.end_headers()
        self.wfile.write(B)
    def log_message(self, *a): pass
HTTPServer(("127.0.0.1", 8791), H).serve_forever()
PY
SERVER=$!
sleep 1
rm -f "$ORG/operator.json"
OUT=$(printf '\x04' | HOME="$FIX" script -q /dev/null $TVC login --org alias-a 2>&1)
CODE=$?
echo "   exit=$CODE"
echo "$OUT" | tr -d '\r' | tail -1 | sed 's/^/   /'
echo "$OUT" | grep -q "Successfully logged in" \
  && echo "   reported success: yes" || echo "   reported success: NO"
test -f "$ORG/operator.json" \
  && echo "   operator key on disk: yes (login had already succeeded)" \
  || echo "   operator key on disk: no"
kill $SERVER 2>/dev/null
rm -rf "$FIX"

daniilrrr · 2026-08-10T20:53:53Z

+        shell_println!(ctx)?;
+        shell_println!(
+            ctx,
+            "This key exists only on this machine; if it's lost you cannot \


(human) add a "WARNING" or similar callout here? kinda a big deal imo

Re-signed: 268091e

daniilrrr · 2026-08-10T21:03:48Z

+        // Non-interactive mode cannot prompt for the destination; reject
+        // before loading config or resolving the organization.
+        if ctx.is_non_interactive() && self.output.is_none() {
+            return Err(error_required_in_non_interactive("--output"));
+        }


(AGENT) blocking — piped stdin falls through to a prompt that cannot run

ctx.is_non_interactive() is only the --non-interactive flag plus JSON mode (output.rs:204); it says nothing about whether stdin is a TTY. So a piped run without the env var reaches prompts::text at line 63 and dies in inquire:

$ tvc keys backup-operator-key < /dev/null error: The input device is not a TTY (exit 1)

instead of --output is required in non-interactive mode. deploy/approve.rs:121 guards with ctx.is_non_interactive() || !stdin_can_prompt(), and so does the login nudge added in this same PR (login.rs:608) — this is the one call site that doesn't.

Binding the decision once also fixes the overwrite branch (see the comment on line 78), which has the same hole:

Suggested change

// Non-interactive mode cannot prompt for the destination; reject

// before loading config or resolving the organization.

if ctx.is_non_interactive() && self.output.is_none() {

return Err(error_required_in_non_interactive("--output"));

}

// Reject before loading config or resolving the organization when

// there is no way to prompt for the destination: --non-interactive,

// JSON mode, or a non-TTY stdin.

let can_prompt = !ctx.is_non_interactive() && prompts::stdin_can_prompt();

if !can_prompt && self.output.is_none() {

return Err(error_required_in_non_interactive("--output"));

}

Needs prompts::stdin_can_prompt added to the use at line 9.

(human) good call

Tracked in https://linear.app/turnkey/issue/TVC-278/

Re-signed: 91ddd64

daniilrrr · 2026-08-10T21:03:49Z

+        if destination.exists() && !self.overwrite {
+            if ctx.is_non_interactive() {
+                bail!(
+                    "destination {} already exists; pass --overwrite to replace it",
+                    destination.display()
+                );
+            }
+
+            prompts::confirm_or_bail(&format!("Overwrite {}?", destination.display()), "backup")?;
+        }


(AGENT) blocking — same TTY hole on the overwrite path

Second half of the fix on line 42. As written, a piped run whose --output already exists skips the bail! (the flag isn't set) and calls confirm_or_bail, which fails with The input device is not a TTY rather than the actionable pass --overwrite to replace it.

Reusing can_prompt from the suggestion above:

Suggested change

if destination.exists() && !self.overwrite {

if ctx.is_non_interactive() {

bail!(

"destination {} already exists; pass --overwrite to replace it",

destination.display()

);

}

prompts::confirm_or_bail(&format!("Overwrite {}?", destination.display()), "backup")?;

}

if destination.exists() && !self.overwrite {

if !can_prompt {

bail!(

"destination {} already exists; pass --overwrite to replace it",

destination.display()

);

}

prompts::confirm_or_bail(&format!("Overwrite {}?", destination.display()), "backup")?;

}

Apply together with the line 42 suggestion — neither compiles alone.

(human) agree

Tracked in https://linear.app/turnkey/issue/TVC-278/

Re-signed: 91ddd64

daniilrrr · 2026-08-10T21:03:50Z

+    // Written with default (umask) permissions, matching
+    // `StoredQosOperatorKey::save`; tightening both is tracked by TVC-241.
+    tokio::fs::write(&destination, &bytes)
+        .await
+        .with_context(|| format!("failed to write backup: {}", destination.display()))?;


(AGENT) blocking — the backup of a private key is created world-readable

Deferring StoredQosOperatorKey::save's permissions to TVC-241 is reasonable; deferring them here is less so. This command's entire purpose is writing private key material to a new path the user chose, and the output text three lines later tells them to treat the file as a secret ("The backup contains the PRIVATE key. Store it somewhere safe"). At umask defaults that file is typically 0644. The code should agree with the advice it prints, and it's a local fix rather than a cross-cutting one:

Suggested change

// Written with default (umask) permissions, matching

// `StoredQosOperatorKey::save`; tightening both is tracked by TVC-241.

tokio::fs::write(&destination, &bytes)

.await

.with_context(|| format!("failed to write backup: {}", destination.display()))?;

// The backup holds private key material, so create it 0600 rather than at

// umask defaults. TVC-241 tracks the same tightening for the primary key

// file, which this command reads but does not write.

let mut options = tokio::fs::OpenOptions::new();

options.write(true).create(true).truncate(true);

#[cfg(unix)]

options.mode(0o600);

let write_context = || format!("failed to write backup: {}", destination.display());

options

.open(&destination)

.await

.with_context(write_context)?

.write_all(&bytes)

.await

.with_context(write_context)?;

Needs use tokio::io::AsyncWriteExt; and, under #[cfg(unix)], use std::os::unix::fs::OpenOptionsExt;. Happy to be told this belongs in TVC-241 with the other one instead — but then it's worth saying so in the doc comment, since a reader currently sees "matching save" and may not realize that means 0644.

(human) I'm not familiar with Unix conventions but this seems fine, albeit not blocking imo

I think I disagree with all the robots here. This should be a read and copy. We shouldn't have to think about perms for backup, just copy the original file's perms. Also, copy as atomic and allows the OS to handle the copy in the most efficient way

Tracked in https://linear.app/turnkey/issue/TVC-278/

Re-signed: 91ddd64

daniilrrr · 2026-08-10T21:03:51Z

+    let key: StoredQosOperatorKey = serde_json::from_slice(&bytes).with_context(|| {
+        format!(
+            "operator key at {} is not a valid operator key file",
+            source.display()
+        )
+    })?;


(AGENT) medium — "parsed to validate it" is a much weaker claim than it reads as

The doc at line 95 says the source "is parsed to validate it." What's actually checked is 130 bytes of hex on the public key. private_key is still an unparsed String — which means the one field that determines whether the backup is restorable is never looked at:

$ cat operator.json # public_key: 130 random bytes, private_key: "not-even-hex" $ tvc keys backup-operator-key --output bk.json Operator key backed up! (exit 0)

A backup that cannot be restored reports success in green. Per AGENTS.md's parse-don't-validate rule this is the remaining unparsed field on the DTO, and the parser already exists (HexSeed / LocalPair::from_hex_seed, pair.rs:30/:67) — plus LocalPair::public_key() returns the same composite encoding, so the round-trip check is nearly free:

Suggested change

let key: StoredQosOperatorKey = serde_json::from_slice(&bytes).with_context(|| {

format!(

"operator key at {} is not a valid operator key file",

source.display()

)

})?;

let key: StoredQosOperatorKey = serde_json::from_slice(&bytes).with_context(|| {

format!(

"operator key at {} is not a valid operator key file",

source.display()

)

})?;

// A backup is only worth reporting as one if it can be restored, so prove

// the stored seed derives the stored public key before copying.

let derived = LocalPair::from_hex_seed(&key.private_key).with_context(|| {

format!(

"operator key at {} has an unusable private key",

source.display()

)

})?;

if QosOperatorPublicKey::try_from(derived.public_key().as_slice())? != key.public_key {

bail!(

"operator key at {} is inconsistent: its private key does not derive its public key",

source.display()

);

}

Needs use crate::pair::{LocalPair, Pair};. If you'd rather keep back_up cheap and I/O-only, the alternative is to soften the doc comment — but then the success message probably shouldn't be as confident as it is.

(human) I think this is good type tightening for the private key. I also think that maybe we could store it as something besides a String for additional redaction-safety for logging

Yeah, we also don't use zeorize in here. I think this is a larger refactor though

Tracked in https://linear.app/turnkey/issue/TVC-278/

this is my biggest concern in the PR, because it's an easy fix to delay, forget about, then accidentally log or leak private key info. Any chance we can store it as HexSeed in this PR? If not, can TVC-278 be like an immediate follow ticket?

Yeah, I actually already did a bunch of TVC-278 in here, just not this one - so can follow up right after. The issue that's described here is that we don't properly validate the key before backing it up, but it's a corner case. We shouldn't be backing up any keys that weren't created by the cli to begin with. You'd have to go in and manually manipulate the key.

I agree that it should happen and we don't want to keep the key as a string, etc etc... but all these things existed before this ticket. I'm not making the security posture worse with this backup mechanism

daniilrrr · 2026-08-10T21:03:59Z

+        let backed_up = if prompts::confirm("Back up your operator key now?", true)? {
+            let destination: PathBuf = prompts::text(
+                "Backup file path",
+                Some(&format!("operator-{org_alias}-backup.json")),
+            )?
+            .into();
+
+            // Declining the overwrite skips the backup rather than failing:
+            // login has already succeeded and must not die here.
+            let proceed = !destination.exists()
+                || prompts::confirm(&format!("Overwrite {}?", destination.display()), false)?;
+
+            if proceed {
+                match back_up(org_alias.to_string(), local.key_path.clone(), destination).await {
+                    Ok(report) => Some(report),
+                    Err(error) => {
+                        // Deliberately swallowed at this endpoint: the backup
+                        // is advisory and the login outcome must still land.
+                        shell_eprintln!(ctx, "WARNING: backup failed: {error:#}")?;
+                        None
+                    }
+                }
+            } else {
+                None
+            }
+        } else {
+            None
+        };


(AGENT) blocking — "must not die here" only holds for backup errors, not for the prompts

The comment on line 623 commits to login surviving this block, and the back_up error path honours it. The prompts don't: prompts::confirm and prompts::text return Result, and every one of them is ?-propagated out of find_or_generate_operator_key into execute_login. Escape or Ctrl-C at "Back up your operator key now?" or at "Backup file path" therefore aborts login with a non-zero exit and no Successfully logged in! output — for a run that had already saved the config (line 336) and both key files (line 589) and genuinely succeeded.

Routing the whole attempt through one error sink makes the comment true, and folds in the missing is_dir guard (see the note on line 629):

Suggested change

let backed_up = if prompts::confirm("Back up your operator key now?", true)? {

let destination: PathBuf = prompts::text(

"Backup file path",

Some(&format!("operator-{org_alias}-backup.json")),

)?

.into();

// Declining the overwrite skips the backup rather than failing:

// login has already succeeded and must not die here.

let proceed = !destination.exists()

|| prompts::confirm(&format!("Overwrite {}?", destination.display()), false)?;

if proceed {

match back_up(org_alias.to_string(), local.key_path.clone(), destination).await {

Ok(report) => Some(report),

Err(error) => {

// Deliberately swallowed at this endpoint: the backup

// is advisory and the login outcome must still land.

shell_eprintln!(ctx, "WARNING: backup failed: {error:#}")?;

None

}

}

} else {

None

}

} else {

None

};

// Everything below is advisory: a prompt the user escapes out of and a

// backup that fails both degrade to a warning, because the config and

// both key files are already saved by this point and the login outcome

// must still land.

let attempt: Result<Option<OperatorKeyBackedUp>> = async {

if !prompts::confirm("Back up your operator key now?", true)? {

return Ok(None);

}

let destination: PathBuf = prompts::text(

"Backup file path",

Some(&format!("operator-{org_alias}-backup.json")),

)?

.into();

if destination.is_dir() {

bail!(

"destination {} is a directory; include a file name",

destination.display()

);

}

if destination.exists()

&& !prompts::confirm(&format!("Overwrite {}?", destination.display()), false)?

{

return Ok(None);

}

back_up(org_alias.to_string(), local.key_path.clone(), destination)

.await

.map(Some)

}

.await;

let backed_up = match attempt {

Ok(report) => report,

Err(error) => {

shell_eprintln!(ctx, "WARNING: backup skipped: {error:#}")?;

None

}

};

Needs OperatorKeyBackedUp added to the back_up import on line 4. A PTY test that sends \x1b (or closes the pty) at the destination prompt and still expects Successfully logged in! would lock this in — the three existing tests all drive the happy paths.

(human) I think this is a good shout

Tracked in https://linear.app/turnkey/issue/TVC-278/

Re-signed: 268091e

daniilrrr · 2026-08-10T21:04:00Z

+                || prompts::confirm(&format!("Overwrite {}?", destination.display()), false)?;
+
+            if proceed {
+                match back_up(org_alias.to_string(), local.key_path.clone(), destination).await {


(AGENT) medium — the nudge re-implements Args::run's prompt logic, and the two have already drifted

back_up is shared, but the ~15 lines above it (default destination, existence check, overwrite confirmation) are a second copy of backup_operator_key.rs:62-86, and they already disagree:

standalone login nudge

directory destination rejected up front (:70) not checked

overwrite declined confirm_or_bail → error skip, keep going

The second row is a deliberate and correct difference. The first isn't: type a directory at login's prompt and you get Overwrite /some/dir? → yes → WARNING: backup failed: <raw io error>. That's exactly the drift the duplication predicts, and there will be more of it the next time either prompt changes.

Suggest lifting a pub(crate) fn prompt_for_backup_destination(alias: &str) -> Result<Option<PathBuf>> into backup_operator_key.rs next to back_up — it owns the default name, the is_dir rejection, and the overwrite question, and returns None for "user declined". Both call sites then differ only in how they treat None. (The suggestion on line 616 patches the is_dir symptom inline; this is the version that stops it recurring.)

(human) - sure, esp if they've drifted

Done: 91ddd64 (shared prompt_for_backup_destination), 268091e (login call site)

daniilrrr · 2026-08-10T21:04:01Z

    if let Some(operator_key) = StoredQosOperatorKey::load(&local.key_path).await? {
        debug!("using existing operator key");
        shell_println!(ctx, "Using existing operator key.")?;
+        shell_println!(ctx, "Tip: back it up with `tvc keys backup-operator-key`.")?;


(AGENT) nit — unconditional tip on every re-login

Correctly suppressed in JSON mode (shell_println! goes through the human sink), but it still prints on every --non-interactive human run and on every interactive re-login, forever, with no notion of whether a backup already exists. As a stopgap for TVC-53 that's defensible — there's no backup state to check against. Two smaller options if you want to cut the nagging: gate it on prompts::stdin_can_prompt() so scripted runs stay quiet, or record the last backup path in the local operator record and only nudge when it's absent.

(human) I think this is fine, up to you

Tracked in https://linear.app/turnkey/issue/TVC-282/

daniilrrr · 2026-08-10T21:04:03Z

+/// One-shot mock Turnkey API that answers the whoami query, enough to carry
+/// login past its credential verification and into the operator-key flow.
+/// Same shape as `tests/error_output.rs::spawn_json_server`.
+fn spawn_whoami_server() -> (String, JoinHandle<()>) {


(AGENT) nit — this helper's home is tests/common.rs

The doc comment already says it: "Same shape as tests/error_output.rs::spawn_json_server." Now that tests/common.rs exists in this very commit, the duplicate should move there rather than be documented as a duplicate. common.rs is #![allow(dead_code)] precisely so per-binary unused helpers are fine.

Also worth a line noting the one-shot contract — the thread accept()s exactly once, so a login that made a second request would see connection-refused rather than a hang. That's the right call for these tests, it's just invisible to the next person adding one.

Unrelated to the mechanics: carrying PTY login past credential verification is a genuinely useful unlock, and having all three onboarding paths covered end-to-end is more than most CLI onboarding flows get.

(human) agree

Tracked in https://linear.app/turnkey/issue/TVC-280/

daniilrrr · 2026-08-11T20:45:09Z

+        // Not `Self(Default::default())`: std's array `Default` stops at 32
+        // elements, so the repeat expression delegates per element instead.


dont think this comment is still relevant?

daniilrrr · 2026-08-11T20:46:18Z

            &path,
            serde_json::to_string(&StoredQosOperatorKey {
-                public_key: "unused".to_string(),
+                // The resolve path only reads the seed; a nil key stands in.


rm redundant comment

daniilrrr · 2026-08-11T20:47:59Z

+//! Each test binary that declares `mod common;` compiles its own copy and
+//! uses a subset of these helpers, so unused items are expected in every
+//! build of this file; the crate-level allow keeps those builds quiet.


last phrase in the paragraph is definitely false and should be removed

i dont think any of this comment is true, I see every func being used and no dead code

daniilrrr · 2026-08-11T21:01:49Z

+    let key: StoredQosOperatorKey = serde_json::from_slice(&bytes).with_context(|| {
+        format!(
+            "operator key at {} is not a valid operator key file",
+            source.display()
+        )
+    })?;


this is my biggest concern in the PR, because it's an easy fix to delay, forget about, then accidentally log or leak private key info. Any chance we can store it as HexSeed in this PR? If not, can TVC-278 be like an immediate follow ticket?

r-n-o

+1 from a dependency-reviewers POV. No new deps ✅

richardpringle requested a review from a team as a code owner August 10, 2026 19:52

emostov previously approved these changes Aug 10, 2026

View reviewed changes

richardpringle mentioned this pull request Aug 10, 2026

refactor(tvc): seal operator backends behind a Signer port #237

Merged

daniilrrr reviewed Aug 10, 2026

View reviewed changes

richardpringle mentioned this pull request Aug 11, 2026

fix(tvc): one profile per organization, id-keyed key dirs, and eager config migration #241

Open

richardpringle dismissed emostov’s stale review via 5ae8e3a August 11, 2026 16:29

richardpringle force-pushed the richard/224-breakup-1 branch from 2d1908d to 5ae8e3a Compare August 11, 2026 16:29

refactor(tvc): parse stored operator public keys into a typed composite

4f402bf

richardpringle force-pushed the richard/224-breakup-1 branch 5 times, most recently from 0fe438b to e00d10f Compare August 11, 2026 20:03

richardpringle added 2 commits August 11, 2026 16:03

feat(tvc): add keys backup-operator-key command

91ddd64

feat(tvc): offer operator key backup during login onboarding

268091e

richardpringle force-pushed the richard/224-breakup-1 branch from e00d10f to 268091e Compare August 11, 2026 20:04

richardpringle requested review from daniilrrr and emostov August 11, 2026 20:26

daniilrrr reviewed Aug 11, 2026

View reviewed changes

daniilrrr approved these changes Aug 12, 2026

View reviewed changes

r-n-o approved these changes Aug 12, 2026

View reviewed changes

richardpringle merged commit 48c9d5a into main Aug 12, 2026
3 checks passed

-    // Written with default (umask) permissions, matching
-    // `StoredQosOperatorKey::save`; tightening both is tracked by TVC-241.
-    tokio::fs::write(&destination, &bytes)
-        .await
-        .with_context(|| format!("failed to write backup: {}", destination.display()))?;
+    // The backup holds private key material, so create it 0600 rather than at
+    // umask defaults. TVC-241 tracks the same tightening for the primary key
+    // file, which this command reads but does not write.
+    let mut options = tokio::fs::OpenOptions::new();
+    options.write(true).create(true).truncate(true);
+    #[cfg(unix)]
+    options.mode(0o600);
+    let write_context = || format!("failed to write backup: {}", destination.display());
+    options
+        .open(&destination)
+        .await
+        .with_context(write_context)?
+        .write_all(&bytes)
+        .await
+        .with_context(write_context)?;

	standalone	login nudge
directory destination	rejected up front (`:70`)	not checked
overwrite declined	`confirm_or_bail` → error	skip, keep going

		// Not `Self(Default::default())`: std's array `Default` stops at 32
		// elements, so the repeat expression delegates per element instead.

Uh oh!

Conversation

richardpringle commented Aug 10, 2026

Typed operator public key

TVC-52 — tvc keys backup-operator-key

TVC-53 — onboarding backup nudge

Uh oh!

emostov left a comment

Choose a reason for hiding this comment

Uh oh!

daniilrrr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

1. No --output, stdin is a pipe, TVC_NON_INTERACTIVE unset

2. --output already exists, stdin is a pipe, TVC_NON_INTERACTIVE unset

3. Permissions of a written backup

4. An operator key whose private key cannot derive its public key

5. login: cancelling the backup prompt kills a login that already succeeded

Not on this list, deliberately

Uh oh!

daniilrrr Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TVC-52 — `tvc keys backup-operator-key`

daniilrrr left a comment •

edited

Loading

1. No `--output`, stdin is a pipe, `TVC_NON_INTERACTIVE` unset

2. `--output` already exists, stdin is a pipe, `TVC_NON_INTERACTIVE` unset

5. `login`: cancelling the backup prompt kills a login that already succeeded

daniilrrr Aug 10, 2026 •

edited

Loading