diff --git a/src-tauri/src/commands/forge.rs b/src-tauri/src/commands/forge.rs index f8f39987e3..74da1200de 100644 --- a/src-tauri/src/commands/forge.rs +++ b/src-tauri/src/commands/forge.rs @@ -25,11 +25,17 @@ use crate::web::event_bridge::{emit_event, EventEmitter, WorkTaskChange, WORK_TA /// Hard cap for one reverse-lookup batch (a screen shows ~30 rows). const LOOKUP_KEYS_CAP: usize = 100; +/// The remote the panel reads when a folder has no saved selection. The +/// historical default, named once so both the resolution and its docs agree. +const DEFAULT_FORGE_REMOTE: &str = "origin"; /// Task card titles inherit the automation convention: 80 chars. const TITLE_CAP: usize = 80; #[derive(Debug, Clone, Serialize)] pub struct ForgeRemote { + /// Which remote this was resolved from — echoed back so the panel can show + /// the active choice instead of inferring it from the URL. + pub remote_name: String, pub server_host: String, pub owner_repo: String, pub remote_url: String, @@ -409,15 +415,46 @@ pub struct ForgeTaskLink { // ── shared business logic (both modes) ────────────────────────────────────── -/// The folder's `origin` remote, parsed into forge coordinates. `None` when -/// there is no origin or its URL is not a recognizable forge repo. +/// The folder's selected remote — the panel's saved choice, or `origin` when it +/// has none — parsed into forge coordinates. `None` when the folder has no such +/// remote or its URL is not a recognizable forge repo. pub async fn folder_forge_remote_core( db: &AppDatabase, folder_id: i32, +) -> Result, AppCommandError> { + // Resolving the selection HERE is what makes every forge operation follow + // it: `resolve_folder_repo` calls this, so lists, comments, merges and task + // creation all read the repository the panel is showing. + // The selection has a store of its own rather than living in the panel + // settings: the picker saves it, and a settings save — including the "use + // global defaults" drop — must not be able to take it away. See + // `forge::remotes`. + let selected = forge::remotes::load_selected(&db.conn, folder_id) + .await + .map_err(AppCommandError::db)?; + folder_remote_named( + db, + folder_id, + selected.as_deref().unwrap_or(DEFAULT_FORGE_REMOTE), + ) + .await +} + +/// One NAMED remote of a folder, parsed into forge coordinates — the same +/// mechanics as the selection above, for a remote named outright. +/// +/// Used for the folder's own `origin`, which is a fact about the working copy +/// rather than about what the panel is showing: the picker can point the panel +/// at a parent while the branch codeg can write to stays `origin`. See +/// `ForgeSourceMeta::fork_repo`. +async fn folder_remote_named( + db: &AppDatabase, + folder_id: i32, + remote_name: &str, ) -> Result, AppCommandError> { let folder = get_folder_core(db, folder_id).await?; let output = crate::process::tokio_command("git") - .args(["-C", &folder.path, "remote", "get-url", "origin"]) + .args(["-C", &folder.path, "remote", "get-url", remote_name]) .output() .await .map_err(|e| AppCommandError::io_error("failed to run git").with_detail(e.to_string()))?; @@ -430,6 +467,7 @@ pub async fn folder_forge_remote_core( }; let profile = forge::host_profile(&db.conn, &server_host).await; Ok(Some(ForgeRemote { + remote_name: remote_name.to_string(), server_host, // A GitLab mounted under a relative URL root puts that mount path in // front of every repository path a git remote carries, while no API @@ -461,6 +499,16 @@ fn redact_userinfo(url: &str) -> String { } } +/// The folder's repository, or the configuration error every caller wants. +async fn folder_forge_remote_required( + db: &AppDatabase, + folder_id: i32, +) -> Result { + folder_forge_remote_core(db, folder_id).await?.ok_or_else(|| { + AppCommandError::configuration_missing("this folder has no recognizable forge remote") + }) +} + /// Resolve the folder's repository AND the credential to read it with — the /// two things every workbench read needs and neither of which the client may /// supply. @@ -469,19 +517,56 @@ async fn resolve_folder_repo( folder_id: i32, account_id: Option<&str>, ) -> Result<(ForgeRemote, forge::ResolvedAuth), AppCommandError> { - let remote = folder_forge_remote_core(db, folder_id) - .await? - .ok_or_else(|| { - AppCommandError::configuration_missing( - "this folder has no recognizable forge remote (origin)", - ) - })?; + let remote = folder_forge_remote_required(db, folder_id).await?; let auth = forge::resolve_forge_auth(&db.conn, remote.provider, &remote.server_host, account_id) .await?; Ok((remote, auth)) } +/// Resolve for a WRITE, whose caller also says which repository it believed it +/// was writing to. +/// +/// The belief is checked BEFORE the credential is looked up: a stale panel is +/// told its coordinates are stale — a fact it can act on — rather than about +/// an account it never asked for. A caller that names no coordinates (an older +/// client, or one with nothing readable on screen) is resolved exactly as +/// before, so nothing that worked stops working. +async fn resolve_folder_repo_for_write( + db: &AppDatabase, + folder_id: i32, + account_id: Option<&str>, + expected: Option<(&str, &str)>, +) -> Result<(ForgeRemote, forge::ResolvedAuth), AppCommandError> { + let remote = folder_forge_remote_required(db, folder_id).await?; + if let Some((host, repo)) = expected { + if remote.server_host != host || !forge::same_repo(&remote.owner_repo, repo) { + return Err(write_mismatch(&remote, host, repo)); + } + } + let auth = + forge::resolve_forge_auth(&db.conn, remote.provider, &remote.server_host, account_id) + .await?; + Ok((remote, auth)) +} + +/// The refusal a stale write gets. Carries the i18n key the panel recognises +/// so it can re-resolve instead of leaving the reader on a repository the +/// folder has already left — the same judgement the trigger path makes, in +/// words that fit a comment, a close or a merge (see the key's own note). +fn write_mismatch(remote: &ForgeRemote, expected_host: &str, expected_repo: &str) -> AppCommandError { + let actual = format!("{}/{}", remote.server_host, remote.owner_repo); + let expected = format!("{expected_host}/{expected_repo}"); + let mut params = std::collections::BTreeMap::new(); + params.insert("expected".to_string(), expected.clone()); + params.insert("actual".to_string(), actual.clone()); + AppCommandError::configuration_invalid(format!( + "this panel was showing {expected}, but the folder's remote is now {actual} — the \ + write was refused rather than sent to the wrong repository" + )) + .with_i18n(forge::WRITE_MISMATCH_I18N_KEY, params) +} + pub async fn forge_list_issues_core( db: &AppDatabase, folder_id: i32, @@ -594,7 +679,9 @@ pub async fn forge_create_comment_core( draft: forge::CommentDraft, ) -> Result { let (kind, number, body) = draft.resolve().map_err(AppCommandError::from)?; - let (remote, auth) = resolve_folder_repo(db, folder_id, draft.account_id.as_deref()).await?; + let (remote, auth) = + resolve_folder_repo_for_write(db, folder_id, draft.account_id.as_deref(), draft.expected.pair()) + .await?; Ok(match remote.provider { // No kind: a pull request IS an issue at GitHub, and one endpoint // serves both (`/pulls/{n}/comments` is the review-comment collection, @@ -624,7 +711,13 @@ pub async fn forge_set_item_state_core( request: forge::StateChangeRequest, ) -> Result { let (kind, number, action) = request.resolve().map_err(AppCommandError::from)?; - let (remote, auth) = resolve_folder_repo(db, folder_id, request.account_id.as_deref()).await?; + let (remote, auth) = resolve_folder_repo_for_write( + db, + folder_id, + request.account_id.as_deref(), + request.expected.pair(), + ) + .await?; Ok(match remote.provider { ForgeProvider::GitHub => { forge::github::set_item_state(&auth, &remote.owner_repo, kind, number, action).await? @@ -649,7 +742,9 @@ pub async fn forge_create_issue_core( draft: forge::NewIssueDraft, ) -> Result { let resolved = draft.resolve().map_err(AppCommandError::from)?; - let (remote, auth) = resolve_folder_repo(db, folder_id, draft.account_id.as_deref()).await?; + let (remote, auth) = + resolve_folder_repo_for_write(db, folder_id, draft.account_id.as_deref(), draft.expected.pair()) + .await?; Ok(match remote.provider { ForgeProvider::GitHub => { forge::github::create_issue(&auth, &remote.owner_repo, &resolved).await? @@ -777,7 +872,13 @@ pub async fn forge_merge_change_core( request: forge::ChangeMergeRequest, ) -> Result, AppCommandError> { let (number, method, head_sha) = request.resolve().map_err(AppCommandError::from)?; - let (remote, auth) = resolve_folder_repo(db, folder_id, request.account_id.as_deref()).await?; + let (remote, auth) = resolve_folder_repo_for_write( + db, + folder_id, + request.account_id.as_deref(), + request.expected.pair(), + ) + .await?; let head_sha = head_sha.as_deref(); Ok(match remote.provider { ForgeProvider::GitHub => { @@ -879,6 +980,59 @@ pub async fn work_task_create_from_forge_core( None }; + // ── Which repository the WORK will be pushed to ───────────────────────── + // + // The panel's selection names the repository being READ (the parent, in the + // fork workflow: `origin` is the contributor's own copy); the folder's own + // `origin` names the one codeg can WRITE to. Both are known only HERE — + // delivery runs long after the panel may have moved on, and the remote list + // is the folder's mutable state, so the answer is recorded on the task. + let origin_repo = folder_remote_named(db, draft.folder_id, DEFAULT_FORGE_REMOTE) + .await? + // Only a remote on the SAME host is a candidate: the push spends this + // task's credential, which belongs to `server_host`. + .filter(|remote| remote.server_host == server_host) + .map(|remote| remote.owner_repo); + let fork_repo = origin_repo + .clone() + .filter(|repo| !forge::same_repo(repo, &owner_repo)); + + // GitLab spells a cross-project merge request with project IDs rather than a + // qualified head ref — BOTH ends of it — so they are resolved HERE, while + // the user can still be told, and recorded on the task. The fork's id is the + // project the merge request is created ON, and it is also how a retry + // recognises its own merge request again: the list payload names a foreign + // source project by number alone (see `ForgePr::with_resolved_head`). The + // target's id goes in the body, because GitLab does not infer it from the + // fork's upstream — a create without it lands on the fork itself. + let mut fork_project_id = None; + let mut owner_project_id = None; + if let Some(fork) = fork_repo.as_deref() { + if provider == ForgeProvider::GitLab { + let resolved = forge::gitlab::resolve_project_id(&auth, fork).await; + fork_project_id = Some(gitlab_project_id( + resolved, + GitLabEnd::Fork, + fork, + &owner_repo, + )?); + let resolved = forge::gitlab::resolve_project_id(&auth, &owner_repo).await; + owner_project_id = Some(gitlab_project_id( + resolved, + GitLabEnd::Target, + fork, + &owner_repo, + )?); + } + } + + // A review whose head is somebody else's fork is deliberately NOT refused + // here. Whether this account may write into that fork is a server-side fact + // — the author's "allow edits from maintainers", which codeg cannot read up + // front — and a gate here would close a channel that works for exactly the + // people this panel is for (a maintainer pushing a fix into a contributor's + // review). The push decides instead, and its refusal names the way out. + let key = forge::source_key( provider.as_str(), &server_host, @@ -913,6 +1067,9 @@ pub async fn work_task_create_from_forge_core( // the task is queued cannot silently change what gets worked on. head_sha: pull.as_ref().map(|p| p.head_sha.clone()), head_repo: pull.as_ref().map(|p| p.head_repo.clone()), + fork_repo, + fork_project_id, + owner_project_id, result_pr: None, // Always stamped explicitly, both answers: the engine reads it as the // user's decision, and an absent field there means "an older row that @@ -1031,6 +1188,28 @@ pub async fn forge_settings_set_core( Ok(forge::settings::save(&db.conn, folder_id, settings).await?) } +/// Every folder's remote selection at once — what the picker reads. Held whole +/// for the same reason the panel settings are: switching folders costs no round +/// trip, and a selection that no longer resolves is still shown for what the +/// folder is set to. +pub async fn forge_remote_get_core( + db: &AppDatabase, +) -> Result { + Ok(forge::remotes::load(&db.conn).await?) +} + +/// Save ONE folder's selection and hand back every folder's as stored. `None` +/// (or a blank name) puts the folder back on the default remote — the picker's +/// "default (origin)" answer, and what lets a folder be moved off a choice +/// WITHOUT the settings dialog (which does not edit this at all). +pub async fn forge_remote_set_core( + db: &AppDatabase, + folder_id: i32, + remote: Option, +) -> Result { + Ok(forge::remotes::save(&db.conn, folder_id, remote).await?) +} + pub async fn work_task_lookup_by_source_core( db: &AppDatabase, mut source_keys: Vec, @@ -1065,6 +1244,55 @@ fn truncate_chars(input: &str, cap: usize) -> String { input.chars().take(cap).collect() } +/// Which end of a cross-project merge request a project id is being resolved +/// for — the two refusals have different things to tell the user. +/// +/// Both ends are resolved at the TRIGGER, where the user can still choose +/// something else, because a project the token cannot read (private without +/// access, renamed, deleted) leaves the delivery nowhere to go: the merge +/// request is created ON the fork — GitLab resolves `source_branch` in the +/// project the request is addressed to — and the target is named in its body. +/// Every coordinate codeg holds is a path, and GitLab names projects by number, +/// so the two lookups happen here: once, with the target's id recorded on the +/// task rather than looked up again on a retry. +#[derive(Clone, Copy, PartialEq, Eq)] +enum GitLabEnd { + /// The project the merge request is created ON: this folder's `origin`. + Fork, + /// The project it is aimed at: the repository the panel was reading. + Target, +} + +/// A GitLab project id the delivery cannot do without, or the refusal that says +/// which half it was for. +/// +/// Resolved at the trigger, both ends, because that is where the user can still +/// choose something else — and the target is recorded rather than looked up +/// later so a retry or a recovered delivery has the pair on the task already. +fn gitlab_project_id( + resolved: Result, + end: GitLabEnd, + fork_repo: &str, + source_repo: &str, +) -> Result { + resolved.map_err(|e| { + let message = match end { + GitLabEnd::Fork => format!( + "this folder's `origin` is {fork_repo}, so this task's work would be delivered as a \ + merge request FROM that fork — but codeg could not read that GitLab project ({e}), \ + so there would be nowhere to open it. Check that the account can see {fork_repo}, \ + or point the panel at a folder whose `origin` IS {source_repo}." + ), + GitLabEnd::Target => format!( + "this task's work would be delivered as a merge request from {fork_repo} into \ + {source_repo} — but codeg could not read {source_repo} on GitLab ({e}), so there \ + would be nowhere to open it. Refresh the workbench and try again." + ), + }; + AppCommandError::invalid_input(message) + }) +} + // ── Tauri wrappers (desktop mode) ─────────────────────────────────────────── #[cfg(feature = "tauri-runtime")] @@ -1234,6 +1462,24 @@ pub async fn forge_settings_set( forge_settings_set_core(&db, folder_id, settings).await } +#[cfg(feature = "tauri-runtime")] +#[cfg_attr(feature = "tauri-runtime", tauri::command)] +pub async fn forge_remote_get( + db: tauri::State<'_, AppDatabase>, +) -> Result { + forge_remote_get_core(&db).await +} + +#[cfg(feature = "tauri-runtime")] +#[cfg_attr(feature = "tauri-runtime", tauri::command)] +pub async fn forge_remote_set( + db: tauri::State<'_, AppDatabase>, + folder_id: i32, + remote: Option, +) -> Result { + forge_remote_set_core(&db, folder_id, remote).await +} + // AppCommandError ← ForgeError conversion lives in `forge::mod` (used above // via `?` and the explicit map for `source_key`). #[allow(unused)] @@ -1245,6 +1491,198 @@ fn _assert_forge_error_converts(err: ForgeError) -> AppCommandError { mod tests { use super::*; + /// Minimal git fixture: a repo with remotes and no commits needed. + fn git_run(dir: &std::path::Path, args: &[&str]) { + let status = std::process::Command::new("git") + .args(args) + .current_dir(dir) + .status() + .expect("run git"); + assert!(status.success(), "git {args:?} failed"); + } + + /// The panel follows the folder's SAVED remote selection rather than a + /// hardcoded `origin` — the fork workflow (`origin` = your fork, `upstream` + /// = the parent) is the point of the switch. With nothing saved the read is + /// byte-for-byte the old behavior, which is what keeps existing installs + /// unchanged. + #[tokio::test] + async fn folder_forge_remote_follows_the_saved_selection() { + use crate::db::service::folder_service; + + let db = crate::db::test_helpers::fresh_in_memory_db().await; + let dir = tempfile::tempdir().expect("tempdir"); + git_run(dir.path(), &["init", "-q"]); + git_run(dir.path(), &["remote", "add", "origin", "https://github.com/me/app.git"]); + git_run(dir.path(), &["remote", "add", "upstream", "https://github.com/acme/app.git"]); + let folder = folder_service::add_folder(&db.conn, dir.path().to_str().unwrap()) + .await + .expect("folder row"); + + // Nothing saved → the historical default. + let remote = folder_forge_remote_core(&db, folder.id) + .await + .expect("resolve") + .expect("origin resolves"); + assert_eq!(remote.remote_name, "origin"); + assert_eq!(remote.owner_repo, "me/app"); + + // A folder-scoped save must change what the very next read resolves. + crate::forge::remotes::save(&db.conn, folder.id, Some("upstream".into())) + .await + .expect("save"); + + let remote = folder_forge_remote_core(&db, folder.id) + .await + .expect("resolve") + .expect("upstream resolves"); + assert_eq!(remote.remote_name, "upstream"); + assert_eq!(remote.owner_repo, "acme/app"); + } + + /// A selection naming a remote the folder does not have is `None`, the same + /// answer a missing `origin` gives. The panel explains it instead of + /// spending a request on the wrong repository. + #[tokio::test] + async fn a_missing_selected_remote_resolves_to_none() { + use crate::db::service::folder_service; + + let db = crate::db::test_helpers::fresh_in_memory_db().await; + let dir = tempfile::tempdir().expect("tempdir"); + git_run(dir.path(), &["init", "-q"]); + git_run(dir.path(), &["remote", "add", "origin", "https://github.com/me/app.git"]); + let folder = folder_service::add_folder(&db.conn, dir.path().to_str().unwrap()) + .await + .expect("folder row"); + + crate::forge::remotes::save(&db.conn, folder.id, Some("upstream".into())) + .await + .expect("save"); + + assert!( + folder_forge_remote_core(&db, folder.id) + .await + .expect("resolve") + .is_none(), + "a missing selected remote must not fall back to origin" + ); + } + + /// The maintainer's reproducer as a regression: the trigger dialog's "use + /// the global defaults" save DROPS the folder's whole panel-settings row, + /// which used to take the picker's remote selection with it — a choice the + /// user had watched succeed, gone on the next resolve. The selection is not + /// part of that row any more (see `forge::remotes`), so no settings save of + /// any shape can reach it. + #[tokio::test] + async fn a_settings_save_cannot_clear_the_remote_selection() { + use crate::db::service::folder_service; + + let db = crate::db::test_helpers::fresh_in_memory_db().await; + let dir = tempfile::tempdir().expect("tempdir"); + git_run(dir.path(), &["init", "-q"]); + git_run(dir.path(), &["remote", "add", "origin", "https://github.com/me/app.git"]); + git_run(dir.path(), &["remote", "add", "upstream", "https://github.com/acme/app.git"]); + let folder = folder_service::add_folder(&db.conn, dir.path().to_str().unwrap()) + .await + .expect("folder row"); + + // The picker's write: the selection, on its own. + crate::forge::remotes::save(&db.conn, folder.id, Some("upstream".into())) + .await + .expect("save the selection"); + + // Every shape of panel-settings save: the folder's own row, the global + // row, and the drop that "use the global defaults" performs. + let settings = crate::forge::settings::ForgePanelSettings { + writeback_default: false, + ..Default::default() + }; + forge_settings_set_core(&db, Some(folder.id), Some(settings.clone())) + .await + .expect("folder row"); + forge_settings_set_core(&db, None, Some(settings)) + .await + .expect("global row"); + forge_settings_set_core(&db, Some(folder.id), None) + .await + .expect("drop the folder row"); + + let remote = folder_forge_remote_core(&db, folder.id) + .await + .expect("resolve") + .expect("upstream still resolves"); + assert_eq!(remote.remote_name, "upstream"); + assert_eq!(remote.owner_repo, "acme/app"); + } + + /// The maintainer's two-client scenario: one window changes the folder's + /// remote, the other sends a write carrying the coordinates it still has on + /// screen. The write must be REFUSED — never redirected into the repository + /// the selection now names — and the refusal has to be one the panel can + /// recognise (its i18n key) and re-resolve from. + #[tokio::test] + async fn a_write_with_stale_coordinates_is_refused_not_redirected() { + use crate::db::service::folder_service; + + let db = crate::db::test_helpers::fresh_in_memory_db().await; + let dir = tempfile::tempdir().expect("tempdir"); + git_run(dir.path(), &["init", "-q"]); + git_run(dir.path(), &["remote", "add", "origin", "https://github.com/me/app.git"]); + git_run(dir.path(), &["remote", "add", "upstream", "https://github.com/acme/app.git"]); + let folder = folder_service::add_folder(&db.conn, dir.path().to_str().unwrap()) + .await + .expect("folder row"); + + // Window B switches the folder to the parent while window A still + // shows the fork; A's write names the fork. + crate::forge::remotes::save(&db.conn, folder.id, Some("upstream".into())) + .await + .expect("switch"); + + let refused = resolve_folder_repo_for_write( + &db, + folder.id, + None, + Some(("github.com", "me/app")), + ) + .await + .expect_err("stale coordinates must be refused"); + assert!( + matches!( + refused.code, + crate::app_error::AppErrorCode::ConfigurationInvalid + ), + "{:?}", + refused.code + ); + assert_eq!(refused.i18n_key.as_deref(), Some(forge::WRITE_MISMATCH_I18N_KEY)); + let params = refused.i18n_params.expect("both repositories are named"); + assert_eq!(params.get("expected").map(String::as_str), Some("github.com/me/app")); + assert_eq!(params.get("actual").map(String::as_str), Some("github.com/acme/app")); + + // The coordinates the panel is ACTUALLY showing get past the check, and + // stop at the next gate (no account is configured here). That is what + // proves the refusal above came from the coordinate check rather than + // from the folder failing to resolve at all. + let past = resolve_folder_repo_for_write( + &db, + folder.id, + None, + Some(("github.com", "acme/app")), + ) + .await + .expect_err("no account"); + assert_eq!(past.i18n_key.as_deref(), Some(forge::NO_ACCOUNT_I18N_KEY)); + + // Naming nothing behaves exactly as it did before this check existed: + // a build that predates it keeps working. + let unnamed = resolve_folder_repo_for_write(&db, folder.id, None, None) + .await + .expect_err("no account"); + assert_eq!(unnamed.i18n_key.as_deref(), Some(forge::NO_ACCOUNT_I18N_KEY)); + } + const URL: &str = "https://github.com/acme/app/issues/7"; fn all_scenarios() -> [ForgeScenario; 4] { @@ -1666,6 +2104,9 @@ mod tests { head_ref: None, head_sha: None, head_repo: None, + fork_repo: None, + fork_project_id: None, + owner_project_id: None, result_pr: None, writeback: stored, }; @@ -1705,4 +2146,38 @@ mod tests { assert_eq!(s.is_report(), expect, "{s:?}"); } } + + /// GitLab's cross-project delivery needs the fork's project id, and the id + /// is resolved at the TRIGGER: failing there refuses the task while the + /// user can still choose something else, instead of after the agent's work + /// has nowhere to go. GitHub and Gitea never call this. + #[test] + fn a_gitlab_end_whose_project_id_cannot_be_read_is_refused_at_trigger() { + assert_eq!( + gitlab_project_id(Ok(4711), GitLabEnd::Fork, "me/app", "acme/app").expect("resolves"), + 4711 + ); + + let not_found = || forge::ForgeError::Api { + status: 404, + message: "404 Project Not Found".into(), + }; + let refusal = gitlab_project_id(Err(not_found()), GitLabEnd::Fork, "me/app", "acme/app") + .expect_err("unreadable fork"); + assert!(refusal.message.contains("me/app"), "{}", refusal.message); + assert!(refusal.message.contains("acme/app"), "{}", refusal.message); + assert!( + refusal.message.contains("404"), + "the forge's own reason rides along: {}", + refusal.message + ); + + // The other end says which repository it could not read, because that + // is the part the user can act on — the fork is already settled. + let refusal = + gitlab_project_id(Err(not_found()), GitLabEnd::Target, "me/app", "acme/app") + .expect_err("unreadable target"); + assert!(refusal.message.contains("me/app"), "{}", refusal.message); + assert!(refusal.message.contains("acme/app"), "{}", refusal.message); + } } diff --git a/src-tauri/src/forge/deliver.rs b/src-tauri/src/forge/deliver.rs index cab6c98dd7..9bfef7071d 100644 --- a/src-tauri/src/forge/deliver.rs +++ b/src-tauri/src/forge/deliver.rs @@ -120,13 +120,52 @@ pub struct ForgePr { pub head_sha: String, pub head_ref: String, /// `owner/repo` of the head — compared with `same_repo`, never `==`. + /// GitLab's LIST payload abbreviates a foreign source project to + /// `project-{id}`; see [`ForgePr::with_resolved_head`]. pub head_repo: String, pub base_ref: String, } +impl ForgePr { + /// Turn GitLab's `project-{id}` placeholder back into a repository path. + /// + /// GitLab names a foreign source project by NUMBER alone in every list + /// payload — `map_merge_request` writes that as `project-{id}` on purpose, + /// so a gate does not spend one request per row to learn the same thing. + /// A delivery that knows which id it recorded at trigger time substitutes + /// the fork's path for free, and every comparison downstream (the four-way + /// match's head criterion, `check_pull_target`) then works on the same + /// spelling the detail endpoint would have given. + /// + /// Only the RECORDED id is substituted: another fork's merge request keeps + /// its placeholder, which matches nothing — that is what keeps a + /// stranger's merge request from being adopted. + pub fn with_resolved_head(mut self, fork_project_id: Option, fork_repo: &str) -> Self { + if let Some(id) = fork_project_id { + if self.head_repo == format!("project-{id}") { + self.head_repo = fork_repo.to_string(); + } + } + self + } +} + /// What to open, once we know nothing suitable exists yet. #[derive(Debug, Clone)] pub struct NewPullRequest<'a> { + /// GitLab only: the project that OWNS the head branch, when it is not the + /// repository being merged into. GitHub and Gitea express that in `head` + /// (`owner:branch`); GitLab needs both ends by id, and the request has to be + /// ADDRESSED to this one — GitLab resolves `source_branch` in the project + /// the request is sent to, so naming the fork while addressing the target + /// answers "source_branch does not exist" (verified against gitlab.com). + /// `None` = a same-project merge request, created on `owner_repo` itself. + pub source_project_id: Option, + /// GitLab only, and the other half of the same fact: the project the merge + /// request is aimed at. Required exactly when `source_project_id` is set — + /// GitLab does NOT fall back to the fork's upstream, and a request without + /// it makes the fork its own target. + pub target_project_id: Option, pub title: &'a str, pub head: &'a str, pub base: &'a str, @@ -251,11 +290,20 @@ pub trait ForgeDeliveryApi: Send + Sync { remote_branch: &str, ) -> Result<(), String>; - /// Pull requests whose head is `head_branch` in the source repository, in - /// ANY state — a merged or closed one is exactly what recovery must see. + /// Pull requests whose head is `head_branch` in `head_repo`, in ANY state — + /// a merged or closed one is exactly what recovery must see. + /// + /// Two repositories, deliberately, because a cross-repository delivery has + /// two: the collection that is LISTED is `ctx.owner_repo` (a pull request + /// lives where it is merged into), while `head_repo` is the repository the + /// branch was pushed to, which the match compares a row's head against and + /// which GitHub's `head={owner}:{branch}` pre-filter names. Aiming the list + /// at the head's repository instead comes back empty — which reads as "no + /// pull request exists" and earns a 422 from the create that follows. async fn find_pulls( &self, ctx: &DeliveryCtx<'_>, + head_repo: &str, head_branch: &str, ) -> Result, String>; @@ -341,11 +389,14 @@ impl ForgeDeliveryApi for ForgeDelivery { async fn find_pulls( &self, ctx: &DeliveryCtx<'_>, + head_repo: &str, head_branch: &str, ) -> Result, String> { let auth = resolve(ctx).await?; match ctx.provider { - ForgeProvider::GitHub => find_pulls(&auth, ctx.owner_repo, head_branch).await, + ForgeProvider::GitHub => { + find_pulls(&auth, ctx.owner_repo, head_repo, head_branch).await + } ForgeProvider::GitLab => { gitlab::find_merge_requests(&auth, ctx.owner_repo, head_branch).await } @@ -440,6 +491,12 @@ async fn resolve(ctx: &DeliveryCtx<'_>) -> Result { /// `GET /repos/{o}/{r}/pulls?head={owner}:{branch}&state=all`. /// +/// `owner_repo` is the repository whose pull requests are listed (the one the +/// merge is aimed at); `head_repo` is the one the branch was pushed to, and the +/// ONLY thing taken from it is the owner in the filter — GitHub pre-selects by +/// head owner, so a fork's branch has to be asked for as `owner:branch` while +/// the collection in the path stays the target's. +/// /// Unlike `assignee`/`labels` (silently ignored by this endpoint — see /// `github.rs`), the `head` filter IS applied; verified against the live API /// before this was written. The four-way match still runs locally afterwards: @@ -447,14 +504,17 @@ async fn resolve(ctx: &DeliveryCtx<'_>) -> Result { pub async fn find_pulls( auth: &ResolvedAuth, owner_repo: &str, + head_repo: &str, head_branch: &str, ) -> Result, ForgeError> { let repo = super::normalize_repo(owner_repo) .ok_or_else(|| ForgeError::Invalid(format!("bad repository path: {owner_repo}")))?; - let owner = repo + let head = super::normalize_repo(head_repo) + .ok_or_else(|| ForgeError::Invalid(format!("bad repository path: {head_repo}")))?; + let owner = head .split('/') .next() - .ok_or_else(|| ForgeError::Invalid(format!("bad repository path: {owner_repo}")))?; + .ok_or_else(|| ForgeError::Invalid(format!("bad repository path: {head_repo}")))?; let url = format!( "{}/repos/{}/pulls?head={}:{}&state=all&per_page=100", auth.api_base, @@ -1101,12 +1161,19 @@ mod tests { } fn pull_json(number: i64, merged_at: Option<&str>) -> serde_json::Value { + pull_json_from(number, merged_at, "Acme/App") + } + + /// Same, with the head repository spelled out: the cross-repository case, + /// where the row is in the target's collection while its head lives in a + /// fork. + fn pull_json_from(number: i64, merged_at: Option<&str>, head_repo: &str) -> serde_json::Value { serde_json::json!({ "number": number, "html_url": format!("https://github.test/acme/app/pull/{number}"), "state": if merged_at.is_some() { "closed" } else { "open" }, "merged_at": merged_at, - "head": { "sha": "abc123", "ref": "task/7", "repo": { "full_name": "Acme/App" } }, + "head": { "sha": "abc123", "ref": "task/7", "repo": { "full_name": head_repo } }, "base": { "ref": "main" }, }) } @@ -1133,6 +1200,7 @@ mod tests { // verified against the live API before this was written. let rows = match q.get("head").map(String::as_str) { Some("acme:task/7") => vec![pull_json(4, None)], + Some("me:task/7") => vec![pull_json_from(8, None, "me/app")], Some("acme:merged") => vec![pull_json(5, Some("2026-08-18T00:00:00Z"))], _ => vec![], }; @@ -1144,6 +1212,13 @@ mod tests { async { Json(pull_json(6, None)) } }), ) + .route( + "/repos/me/app/pulls", + // The fork's own collection: the pull request was opened in the + // target, so a search aimed here comes back EMPTY — the shape + // of the mistake, reproduced. + get(|| async { Json(serde_json::Value::Array(vec![])) }), + ) .route( "/conflict/repos/acme/app/pulls", post(|| async { @@ -1182,15 +1257,40 @@ mod tests { let (api_base, _, _, _) = mock_api().await; let auth = auth_for(api_base); - let open = find_pulls(&auth, "Acme/App", "task/7").await.unwrap(); + let open = find_pulls(&auth, "Acme/App", "Acme/App", "task/7").await.unwrap(); assert_eq!(open.len(), 1); assert!(!open[0].merged && open[0].state == "open"); assert_eq!(open[0].head_repo, "Acme/App"); // canonical casing preserved - let merged = find_pulls(&auth, "acme/app", "merged").await.unwrap(); + let merged = find_pulls(&auth, "acme/app", "acme/app", "merged").await.unwrap(); assert!(merged[0].merged, "merged_at must set the merged flag"); - assert!(find_pulls(&auth, "acme/app", "nothing").await.unwrap().is_empty()); + assert!(find_pulls(&auth, "acme/app", "acme/app", "nothing") + .await + .unwrap() + .is_empty()); + } + + /// A cross-repository search: the collection listed is the one being merged + /// into, the `head` filter names the fork that holds the branch. Aiming the + /// collection at the head's repository instead comes back empty, which a + /// delivery reads as "no pull request exists" — the 422 a retry then earns. + #[tokio::test] + async fn a_cross_repo_search_lists_the_target_and_filters_by_the_fork() { + let (api_base, _, _, _) = mock_api().await; + let auth = auth_for(api_base); + let found = find_pulls(&auth, "acme/app", "me/app", "task/7") + .await + .unwrap(); + assert_eq!(found.len(), 1); + assert_eq!(found[0].number, 8); + assert_eq!(found[0].head_repo, "me/app", "the row names the fork"); + // And the head's own repository as the collection: the mock answers the + // way the real endpoint does, with nothing. + assert!(find_pulls(&auth, "me/app", "me/app", "task/7") + .await + .unwrap() + .is_empty()); } #[tokio::test] @@ -1201,6 +1301,8 @@ mod tests { &auth, "acme/app", &NewPullRequest { + source_project_id: None, + target_project_id: None, title: "Fix #7", head: "task/7", base: "main", @@ -1228,6 +1330,8 @@ mod tests { &auth, "acme/app", &NewPullRequest { + source_project_id: None, + target_project_id: None, title: "t", head: "task/7", base: "main", diff --git a/src-tauri/src/forge/gitea.rs b/src-tauri/src/forge/gitea.rs index 11a4b7452d..bae434bd0a 100644 --- a/src-tauri/src/forge/gitea.rs +++ b/src-tauri/src/forge/gitea.rs @@ -2469,6 +2469,8 @@ mod tests { let (api_base, seen) = mock_api().await; let auth = auth_for(api_base); let mut req = NewPullRequest { + source_project_id: None, + target_project_id: None, title: "Fix the crash", head: "codeg/task-1", base: "main", diff --git a/src-tauri/src/forge/gitlab.rs b/src-tauri/src/forge/gitlab.rs index 01bdfe50b0..7aad068c3e 100644 --- a/src-tauri/src/forge/gitlab.rs +++ b/src-tauri/src/forge/gitlab.rs @@ -322,19 +322,33 @@ pub async fn create_merge_request( owner_repo: &str, req: &NewPullRequest<'_>, ) -> Result { - let project = project_ref(owner_repo)?; + // Where the request is ADDRESSED. GitLab resolves `source_branch` in the + // project the request is sent to, so a merge request from a fork is created + // ON the fork, with the project it is aimed at named in the body. Addressing + // the target and naming the fork in `source_project_id` instead is answered + // with `source_branch does not exist` — measured against gitlab.com, not + // guessed. + let project = match req.source_project_id { + Some(fork) => fork.to_string(), + None => project_ref(owner_repo)?, + }; let url = format!("{}/projects/{project}/merge_requests", auth.api_base); let title = if req.draft && !is_draft_title(req.title) { format!("Draft: {}", req.title) } else { req.title.to_string() }; - let body = serde_json::json!({ + let mut body = serde_json::json!({ "source_branch": req.head, "target_branch": req.base, "title": title, "description": req.body, }); + // The other half of a cross-project merge request. Omitting it does not + // fall back to the fork's upstream — it makes the fork its own target. + if let Some(id) = req.target_project_id { + body["target_project_id"] = serde_json::json!(id); + } let raw: RawMergeRequest = api_post(auth, &url, &body) .await? .json() @@ -850,6 +864,33 @@ async fn project_path(auth: &ResolvedAuth, project_id: i64) -> Option { Some(project.path_with_namespace).filter(|p| !p.is_empty()) } +/// `path_with_namespace` → project id — the inverse of [`project_path`], and +/// the one coordinate a cross-project merge request cannot be opened without. +/// +/// GitLab takes the SOURCE project of a merge request by id, while everything +/// codeg holds (a remote URL, a folder, a task's recorded fork) is a path. The +/// id is resolved once, at trigger time, and recorded on the task: the list +/// payload the retry path reads names a foreign source project by number alone +/// (see `map_merge_request`), so the delivery needs this same id to turn that +/// placeholder back into a repository. +pub async fn resolve_project_id( + auth: &ResolvedAuth, + owner_repo: &str, +) -> Result { + #[derive(Deserialize)] + struct RawProject { + id: i64, + } + let project = project_ref(owner_repo)?; + let url = format!("{}/projects/{project}", auth.api_base); + let raw: RawProject = api_get(auth, &url) + .await? + .json() + .await + .map_err(|e| ForgeError::Network(format!("bad project payload: {e}")))?; + Ok(raw.id) +} + pub(crate) async fn api_get( auth: &ResolvedAuth, url: &str, @@ -1425,6 +1466,9 @@ mod tests { let user_hits = Arc::new(AtomicUsize::new(0)); let last_query: Arc>> = Default::default(); let seen = creates.clone(); + // The fork's own create records into the same list from a second + // closure, so it needs its own handle. + let fork_seen = creates.clone(); let issue_notes = notes.clone(); let mr_notes = notes.clone(); let hits = user_hits.clone(); @@ -1460,6 +1504,12 @@ mod tests { (headers, Json(serde_json::Value::Array(rows))) }), ) + // The project itself — what `resolve_project_id` reads, and the id + // a cross-project merge request is opened with. + .route( + "/projects/group%2Fsub%2Fproj", + get(|| async { Json(serde_json::json!({ "id": 4711 })) }), + ) .route( "/projects/group%2Fsub%2Fproj/merge_requests", get(move |Query(q): Query>| async move { @@ -1495,6 +1545,21 @@ mod tests { async { Json(mr_json(8, "opened", 1)) } }), ) + // A merge request created ON a fork: the project in the path owns + // the branch, the target is named in the body. The payload that + // comes back names the fork by number alone, which is what the + // claim path has to turn back into a repository. + .route( + "/projects/4712/merge_requests", + post(move |Json(body): Json| { + fork_seen.lock().unwrap().push(body); + async { + let mut mr = mr_json(9, "opened", 4712); + mr["target_project_id"] = serde_json::json!(4711); + Json(mr) + } + }), + ) .route( "/projects/group%2Fsub%2Fproj/merge_requests/4", get(|| async { @@ -1979,6 +2044,62 @@ mod tests { assert!(get_merge_request(&auth, "not-a-path", 4).await.is_err()); } + /// The path → id lookup a trigger performs, and the cross-project body it + /// The shape of a cross-project merge request, which the live API settled: + /// the request is ADDRESSED to the fork — GitLab resolves `source_branch` + /// in the project in the path, so addressing the target while naming the + /// fork answers `source_branch does not exist` — and the target is named in + /// the body. Omitting that does not fall back to the fork's upstream: it + /// makes the fork its own target. + #[tokio::test] + async fn a_cross_project_merge_request_is_created_on_the_fork() { + let (api_base, creates, _, _, _) = mock_api().await; + let auth = auth_for(api_base); + + assert_eq!( + resolve_project_id(&auth, "Group/Sub/Proj").await.expect("id"), + 4711, + "the path is asked for exactly as the API spells it" + ); + // A path this token cannot see (or that is not a path at all) is an + // error, never a silent 0 — the trigger refuses the task on it. + assert!(resolve_project_id(&auth, "not-a-path").await.is_err()); + + let made = create_merge_request( + &auth, + "group/sub/proj", + &NewPullRequest { + source_project_id: Some(4712), + target_project_id: Some(4711), + title: "From the fork", + head: "task/7", + base: "main", + body: "", + draft: false, + }, + ) + .await + .expect("create"); + assert_eq!(made.number, 9); + let sent = creates.lock().unwrap().first().cloned().unwrap(); + assert_eq!(sent["target_project_id"], 4711); + assert_eq!(sent["source_branch"], "task/7", "the branch stays bare"); + assert!(sent.get("source_project_id").is_none(), "{sent}"); + // And what the target's own list will say about it: a number where a + // repository name belongs, which only the recorded fork id can turn + // back into one. + assert_eq!(made.head_repo, "project-4712"); + assert_eq!( + made.clone().with_resolved_head(Some(4712), "me/fork").head_repo, + "me/fork" + ); + assert_eq!( + made.with_resolved_head(Some(999), "someone/else").head_repo, + "project-4712", + "another fork's merge request is not this task's" + ); + } + /// The delivery's lookup: by source branch, in any state, mapped into the /// same shape the four-way match already knows. #[tokio::test] @@ -2006,6 +2127,8 @@ mod tests { &auth, "group/sub/proj", &NewPullRequest { + source_project_id: None, + target_project_id: None, title: "Fix #7", head: "task/7", base: "main", @@ -2021,12 +2144,17 @@ mod tests { assert_eq!(sent["target_branch"], "main"); assert_eq!(sent["title"], "Draft: Fix #7"); assert_eq!(sent["description"], "Closes #7"); + // A SAME-project merge request must not name a source project: GitLab + // reads that field as "the branch lives over there". + assert!(sent.get("source_project_id").is_none(), "{sent}"); // Not a draft, and an already-prefixed title is not prefixed twice. create_merge_request( &auth, "group/sub/proj", &NewPullRequest { + source_project_id: None, + target_project_id: None, title: "Draft: Fix #7", head: "task/7", base: "main", diff --git a/src-tauri/src/forge/mod.rs b/src-tauri/src/forge/mod.rs index bd3af8e7c2..ad8eab6c4b 100644 --- a/src-tauri/src/forge/mod.rs +++ b/src-tauri/src/forge/mod.rs @@ -10,6 +10,7 @@ pub mod envelope; pub mod gitea; pub mod github; pub mod gitlab; +pub mod remotes; pub mod settings; use std::sync::RwLock; @@ -193,6 +194,14 @@ pub const UNSUPPORTED_HOST_I18N_KEY: &str = "Forge.errors.unsupportedHost"; /// [`NO_ACCOUNT_I18N_KEY`] is. pub const WRONG_FORGE_I18N_KEY: &str = "Forge.errors.wrongForge"; +/// i18n key for the refusal a WRITE gets when the coordinates it carried no +/// longer match the folder's remote (see [`ExpectedCoordinates`]). Root-dotted +/// like the others, and its own key rather than the trigger's +/// `Forge.folderMismatch`: that sentence is written about an ISSUE's +/// repository, and reading it over a comment, a close or a merge names the +/// wrong thing. Same judgement, same recovery, its own words. +pub const WRITE_MISMATCH_I18N_KEY: &str = "Forge.writeMismatch"; + #[derive(Debug, thiserror::Error)] pub enum ForgeError { /// No usable account/token for the requested host (or the token is dead). @@ -391,6 +400,41 @@ pub fn parse_remote_url(url: &str) -> Option<(String, String)> { Some((host, normalize_repo(path)?)) } +/// Where the client believes the folder's repository is, carried by every +/// WRITE so a stale view cannot be redirected into another repository. +/// +/// The panel is not the only client: the same folder can be open in a second +/// window, in a browser tab against this server, or in an old build, and the +/// repository a folder reads is mutable state (see `forge::remotes`). A write +/// that names no repository follows whatever the selection says NOW — which is +/// how a comment meant for one repository lands in another. +/// +/// Both fields are optional in the wire form, and a client that sends neither +/// keeps the old behaviour exactly. Flattened into each request rather than +/// nested, so the write payloads keep the flat shape their other fields have. +#[derive(Debug, Clone, Default, serde::Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct ExpectedCoordinates { + #[serde(default)] + pub expected_server_host: Option, + #[serde(default)] + pub expected_owner_repo: Option, +} + +impl ExpectedCoordinates { + /// The pair to compare against, or `None` when the client named neither + /// side — or only one. Half a coordinate cannot be compared, and inventing + /// the other half would turn a stale client into a wrong refusal. + pub fn pair(&self) -> Option<(&str, &str)> { + let host = self.expected_server_host.as_deref()?.trim(); + let repo = self.expected_owner_repo.as_deref()?.trim(); + if host.is_empty() || repo.is_empty() { + return None; + } + Some((host, repo)) + } +} + /// Provenance snapshot stored in `work_task.source_meta` (JSON) and mirrored /// to the frontend as `ForgeSourceMeta` in `src/lib/types.ts`. #[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] @@ -418,6 +462,34 @@ pub struct ForgeSourceMeta { pub head_sha: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub head_repo: Option, + /// The repository the WORK is pushed to when it is not the source — the + /// folder's `origin`, recorded at trigger time whenever the panel was + /// pointed at another remote (the fork workflow: the picker selects the + /// parent, so the issues are the parent's while the branch codeg can write + /// to is the user's own copy). + /// + /// `None` is "push to the source", which is every task triggered before + /// this field existed and every one triggered from a folder that IS the + /// source. Read through `delivery_push_repo`, never directly. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub fork_repo: Option, + /// GitLab only: the FORK's project id, resolved at trigger time. + /// + /// It is the project a cross-project merge request is created ON + /// (`POST /projects/{id}/merge_requests` — GitLab resolves `source_branch` + /// in the project the request is addressed to), and — because GitLab's list + /// payload names a foreign source project by number alone — it is also what + /// turns that `project-{id}` placeholder back into a repository when a + /// delivery has to recognise its own merge request on a retry. See + /// `ForgePr::with_resolved_head`. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub fork_project_id: Option, + /// GitLab only: the id of the project the merge request is AIMED at (the + /// repository the panel was reading). Recorded because the create has to + /// spell it out on a request addressed to the fork — GitLab does not infer + /// the target from the fork's upstream. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub owner_project_id: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub result_pr: Option, /// Whether this task comments its outcome back on the item when it @@ -1007,6 +1079,8 @@ pub struct CommentDraft { pub body: String, #[serde(default)] pub account_id: Option, + #[serde(flatten)] + pub expected: ExpectedCoordinates, } impl CommentDraft { @@ -1080,6 +1154,8 @@ pub struct StateChangeRequest { pub action: ForgeStateAction, #[serde(default)] pub account_id: Option, + #[serde(flatten)] + pub expected: ExpectedCoordinates, } impl StateChangeRequest { @@ -1200,6 +1276,8 @@ pub struct ChangeMergeRequest { pub head_sha: Option, #[serde(default)] pub account_id: Option, + #[serde(flatten)] + pub expected: ExpectedCoordinates, } impl ChangeMergeRequest { @@ -1232,6 +1310,8 @@ pub struct NewIssueDraft { pub labels: Vec, #[serde(default)] pub account_id: Option, + #[serde(flatten)] + pub expected: ExpectedCoordinates, } /// A validated new issue — the only shape a provider client will take. @@ -1745,6 +1825,70 @@ mod tests { assert!(serde_json::to_value(&dead_token).unwrap().get("i18n_key").is_none()); } + /// Coordinates are only comparable when BOTH halves arrive. With one of + /// them missing there is nothing to compare, and inventing the other half + /// would turn a stale client into a wrong refusal. + #[test] + fn expected_coordinates_need_both_halves_to_be_comparable() { + let both = ExpectedCoordinates { + expected_server_host: Some(" github.com ".into()), + expected_owner_repo: Some(" me/app ".into()), + }; + assert_eq!(both.pair(), Some(("github.com", "me/app"))); + + assert_eq!(ExpectedCoordinates::default().pair(), None); + assert_eq!( + ExpectedCoordinates { + expected_server_host: Some("github.com".into()), + expected_owner_repo: None, + } + .pair(), + None, + "half a coordinate cannot be compared" + ); + assert_eq!( + ExpectedCoordinates { + expected_server_host: Some(" ".into()), + expected_owner_repo: Some("me/app".into()), + } + .pair(), + None + ); + } + + /// The shape a write actually puts on the wire: the pair rides FLAT beside + /// the request's own fields, and a payload from a build that predates the + /// check decodes to "named nothing" rather than failing to decode at all. + #[test] + fn a_write_payload_carries_its_coordinates_flat() { + let draft: CommentDraft = serde_json::from_str( + r#"{"kind":"issue","number":7,"body":"hi","expectedServerHost":"github.com","expectedOwnerRepo":"me/app"}"#, + ) + .expect("decodes"); + assert_eq!(draft.expected.pair(), Some(("github.com", "me/app"))); + + let old: CommentDraft = + serde_json::from_str(r#"{"kind":"issue","number":7,"body":"hi"}"#).expect("decodes"); + assert_eq!(old.expected.pair(), None); + + // The other three write payloads flatten the same pair. + let merge: ChangeMergeRequest = serde_json::from_str( + r#"{"number":7,"method":"merge","expectedServerHost":"github.com","expectedOwnerRepo":"me/app"}"#, + ) + .expect("decodes"); + assert_eq!(merge.expected.pair(), Some(("github.com", "me/app"))); + let state: StateChangeRequest = serde_json::from_str( + r#"{"kind":"pr","number":7,"action":"close","expectedServerHost":"github.com","expectedOwnerRepo":"me/app"}"#, + ) + .expect("decodes"); + assert_eq!(state.expected.pair(), Some(("github.com", "me/app"))); + let issue: NewIssueDraft = serde_json::from_str( + r#"{"title":"t","expectedServerHost":"github.com","expectedOwnerRepo":"me/app"}"#, + ) + .expect("decodes"); + assert_eq!(issue.expected.pair(), Some(("github.com", "me/app"))); + } + #[test] fn source_key_normalizes_and_validates() { assert_eq!( @@ -2216,6 +2360,7 @@ mod tests { number, body: body.into(), account_id: None, + expected: ExpectedCoordinates::default(), }; assert_eq!( draft("issue", 7, " looks fixed ").resolve().unwrap(), @@ -2277,6 +2422,7 @@ mod tests { body: body.map(str::to_string), labels: labels.into_iter().map(str::to_string).collect(), account_id: None, + expected: ExpectedCoordinates::default(), }; assert_eq!( draft(" Login times out ", Some(" steps "), vec![" bug ", "", "bug", "docs"]) @@ -2300,6 +2446,7 @@ mod tests { body: None, labels: distinct.clone(), account_id: None, + expected: ExpectedCoordinates::default(), } .resolve() .unwrap(); @@ -2313,6 +2460,7 @@ mod tests { body: None, labels: absurd, account_id: None, + expected: ExpectedCoordinates::default(), } .resolve() .unwrap() diff --git a/src-tauri/src/forge/remotes.rs b/src-tauri/src/forge/remotes.rs new file mode 100644 index 0000000000..90fe56f334 --- /dev/null +++ b/src-tauri/src/forge/remotes.rs @@ -0,0 +1,267 @@ +//! Which git remote each folder's forge panel reads. +//! +//! **Not a field of [`ForgePanelSettings`](super::settings::ForgePanelSettings)**, +//! and not one shared JSON blob either. Each folder owns one metadata key: +//! `forge_panel_remote:`. +//! +//! That shape is deliberate. The picker can be used from more than one window, +//! and the database pool has multiple connections. A shared read-modify-write +//! blob lets two saves race: +//! +//! 1. window A reads the map, +//! 2. window B reads the same map, +//! 3. A writes folder 1, +//! 4. B writes folder 2 from its stale copy and silently drops folder 1. +//! +//! Per-folder keys remove that race entirely: unrelated folders never rewrite +//! one another. A folder with no key reads the historical `origin`; absence is +//! the default answer. +//! +//! The public API still returns a [`ForgeRemoteStore`] containing every saved +//! folder so the frontend can switch folders without another round trip. That +//! store is assembled from independent rows at read time; it is not persisted +//! as one value. + +use std::collections::BTreeMap; + +use sea_orm::{ColumnTrait, DatabaseConnection, EntityTrait, QueryFilter}; +use serde::{Deserialize, Serialize}; + +use crate::db::entities::app_metadata; +use crate::db::error::DbError; +use crate::db::service::app_metadata_service; + +/// Prefix for one folder's selection. The suffix is the decimal folder id. +const REMOTE_KEY_PREFIX: &str = "forge_panel_remote:"; + +/// Every folder's selection at once, as exposed to the frontend. +/// +/// Persistence is per-folder even though the wire shape is aggregated. +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] +pub struct ForgeRemoteStore { + #[serde(default)] + pub folders: BTreeMap, +} + +impl ForgeRemoteStore { + /// The name this folder is set to, or `None` for the default. + pub fn selected(&self, folder_id: i32) -> Option<&str> { + self.folders.get(&folder_id).map(String::as_str) + } +} + +fn remote_key(folder_id: i32) -> String { + format!("{REMOTE_KEY_PREFIX}{folder_id}") +} + +fn parse_folder_id(key: &str) -> Option { + key.strip_prefix(REMOTE_KEY_PREFIX)?.parse().ok() +} + +fn trim(value: Option) -> Option { + value + .map(|v| v.trim().to_string()) + .filter(|v| !v.is_empty()) +} + +/// Read every saved folder selection. +/// +/// Each row is independent in storage; this aggregation exists only for the +/// frontend's "load once, switch folders locally" API. +pub async fn load(conn: &DatabaseConnection) -> Result { + let rows = app_metadata::Entity::find() + .filter(app_metadata::Column::Key.starts_with(REMOTE_KEY_PREFIX)) + .filter(app_metadata::Column::DeletedAt.is_null()) + .all(conn) + .await?; + + let mut folders = BTreeMap::new(); + for row in rows { + let Some(folder_id) = parse_folder_id(&row.key) else { + continue; + }; + let Some(remote) = trim(Some(row.value)) else { + continue; + }; + folders.insert(folder_id, remote); + } + + Ok(ForgeRemoteStore { folders }) +} + +/// Read one folder directly from its own key — the hot path used by every forge +/// operation. No shared store is read or rewritten. +pub async fn load_selected( + conn: &DatabaseConnection, + folder_id: i32, +) -> Result, DbError> { + let raw = app_metadata_service::get_value(conn, &remote_key(folder_id)).await?; + Ok(trim(raw)) +} + +/// Save exactly one folder's selection and return the aggregated frontend view. +/// +/// A blank or absent name clears the key, putting the folder back on the +/// historical `origin` default. Different folder ids touch different database +/// rows, so concurrent saves cannot overwrite one another. +pub async fn save( + conn: &DatabaseConnection, + folder_id: i32, + remote: Option, +) -> Result { + let key = remote_key(folder_id); + match trim(remote) { + Some(name) => app_metadata_service::upsert_value(conn, &key, &name).await?, + None => { + app_metadata::Entity::delete_many() + .filter(app_metadata::Column::Key.eq(key)) + .exec(conn) + .await?; + } + } + load(conn).await +} + +#[cfg(test)] +mod tests { + use super::*; + + /// A folder with no key is on the default, not on a stored empty string the + /// resolver would have to special-case. + #[tokio::test] + async fn an_untouched_folder_has_no_selection_of_its_own() { + let db = crate::db::test_helpers::fresh_in_memory_db().await; + assert_eq!(load_selected(&db.conn, 1).await.expect("read"), None); + assert_eq!(load(&db.conn).await.expect("store"), ForgeRemoteStore::default()); + } + + /// One folder's value is trimmed on write; blank and absent values clear the + /// key so the resolver goes back to `origin`. + #[tokio::test] + async fn saving_trims_the_name_and_clearing_removes_the_key() { + let db = crate::db::test_helpers::fresh_in_memory_db().await; + + save(&db.conn, 1, Some(" upstream ".into())) + .await + .expect("save"); + assert_eq!( + load_selected(&db.conn, 1).await.expect("selected").as_deref(), + Some("upstream") + ); + + save(&db.conn, 1, Some(" ".into())) + .await + .expect("blank clears"); + assert_eq!(load_selected(&db.conn, 1).await.expect("selected"), None); + + save(&db.conn, 1, Some("upstream".into())) + .await + .expect("save again"); + save(&db.conn, 1, None).await.expect("clear"); + assert_eq!(load_selected(&db.conn, 1).await.expect("selected"), None); + } + + /// The frontend still receives every folder at once even though persistence + /// is one key per folder. + #[tokio::test] + async fn load_aggregates_the_independent_folder_keys() { + let db = crate::db::test_helpers::fresh_in_memory_db().await; + + save(&db.conn, 3, Some("upstream".into())) + .await + .expect("folder 3"); + save(&db.conn, 4, Some("backup".into())) + .await + .expect("folder 4"); + + let store = load(&db.conn).await.expect("reload"); + assert_eq!(store.selected(3), Some("upstream")); + assert_eq!(store.selected(4), Some("backup")); + assert_eq!(store.selected(5), None); + } + + /// This is the race the shared JSON blob could not make safe: two windows + /// save different folders at the same time. With per-folder keys each write + /// targets a different row, so neither can erase the other. + #[tokio::test] + async fn concurrent_saves_to_different_folders_cannot_overwrite_each_other() { + let db = crate::db::test_helpers::fresh_in_memory_db().await; + + let (left, right) = tokio::join!( + save(&db.conn, 3, Some("upstream".into())), + save(&db.conn, 4, Some("backup".into())) + ); + left.expect("folder 3 save"); + right.expect("folder 4 save"); + + let store = load(&db.conn).await.expect("reload"); + assert_eq!(store.selected(3), Some("upstream")); + assert_eq!(store.selected(4), Some("backup")); + } + + /// Clearing one folder deletes only that folder's row. + #[tokio::test] + async fn clearing_one_folder_leaves_the_others_untouched() { + let db = crate::db::test_helpers::fresh_in_memory_db().await; + + save(&db.conn, 3, Some("upstream".into())) + .await + .expect("folder 3"); + save(&db.conn, 4, Some("backup".into())) + .await + .expect("folder 4"); + + save(&db.conn, 3, None).await.expect("clear folder 3"); + let store = load(&db.conn).await.expect("reload"); + assert_eq!(store.selected(3), None); + assert_eq!(store.selected(4), Some("backup")); + } + + /// Malformed keys or blank values are ignored when building the aggregate + /// store; one bad metadata row must not break every forge panel. + #[tokio::test] + async fn malformed_rows_do_not_break_the_store() { + let db = crate::db::test_helpers::fresh_in_memory_db().await; + + app_metadata_service::upsert_value( + &db.conn, + &format!("{REMOTE_KEY_PREFIX}not-an-id"), + "upstream", + ) + .await + .expect("malformed id"); + app_metadata_service::upsert_value( + &db.conn, + &format!("{REMOTE_KEY_PREFIX}7"), + " ", + ) + .await + .expect("blank value"); + app_metadata_service::upsert_value( + &db.conn, + &format!("{REMOTE_KEY_PREFIX}8"), + " backup ", + ) + .await + .expect("valid value"); + + let store = load(&db.conn).await.expect("load"); + assert_eq!(store.selected(7), None); + assert_eq!(store.selected(8), Some("backup")); + } + + /// The aggregated wire shape still serializes folder ids as JSON object + /// keys, which is what the TypeScript `Record` consumes. + #[test] + fn folder_keys_survive_the_json_round_trip() { + let store = ForgeRemoteStore { + folders: [(42, "upstream".to_string())].into_iter().collect(), + }; + let encoded = serde_json::to_string(&store).expect("serializable"); + assert!(encoded.contains("\"42\""), "{encoded}"); + assert_eq!( + serde_json::from_str::(&encoded).expect("decodes"), + store + ); + } +} diff --git a/src-tauri/src/forge/settings.rs b/src-tauri/src/forge/settings.rs index 35709d55f8..d80dfd03ea 100644 --- a/src-tauri/src/forge/settings.rs +++ b/src-tauri/src/forge/settings.rs @@ -15,6 +15,12 @@ //! covers a KIND of work item — how an issue should be handled as opposed to a //! review, which is a distinction the task engine has no word for. //! +//! What it does NOT hold either is the panel's remote selection: that lives in +//! [`super::remotes`], because the picker saves it on every click while this +//! blob is rewritten WHOLESALE by a dialog — and one field living in the +//! other's blob is how "use global defaults" came to destroy a choice the +//! picker had already saved. +//! //! Stored as ONE JSON blob in `app_metadata` — the global row and every //! override together — rather than a row per scope. The whole thing is read //! once per page load and once per trigger, and a save is a read-modify-write diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index b33af7db67..0383da1530 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -1823,6 +1823,8 @@ mod tauri_app { forge_commands::work_task_lookup_by_source, forge_commands::forge_settings_get, forge_commands::forge_settings_set, + forge_commands::forge_remote_get, + forge_commands::forge_remote_set, terminal_commands::terminal_spawn, terminal_commands::terminal_write, terminal_commands::terminal_resize, diff --git a/src-tauri/src/web/handlers/forge.rs b/src-tauri/src/web/handlers/forge.rs index b01a757530..b8280c6e0c 100644 --- a/src-tauri/src/web/handlers/forge.rs +++ b/src-tauri/src/web/handlers/forge.rs @@ -9,6 +9,7 @@ use serde::Deserialize; use crate::app_error::AppCommandError; use crate::app_state::AppState; use crate::commands::forge as core; +use crate::forge::remotes::ForgeRemoteStore; use crate::forge::settings::{ForgePanelSettings, ForgeSettingsStore}; use crate::forge::{ ChangeFilesQuery, ChangeMergeRequest, ChangeQuery, CommentDraft, CommentFilters, CountFilters, @@ -297,3 +298,29 @@ pub async fn forge_settings_set( core::forge_settings_set_core(&state.db, params.folder_id, params.settings).await?, )) } + +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct RemoteSetParams { + pub folder_id: i32, + /// The remote to read the folder from, or null for the default. A blank + /// name is the same answer — the store normalizes it (see + /// `forge::remotes`). + #[serde(default)] + pub remote: Option, +} + +pub async fn forge_remote_get( + Extension(state): Extension>, +) -> Result, AppCommandError> { + Ok(Json(core::forge_remote_get_core(&state.db).await?)) +} + +pub async fn forge_remote_set( + Extension(state): Extension>, + Json(params): Json, +) -> Result, AppCommandError> { + Ok(Json( + core::forge_remote_set_core(&state.db, params.folder_id, params.remote).await?, + )) +} diff --git a/src-tauri/src/web/router.rs b/src-tauri/src/web/router.rs index 664342481c..29282abbad 100644 --- a/src-tauri/src/web/router.rs +++ b/src-tauri/src/web/router.rs @@ -1526,6 +1526,14 @@ pub fn build_router( "/forge_settings_set", post(handlers::forge::forge_settings_set), ) + .route( + "/forge_remote_get", + post(handlers::forge::forge_remote_get), + ) + .route( + "/forge_remote_set", + post(handlers::forge::forge_remote_set), + ) .route( "/work_task_deliver_pr", post(handlers::work_task::work_task_deliver_pr), diff --git a/src-tauri/src/work_task/engine.rs b/src-tauri/src/work_task/engine.rs index 23a827b6b8..c46f8662e5 100644 --- a/src-tauri/src/work_task/engine.rs +++ b/src-tauri/src/work_task/engine.rs @@ -49,7 +49,9 @@ use crate::forge::deliver::{ adopt_pull_request, pull_request_body, writeback_comment_body, DeliveryCtx, ForgeDeliveryApi, ForgePr, NewPullRequest, PrAdoption, TaskOutcome, }; -use crate::forge::{ForgeItemKind, ForgeSourceMeta, SOURCE_KIND_ISSUE, SOURCE_KIND_PR}; +use crate::forge::{ + ForgeItemKind, ForgeProvider, ForgeSourceMeta, SOURCE_KIND_ISSUE, SOURCE_KIND_PR, +}; use crate::logging::throttle::{LagLogThrottle, LAG_LOG_WINDOW}; use crate::models::{ AgentType, FollowUpIntent, WorkTaskConfig, WorkTaskFolderSettings, WorkTaskMergeOp, @@ -4035,12 +4037,14 @@ impl TaskEngine { trigger it again" .to_string() })?; - // The push lands in the recorded HEAD repository — the fork, when - // the pull request comes from one. Resolvability is re-checked - // here, before the CAS, so a row whose fork codeg cannot name - // (written by an older build, or hydrated while the fork was - // already gone) is refused with the task left exactly as it was. - pull_push_repo(&meta)?; + // The push lands in the repository this task's work belongs to — + // the pull request's own head (the fork, when it comes from one), + // or the recorded fork of an issue task. Resolvability is + // re-checked here, before the CAS, so a row whose fork codeg + // cannot name (written by an older build, or hydrated while the + // fork was already gone) is refused with the task left exactly as + // it was. + delivery_push_repo(&meta)?; head_ref } else { work_branch.clone() @@ -4337,26 +4341,47 @@ impl TaskEngine { account_id: &meta.account_id, owner_repo: &meta.owner_repo, }; + // Where the work lands: the source, or the fork this folder's `origin` + // named when the task was triggered. Resolved ONCE, before the push, + // because the same answer decides three things — the push's target, the + // `head` a new pull request is created with, and (on GitHub and Gitea) + // which owner the look-before-create asks for. The pull request itself + // is still listed in the SOURCE's collection either way. + let push_repo = delivery_push_repo(meta)?; // Fast-forward push of the same commits is a no-op, so a retry after a // later step failed costs nothing and changes nothing. self.forge - .push_branch(&ctx, wt_path, &meta.owner_repo, work_branch, work_branch) + .push_branch(&ctx, wt_path, &push_repo, work_branch, work_branch) .await .map_err(|e| format!("could not push the task branch: {e}"))?; // Look before creating: the previous attempt may have opened the pull - // request and died on the way to settling it. + // request and died on the way to settling it. The LIST is the source's + // — a pull request lives where it is merged into — while the branch + // being searched for is the fork's, which GitHub's + // `head={owner}:{branch}` pre-filter names and the four-way match + // compares against. Aim it at the fork instead and the list comes back + // empty, which is how a retry opens a second pull request and eats a + // 422. let existing = self .forge - .find_pulls(&ctx, work_branch) + .find_pulls(&ctx, &push_repo, work_branch) .await .map_err(|e| format!("could not check for an existing pull request: {e}"))?; + // GitLab's list payload names a foreign source project by NUMBER only + // (`project-{id}`); the task recorded which id that is at trigger time, + // so the placeholder becomes the fork's path before the match — without + // it, a retry can never recognise the merge request it already opened. + let existing: Vec = existing + .into_iter() + .map(|pr| pr.with_resolved_head(meta.fork_project_id, &push_repo)) + .collect(); match adopt_pull_request( existing, expected_head, work_branch, base_branch, - &meta.owner_repo, + &push_repo, ) { PrAdoption::Merged(pr) | PrAdoption::Open(pr) => Ok(pr), PrAdoption::ClosedUnmerged(pr) => Err(format!( @@ -4375,12 +4400,30 @@ impl TaskEngine { )), PrAdoption::NoMatch => { let body = pull_request_body(&meta.url, meta.number, task_id); + // The head goes to the FORK when there is one — as + // `owner:branch`, which is how both GitHub and Gitea spell a + // repository other than the target. GitLab has no qualified + // head: its side of the same fact is the pair of project ids + // below, and its `source_branch` stays bare. + let head = head_ref_for(meta.provider, &push_repo, &meta.owner_repo, work_branch); + // The id pair a cross-project GitLab merge request needs: the + // project the request is created ON (the fork that holds the + // branch) and the one it is aimed at. A same-project delivery + // carries neither. + let (source_project_id, target_project_id) = + if crate::forge::same_repo(&push_repo, &meta.owner_repo) { + (None, None) + } else { + (meta.fork_project_id, meta.owner_project_id) + }; self.forge .create_pull( &ctx, &NewPullRequest { + source_project_id, + target_project_id, title, - head: work_branch, + head: &head, base: base_branch, body: &body, draft, @@ -4449,7 +4492,7 @@ impl TaskEngine { meta.number )); } - let push_repo = pull_push_repo(meta)?; + let push_repo = delivery_push_repo(meta)?; self.forge .push_branch(&ctx, wt_path, &push_repo, work_branch, remote_branch) .await @@ -4516,18 +4559,20 @@ impl TaskEngine { remote_branch: &str, ) -> Result<(), String> { let noun = meta.provider.change_noun(); - // Compared against the head repository RECORDED at trigger time (the - // fork, when the pull request comes from one — rows without one are - // same-repo by construction), not against the source repository: a - // fork's pull request legitimately lives elsewhere, and the thing - // being caught here is the head moving since the task was made. - let recorded_repo = meta - .head_repo - .as_deref() - .map(str::trim) - .filter(|r| !r.is_empty()) - .unwrap_or(&meta.owner_repo); - if !crate::forge::same_repo(&pr.head_repo, recorded_repo) { + // Compared against the repository this task's work is pushed to — the + // head of a pull-request task, or the recorded fork of an issue task — + // not against the source: a cross-repository delivery legitimately has + // its head elsewhere, and the thing being caught here is that head + // moving since the task was made. Rows written before the fork was + // recorded are same-repo by construction and answer the source. + let recorded_repo = delivery_push_repo(meta)?; + // GitLab's list payload abbreviates a foreign source project to + // `project-{id}`; the task recorded which id is its fork, so the + // placeholder becomes the path before the comparison — otherwise a + // cross-project merge request created a moment ago reads as one that + // was retargeted under us. + let pr = pr.clone().with_resolved_head(meta.fork_project_id, &recorded_repo); + if !crate::forge::same_repo(&pr.head_repo, &recorded_repo) { return Err(format!( "{noun} #{} now comes from {}, not {recorded_repo} — check it before delivering \ again", @@ -4864,8 +4909,22 @@ impl TaskEngine { .await; return; } - let found = match self.forge.find_pulls(&ctx, remote_branch).await { - Ok(prs) => prs, + // An ISSUE task's own branch lives in the repository recorded for it — + // the fork, when the folder's `origin` was not the source. Recovery is + // the one place that has to agree with `push_and_open` about where + // that is, or an interrupted delivery re-opens what already exists. + let push_repo = match delivery_push_repo(&meta) { + Ok(repo) => repo, + Err(e) => { + self.bounce_delivery(task, e).await; + return; + } + }; + let found = match self.forge.find_pulls(&ctx, &push_repo, remote_branch).await { + Ok(prs) => prs + .into_iter() + .map(|pr| pr.with_resolved_head(meta.fork_project_id, &push_repo)) + .collect(), Err(e) => { self.bounce_delivery( task, @@ -4883,7 +4942,7 @@ impl TaskEngine { expected_head, remote_branch, base_branch, - &meta.owner_repo, + &push_repo, ) { PrAdoption::Merged(pr) | PrAdoption::Open(pr) => { if let Err(e) = self.settle_delivery(task_id, &meta, task.run_seq, &pr).await { @@ -6140,31 +6199,59 @@ fn classify_push_refusal(error: &str) -> PushRefusal { PushRefusal::Unknown } -/// The repository a pull-request task's push-back lands in: the HEAD -/// repository recorded at trigger time — the fork, when the pull request comes -/// from one. A row that recorded none falls back to the source repository: -/// builds that predate the field refused forks at trigger, so their rows are -/// same-repo by construction. `Err` is the one head codeg cannot push to ever — -/// a fork it cannot name (GitLab's unresolved `project-{id}` placeholder, or a -/// fork deleted since GitHub hydrated the row). -fn pull_push_repo(meta: &ForgeSourceMeta) -> Result { +/// The repository a task's WORK lands in. +/// +/// Three answers, in order: +/// - a task that IS a pull request pushes back to that pull request's own head +/// repository (the fork, when it comes from one); +/// - an issue task pushes its own branch to the fork its folder's `origin` +/// named when the task was triggered, if that was not the source; +/// - otherwise the source itself, which is every row written before the fork +/// was recorded at all. +/// +/// `Err` is the one repository codeg can never push to: a fork it cannot name +/// (GitLab's unresolved `project-{id}` placeholder, or a fork deleted since the +/// row was written). +fn delivery_push_repo(meta: &ForgeSourceMeta) -> Result { let recorded = meta .head_repo .as_deref() + .or(meta.fork_repo.as_deref()) .map(str::trim) .filter(|r| !r.is_empty()) .unwrap_or(&meta.owner_repo); crate::forge::normalize_repo(recorded).ok_or_else(|| { format!( - "{} #{} comes from a fork whose repository codeg cannot see (it may be private or \ - deleted), so there is nowhere to push the work back to — its commits stay on the \ - task's local branch", + "{} #{} would be delivered from a repository codeg cannot see ({recorded}; it may be \ + private, deleted, or a fork this build cannot name), so there is nowhere to push the \ + work — its commits stay on the task's local branch", meta.provider.change_noun(), meta.number ) }) } +/// The `head` a create-pull request takes. +/// +/// The branch alone when the head repository IS the target, and `owner:branch` +/// when it is not — the qualified form GitHub and Gitea both accept, and the +/// only way to say "from my fork" without a second field. GitLab is excluded +/// because it does not read a qualified head at all: a cross-project merge +/// request is addressed to the FORK and names the target in its body (see +/// [`NewPullRequest`]), so its `source_branch` stays bare. +fn head_ref_for( + provider: ForgeProvider, + push_repo: &str, + target_repo: &str, + branch: &str, +) -> String { + if provider == ForgeProvider::GitLab || crate::forge::same_repo(push_repo, target_repo) { + return branch.to_string(); + } + let owner = push_repo.split('/').next().unwrap_or(push_repo); + format!("{owner}:{branch}") +} + /// Pick the launch mode for a pump-driven launch from the task's history: a /// task with a prior conversation continues (retry semantics); a pristine one /// starts fresh. Explicit returns launch directly with `LaunchMode::Return`. @@ -9227,7 +9314,36 @@ mod tests { /// `(repository, work branch, remote branch)` of every push. pushes: Mutex>, created: Mutex>, + /// `(source project id, target project id)` of every create, in order. + /// GitLab's way of naming both ends of a cross-project merge request — + /// the fork it is created ON and the project it is aimed at — and the + /// pair a same-project create must leave out entirely. + source_projects: Mutex, Option)>>, + /// `(repository the search was AIMED at, head branch)` of every + /// lookup — the head side of a cross-repository delivery is a claim + /// about which repository that must be, and this is what makes it + /// observable from a test. + /// `(repository searched, head repository, head branch)` of every + /// lookup. The search's aim is the one that decides whether a retry + /// sees what it already opened — a pull request lives in the repository + /// being merged INTO — so the fixtures record both sides and the + /// cross-repository tests assert them; `collection_owner` below is what + /// makes a wrong aim fail rather than merely be logged. + finds: Mutex>, + /// Which repository's collection really holds `existing`. + /// + /// A forge lists what its own collection contains, so a search aimed + /// anywhere else comes back empty. `None` = the fixture does not model + /// it, which is fine for a same-repository delivery (the head and the + /// collection are the same repository, so the aim cannot diverge); the + /// cross-repository fixtures set it, so aiming at the head fails the + /// adoption assertions instead of passing quietly. + collection_owner: Mutex>, existing: Mutex>, + /// OID of the branch the last push published — what a real forge + /// records as the new pull request's head, and therefore what the + /// four-way match compares against on the next delivery. + pushed_head: Mutex>, /// What the source repository's base branch points at. The fixture /// seeds it with the task's own base, i.e. "nothing unpushed". remote_base: Mutex>, @@ -9282,6 +9398,11 @@ mod tests { work_branch.to_string(), remote_branch.to_string(), )); + // What the forge now sees on that branch — the OID a created pull + // request will report, and the anchor every later delivery is + // matched against. + *self.pushed_head.lock().await = + task_git::rev_parse(worktree_path, work_branch).await.ok(); if let Some(changed) = self.after_push.lock().await.clone() { *self.existing.lock().await = vec![changed]; } @@ -9290,12 +9411,22 @@ mod tests { async fn find_pulls( &self, - _ctx: &DeliveryCtx<'_>, - _head_branch: &str, + ctx: &DeliveryCtx<'_>, + head_repo: &str, + head_branch: &str, ) -> Result, String> { if let Some(e) = &self.find_error { return Err(e.clone()); } + self.finds.lock().await.push(( + ctx.owner_repo.to_string(), + head_repo.to_string(), + head_branch.to_string(), + )); + let owner = self.collection_owner.lock().await.clone(); + if owner.is_some_and(|owner| owner != ctx.owner_repo) { + return Ok(Vec::new()); + } Ok(self.existing.lock().await.clone()) } @@ -9353,7 +9484,7 @@ mod tests { async fn create_pull( &self, - _ctx: &DeliveryCtx<'_>, + ctx: &DeliveryCtx<'_>, req: &NewPullRequest<'_>, ) -> Result { if let Some(e) = &self.create_error { @@ -9365,16 +9496,44 @@ mod tests { req.base.to_string(), req.draft, )); - Ok(ForgePr { + self.source_projects + .lock() + .await + .push((req.source_project_id, req.target_project_id)); + // The forge's own reading of the request. GitLab names a foreign + // SOURCE PROJECT by number in every list payload — reporting it + // that way is what makes the claim path's placeholder substitution + // load-bearing rather than decorative. The other two spell a fork + // as `owner:branch`, so the head lives in `owner/{name}` while the + // pull request itself sits in the target. + let head_repo = if let Some(id) = req.source_project_id { + format!("project-{id}") + } else if let Some((owner, _)) = req.head.split_once(':') { + match ctx.owner_repo.split_once('/') { + Some((_, name)) => format!("{owner}/{name}"), + None => ctx.owner_repo.to_string(), + } + } else { + ctx.owner_repo.to_string() + }; + let pr = ForgePr { number: 42, html_url: "https://github.test/acme/app/pull/42".to_string(), state: "open".to_string(), merged: false, - head_sha: "unused-by-the-fake".to_string(), - head_ref: req.head.to_string(), - head_repo: "acme/app".to_string(), + head_sha: self + .pushed_head + .lock() + .await + .clone() + .unwrap_or_else(|| "unused-by-the-fake".to_string()), + head_ref: req.head.split_once(':').map_or(req.head, |(_, b)| b).to_string(), + head_repo, base_ref: req.base.to_string(), - }) + }; + // Published: the NEXT delivery of the same task has to find it. + self.existing.lock().await.push(pr.clone()); + Ok(pr) } } @@ -9528,6 +9687,33 @@ mod tests { } } + /// The fixture's task re-pointed at GitLab, with the fork's project id the + /// trigger would have resolved (`resolve_project_id`). + async fn set_gitlab_fork(f: &Delivery, fork_repo: &str, fork_project_id: i64, target_id: i64) { + use sea_orm::{ActiveModelTrait, Set}; + + let task = row(&f.engine, f.task_id).await; + let mut meta: ForgeSourceMeta = + serde_json::from_str(task.source_meta.as_deref().expect("meta")).expect("decode"); + meta.provider = crate::forge::ForgeProvider::GitLab; + meta.fork_repo = Some(fork_repo.to_string()); + meta.fork_project_id = Some(fork_project_id); + meta.owner_project_id = Some(target_id); + // Same as `set_fork`: the fixture's merge requests are the source + // project's, not the fork's. + f.forge + .collection_owner + .lock() + .await + .replace("acme/app".to_string()); + let mut active: crate::db::entities::work_task::ActiveModel = task.into(); + active.source_meta = Set(Some(serde_json::to_string(&meta).expect("encode"))); + active + .update(&f.engine.db.conn) + .await + .expect("record the gitlab fork"); + } + async fn row(engine: &Arc, id: i32) -> crate::db::entities::work_task::Model { work_task_service::get_model(&engine.db.conn, id) .await @@ -9576,6 +9762,187 @@ mod tests { assert!(f.engine.merging.lock().await.is_empty()); } + /// The fixture's task with a recorded FORK — what the trigger writes when + /// the panel's folder points at a parent (the picker's selection is the + /// SOURCE there; the work is pushed to the folder's own `origin`). + async fn set_fork(f: &Delivery, fork_repo: &str) { + use sea_orm::{ActiveModelTrait, Set}; + + let task = row(&f.engine, f.task_id).await; + let mut meta: ForgeSourceMeta = + serde_json::from_str(task.source_meta.as_deref().expect("meta")).expect("decode"); + meta.fork_repo = Some(fork_repo.to_string()); + let mut active: crate::db::entities::work_task::ActiveModel = task.into(); + active.source_meta = Set(Some(serde_json::to_string(&meta).expect("encode"))); + active.update(&f.engine.db.conn).await.expect("record the fork"); + // The fixture's pull requests live in the source's collection, so a + // search aimed at the fork comes back empty — the way a real forge + // answers. + f.forge + .collection_owner + .lock() + .await + .replace("acme/app".to_string()); + } + + /// The row a retry finds: back in review, nothing settled, the delivery's + /// in-flight intent spent. That is the state an interrupted delivery leaves + /// behind, and the one the second attempt has to handle. + async fn reset_to_review(f: &Delivery) { + use sea_orm::{ActiveModelTrait, Set}; + + let task = row(&f.engine, f.task_id).await; + let mut active: crate::db::entities::work_task::ActiveModel = task.into(); + active.status = Set(WorkTaskStatus::Review); + active.completion_kind = Set(None); + active.merge_state = Set(None); + active.finished_at = Set(None); + active.update(&f.engine.db.conn).await.expect("back to review"); + } + + /// The fork workflow end to end: the panel is pointed at the parent, so the + /// issue is the parent's while the branch codeg can write to is the user's + /// own copy. The push has to land THERE and the pull request has to say so — + /// otherwise the account is asked to write to a repository it can only + /// read, and the task fails at the very end, after the agent has done the + /// work. + #[tokio::test] + async fn an_issue_task_pushes_to_its_fork_and_opens_a_cross_repo_pull_request() { + let f = delivery_fixture(FakeForge::default()).await; + set_fork(&f, "me/app").await; + + let url = f + .engine + .deliver_pr(f.task_id, None, false, false) + .await + .expect("delivery"); + + assert_eq!(url, "https://github.test/acme/app/pull/42"); + assert_eq!( + f.forge.pushes.lock().await.as_slice(), + [("me/app".to_string(), "task/7".to_string(), "task/7".to_string())], + "the work branch lands in the fork, not in the source" + ); + assert_eq!( + f.forge.created.lock().await.as_slice(), + [( + // No title was passed, so the task's own title is used. + "#7 · Fix the login flow".to_string(), + "me:task/7".to_string(), + "main".to_string(), + false + )], + "the head is the qualified form that makes the pull request cross-repository" + ); + // The search for an existing pull request: LISTED in the target's + // collection, filtered by the fork that holds it. Aiming the list at + // the head's repository returns nothing — which is how a retry opens a + // second pull request. + assert_eq!( + f.forge.finds.lock().await.as_slice(), + [("acme/app".to_string(), "me/app".to_string(), "task/7".to_string())], + "the collection is the one being merged into, the head filter names the fork" + ); + + let task = row(&f.engine, f.task_id).await; + assert_eq!(task.status, WorkTaskStatus::Done); + assert_eq!(task.completion_kind.as_deref(), Some("delivered_pr")); + } + + /// GitLab's half of the same story: the far side of a cross-project merge + /// request is a project ID (not a qualified head ref), the list payload + /// comes back as `project-{id}` — and the retry still has to ADOPT the merge + /// request the first delivery opened rather than open a second one. + #[tokio::test] + async fn a_gitlab_task_delivers_from_its_fork_and_adopts_its_own_merge_request() { + let f = delivery_fixture(FakeForge::default()).await; + set_gitlab_fork(&f, "me/app", 4711, 4712).await; + + f.engine + .deliver_pr(f.task_id, None, false, false) + .await + .expect("first delivery"); + + assert_eq!( + f.forge.pushes.lock().await.as_slice(), + [("me/app".to_string(), "task/7".to_string(), "task/7".to_string())] + ); + assert_eq!( + f.forge.created.lock().await.as_slice(), + [( + "#7 · Fix the login flow".to_string(), + // The branch stays BARE on GitLab: the other side is the id. + "task/7".to_string(), + "main".to_string(), + false + )] + ); + assert_eq!( + f.forge.source_projects.lock().await.as_slice(), + [(Some(4711), Some(4712))], + "a cross-project merge request names BOTH ends: the fork it is created \ + on and the project it is aimed at" + ); + // What the forge now serves — and what a retry has to recognise. + assert!( + f.forge + .existing + .lock() + .await + .iter() + .any(|pr| pr.head_repo == "project-4711"), + "the list payload names the fork by number" + ); + + reset_to_review(&f).await; + f.engine + .deliver_pr(f.task_id, None, false, false) + .await + .expect("second delivery"); + + assert_eq!( + f.forge.created.lock().await.len(), + 1, + "the placeholder must be resolved, or the retry opens a second merge request" + ); + assert_eq!(row(&f.engine, f.task_id).await.status, WorkTaskStatus::Done); + } + + /// The retry half of the same story, and why the head-side aim is + /// load-bearing rather than cosmetic: a second delivery that searched the + /// SOURCE for this branch would come back empty, conclude nothing exists, + /// and open a SECOND pull request for the same head — which the real forge + /// answers with a 422, after the work has already been pushed. + #[tokio::test] + async fn a_retry_adopts_the_cross_repo_pull_request_instead_of_opening_a_second() { + let f = delivery_fixture(FakeForge::default()).await; + set_fork(&f, "me/app").await; + f.engine + .deliver_pr(f.task_id, None, false, false) + .await + .expect("first delivery"); + assert_eq!(f.forge.created.lock().await.len(), 1); + + reset_to_review(&f).await; + f.engine + .deliver_pr(f.task_id, None, false, false) + .await + .expect("second delivery"); + + assert_eq!( + f.forge.created.lock().await.len(), + 1, + "the pull request the first delivery opened must be adopted, not duplicated" + ); + assert_eq!( + f.forge.source_projects.lock().await.as_slice(), + [(None, None)], + "the project ids are GitLab's spelling of the far side — GitHub and \ + Gitea put it in the head instead, and must not send them" + ); + assert_eq!(row(&f.engine, f.task_id).await.status, WorkTaskStatus::Done); + } + /// The delivery's own version of the offer both other acceptances make: /// take the checkout along. Safe here in a way a bare `branch -D` is not — /// by the time this runs the commits it destroys locally are on the forge. @@ -11774,6 +12141,82 @@ mod tests { assert_eq!(row(&f.engine, f.task_id).await.status, WorkTaskStatus::Done); } + /// A review whose head is somebody ELSE's fork is delivered to that fork, + /// not refused up front: whether this account may write there is a + /// server-side fact (the author's "allow edits from maintainers"), so the + /// push is what decides — which is also the only way a maintainer can push + /// a fix into a contributor's review. + #[tokio::test] + async fn a_review_from_a_third_party_fork_is_pushed_there() { + let f = delivery_fixture(FakeForge::default()).await; + as_pull_request_task(&f, open_pull("at-the-old-head", "feature", "contributor/app")) + .await; + f.forge.existing.lock().await.push(open_pull( + "at-the-old-head", + "feature", + "contributor/app", + )); + + f.engine + .deliver_pr(f.task_id, None, false, false) + .await + .expect("pushed back into the review's own fork"); + assert_eq!( + f.forge.pushes.lock().await.as_slice(), + [( + "contributor/app".to_string(), + "task/7".to_string(), + "feature".to_string() + )], + "the branch goes to the fork the review came from, not to the source" + ); + assert_eq!(row(&f.engine, f.task_id).await.status, WorkTaskStatus::Done); + } + + /// And when that fork refuses us — the author never allowed edits by + /// maintainers, or this account has no business there — the refusal has to + /// name the way out rather than just report the forge's own words. The work + /// stays on the task's branch, so a retry after the box is ticked lands. + #[tokio::test] + async fn a_refused_push_into_a_strangers_fork_names_the_way_out() { + let forge = FakeForge { + push_error: Some( + "remote: You are not allowed to push code to this project. error: 403".into(), + ), + ..FakeForge::default() + }; + let f = delivery_fixture(forge).await; + as_pull_request_task(&f, open_pull("at-the-old-head", "feature", "contributor/app")) + .await; + f.forge.existing.lock().await.push(open_pull( + "at-the-old-head", + "feature", + "contributor/app", + )); + + let err = f + .engine + .deliver_pr(f.task_id, None, false, false) + .await + .expect_err("no permission on the fork"); + assert!(err.contains("contributor/app"), "names the fork: {err}"); + assert!( + err.contains("allow edits from maintainers"), + "names the way out: {err}" + ); + assert_eq!( + row(&f.engine, f.task_id).await.status, + WorkTaskStatus::Review, + "the task goes back to review with its work intact" + ); + assert!( + task_git::rev_parse(f.root.to_str().unwrap(), "refs/heads/task/7") + .await + .is_ok(), + "and the commits are still on the branch" + ); + } + /// Everything that would make the push land somewhere it does not belong /// is refused BEFORE the push, with the task untouched in review. #[tokio::test] diff --git a/src/components/forge/forge-issue-detail-sheet.test.tsx b/src/components/forge/forge-issue-detail-sheet.test.tsx index a013fd124e..8f80488267 100644 --- a/src/components/forge/forge-issue-detail-sheet.test.tsx +++ b/src/components/forge/forge-issue-detail-sheet.test.tsx @@ -29,6 +29,7 @@ import type { ForgeCheck, ForgeComment, ForgeCommentList, + ForgeExpectedRepo, ForgeIdentity, ForgeIssueRow, ForgeLabel, @@ -282,18 +283,27 @@ function mount( onRowUpdated?: (updated: ForgeIssueRow) => void onCommentPosted?: (item: { isPr: boolean; number: number }) => void folderId?: number | null + repo?: string | null + /** The pair a write carries — the repository the panel is showing. */ + expected?: ForgeExpectedRepo | null + /** Called when a write comes back refused as stale. */ + onStaleRepository?: () => void } = {} ) { const onOpenChange = handlers.onOpenChange ?? vi.fn() const onStart = handlers.onStart ?? vi.fn() const onRowUpdated = handlers.onRowUpdated ?? vi.fn() const onCommentPosted = handlers.onCommentPosted ?? vi.fn() + const onStaleRepository = handlers.onStaleRepository ?? vi.fn() const view = render( ) - return { onOpenChange, onStart, onRowUpdated, onCommentPosted, view } + return { + onOpenChange, + onStart, + onRowUpdated, + onCommentPosted, + onStaleRepository, + view, + } +} + +/** + * The panel as the PAGE renders it, repository included. + * + * The remote is a page-level fact handed down (see `repoKey` in + * `forge-page.tsx`), so the only way to rehearse a switch of it is to re-render + * this panel with another value — the folder does not move. + */ +function panelWithRepo( + item: ForgeIssueRow | null, + folderId: number, + repo: string | null +) { + return ( + + + + ) } beforeEach(() => { @@ -959,13 +1004,19 @@ describe("ForgeIssueDetailSheet writes", () => { await user.click(screen.getByRole("button", { name: "Comment" })) await waitFor(() => - expect(forgeCreateComment).toHaveBeenCalledWith(7, { - kind: "issue", - number: 42, - // Trimmed before it goes out — a comment padded with what a keyboard - // left behind is one nobody meant to publish. - body: "looks fixed", - }) + expect(forgeCreateComment).toHaveBeenCalledWith( + 7, + { + kind: "issue", + number: 42, + // Trimmed before it goes out — a comment padded with what a keyboard + // left behind is one nobody meant to publish. + body: "looks fixed", + }, + // No repository named by this caller — see the test below for the one + // that does. + null + ) ) expect(await screen.findByText("looks fixed")).toBeInTheDocument() expect(screen.getByText("alice")).toBeInTheDocument() @@ -1008,6 +1059,93 @@ describe("ForgeIssueDetailSheet writes", () => { expect(submit).toBeDisabled() }) + /** What the backend answers with when the coordinates a write carried no + * longer match the folder's remote (see `WRITE_MISMATCH_I18N_KEY`). */ + const writeMismatch = { + code: "configuration_invalid", + message: + "this panel was showing github.com/me/app, but the folder's remote is now github.com/acme/app", + i18n_key: "Forge.writeMismatch", + i18n_params: { + expected: "github.com/me/app", + actual: "github.com/acme/app", + }, + } + + it("carries the repository this panel is showing on the post", async () => { + const user = userEvent.setup() + forgeListComments.mockResolvedValue(commentPage([])) + forgeCreateComment.mockResolvedValue(comment({ id: "991", body: "hi" })) + mount(row(), null, { + expected: { + expectedServerHost: "github.com", + expectedOwnerRepo: "me/app", + }, + }) + await screen.findByText("No comments yet") + + await user.type(screen.getByPlaceholderText("Leave a comment…"), "hi") + await user.click(screen.getByRole("button", { name: "Comment" })) + + await waitFor(() => + expect(forgeCreateComment).toHaveBeenCalledWith( + 7, + expect.objectContaining({ body: "hi" }), + { expectedServerHost: "github.com", expectedOwnerRepo: "me/app" } + ) + ) + }) + + it("hands a stale-repository refusal to the page instead of only reporting it", async () => { + const user = userEvent.setup() + forgeListComments.mockResolvedValue(commentPage([])) + forgeCreateComment.mockRejectedValue(writeMismatch) + const { onStaleRepository, onCommentPosted } = mount(row(), null, { + expected: { + expectedServerHost: "github.com", + expectedOwnerRepo: "me/app", + }, + }) + await screen.findByText("No comments yet") + + await user.type( + screen.getByPlaceholderText("Leave a comment…"), + "meant for the fork" + ) + await user.click(screen.getByRole("button", { name: "Comment" })) + + // The panel is stale, so re-resolving is the only fix — this is the + // callback the page turns into one. Nothing was published, and nothing was + // adopted as if it had been. + await waitFor(() => expect(onStaleRepository).toHaveBeenCalled()) + expect(onCommentPosted).not.toHaveBeenCalled() + }) + + it("does the same for a close the folder has moved out from under", async () => { + const user = userEvent.setup() + forgeSetItemState.mockRejectedValue(writeMismatch) + const { onStaleRepository, onRowUpdated } = mount(row(), null, { + expected: { + expectedServerHost: "github.com", + expectedOwnerRepo: "me/app", + }, + }) + + await user.click( + screen.getByRole("button", { name: "Close #42 on the forge" }) + ) + await user.click( + within(screen.getByRole("alertdialog")).getByRole("button", { + name: "Close", + }) + ) + + await waitFor(() => expect(onStaleRepository).toHaveBeenCalled()) + // Not flipped locally: the write never happened, and pretending it did is + // the guess this panel refuses to make everywhere else. + expect(onRowUpdated).not.toHaveBeenCalled() + }) + it("confirms a close, then adopts the row the forge answered with", async () => { const user = userEvent.setup() // GitHub's PATCH answers with bare label names on GitLab; here the point @@ -1030,11 +1168,15 @@ describe("ForgeIssueDetailSheet writes", () => { }) ) await waitFor(() => - expect(forgeSetItemState).toHaveBeenCalledWith(7, { - kind: "issue", - number: 42, - action: "close", - }) + expect(forgeSetItemState).toHaveBeenCalledWith( + 7, + { + kind: "issue", + number: 42, + action: "close", + }, + null + ) ) await waitFor(() => expect(onRowUpdated).toHaveBeenCalled()) const adopted = vi.mocked(onRowUpdated).mock.calls[0][0] as ForgeIssueRow @@ -2133,14 +2275,18 @@ describe("ForgeIssueDetailSheet merge box", () => { await user.click(screen.getByRole("button", { name: "Squash and merge" })) await waitFor(() => - expect(forgeMergeChange).toHaveBeenCalledWith(7, { - number: 42, - method: "squash", - // The commit the panel DECIDED on — its diff, its files and its checks - // all describe this one. Both forges refuse with a 409 if the branch - // has moved, which is the point of sending it. - headSha: "abc123", - }) + expect(forgeMergeChange).toHaveBeenCalledWith( + 7, + { + number: 42, + method: "squash", + // The commit the panel DECIDED on — its diff, its files and its checks + // all describe this one. Both forges refuse with a 409 if the branch + // has moved, which is the point of sending it. + headSha: "abc123", + }, + null + ) ) // The FORGE's row, not a local flip: GitHub has no merged state, and only // its answer knows this one landed rather than closed. @@ -2209,7 +2355,8 @@ describe("ForgeIssueDetailSheet merge box", () => { await waitFor(() => expect(forgeMergeChange).toHaveBeenCalledWith( 7, - expect.objectContaining({ headSha: "reviewed1" }) + expect.objectContaining({ headSha: "reviewed1" }), + null ) ) }) @@ -2676,6 +2823,121 @@ describe("ForgeIssueDetailSheet conversation rail", () => { const column = card?.previousElementSibling expect(column?.firstElementChild).toHaveClass("rounded-full") }) + + /** + * The repository is part of what the panel's own lookups are ABOUT, so a + * switch of it has to blank them, not just re-ask. + * + * `forgeIdentity` is asked for by FOLDER and answered about whatever remote + * that folder names — and the real backend reads the same folder, so the + * answer for the folder really does change under a switch. Keyed on the folder + * alone the panel went on naming the previous repository's account until its + * own request came back, over a row that had already been re-read from the new + * one. The window is the whole point: both requests are held open here, since + * an answer that resolved would paper over the stale one a moment later. + */ + it("names no account from the repository the switch left, not even for a frame", async () => { + const settle = new Map void>() + forgeIdentity.mockImplementation( + (folderId: number) => + new Promise((resolve) => { + settle.set(folderId, resolve) + }) + ) + + const { view } = mount(row(), null, { + folderId: 7, + repo: "github.com/me/codeg", + }) + await waitFor(() => expect(forgeIdentity).toHaveBeenCalledWith(7)) + settle.get(7)?.({ username: "on-the-fork", avatar_url: null }) + await waitFor(() => + expect( + screen.getByRole("img", { name: "Commenting as on-the-fork" }) + ).toBeInTheDocument() + ) + + // Same folder, other repository — the picker in action. + view.rerender(panelWithRepo(row(), 7, "github.com/acme/codeg-parent")) + + // The new repository's lookup has gone out and has not answered, and the + // fork's account must not be standing in for it. + await waitFor(() => expect(forgeIdentity).toHaveBeenCalledTimes(2)) + expect(screen.queryByRole("img", { name: /Commenting as/ })).toBeNull() + + settle.get(7)?.({ username: "on-the-parent", avatar_url: null }) + await waitFor(() => + expect( + screen.getByRole("img", { name: "Commenting as on-the-parent" }) + ).toBeInTheDocument() + ) + }) + + /** The same rule for the merge box: which methods the forge permits is a fact + * about the repository, and the box must not go on offering the fork's while + * the parent's is on screen. Held open, as the account lookup is — the claim + * is about the window before the new answer lands. */ + it("withholds the merge on a repository switch until the new one answers", async () => { + forgeChangeDetail.mockResolvedValue({ + number: 42, + base_ref: "main", + head_ref: "fix/timeout", + head_repo: null, + head_sha: "abc123", + draft: false, + state: "open", + mergeable: true, + merge_state: "clean", + additions: 1, + deletions: 1, + changed_files: 1, + commits: 1, + checks: { checks: [], available: true, partial: false }, + }) + // Held in a list rather than a `let … | null`: the resolve is stored from + // inside the mock, and TypeScript's control-flow analysis cannot see an + // assignment made in a callback, so it narrows a `let` back to `null` at + // the call site and refuses the call outright. + const settles: Array<(value: ForgeMergeOptions) => void> = [] + forgeMergeOptions + .mockResolvedValueOnce({ + methods: ["squash"], + default_method: "squash", + merge_strategy: "squash", + }) + .mockImplementationOnce( + () => + new Promise((resolve) => { + settles.push(resolve) + }) + ) + + const mergeButton = () => + screen.getByRole("button", { name: /^(Merge|Merging…)$/ }) + const { view } = mount(row({ is_pr: true }), null, { + folderId: 7, + repo: "github.com/me/codeg", + }) + await waitFor(() => expect(mergeButton()).toBeEnabled()) + + view.rerender( + panelWithRepo(row({ is_pr: true }), 7, "github.com/acme/codeg-parent") + ) + + // Blanked rather than carried over: the method the fork prefers is a claim + // about a repository this panel is no longer reading, and the box says so + // by not being willing to merge until the new repository has answered. + await waitFor(() => expect(forgeMergeOptions).toHaveBeenCalledTimes(2)) + expect(mergeButton()).toBeDisabled() + + // And the parent's own answer still lands when it comes. + settles[0]?.({ + methods: ["merge"], + default_method: "merge", + merge_strategy: "merge_commit", + }) + await waitFor(() => expect(mergeButton()).toBeEnabled()) + }) }) /** diff --git a/src/components/forge/forge-issue-detail-sheet.tsx b/src/components/forge/forge-issue-detail-sheet.tsx index 02a7015763..9833c9d3d3 100644 --- a/src/components/forge/forge-issue-detail-sheet.tsx +++ b/src/components/forge/forge-issue-detail-sheet.tsx @@ -96,6 +96,7 @@ import { forgeSetItemState, } from "@/lib/api" import { + isForgeWriteMismatch, type AppErrorTranslator, toLocalizedErrorMessage, } from "@/lib/app-error" @@ -110,6 +111,7 @@ import type { ForgeCheckList, ForgeCheckState, ForgeComment, + ForgeExpectedRepo, ForgeIdentity, ForgeIssueRow, ForgeMergeMethod, @@ -275,6 +277,8 @@ function CommentThread({ kind, number, identity, + expected, + onStaleRepository, onPosted, beforeComposer, viewportRef, @@ -287,6 +291,12 @@ function CommentThread({ * thread is keyed by the ITEM and remounts as the reader clicks the list, * while the identity is a property of the folder. */ identity: ForgeIdentity | null + /** The repository on screen, passed to the composer so a post is REFUSED + * rather than landing in whichever repository the folder reads by the time + * it arrives. See [`CommentComposer`]. */ + expected: ForgeExpectedRepo | null + /** That refusal reached the composer: the page re-resolves. */ + onStaleRepository: () => void /** A comment landed on the forge, and here it is. The caller bumps the * item's count so the header stops trailing the thread underneath it. */ onPosted: (comment: ForgeComment) => void @@ -487,6 +497,8 @@ function CommentThread({ kind={kind} number={number} identity={identity} + expected={expected} + onStaleRepository={onStaleRepository} onPosted={(comment) => { // Into its own slot, not into the paged collection — see `posted` // for why that ordering and that race both matter. Nothing is @@ -674,6 +686,8 @@ function CommentComposer({ kind, number, identity, + expected, + onStaleRepository, onPosted, }: { folderId: number @@ -682,6 +696,13 @@ function CommentComposer({ /** Who the comment would be signed as, or `null` while that is still being * resolved — or could not be. See [`useForgeIdentity`]. */ identity: ForgeIdentity | null + /** The repository the panel is showing, so the post can be REFUSED rather + * than land in whichever one the folder's selection names by the time it + * arrives. `null` = name nothing, which is the old behaviour. */ + expected: ForgeExpectedRepo | null + /** The refusal above means the panel is stale: hand it to the page, which + * re-resolves and tears this panel down with it. */ + onStaleRepository: () => void onPosted: (comment: ForgeComment) => void }) { const t = useTranslations("Forge") @@ -698,21 +719,46 @@ function CommentComposer({ setPosting(true) setFailure(null) try { - const comment = await forgeCreateComment(folderId, { - kind, - number, - body: trimmed, - }) + const comment = await forgeCreateComment( + folderId, + { + kind, + number, + body: trimmed, + }, + expected + ) // Only now — a draft cleared before the answer would lose what somebody // wrote to a network failure they cannot retry from. setBody("") onPosted(comment) } catch (error) { + if (isForgeWriteMismatch(error)) { + // The folder has moved to another repository, so this panel is stale + // and the page is about to re-resolve — which unmounts this composer + // and the strip with it. A toast survives the teardown, so the reason + // is still readable after the panel it belonged to is gone. + toast.error( + toLocalizedErrorMessage(error, tRoot as unknown as AppErrorTranslator) + ) + onStaleRepository() + return + } setFailure({ error }) } finally { setPosting(false) } - }, [folderId, kind, number, onPosted, posting, trimmed]) + }, [ + expected, + folderId, + kind, + number, + onPosted, + onStaleRepository, + posting, + tRoot, + trimmed, + ]) return (
@@ -846,6 +892,11 @@ type FileStatusLabelKey = const RAIL = "flex gap-2.5" const RAIL_BODY = "min-w-0 flex-1" +/** Stands in for a caller that wired no re-resolve — a fixture, a preview. A + * fresh arrow per render would also invalidate every `useCallback` that + * depends on it, which is the other reason this is one shared value. */ +const NO_OP = () => {} + /** * The gutter's own column, and what pins what sits in it. * @@ -1867,29 +1918,34 @@ function mergeMethodText( */ function useForgeIdentity( folderId: number | null, + repo: string | null | undefined, enabled: boolean ): ForgeIdentity | null { const [identity, setIdentity] = useState(null) - /** The folder the answer above describes. */ - const [shown, setShown] = useState(null) + /** The folder AND remote the answer above describes. Both, because the + * folder picks the repository only until the picker points it somewhere + * else — the account belongs to the repository, and one folder can name + * several in turn. */ + const [shown, setShown] = useState(null) const reqRef = useRef(0) + const key = `${folderId}:${repo}` // Absorbed during RENDER, as [`useMergeOptions`] does: an effect would commit // one frame naming the account of the repository the panel just left. Keyed - // on the FOLDER alone, so closing the panel keeps the answer rather than - // blanking the avatar every time it is reopened. - if (folderId !== shown) { - setShown(folderId) + // on the repository rather than on the folder alone — so closing the panel + // keeps the answer rather than blanking the avatar every time it is reopened, + // while a switch to another remote DOES blank it. + if (key !== shown) { + setShown(key) setIdentity(null) } useEffect(() => { // Claimed BEFORE the early return, so a run that asks for nothing still // invalidates whatever the last one had in flight. Otherwise a lookup for - // the folder the panel was last opened on lands after the reader has - // switched repositories — the reset above has already been and gone by - // then, because it keys on the folder and the folder stopped changing — - // and the next open names an account from the repository before this one. + // the repository the panel was last opened on lands after the reader has + // switched to another — the reset above has already been and gone by then + // — and the next open names an account from the repository before this one. const id = ++reqRef.current if (folderId == null || !enabled) return void forgeIdentity(folderId) @@ -1899,7 +1955,7 @@ function useForgeIdentity( .catch(() => { if (id === reqRef.current) setIdentity(null) }) - }, [folderId, enabled]) + }, [folderId, repo, enabled]) return identity } @@ -1924,18 +1980,21 @@ const FALLBACK_METHODS: readonly ForgeMergeMethod[] = ["merge"] */ function useMergeOptions( folderId: number | null, + repo: string | null | undefined, enabled: boolean ): ForgeMergeOptions | null { const [options, setOptions] = useState(null) - /** The folder the answer above describes, so a folder switch cannot leave - * one repository's permitted methods on another's button. */ - const [shown, setShown] = useState(null) + /** The folder AND remote the answer above describes, so neither a folder + * switch nor a switch of the remote within it can leave one repository's + * permitted methods on another's button. */ + const [shown, setShown] = useState(null) const reqRef = useRef(0) + const key = `${folderId}:${repo}` // Absorbed during RENDER, the same rule `useChangeDetail` follows: an effect // would commit one frame of the previous repository's menu. - if (folderId !== shown) { - setShown(folderId) + if (key !== shown) { + setShown(key) setOptions(null) } @@ -1955,7 +2014,7 @@ function useMergeOptions( }) } }) - }, [folderId, enabled]) + }, [folderId, repo, enabled]) return options } @@ -2209,6 +2268,9 @@ function MergeBox({ function Conversation({ row, folderId, + repo, + expected, + onStaleRepository, identity, onCommentPosted, beforeComposer, @@ -2217,6 +2279,13 @@ function Conversation({ }: { row: ForgeIssueRow folderId: number | null + /** The repository `folderId` is pointed at — part of the thread's key, so a + * switch of the remote resets it exactly as a switch of the item does. */ + repo?: string | null + /** The same repository as the pair every write carries. See + * [`ForgeIssueDetailSheet`] — this only passes it down. */ + expected: ForgeExpectedRepo | null + onStaleRepository: () => void /** Who a comment from here would be signed as — see [`useForgeIdentity`]. */ identity: ForgeIdentity | null onCommentPosted: (item: { isPr: boolean; number: number }) => void @@ -2267,20 +2336,25 @@ function Conversation({
- {/* Keyed by the ITEM, not by the row object: the page re-reads the row - from the list on every render, so identity changes whenever anything - behind the panel refreshes — and a thread that remounted on each of - those would re-fetch, lose its loaded pages and scroll the reader back - to the top. The panel is non-modal, though, so clicking a different - row swaps the item underneath without ever closing; the key is what - resets it when that happens. */} + {/* Keyed by the ITEM *and the repository*, not by the row object: the page + re-reads the row from the list on every render, so identity changes + whenever anything behind the panel refreshes — and a thread that + remounted on each of those would re-fetch, lose its loaded pages and + scroll the reader back to the top. The panel is non-modal, though, so + clicking a different row swaps the item underneath without ever + closing; the key is what resets it when that happens. The repository + belongs in the key for the same reason the item does — the same number + names a different item in another repository, and its comments would + be the wrong thread entirely. */} {folderId != null ? ( void onOpenChange: (open: boolean) => void /** Opens the page's trigger dialog on this item. */ onStart: () => void @@ -2440,11 +2545,11 @@ export function ForgeIssueDetailSheet({ change != null && row?.state === "open" && (detail.detail?.state ?? "open") === "open" - const mergeOptions = useMergeOptions(change?.folderId ?? null, canMerge) + const mergeOptions = useMergeOptions(change?.folderId ?? null, repo, canMerge) /** Held HERE, above the thread that remounts per item — the account is a - * property of the folder, not of the item being read. Gated on the panel + * property of the repository, not of the item being read. Gated on the panel * being open, because the drawer is mounted whether or not it is. */ - const identity = useForgeIdentity(folderId, row != null) + const identity = useForgeIdentity(folderId, repo, row != null) /** * The element the conversation is scrolled in, for the virtualized thread @@ -2497,11 +2602,15 @@ export function ForgeIssueDetailSheet({ if (row == null || folderId == null) return setChanging(true) try { - const updated = await forgeSetItemState(folderId, { - kind: row.is_pr ? "pr" : "issue", - number: row.number, - action, - }) + const updated = await forgeSetItemState( + folderId, + { + kind: row.is_pr ? "pr" : "issue", + number: row.number, + action, + }, + expected + ) setPendingAction(null) onRowUpdated(mergeForgeRowUpdate(row, updated)) } catch (error) { @@ -2510,11 +2619,14 @@ export function ForgeIssueDetailSheet({ toast.error( toLocalizedErrorMessage(error, tRoot as unknown as AppErrorTranslator) ) + // Refused because the folder has moved on: the page re-resolves, and + // this panel goes with it. The toast above outlives both. + if (isForgeWriteMismatch(error)) onStaleRepository() } finally { setChanging(false) } }, - [folderId, onRowUpdated, row, tRoot] + [expected, folderId, onRowUpdated, onStaleRepository, row, tRoot] ) const reloadDetail = detail.reload @@ -2523,16 +2635,20 @@ export function ForgeIssueDetailSheet({ if (row == null || folderId == null) return setMerging(true) try { - const updated = await forgeMergeChange(folderId, { - number: row.number, - method: pending.method, - // The commit the DIALOG was armed with, not whatever the panel holds - // now. The diff, the file list and the checks all describe that one, - // so a merge that quietly landed a newer one would land code nobody - // in this conversation ever saw. Both forges answer 409 if the branch - // has moved, in their own words. - headSha: pending.headSha, - }) + const updated = await forgeMergeChange( + folderId, + { + number: row.number, + method: pending.method, + // The commit the DIALOG was armed with, not whatever the panel holds + // now. The diff, the file list and the checks all describe that one, + // so a merge that quietly landed a newer one would land code nobody + // in this conversation ever saw. Both forges answer 409 if the branch + // has moved, in their own words. + headSha: pending.headSha, + }, + expected + ) setPendingMerge(null) // `null` is "it merged, and the row could not be read back" — GitHub's // merge response does not contain the pull request, so the row costs a @@ -2560,6 +2676,11 @@ export function ForgeIssueDetailSheet({ toast.error( toLocalizedErrorMessage(error, tRoot as unknown as AppErrorTranslator) ) + // A refusal because the folder has moved on is the one failure here + // the page can FIX (see the prop's note) — and it must, because every + // later action in this panel would be aimed at the same stale + // repository. + if (isForgeWriteMismatch(error)) onStaleRepository() // The confirmation is DISMISSED on failure, unlike the close/reopen // one that stays put. It has to be: the likeliest refusal is "Head // branch was modified. Review and try the merge again.", and the whole @@ -2575,7 +2696,16 @@ export function ForgeIssueDetailSheet({ setMerging(false) } }, - [folderId, onRowUpdated, reloadDetail, row, t, tRoot] + [ + expected, + folderId, + onRowUpdated, + onStaleRepository, + reloadDetail, + row, + t, + tRoot, + ] ) if (row == null) return null @@ -2707,6 +2837,9 @@ export function ForgeIssueDetailSheet({ = {}): ForgeIssueRow { } } -function mount(labelOptions: ForgeLabel[] = []) { +function mount( + labelOptions: ForgeLabel[] = [], + handlers: { + expected?: ForgeExpectedRepo | null + onStaleRepository?: () => void + } = {} +) { const onOpenChange = vi.fn() const onCreated = vi.fn() + const onStaleRepository = handlers.onStaleRepository ?? vi.fn() render( ) - return { onOpenChange, onCreated } + return { onOpenChange, onCreated, onStaleRepository } } beforeEach(() => { @@ -112,13 +121,19 @@ describe("ForgeNewIssueDialog", () => { await user.click(screen.getByRole("button", { name: "Create issue" })) await waitFor(() => - expect(forgeCreateIssue).toHaveBeenCalledWith(7, { - title: "Login times out", - // Null, not "": GitHub stores an empty string as a body and the issue - // then renders an empty description block. - body: null, - labels: ["bug"], - }) + expect(forgeCreateIssue).toHaveBeenCalledWith( + 7, + { + title: "Login times out", + // Null, not "": GitHub stores an empty string as a body and the issue + // then renders an empty description block. + body: null, + labels: ["bug"], + }, + // No repository named by this caller — the page passes one in + // production (see the test below). + null + ) ) // The forge's row — the number and the URL only exist once it is written. expect(onCreated).toHaveBeenCalledWith( @@ -147,6 +162,56 @@ describe("ForgeNewIssueDialog", () => { // A picker that can only ever open an empty list is worse than no picker. expect(screen.queryByText("Labels")).not.toBeInTheDocument() }) + + it("carries the repository the dialog was opened over", async () => { + const user = userEvent.setup() + forgeCreateIssue.mockResolvedValue(created()) + mount([], { + expected: { + expectedServerHost: "github.com", + expectedOwnerRepo: "me/app", + }, + }) + + await user.type(screen.getByLabelText("Title"), "Login times out") + await user.click(screen.getByRole("button", { name: "Create issue" })) + + await waitFor(() => + expect(forgeCreateIssue).toHaveBeenCalledWith( + 7, + expect.objectContaining({ title: "Login times out" }), + { expectedServerHost: "github.com", expectedOwnerRepo: "me/app" } + ) + ) + }) + + it("tells the page to re-resolve when the folder has moved on", async () => { + const user = userEvent.setup() + // What the backend answers with when the coordinates no longer match the + // folder's remote (`WRITE_MISMATCH_I18N_KEY`). + forgeCreateIssue.mockRejectedValue({ + code: "configuration_invalid", + message: "this panel was showing github.com/me/app", + i18n_key: "Forge.writeMismatch", + i18n_params: { + expected: "github.com/me/app", + actual: "github.com/acme/app", + }, + }) + const { onStaleRepository, onCreated } = mount([], { + expected: { + expectedServerHost: "github.com", + expectedOwnerRepo: "me/app", + }, + }) + + await user.type(screen.getByLabelText("Title"), "Login times out") + await user.click(screen.getByRole("button", { name: "Create issue" })) + + await waitFor(() => expect(onStaleRepository).toHaveBeenCalled()) + // Nothing was filed, and the dialog does not pretend otherwise. + expect(onCreated).not.toHaveBeenCalled() + }) }) /** diff --git a/src/components/forge/forge-new-issue-dialog.tsx b/src/components/forge/forge-new-issue-dialog.tsx index 6c0248f0ca..6dc85bf2a5 100644 --- a/src/components/forge/forge-new-issue-dialog.tsx +++ b/src/components/forge/forge-new-issue-dialog.tsx @@ -3,6 +3,7 @@ import { useCallback, useState } from "react" import { useTranslations } from "next-intl" import { Plus } from "lucide-react" +import { toast } from "sonner" import { ForgeLabelChip } from "@/components/forge/forge-issue-row" import { Button } from "@/components/ui/button" import { @@ -19,11 +20,15 @@ import { Textarea } from "@/components/ui/textarea" import { useImeGuard } from "@/hooks/use-ime-guard" import { forgeCreateIssue } from "@/lib/api" import { + isForgeWriteMismatch, type AppErrorTranslator, toLocalizedErrorMessage, } from "@/lib/app-error" import { cn } from "@/lib/utils" -import type { ForgeIssueRow, ForgeLabel } from "@/lib/types" +import type { ForgeExpectedRepo, ForgeIssueRow, ForgeLabel } from "@/lib/types" + +/** Stands in for a caller that wired no re-resolve (a preview, a fixture). */ +const NO_OP = () => {} /** Mirrors `MAX_TITLE_CHARS` in src-tauri/src/forge/mod.rs. Enforced here as * well as there so the counter and the button agree with what the forge will @@ -52,6 +57,8 @@ export function ForgeNewIssueDialog({ folderId, repo, labelOptions, + expected = null, + onStaleRepository = NO_OP, onOpenChange, onCreated, }: { @@ -59,6 +66,13 @@ export function ForgeNewIssueDialog({ folderId: number /** `owner/repo`, for the description — the backend derives its own. */ repo: string + /** The repository this dialog was opened over, as the pair a write carries + * (see `ForgeExpectedRepo`). `null` sends none, which is the old + * behaviour. */ + expected?: ForgeExpectedRepo | null + /** The write was refused because the folder has moved to another + * repository; the page re-resolves, which closes this dialog. */ + onStaleRepository?: () => void /** The repository's label vocabulary, already fetched by the page. Empty * when it has none (or the read failed), in which case no label control is * offered at all — one that can only show an empty list is worse than none. */ @@ -97,21 +111,45 @@ export function ForgeNewIssueDialog({ setCreating(true) setFailure(null) try { - const row = await forgeCreateIssue(folderId, { - title: trimmedTitle, - body: body.trim() === "" ? null : body.trim(), - labels, - }) + const row = await forgeCreateIssue( + folderId, + { + title: trimmedTitle, + body: body.trim() === "" ? null : body.trim(), + labels, + }, + expected + ) // Only once it exists: clearing before the answer would lose what // somebody wrote to a network failure they cannot retry from. reset() onCreated(row) } catch (error) { + if (isForgeWriteMismatch(error)) { + // The page's re-resolve closes this dialog, so the inline strip would + // be torn down before it could be read — a toast outlives it. + toast.error( + toLocalizedErrorMessage(error, tRoot as unknown as AppErrorTranslator) + ) + onStaleRepository() + return + } setFailure({ error }) } finally { setCreating(false) } - }, [body, canCreate, folderId, labels, onCreated, reset, trimmedTitle]) + }, [ + body, + canCreate, + expected, + folderId, + labels, + onCreated, + onStaleRepository, + reset, + tRoot, + trimmedTitle, + ]) return ( ({ // each of them. forgeSettingsGet: vi.fn(), forgeSettingsSet: vi.fn(), + // The panel's remote selections — read once on mount, written by the picker. + // Its own store, so the settings mocks above never answer for it. + forgeRemoteGet: vi.fn(), + forgeRemoteSet: vi.fn(), + // Called DURING RENDER to build the pair every write carries, so this mock + // has to answer like the real helper rather than like a spy. + forgeExpectedRepo: ( + remote: { server_host: string; owner_repo: string } | null + ) => + remote === null + ? null + : { + expectedServerHost: remote.server_host, + expectedOwnerRepo: remote.owner_repo, + }, + gitListRemotes: vi.fn(), })) vi.mock("@/lib/platform", () => ({ subscribe: vi.fn().mockResolvedValue(() => {}), @@ -109,9 +126,13 @@ import { forgeCreateIssue, forgeListComments, forgeListLabels, + forgeRemoteGet, + forgeRemoteSet, forgeSetItemState, forgeSettingsGet, + forgeSettingsSet, forgeTabCount, + gitListRemotes, workTaskLookupBySource, } from "@/lib/api" @@ -223,6 +244,230 @@ beforeEach(() => { global: { writeback_default: true, scenario_prompts: {} }, folders: {}, }) + // Nothing picked in any folder: the panel reads the default remote. + vi.mocked(forgeRemoteGet).mockResolvedValue({ folders: {} }) + vi.mocked(gitListRemotes).mockResolvedValue([]) +}) + +describe("ForgePage remote picker", () => { + function mountWithRemotes() { + useAppWorkspaceStore.setState({ + folders: [ + { + id: 1, + name: "codeg", + path: "/repo", + parent_id: null, + kind: "regular", + }, + ] as never, + }) + vi.mocked(gitListRemotes).mockResolvedValue([ + { name: "origin", url: "https://github.com/me/codeg.git" }, + { name: "upstream", url: "https://github.com/xintaofei/codeg.git" }, + ]) + vi.mocked(forgeListIssues).mockResolvedValue(listOf([])) + mount() + } + + it("lists the folder's remotes and saves the picked one in its own store", async () => { + mountWithRemotes() + vi.mocked(forgeRemoteSet).mockResolvedValue({ + folders: { "1": "upstream" }, + }) + + await userEvent.click( + await screen.findByRole("combobox", { name: "Remote" }) + ) + await userEvent.click( + await screen.findByRole("option", { name: "upstream" }) + ) + + // Its OWN command. A picker click must not land in the panel-settings blob: + // that is what used to detach the folder from the global row, and what let + // a later "use global defaults" save destroy the choice. + await waitFor(() => + expect(vi.mocked(forgeRemoteSet)).toHaveBeenCalledWith(1, "upstream") + ) + expect(vi.mocked(forgeSettingsSet)).not.toHaveBeenCalled() + // And the page re-reads the repository it is now pointed at. + await waitFor(() => + expect(vi.mocked(folderForgeRemote).mock.calls.length).toBeGreaterThan(1) + ) + }) + + it("offers the default explicitly and clears the choice with it", async () => { + // The folder is on `upstream`. Nothing in the settings dialog edits the + // selection, so this item is the only way back to `origin` — and it has to + // CLEAR the entry rather than save the name, or the folder would look like + // it had chosen `origin` rather than gone back to the default. + vi.mocked(forgeRemoteGet).mockResolvedValue({ + folders: { "1": "upstream" }, + }) + mountWithRemotes() + vi.mocked(forgeRemoteSet).mockResolvedValue({ folders: {} }) + + await userEvent.click( + await screen.findByRole("combobox", { name: "Remote" }) + ) + await userEvent.click( + await screen.findByRole("option", { name: "Default (origin)" }) + ) + + await waitFor(() => + expect(vi.mocked(forgeRemoteSet)).toHaveBeenCalledWith(1, null) + ) + }) +}) + +/** + * What a remote switch must TEAR DOWN. + * + * The picker changes which repository every request is aimed at, so every piece + * of state that was only true of the previous one has to go with it. Left + * behind, each of these reads as a fact about the NEW repository: a page + * number, a label vocabulary, and the previous repository's rows — the last one + * painted by a response that was already in flight when the switch happened. + */ +describe("ForgePage remote switch", () => { + const UPSTREAM: ForgeRemote = { + remote_name: "upstream", + server_host: "github.com", + owner_repo: "acme/codeg-parent", + remote_url: "https://github.com/acme/codeg-parent.git", + provider: "github", + supported: true, + } + + function mountWithPicker() { + useAppWorkspaceStore.setState({ + folders: [ + { + id: 1, + name: "codeg", + path: "/repo", + parent_id: null, + kind: "regular", + }, + ] as never, + }) + vi.mocked(gitListRemotes).mockResolvedValue([ + { name: "origin", url: "https://github.com/me/codeg.git" }, + { name: "upstream", url: "https://github.com/acme/codeg-parent.git" }, + ]) + // The first resolution is the fork; every one after the pick is the + // parent, which is what makes the switch observable from the outside. + vi.mocked(folderForgeRemote) + .mockResolvedValueOnce(REMOTE) + .mockResolvedValue(UPSTREAM) + mount() + } + + async function pickUpstream(user: ReturnType) { + await user.click(await screen.findByRole("combobox", { name: "Remote" })) + await user.click(await screen.findByRole("option", { name: "upstream" })) + } + + it("returns to page 1 instead of asking the new repository for the old page", async () => { + const user = userEvent.setup() + vi.mocked(forgeListIssues).mockImplementation(async (_folderId, req) => + listOf([issue(req.page ?? 1, `row on page ${req.page}`)], { + page: req.page ?? 1, + total_count: 57, + has_next: (req.page ?? 1) < 3, + }) + ) + mountWithPicker() + await screen.findByText("row on page 1") + await user.click(screen.getByRole("button", { name: "Go to page 3" })) + await screen.findByText("row on page 3") + + const before = sentQueries().length + await pickUpstream(user) + + // The FIRST request aimed at the new repository is the one that matters: + // page 3 of one repository is a different slice of another, and a header + // saying "page 3" over rows the reader never asked for is the same bug the + // page-size control already avoids. Asserted over EVERY request since the + // pick rather than only the last one: a fetch for the old page can be + // overtaken by the refetch that corrects it, and the wasted round trip — + // and the rows it briefly paints — would go unnoticed. + await waitFor(() => { + expect(vi.mocked(folderForgeRemote).mock.calls.length).toBeGreaterThan(1) + }) + expect(lastQuery().page ?? 1).toBe(1) + expect( + sentQueries() + .slice(before) + .map((q) => q.page ?? 1) + ).not.toContain(3) + }) + + it("clears the label filter, which belongs to the old repository", async () => { + const user = userEvent.setup() + // The parent's vocabulary does NOT include `bug`. Left in place, the filter + // would come back empty and read as "this repository has no issues". + vi.mocked(forgeListIssues).mockResolvedValue(listOf([issue(1, "a row")])) + vi.mocked(forgeListLabels) + .mockResolvedValueOnce({ + labels: [{ name: "bug", color: "#d73a4a" }], + truncated: false, + }) + .mockResolvedValue({ + labels: [{ name: "feature", color: "#0e8a16" }], + truncated: false, + }) + mountWithPicker() + await screen.findByText("a row") + + await user.click(screen.getByRole("button", { name: "Labels" })) + await user.click(await screen.findByRole("option", { name: "bug" })) + // Closing the popover, so the click that follows reaches the picker. + await user.keyboard("{Escape}") + await waitFor(() => expect(lastQuery().labels).toEqual(["bug"])) + + await pickUpstream(user) + + await waitFor(() => { + expect(vi.mocked(folderForgeRemote).mock.calls.length).toBeGreaterThan(1) + }) + expect(lastQuery().labels ?? []).toEqual([]) + // And the label list on offer is the new repository's, not a filter built + // from names that only existed in the old one. + expect(vi.mocked(forgeListLabels).mock.calls.length).toBeGreaterThan(1) + }) + + it("never paints the previous repository's rows over the new one", async () => { + const user = userEvent.setup() + let releaseOld: ((value: ForgeIssueList) => void) | null = null + let calls = 0 + vi.mocked(forgeListIssues).mockImplementation(async () => { + calls += 1 + // The first page is held in flight and released by the test AFTER the + // switch; the second (the parent's) resolves immediately. + if (calls === 1) { + return new Promise((resolve) => { + releaseOld = resolve + }) + } + return listOf([issue(99, "row from the parent")]) + }) + mountWithPicker() + await screen.findByRole("combobox", { name: "Remote" }) + + await pickUpstream(user) + // The parent's rows land first… + await screen.findByText("row from the parent") + // …and only then does the response that was already in flight arrive, + // carrying a row from the fork the panel has left. + await act(async () => { + releaseOld?.(listOf([issue(1, "row from the fork")])) + await Promise.resolve() + }) + + expect(screen.queryByText("row from the fork")).toBeNull() + expect(screen.getByText("row from the parent")).toBeInTheDocument() + }) }) describe("ForgePage list failures", () => { @@ -376,6 +621,7 @@ describe("ForgePage list failures", () => { */ describe("ForgePage sort control", () => { const GITEA: ForgeRemote = { + remote_name: "origin", server_host: "git.corp.example", owner_repo: "acme/app", remote_url: "https://git.corp.example/acme/app.git", @@ -418,6 +664,7 @@ describe("ForgePage sort control", () => { */ describe("ForgePage on a host that is neither forge", () => { const UNSUPPORTED: ForgeRemote = { + remote_name: "origin", server_host: "gitee.com", owner_repo: "someone/thing", remote_url: "https://gitee.com/someone/thing.git", @@ -1715,9 +1962,7 @@ describe("ForgePage writes", () => { vi.mocked(folderForgeRemote).mockResolvedValue(null) vi.mocked(forgeListIssues).mockResolvedValue(listOf([])) mount() - await screen.findByText( - "This folder has no recognizable forge remote (origin)" - ) + await screen.findByText("This folder has no recognizable forge remote") // Nowhere for the issue to go — the backend would refuse it, and a button // that can only fail is worse than no button. expect( @@ -1742,7 +1987,13 @@ describe("ForgePage writes", () => { await waitFor(() => expect(forgeCreateIssue).toHaveBeenCalledWith( 1, - expect.objectContaining({ title: "Login times out" }) + expect.objectContaining({ title: "Login times out" }), + // The repository the page is SHOWING, handed to the dialog so the + // write is refused rather than redirected if the folder has moved. + { + expectedServerHost: "github.com", + expectedOwnerRepo: "xintaofei/codeg", + } ) ) // Straight into the panel on what was just filed: the number and the link @@ -1759,6 +2010,39 @@ describe("ForgePage writes", () => { expect(vi.mocked(forgeListIssues).mock.calls.length).toBe(before) }) + it("re-resolves the repository when a write says the folder has moved on", async () => { + const user = userEvent.setup() + vi.mocked(forgeListIssues).mockResolvedValue(listOf([])) + // What the backend answers with when the coordinates a write carried no + // longer match the folder's remote (`WRITE_MISMATCH_I18N_KEY`): the write + // was refused, and the page's job is to stop showing a repository the + // folder has left. + vi.mocked(forgeCreateIssue).mockRejectedValue({ + code: "configuration_invalid", + message: "this panel was showing github.com/xintaofei/codeg", + i18n_key: "Forge.writeMismatch", + i18n_params: { + expected: "github.com/xintaofei/codeg", + actual: "github.com/acme/other", + }, + }) + mount() + const before = vi.mocked(folderForgeRemote).mock.calls.length + + await user.click(await screen.findByRole("button", { name: "New issue" })) + await user.type(screen.getByLabelText("Title"), "Login times out") + await user.click(screen.getByRole("button", { name: "Create issue" })) + + // Re-resolved — which is what tears the stale rows, panel and dialogs + // down with it. Merely toasting would leave every later action aimed at + // the same repository the refusal was about. + await waitFor(() => + expect(vi.mocked(folderForgeRemote).mock.calls.length).toBeGreaterThan( + before + ) + ) + }) + it("counts the issue it filed onto the tab badge", async () => { const user = userEvent.setup() vi.mocked(forgeListIssues).mockResolvedValue( diff --git a/src/components/forge/forge-page.tsx b/src/components/forge/forge-page.tsx index 34847ecdd3..1ad131fd70 100644 --- a/src/components/forge/forge-page.tsx +++ b/src/components/forge/forge-page.tsx @@ -9,6 +9,7 @@ import { type ReactNode, } from "react" import { useTranslations } from "next-intl" +import { toast } from "sonner" import { Check, ExternalLink, @@ -73,15 +74,20 @@ import { ForgeStartDialog } from "@/components/forge/forge-start-dialog" import { useIsMobile } from "@/hooks/use-mobile" import { folderForgeRemote, + forgeExpectedRepo, forgeListIssues, forgeListLabels, + forgeRemoteGet, + forgeRemoteSet, forgeSettingsGet, forgeTabCount, + gitListRemotes, openSettingsWindow, workTaskLookupBySource, } from "@/lib/api" import { extractAppCommandError, + toErrorMessage, toLocalizedErrorMessage, type AppErrorTranslator, } from "@/lib/app-error" @@ -104,10 +110,12 @@ import type { ForgeLabel, ForgeProviderId, ForgeRemote, + ForgeRemoteStore, ForgeSort, ForgeTab, ForgeSettingsStore, ForgeTaskLink, + GitRemote, } from "@/lib/types" import { useAppWorkspaceStore } from "@/stores/app-workspace-store" import { useForgeRefreshStore } from "@/stores/forge-refresh-store" @@ -127,6 +135,15 @@ const FOLDER_STORAGE_KEY = "forge:folderId" * matches, so every symbol in here read as one that does not exist. */ const LABEL_SCOPE_SEP = String.fromCharCode(0) +/** The remote the panel reads when a folder has no selection — mirrors + * `DEFAULT_FORGE_REMOTE` in `src-tauri/src/commands/forge.rs`. */ +const DEFAULT_REMOTE = "origin" + +/** The picker's "no selection" item. MUST NOT be a possible git remote name: + * a space cannot appear in a refname, which is what a remote name is — so this + * can never collide with a real remote the folder happens to have. */ +const REMOTE_DEFAULT_ITEM = " default" + /** Must mirror `NO_ACCOUNT_I18N_KEY` in src-tauri/src/forge/mod.rs. The key — * not the error `code` — is the discriminator: `configuration_missing` is a * generic code that other failures share, and offering "add an account" for @@ -561,9 +578,20 @@ export function ForgePage() { } return projectFolders[0]?.id ?? null }, [folderId, projectFolders]) + const effectiveFolderPath = useMemo( + () => projectFolders.find((f) => f.id === effectiveFolderId)?.path ?? null, + [projectFolders, effectiveFolderId] + ) const [remote, setRemote] = useState(null) const [remoteLoading, setRemoteLoading] = useState(false) + /** Every remote in the selected folder — the picker's options. Loaded with + * the folder, not with the resolved remote, so a selection that no longer + * resolves still lets the user move off it. */ + const [remotes, setRemotes] = useState([]) + /** Bumped after the selection is saved. The resolution effect depends on it, + * so the page re-reads the repository it is now pointed at. */ + const [remoteVersion, setRemoteVersion] = useState(0) /** Bumped when the backend reports it had this host's forge wrong. It is a * dependency of the remote lookup, so bumping it re-derives `provider` — * which is what makes the correction visible in the tab wording too, not @@ -610,10 +638,21 @@ export function ForgePage() { * than as a reason to wait. Held as the whole store rather than as one * folder's resolved values so switching folders costs no round trip. */ const [settings, setSettings] = useState(null) + /** Which git remote each folder reads — the picker's own store, held whole + * like the settings above so switching folders costs no round trip. Read + * and written ONLY by the picker: the settings dialog cannot reach it, and + * vice versa. `null` means "not loaded yet, or the read failed", which the + * picker treats as the default rather than as a reason to wait. */ + const [remoteStore, setRemoteStore] = useState(null) const [settingsOpen, setSettingsOpen] = useState(false) const [labelOptions, setLabelOptions] = useState([]) const [labelsTruncated, setLabelsTruncated] = useState(false) const reqRef = useRef(0) + /** The repository the generation above belongs to, and a counter kept in + * step with it. `reqRef` is the guard for EVERY request aimed at the current + * repository, so it has to be claimed when the repository changes and there + * is no request of ours to claim it — see the render-phase bump below. */ + const repoRef = useRef(null) /** Rows taken from a write, keyed by item — what `reconcile` writes back * over a list response that went out before the write. A ref, not state: it * has to be readable by a fetch already in flight, and changing it must @@ -678,7 +717,62 @@ export function ForgePage() { return () => { cancelled = true } - }, [effectiveFolderId, forgeCorrection]) + }, [effectiveFolderId, forgeCorrection, remoteVersion]) + + // The picker's options come straight from git, so the list is complete even + // when none of them is recognizable as a forge. + useEffect(() => { + if (effectiveFolderPath == null) { + setRemotes([]) + return + } + let cancelled = false + gitListRemotes(effectiveFolderPath) + .then((list) => { + if (!cancelled) setRemotes(list) + }) + .catch(() => { + if (!cancelled) setRemotes([]) + }) + return () => { + cancelled = true + } + }, [effectiveFolderPath]) + + /** + * A write came back REFUSED because the folder now reads another repository. + * + * Re-resolve: the rows, the counts, the panel and the trigger dialog all + * belong to a repository this page is no longer showing, and the resolution + * effect's teardown is what puts them away. The message itself is shown by + * whoever caught the refusal — the panel that raised it is about to unmount. + */ + const handleStaleRepository = useCallback(() => { + setRemoteVersion((v) => v + 1) + }, []) + + // Switching the remote saves the choice on the folder, then re-runs the + // resolution above: the rows on screen belong to the repository the backend + // would read next, so the old ones must not survive the switch. + // + // Its OWN store, not the panel settings: that blob is rewritten wholesale by + // the settings dialog, and the picker writing into it is what used to detach + // the folder from the global row — and what let a later "use global + // defaults" save destroy a choice the user had just made. `null` is the + // picker's "default" item: no selection, so the folder reads `origin`. + const handlePickRemote = useCallback( + async (name: string | null) => { + if (effectiveFolderId == null) return + try { + const next = await forgeRemoteSet(effectiveFolderId, name) + setRemoteStore(next) + setRemoteVersion((v) => v + 1) + } catch (e) { + toast.error(toErrorMessage(e)) + } + }, + [effectiveFolderId] + ) /** * The remote only when codeg can actually read it. @@ -696,11 +790,78 @@ export function ForgePage() { */ const readable = remote?.supported ? remote : null - /** Which list the rows belong to — see [`LoadedList`]. */ - const listScope = `${effectiveFolderId}:${tab}` + /** + * WHICH repository everything below is about. + * + * The folder alone is not the answer, and the picker is why: one folder can + * be pointed at several repositories in turn, so the page number, the label + * vocabulary and the counted rows on screen are facts about a (folder, + * remote) PAIR. Keyed on the folder alone they outlive the switch, and each + * one then reads as a fact about the repository now on screen. + * + * A NAME rather than the object, so it is a stable fetch dependency: the + * resolution hands back a fresh object every time, and comparing those would + * re-run every fetch on each re-resolution of the SAME remote. + */ + const repoKey = + readable == null ? "none" : `${readable.server_host}/${readable.owner_repo}` + + /** + * What every WRITE carries: the repository this panel is SHOWING. + * + * Built from the RESOLVED remote rather than from `repoKey`, though the two + * name the same repository — the backend compares this pair against what it + * derives, and splitting the key back apart would put a parser between two + * spellings of one fact. + */ + const expectedRepo = useMemo(() => forgeExpectedRepo(readable), [readable]) + + /** + * The switch claims a generation of its own. + * + * `reqRef` is what decides whether an answer is still wanted, and today it + * happens to be safe without this: the refetch for the new repository runs on + * the commit that resolves it and takes the next number, so an answer still in + * the air loses. That safety is a consequence of the refetch happening at all, + * though — not of anything the switch does — and the two states where it does + * NOT happen are exactly the ones where a stale answer can still be believed: + * a remote that resolves to nothing readable (no refetch is fired, so no + * number is claimed), and the frame between the teardown and the resolution. + * Taking the number here states the rule directly — a repository change + * invalidates everything aimed at the last one — and needs no fetch to be + * fired for it to hold. + * + * During RENDER, so it is claimed in the same commit that resolves the new + * remote and before any effect can run. Absorbed, because this runs on every + * render and writing state unconditionally would loop. + */ + if (repoRef.current !== repoKey) { + repoRef.current = repoKey + reqRef.current += 1 + } + + /** The folder's selected remote name even when it does not resolve — the + * picker must show what the folder is SET to, not only what loaded. Read + * from the selection store rather than from the resolution, which reports + * the default as a name whenever nothing was chosen. */ + const selectedRemoteName = useMemo( + () => + effectiveFolderId == null + ? null + : (remoteStore?.folders[String(effectiveFolderId)] ?? null), + [remoteStore, effectiveFolderId] + ) + + /** Which list the rows belong to — see [`LoadedList`]. Carries the remote, + * not just the folder: switching the picker swaps the repository under the + * same folder id, and a page read from one forge must never be shown as the + * other's. */ + const listScope = `${effectiveFolderId}:${repoKey}:${tab}` /** Which RESULT SET the badges count — see [`TabCounts`]. No tab, no page, - * no order: none of the three can change either number. */ - const countsScope = `${effectiveFolderId}:${stateFilter}:${assignedMe}:${labelFilter.join(LABEL_SCOPE_SEP)}:${search}` + * no order: none of the three can change either number. Remote included for + * the same reason as `listScope`: the two repositories have unrelated + * totals. */ + const countsScope = `${effectiveFolderId}:${repoKey}:${stateFilter}:${assignedMe}:${labelFilter.join(LABEL_SCOPE_SEP)}:${search}` /** * Everything that decides whether a row belongs on the page being shown: the * folder and tab, the filter set, and the order and page number that place it @@ -945,6 +1106,11 @@ export function ForgePage() { // straight back. A failure is silent on purpose — the trigger dialog falls // back to the built-in defaults, and a toast about preferences nobody asked // for yet would be noise over a page that works. + // + // The remote selections come along for the same ride and the same reason: + // they are the picker's own store, read once, and a failure leaves the + // picker on the default rather than blocking a page that reads repositories + // perfectly well. useEffect(() => { let cancelled = false forgeSettingsGet() @@ -952,6 +1118,11 @@ export function ForgePage() { if (!cancelled) setSettings(s) }) .catch(() => {}) + forgeRemoteGet() + .then((s) => { + if (!cancelled) setRemoteStore(s) + }) + .catch(() => {}) const open = () => setSettingsOpen(true) window.addEventListener(OPEN_FORGE_SETTINGS_EVENT, open) return () => { @@ -974,18 +1145,35 @@ export function ForgePage() { // A different repository has a different label vocabulary, so a selection // made against the old one would filter by labels that may not exist here. + // The remote is part of "a different repository" — one folder can be pointed + // at a fork and then its parent — so the key is the pair, not the folder. // Derived during render rather than in an effect: this has to catch the // FALLBACK path too (the stored folder disappearing from the workspace), and // an effect would spend an extra render — and an extra request — doing it. - const [labelledFolder, setLabelledFolder] = useState(effectiveFolderId) - if (labelledFolder !== effectiveFolderId) { - setLabelledFolder(effectiveFolderId) + const [labelledScope, setLabelledScope] = useState( + `${effectiveFolderId}:${repoKey}` + ) + if (labelledScope !== `${effectiveFolderId}:${repoKey}`) { + setLabelledScope(`${effectiveFolderId}:${repoKey}`) if (labelFilter.length > 0) { setLabelFilter([]) setPage(1) } } + // The page number belongs to the repository as much as the label selection + // does: page 3 of a fork is a different slice of its parent, and asking the + // parent for it lands the reader on rows nobody chose. Switched the same way + // — during render, so the reset is committed in the same pass that the new + // repository resolves, BEFORE any effect can fetch the old page against it. + const [pagedScope, setPagedScope] = useState( + `${effectiveFolderId}:${repoKey}` + ) + if (pagedScope !== `${effectiveFolderId}:${repoKey}`) { + setPagedScope(`${effectiveFolderId}:${repoKey}`) + setPage(1) + } + // The repository's label vocabulary — once per repository, not per page: // labels barely change, and on GitHub this runs on the core quota rather than // search's much smaller one. Best-effort: a repository whose labels cannot be @@ -1408,6 +1596,9 @@ export function ForgePage() { folderId={effectiveFolderId} onPickFolder={pickFolder} remote={remote} + remotes={remotes} + remoteName={selectedRemoteName} + onPickRemote={handlePickRemote} /> {/* Only once a repository is resolved: without one there is nowhere @@ -1687,6 +1878,14 @@ export function ForgePage() { // list was fetched with, so a folder switch (which closes the panel — // see the reset effect above) cannot leave the two disagreeing. folderId={effectiveFolderId} + // Which repository that folder is pointed AT. The panel's repository + // facts — the account a comment is signed as, the merge methods the + // forge permits — are asked for by folder, so the folder alone cannot + // tell the panel whether its answer is still about the repository on + // screen. Same spelling as the scopes above, from the same value. + repo={repoKey} + expected={expectedRepo} + onStaleRepository={handleStaleRepository} onOpenChange={(open) => { if (!open) setDetailRow(null) }} @@ -1706,6 +1905,8 @@ export function ForgePage() { // repository — one read serves both, and the dialog must not wait on // a round trip to draw. labelOptions={labelOptions} + expected={expectedRepo} + onStaleRepository={handleStaleRepository} onOpenChange={setNewIssueOpen} onCreated={(created) => { setNewIssueOpen(false) @@ -1847,6 +2048,9 @@ function RepoBar({ folderId, onPickFolder, remote, + remotes, + remoteName, + onPickRemote, }: { folders: readonly FolderSelectOption[] folderId: number | null @@ -1854,6 +2058,14 @@ function RepoBar({ /** `null` until the folder resolves, or for a folder with no forge remote — * the picker still has to be usable, so only the right half goes away. */ remote: ForgeRemote | null + /** Every remote in the folder — the picker's options. */ + remotes: GitRemote[] + /** The folder's SAVED selection, or `null` when it is on the default. NOT + * the resolved name: a folder with nothing saved resolves to `origin`, and + * painting that as a picked remote would hide the fact that the folder is + * following the default — and the item that clears a choice. */ + remoteName: string | null + onPickRemote: (name: string | null) => void }) { const t = useTranslations("Forge") @@ -1869,6 +2081,37 @@ function RepoBar({ title={t("pickFolder")} variant="ghost" /> + {remotes.length > 0 ? ( + + ) : null} {remote != null ? ( <> diff --git a/src/components/forge/forge-settings-dialog.test.tsx b/src/components/forge/forge-settings-dialog.test.tsx index ea98265365..9c476e4f02 100644 --- a/src/components/forge/forge-settings-dialog.test.tsx +++ b/src/components/forge/forge-settings-dialog.test.tsx @@ -156,6 +156,25 @@ describe("ForgeSettingsDialog global scope", () => { expect(onSaved).toHaveBeenCalledWith({ global: settings, folders: {} }) }) + it("sends nothing about the remote, which the panel's picker owns", async () => { + const user = userEvent.setup() + await mountLoaded() + + await user.click(screen.getByRole("button", { name: "Save" })) + + await waitFor(() => expect(forgeSettingsSet).toHaveBeenCalled()) + // The selection lives in its own store and is written by the picker alone. + // A settings save must not carry a value a later read would treat as + // chosen — and must not be able to clear one either: this blob is dropped + // wholesale by "use the global defaults". + expect(Object.keys(lastSave().settings).sort()).toEqual([ + "default_issue_scenario", + "default_pr_scenario", + "scenario_prompts", + "writeback_default", + ]) + }) + it("keeps each scenario's instruction under its own segment, and marks the ones in use", async () => { const user = userEvent.setup() await mountLoaded() diff --git a/src/components/forge/forge-start-dialog.test.tsx b/src/components/forge/forge-start-dialog.test.tsx index 1254707605..4ea5a785cb 100644 --- a/src/components/forge/forge-start-dialog.test.tsx +++ b/src/components/forge/forge-start-dialog.test.tsx @@ -41,6 +41,7 @@ vi.mock("@/contexts/workbench-route-context", () => ({ })) const GITHUB: ForgeRemote = { + remote_name: "origin", server_host: "github.com", owner_repo: "o/r", remote_url: "https://github.com/o/r.git", @@ -48,6 +49,7 @@ const GITHUB: ForgeRemote = { supported: true, } const GITLAB: ForgeRemote = { + remote_name: "origin", server_host: "gitlab.com", owner_repo: "group/sub/app", remote_url: "https://gitlab.com/group/sub/app.git", diff --git a/src/i18n/messages/ar.json b/src/i18n/messages/ar.json index 08361d289d..2449587e2e 100644 --- a/src/i18n/messages/ar.json +++ b/src/i18n/messages/ar.json @@ -5617,8 +5617,10 @@ }, "Forge": { "title": "لوحة المستودع", + "remote": "remote", + "remoteDefault": "الافتراضي ({name})", "pickFolder": "اختر مجلد مشروع", - "noRemote": "لا يحتوي هذا المجلد على remote معروف (origin)", + "noRemote": "لا يحتوي هذا المجلد على remote معروف", "errors": { "noAccount": "لا يوجد حساب {provider} مُهيّأ للمضيف {host}. أضف حسابًا من الإعدادات ← التحكم بالإصدارات لتحميل هذا المستودع.", "unsupportedHost": "لوحة المستودع تدعم GitHub و GitLab و Gitea فقط، و{host} لا يُعرَف كأي منها. إذا كان مثيلًا مستضافًا ذاتيًا من GitHub Enterprise أو GitLab أو Gitea أو Forgejo، فأضف له حسابًا من الإعدادات ← التحكم بالإصدارات.", @@ -5775,6 +5777,7 @@ "previewEmpty": "(بدون وصف)", "duplicateBody": "توجد مهمة نشطة تعالج هذا العنصر بالفعل: {title} ({status}). إنشاء أخرى على أي حال؟", "folderMismatch": "remote هذا المجلد هو {remote}، وليس مستودع هذه القضية — اختر المجلد المطابق.", + "writeMismatch": "كانت هذه اللوحة تعرض {expected}، لكن الطرف البعيد للمجلد أصبح الآن {actual} — تم رفض الكتابة بدلاً من إرسالها إلى المستودع الخاطئ. تم تحديث اللوحة؛ حاول مرة أخرى.", "cancel": "إلغاء", "create": "إنشاء مهمة", "creating": "جارٍ الإنشاء…", diff --git a/src/i18n/messages/de.json b/src/i18n/messages/de.json index a4fcf64caa..ecb20f9ca5 100644 --- a/src/i18n/messages/de.json +++ b/src/i18n/messages/de.json @@ -5617,8 +5617,10 @@ }, "Forge": { "title": "Repository-Panel", + "remote": "Remote", + "remoteDefault": "Standard ({name})", "pickFolder": "Projektordner wählen", - "noRemote": "Dieser Ordner hat kein erkennbares Forge-Remote (origin)", + "noRemote": "Dieser Ordner hat kein erkennbares Forge-Remote", "errors": { "noAccount": "Für {host} ist kein {provider}-Konto konfiguriert. Füge unter Einstellungen → Versionskontrolle eines hinzu, um dieses Repository zu laden.", "unsupportedHost": "Das Repository-Panel unterstützt nur GitHub, GitLab und Gitea, und {host} wird als keines davon erkannt. Falls es sich um eine selbst gehostete GitHub-Enterprise-, GitLab-, Gitea- oder Forgejo-Instanz handelt, füge unter Einstellungen → Versionskontrolle ein Konto dafür hinzu.", @@ -5775,6 +5777,7 @@ "previewEmpty": "(keine Beschreibung)", "duplicateBody": "Eine aktive Aufgabe bearbeitet dieses Element bereits: {title} ({status}). Trotzdem eine weitere erstellen?", "folderMismatch": "Das Remote dieses Ordners ist {remote}, nicht das Repository dieses Issues — wähle den passenden Ordner.", + "writeMismatch": "Dieses Panel zeigte {expected}, aber das Remote des Ordners ist jetzt {actual} — der Schreibvorgang wurde abgelehnt statt an das falsche Repository gesendet. Das Panel wurde neu geladen; bitte erneut versuchen.", "cancel": "Abbrechen", "create": "Aufgabe erstellen", "creating": "Erstelle…", diff --git a/src/i18n/messages/en.json b/src/i18n/messages/en.json index 7c84cab256..3043a2b5c3 100644 --- a/src/i18n/messages/en.json +++ b/src/i18n/messages/en.json @@ -5617,8 +5617,10 @@ }, "Forge": { "title": "Repository panel", + "remote": "Remote", + "remoteDefault": "Default ({name})", "pickFolder": "Pick a project folder", - "noRemote": "This folder has no recognizable forge remote (origin)", + "noRemote": "This folder has no recognizable forge remote", "errors": { "noAccount": "No {provider} account is configured for {host}. Add one under Settings → Version Control to load this repository.", "unsupportedHost": "The repository panel supports GitHub, GitLab and Gitea only, and {host} is not recognized as any of them. If it is a self-hosted GitHub Enterprise, GitLab, Gitea or Forgejo instance, add an account for it under Settings → Version Control.", @@ -5775,6 +5777,7 @@ "previewEmpty": "(no description)", "duplicateBody": "An active task already handles this item: {title} ({status}). Create another one anyway?", "folderMismatch": "This folder's remote is {remote}, not this issue's repository — pick the matching folder.", + "writeMismatch": "This panel was showing {expected}, but the folder's remote is now {actual} — the write was refused rather than sent to the wrong repository. The panel has been refreshed; try again.", "cancel": "Cancel", "create": "Create task", "creating": "Creating…", diff --git a/src/i18n/messages/es.json b/src/i18n/messages/es.json index 7d7af796b7..edae24c644 100644 --- a/src/i18n/messages/es.json +++ b/src/i18n/messages/es.json @@ -5617,8 +5617,10 @@ }, "Forge": { "title": "Panel del repositorio", + "remote": "Remoto", + "remoteDefault": "Predeterminado ({name})", "pickFolder": "Elige una carpeta de proyecto", - "noRemote": "Esta carpeta no tiene un remoto reconocible (origin)", + "noRemote": "Esta carpeta no tiene un remoto reconocible", "errors": { "noAccount": "No hay ninguna cuenta de {provider} configurada para {host}. Añade una en Configuración → Control de versiones para cargar este repositorio.", "unsupportedHost": "El panel del repositorio solo admite GitHub, GitLab y Gitea, y {host} no se reconoce como ninguno de ellos. Si es una instancia autoalojada de GitHub Enterprise, GitLab, Gitea o Forgejo, añade una cuenta para ella en Configuración → Control de versiones.", @@ -5775,6 +5777,7 @@ "previewEmpty": "(sin descripción)", "duplicateBody": "Ya hay una tarea activa para este elemento: {title} ({status}). ¿Crear otra de todos modos?", "folderMismatch": "El remoto de esta carpeta es {remote}, no el repositorio de este issue: elige la carpeta correcta.", + "writeMismatch": "Este panel mostraba {expected}, pero el remoto de la carpeta ahora es {actual}: la escritura se rechazó en lugar de enviarla al repositorio equivocado. El panel se ha actualizado; inténtalo de nuevo.", "cancel": "Cancelar", "create": "Crear tarea", "creating": "Creando…", diff --git a/src/i18n/messages/fr.json b/src/i18n/messages/fr.json index 12da2f2c78..74cf50a927 100644 --- a/src/i18n/messages/fr.json +++ b/src/i18n/messages/fr.json @@ -5617,8 +5617,10 @@ }, "Forge": { "title": "Panneau du dépôt", + "remote": "Remote", + "remoteDefault": "Par défaut ({name})", "pickFolder": "Choisir un dossier de projet", - "noRemote": "Ce dossier n'a pas de remote reconnaissable (origin)", + "noRemote": "Ce dossier n'a pas de remote reconnaissable", "errors": { "noAccount": "Aucun compte {provider} n'est configuré pour {host}. Ajoutez-en un dans Paramètres → Contrôle de version pour charger ce dépôt.", "unsupportedHost": "Le panneau du dépôt ne prend en charge que GitHub, GitLab et Gitea, et {host} n'est reconnu comme aucun d'entre eux. S'il s'agit d'une instance auto-hébergée de GitHub Enterprise, GitLab, Gitea ou Forgejo, ajoutez-y un compte dans Paramètres → Contrôle de version.", @@ -5775,6 +5777,7 @@ "previewEmpty": "(aucune description)", "duplicateBody": "Une tâche active traite déjà cet élément : {title} ({status}). En créer une autre quand même ?", "folderMismatch": "Le remote de ce dossier est {remote}, pas le dépôt de cette issue — choisissez le bon dossier.", + "writeMismatch": "Ce panneau affichait {expected}, mais le dépôt distant du dossier est désormais {actual} — l’écriture a été refusée plutôt qu’envoyée au mauvais dépôt. Le panneau a été actualisé ; réessayez.", "cancel": "Annuler", "create": "Créer la tâche", "creating": "Création…", diff --git a/src/i18n/messages/ja.json b/src/i18n/messages/ja.json index be0fef3a5b..fac3e7c3f8 100644 --- a/src/i18n/messages/ja.json +++ b/src/i18n/messages/ja.json @@ -5617,8 +5617,10 @@ }, "Forge": { "title": "リポジトリパネル", + "remote": "リモート", + "remoteDefault": "デフォルト（{name}）", "pickFolder": "プロジェクトフォルダを選択", - "noRemote": "このフォルダには認識できるリモート（origin）がありません", + "noRemote": "このフォルダには認識できるリモートがありません", "errors": { "noAccount": "{host} の {provider} アカウントが設定されていません。「設定 → バージョン管理」で追加するとこのリポジトリを読み込めます。", "unsupportedHost": "リポジトリパネルは GitHub、GitLab、Gitea にのみ対応しており、{host} はそのいずれとしても認識できません。自己ホスト型の GitHub Enterprise、GitLab、Gitea、Forgejo インスタンスであれば、「設定 → バージョン管理」でアカウントを追加してください。", @@ -5775,6 +5777,7 @@ "previewEmpty": "（説明なし）", "duplicateBody": "この項目はすでにアクティブなタスクが対応中です：{title}（{status}）。それでも作成しますか？", "folderMismatch": "このフォルダのリモートは {remote} で、この Issue のリポジトリと一致しません。対応するフォルダを選択してください。", + "writeMismatch": "このパネルは {expected} を表示していましたが、フォルダーのリモートは現在 {actual} です — 書き込みは誤ったリポジトリに送られる代わりに拒否されました。パネルを再読み込みしました。もう一度お試しください。", "cancel": "キャンセル", "create": "タスクを作成", "creating": "作成中…", diff --git a/src/i18n/messages/ko.json b/src/i18n/messages/ko.json index 43e1b7255d..85c8141c21 100644 --- a/src/i18n/messages/ko.json +++ b/src/i18n/messages/ko.json @@ -5617,8 +5617,10 @@ }, "Forge": { "title": "리포지토리 패널", + "remote": "원격", + "remoteDefault": "기본값({name})", "pickFolder": "프로젝트 폴더 선택", - "noRemote": "이 폴더에는 인식 가능한 원격 저장소(origin)가 없습니다", + "noRemote": "이 폴더에는 인식 가능한 원격 저장소가 없습니다", "errors": { "noAccount": "{host}에 대한 {provider} 계정이 설정되지 않았습니다. 설정 → 버전 관리에서 추가하면 이 저장소를 불러올 수 있습니다.", "unsupportedHost": "리포지토리 패널은 GitHub, GitLab, Gitea만 지원하며, {host}는 그중 어느 쪽으로도 인식되지 않습니다. 자체 호스팅 GitHub Enterprise, GitLab, Gitea 또는 Forgejo 인스턴스라면 설정 → 버전 관리에서 계정을 추가하세요.", @@ -5775,6 +5777,7 @@ "previewEmpty": "(설명 없음)", "duplicateBody": "이미 활성 작업이 이 항목을 처리 중입니다: {title}({status}). 그래도 새로 만들까요?", "folderMismatch": "현재 폴더의 원격은 {remote}로, 이 Issue의 저장소와 일치하지 않습니다. 일치하는 폴더를 선택하세요.", + "writeMismatch": "이 패널은 {expected}을(를) 표시하고 있었지만 폴더의 원격은 이제 {actual}입니다 — 쓰기는 잘못된 저장소로 보내지지 않고 거부되었습니다. 패널을 새로 고쳤습니다. 다시 시도하세요.", "cancel": "취소", "create": "작업 생성", "creating": "생성 중…", diff --git a/src/i18n/messages/pt.json b/src/i18n/messages/pt.json index 095a75b598..928b038ed0 100644 --- a/src/i18n/messages/pt.json +++ b/src/i18n/messages/pt.json @@ -5617,8 +5617,10 @@ }, "Forge": { "title": "Painel do repositório", + "remote": "Remoto", + "remoteDefault": "Padrão ({name})", "pickFolder": "Escolha uma pasta de projeto", - "noRemote": "Esta pasta não tem um remoto reconhecível (origin)", + "noRemote": "Esta pasta não tem um remoto reconhecível", "errors": { "noAccount": "Nenhuma conta {provider} configurada para {host}. Adicione uma em Configurações → Controle de versão para carregar este repositório.", "unsupportedHost": "O painel do repositório só oferece suporte a GitHub, GitLab e Gitea, e {host} não é reconhecido como nenhum deles. Se for uma instância auto-hospedada do GitHub Enterprise, GitLab, Gitea ou Forgejo, adicione uma conta para ela em Configurações → Controle de versão.", @@ -5775,6 +5777,7 @@ "previewEmpty": "(sem descrição)", "duplicateBody": "Já existe uma tarefa ativa para este item: {title} ({status}). Criar outra mesmo assim?", "folderMismatch": "O remoto desta pasta é {remote}, não o repositório deste issue — escolha a pasta correspondente.", + "writeMismatch": "Este painel mostrava {expected}, mas o remoto da pasta agora é {actual} — a gravação foi recusada em vez de enviada para o repositório errado. O painel foi atualizado; tente novamente.", "cancel": "Cancelar", "create": "Criar tarefa", "creating": "Criando…", diff --git a/src/i18n/messages/zh-CN.json b/src/i18n/messages/zh-CN.json index dc4e9eeeed..8a7bf63d27 100644 --- a/src/i18n/messages/zh-CN.json +++ b/src/i18n/messages/zh-CN.json @@ -5617,8 +5617,10 @@ }, "Forge": { "title": "仓库面板", + "remote": "远端", + "remoteDefault": "默认（{name}）", "pickFolder": "选择项目文件夹", - "noRemote": "该文件夹没有可识别的代码托管远端（origin）", + "noRemote": "该文件夹没有可识别的代码托管远端", "errors": { "noAccount": "还没有为 {host} 配置 {provider} 账号。请在「设置 → 版本控制」中添加后再加载该仓库。", "unsupportedHost": "仓库面板目前仅支持 GitHub、GitLab 和 Gitea，而 {host} 不是其中任何一种。如果它是自建的 GitHub Enterprise、GitLab、Gitea 或 Forgejo 实例，请在「设置 → 版本控制」中为它添加账号。", @@ -5775,6 +5777,7 @@ "previewEmpty": "（无描述）", "duplicateBody": "已有一个活跃任务在处理该条目：{title}（{status}）。仍要再建一个吗？", "folderMismatch": "当前文件夹的远端是 {remote}，与该 Issue 的仓库不一致——请选择匹配的文件夹。", + "writeMismatch": "这个面板显示的是 {expected}，但该文件夹的远端现在是 {actual} —— 写入已被拒绝，而不是发到错误的仓库。面板已刷新，请重试。", "cancel": "取消", "create": "创建待办任务", "creating": "创建中…", diff --git a/src/i18n/messages/zh-TW.json b/src/i18n/messages/zh-TW.json index d5500e9e49..c67e9aff88 100644 --- a/src/i18n/messages/zh-TW.json +++ b/src/i18n/messages/zh-TW.json @@ -5617,8 +5617,10 @@ }, "Forge": { "title": "儲存庫面板", + "remote": "遠端", + "remoteDefault": "預設（{name}）", "pickFolder": "選擇專案資料夾", - "noRemote": "該資料夾沒有可識別的程式碼託管遠端（origin）", + "noRemote": "該資料夾沒有可識別的程式碼託管遠端", "errors": { "noAccount": "尚未為 {host} 設定 {provider} 帳號。請先在「設定 → 版本控制」中新增，才能載入這個儲存庫。", "unsupportedHost": "儲存庫面板目前僅支援 GitHub、GitLab 與 Gitea，而 {host} 不屬於其中任何一種。如果它是自架的 GitHub Enterprise、GitLab、Gitea 或 Forgejo 執行個體，請在「設定 → 版本控制」中為它新增帳號。", @@ -5775,6 +5777,7 @@ "previewEmpty": "（無描述）", "duplicateBody": "已有一個進行中的任務在處理該條目：{title}（{status}）。仍要再建一個嗎？", "folderMismatch": "目前資料夾的遠端是 {remote}，與該 Issue 的儲存庫不一致——請選擇相符的資料夾。", + "writeMismatch": "這個面板顯示的是 {expected}，但該資料夾的遠端現在是 {actual} —— 寫入已被拒絕，而不是送到錯誤的儲存庫。面板已重新整理，請重試。", "cancel": "取消", "create": "建立待辦任務", "creating": "建立中…", diff --git a/src/lib/api.ts b/src/lib/api.ts index e8e62b3fc8..d784032a85 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -33,6 +33,7 @@ import type { ForgeComment, ForgeCreateResult, ForgeCommentList, + ForgeExpectedRepo, ForgeIdentity, ForgeIssueList, ForgeIssueRow, @@ -41,6 +42,7 @@ import type { ForgeMergeOptions, ForgePanelSettings, ForgeRemote, + ForgeRemoteStore, ForgeSettingsStore, ForgeSort, ForgeStateAction, @@ -5698,6 +5700,22 @@ export async function forgeListComments( }) } +/** + * The pair a write should carry, from the repository the panel is SHOWING. + * + * `null` for a folder with nothing readable on screen: such a write is refused + * by the resolution itself, so there is nothing to compare it against. + */ +export function forgeExpectedRepo( + remote: Pick | null | undefined +): ForgeExpectedRepo | null { + if (remote == null) return null + return { + expectedServerHost: remote.server_host, + expectedOwnerRepo: remote.owner_repo, + } +} + /** * Post one comment, and get back the comment as the FORGE stored it. * @@ -5716,7 +5734,8 @@ export async function forgeCreateComment( number: number body: string accountId?: string | null - } + }, + expected?: ForgeExpectedRepo | null ): Promise { return getTransport().call("forge_create_comment", { folderId, @@ -5725,6 +5744,7 @@ export async function forgeCreateComment( number: draft.number, body: draft.body, accountId: draft.accountId ?? null, + ...(expected ?? {}), }, }) } @@ -5744,7 +5764,8 @@ export async function forgeSetItemState( number: number action: ForgeStateAction accountId?: string | null - } + }, + expected?: ForgeExpectedRepo | null ): Promise { return getTransport().call("forge_set_item_state", { folderId, @@ -5753,6 +5774,7 @@ export async function forgeSetItemState( number: request.number, action: request.action, accountId: request.accountId ?? null, + ...(expected ?? {}), }, }) } @@ -5767,7 +5789,8 @@ export async function forgeCreateIssue( body?: string | null labels?: string[] accountId?: string | null - } + }, + expected?: ForgeExpectedRepo | null ): Promise { return getTransport().call("forge_create_issue", { folderId, @@ -5776,6 +5799,7 @@ export async function forgeCreateIssue( body: draft.body ?? null, labels: draft.labels ?? [], accountId: draft.accountId ?? null, + ...(expected ?? {}), }, }) } @@ -5876,7 +5900,8 @@ export async function forgeMergeChange( method: ForgeMergeMethod headSha?: string | null accountId?: string | null - } + }, + expected?: ForgeExpectedRepo | null ): Promise { return getTransport().call("forge_merge_change", { folderId, @@ -5885,6 +5910,7 @@ export async function forgeMergeChange( method: request.method, headSha: request.headSha ?? null, accountId: request.accountId ?? null, + ...(expected ?? {}), }, }) } @@ -5927,3 +5953,26 @@ export async function forgeSettingsSet( ): Promise { return getTransport().call("forge_settings_set", { folderId, settings }) } + +/** Every folder's remote selection at once — what the panel's picker reads. + * Held as the whole store so switching folders costs no round trip, and a + * selection that no longer resolves is still shown for what the folder is set + * to. */ +export async function forgeRemoteGet(): Promise { + return getTransport().call("forge_remote_get", {}) +} + +/** + * Save ONE folder's remote selection and get every folder's back as stored. + * + * `remote = null` (or a blank name) puts the folder back on the default + * remote — the picker's "default (origin)" answer. Its own command rather than + * a field on the settings save: the picker writes this on every click, and a + * settings save must not be able to take it away. + */ +export async function forgeRemoteSet( + folderId: number, + remote: string | null +): Promise { + return getTransport().call("forge_remote_set", { folderId, remote }) +} diff --git a/src/lib/app-error.ts b/src/lib/app-error.ts index 9ddaab9e53..8854ba1ec0 100644 --- a/src/lib/app-error.ts +++ b/src/lib/app-error.ts @@ -69,6 +69,20 @@ export function extractAppCommandError(error: unknown): AppCommandError | null { // If the backend enum ever renames, both sides must change together. export const NOT_A_GIT_REPO_CODE = "not_a_git_repository" +// Must mirror `WRITE_MISMATCH_I18N_KEY` in src-tauri/src/forge/mod.rs. A WRITE +// that carried coordinates no longer matching the folder's remote comes back +// with this key, and the panel's job is to re-resolve the repository rather +// than leave the reader on one the folder has left. +export const FORGE_WRITE_MISMATCH_I18N_KEY = "Forge.writeMismatch" + +/** Whether this failure is that refusal — the one thing a caller must ACT on + * rather than merely report. */ +export function isForgeWriteMismatch(error: unknown): boolean { + return ( + extractAppCommandError(error)?.i18n_key === FORGE_WRITE_MISMATCH_I18N_KEY + ) +} + export function isNotAGitRepoError(error: unknown): boolean { const appError = extractAppCommandError(error) if (appError?.code === NOT_A_GIT_REPO_CODE) return true diff --git a/src/lib/forge-settings.ts b/src/lib/forge-settings.ts index 1db8ac22d4..5c0fe70ce7 100644 --- a/src/lib/forge-settings.ts +++ b/src/lib/forge-settings.ts @@ -1,5 +1,15 @@ import type { ForgePanelSettings, ForgeSettingsStore } from "@/lib/types" +/** The built-in defaults for one scope — mirrors `ForgePanelSettings::default` + * (write-back on; everything else unset). Shared so a caller that needs a base + * to spread over does not enumerate the fields it does not edit. */ +export const DEFAULT_FORGE_PANEL_SETTINGS: ForgePanelSettings = { + default_issue_scenario: null, + default_pr_scenario: null, + writeback_default: true, + scenario_prompts: {}, +} + /** * Sentinel folder id of the global row — the same one the task settings dialog * uses for its own "all folders" scope, so the two surfaces speak one language. diff --git a/src/lib/types.ts b/src/lib/types.ts index 8aee63540c..1152b05cde 100644 --- a/src/lib/types.ts +++ b/src/lib/types.ts @@ -1691,6 +1691,11 @@ export interface ForgeSourceMeta { head_ref?: string | null head_sha?: string | null head_repo?: string | null + /** The repository the task's work is pushed to when it is not the source — + * the folder's `origin` recorded at trigger time, for the fork workflow + * (the panel reads the parent; the branch codeg writes is the user's own + * copy). Absent = push to the source. */ + fork_repo?: string | null /** URL of the PR created by the delivery acceptance path (P1). */ result_pr?: string | null /** The trigger dialog's write-back answer, frozen at trigger time. Absent on @@ -1986,8 +1991,27 @@ export interface ForgeChangedFileList { has_next: boolean } +/** + * Which repository a WRITE believes it is writing to — mirrors + * `forge::ExpectedCoordinates`. + * + * Sent flat beside a write's own fields, checked against what the folder's + * remote resolves to at that moment, and refused — never redirected — when the + * two disagree. That is what stops a second window or a stale browser tab from + * posting, closing, filing or merging into the repository the selection has + * since moved to. Absent on a request from a build that predates the check, + * which keeps behaving exactly as it did. + */ +export interface ForgeExpectedRepo { + expectedServerHost: string + expectedOwnerRepo: string +} + /** A folder's `origin` remote parsed into forge coordinates. */ export interface ForgeRemote { + /** Which remote this was resolved from — the panel shows it so the active + * choice is visible rather than inferred from the URL. */ + remote_name: string server_host: string owner_repo: string remote_url: string @@ -2097,6 +2121,20 @@ export interface ForgeSettingsStore { /** Reserved `scenario_prompts` key applied to every scenario. */ export const FORGE_SCENARIO_PROMPT_ALL = "all" +/** Which git remote each folder's forge panel reads — mirrors + * `forge::remotes::ForgeRemoteStore`. + * + * Deliberately NOT a field of `ForgePanelSettings`: the picker saves this on + * every click, while the panel settings are a blob the settings dialog + * rewrites wholesale — so one field living in the other's blob is how "use + * global defaults" came to destroy a choice the picker had already saved. */ +export interface ForgeRemoteStore { + /** Keyed by folder id (JSON has no integer keys, so they arrive as strings). + * A folder with no entry reads the historical `origin` — absence IS the + * default answer, so there is no global row to fall back to. */ + folders: Record +} + /** Discriminated trigger outcome — duplicate/mismatch are answers, not errors. */ export type ForgeCreateResult = | { outcome: "created"; task: WorkTask }