feat(pr): order cross-repo merges — an upstream task must land first (#110) - #159
feat(pr): order cross-repo merges — an upstream task must land first (#110)#159SoloJiang wants to merge 1 commit into
Conversation
…110) The piece that makes a change set cross-repo rather than several unrelated PRs: a consumer 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. Built as a FOURTH axis of `judge::merge_readiness` rather than a separate gate. That function is already the single source of truth every consumer reads — the monitor's Needs-you notice, the auto-merge gate, the UI — so ordering reaches all of them by construction instead of being re-derived per caller, and it inherits the rule that matters most: any axis `Unknown` yields `Indeterminate`, never a verdict. `UpstreamStatus` distinguishes four states, and two of the distinctions are load-bearing: - `None` (no edge) vs `Unknown` (edge exists, state unresolvable). `None` releases the merge; a missing task row or unreadable DB must never borrow that. Every failure path in `upstream_merge_state` returns `Unknown`. - An upstream with NO registered PR is `Pending`, not `Unknown` — it demonstrably has not merged anything. That is a fact, not missing information, and it decides `Blocked` (correct) vs `Indeterminate`. `Merged` also requires that the upstream have at least one PR and that ALL of them landed: a task is not done because one of its two PRs merged. Ordering is resolved per sweep, never cached on the PR row — the upstream's lifecycle moves on its own schedule, and a stale copy would either hold a mergeable PR back or release one early. All three computation sites (monitor sweep, and both of auto-merge's confirmation reads) resolve it fresh. M0046 adds ONE column rather than a join table: one upstream per task is what a producer→consumer pair needs, and it is the minimum that answers whether ordered cross-repo delivery is worth building out. A real topological sequencer wants many-to-many and its own table — noted in the migration and the type's docs. `set_direction_upstream` refuses a self-edge (unresolvable by construction); longer cycles are impossible with a single upstream and are explicitly left to that future graph. Verification - `cargo test`: 1536 lib passed (was 1525), all integration suites, 0 failed - `pnpm build` clean; `pnpm test` 155 passed; `git diff --check` clean - Mutation-tested all six guards — un-blocking a pending upstream, treating Unknown as clear, dropping the axis from the fold, reporting a dangling edge as None, releasing on one of several PRs, and calling a PR-less upstream merged — every one goes RED Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4609c23ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return Ok(()); | ||
| }; | ||
| let mut a: direction::ActiveModel = row.into(); | ||
| a.depends_on_direction_id = Set(upstream_id); |
There was a problem hiding this comment.
When a normal cross-repository plan creates its producer and consumer directions, every creation path leaves depends_on_direction_id at its default of 0: a repository-wide search finds this setter called only by its tests, and neither the planner nor a command accepts an upstream direction. Consequently upstream_merge_state returns None for all production-created tasks, so the new auto-merge gate can still merge the consumer first; wire this write into the plan-confirmation or direction-creation flow.
Useful? React with 👍 / 👎.
| UpstreamStatus::Unknown { reason } => Some(format!("上游任务状态未知({reason})")), | ||
| UpstreamStatus::Pending { what } => Some(format!("上游任务「{what}」还没合并")), |
There was a problem hiding this comment.
When an upstream is pending or unknown, these Rust literals are emitted directly as user-facing Chinese text rather than being selected from the active UI locale, so the new state cannot be translated and English users receive Chinese wording. Return a structured reason and render it through keys in both src/i18n/en.ts and src/i18n/zh.ts.
AGENTS.md reference: AGENTS.md:L14-L17
Useful? React with 👍 / 👎.
| }; | ||
| // Merged only when the upstream has at least one PR and EVERY one of them | ||
| // has landed: a task that opened two PRs is not done because one merged. | ||
| if !prs.is_empty() && prs.iter().all(|p| p.lifecycle == "merged") { |
There was a problem hiding this comment.
When an upstream PR targets a staging, release, or stacked feature branch, a merged lifecycle only proves that it landed on that PR's base; it does not prove that the producer change reached the repository's default branch as required by this feature. Because this condition ignores the stored base_ref, it releases the consumer and permits auto-merge even though the referenced producer change may still be absent from the default branch; compare each merged PR's base with the upstream repository's recorded/live default before returning Merged.
Useful? React with 👍 / 👎.
| if upstream_id == direction_id { | ||
| anyhow::bail!("a task cannot depend on itself"); | ||
| } |
There was a problem hiding this comment.
When two or more tasks are assigned cyclically—for example, first setting A to depend on B and then B to depend on A—this self-edge-only check accepts both writes. Each task's upstream_merge_state then remains Pending on the other, so neither PR can ever become mergeable; a single-upstream graph can still contain arbitrarily long cycles, so validate the upstream chain before persisting the edge.
Useful? React with 👍 / 👎.
2 participants
Why
This is the piece that makes a change set cross-repo rather than several unrelated PRs. A consumer 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.
#110has an auto-merge gate, a monitor and a truly-mergeable judgement, but nothing that knows one task must wait for another. Ordering is the part that distinguishes weft from a single-repo tool, and it was the thinnest code in the feature.Design
Built as a fourth axis of
judge::merge_readiness, not a separate gate.That function is already the single source of truth every consumer reads — the monitor's Needs-you notice, the auto-merge gate, the UI — so ordering reaches all of them by construction instead of being re-derived per caller. It also inherits the rule that matters most: any axis
UnknownyieldsIndeterminate, never a verdict.UpstreamStatushas four states, and two of the distinctions are load-bearing:None(no edge) vsUnknown(edge exists, state unresolvable).Nonereleases the merge; a missing task row or an unreadable DB must never borrow that. Every failure path inupstream_merge_statereturnsUnknown.Pending, notUnknown. It demonstrably has not merged anything — a fact, not missing information — and this decidesBlocked(correct) vsIndeterminate.Mergedadditionally requires that the upstream have at least one PR and that all of them landed: a task is not done because one of its two PRs merged.Ordering is resolved per sweep, never cached on the PR row — the upstream's lifecycle moves on its own schedule, and a stale copy would either hold a mergeable PR back or release one early. All three computation sites (the monitor sweep, and both of auto-merge's confirmation reads) resolve it fresh.
Scope
M0046adds one column, not a join table. One upstream per task is what a producer→consumer pair needs and is the minimum that answers whether ordered cross-repo delivery is worth building out. A real topological sequencer wants many-to-many and its own table — recorded in the migration doc and onUpstreamStatus.set_direction_upstreamrefuses a self-edge (unresolvable by construction, and it would render as a task no human can ever merge). Longer cycles cannot be expressed with a single upstream and are explicitly left to that future graph.No UI in this PR — the axis flows into the existing notice text, so a blocked consumer already reads
上游任务「…」还没合并in Needs-you.Verification
Mutation-tested all six guards — un-blocking a pending upstream, treating
Unknownas clear, dropping the axis from the fold, reporting a dangling edge asNone, releasing on one of several PRs, and calling a PR-less upstream merged. Every one goes RED.Next
Validating this end-to-end on a real pair (
RedditFind/ai-chat-kit→RedditFind/find, coupled by submodule) is the actual question — whether dependency-ordered delivery feels like leverage. That needs a real change spanning both repos and is deliberately not part of this PR.🤖 Generated with Claude Code