Repository navigation
✨ dependencies: hold a PR on blocked until the PRs it depends on merge, across repos - #17689
Conversation
… on merge, across repos A PR body's `Depends-on: #N` / `owner/repo#N` lines were read only by the dashboard's critical path; nothing stopped a merge lane from landing the PR before its prerequisite, so a person had to hold it by hand. - The blocked sweep now puts the existing `blocked` label on an open, non-draft PR whose Depends-on names an open pull request (same repo or another), with one `<!-- hive:blocked-on pr=owner/repo#N -->` comment per open target, written by the same helper a relay's blocked verdict uses (postBlockedOnPRMarker, refactored out of RecordVerdictBlocker). - It lifts its own `blocked` with the existing `<!-- hive:unblocked -->` note once every marked target merged or closed (the note names one that closed without merging) or is no longer listed. - A person's `blocked` is never placed over or lifted; a person removing the hive's `blocked` is not overridden for the same refs (newest unlabeled event by a non-App actor after an App labeled event); a new open target re-arms it. Unreadable targets place nothing, logged once. - Writes go through the write-back gate as gate_comment. - The shared lane label gate (labelBlockReason) refuses `blocked` as label:blocked, so the self-authored/trusted-bot, trusted-author, contributor and queued lanes, the merge train and the native queue (withdraw) all refuse it; the queued lane re-checks after refetch; the dashboard merge verdict and human-merge shortcuts refuse it too. - Stage: label:blocked is a held gate like label:hold; the route's gate reason reads "blocked on owner/repo#N", and the activity monitor PR row carries blocked_on_pr and the "Blocked on PR #N" caption. - hive-open-issue comment --blocked-on-pr accepts owner/repo#n. Docs: blocked reference (Issues and PRs), stage labels, operator reference, hive-open-issue, dashboard critical path, notifications. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BQYnPGFJx81c8JXR3o1sg1 Signed-off-by: Danathar <Danathar@users.noreply.github.com>
kubestellar-prow
Bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubestellar-prow
Bot
added
the
size/XXL
|
@Danathar this PR is back in your court ( Review asks:
Reply here or push a new head to this branch (same sign-off); CI and the review swarm re-run automatically and the PR returns to the normal path. Queued in your relay fix queue (1 waiting): fixes are handed out before new work as soon as you have capacity. |
|
Triage of the red checks on
Fork PR, so the hive is not pushing to this branch. 🐝 Hive Agent: — hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
![hivecommons-hive[bot]](https://avatars.githubusercontent.com/in/3568013?s=60&v=4){kind=small}
There was a problem hiding this comment.
The red contract-tests lane is a test-fixture problem, not a logic problem, and it is a one-line fix.
Why CI is red. bin/hive-open-issue.sh:352 lists acme as a documentation-placeholder owner and exits 2 for any --repo acme/... that is not in $HIVE_REPOS (hive-open-issue.sh:369). The three new cases in bin/test_hive_open_issue.sh:250, :264 and :278 all pass --repo "acme/widget", so the first one exits 2 before writing a request and the test script's set -e aborts the run — which is why the job log stops after the --parent section's "Second child" with Process completed with exit code 2 and no FAIL: line. I reproduced it by running the head's script directly: --repo acme/widget 7 --body x --blocked-on-pr acme/gadget#42 --dry-run → "--repo 'acme/widget' is a documentation placeholder", rc=2. Switching those three to hiveorg/widgets (the owner every other case in the file uses) should fix it. The --blocked-on-pr acme/gadget#42 target is fine — it is only matched by the regex at :287, not by the repo gate. build-and-test is red only because it requires the contract lane; NOTICE matches the module graph failed on a DNS timeout to gopkg.in from the runner, which is infrastructure.
Correctness read of the sweep (src/pkg/github/pr_blocked_depends_on.go, head f221525) — I read it against the existing latestLabelEventByApp / postBlockedOnPRMarker / unblockItem paths and did not find a defect:
- Ownership test at
:206-209(latestKind == "labeled" && appIsLatest) matches whatlatestLabelEventByApp(pr_level_hold.go:290-294) now derives from the samelabelEventHistory, so the sweep and the hard-suppress reconcile agree on whoseblockedit is. - Lift path keeps the block on a target read error (
:286-289) and lifts only when every marked target is merged/closed/unlisted; a hiveblockedwith no markers is left alone (:270). That is the conservative direction. - Placement on a person-released PR (
:244-249) re-arms only whenfresh(open, unmarked) is non-empty; a body re-listing an already-marked target re-arms too, as the comment at:235-237says. Consistent with the body's "respect people" rules. - Per-pass
cachekeyed by lowercasedowner/repo#nmeans oneIssues.Getper target per sweep across all repos.blockedPRUnreadableis process-lifetime and unbounded, but grows one entry per (PR, bad ref) pair, so it is bounded by the open-PR population.
Body and diff agree on everything I checked. The change is large (32 files) and touches every merge lane's prefilter, so a lane owner should still read labelBlockReason and the native-queue withdrawal; the only thing I can say with certainty is that the one visible failure is the fixture owner above.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Hive status — updated in place; edits do not notify. Full history is in the Activity Monitor. Review findings from CI triage for the red checks at
I verified the script change itself does what the test intends: Not this PR's fault:
Scope note: this comment is CI triage only. I have not reviewed the ~1,000 lines of Go in Review findings from Security perspective, read at head f221525. 1. The sweep lifts a hive
Before this PR, Suggested fix: only treat a marker as droppable-by-body when it is this sweep's own comment; otherwise fall through to the target read so the block holds until N settles. --- a/src/pkg/github/pr_blocked_depends_on.go
+++ b/src/pkg/github/pr_blocked_depends_on.go
@@ func (c *Client) reconcileBlockedPR(
- marked := blockedPRMarkedTargets(comments)
+ marked, viaDependsOn := blockedPRMarkedTargets(comments)
@@
- c.liftOrKeepBlockedPR(ctx, repo, pr, labels, deps, marked, fresh, cache, res)
+ c.liftOrKeepBlockedPR(ctx, repo, pr, labels, deps, marked, viaDependsOn, fresh, cache, res)
@@ func (c *Client) liftOrKeepBlockedPR(
- for _, t := range marked {
- if !listed[t.key()] {
+ for _, t := range marked {
+ // Only a marker this sweep wrote from a Depends-on line may be
+ // dropped by editing the body; a --blocked-on-pr or verdict
+ // marker holds until its target settles.
+ if !listed[t.key()] && viaDependsOn[t.key()] {
clauses = append(clauses, t.ref()+" is no longer listed under `Depends-on:`")
continue
}
@@ func blockedPRMarkedTargets(
-func blockedPRMarkedTargets(comments []*gh.IssueComment) []blockedPRTarget {
+func blockedPRMarkedTargets(comments []*gh.IssueComment) ([]blockedPRTarget, map[string]bool) {
seen := map[string]bool{}
+ viaDependsOn := map[string]bool{}
var out []blockedPRTarget
for _, cm := range comments {
for _, m := range blockedOnMarkerRE.FindAllStringSubmatch(cm.GetBody(), -1) {
...
t := blockedPRTarget{repo: m[1], number: n}
+ if strings.Contains(cm.GetBody(), "says `Depends-on: ") {
+ viaDependsOn[t.key()] = true
+ }
if !seen[t.key()] {(or carry an explicit 2. Low — untrusted body drives unbounded cross-repo reads and a growing process map —
Review findings from Style / maintainability (read at head f221525): the sweep mirrors
Nit: Review findings from Correctness review at head f221525. The lift rule treats every hive-written blocked-on marker on a PR as a Depends-on placement, so a
But this sweep is not the only hive writer of that marker + label on a pull request: Suggested fix: only let "dropped from the body" count as settled for markers this sweep wrote; for any other hive marker, read the target and keep the block while it is open. --- a/src/pkg/github/pr_blocked_depends_on.go
+++ b/src/pkg/github/pr_blocked_depends_on.go
@@ func (c *Client) liftOrKeepBlockedPR(
- for _, t := range marked {
- if !listed[t.key()] {
+ for _, t := range marked {
+ // Only a marker this sweep wrote came from a Depends-on line, so only
+ // that one lifts when the line is dropped; another hive writer's
+ // marker (the watcher's --blocked-on-pr) holds until its PR settles.
+ if !listed[t.key()] && t.dependsOn {
clauses = append(clauses, t.ref()+" is no longer listed under `Depends-on:`")
continue
}
@@ type blockedPRTarget struct {
repo string
number int
+ // dependsOn: the marker comment was written by this sweep.
+ dependsOn bool
}
@@ func blockedPRMarkedTargets(comments []*gh.IssueComment) []blockedPRTarget {
- t := blockedPRTarget{repo: m[1], number: n}
+ t := blockedPRTarget{repo: m[1], number: n, dependsOn: strings.Contains(cm.GetBody(), "says `Depends-on: ")}( Advisory (not blocking): |
Review follow-ups (below
|
hive-open-issue refuses acme/widget as a documentation placeholder, so the new section exited 2 before its checks ran. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BQYnPGFJx81c8JXR3o1sg1 Signed-off-by: Danathar <Danathar@users.noreply.github.com>
Summary
A PR whose body says
Depends-on: #NorDepends-on: owner/repo#Nnow waits for those pull requests. The blocked sweep puts the existingblockedlabel on it, with the existing<!-- hive:blocked-on pr=owner/repo#N -->marker (one per open target). Every merge lane refuses ablockedPR. The sweep lifts the label with the existing<!-- hive:unblocked -->note once the targets have merged or closed, or are dropped from the body. People win: the hive never lifts ablockeda person set, and never puts its own back after a person removed it unless a new dependency appears. This is the design agreed per maintainer discussion: reuseblockedand the blocked-on marker, no new label or marker family.Why
Some PRs must land after another PR, often in another repository. A common case is tests that need an upstream change. Agents already write
Depends-on:lines (the default policies ask for them), but only the dashboard's critical path read them. A merge lane could land the PR first, so a person had to hold it and remember to release it.How
Place. For each open, non-draft PR in a configured repo, the sweep reads the
Depends-on:targets (ParseDependsOn). When at least one is an open pull request:blocked;The marker comment is written by
postBlockedOnPRMarker, the PR branch ofRecordVerdictBlockermoved into a shared helper. So the marker is byte-identical to the one a relay's blocked verdict writes. These targets place nothing:issue=form for PRs);Respect people.
blockedis the hive's only when its newest label event is alabeledby the App bot. The hard-suppress reconcile uses the same rule. Any otherblockedis a person's: the sweep never places over it and never lifts it.unlabeledby a non-App actor, and the App labeled it earlier. In that case the sweep does not re-applyblockedfor targets already marked on the PR. A newly listed open target re-arms it.Lift. The blocked sweep now covers PRs too. It lifts the hive's own
blockedonce every marked target has merged or closed, or is no longer listed underDepends-on:, so a stale marker never holds the PR forever. The note:Unblocked: owner/repo#N …, so the existing duplicate-note check still applies;Cross-repo targets are read by the
owner/repoin the marker.Lanes. The shared label gate
labelBlockReasonnow refusesblockedaslabel:blocked. It is checked after the hold family, so a PR with both still reads as held. Every lane's prefilter goes through that gate, so ablockedPR is refused by:exclude_labelssays;label:blockedwithdraws the App's own entry.Outside the lanes:
automergeBlockLabelReason) refuses it;delegatedHumanMergeGated) refuse it.I chose the shared gate over adding
blockedtoDefaultTrustedAuthorExcludeLabels. That default only covers the trusted-author lane: the self-authored, trusted-bot and queued lanes have no exclude list. It would also leave out hives that set an explicit list. One side effect: a contributor PR wearingblockednow reportslabel:blockedinstead ofexcluded-label:blocked. Two contributor-lane tests were updated for that.Stage and parity.
label:blockedjoinslabel:holdand sentinel as a gate, not a stage (#17327). The PR's stage is its head's real progress. The route carries the gate reasonblocked on owner/repo#N, taken from the marker the sweep last read, or "theblockedlabel is set" when no marker has been read. So the 090 entry comment reads "Held: blocked on …". On the activity monitor, the PR row now getsblocked_on_prand the caption clauseBlocked on PR #N — lifts when it merges, the same fields a blocked issue already has. The MCPstage_tracker_readtool reads the same/api/stage-trackerpayload, so the API test covers both. Inprs_by_bandablockedPR was already in the waiting band (the default waiting labels includeblocked).hive-open-issue.
comment --blocked-on-prnow takesowner/repo#nas well as<n>. The request carriesblocked_on_pr_repo, and the watcher reads that repository.Defaults and write-back gate
The behaviour is on for every hive, with no new setting. The writes, the label with its marker comments and the lift with its note, go through the write-back gate as
gate_comment. That is the kindRecordVerdictBlockerand the marker-less blocked ask already use for the same marker and label, so no new kind or kind-policy parity was needed. The sweep needs the App bot login to tell its own label from a person's, and does nothing without one.Interplay with the human-merge-path holds (#17609, #17622)
blockedis not a hold source.OtherHoldSourcesForPRdoes not count it (a test pins this), so the human-merge-pathholdstill lifts when its own reason goes, while the lanes keep refusing the PR onblocked. Each label is lifted by its own sweep.applyHumanMergePathFenceRoutereads only hold-class labels, so it is unchanged; theblockedgate rides the route as a separate gate.Docs
labels-and-control-signals/reference.md:blockedapplies to Issues and PRs, with the PR placement and lift rules.stage-labels.md: the gate row now listslabel:blocked.operator-reference/README.md: the exclude-list rows sayblockedis always refused.hive-open-issue.md: the cross-repo form.dashboard.md: the Depends-on critical path.notifications.md:postBlockedOnPRMarkerreplacesRecordVerdictBlockeras the protocol_command site.changelog.d/added-blocked-pr-depends-on.md.Tests
None were run locally (AGENTS.md); CI is the verdict.
gofmt -landbash -nare clean. No JS orstatic/change, soindex.htmlis untouched.pr_blocked_depends_on_test.go:OtherHoldSourcesForPRignoresblocked;blocked_on_pr_repo.automerge_blocked_gate_test.go: every lane prefilter refusesblocked, the merge train does not seat it, the native queue withdraws it, and the gate clause names the target.derive_pr_blocked_gate_test.go:label:blockedclassifies exactly likelabel:holdon every lane.stage_tracker_blocked_pr_test.go: the PR row'sblocked_on_prand caption, and the delegated human-merge gate.merge_eligibility_blocked_test.go: the dashboard merge verdict.bin/test_hive_open_issue.sh:--blocked-on-pr owner/repo#n.Ratchets: no raw comment emitter was added; the marker comment goes through the shared helper. New write calls carry
// writeback:gatedbehind a gate check, so the direct-writes baseline is unchanged.Out of scope and follow-ups
blockedPR. Repositories that merge through Tide should addblockedto Tide'smissingLabels.blockedyet. The row shows theblockedpill and caption instead.🤖 Generated with Claude Code
https://claude.ai/code/session_01BQYnPGFJx81c8JXR3o1sg1