cleanup(engine): make process_command stdout-clean (drop REPL-only prints) (#76)

The engine's `process_command` had three direct stdout writes that
made sense in a REPL but corrupted any other channel an embedder
might be using stdout for:

  src/sql/mod.rs:150 — let _ = table.print_table_schema();   (CREATE)
  src/sql/mod.rs:208 — db_table.print_table_data();          (INSERT)
  src/sql/mod.rs:224 — print!("{rendered}");                 (SELECT)

Plus a leftover debug print in src/sql/parser/create.rs:190 dumping
unhandled `sqlparser::ast::TableConstraint` debug reprs to stdout.

This is the issue PR #73 papered over with the dup2(2,1) dance in
sqlrite-mcp/src/stdio_redirect.rs. That fix is still in place
(belt-and-suspenders against future regressions), but the engine now
behaves correctly without it — the SDKs (Python, Node, Go, WASM,
FFI), the Tauri desktop, and any future embedder all benefit.

## How

Refactored process_command to return a richer struct, with a
backwards-compat wrapper:

  pub struct CommandOutput {
      pub status:   String,           // "INSERT Statement executed.", etc.
      pub rendered: Option<String>,   // SELECT prettytable; None otherwise
  }

  pub fn process_command(query, db) -> Result<String>
  // backwards-compat: returns just .status

  pub fn process_command_with_render(query, db) -> Result<CommandOutput>
  // new: rich return, never writes to stdout

The REPL (`src/main.rs`) calls `_with_render` and prints rendered
above status — same UX as before. The .ask meta-command does the
same and concatenates the two pieces into the single String it
bubbles up to the REPL's outer dispatch loop.

The CREATE-TABLE schema dump and INSERT row dump aren't preserved —
they were small UX niceties that interactive REPL users rarely need
(the schema you just CREATEd, the row you just INSERTed). The INSERT
case was actively bad UX on any non-trivial table — it dumped the
entire table after every insert. Both gone.

`sqlrite::process_command` keeps its existing signature for
backwards compatibility — every existing call site (Connection::execute,
the SDK FFI shims, the .ask handler's inline runner, the engine's
own tests) keeps working unchanged.

## Verified

- `cargo build --workspace` clean
- `cargo test --workspace --exclude sqlrite-{python,nodejs,desktop}` —
  330+ tests across engine + sqlrite-mcp + sqlrite-ask + sqlrite-ffi,
  all passing (engine's 251-test integration suite covers the
  process_command paths most directly affected)
- REPL smoke: CREATE / INSERT / SELECT round-trip prints rendered
  table above status, no surprises
- MCP smoke: same round-trip via JSON-RPC over stdio — `cat 2>/dev/null`
  filtered stderr is now empty (was prettytable noise before;
  stdio_redirect was the only thing keeping it off the protocol channel)

## Docs

- `docs/architecture.md` — updates the engine module map entries for
  src/lib.rs and src/sql/mod.rs to mention the new types + the
  "never writes to stdout" guarantee
- `docs/sql-engine.md` — entry-point section now shows both functions
  + the CommandOutput shape; explains the rendered-vs-status split
- `docs/mcp.md` — security-notes section reframes stdio_redirect as
  defense-in-depth now that the engine is clean
- `sqlrite-mcp/src/stdio_redirect.rs` — module-level comment rewritten
  to call out the engine fix + the three concrete future-regression
  scenarios the redirect protects against

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: joaoh82

docs/architecture.md

@@ -89,13 +89,13 @@ The engine never depends on the SDK crates; the SDK crates each depend on the en
 | Module | What it owns |
 |---|---|
 | [`src/main.rs`](../src/main.rs) | Binary entry: init env_logger, build rustyline editor, run the REPL loop, route input to either the meta or SQL dispatcher |
-| [`src/lib.rs`](../src/lib.rs) | Library entry: re-exports `Connection`, `Statement`, `Rows`, `Value`, `Database`, `process_command`, the `ask` module (when feature on), etc. — the stable public surface every SDK binds against |
+| [`src/lib.rs`](../src/lib.rs) | Library entry: re-exports `Connection`, `Statement`, `Rows`, `Value`, `Database`, `process_command` / `process_command_with_render` / `CommandOutput`, the `ask` module (when feature on), etc. — the stable public surface every SDK binds against |
 | [`src/connection.rs`](../src/connection.rs) | `Connection` / `Statement` / `Rows` / `Row` / `OwnedRow` / `FromValue` — the Phase 5a public API |
 | [`src/ask/`](../src/ask/) | Engine integration with `sqlrite-ask`: `ConnectionAskExt`, `ask_with_database`, the `schema::dump_schema_for_database` helper. The `schema` submodule is always available; the rest is gated behind the `ask` feature. Phase 7g.2. |
 | [`src/repl/`](../src/repl/) | `REPLHelper` (implements rustyline's `Helper` trait: completer, hinter, highlighter, validator). Also `get_config` and `get_command_type` |
 | [`src/meta_command/`](../src/meta_command/) | `MetaCommand` enum, parsing (`.open FOO.db` → `Open(PathBuf)`, `.ask <Q>` → `Ask(String)`), and dispatch to persistence + ask helpers |
 | [`src/error.rs`](../src/error.rs) | `SQLRiteError` (thiserror-derived), `Result<T>` alias, hand-rolled `PartialEq` that handles `io::Error` |
-| [`src/sql/mod.rs`](../src/sql/mod.rs) | `SQLCommand` classifier, `process_command` — the top-level entry that parses a SQL string and routes to the right executor. Also triggers auto-save. |
+| [`src/sql/mod.rs`](../src/sql/mod.rs) | `SQLCommand` classifier, `process_command` / `process_command_with_render` — the top-level entries that parse a SQL string and route to the right executor. Also triggers auto-save. **Never writes to stdout** — for SELECT statements, the rendered prettytable comes back inside `CommandOutput.rendered` so the REPL can print it (the engine itself doesn't); the SDK / FFI / MCP callers ignore it. |
 | [`src/sql/parser/`](../src/sql/parser/) | Takes a `sqlparser::ast::Statement` and produces internal query structs (`CreateQuery`, `InsertQuery`, `SelectQuery`) with only the fields we actually use |
 | [`src/sql/executor.rs`](../src/sql/executor.rs) | `execute_select`, `execute_delete`, `execute_update`, plus the shared expression evaluator `eval_expr` / `eval_predicate`. Also the bounded-heap top-k optimization (Phase 7c) and the HNSW probe shortcut (Phase 7d.2). |
 | [`src/sql/db/database.rs`](../src/sql/db/database.rs) | `Database`: table map + optional `source_path` + optional long-lived `Pager` + transaction-snapshot state |

docs/mcp.md

@@ -309,7 +309,7 @@ Stderr is reserved for diagnostics (panics, the MCP startup banner). Anything wr
 
 - **Logging hygiene:** the `query`, `execute`, and `ask` tools receive arbitrary user-supplied SQL and questions. The server emits these to stderr only on errors (so the MCP log pane is useful for debugging) — but if you wrap `sqlrite-mcp`'s stderr in your own logging stack, treat the contents as potentially sensitive.
 
-- **Stdout is owned by the protocol.** The binary redirects process fd 1 to fd 2 at startup so any errant `print!` from anywhere in the dep tree goes to stderr instead of corrupting the JSON-RPC channel. See `sqlrite-mcp/src/stdio_redirect.rs` for the dance.
+- **Stdout is owned by the protocol.** As of the engine-stdout-pollution cleanup, the SQLRite engine itself doesn't print to stdout — the REPL-convenience prints (CREATE schema, INSERT row dump, SELECT result table) all moved out: SELECT's rendered table comes back inside [`CommandOutput::rendered`](../src/sql/mod.rs) for the REPL to print itself, the others were dropped entirely. The MCP binary additionally redirects process fd 1 → fd 2 at startup as **defense in depth** — protects against future regressions if a contributor (or a transitive dep) ever reintroduces a stray `print!`. See `sqlrite-mcp/src/stdio_redirect.rs` for the dance.
 
 ---
 

docs/sql-engine.md

@@ -8,9 +8,21 @@ Every SQL statement goes through [`process_command`](../src/sql/mod.rs):
 
 ```rust
 pub fn process_command(query: &str, db: &mut Database) -> Result<String>
+
+// Richer variant — returns both the status line and (for SELECT) the
+// pre-rendered prettytable. The REPL uses this so it can print the
+// table above the status; SDK / FFI / MCP callers ignore the rendered
+// field and use the typed-row API for actual row access.
+pub fn process_command_with_render(query: &str, db: &mut Database)
+    -> Result<CommandOutput>;
+
+pub struct CommandOutput {
+    pub status: String,
+    pub rendered: Option<String>,   // Some(_) only for SELECT
+}
 ```
 
-Given a raw line of SQL (e.g. `"SELECT name FROM users WHERE age > 25;"`) and the in-memory database, it returns either a human-readable status message or a typed error.
+Given a raw line of SQL (e.g. `"SELECT name FROM users WHERE age > 25;"`) and the in-memory database, both functions return either a human-readable status (and, for SELECT, a rendered prettytable) or a typed error. **Neither writes to stdout** — the REPL is the only consumer that prints anything; everyone else (Tauri desktop, SDKs, the MCP server) just reads the returned struct.
 
 The function's shape:
 
@@ -19,7 +31,7 @@ The function's shape:
 3. Classify as read-only vs. writing.
 4. Match on `Statement::*` and dispatch to the appropriate executor.
 5. If the statement writes and the DB is file-backed, auto-save.
-6. Return the status string.
+6. Return `CommandOutput { status, rendered }`. (`process_command` is a backwards-compat wrapper that just returns `.status`.)
 
 ## Parsing
 

sqlrite-mcp/src/stdio_redirect.rs

@@ -8,11 +8,25 @@
 //! in by accident, a banner from a third-party crate. The MCP client
 //! sees garbage where it expected JSON and disconnects.
 //!
-//! The SQLRite engine's REPL-convenience prints inside
-//! `process_command` (the `CREATE TABLE` schema dump at sql/mod.rs:150,
-//! the `INSERT` row dump at sql/mod.rs:208, the `SELECT` result table
-//! at sql/mod.rs:224) are exactly this kind of write. They're great
-//! for an interactive REPL; lethal for an MCP server.
+//! ## Engine-side fix + this defense-in-depth backstop
+//!
+//! As of the engine-stdout-pollution cleanup, the SQLRite engine's
+//! `process_command` no longer writes to stdout. The historical REPL-
+//! convenience prints (CREATE TABLE schema dump, INSERT row dump,
+//! SELECT result table) all moved out: the rendered SELECT table now
+//! comes back inside [`sqlrite::CommandOutput::rendered`] for the REPL
+//! to print itself, and the CREATE / INSERT prints were dropped
+//! entirely (the latter was a spammy bug-feature anyway). So in normal
+//! operation this redirect catches nothing.
+//!
+//! We keep the redirect anyway as **defense in depth.** Three concrete
+//! futures it protects against: (a) a future engine PR accidentally
+//! reintroducing a `println!` that the test suite doesn't catch
+//! (engine tests don't assert on stdout); (b) a transitive dep adding
+//! a startup banner; (c) a debug print left in mid-development. Any
+//! one of those would silently break the MCP server without this
+//! safety net. Cost: ~140 LOC + a libc dep that's already a
+//! transitive of clap.
 //!
 //! ## What this module does
 //!

src/lib.rs

@@ -88,7 +88,7 @@ pub use sql::pager::{
     AccessMode, MASTER_TABLE_NAME, open_database, open_database_read_only, open_database_with_mode,
     save_database,
 };
-pub use sql::process_command;
+pub use sql::{CommandOutput, process_command, process_command_with_render};
 
 // Re-export sqlparser so downstream crates (the Tauri desktop app, the
 // eventual WASM bindings) can reach into the AST without pulling a

src/main.rs

@@ -10,7 +10,7 @@ use std::path::PathBuf;
 
 use meta_command::handle_meta_command;
 use repl::{CommandType, REPLHelper, get_command_type, get_config};
-use sqlrite::{Database, process_command};
+use sqlrite::{Database, process_command_with_render};
 
 use rustyline::Editor;
 use rustyline::error::ReadlineError;
@@ -144,10 +144,21 @@ fn main() -> rustyline::Result<()> {
                 // Parsing user's input and returning and enum of repl::CommandType
                 match get_command_type(command.trim()) {
                     CommandType::SQLCommand(_cmd) => {
-                        // process_command takes care of tokenizing, parsing and executing
-                        // the SQL Statement and returning a Result<String, SQLRiteError>
-                        match process_command(&command, &mut db) {
-                            Ok(response) => println!("{response}"),
+                        // `process_command_with_render` returns a CommandOutput
+                        // carrying both the status line and (for SELECT) the
+                        // pre-rendered prettytable. Print rendered first if
+                        // present so the user sees the rows above the
+                        // confirmation. Prior to the engine-stdout-pollution
+                        // cleanup the engine printed the table itself, which
+                        // corrupted any non-REPL stdout channel — now the REPL
+                        // owns the printing.
+                        match process_command_with_render(&command, &mut db) {
+                            Ok(output) => {
+                                if let Some(rendered) = output.rendered.as_deref() {
+                                    print!("{rendered}");
+                                }
+                                println!("{}", output.status);
+                            }
                             Err(err) => eprintln!("An error occured: {err}"),
                         }
                     }

src/meta_command/mod.rs

@@ -8,7 +8,7 @@ use crate::repl::REPLHelper;
 use sqlrite::error::{Result, SQLRiteError};
 use sqlrite::sql::db::database::Database;
 use sqlrite::sql::pager::{open_database, save_database};
-use sqlrite::{ask::ask_with_database, process_command};
+use sqlrite::{ask::ask_with_database, process_command_with_render};
 use sqlrite_ask::AskConfig;
 
 #[derive(Debug, PartialEq)]
@@ -251,11 +251,16 @@ fn handle_ask(
     }
 
     // Run the generated SQL through the same pipeline as a typed
-    // statement. process_command handles both DDL/DML (returns a
-    // status string like "INSERT Statement executed.") and SELECT
-    // (returns a rendered table). Either is fine to return up to the
-    // outer dispatch loop, which just prints what we hand back.
-    process_command(&resp.sql, db)
+    // statement. We use the `_with_render` variant so SELECTs come back
+    // with their rendered prettytable; concatenate it above the status
+    // line so the REPL's outer dispatch (which just prints whatever
+    // string we return) shows both. DDL/DML statements return only a
+    // status — `rendered` is `None` and we skip the prepend.
+    let output = process_command_with_render(&resp.sql, db)?;
+    Ok(match output.rendered {
+        Some(rendered) => format!("{rendered}{}", output.status),
+        None => output.status,
+    })
 }
 
 #[cfg(test)]

src/sql/mod.rs

@@ -41,10 +41,52 @@ impl SQLCommand {
     }
 }
 
-/// Performs initial parsing of SQL Statement using sqlparser-rs
+/// Output of running one SQL statement through the engine.
+///
+/// Two fields:
+///
+/// - `status` is the short human-readable confirmation line every caller
+///   wants ("INSERT Statement executed.", "3 rows updated.", "BEGIN", etc.).
+/// - `rendered` is the pre-formatted prettytable rendering of a SELECT's
+///   result rows. Populated only for `SELECT` statements; `None` for every
+///   other statement type. The REPL prints this above the status line so
+///   users see both the rows and the confirmation; SDK / FFI / MCP callers
+///   ignore it and reach for the typed-row APIs (`Connection::prepare` →
+///   `Statement::query` → `Rows`) when they want row data instead.
+///
+/// Splitting the two means [`process_command_with_render`] can return
+/// everything the REPL needs without writing to stdout itself —
+/// historically `process_command` would `print!()` the rendered table
+/// directly, which corrupted any non-REPL stdout channel (the MCP server's
+/// JSON-RPC wire, structured loggers piping engine output, …).
+#[derive(Debug, Clone)]
+pub struct CommandOutput {
+    pub status: String,
+    pub rendered: Option<String>,
+}
+
+/// Backwards-compatible wrapper around [`process_command_with_render`] that
+/// returns just the status string. Every existing call site (the public
+/// `Connection::execute`, the SDK FFI shims, the .ask meta-command's
+/// inline runner, the engine's own tests) keeps working unchanged.
+///
+/// Callers that want the rendered SELECT table (the REPL, future
+/// terminal-style consumers) should call [`process_command_with_render`]
+/// directly and inspect [`CommandOutput::rendered`].
 pub fn process_command(query: &str, db: &mut Database) -> Result<String> {
+    process_command_with_render(query, db).map(|o| o.status)
+}
+
+/// Performs initial parsing of SQL Statement using sqlparser-rs.
+///
+/// Returns a [`CommandOutput`] carrying both the status string and (for
+/// SELECT statements) the pre-rendered prettytable output. **Never writes
+/// to stdout.** The REPL is responsible for printing whatever it wants
+/// from the returned struct.
+pub fn process_command_with_render(query: &str, db: &mut Database) -> Result<CommandOutput> {
     let dialect = SQLiteDialect {};
     let message: String;
+    let mut rendered: Option<String> = None;
     let mut ast = Parser::parse_sql(&dialect, query).map_err(SQLRiteError::from)?;
 
     if ast.len() > 1 {
@@ -58,7 +100,10 @@ pub fn process_command(query: &str, db: &mut Database) -> Result<String> {
     // Return a benign status rather than panicking on `pop().unwrap()`. Callers
     // (REPL, Tauri app) treat this as a no-op with no disk write triggered.
     let Some(query) = ast.pop() else {
-        return Ok("No statement to execute.".to_string());
+        return Ok(CommandOutput {
+            status: "No statement to execute.".to_string(),
+            rendered: None,
+        });
     };
 
     // Transaction boundary statements are routed to Database-level
@@ -68,7 +113,10 @@ pub fn process_command(query: &str, db: &mut Database) -> Result<String> {
     match &query {
         Statement::StartTransaction { .. } => {
             db.begin_transaction()?;
-            return Ok(String::from("BEGIN"));
+            return Ok(CommandOutput {
+                status: String::from("BEGIN"),
+                rendered: None,
+            });
         }
         Statement::Commit { .. } => {
             if !db.in_transaction() {
@@ -94,11 +142,17 @@ pub fn process_command(query: &str, db: &mut Database) -> Result<String> {
                 }
             }
             db.commit_transaction()?;
-            return Ok(String::from("COMMIT"));
+            return Ok(CommandOutput {
+                status: String::from("COMMIT"),
+                rendered: None,
+            });
         }
         Statement::Rollback { .. } => {
             db.rollback_transaction()?;
-            return Ok(String::from("ROLLBACK"));
+            return Ok(CommandOutput {
+                status: String::from("ROLLBACK"),
+                rendered: None,
+            });
         }
         _ => {}
     }
@@ -147,12 +201,14 @@ pub fn process_command(query: &str, db: &mut Database) -> Result<String> {
                         }
                         false => {
                             let table = Table::new(payload);
-                            let _ = table.print_table_schema();
+                            // Note: we used to call `table.print_table_schema()` here
+                            // for REPL convenience. Removed because it wrote
+                            // directly to stdout, which corrupted any non-REPL
+                            // protocol channel (most painfully the MCP server's
+                            // JSON-RPC wire). The status line below is enough for
+                            // the REPL; users who want to inspect the schema can
+                            // run a follow-up describe / `.tables`-style command.
                             db.tables.insert(table_name.to_string(), table);
-                            // Iterate over everything.
-                            // for (table_name, _) in &db.tables {
-                            //     println!("{}" , table_name);
-                            // }
                             message = String::from("CREATE TABLE Statement executed.");
                         }
                     }
@@ -205,7 +261,12 @@ pub fn process_command(query: &str, db: &mut Database) -> Result<String> {
                                     ));
                                 }
                             }
-                            db_table.print_table_data();
+                            // Note: we used to call `db_table.print_table_data()`
+                            // here, which dumped the *entire* table to stdout
+                            // after every INSERT. Beyond corrupting non-REPL
+                            // stdout channels, that's actively bad UX on any
+                            // table with more than a few rows. Removed in the
+                            // engine-stdout-pollution cleanup.
                         }
                         false => {
                             return Err(SQLRiteError::Internal("Table doesn't exist".to_string()));
@@ -219,9 +280,13 @@ pub fn process_command(query: &str, db: &mut Database) -> Result<String> {
         }
         Statement::Query(_) => {
             let select_query = SelectQuery::new(&query)?;
-            let (rendered, rows) = executor::execute_select(select_query, db)?;
-            // Print the result table above the status message so the REPL shows both.
-            print!("{rendered}");
+            let (rendered_table, rows) = executor::execute_select(select_query, db)?;
+            // Stash the rendered prettytable in the output so the REPL
+            // (or any terminal-style consumer) can print it above the
+            // status line. SDK / FFI / MCP callers ignore this field.
+            // The previous implementation `print!("{rendered}")`-ed
+            // directly to stdout, which broke every non-REPL embedder.
+            rendered = Some(rendered_table);
             message = format!(
                 "SELECT Statement executed. {rows} row{s} returned.",
                 s = if rows == 1 { "" } else { "s" }
@@ -267,7 +332,10 @@ pub fn process_command(query: &str, db: &mut Database) -> Result<String> {
         pager::save_database(db, &path)?;
     }
 
-    Ok(message)
+    Ok(CommandOutput {
+        status: message,
+        rendered,
+    })
 }
 
 #[cfg(test)]

src/sql/parser/create.rs

@@ -184,11 +184,14 @@ impl CreateQuery {
                         is_unique,
                     });
                 }
-                // TODO: Handle constraints,
-                // Default value and others.
-                for constraint in constraints {
-                    println!("{constraint:?}");
-                }
+                // TODO: handle constraints + default values + check
+                // constraints + ON DELETE / ON UPDATE referential actions
+                // properly. They're currently parsed by `sqlparser` and
+                // dropped on the floor here. (Previously we `println!`-ed
+                // them to stdout as a debug aid — removed in the
+                // engine-stdout-pollution cleanup; flip to a `tracing`
+                // span if we ever want them visible in dev builds.)
+                let _ = constraints;
                 Ok(CreateQuery {
                     table_name: table_name.to_string(),
                     columns: parsed_columns,