Address remaining PR review comments, fix CI test failure, gate release to main tags

- params: change FromStr::Err from String to CliError so ? propagation works
- main: simplify param parse error handling now that Err is already CliError
- main: lock stdout once before jq output loop instead of per-iteration
- main: align --max-pages help text to match actual behavior
- README: align --max-pages description with CLI help
- login: fall back to stdin when no TTY (rpassword requires /dev/tty);
  fixes test_login_flow failure in CI (error 6: no such device or address)
- e2e: update test_login_flow ignore reason to reflect actual requirement
- integration: simplify test_param_parse_error_produces_usage_error now
  that parse error is already a CliError
- release.yml: remove pull_request trigger — release only fires on version tags
- release.yml: simplify plan job outputs (no more PR-vs-tag ternary logic)
- release.yml: add main-branch guard that rejects tags not on main

Author: ilyazub

.github/workflows/release.yml

@@ -39,7 +39,6 @@ permissions:
 # If there's a prerelease-style suffix to the version, then the release(s)
 # will be marked as a prerelease.
 on:
-  pull_request:
   push:
     tags:
       - '**[0-9]+.[0-9]+.[0-9]+*'
@@ -50,16 +49,23 @@ jobs:
     runs-on: "ubuntu-22.04"
     outputs:
       val: ${{ steps.plan.outputs.manifest }}
-      tag: ${{ !github.event.pull_request && github.ref_name || '' }}
-      tag-flag: ${{ !github.event.pull_request && format('--tag={0}', github.ref_name) || '' }}
-      publishing: ${{ !github.event.pull_request }}
+      tag: ${{ github.ref_name }}
+      tag-flag: ${{ format('--tag={0}', github.ref_name) }}
+      publishing: 'true'
     env:
       GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
     steps:
       - uses: actions/checkout@v6
         with:
-          persist-credentials: false
+          persist-credentials: true
           submodules: recursive
+      - name: Verify tag is on main branch
+        run: |
+          git fetch origin main --depth=50
+          if ! git merge-base --is-ancestor "${{ github.sha }}" origin/main; then
+            echo "::error::Release tags must be pushed from the main branch. Aborting."
+            exit 1
+          fi
       - name: Install dist
         # we specify bash to get pipefail; it guards against the `curl` command
         # failing. otherwise `sh` won't catch that `curl` returned non-0
@@ -70,14 +76,9 @@ jobs:
         with:
           name: cargo-dist-cache
           path: ~/.cargo/bin/dist
-      # sure would be cool if github gave us proper conditionals...
-      # so here's a doubly-nested ternary-via-truthiness to try to provide the best possible
-      # functionality based on whether this is a pull_request, and whether it's from a fork.
-      # (PRs run on the *source* but secrets are usually on the *target* -- that's *good*
-      # but also really annoying to build CI around when it needs secrets to work right.)
       - id: plan
         run: |
-          dist ${{ (!github.event.pull_request && format('host --steps=create --tag={0}', github.ref_name)) || 'plan' }} --output-format=json > plan-dist-manifest.json
+          dist host --steps=create --tag=${{ github.ref_name }} --output-format=json > plan-dist-manifest.json
           echo "dist ran successfully"
           cat plan-dist-manifest.json
           echo "manifest=$(jq -c "." plan-dist-manifest.json)" >> "$GITHUB_OUTPUT"

README.md

@@ -118,7 +118,7 @@ serpapi login
 - `--jq <expr>` — Client-side jq filter applied to JSON output (same as `gh --jq`)
 - `--api-key <key>` — Override API key (takes priority over environment and config file)
 - `--all-pages` — Fetch all result pages and merge array fields across pages
-- `--max-pages <n>` — Limit fetching to the first `n` pages when paginating (can be used with or without `--all-pages`)
+- `--max-pages <n>` — Maximum number of pages to fetch when using `--all-pages`
 
 **⚠️ Important: Flag Position**
 

src/commands/login.rs

@@ -2,14 +2,30 @@ use crate::commands::{check_api_error, make_client, network_err};
 use crate::config;
 use crate::error::CliError;
 use std::collections::HashMap;
+use std::io::{self, IsTerminal, Write};
 
 /// Prompt the user for their SerpApi API key, verify it, and persist it to the config file.
 pub async fn run() -> Result<(), CliError> {
-    let api_key = rpassword::prompt_password("Enter your SerpApi API key: ").map_err(|e| {
-        CliError::UsageError {
-            message: format!("Failed to read input: {e}"),
-        }
-    })?;
+    let api_key = if io::stdin().is_terminal() {
+        rpassword::prompt_password("Enter your SerpApi API key: ").map_err(|e| {
+            CliError::UsageError {
+                message: format!("Failed to read input: {e}"),
+            }
+        })?
+    } else {
+        // Non-interactive (piped/CI): print prompt to stderr and read from stdin
+        eprint!("Enter your SerpApi API key: ");
+        io::stderr().flush().map_err(|e| CliError::UsageError {
+            message: format!("Failed to flush stderr: {e}"),
+        })?;
+        let mut line = String::new();
+        io::stdin()
+            .read_line(&mut line)
+            .map_err(|e| CliError::UsageError {
+                message: format!("Failed to read input: {e}"),
+            })?;
+        line
+    };
     let api_key = api_key.trim();
 
     if api_key.is_empty() {

src/main.rs

@@ -36,7 +36,7 @@ enum Command {
         /// Fetch all pages and merge array results
         #[arg(long)]
         all_pages: bool,
-        /// Maximum number of pages to fetch; implies pagination (default: unlimited with --all-pages)
+        /// Maximum number of pages to fetch when paginating with --all-pages (default: unlimited)
         #[arg(long)]
         max_pages: Option<usize>,
     },
@@ -75,11 +75,7 @@ async fn main() {
                 .iter()
                 .map(|s| s.parse::<params::Param>())
                 .collect::<Result<Vec<_>, _>>()
-                .unwrap_or_else(|e| {
-                    die(error::CliError::UsageError {
-                        message: e.to_string(),
-                    })
-                });
+                .unwrap_or_else(|e| die(e));
             commands::search::run(
                 parsed_params,
                 &api_key,
@@ -98,11 +94,7 @@ async fn main() {
                 .iter()
                 .map(|s| s.parse::<params::Param>())
                 .collect::<Result<Vec<_>, _>>()
-                .unwrap_or_else(|e| {
-                    die(error::CliError::UsageError {
-                        message: e.to_string(),
-                    })
-                });
+                .unwrap_or_else(|e| die(e));
             commands::locations::run(parsed_params).await
         }
         Command::Archive { id } => {
@@ -125,8 +117,10 @@ async fn main() {
                         message: e.to_string(),
                     })
                 });
+                let stdout = std::io::stdout();
+                let mut out = stdout.lock();
                 for v in &results {
-                    if let Err(e) = output::print_jq_value(v, &mut std::io::stdout()) {
+                    if let Err(e) = output::print_jq_value(v, &mut out) {
                         die(error::CliError::ApiError {
                             message: format!("Output error: {e}"),
                         });

src/params.rs

@@ -8,21 +8,25 @@ pub struct Param {
 }
 
 impl FromStr for Param {
-    type Err = String;
+    type Err = crate::error::CliError;
 
     fn from_str(s: &str) -> Result<Self, Self::Err> {
         let mut parts = s.splitn(2, '=');
         match (parts.next(), parts.next()) {
-            (Some(""), Some(_)) => Err("Empty parameter key".to_string()),
-            (Some("api_key"), Some(_)) => Err(
-                "Use --api-key or SERPAPI_KEY instead of passing api_key as a parameter"
+            (Some(""), Some(_)) => Err(crate::error::CliError::UsageError {
+                message: "Empty parameter key".to_string(),
+            }),
+            (Some("api_key"), Some(_)) => Err(crate::error::CliError::UsageError {
+                message: "Use --api-key or SERPAPI_KEY instead of passing api_key as a parameter"
                     .to_string(),
-            ),
+            }),
             (Some(key), Some(value)) => Ok(Self {
                 key: key.to_string(),
                 value: value.to_string(),
             }),
-            (Some(""), None) | (None, _) => Err("Empty parameter".to_string()),
+            (Some(""), None) | (None, _) => Err(crate::error::CliError::UsageError {
+                message: "Empty parameter".to_string(),
+            }),
             (Some(value), None) => Ok(Self {
                 key: "q".to_string(),
                 value: value.to_string(),

tests/e2e.rs

@@ -167,7 +167,7 @@ fn test_no_args() {
 }
 
 #[test]
-#[ignore = "requires live SERPAPI_KEY env var"]
+#[ignore = "requires live SERPAPI_KEY env var and network access"]
 fn test_login_flow() {
     let api_key = env::var("SERPAPI_KEY").expect("SERPAPI_KEY not set");
     let config_dir = serpapi_cli::config::config_dir();

tests/integration_tests.rs

@@ -262,17 +262,13 @@ mod error_tests {
 
     #[test]
     fn test_param_parse_error_produces_usage_error() {
-        // "=value" has an empty key; parsing must fail
+        // "=value" has an empty key; parsing must fail with a CliError directly
         let result: Result<Param, _> = "=value".parse();
         assert!(result.is_err());
-        // The error message should be wrappable into a UsageError as the fix does
-        let err_msg = result.unwrap_err().to_string();
-        let usage_err = CliError::UsageError {
-            message: err_msg.clone(),
-        };
-        let display = format!("{}", usage_err);
+        let err = result.unwrap_err();
+        let display = format!("{}", err);
         assert!(display.contains("Usage error"));
-        assert!(display.contains(&err_msg));
+        assert!(display.contains("Empty parameter key"));
     }
 
     #[test]