From fa85b911fcf38aeea33019aaed1db05306b20f30 Mon Sep 17 00:00:00 2001 From: ghbvf <104540935+ghbvf@users.noreply.github.com> Date: Thu, 18 Jun 2026 03:28:18 +0800 Subject: [PATCH 1/4] =?UTF-8?q?fix(pr):=20webhook=20=E5=85=A5=E7=AB=99?= =?UTF-8?q?=E5=90=8C=E6=AD=A5=E6=9B=B4=E6=96=B0=20PR=20=E5=88=97=E8=A1=A8?= =?UTF-8?q?=E5=BF=AB=E7=85=A7=20+=20webhook/poll=20=E8=AF=8A=E6=96=AD?= =?UTF-8?q?=EF=BC=88#61=20#62=EF=BC=89?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #61: webhook 收到的 PR 现在走 upsert+emit prs:updated（即使 autoReview=off 也进列表），不再只 dispatch。payload_to_candidate 重写为纯函数 parse_delivery → ParseResult(Routable/WrongRepo/Malformed)，承载 title/labels/url（不再 gh pr view）；新增 registry::update_present（status-only 刷新现有行，不插入）、 commands::webhook_view（复用 should_skip/cooldown_skip 门，对齐 poll 路径）与 AppHandle 绑定的 ingest_webhook（config/ledger fail-closed），删除 gate_dispatchable /gate_candidates；webhook seam 由 set_dispatcher 改为 set_ingestor(WebhookEvent)。 #62: 无新事件类型，命令拉取。webhook.rs 新增 DeliveryStatus/WebhookDelivery + 50 条环形缓冲（每请求恰好记一条）；scheduler.rs 新增 PollDiag + PollStatus， discover_emit_dispatch 记录 started/discovered/persist/error；新增命令 webhook_deliveries / poll_status。所有新 wire 类型配 camelCase golden 测试 （Medium 载体，ai-robust.md）。 Co-Authored-By: Claude Opus 4.8 (1M context) --- src-tauri/src/lib.rs | 43 +- src-tauri/src/pr/commands.rs | 417 +++++++++++++-- src-tauri/src/pr/ledger.rs | 2 +- src-tauri/src/pr/registry.rs | 70 +++ src-tauri/src/pr/scheduler.rs | 311 ++++++++++- src-tauri/src/pr/webhook.rs | 947 ++++++++++++++++++++++++++-------- 6 files changed, 1487 insertions(+), 303 deletions(-) diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 717a442..4f4e62d 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -54,28 +54,31 @@ pub fn run() { Box::pin(run_auto_dispatch(app, project_id, cands)) } })); - // Install the WEBHOOK trigger's dispatch hook (#9). The webhook is a - // second auto-trigger source: its axum handler maps a push payload to a - // `Candidate` and hands it here. Unlike the scheduler (whose candidates - // are pre-gated by `build_view`), webhook candidates arrive raw, so this - // closure applies the parity gates the composition root owns — the same - // `autoReview` gate the scheduler applies at its call site, then the - // static/cooldown gates (`gate_dispatchable`) — before reusing the very - // same `run_auto_dispatch`. Keeping the gates here (not in the handler) - // is what lets `pr::webhook` stay runtime-agnostic (never names AppHandle). - state.webhook.set_dispatcher(Arc::new({ + // Install the WEBHOOK trigger's ingest hook (#9 / #61). The webhook is a + // second auto-trigger source: its axum handler parses + routes a push payload + // into a `WebhookEvent` and hands it here. The ingest (`pr::commands::ingest_webhook`) + // upserts the persisted PR list + emits `prs:updated` (so webhook PRs enter the + // list even when autoReview is OFF — the #61 fix), applies that project's + // static/cooldown gates (`webhook_view`) + the SAME per-project `autoReview` + // gate the scheduler uses, and dispatches the clean candidate by reusing the + // very same `run_auto_dispatch` (captured + cloned below, exactly as the + // scheduler's dispatcher). Keeping all this in the ingest (not the handler) is + // what lets `pr::webhook` stay runtime-agnostic (never names AppHandle); the + // ingest also records the terminal delivery diagnostic (#62). + let webhook_dispatcher: pr::scheduler::ProjectDispatcher = Arc::new({ let app = app.handle().clone(); - move |project_id: String, cands| { + move |project_id, cands| { + let app = app.clone(); + Box::pin(run_auto_dispatch(app, project_id, cands)) + } + }); + state.webhook.set_ingestor(Arc::new({ + let app = app.handle().clone(); + move |ev| { let app = app.clone(); + let dispatcher = webhook_dispatcher.clone(); Box::pin(async move { - // The webhook handler already routed by repo to the owning - // project (#35); apply that project's autoReview + static/cooldown - // gates before reusing the same per-project run_auto_dispatch. - if !pr::scheduler::auto_review_enabled(&app, &project_id) { - return; - } - let gated = pr::commands::gate_dispatchable(&app, &project_id, cands); - run_auto_dispatch(app, project_id, gated).await; + pr::commands::ingest_webhook(&app, &dispatcher, ev).await; }) } })); @@ -104,6 +107,8 @@ pub fn run() { pr::commands::start_webhook, pr::commands::stop_webhook, pr::commands::webhook_status, + pr::commands::webhook_deliveries, + pr::commands::poll_status, review::commands::get_codex_status, review::commands::start_codex, review::commands::stop_codex, diff --git a/src-tauri/src/pr/commands.rs b/src-tauri/src/pr/commands.rs index 33b475d..cb499b8 100644 --- a/src-tauri/src/pr/commands.rs +++ b/src-tauri/src/pr/commands.rs @@ -9,7 +9,8 @@ use crate::model::{Candidate, PullRequestView}; use super::discover::{self, MonitorParams}; use super::gh::{gh_auth_status, GhRow, GhStatus, GithubCli}; use super::ledger::{now_epoch, Ledger}; -use super::webhook::WebhookStatus; +use super::scheduler::{PollStatus, ProjectDispatcher}; +use super::webhook::{DeliveryStatus, IngestIntent, WebhookDelivery, WebhookEvent, WebhookStatus}; /// Annotates one discovered row for the PR list and surfaces its dispatchable /// [`Candidate`] when nothing gates it. Conflict (both trigger labels) skips @@ -257,36 +258,105 @@ pub fn set_pr_archived( Ok(()) } -/// Apply the SAME static + cooldown gates the poll path applies (via `build_view`) -/// to `project_id`'s webhook-sourced candidates (#35), so a push trigger has dispatch -/// parity with that project's scheduler: no draft / fork / disallowed-author / -/// within-cooldown PR slips through just because it arrived by webhook. Resolves THAT -/// project's config (authors / cooldown) and loads ITS ledger partition, then filters -/// each candidate through [`discover::should_skip`] and [`discover::cooldown_skip`]. +/// Annotate a webhook-sourced candidate into a [`PullRequestView`] + dispatch decision +/// (#61), applying the SAME static + cooldown gates the poll path applies via +/// [`build_view`] — minus the conflict branch (the caller handles conflict / StatusOnly +/// upstream from the parsed [`IngestIntent`], so this only ever sees a single-label +/// candidate). The push-path counterpart to `build_view`: `skip_reason = should_skip(..) +/// .or_else(|| cooldown_skip(..))`; `dispatchable = skip_reason.is_none().then(..)`, so a +/// clean candidate dispatches and a gated one (draft / fork / disallowed-author / +/// within-cooldown) becomes a skipped row with NO dispatch — dispatch parity with that +/// project's scheduler, so nothing slips through just because it arrived by webhook. /// -/// BOTH reads fail CLOSED: an unresolvable project / unreadable config OR an -/// unreadable ledger returns an empty Vec (dispatch nothing), same spirit as -/// [`super::scheduler::auto_review_enabled`]. The ledger is the dedup/cooldown source -/// of truth — degrading it to an empty ledger (the prior `unwrap_or_default()`) would -/// pass EVERY cooldown/dedup gate and re-review storm, so an unprovable "not a recent +/// The view's title / labels / url come from the webhook payload (passed in by the +/// caller), so the list row is built without a `gh pr view` round trip. +/// +/// Pure (no `AppHandle`) so the gate composition is unit-tested without a Tauri handle — +/// it replaces the deleted `gate_candidates` parity test (it asserts BOTH the static and +/// the cooldown gate apply; the predicates themselves are tested at their source in +/// `discover.rs`). +fn webhook_view( + cand: Candidate, + title: String, + labels: Vec, + url: String, + params: &MonitorParams, + ledger: &Ledger, + now: u64, +) -> (PullRequestView, Option) { + let skip_reason = discover::should_skip(&cand, params, ledger) + .or_else(|| discover::cooldown_skip(&cand, params, ledger, now)); + // Clone for dispatch only when it passes every static + cooldown gate; a gated row + // contributes a view but no dispatch candidate (parity with `build_view`). + let dispatchable = skip_reason.is_none().then(|| cand.clone()); + let view = PullRequestView { + number: cand.number, + title, + labels, + url, + kind: cand.kind, + skip_reason, + }; + (view, dispatchable) +} + +/// The AppHandle-bound webhook ingest (#61): the body of the [`WebhookIngestor`] the +/// composition root installs. Takes ONE parsed, routed [`WebhookEvent`] and (a) upserts / +/// updates the persisted PR list row, (b) emits `prs:updated` so the list reflects the +/// push WITHOUT waiting for the next poll round (the #61 fix — webhook PRs now enter the +/// list even when autoReview is off), and (c) dispatches the gated-clean candidate iff +/// that project's autoReview is on. Records EXACTLY ONE delivery diagnostic at the end +/// (#62) — the handler records the early-exit classifications, this records the routable +/// terminal status. +/// +/// **Fail-closed (parity with the deleted `gate_dispatchable`'s fail-closed reads):** an +/// unresolvable project / unreadable config OR an unreadable ledger records a `Gated` +/// delivery with the error message and RETURNS without upsert/emit/dispatch. The ledger +/// is the dedup/cooldown source of truth — degrading it to an empty ledger would pass +/// EVERY cooldown/dedup gate and re-review storm, so an unprovable "not a recent /// duplicate" must fail closed, matching the poll path (`discover` uses /// `Ledger::load(app, project_id)?`). /// -/// Called by the composition root's webhook dispatcher closure (`lib.rs`) with the -/// `project_id` the route matched; the conflict (both-labels) gate already dropped in -/// `webhook::payload_to_candidate`. -/// -/// Coverage: the pure predicate composition is unit-tested via [`gate_candidates`]; -/// the predicates themselves at their source (`discover::should_skip` / -/// `cooldown_skip`). The `AppHandle`-bound load branches run in the live app (a Tauri -/// `AppHandle` isn't constructible in a plain test). -pub(crate) fn gate_dispatchable( +/// Reuses the registry's single serialized write seam ([`registry::mutate_tracked`]) for +/// the upsert + emit (so this can't interleave with a poll-cycle upsert / `set_pr_archived` +/// and lose a write) and the scheduler's per-project `auto_review_enabled` gate for the +/// dispatch decision — the SAME primitives both auto-trigger paths share. +pub(crate) async fn ingest_webhook( app: &tauri::AppHandle, - project_id: &str, - candidates: Vec, -) -> Vec { - let Ok(project) = config_service::project(app, project_id) else { - return Vec::new(); + dispatcher: &ProjectDispatcher, + ev: WebhookEvent, +) { + let WebhookEvent { + project_id, + action, + repo, + number, + title, + labels, + url, + intent, + } = ev; + + // Resolve config + ledger, failing closed on either error (see the fn doc). On a + // failure we record a `Gated` delivery with the error and return without touching the + // list — fail-closed parity with the deleted `gate_dispatchable`. + let record_failclosed = |app: &tauri::AppHandle, msg: String| { + record_webhook_delivery( + app, + &repo, + &action, + number, + None, + DeliveryStatus::Gated, + Some(msg), + ); + }; + let project = match config_service::project(app, &project_id) { + Ok(p) => p, + Err(e) => { + record_failclosed(app, format!("项目配置不可读：{}", e.message)); + return; + } }; let params = MonitorParams { repo: project.repo, @@ -295,30 +365,197 @@ pub(crate) fn gate_dispatchable( authors: project.authors, pr_cooldown_seconds: project.pr_cooldown_seconds, }; - let Ok(ledger) = Ledger::load(app, project_id) else { - return Vec::new(); + let ledger = match Ledger::load(app, &project_id) { + Ok(l) => l, + Err(e) => { + record_failclosed(app, format!("ledger 不可读：{}", e.message)); + return; + } }; - gate_candidates(candidates, ¶ms, &ledger, now_epoch()) + let now = now_epoch(); + + // Build the list-row view + dispatch decision + terminal delivery status from the + // parsed intent. `upsert` is the insert-or-update Track path; `update_present` is the + // status-only path (refresh an EXISTING row, never insert). The `kind` for a row that + // has no candidate falls back to a sensible non-empty string. + enum WriteKind { + Upsert, + UpdatePresent, + } + let (view, dispatchable, status, message): ( + PullRequestView, + Option, + DeliveryStatus, + Option, + ); + let write_kind: WriteKind; + + match intent { + IngestIntent::Track { + candidate: Some(cand), + .. + } => { + let (v, d) = webhook_view(cand, title, labels, url, ¶ms, &ledger, now); + // A single trigger label. If the gate passed (skip_reason None) the candidate + // is dispatchable — the autoReview gate below decides Dispatched vs ListUpdated; + // a gated one (draft/fork/author/cooldown) is a `Gated` row carrying the reason. + let (st, msg) = match (&d, &v.skip_reason) { + (Some(_), _) => (DeliveryStatus::Dispatched, None), + (None, reason) => (DeliveryStatus::Gated, reason.clone()), + }; + view = v; + dispatchable = d; + status = st; + message = msg; + write_kind = WriteKind::Upsert; + } + IngestIntent::Track { + candidate: None, + conflict: _, + } => { + // Both trigger labels (conflict): a skipped row, never dispatched. Kind + // "review" for the view (the parse picked review for the conflict view). + view = PullRequestView { + number, + title, + labels, + url, + kind: "review".to_string(), + skip_reason: Some(discover::BOTH_TRIGGER_LABELS_REASON.to_string()), + }; + dispatchable = None; + status = DeliveryStatus::Gated; + message = Some(discover::BOTH_TRIGGER_LABELS_REASON.to_string()); + write_kind = WriteKind::Upsert; + } + IngestIntent::StatusOnly { reason } => { + // Closed/merged or trigger-label-removed: refresh an existing row's status, + // never insert, never dispatch. `kind` from the current labels (review/check) + // or "review" as a sensible default. + let kind = if labels.iter().any(|l| l == ¶ms.review_label) { + "review" + } else if labels.iter().any(|l| l == ¶ms.check_label) { + "check" + } else { + "review" + }; + // The terminal delivery status distinguishes a closed PR (NotOpen) from a + // trigger-label-removed one (NoTriggerLabel) by the reason `parse_delivery` set. + let st = if reason == "PR 已关闭或合并" { + DeliveryStatus::NotOpen + } else { + DeliveryStatus::NoTriggerLabel + }; + view = PullRequestView { + number, + title, + labels, + url, + kind: kind.to_string(), + skip_reason: Some(reason.clone()), + }; + dispatchable = None; + status = st; + message = Some(reason); + write_kind = WriteKind::UpdatePresent; + } + } + + // Persist + emit through the single serialized write seam. The Upsert path always + // persists + emits (an upsert always changes the set); the UpdatePresent path persists + // + emits ONLY when the row existed (mirrors `set_pr_archived`'s no-op skip), so a + // status-only event for an untracked PR is a benign no-op. + let emitted = super::registry::mutate_tracked(app, &project_id, |tracked| match write_kind { + WriteKind::Upsert => { + tracked.upsert(std::slice::from_ref(&view), now); + ( + true, + Some(super::registry::project_snapshot(tracked, app, &project_id)), + ) + } + WriteKind::UpdatePresent => { + if tracked.update_present(&view, now) { + ( + true, + Some(super::registry::project_snapshot(tracked, app, &project_id)), + ) + } else { + (false, None) // untracked PR — nothing changed, skip persist + emit. + } + } + }); + match emitted { + Ok(Some(list)) => { + let _ = app.emit( + crate::events::PRS_UPDATED_EVENT, + &crate::events::PrEvent::Updated { + project_id: project_id.clone(), + prs: list, + }, + ); + } + // Persist no-op (untracked status-only PR) — nothing to emit. + Ok(None) => {} + // A store failure leaves the list unchanged; the delivery diagnostic below still + // records the (would-be) terminal status so the panel surfaces the event. + Err(_) => {} + } + + // Dispatch the gated-clean candidate iff autoReview is on (the SAME per-project gate + // the scheduler applies at its call site). Detached spawn, mirroring the scheduler's + // detached dispatch (a stop must not cancel a start in flight). + let mut final_status = status; + if let Some(cand) = dispatchable { + if super::scheduler::auto_review_enabled(app, &project_id) { + drop(tauri::async_runtime::spawn(dispatcher( + project_id.clone(), + vec![cand], + ))); + final_status = DeliveryStatus::Dispatched; + } else { + // A clean candidate but autoReview off: the list was updated, no dispatch — + // by design (#61: webhook PRs enter the list even with autoReview off). + final_status = DeliveryStatus::ListUpdated; + } + } + + // Record the single terminal delivery diagnostic (#62) for this routable event. + record_webhook_delivery( + app, + &repo, + &action, + number, + Some(view.kind), + final_status, + message, + ); } -/// Drop candidates a fresh `should_skip` / `cooldown_skip` rejects against `ledger`. -/// Split from [`gate_dispatchable`] so the predicate composition is unit-testable -/// without an `AppHandle` — locking that the webhook gate applies BOTH the static and -/// the cooldown gate (the predicates themselves are tested at their source in -/// `discover.rs`). -fn gate_candidates( - candidates: Vec, - params: &MonitorParams, - ledger: &Ledger, - now: u64, -) -> Vec { - candidates - .into_iter() - .filter(|c| { - discover::should_skip(c, params, ledger).is_none() - && discover::cooldown_skip(c, params, ledger, now).is_none() - }) - .collect() +/// Record one webhook-delivery diagnostic into the manager's ring (#62) via `AppState`. +/// Helper so `ingest_webhook`'s several record sites (fail-closed + terminal) stay one +/// liners and never leak the secret/token into the diagnostic. +fn record_webhook_delivery( + app: &tauri::AppHandle, + repo: &str, + action: &Option, + number: u64, + kind: Option, + status: DeliveryStatus, + message: Option, +) { + use tauri::Manager; + app.state::() + .webhook + .record_delivery(WebhookDelivery { + received_at_epoch: now_epoch(), + event: "pull_request".to_string(), + action: action.clone(), + repo: Some(repo.to_string()), + pr_number: Some(number), + kind, + status, + message, + }); } /// Starts the webhook receiver + Cloudflare Quick Tunnel. Requires `webhook_enabled` @@ -409,6 +646,29 @@ pub async fn webhook_status( .await) } +/// Snapshot of the webhook-delivery diagnostics ring (#62) for the settings panel — +/// the recent window of "did GitHub reach us, and what did we do with each delivery". +/// Oldest→newest; capped at the manager's ring size. Never carries the secret/token. +#[tauri::command] +pub async fn webhook_deliveries( + state: tauri::State<'_, crate::state::AppState>, +) -> AppResult> { + Ok(state.webhook.deliveries_snapshot()) +} + +/// Reports `project_id`'s poll-loop status (#62) for the settings panel: whether the +/// loop is running, its resolved interval, and the last cycle's diagnostics (started / +/// success / error / persist epochs + discovered count). Pulled on demand — NO new event +/// type, so the `events.rs` union stays untouched. +#[tauri::command] +pub async fn poll_status( + app: tauri::AppHandle, + state: tauri::State<'_, crate::state::AppState>, + project_id: &str, +) -> AppResult { + Ok(state.scheduler.poll_status(&app, project_id)) +} + #[cfg(test)] mod tests { use super::*; @@ -505,8 +765,13 @@ mod tests { assert!(cand.is_none()); } + // Migrated from the deleted `gate_candidates` parity test: `webhook_view` (#61) must + // apply BOTH the static (already-dispatched) and the cooldown gate to a webhook-sourced + // candidate — a clean one dispatches (Some), a gated one is a skipped row (None) — so a + // push trigger has dispatch parity with the poll path. The predicates themselves are + // tested at their source in `discover.rs`. #[test] - fn gate_candidates_drops_dispatched_and_cooldown_but_keeps_clean() { + fn webhook_view_applies_both_static_and_cooldown_gates() { use crate::pr::ledger::{dispatch_key, DispatchEvent}; use std::collections::HashSet; @@ -527,10 +792,56 @@ mod tests { }], }; - // Locks that the webhook gate applies BOTH the static (dispatched) and the - // cooldown gate — only the clean candidate survives. - let kept = gate_candidates(vec![clean, dispatched, cooled], ¶ms(), &ledger, 1_500); - assert_eq!(kept.len(), 1); - assert_eq!(kept[0].number, 1); + let meta = |n: u64| { + ( + format!("PR {n}"), + vec!["review-label".to_string()], + format!("https://x/{n}"), + ) + }; + + // Clean candidate → no skip_reason → dispatchable Some. + let (v1, d1) = { + let (t, l, u) = meta(1); + webhook_view(clean, t, l, u, ¶ms(), &ledger, 1_500) + }; + assert_eq!(v1.number, 1); + assert_eq!(v1.title, "PR 1"); + assert_eq!(v1.skip_reason, None); + assert!(d1.is_some(), "a clean candidate is dispatchable"); + + // Already-dispatched (static gate) → skip_reason Some → dispatchable None. + let (v2, d2) = { + let (t, l, u) = meta(2); + webhook_view(dispatched, t, l, u, ¶ms(), &ledger, 1_500) + }; + assert!( + v2.skip_reason + .as_deref() + .is_some_and(|r| r.contains("already dispatched")), + "static gate fires: {:?}", + v2.skip_reason + ); + assert!( + d2.is_none(), + "a statically-gated candidate is not dispatchable" + ); + + // Within cooldown (cooldown gate) → skip_reason Some → dispatchable None. + let (v3, d3) = { + let (t, l, u) = meta(3); + webhook_view(cooled, t, l, u, ¶ms(), &ledger, 1_500) + }; + assert!( + v3.skip_reason + .as_deref() + .is_some_and(|r| r.contains("within cooldown")), + "cooldown gate fires: {:?}", + v3.skip_reason + ); + assert!( + d3.is_none(), + "a cooldown-gated candidate is not dispatchable" + ); } } diff --git a/src-tauri/src/pr/ledger.rs b/src-tauri/src/pr/ledger.rs index e98feb3..6bb9af5 100644 --- a/src-tauri/src/pr/ledger.rs +++ b/src-tauri/src/pr/ledger.rs @@ -140,7 +140,7 @@ impl Ledger { /// `has_dispatched` / cooldown check for one project never sees another's records. /// /// **Lock-free read (intentional).** The discovery path (`commands::discover`) and - /// the webhook gate (`commands::gate_dispatchable`) call this OUTSIDE + /// the webhook ingest (`commands::ingest_webhook` → `webhook_view`) call this OUTSIDE /// [`LEDGER_WRITE_LOCK`]; a load is a single whole-value store read (no torn read) /// and a stale-by-one-round snapshot is acceptable because it only gates an /// OPTIMIZATION — the real double-dispatch backstop is the session registry's diff --git a/src-tauri/src/pr/registry.rs b/src-tauri/src/pr/registry.rs index d331652..164aacd 100644 --- a/src-tauri/src/pr/registry.rs +++ b/src-tauri/src/pr/registry.rs @@ -193,6 +193,32 @@ impl TrackedPrs { false } } + + /// Refreshes an EXISTING tracked row's display fields (title / labels / url / kind / + /// skip_reason) and bumps its `last_seen_epoch` to `now`, returning `true`. Returns + /// `false` and inserts NOTHING when no row with `view.number` exists. + /// + /// The status-only counterpart to [`Self::upsert`] (#61): a webhook event that should + /// update an ALREADY-TRACKED PR's status (a closed/merged PR, or one whose trigger + /// label was removed) without conjuring a brand-new row for a PR the poll path never + /// surfaced. `first_seen_epoch` / `archived` are preserved (same as the upsert hit + /// path). The caller skips persist + re-emit when this returns `false` (mirroring + /// `set_archived`'s unknown-number no-op), so a status-only event for an untracked PR + /// is a benign no-op rather than a phantom insert. + pub fn update_present(&mut self, view: &PullRequestView, now: u64) -> bool { + if let Some(existing) = self.prs.iter_mut().find(|p| p.number == view.number) { + existing.title = view.title.clone(); + existing.labels = view.labels.clone(); + existing.url = view.url.clone(); + existing.kind = view.kind.clone(); + existing.skip_reason = view.skip_reason.clone(); + existing.last_seen_epoch = now; + // first_seen_epoch and archived are preserved (parity with the upsert hit). + true + } else { + false + } + } } /// The single serialized read-modify-write seam for the persisted set (F1). Holds @@ -415,6 +441,50 @@ mod tests { assert_eq!(t.prs.len(), 1); } + #[test] + fn update_present_hit_refreshes_fields_and_bumps_last_seen() { + // #61 status-only path: an existing row is refreshed + its presence clock bumped, + // preserving first_seen_epoch + archived (parity with the upsert hit path). + let mut t = TrackedPrs::default(); + t.upsert(&[view(1, "old title")], 1_000); + t.set_archived(1, true); + + let mut refreshed = view(1, "new title"); + refreshed.skip_reason = Some("PR 已关闭或合并".to_string()); + refreshed.labels = vec!["closed-now".to_string()]; + assert!( + t.update_present(&refreshed, 2_000), + "an existing row updates and returns true" + ); + + assert_eq!(t.prs.len(), 1, "update_present must not insert on a hit"); + let pr = &t.prs[0]; + assert_eq!(pr.title, "new title", "display fields refresh"); + assert_eq!(pr.labels, vec!["closed-now".to_string()]); + assert_eq!(pr.skip_reason.as_deref(), Some("PR 已关闭或合并")); + assert_eq!(pr.first_seen_epoch, 1_000, "first_seen_epoch preserved"); + assert_eq!(pr.last_seen_epoch, 2_000, "last_seen_epoch bumped"); + assert!( + pr.archived, + "archived preserved across a status-only update" + ); + } + + #[test] + fn update_present_miss_returns_false_and_does_not_insert() { + // A status-only event for a PR the poll path never surfaced is a benign no-op: + // no row exists, so nothing is inserted and the caller skips persist + emit. + let mut t = TrackedPrs::default(); + t.upsert(&[view(1, "PR one")], 1_000); + + assert!( + !t.update_present(&view(999, "ghost"), 2_000), + "an unknown number returns false" + ); + assert_eq!(t.prs.len(), 1, "no insert on a miss"); + assert!(t.prs.iter().all(|p| p.number != 999)); + } + #[test] fn to_view_list_current_within_grace_and_stale_beyond() { let t = TrackedPrs { diff --git a/src-tauri/src/pr/scheduler.rs b/src-tauri/src/pr/scheduler.rs index 9fbe20c..c81d5c9 100644 --- a/src-tauri/src/pr/scheduler.rs +++ b/src-tauri/src/pr/scheduler.rs @@ -42,8 +42,9 @@ use std::pin::Pin; use std::sync::{Arc, Mutex as StdMutex}; use std::time::Duration; +use serde::Serialize; use tauri::async_runtime::JoinHandle; -use tauri::Emitter; // for app.emit +use tauri::{AppHandle, Emitter}; // Emitter for app.emit use tokio::sync::Notify; use tokio::time::MissedTickBehavior; @@ -54,6 +55,83 @@ use crate::model::{Candidate, TrackedPrView}; use super::registry; +/// Internal poll-loop diagnostics (#62): timestamps + counters the panel reads to +/// answer "is the loop alive, when did it last run, and what happened". Pure data with +/// pure mutators (unit-tested below) — the `Scheduler` owns one behind an `Arc` +/// and the cycle updates it; `SchedulerSet::poll_status` snapshots it into the wire +/// [`PollStatus`]. NO new event type: the frontend pulls this via the `poll_status` +/// command, keeping the `events.rs` union untouched. `pub(crate)` only so the +/// `pub(crate)` [`Scheduler::poll_diag`] accessor's return type is visibility-consistent; +/// it is not a public surface — the public wire type is [`PollStatus`]. +#[derive(Debug, Clone, Default)] +pub(crate) struct PollDiag { + /// Epoch of the most recent cycle entry (a tick / wake / reconfigure fired a cycle). + last_started_epoch: Option, + /// Epoch of the most recent successful discovery (discover returned `Ok`). + last_success_epoch: Option, + /// Epoch of the most recent discovery error. + last_error_epoch: Option, + /// The most recent discovery error message (kept until the next error overwrites it). + last_error_message: Option, + /// Epoch of the most recent SUCCESSFUL persist of the round's list. + last_persist_epoch: Option, + /// PR count discovered in the most recent successful cycle. + last_discovered_count: Option, +} + +impl PollDiag { + /// A cycle started (entered the body). Bumps `last_started_epoch`. + fn mark_started(&mut self, now: u64) { + self.last_started_epoch = Some(now); + } + + /// Discovery succeeded with `count` rows. Records the success epoch + count. + fn mark_discovered(&mut self, count: u64, now: u64) { + self.last_success_epoch = Some(now); + self.last_discovered_count = Some(count); + } + + /// The round's list persisted successfully. Records the persist epoch. + fn mark_persist(&mut self, now: u64) { + self.last_persist_epoch = Some(now); + } + + /// Discovery (or persist) failed. Records the error epoch + message. + fn mark_error(&mut self, msg: String, now: u64) { + self.last_error_epoch = Some(now); + self.last_error_message = Some(msg); + } +} + +/// Poll-loop status reported to the settings panel (#62), pulled via the `poll_status` +/// command (NOT a new event type — the `events.rs` union stays untouched). `running` + +/// `interval_secs` describe the loop; the rest mirror [`PollDiag`]'s last-cycle fields. +/// +/// camelCase wire type mirrored in `src/pr/types.ts` (Medium carrier per +/// `.claude/rules/prmonitor/ai-robust.md`; a `poll_status_wire_shape_*` golden test pins +/// the key shape so a rename can't silently drift the TS mirror). pr-slice-private (not a +/// cross-slice contract), same placement as [`super::webhook::WebhookStatus`]. +#[derive(Debug, Clone, Serialize, Default)] +#[serde(rename_all = "camelCase")] +pub struct PollStatus { + /// Whether this project's poll loop is currently running. + pub running: bool, + /// The resolved poll period (secs) — that project's `poll_interval_secs`, clamped. + pub interval_secs: u64, + /// Epoch of the most recent cycle entry. + pub last_started_epoch: Option, + /// Epoch of the most recent successful discovery. + pub last_success_epoch: Option, + /// Epoch of the most recent discovery error. + pub last_error_epoch: Option, + /// The most recent discovery error message. + pub last_error_message: Option, + /// Epoch of the most recent successful persist. + pub last_persist_epoch: Option, + /// PR count discovered in the most recent successful cycle. + pub last_discovered_count: Option, +} + /// Abstract per-cycle dispatch hook: consumes a cycle's `project_id` plus its /// dispatchable [`Candidate`]s and drives them to completion (in practice: /// auto-start their reviews concurrently). The leading `project_id` (#35) is the @@ -85,6 +163,11 @@ pub struct Scheduler { /// `#[derive(Default)]` still holds) — a `None` dispatcher means a cycle discovers /// + emits but starts no reviews (the pre-#8 behavior). dispatcher: StdMutex>, + /// Per-loop poll diagnostics (#62), shared with the cycle closure so each cycle + /// records its started / discovered / persist / error timestamps. `Arc>` + /// (defaults to an empty `PollDiag`, so `#[derive(Default)]` still holds) read by + /// [`Self::poll_diag`] for the `poll_status` command. + diag: Arc>, } /// The live task plus the channels the loop selects on. @@ -140,6 +223,9 @@ impl Scheduler { ) } }; + // Snapshot the shared diag handle into the cycle closure (#62) so each cycle + // records its timestamps into the same `PollDiag` `poll_diag` reads. + let diag = self.diag.clone(); let on_cycle = { let app = app.clone(); let project_id = project_id.clone(); @@ -147,7 +233,10 @@ impl Scheduler { let app = app.clone(); let project_id = project_id.clone(); let dispatcher = dispatcher.clone(); - async move { discover_emit_dispatch(&app, &project_id, dispatcher.as_ref()).await } + let diag = diag.clone(); + async move { + discover_emit_dispatch(&app, &project_id, dispatcher.as_ref(), &diag).await + } } }; @@ -197,6 +286,27 @@ impl Scheduler { task.reconfigure.notify_one(); } } + + /// Whether this scheduler's loop task is live (a slot present whose handle hasn't + /// finished). Drives [`SchedulerSet::poll_status`]'s `running` flag (#62). A finished + /// task slot (the loop exited) reports `false`. `pub(crate)`: only `SchedulerSet` + /// (this module) reads it — not a public surface. + pub(crate) fn is_running(&self) -> bool { + self.task + .lock() + .unwrap() + .as_ref() + .map(|t| !t.handle.inner().is_finished()) + .unwrap_or(false) + } + + /// Snapshot of this loop's diagnostics (#62) for [`SchedulerSet::poll_status`]. A + /// clone so the lock is released before the caller maps it into the wire + /// [`PollStatus`]. `pub(crate)` (returns the module-private [`PollDiag`]): only + /// `SchedulerSet` reads it — the public wire surface is [`PollStatus`]. + pub(crate) fn poll_diag(&self) -> PollDiag { + self.diag.lock().unwrap().clone() + } } /// The composition-root handle for ALL projects' poll loops (#35). Lives in @@ -306,6 +416,47 @@ impl SchedulerSet { } map.clear(); } + + /// Reports `project_id`'s poll-loop status (#62) for the settings panel, pulled via + /// the `poll_status` command. When that project's scheduler exists AND is running, the + /// diag fields are copied from its live [`PollDiag`]; otherwise `running: false` with + /// the default (all-`None`) diag. `interval_secs` is always the resolved period for + /// that project (so the panel shows the configured cadence even while stopped), + /// clamped through [`resolve_period`] from the persisted `poll_interval_secs`. + pub fn poll_status( + &self, + app: &AppHandle, + project_id: &str, + ) -> PollStatus { + let interval_secs = + resolve_period(config_service::project(app, project_id).map(|p| p.poll_interval_secs)); + // Snapshot the running scheduler's diag (if any) without holding the map lock + // across the projection. + let diag = { + let map = self.inner.lock().unwrap(); + match map.get(project_id) { + Some(s) if s.is_running() => Some(s.poll_diag()), + _ => None, + } + }; + match diag { + Some(d) => PollStatus { + running: true, + interval_secs, + last_started_epoch: d.last_started_epoch, + last_success_epoch: d.last_success_epoch, + last_error_epoch: d.last_error_epoch, + last_error_message: d.last_error_message, + last_persist_epoch: d.last_persist_epoch, + last_discovered_count: d.last_discovered_count, + }, + None => PollStatus { + running: false, + interval_secs, + ..PollStatus::default() + }, + } + } } /// The poll loop, with its period source and per-cycle action injected (F4) so @@ -372,9 +523,19 @@ async fn discover_emit_dispatch( app: &tauri::AppHandle, project_id: &str, dispatcher: Option<&ProjectDispatcher>, + diag: &Arc>, ) { + // Mark this cycle as started (#62) before the (possibly slow) discovery, so the panel + // shows the loop is actively working even while `gh` is in flight. + diag.lock() + .unwrap() + .mark_started(super::ledger::now_epoch()); let (event, dispatchable) = match super::commands::discover(app, project_id).await { Ok((views, dispatchable)) => { + // Discovery succeeded: record the success epoch + count (#62). + diag.lock() + .unwrap() + .mark_discovered(views.len() as u64, super::ledger::now_epoch()); // Upsert this round and persist it through the registry's single // serialized write seam (F1): `mutate_tracked` holds the cross-writer lock // across load→upsert→save so a concurrent `set_pr_archived` can't interleave @@ -386,23 +547,39 @@ async fn discover_emit_dispatch( // Updated (F2). Auto-dispatch is independent of persistence and still runs. let now = super::ledger::now_epoch(); let grace = registry::presence_grace_secs(app, project_id); - let event = persist_event( - project_id, - registry::mutate_tracked(app, project_id, |tracked| { - tracked.upsert(&views, now); - (true, registry::to_view_list(tracked, now, grace)) - }), - ); - (event, dispatchable) + let persisted = registry::mutate_tracked(app, project_id, |tracked| { + tracked.upsert(&views, now); + (true, registry::to_view_list(tracked, now, grace)) + }); + // Record the persist outcome (#62): a successful persist bumps the persist + // clock; a store failure records the error (it surfaces as `PrEvent::Error` + // below — keep the diag's error state in lockstep with what the UI sees). + match &persisted { + Ok(_) => diag + .lock() + .unwrap() + .mark_persist(super::ledger::now_epoch()), + Err(e) => diag + .lock() + .unwrap() + .mark_error(e.message.clone(), super::ledger::now_epoch()), + } + (persist_event(project_id, persisted), dispatchable) } // On discovery error the dispatchable list is empty — nothing auto-starts. - Err(e) => ( - PrEvent::Error { - project_id: project_id.to_string(), - message: e.message, - }, - Vec::new(), - ), + Err(e) => { + // Record the discovery error (#62) so the panel surfaces "last cycle failed". + diag.lock() + .unwrap() + .mark_error(e.message.clone(), super::ledger::now_epoch()); + ( + PrEvent::Error { + project_id: project_id.to_string(), + message: e.message, + }, + Vec::new(), + ) + } }; let _ = app.emit(PRS_UPDATED_EVENT, &event); // ignore emit error (window may be gone) @@ -758,4 +935,104 @@ mod tests { .expect("stop should let the spawned task finish") .expect("loop task should not panic"); } + + // ── Poll diagnostics (#62) ───────────────────────────────────────────── + // The pure `PollDiag` mutators in isolation (the cycle wiring that calls them + // needs an `AppHandle`, so it runs in the live app). Each mutator sets exactly + // its fields, so a snapshot taken by `poll_status` reflects the last cycle. + + #[test] + fn default_scheduler_has_empty_poll_diag() { + // `#[derive(Default)]` must keep working with the new `diag` field: a fresh + // scheduler is not running and its diag is all-`None`. + let scheduler = Scheduler::default(); + assert!( + !scheduler.is_running(), + "a default scheduler is not running" + ); + let d = scheduler.poll_diag(); + assert!(d.last_started_epoch.is_none()); + assert!(d.last_success_epoch.is_none()); + assert!(d.last_error_epoch.is_none()); + assert!(d.last_persist_epoch.is_none()); + assert!(d.last_discovered_count.is_none()); + } + + #[test] + fn poll_diag_mark_started_sets_started_epoch_only() { + let mut d = PollDiag::default(); + d.mark_started(100); + assert_eq!(d.last_started_epoch, Some(100)); + assert!(d.last_success_epoch.is_none()); + assert!(d.last_error_epoch.is_none()); + } + + #[test] + fn poll_diag_mark_discovered_records_count_and_success() { + let mut d = PollDiag::default(); + d.mark_discovered(7, 200); + assert_eq!(d.last_success_epoch, Some(200)); + assert_eq!(d.last_discovered_count, Some(7)); + // mark_discovered does not touch the error fields. + assert!(d.last_error_epoch.is_none()); + } + + #[test] + fn poll_diag_mark_persist_sets_persist_epoch() { + let mut d = PollDiag::default(); + d.mark_persist(300); + assert_eq!(d.last_persist_epoch, Some(300)); + } + + #[test] + fn poll_diag_mark_error_records_epoch_and_message() { + let mut d = PollDiag::default(); + d.mark_started(100); + d.mark_error("gh exploded".to_string(), 400); + assert_eq!(d.last_error_epoch, Some(400)); + assert_eq!(d.last_error_message.as_deref(), Some("gh exploded")); + // A prior started epoch is untouched by an error (they are independent clocks). + assert_eq!(d.last_started_epoch, Some(100)); + // An error does not advance the success clock. + assert!(d.last_success_epoch.is_none()); + } + + // Wire-shape lock for `PollStatus` (#62, Medium carrier per ai-robust.md): the + // `poll_status` command's wire type, mirrored in `src/pr/types.ts`; a field rename + // would drift the TS mirror silently. Same pattern as + // `webhook_status_wire_shape_is_camel_case`. + #[test] + fn poll_status_wire_shape_is_camel_case() { + let s = PollStatus { + running: true, + interval_secs: 120, + last_started_epoch: Some(1_700_000_000), + last_success_epoch: Some(1_700_000_010), + last_error_epoch: None, + last_error_message: None, + last_persist_epoch: Some(1_700_000_011), + last_discovered_count: Some(3), + }; + let v = serde_json::to_value(&s).expect("PollStatus serializes"); + + // camelCase keys present. + assert!(v.get("running").is_some()); + assert!(v.get("intervalSecs").is_some()); + assert!(v.get("lastStartedEpoch").is_some()); + assert!(v.get("lastSuccessEpoch").is_some()); + assert!(v.get("lastErrorEpoch").is_some()); + assert!(v.get("lastErrorMessage").is_some()); + assert!(v.get("lastPersistEpoch").is_some()); + assert!(v.get("lastDiscoveredCount").is_some()); + + // snake_case forms absent — a rename would surface here. + assert!(v.get("interval_secs").is_none()); + assert!(v.get("last_started_epoch").is_none()); + assert!(v.get("last_discovered_count").is_none()); + + // `None` fields serialize to JSON null (not omitted), so the TS mirror's + // optional-or-null contract stays closed. + assert_eq!(v["lastErrorEpoch"], serde_json::Value::Null); + assert_eq!(v["lastErrorMessage"], serde_json::Value::Null); + } } diff --git a/src-tauri/src/pr/webhook.rs b/src-tauri/src/pr/webhook.rs index bbfe1e4..c3d8555 100644 --- a/src-tauri/src/pr/webhook.rs +++ b/src-tauri/src/pr/webhook.rs @@ -33,11 +33,12 @@ //! to refresh it (the composition root wires that restart on `set_config`). //! //! **Layering.** The axum handler is runtime-agnostic — it never names -//! `AppHandle`. The autoReview gate and the static/cooldown gates (parity with -//! the poll path) live in the [`ProjectDispatcher`] closure the root installs via -//! [`WebhookManager::set_dispatcher`] (which holds the concrete app handle), exactly -//! as [`super::scheduler::Scheduler`] does. The handler's only job is verify → -//! parse → route → hand off. +//! `AppHandle`. The list upsert + `prs:updated` emit (#61) and the autoReview + +//! static/cooldown gates (parity with the poll path) live in the [`WebhookIngestor`] +//! closure the root installs via [`WebhookManager::set_ingestor`] (which holds the +//! concrete app handle), exactly as [`super::scheduler::Scheduler`] does. The handler's +//! only job is verify → parse → route → hand off (and record the early-exit delivery +//! diagnostics, #62). //! //! **Security.** The endpoint is public (via the tunnel), so every request is //! HMAC-verified (`X-Hub-Signature-256`) against the configured secret before the @@ -47,6 +48,9 @@ //! GitHub write, breaking the app's read-only `gh` surface) — the user pastes the //! tunnel URL + secret into the repo's webhook settings by hand. +use std::collections::VecDeque; +use std::future::Future; +use std::pin::Pin; use std::process::Stdio; use std::sync::{Arc, Mutex as StdMutex}; use std::time::Duration; @@ -65,12 +69,142 @@ use tokio::io::{AsyncBufRead, AsyncBufReadExt, BufReader, Lines}; use tokio::process::{Child, Command}; use tokio::sync::oneshot; -use super::scheduler::ProjectDispatcher; +use super::ledger; use crate::error::{AppError, AppResult}; use crate::model::{Candidate, WebhookTunnelMode}; type HmacSha256 = Hmac; +/// The webhook ingest hook the composition root installs (replacing the old raw +/// `ProjectDispatcher`). Called with one parsed, routed [`WebhookEvent`]; its body +/// (the root's `ingest_webhook` wrapper in `commands.rs`) upserts the persisted PR +/// list, emits `prs:updated`, and dispatches the gated candidate — keeping the axum +/// handler runtime-agnostic (the closure holds the concrete `AppHandle`, the +/// handler never names it). Boxed-future + `Arc` so it is `Clone`able into the +/// `WebhookCtx` the handler shares, mirroring [`super::scheduler::ProjectDispatcher`]. +pub type WebhookIngestor = + Arc Pin + Send>> + Send + Sync>; + +/// Cap on the retained webhook-delivery diagnostics ring (#62). Old deliveries are +/// popped from the front once the buffer is full — the panel only ever needs a recent +/// window for "did GitHub reach us, and what did we do with it". +const DELIVERY_RING_CAP: usize = 50; + +/// One terminal classification of a received webhook delivery (#62), recorded EXACTLY +/// once per request so the settings panel can diagnose "GitHub posted but nothing +/// happened" without new event types (pulled via the `webhook_deliveries` command). +/// +/// camelCase wire enum mirrored in `src/pr/types.ts` (Medium carrier per +/// `.claude/rules/prmonitor/ai-robust.md`; a `webhook_delivery_wire_shape_*` golden +/// test pins the strings + key shape so a rename can't silently drift the TS mirror). +#[derive(Debug, Clone, Copy, Serialize)] +#[serde(rename_all = "camelCase")] +pub enum DeliveryStatus { + /// HMAC verification failed (401) — wrong/absent signature. + Unauthorized, + /// Body did not parse as JSON, or a `pull_request` payload was malformed. + BadPayload, + /// A non-`pull_request` event (e.g. GitHub's `ping`) — acknowledged, no action. + Ignored, + /// Verified but the event's repo matches no enabled project's route (fail-closed). + WrongRepo, + /// Open PR carrying neither trigger label (label removed) — list updated, no dispatch. + NoTriggerLabel, + /// PR not open (closed/merged) — list updated to reflect it, never dispatched. + NotOpen, + /// A single trigger label, but a gate (conflict / draft / fork / author / cooldown) + /// blocked dispatch — list updated, review NOT auto-started. + Gated, + /// A clean candidate with autoReview ON — review auto-dispatched. + Dispatched, + /// A clean candidate with autoReview OFF — list updated only (no dispatch by design). + ListUpdated, +} + +/// One recorded webhook delivery diagnostic (#62). MUST NOT carry the secret/token or +/// the raw signature — only the routing + classification metadata the panel renders. +/// +/// camelCase wire type mirrored in `src/pr/types.ts` (Medium carrier per +/// `.claude/rules/prmonitor/ai-robust.md`). +#[derive(Debug, Clone, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct WebhookDelivery { + /// When the request was received (epoch secs, from [`ledger::now_epoch`]). + pub received_at_epoch: u64, + /// The `X-GitHub-Event` header value (e.g. `"pull_request"`, `"ping"`, or `""`). + pub event: String, + /// The PR `action` (`"labeled"` / `"opened"` / …) when parseable; else `None`. + pub action: Option, + /// The event repo `owner/name` when known; else `None`. + pub repo: Option, + /// The PR number when known; else `None`. + pub pr_number: Option, + /// The classified turn kind (`"review"` / `"check"`) when applicable; else `None`. + pub kind: Option, + /// The terminal classification of this delivery. + pub status: DeliveryStatus, + /// A short human-readable (Chinese) note (skip reason / error); `None` → null. + pub message: Option, +} + +/// Outcome of parsing a verified `pull_request` payload against the route snapshot +/// (#61). Pure (no `AppHandle`) so the whole classification is unit-tested; the +/// `AppHandle`-bound ingest in `commands.rs` consumes it. The handler maps each +/// non-`Routable` arm to a terminal [`DeliveryStatus`] inline; `Routable` is handed to +/// `ingest_webhook`, which records the terminal status from the [`IngestIntent`]. +#[derive(Debug)] +pub enum ParseResult { + /// The event routes to an enabled project and parsed cleanly — proceed to ingest. + /// `Box`ed because `WebhookEvent` is much larger than the other variants + /// (`clippy::large_enum_variant`) and is always heap-handed to the ingestor anyway. + Routable(Box), + /// Verified, but the event's repo matched no enabled route (fail-closed drop). The + /// repo (when known) is carried for the delivery diagnostic. + WrongRepo { repo: Option }, + /// No `pull_request`, or a required field (number / head.sha / head.ref) is missing. + Malformed, +} + +/// One parsed + routed webhook event (#61): the metadata the ingest needs to upsert the +/// PR list row, emit `prs:updated`, and (when the intent yields a candidate) dispatch. +/// Built purely by [`parse_delivery`]; consumed by `commands::ingest_webhook`. +#[derive(Debug)] +pub struct WebhookEvent { + /// The routing key (the matched [`ProjectRoute::id`]). + pub project_id: String, + /// The PR `action` (for the delivery diagnostic). + pub action: Option, + /// The matched repo `owner/name` (for the delivery diagnostic). + pub repo: String, + /// The PR number. + pub number: u64, + /// The PR title (`""` when absent). + pub title: String, + /// The PR's current label names. + pub labels: Vec, + /// The PR's HTML URL (`""` when absent). + pub url: String, + /// What to do with this event: track (with an optional dispatch candidate) or + /// update the list's status only. + pub intent: IngestIntent, +} + +/// What an ingest should do with a [`WebhookEvent`] (#61). +#[derive(Debug)] +pub enum IngestIntent { + /// An OPEN PR with a clean single trigger label (`candidate: Some`) — upsert + maybe + /// dispatch — or BOTH trigger labels (`conflict: true`, `candidate: None`) — upsert as + /// a skipped row, never dispatch (mirrors the poll path's conflict drop). + Track { + candidate: Option, + conflict: bool, + }, + /// The PR should appear in the list with a skip reason but never dispatch: a + /// closed/merged PR, or an open PR whose trigger label was removed. The ingest + /// refreshes an EXISTING row's status (no insert). + StatusOnly { reason: String }, +} + /// cloudflared prints the assigned Quick Tunnel URL to stderr within a few seconds; /// cap the wait so a stuck binary can't hang `start_webhook`. const TUNNEL_URL_TIMEOUT: Duration = Duration::from_secs(20); @@ -187,15 +321,22 @@ pub struct ProjectRoute { #[derive(Default)] pub struct WebhookManager { /// Installed once by the composition root (lib.rs) BEFORE any start, like - /// [`super::scheduler::Scheduler::set_dispatcher`]. The closure is called with the - /// routed `project_id` (#35) + the event's candidates; it applies the autoReview + - /// static/cooldown gates and runs `auto_dispatch`, keeping the axum handler - /// runtime-agnostic. - dispatcher: StdMutex>, + /// [`super::scheduler::Scheduler::set_dispatcher`]. The closure is called with one + /// parsed, routed [`WebhookEvent`] (#61); its body upserts the persisted PR list, + /// emits `prs:updated`, and (when gated-clean) dispatches — keeping the axum handler + /// runtime-agnostic (the closure holds the concrete `AppHandle`). + ingestor: StdMutex>, runtime: StdMutex>, /// Serializes `start` (bind + spawn + tunnel-URL await) so concurrent starts /// can't double-bind the port. start_lock: tokio::sync::Mutex<()>, + /// The webhook-delivery diagnostics ring (#62), capped at [`DELIVERY_RING_CAP`]. + /// Recorded EXACTLY once per request (by the handler for early exits, by + /// `ingest_webhook` for the routable terminal status) and read by the + /// `webhook_deliveries` command. A process-shared `StdMutex>` so it + /// survives start/stop cycles (a stop tears down the runtime, not the diagnostics), + /// keeping `#[derive(Default)]` (an empty ring). + deliveries: Arc>>, } /// The live receiver + (optional) tunnel handles. [`Self::teardown`] aborts @@ -291,9 +432,29 @@ impl WebhookRuntime { } impl WebhookManager { - /// Install the dispatch hook (composition root, before any start). - pub fn set_dispatcher(&self, d: ProjectDispatcher) { - *self.dispatcher.lock().unwrap() = Some(d); + /// Install the ingest hook (composition root, before any start). Replaces any prior + /// hook (last writer wins). + pub fn set_ingestor(&self, i: WebhookIngestor) { + *self.ingestor.lock().unwrap() = Some(i); + } + + /// Record one webhook-delivery diagnostic (#62), popping the oldest when the ring is + /// full ([`DELIVERY_RING_CAP`]). Called EXACTLY once per request — by the handler for + /// the early-exit classifications, by `ingest_webhook` for the routable terminal + /// status. Sync + interior-mutable so it is callable from the runtime-agnostic + /// handler and from `ingest_webhook` alike (via `AppState`). + pub fn record_delivery(&self, d: WebhookDelivery) { + let mut ring = self.deliveries.lock().unwrap(); + if ring.len() >= DELIVERY_RING_CAP { + ring.pop_front(); + } + ring.push_back(d); + } + + /// Snapshot the delivery ring oldest→newest (#62) for the `webhook_deliveries` + /// command. A clone so the lock is released before the caller serializes. + pub fn deliveries_snapshot(&self) -> Vec { + self.deliveries.lock().unwrap().iter().cloned().collect() } /// Start (or restart) the local receiver + (per-`mode`) tunnel. Tears down any prior @@ -355,12 +516,12 @@ impl WebhookManager { )); } - let dispatcher = self - .dispatcher + let ingestor = self + .ingestor .lock() .unwrap() .clone() - .ok_or_else(|| AppError::new("webhook dispatcher 未初始化".to_string()))?; + .ok_or_else(|| AppError::new("webhook ingestor 未初始化".to_string()))?; // LOCAL bind only — the public path is the tunnel; the raw port is never // world-reachable. Shared by all three modes. @@ -401,7 +562,8 @@ impl WebhookManager { let ctx = Arc::new(WebhookCtx { secret, routes, - dispatcher, + ingestor, + deliveries: self.deliveries.clone(), }); let router = Router::new() .route(WEBHOOK_PATH, post(handle_webhook)) @@ -629,18 +791,47 @@ struct WebhookCtx { /// the runtime's life — a project add/remove/enable requires a webhook restart (the /// composition root wires that on `set_config`). routes: Vec, - dispatcher: ProjectDispatcher, + /// The injected ingest hook (#61): handed each parsed, routed [`WebhookEvent`]. + ingestor: WebhookIngestor, + /// The manager's delivery-diagnostics ring (#62), shared so the handler can record + /// the early-exit classifications directly (the routable terminal status is recorded + /// by `ingest_webhook` via `AppState`). + deliveries: Arc>>, } -/// `POST /webhook`. Verify the GitHub HMAC, map a `pull_request` payload to a -/// [`Candidate`], and hand it to the dispatcher off the request path (so GitHub gets -/// a fast 2xx). Non-`pull_request` events (e.g. the `ping` GitHub sends on setup) -/// are acknowledged without acting. +/// Record one early-exit delivery diagnostic into the shared ring (#62), popping the +/// oldest when full. Mirrors [`WebhookManager::record_delivery`] but operates on the +/// `WebhookCtx`'s shared handle (the handler has no `&WebhookManager`). The `event` / +/// `repo` / etc. are whatever is known at the exit point. +fn record_into(ring: &Arc>>, d: WebhookDelivery) { + let mut ring = ring.lock().unwrap(); + if ring.len() >= DELIVERY_RING_CAP { + ring.pop_front(); + } + ring.push_back(d); +} + +/// `POST /webhook`. Verify the GitHub HMAC, parse + route the `pull_request` payload, +/// and hand a [`ParseResult::Routable`] to the injected ingestor off the request path +/// (so GitHub gets a fast 2xx). Non-`pull_request` events (e.g. the `ping` GitHub sends +/// on setup) are acknowledged without acting. +/// +/// Records EXACTLY ONE webhook-delivery diagnostic (#62) per request: the handler +/// records the early-exit classifications (`Unauthorized` / `Ignored` / `BadPayload` / +/// `WrongRepo`) here; for a `Routable` it records NOTHING and lets `ingest_webhook` +/// record the terminal status (it knows whether the candidate gated / dispatched). async fn handle_webhook( State(ctx): State>, headers: HeaderMap, body: Bytes, ) -> StatusCode { + let received_at = ledger::now_epoch(); + let event = headers + .get("x-github-event") + .and_then(|v| v.to_str().ok()) + .unwrap_or("") + .to_string(); + // A missing header or a non-UTF-8 value both collapse to "" — equivalent to an // absent signature, which `verify_signature`'s `strip_prefix("sha256=")` gate then // rejects (fail-closed). The public endpoint never acts on an unverified request. @@ -649,30 +840,108 @@ async fn handle_webhook( .and_then(|v| v.to_str().ok()) .unwrap_or(""); if !verify_signature(&ctx.secret, &body, signature) { + // The signature/secret is NEVER recorded — only that verification failed. + record_into( + &ctx.deliveries, + WebhookDelivery { + received_at_epoch: received_at, + event, + action: None, + repo: None, + pr_number: None, + kind: None, + status: DeliveryStatus::Unauthorized, + message: Some("HMAC 校验失败（签名/密钥不匹配）".to_string()), + }, + ); return StatusCode::UNAUTHORIZED; } - let event = headers - .get("x-github-event") - .and_then(|v| v.to_str().ok()) - .unwrap_or(""); if event != "pull_request" { + record_into( + &ctx.deliveries, + WebhookDelivery { + received_at_epoch: received_at, + event, + action: None, + repo: None, + pr_number: None, + kind: None, + status: DeliveryStatus::Ignored, + message: Some("非 pull_request 事件（已确认，不处理）".to_string()), + }, + ); return StatusCode::OK; } let payload: Value = match serde_json::from_slice(&body) { Ok(v) => v, - Err(_) => return StatusCode::BAD_REQUEST, + Err(_) => { + record_into( + &ctx.deliveries, + WebhookDelivery { + received_at_epoch: received_at, + event, + action: None, + repo: None, + pr_number: None, + kind: None, + status: DeliveryStatus::BadPayload, + message: Some("请求体不是合法 JSON".to_string()), + }, + ); + return StatusCode::BAD_REQUEST; + } }; - - if let Some((project_id, candidate)) = payload_to_candidate(&payload, &ctx.routes) { - // Detached: the dispatcher future is `Send + 'static`; the gates + review - // start run independently of this response. The routed `project_id` (#35) tells - // the composition root which project's gates/engine to run under. - let dispatcher = ctx.dispatcher.clone(); - drop(spawn(dispatcher(project_id, vec![candidate]))); + let action = payload + .get("action") + .and_then(Value::as_str) + .map(str::to_string); + + match parse_delivery(&payload, &ctx.routes) { + ParseResult::Routable(ev) => { + // Detached: the ingestor future is `Send + 'static`; the upsert / emit / + // dispatch run independently of this response. `ingest_webhook` records the + // terminal delivery status itself (it owns the gate/dispatch decision). + let ingestor = ctx.ingestor.clone(); + drop(spawn(ingestor(*ev))); + StatusCode::OK + } + ParseResult::WrongRepo { repo } => { + record_into( + &ctx.deliveries, + WebhookDelivery { + received_at_epoch: received_at, + event, + action, + repo, + pr_number: None, + kind: None, + status: DeliveryStatus::WrongRepo, + message: Some("仓库未匹配任何已启用项目（已忽略）".to_string()), + }, + ); + StatusCode::OK + } + // A malformed `pull_request` payload is acknowledged (200) — GitHub need not + // retry a payload we can't parse — but recorded as `BadPayload` for diagnosis. + ParseResult::Malformed => { + record_into( + &ctx.deliveries, + WebhookDelivery { + received_at_epoch: received_at, + event, + action, + repo: None, + pr_number: None, + kind: None, + status: DeliveryStatus::BadPayload, + message: Some("pull_request 载荷缺少必要字段".to_string()), + }, + ); + StatusCode::OK + } } - StatusCode::OK } /// Constant-time verify of a GitHub `X-Hub-Signature-256` header (`sha256=`) @@ -696,27 +965,42 @@ fn verify_signature(secret: &str, body: &[u8], header: &str) -> bool { mac.verify_slice(&expected).is_ok() } -/// Map a GitHub `pull_request` webhook payload to a `(project_id, Candidate)` to -/// dispatch (#35), or `None` when it routes to no enabled project or carries no single -/// trigger label. Conflict (BOTH of the routed project's trigger labels) drops here, -/// mirroring the poll path's discovery-stage conflict skip; the remaining gates -/// (cross-repo / draft / author / cooldown) are applied downstream by the dispatcher -/// closure via [`super::discover::should_skip`] / [`super::discover::cooldown_skip`], -/// so this stays a pure parse+route+map (the `is_draft` / `is_cross_repository` flags -/// it extracts are what those gates read). Pure — unit-tested without a server. -fn payload_to_candidate(payload: &Value, routes: &[ProjectRoute]) -> Option<(String, Candidate)> { - let pr = payload.get("pull_request")?; - - // Repo-routing gate (#35, was F2's single-repo ownership gate): the HMAC proves the - // POST came from a sender who knows the secret — NOT that the event is for a repo THIS - // app monitors/reviews. With one global receiver serving many projects, route the - // event to the ENABLED project whose `repo` matches; a payload matching none is - // dropped (fail-closed — same spirit as the old single-repo gate, so a misconfigured - // webhook / reused secret on an unmonitored repo can't cross-trigger a review). - // Match the event's repo (top-level `repository.full_name`, falling back to the PR's - // `base.repo.full_name`) case-insensitively against each route's `repo` (GitHub's - // repo-name semantics, matching the poll path's `gh --repo`). The matched route - // supplies the project id to dispatch under AND the labels to classify by. +/// Parse + route + classify a GitHub `pull_request` webhook payload (#61) into a +/// [`ParseResult`]. PURE (no `AppHandle`) so the whole classification is unit-tested +/// without a server; the `AppHandle`-bound ingest (`commands::ingest_webhook`) consumes +/// the `Routable` arm. +/// +/// Unlike the old `payload_to_candidate` (which returned `None` for every non-dispatch +/// case, so a webhook never touched the persisted PR list), this surfaces the FULL +/// outcome the ingest needs to upsert + emit even when nothing dispatches (the #61 fix): +/// +/// - missing `pull_request`, or a required field (`number` / `head.sha` / `head.ref`) +/// absent → [`ParseResult::Malformed`]; +/// - repo matches no enabled route → [`ParseResult::WrongRepo`] (fail-closed: HMAC +/// proves the secret is known, NOT that the event is for a monitored repo); +/// - PR not open (closed/merged) → `Routable` with [`IngestIntent::StatusOnly`] +/// ("PR 已关闭或合并") so the list row reflects it (the poll path never lists closed +/// PRs; this is the push-path equivalent — list-only, never dispatched); +/// - open, BOTH trigger labels → `Track { candidate: None, conflict: true }` (mirrors +/// the poll path's discovery-stage conflict drop — upserted as a skipped row); +/// - open, exactly one trigger label → `Track { candidate: Some(..), conflict: false }` +/// (the remaining static/cooldown gates run downstream in `webhook_view`); +/// - open, NEITHER trigger label → `StatusOnly` ("触发 label 已移除"). +/// +/// The metadata (title / labels / url) is extracted for the list row regardless of the +/// dispatch decision — the webhook payload carries it, so the ingest never re-fetches +/// via `gh pr view`. +fn parse_delivery(payload: &Value, routes: &[ProjectRoute]) -> ParseResult { + let Some(pr) = payload.get("pull_request") else { + return ParseResult::Malformed; + }; + + // Repo-routing gate (#35, was F2's single-repo ownership gate): match the event's + // repo (top-level `repository.full_name`, falling back to the PR's + // `base.repo.full_name`) case-insensitively against each route's `repo`. A payload + // matching none is dropped fail-closed (the HMAC proves the secret is known, not + // that the event is for a repo this app monitors). The matched route supplies the + // project id to track under AND the labels to classify by. let event_repo = payload .get("repository") .and_then(|r| r.get("full_name")) @@ -726,41 +1010,68 @@ fn payload_to_candidate(payload: &Value, routes: &[ProjectRoute]) -> Option<(Str .and_then(|b| b.get("repo")) .and_then(|r| r.get("full_name")) .and_then(Value::as_str) - })?; - let route = routes - .iter() - .find(|r| r.repo.eq_ignore_ascii_case(event_repo))?; - - // Parity with the poll path's `--state open` (`gh.rs`): only an OPEN PR is a - // dispatch candidate. A closed/merged PR still carrying a trigger label (a - // `closed` delivery, or a label touched post-merge) must NOT start a review — the - // poll path never lists closed PRs; this is the push-path equivalent. Missing / - // non-`"open"` state fails safe to no candidate. - if pr.get("state").and_then(Value::as_str) != Some("open") { - return None; - } + }); + let route = match event_repo + .and_then(|repo| routes.iter().find(|r| r.repo.eq_ignore_ascii_case(repo))) + { + Some(r) => r, + None => { + // Carry the repo (when known) for the delivery diagnostic. + return ParseResult::WrongRepo { + repo: event_repo.map(str::to_string), + }; + } + }; - let label_names: Vec<&str> = pr - .get("labels")? - .as_array()? - .iter() - .filter_map(|l| l.get("name").and_then(Value::as_str)) - .collect(); - // Classify with the MATCHED project's labels (#35) — review/check labels are - // per-project, so a payload routed to project B is classified by B's labels. - let has_review = label_names.contains(&route.review_label.as_str()); - let has_check = label_names.contains(&route.check_label.as_str()); - let kind = match (has_review, has_check) { - (true, true) => return None, // conflict — both trigger labels (poll path skips too) - (true, false) => "review", - (false, true) => "check", - (false, false) => return None, + // Required fields for a usable row + dispatch candidate. A `pull_request` lacking + // any of these is malformed (GitHub always sends them on a real PR event). + let Some(number) = pr.get("number").and_then(Value::as_u64) else { + return ParseResult::Malformed; + }; + let head = pr.get("head"); + let Some(head_sha) = head + .and_then(|h| h.get("sha")) + .and_then(Value::as_str) + .map(str::to_string) + else { + return ParseResult::Malformed; + }; + let Some(head_ref) = head + .and_then(|h| h.get("ref")) + .and_then(Value::as_str) + .map(str::to_string) + else { + return ParseResult::Malformed; }; - let number = pr.get("number")?.as_u64()?; - let head = pr.get("head")?; - let head_sha = head.get("sha")?.as_str()?.to_string(); - let head_ref = head.get("ref")?.as_str()?.to_string(); + // Display metadata for the list row — extracted regardless of the dispatch decision + // so a webhook-tracked row carries title/labels/url without a `gh pr view` round + // trip. `html_url` is the field GitHub's PR webhook carries (the poll path uses + // gh's `url`, the same PR HTML URL); absent → "". + let title = pr + .get("title") + .and_then(Value::as_str) + .unwrap_or("") + .to_string(); + let url = pr + .get("html_url") + .and_then(Value::as_str) + .unwrap_or("") + .to_string(); + let labels: Vec = pr + .get("labels") + .and_then(Value::as_array) + .map(|arr| { + arr.iter() + .filter_map(|l| l.get("name").and_then(Value::as_str).map(str::to_string)) + .collect() + }) + .unwrap_or_default(); + let action = payload + .get("action") + .and_then(Value::as_str) + .map(str::to_string); + let author = pr .get("user") .and_then(|u| u.get("login")) @@ -777,25 +1088,76 @@ fn payload_to_candidate(payload: &Value, routes: &[ProjectRoute]) -> Option<(Str .and_then(Value::as_str) .map(str::to_string) }; - let head_repo = full_name(head.get("repo")); + let head_repo = full_name(head.and_then(|h| h.get("repo"))); let base_repo = full_name(pr.get("base").and_then(|b| b.get("repo"))); let is_cross_repository = match (head_repo, base_repo) { (Some(h), Some(b)) => h != b, _ => true, }; - Some(( - route.id.clone(), - Candidate { - number, - head_sha, - head_ref, - author, - is_cross_repository, - is_draft, - kind: kind.to_string(), + // Helper to build the routed event with a given intent (the metadata is shared). + let event = |intent: IngestIntent| WebhookEvent { + project_id: route.id.clone(), + action: action.clone(), + repo: route.repo.clone(), + number, + title: title.clone(), + labels: labels.clone(), + url: url.clone(), + intent, + }; + + // Parity with the poll path's `--state open` (`gh.rs`): a closed/merged PR is NOT a + // dispatch candidate, but unlike the old drop it now upserts a list row reflecting + // the closed state (StatusOnly — list-only, never dispatched). + if pr.get("state").and_then(Value::as_str) != Some("open") { + return ParseResult::Routable(Box::new(event(IngestIntent::StatusOnly { + reason: "PR 已关闭或合并".to_string(), + }))); + } + + // Classify with the MATCHED project's labels (#35) — review/check labels are + // per-project, so a payload routed to project B is classified by B's labels. + let has_review = labels.iter().any(|l| l == &route.review_label); + let has_check = labels.iter().any(|l| l == &route.check_label); + let intent = match (has_review, has_check) { + // Both trigger labels → conflict: track as a skipped row, never dispatch (mirrors + // the poll path's discovery-stage conflict drop). Kind "review" for the view. + (true, true) => IngestIntent::Track { + candidate: None, + conflict: true, + }, + (true, false) => IngestIntent::Track { + candidate: Some(Candidate { + number, + head_sha, + head_ref, + author, + is_cross_repository, + is_draft, + kind: "review".to_string(), + }), + conflict: false, }, - )) + (false, true) => IngestIntent::Track { + candidate: Some(Candidate { + number, + head_sha, + head_ref, + author, + is_cross_repository, + is_draft, + kind: "check".to_string(), + }), + conflict: false, + }, + // Neither trigger label (e.g. an `unlabeled` delivery removing the trigger): + // the PR should still appear in the list with a skip reason, but never dispatch. + (false, false) => IngestIntent::StatusOnly { + reason: "触发 label 已移除".to_string(), + }, + }; + ParseResult::Routable(Box::new(event(intent))) } /// Probe whether `cloudflared` is runnable (`cloudflared --version`). Never errors; @@ -1050,6 +1412,33 @@ mod tests { vec![route("default", "owner/repo", review_label, check_label)] } + /// Test helper: assert a `parse_delivery` result is `Routable` and return its event. + fn routable(p: &Value, routes: &[ProjectRoute]) -> WebhookEvent { + match parse_delivery(p, routes) { + ParseResult::Routable(ev) => *ev, + ParseResult::WrongRepo { repo } => { + panic!("expected Routable, got WrongRepo {{ repo: {repo:?} }}") + } + ParseResult::Malformed => panic!("expected Routable, got Malformed"), + } + } + + /// Test helper: assert a `Routable` event tracks a dispatchable candidate and return + /// it (panics on a conflict / StatusOnly / non-Routable result). + fn dispatch_candidate(p: &Value, routes: &[ProjectRoute]) -> Candidate { + match routable(p, routes).intent { + IngestIntent::Track { + candidate: Some(c), .. + } => c, + IngestIntent::Track { + candidate: None, .. + } => panic!("expected a dispatch candidate, got a conflict Track"), + IngestIntent::StatusOnly { reason } => { + panic!("expected a dispatch candidate, got StatusOnly: {reason}") + } + } + } + /// The minimal route list every webhook `start` test needs (one project for /// `owner/repo`). The receiver params are global; routing/labels live here now. fn start_routes() -> Vec { @@ -1057,12 +1446,13 @@ mod tests { } #[test] - fn payload_to_candidate_maps_review_label() { + fn parse_delivery_maps_review_label() { let p = pr_payload(&["needs-review"], serde_json::json!({})); - let (project_id, c) = - payload_to_candidate(&p, &single_route("needs-review", "needs-check")) - .expect("review candidate"); - assert_eq!(project_id, "default"); + let ev = routable(&p, &single_route("needs-review", "needs-check")); + assert_eq!(ev.project_id, "default"); + assert_eq!(ev.number, 42); + assert_eq!(ev.action.as_deref(), Some("labeled")); + let c = dispatch_candidate(&p, &single_route("needs-review", "needs-check")); assert_eq!(c.number, 42); assert_eq!(c.kind, "review"); assert_eq!(c.head_sha, "abc123"); @@ -1073,80 +1463,104 @@ mod tests { } #[test] - fn payload_to_candidate_maps_check_label() { + fn parse_delivery_carries_metadata_for_the_list_row() { + // #61: the event carries title/labels/url for the persisted list row (no + // `gh pr view` round trip). `html_url` is the field GitHub's PR webhook sends. + let p = pr_payload( + &["needs-review"], + serde_json::json!({ + "title": "Add the thing", + "html_url": "https://github.com/owner/repo/pull/42", + }), + ); + let ev = routable(&p, &single_route("needs-review", "needs-check")); + assert_eq!(ev.title, "Add the thing"); + assert_eq!(ev.url, "https://github.com/owner/repo/pull/42"); + assert_eq!(ev.labels, vec!["needs-review".to_string()]); + assert_eq!(ev.repo, "owner/repo"); + } + + #[test] + fn parse_delivery_maps_check_label() { let p = pr_payload(&["needs-check"], serde_json::json!({})); - let (project_id, c) = - payload_to_candidate(&p, &single_route("needs-review", "needs-check")) - .expect("check candidate"); - assert_eq!(project_id, "default"); + let ev = routable(&p, &single_route("needs-review", "needs-check")); + assert_eq!(ev.project_id, "default"); + let c = dispatch_candidate(&p, &single_route("needs-review", "needs-check")); assert_eq!(c.kind, "check"); } #[test] - fn payload_to_candidate_skips_conflict_and_no_trigger_label() { - // Both trigger labels → conflict → None (mirrors the poll path). + fn parse_delivery_conflict_tracks_without_a_candidate() { + // Both trigger labels → conflict → Track { candidate: None, conflict: true } + // (mirrors the poll path's discovery-stage conflict drop; upserted as a skipped + // row but never dispatched). let both = pr_payload(&["needs-review", "needs-check"], serde_json::json!({})); - assert!( - payload_to_candidate(&both, &single_route("needs-review", "needs-check")).is_none() - ); - // No trigger label → None. + match routable(&both, &single_route("needs-review", "needs-check")).intent { + IngestIntent::Track { + candidate, + conflict, + } => { + assert!(candidate.is_none(), "conflict yields no dispatch candidate"); + assert!(conflict, "both labels → conflict"); + } + other => panic!("expected a conflict Track, got something else: {other:?}"), + } + } + + #[test] + fn parse_delivery_no_trigger_label_is_status_only() { + // No trigger label (e.g. an `unlabeled` removing the trigger) → StatusOnly so + // the row still appears with a skip reason, never dispatched (#61). let none = pr_payload(&["unrelated"], serde_json::json!({})); - assert!( - payload_to_candidate(&none, &single_route("needs-review", "needs-check")).is_none() - ); + match routable(&none, &single_route("needs-review", "needs-check")).intent { + IngestIntent::StatusOnly { reason } => assert_eq!(reason, "触发 label 已移除"), + other => panic!("expected StatusOnly, got {other:?}"), + } } #[test] - fn payload_to_candidate_skips_non_open_pr() { - // A closed/merged PR carrying a trigger label must NOT dispatch (parity with - // the poll path's `--state open`). closed / merged / missing state → None. - let closed = pr_payload(&["needs-review"], serde_json::json!({ "state": "closed" })); - assert!( - payload_to_candidate(&closed, &single_route("needs-review", "needs-check")).is_none() - ); - let merged = pr_payload(&["needs-review"], serde_json::json!({ "state": "merged" })); - assert!( - payload_to_candidate(&merged, &single_route("needs-review", "needs-check")).is_none() - ); - // Defensive: a payload with no `state` field fails safe to no candidate. + fn parse_delivery_non_open_pr_is_status_only() { + // A closed/merged PR is now Routable as StatusOnly (list row reflects it) rather + // than dropped — parity with the poll path's `--state open` for DISPATCH, but the + // row still updates (#61). closed / merged → StatusOnly("PR 已关闭或合并"). + for state in ["closed", "merged"] { + let p = pr_payload(&["needs-review"], serde_json::json!({ "state": state })); + match routable(&p, &single_route("needs-review", "needs-check")).intent { + IngestIntent::StatusOnly { reason } => assert_eq!(reason, "PR 已关闭或合并"), + other => panic!("state {state}: expected StatusOnly, got {other:?}"), + } + } + // A payload with `state: null` is also non-open → StatusOnly. let no_state = pr_payload(&["needs-review"], serde_json::json!({ "state": null })); - assert!( - payload_to_candidate(&no_state, &single_route("needs-review", "needs-check")).is_none() - ); - // Sanity: the default helper payload IS open and still maps. + match routable(&no_state, &single_route("needs-review", "needs-check")).intent { + IngestIntent::StatusOnly { reason } => assert_eq!(reason, "PR 已关闭或合并"), + other => panic!("null state: expected StatusOnly, got {other:?}"), + } + // Sanity: the default helper payload IS open and dispatches. let open = pr_payload(&["needs-review"], serde_json::json!({})); - assert!( - payload_to_candidate(&open, &single_route("needs-review", "needs-check")).is_some() - ); + let _ = dispatch_candidate(&open, &single_route("needs-review", "needs-check")); } #[test] - fn payload_to_candidate_preserves_draft_and_fork_flags_for_downstream_gates() { + fn parse_delivery_preserves_draft_and_fork_flags_for_downstream_gates() { // draft + fork flags are PRESERVED (not dropped here) — the dispatcher's - // should_skip applies them. A draft fork PR still maps to a candidate; the + // should_skip applies them. A draft fork PR still yields a candidate; the // gate, not the parse, decides to skip it. let draft = pr_payload(&["needs-review"], serde_json::json!({ "draft": true })); - assert!( - payload_to_candidate(&draft, &single_route("needs-review", "needs-check")) - .unwrap() - .1 - .is_draft - ); + assert!(dispatch_candidate(&draft, &single_route("needs-review", "needs-check")).is_draft); let fork = pr_payload( &["needs-review"], serde_json::json!({ "head": { "sha": "s", "ref": "r", "repo": { "full_name": "forker/repo" } } }), ); assert!( - payload_to_candidate(&fork, &single_route("needs-review", "needs-check")) - .unwrap() - .1 + dispatch_candidate(&fork, &single_route("needs-review", "needs-check")) .is_cross_repository ); } #[test] - fn payload_to_candidate_treats_missing_repo_as_cross_repo() { + fn parse_delivery_treats_missing_repo_as_cross_repo() { // A deleted-fork head with no repo info → fail safe to cross-repo (skipped // downstream), never run codex against unattributable code. let p = pr_payload( @@ -1154,42 +1568,51 @@ mod tests { serde_json::json!({ "head": { "sha": "s", "ref": "r", "repo": null } }), ); assert!( - payload_to_candidate(&p, &single_route("needs-review", "needs-check")) - .unwrap() - .1 + dispatch_candidate(&p, &single_route("needs-review", "needs-check")) .is_cross_repository ); } #[test] - fn payload_to_candidate_none_without_pull_request() { - let p = serde_json::json!({ "action": "labeled" }); - assert!(payload_to_candidate(&p, &single_route("needs-review", "needs-check")).is_none()); + fn parse_delivery_malformed_without_pull_request_or_required_fields() { + // No `pull_request` → Malformed. + let no_pr = serde_json::json!({ "action": "labeled" }); + assert!(matches!( + parse_delivery(&no_pr, &single_route("needs-review", "needs-check")), + ParseResult::Malformed + )); + // Missing required head fields (no sha) → Malformed (route matches, but the PR + // can't form a candidate / row key). + let no_sha = pr_payload( + &["needs-review"], + serde_json::json!({ "head": { "ref": "r", "repo": { "full_name": "owner/repo" } } }), + ); + assert!(matches!( + parse_delivery(&no_sha, &single_route("needs-review", "needs-check")), + ParseResult::Malformed + )); } #[test] - fn payload_to_candidate_fails_closed_when_repo_matches_no_route() { + fn parse_delivery_wrong_repo_when_no_route_matches() { // Repo-routing gate (#35): a verified payload whose repo matches NO enabled - // route is DROPPED (HMAC proves the secret is known, not that the event is for - // a repo this app monitors). A reused secret on an unmonitored repo must not - // cross-trigger a review. Payload repo `owner/repo` (the helper default) - // against routes for `owner/a` + `owner/b` → no match → None. + // route is `WrongRepo` (HMAC proves the secret is known, not that the event is + // for a repo this app monitors). Payload repo `owner/repo` (helper default) + // against routes for `owner/a` + `owner/b` → WrongRepo carrying the repo. let p = pr_payload(&["needs-review"], serde_json::json!({})); let routes = vec![ route("a", "owner/a", "needs-review", "needs-check"), route("b", "owner/b", "needs-review", "needs-check"), ]; - assert!( - payload_to_candidate(&p, &routes).is_none(), - "a payload matching no enabled route must fail closed (None)" - ); + match parse_delivery(&p, &routes) { + ParseResult::WrongRepo { repo } => assert_eq!(repo.as_deref(), Some("owner/repo")), + other => panic!("expected WrongRepo, got {other:?}"), + } // Sanity: adding the matching route makes the SAME payload route + dispatch, - // so the None above is the routing gate, not a parse failure. + // so the WrongRepo above is the routing gate, not a parse failure. let mut routes_with_match = routes; routes_with_match.push(route("c", "owner/repo", "needs-review", "needs-check")); - let (project_id, _c) = payload_to_candidate(&p, &routes_with_match) - .expect("payload routes to the matching project"); - assert_eq!(project_id, "c"); + assert_eq!(routable(&p, &routes_with_match).project_id, "c"); } #[test] @@ -1351,7 +1774,7 @@ mod tests { #[tokio::test] async fn command_mode_start_reports_configured_public_url() { let mgr = WebhookManager::default(); - mgr.set_dispatcher(Arc::new(|_, _| Box::pin(async {}))); + mgr.set_ingestor(Arc::new(|_| Box::pin(async {}))); // port 0 → OS picks a free port; `{port}` substitutes into the (harmless) sleep // args. cloudflared_bin is bogus on purpose — command mode must NOT require it. @@ -1395,7 +1818,7 @@ mod tests { #[tokio::test] async fn command_mode_self_heals_when_child_exits() { let mgr = WebhookManager::default(); - mgr.set_dispatcher(Arc::new(|_, _| Box::pin(async {}))); + mgr.set_ingestor(Arc::new(|_| Box::pin(async {}))); let s = mgr .start( @@ -1449,7 +1872,7 @@ mod tests { #[tokio::test] async fn listener_mode_has_no_child_and_does_not_self_heal() { let mgr = WebhookManager::default(); - mgr.set_dispatcher(Arc::new(|_, _| Box::pin(async {}))); + mgr.set_ingestor(Arc::new(|_| Box::pin(async {}))); let s = mgr .start( @@ -1506,7 +1929,7 @@ mod tests { #[tokio::test] async fn stop_kills_and_reaps_tunnel_child() { let mgr = WebhookManager::default(); - mgr.set_dispatcher(Arc::new(|_, _| Box::pin(async {}))); + mgr.set_ingestor(Arc::new(|_| Box::pin(async {}))); let s = mgr .start( @@ -1571,7 +1994,7 @@ mod tests { #[tokio::test] async fn listener_mode_empty_public_url_reports_none() { let mgr = WebhookManager::default(); - mgr.set_dispatcher(Arc::new(|_, _| Box::pin(async {}))); + mgr.set_ingestor(Arc::new(|_| Box::pin(async {}))); let s = mgr .start( @@ -1648,7 +2071,7 @@ mod tests { #[tokio::test] async fn command_mode_blank_command_errs() { let mgr = WebhookManager::default(); - mgr.set_dispatcher(Arc::new(|_, _| Box::pin(async {}))); + mgr.set_ingestor(Arc::new(|_| Box::pin(async {}))); let r = mgr .start( @@ -1667,69 +2090,73 @@ mod tests { } /// F2 (now #35 routing): a verified payload whose repository matches NO enabled - /// project's route must NOT map to a candidate — the HMAC proves the secret is known, + /// project's route is `WrongRepo` (fail closed) — the HMAC proves the secret is known, /// not that the event is for a repo this app reviews. A misconfigured webhook / reused - /// secret on an unmonitored repo is dropped (fail closed). + /// secret on an unmonitored repo never tracks/dispatches. #[test] - fn payload_to_candidate_requires_matching_repo() { - // A different `base.repo.full_name` (no top-level `repository`) → None despite a - // valid trigger label: no route matches `evil/repo`. + fn parse_delivery_requires_matching_repo() { + // A different `base.repo.full_name` (no top-level `repository`) → WrongRepo despite + // a valid trigger label: no route matches `evil/repo`. let other = pr_payload( &["needs-review"], serde_json::json!({ "base": { "repo": { "full_name": "evil/repo" } } }), ); - assert!( - payload_to_candidate(&other, &single_route("needs-review", "needs-check")).is_none(), - "a payload for an unrouted repo must not dispatch" - ); + match parse_delivery(&other, &single_route("needs-review", "needs-check")) { + ParseResult::WrongRepo { repo } => assert_eq!(repo.as_deref(), Some("evil/repo")), + other => panic!("a payload for an unrouted repo must be WrongRepo, got {other:?}"), + } // Top-level `repository.full_name` (what GitHub actually sends) is honored and - // takes precedence: matching it admits the candidate (routed to the matched id). + // takes precedence: matching it routes to the matched id. let mut top = pr_payload(&["needs-review"], serde_json::json!({})); top.as_object_mut().unwrap().insert( "repository".to_string(), serde_json::json!({ "full_name": "owner/repo" }), ); assert_eq!( - payload_to_candidate(&top, &single_route("needs-review", "needs-check")) - .map(|(id, _)| id), - Some("default".to_string()) + routable(&top, &single_route("needs-review", "needs-check")).project_id, + "default" ); // Case-insensitive (GitHub repo-name semantics): a route for `Owner/Repo` matches // the event's `owner/repo`. let p = pr_payload(&["needs-review"], serde_json::json!({})); - assert!(payload_to_candidate( + let _ = dispatch_candidate( &p, &[route( "default", "Owner/Repo", "needs-review", - "needs-check" - )] - ) - .is_some()); + "needs-check", + )], + ); - // Missing repo entirely (no top-level `repository`, no `base.repo`) → None. + // Missing repo entirely (no top-level `repository`, no `base.repo`) → WrongRepo + // with `repo: None`. let no_repo = pr_payload( &["needs-review"], serde_json::json!({ "base": { "repo": null } }), ); - assert!( - payload_to_candidate(&no_repo, &single_route("needs-review", "needs-check")).is_none() - ); + match parse_delivery(&no_repo, &single_route("needs-review", "needs-check")) { + ParseResult::WrongRepo { repo } => assert!(repo.is_none()), + other => panic!("missing repo must be WrongRepo {{ repo: None }}, got {other:?}"), + } - // Empty route list (no enabled projects) → nothing can match → None. + // Empty route list (no enabled projects) → nothing can match → WrongRepo. let any = pr_payload(&["needs-review"], serde_json::json!({})); - assert!(payload_to_candidate(&any, &[]).is_none()); + assert!(matches!( + parse_delivery(&any, &[]), + ParseResult::WrongRepo { .. } + )); } /// #35: with several enabled projects sharing ONE receiver, a payload routes to the /// project whose repo matches (NOT the first in the list) AND is classified by THAT /// project's labels — project B's `b-review` admits a review under B's id even though - /// project A (a different repo, different labels) comes first. + /// project A (a different repo, different labels) comes first. A right-repo / wrong-label + /// event is StatusOnly (the row still updates, no dispatch — #61), NOT WrongRepo. #[test] - fn payload_to_candidate_routes_to_matching_project_and_uses_its_labels() { + fn parse_delivery_routes_to_matching_project_and_uses_its_labels() { let routes = vec![ route("proj-a", "owner/a", "a-review", "a-check"), route("proj-b", "owner/b", "b-review", "b-check"), @@ -1741,35 +2168,40 @@ mod tests { "repository".to_string(), serde_json::json!({ "full_name": "owner/b" }), ); - let (project_id, c) = - payload_to_candidate(&for_b, &routes).expect("routes to proj-b on a B-label match"); + let ev = routable(&for_b, &routes); assert_eq!( - project_id, "proj-b", + ev.project_id, "proj-b", "routed to the matching project, not the first" ); - assert_eq!(c.kind, "review"); + assert_eq!(dispatch_candidate(&for_b, &routes).kind, "review"); - // The SAME repo with project A's label is NOT a B trigger → dropped (labels are - // per-project; B doesn't classify on A's labels). + // The SAME repo with project A's label is NOT a B trigger → StatusOnly (labels are + // per-project; B doesn't classify on A's labels). The repo matched, so it routes to + // proj-b but yields no dispatch candidate. let mut wrong_label = pr_payload(&["a-review"], serde_json::json!({})); wrong_label.as_object_mut().unwrap().insert( "repository".to_string(), serde_json::json!({ "full_name": "owner/b" }), ); + let wl = routable(&wrong_label, &routes); + assert_eq!(wl.project_id, "proj-b"); assert!( - payload_to_candidate(&wrong_label, &routes).is_none(), - "project B does not classify on project A's labels" + matches!(wl.intent, IngestIntent::StatusOnly { .. }), + "project B does not classify on project A's labels → StatusOnly, not a candidate" ); // A payload for owner/a with B's check label is classified by A's labels (none - // match) → dropped — confirms classification uses the ROUTED project's labels. + // match) → StatusOnly under proj-a — confirms classification uses the ROUTED + // project's labels. let mut for_a = pr_payload(&["b-check"], serde_json::json!({})); for_a.as_object_mut().unwrap().insert( "repository".to_string(), serde_json::json!({ "full_name": "owner/a" }), ); + let fa = routable(&for_a, &routes); + assert_eq!(fa.project_id, "proj-a"); assert!( - payload_to_candidate(&for_a, &routes).is_none(), + matches!(fa.intent, IngestIntent::StatusOnly { .. }), "owner/a is classified by A's labels, not B's" ); } @@ -1862,7 +2294,7 @@ mod tests { tauri::async_runtime::block_on(async move { let mgr = WebhookManager::default(); - mgr.set_dispatcher(Arc::new(|_, _| Box::pin(async {}))); + mgr.set_ingestor(Arc::new(|_| Box::pin(async {}))); for i in 0..3 { let s = mgr @@ -1886,4 +2318,93 @@ mod tests { } }); } + + // Wire-shape lock for `WebhookDelivery` (#62) — the `webhook_deliveries` command's + // wire type, mirrored in `src/pr/types.ts` (Medium carrier per ai-robust.md; a field + // rename would drift the TS mirror silently). Same pattern as + // `webhook_status_wire_shape_is_camel_case`. + #[test] + fn webhook_delivery_wire_shape_is_camel_case() { + let d = WebhookDelivery { + received_at_epoch: 1_700_000_000, + event: "pull_request".to_string(), + action: Some("labeled".to_string()), + repo: Some("owner/repo".to_string()), + pr_number: Some(42), + kind: Some("review".to_string()), + status: DeliveryStatus::Dispatched, + message: None, + }; + let v = serde_json::to_value(&d).expect("WebhookDelivery serializes"); + + // camelCase keys present. + assert!(v.get("receivedAtEpoch").is_some()); + assert!(v.get("event").is_some()); + assert!(v.get("action").is_some()); + assert!(v.get("repo").is_some()); + assert!(v.get("prNumber").is_some()); + assert!(v.get("kind").is_some()); + assert!(v.get("status").is_some()); + + // snake_case forms absent — a rename would surface here. + assert!(v.get("received_at_epoch").is_none()); + assert!(v.get("pr_number").is_none()); + + // `message: None` serializes to JSON null (not omitted), so the TS mirror's + // `message: string | null` stays a closed contract. + assert_eq!(v["message"], serde_json::Value::Null); + // The status discriminator serializes camelCase (see the dedicated lock below). + assert_eq!(v["status"], "dispatched"); + } + + // Cross-agent wire contract lock for `DeliveryStatus` (#62): the frontend mirrors + // these exact camelCase strings. A variant rename or `rename_all` change surfaces here. + #[test] + fn delivery_status_serializes_to_pinned_wire_strings() { + let cases = [ + (DeliveryStatus::Unauthorized, "unauthorized"), + (DeliveryStatus::BadPayload, "badPayload"), + (DeliveryStatus::Ignored, "ignored"), + (DeliveryStatus::WrongRepo, "wrongRepo"), + (DeliveryStatus::NoTriggerLabel, "noTriggerLabel"), + (DeliveryStatus::NotOpen, "notOpen"), + (DeliveryStatus::Gated, "gated"), + (DeliveryStatus::Dispatched, "dispatched"), + (DeliveryStatus::ListUpdated, "listUpdated"), + ]; + for (status, wire) in cases { + assert_eq!( + serde_json::to_value(status).expect("DeliveryStatus serializes"), + serde_json::Value::String(wire.to_string()), + "{status:?} must serialize to {wire:?}" + ); + } + } + + // The delivery ring caps at DELIVERY_RING_CAP, popping the oldest (FIFO) when full, + // and the snapshot is oldest→newest. Drives the manager's record/snapshot directly. + #[test] + fn delivery_ring_caps_and_snapshots_oldest_first() { + let mgr = WebhookManager::default(); + for i in 0..(DELIVERY_RING_CAP as u64 + 10) { + mgr.record_delivery(WebhookDelivery { + received_at_epoch: i, + event: "pull_request".to_string(), + action: None, + repo: None, + pr_number: Some(i), + kind: None, + status: DeliveryStatus::Ignored, + message: None, + }); + } + let snap = mgr.deliveries_snapshot(); + assert_eq!(snap.len(), DELIVERY_RING_CAP, "ring capped at the cap"); + // The 10 oldest were popped: the surviving window is [10, .., CAP+9], oldest first. + assert_eq!(snap.first().unwrap().pr_number, Some(10)); + assert_eq!( + snap.last().unwrap().pr_number, + Some(DELIVERY_RING_CAP as u64 + 9) + ); + } } From 1bc2c306b4a31c16599c9c271043e856c2964f49 Mon Sep 17 00:00:00 2001 From: ghbvf <104540935+ghbvf@users.noreply.github.com> Date: Thu, 18 Jun 2026 03:33:43 +0800 Subject: [PATCH 2/4] =?UTF-8?q?feat(pr):=20webhook=20delivery=20=E5=88=97?= =?UTF-8?q?=E8=A1=A8=20+=20poll=20loop=20=E7=8A=B6=E6=80=81=E5=89=8D?= =?UTF-8?q?=E7=AB=AF=E5=B1=95=E7=A4=BA=EF=BC=88#62=EF=BC=89?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 镜像后端两个诊断 command 的 wire 形状并在 UI 展示： - types.ts: 新增 DeliveryStatus 联合 / WebhookDelivery / PollStatus（切片私有，serde camelCase） - api.ts: webhookDeliveries() / pollStatus(projectId) invoke 包装 - usePrStore.ts: 按项目 pollStatus 分区 + pollStatusActive getter + refreshPollStatus action；subscribe 两分支末尾、init、toggle 后刷新 - PollControls.vue: 轮询循环运行/暂停指示、最近成功时间、失败/停滞告警、间隔提示 + ~10s 兜底刷新 - WebhookPanel.vue: 最近 deliveries 列表（最近优先），状态徽章 + assertNever 穷尽，挂载/启停/手动刷新 + ~5s 兜底刷新 Co-Authored-By: Claude Opus 4.8 (1M context) --- src/pr/PollControls.vue | 86 +++++++++++++++++ src/pr/WebhookPanel.vue | 200 ++++++++++++++++++++++++++++++++++++++-- src/pr/api.ts | 20 +++- src/pr/types.ts | 46 +++++++++ src/pr/usePrStore.ts | 34 ++++++- 5 files changed, 375 insertions(+), 11 deletions(-) diff --git a/src/pr/PollControls.vue b/src/pr/PollControls.vue index 1a8d6ed..cbaac60 100644 --- a/src/pr/PollControls.vue +++ b/src/pr/PollControls.vue @@ -16,17 +16,58 @@ const lastPulledText = computed(() => : new Date(store.lastPulledAtActive).toLocaleTimeString(), ); +// Backend poll-loop diagnostics (#62), distinct from the optimistic `pollingActive` +// toggle: this reflects what the loop ACTUALLY did (heartbeat / last success / last +// error), so a running-but-silently-failing loop is visible. `*Epoch` fields are +// UNIX seconds — *1000 for new Date(). +const poll = computed(() => store.pollStatusActive); + +// Format an epoch-secs value as a local time, or「从未」when absent. +function epochTime(epoch: number | null | undefined): string { + return epoch == null ? "从未" : new Date(epoch * 1000).toLocaleTimeString(); +} + +const lastSuccessText = computed(() => epochTime(poll.value?.lastSuccessEpoch)); + +// Show the failure line only when there's an error AND it's the latest signal: no +// success yet, or the error epoch is newer than the last success (a recovered loop +// shouldn't keep nagging about a stale error). +const showError = computed(() => { + const p = poll.value; + if (!p || p.lastErrorEpoch == null) return false; + return p.lastSuccessEpoch == null || p.lastErrorEpoch > p.lastSuccessEpoch; +}); + +// "运行中但长时间未成功": the loop claims running but has never succeeded, or its +// last success is older than ~3 intervals — a subtle stall warning even with no hard +// error recorded. +const stalled = computed(() => { + const p = poll.value; + if (!p || !p.running) return false; + if (p.lastSuccessEpoch == null) return true; + const ageSecs = Date.now() / 1000 - p.lastSuccessEpoch; + return ageSecs > Math.max(p.intervalSecs * 3, 60); +}); + // Hold the resolved UnlistenFn so onUnmounted can invoke it. // init() subscribes (awaits the listener registration) then baselines the list // from the backend snapshot, closing the startup lost-event race (#27 F3). // As an async action, init() flattens its inner Promise, so awaiting // it yields the UnlistenFn itself. let unlisten: Awaited> | null = null; +// Backstop refresh: the store refreshes poll status on each prs:updated event and on +// toggle, but a fully-idle/failing loop emits nothing — poll the backend every ~10s +// while mounted so the heartbeat/error line keeps up to date. Cleared on unmount. +let pollTimer: ReturnType | null = null; onMounted(async () => { unlisten = await store.init(); + pollTimer = setInterval(() => { + if (activeProjectId.value) store.refreshPollStatus(activeProjectId.value); + }, 10_000); }); onUnmounted(() => { unlisten?.(); + if (pollTimer !== null) clearInterval(pollTimer); }); @@ -49,6 +90,19 @@ onUnmounted(() => {

上次拉取：{{ lastPulledText }}

+
+

+ 轮询循环： + 运行中 + 已暂停 + · 间隔 {{ poll.intervalSecs }}s +

+

最近成功：{{ lastSuccessText }}

+

运行中但长时间未成功，请检查认证 / 网络。

+

+ 最近失败：{{ poll.lastErrorMessage ?? "未知错误" }} +

+
@@ -69,4 +123,36 @@ onUnmounted(() => { font-size: var(--font-size-sm); margin: var(--space-4) 0 0; } +.poll-status { + margin-top: var(--space-2); +} +.poll-status p { + margin: var(--space-2) 0 0; +} +.status-line { + display: flex; + align-items: center; + gap: var(--space-2); +} +.running { + color: var(--color-success); + font-weight: 600; +} +.paused { + color: var(--color-text-muted); +} +.hint { + color: var(--color-text-muted); + font-size: var(--font-size-xs); +} +.warn { + color: var(--color-warn); + font-size: var(--font-size-sm); + margin: var(--space-2) 0 0; +} +.error { + color: var(--color-danger); + font-size: var(--font-size-sm); + margin: var(--space-2) 0 0; +} diff --git a/src/pr/WebhookPanel.vue b/src/pr/WebhookPanel.vue index 6ec7f7b..9ac837b 100644 --- a/src/pr/WebhookPanel.vue +++ b/src/pr/WebhookPanel.vue @@ -12,9 +12,14 @@ // cross-slice reference is a TYPE-only `import type { AppConfig }` — erased at // compile time, so it creates no runtime dependency edge (the slice-boundary test // in src/slice-boundary.test.ts allows type-only imports for exactly this reason). -import { computed, onMounted, ref } from "vue"; -import { startWebhook, stopWebhook, webhookStatus } from "./api"; -import type { WebhookStatus } from "./types"; +import { computed, onMounted, onUnmounted, ref } from "vue"; +import { + startWebhook, + stopWebhook, + webhookDeliveries, + webhookStatus, +} from "./api"; +import type { DeliveryStatus, WebhookDelivery, WebhookStatus } from "./types"; import { assertNever } from "../types"; import type { AppConfig } from "../config/types"; @@ -31,6 +36,9 @@ const status = ref(null); const busy = ref(false); const error = ref(null); const copied = ref(false); +// Recent webhook deliveries (#62) — the receiver's diagnostic ring (oldest→newest); +// reversed for most-recent-first display. Self-contained local ref, no Pinia. +const deliveries = ref([]); // Are there unsaved webhook-field edits? `start_webhook` reads the PERSISTED config, // so any draft change that hasn't been saved would NOT take effect — gating start on @@ -113,14 +121,84 @@ async function run(fn: () => Promise) { } } -onMounted(() => run(webhookStatus)); +// Pull the delivery diagnostics ring (#62). Tolerates a rejected command via the +// shared `error` ref + `toMessage` pattern — a failed fetch must not crash the panel +// nor blank the tunnel controls. +async function loadDeliveries() { + try { + deliveries.value = await webhookDeliveries(); + } catch (e) { + error.value = toMessage(e); + } +} + +// Most-recent-first view: the backend returns the ring oldest→newest, so reverse a +// shallow copy for display. +const recentDeliveries = computed(() => [...deliveries.value].reverse()); + +// Short Chinese labels for each DeliveryStatus. Keyed by the full union so adding a +// backend arm without a label fails type-checking here (Record over the union). +const statusLabels: Record = { + unauthorized: "签名校验失败", + badPayload: "载荷无效", + ignored: "已忽略", + wrongRepo: "仓库不匹配", + noTriggerLabel: "无触发标签", + notOpen: "PR 非 open", + gated: "被拦截", + dispatched: "已派发", + listUpdated: "已更新列表", +}; + +// Tone class per status, so dispatched/listUpdated read as success, the skip/gate +// outcomes as warn, and the hard failures as danger. +function statusTone(s: DeliveryStatus): "ok" | "warn" | "danger" { + switch (s) { + case "dispatched": + case "listUpdated": + return "ok"; + case "unauthorized": + case "badPayload": + case "wrongRepo": + return "danger"; + case "ignored": + case "noTriggerLabel": + case "notOpen": + case "gated": + return "warn"; + default: + // Exhaustive: a new DeliveryStatus arm fails to compile here (#62). + return assertNever(s); + } +} -function onStart() { - return run(startWebhook); +function deliveryTime(epochSecs: number): string { + return new Date(epochSecs * 1000).toLocaleTimeString(); } -function onStop() { - return run(stopWebhook); +onMounted(() => { + run(webhookStatus); + loadDeliveries(); + // Light backstop refresh while the receiver is up: new deliveries arrive + // server-side with no push channel, so poll the ring every ~5s. Cleared on unmount. + deliveryTimer = setInterval(() => { + if (status.value?.running) loadDeliveries(); + }, 5_000); +}); + +let deliveryTimer: ReturnType | null = null; +onUnmounted(() => { + if (deliveryTimer !== null) clearInterval(deliveryTimer); +}); + +async function onStart() { + await run(startWebhook); + await loadDeliveries(); +} + +async function onStop() { + await run(stopWebhook); + await loadDeliveries(); } // Re-query status without a full restart — useful when the tunnel is up but the @@ -214,6 +292,43 @@ async function copyUrl() {

{{ status.message }}

{{ error }}

+ + +
+
+

最近 deliveries

+ +
+

暂无 delivery 记录

+
    +
  • + {{ deliveryTime(d.receivedAtEpoch) }} + + {{ d.event }} + + + + + + {{ statusLabels[d.status] }} + + {{ d.message }} +
    • +
+
@@ -315,4 +430,73 @@ async function copyUrl() { font-size: var(--font-size-sm); color: var(--color-danger); } +.deliveries { + display: flex; + flex-direction: column; + gap: var(--space-3); +} +.deliveries-head { + display: flex; + align-items: center; + justify-content: space-between; +} +.sub-title { + margin: 0; + font-size: var(--font-size-sm); + color: var(--color-text); +} +.delivery-list { + list-style: none; + margin: 0; + padding: 0; + display: flex; + flex-direction: column; + gap: var(--space-2); + max-height: 240px; + overflow-y: auto; +} +.delivery-row { + display: flex; + flex-wrap: wrap; + align-items: center; + gap: var(--space-2) var(--space-3); + padding: var(--space-2) var(--space-3); + font-size: var(--font-size-xs); + background: var(--color-neutral-bg); + border-radius: var(--radius-sm); +} +.d-time { + font-family: var(--font-mono); + color: var(--color-text-muted); +} +.d-event { + font-family: var(--font-mono); + color: var(--color-text); +} +.d-repo { + color: var(--color-text-muted); +} +.d-msg { + flex-basis: 100%; + color: var(--color-text-muted); + word-break: break-word; +} +.badge { + padding: 0 var(--space-2); + border-radius: var(--radius-sm); + font-size: var(--font-size-xs); + white-space: nowrap; +} +.tone-ok { + color: var(--color-success); + background: var(--color-success-bg); +} +.tone-warn { + color: var(--color-warn); + background: var(--color-warn-bg); +} +.tone-danger { + color: var(--color-danger); + background: var(--color-danger-bg); +} diff --git a/src/pr/api.ts b/src/pr/api.ts index def28f8..b6fe572 100644 --- a/src/pr/api.ts +++ b/src/pr/api.ts @@ -2,7 +2,12 @@ // `prs:updated` event subscription. import { invoke, listen } from "../api"; import type { PrEvent, TrackedPrView } from "../types"; -import type { GhStatus, WebhookStatus } from "./types"; +import type { + GhStatus, + PollStatus, + WebhookDelivery, + WebhookStatus, +} from "./types"; // Mirrors src-tauri/src/events.rs::PRS_UPDATED_EVENT (this TS side is the // open downstream end of the event-name funnel — keep in lockstep). @@ -70,3 +75,16 @@ export function stopWebhook(): Promise { export function webhookStatus(): Promise { return invoke("webhook_status"); } + +// Webhook delivery diagnostics ring (#62): the most recent received deliveries the +// receiver classified, oldest→newest (the WebhookPanel reverses for display). No +// arguments — the ring is process-global. +export function webhookDeliveries(): Promise { + return invoke("webhook_deliveries"); +} + +// One project's poll-loop status snapshot (#62). The JS `projectId` key maps to the +// Rust `project_id` snake_case arg (same convention as pollNow/getPrs above). +export function pollStatus(projectId: string): Promise { + return invoke("poll_status", { projectId }); +} diff --git a/src/pr/types.ts b/src/pr/types.ts index 4f4bd0b..2f7941e 100644 --- a/src/pr/types.ts +++ b/src/pr/types.ts @@ -20,3 +20,49 @@ export interface WebhookStatus { cloudflaredInstalled: boolean; message: string; } + +// DeliveryStatus is the terminal classification of one received webhook delivery +// (#62), mirrored from `pr/webhook.rs::DeliveryStatus` (serde camelCase). Slice- +// private command-return mirror — not a cross-slice contract — so it lives here, +// not in src/types.ts. The Rust side pins these exact strings with a wire-shape +// golden test, so this union must stay in lockstep. +export type DeliveryStatus = + | "unauthorized" + | "badPayload" + | "ignored" + | "wrongRepo" + | "noTriggerLabel" + | "notOpen" + | "gated" + | "dispatched" + | "listUpdated"; + +// WebhookDelivery mirrors `pr/webhook.rs::WebhookDelivery` (the `webhook_deliveries` +// command's Rust return shape, serde camelCase). Slice-private diagnostics row, not +// a cross-slice contract — same rationale as WebhookStatus. The `*Epoch` field is +// UNIX seconds (u64); multiply by 1000 for `new Date(...)`. +export interface WebhookDelivery { + receivedAtEpoch: number; + event: string; + action: string | null; + repo: string | null; + prNumber: number | null; + kind: string | null; + status: DeliveryStatus; + message: string | null; +} + +// PollStatus mirrors `pr/scheduler.rs::PollStatus` (the `poll_status` command's Rust +// return shape, serde camelCase). Slice-private command-return mirror — same +// rationale as WebhookStatus. The `*Epoch` fields are UNIX seconds (u64); multiply +// by 1000 for `new Date(...)`. +export interface PollStatus { + running: boolean; + intervalSecs: number; + lastStartedEpoch: number | null; + lastSuccessEpoch: number | null; + lastErrorEpoch: number | null; + lastErrorMessage: string | null; + lastPersistEpoch: number | null; + lastDiscoveredCount: number | null; +} diff --git a/src/pr/usePrStore.ts b/src/pr/usePrStore.ts index 7334c6e..d948740 100644 --- a/src/pr/usePrStore.ts +++ b/src/pr/usePrStore.ts @@ -17,12 +17,13 @@ import { ghStatus, onPrsUpdated, pollNow, + pollStatus as fetchPollStatus, setPrArchived, startPolling, stopPolling, } from "./api"; import type { TrackedPrView } from "../types"; -import type { GhStatus } from "./types"; +import type { GhStatus, PollStatus } from "./types"; import { useProjects } from "../projects"; interface PrState { @@ -44,6 +45,10 @@ interface PrState { // gh CLI auth is process-global, not per-project — stays scalar. gh: GhStatus | null; ghLoading: boolean; + // Per-project backend poll-loop status (#62), keyed by projectId. Null until the + // first refreshPollStatus lands (or if its command rejects). Diagnostics-only — + // distinct from the optimistic `polling` flag above, which mirrors the UI toggle. + pollStatus: Record; } // A rejected Tauri invoke throws the AppError object `{ message }`; fall back to @@ -63,6 +68,7 @@ export const usePrStore = defineStore("pr", { snapshotLoaded: {}, gh: null, ghLoading: false, + pollStatus: {}, }), getters: { // Parameterized tracking-aware partitions (#38), scoped to one project (#35). @@ -120,6 +126,11 @@ export const usePrStore = defineStore("pr", { pollingActive(state): boolean { return state.polling[useProjects().activeProjectId.value] ?? true; }, + // The ACTIVE project's backend poll-loop status (#62), or null before the first + // refresh / on a rejected fetch. PollControls reads this for the diagnostics line. + pollStatusActive(state): PollStatus | null { + return state.pollStatus[useProjects().activeProjectId.value] ?? null; + }, }, actions: { // Wire the `prs:updated` push stream into state, routing each payload to the @@ -147,6 +158,10 @@ export const usePrStore = defineStore("pr", { this.error[pid] = e.message; } this.loading[pid] = false; + // Refresh the backend poll-loop status on EVERY event (both branches), so a + // running-but-failing loop still surfaces its latest error/heartbeat in the + // diagnostics line (#62). Fire-and-forget — refreshPollStatus swallows errors. + void this.refreshPollStatus(pid); }); }, // Read one project's backend PR snapshot into its partition (#35). Guarded @@ -177,7 +192,10 @@ export const usePrStore = defineStore("pr", { const unlisten = this.subscribe(); // onPrsUpdated -> Promise await unlisten; // ensure the listener is registered const activeId = useProjects().activeProjectId.value; - if (activeId) await this.loadSnapshot(activeId); // baseline active project + if (activeId) { + await this.loadSnapshot(activeId); // baseline active project + void this.refreshPollStatus(activeId); // baseline poll-loop diagnostics (#62) + } return unlisten; }, // Switch the active project (#35): persist the selection, baseline its list if @@ -215,6 +233,8 @@ export const usePrStore = defineStore("pr", { await startPolling(); for (const id of ids) this.polling[id] = true; } + // Reflect the start/stop in the active project's backend diagnostics (#62). + void this.refreshPollStatus(activeId); } catch (err) { this.error[activeId] = toMessage(err); } @@ -241,5 +261,15 @@ export const usePrStore = defineStore("pr", { this.ghLoading = false; } }, + // Read one project's backend poll-loop status into its partition (#62). Pure + // diagnostics: a rejected command leaves the prior snapshot as-is (no error + // banner, no list mutation) so a transient fetch failure can't blank the readout. + async refreshPollStatus(id: string) { + try { + this.pollStatus[id] = await fetchPollStatus(id); + } catch { + /* leave as-is */ + } + }, }, }); From 8bbf1eb4e655cf4558625f0befd4424376782125 Mon Sep 17 00:00:00 2001 From: ghbvf <104540935+ghbvf@users.noreply.github.com> Date: Thu, 18 Jun 2026 03:53:22 +0800 Subject: [PATCH 3/4] =?UTF-8?q?fix(pr):=20=E5=86=85=E7=BD=AE=20review=20?= =?UTF-8?q?=E4=BF=AE=E5=A4=8D=E2=80=94=E2=80=94=E6=B6=88=E9=99=A4=20Status?= =?UTF-8?q?Only=20=E5=AD=97=E7=AC=A6=E4=B8=B2=E8=80=A6=E5=90=88=20+=20webh?= =?UTF-8?q?ook=20=E6=8C=81=E4=B9=85=E5=8C=96=E5=A4=B1=E8=B4=A5=E9=80=8F?= =?UTF-8?q?=E4=BC=A0=20+=20=E8=AF=8A=E6=96=AD=20UI/=E6=B5=8B=E8=AF=95?= =?UTF-8?q?=E8=A1=A5=E5=85=A8=EF=BC=88#61=20#62=EF=BC=89?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 六维内置 review 的 small findings 自动修： - 后端：IngestIntent::StatusOnly 改携带 sealed StatusOnlyKind（type-locked， 消除 parse_delivery↔ingest_webhook 的字符串比较耦合，Soft→Hard）；webhook mutate_tracked 失败改 emit PrEvent::Error（与 poll 路径对称，不再静默吞）； final_status 单点决策 + WriteKind 提到模块级；fork 检测改 eq_ignore_ascii_case； make_dispatcher 去重；record_delivery 委托 record_into；过期模块/类型 doc 订正； 补 poll_status golden 4 个 snake_case 缺席断言 + parse_delivery Malformed/labels-缺失/ WebhookDelivery all-None null 序列化测试。 - 前端：PollControls stalled 首轮误报修复（用 lastStartedEpoch）+ interval 友好格式化； WebhookPanel 独立 deliveryError/deliveryLoading + 时间戳含日期 + listUpdated 无 message 时给固定提示；轮询间隔魔法数常量化。 Co-Authored-By: Claude Opus 4.8 (1M context) --- src-tauri/src/lib.rs | 37 ++++--- src-tauri/src/pr/commands.rs | 108 ++++++++++++-------- src-tauri/src/pr/discover.rs | 2 +- src-tauri/src/pr/scheduler.rs | 4 + src-tauri/src/pr/webhook.rs | 180 +++++++++++++++++++++++++++++----- src/pr/PollControls.vue | 36 +++++-- src/pr/WebhookPanel.vue | 54 ++++++++-- 7 files changed, 317 insertions(+), 104 deletions(-) diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index 4f4e62d..1f58e98 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -47,13 +47,9 @@ pub fn run() { // is [`run_auto_dispatch`], the composition-root assembly that picks the // concrete engine and injects it into the engine-agnostic // [`dispatch::auto_dispatch`] (so adding an engine never edits `dispatch`). - state.scheduler.set_dispatcher(Arc::new({ - let app = app.handle().clone(); - move |project_id, cands| { - let app = app.clone(); - Box::pin(run_auto_dispatch(app, project_id, cands)) - } - })); + state + .scheduler + .set_dispatcher(make_dispatcher(app.handle().clone())); // Install the WEBHOOK trigger's ingest hook (#9 / #61). The webhook is a // second auto-trigger source: its axum handler parses + routes a push payload // into a `WebhookEvent` and hands it here. The ingest (`pr::commands::ingest_webhook`) @@ -61,17 +57,11 @@ pub fn run() { // list even when autoReview is OFF — the #61 fix), applies that project's // static/cooldown gates (`webhook_view`) + the SAME per-project `autoReview` // gate the scheduler uses, and dispatches the clean candidate by reusing the - // very same `run_auto_dispatch` (captured + cloned below, exactly as the - // scheduler's dispatcher). Keeping all this in the ingest (not the handler) is + // very same `run_auto_dispatch` via the shared `make_dispatcher` helper (the + // SAME `ProjectDispatcher` the scheduler gets). Keeping all this in the ingest (not the handler) is // what lets `pr::webhook` stay runtime-agnostic (never names AppHandle); the // ingest also records the terminal delivery diagnostic (#62). - let webhook_dispatcher: pr::scheduler::ProjectDispatcher = Arc::new({ - let app = app.handle().clone(); - move |project_id, cands| { - let app = app.clone(); - Box::pin(run_auto_dispatch(app, project_id, cands)) - } - }); + let webhook_dispatcher = make_dispatcher(app.handle().clone()); state.webhook.set_ingestor(Arc::new({ let app = app.handle().clone(); move |ev| { @@ -133,6 +123,21 @@ pub fn run() { }); } +/// Build the per-cycle [`pr::scheduler::ProjectDispatcher`] both auto-trigger sources +/// share — the poll scheduler and the webhook ingestor. Both drive a dispatchable +/// `(project_id, candidates)` through the SAME [`run_auto_dispatch`] (the composition +/// root's gate + concrete-engine assembly), so this single helper removes the duplicated +/// `Arc::new(move |..| Box::pin(run_auto_dispatch(..)))` closure that was built verbatim +/// at both wiring sites. +fn make_dispatcher( + app: tauri::AppHandle, +) -> pr::scheduler::ProjectDispatcher { + Arc::new(move |project_id, cands| { + let app = app.clone(); + Box::pin(run_auto_dispatch(app, project_id, cands)) + }) +} + /// Composition-root assembly for one auto-trigger cycle: this is the ONE place that /// names the concrete review engine. It loads + validates config, builds the codex /// [`review::engines::codex::CodexEngine`] (the [`review::engine::ReviewEngine`] the diff --git a/src-tauri/src/pr/commands.rs b/src-tauri/src/pr/commands.rs index cb499b8..4363520 100644 --- a/src-tauri/src/pr/commands.rs +++ b/src-tauri/src/pr/commands.rs @@ -12,6 +12,15 @@ use super::ledger::{now_epoch, Ledger}; use super::scheduler::{PollStatus, ProjectDispatcher}; use super::webhook::{DeliveryStatus, IngestIntent, WebhookDelivery, WebhookEvent, WebhookStatus}; +/// Which registry write `ingest_webhook` performs for a parsed intent: `Upsert` is the +/// insert-or-update Track path; `UpdatePresent` is the status-only path (refresh an +/// EXISTING row, never insert). File-private + module-level (not defined inside the async +/// fn body) so the ingest reads as one straight-line decision. +enum WriteKind { + Upsert, + UpdatePresent, +} + /// Annotates one discovered row for the PR list and surfaces its dispatchable /// [`Candidate`] when nothing gates it. Conflict (both trigger labels) skips /// first, matching `router.py`'s discovery-stage drop; otherwise static gates @@ -374,21 +383,20 @@ pub(crate) async fn ingest_webhook( }; let now = now_epoch(); - // Build the list-row view + dispatch decision + terminal delivery status from the - // parsed intent. `upsert` is the insert-or-update Track path; `update_present` is the - // status-only path (refresh an EXISTING row, never insert). The `kind` for a row that - // has no candidate falls back to a sensible non-empty string. - enum WriteKind { - Upsert, - UpdatePresent, - } - let (view, dispatchable, status, message): ( + // Build the list-row view + dispatch decision + (for the already-terminal cases) the + // delivery status from the parsed intent. The terminal status for a DISPATCHABLE + // candidate is NOT decided here — it depends on the autoReview gate below + // (Dispatched vs ListUpdated), so it is finalized in ONE place after persist+dispatch + // (`final_status`) rather than pre-assigned and overwritten. For the gated / conflict / + // status-only cases the status IS terminal (no dispatch can change it), so it is set + // here as `provisional_status`. + let (view, dispatchable, provisional_status, message, write_kind): ( PullRequestView, Option, DeliveryStatus, Option, + WriteKind, ); - let write_kind: WriteKind; match intent { IngestIntent::Track { @@ -396,16 +404,18 @@ pub(crate) async fn ingest_webhook( .. } => { let (v, d) = webhook_view(cand, title, labels, url, ¶ms, &ledger, now); - // A single trigger label. If the gate passed (skip_reason None) the candidate - // is dispatchable — the autoReview gate below decides Dispatched vs ListUpdated; - // a gated one (draft/fork/author/cooldown) is a `Gated` row carrying the reason. + // A single trigger label. A gated one (draft/fork/author/cooldown) is a `Gated` + // row carrying the reason — terminal. A clean (dispatchable) one's terminal + // status (Dispatched vs ListUpdated) is decided by the autoReview gate below, so + // `provisional_status` here is a placeholder ONLY consulted when `dispatchable` + // is `None`; `final_status` always overrides it for the dispatchable path. let (st, msg) = match (&d, &v.skip_reason) { (Some(_), _) => (DeliveryStatus::Dispatched, None), (None, reason) => (DeliveryStatus::Gated, reason.clone()), }; view = v; dispatchable = d; - status = st; + provisional_status = st; message = msg; write_kind = WriteKind::Upsert; } @@ -424,28 +434,21 @@ pub(crate) async fn ingest_webhook( skip_reason: Some(discover::BOTH_TRIGGER_LABELS_REASON.to_string()), }; dispatchable = None; - status = DeliveryStatus::Gated; + provisional_status = DeliveryStatus::Gated; message = Some(discover::BOTH_TRIGGER_LABELS_REASON.to_string()); write_kind = WriteKind::Upsert; } - IngestIntent::StatusOnly { reason } => { + IngestIntent::StatusOnly { kind: status_kind } => { // Closed/merged or trigger-label-removed: refresh an existing row's status, - // never insert, never dispatch. `kind` from the current labels (review/check) - // or "review" as a sensible default. - let kind = if labels.iter().any(|l| l == ¶ms.review_label) { - "review" - } else if labels.iter().any(|l| l == ¶ms.check_label) { + // never insert, never dispatch. `kind` from the current labels (check vs the + // review default). The reason text + terminal delivery status both come from the + // type-locked `StatusOnlyKind` (no string compare — see FIX 1 / ai-robust.md). + let kind = if labels.iter().any(|l| l == ¶ms.check_label) { "check" } else { "review" }; - // The terminal delivery status distinguishes a closed PR (NotOpen) from a - // trigger-label-removed one (NoTriggerLabel) by the reason `parse_delivery` set. - let st = if reason == "PR 已关闭或合并" { - DeliveryStatus::NotOpen - } else { - DeliveryStatus::NoTriggerLabel - }; + let reason = status_kind.reason().to_string(); view = PullRequestView { number, title, @@ -455,7 +458,7 @@ pub(crate) async fn ingest_webhook( skip_reason: Some(reason.clone()), }; dispatchable = None; - status = st; + provisional_status = status_kind.delivery_status(); message = Some(reason); write_kind = WriteKind::UpdatePresent; } @@ -496,28 +499,49 @@ pub(crate) async fn ingest_webhook( } // Persist no-op (untracked status-only PR) — nothing to emit. Ok(None) => {} - // A store failure leaves the list unchanged; the delivery diagnostic below still - // records the (would-be) terminal status so the panel surfaces the event. - Err(_) => {} + // A store failure surfaces via `PrEvent::Error` (the project's error banner) — + // SYMMETRIC with the poll path (`scheduler::persist_event` emits `PrEvent::Error` + // on a persist failure). The delivery record below still reflects the dispatch + // decision: dispatch is INDEPENDENT of persistence (it still runs if + // dispatchable + autoReview), the same contract as the poll path. The list is + // unchanged, but the banner tells the user the persist failed. + Err(e) => { + let _ = app.emit( + crate::events::PRS_UPDATED_EVENT, + &crate::events::PrEvent::Error { + project_id: project_id.clone(), + message: format!("PR 列表持久化失败：{}", e.message), + }, + ); + } } // Dispatch the gated-clean candidate iff autoReview is on (the SAME per-project gate // the scheduler applies at its call site). Detached spawn, mirroring the scheduler's - // detached dispatch (a stop must not cancel a start in flight). - let mut final_status = status; - if let Some(cand) = dispatchable { - if super::scheduler::auto_review_enabled(app, &project_id) { + // detached dispatch (a stop must not cancel a start in flight). The terminal delivery + // status is decided HERE, in ONE place, from the dispatch decision — a `dispatchable` + // candidate becomes `Dispatched` (autoReview on) or `ListUpdated` (autoReview off); + // every other case keeps its already-terminal `provisional_status`. + let final_status = match dispatchable { + Some(cand) if super::scheduler::auto_review_enabled(app, &project_id) => { drop(tauri::async_runtime::spawn(dispatcher( project_id.clone(), vec![cand], ))); - final_status = DeliveryStatus::Dispatched; - } else { - // A clean candidate but autoReview off: the list was updated, no dispatch — - // by design (#61: webhook PRs enter the list even with autoReview off). - final_status = DeliveryStatus::ListUpdated; + DeliveryStatus::Dispatched } - } + // A clean candidate but autoReview off: the list was updated, no dispatch — by + // design (#61: webhook PRs enter the list even with autoReview off). This + // AppHandle-bound path (autoReview-off → ListUpdated, the #61 core "PR enters the + // list even with autoReview off") is verified via integration / manual verify, NOT + // a unit test — `ingest_webhook` is generic over `tauri::Runtime` and an + // `AppHandle` isn't constructible in a plain `#[test]`, so the coverage story for + // this branch lives in the verify pass, not in `mod tests`. + Some(_) => DeliveryStatus::ListUpdated, + // No dispatch candidate (gated / conflict / status-only): the status set in the + // intent match is already terminal. + None => provisional_status, + }; // Record the single terminal delivery diagnostic (#62) for this routable event. record_webhook_delivery( diff --git a/src-tauri/src/pr/discover.rs b/src-tauri/src/pr/discover.rs index 91abe15..a9ac83e 100644 --- a/src-tauri/src/pr/discover.rs +++ b/src-tauri/src/pr/discover.rs @@ -1,5 +1,5 @@ //! Gating + dedup — pure port of `router.py`'s `should_skip` / -//! `recent_dispatch_reason` / `live_gate_skip`. +//! `recent_dispatch_reason` / `live_gate_skip` (reserved). //! //! These are pure predicates over a [`Candidate`], the [`MonitorParams`] config //! snapshot, the [`Ledger`], and a clock, so the discovery semantics are unit- diff --git a/src-tauri/src/pr/scheduler.rs b/src-tauri/src/pr/scheduler.rs index c81d5c9..0ab6f24 100644 --- a/src-tauri/src/pr/scheduler.rs +++ b/src-tauri/src/pr/scheduler.rs @@ -1029,6 +1029,10 @@ mod tests { assert!(v.get("interval_secs").is_none()); assert!(v.get("last_started_epoch").is_none()); assert!(v.get("last_discovered_count").is_none()); + assert!(v.get("last_success_epoch").is_none()); + assert!(v.get("last_error_epoch").is_none()); + assert!(v.get("last_error_message").is_none()); + assert!(v.get("last_persist_epoch").is_none()); // `None` fields serialize to JSON null (not omitted), so the TS mirror's // optional-or-null contract stays closed. diff --git a/src-tauri/src/pr/webhook.rs b/src-tauri/src/pr/webhook.rs index c3d8555..2baf8a6 100644 --- a/src-tauri/src/pr/webhook.rs +++ b/src-tauri/src/pr/webhook.rs @@ -18,9 +18,9 @@ //! list); a webhook is push-shaped (GitHub hands us one event). Per the //! [`crate::dispatch`] doc, "a future webhook trigger calls the same `auto_dispatch` //! with the candidates a push event yields" — that is exactly this module: the -//! handler maps a payload to a `(project_id, Candidate)` and hands it to the injected -//! [`ProjectDispatcher`] (the composition root's gate + `auto_dispatch` closure), -//! reusing the entire vetted dispatch path with zero duplication. +//! handler maps a payload to a [`WebhookEvent`] and hands it to the injected +//! [`WebhookIngestor`] (the composition root's upsert/emit + gate + `auto_dispatch` +//! closure), reusing the entire vetted dispatch path with zero duplication. //! //! **Multi-project routing (#35).** One global receiver / port / secret / tunnel //! serves EVERY monitored project. The handler routes each verified event to the @@ -75,13 +75,15 @@ use crate::model::{Candidate, WebhookTunnelMode}; type HmacSha256 = Hmac; -/// The webhook ingest hook the composition root installs (replacing the old raw -/// `ProjectDispatcher`). Called with one parsed, routed [`WebhookEvent`]; its body -/// (the root's `ingest_webhook` wrapper in `commands.rs`) upserts the persisted PR -/// list, emits `prs:updated`, and dispatches the gated candidate — keeping the axum -/// handler runtime-agnostic (the closure holds the concrete `AppHandle`, the -/// handler never names it). Boxed-future + `Arc` so it is `Clone`able into the -/// `WebhookCtx` the handler shares, mirroring [`super::scheduler::ProjectDispatcher`]. +/// The webhook ingest hook the composition root installs. Called with one parsed, routed +/// [`WebhookEvent`]; its body (the root's `ingest_webhook` wrapper in `commands.rs`) +/// upserts the persisted PR list, emits `prs:updated`, and dispatches the gated candidate +/// — keeping the axum handler runtime-agnostic (the closure holds the concrete +/// `AppHandle`, the handler never names it). Boxed-future + `Arc` so it is `Clone`able +/// into the `WebhookCtx` the handler shares. Installed once as a closure and Arc-shared — +/// the same lifecycle convention as [`super::scheduler::ProjectDispatcher`] (their +/// signatures differ: this takes a [`WebhookEvent`], `ProjectDispatcher` takes +/// `(String, Vec)`). pub type WebhookIngestor = Arc Pin + Send>> + Send + Sync>; @@ -201,8 +203,46 @@ pub enum IngestIntent { }, /// The PR should appear in the list with a skip reason but never dispatch: a /// closed/merged PR, or an open PR whose trigger label was removed. The ingest - /// refreshes an EXISTING row's status (no insert). - StatusOnly { reason: String }, + /// refreshes an EXISTING row's status (no insert). The [`StatusOnlyKind`] + /// SINGLE-SOURCES both the skip-reason text and the terminal [`DeliveryStatus`]. + StatusOnly { kind: StatusOnlyKind }, +} + +/// Which status-only outcome a non-dispatch [`IngestIntent::StatusOnly`] is (#61). The +/// Hard carrier (sealed enum) for the reason-text ↔ delivery-status pairing per +/// `.claude/rules/prmonitor/ai-robust.md`: it SINGLE-SOURCES both the human-readable +/// skip reason and the terminal [`DeliveryStatus`], so the ingest can no longer derive +/// the status from a free-form string compare (the old `if reason == "PR 已关闭或合并"` +/// cross-function string protocol — fragile, Soft). A new status-only case must add a +/// variant here and the compiler forces both [`Self::reason`] and +/// [`Self::delivery_status`] to handle it, making a reason/status mismatch unexpressible. +#[derive(Debug, Clone, Copy)] +pub enum StatusOnlyKind { + /// The PR is not open (closed / merged) — list row reflects it, never dispatched. + ClosedOrMerged, + /// An OPEN PR carrying neither trigger label (label removed) — list-only, no dispatch. + TriggerLabelRemoved, +} + +impl StatusOnlyKind { + /// The human-readable (Chinese) skip reason for this status-only outcome — the row's + /// `skip_reason` and the delivery diagnostic's `message`. Single-sourced here so the + /// text can't drift between `parse_delivery` and `ingest_webhook`. + pub fn reason(self) -> &'static str { + match self { + StatusOnlyKind::ClosedOrMerged => "PR 已关闭或合并", + StatusOnlyKind::TriggerLabelRemoved => "触发 label 已移除", + } + } + + /// The terminal [`DeliveryStatus`] for this status-only outcome. Single-sourced here + /// so `ingest_webhook` maps it by type, never by re-comparing the reason string. + pub fn delivery_status(self) -> DeliveryStatus { + match self { + StatusOnlyKind::ClosedOrMerged => DeliveryStatus::NotOpen, + StatusOnlyKind::TriggerLabelRemoved => DeliveryStatus::NoTriggerLabel, + } + } } /// cloudflared prints the assigned Quick Tunnel URL to stderr within a few seconds; @@ -444,11 +484,10 @@ impl WebhookManager { /// status. Sync + interior-mutable so it is callable from the runtime-agnostic /// handler and from `ingest_webhook` alike (via `AppState`). pub fn record_delivery(&self, d: WebhookDelivery) { - let mut ring = self.deliveries.lock().unwrap(); - if ring.len() >= DELIVERY_RING_CAP { - ring.pop_front(); - } - ring.push_back(d); + // Delegate to the free `record_into` so the ring-push (cap + pop-front) lives in + // ONE implementation — the handler records via `record_into(&ctx.deliveries, ..)`, + // this records via the manager's own handle, both through the same body. + record_into(&self.deliveries, d); } /// Snapshot the delivery ring oldest→newest (#62) for the `webhook_deliveries` @@ -1091,7 +1130,9 @@ fn parse_delivery(payload: &Value, routes: &[ProjectRoute]) -> ParseResult { let head_repo = full_name(head.and_then(|h| h.get("repo"))); let base_repo = full_name(pr.get("base").and_then(|b| b.get("repo"))); let is_cross_repository = match (head_repo, base_repo) { - (Some(h), Some(b)) => h != b, + // Case-insensitive: GitHub `full_name` is case-insensitive (same as the route + // match's `eq_ignore_ascii_case`), so a case-only difference is NOT a fork. + (Some(h), Some(b)) => !h.eq_ignore_ascii_case(&b), _ => true, }; @@ -1112,7 +1153,7 @@ fn parse_delivery(payload: &Value, routes: &[ProjectRoute]) -> ParseResult { // the closed state (StatusOnly — list-only, never dispatched). if pr.get("state").and_then(Value::as_str) != Some("open") { return ParseResult::Routable(Box::new(event(IngestIntent::StatusOnly { - reason: "PR 已关闭或合并".to_string(), + kind: StatusOnlyKind::ClosedOrMerged, }))); } @@ -1154,7 +1195,7 @@ fn parse_delivery(payload: &Value, routes: &[ProjectRoute]) -> ParseResult { // Neither trigger label (e.g. an `unlabeled` delivery removing the trigger): // the PR should still appear in the list with a skip reason, but never dispatch. (false, false) => IngestIntent::StatusOnly { - reason: "触发 label 已移除".to_string(), + kind: StatusOnlyKind::TriggerLabelRemoved, }, }; ParseResult::Routable(Box::new(event(intent))) @@ -1433,8 +1474,11 @@ mod tests { IngestIntent::Track { candidate: None, .. } => panic!("expected a dispatch candidate, got a conflict Track"), - IngestIntent::StatusOnly { reason } => { - panic!("expected a dispatch candidate, got StatusOnly: {reason}") + IngestIntent::StatusOnly { kind } => { + panic!( + "expected a dispatch candidate, got StatusOnly: {}", + kind.reason() + ) } } } @@ -1510,12 +1554,42 @@ mod tests { #[test] fn parse_delivery_no_trigger_label_is_status_only() { // No trigger label (e.g. an `unlabeled` removing the trigger) → StatusOnly so - // the row still appears with a skip reason, never dispatched (#61). + // the row still appears with a skip reason, never dispatched (#61). Assert via the + // type-locked `StatusOnlyKind` (no bare string coupling — the reason text lives on + // the enum), and that the kind's `reason()` is the expected one. let none = pr_payload(&["unrelated"], serde_json::json!({})); match routable(&none, &single_route("needs-review", "needs-check")).intent { - IngestIntent::StatusOnly { reason } => assert_eq!(reason, "触发 label 已移除"), + IngestIntent::StatusOnly { kind } => { + assert!(matches!(kind, StatusOnlyKind::TriggerLabelRemoved)); + assert_eq!(kind.reason(), "触发 label 已移除"); + } other => panic!("expected StatusOnly, got {other:?}"), } + + // `labels` null and the `labels` key absent both fall back via `unwrap_or_default()` + // to an empty label set → neither trigger label → StatusOnly { TriggerLabelRemoved } + // (locks the fallback: a missing/null `labels` is treated as "no trigger labels", + // not a malformed payload). + for labels in [serde_json::json!(null), serde_json::Value::Null] { + let mut p = pr_payload(&["unrelated"], serde_json::json!({})); + // Overwrite the PR's `labels` with null, then also test the key being absent. + p["pull_request"]["labels"] = labels; + match routable(&p, &single_route("needs-review", "needs-check")).intent { + IngestIntent::StatusOnly { kind } => { + assert!(matches!(kind, StatusOnlyKind::TriggerLabelRemoved)); + } + other => panic!("null labels: expected StatusOnly, got {other:?}"), + } + } + // `labels` key entirely absent (removed from the PR object) → same fallback. + let mut p = pr_payload(&["unrelated"], serde_json::json!({})); + p["pull_request"].as_object_mut().unwrap().remove("labels"); + match routable(&p, &single_route("needs-review", "needs-check")).intent { + IngestIntent::StatusOnly { kind } => { + assert!(matches!(kind, StatusOnlyKind::TriggerLabelRemoved)); + } + other => panic!("absent labels key: expected StatusOnly, got {other:?}"), + } } #[test] @@ -1523,17 +1597,23 @@ mod tests { // A closed/merged PR is now Routable as StatusOnly (list row reflects it) rather // than dropped — parity with the poll path's `--state open` for DISPATCH, but the // row still updates (#61). closed / merged → StatusOnly("PR 已关闭或合并"). + // Assert via the type-locked `StatusOnlyKind` (no bare string coupling). for state in ["closed", "merged"] { let p = pr_payload(&["needs-review"], serde_json::json!({ "state": state })); match routable(&p, &single_route("needs-review", "needs-check")).intent { - IngestIntent::StatusOnly { reason } => assert_eq!(reason, "PR 已关闭或合并"), + IngestIntent::StatusOnly { kind } => { + assert!(matches!(kind, StatusOnlyKind::ClosedOrMerged)); + assert_eq!(kind.reason(), "PR 已关闭或合并"); + } other => panic!("state {state}: expected StatusOnly, got {other:?}"), } } - // A payload with `state: null` is also non-open → StatusOnly. + // A payload with `state: null` is also non-open → StatusOnly { ClosedOrMerged }. let no_state = pr_payload(&["needs-review"], serde_json::json!({ "state": null })); match routable(&no_state, &single_route("needs-review", "needs-check")).intent { - IngestIntent::StatusOnly { reason } => assert_eq!(reason, "PR 已关闭或合并"), + IngestIntent::StatusOnly { kind } => { + assert!(matches!(kind, StatusOnlyKind::ClosedOrMerged)) + } other => panic!("null state: expected StatusOnly, got {other:?}"), } // Sanity: the default helper payload IS open and dispatches. @@ -1591,6 +1671,26 @@ mod tests { parse_delivery(&no_sha, &single_route("needs-review", "needs-check")), ParseResult::Malformed )); + // Missing `number` (route matches, head is fine) → Malformed: no usable row key. + let mut no_number = pr_payload(&["needs-review"], serde_json::json!({})); + no_number["pull_request"] + .as_object_mut() + .unwrap() + .remove("number"); + assert!(matches!( + parse_delivery(&no_number, &single_route("needs-review", "needs-check")), + ParseResult::Malformed + )); + // `head` present but missing `ref` (sha present) → Malformed: a partial head can't + // form a candidate (the head_ref extraction fails). + let no_head_ref = pr_payload( + &["needs-review"], + serde_json::json!({ "head": { "sha": "s", "repo": { "full_name": "owner/repo" } } }), + ); + assert!(matches!( + parse_delivery(&no_head_ref, &single_route("needs-review", "needs-check")), + ParseResult::Malformed + )); } #[test] @@ -2357,6 +2457,32 @@ mod tests { assert_eq!(v["status"], "dispatched"); } + // Every `Option` field of `WebhookDelivery` serializes to JSON `null` (NOT omitted) + // when `None`, so the TS mirror's `field: T | null` stays a closed contract (an + // `Option` that omitted-on-None would force the TS side to also mark the field + // optional `?`, drifting the shape). An all-None instance pins this for each field. + #[test] + fn webhook_delivery_all_none_fields_serialize_to_json_null() { + let d = WebhookDelivery { + received_at_epoch: 0, + event: String::new(), + action: None, + repo: None, + pr_number: None, + kind: None, + status: DeliveryStatus::Ignored, + message: None, + }; + let v = serde_json::to_value(&d).expect("WebhookDelivery serializes"); + for field in ["action", "repo", "prNumber", "kind", "message"] { + assert_eq!( + v[field], + serde_json::Value::Null, + "{field} must serialize to JSON null (not be omitted) when None" + ); + } + } + // Cross-agent wire contract lock for `DeliveryStatus` (#62): the frontend mirrors // these exact camelCase strings. A variant rename or `rename_all` change surfaces here. #[test] diff --git a/src/pr/PollControls.vue b/src/pr/PollControls.vue index cbaac60..9e27465 100644 --- a/src/pr/PollControls.vue +++ b/src/pr/PollControls.vue @@ -10,6 +10,10 @@ import { useProjects } from "../projects"; const store = usePrStore(); const { activeProjectId } = useProjects(); +// Backstop poll-status refresh cadence: a fully-idle/failing loop emits no events, so +// re-query the heartbeat/error line on this interval while mounted. +const POLL_STATUS_REFRESH_MS = 10_000; + const lastPulledText = computed(() => store.lastPulledAtActive === null ? "从未" @@ -29,6 +33,12 @@ function epochTime(epoch: number | null | undefined): string { const lastSuccessText = computed(() => epochTime(poll.value?.lastSuccessEpoch)); +// Friendlier interval readout: whole minutes render as「N 分钟」, anything else as raw +// seconds. +function formatInterval(secs: number): string { + return secs >= 60 && secs % 60 === 0 ? `${secs / 60} 分钟` : `${secs}s`; +} + // Show the failure line only when there's an error AND it's the latest signal: no // success yet, or the error epoch is newer than the last success (a recovered loop // shouldn't keep nagging about a stale error). @@ -38,15 +48,25 @@ const showError = computed(() => { return p.lastSuccessEpoch == null || p.lastErrorEpoch > p.lastSuccessEpoch; }); -// "运行中但长时间未成功": the loop claims running but has never succeeded, or its -// last success is older than ~3 intervals — a subtle stall warning even with no hard -// error recorded. +// "运行中但长时间未成功": the loop claims running but its progress has stalled — a +// subtle warning even with no hard error recorded. Gated against the first-cycle +// false positive: a just-started loop with no success yet is NOT stalled until it's +// been running past one interval (use lastStartedEpoch to tell "never started a +// cycle" from "first cycle overran"). const stalled = computed(() => { const p = poll.value; if (!p || !p.running) return false; - if (p.lastSuccessEpoch == null) return true; - const ageSecs = Date.now() / 1000 - p.lastSuccessEpoch; - return ageSecs > Math.max(p.intervalSecs * 3, 60); + const nowSecs = Date.now() / 1000; + if (p.lastSuccessEpoch == null) { + // No success yet: not stalled until a cycle has actually been in flight longer + // than one interval. No lastStartedEpoch → never entered a cycle → not stalled. + if (p.lastStartedEpoch == null) return false; + return nowSecs - p.lastStartedEpoch > Math.max(p.intervalSecs, 60); + } + // Stale success: flag only once the last success is older than ~2 intervals, so a + // fresh success isn't mistaken for a stall. + const ageSecs = nowSecs - p.lastSuccessEpoch; + return ageSecs > Math.max(p.intervalSecs * 2, 60); }); // Hold the resolved UnlistenFn so onUnmounted can invoke it. @@ -63,7 +83,7 @@ onMounted(async () => { unlisten = await store.init(); pollTimer = setInterval(() => { if (activeProjectId.value) store.refreshPollStatus(activeProjectId.value); - }, 10_000); + }, POLL_STATUS_REFRESH_MS); }); onUnmounted(() => { unlisten?.(); @@ -95,7 +115,7 @@ onUnmounted(() => { 轮询循环： 运行中 已暂停 - · 间隔 {{ poll.intervalSecs }}s + · 间隔 {{ formatInterval(poll.intervalSecs) }}

最近成功：{{ lastSuccessText }}

运行中但长时间未成功，请检查认证 / 网络。

diff --git a/src/pr/WebhookPanel.vue b/src/pr/WebhookPanel.vue index 9ac837b..a42727e 100644 --- a/src/pr/WebhookPanel.vue +++ b/src/pr/WebhookPanel.vue @@ -39,6 +39,13 @@ const copied = ref(false); // Recent webhook deliveries (#62) — the receiver's diagnostic ring (oldest→newest); // reversed for most-recent-first display. Self-contained local ref, no Pinia. const deliveries = ref([]); +// Delivery fetch state, kept SEPARATE from the panel-wide `error`: `run()` resets +// `error` to null on every start/stop/refresh, and these fire concurrently in +// onMounted, so sharing one ref clobbers it. `deliveryLoading` also drives the +// loading-vs-empty distinction and disables the refresh button while a fetch is in +// flight (prevents overlapping refreshes). +const deliveryError = ref(null); +const deliveryLoading = ref(false); // Are there unsaved webhook-field edits? `start_webhook` reads the PERSISTED config, // so any draft change that hasn't been saved would NOT take effect — gating start on @@ -122,13 +129,18 @@ async function run(fn: () => Promise) { } // Pull the delivery diagnostics ring (#62). Tolerates a rejected command via the -// shared `error` ref + `toMessage` pattern — a failed fetch must not crash the panel -// nor blank the tunnel controls. +// dedicated `deliveryError` ref + `toMessage` pattern — a failed fetch must not crash +// the panel nor blank the tunnel controls, and must NOT clobber the panel-wide +// `error` (which `run()` owns). `deliveryLoading` is toggled via try/finally. async function loadDeliveries() { + deliveryLoading.value = true; + deliveryError.value = null; try { deliveries.value = await webhookDeliveries(); } catch (e) { - error.value = toMessage(e); + deliveryError.value = toMessage(e); + } finally { + deliveryLoading.value = false; } } @@ -172,18 +184,38 @@ function statusTone(s: DeliveryStatus): "ok" | "warn" | "danger" { } } +// Include the date: the 50-cap ring can span midnight, and a time-only stamp makes +// cross-day entries ambiguous. function deliveryTime(epochSecs: number): string { - return new Date(epochSecs * 1000).toLocaleTimeString(); + return new Date(epochSecs * 1000).toLocaleString(undefined, { + month: "short", + day: "numeric", + hour: "2-digit", + minute: "2-digit", + second: "2-digit", + }); } +// Per-delivery explanatory line: prefer the backend message; otherwise, for a +// listUpdated delivery with no message, explain the #61 core scenario (autoReview off +// → enqueued but not dispatched) so the green status isn't reasonless. Empty string = +// nothing to show. +function deliveryDetail(d: WebhookDelivery): string { + if (d.message) return d.message; + if (d.status === "listUpdated") return "autoReview 关闭：已入列表，未派发 review"; + return ""; +} + +// Light backstop refresh cadence while the receiver is up: new deliveries arrive +// server-side with no push channel, so poll the ring on this interval. +const DELIVERY_REFRESH_MS = 5_000; + onMounted(() => { run(webhookStatus); loadDeliveries(); - // Light backstop refresh while the receiver is up: new deliveries arrive - // server-side with no push channel, so poll the ring every ~5s. Cleared on unmount. deliveryTimer = setInterval(() => { if (status.value?.running) loadDeliveries(); - }, 5_000); + }, DELIVERY_REFRESH_MS); }); let deliveryTimer: ReturnType | null = null; @@ -301,13 +333,15 @@ async function copyUrl() { -

暂无 delivery 记录

+

{{ deliveryError }}

+

加载中…

+

暂无 delivery 记录

  • {{ statusLabels[d.status] }} - {{ d.message }} + {{ deliveryDetail(d) }}
From bce2e71f62b61111aa3272da28fb232082a83189 Mon Sep 17 00:00:00 2001 From: ghbvf <104540935+ghbvf@users.noreply.github.com> Date: Thu, 18 Jun 2026 04:46:35 +0800 Subject: [PATCH 4/4] =?UTF-8?q?fix(pr):=20pr-review=20findings=20=E4=BF=AE?= =?UTF-8?q?=E5=A4=8D=E2=80=94=E2=80=94malformed=20labels=20=E6=8B=92?= =?UTF-8?q?=E6=94=B6=20+=20ingest=20=E7=BA=AF=20decision=20seam=20+=20poll?= =?UTF-8?q?=20=E7=8A=B6=E6=80=81=E4=B8=80=E8=87=B4=E6=80=A7=EF=BC=88#61=20?= =?UTF-8?q?#62=EF=BC=89?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 根因簇 C2/C5/C3/C4 的 small findings： - F3[安全] parse_delivery：labels 缺失/null/非数组 → ParseResult::Malformed （仅显式 [] 表示无触发 label），坏 payload 不再被当成有效状态更新； 改正上一轮误锁 null/absent→TriggerLabelRemoved 的测试。 - F7[测试] 抽出纯 decide_ingest seam（intent/params/ledger/autoReview → view/ WriteKind/dispatchable/status/message），ingest_webhook 退化为薄 IO 壳； 补全 7 分支单测（clean×autoReview / gated draft+cooldown / conflict / StatusOnly closed+label-removed），移除"未单测"注释。 - F4[产品] refreshPollStatus 成功后同步 polling[id]=running，按钮以后端为源。 - F5[产品] switchTo 切项目后立即刷新 pollStatus。 - F6[产品] listUpdated 文案改 mode-agnostic「未派发自动任务」（check 不再误显 review）。 - F8[测试] usePrStore.test 补 pollStatus mock + refreshPollStatus 全链路断言。 遗留（large·并发/生命周期，需人工决策，见 pm:fix）： F1[P1] webhook delivery detached spawn 乱序覆盖+过期派发；F2 spawn 即记 Dispatched 误报。 Refs: PR #66 pm:pr-review F3-F8（Discovered via /fix #66） Co-Authored-By: Claude Opus 4.8 (1M context) --- src-tauri/src/pr/commands.rs | 544 ++++++++++++++++++++++++++++------- src-tauri/src/pr/webhook.rs | 88 ++++-- src/pr/WebhookPanel.vue | 6 +- src/pr/usePrStore.test.ts | 92 ++++++ src/pr/usePrStore.ts | 13 +- 5 files changed, 609 insertions(+), 134 deletions(-) diff --git a/src-tauri/src/pr/commands.rs b/src-tauri/src/pr/commands.rs index 4363520..eef9a1f 100644 --- a/src-tauri/src/pr/commands.rs +++ b/src-tauri/src/pr/commands.rs @@ -309,8 +309,163 @@ fn webhook_view( (view, dispatchable) } +/// The full ingest decision for ONE routed [`WebhookEvent`], computed PURELY from plain +/// data by [`decide_ingest`] (no `AppHandle`) so EVERY branch is unit-tested. The +/// AppHandle-bound [`ingest_webhook`] becomes a thin shell that just performs the IO this +/// describes: `write` the row through the serialized seam, emit, spawn the dispatch when +/// `dispatchable` is `Some` AND autoReview is on, and record the delivery with `status` / +/// `message`. +struct IngestDecision { + /// The list-row view to persist (Upsert) or refresh (UpdatePresent). + view: PullRequestView, + /// Which registry write to perform for `view`. + write: WriteKind, + /// The candidate to auto-dispatch when autoReview is on; `None` = nothing to dispatch + /// (gated / conflict / status-only). Whether a `Some` is actually spawned is the + /// shell's call (it depends on autoReview), but the TERMINAL `status` below already + /// reflects the autoReview gate, so the shell never re-decides the status. + dispatchable: Option, + /// The single terminal delivery diagnostic status (#62) — finalized HERE from the + /// dispatch decision + autoReview, so the shell records it verbatim. + status: DeliveryStatus, + /// The delivery diagnostic's human-readable note (skip reason / `None` for a clean + /// dispatch). + message: Option, +} + +/// PURE webhook-ingest decision (#61/#62): maps a parsed [`IngestIntent`] + the project's +/// gating inputs to the FULL [`IngestDecision`] (view + write + dispatch + terminal +/// delivery status + message), WITHOUT any `AppHandle` so every branch is unit-tested. +/// Extracted from the old inline `ingest_webhook` body so the decision path — including the +/// #61 core "autoReview off still LISTS the PR" — has automated coverage rather than only a +/// "verified via integration / manual verify" note. Behavior-preserving: the IO shell +/// ([`ingest_webhook`]) feeds it the same data the inline match consumed and acts on its +/// output verbatim. +/// +/// `auto_review` is the project's resolved autoReview flag (the shell reads it ONCE off the +/// loaded project — the SAME per-project gate `scheduler::auto_review_enabled` resolves — +/// and passes it here so the dispatch decision and the terminal status agree on one value). +/// +/// Branch semantics (each preserved from the inline body): +/// - `Track { candidate: Some(cand), .. }`: run the SAME static + cooldown gates as the +/// poll path (via [`webhook_view`]). `write = Upsert`. Gated (skip_reason `Some`) → +/// `dispatchable = None`, `status = Gated`, `message = skip_reason`. Clean (skip_reason +/// `None`) → `dispatchable = Some(cand)`; `status = if auto_review { Dispatched } else +/// { ListUpdated }` (#61: autoReview off still lists), `message = None`. +/// - `Track { candidate: None, conflict: .. }`: both trigger labels → a skipped "review" +/// row with the conflict reason; `write = Upsert`; no dispatch; `status = Gated`. +/// - `StatusOnly { kind }`: refresh an EXISTING row's status (`write = UpdatePresent`), +/// never insert / dispatch. Reason text + terminal status BOTH come from the type-locked +/// [`StatusOnlyKind`] (no string compare — see ai-robust.md). +// The flat plain-data arg list (the row metadata + the gating inputs) is deliberate: this +// is a PURE decision seam whose whole point is to be callable from a `#[test]` with no +// `AppHandle`, so it takes exactly the data the IO shell already holds rather than an +// AppHandle-bound bundle. Bundling into a struct would just move the arg count around and +// add a single-use type — same as `review::session::start_review`'s allow. +#[allow(clippy::too_many_arguments)] +fn decide_ingest( + intent: IngestIntent, + number: u64, + title: String, + labels: Vec, + url: String, + params: &MonitorParams, + ledger: &Ledger, + now: u64, + auto_review: bool, +) -> IngestDecision { + match intent { + IngestIntent::Track { + candidate: Some(cand), + .. + } => { + let (view, dispatchable) = webhook_view(cand, title, labels, url, params, ledger, now); + // A single trigger label. A gated one (draft/fork/author/cooldown) is a `Gated` + // row carrying the reason. A clean (dispatchable) one's terminal status is + // decided HERE by the autoReview flag — Dispatched (on) vs ListUpdated (off, the + // #61 core "PR enters the list even with autoReview off"); the shell only acts on + // `dispatchable` + `auto_review`, it never re-derives the status. + match (dispatchable, &view.skip_reason) { + (Some(cand), _) => IngestDecision { + status: if auto_review { + DeliveryStatus::Dispatched + } else { + DeliveryStatus::ListUpdated + }, + message: None, + dispatchable: Some(cand), + write: WriteKind::Upsert, + view, + }, + (None, reason) => IngestDecision { + status: DeliveryStatus::Gated, + message: reason.clone(), + dispatchable: None, + write: WriteKind::Upsert, + view, + }, + } + } + IngestIntent::Track { + candidate: None, + conflict: _, + } => { + // Both trigger labels (conflict): a skipped row, never dispatched. Kind "review" + // for the view (the parse picked review for the conflict view). + let view = PullRequestView { + number, + title, + labels, + url, + kind: "review".to_string(), + skip_reason: Some(discover::BOTH_TRIGGER_LABELS_REASON.to_string()), + }; + IngestDecision { + view, + write: WriteKind::Upsert, + dispatchable: None, + status: DeliveryStatus::Gated, + message: Some(discover::BOTH_TRIGGER_LABELS_REASON.to_string()), + } + } + IngestIntent::StatusOnly { kind: status_kind } => { + // Closed/merged or trigger-label-removed: refresh an existing row's status, + // never insert, never dispatch. `kind` from the current labels (check vs the + // review default). The reason text + terminal delivery status both come from the + // type-locked `StatusOnlyKind` (no string compare — see FIX 1 / ai-robust.md). + let kind = if labels.iter().any(|l| l == ¶ms.check_label) { + "check" + } else { + "review" + }; + let reason = status_kind.reason().to_string(); + let view = PullRequestView { + number, + title, + labels, + url, + kind: kind.to_string(), + skip_reason: Some(reason.clone()), + }; + IngestDecision { + view, + write: WriteKind::UpdatePresent, + dispatchable: None, + status: status_kind.delivery_status(), + message: Some(reason), + } + } + } +} + /// The AppHandle-bound webhook ingest (#61): the body of the [`WebhookIngestor`] the -/// composition root installs. Takes ONE parsed, routed [`WebhookEvent`] and (a) upserts / +/// composition root installs. A THIN IO shell around the pure [`decide_ingest`]: it +/// resolves config + ledger (fail-closed), calls `decide_ingest`, then performs only the +/// AppHandle-bound IO — the registry `write`, the `prs:updated` emit, the detached dispatch +/// spawn, and the single delivery record. The branch LOGIC (view + write + dispatchable + +/// terminal status + message) lives in `decide_ingest` and is unit-tested there; the +/// remaining `mutate_tracked` / `emit` / `spawn` here is the untestable AppHandle shell. +/// Takes ONE parsed, routed [`WebhookEvent`] and (a) upserts / /// updates the persisted PR list row, (b) emits `prs:updated` so the list reflects the /// push WITHOUT waiting for the next poll round (the #61 fix — webhook PRs now enter the /// list even when autoReview is off), and (c) dispatches the gated-clean candidate iff @@ -328,8 +483,9 @@ fn webhook_view( /// /// Reuses the registry's single serialized write seam ([`registry::mutate_tracked`]) for /// the upsert + emit (so this can't interleave with a poll-cycle upsert / `set_pr_archived` -/// and lose a write) and the scheduler's per-project `auto_review_enabled` gate for the -/// dispatch decision — the SAME primitives both auto-trigger paths share. +/// and lose a write). The dispatch decision uses the project's `auto_review` flag — read +/// ONCE off the same loaded project that `scheduler::auto_review_enabled` resolves from, so +/// both auto-trigger paths share the SAME per-project autoReview gate. pub(crate) async fn ingest_webhook( app: &tauri::AppHandle, dispatcher: &ProjectDispatcher, @@ -374,6 +530,12 @@ pub(crate) async fn ingest_webhook( authors: project.authors, pr_cooldown_seconds: project.pr_cooldown_seconds, }; + // The project's autoReview flag, read ONCE off the SAME loaded project (the per-project + // gate `scheduler::auto_review_enabled` resolves from the same `config_service::project`). + // Reading it here — rather than re-loading config via `auto_review_enabled` after persist — + // ties the dispatch decision and the terminal delivery status to one consistent value and + // lets the pure `decide_ingest` finalize both. + let auto_review = project.auto_review; let ledger = match Ledger::load(app, &project_id) { Ok(l) => l, Err(e) => { @@ -383,87 +545,27 @@ pub(crate) async fn ingest_webhook( }; let now = now_epoch(); - // Build the list-row view + dispatch decision + (for the already-terminal cases) the - // delivery status from the parsed intent. The terminal status for a DISPATCHABLE - // candidate is NOT decided here — it depends on the autoReview gate below - // (Dispatched vs ListUpdated), so it is finalized in ONE place after persist+dispatch - // (`final_status`) rather than pre-assigned and overwritten. For the gated / conflict / - // status-only cases the status IS terminal (no dispatch can change it), so it is set - // here as `provisional_status`. - let (view, dispatchable, provisional_status, message, write_kind): ( - PullRequestView, - Option, - DeliveryStatus, - Option, - WriteKind, + // The full ingest decision (view + write + dispatch + terminal status + message) is + // computed by the PURE `decide_ingest` (unit-tested per branch); the rest of this fn is + // the thin AppHandle-bound IO shell that acts on it. + let IngestDecision { + view, + write: write_kind, + dispatchable, + status: final_status, + message, + } = decide_ingest( + intent, + number, + title, + labels, + url, + ¶ms, + &ledger, + now, + auto_review, ); - match intent { - IngestIntent::Track { - candidate: Some(cand), - .. - } => { - let (v, d) = webhook_view(cand, title, labels, url, ¶ms, &ledger, now); - // A single trigger label. A gated one (draft/fork/author/cooldown) is a `Gated` - // row carrying the reason — terminal. A clean (dispatchable) one's terminal - // status (Dispatched vs ListUpdated) is decided by the autoReview gate below, so - // `provisional_status` here is a placeholder ONLY consulted when `dispatchable` - // is `None`; `final_status` always overrides it for the dispatchable path. - let (st, msg) = match (&d, &v.skip_reason) { - (Some(_), _) => (DeliveryStatus::Dispatched, None), - (None, reason) => (DeliveryStatus::Gated, reason.clone()), - }; - view = v; - dispatchable = d; - provisional_status = st; - message = msg; - write_kind = WriteKind::Upsert; - } - IngestIntent::Track { - candidate: None, - conflict: _, - } => { - // Both trigger labels (conflict): a skipped row, never dispatched. Kind - // "review" for the view (the parse picked review for the conflict view). - view = PullRequestView { - number, - title, - labels, - url, - kind: "review".to_string(), - skip_reason: Some(discover::BOTH_TRIGGER_LABELS_REASON.to_string()), - }; - dispatchable = None; - provisional_status = DeliveryStatus::Gated; - message = Some(discover::BOTH_TRIGGER_LABELS_REASON.to_string()); - write_kind = WriteKind::Upsert; - } - IngestIntent::StatusOnly { kind: status_kind } => { - // Closed/merged or trigger-label-removed: refresh an existing row's status, - // never insert, never dispatch. `kind` from the current labels (check vs the - // review default). The reason text + terminal delivery status both come from the - // type-locked `StatusOnlyKind` (no string compare — see FIX 1 / ai-robust.md). - let kind = if labels.iter().any(|l| l == ¶ms.check_label) { - "check" - } else { - "review" - }; - let reason = status_kind.reason().to_string(); - view = PullRequestView { - number, - title, - labels, - url, - kind: kind.to_string(), - skip_reason: Some(reason.clone()), - }; - dispatchable = None; - provisional_status = status_kind.delivery_status(); - message = Some(reason); - write_kind = WriteKind::UpdatePresent; - } - } - // Persist + emit through the single serialized write seam. The Upsert path always // persists + emits (an upsert always changes the set); the UpdatePresent path persists // + emits ONLY when the row existed (mirrors `set_pr_archived`'s no-op skip), so a @@ -516,34 +618,26 @@ pub(crate) async fn ingest_webhook( } } - // Dispatch the gated-clean candidate iff autoReview is on (the SAME per-project gate - // the scheduler applies at its call site). Detached spawn, mirroring the scheduler's - // detached dispatch (a stop must not cancel a start in flight). The terminal delivery - // status is decided HERE, in ONE place, from the dispatch decision — a `dispatchable` - // candidate becomes `Dispatched` (autoReview on) or `ListUpdated` (autoReview off); - // every other case keeps its already-terminal `provisional_status`. - let final_status = match dispatchable { - Some(cand) if super::scheduler::auto_review_enabled(app, &project_id) => { + // Dispatch the gated-clean candidate iff autoReview is on. `decide_ingest` already + // gates `dispatchable` to `Some` ONLY for a clean (un-skipped) candidate AND already + // baked the autoReview flag into `final_status` (Dispatched on / ListUpdated off, the + // #61 core "PR enters the list even with autoReview off"), so the shell just spawns + // when both hold — it never re-decides the status. The same `auto_review` value drives + // both, so the spawn and the recorded status can't disagree. Detached spawn, mirroring + // the scheduler's detached dispatch (a stop must not cancel a start in flight); the + // JoinHandle is dropped explicitly so the task runs to completion regardless of caller. + if let Some(cand) = dispatchable { + if auto_review { drop(tauri::async_runtime::spawn(dispatcher( project_id.clone(), vec![cand], ))); - DeliveryStatus::Dispatched } - // A clean candidate but autoReview off: the list was updated, no dispatch — by - // design (#61: webhook PRs enter the list even with autoReview off). This - // AppHandle-bound path (autoReview-off → ListUpdated, the #61 core "PR enters the - // list even with autoReview off") is verified via integration / manual verify, NOT - // a unit test — `ingest_webhook` is generic over `tauri::Runtime` and an - // `AppHandle` isn't constructible in a plain `#[test]`, so the coverage story for - // this branch lives in the verify pass, not in `mod tests`. - Some(_) => DeliveryStatus::ListUpdated, - // No dispatch candidate (gated / conflict / status-only): the status set in the - // intent match is already terminal. - None => provisional_status, - }; + } - // Record the single terminal delivery diagnostic (#62) for this routable event. + // Record the single terminal delivery diagnostic (#62) for this routable event. The + // status came straight from `decide_ingest` — the dispatch decision above only acts on + // it, it does not override it. record_webhook_delivery( app, &repo, @@ -868,4 +962,246 @@ mod tests { "a cooldown-gated candidate is not dispatchable" ); } + + // ── `decide_ingest` (F7): the PURE ingest-decision seam extracted from the + // AppHandle-bound `ingest_webhook` body so EVERY branch (incl. the #61 core + // "autoReview off still LISTS the PR") has automated coverage rather than only the old + // "verified via integration / NOT a unit test" note. Each test asserts the FULL + // decision: view fields + write + dispatchable + terminal status + message. + use super::super::webhook::StatusOnlyKind; + + /// A clean single-label candidate wrapped as `IngestIntent::Track { candidate: Some }`. + /// `row` builds a non-draft, non-fork, allowed-author candidate → clean under the + /// empty-ledger `params()` gates, so the only thing left to vary is autoReview. + fn wrap_some(number: u64) -> IngestIntent { + IngestIntent::Track { + candidate: Some(row(number, "review", false).candidate), + conflict: false, + } + } + + #[test] + fn decide_ingest_clean_candidate_auto_review_on_dispatches() { + // #61: a clean candidate with autoReview ON → Upsert row, dispatch the candidate, + // status Dispatched, no message. + let d = decide_ingest( + wrap_some(1), + 1, + "PR 1".to_string(), + vec!["review-label".to_string()], + "https://x/1".to_string(), + ¶ms(), + &Ledger::default(), + 0, + true, + ); + assert_eq!(d.view.number, 1); + assert_eq!(d.view.kind, "review"); + assert_eq!(d.view.skip_reason, None); + assert!(matches!(d.write, WriteKind::Upsert)); + assert!(d.dispatchable.is_some(), "clean candidate is dispatchable"); + assert!(matches!(d.status, DeliveryStatus::Dispatched)); + assert_eq!(d.message, None); + } + + #[test] + fn decide_ingest_clean_candidate_auto_review_off_lists_without_dispatch() { + // THE #61 CORE: a clean candidate with autoReview OFF → still Upserts the row + + // surfaces a dispatchable (the shell just won't spawn it), status ListUpdated (NOT + // Dispatched), no message. This is the branch that previously had no unit test. + let d = decide_ingest( + wrap_some(2), + 2, + "PR 2".to_string(), + vec!["review-label".to_string()], + "https://x/2".to_string(), + ¶ms(), + &Ledger::default(), + 0, + false, + ); + assert_eq!(d.view.number, 2); + assert_eq!(d.view.skip_reason, None); + assert!(matches!(d.write, WriteKind::Upsert)); + assert!( + d.dispatchable.is_some(), + "autoReview-off still surfaces the candidate (the shell gates the spawn)" + ); + assert!( + matches!(d.status, DeliveryStatus::ListUpdated), + "autoReview off → ListUpdated, not Dispatched" + ); + assert_eq!(d.message, None); + } + + #[test] + fn decide_ingest_static_gated_candidate_is_gated_no_dispatch() { + // A draft candidate is gated by `should_skip` → Upsert a skipped row, NO dispatch, + // status Gated, message = the skip reason. autoReview on must NOT override the gate. + let mut cand = row(3, "review", false).candidate; + cand.is_draft = true; + let intent = IngestIntent::Track { + candidate: Some(cand), + conflict: false, + }; + let d = decide_ingest( + intent, + 3, + "PR 3".to_string(), + vec!["review-label".to_string()], + "https://x/3".to_string(), + ¶ms(), + &Ledger::default(), + 0, + true, + ); + assert_eq!(d.view.skip_reason, Some("draft PR".to_string())); + assert!(matches!(d.write, WriteKind::Upsert)); + assert!( + d.dispatchable.is_none(), + "a gated candidate never dispatches" + ); + assert!(matches!(d.status, DeliveryStatus::Gated)); + assert_eq!(d.message, Some("draft PR".to_string())); + } + + #[test] + fn decide_ingest_cooldown_gated_candidate_is_gated_no_dispatch() { + // A candidate within its dispatch cooldown is gated by `cooldown_skip` → Gated row, + // no dispatch, message = the cooldown reason (even with autoReview on). + use crate::pr::ledger::{dispatch_key, DispatchEvent}; + use std::collections::HashSet; + + let cand = row(4, "review", false).candidate; + let ledger = Ledger { + dispatched: HashSet::new(), + events: vec![DispatchEvent { + pr: 4, + kind: "review".to_string(), + head_sha: cand.head_sha.clone(), + key: dispatch_key(4, &cand.head_sha, "review"), + dispatched_at_epoch: 1_000, + }], + }; + let intent = IngestIntent::Track { + candidate: Some(cand), + conflict: false, + }; + // dispatched 500s before `now` (1800s cooldown) → within window. + let d = decide_ingest( + intent, + 4, + "PR 4".to_string(), + vec!["review-label".to_string()], + "https://x/4".to_string(), + ¶ms(), + &ledger, + 1_500, + true, + ); + assert!( + d.view + .skip_reason + .as_deref() + .is_some_and(|r| r.contains("within cooldown")), + "cooldown gate fires: {:?}", + d.view.skip_reason + ); + assert!(matches!(d.write, WriteKind::Upsert)); + assert!(d.dispatchable.is_none()); + assert!(matches!(d.status, DeliveryStatus::Gated)); + assert!(d + .message + .as_deref() + .is_some_and(|m| m.contains("within cooldown"))); + } + + #[test] + fn decide_ingest_conflict_is_gated_with_both_labels_reason() { + // Both trigger labels (`candidate: None, conflict: true`) → Upsert a skipped "review" + // row carrying the BOTH-labels reason, NO dispatch, status Gated. + let intent = IngestIntent::Track { + candidate: None, + conflict: true, + }; + let d = decide_ingest( + intent, + 5, + "PR 5".to_string(), + vec!["review-label".to_string(), "check-label".to_string()], + "https://x/5".to_string(), + ¶ms(), + &Ledger::default(), + 0, + true, + ); + assert_eq!(d.view.number, 5); + assert_eq!(d.view.kind, "review"); + assert_eq!( + d.view.skip_reason, + Some(discover::BOTH_TRIGGER_LABELS_REASON.to_string()) + ); + assert!(matches!(d.write, WriteKind::Upsert)); + assert!(d.dispatchable.is_none()); + assert!(matches!(d.status, DeliveryStatus::Gated)); + assert_eq!( + d.message, + Some(discover::BOTH_TRIGGER_LABELS_REASON.to_string()) + ); + } + + #[test] + fn decide_ingest_status_only_closed_is_not_open_update_present() { + // A closed/merged PR → StatusOnly { ClosedOrMerged }: UpdatePresent (refresh an + // existing row, never insert / dispatch), status NotOpen, reason "PR 已关闭或合并". + let intent = IngestIntent::StatusOnly { + kind: StatusOnlyKind::ClosedOrMerged, + }; + let d = decide_ingest( + intent, + 6, + "PR 6".to_string(), + vec!["review-label".to_string()], + "https://x/6".to_string(), + ¶ms(), + &Ledger::default(), + 0, + true, + ); + assert_eq!(d.view.number, 6); + assert_eq!(d.view.kind, "review"); + assert_eq!(d.view.skip_reason, Some("PR 已关闭或合并".to_string())); + assert!(matches!(d.write, WriteKind::UpdatePresent)); + assert!(d.dispatchable.is_none()); + assert!(matches!(d.status, DeliveryStatus::NotOpen)); + assert_eq!(d.message, Some("PR 已关闭或合并".to_string())); + } + + #[test] + fn decide_ingest_status_only_trigger_label_removed_is_no_trigger_label_update_present() { + // An open PR with the trigger label removed → StatusOnly { TriggerLabelRemoved }: + // UpdatePresent, status NoTriggerLabel, reason "触发 label 已移除". With ONLY the + // check label present, the view kind is "check" (the labels-derived kind). + let intent = IngestIntent::StatusOnly { + kind: StatusOnlyKind::TriggerLabelRemoved, + }; + let d = decide_ingest( + intent, + 7, + "PR 7".to_string(), + vec!["check-label".to_string()], + "https://x/7".to_string(), + ¶ms(), + &Ledger::default(), + 0, + false, + ); + assert_eq!(d.view.number, 7); + assert_eq!(d.view.kind, "check", "check label present → kind check"); + assert_eq!(d.view.skip_reason, Some("触发 label 已移除".to_string())); + assert!(matches!(d.write, WriteKind::UpdatePresent)); + assert!(d.dispatchable.is_none()); + assert!(matches!(d.status, DeliveryStatus::NoTriggerLabel)); + assert_eq!(d.message, Some("触发 label 已移除".to_string())); + } } diff --git a/src-tauri/src/pr/webhook.rs b/src-tauri/src/pr/webhook.rs index 2baf8a6..4ba2b9a 100644 --- a/src-tauri/src/pr/webhook.rs +++ b/src-tauri/src/pr/webhook.rs @@ -163,7 +163,8 @@ pub enum ParseResult { /// Verified, but the event's repo matched no enabled route (fail-closed drop). The /// repo (when known) is carried for the delivery diagnostic. WrongRepo { repo: Option }, - /// No `pull_request`, or a required field (number / head.sha / head.ref) is missing. + /// No `pull_request`, or a required field (number / head.sha / head.ref / labels) is + /// missing or structurally invalid (`labels` not an array — F3). Malformed, } @@ -1013,8 +1014,10 @@ fn verify_signature(secret: &str, body: &[u8], header: &str) -> bool { /// case, so a webhook never touched the persisted PR list), this surfaces the FULL /// outcome the ingest needs to upsert + emit even when nothing dispatches (the #61 fix): /// -/// - missing `pull_request`, or a required field (`number` / `head.sha` / `head.ref`) -/// absent → [`ParseResult::Malformed`]; +/// - missing `pull_request`, or a required field (`number` / `head.sha` / `head.ref` / +/// `labels`) absent / structurally invalid → [`ParseResult::Malformed`] (`labels` must +/// be an array — GitHub always sends one, possibly empty `[]`; an absent / `null` / +/// non-array `labels` is malformed, NOT silently "no trigger label" — F3); /// - repo matches no enabled route → [`ParseResult::WrongRepo`] (fail-closed: HMAC /// proves the secret is known, NOT that the event is for a monitored repo); /// - PR not open (closed/merged) → `Routable` with [`IngestIntent::StatusOnly`] @@ -1097,15 +1100,22 @@ fn parse_delivery(payload: &Value, routes: &[ProjectRoute]) -> ParseResult { .and_then(Value::as_str) .unwrap_or("") .to_string(); - let labels: Vec = pr - .get("labels") - .and_then(Value::as_array) - .map(|arr| { - arr.iter() - .filter_map(|l| l.get("name").and_then(Value::as_str).map(str::to_string)) - .collect() - }) - .unwrap_or_default(); + // `labels` is a REQUIRED structural field, same tier as number/head.sha/head.ref: + // GitHub ALWAYS sends a `labels` array on a real PR event (possibly empty `[]`), so an + // absent / `null` / non-array `labels` is a malformed payload, NOT "no labels". Coercing + // it to an empty Vec (the old `unwrap_or_default()`) silently turned a malformed payload + // into a valid "no trigger label" state update on a tracked PR (→ StatusOnly + // TriggerLabelRemoved) — F3. Validated HERE (alongside the other required-field + // extractions, before the open/closed + label classification that needs the names) so it + // rejects regardless of open/closed: a malformed payload is malformed either way. An + // EXPLICIT empty array `[]` is still valid → empty Vec → genuine "no trigger label". + let Some(labels_arr) = pr.get("labels").and_then(Value::as_array) else { + return ParseResult::Malformed; + }; + let labels: Vec = labels_arr + .iter() + .filter_map(|l| l.get("name").and_then(Value::as_str).map(str::to_string)) + .collect(); let action = payload .get("action") .and_then(Value::as_str) @@ -1566,29 +1576,53 @@ mod tests { other => panic!("expected StatusOnly, got {other:?}"), } - // `labels` null and the `labels` key absent both fall back via `unwrap_or_default()` - // to an empty label set → neither trigger label → StatusOnly { TriggerLabelRemoved } - // (locks the fallback: a missing/null `labels` is treated as "no trigger labels", - // not a malformed payload). - for labels in [serde_json::json!(null), serde_json::Value::Null] { + // F3: a `null` or non-array `labels`, and the `labels` key entirely absent, are + // structurally MALFORMED (GitHub always sends a `labels` array), NOT silently "no + // trigger label". The old behavior (`unwrap_or_default()` → empty Vec → + // TriggerLabelRemoved) turned a malformed payload into a valid state update on a + // tracked PR; this test now locks the rejection. (A `null` JSON value and a + // structurally non-array value both fail `Value::as_array`.) + for labels in [serde_json::json!(null), serde_json::json!("not-an-array")] { let mut p = pr_payload(&["unrelated"], serde_json::json!({})); - // Overwrite the PR's `labels` with null, then also test the key being absent. p["pull_request"]["labels"] = labels; - match routable(&p, &single_route("needs-review", "needs-check")).intent { - IngestIntent::StatusOnly { kind } => { - assert!(matches!(kind, StatusOnlyKind::TriggerLabelRemoved)); - } - other => panic!("null labels: expected StatusOnly, got {other:?}"), - } + assert!( + matches!( + parse_delivery(&p, &single_route("needs-review", "needs-check")), + ParseResult::Malformed + ), + "null/non-array labels must be Malformed, not a coerced empty set" + ); } - // `labels` key entirely absent (removed from the PR object) → same fallback. + // `labels` key entirely absent (removed from the PR object) → Malformed too. let mut p = pr_payload(&["unrelated"], serde_json::json!({})); p["pull_request"].as_object_mut().unwrap().remove("labels"); - match routable(&p, &single_route("needs-review", "needs-check")).intent { + assert!( + matches!( + parse_delivery(&p, &single_route("needs-review", "needs-check")), + ParseResult::Malformed + ), + "absent labels key must be Malformed" + ); + + // …but an EXPLICIT empty array `[]` is the GENUINE "no trigger label" case and stays + // valid: an OPEN PR with `[]` → StatusOnly { TriggerLabelRemoved } (list-only, no + // dispatch). This is the case the absent/null subcases above must NOT be conflated + // with — `[]` is a real "labels were removed" state, absent `labels` is malformed. + let empty_open = pr_payload(&[], serde_json::json!({})); + match routable(&empty_open, &single_route("needs-review", "needs-check")).intent { IngestIntent::StatusOnly { kind } => { assert!(matches!(kind, StatusOnlyKind::TriggerLabelRemoved)); } - other => panic!("absent labels key: expected StatusOnly, got {other:?}"), + other => panic!("empty [] on open PR: expected StatusOnly, got {other:?}"), + } + // An EXPLICIT empty array `[]` on a CLOSED PR → StatusOnly { ClosedOrMerged } (the + // closed-state check precedes label classification, so `[]` doesn't shadow it). + let empty_closed = pr_payload(&[], serde_json::json!({ "state": "closed" })); + match routable(&empty_closed, &single_route("needs-review", "needs-check")).intent { + IngestIntent::StatusOnly { kind } => { + assert!(matches!(kind, StatusOnlyKind::ClosedOrMerged)); + } + other => panic!("empty [] on closed PR: expected StatusOnly, got {other:?}"), } } diff --git a/src/pr/WebhookPanel.vue b/src/pr/WebhookPanel.vue index a42727e..62014fd 100644 --- a/src/pr/WebhookPanel.vue +++ b/src/pr/WebhookPanel.vue @@ -198,11 +198,13 @@ function deliveryTime(epochSecs: number): string { // Per-delivery explanatory line: prefer the backend message; otherwise, for a // listUpdated delivery with no message, explain the #61 core scenario (autoReview off -// → enqueued but not dispatched) so the green status isn't reasonless. Empty string = +// → enqueued but not dispatched) so the green status isn't reasonless. Mode-agnostic +// wording (#66 F6): a delivery may carry kind "review" OR "check", so the hardcoded +// "未派发 review" was wrong for check-kind PRs — say "自动任务" instead. Empty string = // nothing to show. function deliveryDetail(d: WebhookDelivery): string { if (d.message) return d.message; - if (d.status === "listUpdated") return "autoReview 关闭：已入列表，未派发 review"; + if (d.status === "listUpdated") return "autoReview 关闭：已入列表，未派发自动任务"; return ""; } diff --git a/src/pr/usePrStore.test.ts b/src/pr/usePrStore.test.ts index 77a1400..20e18d3 100644 --- a/src/pr/usePrStore.test.ts +++ b/src/pr/usePrStore.test.ts @@ -6,12 +6,30 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; import { createPinia, setActivePinia } from "pinia"; import type { PrEvent, TrackedPrView } from "../types"; +import type { PollStatus } from "./types"; import type { Project } from "../config/types"; // Captured callback handed to `onPrsUpdated`, so a test can push a `PrEvent` // through the same path `subscribe()` wires up. let prsCb: ((e: PrEvent) => void) | null = null; +// A full PollStatus shape (#66 F8) so the `pollStatus` mock resolves the same wire +// form refreshPollStatus writes; `running` defaults true so the F4 reconciliation +// keeps `polling` at its baseline unless a test overrides it. +const pollStatus = ( + over: Partial = {}, +): PollStatus => ({ + running: true, + intervalSecs: 60, + lastStartedEpoch: null, + lastSuccessEpoch: null, + lastErrorEpoch: null, + lastErrorMessage: null, + lastPersistEpoch: null, + lastDiscoveredCount: null, + ...over, +}); + vi.mock("./api", () => ({ pollNow: vi.fn(() => Promise.resolve()), startPolling: vi.fn(() => Promise.resolve()), @@ -19,6 +37,20 @@ vi.mock("./api", () => ({ ghStatus: vi.fn(() => Promise.resolve({ authenticated: true, message: "" })), getPrs: vi.fn(() => Promise.resolve([])), setPrArchived: vi.fn(() => Promise.resolve()), + // pollStatus mock (#66 F8): refreshPollStatus is fired from subscribe()/init()/ + // toggle()/switchTo(), so the chains need a resolved PollStatus to write. + pollStatus: vi.fn(() => + Promise.resolve({ + running: true, + intervalSecs: 60, + lastStartedEpoch: null, + lastSuccessEpoch: null, + lastErrorEpoch: null, + lastErrorMessage: null, + lastPersistEpoch: null, + lastDiscoveredCount: null, + }), + ), onPrsUpdated: vi.fn((cb: (e: PrEvent) => void) => { prsCb = cb; // onPrsUpdated returns a Promise. @@ -82,6 +114,8 @@ beforeEach(() => { vi.mocked(api.stopPolling).mockResolvedValue(undefined); vi.mocked(api.getPrs).mockResolvedValue([]); vi.mocked(api.setPrArchived).mockResolvedValue(undefined); + // Restore the pollStatus default wiped by clearAllMocks (#66 F8). + vi.mocked(api.pollStatus).mockResolvedValue(pollStatus()); vi.mocked(api.onPrsUpdated).mockImplementation((cb) => { prsCb = cb; return Promise.resolve(() => {}); @@ -166,6 +200,18 @@ describe("usePrStore subscribe()", () => { prsCb?.({ kind: "updated", projectId: "p2", prs: [view(10)] }); expect(store.hasNewPr.p2).toBe(false); }); + + it("refreshes the backend poll status for the event's project (#66 F8)", () => { + const store = usePrStore(); + store.subscribe(); + + prsCb?.({ kind: "updated", projectId: "p1", prs: [view(1)] }); + expect(api.pollStatus).toHaveBeenCalledWith("p1"); + + // The error branch must refresh too, so a running-but-failing loop still surfaces. + prsCb?.({ kind: "error", projectId: "p2", message: "boom" }); + expect(api.pollStatus).toHaveBeenCalledWith("p2"); + }); }); describe("usePrStore pollNow()", () => { @@ -190,15 +236,24 @@ describe("usePrStore pollNow()", () => { describe("usePrStore toggle()", () => { it("polling -> paused calls stopPolling and flips polling=false for all projects", async () => { + // Backend confirms the loop stopped, so the F4 reconciliation in the trailing + // refreshPollStatus agrees with the optimistic flip (default mock would report + // running:true and bounce p1 back, masking the optimistic stop under test). + vi.mocked(api.pollStatus).mockResolvedValue(pollStatus({ running: false })); const store = usePrStore(); expect(store.pollingActive).toBe(true); await store.toggle(); + // Let the fire-and-forget refreshPollStatus(activeId) settle so its reconciliation + // (p1 -> running:false) is reflected before asserting. + await Promise.resolve(); expect(api.stopPolling).toHaveBeenCalledOnce(); expect(store.pollingFor("p1")).toBe(false); expect(store.pollingFor("p2")).toBe(false); expect(store.errorActive).toBeNull(); + // Reflects the start/stop in the active project's backend diagnostics (#66 F8). + expect(api.pollStatus).toHaveBeenCalledWith("p1"); }); it("on a rejected command sets error and does NOT flip polling", async () => { @@ -263,6 +318,8 @@ describe("usePrStore init()", () => { expect(api.getPrs).toHaveBeenCalledWith("p1"); expect(store.prs.p1).toEqual(snapshot); expect(typeof (await unlisten)).toBe("function"); + // Baselines the active project's poll-loop diagnostics (#66 F8). + expect(api.pollStatus).toHaveBeenCalledWith("p1"); }); it("registers the listener BEFORE reading the snapshot (#27 F3 race guard)", async () => { @@ -300,6 +357,8 @@ describe("usePrStore switchTo()", () => { expect(store.hasNewPr.p2).toBe(false); expect(api.getPrs).toHaveBeenCalledWith("p2"); expect(store.prs.p2).toEqual(snapshot); + // Refreshes the switched-to project's poll diagnostics immediately (#66 F5/F8). + expect(api.pollStatus).toHaveBeenCalledWith("p2"); }); }); @@ -366,3 +425,36 @@ describe("usePrStore setArchived()", () => { expect(store.error.p1).toBe("archive failed"); }); }); + +describe("usePrStore refreshPollStatus() (#62, #66 F4/F8)", () => { + it("writes pollStatus and reconciles the optimistic polling flag to backend running", async () => { + // Backend reports the loop STOPPED — the optimistic flag (default true) must + // reconcile to false so the pause/resume button reads "恢复轮询" (#66 F4). + const status = pollStatus({ running: false, lastDiscoveredCount: 3 }); + vi.mocked(api.pollStatus).mockResolvedValueOnce(status); + const store = usePrStore(); + expect(store.pollingFor("p1")).toBe(true); + + await store.refreshPollStatus("p1"); + + expect(api.pollStatus).toHaveBeenCalledWith("p1"); + expect(store.pollStatus.p1).toEqual(status); + expect(store.pollingFor("p1")).toBe(false); + }); + + it("leaves the prior pollStatus AND polling flag unchanged on a rejected fetch (error swallowed)", async () => { + const store = usePrStore(); + // Seed a prior good status + a known optimistic flag. + const prior = pollStatus({ running: true, lastDiscoveredCount: 1 }); + store.pollStatus.p1 = prior; + store.polling.p1 = true; + + vi.mocked(api.pollStatus).mockRejectedValueOnce({ message: "status failed" }); + await store.refreshPollStatus("p1"); + + // Rejected fetch: no error banner, no mutation of either field. + expect(store.pollStatus.p1).toEqual(prior); + expect(store.polling.p1).toBe(true); + expect(store.error.p1).toBeUndefined(); + }); +}); diff --git a/src/pr/usePrStore.ts b/src/pr/usePrStore.ts index d948740..aecbfb0 100644 --- a/src/pr/usePrStore.ts +++ b/src/pr/usePrStore.ts @@ -204,6 +204,10 @@ export const usePrStore = defineStore("pr", { await useProjects().setActive(id); this.hasNewPr[id] = false; await this.loadSnapshot(id); + // Refresh the switched-to project's poll diagnostics now (#66 F5): otherwise + // PollControls shows stale/empty status until the 10s backstop timer or the + // next prs:updated event. Fire-and-forget — refreshPollStatus swallows errors. + void this.refreshPollStatus(id); }, async pollNow(projectId: string) { if (this.loading[projectId]) return; @@ -264,9 +268,16 @@ export const usePrStore = defineStore("pr", { // Read one project's backend poll-loop status into its partition (#62). Pure // diagnostics: a rejected command leaves the prior snapshot as-is (no error // banner, no list mutation) so a transient fetch failure can't blank the readout. + // On a SUCCESSFUL fetch, reconcile the optimistic `polling` flag to the backend + // truth (#66 F4): the pause/resume button reads `pollingActive` (optimistic), so + // if the backend loop is actually stopped the button must say "恢复轮询" — letting + // the first click resume rather than mistakenly stop. The catch leaves both + // `pollStatus` and `polling` unchanged so a transient failure can't flip the flag. async refreshPollStatus(id: string) { try { - this.pollStatus[id] = await fetchPollStatus(id); + const status = await fetchPollStatus(id); + this.pollStatus[id] = status; + this.polling[id] = status.running; } catch { /* leave as-is */ }