From 6d7e2eade130d6eea3ab889746c45e802ffd1088 Mon Sep 17 00:00:00 2001 From: Romuald Priol Date: Fri, 10 Jul 2026 11:41:24 +0200 Subject: [PATCH] resolve 45 fixes Signed-off-by: Romuald Priol --- .github/workflows/ci-test.yml | 17 +- CHANGELOG.md | 18 ++ backend/Dockerfile | 3 + backend/scripts/disc-introspection-mcp.py | 5 + .../scripts/test_disc_introspection_mcp.py | 18 ++ backend/src/agents/runner.rs | 179 +++++++++++++++--- backend/src/agents/runner_test.rs | 34 +++- backend/src/api/audit/validation.rs | 1 + backend/src/api/contacts.rs | 2 +- backend/src/api/disc_git.rs | 26 ++- backend/src/api/disc_invite.rs | 85 ++++++++- backend/src/api/discover.rs | 15 +- backend/src/api/docs.rs | 8 +- backend/src/api/federation.rs | 55 ++++++ backend/src/api/git_ops.rs | 9 + backend/src/api/mcp_remote.rs | 10 +- backend/src/api/projects/clone.rs | 4 + backend/src/api/setup.rs | 90 +++++++-- backend/src/api/workflows.rs | 27 ++- backend/src/core/backup.rs | 147 ++++++++++++-- backend/src/core/config.rs | 47 +++-- backend/src/core/env.rs | 62 ++++-- backend/src/core/host_mcp_discovery.rs | 5 +- backend/src/core/key_discovery.rs | 98 +++++++++- backend/src/core/kronn_state.rs | 45 ++++- backend/src/core/kronn_state_test.rs | 33 ++++ backend/src/core/mcp_scanner.rs | 179 ++++++++++++++---- backend/src/core/mcp_scanner_test.rs | 169 +++++++++++++++++ backend/src/core/net_expose.rs | 20 +- backend/src/core/oauth2_cache.rs | 12 +- backend/src/core/rtk_detect.rs | 10 +- backend/src/core/run_notify.rs | 54 +++++- backend/src/core/scanner.rs | 5 + backend/src/core/scanner_test.rs | 2 + backend/src/core/tailscale.rs | 9 + backend/src/core/worktree.rs | 21 +- backend/src/core/ws_client.rs | 17 +- backend/src/db/learnings.rs | 19 +- backend/src/db/mod.rs | 86 +++++++-- backend/src/db/tests.rs | 45 ++++- backend/src/db/workflows.rs | 107 +++++++++-- backend/src/main.rs | 100 ++++++---- backend/src/workflows/batch_step.rs | 28 ++- backend/src/workflows/gate_checkpoint.rs | 41 ++++ backend/src/workflows/mod.rs | 52 ++++- backend/src/workflows/runner.rs | 173 ++++++++++++++--- backend/src/workflows/steps.rs | 42 +++- backend/src/workflows/template.rs | 173 ++++++++++++++--- backend/src/workflows/tracker/github.rs | 9 +- backend/src/workflows/trigger.rs | 142 ++++++++------ backend/src/workflows/workspace.rs | 81 +++++++- backend/tests/api_tests.rs | 18 +- docker-compose.yml | 1 + docs/inconsistencies-tech-debt.md | 2 +- .../TD-20260629-import-downgrade-wipe.md | 9 + ...D-20260709-nonisolated-overlapping-runs.md | 60 ++++++ frontend/e2e/README.md | 24 +++ frontend/e2e/ensure-browser.mjs | 14 ++ frontend/package.json | 2 +- frontend/scripts/check-types-drift.mjs | 145 +++++++++++--- frontend/src/components/ChatInput.tsx | 3 + .../src/components/settings/AgentsSection.tsx | 10 +- .../src/components/settings/DebugSection.tsx | 14 +- .../components/settings/IdentitySection.tsx | 8 +- .../components/settings/ProfilesSection.tsx | 7 +- .../settings/__tests__/DebugSection.test.tsx | 14 +- .../components/workflows/StepBranchMap.tsx | 27 ++- frontend/src/lib/i18n.ts | 15 ++ frontend/src/pages/McpPage.tsx | 4 + frontend/src/pages/SettingsPage.tsx | 45 +++-- frontend/src/pages/WorkflowsPage.css | 10 +- frontend/src/pages/WorkflowsPage.tsx | 58 +++++- .../src/pages/__tests__/SettingsPage.test.tsx | 19 ++ .../pages/__tests__/WorkflowsPage.test.tsx | 60 ++++++ lib/mcps.sh | 26 ++- 75 files changed, 2763 insertions(+), 471 deletions(-) create mode 100644 docs/tech-debt/TD-20260709-nonisolated-overlapping-runs.md create mode 100644 frontend/e2e/ensure-browser.mjs diff --git a/.github/workflows/ci-test.yml b/.github/workflows/ci-test.yml index ed18d6e0..7fae51aa 100644 --- a/.github/workflows/ci-test.yml +++ b/.github/workflows/ci-test.yml @@ -2,7 +2,9 @@ name: CI Tests on: pull_request: - types: [labeled, synchronize] + # `opened`/`reopened` exist for the require-ci-label guard below: a fresh + # PR without the label gets a visible RED check instead of zero runs. + types: [opened, reopened, labeled, synchronize] # Opt every JS-based action (checkout, setup-node, cache, pnpm/action-setup, # setup-python) into the Node 24 runtime ahead of GitHub's 2026-06-02 forced @@ -20,6 +22,19 @@ env: ONNXRUNTIME_NODE_INSTALL: skip jobs: + # Anti green-by-skip: every other job is gated on the `ci-test` label, so an + # unlabeled PR used to show all-skipped — which satisfies required status + # checks and reads as green. This job INVERTS that: it only runs (and fails) + # when the label is missing, making the untested state visibly red. + require-ci-label: + if: ${{ !contains(github.event.pull_request.labels.*.name, 'ci-test') }} + runs-on: ubuntu-latest + steps: + - name: Fail visibly when the ci-test label is missing + run: | + echo "::error::This PR has no 'ci-test' label — every CI job was SKIPPED (green-by-skip). Add the label to run the suite." + exit 1 + test-backend: if: contains(github.event.pull_request.labels.*.name, 'ci-test') runs-on: ubuntu-latest diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e6b61b7..22fa8831 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,24 @@ Versioning: [Semantic Versioning](https://semver.org/spec/v2.0.0.html). _« Socle clean » — the Phase-1 hardening batch from the 2026-07 full audit: security defaults, honest CI, operational reliability, and the two structural taxes (typegen, step dispatch) paid before the continual-learning work starts._ +### Fixed (hardening pass — post-merge adversarial review, 7 reviewers over the whole codebase) + +- **A panicking DB closure no longer kills the entire backend.** One panic inside a `with_conn` closure poisoned the single shared connection mutex — every later DB call in the process failed with "Mutex poisoned" until restart (observed live). `with_conn` now recovers a poisoned lock (a panicked closure can't leave SQLite mid-transaction) and catches panics at the source, surfacing the panic message in the API error. +- **Scheduled/manual DB backup no longer freezes the whole API.** The SQLite online-copy ran in 5-page steps with a 50ms pause per step *while holding the app's only DB connection* (~2.5s of global freeze per MB — at every boot and every 24h). Now a single-step copy. Also: the boot-tick backup is skipped when a recent backup already exists (a restart loop no longer wipes the retention window), malformed backup env vars warn instead of silently defaulting, and prune failures are logged. Locked by an end-to-end `perform_backup` test. +- **Cron triggers fire exactly once per occurrence.** The stateless "next occurrence within 30s" window overlapped itself: an occurrence landing on the seam fired on BOTH surrounding engine ticks (~1 in 31 → two concurrent runs of the same cron: double worktrees, double PR reviews), and a tick delayed by a slow tracker poll silently *skipped* the occurrence. The engine now evaluates the half-open window `(last-tick, now]`. +- **Cancel means cancelled.** `Cancelled` is now sticky at the SQL level: the runner's late terminal write can no longer flip a user-cancelled run back to `Failed`, the gate-resume window can no longer resurrect it to `Running`, the rollback chain checks the cancel token between compensation steps, and `Pending`/`WaitingApproval` runs (previously uncancellable) can be stopped. +- **A mid-stream Ollama failure now FAILS the step.** The stream-lifetime child exited 0 on every path, so a connection lost mid-generation — or Ollama's in-band `{"error": "model runner has unexpectedly stopped"}` object — produced a *successful* step with truncated or empty output feeding the next steps. The lifeline now reports the stream's real outcome (clean `done` = 0, error/truncation = 1 with the reason in the step error). +- **MCP config writes can never strip secrets again (2026-06-30 incident class).** All six agent-config writers (`.mcp.json`, Codex, Copilot, Gemini, Vibe, Kiro) now ABORT the whole file write when a configured env can't be decrypted — previously they silently wrote `env: {}` (or dropped the entry), clobbering the good secrets already on disk. Same closure applied to the neighbours: Codex `auth.json` is merged (a ChatGPT-OAuth login survives a key sync), a corrupt Gemini `settings.json` is left untouched instead of being replaced, `docs/.kronn.json` isn't rebuilt from default on a read error, the config-migration re-save goes through the atomic writer, and test-mode aborts (with rollback) if its restore state can't be persisted. +- **The LAN-exposure boot guard now checks auth that's actually ENFORCED.** A configured token with `auth_enabled: false` is a middleware no-op — yet it passed the guard (the Docker default: auth off + auto-generated token). The guard now requires enabled auth AND a token; setting `KRONN_AUTH_TOKEN` explicitly now also enables auth (as the guard's own error message always promised); and docker detection uses real container markers (`KRONN_IN_DOCKER` / `/.dockerenv`) instead of `KRONN_DATA_DIR`, which any native user can set (that false "docker" reading disabled the guard and the auth-on default). +- **Interrupted runs finally reach the failure webhook.** `should_notify` included `Interrupted` but the only notify site was the tail of the process that had just *died* — the flagship "cron died at 6am" alert never fired. The boot reconcile now reports what it flipped and the webhook fires at startup; gate-resumed and MCP-triggered runs notify too; `Interrupted` durations no longer poison the `next_check` scheduling average. +- **Engine-side concurrency check is atomic with the run insert** (a manual trigger racing a cron tick could put a `limit=1` workflow at 2 concurrent runs), and a batch step that misses the single `BatchRunFinished` broadcast (channel lag) re-reads the child run from the DB instead of timing out 2h and reporting a false failure. +- **Every unattended network/process path is bounded.** GitHub tracker client (a black-holed connection stalled the ENGINE TICK LOOP — all crons and trackers — until restart), workspace lifecycle hooks (a hung `npm ci` pinned the run and its concurrency slot forever, outside the cancel race), Kiro installer curl, repo-discovery pagination, docs-sidecar POST, P2P WebSocket handshake (a half-open peer permanently blocked reconnection), git push/clone (terminal-prompt + low-speed stall protection), and Ollama chat connect (a firewalled host waited the full 600s before "unreachable"). +- **Workflow engine correctness on empty/degenerate agent output:** a `NO_RESULTS` envelope now routes to the action the author declared (the fallback hardcoded `Stop`, hijacking `Goto` recovery branches); a present-but-non-string envelope `status` (`null`, a number) no longer coerces to `"OK"` (failure direction inverted); the Gemini MCP-noise strip is applied even when token count is 0; agent stdout read errors are logged instead of masquerading as EOF. +- **macOS: the "branch already checked out in the main repo" guard actually fires.** git prints canonical paths (`/private/var/…`) while Kronn holds the user's spelling — the equality check missed, and instead of blocking, the code handed the MAIN CHECKOUT back as an isolated worktree. Paths are now compared canonicalized. +- **Silent frontend failures got voices:** a failed workflow launch shows an error toast (was: modal closes, nothing happens, reason in devtools only), launch-variable `pattern`s are validated in the modal (the backend rejection was invisible), a previous launch's late stream callbacks can't write into the next run's live view (generation guard), out-of-order detail fetches can't repaint the wrong workflow, and ~25 silent catch blocks across Settings/MCP/Chat now toast (with optimistic-state revert where no refetch exists). +- **`check-types-drift` really covers unions now.** Tagged unions were compared first-variant-only while the script claimed full coverage — adding a field to `WorkflowTrigger::Tracker` passed CI green. Union of all variants' fields on both sides, quoted-key support, and an honest coverage line (`168 field-compared, 21 skipped…`). +- **DB rows that fail to parse are LOUD:** a workflow whose stored trigger/steps JSON no longer parses logs an error naming it (was: silently became Manual/zero-step and "ran" green), unknown learning statuses warn (a `Rejected` learning silently reappearing as `Pending` rewrote a human decision), editing a concurrently-deleted workflow 404s instead of reporting success. + ### Security - **Secure network defaults — no more accidental LAN exposure.** A stock `docker compose up` now publishes Kronn on `127.0.0.1` only (gateway + frontend), so a fresh install is not reachable from other machines. Exposing to the LAN is an explicit opt-in (`KRONN_BIND=0.0.0.0`) — and when you opt in, the backend **refuses to start unauthenticated**: it requires `KRONN_AUTH_TOKEN`, enabled auth, or an explicit `KRONN_ALLOW_INSECURE_LAN=1` acknowledgment. Closes the "any LAN peer could trigger an Exec step with your credentials (→ host root via the mounted docker socket)" chain flagged by the audit, while keeping the deliberate WSL-backend / Mac-frontend split working (opt-in + token). diff --git a/backend/Dockerfile b/backend/Dockerfile index a8ad1e59..842b42a0 100644 --- a/backend/Dockerfile +++ b/backend/Dockerfile @@ -178,6 +178,9 @@ RUN mkdir -p /home/kronn/.local/bin /home/kronn/.local/share/uv \ RUN chmod 1777 /home ENV KRONN_DATA_DIR=/data +# Explicit container marker — is_docker() must NOT key on KRONN_DATA_DIR (a +# native user can set that too, and a false "docker" fail-opens the LAN guard). +ENV KRONN_IN_DOCKER=1 ENV PATH="/home/kronn/.local/bin:/usr/local/bin:${PATH}" # The container runs as the non-root `kronn` user, but npm's default global # prefix (/usr/local) is root-owned → `npm install -g ` fails with diff --git a/backend/scripts/disc-introspection-mcp.py b/backend/scripts/disc-introspection-mcp.py index a4c4642a..2f798302 100755 --- a/backend/scripts/disc-introspection-mcp.py +++ b/backend/scripts/disc-introspection-mcp.py @@ -1940,6 +1940,11 @@ def _current_disc_meta(): try: url = f"{_backend_url()}/api/discussions/{disc_id}/meta" req = urllib.request.Request(url, method="GET") + # Same bearer as _http(): without it, an auth-enforced instance 401s + # this read and the silent fallback drops project/agent inheritance. + token = os.environ.get("KRONN_AUTH_TOKEN") + if token: + req.add_header("Authorization", f"Bearer {token}") with urllib.request.urlopen(req, timeout=5) as resp: payload = json.loads(resp.read().decode("utf-8")) data = payload.get("data") or {} diff --git a/backend/scripts/test_disc_introspection_mcp.py b/backend/scripts/test_disc_introspection_mcp.py index 5d712361..47370751 100644 --- a/backend/scripts/test_disc_introspection_mcp.py +++ b/backend/scripts/test_disc_introspection_mcp.py @@ -3505,6 +3505,24 @@ def test_http_omits_auth_when_no_token(self): req = urlopen.call_args.args[0] self.assertIsNone(req.get_header("Authorization")) + def test_current_disc_meta_adds_bearer_when_token_present(self): + # Regression (Codex audit 2026-07-12): this read used a bare urlopen — + # on an auth-enforced instance it 401'd silently and project/agent + # inheritance fell back to defaults. + cm = mock.MagicMock() + cm.__enter__.return_value.read.return_value = b'{"success": true, "data": {"project_id": "p-1"}}' + cm.__exit__.return_value = False + with mock.patch.dict(os.environ, + {"KRONN_BACKEND_URL": "http://127.0.0.1:3140", + "KRONN_AUTH_TOKEN": "sekret", + "KRONN_DISCUSSION_ID": "d-1"}, + clear=True), \ + mock.patch("urllib.request.urlopen", return_value=cm) as urlopen: + meta = self.mod._current_disc_meta() + req = urlopen.call_args.args[0] + self.assertEqual(req.get_header("Authorization"), "Bearer sekret") + self.assertEqual(meta.get("project_id"), "p-1") + def test_http_text_adds_bearer_when_token_present(self): cm = mock.MagicMock() cm.__enter__.return_value.read.return_value = b'{"kind":"kronn.workflow"}' diff --git a/backend/src/agents/runner.rs b/backend/src/agents/runner.rs index 0cb420de..c195e1f8 100644 --- a/backend/src/agents/runner.rs +++ b/backend/src/agents/runner.rs @@ -925,8 +925,20 @@ pub async fn start_agent_with_config(config: AgentStartConfig<'_>) -> Result { + if tx_out.send(line).await.is_err() { break; } + } + Ok(None) => break, + Err(e) => { + tracing::warn!("agent stdout read error (output truncated): {}", e); + break; + } + } } }); } @@ -937,8 +949,17 @@ pub async fn start_agent_with_config(config: AgentStartConfig<'_>) -> Result { + if tx_err.send(line).await.is_err() { break; } + } + Ok(None) => break, + Err(e) => { + tracing::warn!("agent stderr read error (output truncated): {}", e); + break; + } + } } }); } @@ -948,10 +969,19 @@ pub async fn start_agent_with_config(config: AgentStartConfig<'_>) -> Result { + tracing::debug!("agent stderr: {}", line); + if let Ok(mut buf) = capture.lock() { + buf.push(line); + } + } + Ok(None) => break, + Err(e) => { + tracing::warn!("agent stderr read error (capture truncated): {}", e); + break; + } } } })); @@ -974,7 +1004,7 @@ pub(crate) async fn ensure_kiro_cli_available() -> Result<(), String> { .args([ "-c", "command -v unzip >/dev/null 2>&1 || { echo 'Missing dependency: unzip' >&2; exit 127; }; \ - curl -fsSL https://cli.kiro.dev/install | bash", + curl -fsSL --connect-timeout 10 --max-time 300 https://cli.kiro.dev/install | bash", ]) .output() .await @@ -1149,8 +1179,24 @@ const OLLAMA_NUM_CTX_AUTO_CEILING: u64 = 32768; /// mutating process env): a value below the floor or unparseable → None (the /// auto/model-derived path decides). pub(crate) fn parse_num_ctx_cap(raw: Option) -> Option { - raw.and_then(|v| v.trim().parse::().ok()) - .filter(|&v| v >= OLLAMA_NUM_CTX_FLOOR) + let raw = raw?; + let trimmed = raw.trim(); + if trimmed.is_empty() { + return None; + } + match trimmed.parse::() { + Ok(v) if v >= OLLAMA_NUM_CTX_FLOOR => Some(v), + _ => { + // An operator who SET the override asked for a specific (usually + // smaller) window; silently falling through to the auto cap can + // hand them up to 32K — the opposite of what they wanted on a + // RAM-constrained box. Loud, not silent. + tracing::warn!( + "KRONN_OLLAMA_NUM_CTX_CAP=\"{trimmed}\" ignored (not a number ≥ {OLLAMA_NUM_CTX_FLOOR}) — using the model-derived auto cap instead, which may be LARGER than intended" + ); + None + } + } } /// Effective ctx cap (0.8.11 — product default, zero configuration): @@ -1188,28 +1234,49 @@ async fn ollama_model_ctx_limit(base: &str, model: &str) -> Option { static CACHE: std::sync::OnceLock>>> = std::sync::OnceLock::new(); let cache = CACHE.get_or_init(Default::default); - if let Some(hit) = cache.lock().ok().and_then(|c| c.get(model).copied()) { + // Key on base+model: switching the Ollama endpoint in config must not + // serve the previous server's limits for the rest of the process. + let cache_key = format!("{base}|{model}"); + if let Some(hit) = cache.lock().ok().and_then(|c| c.get(&cache_key).copied()) { return hit; } - let fetched = async { + // Ok(_) = definitive answer from Ollama (cacheable — the value is static + // per model tag, present or genuinely absent). Err = transport failure: + // NOT cached. /api/show typically fires at the first Ollama step after + // boot, exactly when Ollama may be busy cold-loading a 32b model — one + // transient 5s miss must not pin the 8192 fallback (and its silent + // truncation) for the whole process lifetime. + let fetched: Result, ()> = async { let client = reqwest::Client::builder() .timeout(std::time::Duration::from_secs(5)) .build() - .ok()?; + .map_err(|_| ())?; let resp = client .post(format!("{}/api/show", base)) .json(&serde_json::json!({ "model": model })) .send() .await - .ok()?; - let v: serde_json::Value = resp.json().await.ok()?; - parse_context_length(&v) + .map_err(|_| ())? + .error_for_status() + .map_err(|_| ())?; + let v: serde_json::Value = resp.json().await.map_err(|_| ())?; + Ok(parse_context_length(&v)) } .await; - if let Ok(mut c) = cache.lock() { - c.insert(model.to_string(), fetched); + match fetched { + Ok(limit) => { + if let Ok(mut c) = cache.lock() { + c.insert(cache_key, limit); + } + limit + } + Err(()) => { + tracing::warn!( + "Ollama /api/show failed for {model} — portable ctx fallback for this call only (will retry next step)" + ); + None + } } - fetched } /// Size the context window to the prompt, bounded by [FLOOR, cap]. Coarse on @@ -1319,15 +1386,29 @@ pub(crate) async fn forward_ollama_line( line: &str, tx: &tokio::sync::mpsc::Sender, stderr: &Arc>>, + got_done: &mut bool, + got_error: &mut bool, ) -> bool { if line.trim().is_empty() { return true; } if let Ok(json) = serde_json::from_str::(line) { + // In-band error object ({"error":"model runner has unexpectedly + // stopped"}, sent on a 200 stream when the model crashes mid- + // generation). Swallowing it made the step SUCCEED with empty or + // truncated output — surface it so the step fails with the reason. + if let Some(err) = json["error"].as_str() { + tracing::warn!("Ollama in-band error: {err}"); + if let Ok(mut stderr) = stderr.lock() { + stderr.push(format!("Ollama error: {err}")); + } + *got_error = true; + } if let Some(text) = json["message"]["content"].as_str() { if !text.is_empty() && tx.send(text.to_string()).await.is_err() { return false; } } if json["done"].as_bool() == Some(true) { + *got_done = true; let prompt_tokens = json["prompt_eval_count"].as_u64().unwrap_or(0); let eval_tokens = json["eval_count"].as_u64().unwrap_or(0); if let Ok(mut stderr) = stderr.lock() { @@ -1395,6 +1476,11 @@ async fn start_ollama_http( let client = reqwest::Client::builder() .timeout(std::time::Duration::from_secs(600)) // 10 min max for slow models + // Connect is bounded separately: without this, a black-holed OLLAMA_HOST + // (firewall DROP, wrong host.docker.internal) sits in TCP connect for the + // full 600s before "unreachable" surfaces, instead of failing in 5s like + // the /api/show probe above. + .connect_timeout(std::time::Duration::from_secs(5)) .build() .map_err(|e| format!("HTTP client error: {}", e))?; @@ -1421,12 +1507,17 @@ async fn start_ollama_http( // after the stream to finalize (a `sleep 3600` placeholder blocked there // for an hour — the UI spinner never stopped, 2026-07-01 bug), and the // audit zombie-probe `try_wait()`s after 60s idle (an already-exited child - // would be a false zombie during a slow model cold-load). Solution: `cat` - // with a piped stdin whose write end is HELD BY the streaming task below — - // when the stream ends (or errors, or the task dies), the pipe closes, - // `cat` sees EOF and exits 0 → wait() returns success immediately, and - // while the stream lives the child is genuinely alive for the probe. - let mut dummy_child = crate::core::cmd::async_cmd("cat") + // would be a false zombie during a slow model cold-load). + // + // Solution: a tiny sh lifeline that blocks on `read` from a piped stdin + // held by the streaming task, then exits with the STATUS the task writes. + // The old `cat` variant exited 0 on every path — a mid-stream HTTP error + // or an in-band {"error":…} object still looked like a SUCCESSFUL step + // with truncated/empty output. Now: task writes "0\n" on a clean `done`, + // "1\n" on stream/in-band errors, and if the task dies without writing + // anything, `read` hits EOF and the lifeline exits 1 — fail-safe. + let mut dummy_child = crate::core::cmd::async_cmd("sh") + .args(["-c", r#"read -r s; exit "${s:-1}""#]) .stdin(std::process::Stdio::piped()) .stdout(std::process::Stdio::null()) .stderr(std::process::Stdio::null()) @@ -1436,18 +1527,32 @@ async fn start_ollama_http( // Spawn a background task to read the HTTP stream and forward text to the channel. tokio::spawn(async move { - // Holds `cat`'s stdin open for the duration of the stream — dropped on - // every exit path of this task, which lets the child exit. - let _stream_lifetime = stdin_guard; + // Holds the lifeline's stdin open for the duration of the stream. + let mut lifeline = stdin_guard; + // Report the stream's outcome as the lifeline child's exit code. + // `ok=false` also covers "consumer dropped" (cancel) — the run is + // being torn down anyway, so the failed wait() is never surfaced. + async fn finish(lifeline: &mut Option, ok: bool) { + if let Some(mut stdin) = lifeline.take() { + use tokio::io::AsyncWriteExt; + let _ = stdin.write_all(if ok { b"0\n" } else { b"1\n" }).await; + } + } use futures::StreamExt; let mut stream = response.bytes_stream(); let mut buffer = String::new(); + let mut got_done = false; + let mut got_error = false; while let Some(chunk) = stream.next().await { let bytes = match chunk { Ok(b) => b, Err(e) => { tracing::warn!("Ollama stream error: {}", e); + if let Ok(mut se) = stderr_clone.lock() { + se.push(format!("Ollama stream error (connection lost mid-generation): {e}")); + } + got_error = true; break; } }; @@ -1459,7 +1564,8 @@ async fn start_ollama_http( let line = buffer[..newline_pos].to_string(); buffer = buffer[newline_pos + 1..].to_string(); // Consumer gone (cancel) → stop reading the HTTP body. - if !forward_ollama_line(&line, &tx, &stderr_clone).await { + if !forward_ollama_line(&line, &tx, &stderr_clone, &mut got_done, &mut got_error).await { + finish(&mut lifeline, false).await; return; } } @@ -1468,7 +1574,18 @@ async fn start_ollama_http( // Non-streaming responses (format-constrained / TypedSchema steps set // stream:false) arrive as a single JSON object with no trailing // newline, so the line loop above never fires — flush the remainder. - let _ = forward_ollama_line(buffer.trim(), &tx, &stderr_clone).await; + let _ = forward_ollama_line(buffer.trim(), &tx, &stderr_clone, &mut got_done, &mut got_error).await; + + // A stream that ends without the terminal `done` chunk was truncated + // (server closed the connection, proxy cut it, model unloaded) — that + // must not pass as a successful step. + if !got_done && !got_error { + if let Ok(mut se) = stderr_clone.lock() { + se.push("Ollama stream ended without a terminal 'done' chunk — output is likely truncated".into()); + } + got_error = true; + } + finish(&mut lifeline, got_done && !got_error).await; }); Ok(AgentProcess { diff --git a/backend/src/agents/runner_test.rs b/backend/src/agents/runner_test.rs index 9fc44613..5a1905eb 100644 --- a/backend/src/agents/runner_test.rs +++ b/backend/src/agents/runner_test.rs @@ -124,14 +124,17 @@ mod tests { use std::sync::{Arc, Mutex}; let (tx, mut rx) = tokio::sync::mpsc::channel::(8); let stderr = Arc::new(Mutex::new(Vec::::new())); + let (mut done, mut err) = (false, false); // A streamed content chunk... - forward_ollama_line(r#"{"message":{"content":"391"},"done":false}"#, &tx, &stderr).await; + forward_ollama_line(r#"{"message":{"content":"391"},"done":false}"#, &tx, &stderr, &mut done, &mut err).await; + assert!(!done && !err); // ...then the terminal `done` object (identical shape to a non-stream // single-object response), carrying the token counts. forward_ollama_line( r#"{"message":{"content":""},"done":true,"prompt_eval_count":12,"eval_count":3}"#, - &tx, &stderr, + &tx, &stderr, &mut done, &mut err, ).await; + assert!(done && !err, "terminal chunk sets got_done, no error"); drop(tx); let mut got = String::new(); while let Some(s) = rx.recv().await { got.push_str(&s); } @@ -144,11 +147,34 @@ mod tests { use std::sync::{Arc, Mutex}; let (tx, mut rx) = tokio::sync::mpsc::channel::(8); let stderr = Arc::new(Mutex::new(Vec::::new())); - forward_ollama_line(" ", &tx, &stderr).await; // blank tail buffer - forward_ollama_line("{not json", &tx, &stderr).await; // partial/garbage + let (mut done, mut err) = (false, false); + forward_ollama_line(" ", &tx, &stderr, &mut done, &mut err).await; // blank tail buffer + forward_ollama_line("{not json", &tx, &stderr, &mut done, &mut err).await; // partial/garbage drop(tx); assert!(rx.recv().await.is_none(), "no content forwarded"); assert!(stderr.lock().unwrap().is_empty(), "no token line captured"); + assert!(!done && !err, "garbage neither completes nor fails the stream"); + } + + #[tokio::test] + async fn forward_ollama_line_surfaces_in_band_error() { + use std::sync::{Arc, Mutex}; + let (tx, mut rx) = tokio::sync::mpsc::channel::(8); + let stderr = Arc::new(Mutex::new(Vec::::new())); + let (mut done, mut err) = (false, false); + // Ollama's mid-stream error shape (HTTP 200, model crashed): used to + // be silently ignored → step "succeeded" with empty output. + forward_ollama_line( + r#"{"error":"model runner has unexpectedly stopped"}"#, + &tx, &stderr, &mut done, &mut err, + ).await; + drop(tx); + assert!(err, "in-band error object must mark the stream failed"); + assert!(!done); + assert!(rx.recv().await.is_none()); + let lines = stderr.lock().unwrap(); + assert!(lines.iter().any(|l| l.contains("model runner has unexpectedly stopped")), + "error reason must reach the stderr tail shown on step failure: {lines:?}"); } #[test] diff --git a/backend/src/api/audit/validation.rs b/backend/src/api/audit/validation.rs index 5e596387..06a7941c 100644 --- a/backend/src/api/audit/validation.rs +++ b/backend/src/api/audit/validation.rs @@ -749,6 +749,7 @@ mod tests { } #[test] + #[serial] #[serial(kronn_templates_env)] fn missing_template_only_flags_empty_dest() { // No template on disk (sub-audit case): we can only flag a diff --git a/backend/src/api/contacts.rs b/backend/src/api/contacts.rs index a460e081..b376284f 100644 --- a/backend/src/api/contacts.rs +++ b/backend/src/api/contacts.rs @@ -32,7 +32,7 @@ pub async fn add( } // Ping the peer to check reachability (non-blocking, 3s timeout) - let health_url = format!("{}/api/health", &kronn_url); + let health_url = format!("{}/api/health", kronn_url); let ping_error = reqwest::Client::new() .get(&health_url) .timeout(std::time::Duration::from_secs(3)) diff --git a/backend/src/api/disc_git.rs b/backend/src/api/disc_git.rs index e7aebb98..d2bf5f2c 100644 --- a/backend/src/api/disc_git.rs +++ b/backend/src/api/disc_git.rs @@ -468,13 +468,17 @@ pub async fn test_mode_enter( return Json(ApiResponse::err(format!("Failed to unlock worktree: {}", e))); } let did = disc.id.clone(); - let _ = state.db.with_conn(move |conn| { + if let Err(e) = state.db.with_conn(move |conn| { conn.execute( "UPDATE discussions SET workspace_path = NULL WHERE id = ?1", rusqlite::params![did], )?; Ok(()) - }).await; + }).await { + // Worktree is already gone on disk — a stale pointer is + // recoverable, but say so instead of pretending it's clean. + tracing::warn!("Test mode: worktree removed but clearing workspace_path failed: {e}"); + } } // ── Checkout the discussion branch in the main repo ───────────────── @@ -488,15 +492,29 @@ pub async fn test_mode_enter( } // ── Persist test-mode state in DB ──────────────────────────────────── + // This row is the ONLY record of how to undo the working-tree mutation we + // just made (previous branch + the stash holding the user's dirty files). + // If it doesn't persist, exiting test mode can't restore and the stash + // pointer is lost — so a failure here rolls the checkout back instead of + // reporting success. let previous_branch = state_before.current_branch.clone(); let restore = previous_branch.clone(); let stash_ref_clone = if stashed { Some(stash_message.clone()) } else { None }; let did = disc.id.clone(); - let _ = state.db.with_conn(move |conn| { + if let Err(e) = state.db.with_conn(move |conn| { crate::db::discussions::update_discussion_test_mode( conn, &did, Some(&restore), stash_ref_clone.as_deref(), ) - }).await; + }).await { + let _ = crate::core::worktree::checkout_branch(&repo_path, &previous_branch); + let _ = crate::core::worktree::reattach_worktree(&repo_path, &project.name, &disc.title, &branch); + if stashed { + let _ = crate::core::worktree::stash_pop_by_message(&repo_path, &stash_message); + } + return Json(ApiResponse::err(format!( + "Could not persist test-mode restore state ({e}) — checkout rolled back to `{previous_branch}`" + ))); + } tracing::info!( "Test mode ON for disc '{}': main repo {} → {} (stashed={})", diff --git a/backend/src/api/disc_invite.rs b/backend/src/api/disc_invite.rs index 61dc32e6..41416715 100644 --- a/backend/src/api/disc_invite.rs +++ b/backend/src/api/disc_invite.rs @@ -796,16 +796,17 @@ pub async fn fetch_file( if from_code.is_empty() { return Json(ApiResponse::err("from_invite_code required")); } - // Authenticate the caller (must be a known contact). - match state + // Authenticate the caller (must be a known contact) — and KEEP its id: + // being a contact is not enough to read arbitrary files (see below). + let caller = match state .db .with_conn(move |conn| crate::db::contacts::find_contact_by_invite_code(conn, &from_code)) .await { - Ok(Some(_)) => {} + Ok(Some(c)) => c, Ok(None) => return Json(ApiResponse::err("unknown peer (invite code not in contacts)")), Err(e) => return Json(ApiResponse::err(format!("contact lookup error: {e}"))), - } + }; let file_id = req.file_id.clone(); let cf = match state @@ -825,6 +826,31 @@ pub async fn fetch_file( Err(e) => return Json(ApiResponse::err(format!("file lookup error: {e}"))), }; + // Scope check: the file's discussion must be SHARED WITH this contact. + // Same `found: false` shape as a missing file — no existence oracle. + let disc_id = cf.discussion_id.clone(); + let shared_with_caller = match state + .db + .with_conn(move |conn| crate::db::discussions::get_discussion(conn, &disc_id)) + .await + { + Ok(Some(d)) => d.shared_with.contains(&caller.id), + Ok(None) => false, + Err(e) => return Json(ApiResponse::err(format!("discussion lookup error: {e}"))), + }; + if !shared_with_caller { + tracing::warn!( + "fetch-file refused: contact '{}' asked for file {} of a discussion not shared with it", + caller.pseudo, cf.id + ); + return Json(ApiResponse::ok(FetchFileResponse { + found: false, + filename: None, + mime_type: None, + data_base64: None, + })); + } + let Some(disk_path) = cf.disk_path.clone() else { // Text-only context file (no binary on disk) — nothing to transfer. return Json(ApiResponse::ok(FetchFileResponse { @@ -884,6 +910,57 @@ mod tests { AppState::new_defaults(cfg, db, DEFAULT_MAX_CONCURRENT_AGENTS) } + #[tokio::test] + async fn fetch_file_is_scoped_to_discussions_shared_with_the_caller() { + // Regression (Codex audit 2026-07-12): any accepted contact could + // read ANY context file by id. + let state = make_state_with_disc("d-fetch-1").await; + let tmp = tempfile::TempDir::new().unwrap(); + let blob = tmp.path().join("doc.pdf"); + std::fs::write(&blob, b"BYTES").unwrap(); + let blob_str = blob.to_string_lossy().to_string(); + state.db.with_conn(move |conn| { + crate::db::contacts::insert_contact(conn, &crate::models::Contact { + id: "c-1".into(), + pseudo: "peer".into(), + avatar_email: None, + kronn_url: "http://peer.local".into(), + invite_code: "kr-inv-abc".into(), + status: "accepted".into(), + created_at: chrono::Utc::now(), + updated_at: chrono::Utc::now(), + })?; + crate::db::discussions::insert_federated_context_file( + conn, "f-1", "d-fetch-1", "m-1", "doc.pdf", "application/pdf", 5, &blob_str, + ).map_err(|e| anyhow::anyhow!(e))?; + Ok(()) + }).await.unwrap(); + + // Known contact, but the discussion is NOT shared with it → found: false. + let resp = fetch_file(State(state.clone()), Json(FetchFileRequest { + file_id: "f-1".into(), + from_invite_code: "kr-inv-abc".into(), + })).await; + let body = resp.0.data.expect("ok envelope (no existence oracle)"); + assert!(!body.found, "unshared discussion must not leak files"); + assert!(body.data_base64.is_none()); + + // Share the discussion with that contact → the bytes flow. + state.db.with_conn(|conn| { + crate::db::discussions::update_discussion_sharing(conn, "d-fetch-1", "sh-1", &["c-1".to_string()]).map(|_| ()) + }).await.unwrap(); + let resp = fetch_file(State(state.clone()), Json(FetchFileRequest { + file_id: "f-1".into(), + from_invite_code: "kr-inv-abc".into(), + })).await; + let body = resp.0.data.unwrap(); + assert!(body.found); + use base64::Engine as _; + let bytes = base64::engine::general_purpose::STANDARD + .decode(body.data_base64.unwrap()).unwrap(); + assert_eq!(bytes, b"BYTES"); + } + #[tokio::test] async fn invite_peer_returns_plain_token_for_existing_disc() { let state = make_state_with_disc("d-invite-1").await; diff --git a/backend/src/api/discover.rs b/backend/src/api/discover.rs index bf363f1c..fd05d853 100644 --- a/backend/src/api/discover.rs +++ b/backend/src/api/discover.rs @@ -250,8 +250,19 @@ fn normalize_repo_url(url: &str) -> String { } /// Fetch all repos for the authenticated GitHub user, including organization repos. +/// Bounded per request: discovery paginates in a loop inside a GET handler — +/// one stalled page (self-hosted GitLab that accepts and never answers) would +/// pin the handler forever; the UI modal has no cancel. +fn discovery_client() -> reqwest::Client { + reqwest::Client::builder() + .connect_timeout(std::time::Duration::from_secs(5)) + .timeout(std::time::Duration::from_secs(15)) + .build() + .unwrap_or_default() +} + async fn fetch_github_repos(token: &str) -> Result, String> { - let client = reqwest::Client::new(); + let client = discovery_client(); let mut all_repos = vec![]; let mut seen = std::collections::HashSet::new(); @@ -351,7 +362,7 @@ fn parse_github_repo(r: &serde_json::Value) -> RemoteRepo { /// Fetch all repos for the authenticated GitLab user, including group repos. async fn fetch_gitlab_repos(token: &str, api_url: &str) -> Result, String> { - let client = reqwest::Client::new(); + let client = discovery_client(); let base = api_url.trim_end_matches('/'); let mut all_repos = vec![]; let mut seen = std::collections::HashSet::new(); diff --git a/backend/src/api/docs.rs b/backend/src/api/docs.rs index 7812ea33..60445e87 100644 --- a/backend/src/api/docs.rs +++ b/backend/src/api/docs.rs @@ -242,7 +242,13 @@ async fn proxy_to_sidecar( ); } - let client = reqwest::Client::new(); + // Local sidecar, but a wedged process must not pin this request forever; + // 300s leaves ample room for a legitimately long docgen. + let client = reqwest::Client::builder() + .connect_timeout(std::time::Duration::from_secs(5)) + .timeout(std::time::Duration::from_secs(300)) + .build() + .unwrap_or_default(); let url = format!("{}/{}", sidecar.base_url, route); let resp = match client.post(&url).json(&payload).send().await { Ok(r) => r, diff --git a/backend/src/api/federation.rs b/backend/src/api/federation.rs index dc07a4c6..8ac84081 100644 --- a/backend/src/api/federation.rs +++ b/backend/src/api/federation.rs @@ -222,10 +222,27 @@ pub async fn fetch_and_store_attachment( tracing::warn!("F8: host did not return bytes for {file_id}"); return; }; + // Bound the transfer: a buggy/compromised peer must not force an + // arbitrary allocation. + let cap = fetch_cap_bytes(size); + if !b64_len_within_cap(data_b64.len(), cap) { + tracing::warn!( + "F8: peer {} sent {} b64 bytes for {file_id} (announced {size}) — refusing oversized transfer", + host.pseudo, data_b64.len() + ); + return; + } let bytes = match base64::engine::general_purpose::STANDARD.decode(data_b64) { Ok(b) => b, Err(_) => return, }; + if bytes.len() > cap { + tracing::warn!( + "F8: decoded {} bytes for {file_id} exceeds cap {cap} (announced {size}) — dropping", + bytes.len() + ); + return; + } // Mirror discs have no project work_dir → persistent config-dir store. let disk_path = match crate::core::context_files::save_file_to_disk(file_id, filename, &bytes) { @@ -286,3 +303,41 @@ pub async fn fetch_and_store_attachment( pending: false, // F15+ ready: binary stored → front swaps placeholder for the file }); } + +/// Transfer cap for a fetched F8 attachment: 2× the announced size, clamped +/// to [1 MB, 64 MB]. Pure — the announced size comes from the peer's message +/// and is not trusted beyond sizing the allowance. +pub(crate) fn fetch_cap_bytes(announced: i64) -> usize { + const MAX_FETCHED_FILE_BYTES: usize = 64 * 1024 * 1024; + (announced.max(0) as usize) + .saturating_mul(2) + .clamp(1024 * 1024, MAX_FETCHED_FILE_BYTES) +} + +/// Pre-decode check on the base64 payload length (4/3 expansion + head slack) +/// so an oversized body is refused BEFORE allocating the decoded buffer. +pub(crate) fn b64_len_within_cap(b64_len: usize, cap: usize) -> bool { + b64_len <= cap.saturating_mul(4) / 3 + 1024 +} + +#[cfg(test)] +mod transfer_cap_tests { + use super::*; + + #[test] + fn fetch_cap_scales_with_announcement_within_bounds() { + assert_eq!(fetch_cap_bytes(0), 1024 * 1024, "no/zero announcement → 1MB floor"); + assert_eq!(fetch_cap_bytes(-5), 1024 * 1024, "negative announcement → floor"); + assert_eq!(fetch_cap_bytes(10 * 1024 * 1024), 20 * 1024 * 1024, "2× announced"); + assert_eq!(fetch_cap_bytes(i64::MAX), 64 * 1024 * 1024, "absolute ceiling"); + } + + #[test] + fn b64_precheck_refuses_oversized_and_accepts_valid() { + let cap = fetch_cap_bytes(6); // tiny announcement → 1MB floor + assert!(b64_len_within_cap(8, cap), "a valid tiny payload passes"); + assert!(!b64_len_within_cap(cap * 2, cap), "a payload twice the cap is refused pre-decode"); + // Boundary: exactly the b64 expansion of the cap passes. + assert!(b64_len_within_cap(cap * 4 / 3, cap)); + } +} diff --git a/backend/src/api/git_ops.rs b/backend/src/api/git_ops.rs index ef22482b..7dc52b54 100644 --- a/backend/src/api/git_ops.rs +++ b/backend/src/api/git_ops.rs @@ -403,6 +403,7 @@ fn ssh_to_https_with_token(remote_url: &str, token: &str) -> Option { /// Push the current branch to origin. pub fn run_git_push(repo_path: &Path, github_token: Option<&str>) -> Result { let branch_output = sync_cmd("git") + .env("GIT_TERMINAL_PROMPT", "0") .args(["branch", "--show-current"]) .current_dir(repo_path) .output() @@ -416,6 +417,7 @@ pub fn run_git_push(repo_path: &Path, github_token: Option<&str>) -> Result) -> Result = runs .iter() .filter(|r| { + // Interrupted is terminal but deliberately NOT a duration sample: + // its finished_at is stamped by the boot reconcile, so "duration" + // would be the crash-to-reboot gap (a laptop closed overnight = + // one 10h sample among ten 5min ones → next_check hours late). matches!( r.status, RunStatus::Success | RunStatus::Failed | RunStatus::Cancelled | RunStatus::StoppedByGuard - | RunStatus::Interrupted ) }) .filter_map(|r| { @@ -244,7 +247,7 @@ pub async fn workflow_trigger( let agents = cfg.agents.clone(); drop(cfg); if let Err(e) = crate::workflows::runner::execute_run( - state_for_run, + state_for_run.clone(), &wf_for_run, &mut run_exec, &tokens, @@ -257,6 +260,9 @@ pub async fn workflow_trigger( { tracing::error!("Workflow run {} failed: {}", run_exec.id, e); } + // Agent-triggered runs are as unattended as cron runs — same webhook + // contract on a non-success terminal state (best-effort, bounded). + crate::core::run_notify::notify_if_failed(&state_for_run, &wf_for_run, &run_exec).await; }); Json(ApiResponse::ok(McpTriggerWorkflowResponse { diff --git a/backend/src/api/projects/clone.rs b/backend/src/api/projects/clone.rs index 63d6de95..1c140d33 100644 --- a/backend/src/api/projects/clone.rs +++ b/backend/src/api/projects/clone.rs @@ -161,6 +161,10 @@ async fn clone_with_fallbacks( let res = tokio::task::spawn_blocking(move || { sync_cmd("git") .env("GIT_TERMINAL_PROMPT", "0") + // Abort a clone stalled under 1 KB/s for 60s instead of + // pinning the blocking thread on a dead network forever. + .env("GIT_HTTP_LOW_SPEED_LIMIT", "1024") + .env("GIT_HTTP_LOW_SPEED_TIME", "60") .args(["clone", &cand_owned, &dest_buf.to_string_lossy()]) .output() }) diff --git a/backend/src/api/setup.rs b/backend/src/api/setup.rs index 06db3542..1dc49e8a 100644 --- a/backend/src/api/setup.rs +++ b/backend/src/api/setup.rs @@ -856,25 +856,63 @@ fn sync_codex_auth(key: Option<&str>) { let codex_dir = std::path::PathBuf::from(home).join(".codex"); let codex_auth_path = codex_dir.join("auth.json"); + // MERGE into the existing auth.json — never wholesale-replace it. A user + // logged into Codex via ChatGPT-subscription OAuth has refresh/access + // tokens in this file that Kronn didn't write; replacing (or deleting) + // the file destroyed that login. We only own two fields. + let existing: serde_json::Map = + match std::fs::read_to_string(&codex_auth_path) { + Ok(s) => match serde_json::from_str::(&s) { + Ok(serde_json::Value::Object(m)) => m, + Ok(_) | Err(_) if key.is_some() => { + // Corrupt/non-object file and we're about to write: don't + // merge garbage, but say what we're replacing. + tracing::warn!("{} was not valid JSON — rewriting it", codex_auth_path.display()); + serde_json::Map::new() + } + _ => { + tracing::warn!("{} unreadable as JSON — leaving it untouched", codex_auth_path.display()); + return; + } + }, + Err(_) => serde_json::Map::new(), // missing file: start fresh + }; + match key { Some(k) => { - // Create .codex dir if needed let _ = std::fs::create_dir_all(&codex_dir); - let content = serde_json::json!({ - "auth_mode": "apikey", - "OPENAI_API_KEY": k, - }); - // Safety: serializing a serde_json::Value literal cannot fail + let mut merged = existing; + merged.insert("auth_mode".into(), serde_json::Value::String("apikey".into())); + merged.insert("OPENAI_API_KEY".into(), serde_json::Value::String(k.into())); + let content = serde_json::Value::Object(merged); + // Safety: serializing a serde_json::Value cannot fail match std::fs::write(&codex_auth_path, serde_json::to_string_pretty(&content).expect("JSON Value serialization cannot fail")) { - Ok(_) => tracing::info!("Synced OpenAI key to {}", codex_auth_path.display()), + Ok(_) => tracing::info!("Synced OpenAI key into {} (other fields preserved)", codex_auth_path.display()), Err(e) => tracing::warn!("Failed to write {}: {}", codex_auth_path.display(), e), } } None => { - // Delete the auth file so Codex falls back to its own login/subscription - match std::fs::remove_file(&codex_auth_path) { - Ok(_) => tracing::info!("Removed {} (Codex will use local auth)", codex_auth_path.display()), - Err(e) => tracing::warn!("Failed to remove {}: {}", codex_auth_path.display(), e), + // Remove ONLY our fields; other credentials (OAuth tokens) stay. + // Delete the file only when nothing else remains in it. + let mut merged = existing; + if merged.remove("OPENAI_API_KEY").is_none() && merged.get("auth_mode").and_then(|v| v.as_str()) != Some("apikey") { + return; // nothing of ours in there + } + if merged.get("auth_mode").and_then(|v| v.as_str()) == Some("apikey") { + merged.remove("auth_mode"); + } + if merged.is_empty() { + match std::fs::remove_file(&codex_auth_path) { + Ok(_) => tracing::info!("Removed {} (Codex will use local auth)", codex_auth_path.display()), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => tracing::warn!("Failed to remove {}: {}", codex_auth_path.display(), e), + } + } else { + let content = serde_json::Value::Object(merged); + match std::fs::write(&codex_auth_path, serde_json::to_string_pretty(&content).expect("JSON Value serialization cannot fail")) { + Ok(_) => tracing::info!("Removed Kronn's API-key fields from {} (other credentials preserved)", codex_auth_path.display()), + Err(e) => tracing::warn!("Failed to write {}: {}", codex_auth_path.display(), e), + } } } } @@ -943,8 +981,17 @@ pub async fn discover_keys( } if imported_count > 0 { - let _ = config::save(&config).await; - tracing::info!("Auto-imported {} API key(s)", imported_count); + match config::save(&config).await { + Ok(_) => tracing::info!("Auto-imported {} API key(s)", imported_count), + // Keys live only in memory now — claiming success would leave the + // user believing they persisted (they vanish at next restart). + Err(e) => { + tracing::error!("Auto-imported {} API key(s) but SAVING the config failed: {e} — keys are in memory only and will be lost at restart", imported_count); + return Json(ApiResponse::err(format!( + "Imported {imported_count} key(s) but persisting config.toml failed: {e}" + ))); + } + } } Json(ApiResponse::ok(DiscoverKeysResponse { @@ -1072,11 +1119,16 @@ pub async fn db_backup( let result = state.db.with_conn(move |conn| { let mut dst = rusqlite::Connection::open(&backup_path_owned)?; let backup = rusqlite::backup::Backup::new(conn, &mut dst)?; - // `step(-1)` runs the backup to completion in one call. For - // very large DBs (>500MB) we'd want to step in pages with a - // sleep between to keep the mutex contention down, but - // Kronn's DBs are typically <100MB. - backup.run_to_completion(5, std::time::Duration::from_millis(50), None)?; + // One-shot copy via `step(-1)` (all pages in a single call). Pausing + // between page batches only helps when OTHER connections could write + // in the gaps — Kronn has a single shared connection, so a pause just + // holds the global mutex longer (~2.5s/MB) for no benefit. + // NOT `run_to_completion(-1, …)`: it asserts pages_per_step > 0 and + // panics (2026-07-09 boot-tick incident — poisoned the DB mutex). + match backup.step(-1)? { + rusqlite::backup::StepResult::Done => {} + other => anyhow::bail!("backup did not complete in one step: {other:?}"), + } Ok(()) }).await; @@ -1749,6 +1801,7 @@ pub async fn open_url(Json(req): Json) -> Json> #[cfg(test)] mod tests { use super::*; + use serial_test::serial; use crate::core::config; // ─── import_clear_statements (I10: selective clear, no downgrade wipe) ──── @@ -2032,6 +2085,7 @@ mod tests { /// `global_context` travels in the exported config.toml but was dropped on /// import before 0.8.9. The merge must now re-apply it (+ its mode). #[tokio::test] + #[serial] async fn import_reapplies_global_context() { let _lock = ENV_LOCK.lock().await; let tmp = std::env::temp_dir().join(format!( diff --git a/backend/src/api/workflows.rs b/backend/src/api/workflows.rs index ca2b8703..4a58c9f7 100644 --- a/backend/src/api/workflows.rs +++ b/backend/src/api/workflows.rs @@ -1008,7 +1008,10 @@ pub async fn update( let w = updated.clone(); match state.db.with_conn(move |conn| crate::db::workflows::update_workflow(conn, &w)).await { - Ok(()) => Json(ApiResponse::ok(updated)), + Ok(true) => Json(ApiResponse::ok(updated)), + // The workflow existed when we loaded it above but was deleted + // concurrently before the UPDATE landed → 404, not a fake success. + Ok(false) => Json(ApiResponse::err_coded(ApiErrorCode::NotFound, "Workflow not found")), Err(e) => Json(ApiResponse::err(format!("DB error: {}", e))), } } @@ -2053,14 +2056,17 @@ pub async fn cancel_run( // from the agent-task finally path on their own tokens. let run_id_for_db2 = run_id.clone(); let forced_statuses = state.db.with_conn(move |conn| { + // Pending (inserted, runner not started) and WaitingApproval (gate- + // paused, token dropped at pause) runs were UNCANCELLABLE before: + // the token no-ops and this UPDATE only matched 'Running'. let parent_n = conn.execute( "UPDATE workflow_runs SET status = 'Cancelled', finished_at = datetime('now') \ - WHERE id = ?1 AND status = 'Running'", + WHERE id = ?1 AND status IN ('Running', 'Pending', 'WaitingApproval')", rusqlite::params![&run_id_for_db2], )?; let children_n = conn.execute( "UPDATE workflow_runs SET status = 'Cancelled', finished_at = datetime('now') \ - WHERE parent_run_id = ?1 AND status = 'Running'", + WHERE parent_run_id = ?1 AND status IN ('Running', 'Pending', 'WaitingApproval')", rusqlite::params![&run_id_for_db2], )?; Ok((parent_n, children_n)) @@ -2225,10 +2231,14 @@ pub async fn decide_run( drop(cfg); let mut run_mut = run_for_resume; if let Err(e) = crate::workflows::runner::resume_run( - state_clone, &workflow, &mut run_mut, decision, &tokens, &agents, None, + state_clone.clone(), &workflow, &mut run_mut, decision, &tokens, &agents, None, ).await { tracing::error!("Resume run {} failed: {}", run_id_for_log, e); } + // A gate-resumed run (human approve or the auto-approve timer) that + // then fails is exactly as unattended as its scheduled first half — + // same webhook contract as the engine-spawn tail. + crate::core::run_notify::notify_if_failed(&state_clone, &workflow, &run_mut).await; }); Json(ApiResponse::ok(DecideRunResponse { @@ -2374,7 +2384,8 @@ pub async fn test_worktree( let project_path = if let Some(pid) = workflow.project_id.clone() { match state.db.with_conn(move |conn| crate::db::projects::get_project(conn, &pid)).await { Ok(Some(p)) => p.path, - _ => return Json(ApiResponse::err_coded(ApiErrorCode::NotFound, "Project not found for this workflow")), + Ok(None) => return Json(ApiResponse::err_coded(ApiErrorCode::NotFound, "Project not found for this workflow")), + Err(e) => return Json(ApiResponse::err_coded(ApiErrorCode::Internal, format!("DB error: {}", e))), } } else { return Json(ApiResponse::err("Workflow has no project — cannot create a test worktree")); @@ -2448,13 +2459,15 @@ pub async fn delete_test_worktree( move |conn| crate::db::workflows::get_workflow(conn, &wf_id) }).await { Ok(Some(w)) => w, - _ => return Json(ApiResponse::err_coded(ApiErrorCode::NotFound, "Parent workflow not found")), + Ok(None) => return Json(ApiResponse::err_coded(ApiErrorCode::NotFound, "Parent workflow not found")), + Err(e) => return Json(ApiResponse::err_coded(ApiErrorCode::Internal, format!("DB error: {}", e))), }; let project_path = if let Some(pid) = workflow.project_id.clone() { match state.db.with_conn(move |conn| crate::db::projects::get_project(conn, &pid)).await { Ok(Some(p)) => p.path, - _ => return Json(ApiResponse::err("Project not found")), + Ok(None) => return Json(ApiResponse::err_coded(ApiErrorCode::NotFound, "Project not found")), + Err(e) => return Json(ApiResponse::err_coded(ApiErrorCode::Internal, format!("DB error: {}", e))), } } else { return Json(ApiResponse::err("Workflow has no project")); diff --git a/backend/src/core/backup.rs b/backend/src/core/backup.rs index 37c25f64..4d25717c 100644 --- a/backend/src/core/backup.rs +++ b/backend/src/core/backup.rs @@ -35,10 +35,17 @@ pub fn backup_filename(now: DateTime) -> String { format!("{BACKUP_PREFIX}{}.{BACKUP_EXT}", now.format("%Y%m%d-%H%M%S")) } +/// True when `name` is one of our scheduled backup files. +fn is_backup_name(name: &str) -> bool { + name.starts_with(BACKUP_PREFIX) && name.ends_with(&format!(".{BACKUP_EXT}")) +} + /// Delete the oldest scheduled backups in `dir`, keeping the `keep_n` most /// recent (by filename, which sorts chronologically thanks to the timestamp /// format). Only touches files matching our prefix/ext. Returns how many were -/// removed. Never errors on individual unlink failures (best-effort). +/// removed. Never errors on individual unlink failures (best-effort), but +/// warns — a permissions problem on an external KRONN_BACKUP_DIR would +/// otherwise accumulate backups unbounded while logging success. pub fn prune_old_backups(dir: &Path, keep_n: usize) -> usize { let mut ours: Vec = match std::fs::read_dir(dir) { Ok(rd) => rd @@ -46,11 +53,14 @@ pub fn prune_old_backups(dir: &Path, keep_n: usize) -> usize { .filter(|p| { p.file_name() .and_then(|n| n.to_str()) - .map(|n| n.starts_with(BACKUP_PREFIX) && n.ends_with(&format!(".{BACKUP_EXT}"))) + .map(is_backup_name) .unwrap_or(false) }) .collect(), - Err(_) => return 0, + Err(e) => { + tracing::warn!(target: "backup", "cannot list backup dir {}: {e} — pruning skipped", dir.display()); + return 0; + } }; if ours.len() <= keep_n { return 0; @@ -59,8 +69,9 @@ pub fn prune_old_backups(dir: &Path, keep_n: usize) -> usize { let to_remove = ours.len() - keep_n; let mut removed = 0; for p in ours.into_iter().take(to_remove) { - if std::fs::remove_file(&p).is_ok() { - removed += 1; + match std::fs::remove_file(&p) { + Ok(()) => removed += 1, + Err(e) => tracing::warn!(target: "backup", "failed to prune {}: {e}", p.display()), } } removed @@ -78,7 +89,19 @@ pub async fn perform_backup(db: &Database, dir: &Path, keep_n: usize) -> anyhow: db.with_conn(move |conn| { let mut dst = rusqlite::Connection::open(&dest_owned)?; let backup = rusqlite::backup::Backup::new(conn, &mut dst)?; - backup.run_to_completion(5, Duration::from_millis(50), None)?; + // One-shot copy: sqlite3_backup_step(-1) copies every page in a + // single call. The paged 5-pages/50ms variant is designed to let + // OTHER connections write between steps — but Kronn has a single + // shared connection, so the pauses just held the global DB mutex + // ~2.5s/MB while every API handler queued behind it. + // MUST be `step(-1)`, NOT `run_to_completion(-1, …)`: the latter + // asserts pages_per_step > 0 and PANICS — observed live 2026-07-09, + // where the boot-tick backup poisoned the DB mutex for the whole + // process (see `perform_backup_writes_a_readable_copy`). + match backup.step(-1)? { + rusqlite::backup::StepResult::Done => {} + other => anyhow::bail!("backup did not complete in one step: {other:?}"), + } Ok(()) }) .await @@ -93,6 +116,38 @@ pub async fn perform_backup(db: &Database, dir: &Path, keep_n: usize) -> anyhow: Ok(Some(dest)) } +/// Parse an env var, falling back to `default` when unset. A SET but +/// unparseable value warns instead of silently defaulting. +fn env_or_default(var: &str, default: T) -> T { + match std::env::var(var) { + Ok(s) => s.trim().parse().unwrap_or_else(|_| { + tracing::warn!(target: "backup", "{var}={s:?} is not a valid number — using default {default}"); + default + }), + Err(_) => default, + } +} + +/// True when the newest existing backup is younger than half the schedule +/// interval — a fresh tick then adds nothing but churn. Guards against +/// restart loops (cargo-watch, container crash loop): the immediate +/// first interval tick would otherwise write one backup per process start +/// and, with count-based pruning, wipe a week of history in minutes. +fn should_skip_backup(newest_age: Duration, interval: Duration) -> bool { + newest_age < interval / 2 +} + +/// Age (by mtime) of the newest scheduled backup in `dir`, if any. +fn newest_backup_age(dir: &Path) -> Option { + let newest = std::fs::read_dir(dir) + .ok()? + .filter_map(|e| e.ok()) + .filter(|e| e.file_name().to_str().map(is_backup_name).unwrap_or(false)) + .filter_map(|e| e.metadata().ok()?.modified().ok()) + .max()?; + newest.elapsed().ok() +} + /// Periodic backup task. Mirrors `learning_sweep`: tick on an interval, run one /// backup, log failures, never crash the loop. pub struct BackupScheduler { @@ -105,18 +160,12 @@ impl BackupScheduler { /// Build from env: `KRONN_BACKUP_INTERVAL_HOURS` (default 24, 0 disables), /// `KRONN_BACKUP_KEEP` (default 7). pub fn from_env(db: Arc) -> Option> { - let hours: u64 = std::env::var("KRONN_BACKUP_INTERVAL_HOURS") - .ok() - .and_then(|s| s.trim().parse().ok()) - .unwrap_or(24); + let hours: u64 = env_or_default("KRONN_BACKUP_INTERVAL_HOURS", 24); if hours == 0 { tracing::info!(target: "backup", "scheduled backups disabled (KRONN_BACKUP_INTERVAL_HOURS=0)"); return None; } - let keep_n: usize = std::env::var("KRONN_BACKUP_KEEP") - .ok() - .and_then(|s| s.trim().parse().ok()) - .unwrap_or(7); + let keep_n: usize = env_or_default("KRONN_BACKUP_KEEP", 7); Some(Arc::new(Self { db, interval: Duration::from_secs(hours * 3600), keep_n })) } @@ -134,6 +183,19 @@ impl BackupScheduler { let mut tick = tokio::time::interval(self.interval); loop { tick.tick().await; + // The first tick fires immediately (good: boot backup after long + // downtime) — but skip when a recent backup already exists, or a + // restart loop would prune the whole history in minutes. + if let Some(age) = newest_backup_age(&dir) { + if should_skip_backup(age, self.interval) { + tracing::debug!( + target: "backup", + "skipping scheduled backup: newest is {}s old (< interval/2)", + age.as_secs() + ); + continue; + } + } match perform_backup(&self.db, &dir, self.keep_n).await { Ok(Some(p)) => tracing::info!(target: "backup", "scheduled backup written: {}", p.display()), Ok(None) => {} @@ -146,8 +208,43 @@ impl BackupScheduler { #[cfg(test)] mod tests { use super::*; + use serial_test::serial; + + #[tokio::test] + async fn perform_backup_writes_a_readable_copy() { + // End-to-end through the REAL copy path. This is the test that was + // missing on 2026-07-09: `run_to_completion(-1, …)` type-checked but + // panicked at runtime (rusqlite asserts pages_per_step > 0), and the + // panic fired inside `with_conn` — poisoning the DB mutex at the + // boot backup tick and killing every later DB call in the process. + let tmp = tempfile::TempDir::new().unwrap(); + let db_path = tmp.path().join("kronn.db"); + let db = crate::db::Database::open_path(&db_path).expect("open db"); + let backup_dir = tmp.path().join("backups"); + + let written = perform_backup(&db, &backup_dir, 3) + .await + .expect("backup must not fail (a panic here poisons the DB mutex)") + .expect("file-backed DB → a backup file is written"); + assert!(written.exists()); + + // The copy is a valid SQLite DB with the migrated schema. + let copy = rusqlite::Connection::open(&written).unwrap(); + let n: i64 = copy + .query_row("SELECT count(*) FROM sqlite_master WHERE type='table'", [], |r| r.get(0)) + .unwrap(); + assert!(n > 0, "backup carries the schema ({n} tables)"); + + // And the source connection is still usable afterwards (not poisoned). + db.with_conn(|conn| { + conn.query_row("SELECT 1", [], |r| r.get::<_, i64>(0)).map_err(Into::into) + }) + .await + .expect("source DB usable after backup"); + } #[test] + #[serial] fn resolve_backup_dir_defaults_in_volume_then_env_external() { std::env::remove_var("KRONN_BACKUP_DIR"); let (dir, ext) = resolve_backup_dir(Path::new("/data")); @@ -167,6 +264,28 @@ mod tests { assert_eq!(backup_filename(ts), "kronn-auto-20260707-060504.db"); } + #[test] + fn should_skip_backup_only_when_newest_is_younger_than_half_interval() { + let day = Duration::from_secs(24 * 3600); + assert!(should_skip_backup(Duration::from_secs(60), day), "restart 1min after a backup → skip"); + assert!(should_skip_backup(day / 2 - Duration::from_secs(1), day)); + assert!(!should_skip_backup(day / 2, day), "at half the interval → back up"); + assert!(!should_skip_backup(day * 7, day), "boot after long downtime → back up"); + } + + #[test] + fn newest_backup_age_none_when_dir_empty_or_foreign_only() { + let tmp = std::env::temp_dir().join(format!("kronn-newest-{}", std::process::id())); + std::fs::create_dir_all(&tmp).unwrap(); + std::fs::write(tmp.join("important.db"), b"foreign").unwrap(); + assert!(newest_backup_age(&tmp).is_none(), "foreign files must not count as backups"); + + std::fs::write(tmp.join("kronn-auto-20260101-000000.db"), b"x").unwrap(); + let age = newest_backup_age(&tmp).expect("our backup must be seen"); + assert!(age < Duration::from_secs(60), "just-written backup must have ~zero age"); + std::fs::remove_dir_all(&tmp).ok(); + } + #[test] fn prune_keeps_n_most_recent_and_ignores_foreign_files() { let tmp = std::env::temp_dir().join(format!("kronn-prune-{}", std::process::id())); diff --git a/backend/src/core/config.rs b/backend/src/core/config.rs index 343e7e1b..2d949b36 100644 --- a/backend/src/core/config.rs +++ b/backend/src/core/config.rs @@ -101,7 +101,10 @@ pub async fn load() -> Result> { if needs_save { let updated = toml::to_string_pretty(&config) .context("Failed to serialize config")?; - tokio::fs::write(&path, updated).await?; + // Same atomic temp+fsync+rename path as save() — a plain fs::write + // here could truncate-then-fail and lose the encryption_secret + // (2026-06-30 incident class). + persist_atomic(config_dir()?, path, updated).await?; } Ok(Some(config)) @@ -123,19 +126,23 @@ pub async fn save(config: &AppConfig) -> Result<()> { .context("Failed to serialize config")?; let path = config_path()?; - // Atomic write on a blocking thread: fully-written temp (0600) → fsync → - // rename over the target (atomic on one filesystem) → fsync the dir. A crash - // or a concurrent reader never sees a half-written config, so the key/token - // fields can't be lost to a torn write. Replaces the old `fs::write`, which - // could truncate-then-fail and leave a corrupt config. - let dir_c = dir.clone(); - let path_c = path.clone(); - tokio::task::spawn_blocking(move || write_config_atomic(&dir_c, &path_c, content.as_bytes())) + persist_atomic(dir, path.clone(), content).await?; + + tracing::info!("Config saved to {}", path.display()); + Ok(()) +} + +/// Atomic write on a blocking thread: fully-written temp (0600) → fsync → +/// rename over the target (atomic on one filesystem) → fsync the dir. A crash +/// or a concurrent reader never sees a half-written config, so the key/token +/// fields can't be lost to a torn write. Replaces the old `fs::write`, which +/// could truncate-then-fail and leave a corrupt config. Shared by `save()` +/// and the migration re-save in `load()`. +async fn persist_atomic(dir: PathBuf, path: PathBuf, content: String) -> Result<()> { + tokio::task::spawn_blocking(move || write_config_atomic(&dir, &path, content.as_bytes())) .await .context("config write task panicked")? .context("atomic config write failed")?; - - tracing::info!("Config saved to {}", path.display()); Ok(()) } @@ -334,6 +341,7 @@ pub async fn is_first_run() -> Result { #[cfg(test)] mod tests { use super::*; + use serial_test::serial; /// Tests that mutate `KRONN_DATA_DIR` (a process-wide env var) must /// run serialized — parallel execution would have them stomp each @@ -453,6 +461,7 @@ mod tests { } #[test] + #[serial] // lock path derives from KRONN_DATA_DIR — races the env-mutating tests fn data_dir_lock_is_exclusive() { let dir = scratch_dir("lock"); let g1 = acquire_lock_in(&dir).expect("first exclusive lock must succeed"); @@ -469,6 +478,7 @@ mod tests { /// regeneration is exactly what orphaned every secret in the 2026-06-30 /// incident. The DB-aware reconciler owns key resolution now. #[tokio::test] + #[serial] async fn load_does_not_regenerate_a_missing_encryption_secret() { let _lock = ENV_LOCK.lock().await; let tmp = scratch_dir("noregen"); @@ -490,6 +500,7 @@ mod tests { } #[tokio::test] + #[serial] async fn load_keeps_an_existing_encryption_secret() { let _lock = ENV_LOCK.lock().await; let tmp = scratch_dir("keepsecret"); @@ -514,6 +525,7 @@ mod tests { /// Legacy single-key fields (`tokens.anthropic/openai/google`) must migrate /// into the multi-key `keys[]` on load, and the legacy fields get cleared. #[tokio::test] + #[serial] async fn load_migrates_legacy_single_keys_to_multikey() { let _lock = ENV_LOCK.lock().await; let tmp = scratch_dir("legacymig"); @@ -543,6 +555,7 @@ mod tests { /// Atomic rename guarantees a concurrent flurry of `save()` never yields a /// torn, unparseable config — `load()` always parses cleanly. #[tokio::test] + #[serial] async fn concurrent_saves_never_produce_a_torn_config() { let _lock = ENV_LOCK.lock().await; let tmp = scratch_dir("concurrent"); @@ -574,6 +587,7 @@ mod tests { /// persists — a backend restart must NOT reset it. Also checks /// secret_themes round-trips (operator-local plaintext overrides). #[tokio::test] + #[serial] async fn secret_theme_fields_survive_save_and_reload() { let _lock = ENV_LOCK.lock().await; let tmp = std::env::temp_dir().join(format!( @@ -615,6 +629,7 @@ mod tests { /// the default is OFF — this is the macOS-Docker 401 fix: a generated token /// must NOT silently turn auth on and lock the user out on first launch. #[tokio::test] + #[serial] async fn load_autogenerates_token_and_defaults_auth_enabled_to_platform() { let _lock = ENV_LOCK.lock().await; let tmp = std::env::temp_dir().join(format!( @@ -635,12 +650,14 @@ mod tests { cfg.server.auth_enabled = true; save(&cfg).await.expect("save must succeed"); + // Docker mode is now the explicit KRONN_IN_DOCKER marker (KRONN_DATA_DIR + // alone only relocates data — a native user can set it too). + std::env::set_var("KRONN_IN_DOCKER", "1"); let loaded = load().await.expect("load Ok").expect("Some after save"); assert!( loaded.server.auth_token.is_some(), "load() must auto-generate an auth token when none is set" ); - // KRONN_DATA_DIR is set here → is_docker() == true → auth_on_by_default() == false. assert!( !loaded.server.auth_enabled, "auth must default OFF under Docker even though the saved value was true \ @@ -648,12 +665,14 @@ mod tests { crate::core::env::auth_on_by_default() ); + std::env::remove_var("KRONN_IN_DOCKER"); std::env::remove_var("KRONN_DATA_DIR"); let _ = std::fs::remove_dir_all(&tmp); } #[cfg(unix)] #[tokio::test] + #[serial] async fn save_chmods_dir_700_and_file_600_on_unix() { let _lock = ENV_LOCK.lock().await; // Real save() round-trip via KRONN_DATA_DIR override so we don't @@ -691,6 +710,7 @@ mod tests { } #[tokio::test] + #[serial] async fn load_returns_none_when_no_config_file() { let _lock = ENV_LOCK.lock().await; let tmp = std::env::temp_dir().join(format!( @@ -712,6 +732,7 @@ mod tests { } #[tokio::test] + #[serial] async fn is_first_run_true_before_any_save() { let _lock = ENV_LOCK.lock().await; let tmp = std::env::temp_dir().join(format!( @@ -739,6 +760,7 @@ mod tests { } #[tokio::test] + #[serial] async fn save_then_load_preserves_default_scan_ignores() { let _lock = ENV_LOCK.lock().await; let tmp = std::env::temp_dir().join(format!( @@ -773,6 +795,7 @@ mod tests { } #[tokio::test] + #[serial] async fn save_preserves_anti_hallucination_mode_and_default_tier() { // 0.8.7 + 0.8.6 fields that should NEVER be dropped on roundtrip. let _lock = ENV_LOCK.lock().await; diff --git a/backend/src/core/env.rs b/backend/src/core/env.rs index 56fae1d2..d256b55d 100644 --- a/backend/src/core/env.rs +++ b/backend/src/core/env.rs @@ -4,9 +4,18 @@ //! All checks are runtime (env vars), not compile-time — the same binary works in both contexts. /// Detect if running inside a Docker container. -/// Docker is indicated by `KRONN_DATA_DIR` (set by docker-compose). +/// +/// Checks the explicit `KRONN_IN_DOCKER` marker (set by our Dockerfile / +/// docker-compose) plus the container runtimes' own markers, which also cover +/// images launched from an older compose file. Deliberately NOT keyed on +/// `KRONN_DATA_DIR`: that is a generic data-dir override a NATIVE user can set +/// too, and "docker" downgrades security posture (auth off by default, the +/// LAN-bind boot guard trusts KRONN_BIND instead of the real bind host) — a +/// false positive there is fail-open. pub fn is_docker() -> bool { - std::env::var("KRONN_DATA_DIR").is_ok() + std::env::var("KRONN_IN_DOCKER").is_ok() + || std::path::Path::new("/.dockerenv").exists() + || std::path::Path::new("/run/.containerenv").exists() } /// Whether API auth should be ENABLED by default when a fresh config first @@ -65,38 +74,55 @@ mod tests { #[test] #[serial] - fn is_docker_true_when_data_dir_set() { - let old = std::env::var("KRONN_DATA_DIR").ok(); - std::env::set_var("KRONN_DATA_DIR", "/data"); + fn is_docker_true_when_marker_set() { + let old = std::env::var("KRONN_IN_DOCKER").ok(); + std::env::set_var("KRONN_IN_DOCKER", "1"); assert!(is_docker()); - if let Some(v) = old { std::env::set_var("KRONN_DATA_DIR", v); } else { std::env::remove_var("KRONN_DATA_DIR"); } + if let Some(v) = old { std::env::set_var("KRONN_IN_DOCKER", v); } else { std::env::remove_var("KRONN_IN_DOCKER"); } } #[test] #[serial] - fn is_docker_false_when_data_dir_unset() { - let old = std::env::var("KRONN_DATA_DIR").ok(); - std::env::remove_var("KRONN_DATA_DIR"); - assert!(!is_docker()); - if let Some(v) = old { std::env::set_var("KRONN_DATA_DIR", v); } + fn is_docker_ignores_data_dir_override() { + // KRONN_DATA_DIR is a generic data-relocation knob a NATIVE user can + // set; it must NOT flip docker mode (fail-open on the LAN guard). + let old_marker = std::env::var("KRONN_IN_DOCKER").ok(); + let old_data = std::env::var("KRONN_DATA_DIR").ok(); + std::env::remove_var("KRONN_IN_DOCKER"); + std::env::set_var("KRONN_DATA_DIR", "/tmp/relocated"); + // (skip on machines actually running tests inside a container) + if !std::path::Path::new("/.dockerenv").exists() + && !std::path::Path::new("/run/.containerenv").exists() + { + assert!(!is_docker(), "data-dir override alone must not mean docker"); + } + if let Some(v) = old_marker { std::env::set_var("KRONN_IN_DOCKER", v); } + match old_data { + Some(v) => std::env::set_var("KRONN_DATA_DIR", v), + None => std::env::remove_var("KRONN_DATA_DIR"), + } } #[test] #[serial] fn auth_off_by_default_under_docker() { - let old = std::env::var("KRONN_DATA_DIR").ok(); - std::env::set_var("KRONN_DATA_DIR", "/data"); + let old = std::env::var("KRONN_IN_DOCKER").ok(); + std::env::set_var("KRONN_IN_DOCKER", "1"); assert!(!auth_on_by_default(), "Docker → auth must default OFF (localhost bypass can't see the real client behind NAT)"); - if let Some(v) = old { std::env::set_var("KRONN_DATA_DIR", v); } else { std::env::remove_var("KRONN_DATA_DIR"); } + if let Some(v) = old { std::env::set_var("KRONN_IN_DOCKER", v); } else { std::env::remove_var("KRONN_IN_DOCKER"); } } #[test] #[serial] fn auth_on_by_default_when_native() { - let old = std::env::var("KRONN_DATA_DIR").ok(); - std::env::remove_var("KRONN_DATA_DIR"); - assert!(auth_on_by_default(), "Native (Tauri/CLI) → auth defaults ON; localhost bypass keeps it transparent"); - if let Some(v) = old { std::env::set_var("KRONN_DATA_DIR", v); } + let old = std::env::var("KRONN_IN_DOCKER").ok(); + std::env::remove_var("KRONN_IN_DOCKER"); + if !std::path::Path::new("/.dockerenv").exists() + && !std::path::Path::new("/run/.containerenv").exists() + { + assert!(auth_on_by_default(), "Native (Tauri/CLI) → auth defaults ON; localhost bypass keeps it transparent"); + } + if let Some(v) = old { std::env::set_var("KRONN_IN_DOCKER", v); } } #[test] diff --git a/backend/src/core/host_mcp_discovery.rs b/backend/src/core/host_mcp_discovery.rs index 52383438..b92245d0 100644 --- a/backend/src/core/host_mcp_discovery.rs +++ b/backend/src/core/host_mcp_discovery.rs @@ -385,7 +385,8 @@ fn build_from_json_entry( let transport = if let Some(cmd) = command { McpTransport::Stdio { command: cmd, args } - } else if let Some(u) = http_url.or_else(|| url.clone()) { + } else { + let u = http_url.or_else(|| url.clone())?; // Gemini uses `httpUrl` for Streamable; Claude uses `type: "http"`. // We map to Streamable when `type` is `http` or `httpUrl` is set; // otherwise default to SSE (legacy default). @@ -394,8 +395,6 @@ fn build_from_json_entry( } else { McpTransport::Sse { url: u } } - } else { - return None; // No transport — skip silently }; let env_keys: Vec = env_map.keys().cloned().collect(); diff --git a/backend/src/core/key_discovery.rs b/backend/src/core/key_discovery.rs index 5d54f121..2ea0bee1 100644 --- a/backend/src/core/key_discovery.rs +++ b/backend/src/core/key_discovery.rs @@ -171,11 +171,30 @@ pub fn write_gemini_key(key: Option<&str>) { match key { Some(k) => { let _ = std::fs::create_dir_all(&gemini_dir); - // Read existing settings to preserve other fields - let mut settings: serde_json::Value = std::fs::read_to_string(&settings_path) - .ok() - .and_then(|c| serde_json::from_str(&c).ok()) - .unwrap_or_else(|| serde_json::json!({})); + // Read existing settings to preserve other fields. A MISSING file + // starts from {}, but an unreadable/corrupt one must abort — + // rewriting would replace the user's whole Gemini config + // (mcpServers, theme, …) with just apiKey. + let mut settings = match std::fs::read_to_string(&settings_path) { + Ok(c) => match serde_json::from_str::(&c) { + Ok(v) => v, + Err(e) => { + tracing::warn!( + "Not syncing Google key: {} is not valid JSON ({e}); refusing to overwrite it", + settings_path.display() + ); + return; + } + }, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => serde_json::json!({}), + Err(e) => { + tracing::warn!( + "Not syncing Google key: cannot read {}: {e}; refusing to overwrite it", + settings_path.display() + ); + return; + } + }; settings["apiKey"] = serde_json::Value::String(k.to_string()); match std::fs::write(&settings_path, serde_json::to_string_pretty(&settings).unwrap()) { Ok(_) => tracing::info!("Synced Google key to {}", settings_path.display()), @@ -188,8 +207,10 @@ pub fn write_gemini_key(key: Option<&str>) { if let Ok(mut settings) = serde_json::from_str::(&content) { if let Some(obj) = settings.as_object_mut() { obj.remove("apiKey"); - let _ = std::fs::write(&settings_path, serde_json::to_string_pretty(&settings).unwrap()); - tracing::info!("Removed Google key from {}", settings_path.display()); + match std::fs::write(&settings_path, serde_json::to_string_pretty(&settings).unwrap()) { + Ok(_) => tracing::info!("Removed Google key from {}", settings_path.display()), + Err(e) => tracing::warn!("Failed to remove Google key from {}: {e}", settings_path.display()), + } } } } @@ -327,6 +348,69 @@ mod tests { assert!(keys.len() <= 10); } + // ─── write_gemini_key ──────────────────────────────────────────────────── + + #[test] + #[serial] + fn write_gemini_key_starts_from_empty_when_file_missing() { + let tmp = std::env::temp_dir().join("kronn-test-gemini-write-missing"); + let _ = std::fs::remove_dir_all(&tmp); + std::fs::create_dir_all(&tmp).unwrap(); + + let old_home = std::env::var("HOME").ok(); + std::env::set_var("HOME", &tmp); + write_gemini_key(Some("AIza-new")); + if let Some(h) = old_home { std::env::set_var("HOME", h); } + + let content = std::fs::read_to_string(tmp.join(".gemini/settings.json")).unwrap(); + let v: serde_json::Value = serde_json::from_str(&content).unwrap(); + assert_eq!(v["apiKey"].as_str(), Some("AIza-new")); + let _ = std::fs::remove_dir_all(&tmp); + } + + #[test] + #[serial] + fn write_gemini_key_preserves_other_fields() { + let tmp = std::env::temp_dir().join("kronn-test-gemini-write-preserve"); + let _ = std::fs::remove_dir_all(&tmp); + std::fs::create_dir_all(tmp.join(".gemini")).unwrap(); + std::fs::write( + tmp.join(".gemini/settings.json"), + r#"{"apiKey":"old","theme":"dark","mcpServers":{"foo":{}}}"#, + ).unwrap(); + + let old_home = std::env::var("HOME").ok(); + std::env::set_var("HOME", &tmp); + write_gemini_key(Some("AIza-new")); + if let Some(h) = old_home { std::env::set_var("HOME", h); } + + let content = std::fs::read_to_string(tmp.join(".gemini/settings.json")).unwrap(); + let v: serde_json::Value = serde_json::from_str(&content).unwrap(); + assert_eq!(v["apiKey"].as_str(), Some("AIza-new")); + assert_eq!(v["theme"].as_str(), Some("dark"), "user settings must survive the sync"); + assert!(v["mcpServers"]["foo"].is_object(), "mcpServers must survive the sync"); + let _ = std::fs::remove_dir_all(&tmp); + } + + #[test] + #[serial] + fn write_gemini_key_refuses_to_overwrite_corrupt_settings() { + let tmp = std::env::temp_dir().join("kronn-test-gemini-write-corrupt"); + let _ = std::fs::remove_dir_all(&tmp); + std::fs::create_dir_all(tmp.join(".gemini")).unwrap(); + let corrupt = "{ not json — user's mcpServers live here"; + std::fs::write(tmp.join(".gemini/settings.json"), corrupt).unwrap(); + + let old_home = std::env::var("HOME").ok(); + std::env::set_var("HOME", &tmp); + write_gemini_key(Some("AIza-new")); + if let Some(h) = old_home { std::env::set_var("HOME", h); } + + let content = std::fs::read_to_string(tmp.join(".gemini/settings.json")).unwrap(); + assert_eq!(content, corrupt, "a corrupt settings.json must be left untouched, never replaced by {{apiKey}}"); + let _ = std::fs::remove_dir_all(&tmp); + } + // ─── Cross-platform: home_dir resolution ──────────────────────────────── #[test] diff --git a/backend/src/core/kronn_state.rs b/backend/src/core/kronn_state.rs index 915a1d27..d01cbe61 100644 --- a/backend/src/core/kronn_state.rs +++ b/backend/src/core/kronn_state.rs @@ -78,9 +78,32 @@ fn state_path(project_path: &Path) -> std::path::PathBuf { /// Read `docs/.kronn.json` if present and parseable. Any I/O or JSON error /// returns `None` — callers fall back to legacy detection paths. pub fn read(project_path: &Path) -> Option { + read_for_mutation(project_path).ok().flatten() +} + +/// Like `read`, but distinguishes a MISSING file (`Ok(None)` — mutators may +/// start from default) from an unreadable/corrupt one (`Err` — mutators must +/// abort rather than rebuild from default and clobber the existing audit +/// history: `bootstrapped_at`, `validated_at`, audits). +fn read_for_mutation(project_path: &Path) -> Result, String> { let path = state_path(project_path); - let data = std::fs::read_to_string(&path).ok()?; - serde_json::from_str(&data).ok() + match std::fs::read_to_string(&path) { + Ok(data) => serde_json::from_str(&data) + .map(Some) + .map_err(|e| format!("{} exists but is not valid JSON: {e}", path.display())), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(None), + Err(e) => Err(format!("Cannot read {}: {e}", path.display())), + } +} + +/// Load state for a mutation: default for a missing file, `Err` (with a warn) +/// for an unreadable/corrupt one so the caller aborts instead of clobbering. +fn load_for_mutation(project_path: &Path) -> Result { + read_for_mutation(project_path) + .map(Option::unwrap_or_default) + .inspect_err(|e| { + tracing::warn!("Refusing to rewrite Kronn state from default: {e}"); + }) } /// Atomic-ish write of `docs/.kronn.json`. Always rewrites the `_readme` @@ -113,7 +136,7 @@ fn kronn_version() -> String { /// engine itself decides whether to call this (e.g. once per successful /// full/partial run). pub fn record_audit(project_path: &Path, audit_type: &str) -> Result<(), String> { - let mut state = read(project_path).unwrap_or_default(); + let mut state = load_for_mutation(project_path)?; state.audits.push(AuditEntry { date: today_iso(), kronn_version: kronn_version(), @@ -124,7 +147,7 @@ pub fn record_audit(project_path: &Path, audit_type: &str) -> Result<(), String> /// Set `validated_at`. No-op if already set (preserves the original date). pub fn mark_validated(project_path: &Path) -> Result<(), String> { - let mut state = read(project_path).unwrap_or_default(); + let mut state = load_for_mutation(project_path)?; if state.validated_at.is_none() { state.validated_at = Some(today_iso()); } @@ -133,7 +156,7 @@ pub fn mark_validated(project_path: &Path) -> Result<(), String> { /// Set `bootstrapped_at`. No-op if already set. pub fn mark_bootstrapped(project_path: &Path) -> Result<(), String> { - let mut state = read(project_path).unwrap_or_default(); + let mut state = load_for_mutation(project_path)?; if state.bootstrapped_at.is_none() { state.bootstrapped_at = Some(today_iso()); } @@ -164,9 +187,15 @@ pub fn mark_bootstrapped(project_path: &Path) -> Result<(), String> { /// skipped. Write errors propagate as `Err(String)` — caller decides /// whether to log + fall through to legacy detection (read-only FS, etc.). pub fn backfill_from_legacy_state(project_path: &Path) -> Result { - // Skip if already present — backfill is one-shot. - if read(project_path).is_some() { - return Ok(false); + // Skip if already present — backfill is one-shot. A corrupt/unreadable + // file also skips: overwriting it would destroy the real audit history. + match read_for_mutation(project_path) { + Ok(Some(_)) => return Ok(false), + Ok(None) => {} + Err(e) => { + tracing::warn!("Skipping legacy backfill: {e}"); + return Ok(false); + } } let has_checksums = diff --git a/backend/src/core/kronn_state_test.rs b/backend/src/core/kronn_state_test.rs index 6ace74fe..83023350 100644 --- a/backend/src/core/kronn_state_test.rs +++ b/backend/src/core/kronn_state_test.rs @@ -26,6 +26,39 @@ fn read_malformed_returns_none() { cleanup(&tmp); } +#[test] +fn mutators_abort_on_corrupt_state_instead_of_clobbering() { + let tmp = fresh_tmp("corrupt-no-clobber"); + let corrupt = "{ \"audits\": [ …truncated by a torn write"; + std::fs::write(tmp.join("docs/.kronn.json"), corrupt).unwrap(); + + assert!(record_audit(&tmp, "full").is_err(), "record_audit must abort on corrupt state"); + assert!(mark_validated(&tmp).is_err(), "mark_validated must abort on corrupt state"); + assert!(mark_bootstrapped(&tmp).is_err(), "mark_bootstrapped must abort on corrupt state"); + + let content = std::fs::read_to_string(tmp.join("docs/.kronn.json")).unwrap(); + assert_eq!(content, corrupt, "a corrupt .kronn.json must never be rebuilt from default"); + cleanup(&tmp); +} + +#[test] +fn backfill_skips_when_state_file_is_corrupt() { + let tmp = fresh_tmp("backfill-skip-corrupt"); + let corrupt = "{ not json"; + std::fs::write(tmp.join("docs/.kronn.json"), corrupt).unwrap(); + // A legacy signal that WOULD trigger backfill on a fresh project. + std::fs::write( + tmp.join("docs/checksums.json"), + r#"{"audited_at": "2026-01-01", "mappings": []}"#, + ).unwrap(); + + let did = backfill_from_legacy_state(&tmp).unwrap(); + assert!(!did, "backfill must not overwrite a corrupt .kronn.json"); + let content = std::fs::read_to_string(tmp.join("docs/.kronn.json")).unwrap(); + assert_eq!(content, corrupt); + cleanup(&tmp); +} + #[test] fn record_audit_creates_file_with_readme() { let tmp = fresh_tmp("record-creates"); diff --git a/backend/src/core/mcp_scanner.rs b/backend/src/core/mcp_scanner.rs index d13bb654..3867ad6c 100644 --- a/backend/src/core/mcp_scanner.rs +++ b/backend/src/core/mcp_scanner.rs @@ -521,6 +521,59 @@ pub(crate) fn sync_claude_enabled_servers(project_path: &str, mcp_servers: &Hash /// Write a .mcp.json to `target_dir` with all MCP configs that have `include_general` set. /// Used for general discussions (no project) so agents still have access to global MCPs. +/// Decrypt a config's env for writing into an on-disk agent config file. +/// +/// `Err` = the config EXPECTS secrets (`env_keys` non-empty) but decryption +/// failed — the caller MUST abort the whole file write, leaving the existing +/// on-disk file untouched. Both alternatives clobber previously-good secrets +/// (the 2026-06-30 incident class): writing the entry with `env: {}` strips +/// them, and skipping just the entry deletes it from the regenerated file. +/// A config with no expected keys degrades to an empty map. +pub(crate) fn decrypt_env_strict( + config: &crate::models::McpConfig, + secret: &str, +) -> Result, String> { + match crate::db::mcps::decrypt_env(&config.env_encrypted, secret) { + Ok(e) => Ok(e), + Err(_) if config.env_keys.is_empty() => Ok(HashMap::new()), + Err(e) => { + let msg = format!( + "MCP '{}': {} env key(s) configured but decryption failed ({}) — ABORTING this \ + config-file write so existing on-disk secrets are not clobbered. Fix the \ + encryption key (Settings → Security) or re-enter the MCP's env values.", + config.label, + config.env_keys.len(), + e + ); + tracing::error!("{msg}"); + Err(msg) + } + } +} + +/// Availability of `command` for a HOST CLI. Codex/Copilot read their global +/// config on the host — natively the backend PATH IS the host PATH, but under +/// Docker it isn't: check the mounted host bins (KRONN_HOST_BIN), accepting +/// symlink entries (brew Cellar links dangle inside the container — see +/// rtk_detect). Absolute paths and an unset KRONN_HOST_BIN are unverifiable +/// from the container → keep the entry (a startup warning on the host beats +/// silently dropping a working MCP). +pub(crate) fn host_mcp_command_available(command: &str) -> bool { + if !crate::core::env::is_docker() { + return is_command_available(command); + } + if command.starts_with('/') || command.starts_with('.') { + return true; + } + match std::env::var("KRONN_HOST_BIN") { + Ok(hb) => std::env::split_paths(&hb).any(|dir| { + let p = dir.join(command); + p.exists() || p.symlink_metadata().is_ok() + }), + Err(_) => true, + } +} + pub fn write_general_mcp_json( conn: &rusqlite::Connection, secret: &str, @@ -546,7 +599,7 @@ pub fn write_general_mcp_json( Some(s) => s, None => continue, }; - let env = db::mcps::decrypt_env(&config.env_encrypted, secret).unwrap_or_default(); + let env = decrypt_env_strict(config, secret)?; let entry = match &server.transport { McpTransport::Stdio { command, args } => { @@ -678,19 +731,10 @@ pub fn sync_project_mcps_to_disk( continue; } - // Decrypt env — skip MCP if decryption fails and keys are expected - let env = match db::mcps::decrypt_env(&config.env_encrypted, secret) { - Ok(e) => e, - Err(e) => { - if !config.env_keys.is_empty() { - tracing::warn!( - "MCP '{}' has {} env keys but decryption failed ({}) — writing without secrets", - config.label, config.env_keys.len(), e - ); - } - HashMap::new() - } - }; + // Decrypt env — a failure with expected keys ABORTS the whole sync: + // both "write with empty env" and "drop the entry from the regenerated + // file" clobber the good secrets already on disk (2026-06-30 class). + let env = decrypt_env_strict(config, secret)?; let entry = match &server.transport { McpTransport::Stdio { command, args } => { @@ -958,8 +1002,11 @@ fn sync_vibe_project_config( None => continue, }; - let env = crate::db::mcps::decrypt_env(&config.env_encrypted, secret) - .unwrap_or_default(); + let env = match decrypt_env_strict(config, secret) { + Ok(e) => e, + // Abort the whole Vibe write — see decrypt_env_strict. + Err(_) => return, + }; let name = config.label.clone(); @@ -1185,8 +1232,21 @@ impl HostMcpSync for CodexSync { continue; } }; - let env = db::mcps::decrypt_env(&config.env_encrypted, secret) - .unwrap_or_default(); + // Parity with the .mcp.json writer: a dead entry fails Codex's + // startup with "No such file or directory" on every spawn. + // Host-aware: this file is read by the HOST CLI, not the backend. + if !host_mcp_command_available(&command) { + tracing::warn!( + "MCP '{}' skipped for Codex: `{}` is not installed on this host — install it (e.g. `brew install uv` for uvx) then resync", + config.label, command + ); + continue; + } + let env = match decrypt_env_strict(config, secret) { + Ok(e) => e, + // Abort the whole Codex sync plan — see decrypt_env_strict. + Err(_) => return None, + }; // Codex requires names matching ^[a-zA-Z0-9_-]+$ — slugify let raw_key = config.label.clone(); let key = slugify_label(&raw_key); @@ -1320,8 +1380,19 @@ impl HostMcpSync for CopilotSync { } }; - let env = db::mcps::decrypt_env(&config.env_encrypted, secret) - .unwrap_or_default(); + // Same host-aware availability parity as the Codex writer above. + if !host_mcp_command_available(&command) { + tracing::warn!( + "MCP '{}' skipped for Copilot: `{}` is not installed on this host — install it then resync", + config.label, command + ); + continue; + } + let env = match decrypt_env_strict(config, secret) { + Ok(e) => e, + // Abort the whole Copilot sync plan — see decrypt_env_strict. + Err(_) => return None, + }; let key = config.label.clone(); mcp_servers.insert(key, McpServerEntry { @@ -1548,10 +1619,11 @@ fn build_kronn_managed_json_entry( server: &crate::models::McpServer, secret: &str, use_http_url_for_streamable: bool, -) -> Option { +) -> Result, String> { use crate::models::McpTransport; - let env = crate::db::mcps::decrypt_env(&config.env_encrypted, secret) - .unwrap_or_default(); + // Err = decrypt failure with expected keys → the caller must abort its + // whole host-config write (see decrypt_env_strict). + let env = decrypt_env_strict(config, secret)?; let mut obj = serde_json::Map::new(); match &server.transport { @@ -1582,7 +1654,7 @@ fn build_kronn_managed_json_entry( obj.insert("url".into(), serde_json::Value::String(url.clone())); } } - McpTransport::ApiOnly => return None, + McpTransport::ApiOnly => return Ok(None), } let mut marker = serde_json::Map::new(); @@ -1590,7 +1662,7 @@ fn build_kronn_managed_json_entry( marker.insert("config_id".into(), serde_json::Value::String(config.id.clone())); obj.insert("_kronn".into(), serde_json::Value::Object(marker)); - Some(serde_json::Value::Object(obj)) + Ok(Some(serde_json::Value::Object(obj))) } /// Outcome of attempting to load+merge a JSON host config (Claude/Gemini/Copilot). @@ -1786,8 +1858,11 @@ impl HostMcpSync for ClaudeSync { if !should_host_sync(config) { continue; } let server = match server_map.get(&config.server_id) { Some(s) => s, None => continue }; let entry = match build_kronn_managed_json_entry(config, server, secret, false) { - Some(e) => e, - None => continue, // ApiOnly skipped + Ok(Some(e)) => e, + Ok(None) => continue, // ApiOnly skipped + // Decrypt failure: abort the whole Claude host sync so the + // existing on-disk secrets are preserved (already logged). + Err(_) => return None, }; all_managed_ids.insert(config.id.clone()); @@ -2077,9 +2152,14 @@ impl HostMcpSync for GeminiSync { if !should_host_sync(config) { continue; } let server = match server_map.get(&config.server_id) { Some(s) => s, None => continue }; // Gemini: use `httpUrl` for Streamable HTTP. - if let Some(entry) = build_kronn_managed_json_entry(config, server, secret, true) { - kronn_entries.insert(config.label.clone(), entry); - kronn_config_ids.insert(config.id.clone()); + match build_kronn_managed_json_entry(config, server, secret, true) { + Ok(Some(entry)) => { + kronn_entries.insert(config.label.clone(), entry); + kronn_config_ids.insert(config.id.clone()); + } + Ok(None) => {} // ApiOnly skipped + // Decrypt failure: abort the whole Gemini host sync (logged). + Err(_) => return None, } } @@ -2807,6 +2887,7 @@ fn resolve_host_path(path: &str) -> String { #[cfg(test)] mod host_sync_tests { use super::*; + use serial_test::serial; use std::collections::HashSet; fn kronn_entry(config_id: &str, command: &str) -> serde_json::Value { @@ -3032,7 +3113,36 @@ mod host_sync_tests { source: McpSource::Registry, api_spec: None, }; - assert!(build_kronn_managed_json_entry(&config, &server, "secret-not-used", false).is_none()); + assert!(build_kronn_managed_json_entry(&config, &server, "secret-not-used", false).unwrap().is_none()); + } + + #[test] + fn decrypt_env_strict_aborts_on_undecryptable_expected_keys() { + use crate::models::{HostSyncMode, McpConfig}; + // Secrets EXPECTED + garbage ciphertext → Err: the caller must abort + // its file write instead of clobbering on-disk secrets (2026-06-30 + // incident class — the old code wrote env:{} silently). + let mut config = McpConfig { + id: "u1".into(), server_id: "s1".into(), label: "linear".into(), + env_keys: vec!["API_TOKEN".into()], + env_encrypted: "not-base64-not-ciphertext".into(), + args_override: None, is_global: false, include_general: true, + config_hash: String::new(), project_ids: vec![], + host_sync: HostSyncMode::GlobalOnly, + }; + assert!(decrypt_env_strict(&config, "0123456789abcdef0123456789abcdef").is_err()); + // No expected keys → same garbage degrades to an empty map (nothing to lose). + config.env_keys = vec![]; + assert_eq!(decrypt_env_strict(&config, "0123456789abcdef0123456789abcdef").unwrap(), HashMap::new()); + // And the builder propagates the abort signal. + config.env_keys = vec!["API_TOKEN".into()]; + let server = crate::models::McpServer { + id: "s1".into(), name: "Linear".into(), description: String::new(), + transport: crate::models::McpTransport::Stdio { command: "npx".into(), args: vec![] }, + source: crate::models::McpSource::Registry, + api_spec: None, + }; + assert!(build_kronn_managed_json_entry(&config, &server, "0123456789abcdef0123456789abcdef", false).is_err()); } #[test] @@ -3051,7 +3161,7 @@ mod host_sync_tests { source: McpSource::Registry, api_spec: None, }; - let entry = build_kronn_managed_json_entry(&config, &server, "secret", false).unwrap(); + let entry = build_kronn_managed_json_entry(&config, &server, "secret", false).unwrap().unwrap(); let marker = entry.get("_kronn").unwrap(); assert_eq!(marker.get("managed").unwrap().as_bool(), Some(true)); assert_eq!(marker.get("config_id").unwrap().as_str(), Some("uuid-marker")); @@ -3169,6 +3279,7 @@ mod host_sync_tests { } #[test] + #[serial] fn inject_kronn_internal_path_resolution() { // Pin the user-reported bug 2026-05-10: with KRONN_INTROSPECTION_PUBLIC_PATH // unset AND running in Docker (= `/app/scripts/...` only), Kronn used to @@ -3354,12 +3465,12 @@ mod host_sync_tests { api_spec: None, }; // Gemini convention - let gemini_entry = build_kronn_managed_json_entry(&config, &server, "s", true).unwrap(); + let gemini_entry = build_kronn_managed_json_entry(&config, &server, "s", true).unwrap().unwrap(); assert_eq!(gemini_entry.get("httpUrl").unwrap().as_str(), Some("https://example.com/mcp")); assert!(gemini_entry.get("type").is_none()); // Claude convention (type:"http" + url) - let claude_entry = build_kronn_managed_json_entry(&config, &server, "s", false).unwrap(); + let claude_entry = build_kronn_managed_json_entry(&config, &server, "s", false).unwrap().unwrap(); assert_eq!(claude_entry.get("type").unwrap().as_str(), Some("http")); assert_eq!(claude_entry.get("url").unwrap().as_str(), Some("https://example.com/mcp")); } diff --git a/backend/src/core/mcp_scanner_test.rs b/backend/src/core/mcp_scanner_test.rs index 053c36b8..c19656ca 100644 --- a/backend/src/core/mcp_scanner_test.rs +++ b/backend/src/core/mcp_scanner_test.rs @@ -1795,6 +1795,174 @@ args = ["@example/old-mcp"] // so we can drive it from this test module. use crate::core::mcp_scanner::{CodexSync, CopilotSync, HostMcpSync}; + #[test] + #[serial] + fn host_mcp_command_available_respects_the_host_container_boundary() { + // Codex review (2026-07-12): the host writers must judge availability + // where the HOST CLI runs, not in the backend's PATH. + use crate::core::mcp_scanner::host_mcp_command_available; + let tmp = tempfile::TempDir::new().unwrap(); + let prev_docker = std::env::var("KRONN_IN_DOCKER").ok(); + let prev_hb = std::env::var("KRONN_HOST_BIN").ok(); + + // Native: delegates to the backend-PATH check. + std::env::remove_var("KRONN_IN_DOCKER"); + if !std::path::Path::new("/.dockerenv").exists() + && !std::path::Path::new("/run/.containerenv").exists() + { + assert!(!host_mcp_command_available("definitely-not-a-binary-kronn-test")); + assert!(host_mcp_command_available("sh")); + } + + // Docker + host bin present as a DANGLING symlink (brew Cellar) → kept. + std::env::set_var("KRONN_IN_DOCKER", "1"); + std::env::set_var("KRONN_HOST_BIN", tmp.path()); + std::os::unix::fs::symlink("/no/such/Cellar/uv/bin/uvx", tmp.path().join("uvx")).unwrap(); + assert!(host_mcp_command_available("uvx"), "host symlink entry counts even when dangling in-container"); + // Docker + host bins mounted but binary absent → skip. + assert!(!host_mcp_command_available("glab"), "absent from host bins → dead entry"); + // Docker + absolute path → unverifiable from the container → keep. + assert!(host_mcp_command_available("/usr/local/bin/uvx")); + // Docker without a host-bin mount → keep (never drop what may work). + std::env::remove_var("KRONN_HOST_BIN"); + assert!(host_mcp_command_available("uvx")); + + match prev_docker { + Some(v) => std::env::set_var("KRONN_IN_DOCKER", v), + None => std::env::remove_var("KRONN_IN_DOCKER"), + } + match prev_hb { + Some(v) => std::env::set_var("KRONN_HOST_BIN", v), + None => std::env::remove_var("KRONN_HOST_BIN"), + } + } + + #[test] + #[serial] + fn docker_host_bin_uvx_survives_the_codex_plan() { + // Codex blocker test, verbatim: KRONN_HOST_BIN carries `uvx` (dangling + // symlink, the brew shape), backend PATH does not → entry KEPT. + let tmp = setup_tmp("codex-hostbin-keep"); + let home = tmp.join("fake-home"); + let hostbin = tmp.join("host-bin"); + std::fs::create_dir_all(&home).unwrap(); + std::fs::create_dir_all(&hostbin).unwrap(); + std::os::unix::fs::symlink("/no/such/Cellar/uv/bin/uvx", hostbin.join("uvx")).unwrap(); + + let prev_home = std::env::var("KRONN_HOST_HOME").ok(); + let prev_docker = std::env::var("KRONN_IN_DOCKER").ok(); + let prev_hb = std::env::var("KRONN_HOST_BIN").ok(); + std::env::set_var("KRONN_HOST_HOME", home.to_string_lossy().to_string()); + std::env::set_var("KRONN_IN_DOCKER", "1"); + std::env::set_var("KRONN_HOST_BIN", hostbin.to_string_lossy().to_string()); + + let conn = rusqlite::Connection::open_in_memory().unwrap(); + crate::db::migrations::run(&conn).unwrap(); + crate::db::mcps::upsert_server(&conn, &crate::models::McpServer { + id: "srv-uvx".into(), + name: "Atlassian".into(), + description: String::new(), + transport: crate::models::McpTransport::Stdio { + command: "uvx".into(), + args: vec!["mcp-atlassian".into()], + }, + source: crate::models::McpSource::Registry, + api_spec: None, + }).unwrap(); + crate::db::mcps::insert_config(&conn, &crate::models::McpConfig { + id: "cfg-uvx".into(), + server_id: "srv-uvx".into(), + label: "atlassian".into(), + env_keys: vec![], + env_encrypted: String::new(), + args_override: None, + is_global: true, + include_general: true, + config_hash: "h".into(), + project_ids: vec![], + host_sync: crate::models::HostSyncMode::GlobalOnly, + }).unwrap(); + + let plan = CodexSync.prepare(&conn, "secret-irrelevant").expect("plan"); + assert!(plan.content.contains("[mcp_servers.atlassian]"), + "a host-installed uvx MCP must survive the Codex plan under Docker. Got:\n{}", + plan.content); + + match prev_home { + Some(v) => std::env::set_var("KRONN_HOST_HOME", v), + None => std::env::remove_var("KRONN_HOST_HOME"), + } + match prev_docker { + Some(v) => std::env::set_var("KRONN_IN_DOCKER", v), + None => std::env::remove_var("KRONN_IN_DOCKER"), + } + match prev_hb { + Some(v) => std::env::set_var("KRONN_HOST_BIN", v), + None => std::env::remove_var("KRONN_HOST_BIN"), + } + cleanup(&tmp); + } + + #[test] + #[serial] + fn unavailable_command_is_skipped_by_codex_and_copilot_writers() { + // Parity regression (Codex install, 2026-07-12): entries whose + // command isn't on the host failed every Codex startup with + // "No such file or directory" — the .mcp.json writer filters them, + // the host writers didn't. + let tmp = setup_tmp("host-sync-unavailable-cmd"); + let home = tmp.join("fake-home"); + std::fs::create_dir_all(&home).unwrap(); + let prev = std::env::var("KRONN_HOST_HOME").ok(); + std::env::set_var("KRONN_HOST_HOME", home.to_string_lossy().to_string()); + + let conn = rusqlite::Connection::open_in_memory().unwrap(); + crate::db::migrations::run(&conn).unwrap(); + let server = crate::models::McpServer { + id: "srv-ghost".into(), + name: "Ghost".into(), + description: String::new(), + transport: crate::models::McpTransport::Stdio { + command: "definitely-not-a-binary-kronn-test".into(), + args: vec!["serve".into()], + }, + source: crate::models::McpSource::Registry, + api_spec: None, + }; + crate::db::mcps::upsert_server(&conn, &server).unwrap(); + crate::db::mcps::insert_config(&conn, &crate::models::McpConfig { + id: "cfg-ghost".into(), + server_id: "srv-ghost".into(), + label: "ghost".into(), + env_keys: vec![], + env_encrypted: String::new(), + args_override: None, + is_global: true, + include_general: true, + config_hash: "h".into(), + project_ids: vec![], + host_sync: crate::models::HostSyncMode::GlobalOnly, + }).unwrap(); + + let codex = CodexSync.prepare(&conn, "secret-irrelevant") + .expect("plan still produced (kronn-internal)"); + assert!(!codex.content.contains("definitely-not-a-binary-kronn-test"), + "unavailable command must not reach config.toml:\n{}", codex.content); + assert!(codex.content.contains("kronn-internal"), "bridge injection survives the filter"); + + let copilot = CopilotSync.prepare(&conn, "secret-irrelevant") + .expect("plan still produced (kronn-internal)"); + assert!(!copilot.content.contains("definitely-not-a-binary-kronn-test"), + "unavailable command must not reach mcp-config.json:\n{}", copilot.content); + assert!(copilot.content.contains("kronn-internal"), "bridge injection survives the filter"); + + match prev { + Some(v) => std::env::set_var("KRONN_HOST_HOME", v), + None => std::env::remove_var("KRONN_HOST_HOME"), + } + cleanup(&tmp); + } + #[test] #[serial] fn codex_global_sync_emits_kronn_internal_into_config_toml() { @@ -1907,6 +2075,7 @@ args = ["@example/old-mcp"] } #[test] + #[serial] // READS KRONN_INTROSPECTION_PUBLIC_PATH — races the tests that set it fn write_general_mcp_json_seeds_all_agent_configs_with_kronn_internal() { use rusqlite::Connection; let tmp = setup_tmp("general-all-agents"); diff --git a/backend/src/core/net_expose.rs b/backend/src/core/net_expose.rs index fa78771b..58fd1a0d 100644 --- a/backend/src/core/net_expose.rs +++ b/backend/src/core/net_expose.rs @@ -72,7 +72,13 @@ pub fn insecure_lan_boot_error( token_configured: bool, ack_insecure: bool, ) -> Option { - if ack_insecure || auth_enabled || token_configured || !lan_exposed { + // Actually-enforced auth = auth_enabled AND a token. Either alone is + // open in the middleware (`auth_allows`: auth off → open; no token → + // open) — the Docker default (auth OFF + auto-generated token) used to + // pass this guard on `token_configured` while every endpoint stayed + // wide open to the LAN. + let auth_enforced = auth_enabled && token_configured; + if ack_insecure || auth_enforced || !lan_exposed { return None; } Some( @@ -142,10 +148,14 @@ mod tests { assert!(insecure_lan_boot_error(false, false, false, false).is_none()); // Exposed + unauthenticated + not acknowledged → BLOCK. assert!(insecure_lan_boot_error(true, false, false, false).is_some()); - // Exposed but authenticated (token) → fine. - assert!(insecure_lan_boot_error(true, false, true, false).is_none()); - // Exposed but auth_enabled → fine. - assert!(insecure_lan_boot_error(true, true, false, false).is_none()); + // Exposed + auth ENFORCED (enabled AND token) → fine. + assert!(insecure_lan_boot_error(true, true, true, false).is_none()); + // Token WITHOUT auth_enabled is a middleware no-op (auth off → every + // endpoint open) — the Docker default (auth off + auto-generated + // token) must NOT pass the guard on the strength of the dead token. + assert!(insecure_lan_boot_error(true, false, true, false).is_some()); + // auth_enabled WITHOUT a token is equally open (no token → open). + assert!(insecure_lan_boot_error(true, true, false, false).is_some()); // Exposed + unauthenticated but explicitly acknowledged → allowed. assert!(insecure_lan_boot_error(true, false, false, true).is_none()); } diff --git a/backend/src/core/oauth2_cache.rs b/backend/src/core/oauth2_cache.rs index a257c948..9a1d7ffc 100644 --- a/backend/src/core/oauth2_cache.rs +++ b/backend/src/core/oauth2_cache.rs @@ -137,18 +137,18 @@ pub async fn resolve_token( "token exchange failed ({}): {}", status, // Trim the body so we don't dump kilobytes of Adobe HTML into logs. - &body.chars().take(300).collect::(), + body.chars().take(300).collect::(), )); } // Accept both `access_token` (RFC 6749 canonical) and providers that // drift slightly. `expires_in` is seconds — default 3600 if absent. let json: serde_json::Value = serde_json::from_str(&body) - .map_err(|e| format!("token response JSON parse error: {} — body was: {}", e, &body.chars().take(200).collect::()))?; + .map_err(|e| format!("token response JSON parse error: {} — body was: {}", e, body.chars().take(200).collect::()))?; let access_token = json.get("access_token") .and_then(|v| v.as_str()) - .ok_or_else(|| format!("token response missing `access_token` field: {}", &body.chars().take(200).collect::()))? + .ok_or_else(|| format!("token response missing `access_token` field: {}", body.chars().take(200).collect::()))? .to_string(); let expires_in = json.get("expires_in") @@ -274,7 +274,7 @@ pub async fn resolve_token_exchange( return Err(format!( "token exchange failed ({}): {}", status, - &body.chars().take(300).collect::(), + body.chars().take(300).collect::(), )); } @@ -282,14 +282,14 @@ pub async fn resolve_token_exchange( .map_err(|e| format!( "token response JSON parse error: {} — body was: {}", e, - &body.chars().take(200).collect::(), + body.chars().take(200).collect::(), ))?; // Extract via JSONPath. We use `serde_json_path` (already a workspace // dep for ExtractSpec). A miss is a hard error — without the token // there's nothing to inject downstream. let access_token = extract_token_jsonpath(&json, token_jsonpath) - .map_err(|e| format!("token extraction failed: {e} — body was: {}", &body.chars().take(200).collect::()))?; + .map_err(|e| format!("token extraction failed: {e} — body was: {}", body.chars().take(200).collect::()))?; // Cache (unless ttl=0, which only test code should use). if ttl_seconds > 0 { diff --git a/backend/src/core/rtk_detect.rs b/backend/src/core/rtk_detect.rs index 60c5b308..629c112c 100644 --- a/backend/src/core/rtk_detect.rs +++ b/backend/src/core/rtk_detect.rs @@ -146,6 +146,7 @@ fn resolve_home() -> Option { #[cfg(test)] mod tests { use super::*; + use serial_test::serial; use std::fs; use std::sync::Mutex; use tempfile::TempDir; @@ -183,12 +184,13 @@ mod tests { /// `KRONN_HOST_BIN`), NOT the container's baked `/usr/local/bin/rtk` — else /// the UI never offers to install RTK on the user's machine. #[test] + #[serial] fn rtk_binary_available_in_docker_checks_host_bins() { let _guard = ENV_MUTEX.lock().unwrap_or_else(|p| p.into_inner()); let tmp = TempDir::new().expect("tempdir"); - let prev_data = std::env::var("KRONN_DATA_DIR").ok(); + let prev_data = std::env::var("KRONN_IN_DOCKER").ok(); let prev_hb = std::env::var("KRONN_HOST_BIN").ok(); - std::env::set_var("KRONN_DATA_DIR", "/data"); // → is_docker() == true + std::env::set_var("KRONN_IN_DOCKER", "1"); // → is_docker() == true std::env::set_var("KRONN_HOST_BIN", tmp.path()); assert!(!rtk_binary_available(), "Docker + no host rtk → unavailable"); @@ -206,8 +208,8 @@ mod tests { } match prev_data { - Some(v) => std::env::set_var("KRONN_DATA_DIR", v), - None => std::env::remove_var("KRONN_DATA_DIR"), + Some(v) => std::env::set_var("KRONN_IN_DOCKER", v), + None => std::env::remove_var("KRONN_IN_DOCKER"), } match prev_hb { Some(v) => std::env::set_var("KRONN_HOST_BIN", v), diff --git a/backend/src/core/run_notify.rs b/backend/src/core/run_notify.rs index 33089434..e182208e 100644 --- a/backend/src/core/run_notify.rs +++ b/backend/src/core/run_notify.rs @@ -35,12 +35,17 @@ pub fn build_payload(workflow_name: &str, status: &RunStatus, run_id: &str, star serde_json::json!({ "text": text }).to_string() } -/// Resolve the effective notify URL (config first, then env), trimmed & non-empty. +/// Resolve the effective notify URL (config first, then env), trimmed & +/// non-empty. The config value is trimmed/filtered BEFORE the env fallback — +/// an empty/whitespace config entry must not mask a valid env URL. fn resolve_url(cfg_url: Option) -> Option { + let non_empty = |u: String| { + let u = u.trim().to_string(); + (!u.is_empty()).then_some(u) + }; cfg_url - .or_else(|| std::env::var("KRONN_FAILURE_NOTIFY_URL").ok()) - .map(|u| u.trim().to_string()) - .filter(|u| !u.is_empty()) + .and_then(non_empty) + .or_else(|| std::env::var("KRONN_FAILURE_NOTIFY_URL").ok().and_then(non_empty)) } /// Fire the failure webhook if the run failed and a URL is configured. @@ -48,11 +53,31 @@ pub async fn notify_if_failed(state: &AppState, workflow: &Workflow, run: &Workf if !should_notify(&run.status) { return; } + notify_terminal(state, &workflow.name, &run.status, &run.id, &run.started_at.to_rfc3339()).await; +} + +/// Webhook the boot-reconciled Interrupted runs. The engine-spawn tail can +/// never notify these — the process that owned them died mid-run; this is +/// the only origin for an `Interrupted` alert ("cron died at 6am" case). +pub async fn notify_boot_interrupted(state: &AppState) { + for r in state.db.take_boot_interrupted() { + notify_terminal(state, &r.workflow_name, &RunStatus::Interrupted, &r.run_id, &r.started_at).await; + } +} + +/// Primitive-field variant shared by all notify origins. +pub async fn notify_terminal( + state: &AppState, + workflow_name: &str, + status: &RunStatus, + run_id: &str, + started_rfc3339: &str, +) { let cfg_url = { state.config.read().await.server.failure_notify_url.clone() }; let Some(url) = resolve_url(cfg_url) else { return; }; - let body = build_payload(&workflow.name, &run.status, &run.id, &run.started_at.to_rfc3339()); + let body = build_payload(workflow_name, status, run_id, started_rfc3339); // Bounded client: reqwest's default has NO request timeout, so a hanging // webhook endpoint would pin this task forever and accumulate across // failed runs — the opposite of "best-effort" (Copilot review, PR #114). @@ -72,20 +97,21 @@ pub async fn notify_if_failed(state: &AppState, workflow: &Workflow, run: &Workf .await; match send { Ok(r) if r.status().is_success() => { - tracing::info!("Failure notification sent for run {} ({:?})", run.id, run.status) + tracing::info!("Failure notification sent for run {} ({:?})", run_id, status) } Ok(r) => tracing::warn!( "Failure notification returned {} for run {}", r.status(), - run.id + run_id ), - Err(e) => tracing::warn!("Failure notification POST failed for run {}: {}", run.id, e), + Err(e) => tracing::warn!("Failure notification POST failed for run {}: {}", run_id, e), } } #[cfg(test)] mod tests { use super::*; + use serial_test::serial; #[test] fn should_notify_only_on_non_success_terminal() { @@ -109,11 +135,21 @@ mod tests { } #[test] + #[serial] fn resolve_url_prefers_config_trims_and_rejects_empty() { + // Env-free assertions first (this test is the only env manipulator). + std::env::remove_var("KRONN_FAILURE_NOTIFY_URL"); assert_eq!(resolve_url(Some(" https://hook ".into())).as_deref(), Some("https://hook")); assert_eq!(resolve_url(Some(" ".into())), None); // No config, no env → None. - std::env::remove_var("KRONN_FAILURE_NOTIFY_URL"); assert_eq!(resolve_url(None), None); + + // An empty/whitespace config value must NOT mask a valid env URL. + std::env::set_var("KRONN_FAILURE_NOTIFY_URL", " https://env-hook "); + assert_eq!(resolve_url(Some(" ".into())).as_deref(), Some("https://env-hook")); + assert_eq!(resolve_url(None).as_deref(), Some("https://env-hook")); + // A real config value still wins over env. + assert_eq!(resolve_url(Some("https://cfg-hook".into())).as_deref(), Some("https://cfg-hook")); + std::env::remove_var("KRONN_FAILURE_NOTIFY_URL"); } } diff --git a/backend/src/core/scanner.rs b/backend/src/core/scanner.rs index d8c8828c..382e768a 100644 --- a/backend/src/core/scanner.rs +++ b/backend/src/core/scanner.rs @@ -851,6 +851,7 @@ mod tests { // ─── restore_host_path ────────────────────────────────────────────────── #[test] + #[serial] fn restore_host_path_no_host_home() { std::env::remove_var("KRONN_HOST_HOME"); let path = Path::new("/some/local/path"); @@ -860,6 +861,7 @@ mod tests { // ─── resolve_host_path ────────────────────────────────────────────────── #[test] + #[serial] fn resolve_host_path_passthrough_without_env() { std::env::remove_var("KRONN_HOST_HOME"); let result = resolve_host_path("/home/user/repos"); @@ -886,11 +888,14 @@ mod tests { } #[test] + #[serial] fn resolve_host_path_refuses_traversal_with_host_home() { // Even if KRONN_HOST_HOME is set, a `..` in the path must not be // mapped into /host-home — that would let a caller pivot outside // the mount root. We return the original PathBuf so downstream // operations fail rather than silently succeeding on the wrong target. + // #[serial]: this env mutation raced every non-neighbouring #[serial] + // test reading KRONN_HOST_HOME (the codex_global_sync flaky). std::env::set_var("KRONN_HOST_HOME", "/home/user"); let result = resolve_host_path("/home/user/../etc/passwd"); assert_eq!(result, PathBuf::from("/home/user/../etc/passwd")); diff --git a/backend/src/core/scanner_test.rs b/backend/src/core/scanner_test.rs index 7e22bfcf..e1b000e9 100644 --- a/backend/src/core/scanner_test.rs +++ b/backend/src/core/scanner_test.rs @@ -1,8 +1,10 @@ #[cfg(test)] mod tests { use crate::core::scanner::*; + use serial_test::serial; #[test] + #[serial] fn resolve_host_path_no_env() { // Without KRONN_HOST_HOME, paths should pass through unchanged std::env::remove_var("KRONN_HOST_HOME"); diff --git a/backend/src/core/tailscale.rs b/backend/src/core/tailscale.rs index b0c50890..c9cc0708 100644 --- a/backend/src/core/tailscale.rs +++ b/backend/src/core/tailscale.rs @@ -348,6 +348,7 @@ fn classify_ip(ip: &str, iface: &str) -> Option<(String, String, String)> { #[cfg(test)] mod tests { use super::*; + use serial_test::serial; /// `KRONN_HOST_IPS` is a process-global env var that several tests below /// set/remove. cargo runs tests in parallel by default, so without a serial @@ -446,6 +447,7 @@ mod tests { } #[test] + #[serial] fn parse_host_ips_env_valid() { let _env = env_guard(); std::env::set_var("KRONN_HOST_IPS", "eth0:192.168.1.50,tailscale0:100.100.50.1,tun0:10.8.0.5"); @@ -461,6 +463,7 @@ mod tests { } #[test] + #[serial] fn parse_host_ips_env_empty() { let _env = env_guard(); std::env::set_var("KRONN_HOST_IPS", ""); @@ -469,6 +472,7 @@ mod tests { } #[test] + #[serial] fn parse_host_ips_env_unset() { let _env = env_guard(); std::env::remove_var("KRONN_HOST_IPS"); @@ -476,6 +480,7 @@ mod tests { } #[test] + #[serial] fn parse_host_ips_env_skips_localhost_and_docker() { let _env = env_guard(); std::env::set_var("KRONN_HOST_IPS", "lo:127.0.0.1,docker0:172.17.0.1,eth0:192.168.1.10"); @@ -557,6 +562,7 @@ mod tests { } #[test] + #[serial] fn detect_via_host_env_returns_tailscale_when_present() { let _env = env_guard(); std::env::set_var( @@ -569,6 +575,7 @@ mod tests { } #[test] + #[serial] fn detect_via_host_env_returns_none_when_no_tailscale_entry() { let _env = env_guard(); std::env::set_var( @@ -580,6 +587,7 @@ mod tests { } #[test] + #[serial] fn detect_via_host_env_returns_none_when_unset() { let _env = env_guard(); std::env::remove_var("KRONN_HOST_IPS"); @@ -587,6 +595,7 @@ mod tests { } #[test] + #[serial] fn parse_host_ips_env_malformed_entries_are_skipped() { let _env = env_guard(); // No colon, trailing comma, only one part — all skipped silently. diff --git a/backend/src/core/worktree.rs b/backend/src/core/worktree.rs index 382d2175..c7117ea5 100644 --- a/backend/src/core/worktree.rs +++ b/backend/src/core/worktree.rs @@ -56,6 +56,19 @@ pub struct WorktreeInfo { } /// Check if a branch is checked out in any worktree (including the main repo). +/// Path equality that survives symlinks. git prints CANONICAL paths +/// (`/private/var/…` on macOS) while Kronn holds the user's spelling +/// (`/var/…`, `~/Sites` symlinks…). A naive `==` made the "branch checked +/// out in the main repo" guard misfire on macOS: instead of blocking, it +/// took the reuse path and handed the MAIN CHECKOUT back as a "worktree" +/// (is_main_repo: false) — the agent then edited the user's live tree +/// believing it was isolated. +fn same_path(a: &Path, b: &Path) -> bool { + let ca = a.canonicalize().unwrap_or_else(|_| a.to_path_buf()); + let cb = b.canonicalize().unwrap_or_else(|_| b.to_path_buf()); + ca == cb +} + fn branch_checked_out_at(repo_path: &Path, branch: &str) -> Option { let output = sync_cmd("git") .args(["worktree", "list", "--porcelain"]) @@ -169,7 +182,7 @@ pub fn create_discussion_worktree( // branches before the agent can work. This avoids the agent modifying files under // a running dev environment. if let Some(existing_path) = branch_checked_out_at(repo_path, &branch) { - if existing_path == repo_path { + if same_path(&existing_path, repo_path) { return Err(format!( "Branch {} is currently checked out in the main repo. Please switch to another branch before continuing.", branch @@ -301,7 +314,7 @@ pub fn reattach_worktree( // Block if branch is checked out in the main repo (user is testing) if let Some(existing_path) = branch_checked_out_at(repo_path, existing_branch) { - if existing_path == repo_path { + if same_path(&existing_path, repo_path) { return Err(format!( "Branch {} is currently checked out in the main repo. Please switch to another branch first.", existing_branch @@ -1002,7 +1015,9 @@ mod tests { let repo = make_test_repo("checkout-at"); let result = branch_checked_out_at(repo.path(), "main"); assert!(result.is_some(), "main should be found as checked out"); - assert_eq!(result.unwrap(), repo.path()); + // Compare through same_path: git canonicalizes (`/private/var/…` on + // macOS) while tempfile returns the symlinked spelling (`/var/…`). + assert!(same_path(&result.unwrap(), repo.path())); } #[test] diff --git a/backend/src/core/ws_client.rs b/backend/src/core/ws_client.rs index 65889782..e46949b9 100644 --- a/backend/src/core/ws_client.rs +++ b/backend/src/core/ws_client.rs @@ -77,7 +77,22 @@ async fn connect_to_peer(state: AppState, contact: crate::models::Contact) { tracing::debug!("WS client: connecting to {} ({})", contact.pseudo, ws_url); - match tokio_tungstenite::connect_async(&ws_url).await { + // Bound the handshake: a peer that accepts TCP but never completes the + // WS upgrade (NAT/relay half-open) would otherwise pin this task inside + // connect_async forever — and the manager loop only respawns FINISHED + // tasks, so that contact would stay silently unreachable until restart. + let connect = tokio::time::timeout( + std::time::Duration::from_secs(30), + tokio_tungstenite::connect_async(&ws_url), + ) + .await + .unwrap_or_else(|_| { + Err(tokio_tungstenite::tungstenite::Error::Io(std::io::Error::new( + std::io::ErrorKind::TimedOut, + "WS handshake timed out after 30s", + ))) + }); + match connect { Ok((ws_stream, _)) => { let session_start = Instant::now(); tracing::info!("WS client: connected to {}", contact.pseudo); diff --git a/backend/src/db/learnings.rs b/backend/src/db/learnings.rs index aa5ddb9b..c2d09a2f 100644 --- a/backend/src/db/learnings.rs +++ b/backend/src/db/learnings.rs @@ -38,8 +38,23 @@ fn row_to_learning(row: &Row) -> rusqlite::Result { id: row.get(0)?, claim: row.get(1)?, evidence, - kind: LearningKind::from_db(&kind_s).unwrap_or(LearningKind::Inference), - status: LearningStatus::from_db(&status_s).unwrap_or(LearningStatus::Pending), + // Unknown stored labels must not be rewritten silently: a Rejected + // learning resurfacing as Pending is a human decision undone. Keep the + // fallback (row stays loadable) but name the raw value. + kind: LearningKind::from_db(&kind_s).unwrap_or_else(|| { + tracing::warn!( + target: "continual_learning", + "learning {id_for_log}: unknown kind '{kind_s}' in DB — falling back to Inference", + ); + LearningKind::Inference + }), + status: LearningStatus::from_db(&status_s).unwrap_or_else(|| { + tracing::warn!( + target: "continual_learning", + "learning {id_for_log}: unknown status '{status_s}' in DB — falling back to Pending", + ); + LearningStatus::Pending + }), scope: scope_s.as_deref().and_then(LearningScope::from_db), confidence: row.get(6)?, faithfulness: faith_s.as_deref().and_then(Faithfulness::from_db), diff --git a/backend/src/db/mod.rs b/backend/src/db/mod.rs index d6efe47b..ba46d4c9 100644 --- a/backend/src/db/mod.rs +++ b/backend/src/db/mod.rs @@ -30,6 +30,11 @@ use crate::core::config; pub struct Database { conn: Arc>, path: PathBuf, + /// Runs flipped `Running`/`Pending` → `Interrupted` by THIS boot's + /// reconcile. Drained exactly once by the boot notifier + /// (`run_notify::notify_boot_interrupted`) — the process that would have + /// webhooked these failures died with them. + boot_interrupted: Mutex>, } impl Database { @@ -47,7 +52,11 @@ impl Database { .context("Failed to open in-memory database")?; conn.execute_batch("PRAGMA foreign_keys=ON;")?; migrations::run(&conn)?; - Ok(Self { conn: Arc::new(Mutex::new(conn)), path: PathBuf::from(":memory:") }) + Ok(Self { + conn: Arc::new(Mutex::new(conn)), + path: PathBuf::from(":memory:"), + boot_interrupted: Mutex::new(Vec::new()), + }) } /// Open a database at a specific path (useful for testing). @@ -100,13 +109,15 @@ impl Database { // 0.8.4 (#317 / B1) — reconcile stale `Running` audit_runs at // boot. A backend crash, container restart, or kill -9 during // an audit leaves the row stuck `Running` forever, polluting - // the recap chip strip + the "active audits" badge. Any run - // older than 30 min is force-flipped to `Interrupted` (the - // resume mechanism still works because last_completed_step - // survives — see `reconcile_stale_runs_preserves_last_completed_step`). - match audit_runs::reconcile_stale_runs(&conn, 30 * 60) { + // the recap chip strip + the "active audits" badge. + // Cutoff 0, same reasoning as the workflow_runs reconcile below: + // at boot no in-process audit runner exists, so every `Running` + // row is a zombie — and the flip is resume-safe because + // last_completed_step survives (see + // `reconcile_stale_runs_preserves_last_completed_step`). + match audit_runs::reconcile_stale_runs(&conn, 0) { Ok(0) => {} - Ok(n) => tracing::info!("Reconciled {} stale audit_runs (status was 'Running' for > 30 min)", n), + Ok(n) => tracing::info!("Reconciled {} zombie audit_runs left 'Running' by a previous process → Interrupted", n), Err(e) => tracing::warn!("Failed to reconcile stale audit_runs: {}", e), } @@ -117,11 +128,18 @@ impl Database { // so every `Running`/`Pending` row is by definition a zombie — a grace // window would just leave a freshly-interrupted run lying about its // status for up to that long (Copilot review, PR #114). - match workflows::reconcile_stale_runs(&conn, 0) { - Ok(0) => {} - Ok(n) => tracing::info!("Reconciled {} zombie workflow_runs left 'Running'/'Pending' by a previous process → Interrupted", n), - Err(e) => tracing::warn!("Failed to reconcile stale workflow_runs: {}", e), - } + let boot_interrupted = match workflows::reconcile_stale_runs(&conn, 0) { + Ok(v) => { + if !v.is_empty() { + tracing::info!("Reconciled {} zombie workflow_runs left 'Running'/'Pending' by a previous process → Interrupted", v.len()); + } + v + } + Err(e) => { + tracing::warn!("Failed to reconcile stale workflow_runs: {}", e); + Vec::new() + } + }; // 0.8.6 — auto-purge api_call_logs older than 90 days at boot. // Generous default : keeps a quarter of audit trail for debug @@ -133,7 +151,11 @@ impl Database { Err(e) => tracing::warn!("Failed to auto-purge api_call_logs: {}", e), } - Ok(Self { conn: Arc::new(Mutex::new(conn)), path: path.clone() }) + Ok(Self { + conn: Arc::new(Mutex::new(conn)), + path: path.clone(), + boot_interrupted: Mutex::new(boot_interrupted), + }) } /// Get the database file path. @@ -141,6 +163,15 @@ impl Database { &self.path } + /// Drain the boot-reconciled Interrupted runs (once). Returns empty on + /// every subsequent call — the boot notifier is the single consumer. + pub fn take_boot_interrupted(&self) -> Vec { + self.boot_interrupted + .lock() + .map(|mut v| std::mem::take(&mut *v)) + .unwrap_or_default() + } + /// Execute a blocking closure with the database connection. /// Runs inside `spawn_blocking` so the Tokio worker thread is never blocked /// waiting on the mutex or executing a synchronous SQLite query. @@ -151,8 +182,33 @@ impl Database { { let conn = self.conn.clone(); tokio::task::spawn_blocking(move || { - let conn = conn.lock().map_err(|e| anyhow::anyhow!("Mutex poisoned: {e}"))?; - f(&conn) + // Poison is recoverable here: a panicked closure can't leave + // SQLite mid-transaction (rusqlite rolls back on drop), while + // treating poison as fatal turns one panic into a full outage. + let guard = match conn.lock() { + Ok(g) => g, + Err(poisoned) => { + tracing::error!("DB mutex was poisoned by a previous panic — recovering the lock"); + // Clear the flag or every later lock() re-enters this + // error path (log spam on each DB call, forever). + conn.clear_poison(); + poisoned.into_inner() + } + }; + // Catch panics BEFORE they unwind through the guard: the mutex + // never poisons, and the panic message reaches the API error. + match std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| f(&guard))) { + Ok(r) => r, + Err(payload) => { + let msg = payload + .downcast_ref::<&str>() + .map(|s| s.to_string()) + .or_else(|| payload.downcast_ref::().cloned()) + .unwrap_or_else(|| "non-string panic payload".into()); + tracing::error!("DB closure panicked: {msg}"); + Err(anyhow::anyhow!("DB closure panicked: {msg}")) + } + } }) .await .map_err(|e| anyhow::anyhow!("spawn_blocking failed: {e}"))? diff --git a/backend/src/db/tests.rs b/backend/src/db/tests.rs index d469d519..dbbdcdd1 100644 --- a/backend/src/db/tests.rs +++ b/backend/src/db/tests.rs @@ -1452,8 +1452,10 @@ fn reconcile_stale_runs_flips_only_old_running_pending_to_interrupted() { done.started_at = Utc::now() - Duration::hours(2); crate::db::workflows::insert_run(&conn, &done).unwrap(); - let n = crate::db::workflows::reconcile_stale_runs(&conn, 30 * 60).unwrap(); - assert_eq!(n, 2, "the two stale in-flight runs are reconciled"); + let flipped = crate::db::workflows::reconcile_stale_runs(&conn, 30 * 60).unwrap(); + assert_eq!(flipped.len(), 2, "the two stale in-flight runs are reconciled"); + // The reconciler reports enough to webhook the dead runs. + assert!(flipped.iter().all(|r| !r.workflow_name.is_empty() && !r.started_at.is_empty())); let by_id = |id: &str| crate::db::workflows::get_run(&conn, id).unwrap().unwrap(); assert_eq!(by_id("stale").status, RunStatus::Interrupted); @@ -1464,11 +1466,46 @@ fn reconcile_stale_runs_flips_only_old_running_pending_to_interrupted() { // Cutoff 0 = the BOOT call (Copilot, PR #114): at boot there is no runner, // so even a JUST-started zombie must flip — no 30-min lie window. - let n0 = crate::db::workflows::reconcile_stale_runs(&conn, 0).unwrap(); - assert_eq!(n0, 1, "the fresh zombie is reconciled at cutoff 0"); + let flipped0 = crate::db::workflows::reconcile_stale_runs(&conn, 0).unwrap(); + assert_eq!(flipped0.len(), 1, "the fresh zombie is reconciled at cutoff 0"); assert_eq!(by_id("fresh").status, RunStatus::Interrupted); } +#[test] +fn cancelled_status_is_sticky_in_update_run_progress() { + // The user's forced cancel races the runner's own writes; the runner + // used to win (Cancelled → Failed minutes after "stopped", or + // Cancelled → Running in the gate-resume window). Only a Cancelled + // write may touch a Cancelled row. + let conn = test_db(); + crate::db::workflows::insert_workflow(&conn, &sample_workflow("w1")).unwrap(); + let mut run = sample_run("r1", "w1"); + run.status = RunStatus::Running; + crate::db::workflows::insert_run(&conn, &run).unwrap(); + + // User cancels (what api cancel_run's forced UPDATE does). + run.status = RunStatus::Cancelled; + let snap = crate::db::workflows::RunProgressSnapshot::from_run(&run); + assert!(crate::db::workflows::update_run_progress(&conn, snap).unwrap()); + + // Runner's late terminal write must be BLOCKED… + run.status = RunStatus::Failed; + let snap = crate::db::workflows::RunProgressSnapshot::from_run(&run); + assert!(!crate::db::workflows::update_run_progress(&conn, snap).unwrap(), "Failed-over-Cancelled must be rejected"); + // …and so must the resume path's Running resurrection. + run.status = RunStatus::Running; + let snap = crate::db::workflows::RunProgressSnapshot::from_run(&run); + assert!(!crate::db::workflows::update_run_progress(&conn, snap).unwrap(), "Running-over-Cancelled must be rejected"); + + let row = crate::db::workflows::get_run(&conn, "r1").unwrap().unwrap(); + assert_eq!(row.status, RunStatus::Cancelled, "row stays Cancelled"); + + // Idempotent cancel re-write stays allowed. + run.status = RunStatus::Cancelled; + let snap = crate::db::workflows::RunProgressSnapshot::from_run(&run); + assert!(crate::db::workflows::update_run_progress(&conn, snap).unwrap()); +} + #[test] fn list_runs_enriches_subworkflow_parent_provenance() { let conn = test_db(); diff --git a/backend/src/db/workflows.rs b/backend/src/db/workflows.rs index e940ec7a..3d40cd80 100644 --- a/backend/src/db/workflows.rs +++ b/backend/src/db/workflows.rs @@ -50,17 +50,47 @@ fn run_status_str(s: &RunStatus) -> &'static str { /// zombie as in-progress. Flips rows older than `stale_after_secs` to the /// terminal `Interrupted` status (distinct from `Failed`). Mirrors /// `audit_runs::reconcile_stale_runs`. Returns the number reconciled. -pub fn reconcile_stale_runs(conn: &Connection, stale_after_secs: i64) -> Result { +/// A run flipped to `Interrupted` by the boot reconcile — enough info to fire +/// the failure webhook for it. The process that would normally have notified +/// (the engine spawn's tail) died with the run, so the boot path is the ONLY +/// place an Interrupted notification can originate. +#[derive(Debug, Clone)] +pub struct ReconciledRun { + pub run_id: String, + pub workflow_name: String, + /// RFC3339, as stored. + pub started_at: String, +} + +pub fn reconcile_stale_runs(conn: &Connection, stale_after_secs: i64) -> Result> { let cutoff = (Utc::now() - chrono::Duration::seconds(stale_after_secs)).to_rfc3339(); let now_rfc = Utc::now().to_rfc3339(); - let affected = conn.execute( - "UPDATE workflow_runs SET - status = 'Interrupted', - finished_at = COALESCE(finished_at, ?2) - WHERE status IN ('Running', 'Pending') AND started_at < ?1", - params![cutoff, now_rfc], + // Select-then-update is race-free here: the caller holds the single + // shared connection for both statements. + let mut stmt = conn.prepare( + "SELECT r.id, COALESCE(w.name, r.workflow_id), r.started_at + FROM workflow_runs r LEFT JOIN workflows w ON w.id = r.workflow_id + WHERE r.status IN ('Running', 'Pending') AND r.started_at < ?1", )?; - Ok(affected as u64) + let flipped: Vec = stmt + .query_map(params![cutoff], |row| { + Ok(ReconciledRun { + run_id: row.get(0)?, + workflow_name: row.get(1)?, + started_at: row.get(2)?, + }) + })? + .collect::>()?; + if !flipped.is_empty() { + conn.execute( + "UPDATE workflow_runs SET + status = 'Interrupted', + finished_at = COALESCE(finished_at, ?2) + WHERE status IN ('Running', 'Pending') AND started_at < ?1", + params![cutoff, now_rfc], + )?; + } + Ok(flipped) } // ─── Workflows CRUD ───────────────────────────────────────────────────────── @@ -566,8 +596,11 @@ pub fn insert_workflow(conn: &Connection, wf: &Workflow) -> Result<()> { Ok(()) } -pub fn update_workflow(conn: &Connection, wf: &Workflow) -> Result<()> { - conn.execute( +/// Returns `false` when the row no longer exists (deleted concurrently) — +/// a TYPED signal, so callers never string-match the error message. +/// Reporting success on 0 rows would silently drop the caller's edit. +pub fn update_workflow(conn: &Connection, wf: &Workflow) -> Result { + let n = conn.execute( "UPDATE workflows SET name = ?2, project_id = ?3, trigger_json = ?4, steps_json = ?5, actions_json = ?6, safety_json = ?7, workspace_config_json = ?8, concurrency_limit = ?9, enabled = ?10, updated_at = ?11, guards = ?12, artifacts = ?13, @@ -592,7 +625,7 @@ pub fn update_workflow(conn: &Connection, wf: &Workflow) -> Result<()> { if wf.variables.is_empty() { None } else { Some(serde_json::to_string(&wf.variables)?) }, ], )?; - Ok(()) + Ok(n > 0) } pub fn delete_workflow(conn: &Connection, id: &str) -> Result<()> { @@ -829,7 +862,9 @@ pub fn claim_waiting_run(conn: &Connection, run_id: &str, new_status: &RunStatus } pub fn update_run(conn: &Connection, run: &WorkflowRun) -> Result<()> { - update_run_progress(conn, RunProgressSnapshot::from_run(run)) + // Same Cancelled-stickiness semantics; callers of this convenience + // wrapper don't act on the raced-cancel signal. + update_run_progress(conn, RunProgressSnapshot::from_run(run)).map(|_| ()) } /// Lightweight snapshot of a WorkflowRun for progress updates. @@ -865,15 +900,25 @@ impl RunProgressSnapshot { } } -pub fn update_run_progress(conn: &Connection, snap: RunProgressSnapshot) -> Result<()> { - conn.execute( +/// Returns `false` when the row was NOT updated — either it no longer exists, +/// or the `Cancelled` stickiness guard blocked the write. +/// +/// `Cancelled` is STICKY: a user's forced cancel (api cancel_run) races the +/// runner's own progress/terminal writes, and the runner used to win — +/// overwriting `Cancelled` with `Running` (gate-resume window) or `Failed` +/// (rollback-chain tail) minutes after the user was told "stopped". Only a +/// `Cancelled` write may touch a `Cancelled` row. Runner lifecycle sites +/// treat `false` as "cancelled beneath us" and stop. +pub fn update_run_progress(conn: &Connection, snap: RunProgressSnapshot) -> Result { + let new_status = run_status_str(&snap.status); + let affected = conn.execute( "UPDATE workflow_runs SET status = ?2, step_results_json = ?3, tokens_used = ?4, workspace_path = ?5, finished_at = ?6, state = ?7, produced_branches = ?8 - WHERE id = ?1", + WHERE id = ?1 AND (status != 'Cancelled' OR ?2 = 'Cancelled')", params![ snap.id, - run_status_str(&snap.status), + new_status, serde_json::to_string(&snap.step_results)?, snap.tokens_used as i64, snap.workspace_path, @@ -882,7 +927,7 @@ pub fn update_run_progress(conn: &Connection, snap: RunProgressSnapshot) -> Resu if snap.produced_branches.is_empty() { None } else { Some(serde_json::to_string(&snap.produced_branches)?) }, ], )?; - Ok(()) + Ok(affected > 0) } /// Delete a single run. @@ -971,6 +1016,7 @@ pub fn mark_issue_processed(conn: &Connection, workflow_id: &str, issue_id: &str // ─── Row mappers ──────────────────────────────────────────────────────────── fn row_to_workflow(row: &rusqlite::Row) -> Workflow { + let id: String = row.get(0).unwrap_or_default(); let trigger_str: String = row.get(3).unwrap_or_default(); let steps_str: String = row.get(4).unwrap_or_default(); let actions_str: String = row.get(5).unwrap_or_default(); @@ -983,12 +1029,33 @@ fn row_to_workflow(row: &rusqlite::Row) -> Workflow { let exec_allowlist_str: Option = row.get(15).unwrap_or(None); let variables_str: Option = row.get(16).unwrap_or(None); + // These two fallbacks keep a workflow with corrupt JSON loadable (booting + // matters), but they MUST be loud: a silently-Manual trigger kills a cron + // workflow, and silently-empty steps make a corrupt workflow run green as + // a no-op. + let trigger = serde_json::from_str(&trigger_str).unwrap_or_else(|e| { + tracing::error!( + workflow_id = %id, + error = %e, + "corrupt trigger_json — falling back to Manual (scheduled/tracker triggers DISABLED for this workflow)" + ); + WorkflowTrigger::Manual + }); + let steps: Vec = serde_json::from_str(&steps_str).unwrap_or_else(|e| { + tracing::error!( + workflow_id = %id, + error = %e, + "corrupt steps_json — falling back to ZERO steps (this workflow would run as a no-op)" + ); + Vec::new() + }); + Workflow { - id: row.get(0).unwrap_or_default(), + id, name: row.get(1).unwrap_or_default(), project_id: row.get(2).unwrap_or(None), - trigger: serde_json::from_str(&trigger_str).unwrap_or(WorkflowTrigger::Manual), - steps: serde_json::from_str(&steps_str).unwrap_or_default(), + trigger, + steps, actions: serde_json::from_str(&actions_str).unwrap_or_default(), safety: serde_json::from_str(&safety_str).unwrap_or(WorkflowSafety { sandbox: false, max_files: None, max_lines: None, require_approval: false, diff --git a/backend/src/main.rs b/backend/src/main.rs index a1ac2dbd..4b0358a2 100644 --- a/backend/src/main.rs +++ b/backend/src/main.rs @@ -111,9 +111,28 @@ async fn main() -> anyhow::Result<()> { .ok() .map(|t| t.trim().to_string()) .filter(|t| !t.is_empty()); - if app_config.server.auth_token.is_none() { - if let Some(t) = env_token { - app_config.server.auth_token = Some(t); + if let Some(ref t) = env_token { + match &app_config.server.auth_token { + None => app_config.server.auth_token = Some(t.clone()), + // Both set but DIFFERENT: the middleware validates the config + // token while spawned children (sidecar) inherit the env one — + // every sidecar call 401s with no visible cause. Don't silently + // pick a winner; tell the operator which one wins. + Some(cfg) if cfg != t => tracing::warn!( + "KRONN_AUTH_TOKEN env differs from the token in config.toml — the API \ + validates the CONFIG token, but child processes (MCP sidecar, agent \ + CLIs) inherit the ENV one and will get 401s. Align them (unset the env \ + var, or clear server.auth_token in config.toml)." + ), + Some(_) => {} + } + // An operator who explicitly sets KRONN_AUTH_TOKEN is asking for auth + // — and the LAN boot guard's own error message promises this env var + // secures the instance. A token with auth_enabled=false is a no-op in + // the middleware, so honor the intent. + if !app_config.server.auth_enabled { + tracing::info!("KRONN_AUTH_TOKEN set — enabling API authentication (was disabled)"); + app_config.server.auth_enabled = true; } } if let Some(ref token) = app_config.server.auth_token { @@ -139,7 +158,11 @@ async fn main() -> anyhow::Result<()> { // a fresh `docker compose up` publishes on 127.0.0.1 (see docker-compose.yml // `KRONN_BIND`), so this never fires for the standard install. Exposing to // the LAN then requires a token, auth, or an explicit risk acknowledgment. - let is_docker = std::env::var("KRONN_DATA_DIR").is_ok(); + // Real container detection (KRONN_IN_DOCKER / /.dockerenv) — NOT the old + // KRONN_DATA_DIR proxy: a native install relocating its data dir must not + // silently switch the guard to docker mode (where an unset KRONN_BIND + // reads as "not exposed" even when the actual bind host is 0.0.0.0). + let is_docker = kronn::core::env::is_docker(); let kronn_bind = std::env::var("KRONN_BIND").ok(); let ack_insecure = std::env::var("KRONN_ALLOW_INSECURE_LAN") .map(|v| matches!(v.trim(), "1" | "true" | "yes")) @@ -192,40 +215,41 @@ async fn main() -> anyhow::Result<()> { tokio::spawn(async move { sc.start().await }); } + // B6 — webhook the runs the PREVIOUS process died holding (flipped to + // Interrupted by the boot reconcile). The engine-spawn tail can never + // notify these; without this, "cron died at 6am" stays silent — the + // exact case the failure webhook exists for. Best-effort, bounded. + { + let st = state.clone(); + tokio::spawn(async move { + kronn::core::run_notify::notify_boot_interrupted(&st).await; + }); + } + // Workflow engine gets a clone of the state so it can spawn runs that // need full access (batch fan-out, ws broadcasts, agent semaphore). let workflow_engine = Arc::new(WorkflowEngine::new(state.clone())); - // ── Orphan scan ──────────────────────────────────────────────────────── - // Previous process may have crashed/been killed while workflow_runs or - // discussions were in the "Running" state. Nothing is listening for them - // anymore (cancel registry is empty at boot), so the UI would show them - // as running forever without this cleanup. We mark any still-Running row - // as Failed with a clear note. - let cleaned = state.db.with_conn(|conn| { - // 2026-06-10 — also reap `Pending`: a run row created but never - // picked up before the previous process died is just as orphaned as - // a `Running` one (the cancel registry is empty at boot, nothing - // will ever advance it). `WaitingApproval` is INTENTIONALLY left - // alone — it's a durable human-gate state that survives restarts by - // design and resumes via /decide. - let runs = conn.execute( - "UPDATE workflow_runs SET status = 'Failed', finished_at = datetime('now') \ - WHERE status IN ('Running', 'Pending')", + // ── Orphan scan (workflow_runs) ───────────────────────────────────────── + // Superseded by the boot reconcile in `Database::open_path` (0.8.11 B5): + // zombies are flipped to `Interrupted` there — the terminal state the UI, + // the duration averages, and the failure webhook all expect. The legacy + // UPDATE here marked them `Failed` AFTER that reconcile (so it always + // matched 0 rows) and would have mislabeled + bypassed the boot + // notification if it ever fired. Kept as an assertion-style probe only. + match state.db.with_conn(|conn| { + conn.query_row( + "SELECT COUNT(*) FROM workflow_runs WHERE status IN ('Running', 'Pending')", [], - )?; - // Append a marker message to the last agent response so the UI shows - // what happened. We can't easily UPDATE the last message content from - // here without rehydrating the messages table schema, so we just log - // the count — the disc will show as "no reply yet" until re-run. - Ok(runs) - }).await; - match cleaned { - Ok(n) if n > 0 => tracing::warn!( - "Orphan scan: {} workflow_runs left Running by previous process, marked as Failed", n + |row| row.get::<_, i64>(0), + ).map_err(Into::into) + }).await { + Ok(0) => tracing::info!("Orphan scan: nothing to clean up (boot reconcile already ran)"), + Ok(n) => tracing::warn!( + "Orphan scan: {n} workflow_runs still Running/Pending AFTER the boot reconcile — \ + this should be impossible, investigate db::open_path ordering" ), - Ok(_) => tracing::info!("Orphan scan: nothing to clean up"), - Err(e) => tracing::warn!("Orphan scan failed: {}", e), + Err(e) => tracing::warn!("Orphan scan probe failed: {}", e), } // ── Registry sync ───────────────────────────────────────────────────── @@ -320,8 +344,16 @@ async fn main() -> anyhow::Result<()> { } } if imported > 0 { - let _ = config::save(&config).await; - tracing::info!("Auto-imported {} API key(s) from agent configs", imported); + match config::save(&config).await { + Ok(_) => tracing::info!("Auto-imported {} API key(s) from agent configs", imported), + // Don't log success over a failed persist: the keys exist only + // in memory and silently vanish (with any user edits layered + // on them) at the next restart. + Err(e) => tracing::error!( + "Auto-imported {} API key(s) but saving config.toml FAILED: {e} — keys are in-memory only until the next successful save", + imported + ), + } } } diff --git a/backend/src/workflows/batch_step.rs b/backend/src/workflows/batch_step.rs index dabf1b43..bf8ea7cf 100644 --- a/backend/src/workflows/batch_step.rs +++ b/backend/src/workflows/batch_step.rs @@ -343,10 +343,34 @@ pub async fn execute_batch_quick_prompt_step( } Ok(Err(tokio::sync::broadcast::error::RecvError::Lagged(n))) => { tracing::warn!( - "BatchQuickPrompt step '{}' lagged {} WS messages — keep listening", + "BatchQuickPrompt step '{}' lagged {} WS messages — re-checking child run in DB", step.name, n ); - continue; + // The dropped window may have held the SINGLE terminal + // BatchRunFinished (emitted exactly once) — without this + // re-read the step would wait the full 2h timeout and be + // marked Failed while the child batch is actually Success. + let cid = child_run_id.clone(); + match state.db.with_conn(move |conn| crate::db::workflows::get_run(conn, &cid)).await { + Ok(Some(child)) if matches!( + child.status, + crate::models::RunStatus::Success + | crate::models::RunStatus::Failed + | crate::models::RunStatus::Cancelled + | crate::models::RunStatus::StoppedByGuard + | crate::models::RunStatus::Interrupted + ) => { + final_total = child.batch_total; + final_ok = child.batch_completed; + final_failed = child.batch_failed; + tracing::info!( + "BatchQuickPrompt step '{}' recovered terminal state from DB after lag: {}/{} ok, {} failed", + step.name, final_ok, final_total, final_failed + ); + break; + } + _ => continue, // still running (or transient DB miss) — keep listening + } } Ok(Err(tokio::sync::broadcast::error::RecvError::Closed)) => { return fail(step, start, "WS broadcast channel closed while waiting for batch completion"); diff --git a/backend/src/workflows/gate_checkpoint.rs b/backend/src/workflows/gate_checkpoint.rs index a2382d3d..14ef48bd 100644 --- a/backend/src/workflows/gate_checkpoint.rs +++ b/backend/src/workflows/gate_checkpoint.rs @@ -145,6 +145,27 @@ pub fn commit_checkpoint( /// have verified the SHA came from the run's own `state` map (no /// arbitrary-SHA reset). pub fn reset_to_checkpoint(project_path: &Path, sha: &str) -> Result<(), String> { + // TD-20260709 (C) — this can run HOURS after the checkpoint, on a tree a + // human or another run may have touched since. Uncommitted changes are + // not ours to destroy: refuse, the caller degrades gracefully. + let st = crate::core::cmd::sync_cmd("git") + .args(["status", "--porcelain"]) + .current_dir(project_path) + .output() + .map_err(|e| format!("git status spawn failed: {e}"))?; + if !st.status.success() { + return Err(format!( + "git status failed before reset: {}", + String::from_utf8_lossy(&st.stderr) + )); + } + if !st.stdout.is_empty() { + return Err(format!( + "main tree has uncommitted changes not from this run — refusing `git reset --hard` \ + (TD-20260709). Dirty entries:\n{}", + String::from_utf8_lossy(&st.stdout).trim_end() + )); + } let r = crate::core::cmd::sync_cmd("git") .args(["reset", "--hard", sha]) .current_dir(project_path) @@ -311,6 +332,26 @@ mod tests { let _ = fs::remove_dir_all(&tmp); } + #[test] + fn reset_refuses_when_tree_has_uncommitted_changes() { + // TD-20260709 (C): a deferred reset must never destroy WIP the run + // didn't create. + let tmp = tmp_repo(); + let sha = match commit_checkpoint(&tmp, "g1", "run-1") { + CheckpointOutcome::Committed { sha } => sha, + other => panic!("expected Committed, got {other:?}"), + }; + fs::write(tmp.join("wip.txt"), "human work in progress").unwrap(); + let err = reset_to_checkpoint(&tmp, &sha).unwrap_err(); + assert!(err.contains("uncommitted changes"), "must name the refusal reason: {err}"); + assert_eq!( + fs::read_to_string(tmp.join("wip.txt")).unwrap(), + "human work in progress", + "the dirty file must be untouched" + ); + let _ = fs::remove_dir_all(&tmp); + } + #[test] fn reset_to_checkpoint_returns_err_on_bogus_sha() { let tmp = tmp_repo(); diff --git a/backend/src/workflows/mod.rs b/backend/src/workflows/mod.rs index 64a561e2..09d680d4 100644 --- a/backend/src/workflows/mod.rs +++ b/backend/src/workflows/mod.rs @@ -39,11 +39,21 @@ use crate::AppState; /// The workflow engine — runs in the background, checks triggers, spawns runs. pub struct WorkflowEngine { state: AppState, + /// End of the previous trigger-evaluation window. Each tick evaluates + /// `(last_trigger_check, now]` so a cron occurrence fires exactly once — + /// no double-fire on the 30s window seam, no skip when a slow tracker + /// poll delays the tick. Swapped at the START of the tick (at-most-once: + /// a tick that errors mid-way drops its window rather than re-firing + /// already-spawned runs on the retry). + last_trigger_check: tokio::sync::Mutex>, } impl WorkflowEngine { pub fn new(state: AppState) -> Self { - Self { state } + Self { + state, + last_trigger_check: tokio::sync::Mutex::new(Utc::now()), + } } /// Convenience accessors so existing code using `self.db` / `self.config` @@ -90,7 +100,8 @@ impl WorkflowEngine { let wf_clone = wf.clone(); let db2 = self.db().clone(); match db2.with_conn(move |conn| crate::db::workflows::update_workflow(conn, &wf_clone)).await { - Ok(()) => { + Ok(false) => tracing::warn!("Heal skipped: workflow '{}' vanished mid-pass", wf.name), + Ok(true) => { healed_count += 1; tracing::info!( "Healed workflow '{}' (id={}): upgraded steps {:?} to Structured", @@ -113,6 +124,13 @@ impl WorkflowEngine { /// Check all enabled workflows and fire triggers. async fn check_triggers(&self) -> anyhow::Result<()> { + // Claim this tick's evaluation window (see `last_trigger_check`). + let now = Utc::now(); + let since = { + let mut last = self.last_trigger_check.lock().await; + std::mem::replace(&mut *last, now) + }; + let db = self.db().clone(); let workflows = db.with_conn(|conn| { crate::db::workflows::list_workflows(conn) @@ -123,7 +141,7 @@ impl WorkflowEngine { continue; } - if !trigger::should_fire(&wf.trigger) { + if !trigger::should_fire(&wf.trigger, since, now) { continue; } @@ -245,10 +263,34 @@ impl WorkflowEngine { parent_run_started_at: None, }; - // Persist the run + // Persist the run — concurrency check + insert in ONE closure (the + // single shared connection makes the closure atomic). The advisory + // check in check_triggers releases the lock before this insert; a + // manual HTTP trigger (which is atomic, api/workflows.rs) could land + // in that gap and put a limit=1 workflow at 2 concurrent runs. let r = run.clone(); + let limit = wf.concurrency_limit; + let wf_id_check = wf.id.clone(); let db = self.db().clone(); - db.with_conn(move |conn| crate::db::workflows::insert_run(conn, &r)).await?; + let inserted = db + .with_conn(move |conn| { + if let Some(max) = limit { + let active = crate::db::workflows::count_active_runs(conn, &wf_id_check)?; + if active >= max { + return Ok(false); + } + } + crate::db::workflows::insert_run(conn, &r)?; + Ok(true) + }) + .await?; + if !inserted { + tracing::info!( + "Workflow '{}' trigger skipped at insert — concurrency limit filled by a concurrent trigger", + wf.name + ); + return Ok(()); + } tracing::info!("Spawning workflow run {} for '{}'", run.id, wf.name); diff --git a/backend/src/workflows/runner.rs b/backend/src/workflows/runner.rs index de492798..0d407487 100644 --- a/backend/src/workflows/runner.rs +++ b/backend/src/workflows/runner.rs @@ -209,11 +209,20 @@ pub async fn execute_run( let cancel_guard = crate::CancelGuard::insert(&state.cancel_registry, run.id.clone()); let cancel_token = cancel_guard.token.clone(); - // Update run status to Running + // Update run status to Running. `false` = the Cancelled-stickiness guard + // blocked the write: the user cancelled in the window between our caller + // claiming the run (insert / gate-resume claim) and this line. Without + // this check the write resurrected a Cancelled row to Running (fresh + // token, never cancelled) and the run executed to completion. run.status = RunStatus::Running; let snap = crate::db::workflows::RunProgressSnapshot::from_run(run); let db2 = db.clone(); - db2.with_conn(move |conn| crate::db::workflows::update_run_progress(conn, snap)).await?; + let claimed = db2.with_conn(move |conn| crate::db::workflows::update_run_progress(conn, snap)).await?; + if !claimed { + tracing::info!("Run {} was cancelled (or deleted) before execution started — aborting", run.id); + run.status = RunStatus::Cancelled; + return Ok(()); + } // Resolve project + companion-repo context. Same pattern as the // audit pipeline (api/audit/full.rs:58-74): pre-format the @@ -324,6 +333,49 @@ pub async fn execute_run( None }; + // TD-20260709 (A) — a run about to execute in the MAIN checkout takes a + // per-project exclusivity guard (isolated worktree runs skip it). Held + // for this execution's lifetime; the checkpoint-reset preflight covers + // the deferred gate-pause window. + let _main_tree_guard = if workspace.is_none() && !project_path.is_empty() { + match crate::workflows::workspace::MainTreeGuard::acquire(&project_path, &run.id) { + Ok(g) => Some(g), + Err(holder) => { + let msg = format!( + "Refusing to run in the main checkout: run {holder} is already executing there \ + (two non-isolated runs would contaminate each other's files and .kronn/ state). \ + Wait for it, or enable worktree isolation on one of the workflows." + ); + run.status = RunStatus::Failed; + run.step_results.push(StepResult { + step_name: "__workspace__".to_string(), + status: RunStatus::Failed, + output: msg.clone(), + tokens_used: 0, + duration_ms: 0, + started_at: None, + condition_result: None, + envelope_detected: None, + step_kind: Some("Preflight".into()), + step_api_plugin_slug: None, + step_api_endpoint_path: None, + is_rollback: false, + child_run_id: None, + step_agent: None, + step_model: None, + }); + run.finished_at = Some(Utc::now()); + let snap = crate::db::workflows::RunProgressSnapshot::from_run(run); + let db_g = db.clone(); + db_g.with_conn(move |conn| crate::db::workflows::update_run_progress(conn, snap)).await?; + emit(RunEvent::RunError { error: msg }).await; + return Ok(()); + } + } + } else { + None + }; + // Determine working directory let work_dir = workspace.as_ref() .map(|ws| ws.path.to_string_lossy().to_string()) @@ -341,7 +393,13 @@ pub async fn execute_run( // (npm install, env preparation, etc.) the operator didn't ask for. if !is_resume && !is_inherited_workspace { if let Some(ref ws) = workspace { - let _ = ws.before_run().await; + if let Err(e) = ws.before_run().await { + tracing::warn!( + run_id = %run.id, + error = %e, + "before_run hook could not be executed — workspace may be unprepared" + ); + } } } @@ -873,10 +931,21 @@ pub async fn execute_run( "gate_checkpoint_before skipped — workflow uses Isolated worktree mode", ); } else if let Some(pid) = workflow.project_id.as_ref() { - let project_path_opt = state.db.with_conn({ + let project_path_opt = match state.db.with_conn({ let pid2 = pid.clone(); move |conn| crate::db::projects::get_project(conn, &pid2) - }).await.ok().flatten(); + }).await { + Ok(p) => p, + Err(e) => { + tracing::warn!( + run_id = %run.id, + step = %step.name, + error = %e, + "project lookup failed — gate checkpoint commit skipped" + ); + None + } + }; if let Some(proj) = project_path_opt { let ckp = super::gate_checkpoint::commit_checkpoint( std::path::Path::new(&proj.path), @@ -1499,7 +1568,13 @@ pub async fn execute_run( // Run after_run hook (skip when paused — the run isn't done yet). if !paused_for_approval { if let Some(ref ws) = workspace { - let _ = ws.after_run().await; + if let Err(e) = ws.after_run().await { + tracing::warn!( + run_id = %run.id, + error = %e, + "after_run hook could not be executed" + ); + } } } @@ -1550,6 +1625,20 @@ pub async fn execute_run( ); for rb_step in &workflow.on_failure { + // The ⏹ cancel was only raced against the PRIMARY step loop — + // a cancel during a long rollback chain was acknowledged in the + // UI while the remaining compensation agents kept executing (and + // the terminal write then flipped the row back to Failed; now + // blocked anyway by the Cancelled-stickiness guard). + if cancel_token.is_cancelled() { + tracing::info!( + target: "kronn::workflow_rollback", + run_id = %run.id, + "Cancel requested — abandoning remaining rollback steps" + ); + run.status = RunStatus::Cancelled; + break; + } emit(RunEvent::StepStart { step_name: rb_step.name.clone(), step_index: run.step_results.len(), @@ -1693,9 +1782,15 @@ pub async fn execute_run( }); let snap = crate::db::workflows::RunProgressSnapshot::from_run(run); let db = state.db.clone(); - let _ = db.with_conn(move |conn| { + if let Err(e) = db.with_conn(move |conn| { crate::db::workflows::update_run_progress(conn, snap) - }).await; + }).await { + tracing::error!( + run_id = %run.id, + error = %e, + "failed to persist produced_branches — the preserved branch pointer is the only record of this run's work" + ); + } } } Err(e) => { @@ -1870,22 +1965,37 @@ pub async fn resume_run( &run.id, workflow.workspace_config.as_ref().map(|c| c.hooks.clone()), ); - if let Ok(outcome) = ws.cleanup().await { - if let Some(preserved) = outcome.preserved { - // Reject still preserves anything the agent committed — - // the operator's "no" is on the gate, not on the work - // already on disk. They may want to recover it. - run.produced_branches.push(crate::models::ProducedBranch { - branch_name: preserved.branch_name, - head_sha: preserved.head_sha, - ahead: preserved.ahead, - pushed_upstream: preserved.pushed_upstream, - }); - let snap = crate::db::workflows::RunProgressSnapshot::from_run(run); - let db = state.db.clone(); - let _ = db.with_conn(move |conn| { - crate::db::workflows::update_run_progress(conn, snap) - }).await; + match ws.cleanup().await { + Ok(outcome) => { + if let Some(preserved) = outcome.preserved { + // Reject still preserves anything the agent committed — + // the operator's "no" is on the gate, not on the work + // already on disk. They may want to recover it. + run.produced_branches.push(crate::models::ProducedBranch { + branch_name: preserved.branch_name, + head_sha: preserved.head_sha, + ahead: preserved.ahead, + pushed_upstream: preserved.pushed_upstream, + }); + let snap = crate::db::workflows::RunProgressSnapshot::from_run(run); + let db = state.db.clone(); + if let Err(e) = db.with_conn(move |conn| { + crate::db::workflows::update_run_progress(conn, snap) + }).await { + tracing::error!( + run_id = %run.id, + error = %e, + "failed to persist produced_branches on Reject — the preserved branch pointer is the only record of this run's work" + ); + } + } + } + Err(e) => { + tracing::warn!( + run_id = %run.id, + error = %e, + "workspace cleanup failed on Reject — worktree may be left behind" + ); } } } @@ -1957,9 +2067,20 @@ pub async fn resume_run( if let Some(sha) = run.state.get(&checkpoint_key).cloned() { if let Some(pid) = workflow.project_id.as_ref() { let pid2 = pid.clone(); - let project = state.db.with_conn(move |conn| { + let project = match state.db.with_conn(move |conn| { crate::db::projects::get_project(conn, &pid2) - }).await.ok().flatten(); + }).await { + Ok(p) => p, + Err(e) => { + tracing::warn!( + run_id = %run.id, + gate = %gate_step_name, + error = %e, + "project lookup failed — checkpoint reset skipped before Goto" + ); + None + } + }; if let Some(proj) = project { match super::gate_checkpoint::reset_to_checkpoint( std::path::Path::new(&proj.path), diff --git a/backend/src/workflows/steps.rs b/backend/src/workflows/steps.rs index 35ca0bff..3abf63cc 100644 --- a/backend/src/workflows/steps.rs +++ b/backend/src/workflows/steps.rs @@ -255,7 +255,13 @@ pub async fn execute_step( ); let mut final_validation_error: Option = validation_error.clone(); let mut repair_valid = false; - if let Ok(repair_output) = run_agent_with_timeout(step, project_path, work_dir, &repair_prompt, tokens_config, full_access, model_tiers, None).await { + let repair_res = run_agent_with_timeout(step, project_path, work_dir, &repair_prompt, tokens_config, full_access, model_tiers, None).await; + if let Err(ref e) = repair_res { + // The repair RUN itself failed (spawn/timeout) — + // distinct from "repair produced invalid output". + tracing::warn!("Step '{}': repair run failed: {}", step.name, e); + } + if let Ok(repair_output) = repair_res { total_tokens += repair_output.tokens_used; let repaired_env = crate::workflows::template::extract_step_envelope(&repair_output.text); let repaired_error = match (&step.output_format, &repaired_env) { @@ -301,7 +307,16 @@ pub async fn execute_step( "local schema validation failed after repair — escalating to Claude" ); let escalated = escalation_step(step); - if let Ok(esc) = run_agent_with_timeout(&escalated, project_path, work_dir, &prompt, tokens_config, full_access, model_tiers, None).await { + let esc_res = run_agent_with_timeout(&escalated, project_path, work_dir, &prompt, tokens_config, full_access, model_tiers, None).await; + if let Err(ref e) = esc_res { + tracing::warn!( + target: "kronn::ollama::escalation", + step = %step.name, + error = %e, + "escalation run itself failed (spawn/timeout) — falling through to on_invalid handling" + ); + } + if let Ok(esc) = esc_res { total_tokens += esc.tokens_used; let esc_env = crate::workflows::template::extract_step_envelope(&esc.text); let esc_error = match (&step.output_format, &esc_env) { @@ -419,8 +434,17 @@ pub async fn execute_step( ); if condition_action.is_none() && envelope_aware { if let Some(env) = crate::workflows::template::extract_step_envelope(&final_output) { - if env.status == "NO_RESULTS" && step.on_result.iter().any(|r| r.contains == "NO_RESULTS") { - condition_action = Some(ConditionAction::Stop); + if env.status == "NO_RESULTS" { + // Honor the action the author DECLARED on the + // matching rule (a NO_RESULTS → Goto("handle_empty") + // recovery step must run) — the old hardcoded Stop + // hijacked the branch whenever the agent emitted the + // envelope but forgot the [SIGNAL: …] line. + condition_action = step + .on_result + .iter() + .find(|r| r.contains == "NO_RESULTS") + .map(|r| r.action.clone()); } } } @@ -759,7 +783,7 @@ async fn drive_agent_to_output( line.clone() } else { output.push('\n'); - format!("\n{}", &line) + format!("\n{}", line) }; output.push_str(&line); if let Some(tx) = progress_tx { @@ -828,9 +852,11 @@ async fn drive_agent_to_output( stream_json_tokens } else { let (cleaned, count) = runner::parse_token_usage(agent, &output, &stderr_lines); - if count > 0 { - output = cleaned; - } + // Adopt the cleaned output unconditionally: some agents (GeminiCli) + // return count=0 but still strip real noise (MCP handshake markers, + // "[MCP error]…" lines) — gating on count>0 silently re-injected that + // noise into the recorded step output and every {{steps.X.output}}. + output = cleaned; count }; diff --git a/backend/src/workflows/template.rs b/backend/src/workflows/template.rs index 4ff4a3b8..bce3819b 100644 --- a/backend/src/workflows/template.rs +++ b/backend/src/workflows/template.rs @@ -276,7 +276,7 @@ pub fn validate_step_references(steps: &[crate::models::WorkflowStep]) -> Result // know the actual JSON shape ahead of execution. static RE: std::sync::LazyLock = std::sync::LazyLock::new(|| { regex_lite::Regex::new( - r"\{\{\s*(?:steps\.([A-Za-z0-9_\-]+)\.(data|summary|status|data_json)(?:\.[A-Za-z0-9_\-]+)*|previous_step\.(data|summary|status|data_json)(?:\.[A-Za-z0-9_\-]+)*)\s*\}\}" + r"\{\{\s*(?:steps\.([A-Za-z0-9_\-]+)\.(data|summary|status|data_json|output)((?:\.[A-Za-z0-9_\-]+)*)|previous_step\.(data|summary|status|data_json|output)((?:\.[A-Za-z0-9_\-]+)*))\s*\}\}" ).unwrap() }); @@ -315,9 +315,22 @@ pub fn validate_step_references(steps: &[crate::models::WorkflowStep]) -> Result if let (Some(name), Some(field)) = (caps.get(1), caps.get(2)) { let target_name = name.as_str(); let target_field = field.as_str(); + // `.output` is raw text — a nested subpath can never resolve + // and would ship the literal placeholder into the prompt. + if target_field == "output" && caps.get(3).map(|m| !m.as_str().is_empty()).unwrap_or(false) { + errors.push(format!( + "Étape '{}' référence {{{{steps.{}.output{}}}}} — `.output` est du texte brut, il n'a pas de sous-chemin. Utilise {{{{steps.{}.output}}}} tel quel, ou passe par .data pour du JSON structuré.", + step.name, target_name, caps.get(3).map(|m| m.as_str()).unwrap_or(""), target_name + )); + continue; + } // Look only in strictly-upstream steps (can't read self or future) let upstream = steps.iter().take(idx).find(|s| s.name == target_name); match upstream { + // `.output` is the RAW text — any upstream format is fine; + // only existence/ordering matter (a typo'd name used to + // ship the literal placeholder into the prompt, silently). + Some(_) if target_field == "output" => {} Some(target) if produces_structured(target) => {} Some(target) => errors.push(format!( "Étape '{}' référence {{{{steps.{}.{}}}}}, mais l'étape '{}' est en output_format: FreeText. Passe-la en Structured pour qu'elle expose .data / .summary / .status.", @@ -338,8 +351,15 @@ pub fn validate_step_references(steps: &[crate::models::WorkflowStep]) -> Result } } } - } else if let Some(field) = caps.get(3) { + } else if let Some(field) = caps.get(4) { let target_field = field.as_str(); + if target_field == "output" && caps.get(5).map(|m| !m.as_str().is_empty()).unwrap_or(false) { + errors.push(format!( + "Étape '{}' référence {{{{previous_step.output{}}}}} — `.output` est du texte brut, il n'a pas de sous-chemin.", + step.name, caps.get(5).map(|m| m.as_str()).unwrap_or("") + )); + continue; + } if idx == 0 { errors.push(format!( "Étape '{}' utilise {{{{previous_step.{}}}}} mais c'est la première étape du workflow — il n'y a pas de précédente.", @@ -347,7 +367,7 @@ pub fn validate_step_references(steps: &[crate::models::WorkflowStep]) -> Result )); } else { let prev = &steps[idx - 1]; - if !produces_structured(prev) { + if target_field != "output" && !produces_structured(prev) { errors.push(format!( "Étape '{}' utilise {{{{previous_step.{}}}}}, mais l'étape précédente '{}' est en output_format: FreeText. Passe-la en Structured.", step.name, target_field, prev.name @@ -783,7 +803,10 @@ fn value_type_name(value: &serde_json::Value) -> &'static str { /// Try to extract a `---STEP_OUTPUT--- ... ---END_STEP_OUTPUT---` envelope from raw text. pub fn extract_step_envelope(text: &str) -> Option { - // Strategy 1: delimited block + // Strategy 1: delimited block. When markers are PRESENT they are + // authoritative — a malformed block returns None (→ repair) instead of + // falling through to strategy 2, which could adopt a quoted example + // from the agent's own reasoning. if let Some(start) = text.find("---STEP_OUTPUT---") { let after_delim = &text[start + "---STEP_OUTPUT---".len()..]; if let Some(end) = after_delim.find("---END_STEP_OUTPUT---") { @@ -800,41 +823,67 @@ pub fn extract_step_envelope(text: &str) -> Option { return envelope_from_json(&parsed); } } + return None; } - // Strategy 2: find last JSON object with "data" and "status" fields - let mut last_match = None; - for (i, _) in text.rmatch_indices('{') { - let candidate = &text[i..]; - // Find the matching closing brace + // Strategy 2 (legacy, marker-less — Batch* structured output): the LAST + // top-level parsable JSON object of the text must ITSELF be a valid + // envelope. No walking back to an earlier envelope-like object. + let mut last_json: Option = None; + let mut i = 0; + while let Some(rel) = text[i..].find('{') { + let start = i + rel; let mut depth = 0i32; - let mut end_idx = 0; - for (j, ch) in candidate.char_indices() { + let mut end = None; + // String-aware balancing: braces inside JSON strings don't count + // (`{"data": "brace } inside", ...}` must balance at the real end). + let mut in_string = false; + let mut escaped = false; + for (j, ch) in text[start..].char_indices() { + if escaped { + escaped = false; + continue; + } match ch { - '{' => depth += 1, - '}' => { + '\\' if in_string => escaped = true, + '"' => in_string = !in_string, + '{' if !in_string => depth += 1, + '}' if !in_string => { depth -= 1; - if depth == 0 { end_idx = j + 1; break; } + if depth == 0 { end = Some(start + j + 1); break; } } _ => {} } } - if end_idx == 0 { continue; } - let json_candidate = &candidate[..end_idx]; - if let Ok(parsed) = serde_json::from_str::(json_candidate) { - if parsed.get("data").is_some() && parsed.get("status").is_some() { - last_match = envelope_from_json(&parsed); - break; // found the last (rightmost) valid envelope + match end { + Some(e) => { + if let Ok(v) = serde_json::from_str::(&text[start..e]) { + last_json = Some(v); + i = e; // nested braces of a parsed object are not top-level candidates + } else { + i = start + 1; + } } + None => break, } } - - last_match + last_json + .filter(|v| v.get("data").is_some() && v.get("status").is_some()) + .as_ref() + .and_then(envelope_from_json) } fn envelope_from_json(val: &serde_json::Value) -> Option { let data = val.get("data")?; - let status = val.get("status")?.as_str().unwrap_or("OK"); + // The contract says `status` is a string. A present-but-non-string value + // must NOT read as OK: `"status": null` is a model failing mid-envelope — + // coercing that to success inverted the failure direction. Numbers keep + // their text form (`200` → "200") so contains-rules still match them. + let status = match val.get("status")? { + serde_json::Value::String(s) => s.clone(), + serde_json::Value::Number(n) => n.to_string(), + _ => "ERROR".to_string(), + }; let summary = val.get("summary").and_then(|s| s.as_str()).unwrap_or(""); // `data`: unwrap strings for clean prompt interpolation. @@ -848,7 +897,7 @@ fn envelope_from_json(val: &serde_json::Value) -> Option { Some(StepEnvelope { data: data_str, - status: status.to_string(), + status, summary: summary.to_string(), data_json, }) @@ -1377,6 +1426,82 @@ mod tests { } } + #[test] + fn strategy2_only_accepts_the_last_toplevel_json_as_envelope() { + // Codex acceptance matrix for the bounded strategy-2 fallback. + + // 1. Legacy bare envelope as the final JSON (Batch* shape) → OK. + let legacy = r#"some preamble {"data": [1, 2], "status": "OK", "summary": "s"}"#; + let env = extract_step_envelope(legacy).expect("legacy final envelope accepted"); + assert_eq!(env.status, "OK"); + + // 2. Markers present but malformed block → None, even with a valid + // bare envelope elsewhere (markers are authoritative → repair). + let marked_bad = r#"quoted example: {"data": "x", "status": "OK"} +---STEP_OUTPUT--- +{"data": [1,], "status": "OK" +---END_STEP_OUTPUT---"#; + assert!(extract_step_envelope(marked_bad).is_none(), "malformed marked block must not fall back"); + + // 3. Earlier envelope-like JSON + LATER non-envelope JSON → None + // (the earlier one may be a quoted example from the reasoning). + let example_then_other = r#"I will emit {"data": [], "status": "OK"} at the end. Result: {"count": 3}"#; + assert!(extract_step_envelope(example_then_other).is_none(), "must not walk back past the last JSON"); + + // 4. Earlier non-envelope JSON + FINAL valid envelope → OK. + let other_then_envelope = r#"stats: {"count": 3} then {"data": {"items": [1]}, "status": "OK"}"#; + let env = extract_step_envelope(other_then_envelope).expect("final envelope accepted"); + assert_eq!(env.status, "OK"); + + // 5. Codex durcissement: braces inside JSON strings must not break + // the balancing (string-aware scanner). + let brace_in_string = r#"done: {"data": "brace } inside string", "status": "OK"}"#; + let env = extract_step_envelope(brace_in_string).expect("string-aware balancing"); + assert_eq!(env.data, "brace } inside string"); + let escaped_quote = r#"x: {"data": "quote \" then } brace", "status": "OK"}"#; + assert!(extract_step_envelope(escaped_quote).is_some(), "escaped quotes handled"); + } + + #[test] + fn validate_output_refs_check_existence_and_ordering_only() { + use crate::models::StepOutputFormat::FreeText; + // A typo'd `.output` used to pass BOTH validation layers and ship + // the literal placeholder into the agent prompt. + + // Valid upstream .output on a FreeText producer → OK (raw-text contract). + let ok = vec![step("a", "do things", FreeText), step("b", "resume: {{steps.a.output}}", FreeText)]; + assert!(validate_step_references(&ok).is_ok()); + // previous_step.output on a FreeText predecessor → OK too. + let ok2 = vec![step("a", "do", FreeText), step("b", "resume: {{previous_step.output}}", FreeText)]; + assert!(validate_step_references(&ok2).is_ok()); + + // Typo'd step name → save-time error. + let typo = vec![step("a", "do", FreeText), step("b", "resume: {{steps.typo.output}}", FreeText)]; + let errs = validate_step_references(&typo).unwrap_err(); + assert!(errs[0].contains("aucune étape ne porte le nom"), "{errs:?}"); + + // Self-reference → error. + let selfref = vec![step("a", "loop: {{steps.a.output}}", FreeText)]; + assert!(validate_step_references(&selfref).is_err()); + + // Forward reference → error. + let fwd = vec![step("a", "peek: {{steps.b.output}}", FreeText), step("b", "do", FreeText)]; + let errs = validate_step_references(&fwd).unwrap_err(); + assert!(errs[0].contains("pas exécutée avant"), "{errs:?}"); + + // previous_step.output on the FIRST step → error. + let first = vec![step("a", "{{previous_step.output}}", FreeText)]; + assert!(validate_step_references(&first).is_err()); + + // Codex blocker: a nested subpath on raw .output can never resolve. + let nested = vec![step("a", "do", FreeText), step("b", "x: {{steps.a.output.foo}}", FreeText)]; + let errs = validate_step_references(&nested).unwrap_err(); + assert!(errs[0].contains("texte brut"), "{errs:?}"); + let nested_prev = vec![step("a", "do", FreeText), step("b", "x: {{previous_step.output.foo}}", FreeText)]; + let errs = validate_step_references(&nested_prev).unwrap_err(); + assert!(errs[0].contains("texte brut"), "{errs:?}"); + } + #[test] fn validate_ok_when_upstream_is_structured() { use crate::models::StepOutputFormat::*; diff --git a/backend/src/workflows/tracker/github.rs b/backend/src/workflows/tracker/github.rs index 5313c26e..27494226 100644 --- a/backend/src/workflows/tracker/github.rs +++ b/backend/src/workflows/tracker/github.rs @@ -42,7 +42,14 @@ impl GitHubTracker { owner, repo, token, - client: reqwest::Client::new(), + // Bounded: this client is awaited INLINE from the engine's single + // tick loop — an unbounded hang here stalls every cron and tracker + // in the system until restart, not just this poll. + client: reqwest::Client::builder() + .connect_timeout(std::time::Duration::from_secs(5)) + .timeout(std::time::Duration::from_secs(30)) + .build() + .unwrap_or_default(), } } diff --git a/backend/src/workflows/trigger.rs b/backend/src/workflows/trigger.rs index 958eea4f..0549016b 100644 --- a/backend/src/workflows/trigger.rs +++ b/backend/src/workflows/trigger.rs @@ -5,24 +5,32 @@ //! - Manual: always returns false (triggered via API only) use std::str::FromStr; -use chrono::Utc; +use chrono::{DateTime, Utc}; use crate::models::*; -/// Check if a trigger should fire right now (within a 30s window). -pub fn should_fire(trigger: &WorkflowTrigger) -> bool { +/// Did the trigger have an occurrence in the window `(since, now]`? +/// +/// The engine passes the previous tick's timestamp as `since`, so each cron +/// occurrence fires EXACTLY ONCE regardless of tick jitter. The old stateless +/// version fired on "next occurrence within 30s of now": with 30s ticks that +/// window ([0s, 31s) after truncation) overlapped itself — an occurrence +/// landing on the seam fired on BOTH surrounding ticks (two concurrent runs +/// of the same cron, ~1 occurrence in 31), and a tick delayed past the window +/// (slow tracker poll starving the loop) silently skipped the occurrence. +pub fn should_fire(trigger: &WorkflowTrigger, since: DateTime, now: DateTime) -> bool { match trigger { - WorkflowTrigger::Cron { schedule } => check_cron(schedule), + WorkflowTrigger::Cron { schedule } => cron_fires_between(schedule, since, now), WorkflowTrigger::Tracker { interval, .. } => { // Tracker uses interval as a cron expression for polling frequency - check_cron(interval) + cron_fires_between(interval, since, now) } WorkflowTrigger::Manual => false, } } -/// Check if a cron expression matches within the current 30s window. -fn check_cron(cron_expr: &str) -> bool { +/// True when the cron expression has an occurrence in `(since, now]`. +fn cron_fires_between(cron_expr: &str, since: DateTime, now: DateTime) -> bool { // cron crate expects 6 fields (with seconds), add "0 " prefix if 5 fields let expr = if cron_expr.split_whitespace().count() < 6 { format!("0 {}", cron_expr) @@ -31,15 +39,11 @@ fn check_cron(cron_expr: &str) -> bool { }; match cron::Schedule::from_str(&expr) { - Ok(schedule) => { - let now = Utc::now(); - if let Some(next) = schedule.upcoming(Utc).next() { - let diff = (next - now).num_seconds(); - (0..=30).contains(&diff) - } else { - false - } - } + Ok(schedule) => schedule + .after(&since) + .next() + .map(|occ| occ <= now) + .unwrap_or(false), Err(e) => { tracing::error!("Invalid cron expression '{}': {}", cron_expr, e); false @@ -50,79 +54,96 @@ fn check_cron(cron_expr: &str) -> bool { #[cfg(test)] mod tests { use super::*; + use chrono::Duration; // ─── Manual trigger ────────────────────────────────────────────────── #[test] fn manual_trigger_never_fires() { - assert!(!should_fire(&WorkflowTrigger::Manual)); + let now = Utc::now(); + assert!(!should_fire(&WorkflowTrigger::Manual, now - Duration::seconds(30), now)); } // ─── Cron trigger ──────────────────────────────────────────────────── + fn fires(expr: &str, since: DateTime, now: DateTime) -> bool { + cron_fires_between(expr, since, now) + } + #[test] fn invalid_cron_expression_returns_false() { - assert!(!check_cron("not a cron")); + let now = Utc::now(); + assert!(!fires("not a cron", now - Duration::seconds(30), now)); + assert!(!fires("", now - Duration::seconds(30), now)); + assert!(!fires("99 99 99 99 99", now - Duration::seconds(30), now)); + } + + #[test] + fn occurrence_inside_window_fires() { + // Deterministic: pick a fixed occurrence and build windows around it. + // "0 0 7 * * *" = every day at 07:00:00. + let occ = "2026-07-09T07:00:00Z".parse::>().unwrap(); + assert!(fires("0 0 7 * * *", occ - Duration::seconds(30), occ), + "occurrence exactly at `now` fires (window is right-inclusive)"); + assert!(fires("0 0 7 * * *", occ - Duration::seconds(10), occ + Duration::seconds(20)), + "occurrence strictly inside the window fires"); } #[test] - fn invalid_cron_expression_empty_returns_false() { - assert!(!check_cron("")); + fn occurrence_fires_exactly_once_across_adjacent_windows() { + // THE double-fire regression (concurrency review, 0.8.11): with the + // old "next within [0,31)s of now" logic an occurrence at the seam + // fired on both surrounding 30s ticks. Windows are half-open + // (since, now] so adjacent windows partition time. + let occ = "2026-07-09T07:00:00Z".parse::>().unwrap(); + let tick1_start = occ - Duration::milliseconds(30_400); // occ − 30.4s + let tick1_end = tick1_start + Duration::seconds(30); // occ − 0.4s + let tick2_end = tick1_end + Duration::seconds(30); // occ + 29.6s + let in_first = fires("0 0 7 * * *", tick1_start, tick1_end); + let in_second = fires("0 0 7 * * *", tick1_end, tick2_end); + assert!(!in_first, "occurrence is after the first window's end"); + assert!(in_second, "…and fires in the second window"); } #[test] - fn cron_far_future_does_not_fire() { - // "0 0 31 2 *" = Feb 31st, which never occurs — next occurrence - // will be far in the future (or never), so should not fire within 30s. - // Use a schedule that is guaranteed to be far away: - // "0 0 1 1 *" = Jan 1st at midnight — unless we happen to be running - // at exactly that moment, this should return false. - // Instead, use a trick: schedule in the past minute but not this 30s window. - // Safest test: an always-invalid expression. - assert!(!check_cron("99 99 99 99 99")); + fn delayed_tick_still_catches_the_occurrence() { + // Tick starvation (slow tracker poll): the next evaluation happens + // 90s late — the occurrence must STILL fire (old logic skipped it). + let occ = "2026-07-09T07:00:00Z".parse::>().unwrap(); + assert!(fires("0 0 7 * * *", occ - Duration::seconds(30), occ + Duration::seconds(90))); + } + + #[test] + fn no_occurrence_in_window_does_not_fire() { + let occ = "2026-07-09T07:00:00Z".parse::>().unwrap(); + assert!(!fires("0 0 7 * * *", occ + Duration::seconds(1), occ + Duration::seconds(31))); } #[test] fn cron_five_field_expression_gets_seconds_prefix() { - // Verify that a 5-field expression doesn't panic/error. - // It may or may not fire depending on current time, but must not crash. - let _result = check_cron("* * * * *"); - // No panic = pass. The result depends on timing. + // "* * * * *" = every minute; a 61s window always contains one. + let now = Utc::now(); + assert!(fires("* * * * *", now - Duration::seconds(61), now)); } #[test] fn cron_six_field_expression_accepted() { - // 6-field expression (with seconds) should not panic. - let _result = check_cron("0 * * * * *"); + let now = Utc::now(); + assert!(fires("0 * * * * *", now - Duration::seconds(61), now)); } // ─── should_fire dispatch ──────────────────────────────────────────── #[test] fn should_fire_cron_invalid_returns_false() { - let trigger = WorkflowTrigger::Cron { - schedule: "invalid cron".into(), - }; - assert!(!should_fire(&trigger)); - } - - #[test] - fn should_fire_tracker_invalid_interval_returns_false() { - let trigger = WorkflowTrigger::Tracker { - source: TrackerSourceConfig::GitHub { - owner: "test".into(), - repo: "test".into(), - }, - query: "label:bug".into(), - labels: vec!["bug".into()], - interval: "invalid".into(), - }; - assert!(!should_fire(&trigger)); + let now = Utc::now(); + let trigger = WorkflowTrigger::Cron { schedule: "invalid cron".into() }; + assert!(!should_fire(&trigger, now - Duration::seconds(30), now)); } #[test] fn should_fire_tracker_uses_interval_as_cron() { - // A tracker trigger with a valid but far-future cron should not fire. + let now = Utc::now(); let trigger = WorkflowTrigger::Tracker { source: TrackerSourceConfig::GitHub { owner: "owner".into(), @@ -130,10 +151,15 @@ mod tests { }, query: "".into(), labels: vec![], - interval: "0 0 1 1 *".into(), // Jan 1st midnight — unlikely to match now + interval: "* * * * *".into(), + }; + assert!(should_fire(&trigger, now - Duration::seconds(61), now)); + let invalid = WorkflowTrigger::Tracker { + source: TrackerSourceConfig::GitHub { owner: "o".into(), repo: "r".into() }, + query: "".into(), + labels: vec![], + interval: "invalid".into(), }; - // This should not fire (unless test runs at exactly Jan 1 00:00) - // Main goal: no panic, correct dispatch path - let _result = should_fire(&trigger); + assert!(!should_fire(&invalid, now - Duration::seconds(61), now)); } } diff --git a/backend/src/workflows/workspace.rs b/backend/src/workflows/workspace.rs index de90176a..b22fa50a 100644 --- a/backend/src/workflows/workspace.rs +++ b/backend/src/workflows/workspace.rs @@ -11,6 +11,45 @@ use crate::core::cmd::async_cmd; use crate::models::WorkspaceHooks; /// An active workspace (git worktree) for a workflow run. +/// TD-20260709 (A) — exclusivity of a project's MAIN checkout across +/// non-isolated runs: two of them cross-contaminate `.kronn/` machine files +/// and each other's edits. Isolated (worktree) runs never take this lock. +pub struct MainTreeGuard { + key: String, +} + +fn main_tree_locks() -> &'static std::sync::Mutex> { + static LOCKS: std::sync::OnceLock>> = + std::sync::OnceLock::new(); + LOCKS.get_or_init(Default::default) +} + +impl MainTreeGuard { + /// `None` = another run already owns this project's main tree; the + /// caller must refuse to run (holder's run id is returned for the error). + pub fn acquire(project_path: &str, run_id: &str) -> Result { + // Canonical key: two spellings of the same checkout (symlink, + // relative) must not bypass the mutex. + let key = std::fs::canonicalize(project_path) + .map(|p| p.to_string_lossy().to_string()) + .unwrap_or_else(|_| project_path.to_string()); + let mut locks = main_tree_locks().lock().unwrap_or_else(|p| p.into_inner()); + if let Some(holder) = locks.get(&key) { + return Err(holder.clone()); + } + locks.insert(key.clone(), run_id.to_string()); + Ok(Self { key }) + } +} + +impl Drop for MainTreeGuard { + fn drop(&mut self) { + if let Ok(mut locks) = main_tree_locks().lock() { + locks.remove(&self.key); + } + } +} + pub struct Workspace { /// Path to the worktree directory pub path: PathBuf, @@ -306,12 +345,29 @@ impl Workspace { if let Some(cmd) = cmd { tracing::info!("Running workspace hook '{}': {}", hook_name, cmd); - let output = async_cmd("sh") + // Bounded: a hung hook (e.g. `npm ci` against a dead registry) + // would otherwise pin the run and its concurrency slot forever — + // this await is outside the cancel race, so Stop can't reach it. + // `kill_on_drop` ensures the timed-out child doesn't leak. + const HOOK_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(600); + let mut command = async_cmd("sh"); + command .args(["-c", cmd]) .current_dir(&self.path) - .output() - .await - .with_context(|| format!("Failed to run {} hook", hook_name))?; + .kill_on_drop(true); + let output = match tokio::time::timeout(HOOK_TIMEOUT, command.output()).await { + Ok(res) => res.with_context(|| format!("Failed to run {} hook", hook_name))?, + Err(_) => { + tracing::warn!( + "Hook '{}' timed out after {}s — killing it", + hook_name, HOOK_TIMEOUT.as_secs() + ); + return Err(anyhow::anyhow!( + "Failed to run {} hook: timed out after {}s", + hook_name, HOOK_TIMEOUT.as_secs() + )); + } + }; if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr); @@ -326,6 +382,23 @@ impl Workspace { #[cfg(test)] mod tests { + use super::MainTreeGuard; + + #[test] + fn main_tree_guard_is_exclusive_per_project_and_released_on_drop() { + let dir_a = tempfile::TempDir::new().unwrap(); + let dir_b = tempfile::TempDir::new().unwrap(); + let a = dir_a.path().to_string_lossy().to_string(); + let b = dir_b.path().to_string_lossy().to_string(); + let g1 = MainTreeGuard::acquire(&a, "run-1").expect("first acquire"); + let denied = MainTreeGuard::acquire(&a, "run-2"); + assert_eq!(denied.err().as_deref(), Some("run-1"), "second run must be refused, naming the holder"); + // A different project is unaffected. + let _other = MainTreeGuard::acquire(&b, "run-3").expect("other project free"); + drop(g1); + let _g2 = MainTreeGuard::acquire(&a, "run-2").expect("released on drop"); + } + use super::*; // ─── sanitize_name ─────────────────────────────────────────────────── diff --git a/backend/tests/api_tests.rs b/backend/tests/api_tests.rs index 9aa061cf..fc5a96e8 100644 --- a/backend/tests/api_tests.rs +++ b/backend/tests/api_tests.rs @@ -3988,7 +3988,7 @@ async fn disc_sync_request_resends_missing_messages() { /// contact (base64) and rejects an unknown caller. This is the binary-transfer /// leg of P2P file/doc recovery. #[tokio::test] -async fn fetch_file_serves_bytes_to_known_contact_and_rejects_unknown() { +async fn fetch_file_scoped_to_shared_disc_serves_bytes_and_rejects_unknown() { let state = test_state(); // A trusted contact (the caller authenticates with this invite code). let now = chrono::Utc::now(); @@ -4024,7 +4024,21 @@ async fn fetch_file_serves_bytes_to_known_contact_and_rejects_unknown() { ).map_err(|e| anyhow::anyhow!(e)) }).await.unwrap(); - // Known contact → bytes (base64 of "hello-doc-bytes"). + // 0.9 scoping: being a KNOWN contact is no longer enough — the file's + // discussion must be shared with the caller. Unshared → found: false. + let (st, json) = post_json( + build_router_with_auth(state.clone(), false), + "/api/disc/fetch-file", + serde_json::json!({ "file_id": "file1", "from_invite_code": "kronn:PeerAlpha@10.0.0.9:3140" }), + ).await; + assert_eq!(st, StatusCode::OK); + assert_eq!(json["data"]["found"], false, "unshared discussion must not leak files to a mere contact"); + assert!(json["data"]["data_base64"].is_null()); + + // Share the discussion with this contact → the bytes flow. + state.db.with_conn(|conn| { + kronn::db::discussions::update_discussion_sharing(conn, "d1", "sh-1", &["c1".to_string()]).map(|_| ()) + }).await.unwrap(); let (st, json) = post_json( build_router_with_auth(state.clone(), false), "/api/disc/fetch-file", diff --git a/docker-compose.yml b/docker-compose.yml index 3e6bf711..b18710b5 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -120,6 +120,7 @@ services: environment: - KRONN_TEMPLATES_DIR=/app/templates - KRONN_DATA_DIR=/data + - KRONN_IN_DOCKER=1 - KRONN_HOST_HOME=${HOME} # Path written into project-level `.mcp.json` / `.kiro/settings/…` # / `.gemini/settings.json` / `~/.codex/config.toml` for the diff --git a/docs/inconsistencies-tech-debt.md b/docs/inconsistencies-tech-debt.md index d39cd1a6..8df13ff7 100644 --- a/docs/inconsistencies-tech-debt.md +++ b/docs/inconsistencies-tech-debt.md @@ -45,7 +45,7 @@ | TD-20260630-workflow-run-restart-resilience | A workflow run in-flight when the backend restarts is lost: marked `Failed`, unprocessed items dropped, no resume, and no distinction from a real failure. Hit the PR-Review cron (4/27 PRs done, restart ~18:15 killed it mid-5th-child). Frequent under `cargo-watch` (any rebuild restarts the process). Fix: boot-time scan marks orphaned `Running`/`Pending` runs as a new `Interrupted` status (not `Failed`); then persist the foreach done-set + re-enqueue from checkpoint. Detail: `docs/tech-debt/TD-20260630-workflow-run-restart-resilience.md`. | Backend / Workflows | Medium | | TD-20260630-apicall-link-header-pagination | Workflow `ApiCall` auto-pagination (`walk_pages`) only handles object-envelope list APIs (`{issues:[…]}` + body `has_more`/`total`/`cursor`); it can't do GitHub-style **bare array + `Link` header** (`send_with_retry` drops headers; `detect_items_key` needs an object; `/reviews` ignores `sort`/`direction`). GitHub list calls silently truncate at one page — defeated the PR-Review `skip_check` dedup (re-reviewed PR 1800 every cron). Mitigated with `per_page=100` on fetch_reviews/comments/files (covers ≤100). Durable fix: surface the `Link` header + a `PaginationSpec::LinkHeader` walk that accumulates bare arrays. Detail: `docs/tech-debt/TD-20260630-apicall-link-header-pagination.md`. | Backend / Workflows | Low | | TD-20260629-e2e-seed-vs-real-db | Several Playwright E2E specs are calibrated on a freshly-seeded DB (a11y `a11y-baseline.json` element-count baselines, `audit-banner-lifecycle` "no audit in progress" precondition, 10 s introspection budget) → false reds against a real rich/stateful DB (proven on WSL pre-merge: 8/72 fail, identical on `main`). CI is unaffected (seeds fresh). Fix: state-relative a11y baselines, force a known audit state in setup, bump cold-first-call budget to ~20 s. | CI / Frontend | Low | -| TD-20260629-import-downgrade-wipe | `do_import_db` is a destructive full restore (DELETE all tables → re-insert from payload). Importing an export OLDER than the running version wipes tables absent from it (`quick_apis`/`learnings` default to `[]`) — a v3 import on a v4 box silently empties them. Fix: warn when `export.version < current` + don't clear tables the payload doesn't carry (selective clear / merge). | Backend | Medium | +| TD-20260629-import-downgrade-wipe | RESOLVED (0.8.11): downgrade no longer wipes (warning + selective clear). Residual: export fidelity gaps (`quick_prompt_versions`, context-file blobs, `learning_rejections`) — see the TD file. | Backend | Medium | | TD-20260701-typegen-generated-aggregate | `make typegen` no longer rebuilds `frontend/src/types/generated.ts` — ts-rs 12 writes per-type files to `backend/bindings/` (native `export type`, serde-default fields REQUIRED) but the committed aggregate uses `export interface` with serde-default fields OPTIONAL (`?`), and nothing folds bindings → aggregate (drifted since ~2026-06-25). A straight concat breaks ~189 call sites; a faithful generator must reproduce serde-default→optional (info only in Rust source, not the bindings). Interim: new fields hand-added to `generated.ts`. Detail: `docs/tech-debt/TD-20260701-typegen-generated-aggregate.md`. | CI / Tooling / Frontend | Medium | | TD-20260629-p2p-native-binding | Contacts/P2P can't work natively: native binds `127.0.0.1` (only Docker forces `0.0.0.0`), `network-info` advertised `127.0.0.1`/empty `detected_ips`, and contact status is reachability-only (no peer-side accept). Shipped: `KRONN_HOST` + UdpSocket LAN-IP detection + a Settings "Allow connections from other devices" toggle (`net_expose`, secure-by-default, Tauri restart button; desktop now honors `server.host`). Remaining: peer auth for non-localhost ops (beyond `/health`), WSL guidance (portproxy/Tailscale), optional live re-bind. Tailscale = recommended path. | Backend / Networking | Medium | diff --git a/docs/tech-debt/TD-20260629-import-downgrade-wipe.md b/docs/tech-debt/TD-20260629-import-downgrade-wipe.md index 2e4894f7..5db3e0fc 100644 --- a/docs/tech-debt/TD-20260629-import-downgrade-wipe.md +++ b/docs/tech-debt/TD-20260629-import-downgrade-wipe.md @@ -18,3 +18,12 @@ ## Notes - Surfaced 2026-06-29 while debugging "no QAs after import". Root cause was benign (the export was v3, made before `quick_apis` existed → re-export from the now-v4 source fixes it), but it exposed this destructive-downgrade footgun. The body-limit import bug fixed the same day is unrelated (`backend/src/lib.rs` `DefaultBodyLimit`). + + +## Residual 2026-07-12 (Codex audit, disc 3f603a34) + +The downgrade wipe itself is FIXED (warning + selective clear). What remains is +requalified as **export fidelity**, a separate concern from the original bug: +`build_export` does not cover `quick_prompt_versions`, on-disk `context_files` +blobs, or `learning_rejections` — a restore from export is not 100% faithful. +Track as its own work item. diff --git a/docs/tech-debt/TD-20260709-nonisolated-overlapping-runs.md b/docs/tech-debt/TD-20260709-nonisolated-overlapping-runs.md new file mode 100644 index 00000000..cdbaddb7 --- /dev/null +++ b/docs/tech-debt/TD-20260709-nonisolated-overlapping-runs.md @@ -0,0 +1,60 @@ +# TD — Overlapping non-isolated runs: main-tree cross-contamination + checkpoint reset + +**Date:** 2026-07-09 +**Origin:** 0.8.11 hardening pass — codebase-wide concurrency review (finding F5). +**Severity:** RISK (requires a specific, now-rarer setup — see mitigations shipped). +**Status:** OPEN — needs a design decision, not a hot-fix. + +## The problem + +When worktree creation fails (and `require_isolation` is off), a run falls back +to executing **in the project's main checkout** (`runner.rs` worktree-failure +fallback). Two overlapping runs in that mode share mutable state: + +1. **`.kronn/` machine files** — `manifest.json` (read by round-2 triage + hydration), `tasks.json`, `decision_ids.txt`, `files_touched.txt` are all + fixed paths under `work_dir`. Run A's triage can hydrate `unchanged` items + from run B's manifest; B's `tasks.json` overwrites A's, so A's foreach fans + out B's tasks. + +2. **Gate-checkpoint `git reset --hard`** — gate checkpointing only activates + in non-isolated mode (`runner.rs` skips it when `workspace_path.is_some()`). + A run paused at its gate holds a checkpoint on the **shared** checkout; when + the operator later picks *Request Changes*, `reset_to_checkpoint(proj.path)` + hard-resets the main tree — wiping any uncommitted work another run (or the + human) did there since the checkpoint, potentially **hours later**. The + `StagedChangesPresent` guard exists only at *commit* time, not at reset time. + +## Why it's rarer after 0.8.11 + +- The cron double-fire (two concurrent runs of the same workflow ~1/31 + occurrences) is fixed — the most likely source of surprise overlap is gone. +- The engine's concurrency check is now atomic with the insert, so + `concurrency_limit: 1` is actually enforced against racing triggers. +- Cancelled is sticky and Pending/WaitingApproval runs are cancellable, so an + operator can reliably kill one of two overlapping runs. + +Remaining exposure: two *different* workflows bound to the same project, both +falling back to main-tree mode (or one manual + one cron), plus a human working +in the checkout while a gate-paused run holds a checkpoint. + +## Options (pick one) + +| Option | Sketch | Cost | +|---|---|---| +| A. Per-project main-tree mutex | An in-process `Mutex>`: a run entering main-tree mode acquires it; a second run either queues or fails fast with a clear error. | Small. Doesn't cover the human-WIP case. | +| B. Namespace `.kronn/` per run | `.kronn/runs//…` for machine files; triage reads its own run's dir. | Medium — every reader/writer of the fixed paths moves. Fixes contamination, not the reset. | +| C. Guard the reset | Before `reset_to_checkpoint`, refuse (→ COMMENT-style degrade) if the tree has changes NOT introduced by this run (diff vs checkpoint SHA on files the run touched). | Medium. Fixes the destructive half only. | +| D. Kill main-tree fallback | Make `require_isolation` the only mode; a failed worktree creation fails the run. | Smallest code, biggest behavior change (breaks setups where worktrees can't be created — exotic filesystems). | + +**Recommendation:** A + C (cheap, orthogonal, cover both halves); B if/when +`.kronn/` machine files grow more consumers. + +## Pointers + +- `backend/src/workflows/runner.rs` — fallback (~:315), triage machine files + (~:1206), checkpoint commit (~:875) and reset (~:1957). +- `backend/src/workflows/gate_checkpoint.rs` — `reset_to_checkpoint`, + `StagedChangesPresent` (commit-time only). +- `backend/src/workflows/workspace.rs` — worktree naming (run-id suffixed — + the isolated path is already collision-free). diff --git a/frontend/e2e/README.md b/frontend/e2e/README.md index b7c12638..49b5d920 100644 --- a/frontend/e2e/README.md +++ b/frontend/e2e/README.md @@ -12,6 +12,30 @@ make test-e2e-ui # UI Playwright (debug visuel, headed) Vite dev server est auto-spawné par Playwright (cf. `playwright.config.ts::webServer`). Le backend doit tourner séparément. +### Navigateurs Playwright (⚠ après chaque bump de version) + +`pnpm test:e2e` installe automatiquement le binaire requis (via +`e2e/ensure-browser.mjs` — skippé en CI où les navigateurs sont pré-cuits dans +l'image conteneur). Si tu lances `npx playwright test` +directement après un bump de `@playwright/test`, chaque spec échoue en 0ms avec +`Executable doesn't exist … chromium_headless_shell-` — ce n'est PAS un bug +de l'app : chaque version de Playwright épingle sa propre révision de navigateur +(incident 2026-07-09 : deux suites entières perdues là-dessus). + +Deux pièges connus sur ce poste : +- **Download qui rampe** : l'installeur Node télécharge parfois à quelques KB/s + alors que le CDN répond à ~4 MB/s en curl. Plan B qui marche : télécharger le + zip directement (`https://cdn.playwright.dev/builds/cft//mac-arm64/chrome-headless-shell-mac-arm64.zip`, + `browserVersion` dans `playwright-core/browsers.json`), le dézipper dans + `~/Library/Caches/ms-playwright/chromium_headless_shell-/` et `touch + INSTALLATION_COMPLETE DEPENDENCIES_VALIDATED`. +- **Dossier partiel** : un download interrompu laisse un dossier sans le binaire + dedans — `rm -rf` le dossier de la révision avant de réinstaller. + +La CI n'est pas concernée : elle utilise l'image `mcr.microsoft.com/playwright` +avec les navigateurs pré-installés (le tag doit matcher la version de +`@playwright/test` — voir le commentaire dans `ci-test.yml`). + ## Architecture ``` diff --git a/frontend/e2e/ensure-browser.mjs b/frontend/e2e/ensure-browser.mjs new file mode 100644 index 00000000..5495c780 --- /dev/null +++ b/frontend/e2e/ensure-browser.mjs @@ -0,0 +1,14 @@ +// Local-only guard for `pnpm test:e2e`: installs the Playwright headless +// shell when missing. Skips in CI — the container job pre-bakes browsers +// (PLAYWRIGHT_BROWSERS_PATH) precisely to avoid the flaky CDN download. +import { spawnSync } from 'node:child_process'; + +if (process.env.CI || process.env.PLAYWRIGHT_BROWSERS_PATH) { + process.exit(0); +} + +const r = spawnSync('playwright', ['install', 'chromium', '--only-shell'], { + stdio: 'inherit', + shell: process.platform === 'win32', +}); +process.exit(r.status ?? 1); diff --git a/frontend/package.json b/frontend/package.json index 3c08feb6..259e17b6 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -15,7 +15,7 @@ "test": "vitest run", "test:watch": "vitest", "test:coverage": "vitest run --coverage", - "test:e2e": "playwright test", + "test:e2e": "node e2e/ensure-browser.mjs && playwright test", "test:e2e:ui": "playwright test --ui", "test:e2e:debug": "playwright test --debug", "test:perf": "playwright test --config=playwright.perf.config.ts", diff --git a/frontend/scripts/check-types-drift.mjs b/frontend/scripts/check-types-drift.mjs index 63239082..1e64952a 100644 --- a/frontend/scripts/check-types-drift.mjs +++ b/frontend/scripts/check-types-drift.mjs @@ -14,7 +14,11 @@ // for every type that generated.ts ALREADY declares, compares its field NAMES // against the ts-rs binding and fails if the binding has fields the aggregate is // missing. Field NAMES only (not types) — so the deliberate bigint/optional -// simplifications never trip it. Run after `cargo test export_bindings`. +// simplifications never trip it. Tagged unions are compared too: the union of +// field names across ALL top-level `{ … }` variants on each side (so drift in +// a non-first variant is caught). Pure aliases (no object body) are skipped — +// and reported, so "OK" can't silently mean "compared nothing". +// Run after `cargo test export_bindings`. import { readFileSync, readdirSync, statSync, existsSync } from 'node:fs'; import { join, basename, dirname } from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -44,61 +48,158 @@ function stripComments(s) { return s.replace(/\/\*[\s\S]*?\*\//g, '').replace(/^\s*\/\/.*$/gm, ''); } +/** Skip a string literal starting at src[i] (a quote char); returns the index + * of the closing quote (or end of src). Keeps braces/pipes inside strings + * from confusing the depth trackers below. */ +function skipString(src, i) { + const q = src[i]; + for (i++; i < src.length; i++) { + if (src[i] === '\\') i++; + else if (src[i] === q) return i; + } + return i; +} + /** Top-level field names in an object-type body. Handles nested objects by - * tracking brace depth — only depth-1 `name:` / `name?:` are fields. */ + * tracking brace depth — only depth-1 `name:` / `name?:` are fields. + * Handles ts-rs quoted keys too (`"type": "Agent"`). */ function fieldNames(body) { const clean = stripComments(body); const names = new Set(); let depth = 0; - // Walk token by token, recording identifiers at depth 1 immediately before a `:`. - const re = /([{}])|(\b[a-zA-Z_][a-zA-Z0-9_]*)\s*\??\s*:/g; + // Walk token by token, recording keys at depth 1 immediately before a `:`. + // Alternatives: brace | quoted key | bare string (consumed, no depth effect) | bare key. + const re = /([{}])|"((?:[^"\\]|\\.)*)"\s*\??\s*:|("(?:[^"\\]|\\.)*"|'(?:[^'\\]|\\.)*')|(\b[a-zA-Z_$][a-zA-Z0-9_$]*)\s*\??\s*:/g; let m; while ((m = re.exec(clean))) { if (m[1] === '{') depth++; else if (m[1] === '}') depth--; - else if (m[2] && depth === 1) names.add(m[2]); + else if (m[2] !== undefined && depth === 1) names.add(m[2]); + else if (m[4] && depth === 1) names.add(m[4]); } return names; } -/** Extract the object body `{ … }` for `export (type|interface) Name` from src, - * or null if it isn't a single object type (union / alias). */ -function objectBody(src, name) { - const re = new RegExp(`export (?:interface|type) ${name}\\b[^{]*(\\{)`, 'm'); +/** Extract the declaration for `export (interface|type) Name` from src. + * - interface → { kind: 'interface', rhs: balanced `{ … }` body } + * - type alias → { kind: 'type', rhs: right-hand side after `=` }, bounded at + * the first top-level `;` (or balanced end) so it can NEVER run past the + * current declaration into the next type. + * null if the name isn't declared. */ +function declaration(src, name) { + const re = new RegExp(`export (interface|type) ${name}\\b`); const m = re.exec(src); if (!m) return null; - // Balance braces from the opening `{`. - let i = m.index + m[0].length - 1; + let i = m.index + m[0].length; + if (m[1] === 'interface') { + // Skip to the opening `{` (past any extends clause), then balance. + while (i < src.length && src[i] !== '{' && src[i] !== ';') i++; + if (src[i] !== '{') return null; + let depth = 0; + const open = i; + for (; i < src.length; i++) { + const c = src[i]; + if (c === '"' || c === "'" || c === '`') { i = skipString(src, i); continue; } + if (c === '{') depth++; + else if (c === '}') { depth--; if (depth === 0) return { kind: 'interface', rhs: src.slice(open, i + 1) }; } + } + return null; + } + // type alias: skip generic params to the `=` at angle-depth 0. + let angle = 0; + for (;; i++) { + if (i >= src.length) return null; + const c = src[i]; + if (c === '<') angle++; + else if (c === '>') angle--; + else if (c === '=' && angle === 0) { i++; break; } + else if (c === ';') return null; + } + // Right-hand side ends at the first `;` outside any bracket/string nesting. let depth = 0; const start = i; for (; i < src.length; i++) { - if (src[i] === '{') depth++; - else if (src[i] === '}') { depth--; if (depth === 0) return src.slice(start, i + 1); } + const c = src[i]; + if (c === '"' || c === "'" || c === '`') { i = skipString(src, i); continue; } + if (c === '{' || c === '(' || c === '[') depth++; + else if (c === '}' || c === ')' || c === ']') depth--; + else if (c === ';' && depth === 0) break; + } + return { kind: 'type', rhs: src.slice(start, i) }; +} + +/** Split a type-alias right-hand side on TOP-LEVEL `|` only (depth-aware, so + * pipes nested in variant bodies / generics / strings don't split). */ +function splitUnion(rhs) { + const parts = []; + let depth = 0; + let start = 0; + for (let i = 0; i < rhs.length; i++) { + const c = rhs[i]; + if (c === '"' || c === "'" || c === '`') { i = skipString(rhs, i); continue; } + if (c === '{' || c === '(' || c === '[' || c === '<') depth++; + else if (c === '}' || c === ')' || c === ']' || c === '>') depth--; + else if (c === '|' && depth === 0) { parts.push(rhs.slice(start, i)); start = i + 1; } + } + parts.push(rhs.slice(start)); + return parts; +} + +/** Union of field names across ALL top-level object variants of a declaration + * (tagged unions included — every `{ … }` variant contributes, not just the + * first). Returns null when the declaration has no object body at all + * (pure alias, e.g. `type OnInvalid = "Continue" | "Fail"`). */ +function unionFields(decl) { + if (!decl) return null; + if (decl.kind === 'interface') return fieldNames(decl.rhs); + let hasObjectVariant = false; + const names = new Set(); + for (const part of splitUnion(decl.rhs)) { + const t = part.trim(); + if (t.startsWith('{')) { + hasObjectVariant = true; + for (const n of fieldNames(t)) names.add(n); + } } - return null; + return hasObjectVariant ? names : null; } -const generated = readFileSync(GENERATED, 'utf8'); +const generated = stripComments(readFileSync(GENERATED, 'utf8')); const declared = new Set([...generated.matchAll(/export (?:interface|type) (\w+)/g)].map((m) => m[1])); const drift = []; +let compared = 0; +const skipped = []; // { name, reason } — declared but not field-comparable for (const f of walk(BINDINGS)) { const name = basename(f, '.ts'); if (!declared.has(name)) continue; // backend-only type not in the aggregate → out of scope - const bindingSrc = readFileSync(f, 'utf8'); - const bindingBody = objectBody(bindingSrc, name); - const genBody = objectBody(generated, name); - if (!bindingBody || !genBody) continue; // union/alias — no field comparison - const want = fieldNames(bindingBody); - const have = fieldNames(genBody); + const bindingSrc = stripComments(readFileSync(f, 'utf8')); + const want = unionFields(declaration(bindingSrc, name)); + const have = unionFields(declaration(generated, name)); + if (!want || !have) { + const side = !want && !have ? 'both sides' : !want ? 'binding side' : 'generated.ts side'; + skipped.push({ name, reason: `pure alias (no object body) on ${side}` }); + continue; + } + compared++; const missing = [...want].filter((fld) => !have.has(fld)); if (missing.length) drift.push({ name, missing }); } +const frontendOnly = declared.size - compared - skipped.length; // declared in generated.ts, no binding file +const coverage = + `[types-drift] coverage: ${compared} type(s) field-compared, ${skipped.length} skipped` + + ` (alias/no object body), ${frontendOnly} frontend-only (no binding), of ${declared.size} declared.`; + if (drift.length) { console.error(`[types-drift] ${drift.length} frontend type(s) are MISSING fields present in the Rust model:`); for (const d of drift) console.error(` ${d.name}: ${d.missing.join(', ')}`); console.error(`\nAdd the field(s) to src/types/generated.ts (see backend/bindings/.ts).`); + console.error(coverage); process.exit(1); } -console.log(`[types-drift] OK — no field drift on the ${declared.size} frontend-declared types.`); +if (skipped.length) { + console.log(`[types-drift] skipped (no field comparison): ${skipped.map((s) => s.name).sort().join(', ')}`); +} +console.log(coverage); +console.log(`[types-drift] OK — no field drift on the ${compared} field-compared frontend-declared types.`); diff --git a/frontend/src/components/ChatInput.tsx b/frontend/src/components/ChatInput.tsx index 3e61eb46..35857a87 100644 --- a/frontend/src/components/ChatInput.tsx +++ b/frontend/src/components/ChatInput.tsx @@ -382,6 +382,9 @@ export function ChatInput({ window.dispatchEvent(new CustomEvent('kronn:discussion-updated')); } catch (e) { console.warn('auto-activate skills failed:', e); + // Non-blocking by design (the message still sends), but the user must + // know the agent runs WITHOUT the skill(s) they believe are active. + toast(t('skills.autoActivateFailed', triggered.map(s => s.name).join(', ')), 'warning'); } } diff --git a/frontend/src/components/settings/AgentsSection.tsx b/frontend/src/components/settings/AgentsSection.tsx index e31cdc2f..7cc875c1 100644 --- a/frontend/src/components/settings/AgentsSection.tsx +++ b/frontend/src/components/settings/AgentsSection.tsx @@ -525,13 +525,13 @@ export function AgentsSection({ tabIndex={0} className="flex-row gap-4 cursor-pointer" onClick={async () => { - try { await configApi.setAgentAccess({ agent: agent.agent_type, full_access: !isFullAccess }); } catch (err) { console.warn('Settings action failed:', err); } + try { await configApi.setAgentAccess({ agent: agent.agent_type, full_access: !isFullAccess }); } catch (err) { console.warn('Settings action failed:', err); toast(t('common.actionFailed', userError(err)), 'error'); } refetchAgentAccess(); }} onKeyDown={async (e) => { if (e.key === ' ' || e.key === 'Enter') { e.preventDefault(); - try { await configApi.setAgentAccess({ agent: agent.agent_type, full_access: !isFullAccess }); } catch (err) { console.warn('Settings action failed:', err); } + try { await configApi.setAgentAccess({ agent: agent.agent_type, full_access: !isFullAccess }); } catch (err) { console.warn('Settings action failed:', err); toast(t('common.actionFailed', userError(err)), 'error'); } refetchAgentAccess(); } }} @@ -562,7 +562,7 @@ export function AgentsSection({ title={isDisabled ? t('config.enableOverride') : t('config.disableOverride')} aria-label={isDisabled ? t('config.enableOverride') : t('config.disableOverride')} onClick={async () => { - try { await configApi.toggleTokenOverride(tf.key); } catch (err) { console.warn('Settings action failed:', err); } + try { await configApi.toggleTokenOverride(tf.key); } catch (err) { console.warn('Settings action failed:', err); toast(t('common.actionFailed', userError(err)), 'error'); } refetchTokens(); }} > @@ -593,7 +593,7 @@ export function AgentsSection({ ) : ( )} @@ -617,7 +617,7 @@ export function AgentsSection({