diff --git a/src-tauri/src/bus/server.rs b/src-tauri/src/bus/server.rs index 480a4ee3..ebc89907 100644 --- a/src-tauri/src/bus/server.rs +++ b/src-tauri/src/bus/server.rs @@ -1196,7 +1196,9 @@ fn planner_specs() -> Value { "hint": { "type": "string", "enum": ["normal", "deep"], "description": "Optional routing hint only: normal prefers Codex for cheap batches; deep prefers Claude for deeper reasoning. This is not a tool choice." }, "base_branch": { "type": "string", - "description": "Branch in the target repo to branch the new work OFF. Leave empty to use the repo's default branch (main/master). Set it only when the repo merges into a non-default branch (develop/staging/a release branch)." } + "description": "Branch in the target repo to branch the new work OFF. Leave empty to use the repo's default branch (main/master). Set it only when the repo merges into a non-default branch (develop/staging/a release branch)." }, + "depends_on": { "type": "string", + "description": "Optional: the exact `name` of ANOTHER task in THIS SAME call's `directions` list that must be merged before this one is allowed to merge — use this for a cross-repo change set where this task consumes something the other task produces (e.g. a submodule SHA bump, a version bump). Leave empty when this task has no upstream. The name MUST resolve to exactly one task proposed in this SAME call: a typo, an ambiguous duplicate name, a name from another proposal, or a task that gets denied all BLOCK this task's merge (it is never released on a bad reference) until a later propose_directions call supplies a name that resolves cleanly." } }, "required": ["name", "repo", "reason"] } } }, "required": ["directions"] } }, @@ -1291,8 +1293,8 @@ pub async fn serve( #[cfg(test)] mod tests { use super::{ - is_weft_internal_tool, register_pr_tool, serve, session_servers_for_kind, summarize, - tool_specs, UNKNOWN_ENGINE, + is_weft_internal_tool, planner_specs, register_pr_tool, serve, session_servers_for_kind, + summarize, tool_specs, UNKNOWN_ENGINE, }; use crate::ask::RiskLevel; use crate::store::Db; @@ -1320,6 +1322,40 @@ mod tests { assert_ne!(action_key2, action_key); } + /// THE FIX (Codex review, PR #159 bus/server.rs:1201): this SPEC TEXT is the lead's only + /// source of truth for what `depends_on` actually does — it must never drift from the + /// real, fail-closed behavior `record_upstream_edges` implements (see planner.rs's + /// `record_upstream_edges_blocks_an_ambiguous_duplicate_name_instead_of_guessing` and + /// `confirm_blocks_a_consumer_whose_named_upstream_was_denied`: an unresolvable name + /// BLOCKS the task's merge, it is never silently ignored). An earlier version of this text + /// claimed the OPPOSITE ("a typo ... is silently ignored"), which could lead the lead to + /// believe a bad reference is harmless when it actually strands the task's readiness until + /// a fresh `propose_directions` call fixes the reference. + #[test] + fn depends_on_spec_text_describes_the_real_fail_closed_behavior() { + let specs = planner_specs(); + let propose = specs + .as_array() + .unwrap() + .iter() + .find(|t| t["name"] == "propose_directions") + .expect("propose_directions tool spec exists"); + let desc = propose["inputSchema"]["properties"]["directions"]["items"]["properties"] + ["depends_on"]["description"] + .as_str() + .expect("depends_on has a description"); + assert!( + desc.contains("BLOCK"), + "must tell the lead an unresolvable name blocks the task, not just describe the \ + happy path: {desc}" + ); + assert!( + !desc.to_lowercase().contains("ignored"), + "must never claim (even in passing) that a bad reference is ignored — the real \ + behavior is fail-closed, not a silent no-op: {desc}" + ); + } + /// Issue #89: an MCP/fallback ask (no `command`/`file_path` field) shows only /// the bare tool name as `summary`, but `action_key` must fold in the full /// args so two calls with different args don't collide. diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs index f8e28305..2a2150f1 100644 --- a/src-tauri/src/commands.rs +++ b/src-tauri/src/commands.rs @@ -1739,6 +1739,10 @@ pub struct WriteTrigger { pub base_branch: String, pub hint: crate::engine_routing::RoutingHint, pub route: Option, + /// See `planner::PendingWrite::depends_on` — surfaced here so the per-lane Needs-you card + /// can show which producer gates this consumer's merge (Codex review, PR #159 + /// planner.rs:109). + pub depends_on: String, } /// Every pending write declaration across the workspace's threads — the @@ -1776,6 +1780,7 @@ pub async fn write_triggers( base_branch: p.base_branch, hint: p.hint, route: p.route, + depends_on: p.depends_on, }); } } diff --git a/src-tauri/src/git.rs b/src-tauri/src/git.rs index fd0de753..10c83874 100644 --- a/src-tauri/src/git.rs +++ b/src-tauri/src/git.rs @@ -1099,6 +1099,30 @@ pub fn current_branch(repo: &Path) -> Result { /// The repo's `origin` remote URL, if one is configured. None for a freshly /// `git init`-ed local repo (no origin) or any git error — callers treat that /// as "no remote identity, dedup by path only". +/// +/// Uses `git remote get-url origin` — which EXPANDS any `url..insteadOf` +/// rewrite configured for it — DELIBERATELY, not `git config --get remote. +/// origin.url` (the raw, unexpanded value a prior version of this function +/// used). A fourth independent review round on PR #159 (repo.rs:3925 → +/// git.rs:1120) proved with `GIT_TRACE` against a real repo that this +/// matters for safety, not just semantics: `live_default_branch` (this +/// module) runs `git ls-remote origin`, which git ALWAYS resolves through any +/// configured `insteadOf` rewrite — there is no way to make an actual fetch +/// operation skip it. If this function returned the raw pre-rewrite value +/// while `ls-remote` queries the rewritten target, `store::repo:: +/// remote_matches_pr_host` would validate an identity that is NOT the one +/// `ls-remote` is about to answer for — an attacker (or an innocent corporate +/// mirror setup with a stale HEAD) could make a merge-readiness check pass +/// while the actual git operation talks to a different repository entirely. +/// The raw value's own justification (a mirror rewrite must not make dedup +/// treat two clones of the same conceptual repo as different repos) is a +/// real but much lower-stakes tradeoff than an irreversible auto-merge +/// decision, and applies to EVERY caller of this shared function — better to +/// accept an occasional missed dedup than to weaken the one check standing +/// between "vetted this is really the PR's repo" and auto-merge. Identical +/// output whenever no `insteadOf` rule applies, i.e. every repo that doesn't +/// use this git feature — this is a no-op for the overwhelming majority of +/// checkouts either way. pub fn remote_url(repo: &Path) -> Option { git(repo, &["remote", "get-url", "origin"]) .ok() @@ -1310,6 +1334,55 @@ mod tests { let _ = std::fs::remove_dir_all(&repo); } + /// THE FIX (Codex review, PR #159 repo.rs:3925 → git.rs:1120, a fourth independent review + /// round): `remote_url_reads_origin_or_none` above configures no `insteadOf` rewrite, so + /// the raw config value and the expanded value happen to coincide there — it cannot tell + /// `git remote get-url origin` (expanded) apart from `git config --get remote.origin.url` + /// (raw), which is exactly how a prior version of this function silently regressed to the + /// unsafe raw reading. This test configures a LOCAL (repo-scoped, never global) `insteadOf` + /// rewrite that redirects the configured URL to a totally different "decoy" path, so the + /// two readings genuinely disagree — proving `remote_url` reports what `ls-remote`/fetch + /// will ACTUALLY resolve to, not merely what the config text says. + #[test] + fn remote_url_reports_the_insteadof_expanded_target_not_the_raw_configured_value() { + let decoy = tmp("remote-insteadof-decoy"); + init_repo(&decoy).unwrap(); + let repo = tmp("remote-insteadof-repo"); + init_repo(&repo).unwrap(); + git( + &repo, + &["remote", "add", "origin", "https://github.com/acme/real-repo.git"], + ) + .unwrap(); + let key = format!("url.{}.insteadOf", decoy.to_str().unwrap()); + git(&repo, &[ + "config", + &key, + "https://github.com/acme/real-repo.git", + ]) + .unwrap(); + + // The raw configured value never changed... + assert_eq!( + git(&repo, &["config", "--get", "remote.origin.url"]).unwrap(), + "https://github.com/acme/real-repo.git" + ); + // ...but `remote_url` must report the EXPANDED target: that's what an actual + // `git ls-remote origin` (and every other real fetch) resolves to. A caller that + // validated identity against the raw value while the real operation went through the + // rewrite would vet one repository and talk to a completely different one. + assert_eq!( + remote_url(&repo).as_deref(), + Some(decoy.to_str().unwrap()), + "remote_url must return the insteadOf-expanded target, not the pre-rewrite config \ + text — otherwise identity checks built on it can pass while the real fetch talks \ + to a different repo entirely" + ); + + let _ = std::fs::remove_dir_all(&repo); + let _ = std::fs::remove_dir_all(&decoy); + } + #[test] fn worktree_branches_from_recorded_base_not_current_head() { let repo = tmp("base"); diff --git a/src-tauri/src/host/automerge.rs b/src-tauri/src/host/automerge.rs index 951ca8f4..354989ae 100644 --- a/src-tauri/src/host/automerge.rs +++ b/src-tauri/src/host/automerge.rs @@ -367,6 +367,7 @@ async fn evaluate_row( // 5). Mirrors `host::monitor`'s own `check_one` exactly (same // `spawn_blocking` discipline). let target = PrTarget { + host_base: pr.host_base.clone(), owner: pr.host_owner.clone(), repo: pr.host_repo.clone(), number: pr.number, @@ -388,7 +389,11 @@ async fn evaluate_row( return RowVerdict::Skip; // couldn't confirm live state — never merge on a guess } }; - let fresh_readiness = judge::merge_readiness(&snapshot.ci, &snapshot.review, &snapshot.conflict); + // Re-resolved here rather than trusting the swept row: this is the + // pre-merge confirmation, and an upstream can have moved since the sweep. + let upstream = repo::upstream_merge_state(db, pr.direction_id).await; + let fresh_readiness = + judge::merge_readiness(&snapshot.ci, &snapshot.review, &snapshot.conflict, &upstream); if let Err(e) = repo::apply_pull_request_snapshot(db, pr.id, &snapshot, &fresh_readiness).await { eprintln!( "[weft][automerge] pr #{}: could not save pre-merge confirmation snapshot: {e}", @@ -415,6 +420,78 @@ async fn evaluate_row( if final_decision != AutoMergeDecision::Merge { return RowVerdict::Skip; // downgraded since the pre-filter — silent, like any other skip } + + // Test-only seam: let a test pause HERE — between the upstream read this authorization + // was based on (above) and the re-check immediately below — to drive a genuinely + // concurrent write from a separate task. See `tests::between_upstream_authorization_probe`. + #[cfg(test)] + tests::between_upstream_authorization_probe(pr.direction_id).await; + + // Step 5.5: re-check the upstream axis ONE more time, as close to the actual merge call as + // this function can get — Codex review, PR #159 automerge.rs:395. Unlike CI/review/ + // conflict, GitHub has no concept of this product's own cross-repo dependency ordering to + // enforce server-side: `--match-head-commit` (step 6, in `maybe_merge_one`) only guards the + // PR's OWN head commit moving, never this LOCAL fact. Without this, a re-proposal that + // adds or replaces this consumer's dependency in the window between the read above and + // `run_gh_merge` actually executing would merge a consumer whose upstream just changed, + // with no backstop at all — CI/review/conflict staleness in that same window is at least + // partly covered by GitHub's OWN branch-protection enforcement, but this axis is purely a + // Weft-side invariant that only Weft can protect. This is INTENTIONALLY separate from (and + // does not touch) `record_upstream_edges`'s two-pass write-ordering mechanism (round 5+6): + // that solves DB write atomicity for a multi-edge write; this is a read-authorize-execute + // TOCTOU window on this file's OWN side, and re-reading the SAME already-atomic source of + // truth closer to the action is sufficient — no new lock or generation counter needed, and + // none of that machinery would help here anyway (it does not extend the window this file's + // own control flow takes to reach `run_gh_merge`). + // + // A sixth review round (Codex review, PR #159 automerge.rs:457) pressed on the word + // "narrows" above: this re-check does NOT close the window, it only moves it as late as + // this file's own control flow allows. The REMAINING window runs from the instant this + // read returns to the instant GitHub actually processes the `gh pr merge` call dispatched + // in `maybe_merge_one`'s step 6 — dominated by process-spawn + network round-trip time to + // GitHub's API (realistically ~100ms to a few seconds, NOT a negligible number of CPU + // cycles; the handful of synchronous Rust statements between this check and the + // `spawn_blocking` call in step 6 contribute microseconds, not the risk). + // + // Full closure was considered and rejected as disproportionate to the residual risk: + // - A lock held from this re-check through `run_gh_merge`'s completion would need a NEW + // coordination primitive between this module and every writer of + // `depends_on_direction_id` (`planner::set_upstream_edge_if_changed`) — a module that + // today has zero automerge awareness — plus its own stuck-lock recovery story for a + // crashed/hung `gh` process, to gate a race this narrow. + // - A generation counter checked just before dispatch does not actually help: unlike + // `--match-head-commit` (enforced ATOMICALLY by GitHub itself, server-side, as part of + // executing the merge), there is no GitHub-side hook for this Weft-only axis, so + // "check-then-dispatch" has the identical TOCTOU shape no matter how late the check + // runs, unless it is ITSELF paired with the same lock above — it does not remove the + // cost, it only relocates it. + // - A synthetic GitHub check run plus a required branch-protection rule could in + // principle give this axis the same atomic, server-side enforcement + // `--match-head-commit` gets for head-sha staleness — but that depends on external repo + // configuration this codebase does not manage today, and would be a new integration + // surface (Checks API, a setup burden on every target repo) rather than a + // review-response-sized fix. + // + // The residual window is accepted, not ignored: reaching it requires a human or lead to + // CONFIRM a re-proposal that changes THIS SPECIFIC direction's upstream within a + // sub-few-second window that carries no visible "a merge is dispatching right now" cue — + // and even if hit, `maybe_merge_one`'s own step 7 re-reads and re-persists this exact row's + // true state, INCLUDING this axis, immediately after the merge attempt completes, so the + // consequence is bounded to one merge whose upstream had just changed (a recoverable, + // visible-on-CI outcome, the same class of risk as any other check-then-merge gap) and + // self-corrects in the DB rather than silently drifting. + match repo::upstream_merge_state(db, pr.direction_id).await { + super::UpstreamStatus::None | super::UpstreamStatus::Merged => {} + other => { + eprintln!( + "[weft][automerge] pr #{}: upstream changed to {other:?} after authorization — \ + aborting this merge attempt", + pr.id + ); + return RowVerdict::Skip; + } + } + RowVerdict::Merge { head_sha: snapshot.head_sha } } @@ -469,7 +546,12 @@ async fn maybe_merge_one( // Step 7: regardless of outcome, one more fresh read (same injected // resolver as step 4) + persist + marker. - let target2 = PrTarget { owner: pr.host_owner.clone(), repo: pr.host_repo.clone(), number: pr.number }; + let target2 = PrTarget { + host_base: pr.host_base.clone(), + owner: pr.host_owner.clone(), + repo: pr.host_repo.clone(), + number: pr.number, + }; let confirmed = tokio::task::spawn_blocking(move || { resolver(host_kind).and_then(|h| h.fetch_status(&target2)) }) @@ -482,7 +564,8 @@ async fn maybe_merge_one( let (state, state_error) = match &confirmed { Ok(s) => { - let r = judge::merge_readiness(&s.ci, &s.review, &s.conflict); + let upstream = repo::upstream_merge_state(db, pr.direction_id).await; + let r = judge::merge_readiness(&s.ci, &s.review, &s.conflict, &upstream); if let Err(e) = repo::apply_pull_request_snapshot(db, pr.id, s, &r).await { eprintln!( "[weft][automerge] pr #{}: could not save confirmation snapshot: {e}", @@ -558,22 +641,13 @@ fn lifecycle_state_tag(lifecycle: PrLifecycle) -> &'static str { /// /// `host_base` (the hostname recorded at registration — see `host/mod.rs`'s /// module doc on why GitHub Enterprise needs this recorded per row) is -/// threaded into `--repo` as `HOST/OWNER/REPO` when non-empty, so this call -/// targets the SAME host the row was registered against rather than -/// whatever `gh`'s own configured default happens to be (review round 1 -/// Codex P1). +/// threaded into `--repo` as `HOST/OWNER/REPO` when non-empty, via the SAME +/// `super::qualified_repo_slug` `github::GitHubHost::fetch_status` uses, so this call and every +/// status read of the same row can never independently drift on which host they each target +/// (review round 1 Codex P1; Codex review, PR #159 repo.rs:3873 — the status-fetch path used +/// to skip this entirely). fn run_gh_merge(host_base: &str, owner: &str, repo: &str, number: i32, head_sha: &str) -> Result<(), String> { - // Same defense-in-depth guard `github::GitHubHost::fetch_status` applies - // independently of `parse_pr_url`'s own validation, for the exact same - // reason: `--repo` here is built the same way (a joined argument), so it - // inherits the exact same confirmed SSRF risk (see `parse_pr_url`'s doc) - // and gets the exact same guard — extended to `host_base` too, since - // that is now ALSO folded into the same argument. - if host_base.contains('/') || owner.contains('/') || repo.contains('/') { - return Err(format!( - "refusing to merge a repo slug with an embedded '/' (host_base={host_base:?}, owner={owner:?}, repo={repo:?}) — this would be reinterpreted as a host override" - )); - } + let repo_arg = super::qualified_repo_slug(host_base, owner, repo)?; // A `Ready` verdict can only ever be produced from a SUCCESSFUL snapshot // (which always sets a real `head_sha`), so this should be unreachable // through the gate above — kept anyway as cheap, independent insurance @@ -581,9 +655,8 @@ fn run_gh_merge(host_base: &str, owner: &str, repo: &str, number: i32, head_sha: if head_sha.is_empty() { return Err("refusing to merge: no confirmed head_sha on record".to_string()); } - let repo_slug = format!("{owner}/{repo}"); let out = Command::new("gh") - .args(build_merge_args(host_base, &repo_slug, number, head_sha)) + .args(build_merge_args(&repo_arg, number, head_sha)) // Checks run user tooling that a GUI launch's minimal PATH can't // resolve (Homebrew/local installs of `gh`) — same reasoning as // `github::GitHubHost::fetch_status` / `check::run_check`. @@ -604,29 +677,22 @@ fn run_gh_merge(host_base: &str, owner: &str, repo: &str, number: i32, head_sha: } /// The exact `gh pr merge` argument vector, pulled out as its own pure -/// function so both the presence of `--match-head-commit` (this feature's +/// function so the presence of `--match-head-commit` (this feature's /// server-side head-consistency enforcement — the mechanism `gate`'s own doc -/// points to instead of a second client-side sha comparison) AND the -/// host-targeting `--repo` value are independently, directly unit-tested -/// below without ever spawning a process. `run_gh_merge` is the only caller; -/// nothing here talks to `gh`. -fn build_merge_args(host_base: &str, repo_slug: &str, number: i32, head_sha: &str) -> Vec { - // `gh`'s `-R/--repo` grammar is `[HOST/]OWNER/REPO` — an explicit host - // segment selects that host instead of `gh`'s own configured default. - // Empty `host_base` (a row from before this field existed) falls back to - // the plain two-segment form, preserving `gh`'s own default-host - // behavior exactly as before this fix. - let repo_arg = if host_base.is_empty() { - repo_slug.to_string() - } else { - format!("{host_base}/{repo_slug}") - }; +/// points to instead of a second client-side sha comparison) is +/// independently, directly unit-tested below without ever spawning a +/// process. `run_gh_merge` is the only caller; nothing here talks to `gh`. +/// `repo_arg` arrives ALREADY host-qualified (`super::qualified_repo_slug`, +/// called by `run_gh_merge` — see that function's doc for why the folding +/// itself lives there, shared with `github::GitHubHost::fetch_status`, +/// rather than duplicated in this formatter too). +fn build_merge_args(repo_arg: &str, number: i32, head_sha: &str) -> Vec { vec![ "pr".to_string(), "merge".to_string(), number.to_string(), "--repo".to_string(), - repo_arg, + repo_arg.to_string(), "--squash".to_string(), "--match-head-commit".to_string(), head_sha.to_string(), @@ -769,13 +835,15 @@ mod tests { } } - // --- build_merge_args: the head-consistency AND host-targeting - // enforcement must actually reach the `gh` invocation, not just exist in - // a doc comment -------------------------------------------------------- + // --- build_merge_args: the head-consistency enforcement must actually + // reach the `gh` invocation, not just exist in a doc comment. Host- + // targeting (`[HOST/]OWNER/REPO` folding) is `super::qualified_repo_slug`'s + // job now — see `host::tests` for that coverage, shared with + // `github::GitHubHost::fetch_status` -------------------------------------- #[test] fn build_merge_args_always_squashes_and_pins_match_head_commit_to_the_exact_sha() { - let args = build_merge_args("", "acme/widgets", 42, "deadbeef"); + let args = build_merge_args("acme/widgets", 42, "deadbeef"); assert_eq!( args, vec!["pr", "merge", "42", "--repo", "acme/widgets", "--squash", "--match-head-commit", "deadbeef"] @@ -793,25 +861,16 @@ mod tests { } #[test] - fn build_merge_args_folds_a_non_empty_host_base_into_the_repo_argument() { - // Review round 1 Codex P1: a GitHub Enterprise row's recorded host - // must actually reach the invocation, not silently fall back to - // `gh`'s own default host. - let args = build_merge_args("github.acme-corp.com", "acme/widgets", 42, "deadbeef"); + fn build_merge_args_passes_through_an_already_host_qualified_repo_arg_verbatim() { + // Review round 1 Codex P1 / Codex review PR #159 repo.rs:3873: a GitHub Enterprise + // row's recorded host must actually reach the invocation — folding now happens in + // `super::qualified_repo_slug` (`run_gh_merge`'s job to call), so this only needs to + // prove the ALREADY-qualified string reaches --repo verbatim, untouched. + let args = build_merge_args("github.acme-corp.com/acme/widgets", 42, "deadbeef"); let idx = args.iter().position(|a| a == "--repo").unwrap(); assert_eq!(args[idx + 1], "github.acme-corp.com/acme/widgets"); } - #[test] - fn build_merge_args_empty_host_base_preserves_the_plain_two_segment_form() { - // A row from before `host_base` was recorded (or a plain github.com - // one, if this crate ever stops populating it for that case) must - // not regress into passing a bare empty host segment. - let args = build_merge_args("", "acme/widgets", 42, "deadbeef"); - let idx = args.iter().position(|a| a == "--repo").unwrap(); - assert_eq!(args[idx + 1], "acme/widgets"); - } - // --- lifecycle_state_tag: exhaustive, always distinguishable ---------- #[test] @@ -996,6 +1055,49 @@ mod tests { }))) } + /// Test-only seam (mirrors `planner::tests::between_upstream_passes_probe`): lets a test + /// PAUSE `evaluate_row` exactly between its FIRST upstream read (step 4) and its re-check + /// (step 5.5) — the same window a concurrent re-proposal could land in (Codex review, PR + /// #159 automerge.rs:395) — write a NEW dependency onto `direction_id` from a SEPARATE + /// task, then release it. `arm_between_upstream_authorization_probe(direction_id)` returns + /// `(reached_rx, resume_tx)`: `reached_rx` resolves once the probe is hit, and sending on + /// `resume_tx` lets `evaluate_row` continue into its re-check. A no-op (never blocks) for + /// any direction_id that wasn't armed. + #[allow(clippy::type_complexity)] + fn between_upstream_authorization_map() -> &'static std::sync::Mutex< + std::collections::HashMap, tokio::sync::oneshot::Receiver<()>)>, + > { + static M: std::sync::OnceLock< + std::sync::Mutex< + std::collections::HashMap, tokio::sync::oneshot::Receiver<()>)>, + >, + > = std::sync::OnceLock::new(); + M.get_or_init(|| std::sync::Mutex::new(std::collections::HashMap::new())) + } + + fn arm_between_upstream_authorization_probe( + direction_id: i32, + ) -> (tokio::sync::oneshot::Receiver<()>, tokio::sync::oneshot::Sender<()>) { + let (reached_tx, reached_rx) = tokio::sync::oneshot::channel(); + let (resume_tx, resume_rx) = tokio::sync::oneshot::channel(); + between_upstream_authorization_map() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .insert(direction_id, (reached_tx, resume_rx)); + (reached_rx, resume_tx) + } + + pub(super) async fn between_upstream_authorization_probe(direction_id: i32) { + let armed = between_upstream_authorization_map() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .remove(&direction_id); + if let Some((reached_tx, resume_rx)) = armed { + let _ = reached_tx.send(()); + let _ = resume_rx.await; + } + } + /// A tracked row whose STORED state is fully ready (`pre_decision` — /// step 1 — reads `Merge`) and the opt-in switch is on — the exact /// precondition every test above needs, isolated here once. Deliberately @@ -1094,6 +1196,66 @@ mod tests { assert_eq!(verdict, RowVerdict::Merge { head_sha: "fresh_sha_fully_ready".to_string() }); } + /// THE FIX (Codex review, PR #159 automerge.rs:395): between `evaluate_row`'s upstream + /// read (step 4) and its FINAL authorization, a re-proposal could add or replace this + /// consumer's dependency — `--match-head-commit` (step 6, in `maybe_merge_one`) only + /// guards the PR's OWN head commit moving, never this LOCAL ordering fact, so an already- + /// authorized sweep would otherwise merge a consumer whose upstream just changed, with no + /// backstop at all. Pauses `evaluate_row` exactly in that window (`between_upstream_ + /// authorization_probe`, mirroring `planner::tests::between_upstream_passes_probe`'s exact + /// pattern) and drives a genuinely concurrent write — `repo::set_direction_upstream`, the + /// same primitive `record_upstream_edges` itself calls — from a separately spawned task. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn evaluate_row_aborts_when_a_dependency_is_added_after_authorization_but_before_the_recheck() { + let tag = format!("weft-automerge-upstream-race-{}", std::process::id()); + let weft_home = std::env::temp_dir().join(format!("{tag}-home")); + let _ = std::fs::remove_dir_all(&weft_home); + std::fs::create_dir_all(&weft_home).unwrap(); + // File-backed (not :memory:) DB, same reason as `planner::tests::thread_gate_ + // serializes_concurrent_confirms`: the cloned pool handle in the spawned task must see + // the SAME store as the test's own concurrent write. + let db_file = weft_home.join("automerge-race.sqlite"); + let db = Db::connect(&format!("sqlite://{}?mode=rwc", db_file.to_str().unwrap())) + .await + .unwrap(); + let pr = seam_fixture(&db).await; // dependency-free, everything else Ready + let consumer_direction_id = pr.direction_id; + // A real, undecided producer to attach as the race's dependency. + let thread = repo::get_thread(&db, pr.thread_id).await.unwrap().unwrap(); + let repo_ref = repo::get_repo(&db, pr.repo_id).await.unwrap().unwrap(); + let producer = repo::create_direction( + &db, thread.id, "producer", "codex", repo_ref.id, "why", "impl-only", "", + ) + .await + .unwrap(); + let backoff = MergeBackoffState::default(); + + let (reached_rx, resume_tx) = tests::arm_between_upstream_authorization_probe(consumer_direction_id); + let db2 = db.clone(); + let pr2 = pr.clone(); + let backoff2 = backoff.clone(); + let evaluate_handle = tokio::spawn(async move { + evaluate_row(&db2, &pr2, HostKind::GitHub, &backoff2, resolver_fresh_fully_ready).await + }); + + // Wait for evaluate_row to reach the gap between its upstream read and its re-check, + // then — WHILE it is still paused there — add a real dependency from a separate task, + // the same write `record_upstream_edges` itself performs. + reached_rx.await.expect("evaluate_row reached the between-authorization probe"); + repo::set_direction_upstream(&db, consumer_direction_id, producer.id).await.unwrap(); + let _ = resume_tx.send(()); + + let verdict = evaluate_handle.await.unwrap(); + assert_eq!( + verdict, + RowVerdict::Skip, + "a dependency added between authorization and the re-check must abort this merge \ + attempt, not silently proceed on the stale (dependency-free) read" + ); + + let _ = std::fs::remove_dir_all(&weft_home); + } + #[tokio::test] async fn evaluate_row_respects_the_backoff_even_when_the_stored_row_still_looks_ready() { let db = Db::connect("sqlite::memory:").await.unwrap(); diff --git a/src-tauri/src/host/github.rs b/src-tauri/src/host/github.rs index 505f824f..f53290f9 100644 --- a/src-tauri/src/host/github.rs +++ b/src-tauri/src/host/github.rs @@ -27,23 +27,14 @@ impl PrHost for GitHubHost { } fn fetch_status(&self, target: &PrTarget) -> Result { - // Defense in depth alongside `host::parse_pr_url`'s own validation: - // this format! joins owner/repo into a SINGLE `--repo` argument, and - // `gh`'s `[HOST/]OWNER/REPO` grammar treats an extra `/`-delimited - // segment as a HOST OVERRIDE (the confirmed SSRF — see - // `parse_pr_url`'s doc). `parse_pr_url` is the only current producer - // of a `PrTarget`, but refusing here too means this call can never be - // tricked into a host override even if some future caller builds a - // `PrTarget` another way. - if target.owner.contains('/') || target.repo.contains('/') { - return Err(HostError::Other { - message: format!( - "refusing to query a repo slug with an embedded '/' (owner={:?}, repo={:?}) — this would be reinterpreted as a host override", - target.owner, target.repo - ), - }); - } - let repo_slug = format!("{}/{}", target.owner, target.repo); + // `super::qualified_repo_slug` folds `target.host_base` in (GitHub Enterprise support — + // Codex review, PR #159 repo.rs:3873: this call used to build a bare OWNER/REPO + // argument with no host at all, always querying `gh`'s own configured default instead + // of the recorded install) and refuses an embedded '/' in any of the three inputs + // before this can ever shell out — the confirmed SSRF this crate's `[HOST/]OWNER/REPO` + // grammar otherwise allows (see `parse_pr_url`'s doc). + let repo_slug = super::qualified_repo_slug(&target.host_base, &target.owner, &target.repo) + .map_err(|message| HostError::Other { message })?; let out = Command::new("gh") .args(["pr", "view", &target.number.to_string(), "--repo", &repo_slug, "--json", JSON_FIELDS]) // Checks run user tooling that a GUI launch's minimal PATH can't @@ -363,8 +354,12 @@ mod tests { // without ever spawning a process (the guard returns before // `Command::new` runs). let host = GitHubHost; - let bad_owner = - PrTarget { owner: "evil.example.org/ownerx".to_string(), repo: "repox".to_string(), number: 5 }; + let bad_owner = PrTarget { + host_base: String::new(), + owner: "evil.example.org/ownerx".to_string(), + repo: "repox".to_string(), + number: 5, + }; match host.fetch_status(&bad_owner) { Err(HostError::Other { message }) => { assert!(message.contains("host override"), "got: {message}"); @@ -372,13 +367,33 @@ mod tests { other => panic!("expected the embedded-slash guard's own error, got {other:?}"), } - let bad_repo = PrTarget { owner: "owner".to_string(), repo: "a/b".to_string(), number: 5 }; + let bad_repo = PrTarget { + host_base: String::new(), + owner: "owner".to_string(), + repo: "a/b".to_string(), + number: 5, + }; match host.fetch_status(&bad_repo) { Err(HostError::Other { message }) => { assert!(message.contains("host override"), "got: {message}"); } other => panic!("expected the embedded-slash guard's own error, got {other:?}"), } + + // Codex review, PR #159 repo.rs:3873: `host_base` now reaches this call too (it used + // to be entirely absent from `PrTarget`) — the guard must cover it just as strictly. + let bad_host_base = PrTarget { + host_base: "evil.example.org/extra".to_string(), + owner: "owner".to_string(), + repo: "repo".to_string(), + number: 5, + }; + match host.fetch_status(&bad_host_base) { + Err(HostError::Other { message }) => { + assert!(message.contains("host override"), "got: {message}"); + } + other => panic!("expected the embedded-slash guard's own error for a smuggled host_base, got {other:?}"), + } } #[test] diff --git a/src-tauri/src/host/judge.rs b/src-tauri/src/host/judge.rs index c3277b1a..4cfa73fb 100644 --- a/src-tauri/src/host/judge.rs +++ b/src-tauri/src/host/judge.rs @@ -5,12 +5,22 @@ //! judgement; everything else here builds the human-facing Needs-you notice //! from its result. //! +//! A FOURTH axis joins those three for cross-repo change sets: an upstream +//! task's PR must be merged before this one is mergeable at all. It is an axis +//! rather than a separate gate on purpose — [`merge_readiness`] is the single +//! source of truth every consumer already reads (monitor notice, auto-merge +//! gate, the UI), so ordering flows to all of them by construction instead of +//! being re-derived per caller. +//! //! This file is the primary target of this PR's mutation self-check (see the //! PR body): flip any arm of [`ci_verdict`] / [`review_verdict`] / -//! [`conflict_verdict`] / the `has_unknown` / `has_blocking` branches in -//! [`merge_readiness`], and a test below must go red. +//! [`conflict_verdict`] / [`upstream_verdict`] / the `has_unknown` / +//! `has_blocking` branches in [`merge_readiness`], and a test below must go +//! red. -use super::{CiStatus, ConflictStatus, HostError, HostKind, MergeReadiness, ReviewStatus}; +use super::{ + CiStatus, ConflictStatus, HostError, HostKind, MergeReadiness, ReviewStatus, UpstreamStatus, +}; /// Each axis reduced to a 3-way verdict before combining — keeps /// `merge_readiness` a single small exhaustive match instead of a spelled-out @@ -41,6 +51,22 @@ fn review_verdict(s: &ReviewStatus) -> AxisVerdict { } } +fn upstream_verdict(s: &UpstreamStatus) -> AxisVerdict { + match s { + UpstreamStatus::Unknown { .. } => AxisVerdict::Unknown, + UpstreamStatus::None | UpstreamStatus::Merged => AxisVerdict::Clear, + UpstreamStatus::Pending { .. } => AxisVerdict::Blocking, + } +} + +fn upstream_reason(s: &UpstreamStatus) -> Option { + match s { + UpstreamStatus::Unknown { reason } => Some(format!("上游任务状态未知({reason})")), + UpstreamStatus::Pending { what } => Some(format!("上游任务「{what}」还没合并")), + UpstreamStatus::None | UpstreamStatus::Merged => None, + } +} + fn conflict_verdict(s: &ConflictStatus) -> AxisVerdict { match s { ConflictStatus::Unknown { .. } => AxisVerdict::Unknown, @@ -93,22 +119,34 @@ fn conflict_reason(s: &ConflictStatus) -> Option { /// as `Indeterminate` (we must never claim `Ready` OR `Blocked` when we can't /// actually tell); otherwise any `Blocking` axis makes it `Blocked`; /// otherwise `Ready`. `reasons` always lists every non-clear axis (not just -/// the first found), in a fixed CI → review → conflict order so the same +/// the first found), in a fixed CI → review → conflict → upstream order so +/// the same /// input always renders identical text (no notice-flapping on wording order /// alone — see `plan_notice_action`). pub fn merge_readiness( ci: &CiStatus, review: &ReviewStatus, conflict: &ConflictStatus, + upstream: &UpstreamStatus, ) -> MergeReadiness { - let verdicts = [ci_verdict(ci), review_verdict(review), conflict_verdict(conflict)]; + let verdicts = [ + ci_verdict(ci), + review_verdict(review), + conflict_verdict(conflict), + upstream_verdict(upstream), + ]; let has_unknown = verdicts.contains(&AxisVerdict::Unknown); let has_blocking = verdicts.contains(&AxisVerdict::Blocking); - let reasons: Vec = [ci_reason(ci), review_reason(review), conflict_reason(conflict)] - .into_iter() - .flatten() - .collect(); + let reasons: Vec = [ + ci_reason(ci), + review_reason(review), + conflict_reason(conflict), + upstream_reason(upstream), + ] + .into_iter() + .flatten() + .collect(); if has_unknown { MergeReadiness::Indeterminate { reasons } @@ -214,17 +252,116 @@ mod tests { (CiStatus::Passing, ReviewStatus::Approved, ConflictStatus::Clean) } + /// The ordering axis, on a change set whose other three axes are perfect. + /// This is the whole point: a consumer PR can be green, approved and + /// conflict-free and STILL not be mergeable, because merging it would land + /// a commit referencing a producer change that is not on any default + /// branch yet. + #[test] + fn a_pending_upstream_blocks_an_otherwise_perfect_pr() { + let (ci, review, conflict) = ready(); + let readiness = merge_readiness( + &ci, + &review, + &conflict, + &UpstreamStatus::Pending { what: "chat-kit: expose send".into() }, + ); + match readiness { + MergeReadiness::Blocked { reasons } => { + assert_eq!(reasons.len(), 1, "only the ordering axis is unclear"); + assert!( + reasons[0].contains("chat-kit: expose send"), + "the notice must name WHICH task is holding it: {reasons:?}" + ); + } + other => panic!("a pending upstream must block, got {other:?}"), + } + } + + /// Once the producer lands, the consumer is free — no residue. + #[test] + fn a_merged_upstream_clears_the_axis() { + let (ci, review, conflict) = ready(); + assert_eq!( + merge_readiness(&ci, &review, &conflict, &UpstreamStatus::Merged), + MergeReadiness::Ready + ); + } + + /// An unresolvable upstream must never read as "no upstream". `None` would + /// release the merge; the honest answer to "can we tell?" is no, and this + /// axis obeys the same rule as the other three. + #[test] + fn an_unknown_upstream_is_indeterminate_not_ready() { + let (ci, review, conflict) = ready(); + let readiness = merge_readiness( + &ci, + &review, + &conflict, + &UpstreamStatus::Unknown { reason: "上游任务 #7 不存在".into() }, + ); + match readiness { + MergeReadiness::Indeterminate { reasons } => { + assert!(reasons[0].contains("#7"), "reason keeps the diagnostic: {reasons:?}"); + } + other => panic!("an unknown upstream must be indeterminate, got {other:?}"), + } + } + + /// Ordering does not outrank the other axes — it joins them. A PR blocked + /// on BOTH a failing CI and a pending upstream reports both, in the fixed + /// order the notice depends on. + #[test] + fn upstream_joins_the_other_axes_rather_than_overriding_them() { + let readiness = merge_readiness( + &CiStatus::Failing, + &ReviewStatus::Approved, + &ConflictStatus::Clean, + &UpstreamStatus::Pending { what: "producer".into() }, + ); + match readiness { + MergeReadiness::Blocked { reasons } => { + assert_eq!(reasons.len(), 2, "both axes are named: {reasons:?}"); + assert!(reasons[0].contains("CI"), "CI comes first: {reasons:?}"); + assert!(reasons[1].contains("producer"), "upstream comes last: {reasons:?}"); + } + other => panic!("expected blocked, got {other:?}"), + } + } + + /// A task with no ordering edge behaves exactly as before this axis + /// existed — the overwhelmingly common case must be untouched. + #[test] + fn no_upstream_is_indistinguishable_from_the_three_axis_judgement() { + for (ci, review, conflict) in [ + (CiStatus::Passing, ReviewStatus::Approved, ConflictStatus::Clean), + (CiStatus::Failing, ReviewStatus::Approved, ConflictStatus::Clean), + ( + CiStatus::Unknown { reason: "x".into() }, + ReviewStatus::Approved, + ConflictStatus::Clean, + ), + ] { + let with_none = merge_readiness(&ci, &review, &conflict, &UpstreamStatus::None); + let with_merged = merge_readiness(&ci, &review, &conflict, &UpstreamStatus::Merged); + assert_eq!( + with_none, with_merged, + "None and Merged are both Clear, so they must agree" + ); + } + } + #[test] fn all_three_axes_clear_is_ready() { let (ci, review, conflict) = ready(); - assert_eq!(merge_readiness(&ci, &review, &conflict), MergeReadiness::Ready); + assert_eq!(merge_readiness(&ci, &review, &conflict, &UpstreamStatus::None), MergeReadiness::Ready); } #[test] fn ci_not_configured_counts_as_clear_not_blocking() { let (_, review, conflict) = ready(); assert_eq!( - merge_readiness(&CiStatus::NotConfigured, &review, &conflict), + merge_readiness(&CiStatus::NotConfigured, &review, &conflict, &UpstreamStatus::None), MergeReadiness::Ready ); } @@ -232,7 +369,7 @@ mod tests { #[test] fn failing_ci_alone_blocks() { let (_, review, conflict) = ready(); - match merge_readiness(&CiStatus::Failing, &review, &conflict) { + match merge_readiness(&CiStatus::Failing, &review, &conflict, &UpstreamStatus::None) { MergeReadiness::Blocked { reasons } => { assert_eq!(reasons, vec!["CI 未通过".to_string()]); } @@ -244,7 +381,7 @@ mod tests { fn pending_ci_alone_blocks() { let (_, review, conflict) = ready(); assert!(matches!( - merge_readiness(&CiStatus::Pending, &review, &conflict), + merge_readiness(&CiStatus::Pending, &review, &conflict, &UpstreamStatus::None), MergeReadiness::Blocked { .. } )); } @@ -253,7 +390,7 @@ mod tests { fn changes_requested_alone_blocks() { let (ci, _, conflict) = ready(); assert!(matches!( - merge_readiness(&ci, &ReviewStatus::ChangesRequested, &conflict), + merge_readiness(&ci, &ReviewStatus::ChangesRequested, &conflict, &UpstreamStatus::None), MergeReadiness::Blocked { .. } )); } @@ -268,7 +405,7 @@ mod tests { &ci, &ReviewStatus::AwaitingApproval { unresolved_discussions: None }, &conflict - ), + , &UpstreamStatus::None), MergeReadiness::Blocked { .. } )); } @@ -286,7 +423,7 @@ mod tests { &ci, &ReviewStatus::AwaitingApproval { unresolved_discussions }, &conflict - ), + , &UpstreamStatus::None), MergeReadiness::Blocked { .. } ), "unresolved_discussions={unresolved_discussions:?} must still block" @@ -301,6 +438,7 @@ mod tests { &ci, &ReviewStatus::AwaitingApproval { unresolved_discussions: Some(true) }, &conflict, + &UpstreamStatus::None, ); match readiness { MergeReadiness::Blocked { reasons } => { @@ -314,7 +452,7 @@ mod tests { fn conflicting_alone_blocks() { let (ci, review, _) = ready(); assert!(matches!( - merge_readiness(&ci, &review, &ConflictStatus::Conflicting), + merge_readiness(&ci, &review, &ConflictStatus::Conflicting, &UpstreamStatus::None), MergeReadiness::Blocked { .. } )); } @@ -327,6 +465,7 @@ mod tests { &CiStatus::Unknown { reason: "gh not authenticated".to_string() }, &ReviewStatus::ChangesRequested, &ConflictStatus::Conflicting, + &UpstreamStatus::None, ); match readiness { MergeReadiness::Indeterminate { reasons } => { @@ -343,6 +482,7 @@ mod tests { &CiStatus::Failing, &ReviewStatus::ChangesRequested, &ConflictStatus::Conflicting, + &UpstreamStatus::None, ); match readiness { MergeReadiness::Blocked { reasons } => { diff --git a/src-tauri/src/host/mod.rs b/src-tauri/src/host/mod.rs index 32e88a4d..36376ce7 100644 --- a/src-tauri/src/host/mod.rs +++ b/src-tauri/src/host/mod.rs @@ -99,13 +99,54 @@ impl HostKind { /// One PR/MR's identity + where to find it, host-agnostic. A `PrHost` maps /// this into its own CLI invocation. +/// +/// `host_base` (the hostname recorded at registration — e.g. a GitHub Enterprise install; see +/// [`qualified_repo_slug`]) must reach every `PrHost` invocation, not just the merge path +/// (Codex review, PR #159 repo.rs:3873): `github::GitHubHost::fetch_status` used to build its +/// `--repo` argument from `owner`/`repo` alone, with no way to carry a recorded host at all — +/// so a GHE row's STATUS reads always queried `gh`'s own configured default host instead of the +/// recorded install, while `automerge::run_gh_merge`'s MERGE call already folded `host_base` in +/// correctly (since review round 1). Two paths inside the SAME feature disagreeing about which +/// server to talk to for the SAME row is a strong signal on its own, and the consequence is +/// real: a GHE user could have status read from (or silently fail against) the wrong server +/// entirely, then have that status drive an irreversible merge decision. #[derive(Clone, Debug, PartialEq, Eq)] pub struct PrTarget { + pub host_base: String, pub owner: String, pub repo: String, pub number: i32, } +/// Build the `[HOST/]OWNER/REPO` argument `gh`'s `--repo`/`-R` flag expects, or reject the call +/// before it can ever shell out — the ONE place every `gh` invocation in this crate resolves +/// which server to target, so no two call sites can independently drift on it (see +/// [`PrTarget`]'s doc for why that drift was a real bug, not a hypothetical one). +/// +/// GitHub Enterprise needs the host segment to target the recorded install instead of `gh`'s +/// own configured default; `host_base` of `""` (a row from before this field existed, or a +/// plain github.com one) falls back to the plain two-segment form, matching `gh`'s own +/// default-host behavior exactly — a no-op for the overwhelming majority of checkouts. +/// +/// Refuses an embedded `/` in ANY of the three inputs before ever building the argument: `gh`'s +/// `[HOST/]OWNER/REPO` grammar treats an extra `/`-delimited segment as a HOST OVERRIDE — this +/// crate's confirmed SSRF vector if an owner/repo/host string reaches a shell-out with an +/// unexpected slash in it (see [`parse_pr_url`]'s doc). Centralizing the guard here, alongside +/// the formatting it protects, means a future THIRD caller cannot forget it the way the +/// status-fetch path once forgot to fold in `host_base` at all. +pub fn qualified_repo_slug(host_base: &str, owner: &str, repo: &str) -> Result { + if host_base.contains('/') || owner.contains('/') || repo.contains('/') { + return Err(format!( + "refusing to target a repo slug with an embedded '/' (host_base={host_base:?}, owner={owner:?}, repo={repo:?}) — this would be reinterpreted as a host override" + )); + } + Ok(if host_base.is_empty() { + format!("{owner}/{repo}") + } else { + format!("{host_base}/{owner}/{repo}") + }) +} + /// Lifecycle of the change unit itself — distinct from merge READINESS, which /// only makes sense while `Open` (see `judge::notice_text`'s caller in /// `monitor`, which treats a non-`Open` row as "nothing left to judge"). @@ -201,6 +242,28 @@ pub enum ConflictStatus { Conflicting, } +/// Whether the task this PR belongs to is waiting on ANOTHER task's PR. +/// +/// Ordering exists because a consumer repo pinned to a producer (a submodule +/// SHA, a version bump) cannot be green until the producer's change is on its +/// default branch. Merging the consumer first does not merely reorder work — +/// it lands a commit referencing something nobody else can resolve. +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[serde(tag = "state", rename_all = "snake_case")] +pub enum UpstreamStatus { + /// No ordering edge recorded — the overwhelmingly common case, and the + /// only one that existed before cross-repo change sets. + None, + /// Every upstream task's PR is merged; this one is free to go. + Merged, + /// An upstream is still outstanding. `what` names it for the human. + Pending { what: String }, + /// An upstream edge exists but its state could not be established (the + /// task row is gone, its PR was never registered). Deliberately NOT + /// treated as "no upstream": the honest answer to "can we tell?" is no. + Unknown { reason: String }, +} + /// The `truly mergeable` bar (this repo's CLAUDE.md "GitHub Remote Review /// Workflow" section), turned into code instead of prose: CI green × review /// clear/approved × no conflict. See `judge::merge_readiness` for the @@ -427,6 +490,45 @@ mod tests { assert_eq!(HostKind::GitLab.as_str(), "gitlab"); } + // --- qualified_repo_slug: the ONE shared source both `github::GitHubHost:: + // fetch_status` and `automerge::run_gh_merge` resolve a `gh --repo` target + // through — Codex review, PR #159 repo.rs:3873 --------------------------- + + #[test] + fn qualified_repo_slug_folds_a_non_empty_host_base_in() { + // Review round 1 Codex P1 (originally caught on the merge path only): + // a GitHub Enterprise row's recorded host must actually reach the + // invocation, not silently fall back to `gh`'s own default host. + assert_eq!( + qualified_repo_slug("github.acme-corp.com", "acme", "widgets").unwrap(), + "github.acme-corp.com/acme/widgets" + ); + } + + #[test] + fn qualified_repo_slug_empty_host_base_preserves_the_plain_two_segment_form() { + // A row from before `host_base` was recorded (or a plain github.com + // one) must not regress into passing a bare empty host segment. + assert_eq!(qualified_repo_slug("", "acme", "widgets").unwrap(), "acme/widgets"); + } + + #[test] + fn qualified_repo_slug_refuses_an_embedded_slash_in_any_of_the_three_inputs() { + for (host_base, owner, repo) in [ + ("evil.example.org/extra", "acme", "widgets"), + ("", "evil.example.org/acme", "widgets"), + ("", "acme", "evil.example.org/widgets"), + ] { + match qualified_repo_slug(host_base, owner, repo) { + Err(message) => assert!(message.contains("host override"), "got: {message}"), + Ok(slug) => panic!( + "expected the embedded-slash guard to fire for host_base={host_base:?} \ + owner={owner:?} repo={repo:?}, got a slug instead: {slug:?}" + ), + } + } + } + #[test] fn native_terminology_is_host_specific() { assert_eq!(HostKind::GitHub.native_noun(), "Pull request"); diff --git a/src-tauri/src/host/monitor.rs b/src-tauri/src/host/monitor.rs index 6ab830ee..8acc1c6d 100644 --- a/src-tauri/src/host/monitor.rs +++ b/src-tauri/src/host/monitor.rs @@ -148,7 +148,12 @@ async fn check_one( .await; return; }; - let target = PrTarget { owner: pr.host_owner.clone(), repo: pr.host_repo.clone(), number: pr.number }; + let target = PrTarget { + host_base: pr.host_base.clone(), + owner: pr.host_owner.clone(), + repo: pr.host_repo.clone(), + number: pr.number, + }; let result = tokio::task::spawn_blocking(move || { super::resolve_host(kind).and_then(|h| h.fetch_status(&target)) }) @@ -174,7 +179,14 @@ async fn apply_probe_result( ) { let desired: Option<(NoticeKind, String)> = match &result { Ok(snapshot) => { - let readiness = judge::merge_readiness(&snapshot.ci, &snapshot.review, &snapshot.conflict); + // Ordering is resolved per sweep, not cached on the row: the + // upstream's lifecycle changes on ITS own schedule, and a stale + // copy here would either hold a mergeable PR back or release one + // early. `direction_id == 0` (a legacy row) resolves to Unknown, + // which is the honest answer for a PR whose task we cannot find. + let upstream = repo::upstream_merge_state(db, pr.direction_id).await; + let readiness = + judge::merge_readiness(&snapshot.ci, &snapshot.review, &snapshot.conflict, &upstream); let changed = snapshot_changed(pr, snapshot, &readiness); if let Err(e) = repo::apply_pull_request_snapshot(db, pr.id, snapshot, &readiness).await { eprintln!("[weft][host] pr #{}: could not save snapshot: {e}", pr.id); diff --git a/src-tauri/src/planner.rs b/src-tauri/src/planner.rs index e3f1e138..df90a390 100644 --- a/src-tauri/src/planner.rs +++ b/src-tauri/src/planner.rs @@ -100,6 +100,29 @@ pub struct ResolvedDirection { /// The direction id this lane was materialized into (0 = unset). Carried through from the /// stored proposal so the confirmed fast-path can re-dispatch by recorded id (no matching). pub direction_id: i32, + /// Optional: the `name` of ANOTHER direction in this SAME proposal that + /// this one must merge after (a producer→consumer ordering edge; issue + /// #110 T4). `""` = no upstream. Same extension-field mechanism as + /// `hint` (see `depends_on_from_value`) rather than a `ProposedDirection` + /// field — that struct is used by a large test/DB surface (R47-3-style + /// concern), so the extension lives in plan JSON instead. DISPLAY-facing + /// only — the human-readable name shown in `ScopeReview`/the Needs-you + /// card (`dependsOnLabel` on the frontend). The backend does not resolve + /// depends_on by re-matching this name; see `depends_on_index`. + pub depends_on: String, + /// The STABLE index into this SAME proposal's `directions` that `depends_on`'s name + /// resolved to — computed ONCE at proposal-save time (`resolve_depends_on_indices`), not + /// re-derived by name search later. `None` when `depends_on` is empty (no dependency) OR + /// when the name did not resolve to exactly one OTHER lane (self-reference, an ambiguous + /// duplicate, or not found) — `record_upstream_edges` tells those two `None` cases apart by + /// checking whether `depends_on` itself is empty. `#[serde(skip)]`: purely a backend + /// resolution detail, never sent to the frontend (which only ever needs the name, for + /// display) — replaces the OLD design where `record_upstream_edges` re-searched sibling + /// lanes by name on every approve/deny (Codex review, PR #159 planner.rs:1554 and + /// planner.rs:1557 — both root-caused in that re-search's decision-state/self-reference + /// blind spots; see the PR body for the full redesign rationale). + #[serde(skip)] + pub depends_on_index: Option, } /// Resolve one proposed direction's write-repo name to a workspace repo id. @@ -123,6 +146,11 @@ pub fn resolve(dir: &ProposedDirection, repos: &[(i32, String)]) -> ResolvedDire hint: crate::engine_routing::RoutingHint::default(), route: None, direction_id: dir.direction_id, + // Not carried by `ProposedDirection` (see that field's doc) — callers that + // need the real value overlay it from the raw JSON afterward, exactly as + // `resolved_from_plan` already does for `hint`. + depends_on: String::new(), + depends_on_index: None, } } @@ -298,8 +326,10 @@ pub async fn save_proposal(db: &Db, thread_id: i32, proposal: &Proposal) -> Resu /// Store a proposal received across an untrusted JSON boundary. Keeping this /// raw-value seam lets older typed callers remain source-compatible while the -/// optional planner `hint` survives round-trips through human edits and the -/// server-owned decision/id fields. Unknown hints are normalized to `normal`. +/// optional planner `hint` AND `depends_on` (issue #110 T4 ordering edge) +/// survive round-trips through human edits and the server-owned decision/id +/// fields. Unknown hints are normalized to `normal`; `depends_on` is a bare +/// name reference, resolved to an id later (see `confirm`). pub async fn save_proposal_value(db: &Db, thread_id: i32, input: &Value) -> Result<()> { let gate = thread_gate(thread_id); let _gate = gate.lock().await; @@ -315,6 +345,8 @@ pub async fn save_proposal_value(db: &Db, thread_id: i32, input: &Value) -> Resu } let mut value = serde_json::to_value(&p)?; normalize_input_hints(&mut value, input); + normalize_input_depends_on(&mut value, input); + resolve_depends_on_indices(&mut value); let json = serde_json::to_string(&value)?; // Bump the proposal VERSION on EVERY re-propose (R50-2). `upsert_plan` uses `version` as the // INSERT created_at but PRESERVES created_at on UPDATE; for a re-propose (existing row) the @@ -385,10 +417,178 @@ fn preserve_hints_from_baseline(value: &mut Value, baseline: Option<&Value>) { } } +/// Read the optional `depends_on` extension value (the `name` of ANOTHER +/// direction in the SAME proposal this one must merge after — issue #110 T4 +/// ordering edge) without making it part of the long-standing +/// `ProposedDirection` Rust struct. Same reasoning and mechanism as +/// `hint_from_value`: that struct is used by a large test/DB surface, so this +/// extension lives in plan JSON instead. `""` (absent / not a string) means +/// no upstream. +fn depends_on_from_value(value: &Value, index: usize) -> String { + value + .get("directions") + .and_then(Value::as_array) + .and_then(|directions| directions.get(index)) + .and_then(|direction| direction.get("depends_on")) + .and_then(Value::as_str) + .unwrap_or("") + .to_string() +} + +/// A fresh lead proposal owns its `depends_on` values — mirrors +/// `normalize_input_hints`: do not inherit a prior lane's value by index +/// position just because this proposal happens to be the same length. +fn normalize_input_depends_on(value: &mut Value, input: &Value) { + let Some(directions) = value.get_mut("directions").and_then(Value::as_array_mut) else { + return; + }; + let input_directions = input.get("directions").and_then(Value::as_array); + for (index, direction) in directions.iter_mut().enumerate() { + let Some(object) = direction.as_object_mut() else { + continue; + }; + let depends_on = input_directions + .and_then(|items| items.get(index)) + .and_then(|item| item.get("depends_on")) + .and_then(Value::as_str) + .unwrap_or(""); + object.insert("depends_on".to_string(), Value::String(depends_on.to_string())); + } +} + +/// Server-side approve/confirm updates deserialize the long-standing Proposal +/// type, which intentionally does not carry `depends_on` — mirrors +/// `preserve_hints_from_baseline`: preserve the stored value by index for +/// those updates only; a fresh lead proposal goes through +/// `normalize_input_depends_on` instead. +fn preserve_depends_on_from_baseline(value: &mut Value, baseline: Option<&Value>) { + let Some(directions) = value.get_mut("directions").and_then(Value::as_array_mut) else { + return; + }; + let baseline_directions = baseline.and_then(|value| value.get("directions")).and_then(Value::as_array); + for (index, direction) in directions.iter_mut().enumerate() { + let Some(object) = direction.as_object_mut() else { + continue; + }; + let depends_on = baseline_directions + .and_then(|items| items.get(index)) + .and_then(|item| item.get("depends_on")) + .and_then(Value::as_str) + .unwrap_or(""); + object.insert("depends_on".to_string(), Value::String(depends_on.to_string())); + } +} + +/// Read the `depends_on_index` extension value written by [`resolve_depends_on_indices`] — +/// `Some(i)` when `depends_on`'s name resolved to sibling index `i` in THIS SAME proposal, +/// `None` when it did not (which callers distinguish from "no `depends_on` at all" by checking +/// whether the name itself, from `depends_on_from_value`, is empty). +fn depends_on_index_from_value(value: &Value, index: usize) -> Option { + value + .get("directions") + .and_then(Value::as_array) + .and_then(|directions| directions.get(index)) + .and_then(|direction| direction.get("depends_on_index")) + .and_then(Value::as_u64) + .map(|i| i as usize) +} + +/// Resolve every lane's `depends_on` NAME to a STABLE INDEX into this SAME proposal's +/// `directions`, once, right here — not deferred to `record_upstream_edges`, which used to +/// re-search sibling lanes BY NAME on every single approve/deny (Codex review, PR #159: the +/// index-based redesign this function anchors; see the PR body for the full rationale). Called +/// from BOTH places a proposal's `depends_on` names get established: `save_proposal_value` +/// (after `normalize_input_depends_on`) for a fresh lead proposal, and `proposal_json_with_hints` +/// (after `preserve_depends_on_from_baseline`) for every server-driven re-save (approve/deny/ +/// settle) — recomputing fresh from whatever names are in `value` at that point is always +/// correct and cheap (a handful of string comparisons), since the RESULT only ever depends on +/// the CURRENT set of names, never on decision/materialization state (which is exactly the bug +/// the old name-search-at-write-time design had: `record_upstream_edges` used to filter +/// candidates by `direction_id != 0`, so a duplicate name could look like a false singleton +/// purely because one sibling hadn't materialized YET — Codex review, PR #159 planner.rs:1554). +/// +/// A name resolves to `Some(index)` only when it matches EXACTLY ONE OTHER lane's name (never +/// itself — Codex review, PR #159 planner.rs:1557: a self-reference used to resolve to a +/// singleton match of the lane's OWN materialized id, attempt a self-edge write, +/// `set_direction_upstream` would reject that write, and the rejection was invisible — +/// `depends_on_direction_id` just stayed at its prior value, `0`, reading as "no dependency" +/// instead of blocked). Anything else — empty match (typo / a name from outside this proposal, +/// which the tool contract already says never resolves), 2+ matches (a genuine ambiguous +/// duplicate name — this codebase explicitly supports duplicate same-name lanes elsewhere), or +/// a match on nothing but the lane's own index (self-reference) — resolves to `None`, and +/// `record_upstream_edges` maps that (given a non-empty `depends_on` name) to the SAME blocking +/// sentinel every other unresolvable case in this feature uses. Never silently free to merge. +fn resolve_depends_on_indices(value: &mut Value) { + let Some(directions) = value.get("directions").and_then(Value::as_array) else { + return; + }; + // Every lane's name and depends_on, in order — the ONLY inputs resolution needs. Collected + // as owned data up front so the write pass below can hold a fresh `&mut` without conflicting + // with these reads (this loop needs every OTHER lane's name to resolve any ONE lane's target). + let names: Vec = directions + .iter() + .map(|d| d.get("name").and_then(Value::as_str).unwrap_or("").trim().to_string()) + .collect(); + let depends_on: Vec = directions + .iter() + .map(|d| d.get("depends_on").and_then(Value::as_str).unwrap_or("").trim().to_string()) + .collect(); + let resolved: Vec> = depends_on + .iter() + .enumerate() + .map(|(own_index, name)| { + if name.is_empty() { + return None; // no dependency declared + } + let matches: Vec = names + .iter() + .enumerate() + .filter(|(i, n)| *i != own_index && n.as_str() == name.as_str()) + .map(|(i, _)| i) + .collect(); + match matches.as_slice() { + [i] => Some(*i), + [] => { + eprintln!( + "[weft][planner] lane {own_index} ({:?}): depends_on {name:?} matches no \ + OTHER lane in this proposal by name — blocking rather than guessing", + names.get(own_index).map(String::as_str).unwrap_or("") + ); + None + } + _ => { + eprintln!( + "[weft][planner] lane {own_index} ({:?}): depends_on {name:?} matches {} \ + other lanes in this proposal ambiguously — blocking rather than guessing", + names.get(own_index).map(String::as_str).unwrap_or(""), + matches.len() + ); + None + } + } + }) + .collect(); + let Some(directions) = value.get_mut("directions").and_then(Value::as_array_mut) else { + return; + }; + for (index, direction) in directions.iter_mut().enumerate() { + let Some(object) = direction.as_object_mut() else { + continue; + }; + let index_value = match resolved.get(index).copied().flatten() { + Some(i) => Value::Number(i.into()), + None => Value::Null, + }; + object.insert("depends_on_index".to_string(), index_value); + } +} + fn proposal_json_with_hints(proposal: &Proposal, baseline: &str) -> Result { let mut value = serde_json::to_value(proposal)?; let baseline = serde_json::from_str::(baseline).ok(); preserve_hints_from_baseline(&mut value, baseline.as_ref()); + preserve_depends_on_from_baseline(&mut value, baseline.as_ref()); + resolve_depends_on_indices(&mut value); Ok(serde_json::to_string(&value)?) } @@ -568,6 +768,8 @@ async fn resolved_from_plan( .collect(); for (index, direction) in directions.iter_mut().enumerate() { direction.hint = hint_from_value(&raw, index); + direction.depends_on = depends_on_from_value(&raw, index); + direction.depends_on_index = depends_on_index_from_value(&raw, index); } Ok(ResolvedProposal { thread_id, @@ -1007,6 +1209,31 @@ async fn confirm_with_manual_tool_with_session_liveness( recreated_fastpath.push(id); } } + // THE FIX (Codex review, PR #159 planner.rs:1549): `record_upstream_edges` runs AFTER + // the plan-confirmation CAS commits (see the main path below), as a separate, best- + // effort step — never inside the same transaction (see that function's own doc on why: + // it is metadata layered on top of already-durable state, not part of what makes any + // SINGLE decision atomic). If the process exits between that CAS committing and + // `record_upstream_edges` finishing — a hard crash, not just the in-process transient + // errors `set_upstream_edge_if_changed` now falls back safely from — a newly + // materialized consumer can be left with `depends_on_direction_id` still at its schema + // default `0`, reading as dependency-free forever: THIS fast path only re-dispatches by + // recorded id, it never repairs edges, so restart alone could never heal it before this + // fix. Closed by re-running the SAME resolution + write here, every time this fast path + // is taken (every redispatch, not only the one right after a crash) — cheap and safe + // because it is exactly as idempotent as the main path's own call: `resolved` (built + // above via `resolved_from_plan`) already carries each lane's save-time-resolved + // `depends_on_index`, so no new resolution logic is needed, and + // `set_upstream_edge_if_changed`'s own early return (`dir.depends_on_direction_id == + // target`) makes every already-correct edge a no-op — only a genuinely missing/stale one + // is actually rewritten. + let fastpath_proposal: Proposal = serde_json::from_str(&start_plan.proposal).unwrap_or_default(); + record_upstream_edges( + db, + thread_id, + &upstream_lanes_from_resolved(&resolved, &fastpath_proposal), + ) + .await; return Ok(matching); } let existing_dirs = repo::list_directions(db, thread_id).await?; @@ -1339,9 +1566,411 @@ async fn confirm_with_manual_tool_with_session_liveness( ) .await; } + record_upstream_edges( + db, + thread_id, + &upstream_lanes_from_resolved(&resolved, &proposal), + ) + .await; Ok(dispatch_ids) } +/// One lane's identity for resolving `depends_on` names to ids — the minimal +/// view [`record_upstream_edges`] needs, independent of which of the TWO +/// call sites that can transition a plan to "confirmed" supplies it: +/// `confirm`'s main body (which has a full `ResolvedProposal`) and +/// `auto_settle_if_fully_decided` (the per-lane-approval settle path, issue +/// #104's "plan approve → per-lane approve → batch confirm is the same +/// decision" — which only has the typed `Proposal` plus the just-persisted +/// raw JSON, never a `ResolvedProposal`). Owns its strings rather than +/// borrowing so both call sites can build it without lifetime gymnastics — +/// this runs at most once per confirm/settle, so the extra allocations are +/// immaterial. +/// How a lane's `depends_on` resolves, as already decided by [`resolve_depends_on_indices`] at +/// proposal-save time — `record_upstream_edges` never re-derives this by name. A discriminated +/// value (CLAUDE.md "discriminated state, exhaustive map"), not a name string plus a +/// separately-searched candidate list, so every caller maps it with one exhaustive `match` +/// instead of re-deriving "is this ambiguous / self-referential / just not decided yet" at each +/// use site. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum DependsOnTarget { + /// No `depends_on` was declared — this lane has no upstream. + None, + /// Resolved to exactly one OTHER lane in this SAME proposal, by its stable index. + Index(usize), + /// A `depends_on` name was declared but did not resolve to exactly one OTHER lane at + /// proposal-save time — empty match, ambiguous duplicate match, or a self-reference (Codex + /// review, PR #159 planner.rs:1554 / planner.rs:1557). Always blocking. + Unresolved, +} + +/// Combine a lane's raw `depends_on` name with its already-resolved `depends_on_index` into one +/// [`DependsOnTarget`] — the single place that decides "no dependency" (empty name) apart from +/// "declared but unresolved" (non-empty name, no index), shared by both `UpstreamLane` builders +/// below so they can't drift on this distinction. +fn depends_on_target_of(name: &str, index: Option) -> DependsOnTarget { + if name.trim().is_empty() { + DependsOnTarget::None + } else { + match index { + Some(i) => DependsOnTarget::Index(i), + None => DependsOnTarget::Unresolved, + } + } +} + +struct UpstreamLane { + direction_id: i32, + /// This lane's OWN decision ("" | "approved" | "denied") — needed to tell + /// "not decided / referenced by typo" apart from "explicitly denied" when + /// this lane is the TARGET of another lane's `depends_on` (Codex review, + /// PR #159 planner.rs:1534): a denied producer never materializes + /// (`direction_id` stays 0 forever, same as an undecided one), but it is + /// a permanent, decided fact — not a "maybe later" — and a consumer + /// naming it must be actively blocked, not silently left as "no upstream". + decision: String, + depends_on: DependsOnTarget, +} + +/// Build [`UpstreamLane`]s from a `ResolvedProposal` (confirm's main body, +/// which already resolved `depends_on`/`depends_on_index` via `resolved_from_plan`) zipped with +/// the `Proposal`'s per-lane materialized ids. Indices align 1:1 — both +/// derive from the same stored proposal snapshot (see `resolved_from_plan`'s +/// doc), which is also what makes a resolved `DependsOnTarget::Index` a valid index into THIS +/// SAME `lanes` vec. +fn upstream_lanes_from_resolved(resolved: &ResolvedProposal, proposal: &Proposal) -> Vec { + resolved + .directions + .iter() + .zip(proposal.directions.iter()) + .map(|(r, p)| UpstreamLane { + direction_id: p.direction_id, + decision: p.decision.clone(), + depends_on: depends_on_target_of(&r.depends_on, r.depends_on_index), + }) + .collect() +} + +/// Build [`UpstreamLane`]s from a typed `Proposal` (has `direction_id`/ +/// `decision` but never `depends_on`/`depends_on_index` — see those fields' docs) plus the raw +/// JSON `Value` of the SAME proposal. Used by `auto_settle_if_fully_decided`, which +/// persists via `proposal_json_with_hints` rather than `resolved_from_plan` +/// and so never builds a full `ResolvedProposal` (that additionally needs a +/// live workspace-repo lookup this settle path has no reason to pay for). +fn upstream_lanes_from_raw(proposal: &Proposal, raw: &Value) -> Vec { + proposal + .directions + .iter() + .enumerate() + .map(|(idx, p)| UpstreamLane { + direction_id: p.direction_id, + decision: p.decision.clone(), + depends_on: depends_on_target_of(&depends_on_from_value(raw, idx), depends_on_index_from_value(raw, idx)), + }) + .collect() +} + +/// Resolve every lane's `depends_on` (the `name` of ANOTHER direction in this +/// SAME proposal it must merge after — issue #110 T4) to that direction's +/// materialized id, and record the edge via `set_direction_upstream`. Called +/// from EVERY production place a lane's `depends_on` needs (re-)resolving: +/// after each INDIVIDUAL `approve_direction`/`deny_direction` decision (via +/// `resolve_and_record_upstream_edges`), and after `confirm`'s main body +/// materializes a batch. Previously this ran only once a proposal's LAST lane +/// was decided or once a whole batch materialized (Codex review, PR #159 +/// planner.rs:1420 — the per-lane settle path used to skip this entirely) — +/// but `approve_direction` DISPATCHES the lane it just approved on return, so +/// even THAT fix left a window: a consumer approved before its producer is +/// even decided got dispatched with `depends_on_direction_id` still `0` (Codex +/// review, PR #159 planner.rs:1776 — found on the round-3 push). Running this +/// after every individual decision, not only the settling one, closes that +/// window; see `UNRESOLVED_UPSTREAM_SENTINEL`'s doc for how a not-yet- +/// resolvable reference stays actively blocked (never silently `0`) until a +/// LATER call — triggered by the referenced lane's own decision — resolves it +/// for real. +/// +/// Runs AFTER its caller's own persist/CAS/materialize has committed: the +/// ordering edge is metadata layered on top of already-durable state, not +/// part of what makes any SINGLE decision atomic, so it is intentionally +/// best-effort here — see `notify_upstream_edge_write_failed` for why a write +/// failure escalates to a Needs-you notice instead of failing the causing +/// operation outright (Codex review, planner.rs:1470). +/// +/// Per lane: `DependsOnTarget::None` clears any STALE edge a previous proposal +/// left behind (Codex review, planner.rs:1447 — a re-proposal that drops +/// `depends_on` must not leave the old upstream blocking forever). +/// `DependsOnTarget::Index(i)` was ALREADY resolved unambiguously against sibling NAMES at +/// proposal-save time (`resolve_depends_on_indices`) — the only thing still dynamic here is +/// lane `i`'s CURRENT decision: not yet materialized (may still resolve on a later decision), +/// explicitly DENIED (Codex review, planner.rs:1534 — a permanent, decided fact, not a "maybe +/// later"), or materialized (its real id). `DependsOnTarget::Unresolved` (an empty match, an +/// AMBIGUOUS match — this codebase explicitly supports duplicate same-name lanes elsewhere — or +/// a self-reference, ALL decided once at save time, never re-derived here) is unconditionally +/// blocking. Every blocking case records `DENIED_UPSTREAM_SENTINEL` / `UNRESOLVED_UPSTREAM_ +/// SENTINEL` rather than leaving the edge at `0`, which would read as "no upstream, free to +/// merge." +/// +/// Resolves EVERY lane's target before writing any of them, then applies in two passes +/// (release-stale, then apply-real) rather than one straight-through pass (Codex review, PR +/// #159 planner.rs:1570): a re-proposal that REVERSES a reused dependency (A depended on B; B +/// should now depend on A instead) can list the lanes in either order, and a single pass that +/// happens to process the new dependent before the old one tries to write the reversed edge +/// while the stale original is still in the DB — `set_direction_upstream`'s cycle check +/// correctly (if unhelpfully, for a same-batch artifact) rejects that as a cycle, silently +/// stranding the new edge at `0`. See the two-pass loop's own comments for why clearing first +/// is unconditionally cycle-safe and stays a no-op for every lane that isn't actually changing. +async fn record_upstream_edges(db: &Db, thread_id: i32, lanes: &[UpstreamLane]) { + let mut targets: Vec<(i32, i32)> = Vec::with_capacity(lanes.len()); + for lane in lanes { + if lane.direction_id == 0 { + continue; // this lane never materialized — nothing to record an edge ON. + } + let target = match lane.depends_on { + DependsOnTarget::None => 0, + DependsOnTarget::Unresolved => repo::UNRESOLVED_UPSTREAM_SENTINEL, + DependsOnTarget::Index(i) => match lanes.get(i) { + // The named lane is a decided, permanent dead end — block, don't guess it might + // still work out (Codex review, planner.rs:1534). + Some(target_lane) if target_lane.decision == "denied" => repo::DENIED_UPSTREAM_SENTINEL, + // Materialized: its real id. + Some(target_lane) if target_lane.direction_id != 0 => target_lane.direction_id, + // Named lane exists (the index was resolved against THIS SAME proposal) but + // hasn't been decided/materialized yet — may still resolve once it is; block in + // the meantime, never silently 0. + Some(_) => repo::UNRESOLVED_UPSTREAM_SENTINEL, + // Defensive: `resolve_depends_on_indices` only ever produces an index into THIS + // SAME proposal's directions, so `lanes` (built 1:1 from that same snapshot — + // see `upstream_lanes_from_resolved`/`upstream_lanes_from_raw`'s docs) should + // never actually miss it. Fail closed if it somehow does anyway. + None => repo::UNRESOLVED_UPSTREAM_SENTINEL, + }, + }; + targets.push((lane.direction_id, target)); + } + + // PASS 1 — release every STALE edge that this call is about to REPLACE, before pass 2 + // writes any real (possibly non-zero) target. Without this, a re-proposal that REVERSES a + // reused dependency (A depended on B; the new shape has B depend on A instead) processed in + // the "wrong" lane order would try to write B→A while the old A→B edge is still in the DB: + // `set_direction_upstream`'s cycle walk correctly (if unhelpfully, for a same-batch + // artifact) sees that as a genuine cycle and rejects the write, silently leaving B's real + // new dependency stuck at 0 — allowing it to merge before A (Codex review, PR #159 + // planner.rs:1570). + // + // Clears to `UNRESOLVED_UPSTREAM_SENTINEL`, NOT `0` (Codex review, PR #159 + // planner.rs:1600, a follow-up round on this same fix): this function's two passes are two + // SEPARATE, non-transactional awaited writes, with no lock held across the gap between + // them — `host::automerge`/`host::monitor` run on their own independent sweep loop and can + // read this row at any point, including exactly between pass 1 and pass 2. A REPLACED + // dependency (a reused direction whose old real edge is swapped for a different real one, + // not merely cleared to empty) is the one case pass 1 actually writes something for; if + // that write were `0`, a sweep landing in the gap would read `UpstreamStatus::None` — "no + // upstream, free to merge" — genuinely true of the DB row at that instant, and could + // release a PR whose stored other-axis state was already cached `Ready`, before pass 2 has + // installed the real new producer. The sentinel closes that: `creates_cycle`'s walk + // dead-ends on it exactly like it does on `0` (a sentinel is never a real row's id — see + // `set_direction_upstream`'s doc), so it is EQUALLY safe against a false cycle rejection in + // pass 2, but a sweep reading it mid-transition sees `UpstreamStatus::Unknown` (blocking), + // never `None` (releasing) — the same fail-closed answer `UNRESOLVED_UPSTREAM_SENTINEL` + // already gives for "not resolved yet" everywhere else in this function. Only touching + // lanes whose target is ACTUALLY different from their current value (not every lane in the + // batch) keeps the common case — most decisions touch no `depends_on` at all, and most + // existing edges aren't changing — at zero extra writes, and avoids a lane that ISN'T + // changing ever transiently reading anything but its own already-correct value. + for &(direction_id, target) in &targets { + if let Ok(Some(dir)) = repo::get_direction(db, direction_id).await { + if dir.depends_on_direction_id != 0 && dir.depends_on_direction_id != target { + set_upstream_edge_if_changed(db, thread_id, direction_id, repo::UNRESOLVED_UPSTREAM_SENTINEL).await; + } + } + } + + // Test-only seam: let a test pause HERE — exactly the window a concurrently running + // automerge/monitor sweep could land in — read DB state from a separate task, then release + // it. See `tests::between_upstream_passes_probe`. + #[cfg(test)] + tests::between_upstream_passes_probe(thread_id).await; + + // PASS 2 — apply every real target now that no stale edge from this same batch can still + // be in the way of a legitimate reversal. + for (direction_id, target) in targets { + set_upstream_edge_if_changed(db, thread_id, direction_id, target).await; + } +} + +/// Write `target` as `direction_id`'s upstream edge — but only if it isn't +/// ALREADY `target`. `record_upstream_edges` now re-processes every lane on +/// every individual approve/deny (not only once, at the end — see its doc), +/// so the SAME still-unresolved lane can be visited many times before its +/// dependency actually resolves; reading first keeps the overwhelmingly +/// common case (a lane whose edge isn't changing THIS call — including every +/// plan that never uses `depends_on` at all) to one read and zero writes, +/// rather than a redundant UPDATE on every single decision in the proposal. +async fn set_upstream_edge_if_changed(db: &Db, thread_id: i32, direction_id: i32, target: i32) { + match repo::get_direction(db, direction_id).await { + Ok(Some(dir)) if dir.depends_on_direction_id == target => return, + Ok(Some(_)) => {} + Ok(None) => return, // the direction is gone — nothing to write. + Err(e) => { + // THE FIX (Codex review, PR #159 planner.rs:1797): a read failure here is JUST AS + // REAL a reason a stale/missing edge might survive undetected as the write-failure + // branch below, and until this fix it was treated differently — logged, but never + // falling back to the sentinel. That asymmetry mattered concretely: a direction that + // has NEVER had a successful upstream write (its column still at the schema default + // `0`) reads as "no dependency" exactly like a genuinely dependency-free lane, and + // this function's caller (`approve_direction`/`confirm`) does not gate its own + // success on this call — see this function's own doc: "intentionally best-effort," + // never a hard failure of the causing operation — so the newly materialized + // consumer is dispatched, and a later healthy auto-merge read sees `None` and can + // merge it before its producer. `set_direction_upstream` does its OWN read + // internally before writing (see that function), so a transient failure in THIS + // read does not imply the fallback write below will also fail. + eprintln!( + "[weft][planner] direction {direction_id}: could not read current state before \ + recording upstream {target}: {e}" + ); + notify_upstream_edge_write_failed(thread_id, direction_id, target, &e); + fall_back_to_unresolved_sentinel(db, direction_id, "the read failure above").await; + return; + } + } + if let Err(e) = repo::set_direction_upstream(db, direction_id, target).await { + eprintln!("[weft][planner] direction {direction_id}: could not record upstream {target}: {e}"); + notify_upstream_edge_write_failed(thread_id, direction_id, target, &e); + // A REJECTED/FAILED write must never leave the edge at whatever it happened to be + // BEFORE this call — if that prior value is `0` ("no dependency"), the direction would + // read as free to merge despite a prerequisite that was just rejected, not satisfied + // (Codex review, PR #159 planner.rs:1772: a mutual 2-cycle's SECOND edge is correctly + // rejected by `set_direction_upstream`'s cycle check — self-reference can no longer + // reach this point at all, excluded at resolve time, but a LONGER cycle only shows up + // graph-globally and still reaches this write-time check — and that rejection was + // silently swallowed here, leaving the rejected lane looking dependency-free). Falls + // back to the SAME blocking sentinel every other unresolvable case in this feature + // already uses (`UNRESOLVED_UPSTREAM_SENTINEL` — reused, not a new one): writing a + // sentinel can never itself be rejected as a cycle (it is never a real row's id — see + // `set_direction_upstream`'s doc), so this fallback write is unconditionally safe to + // attempt regardless of what the original `target` was. + fall_back_to_unresolved_sentinel(db, direction_id, "the rejected write above").await; + } +} + +/// Shared tail of both `set_upstream_edge_if_changed` failure branches (a rejected write, or a +/// read that failed before a write was even attempted): write the blocking sentinel so the edge +/// is never left at whatever value it happened to have before this call — see the two call +/// sites' own doc for why `0` surviving either failure is unsafe. `context` names which prior +/// failure this is recovering from, for the log line only; the fallback write itself is +/// unconditionally safe to attempt regardless of what failed above (`UNRESOLVED_UPSTREAM_ +/// SENTINEL` is never a real row's id, so `set_direction_upstream`'s cycle check can never +/// reject it — see that function's doc). +async fn fall_back_to_unresolved_sentinel(db: &Db, direction_id: i32, context: &str) { + if let Err(e2) = repo::set_direction_upstream(db, direction_id, repo::UNRESOLVED_UPSTREAM_SENTINEL).await { + eprintln!( + "[weft][planner] direction {direction_id}: ALSO could not fall back to the \ + blocking sentinel after {context}: {e2} — this direction's upstream edge may \ + still read as a stale, possibly-\"no dependency\" value" + ); + } +} + +/// Human-facing text for a failed `set_direction_upstream` write (Codex +/// review, PR #159 planner.rs:1470). By the time this runs, the plan is +/// ALREADY durably confirmed and its directions already dispatched — an +/// eprintln alone would be the ONLY signal that the ordering safety edge +/// silently did not land, invisible to anyone not tailing the process log. +/// Kept as a PURE function (unit-tested without a Tauri runtime) mirroring +/// `host::judge::give_up_text`'s split between content and the thin, +/// runtime-dependent call site that posts it — see +/// `notify_upstream_edge_write_failed`. `upstream_id == 0` means this was a +/// stale-edge CLEAR that failed, not a fresh write. +fn upstream_edge_write_failed_text(direction_id: i32, upstream_id: i32, err: &anyhow::Error) -> String { + // The recovery instruction deliberately does NOT say "just confirm/approve again": the + // confirmed-fast-path retry never re-calls record_upstream_edges (only the transition INTO + // "confirmed" does), so a bare retry cannot rescue this edge — only a fresh re-propose + // (which resets status back to "proposed") re-earns a pass through it. + if upstream_id == 0 { + format!( + "⚠️ 任务 #{direction_id} 清除过期的上游依赖失败:{err}。自动合并的排序保护可能仍卡在一个已经不存在的依赖上——单纯重新确认/批准不会重试这次写入,请让 agent 针对这个任务重新完整提一次案(re-propose)。" + ) + } else { + format!( + "⚠️ 任务 #{direction_id} 的上游依赖(#{upstream_id})记录失败:{err}。跨仓合并排序保护可能未生效,这个任务有可能在它依赖的任务之前被自动合并——单纯重新确认/批准不会重试这次写入,请让 agent 针对这个任务重新完整提一次案(re-propose)。" + ) + } +} + +/// Best-effort Needs-you escalation for a failed upstream-edge write. +/// +/// Deliberately NOT made transactional with (or a hard failure of) the +/// confirm/settle call that reached this point, despite Codex's review +/// suggesting exactly that (planner.rs:1470): by the time `record_upstream_ +/// edges` runs, the plan's directions are ALREADY durably created (materialize_ +/// direction already ran) and this call's own return is what the caller uses +/// to dispatch workers against them — turning THIS failure into an `Err` this +/// late would make `confirm`/`approve_direction` report failure for work that +/// in fact succeeded. And a "just retry" recovery does not actually help: the +/// confirmed-fast-path retry re-dispatches directions but never re-calls +/// `record_upstream_edges` (that only runs on the transition INTO "confirmed", +/// not on every re-entry), so neither path can rescue this specific edge — +/// only a fresh `save_proposal`/re-propose can, since that resets status back +/// to "proposed" and re-earns a pass through this function. A visible-but- +/// non-blocking notice is the correct shape for a write that is supplementary +/// safety metadata layered on top of already-real work, not a coin flip on +/// whether that work happened at all. +/// +/// Uses the same `crate::APP_HANDLE` global fallback `bus::server:: +/// emit_proposal_row` also reaches for from deep inside planner/bus logic +/// that has no `BusRegistry` of its own to thread through — but NOT an exact +/// mirror of that function's shape, and weaker: `emit_proposal_row` durably +/// writes to `lead_message` FIRST and only gates the LIVE push notification on +/// `APP_HANDLE`, so a restart before the live push is seen still leaves the +/// row for the human to find. This function has no durable counterpart to +/// fall back on — `BusRegistry`'s asks/notices are in-memory only (this +/// codebase has no ask/notice DB table at all), so a restart between the +/// write failure and a human seeing the notice loses BOTH silently, quietly +/// reverting to the original "nobody can tell" problem with a narrower (but +/// nonzero) window. This is an EXISTING tradeoff already made elsewhere in +/// this codebase (issue #88's stall notice is the same shape: memory-only, +/// no reboot re-scan), not a regression unique to this function — but it is a +/// real limitation, not a fully-closed gap. +/// +/// `APP_HANDLE` itself is not the weak link here: it is set once, synchronously, +/// inside `.setup()`, and `confirm`/`approve_direction` are reachable only +/// through Tauri IPC, which cannot fire before `.setup()` completes — so in +/// every real (non-test) run this branch is live. The `None` branches below +/// exist for the headless test harness, which is also why the notice TEXT is +/// unit-tested separately above rather than through this call site. +/// +/// FOLLOW-UP (Codex review, PR #159 planner.rs:1905), deliberately not done here: this +/// notice is never retracted if a LATER re-propose successfully rewrites the same +/// direction's edge — a human can be left staring at a stale warning after the underlying +/// problem is already fixed. `BusRegistry::cancel_open_asks_by_id` (used by `host::monitor` +/// for exactly this "supersede an earlier notice" purpose) could retract it, but +/// `host::monitor`'s use of that primitive leans on a `HashMap` that +/// its OWN long-lived sweep loop owns and threads as `&mut` across every iteration (see +/// `host::monitor`'s `notices` parameter) — this call site has no equivalent: it is reached +/// from one-shot confirm/re-propose IPC handlers, not a loop, with no natural owner for +/// mutable state that must survive between two calls that can be minutes or hours apart. +/// Wiring retraction in here would mean introducing a NEW global correlation map +/// (direction_id -> last-posted ask id) purely to track that, plus its own tests (posted/ +/// retracted/no-cross-direction-cancel/no-double-post) — real but non-trivial new machinery +/// for a UX polish concern, not a correctness one (the sentinel this notice warns about +/// already blocks a free merge regardless of whether the notice itself is ever retracted). +/// Left as a follow-up rather than bundled into this review response. +fn notify_upstream_edge_write_failed(thread_id: i32, direction_id: i32, upstream_id: i32, err: &anyhow::Error) { + use tauri::Manager; + let Some(app) = crate::APP_HANDLE.get() else { + return; + }; + let Some(bus) = app.try_state::() else { + return; + }; + bus.notify_human_action_required( + thread_id, + &direction_id.to_string(), + &upstream_edge_write_failed_text(direction_id, upstream_id, err), + ); +} + /// Tear down lanes created in the current confirm attempt (used on any failure to keep /// confirm atomic). Best-effort per lane — errors are ignored so the original failure /// is the one returned. @@ -1384,6 +2013,15 @@ fn fully_decided(p: &Proposal) -> bool { /// thread_gate hold), so this CAS is expected to always apply in production — kept /// defensive (like `confirm`'s final commit) rather than propagated as an error, because the /// caller's OWN action (the approve/deny that got us here) already succeeded regardless. +/// +/// THIS is the other of the two places a plan can transition to "confirmed" (see +/// `record_upstream_edges`'s doc for the exhaustive pair) — a plan resolved entirely through +/// per-lane Needs-you cards never reaches `confirm`'s main body at all, and a LATER batch +/// `confirm()` call on an already-"confirmed" plan exits through its idempotent fast path +/// before reaching it either. Before this fix (Codex review, PR #159 planner.rs:1420) that +/// meant `record_upstream_edges` never ran for this — the overwhelmingly common — approval +/// flow, so `depends_on` reproduced the exact bug it was written to close for anyone who +/// approves cards one at a time instead of clicking the batch Confirm button. async fn auto_settle_if_fully_decided( db: &Db, thread_id: i32, @@ -1393,9 +2031,36 @@ async fn auto_settle_if_fully_decided( if !fully_decided(proposal) { return; } - if let Ok(json) = proposal_json_with_hints(proposal, baseline) { - let _ = repo::mark_plan_confirmed_cas(db, thread_id, &json, "proposed").await; - } + let Ok(json) = proposal_json_with_hints(proposal, baseline) else { + return; + }; + let _ = repo::mark_plan_confirmed_cas(db, thread_id, &json, "proposed").await; + // Upstream edges are NOT resolved here (unlike before Codex review, PR #159 + // planner.rs:1776 found the gap this closed): `approve_direction`/`deny_direction` + // now call `resolve_and_record_upstream_edges` themselves, right after every + // INDIVIDUAL decision — including ones that don't (yet) fully decide the + // proposal — because `approve_direction` dispatches the lane it just approved + // immediately on return, before this function would otherwise ever run. By the + // time this function is reached, the caller has already resolved edges for the + // exact `proposal` state this CAS just confirmed. +} + +/// Resolve and record every lane's upstream edge for the CURRENT `proposal` +/// state (see `record_upstream_edges`'s doc). Called after EVERY individual +/// `approve_direction`/`deny_direction` decision — not only once a proposal +/// is fully decided — because a lane can be dispatched (by the caller, on +/// return) the moment ITS OWN decision persists, regardless of whether +/// siblings are still pending (Codex review, PR #159 planner.rs:1776). +/// `baseline` is the plan row's proposal JSON as read at the START of the +/// caller's operation — the same value `persist_decision`/`proposal_json_ +/// with_hints` already use to preserve extension fields through the typed +/// `Proposal` round-trip. +async fn resolve_and_record_upstream_edges(db: &Db, thread_id: i32, proposal: &Proposal, baseline: &str) { + let Ok(json) = proposal_json_with_hints(proposal, baseline) else { + return; + }; + let raw = serde_json::from_str::(&json).unwrap_or(Value::Null); + record_upstream_edges(db, thread_id, &upstream_lanes_from_raw(proposal, &raw)).await; } /// Restore the disk-reclaim state when a reused-lane approval fails after a @@ -1780,6 +2445,10 @@ async fn approve_direction_with_pin_with_session_liveness( ) .await; } + // Resolve/record BEFORE returning: the caller dispatches this lane the moment + // this call returns, regardless of whether siblings are still pending (Codex + // review, PR #159 planner.rs:1776). + resolve_and_record_upstream_edges(db, thread_id, &proposal, &plan.proposal).await; auto_settle_if_fully_decided(db, thread_id, &proposal, &plan.proposal).await; return Ok(id); } @@ -1846,6 +2515,9 @@ async fn approve_direction_with_pin_with_session_liveness( &route, ) .await; + // Resolve/record BEFORE returning — see the reused-lane branch above for why (Codex + // review, PR #159 planner.rs:1776). + resolve_and_record_upstream_edges(db, thread_id, &proposal, &plan.proposal).await; auto_settle_if_fully_decided(db, thread_id, &proposal, &plan.proposal).await; Ok(dir.id) } @@ -1868,6 +2540,10 @@ pub async fn deny_direction(db: &Db, thread_id: i32, index: usize) -> Result<(St pd.decision = "denied".to_string(); let info = (pd.name.clone(), pd.repo.clone()); persist_decision(db, thread_id, &proposal, &plan).await?; + // A deny can be the event that makes ANOTHER lane's depends_on resolve to the + // deny-sentinel (Codex review, PR #159 planner.rs:1534) — must run regardless of + // whether this is the proposal's last undecided lane. + resolve_and_record_upstream_edges(db, thread_id, &proposal, &plan.proposal).await; auto_settle_if_fully_decided(db, thread_id, &proposal, &plan.proposal).await; Ok(info) } @@ -1977,6 +2653,13 @@ pub struct PendingWrite { pub base_branch: String, pub hint: crate::engine_routing::RoutingHint, pub route: Option, + /// The `name` of ANOTHER direction in this SAME proposal this one must merge after (issue + /// #110 T4); `""` = no upstream. Carried through so the per-lane Needs-you card can show + /// the human WHICH producer gates this consumer at the moment they approve it — before + /// this field existed, the edge that decides whether the consumer's merge stays blocked + /// was invisible at exactly the point the human made the decision (Codex review, PR #159 + /// planner.rs:109). + pub depends_on: String, } /// The pending write declarations for a thread (known repo + undecided). @@ -2018,6 +2701,7 @@ async fn pending_writes_with_session_liveness( base_branch: d.base_branch.clone(), hint: d.hint, route: d.route.clone(), + depends_on: d.depends_on.clone(), }); } } @@ -2095,6 +2779,49 @@ mod tests { .insert(thread_id, (new_proposal_json.to_string(), new_status.to_string())); } + /// Test-only seam (mirrors `approve_persist_gate`/`confirm_cas_gate`): lets a test PAUSE + /// `record_upstream_edges` exactly between its two passes — the same window a concurrently + /// running automerge/monitor sweep could land in (Codex review, PR #159 planner.rs:1600) — + /// read DB state from a SEPARATE task while paused, then release it. `arm_between_ + /// upstream_passes_probe(thread_id)` returns `(reached_rx, resume_tx)`: `reached_rx` + /// resolves once the probe is hit, and sending on `resume_tx` lets `record_upstream_edges` + /// continue into pass 2. A no-op (never blocks) for any thread_id that wasn't armed — every + /// other test that reaches this function is unaffected. + #[allow(clippy::type_complexity)] + fn between_upstream_passes_map() -> &'static std::sync::Mutex< + std::collections::HashMap, tokio::sync::oneshot::Receiver<()>)>, + > { + static M: std::sync::OnceLock< + std::sync::Mutex< + std::collections::HashMap, tokio::sync::oneshot::Receiver<()>)>, + >, + > = std::sync::OnceLock::new(); + M.get_or_init(|| std::sync::Mutex::new(std::collections::HashMap::new())) + } + + fn arm_between_upstream_passes_probe( + thread_id: i32, + ) -> (tokio::sync::oneshot::Receiver<()>, tokio::sync::oneshot::Sender<()>) { + let (reached_tx, reached_rx) = tokio::sync::oneshot::channel(); + let (resume_tx, resume_rx) = tokio::sync::oneshot::channel(); + between_upstream_passes_map() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .insert(thread_id, (reached_tx, resume_rx)); + (reached_rx, resume_tx) + } + + pub(super) async fn between_upstream_passes_probe(thread_id: i32) { + let armed = between_upstream_passes_map() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .remove(&thread_id); + if let Some((reached_tx, resume_rx)) = armed { + let _ = reached_tx.send(()); + let _ = resume_rx.await; + } + } + /// Fired by `confirm` (test build only) just before its CAS. If a race is armed for /// `thread_id`, apply it ONCE (replacing the stored plan) and disarm. pub(super) async fn confirm_cas_gate(db: &Db, thread_id: i32) { @@ -2172,6 +2899,38 @@ mod tests { ); } + /// The one PURELY-testable piece of `notify_upstream_edge_write_failed` (Codex review, PR + /// #159 planner.rs:1470): the notice text itself, unit-tested without a Tauri runtime — + /// mirrors `host::judge::give_up_text`'s split between content and its thin, untestable + /// call site. Must name the direction so a human knows WHICH task's ordering safety edge + /// is in question, and must name the recovery action (re-approve/re-confirm) since nothing + /// else in the system retries this write on its own. + #[test] + fn upstream_edge_write_failed_text_names_the_direction_and_the_recovery_path() { + let err = anyhow::anyhow!("database is locked"); + let text = upstream_edge_write_failed_text(42, 7, &err); + assert!(text.contains("42"), "must name the direction whose edge failed to write: {text}"); + assert!(text.contains("7"), "must name the upstream it was trying to point at: {text}"); + assert!(text.contains("database is locked"), "must keep the underlying error: {text}"); + assert!( + text.contains("审批") || text.contains("确认"), + "must name the recovery path (re-approve/re-confirm) — nothing else retries this \ + write automatically: {text}" + ); + } + + /// The `upstream_id == 0` case (a STALE-edge CLEAR failed, not a fresh write) must read + /// distinctly — conflating the two would tell a human to look for a wrong-upstream problem + /// when the actual issue is a leftover one that failed to clear. + #[test] + fn upstream_edge_write_failed_text_distinguishes_a_failed_clear_from_a_failed_write() { + let err = anyhow::anyhow!("database is locked"); + let clear_text = upstream_edge_write_failed_text(42, 0, &err); + let write_text = upstream_edge_write_failed_text(42, 7, &err); + assert_ne!(clear_text, write_text, "a failed clear and a failed write must not read identically"); + assert!(clear_text.contains("42"), "must still name the direction: {clear_text}"); + } + #[tokio::test] async fn fresh_reproposal_omitting_hints_defaults_each_lane_to_normal() { let db = Db::connect("sqlite::memory:").await.unwrap(); @@ -4896,6 +5655,54 @@ mod tests { let _ = std::fs::remove_dir_all(&weft_home); } + /// THE FIX (Codex review, PR #159 planner.rs:109): `depends_on` must flow all the way + /// through to `PendingWrite` — the per-lane Needs-you card's data source (via + /// `commands::WriteTrigger`) — not only into the batch `ScopeReview` dialog's + /// `ResolvedDirection`. Before this field existed on `PendingWrite`, the human approving a + /// single lane through the Needs-you card could not see which producer gates it, even + /// though the edge was already decided in the data. Mirrors + /// `pending_writes_carry_base_branch`'s pattern but exercises the RAW-JSON `depends_on` + /// path (`save_proposal_value`, not the typed `Proposal`/`ProposedDirection` — see that + /// field's doc on `ResolvedDirection`). + #[tokio::test] + async fn pending_writes_carry_depends_on() { + let _env = crate::paths::ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let tag = format!("weft-pwdeps-{}", std::process::id()); + let root = std::env::temp_dir().join(format!("{tag}-root")); + let weft_home = std::env::temp_dir().join(format!("{tag}-home")); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + std::env::set_var("WEFT_HOME", weft_home.to_str().unwrap()); + let repo_path = make_repo(&root, "api"); + + let db = Db::connect("sqlite::memory:").await.unwrap(); + let ws = repo::create_workspace(&db, "ws").await.unwrap(); + let _ra = repo::add_repo_ref(&db, ws.id, "api", repo_path.to_str().unwrap(), "main", "", true) + .await + .unwrap(); + let t = repo::create_thread(&db, ws.id, "t1", "feature", "claude").await.unwrap(); + let raw = serde_json::json!({ + "rationale": "r", + "directions": [ + {"name": "producer", "repo": "api", "reason": "r", "mandate": "impl-only"}, + {"name": "consumer", "repo": "api", "reason": "r", "mandate": "impl-only", "depends_on": "producer"}, + ] + }); + save_proposal_value(&db, t.id, &raw).await.unwrap(); + let pending = pending_writes(&db, t.id).await.unwrap(); + assert_eq!(pending.len(), 2); + assert_eq!(pending[0].depends_on, "", "producer names no upstream"); + assert_eq!( + pending[1].depends_on, "producer", + "depends_on must flow from the proposal into PendingWrite, not stay stuck at \ + ResolvedDirection — this is what the per-lane Needs-you card renders" + ); + + std::env::remove_var("WEFT_HOME"); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + } + #[tokio::test] async fn confirm_does_not_roll_back_a_reused_lane_when_a_later_lane_fails() { // Lane A: approved via Needs-you (pre-existing, materialized). Lane B: a NEW @@ -5925,6 +6732,7 @@ mod tests { base_branch: "HEAD".to_string(), target_branch: "".to_string(), branch: "feat/a".to_string(), + depends_on_direction_id: 0, created_at: "0".to_string(), }; @@ -5967,6 +6775,7 @@ mod tests { base_branch: base.to_string(), target_branch: "".to_string(), branch: "feat/a".to_string(), + depends_on_direction_id: 0, created_at: "0".to_string(), }; // Non-empty bases are normalized purely by string prefix-stripping — no repo @@ -6029,6 +6838,7 @@ mod tests { base_branch: base.to_string(), target_branch: target.to_string(), branch: "feat/a".to_string(), + depends_on_direction_id: 0, created_at: "0".to_string(), }; @@ -6119,6 +6929,999 @@ mod tests { let _ = std::fs::remove_dir_all(&weft_home); } + /// THE WIRING (issue #110 T4, Codex review on PR #159): before this test existed, + /// `repo::set_direction_upstream` was called ONLY by `repo.rs`'s own unit tests — no + /// production path (planner or command) ever wrote `depends_on_direction_id`, so + /// `upstream_merge_state` returned `None` for every production-created task and the + /// auto-merge gate could still merge a consumer before its producer. This drives the + /// REAL production entry point (`confirm`, exactly what a human's "confirm" click and + /// the lead's `propose_directions` tool ultimately reach) with a producer/consumer pair + /// proposed in ONE call, the consumer naming the producer by `name` via `depends_on` — + /// then asserts the edge landed on the REAL direction rows, not a parallel copy. + #[tokio::test] + async fn confirm_records_the_upstream_edge_from_depends_on() { + let _env = crate::paths::ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let tag = format!("weft-confirm-upstream-edge-{}", std::process::id()); + let root = std::env::temp_dir().join(format!("{tag}-root")); + let weft_home = std::env::temp_dir().join(format!("{tag}-home")); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + std::env::set_var("WEFT_HOME", weft_home.to_str().unwrap()); + let _repo_path = make_repo(&root, "api"); + let db = Db::connect("sqlite::memory:").await.unwrap(); + let ws = repo::create_workspace(&db, "ws").await.unwrap(); + let _ra = repo::add_repo_ref(&db, ws.id, "api", root.join("api").to_str().unwrap(), "main", "", true) + .await + .unwrap(); + let t = repo::create_thread(&db, ws.id, "t1", "feature", "claude").await.unwrap(); + + // `depends_on` is a JSON extension field (not on the typed `ProposedDirection` — see + // its doc), so it is exercised through the SAME raw-JSON boundary a real + // `propose_directions` tool call crosses (`save_proposal_value`), not the typed struct. + let raw = serde_json::json!({ + "rationale": "cross-repo change set", + "directions": [ + {"name": "producer", "repo": "api", "reason": "r", "mandate": "impl-only"}, + {"name": "consumer", "repo": "api", "reason": "r", "mandate": "impl-only", "depends_on": "producer"}, + ] + }); + save_proposal_value(&db, t.id, &raw).await.unwrap(); + let ids = confirm(&db, t.id).await.unwrap(); + assert_eq!(ids.len(), 2, "both known-repo lanes are dispatched"); + + let producer_dir = repo::get_direction(&db, ids[0]).await.unwrap().unwrap(); + let consumer_dir = repo::get_direction(&db, ids[1]).await.unwrap().unwrap(); + assert_eq!(producer_dir.name, "producer"); + assert_eq!(consumer_dir.name, "consumer"); + assert_eq!( + consumer_dir.depends_on_direction_id, producer_dir.id, + "confirm must resolve depends_on:\"producer\" to the producer's REAL materialized \ + id and persist it on the consumer's OWN direction row — this is the write no \ + production path used to reach" + ); + assert_eq!(producer_dir.depends_on_direction_id, 0, "the producer itself has no upstream"); + + // The axis this edge exists to feed: with no PR registered yet, the consumer must read + // as genuinely blocked — proving the wired edge actually reaches merge_readiness's + // fourth axis end to end, not just the DB column. + assert!( + matches!( + repo::upstream_merge_state(&db, consumer_dir.id).await, + crate::host::UpstreamStatus::Pending { .. } + ), + "the wired edge must gate upstream_merge_state, which feeds judge::merge_readiness" + ); + + let removed = repo::delete_thread_cascade(&db, t.id).await.unwrap(); + let _ = materialize::cleanup_worktrees(&db, &removed).await; + std::env::remove_var("WEFT_HOME"); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + } + + /// THE DECISIVE GAP (Codex review, PR #159 planner.rs:1420, found on the round-2 + /// adversarial re-review of the fix above): `confirm_records_the_upstream_edge_from_ + /// depends_on` only proved the BATCH-confirm path writes the edge. But a proposal can + /// ALSO be settled entirely through the per-lane Needs-you cards (`approve_direction` / + /// `deny_direction`) — issue #104's "plan approve → per-lane approve → batch confirm is + /// the same decision" — which marks the plan confirmed via `auto_settle_if_fully_decided` + /// and NEVER reaches `confirm`'s main body at all. This drives THAT exact flow: both + /// lanes approved one at a time, never calling `confirm`, and proves the edge still + /// lands — then confirms a LATER `confirm()` call (idempotent fast path) doesn't disturb + /// it either. + #[tokio::test] + async fn approve_direction_records_the_upstream_edge_when_settled_lane_by_lane() { + let _env = crate::paths::ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let tag = format!("weft-approve-upstream-edge-{}", std::process::id()); + let root = std::env::temp_dir().join(format!("{tag}-root")); + let weft_home = std::env::temp_dir().join(format!("{tag}-home")); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + std::env::set_var("WEFT_HOME", weft_home.to_str().unwrap()); + let _repo_path = make_repo(&root, "api"); + let db = Db::connect("sqlite::memory:").await.unwrap(); + let ws = repo::create_workspace(&db, "ws").await.unwrap(); + let _ra = repo::add_repo_ref(&db, ws.id, "api", root.join("api").to_str().unwrap(), "main", "", true) + .await + .unwrap(); + let t = repo::create_thread(&db, ws.id, "t1", "feature", "claude").await.unwrap(); + + let raw = serde_json::json!({ + "rationale": "cross-repo change set", + "directions": [ + {"name": "producer", "repo": "api", "reason": "r", "mandate": "impl-only"}, + {"name": "consumer", "repo": "api", "reason": "r", "mandate": "impl-only", "depends_on": "producer"}, + ] + }); + save_proposal_value(&db, t.id, &raw).await.unwrap(); + + // Approve EACH lane individually via the Needs-you per-card path — `confirm`'s main + // body is NEVER called in this test. + let producer_id = approve_direction(&db, t.id, 0, "claude").await.unwrap(); + let consumer_id = approve_direction(&db, t.id, 1, "claude").await.unwrap(); + + // The second approve completed the LAST decision, so auto_settle_if_fully_decided + // must have flipped the plan to "confirmed" on its own. + let plan = repo::get_plan(&db, t.id).await.unwrap().unwrap(); + assert_eq!(plan.status, "confirmed", "the last per-lane approval settles the plan"); + + let producer_dir = repo::get_direction(&db, producer_id).await.unwrap().unwrap(); + let consumer_dir = repo::get_direction(&db, consumer_id).await.unwrap().unwrap(); + assert_eq!( + consumer_dir.depends_on_direction_id, producer_dir.id, + "per-lane approval must ALSO record the upstream edge — this is the exact bug the \ + batch-confirm-only wiring reproduced for anyone using the supported card-by-card flow" + ); + assert!( + matches!( + repo::upstream_merge_state(&db, consumer_dir.id).await, + crate::host::UpstreamStatus::Pending { .. } + ), + "the edge recorded via per-lane approval must gate upstream_merge_state too" + ); + + // A later confirm() (a human also clicking batch Confirm, or a redispatch retry) hits + // the idempotent fast path on an already-"confirmed" plan. Both lanes were already + // individually approved (not left pending), so the fast path — which only + // re-dispatches PENDING lanes, see its own doc — has nothing new to hand back; the + // point of this call is only that it must not ERROR and must not disturb the edge + // already recorded. + let redispatch = confirm(&db, t.id).await.unwrap(); + assert!(redispatch.is_empty(), "both lanes were already individually approved, not pending"); + let consumer_dir_after = repo::get_direction(&db, consumer_id).await.unwrap().unwrap(); + assert_eq!( + consumer_dir_after.depends_on_direction_id, producer_dir.id, + "a later confirm() on an already-confirmed plan must not disturb the recorded edge" + ); + + let removed = repo::delete_thread_cascade(&db, t.id).await.unwrap(); + let _ = materialize::cleanup_worktrees(&db, &removed).await; + std::env::remove_var("WEFT_HOME"); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + } + + /// THE DECISIVE GAP, ROUND 2 (Codex review, PR #159 planner.rs:1776, found on the + /// round-3 adversarial re-review of the fix above): that test approved producer THEN + /// consumer — the ORDER a human is likely to follow, but not the only one the per-lane + /// Needs-you flow allows. `approve_direction` DISPATCHES the lane it just approved + /// immediately on return (the caller starts a worker against the returned id), so if the + /// CONSUMER is approved BEFORE the producer is even decided, the consumer was handed back + /// to its caller — worker potentially already starting — while `record_upstream_edges` + /// had never run for it at all (it only ran once the WHOLE proposal was fully decided). + /// This drives EXACTLY that order and asserts the edge is ALREADY blocking (not `0`) the + /// instant `approve_direction` returns for the consumer — before the producer is decided + /// at all — then that approving the producer afterward self-heals it to the real id. + #[tokio::test] + async fn approving_the_consumer_before_its_producer_blocks_immediately_not_after_settling() { + let _env = crate::paths::ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let tag = format!("weft-upstream-consumer-first-{}", std::process::id()); + let root = std::env::temp_dir().join(format!("{tag}-root")); + let weft_home = std::env::temp_dir().join(format!("{tag}-home")); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + std::env::set_var("WEFT_HOME", weft_home.to_str().unwrap()); + let _repo_path = make_repo(&root, "api"); + let db = Db::connect("sqlite::memory:").await.unwrap(); + let ws = repo::create_workspace(&db, "ws").await.unwrap(); + let _ra = repo::add_repo_ref(&db, ws.id, "api", root.join("api").to_str().unwrap(), "main", "", true) + .await + .unwrap(); + let t = repo::create_thread(&db, ws.id, "t1", "feature", "claude").await.unwrap(); + + let raw = serde_json::json!({ + "rationale": "cross-repo change set", + "directions": [ + {"name": "producer", "repo": "api", "reason": "r", "mandate": "impl-only"}, + {"name": "consumer", "repo": "api", "reason": "r", "mandate": "impl-only", "depends_on": "producer"}, + ] + }); + save_proposal_value(&db, t.id, &raw).await.unwrap(); + + // Approve the CONSUMER FIRST (index 1) — the producer (index 0) is still fully + // undecided at this point. `fully_decided` is false, so auto_settle does nothing; + // the fix under test is that `approve_direction` resolves this lane's edge itself, + // regardless. + let consumer_id = approve_direction(&db, t.id, 1, "claude").await.unwrap(); + + let consumer_dir = repo::get_direction(&db, consumer_id).await.unwrap().unwrap(); + assert_eq!( + consumer_dir.depends_on_direction_id, + repo::UNRESOLVED_UPSTREAM_SENTINEL, + "the instant approve_direction returns the consumer for dispatch, its edge must \ + ALREADY be blocking — not 0, which would read as \"no upstream, free to merge\" \ + for the entire window until the producer is later decided" + ); + match repo::upstream_merge_state(&db, consumer_id).await { + crate::host::UpstreamStatus::Unknown { .. } => {} + other => panic!( + "a consumer dispatched before its producer is even decided must never read as \ + free to merge: got {other:?}" + ), + } + + // NOW approve the producer. The consumer's edge must self-heal to the real id. + let producer_id = approve_direction(&db, t.id, 0, "claude").await.unwrap(); + let consumer_dir_after = repo::get_direction(&db, consumer_id).await.unwrap().unwrap(); + assert_eq!( + consumer_dir_after.depends_on_direction_id, producer_id, + "once the producer materializes, the consumer's unresolved sentinel must upgrade \ + to the producer's REAL id" + ); + assert!( + matches!( + repo::upstream_merge_state(&db, consumer_id).await, + crate::host::UpstreamStatus::Pending { .. } + ), + "with a real producer now on record, the consumer reads as Pending (a fact), not \ + merely Unknown (can't tell)" + ); + + let removed = repo::delete_thread_cascade(&db, t.id).await.unwrap(); + let _ = materialize::cleanup_worktrees(&db, &removed).await; + std::env::remove_var("WEFT_HOME"); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + } + + /// THE FIX (Codex review, PR #159 planner.rs:1461 and, on the round-3 follow-up, + /// planner.rs:1776): this codebase explicitly supports duplicate same-name lanes in one + /// proposal (`confirm`'s consumed-set reconciliation exists BECAUSE of this). If a THIRD + /// lane's `depends_on` names one of two same-named lanes, silently binding to whichever + /// happens to be first could point at the WRONG producer — worse than no edge at all, + /// since it would look correctly wired while actually gating on an unrelated task. Must + /// neither guess NOR silently leave the edge at `0` (which would read as "no upstream, + /// free to merge") — it must actively BLOCK via `UNRESOLVED_UPSTREAM_SENTINEL` until the + /// lead disambiguates the names. + #[tokio::test] + async fn record_upstream_edges_blocks_an_ambiguous_duplicate_name_instead_of_guessing() { + let _env = crate::paths::ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let tag = format!("weft-upstream-ambiguous-name-{}", std::process::id()); + let root = std::env::temp_dir().join(format!("{tag}-root")); + let weft_home = std::env::temp_dir().join(format!("{tag}-home")); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + std::env::set_var("WEFT_HOME", weft_home.to_str().unwrap()); + let _repo_path = make_repo(&root, "api"); + let db = Db::connect("sqlite::memory:").await.unwrap(); + let ws = repo::create_workspace(&db, "ws").await.unwrap(); + let _ra = repo::add_repo_ref(&db, ws.id, "api", root.join("api").to_str().unwrap(), "main", "", true) + .await + .unwrap(); + let t = repo::create_thread(&db, ws.id, "t1", "feature", "claude").await.unwrap(); + + // TWO lanes named "producer" (a legitimate duplicate-name proposal) + a "consumer" + // that names "producer" — ambiguous, must resolve to NEITHER. + let raw = serde_json::json!({ + "rationale": "cross-repo change set", + "directions": [ + {"name": "producer", "repo": "api", "reason": "r1", "mandate": "impl-only"}, + {"name": "producer", "repo": "api", "reason": "r2", "mandate": "impl-only"}, + {"name": "consumer", "repo": "api", "reason": "r3", "mandate": "impl-only", "depends_on": "producer"}, + ] + }); + save_proposal_value(&db, t.id, &raw).await.unwrap(); + let ids = confirm(&db, t.id).await.unwrap(); + assert_eq!(ids.len(), 3, "all three known-repo lanes are dispatched"); + + let consumer_dir = repo::get_direction(&db, ids[2]).await.unwrap().unwrap(); + assert_eq!( + consumer_dir.depends_on_direction_id, + repo::UNRESOLVED_UPSTREAM_SENTINEL, + "an ambiguous depends_on (two lanes share the name) must actively BLOCK — never \ + silently bind to whichever producer happened to be first, and never silently read \ + as having no upstream at all either" + ); + match repo::upstream_merge_state(&db, ids[2]).await { + crate::host::UpstreamStatus::Unknown { .. } => {} + other => panic!("an ambiguous depends_on must never read as free to merge: {other:?}"), + } + + let removed = repo::delete_thread_cascade(&db, t.id).await.unwrap(); + let _ = materialize::cleanup_worktrees(&db, &removed).await; + std::env::remove_var("WEFT_HOME"); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + } + + /// THE FIX (index-based redesign, Codex review PR #159 planner.rs:1557): a lane whose + /// `depends_on` names ITSELF must be rejected at PROPOSAL-SAVE time, not deferred to + /// `set_direction_upstream`'s best-effort write. Under the OLD name-search design, a + /// uniquely-named self-reference resolved via candidate search to a SINGLETON match — the + /// lane's own materialized id — attempted a self-edge write, `set_direction_upstream` + /// rejected it, and the rejection was invisible: `depends_on_direction_id` just stayed at + /// its prior value, `0`, reading as "no dependency" instead of blocked. Proves BOTH halves: + /// the rejection is a SAVE-TIME, structural fact (`depends_on_index` is `null` immediately + /// after `save_proposal_value`, before any confirm/materialize — no write is ever + /// attempted), and the end-to-end result is VISIBLY blocking, never silently free to merge. + #[tokio::test] + async fn a_self_referential_depends_on_is_rejected_at_proposal_save_time() { + let _env = crate::paths::ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let tag = format!("weft-upstream-self-ref-{}", std::process::id()); + let root = std::env::temp_dir().join(format!("{tag}-root")); + let weft_home = std::env::temp_dir().join(format!("{tag}-home")); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + std::env::set_var("WEFT_HOME", weft_home.to_str().unwrap()); + let _repo_path = make_repo(&root, "api"); + let db = Db::connect("sqlite::memory:").await.unwrap(); + let ws = repo::create_workspace(&db, "ws").await.unwrap(); + let _ra = repo::add_repo_ref(&db, ws.id, "api", root.join("api").to_str().unwrap(), "main", "", true) + .await + .unwrap(); + let t = repo::create_thread(&db, ws.id, "t1", "feature", "claude").await.unwrap(); + + let raw = serde_json::json!({ + "rationale": "r", + "directions": [ + {"name": "solo", "repo": "api", "reason": "r", "mandate": "impl-only", "depends_on": "solo"}, + ] + }); + save_proposal_value(&db, t.id, &raw).await.unwrap(); + + // SAVE-TIME: the rejection is a structural fact in the stored JSON, before anything is + // confirmed or materialized — proving this is caught at resolution, not deferred to a + // write that then silently fails. + let stored = repo::get_plan(&db, t.id).await.unwrap().unwrap(); + let stored_json: Value = serde_json::from_str(&stored.proposal).unwrap(); + assert_eq!( + stored_json["directions"][0]["depends_on_index"], + Value::Null, + "a self-reference must be flagged unresolvable at proposal-save time, not left \ + looking like a valid pending reference to itself" + ); + + // END-TO-END: confirming it must read as VISIBLY blocked, never silently free to merge. + let ids = confirm(&db, t.id).await.unwrap(); + assert_eq!(ids.len(), 1); + let dir = repo::get_direction(&db, ids[0]).await.unwrap().unwrap(); + assert_ne!( + dir.depends_on_direction_id, 0, + "a self-reference must never silently read as \"no dependency\" — the exact bug \ + the old name-search-at-write-time design had (the self-edge write was rejected by \ + set_direction_upstream, but the rejection left depends_on_direction_id at its \ + prior value, 0)" + ); + match repo::upstream_merge_state(&db, ids[0]).await { + crate::host::UpstreamStatus::Unknown { .. } => {} + other => panic!("a self-reference must read Unknown (blocking), never {other:?}"), + } + + let removed = repo::delete_thread_cascade(&db, t.id).await.unwrap(); + let _ = materialize::cleanup_worktrees(&db, &removed).await; + std::env::remove_var("WEFT_HOME"); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + } + + /// THE FIX (index-based redesign, Codex review PR #159 planner.rs:1554): two lanes share a + /// name, and a third names it via `depends_on`. Under the OLD design, `record_upstream_ + /// edges`'s candidate search filtered by `direction_id != 0` ("materialized") on EVERY + /// approve/deny re-run — so once ONE of the two duplicates got individually approved (while + /// the other stayed pending), the filter saw only ONE candidate and silently bound the + /// consumer to WHICHEVER duplicate happened to materialize first, even though the name was + /// genuinely ambiguous. Proves resolution is now a FIXED, save-time fact that decision + /// state cannot retroactively change: approves ONE duplicate individually (materializing + /// it, leaving the other genuinely pending) and confirms the ambiguity is UNCHANGED by that. + #[tokio::test] + async fn a_duplicate_name_stays_ambiguous_regardless_of_which_sibling_materializes_first() { + let _env = crate::paths::ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let tag = format!("weft-upstream-dup-name-decision-state-{}", std::process::id()); + let root = std::env::temp_dir().join(format!("{tag}-root")); + let weft_home = std::env::temp_dir().join(format!("{tag}-home")); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + std::env::set_var("WEFT_HOME", weft_home.to_str().unwrap()); + let _repo_path = make_repo(&root, "api"); + let db = Db::connect("sqlite::memory:").await.unwrap(); + let ws = repo::create_workspace(&db, "ws").await.unwrap(); + let _ra = repo::add_repo_ref(&db, ws.id, "api", root.join("api").to_str().unwrap(), "main", "", true) + .await + .unwrap(); + let t = repo::create_thread(&db, ws.id, "t1", "feature", "claude").await.unwrap(); + + // TWO lanes named "producer" (index 0 and 1) + "consumer" naming "producer" (index 2). + let raw = serde_json::json!({ + "rationale": "r", + "directions": [ + {"name": "producer", "repo": "api", "reason": "r1", "mandate": "impl-only"}, + {"name": "producer", "repo": "api", "reason": "r2", "mandate": "impl-only"}, + {"name": "consumer", "repo": "api", "reason": "r3", "mandate": "impl-only", "depends_on": "producer"}, + ] + }); + save_proposal_value(&db, t.id, &raw).await.unwrap(); + + // Approve ONLY the first "producer" duplicate (index 0) — materializing it while the + // second duplicate (index 1) and "consumer" (index 2) stay pending, undecided. + let first_producer_id = approve_direction(&db, t.id, 0, "claude").await.unwrap(); + + // Now approve "consumer" — this re-resolves depends_on with ONE duplicate materialized + // and the OTHER still pending. Under the old design this is exactly where the false + // singleton bug fired. + let consumer_id = approve_direction(&db, t.id, 2, "claude").await.unwrap(); + + let consumer_dir = repo::get_direction(&db, consumer_id).await.unwrap().unwrap(); + assert_ne!( + consumer_dir.depends_on_direction_id, first_producer_id, + "the ambiguous name must NOT silently bind to whichever duplicate happened to \ + materialize first — that is the exact bug this test guards against" + ); + assert_eq!( + consumer_dir.depends_on_direction_id, + repo::UNRESOLVED_UPSTREAM_SENTINEL, + "an ambiguous name must actively block regardless of the duplicates' decision states" + ); + match repo::upstream_merge_state(&db, consumer_id).await { + crate::host::UpstreamStatus::Unknown { .. } => {} + other => panic!( + "a consumer bound to an ambiguous name must never read as free to merge, even \ + with one duplicate materialized: {other:?}" + ), + } + + let removed = repo::delete_thread_cascade(&db, t.id).await.unwrap(); + let _ = materialize::cleanup_worktrees(&db, &removed).await; + std::env::remove_var("WEFT_HOME"); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + } + + /// THE FIX (index-based redesign): proves duplicate-name ambiguity is now a STRUCTURAL, + /// proposal-save-time fact (`depends_on_index` is `null` the instant `save_proposal_value` + /// returns), not something `record_upstream_edges` has to re-derive by searching sibling + /// lanes' names and materialization state on every decision — under the new design, there + /// is no more "ambiguity resolution" happening at write time at all, only a lookup of an + /// already-decided fact. Companion to `record_upstream_edges_blocks_an_ambiguous_ + /// duplicate_name_instead_of_guessing` (below), which proves the END-TO-END behavior; this + /// one proves WHEN the decision is made. + #[tokio::test] + async fn duplicate_name_ambiguity_is_decided_at_save_time_before_any_decision_is_made() { + let db = Db::connect("sqlite::memory:").await.unwrap(); + let ws = repo::create_workspace(&db, "ws").await.unwrap(); + let _ra = repo::add_repo_ref(&db, ws.id, "api", "/tmp/dup-ambiguity-save-time-api", "main", "", true) + .await + .unwrap(); + let t = repo::create_thread(&db, ws.id, "t1", "feature", "claude").await.unwrap(); + + let raw = serde_json::json!({ + "rationale": "r", + "directions": [ + {"name": "producer", "repo": "api", "reason": "r1", "mandate": "impl-only"}, + {"name": "producer", "repo": "api", "reason": "r2", "mandate": "impl-only"}, + {"name": "consumer", "repo": "api", "reason": "r3", "mandate": "impl-only", "depends_on": "producer"}, + ] + }); + save_proposal_value(&db, t.id, &raw).await.unwrap(); + + // No confirm, no approve, nothing materialized — just the raw stored proposal. + let stored = repo::get_plan(&db, t.id).await.unwrap().unwrap(); + let stored_json: Value = serde_json::from_str(&stored.proposal).unwrap(); + assert_eq!( + stored_json["directions"][2]["depends_on_index"], + Value::Null, + "an ambiguous duplicate name must be flagged unresolvable the instant the proposal \ + is saved — this is a structural fact about the proposal's own shape, decidable \ + with zero knowledge of any decision that hasn't happened yet" + ); + } + + /// THE FIX (Codex review, PR #159 planner.rs:1772): a mutual 2-cycle (`a` depends on `b`, + /// `b` depends on `a`) is NOT a self-reference (excluded at resolve time — see + /// `resolve_depends_on_indices`) and NOT an ambiguous name — each name resolves to exactly + /// ONE other lane, cleanly, at save time. The cycle only exists at the GRAPH level, which + /// per-lane index resolution cannot see; it only surfaces when `record_upstream_edges`'s + /// two-pass write actually tries to install both edges: whichever lane processes first + /// writes successfully, and the second write is correctly rejected by + /// `set_direction_upstream`'s `creates_cycle` check. Before this fix, that rejection was + /// swallowed by `set_upstream_edge_if_changed`, leaving the rejected lane's + /// `depends_on_direction_id` at its prior value (`0`) — reading as dependency-free and + /// free to auto-merge despite its declared (if cyclic) prerequisite never being satisfied. + #[tokio::test] + async fn a_mutual_cycle_leaves_the_rejected_lane_blocked_not_dependency_free() { + let _env = crate::paths::ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let tag = format!("weft-upstream-mutual-cycle-{}", std::process::id()); + let root = std::env::temp_dir().join(format!("{tag}-root")); + let weft_home = std::env::temp_dir().join(format!("{tag}-home")); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + std::env::set_var("WEFT_HOME", weft_home.to_str().unwrap()); + let _repo_path = make_repo(&root, "api"); + let db = Db::connect("sqlite::memory:").await.unwrap(); + let ws = repo::create_workspace(&db, "ws").await.unwrap(); + let _ra = repo::add_repo_ref(&db, ws.id, "api", root.join("api").to_str().unwrap(), "main", "", true) + .await + .unwrap(); + let t = repo::create_thread(&db, ws.id, "t1", "feature", "claude").await.unwrap(); + + let raw = serde_json::json!({ + "rationale": "r", + "directions": [ + {"name": "a", "repo": "api", "reason": "r", "mandate": "impl-only", "depends_on": "b"}, + {"name": "b", "repo": "api", "reason": "r", "mandate": "impl-only", "depends_on": "a"}, + ] + }); + save_proposal_value(&db, t.id, &raw).await.unwrap(); + let ids = confirm(&db, t.id).await.unwrap(); + assert_eq!(ids.len(), 2, "both lanes are dispatched — the cycle blocks EDGES, not materialization"); + let (a_id, b_id) = (ids[0], ids[1]); + + let a_dir = repo::get_direction(&db, a_id).await.unwrap().unwrap(); + let b_dir = repo::get_direction(&db, b_id).await.unwrap().unwrap(); + // `record_upstream_edges`'s two-pass write processes lanes in PROPOSAL ARRAY ORDER + // (`a` at index 0, `b` at index 1 — see `upstream_lanes_from_resolved`'s "indices + // align 1:1" doc): `a`'s write (a -> b) lands first, while b -> a does not exist yet, + // so it succeeds cleanly; `b`'s write (b -> a) then finds a -> b ALREADY in the DB and + // is correctly rejected as a cycle by `set_direction_upstream`. + assert_eq!( + a_dir.depends_on_direction_id, b_id, + "the FIRST half of the cycle to be written wins cleanly" + ); + assert_eq!( + b_dir.depends_on_direction_id, + repo::UNRESOLVED_UPSTREAM_SENTINEL, + "the SECOND half — rejected by the cycle check — must land on the blocking \ + sentinel, not silently stay at 0 (\"no dependency\") just because \ + set_direction_upstream's write failed" + ); + match repo::upstream_merge_state(&db, b_id).await { + crate::host::UpstreamStatus::Unknown { .. } => {} + other => panic!("the rejected half of a cycle must read Unknown (blocking), never {other:?}"), + } + + let removed = repo::delete_thread_cascade(&db, t.id).await.unwrap(); + let _ = materialize::cleanup_worktrees(&db, &removed).await; + std::env::remove_var("WEFT_HOME"); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + } + + /// THE FIX (Codex review, PR #159 planner.rs:1797): a read failure in + /// `set_upstream_edge_if_changed`, BEFORE any write is even attempted, must fall back to + /// the blocking sentinel exactly like a rejected write does — otherwise a direction that + /// has never had a successful edge write stays at the schema default `0`, indistinguishable + /// from a genuinely dependency-free lane. + #[tokio::test] + async fn a_read_failure_before_the_write_falls_back_to_the_blocking_sentinel() { + let _env = crate::paths::ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let tag = format!("weft-upstream-read-fail-{}", std::process::id()); + let root = std::env::temp_dir().join(format!("{tag}-root")); + let weft_home = std::env::temp_dir().join(format!("{tag}-home")); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + std::env::set_var("WEFT_HOME", weft_home.to_str().unwrap()); + let _repo_path = make_repo(&root, "api"); + let db = Db::connect("sqlite::memory:").await.unwrap(); + let ws = repo::create_workspace(&db, "ws").await.unwrap(); + let _ra = repo::add_repo_ref(&db, ws.id, "api", root.join("api").to_str().unwrap(), "main", "", true) + .await + .unwrap(); + let t = repo::create_thread(&db, ws.id, "t1", "feature", "claude").await.unwrap(); + + let raw = serde_json::json!({ + "rationale": "r", + "directions": [ + {"name": "producer", "repo": "api", "reason": "r", "mandate": "impl-only"}, + {"name": "consumer", "repo": "api", "reason": "r", "mandate": "impl-only", "depends_on": "producer"}, + ] + }); + save_proposal_value(&db, t.id, &raw).await.unwrap(); + let ids = confirm(&db, t.id).await.unwrap(); + assert_eq!(ids.len(), 2); + let (producer_id, consumer_id) = (ids[0], ids[1]); + let before = repo::get_direction(&db, consumer_id).await.unwrap().unwrap(); + assert_eq!(before.depends_on_direction_id, producer_id, "precondition: the edge landed normally"); + + // Simulate a later re-check (the same call every confirm/approve makes internally) + // whose READ transiently fails, before it can even attempt a write. + repo::fail_write::while_failing("get_direction", async { + set_upstream_edge_if_changed(&db, t.id, consumer_id, producer_id).await; + }) + .await; + + let after = repo::get_direction(&db, consumer_id).await.unwrap().unwrap(); + assert_eq!( + after.depends_on_direction_id, + repo::UNRESOLVED_UPSTREAM_SENTINEL, + "a read failure before the write must fall back to the blocking sentinel — a \ + caller cannot tell 'the read failed, the prior value happened to already be \ + right' from 'the read failed, and the prior value was 0 (unset)' without this \ + signal, so the safe choice is to always land on the blocking sentinel" + ); + + let removed = repo::delete_thread_cascade(&db, t.id).await.unwrap(); + let _ = materialize::cleanup_worktrees(&db, &removed).await; + std::env::remove_var("WEFT_HOME"); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + } + + /// THE FIX (Codex review, PR #159 planner.rs:1549): `record_upstream_edges` runs AFTER the + /// plan-confirmation CAS commits, as a separate best-effort step (see that function's own + /// doc). Simulates a crash in exactly that window: the edge is reset to the untouched + /// schema default `0`, precisely as it would read if `record_upstream_edges` had never run + /// at all after a real CAS commit. A restart's redispatch (the idempotent "already + /// confirmed" fast path `confirm()` takes on a second call) must REPAIR the edge, not just + /// re-dispatch workers against a permanently dependency-free-looking consumer forever. + #[tokio::test] + async fn the_confirmed_fast_path_repairs_an_upstream_edge_left_at_the_default_by_an_interrupted_record() { + let _env = crate::paths::ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let tag = format!("weft-upstream-fastpath-repair-{}", std::process::id()); + let root = std::env::temp_dir().join(format!("{tag}-root")); + let weft_home = std::env::temp_dir().join(format!("{tag}-home")); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + std::env::set_var("WEFT_HOME", weft_home.to_str().unwrap()); + let _repo_path = make_repo(&root, "api"); + let db = Db::connect("sqlite::memory:").await.unwrap(); + let ws = repo::create_workspace(&db, "ws").await.unwrap(); + let _ra = repo::add_repo_ref(&db, ws.id, "api", root.join("api").to_str().unwrap(), "main", "", true) + .await + .unwrap(); + let t = repo::create_thread(&db, ws.id, "t1", "feature", "claude").await.unwrap(); + + let raw = serde_json::json!({ + "rationale": "r", + "directions": [ + {"name": "producer", "repo": "api", "reason": "r", "mandate": "impl-only"}, + {"name": "consumer", "repo": "api", "reason": "r", "mandate": "impl-only", "depends_on": "producer"}, + ] + }); + save_proposal_value(&db, t.id, &raw).await.unwrap(); + let ids = confirm(&db, t.id).await.unwrap(); + assert_eq!(ids.len(), 2); + let (producer_id, consumer_id) = (ids[0], ids[1]); + let before = repo::get_direction(&db, consumer_id).await.unwrap().unwrap(); + assert_eq!(before.depends_on_direction_id, producer_id, "precondition: the normal path wrote the edge"); + + // Simulate the crash: reset the edge to the untouched schema default, exactly as if + // `record_upstream_edges` had never run after the CAS commit above. + repo::set_direction_upstream(&db, consumer_id, 0).await.unwrap(); + let mid = repo::get_direction(&db, consumer_id).await.unwrap().unwrap(); + assert_eq!(mid.depends_on_direction_id, 0, "precondition: the edge now reads exactly like a crash-interrupted one"); + + // The plan is ALREADY "confirmed" (the first confirm() call above set that) — this + // second call takes the idempotent fast path, not the main materialize-and-CAS path. + let redispatched = confirm(&db, t.id).await.unwrap(); + assert_eq!(redispatched.len(), 2, "the fast path still redispatches both lanes by their recorded ids"); + + let after = repo::get_direction(&db, consumer_id).await.unwrap().unwrap(); + assert_eq!( + after.depends_on_direction_id, producer_id, + "the fast path must REPAIR a missing upstream edge on every redispatch, not just \ + re-dispatch workers against a permanently dependency-free-looking consumer" + ); + + let removed = repo::delete_thread_cascade(&db, t.id).await.unwrap(); + let _ = materialize::cleanup_worktrees(&db, &removed).await; + std::env::remove_var("WEFT_HOME"); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + } + + /// THE FIX (Codex review, PR #159 planner.rs:1447): a lead re-proposes a reusable + /// consumer lane WITHOUT its former `depends_on`. Confirmation must not leave the + /// direction row holding the STALE edge from the earlier proposal — the monitor/ + /// auto-merge gate would otherwise keep blocking (or gating) it on an upstream the lead + /// no longer intends to declare. + #[tokio::test] + async fn confirm_clears_a_stale_upstream_edge_when_a_reproposal_drops_depends_on() { + let _env = crate::paths::ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let tag = format!("weft-upstream-stale-clear-{}", std::process::id()); + let root = std::env::temp_dir().join(format!("{tag}-root")); + let weft_home = std::env::temp_dir().join(format!("{tag}-home")); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + std::env::set_var("WEFT_HOME", weft_home.to_str().unwrap()); + let _repo_path = make_repo(&root, "api"); + let db = Db::connect("sqlite::memory:").await.unwrap(); + let ws = repo::create_workspace(&db, "ws").await.unwrap(); + let _ra = repo::add_repo_ref(&db, ws.id, "api", root.join("api").to_str().unwrap(), "main", "", true) + .await + .unwrap(); + let t = repo::create_thread(&db, ws.id, "t1", "feature", "claude").await.unwrap(); + + // First proposal: consumer depends on producer. Approve the producer lane so its + // direction exists, but leave the consumer PENDING so confirm() can still act on it. + let first = serde_json::json!({ + "rationale": "r", + "directions": [ + {"name": "producer", "repo": "api", "reason": "r", "mandate": "impl-only"}, + {"name": "consumer", "repo": "api", "reason": "r", "mandate": "impl-only", "depends_on": "producer"}, + ] + }); + save_proposal_value(&db, t.id, &first).await.unwrap(); + let producer_id = approve_direction(&db, t.id, 0, "claude").await.unwrap(); + let ids = confirm(&db, t.id).await.unwrap(); + assert_eq!(ids.len(), 1, "only the pending consumer lane is dispatched by this confirm"); + let consumer_id = ids[0]; + let consumer_dir = repo::get_direction(&db, consumer_id).await.unwrap().unwrap(); + assert_eq!( + consumer_dir.depends_on_direction_id, producer_id, + "precondition: the first proposal's edge landed" + ); + + // Re-propose the SAME consumer lane (reused: same name+repo+base) WITHOUT + // `depends_on` this time — the lead has dropped the dependency. + let second = serde_json::json!({ + "rationale": "r2", + "directions": [ + {"name": "consumer", "repo": "api", "reason": "r", "mandate": "impl-only"}, + ] + }); + save_proposal_value(&db, t.id, &second).await.unwrap(); + let ids2 = confirm(&db, t.id).await.unwrap(); + assert_eq!(ids2, vec![consumer_id], "the re-proposal reuses the SAME consumer direction"); + + let consumer_dir_after = repo::get_direction(&db, consumer_id).await.unwrap().unwrap(); + assert_eq!( + consumer_dir_after.depends_on_direction_id, 0, + "dropping depends_on in a re-proposal must CLEAR the stale edge, not leave it \ + pointing at a producer the lead no longer named" + ); + assert_eq!( + repo::upstream_merge_state(&db, consumer_id).await, + crate::host::UpstreamStatus::None, + "with the edge cleared, the ordering axis must read as no-upstream again" + ); + + let removed = repo::delete_thread_cascade(&db, t.id).await.unwrap(); + let _ = materialize::cleanup_worktrees(&db, &removed).await; + std::env::remove_var("WEFT_HOME"); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + } + + /// THE FIX (Codex review, PR #159 planner.rs:1570): a re-proposal that REVERSES a reused + /// dependency (round 1: "x" depends on "y"; round 2: "y" depends on "x" instead) must land + /// the reversed edge regardless of which lane the lead happens to list first. Lists the NEW + /// consumer ("y") BEFORE the new producer ("x") deliberately — the exact shape that, before + /// `record_upstream_edges`'s two-pass fix, hit `set_direction_upstream`'s cycle check while + /// the stale x→y edge was still in the DB (a single straight-through pass would try writing + /// y→x first, see x still pointing at y, and reject it as a cycle) and silently stranded + /// y's real new dependency at `0`. + #[tokio::test] + async fn confirm_applies_a_reversed_dependency_regardless_of_lane_order() { + let _env = crate::paths::ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let tag = format!("weft-upstream-reversed-edge-{}", std::process::id()); + let root = std::env::temp_dir().join(format!("{tag}-root")); + let weft_home = std::env::temp_dir().join(format!("{tag}-home")); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + std::env::set_var("WEFT_HOME", weft_home.to_str().unwrap()); + let _repo_path = make_repo(&root, "api"); + let db = Db::connect("sqlite::memory:").await.unwrap(); + let ws = repo::create_workspace(&db, "ws").await.unwrap(); + let _ra = repo::add_repo_ref(&db, ws.id, "api", root.join("api").to_str().unwrap(), "main", "", true) + .await + .unwrap(); + let t = repo::create_thread(&db, ws.id, "t1", "feature", "claude").await.unwrap(); + + // Round 1: x depends on y. + let first = serde_json::json!({ + "rationale": "r", + "directions": [ + {"name": "x", "repo": "api", "reason": "r", "mandate": "impl-only", "depends_on": "y"}, + {"name": "y", "repo": "api", "reason": "r", "mandate": "impl-only"}, + ] + }); + save_proposal_value(&db, t.id, &first).await.unwrap(); + let ids = confirm(&db, t.id).await.unwrap(); + assert_eq!(ids.len(), 2, "both lanes are dispatched"); + let x_id = ids[0]; + let y_id = ids[1]; + assert_eq!( + repo::get_direction(&db, x_id).await.unwrap().unwrap().depends_on_direction_id, + y_id, + "precondition: the first proposal's x→y edge landed" + ); + + // Round 2: SAME two lanes reused (same name+repo+base — matched by + // `reusable_direction_for_preview`'s identity, not array position), REVERSED: y now + // depends on x, x drops its dependency. "y" (the NEW consumer) is listed FIRST. + let second = serde_json::json!({ + "rationale": "r2", + "directions": [ + {"name": "y", "repo": "api", "reason": "r", "mandate": "impl-only", "depends_on": "x"}, + {"name": "x", "repo": "api", "reason": "r", "mandate": "impl-only"}, + ] + }); + save_proposal_value(&db, t.id, &second).await.unwrap(); + let ids2 = confirm(&db, t.id).await.unwrap(); + assert_eq!(ids2.len(), 2, "both reused lanes are re-dispatched"); + + let x_after = repo::get_direction(&db, x_id).await.unwrap().unwrap(); + let y_after = repo::get_direction(&db, y_id).await.unwrap().unwrap(); + assert_eq!( + x_after.depends_on_direction_id, 0, + "x no longer names a dependency — its stale edge must be cleared" + ); + assert_eq!( + y_after.depends_on_direction_id, x_id, + "y must end up depending on x — the REVERSED edge — not stuck at 0 just because \ + the stale x→y edge was still in the DB when y's lane was processed first" + ); + assert_eq!( + repo::upstream_merge_state(&db, y_id).await, + crate::host::UpstreamStatus::Pending { what: "x".into() }, + "the axis this edge feeds must see the REVERSED relationship: y now blocked on x" + ); + + let removed = repo::delete_thread_cascade(&db, t.id).await.unwrap(); + let _ = materialize::cleanup_worktrees(&db, &removed).await; + std::env::remove_var("WEFT_HOME"); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + } + + /// THE FIX (Codex review, PR #159 planner.rs:1600, a follow-up round on planner.rs:1570's + /// own fix): `record_upstream_edges`'s two passes are two SEPARATE, non-transactional + /// awaited writes with no lock held across the gap — `host::automerge`/`host::monitor` run + /// on their own independent sweep loop and can read a direction's row at any point, + /// including exactly between pass 1 and pass 2. Re-proposes a reused consumer's dependency + /// from producer1 to producer2 (a REPLACEMENT: pass 1 must actually clear something, + /// unlike a fresh 0→X assignment) and pauses `record_upstream_edges` at that exact gap + /// (`between_upstream_passes_probe`) to drive a CONCURRENT read of `upstream_merge_state` — + /// the same call `host::automerge`/`host::monitor` make — from a separate task. Before the + /// sentinel fix, pass 1 cleared to `0`, which reads `UpstreamStatus::None` ("no upstream, + /// free to merge") — true of the DB row at that instant, and enough for a sweep to release + /// a PR whose other 3 axes were already cached `Ready`, before pass 2 installs producer2. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn record_upstream_edges_never_reads_free_to_merge_between_its_two_passes() { + let _env = crate::paths::ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let tag = format!("weft-upstream-between-passes-{}", std::process::id()); + let root = std::env::temp_dir().join(format!("{tag}-root")); + let weft_home = std::env::temp_dir().join(format!("{tag}-home")); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + std::env::set_var("WEFT_HOME", weft_home.to_str().unwrap()); + let _repo_path = make_repo(&root, "api"); + // File-backed (not :memory:) DB, same reason as `thread_gate_serializes_concurrent_ + // confirms`: the cloned pool handle in the spawned task must see the SAME store as the + // test's own concurrent read, and an in-memory SQLite connection is private to itself. + let db_file = weft_home.join("between-passes.sqlite"); + std::fs::create_dir_all(&weft_home).unwrap(); + let db = Db::connect(&format!("sqlite://{}?mode=rwc", db_file.to_str().unwrap())) + .await + .unwrap(); + let ws = repo::create_workspace(&db, "ws").await.unwrap(); + let _ra = repo::add_repo_ref(&db, ws.id, "api", root.join("api").to_str().unwrap(), "main", "", true) + .await + .unwrap(); + let t = repo::create_thread(&db, ws.id, "t1", "feature", "claude").await.unwrap(); + + // Round 1: consumer depends on producer1. + let first = serde_json::json!({ + "rationale": "r", + "directions": [ + {"name": "producer1", "repo": "api", "reason": "r", "mandate": "impl-only"}, + {"name": "producer2", "repo": "api", "reason": "r", "mandate": "impl-only"}, + {"name": "consumer", "repo": "api", "reason": "r", "mandate": "impl-only", "depends_on": "producer1"}, + ] + }); + save_proposal_value(&db, t.id, &first).await.unwrap(); + let ids = confirm(&db, t.id).await.unwrap(); + assert_eq!(ids.len(), 3, "all three lanes are dispatched"); + let producer2_id = ids[1]; + let consumer_id = ids[2]; + assert_eq!( + repo::get_direction(&db, consumer_id).await.unwrap().unwrap().depends_on_direction_id, + ids[0], + "precondition: consumer depends on producer1" + ); + + // Round 2: SAME three lanes reused, consumer's dependency REPLACED with producer2 — + // pass 1 has a real, non-zero edge to actually clear (unlike a fresh 0→X assignment). + let second = serde_json::json!({ + "rationale": "r2", + "directions": [ + {"name": "producer1", "repo": "api", "reason": "r", "mandate": "impl-only"}, + {"name": "producer2", "repo": "api", "reason": "r", "mandate": "impl-only"}, + {"name": "consumer", "repo": "api", "reason": "r", "mandate": "impl-only", "depends_on": "producer2"}, + ] + }); + save_proposal_value(&db, t.id, &second).await.unwrap(); + + let (reached_rx, resume_tx) = tests::arm_between_upstream_passes_probe(t.id); + let db2 = db.clone(); + let tid = t.id; + let confirm_handle = tokio::spawn(async move { confirm(&db2, tid).await }); + + // Wait for record_upstream_edges to reach the gap between its two passes, then — + // WHILE it is still paused there — read exactly what a concurrent automerge/monitor + // sweep would read. + reached_rx.await.expect("record_upstream_edges reached the between-passes probe"); + let mid_transition = repo::upstream_merge_state(&db, consumer_id).await; + // Release the pause so pass 2 (and the rest of confirm) can complete. + let _ = resume_tx.send(()); + let ids2 = confirm_handle.await.unwrap().unwrap(); + assert_eq!(ids2.len(), 3, "all three reused lanes are re-dispatched"); + + assert!( + !matches!(mid_transition, crate::host::UpstreamStatus::None), + "a concurrent sweep landing between record_upstream_edges's two passes must NEVER \ + read None (\"no upstream, free to merge\") — got {mid_transition:?}, which would \ + let automerge release this PR before pass 2 installs the real new producer" + ); + assert!( + matches!(mid_transition, crate::host::UpstreamStatus::Unknown { .. }), + "mid-transition must read Unknown (blocking, fail-closed), not any other verdict — \ + got {mid_transition:?}" + ); + + let consumer_after = repo::get_direction(&db, consumer_id).await.unwrap().unwrap(); + assert_eq!( + consumer_after.depends_on_direction_id, producer2_id, + "after resuming, pass 2 must still land the real replacement edge" + ); + + let removed = repo::delete_thread_cascade(&db, t.id).await.unwrap(); + let _ = materialize::cleanup_worktrees(&db, &removed).await; + std::env::remove_var("WEFT_HOME"); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + } + + /// THE FIX (Codex review, PR #159 planner.rs:1534): when the human DENIES the producer lane + /// (not merely leaves it undecided), a consumer naming it via `depends_on` must NOT read as + /// having no upstream — its declared prerequisite is a decided, permanent dead end. Denying + /// producer then confirming the still-pending consumer must record the deny-sentinel edge, + /// keeping `upstream_merge_state` (and therefore the auto-merge gate) blocked rather than + /// silently free. + #[tokio::test] + async fn confirm_blocks_a_consumer_whose_named_upstream_was_denied() { + let _env = crate::paths::ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let tag = format!("weft-upstream-denied-producer-{}", std::process::id()); + let root = std::env::temp_dir().join(format!("{tag}-root")); + let weft_home = std::env::temp_dir().join(format!("{tag}-home")); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + std::env::set_var("WEFT_HOME", weft_home.to_str().unwrap()); + let _repo_path = make_repo(&root, "api"); + let db = Db::connect("sqlite::memory:").await.unwrap(); + let ws = repo::create_workspace(&db, "ws").await.unwrap(); + let _ra = repo::add_repo_ref(&db, ws.id, "api", root.join("api").to_str().unwrap(), "main", "", true) + .await + .unwrap(); + let t = repo::create_thread(&db, ws.id, "t1", "feature", "claude").await.unwrap(); + + let raw = serde_json::json!({ + "rationale": "cross-repo change set", + "directions": [ + {"name": "producer", "repo": "api", "reason": "r", "mandate": "impl-only"}, + {"name": "consumer", "repo": "api", "reason": "r", "mandate": "impl-only", "depends_on": "producer"}, + ] + }); + save_proposal_value(&db, t.id, &raw).await.unwrap(); + + // Deny the producer lane — it never materializes (direction_id stays 0), but this is a + // DECIDED fact, not an undecided/typo'd reference. + deny_direction(&db, t.id, 0).await.unwrap(); + // Batch-confirm the still-pending consumer. + let ids = confirm(&db, t.id).await.unwrap(); + assert_eq!(ids.len(), 1, "only the pending consumer lane is dispatched"); + let consumer_id = ids[0]; + + let consumer_dir = repo::get_direction(&db, consumer_id).await.unwrap().unwrap(); + assert_eq!( + consumer_dir.depends_on_direction_id, + repo::DENIED_UPSTREAM_SENTINEL, + "a denied producer must record the deny-sentinel, not 0 (no upstream)" + ); + match repo::upstream_merge_state(&db, consumer_id).await { + crate::host::UpstreamStatus::Unknown { .. } => {} + other => panic!( + "a consumer whose named upstream was denied must NOT read as free to merge \ + (None) — its declared prerequisite can never land: got {other:?}" + ), + } + + let removed = repo::delete_thread_cascade(&db, t.id).await.unwrap(); + let _ = materialize::cleanup_worktrees(&db, &removed).await; + std::env::remove_var("WEFT_HOME"); + let _ = std::fs::remove_dir_all(&root); + let _ = std::fs::remove_dir_all(&weft_home); + } + /// THE WHOLE POINT (#42 round-6): the confirmed fast-path re-dispatches each pending lane by its /// OWN RECORDED direction id — NOT by re-matching name+repo+base. This is exactly the permutation /// that bounced the old heuristic for six review rounds: a confirmed plan with an APPROVED blank diff --git a/src-tauri/src/store/entities/direction.rs b/src-tauri/src/store/entities/direction.rs index 9e575181..6ee69c10 100644 --- a/src-tauri/src/store/entities/direction.rs +++ b/src-tauri/src/store/entities/direction.rs @@ -22,6 +22,11 @@ pub struct Model { /// in Needs-you and kept for audit. #[sea_orm(default_value = "")] pub reason: String, + /// Another task that must be MERGED before this one is considered + /// mergeable — the cross-repo ordering edge (producer → consumer). + /// `0` = no upstream, same convention as `repo_id`. + #[sea_orm(default_value = 0)] + pub depends_on_direction_id: i32, /// Explicit engine selections are pins. Legacy rows stay pinned; a new /// route-derived direction is explicitly created unpinned. #[sea_orm(default_value = true)] diff --git a/src-tauri/src/store/migration/mod.rs b/src-tauri/src/store/migration/mod.rs index 96bb4b3e..21039b46 100644 --- a/src-tauri/src/store/migration/mod.rs +++ b/src-tauri/src/store/migration/mod.rs @@ -57,6 +57,7 @@ impl MigratorTrait for Migrator { Box::new(M0043SessionModel), Box::new(M0044EngineRoutingPin), Box::new(M0045PullRequest), + Box::new(M0046DirectionUpstream), ] } } @@ -2001,6 +2002,61 @@ impl MigrationTrait for M0044EngineRoutingPin { /// host-normalized state) so "what is this PR/MR waiting on" is a store fact /// the background monitor (`crate::host::monitor`) can read and update, not /// something that only lives in an agent's turn or a chat session's memory. +/// One task may declare ANOTHER task as its upstream, so a cross-repo change +/// set can be merged in dependency order: the producer's PR lands before the +/// consumer's is considered mergeable at all. +/// +/// A single column, not a join table, and deliberately so. It expresses one +/// upstream per task, which is what a producer→consumer pair needs, and this +/// is the minimum that answers whether ordered cross-repo delivery is worth +/// building out. A real topological sequencer wants many-to-many and will need +/// its own table — see the module docs on `host::judge::UpstreamStatus`. +/// +/// `0` means "no upstream", matching the `direction.repo_id` convention rather +/// than introducing a nullable column with different emptiness semantics. +/// Existing rows migrate to 0: an upgrade must never invent a dependency that +/// would block a task the user can merge today. +pub struct M0046DirectionUpstream; +impl MigrationName for M0046DirectionUpstream { + fn name(&self) -> &str { + "m0046_direction_upstream" + } +} +#[async_trait::async_trait] +impl MigrationTrait for M0046DirectionUpstream { + async fn up(&self, manager: &SchemaManager) -> Result<(), DbErr> { + let result = manager + .alter_table( + Table::alter() + .table(Alias::new("direction")) + .add_column( + ColumnDef::new(Alias::new("depends_on_direction_id")) + .integer() + .not_null() + .default(0), + ) + .to_owned(), + ) + .await; + match result { + Ok(()) => Ok(()), + Err(err) if err.to_string().to_lowercase().contains("duplicate column") => Ok(()), + Err(err) => Err(err), + } + } + + async fn down(&self, manager: &SchemaManager) -> Result<(), DbErr> { + manager + .alter_table( + Table::alter() + .table(Alias::new("direction")) + .drop_column(Alias::new("depends_on_direction_id")) + .to_owned(), + ) + .await + } +} + pub struct M0045PullRequest; impl MigrationName for M0045PullRequest { fn name(&self) -> &str { @@ -2044,7 +2100,7 @@ impl MigrationTrait for M0045PullRequest { #[cfg(test)] mod tests { - use super::{gateway_components_to_backend, M0044EngineRoutingPin, M0045PullRequest}; + use super::{gateway_components_to_backend, M0044EngineRoutingPin, M0045PullRequest, M0046DirectionUpstream}; #[test] fn gateway_tier_rewritten_to_backend() { @@ -2403,6 +2459,83 @@ mod tests { assert!(distinct.is_ok(), "a different PR number must still insert cleanly"); } + /// M0046 (issue #110 T4, Codex review PR #159 migration/mod.rs:2036): the + /// planner/store tests otherwise use a freshly-migrated database whose earliest + /// migration already creates `direction` WITH `depends_on_direction_id` present — + /// they can never detect a regression in THIS migration's actual upgrade path. + /// This starts from a pre-M0046 `direction` table (no such column at all, mirroring + /// what a real user's existing database looks like right before upgrading), and + /// verifies: the column gets added, EXISTING rows default to `0` (never inventing a + /// dependency that would block a task the user could already merge — the migration's + /// own doc comment's promise), an explicit non-zero value set afterward is a normal + /// column value, and a RERUN (the "duplicate column" catch in `up()`) is safe and + /// does not clobber it. + #[tokio::test] + async fn m0046_direction_upstream_column_defaults_existing_rows_to_zero_and_reruns_safely() { + use sea_orm::{ConnectionTrait, Database, Statement}; + use sea_orm_migration::{MigrationTrait, SchemaManager}; + + let db = Database::connect("sqlite::memory:").await.unwrap(); + let backend = db.get_database_backend(); + // Pre-M0046 shape: a `direction` table that predates this migration entirely — + // no `depends_on_direction_id` column at all, only what an upgrade needs to + // exercise (ALTER TABLE ADD COLUMN does not care about sibling columns). + db.execute(Statement::from_string( + backend, + "CREATE TABLE direction (id INTEGER PRIMARY KEY, name TEXT NOT NULL)".to_owned(), + )) + .await + .unwrap(); + db.execute(Statement::from_string( + backend, + "INSERT INTO direction (id, name) VALUES (1, 'producer'), (2, 'consumer')".to_owned(), + )) + .await + .unwrap(); + + M0046DirectionUpstream.up(&SchemaManager::new(&db)).await.unwrap(); + + for id in [1, 2] { + let row = db + .query_one(Statement::from_string( + backend, + format!("SELECT depends_on_direction_id FROM direction WHERE id = {id}"), + )) + .await + .unwrap() + .unwrap(); + let value: i32 = row.try_get("", "depends_on_direction_id").unwrap(); + assert_eq!( + value, 0, + "an existing pre-M0046 row must default to 0 (no upstream) — never invent a \ + dependency that would block a task the user could already merge" + ); + } + + // An explicit edge, set the way `repo::set_direction_upstream` would. + db.execute(Statement::from_string( + backend, + "UPDATE direction SET depends_on_direction_id = 1 WHERE id = 2".to_owned(), + )) + .await + .unwrap(); + + // Re-running the migration (the "duplicate column" catch in `up()`) must succeed, + // not error — and must not reset the explicit value back to the column default. + M0046DirectionUpstream.up(&SchemaManager::new(&db)).await.unwrap(); + + let row = db + .query_one(Statement::from_string( + backend, + "SELECT depends_on_direction_id FROM direction WHERE id = 2".to_owned(), + )) + .await + .unwrap() + .unwrap(); + let value: i32 = row.try_get("", "depends_on_direction_id").unwrap(); + assert_eq!(value, 1, "a rerun must not clobber an already-recorded upstream edge"); + } + /// M0037: code_checkpoint exists after migration and round-trips a row. #[tokio::test] async fn m0037_code_checkpoint_table_created() { diff --git a/src-tauri/src/store/repo.rs b/src-tauri/src/store/repo.rs index b9afc5c9..1c4ff7c9 100644 --- a/src-tauri/src/store/repo.rs +++ b/src-tauri/src/store/repo.rs @@ -13,6 +13,7 @@ use sea_orm::{ TryIntoModel, }; use std::collections::HashMap; +use crate::host; /// A manual route selected for a reused direction before its first native /// conversation. `session_id` is present only when an interrupted initial @@ -89,7 +90,10 @@ pub(crate) mod fail_write { /// Mark a store write as injectable by [`fail_write`]'s `name`. Expands to /// NOTHING outside `cfg(test)` — see that module's doc for the boundary. Place /// it as the first statement of a write that returns `anyhow::Result`, so an -/// armed failure lands before any partial mutation. +/// armed failure lands before any partial mutation. The mechanism is equally +/// valid on a READ that returns `anyhow::Result` (e.g. `get_direction`, +/// PR #159 planner.rs:1797's read-before-write failure path) — the macro name +/// follows this module's dominant use, not a hard restriction to writes. macro_rules! fail_write { ($name:literal) => { #[cfg(test)] @@ -1558,11 +1562,113 @@ pub fn normalize_mandate(m: &str) -> &'static str { } pub async fn get_direction(db: &Db, direction_id: i32) -> Result> { + fail_write!("get_direction"); Ok(direction::Entity::find_by_id(direction_id) .one(&db.0) .await?) } +/// `direction.depends_on_direction_id` sentinel meaning "the declared +/// upstream lane was explicitly DENIED, not merely undecided or unnamed" — +/// distinct from `0` ("no upstream declared at all"). Never a real direction +/// id (SQLite `AUTOINCREMENT` ids start at 1 and only go up), so +/// `upstream_merge_state`'s `get_direction` lookup on it naturally falls +/// through the SAME "dangling edge" `Unknown` branch a nonexistent positive id +/// would — see `record_upstream_edges` (planner.rs, Codex review PR #159 +/// planner.rs:1534) for why this must actively block rather than silently +/// read as "no upstream" (`0`), which would let a consumer whose declared +/// prerequisite was refused merge anyway. +pub(crate) const DENIED_UPSTREAM_SENTINEL: i32 = -1; + +/// `direction.depends_on_direction_id` sentinel meaning "a `depends_on` name +/// was declared but does not (yet, or ever) resolve to exactly one +/// materialized, non-denied lane in the same proposal" — a lane that is +/// still pending, a typo'd/cross-proposal name, or an AMBIGUOUS match (two+ +/// candidates). Distinct from `0` for the SAME reason `DENIED_UPSTREAM_ +/// SENTINEL` is: this lane's stated prerequisite is not satisfied, so it +/// must not read as having no upstream at all. +/// +/// This is the fix for the race Codex found on the round-3 push (PR #159 +/// planner.rs:1776): `record_upstream_edges` used to run ONLY once a +/// proposal's LAST lane was decided (`auto_settle_if_fully_decided`) or once +/// a whole batch materialized (`confirm`). But `approve_direction` DISPATCHES +/// the lane it just approved immediately on return — if a consumer is +/// approved before its producer is even decided, the window between "this +/// call returns and a worker starts" to "the producer is later decided" left +/// `depends_on_direction_id` at `0`, i.e. exactly the state that reads as +/// "no upstream, free to merge." `record_upstream_edges` now runs after +/// EVERY individual approve/deny (see `resolve_and_record_upstream_edges`), +/// and writes THIS sentinel whenever a lane's `depends_on` cannot yet be +/// resolved — self-healing once the referenced lane later materializes, +/// since the next decision's call re-resolves every lane again from +/// scratch and upgrades the sentinel to the real id. +pub(crate) const UNRESOLVED_UPSTREAM_SENTINEL: i32 = -2; + +/// Record (or clear, with `0`) the task this one must merge after. +/// +/// Deliberately refuses a SELF edge: a task waiting on itself can never +/// resolve, and `upstream_merge_state` would report it as permanently +/// `Pending` — a task no human could ever merge, with a reason that reads like +/// a bug. +/// +/// Also refuses any LONGER cycle (A→B→C→A, …). A single upstream per task +/// cannot express a fan-in graph, but it can still express a cycle: each hop +/// only needs its own one upstream slot to point back around. Every task on a +/// cycle would report the OTHER as still-`Pending` forever — both PRs +/// permanently unmergeable, with no reason that names the actual problem. See +/// [`creates_cycle`]; the day this becomes a real many-to-many graph it needs +/// a real graph-level cycle check (see `M0046DirectionUpstream`). +/// +/// `upstream_id == DENIED_UPSTREAM_SENTINEL` is accepted like any other +/// non-zero id — it never collides with `direction_id` (a real row's id) or +/// with a real chain (the sentinel has no row, so `creates_cycle`'s walk +/// dead-ends there immediately). +pub async fn set_direction_upstream( + db: &Db, + direction_id: i32, + upstream_id: i32, +) -> Result<()> { + if upstream_id == direction_id { + anyhow::bail!("a task cannot depend on itself"); + } + if creates_cycle(db, direction_id, upstream_id).await? { + anyhow::bail!("this would create a dependency cycle"); + } + let Some(row) = direction::Entity::find_by_id(direction_id).one(&db.0).await? else { + return Ok(()); + }; + let mut a: direction::ActiveModel = row.into(); + a.depends_on_direction_id = Set(upstream_id); + a.update(&db.0).await?; + Ok(()) +} + +/// Would recording `direction_id → upstream_id` close a cycle? Walks +/// `upstream_id`'s OWN chain (its upstream, that task's upstream, …): if the +/// walk ever reaches `direction_id`, adding the new edge would complete a +/// loop back to where it started. A `visited` set bounds the walk at the +/// number of distinct tasks touched even if the existing data already +/// contains a cycle (defense in depth — this function is the only writer, so +/// that should not be reachable, but the walk must still terminate if it +/// somehow is). `0` (no upstream) and a dangling/missing row both end the +/// walk with "no cycle" — a chain that runs out is not a loop. +async fn creates_cycle(db: &Db, direction_id: i32, upstream_id: i32) -> Result { + let mut visited: std::collections::HashSet = std::collections::HashSet::new(); + let mut current = upstream_id; + loop { + if current == direction_id { + return Ok(true); + } + if current == 0 || !visited.insert(current) { + return Ok(false); + } + current = match direction::Entity::find_by_id(current).one(&db.0).await? { + Some(row) => row.depends_on_direction_id, + None => return Ok(false), + }; + } +} + pub async fn set_direction_engine_pinned( db: &Db, direction_id: i32, @@ -3658,6 +3764,427 @@ pub async fn get_pull_request(db: &Db, id: i32) -> Result host::UpstreamStatus { + if direction_id == 0 { + // The pre-existing "no owning direction" convention (`direction.repo_id`'s own + // "0 = unset" sentinel; see `register_pr_tool_from_the_lead_falls_back_to_unset_ + // direction` in `bus/server.rs`, a real, tested, supported production path): a PR the + // lead registers without a numeric direction argument is tracked with + // `pull_request.direction_id = 0`, and every caller of this function + // (`host::automerge`, `host::monitor`) passes that field straight through as + // `direction_id`. Falling through to `get_direction(db, 0)` below always finds + // nothing and reads `Unknown` ("找不到这个任务"), which forces every such PR's + // readiness to `Indeterminate` FOREVER (Codex review, PR #159 repo.rs:3792) — there is + // no direction row here that could ever carry a `depends_on_direction_id`, so "no + // upstream" is the honest answer, not "can't tell". `judge::merge_readiness` already + // treats `None` identically to having no axis at all (see judge.rs's + // `no_upstream_is_indistinguishable_from_the_three_axis_judgement`). + return host::UpstreamStatus::None; + } + let dir = match get_direction(db, direction_id).await { + Ok(Some(d)) => d, + Ok(None) => { + return host::UpstreamStatus::Unknown { + reason: "找不到这个任务".into(), + } + } + Err(e) => { + return host::UpstreamStatus::Unknown { + reason: format!("读取任务失败: {e}"), + } + } + }; + if dir.depends_on_direction_id == 0 { + return host::UpstreamStatus::None; + } + if dir.depends_on_direction_id == DENIED_UPSTREAM_SENTINEL { + return host::UpstreamStatus::Unknown { + reason: "所依赖的上游任务已被拒绝,这个任务的前提不再成立".into(), + }; + } + if dir.depends_on_direction_id == UNRESOLVED_UPSTREAM_SENTINEL { + return host::UpstreamStatus::Unknown { + reason: "上游任务尚未确定(可能还未审批,或依赖的名字有歧义),暂时无法判断是否可以合并".into(), + }; + } + let upstream = match get_direction(db, dir.depends_on_direction_id).await { + Ok(Some(u)) => u, + Ok(None) => { + return host::UpstreamStatus::Unknown { + reason: format!("上游任务 #{} 不存在", dir.depends_on_direction_id), + } + } + Err(e) => { + return host::UpstreamStatus::Unknown { + reason: format!("读取上游任务失败: {e}"), + } + } + }; + let prs = match pull_request::Entity::find() + .filter(pull_request::Column::DirectionId.eq(upstream.id)) + .all(&db.0) + .await + { + Ok(rows) => rows, + Err(e) => { + return host::UpstreamStatus::Unknown { + reason: format!("读取上游 PR 失败: {e}"), + } + } + }; + // Merged only when EVERY registered PR for the upstream reads "merged" — a CLOSED-without- + // merging row blocks, exactly like an OPEN one (Codex review, PR #159 repo.rs:3850, + // reverting repo.rs:3793's "ignore closed rows" from an earlier round of this same review + // chain). That earlier version ignored any closed-without-merging row on the theory it was + // always a superseded/abandoned retry (PR #1 closed, replacement PR #2 merged) — but + // `one_merged_pr_does_not_release_an_upstream_with_two` (below) already establishes that a + // SINGLE upstream can legitimately own multiple INDEPENDENTLY required PRs, not only + // sequential retries of the same one, and this store has no persisted "supersedes" + // relationship to tell those two shapes apart: {PR #1 closed, PR #2 merged} is IDENTICAL + // data whether #2 replaced #1 or #1 was a second required PR that got abandoned while #2 + // (unrelated in scope) merged. Given that ambiguity, this axis fails closed — the same + // posture as every other branch in this function and this PR's own design constraint + // (T4 may only ADD rejection reasons, never a release path): a superseded PR now blocks its + // consumer forever again (a visible, human-recoverable liveness gap — auto-merge stays + // opt-in and a human can always see and act on a stuck `Pending`), which is safer than + // silently releasing a consumer whose producer's real required work never landed. A proper + // fix needs an explicit, persisted supersedes/replaces relationship (who marks it, and + // when) — real scope beyond this review round; flagged as a follow-up. + if !prs.is_empty() && prs.iter().all(|p| p.lifecycle == "merged") { + // Every PR on one task normally targets the same host repo; use the FIRST one as the + // identity `all_prs_landed_on_live_default_branch` must see reflected in the local + // clone's recorded remote before it trusts that clone to answer "what is the default + // branch" (a fork/mirror checkout's `origin` need not be the repo this PR actually + // targets). + let pr0 = &prs[0]; + let host_base = pr0.host_base.clone(); + let host_owner = pr0.host_owner.clone(); + let host_repo = pr0.host_repo.clone(); + match all_prs_landed_on_live_default_branch(db, upstream.repo_id, &host_base, &host_owner, &host_repo, prs) + .await + { + None => { + // Never optimistically release the merge on an unresolvable default + // branch (offline, repo row gone, local clone doesn't provably match + // the PR's own recorded host repo) — the honest answer is "can't + // tell", the same rule every other failure branch above follows. + return host::UpstreamStatus::Unknown { + reason: "无法确认上游仓库的默认分支".into(), + }; + } + Some(true) => return host::UpstreamStatus::Merged, + Some(false) => {} // fall through to Pending below + } + } + host::UpstreamStatus::Pending { + what: upstream.name.clone(), + } +} + +/// A [`parse_remote_host_and_path`] result. +struct RemoteHostPath { + host: String, + path: String, + /// The remote's EFFECTIVE port, but ONLY when it came from a web transport scheme + /// (`https://`/`http://`) — the ONE case where it is meaningfully comparable to + /// `host_base`'s own port (see [`remote_matches_pr_host`]'s doc). When the URL omits + /// the port, this fills in the SCHEME'S OWN standard port (443 for https, 80 for + /// http) rather than leaving it absent — an omitted port on a web URL is not + /// ambiguous, it unambiguously IS that standard port (Codex review, PR #159 + /// repo.rs:4035). `None` for `ssh://`, scp-style (`user@host:path`), and bare + /// `host:path` (no `://` at all) — a port on any of those is (or would be) an SSH + /// port, unrelated to the API's own port, so there is no web-facing "effective port" + /// to compute at all, written or not. + comparable_port: Option, +} + +/// Split a git remote URL into (bare lowercase host, lowercase repo path — +/// leading/trailing slashes and a trailing `.git` stripped, plus a comparable port when +/// the transport is one where that's meaningful), or `None` when +/// the shape isn't one of the three this codebase's remotes actually take: +/// `scheme://[user@]host[:port]/path` (https/ssh), `[user@]host:path` +/// (scp-style, e.g. `git@github.com:owner/repo.git`), or a bare `host:path`. +/// Structural parsing, not `git::git_url_key` (which normalizes for EQUALITY +/// between two remote strings, not for extracting a host/path to compare +/// against a SEPARATELY-recorded host/owner/repo triple) and not a +/// substring/suffix scan (see [`remote_matches_pr_host`]'s doc for why the +/// latter is unsafe here). +fn parse_remote_host_and_path(remote_url: &str) -> Option { + let no_slash = remote_url.trim().trim_end_matches('/'); + let without_git = no_slash.strip_suffix(".git").unwrap_or(no_slash); + let (scheme, authority, path) = if let Some(pos) = without_git.find("://") { + let rest = &without_git[pos + 3..]; + let (authority, path) = match rest.find('/') { + Some(s) => (&rest[..s], &rest[s + 1..]), + None => (rest, ""), + }; + (Some(&without_git[..pos]), authority, path) + } else if let Some(colon) = without_git.find(':') { + (None, &without_git[..colon], &without_git[colon + 1..]) + } else { + return None; + }; + // Strip `user@` (userinfo) to reach the bare host[:port]. + let host_maybe_port = authority.rsplit('@').next().unwrap_or(authority); + let host = host_maybe_port.split(':').next().unwrap_or(host_maybe_port); + let path = path.trim_matches('/'); + if host.is_empty() || path.is_empty() { + return None; + } + let comparable_port = match scheme.map(str::to_lowercase).as_deref() { + Some("https") => Some( + host_maybe_port + .rsplit_once(':') + .map(|(_, port)| port.to_string()) + .unwrap_or_else(|| "443".to_string()), + ), + Some("http") => Some( + host_maybe_port + .rsplit_once(':') + .map(|(_, port)| port.to_string()) + .unwrap_or_else(|| "80".to_string()), + ), + _ => None, + }; + Some(RemoteHostPath { + host: host.to_lowercase(), + path: path.to_lowercase(), + comparable_port, + }) +} + +/// Does `remote_url` (any git remote URL string — `live_default_branch_for_repo` +/// passes the checkout's CURRENT, LIVE `origin`, not a possibly-stale DB +/// recording; see that function's doc) point at the EXACT SAME host repo a +/// PR row's recorded identity (`host_base`, `host_owner`, `host_repo`) claims? +/// +/// `live_default_branch_for_repo` runs this BEFORE trusting a local clone's +/// live default branch to answer for a specific PR (Codex review, PR #159 +/// repo.rs:3794): the clone's `origin` is whatever repo it happens to be +/// checked out against — a fork, a mirror, a re-pointed remote — which is not +/// guaranteed to be the repo the PR's `host_owner`/`host_repo` actually name. +/// Trusting it blindly could read a fork's default branch as the upstream's, +/// letting a coincidental name match (or mismatch) flip `Merged` the wrong +/// way in either direction. +/// +/// EXACT equality on the parsed host and path (via [`parse_remote_host_and_path`]), +/// not a substring/suffix check: a prior version of this function used +/// `.contains()`/`.ends_with()`, which a second independent review round +/// caught accepting `other-acme/api` for `acme/api` (GitHub owner names allow +/// hyphens, and a fork commonly keeps the upstream's repo name — `other-acme` +/// ending in `acme/api` is not a hypothetical) and `mygitlab.com` for +/// `gitlab.com` — reopening, via a different trigger, the exact +/// wrong-repo-vetted-as-Merged hole this function exists to close. `host_owner` +/// may itself contain `/` (a GitLab nested subgroup path — see that field's +/// doc on `pull_request::Model`), which plain path equality handles for free: +/// the FULL owner/subgroup/repo path must match, not just the trailing +/// segment, closing the same collision class for nested groups too (currently +/// unreachable — `HostKind::parse`/`resolve_host` have no GitLab backend yet — +/// but fixed here alongside the GitHub case since the root cause is shared). +/// +/// Host and path compare WITHOUT a port (a third review round caught the +/// asymmetry: `parse_remote_host_and_path` already strips the remote's port, +/// but `host_base` — which for a self-hosted install can read like +/// `git.internal:8443` — was compared as-is, so even a genuinely matching +/// host on a non-standard port always failed). Stripping it from BOTH sides +/// rather than requiring an exact port match on either is deliberate there: +/// SSH and HTTPS remotes for the SAME repo commonly use DIFFERENT ports (22 +/// vs. a custom HTTPS port), and an ssh:// port is unrelated to `host_base`'s +/// own API port — the hostname alone is enough for THAT comparison. +/// +/// But an unconditional strip went too far (a fourth review round, Codex +/// review PR #159 repo.rs:3990): TWO DIFFERENT web-facing forge instances on +/// the SAME hostname but DIFFERENT HTTPS ports would also match, since both +/// sides' ports were discarded entirely rather than compared. The port DOES +/// matter when it is unambiguously comparable — an `https://`/`http://` +/// remote's own port against `host_base`'s (see [`parse_remote_host_and_path`]'s +/// `comparable_port` — `None` for ssh:///scp-style/bare, exactly the shapes the +/// third round's fix was protecting). +/// +/// A fifth review round (Codex review, PR #159 repo.rs:4035) caught that the +/// fourth round's fix went too far in the OTHER direction: it treated an +/// OMITTED web-transport port as "no signal," matching it against ANY +/// `host_base` port including a non-standard one. But `https://host/path` +/// with no port isn't ambiguous — by the same standard `gh`/a browser itself +/// uses, it unambiguously means port 443 (80 for `http://`). `host_base` is +/// symmetric: it is always extracted from a PR's OWN `https://`/`http://` web +/// URL (see `parse_pr_url`, which requires one of those two prefixes) and +/// never carries a scheme of its own, so an omitted `host_base` port means +/// the same standard 443 by that same convention — not "unspecified" either. +/// So both sides now fill in the scheme's standard port when it isn't +/// written (see `comparable_port`'s doc and this function's `expected_port` +/// below) and compare like any other explicit port: a `gitlab.corp` remote +/// with no port and a `host_base` of `gitlab.corp:8443` now correctly +/// DISAGREE (443 vs. 8443) rather than vacuously matching. The ONLY +/// remaining carve-out is for transports where the port concept doesn't +/// apply to this comparison at all — ssh://, scp-style, and bare +/// `host:path`, whose port (if any) denotes an SSH port unrelated to the +/// API's own HTTPS port — those keep skipping the port check entirely, +/// exactly as the third round's fix intended. +fn remote_matches_pr_host(remote_url: &str, host_base: &str, host_owner: &str, host_repo: &str) -> bool { + let host_base = host_base.trim(); + let host_owner = host_owner.trim(); + let host_repo = host_repo.trim(); + if host_base.is_empty() || host_owner.is_empty() || host_repo.is_empty() { + return false; + } + let Some(remote) = parse_remote_host_and_path(remote_url) else { + return false; + }; + let expected_host = host_base.split(':').next().unwrap_or(host_base).to_lowercase(); + let expected_path = format!("{}/{}", host_owner.to_lowercase(), host_repo.to_lowercase()); + if remote.host != expected_host || remote.path != expected_path { + return false; + } + // `host_base` never carries a scheme of its own, but (see this function's doc above) + // it is always extracted from a PR's OWN `https://`/`http://` web URL — so an omitted + // port here means the same standard port (443) an omitted port on an `https://` + // remote means, not "unspecified." + let expected_port = host_base + .split(':') + .nth(1) + .map(str::to_string) + .unwrap_or_else(|| "443".to_string()); + match &remote.comparable_port { + Some(remote_port) => *remote_port == expected_port, + // ssh/scp/bare: the port, if any, is an SSH port — not comparable to + // `host_base`'s web-facing port at all, so it never blocks a match. + None => true, + } +} + +/// Whether EVERY given `prs` genuinely reached the live default branch of the repo behind +/// `repo_id` (see `upstream_merge_state`) — `None` when the default branch itself can't even +/// be resolved, `Some(bool)` once it can. Only trusts the local clone when its CURRENT, LIVE +/// `origin` remote (`git remote get-url origin`, via `crate::git::remote_url`) matches the +/// PR's OWN recorded host identity ([`remote_matches_pr_host`]) — never for a repo whose +/// checkout could belong to a different repo than the PR being vetted. +/// +/// Checks the LIVE remote, not `repo_ref.remote_url` (a third independent +/// review round on PR #159 caught a TOCTOU gap in an earlier version that +/// checked the DB-recorded value: that string is captured once, at add/clone +/// time, and never updated — if a user later re-points the local checkout's +/// `origin` with `git remote set-url`, the stored value would still describe +/// the OLD target while `git ls-remote` right below queries the NEW one, +/// letting a re-pointed fork/mirror slip the check that was supposed to catch +/// exactly this). Reading the live remote and calling `ls-remote` inside the +/// SAME `spawn_blocking` closure closes the gap: both operations see the +/// identical state, with no window for it to change in between. +/// +/// A PR counts as landed when its recorded `base_ref` equals the live default branch's NAME — +/// nothing more clever than that. A prior round (Codex review, PR #159 repo.rs:3892) added a +/// commit-ancestry fallback here (`head_sha` an ANCESTOR of the live default branch's tip) to +/// close a real liveness gap: a repo renaming its default branch AFTER a producer's PR already +/// merged into the OLD name would otherwise block every consumer on a name that no longer +/// exists, forever. That fallback was REMOVED again one round later (Codex review, PR #159 +/// repo.rs:4088) once it turned out to be dead weight for this product's OWN dominant merge +/// path: `host::automerge::run_gh_merge` always squash-merges (see its doc — matches this +/// repo's own convention), and a squash merge's resulting commit on the default branch is a +/// NEW commit GitHub creates, not a descendant of `head_sha` at all — so for any producer PR +/// landed by Weft's own auto-merge (plausibly the single most common way an in-flight T4 +/// producer actually merges), the ancestry check could NEVER succeed, rename or not. What was +/// left after that was a mechanism that only theoretically helps the narrowing intersection of +/// "the repo was renamed" AND "the producer was merged by something other than this product's +/// own auto-merge" — real but doubly rare, and NOT worth the schema/API surface a genuine fix +/// would need (GitHub's actual merge-commit OID is a different field than `head_sha`, not +/// currently fetched or stored anywhere in this codebase — tracking it would mean a new +/// migration and a new GraphQL field for a shrinking payoff). Removing this only ever makes +/// MORE cases correctly fall through to `Pending` (never fewer) — a human who hits the +/// (rare, now-again-unclosed) renamed-branch case still sees the same honest "hasn't merged +/// yet" notice as any other Pending upstream, and can investigate; that is a visible, human- +/// recoverable liveness gap, not a silent one, and strictly safer than the alternative of +/// deepening this mechanism to chase an increasingly narrow edge case. +/// +/// Every `prs` row must share the SAME host identity as `prs[0]` (Codex review, PR #159, +/// post-round-6 pass: "verify every PR's target repository separately"): `register_pr_tool` +/// parses each PR's host/owner/repo straight from whatever URL it's given, with no check that +/// it matches the direction's own repo, so nothing stops two PRs on the SAME direction from +/// genuinely targeting DIFFERENT host repos. This function only ever vets ONE local checkout +/// (`repo_id`'s), so it can only meaningfully verify the identity it already checked +/// (`prs[0]`'s) — for any OTHER row, `base_ref == default_branch` is a coincidence of two +/// repos sharing a common branch name (`main`, `master`, …), not a real verification, and would +/// let a PR that never touched this repo at all count as "landed" by name alone. Rather than +/// resolving and verifying each row against its OWN target repo (which may have no local clone +/// registered in this workspace at all), this fails closed: a mixed-identity roster reads +/// `Unknown`, same as an unresolvable default branch — never silently trusts a coincidental +/// name match against the wrong repo's history. +/// +/// Deliberately does NOT fall back to `git::recorded_base_or_default`'s +/// offline "best guess" the way materializing a worktree does — that fallback +/// exists because a new worktree must branch off SOMETHING, but here a wrong +/// guess could optimistically release a merge that has not actually reached +/// the default branch. `None` on ANY failure (repo row gone, unreadable DB, +/// remote doesn't match, offline/no remote, mixed PR host identities) — the caller turns that +/// into `Unknown`, never `Merged`. +/// +/// Runs entirely off the async runtime (Codex review, PR #159 repo.rs:3821): +/// this is reached from the monitor's per-sweep-tick probe and both of +/// automerge's confirmation reads, so a slow/unreachable remote must not tie +/// up a Tokio worker the way `host::monitor::check_one`'s host probe already +/// avoids doing (same `spawn_blocking` discipline, no new pattern) — the +/// per-PR ancestry checks run inside the SAME blocking closure for the same +/// reason, rather than back on the async caller. +async fn all_prs_landed_on_live_default_branch( + db: &Db, + repo_id: i32, + host_base: &str, + host_owner: &str, + host_repo: &str, + prs: Vec, +) -> Option { + let repo_ref = get_repo(db, repo_id).await.ok().flatten()?; + let path = std::path::PathBuf::from(repo_ref.local_git_path); + let host_base = host_base.to_string(); + let host_owner = host_owner.to_string(); + let host_repo = host_repo.to_string(); + tokio::task::spawn_blocking(move || { + let live_remote = crate::git::remote_url(&path)?; + if !remote_matches_pr_host(&live_remote, &host_base, &host_owner, &host_repo) { + return None; + } + // Every row must share `prs[0]`'s (the only identity actually vetted above) host + // identity — this function only ever checks ONE local checkout, so a row targeting a + // genuinely different repo can only ever be "verified" by a coincidental base_ref name + // match, never a real one (see this function's doc). + if !prs.iter().all(|p| { + p.host_base.eq_ignore_ascii_case(&host_base) + && p.host_owner.eq_ignore_ascii_case(&host_owner) + && p.host_repo.eq_ignore_ascii_case(&host_repo) + }) { + return None; + } + let default_branch = crate::git::live_default_branch(&path)?; + Some(prs.iter().all(|p| p.base_ref.trim() == default_branch)) + }) + .await + .ok() + .flatten() +} + pub async fn list_open_pull_requests( db: &Db, max_probe_fail_count: i32, @@ -7418,6 +7945,1055 @@ mod tests { ); } + /// Fixture: a workspace + thread + two tasks, the second depending on the + /// first, mirroring a producer→consumer change set. `repo_path` need not + /// resolve to a real git repo for tests that only exercise PR/DB state + /// (Pending/Unknown) — pass a fake path (see [`ordered_pair`]) for those. + /// The "actually Merged" happy path needs a REAL repo with a resolvable + /// live default branch instead — pass a [`make_repo_with_origin`] clone. + /// + /// `remote_url` is no longer recorded on the `repo_ref` row on purpose: + /// since the repo.rs:3925 fix, `live_default_branch_for_repo` checks the + /// checkout's LIVE `origin` (via `crate::git::remote_url`), never the + /// DB-recorded value — so this fixture doesn't need to set one for the + /// default-branch tests to exercise the real code path. + async fn ordered_pair_at(db: &Db, repo_path: &str) -> (i32, i32) { + let ws = create_workspace(db, "ws_order").await.unwrap(); + let r = add_repo_ref(db, ws.id, "api", repo_path, "main", "", true) + .await + .unwrap(); + let t = create_thread(db, ws.id, "t", "feature", "claude").await.unwrap(); + let producer = create_direction(db, t.id, "producer", "claude", r.id, "r", "impl-only", "") + .await + .unwrap(); + let consumer = create_direction(db, t.id, "consumer", "claude", r.id, "r", "impl-only", "") + .await + .unwrap(); + set_direction_upstream(db, consumer.id, producer.id).await.unwrap(); + (producer.id, consumer.id) + } + + /// [`ordered_pair_at`] with a FAKE, non-existent repo path — fine for any + /// test that only exercises PR/DB state (Pending/Unknown), which never + /// touches git. `upstream_merge_state`'s "actually reached the default + /// branch" check needs a REAL repo instead (see `make_repo_with_origin`). + async fn ordered_pair(db: &Db) -> (i32, i32) { + ordered_pair_at(db, "/tmp/ordered-api").await + } + + fn sh(dir: &std::path::Path, args: &[&str]) { + let st = std::process::Command::new(args[0]) + .args(&args[1..]) + .current_dir(dir) + .status() + .unwrap(); + assert!(st.success(), "cmd {:?} failed", args); + } + + /// [`sh`], but captures and returns trimmed stdout — for the handful of setup steps that + /// need the command's OUTPUT (e.g. `git rev-parse HEAD`), not just its exit status. + fn sh_out(dir: &std::path::Path, args: &[&str]) -> String { + let out = std::process::Command::new(args[0]) + .args(&args[1..]) + .current_dir(dir) + .output() + .unwrap(); + assert!(out.status.success(), "cmd {:?} failed: {}", args, String::from_utf8_lossy(&out.stderr)); + String::from_utf8_lossy(&out.stdout).trim().to_string() + } + + /// A real, bare LOCAL git repo, at `//acme/api` so its + /// absolute path always ends in a clean two-segment "owner/repo" tail. + /// Returns (absolute repo path, host_owner, host_repo) — the identity + /// `remote_matches_pr_host` will accept for a `file://localhost/` + /// remote pointed at it (see [`file_url`]), DERIVED from wherever `root` + /// actually lives on disk (varies per test run) rather than a fixed + /// literal; `host_owner` is everything in the path before the final + /// segment (so it naturally absorbs the long temp-dir prefix, exactly + /// the same way a GitLab nested-subgroup path would — see + /// `remote_matches_pr_host`'s doc on that). + fn make_bare_repo(root: &std::path::Path, name: &str, default_branch: &str) -> (std::path::PathBuf, String, String) { + let repo = root.join(name).join("acme").join("api"); + std::fs::create_dir_all(&repo).unwrap(); + sh(&repo, &["git", "init", "-q"]); + sh(&repo, &["git", "config", "user.email", "t@t.t"]); + sh(&repo, &["git", "config", "user.name", "t"]); + std::fs::write(repo.join("README.md"), "# x\n").unwrap(); + sh(&repo, &["git", "add", "-A"]); + sh(&repo, &["git", "commit", "-q", "-m", "init"]); + sh(&repo, &["git", "branch", "-M", default_branch]); + let abs = repo.canonicalize().unwrap(); + let path_str = abs.to_str().unwrap().trim_start_matches('/').to_string(); + let (owner, name) = path_str.rsplit_once('/').expect("temp path has multiple segments"); + (abs, owner.to_string(), name.to_string()) + } + + /// A direct `file://localhost/` URL for a [`make_bare_repo`] + /// repo — resolvable by `git ls-remote`/`git clone` with NO `url.*. + /// insteadOf` rewrite involved, so `git remote get-url origin` on a clone + /// pointed at it reports this SAME string verbatim. A prior version of + /// these fixtures used `git remote set-url` to a fake `https://github.com/ + /// o/r` plus an `insteadOf` redirect to make a real local repo LOOK like a + /// clean GitHub identity; a FOURTH independent review round proved that + /// unsafe (PR #159 repo.rs:3925 → git.rs:1120): `insteadOf` is ALWAYS + /// applied to the actual fetch/`ls-remote` operation regardless of which + /// value `git::remote_url` happens to read, so a test built on that trick + /// could pass while hiding exactly the "vetted identity ≠ actually-queried + /// identity" mismatch this whole feature exists to catch — see + /// `crate::git::remote_url`'s doc. `file://localhost/...` needs no + /// rewrite at all: there is nothing to expand, so a test built on it + /// stays valid regardless of whether production reads the raw or the + /// `insteadOf`-expanded value. + fn file_url(abs_path: &std::path::Path) -> String { + format!("file://localhost{}", abs_path.to_str().unwrap()) + } + + /// A clone of a fresh [`make_bare_repo`] repo, with its `origin` set to + /// that repo's [`file_url`] — genuinely resolvable, no rewrite tricks. + /// Returns (clone path, host_base = "localhost", host_owner, host_repo) + /// — the identity `remote_matches_pr_host` will accept for this clone. + fn make_repo_with_origin( + root: &std::path::Path, + default_branch: &str, + ) -> (std::path::PathBuf, String, String, String) { + let (origin_abs, host_owner, host_repo) = make_bare_repo(root, "origin", default_branch); + let clone = root.join("clone"); + sh(root, &["git", "clone", "-q", &file_url(&origin_abs), clone.to_str().unwrap()]); + sh(&clone, &["git", "config", "user.email", "t@t.t"]); + sh(&clone, &["git", "config", "user.name", "t"]); + (clone, "localhost".to_string(), host_owner, host_repo) + } + + async fn register_open_pr(db: &Db, direction_id: i32, number: i32) -> pull_request::Model { + register_open_pr_at(db, direction_id, number, "github.com", "o", "r").await + } + + /// [`register_open_pr`] with an explicit host identity — for tests that + /// need it to match (or deliberately NOT match) a real [`make_repo_with_origin`] + /// clone's derived (host_base, host_owner, host_repo). + async fn register_open_pr_at( + db: &Db, + direction_id: i32, + number: i32, + host_base: &str, + host_owner: &str, + host_repo: &str, + ) -> pull_request::Model { + register_pull_request( + db, 1, direction_id, 0, "github", host_base, host_owner, host_repo, number, "", "", + ) + .await + .unwrap() + } + + /// A task with no edge is `None` — the state that lets a PR merge. Every + /// other answer in this function has to EARN that. + #[tokio::test] + async fn a_task_with_no_upstream_reports_none() { + let db = mem().await; + let ws = create_workspace(&db, "ws_solo").await.unwrap(); + let r = add_repo_ref(&db, ws.id, "api", "/tmp/solo-api", "main", "", true) + .await + .unwrap(); + let t = create_thread(&db, ws.id, "t", "feature", "claude").await.unwrap(); + let d = create_direction(&db, t.id, "solo", "claude", r.id, "r", "impl-only", "") + .await + .unwrap(); + + assert_eq!( + upstream_merge_state(&db, d.id).await, + crate::host::UpstreamStatus::None + ); + } + + /// THE FIX (Codex review, PR #159 repo.rs:3792): `pull_request.direction_id == 0` is a + /// pre-existing, legitimate, TESTED state — a PR the lead registers without a numeric + /// direction argument (see `bus::server::register_pr_tool_from_the_lead_falls_back_to_ + /// unset_direction`) is tracked exactly this way, and both real callers of this function + /// (`host::automerge`, `host::monitor`) pass `pr.direction_id` straight through as the + /// `direction_id` argument. Before this fix, `direction_id == 0` fell through to + /// `get_direction(db, 0)`, which always finds nothing and returns `Unknown` — forcing + /// every such PR's readiness to `Indeterminate` FOREVER, even though there is no direction + /// row here that ever could carry a dependency. `judge::merge_readiness` already proves + /// `None` behaves exactly like having no axis at all (judge.rs's + /// `no_upstream_is_indistinguishable_from_the_three_axis_judgement`), so the only gap + /// actually worth covering here is whether `upstream_merge_state` itself produces `None` + /// for this input, not `Unknown`. + #[tokio::test] + async fn a_pr_with_no_owning_direction_reports_none_not_unknown() { + let db = mem().await; + // Mirrors `register_pr_tool_from_the_lead_falls_back_to_unset_direction`'s DB state: + // `direction_id = 0`, no direction row exists at all. + register_open_pr(&db, 0, 1).await; + + assert_eq!( + upstream_merge_state(&db, 0).await, + crate::host::UpstreamStatus::None, + "a PR with no owning direction has no direction row that could ever carry a \ + dependency — this must never read Unknown (which would force Indeterminate \ + readiness forever)" + ); + } + + /// An upstream that has not opened a PR is PENDING, not Unknown: it + /// demonstrably has not merged anything. That is a fact, not missing + /// information, and the difference decides whether the consumer is + /// `Blocked` (correct) or `Indeterminate`. + #[tokio::test] + async fn an_upstream_with_no_pr_is_pending_not_unknown() { + let db = mem().await; + let (_producer, consumer) = ordered_pair(&db).await; + + match upstream_merge_state(&db, consumer).await { + crate::host::UpstreamStatus::Pending { what } => assert_eq!(what, "producer"), + other => panic!("expected pending, got {other:?}"), + } + } + + /// The ordering itself: open upstream PR → blocked; merged (AND landed on + /// the repo's real default branch) → free. Needs a REAL repo (not the fake + /// `/tmp/ordered-api` path `ordered_pair` uses elsewhere in this file) so + /// the default-branch check below has a genuine live default to resolve + /// against — see `make_repo_with_origin`. + #[tokio::test] + async fn an_upstream_pr_releases_the_consumer_only_once_merged() { + let db = mem().await; + let root = std::env::temp_dir() + .join(format!("weft-upstream-merged-releases-once-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(&root).unwrap(); + let (clone_path, host_base, host_owner, host_repo) = make_repo_with_origin(&root, "main"); + let (producer, consumer) = ordered_pair_at(&db, clone_path.to_str().unwrap()).await; + let pr = register_open_pr_at(&db, producer, 1, &host_base, &host_owner, &host_repo).await; + + assert!( + matches!( + upstream_merge_state(&db, consumer).await, + crate::host::UpstreamStatus::Pending { .. } + ), + "an OPEN upstream PR must hold the consumer" + ); + + let snapshot = crate::host::PrSnapshot { + head_sha: "abc".into(), + base_ref: "main".into(), + url: String::new(), + title: String::new(), + lifecycle: crate::host::PrLifecycle::Merged, + ci: crate::host::CiStatus::Passing, + review: crate::host::ReviewStatus::Approved, + conflict: crate::host::ConflictStatus::Clean, + }; + apply_pull_request_snapshot(&db, pr.id, &snapshot, &crate::host::MergeReadiness::Ready) + .await + .unwrap(); + + assert_eq!( + upstream_merge_state(&db, consumer).await, + crate::host::UpstreamStatus::Merged, + "base_ref \"main\" matches the repo's real live default branch, so this must still \ + read Merged under the new default-branch check" + ); + + let _ = std::fs::remove_dir_all(&root); + } + + /// THE FIX (Codex P1, PR #159 review): a `merged` lifecycle alone is not + /// enough. A PR that merged into a staging/release/stacked-feature branch + /// has landed on THAT PR's base, not on the repo's default branch — so the + /// producer change a submodule-pinned/version-bumped consumer actually + /// needs is still absent from what other repos consume. Before this fix, + /// `upstream_merge_state` ignored the stored `base_ref` entirely and would + /// have returned `Merged` here, releasing the consumer's auto-merge on a + /// change that never reached `main`. + #[tokio::test] + async fn a_merged_pr_targeting_a_non_default_branch_does_not_release_the_consumer() { + let db = mem().await; + let root = std::env::temp_dir() + .join(format!("weft-upstream-merged-non-default-base-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(&root).unwrap(); + let (clone_path, host_base, host_owner, host_repo) = make_repo_with_origin(&root, "main"); + let (producer, consumer) = ordered_pair_at(&db, clone_path.to_str().unwrap()).await; + let pr = register_open_pr_at(&db, producer, 1, &host_base, &host_owner, &host_repo).await; + + // "Merged" — but into `release`, not the repo's real default `main`. + let snapshot = crate::host::PrSnapshot { + head_sha: "abc".into(), + base_ref: "release".into(), + url: String::new(), + title: String::new(), + lifecycle: crate::host::PrLifecycle::Merged, + ci: crate::host::CiStatus::Passing, + review: crate::host::ReviewStatus::Approved, + conflict: crate::host::ConflictStatus::Clean, + }; + apply_pull_request_snapshot(&db, pr.id, &snapshot, &crate::host::MergeReadiness::Ready) + .await + .unwrap(); + + assert!( + matches!( + upstream_merge_state(&db, consumer).await, + crate::host::UpstreamStatus::Pending { .. } + ), + "a PR merged into a non-default branch must NOT release the consumer — the producer \ + change has not reached main, which is what a cross-repo consumer actually needs" + ); + + let _ = std::fs::remove_dir_all(&root); + } + + /// A repo can rename its default branch (`main` → `trunk`) AFTER a producer's PR already + /// merged into the OLD name. A prior round (Codex review, PR #159 repo.rs:3892) added a + /// commit-ancestry fallback specifically to still release the consumer in this situation — + /// and a LATER round (Codex review, PR #159 repo.rs:4088) removed it again: this product's + /// own `host::automerge::run_gh_merge` always squash-merges, so for any producer PR landed + /// by Weft's own auto-merge the ancestry check could never succeed anyway (a squash's + /// resulting commit is a NEW one GitHub creates, not a descendant of `head_sha`) — the + /// fallback was dead weight for this product's dominant merge path, not worth the + /// migration a real fix would need for a shrinking payoff (see `all_prs_landed_on_live_ + /// default_branch`'s doc for the full reasoning). This test pins the CURRENT, reverted + /// behavior: even when the underlying commit genuinely IS reachable from the renamed + /// branch's tip (a TRUE rename via `git branch -m`, preserving history, not a fixture + /// trick), a bare NAME comparison against the new name must NOT optimistically read + /// `Merged` — this is now a plain, visible, human-recoverable `Pending`, same as any other + /// base_ref mismatch. + #[tokio::test] + async fn a_merged_pr_whose_base_branch_was_later_renamed_stays_pending_not_merged() { + let db = mem().await; + let root = std::env::temp_dir() + .join(format!("weft-upstream-renamed-default-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(&root).unwrap(); + let (origin_abs, host_owner, host_repo) = make_bare_repo(&root, "origin", "main"); + let clone_path = root.join("clone"); + sh(&root, &["git", "clone", "-q", &file_url(&origin_abs), clone_path.to_str().unwrap()]); + sh(&clone_path, &["git", "config", "user.email", "t@t.t"]); + sh(&clone_path, &["git", "config", "user.name", "t"]); + let merged_sha = sh_out(&clone_path, &["git", "rev-parse", "HEAD"]); + + // The origin repo renames its default branch AFTER the PR above already merged into + // "main". The local checkout fetches — proving this stays Pending even when the + // renamed branch's ancestry IS resolvable locally, not merely when it isn't. + sh(&origin_abs, &["git", "branch", "-m", "main", "trunk"]); + sh(&clone_path, &["git", "fetch", "-q", "origin"]); + + let (producer, consumer) = ordered_pair_at(&db, clone_path.to_str().unwrap()).await; + let pr = register_open_pr_at(&db, producer, 1, "localhost", &host_owner, &host_repo).await; + let snapshot = crate::host::PrSnapshot { + head_sha: merged_sha, + base_ref: "main".into(), // the OLD name — a real, historical snapshot, never rewritten + url: String::new(), + title: String::new(), + lifecycle: crate::host::PrLifecycle::Merged, + ci: crate::host::CiStatus::Passing, + review: crate::host::ReviewStatus::Approved, + conflict: crate::host::ConflictStatus::Clean, + }; + apply_pull_request_snapshot(&db, pr.id, &snapshot, &crate::host::MergeReadiness::Ready) + .await + .unwrap(); + + assert!( + matches!( + upstream_merge_state(&db, consumer).await, + crate::host::UpstreamStatus::Pending { .. } + ), + "a renamed default branch is a plain base_ref mismatch now — no ancestry fallback \ + to optimistically rescue it, even though the commit genuinely is reachable from \ + the renamed branch's tip" + ); + + let _ = std::fs::remove_dir_all(&root); + } + + /// FAIL-SAFE (Codex P1, PR #159 review): when the upstream repo's live + /// default branch cannot be resolved at all (offline, no remote, the repo + /// row's path is gone) this must NEVER optimistically fall back to a best + /// guess and release the merge — it must read `Unknown`, the same honest + /// "can't tell" every other failure branch in this function returns. + /// Reuses the ordinary fake-path `ordered_pair` fixture: `/tmp/ordered-api` + /// is not a real git repo, so `live_default_branch` genuinely fails here. + #[tokio::test] + async fn an_unresolvable_default_branch_is_unknown_never_optimistically_merged() { + let db = mem().await; + let (producer, consumer) = ordered_pair(&db).await; + let pr = register_open_pr(&db, producer, 1).await; + let snapshot = crate::host::PrSnapshot { + head_sha: "abc".into(), + base_ref: "main".into(), + url: String::new(), + title: String::new(), + lifecycle: crate::host::PrLifecycle::Merged, + ci: crate::host::CiStatus::Passing, + review: crate::host::ReviewStatus::Approved, + conflict: crate::host::ConflictStatus::Clean, + }; + apply_pull_request_snapshot(&db, pr.id, &snapshot, &crate::host::MergeReadiness::Ready) + .await + .unwrap(); + + match upstream_merge_state(&db, consumer).await { + crate::host::UpstreamStatus::Unknown { .. } => {} + other => panic!( + "an unresolvable default branch must be Unknown, never an optimistic verdict: {other:?}" + ), + } + } + + /// THE FIX (Codex review, PR #159 repo.rs:3794 and, on the LIVE-remote follow-up, + /// repo.rs:3925): the local clone's `origin` need not be the repo the PR actually targets + /// (a fork checkout, a re-pointed remote, a mirror). This sets up the WORST case — the + /// local clone's live default branch happens to COINCIDE with the merged PR's `base_ref` + /// ("main" == "main") — while the clone's LIVE `origin` remote does NOT match the PR's + /// recorded host identity. Before the first fix this coincidence would have read `Merged`; + /// it must fail safe to `Unknown`. + #[tokio::test] + async fn a_local_clone_that_does_not_match_the_prs_host_identity_is_unknown_not_merged() { + let db = mem().await; + let root = std::env::temp_dir() + .join(format!("weft-upstream-merged-mismatched-host-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(&root).unwrap(); + // A REAL clone (host_base "localhost", host_owner/host_repo derived from the filesystem + // path) registered against the PR's UNRELATED, hardcoded identity (host_base + // "github.com", host_owner "o", host_repo "r" — register_open_pr's default) — a genuine + // mismatch, not a manufactured one: no fixture trick is needed to make the clone's live + // origin disagree with the PR's recorded host identity, it simply does. + let (clone_path, _host_base, _host_owner, _host_repo) = make_repo_with_origin(&root, "main"); + let (producer, consumer) = ordered_pair_at(&db, clone_path.to_str().unwrap()).await; + let pr = register_open_pr(&db, producer, 1).await; // host_owner="o", host_repo="r", host_base="github.com" + + let snapshot = crate::host::PrSnapshot { + head_sha: "abc".into(), + base_ref: "main".into(), // COINCIDENTALLY equal to the local clone's live default. + url: String::new(), + title: String::new(), + lifecycle: crate::host::PrLifecycle::Merged, + ci: crate::host::CiStatus::Passing, + review: crate::host::ReviewStatus::Approved, + conflict: crate::host::ConflictStatus::Clean, + }; + apply_pull_request_snapshot(&db, pr.id, &snapshot, &crate::host::MergeReadiness::Ready) + .await + .unwrap(); + + match upstream_merge_state(&db, consumer).await { + crate::host::UpstreamStatus::Unknown { .. } => {} + other => panic!( + "a local clone whose recorded remote does NOT match the PR's own host identity \ + must never be trusted to vet its default branch, even on a coincidental \ + base_ref match: got {other:?}" + ), + } + + let _ = std::fs::remove_dir_all(&root); + } + + /// THE FIX (Codex review, post-round-6 pass: "verify every PR's target repository + /// separately"): an upstream direction with TWO merged PR rows where only the FIRST row's + /// host identity actually matches the local checkout — `register_pr_tool` parses each PR's + /// host/owner/repo straight from whatever URL it's given, with no cross-check against the + /// direction's own repo, so nothing prevents this. Before this fix, the SECOND row's + /// `base_ref` happening to equal "main" (a name shared by countless unrelated repos) was + /// enough to make this return `Merged`, even though that row has nothing to do with the + /// repo the local checkout actually verified — a coincidental name match standing in for a + /// real verification. + #[tokio::test] + async fn a_merged_pr_targeting_an_unrelated_repo_is_unknown_not_merged_even_on_a_coincidental_base_ref() { + let db = mem().await; + let root = std::env::temp_dir() + .join(format!("weft-upstream-mixed-pr-identity-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(&root).unwrap(); + let (clone_path, host_base, host_owner, host_repo) = make_repo_with_origin(&root, "main"); + let (producer, consumer) = ordered_pair_at(&db, clone_path.to_str().unwrap()).await; + // #1 genuinely matches the local checkout's identity. + let matching = register_open_pr_at(&db, producer, 1, &host_base, &host_owner, &host_repo).await; + // #2 targets a COMPLETELY unrelated repo — registered against the SAME direction, which + // nothing today prevents. + let unrelated = register_open_pr_at(&db, producer, 2, "localhost", "totally-unrelated-owner", "unrelated-repo").await; + + let matching_snapshot = crate::host::PrSnapshot { + head_sha: "abc".into(), + base_ref: "main".into(), + url: String::new(), + title: String::new(), + lifecycle: crate::host::PrLifecycle::Merged, + ci: crate::host::CiStatus::Passing, + review: crate::host::ReviewStatus::Approved, + conflict: crate::host::ConflictStatus::Clean, + }; + apply_pull_request_snapshot(&db, matching.id, &matching_snapshot, &crate::host::MergeReadiness::Ready) + .await + .unwrap(); + // The unrelated repo's PR merged into a branch that COINCIDENTALLY shares the real + // checkout's default branch NAME ("main") — the exact false-positive shape this fix + // closes. + let unrelated_snapshot = crate::host::PrSnapshot { + head_sha: "def".into(), + base_ref: "main".into(), + url: String::new(), + title: String::new(), + lifecycle: crate::host::PrLifecycle::Merged, + ci: crate::host::CiStatus::Passing, + review: crate::host::ReviewStatus::Approved, + conflict: crate::host::ConflictStatus::Clean, + }; + apply_pull_request_snapshot(&db, unrelated.id, &unrelated_snapshot, &crate::host::MergeReadiness::Ready) + .await + .unwrap(); + + match upstream_merge_state(&db, consumer).await { + crate::host::UpstreamStatus::Unknown { .. } => {} + other => panic!( + "a roster of PRs spanning more than one host identity must never resolve via a \ + coincidental base_ref name match against the ONE checkout this function \ + actually verified: got {other:?}" + ), + } + + let _ = std::fs::remove_dir_all(&root); + } + + /// THE FIX (Codex review, PR #159 repo.rs:3925): proves `live_default_branch_for_repo` + /// reads the checkout's LIVE `origin` remote on EVERY call, not a value captured once and + /// cached/trusted from before. Same `repo_ref` DB row throughout — the ONLY thing that + /// changes between the two assertions is the checkout's actual `git remote set-url`, i.e. + /// exactly the re-pointed-remote scenario the review described. If this read the + /// DB-recorded `remote_url` (or any other stale snapshot) instead of the live one, the + /// second assertion would still see the FIRST (matching) answer. + #[tokio::test] + async fn upstream_merge_state_reflects_the_remote_being_repointed_after_registration() { + let db = mem().await; + let root = std::env::temp_dir() + .join(format!("weft-upstream-repointed-remote-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(&root).unwrap(); + // live origin is a real, resolvable file:// repo whose derived identity is registered below + let (clone_path, host_base, host_owner, host_repo) = make_repo_with_origin(&root, "main"); + let (producer, consumer) = ordered_pair_at(&db, clone_path.to_str().unwrap()).await; + let pr = register_open_pr_at(&db, producer, 1, &host_base, &host_owner, &host_repo).await; + let snapshot = crate::host::PrSnapshot { + head_sha: "abc".into(), + base_ref: "main".into(), + url: String::new(), + title: String::new(), + lifecycle: crate::host::PrLifecycle::Merged, + ci: crate::host::CiStatus::Passing, + review: crate::host::ReviewStatus::Approved, + conflict: crate::host::ConflictStatus::Clean, + }; + apply_pull_request_snapshot(&db, pr.id, &snapshot, &crate::host::MergeReadiness::Ready) + .await + .unwrap(); + + assert_eq!( + upstream_merge_state(&db, consumer).await, + crate::host::UpstreamStatus::Merged, + "precondition: the live remote matches the PR's host identity" + ); + + // Re-point the SAME checkout's origin to an unrelated repo — no DB write at all, only + // `git remote set-url` on disk. `repo_ref.remote_url` (if this read it) would still say + // whatever was captured at `add_repo_ref` time. + sh(&clone_path, &["git", "remote", "set-url", "origin", "https://github.com/someone-else/other-repo"]); + + match upstream_merge_state(&db, consumer).await { + crate::host::UpstreamStatus::Unknown { .. } => {} + other => panic!( + "after the checkout's origin was re-pointed away from the PR's real host repo, \ + the SAME DB row must stop reading Merged — got {other:?} (a stale/cached remote \ + check would still show the old, no-longer-true answer)" + ), + } + + let _ = std::fs::remove_dir_all(&root); + } + + /// DIRECT tests of `remote_matches_pr_host` (a third independent review round on PR #159 + /// caught a boundary-anchoring bug: a prior `.contains()`/`.ends_with()` version was only + /// covered INDIRECTLY through `upstream_merge_state`, and that fixture used + /// `"unrelated-owner"`/`"unrelated-repo"` — a "completely unrelated" negative that cannot + /// exercise a NEAR-MISS boundary collision, giving false confidence). Every reject case + /// below is a NEAR miss on purpose: differs from the expected identity by exactly the kind + /// of thing a naive substring/suffix check would let through. + mod remote_matches_pr_host_tests { + use super::*; + + /// A fork commonly keeps the upstream's repo name, and GitHub owner/org names allow + /// hyphens — `other-acme` legitimately ending in `acme` is not a hypothetical. + #[test] + fn rejects_an_owner_that_merely_ends_with_the_expected_owner() { + assert!(!remote_matches_pr_host("https://github.com/other-acme/api.git", "github.com", "acme", "api")); + assert!(!remote_matches_pr_host("git@github.com:other-acme/api.git", "github.com", "acme", "api")); + } + + /// The other direction: `acme-corp` starts with `acme` but is a different owner. + #[test] + fn rejects_an_owner_that_merely_starts_with_the_expected_owner() { + assert!(!remote_matches_pr_host("https://github.com/acme-corp/api.git", "github.com", "acme", "api")); + } + + /// `api-v2` shares a prefix with `api` but is a different repo. + #[test] + fn rejects_a_repo_that_merely_shares_a_prefix_with_the_expected_repo() { + assert!(!remote_matches_pr_host("https://github.com/acme/api-v2.git", "github.com", "acme", "api")); + } + + /// `mygitlab.com` CONTAINS `gitlab.com` as a substring but is a different host. + #[test] + fn rejects_a_host_that_merely_contains_the_expected_host_as_a_substring() { + assert!(!remote_matches_pr_host("https://mygitlab.com/o/r.git", "gitlab.com", "o", "r")); + } + + /// `gitlab.com.evil.tld` has `gitlab.com` as a PREFIX (a classic domain-spoofing + /// shape) but is a different host entirely. + #[test] + fn rejects_a_host_that_merely_has_the_expected_host_as_a_prefix() { + assert!(!remote_matches_pr_host("https://gitlab.com.evil.tld/o/r.git", "gitlab.com", "o", "r")); + } + + /// A GitLab nested-subgroup analogue of the owner-suffix collision: `host_owner` can + /// itself contain `/` (see that field's doc on `pull_request::Model`), and an + /// "evil-group/subgroup" must not match a bare "group/subgroup" owner just because the + /// tail segments agree — same root cause as the GitHub case, fixed together even + /// though no GitLab backend is wired up yet to reach this in production. + #[test] + fn rejects_a_nested_subgroup_path_that_merely_ends_with_the_expected_path() { + assert!(!remote_matches_pr_host( + "https://gitlab.com/evil-group/subgroup/repo.git", + "gitlab.com", + "group/subgroup", + "repo" + )); + } + + /// A genuine nested subgroup DOES match — the fix must not overcorrect into rejecting + /// every multi-segment owner. + #[test] + fn accepts_a_genuine_nested_subgroup_path() { + assert!(remote_matches_pr_host( + "https://gitlab.com/group/subgroup/repo.git", + "gitlab.com", + "group/subgroup", + "repo" + )); + } + + /// The full accepted matrix for a genuinely matching identity: HTTPS/SSH/scp-style, + /// with and without `.git`, with a port, mixed case, and a trailing slash all resolve + /// to the SAME repo and must all accept. + #[test] + fn accepts_every_remote_shape_for_a_genuinely_matching_identity() { + let cases = [ + "https://github.com/acme/api", + "https://github.com/acme/api.git", + "https://github.com/acme/api.git/", + "https://github.com/acme/api/", + "git@github.com:acme/api.git", + "git@github.com:acme/api", + "ssh://git@github.com/acme/api.git", + "ssh://git@github.com:22/acme/api.git", + "https://GitHub.com/Acme/API.git", + "HTTPS://GITHUB.COM/ACME/API", + ]; + for remote in cases { + assert!( + remote_matches_pr_host(remote, "github.com", "acme", "api"), + "expected a match for {remote:?}" + ); + } + } + + /// THE FIX (Codex review, PR #159 repo.rs:3924): a self-hosted install's `host_base` + /// can read like `git.internal:8443` (the API's port). The remote's OWN port + /// (SSH's 22 here, deliberately DIFFERENT from the API port) must not prevent a + /// match — both sides compare host WITHOUT a port, since SSH and HTTPS access to the + /// SAME repo routinely use different ports. + #[test] + fn accepts_a_self_hosted_host_base_with_an_explicit_port_regardless_of_the_remotes_own_port() { + assert!(remote_matches_pr_host( + "https://git.internal:8443/acme/api.git", + "git.internal:8443", + "acme", + "api" + )); + assert!(remote_matches_pr_host( + "git@git.internal:acme/api.git", // SSH, no port at all in the remote + "git.internal:8443", + "acme", + "api" + )); + assert!(remote_matches_pr_host( + "ssh://git@git.internal:22/acme/api.git", // SSH on port 22, API on 8443 + "git.internal:8443", + "acme", + "api" + )); + } + + /// The port-stripping above must not turn into a substring/prefix hole of its own — + /// a DIFFERENT host that merely shares a port suffix must still be rejected. + #[test] + fn rejects_a_different_host_that_happens_to_share_a_port() { + assert!(!remote_matches_pr_host( + "https://evil.example:8443/acme/api.git", + "git.internal:8443", + "acme", + "api" + )); + } + + /// THE FIX (Codex review, PR #159 repo.rs:3990): the round-3 port fix above stripped + /// BOTH sides' ports UNCONDITIONALLY to let a legitimate ssh-vs-https port difference + /// through — but that also let two DIFFERENT https forge instances on the SAME + /// hostname match each other just because their ports both got discarded. A near-miss + /// on purpose, matching this test module's own convention: same host, same owner/repo + /// path — only the HTTPS port differs — not two unrelated hosts (already covered by + /// `rejects_a_different_host_that_happens_to_share_a_port`, a different axis). + #[test] + fn rejects_a_different_https_port_on_the_same_hostname_even_with_matching_owner_and_repo() { + assert!(!remote_matches_pr_host( + "https://gitlab.corp:9443/acme/api.git", + "gitlab.corp:8443", + "acme", + "api" + )); + } + + /// SUPERSEDED (Codex review, PR #159 repo.rs:4035 — a fifth review round): this test + /// used to assert the OPPOSITE (`accepts_an_https_remote_with_no_explicit_port_even_when_host_base_records_one`) + /// on the theory that an omitted port is "not distinguishable from... didn't spell out + /// the port." That theory was wrong: an omitted port on an `https://` URL is NOT + /// ambiguous, it unambiguously means the standard port 443 — so a remote silently on + /// 443 and a `host_base` explicitly on 8443 are a genuine, detectable port mismatch, + /// not a "maybe the same install" case. See the near-miss pair below for the corrected + /// behavior at both standard (443) and non-standard (8443) ports, from both sides. + #[test] + fn rejects_an_https_remote_with_no_explicit_port_against_a_host_base_on_a_non_standard_port() { + assert!(!remote_matches_pr_host( + "https://gitlab.corp/acme/api.git", + "gitlab.corp:8443", + "acme", + "api" + )); + } + + /// THE FIX (Codex review, PR #159 repo.rs:4035): an omitted HTTPS port means the + /// standard port 443 by definition (the same convention `gh`/a browser use), not "no + /// signal" — so it MATCHES an explicit `:443` on the other side, from either + /// direction (remote omits/host_base spells it out, and vice versa). `host_base` is + /// always derived from a PR's own `https://`/`http://` URL (`parse_pr_url`), so the + /// same standard-port convention applies to its own omitted port too. + #[test] + fn an_omitted_https_port_matches_an_explicit_standard_443_from_either_side() { + assert!(remote_matches_pr_host( + "https://gitlab.corp/acme/api.git", + "gitlab.corp:443", + "acme", + "api" + )); + assert!(remote_matches_pr_host( + "https://gitlab.corp:443/acme/api.git", + "gitlab.corp", + "acme", + "api" + )); + } + + /// The flip side of the fix above: since an omitted port unambiguously means 443, it + /// must NOT match an explicit NON-standard port (8443) on the other side, from either + /// direction. This is the exact near-miss the previous "absent port is not a signal" + /// design let through. + #[test] + fn an_omitted_https_port_does_not_match_an_explicit_non_standard_port_from_either_side() { + assert!(!remote_matches_pr_host( + "https://gitlab.corp/acme/api.git", + "gitlab.corp:8443", + "acme", + "api" + )); + assert!(!remote_matches_pr_host( + "https://gitlab.corp:8443/acme/api.git", + "gitlab.corp", + "acme", + "api" + )); + } + + /// Scheme-awareness check: `http://` (not `https://`) must default its omitted port to + /// the plain-HTTP standard (80), not silently reuse 443 — a copy-paste mutation that + /// hardcoded 443 for both match arms would make this test the only one to catch it, + /// since every other case in this module uses `https://`. + #[test] + fn an_omitted_http_port_is_the_standard_80_not_443() { + assert!(remote_matches_pr_host( + "http://gitlab.corp/acme/api.git", + "gitlab.corp:80", + "acme", + "api" + )); + assert!(!remote_matches_pr_host( + "http://gitlab.corp/acme/api.git", + "gitlab.corp:443", + "acme", + "api" + )); + } + + /// Empty/missing identity components must never match (an unset `remote_url` — the + /// common case before this feature existed — must fail safe, not vacuously match). + #[test] + fn rejects_empty_inputs() { + assert!(!remote_matches_pr_host("", "github.com", "acme", "api")); + assert!(!remote_matches_pr_host("https://github.com/acme/api", "", "acme", "api")); + assert!(!remote_matches_pr_host("https://github.com/acme/api", "github.com", "", "api")); + assert!(!remote_matches_pr_host("https://github.com/acme/api", "github.com", "acme", "")); + } + + /// A malformed/unrecognized remote shape (no scheme, no colon) must fail safe. + #[test] + fn rejects_an_unparseable_remote() { + assert!(!remote_matches_pr_host("not-a-remote-url", "github.com", "acme", "api")); + } + } + + /// TWO upstream PRs, one merged: still pending. A task is not done because + /// one of its PRs landed. + #[tokio::test] + async fn one_merged_pr_does_not_release_an_upstream_with_two() { + let db = mem().await; + let (producer, consumer) = ordered_pair(&db).await; + let first = register_open_pr(&db, producer, 1).await; + let _second = register_open_pr(&db, producer, 2).await; + + let snapshot = crate::host::PrSnapshot { + head_sha: "abc".into(), + base_ref: "main".into(), + url: String::new(), + title: String::new(), + lifecycle: crate::host::PrLifecycle::Merged, + ci: crate::host::CiStatus::Passing, + review: crate::host::ReviewStatus::Approved, + conflict: crate::host::ConflictStatus::Clean, + }; + apply_pull_request_snapshot(&db, first.id, &snapshot, &crate::host::MergeReadiness::Ready) + .await + .unwrap(); + + assert!( + matches!( + upstream_merge_state(&db, consumer).await, + crate::host::UpstreamStatus::Pending { .. } + ), + "every upstream PR must land before the consumer is free" + ); + } + + /// THE FIX (Codex review, PR #159 repo.rs:3850): reverts repo.rs:3793's "ignore a closed + /// row once something else merged" from an earlier round of this same review chain. That + /// version assumed a closed-without-merging row was always a superseded/abandoned retry of + /// the SAME work — but `one_merged_pr_does_not_release_an_upstream_with_two` (this file) + /// already establishes a single upstream can legitimately own multiple INDEPENDENTLY + /// required PRs, and this store has no persisted "supersedes" relationship to tell a + /// replacement apart from a second required PR that got abandoned: {PR #1 closed, PR #2 + /// merged} is IDENTICAL data either way. This test proves the axis now fails closed on + /// that ambiguity — a closed-without-merging row blocks the SAME as an open one, exactly + /// mirroring `one_merged_pr_does_not_release_an_upstream_with_two`'s existing open-PR case + /// (see `upstream_merge_state`'s doc for the full reasoning and the deferred proper fix). + #[tokio::test] + async fn a_closed_without_merging_pr_still_blocks_the_consumer_even_with_a_later_merge() { + let db = mem().await; + let root = std::env::temp_dir() + .join(format!("weft-upstream-closed-sibling-pr-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(&root).unwrap(); + let (clone_path, host_base, host_owner, host_repo) = make_repo_with_origin(&root, "main"); + let (producer, consumer) = ordered_pair_at(&db, clone_path.to_str().unwrap()).await; + let closed = register_open_pr_at(&db, producer, 1, &host_base, &host_owner, &host_repo).await; + let merged = register_open_pr_at(&db, producer, 2, &host_base, &host_owner, &host_repo).await; + + // #1 closes WITHOUT merging — could be a superseded retry, or could be a second + // independently required PR that got abandoned. This store cannot tell which. + let closed_snapshot = crate::host::PrSnapshot { + head_sha: "abc".into(), + base_ref: "main".into(), + url: String::new(), + title: String::new(), + lifecycle: crate::host::PrLifecycle::Closed, + ci: crate::host::CiStatus::Passing, + review: crate::host::ReviewStatus::Approved, + conflict: crate::host::ConflictStatus::Clean, + }; + apply_pull_request_snapshot(&db, closed.id, &closed_snapshot, &crate::host::MergeReadiness::Ready) + .await + .unwrap(); + assert!( + matches!( + upstream_merge_state(&db, consumer).await, + crate::host::UpstreamStatus::Pending { .. } + ), + "precondition: nothing has merged yet" + ); + + // #2 genuinely merges into the real default branch — #1 stays closed-without-merging. + let merged_snapshot = crate::host::PrSnapshot { + head_sha: "def".into(), + base_ref: "main".into(), + url: String::new(), + title: String::new(), + lifecycle: crate::host::PrLifecycle::Merged, + ci: crate::host::CiStatus::Passing, + review: crate::host::ReviewStatus::Approved, + conflict: crate::host::ConflictStatus::Clean, + }; + apply_pull_request_snapshot(&db, merged.id, &merged_snapshot, &crate::host::MergeReadiness::Ready) + .await + .unwrap(); + + assert!( + matches!( + upstream_merge_state(&db, consumer).await, + crate::host::UpstreamStatus::Pending { .. } + ), + "a closed-without-merging sibling PR must keep blocking the consumer even after \ + another PR on the same upstream merged — this store cannot tell a superseded \ + retry apart from a second required PR that never landed, so it must fail closed \ + exactly like the still-open case already does" + ); + + let _ = std::fs::remove_dir_all(&root); + } + + /// A dangling edge must NOT read as "no upstream". `None` would release the + /// merge on the strength of a row we could not find. + #[tokio::test] + async fn a_dangling_upstream_edge_is_unknown_never_none() { + let db = mem().await; + let ws = create_workspace(&db, "ws_dangle").await.unwrap(); + let r = add_repo_ref(&db, ws.id, "api", "/tmp/dangle-api", "main", "", true) + .await + .unwrap(); + let t = create_thread(&db, ws.id, "t", "feature", "claude").await.unwrap(); + let d = create_direction(&db, t.id, "consumer", "claude", r.id, "r", "impl-only", "") + .await + .unwrap(); + set_direction_upstream(&db, d.id, 4242).await.unwrap(); + + match upstream_merge_state(&db, d.id).await { + crate::host::UpstreamStatus::Unknown { reason } => { + assert!(reason.contains("4242"), "reason names the missing id: {reason}"); + } + other => panic!("a dangling edge must be Unknown, got {other:?}"), + } + } + + /// A task row that is genuinely gone (deleted, or an id that was never valid) is Unknown — + /// the monitor resolves ordering by direction, and "no such task" is not evidence of "no + /// dependency". Uses `4242` (this file's established "definitely nonexistent" id, same as + /// `a_dangling_upstream_edge_is_unknown_never_none`), NOT `0`: an earlier version of this + /// test used `0` itself and called it "legacy `direction_id == 0`", conflating a genuinely + /// missing row with the separate, legitimate, TESTED "no owning direction" convention a + /// lead-registered PR uses (see `a_pr_with_no_owning_direction_reports_none_not_unknown` + /// and `bus::server::register_pr_tool_from_the_lead_falls_back_to_unset_direction`) — the + /// exact mistake Codex review (PR #159 repo.rs:3792) caught. + #[tokio::test] + async fn an_unknown_task_is_unknown_not_none() { + let db = mem().await; + match upstream_merge_state(&db, 4242).await { + crate::host::UpstreamStatus::Unknown { .. } => {} + other => panic!("expected unknown, got {other:?}"), + } + } + + /// Fixture: `count` bare tasks (same workspace/thread/repo, no upstream + /// edges yet) for the cycle-rejection tests below to wire up. + async fn bare_directions(db: &Db, count: usize) -> Vec { + let ws = create_workspace(db, "ws_cycle").await.unwrap(); + let r = add_repo_ref(db, ws.id, "api", "/tmp/cycle-api", "main", "", true) + .await + .unwrap(); + let t = create_thread(db, ws.id, "t", "feature", "claude").await.unwrap(); + let mut ids = Vec::with_capacity(count); + for i in 0..count { + let d = create_direction(db, t.id, &format!("d{i}"), "claude", r.id, "r", "impl-only", "") + .await + .unwrap(); + ids.push(d.id); + } + ids + } + + /// THE FIX (Codex P2, PR #159 review): the old check only rejected a + /// SELF edge (A depends on A). A→B, then B→A closes a 2-cycle through two + /// perfectly legal single-upstream writes — the old code accepted both. + /// Once closed, EACH side's `upstream_merge_state` reads the OTHER as + /// permanently `Pending`: neither PR can ever become mergeable, with no + /// reason that names the actual problem. + #[tokio::test] + async fn set_direction_upstream_rejects_a_two_cycle() { + let db = mem().await; + let ids = bare_directions(&db, 2).await; + let (a, b) = (ids[0], ids[1]); + + set_direction_upstream(&db, a, b).await.unwrap(); + let err = set_direction_upstream(&db, b, a) + .await + .expect_err("B depending on A must be rejected — it would close A→B→A"); + assert!( + err.to_string().contains("cycle"), + "error should name the problem: {err}" + ); + + // The rejected write must not have landed: B still has no upstream. + let b_dir = get_direction(&db, b).await.unwrap().unwrap(); + assert_eq!(b_dir.depends_on_direction_id, 0, "the rejected edge must not persist"); + } + + /// A single-upstream-per-task graph can still contain an arbitrarily LONG + /// cycle (A→B→C→A): each hop only needs its own one upstream slot to point + /// back around. The rejection must walk the whole chain, not just the + /// immediate edge. + #[tokio::test] + async fn set_direction_upstream_rejects_a_longer_cycle() { + let db = mem().await; + let ids = bare_directions(&db, 3).await; + let (a, b, c) = (ids[0], ids[1], ids[2]); + + set_direction_upstream(&db, a, b).await.unwrap(); // A -> B + set_direction_upstream(&db, b, c).await.unwrap(); // B -> C + let err = set_direction_upstream(&db, c, a) + .await + .expect_err("C depending on A must be rejected — it would close A→B→C→A"); + assert!( + err.to_string().contains("cycle"), + "error should name the problem: {err}" + ); + + let c_dir = get_direction(&db, c).await.unwrap().unwrap(); + assert_eq!(c_dir.depends_on_direction_id, 0, "the rejected edge must not persist"); + } + + /// Two INDEPENDENT tasks pointing at the SAME upstream is a fan-in, not a + /// cycle — must stay allowed. A single-upstream-per-task model can't + /// express fan-OUT (one producer, many consumers) any other way. + #[tokio::test] + async fn set_direction_upstream_allows_two_consumers_of_the_same_producer() { + let db = mem().await; + let ids = bare_directions(&db, 3).await; + let (producer, consumer_1, consumer_2) = (ids[0], ids[1], ids[2]); + + set_direction_upstream(&db, consumer_1, producer).await.unwrap(); + set_direction_upstream(&db, consumer_2, producer).await.unwrap(); + + let c1 = get_direction(&db, consumer_1).await.unwrap().unwrap(); + let c2 = get_direction(&db, consumer_2).await.unwrap().unwrap(); + assert_eq!(c1.depends_on_direction_id, producer); + assert_eq!(c2.depends_on_direction_id, producer); + } + #[tokio::test] async fn next_turn_id_increments_from_last_row() { let db = Db::connect("sqlite::memory:").await.unwrap(); @@ -7556,7 +9132,12 @@ mod tests { conflict: crate::host::ConflictStatus::Clean, }; let readiness = - crate::host::judge::merge_readiness(&snapshot.ci, &snapshot.review, &snapshot.conflict); + crate::host::judge::merge_readiness( + &snapshot.ci, + &snapshot.review, + &snapshot.conflict, + &host::UpstreamStatus::None, + ); apply_pull_request_snapshot(&db, broken.id, &snapshot, &readiness).await.unwrap(); let listed = list_open_pull_requests(&db, 2).await.unwrap(); assert_eq!(listed.len(), 2, "a success must reset the failure streak and rejoin the sweep"); @@ -7624,7 +9205,12 @@ mod tests { review: crate::host::ReviewStatus::Approved, conflict: crate::host::ConflictStatus::Clean, }; - let readiness = crate::host::judge::merge_readiness(&snapshot.ci, &snapshot.review, &snapshot.conflict); + let readiness = crate::host::judge::merge_readiness( + &snapshot.ci, + &snapshot.review, + &snapshot.conflict, + &host::UpstreamStatus::None, + ); apply_pull_request_snapshot(&db, pr.id, &snapshot, &readiness) .await .unwrap(); @@ -7656,7 +9242,12 @@ mod tests { review: crate::host::ReviewStatus::Approved, conflict: crate::host::ConflictStatus::Clean, }; - let readiness = crate::host::judge::merge_readiness(&snapshot.ci, &snapshot.review, &snapshot.conflict); + let readiness = crate::host::judge::merge_readiness( + &snapshot.ci, + &snapshot.review, + &snapshot.conflict, + &host::UpstreamStatus::None, + ); apply_pull_request_snapshot(&db, pr.id, &snapshot, &readiness) .await .unwrap(); diff --git a/src/board/NeedsRows.tsx b/src/board/NeedsRows.tsx index ee34d46a..241f6e38 100644 --- a/src/board/NeedsRows.tsx +++ b/src/board/NeedsRows.tsx @@ -6,6 +6,7 @@ import { ArrowUpRight, Check, GitBranch, + GitMerge, Layers, RefreshCw, Send, @@ -22,6 +23,7 @@ import { routeLabelKey, routeReasonKey, routeToolName } from "../lib/engineRouti import { needsRoutingControlOf } from "./needsRoutingControl"; import { noticeTokenKey } from "../lib/noticeTokens"; import { ASK_CARD_TONE, ASK_DOT_TONE, NOTICE_FOOTER_VIEW } from "./needsCardView"; +import { dependsOnLabel } from "./dependsOnView"; export function WriteTriggerRow({ item }: { item: WriteTrigger }) { const { approveWriteTrigger, denyWriteTrigger, selectThread, defaultTool, installedTools } = @@ -36,6 +38,7 @@ export function WriteTriggerRow({ item }: { item: WriteTrigger }) { defaultTool, }); const context = [item.thread_title, item.name].filter(Boolean).join(" · "); + const dependsOn = dependsOnLabel(item.depends_on); async function act(fn: () => Promise) { if (busy) return; @@ -87,15 +90,26 @@ export function WriteTriggerRow({ item }: { item: WriteTrigger }) { {routingControl.showBlockedStatus && {t("scope.engineRouteBlockedHint")}} )} - {item.base_branch && ( -
- - - {item.base_branch} - + {(item.base_branch || dependsOn) && ( +
+ {item.base_branch && ( + + + {item.base_branch} + + )} + {dependsOn && ( + + + {t("scope.dependsOn", { name: dependsOn })} + + )}
)} {/* Issue #103: approving this ALSO propagates a read-only auto-allow to diff --git a/src/board/ScopeReview.tsx b/src/board/ScopeReview.tsx index 322ff778..8acef53c 100644 --- a/src/board/ScopeReview.tsx +++ b/src/board/ScopeReview.tsx @@ -1,7 +1,7 @@ import { useEffect, useMemo, useRef, useState } from "react"; import { motion } from "motion/react"; import { useTranslation } from "react-i18next"; -import { AlertTriangle, Check, GitBranch, Sparkles, X } from "lucide-react"; +import { AlertTriangle, Check, GitBranch, GitMerge, Sparkles, X } from "lucide-react"; import { useStore } from "../state/store"; import type { ResolvedDirection } from "../lib/types"; import { Button } from "../components/ui/Button"; @@ -12,6 +12,7 @@ import { spawnableToolsOf } from "../lib/toolStatus.ts"; import { PlanSummary } from "../session/blocks/PlanSummary"; import { latestPlanCard, type ParsedPlanCard } from "../session/planCard"; import { SCOPE_CONFIRM_DISABLED, scopeConfirmStateOf } from "./scopeConfirmState"; +import { dependsOnLabel } from "./dependsOnView"; // One sub-task lane: exactly one write repo per direction (scope rework). The // old read/write/none taxonomy is gone from this gate — every lane here is a @@ -296,6 +297,7 @@ function ScopeLaneRow({ lane, index, confirming }: { lane: ScopeLane; index: num // Per-proposal version: changes on EVERY re-proposal (R50-2). Threaded into BaseBranchField so a // re-propose with the same name/repo/base still resets a dirty (unblurred) base edit. const proposalVersion = proposal?.created_at ?? ""; + const dependsOn = dependsOnLabel(lane.direction.depends_on); return ( )} + {dependsOn && ( +
+ + {t("scope.dependsOn", { name: dependsOn })} +
+ )}
{ + assert.equal(dependsOnLabel("producer"), "producer"); + assert.equal(dependsOnLabel(" producer "), "producer", "surrounding whitespace is trimmed"); +}); + +test("an empty depends_on returns null — no dependency to show", () => { + assert.equal(dependsOnLabel(""), null); +}); + +test("a whitespace-only depends_on returns null too", () => { + // The backend's own edge resolution trims before deciding "is this empty" + // (planner::record_upstream_edges reads `lane.depends_on.trim()`) — the UI must not show a + // phantom dependency for a value that resolves to "no upstream" server-side. + assert.equal(dependsOnLabel(" "), null); +}); + +test("undefined or null (a defensive fallback, e.g. stale cached state) returns null", () => { + assert.equal(dependsOnLabel(undefined), null); + assert.equal(dependsOnLabel(null), null); +}); + +test("a name containing internal whitespace is preserved, only the ends are trimmed", () => { + assert.equal(dependsOnLabel(" the producer task "), "the producer task"); +});