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); } }