diff --git a/src-tauri/src/config/commands.rs b/src-tauri/src/config/commands.rs index 66f11c6..91b7dd5 100644 --- a/src-tauri/src/config/commands.rs +++ b/src-tauri/src/config/commands.rs @@ -21,3 +21,14 @@ pub fn get_config(app: tauri::AppHandle) -> AppResult(app: tauri::AppHandle, config: AppConfig) -> AppResult<()> { service::save(&app, config) } + +/// Persists the active project selection (#35) without re-validating the whole +/// config — see [`service::set_active_project`]. The frontend calls this on every +/// project switch so the last-viewed project survives a restart. +#[tauri::command] +pub fn set_active_project( + app: tauri::AppHandle, + project_id: String, +) -> AppResult<()> { + service::set_active_project(&app, &project_id) +} diff --git a/src-tauri/src/config/model.rs b/src-tauri/src/config/model.rs index 15be9b6..796334c 100644 --- a/src-tauri/src/config/model.rs +++ b/src-tauri/src/config/model.rs @@ -7,16 +7,28 @@ use serde::{Deserialize, Serialize}; use crate::error::{AppError, AppResult}; use crate::model::{EngineKind, SourceKind, WebhookTunnelMode}; -/// Persisted application configuration. Defaults target the gocell repo the -/// app is built to serve. +/// One monitored project (#35). What was previously the flat per-repo subset of +/// [`AppConfig`] is now a list element: each project carries its own repo, paths, +/// labels, source/engine kind, and auto-review toggle, so the app can poll and +/// review several repos in parallel. Global webhook/shell settings stay on +/// [`AppConfig`] (one receiver serves all projects). /// -/// `#[serde(default)]` makes deserialization forward-compatible: a persisted -/// config missing fields (older versions, or before a #11 field is added) fills -/// absent fields from [`Default`] instead of failing. Do not add -/// `#[serde(deny_unknown_fields)]` — it would break that forward-compat. +/// `#[serde(default)]` mirrors [`AppConfig`]'s forward-compat contract: a project +/// object missing fields fills them from [`Default`]. Field defaults match the +/// historical single-project [`AppConfig`] defaults (the gocell repo the app was +/// built to serve) so a migrated config keeps identical behavior. #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(rename_all = "camelCase", default)] -pub struct AppConfig { +pub struct Project { + /// Stable identifier (the migration assigns `"default"` to the lifted + /// single-project config; new projects get a fresh id). Used as the routing + /// key on every PR/review event and by [`super::service::project`] lookup. + pub id: String, + /// Human-readable label shown in the project switcher. + pub name: String, + /// Whether this project is polled/reviewed. Disabled projects are skipped by + /// `validate` (their fields are not checked) and by the scheduler. + pub enabled: bool, /// Monitored repo, `owner/name`. pub repo: String, /// Absolute path to the local clone codex runs the pr-review skill against. @@ -41,6 +53,49 @@ pub struct AppConfig { pub engine_kind: EngineKind, /// 是否在发现 dispatchable PR 时自动派发 review（false=仅手动「开始 review」触发）。 pub auto_review: bool, +} + +impl Default for Project { + fn default() -> Self { + Self { + id: String::new(), + name: String::new(), + enabled: true, + repo: "ghbvf/gocell".to_string(), + repo_root: String::new(), + poll_interval_secs: 120, + authors: Vec::new(), + review_label: "pr-status/needs-review-again".to_string(), + check_label: "pr-status/needs-check-fix".to_string(), + skill_rel_path: ".codex/skills/pr-review/SKILL.md".to_string(), + pr_cooldown_seconds: 1800, + source_kind: SourceKind::default(), + engine_kind: EngineKind::default(), + // Boot defaults to manual review: scheduler polls/emits but does NOT + // auto-dispatch codex at startup (avoids clashing with other review + // processes). Flip-back guarded by `default_auto_review_is_off`. + auto_review: false, + } + } +} + +/// Persisted application configuration (#35: multi-project). Holds the list of +/// monitored [`Project`]s plus the GLOBAL webhook/shell settings (one webhook +/// receiver serves every project). +/// +/// `#[serde(default)]` makes deserialization forward-compatible: a persisted +/// config missing fields (older versions, or before a #11 field is added) fills +/// absent fields from [`Default`] instead of failing. Do not add +/// `#[serde(deny_unknown_fields)]` — it would break that forward-compat. The +/// legacy flat single-project shape is upgraded by `super::service::migrate_value` +/// before deserialization, so old stored configs still load. +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(rename_all = "camelCase", default)] +pub struct AppConfig { + /// Monitored projects. Empty on first launch (onboarding then adds the first). + pub projects: Vec, + /// `id` of the project the UI currently focuses. Empty when `projects` is empty. + pub active_project_id: String, /// 是否启用 webhook 接收端（#9）。开关本身只 gate「能否启动」本地接收端 + Cloudflare /// 隧道（手动 `start_webhook` 命令）——不自动起、不影响轮询路径。 pub webhook_enabled: bool, @@ -68,20 +123,8 @@ pub struct AppConfig { impl Default for AppConfig { fn default() -> Self { Self { - repo: "ghbvf/gocell".to_string(), - repo_root: String::new(), - poll_interval_secs: 120, - authors: Vec::new(), - review_label: "pr-status/needs-review-again".to_string(), - check_label: "pr-status/needs-check-fix".to_string(), - skill_rel_path: ".codex/skills/pr-review/SKILL.md".to_string(), - pr_cooldown_seconds: 1800, - source_kind: SourceKind::default(), - engine_kind: EngineKind::default(), - // Boot defaults to manual review: scheduler polls/emits but does NOT - // auto-dispatch codex at startup (avoids clashing with other review - // processes). Flip-back guarded by `default_auto_review_is_off`. - auto_review: false, + projects: Vec::new(), + active_project_id: String::new(), webhook_enabled: false, webhook_port: 8787, webhook_secret: String::new(), @@ -93,7 +136,12 @@ impl Default for AppConfig { } } -/// Validates config fields before persisting (hard-reject on failure). +/// Minimum `webhook_secret` length (trimmed chars) when the receiver is enabled. The +/// secret is the SOLE gate on a public HMAC-SHA256 endpoint, so a 1–2 char value is +/// brute-forceable; require a floor (GitHub recommends a long random secret). +const WEBHOOK_SECRET_MIN_LEN: usize = 16; + +/// Validates one [`Project`]'s fields (hard-reject on failure). /// /// Checks: `repo` is `owner/name` (one slash, both sides non-empty, no /// whitespace — the backend boundary `gh pr list --repo` consumes, mirroring the @@ -114,40 +162,35 @@ impl Default for AppConfig { /// (src/config/fields.ts) keys on — locked at this end by the `validate_error_*` /// test below (PR #41 F4, Medium). Checks run in wizard-step order so the first /// failure routes to the earliest offending step. -/// Minimum `webhook_secret` length (trimmed chars) when the receiver is enabled. The -/// secret is the SOLE gate on a public HMAC-SHA256 endpoint, so a 1–2 char value is -/// brute-forceable; require a floor (GitHub recommends a long random secret). -const WEBHOOK_SECRET_MIN_LEN: usize = 16; - -pub fn validate(config: &AppConfig) -> AppResult<()> { +pub fn validate_project(project: &Project) -> AppResult<()> { // owner/name: exactly one slash, both sides non-empty, no whitespace anywhere // (mirrors the frontend REPO_RE `^[^/\s]+\/[^/\s]+$`). - let repo_parts: Vec<&str> = config.repo.split('/').collect(); + let repo_parts: Vec<&str> = project.repo.split('/').collect(); let repo_ok = repo_parts.len() == 2 && !repo_parts[0].is_empty() && !repo_parts[1].is_empty() - && !config.repo.chars().any(char::is_whitespace); + && !project.repo.chars().any(char::is_whitespace); if !repo_ok { return Err(AppError::new(format!( "repo 必须是 owner/name 格式: {}", - config.repo + project.repo ))); } - let repo_root = config.repo_root.trim(); + let repo_root = project.repo_root.trim(); let root = Path::new(repo_root); if repo_root.is_empty() || !root.is_absolute() || !root.is_dir() { return Err(AppError::new(format!( "repoRoot 必须是存在的绝对目录路径: {}", - config.repo_root + project.repo_root ))); } - let skill_rel = Path::new(&config.skill_rel_path); + let skill_rel = Path::new(&project.skill_rel_path); if skill_rel.is_absolute() { return Err(AppError::new(format!( "skillRelPath 必须是相对路径: {}", - config.skill_rel_path + project.skill_rel_path ))); } let skill = root.join(skill_rel); @@ -166,24 +209,35 @@ pub fn validate(config: &AppConfig) -> AppResult<()> { if !skill_canon.starts_with(&root_canon) { return Err(AppError::new(format!( "skillRelPath 不能逃逸 repoRoot: {}", - config.skill_rel_path + project.skill_rel_path ))); } - if config.poll_interval_secs == 0 { + if project.poll_interval_secs == 0 { return Err(AppError::new("pollIntervalSecs 必须大于 0")); } - if config.pr_cooldown_seconds == 0 { + if project.pr_cooldown_seconds == 0 { return Err(AppError::new("prCooldownSeconds 必须大于 0")); } - if config.review_label.trim().is_empty() { + if project.review_label.trim().is_empty() { return Err(AppError::new("reviewLabel 不能为空")); } - if config.check_label.trim().is_empty() { + if project.check_label.trim().is_empty() { return Err(AppError::new("checkLabel 不能为空")); } + Ok(()) +} + +/// Validates the whole [`AppConfig`] before persisting (hard-reject on failure). +/// +/// Validates the GLOBAL webhook fields (only when the receiver is enabled), then +/// every `enabled` [`Project`] via [`validate_project`], and rejects duplicate +/// project `id`s or `repo`s (each would make event routing / dedup ambiguous). An +/// empty `projects` list (first launch, before onboarding adds one) is VALID — +/// onboarding is the gate that fills it. +pub fn validate(config: &AppConfig) -> AppResult<()> { // Webhook fields are only constrained when the receiver is enabled: a public // endpoint (reached via the cloudflared tunnel) MUST have a secret or any POST // could forge a review trigger; a zero port can't bind. Disabled → unconstrained @@ -218,6 +272,69 @@ pub fn validate(config: &AppConfig) -> AppResult<()> { } } + // Per-project fields: validate each ENABLED project; disabled ones are skipped + // (their fields may be intentionally incomplete). The id/repo of every project + // (enabled or not) is a routing/dedup key, so duplicates are rejected regardless. + let mut seen_ids: std::collections::HashSet<&str> = std::collections::HashSet::new(); + let mut seen_repos: std::collections::HashSet = std::collections::HashSet::new(); + for project in &config.projects { + // id-format lock (Medium carrier): the id becomes a store-key PREFIX + // (`dispatched:{id}` / `events:{id}` in ledger.rs, `tracked:{id}` in + // registry.rs). An id containing `:` would split the partition wrong and an + // empty / whitespace id would alias or corrupt a key — either silently merges + // two projects' dedup/retention partitions → dedup failure → re-review storm. + // Checked for EVERY project (not just enabled): a disabled project's stores + // persist and its id re-enters the key space the moment it is re-enabled. The + // Hard path (future) is a `ProjectId` newtype whose typed constructor rejects + // these at the type level, making the bad shape unexpressible. + if project.id.is_empty() + || project.id.contains(':') + || project.id.chars().any(char::is_whitespace) + { + return Err(AppError::new(format!( + "projectId 非法（不能为空、含 `:` 或空白字符）: {:?}", + project.id + ))); + } + if !seen_ids.insert(project.id.as_str()) { + return Err(AppError::new(format!( + "项目 id 重复: {}（每个项目的 id 必须唯一）", + project.id + ))); + } + // GitHub repo names are case-INSENSITIVE (`Owner/Repo` and `owner/repo` are the + // same repository), and the webhook router matches them with + // `eq_ignore_ascii_case` — so dedup must normalize too, or two case variants + // would each register a project for the SAME repo (duplicate PR rows, double + // dispatch). Normalize to lowercase before the uniqueness check. + if !seen_repos.insert(project.repo.to_ascii_lowercase()) { + return Err(AppError::new(format!( + "项目 repo 重复: {}（同一仓库不能监控两次，大小写不敏感）", + project.repo + ))); + } + if project.enabled { + validate_project(project)?; + } + } + + // When there ARE projects, `active_project_id` must point at one of them. A dangling + // active id (e.g. the active project was deleted but the pointer wasn't updated) would + // leave the UI `hydrate`d on a non-existent project and make `active_repo_root` silently + // fall back to "" (codex handshake cwd lost). An empty `projects` keeps the first-launch + // semantics (active id "" + no projects → onboarding), so only guard the non-empty case. + if !config.projects.is_empty() + && !config + .projects + .iter() + .any(|p| p.id == config.active_project_id) + { + return Err(AppError::new(format!( + "activeProjectId 不指向任何现有项目: {:?}", + config.active_project_id + ))); + } + Ok(()) } @@ -235,9 +352,91 @@ pub fn validate(config: &AppConfig) -> AppResult<()> { mod tests { use super::*; + /// A [`Project`] whose filesystem-dependent fields point at this crate (so + /// `validate_project` passes) — the per-project analogue of the old `valid_base`. + fn valid_project() -> Project { + Project { + id: "default".to_string(), + name: "default".to_string(), + repo_root: env!("CARGO_MANIFEST_DIR").to_string(), + skill_rel_path: "Cargo.toml".to_string(), + ..Project::default() + } + } + + /// A valid [`AppConfig`] containing exactly one valid project — the new analogue + /// of the old `valid_base`. Tests that exercise a bad per-project field mutate + /// `..valid_project()` into the single slot. + fn valid_base() -> AppConfig { + AppConfig { + projects: vec![valid_project()], + active_project_id: "default".to_string(), + ..AppConfig::default() + } + } + + /// Wraps one project as the sole element of an otherwise-default `AppConfig`, + /// so `validate(&with_project(p))` routes through the per-project loop. + fn with_project(project: Project) -> AppConfig { + AppConfig { + projects: vec![project], + active_project_id: "default".to_string(), + ..AppConfig::default() + } + } + #[test] fn app_config_wire_shape_is_camel_case() { let config = AppConfig { + projects: vec![Project::default()], + active_project_id: "default".to_string(), + webhook_enabled: false, + webhook_port: 8787, + webhook_secret: "shh".to_string(), + cloudflared_bin: "cloudflared".to_string(), + webhook_tunnel_mode: WebhookTunnelMode::default(), + webhook_tunnel_command: String::new(), + webhook_public_url: String::new(), + }; + + let v = serde_json::to_value(&config).expect("AppConfig serializes"); + + // Multi-project keys present (camelCase). + assert!(v.get("projects").is_some()); + assert!(v.get("activeProjectId").is_some()); + // Global webhook keys stay at the top level. + assert!(v.get("webhookEnabled").is_some()); + assert!(v.get("webhookPort").is_some()); + assert!(v.get("webhookSecret").is_some()); + assert!(v.get("cloudflaredBin").is_some()); + assert!(v.get("webhookTunnelMode").is_some()); + assert_eq!(v["webhookTunnelMode"], "quick"); + assert!(v.get("webhookTunnelCommand").is_some()); + assert!(v.get("webhookPublicUrl").is_some()); + + // snake_case forms absent — a rename would surface here. + assert!(v.get("active_project_id").is_none()); + assert!(v.get("webhook_enabled").is_none()); + assert!(v.get("webhook_port").is_none()); + assert!(v.get("webhook_secret").is_none()); + assert!(v.get("cloudflared_bin").is_none()); + assert!(v.get("webhook_tunnel_mode").is_none()); + assert!(v.get("webhook_tunnel_command").is_none()); + assert!(v.get("webhook_public_url").is_none()); + + // The per-project fields must NOT have leaked back to the top level (they + // moved into `Project` — a regression that re-flattened them surfaces here). + assert!(v.get("repo").is_none()); + assert!(v.get("repoRoot").is_none()); + assert!(v.get("autoReview").is_none()); + } + + #[test] + fn project_wire_shape_is_camel_case() { + let project = Project { + id: "p1".to_string(), + name: "Project One".to_string(), + enabled: true, repo: "owner/name".to_string(), repo_root: "/path/to/repo".to_string(), poll_interval_secs: 120, @@ -249,18 +448,14 @@ mod tests { source_kind: SourceKind::default(), engine_kind: EngineKind::default(), auto_review: false, - webhook_enabled: false, - webhook_port: 8787, - webhook_secret: "shh".to_string(), - cloudflared_bin: "cloudflared".to_string(), - webhook_tunnel_mode: WebhookTunnelMode::default(), - webhook_tunnel_command: String::new(), - webhook_public_url: String::new(), }; - let v = serde_json::to_value(&config).expect("AppConfig serializes"); + let v = serde_json::to_value(&project).expect("Project serializes"); // camelCase keys present. + assert!(v.get("id").is_some()); + assert!(v.get("name").is_some()); + assert!(v.get("enabled").is_some()); assert!(v.get("repo").is_some()); assert!(v.get("repoRoot").is_some()); assert!(v.get("pollIntervalSecs").is_some()); @@ -274,14 +469,6 @@ mod tests { assert!(v.get("engineKind").is_some()); assert_eq!(v["engineKind"], "codex"); assert!(v.get("autoReview").is_some()); - assert!(v.get("webhookEnabled").is_some()); - assert!(v.get("webhookPort").is_some()); - assert!(v.get("webhookSecret").is_some()); - assert!(v.get("cloudflaredBin").is_some()); - assert!(v.get("webhookTunnelMode").is_some()); - assert_eq!(v["webhookTunnelMode"], "quick"); - assert!(v.get("webhookTunnelCommand").is_some()); - assert!(v.get("webhookPublicUrl").is_some()); // snake_case forms absent — a rename would surface here. assert!(v.get("repo_root").is_none()); @@ -293,138 +480,241 @@ mod tests { assert!(v.get("source_kind").is_none()); assert!(v.get("engine_kind").is_none()); assert!(v.get("auto_review").is_none()); - assert!(v.get("webhook_enabled").is_none()); - assert!(v.get("webhook_port").is_none()); - assert!(v.get("webhook_secret").is_none()); - assert!(v.get("cloudflared_bin").is_none()); - assert!(v.get("webhook_tunnel_mode").is_none()); - assert!(v.get("webhook_tunnel_command").is_none()); - assert!(v.get("webhook_public_url").is_none()); } /// First-launch marker lock (Medium). The frontend routes a fresh install into - /// onboarding by detecting `config.repoRoot === ""` (src/App.vue) — that empty - /// default is the contract. If a future change gave `repo_root` a non-empty - /// default, the frontend would silently skip onboarding and the poll-loop gate - /// in lib.rs (`load_validated`) would change behavior; this assertion fails first - /// so the coupling is machine-checked rather than comment-only. + /// onboarding by detecting that no project exists yet (`config.projects` empty) + /// — that empty default is the contract. If a future change gave `AppConfig` a + /// pre-populated project, the frontend would silently skip onboarding and the + /// poll-loop gate would change behavior; this assertion fails first so the + /// coupling is machine-checked rather than comment-only. #[test] - fn default_repo_root_is_empty_first_launch_marker() { - assert_eq!(AppConfig::default().repo_root, ""); + fn default_has_no_projects() { + assert!(AppConfig::default().projects.is_empty()); + assert_eq!(AppConfig::default().active_project_id, ""); } - /// Default-manual-review lock (Medium). A fresh/reset config must NOT - /// auto-dispatch codex review at boot: `auto_review` defaults off, so the - /// scheduler only polls/emits PRs and codex is left to the explicit triggers + /// Default-manual-review lock (Medium). A fresh project must NOT auto-dispatch + /// codex review at boot: `Project::auto_review` defaults off, so the scheduler + /// only polls/emits PRs and codex is left to the explicit triggers /// (`start_review` / `start_codex`). Locking the default here makes that intent /// machine-checked — a silent flip back to `true` would reintroduce the /// boot-time review-process clash this guards against, and fails CI first. #[test] fn default_auto_review_is_off() { - assert!(!AppConfig::default().auto_review); + assert!(!Project::default().auto_review); + } + + #[test] + fn validate_accepts_empty_projects_first_launch() { + // No project yet (onboarding not run) — valid; onboarding is the gate. + assert!(validate(&AppConfig::default()).is_ok()); } #[test] fn validate_accepts_existing_repo_root_and_skill() { + assert!(validate(&valid_base()).is_ok()); + // The per-project validator agrees directly. + assert!(validate_project(&valid_project()).is_ok()); + } + + #[test] + fn validate_skips_disabled_projects() { + // A disabled project with a bogus repo_root must NOT fail validation — its + // fields are not checked (only enabled projects are). + let disabled = Project { + enabled: false, + repo_root: "/no/such/dir/xyz".to_string(), + skill_rel_path: "definitely_missing.md".to_string(), + ..valid_project() + }; + assert!(validate(&with_project(disabled)).is_ok()); + } + + #[test] + fn validate_rejects_duplicate_project_ids() { let config = AppConfig { - repo_root: env!("CARGO_MANIFEST_DIR").to_string(), - skill_rel_path: "Cargo.toml".to_string(), + projects: vec![ + Project { + repo: "owner/a".to_string(), + ..valid_project() + }, + Project { + repo: "owner/b".to_string(), + ..valid_project() + }, + ], + active_project_id: "default".to_string(), ..AppConfig::default() }; - assert!(validate(&config).is_ok()); + assert!(validate(&config).is_err()); } #[test] - fn validate_rejects_empty_repo_root() { + fn validate_rejects_duplicate_project_repos() { let config = AppConfig { - repo_root: String::new(), - skill_rel_path: "Cargo.toml".to_string(), + projects: vec![ + Project { + id: "a".to_string(), + ..valid_project() + }, + Project { + id: "b".to_string(), + ..valid_project() + }, + ], + active_project_id: "a".to_string(), ..AppConfig::default() }; + // Both share the default `repo` (ghbvf/gocell) → reject. assert!(validate(&config).is_err()); } #[test] - fn validate_rejects_missing_repo_root() { + fn validate_rejects_duplicate_project_repos_case_insensitive() { + // GitHub repo names are case-insensitive and the webhook router matches with + // `eq_ignore_ascii_case`, so `Owner/Repo` and `owner/repo` are the SAME repo → + // monitoring both must be rejected (else duplicate rows + double dispatch). let config = AppConfig { - repo_root: "/no/such/dir/xyz".to_string(), - skill_rel_path: "Cargo.toml".to_string(), + projects: vec![ + Project { + id: "a".to_string(), + repo: "Owner/Repo".to_string(), + ..valid_project() + }, + Project { + id: "b".to_string(), + repo: "owner/repo".to_string(), + ..valid_project() + }, + ], + active_project_id: "a".to_string(), ..AppConfig::default() }; assert!(validate(&config).is_err()); } #[test] - fn validate_rejects_missing_skill() { + fn validate_rejects_dangling_active_project_id() { + // Non-empty projects but `active_project_id` matches none → reject (a dangling + // pointer would strand the UI / lose the codex handshake cwd). let config = AppConfig { - repo_root: env!("CARGO_MANIFEST_DIR").to_string(), - skill_rel_path: "definitely_missing.md".to_string(), - ..AppConfig::default() + active_project_id: "ghost".to_string(), + ..valid_base() }; assert!(validate(&config).is_err()); + + // The pointer matching an existing project is accepted. + assert!(validate(&valid_base()).is_ok()); + + // Empty projects keeps first-launch semantics: active id "" + no projects is OK. + assert!(validate(&AppConfig::default()).is_ok()); + } + + #[test] + fn validate_rejects_project_id_with_colon_or_whitespace() { + // The id becomes a store-key prefix (`dispatched:{id}` / `events:{id}` / + // `tracked:{id}`); a `:`, whitespace, or empty id corrupts that partition → + // dedup failure → re-review storm. Rejected for EVERY project (even disabled), + // since a disabled project's id re-enters the key space when re-enabled. + for bad in ["", "a:b", "has space", "tab\tid", "\n"] { + assert!( + validate(&with_project(Project { + id: bad.to_string(), + ..valid_project() + })) + .is_err(), + "expected id {bad:?} to be rejected" + ); + } + // A disabled project with a bad id is STILL rejected (its store key persists). + assert!(validate(&with_project(Project { + id: "a:b".to_string(), + enabled: false, + ..valid_project() + })) + .is_err()); + // A clean id (no `:`, no whitespace, non-empty) is accepted. + assert!(validate(&with_project(Project { + id: "default".to_string(), + ..valid_project() + })) + .is_ok()); + } + + #[test] + fn validate_rejects_empty_repo_root() { + assert!(validate(&with_project(Project { + repo_root: String::new(), + ..valid_project() + })) + .is_err()); + } + + #[test] + fn validate_rejects_missing_repo_root() { + assert!(validate(&with_project(Project { + repo_root: "/no/such/dir/xyz".to_string(), + ..valid_project() + })) + .is_err()); + } + + #[test] + fn validate_rejects_missing_skill() { + assert!(validate(&with_project(Project { + skill_rel_path: "definitely_missing.md".to_string(), + ..valid_project() + })) + .is_err()); } #[test] fn validate_rejects_relative_repo_root() { // `repo_root` must be absolute (doc contract) regardless of CWD. - let config = AppConfig { + assert!(validate(&with_project(Project { repo_root: "src".to_string(), - skill_rel_path: "Cargo.toml".to_string(), - ..AppConfig::default() - }; - assert!(validate(&config).is_err()); + ..valid_project() + })) + .is_err()); } #[test] fn validate_rejects_absolute_skill_rel_path() { // An absolute skill path would let `Path::join` discard `repo_root`. - let config = AppConfig { - repo_root: env!("CARGO_MANIFEST_DIR").to_string(), + assert!(validate(&with_project(Project { skill_rel_path: "/etc/hosts".to_string(), - ..AppConfig::default() - }; - assert!(validate(&config).is_err()); + ..valid_project() + })) + .is_err()); } #[test] fn validate_rejects_skill_escaping_repo_root() { // `repo_root`/src + `../Cargo.toml` resolves to repo_root/Cargo.toml, // which is outside repo_root/src — must be rejected. - let config = AppConfig { + assert!(validate(&with_project(Project { repo_root: format!("{}/src", env!("CARGO_MANIFEST_DIR")), skill_rel_path: "../Cargo.toml".to_string(), - ..AppConfig::default() - }; - assert!(validate(&config).is_err()); + ..valid_project() + })) + .is_err()); } #[test] fn validate_rejects_zero_intervals() { - let base = AppConfig { - repo_root: env!("CARGO_MANIFEST_DIR").to_string(), - skill_rel_path: "Cargo.toml".to_string(), - ..AppConfig::default() - }; - assert!(validate(&AppConfig { + assert!(validate(&with_project(Project { poll_interval_secs: 0, - ..base.clone() - }) + ..valid_project() + })) .is_err()); - assert!(validate(&AppConfig { + assert!(validate(&with_project(Project { pr_cooldown_seconds: 0, - ..base - }) + ..valid_project() + })) .is_err()); } - fn valid_base() -> AppConfig { - AppConfig { - repo_root: env!("CARGO_MANIFEST_DIR").to_string(), - skill_rel_path: "Cargo.toml".to_string(), - ..AppConfig::default() - } - } - #[test] fn validate_rejects_bad_repo() { // owner/name boundary the `gh pr list --repo` call consumes (PR #41 F2). @@ -439,18 +729,18 @@ mod tests { " ghbvf/gocell", ] { assert!( - validate(&AppConfig { + validate(&with_project(Project { repo: bad.to_string(), - ..valid_base() - }) + ..valid_project() + })) .is_err(), "expected {bad:?} to be rejected" ); } - assert!(validate(&AppConfig { + assert!(validate(&with_project(Project { repo: "ghbvf/gocell".to_string(), - ..valid_base() - }) + ..valid_project() + })) .is_ok()); } @@ -459,15 +749,15 @@ mod tests { // Each label feeds `gh pr list --label`; a blank one makes every poll // match nothing / fail (PR #41 F2). for blank in ["", " "] { - assert!(validate(&AppConfig { + assert!(validate(&with_project(Project { review_label: blank.to_string(), - ..valid_base() - }) + ..valid_project() + })) .is_err()); - assert!(validate(&AppConfig { + assert!(validate(&with_project(Project { check_label: blank.to_string(), - ..valid_base() - }) + ..valid_project() + })) .is_err()); } } @@ -577,16 +867,17 @@ mod tests { /// the onboarding step that owns the field by matching the message's leading /// field token — checking `skill` first (the path-escape message names both /// skill and repoRoot) and `repoRoot` before `repo` (since "repoRoot" has - /// "repo" as a prefix). This pins that each `validate()` failure message starts - /// with the token downstream relies on, so a Rust-side wording change that would - /// silently break wizard routing fails CI here. The matching downstream cases - /// live in fields.test.ts; the shared field tokens are the cross-end contract. + /// "repo" as a prefix). This pins that each per-project `validate_project` + /// failure message starts with the token downstream relies on, so a Rust-side + /// wording change that would silently break wizard routing fails CI here. The + /// matching downstream cases live in fields.test.ts; the shared field tokens are + /// the cross-end contract. #[test] fn validate_error_messages_start_with_routing_field_token() { - let base = valid_base(); - let msg = |c: AppConfig| validate(&c).unwrap_err().message; + let base = valid_project(); + let msg = |p: Project| validate_project(&p).unwrap_err().message; - let repo_err = msg(AppConfig { + let repo_err = msg(Project { repo: "not-a-repo".to_string(), ..base.clone() }); @@ -595,32 +886,32 @@ mod tests { // first) would route the repo error to the wrong step. assert!(!repo_err.starts_with("repoRoot"), "{repo_err}"); - assert!(msg(AppConfig { + assert!(msg(Project { repo_root: String::new(), ..base.clone() }) .starts_with("repoRoot")); - assert!(msg(AppConfig { + assert!(msg(Project { skill_rel_path: "/etc/hosts".to_string(), ..base.clone() }) .starts_with("skill")); - assert!(msg(AppConfig { + assert!(msg(Project { poll_interval_secs: 0, ..base.clone() }) .starts_with("pollIntervalSecs")); - assert!(msg(AppConfig { + assert!(msg(Project { pr_cooldown_seconds: 0, ..base.clone() }) .starts_with("prCooldownSeconds")); - assert!(msg(AppConfig { + assert!(msg(Project { review_label: " ".to_string(), ..base.clone() }) .starts_with("reviewLabel")); - assert!(msg(AppConfig { + assert!(msg(Project { check_label: String::new(), ..base }) @@ -633,9 +924,9 @@ mod tests { #[test] fn unknown_fields_are_ignored() { let parsed: AppConfig = - serde_json::from_value(serde_json::json!({"repo": "x/y", "futureField": 42})) + serde_json::from_value(serde_json::json!({"activeProjectId": "x", "futureField": 42})) .expect("unknown fields are ignored"); - assert_eq!(parsed.repo, "x/y"); + assert_eq!(parsed.active_project_id, "x"); } // Forward-compat lock: `#[serde(default)]` lets older/partial persisted @@ -653,10 +944,10 @@ mod tests { #[test] fn partial_object_fills_rest_from_default() { - let parsed: AppConfig = serde_json::from_value(serde_json::json!({"repo": "x/y"})) + let parsed: AppConfig = serde_json::from_value(serde_json::json!({"activeProjectId": "x"})) .expect("partial object deserializes via serde(default)"); let expected = AppConfig { - repo: "x/y".to_string(), + active_project_id: "x".to_string(), ..AppConfig::default() }; assert_eq!( @@ -664,4 +955,21 @@ mod tests { serde_json::to_value(&expected).expect("expected serializes") ); } + + // Forward-compat lock for the new `Project` element: a project object missing + // fields fills them from `Project::default()` (same `#[serde(default)]` + // contract as `AppConfig`). + #[test] + fn project_partial_object_fills_rest_from_default() { + let parsed: Project = serde_json::from_value(serde_json::json!({"id": "p1"})) + .expect("partial project deserializes via serde(default)"); + let expected = Project { + id: "p1".to_string(), + ..Project::default() + }; + assert_eq!( + serde_json::to_value(&parsed).expect("parsed serializes"), + serde_json::to_value(&expected).expect("expected serializes") + ); + } } diff --git a/src-tauri/src/config/service.rs b/src-tauri/src/config/service.rs index d2c86f1..8caeb71 100644 --- a/src-tauri/src/config/service.rs +++ b/src-tauri/src/config/service.rs @@ -4,18 +4,121 @@ //! frontend calls the `get_config` / `set_config` commands (not the store plugin //! directly), so all reads/writes funnel through here. +use serde_json::{json, Map, Value}; use tauri_plugin_store::StoreExt; use super::model::AppConfig; use crate::error::{AppError, AppResult}; +/// Re-export the project domain type THROUGH the config public service surface (#35, +/// F9). The `pr` slice (scheduler / commands) depends on `Project` via +/// `config::service::Project`, not `config::model::Project` — so its cross-slice +/// coupling is to the service (the slice's public API), keeping the model an internal +/// detail the service mediates. The functions below (`project` / `project_validated`) +/// use `Project` through this same re-export. +pub use super::model::Project; + /// Store file holding the persisted config. const STORE_FILE: &str = "config.json"; /// Key under which the [`AppConfig`] value lives in the store. const CONFIG_KEY: &str = "appConfig"; +/// `id`/`name` assigned to the single project lifted out of a legacy flat config by +/// [`migrate_value`] (#35). One source so the migration and its tests agree on the +/// id the active-project pointer (`activeProjectId`) is also set to. +const MIGRATED_PROJECT_ID: &str = "default"; + +/// The 11 per-project keys lifted out of the legacy flat single-project config +/// into the migrated [`Project`] object (#35). camelCase wire names (the persisted +/// shape — `save` writes `serde_json::to_value(&AppConfig)`, which is camelCase). +const PROJECT_KEYS: &[&str] = &[ + "repo", + "repoRoot", + "pollIntervalSecs", + "authors", + "reviewLabel", + "checkLabel", + "skillRelPath", + "prCooldownSeconds", + "sourceKind", + "engineKind", + "autoReview", +]; + +/// The 7 GLOBAL webhook/shell keys that stay at the top level of the migrated +/// [`AppConfig`] (#35: one webhook receiver serves every project). +const WEBHOOK_KEYS: &[&str] = &[ + "webhookEnabled", + "webhookPort", + "webhookSecret", + "cloudflaredBin", + "webhookTunnelMode", + "webhookTunnelCommand", + "webhookPublicUrl", +]; + +/// Migrates a raw persisted config value to the #35 multi-project shape. +/// +/// Pure (no IO) so it is table-testable; the only caller is [`load`], which runs it +/// on the raw stored value before `serde_json::from_value::`. The result +/// is always fed through `from_value` (which is lenient via `#[serde(default)]`), so +/// this only needs to produce the right *shape* — missing keys are filled by +/// `Default` afterward. +/// +/// Detect-by-key (idempotent): a value that already has a `projects` key is the new +/// shape → returned unchanged, so a second pass (or a `save`d config reloaded) is a +/// no-op. Otherwise the legacy flat single-project shape is upgraded: +/// - the 11 [`PROJECT_KEYS`] (whichever exist) are lifted into one project object +/// tagged `id`/`name` = `"default"`, `enabled` = `true`; +/// - the 7 [`WEBHOOK_KEYS`] (whichever exist) stay at the top level; +/// - `projects` = `[thatProject]`, `activeProjectId` = `"default"`. +/// +/// An empty object `{}` (and any non-object) is treated as FIRST LAUNCH — it produces +/// `{ "projects": [], "activeProjectId": "" }` so onboarding triggers, rather than a +/// migrated default project. (A `{}` has no flat keys to lift; materializing a +/// gocell-default project would skip onboarding.) +fn migrate_value(raw: Value) -> Value { + let Value::Object(old) = raw else { + // Non-object (null / array / scalar): treat as first launch. + return json!({ "projects": [], "activeProjectId": "" }); + }; + + // Already new shape → identity (idempotent). + if old.contains_key("projects") { + return Value::Object(old); + } + + // Empty object → first launch (no project, so onboarding triggers). + if old.is_empty() { + return json!({ "projects": [], "activeProjectId": "" }); + } + + // Legacy flat single-project shape: lift the per-project keys into one project. + let mut project = Map::new(); + project.insert("id".to_string(), json!(MIGRATED_PROJECT_ID)); + project.insert("name".to_string(), json!(MIGRATED_PROJECT_ID)); + project.insert("enabled".to_string(), json!(true)); + for key in PROJECT_KEYS { + if let Some(v) = old.get(*key) { + project.insert((*key).to_string(), v.clone()); + } + } + + let mut new = Map::new(); + new.insert("projects".to_string(), json!([Value::Object(project)])); + new.insert("activeProjectId".to_string(), json!(MIGRATED_PROJECT_ID)); + for key in WEBHOOK_KEYS { + if let Some(v) = old.get(*key) { + new.insert((*key).to_string(), v.clone()); + } + } + + Value::Object(new) +} /// Loads the persisted configuration, falling back to [`AppConfig::default`] -/// when nothing has been stored yet. +/// when nothing has been stored yet. The raw stored value is run through +/// [`migrate_value`] first so a legacy flat single-project config (#35) upgrades to +/// the multi-project shape before deserialization. pub fn load(app: &tauri::AppHandle) -> AppResult { let store = app .store(STORE_FILE) @@ -25,11 +128,58 @@ pub fn load(app: &tauri::AppHandle) -> AppResult Ok(AppConfig::default()), // Surface (don't silently discard) a corrupt/incompatible persisted // config so the user can fix it rather than lose their settings. - Some(value) => serde_json::from_value(value) + Some(value) => serde_json::from_value(migrate_value(value)) .map_err(|e| AppError::new(format!("解析持久化配置失败: {e}"))), } } +/// Looks up a [`Project`] by `id` in the persisted config. Callers in other slices +/// (scheduler / review dispatch / webhook routing) resolve the project they act on +/// through here, so the config slice stays the single owner of project lookup. +pub fn project(app: &tauri::AppHandle, id: &str) -> AppResult { + let config = load(app)?; + config + .projects + .into_iter() + .find(|p| p.id == id) + .ok_or_else(|| AppError::new(format!("找不到项目: {id}"))) +} + +/// Looks up a [`Project`] by `id` and validates its filesystem-dependent fields +/// before returning it. The per-project analogue of [`load_validated`]: the review +/// slice calls this right before attaching the project's skill path to a codex turn +/// so a broken/escaped skill path is rejected at dispatch time (the same guarantee +/// the old single-project `load_validated` gave). Validation stays inside the config +/// slice (via [`crate::config::model::validate_project`]) so review depends only on +/// `config::service`, never `config::model`. +pub fn project_validated( + app: &tauri::AppHandle, + id: &str, +) -> AppResult { + let p = project(app, id)?; + crate::config::model::validate_project(&p)?; + Ok(p) +} + +/// Returns the `repo_root` of the active project, or an empty string when there is +/// no active project (first launch, or `active_project_id` matches nothing). +/// +/// Degrades gracefully (returns `Ok("")` rather than an error) so the GLOBAL codex +/// status probe — which only needs *a* repo root to spawn its app-server check — can +/// treat "no active project yet" as "unavailable" instead of surfacing an error. It +/// also does NOT validate the path (mirroring `load`'s leniency for read-only +/// consumers); callers that will actually *use* the path go through +/// [`project_validated`] instead. +pub fn active_repo_root(app: &tauri::AppHandle) -> AppResult { + let config = load(app)?; + Ok(config + .projects + .iter() + .find(|p| p.id == config.active_project_id) + .map(|p| p.repo_root.clone()) + .unwrap_or_default()) +} + /// Loads the persisted config and validates its filesystem-dependent fields, for /// callers that will *use* those paths (e.g. the review slice attaching the skill /// path to a codex turn). Keeps validation inside the config slice so callers @@ -44,12 +194,18 @@ pub fn load_validated(app: &tauri::AppHandle) -> AppResult /// Persists the configuration after validating filesystem-dependent fields. pub fn save(app: &tauri::AppHandle, config: AppConfig) -> AppResult<()> { super::model::validate(&config)?; + persist(app, &config) +} +/// Writes `config` to the store (no validation). Private — the validating [`save`] +/// and the lenient [`set_active_project`] both funnel through here so the +/// store-write plumbing lives in one place. +fn persist(app: &tauri::AppHandle, config: &AppConfig) -> AppResult<()> { let store = app .store(STORE_FILE) .map_err(|e| AppError::new(format!("打开配置存储失败: {e}")))?; - let value = serde_json::to_value(&config).map_err(|e| AppError::new(e.to_string()))?; + let value = serde_json::to_value(config).map_err(|e| AppError::new(e.to_string()))?; // tauri-plugin-store 2.x: `Store::set` is infallible and returns `()`. store.set(CONFIG_KEY, value); store @@ -57,3 +213,167 @@ pub fn save(app: &tauri::AppHandle, config: AppConfig) -> .map_err(|e| AppError::new(format!("写入配置存储失败: {e}")))?; Ok(()) } + +/// Persists `active_project_id` (#35) WITHOUT re-validating the whole config — +/// switching the viewed project is a UI navigation action, not a config edit, and +/// must not be blocked because some OTHER enabled project's filesystem field went +/// stale. Verifies the target project exists (a stale id is rejected), then writes +/// the pointer through the same store as [`save`]. The frontend `useProjects().setActive` +/// calls the `set_active_project` command, which funnels here. +/// +/// **Concurrency (intentional, no lock).** This does a load→mutate→persist with NO +/// `config.json` write lock, so it is last-write-wins against a concurrent [`save`]: +/// a `set_active_project` and a `save` racing could each clobber the other's write. +/// That is acceptable because `active_project_id` is UI navigation state, not a +/// correctness-critical field — the worst case is the focused project momentarily +/// reverts and the next UI action re-sets it. (The dedup/retention stores that ARE +/// correctness-critical have their own write locks; this pointer does not warrant one.) +pub fn set_active_project(app: &tauri::AppHandle, id: &str) -> AppResult<()> { + let mut config = load(app)?; + if !config.projects.iter().any(|p| p.id == id) { + return Err(AppError::new(format!("找不到项目: {id}"))); + } + config.active_project_id = id.to_string(); + persist(app, &config) +} + +/// `migrate_value` correctness lock (#35). The migration is the bridge between the +/// legacy flat single-project persisted shape and the multi-project [`AppConfig`]; +/// it is pure (no IO) so it can be characterized directly here. These tests pin the +/// four cases — legacy flat → single default project, already-new identity, webhook +/// lift, first-launch empty — plus idempotence; a shape regression fails CI here. +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn migrate_old_flat_produces_single_default_project() { + let raw = json!({ + "repo": "octocat/hello", + "repoRoot": "/tmp/hello", + "pollIntervalSecs": 60, + "authors": ["octocat"], + "reviewLabel": "needs-review", + "checkLabel": "needs-check", + "skillRelPath": ".codex/skills/pr-review/SKILL.md", + "prCooldownSeconds": 900, + "sourceKind": "github", + "engineKind": "codex", + "autoReview": true + }); + + let migrated = migrate_value(raw); + + // Top-level multi-project shape. + assert_eq!(migrated["activeProjectId"], MIGRATED_PROJECT_ID); + let projects = migrated["projects"].as_array().expect("projects array"); + assert_eq!(projects.len(), 1); + + let project = &projects[0]; + assert_eq!(project["id"], MIGRATED_PROJECT_ID); + assert_eq!(project["name"], MIGRATED_PROJECT_ID); + assert_eq!(project["enabled"], true); + // The 11 lifted per-project keys carried over verbatim. + assert_eq!(project["repo"], "octocat/hello"); + assert_eq!(project["repoRoot"], "/tmp/hello"); + assert_eq!(project["pollIntervalSecs"], 60); + assert_eq!(project["authors"], json!(["octocat"])); + assert_eq!(project["reviewLabel"], "needs-review"); + assert_eq!(project["checkLabel"], "needs-check"); + assert_eq!(project["skillRelPath"], ".codex/skills/pr-review/SKILL.md"); + assert_eq!(project["prCooldownSeconds"], 900); + assert_eq!(project["sourceKind"], "github"); + assert_eq!(project["engineKind"], "codex"); + assert_eq!(project["autoReview"], true); + + // The migrated shape must deserialize into a real `AppConfig` (lenient path + // `load` uses) with the lifted values intact. + let config: AppConfig = + serde_json::from_value(migrated).expect("migrated shape deserializes"); + assert_eq!(config.active_project_id, MIGRATED_PROJECT_ID); + assert_eq!(config.projects.len(), 1); + assert_eq!(config.projects[0].repo, "octocat/hello"); + assert!(config.projects[0].auto_review); + } + + #[test] + fn migrate_new_shape_is_identity() { + // A value already carrying `projects` is the new shape → returned unchanged. + let raw = json!({ + "projects": [{ "id": "p1", "repo": "owner/name" }], + "activeProjectId": "p1", + "webhookEnabled": false + }); + assert_eq!(migrate_value(raw.clone()), raw); + } + + #[test] + fn migrate_lifts_webhook_to_top_level() { + let raw = json!({ + "repo": "octocat/hello", + "repoRoot": "/tmp/hello", + "webhookEnabled": true, + "webhookPort": 9000, + "webhookSecret": "super-secret-0123456789", + "cloudflaredBin": "/usr/bin/cloudflared", + "webhookTunnelMode": "command", + "webhookTunnelCommand": "cloudflared tunnel run --url http://127.0.0.1:{port} t", + "webhookPublicUrl": "https://example.com" + }); + + let migrated = migrate_value(raw); + + // Webhook keys at the TOP level (not inside the project). + assert_eq!(migrated["webhookEnabled"], true); + assert_eq!(migrated["webhookPort"], 9000); + assert_eq!(migrated["webhookSecret"], "super-secret-0123456789"); + assert_eq!(migrated["cloudflaredBin"], "/usr/bin/cloudflared"); + assert_eq!(migrated["webhookTunnelMode"], "command"); + assert_eq!( + migrated["webhookTunnelCommand"], + "cloudflared tunnel run --url http://127.0.0.1:{port} t" + ); + assert_eq!(migrated["webhookPublicUrl"], "https://example.com"); + + // And NOT duplicated into the project object. + let project = &migrated["projects"][0]; + assert!(project.get("webhookEnabled").is_none()); + assert!(project.get("webhookSecret").is_none()); + + // Round-trips into a real `AppConfig` with webhook fields at the top level. + let config: AppConfig = + serde_json::from_value(migrated).expect("migrated shape deserializes"); + assert!(config.webhook_enabled); + assert_eq!(config.webhook_port, 9000); + assert_eq!(config.webhook_public_url, "https://example.com"); + } + + #[test] + fn migrate_empty_is_first_launch_empty_projects() { + // `{}` → first launch: no project (so onboarding triggers), not a migrated + // gocell-default project. + let migrated = migrate_value(json!({})); + assert_eq!(migrated["projects"], json!([])); + assert_eq!(migrated["activeProjectId"], ""); + + let config: AppConfig = + serde_json::from_value(migrated).expect("first-launch shape deserializes"); + assert!(config.projects.is_empty()); + assert_eq!(config.active_project_id, ""); + } + + #[test] + fn migrate_is_idempotent() { + // Migrating a legacy flat config, then migrating the RESULT, is a no-op on + // the second pass (the result already has a `projects` key). This is what + // makes `save` (always new shape) → next `load` a stable fixed point. + let raw = json!({ + "repo": "octocat/hello", + "repoRoot": "/tmp/hello", + "autoReview": false + }); + let once = migrate_value(raw); + let twice = migrate_value(once.clone()); + assert_eq!(once, twice); + } +} diff --git a/src-tauri/src/events.rs b/src-tauri/src/events.rs index 6455c34..7b3c960 100644 --- a/src-tauri/src/events.rs +++ b/src-tauri/src/events.rs @@ -25,12 +25,17 @@ pub const REVIEW_EVENT: &str = "review:event"; pub enum PrEvent { /// The retained tracked-PR list (the persisted-retention view, not a raw /// per-round discovery — a transient miss flips presence rather than dropping - /// a row). + /// a row). `project_id` is the routing key (#35): the frontend keys the PR list + /// it updates by which project this refresh belongs to. #[serde(rename_all = "camelCase")] - Updated { prs: Vec }, - /// A discovery cycle failed; the loop keeps running. + Updated { + project_id: String, + prs: Vec, + }, + /// A discovery cycle failed; the loop keeps running. `project_id` scopes the + /// error to the offending project (#35). #[serde(rename_all = "camelCase")] - Error { message: String }, + Error { project_id: String, message: String }, } /// A single streamed unit of a review session, forwarded to the frontend. @@ -42,9 +47,11 @@ pub enum PrEvent { #[derive(Debug, Clone, Serialize)] #[serde(rename_all = "camelCase", tag = "kind")] pub enum ReviewEvent { - /// Incremental assistant message text. + /// Incremental assistant message text. `project_id` is the routing key (#35): + /// the frontend attributes the streamed delta to the owning project's session. #[serde(rename_all = "camelCase")] MessageDelta { + project_id: String, thread_id: String, item_id: String, text: String, @@ -52,24 +59,35 @@ pub enum ReviewEvent { /// Incremental reasoning text. #[serde(rename_all = "camelCase")] ReasoningDelta { + project_id: String, thread_id: String, item_id: String, text: String, }, /// The review turn ended (`completed` / `interrupted` / `failed`). #[serde(rename_all = "camelCase")] - TurnCompleted { thread_id: String, status: String }, + TurnCompleted { + project_id: String, + thread_id: String, + status: String, + }, /// A session-level error. #[serde(rename_all = "camelCase")] - Error { thread_id: String, message: String }, + Error { + project_id: String, + thread_id: String, + message: String, + }, /// An auto-trigger dispatch-level notice NOT tied to any one session — config /// invalid, one/more `start_review` failures, or a ledger-write failure during - /// `crate::dispatch::auto_dispatch`. Carries no `threadId`; the frontend - /// surfaces it as an app-level "auto review" notice (the availability banner), - /// not a session stream event. `message` is single-word so no per-variant - /// `rename_all` is needed (the container tag rename still maps the variant name - /// to the camelCase `"dispatchError"`). - DispatchError { message: String }, + /// `crate::dispatch::auto_dispatch`. Carries no `threadId` (session-less), but + /// DOES carry `project_id` (#35) so the frontend can scope the app-level "auto + /// review" notice to the offending project. Now that it has >1 field, it needs + /// its own `#[serde(rename_all = "camelCase")]` so `projectId` serializes + /// camelCase (the container tag rename only maps the variant name to the + /// camelCase `"dispatchError"` — it does not propagate to field keys). + #[serde(rename_all = "camelCase")] + DispatchError { project_id: String, message: String }, } /// Serde wire-shape lock for the `ReviewEvent` discriminated union. @@ -111,12 +129,16 @@ mod tests { #[test] fn pr_updated_wire_shape_is_camel_case() { let event = PrEvent::Updated { + project_id: "p1".to_string(), prs: vec![sample_view()], }; let v = serde_json::to_value(&event).expect("PrEvent serializes"); assert_eq!(v["kind"], "updated"); + // `projectId` routing key present (camelCase); snake_case absent (#35). + assert!(v.get("projectId").is_some()); + assert!(v.get("project_id").is_none()); assert!(v.get("prs").is_some()); // The row carries the flattened `PullRequestView` keys plus the retention // fields — a drift in `TrackedPrView`'s wire shape surfaces here too. @@ -136,12 +158,15 @@ mod tests { #[test] fn pr_error_wire_shape_is_camel_case() { let event = PrEvent::Error { + project_id: "p1".to_string(), message: "boom".to_string(), }; let v = serde_json::to_value(&event).expect("PrEvent serializes"); assert_eq!(v["kind"], "error"); + assert!(v.get("projectId").is_some()); + assert!(v.get("project_id").is_none()); assert!(v.get("message").is_some()); } @@ -160,6 +185,7 @@ mod tests { #[test] fn message_delta_wire_shape_is_camel_case() { let event = ReviewEvent::MessageDelta { + project_id: "p1".to_string(), thread_id: "t1".to_string(), item_id: "i1".to_string(), text: "hello".to_string(), @@ -171,11 +197,13 @@ mod tests { assert_eq!(v["kind"], "messageDelta"); // camelCase field keys present. + assert!(v.get("projectId").is_some()); assert!(v.get("threadId").is_some()); assert!(v.get("itemId").is_some()); assert!(v.get("text").is_some()); // snake_case forms absent — a rename would surface here. + assert!(v.get("project_id").is_none()); assert!(v.get("thread_id").is_none()); assert!(v.get("item_id").is_none()); } @@ -183,15 +211,18 @@ mod tests { #[test] fn reasoning_delta_wire_shape_is_camel_case() { let event = ReviewEvent::ReasoningDelta { + project_id: "p1".to_string(), thread_id: "t1".to_string(), item_id: "i1".to_string(), text: "why".to_string(), }; let v = serde_json::to_value(&event).expect("ReviewEvent serializes"); assert_eq!(v["kind"], "reasoningDelta"); + assert!(v.get("projectId").is_some()); assert!(v.get("threadId").is_some()); assert!(v.get("itemId").is_some()); assert!(v.get("text").is_some()); + assert!(v.get("project_id").is_none()); assert!(v.get("thread_id").is_none()); assert!(v.get("item_id").is_none()); } @@ -199,24 +230,30 @@ mod tests { #[test] fn error_event_wire_shape_is_camel_case() { let event = ReviewEvent::Error { + project_id: "p1".to_string(), thread_id: "t1".to_string(), message: "boom".to_string(), }; let v = serde_json::to_value(&event).expect("ReviewEvent serializes"); assert_eq!(v["kind"], "error"); + assert!(v.get("projectId").is_some()); assert!(v.get("threadId").is_some()); assert!(v.get("message").is_some()); + assert!(v.get("project_id").is_none()); assert!(v.get("thread_id").is_none()); } #[test] fn dispatch_error_wire_shape_is_camel_case_and_session_less() { let event = ReviewEvent::DispatchError { + project_id: "p1".to_string(), message: "boom".to_string(), }; let v = serde_json::to_value(&event).expect("ReviewEvent serializes"); - // Variant tag camelCased by the container rule; carries only `message`. + // Variant tag camelCased by the container rule; carries `projectId` + `message`. assert_eq!(v["kind"], "dispatchError"); + assert!(v.get("projectId").is_some()); + assert!(v.get("project_id").is_none()); assert!(v.get("message").is_some()); // Session-less: no thread id (a rename / accidental field would surface here, // and the `src/types.ts` mirror must stay session-less in lockstep). @@ -227,6 +264,7 @@ mod tests { #[test] fn turn_completed_wire_shape_is_camel_case() { let event = ReviewEvent::TurnCompleted { + project_id: "p1".to_string(), thread_id: "t1".to_string(), status: "completed".to_string(), }; @@ -237,10 +275,12 @@ mod tests { assert_eq!(v["kind"], "turnCompleted"); // camelCase field keys present. + assert!(v.get("projectId").is_some()); assert!(v.get("threadId").is_some()); assert!(v.get("status").is_some()); // snake_case form absent — a rename would surface here. + assert!(v.get("project_id").is_none()); assert!(v.get("thread_id").is_none()); } } diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index a743236..717a442 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -49,9 +49,9 @@ pub fn run() { // [`dispatch::auto_dispatch`] (so adding an engine never edits `dispatch`). state.scheduler.set_dispatcher(Arc::new({ let app = app.handle().clone(); - move |cands| { + move |project_id, cands| { let app = app.clone(); - Box::pin(run_auto_dispatch(app, cands)) + Box::pin(run_auto_dispatch(app, project_id, cands)) } })); // Install the WEBHOOK trigger's dispatch hook (#9). The webhook is a @@ -65,14 +65,17 @@ pub fn run() { // is what lets `pr::webhook` stay runtime-agnostic (never names AppHandle). state.webhook.set_dispatcher(Arc::new({ let app = app.handle().clone(); - move |cands| { + move |project_id: String, cands| { let app = app.clone(); Box::pin(async move { - if !pr::scheduler::auto_review_enabled(&app) { + // 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, cands); - run_auto_dispatch(app, gated).await; + let gated = pr::commands::gate_dispatchable(&app, &project_id, cands); + run_auto_dispatch(app, project_id, gated).await; }) } })); @@ -107,6 +110,7 @@ pub fn run() { review::commands::start_review, review::commands::stop_review, review::commands::list_review_sessions, + config::commands::set_active_project, ]) .build(tauri::generate_context!()) .expect("error while building tauri application") @@ -134,24 +138,27 @@ pub fn run() { /// comment-only (PR #31 finding F1). async fn run_auto_dispatch( app: tauri::AppHandle, + project_id: String, candidates: Vec, ) { if candidates.is_empty() { return; } - // A bad / hand-edited config must not take the poll loop down: skip the batch, - // logged + surfaced to the UI (a desktop user never sees stderr). - let cfg = match config::service::load_validated(&app) { - Ok(cfg) => cfg, + // Resolve + validate THIS project (#35): a bad / hand-edited project config (e.g. an + // escaped skill path) must not take the poll loop down — skip the batch, logged + + // surfaced to the UI scoped to the project (a desktop user never sees stderr). + // `project_validated` re-runs the skill-path validation before codex attaches it. + let project = match config::service::project_validated(&app, &project_id) { + Ok(p) => p, Err(e) => { let msg = format!("配置无效，自动 review 跳过本轮（{}）", e.message); - eprintln!("auto-dispatch 跳过本轮：{msg}"); - emit_dispatch_error(&app, msg); + eprintln!("auto-dispatch 跳过本轮（{project_id}）：{msg}"); + emit_dispatch_error(&app, &project_id, msg); return; } }; - let skill_abs = skill_abs_path(&cfg.repo_root, &cfg.skill_rel_path); + let skill_abs = skill_abs_path(&project.repo_root, &project.skill_rel_path); let state = app.state::(); // Respect an explicit user `stop_codex`: a stopped codex is NOT auto-revived by a // dispatchable PR. Skip this batch silently (same as the autoReview-off skip — no @@ -165,24 +172,35 @@ async fn run_auto_dispatch( codex: &state.codex, registry: &state.sessions, codex_bin: review::commands::CODEX_BIN, - repo: &cfg.repo, - repo_root: &cfg.repo_root, + project_id: &project_id, + repo: &project.repo, + repo_root: &project.repo_root, skill_abs_path: &skill_abs, }; // The review slice owns "what counts as active"; the pr slice owns the ledger. - let active = state.sessions.active_pairs(); - let record = |cands: &[Candidate]| pr::ledger::record_dispatched(&app, cands); - let report = |msg: String| emit_dispatch_error(&app, msg); + // Both are scoped to this project (#35) so a PR number active in one project does + // not gate the same number in another, and dedup writes land in the right partition. + let active = state.sessions.active_pairs(&project_id); + let record = |cands: &[Candidate]| pr::ledger::record_dispatched(&app, &project_id, cands); + let report = |msg: String| emit_dispatch_error(&app, &project_id, msg); dispatch::auto_dispatch(candidates, &engine, &active, &record, &report).await; } /// Emit a session-less [`events::ReviewEvent::DispatchError`] to the review area -/// (the availability banner). Best-effort — a gone window is not an error worth +/// (the availability banner), routed to `project_id` (#35) so the frontend shows it +/// on the right project. Best-effort — a gone window is not an error worth /// propagating from the poll loop. -fn emit_dispatch_error(app: &tauri::AppHandle, message: String) { +fn emit_dispatch_error( + app: &tauri::AppHandle, + project_id: &str, + message: String, +) { let _ = app.emit( events::REVIEW_EVENT, - &events::ReviewEvent::DispatchError { message }, + &events::ReviewEvent::DispatchError { + project_id: project_id.to_string(), + message, + }, ); } diff --git a/src-tauri/src/pr/commands.rs b/src-tauri/src/pr/commands.rs index 4e93072..33b475d 100644 --- a/src-tauri/src/pr/commands.rs +++ b/src-tauri/src/pr/commands.rs @@ -47,33 +47,38 @@ fn build_view( (view, dispatchable) } -/// Discovers the monitored repo's open trigger-labelled PRs now, returning both -/// the annotated views (for the PR list / snapshot) and the dispatchable -/// [`Candidate`]s — the clean rows (`skip_reason` None), which already exclude -/// conflict / draft / cross-repo / disallowed-author / already-dispatched / -/// within-cooldown PRs. Reads config (repo, labels, authors, cooldown) and the -/// dedup ledger; performs two `gh pr list` calls (review + check labels). +/// Discovers `project_id`'s monitored repo's open trigger-labelled PRs now (#35), +/// returning both the annotated views (for the PR list / snapshot) and the +/// dispatchable [`Candidate`]s — the clean rows (`skip_reason` None), which already +/// exclude conflict / draft / cross-repo / disallowed-author / already-dispatched / +/// within-cooldown PRs. Reads THAT project's config (repo, labels, authors, cooldown) +/// and its ledger partition; performs two `gh pr list` calls (review + check labels). /// -/// This is the shared discovery body driven by the scheduler's poll loop -/// (`scheduler::discover_emit_dispatch`), its only caller; there is no -/// manual-fetch command — the frontend triggers a refresh via `poll_now`. The -/// dispatchable candidates flow to the auto-trigger dispatcher ([`crate::dispatch`]). +/// This is the shared discovery body driven by the scheduler's per-project poll loop +/// (`scheduler::discover_emit_dispatch`), its only caller; there is no manual-fetch +/// command — the frontend triggers a refresh via `poll_now`. The dispatchable +/// candidates flow to the auto-trigger dispatcher ([`crate::dispatch`]). pub(crate) async fn discover( app: &tauri::AppHandle, + project_id: &str, ) -> AppResult<(Vec, Vec)> { // Cross-slice read of the config slice's public service (function-level, not - // a type contract — `AppConfig` stays config-private; we snapshot the fields - // the pr slice needs into `MonitorParams`). - let cfg = config_service::load(app)?; + // a type contract — `AppConfig` stays config-private; we resolve THIS project by + // id and snapshot the fields the pr slice needs into `MonitorParams`). + let project = config_service::project(app, project_id)?; let params = MonitorParams { - repo: cfg.repo, - review_label: cfg.review_label, - check_label: cfg.check_label, - authors: cfg.authors, - pr_cooldown_seconds: cfg.pr_cooldown_seconds, + repo: project.repo, + review_label: project.review_label, + check_label: project.check_label, + authors: project.authors, + pr_cooldown_seconds: project.pr_cooldown_seconds, }; - let ledger = Ledger::load(app)?; + // Lock-free ledger read (no `LEDGER_WRITE_LOCK`): a stale-by-one-round snapshot is + // fine here — this gate is an optimization, and the session registry's atomic + // `try_reserve_pair` test-and-set is the real double-dispatch backstop (see + // `Ledger::load`). The write path (`record_dispatched`) is the half that locks. + let ledger = Ledger::load(app, project_id)?; let source = GithubCli::new( params.repo.clone(), params.review_label.clone(), @@ -94,32 +99,41 @@ pub(crate) async fn discover( Ok((views, dispatchable)) } -/// Starts the scheduled-pull loop only when the persisted config validates, -/// returning the validation error (without starting) otherwise. +/// Reconciles the per-project scheduler set to the enabled projects, only when the +/// persisted config validates — returning the validation error (without starting any +/// loop) otherwise. +/// +/// **Multi-project semantics (#35):** this is "start ALL enabled projects". It +/// delegates to [`crate::pr::scheduler::SchedulerSet::reconcile`], which is +/// idempotent — it creates a loop for each newly-enabled project, stops + drops loops +/// for disabled/removed ones, and reconfigures survivors. So a launch, a `start_polling`, +/// and a post-save `reschedule` all funnel through the same reconcile. /// /// The single enforcement point for the "no poll loop under an invalid config" -/// funnel (PR #41 F1). BOTH entry paths go through here, so neither can start the -/// loop on a config that fails [`config_service::load_validated`]: -/// - launch (`lib.rs` setup) discards the `Err` so a first launch (empty -/// `repoRoot` default) routes to onboarding instead of polling a default config; +/// funnel (PR #41 F1). BOTH entry paths go through here, so neither can start any loop +/// on a config that fails [`config_service::load_validated`]: +/// - launch (`lib.rs` setup) discards the `Err` so a first launch (empty `projects` +/// default) routes to onboarding instead of polling; /// - the public `start_polling` command surfaces the `Err` to the frontend. /// -/// Previously the command called `scheduler.start` directly, leaving the funnel's -/// downstream open: a user could start the loop on an invalid hand-edited config, -/// spamming a per-cycle `DispatchError` and running `gh pr list` against a bad repo. +/// `load_validated` returns the validated [`AppConfig`], so `reconcile` reads its +/// `projects` directly (no second load — the validated snapshot is the source). pub(crate) fn start_if_config_valid( app: &tauri::AppHandle, state: &crate::state::AppState, ) -> AppResult<()> { - config_service::load_validated(app)?; - state.scheduler.start(app.clone()); + let cfg = config_service::load_validated(app)?; + state.scheduler.reconcile(app, &cfg.projects); Ok(()) } -/// Starts the scheduled-pull loop. Idempotent (a no-op if already running); errors -/// without starting when the persisted config is invalid (see -/// [`start_if_config_valid`]), so the loop never runs under a bad config — the -/// frontend surfaces the returned error. +/// Starts polling for ALL enabled projects (#35). Idempotent — reconciles the +/// scheduler set to the enabled projects (creates new ones, stops removed/disabled +/// ones, reconfigures survivors). Errors without starting any loop when the persisted +/// config is invalid (see [`start_if_config_valid`]), so no loop runs under a bad +/// config — the frontend surfaces the returned error. Takes no `project_id`: the +/// set always mirrors exactly the enabled projects, so there is no per-project +/// "start" — enabling a project (then saving + reconciling) is how it joins. #[tauri::command] pub async fn start_polling( app: tauri::AppHandle, @@ -128,30 +142,50 @@ pub async fn start_polling( start_if_config_valid(&app, state.inner()) } -/// Stops the scheduled-pull loop (no-op if not running). +/// Stops polling for ALL projects (#35) — `stop_all` tears down every running loop +/// and empties the set (no-op if none running). This is the global pause; a later +/// `start_polling` / `reschedule` rebuilds the enabled loops from scratch. There is +/// deliberately no per-project stop command: disabling a project in config + saving +/// (which reconciles) stops just that one, keeping config the single source of which +/// projects run. #[tauri::command] pub async fn stop_polling(state: tauri::State<'_, crate::state::AppState>) -> AppResult<()> { - state.scheduler.stop(); + state.scheduler.stop_all(); Ok(()) } -/// Triggers an immediate discovery cycle ("立即拉取"). Returns an error when the -/// scheduler is paused (stopped): `wake` is a no-op on a stopped loop and would -/// emit no `prs:updated` event, leaving the frontend stuck in a loading state. -/// Defense-in-depth alongside the disabled-while-paused button. +/// Triggers an immediate discovery cycle for `project_id` ("立即拉取", #35). Returns +/// an error when that project's scheduler is paused/unknown (stopped): `wake` is a +/// no-op on a missing loop and would emit no `prs:updated` event, leaving the +/// frontend stuck in a loading state. Defense-in-depth alongside the +/// disabled-while-paused button. #[tauri::command] -pub async fn poll_now(state: tauri::State<'_, crate::state::AppState>) -> AppResult<()> { - if state.scheduler.wake() { +pub async fn poll_now( + _app: tauri::AppHandle, + state: tauri::State<'_, crate::state::AppState>, + project_id: &str, +) -> AppResult<()> { + if state.scheduler.wake(project_id) { Ok(()) } else { Err(crate::error::AppError::new("轮询已暂停，请先恢复轮询")) } } -/// Re-reads the poll period and rebuilds the loop's ticker (after a config save). +/// Re-reads every project's poll period and reconciles the scheduler set after a +/// config save (#35). Reconcile is idempotent: it adds loops for newly-enabled +/// projects, stops loops for disabled/removed ones, and rebuilds each survivor's +/// ticker (the immediate first tick also re-polls). Reads the persisted config to get +/// the current `projects`; a read failure surfaces to the frontend. NOT validated +/// (unlike `start_polling`): a save already validated, and reconcile only acts on +/// `enabled` projects whose fields the save checked. #[tauri::command] -pub async fn reschedule(state: tauri::State<'_, crate::state::AppState>) -> AppResult<()> { - state.scheduler.reconfigure(); +pub async fn reschedule( + app: tauri::AppHandle, + state: tauri::State<'_, crate::state::AppState>, +) -> AppResult<()> { + let cfg = config_service::load(&app)?; + state.scheduler.reconcile(&app, &cfg.projects); Ok(()) } @@ -161,39 +195,52 @@ pub async fn gh_status() -> AppResult { Ok(gh_auth_status("gh").await) } -/// Returns the retained tracked-PR list (the persisted `prs.json` set projected at -/// the current epoch) so the frontend can render current state on mount without -/// waiting for the next `prs:updated` event (closes the startup lost-event race). -/// Reads only `app` — the persisted set survives restarts, so this no longer -/// depends on the scheduler having run this session. +/// Returns `project_id`'s retained tracked-PR list (#35) — that project's persisted +/// `prs.json` partition projected at the current epoch — so the frontend can render +/// the active project's state on mount without waiting for the next `prs:updated` +/// event (closes the startup lost-event race). Reads only `app` (+ the project id): +/// the persisted set survives restarts, so this no longer depends on the scheduler +/// having run this session. The grace window is resolved from THAT project's period. #[tauri::command] pub fn get_prs( app: tauri::AppHandle, + project_id: &str, ) -> AppResult> { - let tracked = super::registry::TrackedPrs::load(&app)?; - Ok(super::registry::project(&tracked, &app)) + let tracked = super::registry::TrackedPrs::load(&app, project_id)?; + Ok(super::registry::project_snapshot( + &tracked, &app, project_id, + )) } -/// Sets a tracked PR's `archived` flag and re-emits the retained list immediately -/// so the UI reflects the archive/unarchive without waiting for the next poll -/// round. PRs are never auto-evicted; archiving is how users retire inactive rows. +/// Sets a tracked PR's `archived` flag within `project_id`'s set (#35) and re-emits +/// that project's retained list immediately so the UI reflects the archive/unarchive +/// without waiting for the next poll round. PRs are never auto-evicted; archiving is +/// how users retire inactive rows. /// /// Routes through the registry's single serialized write seam ([`registry::mutate_tracked`], -/// F1) so this can't interleave with a poll-cycle upsert and lose a write. An unknown -/// / raced `number` is a benign no-op: `set_archived` reports no change, the closure -/// returns `persist == false` so the seam skips the store write, and we emit nothing — -/// no phantom write/event — returning `Ok` rather than erroring. On a real change we -/// re-emit the retained projection (built inside the seam, under the lock) so the list -/// updates immediately. +/// F1) scoped to `project_id` so this can't interleave with a poll-cycle upsert and lose +/// a write. An unknown / raced `number` is a benign no-op: `set_archived` reports no +/// change, the closure returns `persist == false` so the seam skips the store write, and +/// we emit nothing — no phantom write/event — returning `Ok` rather than erroring. On a +/// real change we re-emit the retained projection (built inside the seam, under the lock, +/// carrying `project_id`) so the list updates immediately. #[tauri::command] pub fn set_pr_archived( app: tauri::AppHandle, + project_id: String, number: u64, archived: bool, ) -> AppResult<()> { - let emitted = super::registry::mutate_tracked(&app, |tracked| { + let emitted = super::registry::mutate_tracked(&app, &project_id, |tracked| { if tracked.set_archived(number, archived) { - (true, Some(super::registry::project(tracked, &app))) + ( + true, + Some(super::registry::project_snapshot( + tracked, + &app, + &project_id, + )), + ) } else { (false, None) // unknown number — nothing changed, skip persist + emit. } @@ -201,28 +248,33 @@ pub fn set_pr_archived( if let Some(list) = emitted { let _ = app.emit( crate::events::PRS_UPDATED_EVENT, - &crate::events::PrEvent::Updated { prs: list }, + &crate::events::PrEvent::Updated { + project_id, + prs: list, + }, ); } Ok(()) } /// Apply the SAME static + cooldown gates the poll path applies (via `build_view`) -/// to webhook-sourced candidates, so a push trigger has dispatch parity with the -/// scheduler: no draft / fork / disallowed-author / within-cooldown PR slips through -/// just because it arrived by webhook. Reads config (authors / cooldown) and the -/// dedup ledger, then filters each candidate through [`discover::should_skip`] and -/// [`discover::cooldown_skip`]. +/// 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`]. /// -/// BOTH reads fail CLOSED: an 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 duplicate" must fail closed, matching the poll -/// path (`discover` uses `Ledger::load(app)?`). +/// 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 +/// 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`); the -/// conflict (both-labels) gate already dropped in `webhook::payload_to_candidate`. +/// 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` / @@ -230,19 +282,20 @@ pub fn set_pr_archived( /// `AppHandle` isn't constructible in a plain test). pub(crate) fn gate_dispatchable( app: &tauri::AppHandle, + project_id: &str, candidates: Vec, ) -> Vec { - let Ok(cfg) = config_service::load(app) else { + let Ok(project) = config_service::project(app, project_id) else { return Vec::new(); }; let params = MonitorParams { - repo: cfg.repo, - review_label: cfg.review_label, - check_label: cfg.check_label, - authors: cfg.authors, - pr_cooldown_seconds: cfg.pr_cooldown_seconds, + repo: project.repo, + review_label: project.review_label, + check_label: project.check_label, + authors: project.authors, + pr_cooldown_seconds: project.pr_cooldown_seconds, }; - let Ok(ledger) = Ledger::load(app) else { + let Ok(ledger) = Ledger::load(app, project_id) else { return Vec::new(); }; gate_candidates(candidates, ¶ms, &ledger, now_epoch()) @@ -273,6 +326,14 @@ fn gate_candidates( /// `*.trycloudflare.com` URL to paste into GitHub). The local server binds /// `127.0.0.1` only — the public path is the tunnel. /// +/// **Multi-project (#35):** ONE receiver serves every project. The handler routes each +/// incoming event by repo, so this builds a [`super::webhook::ProjectRoute`] from every +/// `enabled` project (its repo + trigger labels + id) and hands the snapshot to +/// [`super::webhook::WebhookManager::start`]. The receiver params (port / secret / +/// cloudflared_bin / tunnel) stay GLOBAL on [`AppConfig`]. The route snapshot is fixed +/// for the runtime's life — a project add/remove requires a webhook restart (the +/// composition root wires that on `set_config`). +/// /// Uses `load_validated` (not bare `load`) so the SAME `validate` the wizard / poll /// path enforce gates the start: when `webhook_enabled`, an empty `webhookSecret` or a /// zero `webhookPort` (a hand-edited config that would bind a random port) is rejected @@ -290,14 +351,25 @@ pub async fn start_webhook( "请先在设置中启用 Webhook 并保存配置", )); } + // One route per enabled project — the handler matches an event's repo against these + // and tags the dispatched candidate with the owning project's id (#35). + let routes: Vec = cfg + .projects + .iter() + .filter(|p| p.enabled) + .map(|p| super::webhook::ProjectRoute { + id: p.id.clone(), + repo: p.repo.clone(), + review_label: p.review_label.clone(), + check_label: p.check_label.clone(), + }) + .collect(); state .webhook .start( cfg.webhook_port, cfg.webhook_secret, - cfg.repo, - cfg.review_label, - cfg.check_label, + routes, cfg.cloudflared_bin, super::webhook::TunnelSpec { mode: cfg.webhook_tunnel_mode, diff --git a/src-tauri/src/pr/ledger.rs b/src-tauri/src/pr/ledger.rs index 0fdacc8..e98feb3 100644 --- a/src-tauri/src/pr/ledger.rs +++ b/src-tauri/src/pr/ledger.rs @@ -12,6 +12,7 @@ //! cycle's started candidates) so unbounded concurrent starts can't race the store. use std::collections::HashSet; +use std::sync::Mutex; use serde::{Deserialize, Serialize}; use tauri_plugin_store::StoreExt; @@ -21,10 +22,39 @@ use crate::model::Candidate; /// Store file holding the persisted ledger. const STORE_FILE: &str = "ledger.json"; -/// Key holding the set of dispatched dedup keys. -const DISPATCHED_KEY: &str = "dispatched"; -/// Key holding the dispatch-event log (for cooldown). -const EVENTS_KEY: &str = "events"; +/// Key PREFIX holding the per-project set of dispatched dedup keys (#35). The +/// effective key is `dispatched:{project_id}` (see [`dispatched_key`]); a single +/// `ledger.json` holds every project's partition under its own key. +const DISPATCHED_KEY_PREFIX: &str = "dispatched"; +/// Key PREFIX holding the per-project dispatch-event log for cooldown (#35). The +/// effective key is `events:{project_id}` (see [`events_key`]). +const EVENTS_KEY_PREFIX: &str = "events"; + +/// Serializes EVERY load→stage→save of `ledger.json` across projects (#35). The +/// store is one file holding all projects' partitions (`dispatched:{pid}` / +/// `events:{pid}`); `Store::save` rewrites the WHOLE file, so two parallel project +/// cycles each doing a load→stage→save would interleave and one would clobber the +/// other's just-written partition (a lost dispatch record → re-review storm). A +/// process-global `Mutex<()>` (the data lives in the store, not behind the lock) +/// guards the critical section in [`Ledger::record_many`]; a module static so the +/// lock IDENTITY is fixed (a caller cannot serialize on the wrong mutex). Mirrors +/// the registry's `WRITE_LOCK` rationale. `std` (not `tokio`) `Mutex`: the guarded +/// section is fully synchronous (`tauri-plugin-store` reads/writes are sync), so no +/// `.await` is ever held across the guard. +static LEDGER_WRITE_LOCK: Mutex<()> = Mutex::new(()); + +/// Store key for a project's dispatched dedup-key set: `dispatched:{project_id}` +/// (#35). Partitions the shared `ledger.json` so two projects' identical +/// `(number, head_sha, kind)` dedup keys never collide. +fn dispatched_key(project_id: &str) -> String { + format!("{DISPATCHED_KEY_PREFIX}:{project_id}") +} + +/// Store key for a project's dispatch-event (cooldown) log: `events:{project_id}` +/// (#35). Same partitioning rationale as [`dispatched_key`]. +fn events_key(project_id: &str) -> String { + format!("{EVENTS_KEY_PREFIX}:{project_id}") +} /// One recorded dispatch — the cooldown source (mirrors `router.py` /// dispatch-events: `(pr, kind, dispatchedAtEpoch)`). @@ -65,15 +95,28 @@ pub(crate) fn now_epoch() -> u64 { } /// Load the ledger, batch-record the started candidates at one epoch, and persist -/// — the dispatch-time landing in ONE call. Stamps the clock internally so callers -/// (the dispatcher, [`crate::dispatch`]) pass only the candidates that started; -/// the load + stage + persist + clock all stay in the pr slice. +/// — the dispatch-time landing in ONE call, scoped to `project_id` (#35). Stamps the +/// clock internally so callers (the dispatcher, [`crate::dispatch`]) pass only the +/// candidates that started; the load + stage + persist + clock all stay in the pr +/// slice. The load→stage→save runs under [`LEDGER_WRITE_LOCK`] (in +/// [`Ledger::record_many`]) so parallel project cycles can't clobber each other's +/// whole-file rewrite. pub fn record_dispatched( app: &tauri::AppHandle, + project_id: &str, cands: &[Candidate], ) -> AppResult<()> { - let mut ledger = Ledger::load(app)?; - ledger.record_many(app, cands, now_epoch()) + // Hold the cross-project write lock across the WHOLE load→stage→save (#35): N + // parallel project cycles each rewrite the same `ledger.json` (whole-file save), + // so a load here racing another project's save would drop that project's + // just-recorded partition. The guard makes load + persist one atomic section. + // `.unwrap()` matches the registry's std-Mutex convention; the section is + // synchronous (store reads/writes are sync) so no `.await` is held across it, and + // a panic mid-section can't leave torn state (the data lives in the store, each + // key rewritten wholesale by `record_many`). Poisoning is therefore benign. + let _guard = LEDGER_WRITE_LOCK.lock().unwrap(); + let mut ledger = Ledger::load(app, project_id)?; + ledger.record_many(app, project_id, cands, now_epoch()) } /// Remaining cooldown seconds when `last` is within `secs` of `now`, else `None` @@ -89,21 +132,33 @@ pub fn cooldown_remaining(now: u64, last: u64, secs: u64) -> Option { } impl Ledger { - /// Loads the persisted ledger, defaulting to empty when nothing is stored or - /// a value is corrupt (a corrupt ledger must never block discovery — the worst - /// case is a duplicate dispatch, which the in-process registry guard then drops - /// for any still-active session). - pub fn load(app: &tauri::AppHandle) -> AppResult { + /// Loads `project_id`'s partition of the persisted ledger (#35), defaulting to + /// empty when nothing is stored or a value is corrupt (a corrupt ledger must + /// never block discovery — the worst case is a duplicate dispatch, which the + /// in-process registry guard then drops for any still-active session). Reads only + /// this project's keys (`dispatched:{project_id}` / `events:{project_id}`), so a + /// `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 + /// [`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 + /// `try_reserve_pair` atomic test-and-set at start time. A read racing a concurrent + /// write at worst lets one extra candidate through the cooldown/dedup gate, which + /// the reservation then rejects. The write path ([`record_dispatched`]) DOES hold + /// the lock across its own load→stage→save (a lost write there is unrecoverable). + pub fn load(app: &tauri::AppHandle, project_id: &str) -> AppResult { let store = app .store(STORE_FILE) .map_err(|e| AppError::new(format!("打开 ledger 存储失败: {e}")))?; let dispatched = store - .get(DISPATCHED_KEY) + .get(dispatched_key(project_id)) .and_then(|v| serde_json::from_value::>(v).ok()) .unwrap_or_default(); let events = store - .get(EVENTS_KEY) + .get(events_key(project_id)) .and_then(|v| serde_json::from_value::>(v).ok()) .unwrap_or_default(); @@ -126,18 +181,27 @@ impl Ledger { .max() } - /// Records a batch of dispatches (key + event per candidate) and persists - /// **once**. Invoked by the auto-trigger dispatcher ([`crate::dispatch`]) after - /// a poll cycle's reviews have started; PR discovery itself never dispatches. + /// Records a batch of dispatches (key + event per candidate) into `project_id`'s + /// partition and persists **once**. Invoked by the auto-trigger dispatcher + /// ([`crate::dispatch`]) via [`record_dispatched`] after a poll cycle's reviews + /// have started; PR discovery itself never dispatches. /// /// The single-persist shape matters under unbounded concurrent starts: staging /// every candidate's key/event in memory and saving the store one time avoids /// the interleaved store writes (and redundant saves) that per-candidate /// `record` calls would produce. An empty `cands` slice still touches the store /// (a harmless no-op save) — callers gate on non-empty before calling. + /// + /// **Concurrency (#35):** writes ONLY this project's keys + /// (`dispatched:{project_id}` / `events:{project_id}`), but `Store::save` rewrites + /// the whole `ledger.json`. The cross-project lost-update race that creates is + /// closed by [`record_dispatched`], which holds [`LEDGER_WRITE_LOCK`] across its + /// `load` → this `record_many`, so the load this method's `self` came from and the + /// save below are one atomic critical section relative to other projects' cycles. pub fn record_many( &mut self, app: &tauri::AppHandle, + project_id: &str, cands: &[Candidate], epoch: u64, ) -> AppResult<()> { @@ -147,11 +211,11 @@ impl Ledger { .store(STORE_FILE) .map_err(|e| AppError::new(format!("打开 ledger 存储失败: {e}")))?; store.set( - DISPATCHED_KEY, + dispatched_key(project_id), serde_json::to_value(&self.dispatched).map_err(|e| AppError::new(e.to_string()))?, ); store.set( - EVENTS_KEY, + events_key(project_id), serde_json::to_value(&self.events).map_err(|e| AppError::new(e.to_string()))?, ); store @@ -258,6 +322,50 @@ mod tests { assert_eq!(dispatch_key(7, "deadbeef", "check"), "7@deadbeef:check"); } + // Project store-key partitioning (#35). The persisted `ledger.json` holds every + // project's dedup set / cooldown log under a project-scoped store key; the dedup + // KEY format (`{number}@{head_sha}:{kind}`) is unchanged. This pins that two + // projects' keys differ so a same-(number, head, kind) dispatch in one project + // can't be read as already-dispatched in another (the `Store::set` slot is + // distinct), and that the suffix is the raw project id. + #[test] + fn project_store_keys_are_partitioned() { + assert_eq!(dispatched_key("alpha"), "dispatched:alpha"); + assert_eq!(events_key("alpha"), "events:alpha"); + assert_ne!(dispatched_key("alpha"), dispatched_key("beta")); + assert_ne!(events_key("alpha"), events_key("beta")); + // The dedup KEY format itself is project-agnostic and unchanged — isolation + // comes from the STORE key, not from baking the project into the dedup key. + assert_eq!(dispatch_key(1, "sha", "review"), "1@sha:review"); + } + + // Ledger isolation (#35): two projects whose dedup sets are loaded from distinct + // store-key partitions do NOT collide even when an identical (number, head, kind) + // candidate was dispatched in one. `Ledger::load` is the partitioning seam (it + // reads `dispatched:{pid}`); here we simulate the two loaded partitions directly + // (the live `load` needs a Tauri store) and assert `has_dispatched` is true for + // the project that recorded it and false for the other — the dedup gate is + // per-project, so PR #1@sha:review reviewed under project A is still dispatchable + // under project B. + #[test] + fn dedup_does_not_collide_across_projects() { + let key = dispatch_key(1, "sha", "review"); + + // Project A staged the candidate; project B's partition is empty. + let mut ledger_a = Ledger::default(); + ledger_a.stage_all(&[cand(1, "review")], 1_000); + let ledger_b = Ledger::default(); + + assert!( + ledger_a.has_dispatched(&key), + "project A recorded the dispatch" + ); + assert!( + !ledger_b.has_dispatched(&key), + "the SAME (number, head, kind) must NOT read as dispatched under project B" + ); + } + #[test] fn cooldown_remaining_within_window() { // 100s cooldown, dispatched 30s ago → 70s remaining. diff --git a/src-tauri/src/pr/registry.rs b/src-tauri/src/pr/registry.rs index 6726adc..d331652 100644 --- a/src-tauri/src/pr/registry.rs +++ b/src-tauri/src/pr/registry.rs @@ -23,12 +23,21 @@ use crate::model::{PrPresence, PullRequestView, TrackedPrView}; /// Store file holding the persisted tracked-PR set. const STORE_FILE: &str = "prs.json"; -/// Key holding the list of tracked PRs. -const TRACKED_KEY: &str = "tracked"; -/// Unbounded-growth cap. Beyond this, [`TrackedPrs::prune`] drops the least -/// recently seen records (never the recent working set) — see its doc. +/// Key PREFIX holding the per-project list of tracked PRs (#35). The effective key +/// is `tracked:{project_id}` (see [`tracked_key`]); a single `prs.json` holds every +/// project's tracked set under its own key, so two projects' PRs never mingle in one +/// list. +const TRACKED_KEY_PREFIX: &str = "tracked"; +/// Unbounded-growth cap, applied PER PROJECT (#35). Beyond this, [`TrackedPrs::prune`] +/// drops the least recently seen records (never the recent working set) — see its doc. const MAX_TRACKED: usize = 500; +/// Store key for a project's tracked-PR set: `tracked:{project_id}` (#35). +/// Partitions the shared `prs.json` so each project's retained list is isolated. +fn tracked_key(project_id: &str) -> String { + format!("{TRACKED_KEY_PREFIX}:{project_id}") +} + /// Serializes every read-modify-write of the persisted set (F1, PR #43). The two /// writers — the poll cycle's upsert and the `set_pr_archived` command — each do a /// load→mutate→save of the whole `prs.json`; without a shared critical section they @@ -40,6 +49,13 @@ const MAX_TRACKED: usize = 500; /// serialize on the wrong mutex, which closes the funnel downstream as well as up. /// `std` (not `tokio`) `Mutex`: the guarded section is fully synchronous, so no /// `.await` is ever held across the guard. +/// +/// **Multi-project (#35):** the lock stays GLOBAL (not per-project) on purpose. Each +/// project's set lives under its own store key (`tracked:{project_id}`), but +/// `Store::save` rewrites the WHOLE `prs.json` — so two projects' parallel poll cycles +/// each doing a load→mutate→save would still clobber each other's just-written key. A +/// single global gate over the shared file is the correct granularity; a per-project +/// lock would reopen that cross-project lost-update race. static WRITE_LOCK: Mutex<()> = Mutex::new(()); /// One persisted PR. `first_seen_epoch` is set once on insert and preserved across @@ -66,16 +82,18 @@ pub struct TrackedPrs { } impl TrackedPrs { - /// Loads the persisted set, defaulting to empty when nothing is stored or the - /// value is corrupt (a corrupt registry must never block discovery — the worst - /// case is the list rebuilds from the next round's discovery). - pub fn load(app: &tauri::AppHandle) -> AppResult { + /// Loads `project_id`'s persisted set (#35), defaulting to empty when nothing is + /// stored or the value is corrupt (a corrupt registry must never block discovery — + /// the worst case is the list rebuilds from the next round's discovery). Reads only + /// this project's key (`tracked:{project_id}`), so one project's retained list never + /// shows another's PRs. + pub fn load(app: &tauri::AppHandle, project_id: &str) -> AppResult { let store = app .store(STORE_FILE) .map_err(|e| AppError::new(format!("打开 PR 存储失败: {e}")))?; let prs = store - .get(TRACKED_KEY) + .get(tracked_key(project_id)) .and_then(|v| serde_json::from_value::>(v).ok()) .unwrap_or_default(); @@ -89,13 +107,17 @@ impl TrackedPrs { /// *not expressible* outside this module: a new writer has no way to call `save`, /// so it must go through `mutate_tracked` and inherit the serialization. Making /// this `pub` reopens the lost-update race — do not. - fn save(&self, app: &tauri::AppHandle) -> AppResult<()> { + fn save( + &self, + app: &tauri::AppHandle, + project_id: &str, + ) -> AppResult<()> { let store = app .store(STORE_FILE) .map_err(|e| AppError::new(format!("打开 PR 存储失败: {e}")))?; // tauri-plugin-store 2.x: `Store::set` is infallible and returns `()`. store.set( - TRACKED_KEY, + tracked_key(project_id), serde_json::to_value(&self.prs).map_err(|e| AppError::new(e.to_string()))?, ); store @@ -188,8 +210,12 @@ impl TrackedPrs { /// fixes F1 (upstream: `save` private; downstream: one fixed static [`WRITE_LOCK`]). /// Reads (`get_prs`, the projection below) need no lock: a load is a single whole-value /// store read, so a torn read can't happen and a stale-by-one-round snapshot self-heals. +/// +/// `project_id` (#35) scopes the load + save to that project's key; the GLOBAL +/// [`WRITE_LOCK`] still guards the whole-file `prs.json` rewrite across projects. pub fn mutate_tracked( app: &tauri::AppHandle, + project_id: &str, mutate: impl FnOnce(&mut TrackedPrs) -> (bool, T), ) -> AppResult where @@ -201,10 +227,10 @@ where // `.await` and no panic-prone step (load/save return `Result`, the closures are // pure), so the guard is never poisoned in practice. let _guard = WRITE_LOCK.lock().unwrap(); - let mut tracked = TrackedPrs::load(app)?; + let mut tracked = TrackedPrs::load(app, project_id)?; let (persist, out) = mutate(&mut tracked); if persist { - tracked.save(app)?; + tracked.save(app, project_id)?; } Ok(out) } @@ -239,28 +265,35 @@ pub fn to_view_list(tracked: &TrackedPrs, now: u64, grace_secs: u64) -> Vec(app: &tauri::AppHandle) -> u64 { - super::scheduler::resolve_period(config_service::load(app).map(|c| c.poll_interval_secs)) - .saturating_mul(2) +/// Presence grace window for `project_id`: `2 ×` that project's resolved poll period +/// (#35), so a single missed round keeps a PR `Current` (it only flips `Stale` after +/// the window). The period is THAT project's `poll_interval_secs` (resolved via +/// [`config_service::project`]), clamped through [`super::scheduler::resolve_period`] +/// rather than re-hardcoding the default — a missing project or config-read failure +/// degrades to the default period (×2), matching the scheduler's per-project fallback. +pub fn presence_grace_secs(app: &tauri::AppHandle, project_id: &str) -> u64 { + super::scheduler::resolve_period( + config_service::project(app, project_id).map(|p| p.poll_interval_secs), + ) + .saturating_mul(2) } -/// Projects the tracked set at *now* with the live grace window — the common -/// `to_view_list(tracked, now_epoch(), presence_grace_secs(app))` the command -/// call sites (`get_prs`, `set_pr_archived`'s re-emit) share. The scheduler keeps -/// its inline `to_view_list` form because it already holds the cycle's `now`. -pub fn project( +/// Projects `project_id`'s tracked set at *now* with that project's live grace window +/// — the common `to_view_list(tracked, now_epoch(), presence_grace_secs(app, pid))` +/// the command call sites (`get_prs`, `set_pr_archived`'s re-emit) share. The +/// scheduler keeps its inline `to_view_list` form because it already holds the cycle's +/// `now`. Named `project_snapshot` (#35) to avoid colliding with the config slice's +/// `config::service::project` (which looks up a `Project` by id) — this is the registry +/// PROJECTION of tracked rows into wire views, a distinct responsibility. +pub fn project_snapshot( tracked: &TrackedPrs, app: &tauri::AppHandle, + project_id: &str, ) -> Vec { to_view_list( tracked, super::ledger::now_epoch(), - presence_grace_secs(app), + presence_grace_secs(app, project_id), ) } diff --git a/src-tauri/src/pr/scheduler.rs b/src-tauri/src/pr/scheduler.rs index aaa85f0..9fbe20c 100644 --- a/src-tauri/src/pr/scheduler.rs +++ b/src-tauri/src/pr/scheduler.rs @@ -21,12 +21,22 @@ //! longer drops a row — it flips presence `Current`→`Stale` after the grace window. //! //! **Auto-trigger (#8).** Each cycle also passes its dispatchable candidates (the -//! clean rows) to an injected [`Dispatcher`] hook. The hook is the seam that keeps -//! the `pr` slice review-agnostic: the loop knows nothing about how a review -//! starts, only that a closure consumes `Vec`. The composition root -//! ([`crate::dispatch`]) installs the real dispatcher via [`Scheduler::set_dispatcher`] -//! before `start`, so even the immediate first tick dispatches. - +//! clean rows) to an injected [`ProjectDispatcher`] hook. The hook is the seam that +//! keeps the `pr` slice review-agnostic: the loop knows nothing about how a review +//! starts, only that a closure consumes `(project_id, Vec)`. The +//! composition root ([`crate::dispatch`]) installs the real dispatcher via +//! [`SchedulerSet::set_dispatcher`] before reconciling, so even the immediate first +//! tick dispatches. +//! +//! **Multi-project (#35).** A single [`Scheduler`] drives ONE project's poll loop; +//! [`SchedulerSet`] owns a `project_id → Arc` map and reconciles it to the +//! enabled projects (create/stop/reconfigure). Each scheduler captures its +//! `project_id` into the cycle closure, so every `PrEvent` it emits and every +//! dispatcher call it makes carries the routing key (#35). The dispatcher is shared: +//! [`SchedulerSet::set_dispatcher`] is installed once and cloned into each scheduler +//! on `reconcile`, so a project added later still inherits it. + +use std::collections::HashMap; use std::future::Future; use std::pin::Pin; use std::sync::{Arc, Mutex as StdMutex}; @@ -37,21 +47,23 @@ use tauri::Emitter; // for app.emit use tokio::sync::Notify; use tokio::time::MissedTickBehavior; -use crate::config::service as config_service; +use crate::config::service::{self as config_service, Project}; use crate::error::AppResult; use crate::events::{PrEvent, PRS_UPDATED_EVENT}; use crate::model::{Candidate, TrackedPrView}; use super::registry; -/// Abstract per-cycle dispatch hook: consumes the cycle's dispatchable -/// [`Candidate`]s and drives them to completion (in practice: auto-start their -/// reviews concurrently). Boxed-future + `Arc` so it is `Clone`able into the cycle -/// closure and erased of the review slice's types — the `pr` slice stays -/// review-agnostic (the only cross-slice contract it sees is `Candidate`). The +/// 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 +/// routing key the composition root's dispatcher needs to resolve the project's repo +/// / engine / ledger partition. Boxed-future + `Arc` so it is `Clone`able into each +/// scheduler's cycle closure and erased of the review slice's types — the `pr` slice +/// stays review-agnostic (the only cross-slice contract it sees is `Candidate`). The /// real implementation lives in the composition root ([`crate::dispatch`]). -pub type Dispatcher = - Arc) -> Pin + Send>> + Send + Sync>; +pub type ProjectDispatcher = + Arc) -> Pin + Send>> + Send + Sync>; /// Default poll period when config is unreadable or non-positive. Mirrors /// `AppConfig::default().poll_interval_secs`. A 0 period would make @@ -60,17 +72,19 @@ pub type Dispatcher = /// window rather than re-hardcoding the default. pub(crate) const DEFAULT_POLL_INTERVAL_SECS: u64 = 120; -/// The composition-root handle for the scheduled-pull loop. Lives in -/// [`crate::state::AppState`]; all methods take `&self` and use interior -/// mutability so a single shared `State` can drive it. +/// The poll-loop handle for ONE project (#35). Owned by a [`SchedulerSet`] entry +/// keyed by `project_id`; all methods take `&self` and use interior mutability so the +/// set can drive it through an `Arc`. Pre-#35 this was the single +/// composition-root handle in `AppState`; now `AppState` holds the [`SchedulerSet`]. #[derive(Default)] pub struct Scheduler { task: StdMutex>, - /// The auto-trigger dispatch hook (#8), installed by the composition root via - /// [`Self::set_dispatcher`] before `start`. `Mutex>` defaults to - /// `None` (so `#[derive(Default)]` still holds) — a `None` dispatcher means a - /// cycle discovers + emits but starts no reviews (the pre-#8 behavior). - dispatcher: StdMutex>, + /// The auto-trigger dispatch hook (#8), installed via [`Self::set_dispatcher`] + /// before `start` ([`SchedulerSet::reconcile`] clones the set's shared dispatcher + /// into each scheduler). `Mutex>` defaults to `None` (so + /// `#[derive(Default)]` still holds) — a `None` dispatcher means a cycle discovers + /// + emits but starts no reviews (the pre-#8 behavior). + dispatcher: StdMutex>, } /// The live task plus the channels the loop selects on. @@ -82,17 +96,19 @@ struct RunningTask { } impl Scheduler { - /// Installs the auto-trigger dispatch hook (#8). Called once by the composition - /// root *before* `start`, so the immediate first tick already dispatches. - /// Replaces any prior hook (last writer wins); a never-set dispatcher leaves - /// cycles discover-and-emit only. - pub fn set_dispatcher(&self, d: Dispatcher) { + /// Installs the auto-trigger dispatch hook (#8). Called *before* `start` (by + /// [`SchedulerSet::reconcile`], cloning the set's shared dispatcher), so the + /// immediate first tick already dispatches. Replaces any prior hook (last writer + /// wins); a never-set dispatcher leaves cycles discover-and-emit only. + pub fn set_dispatcher(&self, d: ProjectDispatcher) { *self.dispatcher.lock().unwrap() = Some(d); } - /// Spawns the poll loop. Idempotent: if a task is already live this is a - /// no-op (no double-spawn). A finished task slot is replaced. - pub fn start(&self, app: tauri::AppHandle) { + /// Spawns this project's poll loop (#35), capturing `project_id` into the cycle so + /// every emit / dispatch it makes is routed to that project. Idempotent: if a task + /// is already live this is a no-op (no double-spawn). A finished task slot is + /// replaced. + pub fn start(&self, app: tauri::AppHandle, project_id: String) { let mut slot = self.task.lock().unwrap(); if let Some(task) = slot.as_ref() { if !task.handle.inner().is_finished() { @@ -105,23 +121,33 @@ impl Scheduler { let stop = Arc::new(Notify::new()); // Snapshot the installed dispatcher once into the cycle closure: the loop - // task outlives this `start` call, so it captures an owned `Option` - // rather than re-locking `self` each cycle. `None` ⇒ no auto-trigger. + // task outlives this `start` call, so it captures an owned + // `Option` rather than re-locking `self` each cycle. + // `None` ⇒ no auto-trigger. let dispatcher = self.dispatcher.lock().unwrap().clone(); - // Production wiring: the period comes from the live config each rebuild, - // and each cycle discovers → upserts the persisted set → emits → dispatches. - // Both are injected into the generic `run_loop` so the lifecycle is testable (F4). + // Production wiring: the period comes from THIS project's live config each + // rebuild (resolved by id, with a default fallback if the project is gone), and + // each cycle discovers → upserts the persisted set → emits → dispatches, all + // scoped to `project_id`. Both are injected into the generic `run_loop` so the + // lifecycle is testable (F4). let period_provider = { let app = app.clone(); - move || resolve_period(config_service::load(&app).map(|c| c.poll_interval_secs)) + let project_id = project_id.clone(); + move || { + resolve_period( + config_service::project(&app, &project_id).map(|p| p.poll_interval_secs), + ) + } }; let on_cycle = { let app = app.clone(); + let project_id = project_id.clone(); move || { let app = app.clone(); + let project_id = project_id.clone(); let dispatcher = dispatcher.clone(); - async move { discover_emit_dispatch(&app, dispatcher.as_ref()).await } + async move { discover_emit_dispatch(&app, &project_id, dispatcher.as_ref()).await } } }; @@ -173,6 +199,115 @@ impl Scheduler { } } +/// The composition-root handle for ALL projects' poll loops (#35). Lives in +/// [`crate::state::AppState`]; all methods take `&self` and use interior mutability so +/// a single shared `State` can drive every project. Owns a +/// `project_id → Arc` map plus the one shared [`ProjectDispatcher`] cloned +/// into each scheduler on [`Self::reconcile`]. +#[derive(Default)] +pub struct SchedulerSet { + /// One [`Scheduler`] per RUNNING project, keyed by `project_id`. `Arc` so a + /// scheduler outlives a transient map borrow (the spawned loop holds no map + /// reference; the map only holds the control handle). + inner: StdMutex>>, + /// The auto-trigger dispatch hook (#8), installed once by the composition root via + /// [`Self::set_dispatcher`] and cloned into each scheduler on `reconcile`. `None` + /// (the `#[derive(Default)]` value) leaves every cycle discover-and-emit only. + dispatcher: StdMutex>, +} + +impl SchedulerSet { + /// Installs the shared auto-trigger dispatch hook (#8/#35). Called once by the + /// composition root *before* the first [`Self::reconcile`], so a scheduler created + /// by that reconcile inherits it and its immediate first tick already dispatches. + /// Replaces any prior hook (last writer wins); already-running schedulers keep the + /// dispatcher they were created with (a re-install only affects future creates). + pub fn set_dispatcher(&self, d: ProjectDispatcher) { + *self.dispatcher.lock().unwrap() = Some(d); + } + + /// Reconciles the running schedulers to `projects` (#35). Idempotent — safe to call + /// on every config save: + /// - an `enabled` project NOT yet in the map → create a [`Scheduler`], install the + /// shared dispatcher, and `start` it (captures the project's id); + /// - a mapped id that is no longer enabled (disabled, removed, or absent from + /// `projects`) → `stop` it and drop it from the map; + /// - a surviving enabled project → `reconfigure` (re-read its period; the first + /// tick fires immediately, so this also re-polls). + /// + /// A DISABLED project is treated identically to a removed one (stopped), so the + /// scheduler set always mirrors exactly the enabled projects. + pub fn reconcile(&self, app: &tauri::AppHandle, projects: &[Project]) { + let dispatcher = self.dispatcher.lock().unwrap().clone(); + let mut map = self.inner.lock().unwrap(); + + // The set of ids that SHOULD be running (enabled projects). + let enabled_ids: std::collections::HashSet<&str> = projects + .iter() + .filter(|p| p.enabled) + .map(|p| p.id.as_str()) + .collect(); + + // Stop + drop schedulers whose project is no longer enabled (disabled / removed). + map.retain(|id, scheduler| { + if enabled_ids.contains(id.as_str()) { + true + } else { + scheduler.stop(); + false + } + }); + + // Create-or-reconfigure each enabled project. + for project in projects.iter().filter(|p| p.enabled) { + match map.get(&project.id) { + Some(scheduler) => scheduler.reconfigure(), // survivor: re-read period + re-poll. + None => { + let scheduler = Arc::new(Scheduler::default()); + if let Some(d) = dispatcher.clone() { + scheduler.set_dispatcher(d); + } + scheduler.start(app.clone(), project.id.clone()); + map.insert(project.id.clone(), scheduler); + } + } + } + } + + /// Triggers an immediate discovery on `project_id`'s running loop ("立即拉取"). + /// Returns whether a running scheduler was actually woken: `false` when that + /// project is unknown / stopped, so the caller (`poll_now`) can surface an error + /// instead of leaving the frontend awaiting a `prs:updated` that never arrives. + pub fn wake(&self, project_id: &str) -> bool { + self.inner + .lock() + .unwrap() + .get(project_id) + .map(|s| s.wake()) + .unwrap_or(false) + } + + /// Asks `project_id`'s running loop to rebuild its ticker with a fresh period + /// (no-op if that project is unknown / stopped). Because the first tick fires + /// immediately, this also triggers an immediate discovery. + pub fn reconfigure(&self, project_id: &str) { + if let Some(s) = self.inner.lock().unwrap().get(project_id) { + s.reconfigure(); + } + } + + /// Stops + drops EVERY project's loop (the `stop_polling`-all path + app shutdown). + /// After this the set is empty; a later [`Self::reconcile`] re-creates the enabled + /// schedulers from scratch. + pub fn stop_all(&self) { + let mut map = self.inner.lock().unwrap(); + for scheduler in map.values() { + scheduler.stop(); + } + map.clear(); + } +} + /// The poll loop, with its period source and per-cycle action injected (F4) so /// the lifecycle (start/wake/reconfigure/stop) is unit-testable without `gh`, /// config, or an `AppHandle`. Outer loop rebuilds the ticker on `reconfigure`; @@ -235,33 +370,44 @@ async fn run_loop( /// empty dispatchable list skips the hook entirely. async fn discover_emit_dispatch( app: &tauri::AppHandle, - dispatcher: Option<&Dispatcher>, + project_id: &str, + dispatcher: Option<&ProjectDispatcher>, ) { - let (event, dispatchable) = match super::commands::discover(app).await { + let (event, dispatchable) = match super::commands::discover(app, project_id).await { Ok((views, dispatchable)) => { // 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 - // and lose a write. The closure always persists (an upsert always changes - // the set); the projection is built inside the seam from the just-upserted - // set. `now`/`grace` are read before the lock to keep the critical section + // and lose a write. Scoped to `project_id` (#35) so this project's set is + // isolated. The closure always persists (an upsert always changes the set); + // the projection is built inside the seam from the just-upserted set. + // `now`/`grace` are read before the lock to keep the critical section // minimal. A store failure (load or save) maps to Error, not a misleading // Updated (F2). Auto-dispatch is independent of persistence and still runs. let now = super::ledger::now_epoch(); - let grace = registry::presence_grace_secs(app); - let event = persist_event(registry::mutate_tracked(app, |tracked| { - tracked.upsert(&views, now); - (true, registry::to_view_list(tracked, now, grace)) - })); + 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) } // On discovery error the dispatchable list is empty — nothing auto-starts. - Err(e) => (PrEvent::Error { message: e.message }, Vec::new()), + Err(e) => ( + 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) if let Some(d) = dispatcher { - if !dispatchable.is_empty() && auto_review_enabled(app) { + if !dispatchable.is_empty() && auto_review_enabled(app, project_id) { // Spawn the dispatch DETACHED rather than awaiting it inline. This cycle // runs inside the loop's stop-cancellable `select!` (the F1 cancellation // domain that lets a stop reap the in-flight `gh` child). Awaiting @@ -276,34 +422,46 @@ async fn discover_emit_dispatch( // guard. Discovery itself stays cancellable (it is awaited above), so a // stop still reaps `gh`. The JoinHandle is dropped explicitly (detached): // the task runs to completion regardless of the poll loop. - drop(tauri::async_runtime::spawn(d(dispatchable))); + drop(tauri::async_runtime::spawn(d( + project_id.to_string(), + dispatchable, + ))); } } } -/// 每轮重读自动 review 开关（运行时切换无需重启）。 -/// 这是 pr→config 的**函数级跨切片读**（走 config 公有 service，AppConfig 仍 config 私有）。 -/// load 失败 → 返回 false（不派发）：config 不可读时不擅自消耗 review 额度/算力， -/// 宁可漏触发也不误触发；下一轮 load 成功即恢复。 +/// 每轮重读 `project_id` 的自动 review 开关（运行时切换无需重启，#35 按项目）。 +/// 这是 pr→config 的**函数级跨切片读**（走 config 公有 service `project`，`AppConfig` +/// 仍 config 私有；`auto_review` 现为 [`Project`] 字段）。load / 找不到项目 → 返回 false +/// （不派发）：config 不可读或项目缺失时不擅自消耗 review 额度/算力，宁可漏触发也不误触发； +/// 下一轮 load 成功即恢复。 /// `pub(crate)`：webhook trigger（[`crate::pr::webhook`]）的派发闭包复用同一开关， -/// 与本轮询调用点一致——两条 auto-trigger 路径共用同一 autoReview gate。 -pub(crate) fn auto_review_enabled(app: &tauri::AppHandle) -> bool { - config_service::load(app) - .map(|c| c.auto_review) +/// 与本轮询调用点一致——两条 auto-trigger 路径共用同一 per-project autoReview gate。 +pub(crate) fn auto_review_enabled( + app: &tauri::AppHandle, + project_id: &str, +) -> bool { + config_service::project(app, project_id) + .map(|p| p.auto_review) .unwrap_or(false) } -/// Maps the locked persist seam's result to the cycle event (F2). A successful -/// load→upsert→save yields the retained projection (`Updated`); any store failure -/// (load or save, surfaced as `Err` by [`registry::mutate_tracked`]) yields `Error` -/// instead of a misleading `Updated` — the in-memory set was not durably persisted -/// (it vanishes on restart), so claiming "retained snapshot updated" would lie to the -/// UI. Pure over the seam's result, so the decision is unit-testable without an -/// `AppHandle` or a store (the emit + dispatch around it need a live app). -fn persist_event(result: AppResult>) -> PrEvent { +/// Maps the locked persist seam's result to the cycle event (F2), scoped to +/// `project_id` (#35). A successful load→upsert→save yields the retained projection +/// (`Updated`); any store failure (load or save, surfaced as `Err` by +/// [`registry::mutate_tracked`]) yields `Error` instead of a misleading `Updated` — +/// the in-memory set was not durably persisted (it vanishes on restart), so claiming +/// "retained snapshot updated" would lie to the UI. Pure over the seam's result, so +/// the decision is unit-testable without an `AppHandle` or a store (the emit + +/// dispatch around it need a live app). +fn persist_event(project_id: &str, result: AppResult>) -> PrEvent { match result { - Ok(list) => PrEvent::Updated { prs: list }, + Ok(list) => PrEvent::Updated { + project_id: project_id.to_string(), + prs: list, + }, Err(e) => PrEvent::Error { + project_id: project_id.to_string(), message: format!("PR 列表持久化失败：{}", e.message), }, } @@ -351,21 +509,28 @@ mod tests { // un-persisted in-memory set as the retained snapshot (silently lost on restart). #[test] fn persist_event_on_persist_ok_is_updated() { - let ev = persist_event(Ok(Vec::new())); - assert!( - matches!(ev, PrEvent::Updated { .. }), - "a successful persist emits Updated" - ); + let ev = persist_event("p1", Ok(Vec::new())); + // #35: the event carries the routing `project_id`. + match ev { + PrEvent::Updated { project_id, .. } => assert_eq!(project_id, "p1"), + other => panic!("a successful persist emits Updated, not {other:?}"), + } } #[test] fn persist_event_on_store_failure_is_error_not_updated() { - let ev = persist_event(Err(AppError::new("写入 PR 存储失败: disk full"))); + let ev = persist_event("p1", Err(AppError::new("写入 PR 存储失败: disk full"))); match ev { - PrEvent::Error { message } => assert!( - message.contains("持久化失败"), - "Error carries a persist-failure message, got {message:?}" - ), + PrEvent::Error { + project_id, + message, + } => { + assert_eq!(project_id, "p1", "Error is routed to the project (#35)"); + assert!( + message.contains("持久化失败"), + "Error carries a persist-failure message, got {message:?}" + ); + } other => panic!("a store failure must emit Error, not {other:?}"), } } @@ -387,23 +552,29 @@ mod tests { #[tokio::test] async fn set_dispatcher_stores_and_retrieves_the_hook() { use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Mutex as TestMutex; // The hook plumbing in isolation: `set_dispatcher` stores a counting // closure; retrieving it (the same `lock().clone()` `start` does) and - // invoking it with sample candidates must run the closure. This verifies - // storage/retrieval without needing an `AppHandle` or real dispatch. + // invoking it with a project id + sample candidates must run the closure. + // Verifies storage/retrieval AND that the leading `project_id` (#35) reaches + // the hook — without needing an `AppHandle` or real dispatch. let scheduler = Scheduler::default(); let count = Arc::new(AtomicUsize::new(0)); let seen = Arc::new(AtomicUsize::new(0)); - let dispatcher: Dispatcher = { + let seen_pid: Arc>> = Arc::new(TestMutex::new(None)); + let dispatcher: ProjectDispatcher = { let count = Arc::clone(&count); let seen = Arc::clone(&seen); - Arc::new(move |cands: Vec| { + let seen_pid = Arc::clone(&seen_pid); + Arc::new(move |project_id: String, cands: Vec| { let count = Arc::clone(&count); let seen = Arc::clone(&seen); + let seen_pid = Arc::clone(&seen_pid); Box::pin(async move { count.fetch_add(1, Ordering::SeqCst); seen.fetch_add(cands.len(), Ordering::SeqCst); + *seen_pid.lock().unwrap() = Some(project_id); }) }) }; @@ -415,10 +586,74 @@ mod tests { .unwrap() .clone() .expect("set_dispatcher stores the hook"); - stored(vec![candidate(1, "review"), candidate(2, "check")]).await; + stored( + "p1".to_string(), + vec![candidate(1, "review"), candidate(2, "check")], + ) + .await; assert_eq!(count.load(Ordering::SeqCst), 1, "hook ran once"); assert_eq!(seen.load(Ordering::SeqCst), 2, "hook saw both candidates"); + assert_eq!( + seen_pid.lock().unwrap().as_deref(), + Some("p1"), + "the routing project_id reached the hook (#35)" + ); + } + + // ── SchedulerSet (#35) ────────────────────────────────────────────────── + // The map-management methods that don't need an `AppHandle` (`reconcile` does, + // so it's exercised in the live app). These lock the multi-project invariants: + // a default set is empty + dispatcher-less, `wake`/`reconfigure` on an unknown + // project are safe no-ops, and `stop_all` clears the map. + + #[test] + fn default_scheduler_set_is_empty_and_dispatcherless() { + let set = SchedulerSet::default(); + assert!(set.inner.lock().unwrap().is_empty()); + assert!(set.dispatcher.lock().unwrap().is_none()); + } + + #[test] + fn scheduler_set_wake_unknown_project_is_false() { + // `poll_now` relies on this: waking a project with no running scheduler + // returns false so the command can surface "paused" rather than hang. + let set = SchedulerSet::default(); + assert!(!set.wake("nope")); + } + + #[test] + fn scheduler_set_reconfigure_and_stop_all_unknown_are_noops() { + let set = SchedulerSet::default(); + set.reconfigure("nope"); // no panic on an empty map + set.stop_all(); // no panic on an empty map + assert!(set.inner.lock().unwrap().is_empty()); + } + + #[test] + fn scheduler_set_set_dispatcher_stores_shared_hook() { + // The set's shared dispatcher is what `reconcile` clones into each scheduler; + // installing it before any reconcile is what lets a later-added project + // inherit auto-dispatch. + let set = SchedulerSet::default(); + let dispatcher: ProjectDispatcher = + Arc::new(|_pid: String, _cands: Vec| Box::pin(async {})); + set.set_dispatcher(dispatcher); + assert!(set.dispatcher.lock().unwrap().is_some()); + } + + #[test] + fn scheduler_set_stop_all_clears_running_entries() { + // Seed the map directly with a never-started scheduler (no `AppHandle` + // needed): `stop_all` must drop every entry so a later reconcile rebuilds. + let set = SchedulerSet::default(); + set.inner + .lock() + .unwrap() + .insert("p1".to_string(), Arc::new(Scheduler::default())); + assert_eq!(set.inner.lock().unwrap().len(), 1); + set.stop_all(); + assert!(set.inner.lock().unwrap().is_empty()); } fn candidate(number: u64, kind: &str) -> Candidate { diff --git a/src-tauri/src/pr/webhook.rs b/src-tauri/src/pr/webhook.rs index de2f1d4..bbfe1e4 100644 --- a/src-tauri/src/pr/webhook.rs +++ b/src-tauri/src/pr/webhook.rs @@ -18,16 +18,26 @@ //! 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 [`Candidate`] and hands it to the injected -//! [`Dispatcher`] (the composition root's gate + `auto_dispatch` closure), reusing -//! the entire vetted dispatch path with zero duplication. +//! 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. +//! +//! **Multi-project routing (#35).** One global receiver / port / secret / tunnel +//! serves EVERY monitored project. The handler routes each verified event to the +//! enabled project whose `repo` matches the payload's repository (case-insensitive), +//! classifies review-vs-check with THAT project's labels, and dispatches under that +//! project's id. A payload whose repo matches no enabled project is dropped +//! (fail-closed — same spirit as the old single-repo ownership gate). The route list +//! ([`WebhookCtx::routes`]) is a SNAPSHOT taken at [`WebhookManager::start`] time from +//! the enabled projects; adding/removing/enabling a project requires a webhook restart +//! 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 [`Dispatcher`] closure the root installs via +//! 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 → map → hand off. +//! parse → route → hand off. //! //! **Security.** The endpoint is public (via the tunnel), so every request is //! HMAC-verified (`X-Hub-Signature-256`) against the configured secret before the @@ -55,7 +65,7 @@ use tokio::io::{AsyncBufRead, AsyncBufReadExt, BufReader, Lines}; use tokio::process::{Child, Command}; use tokio::sync::oneshot; -use super::scheduler::Dispatcher; +use super::scheduler::ProjectDispatcher; use crate::error::{AppError, AppResult}; use crate::model::{Candidate, WebhookTunnelMode}; @@ -153,15 +163,35 @@ pub struct TunnelSpec { pub public_url: String, } +/// One enabled project's webhook routing info (#35) — the minimal slice of +/// [`crate::config::model::Project`] the handler needs to route a push event: +/// match the event repo, classify review-vs-check by THIS project's labels, and +/// dispatch under THIS project's id. The composition root builds the list from +/// every `enabled` project at [`WebhookManager::start`] time (see [`WebhookCtx::routes`]); +/// it does NOT carry the full `Project` so the `pr` slice stays decoupled from the +/// config slice's domain model (parity with how `start` takes flat receiver params). +pub struct ProjectRoute { + /// The routing key emitted on the dispatched candidate (`Project::id`). + pub id: String, + /// The monitored repo `owner/name` matched (case-insensitively) against the + /// event's repository. + pub repo: String, + /// Label that classifies an event as a `review` turn (this project's). + pub review_label: String, + /// Label that classifies an event as a `check` turn (this project's). + pub check_label: String, +} + /// Owns the running receiver + tunnel. `&self` methods + interior mutability so it /// lives in `AppState` (which stays `Default`), mirroring `Scheduler`/`CodexManager`. #[derive(Default)] pub struct WebhookManager { /// Installed once by the composition root (lib.rs) BEFORE any start, like - /// [`super::scheduler::Scheduler::set_dispatcher`]. The closure 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 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>, runtime: StdMutex>, /// Serializes `start` (bind + spawn + tunnel-URL await) so concurrent starts /// can't double-bind the port. @@ -262,7 +292,7 @@ impl WebhookRuntime { impl WebhookManager { /// Install the dispatch hook (composition root, before any start). - pub fn set_dispatcher(&self, d: Dispatcher) { + pub fn set_dispatcher(&self, d: ProjectDispatcher) { *self.dispatcher.lock().unwrap() = Some(d); } @@ -279,14 +309,19 @@ impl WebhookManager { /// `public_url` is empty). Does NOT require cloudflared. /// - `Listener`: bind only, spawn no child; `public_url` from config. Does NOT /// require cloudflared. - #[allow(clippy::too_many_arguments)] + /// + /// `routes` (#35) is the SNAPSHOT of enabled projects' routing info the handler + /// matches each event against (the composition root builds it from every `enabled` + /// [`crate::config::model::Project`] at this call). It is captured into + /// [`WebhookCtx::routes`] and never refreshed for the life of the runtime — a + /// project add/remove/enable change requires a restart (the root wires that on + /// `set_config`). The receiver params (port / secret / cloudflared_bin) stay GLOBAL + /// (one receiver serves all projects). pub async fn start( &self, port: u16, secret: String, - repo: String, - review_label: String, - check_label: String, + routes: Vec, cloudflared_bin: String, tunnel: TunnelSpec, ) -> AppResult { @@ -365,9 +400,7 @@ impl WebhookManager { let ctx = Arc::new(WebhookCtx { secret, - repo, - review_label, - check_label, + routes, dispatcher, }); let router = Router::new() @@ -587,13 +620,16 @@ impl WebhookManager { /// Shared, runtime-agnostic state for the axum handler. struct WebhookCtx { secret: String, - /// The monitored repo `owner/name` (from `AppConfig.repo`). The handler requires a - /// verified payload's repository to match this (F2) — HMAC proves the secret is - /// known, not that the event is for the repo this app reviews. - repo: String, - review_label: String, - check_label: String, - dispatcher: Dispatcher, + /// The enabled projects' routing info (#35), a SNAPSHOT taken at + /// [`WebhookManager::start`] time from every `enabled` + /// [`crate::config::model::Project`]. The handler routes a verified payload to the + /// project whose `repo` matches the event repository (case-insensitive); a payload + /// matching no route is dropped (fail-closed — HMAC proves the secret is known, not + /// that the event is for a repo this app reviews). This list does NOT refresh for + /// 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, } /// `POST /webhook`. Verify the GitHub HMAC, map a `pull_request` payload to a @@ -629,13 +665,12 @@ async fn handle_webhook( Err(_) => return StatusCode::BAD_REQUEST, }; - if let Some(candidate) = - payload_to_candidate(&payload, &ctx.repo, &ctx.review_label, &ctx.check_label) - { + 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. + // 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(vec![candidate]))); + drop(spawn(dispatcher(project_id, vec![candidate]))); } StatusCode::OK } @@ -661,31 +696,27 @@ 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 dispatchable [`Candidate`], or -/// `None` when it carries no single trigger label. Conflict (BOTH 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+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, - repo: &str, - review_label: &str, - check_label: &str, -) -> Option { +/// 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-ownership gate (F2): the HMAC proves the POST came from a sender who knows the - // secret — NOT that the event is for the repo THIS app monitors/reviews. A webhook - // misconfigured onto a different repo, or a reused secret, would otherwise let a - // label event elsewhere cross-trigger a review of the configured repo (the engine - // always reviews `cfg.repo`, so the payload's PR number would be applied to the wrong - // repo). Require the event's repo (top-level `repository.full_name`, falling back to - // the PR's `base.repo.full_name`) to equal the configured `repo`; missing or - // mismatched → no candidate (fail closed). Case-insensitive, matching GitHub's - // repo-name semantics (and the poll path's `gh --repo`). + // 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. let event_repo = payload .get("repository") .and_then(|r| r.get("full_name")) @@ -695,11 +726,10 @@ fn payload_to_candidate( .and_then(|b| b.get("repo")) .and_then(|r| r.get("full_name")) .and_then(Value::as_str) - }); - match event_repo { - Some(r) if r.eq_ignore_ascii_case(repo) => {} - _ => return None, - } + })?; + 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 @@ -716,8 +746,10 @@ fn payload_to_candidate( .iter() .filter_map(|l| l.get("name").and_then(Value::as_str)) .collect(); - let has_review = label_names.contains(&review_label); - let has_check = label_names.contains(&check_label); + // 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", @@ -752,15 +784,18 @@ fn payload_to_candidate( _ => true, }; - Some(Candidate { - number, - head_sha, - head_ref, - author, - is_cross_repository, - is_draft, - kind: kind.to_string(), - }) + Some(( + route.id.clone(), + Candidate { + number, + head_sha, + head_ref, + author, + is_cross_repository, + is_draft, + kind: kind.to_string(), + }, + )) } /// Probe whether `cloudflared` is runnable (`cloudflared --version`). Never errors; @@ -999,11 +1034,35 @@ mod tests { serde_json::json!({ "action": "labeled", "pull_request": pr }) } + fn route(id: &str, repo: &str, review_label: &str, check_label: &str) -> ProjectRoute { + ProjectRoute { + id: id.to_string(), + repo: repo.to_string(), + review_label: review_label.to_string(), + check_label: check_label.to_string(), + } + } + + /// A one-project route list for `owner/repo` (id `"default"`) — the single-project + /// analogue of the old flat `(repo, review_label, check_label)` args, so the + /// existing parse/map tests read unchanged apart from the routing wrapper. + fn single_route(review_label: &str, check_label: &str) -> Vec { + vec![route("default", "owner/repo", review_label, check_label)] + } + + /// 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 { + single_route("review", "check") + } + #[test] fn payload_to_candidate_maps_review_label() { let p = pr_payload(&["needs-review"], serde_json::json!({})); - let c = payload_to_candidate(&p, "owner/repo", "needs-review", "needs-check") - .expect("review candidate"); + let (project_id, c) = + payload_to_candidate(&p, &single_route("needs-review", "needs-check")) + .expect("review candidate"); + assert_eq!(project_id, "default"); assert_eq!(c.number, 42); assert_eq!(c.kind, "review"); assert_eq!(c.head_sha, "abc123"); @@ -1016,8 +1075,10 @@ mod tests { #[test] fn payload_to_candidate_maps_check_label() { let p = pr_payload(&["needs-check"], serde_json::json!({})); - let c = payload_to_candidate(&p, "owner/repo", "needs-review", "needs-check") - .expect("check candidate"); + let (project_id, c) = + payload_to_candidate(&p, &single_route("needs-review", "needs-check")) + .expect("check candidate"); + assert_eq!(project_id, "default"); assert_eq!(c.kind, "check"); } @@ -1025,10 +1086,14 @@ mod tests { fn payload_to_candidate_skips_conflict_and_no_trigger_label() { // Both trigger labels → conflict → None (mirrors the poll path). let both = pr_payload(&["needs-review", "needs-check"], serde_json::json!({})); - assert!(payload_to_candidate(&both, "owner/repo", "needs-review", "needs-check").is_none()); + assert!( + payload_to_candidate(&both, &single_route("needs-review", "needs-check")).is_none() + ); // No trigger label → None. let none = pr_payload(&["unrelated"], serde_json::json!({})); - assert!(payload_to_candidate(&none, "owner/repo", "needs-review", "needs-check").is_none()); + assert!( + payload_to_candidate(&none, &single_route("needs-review", "needs-check")).is_none() + ); } #[test] @@ -1037,20 +1102,22 @@ mod tests { // 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, "owner/repo", "needs-review", "needs-check").is_none() + 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, "owner/repo", "needs-review", "needs-check").is_none() + payload_to_candidate(&merged, &single_route("needs-review", "needs-check")).is_none() ); // Defensive: a payload with no `state` field fails safe to no candidate. let no_state = pr_payload(&["needs-review"], serde_json::json!({ "state": null })); assert!( - payload_to_candidate(&no_state, "owner/repo", "needs-review", "needs-check").is_none() + payload_to_candidate(&no_state, &single_route("needs-review", "needs-check")).is_none() ); // Sanity: the default helper payload IS open and still maps. let open = pr_payload(&["needs-review"], serde_json::json!({})); - assert!(payload_to_candidate(&open, "owner/repo", "needs-review", "needs-check").is_some()); + assert!( + payload_to_candidate(&open, &single_route("needs-review", "needs-check")).is_some() + ); } #[test] @@ -1060,8 +1127,9 @@ mod tests { // gate, not the parse, decides to skip it. let draft = pr_payload(&["needs-review"], serde_json::json!({ "draft": true })); assert!( - payload_to_candidate(&draft, "owner/repo", "needs-review", "needs-check") + payload_to_candidate(&draft, &single_route("needs-review", "needs-check")) .unwrap() + .1 .is_draft ); @@ -1070,8 +1138,9 @@ mod tests { serde_json::json!({ "head": { "sha": "s", "ref": "r", "repo": { "full_name": "forker/repo" } } }), ); assert!( - payload_to_candidate(&fork, "owner/repo", "needs-review", "needs-check") + payload_to_candidate(&fork, &single_route("needs-review", "needs-check")) .unwrap() + .1 .is_cross_repository ); } @@ -1085,8 +1154,9 @@ mod tests { serde_json::json!({ "head": { "sha": "s", "ref": "r", "repo": null } }), ); assert!( - payload_to_candidate(&p, "owner/repo", "needs-review", "needs-check") + payload_to_candidate(&p, &single_route("needs-review", "needs-check")) .unwrap() + .1 .is_cross_repository ); } @@ -1094,7 +1164,32 @@ mod tests { #[test] fn payload_to_candidate_none_without_pull_request() { let p = serde_json::json!({ "action": "labeled" }); - assert!(payload_to_candidate(&p, "owner/repo", "needs-review", "needs-check").is_none()); + assert!(payload_to_candidate(&p, &single_route("needs-review", "needs-check")).is_none()); + } + + #[test] + fn payload_to_candidate_fails_closed_when_repo_matches_no_route() { + // 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. + 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)" + ); + // Sanity: adding the matching route makes the SAME payload route + dispatch, + // so the None 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"); } #[test] @@ -1256,7 +1351,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_dispatcher(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. @@ -1264,9 +1359,7 @@ mod tests { .start( 0, "shh".to_string(), - "owner/repo".to_string(), - "review".to_string(), - "check".to_string(), + start_routes(), "prmonitor-no-such-cloudflared".to_string(), TunnelSpec { mode: WebhookTunnelMode::Command, @@ -1302,15 +1395,13 @@ 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_dispatcher(Arc::new(|_, _| Box::pin(async {}))); let s = mgr .start( 0, "shh".to_string(), - "owner/repo".to_string(), - "review".to_string(), - "check".to_string(), + start_routes(), "bogus".to_string(), TunnelSpec { mode: WebhookTunnelMode::Command, @@ -1358,15 +1449,13 @@ 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_dispatcher(Arc::new(|_, _| Box::pin(async {}))); let s = mgr .start( 0, "shh".to_string(), - "owner/repo".to_string(), - "review".to_string(), - "check".to_string(), + start_routes(), "prmonitor-no-such-cloudflared".to_string(), TunnelSpec { mode: WebhookTunnelMode::Listener, @@ -1417,15 +1506,13 @@ 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_dispatcher(Arc::new(|_, _| Box::pin(async {}))); let s = mgr .start( 0, "shh".to_string(), - "owner/repo".to_string(), - "review".to_string(), - "check".to_string(), + start_routes(), "bogus".to_string(), TunnelSpec { mode: WebhookTunnelMode::Command, @@ -1484,15 +1571,13 @@ 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_dispatcher(Arc::new(|_, _| Box::pin(async {}))); let s = mgr .start( 0, "shh".to_string(), - "owner/repo".to_string(), - "review".to_string(), - "check".to_string(), + start_routes(), "prmonitor-no-such-cloudflared".to_string(), TunnelSpec { mode: WebhookTunnelMode::Listener, @@ -1563,15 +1648,13 @@ 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_dispatcher(Arc::new(|_, _| Box::pin(async {}))); let r = mgr .start( 0, "shh".to_string(), - "owner/repo".to_string(), - "review".to_string(), - "check".to_string(), + start_routes(), "bogus".to_string(), TunnelSpec { mode: WebhookTunnelMode::Command, @@ -1583,36 +1666,49 @@ mod tests { assert!(r.is_err(), "blank command-mode command must Err"); } - /// F2: a verified payload whose repository is NOT the configured repo must NOT map to - /// a candidate — the HMAC proves the secret is known, not that the event is for the - /// repo this app reviews. A misconfigured webhook / reused secret on another repo is - /// dropped (fail closed). + /// 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, + /// not that the event is for a repo this app reviews. A misconfigured webhook / reused + /// secret on an unmonitored repo is dropped (fail closed). #[test] fn payload_to_candidate_requires_matching_repo() { // A different `base.repo.full_name` (no top-level `repository`) → None despite a - // valid trigger label. + // 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, "owner/repo", "needs-review", "needs-check").is_none(), - "a payload for a different repo must not dispatch" + payload_to_candidate(&other, &single_route("needs-review", "needs-check")).is_none(), + "a payload for an unrouted repo must not dispatch" ); // Top-level `repository.full_name` (what GitHub actually sends) is honored and - // takes precedence: matching it admits the candidate. + // takes precedence: matching it admits the candidate (routed 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!(payload_to_candidate(&top, "owner/repo", "needs-review", "needs-check").is_some()); + assert_eq!( + payload_to_candidate(&top, &single_route("needs-review", "needs-check")) + .map(|(id, _)| id), + Some("default".to_string()) + ); - // Case-insensitive (GitHub repo-name semantics): configured `Owner/Repo` matches + // 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(&p, "Owner/Repo", "needs-review", "needs-check").is_some()); + assert!(payload_to_candidate( + &p, + &[route( + "default", + "Owner/Repo", + "needs-review", + "needs-check" + )] + ) + .is_some()); // Missing repo entirely (no top-level `repository`, no `base.repo`) → None. let no_repo = pr_payload( @@ -1620,7 +1716,61 @@ mod tests { serde_json::json!({ "base": { "repo": null } }), ); assert!( - payload_to_candidate(&no_repo, "owner/repo", "needs-review", "needs-check").is_none() + payload_to_candidate(&no_repo, &single_route("needs-review", "needs-check")).is_none() + ); + + // Empty route list (no enabled projects) → nothing can match → None. + let any = pr_payload(&["needs-review"], serde_json::json!({})); + assert!(payload_to_candidate(&any, &[]).is_none()); + } + + /// #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. + #[test] + fn payload_to_candidate_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"), + ]; + + // A payload for owner/b carrying B's review label → routed to proj-b, kind review. + let mut for_b = pr_payload(&["b-review"], serde_json::json!({})); + for_b.as_object_mut().unwrap().insert( + "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"); + assert_eq!( + project_id, "proj-b", + "routed to the matching project, not the first" + ); + assert_eq!(c.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). + 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" }), + ); + assert!( + payload_to_candidate(&wrong_label, &routes).is_none(), + "project B does not classify on project A's labels" + ); + + // 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. + 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" }), + ); + assert!( + payload_to_candidate(&for_a, &routes).is_none(), + "owner/a is classified by A's labels, not B's" ); } @@ -1712,16 +1862,14 @@ mod tests { tauri::async_runtime::block_on(async move { let mgr = WebhookManager::default(); - mgr.set_dispatcher(Arc::new(|_| Box::pin(async {}))); + mgr.set_dispatcher(Arc::new(|_, _| Box::pin(async {}))); for i in 0..3 { let s = mgr .start( port, "shh".to_string(), - "owner/repo".to_string(), - "review".to_string(), - "check".to_string(), + start_routes(), "bogus".to_string(), TunnelSpec { mode: WebhookTunnelMode::Listener, diff --git a/src-tauri/src/review/commands.rs b/src-tauri/src/review/commands.rs index 2868105..159ad96 100644 --- a/src-tauri/src/review/commands.rs +++ b/src-tauri/src/review/commands.rs @@ -12,31 +12,51 @@ use crate::state::AppState; /// `pub(crate)` const rather than re-stating the literal. pub(crate) const CODEX_BIN: &str = "codex"; +/// Rejects any review `kind` other than `review` / `check` at the command boundary. +/// +/// `session.rs` branches on `kind == "check"` and treats EVERY other value as a +/// `review` turn — so an unvalidated kind (a bogus string from a buggy/forged invoke) +/// would silently run a full review while the registry/ledger key keeps the bogus +/// kind, splitting dedup. Whitelisting here fails fast before any session starts. The +/// Hard path (future) is a shared Rust enum ↔ TS union; this is the Medium guard until +/// then. Pure (no `AppHandle`) so it is unit-testable. +fn validate_kind(kind: &str) -> AppResult<()> { + if kind == "review" || kind == "check" { + Ok(()) + } else { + Err(AppError::new(format!( + "kind 非法（只接受 review | check）: {kind:?}" + ))) + } +} + /// Reports codex app-server availability for the StatusBar. Ensures the resident /// connection (lazy start: first call spawns + handshakes, later calls reuse) and /// reports `available` + version. The probe never errors (failures map to a /// status struct); only the config read can fail. /// -/// `repo_root` (the codex cwd) is read from the config slice's public service — -/// the same cross-slice, function-level read the pr slice uses; `AppConfig` stays -/// config-private. +/// The codex app-server is GLOBAL and single (one resident process for all +/// projects); its spawn-handshake cwd is the ACTIVE project's `repo_root`, read from +/// the config slice's public service (#35). Per-turn `cwd` scopes each review's +/// working dir, so this is only the handshake cwd. `AppConfig` stays config-private. #[tauri::command] pub async fn get_codex_status( app: tauri::AppHandle, state: tauri::State<'_, AppState>, ) -> AppResult { - let cfg = config_service::load(&app)?; - Ok(state.codex.status(CODEX_BIN, &cfg.repo_root).await) + let repo_root = config_service::active_repo_root(&app)?; + Ok(state.codex.status(CODEX_BIN, &repo_root).await) } /// 显式启动常驻 codex app-server（清除「已停止」标记并拉起握手）。返回最新状态。 +/// 全局单例 codex 的握手 cwd 取「活动项目」的 `repo_root`（#35）；每轮 review 的实际工作目录由 per-turn `cwd` 覆盖。 #[tauri::command] pub async fn start_codex( app: tauri::AppHandle, state: tauri::State<'_, AppState>, ) -> AppResult { - let cfg = config_service::load(&app)?; - Ok(state.codex.start(CODEX_BIN, &cfg.repo_root).await) + let repo_root = config_service::active_repo_root(&app)?; + Ok(state.codex.start(CODEX_BIN, &repo_root).await) } /// 显式停止常驻 codex app-server（设「已停止」标记 + 杀进程；被动状态探测此后不再自动拉起，显式 review 仍会强制启动）。 @@ -46,22 +66,29 @@ pub fn stop_codex(state: tauri::State<'_, AppState>) -> AppResult { Ok(state.codex.stop()) } -/// Start a review for `pr_number` (`kind` = `"review"` or `"check"`), returning -/// the session id (codex `threadId`). Output streams out-of-band via the -/// `review:event` Tauri event ([`crate::events::ReviewEvent`]). +/// Start a review for `(project_id, pr_number)` (`kind` = `"review"` or `"check"`), +/// returning the session id (codex `threadId`). Output streams out-of-band via the +/// `review:event` Tauri event ([`crate::events::ReviewEvent`]), each event stamped +/// with `project_id` (#35) so the frontend routes it to the owning project. #[tauri::command] pub async fn start_review( app: tauri::AppHandle, state: tauri::State<'_, AppState>, + project_id: String, pr_number: u64, kind: String, ) -> AppResult { - // `load_validated` re-checks the persisted config's paths so an absent / - // escaping `skillRelPath` (e.g. a hand-edited config) fails before we attach - // the skill path to the turn, rather than handing codex a bad path. The review - // slice depends only on `config::service`, never `config::model`. - let cfg = config_service::load_validated(&app)?; - let skill_abs = skill_abs_path(&cfg.repo_root, &cfg.skill_rel_path); + // Reject a bogus `kind` BEFORE any side effect (project resolve / codex resume): + // `session.rs` treats every non-"check" value as a review, so an unvalidated kind + // would run a full review under a bad registry key (see `validate_kind`). + validate_kind(&kind)?; + // Resolve the project being reviewed (#35) and re-check ITS filesystem-dependent + // paths so an absent / escaping `skillRelPath` (e.g. a hand-edited config) fails + // before we attach the skill path to the turn, rather than handing codex a bad path. + // `project_validated` is the per-project analogue of the old `load_validated`; the + // review slice still depends only on `config::service`, never `config::model`. + let project = config_service::project_validated(&app, &project_id)?; + let skill_abs = skill_abs_path(&project.repo_root, &project.skill_rel_path); // MANUAL force-start: a user asking to review overrides a prior `stop_codex`. // `resume()` clears the user-stop flag BEFORE `engine.start()` reaches the // `connection()` funnel (which refuses when stopped). Auto-dispatch does NOT @@ -72,13 +99,14 @@ pub async fn start_review( codex: &state.codex, registry: &state.sessions, codex_bin: CODEX_BIN, - repo: &cfg.repo, - repo_root: &cfg.repo_root, + project_id: &project.id, + repo: &project.repo, + repo_root: &project.repo_root, skill_abs_path: &skill_abs, }; - // `Deduped` = the registry already has an in-flight review for this `(pr, kind)`: - // a manual re-start is a benign no-op surfaced as an error (the UI shows it; nothing - // double-starts). Stop the running one first to re-review. + // `Deduped` = the registry already has an in-flight review for this + // `(project_id, pr, kind)`: a manual re-start is a benign no-op surfaced as an error + // (the UI shows it; nothing double-starts). Stop the running one first to re-review. match engine.start(pr_number, &kind).await? { StartReviewOutcome::Started(session_id) => Ok(session_id), StartReviewOutcome::Deduped => Err(AppError::new(format!( @@ -95,16 +123,19 @@ pub async fn stop_review( state: tauri::State<'_, AppState>, session_id: String, ) -> AppResult<()> { - let cfg = config_service::load(&app)?; + // `stop` interrupts an already-live turn purely by its session id (codex + // `threadId`); it needs neither the project, the repo, nor the skill path (see + // `session::stop_review`, where `codex_bin`/`repo_root` are bound to `_`). So we + // build the engine with empty context fields and skip the config read entirely — + // a missing / invalid config must not block stopping a running review. let engine = CodexEngine { app: &app, codex: &state.codex, registry: &state.sessions, codex_bin: CODEX_BIN, - repo: &cfg.repo, - repo_root: &cfg.repo_root, - // `stop` interrupts by session id; it needs neither the repo nor the skill - // path, so we skip resolving the skill path here. + project_id: "", + repo: "", + repo_root: "", skill_abs_path: "", }; engine.stop(&session_id).await @@ -125,3 +156,22 @@ fn skill_abs_path(repo_root: &str, skill_rel_path: &str) -> String { .to_string_lossy() .into_owned() } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn validate_kind_accepts_review_and_check_only() { + assert!(validate_kind("review").is_ok()); + assert!(validate_kind("check").is_ok()); + // Any other value (incl. case variants / empty / arbitrary) is rejected so it + // can never reach `session.rs` and run as a review under a bogus key. + for bad in ["", "Review", "CHECK", "foo", "review ", "reviewcheck"] { + assert!( + validate_kind(bad).is_err(), + "expected kind {bad:?} rejected" + ); + } + } +} diff --git a/src-tauri/src/review/engines/codex/engine.rs b/src-tauri/src/review/engines/codex/engine.rs index 2b70aab..b11006b 100644 --- a/src-tauri/src/review/engines/codex/engine.rs +++ b/src-tauri/src/review/engines/codex/engine.rs @@ -19,6 +19,10 @@ pub struct CodexEngine<'a, R: tauri::Runtime> { pub registry: &'a SessionRegistry, /// The codex binary name (PATH-resolved; matches `get_codex_status`). pub codex_bin: &'a str, + /// Owning project id (#35): scopes the registry reservation / dedup and stamps + /// every streamed `ReviewEvent` so the frontend attributes it to the right + /// project. The composition root (lib.rs) sets it from the project being acted on. + pub project_id: &'a str, /// Monitored repo `owner/name` (named in the review prompt). pub repo: &'a str, /// Absolute local clone path codex runs the skill against (the turn cwd). @@ -37,6 +41,7 @@ impl ReviewEngine for CodexEngine<'_, R> { self.repo, self.repo_root, self.skill_abs_path, + self.project_id, pr_number, kind, ) diff --git a/src-tauri/src/review/session.rs b/src-tauri/src/review/session.rs index 3314b05..28c062e 100644 --- a/src-tauri/src/review/session.rs +++ b/src-tauri/src/review/session.rs @@ -55,6 +55,11 @@ pub enum SessionStatus { #[derive(Debug, Clone, Serialize)] #[serde(rename_all = "camelCase")] pub struct SessionInfo { + /// Owning project (#35): the routing key the UI filters its session list by. + /// A PR number is unique only *within* a project, so a session is identified + /// to the user by `(project_id, pr_number, kind)` — `thread_id` stays the + /// globally-unique registry key (codex assigns one per `thread/start`). + pub project_id: String, pub thread_id: String, pub turn_id: String, pub pr_number: u64, @@ -72,17 +77,23 @@ pub struct SessionRegistry { inner: Arc>, } -/// The registry's single critical section: the session map AND the set of `(pr, kind)` -/// pairs RESERVED by an in-flight [`start_review`] that has not yet inserted its -/// `Starting` session. Both live under ONE mutex, so reserve / promote-to-session / -/// [`SessionRegistry::active_pairs`] are mutually atomic — the reservation closes the -/// window where a session exists conceptually (its `thread/start` is mid-flight) but -/// isn't yet in `sessions`, the gap the old snapshot-then-act guard could not see (two -/// concurrent webhook deliveries both passing an empty snapshot → double review). +/// The registry's single critical section: the session map AND the set of +/// `(project_id, pr, kind)` triples RESERVED by an in-flight [`start_review`] that has +/// not yet inserted its `Starting` session. Both live under ONE mutex, so reserve / +/// promote-to-session / [`SessionRegistry::active_pairs`] are mutually atomic — the +/// reservation closes the window where a session exists conceptually (its +/// `thread/start` is mid-flight) but isn't yet in `sessions`, the gap the old +/// snapshot-then-act guard could not see (two concurrent webhook deliveries both +/// passing an empty snapshot → double review). +/// +/// The reservation key carries `project_id` (#35): a PR number is unique only within a +/// project, so a PR #7 review in project A must NOT dedup against a PR #7 review in +/// project B. `sessions` stays keyed by `thread_id` alone — codex assigns a globally +/// unique `threadId` per `thread/start`, so it needs no project dimension. #[derive(Default)] struct RegistryState { sessions: HashMap, - reserved: HashSet<(u64, String)>, + reserved: HashSet<(String, u64, String)>, } /// Outcome of [`SessionRegistry::begin_interrupt`] — the atomic guard that makes @@ -124,29 +135,37 @@ impl SessionRegistry { } } - /// Atomically reserve `(pr_number, kind)` for a dispatch about to start a review, - /// BEFORE the async `thread/start` — so the pair is visible to a concurrent - /// dispatch's guard the instant this returns, not only after the `Starting` insert. - /// `true` = the caller now OWNS the reservation; `false` = the pair is already - /// covered (a prior reservation OR an in-flight session), so the caller must NOT - /// start and must NOT release (it owns nothing). One synchronous critical section - /// against the same mutex as `sessions`, so two concurrent reservations for the - /// same pair cannot both win (test-and-set) — this is what makes the idempotency - /// boundary atomic rather than a snapshot. - pub fn try_reserve_pair(&self, pr_number: u64, kind: &str) -> bool { + /// Atomically reserve `(project_id, pr_number, kind)` for a dispatch about to start + /// a review, BEFORE the async `thread/start` — so the triple is visible to a + /// concurrent dispatch's guard the instant this returns, not only after the + /// `Starting` insert. `true` = the caller now OWNS the reservation; `false` = the + /// triple is already covered (a prior reservation OR an in-flight session), so the + /// caller must NOT start and must NOT release (it owns nothing). One synchronous + /// critical section against the same mutex as `sessions`, so two concurrent + /// reservations for the same triple cannot both win (test-and-set) — this is what + /// makes the idempotency boundary atomic rather than a snapshot. `project_id` scopes + /// the dedup (#35): the same PR number in two different projects reserves + /// independently. + pub fn try_reserve_pair(&self, project_id: &str, pr_number: u64, kind: &str) -> bool { let mut st = self.inner.lock().unwrap(); let covered_by_session = st.sessions.values().any(|s| { - s.pr_number == pr_number + s.project_id == project_id + && s.pr_number == pr_number && s.kind == kind && matches!( s.status, SessionStatus::Starting | SessionStatus::Running | SessionStatus::Interrupting ) }); - if covered_by_session || st.reserved.contains(&(pr_number, kind.to_string())) { + if covered_by_session + || st + .reserved + .contains(&(project_id.to_string(), pr_number, kind.to_string())) + { return false; } - st.reserved.insert((pr_number, kind.to_string())); + st.reserved + .insert((project_id.to_string(), pr_number, kind.to_string())); true } @@ -154,13 +173,14 @@ impl SessionRegistry { /// reach a `Starting` session (a `thread/start` failure / early return / panic /// before the insert). A successful start hands the reservation to the inserted /// session via [`Self::promote_reservation`], so the happy path never calls this. - /// Idempotent (a missing pair is a no-op). - fn release_pair(&self, pr_number: u64, kind: &str) { - self.inner - .lock() - .unwrap() - .reserved - .remove(&(pr_number, kind.to_string())); + /// Idempotent (a missing triple is a no-op). Keyed by the full + /// `(project_id, pr, kind)` so it frees exactly the triple `try_reserve_pair` took. + fn release_pair(&self, project_id: &str, pr_number: u64, kind: &str) { + self.inner.lock().unwrap().reserved.remove(&( + project_id.to_string(), + pr_number, + kind.to_string(), + )); } /// Insert the just-started session as `Starting` AND drop its reservation in ONE @@ -170,7 +190,8 @@ impl SessionRegistry { /// in-flight session. fn promote_reservation(&self, info: SessionInfo) { let mut st = self.inner.lock().unwrap(); - st.reserved.remove(&(info.pr_number, info.kind.clone())); + st.reserved + .remove(&(info.project_id.clone(), info.pr_number, info.kind.clone())); st.sessions.insert(info.thread_id.clone(), info); } @@ -217,31 +238,43 @@ impl SessionRegistry { } /// The `(pr_number, kind)` of every in-flight session - /// (`Starting`/`Running`/`Interrupting`) PLUS every RESERVED pair — the - /// auto-trigger registry guard's view. A PR with an in-flight (or reserved) session - /// of a given kind must not be re-dispatched; a terminal (`Done`/`Failed`) session - /// is finished and excluded. Including reservations is what lets a not-yet-`Starting` - /// dispatch still block a concurrent one. Owning the "what counts as active" rule - /// here keeps [`SessionStatus`] inside the review slice — the composition-layer - /// dispatcher consumes only the pairs, so it never imports the session state machine. - pub fn active_pairs(&self) -> Vec<(u64, String)> { + /// (`Starting`/`Running`/`Interrupting`) PLUS every RESERVED triple **belonging to + /// `project_id`** — the auto-trigger registry guard's view, scoped to one project + /// (#35). A PR with an in-flight (or reserved) session of a given kind must not be + /// re-dispatched *within the same project*; a PR #7 in project A does NOT block a PR + /// #7 in project B. A terminal (`Done`/`Failed`) session is finished and excluded. + /// Including reservations is what lets a not-yet-`Starting` dispatch still block a + /// concurrent one. Owning the "what counts as active" rule here keeps + /// [`SessionStatus`] inside the review slice — the composition-layer dispatcher + /// consumes only the pairs, so it never imports the session state machine. The + /// returned pairs drop the project dimension because the caller already scopes its + /// candidate batch to this project. + pub fn active_pairs(&self, project_id: &str) -> Vec<(u64, String)> { let st = self.inner.lock().unwrap(); let mut pairs: Vec<(u64, String)> = st .sessions .values() .filter(|s| { - matches!( - s.status, - SessionStatus::Starting | SessionStatus::Running | SessionStatus::Interrupting - ) + s.project_id == project_id + && matches!( + s.status, + SessionStatus::Starting + | SessionStatus::Running + | SessionStatus::Interrupting + ) }) .map(|s| (s.pr_number, s.kind.clone())) .collect(); - // A reserved pair has no session yet (its `thread/start` is mid-flight) but is - // every bit as "in flight" — include it so the dispatch guard and a concurrent - // reserve both see it. A duplicate vs a just-promoted session is harmless (the - // guard does set membership, not counting). - pairs.extend(st.reserved.iter().cloned()); + // A reserved triple has no session yet (its `thread/start` is mid-flight) but is + // every bit as "in flight" — include it (scoped to this project) so the dispatch + // guard and a concurrent reserve both see it. A duplicate vs a just-promoted + // session is harmless (the guard does set membership, not counting). + pairs.extend( + st.reserved + .iter() + .filter(|(pid, _, _)| pid == project_id) + .map(|(_, pr, kind)| (*pr, kind.clone())), + ); pairs } } @@ -254,6 +287,7 @@ impl SessionRegistry { /// unrepresentable, not merely hand-avoided on each return path. struct ReservationGuard<'a> { registry: &'a SessionRegistry, + project_id: String, pr_number: u64, kind: String, armed: bool, @@ -269,7 +303,8 @@ impl ReservationGuard<'_> { impl Drop for ReservationGuard<'_> { fn drop(&mut self) { if self.armed { - self.registry.release_pair(self.pr_number, &self.kind); + self.registry + .release_pair(&self.project_id, self.pr_number, &self.kind); } } } @@ -292,14 +327,17 @@ pub async fn start_review( repo: &str, repo_root: &str, skill_abs_path: &str, + project_id: &str, pr_number: u64, kind: &str, ) -> AppResult { - // Atomic test-and-set BEFORE any `.await`: if this `(pr, kind)` is already reserved - // or covered by an in-flight session, do NOT start a second review. This is the - // idempotency boundary — atomic, not the old snapshot-then-act guard that two - // concurrent webhook deliveries could both pass before either's `Starting` landed. - if !registry.try_reserve_pair(pr_number, kind) { + // Atomic test-and-set BEFORE any `.await`: if this `(project_id, pr, kind)` is + // already reserved or covered by an in-flight session, do NOT start a second review. + // This is the idempotency boundary — atomic, not the old snapshot-then-act guard + // that two concurrent webhook deliveries could both pass before either's `Starting` + // landed. `project_id` scopes the dedup (#35) so the same PR in two projects starts + // independently. + if !registry.try_reserve_pair(project_id, pr_number, kind) { return Ok(StartReviewOutcome::Deduped); } // From here, ANY early return / `?` / panic before `promote_reservation` releases @@ -307,6 +345,7 @@ pub async fn start_review( // it to the inserted session instead. let reservation = ReservationGuard { registry, + project_id: project_id.to_string(), pr_number, kind: kind.to_string(), armed: true, @@ -332,6 +371,7 @@ pub async fn start_review( // `turn/start` failure flips it to `Failed` (visible to `list_review_sessions`, // not vanished); `turn_id` is filled once the turn starts. registry.promote_reservation(SessionInfo { + project_id: project_id.to_string(), thread_id: thread_id.clone(), turn_id: String::new(), pr_number, @@ -375,7 +415,18 @@ pub async fn start_review( registry.set_running(&thread_id, turn_id); - tauri::async_runtime::spawn(pump(rx, thread_id.clone(), app.clone(), registry.clone())); + // Capture `project_id` as an owned String at spawn time so the pump stamps every + // emitted `ReviewEvent` with it WITHOUT re-looking-up the session per event (#35): + // the routing key is fixed for the session's life, and a lookup would also race the + // terminal removal. The pump filters the shared notification stream by `thread_id` + // but carries `project_id` to attribute each delta to the owning project. + tauri::async_runtime::spawn(pump( + rx, + project_id.to_string(), + thread_id.clone(), + app.clone(), + registry.clone(), + )); Ok(StartReviewOutcome::Started(thread_id)) } @@ -390,6 +441,11 @@ pub async fn start_review( /// app-server (PR #47 F2). The params stay in the signature because the /// `ReviewEngine` impl (`engine.rs`) passes them; Rust does not lint unused fn /// params, so this is clippy-clean. +/// +/// Keys on `session_id` (the codex `threadId`) ONLY — never reads any per-project +/// config — so the command builds its `CodexEngine` with `project_id: ""` (and empty +/// repo/skill fields, `commands.rs::stop_review`). A missing / invalid config must not +/// be able to block stopping a running review. pub async fn stop_review( codex: &CodexManager, registry: &SessionRegistry, @@ -443,9 +499,12 @@ pub async fn stop_review( /// Pump task: forward this session's notifications to the frontend as /// [`ReviewEvent`]s until the turn completes (or the connection drops). Filters by -/// `thread_id` since the broadcast carries every session's stream. +/// `thread_id` since the broadcast carries every session's stream; stamps every +/// emitted event with `project_id` (#35), the owning-project routing key captured at +/// spawn (fixed for the session's life — never re-looked-up per event). async fn pump( mut rx: broadcast::Receiver>, + project_id: String, thread_id: String, app: tauri::AppHandle, registry: SessionRegistry, @@ -459,11 +518,11 @@ async fn pump( // `Sender` alive across a dead reader, so `RecvError::Closed` below never // fires for a process-death; this is what catches that case). Ok(note) if matches!(note.as_ref(), ServerNotification::ConnectionClosed) => { - fail_connection_closed(®istry, &app, &thread_id); + fail_connection_closed(®istry, &app, &project_id, &thread_id); break; } Ok(note) => { - let Some(event) = map_notification(¬e, &thread_id) else { + let Some(event) = map_notification(¬e, &project_id, &thread_id) else { continue; }; if let ReviewEvent::TurnCompleted { status, .. } = &event { @@ -485,6 +544,7 @@ async fn pump( let _ = app.emit( REVIEW_EVENT, &ReviewEvent::Error { + project_id: project_id.clone(), thread_id: thread_id.clone(), message: format!("codex 输出流滞后，丢弃 {n} 条消息（review 中断）"), }, @@ -495,7 +555,7 @@ async fn pump( // `RpcClient` was torn down, e.g. manager shutdown). Same terminal // outcome as the synthetic `ConnectionClosed` above. Err(broadcast::error::RecvError::Closed) => { - fail_connection_closed(®istry, &app, &thread_id); + fail_connection_closed(®istry, &app, &project_id, &thread_id); break; } } @@ -509,12 +569,14 @@ async fn pump( fn fail_connection_closed( registry: &SessionRegistry, app: &tauri::AppHandle, + project_id: &str, thread_id: &str, ) { registry.set_status(thread_id, SessionStatus::Failed); let _ = app.emit( REVIEW_EVENT, &ReviewEvent::Error { + project_id: project_id.to_string(), thread_id: thread_id.to_string(), message: "codex 连接已关闭".to_string(), }, @@ -522,12 +584,18 @@ fn fail_connection_closed( } /// Map one codex notification to a [`ReviewEvent`] for this session, or `None` -/// if it belongs to another thread / is not a streamed unit we forward. Pure — -/// unit-tested below. -fn map_notification(note: &ServerNotification, thread_id: &str) -> Option { +/// if it belongs to another thread / is not a streamed unit we forward. Every +/// produced event is stamped with `project_id` (#35), the owning-project routing key +/// the pump captured at spawn. Pure — unit-tested below. +fn map_notification( + note: &ServerNotification, + project_id: &str, + thread_id: &str, +) -> Option { match note { ServerNotification::AgentMessageDelta(d) if d.thread_id == thread_id => { Some(ReviewEvent::MessageDelta { + project_id: project_id.to_string(), thread_id: d.thread_id.clone(), item_id: d.item_id.clone(), text: d.delta.clone(), @@ -535,6 +603,7 @@ fn map_notification(note: &ServerNotification, thread_id: &str) -> Option { Some(ReviewEvent::ReasoningDelta { + project_id: project_id.to_string(), thread_id: d.thread_id.clone(), item_id: d.item_id.clone(), text: d.delta.clone(), @@ -542,6 +611,7 @@ fn map_notification(note: &ServerNotification, thread_id: &str) -> Option { Some(ReviewEvent::TurnCompleted { + project_id: project_id.to_string(), thread_id: d.thread_id.clone(), status: d.turn.status.clone(), }) @@ -603,12 +673,15 @@ mod tests { #[test] fn map_notification_forwards_own_thread_message_delta() { - match map_notification(&msg_delta("t1"), "t1") { + match map_notification(&msg_delta("t1"), "p1", "t1") { Some(ReviewEvent::MessageDelta { + project_id, thread_id, item_id, text, }) => { + // The pump stamps the captured owning-project id on every event (#35). + assert_eq!(project_id, "p1"); assert_eq!(thread_id, "t1"); assert_eq!(item_id, "it"); assert_eq!(text, "hello"); @@ -620,7 +693,7 @@ mod tests { #[test] fn map_notification_drops_other_thread() { // A delta for a different session must not leak into this pump. - assert!(map_notification(&msg_delta("other"), "t1").is_none()); + assert!(map_notification(&msg_delta("other"), "p1", "t1").is_none()); } #[test] @@ -634,13 +707,13 @@ mod tests { delta_base64: "dGhl".to_string(), cap_reached: false, }); - assert!(map_notification(&output, "t1").is_none()); + assert!(map_notification(&output, "p1", "t1").is_none()); let other = ServerNotification::Other { method: "thread/futureThing".to_string(), params: serde_json::json!({}), }; - assert!(map_notification(&other, "t1").is_none()); + assert!(map_notification(&other, "p1", "t1").is_none()); } #[test] @@ -652,7 +725,7 @@ mod tests { delta: "thinking".to_string(), }); assert!(matches!( - map_notification(&n, "t1"), + map_notification(&n, "p1", "t1"), Some(ReviewEvent::ReasoningDelta { .. }) )); } @@ -665,8 +738,13 @@ mod tests { status: "interrupted".to_string(), }, }); - match map_notification(&n, "t1") { - Some(ReviewEvent::TurnCompleted { status, .. }) => assert_eq!(status, "interrupted"), + match map_notification(&n, "p1", "t1") { + Some(ReviewEvent::TurnCompleted { + project_id, status, .. + }) => { + assert_eq!(project_id, "p1"); + assert_eq!(status, "interrupted"); + } other => panic!("expected TurnCompleted, got {other:?}"), } } @@ -699,6 +777,7 @@ mod tests { fn registry_insert_status_and_list_roundtrip() { let reg = SessionRegistry::default(); reg.insert(SessionInfo { + project_id: "p1".to_string(), thread_id: "t1".to_string(), turn_id: "tn1".to_string(), pr_number: 7, @@ -721,6 +800,7 @@ mod tests { let reg = SessionRegistry::default(); let info = |thread: &str, pr: u64, kind: &str, status| { reg.insert(SessionInfo { + project_id: "p1".to_string(), thread_id: thread.to_string(), turn_id: String::new(), pr_number: pr, @@ -734,7 +814,7 @@ mod tests { info("d", 4, "review", SessionStatus::Done); // terminal → excluded info("e", 5, "check", SessionStatus::Failed); // terminal → excluded - let mut pairs = reg.active_pairs(); + let mut pairs = reg.active_pairs("p1"); pairs.sort(); assert_eq!( pairs, @@ -749,20 +829,23 @@ mod tests { #[test] fn try_reserve_pair_is_atomic_test_and_set() { let reg = SessionRegistry::default(); - assert!(reg.try_reserve_pair(7, "review"), "first reservation wins"); assert!( - !reg.try_reserve_pair(7, "review"), + reg.try_reserve_pair("p1", 7, "review"), + "first reservation wins" + ); + assert!( + !reg.try_reserve_pair("p1", 7, "review"), "second is rejected while reserved" ); // A reservation shows up in active_pairs BEFORE any Starting session exists — // exactly the gap the old snapshot-then-act guard could not see. - assert!(reg.active_pairs().contains(&(7, "review".to_string()))); - // A different kind for the same PR is independent (key is (pr, kind)). - assert!(reg.try_reserve_pair(7, "check")); + assert!(reg.active_pairs("p1").contains(&(7, "review".to_string()))); + // A different kind for the same PR is independent (key is (project, pr, kind)). + assert!(reg.try_reserve_pair("p1", 7, "check")); // Release frees it for a later cycle. - reg.release_pair(7, "review"); + reg.release_pair("p1", 7, "review"); assert!( - reg.try_reserve_pair(7, "review"), + reg.try_reserve_pair("p1", 7, "review"), "reservable again after release" ); } @@ -772,23 +855,86 @@ mod tests { let reg = SessionRegistry::default(); // An in-flight (Running) session covers the pair even with no reservation. reg.insert(SessionInfo { + project_id: "p1".to_string(), thread_id: "t1".to_string(), turn_id: "tn".to_string(), pr_number: 7, kind: "review".to_string(), status: SessionStatus::Running, }); - assert!(!reg.try_reserve_pair(7, "review")); + assert!(!reg.try_reserve_pair("p1", 7, "review")); // A different kind is still reservable; a terminal session would not block // (covered by the active_pairs in-flight filter, exercised elsewhere). - assert!(reg.try_reserve_pair(7, "check")); + assert!(reg.try_reserve_pair("p1", 7, "check")); + } + + #[test] + fn reservations_and_active_pairs_are_isolated_per_project() { + // #35: a PR number is unique only WITHIN a project. The same `(pr, kind)` in two + // projects must reserve independently, and `active_pairs` must scope to its + // project — a PR #7 review in project A must never block PR #7 in project B, + // nor leak into B's active-pairs snapshot. + let reg = SessionRegistry::default(); + assert!(reg.try_reserve_pair("A", 7, "review"), "A reserves freely"); + assert!( + reg.try_reserve_pair("B", 7, "review"), + "B reserves the same (pr, kind) independently of A" + ); + // Re-reserving within the SAME project still dedups (the within-project guard). + assert!( + !reg.try_reserve_pair("A", 7, "review"), + "dedup within a project is preserved" + ); + + // Each project's active_pairs sees ONLY its own reservation. + assert_eq!(reg.active_pairs("A"), vec![(7, "review".to_string())]); + assert_eq!(reg.active_pairs("B"), vec![(7, "review".to_string())]); + assert!( + reg.active_pairs("C").is_empty(), + "a project with nothing in flight sees an empty snapshot" + ); + + // An in-flight SESSION (not just a reservation) is likewise project-scoped: + // A's promoted session does not appear in B's active_pairs and does not block B. + reg.promote_reservation(SessionInfo { + project_id: "A".to_string(), + thread_id: "tA".to_string(), + turn_id: String::new(), + pr_number: 9, + kind: "review".to_string(), + status: SessionStatus::Running, + }); + assert!( + reg.active_pairs("A").contains(&(9, "review".to_string())), + "A's session shows in A" + ); + assert!( + !reg.active_pairs("B").contains(&(9, "review".to_string())), + "A's session must not leak into B" + ); + assert!( + reg.try_reserve_pair("B", 9, "review"), + "A's in-flight (9, review) session does not block B's (9, review)" + ); + + // Releasing A's reservation leaves B's untouched (full-triple keying). + reg.release_pair("A", 7, "review"); + assert!( + reg.try_reserve_pair("A", 7, "review"), + "A reservable again after its own release" + ); + assert!( + !reg.try_reserve_pair("B", 7, "review"), + "B's reservation was not disturbed by A's release" + ); } #[test] fn promote_reservation_hands_off_without_a_gap() { let reg = SessionRegistry::default(); - assert!(reg.try_reserve_pair(7, "review")); + assert!(reg.try_reserve_pair("p1", 7, "review")); reg.promote_reservation(SessionInfo { + project_id: "p1".to_string(), thread_id: "t1".to_string(), turn_id: String::new(), pr_number: 7, @@ -797,9 +943,9 @@ mod tests { }); // After promotion the pair is covered by the Starting session, not the reserved // set — and a concurrent reserve still loses (continuous coverage, no gap). - assert!(!reg.try_reserve_pair(7, "review")); + assert!(!reg.try_reserve_pair("p1", 7, "review")); // The reservation was CONSUMED, not double-counted: exactly one active pair. - let pairs = reg.active_pairs(); + let pairs = reg.active_pairs("p1"); assert_eq!( pairs .iter() @@ -822,7 +968,7 @@ mod tests { let reg = reg.clone(); let winners = Arc::clone(&winners); handles.push(tokio::spawn(async move { - if reg.try_reserve_pair(7, "review") { + if reg.try_reserve_pair("p1", 7, "review") { winners.fetch_add(1, Ordering::SeqCst); } })); @@ -841,6 +987,7 @@ mod tests { fn begin_interrupt_is_atomic_and_idempotent() { let reg = SessionRegistry::default(); reg.insert(SessionInfo { + project_id: "p1".to_string(), thread_id: "t1".to_string(), turn_id: "tn1".to_string(), pr_number: 7, @@ -869,6 +1016,7 @@ mod tests { fn rollback_interrupt_reverts_only_interrupting() { let reg = SessionRegistry::default(); reg.insert(SessionInfo { + project_id: "p1".to_string(), thread_id: "t1".to_string(), turn_id: "tn1".to_string(), pr_number: 7, @@ -888,8 +1036,11 @@ mod tests { #[test] fn session_info_wire_shape_is_camel_case() { - // Locks the contract with `src/review/types.ts` (Medium carrier). + // Locks the contract with `src/review/types.ts` (Medium carrier). The + // `projectId` routing key (#35) must serialize camelCase and mirror + // `ReviewSession.projectId` on the TS side; snake_case must stay absent. let v = serde_json::to_value(SessionInfo { + project_id: "p1".to_string(), thread_id: "t1".to_string(), turn_id: "tn1".to_string(), pr_number: 7, @@ -897,10 +1048,16 @@ mod tests { status: SessionStatus::Running, }) .expect("SessionInfo serializes"); + assert_eq!(v["projectId"], "p1"); assert_eq!(v["threadId"], "t1"); assert_eq!(v["turnId"], "tn1"); assert_eq!(v["prNumber"], 7); + // `kind` ("review"/"check") is a frontend contract field (mirrored by + // `ReviewSession.kind` in `src/review/types.ts`); pin it so a rename / drop + // surfaces here in lockstep with the camelCase keys. + assert_eq!(v["kind"], "review"); assert_eq!(v["status"], "running"); + assert!(v.get("project_id").is_none()); assert!(v.get("thread_id").is_none()); } @@ -925,6 +1082,7 @@ mod tests { // `Failed` if the turn never starts (so it stays visible, not vanished). let reg = SessionRegistry::default(); reg.insert(SessionInfo { + project_id: "p1".to_string(), thread_id: "t1".to_string(), turn_id: String::new(), pr_number: 7, diff --git a/src-tauri/src/state.rs b/src-tauri/src/state.rs index ed2fb51..7755a88 100644 --- a/src-tauri/src/state.rs +++ b/src-tauri/src/state.rs @@ -5,8 +5,11 @@ #[derive(Default)] pub struct AppState { - /// The scheduled-pull loop handle (PR4). Long-lived; methods take `&self`. - pub scheduler: crate::pr::scheduler::Scheduler, + /// The per-project scheduled-pull loops (#35): a `project_id → Scheduler` set the + /// composition root reconciles to the enabled projects. Long-lived; methods take + /// `&self`. Pre-#35 this was a single `Scheduler`; now one app drives N parallel + /// poll loops, one per enabled project. + pub scheduler: crate::pr::scheduler::SchedulerSet, /// The resident codex app-server connection (PR5). Lazily started, kept alive /// so reviews start fast; killed on app shutdown. Methods take `&self`. pub codex: crate::review::engines::codex::CodexManager, diff --git a/src/App.vue b/src/App.vue index bdc02df..0db7391 100644 --- a/src/App.vue +++ b/src/App.vue @@ -2,13 +2,14 @@ // Composition-root layout: wires the slice views together and switches between the // monitor / settings / onboarding views. Slices own their own UI + state; App only // arranges them. -import { computed, onMounted, ref } from "vue"; +import { computed, onMounted, ref, watch } from "vue"; import { appVersion } from "./config/api"; import { useConfigStore } from "./config/useConfigStore"; import SettingsView from "./config/SettingsView.vue"; import OnboardingWizard from "./config/OnboardingWizard.vue"; import PollControls from "./pr/PollControls.vue"; import PrList from "./pr/PrList.vue"; +import ProjectSwitcher from "./pr/ProjectSwitcher.vue"; import WebhookPanel from "./pr/WebhookPanel.vue"; import { usePrStore } from "./pr/usePrStore"; import StatusBar from "./StatusBar.vue"; @@ -17,6 +18,7 @@ import ReviewSessions from "./review/ReviewSessions.vue"; import { useReviewStore } from "./review/useReviewStore"; import { reschedule, startPolling } from "./pr/api"; import { useAppView } from "./useAppView"; +import { useProjects } from "./projects"; const version = ref(""); // Gate view selection until the config load resolves, so a first-launch user never @@ -26,6 +28,9 @@ const booting = ref(true); const prStore = usePrStore(); const configStore = useConfigStore(); const { currentView, goMonitor, goSettings, goOnboarding } = useAppView(); +// Active project (#35): the switcher rail flips it; the PR + review views below +// resolve their data against it. Persisted via useProjects().setActive. +const { activeProjectId } = useProjects(); // Login / availability banner (composition layer only): #8 auto-triggers reviews, // which silently stall if `gh` isn't authenticated or codex is unavailable. @@ -36,10 +41,14 @@ const { currentView, goMonitor, goSettings, goOnboarding } = useAppView(); // `dispatchError` is the session-less auto-trigger notice (#8): the backend // dispatcher emits it on a bad config / start failure / ledger-write failure, so // the same banner that warns "auto review paused" also reports "auto review failed". -const { codex, dispatchError, clearDispatchError } = useReviewStore(); +const { codex, dispatchError, clearDispatchError, clearFocus } = useReviewStore(); const ghBlocked = computed(() => prStore.gh?.authenticated === false); const codexBlocked = computed(() => codex.value?.available === false); const showPrompt = computed(() => ghBlocked.value || codexBlocked.value); +// dispatchError is keyed per project (#35): show the active project's notice. +const activeDispatchError = computed( + () => dispatchError.value[activeProjectId.value] ?? null, +); // A config LOAD failure leaves `config` null; surface it instead of silently // degrading to an empty monitor (finding F3). The user can open Settings to retry // (SettingsView re-loads on mount) or re-save. @@ -60,7 +69,14 @@ onMounted(async () => { // and forcing a returning user back through onboarding on a transient error would // be worse than degrading to the monitor view. await configStore.load(); - if (configStore.config && configStore.config.repoRoot === "") { + // Seed the project switcher + active selection from the loaded config (#35). + if (configStore.config) useProjects().hydrate(configStore.config); + // First-launch detection is now "no projects yet": migration wraps a legacy + // single-repo config into projects:[default], and onboarding creates the first + // project — so an empty `projects` is the only fresh-install state that routes to + // onboarding. A null config means the load failed (IPC error); degrade to the + // monitor rather than forcing a returning user back through onboarding. + if (configStore.config && configStore.config.projects.length === 0) { goOnboarding(); } else { goMonitor(); @@ -73,23 +89,31 @@ onMounted(async () => { // the poll interval. start_polling is idempotent (no-op if already running) and // recovers the gated-off case; reschedule then applies the new period. async function onConfigSaved() { + // A save may have added / removed / edited projects, so re-load and re-hydrate the + // switcher + active selection before reconciling the loops (#35). + await configStore.load(); + if (configStore.config) useProjects().hydrate(configStore.config); + const ids = useProjects().projects.value.map((p) => p.id); + const activeId = activeProjectId.value; try { + // start_polling reconciles ALL enabled projects' loops (idempotent); recovers a + // project whose loop was gated off at launch by a previously-invalid config. await startPolling(); - prStore.polling = true; + for (const id of ids) prStore.polling[id] = true; } catch (e) { - // start_polling now rejects under an invalid config (finding F1). Keep the flag + // start_polling rejects under an invalid config (finding F1). Keep the flags // honest and surface the error (finding F3) so a saved-but-not-running monitor is // visible — not just a console line implying the loop resumed. - prStore.polling = false; - prStore.error = toMsg(e); + for (const id of ids) prStore.polling[id] = false; + prStore.error[activeId] = toMsg(e); return; } - // The loop is running; a reschedule failure only delays the period rebuild (the + // The loops are running; a reschedule failure only delays the period rebuild (the // next poll still runs on the old period), so surface it without claiming a stop. try { await reschedule(); } catch (e) { - prStore.error = toMsg(e); + prStore.error[activeId] = toMsg(e); } } @@ -98,15 +122,21 @@ async function onConfigSaved() { // store before entering the monitor view — this is the downstream that closes the // poll-gate funnel (upstream = the lib.rs validity gate). async function onOnboardingDone() { + // Onboarding created the first project; re-load + hydrate so the switcher and the + // active selection reflect it before the monitor view mounts (#35). + await configStore.load(); + if (configStore.config) useProjects().hydrate(configStore.config); + const ids = useProjects().projects.value.map((p) => p.id); + const activeId = activeProjectId.value; try { await startPolling(); - prStore.polling = true; + for (const id of ids) prStore.polling[id] = true; } catch (e) { - // Keep the polling flag honest (PollControls won't claim the loop is running) + // Keep the polling flags honest (PollControls won't claim the loop is running) // AND surface the error (finding F3) so the user sees why the monitor didn't // start — not just a console line — and can retry from the monitor controls. - prStore.polling = false; - prStore.error = toMsg(e); + for (const id of ids) prStore.polling[id] = false; + prStore.error[activeId] = toMsg(e); } goMonitor(); } @@ -123,10 +153,18 @@ async function onOnboardingDone() { const selectedNumber = ref(null); const selectedPr = computed( () => - prStore.prs.find( + (prStore.prs[activeProjectId.value] ?? []).find( (p) => !p.archived && p.number === selectedNumber.value, ) ?? null, ); +// Switching projects clears the selection AND the focused review (#35). PR numbers +// collide across repos, so a carried-over selection could point ReviewPanel at the +// wrong PR; and the review focus is a global singleton, so without clearFocus the +// panel would keep showing/stopping the previous project's session (pr-review F5). +watch(activeProjectId, () => { + selectedNumber.value = null; + clearFocus(); +});

-

- ⚠ 自动 review 异常：{{ dispatchError }} +

+ ⚠ 自动 review 异常：{{ activeDispatchError }} diff --git a/src/config/OnboardingWizard.vue b/src/config/OnboardingWizard.vue index 2f2586c..a458b05 100644 --- a/src/config/OnboardingWizard.vue +++ b/src/config/OnboardingWizard.vue @@ -1,19 +1,23 @@ + + + + diff --git a/src/config/ProjectsManager.vue b/src/config/ProjectsManager.vue new file mode 100644 index 0000000..a1ba214 --- /dev/null +++ b/src/config/ProjectsManager.vue @@ -0,0 +1,151 @@ + + + + + diff --git a/src/config/SettingsView.vue b/src/config/SettingsView.vue index 1a1ea66..c3f92d9 100644 --- a/src/config/SettingsView.vue +++ b/src/config/SettingsView.vue @@ -1,14 +1,17 @@ @@ -130,7 +119,15 @@ async function onSave() {