feat(pr): order cross-repo merges — an upstream task must land first (#110)

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.

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<crate::engine_routing::RouteDecision>,
+    /// 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,
             });
         }
     }

src-tauri/src/git.rs

@@ -1099,6 +1099,30 @@ pub fn current_branch(repo: &Path) -> Result<String> {
 /// 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.<x>.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<String> {
     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");