TVC-66: Make non-interactive mode explicit in tvc

[richardpringle]

Linear: TVC-66

Summary:

• Add a global --non-interactive flag backed by TVC_NON_INTERACTIVE, using clap boolean parsing so falsey values do not force non-interactive mode.
• Introduce explicit InteractionMode plumbing for commands that may prompt, keeping prompt helpers as TTY UI primitives.
• Make non-interactive config/login paths fail fast without prompting or offering to save filled config, and document the behavior in help output.

Tests:

• cargo fmt -p tvc -- --check
• cargo test -p tvc login
• cargo test -p tvc non_interactive
• cargo test -p tvc deploy_create_help

@tkhq/dependency-reviewers, I added thiserror as a dependency to tvc. That's the only change, it was already in the dependency tree.

[daniilrrr]

this is really nice, hard work that will make the tvc SDK much clearer to use and maintain. great job

i have some changes I would like to see throughout

[daniilrrr]

remember to delete this file

[daniilrrr]

requires dep reviewer but thiserror is v commonly used, so I don't expect any issue

[richardpringle]

Yeah, it's already a dep in the workspace so I'm not actually adding anything new, just exposing it to tvc.

[daniilrrr]

for every command here that has an interactive and non-interactive version, let's do an explicit match

DeployCommands::Approve(args) => {
                    match non_interactive {
                        true => {commands::deploy::approve::run_non_interactive(args).await}
                        false => {commands::deploy::approve::run_interactive(args).await}
                    }
                }

I think the current version is a little over-LLM'd and relies on the ordering of the commands to be correct at the expense of clarity.

---

a second point is that, at a glance, in order to make maintainability easier I believe all of these funcs with 2 versions should instead be

run_interactive(args) -> run(args: Args, is_non_interactive: bool)

to not have to maintain 2 code paths for each interactive command.

I plan to argue that on a separate comment as I read the rest of the PR. We will see if I retain this view

[daniilrrr]

having read the PR, you've done a nice job flattening the logic of each run() command into a (1) setup and config phase, then (2) post SDK API call phase. It is (1) that differs between interactive and non-interactive, while the rest is standard

To me, it's better for maintenance and discoverability to have a diamond shaped

1. run()
   2a. setup_non_interactive(), or
   2b. setup_interactive()
2. post()

let me know what you think

[richardpringle]

The goal here was to flatten everything out as much as possible. I even added the guards instead of the if-statements the LLM produced (matching on a bool will fail clippy 😉). I honestly prefer a single match statement with no nested logic, the nature if guards and wildcard matches means that order matters on every match statement.

With that said, I don't mind adding the if-statements back.

As far as your suggestion of setup_(non_)interavtive, I'm not sure I see how that changes things. I think this one is worth hopping on a call to talk about so I can understand the benefit of what you're suggesting

[daniilrrr]

extract "<FILL_IN" as a constant to use everywhere and prevent fat finger bugs later

[richardpringle]

I'll probably leave this one in this PR and do a single pass of "floating" numbers are strings in the code, everything that is a constant should be defined as such (effectively just named constants). There are a bunch more below too.

[daniilrrr]

this is clever but a little indirect. I understand why you did it.

personal opinion but I don't like how it implies that errors will always be produced as opposed to ending with Ok(()) in the happy path. This is a nit tho

i think for a little extra clarity you could rename here and in deploy.rs

AppConfigValidationErrors::ok_or_errors(errors)

[richardpringle]

This is a nit tho

This one was hand-written and I definitely got the ick writing it haha. It was better than always having to check for is_empty every time though.

Your name gets rid of the ick haha, thanks!

[daniilrrr]

do the underscore version here (app_id) to be consistent with app.rs/validate() func, not the flag version

[daniilrrr]

icky unreachable

i have a way to kill this unreachable! that you may like

refactor the bail_ to use a public inner helper

pub fn bail_required_in_non_interactive(flag_hint: &str) -> Result<()> {
    Err(error_required_in_non_interactive(flag_hint))
}

pub fn error_required_in_non_interactive(flag_hint: &str) -> Error {
    anyhow!(
        "{flag_hint} is required in non-interactive mode \
         (set {flag_hint} or run in a TTY without --non-interactive \
         / {NON_INTERACTIVE_ENV}=true)"
    )
}

then call the inner helper here in a one-liner

    let org_query = args.org.ok_or_else(|| error_required_in_non_interactive("--org"))?;

[richardpringle]

Now this is dumb LLM code. I can just use an else-return here.

[daniilrrr]

fabulous refactoring job, really nice to centralize everything into shared code

[daniilrrr]

not just here but this pattern of shared "core" logic in the other non/interactive commands is great

[daniilrrr]

at the risk of overengineering, should we refactor these checks into the prompts package and do them at the beginning of every run_interactive() and run_non_interactive() version of EVERY command for safety? They're already duplicated between deploy/init.rs here and app/init.rs. Something like

/// In interactive mode, the `--interactive` fill flag only requires a real TTY.
  pub fn require_tty_for_interactive(interactive: bool) -> Result<()> {
      if interactive {
          ensure_stdin_is_tty()?;
      }
      Ok(())
  }

  /// In non-interactive mode, the `--interactive` fill flag is contradictory.
  pub fn reject_interactive_flag(interactive: bool) -> Result<()> {
      if interactive {
          bail_interactive_conflicts_with_non_interactive()?;
      }
      Ok(())
  }

  Then both app/init.rs and deploy/init.rs collapse to:

  pub async fn run_interactive(args: Args) -> Result<()> {
      prompts::require_tty_for_interactive(args.interactive)?;
      run(args).await
  }

  pub async fn run_non_interactive(args: Args) -> Result<()> {
      prompts::reject_interactive_flag(args.interactive)?;
      run(args).await
  }

[richardpringle]

🤔, I don't think this is overengineering, this is a good callout so we don't have to remember to add the checks to future commands

[daniilrrr]

nit - reduce this long conditional and just return early instead.

if args.skip_post { return Ok(()); }

makes it easier for me to read and IMO, usually, any set of nested braces you can kill is a win

[daniilrrr]

once you return early, the shape is clearer to me and can be refactored to a flatter procedure without conditionals

1. try to resolve the operator_id, if not configured
2. construct PostApprovalPlan
3. post_approval_to_api()

which reads nicely

[daniilrrr]

can also collapse operator resolution logic between here and the run_non_interactive() part

[daniilrrr]

LLM code below. because resolve_operator_id() is used by both paths we need to pass it a bool indicating the origin of the call, which is against the overall spirit of your PR, but it's preeeetty clean

pub async fn run_interactive(args: Args) -> anyhow::Result<()> {
      let do_prompt_user = !args.dangerous_skip_interactive;

      // Guard: bail fast before fetching the manifest if review prompts are
      // required but the caller has no TTY to answer them.
      if do_prompt_user && !stdin_can_prompt() {
          bail_required_in_non_interactive("--dangerous-skip-interactive")?;
      }

      let (manifest, fetched_manifest_id) = load_manifest(&args).await?;

      if do_prompt_user {
          interactive_approve(&manifest)?;
      }

      if args.dry_run {
          println!("Dry run complete. No approval generated.");
          return Ok(());
      }

      let approval = sign_and_output(&args, &manifest).await?;

      if args.skip_post {
          return Ok(());
      }

      post_approval(
          &args,
          &approval,
          fetched_manifest_id.as_deref(),
          OperatorPrompt::Allow,
      )
      .await
  }

  pub async fn run_non_interactive(args: Args) -> anyhow::Result<()> {
      if !args.dangerous_skip_interactive {
          bail_required_in_non_interactive("--dangerous-skip-interactive")?;
      }

      let (manifest, fetched_manifest_id) = load_manifest(&args).await?;

      if args.dry_run {
          println!("Dry run complete. No approval generated.");
          return Ok(());
      }

      let approval = sign_and_output(&args, &manifest).await?;

      if args.skip_post {
          return Ok(());
      }

      post_approval(
          &args,
          &approval,
          fetched_manifest_id.as_deref(),
          OperatorPrompt::Disallow,
      )
      .await
  }

  And the helpers they rely on (place near PostApprovalPlan):

  /// Whether operator-id disambiguation may fall back to an interactive prompt.
  enum OperatorPrompt {
      Allow,
      Disallow,
  }

  async fn post_approval(
      args: &Args,
      approval: &Approval,
      fetched_manifest_id: Option<&str>,
      prompt: OperatorPrompt,
  ) -> anyhow::Result<()> {
      let manifest_id = resolve_manifest_id(args, fetched_manifest_id)?;
      let operator_id = resolve_operator_id(args, prompt).await?;

      let plan = PostApprovalPlan {
          manifest_id: &manifest_id,
          operator_id: &operator_id,
      };
      post_approval_to_api(plan, approval).await
  }

  async fn resolve_operator_id(args: &Args, prompt: OperatorPrompt) -> anyhow::Result<String> {
      if let Some(id) = &args.operator_id {
          return Ok(id.clone());
      }

      let saved_ids = load_saved_operator_ids().await?;
      match saved_ids.as_slice() {
          [] => bail!(
              "--operator-id is required to post approval to API. \
               No saved operator IDs found. \
               Use --skip-post to only generate the approval locally."
          ),
          [id] => Ok(id.clone()),
          _ if matches!(prompt, OperatorPrompt::Allow) && stdin_can_prompt() => Ok(prompts::select(
              "Select approving operator",
              saved_ids.iter().map(String::as_str).collect::<Vec<_>>(),
          )?
          .to_string()),
          _ => bail!(
              "--operator-id is required to post approval to API when multiple saved operator IDs are available"
          ),
      }
  }

[richardpringle]

@daniilrrr FYI, you can use ```rust to get syntax highlighting in comments

[richardpringle]

Part of the point of the whole PR is to remove the bool "can" prompt logic. I think it's a lot harder to maintain and I base that on my first pass at implementing this feature. There's conditional logic everywhere. That means that every helper function needs 2x the amount of tests to get full coverage. Instead, the ethos here is to flatten conditional logic as much as possible at the small cost of some duplication. IMO, the maintenance cost of the duplication is much much lower.

With all that said, you've made me realize that I'm missing a bunch of unit tests that would clearly emphasize my point a little better.

[richardpringle]

      if let Some(id) = &args.operator_id {
          return Ok(id.clone());
      }

This is an interesting suggestion. Do I go overboard avoiding allocations? Absolutely. But I find it easier to write code where I minimize allocation by default. It makes code a lot easier to change when you do discover a hot-loop that's needlessly allocating. In a cli, optimizing for fewer allocations is almost certainly never going to happen, but if you generally avoid needlessly cloning, you never have to ask that question. I dont' think the cost is very high here (it's calling post_approval_to_api in each branch instead of once).

If the function was more than 100 lines or something, some of these things could be factored out differently, but I think that's only worth it when the function starts getting longer.

[daniilrrr]

Instead, the ethos here is to flatten conditional logic as much as possible at the small cost of some duplication.

fair tradeoff. add this sentence to the PR to make it crystal clear what your goal is :)

But I find it easier to write code where I minimize allocation by default. It makes code a lot easier to change when you do discover a hot-loop that's needlessly allocating.

that's a good pointer that I will try and incorporate in my future code, thanks

---

What I'm getting at is that in the other commands you have a super nice flattening of the commands into interactive / non interactive funcs to call and have removed the conditional logic, except here where we still hinge on if let Some(id) = &args.operator_id { ... } else {...}. I think it could be cleaner and more testable to to have resolve_operator_id_interactive() and resolve_operator_id_noninteractive() here. The LLM code did not correctly illustrate my example. I'll paste something better in a few min, hopefully

[daniilrrr]

pretty flat logic example, curious what you think

LLM code below:

pub async fn run_interactive(args: Args) -> anyhow::Result<()> {
    let do_prompt_user = !args.dangerous_skip_interactive;

    // Guard: bail fast before fetching the manifest if review prompts are
    // required but the caller has no TTY to answer them.
    if do_prompt_user && !stdin_can_prompt() {
        bail_required_in_non_interactive("--dangerous-skip-interactive")?;
    }

    let (manifest, fetched_manifest_id) = load_manifest(&args).await?;

    if do_prompt_user {
        interactive_approve(&manifest)?;
    }

    if args.dry_run {
        println!("Dry run complete. No approval generated.");
        return Ok(());
    }

    let approval = sign_and_output(&args, &manifest).await?;

    if args.skip_post { return Ok(()); }

    let manifest_id = resolve_manifest_id(&args, fetched_manifest_id.as_deref())?;
    let operator_id = resolve_operator_id_interactive().await?;

    let plan = PostApprovalPlan {
        manifest_id: manifest_id.as_str(),
        operator_id: &operator_id,
    };

    post_approval_to_api(plan, &approval).await?;

    Ok(())
}

pub async fn run_non_interactive(args: Args) -> anyhow::Result<()> {
    if !args.dangerous_skip_interactive {
        bail_required_in_non_interactive("--dangerous-skip-interactive")?;
    }

    let (manifest, fetched_manifest_id) = load_manifest(&args).await?;

    if args.dry_run {
        println!("Dry run complete. No approval generated.");
        return Ok(());
    }

    let approval = sign_and_output(&args, &manifest).await?;

    if args.skip_post { return Ok(()); }

    let manifest_id = resolve_manifest_id(&args, fetched_manifest_id.as_deref())?;
    let operator_id = resolve_operator_id_noninteractive().await?;

    let plan = PostApprovalPlan {
        manifest_id: manifest_id.as_str(),
        operator_id: &operator_id,
    };

    post_approval_to_api(plan, &approval).await?;

    Ok(())
}

...
and the 2 versions

async fn resolve_operator_id_interactive() -> anyhow::Result<String> {
    let saved_ids = load_saved_operator_ids().await?;
    match &*saved_ids {
        [] => bail!(
            "--operator-id is required to post approval to API. \
             No saved operator IDs found. \
             Use --skip-post to only generate the approval locally."
        ),
        [id] => Ok(id.clone()),
        _ if stdin_can_prompt() => Ok(prompts::select(
            "Select approving operator",
            saved_ids.iter().map(String::as_str).collect::<Vec<_>>(),
        )?
        .to_string()),
        _ => bail!(
            "--operator-id is required to post approval to API when multiple saved operator IDs are available"
        ),
    }
}

async fn resolve_operator_id_noninteractive() -> anyhow::Result<String> {
    let saved_ids = load_saved_operator_ids().await?;
    match &*saved_ids {
        [] => bail!(
            "--operator-id is required to post approval to API. \
             No saved operator IDs found. \
             Use --skip-post to only generate the approval locally."
        ),
        [id] => Ok(id.clone()),
        _ => bail!(
            "--operator-id is required to post approval to API when multiple saved operator IDs are available"
        ),
    }
}

[daniilrrr]

PR description out of date, InteractionMode not in this PR