From 658d9b5a687db7f1fc8de60cf07680eebe5d7f7c Mon Sep 17 00:00:00 2001 From: ANonABento Date: Sat, 22 Aug 2026 23:40:23 -0400 Subject: [PATCH] fix(pipeline): say what actually failed, and guard against dead modules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things, the second found while fixing the first. Trigger failures now carry their reason to the card. Every failing card read "Execution failed" while the real reason existed only in a log — `git push --force failed: fatal: 'origin' does not appear to be a git repository`, for instance. `create_pr`, `run_script` and the managed-turn completion all called `mark_complete` with no detail, so the generic fallback was what the user saw. Each now passes one: - create_pr keeps the error it was previously only logging and emitting as an ephemeral Tauri event (which helps only if a panel is mounted). - run_script names the step: "Step 'lint' failed: exited 1", rather than leaving you to guess which of six steps it was. - the managed turn reports the CLI and exit code. `failure_message()` applies the generic string only when there genuinely is no reason, and treats blank or whitespace-only detail as absent — otherwise a card could store an empty error that renders as no error at all. ~1,165 lines of pipeline Rust are never compiled. While fixing the above I edited `pipeline/completion.rs`, added a test, and watched it not run. `completion.rs`, `engine.rs`, `events.rs`, `exit.rs` and `test_utils.rs` have no `mod` declaration anywhere: commit 234a992 "Split pipeline/mod.rs" created them and never wired them up, so the split silently never took effect. `mod.rs` kept the live implementation, and duplicate copies of `decide_completion` and `mark_complete_with_error` have sat beside it ever since. The trap is that it all reads like production code. `cargo build` passes, `clippy` passes, the file looks live, and editing it changes nothing at runtime. The real fix went into `pipeline/mod.rs`. `scripts/check-rust-modules.js` now fails CI on any `.rs` file no `mod` declaration reaches, with the five known ones in a documented `KNOWN_DEAD` allowlist so new files are still guarded. It also fails if that list names a file that no longer exists, so it shrinks rather than rots. Directories holding no Rust (`db/migrations/`, which is .sql read at runtime) are skipped. Deleting the five or finishing the split is a call for whoever owns that refactor — declaring them as-is will not build, since the symbols are duplicated. Filed on the roadmap. --- .github/workflows/ci.yml | 4 +- .github/workflows/release.yml | 2 + .tickets/_docs/ROADMAP.md | 30 +++++--- package.json | 1 + scripts/check-rust-modules.js | 119 +++++++++++++++++++++++++++++ src-tauri/src/pipeline/mod.rs | 37 ++++++++- src-tauri/src/pipeline/triggers.rs | 64 ++++++++++++++-- 7 files changed, 236 insertions(+), 21 deletions(-) create mode 100755 scripts/check-rust-modules.js diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2be8512e..a2a201ef 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -68,6 +68,8 @@ jobs: run: pnpm run test:ipc - name: Type scale check run: pnpm run test:type-scale + - name: Rust module check + run: pnpm run test:rust-modules - name: Frontend tests run: pnpm exec vitest run @@ -100,7 +102,7 @@ jobs: '', 'Checks run:', '- Rust: `cargo check --workspace`, `cargo clippy --workspace --all-targets -- -D warnings`, `cargo test --workspace`', - '- Frontend: `pnpm run type-check`, `pnpm run lint`, `pnpm run test:ipc`, `pnpm run test:type-scale`, `pnpm audit --audit-level moderate`, `pnpm exec vitest run`', + '- Frontend: `pnpm run type-check`, `pnpm run lint`, `pnpm run test:ipc`, `pnpm run test:type-scale`, `pnpm run test:rust-modules`, `pnpm audit --audit-level moderate`, `pnpm exec vitest run`', '', 'Please inspect the failed workflow run and fix main.' ].join('\n'); diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index c513cf4b..0195501b 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -57,6 +57,8 @@ jobs: run: pnpm run test:ipc - name: Type scale check run: pnpm run test:type-scale + - name: Rust module check + run: pnpm run test:rust-modules - name: Dependency audit run: pnpm audit --audit-level moderate diff --git a/.tickets/_docs/ROADMAP.md b/.tickets/_docs/ROADMAP.md index 79bf9678..6ad42a33 100644 --- a/.tickets/_docs/ROADMAP.md +++ b/.tickets/_docs/ROADMAP.md @@ -35,6 +35,13 @@ replaces the terminal. with a 60s budget and **never** hard-kills — exhausting it injects anyway and leaves the pane inspectable. `CLI_HEALTH_SPECS` now probes the interactive codex path so the same drift can't ship silently again. +- Trigger failures now say what went wrong (2026-08-22). Every failing card read + "Execution failed" while the real reason — `git push --force failed: 'origin' + does not appear to be a git repository` — existed only in a log. `create_pr`, + `run_script` and the managed-turn completion all called `mark_complete` with + no detail; they now pass one, and `failure_message()` falls back to the generic + string only when there genuinely isn't a reason (blank details included, so a + card can't end up with an empty error that renders as none). - `merge_to_main` pushed the wrong thing (2026-08-22). It ran `git push origin HEAD:refs/heads/main` from `resolve_working_dir`, which is the **shared workspace checkout** whenever the task has no worktree — and terminal @@ -109,17 +116,18 @@ Empty. The last item — the stale `/usr/local/bin/claude` — was removed ## 🟡 Important (rough edges, not blockers) -2b. **Trigger failures surface as "Execution failed" on the card.** The real - reason exists — `[create_pr] Failed …: git push --force failed: fatal: - 'origin' does not appear to be a git repository` — but only in the log. - `handle_trigger_failure` propagates a specific message and does show it - (e.g. "Cannot create PR: task has no branch_name"), while the paths going - through `mark_complete_with_error` with `error_detail: None` fall back to the - generic string. Thread the detail through so the card says what actually - happened. Found 2026-08-22 while testing `auto_setup` / `create_pr`, both of - which are otherwise **correct** — the first paths this sweep found that - behave properly. - +2b. **~1,165 lines of Rust in `pipeline/` are never compiled.** Commit 234a992 + "Split pipeline/mod.rs" created `completion.rs`, `engine.rs`, `events.rs`, + `exit.rs` and `test_utils.rs` but never added the `mod` declarations — so the + split silently never took effect. `mod.rs` kept the live implementation and + duplicate copies of `decide_completion` / `mark_complete_with_error` have sat + beside it since, compiling never and tested never. The trap is that it all + *reads* like production code: `cargo build` and `clippy` pass, and editing it + changes nothing at runtime (someone did exactly that, 2026-08-22). + `scripts/check-rust-modules.js` now guards every **new** file and carries + these five in a documented `KNOWN_DEAD` allowlist. + **Decision needed:** delete them, or finish the split. Declaring them as-is + will not build — duplicate symbols. 2. **Other inert settings surfaces.** Flagged while wiring Appearance, not fixed: custom keyboard shortcuts render but do nothing (`shortcuts-tab.tsx:54`), and the OpenRouter / Google / Ollama provider cards are "Coming soon". Framer diff --git a/package.json b/package.json index b31d245e..f8046bae 100644 --- a/package.json +++ b/package.json @@ -17,6 +17,7 @@ "lint": "eslint src/", "test:ipc": "node scripts/check-ipc-registration.js", "test:type-scale": "node scripts/check-type-scale.js", + "test:rust-modules": "node scripts/check-rust-modules.js", "format": "prettier --write 'src/**/*.{ts,tsx,css}'", "test": "vitest", "test:run": "vitest run", diff --git a/scripts/check-rust-modules.js b/scripts/check-rust-modules.js new file mode 100755 index 00000000..93344b7d --- /dev/null +++ b/scripts/check-rust-modules.js @@ -0,0 +1,119 @@ +#!/usr/bin/env node + +/** + * Fails when a `.rs` file sits in a module directory but no `mod` declaration + * pulls it in — i.e. it is never compiled. + * + * This is not hypothetical. Commit 234a992 "Split pipeline/mod.rs" created + * `completion.rs`, `engine.rs`, `events.rs` and `exit.rs`, but never added the + * `mod` lines, so the split silently never took effect: `mod.rs` kept the live + * implementation and ~1.2k lines of duplicate `decide_completion` / + * `mark_complete_with_error` sat beside it, compiling never, tested never. + * + * The failure mode is nasty because everything looks fine: `cargo build` + * passes, `cargo clippy` passes, the file reads like production code, and + * editing it changes nothing at runtime. Someone did exactly that before this + * check existed. + */ + +import fs from 'node:fs' +import path from 'node:path' + +const root = process.cwd() +const crates = ['src-tauri/src', 'mcp-server/src'] + +/** Declarations in a parent file: `mod foo;`, `pub mod foo;`, `pub(crate) mod foo;`. */ +function declaredMods(source) { + const found = new Set() + for (const m of source.matchAll(/^\s*(?:pub(?:\s*\([^)]*\))?\s+)?mod\s+([A-Za-z_][A-Za-z0-9_]*)\s*;/gm)) { + found.add(m[1]) + } + return found +} + +/** + * Known-dead files, left in place pending a decision. + * + * Commit 234a992 "Split pipeline/mod.rs" created these and never declared them. + * Declaring them now would not build — they duplicate symbols that still live + * in `pipeline/mod.rs` (`decide_completion`, `mark_complete_with_error`, ...), + * so the choice is *delete them* or *finish the split*, and that belongs to + * whoever owns the refactor. Listed here so the check still guards every new + * file instead of being switched off entirely. + * + * Shrink this list; never grow it. + */ +const KNOWN_DEAD = new Set([ + 'src-tauri/src/pipeline/completion.rs', + 'src-tauri/src/pipeline/engine.rs', + 'src-tauri/src/pipeline/events.rs', + 'src-tauri/src/pipeline/exit.rs', + 'src-tauri/src/pipeline/test_utils.rs', +]) + +const offenders = [] + +function checkDir(dir, parentFile) { + if (!fs.existsSync(parentFile)) return + const declared = declaredMods(fs.readFileSync(parentFile, 'utf8')) + + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name) + if (entry.isDirectory()) { + // A subdirectory only matters if it actually holds Rust. `db/migrations/` + // is .sql read at runtime, not compiled. + const holdsRust = fs + .readdirSync(full, { withFileTypes: true }) + .some((e) => (e.isFile() && e.name.endsWith('.rs')) || e.isDirectory()) + if (!holdsRust) continue + + // A subdirectory is a module only if the parent declares it; its own + // mod.rs then governs the files inside. + const name = entry.name + if (!declared.has(name)) { + offenders.push(`${path.relative(root, full)}/ (directory not declared in ${path.relative(root, parentFile)})`) + continue + } + checkDir(full, path.join(full, 'mod.rs')) + continue + } + if (!entry.name.endsWith('.rs')) continue + const stem = entry.name.slice(0, -3) + if (stem === 'mod' || stem === 'main' || stem === 'lib') continue + const rel = path.relative(root, full) + if (!declared.has(stem) && !KNOWN_DEAD.has(rel)) { + const lines = fs.readFileSync(full, 'utf8').split('\n').length + offenders.push(`${rel} (${lines} lines, never compiled)`) + } + } +} + +for (const crate of crates) { + const dir = path.join(root, crate) + if (!fs.existsSync(dir)) continue + // Crate root declares the top-level modules. + const rootFile = ['lib.rs', 'main.rs'] + .map((f) => path.join(dir, f)) + .find((f) => fs.existsSync(f)) + if (rootFile) checkDir(dir, rootFile) +} + +if (offenders.length > 0) { + console.error('Rust files that no `mod` declaration pulls in — these are never compiled:') + for (const o of offenders) console.error(` - ${o}`) + console.error('\nAdd the missing `mod ;`, or delete the file. A file that') + console.error('compiles never and tests never is worse than no file: it reads like') + console.error('production code and editing it changes nothing.') + process.exit(1) +} + +const stale = [...KNOWN_DEAD].filter((f) => !fs.existsSync(path.join(root, f))) +if (stale.length > 0) { + console.error('KNOWN_DEAD lists files that no longer exist — remove them from the list:') + for (const f of stale) console.error(` - ${f}`) + process.exit(1) +} + +console.log( + `Rust module check passed (every new .rs file is reachable; ${String(KNOWN_DEAD.size)} known-dead file(s) pending a decision).`, +) diff --git a/src-tauri/src/pipeline/mod.rs b/src-tauri/src/pipeline/mod.rs index 16460bd1..b80f1110 100644 --- a/src-tauri/src/pipeline/mod.rs +++ b/src-tauri/src/pipeline/mod.rs @@ -319,6 +319,23 @@ pub enum CompletionAction { Failed, } +/// The message stored on the task when a trigger fails. +/// +/// Callers that know *why* should say so. "Execution failed" tells the user +/// nothing they can act on, and until the detail was threaded through it was +/// what every failing card showed while the real reason — `git push --force +/// failed: 'origin' does not appear to be a git repository`, say — sat only in +/// a log nobody knew to open. +/// +/// Blank and whitespace-only details fall back too, so a card can never end up +/// with an empty error that renders as no error at all. +pub(crate) fn failure_message(error_detail: Option<&str>) -> &str { + match error_detail { + Some(detail) if !detail.trim().is_empty() => detail, + _ => "Execution failed", + } +} + /// Pure decision: given task state and column triggers, what should happen on completion? pub fn decide_completion( task: &Task, @@ -1168,7 +1185,7 @@ pub fn mark_complete_with_error( // Rebase-fail → ConflictResolver. if let Some(target_col_id) = get_on_failure_move_target(column.triggers.as_deref()) { if let Ok(target_col) = db::get_column(conn, &target_col_id) { - let error_msg = error_detail.unwrap_or("Execution failed"); + let error_msg = failure_message(error_detail); log::info!( "[pipeline] task {} failed in column '{}' — routing to '{}' via on_failure", task_id, @@ -1229,7 +1246,7 @@ pub fn mark_complete_with_error( } } - let error_msg = error_detail.unwrap_or("Execution failed"); + let error_msg = failure_message(error_detail); let updated_task = db::update_task_pipeline_state( conn, task_id, @@ -1385,6 +1402,22 @@ pub(crate) fn promote_queued_tasks(app: &AppHandle, workspace_id: &str) { #[cfg(test)] mod tests { + + #[test] + fn failure_message_prefers_the_real_reason() { + // Every failing card used to read "Execution failed" while the actual + // reason sat in a log. Callers now pass the detail; this only fills in + // when there genuinely isn't one. + assert_eq!( + failure_message(Some("git push --force failed: no such remote")), + "git push --force failed: no such remote" + ); + assert_eq!(failure_message(None), "Execution failed"); + // Never store a blank error — it renders as no error at all. + assert_eq!(failure_message(Some("")), "Execution failed"); + assert_eq!(failure_message(Some(" ")), "Execution failed"); + } + use super::*; /// Create a minimal task for testing decision logic. diff --git a/src-tauri/src/pipeline/triggers.rs b/src-tauri/src/pipeline/triggers.rs index 8cca484a..e4e62d80 100644 --- a/src-tauri/src/pipeline/triggers.rs +++ b/src-tauri/src/pipeline/triggers.rs @@ -2271,6 +2271,16 @@ fn start_managed_trigger_turn( .await; let turn_succeeded = matches!(result, Ok(ref turn) if turn.exit_code == Some(0)); let should_replay_queue = turn_succeeded; + // Why it failed, for the card. Without this the user sees only + // "Execution failed" for a run that has a perfectly specific reason. + let turn_failure_detail: Option = match &result { + Ok(turn) if turn.exit_code == Some(0) => None, + Ok(turn) => Some(match turn.exit_code { + Some(code) => format!("{} exited {}", cli_type, code), + None => format!("{} was terminated before it exited", cli_type), + }), + Err(e) => Some(format!("{} failed to run: {:?}", cli_type, e)), + }; if let Ok(conn) = rusqlite::Connection::open(db::db_path()) { let _ = conn.execute_batch("PRAGMA journal_mode=WAL;"); let (session_status, task_status, exit_code) = match result { @@ -2362,9 +2372,13 @@ fn start_managed_trigger_turn( .map(|t| trigger_column_id.as_deref() == Some(t.column_id.as_str())) .unwrap_or(false); if still_here { - if let Err(e) = - super::mark_complete(&conn, &app_for_turn, &task_id, turn_succeeded) - { + if let Err(e) = super::mark_complete_with_error( + &conn, + &app_for_turn, + &task_id, + turn_succeeded, + turn_failure_detail.as_deref(), + ) { log::warn!( "[triggers] managed turn completion failed for task {}: {}", task_id, @@ -3273,6 +3287,9 @@ fn execute_create_pr( } }; + // Carries the reason from whichever branch failed, so the card can say + // what happened instead of a bare "Execution failed". + let mut failure_detail: Option = None; let success = match result { Ok(Ok((pr_number, pr_url))) => { log::info!( @@ -3288,6 +3305,10 @@ fn execute_create_pr( } Ok(Err(e)) => { log::error!("[create_pr] Failed for task {}: {}", task_id, e); + // Kept so it can reach the card. Emitting it as a Tauri event + // (below) only helps if a panel happens to be mounted, and the + // log only helps someone who knows to go looking. + failure_detail = Some(e.clone()); // If the rebase reported conflicts, flag the task for manual // review instead of letting the trigger silently retry — we @@ -3328,13 +3349,20 @@ fn execute_create_pr( } Err(e) => { log::error!("[create_pr] Join error for task {}: {}", task_id, e); + failure_detail = Some(format!("create_pr task did not finish: {}", e)); false } }; // Mark complete so pipeline can advance (also emits tasks:changed) if let Some(conn) = conn { - if let Err(e) = super::mark_complete(&conn, &app_handle, &task_id, success) { + if let Err(e) = super::mark_complete_with_error( + &conn, + &app_handle, + &task_id, + success, + failure_detail.as_deref(), + ) { log::error!("[create_pr] mark_complete failed: {}", e); } } @@ -3483,6 +3511,10 @@ fn execute_run_script( // Execute steps in a background task (all data is owned) tokio::spawn(async move { let mut success = true; + // Why it failed, so the card can say more than "Execution failed". + // Deliberately names the step — "Check failed" alone doesn't tell you + // which of six steps it was. + let mut failure_detail: Option = None; let total = resolved_steps.len(); for (i, step) in resolved_steps.iter().enumerate() { @@ -3540,11 +3572,21 @@ fn execute_run_script( ); } if !out.status.success() { - if *is_check { + let msg = if *is_check { let msg = fail_message.as_deref().unwrap_or("Check failed"); log::warn!("[script:{}] Check failed: {}", task_id, msg); - } + msg.to_string() + } else { + let code = out + .status + .code() + .map(|c| c.to_string()) + .unwrap_or_else(|| "signal".to_string()); + format!("exited {}", code) + }; if !continue_on_error { + failure_detail = + Some(format!("Step '{}' failed: {}", name, msg)); success = false; break; } @@ -3558,6 +3600,8 @@ fn execute_run_script( e ); if !continue_on_error { + failure_detail = + Some(format!("Step '{}' could not run: {}", name, e)); success = false; break; } @@ -3621,7 +3665,13 @@ fn execute_run_script( e ); } - if let Err(e) = super::mark_complete(&conn, &app_handle, &task_id, success) { + if let Err(e) = super::mark_complete_with_error( + &conn, + &app_handle, + &task_id, + success, + failure_detail.as_deref(), + ) { log::error!("[script:{}] mark_complete failed: {}", task_id, e); } }